fix(browse): close the gaps a cross-model review found in drag selection

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<typeof import('./TableView')>(),
-	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(
+			<QueryClientProvider client={new QueryClient()}>
+				<DatabaseTableView databaseName="data" tableName="dog" />
+			</QueryClientProvider>,
+		);
+		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.

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<ReadonlySet<unknown>>(EMPTY_SELECTION, [selectionEpoch]);
+	const [selectedKeys, setSelectedKeys] = useEffectedState<ReadonlySet<unknown>>(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,
 	]);
 

src/features/instance/databases/components/TableView.test.tsx

@@ -138,9 +138,10 @@ const selectableRows: Record<string, unknown>[] = [
 ];
 
 function SelectionHarness(
-	{ onRowClick, rows = selectableRows }: {
+	{ onRowClick, rows = selectableRows, resultSetKey = 'page-0' }: {
 		onRowClick?: () => void;
 		rows?: Record<string, unknown>[];
+		resultSetKey?: string;
 	} = {},
 ) {
 	const columnFiltersForm = useForm<z.infer<typeof ColumnFiltersSchema>>({ 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(<SelectionHarness rows={rangeRows} resultSetKey="page-0" />);
+		fireEvent.click(checkboxes()[0]);
+		expect(checkedKeys()).toEqual([true, false, false, false, false]);
+
+		rerender(<SelectionHarness rows={rangeRows} resultSetKey="page-1" />);
+		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(<SelectionHarness rows={rangeRows} />);
 
 		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(<SelectionHarness rows={rangeRows} />);
 
 		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(<SelectionHarness rows={rangeRows} onRowClick={() => 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(<SelectionHarness rows={rangeRows} onRowClick={() => 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(<SelectionHarness rows={rangeRows} />);
+
+		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(<SelectionHarness rows={rangeRows} />);
+
+		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(<SelectionHarness rows={rangeRows} />);
 
 		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(<SelectionHarness rows={rangeRows} />);
+
+		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]);
 	});