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
10 changes: 9 additions & 1 deletion src/features/instance/databases/DESIGN.md
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,15 @@ schema load. Add Records, Import and Export CSV need the key itself, so they wai
that names one (`hasKnownPrimaryKey`, in the toolbar and in
[`TableContextMenuItems`](components/TableContextMenuItems.tsx), the tree and overview menus). A
table nothing has described yet gets the same answer as one without a key, rather than an Export
that then reports "no primary key found" (#1941).
that then reports "no primary key found" (#1941). The menus' Drop Table waits for the map to hold
the table too: one the map no longer holds may already have been dropped. The table view needs no
such gate, because `resolveDatabasesRedirect` leaves a table the map no longer holds. Browse reads
the describe map and the schema-relationship map by database and table name only through
[`describedTable.ts`](functions/describedTable.ts), which takes their own keys, so a table named like
an `Object.prototype` member (`constructor`, `toString`) is never found there by mistake.

A Radix menu item runs its `onClick` even while it is disabled, and only CSS keeps a pointer off it,
so an item that can be disabled takes its action through `onSelect`, which Radix gates.

`$id` is not a stand-in key. These are live-probe observations on `harperdb:4.7.36` and
`harper-pro:5.2.13` (2026-09), not covered by a test here:
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { useInstanceClientIdParams } from '@/config/useInstanceClient';
import {
declaredRelationships,
describedDatabase,
describedTable,
} from '@/features/instance/databases/functions/describedTable';
import { getSchemaRelationshipsQueryOptions } from '@/features/instance/databases/functions/schemaRelationships';
import { AddTableRowModal } from '@/features/instance/databases/modals/AddTableRowModal';
import { CreateNewTableModal } from '@/features/instance/databases/modals/CreateNewTableModal';
Expand Down Expand Up @@ -55,7 +60,7 @@ export function DatabaseActionModals({ instanceDatabaseMap }: { instanceDatabase
);

const addTableFromMap = addTarget
? instanceDatabaseMap?.[addTarget.databaseName]?.[addTarget.tableName]
? describedTable(instanceDatabaseMap, addTarget.databaseName, addTarget.tableName)
: undefined;
const [describedAddTable, setDescribedAddTable] = useState<{ target: AddTarget; table: InstanceTable }>();
const addInstanceTable = addTableFromMap
Expand Down Expand Up @@ -123,8 +128,12 @@ export function DatabaseActionModals({ instanceDatabaseMap }: { instanceDatabase
<AddTableRowModal
key={`add-${addTarget.databaseName}/${addTarget.tableName}`}
instanceTable={addInstanceTable}
databaseTables={instanceDatabaseMap?.[addTarget.databaseName]}
schemaRelationships={schemaRelationshipMap?.[addTarget.databaseName]?.[addTarget.tableName]}
databaseTables={describedDatabase(instanceDatabaseMap, addTarget.databaseName)}
schemaRelationships={declaredRelationships(
schemaRelationshipMap,
addTarget.databaseName,
addTarget.tableName,
)}
isModalOpen
setIsModalOpen={open => {
if (!open) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { DropdownMenu, DropdownMenuContent, DropdownMenuItem, DropdownMenuTrigge
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip';
import { useInstanceClientIdParams } from '@/config/useInstanceClient';
import { TableRowContextMenu } from '@/features/instance/databases/components/TableRowContextMenu';
import { describedDatabase, describedTableNames } from '@/features/instance/databases/functions/describedTable';
import { summarizeDatabaseCounts } from '@/features/instance/databases/functions/summarizeDatabaseCounts';
import { useInstanceBrowseManagePermission, useInstanceImportOperationsPermission } from '@/hooks/usePermissions';
import { InstanceDatabaseMap } from '@/integrations/api/api.patch';
Expand Down Expand Up @@ -58,8 +59,8 @@ export function DatabaseOverview({ instanceDatabaseMap, databaseName }: {
const canManage = useInstanceBrowseManagePermission();
const canImport = useInstanceImportOperationsPermission();

const tables = instanceDatabaseMap?.[databaseName] ?? {};
const tableNames = Object.keys(tables).sort();
const tables = describedDatabase(instanceDatabaseMap, databaseName) ?? {};
const tableNames = describedTableNames(instanceDatabaseMap, databaseName);

// Sizes/schema are already in the fast (count-skipping) map, so they render instantly. Row counts
// aren't, so fetch them per-table via describe_table's cheap estimate path -- but only for the
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -45,9 +45,9 @@ vi.mock('@/hooks/useAuth', () => ({
useStaffPermission: () => staffState.isInstanceOperator,
}));

const cleanupOrphanBlobs = vi.hoisted(() => ({ mutate: vi.fn() }));
const cleanupOrphanBlobs = vi.hoisted(() => ({ mutate: vi.fn(), isPending: false }));
vi.mock('@/integrations/api/instance/database/cleanupOrphanBlobs', () => ({
useCleanupOrphanBlobsMutation: () => ({ mutate: cleanupOrphanBlobs.mutate, isPending: false }),
useCleanupOrphanBlobsMutation: () => ({ mutate: cleanupOrphanBlobs.mutate, isPending: cleanupOrphanBlobs.isPending }),
}));

vi.mock('@/hooks/usePermissions', () => ({
Expand Down Expand Up @@ -174,6 +174,7 @@ afterEach(() => {
watchedValues.calls = [];
deleteRecords.mutate.mockReset();
cleanupOrphanBlobs.mutate.mockReset();
cleanupOrphanBlobs.isPending = false;
routerState.params = {};
staffState.isInstanceOperator = false;
vi.restoreAllMocks();
Expand Down Expand Up @@ -637,6 +638,22 @@ describe('DatabaseTableView asking before an action', () => {
expect(document.body.style.pointerEvents).not.toBe('none');
});

it('does not ask again while a cleanup is already running', () => {
staffState.isInstanceOperator = true;
routerState.params = { instanceId: 'instance-1' };
cleanupOrphanBlobs.isPending = true;
renderView();

openTableOptions();
const cleanup = screen.getByRole('menuitem', { name: 'Cleanup Orphan Blobs' });
expect(cleanup.getAttribute('aria-disabled')).toBe('true');
fireEvent.click(cleanup);
fireEvent.keyDown(cleanup, { key: 'Enter' });

expect(question('Clean up orphan blobs?')).toBeNull();
expect(cleanupOrphanBlobs.mutate).not.toHaveBeenCalled();
});

// The view stays mounted when the route moves to another table, so a question asked about one
// must not be answerable once the view shows the other.
it('drops a question once the view is showing another table, and does not bring it back', () => {
Expand Down
18 changes: 12 additions & 6 deletions src/features/instance/databases/components/DatabaseTableView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,12 @@ import {
DropdownMenuTrigger,
} from '@/components/ui/dropdownMenu';
import { useInstanceClientIdParams } from '@/config/useInstanceClient';
import {
declaredRelationships,
describedDatabase,
describedTable,
describedTableNames,
} from '@/features/instance/databases/functions/describedTable';
import {
ExtraRecordAttribute,
extraRecordAttributes,
Expand Down Expand Up @@ -169,8 +175,8 @@ export function DatabaseTableView({
// describe_table backfills this table's count asynchronously. Render schema from whichever arrives first
// -- the map is usually ready before describe_table -- so columns and records are never gated on the
// (slower) count scan. describe_table wins once present: it's the per-table, refresh-invalidated copy.
const databaseTables = instanceDatabaseMap?.[databaseName];
const tableFromMap = databaseTables?.[tableName];
const databaseTables = describedDatabase(instanceDatabaseMap, databaseName);
const tableFromMap = describedTable(instanceDatabaseMap, databaseName, tableName);
const instanceTable = describeTableData ?? tableFromMap;

// What the empty state may actually invite, each gated on the source it would use rather than on
Expand All @@ -195,7 +201,7 @@ export function DatabaseTableView({
const hasReplaceGrants = useInstanceTablePutPermission(instanceId ?? clusterId, databaseName, tableName);
const replaceBlockedReason = replaceRecordsBlockedReason(registrationInfo?.version, hasReplaceGrants);
const canReplaceRecords = replaceBlockedReason === undefined;
const schemaRelationships = schemaRelationshipMap?.[databaseName]?.[tableName];
const schemaRelationships = declaredRelationships(schemaRelationshipMap, databaseName, tableName);
// Read the attributes from the same map the modal's Random Data option reads (NOT the
// describe-first `instanceTable`): where the two disagree, offering Seed reopens the empty-dropdown
// dead end this gate exists to close.
Expand Down Expand Up @@ -247,8 +253,8 @@ export function DatabaseTableView({
// answers `false` when it simply doesn't know, which would offer an irreversible action on a
// guess): dropping a table needs positive evidence that another one remains.
const canDropTable = useMemo(
() => canManageBrowseInstance && !!databaseTables && Object.keys(databaseTables).length > 1,
[canManageBrowseInstance, databaseTables],
() => canManageBrowseInstance && describedTableNames(instanceDatabaseMap, databaseName).length > 1,
[canManageBrowseInstance, instanceDatabaseMap, databaseName],
);

const { toggled: filtersToggled, toggleOn: showFilters, toggleOff: hideFilters } = useToggler(false);
Expand Down Expand Up @@ -1157,7 +1163,7 @@ export function DatabaseTableView({
&& !!instanceId && (
<DropdownMenuItem
className="focus:bg-yellow/70 focus:text-white"
onClick={onCleanupOrphanBlobs}
onSelect={onCleanupOrphanBlobs}
disabled={isCleanupOrphanBlobsPending}
>
<BrushCleaningIcon className="text-yellow " />
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,12 @@ describe('buildItems', () => {
expect(items['empty'].children).toEqual([]);
});

it('lists only databases the map has a describe for', () => {
const { items } = buildItems({ ...map, hole: null } as unknown as InstanceDatabaseMap, { canManage: true });
expect(items[rootId].children).toEqual([createTableId, 'alpha', 'beta', 'empty']);
expect(items['hole']).toBeUndefined();
});

it('gives tables composite ids so duplicate names across databases do not collide', () => {
const { items } = buildItems(map, { canManage: true });
expect(items['alpha/shared'].data).toEqual({ kind: 'table', databaseName: 'alpha', tableName: 'shared' });
Expand Down
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import { describedDatabaseNames, describedTableNames } from '@/features/instance/databases/functions/describedTable';
import { InstanceDatabaseMap } from '@/integrations/api/api.patch';
import type { TreeItem } from 'react-complex-tree';
import { createTableId, rootId } from './specialItems';
Expand Down Expand Up @@ -28,10 +29,10 @@ export function buildItems(
{ canManage }: { canManage: boolean },
): { items: Record<string, TreeItem<DbTreeData>>; rootId: string } {
const items: Record<string, TreeItem<DbTreeData>> = {};
const databaseNames = Object.keys(instanceDatabaseMap || {}).sort();
const databaseNames = describedDatabaseNames(instanceDatabaseMap);

for (const databaseName of databaseNames) {
const tableNames = Object.keys(instanceDatabaseMap?.[databaseName] || {}).sort();
const tableNames = describedTableNames(instanceDatabaseMap, databaseName);
items[databaseName] = {
index: databaseName,
isFolder: true,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -32,12 +32,12 @@ function renderMenu(rolePermission: Record<string, unknown>, dog: Record<string,
return renderMenuOver(rolePermission, { data: { dog, cat: keyedTable } });
}

function renderMenuOver(rolePermission: Record<string, unknown>, instanceDatabaseMap: unknown) {
function renderMenuOver(rolePermission: Record<string, unknown>, instanceDatabaseMap: unknown, tableName = 'dog') {
permission.current = rolePermission as unknown as LocalRolePermission;
const menu = (map: unknown) => (
<ContextMenu open>
<ContextMenuContent forceMount>
<TableContextMenuItems databaseName="data" tableName="dog" instanceDatabaseMap={map as never} />
<TableContextMenuItems databaseName="data" tableName={tableName} instanceDatabaseMap={map as never} />
</ContextMenuContent>
</ContextMenu>
);
Expand Down Expand Up @@ -109,9 +109,14 @@ describe('TableContextMenuItems on a table with no primary key', () => {
});

describe('TableContextMenuItems on a table the map does not describe', () => {
const dropTable = () => screen.queryByRole('menuitem', { name: 'Drop Table' });
// Two other tables, so the database's last table is not what withholds Drop Table.
const otherTables = { cat: keyedTable, cow: keyedTable };

function expectNothingOffered() {
expect(addRecords()).toBeNull();
expect(importData()).toBeNull();
expect(dropTable()).toBeNull();
expect(exportCsv().hasAttribute('data-disabled')).toBe(true);
fireEvent.click(exportCsv());
expect(exportCsvSpy).not.toHaveBeenCalled();
Expand All @@ -123,17 +128,23 @@ describe('TableContextMenuItems on a table the map does not describe', () => {
});

it('offers nothing that needs its key when the map lacks the table', () => {
renderMenuOver({ super_user: true }, { data: { cat: keyedTable } });
renderMenuOver({ super_user: true }, { data: otherTables });
expectNothingOffered();
});

it('reads only the map’s own tables, whatever the table is called', () => {
renderMenuOver({ super_user: true }, { data: otherTables }, 'constructor');
expectNothingOffered();
});

// Both menus list tables from the map, so an open one meets an undescribed table when a refetch
// lands without it: another tab or user dropped it.
it('withdraws them from an open menu once a refetched map drops the table', () => {
const { rerenderOver } = renderMenuOver({ super_user: true }, { data: { dog: keyedTable, cat: keyedTable } });
const { rerenderOver } = renderMenuOver({ super_user: true }, { data: { dog: keyedTable, ...otherTables } });
expect(exportCsv().hasAttribute('data-disabled')).toBe(false);
expect(dropTable()).not.toBeNull();

rerenderOver({ data: { cat: keyedTable } });
rerenderOver({ data: otherTables });
expectNothingOffered();
});
});
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,9 @@
import { ContextMenuItem, ContextMenuSeparator } from '@/components/ui/contextMenu';
import {
describedDatabase,
describedTable,
describedTableNames,
} from '@/features/instance/databases/functions/describedTable';
import { browsePrimaryKey } from '@/features/instance/databases/functions/formatBrowseDataTableHeader';
import { useExportTableCsv } from '@/features/instance/databases/hooks/useExportTableCsv';
import {
Expand Down Expand Up @@ -28,8 +33,9 @@ export function TableContextMenuItems({ databaseName, tableName, instanceDatabas
const canImport = useInstanceImportDataPermission(instanceId ?? clusterId, databaseName, tableName);
const { exportCsv } = useExportTableCsv();

const isLastTable = Object.keys(instanceDatabaseMap?.[databaseName] || {}).length <= 1;
const instanceTable = instanceDatabaseMap?.[databaseName]?.[tableName];
const databaseTables = describedDatabase(instanceDatabaseMap, databaseName);
const isLastTable = describedTableNames(instanceDatabaseMap, databaseName).length <= 1;
const instanceTable = describedTable(instanceDatabaseMap, databaseName, tableName);
const primaryKey = browsePrimaryKey(instanceTable);
// Same withdrawal as the table view's toolbar: rows added here couldn't be listed there.
const hasKnownPrimaryKey = !!primaryKey;
Expand All @@ -41,7 +47,7 @@ export function TableContextMenuItems({ databaseName, tableName, instanceDatabas
primaryKey,
conditions: null,
instanceTable,
databaseTables: instanceDatabaseMap?.[databaseName],
databaseTables,
});
};

Expand All @@ -63,7 +69,7 @@ export function TableContextMenuItems({ databaseName, tableName, instanceDatabas
<CloudDownloadIcon />
Export CSV
</ContextMenuItem>
{canManage && !isLastTable && (
{canManage && instanceTable && !isLastTable && (
<>
<ContextMenuSeparator />
<ContextMenuItem
Expand Down
50 changes: 50 additions & 0 deletions src/features/instance/databases/functions/describedTable.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,50 @@
import type { SchemaRelationshipMap } from '@/features/instance/databases/functions/schemaRelationships';
import { InstanceDatabaseMap } from '@/integrations/api/api.patch';
import { describe, expect, it } from 'vitest';
import {
declaredRelationships,
describedDatabase,
describedDatabaseNames,
describedTable,
describedTableNames,
ownEntry,
} from './describedTable';

const dog = { attributes: [] };
const map = { data: { dog } } as unknown as InstanceDatabaseMap;
const relationships = [{ attribute: 'owner' }];
const relationshipMap = { data: { dog: relationships } } as unknown as SchemaRelationshipMap;

describe('describedDatabase and describedTable', () => {
it('return the map’s own entries', () => {
expect(describedDatabase(map, 'data')).toEqual({ dog });
expect(describedTable(map, 'data', 'dog')).toBe(dog);
expect(declaredRelationships(relationshipMap, 'data', 'dog')).toBe(relationships);
});

it('return nothing for a name the map lacks, or before the map loads', () => {
expect(describedDatabase(undefined, 'data')).toBeUndefined();
expect(describedTable(undefined, 'data', 'dog')).toBeUndefined();
expect(describedTable(map, 'data', 'cat')).toBeUndefined();
expect(describedTable(map, 'other', 'dog')).toBeUndefined();
expect(declaredRelationships(undefined, 'data', 'dog')).toBeUndefined();
expect(ownEntry(null, 'data')).toBeUndefined();
expect(ownEntry({ data: null }, 'data')).toBeUndefined();
});

it('name the databases and tables that have a describe, sorted', () => {
const withHoles = { beta: { b: dog, a: dog, hole: null }, alpha: {}, hole: null } as unknown as InstanceDatabaseMap;
expect(describedDatabaseNames(withHoles)).toEqual(['alpha', 'beta']);
expect(describedDatabaseNames(undefined)).toEqual([]);
expect(describedTableNames(withHoles, 'beta')).toEqual(['a', 'b']);
expect(describedTableNames(withHoles, 'hole')).toEqual([]);
expect(describedTableNames(undefined, 'beta')).toEqual([]);
});

it('never find an Object.prototype member', () => {
expect(describedDatabase(map, 'constructor')).toBeUndefined();
expect(describedTable(map, 'data', 'toString')).toBeUndefined();
expect(describedTable(map, 'constructor', 'name')).toBeUndefined();
expect(declaredRelationships(relationshipMap, 'data', 'constructor')).toBeUndefined();
});
});
41 changes: 41 additions & 0 deletions src/features/instance/databases/functions/describedTable.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
import { SchemaRelationship, SchemaRelationshipMap } from '@/features/instance/databases/functions/schemaRelationships';
import { InstanceDatabaseMap, InstanceDatabaseTableMap, InstanceTable } from '@/integrations/api/api.patch';

export function ownEntry<T>(record: Readonly<Record<string, T>> | null | undefined, key: string): T | undefined {
return record && Object.hasOwn(record, key) ? (record[key] ?? undefined) : undefined;
}

export function describedDatabase(
instanceDatabaseMap: InstanceDatabaseMap | undefined,
databaseName: string,
): InstanceDatabaseTableMap | undefined {
return ownEntry(instanceDatabaseMap, databaseName);
}

export function describedDatabaseNames(instanceDatabaseMap: InstanceDatabaseMap | undefined): string[] {
return Object.keys(instanceDatabaseMap ?? {}).filter((name) => !!describedDatabase(instanceDatabaseMap, name)).sort();
}

export function describedTableNames(
instanceDatabaseMap: InstanceDatabaseMap | undefined,
databaseName: string,
): string[] {
const databaseTables = describedDatabase(instanceDatabaseMap, databaseName);
return Object.keys(databaseTables ?? {}).filter((name) => !!ownEntry(databaseTables, name)).sort();
}

export function describedTable(
instanceDatabaseMap: InstanceDatabaseMap | undefined,
databaseName: string,
tableName: string,
): InstanceTable | undefined {
return ownEntry(describedDatabase(instanceDatabaseMap, databaseName), tableName);
}

export function declaredRelationships(
schemaRelationshipMap: SchemaRelationshipMap | undefined,
databaseName: string,
tableName: string,
): SchemaRelationship[] | undefined {
return ownEntry(ownEntry(schemaRelationshipMap, databaseName), tableName);
}
Loading
Loading