Repository navigation
fix: restore run TUI elapsed timer across retry, failure, cancel, and remote paths - #221
Merged
compozybot merged 4 commits intoJul 3, 2026
Merged
Conversation
Thread the executor's authoritative JobCompletedPayload.DurationMs through jobFinishedMsg into handleJobFinished, which previously derived elapsed time from a startedAt that the retry flow never seeds (retry attempts emit no JobStarted; a remote tab can bootstrap a job mid-retry). The sidebar timer no longer stays blank after a retried job completes. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
…e jobs Complete the retry-timer fix across every terminal outcome. Add DurationMs to JobFailedPayload/JobCancelledPayload and populate it in the executor (markGiveUp/markCanceled, zero-guarded when no attempt started), thread it through the failure and cancel UI translators, and add DurationMs to the RunJobSummary contract populated by the daemon snapshot builder (authoritative payload value with a JobStarted->terminal timestamp fallback for historical journals). The remote bootstrap threads it into every terminal arm, so a tab attaching to a job that finished before attach shows the real elapsed time instead of a bogus ~00:00. No persistence/schema migration required. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThis PR adds ChangesJob Duration Threading
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
|
| Filename | Overview |
|---|---|
| internal/core/run/executor/lifecycle.go | Adds zero-guarded elapsedMs() helper and populates DurationMs on JobFailedPayload and JobCancelledPayload, mirroring the existing markSuccess pattern. |
| internal/core/run/ui/update.go | Core UI fix: handleJobFinished now prefers authoritative DurationMs and backfills startedAt for coherence when it was never seeded by a retry path. |
| internal/daemon/run_snapshot.go | Adds startedAt to runSnapshotJob and resolveDurationMs with payload-wins/timestamp-fallback for historical journals; all three terminal apply methods updated. |
| internal/core/run/ui/remote.go | Adds summaryDurationMs nil-safe helper and threads it through all three terminal bootstrap arms (completed, failed, canceled). |
| pkg/compozy/events/kinds/job.go | Extends JobFailedPayload and JobCancelledPayload with DurationMs int64, matching the existing field on JobCompletedPayload. |
| internal/api/contract/types.go | Adds DurationMs int64 to RunJobSummary for the remote-attach snapshot contract. |
| internal/core/run/ui/model.go | Threads DurationMs from payloads into jobFinishedMsg across the failure, completed, and cancelled translators. |
| internal/core/run/ui/types.go | Adds DurationMs int64 to jobFinishedMsg to carry authoritative duration from executor to UI handler. |
| internal/core/run/executor/execution_acp_test.go | New tests cover failed/canceled/pre-attempt zero-guard terminal duration paths; subtests use t.Parallel() correctly. |
| internal/core/run/ui/update_test.go | Adds reproduction test for the retry→success timer restoration scenario; verifies sidebarTimeString renders the authoritative duration. |
| internal/core/run/ui/adapter_test.go | Adds translator threading tests for completed and cancelled events, and extends the existing failure test with a DurationMs assertion. |
| internal/core/run/ui/remote_test.go | Adds test verifying all three terminal bootstrap arms thread DurationMs; extends existing snapshot test with duration assertion. |
| internal/daemon/run_snapshot_test.go | New TestRunSnapshotJobResolveDurationMs covers payload-wins, timestamp-fallback, and no-start cases; extends existing lifecycle test with a duration assertion. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant E as Executor (lifecycle.go)
participant J as Journal / Event Bus
participant T as UI Translator (model.go)
participant H as handleJobFinished (update.go)
participant S as Sidebar Timer
Note over E: markGiveUp / markCanceled / markSuccess
E->>E: elapsedMs() → DurationMs
E->>J: "emit JobFailed/Cancelled/Completed {DurationMs}"
J->>T: translateMessages / translateJobCompletedEvent
T->>H: "jobFinishedMsg{DurationMs}"
H->>H: "if DurationMs>0 → job.duration = DurationMs*ms"
H->>H: backfill startedAt if zero
H->>S: render duration
Note over J,S: Remote pre-attach path
J->>J: "RunSnapshotBuilder.applyJob*"
J->>J: resolveDurationMs(payload vs timestamp)
J->>J: RunJobSummary.DurationMs set
J->>T: "remoteBootstrapTerminalMsgs{summaryDurationMs}"
T->>H: "jobFinishedMsg{DurationMs}"
H->>S: render duration
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant E as Executor (lifecycle.go)
participant J as Journal / Event Bus
participant T as UI Translator (model.go)
participant H as handleJobFinished (update.go)
participant S as Sidebar Timer
Note over E: markGiveUp / markCanceled / markSuccess
E->>E: elapsedMs() → DurationMs
E->>J: "emit JobFailed/Cancelled/Completed {DurationMs}"
J->>T: translateMessages / translateJobCompletedEvent
T->>H: "jobFinishedMsg{DurationMs}"
H->>H: "if DurationMs>0 → job.duration = DurationMs*ms"
H->>H: backfill startedAt if zero
H->>S: render duration
Note over J,S: Remote pre-attach path
J->>J: "RunSnapshotBuilder.applyJob*"
J->>J: resolveDurationMs(payload vs timestamp)
J->>J: RunJobSummary.DurationMs set
J->>T: "remoteBootstrapTerminalMsgs{summaryDurationMs}"
T->>H: "jobFinishedMsg{DurationMs}"
H->>S: render duration
Reviews (3): Last reviewed commit: "test: wrap translator duration tests in ..." | Re-trigger Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
internal/daemon/run_snapshot_test.go (1)
14-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign new subtest names with the file's "Should..." convention.
Sibling subtests in this same function (e.g., "Should clone run job summary code files") follow a "Should..." naming convention, but the new
resolveDurationMssubtests use plain descriptions instead.♻️ Proposed rename for consistency
- t.Run("prefers the authoritative payload duration", func(t *testing.T) { + t.Run("Should prefer the authoritative payload duration", func(t *testing.T) { t.Parallel() job := &runSnapshotJob{startedAt: base} if got := job.resolveDurationMs(90_000, base.Add(2*time.Second)); got != 90_000 { t.Fatalf("want payload duration 90000, got %d", got) } }) - t.Run("falls back to the started->terminal timestamp span", func(t *testing.T) { + t.Run("Should fall back to the started->terminal timestamp span", func(t *testing.T) { t.Parallel() job := &runSnapshotJob{startedAt: base} if got := job.resolveDurationMs(0, base.Add(3*time.Second)); got != 3000 { t.Fatalf("want fallback duration 3000, got %d", got) } }) - t.Run("reports zero when the job never started", func(t *testing.T) { + t.Run("Should report zero when the job never started", func(t *testing.T) { t.Parallel() job := &runSnapshotJob{} if got := job.resolveDurationMs(0, base.Add(3*time.Second)); got != 0 { t.Fatalf("want zero duration without a start, got %d", got) } })As per path instructions, "MUST use t.Run("Should...") pattern for ALL test cases".
🤖 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 `@internal/daemon/run_snapshot_test.go` around lines 14 - 39, The new subtests in TestRunSnapshotJobResolveDurationMs do not follow the required “Should...” naming convention used elsewhere in this test file. Rename each t.Run case in TestRunSnapshotJobResolveDurationMs to start with “Should...” so it matches the existing pattern and the path instructions, while keeping the same coverage for resolveDurationMs behavior.Source: Path instructions
🤖 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.
Inline comments:
In `@internal/core/run/executor/execution_acp_test.go`:
- Around line 1454-1515: Rename each subtest in
TestJobLifecycleTerminalEventsCarryDuration to follow the required
t.Run("Should...") convention, and add t.Parallel() at the start of each
independent subtest. Keep the existing assertions and setup in the new subtests,
and apply the same pattern consistently across the lifecycle scenarios so the
test names and execution model match the project guidelines.
In `@internal/core/run/ui/adapter_test.go`:
- Around line 44-70: The new duration translation tests are written as
standalone test functions instead of the required subtest format. Update
TestTranslateEventJobCompletedThreadsDuration and the matching
TestTranslateEventJobCancelledThreadsDuration to be wrapped in t.Run with a
"Should..." subtest name, keeping the existing assertions inside the subtest
body. Make the change in the translateEventForTest coverage so all new cases
follow the repo’s test naming convention.
---
Nitpick comments:
In `@internal/daemon/run_snapshot_test.go`:
- Around line 14-39: The new subtests in TestRunSnapshotJobResolveDurationMs do
not follow the required “Should...” naming convention used elsewhere in this
test file. Rename each t.Run case in TestRunSnapshotJobResolveDurationMs to
start with “Should...” so it matches the existing pattern and the path
instructions, while keeping the same coverage for resolveDurationMs behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7c494b1d-cb1e-4d66-bbfb-019953abe654
📒 Files selected for processing (13)
internal/api/contract/types.gointernal/core/run/executor/execution_acp_test.gointernal/core/run/executor/lifecycle.gointernal/core/run/ui/adapter_test.gointernal/core/run/ui/model.gointernal/core/run/ui/remote.gointernal/core/run/ui/remote_test.gointernal/core/run/ui/types.gointernal/core/run/ui/update.gointernal/core/run/ui/update_test.gointernal/daemon/run_snapshot.gointernal/daemon/run_snapshot_test.gopkg/compozy/events/kinds/job.go
Addresses PR review: rename the TestJobLifecycleTerminalEventsCarryDuration and TestRunSnapshotJobResolveDurationMs subtests to the repo's Should-prefixed convention and mark the independent lifecycle subtests t.Parallel(). Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
Addresses the remaining CodeRabbit review comment: the two TestTranslateEvent*ThreadsDuration cases now run inside a Should-prefixed t.Run subtest, matching the ui package's dominant convention. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
pedronauck
added a commit
that referenced
this pull request
Jul 3, 2026
Add human-written release notes for the next release covering the work merged since v0.2.10: - feat: dependency-aware parallel task execution (--parallel-tasks) (#212) - feat: agentic recovery for failed runs (--recovery) (#212) - feat: COMPOZY_HOME for isolated per-project daemons (#216) - feat: repo-level default overrides in compozy setup (#90) - fix: safer worktree management for parallel runs (#217, #223) - fix: ACP runs consistently apply the selected model (#215) - fix: run TUI elapsed timer restored across all terminal outcomes (#221) Co-Authored-By: Claude Fable 5 <[email protected]>
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
2 participants
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
Fixes the run TUI bug where the elapsed timer disappears after a retried job succeeds — and completes the fix so the timer is correct for every terminal outcome (success, failure, cancel) and for remote tabs that attach after a job already finished.
Root cause: the UI derived a job's elapsed time from a locally-tracked
startedAtthat the retry flow never seeds (retry attempts emit no freshJobStarted, and a remote tab can bootstrap a job mid-retry), while discarding the authoritative duration the executor already computes.What changed
Core fix — thread the executor's authoritative
JobCompletedPayload.DurationMsinto the UI:jobFinishedMsggainsDurationMs;translateJobCompletedEventcarries it;handleJobFinishedprefers it and backfillsstartedAtfor triple-coherence.Completion across all terminal outcomes — no persistence/schema migration required:
DurationMsadded toJobFailedPayload/JobCancelledPayload, populated inexecutor/lifecycle.govia a zero-guardedelapsedMs()(guard matters:markGiveUp/markCanceledcan fire before any attempt starts), threaded through the failure and cancel UI translators.DurationMsadded to theRunJobSummarycontract; the daemon snapshot builder populates it from the authoritative payload with aJobStarted → terminaltimestamp fallback for historical journals;remote.gothreads it into all three terminal bootstrap arms.Why this altitude
Duration was only carried on the live
JobCompletedevent. Generalizing it to all terminal payloads + the remote summary contract fixes the whole class (blank timer on retry→failure/cancel; ~00:00on remote pre-attach) instead of special-casing the success arm.Testing
gofmt -s: clean ·go build ./...: OK ·golangci-lint(5 packages): 0 issuesgo test -race:pkg/compozy/events/kinds,internal/core/run/ui,internal/core/run/executor,internal/daemon,internal/api/contract— all passresolveDurationMs(payload-wins / timestamp-fallback / no-start), plus a translator guard and reproduction test in the core fix.Summary by CodeRabbit
New Features
Bug Fixes
Tests