fix(desktop): keep the new-task composer's contents when the target changes - #3432
Conversation
There was a problem hiding this comment.
Pull request overview
Fixes Desktop new-task composer state handoff so that changing the workspace picker target (Project) does not make in-progress composition appear to disappear. This aligns draft text plus staged attachments/quotes with the currently selected new-task target, while keeping Session-scoped state isolated.
Changes:
- Carry the visible new-task draft across
(profileId, hostId, projectId)target changes inChatComposerRegion, with special handling for the unresolved startup key and reload-intent persistence. - Add a generic “pending bucket rekey” pathway for staged attachments and quotes so they move with the selected new-task target.
- Add unit + e2e coverage to ensure the new behavior fails on
mainand passes with the fix.
Required Conclusion
- Is the current solution optimal for the actual problem? Mostly yes: it addresses the root cause (keyed buckets swapping on target change) at the correct ownership points (draft in
ChatComposerRegion, staged items in the staging hooks) with minimal new abstraction. - If applicable, what production code can be deleted? none identified.
- If applicable, what low-quality tests can be deleted or replaced? none identified.
- Is a deeper refactor required, and what should the final structure be? No deeper refactor required; the current split (draft vs staged buckets) is appropriate. The small shared helper hook for staged buckets is a reasonable end state.
- Is the reviewed revision ready to merge? Yes, assuming the inline comment is addressed or explicitly waived by the maintainer.
- What residual risks or verification gaps remain? Minor edge-case risk around object-map key handling in
rekeyPending(prototype-chain key collisions) and typical flake risk in compositor-timing-sensitive e2e; no other verification gaps identified from the diff.
Review-relevant risks / protected areas: No direct impact identified on security/auth boundaries, licensing, releases, or governance in this diff; primary impact is user-visible Desktop behavior for draft/staging preservation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| apps/desktop/src/renderer/use-new-task-pending-carry.ts | New hook to rekey staged-item buckets when the new-task target changes. |
| apps/desktop/src/renderer/use-app-shell-composer-quotes.ts | Plumbs new-task target key into quote staging; invokes carry hook. |
| apps/desktop/src/renderer/use-app-shell-composer-attachments.ts | Plumbs new-task target key into attachment staging; invokes carry hook. |
| apps/desktop/src/renderer/chat-composer-region.tsx | Carries draft text across target key changes with unresolved/reload-intent handling. |
| apps/desktop/src/renderer/app-shell.tsx | Wires currentNewTaskDraftKey into attachments/quotes staging hooks. |
| apps/desktop/src/renderer/app-shell-pending-attachments.ts | Adds rekeyPending helper for moving staged buckets between keys. |
| apps/desktop/src/main/tests/new-task-pending-carry.test.ts | New unit tests asserting staged attachments/quotes move with target and don’t leak into Sessions. |
| apps/desktop/src/main/tests/chat-composer-region-draft-handoff.test.ts | Expanded unit coverage for draft carry across target changes and reload-intent behavior. |
| apps/desktop/e2e/new-task-draft-target.spec.ts | New e2e test validating real-window wiring: picker selection does not drop visible draft. |
| apps/desktop/e2e/fixtures.ts | Adds a seeded project + a dedicated fixture window to support the new e2e test. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for fixing the target-key isolation and the earlier prototype-chain issue. The exact-head CI is green, but two normal asynchronous paths still let pending content escape its intended owner. I left both comments inline with a single-owner final state, so this should not require another parallel carry mechanism.\n\nAI-assisted review disclosure: Codex performed an independent exact-head review; I verified the affected async ownership paths and live PR state before posting.
…hanges Draft text, staged attachments and staged quotes are all keyed by (profileId, hostId, projectId) since apache#3122, and the workspace picker that changes the project part sits directly under the composer — so choosing a Project re-keyed all three mid-composition and what the user was writing dropped out of view. Carry them to the target the user selects. The buckets stay keyed per target, so apache#3122's Host-scoped new-task state is unchanged; they move with the selection instead of staying behind under the key the user navigated away from. The move is unconditional: the target arrived at holds what was brought to it and nothing else. Carrying only into an empty target would leave copies under every key the composer passed through, and one would resurface later — send the task, come back to an empty composer, pick another target, and the text just sent would reappear as that target's own draft. The carry keys on the new-task target rather than on the composer's active key, so a Session keeps its own draft and attachments when the target moves behind it. Leaving the unresolved slot keeps its existing reload-lease guard: that transition is startup settling, not a choice, and its draft may belong to one specific target. Fixes apache#3408 Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — this is a careful fix, and the part that's easy to get wrong is the part that's handled best.
What this solves / how: since #3122 everything the session-less composer holds — draft text, staged attachments, staged quotes — is keyed by (profileId, hostId, projectId), and the workspace picker sits directly below the composer. So "type the task, then pick where it runs" — the ordinary order — re-keys all three mid-composition and the content vanishes from under the user. This moves the staged buckets to the key the user lands on, in a layout effect so it lands in the same commit that re-keyed them and no frame of "nothing staged" ever paints.
On the design, since it's the part worth checking: the obvious reading is that this adds a shadow copy of composer state. It doesn't — rekeyPending deletes the source key before writing the destination, so it's a move, not a duplicate. The underlying tension is real though: per-target drafts and "content follows the target I pick" pull in opposite directions. Resolving it by letting only the new-task key follow, while a Session's staged items stay with the Session they were staged for, is the right cut, and the hook comment says so explicitly.
The one thing that reads as duplication — text going through composer.setDraft while buckets go through rekeyPending — turns out to be forced rather than incidental. The composer's draft hook re-remembers live text under the key it's leaving (use-composer-draft.ts), so text can't be moved, only copied forward with the empty case assigned unconditionally to stamp out what was left behind. Buckets live in a plain state map and can genuinely be deleted. Both paths land on the same rule: the destination gets exactly what the source had, including nothing. That's what stops a bucket left under a key the composer merely passed through from resurfacing later as that key's own staged set — send a task, come back, pick another target, and the text you just sent would otherwise reappear.
Test coverage matches the risk: carry-follows, no-resurrection-after-send, targeted reload restore, Session isolation, and a two-way e2e through the picker.
LGTM.
AI-assisted review. Three passes were run independently — architecture, implementation correctness, and an adversarial attempt to find the shadow copy. The "this must be duplicated state" hypothesis was raised first and then disproved against the source, including verifying that the draft hook's re-remember behaviour is real rather than assumed.
Review found two ways pending content could escape the owner an in-flight operation had captured. Staged files and quotes no longer follow the workspace picker at all. They are in-memory intent, not Host state — nothing persists them and nothing restores them per target — so they go back to one key that never moves, as they were before apache#3122. `send()` captures the key it submitted from and clears exactly that key once it resolves, so a key that followed the picker left the files it had already delivered staged under the new target, ready to be sent a second time. A key that cannot go stale needs no re-keying rule to keep it honest, which removes rekeyPending and the carry hook with it. The draft text keeps its target-scoped key, because its reload lease belongs to one specific target. ChatComposerRegion therefore defers its carry while a send is settling: the submission owns the text it submitted until the completion clears the key it captured, and text typed after that send is carried once it does. pickAttachments resolves its owner after the native dialog closes rather than capturing it before opening. The surface can change while a dialog is up, and files the user just chose belong in the composer they are looking at, not in a bucket they have left where the files are invisible but still sendable. Generated-by: Claude Code (Claude Opus 5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
dc8c226 to
d28de25
Compare
|
Both findings are fixed in d28de25, and the description above is rewritten to match. Rather P1. Staged files and quotes no longer follow the picker at all. They are in-memory intent, The draft text does need its target-scoped key, since its reload lease belongs to one specific P2. Net −99 lines on the fix. The e2e now also drops a real Two housekeeping notes. The branch is rebased onto current |
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewing at d28de250 — the rebase carried a real design change, so the earlier approval (against dc8c226f) describes code that no longer exists.
The change is a simplification, and a better one than what I approved. rekeyPending is gone; staged attachments and quotes now live under a single fixed NEW_TASK_PENDING_KEY instead of being moved between per-target keys.
That's the stronger answer to the same problem. The earlier version kept the (profileId, hostId, projectId) keying and added a move rule to compensate when the picker changed it. This version asks whether that keying earned its place at all — and it didn't: staged files and quotes are in-memory intent, never persisted and never restored per target, so keying them by target provided nothing while causing the exact bug being fixed. Removing the dimension beats patching around it, and the file comment makes that argument explicitly rather than leaving it implied.
It also closes the resubmission risk by construction rather than by rule. send() captures the key it submitted from and clears that key on resolve; with a key that followed the picker, files just sent would remain staged under the new target and could go out twice. A key that never moves cannot go stale, so no re-keying invariant has to be maintained to keep it honest.
Verified the earlier foundations survived the rebase, since that's where a rebase usually goes wrong:
- The unconditional
setDrafton the empty case is still there, now with anewTaskSendPendingguard so an in-flight submission doesn't carry text to a new target. - The
previous === UNRESOLVED_NEW_TASK_DRAFT_KEYreload gate is unchanged. - Draft text still can't cross from a Session into the new-task slot.
CI is green at this head.
LGTM — this supersedes my approval of dc8c226f.
AI-assisted review. Re-run against the new head after the rebase changed the approach; the earlier conclusion was not carried over.
Summary
Everything the new-task composer holds — draft text, staged attachments, staged quotes — was
keyed by
(profileId, hostId, projectId)since #3122. The workspace picker that changes theproject part sits directly under the composer, so choosing a Project re-keyed all three
mid-composition and what you were writing dropped out of view.
The composer now has two owners, and neither can move out from under an operation still
running against it:
not Host state — nothing persists them and nothing restores them per target — so keying them
by the target only made them vanish when the picker moved. A stable key is also the only
owner an in-flight send can safely have:
send()captures the key it submitted from andclears exactly that key once it resolves, so a key that followed the picker would leave the
files it had already delivered staged under the new target, ready to be sent twice.
specific target, and is carried to the target you pick.
ChatComposerRegiondefers thatcarry while a send is settling, so the submission owns the text it submitted until the
completion clears it — and text typed after that send is carried once it does.
pickAttachmentsresolves its owner after the native file dialog closes rather than capturingit before opening: the surface can change while a dialog is up, and files you just chose belong
in the composer you are looking at, not in a bucket you have left where they would be invisible
but still sendable.
Two smaller notes: the draft carry is unconditional, including the empty case, so the target
you arrive at holds what you brought and nothing else — otherwise a copy left under a key the
composer passed through resurfaces later as that target's own draft. And three imports gained
.jsextensions sonode --testcan load the staging hooks, which is what the tests need toreach them.
Fixes #3408
Verification
The same moment in both shots: a draft typed on the new-task surface, immediately after
choosing a different target in the workspace picker (both show 无项目 selected).
e2e/new-task-draft-target.spec.ts(new) — a real window, for both halves: type then pick aProject, and drop a file then pick a Project. On
mainthe first fails withReceived: ""and the second with
Expected: 1, Received: 0.chat-composer-region-draft-handoff.test.ts— 7 tests, 2 of which fail onmain. The restguard the new behaviour, including the two that cover a target change during a send: the
submitted text ends up in neither slot, and text typed after it is still handed over.
new-task-staged-content.test.ts(new) — Session isolation, a completing send clearing whatit submitted, and files chosen in a dialog landing in the composer now on screen.
npm run lint,npm run format:check,npm --workspace @maka/desktop run typecheck: clean.Two suites here are not fully green, and neither set is this change:
npm --workspace @maka/desktop run test: 13 failures, an identical set on a cleanmaincheckout —
EBUSYunlinking SQLite files, and a shell-script fake CLI.prompt-rail:160,settings:51,sidebar-project-row:13,skill-draft-lifecycle:67and:91,session-workbar:150) failon plain
origin/mainhere too; I ran them there to check. The seventh,accessibility-coverage:24, times out at exactly the 60s limit under full-suite load andpasses in isolation.
My host is Windows x64, which CONTRIBUTING lists as an unsupported target — that is what both
sets above are — so I have not run a packaged-app check either.
AI use
Select exactly one:
Tool(s) and scope: Claude Code (Claude Opus 5) — root-cause tracing, the fix, the tests and a
draft of this description. Reviewed, run and verified locally by me.
Checklist
Does this PR entail a change in behavior?