feat(database): add project-scoped PostgreSQL query tabs#8685
Conversation
…-tabs # Conflicts: # src/renderer/src/components/tab-bar/TerminalTabLeadingIcon.tsx # src/renderer/src/i18n/locales/en.json # src/renderer/src/i18n/locales/es.json # src/renderer/src/i18n/locales/ja.json # src/renderer/src/i18n/locales/ko.json # src/renderer/src/i18n/locales/zh.json
📝 WalkthroughWalkthroughAdds PostgreSQL database connectivity with encrypted profile storage, SSH routing, runtime RPCs, query execution, catalog and schema inspection, and cancellation. Introduces shared database contracts and runtime capabilities. Adds database tabs, connection/profile controls, schema and result panes, tab ordering and lifecycle handling, palette and focus integration, localization, and packaged PostgreSQL dependencies. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Nitpick comments (10)
src/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsx (1)
709-721: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared
openDatabaseTabhelper instead of duplicating tab construction.Terminal.tsx's
handleNewDatabaseTabalready delegates toopenDatabaseTab(worktreeId, targetGroupId)fromdatabase-tab-actions.ts. This callback instead re-implements the same initial-state construction by hand (connection/queryDraft/readOnlylisted individually), which can silently drop futureDatabaseTabStatefields and diverges from the single source of truth.♻️ Proposed refactor
- const createFloatingDatabaseTab = useCallback(() => { - const tab = createUnifiedTab(FLOATING_TERMINAL_WORKTREE_ID, 'database', { - label: translate('auto.components.database.tab.title', 'Database Query'), - database: { - connection: { ...DEFAULT_DATABASE_TAB_STATE.connection }, - queryDraft: DEFAULT_DATABASE_TAB_STATE.queryDraft, - readOnly: DEFAULT_DATABASE_TAB_STATE.readOnly - }, - targetGroupId: activeGroup?.id, - activate: true - }) - activateTab(tab.id) - }, [activateTab, activeGroup, createUnifiedTab]) + const createFloatingDatabaseTab = useCallback(() => { + const tabId = openDatabaseTab(FLOATING_TERMINAL_WORKTREE_ID, activeGroup?.id) + activateTab(tabId) + }, [activateTab, activeGroup])src/shared/workspace-session-schema.ts (1)
128-143: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate connection-shape schema (see consolidated comment).
This
connectionsub-schema (host/port/database/schema/user/sslMode) re-declares the same shape already validated indatabase-profile-store.ts'sStoredConnectionSchema. See the consolidated comment for the cross-file DRY recommendation.src/main/database/database-profile-store.ts (2)
127-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
persistedConnectionhelper (see consolidated comment).This function duplicates
database-profile-service.ts's same-named helper with slightly different behavior (no trimming here). See the consolidated comment for the cross-file DRY recommendation.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPostgres connection shape has no single source of truth; extract a shared schema + normalizer.
The same 7-field Postgres connection shape (
providerId/host/port/database/schema/user/sslMode) is independently hand-declared as a Zod schema indatabase-profile-store.tsandworkspace-session-schema.ts(and again in the RPC layer'sDatabaseConnectionschema), and the "normalize connection for persistence" logic is duplicated as two same-namedpersistedConnectionfunctions with diverging behavior (one trims, one doesn't). This increases the risk that a future change to a constraint (e.g.,sslModevalues, field length limits) is applied inconsistently across boundaries.
src/main/database/database-profile-store.ts#L14-22: treat thisStoredConnectionSchemaas the canonical shape (or extract it to a shared module) and have other layers.extend()/reuse it instead of re-declaring the same fields.src/main/database/database-profile-store.ts#L127-137: replace thispersistedConnectionwith a single shared normalizer (shared with the service-layer version below) so trimming behavior is consistent regardless of call path.src/main/database/database-profile-service.ts#L116-126: same as above — consolidate with the store'spersistedConnectioninto one shared helper.src/shared/workspace-session-schema.ts#L131-139: derive this nestedconnectionschema from the same shared Postgres-connection schema instead of re-declaring the field list.src/main/database/database-profile-service.ts (1)
116-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate
persistedConnectionhelper (see consolidated comment).This function duplicates
database-profile-store.ts's same-named helper (that one doesn't trim). See the consolidated comment for the cross-file DRY recommendation.src/main/database/database-service.ts (1)
41-72: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the repeated resolve→route→dispatch pattern.
testConnection,introspect,catalog, andexecuteare identical except for the provider method invoked. Extracting a shared helper prevents future new operations from accidentally skipping profile resolution or SSH routing.♻️ Proposed consolidation
+ private async run<TRequest extends DatabaseConnectionRequest, TResult>( + request: TRequest, + operation: ( + provider: DatabaseProvider, + routedRequest: ResolvedDatabaseRequest<TRequest> + ) => Promise<TResult> + ): Promise<TResult> { + return this.withProjectConnection(this.resolveRequest(request), (routedRequest) => + operation(this.getProvider(routedRequest.connection.providerId), routedRequest) + ) + } + async testConnection( request: DatabaseConnectionRequest, signal?: AbortSignal ): Promise<DatabaseConnectionTestResult> { - return this.withProjectConnection(this.resolveRequest(request), (routedRequest) => - this.getProvider(routedRequest.connection.providerId).testConnection(routedRequest, signal) - ) + return this.run(request, (provider, routedRequest) => provider.testConnection(routedRequest, signal)) } // ...same for introspect, catalog, executesrc/main/database/database-ssh-connection-route.test.ts (1)
5-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture host coincides with the loopback address, weakening the
tlsServerNameassertion.The fixture uses
host: '127.0.0.1', the same value the tunnel rewritesconnection.hostto. The assertion at line 46-51 checkstlsServerName: '127.0.0.1', which would pass even if the implementation mistakenly hardcoded the loopback address instead of preserving the original remote host. Use a distinct remote-looking host (e.g.'db.internal.example.com') to actually exercise that TLS verification still targets the real server name.Also applies to: 46-51
src/renderer/src/components/database/useDatabaseProfiles.ts (1)
43-66: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winSelecting a profile re-triggers a full profile-list reload.
selectProfile(lines 73-88) already syncsprofileName/rememberPasswordfrom the in-memoryprofilesarray and then updatesdatabase.profileId. Because this effect depends ondatabase.profileId, that update re-runs the wholelistDatabaseProfilesIPC call (with vault decryption) again — redundant work on every profile selection. Consider keying this effect only on[worktreeId, nodeIdentity]and deriving the "selected profile" sync via a separateuseEffect/useMemoover the already-loadedprofilesstate.As per coding guidelines,
**/*.{js,jsx,ts,tsx,sh,ps1}should "avoid unnecessary API calls".Source: Coding guidelines
src/main/database/postgres-catalog.ts (1)
8-28: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider bounding the catalog queries like the rest of the provider.
postgres-provider.tsbounds query rows and schema columns (MAX_QUERY_ROWS,MAX_SCHEMA_COLUMNS), but thepg_database/information_schema.schematalistings here have noLIMIT. On a multi-tenant instance with many databases/schemas this could return an unexpectedly large payload back to the renderer.src/main/database/postgres-provider.test.ts (1)
211-230: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the cancel client is closed
Add an expectation that the short-lived cancel connection calls
end(); that keeps this test from missing a cleanup regression.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 4e7078d9-a83a-42ca-b684-41ffa07942a2
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (66)
config/packaged-runtime-node-modules.cjsconfig/scripts/electron-builder-config.test.mjspackage.jsonsrc/main/database/database-credential-vault.tssrc/main/database/database-profile-service.test.tssrc/main/database/database-profile-service.tssrc/main/database/database-profile-store.tssrc/main/database/database-provider.tssrc/main/database/database-service.test.tssrc/main/database/database-service.tssrc/main/database/database-ssh-connection-route.test.tssrc/main/database/database-ssh-connection-route.tssrc/main/database/database-vault-key-protection.tssrc/main/database/postgres-catalog.tssrc/main/database/postgres-provider.test.tssrc/main/database/postgres-provider.tssrc/main/ipc/ssh.tssrc/main/runtime/rpc/methods/database.tssrc/main/runtime/rpc/methods/index.tssrc/renderer/src/components/Terminal.tsxsrc/renderer/src/components/WorktreeJumpPalette.tsxsrc/renderer/src/components/database/DatabaseConnectionForm.tsxsrc/renderer/src/components/database/DatabaseContextToolbar.tsxsrc/renderer/src/components/database/DatabasePane.tsxsrc/renderer/src/components/database/DatabaseResults.tsxsrc/renderer/src/components/database/DatabaseSchemaTree.tsxsrc/renderer/src/components/database/database-tab-actions.tssrc/renderer/src/components/database/database-tab-credentials.test.tssrc/renderer/src/components/database/database-tab-credentials.tssrc/renderer/src/components/database/useDatabaseProfiles.tssrc/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsxsrc/renderer/src/components/tab-bar/DatabaseTab.tsxsrc/renderer/src/components/tab-bar/TabBar.tsxsrc/renderer/src/components/tab-bar/TerminalTabLeadingIcon.tsxsrc/renderer/src/components/tab-bar/group-tab-order.tssrc/renderer/src/components/tab-bar/reconcile-order.test.tssrc/renderer/src/components/tab-bar/reconcile-order.tssrc/renderer/src/components/tab-bar/tab-create-menu-options.test.tssrc/renderer/src/components/tab-bar/tab-create-menu-options.tssrc/renderer/src/components/tab-group/TabGroupPanel.tsxsrc/renderer/src/components/tab-group/tab-drag-preview-activation.tssrc/renderer/src/components/tab-group/useTabDragSplit.tssrc/renderer/src/components/tab-group/useTabGroupWorkspaceModel.tssrc/renderer/src/components/terminal/tab-type-cycle.tssrc/renderer/src/hooks/modal-return-focus-action.tssrc/renderer/src/hooks/resolve-zoom-target.tssrc/renderer/src/hooks/useModalReturnFocus.tssrc/renderer/src/i18n/locales/en.jsonsrc/renderer/src/i18n/locales/es.jsonsrc/renderer/src/i18n/locales/ja.jsonsrc/renderer/src/i18n/locales/ko.jsonsrc/renderer/src/i18n/locales/zh.jsonsrc/renderer/src/lib/simulator-palette-search.tssrc/renderer/src/lib/workspace-tab-palette-search.tssrc/renderer/src/lib/worktree-runtime-owner.test.tssrc/renderer/src/lib/worktree-runtime-owner.tssrc/renderer/src/runtime/runtime-database-client.test.tssrc/renderer/src/runtime/runtime-database-client.tssrc/renderer/src/store/selectors.tssrc/renderer/src/store/slices/tabs.tssrc/renderer/src/store/slices/worktrees.tssrc/shared/database-types.tssrc/shared/protocol-version.tssrc/shared/types.tssrc/shared/workspace-session-schema.test.tssrc/shared/workspace-session-schema.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/renderer/src/components/terminal/tab-type-cycle.ts (1)
34-36: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider removing redundant branches.
Since the fallback return at the end of this function is
return activeTabId, the explicit branch fordatabase(along with the one forsimulator) is redundant. You can optionally remove them for a more concise implementation that falls through to the default behavior.♻️ Proposed refactor
- if (activeTabType === 'simulator') { - return activeTabId - } - if (activeTabType === 'database') { - return activeTabId - } return activeTabId
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f35b4ce3-5ae9-4e37-b0ef-55d6973e84b4
📒 Files selected for processing (23)
src/main/database/database-profile-service.test.tssrc/main/database/database-profile-service.tssrc/main/database/database-profile-store.tssrc/main/database/database-service.test.tssrc/main/database/database-service.tssrc/main/database/database-ssh-connection-route.test.tssrc/main/database/postgres-catalog.tssrc/main/database/postgres-provider.test.tssrc/main/database/postgres-provider.tssrc/renderer/src/components/database/DatabaseConnectionForm.tsxsrc/renderer/src/components/database/DatabaseContextToolbar.tsxsrc/renderer/src/components/database/DatabasePane.tsxsrc/renderer/src/components/database/useDatabaseProfiles.tssrc/renderer/src/components/floating-terminal/FloatingTerminalPanel.tsxsrc/renderer/src/components/tab-bar/DatabaseTab.tsxsrc/renderer/src/components/tab-bar/TabBar.tsxsrc/renderer/src/components/terminal/tab-type-cycle.test.tssrc/renderer/src/components/terminal/tab-type-cycle.tssrc/renderer/src/hooks/ipc-tab-switch.test.tssrc/renderer/src/hooks/ipc-tab-switch.tssrc/renderer/src/runtime/runtime-database-client.test.tssrc/renderer/src/runtime/runtime-database-client.tssrc/shared/database-types.ts
🚧 Files skipped from review as they are similar to previous changes (14)
- src/main/database/postgres-catalog.ts
- src/shared/database-types.ts
- src/renderer/src/runtime/runtime-database-client.test.ts
- src/renderer/src/components/database/DatabaseConnectionForm.tsx
- src/renderer/src/components/database/DatabaseContextToolbar.tsx
- src/renderer/src/components/database/DatabasePane.tsx
- src/renderer/src/runtime/runtime-database-client.ts
- src/main/database/database-profile-service.test.ts
- src/main/database/database-ssh-connection-route.test.ts
- src/renderer/src/components/tab-bar/DatabaseTab.tsx
- src/main/database/postgres-provider.ts
- src/renderer/src/components/database/useDatabaseProfiles.ts
- src/renderer/src/components/tab-bar/TabBar.tsx
- src/main/database/postgres-provider.test.ts
2 participants
Summary
Closes #8684.
Adds PostgreSQL as a first-class project workspace tab next to Terminal, Browser, and Markdown. Database queries execute on the runtime that owns the project and follow the project's configured SSH connection when the worktree is SSH-backed.
This is intentionally different from the global database page in #6983: the tab, connection ownership, and query route remain attached to the project so local, paired-remote, and SSH-worktree environments use the same execution context.
User-visible behavior
New Database Queryin the existing new-tab menuImplementation
pgruntime dependencies used by desktop and headless server buildsScreenshots
New-tab menu entry
Insert
new-tab-menu-review.pnghere (showsNew Database Queryalongside the existing workspace tab types).Database Query tab running in the project environment
Insert
project-database-tab-review.pnghere (shows the runtime/SSH route, database and schema selectors, editor, and bounded result grid).Testing
pnpm lintpnpm typecheckpnpm testpnpm buildFull suite result:
Manual/package validation:
ssh-p8AI Review Report
The implementation was reviewed with an AI coding agent for:
The review flagged and the branch addressed packaged dependency omission, missing server-side cancellation, edited SSH endpoint handling, credential/profile separation, and session-schema compatibility. macOS and Linux were exercised directly; Windows behavior was reviewed and covered by the cross-platform typecheck/test/build paths but was not manually dogfooded.
Security Audit
pg_cancel_backendfor the captured backend PID.safeStoragewhere available.0600and write/permission failures fail loudly.Known limitation: a headless Linux runtime without a usable secret service falls back to a random vault key stored in a mode-
0600file. This protects against config leakage and other OS users, but not compromise of the Orca Unix account. The UI and documentation should not describe that fallback as equivalent to an OS keychain.Notes