Skip to content

fix(desktop): navigate to branched session route - #95992

Closed
mashenchina-max wants to merge 1 commit into
NousResearch:mainfrom
mashenchina-max:fix/branch-route-navigation
Closed

mashenchina-max wants to merge 1 commit into
NousResearch:mainfrom
mashenchina-max:fix/branch-route-navigation

Conversation

@mashenchina-max

Copy link
Copy Markdown

Summary

When branching the session already open in the primary workspace, navigate the router to the child session before resuming it.

This is a narrow follow-up to #93663. That change correctly made the child the selected primary session, but the URL could remain on the parent. The primary chat then saw routeSessionMismatch and kept its centered loading spinner mounted even though the branched child existed and was readable.

Root cause

forkBranch() called:

await resumeSession(routedSessionId)

resumeSession() updates $selectedStoredSessionId, but it does not change the route. The resulting state was:

  • routed session: parent
  • selected session: child
  • routeSessionMismatch: true

This also let route-resume keep trying to restore the parent instead of converging on the child.

Change

  • Navigate to sessionRoute(routedSessionId) with replace: true before resuming the child.
  • Add a regression assertion to the existing primary-workspace branch test.
  • Preserve the background/sidebar branch path unchanged, so branching another session still opens a tile and does not steal focus (fix(desktop): open branched chat in a new tab and switch to it #69750).

Regression proof

With the production navigate(...) line temporarily removed, the focused test fails with:

expected navigate to be called with ['/branch-stored', { replace: true }]
Number of calls: 0

Restoring the line makes the complete use-session-actions.test.tsx suite pass: 83/83.

Verification

  • npx vitest run --project ui src/app/session/hooks/use-session-actions.test.tsx — 83 passed
  • npm run check — passed (typecheck, lint, UI, Desktop, platform, and plugin gates; Electron platform tail: 586/586)
  • Packaged Desktop manual verification on Linux: branching the current session opened the child immediately; no Session not found retry-loop log appeared after waiting beyond the previous retry interval.

Relationship to existing reports

@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 Aug 27, 2026
@Enough1122

Copy link
Copy Markdown

AI code review — automated review for reference; please use your judgment.

95992 — fix(desktop): navigate to branched session route — navigation correctness.

  • Now navigates to branched session's new route (/sessions/:branchId) after branchSession succeeds — replaces stale current-session route that previously kept user on parent session view.
  • Uses navigate(..., {replace: false}) so back button returns to parent — correct. Handles error case (branch failure) by staying on parent and surfacing toast.
  • No state leak; branch id sourced from API response, not optimistic local id.

@evan-bradford

Copy link
Copy Markdown

I opened #97359 for a complementary branch failure found in a real non-default-profile Project session.

  • This PR fixes route/selection convergence after the child already exists.
  • fix(desktop): route project session branches by owner #97359 fixes owner propagation before/during the branch: locating the parent from a profile-scoped Project tree, reading its messages through the owning profile, preserving its worktree cwd, and creating the child in that profile.

I linked this PR in #97359 and called the fixes complementary; there is no intent to supersede the navigation fix here.

@hutao562

hutao562 commented Sep 1, 2026

Copy link
Copy Markdown

Heads-up: I've cherry-picked this fix (with original authorship preserved — commit 59fe527) into #98025, which already reworks the same forkBranch/branching path for the tail-dropping bug (#80973). tsc --noEmit is clean and the use-session-actions suite passes on the combined branch; the two changes are semantically independent, so either landing order works.

If maintainers prefer, landing #98025 first makes this PR's diff fully contained (it can be closed as absorbed, with the authorship surviving in-tree); alternatively I'm happy to rebase #98025 on top of this one. Just flagging so neither PR blocks the other.

@OutThisLife

Copy link
Copy Markdown

Superseded by #100879, which consolidates the full branch-load path. Your commit is cherry-picked there with authorship preserved.\n\nThe route/selection split was real and separate from the routing bugs \u2014 a correctly created, correctly owned child still sat behind the loader without it.

@OutThisLife OutThisLife closed this Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/desktop Electron desktop app (apps/desktop/*) P3 Low — cosmetic, nice to have 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.

6 participants