refactor(desktop): move Composer staging into a persistent owner - #5868
Conversation
Capture draft-bound submission context and keep staging readers below the owner. Seal controller and context entry points for R2 M3 D1. Generated-by: OpenAI Codex
Mount the persistent staging owner around both slash-menu mention fixtures so catalog and context-switch browser regressions follow the production provider boundary. Generated-by: OpenAI Codex
5053226 to
19d4818
Compare
There was a problem hiding this comment.
Reviewed the Composer staging ownership change at 19d48183ff09bbba4ea4d2a657c31e30c9041540. The persistent provider now owns attachment, directory, and quote staging (apps/desktop/src/renderer/features/conversation/ui/composer-staging-provider.tsx:38-76); the actual Composer and transcript read it locally, while submission captures a draft-bound snapshot before asynchronous send work (apps/desktop/src/renderer/features/conversation/controller/composer-submit.ts:172, :345-349). I checked the draft/Host switch, same-tick quote, accepted/failed send, and restore paths. I found no substantiated P0–P3 issue in the inspected changes.
Local Node 24 build:test, 25 focused tests, and Desktop preload/main/renderer/stories typecheck passed. The typecheck initially failed because I installed with npm ci --ignore-scripts; applying the repository's required scripts/apply-dependency-patches.mjs resolved those errors. The current head merges cleanly with main 9b089f58; git diff --check passed. The hosted test first attempt failed in an unchanged WorkHub Electron E2E when its page closed during closePopup; the rerun has not completed, so the required gate is not green. I did not run packaged Electron or a real Host/session E2E. This review is not a merge-readiness claim while that gate remains unresolved.
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 at 19d48183 against main (including #5869). Moving staging into a single persistent ComposerStagingProvider preserves behavior: same draft keys and persistence across Session switches, the transcript and Composer still share the quote-editor ref across conditional mounts, guests still get no staging, Side Chat and WorkHub staging are untouched, and cleanup applies to the captured snapshot. Hook gate, renderer architecture check, renderer + stories typecheck, and 312 focused Desktop tests pass locally.
No P0–P2. Three P3s:
- Edited quote survives a successful send — inline.
restoreon the snapshot contract has no production caller (inline); either drop it or note in the README that a later slice uses it.- The new tests drive a fixture rather than AppShell, so the changed follow-up path (
app-shell.tsx:1220-1229: empty quotes →undefined, clear only snapshot quotes) andhasPendingAttachments(app-shell.tsx:1196) aren't exercised. Low risk sinceapp-shell-chat-actions.tstreats missing quotes as[].
Not verified: whether the quote editor is reachable during an ordinary (non-follow-up) send, and the "not mounted" throw if a post-send callback lands after AppShell unmounts (teardown/HMR only).
Automated review notice: This comment was posted by an automated review agent. It is not an independent human review and does not replace one.
| directoryOptions: staging.directoryOptions, | ||
| quotesForSend: () => quotes, | ||
| clearSubmittedContext: staging.clearSubmittedContext, | ||
| clearQuotes: () => staging.clearSubmittedQuotes(quotes ?? []), |
There was a problem hiding this comment.
P3: Clearing matches quotes by object identity, but editing a quote's note replaces the object (use-composer-quotes.ts:88 vs :72). If the user stages a quote, sends (e.g. a follow-up during a running turn), then edits the note before the send resolves, the quote is sent but the edited copy stays staged and goes out again next time — main cleared all quotes. The README says keeping edits is intended, but nothing tests it. Matching by a stable quote id, or a test pinning the intended behavior, would settle it.
| quotesForSend(): QuoteRef[] | undefined; | ||
| clearSubmittedContext(submitted?: readonly PendingAttachment[]): void; | ||
| clearQuotes(): void; | ||
| restore(content: { |
There was a problem hiding this comment.
P3: restore is only called from tests; the provider implements it but no production path uses it. Consider dropping it from the contract, or noting which follow-up will consume it.
Remove the unused public restore operation and share follow-up capture and cleanup with the production enqueue path. Cover edited quote retention, refused and failed sends, empty quotes, and invocation-time revision guards with the persistent staging owner. Generated-by: OpenAI Codex
hqhq1025
left a comment
There was a problem hiding this comment.
Reviewed commit 46722d871dc3eb47e8e6d46287b20fc3c3c02498. This increment extracts the Shell's follow-up submission into createStagedFollowUp (apps/desktop/src/renderer/features/conversation/controller/composer-submit.ts:133-161), removes an unused public staging-restore method, and adds production-enqueue tests for accepted, refused, and thrown queue/steer submissions (apps/desktop/src/main/__tests__/composer-staging-owner.test.ts:265-355). The callback preserves the previous placement, error reporting, and successful-cleanup behavior. Its captured quote list and identity-based cleanup retain edits and additions made while admission is pending (apps/desktop/src/renderer/features/conversation/ui/composer-staging-provider.tsx:47-62; apps/desktop/src/renderer/features/conversation/controller/use-composer-quotes.ts:86-91). I found no substantiated P0-P3 issue in the inspected increment.
Locally, Node 24 Desktop build:test, 14 focused tests, Desktop typecheck, and the renderer architecture check (121 fixtures and ledger) passed. The tree merges cleanly with fetched main 9b089f58, and git diff --check passed. The current-head hosted test check is still running, so the CI gate is not yet established. I did not independently run a packaged Electron app, real Host admission, or the full 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.
Fourth structural catch-up for apache#5274: carries apache#5868 (Composer staging moved into a persistent ComposerStagingProvider owner) and apache#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.
Summary
Refs #4582 — M3 / D, first slice.
Attachment, directory-reference and quote staging currently live in AppShell, so local staging updates invalidate the shell and send callbacks receive render-bound state. Move their controller into a persistent
ComposerStagingProvider, with actual Composer, transcript annotation and session-reference readers below it. AppShell keeps only stable commands; the controller and context/binding modules are sealed by the architecture guards. The attachment service is injected through the Desktop adapter. The staging readers compose under the Conversation publication/observation owner introduced in #5869; transcript state remains private to that owner.Submission captures the original draft before awaiting. Cleanup remains bound to that draft and directory Host. The public snapshot exposes only operations consumed in production; recovery can add restoration when its later slice defines that ownership. Accepted sends remove only captured quotes, preserving quotes added or edited during delivery; failed sends retain staging. The Composer input stays mounted across staging, section and Session changes. AppShell drops from 44 to 43 stateful hook calls and removes its staging-service bridge access.
Readiness, revision-draft state, send-pending state and delivery recovery remain later M3 slices. This preserves current-main revision/queue behavior; it does not implement the product changes in #5058 or #5274. Those open PRs overlap the submit/quote hooks and will need integration through the captured command boundary.
Verification
d7dffca98c5b9f878ef9b71b4dcda4122e03f1c6and AppShell hook gate pass.npm run build,npm run typecheck,npm run lint,npm run format:check, Desktop/UI Knip, ASF headers, Astryx inventory and Windows inventory pass.19d48183: UI 696/696 and full Storybook browser smoke: 441 stories / 481 theme renders, including all Slash Menu and Shared Session Guestplayassertions; the mention-story fixtures mount the persistent staging owner. The existing runner retriedproduct-workhub--retry-while-work-filteredafter one failure under four-way concurrency; it passed alone. No retry policy or timeout was changed.@reference submission, and transcript pointer capture outside the native window). The directory chooser return is mocked; no manual OS dialog or real-model acceptance is claimed. No intended visual redesign.AI use
Tool(s) and scope: OpenAI Codex implemented this ownership migration, tests, documentation and validation under human direction. The commit includes
Generated-by: OpenAI Codex. This PR awaits independent human review.Checklist
Does this PR entail a change in behavior?