You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@maka.apache.org by "Sun-GLiang (via GitHub)" <gi...@apache.org> on 2026/09/17 07:04:35 UTC
[PR] fix(desktop): retain side chats across session switches [maka]
Sun-GLiang opened a new pull request, #5440:
URL: https://github.com/apache/maka/pull/5440
## 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.
## 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
- [x] 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
- [x] Tests cover the change and fail without it
- [x] Lint, format, typecheck and the affected suites pass locally
Does this PR entail a change in behavior?
- [x] Yes — described under Summary above
- [ ] No
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "Astro-Han (via GitHub)" <gi...@apache.org>.
Astro-Han commented on code in PR #5440:
URL: https://github.com/apache/maka/pull/5440#discussion_r4052646391
##########
apps/desktop/src/renderer/features/workbar/controller/use-workbar-controller.ts:
##########
@@ -682,6 +682,44 @@ export function useWorkbarController(
[layout.closeWorkbarTabs, sideConversations, terminal, input.toastApi, terminalCopy.stopFailed, locale],
);
+ useEffect(() => {
+ const authoritativeSessionIds = input.authoritativeSessionIds;
+ if (!authoritativeSessionIds) return;
+ const retiredPanelIds = new Set(
+ sideConversations.panels
+ .filter((panel) => !authoritativeSessionIds.has(panel.sourceSessionId))
Review Comment:
**P2 — absence ≠ deletion: a transient catalog gap permanently retires a live Side Chat.** `authoritativeSessionIds` is the full catalog membership for the latest committed revision, and `commitSessions` replaces it wholesale — so any refresh or reconnect that commits a momentarily smaller set drops a live source session out of this filter for one revision and immediately deletes its fork, transcript, and in-flight turn. Catalog gaps are a real phenomenon (the #4625 review hit the same shape, though there the consequence was only an unmount; here it is destructive). Retire should key on an explicit signal — the removed-ids path of `sessions:changed`, or the archive/delete commands themselves — rather than membership absence; at minimum, require the source to stay absent across a confirmation pass before deleting durable state.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "me2seeks (via GitHub)" <gi...@apache.org>.
me2seeks merged PR #5440:
URL: https://github.com/apache/maka/pull/5440
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "hqhq1025 (via GitHub)" <gi...@apache.org>.
hqhq1025 commented on code in PR #5440:
URL: https://github.com/apache/maka/pull/5440#discussion_r4035003748
##########
apps/desktop/src/renderer/features/workbar/ui/workbar-surface.tsx:
##########
@@ -417,14 +417,27 @@ export function WorkbarSurface(props: {
const locale = useUiLocale();
const copy = getDesktopConversationCopy(locale).workbar;
const tools = workbarToolsForWorkspace(props.workspace);
+ // Inactive Side Chats remain mounted across main-Session navigation. Retain
+ // 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>());
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on code in PR #5440:
URL: https://github.com/apache/maka/pull/5440#discussion_r4035084527
##########
apps/desktop/src/renderer/features/workbar/ui/workbar-surface.tsx:
##########
@@ -417,14 +417,27 @@ export function WorkbarSurface(props: {
const locale = useUiLocale();
const copy = getDesktopConversationCopy(locale).workbar;
const tools = workbarToolsForWorkspace(props.workspace);
+ // Inactive Side Chats remain mounted across main-Session navigation. Retain
+ // 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>());
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on code in PR #5440:
URL: https://github.com/apache/maka/pull/5440#discussion_r4034571473
##########
apps/desktop/src/renderer/features/workbar/controller/use-workbar-controller.ts:
##########
@@ -927,9 +901,11 @@ export function useWorkbarController(
},
rightResizable: layout.workbarResizable,
bottomResizable: layout.bottomPanelResizable,
- quotes: sideConversations.panels.filter(
- (panel) => panel.sourceSessionId === activeSessionId,
- ),
+ // Keep every Side Chat mounted while another main Session is selected.
+ // 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,
Review 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`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #5440:
URL: https://github.com/apache/maka/pull/5440#issuecomment-5740837898
@Astro-Han I addressed the P2 finding in `19a93c6e9`. Side Chats are no longer retired because their source is absent from one catalog refresh. Cleanup now follows an explicit `sessions:changed` event with reason `deleted`. A catalog snapshot does not by itself prove deletion, and retiring a panel also deletes its temporary fork. The regression test covers a temporary catalog gap and recovery, archival, an unrelated Session deletion, and source deletion.
The Workbar controller tests pass (36/36), along with the Desktop main build, renderer typecheck, Biome, and renderer architecture checks (112/112). I have left the P3 subscription behavior unchanged in #5440. Whether hidden Side Chat subscriptions need to be paused can be decided after the separate performance investigation is complete.
Assuming #5440's broader retention behavior is accepted, I suggest merging it first after review and CI. It establishes one lifecycle rule across main Session switches: navigation hides a Side Chat; explicit close or confirmed source deletion cleans it up. #4625 adds the linked child case, but its current cleanup on unrelated navigation or catalog absence conflicts with that rule. Rebasing #4625 afterward would let it add linked child visibility and immutable source ownership without keeping a competing cleanup rule.
<details>
<summary>中文说明</summary>
我已在 `19a93c6e9` 中处理 P2。Side Chat 不再因为某次目录刷新暂时缺少源 Session 就被清理；现在清理依据明确的 `sessions:changed`、`reason: deleted` 事件。目录快照本身不能证明源 Session 已删除，而移除面板还会删除其临时 fork。回归测试覆盖目录短暂缺失及恢复、归档、删除无关 Session，以及删除源 Session。
Workbar controller 测试 36/36 通过，Desktop main 构建、renderer 类型检查、Biome 和架构检查 112/112 也通过。#5440 没有修改 P3 涉及的订阅行为。是否需要暂停隐藏 Side Chat 的订阅，等待独立性能排查完成后再决定。
如果维护者认可 #5440 较广的保留行为，我建议它通过 review 和 CI 后先合入。它先确立跨主 Session 切换时的生命周期规则：导航只隐藏 Side Chat，明确关闭或确认源 Session 已删除时才清理。#4625 增加关联子 Session 场景，但它目前在切到无关 Session 或目录缺项时清理面板的逻辑与这条规则冲突。之后让 #4625 基于 #5440 变基，就可以加入关联子 Session 的可见性和不可变源 Session 所有权，同时避免保留两套冲突的清理规则。
</details>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "Sun-GLiang (via GitHub)" <gi...@apache.org>.
Sun-GLiang commented on PR #5440:
URL: https://github.com/apache/maka/pull/5440#issuecomment-5712457827
@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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org
Re: [PR] fix(desktop): retain side chats across session switches [maka]
Posted by "hqhq1025 (via GitHub)" <gi...@apache.org>.
hqhq1025 commented on code in PR #5440:
URL: https://github.com/apache/maka/pull/5440#discussion_r4034500806
##########
apps/desktop/src/renderer/features/workbar/controller/use-workbar-controller.ts:
##########
@@ -927,9 +901,11 @@ export function useWorkbarController(
},
rightResizable: layout.workbarResizable,
bottomResizable: layout.bottomPanelResizable,
- quotes: sideConversations.panels.filter(
- (panel) => panel.sourceSessionId === activeSessionId,
- ),
+ // Keep every Side Chat mounted while another main Session is selected.
+ // 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,
Review 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org