fix(orchestrator): Keep lifecycle rows visible in the work log - #4762
fix(orchestrator): Keep lifecycle rows visible in the work log#4762mwolson wants to merge 5 commits into
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
Keeping these rows in the work log exposed that the row component had no treatment for them, so they borrowed the wrong one. A stopped tool rendered a green check with a "Completed" tooltip once the turn settled, because the settled-turn fallback treats any neutral row as success. A running tool rendered the "Empty" dash, which is meant for rows with no content. Both now render their own state: a spinner tooltipped "Running" while the turn is live, and a square tooltipped "Stopped" for an interrupted tool. An `inProgress` row that outlives its turn is also shown as stopped rather than completed, since the turn ended without the tool ever reporting a result. Reported by Macroscope on pingdotgg#4762, which caught the stopped-as-completed half.
Keeping these rows in the work log exposed that the row component had no treatment for them, so they borrowed the wrong one. A stopped tool rendered a green check with a "Completed" tooltip once the turn settled, because the settled-turn fallback treats any neutral row as success. A running tool rendered the "Empty" dash, which is meant for rows with no content. Both now render their own state: a spinner tooltipped "Running" while the turn is live, and a square tooltipped "Stopped" for an interrupted tool. An `inProgress` row that outlives its turn is also shown as stopped rather than completed, since the turn ended without the tool ever reporting a result. Reported by Macroscope on pingdotgg#4762, which caught the stopped-as-completed half.
caf6f38 to
d30b4a7
Compare
Codex review of pingdotgg#4762 caught that inferring "stopped" from a settled turn is wrong. `activeTurnInProgress` is thread-wide, and background work legitimately outlives the root run: `RunExecutionService` explicitly keeps ingesting a late background command completion after root terminal, in the order running -> root-finalized -> completed. The inference would have labelled that live work "Stopped" and then jumped it to "Completed". Both indicators now read the item's own lifecycle and nothing else. A genuine interrupt already terminalizes the item at the projection layer, which is what made the inference look unnecessary rather than harmful. Also from that review: - The work group's filter is now `workEntryShouldRenderInWorkLog`, a production helper, so the predicate tests exercise the shipped policy instead of a copy of the expression that could drift from it. - The lifecycle joins the row's accessible name ("Ran command …, running"), since the indicator sits inside the row button and did not contribute to it. The spinner is decorative rather than a nested role="status" announcing a generic "Loading". - Component tests cover running during an active turn, running after settle, stopped in both states, and a signal-less row staying hidden. Tooltip text is absent from static markup, so they assert on the icon and accessible name, as the existing failed-indicator test does.
Keeping these rows in the work log exposed that the row component had no treatment for them, so they borrowed the wrong one. A stopped tool rendered a green check with a "Completed" tooltip once the turn settled, because the settled-turn fallback treats any neutral row as success. A running tool rendered the "Empty" dash, which is meant for rows with no content. Both now render their own state: a spinner tooltipped "Running" while the turn is live, and a square tooltipped "Stopped" for an interrupted tool. An `inProgress` row that outlives its turn is also shown as stopped rather than completed, since the turn ended without the tool ever reporting a result. Reported by Macroscope on pingdotgg#4762, which caught the stopped-as-completed half.
Codex review of pingdotgg#4762 caught that inferring "stopped" from a settled turn is wrong. `activeTurnInProgress` is thread-wide, and background work legitimately outlives the root run: `RunExecutionService` explicitly keeps ingesting a late background command completion after root terminal, in the order running -> root-finalized -> completed. The inference would have labelled that live work "Stopped" and then jumped it to "Completed". Both indicators now read the item's own lifecycle and nothing else. A genuine interrupt already terminalizes the item at the projection layer, which is what made the inference look unnecessary rather than harmful. Also from that review: - The work group's filter is now `workEntryShouldRenderInWorkLog`, a production helper, so the predicate tests exercise the shipped policy instead of a copy of the expression that could drift from it. - The lifecycle joins the row's accessible name ("Ran command …, running"), since the indicator sits inside the row button and did not contribute to it. The spinner is decorative rather than a nested role="status" announcing a generic "Loading". - Component tests cover running during an active turn, running after settle, stopped in both states, and a signal-less row staying hidden. Tooltip text is absent from static markup, so they assert on the icon and accessible name, as the existing failed-indicator test does.
…eachable Codex round 2 on pingdotgg#4762. Once `workEntryShouldRenderInWorkLog` admits neutral rows only when they carry a reported lifecycle, the row's neutral handling can never fire: `showNeutralIndicator` and the settled-turn success fallback both require the opposite. So `turnSettled`, `hasReportedLifecycle`, `showNeutralIndicator`, the "Empty" dash branch, and the compound success expression were all dead. Success is now just `workEntryIndicatesToolSuccess`, and the row no longer reads thread-wide turn state at all. Also from that round: - Tests pin "no stopped indicator" properly. A stopped row could have regressed to the dash while every assertion still passed; there is now a negative `lucide-minus` check, and the running case asserts the spinner carries neither `role="status"` nor `aria-label="Loading"`, so the accessibility fix cannot silently revert. - The comment claiming both states get their own indicator is gone, as is the overstatement that a "Run interrupted" divider always sits beneath a stopped row: `cancelled` maps to stopped too and has no such row.
…eachable Codex round 2 on pingdotgg#4762. Once `workEntryShouldRenderInWorkLog` admits neutral rows only when they carry a reported lifecycle, the row's neutral handling can never fire: `showNeutralIndicator` and the settled-turn success fallback both require the opposite. So `turnSettled`, `hasReportedLifecycle`, `showNeutralIndicator`, the "Empty" dash branch, and the compound success expression were all dead. Success is now just `workEntryIndicatesToolSuccess`, and the row no longer reads thread-wide turn state at all. The running test also asserts the spinner carries neither `role="status"` nor `aria-label="Loading"`, so the accessibility fix cannot silently revert. The comment claiming both states get their own indicator is gone, as is the overstatement that a "Run interrupted" divider always sits beneath a stopped row: `cancelled` maps to stopped too and has no such row.
5a7eae3 to
257f810
Compare
…eachable Codex round 2 on pingdotgg#4762. Once `workEntryShouldRenderInWorkLog` admits neutral rows only when they carry a reported lifecycle, the row's neutral handling can never fire: `showNeutralIndicator` and the settled-turn success fallback both require the opposite. So `turnSettled`, `hasReportedLifecycle`, `showNeutralIndicator`, the "Empty" dash branch, and the compound success expression were all dead. Success is now just `workEntryIndicatesToolSuccess`, and the row no longer reads thread-wide turn state at all. The running test also asserts the spinner carries neither `role="status"` nor `aria-label="Loading"`, so the accessibility fix cannot silently revert. The comment claiming both states get their own indicator is gone, as is the overstatement that a "Run interrupted" divider always sits beneath a stopped row: `cancelled` maps to stopped too and has no such row.
257f810 to
365949a
Compare
ApprovabilityVerdict: Approved at Macroscope's review found this PR approvable — UI bug fix with well-scoped changes to work log display logic. Adds helper functions and smarter collapsing to keep running/stopped tools visible. Includes comprehensive test coverage. No backend or API changes. You can add or adjust custom eligibility rules. Learn more. |
Macroscope on pingdotgg#4762. `projectedWorkEntryStatus` collapses `pending`, `running`, and `waiting` into `inProgress`, so keying the spinner off `toolLifecycleStatus` made a queued tool and an approval request blocked on the user both claim to be running, with a "running" tooltip and accessible suffix to match. That was a regression from this PR: before it, those rows reached the indicator chain with no branch that applied and rendered nothing. `workEntryIsExecuting` reads the projected item's real status instead, so only a genuinely running tool spins. `pending` and `waiting` rows still render, since showing in-flight work is the point of the PR, but they claim nothing. The test helper now derives the projected status from the collapsed lifecycle value, so a fixture cannot assert on a pairing the projection never emits; the previous version let a `stopped` row carry a `running` item.
Dismissing prior approval to re-evaluate 7d5e55c
…eachable Codex round 2 on pingdotgg#4762. Once `workEntryShouldRenderInWorkLog` admits neutral rows only when they carry a reported lifecycle, the row's neutral handling can never fire: `showNeutralIndicator` and the settled-turn success fallback both require the opposite. So `turnSettled`, `hasReportedLifecycle`, `showNeutralIndicator`, the "Empty" dash branch, and the compound success expression were all dead. Success is now just `workEntryIndicatesToolSuccess`, and the row no longer reads thread-wide turn state at all. The running test also asserts the spinner carries neither `role="status"` nor `aria-label="Loading"`, so the accessibility fix cannot silently revert. The comment claiming both states get their own indicator is gone, as is the overstatement that a "Run interrupted" divider always sits beneath a stopped row: `cancelled` maps to stopped too and has no such row.
Macroscope on pingdotgg#4762. `projectedWorkEntryStatus` collapses `pending`, `running`, and `waiting` into `inProgress`, so keying the spinner off `toolLifecycleStatus` made a queued tool and an approval request blocked on the user both claim to be running, with a "running" tooltip and accessible suffix to match. That was a regression from this PR: before it, those rows reached the indicator chain with no branch that applied and rendered nothing. `workEntryIsExecuting` reads the projected item's real status instead, so only a genuinely running tool spins. `pending` and `waiting` rows still render, since showing in-flight work is the point of the PR, but they claim nothing. The test helper now derives the projected status from the collapsed lifecycle value, so a fixture cannot assert on a pairing the projection never emits; the previous version let a `stopped` row carry a `running` item.
25de21d to
0af2a6e
Compare
4b122f7 to
8b32d74
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8b32d74. Configure here.
22bd872 to
a27c1cc
Compare
8b32d74 to
b387e85
Compare
a186d64 to
5b1a115
Compare
519c42a to
4c55679
Compare
b387e85 to
ede1305
Compare
There was a problem hiding this comment.
One issue found: the removal of the activeTurnInProgress prop leaves a stale call site in this file's own test, which no longer typechecks. Everything else in the diff (the Spinner running indicator, the , running/, stopped accessible-name suffixes, and the pinned collapsed work-group selection) is consistent with the existing indicator branches in SimpleWorkEntryRow and the shared UI primitives.
Posted via Macroscope — UI Consistency
Dismissing prior approval to re-evaluate dd1f712
CI Check fails on current CTM because layerTest now surfaces ServerSettingsError while the ProviderSessionManager test layer requires never. orDie matches the other test layers.
dd1f712 to
19d3389
Compare
Dismissing prior approval to re-evaluate 19d3389
Dismissing prior approval to re-evaluate 38109f8

Summary
Keep important lifecycle outcomes visible in the web timeline:
assistant message.
These are provider-agnostic presentation fixes. In each case, the server
projection was correct and the web timeline hid the only useful row.
Problem and Fix
inProgressandstoppedlifecycle rows like rows with no lifecycle signal.Defensive Fixes
ServerSettingslayer where the provider-session test runtime requires an infallible layer.Layer.orDie. Production runtime behavior is unchanged.Validation
vp check: passedsession-logic.test.ts,MessagesTimeline.logic.test.ts, andMessagesTimeline.test.tsx: 105 tests passed after the terminal-error appendcorrect while settled-turn folding hid the final capacity error.
Notes for review
A stopped row intentionally has no lifecycle glyph. The component already uses
XIconfor failure, while the red stop marker belongs to the separate interruptlifecycle row. Inventing another glyph would turn a visibility fix into a design
change.
The terminal-error exception is narrow: it requires an
errorwork item with afailedlifecycle. A recovered provider-retry row remains foldable.The original neutral-row behavior arrived in #3022 ui/messages-timeline. Mobile
does not share the affected web filtering or settled-turn folding paths.
Note
Low Risk
UI-only timeline presentation and tests. No auth, data, or orchestration behavior changes beyond a tiny test-layer
.pipe(Layer.orDie)tweak.Overview
Keeps in-flight and interrupted tool rows visible in the chat work log, and stops inferring their status from the whole turn.
Neutral rows with a reported
inProgressorstoppedlifecycle now render; only rows with no lifecycle signal stay hidden. Collapsed groups pin executing and stopped tools so a later completed row cannot bury them. Pending/waiting tools stay unpinned and show no glyph (the old minus/"Empty" indicator is gone).Row indicators now follow the item’s own status: a spinner only when
structuredPayload.status === "running", no success check for stopped tools, andaria-labelsuffixes for running/stopped/failed.activeTurnInProgressis removed so a settled turn cannot mark live background work as complete.Failed provider-error work entries also stay unfolded when there is no assistant message.
Reviewed by Cursor Bugbot for commit 38109f8. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Keep in-flight and stopped tool rows visible in collapsed work log groups
selectCollapsedWorkLogEntriesin MessagesTimeline.logic.ts to pin executing and stopped entries while showing only the most recent ordinary entries withinMAX_VISIBLE_WORK_LOG_ENTRIESworkEntryHasReportedLifecycle,workEntryIsExecuting, andworkEntryShouldRenderInWorkLogin session-logic.ts to distinguish genuinely running tools from neutral rows with no lifecycle signalSimpleWorkEntryRowin MessagesTimeline.tsx to show a spinner only for running tools, a checkmark for success, an X for failure, and no icon for pending/waiting; stopped state is conveyed via accessible name suffixactiveTurnInProgressprop fromMessagesTimelineandChatView; lifecycle indicators are now driven solely by entry dataderiveTurnFoldskeeps terminal provider error entries visible in settled turns instead of folding them awayworkEntryShouldRenderInWorkLognow hides neutral work log rows that lack any lifecycle signal — any caller expecting those rows to appear in collapsed groups will no longer see themMacroscope summarized 38109f8.