Repository navigation
feat(history): show aggregated query performance insights and regressions (#1413) - #1441
Conversation
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| _ => { | ||
| // Multi-character operators | ||
| let two = if i + 1 < len { | ||
| Some(&sql[i..i + 2]) |
There was a problem hiding this comment.
CRITICAL: Possible panic on non-ASCII SQL (UTF-8 slice off a char boundary)
&sql[i..i + 2] (and &sql[i..i + 3] below) slice a &str by byte offsets. When an ASCII operator byte (=, <, -, *, ...) is directly followed by a multi-byte character, e.g. WHERE a=é or x<日本 (unquoted non-ASCII identifiers are legal in Postgres/MySQL), i + 2/i + 3 lands inside the character and Rust panics with byte index is not a char boundary. This runs on the Tauri command / web RPC path for every history entry, so one such query in history would make the whole Insights view fail. Compare on bytes instead, e.g. bytes.get(i..i + 2) against b"<=" etc., or use sql.get(i..i + 2) which returns None off a boundary. Please add a test with a non-ASCII identifier.
| first.error = error; | ||
| let updated = first.clone(); | ||
| write_query_history(config_dir, connection_id, &entries)?; | ||
| let _ = append_query_history_execution( |
There was a problem hiding this comment.
WARNING: Execution-log write failures are silently discarded
let _ = append_query_history_execution(...) (here and at the non-duplicate path below) drops any I/O or serialization error with no log. Insights then under-count calls with no signal to the user or in logs. The same pattern is used for write_query_history_executions in delete_query_history_entry and backfill_query_history_for_connection, which can leave the executions file out of sync with the visible history (e.g. a deleted entry's SQL stays in the executions log). Please at least log::warn! the error.
| merged.push(entry); | ||
| } | ||
| } | ||
| merged.truncate(max_records); |
There was a problem hiding this comment.
SUGGESTION: merged.truncate(max_records) runs on an unsorted merge
Executions come newest-first, but legacy entries not covered by the execution log are appended afterwards regardless of age, so truncation can drop newer executions in favour of older legacy entries (or vice versa) and total_stored_executions / oldest_recorded_at / retention_cap_reached are computed on that arbitrary subset. Sort the merged list by executed_at before truncating.
| setBackendInsights(response); | ||
| } | ||
| }) | ||
| .catch(() => { |
There was a problem hiding this comment.
WARNING: Backend failure silently falls back to different data
When get_query_history_insights rejects, the error is swallowed and the view silently switches to computeQueryHistoryInsights(entries), which runs over the consecutively de-duplicated list with a hard-coded maxStoredExecutions = 500. Call counts, averages and the retention warning then differ from the backend semantics described in the PR, with no indication to the user. Surface the error (or at least a notice) instead of silently substituting different numbers.
| <button | ||
| type="button" | ||
| key={execEntry.id} | ||
| onClick={() => onDoubleClick(execEntry)} |
There was a problem hiding this comment.
SUGGESTION: Single click loads the query into the editor
onClick is wired to the same handler as onDoubleClick, so a stray click on a contributing execution immediately opens/loads it. The Entries list uses click = select, double-click/Enter = open; keeping that behaviour here avoids accidental tab creation.
Code Review SummaryStatus: No Issues Found | Recommendation: Merge The incremental range since Files Reviewed (25 files in PR diff)
Previous Review Summaries (6 snapshots, latest commit 0a1f229)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 0a1f229)Status: No Issues Found | Recommendation: Merge The incremental range since the last review ( Files Reviewed (29 files in PR diff)
Tests were not executed (read-only review). Previous review (commit 11f5217)Status: No Issues Found | Recommendation: Merge The previous warning (success and error state for the same insights key never reconciled) is resolved. Files Reviewed (2 files in incremental diff)
Tests were not executed (read-only review). Previous review (commit 75f7620)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files in incremental diff)
Tests were not executed (read-only review). Fix these issues in Kilo Cloud Previous review (commit 295b6f7)Status: No Issues Found | Recommendation: Merge Files Reviewed (16 files in incremental diff)
Tests were not executed (read-only review). Previous review (commit b65a352)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (4 files in incremental diff)
Tests were not executed (read-only review). Fix these issues in Kilo Cloud Previous review (commit 0184427)Status: 5 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
SUGGESTION
Files Reviewed (29 files)
Notes: the PR also contains unrelated rustfmt-only reformatting ( Reviewed by claude-sonnet-5.5 · Input: 256 · Output: 1.1K · Cached: 167.7K Review guidance: REVIEW.md from base branch |
b65a352 to
295b6f7
Compare
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for adding a separate, non-deduplicated execution log and for making the retention/sample limits visible in Insights. That avoids treating the existing deduplicated history as a complete execution record. The 34 targeted frontend tests and typecheck pass on this branch.
The Insights request state needs to be tied to its connection and filters before this is merged. QueryHistorySection retains backendInsights while a new request is pending, and insightsData always prefers that retained response over fallbackInsights. If connection A has loaded, then the user switches to B or changes the database/window, A's old aggregates remain visible under the new selection until the next request settles. The cancellation flag prevents an old response arriving late, but does not invalidate the response already stored. Please key the stored response by the request identity, and show only matching data or a loading state. Add a test that loads A, switches to B with its response deliberately unresolved, and checks that A's values are no longer displayed.
The new effect also calls setBackendInsights/setInsightsError synchronously. Please restructure this along with the request state to follow .rules/react.md rule 2; state updates after the async result are fine.
I have not completed a real-app test of the Rust execution-log path. The host Rust build currently stops at missing pkg-config/GLib prerequisites, so the frontend test results above should not be read as backend validation.
|
Thanks for catching this edge case, @debba! Addressed in 75f7620:
|
|
Thanks for catching this edge case, @debba! Addressed in commit 75f7620:
|
| }) | ||
| .then((response) => { | ||
| if (!cancelled) { | ||
| setLoadedInsights({key: requestKey, data: response}); |
There was a problem hiding this comment.
WARNING: Success and error state for the same key are never reconciled
loadedInsights and loadedError are stored independently and only compared against currentInsightsKey. The effect re-runs whenever entries changes (same key), so: (a) a failed fetch followed by a successful refetch leaves loadedError set, so the backend-error banner (and insightsError !== null) stays visible next to valid backend data; (b) a successful fetch followed by a failed refetch keeps the old loadedInsights, so backendInsights stays non-null and stale backend numbers are shown alongside the error instead of the fallback. Clear the opposite state when each request settles, e.g. setLoadedError(null) here and setLoadedInsights(null) in the .catch handler (async callbacks, so this does not violate the no-sync-setState-in-effect rule), or store a single {key, data | error} result.
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for keying the Insights response by connection and filters, and for adding the pending-connection regression. The two targeted suites pass on this head with 42 tests.
There is still a request-state issue when the same selection is fetched again. A failed request stores loadedError, but a later successful request only updates loadedInsights and never clears that error. For example, open Insights for A and let its request fail, switch to B, then return to A and let the new request succeed. Both stored entries now match A's key, so the old error alert remains visible alongside the successful data. Conversely, a later failure does not clear previously loaded data for that same key.
Please keep the result and error in one request state, or clear the opposite state when a request settles, and add a retry/switch-back regression covering failure followed by success and success followed by failure. This matters because the current warning tells users that the backend failed even after the current request succeeded.
I have not run the real-app backend execution-log path on this head.
|
@debba Thanks for the review!
|
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for replacing the independent cached data/error states with one keyed result. I checked the A-to-B-to-A retry handling on 11f5217. The 43 affected frontend tests, typecheck and all 2,389 Rust library tests pass locally; the previous error can no longer coexist with a successful response for that request key.
Please rebase onto current main before the final merge check. There are conflicts in query_history.rs, the desktop command registry and the locale files. Keep the new single-result state and retry regressions while resolving those conflicts. This remains open for that integration update.
11f5217 to
0a1f229
Compare
|
@NewtTheWolf Rebased onto current main and resolved the conflicts in query_history.rs, the desktop command registry, and the locale files while preserving the unified single-result state and retry regression coverage. All 43 targeted frontend tests, typecheck, Prettier, and CI workflows pass cleanly. |
NewtTheWolf
left a comment
NewtTheWolf
left a comment
There was a problem hiding this comment.
Thanks for preserving the unified Insights result during the rebase. On 0a1f229d, the query-history implementation and request-state code are byte-for-byte unchanged from the corrected 11f5217b revision. The A-to-B-to-A retry regression remains covered. The 43 affected frontend tests, typecheck, lint, production build and desktop-free cargo check pass; the full Rust library suite passes serially with 2,528 passed and 19 ignored.
The code correction still looks good, but the branch conflicts with the main that has advanced during this rerun. Please integrate current main again, preserving the single keyed data/error result and the current desktop command registry and locale entries. I am requesting changes for that remaining integration blocker, rather than treating the earlier green CI as evidence for a conflict resolution that has not been reviewed. I have not repeated the native execution-log flow in this pass.
debba
left a comment
debba
left a comment
There was a problem hiding this comment.
Thanks for replacing the separate data/error caches with one keyed result. A successful request now replaces the old error, and a failure replaces old data. The switch-back regression exercises both directions; the 43 affected frontend tests and typecheck pass on this head.
That addresses my previous request. One integration blocker remains: the branch conflicts with current main in packages/web-ui/src/components/layout/sidebar/QueryHistorySection.tsx and its component test. I confirmed both conflicts with a local merge-tree check.
Please rebase and resolve those two files while preserving the unified result state and the retry regression, then rerun the affected tests. I'm requesting changes for the integration conflict, not for the corrected request handling.
|
@debba Rebased cleanly onto latest |
0a1f229 to
df0eba8
Compare
Closes #1413
Summary of Changes
This PR adds an Insights view to Query History that aggregates recorded query executions by normalized query shape and surfaces client-observed performance metrics and period-over-period regressions:
Dialect-Aware Query Shape Normalization (
src-tauri/src/query_history.rs&packages/web-ui/src/utils/queryHistoryInsights.ts):--, plus#in MySQL while respecting MySQL's--whitespace rule) and block comments (/* ... */, including nested block comments in PostgreSQL/MSSQL).'...', PostgreSQLE'...'and$$...$$, MySQL"...") with'?'and numeric/hex/bit/boolean literals with?.IN (?, ?, ...)lists intoIN (?+).SELECT,INSERT,UPDATE,DELETE,WITH), table/column identifiers, and dialect-quoted identifiers ("...",`...`,[...]).Bounded Non-Deduplicated Execution Log & Insights Command (
src-tauri/src/application/productivity.rs):{connection_id}.executions.json) alongside the consecutively-deduplicated history view ({connection_id}.json) so repeated executions of the same query are accurately counted incallCountand timing averages while keepingget_query_historybackward-compatible.get_query_history_insightsacross both Desktop Tauri commands and Web JSON-RPC transport, isolated perconnectionIdand optionaldatabasefilter.Regression Detection & Coverage Disclosure:
Last 24h,Last 7d,Last 30d) against the immediately preceding window of equal length.MIN_REGRESSION_SAMPLES = 3timed executions in both current and prior windows), a minimum absolute increase (>= 10ms), and a minimum relative increase (>= +25%) before flagging a shape asregressed; under-sampled shapes are labeledinsufficientSamples.executionTimeMs: null) from timing averages while reporting their count inmissingDurationCount.Query History Insights UI & Tests (
packages/web-ui/src/components/layout/sidebar/QueryHistorySection.tsx):Regressions first,Total time,Average time,Call count), andRegressions only.Open in [code]smith · Need help on this PR? Tag
@codesmith-botwith what you need.