Repository navigation
fix(status-bar): invalidate the CLI session count on kill and restart - #12468
Conversation
`pty:management:killOne` / `killAll` / `restart` tear sessions down via `adapter.shutdown()` and broadcast nothing — unlike `pty:kill`, which ends in `sendPtyExitToRenderer`. The status-bar count is an event-sourced cache, so killing sessions from Manage Sessions or "Kill all terminals" left the `>_ N` chip frozen until the popover was opened, which itself triggers a refresh. > [!NOTE] > The dual-source split described in the issue text was already fixed by merged #9387. This closes a *different* remaining invalidation gap that produces the same reported symptom. Broadcast the teardown so the chip updates without needing the popover opened. Fixes #8372 Co-authored-by: Orca <help@stably.ai>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdded a listener-based daemon session inventory invalidation mechanism. The session inventory hook refreshes after invalidation when restore readiness is complete and unsubscribes after unmount. Daemon restart, individual session kill, and kill-all operations now emit invalidation events. Kill-all continues cleanup when notification fails. Added unit, reliability, and end-to-end coverage for refresh timing and status-bar session counts. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
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 |
Fails on origin/main, passes on this branch. Test: drops after Manage Sessions kills a foreign daemon session, popover never opened Co-authored-by: Orca <help@stably.ai>
|
Important Correction to this PR's original description. While building the e2e proof I measured the claim and it was wrong. I originally wrote that The gap that does regress is a live daemon session this main process is not attached to — another Orca client, So: the fix is correct and worth landing, but the mechanism in my original description overstated the blast radius. The user-visible symptom in #8372 (stale chip, correct number once the popover is opened) still matches. Unrelated issue found while stabilising the spec, worth someone's attention: navigating to Settings immediately after a pane split sometimes makes the app kill its own pane PTYs — the daemon log shows the app's client id issuing |
Visual proofRecorded with 12468-status-bar-session-count-management-kill.spec.ts.webmWhat you are looking atWhat a reviewer sees in the recording:
… It actually captures the bugSame spec on Test: Caveats and setup notes from building this proofHonest limitations and setup facts the lead needs:
… |
…stablyai#12468) * fix(status-bar): invalidate the CLI session count on kill and restart `pty:management:killOne` / `killAll` / `restart` tear sessions down via `adapter.shutdown()` and broadcast nothing — unlike `pty:kill`, which ends in `sendPtyExitToRenderer`. The status-bar count is an event-sourced cache, so killing sessions from Manage Sessions or "Kill all terminals" left the `>_ N` chip frozen until the popover was opened, which itself triggers a refresh. > [!NOTE] > The dual-source split described in the issue text was already fixed by merged stablyai#9387. This closes a *different* remaining invalidation gap that produces the same reported symptom. Broadcast the teardown so the chip updates without needing the popover opened. Fixes stablyai#8372 Co-authored-by: Orca <help@stably.ai> * test(e2e): add recordable proof for status-bar-cli-session-count Fails on origin/main, passes on this branch. Test: drops after Manage Sessions kills a foreign daemon session, popover never opened Co-authored-by: Orca <help@stably.ai> * fix(status-bar): avoid duplicate inventory refresh after kill all --------- Co-authored-by: Orca <help@stably.ai>
pty:management:killOne/killAll/restarttear sessions down viaadapter.shutdown()and broadcast nothing — unlikepty:kill, which ends insendPtyExitToRenderer. The status-bar count is an event-sourced cache, so killing sessions from Manage Sessions or "Kill all terminals" left the>_ Nchip frozen until the popover was opened, which itself triggers a refresh.Note
The dual-source split described in the issue text was already fixed by merged #9387. This closes a different remaining invalidation gap that produces the same reported symptom.
Fix
Broadcast the teardown so the chip updates without needing the popover opened.
Verification
pnpm typecheckclean on this branchFixes #8372
Made with Orca 🐋