Uh oh!
There was an error while loading. Please reload this page.
refactor(tables): row selection as discriminated union - #4466
Conversation
…ected flag Master checkbox detached from gutter selection state when rows or columns changed after Cmd+A: the predicate matched normalizedSelection bounds exactly (endRow === rows.length-1, endCol === displayColumns.length-1), so any post-selection growth flipped it false while the cell-range overlay still painted every row checked. Replace the structural two-branch predicate with an explicit allRowsSelected flag plus a uniform set-membership check. handleSelectAllRows sets the flag in O(1); handleRowToggle materializes checkedRows when toggling out of "all" mode. Bulk-op read sites (delete, copy, cut, selectedRowCount) honor the flag. Decouple gutter checkbox from cell-range drag: dragging cells no longer fills gutter checkboxes — they reflect explicit row-selection intent only, matching Sheets/Airtable. Cell-range overlay still paints cells.
Collapse `checkedRows: Set<string>` + `allRowsSelected: boolean` into a
single `RowSelection = { kind: 'none' | 'some' | 'all' }`. Impossible
states (all + non-empty Set) become unrepresentable; predicates like
`rowSelectionIncludes` and `rowSelectionIsEmpty` replace ad-hoc checks at
every read site.The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview All table interactions that depend on row selection (select-all toggle, shift checkbox ranges, context-menu delete, keyboard delete/backspace, copy/cut, and row checkbox rendering) are updated to read/write Reviewed by Cursor Bugbot for commit e64787e. Configure here. |
handleContextMenuDelete dispatched the delete but left rowSelection at its prior 'all' or 'some' state. After rows clear and a new row arrives (realtime, undo, append), rowSelectionIncludes returned true for it, rendering it checked and flipping the master checkbox back on.
Greptile SummaryThis PR replaces two coupled state variables (
Confidence Score: 5/5Safe to merge — the change is a pure internal refactor of two coupled state variables into a discriminated union with no external API changes, all call sites updated consistently, and the only behavioural difference (row-range highlight decoupling) is intentional and documented. The helper functions are correct pure functions with no side effects, No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
NONE["RowSelection\nkind: 'none'"]
SOME["RowSelection\nkind: 'some'\nids: Set<string>"]
ALL["RowSelection\nkind: 'all'"]
NONE -->|"handleRowToggle (check row)"| SOME
SOME -->|"handleRowToggle (uncheck last row)"| NONE
SOME -->|"handleRowToggle (shift-range / toggle)"| SOME
SOME -->|"rowSelectionCoversAll → true (master toggle)"| ALL
ALL -->|"handleRowToggle (uncheck any row) via rowSelectionMaterialize"| SOME
ALL -->|"handleClearSelection / cell click"| NONE
NONE -->|"handleSelectAllRows / master toggle"| ALL
SOME -->|"handleClearSelection / cell click"| NONE
Reviews (5): Last reviewed commit: "fix(tables): guard context-menu delete o..." | Re-trigger Greptile |
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
- guard selectedRowCount 'all' branch on contextRow membership in rows - restore blank line between row-selection helpers and constants
waleedlatif1
commented
May 6, 2026
The helper compares NormalizedSelection (cell-range) state for a given row, not RowSelection. The old name collided with the new row-selection discriminated union and read ambiguously.
waleedlatif1
commented
May 6, 2026
@cursor review |
waleedlatif1
commented
May 6, 2026
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…ion on cancel - Guard the kind='all' branch on contextRow membership in currentRows (matches the same fix applied to selectedRowCount), so a context menu on a stale row no longer deletes the entire table. - Drop the eager rowSelection clear at modal-open time. The modal's onSuccess already calls handleClearSelection after the mutation resolves, so the post-delete invariant still holds; if the user cancels, the selection is now preserved.
waleedlatif1
commented
May 6, 2026
waleedlatif1
commented
May 6, 2026
@cursor review |
Uh oh!
There was an error while loading. Please reload this page.
waleedlatif1
commented
May 6, 2026
@cursor review |
waleedlatif1
commented
May 6, 2026
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit e64787e. Configure here.
Uh oh!
There was an error while loading. Please reload this page.
* fix(tables): decouple master checkbox from cell-range, add allRowsSelected flag
Master checkbox detached from gutter selection state when rows or columns
changed after Cmd+A: the predicate matched normalizedSelection bounds
exactly (endRow === rows.length-1, endCol === displayColumns.length-1),
so any post-selection growth flipped it false while the cell-range
overlay still painted every row checked.
Replace the structural two-branch predicate with an explicit
allRowsSelected flag plus a uniform set-membership check. handleSelectAllRows
sets the flag in O(1); handleRowToggle materializes checkedRows when
toggling out of "all" mode. Bulk-op read sites (delete, copy, cut,
selectedRowCount) honor the flag.
Decouple gutter checkbox from cell-range drag: dragging cells no longer
fills gutter checkboxes — they reflect explicit row-selection intent
only, matching Sheets/Airtable. Cell-range overlay still paints cells.
* refactor(tables): row selection as discriminated union
Collapse `checkedRows: Set<string>` + `allRowsSelected: boolean` into a
single `RowSelection = { kind: 'none' | 'some' | 'all' }`. Impossible
states (all + non-empty Set) become unrepresentable; predicates like
`rowSelectionIncludes` and `rowSelectionIsEmpty` replace ad-hoc checks at
every read site.
* fix(tables): clear row selection after context-menu delete
handleContextMenuDelete dispatched the delete but left rowSelection at
its prior 'all' or 'some' state. After rows clear and a new row arrives
(realtime, undo, append), rowSelectionIncludes returned true for it,
rendering it checked and flipping the master checkbox back on.
* chore(tables): address review nits on row selection refactor
- guard selectedRowCount 'all' branch on contextRow membership in rows
- restore blank line between row-selection helpers and constants
* chore(tables): rename rowSelectionChanged to cellRangeRowChanged
The helper compares NormalizedSelection (cell-range) state for a given
row, not RowSelection. The old name collided with the new row-selection
discriminated union and read ambiguously.
* fix(tables): guard context-menu delete on stale rows, preserve selection on cancel
- Guard the kind='all' branch on contextRow membership in currentRows
(matches the same fix applied to selectedRowCount), so a context menu
on a stale row no longer deletes the entire table.
- Drop the eager rowSelection clear at modal-open time. The modal's
onSuccess already calls handleClearSelection after the mutation
resolves, so the post-delete invariant still holds; if the user
cancels, the selection is now preserved.
Summary
checkedRows: Set<string>+allRowsSelected: booleaninto a singleRowSelection = { kind: 'none' | 'some' | 'all' }rowSelectionIncludes,rowSelectionIsEmpty) instead of ad-hoc checkssetRowSelectioncall instead of paired updatesType of Change
Testing
Tested manually — gutter check, shift-range check, Cmd+A, master toggle, copy/cut/delete on selection, context menu delete all behave identically.
Checklist