fix(tui): recover activity status when no runs are in flight#64847
fix(tui): recover activity status when no runs are in flight#64847Yanhu007 wants to merge 1 commit intoopenclaw:mainfrom
Conversation
The TUI status bar can get stuck on "streaming" indefinitely after a
chat turn completes. This happens when `state.activeChatRunId` is
cleared by a concurrent event before the "final" event arrives, making
`wasActiveRun` false in `finalizeRun` and skipping the
`setActivityStatus("idle")` call.
Add a recovery condition: when `wasActiveRun` is false but there are
provably no other runs in flight (`sessionRuns.size === 0` and
`activeChatRunId` is null), still transition the activity status. This
is safe because it only fires when the state machine has confirmed no
other work is active.
Apply the same fix to `terminateRun` for consistency.
Fixes openclaw#64825
Greptile SummaryAdds a recovery condition to Confidence Score: 5/5Safe to merge — the recovery condition is well-guarded and cannot interfere with in-flight concurrent runs. Only finding is a P2 suggestion to add a regression test for the new recovery path; no logic errors or data-integrity concerns. No files require special attention. Prompt To Fix All With AIThis is a comment left during a code review.
Path: src/tui/tui-event-handlers.ts
Line: 145-147
Comment:
**Missing regression test for the new recovery path**
The existing test suite has good coverage for the concurrent-run guard (the `run-other` + `run-active` scenario at line 505), but there's no test for the new `wasActiveRun === false` + `sessionRuns.size === 0` + `activeChatRunId === null` recovery case that this PR is fixing. Without it, a future refactor could silently revert the fix and the suite would still pass.
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(tui): recover activity status when n..." | Re-trigger Greptile |
| if (params.wasActiveRun || (sessionRuns.size === 0 && !state.activeChatRunId)) { | ||
| setActivityStatus(params.status); | ||
| } |
There was a problem hiding this comment.
The existing test suite has good coverage for the concurrent-run guard (the run-other + run-active scenario at line 505), but there's no test for the new wasActiveRun === false + sessionRuns.size === 0 + activeChatRunId === null recovery case that this PR is fixing. Without it, a future refactor could silently revert the fix and the suite would still pass.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/tui/tui-event-handlers.ts
Line: 145-147
Comment:
**Missing regression test for the new recovery path**
The existing test suite has good coverage for the concurrent-run guard (the `run-other` + `run-active` scenario at line 505), but there's no test for the new `wasActiveRun === false` + `sessionRuns.size === 0` + `activeChatRunId === null` recovery case that this PR is fixing. Without it, a future refactor could silently revert the fix and the suite would still pass.
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8f47aacb35
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| clearActiveRunIfMatch(params.runId); | ||
| flushPendingHistoryRefreshIfIdle(); | ||
| if (params.wasActiveRun) { | ||
| if (params.wasActiveRun || (sessionRuns.size === 0 && !state.activeChatRunId)) { |
There was a problem hiding this comment.
Don't let background BTW errors override activity status
The new fallback in terminateRun updates activity even when the run was never active, which regresses local /btw behavior: those runs are intentionally excluded from activeChatRunId (src/tui/tui-event-handlers.ts:240) and the send path intentionally avoids status changes for BTW (src/tui/tui-command-handlers.ts:525-554). With this change, if a local BTW run later emits error/aborted and no other run is in flight, the status bar is set to error/aborted, making a side-result failure appear as the main chat state.
Useful? React with 👍 / 👎.
|
Closing this PR because the author has more than 10 active PRs in this repo. Please reduce the active PR queue and reopen or resubmit once it is back under the limit. You can close your own PRs to get back under the limit. |
Summary
The TUI status bar can get stuck on
streamingindefinitely after a chat turn completes. This happens whenstate.activeChatRunIdis cleared by a concurrent event before thefinalevent arrives, causingwasActiveRunto befalseinfinalizeRun— thesetActivityStatus("idle")call is skipped and the spinner never stops.Changes
Add a recovery condition to both
finalizeRunandterminateRun: whenwasActiveRunis false but there are provably no other runs in flight (sessionRuns.size === 0andactiveChatRunIdis null), still transition the activity status.This is safe because it only fires when the state machine has confirmed no other work is active — it cannot interfere with legitimate in-flight runs.
Testing
wasActiveRun === true→ unchanged behaviorwasActiveRun === false, no runs in flight → status recovers to idlesessionRuns.size > 0→ no premature status changeFixes #64825