feat(dashboard): persist per-table snapshot phase timing after completion - #258
Merged
Merged
Conversation
- Add TableProgress.Done and expose done in snapshot status API - Render A/B/C/D breakdown table in Phase Timing panel (running + completed) - Stop hiding timing panel when closing running panel Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR exposes per-table completion (Done) from the capturer, includes done in /api/snapshot/status, adds a Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- In
renderTimingBreakdown, thet.tablevalue is interpolated directly into HTML; if table names can be influenced by users or external systems, consider HTML-escaping the name or using a safer DOM construction pattern to avoid potential XSS. renderTimingBreakdownrebuilds the entire table and setsinnerHTMLon every status update; if this function is called frequently or for many tables, consider a more incremental update strategy (or at least reusing the SVG icon markup outside the function) to reduce unnecessary allocations and DOM work.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `renderTimingBreakdown`, the `t.table` value is interpolated directly into HTML; if table names can be influenced by users or external systems, consider HTML-escaping the name or using a safer DOM construction pattern to avoid potential XSS.
- `renderTimingBreakdown` rebuilds the entire table and sets `innerHTML` on every status update; if this function is called frequently or for many tables, consider a more incremental update strategy (or at least reusing the SVG icon markup outside the function) to reduce unnecessary allocations and DOM work.
## Individual Comments
### Comment 1
<location path="cmd/app/dashboard/pages/snapshot.html" line_range="318-324" />
<code_context>
function showSummaryPanel(data) {
document.getElementById('panel-summary').classList.remove('hidden');
showTimingPanel();
+ if (data.tables && data.tables.length > 0) {
+ document.getElementById('run-read-ms').textContent = fmtMs(data.read_ms);
+ document.getElementById('run-build-ms').textContent = fmtMs(data.build_ms);
+ document.getElementById('run-transform-ms').textContent = fmtMs(data.transform_ms);
+ document.getElementById('run-write-ms').textContent = fmtMs(data.write_ms);
+ document.getElementById('run-rows-per-sec').textContent = data.read_rows_per_sec != null ? data.read_rows_per_sec.toFixed(1) + ' r/s' : '—';
+ renderTimingBreakdown(data.tables, data.state || 'completed');
+ }
document.getElementById('sum-tables').textContent = (data.processed || 0) + ' / ' + (data.total || 0);
</code_context>
<issue_to_address>
**issue (bug_risk):** Handle the case where there are no tables by clearing/ resetting timing metrics and breakdown.
Because we only update these fields when `data.tables && data.tables.length > 0`, a snapshot with zero tables will keep showing values from the previous run. Please add an `else` that resets the `run-*` fields (e.g., to `—`) and clears `timing-breakdown` so the panel doesn’t show stale data.
</issue_to_address>
### Comment 2
<location path="cmd/app/dashboard/pages/snapshot.html" line_range="436" />
<code_context>
+ ? '<span class="font-semibold ' + (isDone ? 'text-text' : 'text-primary') + '">' + fmtMs(total) + '</span>'
+ : '<span class="text-textMuted/40">—</span>';
+ return '<tr class="border-b border-border/40 last:border-0 ' + rowBg + '">'
+ + '<td class="py-2 pr-4 whitespace-nowrap text-sm ' + nameColor + ' font-medium">' + icon + t.table + '</td>'
+ + '<td class="py-2 px-3 text-center font-mono text-xs">' + fmtMs(t.read_ms) + '</td>'
+ + '<td class="py-2 px-3 text-center font-mono text-xs">' + fmtMs(t.build_ms) + '</td>'
</code_context>
<issue_to_address>
**🚨 issue (security):** HTML-escape `t.table` before injecting it into `innerHTML` to avoid markup injection.
Concatenating `t.table` into an HTML string and assigning it via `innerHTML` means any `<`, `>`, or `&` in the table name can break the markup or allow injected HTML. Please either HTML-escape `t.table` before concatenation or build these cells via `createElement`/`textContent` to close off this injection vector.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+318
to
+324
| if (data.tables && data.tables.length > 0) { | ||
| document.getElementById('run-read-ms').textContent = fmtMs(data.read_ms); | ||
| document.getElementById('run-build-ms').textContent = fmtMs(data.build_ms); | ||
| document.getElementById('run-transform-ms').textContent = fmtMs(data.transform_ms); | ||
| document.getElementById('run-write-ms').textContent = fmtMs(data.write_ms); | ||
| document.getElementById('run-rows-per-sec').textContent = data.read_rows_per_sec != null ? data.read_rows_per_sec.toFixed(1) + ' r/s' : '—'; | ||
| renderTimingBreakdown(data.tables, data.state || 'completed'); |
There was a problem hiding this comment.
issue (bug_risk): Handle the case where there are no tables by clearing/ resetting timing metrics and breakdown.
Because we only update these fields when data.tables && data.tables.length > 0, a snapshot with zero tables will keep showing values from the previous run. Please add an else that resets the run-* fields (e.g., to —) and clears timing-breakdown so the panel doesn’t show stale data.
| ? '<span class="font-semibold ' + (isDone ? 'text-text' : 'text-primary') + '">' + fmtMs(total) + '</span>' | ||
| : '<span class="text-textMuted/40">—</span>'; | ||
| return '<tr class="border-b border-border/40 last:border-0 ' + rowBg + '">' | ||
| + '<td class="py-2 pr-4 whitespace-nowrap text-sm ' + nameColor + ' font-medium">' + icon + t.table + '</td>' |
There was a problem hiding this comment.
🚨 issue (security): HTML-escape t.table before injecting it into innerHTML to avoid markup injection.
Concatenating t.table into an HTML string and assigning it via innerHTML means any <, >, or & in the table name can break the markup or allow injected HTML. Please either HTML-escape t.table before concatenation or build these cells via createElement/textContent to close off this injection vector.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 3 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="internal/snapshot/capturer.go">
<violation number="1" location="internal/snapshot/capturer.go:308">
P2: `Done` is derived from `tableIndex`, but `tableIndex` also advances when a table is skipped due to query/scan errors. That marks failed tables as completed in progress output.</violation>
</file>
<file name="cmd/app/dashboard/pages/snapshot.html">
<violation number="1" location="cmd/app/dashboard/pages/snapshot.html:436">
P2: `t.table` is concatenated directly into an HTML string assigned via `innerHTML` without escaping. If a table name contains `<`, `>`, or `&`, it will break the markup or allow HTML injection. Escape the value before interpolation, or build the cell using `createElement`/`textContent`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| for i, t := range c.tables { | ||
| tp := TableProgress{ | ||
| Table: t.FullName(), | ||
| Done: c.completed || i < c.tableIndex, |
There was a problem hiding this comment.
P2: Done is derived from tableIndex, but tableIndex also advances when a table is skipped due to query/scan errors. That marks failed tables as completed in progress output.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At internal/snapshot/capturer.go, line 308:
<comment>`Done` is derived from `tableIndex`, but `tableIndex` also advances when a table is skipped due to query/scan errors. That marks failed tables as completed in progress output.</comment>
<file context>
@@ -304,6 +305,7 @@ func (c *SnapshotCapturer) Progress() Progress {
for i, t := range c.tables {
tp := TableProgress{
Table: t.FullName(),
+ Done: c.completed || i < c.tableIndex,
}
if i < len(c.tableTotals) {
</file context>
| ? '<span class="font-semibold ' + (isDone ? 'text-text' : 'text-primary') + '">' + fmtMs(total) + '</span>' | ||
| : '<span class="text-textMuted/40">—</span>'; | ||
| return '<tr class="border-b border-border/40 last:border-0 ' + rowBg + '">' | ||
| + '<td class="py-2 pr-4 whitespace-nowrap text-sm ' + nameColor + ' font-medium">' + icon + t.table + '</td>' |
There was a problem hiding this comment.
P2: t.table is concatenated directly into an HTML string assigned via innerHTML without escaping. If a table name contains <, >, or &, it will break the markup or allow HTML injection. Escape the value before interpolation, or build the cell using createElement/textContent.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmd/app/dashboard/pages/snapshot.html, line 436:
<comment>`t.table` is concatenated directly into an HTML string assigned via `innerHTML` without escaping. If a table name contains `<`, `>`, or `&`, it will break the markup or allow HTML injection. Escape the value before interpolation, or build the cell using `createElement`/`textContent`.</comment>
<file context>
@@ -406,6 +416,43 @@ <h3 class="text-base sm:text-lg font-semibold text-text">CDC Tables</h3>
+ ? '<span class="font-semibold ' + (isDone ? 'text-text' : 'text-primary') + '">' + fmtMs(total) + '</span>'
+ : '<span class="text-textMuted/40">—</span>';
+ return '<tr class="border-b border-border/40 last:border-0 ' + rowBg + '">'
+ + '<td class="py-2 pr-4 whitespace-nowrap text-sm ' + nameColor + ' font-medium">' + icon + t.table + '</td>'
+ + '<td class="py-2 px-3 text-center font-mono text-xs">' + fmtMs(t.read_ms) + '</td>'
+ + '<td class="py-2 px-3 text-center font-mono text-xs">' + fmtMs(t.build_ms) + '</td>'
</file context>
Render idle header button before async status fetch so the action is never blank. Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
After a snapshot finishes, the dashboard timing panel now keeps a per-table breakdown (Read / Build / Transform / Write + total), so users still see each table's phase timings alongside the aggregate cards.
Changes
doneto each entry in/api/snapshot/statustablesarray (TableProgress.Donein snapshot capturer).timing-breakdowntable under Phase Timing; populate during run and on completion (showSummaryPanel).hideRunningPanel()no longer hides the timing panel (avoids losing breakdown when switching from running to completed).Testing
go test ./...(via pre-commit hook before lint step).Made with Cursor