refactor(tables): row selection as discriminated union by waleedlatif1 · Pull Request #4466 · simstudioai/sim

waleedlatif1 · 2026-05-06T03:31:30Z

Summary

Collapse checkedRows: Set<string> + allRowsSelected: boolean into a single RowSelection = { kind: 'none' | 'some' | 'all' }
Impossible states (all + non-empty Set, both true) become unrepresentable
All read sites use predicates (rowSelectionIncludes, rowSelectionIsEmpty) instead of ad-hoc checks
All write sites use a single setRowSelection call instead of paired updates

Type of Change

Refactor

Testing

Tested manually — gutter check, shift-range check, Cmd+A, master toggle, copy/cut/delete on selection, context menu delete all behave identically.

Checklist

Code follows project style guidelines
Self-reviewed my changes
Tests added/updated and passing
No new warnings introduced
I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

…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.

vercel · 2026-05-06T03:31:35Z

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment

Project	Deployment	Actions	Updated (UTC)
docs	Skipped		May 6, 2026 4:27am

cursor · 2026-05-06T03:31:40Z

PR Summary

Medium Risk
Touches table row-selection state used by delete/cut/copy and context-menu actions, so a bug could affect which rows are modified or removed. Scope is limited to the table UI and is largely a mechanical refactor.

Overview
Row selection state is refactored from a plain Set (plus implicit/derived “all selected” logic) into a single RowSelection discriminated union (none | some | all) with helper predicates for inclusion/emptiness/materialization.

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 RowSelection consistently, including correctly handling the all case without enumerating ids. A small rename clarifies memo logic (rowSelectionChanged → cellRangeRowChanged) and row highlight behavior is simplified to depend on checkbox selection only.

^{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-apps · 2026-05-06T03:35:22Z

Greptile Summary

This PR replaces two coupled state variables (checkedRows: Set<string> + allRowsSelected: boolean) in the Table component with a single discriminated union RowSelection = { kind: 'none' } | { kind: 'some'; ids: Set<string> } | { kind: 'all' }, eliminating previously representable impossible states and centralizing all read/write access through a small set of pure helper functions.

Five module-level pure functions (rowSelectionIncludes, rowSelectionIsEmpty, rowSelectionMaterialize, rowSelectionCoversAll, and a renamed cellRangeRowChanged) replace ad-hoc inline checks at every call site.
All write sites are collapsed to a single setRowSelection call; handleRowToggle materialises the current selection into a temporary Set, mutates it, and normalises back to ROW_SELECTION_NONE when empty, preventing the { kind: 'some', ids: new Set() } dead state.
The isAllRowsSelected memo and selectedRowCount memo are updated to use the new helpers; the removed isRowSelectedByRange path in DataRow reflects the intentional decoupling of gutter-checkbox selection from cell-range highlight established in fix(tables): decouple master checkbox from cell-range #4464.

Confidence Score: 5/5

Safe 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, handleRowToggle correctly prevents the { kind: 'some', ids: new Set() } dead state, rowSelectionCoversAll handles all three kinds correctly, and the materialization path in the checkbox handler is safe. The previously noted issues around selectedRowCount and the blank-line style nit are already addressed in the companion commit. No logic regressions identified.

No files require special attention.

Important Files Changed

Filename	Overview
apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx	Discriminated union refactor replacing two coupled state variables with a single RowSelection type; all read/write sites updated consistently, helpers are correct, no new impossible states introduced, and previous thread issues (selectedRowCount guard, blank line) are already resolved in the companion commit `0f986cf`.

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}

- guard selectedRowCount 'all' branch on contextRow membership in rows - restore blank line between row-selection helpers and constants

waleedlatif1 · 2026-05-06T03:39:14Z

@greptile

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 · 2026-05-06T03:45:44Z

@cursor review

waleedlatif1 · 2026-05-06T03:45:48Z

@greptile

…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 · 2026-05-06T04:31:43Z

@greptile

waleedlatif1 · 2026-05-06T04:31:47Z

@cursor review

waleedlatif1 · 2026-05-06T05:02:12Z

@cursor review

waleedlatif1 · 2026-05-06T05:02:15Z

@greptile

cursor

✅ 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.}

waleedlatif1 added 2 commits May 5, 2026 20:21

vercel Bot temporarily deployed to Preview May 6, 2026 03:33 Inactive

waleedlatif1 mentioned this pull request May 6, 2026

fix(tables): decouple master checkbox from cell-range #4464

Closed

6 tasks

greptile-apps Bot reviewed May 6, 2026

View reviewed changes

Comment thread apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx

Comment thread apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx

Comment thread apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx

chore(tables): address review nits on row selection refactor

0f986cf

- guard selectedRowCount 'all' branch on contextRow membership in rows - restore blank line between row-selection helpers and constants

vercel Bot temporarily deployed to Preview May 6, 2026 03:37 Inactive

chore(tables): rename rowSelectionChanged to cellRangeRowChanged

fa8733c

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.

vercel Bot temporarily deployed to Preview May 6, 2026 03:43 Inactive

cursor Bot reviewed May 6, 2026

View reviewed changes

Comment thread apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx

Comment thread apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx Outdated

vercel Bot temporarily deployed to Preview May 6, 2026 04:27 Inactive

cursor Bot reviewed May 6, 2026

View reviewed changes

Comment thread apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx

cursor Bot reviewed May 6, 2026

View reviewed changes

waleedlatif1 merged commit 1814105 into staging May 6, 2026
14 checks passed

waleedlatif1 deleted the waleedlatif1/tables-row-selection-union branch May 6, 2026 05:10

waleedlatif1 mentioned this pull request May 6, 2026

v0.6.68: atlassian service accounts, 30 day wait block, markdown rendering upgrades, data drains, security hardening, paginated dropdowns #4469

Merged

Conversation

waleedlatif1 commented May 6, 2026

Summary

Type of Change

Testing

Checklist

Uh oh!

vercel Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cursor Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Summary

Uh oh!

greptile-apps Bot commented May 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 5/5

Important Files Changed

Flowchart

Uh oh!

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented May 6, 2026

Uh oh!

waleedlatif1 commented May 6, 2026

Uh oh!

waleedlatif1 commented May 6, 2026

Uh oh!

Uh oh!

Uh oh!

waleedlatif1 commented May 6, 2026

Uh oh!

waleedlatif1 commented May 6, 2026

Uh oh!

Uh oh!

waleedlatif1 commented May 6, 2026

Uh oh!

waleedlatif1 commented May 6, 2026

Uh oh!

cursor Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

vercel Bot commented May 6, 2026 •

edited

Loading

cursor Bot commented May 6, 2026 •

edited

Loading

greptile-apps Bot commented May 6, 2026 •

edited

Loading