fix(row-cache): clamp negative start in View.rowidsInRange#942
Conversation
IdentityView.rowidsInRange(-3, 2) returns nonexistent negative rowids ([-3,-2,-1,0,1]) while SortView/FilterView return an empty slice for the same range. rowidsInRange already clamps end and handles inverted ranges defensively, so the negative-start divergence is an oversight, not a contract choice. This becomes a live bug once 7b wires rowidsInRange into the AG-Grid datasource at a boundary window near row 0. Test only — fix follows. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Clamp start to 0 in IdentityView/SortView/FilterView.rowidsInRange so all three view kinds agree and never index out of range. rowidsInRange already clamped end and handled inverted ranges; this completes that defensive contract on the start side. The positionAt "callers must clamp" contract is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 46fb43b854
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ]; | ||
|
|
||
| test.each(cases)("%s — rowidsInRange(-3, 2) clamps start to 0", (_label, v) => { | ||
| expect(v.rowidsInRange(-3, 2)).toStrictEqual(v.rowidsInRange(0, 2)); |
There was a problem hiding this comment.
This new contract is added without the corresponding View.rowidsInRange implementation change, so the Views.test.ts suite becomes red: IdentityView.rowidsInRange(-3, 2) currently returns [-3, -2, -1, 0, 1], while SortView/FilterView return an empty slice for this range, so this assertion fails for all three cases. Until Views.ts clamps start to 0, any CI job that runs the package Jest tests will fail on this commit.
Useful? React with 👍 / 👎.
📦 TestPyPI package publishedpip install --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo==0.15.2.dev28125040636or with uv: uv pip install --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo==0.15.2.dev28125040636MCP server for Claude Codeclaude mcp add buckaroo-table -- uvx --from "buckaroo[mcp]==0.15.2.dev28125040636" --index-strategy unsafe-best-match --index-url https://test.pypi.org/simple/ --extra-index-url https://pypi.org/simple/ buckaroo-table📖 Docs preview🎨 Storybook preview |
1 participant
Summary
View.rowidsInRange(start, end)is a defensive range accessor — it already clampsendatlength()and returns[]for an inverted range. It does not clamp a negativestart, and the three view kinds diverge there:IdentityView(20).rowidsInRange(-3, 2)→[-3, -2, -1, 0, 1](five nonexistent negative rowids)SortView(...).rowidsInRange(-3, 2)/FilterView(...)→[]IdentityViewsilently emits rowids theRowStorecan never satisfy; the array-backed views drop the window entirely. The renderer composesview at position k → rowid → RowStore.get(rowid), so a boundary window near row 0 (which the AG-Grid datasource will produce onceRowCacheis wired in at phase 7b) reads back garbage from one view kind and nothing from the others.This is an oversight in the range accessor, not the
positionAt"callers must clamp" contract (which is unchanged). The fix clampsstartto 0 in all threerowidsInRangeimplementations so they agree and never index out of range.TDD
Per repo convention, this PR opens with the failing test alone (
46fb43b8) so CI is seen red before the fix lands; the fix commit follows on the same branch. Found while evaluating the phase 1–7a row-cache code (PR #719).Test plan
rowidsInRange(-3, 2))🤖 Generated with Claude Code