From 497bd679f2b7af35fed3dc60093ffe8266db4ebb Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Fri, 11 Sep 2026 18:30:00 -0500 Subject: [PATCH 01/10] feat(browse): select records in the grid and delete them in bulk 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) --- .../databases/components/ColumnFilters.tsx | 13 + .../components/DatabaseTableView.test.tsx | 238 +++++++++++++++++- .../components/DatabaseTableView.tsx | 130 +++++++++- .../databases/components/TableView.test.tsx | 169 +++++++++++++ .../databases/components/TableView.tsx | 178 ++++++++++++- .../database/deleteTableRecords.test.ts | 47 ++++ .../instance/database/deleteTableRecords.ts | 49 +++- 7 files changed, 806 insertions(+), 18 deletions(-) create mode 100644 src/integrations/api/instance/database/deleteTableRecords.test.ts diff --git a/src/features/instance/databases/components/ColumnFilters.tsx b/src/features/instance/databases/components/ColumnFilters.tsx index e131cd4cb..1afff1f3d 100644 --- a/src/features/instance/databases/components/ColumnFilters.tsx +++ b/src/features/instance/databases/components/ColumnFilters.tsx @@ -21,10 +21,13 @@ export function ColumnFilters({ applyFilters, columnFiltersForm, headerGroups, + selectColumnWidth, }: { applyFilters: () => void; columnFiltersForm: UseFormReturn>; headerGroups: HeaderGroup[]; + /** Set when the grid renders its sticky selection gutter, so this row keeps the same columns. */ + selectColumnWidth?: number; }) { const handleSubmit = useCallback((e: KeyboardEvent) => { if (e.key === 'Enter') { @@ -38,6 +41,16 @@ export function ColumnFilters({
{headerGroups.map((headerGroup) => ( + {selectColumnWidth !== undefined && ( + + )} {headerGroup.headers.map((header) => { const relationshipInfo = header.column.columnDef.meta?.relationshipInfo; return ( diff --git a/src/features/instance/databases/components/DatabaseTableView.test.tsx b/src/features/instance/databases/components/DatabaseTableView.test.tsx index dd0818461..b7d7578d1 100644 --- a/src/features/instance/databases/components/DatabaseTableView.test.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.test.tsx @@ -3,9 +3,11 @@ */ import { InstanceDatabaseMap, InstanceTable } from '@/integrations/api/api.patch'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; -import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { act, cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { toast } from 'sonner'; import { afterEach, beforeAll, describe, expect, it, vi } from 'vitest'; import { DatabaseTableView } from './DatabaseTableView'; +import type { TableRowSelection } from './TableView'; // The dropdown's own permission gate is the thing under test; every other permission hook just // needs a fixed answer so the toolbar around it renders without pulling in the auth store/router. @@ -13,6 +15,7 @@ const permissionState = vi.hoisted(() => ({ canManageBrowseInstance: true, canImportFromFile: true, allowedSources: ['csv-data', 'csv-url', 'json-records'] as string[], + canDeleteRecords: true, })); vi.mock('@tanstack/react-router', () => { @@ -43,7 +46,8 @@ vi.mock('@/hooks/usePermissions', () => ({ allowsSource: (kind: string) => permissionState.allowedSources.includes(kind), allowsDestination: () => true, }), - useInstanceSchemaTablePermission: () => true, + useInstanceSchemaTablePermission: (_entityId: string, _database: string, _table: string, action: string) => + action === 'delete' ? permissionState.canDeleteRecords : true, useInstanceTablePutPermission: () => true, })); @@ -65,14 +69,35 @@ vi.mock('@/lib/events/watcher', async (importOriginal) => { // need real table data or a Radix Dialog. TableView's stub keeps its props, so a test can still // show the grid was built from the schema this render actually had. const tableViewColumns = vi.hoisted(() => ({ current: [] as { accessorKey?: string }[] })); +// The stub also hands back the selection wiring, so a test can tick rows the way the real grid +// would and then assert on the toolbar the selection drives. +const tableViewSelection = vi.hoisted(() => ({ + current: undefined as TableRowSelection | undefined, +})); -vi.mock('./TableView', () => ({ - TableView: ({ columns, emptyState }: { columns: { accessorKey?: string }[]; emptyState?: React.ReactNode }) => { +// Only the component is stubbed: `rowSelectionKey` is the real helper deciding which rows this +// component can address, and the derived-selection tests below turn on it behaving as shipped. +vi.mock('./TableView', async (importOriginal) => ({ + ...await importOriginal(), + TableView: ({ columns, emptyState, rowSelection }: { + columns: { accessorKey?: string }[]; + emptyState?: React.ReactNode; + rowSelection?: TableRowSelection; + }) => { tableViewColumns.current = columns; + tableViewSelection.current = rowSelection; // Rendering the slot is what lets a test follow a card click through to the launch it fires. return <>{emptyState}; }, })); + +// Only the mutation hook is stubbed: `describeIncompleteDelete` is the real thing under test when a +// response comes back partial or unreadable, so it must stay the module's own implementation. +const deleteRecords = vi.hoisted(() => ({ mutate: vi.fn() })); +vi.mock('@/integrations/api/instance/database/deleteTableRecords', async (importOriginal) => ({ + ...await importOriginal(), + useDeleteTableRecords: () => ({ mutate: deleteRecords.mutate, isPending: false }), +})); vi.mock('./PickColumnsDropdown', () => ({ PickColumnsDropdown: () => null })); vi.mock('../modals/EditTableRowModal', () => ({ EditTableRowModal: () => null })); @@ -81,6 +106,9 @@ vi.mock('../modals/EditTableRowModal', () => ({ EditTableRowModal: () => null }) // `get*QueryOptions` builder) keeps the real gating logic -- which reads `instanceDatabaseMap` // straight from props -- exercised as written. const describeTableData = vi.hoisted(() => ({ current: undefined as InstanceTable | undefined })); +// The rows the grid is showing. Selection is DERIVED from these, so a test can move a checked row +// off the page the way an invalidation or a focus refetch does. +const pageRows = vi.hoisted(() => ({ current: [] as Record[] })); vi.mock('@tanstack/react-query', async (importOriginal) => { const actual = await importOriginal(); @@ -89,6 +117,8 @@ vi.mock('@tanstack/react-query', async (importOriginal) => { useQuery: (options: { queryKey: readonly unknown[] }) => options.queryKey.includes('describe_table') ? { data: describeTableData.current, isFetching: false, isError: false } + : options.queryKey.includes('search_by_value') + ? { data: { data: pageRows.current }, isFetching: false, isError: false } : { data: undefined, isFetching: false, isError: false, refetch: vi.fn() }, }; }); @@ -109,8 +139,13 @@ afterEach(() => { permissionState.canManageBrowseInstance = true; permissionState.canImportFromFile = true; permissionState.allowedSources = ['csv-data', 'csv-url', 'json-records']; + permissionState.canDeleteRecords = true; tableViewColumns.current = []; + tableViewSelection.current = undefined; + pageRows.current = []; watchedValues.calls = []; + deleteRecords.mutate.mockReset(); + vi.restoreAllMocks(); }); const dogTable = { @@ -119,9 +154,10 @@ const dogTable = { } as unknown as InstanceTable; function renderView( - { instanceDatabaseMap }: { instanceDatabaseMap?: InstanceDatabaseMap } = {}, + { instanceDatabaseMap, rows }: { instanceDatabaseMap?: InstanceDatabaseMap; rows?: Record[] } = {}, ) { describeTableData.current = dogTable; + pageRows.current = rows ?? [{ id: 'abc' }, { id: 'def' }, { id: 1 }, { id: 2 }, { id: 3 }, { id: 'a' }, { id: 'b' }]; const queryClient = new QueryClient(); return render( @@ -267,3 +303,195 @@ describe('DatabaseTableView empty state', () => { expect(screen.getByRole('button', { name: /Import your data/ })).toBeTruthy(); }); }); + +describe('DatabaseTableView bulk delete', () => { + const deleteSelectedButton = () => screen.queryByRole('button', { name: /Delete Selected/ }); + + function selectRecords(keys: unknown[]) { + // Drive the real selection state through the grid's own callbacks rather than setting it + // from outside, so the toolbar is reacting to what a user ticking checkboxes produces. + for (const key of keys) { + act(() => tableViewSelection.current!.toggleRow(key)); + } + } + + it('offers nothing until a row is selected, then names how many', () => { + renderView(); + expect(deleteSelectedButton()).toBeNull(); + + selectRecords(['abc']); + + expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (1)'); + // The danger border is the whole point of the variant; tailwind-merge would have dropped it + // if the button had been given a conflicting one. + expect(deleteSelectedButton()!.classList.contains('border-destructive')).toBe(true); + + selectRecords(['def']); + + expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (2)'); + }); + + it('deletes every selected key once confirmed, then clears the selection', () => { + vi.spyOn(window, 'confirm').mockReturnValue(true); + renderView(); + selectRecords(['abc', 'def']); + + fireEvent.click(deleteSelectedButton()!); + + expect(deleteRecords.mutate).toHaveBeenCalledTimes(1); + const [payload, handlers] = deleteRecords.mutate.mock.calls[0]; + // Raw primary-key values, not stringified row ids -- `delete` addresses records by the value + // stored under the primary key. + expect(payload).toMatchObject({ databaseName: 'data', tableName: 'dog', hashValues: ['abc', 'def'] }); + + act(() => handlers.onSuccess({})); + + expect(deleteSelectedButton()).toBeNull(); + }); + + it('does not delete anything when the confirmation is declined', () => { + vi.spyOn(window, 'confirm').mockReturnValue(false); + renderView(); + selectRecords(['abc']); + + fireEvent.click(deleteSelectedButton()!); + + expect(deleteRecords.mutate).not.toHaveBeenCalled(); + // The selection survives a declined confirmation -- the user backed out of the delete, not + // out of the rows they had picked. + expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (1)'); + }); + + it('withholds the selection column from a role that cannot delete records', () => { + permissionState.canDeleteRecords = false; + renderView(); + expect(tableViewSelection.current).toBeUndefined(); + }); +}); + +describe('DatabaseTableView selection scope', () => { + const deleteSelectedButton = () => screen.queryByRole('button', { name: /Delete Selected/ }); + + it('arms the delete with every key the grid reports as selectable', () => { + vi.spyOn(window, 'confirm').mockReturnValue(true); + renderView(); + + // Select-all hands the grid's page keys straight through, so the delete is aimed at the same + // rows the header checkbox claimed to tick. + act(() => tableViewSelection.current!.toggleAll([1, 2, 3], true)); + expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (3)'); + + fireEvent.click(deleteSelectedButton()!); + + expect(deleteRecords.mutate.mock.calls[0][0]).toMatchObject({ hashValues: [1, 2, 3] }); + + act(() => tableViewSelection.current!.toggleAll([1, 2, 3], false)); + expect(deleteSelectedButton()).toBeNull(); + }); + + it('drops the selection when the grid is refreshed', () => { + // A refetch is the one moment the rows behind the checked keys can change without any query + // parameter moving, so a selection that survived it would describe rows nobody has looked at. + renderView(); + act(() => tableViewSelection.current!.toggleRow('abc')); + expect(deleteSelectedButton()).not.toBeNull(); + + fireEvent.click(screen.getByRole('button', { name: 'Refresh table' })); + + expect(deleteSelectedButton()).toBeNull(); + }); + + it('reports a delete the server only partly applied instead of claiming success', () => { + // `delete` answers 200 while naming the records it couldn't address. Reporting that as a clean + // success is what #1643 was about on the update path. + vi.spyOn(window, 'confirm').mockReturnValue(true); + const warned = vi.spyOn(toast, 'error').mockImplementation(() => ''); + const succeeded = vi.spyOn(toast, 'success').mockImplementation(() => ''); + renderView(); + act(() => tableViewSelection.current!.toggleAll(['a', 'b'], true)); + + fireEvent.click(deleteSelectedButton()!); + act(() => deleteRecords.mutate.mock.calls[0][1].onSuccess({ deleted_hashes: ['a'], skipped_hashes: ['b'] })); + + expect(succeeded).not.toHaveBeenCalled(); + expect(warned.mock.calls[0][1]).toMatchObject({ description: expect.stringContaining('deleted 1 of 2') }); + }); + + it('treats an unreadable delete answer as unproven rather than successful', () => { + vi.spyOn(window, 'confirm').mockReturnValue(true); + const warned = vi.spyOn(toast, 'error').mockImplementation(() => ''); + const succeeded = vi.spyOn(toast, 'success').mockImplementation(() => ''); + renderView(); + act(() => tableViewSelection.current!.toggleRow('a')); + + fireEvent.click(deleteSelectedButton()!); + // Present but not an array: that responder does answer `delete`, and its answer is unreadable. + act(() => deleteRecords.mutate.mock.calls[0][1].onSuccess({ skipped_hashes: null })); + + expect(succeeded).not.toHaveBeenCalled(); + expect(warned).toHaveBeenCalled(); + }); +}); + +describe('DatabaseTableView selection follows the rows on screen', () => { + const deleteSelectedButton = () => screen.queryByRole('button', { name: /Delete Selected/ }); + + it('disarms a selected row that leaves the page without any query parameter changing', () => { + // Adding a record invalidates this same list query, and React Query refetches on window focus + // (no `defaultOptions` are registered, so that default is live). Either can swap the rows while + // the epoch — entity, page, sort, filters, cache mode — is unchanged, so neither the epoch + // reset nor `refreshTable` fires. Without deriving the selection from the rows actually shown, + // "Delete Selected" stays armed for a record the user can no longer see. + const { rerender } = renderView({ rows: [{ id: 'abc' }, { id: 'def' }] }); + act(() => tableViewSelection.current!.toggleRow('abc')); + expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (1)'); + + pageRows.current = [{ id: 'def' }, { id: 'ghi' }]; + rerender( + + + , + ); + + expect(deleteSelectedButton()).toBeNull(); + }); + + it('deletes only the keys still on the page', () => { + vi.spyOn(window, 'confirm').mockReturnValue(true); + const { rerender } = renderView({ rows: [{ id: 'abc' }, { id: 'def' }] }); + act(() => tableViewSelection.current!.toggleAll(['abc', 'def'], true)); + expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (2)'); + + pageRows.current = [{ id: 'def' }]; + rerender( + + + , + ); + expect(deleteSelectedButton()!.textContent).toContain('Delete Selected (1)'); + + fireEvent.click(deleteSelectedButton()!); + + // 'abc' is still in the stored set, but it is no longer a row anyone can see. + expect(deleteRecords.mutate.mock.calls[0][0]).toMatchObject({ hashValues: ['def'] }); + }); + + it('will not arm a delete for a row that only inherits its primary key', () => { + // A primary key named `constructor` on a row that doesn't carry it resolves to the inherited + // function unless the lookup is own-property guarded. + describeTableData.current = { + attributes: [{ attribute: 'constructor', type: 'string', is_primary_key: true, indexed: true }], + primary_key: 'constructor', + } as unknown as InstanceTable; + pageRows.current = [{ other: 'inherits-only' }]; + render( + + + , + ); + + act(() => tableViewSelection.current!.toggleRow(Object.prototype.constructor)); + + expect(deleteSelectedButton()).toBeNull(); + }); +}); diff --git a/src/features/instance/databases/components/DatabaseTableView.tsx b/src/features/instance/databases/components/DatabaseTableView.tsx index d865c7b73..34729ee62 100644 --- a/src/features/instance/databases/components/DatabaseTableView.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.tsx @@ -32,7 +32,10 @@ import { useSessionStorage } from '@/hooks/useSessionStorage'; import { useToggler } from '@/hooks/useToggler'; import { InstanceDatabaseMap } from '@/integrations/api/api.patch'; import { useCleanupOrphanBlobsMutation } from '@/integrations/api/instance/database/cleanupOrphanBlobs'; -import { useDeleteTableRecords } from '@/integrations/api/instance/database/deleteTableRecords'; +import { + describeIncompleteDelete, + useDeleteTableRecords, +} from '@/integrations/api/instance/database/deleteTableRecords'; import { getDescribeTableQueryOptions } from '@/integrations/api/instance/database/getDescribeTable'; import { getSearchByConditionsOptions, @@ -56,6 +59,7 @@ import { getRegistrationInfoQueryOptions } from '@/integrations/api/instance/sta import { setWatchedValue } from '@/lib/events/watcher'; import { keyBy } from '@/lib/keyBy'; import { onClickStopPropagation } from '@/lib/onClickStopPropagation'; +import { pluralize } from '@/lib/pluralize'; import { Row } from '@/lib/table'; import { zodResolver } from '@hookform/resolvers/zod'; import { useQuery, useQueryClient } from '@tanstack/react-query'; @@ -83,7 +87,10 @@ import { toast } from 'sonner'; import { ColumnFiltersSchema } from './ColumnFilters'; import { EmptyResultSet } from './EmptyResultSet'; import { PickColumnsDropdown } from './PickColumnsDropdown'; -import { TableView } from './TableView'; +import { rowSelectionKey, TableRowSelection, TableView } from './TableView'; + +// Stable so `useEffectedState` can reset to it without rebuilding a set on every render. +const EMPTY_SELECTION: ReadonlySet = new Set(); export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName }: { instanceDatabaseMap?: InstanceDatabaseMap; @@ -355,6 +362,38 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName useFilteredList ? appliedSearchConditions : null, ]); + // Primary-key values of the checked rows. Selection describes the rows on screen, so it is dropped + // whenever they change -- otherwise paging away would leave a "Delete Selected" armed with records + // the user can no longer see. + // + // `resultSetKey` describes which rows the grid ASKED for, which is not the whole of "the rows on + // screen", so selection gets its own epoch on top of it: + // - `entityId` -- which server answers. The route swaps instances without remounting this + // component, which is why every sibling piece of per-instance state resets on `allParams`; + // a key carried across that boundary would aim the delete at whatever the NEXT instance + // happens to store under it. + // - `onlyIfCached` -- the cache mode changes which records come back at all, the same reason + // `knownLastPage` above retires on it. + const selectionEpoch = JSON.stringify([instanceParams.entityId, resultSetKey, onlyIfCached]); + const [selectedKeys, setSelectedKeys] = useEffectedState>(EMPTY_SELECTION, [selectionEpoch]); + const toggleRowSelected = useCallback((key: unknown) => { + setSelectedKeys((current) => { + const next = new Set(current); + if (!next.delete(key)) { + next.add(key); + } + return next; + }); + }, [setSelectedKeys]); + const toggleAllSelected = useCallback((keys: unknown[], selectAll: boolean) => { + setSelectedKeys(selectAll ? new Set(keys) : EMPTY_SELECTION); + }, [setSelectedKeys]); + // No delete permission, no reason to offer a selection: the grid renders no checkbox column at all. + const rowSelection = useMemo((): TableRowSelection | undefined => + canDeleteRecords + ? { selectedKeys, toggleRow: toggleRowSelected, toggleAll: toggleAllSelected } + : undefined, [canDeleteRecords, selectedKeys, toggleRowSelected, toggleAllSelected]); + // Full list const searchByValueParams = { ...instanceParams, @@ -402,6 +441,22 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName // arrived yet. const pageRows = tableData?.data; + // What counts as selected is DERIVED from the rows on screen, not just read out of the stored + // set. The epoch reset and the refresh clear bound what can accumulate, but neither fires when + // the list query swaps its rows under unchanged parameters -- adding a record invalidates this + // query and can push a checked row onto another page, and React Query refetches on window focus + // (this app registers no `defaultOptions`, so that default is live). Either leaves a key in the + // set with no row to show for it, and "Delete Selected" armed for a record nobody can see. + // Deriving makes "the selection describes rows on screen" hold by construction rather than by + // remembering to clear at every moment that could break it. + const visibleSelectedKeys = useMemo( + () => + (pageRows ?? []) + .map((row) => rowSelectionKey(row, primaryKey)) + .filter((key) => key !== undefined && selectedKeys.has(key)), + [pageRows, primaryKey, selectedKeys], + ); + // One by id const { data: searchByIdData, isFetching: isSearchByIdFetching, isError: isSearchByIdError } = useQuery( getSearchByIdOptions({ @@ -439,9 +494,14 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName // Records may have been added since a step proved a page terminal, so that proof retires // with the data it was made against. setKnownLastPage(null); + // So does the selection: it names records by primary key, and a refetch is precisely the + // moment the rows behind those keys can change without the query params moving. Anything + // that refreshes the grid -- the toolbar button, a write of ours, another writer's row + // landing -- leaves the checked set describing rows nobody has looked at. + setSelectedKeys(EMPTY_SELECTION); return queryClient.invalidateQueries({ queryKey: [instanceParams.entityId, databaseName, tableName] }); }, - [queryClient, instanceParams.entityId, databaseName, tableName, setKnownLastPage], + [queryClient, instanceParams.entityId, databaseName, tableName, setKnownLastPage, setSelectedKeys], ); // `refreshTable`'s prefix does NOT reach the open record: `getSearchById` keys on // `[entityId, 'search_by_id', databaseName, tableName, ids]`, so `'search_by_id'` sits where the @@ -559,15 +619,62 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName hashValues: hashes, }, { - onSuccess: () => { + onSuccess: (response) => { + // `refreshTable` also drops the selection: this record may well be one of the + // checked rows. void refreshTable(); setIsEditModalOpen(false); + const incomplete = describeIncompleteDelete(response, hashes.length); + if (incomplete) { + toast.error("The record wasn't deleted", { description: incomplete.message }); + return; + } toast.success('Record deleted successfully'); }, }, ); }, [deleteTableRecords, instanceParams, databaseName, tableName, refreshTable]); + // Bulk delete from the toolbar. Unlike the editor's single-record delete there is no record in + // front of the user to check against, so it confirms first; both read the answer the same way. + const onDeleteSelected = useCallback(() => { + const hashValues = visibleSelectedKeys; + if (!hashValues.length) { + return; + } + if (!confirm(`Permanently delete ${pluralize(hashValues.length, 'record', 'records')} from "${tableName}"?`)) { + return; + } + deleteTableRecords( + { + ...instanceParams, + databaseName, + tableName, + hashValues, + }, + { + onSuccess: (response) => { + // `refreshTable` drops the selection -- these rows are exactly the ones that just + // changed underneath it. + void refreshTable(); + const incomplete = describeIncompleteDelete(response, hashValues.length); + if (incomplete) { + toast.error("The records weren't all deleted", { description: incomplete.message }); + return; + } + toast.success(`${pluralize(hashValues.length, 'record', 'records')} deleted successfully`); + }, + }, + ); + }, [ + deleteTableRecords, + instanceParams, + databaseName, + tableName, + refreshTable, + visibleSelectedKeys, + ]); + // Point the editor at a record on the page the grid is showing. The row is kept as well as its // id because the editor falls back to it when the record can't be fetched by that id. const openRecordAt = useCallback((index: number, row: Record) => { @@ -704,7 +811,7 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName return ( <> -
+
{canAddRecords && ( )} + {canDeleteRecords && visibleSelectedKeys.length > 0 && ( + + )}
@@ -758,7 +875,7 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName onClick={onRefreshClick} disabled={isFetching} > - + { }); }); +describe('TableView row selection', () => { + // The last row has no value for the declared primary key -- the #1199 shape -- so it can't be + // named in a delete and must not be selectable. + const selectableRows: Record[] = [ + { id: 1, type: 'dog' }, + { id: 2, type: 'cat' }, + { type: 'orphan' }, + ]; + + function SelectionHarness( + { onRowClick, rows = selectableRows }: { + onRowClick?: () => void; + rows?: Record[]; + } = {}, + ) { + const columnFiltersForm = useForm>({ defaultValues: {} }); + const [columnSizing, setColumnSizing] = useState({}); + const [selectedKeys, setSelectedKeys] = useState>(new Set()); + return ( + > + applyFilters={() => undefined} + columnFiltersForm={columnFiltersForm} + columns={columns} + columnVisibility={{}} + columnSizing={columnSizing} + setColumnSizing={setColumnSizing} + data={rows} + onRowClick={onRowClick} + pageIndex={0} + pageSize={20} + primaryKey="id" + resultSetKey="page-0" + tableIdentity="dev.dog" + rowSelection={{ + selectedKeys, + toggleRow: (key) => + setSelectedKeys((current) => { + const next = new Set(current); + if (!next.delete(key)) { + next.add(key); + } + return next; + }), + toggleAll: (keys, selectAll) => setSelectedKeys(selectAll ? new Set(keys) : new Set()), + }} + setPageIndex={() => undefined} + setPageSize={() => undefined} + filtersToggled={false} + /> + ); + } + + function selectAllCheckbox() { + return document.querySelector('thead input[type="checkbox"]')!; + } + function rowCheckboxes() { + return Array.from(document.querySelectorAll('tbody input[type="checkbox"]')); + } + + it('renders no selection column when the caller supplies no rowSelection', () => { + render(); + expect(document.querySelectorAll('input[type="checkbox"]').length).toBe(0); + }); + + it('selects and deselects every addressable row from the header checkbox', () => { + render(); + // The keyless row gets a checkbox so the column stays aligned, but it can never be ticked. + expect(rowCheckboxes().map((box) => box.disabled)).toEqual([false, false, true]); + + fireEvent.click(selectAllCheckbox()); + expect(rowCheckboxes().map((box) => box.checked)).toEqual([true, true, false]); + // Every *selectable* row is selected, so the header reads as fully checked rather than partial. + expect(selectAllCheckbox().checked).toBe(true); + expect(selectAllCheckbox().indeterminate).toBe(false); + + fireEvent.click(selectAllCheckbox()); + expect(rowCheckboxes().map((box) => box.checked)).toEqual([false, false, false]); + }); + + it('shows the header checkbox as partially selected when only some rows are ticked', () => { + render(); + + fireEvent.click(rowCheckboxes()[0]); + + expect(selectAllCheckbox().checked).toBe(false); + // `indeterminate` is a DOM property with no attribute behind it, so this is the regression + // guard for it actually being assigned to the node. + expect(selectAllCheckbox().indeterminate).toBe(true); + }); + + it('disables the header checkbox when no row on the page can be addressed', () => { + render(); + expect(selectAllCheckbox().disabled).toBe(true); + expect(selectAllCheckbox().indeterminate).toBe(false); + }); + + it('draws the gutter divider as an inset shadow, never as a collapsed border', () => { + // The table is `border-collapse: collapse`, where a cell's borders belong to the table's border + // grid rather than the cell's own box — so a `border-r` here stays behind while the sticky cell + // slides over it, and the scrolled rows show through the 1px seam it leaves. Pinned because the + // obvious "cleanup" is to swap the shadow back for a border. + render(); + const gutterCells = [ + document.querySelector('thead th')!, + ...Array.from(document.querySelectorAll('tbody tr > td:first-child')), + ]; + for (const cell of gutterCells) { + expect(cell.className).toContain('shadow-[inset_-1px_0_0_var(--color-border)]'); + expect(cell.className).not.toMatch(/\bborder-[lr]\b/); + } + }); + + it('will not select a row that only inherits its primary key from Object.prototype', () => { + // `constructor` is a legal attribute name, and a row that doesn't carry it (the #1199 shape) + // resolves to the inherited function on a plain property read: non-null, so the row looks + // selectable, and the SAME reference for every such row — ticking one would tick them all. + const columnsByCtor: ColumnDef>[] = [{ header: 'constructor', accessorKey: 'constructor' }]; + const rows: Record[] = [{ constructor: 'real' }, { other: 'inherits-only' }]; + function CtorHarness() { + const form = useForm>({ defaultValues: {} }); + const [sizing, setSizing] = useState({}); + const [selectedKeys, setSelectedKeys] = useState>(new Set()); + return ( + > + applyFilters={() => undefined} + columnFiltersForm={form} + columns={columnsByCtor} + columnVisibility={{}} + columnSizing={sizing} + setColumnSizing={setSizing} + data={rows} + pageIndex={0} + pageSize={20} + primaryKey="constructor" + resultSetKey="page-0" + tableIdentity="dev.dog" + rowSelection={{ + selectedKeys, + toggleRow: (key) => setSelectedKeys(new Set([key])), + toggleAll: (keys, selectAll) => setSelectedKeys(selectAll ? new Set(keys) : new Set()), + }} + setPageIndex={() => undefined} + setPageSize={() => undefined} + filtersToggled={false} + /> + ); + } + render(); + + expect(rowCheckboxes().map((box) => box.disabled)).toEqual([false, true]); + + // Select-all must reach only the row that actually owns the attribute. + fireEvent.click(selectAllCheckbox()); + expect(rowCheckboxes().map((box) => box.checked)).toEqual([true, false]); + }); + + it('ticks a row without opening the record editor', () => { + // The row click opens the editor; the checkbox sits inside that click target, so without + // stopping propagation every selection would also open a modal over the grid. + let opened = 0; + render( opened++} />); + + fireEvent.click(rowCheckboxes()[1]); + + expect(rowCheckboxes().map((box) => box.checked)).toEqual([false, true, false]); + expect(opened).toBe(0); + }); +}); + describe('TableView column resizing', () => { it('renders a resize handle for each column header', () => { // Regression: the handle used to be gated on columnDef.enableResizing (never set), so it diff --git a/src/features/instance/databases/components/TableView.tsx b/src/features/instance/databases/components/TableView.tsx index 9587cebc7..7e618eafd 100644 --- a/src/features/instance/databases/components/TableView.tsx +++ b/src/features/instance/databases/components/TableView.tsx @@ -11,6 +11,7 @@ import { TableRow, } from '@/components/ui/table'; import { cn } from '@/lib/cn'; +import { onClickStopPropagation } from '@/lib/onClickStopPropagation'; import { Cell, ColumnDef, Row, studioTableFeatures } from '@/lib/table'; import { ColumnSizingState, @@ -20,12 +21,54 @@ import { RowData, useTable, } from '@tanstack/react-table'; -import { Dispatch, ReactNode, SetStateAction, useLayoutEffect, useMemo, useRef, useState } from 'react'; +import { Dispatch, ReactNode, SetStateAction, useEffect, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { UseFormReturn } from 'react-hook-form'; import { z } from 'zod'; import { ColumnFilters, ColumnFiltersSchema } from './ColumnFilters'; import { TablePagination } from './TablePagination'; +// Width of the sticky selection gutter. Not a TanStack column (it isn't sortable, resizable, +// hideable or filterable), so the width lives here and the resize guide adds it back below. +const SELECT_COLUMN_WIDTH = 32; +// The gutter's right-hand divider, drawn as an inset shadow rather than `border-r`. The table is +// `border-collapse: collapse` (Tailwind's preflight), where a cell's borders belong to the table's +// border grid instead of the cell's own box -- so a collapsed border stays put while the sticky cell +// slides over it, leaving a 1px seam that the scrolled rows show through. A shadow paints inside the +// cell's background box and travels with it. +const SELECT_COLUMN_DIVIDER = 'shadow-[inset_-1px_0_0_var(--color-border)]'; + +/** + * Wiring for the sticky checkbox column. Rows are addressed by their primary-key value -- the same + * value the delete operation takes -- rather than by TanStack's row selection state, whose row ids + * are strings and would have to be mapped back to the original (often numeric) keys to delete with. + * + * The owner supplies only the selected set and the two toggles; which rows are on screen, and + * therefore what "all" means, is derived here from the rows actually being rendered. + */ +/** + * The value a row is addressed by for selection and deletion, or `undefined` when it has none. + * + * `Object.hasOwn` rather than a plain property read: a table may legally declare a primary key + * named `constructor`, `toString` or `valueOf`, and a row that doesn't carry that attribute (the + * #1199 shape) would otherwise resolve to the inherited `Object.prototype` member -- a function, + * which reads as non-null so the row looks selectable, serializes to `null` in the delete, and is + * the *same reference* for every such row, so ticking one would tick them all. + */ +export function rowSelectionKey(row: unknown, primaryKey: string | undefined): unknown { + if (!primaryKey || typeof row !== 'object' || row === null || !Object.hasOwn(row, primaryKey)) { + return undefined; + } + const value = (row as Record)[primaryKey]; + return value == null ? undefined : value; +} + +export interface TableRowSelection { + selectedKeys: ReadonlySet; + toggleRow: (key: unknown) => void; + /** `keys` is every selectable row on the page, so the owner never recomputes them. */ + toggleAll: (keys: unknown[], selectAll: boolean) => void; +} + interface BrowseDataTableProps { applyFilters: () => void; columnFiltersForm: UseFormReturn>; @@ -43,6 +86,8 @@ interface BrowseDataTableProps { pageIndex: number; pageSize: number; primaryKey: string; + // Omitted when the user can't delete records: no selection column is rendered at all. + rowSelection?: TableRowSelection; // Identifies the rows on screen (table + page + sort + filters). See the reset effect below. resultSetKey: string; // Identifies which table is on screen, so a new set of columns starts scrolled to the left. @@ -73,6 +118,7 @@ export function TableView({ pageIndex, pageSize, primaryKey, + rowSelection, resultSetKey, tableIdentity, setPageIndex, @@ -107,6 +153,24 @@ export function TableView({ }, }); + // A row is only selectable if it can be addressed by the primary key the delete operation takes; + // a table whose declared primary key doesn't match how its rows are stored has rows that can't be + // (see #1199), and they get a disabled checkbox rather than one that selects an undeletable row. + const isSelectable = !!rowSelection && !!primaryKey; + const selectableKeys = useMemo(() => { + if (!isSelectable) { + return []; + } + return (data ?? []) + .map((row) => rowSelectionKey(row, primaryKey)) + .filter((key) => key !== undefined); + }, [isSelectable, data, primaryKey]); + const selectedOnPage = rowSelection + ? selectableKeys.filter((key) => rowSelection.selectedKeys.has(key)).length + : 0; + const allSelected = selectableKeys.length > 0 && selectedOnPage === selectableKeys.length; + const someSelected = selectedOnPage > 0 && !allSelected; + const scrollContainerRef = useRef(null); const [scrollLeftAtResizeStart, setScrollLeftAtResizeStart] = useState(0); @@ -143,7 +207,8 @@ export function TableView({ if (resizingColumnId) { const minSize = table.options.defaultColumn?.minSize ?? 20; const startSize = table.getColumn(resizingColumnId)?.getSize() ?? 0; - let edge = 0; + // The selection gutter sits left of every real column, so every edge shifts by its width. + let edge = isSelectable ? SELECT_COLUMN_WIDTH : 0; for (const leafColumn of table.getVisibleLeafColumns()) { edge += leafColumn.getSize(); if (leafColumn.id === resizingColumnId) { @@ -192,6 +257,24 @@ export function TableView({ {table.getHeaderGroups().map((headerGroup) => ( + {isSelectable && ( + // z-20: above the other sticky headers, which it crosses over when the + // grid is scrolled sideways. + + rowSelection.toggleAll(selectableKeys, !allSelected)} + /> + + )} {headerGroup.headers.map((header) => ( ({ applyFilters={applyFilters} columnFiltersForm={columnFiltersForm} headerGroups={table.getHeaderGroups()} + selectColumnWidth={isSelectable ? SELECT_COLUMN_WIDTH : undefined} /> )} {!showEmptyPanel && ( - + /* border-y, not border: the grid spans the pane edge to edge, so it has no side + edges to draw -- and a collapsed side border would scroll with the content while + the sticky selection gutter stayed put, leaving a 1px seam at the scrollport + edge. */ + {hasRows ? (table.getRowModel().rows.map((row) => ( ({ row={row} onRowClick={onRowClick} primaryKey={primaryKey} + rowSelection={isSelectable ? rowSelection : undefined} /> ))) : ( - + {isFetching || isAwaitingRows ? : No results.} @@ -270,8 +362,54 @@ export function TableView({ ); } +/** + * `indeterminate` is a DOM property with no HTML attribute behind it, so React can't render it -- + * it has to be assigned to the node. + */ +function SelectAllCheckbox( + { checked, indeterminate, disabled, onToggle }: { + checked: boolean; + indeterminate: boolean; + disabled: boolean; + onToggle: () => void; + }, +) { + const ref = useRef(null); + useEffect(() => { + if (ref.current) { + ref.current.indeterminate = indeterminate; + } + }, [indeterminate]); + return ( + + + + ); +} + +/** + * Fills the selection cell so the whole gutter is the hit target. A bare centred checkbox leaves the + * surrounding padding dead — a click that lands there does nothing at all, since the cell also has to + * swallow the click to keep the row from opening the record editor. + */ +function SelectCellLabel({ children }: { children: ReactNode }) { + return ; +} + function TableBodyRow( - { row, primaryKey, onRowClick }: { row: Row; primaryKey?: string; onRowClick?: (row: Row) => void }, + { row, primaryKey, onRowClick, rowSelection }: { + row: Row; + primaryKey?: string; + onRowClick?: (row: Row) => void; + rowSelection?: TableRowSelection; + }, ) { // TanStack memoizes getVisibleCells() and returns a fresh array whenever the // visible columns change, so depending on it keeps the body in step with the @@ -295,12 +433,40 @@ function TableBodyRow( return visibleCells.map((cell) => ); }, [row, primaryKey, visibleCells]); + const selectionKey = rowSelection ? rowSelectionKey(row.original, primaryKey) : undefined; + const isSelected = selectionKey !== undefined && !!rowSelection?.selectedKeys.has(selectionKey); + return ( onRowClick?.(row)} className={cn('hover:bg-muted/10 data-[state=selected]:bg-muted', onRowClick && 'cursor-pointer')} > + {rowSelection && ( + + + selectionKey !== undefined && rowSelection.toggleRow(selectionKey)} + /> + + + )} {cells} {/* Filler cell matching the header's filler column. */} diff --git a/src/integrations/api/instance/database/deleteTableRecords.test.ts b/src/integrations/api/instance/database/deleteTableRecords.test.ts new file mode 100644 index 000000000..9cd1ce5b1 --- /dev/null +++ b/src/integrations/api/instance/database/deleteTableRecords.test.ts @@ -0,0 +1,47 @@ +import { describe, expect, it } from 'vitest'; +import { describeIncompleteDelete } from './deleteTableRecords'; + +describe('describeIncompleteDelete', () => { + it('reads a clean delete as complete', () => { + expect(describeIncompleteDelete({ deleted_hashes: ['a', 'b'], skipped_hashes: [] }, 2)).toBeUndefined(); + }); + + // `delete` runs against every version Studio manages back to 4.7; a legacy response that names no + // hashes is not evidence the rows survived. + it('reads an absent hash list as complete', () => { + expect(describeIncompleteDelete({ message: 'deleted' }, 2)).toBeUndefined(); + expect(describeIncompleteDelete(undefined, 2)).toBeUndefined(); + }); + + it('reports the records the server skipped', () => { + const incomplete = describeIncompleteDelete({ deleted_hashes: ['a'], skipped_hashes: ['b'] }, 2); + expect(incomplete?.message).toContain('deleted 1 of 2'); + expect(incomplete?.message).toContain('skipped 1'); + expect(incomplete?.wroteNothing).toBe(false); + }); + + // A responder that names skipped records is answering the operation, so the legacy "assume it + // did what was asked" fallback must not also apply -- it produced "deleted 1 of 1 and skipped 1". + it('does not credit the skipped records as deleted when the deleted list is absent', () => { + const incomplete = describeIncompleteDelete({ skipped_hashes: ['b'] }, 2); + expect(incomplete?.message).toContain('deleted 1 of 2'); + expect(incomplete?.message).toContain('skipped 1'); + expect(describeIncompleteDelete({ skipped_hashes: ['a'] }, 1)?.message).toContain('deleted 0 of 1'); + }); + + it('reports a delete that removed nothing as having written nothing', () => { + const incomplete = describeIncompleteDelete({ deleted_hashes: [], skipped_hashes: ['a'] }, 1); + expect(incomplete?.wroteNothing).toBe(true); + }); + + // Present but not an array: that responder does answer `delete`, so its answer is unreadable + // rather than absent -- and an unreadable answer must not be reported as a success. + it('treats a present-but-unreadable hash list as unproven, not empty', () => { + for (const response of [{ skipped_hashes: null }, { deleted_hashes: 'a,b' }, { skipped_hashes: 3 }]) { + const incomplete = describeIncompleteDelete(response as never, 2); + expect(incomplete).toBeDefined(); + // Undecidable, so the caller still refreshes: the delete may have landed and replicated. + expect(incomplete?.wroteNothing).toBe(false); + } + }); +}); diff --git a/src/integrations/api/instance/database/deleteTableRecords.ts b/src/integrations/api/instance/database/deleteTableRecords.ts index 9329f76f4..53cd66d2d 100644 --- a/src/integrations/api/instance/database/deleteTableRecords.ts +++ b/src/integrations/api/instance/database/deleteTableRecords.ts @@ -1,5 +1,6 @@ import { InstanceClientConfig } from '@/config/instanceClientConfig'; import { useMutation } from '@tanstack/react-query'; +import { IncompleteWrite, isMalformedHashes, UNREADABLE_WRITE_MESSAGE } from './incompleteWrite'; interface DeleteTableRecordsData extends InstanceClientConfig { databaseName: string; @@ -7,10 +8,56 @@ interface DeleteTableRecordsData extends InstanceClientConfig { hashValues: unknown[]; } +export interface DeleteTableRecordsResponse { + message?: string; + deleted_hashes?: unknown[]; + skipped_hashes?: unknown[]; +} + +/** + * How the delete fell short of what was asked, or `undefined` when it didn't. + * + * The same shape as `describeIncompleteUpdate`, and deliberately so: `delete` answers 200 while + * naming in `skipped_hashes` the records it couldn't address, so a 200 alone doesn't mean the rows + * are gone. The asymmetry this removes is the one the browse view's write comment warns about -- + * update and put have read their answers since #1643; delete was still reporting every 200 as a + * clean success. + * + * An ABSENT field reads as complete, because `delete` runs against every version Studio manages back + * to 4.7 and an unrecognized legacy response isn't evidence of failure. A field that is present but + * not an array is different: that responder does answer this operation, and its answer is unreadable. + */ +export function describeIncompleteDelete( + data: DeleteTableRecordsResponse | undefined, + recordCount: number, +): IncompleteWrite | undefined { + if (isMalformedHashes(data?.deleted_hashes) || isMalformedHashes(data?.skipped_hashes)) { + return { + message: UNREADABLE_WRITE_MESSAGE, + wroteNothing: false, + }; + } + const skipped = data?.skipped_hashes?.length ?? 0; + // The `?? recordCount` fallback is the legacy-server reading: a responder that names no hashes at + // all told us nothing, so assume it did what was asked. That reading does NOT hold once + // `skipped_hashes` is populated -- this responder does answer the operation, and the records it + // named are exactly the ones it did not delete. + const deleted = data?.deleted_hashes?.length ?? Math.max(0, recordCount - skipped); + if (skipped === 0 && deleted >= recordCount) { + return undefined; + } + return { + message: `Harper deleted ${deleted} of ${recordCount} records${ + skipped > 0 ? ` and skipped ${skipped}` : '' + }. A record is skipped when nothing is stored under its primary key.`, + wroteNothing: Array.isArray(data?.deleted_hashes) && deleted === 0, + }; +} + export async function deleteTableRecords( { databaseName, tableName, hashValues, instanceClient }: DeleteTableRecordsData, ) { - const { data } = await instanceClient.post('/', { + const { data } = await instanceClient.post('/', { operation: 'delete', database: databaseName, table: tableName, From adc93dbfcd5c63a9ab0a294fdfa65c379523cfef Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Fri, 11 Sep 2026 18:30:26 -0500 Subject: [PATCH 02/10] fix(browse): run the grid to the pane edge and inset the toolbar instead 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) --- .../databases/components/DatabaseOverview.tsx | 2 +- .../databases/components/TablePagination.tsx | 2 +- src/features/instance/databases/index.tsx | 23 +++++++++++++------ 3 files changed, 18 insertions(+), 9 deletions(-) diff --git a/src/features/instance/databases/components/DatabaseOverview.tsx b/src/features/instance/databases/components/DatabaseOverview.tsx index 02ea71359..0387a3cf7 100644 --- a/src/features/instance/databases/components/DatabaseOverview.tsx +++ b/src/features/instance/databases/components/DatabaseOverview.tsx @@ -78,7 +78,7 @@ export function DatabaseOverview({ instanceDatabaseMap, databaseName }: { }, [navigate, params, databaseName]); return ( -
+

{databaseName}

diff --git a/src/features/instance/databases/components/TablePagination.tsx b/src/features/instance/databases/components/TablePagination.tsx index 94610c545..c5f296732 100644 --- a/src/features/instance/databases/components/TablePagination.tsx +++ b/src/features/instance/databases/components/TablePagination.tsx @@ -62,7 +62,7 @@ export function TablePagination( return (
-
+
{/* Summary — record count is the essential, kept at every width */}
diff --git a/src/features/instance/databases/index.tsx b/src/features/instance/databases/index.tsx index f49165e15..af3d4b794 100644 --- a/src/features/instance/databases/index.tsx +++ b/src/features/instance/databases/index.tsx @@ -46,18 +46,27 @@ export function Databases() { return ( <> -
+ { + /* gap only while stacked: from md up the table pane sits flush against the sidebar's + divider and insets its own toolbar/footer instead, so the grid itself is full-width. */ + } +
{ /* Drag (or focus + Arrow keys) to resize the sidebar (md+ only; mobile stacks full-width). - The grab zone straddles the edge into the inter-pane gap so it doesn't fight the tree's - scrollbar; only the thin centered line is visible (on hover / drag / focus). */ + The grab zone sits entirely PAST the divider, in the table pane's first few pixels, + because both neighbouring strips are already spoken for: inside the sidebar is the + tree's own 10px scrollbar (`.app-tree-scroll`), which a handle over it would make + undraggable, and the pane is flush now, so reaching further in would shadow the + selection gutter's checkbox. 8px clears the checkbox (centred in a 32px gutter) + and leaves the scrollbar alone. Only the thin line over the divider is visible + (on hover / drag / focus). */ }
Date: Fri, 11 Sep 2026 18:30:46 -0500 Subject: [PATCH 03/10] fix(theme): declare color-scheme so native UI follows the app's theme 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) --- src/index.css | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/index.css b/src/index.css index f01987ec5..3b63f010d 100644 --- a/src/index.css +++ b/src/index.css @@ -15,6 +15,14 @@ @custom-variant dark (&:where(.dark, .dark *)); :root { + /* Tells the browser which palette to paint NATIVE UI in — scrollbars above all, plus form + * controls and the canvas behind the page. Themes here are a `.dark` class, which CSS variables + * follow but the browser's own widgets do not: with `color-scheme` left at its `normal` default + * the scrollbars stayed light against the dark app. Paired with the `.dark` rule below, and + * deliberately not a `prefers-color-scheme` media query — the app's theme is a user setting that + * can disagree with the OS. */ + color-scheme: light; + --white: hsl(0, 0%, 99.61%); /* #fefefe */ --black: hsl(0, 0%, 12.94%); /* #212121 */ /* --black-light: hsl(0, 0%, 12%); #ffffff */ @@ -144,6 +152,10 @@ } .dark { + /* Same specificity as `:root` and declared after it, which is how every token below already + * wins — see the note on `color-scheme` up there. */ + color-scheme: dark; + --background: hsl(250 20% 8%); /* Deep purple-tinted dark background */ --foreground: hsl(0 0% 98%); --card: var(--color-grey-700); From 2e91355e3ab02100aa301a5760ca08a2c09fbaeb Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Fri, 11 Sep 2026 19:01:20 -0500 Subject: [PATCH 04/10] fix(browse): make the selection controls announce and settle correctly MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `` 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) --- src/features/instance/databases/components/ColumnFilters.tsx | 1 - .../instance/databases/components/DatabaseTableView.tsx | 2 +- src/features/instance/databases/components/TableView.tsx | 4 ++-- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/features/instance/databases/components/ColumnFilters.tsx b/src/features/instance/databases/components/ColumnFilters.tsx index 1afff1f3d..5a4e4c91b 100644 --- a/src/features/instance/databases/components/ColumnFilters.tsx +++ b/src/features/instance/databases/components/ColumnFilters.tsx @@ -43,7 +43,6 @@ export function ColumnFilters({ {selectColumnWidth !== undefined && ( - + (null); - useEffect(() => { + useLayoutEffect(() => { if (ref.current) { ref.current.indeterminate = indeterminate; } From 6a88223311a502d4b95cc456f5a3555c34fcedae Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Mon, 14 Sep 2026 08:31:57 -0500 Subject: [PATCH 05/10] fix(browse): key the selection on structure, and require delete's own answer MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) --- .../components/DatabaseTableView.test.tsx | 20 +++++++++++++++++++ .../components/DatabaseTableView.tsx | 2 +- .../database/deleteTableRecords.test.ts | 20 ++++++------------- .../instance/database/deleteTableRecords.ts | 19 +++++++----------- 4 files changed, 34 insertions(+), 27 deletions(-) diff --git a/src/features/instance/databases/components/DatabaseTableView.test.tsx b/src/features/instance/databases/components/DatabaseTableView.test.tsx index b7d7578d1..a3322b2ab 100644 --- a/src/features/instance/databases/components/DatabaseTableView.test.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.test.tsx @@ -401,6 +401,26 @@ describe('DatabaseTableView selection scope', () => { expect(deleteSelectedButton()).toBeNull(); }); + it('drops the selection between dot-containing database and table names', () => { + describeTableData.current = dogTable; + pageRows.current = [{ id: 42 }]; + const { rerender } = render( + + + , + ); + act(() => tableViewSelection.current!.toggleRow(42)); + expect(deleteSelectedButton()).not.toBeNull(); + + rerender( + + + , + ); + + expect(deleteSelectedButton()).toBeNull(); + }); + it('reports a delete the server only partly applied instead of claiming success', () => { // `delete` answers 200 while naming the records it couldn't address. Reporting that as a clean // success is what #1643 was about on the update path. diff --git a/src/features/instance/databases/components/DatabaseTableView.tsx b/src/features/instance/databases/components/DatabaseTableView.tsx index 4213acc0b..48bd026ee 100644 --- a/src/features/instance/databases/components/DatabaseTableView.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.tsx @@ -353,7 +353,7 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName // rows under it change. These are exactly the inputs the row queries below are keyed on, i.e. "which // records are on screen"; the table identity is called out separately because a different table also // means a different set of columns. - const tableIdentity = `${databaseName}.${tableName}`; + const tableIdentity = JSON.stringify([databaseName, tableName]); const resultSetKey = JSON.stringify([ tableIdentity, pageIndex, diff --git a/src/integrations/api/instance/database/deleteTableRecords.test.ts b/src/integrations/api/instance/database/deleteTableRecords.test.ts index 9cd1ce5b1..36ae8a57a 100644 --- a/src/integrations/api/instance/database/deleteTableRecords.test.ts +++ b/src/integrations/api/instance/database/deleteTableRecords.test.ts @@ -6,11 +6,12 @@ describe('describeIncompleteDelete', () => { expect(describeIncompleteDelete({ deleted_hashes: ['a', 'b'], skipped_hashes: [] }, 2)).toBeUndefined(); }); - // `delete` runs against every version Studio manages back to 4.7; a legacy response that names no - // hashes is not evidence the rows survived. - it('reads an absent hash list as complete', () => { - expect(describeIncompleteDelete({ message: 'deleted' }, 2)).toBeUndefined(); - expect(describeIncompleteDelete(undefined, 2)).toBeUndefined(); + it('treats absent hash lists as unproven', () => { + for (const response of [{ message: 'deleted' }, undefined, { deleted_hashes: [] }, { skipped_hashes: [] }]) { + const incomplete = describeIncompleteDelete(response, 2); + expect(incomplete?.message).toContain("didn't report which records"); + expect(incomplete?.wroteNothing).toBe(false); + } }); it('reports the records the server skipped', () => { @@ -20,15 +21,6 @@ describe('describeIncompleteDelete', () => { expect(incomplete?.wroteNothing).toBe(false); }); - // A responder that names skipped records is answering the operation, so the legacy "assume it - // did what was asked" fallback must not also apply -- it produced "deleted 1 of 1 and skipped 1". - it('does not credit the skipped records as deleted when the deleted list is absent', () => { - const incomplete = describeIncompleteDelete({ skipped_hashes: ['b'] }, 2); - expect(incomplete?.message).toContain('deleted 1 of 2'); - expect(incomplete?.message).toContain('skipped 1'); - expect(describeIncompleteDelete({ skipped_hashes: ['a'] }, 1)?.message).toContain('deleted 0 of 1'); - }); - it('reports a delete that removed nothing as having written nothing', () => { const incomplete = describeIncompleteDelete({ deleted_hashes: [], skipped_hashes: ['a'] }, 1); expect(incomplete?.wroteNothing).toBe(true); diff --git a/src/integrations/api/instance/database/deleteTableRecords.ts b/src/integrations/api/instance/database/deleteTableRecords.ts index 53cd66d2d..faad8c844 100644 --- a/src/integrations/api/instance/database/deleteTableRecords.ts +++ b/src/integrations/api/instance/database/deleteTableRecords.ts @@ -1,6 +1,6 @@ import { InstanceClientConfig } from '@/config/instanceClientConfig'; import { useMutation } from '@tanstack/react-query'; -import { IncompleteWrite, isMalformedHashes, UNREADABLE_WRITE_MESSAGE } from './incompleteWrite'; +import { IncompleteWrite, UNREADABLE_WRITE_MESSAGE } from './incompleteWrite'; interface DeleteTableRecordsData extends InstanceClientConfig { databaseName: string; @@ -23,26 +23,21 @@ export interface DeleteTableRecordsResponse { * update and put have read their answers since #1643; delete was still reporting every 200 as a * clean success. * - * An ABSENT field reads as complete, because `delete` runs against every version Studio manages back - * to 4.7 and an unrecognized legacy response isn't evidence of failure. A field that is present but - * not an array is different: that responder does answer this operation, and its answer is unreadable. + * Both hash lists are required to prove the result. A missing or non-array field leaves the write + * unproven and must not be reported as a success. */ export function describeIncompleteDelete( data: DeleteTableRecordsResponse | undefined, recordCount: number, ): IncompleteWrite | undefined { - if (isMalformedHashes(data?.deleted_hashes) || isMalformedHashes(data?.skipped_hashes)) { + if (!Array.isArray(data?.deleted_hashes) || !Array.isArray(data?.skipped_hashes)) { return { message: UNREADABLE_WRITE_MESSAGE, wroteNothing: false, }; } - const skipped = data?.skipped_hashes?.length ?? 0; - // The `?? recordCount` fallback is the legacy-server reading: a responder that names no hashes at - // all told us nothing, so assume it did what was asked. That reading does NOT hold once - // `skipped_hashes` is populated -- this responder does answer the operation, and the records it - // named are exactly the ones it did not delete. - const deleted = data?.deleted_hashes?.length ?? Math.max(0, recordCount - skipped); + const skipped = data.skipped_hashes.length; + const deleted = data.deleted_hashes.length; if (skipped === 0 && deleted >= recordCount) { return undefined; } @@ -50,7 +45,7 @@ export function describeIncompleteDelete( message: `Harper deleted ${deleted} of ${recordCount} records${ skipped > 0 ? ` and skipped ${skipped}` : '' }. A record is skipped when nothing is stored under its primary key.`, - wroteNothing: Array.isArray(data?.deleted_hashes) && deleted === 0, + wroteNothing: deleted === 0, }; } From 317a0c66f360bfd98239837e80b24ca16fdc186b Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Mon, 14 Sep 2026 11:36:37 -0500 Subject: [PATCH 06/10] feat(browse): select rows by modifier-click and step records with arrow 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) --- .../databases/components/TableView.test.tsx | 17 +++++++ .../databases/components/TableView.tsx | 11 ++++- .../modals/EditTableRowModal.test.tsx | 49 +++++++++++++++++-- .../databases/modals/EditTableRowModal.tsx | 25 +++++++++- 4 files changed, 96 insertions(+), 6 deletions(-) diff --git a/src/features/instance/databases/components/TableView.test.tsx b/src/features/instance/databases/components/TableView.test.tsx index ab53cef84..310c82a65 100644 --- a/src/features/instance/databases/components/TableView.test.tsx +++ b/src/features/instance/databases/components/TableView.test.tsx @@ -296,6 +296,23 @@ describe('TableView row selection', () => { expect(rowCheckboxes().map((box) => box.checked)).toEqual([false, true, false]); expect(opened).toBe(0); }); + + it.each([ + ['Control', { ctrlKey: true }], + ['Command', { metaKey: true }], + ['Shift', { shiftKey: true }], + ])('%s-click toggles a row without opening the record editor', (_modifier, eventInit) => { + let opened = 0; + render( opened++} />); + const firstRow = document.querySelector('tbody tr')!; + + fireEvent.click(firstRow, eventInit); + expect(rowCheckboxes().map((box) => box.checked)).toEqual([true, false, false]); + fireEvent.click(firstRow, eventInit); + + expect(rowCheckboxes().map((box) => box.checked)).toEqual([false, false, false]); + expect(opened).toBe(0); + }); }); describe('TableView column resizing', () => { diff --git a/src/features/instance/databases/components/TableView.tsx b/src/features/instance/databases/components/TableView.tsx index fb8100e28..630959a7b 100644 --- a/src/features/instance/databases/components/TableView.tsx +++ b/src/features/instance/databases/components/TableView.tsx @@ -439,7 +439,16 @@ function TableBodyRow( return ( onRowClick?.(row)} + onClick={(event) => { + if (rowSelection && (event.ctrlKey || event.metaKey || event.shiftKey)) { + event.preventDefault(); + if (selectionKey !== undefined) { + rowSelection.toggleRow(selectionKey); + } + return; + } + onRowClick?.(row); + }} className={cn('hover:bg-muted/10 data-[state=selected]:bg-muted', onRowClick && 'cursor-pointer')} > {rowSelection && ( diff --git a/src/features/instance/databases/modals/EditTableRowModal.test.tsx b/src/features/instance/databases/modals/EditTableRowModal.test.tsx index 732a227e0..e616efb55 100644 --- a/src/features/instance/databases/modals/EditTableRowModal.test.tsx +++ b/src/features/instance/databases/modals/EditTableRowModal.test.tsx @@ -595,24 +595,67 @@ describe('EditTableRowModal', () => { expect(onNext).toHaveBeenCalledTimes(1); }); + it.each( + [ + ['ArrowLeft', 'previous'], + ['ArrowUp', 'previous'], + ['ArrowRight', 'next'], + ['ArrowDown', 'next'], + ] as const, + )('%s steps to the %s record outside the editor', (key, direction) => { + const onPrevious = vi.fn(); + const onNext = vi.fn(); + renderModal({ recordNavigation: navigation({ onPrevious, onNext }) }); + + fireEvent.keyDown(screen.getByRole('dialog'), { key }); + + expect(onPrevious).toHaveBeenCalledTimes(direction === 'previous' ? 1 : 0); + expect(onNext).toHaveBeenCalledTimes(direction === 'next' ? 1 : 0); + }); + + it('leaves arrow keys to the editor while it is focused', () => { + const onPrevious = vi.fn(); + const onNext = vi.fn(); + renderModal({ recordNavigation: navigation({ onPrevious, onNext }) }); + + fireEvent.keyDown(screen.getByTestId('editor'), { key: 'ArrowRight' }); + + expect(onPrevious).not.toHaveBeenCalled(); + expect(onNext).not.toHaveBeenCalled(); + }); + it('offers no step past either end of the result set', () => { - renderModal({ recordNavigation: navigation({ hasPrevious: false, hasNext: false }) }); + const onPrevious = vi.fn(); + const onNext = vi.fn(); + renderModal({ + recordNavigation: navigation({ hasPrevious: false, hasNext: false, onPrevious, onNext }), + }); expect(previousButton().hasAttribute('disabled')).toBe(true); expect(nextButton().hasAttribute('disabled')).toBe(true); + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowLeft' }); + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowRight' }); + expect(onPrevious).not.toHaveBeenCalled(); + expect(onNext).not.toHaveBeenCalled(); }); // Leaving mid-write would leave the user watching a record that isn't the one being written. it('waits out a save before stepping away', () => { - renderModal({ recordNavigation: navigation(), isUpdateTableRecordsPending: true }); + const onNext = vi.fn(); + renderModal({ recordNavigation: navigation({ onNext }), isUpdateTableRecordsPending: true }); expect(nextButton().hasAttribute('disabled')).toBe(true); + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowRight' }); + expect(onNext).not.toHaveBeenCalled(); }); it('waits out a delete before stepping away', () => { - renderModal({ recordNavigation: navigation(), isDeleteTableRecordsPending: true }); + const onNext = vi.fn(); + renderModal({ recordNavigation: navigation({ onNext }), isDeleteTableRecordsPending: true }); expect(nextButton().hasAttribute('disabled')).toBe(true); + fireEvent.keyDown(screen.getByRole('dialog'), { key: 'ArrowRight' }); + expect(onNext).not.toHaveBeenCalled(); }); // The next record replaces the editor's contents, so an unsaved draft would go with it — diff --git a/src/features/instance/databases/modals/EditTableRowModal.tsx b/src/features/instance/databases/modals/EditTableRowModal.tsx index e348fc3d7..523a135f5 100644 --- a/src/features/instance/databases/modals/EditTableRowModal.tsx +++ b/src/features/instance/databases/modals/EditTableRowModal.tsx @@ -12,7 +12,7 @@ import { addCommasToNumbers } from '@/lib/addCommasToNumbers'; import { Editor } from '@/lib/monaco/MonacoEditor'; import { WORKER_FREE_JSON_LANGUAGE_ID } from '@/lib/monaco/workerFreeJsonLanguage'; import { ChevronLeftIcon, ChevronRightIcon, Save, Trash, TriangleAlert } from 'lucide-react'; -import { useLayoutEffect, useMemo, useRef, useState } from 'react'; +import { type KeyboardEvent, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { toast } from 'sonner'; import { describeRecordJsonError, tryParseRecordJson } from './recordEditorJson'; import { useRecordJsonErrorMarker } from './recordJsonErrorMarker'; @@ -89,6 +89,7 @@ export function EditTableRowModal({ isDeleteTableRecordsPending: boolean; }) { const monacoTheme = useMonacoTheme(); + const editorContainerRef = useRef(null); // A row that can't be addressed by its declared primary key can't be saved or deleted // individually, so force the editor read-only and hide the write actions regardless of the // user's permissions. @@ -187,6 +188,25 @@ export function EditTableRowModal({ setDiscardedUnsavedEdits(false); step(); }; + const onNavigationKeyDown = (event: KeyboardEvent) => { + if ( + !recordNavigation + || isWritePending + || editorContainerRef.current?.contains(event.target as Node) + ) { + return; + } + const step = event.key === 'ArrowLeft' || event.key === 'ArrowUp' + ? (recordNavigation.hasPrevious ? recordNavigation.onPrevious : undefined) + : event.key === 'ArrowRight' || event.key === 'ArrowDown' + ? (recordNavigation.hasNext ? recordNavigation.onNext : undefined) + : undefined; + if (!step) { + return; + } + event.preventDefault(); + stepToRecord(step); + }; return ( @@ -195,6 +215,7 @@ export function EditTableRowModal({ aria-describedby={undefined} resizable autoFocus={!isReadOnly} + onKeyDown={onNavigationKeyDown} onEscapeKeyDown={!isReadOnly ? (event) => { if (madeChanges) { @@ -255,7 +276,7 @@ export function EditTableRowModal({ // Wrapper owns the flex sizing: @monaco-editor/react applies `className` to its inner // element, not the layout wrapper, so `flex-1 min-h-0` has to live on a div we control // for the editor to shrink with the modal. -
+
Date: Mon, 14 Sep 2026 14:10:51 -0500 Subject: [PATCH 07/10] feat(browse): extend a selection by shift-clicking a range of rows 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) --- .../components/DatabaseTableView.tsx | 20 +- .../databases/components/TableView.test.tsx | 241 +++++++++++++----- .../databases/components/TableView.tsx | 70 ++++- 3 files changed, 263 insertions(+), 68 deletions(-) diff --git a/src/features/instance/databases/components/DatabaseTableView.tsx b/src/features/instance/databases/components/DatabaseTableView.tsx index 48bd026ee..32a9f3419 100644 --- a/src/features/instance/databases/components/DatabaseTableView.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.tsx @@ -388,11 +388,27 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName const toggleAllSelected = useCallback((keys: unknown[], selectAll: boolean) => { setSelectedKeys(selectAll ? new Set(keys) : EMPTY_SELECTION); }, [setSelectedKeys]); + // Adds only. A shift-click extends a selection; it does not clear whatever it happens to cross, + // so dragging the range back and forth can't silently drop a row picked along the way. + const selectRangeOfRows = useCallback((keys: unknown[]) => { + setSelectedKeys((current) => { + const next = new Set(current); + for (const key of keys) { + next.add(key); + } + return next; + }); + }, [setSelectedKeys]); // No delete permission, no reason to offer a selection: the grid renders no checkbox column at all. const rowSelection = useMemo((): TableRowSelection | undefined => canDeleteRecords - ? { selectedKeys, toggleRow: toggleRowSelected, toggleAll: toggleAllSelected } - : undefined, [canDeleteRecords, selectedKeys, toggleRowSelected, toggleAllSelected]); + ? { + selectedKeys, + toggleRow: toggleRowSelected, + toggleAll: toggleAllSelected, + selectRange: selectRangeOfRows, + } + : undefined, [canDeleteRecords, selectedKeys, toggleRowSelected, toggleAllSelected, selectRangeOfRows]); // Full list const searchByValueParams = { diff --git a/src/features/instance/databases/components/TableView.test.tsx b/src/features/instance/databases/components/TableView.test.tsx index 310c82a65..391b9c044 100644 --- a/src/features/instance/databases/components/TableView.test.tsx +++ b/src/features/instance/databases/components/TableView.test.tsx @@ -3,7 +3,7 @@ */ import { ColumnDef } from '@/lib/table'; import { ColumnSizingState, ColumnVisibilityState } from '@tanstack/react-table'; -import { cleanup, fireEvent, render, screen } from '@testing-library/react'; +import { cleanup, createEvent, fireEvent, render, screen } from '@testing-library/react'; import { useState } from 'react'; import { useForm } from 'react-hook-form'; import { afterEach, beforeAll, describe, expect, it } from 'vitest'; @@ -129,65 +129,73 @@ describe('TableView sorting', () => { }); }); -describe('TableView row selection', () => { - // The last row has no value for the declared primary key -- the #1199 shape -- so it can't be - // named in a delete and must not be selectable. - const selectableRows: Record[] = [ - { id: 1, type: 'dog' }, - { id: 2, type: 'cat' }, - { type: 'orphan' }, - ]; +// The last row has no value for the declared primary key -- the #1199 shape -- so it can't be +// named in a delete and must not be selectable. +const selectableRows: Record[] = [ + { id: 1, type: 'dog' }, + { id: 2, type: 'cat' }, + { type: 'orphan' }, +]; - function SelectionHarness( - { onRowClick, rows = selectableRows }: { - onRowClick?: () => void; - rows?: Record[]; - } = {}, - ) { - const columnFiltersForm = useForm>({ defaultValues: {} }); - const [columnSizing, setColumnSizing] = useState({}); - const [selectedKeys, setSelectedKeys] = useState>(new Set()); - return ( - > - applyFilters={() => undefined} - columnFiltersForm={columnFiltersForm} - columns={columns} - columnVisibility={{}} - columnSizing={columnSizing} - setColumnSizing={setColumnSizing} - data={rows} - onRowClick={onRowClick} - pageIndex={0} - pageSize={20} - primaryKey="id" - resultSetKey="page-0" - tableIdentity="dev.dog" - rowSelection={{ - selectedKeys, - toggleRow: (key) => - setSelectedKeys((current) => { - const next = new Set(current); - if (!next.delete(key)) { - next.add(key); - } - return next; - }), - toggleAll: (keys, selectAll) => setSelectedKeys(selectAll ? new Set(keys) : new Set()), - }} - setPageIndex={() => undefined} - setPageSize={() => undefined} - filtersToggled={false} - /> - ); - } +function SelectionHarness( + { onRowClick, rows = selectableRows }: { + onRowClick?: () => void; + rows?: Record[]; + } = {}, +) { + const columnFiltersForm = useForm>({ defaultValues: {} }); + const [columnSizing, setColumnSizing] = useState({}); + const [selectedKeys, setSelectedKeys] = useState>(new Set()); + return ( + > + applyFilters={() => undefined} + columnFiltersForm={columnFiltersForm} + columns={columns} + columnVisibility={{}} + columnSizing={columnSizing} + setColumnSizing={setColumnSizing} + data={rows} + onRowClick={onRowClick} + pageIndex={0} + pageSize={20} + primaryKey="id" + resultSetKey="page-0" + tableIdentity="dev.dog" + rowSelection={{ + selectedKeys, + toggleRow: (key) => + setSelectedKeys((current) => { + const next = new Set(current); + if (!next.delete(key)) { + next.add(key); + } + return next; + }), + toggleAll: (keys, selectAll) => setSelectedKeys(selectAll ? new Set(keys) : new Set()), + selectRange: (keys) => + setSelectedKeys((current) => { + const next = new Set(current); + for (const key of keys) { + next.add(key); + } + return next; + }), + }} + setPageIndex={() => undefined} + setPageSize={() => undefined} + filtersToggled={false} + /> + ); +} - function selectAllCheckbox() { - return document.querySelector('thead input[type="checkbox"]')!; - } - function rowCheckboxes() { - return Array.from(document.querySelectorAll('tbody input[type="checkbox"]')); - } +function selectAllCheckbox() { + return document.querySelector('thead input[type="checkbox"]')!; +} +function rowCheckboxes() { + return Array.from(document.querySelectorAll('tbody input[type="checkbox"]')); +} +describe('TableView row selection', () => { it('renders no selection column when the caller supplies no rowSelection', () => { render(); expect(document.querySelectorAll('input[type="checkbox"]').length).toBe(0); @@ -269,6 +277,14 @@ describe('TableView row selection', () => { selectedKeys, toggleRow: (key) => setSelectedKeys(new Set([key])), toggleAll: (keys, selectAll) => setSelectedKeys(selectAll ? new Set(keys) : new Set()), + selectRange: (keys) => + setSelectedKeys((current) => { + const next = new Set(current); + for (const key of keys) { + next.add(key); + } + return next; + }), }} setPageIndex={() => undefined} setPageSize={() => undefined} @@ -300,7 +316,6 @@ describe('TableView row selection', () => { it.each([ ['Control', { ctrlKey: true }], ['Command', { metaKey: true }], - ['Shift', { shiftKey: true }], ])('%s-click toggles a row without opening the record editor', (_modifier, eventInit) => { let opened = 0; render( opened++} />); @@ -315,6 +330,116 @@ describe('TableView row selection', () => { }); }); +describe('TableView shift-click range selection', () => { + // Five rows, one of them unaddressable so a range has something to step over. + const rangeRows: Record[] = [ + { id: 1 }, + { id: 2 }, + { orphan: true }, + { id: 4 }, + { id: 5 }, + ]; + + // Reported by row index: the keyless row has a checkbox but can never be checked. + const checkboxes = rowCheckboxes; + function checkedKeys() { + return checkboxes().map((box) => box.checked); + } + + function renderRange(onRowClick?: () => void) { + return render(); + } + + it('selects the inclusive range between the anchor and a shift-clicked row', () => { + renderRange(); + fireEvent.click(checkboxes()[0]); + expect(checkedKeys()).toEqual([true, false, false, false, false]); + + fireEvent.click(checkboxes()[3], { shiftKey: true }); + + // Rows 0..3 inclusive, and the keyless row in between stays unselected because it has no + // key to be selected by -- the range steps over it rather than stopping at it. + expect(checkedKeys()).toEqual([true, true, false, true, false]); + }); + + it('extends upward as readily as downward', () => { + renderRange(); + fireEvent.click(checkboxes()[4]); + + fireEvent.click(checkboxes()[1], { shiftKey: true }); + + expect(checkedKeys()).toEqual([false, true, false, true, true]); + }); + + it('re-measures from the same anchor instead of ratcheting along', () => { + // Shift-clicking a nearer row after a farther one must not leave the first range behind as + // the anchor, or each shift-click would extend from wherever the last one landed. + renderRange(); + fireEvent.click(checkboxes()[0]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + expect(checkedKeys()).toEqual([true, true, false, true, true]); + + fireEvent.click(checkboxes()[1], { shiftKey: true }); + + // Still measured from row 0. The range only adds, so rows 3 and 4 stay picked. + expect(checkedKeys()).toEqual([true, true, false, true, true]); + }); + + it('falls back to a plain pick when there is no anchor yet', () => { + renderRange(); + + fireEvent.click(checkboxes()[2 + 1], { shiftKey: true }); + + expect(checkedKeys()).toEqual([false, false, false, true, false]); + }); + + it('takes the anchor from the last pick made without shift', () => { + renderRange(); + fireEvent.click(checkboxes()[0]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + // A plain pick re-anchors, so the next shift measures from row 3, not row 0. + fireEvent.click(checkboxes()[3]); + expect(checkedKeys()).toEqual([true, true, false, false, true]); + + fireEvent.click(checkboxes()[4], { shiftKey: true }); + + expect(checkedKeys()).toEqual([true, true, false, true, true]); + }); + + it('extends from a shift-click on the row itself, not just the checkbox', () => { + let opened = 0; + renderRange(() => opened++); + fireEvent.click(checkboxes()[0]); + + fireEvent.click(document.querySelectorAll('tbody tr')[3], { shiftKey: true }); + + expect(checkedKeys()).toEqual([true, true, false, true, false]); + expect(opened).toBe(0); + }); + + it('keeps a row selected when a range re-covers it', () => { + // The clicked box is already checked and the range only adds, so its `checked` prop never + // changes -- the case where a natively-toggled checkbox would desync from React's state. + renderRange(); + fireEvent.click(checkboxes()[0]); + fireEvent.click(checkboxes()[3], { shiftKey: true }); + + fireEvent.click(checkboxes()[3], { shiftKey: true }); + + expect(checkedKeys()).toEqual([true, true, false, true, false]); + }); + + it('does not drag a text selection across the rows a shift-click spans', () => { + renderRange(); + const row = document.querySelectorAll('tbody tr')[2]; + + const mouseDown = createEvent.mouseDown(row, { shiftKey: true, bubbles: true, cancelable: true }); + fireEvent(row, mouseDown); + + expect(mouseDown.defaultPrevented).toBe(true); + }); +}); + describe('TableView column resizing', () => { it('renders a resize handle for each column header', () => { // Regression: the handle used to be gated on columnDef.enableResizing (never set), so it diff --git a/src/features/instance/databases/components/TableView.tsx b/src/features/instance/databases/components/TableView.tsx index 630959a7b..cee1de5e5 100644 --- a/src/features/instance/databases/components/TableView.tsx +++ b/src/features/instance/databases/components/TableView.tsx @@ -21,7 +21,7 @@ import { RowData, useTable, } from '@tanstack/react-table'; -import { Dispatch, ReactNode, SetStateAction, useLayoutEffect, useMemo, useRef, useState } from 'react'; +import { Dispatch, ReactNode, SetStateAction, useCallback, useLayoutEffect, useMemo, useRef, useState } from 'react'; import { UseFormReturn } from 'react-hook-form'; import { z } from 'zod'; import { ColumnFilters, ColumnFiltersSchema } from './ColumnFilters'; @@ -67,6 +67,8 @@ export interface TableRowSelection { toggleRow: (key: unknown) => void; /** `keys` is every selectable row on the page, so the owner never recomputes them. */ toggleAll: (keys: unknown[], selectAll: boolean) => void; + /** Adds a shift-click range. Only ever adds -- extending a range never clears what it crosses. */ + selectRange: (keys: unknown[]) => void; } interface BrowseDataTableProps { @@ -157,7 +159,7 @@ export function TableView({ // a table whose declared primary key doesn't match how its rows are stored has rows that can't be // (see #1199), and they get a disabled checkbox rather than one that selects an undeletable row. const isSelectable = !!rowSelection && !!primaryKey; - const selectableKeys = useMemo(() => { + const selectableKeys = useMemo(() => { if (!isSelectable) { return []; } @@ -171,6 +173,33 @@ export function TableView({ const allSelected = selectableKeys.length > 0 && selectedOnPage === selectableKeys.length; const someSelected = selectedOnPage > 0 && !allSelected; + // Where a shift-click measures from: the row last picked WITHOUT shift. Held as a key rather + // than an index so that a row set which moves underneath it -- a refetch, a new record pushing + // rows onto another page -- simply fails to find it and falls back to a plain toggle, instead of + // silently extending from whatever row now sits at that position. + const [anchorKey, setAnchorKey] = useState(undefined); + const selectRow = useCallback((key: unknown, extendRange: boolean) => { + if (!rowSelection) { + return; + } + const anchorIndex = extendRange ? selectableKeys.indexOf(anchorKey) : -1; + const targetIndex = selectableKeys.indexOf(key); + if (anchorIndex === -1 || targetIndex === -1) { + // No range to measure: a plain pick, which also becomes the anchor for the next shift. + rowSelection.toggleRow(key); + setAnchorKey(key); + return; + } + const [from, to] = anchorIndex <= targetIndex + ? [anchorIndex, targetIndex] + : [targetIndex, anchorIndex]; + // The range runs over SELECTABLE rows, so it steps over any row in between that has no + // primary key to be addressed by rather than stopping at it. + rowSelection.selectRange(selectableKeys.slice(from, to + 1)); + // The anchor deliberately stays put, so shift-clicking further up or down re-measures from + // the same origin instead of ratcheting along behind the pointer. + }, [rowSelection, selectableKeys, anchorKey]); + const scrollContainerRef = useRef(null); const [scrollLeftAtResizeStart, setScrollLeftAtResizeStart] = useState(0); @@ -315,6 +344,7 @@ export function TableView({ onRowClick={onRowClick} primaryKey={primaryKey} rowSelection={isSelectable ? rowSelection : undefined} + onSelectRow={selectRow} /> ))) : ( @@ -404,11 +434,12 @@ function SelectCellLabel({ children }: { children: ReactNode }) { } function TableBodyRow( - { row, primaryKey, onRowClick, rowSelection }: { + { row, primaryKey, onRowClick, rowSelection, onSelectRow }: { row: Row; primaryKey?: string; onRowClick?: (row: Row) => void; rowSelection?: TableRowSelection; + onSelectRow?: (key: unknown, extendRange: boolean) => void; }, ) { // TanStack memoizes getVisibleCells() and returns a fresh array whenever the @@ -439,12 +470,19 @@ function TableBodyRow( return ( { + if (rowSelection && event.shiftKey) { + event.preventDefault(); + } + }} onClick={(event) => { - if (rowSelection && (event.ctrlKey || event.metaKey || event.shiftKey)) { + // A modified click selects where the pointer already is; shift extends from the anchor. + // A row with no key to select by falls through to the editor rather than doing nothing. + if (rowSelection && selectionKey !== undefined && (event.ctrlKey || event.metaKey || event.shiftKey)) { event.preventDefault(); - if (selectionKey !== undefined) { - rowSelection.toggleRow(selectionKey); - } + onSelectRow?.(selectionKey, event.shiftKey); return; } onRowClick?.(row); @@ -471,7 +509,20 @@ function TableBodyRow( checked={isSelected} // A row with no primary-key value can't be named in a delete, so it can't be selected. disabled={selectionKey === undefined} - onChange={() => selectionKey !== undefined && rowSelection.toggleRow(selectionKey)} + // Driven from `onClick`, not `onChange`: only the mouse event carries `shiftKey`, + // which is what separates a range extend from a plain pick. Keyboard activation + // arrives here as a click too (with `shiftKey` false unless it is being held). + // + // Deliberately 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 -- including the range case + // where an already-checked row is re-selected and the prop never changes. + onClick={(event) => { + if (selectionKey !== undefined) { + onSelectRow?.(selectionKey, event.shiftKey); + } + }} + onChange={noopChange} /> @@ -483,6 +534,9 @@ function TableBodyRow( ); } +/** The click handler above owns the state; React only needs this to accept `checked` as controlled. */ +function noopChange() {} + function TableBodyRowCell({ cell }: { cell: Cell }) { const size = cell.column.getSize(); return ( From 140241c3f412f84c9afefd59a83f250c5a47e8af Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Mon, 14 Sep 2026 14:26:05 -0500 Subject: [PATCH 08/10] feat(browse): let a shift-click range deselect as readily as it selects 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) --- .../components/DatabaseTableView.tsx | 17 +++-- .../databases/components/TableView.test.tsx | 63 +++++++++++++++++-- .../databases/components/TableView.tsx | 32 ++++++---- 3 files changed, 87 insertions(+), 25 deletions(-) diff --git a/src/features/instance/databases/components/DatabaseTableView.tsx b/src/features/instance/databases/components/DatabaseTableView.tsx index 32a9f3419..7e17ef781 100644 --- a/src/features/instance/databases/components/DatabaseTableView.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.tsx @@ -388,13 +388,18 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName const toggleAllSelected = useCallback((keys: unknown[], selectAll: boolean) => { setSelectedKeys(selectAll ? new Set(keys) : EMPTY_SELECTION); }, [setSelectedKeys]); - // Adds only. A shift-click extends a selection; it does not clear whatever it happens to cross, - // so dragging the range back and forth can't silently drop a row picked along the way. - const selectRangeOfRows = useCallback((keys: unknown[]) => { + // One state applied across a shift-click range, rather than a toggle per row: a range that + // toggled would inverse whatever it crossed, so dragging it back over rows already picked would + // silently drop them. The grid decides which state from the anchoring click -- see `selectRow`. + const setRangeSelected = useCallback((keys: unknown[], selected: boolean) => { setSelectedKeys((current) => { const next = new Set(current); for (const key of keys) { - next.add(key); + if (selected) { + next.add(key); + } else { + next.delete(key); + } } return next; }); @@ -406,9 +411,9 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName selectedKeys, toggleRow: toggleRowSelected, toggleAll: toggleAllSelected, - selectRange: selectRangeOfRows, + setRangeSelected, } - : undefined, [canDeleteRecords, selectedKeys, toggleRowSelected, toggleAllSelected, selectRangeOfRows]); + : undefined, [canDeleteRecords, selectedKeys, toggleRowSelected, toggleAllSelected, setRangeSelected]); // Full list const searchByValueParams = { diff --git a/src/features/instance/databases/components/TableView.test.tsx b/src/features/instance/databases/components/TableView.test.tsx index 391b9c044..692a2984a 100644 --- a/src/features/instance/databases/components/TableView.test.tsx +++ b/src/features/instance/databases/components/TableView.test.tsx @@ -172,11 +172,15 @@ function SelectionHarness( return next; }), toggleAll: (keys, selectAll) => setSelectedKeys(selectAll ? new Set(keys) : new Set()), - selectRange: (keys) => + setRangeSelected: (keys, selected) => setSelectedKeys((current) => { const next = new Set(current); for (const key of keys) { - next.add(key); + if (selected) { + next.add(key); + } else { + next.delete(key); + } } return next; }), @@ -277,11 +281,15 @@ describe('TableView row selection', () => { selectedKeys, toggleRow: (key) => setSelectedKeys(new Set([key])), toggleAll: (keys, selectAll) => setSelectedKeys(selectAll ? new Set(keys) : new Set()), - selectRange: (keys) => + setRangeSelected: (keys, selected) => setSelectedKeys((current) => { const next = new Set(current); for (const key of keys) { - next.add(key); + if (selected) { + next.add(key); + } else { + next.delete(key); + } } return next; }), @@ -396,16 +404,59 @@ describe('TableView shift-click range selection', () => { it('takes the anchor from the last pick made without shift', () => { renderRange(); fireEvent.click(checkboxes()[0]); - fireEvent.click(checkboxes()[4], { shiftKey: true }); + fireEvent.click(checkboxes()[1], { shiftKey: true }); // A plain pick re-anchors, so the next shift measures from row 3, not row 0. fireEvent.click(checkboxes()[3]); - expect(checkedKeys()).toEqual([true, true, false, false, true]); + expect(checkedKeys()).toEqual([true, true, false, true, false]); fireEvent.click(checkboxes()[4], { shiftKey: true }); expect(checkedKeys()).toEqual([true, true, false, true, true]); }); + it('deselects the range when the anchoring click deselected its row', () => { + // The range applies the anchor's state, so shift unticks exactly as readily as it ticks -- + // no second modifier, just whichever direction the plain click before it went. + renderRange(); + fireEvent.click(checkboxes()[0]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + expect(checkedKeys()).toEqual([true, true, false, true, true]); + + fireEvent.click(checkboxes()[1]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + + // Rows 1..4 cleared; row 0 is outside the range and keeps what it had. + expect(checkedKeys()).toEqual([true, false, false, false, false]); + }); + + it('keeps a row unselected when a deselect range re-covers it', () => { + // Mirror of the select-side case: the clicked box is already unchecked and the range only + // clears, so its `checked` prop never changes -- where a natively-toggled checkbox would + // desync from React's state. + renderRange(); + fireEvent.click(checkboxes()[0]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + fireEvent.click(checkboxes()[1]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + + fireEvent.click(checkboxes()[4], { shiftKey: true }); + + expect(checkedKeys()).toEqual([true, false, false, false, false]); + }); + + it('deselects a range from the row as well as the checkbox', () => { + let opened = 0; + renderRange(() => opened++); + fireEvent.click(checkboxes()[0]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + fireEvent.click(checkboxes()[3]); + + fireEvent.click(document.querySelectorAll('tbody tr')[1], { shiftKey: true }); + + expect(checkedKeys()).toEqual([true, false, false, false, true]); + expect(opened).toBe(0); + }); + it('extends from a shift-click on the row itself, not just the checkbox', () => { let opened = 0; renderRange(() => opened++); diff --git a/src/features/instance/databases/components/TableView.tsx b/src/features/instance/databases/components/TableView.tsx index cee1de5e5..4dcc236ee 100644 --- a/src/features/instance/databases/components/TableView.tsx +++ b/src/features/instance/databases/components/TableView.tsx @@ -67,8 +67,8 @@ export interface TableRowSelection { toggleRow: (key: unknown) => void; /** `keys` is every selectable row on the page, so the owner never recomputes them. */ toggleAll: (keys: unknown[], selectAll: boolean) => void; - /** Adds a shift-click range. Only ever adds -- extending a range never clears what it crosses. */ - selectRange: (keys: unknown[]) => void; + /** Applies one state to a whole shift-click range. */ + setRangeSelected: (keys: unknown[], selected: boolean) => void; } interface BrowseDataTableProps { @@ -173,21 +173,27 @@ export function TableView({ const allSelected = selectableKeys.length > 0 && selectedOnPage === selectableKeys.length; const someSelected = selectedOnPage > 0 && !allSelected; - // Where a shift-click measures from: the row last picked WITHOUT shift. Held as a key rather - // than an index so that a row set which moves underneath it -- a refetch, a new record pushing - // rows onto another page -- simply fails to find it and falls back to a plain toggle, instead of - // silently extending from whatever row now sits at that position. - const [anchorKey, setAnchorKey] = useState(undefined); + // Where a shift-click measures from, and what it does when it gets there: the row last picked + // WITHOUT shift, plus the state that pick left it in. A range applies the ANCHOR'S state to + // everything it spans, which is what makes shift deselect as readily as it selects -- start by + // unticking a row and the range unticks; start by ticking one and it ticks. One rule covers both + // directions, so neither needs a modifier of its own. + // + // The anchor is held as a key rather than an index so that a row set which moves underneath it -- + // a refetch, a new record pushing rows onto another page -- simply fails to find it and falls + // back to a plain toggle, instead of silently measuring from whatever row now sits there. + const [anchor, setAnchor] = useState<{ key: unknown; selected: boolean } | null>(null); const selectRow = useCallback((key: unknown, extendRange: boolean) => { if (!rowSelection) { return; } - const anchorIndex = extendRange ? selectableKeys.indexOf(anchorKey) : -1; + const anchorIndex = extendRange && anchor ? selectableKeys.indexOf(anchor.key) : -1; const targetIndex = selectableKeys.indexOf(key); - if (anchorIndex === -1 || targetIndex === -1) { - // No range to measure: a plain pick, which also becomes the anchor for the next shift. + if (!anchor || anchorIndex === -1 || targetIndex === -1) { + // No range to measure: a plain pick, which also becomes the anchor for the next shift -- + // carrying the state it just produced, since that is what a range from it will apply. rowSelection.toggleRow(key); - setAnchorKey(key); + setAnchor({ key, selected: !rowSelection.selectedKeys.has(key) }); return; } const [from, to] = anchorIndex <= targetIndex @@ -195,10 +201,10 @@ export function TableView({ : [targetIndex, anchorIndex]; // The range runs over SELECTABLE rows, so it steps over any row in between that has no // primary key to be addressed by rather than stopping at it. - rowSelection.selectRange(selectableKeys.slice(from, to + 1)); + rowSelection.setRangeSelected(selectableKeys.slice(from, to + 1), anchor.selected); // The anchor deliberately stays put, so shift-clicking further up or down re-measures from // the same origin instead of ratcheting along behind the pointer. - }, [rowSelection, selectableKeys, anchorKey]); + }, [rowSelection, selectableKeys, anchor]); const scrollContainerRef = useRef(null); const [scrollLeftAtResizeStart, setScrollLeftAtResizeStart] = useState(0); From f06675a6d9236cf5f8451f39d16b4e91976da484 Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Mon, 14 Sep 2026 15:04:44 -0500 Subject: [PATCH 09/10] fix(browse): take the shift-click direction from the row under the pointer 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) --- .../databases/components/TableView.test.tsx | 57 +++++++++++++------ .../databases/components/TableView.tsx | 46 ++++++++------- 2 files changed, 68 insertions(+), 35 deletions(-) diff --git a/src/features/instance/databases/components/TableView.test.tsx b/src/features/instance/databases/components/TableView.test.tsx index 692a2984a..cf5a37f0e 100644 --- a/src/features/instance/databases/components/TableView.test.tsx +++ b/src/features/instance/databases/components/TableView.test.tsx @@ -381,7 +381,8 @@ describe('TableView shift-click range selection', () => { it('re-measures from the same anchor instead of ratcheting along', () => { // Shift-clicking a nearer row after a farther one must not leave the first range behind as - // the anchor, or each shift-click would extend from wherever the last one landed. + // the anchor, or each shift-click would measure from wherever the last one landed. Anchored + // at row 0 this clears rows 0..1; anchored at row 4 it would have cleared rows 1..4. renderRange(); fireEvent.click(checkboxes()[0]); fireEvent.click(checkboxes()[4], { shiftKey: true }); @@ -389,8 +390,7 @@ describe('TableView shift-click range selection', () => { fireEvent.click(checkboxes()[1], { shiftKey: true }); - // Still measured from row 0. The range only adds, so rows 3 and 4 stay picked. - expect(checkedKeys()).toEqual([true, true, false, true, true]); + expect(checkedKeys()).toEqual([false, false, false, true, true]); }); it('falls back to a plain pick when there is no anchor yet', () => { @@ -425,23 +425,22 @@ describe('TableView shift-click range selection', () => { fireEvent.click(checkboxes()[1]); fireEvent.click(checkboxes()[4], { shiftKey: true }); - // Rows 1..4 cleared; row 0 is outside the range and keeps what it had. + // Row 1's plain click unticked it and anchored there; row 4 is still ticked, so the shift + // measures 1..4 and clears it. Row 0 is outside the range and keeps what it had. expect(checkedKeys()).toEqual([true, false, false, false, false]); }); - it('keeps a row unselected when a deselect range re-covers it', () => { - // Mirror of the select-side case: the clicked box is already unchecked and the range only - // clears, so its `checked` prop never changes -- where a natively-toggled checkbox would - // desync from React's state. + it('selects a run by shift-clicking an unticked row, whatever else is picked', () => { + // The mirror of the clear case: the clicked row is unticked, so the range ticks -- even + // though rows it spans are already picked. renderRange(); fireEvent.click(checkboxes()[0]); - fireEvent.click(checkboxes()[4], { shiftKey: true }); - fireEvent.click(checkboxes()[1]); - fireEvent.click(checkboxes()[4], { shiftKey: true }); + fireEvent.click(checkboxes()[1], { shiftKey: true }); + expect(checkedKeys()).toEqual([true, true, false, false, false]); fireEvent.click(checkboxes()[4], { shiftKey: true }); - expect(checkedKeys()).toEqual([true, false, false, false, false]); + expect(checkedKeys()).toEqual([true, true, false, true, true]); }); it('deselects a range from the row as well as the checkbox', () => { @@ -468,16 +467,42 @@ describe('TableView shift-click range selection', () => { expect(opened).toBe(0); }); - it('keeps a row selected when a range re-covers it', () => { - // The clicked box is already checked and the range only adds, so its `checked` prop never - // changes -- the case where a natively-toggled checkbox would desync from React's state. + it('reverses a range when the same row is shift-clicked again', () => { + // The direction comes from the row under the pointer, so a second shift-click on it undoes + // the first -- which is what makes an over-wide range recoverable without starting over. renderRange(); fireEvent.click(checkboxes()[0]); fireEvent.click(checkboxes()[3], { shiftKey: true }); + expect(checkedKeys()).toEqual([true, true, false, true, false]); fireEvent.click(checkboxes()[3], { shiftKey: true }); - expect(checkedKeys()).toEqual([true, true, false, true, false]); + expect(checkedKeys()).toEqual([false, false, false, false, false]); + }); + + it('clears a run from inside a range the user just built', () => { + // The flow that was inert before the direction came from the clicked row: build a range, then + // shift-click inside it. Nothing happened at all, which reads as the feature being broken. + renderRange(); + fireEvent.click(checkboxes()[0]); + fireEvent.click(checkboxes()[4], { shiftKey: true }); + expect(checkedKeys()).toEqual([true, true, false, true, true]); + + fireEvent.click(checkboxes()[3], { shiftKey: true }); + + expect(checkedKeys()).toEqual([false, false, false, false, true]); + }); + + it('clears a run straight after select-all, with no plain click first', () => { + // Select-all leaves an anchor behind, so clearing the top of the page is one shift-click + // rather than a plain click to establish an anchor and then a shift-click. + render(); + fireEvent.click(selectAllCheckbox()); + expect(checkedKeys()).toEqual([true, true, false, true, true]); + + fireEvent.click(checkboxes()[3], { shiftKey: true }); + + expect(checkedKeys()).toEqual([false, false, false, false, true]); }); it('does not drag a text selection across the rows a shift-click spans', () => { diff --git a/src/features/instance/databases/components/TableView.tsx b/src/features/instance/databases/components/TableView.tsx index 4dcc236ee..660dff313 100644 --- a/src/features/instance/databases/components/TableView.tsx +++ b/src/features/instance/databases/components/TableView.tsx @@ -173,27 +173,29 @@ export function TableView({ const allSelected = selectableKeys.length > 0 && selectedOnPage === selectableKeys.length; const someSelected = selectedOnPage > 0 && !allSelected; - // Where a shift-click measures from, and what it does when it gets there: the row last picked - // WITHOUT shift, plus the state that pick left it in. A range applies the ANCHOR'S state to - // everything it spans, which is what makes shift deselect as readily as it selects -- start by - // unticking a row and the range unticks; start by ticking one and it ticks. One rule covers both - // directions, so neither needs a modifier of its own. + // Where a shift-click measures FROM. Only a position -- the direction comes from the row clicked, + // not from this, so that a shift-click always does the thing the row under the pointer is visibly + // about to do: click a ticked row and the range unticks, click an unticked one and it ticks. // - // The anchor is held as a key rather than an index so that a row set which moves underneath it -- - // a refetch, a new record pushing rows onto another page -- simply fails to find it and falls - // back to a plain toggle, instead of silently measuring from whatever row now sits there. - const [anchor, setAnchor] = useState<{ key: unknown; selected: boolean } | null>(null); + // Taking the direction from the anchor instead (what Gmail does) reads as broken here, because + // this grid has no "plain click selects only this row" baseline to make the anchor's state + // visible: after building a range, shift-clicking inside it re-applied "selected" to rows that + // already were, and nothing happened at all. + // + // Held as a key rather than an index so a row set that moves underneath it -- a refetch, a new + // record pushing rows onto another page -- simply fails to find it and the click degrades to a + // plain pick, instead of silently measuring from whatever row now sits there. + const [anchorKey, setAnchorKey] = useState(undefined); const selectRow = useCallback((key: unknown, extendRange: boolean) => { if (!rowSelection) { return; } - const anchorIndex = extendRange && anchor ? selectableKeys.indexOf(anchor.key) : -1; + const anchorIndex = extendRange ? selectableKeys.indexOf(anchorKey) : -1; const targetIndex = selectableKeys.indexOf(key); - if (!anchor || anchorIndex === -1 || targetIndex === -1) { - // No range to measure: a plain pick, which also becomes the anchor for the next shift -- - // carrying the state it just produced, since that is what a range from it will apply. + if (anchorIndex === -1 || targetIndex === -1) { + // No range to measure: a plain pick, which also becomes the anchor for the next shift. rowSelection.toggleRow(key); - setAnchor({ key, selected: !rowSelection.selectedKeys.has(key) }); + setAnchorKey(key); return; } const [from, to] = anchorIndex <= targetIndex @@ -201,10 +203,13 @@ export function TableView({ : [targetIndex, anchorIndex]; // The range runs over SELECTABLE rows, so it steps over any row in between that has no // primary key to be addressed by rather than stopping at it. - rowSelection.setRangeSelected(selectableKeys.slice(from, to + 1), anchor.selected); - // The anchor deliberately stays put, so shift-clicking further up or down re-measures from - // the same origin instead of ratcheting along behind the pointer. - }, [rowSelection, selectableKeys, anchor]); + // + // One state across the whole range rather than a toggle per row: a range that toggled would + // invert whatever it crossed, so it could never be used to clear a partly-picked run. + rowSelection.setRangeSelected(selectableKeys.slice(from, to + 1), !rowSelection.selectedKeys.has(key)); + // The anchor stays put, so shift-clicking further out re-measures from the same origin + // instead of ratcheting along behind the pointer. + }, [rowSelection, selectableKeys, anchorKey]); const scrollContainerRef = useRef(null); const [scrollLeftAtResizeStart, setScrollLeftAtResizeStart] = useState(0); @@ -306,7 +311,10 @@ export function TableView({ checked={allSelected} indeterminate={someSelected} disabled={selectableKeys.length === 0} - onToggle={() => rowSelection.toggleAll(selectableKeys, !allSelected)} + onToggle={() => { + rowSelection.toggleAll(selectableKeys, !allSelected); + setAnchorKey(allSelected ? undefined : selectableKeys[0]); + }} /> )} From 1ff845f9b2bfc73232192f96882cc004e7f02528 Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Mon, 14 Sep 2026 15:44:16 -0500 Subject: [PATCH 10/10] feat(browse): select a run of rows by dragging down the checkbox gutter 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) --- .../databases/components/TableView.test.tsx | 113 ++++++++++++++++ .../databases/components/TableView.tsx | 126 +++++++++++++++++- 2 files changed, 235 insertions(+), 4 deletions(-) diff --git a/src/features/instance/databases/components/TableView.test.tsx b/src/features/instance/databases/components/TableView.test.tsx index cf5a37f0e..130345554 100644 --- a/src/features/instance/databases/components/TableView.test.tsx +++ b/src/features/instance/databases/components/TableView.test.tsx @@ -516,6 +516,119 @@ describe('TableView shift-click range selection', () => { }); }); +describe('TableView drag selection', () => { + const rangeRows: Record[] = [ + { id: 1 }, + { id: 2 }, + { orphan: true }, + { id: 4 }, + { id: 5 }, + ]; + const checkedKeys = () => rowCheckboxes().map((box) => box.checked); + const rows = () => Array.from(document.querySelectorAll('tbody tr')); + const gutterOf = (index: number) => rows()[index].querySelector('td')!; + + /** Press on one row's gutter, cross the rows named, release. */ + function drag(from: number, over: number[]) { + fireEvent.mouseDown(gutterOf(from), { button: 0 }); + for (const index of over) { + fireEvent.mouseOver(rows()[index]); + } + fireEvent.mouseUp(window); + } + + it('selects every row the pointer crosses', () => { + render(); + + drag(0, [1, 3]); + + // Row 2 has no primary key, so the drag steps over it rather than stopping there. + expect(checkedKeys()).toEqual([true, true, false, true, false]); + }); + + it('clears a run when the drag starts on a row that was already ticked', () => { + render(); + fireEvent.click(selectAllCheckbox()); + expect(checkedKeys()).toEqual([true, true, false, true, true]); + + // The direction is fixed at mousedown by the row pressed, exactly as for a shift-click. + drag(0, [1, 3]); + + expect(checkedKeys()).toEqual([false, false, false, false, true]); + }); + + it('shrinks the range when the pointer backs off, leaving no trail', () => { + render(); + + fireEvent.mouseDown(gutterOf(0), { button: 0 }); + fireEvent.mouseOver(rows()[4]); + expect(checkedKeys()).toEqual([true, true, false, true, true]); + fireEvent.mouseOver(rows()[1]); + fireEvent.mouseUp(window); + + // Rows 3 and 4 were picked on the way out and must not stay picked on the way back. + expect(checkedKeys()).toEqual([true, true, false, false, false]); + }); + + it('puts rows back exactly as it found them, not merely unticked', () => { + render(); + // Row 4 is picked before the drag starts, so backing off must restore it, not clear it. + fireEvent.click(rowCheckboxes()[4]); + + fireEvent.mouseDown(gutterOf(0), { button: 0 }); + fireEvent.mouseOver(rows()[4]); + fireEvent.mouseOver(rows()[1]); + fireEvent.mouseUp(window); + + expect(checkedKeys()).toEqual([true, true, false, false, true]); + }); + + it('does not open the record editor for the rows it crosses', () => { + let opened = 0; + render( opened++} />); + + drag(0, [1, 3]); + // Releasing back over the row it started on would otherwise re-toggle it and open the editor. + fireEvent.mouseDown(gutterOf(0), { button: 0 }); + fireEvent.mouseOver(rows()[3]); + fireEvent.click(rows()[0]); + + expect(opened).toBe(0); + }); + + it('ignores a press that is not the left button', () => { + render(); + + fireEvent.mouseDown(gutterOf(0), { button: 2 }); + fireEvent.mouseOver(rows()[3]); + fireEvent.mouseUp(window); + + expect(checkedKeys()).toEqual([false, false, false, false, false]); + }); + + it('leaves a modified press to the click handlers rather than starting a drag', () => { + // Shift already means "extend from the anchor"; a drag would overwrite that anchor before + // the click could read it. + render(); + fireEvent.click(rowCheckboxes()[0]); + + fireEvent.mouseDown(gutterOf(3), { button: 0, shiftKey: true }); + fireEvent.mouseOver(rows()[4]); + fireEvent.mouseUp(window); + + expect(checkedKeys()).toEqual([true, false, false, false, false]); + }); + + it('stops extending once the button is released', () => { + render(); + + drag(0, [1]); + fireEvent.mouseOver(rows()[4]); + + expect(checkedKeys()).toEqual([true, true, false, false, false]); + }); +}); + describe('TableView column resizing', () => { it('renders a resize handle for each column header', () => { // Regression: the handle used to be gated on columnDef.enableResizing (never set), so it diff --git a/src/features/instance/databases/components/TableView.tsx b/src/features/instance/databases/components/TableView.tsx index 660dff313..f7835a622 100644 --- a/src/features/instance/databases/components/TableView.tsx +++ b/src/features/instance/databases/components/TableView.tsx @@ -21,7 +21,18 @@ import { RowData, useTable, } from '@tanstack/react-table'; -import { Dispatch, ReactNode, SetStateAction, useCallback, useLayoutEffect, useMemo, useRef, useState } from 'react'; +import { + Dispatch, + ReactNode, + SetStateAction, + useCallback, + useEffect, + useLayoutEffect, + useMemo, + useRef, + useState, +} from 'react'; +import type { MouseEvent, RefObject } from 'react'; import { UseFormReturn } from 'react-hook-form'; import { z } from 'zod'; import { ColumnFilters, ColumnFiltersSchema } from './ColumnFilters'; @@ -211,6 +222,91 @@ export function TableView({ // instead of ratcheting along behind the pointer. }, [rowSelection, selectableKeys, anchorKey]); + // Press on a row's gutter and drag: every row the pointer crosses joins the range. Same direction + // rule as a shift-click -- it is fixed at mousedown from the row pressed, so dragging out of a + // ticked row clears a run and out of an unticked one picks one. + // + // `useRef`, not state: this changes on every row crossed and nothing renders from it directly, so + // putting it in state would re-render the whole grid mid-drag for no visible gain. + const dragRef = useRef< + | { + originKey: unknown; + selected: boolean; + /** The selection as it stood when the drag began, so rows it backs off revert exactly. */ + snapshot: ReadonlySet; + applied: unknown[]; + lastKey: unknown; + } + | null + >(null); + // A drag that moved ends on a different row, so no click reaches the row it started on -- except + // when the pointer wanders back and releases there, which would re-toggle it. Reset per press. + const suppressClickRef = useRef(false); + + const beginRowDrag = useCallback( + (key: unknown, event: { button: number; shiftKey: boolean; ctrlKey: boolean; metaKey: boolean }) => { + suppressClickRef.current = false; + // Left button only, and never under a modifier: shift already means "extend from the anchor", + // and starting a drag would overwrite the anchor before the click could read it. + if ( + !rowSelection || key === undefined || event.button !== 0 || event.shiftKey || event.ctrlKey || event.metaKey + ) { + return; + } + dragRef.current = { + originKey: key, + selected: !rowSelection.selectedKeys.has(key), + snapshot: new Set(rowSelection.selectedKeys), + applied: [], + lastKey: key, + }; + }, + [rowSelection], + ); + + const dragOverRow = useCallback((key: unknown) => { + const drag = dragRef.current; + if (!drag || !rowSelection || key === undefined || key === drag.lastKey) { + return; + } + const originIndex = selectableKeys.indexOf(drag.originKey); + const targetIndex = selectableKeys.indexOf(key); + if (originIndex === -1 || targetIndex === -1) { + return; + } + drag.lastKey = key; + const [from, to] = originIndex <= targetIndex + ? [originIndex, targetIndex] + : [targetIndex, originIndex]; + const range = selectableKeys.slice(from, to + 1); + const inRange = new Set(range); + // Rows the drag has backed off go back to what they were when it started, so pulling the + // pointer in again shrinks the range instead of leaving a trail behind it. + const leaving = drag.applied.filter((applied) => !inRange.has(applied)); + const restoreOn = leaving.filter((left) => drag.snapshot.has(left)); + const restoreOff = leaving.filter((left) => !drag.snapshot.has(left)); + if (restoreOn.length) { + rowSelection.setRangeSelected(restoreOn, true); + } + if (restoreOff.length) { + rowSelection.setRangeSelected(restoreOff, false); + } + rowSelection.setRangeSelected(range, drag.selected); + drag.applied = range; + setAnchorKey(drag.originKey); + suppressClickRef.current = true; + }, [rowSelection, selectableKeys]); + + // The release that ends a drag lands wherever the pointer is -- often outside the grid, and + // sometimes outside the window -- so the listener has to be on the window rather than a row. + useEffect(() => { + const endDrag = () => { + dragRef.current = null; + }; + window.addEventListener('mouseup', endDrag); + return () => window.removeEventListener('mouseup', endDrag); + }, []); + const scrollContainerRef = useRef(null); const [scrollLeftAtResizeStart, setScrollLeftAtResizeStart] = useState(0); @@ -359,6 +455,9 @@ export function TableView({ primaryKey={primaryKey} rowSelection={isSelectable ? rowSelection : undefined} onSelectRow={selectRow} + onBeginDrag={beginRowDrag} + onDragOver={dragOverRow} + suppressClickRef={suppressClickRef} /> ))) : ( @@ -448,12 +547,15 @@ function SelectCellLabel({ children }: { children: ReactNode }) { } function TableBodyRow( - { row, primaryKey, onRowClick, rowSelection, onSelectRow }: { + { row, primaryKey, onRowClick, rowSelection, onSelectRow, onBeginDrag, onDragOver, suppressClickRef }: { row: Row; primaryKey?: string; onRowClick?: (row: Row) => void; rowSelection?: TableRowSelection; onSelectRow?: (key: unknown, extendRange: boolean) => void; + onBeginDrag?: (key: unknown, event: MouseEvent) => void; + onDragOver?: (key: unknown) => void; + suppressClickRef?: RefObject; }, ) { // TanStack memoizes getVisibleCells() and returns a fresh array whenever the @@ -484,14 +586,22 @@ function TableBodyRow( return ( onDragOver?.(selectionKey)} + // Shift-click, and a drag, would otherwise pull a text selection across the rows they + // span. The guard belongs on mousedown, where the selection starts -- preventing the + // click is already too late. onMouseDown={(event) => { if (rowSelection && event.shiftKey) { event.preventDefault(); } }} onClick={(event) => { + if (suppressClickRef?.current) { + // A drag ended back on the row it started from; it has already had its answer. + return; + } // A modified click selects where the pointer already is; shift extends from the anchor. // A row with no key to select by falls through to the editor rather than doing nothing. if (rowSelection && selectionKey !== undefined && (event.ctrlKey || event.metaKey || event.shiftKey)) { @@ -515,6 +625,14 @@ function TableBodyRow( )} // The row click opens the record editor; ticking the checkbox must not. onClick={onClickStopPropagation} + onMouseDown={(event) => { + // Stops the press pulling a text selection across the rows a drag crosses. + // It also stops the checkbox taking focus, so that is restored by hand rather + // than silently lost for anyone alternating mouse and keyboard. + event.preventDefault(); + event.currentTarget.querySelector('input')?.focus(); + onBeginDrag?.(selectionKey, event); + }} >