Skip to content

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

Closed
waleedlatif1 wants to merge 1 commit intostagingfrom
waleedlatif1/tables-master-checkbox-fix
Closed

fix(tables): decouple master checkbox from cell-range#4464
waleedlatif1 wants to merge 1 commit intostagingfrom
waleedlatif1/tables-master-checkbox-fix

Conversation

@waleedlatif1
Copy link
Copy Markdown
Collaborator

Summary

  • Master select-all checkbox detached from gutter state when rows or columns changed after Cmd+A — the predicate matched normalizedSelection bounds exactly, so any post-selection growth (row append, column add, realtime insert) flipped it false while the cell-range overlay still painted every row checked.
  • Replaced 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.
  • Decoupled the 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.

Type of Change

  • Bug fix

Testing

Tested manually. Type check + biome clean.

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.
@vercel
Copy link
Copy Markdown

vercel Bot commented May 6, 2026

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

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped May 6, 2026 3:21am

Request Review

@cursor
Copy link
Copy Markdown

cursor Bot commented May 6, 2026

PR Summary

Medium Risk
Changes core table selection semantics and bulk operations (delete/copy/cut), so regressions could cause incorrect rows to be modified or acted on. Scope is contained to table.tsx UI state with no backend/data model changes.

Overview
Introduces an explicit allRowsSelected flag to represent master select-all, replacing the previous logic that inferred “all selected” from cell-range bounds/checked-row size.

Updates row checkbox behavior so gutter checkboxes reflect only explicit row selection (or master select-all), while cell-range selection no longer implicitly checks rows. Bulk operations (context-menu delete, Delete/Backspace clear, copy, cut, and selectedRowCount) now treat allRowsSelected as selecting every current row and materialize checkedRows when toggling out of all-selected mode.

Reviewed by Cursor Bugbot for commit 22b555e. Configure here.

@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented May 6, 2026

Greptile Summary

This PR replaces the structural normalizedSelection-bounds predicate for the master "select all" checkbox with an explicit allRowsSelected boolean flag, and decouples gutter row-checkboxes from cell-range drag selection to match Sheets/Airtable behaviour.

  • handleSelectAllRows (Cmd+A) sets allRowsSelected = true in O(1); bulk ops (delete, copy, cut, count) read the flag directly via a ref, and handleRowToggle materialises checkedRows from rowsRef when transitioning out of "all" mode.
  • isRowChecked is now allRowsSelected || checkedRows.has(row.id), completely decoupling the gutter checkbox from normalizedSelection; isRowSelectedByRange is removed from DataRow.
  • allRowsSelected is cleared in every interaction handler except handleContextMenuDelete, which means newly-arriving rows (realtime, undo, append) after a bulk context-menu delete will appear auto-selected until the user explicitly clears the selection.

Confidence Score: 3/5

Safe to merge with the context-menu delete fix applied; without it, rows arriving after a bulk delete will silently appear selected and could be unintentionally included in the next bulk operation.

The flag is reset consistently across every keyboard, mouse, and column-selection path, but handleContextMenuDelete is the one handler that deletes rows without clearing allRowsSelected. After deletion empties the table, isAllRowsSelected evaluates to false correctly, yet the underlying flag stays true. Any row subsequently appearing via real-time sync, undo, or manual append will immediately render as checked and be included in the next delete/cut/copy the user triggers.

apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx — specifically the handleContextMenuDelete callback around line 522.

Important Files Changed

Filename Overview
apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx Introduces allRowsSelected boolean flag to decouple master checkbox from cell-range selection; correctly clears the flag in most interaction paths but misses handleContextMenuDelete, leaving the flag live after bulk deletion and causing newly-arriving rows to appear auto-selected.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([User presses Cmd+A]) --> B[handleSelectAllRows]
    B --> C[setAllRowsSelected true
setCheckedRows EMPTY
setSelectionAnchor/Focus all]
    C --> D{User action?}
    D -->|Click cell| E[handleCellMouseDown
setAllRowsSelected false
setCheckedRows EMPTY]
    D -->|Click gutter checkbox| F[handleRowToggle
wasAllSelected materializes
checkedRows from rowsRef
setAllRowsSelected false]
    D -->|Navigate/Tab/Arrow| G[Keyboard handler
setAllRowsSelected false
setCheckedRows EMPTY]
    D -->|Column select| H[handleColumnSelect
setAllRowsSelected false
setCheckedRows EMPTY]
    D -->|Context-menu Delete| J[handleContextMenuDelete
collectRowSnapshots ALL
setDeletingRows
closeContextMenu
allRowsSelected NOT reset]
    J --> K[rows becomes empty
isAllRowsSelected = false]
    K --> L{New row arrives?}
    L -->|Yes| M[allRowsSelected still true
new row shows as checked]
    L -->|No| N[Visually correct]
Loading

Comments Outside Diff (2)

  1. apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx, line 491-527 (link)

    P1 allRowsSelected not cleared after context-menu delete

    handleContextMenuDelete collects and deletes all snapshots when allChecked is true, but never resets allRowsSelected. Once the deletion propagates and rows becomes empty, isAllRowsSelected correctly returns false (guarded by rows.length === 0). However, the allRowsSelected flag stays true in state. When any new row arrives afterwards via real-time sync, handleAppendRow, or an undo, isRowChecked={allRowsSelected || checkedRows.has(row.id)} immediately renders it as checked and isAllRowsSelected flips back to true, making the master checkbox appear fully selected again. Every other deletion/navigation path in this PR explicitly calls setAllRowsSelected(false); this callback is the only one that doesn't.

  2. apps/sim/app/workspace/[workspaceId]/tables/[tableId]/components/table/table.tsx, line 522-527 (link)

    P1 Add setAllRowsSelected(false) before closing the context menu. Every other deletion/navigation handler added in this PR resets the flag; this callback is the only one that omits it, leaving allRowsSelected live after all rows are deleted.

Reviews (1): Last reviewed commit: "fix(tables): decouple master checkbox fr..." | Re-trigger Greptile

@waleedlatif1
Copy link
Copy Markdown
Collaborator Author

Superseded by #4466, which contains this commit plus a follow-up refactor that collapses the dual selection state into a single discriminated union.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant