dashboard: land Rollouts on the newest step that has data - #2815
Conversation
A step is listed as soon as its rollout dump file appears, which is earlier than the step can be read. The Rollouts tab baked that newest id straight into its href, so during a live run clicking it regularly landed on an error page and the only way out was editing the step number in the URL by hand. Three states produce that error, and they surface differently: a dump still being written returns 503, a truncated one raises out of torch.load as a 500, and a step recorded with no samples yields a summary with no columns at all, which makes the groups query fail with a 500. Rather than special-case each, the tab now points at #/rollout/latest and the view resolves that against the dump: walk back from the newest step and take the first one that both loads and has at least one sample, then rewrite the URL to it so reloads, Prev/Next and the breadcrumb all work off a real id. The lookback is bounded at 5 steps. Past that this is not a fresh-dump race any more, so the newest step is shown and its real error surfaces instead of the reader being walked silently into old data. A step named explicitly in the URL is still honoured exactly. Only the tab's own landing resolves, so a typed or linked step number never redirects.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
|
@claude review always |
There was a problem hiding this comment.
Looks good — the follow-up commit addresses the mid-resolve navigation race I flagged in the previous review by capturing location.hash before the async resolveLatest() call and skipping location.replace if it changed. Checked the guard logic and the rest of the resolveLatest/renderRollout/parseRoute flow; no other issues found in this diff.
Extended reasoning...
Overview
This is the second push of a small, self-contained frontend change (app.js, views_rollout.js, README.md) that makes the Rollouts tab land on the newest step with readable data instead of the newest listed step id, which can be mid-write or empty. Since my prior review, the only code change is commit e71d9df, which adds an entryHash/location.hash check before calling location.replace in renderRollout.
Security risks
None — this is client-side navigation/UI logic operating on data the client already has read access to; no new API surface, auth, or data exposure changes.
Level of scrutiny
Light-to-moderate is appropriate: pure frontend JS in a dashboard, no test harness exists for it (documented in the PR), and the only substantive concern from the last round (an async race that could yank the user back into the rollout view after they navigated away) has been directly fixed with a straightforward guard. I re-read the fixed code in views_rollout.js lines 86-106 and confirmed the guard captures the hash at entry and bails before rewriting the URL if the hash changed during the await.
Other factors
No other reviewers have outstanding CHANGES_REQUESTED or unaddressed objections in the timeline, and this run's bug hunt reported no findings. The fix is small, targeted, and matches exactly what was requested, so no further human-only judgment call is needed here.
… Rollouts The tab's href is now the constant #/rollout/latest, so clicking it from inside the rollout view is a navigation to a URL the browser has not got, and it pushes a session-history entry; the landing then replaces that entry with the step it resolved. History ends up holding the same step twice in a row, and since the router only listens to hashchange -- which does not fire between two entries with the same fragment -- Back moves between them and changes nothing on screen. One stray click on the tab costs the reader one dead Back press. The old href carried the step id, so from inside the view it equalled the current URL and the browser coalesced the click into a replace. Restoring that by replacing explicitly when the tab points at the view already showing. Verified in Chrome against a served dump whose newest step has no samples: the self-click adds no entry and still resolves to the landing step, one Back leaves the rollout view, an explicitly named step is still never redirected, and Metrics -> Rollouts -> sample -> breadcrumb -> Metrics is unchanged.
…step api() retries a 503 three times, sleeping 1.5s between attempts, and DumpStillWriting is exactly what the server maps to 503 -- so every candidate resolveLatest rejects for being mid-write costs four requests and 4.5s before the walk-back moves on, up to five times over. The landing sits on "finding the newest step with data…" for that whole time, and the re-render after it resolves starts a fresh budget of its own. Here a 503 is the answer rather than a transient: the step is not readable yet, which is the condition the walk-back exists to skip. Waiting for it to clear contradicts the fallback. Opt out for the probes only; every other caller keeps the retry. FRESH_SECONDS caps how many consecutive steps can answer 503 at all, so in practice this recovers seconds rather than the full worst case.
9577b5b to
6d9679e
Compare
| const replaceInPlace = (event) => { | ||
| event.preventDefault(); | ||
| location.replace("#/rollout/latest"); | ||
| }; | ||
| parts.push(nav("Rollouts", "#/rollout/latest", onRollout, onRollout ? replaceInPlace : null)); |
There was a problem hiding this comment.
🟡 (optional) The Rollouts breadcrumb's onclick calls event.preventDefault() unconditionally whenever the nav is active, so ctrl/cmd/middle-click to open the link in a new tab silently breaks (it navigates the current tab via location.replace instead), whereas on base this was a plain anchor that always honored modifier clicks. Fix: only preventDefault/replace for a plain left-click with no modifier keys (check event.button === 0 and !event.ctrlKey/!event.metaKey/!event.shiftKey), letting other clicks fall through to default anchor behavior.
Extended reasoning...
User is already on a rollout/tokens view (onRollout true), so nav("Rollouts", ...) gets the replaceInPlace onclick. Middle-clicking or ctrl+clicking the Rollouts crumb to open the latest step in a new tab still triggers event.preventDefault(); location.replace("#/rollout/latest") on the CURRENT tab, discarding the click's intent and no new tab opens - a capability present before this diff since the anchor had no onclick at all.
Verification: Severity: nit (minor UX regression from base, no crash/error page). app.js crumbs() now does, only when already on the rollout/tokens view (onRollout true): const replaceInPlace = (event) => { event.preventDefault(); location.replace("#/rollout/latest"); }; parts.push(nav("Rollouts", "#/rollout/latest", onRollout, onRollout ? replaceInPlace : null)); and el() installs this via… | nit.…
2 participants
Summary
The Rollouts tab baked the newest step id straight into its
href. A step is listed as soon as its rollout dump file appears, which is earlier than that step can be read, so during a live run clicking the tab regularly landed on an error page — and the only way out was editing the step number in the URL by hand.The tab now points at
#/rollout/latest, which the view resolves against the dump: walk back from the newest step and take the first one that both loads and has at least one sample.Same dump, one click on Rollouts. On the left the newest step (2) is unreadable and the page is a dead end; on the right it falls through to step 1 and shows data.
The three states that break the landing
I damaged a real dummy dump each way and asked the reader directly, because the fix should not be tuned to one error shape:
summary()groups()DumpStillWritingDumpStillWritingRuntimeErrorfromtorch.loadColumnNotFoundErrorFileNotFoundErrorThe no-samples row is the one that matches "the most recent step hasn't generated any rollout yet":
summary()succeeds but returns a frame with no columns, so thegroup_by("group_index")insidegroups()fails. SincerenderRolloutfetches both in aPromise.all, either failure takes the whole page down.Rather than special-case each status, the landing treats "threw" and "loaded but has no samples" identically as not usable yet.
What changed
app.js— the Rollouts link becomes#/rollout/latest;parseRoutemaps that torolloutId: null; the step breadcrumb is skipped while the id is unresolved.views_rollout.js—renderRolloutresolves a null id viaresolveLatest, thenlocation.replaces the hash to the step it picked, so reloads, Prev/Next and the breadcrumb all work off a real id.location.replacerather than assignment keeps the unresolved URL out of history.Two deliberate limits:
#/rollout/2on a broken step still shows that step's error — a shared link or a jump-box entry has to mean exactly what it says.Verification
No JS test harness exists in the repo (no
package.json; the Python tests undertests/fast/dashboard/never load the static files), so this was driven in real Chrome via Playwright against dumps built bytests/fast/dashboard/dummy_dump.pythrough the real dump pipeline, then damaged as above.Clicking Rollouts, on a 3-step dump:
#/rollout/2#/rollout/1#/rollout/1#/rollout/0Guard rails, on the dump whose newest step is broken:
#/rollout/2#/rollout/2, showsHTTP 500— honoured exactly, no redirect#/, not into a redirect loop#/rollout/latest?eval=1with no eval stepsTest plan
pre-commitcovers Python only and no Python file is touched. This is static frontend only: no API, schema or reader behaviour changes, so the dashboard's Python tests are unaffected.