feat(databases): tree nav, database overview, and shared table actions#1471
Conversation
There was a problem hiding this comment.
Code Review
This pull request refactors the database management UI by replacing the select-based sidebar with a tree-based navigation sidebar, introducing a database overview component, and consolidating action modals into a single, always-mounted component. It also extracts CSV export logic into a reusable hook and adds context menus for right-click actions. The review feedback suggests adding unique key props to the consolidated modals to ensure proper state resets when switching targets, and wrapping the CSV blob URL revocation in a setTimeout to prevent download failures in certain browsers.
DavidCockerill
left a comment
There was a problem hiding this comment.
Really like this — the tree rebuild matches the Applications model, and showing sizes/schema instantly from the fast describe_all map while backfilling row counts lazily is exactly the right call for the slow part. Overview numbers all check out on read (DB/audit size off any table's db_size/db_audit_size with ?? 0 for empty DBs, table count from Object.keys, PK falls back primary_key ?? hash_attribute ?? '—'), and the empty-instance / stale-link <Navigate replace> fallbacks each terminate cleanly with no redirect loop.
One thing worth confirming before merge — a trade-off, not a bug:
In plain terms: to fill in the row counts, the overview page asks the server to count every table in every database on the whole instance, even though it's only displaying one database — and it fires the moment the Databases tab opens, since that now lands on this overview by default. The server caps the work (it estimates rather than exhaustively scanning once a table gets large, and caches the result ~1 min), so it won't hang the page. But on an instance with many databases/tables it's more server work on tab-open than the old screen, which only counted the one table you were viewing. If that eagerness is intentional (warming the cache for when you expand other DBs), all good — worth a comment saying so. If not, the fix is small: scope the counted describe to the database being shown, or request estimates explicitly. (DatabaseOverview.tsx:43.)
A few smaller, non-blocking notes:
usePermissions.ts:214-216— the attribute-permission function still has the same unguardedspecificPermission?.tables?.[tableName]deref you just fixed two lines up in the table-level hook; not reachable through this PR's menus (they use the fixed table-level hook), but cheap to guard while you're here.DatabaseActionModals.tsx:107—AddTableRowModalsilently renders nothing if its target isn't in the fast map (e.g. a stale map after an external drop); a toast on the miss would be friendlier. Low likelihood.- Double-click on a database navigates (first click) then expands (second) — consistent with single-click-opens-overview and the differ-guard makes it idempotent, so it's a non-issue functionally; noting since single/double-click races are worth being explicit about.
- No tests lock in the lazy count-backfill, the routing fallbacks, or the tree interactions — worth a couple given how much interaction logic moved here.
— DAIvid (Claude Opus 4.8)
|
Thanks for the thorough read! Addressed in
🤖 Addressed by Claude Code |
kriszyp
left a comment
There was a problem hiding this comment.
Reviewed — this is a clean refactor. Rebuilding the Databases nav on react-complex-tree to match Applications, the single always-mounted action-modal hub, and the composite db/table ids all read well, and it's nicely commented and browser-verified end-to-end. No blocking issues, and both Gemini bot findings (per-target key to reset modal state on target switch; deferring URL.revokeObjectURL so the download isn't cancelled on Firefox/iOS Safari) are already applied at head. Two genuine bug fixes folded in are a bonus:
- the permission crash fix (
usePermissions.ts:189) — the missing optional chain that any restricted (non-structure_user) user would trip opening a table context menu; and - the CSV export hook now has real error handling (
useExportTableCsv.ts:46) where before a failed export left the toast spinning andisExportingCSVstuck true.
The tree builder is the right unit to test and it's tested honestly — buildItems.test.ts covers sort order, the synthetic root, duplicate table names across dbs, empty databases, the canManage gate, and an undefined map. Builder is memoized on [map, canManage], so navigation doesn't rebuild the tree. Good.
Non-blocking items:
Instance-wide counted describe_all on overview open (DatabaseOverview.tsx:33). getDescribeAllQueryOptions without skipRecordCount counts every table in every database, but the overview only shows one database's counts. It's cold-path, cached, and non-blocking (counts backfill with a …), so low severity — but on a large multi-database instance it's a heavier scan than needed. If describe_all supports a per-database count scope, scoping it would avoid touching unrelated databases.
Suggestions: a tiny unit test for the permission crash fix (a permission object where tables[tableName] is undefined) would pin that restricted-user regression; and AddTableRowModal silently no-ops when the target table isn't in instanceDatabaseMap (DatabaseActionModals.tsx:86) — reachable only via a stale tree between invalidation and refetch, so low-risk, but a right-click "Add Record(s)" that does nothing is a confusing dead-end (a toast or refetch-then-open would be friendlier).
One question: does the root "Create a Table" row intentionally seed the modal with the currently-open database rather than no database? Reads deliberate, just confirming.
Perf-wise there's no Harper read/replication hot-path impact here — tree + overview are driven off the single already-loaded describe_all map, no N+1, no re-render storm on nav. Looks good to land.
—
🤖 Reviewed by KrAIs (Claude Opus 4.8) on Kris's behalf. (Gemini/agy local pass unavailable — synthesis is from the Claude pass + the Gemini bot review already on the PR.)
|
Thanks @kriszyp! Follow-ups on the non-blocking items:
All green (tsc / oxlint / dprint / 78 tests). Thanks for the thorough pass! 🤖 Addressed by Claude Code |
kriszyp
left a comment
There was a problem hiding this comment.
Re-reviewed at 283ba95 — thanks for the quick turnaround. All three items from my last pass are correctly resolved: the overview record counts are now scoped to the shown database (and you extracted the redirect logic into a pure, tested resolveDatabasesRedirect along the way — nice), the restricted-user permission crash now has a real regression test (checkSchemaTablePermission.test.ts), and the stale Add-Records target now toasts instead of silently no-op'ing. Approving.
Two small, non-blocking suggestions on the fixes themselves (both low-severity, no rush):
-
Total Records can silently undercount on a settled-but-failed count (
DatabaseOverview.tsx:60).allCountsSettledtreats "settled" as ready, butcountByTable[tableName]is only populated on success, so a failed table contributes0via?? 0— the total renders as a definite-looking number that's short by that table's count, with no~/error indicator (anyEstimatedonly reflectsestimated_record_range, not query failure). It's strictly better than the old permanently-stuck…, and the trigger is rare (adescribe_tablefailing for a table present in the fast map — a race with a concurrent drop, or a transient 5xx withretry: false), but it trades "obviously stuck" for "quietly wrong," which is the less safe direction for a count. Consider flagging failed-but-settled tables the way estimated ones are flagged (or omitting them from "Tables" for consistency). -
The attribute-permission crash guard has no unit test (
usePermissions.ts:206), unlike its siblingcheckSchemaTablePermission. Same one-line-guard pattern, same crash class, and it was the one still-open crash vector from the last pass — worth a parity test, ideally by extracting it the same way you did the sibling.
Neither blocks — approving as-is.
—
🤖 Reviewed by KrAIs (Claude Opus 4.8) on Kris's behalf. (Gemini/agy unavailable this pass — Claude-only.)
|
Spun the deferred review follow-ups out into tracked issues so this PR can land:
The items reviewers flagged that were fixed in-PR (per-database count scoping, permission-crash regression test, stale-map toast, settled-based overview total) remain in this branch. 🤖 via Claude Code |
Rebuild the Databases left nav on the applications tab's react-complex-tree: databases as top-level items with their tables nested, a "+ Create a Table" row, and built-in type-to-filter. Single-click selects (database -> new overview page, table -> data grid); double-click / chevron expands. - Add a database overview page: DB size, audit size, table count, and a per-table breakdown (size, columns, primary key, last updated) with record counts backfilled lazily. Landing on the tab now shows the first DB's overview. - Right-click menus for databases (Create a Table, Drop Database) and tables (Add Record(s), Import Data, Export CSV, Drop Table). The table actions live in one TableContextMenuItems component shared by the tree and the overview's table list, so the two menus can't drift. - Lift the action modals into a single always-mounted DatabaseActionModals hub driven by target-carrying watched values, so an action can target any db/table -- not just the open one. Extract the CSV download into useExportTableCsv. - Fix a Radix quirk where an already-open context menu didn't re-anchor on a second right-click: remount the content keyed by cursor position. Also applied to the applications file tree, which had the same bug. - Fix a latent crash in useInstanceSchemaTablePermission (missing optional chain) that the new right-click menus would trigger for restricted users. - Remove the redundant sidebar "Import Data" button (already in the header). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…evoke Address PR review: - Give the consolidated action modals a per-target `key` so their internal state resets on switch, and drop CreateNewTableModal's manual reset effect (the remount now seeds the prefilled database). - Defer URL.revokeObjectURL in useExportTableCsv so it can't cancel the download before Firefox / iOS Safari start fetching the blob. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…nits Address PR review: - Overview row counts now fetch per-table describe_table for ONLY the shown database (via useQueries) instead of a whole-instance counted describe_all on tab-open; shares cache with the table view. (DatabaseOverview.tsx) - Guard the attribute-permission table deref in useInstanceSchemaTableAttribute- Permission, matching the table-level fix. (usePermissions.ts) - Toast + clear the trigger when Add Records targets a table missing from the fast map (stale map after an external drop). (DatabaseActionModals.tsx) - Extract the tab's redirect rules into resolveDatabasesRedirect (pure, loop-free) and cover them with unit tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Extract the pure table-permission check into checkSchemaTablePermission (no auth-store imports, so it's unit-testable in a node env) and cover the case that previously crashed: a restricted (non-structure_user) user whose database grant doesn't list the table. useInstanceSchemaTablePermission now delegates to it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Address review: - DatabaseOverview: base the aggregate Records total on settled count queries (not data presence) so a single failed/never-resolving describe_table (retry: false) no longer hangs it on "…"; prefix the total with "~" when any contributing count is estimated, matching the per-row labels. - DeleteTableModal / DeleteDatabaseModal: drop the now-always-undefined `trigger` + attemptToRestoreFocus plumbing (callers fire via a target payload with no trigger). Radix's confirmation dialog restores focus on close on its own. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The lifted AddTableRowModal takes stage's optional databaseTables prop so record add is relationship-aware, matching what the right-pane toolbar passes. (This integration lived in the dropped merge commit; restoring it after the rebase.) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
283ba95 to
5796f34
Compare
What & why
Rebuilds the Databases left nav on the same
react-complex-treetech the Applications tab uses, and adds a database overview page. The old dropdown + flat table list only showed one database at a time, had no filter, and kept per-item actions in the right-pane toolbar only.Now:
Highlights
describe_allmap already loaded; counts come from a separate counteddescribe_all. Opening the tab now lands on the first database's overview.TableContextMenuItemscomponent shared by the tree and the overview's table list, so right-clicking a row in either place opens the same menu and they can't drift.DatabaseActionModalsdriven by target-carrying watched values, so an action can target any db/table (even one that isn't open, or an empty database), not just the currently-open one. CSV download extracted intouseExportTableCsv(toolbar keeps its filter-aware export; the tree/overview export the whole table).Fixes folded in
useInstanceSchemaTablePermission(a missing optional chain) that the new right-click menus would trigger for restricted (non-structure_user) users.No new backend operations
Everything reuses existing operations/modals (
create_table,drop_table,drop_database,insert, imports, CSV export). Rename was explicitly out of scope.Verification
+, orange containers, neutral high-contrast leaves).tsc -b, oxlint, and dprint pass.vitestgreen (28 tests, incl. new coverage for the tree-item builder).🤖 Generated with Claude Code