Skip to content

refactor(desktop): move Composer staging into a persistent owner - #5868

Merged
chihumyum merged 3 commits into
apache:mainfrom
chihumyum:refactor/composer-staging-owner
Sep 30, 2026
Merged

chihumyum merged 3 commits into
apache:mainfrom
chihumyum:refactor/composer-staging-owner

Conversation

@chihumyum

@chihumyum chihumyum commented Sep 30, 2026 •

Copy link
Copy Markdown
Contributor

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

  • Desktop: 3,093/3,093, including 35/35 focused staging/revision/queue tests. Mutations that clear edited quotes, omit successful follow-up cleanup, or bypass revision-entry guards each fail a behavioral assertion; the restored implementation passes.
  • Production-owner tests cover ancestor/sibling render isolation, persistent editor-node identity, same-tick quote capture, original-draft/Host cleanup, edited-note retention, accepted/refused/failed sends through the production follow-up enqueue action, empty quotes, and invocation-time revision-entry guards.
  • Renderer architecture fixtures: 121/121; strict architecture comparison against d7dffca98c5b9f878ef9b71b4dcda4122e03f1c6 and 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.
  • Earlier validation at 19d48183: UI 696/696 and full Storybook browser smoke: 441 stories / 481 theme renders, including all Slash Menu and Shared Session Guest play assertions; the mention-story fixtures mount the persistent staging owner. The existing runner retried product-workhub--retry-while-work-filtered after one failure under four-way concurrency; it passed alone. No retry policy or timeout was changed.
  • Real Electron regression: 3/3 isolated fake-backend journeys (directory reference remove/send/reload, cross-Session @ 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

  • No generative tool made a substantive contribution
  • Generative tooling made a substantive contribution

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

  • Tests cover the change and fail without it
  • Lint, format, typecheck and the affected suites pass locally

Does this PR entail a change in behavior?

  • Yes — submission captures staging; successful cleanup preserves quotes added or edited while the send is pending, as described above.
  • No

@github-actions github-actions Bot added the effort/L Under 1000 readable lines label Sep 30, 2026
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
@chihumyum
chihumyum force-pushed the refactor/composer-staging-owner branch from 5053226 to 19d4818 Compare September 30, 2026 13:40
@chihumyum
chihumyum marked this pull request as ready for review September 30, 2026 13:41
@github-actions github-actions Bot added effort/XL Under 2500 readable lines and removed effort/L Under 1000 readable lines labels Sep 30, 2026
@chihumyum
chihumyum requested a review from Astro-Han September 30, 2026 13:44

@hqhq1025 hqhq1025 left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Astro-Han left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. Edited quote survives a successful send — inline.
  2. restore on the snapshot contract has no production caller (inline); either drop it or note in the README that a later slice uses it.
  3. 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) and hasPendingAttachments (app-shell.tsx:1196) aren't exercised. Low risk since app-shell-chat-actions.ts treats 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 ?? []),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 hqhq1025 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@chihumyum
chihumyum merged commit c838e1f into apache:main Sep 30, 2026
1 check passed
@chihumyum
chihumyum deleted the refactor/composer-staging-owner branch September 30, 2026 17:14
ggbdpq added a commit to ggbdpq/maka that referenced this pull request Sep 30, 2026
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

effort/XL Under 2500 readable lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants