feat(sidebar): show all databases on the server as a tree (#139)#1473
Conversation
datlechin
force-pushed
the
feat/139-sidebar-database-tree
branch
from
a0c45b7 to
96ff6fa
Compare
datlechin
force-pushed
the
feat/139-sidebar-database-tree
branch
from
96ff6fa to
04b5f3e
Compare
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0c45b737f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if candidates.contains(where: { $0.id == target.id }) { | ||
| return (db.name, schema) |
There was a problem hiding this comment.
When two databases contain the same schema/table/type, TableInfo.id and Hashable do not include the database, so this lookup treats the row selected in the later database as a match for the first visible database with the same table. In the new multi-database tree, double-clicking db2.public.users can therefore switch/open db1.public.users instead, because locateTable returns the first matching id.
Useful? React with 👍 / 👎.
| func routines(connectionId: UUID, database: String, schema: String?) -> [RoutineInfo] { | ||
| guard database == activeDatabase(for: connectionId) else { return [] } | ||
| return SchemaService.shared.routines(for: connectionId) |
There was a problem hiding this comment.
For schema-level databases, this returns every routine from the active database regardless of the schema argument, even though RoutineInfo carries its schema. As a result, when the active PostgreSQL/MSSQL/Oracle database is expanded, the same procedures/functions are rendered under every schema instead of only under their own schema.
Useful? React with 👍 / 👎.
| let idleEntries = mine.filter { $0.value.inFlightCount == 0 } | ||
| let pool = idleEntries.isEmpty ? mine : idleEntries | ||
| guard let oldest = pool.min(by: { $0.value.lastUsed < $1.value.lastUsed }) else { return } |
There was a problem hiding this comment.
If a user expands or searches enough databases to start metadata loads for more than four databases at once, idleEntries can be empty and this falls back to evicting from mine, then disconnects a driver whose withDriver body is still running. That can abort an otherwise valid schema/table fetch and leave the tree in a failed/loading state just because concurrent loads hit the pool cap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7f293cc53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private var visibleDatabases: [DatabaseMetadata] { | ||
| let nonSystem = databases.filter { !$0.isSystemDatabase } | ||
| guard !searchText.isEmpty else { return nonSystem } | ||
| return nonSystem.filter { databaseMatchesSearch($0) } |
There was a problem hiding this comment.
When the search text is a schema/table/routine name in a database that has not already been expanded, this filter drops that database while its schema/table state is still idle, and no task is scheduled to load its contents. For example, searching for users only finds databases whose contents were previously expanded; unexpanded databases whose names do not contain users never render schemasContent/tablesContent, so their .task loaders never run.
Useful? React with 👍 / 👎.
| SidebarContextMenu( | ||
| clickedTable: selection.first, | ||
| selectedTables: selection, | ||
| isReadOnly: coordinator?.safeModeLevel.blocksAllWrites ?? false, | ||
| onBatchToggleTruncate: { viewModel.batchToggleTruncate(tableNames: $0) }, | ||
| onBatchToggleDelete: { viewModel.batchToggleDelete(tableNames: $0) }, | ||
| coordinator: coordinator |
There was a problem hiding this comment.
In the database tree, the context menu receives only the selected TableInfo, while the menu actions call coordinator APIs that operate in the current active database (or pass only table names for export/delete/truncate). Unlike the primary action below, this path never calls locateTable or switches to the row's database, so right-clicking a table under a non-active database can show structure or queue destructive/export operations against the active database's table with the same name instead.
Useful? React with 👍 / 👎.
… clears canceled tasks (#139)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c88cff546a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let schema, | ||
| schema != coordinator?.toolbarState.currentSchema, | ||
| PluginManager.shared.supportsSchemaSwitching(for: databaseType) { | ||
| await coordinator?.switchSchema(to: schema) |
There was a problem hiding this comment.
When opening a table in another database, this comparison still uses the toolbar schema value from the previous database; switchDatabase(to:) updates the database first but the session is reset to the default schema, so if the target schema has the same name as the old toolbar schema (for example moving from db1.sales to db2.sales), this branch skips switchSchema and opens the table against the wrong schema in the new database.
Useful? React with 👍 / 👎.
Signed-off-by: Ngô Quốc Đạt <datlechin@gmail.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 50e90a834c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| try await group.next() | ||
| group.cancelAll() |
There was a problem hiding this comment.
When the timeout child wins, this withThrowingTaskGroup still waits for the driver.connect() child to finish as the group scope exits; cancelAll() only requests cancellation. For drivers whose connect call is blocked in client/network code and does not promptly observe Swift task cancellation, expanding an unreachable database can hang well past the intended 15-second metadata timeout instead of surfacing the timeout error.
Useful? React with 👍 / 👎.
…d metadata pool (#139)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25cee6a451
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .contextMenu(forSelectionType: DatabaseTreeTableRef.self) { selection in | ||
| SidebarContextMenu( | ||
| clickedTable: selection.first?.table, | ||
| selectedTables: Set(selection.map(\.table)), |
There was a problem hiding this comment.
When a tree selection spans more than one database, this conversion drops the database/schema from each DatabaseTreeTableRef while activateBeforeAction switches only to selection.first. A Command-click selection like db1.users plus db2.orders followed by Truncate/Delete therefore queues both table names against whichever database happens to be first, so one selected table can be operated on in the wrong database.
Useful? React with 👍 / 👎.
| get: { localSelection }, | ||
| set: { localSelection = $0 } |
There was a problem hiding this comment.
In tree layout this binding keeps selections only in localSelection, while app/menu commands such as Copy and Truncate Table check MainContentCommandActions.hasTableSelection, which reads windowState.selectedTables. Selecting a table in the new database tree therefore leaves those commands disabled (or no-op) even though a sidebar row is selected.
Useful? React with 👍 / 👎.
| PluginManager.shared.supportsSchemaSwitching(for: databaseType) { | ||
| await coordinator?.switchSchema(to: schema) | ||
| } | ||
| coordinator?.openTableTab(table) |
There was a problem hiding this comment.
If switching to the target database or schema fails, switchDatabase/switchSchema handle the error internally and return, so this still opens the selected TableInfo in the previous active context. For a database that is listed but later fails to switch (for example reconnect/permission failure), double-clicking its table can open or query the same-named table in the old database after the error alert.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82323b797e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if let existing = registry[connectionId] { | ||
| existing.updateBindings( | ||
| selectedTables: selectedTables, | ||
| pendingTruncates: pendingTruncates, | ||
| pendingDeletes: pendingDeletes, | ||
| tableOperationOptions: tableOperationOptions | ||
| ) | ||
| return existing |
There was a problem hiding this comment.
When a second window for the same connection constructs a sidebar, this branch returns the existing SidebarViewModel after replacing its selectedTables/pending-operation bindings with the new window's WindowSidebarState bindings. The first window still holds the same shared view model, so table selections, copy/delete/truncate enablement, and pending operation state in that window now read and write the second window's sidebar state instead of its own. This only shows up with multiple windows on one connection, but it breaks the window-scoped state separation introduced by WindowSidebarState.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 635b620f79
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| private var visibleDatabases: [DatabaseMetadata] { | ||
| let nonSystem = databases.filter { !$0.isSystemDatabase } |
There was a problem hiding this comment.
When the current database is one of the configured system databases (for example PostgreSQL's default postgres or SQL Server's master), this filter removes it before rendering, so the active database and its tables disappear from the tree; if it is the only database returned, the sidebar becomes a blank list even though the connection is usable. The tree should still include the active database (or otherwise provide a way to show it) while hiding inactive system databases by default.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 397f380a88
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if schema != coordinator?.toolbarState.currentSchema { | ||
| await coordinator?.switchSchema(to: schema) |
There was a problem hiding this comment.
When “Use as Active Schema” is invoked for a schema in a different database, this check still compares against the toolbar schema from the previous database after switchDatabase returns. On reconnecting engines such as PostgreSQL, switching databases clears/resets the session schema, so choosing db2.sales while currently on db1.sales skips switchSchema(to: "sales") and leaves the new database on its default/no schema instead of the requested schema.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 186f2cc51f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| RoutineContextMenu(routine: routine) { selected in | ||
| coordinator?.showRoutineDDL(selected) |
There was a problem hiding this comment.
When a procedure/function row belongs to a non-active database, this context menu still calls showRoutineDDL on the current active driver without switching to the row's database or passing any database identity. Since RoutineInfo only carries the schema/name, asking for DDL on db2.public.fn while db1 is active will fetch the same-named routine from db1 (or fail) instead of the routine shown in the tree.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c951b76436
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| func routines(connectionId: UUID, database: String, schema: String?) -> [RoutineInfo] { | ||
| routineLists[Self.tableKey(connectionId: connectionId, database: database, schema: schema)] ?? [] | ||
| } |
There was a problem hiding this comment.
When the active database's tables are already loaded by SchemaService before this tree renders, reconcileLoads sees the active table state as .loaded and never calls loadTables, so routineLists remains empty even though SchemaService may already have procedures/functions. In that common path (or after refreshDatabase on the active DB, which only refreshes SchemaService), the tree shows active tables but drops the active database's routines until some later tree-specific load happens; delegate active-database routine reads to SchemaService or keep this cache synchronized.
Useful? React with 👍 / 👎.
…o avoid Not Connected flash (#139)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fba22726d0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| private func expansionBinding(for schema: String) -> Binding<Bool> { | ||
| Binding( | ||
| get: { !searchText.isEmpty || expandedSchemas.contains(schema) }, | ||
| get: { !searchText.isEmpty || windowState.expandedTreeSchemas.contains(schema) }, |
There was a problem hiding this comment.
When a BigQuery-style schema tree keeps a dataset expanded in windowState.expandedTreeSchemas, this getter reopens the section without calling loadTables(for:); the only load trigger remains the binding setter. After a reconnect, schema refresh, or reopening this view with an already-expanded dataset whose per-schema state is idle, the expanded section renders the “Loading tables…” row indefinitely until the user collapses and expands it again.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 740f455693
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| isPendingTruncate: pendingTruncates.contains(ref.table.name), | ||
| isPendingDelete: pendingDeletes.contains(ref.table.name) |
There was a problem hiding this comment.
When two visible databases or schemas contain the same table name, these checks mark every matching row as pending because the pending sets are keyed only by table.name. For example, scheduling truncate for db1.public.users will also show the truncate badge on db2.public.users, which makes the tree indicate that the wrong table is queued even before any operation is run.
Useful? React with 👍 / 👎.
…ait for the new connection (#139)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4071b5f06d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| await DatabaseTreeMetadataService.shared.loadDatabases( | ||
| connectionId: connectionId, | ||
| databaseType: connection.type | ||
| ) |
There was a problem hiding this comment.
For connections where supportsDatabaseTree is false, this still awaits fetchDatabases() during every schema load even though the database tree cannot be shown. That adds an unnecessary startup/reload API call for flat-only or unsupported drivers (for example API/file-based connections) and can block initializeView() on listing all databases before post-schema reconciliation completes; guard this call with the same tree-support/layout conditions used by the sidebar.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78c33bc145
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| async let procedures = driver.fetchProcedures(schema: normalizedSchema) | ||
| async let functions = driver.fetchFunctions(schema: normalizedSchema) | ||
| return SchemaObjects( | ||
| tables: try await tables, | ||
| routines: try await procedures + functions |
There was a problem hiding this comment.
When a database allows fetchTables but denies or errors on procedure/function metadata, these async let calls make the whole object load throw and render the schema/database as failed, hiding tables that could otherwise be shown. The flat SchemaService path already treats routine fetches as best-effort via fetchRoutinesSafely, so the new tree path should similarly load tables independently and only omit routines when those calls fail.
Useful? React with 👍 / 👎.
…harden reconnect reset (#139)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 569aacfdc7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| EmptyView() | ||
| .contextMenu(forSelectionType: TableInfo.self) { selection in | ||
| SidebarContextMenu( | ||
| clickedTable: selection.first, |
There was a problem hiding this comment.
When more than one table is selected in the flat sidebar, this passes selection.first from an unordered Set as the clicked table, so row-specific actions like Show Structure, Edit View Definition, and Maintenance can act on an arbitrary selected table rather than the row whose context menu the user opened. The previous per-row menu supplied the actual row as clickedTable, so multi-selecting tables and right-clicking one of them can now open or maintain the wrong object while batch actions still use the full selection.
Useful? React with 👍 / 👎.
1 participant
Summary
DatabaseTreeMetadataServicefacade which delegates toSchemaServiceinternally for the active database, eliminating two-sources-of-truth drift.mysql,information_schema, etc.) hidden by default; active database/schema shown via bold + tint (no iOS checkmark icon); set-active via right-click context menu; opening a non-active database's table auto-switches first.Architecture changes
DatabaseTreeView,DatabaseTreeMetadataService,MetadataConnectionPool.SchemaPickerFooterhidden when the tree is in use to avoid dual schema-switch controls.SchemaServicegains a per-connection generation counter soSidebarViewModelfilter cache fingerprints are O(1) instead of[TableInfo].hashValueO(n).SidebarViewModel.shared(connectionId:databaseType:bindings:)registry: VM survivesSidebarViewrebuilds acrossrebuildPanes/AnyViewwrapping; cleanup hook indisconnectSession.SidebarContainerViewControllerreplaces recursivewithObservationTrackingre-registration with aTask-based await loop.Sidebar polish + bug fixes
FavoritesTabViewrecursiveAnyView(ForEach)replaced with named recursiveFavoriteNodeRowsViewstruct.expandedSchemas/expandedDatabases/expandedTreeDatabaseSchemasmoved from@StatetoWindowSidebarStateso expansion survives view recreation.SidebarTreeViewsearch load throttled (300ms debounce, skips already-loaded schemas).TableRowViewZStack badge →.overlay(alignment: .bottomTrailing); monospaced font replaced with system sans-serif (matches Xcode/Finder).SchemaPickerFooterschemaVersionnotification hack removed (uses@Bindable DatabaseManager.sharednative observation).SidebarContextMenubutton literals +SidebarTreeView/RedisKeyTreeView/Keysheader / FavoritesTabView empty-state strings localized.List.contextMenu(forSelectionType:) primaryAction:(Mail/Finder/Xcode pattern). DatabaseTreeView holds its own selection state to avoid the legacyMainContentView.onChange(selectedTables)auto-open bypass that skippedswitchDatabase..task(id:)on.idle/.loadingrows auto-triggers load so ex-active databases don't get stuck on "Loading…" after a switch.Test plan
Table 'old_db.x' doesn't existerrors).