Add snapshot phase timing display to dashboard UI - #257
Merged
Conversation
Add real-time phase timing metrics to snapshot dashboard. - Add timing card panel showing Read, Build, Transform, Write durations and rows/sec - Add timing labels to individual table progress rows - Include per-phase timing data (read_ms, build_ms, transform_ms, write_ms) in status API response - Calculate and display global read rows per second metric - Add fmtMs() helper for formatting milliseconds to human-readable values
|
ℹ️ 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)
WalkthroughThe PR extends the snapshot status API to include per-table and global read/build/transform/write millisecond totals and read_rows_per_sec. The dashboard HTML adds a hidden Phase Timing panel, a showTimingPanel() helper, and fmtMs(ms). updateRunningUI() now populates global timing and throughput fields from the API, and updateTableProgress() adds per-table inline R/B/T/W timing labels formatted by fmtMs. 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
renderSnapshotStatus, the running progress percentage no longer clamps withMath.min(100, ...), so ifread_rowsovershootstotal_rowsthe bar and label can exceed 100%; consider retaining the clamp to avoid odd UI states when counts are approximate. - Since the backend description mentions global
ReadMs/BuildMs/TransformMs/WriteMsfields onProgress, you may want to reuse those instead of recomputingacc.read/build/transform/writeinhandleSnapshotStatusto avoid divergence between two sources of truth. - The per-table timing label (
R:/B:/T:/W:) is appended as a single monospaced span before the mini progress bar; consider wrapping it in its own flex container or allowing it to wrap on narrow screens to avoid truncating long table names in the main label.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `renderSnapshotStatus`, the running progress percentage no longer clamps with `Math.min(100, ...)`, so if `read_rows` overshoots `total_rows` the bar and label can exceed 100%; consider retaining the clamp to avoid odd UI states when counts are approximate.
- Since the backend description mentions global `ReadMs/BuildMs/TransformMs/WriteMs` fields on `Progress`, you may want to reuse those instead of recomputing `acc.read/build/transform/write` in `handleSnapshotStatus` to avoid divergence between two sources of truth.
- The per-table timing label (`R:/B:/T:/W:`) is appended as a single monospaced span before the mini progress bar; consider wrapping it in its own flex container or allowing it to wrap on narrow screens to avoid truncating long table names in the main label.
## Individual Comments
### Comment 1
<location path="cmd/app/dashboard/pages/snapshot.html" line_range="343-345" />
<code_context>
document.getElementById('run-current-table').textContent = data.current_table || '—';
- var pct = totalRows > 0 ? Math.min(100, Math.round((readRows / totalRows) * 100)) : 0;
+ var pct = totalRows > 0 ? Math.round((readRows / totalRows) * 100) : 0;
document.getElementById('run-progress-bar').style.width = pct + '%';
document.getElementById('run-pct').textContent = pct + '%';
+ document.getElementById('run-read-ms').textContent = fmtMs(data.read_ms);
</code_context>
<issue_to_address>
**issue (bug_risk):** Consider capping progress percentage at 100% to avoid layout issues.
Removing the previous `Math.min(100, ...)` means `pct` can now exceed 100%. If `readRows` surpasses `totalRows` (e.g., due to estimates or rounding), the bar width can overflow its container and cause visual glitches. Unless >100% is intentional, please clamp the width (and optionally the displayed percentage) while still tracking the raw value internally if required.
</issue_to_address>
### Comment 2
<location path="cmd/app/dashboard/pages/snapshot.html" line_range="348-351" />
<code_context>
+ 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 ? data.read_rows_per_sec.toFixed(1) + ' r/s' : '—';
+
var errEl = document.getElementById('run-error');
</code_context>
<issue_to_address>
**suggestion:** Avoid treating 0 rows/s as missing data when displaying throughput.
Because `0` is falsy, this will display `—` when throughput is legitimately `0` (or very small values rounding to `0.0`). It would be better to check specifically for `null`/`undefined`, e.g. `data.read_rows_per_sec != null`, before formatting the value as `x.x r/s`.
```suggestion
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' : '—';
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cmd/app/server.go (1)
1708-1719: 💤 Low valueConsider naming the accumulator struct for clarity.
The anonymous struct works correctly and the division-by-zero guard is present. For readability, consider giving it a short name like
totalsorglobalTiming.♻️ Optional naming improvement
- // Accumulate global timing totals. - var acc struct{ read, build, transform, write int64 } - for _, tp := range progress.Tables { - acc.read += tp.ReadMs - acc.build += tp.BuildMs - acc.transform += tp.TransformMs - acc.write += tp.WriteMs - } + // Accumulate global timing totals. + var totals struct{ read, build, transform, write int64 } + for _, tp := range progress.Tables { + totals.read += tp.ReadMs + totals.build += tp.BuildMs + totals.transform += tp.TransformMs + totals.write += tp.WriteMs + } readRowsPerSec := float64(0) - if acc.read > 0 { - readRowsPerSec = float64(progress.ReadRows) * 1000 / float64(acc.read) + if totals.read > 0 { + readRowsPerSec = float64(progress.ReadRows) * 1000 / float64(totals.read) }🤖 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 `@cmd/app/server.go` around lines 1708 - 1719, Rename the anonymous accumulator variable `acc` to a clearer named struct (e.g., `totals` or `globalTiming`) to improve readability; update its declaration (var acc struct{...}) and all subsequent uses (`acc.read`, `acc.build`, `acc.transform`, `acc.write`) in the loop that iterates `for _, tp := range progress.Tables { ... }` and in the `if acc.read > 0 { ... }` guard so the new name is used consistently when computing `readRowsPerSec` from `progress.ReadRows`.cmd/app/dashboard/pages/snapshot.html (1)
409-413: 💤 Low valueConsider more explicit input validation in fmtMs.
The function works correctly for expected inputs (non-negative milliseconds). The
if (!ms)check is loose and catches 0, null, undefined, false, NaN, and empty string. For clarity and robustness, consider explicit checks.♻️ Optional validation improvement
function fmtMs(ms) { - if (!ms) return '—'; + if (ms == null || ms === 0) return '—'; + if (ms < 0) return '—'; if (ms < 1000) return ms + 'ms'; return (ms / 1000).toFixed(1) + 's'; }🤖 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 `@cmd/app/dashboard/pages/snapshot.html` around lines 409 - 413, The fmtMs function uses a loose if (!ms) which treats 0 and other falsy values as missing; update fmtMs to explicitly handle null/undefined and invalid numbers: first check if ms == null (null or undefined) and return '—', then validate Number.isFinite(ms) and that ms >= 0 (return '—' for NaN, non-numeric or negative), and only then format (so 0 returns "0ms"); refer to the fmtMs function to implement these explicit checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cmd/app/dashboard/pages/snapshot.html`:
- Around line 409-413: The fmtMs function uses a loose if (!ms) which treats 0
and other falsy values as missing; update fmtMs to explicitly handle
null/undefined and invalid numbers: first check if ms == null (null or
undefined) and return '—', then validate Number.isFinite(ms) and that ms >= 0
(return '—' for NaN, non-numeric or negative), and only then format (so 0
returns "0ms"); refer to the fmtMs function to implement these explicit checks.
In `@cmd/app/server.go`:
- Around line 1708-1719: Rename the anonymous accumulator variable `acc` to a
clearer named struct (e.g., `totals` or `globalTiming`) to improve readability;
update its declaration (var acc struct{...}) and all subsequent uses
(`acc.read`, `acc.build`, `acc.transform`, `acc.write`) in the loop that
iterates `for _, tp := range progress.Tables { ... }` and in the `if acc.read >
0 { ... }` guard so the new name is used consistently when computing
`readRowsPerSec` from `progress.ReadRows`.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 85ceea18-5c57-421b-8ec0-49bbfaa488b6
📒 Files selected for processing (2)
cmd/app/dashboard/pages/snapshot.htmlcmd/app/server.go
- Add timing card panel showing Read/Build/Transform/Write durations and rows/s - Add timing labels to individual table progress rows - Per-phase timing data (read_ms, build_ms, transform_ms, write_ms) in status API - Calculate and display global read rows per second metric - fmtMs() helper for formatting milliseconds to human-readable values - Clamp progress percentage at 100% to avoid overflow - Fix rows/s display to treat 0 as valid (not missing) data - Timing label wraps on narrow screens
cnlangzi
force-pushed
the
fix/snapshot-timing-ui-display
branch
from
May 13, 2026 02:54
3fb64d7 to
dc7ce73
Compare
![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 2 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="cmd/app/dashboard/pages/snapshot.html">
<violation number="1" location="cmd/app/dashboard/pages/snapshot.html:343">
P2: Progress percentage is no longer clamped, so the bar can exceed 100% when read rows surpass total rows.</violation>
<violation number="2" location="cmd/app/dashboard/pages/snapshot.html:351">
P3: Throughput display treats `0` as missing data, showing `—` instead of `0.0 r/s`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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.
Fixes: #256
What Changed
Backend (internal/snapshot/capturer.go, cmd/app/server.go):
ReadMs/BuildMs/TransformMs/WriteMsfields toProgressstructProgress()method now computes sum of per-table timings as global valuesread_rows_per_sec = ReadRows * 1000 / ReadMsread_ms/build_ms/transform_ms/write_msfieldsFrontend (cmd/app/dashboard/pages/snapshot.html):
R:/B:/T:/W:inline timing labelsfmtMs(0)renders as—,fmtMs(340)as340ms,fmtMs(1200)as1.2sWhy Changed
Users could not see phase-level timing breakdown to identify bottlenecks. Snapshot performance optimization was impossible without visibility. This fix completes the data chain from backend to UI, providing Read/Build/Transform/Write timing per table and globally.
How to Test
API verification:
read_ms/build_ms/transform_ms/write_msread_rows_per_secDashboard verification:
—for null/zero)R:,B:,T:,W:labels with timing values inlineFormat verification:
fmtMs(0)→—fmtMs(340)→340msfmtMs(1200)→1.2sFiles Modified
cmd/app/server.gointernal/snapshot/capturer.gocmd/app/dashboard/pages/snapshot.html