Repository navigation
refactor(desktop): own context compaction in Conversation - #5893
Conversation
AppShell built the compaction presentation, called the bridge for /compact, and threaded the Host's terminal outcome back into ConversationLifecycle. Move all three into the Conversation owner: the controller creates the one presentation, `compactSession` calls the injected `sessions.compact` service, and the lifecycle hands `contextCompactionOutcome` to the same presentation. The shell keeps only the stable `compactSession` command for composer submission's /compact port. The legacy module moves to a private feature model; the workspace-unavailable classifier moves to an application contract that the legacy helper re-exports. No copy, IPC or behavior change. Generated-by: Claude Code
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 769dcebf43e77b86b79c8f3eca5d034ebf51a776. This moves /compact request handling and the shared running/terminal notice into the persistent Conversation owner (apps/desktop/src/renderer/features/conversation/controller/use-conversation-controller.ts:57-64; apps/desktop/src/renderer/features/conversation/model/context-compaction.ts:145-188). The lifecycle now routes Host completion events to that same owner's command (apps/desktop/src/renderer/features/conversation/ui/conversation-lifecycle.tsx:93-99), while AppShell retains only the submit port. Workspace-unavailable classification is moved to an application contract without changing its predicate. I found no substantiated P0-P3 issue in the inspected paths.
Node 24 full build:test, 16 focused Desktop tests, Desktop typecheck, and the renderer architecture check (121 fixtures plus ledger) passed locally. Fetched main c838e1fa merges cleanly and git diff --check passed. The hosted label check is green, but test remains in progress, so the current-head CI gate is not yet established. I did not run the Electron /compact journey, a live Host compaction, or the full local test suite.
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.
…on-compaction # Conflicts: # apps/desktop/renderer-architecture.json
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed head 770899d5819401cf3f008b5e3a1abb56ee2b76d6. The only change since the previously reviewed 769dcebf is a merge of upstream main (1e80e3b88, the current origin/main). Its one conflict, in apps/desktop/renderer-architecture.json, re-baselines AppShell's nonTriviaTokens (9576 -> 9347) and shifts line numbers. Apart from that file, the PR's diff against main is byte-identical to the earlier diff against c838e1fa. The merge with main is clean, git diff --check passes, and no packages/ or protocol files are touched.
I re-checked that /compact behaves the same as before the move:
- Command routing:
composer-submit.ts(slashCommand.kind === 'compact', which readsactiveIdRefand then callsports.compactSession) is unchanged. AppShell now passes in the controller's stablecompactSessioninstead of a function it rebuilt on every render. - Bridge:
createDesktopConversationServicesspreadsbridge.sessions, soservices.sessions.compactis stillwindow.maka.sessions.compact. - Switching sessions during compaction: the new
isCurrentSessionreadsworkspace.activeIdRef. That is the same ref behind theactiveIdRef(publishedSession) that AppShell used before, so a rejected request is still reported only while its Session is current. - Error reporting: the workspace-unavailable toast and the generic
compactErrorTitle/compactErrorFallbacktoast show the same copy with the same{ sessionId }diagnostic target asshowSessionWorkspaceUnavailableToast/showSessionError. - Busy and in-flight guards: composer submission still owns them, and they are unchanged.
- Host outcome:
ConversationLifecyclesetsonContextCompactionOutcomeafter...props, pointing atcommands.finishContextCompaction. That is the same presentation the command uses, so the running notice is still dismissed. The presentation now lives as long as the controller instead of being rebuilt whenevertoastApichanges, which does not lose anything.
Validation on Node 22.23.2: build:workspace-deps, build:test, typecheck (preload/main/renderer/storybook) and check:architecture pass. 26/26 focused tests pass (conversation-compaction, conversation-owner, app-shell-command-actions, session-reference-composer and related). The full dist/main suite passes 3091/3091. The earlier automated review (5370830715) had no findings, so there was nothing to re-verify.
Findings: no substantiated P0-P3 issues.
Not exercised: the Electron e2e/slash-command-compact.spec.ts journey, a live Host compaction, and Node 24.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
|
Thanks @chihumyum — this was approved for merge, but it now conflicts with |
…on-compaction # Conflicts: # apps/desktop/renderer-architecture.json
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed e9aa420983bca25002d60ec71d13234fd5e1df1c. The only change since my previous review of 770899d5 is the merge of upstream main (0b25078b7), which brings in recent landings on the base branch rather than new work on this refactor.
No P0–P3 findings.
What I checked on the merge, since it touches two files this branch also edits:
app-shell.tsxkeeps both sides' work: the compaction wiring this refactor owns (compactSession,Conversation.ConversationLifecycle) is still present alongside the threeDiagnostics.*usages that arrived withmain's diagnostics extraction. The merge combined the two, it did not clobber either.renderer-architecture.jsoncarries the merged re-baselining, and the head'stestjob is green on it.- No file under
packages/uiorpackages/coreis touched by this branch beyond whatmainitself changed, so the refactor's own surfaces are untouched by the merge.
My earlier verification still stands, and the merge does not disturb any of it: the /compact command routing, the bridge spread that keeps services.sessions.compact intact, the isCurrentSession check reading the same activeIdRef behind publishedSession, the identical error copy and diagnostic target, the composer-owned busy/in-flight guards, and onContextCompactionOutcome still pointing at the same presentation. Nothing in the merged files changes those paths.
Gate on this head: test is green.
What I could not judge
- The Electron
e2e/slash-command-compact.spec.tsjourney, a live Host compaction, and Node 24 — same gaps as the previous pass, unchanged by a merge-only delta.
I did not approve, request changes, or merge.
Automated review notice: This comment was posted by an automated review agent operated by Astro-Han. It is not an independent human review and does not replace one.
|
Thanks @chihumyum for the quick merge of |
Astro-Han
left a comment
There was a problem hiding this comment.
Approved at @Astro-Han's explicit request: the automated re-review of this head (a merge of main to resolve the conflict with #5892) found no P0–P3 issues, CI is green and the branch has no conflicts.
Second verification pass over the Context-and-compaction slice of #3522: both `llm-compaction-events-log-projection-draft` documents (en + synced zh-CN), read line-by-line against current `main`. First pass was #4046; this one re-checks the chapter after the recent compaction-area changes (#5893, #5931, #5792, #5362, #5980). Generated-by: GLM-5.3-Flash (ZCode)
Summary
Refs #4582 — M2, fourth item (context compaction). Landmarks wait for #5712, which rewrites their new home; interaction consumers move with M3.
AppShell currently owns context compaction: it builds the presentation that tracks running notices per Turn, calls
window.maka.sessions.compactfor/compact, and threads the Host's terminalcontextCompactionOutcomeback intoConversationLifecycleas a prop. This moves all three into the Conversation owner.compactSessioncalls the injectedsessions.compactservice, andConversationLifecyclehands the terminal outcome to the same presentation. A reply that arrives after the Host has settled its Turn still adds nothing.compactSessioncommand, forwarded to composer submission's/compactport until M3 moves submission; the Conversation README records it as a transitional capability. The shell loses the presentation, the bridge call, the lifecycle prop and itssession-workspace-errorsimport.app-shell-context-compaction.tsbecomes the privatefeatures/conversation/model/context-compaction.ts, sealed byfeaturePrivateModules; tests reach it throughtesting.ts. Its copy stays inlocales/shell-copy.application/contracts/session-workspace-errors.ts. The legacy helper re-exports it, so its four legacy callers are unchanged.AppShell goes from 9 to 8 bridge paths, 53 to 51 import declarations and 9,614 to 9,385 tokens, and one legacy renderer file is retired. The hook inventory is unchanged (28 hooks / 43 calls). No copy, IPC, storage or visual change.
Verification
conversation-compaction.test.ts: command and outcome share the running notice, a finished failure is refused and shown against its Session, a rejected request is reported (workspace-unavailable and generic) only while its Session is current, and each notice uses the locale current when it is shown. A production-owner test drives/compactand the Host outcome throughConversationProviderandConversationLifecycle. Mutations: dropping the lifecycle hand-off fails the owner test; giving the outcome its own presentation fails three tests.e2e/slash-command-compact.spec.tspasses against a local build (Electron, fake backend).--strict-baseagainstc838e1fa7and the AppShell hook gate pass.build:rendererpass.Test support:
fake-dom.tsgains aquerySelectorthat returnsnull; Astryx toast dismissal only uses it to decide whether focus must move.Overlap:
conversation-owner.test.tsdrops the lifecycle prop on the line next to #5712's harness edit, a one-line resolution for whichever lands second.AI use
Tool(s) and scope: Claude Code implemented this ownership migration, its tests, documentation and validation under human direction. The commit includes
Generated-by: Claude Code. This PR awaits independent human review.Checklist
Does this PR entail a change in behavior?