Skip to content

fix(desktop): retain side chats across session switches - #5440

Open
Sun-GLiang wants to merge 3 commits into
apache:mainfrom
Sun-GLiang:fix/side-chat-session-switch-retention
Open

Sun-GLiang wants to merge 3 commits into
apache:mainfrom
Sun-GLiang:fix/side-chat-session-switch-retention

Conversation

@Sun-GLiang

@Sun-GLiang Sun-GLiang commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Keep each Workbar Side Chat mounted and scoped to its source session when the user switches main sessions, so its temporary fork, transcript, and in-flight turn survive navigation.
  • Continue deleting the temporary fork only when the user explicitly closes the Side Chat.
  • Filter durable mode:side_conversation sessions from the task rail to prevent the fork from flashing before the renderer-local hidden-ID update arrives.

Fixes #5441

Verification

  • npm --workspace @maka/desktop run build:with-deps
  • npx tsc -p apps/desktop/tsconfig.renderer.json --noEmit
  • npx tsc -p apps/desktop/tsconfig.main.json --noEmit
  • Targeted biome check over all seven changed files
  • node --test --test-force-exit apps/desktop/dist/main/__tests__/workbar-controller.test.js apps/desktop/dist/main/__tests__/session-navigation-controller.test.js — 39 passed
  • env -u ELECTRON_RUN_AS_NODE npx playwright test --config e2e/playwright.config.ts e2e/session-workbar.spec.ts --grep "Side Chat survives collapse and source switches" — 1 passed in a real Electron window; the test switches sessions immediately after the Side Chat first send starts, returns to the source, and verifies explicit close is the deletion boundary

The change has no static visual delta; the real-window race test is the behavioral UI evidence.

AI use

Select exactly one:

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

Tool(s) and scope: Maka assisted with root-cause analysis, implementation, and tests. The human contributor reviewed and owns the submitted change.

Checklist

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — described under Summary above
  • No

@github-actions github-actions Bot added the effort/M Under 500 readable lines label Sep 17, 2026

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 3b4977f74759079f12bde8721390772a7aeff8ae. The change keeps Side Chat controllers mounted across Session navigation, hides durable Side Chat forks from the main rail, and preserves explicit-close cleanup. One cross-session entry-point regression remains.

Validation included the production Workbar controller path on this head and exact base, build:test, full workspace typecheck, Desktop 2572/2572, focused Workbar/navigation 39/39, lint, format, ASF headers, changed-file Biome, diff check, hosted test/label, and a clean build/test merge tree against current main a2194e239298229de4dbf61b5de3bdd79fad9c53.

Not covered: packaged macOS/Windows Electron or a real provider-backed Side Chat.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

// WorkbarSurface projects only the active Session's tabs, but retaining
// the inactive panels preserves their hook state and prevents an ordinary
// navigation from running the explicit-dismiss cleanup path.
quotes: sideConversations.panels,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Scope the Side Chat toggle to the active Session

After this change, sideConversations.panels retains Session A's panel globally, but toggleTool('side-chat') still scans the unprojected panelsStateRef.current. Open a Side Chat in A, switch to B, then press the documented primary+Alt+S shortcut: the toggle finds A's hidden tab, activates it, and returns. WorkbarSurface filters that tab out for B, so nothing becomes visible and no B Side Chat is created. A production-controller probe on this head leaves the panel sources as ['a']; the same assertion on exact base creates B as expected (['b']). Please filter Side Chat candidates by the active source Session, or fall through to openTool, before returning.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — this is a real regression. toggleTool was searching the retained global Side Chat tabs while WorkbarSurface projected them by source Session, so it could activate a hidden tab and return. Fixed in 87ddd7fc6 by restricting Side Chat toggle candidates to panels whose sourceSessionId matches the active Session, which falls through to opening a new Side Chat when no current-Session candidate exists. Added an A → B controller regression test; the focused Workbar/navigation suites pass 40/40, along with Desktop build:test, renderer typecheck, Biome, and git diff --check.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head 87ddd7fc63c5c395271ff227af51dff444cc846e.

The previous cross-Session shortcut issue is fixed: the shortcut now filters retained Side Chats by the active source Session, and the new regression fails on exact prior head 3b4977f74759079f12bde8721390772a7aeff8ae. One source-retirement lifecycle issue remains (inline).

Validation included a clean Node 24.18.1 install, build:test, full workspace typecheck, Desktop 2574/2574, focused Workbar/retirement 62/62, renderer architecture 112/112 plus the base-relative policy check, lint, format, ASF headers, changed-file Biome, diff check, and the Side Chat Electron journey under Xvfb. Hosted test is green. The clean merge tree 40b7ea767b101347271afbbb10bd216b6654035f against current main 9f706efb9adfa8a404a402188444d76a072ed851 also passed build:test, full typecheck, and the focused 62 tests.

Not covered: packaged macOS/Windows Electron or a real provider-backed Side Chat.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

// the last authoritative source summary for each one so its subscription and
// eventual explicit-close cleanup never lose the source identity merely
// because another Session is currently selected.
const sideChatSourceSessionsRef = useRef(new Map<string, SessionSummary>());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

[P2] Retire retained Side Chats when their source Session disappears

This cache intentionally keeps inactive Side Chats mounted, but nothing reconciles it against the authoritative Session catalog. After creating a Side Chat for Session A, permanently deleting A and refreshing the catalog leaves A's tab and quote record in global Workbar state. The per-Session projection hides it, while positionedTabs still mounts it, so the user has no route to close it and trigger dismissCompanionCopy(). The Host removal path also leaves the durable side-conversation Session present because it is an ordinary parentSessionId branch, not a subagentParent child. I reproduced both halves through the production controller and retirement paths: after authoritative IDs changed from {a,b} to {b}, A's tab/quote remained; after removing A's family, its committed mode:side_conversation Session still probed as present. Reconcile retained Side Chats when their source ID leaves the authoritative catalog and run the explicit-close cleanup before dropping them.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good catch — this is a real lifecycle leak. Fixed in aa33594b1. The controller now reconciles retained Side Chats against authoritativeSessionIds: when a source Session leaves the authoritative catalog, it removes that source’s tabs with visibility preserved, clears any pending close prompt, and drops the quote record without asking for confirmation. Dropping the quote unmounts QuoteCompanionPanel, so the existing dismissal effect runs stop() plus the durable dismissCompanionCopy() cleanup path; ordinary A → B navigation still retains the panel because A remains authoritative. WorkbarSurface also prunes cached source summaries once no retained quote references them. Added a controller regression covering retention while A is authoritative and retirement after the catalog becomes {b}. Validation: retirement/Workbar focused tests 58/58, Side Chat hook suite 68/68, Desktop main/build:test, renderer typecheck, Biome, diff check, and commit hooks all pass.

@Astro-Han Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Side Chat retention across session switches

Checked the failure mode first: the removed useLayoutEffect retired every panel whose sourceSessionId !== activeSessionId, which unmounted QuoteCompanionPanel and ran the dismissal cleanup in use-quote-companion.ts — exactly the fork deletion this PR fixes. Rekeying retirement on catalog membership is the right seam, and it can't loop (use-side-conversation-workspace.ts's remove returns the same state when nothing matches).

What I verified as correct:

  • Fork survival. quotes: sideConversations.panels plus the narrowed projectWorkbarPanelsForSession set keeps the foreign panel mounted-but-hidden (positionedTabs still iterates allowedPanels), the same way terminal tabs of other sessions already stay mounted. Returning to the source re-projects the tab and flips props.active, so the initial-prompt auto-send in quote-companion-panel.tsx resumes instead of being lost.
  • toggleTool scoping. The raw activeTabId can still name a tab the session projection hides, so the shortcut/launcher previously activated another session's hidden side chat. Scoping by activeSessionIdRef fixes that, and falling through to openTool is the right default.
  • Empty-catalog fence. Skipping retirement while authoritativeSessionIds is undefined (revision === 0) avoids mass-retiring panels before the first list lands.
  • Retirement reuses the explicit-close route (closeTabsWithoutConfirmation + removePanels), so a retired fork is stopped and deleted the same way.

1. An archived source session leaves its side chat mounted and unreachable

use-workbar-controller.ts:685-690 retires on !authoritativeSessionIds.has(panel.sourceSessionId), and authoritativeSessionIds is the whole catalog — archived sessions included (session-catalog-state.ts:79-83, selectAuthoritativeSessionIds). The rail hides archived rows, and the new label filter hides the fork row too.

So: open a side chat on A, switch to B, archive A from the rail. A's panel stays mounted with a live fork subscription, its fork session stays alive and is invisible in the rail, and its tab only projects when A is the active session — so the side chat can't be reopened or closed until A is restored from Settings › 活动. Before this PR the switch to B already destroyed the panel, so this state was unreachable.

Is "retain until the source is deleted" the intended boundary, or should archiving a source retire the panel through the same cleanup as an explicit close? Either way, the suite only covers "source leaves the catalog", so a test would pin the decision.

2. Navigation silently cancels a close confirmation

use-workbar-controller.ts:773-775 clears pendingSideChatClose on every activeSessionId change. That was harmless while the panel died on the switch anyway; now the panel survives, so confirming "delete this side chat" and then switching sessions discards the request while the chat and its fork live on. The new test's closeConfirmation.open === false assertion after catalog-driven retirement is the same silent-cancel shape. The pending entry is keyed by tab id, so carrying it across switches would still be applicable.

nits

  • workbar-surface.tsx:420-434 mutates sideChatSourceSessionsRef.current during render (both the set and the delete pass). The rule against writing refs during render exists for interrupted/discarded renders; if an entry were dropped, the retained panel would get sourceSession === undefined, which gates its composer (sessionHasExactModelChoice(undefined, …) is false) and — worse — makes the unmount cleanup at use-quote-companion.ts:1026-1043 take neither branch, since both require sourceSessionId, so the fork would never be deleted. Deriving the retained summaries from props.quotes in a useMemo, or pruning in an effect, removes the class of problem. Related: the delete pass is driven by props.quotes?.map(...), so if quotes is ever undefined the whole map is wiped each render — WorkbarHost always passes it today, but the prop is optional.
  • session-workbar.spec.ts:150-161: waitForCompanionForkId returns "the first session that isn't the source", and the rewritten test now has a second main session in the catalog before the fork exists, so the pick depends on sessions.list() ordering. The trailing .toBe(false) poll does catch a mis-pick (it would still see the other main session), but selecting the fork by parentSessionId/labels would make it deterministic.

Test coverage otherwise reads well: both new controller tests fail without the change (the first pins the retained panel id and tab, the second pins catalog-driven retirement), and the nav test keeps the same expected row list with a labeled fixture session, so it genuinely pins the new exclusion.

@hqhq1025 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewed exact head aa33594b14826bf7e0ada90c1b87f00d380d7fc3. No P0-P3 findings remain.

The follow-up now retires retained Side Chats when their source leaves the authoritative Session catalog. It removes the matching tabs and panel records while preserving Workbar visibility, which unmounts QuoteCompanionPanel and reaches the existing durable fork cleanup path. The source-summary cache is pruned with the retained quotes. Ordinary A → B navigation still preserves the Side Chat because A remains authoritative, and the prior cross-Session shortcut regression remains fixed.

The new controller regression fails on exact prior head 87ddd7fc63c5c395271ff227af51dff444cc846e and passes here. I also exercised the production Electron path under Xvfb: created a real Side Chat fork, switched to another Session, permanently removed the source, and observed both panel unmount and fork removal from the Host catalog.

Validation on Node 24.18.1 included build:test, full workspace typecheck, Desktop 2574/2574, focused Workbar/retirement/navigation 114/114, renderer architecture 112/112, changed-file Biome, and git diff --check. Hosted test is green. The clean merge tree 94c3dc282825c870ab99338048e5d4f552cf99b9 against current main 9f706efb9adfa8a404a402188444d76a072ed851 also passed build:test, full typecheck, and the focused 114 tests.

Not covered: packaged macOS/Windows Electron or a real provider-backed Side Chat.

Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.

@Sun-GLiang

Copy link
Copy Markdown
Contributor Author

@Astro-Han Thanks for the thorough review. I agree that the new retention behavior needs a clearer lifecycle boundary rather than leaving archived-source Side Chats mounted but unreachable.

Would the following direction match your expectations?

  • Treat navigation, Workbar collapse, and tab changes as temporary: retain the Side Chat and its fork.
  • Treat source archival as a lifecycle boundary for this ephemeral UI: if the source has a Side Chat, make the archive flow explicitly tell the user that archiving will close/delete the temporary Side Chat, then stop and clean up the fork. Permanent source deletion would perform the same cleanup without a second Side Chat confirmation because the parent delete confirmation already owns that decision.
  • Keep a pending Side Chat close confirmation across Session navigation, keyed by the stable panel/tab id. Confirming from Session B would still close Session A's Side Chat; source retirement/deletion would supersede and dismiss an obsolete prompt.
  • Move cleanup identity out of the render-mutated WorkbarSurface cache: keep sourceSessionId as durable panel ownership and pass it independently of the optional SessionSummary, so unmount cleanup never depends on a cached summary. The summary/snapshot would only gate presentation and sending, and cache maintenance would no longer mutate refs during render.
  • Make the E2E helper identify the companion semantically via its source relationship plus Side Conversation metadata, rather than selecting the first non-source Session.

I would also pin the archive behavior and the cross-navigation confirmation behavior with controller/journey tests. Does this lifecycle split—especially treating archive as an explicit cleanup boundary with user-facing notice—look right to you, or would you prefer archived Side Chats to be suspended and recoverable when the source is restored?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/M Under 500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Side Chat disappears after switching main sessions

3 participants