feat(desktop): preserve Side Conversations across linked-session navigation - #4625
feat(desktop): preserve Side Conversations across linked-session navigation#4625testikun wants to merge 2 commits into
Conversation
2829ccb to
4df1c2d
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
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:
- 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.
- 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.
- 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.
- On a Sub Agent, open a Side Conversation, go to the parent, come back. Decide whether losing it is the product behavior you want.
- 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.
testikun
changed the title
|
已处理并重新提交。
|
Generated-by: OpenAI Codex
Generated-by: OpenAI Codex
026daae to
7593446
Compare
Astro-Han
left a comment
Astro-Han
left a comment
There was a problem hiding this comment.
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.
| activeSession: LinkedSession | undefined, | ||
| sessions: readonly LinkedSession[], | ||
| ): string | undefined { | ||
| if (!activeSession) return undefined; |
There was a problem hiding this comment.
P2: this helper did not get the pending fallback that isLinkedSideConversationSessionFamily got at line 45, and the two disagree on exactly the path the fallback exists for.
A pending active Session has no subagent field, so the walk falls through to line 69 and returns the pending id as its own root. use-workbar-controller.ts:693 then hands that to workbar-host.tsx:184, the key flips off the family root, and WorkbarSurface remounts. QuoteCompanionPanel unmounts with it, and the unmount effect at use-quote-companion.ts:734-760 calls dismissCompanionCopy, removing the fork on the Host. dismissalGuardRef at use-quote-companion.ts:301 is a per-instance useRef, so the remounted instance cannot suppress that cleanup.
Net result on that path: line 45 keeps the panel and the tab, so the controller reports retention, while the draft, the transcript and the running Turn are already gone and the user is looking at a tab that no longer owns anything. That is the loss the PR exists to prevent.
Smallest fix: both helpers share one pending judgement. When the active Session is not in sessions, do not return its own id here (return undefined and let the caller keep the previous key, or return the retained panel's family root). While you are there, narrow line 45 as well: as written it retains every panel for any unknown active id, including panels whose source has no relation to it, and a freshly created Session can lack a catalog row until its first send.
| ); | ||
| }); | ||
|
|
||
| it('keeps Side Chat while the active source awaits its catalog row', async () => { |
There was a problem hiding this comment.
P3: this test does not reach the fallback it appears to cover. The panel's sourceSessionId is the active Session's own id, so it returns at side-conversation-session-family.ts:41, the branch that already existed. Delete line 45 and this stays green.
A case where the source is the parent and the active Session is a child that has not entered the catalog yet would pin the new branch, and asserting surfaceKey is unchanged in that case would also catch the P2 above.
| if (current.id === targetSessionId) return true; | ||
| if (visited.has(current.id)) return false; | ||
| visited.add(current.id); | ||
| const parentSessionId = linkedSubagentParentSessionId(current); |
There was a problem hiding this comment.
P3 (unchanged from the previous round, restating because it was not answered): this walks raw linkedSubagentParentSessionId, while the session rail walks projectRevisionLinkedSessionTree (session-revisions.ts:95), which collapses revisions and aliases physical ids to the representative row. After edit-and-resend on parent A produces A', the rail shows B under A' but this walk stops at A, so the family the user sees and the family retention computes have diverged. core/session.ts:632's projectLinkedSessionTree also already carries the cycle guard reimplemented here.
Also still open on line 49 and line 63: both build a fresh Map per panel per call, while input.sessions changes identity on every catalog revision and streams during a Turn. authoritativeSessionIds next door uses sessionIdSetsEqual specifically to avoid that churn.
2 participants
Summary
Closes #4494
Verification
npm exec -- biome checkon changed Workbar files — passed.npm --workspace @maka/desktop run build:test— passed.@maka/uitype errors (autoScroll,settledText, andtrailingAction) outside this change.AI use
Tool(s) and scope: OpenAI Codex reviewed and implemented the focused Desktop Workbar lifecycle change, added regression coverage, and prepared the commit. The commit includes a
Generated-by: Codextrailer.Checklist
Does this PR entail a change in behavior?