(MOT-4283) feat(database): read-side function surface and console renderer - #645
(MOT-4283) feat(database): read-side function surface and console renderer#645rohitg00 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 25 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (50)
📝 WalkthroughWalkthroughThe database worker adds cross-driver schema inspection, table browsing, query analysis, statistics, health reporting, persistence, and row-change APIs. The database UI consumes these APIs through typed RPC wrappers and adds interactive browsing, diagrams, diagnostics, filtering, navigation, and saved table layouts. ChangesDatabase worker APIs
Database UI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 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, 50 skipped (no docs/).
Four for four. Nicely done. |
0a0fca8 to
30b5a0c
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 20
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (14)
database/src/handlers/filter.rs-274-282 (1)
274-282: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
case_sensitiveis silently ignored for non-LIKE operators.The guard requires
f.op.is_like(), and only postgres LIKE operators consumesensitiveat Lines 324-327. On any driver, a filter such as{"op":"equals","case_sensitive":true}is accepted and the flag has no effect. The module documentation at Lines 13-15 states the flag is rejected rather than ignored.Reject the flag whenever the operator cannot honour it.
🐛 Proposed fix
- if f.case_sensitive.is_some() && driver != DriverKind::Postgres && f.op.is_like() { - return Err(invalid( - "case_sensitive is only supported on postgres; elsewhere it is \ - determined by the column collation", - )); - } + if f.case_sensitive.is_some() && !(driver == DriverKind::Postgres && f.op.is_like()) { + return Err(invalid( + "case_sensitive applies only to postgres pattern operators; \ + elsewhere it is determined by the column collation", + )); + }🤖 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/src/handlers/filter.rs` around lines 274 - 282, Update the validation in the filter-handling flow around `f.case_sensitive` so the flag is rejected whenever `f.op` is not a supported LIKE operator, regardless of driver; preserve the existing PostgreSQL-only restriction for case-sensitive LIKE handling and leave the `sensitive` assignment unchanged for valid cases.database/src/handlers/health.rs-111-118 (1)
111-118: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
probelabels every failure asDenied.A timeout, a dropped connection, or a syntax error all become
status: "denied", which tells the reader that the role lacks permission. The UI branches onstatus, not on the reason text. Add anErrorvariant, or classify the failure before choosing the variant.🤖 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/src/handlers/health.rs` around lines 111 - 118, Update the probe function to distinguish permission failures from other probe failures instead of mapping every Err to ProbeResult::Denied. Add or reuse a ProbeResult::Error variant and classify timeout, connection, syntax, and other non-permission failures as Error, while preserving Denied only for actual authorization failures and keeping successful results Available.database/src/handlers/tx_sql_guard.rs-70-111 (1)
70-111: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winA keyword scan cannot see writes performed by functions, so soften the "authority" claim.
The scan catches DML keywords. It does not catch a read-shaped statement whose functions write. All of these pass
is_read_only_sqland execute their side effects underEXPLAIN ANALYZE:
SELECT setval('s', 1)SELECT nextval('s')SELECT pg_terminate_backend(123)SELECT * FROM some_volatile_plpgsql_function()The doc comment states that this check is the authority. Record this class as a known gap so a later reader does not treat the guard as complete. If PostgreSQL is the concern, running
EXPLAIN ANALYZEinside a read-only transaction closes it properly, because the server then rejects the write itself.Separately, the scan reads string literals as statement text.
SELECT * FROM t WHERE status = 'delete'andSELECT ';'are both refused. That fails closed, so it is safe, but a valid read is rejected. Note it in the doc comment so the console can explain the refusal.🤖 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/src/handlers/tx_sql_guard.rs` around lines 70 - 111, Update the documentation above is_read_only_sql to soften the authority claim and record that function side effects can bypass the keyword scan; note that a PostgreSQL read-only transaction is the stronger enforcement mechanism. Also document that keywords and semicolons inside string literals are scanned as SQL text, causing some valid reads to be conservatively rejected, while leaving the implementation unchanged.database/src/handlers/table_view.rs-33-35 (1)
33-35: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winThe view key omits the schema, so same-named tables in different schemas share one layout.
GetTableViewReqandSaveTableViewReqcarry onlytable.BrowseTableReqandDescribeTableReqboth acceptschema. On PostgreSQL and MySQL,public.ordersandarchive.orderstherefore write to the sameview:{db}:orderskey and overwrite each other's widths and column order.Add an optional
schemafield and include it inview_key. Keep the old key shape whenschemais absent so existing stored views still resolve.Also applies to: 52-66
🤖 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/src/handlers/table_view.rs` around lines 33 - 35, Update view_key and the GetTableViewReq/SaveTableViewReq flow to accept an optional schema and include it in generated keys, ensuring schema-qualified tables have distinct layouts. Preserve the existing view:{db}:{table} key format when schema is absent so previously stored views continue resolving.database/src/handlers/column_stats.rs-293-300 (1)
293-300: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA negative
n_distinctwith an unknown row count reports 0 distinct values.
pg_stats.n_distinctis negative when it is a ratio of the row count. Ifrow_countisNone,unwrap_or(0)makes the product0, anddistinct_countbecomesSome(0). That states the column has no distinct values. ReturnNonewhen the row count is unknown.🐛 Proposed fix
- stat.distinct_count = f64_at(r, "n_distinct").map(|n| { - if n < 0.0 { - // Negative means "this fraction of the row count". - (-n * row_count.unwrap_or(0) as f64).round() as i64 - } else { - n as i64 - } - }); + stat.distinct_count = f64_at(r, "n_distinct").and_then(|n| { + if n < 0.0 { + // Negative means "this fraction of the row count", so + // it is unanswerable without the row count. + row_count.map(|total| (-n * total as f64).round() as i64) + } else { + Some(n as i64) + } + });🤖 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/src/handlers/column_stats.rs` around lines 293 - 300, Update the negative-value branch in the distinct_count mapping to return None when row_count is unavailable, rather than calculating from a zero fallback. Preserve the existing ratio calculation and rounding when row_count is Some, and keep non-negative n_distinct handling unchanged.database/src/handlers/explain.rs-379-380 (1)
379-380: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
rows_actualfor MySQL holds an estimate.
rows_produced_per_joinis a planner estimate, not a measured count.walkcomparesrows_estimatedwithrows_actualand emitsEstimateSkewwith the message "statistics are likely stale". On MySQL both values come from the same estimate set, so the warning describes measured skew that was never measured. Leaverows_actualasNonefor the estimate-only path.🤖 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/src/handlers/explain.rs` around lines 379 - 380, Update the MySQL row-mapping logic around rows_estimated and rows_actual so rows_actual remains None when sourced from the estimate-only rows_produced_per_join field. Preserve rows_estimated from rows_examined_per_scan and ensure walk cannot compare two planner estimates or emit measured EstimateSkew for this path.database/src/handlers/explain.rs-188-196 (1)
188-196: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
analyzedis reported as true for MySQL without running ANALYZE.Line 189 only excludes SQLite. For MySQL the statement built at line 194 is always
EXPLAIN FORMAT=JSON, which never executes the statement and never returns timings. The response then claimsanalyzed: truewhile every actual-row and time field is absent. Restrictanalyzedto the driver that actually analyzes, or emitEXPLAIN ANALYZE FORMAT=JSONfor MySQL 8.0.18 and later.🔧 Minimal fix
- // SQLite's EXPLAIN QUERY PLAN has no ANALYZE form. - let analyzed = req.analyze && driver != DriverKind::Sqlite; + // Only postgres collects real timings here: SQLite's EXPLAIN QUERY PLAN has + // no ANALYZE form, and the mysql statement below is estimate-only. + let analyzed = req.analyze && driver == DriverKind::Postgres;🤖 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/src/handlers/explain.rs` around lines 188 - 196, Update the analyzed flag calculation near analyzed so it is true only for drivers whose generated EXPLAIN statement actually performs analysis, currently PostgreSQL. Keep MySQL’s existing EXPLAIN FORMAT=JSON generation unchanged and ensure the response cannot report analyzed: true for MySQL.database/ui/src/page/SqlPanel.tsx-244-247 (1)
244-247: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winThe placeholder hint names a SQLite-only table.
This change removed the
driverprop, but the idle message still suggestsselect name from sqlite_master limit 10. A user on Postgres or MySQL who copies that hint gets an error. Use a driver-neutral example.✏️ Proposed change
- {running ? 'running…' : 'results appear here. try: select name from sqlite_master limit 10'} + {running ? 'running…' : 'results appear here. pick a table in the tree, or type a select statement.'}🤖 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/SqlPanel.tsx` around lines 244 - 247, Update the idle placeholder text in the SqlPanel render branch to use a driver-neutral SQL example instead of referencing the SQLite-only sqlite_master table. Keep the running state message unchanged and ensure the replacement query remains valid across supported database drivers.database/ui/src/page/ErdPanel.tsx-384-397 (1)
384-397: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard an empty
pointsarray.If the worker emits an edge with no routed points,
aandbareundefinedanda.xthrows, which takes down the whole panel.shiftEdgealready tolerates the empty case. Skip such an edge instead.🛡️ Proposed change
const points = shiftEdge(e.points, from, to) const a = points[0] const b = points[points.length - 1] + if (!a || !b) return null🤖 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/ErdPanel.tsx` around lines 384 - 397, Update the edge-rendering flow around shiftEdge in ErdPanel so edges with an empty points array are skipped before accessing points[0] or the final point. Preserve existing rendering for non-empty routes and avoid evaluating endpoint coordinates when no routed points exist.database/ui/src/page/ChangesPanel.tsx-142-144 (1)
142-144: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard against invalid
change.atvalues in the timestamp tooltip.
ChangeRowrendersnew Date(change.at).toISOString()directly, andDate.prototype.toISOString()throwsRangeError: Invalid time valuefor unparseable input. Use a small fallback so an unexpected worker payload keeps the rows visible instead of crashing the panel.🤖 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/ChangesPanel.tsx` around lines 142 - 144, Update the timestamp tooltip in ChangeRow to guard new Date(change.at).toISOString() against invalid dates, using a small safe fallback when parsing fails. Preserve the existing relative(age) display and ensure unexpected worker payloads do not crash the panel.database/ui/src/page/result-grid.tsx-344-354 (1)
344-354: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the keyboard tables use explicit grid semantics.
When
keyboardis supplied,<table className="db-grid">is still implicitrole="table"and native<td>maps torole="cell", notrole="gridcell". Thegridcellrole should only be used inside an explicit grid structure or removed.🤖 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 344 - 354, Update the keyboard-enabled table semantics in the result-grid rendering so the table and its rows/cells form an explicit grid structure, including the required grid and row roles, or remove the explicit gridcell role and rely on native table semantics. Ensure `role="gridcell"` in the `<td>` rendered by the keyboard path is only retained when its parent structure is explicitly declared as a grid.database/ui/styles.css-1051-1066 (1)
1051-1066: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winHover-only reveal hides controls from keyboard users. Both controls start at
opacity: 0and only a:hoverrule on the ancestor raises the opacity. Tab focus does not reveal them, so a keyboard user cannot see the control that holds focus. Add a:focus-visiblerule beside each hover rule.
database/ui/styles.css#L1051-L1066: add[data-iii-ui="database"] .db-col-hide:focus-visible { opacity: 1; }next to theth:hoverrule.database/ui/styles.css#L1153-L1164: add[data-iii-ui="database"] .db-json-copy:focus-visible { opacity: 1; }next to the.db-json-row:hoverrule.🤖 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/styles.css` around lines 1051 - 1066, Update database/ui/styles.css at lines 1051-1066 by adding a focus-visible opacity rule for .db-col-hide beside its th:hover rule, and at lines 1153-1164 by adding the corresponding focus-visible opacity rule for .db-json-copy beside the .db-json-row:hover rule, so keyboard-focused controls become visible.database/ui/build.mjs-69-105 (1)
69-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReset
bufat;so statement at-rules do not hide the next selector.The scanner only clears
bufat{and}. A statement at-rule that has no block, for example@importor@charset, leaves its text inbuf. The next{then produces aheadthat starts with@, so the scanner treats the following rule as an at-rule: it skips the scope check for that selector and pushes a bogus entry ontoatStack. The same effect applies to declarations that precede a nested rule.Clear
bufwhen you read;.🐛 Proposed fix
} else if (ch === '}') { depth-- buf = '' if (atStack.length && depth < atStack.length) atStack.pop() + } else if (ch === ';') { + // A statement at-rule (`@import`, `@charset`, `@layer a;`) or a + // declaration ends here and must not leak into the next selector. + buf = '' } else { buf += ch }🤖 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/build.mjs` around lines 69 - 105, Update the scanner loop in the build parser to clear buf when encountering a semicolon, before processing subsequent text. This ensures statement at-rules and preceding declarations do not contaminate the next selector’s head or atStack handling, while preserving the existing brace-based parsing behavior.database/ui/src/page/CellInspector.tsx-54-59 (1)
54-59: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSet the copied state only after the clipboard write succeeds.
Line 55 discards the
writeTextpromise, and an unavailable Clipboard API or rejected permission can still setcopiedat line 56. AwaitwriteTextin atryblock and callsetCopied(what)only after it resolves.🤖 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/CellInspector.tsx` around lines 54 - 59, Update the copy function so the navigator.clipboard writeText promise is awaited inside a try block, and call setCopied(what) only after the write resolves successfully. Preserve the existing timeout reset behavior, while leaving copied unchanged when the Clipboard API is unavailable or the write rejects.
🧹 Nitpick comments (20)
database/src/handlers/browse.rs (1)
75-81: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo copies of
driver_ofduplicate the existingPool::driver(). Both handlers re-implement thePoolvariant toDriverKindmatch thatPool::driver()already performs, as used indatabase/src/handlers/health.rsline 158. Adding a driver would require editing three places.
database/src/handlers/browse.rs#L75-L81: replace the body withOk(state.pool(db).await.map_err(err_to_str)?.driver())and drop the now-unusedcrate::pool::Poolimport.database/src/handlers/column_stats.rs#L113-L119: delete this copy and call the shared helper orPool::driver()directly, then drop thecrate::pool::Poolimport.🤖 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/src/handlers/browse.rs` around lines 75 - 81, Replace the duplicate driver matching in database/src/handlers/browse.rs lines 75-81 within driver_of with Pool::driver(), and remove the unused crate::pool::Pool import. In database/src/handlers/column_stats.rs lines 113-119, delete the duplicate driver_of implementation and use the shared helper or Pool::driver() directly, then remove its unused Pool import.database/tests/integration.rs (2)
766-790: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd exact-mode coverage for
mean.The test profiles the
notetext column and asserts counts, but never assertsmean. Themean_valueaggregate is the least portable part ofexact_stats. Add two assertions:meanisSomefor the numericscorecolumn, andmeanis absent fornote. That pins the behavior the SQL expression relies on and would surface the driver differences discussed ondatabase/src/handlers/column_stats.rslines 371-381.🤖 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/tests/integration.rs` around lines 766 - 790, Add exact-mode mean assertions to the integration coverage around column_stats::handle: request or profile the numeric score column and assert its mean is Some, while asserting the text note column’s mean is None. Preserve the existing note count, null, distinct, and top-value assertions so the test validates both numeric mean computation and its absence for non-numeric data.
831-838: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReplace the single-element loop with a direct match.
for section in [&h.active_queries]iterates one item. The two following lines already useassert!(matches!(...))for the other sections. Use the same form here, or extend the array with&h.table_sizesso the loop earns its shape.🤖 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/tests/integration.rs` around lines 831 - 838, Replace the single-element loop around h.active_queries with a direct assertion matching health::ProbeResult::Unsupported and validating that the reason contains "sqlite", consistent with the adjacent sections' assert!(matches!(...)) style; do not change the expected failure behavior.database/src/handlers/health.rs (1)
161-181: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
timeout_msis a per-probe budget, so one call can take four times as long.The four probes are awaited in sequence. With the default 15000 ms, a database that stalls returns after up to 60 s. Either run the probes concurrently with
tokio::try_join!/join!, or divide the budget across the probes and state the per-probe meaning in theHealthReqdoc comment.Note that concurrent probes need four pool connections at once, so verify the pool minimum before choosing that option.
🤖 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/src/handlers/health.rs` around lines 161 - 181, Update the probe orchestration in the DriverKind match so the timeout_ms budget applies to the overall health check rather than allowing four sequential probe timeouts; prefer concurrent execution with tokio::join!/try_join! only after verifying the pool can provide four connections, otherwise divide the budget across probes and document the per-probe meaning in HealthReq.database/src/handlers/column_stats.rs (1)
222-225: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the column count in exact mode.
columnsis optional, soexact: truewithout a column list profiles every column. Each column costs two full scans. A 40-column table therefore runs 80 scans from one panel click. Cap the number of columns accepted in exact mode, or require an explicit list.🤖 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/src/handlers/column_stats.rs` around lines 222 - 225, Update the exact-mode column selection around the `wanted` loop and `columns` collection to prevent unbounded profiling when no column list is supplied. Enforce a defined maximum number of columns for exact mode or reject requests lacking an explicit list, while preserving the existing `exact_stats` behavior for accepted columns and returning a clear request error when the limit is exceeded.database/ui/src/lib/rpc.ts (1)
58-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude the validation detail in the thrown error.
calldiscardsparsed.error. The file header states that a missing field must surface as a readable error. The current message names only the function, so the operator cannot see which field failed. Add a compact issue summary.♻️ Proposed change
const parsed = schema.safeParse(raw) if (!parsed.success) { - throw new Error(`unexpected response shape from ${fn}`) + const detail = parsed.error.issues + .slice(0, 3) + .map((i) => `${i.path.join('.') || '(root)'}: ${i.message}`) + .join('; ') + throw new Error(`unexpected response shape from ${fn} — ${detail}`) }🤖 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/lib/rpc.ts` around lines 58 - 65, Update the error path in call to include a compact summary derived from parsed.error alongside the function name, so validation failures identify the missing or invalid field while preserving the existing successful return behavior.database/ui/src/page/index.tsx (2)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winType
MODE_REQUIRESwithDbFunctionand drop the casts.
MODE_REQUIRESdeclares its values asstring, which forcescaps.has(needed as never)on Line 105. Theas nevercast hides any future mismatch between a mode requirement and a real function id. Type the record withDbFunctioninstead. Theas PanelMode[]cast on Line 107 then also becomes unnecessary.♻️ Proposed change
-import { DB } from '../lib/rpc' +import { DB, type DbFunction } from '../lib/rpc' @@ -const MODE_REQUIRES: Partial<Record<PanelMode, string>> = { +const MODE_REQUIRES: Partial<Record<PanelMode, DbFunction>> = { diagram: DB.schemaDiagram, health: DB.health, } @@ - return !needed || caps.has(needed as never) + return !needed || caps.has(needed) }) - return [...ALWAYS, ...optional] as PanelMode[] + return [...ALWAYS, ...optional]Also applies to: 102-108
🤖 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/index.tsx` around lines 41 - 44, Change MODE_REQUIRES to use DbFunction values instead of string, then remove the casts in the capability check and mode iteration around caps.has and Object.keys(MODE_REQUIRES). Preserve the existing PanelMode-to-required-function mapping and behavior.
76-83: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDo not call another setter inside a state updater.
goBackTocallssetSelectedTableinside thesetTrailupdater. React treats updaters as pure functions and invokes them twice in Strict Mode, sosetSelectedTableruns twice per click. The value is the same both times, so behavior does not change today, but the pattern breaks as soon as the derived value is not idempotent. Derivenextfromtrailand call both setters at the same level.♻️ Proposed change
- const goBackTo = useCallback((depth: number) => { - setTrail((prev) => { - const next = prev.slice(0, depth) - const last = next[next.length - 1] - setSelectedTable(last ? last.table : null) - return next - }) - }, []) + const goBackTo = useCallback( + (depth: number) => { + const next = trail.slice(0, depth) + setTrail(next) + setSelectedTable(next[next.length - 1]?.table ?? null) + }, + [trail], + )🤖 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/index.tsx` around lines 76 - 83, Update goBackTo to derive the truncated trail from the current trail value outside the setTrail updater, then call setTrail and setSelectedTable at the same level. Remove the nested setter call so the state updater remains pure while preserving selection of the final table in the resulting trail.database/ui/src/lib/capabilities.ts (1)
32-41: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse a sentinel function to confirm the listing was understood.
probefalls back toALLonly whenidsis empty. IfcollectIdsrecognizes some ids but misses others — for example the engine nests entries under a key thatcollectIdsdoes not walk —idsis non-empty and every optional panel is hidden. The module doc states that guessing the shape wrong should not disable the page, so this path contradicts it.
database::queryanddatabase::listDatabasesexist in every worker version that can serve this page. Treat their absence as proof that the listing was not parsed.♻️ Proposed change
+import { DB, OPTIONAL_FNS, type DbFunction } from './rpc' + export async function probe(host: Host): Promise<Capabilities> { try { const raw = await host.iii.trigger<unknown>('engine::functions::list', {}) const ids = collectIds(raw) - if (ids.size === 0) return ALL + // A listing that does not name the functions this page already depends on + // was not understood; do not read it as evidence of missing features. + if (!ids.has(DB.query) && !ids.has(DB.listDatabases)) return ALL return new Set(OPTIONAL_FNS.filter((fn) => ids.has(fn))) } catch { return ALL } }🤖 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/lib/capabilities.ts` around lines 32 - 41, Update probe so it treats the capability listing as unparsed when either required sentinel function, database::query or database::listDatabases, is absent from ids, returning ALL in that case; retain the existing filtered OPTIONAL_FNS result only when both sentinels are present.database/ui/src/page/HealthPanel.tsx (1)
96-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the probe payload types from
HealthReportinstead of redeclaring them.The
as nevercasts on Lines 120, 144, and 160 disable type checking betweenhealthSchemaindatabase/ui/src/lib/rpc.tsand the render callbacks. The local types have already drifted:healthSchemadeclaresblocking_sqlon a lock entry, andHealthLockon Line 210 omits it. A future field change in the worker schema will not fail the build here.Extract the payload types from the inferred
HealthReportso the two stay bound together.♻️ Proposed change
-type HealthQuery = { - id: string - sql: string - state?: string | null - duration_ms?: number | null - user?: string | null -} -type HealthLock = { blocked_id: string; blocked_sql: string; blocking_id: string; relation?: string | null } -type HealthCache = { hit_ratio: number; blocks_hit: number; blocks_read: number } -type HealthTableSize = { - table: string - schema?: string | null - total_bytes?: number | null - index_bytes?: number | null - row_estimate?: number | null -} +/** Payloads of the `available` branch of each probe, taken from the schema. */ +type Available<K extends keyof HealthReport> = Extract<HealthReport[K], { status: 'available' }>['data'] +type HealthQuery = Available<'active_queries'>[number] +type HealthLock = Available<'locks'>[number] +type HealthCache = Available<'cache'> +type HealthTableSize = Available<'table_sizes'>[number]The
as nevercasts on Lines 120, 144, and 160 can then be removed.Also applies to: 203-218
🤖 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/HealthPanel.tsx` around lines 96 - 160, Derive the health panel payload types from the inferred HealthReport used by healthSchema instead of maintaining separate local HealthQuery, HealthLock, and HealthCache definitions. Update the affected ProbeBody usages, including the table-sizes section, to use those extracted HealthReport property types and remove the as never casts so render callbacks remain type-checked against the RPC schema, including blocking_sql.database/ui/src/page/ErdPanel.tsx (1)
207-219: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftNode dragging re-renders the whole diagram on every pointer move.
moveNodecallssetOffsetsperpointermove. Each update re-renders every node and re-runsshiftEdgefor every edge. The file header states that pan and zoom stay in a ref for exactly this reason, so node drag contradicts that design. With the 200-node diagram this file anticipates, the drag will stutter.Track the live offset in a ref, write
transformonto the dragged node element inside arequestAnimationFrame, and commit tosetOffsetsonce inendNodeDrag.🤖 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/ErdPanel.tsx` around lines 207 - 219, Refactor node dragging around moveNode and endNodeDrag to avoid setOffsets on every pointermove: keep the dragged table’s live offset in a ref, update only the dragged node’s DOM transform within a requestAnimationFrame, and commit the final offset to setOffsets once when the drag ends. Preserve the existing zoom-adjusted delta behavior and ensure pending animation-frame work is handled safely during drag completion.database/ui/src/page/FilterBar.tsx (1)
152-164: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winName the picker triggers for assistive technology.
Each
Pickertrigger exposes only its current value as its accessible name. A screen reader announces "id" and "equals" without stating which segment of the filter each button controls. Add anaria-labelto the trigger.♻️ Proposed refactor: label each segment
function Picker({ label, className, items, onPick, + ariaLabel, }: { label: string className: string items: { value: string; label: string; hint?: string }[] onPick: (value: string) => void + ariaLabel: string }) { return ( <DropdownMenu> <DropdownMenuTrigger asChild> - <button type="button" className={`db-chip-seg ${className}`}> + <button type="button" className={`db-chip-seg ${className}`} aria-label={`${ariaLabel}: ${label}`}> {label} </button><Picker label={filter.column || 'column'} className="col" + ariaLabel="filter column" items={columns.map((c) => ({ value: c.name, label: c.name, hint: c.type.toLowerCase() }))} onPick={setColumn} /> <Picker label={OP_LABEL[filter.op]} className="op" + ariaLabel="filter operator" items={ops.map((o) => ({ value: o, label: OP_LABEL[o], hint: o }))} onPick={(v) => setOp(v as FilterOp)} />🤖 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/FilterBar.tsx` around lines 152 - 164, Add descriptive aria-label props to both Picker components in FilterBar: identify the first trigger as selecting the filter column and the second as selecting the filter operator, while preserving their existing labels, items, and onPick behavior.database/ui/src/page/TableDataPanel.tsx (1)
114-122: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
gridColsrecomputes on every render.
gridRowsis a fresh array wheneverpageRead.datais undefined, anduseTableViewreturns a new controls object on each render. Both are dependencies, so the memo never hits. The same object identities also feeduseGridKeyboard. The computation is small, so this is a nit rather than a defect. Consider memoizing the row fallback and returning stable callbacks fromuseTableView.🤖 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/TableDataPanel.tsx` around lines 114 - 122, Stabilize the dependencies used by gridCols and useGridKeyboard by memoizing the empty-row fallback derived from pageRead.data and ensuring useTableView returns a stable controls object/callbacks across renders. Update the gridCols useMemo dependency flow without changing visible-column ordering or fallback behavior.database/ui/src/page/db-data.ts (2)
84-94: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
splitRefre-introduces the ambiguity documented onColumnInfo.fk.
splitRefassumes the first dot separates schema from table. A table nameda.bon SQLite or MySQL is then sent as schemaa, tableb. The same file documents this exact hazard for joined"table.column"strings.The page identifies a table by the qualified display string, so a full fix means carrying
{ schema, name }through the page state. If that is out of scope, consider splitting only when the driver supports schemas.🤖 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 84 - 94, Update splitRef and the surrounding page state to preserve the full qualified table identity without treating every dot as a schema separator. Carry { schema, name } through the table-selection and display flows, and only split schema-qualified names when the active driver supports schemas; otherwise keep dotted SQLite/MySQL table names in name with schema null.
121-134: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
tableColumnsandtableIndexeseach fetch the whole table description.Both functions call
rpc.describeTableand discard most of the response. A panel that shows columns and indexes performs two identical round trips. Consider exporting a singledescribeTableadapter that returns both projections, and let callers pick.🤖 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 121 - 134, Consolidate the duplicate rpc.describeTable calls used by tableColumns and tableIndexes by exporting a shared describeTable adapter that performs one fetch and exposes both column and index projections. Update both callers to use the adapter and select only their needed projection, preserving the existing tableColumns and tableIndexes return shapes.database/ui/src/lib/grid-cursor.ts (1)
146-149: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueSerialize object values in
rowKeyinstead of usingString.Without a primary key,
rowKeybuilds the identity from every value.String(row[c])turns any object or array into[object Object], so two rows that differ only in a JSON column produce the same key. The reanchor step can then follow the wrong row after a sort.cellTextin this file already handles objects.♻️ Proposed refactor
- return cols.map((c) => `${c}=${String(row[c])}`).join(KEY_SEP) + return cols.map((c) => `${c}=${row[c] === null || row[c] === undefined ? String(row[c]) : cellText(row[c])}`).join(KEY_SEP)Keep
nullandundefineddistinguishable from the empty string, which the existing test on Line 108 ofgrid-cursor.test.tsasserts.🤖 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/lib/grid-cursor.ts` around lines 146 - 149, Update rowKey to serialize object and array cell values using the file’s existing cellText helper instead of String, while preserving distinct representations for null, undefined, and empty strings as required by the existing test. Keep scalar key formatting and primary-key selection unchanged.database/ui/src/page/PlanTree.tsx (2)
110-119: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse a key that stays unique when a node has two warnings of the same kind.
key={w.kind}collides if the worker reports the samekindtwice for one node. React then drops one tooltip. Include the index or the message in the key.🤖 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/PlanTree.tsx` around lines 110 - 119, Update the warning mapping in PlanTree’s mine rendering to give each Tooltip a key unique among warnings, rather than using w.kind alone; include the map index or warning message while preserving the existing tooltip content and behavior.
96-104: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
aria-expandedand hide the leaf placeholder button from assistive technology.The disclosure button is missing
aria-expanded, so a screen reader cannot report the node state. For a leaf node the same element still renders as an empty button labelled "expand" or "collapse", which announces an action that does nothing.Render the placeholder as a non-interactive element for leaves.
♻️ Proposed change
- <button - type="button" - className={`db-plan-twist${hasChildren ? '' : ' leaf'}${isCollapsed ? '' : ' open'}`} - onClick={() => hasChildren && onToggle(node.id)} - aria-label={isCollapsed ? 'expand' : 'collapse'} - tabIndex={hasChildren ? 0 : -1} - > - {hasChildren ? <ChevronRight size={11} aria-hidden /> : null} - </button> + {hasChildren ? ( + <button + type="button" + className={`db-plan-twist${isCollapsed ? '' : ' open'}`} + onClick={() => onToggle(node.id)} + aria-label={isCollapsed ? `expand ${node.label}` : `collapse ${node.label}`} + aria-expanded={!isCollapsed} + > + <ChevronRight size={11} aria-hidden /> + </button> + ) : ( + <span className="db-plan-twist leaf" aria-hidden /> + )}
JsonTree.tsxalready uses this shape for its leaf placeholder, so this also aligns the two trees.🤖 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/PlanTree.tsx` around lines 96 - 104, Update the tree toggle element in the PlanTree node rendering to include aria-expanded reflecting the node’s isCollapsed state for expandable nodes, and render leaf placeholders as non-interactive and hidden from assistive technology instead of empty buttons with misleading labels. Align the behavior with the existing JsonTree.tsx implementation while preserving the current toggle interaction for nodes with children.database/ui/src/lib/grid-cursor.test.ts (1)
41-52: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for
pageUpand thetoaction.The suite tests
pageDownin both the mid-page and boundary cases. It does not testpageUpmid-page, and it does not test thetoaction, which clamps caller-supplied coordinates from a mouse click. Both are cheap to add next to the existing cases.🤖 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/lib/grid-cursor.test.ts` around lines 41 - 52, Add tests alongside the existing pageDown cases in the grid cursor suite for pageUp from the middle of a page, asserting it moves within the page without a page delta, and for the to action, asserting caller-supplied mouse coordinates are clamped to the grid bounds. Use moveCursor and the existing bounds helper and preserve the established boundary behavior.database/ui/src/page/JsonTree.tsx (1)
102-115: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueUse a separator that cannot appear in a key when building the path.
Line 106 joins path segments with
.. A key that contains a dot produces the same path as a nested branch. For example{"a.b": {}}and{"a": {"b": {}}}both produce$.a.b, so expanding one branch expands the other. Use a control character, aslib/grid-cursor.tsdoes withKEY_SEP.♻️ Proposed change
- path={`${path}.${k}`} + path={`${path}\u0001${k}`}🤖 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/JsonTree.tsx` around lines 102 - 115, Update the path construction in the Node mapping within JsonTree to use the shared control-character separator pattern (such as KEY_SEP) instead of ".". Ensure all path creation and matching in the tree uses the same separator so keys containing dots remain distinct from nested branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 10ffb0e1-307b-4d59-9fee-a217007efcc1
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (50)
database/README.mddatabase/skills/SKILL.mddatabase/src/handlers/browse.rsdatabase/src/handlers/catalog/mod.rsdatabase/src/handlers/catalog/mysql.rsdatabase/src/handlers/catalog/postgres.rsdatabase/src/handlers/catalog/sqlite.rsdatabase/src/handlers/column_stats.rsdatabase/src/handlers/diagram.rsdatabase/src/handlers/explain.rsdatabase/src/handlers/filter.rsdatabase/src/handlers/health.rsdatabase/src/handlers/mod.rsdatabase/src/handlers/query.rsdatabase/src/handlers/saved.rsdatabase/src/handlers/schema.rsdatabase/src/handlers/table_view.rsdatabase/src/handlers/tx_sql_guard.rsdatabase/src/main.rsdatabase/src/pool/mod.rsdatabase/src/pool/mysql.rsdatabase/src/pool/postgres.rsdatabase/src/pool/sqlite.rsdatabase/src/triggers/bus.rsdatabase/tests/integration.rsdatabase/ui/build.mjsdatabase/ui/package.jsondatabase/ui/src/lib/capabilities.tsdatabase/ui/src/lib/grid-cursor.test.tsdatabase/ui/src/lib/grid-cursor.tsdatabase/ui/src/lib/rpc.tsdatabase/ui/src/page/CellInspector.tsxdatabase/ui/src/page/ChangesPanel.tsxdatabase/ui/src/page/ColumnStatsPanel.tsxdatabase/ui/src/page/ErdPanel.tsxdatabase/ui/src/page/FilterBar.tsxdatabase/ui/src/page/HealthPanel.tsxdatabase/ui/src/page/JsonTree.tsxdatabase/ui/src/page/PlanTree.tsxdatabase/ui/src/page/RowDetail.tsxdatabase/ui/src/page/SqlPanel.tsxdatabase/ui/src/page/TableDataPanel.tsxdatabase/ui/src/page/db-data.tsdatabase/ui/src/page/icons.tsxdatabase/ui/src/page/index.tsxdatabase/ui/src/page/result-grid.tsxdatabase/ui/src/page/useGridKeyboard.tsdatabase/ui/src/page/useRowChanges.tsdatabase/ui/src/page/useTableView.tsdatabase/ui/styles.css
| // Refuse an exact profile of a very large table rather than starting a | ||
| // scan the caller cannot cancel — on sqlite the driver drops `timeout_ms` | ||
| // entirely, so a row-count guard is the only brake available. | ||
| if let Some(est) = described.row_count_estimate { | ||
| if est > EXACT_ROW_CEILING { | ||
| return Err(err_to_str(DbError::InvalidParam { | ||
| index: 0, | ||
| reason: format!( | ||
| "`{}` has roughly {est} rows; an exact profile would scan all of \ | ||
| them. Re-run without `exact` for planner statistics, or profile \ | ||
| a filtered subset.", | ||
| described.table | ||
| ), | ||
| })); | ||
| } | ||
| } | ||
|
|
||
| let mut columns = Vec::new(); | ||
| for name in &wanted { | ||
| columns.push(exact_stats(state, &db, driver, &target, name, top_n, req.timeout_ms).await?); | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
The row-count guard never fires on SQLite, the driver it was written for.
The comment states that the guard is the only brake available because SQLite drops timeout_ms. But describe_table returns row_count_estimate: None for SQLite — database/tests/integration.rs line 351 asserts this. So if let Some(est) is always false there, and exact: true runs two unbounded full scans per column with no timeout and no ceiling.
Add a fallback for the missing-estimate case. One option is a bounded SELECT COUNT(*) probe before the profile, or a LIMIT-based existence check against the ceiling.
🛡️ Sketch of a fallback
- if let Some(est) = described.row_count_estimate {
- if est > EXACT_ROW_CEILING {
+ // When the driver has no estimate (sqlite), count with a bounded probe
+ // rather than proceeding with no brake at all.
+ let est = match described.row_count_estimate {
+ Some(est) => Some(est),
+ None => run(
+ state,
+ &db,
+ format!(
+ "SELECT COUNT(*) AS n FROM (SELECT 1 FROM {target} LIMIT {})",
+ EXACT_ROW_CEILING + 1
+ ),
+ vec![],
+ req.timeout_ms,
+ )
+ .await?
+ .first()
+ .and_then(|r| i64_at(r, "n")),
+ };
+ if let Some(est) = est {
+ if est > EXACT_ROW_CEILING {🤖 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/src/handlers/column_stats.rs` around lines 205 - 225, Update the
exact-profile guard before the exact_stats loop so SQLite and other cases with
row_count_estimate set to None still enforce EXACT_ROW_CEILING. When no estimate
is available, perform a bounded row-count or LIMIT-based probe against the
target table, reject the request with the existing InvalidParam error when the
ceiling is exceeded, and proceed to exact_stats only when the table is within
the limit.
| format!( | ||
| "SELECT COUNT(*) AS row_count, COUNT({col}) AS non_null, \ | ||
| COUNT(DISTINCT {col}) AS distinct_count, \ | ||
| MIN({col}) AS min_value, MAX({col}) AS max_value, \ | ||
| AVG(CASE WHEN {col} + 0 = {col} THEN {col} END) AS mean_value \ | ||
| FROM {target}" | ||
| ), | ||
| vec![], | ||
| timeout_ms, | ||
| ) | ||
| .await?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
PostgreSQL text + integer operator does not exist AVG on date column error
💡 Result:
The error "operator does not exist" in PostgreSQL typically indicates a type mismatch where PostgreSQL cannot implicitly convert or operate between two data types [1][2]. Because PostgreSQL is a strongly-typed database, it requires explicit handling for such operations [1][2]. The specific errors you are encountering can be addressed as follows: 1. "operator does not exist: text + integer" This occurs because you are attempting to perform arithmetic on a text field (or a string literal) and an integer directly [2]. To fix this, you must explicitly cast the text value to a numeric type before performing the operation [3][2]: SELECT (my_text_column::int + my_integer_column) FROM my_table; 2. "function avg(date) does not exist" (or timestamp) PostgreSQL does not provide a native AVG aggregate function for date or timestamp columns because calculating the "average" of chronological points is not a standard mathematical operation [4][5][6]. To calculate an average date or timestamp, you must convert the dates into a numeric representation, average that number, and then convert the result back into a date or timestamp [7][5]. The most common approach is to use the Unix epoch (seconds since 1970-01-01) [7][5]: SELECT to_timestamp(AVG(EXTRACT(epoch FROM my_date_column)))::date AS average_date FROM my_table; If you are seeing "operator does not exist: date = integer" in a WHERE clause, it is often because a date is being compared to an unquoted value that PostgreSQL interprets as a mathematical expression (e.g., 2026-07-31 being treated as 2026 minus 7 minus 31) [8][9]. Always ensure dates are enclosed in single quotes (e.g., '2026-07-31') or use parameterized queries to avoid these parsing errors [8][9].
Citations:
- 1: https://blog.elevarq.com/does-not-exist-postgresql-errors
- 2: https://runebook.dev/en/docs/postgresql/functions-math/43
- 3: https://stackoverflow.com/questions/51063943/operator-does-not-exist-text-integer-postgres-9-5
- 4: https://postgrespro.com/list/id/3AA82201.C22A5BAB@actarg.com
- 5: https://stackoverflow.com/questions/72440151/using-djangos-orm-to-average-over-timestamp-without-time-zone-postgres-field
- 6: https://www.postgresql.org/message-id/20040308074400.GA20537%40wolff.to
- 7: https://stackoverflow.com/questions/32731570/calculate-average-date-in-postgresql-db
- 8: https://stackoverflow.com/questions/16073084/jdbc-error-operator-does-not-exist-date-integer
- 9: https://stackoverflow.com/questions/73433647/operator-does-not-exist-date-integer-when-filtering-for-dates-in-postgresql
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate column_stats.rs =="
fd -a 'column_stats\.rs$' . || true
echo "== outline =="
file="$(fd 'column_stats\.rs$' . | head -n1)"
if [ -n "$file" ]; then
wc -l "$file"
ast-grep outline "$file" --match exact_stats --view expanded || true
echo "== relevant lines 330-410 =="
sed -n '330,410p' "$file" | nl -ba -v330
fi
echo "== describe_table references =="
rg -n "describe_table|exact_stats|mean_value|exact" -S . | head -n 200Repository: iii-hq/workers
Length of output: 433
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./database/src/handlers/column_stats.rs"
echo "== exact_stats and surrounding lines =="
sed -n '330,410p' "$file" | cat -n
echo "== column_stats.rs size =="
wc -l "$file"
echo "== relevant symbols/usages =="
rg -n "describe_table|exact_stats|mean_value|exact" database/src -S | head -n 250
echo "== column stats test files =="
fd -a 'column_stats|profile|stats' . | grep -E '(test|spec|integration|integration_tests|tests)' -i || trueRepository: iii-hq/workers
Length of output: 7702
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="./database/src/handlers/column_stats.rs"
echo "== column_stats.rs header and exact profile driver usage =="
sed -n '1,240p' "$file" | cat -n
echo "== describe_table location and implementation =="
sed -n '1,180p' database/src/handlers/schema.rs | cat -nRepository: iii-hq/workers
Length of output: 17588
Compute mean_value in a portable way before profiling a non-numeric column.
AVG(CASE WHEN {col} + 0 = {col} THEN {col} END) depends on implicit arithmetic coercion. PostgreSQL rejects text + integer during type checking, so exact profiling a text column fails before returning the rest of the stats. Date/timestamp values also do not have a portable numeric mean aggregate. Use the column catalog type from describe_table and compute mean only for numeric-compatible types; omit it otherwise.
🤖 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/src/handlers/column_stats.rs` around lines 371 - 381, Update the
profiling query construction in the surrounding column-statistics function to
use the type metadata returned by describe_table. Include AVG only for
numeric-compatible column types, and emit a null/omitted mean expression for
text, date, and timestamp types so query type checking succeeds while the other
statistics remain unchanged.
| const pageFetcher = useCallback( | ||
| () => fetchTablePage(host, db, driver, table, page, pageSize, sort), | ||
| [host, db, driver, table, page, pageSize, sort], | ||
| ) | ||
| const countFetcher = useCallback( | ||
| () => countTableRows(host, db, driver, table), | ||
| [host, db, driver, table], | ||
| ) | ||
| const columnsFetcher = useCallback( | ||
| () => tableColumns(host, db, driver, table), | ||
| [host, db, driver, table], | ||
| () => | ||
| browseTable(host, db, table, null, { | ||
| page, | ||
| pageSize, | ||
| sort: sort ? [{ column: sort.column, direction: sort.dir }] : [], | ||
| filters: JSON.parse(appliedKey) as FilterSpec[], | ||
| }), | ||
| [host, db, table, page, pageSize, sort, appliedKey], | ||
| ) | ||
| const columnsFetcher = useCallback(() => tableColumns(host, db, driver, table), [host, db, driver, table]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy lift
browseTable receives a schema-qualified name as a bare table name.
table is the page identifier produced by db-data.listTables, which returns qualify(t.schema, t.name). On postgres it is "public.users". Line 90 passes that whole string as the table argument and null as schema. The worker then looks up a table literally named public.users.
Line 98 shows the intended handling: tableColumns splits the reference before the RPC call. Route the browse read through the same split so both requests address the same table.
🐛 Proposed fix: reuse the splitting adapter
Export the split helper from db-data.ts:
-/** Split a schema-qualified name for the worker, which takes them apart. */
-function splitRef(table: string): { schema: string | null; name: string } {
+/** Split a schema-qualified name for the worker, which takes them apart. */
+export function splitRef(table: string): { schema: string | null; name: string } {Then use it here:
-import { type DbDriver, type TableSort, tableColumns } from './db-data'
+import { type DbDriver, splitRef, type TableSort, tableColumns } from './db-data' const pageFetcher = useCallback(
- () =>
- browseTable(host, db, table, null, {
+ () => {
+ const { schema, name } = splitRef(table)
+ return browseTable(host, db, name, schema, {
page,
pageSize,
sort: sort ? [{ column: sort.column, direction: sort.dir }] : [],
filters: JSON.parse(appliedKey) as FilterSpec[],
- }),
+ })
+ },
[host, db, table, page, pageSize, sort, appliedKey],
)Run the following script to confirm how other callers address the same table:
#!/bin/bash
# Description: Compare table/schema arguments passed to browseTable across the UI.
set -euo pipefail
fd -e ts -e tsx . database/ui/src | xargs rg -n -C 4 'browseTable\('
rg -n -C 3 'splitRef|qualify\(' database/ui/src🤖 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/TableDataPanel.tsx` around lines 88 - 98, Update the
browseTable call in pageFetcher to split the schema-qualified table reference
before passing arguments, reusing the existing splitRef helper from db-data.ts.
Export that helper if needed, and pass its schema and table components to
browseTable instead of the full table string with a null schema; keep
tableColumns consistent with the same split behavior.
…derer The database worker learns to describe itself, and the console page becomes a renderer on top of that rather than a second implementation. Twelve documented functions become twenty-nine. Introspection (listTables, describeTable, describeSchema) returns structured foreign keys rather than a lossy "table.column" string, which is what makes relational navigation and the diagram possible. browseTable takes typed filters and sort specs, so a caller gets paged, filtered, sorted reads without writing dialect SQL. explain parses three plan formats into one tree and computes its own warnings. columnStats reads planner statistics by default and only runs real aggregates when asked. health reports each probe as available, unsupported or denied, so "sqlite has no pg_stat_activity" cannot be mistaken for "nothing is running". schemaDiagram does the layout — component split, ranking, crossing reduction, routed elbows — and returns positioned nodes and the connected groups. Saved queries and history live on the state builtin instead of one browser's localStorage. Filters cover set membership and can be switched off. Stacking is AND, so "is one of open, held" is not expressible as two equalities and needs `in` outright; `not_in` keeps NULL rows, which the plain form silently drops. A disabled filter is skipped rather than validated, because a half-built condition parked mid-edit should not fail the request around it. The page drops roughly 520 lines of per-driver catalog SQL and calls those functions instead. Alongside the existing data and sql modes it gains a schema diagram, a connection health view, and a view of the writes landing in the selected table, which the row-changed trigger has emitted for some time with nothing rendering it. Explain draws a plan tree with a per-node cost bar rather than a grid of text. The grid becomes navigable. Cells were individually focusable buttons, which on a fifty-row page put a thousand tab stops between the filter bar and the pager; they are now one roving-tabindex grid where Tab leaves and the arrows move within. The cursor crosses page boundaries, and survives a sort by re-finding its row key rather than its index. Movement is a pure function with its own tests. Following a foreign key filters the referenced table and leaves a breadcrumb, and the cell inspector reads long values, counts code points rather than UTF-16 units, and renders JSON as a tree instead of a flat line. The grid is customisable, and the layout is not private to one browser. Columns resize by dragging their edge, reorder by dragging their header, and hide from the header itself; widths, order and hidden columns persist through getTableView / saveTableView on the state builtin. Keeping them there rather than in localStorage is the same argument as for history: a layout survives a restart, follows the operator to another machine, and an agent can set a table up for a person to read. Widths are clamped server-side so a stored value cannot produce a layout that has to be undone by hand, and a view is never validated against the live schema — after a rename a column loses its width rather than the whole layout failing to load. Two things the change feed refuses to do, both because the honest answer is narrower than it looks: it never says "live", since a connection using statement capture sees only this worker's writes; and it will not follow a view, since changes are keyed on the table a statement writes to and such a binding would sit at "following" forever while rows changed underneath it. Panels whose function the worker lacks are hidden rather than rendered as controls that always error, discovered once per mount through engine::functions::list.
30b5a0c to
a0c16b2
Compare
2 participants
Fixes MOT-4283
The database worker learns to describe itself. Ask it what tables exist and how they relate, read a table with filters and sorting without writing SQL, get a query's plan back as a readable tree, profile any column, and check connection health. All of it is available to agents and to the console alike.
The database page becomes a full browser on top of that surface. Browse the schema, filter and sort rows, click a foreign key to jump to the row it points at, explore how tables relate one hop at a time in a diagram you can drag around, read long values and JSON in an inspector. Column widths, hidden columns and column order are remembered per table.
Twelve documented functions become twenty-nine. The page drops roughly 520 lines of per-driver catalog SQL and calls those functions instead, so there is one implementation of every read rather than two that drift.
Scope boundary
This is reads and understanding. #640 was writes and connection configuration, and this branch is rebased on top of it. No
triggers/,config.rsorconfiguration.rschanges.What the worker gained
Introspection —
listTables,describeTable,describeSchema. Foreign keys come back structured (schema, table, column) rather than as a joined"table.column"string, which cannot express a schema qualifier unambiguously and breaks outright on a table whose name contains a dot. That fix is what makes relational navigation and the diagram possible at all.Reading —
browseTabletakes typed filters and sort specs and returns the page plus a total honouring the same filters. Compilation lives inhandlers/filter.rs, where the traps are: LIKE metacharacters escaped with an explicitESCAPE,IS NULLand= ''kept as separate operators, and case-insensitivity rejected outside postgres rather than silently ignored.in/not_inexist because filters stack with AND, so set membership is not expressible as OR'd equalities;not_inkeeps NULL rows, which the plain form silently drops.Analysis —
explainparses postgres JSON, mysql JSON and the flat sqlite plan into one tree and computes its own warnings.analyzedefaults to false and is refused server-side for anything that is not a single read, through a newtx_sql_guard::is_read_only_sqlthat rejects mutating CTEs and multi-statement input.columnStatsreads planner statistics by default and only scans when asked.healthreports each probe as available, unsupported or denied, so "sqlite has nopg_stat_activity" cannot be mistaken for "nothing is running".Schema shape —
schemaDiagramdoes component split, ranking, crossing reduction and edge routing, and returns positioned nodes plus the connected groups.focuswalks the foreign-key graph breadth-first from one table; the walk is undirected because following only the declared direction hides every child table. The response names thefrontierone hop beyond what was drawn, so expanding is an informed click rather than a guess.Durable state on the
statebuiltin — saved queries, query history and per-table layout. None are new stores. History used to live in browser localStorage, so it existed for one person, in one browser, until a cache clear. Recording is fire-and-forget and never awaited on the query path.What the console gained
Diagram, health and change-feed panels alongside the existing data and sql modes. Explain draws a plan tree with a per-node cost bar rather than a grid of text.
The grid was made of individually focusable cells, which on a fifty-row page put a thousand tab stops between the filter bar and the pager; it is now one roving-tabindex grid. The cursor crosses server page boundaries and survives a sort by re-finding its row key rather than its index — that movement is a pure function with its own tests. Columns resize, reorder and hide.
Panels whose function the worker lacks are hidden rather than rendered as controls that always error, discovered once per mount through
engine::functions::list.Two deliberate refusals
The change feed never says "live", because a connection using statement capture sees only this worker's writes. And it will not follow a view, because changes are keyed on the table a statement writes to, so such a binding would sit at "following" forever while rows changed underneath it.
Verification
cargo fmt,clippy -D warnings, 342 Rust tests, 19 UI tests (the first in this worker, wired aspnpm testfollowingworktree/ui),tsc, biome, a newbuild.mjsCSS scope assertion, andvalidate_worker.py. Every function exercised live against a running rig on sqlite.Draft because the three-driver e2e matrix has not been run locally yet. Saved queries and history have worker functions and no console panel.
Summary by CodeRabbit