feat(browse): select records in the grid and delete them in bulk - #1706
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a robust row selection and bulk delete feature to the database table view, ensuring selection state is safely derived from visible rows and cleared on refresh. It also adds proper handling and user notifications for incomplete delete responses, refines the sidebar resizing behavior, and sets the appropriate CSS color-scheme for native dark-mode scrollbars. Feedback focuses on accessibility and UX improvements, specifically recommending the removal of aria-hidden on placeholder table cells to preserve screen reader grid structures, utilizing useLayoutEffect to prevent a 1-frame flicker on indeterminate checkboxes, and adding role="img" to the refresh icon for better assistive technology support.
| {selectColumnWidth !== undefined && ( | ||
| <TableCell | ||
| aria-hidden | ||
| style={{ width: `${selectColumnWidth}px` }} | ||
| // The right divider is an inset shadow, not `border-r` — see | ||
| // SELECT_COLUMN_DIVIDER in TableView: a collapsed border doesn't travel | ||
| // with a sticky cell. | ||
| className="sticky top-10 left-0 z-20 bg-card dark:bg-black-dark border-b border-border shadow-[inset_-1px_0_0_var(--color-border)]" | ||
| /> | ||
| )} |
There was a problem hiding this comment.
Using aria-hidden on a TableCell inside a visible table row breaks the table grid structure for screen readers, as they expect every row to have the same number of cells. Removing aria-hidden from this placeholder cell ensures the table structure remains consistent and accessible.
| {selectColumnWidth !== undefined && ( | |
| <TableCell | |
| aria-hidden | |
| style={{ width: `${selectColumnWidth}px` }} | |
| // The right divider is an inset shadow, not `border-r` — see | |
| // SELECT_COLUMN_DIVIDER in TableView: a collapsed border doesn't travel | |
| // with a sticky cell. | |
| className="sticky top-10 left-0 z-20 bg-card dark:bg-black-dark border-b border-border shadow-[inset_-1px_0_0_var(--color-border)]" | |
| /> | |
| )} | |
| {selectColumnWidth !== undefined && ( | |
| <TableCell | |
| style={{ width: `${selectColumnWidth}px` }} | |
| // The right divider is an inset shadow, not `border-r` — see | |
| // SELECT_COLUMN_DIVIDER in TableView: a collapsed border doesn't travel | |
| // with a sticky cell. | |
| className="sticky top-10 left-0 z-20 bg-card dark:bg-black-dark border-b border-border shadow-[inset_-1px_0_0_var(--color-border)]" | |
| /> | |
| )} |
| useEffect(() => { | ||
| if (ref.current) { | ||
| ref.current.indeterminate = indeterminate; | ||
| } | ||
| }, [indeterminate]); |
There was a problem hiding this comment.
Setting the indeterminate property on a checkbox DOM node inside useEffect runs after the browser has painted, which can cause a brief 1-frame visual flicker where the checkbox renders as checked or unchecked before becoming indeterminate. Using useLayoutEffect instead ensures the property is applied synchronously before the browser paints.
| useEffect(() => { | |
| if (ref.current) { | |
| ref.current.indeterminate = indeterminate; | |
| } | |
| }, [indeterminate]); | |
| useLayoutEffect(() => { | |
| if (ref.current) { | |
| ref.current.indeterminate = indeterminate; | |
| } | |
| }, [indeterminate]); |
| disabled={isFetching} | ||
| > | ||
| <RefreshCwIcon /> | ||
| <RefreshCwIcon aria-label="Refresh table" /> |
There was a problem hiding this comment.
When adding an aria-label to an SVG element (like RefreshCwIcon), it is recommended to also add role="img" to ensure that assistive technologies correctly identify the element as an image/icon with an accessible label, rather than ignoring it or treating it as generic layout.
<RefreshCwIcon aria-label="Refresh table" role="img" />
Review feedback on #1706: - The filter row's gutter spacer was `aria-hidden`, which hides a cell that is part of the row's structure; the cell is empty anyway, so the attribute bought nothing and cost the row its shape. - `RefreshCwIcon` carries an `aria-label` but no role, so the label was ignored — an `<svg>` needs `role="img"` for its name to be exposed. - The select-all checkbox assigned `indeterminate` in `useEffect`, one paint after the box had already rendered unchecked. `useLayoutEffect` sets it before the browser paints, so a partial selection never flashes as empty first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
b752a2a to
41a1a2e
Compare
kriszyp
left a comment
There was a problem hiding this comment.
I think this seems useful, looks good to me.
🤖 Reviewed with Codex
Review feedback on #1706: - The filter row's gutter spacer was `aria-hidden`, which hides a cell that is part of the row's structure; the cell is empty anyway, so the attribute bought nothing and cost the row its shape. - `RefreshCwIcon` carries an `aria-label` but no role, so the label was ignored — an `<svg>` needs `role="img"` for its name to be exposed. - The select-all checkbox assigned `indeterminate` in `useEffect`, one paint after the box had already rendered unchecked. `useLayoutEffect` sets it before the browser paints, so a partial selection never flashes as empty first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… answer Review feedback on #1706. Harper permits dots in database and table names, so `${database}.${table}` was not a collision-free identity: `a.b`/`c` and `a`/`b.c` both produce `a.b.c`. With page, sort, filters and cache mode also matching, a selected key survived navigation between those two tables — it appeared already checked in the second one, where bulk delete could remove a row nobody had selected there. The identity is now `JSON.stringify([databaseName, tableName])`, which cannot collide, and it is what both `resultSetKey` and `selectionEpoch` are built from. `describeIncompleteDelete` no longer reads an absent hash list as success. That leniency was inherited from `describeIncompleteUpdate`, but its reason — legacy responders that answer without naming hashes — does not hold for `delete`, which has returned both lists since 4.7.33: arrays on the normal path, and the numeric 0/count of its all-miss legacy path, which the array check catches just as the malformed guard did. Requiring both arrays therefore costs no supported responder anything, and it stops an empty body or an HTML 2xx from something in front of Harper being reported as a clean delete. `describeIncompletePut` has always required its own list this way, so delete now follows the stricter of the two existing precedents rather than the more forgiving one. Requiring the array also settles `wroteNothing`, which no longer has to decide whether an absent list meant zero deleted or simply unknown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
8aeb41a to
4e08f19
Compare
dawsontoth
left a comment
There was a problem hiding this comment.
I want keyboard shortcuts:
- Left and right or up and down arrow keys, when the modal is open and the editor isn't focused, to look between records.
- Cmd/ctrl or shift click to select rows when dealing with the table.
Wouldn't that be unrelated to this PR? I can fix in another PR. |
Adds a sticky checkbox gutter to the browse grid, with a header checkbox that toggles select-all/none and reads as indeterminate when only some rows are ticked, plus a "Delete Selected (n)" toolbar button gated on the table's delete permission. Rows are addressed by their raw primary-key value -- what Harper's `delete` takes as `hash_values` -- rather than by TanStack's row selection state, whose ids are strings that would have to be mapped back to the original (often numeric) keys to delete with. A row that carries no value for the declared primary key (the #1199 shape) cannot be named in a delete, so it gets a disabled checkbox rather than one that selects an undeletable row; the lookup is own-property guarded so a table declaring a key named `constructor` or `toString` can't resolve to the inherited `Object.prototype` member. Selection means "the rows on screen", and that invariant is enforced two ways because neither alone is enough. It is stored against an epoch of (entityId, resultSetKey, onlyIfCached) and dropped whenever that moves or the grid is refreshed -- entityId because the route swaps instances without remounting, so a key carried across would aim the delete at whatever the next instance stores under it, and onlyIfCached for the same reason `knownLastPage` retires on it. But what *counts* as selected is then derived each render by intersecting the stored set with the keys actually rendered, because the list query can swap its rows under unchanged parameters: adding a record invalidates it, and React Query refetches on window focus. Deriving makes the invariant hold by construction rather than by remembering to clear at every such moment. `delete` was the only write path with no incomplete-write helper -- update and put have had one since #1643 -- so it answered 200 while naming skipped records and Studio reported a clean success. `describeIncompleteDelete` mirrors `describeIncompleteUpdate`'s contract: an absent hash list reads as complete, since `delete` runs against every version Studio manages back to 4.7 and an unrecognized legacy response isn't evidence of failure, while a present non-array is a responder that does answer this operation with something we can't read. Applied to both delete paths, since every asymmetry between the write paths so far has come from changing one and not the other. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The 16px between the sidebar's divider and the table came from `gap-4` on the flex row, so it sat outside the grid and the grid could never reach the edge. The gap now applies only while the panes are stacked; from md up the table pane is flush and the inset moves onto the things that should carry it -- the toolbar, the pagination footer (which was 4px where the toolbar was 0), and the database overview, which was borrowing its left inset from the same gap. Two knock-ons the flush layout forced: The resize handle deliberately overhung the divider into that gap. With the gap gone it would have covered the grid's first column, so it now sits entirely past the divider in the pane's first 8px instead. It cannot simply move inside the sidebar either: the tree's own 10px scrollbar lives there, which is what the original straddle was avoiding. 8px clears both the scrollbar and the checkbox centred in the 32px gutter. The grid's tbody had a border on all four sides. Under `border-collapse: collapse` a cell's borders belong to the table's border grid rather than the cell's own box, so the side borders scrolled with the content while the sticky selection gutter stayed put, leaving a 1px seam at the scrollport edge that the scrolled rows showed through. The grid spans the pane edge to edge now and has no side edges to draw, so `border-y` removes the seam and the borders together. The gutter's own divider is an inset shadow for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Theming here is a `.dark` class, which the CSS variables follow but the browser's own widgets do not: with `color-scheme` left at its `normal` default, the UA paints native UI in its light palette regardless of what the page looks like. Scrollbars were the visible symptom -- light track and thumb against the dark grid -- but it covers form controls and the canvas behind the page too. Declared on `:root` and `.dark` rather than behind a `prefers-color-scheme` media query, because the theme is a user setting that can disagree with the OS: someone running the app in light mode on a dark desktop should get light scrollbars. `.dark` matches `:root`'s specificity and is declared after it, which is how every colour token in this file already wins. This is the root cause of the scrollbar rather than a `::-webkit-scrollbar` override, so it fixes every scroll container at once and keeps the scrollbars native in both themes instead of hand-painting them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review feedback on #1706: - The filter row's gutter spacer was `aria-hidden`, which hides a cell that is part of the row's structure; the cell is empty anyway, so the attribute bought nothing and cost the row its shape. - `RefreshCwIcon` carries an `aria-label` but no role, so the label was ignored — an `<svg>` needs `role="img"` for its name to be exposed. - The select-all checkbox assigned `indeterminate` in `useEffect`, one paint after the box had already rendered unchecked. `useLayoutEffect` sets it before the browser paints, so a partial selection never flashes as empty first. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… answer Review feedback on #1706. Harper permits dots in database and table names, so `${database}.${table}` was not a collision-free identity: `a.b`/`c` and `a`/`b.c` both produce `a.b.c`. With page, sort, filters and cache mode also matching, a selected key survived navigation between those two tables — it appeared already checked in the second one, where bulk delete could remove a row nobody had selected there. The identity is now `JSON.stringify([databaseName, tableName])`, which cannot collide, and it is what both `resultSetKey` and `selectionEpoch` are built from. `describeIncompleteDelete` no longer reads an absent hash list as success. That leniency was inherited from `describeIncompleteUpdate`, but its reason — legacy responders that answer without naming hashes — does not hold for `delete`, which has returned both lists since 4.7.33: arrays on the normal path, and the numeric 0/count of its all-miss legacy path, which the array check catches just as the malformed guard did. Requiring both arrays therefore costs no supported responder anything, and it stops an empty body or an HTML 2xx from something in front of Harper being reported as a clean delete. `describeIncompletePut` has always required its own list this way, so delete now follows the stricter of the two existing precedents rather than the more forgiving one. Requiring the array also settles `wroteNothing`, which no longer has to decide whether an absent list meant zero deleted or simply unknown. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ow keys Two shortcuts over the selection and the record editor. Ctrl/Cmd/Shift-clicking a row toggles its checkbox instead of opening the record editor. The gutter checkbox is a 32px target at the far left of a wide grid, so reaching it to tick a row you are already pointing at is the fiddly part of a multi-row delete; a modified click selects wherever the pointer already is. An unmodified click still opens the editor, so the ordinary path is unchanged, and the modifier only does anything when the grid has a selection column at all. Arrow keys step between records while the editor is open: Left/Up for the previous record, Right/Down for the next. The handler sits on the dialog and bails when the event came from inside the Monaco container, so cursor movement inside the JSON still belongs to the editor rather than turning into navigation and losing the user's place. It also respects `hasPrevious`/`hasNext` and stays out of the way while a write is in flight, so it can't step off either end of the result set or race a save. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
c1e8d6f to
317a0c6
Compare
|
@cb1kenobi lol, you already implemented it. I'm a sneaky bastard. 😆 |
Shift-click now selects every row between the last row picked without shift and the one clicked, rather than toggling the single row under the pointer. Picking twenty consecutive records was twenty clicks; it is now two. The anchor is held as a primary-key value rather than a row index, so a row set that moves underneath it -- a background refetch, a new record pushing rows onto another page -- simply fails to find the anchor and the click degrades to a plain pick, instead of silently measuring from whatever row now occupies that position. It stays put across consecutive shift-clicks, so adjusting the far end re-measures from the same origin rather than ratcheting along behind the pointer, and a range only ever ADDS: dragging the far end back over rows it already covered cannot silently drop one picked along the way. The range runs over selectable rows, so a row with no primary key to be addressed by is stepped over rather than ending the range at it. Two mechanics worth knowing, both of which cost a debugging pass: The checkbox is driven from `onClick`, not `onChange`, because only the mouse event carries `shiftKey`. It deliberately does NOT `preventDefault()`: swallowing the activation also swallows the `change` event React uses to reconcile a controlled checkbox, which leaves the box rendering the opposite of the state it just set. That is invisible in the common case and obvious in the range case, where re-covering an already-checked row never changes the `checked` prop at all. Shift-clicking rows otherwise drags a text selection across everything the range spans. The guard belongs on mousedown, where the selection starts -- preventing the click is already too late. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A range now applies the state of the click that anchored it, rather than always selecting. Untick a row and shift-click away from it and the range unticks; tick one and it ticks. Clearing a run of rows was previously only possible one click at a time, or by dropping the whole selection and rebuilding it. One rule covers both directions, so neither needs a modifier of its own -- which matters because the obvious alternative, making the range toggle each row it crosses, inverts whatever it spans. Dragging the far end back over rows already picked would then silently drop them, and a range over a mixed run would produce something nobody asked for. Applying one state is also what makes re-covering a row a no-op, so adjusting the far end stays predictable in both directions. The anchor therefore carries the state its click produced, not just its key. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both of these are in now, and thanks for the nudge on the arrow keys — the timing was funny but the ask was right. Cmd/Ctrl-click toggles a single row without opening the record editor. Plain click is already spoken for here (it opens the editor), so Cmd/Ctrl-click is the "pick this row, don't open it" analogue rather than the usual "add to selection without clearing" — there's nothing to clear, since we never drop a selection on a plain click. Shift-click selects the range between the row you last picked without shift and the one you click. It works from the checkbox or from anywhere on the row. One design note worth surfacing, because it's the part that isn't obvious: a shift range applies the state of the click that anchored it rather than always selecting. Untick a row, shift-click away from it, and the range unticks; tick one and it ticks. That's one rule for both directions, so deselecting a run doesn't need a modifier of its own. The alternative — having the range toggle each row it crosses — looks equivalent and isn't: it inverts whatever it spans, so dragging the far end back over rows you'd already picked would silently drop them, and a range across a mixed run would produce something nobody asked for. Applying a single state also makes re-covering a row a no-op, which is what keeps adjusting the far end predictable. Two other details: the anchor is held as a primary-key value rather than a row index, so if the rows move underneath it (a background refetch, or a new record pushing rows onto another page) the shift-click quietly degrades to a plain pick instead of measuring from whatever row now sits in that slot. And rows with no addressable primary key are stepped over by a range rather than ending it. 🤖 Generated by Claude Opus 5 |
…inter Range deselection was reachable only through a flow nobody would find. The range applied the state of the click that anchored it, so clearing a run meant first unticking a row with a plain click and only then shift-clicking. Any other order did nothing visible, and the most natural attempt -- build a range, then shift-click inside it to clear part of it -- was completely inert, because the anchor still said "selected" and the range re-selected rows that already were. The direction now comes from the row being clicked: shift-click a ticked row and the range unticks, shift-click an unticked one and it ticks. A shift-click therefore always does the thing the row under the pointer is visibly about to do, and a second one on the same row reverses it, so an over-wide range is recoverable without starting over. Anchor-state is what Gmail does, and it reads fine there because a plain click selects exactly one row, which keeps the anchor and its state in view. This grid has no such baseline -- a plain click opens the record editor -- so the anchor's state was invisible and usually stale. The anchor is now only a position, and select-all leaves one behind so clearing the top of a page doesn't need a plain click first. Behaviour confirmed against a real browser before changing anything: neither the label forwarding a click to its checkbox nor the row's mousedown guard strips or suppresses the modifier, so the plumbing was never the problem. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Press in a row's gutter and drag: every row the pointer crosses joins the range. Picking a run no longer needs two aimed clicks at opposite ends of it, which is the awkward part when the rows are far apart or the far end is off screen. The direction is fixed at mousedown by the row pressed, the same rule shift-click uses, so dragging out of a ticked row clears a run and out of an unticked one picks one. Backing the pointer off restores what it crossed to whatever it was when the drag began -- not merely unticked -- so a drag that overshoots is pulled back rather than started over, and rows that were already picked survive it. Held in a ref rather than state: it changes on every row crossed and nothing renders from it directly, so state would re-render the whole grid mid-drag for no visible gain. Verified against a real browser rather than assumed, since jsdom synthesises pointer events differently: `mouseover` does fire on each row while the button is held, `preventDefault` on the press stops a text selection being dragged across the rows, focus still reaches the checkbox afterwards, and a release over a different row fires no click at all. That last one is why the click guard only has to cover the pointer wandering back to the row it started on. A press with a modifier is left alone -- shift already means "extend from the anchor", and starting a drag would overwrite that anchor before the click could read it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds multi-select and bulk delete to the browse grid, plus the styling fixes that
fell out of making the grid full-width.
What you get
A sticky checkbox gutter pinned to the left of the grid. The header checkbox
toggles select-all/none for the page and shows an indeterminate state when only
some rows are ticked. Once anything is selected, a red-bordered
Delete Selected (n) button appears in the toolbar, confirms, and deletes.
Notes for review
Rows are addressed by raw primary-key value, not TanStack's row-selection
state — its row ids are strings, and mapping them back to the original (often
numeric) keys is exactly the kind of round-trip that turns into a wrong delete.
A row with no value under the declared primary key (the #1199 shape) can't be
named in a
delete, so it gets a disabled checkbox. The lookup isown-property-guarded, so a table declaring a key named
constructorortoStringcan't resolve to the inheritedObject.prototypemember.Selection means "the rows on screen", enforced two ways because neither is
sufficient alone:
(entityId, resultSetKey, onlyIfCached)anddropped when that moves or the grid is refreshed.
entityIdmatters becausethe route swaps instances without remounting this component — every sibling
piece of per-instance state resets on
allParamsfor that reason — so a keycarried across would aim the delete at whatever the next instance stores
under it.
onlyIfCachedis in there for the same reasonknownLastPageretires on it.
set with the keys actually rendered, because the list query can swap its rows
under unchanged parameters: adding a record invalidates it, and React Query
refetches on window focus (no
defaultOptionsare registered, so that defaultis live). Deriving makes the invariant hold by construction rather than by
remembering to clear at every moment that could break it.
deletewas the only write path with no incomplete-write helper —updateand
puthave had one since #1643 — so it answered 200 while naming skippedrecords and Studio reported a clean success.
describeIncompleteDeletemirrorsdescribeIncompleteUpdate's contract exactly: an absent hash list reads ascomplete (
deleteruns against every version Studio manages back to 4.7, and anunrecognized legacy response isn't evidence of failure), a present non-array is a
responder that does answer this operation with something unreadable. Applied to
both delete paths, since every asymmetry between the write paths so far has come
from changing one and not the other.
The styling commits
fix(browse): run the grid to the pane edge…— the 16px came fromgap-4on theflex row, outside the grid, so the grid could never reach the edge. The inset
moves onto the toolbar, the pagination footer (which was 4px where the toolbar
was 0) and the database overview, which was borrowing from the same gap. Two
knock-ons: the sidebar resize handle deliberately overhung that gap and would
have covered the grid's first column, but it can't move inside the sidebar
either — the tree's own 10px scrollbar lives there, which is what the original
straddle was avoiding — so it now sits in the pane's first 8px, clearing both.
And the grid's tbody went
border→border-y: underborder-collapse: collapsea cell's borders belong to the table's border grid rather than the cell's own
box, so the side borders scrolled with the content while the sticky gutter stayed
put, leaving a 1px seam the scrolled rows showed through.
fix(theme): declare color-scheme…— the app never declaredcolor-schemeanywhere. Theming is a
.darkclass, which the CSS variables follow but thebrowser's own widgets do not, so native UI was painted in the light palette
regardless. Scrollbars were the visible symptom; this also changes native form
controls app-wide (checkboxes, date pickers) to their dark-mode rendering, which
is intended but is the one change here with reach beyond browse.
Review coverage
Two cross-model rounds (Codex + Gemini) before this PR. Round 1 caught the
instance-crossing selection bug and the missing incomplete-write handling; round 2
caught the derived-selection gap and the resize-handle/scrollbar regression. Both
rounds' findings are fixed, each with a regression test verified to fail without
its fix.
Review feedback addressed (kriszyp, plus Gemini's accessibility notes):
tableIdentitywas${databaseName}.${tableName}, which is not collision-freebecause Harper permits dots in both names —
a.b/canda/b.cboth producea.b.c, so a selected key could survive navigation between them and arrivepre-checked in a table the user never selected it in. It is now
JSON.stringify([databaseName, tableName]), with a dot-containing regression test.describeIncompleteDeleteno longer reads an absent hash list as success.deletehas returned both lists since 4.7.33, so the leniency inherited fromdescribeIncompleteUpdatebought nothing and would have reported an empty body oran HTML 2xx from a proxy as a clean delete. It now follows
describeIncompletePut,the stricter existing precedent. This also settles
wroteNothing— the openquestion this description previously flagged — since it no longer has to decide
whether an absent list meant zero deleted or unknown.
aria-hiddenfrom the filter row's gutter spacer, addedrole="img"tothe refresh icon so its label is actually exposed, and moved the
indeterminateassignment to
useLayoutEffectso a partial selection never flashes unchecked.Testing
tsc,oxlint,dprint, full suite (344 files / 3088 tests) and a productionbuild all clean. Not verified in a live browser — the visual changes were
reasoned from the existing z-index/border model and confirmed against built CSS,
so the sticky layering and the flush layout deserve a look on a real instance.