From 7b0ffcccd063f680fe94f7fd004f93073332e2a1 Mon Sep 17 00:00:00 2001 From: Chris Barber Date: Mon, 14 Sep 2026 16:09:39 -0500 Subject: [PATCH] fix(browse): close the gaps a cross-model review found in drag selection Four reviewers (codex, gemini, cursor-grok, Harper domain) on the whole branch. The blocker: a drag armed click suppression and nothing ever disarmed it. The only reset ran on a gutter press, so once any drag finished, every later row click was swallowed -- the record editor stopped opening and modifier selection went dead until the user happened to press a checkbox again. The flags are now cleared on every press, in the CAPTURE phase so the reset lands before the gutter's own mousedown rather than undoing it. That split the one flag into the two things it was conflating. A press that began in the gutter now suppresses the row's click however it ends, which also fixes a slip nobody had tried: press the 32px gutter, drift into the same row's data cell, release -- the click resolves on the `tr`, where the gutter cell's stopPropagation never sees it, and the record editor opened over what was meant to be a tick. Separately the checkbox now ignores a click that a drag already answered, so wandering back and releasing on the checkbox it started from no longer toggles that one row off again. The shift anchor is reset with the result set. `TableView` is rendered without a key, so it outlives paging, sorting and a table switch while the parent clears the selection; a leftover anchor whose primary-key value also exists in the next result set -- integer keys collide across tables constantly -- turned the next plain shift-click into a range over rows nobody anchored. Delete now shares `onWriteSettled`'s policy rather than half of it: a delete that answers `wroteNothing` leaves the editor open on the record the user has to act on, and both delete paths invalidate the open record as well as the table, which `refreshTable`'s prefix cannot reach. `describeIncompleteDelete` keeps failing closed on a fieldless answer, which is what this PR's review asked for, but no longer requires `skipped_hashes`: that list only adds detail, and demanding it would have turned a provable delete into an error toast. It now matches `describeIncompletePut` exactly -- require the list that proves the outcome, tolerate an absent companion, reject a present non-array. Also flattened the cache keys, which nested their own stringified output three deep, and pruned the narration the review called out. Co-Authored-By: Claude Opus 5 (1M context) --- .../components/DatabaseTableView.test.tsx | 40 +++++- .../components/DatabaseTableView.tsx | 46 ++++--- .../databases/components/TableView.test.tsx | 113 ++++++++++++++-- .../databases/components/TableView.tsx | 125 +++++++++++------- .../database/deleteTableRecords.test.ts | 9 +- .../instance/database/deleteTableRecords.ts | 14 +- 6 files changed, 259 insertions(+), 88 deletions(-) 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;