fix(desktop): retain side chats across session switches - #5440
Sun-GLiang wants to merge 3 commits into
Conversation
Generated-by: Maka
hqhq1025
left a comment
There was a problem hiding this comment.
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, |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
Generated-by: Maka
hqhq1025
left a comment
There was a problem hiding this comment.
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>()); |
There was a problem hiding this comment.
[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.
There was a problem hiding this comment.
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.
Generated-by: Maka
Astro-Han
left a comment
There was a problem hiding this comment.
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.panelsplus the narrowedprojectWorkbarPanelsForSessionset keeps the foreign panel mounted-but-hidden (positionedTabsstill iteratesallowedPanels), the same way terminal tabs of other sessions already stay mounted. Returning to the source re-projects the tab and flipsprops.active, so the initial-prompt auto-send inquote-companion-panel.tsxresumes instead of being lost. toggleToolscoping. The rawactiveTabIdcan still name a tab the session projection hides, so the shortcut/launcher previously activated another session's hidden side chat. Scoping byactiveSessionIdReffixes that, and falling through toopenToolis the right default.- Empty-catalog fence. Skipping retirement while
authoritativeSessionIdsisundefined(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-434mutatessideChatSourceSessionsRef.currentduring render (both thesetand the delete pass). The rule against writing refs during render exists for interrupted/discarded renders; if an entry were dropped, the retained panel would getsourceSession === undefined, which gates its composer (sessionHasExactModelChoice(undefined, …)is false) and — worse — makes the unmount cleanup atuse-quote-companion.ts:1026-1043take neither branch, since both requiresourceSessionId, so the fork would never be deleted. Deriving the retained summaries fromprops.quotesin auseMemo, or pruning in an effect, removes the class of problem. Related: the delete pass is driven byprops.quotes?.map(...), so ifquotesis everundefinedthe whole map is wiped each render —WorkbarHostalways passes it today, but the prop is optional.session-workbar.spec.ts:150-161:waitForCompanionForkIdreturns "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 onsessions.list()ordering. The trailing.toBe(false)poll does catch a mis-pick (it would still see the other main session), but selecting the fork byparentSessionId/labelswould 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
left a comment
There was a problem hiding this comment.
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.
|
@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?
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? |
Summary
mode:side_conversationsessions 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-depsnpx tsc -p apps/desktop/tsconfig.renderer.json --noEmitnpx tsc -p apps/desktop/tsconfig.main.json --noEmitbiome checkover all seven changed filesnode --test --test-force-exit apps/desktop/dist/main/__tests__/workbar-controller.test.js apps/desktop/dist/main/__tests__/session-navigation-controller.test.js— 39 passedenv -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 boundaryThe change has no static visual delta; the real-window race test is the behavioral UI evidence.
AI use
Select exactly one:
Tool(s) and scope: Maka assisted with root-cause analysis, implementation, and tests. The human contributor reviewed and owns the submitted change.
Checklist
Does this PR entail a change in behavior?