Skip to content

fix(desktop): prevent stored-id rotations from stealing session focus - #66122

Closed
UnathiCodex wants to merge 1 commit into
NousResearch:mainfrom
UnathiCodex:fix/desktop-session-selection-race
Closed

UnathiCodex wants to merge 1 commit into
NousResearch:mainfrom
UnathiCodex:fix/desktop-session-selection-race

Conversation

@UnathiCodex

Copy link
Copy Markdown
Contributor

Summary

  • carry the runtime id and previous/next stored ids in the auto-compression rotation event
  • follow a rotation only while that exact runtime, selected session, and route intent still own the foreground
  • preserve overlays/pages by updating their underlying selected chat without navigating away
  • remove the obsolete stored-to-runtime reverse mapping as soon as the cache rotates
  • consume stale rotation events so they cannot replay after a later remount

Reproduction

  1. Keep session A active while it reaches auto-compression.
  2. Navigate from A to B and then C while A's delayed session.info reports its continuation stored_session_id.
  3. The rotation effect introduced in fix(desktop): follow compression's stored-id rotation to prevent thread reload #65984 sees only the next stored id and unconditionally calls navigate with replace.
  4. Both the sidebar selection and transcript jump back to A, overwriting the newer user navigation.

Why this fixes the race

The rotation signal now carries enough provenance to prove that it still belongs to the foreground. The consumer rejects it when either the runtime, selected stored id, or a newer routed session no longer matches. A route overlay has no routed session id, so its underlying selection follows compression without closing the overlay.

Tests

  • npx vitest run src/app/session/hooks/use-session-actions.test.tsx src/app/session/hooks/use-session-state-cache.test.tsx - 32 passed
  • npm run typecheck
  • ESLint on all six changed files

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/desktop Electron desktop app (apps/desktop/*) sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state labels Jul 17, 2026

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Comment

Medium desktop feature fix (95 additions / 101 deletions). Refactors session serialization and routing logic. The getRouteToken changes are straightforward function hoisting with consistent return values.

No security issues. No hardcoded credentials or debug artifacts. Well-scoped refactor.


Reviewed by Hermes Agent (cron batch 2026-07-17)

@tonydwb tonydwb left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review Summary

Verdict: Approved

PR #66122 fix(desktop): prevent stored-id rotations from stealing session focus

Assessment

  • Correctness: Stored session IDs were rotating unexpectedly, causing session focus to switch away from the intended session. The fix adds $activeSessionStoredIdRotation state management to track and prevent unintended rotations. Well-scoped fix.
  • Scope: ~279 additions, 34 deletions. Test file updated with StoredIdRotationHarness.
  • Testing: Tests cover the rotation prevention behavior with dedicated test harness.
  • No issues found

Reviewed by Hermes Agent

@UnathiCodex

Copy link
Copy Markdown
Contributor Author

Could this be reclassified from P3 to P2? This is a functional session-routing correctness bug, not cosmetic polish. A delayed auto-compression rotation from session A can overwrite the user's newer navigation to B or C, switching both the sidebar selection and transcript back to the wrong conversation. The user can recover by reopening the intended session, so P2 ("degraded but workaround exists") fits; P3 ("cosmetic") does not.
Could this be reclassified from P3 to P2? This is a functional session-routing correctness bug, not cosmetic polish. A delayed auto-compression rotation from session A can overwrite the user's newer navigation to B or C, switching both the sidebar selection and transcript back to the wrong conversation. The user can recover by reopening the intended session, so P2 ("degraded but workaround exists") fits; P3 ("cosmetic") does not.

@austinpickett

Copy link
Copy Markdown
Collaborator

Review — Hermes Agent

Verdict: solid fix, but the branch conflicts with main — superseded by a rebase.

The diagnosis and fix are correct. Carrying { previousStoredSessionId, nextStoredSessionId, runtimeSessionId } in the rotation signal (instead of a bare next-id) is exactly the right shape — it lets the route-follow effect prove the event still belongs to the foreground conversation and reject a delayed rotation from a background session. Consuming the event even when stale (so it can't replay after a remount) and updating the underlying selection in place for overlay/page surfaces are both nice touches. Tests cover the four cases well.

Why it can't merge as-is: this branch is CONFLICTING / DIRTY against main. The original PR emits the rotation from use-session-state-cache.ts's ensureSessionState, but on current main that emit logic has since moved into handleTransition in store/session-states.ts (the $sessionStates refactor, #66454). Cherry-picking onto main conflicts there.

I've rebased your work onto current main in #67118, re-homing the emit to handleTransition and keeping your consumer logic + all four tests verbatim. Verified on the rebased branch:

  • vitest --project ui on both test files — 32 passed
  • npm run typecheck — clean
  • eslint on all five changed files — clean

Your authorship is preserved on the commit and credited via Co-authored-by. Closing this in favor of #67118. Thanks for the clean fix and the thorough reproduction, @UnathiCodex.

@teknium1

Copy link
Copy Markdown
Collaborator

Thanks for the focused race analysis. The underlying defect remains on current main: apps/desktop/src/store/session-states.ts:111-116 emits only a next stored id, and apps/desktop/src/app/session/hooks/use-session-actions/index.ts:187-206 unconditionally replaces the route when it receives one.

Problems

  • The original producer location is obsolete after a75a8eda7: current main emits the transition centrally from handleTransition in apps/desktop/src/store/session-states.ts:111-119, rather than from the cache hook. This branch therefore cannot apply directly to current main.

Suggested changes

Automated hermes-sweeper review.

@teknium1 teknium1 added sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform area/sessions Session lifecycle, resume, persistence, history labels Jul 18, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/sessions Session lifecycle, resume, persistence, history comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-session-state Sweeper risk: may lose/corrupt/mis-associate session or context state type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants