feat(database): injectable console page (schema browser + SQL panel) - #598
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
skill-check — worker0 verified, 48 skipped (no docs/).
Four for four. Nicely done. |
Ships the database worker's #/ext/database page over the injectable-UI protocol (#570): a schema tree with lazy per-driver column/PK/FK/index introspection, a sortable paginated row grid with a row inspector, and a read-only SQL panel (shared Monaco CodeEditor, EXPLAIN, per-db history, client-side write-verb gate). Reads the live database via database::query and database::listDatabases. Registered through host.pages.register in the existing database/ui setup, alongside the function-trigger renderer. The SQL editor feeds the live table names plus SQL keywords to the CodeEditor completions prop for as-you-type suggestions.
bbb1442 to
9a4a1ed
Compare
0af193f to
c03232f
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
database/ui/src/page/db-data.ts (1)
536-545: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winClamp
pageSize/offsetto non-negative integers before interpolating.Both go straight into the SQL text. They're typed
number, so this isn't injectable, but a fractional orNaNvalue from any caller produces a syntax error rather than a clean failure.♻️ Proposed guard
const ref = quoteTableRef(driver, table) - const offset = page * pageSize + const limit = Math.max(1, Math.trunc(pageSize) || PAGE_SIZE) + const offset = Math.max(0, Math.trunc(page) || 0) * limit const orderBy = sort ? ` ORDER BY ${quoteIdent(driver, sort.column)} ${sort.dir === 'desc' ? 'DESC' : 'ASC'}` : '' const result = await runQuery( host, db, - `SELECT * FROM ${ref}${orderBy} LIMIT ${pageSize} OFFSET ${offset}`, + `SELECT * FROM ${ref}${orderBy} LIMIT ${limit} OFFSET ${offset}`, ) - return { result, page, pageSize } + return { result, page, pageSize: limit }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/ui/src/page/db-data.ts` around lines 536 - 545, Before constructing the SQL in the page query flow, validate and normalize pageSize and the derived offset to non-negative integers, rejecting or safely handling fractional and non-finite values. Update the interpolation in the runQuery call while preserving the existing pagination behavior for valid inputs.database/ui/src/page/result-grid.tsx (1)
150-201: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding
aria-sortto sortable headers.Sort state is only conveyed visually via the arrow icon; screen reader users get no indication of the current sort column/direction. Setting
aria-sort="ascending"|"descending"|"none"on the<th>would close this gap cheaply.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@database/ui/src/page/result-grid.tsx` around lines 150 - 201, Update the sortable header cells in the cols.map rendering to set aria-sort on each <th> from the existing sorted value: use "ascending" or "descending" for the active column and "none" otherwise. Keep the existing visual sort indicators and behavior unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@database/ui/src/page/cells.ts`:
- Around line 17-23: The copyText function's try/catch block does not actually
handle promise rejections from navigator.clipboard.writeText, which returns a
Promise. The try/catch only guards the synchronous call site, leaving rejected
promises unhandled. Add a .catch() handler to the promise returned by
navigator.clipboard.writeText to suppress rejections, ensuring that
permission-denied and other async errors are actually swallowed as the comment
intends.
In `@database/ui/src/page/db-data.ts`:
- Around line 119-130: Update listDbs to throw when
listDatabasesResponseSchema.safeParse fails instead of returning an empty array,
matching runQuery’s shape-mismatch behavior. Preserve the existing database
mapping for successful responses so index.tsx can distinguish malformed
responses from an empty database list.
- Around line 289-311: Update isReadOnlySql and the transactionQuery SQL guard
to accept only one statement, detecting real statement separators while ignoring
semicolons inside strings or comments. Reject any trailing SQL after the first
statement, and explicitly reject writable PRAGMA forms such as journal_mode=WAL
while preserving genuinely read-only PRAGMAs.
In `@database/ui/src/page/icons.tsx`:
- Around line 13-46: Update the Svg component’s accessibility prop handling so
caller-supplied aria-hidden and aria-label values are preserved instead of being
overridden by the trailing aria-hidden="true". Keep the default behavior hidden
only when no accessibility override is provided, while allowing labeled icons
such as KeyRound to remain accessible.
In `@database/ui/src/page/SqlPanel.tsx`:
- Around line 109-115: Update the database-scoped state in the SqlPanel
component when db changes: reload history via loadHistory(db), clear existing
results and errors, and reset related UI state as appropriate. Also invalidate
or cancel in-flight requests during the db-change transition so responses from
the previous database cannot repopulate the panel or carry over its labels.
- Around line 117-181: Update the SQL execution path used by run and
runReadOnlySql to enforce read-only access at the database boundary rather than
relying on the leading-keyword check in SqlPanel. Reject mutating CTEs and
EXPLAIN ANALYZE statements, and configure the RPC worker’s transaction or
connection mode as read-only so writes cannot execute even when client-side
validation is bypassed.
In `@database/ui/src/page/TableDataPanel.tsx`:
- Around line 77-81: Update the TableDataPanel pagination flow to handle unknown
counts without unbounded navigation: incorporate countRead.error, determine
hasNext from an extra fetched row by requesting pageSize + 1 rows and trimming
the sentinel, and use that bounded result to disable or stop advancing past the
final available page. Preserve total-based pagination when the count is known
and update the related page-fetch/render logic consistently.
---
Nitpick comments:
In `@database/ui/src/page/db-data.ts`:
- Around line 536-545: Before constructing the SQL in the page query flow,
validate and normalize pageSize and the derived offset to non-negative integers,
rejecting or safely handling fractional and non-finite values. Update the
interpolation in the runQuery call while preserving the existing pagination
behavior for valid inputs.
In `@database/ui/src/page/result-grid.tsx`:
- Around line 150-201: Update the sortable header cells in the cols.map
rendering to set aria-sort on each <th> from the existing sorted value: use
"ascending" or "descending" for the active column and "none" otherwise. Keep the
existing visual sort indicators and behavior unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d51d412-c7fe-435a-a8fd-d94df485695e
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
database/ui/package.jsondatabase/ui/page.tsxdatabase/ui/src/page/RowDetail.tsxdatabase/ui/src/page/SchemaTree.tsxdatabase/ui/src/page/SqlPanel.tsxdatabase/ui/src/page/TableDataPanel.tsxdatabase/ui/src/page/cells.tsdatabase/ui/src/page/db-data.tsdatabase/ui/src/page/icons.tsxdatabase/ui/src/page/index.tsxdatabase/ui/src/page/page-styles.tsdatabase/ui/src/page/pagination.tsxdatabase/ui/src/page/result-grid.tsxdatabase/ui/src/page/useDatabaseRead.ts
- read-only SQL gate hardened: single-statement only (semicolons in strings/comments ignored), reject any write/DDL keyword anywhere so mutating CTEs and EXPLAIN ANALYZE cannot slip past the leading keyword, and reject writable PRAGMA forms (journal_mode=WAL) while keeping read PRAGMAs - listDbs throws on a shape mismatch like runQuery, so a malformed response is distinguishable from an empty database list - fetchTablePage fetches a pageSize+1 sentinel row so pagination stays bounded when the row count is unknown; normalize page/size to non-negative integers before interpolating - copyText swallows the async clipboard rejection via ?.catch, not just the sync call - icons preserve caller aria-hidden/aria-label (labeled icons stay accessible) instead of forcing aria-hidden - result grid sets aria-sort on sortable headers Worker-side read-only transaction enforcement is out of scope here (it is a database worker change); this tightens the client gate as defense in depth. SqlPanel already remounts per database via its key, so no separate db-change reset is needed.
#597) * feat(console): CodeEditor completions prop for as-you-type suggestions Adds an opt-in `completions?: readonly string[]` to the shared CodeEditor (#579). Non-empty turns on the suggest popup (the default stays prose-quiet, so markdown/json/yaml behavior is byte-for-byte unchanged) and registers a Monaco completion provider for the editor's language offering those words, disposed on unmount. The database SQL panel is the first consumer, feeding it the live table names plus SQL keywords. * feat(database): injectable console page (schema browser + SQL panel) (#598) * feat(database): injectable console page (schema browser + SQL panel) Ships the database worker's #/ext/database page over the injectable-UI protocol (#570): a schema tree with lazy per-driver column/PK/FK/index introspection, a sortable paginated row grid with a row inspector, and a read-only SQL panel (shared Monaco CodeEditor, EXPLAIN, per-db history, client-side write-verb gate). Reads the live database via database::query and database::listDatabases. Registered through host.pages.register in the existing database/ui setup, alongside the function-trigger renderer. The SQL editor feeds the live table names plus SQL keywords to the CodeEditor completions prop for as-you-type suggestions. * fix(database): address review on the injectable page - read-only SQL gate hardened: single-statement only (semicolons in strings/comments ignored), reject any write/DDL keyword anywhere so mutating CTEs and EXPLAIN ANALYZE cannot slip past the leading keyword, and reject writable PRAGMA forms (journal_mode=WAL) while keeping read PRAGMAs - listDbs throws on a shape mismatch like runQuery, so a malformed response is distinguishable from an empty database list - fetchTablePage fetches a pageSize+1 sentinel row so pagination stays bounded when the row count is unknown; normalize page/size to non-negative integers before interpolating - copyText swallows the async clipboard rejection via ?.catch, not just the sync call - icons preserve caller aria-hidden/aria-label (labeled icons stay accessible) instead of forcing aria-hidden - result grid sets aria-sort on sortable headers Worker-side read-only transaction enforcement is out of scope here (it is a database worker change); this tightens the client gate as defense in depth. SqlPanel already remounts per database via its key, so no separate db-change reset is needed. * fix(database): page edge cases from review - Subtitle: a database configured without a `url` no longer reads "no database configured"; it falls back to the database name, and the worker-unavailable / nothing-configured copy stays for the absent cases. - SQL panel is keyed by database only, so a refresh reloads metadata instead of remounting the editor and discarding an in-progress statement and its results. - "open in sql" seeds through quoteTableRef, so a schema-qualified postgres table quotes component-wise instead of becoming one identifier. - Selectable grid rows are tabbable and activate on Enter/Space, matching the click path; non-clickable rows are unchanged. - SQL history state is bounded by HISTORY_LIMIT, matching what is persisted. - Re-expanding a table whose schema read failed retries instead of staying on the error row forever. - Copy feedback moves into a shared useCopyFeedback hook that cancels its pending timer on unmount, replacing the duplicate implementations in the grid and the row inspector.
Ships the database worker's
#/ext/databasepage over the injectable-UI protocol (#570), the page that #580'sdatabase/uideliberately left as a stub ("No page"):Reads the live database via
database::query/database::listDatabases. Registered throughhost.pages.registerin the existingdatabase/uisetup, next to the function-trigger renderer. No console-source changes.Stacked on
feat/codeeditor-completions: the SQL editor passes table names + keywords to that PR'sCodeEditorcompletionsprop for as-you-type suggestions. Merge the console PR first.Test
tsc + biome + esbuild clean. Live-verified end to end: the page injects and renders against the rig's sqlite (3 tables), the schema tree expands columns/PK/FK, the row grid sorts and inspects, and the SQL panel runs read-only queries with EXPLAIN, history, and table-name autocomplete.
Summary by CodeRabbit