diff --git a/src/features/instance/databases/components/ColumnFilters.tsx b/src/features/instance/databases/components/ColumnFilters.tsx index e131cd4cb..5a4e4c91b 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,15 @@ 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/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/DatabaseTableView.test.tsx b/src/features/instance/databases/components/DatabaseTableView.test.tsx index dd0818461..a3322b2ab 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,215 @@ 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('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. + 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..7e17ef781 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; @@ -346,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, @@ -355,6 +362,59 @@ 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]); + // 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) { + if (selected) { + next.add(key); + } else { + next.delete(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, + setRangeSelected, + } + : undefined, [canDeleteRecords, selectedKeys, toggleRowSelected, toggleAllSelected, setRangeSelected]); + // Full list const searchByValueParams = { ...instanceParams, @@ -402,6 +462,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 +515,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 +640,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 +832,7 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName return ( <> -
+
{canAddRecords && ( )} + {canDeleteRecords && visibleSelectedKeys.length > 0 && ( + + )}
@@ -758,7 +896,7 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName onClick={onRefreshClick} disabled={isFetching} > - + -
+
{/* Summary — record count is the essential, kept at every width */}
diff --git a/src/features/instance/databases/components/TableView.test.tsx b/src/features/instance/databases/components/TableView.test.tsx index e26cbfb43..130345554 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,6 +129,506 @@ describe('TableView sorting', () => { }); }); +// 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()), + setRangeSelected: (keys, selected) => + setSelectedKeys((current) => { + const next = new Set(current); + for (const key of keys) { + if (selected) { + next.add(key); + } else { + next.delete(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"]')); +} + +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); + }); + + 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()), + setRangeSelected: (keys, selected) => + setSelectedKeys((current) => { + const next = new Set(current); + for (const key of keys) { + if (selected) { + next.add(key); + } else { + next.delete(key); + } + } + return next; + }), + }} + 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); + }); + + it.each([ + ['Control', { ctrlKey: true }], + ['Command', { metaKey: 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 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 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 }); + expect(checkedKeys()).toEqual([true, true, false, true, true]); + + fireEvent.click(checkboxes()[1], { shiftKey: true }); + + expect(checkedKeys()).toEqual([false, false, 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()[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, 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 }); + + // 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('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()[1], { shiftKey: true }); + expect(checkedKeys()).toEqual([true, true, false, false, false]); + + fireEvent.click(checkboxes()[4], { shiftKey: true }); + + expect(checkedKeys()).toEqual([true, true, false, true, true]); + }); + + 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++); + 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('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([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', () => { + 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 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 9587cebc7..f7835a622 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,67 @@ import { RowData, useTable, } from '@tanstack/react-table'; -import { Dispatch, ReactNode, SetStateAction, 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'; 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; + /** Applies one state to a whole shift-click range. */ + setRangeSelected: (keys: unknown[], selected: boolean) => void; +} + interface BrowseDataTableProps { applyFilters: () => void; columnFiltersForm: UseFormReturn>; @@ -43,6 +99,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 +131,7 @@ export function TableView({ pageIndex, pageSize, primaryKey, + rowSelection, resultSetKey, tableIdentity, setPageIndex, @@ -107,6 +166,147 @@ 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; + + // 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. + // + // 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 ? 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. + // + // 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]); + + // 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); @@ -143,7 +343,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 +393,27 @@ 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); + setAnchorKey(allSelected ? undefined : selectableKeys[0]); + }} + /> + + )} {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} + onSelectRow={selectRow} + onBeginDrag={beginRowDrag} + onDragOver={dragOverRow} + suppressClickRef={suppressClickRef} /> ))) : ( - + {isFetching || isAwaitingRows ? : No results.} @@ -270,8 +505,58 @@ 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); + useLayoutEffect(() => { + 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, 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 // visible columns change, so depending on it keeps the body in step with the @@ -295,12 +580,85 @@ 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)} + data-state={isSelected ? 'selected' : undefined} + // `mouseOver`, not `mouseEnter`: React synthesises enter from over/out, and only the + // bubbling form is reliable when the pointer crosses into a child cell mid-drag. + onMouseOver={() => 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)) { + event.preventDefault(); + onSelectRow?.(selectionKey, event.shiftKey); + return; + } + onRowClick?.(row); + }} className={cn('hover:bg-muted/10 data-[state=selected]:bg-muted', onRowClick && 'cursor-pointer')} > + {rowSelection && ( + { + // 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); + }} + > + + { + if (selectionKey !== undefined) { + onSelectRow?.(selectionKey, event.shiftKey); + } + }} + onChange={noopChange} + /> + + + )} {cells} {/* Filler cell matching the header's filler column. */} @@ -308,6 +666,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 ( 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). */ }
{ 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. -
+
{ + it('reads a clean delete as complete', () => { + expect(describeIncompleteDelete({ deleted_hashes: ['a', 'b'], skipped_hashes: [] }, 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', () => { + 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); + }); + + 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..faad8c844 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, UNREADABLE_WRITE_MESSAGE } from './incompleteWrite'; interface DeleteTableRecordsData extends InstanceClientConfig { databaseName: string; @@ -7,10 +8,51 @@ 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. + * + * 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 (!Array.isArray(data?.deleted_hashes) || !Array.isArray(data?.skipped_hashes)) { + return { + message: UNREADABLE_WRITE_MESSAGE, + wroteNothing: false, + }; + } + const skipped = data.skipped_hashes.length; + const deleted = data.deleted_hashes.length; + 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: 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,