feat(desktop): preserve Side Conversations across linked-session navigation

[testikun]

Summary

• Preserve temporary Side Conversations while navigating between a source Session and linked Sub Agent descendants.
• Keep original source ownership, mounted panel state, drafts, transcripts, and running Turns intact, then restore the same tabs on return.
• Retain cleanup for explicit close, source removal, and unrelated Sessions.
• Keep the Workbar surface key stable when linked Side Conversation panels open or close, so other tools do not remount.
• Isolate Review and Artifact-derived state by sessionId during linked navigation.

Closes #4494

Current-head real App verification (2026-09-04)

Built and launched commit 80a53b4 in a visible local Electron window, selected real transcript content in the parent task, opened Side Conversation, and navigated through the linked Implementation child control. The Side Conversation and staged quote remained mounted on the child task. These are direct screenshots from the running Maka App; they are not mockups or generated images.

1. Parent task — Side Conversation opened with selected transcript content staged

2. Linked child task — the same Side Conversation and staged content remain mounted

Verification

• rebased the current PR branch onto current apache/main (fcfa055)
• full workspace and Desktop production build — passed
• Playwright test discovery and TypeScript transformation — passed
• real Electron parent → Side Conversation → linked child journey — passed
• the linked child E2E locator now targets the semantic button exposed by the latest main UI
• Biome check on the changed E2E — passed
• git diff --check — passed

AI use

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

Tool(s) and scope: OpenAI Codex reviewed and implemented the focused Desktop Workbar lifecycle change, added regression coverage, rebased the branch onto the latest main branch, ran the real Electron acceptance path, and prepared the commits.

Checklist

• Tests cover the change and fail without it
• Lint, format, typecheck and the affected suites pass locally
• UI changes include real post-change App screenshots in this first PR description

Does this PR entail a change in behavior?

• Yes — described under Summary above
• No

[Astro-Han]

Why this PR, and is it a fix?

Starting here because it changes what the rest means. #4494 says it plainly: "This is consistent with the current temporary lifecycle rather than an implementation regression: the Desktop controller removes panels whose sourceSessionId differs from the active Session, and the Side Conversation documentation specifies cleanup when the owning Session changes." The pre-PR predicate was exactly that, panel.sourceSessionId !== activeSessionId.

So this is a deliberate lifecycle change, and the issue itself is titled feat(desktop) while this PR is titled fix(desktop). Please move it to feat. It matters beyond bookkeeping: a fix is judged against a broken contract, a feature against whether the new contract is the one you want, and the findings below are mostly of the second kind.

Whether the new behavior is worth having I think is yes. Losing a running Turn and a typed draft because you clicked a Sub Agent to check on it is a real cost, and there is no way to get it back. But that is a product call, and it should be made knowing that what ships is asymmetric (F3) and moves a surface-wide mounting boundary (F1).

What the change gets right, since it is not obvious from the diff: the predicate replaces the old predicate in place with no parallel cleanup path and no mirrored state, retention is derived per render from props rather than cached anywhere, so there is no "when does this invalidate" class of bug at all. openSideChatWithQuote still guards panel.sourceSessionId === sourceSessionId, so quoting from the child correctly makes a child-owned panel. And the sourceSessionIdRef change from sourceSession?.id to the immutable prop closes a pre-existing leak the body does not mention: once the source row left the catalog, both dismissCompanionCopy and abandonPendingCompanionCopy used to skip, stranding the fork on the Host permanently.

[P2] The surface remount boundary moved for every Workbar tool, and the written invariant still says otherwise

workbar-host.tsx:184 is now key={props.surfaceKey ?? props.activeId}, and use-workbar-controller.ts:689 returns the family root whenever a panel is retained. So navigating within a family no longer remounts WorkbarSurface; sessionId changes in place for WorkbarPanels keyed by globally-persisted tab ids.

I checked the consumers rather than assuming. Most do guard: useSessionTodo, ArtifactPane (which even self-checks with recordsSessionId), BrowserPanel and SessionInspectorPanel all key their effects on sessionId and swap in place. But session-review-panel.tsx:59 holds gitResult in state that is never cleared on a sessionId change. The effect re-runs load(), and revisionRef orders the responses, but setGitResult only fires when the new read returns, so until then the Review tab renders the previous Session's diff. visibleFileCount carries the previous pagination across too. workbar-surface.tsx:705's artifactCount has the same shape.

Bounded and self-correcting, hence P2 rather than higher. The part that outlasts it is the contract: README.md:25 still reads "the session content surface is remounted when the active session changes", and the PR edits the Side Chat bullet at line 68 without touching it. Every tool written against the "Adding a tool" section will assume a guarantee that no longer holds.

Two ways out, in preference order. Keep key={activeId} and give Side Chat panels their own mount scope outside the session-keyed surface, so no shared boundary moves. Or keep the key change, update README.md:25 to state the new boundary, and clear gitResult and artifactCount on sessionId change.

[P2] Opening or closing a Side Conversation on a Sub Agent remounts everything else

The key is conditional (if (activeSideConversationPanels.length === 0) return activeSessionId;), so on a linked child it flips child → parent when the first panel opens and parent → child when the last one closes. WorkbarSurface unmounts both times, taking the other tabs with it: session-terminal-panel.tsx:209 disposes the xterm on unmount, so the terminal rehydrates and loses scrollback, and Review, Artifacts and Inspector reload and reset.

Before the change, key={activeId} was constant within a session and opening a side chat never remounted anything.

The comment at side-conversation-session-family.ts:47-50 says this design "keeps the mounted Workbar surface stable when one of several retained panels is closed", which is true for the non-last close that the third new test covers, and false for the first open and the last close, which is the common single-panel case. Making the key unconditional removes the oscillation, at which point F1 above is the whole story and the two need resolving together.

[P2] Retention is one-directional, and the README says otherwise

reachesSession walks up from the active session, so a panel survives only when its source is an ancestor. Start a Side Conversation on a Sub Agent, step up to the parent to check something, come back, and it is gone. The PR's own test pins this as intended.

That is the same loss #4494 complains about, in mirror image, with no warning. And README.md:68 says the panel "survives navigation within its linked Session family", which describes symmetric membership.

Whether #4494 wants symmetry is genuinely ambiguous. But the doc and the code disagree either way, and the symmetric version is also smaller: familyRootId(active) === familyRootId(sourceSession) deletes reachesSession entirely, covers sibling Sub Agents, and makes that README sentence true. If the asymmetry is deliberate, say so in both places.

[P2] A pending session view defeats retention on the navigation that needs it most

isLinkedSideConversationSessionFamily has a pending fallback for the source but not for the active session:

if (sourceSessionId === activeSession.id) return true;
const sourceSession = sessions.find((session) => session.id === sourceSessionId);
if (!sourceSession) return false;

pendingSessionView returns a placeholder with no subagent/subagentParent fields, and its own comment says it covers every active id without a loaded summary, not just freshly created tasks. So navigating into a linked child whose catalog row has not landed yet, for example agent-graph-panel.tsx:501's onOpenSession(operator.childSessionId) firing from live turn data ahead of sessions:changed, gives an active session with no parent link, reachesSession returns false immediately, and the synchronous layout effect closes the tab and dismisses the Host fork.

The scenario this PR exists to fix, losing a Side Conversation with content in it, still happens on that path, and it goes through the destruction path that does not show the closeTabs confirmation. Symmetric fallback covers it: if (!sessions.some((session) => session.id === activeSession.id)) return true;

[P3] Three smaller ones

A second lineage walker, and it disagrees with the session rail. reachesSession walks raw linkedSubagentParentSessionId, while the rail uses projectRevisionLinkedSessionTree (session-revisions.ts:95), which collapses revisions and aliases physical ids to the representative row, with a comment saying it exists so edit-and-resend cannot orphan a child. After edit-and-resend on parent A produces A', the rail shows B under A', but reachesSession(B, A') walks to A and stops. The family the user sees and the family retention computes have diverged. core/session.ts:632's projectLinkedSessionTree also already has the cycle guard this file re-implements.

A retained tab lends its ordinal to a new panel. In openSideChatWithQuote, activeTab?.ordinal ?? reserveOrdinal('side-chat') reuses the ordinal of the preferred side-chat tab, but activePanel additionally requires panel.sourceSessionId === sourceSessionId. On child B with a retained A-owned panel, activeTab matches A's tab while activePanel does not, so a new B-owned panel opens on A's ordinal and two tabs share a number until the first prompt renames one. Only reuse the ordinal when activePanel matched.

Render-hot catalog dependency. input.sessions joins the stale-panel layout effect and two memos, and it is a fresh array identity per catalog revision, which streams continuously during a Turn. isLinkedSideConversationSessionFamily builds new Map(sessions.map(...)) per panel per call, and linkedSideConversationFamilyRootId builds another. Note authoritativeSessionIds next door uses a custom sessionIdSetsEqual comparator specifically to avoid this churn. Build the map once in the controller, or depend on the family root id string.

Also: sessions and surfaceKey are threaded as new optional props through two components to do one find, when the controller already has both in hand; and surfaceKey?: plus ?? props.activeId exists for a producer that always supplies it.

On the tests

The first new test is a real pin, it fails on old code and runs through useWorkbarController rather than a fixture. Two of the five pass on old code as well, so they are guards rather than pins, and the checklist's "fail without it" is true of one. More usefully, nothing exercises workbar-host.tsx:184 where the key is applied or workbar-surface.tsx:850 where the source is resolved, so the obligation #4494 actually states, that the draft and running Turn survive and the fork is not removed, is inferred from a returned string. One test rendering WorkbarHost across parent → child → parent and asserting the side-chat service received no fork removal would cover it.

Next step

The title, then the four P2s. F1 and F2 are entangled and should be resolved in one pass; F3 is a product call on whether retention is symmetric; F4 is a small symmetric fallback.

Manual acceptance, since this is user-visible with no Storybook or Playwright coverage and no screenshots:

1. Parent, open a Side Conversation, type a draft, start a Turn, navigate to a linked Sub Agent, wait, come back. Draft, transcript and Turn all intact, fork never appeared in the rail.
2. With it retained, open the Review tab on the parent, then navigate to the child. Confirm Review does not show the parent's diff while the child is selected.
3. On a Sub Agent with a Terminal attached, open a Side Conversation and close it. Confirm the terminal does not detach and its scrollback survives.
4. On a Sub Agent, open a Side Conversation, go to the parent, come back. Decide whether losing it is the product behavior you want.
5. Confirm the retained tab's header shows the source Session's name while the child is active.

Renderer and main typecheck should also complete once the baseline session-collaboration errors clear; your own report says it was never green for the changed files.

Evidence boundary: read at 4df1c2d against merge base 15e4b6b9e, no build, no test run, no Desktop launched, so nothing here is execution-verified. The remount sequence is derived from React key semantics plus the WorkbarPanel key={tab.id} reuse. The pending-view finding rests on pendingSessionView's stated semantics and the agent-graph entry point; I did not observe that frame, and if every entry into a linked child guarantees the catalog row has landed, it drops to P3. The claim that projectLinkedSessionTree has no consumers outside core's tests is from a repo-wide grep excluding node_modules and dist.

AI-assisted review: drafted with Maka.

[Astro-Han]

Re-reviewed at 7593446. Most of the previous round landed; one P2 remains, and it is the one that decides whether the fix works on the path it was written for.

What is resolved, so it does not get re-litigated. The title is feat. The surface remount boundary is documented at its new place (README.md:23-26) and the two tools that carried session state across an in-place sessionId swap now gate it (session-review-panel.tsx:60/118, workbar-surface.tsx:706/746). I checked the other consumers mounted under that surface rather than assuming: use-session-todo.ts:90, browser-panel.tsx:92-94 and session-terminal-panel.tsx:210-230 all reset on a sessionId change, and terminal tabs carry tab.ownerSessionId, so B was a workable choice. The surface key is unconditional now, so the open/close oscillation is gone. The asymmetry of retention is stated in README.md:69-71 and pinned by a test. The ordinal reuse is fixed at use-workbar-controller.ts:428.

One thing worth stating in the merge description: making the key unconditional moved the mount boundary for every linked-family navigation, not only for sessions that have a Side Conversation open. The README now describes that, and the consumer audit above supports it, but it is broader than the title suggests.

P2 and the two P3s are inline. Three P3s from the previous round are still open and were not answered: the second lineage walker that disagrees with the session rail (session-revisions.ts:95), the render-hot catalog dependency, and surfaceKey?: / sessions?: threaded as optional props for a producer that always supplies them. They are noted inline where they have a line.

The test file did not change this round, so the earlier coverage gap stands: nothing exercises workbar-host.tsx:184 where the key is applied, and the obligation #4494 actually states, that the draft and the running Turn survive and the fork is not removed, is still inferred from a returned string. One test rendering WorkbarHost across parent to child to parent and asserting the side-chat service received no fork removal would close it.

Mergeability: the branch conflicts with main only in apps/desktop/renderer-architecture.json. Regenerate it rather than hand-resolving.

Evidence boundary: read at 7593446 against merge base cd4aa3d, no build, no test run, no Desktop launched. The remount and fork-removal chain below is derived from React key semantics plus the unmount effect at use-quote-companion.ts:734-760; I did not observe that frame.

AI-assisted review: drafted with Maka.

[Astro-Han]

Re-reviewed at d133ea5. The lineage walker now matches the rail, the map churn is gone, the sourceSessionId prop fix is exactly right, and the pending-child test is a real pin. What remains is one regression the new controller cache introduces and the coverage question from the last two rounds.

P2: the last-known-family substitution keeps a tab whose fork is already gone. startNewSession sets activeId to undefined (session-workspace-actions.ts:212-218). use-workbar-controller.ts:195-207 then treats that as "not cataloged", substitutes the previous session and catalog, and isLinkedSideConversationSessionFamily returns at line 87 because source equals the substituted active. So the stale-panel effect keeps the tab. At the same time workbar-host.tsx:175 stops rendering the surface, QuoteCompanionPanel unmounts, and use-quote-companion.ts:765-791 dismisses the fork on the Host. On main the same navigation closes the tab cleanly. Smallest fix: only substitute when input.activeSession is defined, and add a controller case for input(undefined, ...) asserting the panel and tab are removed.

P2: the controller substitution undoes the narrowing the helper claims. side-conversation-session-family.ts:91 says the helper must not retain every panel for an unrelated unknown Session, but the controller never lets an unknown active reach it: any uncataloged active id is replaced by the last known one, so every panel is retained and surfaceKey freezes on the old family root. That contradicts #4494's "navigating to an unrelated Session may keep the current temporary cleanup behavior", and nothing tests the unknown-and-unrelated case. Same root cause as the first finding; I would fix them together.

P2: the new E2E cannot fail on the regression it names. session-workbar.spec.ts:495-529 asserts .maka-quote-companion is visible and the staged quote text is unchanged, but both come from controller props (quotes: activeSideConversationPanels), so a remounted panel with a dismissed fork renders identically. The obligation from #4494, that the draft and the running Turn survive and the fork is not removed, is still inferred. Type a draft into the side-chat input before navigating and assert it after, or replace this Electron case with a WorkbarHost render test asserting the side-chat service received no fork removal across parent → child → parent. The latter also covers workbar-host.tsx:184, which no test reaches.

P3s, unchanged or new: sideConversationSurfaceKeyRef (use-workbar-controller.ts:731-741) has an unreachable fallback branch once lastKnownFamilySessionRef exists, so it can go; surfaceKey?: / sessions?: are still optional for a producer that always supplies them; the surface key is the ancestor family's representative id, which compareFreshness can flip after an edit-and-resend, so sessionRevisionFamilyId(root) would be the stable choice.

renderer-architecture.json moved 15588 → 15586, a real reduction, no budget was raised.

Manual checks still owed: (1) side chat with a draft → new task → back; (2) side chat → enter a child via the agent-graph panel before its catalog row lands → draft and running Turn intact; (3) terminal on a child survives opening/closing a side chat on the parent; (4) delete the parent, confirm the fork is cleaned.

Evidence boundary: static read at d133ea5 against c180a2bac3, no build, no tests, no Desktop launched. The unmount → dismiss chain is derived from React semantics plus the effect at use-quote-companion.ts:765.

AI-assisted review: drafted with Maka; I verified the substitution path, the host render guard and the unmount effect myself.

[testikun]

Follow-up on the remaining coverage request: commits 2601168 and a71805a add a real side-chat draft to the parent-to-linked-child E2E journey, assert the same draft after navigation, and stabilize the transcript-selection gesture by waiting for a real non-collapsed browser selection before release. The controller regressions for cleared active and uncataloged unrelated sessions are already present and pass; the pending-child catalog-gap retention path remains intact.

Verification: focused controller suite 24/24 passed, Desktop production build passed, and the focused Electron parent-to-child draft-retention E2E passed.

@Astro-Han please re-review the new head when convenient.

[Astro-Han]

Thanks for preserving source ownership across linked navigation. Two reachable paths still break the central lifetime invariant: a retained Side Conversation must keep the same mounted instance and fork until an actual dismissal/source retirement. Both findings are inline.

I also compared current #4901 (7e2114916): the requirements are complementary, not duplicate fixes. Its queue/receipt reconciliation depends on the panel surviving. When rebasing, preserve this PR's explicit immutable source owner alongside #4901's queue handling; validate send/queue → linked navigation with a catalog gap → receipt/reseed → return → explicit close. A clean textual merge alone does not establish that behavior.

中文

两条可达路径仍破坏同一生命周期契约：需要保留的侧边对话必须维持同一挂载实例和fork，直到真正关闭或source退休。#4901与本PR需求互补，但队列回执依赖面板存活；解冲时要同时保留不可变source owner与队列处理，并走完整组合验收。

AI-assisted review by two fresh reviewers, with production paths cross-checked by the coordinating Codex agent. Findings are source-traced; no current-head live Electron reproduction was run. This head also needs its main conflicts resolved and validation refreshed.

[Astro-Han]

Thanks. Rechecking a71805aef from the original navigation requirement confirms that the two existing P1 threads share one invariant: a retained panel must retain the same mounted identity and immutable source owner.

The real pendingSessionView does not supply the lineage used by the new fallback; a fixture adding that lineage cannot prove catalog-delay safety. Separately, the family helper returns a physical revision representative as the React key. An independent helper probe with A, its newer revision A′, and child B yields retain=true but key A→A′ even on A→B navigation. Unmount then reaches the existing stop/cleanup path for the temporary fork.

Please derive membership and a stable logical scope from the existing revision authority, and avoid destructive classification while the real pending selection is unresolved. Do not add another retention cache to reconcile these two answers. The +634/-43 includes +356/-9 tests; it does not justify wholesale rewriting. The more important scope question is why preserving Side Chat changes the mount/reset obligations of every Workbar tool. Keep unrelated tools session-isolated unless their state retention is also intended and verified. The added artifactCountSessionId state is currently only written, never read, and can be removed.

#4901 changes message/queue projection and does not repair these navigation identity paths. Validate actual mount/stop/cleanup through parent→child→parent, delayed catalog, and revision navigation, not only visible tab records.

中文

两个既有 P1 属同一不变量：判为保留的 panel 必须保留挂载身份和原 source owner。真实 pending placeholder 没有测试夹具补出的 lineage；revision helper 又返回不稳定物理 ID，探针证明有新 revision 时父→child 也会 retain=true 但换 key，从而 stop/cleanup。应复用现有逻辑身份统一判定，未知目录期间不要破坏性清理，不要再加保留缓存。删除只写不读的 Artifact 身份状态，并收窄对其他 Workbar 工具生命周期的扩散；#4901 不修这一层。

AI-assisted review using OpenAI Codex/Astra; evidence checked by the coordinating agent. This is not an independent human review.

[testikun]

Reply for Astro-Han blocker comment is submitted here.

[testikun]

已按两位 P1 阻塞点逐一修复：\n1) 用 pending/ catalog 边界的已知家族锚点替代对 active session id 的直接回退判断，避免真实 pendingSessionView 未带 lineage 时误清理 Side Conversation。\n2) surface key 改为通过 revision 族链路返回稳定的逻辑 family root（），避免 A->A′ 切换时 React key 抖动导致 tab/unmount/fork 清理。\n3) 清理与展示逻辑统一按 linked family 判断，不再只按 ，并补充了回归用例（pending child catalog gap + revision 更新时 key 稳定）。\n当前代码基于 pr-4625-fixes 分支对齐，不再逐次评论已处理结果。

[testikun]

已按两位 P1 阻塞点逐一修复：

1. 用 pending/ catalog 边界的已知家族锚点替代对 active session id 的直接回退判断，避免真实 pendingSessionView 未带 lineage 时误清理 Side Conversation。
2. surface key 改为通过 revision 族链路返回稳定的逻辑 family root（sessionRevisionFamilyId），避免 A→A′ 切换时 React key 抖动导致 tab/unmount/fork 清理。
3. 清理与展示逻辑统一按 linked family 判断，不再只按 panel.sourceSessionId === activeSessionId，并补充了回归用例（pending child catalog gap + revision 更新时 key 稳定）。
   当前代码基于 pr-4625-fixes 分支对齐，不再逐次评论已处理结果。

[testikun]

已按 reviewer 的两条 P1 及相关 P3 一起处理：

1. pendingSession 场景

• 在 use-workbar-controller.ts 中按“真实 pending/catalog 边界”引入 lastKnownFamilySession 回退，pendingActiveSessionBelongsToKnownFamily 仅在 placeholder 场景下保留上一条已知家族，避免真实 pendingSessionView 未带 lineage 时误清理 Side Conversation。
• activeSideConversationPanels、stalePanels 与 linkedSideConversationFamilyRootId 全部改为按 linked family 推导。

2. revision 族 ID 稳定性

• isLinkedSideConversationSessionFamily 与 linkedSideConversationFamilyRootId 改为复用 session-revisions 的 revision 家族图（含别名/折叠），surfaceKey 返回 sessionRevisionFamilyId 的稳定根，不再随 A→A′/代表ID 抖动导致 remount。
• 这样 fork cleanup 不会在只发生 revision 切换时触发。

3. 回归覆盖

• 同步 worktrees/pr-4625-fixes 的测试补齐：新增/调整 pending-child catalog gap 与 revision 稳定 key 回归，补齐 pendingSessionView 场景。

提交：449f1b930（fix: preserve side-chat retention across pending and revision transitions）。
当前我这边已本地提交，但当前账号对 testikun/maka 没有 push 权限，CI 仍在该 run 报错在 renderer architecture 检查的 AppShell 文件集合，不是本次提交改动导致。