Conversation
Four reviewers (codex, gemini, cursor-grok, Harper domain) on the whole branch. The blocker: a drag armed click suppression and nothing ever disarmed it. The only reset ran on a gutter press, so once any drag finished, every later row click was swallowed -- the record editor stopped opening and modifier selection went dead until the user happened to press a checkbox again. The flags are now cleared on every press, in the CAPTURE phase so the reset lands before the gutter's own mousedown rather than undoing it. That split the one flag into the two things it was conflating. A press that began in the gutter now suppresses the row's click however it ends, which also fixes a slip nobody had tried: press the 32px gutter, drift into the same row's data cell, release -- the click resolves on the `tr`, where the gutter cell's stopPropagation never sees it, and the record editor opened over what was meant to be a tick. Separately the checkbox now ignores a click that a drag already answered, so wandering back and releasing on the checkbox it started from no longer toggles that one row off again. The shift anchor is reset with the result set. `TableView` is rendered without a key, so it outlives paging, sorting and a table switch while the parent clears the selection; a leftover anchor whose primary-key value also exists in the next result set -- integer keys collide across tables constantly -- turned the next plain shift-click into a range over rows nobody anchored. Delete now shares `onWriteSettled`'s policy rather than half of it: a delete that answers `wroteNothing` leaves the editor open on the record the user has to act on, and both delete paths invalidate the open record as well as the table, which `refreshTable`'s prefix cannot reach. `describeIncompleteDelete` keeps failing closed on a fieldless answer, which is what this PR's review asked for, but no longer requires `skipped_hashes`: that list only adds detail, and demanding it would have turned a provable delete into an error toast. It now matches `describeIncompletePut` exactly -- require the list that proves the outcome, tolerate an absent companion, reject a present non-array. Also flattened the cache keys, which nested their own stringified output three deep, and pruned the narration the review called out. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the reliability of table row selection, drag-to-select, and record deletion behaviors in the database table view. Key changes include flattening the JSON keys for result sets and selection epochs to prevent nested escaping issues, resetting the shift-click anchor when the result set changes, and refining drag-selection state handling to prevent unintended row-click triggers and handle mouse releases outside the window. Additionally, the record deletion flow now invalidates the open record cache and handles cases where a delete operation wrote nothing. The requirement for skipped_hashes in the delete response has also been relaxed to be optional. I have no feedback to provide as the changes are well-implemented and thoroughly tested.
Coverage Report
File Coverage
|
||||||||||||||||||||||||||||||||||||||||||||||||||
kriszyp
left a comment
kriszyp
left a comment
There was a problem hiding this comment.
Sounds good.
🤖 Reviewed with Codex
| const tableIdentity = JSON.stringify([databaseName, tableName]); | ||
| const resultSetKey = JSON.stringify([ | ||
| tableIdentity, | ||
| const resultSetParts = [ |
There was a problem hiding this comment.
Could this become the one canonical “rows on screen” identity? selectionEpoch at src/features/instance/databases/components/DatabaseTableView.tsx:374-378 includes entityId and onlyIfCached, but resultSetKey omits both; the new reset at src/features/instance/databases/components/TableView.tsx:325-333 sees only resultSetKey/tableIdentity. Establish an anchor on instance A, navigate to instance B with the same database/table/page values, and the parent clears selection while anchorKey survives. If B contains the same primary-key value, the next Shift-click selects a range from that invisible old anchor. The cache-mode toggle has the same failure because it also changes which records are returned. Please use one canonical epoch containing entityId and onlyIfCached for the visible result, selection, and anchor reset (or pass the wider epoch explicitly).
| onSelectRow?.(selectionKey, event.shiftKey); | ||
| // A drag that crossed rows has already set this one; a trailing click here | ||
| // would toggle the row it started from straight back off. | ||
| if (dragMovedRef?.current || selectionKey === undefined) { |
There was a problem hiding this comment.
This guard remains armed for keyboard activation after a moved drag. The gutter deliberately focuses the checkbox at src/features/instance/databases/components/TableView.tsx:656-662, and the comment at src/features/instance/databases/components/TableView.tsx:672-674 correctly notes that keyboard activation arrives as a click. However, dragMovedRef is cleared only by the next window mousedown at src/features/instance/databases/components/TableView.tsx:313-318. After dragging and releasing, pressing Space on that focused checkbox therefore returns here without changing selection; repeated Space presses remain blocked until another mouse press occurs. Please scope suppression to the trailing mouse click—for example by distinguishing keyboard clicks via event.detail—or otherwise retire it after the originating gesture while preserving the no-click release case.
dawsontoth
left a comment
dawsontoth
left a comment
There was a problem hiding this comment.
Please fix up the commit messages Bairber, if you would, thanks!
3 participants
Follow-up to #1706, which merged while a cross-model review of it was still
running. Four reviewers — Codex, Gemini, Cursor-Grok and the Harper domain pass —
looked at the whole branch and found one blocker plus several real defects in the
drag/range selection. This is those fixes; no new features.
The blocker
A drag armed a click-suppression flag and nothing ever disarmed it. The only
reset ran on a gutter press, so once any drag finished, every later row click
was swallowed — the record editor stopped opening and ctrl/cmd/shift row
selection went dead — until the user happened to press a checkbox again. Three of
the four reviewers found this independently.
The flags are now cleared on every press, in the capture phase, so the reset
lands before the gutter's own
mousedownrather than undoing it.What that flag was conflating
Splitting it in two fixed a second bug nobody had tried: press the 32px gutter,
drift into the same row's data cell, release.
mousedownandmouseupshare nocell, so the click resolves on the
tr, where the gutter cell'sstopPropagationnever sees it — and the record editor opened over what wasmeant to be a tick. A press that began in the gutter now owns the row's click
however it ends.
Separately, the checkbox now ignores a click a drag already answered, so
wandering back and releasing on the checkbox you started from no longer toggles
that one row off again.
The shift anchor outlived its result set
TableViewis rendered without akey, so it survives paging, sorting,page-size changes and table switches while the parent clears the selection. An
anchor left behind whose primary-key value also exists in the next result set —
integer keys collide across tables constantly — turned the next plain
shift-click into a range over rows nobody anchored. It now resets with the
result set.
Delete now shares one policy instead of half of it
AGENTS.mdrecords that every asymmetry between the write paths so far came fromchanging one and not the other, and the delete path had drifted: it never called
refreshOpenRecord, and it closed the editor unconditionally. A delete answeringwroteNothingnow leaves the editor open on the record the user has to act on,exactly as
onWriteSettleddoes for a write that landed nothing, and both deletepaths invalidate the open record as well as the table —
refreshTable's prefixcannot reach
search_by_id.One deliberate half-revert
describeIncompleteDeletestill fails closed on a fieldless answer — that waskriszyp's explicit, version-grounded request on #1706 and it stands — but it no
longer requires
skipped_hashes. The domain pass was right that demanding a listwhich only adds detail would turn a provable delete into an error toast. It now
matches
describeIncompletePutexactly: require the list that proves theoutcome, tolerate an absent companion, reject a present non-array.
Also
Flattened the cache keys, which nested their own stringified output three deep
(
selectionEpochcarriedtableIdentityescaped three times over), and prunedthe narration two reviewers called out.
Testing
Four new regression tests, each verified to fail with its fix reverted. Full
suite 345 files / 3134 tests, plus
tsc -b, oxlint, dprint and a productionbuild. Still no browser-level proof of the pointer interactions — the review
called that out as the standing coverage gap — though I did verify the underlying
DOM mechanics against a real headless Chromium while building the drag.
Known, not fixed here
onSuccesshandlers are dropped if the component unmounts mid-flight,so navigating away between clicking Delete Selected and the response leaves the
cache holding deleted rows. This is the pre-existing pattern for all three
writers in this component, so fixing it properly means moving invalidation to
mutation level for update/put/delete together — worth its own PR.
Bounded, but it lands in the interaction being added.