fix: stop the busy indicator looping forever after a mid-turn model switch - #125
Conversation
…witch `#teardownProvider` nulls `#activeRun` before draining the event consumer, so the consumer's `finally` skips its idle reset (its `#activeRun === run` ownership guard is now false). Nothing else re-asserts idle, and `#setStatus` is the only path that broadcasts `session.status_change` — so tearing a provider down mid-turn (setModel / rotate) strands `#status` at thinking/tool_running forever and the web "thinking…/drafting…" indicator loops indefinitely. - Daemon: `#teardownProvider` now emits the idle transition itself when it tore down an active run and nothing took over (guards against the onRecoveryNeeded path installing a fresh run, and against the terminal error state). - Web: `WorkerIndicator` gains a defensive staleness cap — if a session claims "thinking" but produces no message/delta/status activity for 90s and no tool is executing, the indicator clears instead of looping, so a missed idle can never strand the UI again. Test: a mid-turn `setModel` now broadcasts `status_change: idle` and lands the session at idle (previously `waitForIdle` timed out). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 27 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughSession teardown now explicitly transitions status to idle when a mid-turn provider teardown leaves no active run and the session isn't in error, replacing implicit finally-path handling. A new integration test verifies this. Separately, WorkerIndicator visibility now hides after a stall timeout unless a tool is actively running. ChangesSession Teardown Idle Status
WorkerIndicator Visibility Update
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Test as Integration Test
participant Session
participant Provider
Test->>Session: setModel(...) mid-turn
Session->>Provider: teardown()
Session->>Session: clear activeRun / eventConsumerTask
Provider-->>Session: teardown complete
Session->>Session: setStatus("idle") if no active run & no error
Session-->>Test: status_change broadcast ("idle")
Possibly related PRs
🚥 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #125 +/- ##
==========================================
+ Coverage 77.10% 77.83% +0.73%
==========================================
Files 89 89
Lines 14412 14385 -27
==========================================
+ Hits 11112 11197 +85
+ Misses 3300 3188 -112
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@web/src/components/transcript/WorkerIndicator.tsx`:
- Around line 39-44: The fallback timing in WorkerIndicator is using local
component time, which makes stale sessions look fresh again on mount or refocus.
Replace the Date.now()-based lastActiveAt state and the createEffect reset with
a per-session activity timestamp sourced from the store/session model, and make
the indicator derive its stale-window from that persisted value instead of
reinitializing on focusedSessionId() or status() changes. Ensure the logic in
WorkerIndicator keeps the existing stale state across navigation/reload rather
than restarting the 90s timer.
🪄 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 Plus
Run ID: ad56bbc6-eb92-4714-ade2-6139817f59c7
📒 Files selected for processing (3)
src/daemon/session.tssrc/tests/session-integration.test.tsweb/src/components/transcript/WorkerIndicator.tsx
…component clock Addresses a CodeRabbit review point: the staleness fallback seeded lastActiveAt from Date.now() and reset it on every focusedSessionId() change, so refocusing (or remounting after navigation) a session that was already stuck restarted the full 90s window — making a stale session look fresh again. Track last live activity in a per-session side map in the messages store (bumped where the epoch bumps, on applyMessage/applyDelta; cleared on clearSessionMessages). The indicator now reads that store-derived timestamp, so it reflects REAL last activity and a finished-but-stranded session clears immediately on refocus instead of showing for another 90s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The bug
The web "thinking…/drafting…" indicator sometimes loops forever after the model's response has finished. Root cause is daemon-side and deterministic: it happens whenever you run
/modelor/rotatewhile a turn is in flight.#teardownProvider(src/daemon/session.ts) nulls#activeRunbefore draining the event consumer, so the consumer'sfinallyskips its idle reset (#activeRun === runis now false).#setStatusis the only thing that broadcastssession.status_change, so#statusstaysthinking/tool_runningforever andsetModeleven re-broadcasts the stale status via#broadcastInfoUpdate. The indicator (WorkerIndicator.tsx, gated purely on status) loops indefinitely.Fix
#teardownProvideremits the idle transition itself when it tore down an active run and nothing took over — guarded against theonRecoveryNeededrecovery-run path and the terminal error state, so it's a no-op fordestroy()and safe for all callers.WorkerIndicatoradds a staleness cap — if a session claimsthinkingbut produces no activity for 90s and no tool is executing, the indicator clears. The daemon fix is the real fix; this guarantees the UI can never loop forever again even if a status update is ever missed.Test
src/tests/session-integration.test.ts— a mid-turnsetModelnow broadcastsstatus_change: idleand the session lands at idle. Without the daemon fix,waitForIdletimes out.Full daemon suite green (905), web typecheck + lint clean.
🤖 Generated with Claude Code
Summary by CodeRabbit