diff --git a/src/features/instance/databases/components/DatabaseTableView.test.tsx b/src/features/instance/databases/components/DatabaseTableView.test.tsx index a3322b2ab..1fe904037 100644 --- a/src/features/instance/databases/components/DatabaseTableView.test.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.test.tsx @@ -30,8 +30,14 @@ vi.mock('@tanstack/react-router', () => { }; }); +const instanceClientState = vi.hoisted(() => ({ entityId: 'instance-1' })); + vi.mock('@/config/useInstanceClient', () => ({ - useInstanceClientIdParams: () => ({ entityId: 'instance-1', instanceClient: {}, entityType: 'instance' }), + useInstanceClientIdParams: () => ({ + entityId: instanceClientState.entityId, + instanceClient: {}, + entityType: 'instance', + }), })); vi.mock('@/hooks/useAuth', () => ({ @@ -74,18 +80,21 @@ const tableViewColumns = vi.hoisted(() => ({ current: [] as { accessorKey?: stri const tableViewSelection = vi.hoisted(() => ({ current: undefined as TableRowSelection | undefined, })); +const tableViewResultSet = vi.hoisted(() => ({ current: '' })); // 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 }: { + TableView: ({ columns, emptyState, rowSelection, resultSetKey }: { columns: { accessorKey?: string }[]; emptyState?: React.ReactNode; rowSelection?: TableRowSelection; + resultSetKey: string; }) => { tableViewColumns.current = columns; tableViewSelection.current = rowSelection; + tableViewResultSet.current = resultSetKey; // Rendering the slot is what lets a test follow a card click through to the launch it fires. return <>{emptyState}; }, @@ -142,6 +151,8 @@ afterEach(() => { permissionState.canDeleteRecords = true; tableViewColumns.current = []; tableViewSelection.current = undefined; + tableViewResultSet.current = ''; + instanceClientState.entityId = 'instance-1'; pageRows.current = []; watchedValues.calls = []; deleteRecords.mutate.mockReset(); @@ -421,6 +432,31 @@ describe('DatabaseTableView selection scope', () => { expect(deleteSelectedButton()).toBeNull(); }); + it('uses one visible-result identity across instance and cache-mode changes', () => { + const { rerender } = renderView(); + const initialResultSet = tableViewResultSet.current; + act(() => tableViewSelection.current!.toggleRow('abc')); + expect(deleteSelectedButton()).not.toBeNull(); + + instanceClientState.entityId = 'instance-2'; + rerender( + + + , + ); + const nextInstanceResultSet = tableViewResultSet.current; + expect(nextInstanceResultSet).not.toBe(initialResultSet); + expect(deleteSelectedButton()).toBeNull(); + + act(() => tableViewSelection.current!.toggleRow('abc')); + expect(deleteSelectedButton()).not.toBeNull(); + + openTableOptions(); + fireEvent.click(screen.getByRole('menuitem', { name: /Only If Cached/i })); + expect(tableViewResultSet.current).not.toBe(nextInstanceResultSet); + 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 7e17ef781..c2ee3a546 100644 --- a/src/features/instance/databases/components/DatabaseTableView.tsx +++ b/src/features/instance/databases/components/DatabaseTableView.tsx @@ -349,33 +349,24 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName const useFilteredList = filtersToggled && !!appliedSearchConditions; - // The scroll container in TableView is reused across all of these, so it needs to be told when the - // 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. + // One identity owns everything scoped to the visible rows: selection, the shift anchor, and scroll. + // It is flat so names containing dots stay distinct without repeatedly escaping nested JSON keys. const tableIdentity = JSON.stringify([databaseName, tableName]); const resultSetKey = JSON.stringify([ - tableIdentity, + instanceParams.entityId, + databaseName, + tableName, pageIndex, pageSize, sort, useFilteredList ? appliedSearchConditions : null, + onlyIfCached, ]); // 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 [selectedKeys, setSelectedKeys] = useEffectedState>(EMPTY_SELECTION, [resultSetKey]); const toggleRowSelected = useCallback((key: unknown) => { setSelectedKeys((current) => { const next = new Set(current); @@ -641,20 +632,30 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName }, { onSuccess: (response) => { + const incomplete = describeIncompleteDelete(response, hashes.length); + if (incomplete?.wroteNothing) { + // The record is still there, so nothing is stale and the user needs it in front + // of them to act on the message -- the same call `onWriteSettled` makes for a + // write that landed nothing. Refresh neither, and leave the editor open. + toast.error("The record wasn't deleted", { description: incomplete.message }); + return; + } // `refreshTable` also drops the selection: this record may well be one of the - // checked rows. + // checked rows. `refreshOpenRecord` is the second invalidation -- `refreshTable`'s + // prefix doesn't reach `search_by_id`, so without it reopening the row inside + // `gcTime` serves the record that was just deleted. void refreshTable(); + void refreshOpenRecord(); setIsEditModalOpen(false); - const incomplete = describeIncompleteDelete(response, hashes.length); if (incomplete) { - toast.error("The record wasn't deleted", { description: incomplete.message }); + toast.error("The record wasn't fully deleted", { description: incomplete.message }); return; } toast.success('Record deleted successfully'); }, }, ); - }, [deleteTableRecords, instanceParams, databaseName, tableName, refreshTable]); + }, [deleteTableRecords, instanceParams, databaseName, tableName, refreshTable, refreshOpenRecord]); // 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. @@ -676,8 +677,10 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName { onSuccess: (response) => { // `refreshTable` drops the selection -- these rows are exactly the ones that just - // changed underneath it. + // changed underneath it -- and `refreshOpenRecord` reaches the `search_by_id` entry + // it cannot, in case one of them is a record the editor still has cached. void refreshTable(); + void refreshOpenRecord(); const incomplete = describeIncompleteDelete(response, hashValues.length); if (incomplete) { toast.error("The records weren't all deleted", { description: incomplete.message }); @@ -693,6 +696,7 @@ export function DatabaseTableView({ instanceDatabaseMap, databaseName, tableName databaseName, tableName, refreshTable, + refreshOpenRecord, visibleSelectedKeys, ]); diff --git a/src/features/instance/databases/components/TableView.test.tsx b/src/features/instance/databases/components/TableView.test.tsx index 130345554..111cdf9d0 100644 --- a/src/features/instance/databases/components/TableView.test.tsx +++ b/src/features/instance/databases/components/TableView.test.tsx @@ -138,9 +138,10 @@ const selectableRows: Record[] = [ ]; function SelectionHarness( - { onRowClick, rows = selectableRows }: { + { onRowClick, rows = selectableRows, resultSetKey = 'page-0' }: { onRowClick?: () => void; rows?: Record[]; + resultSetKey?: string; } = {}, ) { const columnFiltersForm = useForm>({ defaultValues: {} }); @@ -159,7 +160,7 @@ function SelectionHarness( pageIndex={0} pageSize={20} primaryKey="id" - resultSetKey="page-0" + resultSetKey={resultSetKey} tableIdentity="dev.dog" rowSelection={{ selectedKeys, @@ -393,6 +394,21 @@ describe('TableView shift-click range selection', () => { expect(checkedKeys()).toEqual([false, false, false, true, true]); }); + it('drops the anchor when the result set changes underneath it', () => { + // TableView is not keyed, so it survives paging, sorting and a table switch while the parent + // clears the selection. An anchor left behind whose key exists in the next result set too + // (integer keys collide across tables constantly) would turn a plain shift-click into a range. + const { rerender } = render(); + fireEvent.click(checkboxes()[0]); + expect(checkedKeys()).toEqual([true, false, false, false, false]); + + rerender(); + fireEvent.click(checkboxes()[3], { shiftKey: true }); + + // A plain pick of row 3 only -- not a range measured from the stale anchor at row 0. + expect(checkedKeys()).toEqual([true, false, false, true, false]); + }); + it('falls back to a plain pick when there is no anchor yet', () => { renderRange(); @@ -532,7 +548,7 @@ describe('TableView drag selection', () => { function drag(from: number, over: number[]) { fireEvent.mouseDown(gutterOf(from), { button: 0 }); for (const index of over) { - fireEvent.mouseOver(rows()[index]); + fireEvent.mouseOver(rows()[index], { buttons: 1 }); } fireEvent.mouseUp(window); } @@ -561,9 +577,9 @@ describe('TableView drag selection', () => { render(); fireEvent.mouseDown(gutterOf(0), { button: 0 }); - fireEvent.mouseOver(rows()[4]); + fireEvent.mouseOver(rows()[4], { buttons: 1 }); expect(checkedKeys()).toEqual([true, true, false, true, true]); - fireEvent.mouseOver(rows()[1]); + fireEvent.mouseOver(rows()[1], { buttons: 1 }); fireEvent.mouseUp(window); // Rows 3 and 4 were picked on the way out and must not stay picked on the way back. @@ -576,8 +592,8 @@ describe('TableView drag selection', () => { fireEvent.click(rowCheckboxes()[4]); fireEvent.mouseDown(gutterOf(0), { button: 0 }); - fireEvent.mouseOver(rows()[4]); - fireEvent.mouseOver(rows()[1]); + fireEvent.mouseOver(rows()[4], { buttons: 1 }); + fireEvent.mouseOver(rows()[1], { buttons: 1 }); fireEvent.mouseUp(window); expect(checkedKeys()).toEqual([true, true, false, false, true]); @@ -590,7 +606,7 @@ describe('TableView drag selection', () => { 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.mouseOver(rows()[3], { buttons: 1 }); fireEvent.click(rows()[0]); expect(opened).toBe(0); @@ -600,7 +616,7 @@ describe('TableView drag selection', () => { render(); fireEvent.mouseDown(gutterOf(0), { button: 2 }); - fireEvent.mouseOver(rows()[3]); + fireEvent.mouseOver(rows()[3], { buttons: 1 }); fireEvent.mouseUp(window); expect(checkedKeys()).toEqual([false, false, false, false, false]); @@ -613,17 +629,92 @@ describe('TableView drag selection', () => { fireEvent.click(rowCheckboxes()[0]); fireEvent.mouseDown(gutterOf(3), { button: 0, shiftKey: true }); - fireEvent.mouseOver(rows()[4]); + fireEvent.mouseOver(rows()[4], { buttons: 1 }); fireEvent.mouseUp(window); expect(checkedKeys()).toEqual([true, false, false, false, false]); }); + it('leaves the next click alone once the drag is over', () => { + // A drag that ends on another row produces no click at all, so the suppression it armed has + // to be cleared by the next press -- otherwise it stands there and eats an unrelated click, + // and the record editor stops opening. + let opened = 0; + render( opened++} />); + + drag(0, [3]); + fireEvent.mouseDown(rows()[1]); + fireEvent.click(rows()[1]); + + expect(opened).toBe(1); + }); + + it('does not open the record editor when a press slips out of the gutter mid-click', () => { + // Press the 32px gutter, drift into the row's data cell, release. mousedown and mouseup share + // no cell, so the click resolves on the `tr` itself and the gutter's stopPropagation never + // sees it -- the editor would open on what was meant to be a tick. + let opened = 0; + render( opened++} />); + + fireEvent.mouseDown(gutterOf(0), { button: 0 }); + fireEvent.mouseUp(window); + fireEvent.click(rows()[0]); + + expect(opened).toBe(0); + }); + + it('keeps the origin row picked when a drag wanders back to its checkbox', () => { + // The drag already applied the state to the origin; the trailing click on the very checkbox it + // started from would toggle that one row straight back off. + render(); + + fireEvent.mouseDown(gutterOf(0), { button: 0 }); + fireEvent.mouseOver(rows()[3], { buttons: 1 }); + fireEvent.mouseOver(rows()[0], { buttons: 1 }); + fireEvent.mouseUp(window); + fireEvent.click(rowCheckboxes()[0], { detail: 1 }); + + expect(checkedKeys()[0]).toBe(true); + }); + + it('lets the focused checkbox be toggled from the keyboard after a drag', () => { + render(); + + fireEvent.mouseDown(gutterOf(0), { button: 0 }); + fireEvent.mouseOver(rows()[3], { buttons: 1 }); + fireEvent.mouseUp(window); + expect(document.activeElement).toBe(rowCheckboxes()[0]); + + // Keyboard activation produces a click with no click count (`detail === 0`). + fireEvent.click(rowCheckboxes()[0], { detail: 0 }); + expect(checkedKeys()[0]).toBe(false); + + fireEvent.click(rowCheckboxes()[0], { detail: 0 }); + expect(checkedKeys()[0]).toBe(true); + }); + it('stops extending once the button is released', () => { render(); drag(0, [1]); - fireEvent.mouseOver(rows()[4]); + fireEvent.mouseOver(rows()[4], { buttons: 1 }); + + expect(checkedKeys()).toEqual([true, true, false, false, false]); + }); + + it('stops extending when the button was released outside the window', () => { + // No mouseup reaches us in that case, so the only account of the button no longer being held + // is what each mouseover reports. Without it the drag stays live and carries on selecting as + // soon as the pointer comes back over the grid. + render(); + + fireEvent.mouseDown(gutterOf(0), { button: 0 }); + fireEvent.mouseOver(rows()[1], { buttons: 1 }); + expect(checkedKeys()).toEqual([true, true, false, false, false]); + + // Pointer comes back over the grid with nothing held down. + fireEvent.mouseOver(rows()[3], { buttons: 0 }); + fireEvent.mouseOver(rows()[4], { buttons: 1 }); expect(checkedKeys()).toEqual([true, true, false, false, false]); }); diff --git a/src/features/instance/databases/components/TableView.tsx b/src/features/instance/databases/components/TableView.tsx index f7835a622..ae6eacb38 100644 --- a/src/features/instance/databases/components/TableView.tsx +++ b/src/features/instance/databases/components/TableView.tsx @@ -101,7 +101,7 @@ interface BrowseDataTableProps { 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. + // Canonical identity for the rows on screen (instance + table + page + sort + filters + cache mode). resultSetKey: string; // Identifies which table is on screen, so a new set of columns starts scrolled to the left. tableIdentity: string; @@ -184,18 +184,10 @@ export function TableView({ 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. + // Where a shift-click measures FROM -- a position only. The direction comes from the row clicked, + // so a shift-click always does what the row under the pointer is visibly about to do. Held as a + // key, not an index, so a row set that moves underneath it fails to find the anchor and degrades + // to a plain pick rather than measuring from whatever row now sits there. const [anchorKey, setAnchorKey] = useState(undefined); const selectRow = useCallback((key: unknown, extendRange: boolean) => { if (!rowSelection) { @@ -215,19 +207,12 @@ export function TableView({ // 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. + // The anchor stays put, so shift-clicking further out re-measures from the same origin. }, [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. + // Drag state. Direction is fixed at mousedown by the row pressed, as for a shift-click. `useRef` + // because it changes on every row crossed and nothing renders from it directly. const dragRef = useRef< | { originKey: unknown; @@ -239,13 +224,17 @@ export function TableView({ } | 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); + // A press that began in the gutter must never reach the row's own click, whatever it does next. + // Releasing on a different cell of the SAME row resolves the click on the `tr`, which the gutter + // cell's stopPropagation can't catch, and the record editor opens on what was meant to be a tick. + const pressedInGutterRef = useRef(false); + // Whether a drag actually crossed a row. The checkbox consults this separately: a drag that + // wanders back and releases on the checkbox it started from would otherwise toggle the origin + // straight back off, leaving the one row the pointer never left unselected. + const dragMovedRef = 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 ( @@ -253,6 +242,7 @@ export function TableView({ ) { return; } + pressedInGutterRef.current = true; dragRef.current = { originKey: key, selected: !rowSelection.selectedKeys.has(key), @@ -264,9 +254,19 @@ export function TableView({ [rowSelection], ); - const dragOverRow = useCallback((key: unknown) => { + const dragOverRow = useCallback((key: unknown, buttonsHeld: number) => { const drag = dragRef.current; - if (!drag || !rowSelection || key === undefined || key === drag.lastKey) { + if (!drag) { + return; + } + // Releasing outside the window delivers no mouseup, so the drag would still be live when the + // pointer came back and would carry on selecting with nothing held down. Every mouseover + // reports which buttons are actually down, which is the only account of that we get. + if ((buttonsHeld & 1) === 0) { + dragRef.current = null; + return; + } + if (!rowSelection || key === undefined || key === drag.lastKey) { return; } const originIndex = selectableKeys.indexOf(drag.originKey); @@ -294,19 +294,44 @@ export function TableView({ rowSelection.setRangeSelected(range, drag.selected); drag.applied = range; setAnchorKey(drag.originKey); - suppressClickRef.current = true; + dragMovedRef.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. + // + // The press is watched here too, and that is load-bearing rather than tidy: a drag that ends on a + // different row produces no click at all, so the flags it armed would still be standing when the + // user next clicked a row, and swallow that click instead. Clearing them on every press means + // only the press's own trailing click can be suppressed. It has to be the CAPTURE phase, which + // runs before the gutter's own mousedown -- on the bubble phase this would undo the flag the + // gutter had just set. useEffect(() => { const endDrag = () => { dragRef.current = null; }; + const beginPress = () => { + pressedInGutterRef.current = false; + dragMovedRef.current = false; + }; window.addEventListener('mouseup', endDrag); - return () => window.removeEventListener('mouseup', endDrag); + window.addEventListener('mousedown', beginPress, true); + return () => { + window.removeEventListener('mouseup', endDrag); + window.removeEventListener('mousedown', beginPress, true); + }; }, []); + // The shift anchor belongs to the rows on screen, exactly as the selection does. `TableView` is + // rendered without a key, so it survives paging, sorting, a page-size change and a table switch; + // an anchor left over from the last result set whose primary-key value happens to exist in this + // one (integer keys collide across tables constantly) would turn the next plain shift-click into + // a range over rows the user never anchored. + useLayoutEffect(() => { + setAnchorKey(undefined); + dragRef.current = null; + }, [resultSetKey]); + const scrollContainerRef = useRef(null); const [scrollLeftAtResizeStart, setScrollLeftAtResizeStart] = useState(0); @@ -457,7 +482,8 @@ export function TableView({ onSelectRow={selectRow} onBeginDrag={beginRowDrag} onDragOver={dragOverRow} - suppressClickRef={suppressClickRef} + pressedInGutterRef={pressedInGutterRef} + dragMovedRef={dragMovedRef} /> ))) : ( @@ -547,16 +573,18 @@ function SelectCellLabel({ children }: { children: ReactNode }) { } function TableBodyRow( - { 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; - }, + { row, primaryKey, onRowClick, rowSelection, onSelectRow, onBeginDrag, onDragOver, pressedInGutterRef, dragMovedRef }: + { + 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, buttonsHeld: number) => void; + pressedInGutterRef?: RefObject; + dragMovedRef?: 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 @@ -588,7 +616,7 @@ function TableBodyRow( 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)} + onMouseOver={(event) => onDragOver?.(selectionKey, event.buttons)} // 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. @@ -598,8 +626,8 @@ function TableBodyRow( } }} onClick={(event) => { - if (suppressClickRef?.current) { - // A drag ended back on the row it started from; it has already had its answer. + if (pressedInGutterRef?.current) { + // This press began on a checkbox; the gutter owns it however it ended. return; } // A modified click selects where the pointer already is; shift extends from the anchor. @@ -650,9 +678,12 @@ function TableBodyRow( // 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); + // A drag that crossed rows has already set this one; a trailing click here + // would toggle the row it started from straight back off. + if ((dragMovedRef?.current && event.detail !== 0) || selectionKey === undefined) { + return; } + onSelectRow?.(selectionKey, event.shiftKey); }} onChange={noopChange} /> diff --git a/src/integrations/api/instance/database/deleteTableRecords.test.ts b/src/integrations/api/instance/database/deleteTableRecords.test.ts index 36ae8a57a..deeb00be3 100644 --- a/src/integrations/api/instance/database/deleteTableRecords.test.ts +++ b/src/integrations/api/instance/database/deleteTableRecords.test.ts @@ -6,8 +6,13 @@ describe('describeIncompleteDelete', () => { 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: [] }]) { + // `skipped_hashes` only adds detail; an absent one must not turn a provable delete into an error. + it('reads an absent skipped list as nothing skipped', () => { + expect(describeIncompleteDelete({ deleted_hashes: ['a', 'b'] }, 2)).toBeUndefined(); + }); + + it('treats an absent deleted list as unproven', () => { + for (const response of [{ message: 'deleted' }, undefined, { skipped_hashes: [] }]) { const incomplete = describeIncompleteDelete(response, 2); expect(incomplete?.message).toContain("didn't report which records"); expect(incomplete?.wroteNothing).toBe(false); diff --git a/src/integrations/api/instance/database/deleteTableRecords.ts b/src/integrations/api/instance/database/deleteTableRecords.ts index faad8c844..d7f3b752d 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, UNREADABLE_WRITE_MESSAGE } from './incompleteWrite'; +import { IncompleteWrite, isMalformedHashes, UNREADABLE_WRITE_MESSAGE } from './incompleteWrite'; interface DeleteTableRecordsData extends InstanceClientConfig { databaseName: string; @@ -23,20 +23,24 @@ export interface DeleteTableRecordsResponse { * 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. + * `deleted_hashes` is required, the same way `describeIncompletePut` requires `put_hashes`: `delete` + * has answered with it since 4.7.33, so an absent one is not a legacy responder but an empty body or + * an HTML 2xx from something in front of Harper, and reporting that as a clean delete would claim + * rows are gone on no evidence at all. `skipped_hashes` only adds detail, so an absent one reads as + * "nothing skipped" rather than turning a provable delete into an error; present-but-not-an-array + * is unreadable either way. */ export function describeIncompleteDelete( data: DeleteTableRecordsResponse | undefined, recordCount: number, ): IncompleteWrite | undefined { - if (!Array.isArray(data?.deleted_hashes) || !Array.isArray(data?.skipped_hashes)) { + if (!Array.isArray(data?.deleted_hashes) || isMalformedHashes(data?.skipped_hashes)) { return { message: UNREADABLE_WRITE_MESSAGE, wroteNothing: false, }; } - const skipped = data.skipped_hashes.length; + const skipped = data.skipped_hashes?.length ?? 0; const deleted = data.deleted_hashes.length; if (skipped === 0 && deleted >= recordCount) { return undefined;