feat(database): in-app Postgres/MySQL client (connect, schema browser, SQL query)#6983
Open
kenzouno1 wants to merge 21 commits into
Open
feat(database): in-app Postgres/MySQL client (connect, schema browser, SQL query)#6983kenzouno1 wants to merge 21 commits into
kenzouno1 wants to merge 21 commits into
Conversation
…wser
In-app database client for external Postgres/MySQL servers. Drivers (pg,
mysql2) are bundled and lazy-imported on first connect; all queries run in
the main process over IPC gated to the trusted UI renderer. The renderer
never sees passwords, only password-stripped summaries.
Connections
- Global connection list with add/edit/delete and a Database top-level view
- Passwords encrypted via a strong safeStorage backend; fail-closed decrypt,
warn-and-store behind a banner when the OS keystore is weak or absent
Lifecycle
- Driver abstraction with connect timeout and a mandatory 'error' listener,
so a dropped connection degrades to lost instead of crashing the main
process; connect-race guard and quit-time disposal
- Pooled connections; SSL smart-by-host (localhost disable, remote
verify-full); LOCAL INFILE and multi-statement disabled
- IPC errors normalized to a safe {code, message} allow-list, never raw DSNs
Schema browser
- Lazy, capped introspection (schemas then tables then columns) on a pooled
connection separate from the query path; overflow surfaced when capped
- Virtualized, keyboard-navigable tree with per-node lazy load and refresh
Add the query workflow to the database client: a Monaco SQL editor, a virtualized results grid, and query execution that enforces safety at the database rather than by string matching. - Read-only connections run reads inside a database read-only transaction (Postgres BEGIN + SET TRANSACTION READ ONLY, MySQL START TRANSACTION READ ONLY); allowWrite is derived from the stored connection server-side and never trusted from the renderer - Results are bounded by a server-side cursor (Postgres DECLARE/FETCH) or a row-by-row stream (MySQL) capped at rowLimit+1; the user SQL is never rewritten - Running queries are cancellable server-side via a short-lived side connection (pg_cancel_backend / KILL QUERY) using the captured backend id - Statement timeouts bound runtime; a conservative confirm dialog guards destructive statements on writable connections - Monaco SQL basic-language registered for highlighting A shared SQL classifier drives the cursor path and the confirm dialog only; it is never the security boundary.
… introspection Address code-review findings on the database client: - Reject multi-statement input on read-only connections. A Postgres simple query runs every statement, so a multi-statement string could issue SET TRANSACTION READ WRITE before any query and defeat the read-only transaction; the manager now blocks multi-statement when writes are disallowed (the read-only txn still covers single-statement writes and writing CTEs). - Skip COMMIT on a MySQL connection poisoned by an early stream teardown. On a truncated read the connection has undrained packets; committing would either desync and lose the valid rows or drain the whole result. The read-only transaction is abandoned when the connection is destroyed instead. - Store the column-cache key separator as an escaped code unit instead of a raw NUL byte so the store slice stays a text file (was diffed as binary). - Bound introspection runtime: Postgres statement_timeout on the pool; MySQL a per-query client timeout. Also bound MySQL writes with a client timeout (max_execution_time only covers SELECT). - Reject a second concurrent query on the same connection so cancel always targets the running query.
|
Ready to review this PR? Stage has broken it down into 16 individual chapters for you: 
Chapters generated by Stage for commit 6708c48 on Jul 1, 2026 9:46am UTC. |
Radix <SelectItem> forbids an empty-string value (it is reserved for clearing the selection). The SSL field used value="" for the "Auto (smart by host)" option, which threw and crashed the connection form's error boundary when the dropdown opened. Use an 'auto' sentinel that maps to undefined in the payload, and add a regression test asserting the SSL field never yields an empty string.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThis PR adds database support across shared types, storage, Postgres/MySQL drivers, IPC, preload APIs, renderer state, and database UI. It introduces connection CRUD, secret storage, schema browsing, query and batch execution, table-data editing, a database page, and locale strings for the new feature. Sequence Diagram(s)sequenceDiagram
participant Renderer
participant PreloadAPI
participant DatabaseIPC
participant DbConnectionManager
participant Driver
Renderer->>PreloadAPI: database.query / execute / executeBatch
PreloadAPI->>DatabaseIPC: ipcRenderer.invoke(...)
DatabaseIPC->>DbConnectionManager: query / execute / executeBatch
DbConnectionManager->>Driver: delegated DB operation
Driver-->>DbConnectionManager: result or DbSafeError
DbConnectionManager-->>DatabaseIPC: response payload
DatabaseIPC-->>PreloadAPI: typed IPC result
PreloadAPI-->>Renderer: database result
Suggested Labelsfeature, database Suggested ReviewersNot enough information available to suggest specific reviewers. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (2)
src/renderer/src/components/database/QueryWorkspace.tsx (1)
79-81: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winShortcut label should be platform-specific, not "⌘/Ctrl" combined.
Showing both symbols together doesn't match the platform-specific convention and is confusing on non-Mac systems (no ⌘ key exists there).
Based on learnings/path instructions: "Use `CmdOrCtrl` for Electron menu accelerators and display `⌘` / `⇧` on Mac and `Ctrl+` / `Shift+` on other platforms in UI shortcut labels."♻️ Suggested platform-specific label
+const isMac = navigator.userAgent.includes('Mac') + ... <span className="ml-auto text-[11px] text-muted-foreground"> - {translate('auto.components.database.QueryWorkspace.runHint', '⌘/Ctrl + Enter to run')} + {translate( + 'auto.components.database.QueryWorkspace.runHint', + isMac ? '⌘ + Enter to run' : 'Ctrl + Enter to run' + )} </span>Note: the
en.json(and other locale) fallback string for this key should be updated to match once this is fixed.Source: Path instructions
src/renderer/src/store/slices/database.ts (1)
182-191: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor duplication between
applyDbStatusand thesubscribeDbStatusChangescallback.Both blocks do the identical
set(...dbStatuses...)+dropCacheForNonLive(state)sequence. Could be consolidated into a single internal helper (e.g.applyStatusUpdate) called from all three sites (including the disconnect fix above).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: a9f1cd97-e201-4a8c-b8fc-1c3013dd703c
⛔ 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.cjspackage.jsonsrc/main/database/db-connection-manager.test.tssrc/main/database/db-connection-manager.tssrc/main/database/db-credential-store.test.tssrc/main/database/db-credential-store.tssrc/main/database/db-driver.test.tssrc/main/database/db-driver.tssrc/main/database/mysql-driver.test.tssrc/main/database/mysql-driver.tssrc/main/database/mysql-introspection-queries.test.tssrc/main/database/mysql-introspection-queries.tssrc/main/database/mysql-query.test.tssrc/main/database/mysql-query.tssrc/main/database/postgres-driver.test.tssrc/main/database/postgres-driver.tssrc/main/database/postgres-introspection-queries.test.tssrc/main/database/postgres-introspection-queries.tssrc/main/database/postgres-query.test.tssrc/main/database/postgres-query.tssrc/main/index.tssrc/main/ipc/database.test.tssrc/main/ipc/database.tssrc/main/ipc/register-core-handlers.test.tssrc/main/ipc/register-core-handlers.tssrc/main/ipc/ui.tssrc/main/persistence-db-connections.test.tssrc/main/persistence.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/App.tsxsrc/renderer/src/components/database/ConnectionForm.tsxsrc/renderer/src/components/database/ConnectionList.tsxsrc/renderer/src/components/database/DatabasePage.tsxsrc/renderer/src/components/database/QueryEditor.tsxsrc/renderer/src/components/database/QueryWorkspace.tsxsrc/renderer/src/components/database/ResultsGrid.tsxsrc/renderer/src/components/database/SchemaTree.tsxsrc/renderer/src/components/database/connection-encryption-warning.tsxsrc/renderer/src/components/database/connection-form-defaults.tssrc/renderer/src/components/database/connection-status-indicator.tsxsrc/renderer/src/components/database/schema-tree-rows.test.tssrc/renderer/src/components/database/schema-tree-rows.tssrc/renderer/src/components/sidebar/SidebarNav.tsxsrc/renderer/src/hooks/resolve-zoom-target.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/monaco-sql-language.test.tssrc/renderer/src/lib/monaco-sql-language.tssrc/renderer/src/lib/right-sidebar-visibility.test.tssrc/renderer/src/lib/right-sidebar-visibility.tssrc/renderer/src/store/index.tssrc/renderer/src/store/slices/database.tssrc/renderer/src/store/slices/diffComments.test.tssrc/renderer/src/store/slices/store-test-helpers.tssrc/renderer/src/store/slices/ui.tssrc/renderer/src/store/types.tssrc/renderer/src/web/web-preload-api.tssrc/shared/constants.tssrc/shared/database-types.tssrc/shared/sql-statement-classifier.test.tssrc/shared/sql-statement-classifier.tssrc/shared/types.ts
# Conflicts: # src/main/persistence.ts
…L parsing - await pool teardown during app quit so connections close before exit - validate partial connection updates before persisting them - scope driver error listeners per connection; clear in-flight state on disconnect/reconnect so a replacement pool isn't torn down by a late error - bound the MySQL validation ping and guard timeout teardown callbacks - run writable transaction-block-unsafe statements in autocommit - fix Postgres primary-key introspection join across table name - detect statement separators inside quoted identifiers and strings - retry SQL language registration after a failed chunk load - toast copy success only after the clipboard write resolves - show the platform-specific run-shortcut modifier
Clicking a table or view in the schema browser loads a bounded SELECT into the query editor and runs it, so the generated SQL stays visible and editable. The chevron owns expand/collapse so it no longer competes with the row's primary action. Identifiers are engine-quoted (doubled quote char) to keep reserved/quoted names from breaking out.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plans/260630-1030-database-client-postgres-mysql/phase-05-query-editor-and-results-grid.md (1)
89-90: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMinor wording redundancy: "SQL language" → "SQL".
"language" is redundant since it's registering the Monaco "SQL" basic-language.
Source: Linters/SAST tools
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e4115d94-41d2-4e90-9e5e-cc4c9165299e
📒 Files selected for processing (40)
plans/260630-1030-database-client-postgres-mysql/phase-01-scaffold-and-driver-deps.mdplans/260630-1030-database-client-postgres-mysql/phase-02-connection-model-and-secure-persistence.mdplans/260630-1030-database-client-postgres-mysql/phase-03-driver-abstraction-and-connect.mdplans/260630-1030-database-client-postgres-mysql/phase-04-schema-browser.mdplans/260630-1030-database-client-postgres-mysql/phase-05-query-editor-and-results-grid.mdplans/260630-1030-database-client-postgres-mysql/plan.mdplans/reports/brainstorm-260630-1030-database-client-postgres-mysql-report.mdplans/reports/from-code-reviewer-to-controller-database-client-branch-review.mdplans/reports/fullstack-developer-260701-0750-database-client-phase3-driver-connect-report.mdplans/reports/fullstack-developer-260701-0823-database-client-phase4-schema-browser-report.mdplans/reports/fullstack-developer-260701-0848-database-client-phase5-query-editor-report.mdplans/reports/pr6983-stagereview-risk-review-and-fixes-report.mdplans/reports/tester-260630-1736-database-client-phase2-test-report.mdsrc/main/database/db-connection-manager.test.tssrc/main/database/db-connection-manager.tssrc/main/database/db-driver.test.tssrc/main/database/db-driver.tssrc/main/database/mysql-driver.tssrc/main/database/postgres-introspection-queries.tssrc/main/database/postgres-query.test.tssrc/main/database/postgres-query.tssrc/main/index.tssrc/main/ipc/database.test.tssrc/main/ipc/database.tssrc/main/persistence-db-connections.test.tssrc/renderer/src/components/database/QueryWorkspace.tsxsrc/renderer/src/components/database/ResultsGrid.tsxsrc/renderer/src/components/database/SchemaTree.tsxsrc/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/monaco-sql-language.test.tssrc/renderer/src/lib/monaco-sql-language.tssrc/renderer/src/store/slices/database.tssrc/shared/sql-statement-classifier.test.tssrc/shared/sql-statement-classifier.tssrc/shared/table-preview-query.test.tssrc/shared/table-preview-query.ts
✅ Files skipped from review due to trivial changes (8)
- plans/reports/fullstack-developer-260701-0848-database-client-phase5-query-editor-report.md
- src/shared/table-preview-query.test.ts
- plans/260630-1030-database-client-postgres-mysql/phase-04-schema-browser.md
- plans/reports/fullstack-developer-260701-0750-database-client-phase3-driver-connect-report.md
- plans/260630-1030-database-client-postgres-mysql/phase-01-scaffold-and-driver-deps.md
- plans/260630-1030-database-client-postgres-mysql/phase-02-connection-model-and-secure-persistence.md
- plans/reports/fullstack-developer-260701-0823-database-client-phase4-schema-browser-report.md
- src/renderer/src/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes (23)
- src/renderer/src/lib/monaco-sql-language.test.ts
- src/renderer/src/lib/monaco-sql-language.ts
- src/main/index.ts
- src/shared/sql-statement-classifier.test.ts
- src/main/database/mysql-driver.ts
- src/main/database/postgres-query.test.ts
- src/main/database/db-driver.ts
- src/renderer/src/i18n/locales/es.json
- src/shared/sql-statement-classifier.ts
- src/main/database/postgres-query.ts
- src/renderer/src/components/database/ResultsGrid.tsx
- src/main/database/db-driver.test.ts
- src/main/ipc/database.test.ts
- src/main/persistence-db-connections.test.ts
- src/renderer/src/store/slices/database.ts
- src/main/database/db-connection-manager.test.ts
- src/renderer/src/components/database/QueryWorkspace.tsx
- src/renderer/src/i18n/locales/zh.json
- src/main/database/postgres-introspection-queries.ts
- src/main/ipc/database.ts
- src/main/database/db-connection-manager.ts
- src/renderer/src/i18n/locales/ko.json
- src/renderer/src/components/database/SchemaTree.tsx
…action menus Move connect/disconnect/edit/delete into an overflow menu and right-click context menu sharing one action list; double-click or Enter opens a live connection or connects an idle one. Highlight the whole row for the active connection and simplify the metadata line to a status dot, name, and engine badge.
Add a DBeaver-style data grid on top of the DB client: a tabbed workspace (a permanent Query tab plus a Data tab per opened table), server-side sort/filter/pagination, and staged cell edit + insert + delete applied atomically and keyed by primary key. Free-form query results gain the same sort/filter/paginate by wrapping the read as a subquery (ordinal sort so duplicate column names still work). Parameterized writes run through new trusted-gated database:execute and database:executeBatch IPC channels; allowWrite stays server-derived from the stored readOnly, and editing is enabled only for tables with a primary key on a writable connection.
Author
Update: editable data grid (sort / filter / edit)Pushed a DBeaver-style data grid on top of the client (was out of scope in v1):
Writes are parameterized (values as bind params, identifiers quote-escaped) through new trusted-gated Verification: 1757 unit tests; |
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
src/renderer/src/components/database/data-grid-cell-format.ts (1)
7-15: 🎯 Functional Correctness | 🔵 TrivialConsider special-casing
Date/binary values informatCell.
typeof value === 'object'routesDateand any binary blob (bytea/BLOB columns) throughJSON.stringify, producing a quoted ISO string or a noisy array/object dump instead of a readable representation — relevant for a DB client that will render arbitrary column types.♻️ Proposed refactor
export function formatCell(value: unknown): { text: string; isNull: boolean } { if (value === null || value === undefined) { return { text: 'NULL', isNull: true } } + if (value instanceof Date) { + return { text: value.toISOString(), isNull: false } + } if (typeof value === 'object') { return { text: JSON.stringify(value), isNull: false } } return { text: String(value), isNull: false } }src/renderer/src/components/database/TableDataView.tsx (1)
83-87: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
savingstate against a thrownsaveDbEdits.If
saveDbEditsever rejects (vs. catching internally and writing toview.saveError, asqueryState.errordoes elsewhere),savingstaystrueforever, permanently disabling Save/Revert for that tab. Worth atry/finallyregardless of current store-action behavior.🛡️ Suggested fix
const onSave = async (): Promise<void> => { setSaving(true) - await saveDbEdits(connectionId, tabId) - setSaving(false) + try { + await saveDbEdits(connectionId, tabId) + } finally { + setSaving(false) + } }src/main/ipc/database.ts (1)
296-368: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winExtract the repeated
allowWritederivation into a shared helper.The same 2-line server-side
allowWritederivation is now duplicated acrossdatabase:query,database:execute, anddatabase:executeBatch. Since this is the core read-only enforcement check (explicitly called out in the PR objectives as security-critical), consider extracting it once to reduce the chance that a future edit updates one call site but not the others.♻️ Suggested extraction
+ const resolveAllowWrite = (id: string): boolean => { + const connection = store.getDbConnection(id) + return connection ? !connection.readOnly : false + } + ipcMain.handle( 'database:query', async (event, args: { id: string; sql: string }): Promise<DbQueryResult> => { requireTrusted(event.sender) try { - const connection = store.getDbConnection(args.id) - const allowWrite = connection ? !connection.readOnly : false + const allowWrite = resolveAllowWrite(args.id) const result = await dbConnectionManager.query(args.id, args.sql, {(repeat similarly for
database:executeanddatabase:executeBatch)
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e0b96ed9-0232-4e62-9e43-9ae853baacab
📒 Files selected for processing (54)
config/localization-coverage-allowlist.jsonplans/reports/implementation-260701-1105-database-data-grid-editor-report.mdsrc/main/database/db-connection-manager.test.tssrc/main/database/db-connection-manager.tssrc/main/database/db-driver.tssrc/main/database/mysql-driver.tssrc/main/database/mysql-query.test.tssrc/main/database/mysql-query.tssrc/main/database/postgres-driver.tssrc/main/database/postgres-query.test.tssrc/main/database/postgres-query.tssrc/main/ipc/database.test.tssrc/main/ipc/database.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/components/database/ConnectionList.tsxsrc/renderer/src/components/database/DataGridColumnHeader.tsxsrc/renderer/src/components/database/DatabasePage.tsxsrc/renderer/src/components/database/DatabaseWorkspace.tsxsrc/renderer/src/components/database/QueryWorkspace.tsxsrc/renderer/src/components/database/ResultsGrid.tsxsrc/renderer/src/components/database/SchemaTree.tsxsrc/renderer/src/components/database/TableDataCell.tsxsrc/renderer/src/components/database/TableDataCellEditor.tsxsrc/renderer/src/components/database/TableDataGrid.tsxsrc/renderer/src/components/database/TableDataRowMenu.tsxsrc/renderer/src/components/database/TableDataView.tsxsrc/renderer/src/components/database/connection-row.tsxsrc/renderer/src/components/database/connection-status-indicator.tsxsrc/renderer/src/components/database/data-grid-cell-format.tssrc/renderer/src/components/database/data-grid-column-filter.tsxsrc/renderer/src/components/database/data-grid-filters.test.tssrc/renderer/src/components/database/data-grid-filters.tssrc/renderer/src/components/database/data-grid-sort-state.test.tssrc/renderer/src/components/database/data-grid-sort-state.tssrc/renderer/src/components/database/table-data-edit-buffer.test.tssrc/renderer/src/components/database/table-data-edit-buffer.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/store/database-workspace-tabs.test.tssrc/renderer/src/store/database-workspace-tabs.tssrc/renderer/src/store/slices/database-query-refine.test.tssrc/renderer/src/store/slices/database-table-data.test.tssrc/renderer/src/store/slices/database-table-edit.test.tssrc/renderer/src/store/slices/database.tssrc/renderer/src/web/web-preload-api.tssrc/shared/database-types.tssrc/shared/sql-identifier.tssrc/shared/table-data-sql.test.tssrc/shared/table-data-sql.tssrc/shared/table-preview-query.ts
✅ Files skipped from review due to trivial changes (4)
- src/renderer/src/components/database/data-grid-filters.test.ts
- src/shared/sql-identifier.ts
- src/renderer/src/components/database/data-grid-sort-state.test.ts
- src/renderer/src/components/database/data-grid-sort-state.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/shared/table-preview-query.ts
- src/renderer/src/components/database/DatabasePage.tsx
- src/preload/api-types.ts
- src/renderer/src/components/database/QueryWorkspace.tsx
- src/renderer/src/web/web-preload-api.ts
- src/renderer/src/components/database/SchemaTree.tsx
- src/main/database/mysql-driver.ts
- src/renderer/src/i18n/locales/ko.json
- src/renderer/src/i18n/locales/zh.json
- src/renderer/src/i18n/locales/ja.json
- src/main/database/db-connection-manager.ts
- src/main/database/db-driver.ts
- src/renderer/src/i18n/locales/en.json
Address review feedback on the data grid editor: - executeBatch runs a read-only transaction when !allowWrite (pg + mysql) so a read-only connection stays DB-enforced on the write path too. - bufferToStatements fails fast when a staged row cannot be keyed rather than binding undefined into an UPDATE/DELETE; saveDbEdits surfaces it as a save error. - table-data loads and free-form query runs/refines carry a sequence guard so a slow earlier response cannot overwrite a newer sort/filter/page result. - formatCell renders Date as ISO and binary columns as a byte count.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/renderer/src/components/database/data-grid-cell-format.test.ts (1)
1-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winNo test coverage for
copyCell.Only
formatCellis tested;copyCell's clipboard-write + success/error toast branching (lines 25-33 indata-grid-cell-format.ts) is untested. Consider adding tests that mocknavigator.clipboard.writeTextto resolve/reject and assert the correct toast is shown, and that NULL values are copied as an empty string rather than the literal"NULL".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 921b0d14-1ca5-4330-9ae7-fb2870a7b02a
📒 Files selected for processing (55)
config/localization-coverage-allowlist.jsonplans/reports/implementation-260701-1105-database-data-grid-editor-report.mdsrc/main/database/db-connection-manager.test.tssrc/main/database/db-connection-manager.tssrc/main/database/db-driver.tssrc/main/database/mysql-driver.tssrc/main/database/mysql-query.test.tssrc/main/database/mysql-query.tssrc/main/database/postgres-driver.tssrc/main/database/postgres-query.test.tssrc/main/database/postgres-query.tssrc/main/ipc/database.test.tssrc/main/ipc/database.tssrc/preload/api-types.tssrc/preload/index.tssrc/renderer/src/components/database/ConnectionList.tsxsrc/renderer/src/components/database/DataGridColumnHeader.tsxsrc/renderer/src/components/database/DatabasePage.tsxsrc/renderer/src/components/database/DatabaseWorkspace.tsxsrc/renderer/src/components/database/QueryWorkspace.tsxsrc/renderer/src/components/database/ResultsGrid.tsxsrc/renderer/src/components/database/SchemaTree.tsxsrc/renderer/src/components/database/TableDataCell.tsxsrc/renderer/src/components/database/TableDataCellEditor.tsxsrc/renderer/src/components/database/TableDataGrid.tsxsrc/renderer/src/components/database/TableDataRowMenu.tsxsrc/renderer/src/components/database/TableDataView.tsxsrc/renderer/src/components/database/connection-row.tsxsrc/renderer/src/components/database/connection-status-indicator.tsxsrc/renderer/src/components/database/data-grid-cell-format.test.tssrc/renderer/src/components/database/data-grid-cell-format.tssrc/renderer/src/components/database/data-grid-column-filter.tsxsrc/renderer/src/components/database/data-grid-filters.test.tssrc/renderer/src/components/database/data-grid-filters.tssrc/renderer/src/components/database/data-grid-sort-state.test.tssrc/renderer/src/components/database/data-grid-sort-state.tssrc/renderer/src/components/database/table-data-edit-buffer.test.tssrc/renderer/src/components/database/table-data-edit-buffer.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/store/database-workspace-tabs.test.tssrc/renderer/src/store/database-workspace-tabs.tssrc/renderer/src/store/slices/database-query-refine.test.tssrc/renderer/src/store/slices/database-table-data.test.tssrc/renderer/src/store/slices/database-table-edit.test.tssrc/renderer/src/store/slices/database.tssrc/renderer/src/web/web-preload-api.tssrc/shared/database-types.tssrc/shared/sql-identifier.tssrc/shared/table-data-sql.test.tssrc/shared/table-data-sql.tssrc/shared/table-preview-query.ts
🚧 Files skipped from review as they are similar to previous changes (50)
- src/renderer/src/components/database/data-grid-sort-state.test.ts
- config/localization-coverage-allowlist.json
- src/renderer/src/components/database/data-grid-filters.test.ts
- src/shared/sql-identifier.ts
- src/renderer/src/components/database/TableDataRowMenu.tsx
- src/shared/table-preview-query.ts
- src/renderer/src/store/database-workspace-tabs.test.ts
- src/renderer/src/components/database/DatabasePage.tsx
- src/renderer/src/web/web-preload-api.ts
- src/renderer/src/components/database/DatabaseWorkspace.tsx
- src/renderer/src/components/database/data-grid-column-filter.tsx
- src/renderer/src/store/slices/database-table-edit.test.ts
- src/renderer/src/components/database/connection-status-indicator.tsx
- src/renderer/src/components/database/TableDataCellEditor.tsx
- src/renderer/src/components/database/TableDataCell.tsx
- src/renderer/src/components/database/table-data-edit-buffer.test.ts
- src/renderer/src/components/database/DataGridColumnHeader.tsx
- src/renderer/src/store/slices/database-table-data.test.ts
- src/renderer/src/components/database/data-grid-sort-state.ts
- src/renderer/src/components/database/connection-row.tsx
- src/renderer/src/components/database/ConnectionList.tsx
- src/shared/table-data-sql.test.ts
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/store/database-workspace-tabs.ts
- src/main/database/postgres-query.test.ts
- src/renderer/src/components/database/TableDataGrid.tsx
- src/renderer/src/store/slices/database-query-refine.test.ts
- src/main/database/postgres-driver.ts
- src/renderer/src/components/database/TableDataView.tsx
- src/preload/index.ts
- src/shared/table-data-sql.ts
- src/renderer/src/i18n/locales/es.json
- src/main/database/db-driver.ts
- src/preload/api-types.ts
- src/main/database/mysql-driver.ts
- src/main/database/mysql-query.test.ts
- src/main/ipc/database.ts
- src/renderer/src/components/database/data-grid-filters.ts
- src/renderer/src/components/database/table-data-edit-buffer.ts
- src/renderer/src/components/database/SchemaTree.tsx
- src/renderer/src/components/database/QueryWorkspace.tsx
- src/main/database/db-connection-manager.test.ts
- src/renderer/src/components/database/ResultsGrid.tsx
- src/renderer/src/store/slices/database.ts
- src/main/database/db-connection-manager.ts
- src/renderer/src/i18n/locales/ja.json
- src/renderer/src/i18n/locales/ko.json
- src/shared/database-types.ts
- src/main/ipc/database.test.ts
- src/renderer/src/i18n/locales/zh.json
MySQL verify-full only validated the certificate chain, not the server hostname (mysql2 gates that on verifyIdentity), so a CA-signed cert issued for another host could MITM a remote connection. Set verifyIdentity for verify-full so the hostname is checked too, matching Postgres. Return BIGINT/DECIMAL as strings (supportBigNumbers + bigNumberStrings) so values above 2^53 keep full precision on read and on PK-keyed edits.
Block inline editing of binary/array/JSON cells and compare cells by their display form, so an unchanged complex cell can no longer stage its lossy text (e.g. "[3 bytes]") as a write. Disable editing during an in-flight reload and confirm before closing a Data tab with unsaved edits. Make result cells natively text-selectable (drop click-to-copy). Also guard schema-tree double-load, use a monotonic load epoch, drop stale row-count responses, block save while encryption status is still loading, and fix schema-tree keyboard navigation.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 65294a0c-5b3b-4e4a-bc92-9fb5147b7527
📒 Files selected for processing (12)
src/main/database/mysql-driver.test.tssrc/main/database/mysql-driver.tssrc/renderer/src/components/database/ConnectionForm.tsxsrc/renderer/src/components/database/DatabaseWorkspace.tsxsrc/renderer/src/components/database/ResultsGrid.tsxsrc/renderer/src/components/database/SchemaTree.tsxsrc/renderer/src/components/database/TableDataCell.tsxsrc/renderer/src/components/database/TableDataView.tsxsrc/renderer/src/components/database/data-grid-cell-format.tssrc/renderer/src/components/database/table-data-edit-buffer.test.tssrc/renderer/src/components/database/table-data-edit-buffer.tssrc/renderer/src/store/slices/database.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/database/mysql-driver.test.ts
- src/renderer/src/components/database/table-data-edit-buffer.test.ts
- src/renderer/src/components/database/ConnectionForm.tsx
- src/renderer/src/components/database/table-data-edit-buffer.ts
- src/main/database/mysql-driver.ts
- src/renderer/src/components/database/TableDataView.tsx
- src/renderer/src/store/slices/database.ts
- src/renderer/src/components/database/SchemaTree.tsx
…failure Route database:execute reads through the bounded path (Postgres cursor / MySQL row-by-row stream) so a statement that ships without its own LIMIT can no longer materialize the whole result set in the main process. When ROLLBACK itself fails, release the Postgres client with an eviction flag so a connection that may still hold an open transaction is dropped from the pool instead of being reused.
Wrap JSON.stringify in formatCell so a BigInt (possible inside a JSON column now that the MySQL driver returns big numbers) or a circular reference falls back to String() instead of crashing the grid row render. Make DbColumnFilter a discriminated union so is-null/is-not-null carry no value and every other operator requires one — a comparison filter can no longer bind an accidental undefined.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/database/mysql-query.ts (1)
82-98: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound MySQL row-producing reads before
runDirect().isCursorableRead()only sendsSELECT/WITH/VALUES/TABLEthroughstreamBounded(), soSHOW/EXPLAIN/DESCRIBE/DESCstill go throughconn.query()and can buffer a large result set in memory. Route those reads through the bounded path too.src/main/database/postgres-query.ts (1)
32-67: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound row-returning writes in the direct path.
fetchBoundedstill buffers every row for non-cursorable statements, so... RETURNINGcan bypassrowLimitand blow up memory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 7e3c63a9-6bc8-4a25-9000-fcbeaccc9450
📒 Files selected for processing (8)
src/main/database/mysql-query.test.tssrc/main/database/mysql-query.tssrc/main/database/postgres-query.test.tssrc/main/database/postgres-query.tssrc/renderer/src/components/database/data-grid-cell-format.tssrc/renderer/src/components/database/data-grid-column-filter.tsxsrc/renderer/src/components/database/data-grid-filters.test.tssrc/shared/database-types.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- src/renderer/src/components/database/data-grid-filters.test.ts
- src/renderer/src/components/database/data-grid-cell-format.ts
- src/renderer/src/components/database/data-grid-column-filter.tsx
- src/main/database/postgres-query.test.ts
- src/main/database/mysql-query.test.ts
- src/shared/database-types.ts
The batch paths ran COMMIT outside the try, so a failing COMMIT skipped rollback and returned a client with unknown transaction state to the pool; the MySQL query/execute paths released a connection whose ROLLBACK had failed. Now every run path rolls back and evicts (pg release(true)) / destroys (MySQL) the connection on any COMMIT or ROLLBACK failure.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/database/mysql-query.ts (1)
136-145: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winPoison connections when COMMIT fails, not only when ROLLBACK fails.
A COMMIT rejection can still leave
poisoned === false; if the follow-up ROLLBACK succeeds,finallyreleases the connection back to the pool. Mark it poisoned as soon as COMMIT fails.As per PR objectives, “if COMMIT or ROLLBACK fails, the affected connection is no longer returned to the pool.”
Proposed fix
if (!poisoned) { - await conn.query('COMMIT') + await conn.query('COMMIT').catch((err) => { + poisoned = true + throw err + }) } @@ if (!poisoned) { - await conn.query('COMMIT') + await conn.query('COMMIT').catch((err) => { + poisoned = true + throw err + }) } @@ try { await conn.query('COMMIT') } catch (err) { - await conn.query('ROLLBACK').catch(() => { - poisoned = true - }) + poisoned = true + await conn.query('ROLLBACK').catch(() => {}) throw err }Also applies to: 194-203, 249-257
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: e153f2ca-d7c0-437b-a058-ea3331ce8e46
📒 Files selected for processing (4)
src/main/database/mysql-query.test.tssrc/main/database/mysql-query.tssrc/main/database/postgres-query.test.tssrc/main/database/postgres-query.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/database/mysql-query.test.ts
- src/main/database/postgres-query.test.ts
- src/main/database/postgres-query.ts
The tab wrapper's Enter/Space handler preventDefaults the focused close button's native click, so keyboard users could only re-select the tab. Stop the key event bubbling from the close button so its click fires.
Resolve conflicts in src/main/persistence.ts by combining main's state-hash write-skip optimization (buildStateToSave/computeStateHash) with this branch's DB-secrets encryption and file-hardening logic.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/persistence.ts (2)
3511-3525: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftFix the stale async rename vs sync flush race.
flushOrThrow()can bumpwriteGenerationwhile an async write is already awaitingrename(). If that stale rename completes after the sync flush, it can overwrite fresher state; the post-rename generation check only avoids updatinglastWrittenStateHash, not the file contents.Proposed recovery after a stale async rename lands
await rename(tmpFile, dataFile) renamed = true // Why the gen re-check: a sync flush can interleave during the rename // await, write fresher state, and record its own hash. Recording this // stale hash over it would make later saves skip against content that // is not what the file holds. if (this.writeGeneration === gen) { this.lastWrittenStateHash = stateHash + } else { + // Restore the latest in-memory state if this stale async rename landed + // after a synchronous flush bumped the generation. + this.writeToDiskSync({ force: true }) }Also applies to: 3599-3604
3453-3460: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReapply ACLs to rotated backups.
rotateBackupsAsync/rotateBackupsSynccopy the secret-bearing file into.bak.*without hardening the new snapshot, so those backups can remain readable under default permissions. Harden each backup path after the copy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 400e045b-d024-414c-ae14-322226610e2b
📒 Files selected for processing (12)
package.jsonsrc/main/persistence.tssrc/preload/api-types.tssrc/preload/index.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/web/web-preload-api.tssrc/shared/constants.tssrc/shared/types.ts
💤 Files with no reviewable changes (3)
- src/shared/constants.ts
- src/shared/types.ts
- src/renderer/src/web/web-preload-api.ts
✅ Files skipped from review due to trivial changes (1)
- src/renderer/src/i18n/locales/zh.json
🚧 Files skipped from review as they are similar to previous changes (7)
- src/preload/api-types.ts
- package.json
- src/preload/index.ts
- src/renderer/src/i18n/locales/ko.json
- src/renderer/src/i18n/locales/es.json
- src/renderer/src/i18n/locales/en.json
- src/renderer/src/i18n/locales/ja.json
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an in-app Database client for external Postgres/MySQL: connect, browse schema, run SQL, view results — without leaving Orca. Mirrors the SSH feature's connection-list + encrypted-secrets model. Drivers (
pg,mysql2) are bundled and lazy-imported on first connect; all queries run in the main process over IPC gated to the trusted UI renderer. The sandboxed renderer never sees passwords — only password-stripped summaries.Built in 5 phases (plan under
plans/260630-1030-database-client-postgres-mysql/).What's included
Databasetop-level view. Passwords encrypted at rest via a strongsafeStoragebackend with fail-closed decrypt; on a weak/absent OS keystore, a banner + informed-consent warn-and-store (never a silent weak write).DbDriverabstraction, connect timeout (Promise.race), a mandatory pool'error'listener (a dropped connection degrades tolost, never crashes the main process), connect-race guard, and quit-time disposal. SSL is smart-by-host (localhost→disable, remote→verify-full); LOCAL INFILE and multi-statement are disabled. IPC errors are normalized to a credential-free{code, safeMessage}.SET TRANSACTION READ ONLY/ MySQLSTART TRANSACTION READ ONLY), never by keyword matching.allowWriteis derived from the stored connection server-side and cannot be spoofed by the renderer.DECLARE/FETCH rowLimit+1) or row-by-row stream (MySQL), never buffering the whole result; user SQL is never rewritten (no appendedLIMIT).pg_cancel_backend/KILL QUERY) using the captured backend id.Security posture / red-team
Planning included a 15-finding red-team pass; all Critical/High items are addressed (drivers in the packaged-runtime allowlist, fail-closed credential store, DB-enforced read-only, unhandled-
'error'crash fix, connect timeout, error redaction, capped introspection, real server-side cancel, trusted-sender IPC gate, Monaco SQL registration).A post-implementation branch review (report in
plans/reports/) found 2 High + 2 Medium + Lows, all fixed in the final commit:SET TRANSACTION READ WRITE; …→ read-only connections now reject multi-statement input.statement_timeout, MySQL per-query timeout).Testing
typecheck(node + web) ✓ ·oxlint+ switch-exhaustiveness ✓ · localization catalog parity + coverage ✓ (keys added across en/es/ja/ko/zh).Known limitations / follow-ups
sshTunnelmodel field is reserved but the tunnel is not wired) — a UI disclosure + true tunnel are follow-ups.dataTypeis names-only for now (no OID/type-code → name map); headers render correctly.Notes for reviewers
Cross-platform (macOS/Linux/Windows) via
CmdOrCtrl/ platform checks; STYLEGUIDE tokens + shadcn primitives for UI; nomax-linesdisables.