refactor(tables): decouple UI display from DB position#4448
refactor(tables): decouple UI display from DB position#4448waleedlatif1 merged 4 commits intostagingfrom
Conversation
Delete the PositionGapRows component, the gap-fill loop, and the
GAP_CHECKBOX_CLASS / GAP_ROW_LIMIT / PositionGapRowsProps surface area.
The server's recompactPositions() guarantees positions are 0..N-1
contiguous in the unfiltered view, so the phantom-row machinery has
been defending against a state that essentially never happens.
DataRow now receives an arrayIndex prop and renders {arrayIndex + 1}
in the gutter. Selection coordinates still flow through row.position;
that switches in phase 2.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Decouple Tables UI selection coordinates from DB position: - rowIndex semantics shift from row.position to array index across selection state, mouse handlers, keyboard nav, paste, scrollIntoView - checkedRows: Set<number> (position) → Set<string> (rowId), survives sort/filter and realtime row inserts - lastCheckboxRowRef stores rowId; shift-click range resolves to current array indices for visual-order ranges - Drop positionMap/maxPosition derived state in favor of direct rowsRef reads - ExpandedCellPopover anchors via data-row-id (row-id-stable) instead of data-row (array index) - collectRowSnapshots accepts Iterable<TableRowType> directly - Add bounds-validation effect to clamp anchor/focus when rows.length shrinks (sort change, pagination, realtime delete) - Drop redundant arrayIndex prop on DataRow (rowIndex now equals it) Server-side position math stays at API boundary only: insertRow, duplicateRow, shift-Enter append, paste create-batch, undo snapshots. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ia reduce - Track anchor/focus by rowId so same-length sort changes remap selection to the new visual index instead of leaving it on a different row. - Replace last-row position lookups with Math.max reduce in paste's create-batch and append-row's undo snapshot — under non-position sorts, the last visual row's position is not the largest. - Trim a navigation-noise comment and tighten two over-explanatory ones. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
PR SummaryMedium Risk Overview Row checkbox state is now keyed by Removes position-based helpers ( Reviewed by Cursor Bugbot for commit 633ddba. Configure here. |
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Greptile SummaryThis PR refactors the table component to decouple all UI display logic from
Confidence Score: 5/5Safe to merge — the refactor is internally consistent, all callsites updated, and the remap-effect design correctly handles concurrent sort/selection changes. The core remap mechanism is sound: anchorRowIdRef/focusRowIdRef are only written when selection changes, so the remap effect always reads the pre-sort IDs and never confuses two back-to-back updates. Every callsite that previously read positionMap or maxPositionRef has been updated to use rowsRef.current, and the paste-create position formula correctly captures lastRowPosition once outside the loop. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[rows changes\ne.g. sort/filter] --> B[remap useEffect fires]
B --> C{isColumnSelection?}
C -- yes --> D[skip — column effect\nhandles focus pin]
C -- no --> E{anchor rowIndex\nmatches anchorRowIdRef?}
E -- no, ID mismatch --> F[findIndex by ID\nin new rows]
F --> G{found?}
G -- yes --> H[setSelectionAnchor\nnew index]
G -- no --> I[setSelectionAnchor null]
E -- out of bounds --> I
E -- matches --> J[no-op]
H --> K[anchorRowIdRef effect\nupdates ref for next cycle]
I --> K
L[user clicks cell / checkbox] --> M[setSelectionAnchor / setCheckedRows\nwith array index / row ID]
M --> N[anchorRowIdRef effect\nstores rows index to ID mapping]
N --> O[remap effect ready\nfor next rows change]
Reviews (2): Last reviewed commit: "fix(tables): guard rowId remap effect an..." | Re-trigger Greptile |
- Skip the validation effect when rows is empty (transient state during initial load of a new sort/filter before keepPreviousData populates) so selection survives uncached query changes. - Skip when isColumnSelection is true; the column-selection pinning effect owns focus.rowIndex for those, and remapping would shrink a full-column range to wherever the captured endpoints happened to land after reorder. - Comment lastRowPosition's hoist invariant so a future refactor doesn't move it inside the loop and produce colliding positions. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
|
@greptile |
|
@cursor review |
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 633ddba. Configure here.
Summary
row.position. Position becomes an internal sort key the UI never surfaces.checkedRowsis rowId-keyed so check state survives sort/filter changes and realtime row inserts.position, but the next-position math usesMath.maxover loaded rows (last visual row's position is not the largest under non-position sorts).PositionGapRows,positionMap, andmin/maxPositionRef.Type of Change
Testing
Tested manually across the verification matrix: gutter numbering under sort/filter/sort+filter, single + range selection, shift-click checkbox range, Cmd+A, keyboard nav, copy/paste 3×3, insert above/below, duplicate, Shift+Enter append, undo/redo, expanded-cell popover under sort, pagination + sort. `bun run lint` and `bunx tsc --noEmit` clean.
Known limitation (documented, out of scope): inserting under an active data sort places the new row wherever the sort dictates, not visually adjacent to the anchor. Matches Airtable / Notion behavior.
Checklist