feat: Phase 1b wiring — step timeline, run reconstruction, SSE recovery - #103
Merged
Conversation
Complete the remaining Phase 1b dashboard items: - Step timeline in detail drawer: click a run dot to see per-step progress with status badges, durations, and failure messages - Historical run reconstruction: on page load, fetch /v1/events and replay workflow events to rebuild appState.runs - SSE reconnect recovery: on reconnect, refetch health + events to recover missed state changes - Run-stage HUD: shows current stage and step progress (e.g. "1 run → implementer 10/23") instead of just a count - formatDuration helper for human-readable step durations - 20+ new JS test assertions for step timeline, formatDuration, and terminal-state filtering in runsAtStage Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded run selection UI and a run-detail drawer, per-step timelines on runs, duration formatting, and historical run reconstruction by replaying Changes
Sequence DiagramsequenceDiagram
participant Browser as Browser
participant App as App State
participant EventAPI as /v1/events API
participant SSE as SSE Stream
rect rgba(100, 150, 200, 0.5)
Note over Browser,App: Boot / Reconstruction
Browser->>App: Initialize appState
App->>EventAPI: GET /v1/events
EventAPI-->>App: event history
App->>App: reconstructRuns() (replay events -> build runs & steps)
end
rect rgba(150, 100, 200, 0.5)
Note over SSE,App: Live Event Processing
SSE->>App: workflow.started
App->>App: init run.steps = []
SSE->>App: workflow.step.started
App->>App: append step (type, agent, stage, startedAt)
SSE->>App: workflow.step.completed/failed
App->>App: update step (status, durationMs, message)
SSE->>App: workflow.completed/failed
App->>App: set run.finishedAt
end
rect rgba(200, 150, 100, 0.5)
Note over Browser,App: Run Selection & Drawer
Browser->>App: click run dot
App->>App: selectRun(id) (clear selectedAgent)
App->>Browser: render drawer with run.steps (badges, durations, messages)
Browser->>App: close drawer
App->>App: clear selections
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
When /v1/events returns only recent events, workflow.started events for old runs get reconstructed without their matching terminal events, creating zombie "running" runs. Prune runs older than 30 minutes that are still marked running after reconstruction completes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
dashboard/test.html (1)
409-448: Add a zero-duration step assertion.
formatDuration(0)is covered above, but the step timeline suite only exercises non-zeroduration_ms. That won't catch theduration_ms || nulledge case in the productionprocessWorkflowEventpath, so a legitimate0msstep can still slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/test.html` around lines 409 - 448, Add a test case in the "step timeline tracking" suite that sends a workflow.step.completed (or .failed) event with data.duration_ms set to 0 via processWorkflowEvent and then asserts that testRuns["st-1"].steps[<index>].durationMs is 0 (not null); reference processWorkflowEvent and testRuns so the test exercises the production path that currently uses "duration_ms || null" and ensures zero-duration steps are recorded as 0.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@dashboard/app.js`:
- Around line 89-93: The duration assignment drops legitimate zero values
because it uses the || operator; change the assignment in the block that updates
run.steps[cidx].durationMs from "(evt.data && evt.data.duration_ms) || null" to
use the nullish coalescing operator "(evt.data && evt.data.duration_ms) ?? null"
(so 0 is preserved), and make the identical fix in the other analogous block
(the second occurrence around lines 98-102) that sets run.steps[...].durationMs.
- Around line 545-549: reconnect flow currently replays full history directly
into appState.runs and appState.events while EventSource.onopen is already
delivering live events, allowing older replayed events to overwrite newer live
ones and making reconnect cost proportional to full history; change
reconstructRuns() to (a) use the server-supported cursor (the same cursor used
by /v1/events) to fetch only the delta since the last seen cursor OR (b) replay
history into temporary structures (e.g., tempRuns, tempEvents) and atomically
replace appState.runs and appState.events only after replay finishes;
additionally ensure EventSource.onopen either supplies a starting
cursor/lastEventId to avoid duplicate older events or pauses/buffers incoming
live events until the swap completes so live events cannot be overwritten.
- Around line 276-283: Replace the non-semantic span used for run dots by
creating a button instead of a span via el("button", "run-dot") where the
variable dot is created, set dot.type = "button", move the existing dot.title
content into an accessible label by adding dot.setAttribute("aria-label",
dot.title) (keep dot.title if desired), and keep the existing click handler that
calls selectRun(stageRuns[r].id) so keyboard activation works; also update the
styling rules that target the "run-dot" class to reset default button styles and
add :focus-visible (or similar) focus styling so the new buttons look the same
as before but are keyboard-visible and accessible.
In `@dashboard/style.css`:
- Around line 355-360: Replace the deprecated word-break usage in the .step-msg
rule: remove the "word-break: break-word" declaration and add "overflow-wrap:
anywhere" to achieve equivalent long-word wrapping; update the .step-msg CSS
block (target the .step-msg selector) to use overflow-wrap: anywhere instead of
word-break: break-word so modern browsers follow the current CSS Text spec.
---
Nitpick comments:
In `@dashboard/test.html`:
- Around line 409-448: Add a test case in the "step timeline tracking" suite
that sends a workflow.step.completed (or .failed) event with data.duration_ms
set to 0 via processWorkflowEvent and then asserts that
testRuns["st-1"].steps[<index>].durationMs is 0 (not null); reference
processWorkflowEvent and testRuns so the test exercises the production path that
currently uses "duration_ms || null" and ensures zero-duration steps are
recorded as 0.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: f2e2c46b-9a08-402a-941f-92a2b1e772e1
📒 Files selected for processing (3)
dashboard/app.jsdashboard/style.cssdashboard/test.html
There was a problem hiding this comment.
🧹 Nitpick comments (1)
dashboard/app.js (1)
676-679: Add defensive check for null response.If the fetch succeeds but returns an unexpected response structure (e.g., empty body),
json.dataaccess could throw.Suggested fix
return fetch("/v1/events").then(function(res) { return res.json(); }).then(function(json) { - var events = json.data || []; + var events = (json && json.data) || [];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@dashboard/app.js` around lines 676 - 679, The code that processes the fetch response (the promise chain starting with fetch("/v1/events") and the assignment to var events = json.data || [];) should defensively handle a null/undefined JSON body; change the assignment to verify json is truthy and json.data is present (e.g., if (!json || !json.data) events = []; else events = json.data) or use a safe expression like var events = (json && json.data) || []; ensure this guard sits immediately after res.json() resolves so accessing json.data cannot throw.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@dashboard/app.js`:
- Around line 676-679: The code that processes the fetch response (the promise
chain starting with fetch("/v1/events") and the assignment to var events =
json.data || [];) should defensively handle a null/undefined JSON body; change
the assignment to verify json is truthy and json.data is present (e.g., if
(!json || !json.data) events = []; else events = json.data) or use a safe
expression like var events = (json && json.data) || []; ensure this guard sits
immediately after res.json() resolves so accessing json.data cannot throw.
- Preserve zero-length step durations (use != null instead of ||) - Use button element for run-dot (keyboard accessibility) - Skip full event replay on SSE reconnect (health snapshot only) - Replace deprecated word-break: break-word with overflow-wrap: anywhere Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.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
/v1/eventsand replays workflow events to rebuildappState.runs— dashboard shows runs in progress even after a page refreshTest plan
cargo test— 327 tests passcargo clippy+cargo fmtcleandashboard/test.htmlin browser — all JS tests passtt serve, triggertt run sdlc-auto, click run dot → step timeline visible/v1/events🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Style