Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/features/instance/databases/components/ColumnFilters.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,13 @@ export function ColumnFilters<TData extends RowData>({
applyFilters,
columnFiltersForm,
headerGroups,
selectColumnWidth,
}: {
applyFilters: () => void;
columnFiltersForm: UseFormReturn<z.infer<typeof ColumnFiltersSchema>>;
headerGroups: HeaderGroup<TData>[];
/** 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') {
Expand All @@ -38,6 +41,15 @@ export function ColumnFilters<TData extends RowData>({
<Form {...columnFiltersForm}>
{headerGroups.map((headerGroup) => (
<TableRow key={headerGroup.id} className="border-none">
{selectColumnWidth !== undefined && (
<TableCell
style={{ width: `${selectColumnWidth}px` }}
// The right divider is an inset shadow, not `border-r` — see
// SELECT_COLUMN_DIVIDER in TableView: a collapsed border doesn't travel
// with a sticky cell.
className="sticky top-10 left-0 z-20 bg-card dark:bg-black-dark border-b border-border shadow-[inset_-1px_0_0_var(--color-border)]"
/>
)}
Comment on lines +44 to +52

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Using aria-hidden on a TableCell inside a visible table row breaks the table grid structure for screen readers, as they expect every row to have the same number of cells. Removing aria-hidden from this placeholder cell ensures the table structure remains consistent and accessible.

Suggested change
{selectColumnWidth !== undefined && (
<TableCell
aria-hidden
style={{ width: `${selectColumnWidth}px` }}
// The right divider is an inset shadow, not `border-r` — see
// SELECT_COLUMN_DIVIDER in TableView: a collapsed border doesn't travel
// with a sticky cell.
className="sticky top-10 left-0 z-20 bg-card dark:bg-black-dark border-b border-border shadow-[inset_-1px_0_0_var(--color-border)]"
/>
)}
{selectColumnWidth !== undefined && (
<TableCell
style={{ width: `${selectColumnWidth}px` }}
// The right divider is an inset shadow, not `border-r` — see
// SELECT_COLUMN_DIVIDER in TableView: a collapsed border doesn't travel
// with a sticky cell.
className="sticky top-10 left-0 z-20 bg-card dark:bg-black-dark border-b border-border shadow-[inset_-1px_0_0_var(--color-border)]"
/>
)}

{headerGroup.headers.map((header) => {
const relationshipInfo = header.column.columnDef.meta?.relationshipInfo;
return (
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ export function DatabaseOverview({ instanceDatabaseMap, databaseName }: {
}, [navigate, params, databaseName]);

return (
<div className="pt-15 pb-4 pr-4">
<div className="pt-15 pb-4 px-4">
<div className="flex flex-col md:flex-row items-start md:items-center justify-between gap-3 pb-6">
<div className="flex items-center gap-2 min-w-0">
<h1 className="text-3xl truncate">{databaseName}</h1>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,16 +3,19 @@
*/
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.
const permissionState = vi.hoisted(() => ({
canManageBrowseInstance: true,
canImportFromFile: true,
allowedSources: ['csv-data', 'csv-url', 'json-records'] as string[],
canDeleteRecords: true,
}));

vi.mock('@tanstack/react-router', () => {
Expand Down Expand Up @@ -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,
}));

Expand All @@ -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<typeof import('./TableView')>(),
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<typeof import('@/integrations/api/instance/database/deleteTableRecords')>(),
useDeleteTableRecords: () => ({ mutate: deleteRecords.mutate, isPending: false }),
}));
vi.mock('./PickColumnsDropdown', () => ({ PickColumnsDropdown: () => null }));
vi.mock('../modals/EditTableRowModal', () => ({ EditTableRowModal: () => null }));

Expand All @@ -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<string, unknown>[] }));

vi.mock('@tanstack/react-query', async (importOriginal) => {
const actual = await importOriginal<typeof import('@tanstack/react-query')>();
Expand All @@ -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() },
};
});
Expand All @@ -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 = {
Expand All @@ -119,9 +154,10 @@ const dogTable = {
} as unknown as InstanceTable;

function renderView(
{ instanceDatabaseMap }: { instanceDatabaseMap?: InstanceDatabaseMap } = {},
{ instanceDatabaseMap, rows }: { instanceDatabaseMap?: InstanceDatabaseMap; rows?: Record<string, unknown>[] } = {},
) {
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(
<QueryClientProvider client={queryClient}>
Expand Down Expand Up @@ -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(
<QueryClientProvider client={new QueryClient()}>
<DatabaseTableView databaseName="a.b" tableName="c" />
</QueryClientProvider>,
);
act(() => tableViewSelection.current!.toggleRow(42));
expect(deleteSelectedButton()).not.toBeNull();

rerender(
<QueryClientProvider client={new QueryClient()}>
<DatabaseTableView databaseName="a" tableName="b.c" />
</QueryClientProvider>,
);

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

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

act(() => tableViewSelection.current!.toggleRow(Object.prototype.constructor));

expect(deleteSelectedButton()).toBeNull();
});
});
Loading