Repository navigation
refactor(desktop): move Plan state and readers into Conversation - #5851
Conversation
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 743628e354d2d95aa2a188101a6aa173f95e601d. The Plan controller moves from AppShell into the Conversation PlanProvider (apps/desktop/src/renderer/app-shell.tsx:1781, features/conversation/ui/plan-provider.tsx:25); proposal and execution projections now read that shared owner near their UI surfaces (features/conversation/ui/plan-surfaces.tsx:26-40). The controller's session scope, latest-read fencing, query gate, control retries, and confirmation behavior remain in place; the Desktop adapter delegates the same bridge session methods. I found no substantiated P0–P3 issue in the inspected paths.
Locally, the focused Plan/query-gate tests passed (11/11), the renderer architecture tests and guard passed (112/112), AppShell hook check passed, and Desktop main build plus renderer typecheck passed after applying the repository's dependency patches. The current-head hosted test and label checks pass, and the branch merges cleanly with current main. I did not repeat the PR author's real Electron lifecycle run, online-provider testing, signed packaging, or the full local Desktop suite; this is not merge approval.
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
left a comment
There was a problem hiding this comment.
Reviewed exact head 743628e354d2d95aa2a188101a6aa173f95e601d. No P0–P2 findings; the move looks behaviour-preserving.
use-plan-mode-state.tsis a rename of the old panel hook; the only change is routing the sevenwindow.maka.sessions.*calls through injectedservices(created once inmain.tsx, so subscriptions are not re-armed).PlanProposalCard/PlanExecutionPanelbodies are unchanged.PlanProviderreceives the same session input as the old call site and is not keyed, so session-switch reset, latest-read fencing and teardown are unchanged; the execution panel keeps its!sharedSessionActive && sessionsSelectedguard.- Plan-only updates now re-render the provider and its two readers instead of the whole shell; moved tests only changed imports / gained a
PlanServicesProviderwrapper, andplan-owner.test.tsadds shared-read and render-isolation coverage.
Optional P3 nits:
create-conversation-plan-services.tsreturns the wholebridge.sessionsobject (inline).PlanProviderpasses the hook result straight into the context value; if that object is rebuilt per render, both readers re-render on every shell render. Same cost as before, so not a regression —useMemoif you want it.- Doc comments in
observable-state.ts,session-catalog-state.tsanduse-external-store-selector.tsstill name the deletedapp-shell-session-ui-statemodule.
Checks run locally: ASF headers, git diff --check, check:app-shell-hooks, check:renderer-architecture, windows:inventory (current), astryx surface inventory, Desktop typecheck, and focused Plan / query-gate / session UI-state tests (all passed). Not run: the app itself, full test:dist, renderer build.
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| export function createDesktopConversationPlanServices( | ||
| bridge: Pick<MakaBridge, 'sessions'> = window.maka, | ||
| ): PlanServices { | ||
| return bridge.sessions; |
There was a problem hiding this comment.
P3: this hands the feature the entire bridge.sessions object, so PlanServices narrows only the type, not the runtime surface. The Goal adapter wraps method-by-method; doing the same here would keep the boundary real. The test currently asserts identity, so this may be deliberate — optional.
Own the Plan controller in PlanProvider and keep proposal and execution projections below it, preserving the existing Session lifecycle. Seal the public boundary and register the owner with the architecture guard. Refs apache#4582 (M4) Generated-by: OpenAI Codex
743628e to
082c043
Compare
Resolve the renderer-architecture.json conflict by taking main's ledger (new apache#5851 file entries) as the base and re-pricing every entry from the merged tree via check-renderer-architecture.mjs --write; the check passes against the ratchet baseline.
Review follow-ups (#5274 review): - prepareRevisionSend re-keyed the edit-start snapshot, so quotes the user removed or re-annotated during the edit reappeared on the branch and could be sent. The re-key now lands the plate's current quotes, and the send reads them live (the before-send snapshot retires — the re-key covers its case). - Drops the stale-base footer copy block and a dead regenerate key: no production code on current main references the old footer shape that survived in this branch's base. The no-op gate keeps counting the model-facing comment. Generated-by: GLM-5.3-Flash (ZCode) Merge upstream/main into fix/desktop-revision-structured-context fix(desktop,ui): preserve the quote plate across the in-flight revision send The revision send awaited prepareRevisionSend before reading the plate, and the lifecycle re-keys it mid-send: the plate's current quotes are copied onto the branch child and the source bucket is emptied in place. The still-running sendWithAttachments invocation holds quotesForSend from the render that started the send, so it read the emptied source bucket, got undefined, and submitted the edited replacement without its quotes or their edited annotations (#5274 review). Read the plate through the re-keyed owner explicitly: quotesForSend now resolves the bucket by owner key (defaulting to the live draft key for the non-revision send paths), and the revision send passes the prepared draft's session id. The resumed send therefore reads the child bucket the lifecycle just wrote — removals and re-annotations included. The regression test drives beginEditUserMessage → an in-render quotesForSend capture → prepareRevisionSend and asserts the re-keyed plate with its edited annotation still reaches the send; it fails on the previous head with `actual: undefined`. Generated-by: GLM-5.3-Flash (ZCode) fix(desktop,ui): keep quotes added during a cancelled edit Cancelling a revision draft cleared both draft keys wholesale, so quotes the user staged during the edit were lost together with the edit's own restaged set (#5274 review: "Cancelling an edit deletes quotes added during it" — on main they stayed in the composer, and adding quotes during an edit is supported). clearQuotes now returns the entries it removed, and clearRevisionStagedContext re-stages whatever lies beyond the edit's beginEdit snapshot (matched per QuoteRef field set, counted as a multiset) onto the source Session the cancel returns to — whether the user added them before or after the branch child was prepared. The edit's own items are still dropped wherever the commit left them. The regression tests drive a cancelled edit with a user-added quote on each side of the prepare step and assert the addition survives on the source key while the composer text rolls back; both fail on the previous head, the first with the edit's restaged quote returned instead of the user's own, the second with no plate restore at all. Generated-by: GLM-5.3-Flash (ZCode) fix(ui): treat negative owned counts as fully user-owned clearRevisionStagedContext drops the edit's own quotes by per-key count: the first N entries matching the beginEdit snapshot are the edit's, and everything past that count is the user's own staging. The kept check compared the remaining count with `=== 0`, so once the counter ran negative the surplus stopped matching: when the user added a second copy of a quote the edit had also staged, the third identical plate entry was dropped despite being the user's. Treat any non-positive count as fully user-owned (`<= 0`), so every entry past the edit's own count survives the cancel. Fixes a P3 raised inline on #5274. Generated-by: GLM-5.3-Flash (ZCode) Merge upstream/main (8648258) Brings in the composer-submit extraction (#5815) and the allow-unchanged edit-and-resend decision, merged into the revision staged-context feature: - app-shell.tsx keeps the upstream extraction; the staged-context wiring and the #5274 re-keyed quotesForSend/clearQuotes semantics move into features/conversation/controller/composer-submit.ts (owner-keyed ports). - app-shell-revision-actions.ts keeps the @maka/ui delegation; TurnRevisionDraftBase drops originalText and the unchanged gate (revisionStagedContextUnchanged) so unchanged resends pass through per upstream #5815, while the mixed-context conflict gate stays. - conversation-copy.ts keeps both new locale keys (revisionDraftQuoteConflict, revisionMixedContextUnsupported) on top of upstream's revisionUnchanged removal and #5815 copy updates. - renderer-architecture.json regenerated via check-renderer-architecture --write; app-shell.tsx budget drops 12091 -> 11154. Tests: upstream app-shell-revision-resend gains a stagedContext fake; the ui revision suite expectations updated to the allow-unchanged behavior. Generated-by: GLM-5.3-Flash (ZCode) fix(desktop): assemble the revision staged context outside the frozen shell Root cause: upstream #5546 extracted app-shell helpers and drove the frozen shell's nonTriviaTokens baseline down to 11129 with zero headroom, so the merge that re-attached this PR's stagedContext wiring (a 7-line factory in the createAppShellRevisionActions ports object, +25 tokens) broke the renderer architecture ratchet: 11129 -> 11154. Fix: assemble the RevisionStagedContext closure inside the desktop useComposerAttachments controller (a feature file outside the debt ledger), right beside the hooks that own the quote/attachment buckets. The frozen shell now forwards one member (stagedContext,) instead of building the factory inline. The restoreAttachments entry the shell only carried for that factory is dropped from its destructure: its sole consumer was the deleted block (RevisionStagedContext declares no such member and nothing in @maka/ui reads it from the staged context). Verification: - check:renderer-architecture --base 6e21e61 --strict-base: red (11129 -> 11154) before, green (11129 == base) after; ledger diff is the single app-shell.tsx nonTriviaTokens line, no other entry moved. - desktop focused suites (app-shell-revision-actions, use-composer-quotes-revision-send, app-shell-revision-resend, new-task-staged-content): 18/18 pass. - packages/ui full suite: 710/710 pass. - typecheck green: ui tsconfig + desktop preload/main/renderer/storybook. - biome lint clean on both touched source files; check:asf-headers green. Generated-by: GLM-5.3-Flash (ZCode) Merge upstream/main (28cc4e6) Resolve the renderer-architecture.json conflict by re-pricing the ledger from the merged tree via check-renderer-architecture.mjs --write (all entries at real merged-tree values, within baseline); drop the trailing blank line at EOF in conversation-copy.ts flagged by diff-check. Merge upstream/main (0aa2707) Resolve renderer-architecture.json by re-pricing from the merged tree via check-renderer-architecture.mjs --write. Merge upstream/main (d6876d7) Resolve the renderer-architecture.json conflict by taking main's ledger (new #5851 file entries) as the base and re-pricing every entry from the merged tree via check-renderer-architecture.mjs --write; the check passes against the ratchet baseline. Merge upstream/main (d7dffca) Merge upstream/main (c838e1f) Fourth structural catch-up for #5274: carries #5868 (Composer staging moved into a persistent ComposerStagingProvider owner) and #5884 (task archive timestamps). Conflict resolution ports the revision staged-context semantics onto the new owner structure: ComposerStagingSubmission gains optional owner-key reads/clears (snapshot for captured submissions, live plate for the revision re-key) and ComposerStagingCommands gains stagedContext(); the frozen shell forwards the one composerStaging handle and the revision assembler derives the gate probe and plate reads from it. Merge upstream/main (1e80e3b) into fix/desktop-revision-structured-context # Conflicts: # apps/desktop/renderer-architecture.json Merge upstream/main (c7fa6bb) into fix/desktop-revision-structured-context # Conflicts: # apps/desktop/renderer-architecture.json Merge upstream/main (255ae23) into fix/desktop-revision-structured-context Merge upstream/main (229e1b4) into fix/desktop-revision-structured-context Merge upstream/main (f7633c3) into fix/desktop-revision-structured-context Merge upstream/main (7c90bac) into fix/desktop-revision-structured-context Merge remote-tracking branch 'upstream/main' into fix/desktop-revision-structured-context # Conflicts: # apps/desktop/renderer-architecture.json fix(desktop): drop the stale regenerate footer copy #5468 removed the Regenerate feature (87ff279), deleting the footer's `regenerate` label and the `regenerateRunning` / `regenerateAgain` / `regenerate` / `requestRegenerate` keys from the conversation copy. This branch's stale base still carried the block, so the merge-base diff re-adds those keys across the contract type and all three locales (including the zh-TW 本輪迴答 wording) while nothing on current main references them — merging would resurrect dead strings. Drop the block from the type and the zh-CN / zh-TW / en entries, matching the upstream shape exactly; the `lineage.regenerated*` keys are a different, live feature and stay. Generated-by: GLM-5.3-Flash (ZCode)
Summary
Plan state and proposal projection currently live in
AppShellContent, so Plan updates rebuild the shell even though the proposal and execution surfaces are their actual readers. Move the existing controller into a Conversation-ownedPlanProviderand put both production readers below it. AppShell supplies only the Session target; the public entry exposes regional components and services, not the Plan controller or full-state hook.Inject a narrow Plan service port, preserve the current query gate, scope fencing, confirmation targets and approval/resume retry inputs, and leave settings writes with Session Settings. Register the sole controller owner with the existing architecture guard, remove the old root module/barrel access, and shrink the shell hook inventory from 33 hooks / 55 calls to 32 / 54. No product, visual, storage or IPC behavior change is intended.
Refs #4582 (M4). This is the bounded Plan ownership slice, not completion of the full R2 issue.
Verification
Rebased onto
751f9cc2521ab9cb33f17a1fa356204ea4c7fe2a(including #5850), with the current lockfile and dependency patches. Local checks on the rebased branch:--base upstream/main --strict-base; checker implementation is unchanged.git diff --checkand merge-tree validation against main passed.ChatSurfaceLayout: a shared query/subscription owner updates their output without rerendering the frame or sibling or replacing its draft input. The existing Plan lifecycle tests remain in use. The adapter test covers non-enumerable/Proxy bridge methods.Computer Use acceptance used a real Electron window, preload/Main/Host, Runtime tools and SQLite with an isolated profile and a deterministic local model endpoint. It passed proposal creation/revision/approval, progress updates, Session switching, stop, full process restart, resume/completion, and both cancel/confirm paths for abandonment. During a 1/3 → 2/3 update, the real Composer retained its draft and selected text; typing replaced the preserved selection without refocusing. Read-only SQLite inspection confirmed 14 ordered Plan events and the expected completed/cancelled executions.
That window run used the original implementation before the main-branch integrations above and was not repeated during this conflict resolution. The Plan controller, panels, owner, service port/adapter and owner integration test remain byte-identical to the reviewed original head. Shell integration now preserves #5850's scoped read ports and test-only constructors, and service composition retains the new storage-usage provider. Automated checks were rerun after integration. This does not claim online-provider, signed-package or whole-application performance acceptance. The full Playwright E2E suite was not run; #5447 remains independent Plan lifecycle E2E work.
Integration
#5850 is now on the base branch. This PR preserves its Session-bound readers and private construction modules, uses its testing entry for state construction, and adds the private Plan controller owner. The architecture ledger and Astryx inventory were regenerated from the combined source; no checker rules were weakened. Storage-usage services added by #5832 remain wired alongside the Plan services.
AI use
Tool(s) and scope: OpenAI Codex assisted with architecture analysis, implementation, tests, local validation, Computer Use acceptance and PR preparation. The commit includes
Generated-by: OpenAI Codex.Checklist
Does this PR entail a change in behavior?