Repository navigation
Conversation
Adds T1-T23 (32 cases) for keeping an open edit's text and files in the composer's memory (#5672, #5571), a strictMode option for renderApp and createAppHarness, and the changed existing edit tests (typeIntoEdit, D3). Replaces the two #5501 edit put-back repros in composerDraftsFormalRepro. Production code is unchanged in this commit: most new tests fail on main. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
While an edit is open, its text and files live only in the composer (window-local). The shared persisted draft keeps the unsent draft, so a reload, a second window, a workspace switch or a crash during an edit no longer loses it (#5672, #5571). - useComposerDraft holds the edit's text and files in local state and writes nothing to DraftStore while the edit is visible. When the composer unmounts with an open edit, the edit is appended after the unsent draft once. - Inserts while an edit is visible (Quote in input, palette, MCP App messages) append to the edit; file appends and late file adds are refused instead of replacing the draft behind the edit. - Restores during an edit send (queued card Edit, backend restores) merge in front of the shown unsent draft; their notes go to the review store. - A rejected command phase puts an editing command's input back once. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
- T10: edit in the other workspace before coming back, so ChatPane stays mounted while the send is pending. - T17(b): hold the edit send before the backend (its target still shows); the busy stream changes the history, so the send is refused. - T21: the rejected phase still leaves `void handleSend()` unhandled, as on main; the test stops that rejection at the send's outer wrapper. - T22a: install DOMRect for the context-menu popover. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
With the edit's target still shown, the edit keeps its own buffer behind the shown unsent draft, so the quote test now tells "the visible edit" apart from "the real target" (M25). T17(b) uses the same helper. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
The fix keeps an open edit out of the shared draft, so the #5672 repro passes. It loses its known-failure tag and header. --- _Generated with `xum` • Model: `anthropic:claude-opus-5-5` • Thinking: `high`_
React Compiler skips ChatInputInner when it writes a ref before its declaration.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eda9ae1e3f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
A text-only command such as /vim emits clear-input only. As on main, the edit keeps its files.
📋 Implementation Plan (sha256 427d0c55…f865)Option C plan: keep the edit out of the shared draft (#5672, #5571)1. SummaryThe plan fixes #5672 and #5571 with one rule: an open edit's text and files stay in the composer's memory and never reach the shared, persisted draft. The unsent draft then survives a reload, and a second window never sees the edit. While an edit is visible, every insert goes into the edit or is refused, so nothing writes the hidden draft (section 3, insert-path audit). It is a new PR from main, not a repair of #5801's edit lifecycle.
2. Causes on mainI read main at One cause, three symptoms: entering an edit writes the edit into the shared draft.
While an edit is open, 6 programmatic paths on main write into the edit (in the shared draft) with no edit guard: 3. Rule and designRule.
One owner at a time. The unsent draft never leaves the DraftStore. The edit's text and files have exactly one owner:
Not changed: Insert paths while an edit is open (audit)Source: the Diff Inventory Auditor's report (
Result: while an edit is visible, no path writes U. The one write to U behind an edit is the step 16 race, and it keeps Q. 4. BehaviorU is the unsent draft, E the edit's text and files, Q a restored queued message. "After U" means appended once, below the unsent draft. "Expected" values for main come from reading the code. The failing-first runs (section 9, step 3) confirm them.
Visible changes from main. Each comes from a decision or from the audit in section 3:
D2-B keeps main's destination for quotes, palette inserts and MCP App messages: the visible edit. 5. Kept from R2, and scope reductionsR2 is the behavior the user approved for #5801: a switch ends the edit, and its text, files and notes move once into the draft. Kept:
Scope reductions. Keeping these would need R2's notes routing and ChatPane guards.
6. TestsNew UI tests go in "Expected on main" comes from reading the code. Section 9, step 3 runs the tests on main before the fix and records the real result. Each mutation is applied to the fix commit and must make its test fail.
Test 477 shows that the #5226 guard refuses a reopened edit while the send waits. So no replacement edit can exist during a pending edit send, and no test needs one. T1 details. The test spies on T11 is a required gate. Changed existing tests. The Test Impact Analyst report (
Kept unchanged: T22 (a) opens the transcript menu with a Counts. 23 new scenarios (T1-T23) with 32 cases: T3, T7, T10, T15, T17 and T19 have 2 each, and T22 has 4. 27 mutations (M1-M27; M4 covers T4 and T9, M9 is T6's refresh check, and M24 covers T22 b and c). T21 makes the API call that 7. Earlier #5801 findings and this design#5801 used 14 assessments (an assessment is one review or check run). "Applies" means the same user flow exists with this plan.
Residuals that exist on main and stay out of scope (already in #5808): the #5226 guard is lost when ChatPane itself remounts, for example behind the WorkspaceShell placeholder (item 4), and a non-edit notes override is lost on unmount (item 5). Step 11 sends notes restored during an edit to the review store, which likely also resolves item 6 (Cancel drops notes restored into an open edit). Item 6 stays unconfirmed: the PR claims it only if T17 also covers a Cancel and shows that the restored notes survive once. Main's late 8. Size (estimate vs main)
The first draft estimated +100/−30. Four advisor passes added about 66 lines: send and command ownership, the stop rule, the command D2-B and the insert audit add about +28/−4: The size gate (section 9, step 6) is a proposed ceiling of +225/−55: the estimate plus about 15%. It was +190/−50 before D2-B. The ceiling is close to R2's +228/−67. The net estimate (+150) is below R2's net (+161), but the plan does not claim that the change is well under R2. The perf clearance must accept both the estimate and the ceiling. 9. Delivery and review budget
Review budget (D4-B, decided).
Hard stop. Park the PR with a comment and send a terminal report when any of these happens: a valid finding that needs a new mechanism (a store, gate, guard or persisted field), a third push, a final-check blocker, or a failing required check that is not a proven flake. The GO is then revoked. 10. For the perf owner (8e18f4aada)Production files and functions this plan changes:
Not touched: everything else in Size to clear: about +194/−44 (net +150), with a ceiling of +225/−55 (section 8). The ceiling is close to R2's +228/−67. Per-keystroke path.
Test-only files: 11. Acceptance criteria and dogfoodingAcceptance criteria.
Dogfooding. Build once with
The dogfood run writes evidence to 12. DecisionsD1. Notes when an edit ends without a send
D2. Inserts while an edit is open
D3. The composer while an edit send is in flight
D4. Review budget for the new PR
Generated with |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1cddc014a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| editingMessageForUi && editDraft?.id === editingMessageForUi.id ? editDraft : null; | ||
| const input = visibleEdit ? visibleEdit.text : draft.input; | ||
| const attachments = visibleEdit ? visibleEdit.attachments : draft.attachments; |
There was a problem hiding this comment.
When the persisted draft has attachments whose payloads are still loading, the edit-entry effect waits at draftPayloadsLoaded, so editingMessageForUi is set while editDraft remains null. These fallback lines consequently show the normal unsent draft under the “Editing message” UI, and setTypedText writes any typing into that shared, persisted draft; once ensurePayloads() finishes, the historical edit suddenly replaces it. Initialize the edit buffer independently of the normal draft payload load, or disable the edit textarea until the buffer is ready.
Useful? React with 👍 / 👎.
| // An edit that ended meanwhile (switch, Cancel, lost target): after the unsent draft. | ||
| if (!isLiveEdit(editSessionForSend)) { | ||
| return appendSnapshotToDraft({ text, attachments: sendAttachments }); |
There was a problem hiding this comment.
If the user appends text while an edit send is preparing, the take removes only the captured prefix and leaves the new text in the edit buffer. When the edit then ends after the take—such as by switching workspaces—and the backend subsequently refuses the send, cleanup has already appended that newer text to the draft, and this branch appends the failed prefix after it. That reverses old edit → later text into later text → old edit, unlike the live-edit restoration path immediately below; the taken snapshot needs to be inserted before the moved remainder.
Useful? React with 👍 / 👎.
| } | ||
| } | ||
| setDraftReviews(pending.reviews); | ||
| if (duringEdit && onAddReviewForRestore) pending.reviews.forEach(onAddReviewForRestore); |
There was a problem hiding this comment.
When an edit starts while the normal draft already has a non-null review override—for example after editing a queued message into the composer—a queued or held-input restore during that edit puts its text into the shared draft but sends its notes only to the review store here. Cancel then restores session.preEditReviews; because useComposerDraft gives that override precedence over attached store reviews, the newly restored notes become hidden and are omitted when the merged draft is sent. Merge these notes into the saved pre-edit override, or move all of that override into the review store before restoring it.
Useful? React with 👍 / 👎.
|
Parked at the review budget's hard stop. This PR stays open as a draft.
Generated with |
|
Closing unmerged. The user decided to close this PR without merging (relayed by the coordinator desk on 2026-10-09). Record
The other two open findings
Conclusions of the simplification report
What stays
Generated with |
1 participant
Summary
An open edit's text and files now stay in the composer's memory and never reach the shared, persisted draft. A reload during an edit then keeps the unsent draft (#5672), and a second window on the same workspace never sees the edit (#5571). While an edit is visible, every insert goes into the edit or is refused, so nothing writes the hidden draft.
Fixes #5672
Fixes #5571
Background
On main, entering an edit copies the unsent draft into a React ref and writes the edit text and files into the workspace's shared DraftStore draft (
applyDraftFromPendingin the edit-entry effect). The DraftStore persists that todraft.jsonand broadcasts it to every window. A reload or a second window therefore shows the edit text as the draft, and the unsent draft lives only in the lost ref.This PR replaces #5801, which is parked at its final check and is now a draft. #5801 used 14 assessments. Its final-check blocker was a queued message restored during an edit send that made the draft appear twice. Here the test
a queued message restored during an edit send shows once next to the unsent draft(T3, two cases) covers that flow. This PR starts from main, not from the #5801 branch.Implementation
Rule: normal draft APIs always read and write the workspace's normal draft. Edit input lives only in the window that opened the edit and changes only through explicit edit operations. No setter redirects a normal draft write into the edit buffer.
useComposerDraftgets an in-memory edit buffer (useStateplus a ref) and explicit operations:beginEditDraft,setEditText,setEditAttachments,dropEditDraft,consumeEditDraftIntoDraftandappendSnapshotToDraft. No new store, module or persisted field.ChatInputshows the buffer while the edit is visible, and the draft otherwise. Typing, voice and suggestions route by the visible edit at call time./compactowns its submitted snapshot. Acceptance consumes it. Refusal puts it back once: into the edit if the same edit is still open, else after the unsent draft.Decisions
ChatInputAPI.appendToVisibleInput. Palette/inserts and MCP App messages use it too, so they land where the user sees them, as on main.Scope reductions (vs #5801's approved behavior)
hasOlderHistory)Insert paths while an edit is open
"Visible" means the composer shows the edit. During an edit send the edit is dismissed and the composer shows the unsent draft (D3-A), so writes there are visible.
/insert (UPDATEappend)append, withworkspaceId)appendwith files or notes (no dispatcher today)/and@suggestionsreplace)Two known windows (both visible, nothing hidden is written):
clearQueueround trip gets the queued message merged in front of the unsent draft behind it. Nothing is lost, and the message shows when the edit ends. On main the same race replaced the new edit's text.Size
Production diff vs main: +244/−55 in 5 files. The user raised the ceiling from +225/−55 to +231/−55 for the React Compiler fix below, and to +244/−55 for the review-round-1 fix (see "Review round 1").
src/browser/features/ChatInput/index.tsxsrc/browser/features/ChatInput/useComposerDraft.tssrc/browser/features/ChatInput/useComposerAttachments.tssrc/browser/features/ChatInput/types.tssrc/browser/components/ChatPane/ChatPane.tsxeditSessionRefandappliedEditIdRefare declared above the unmount cleanup that clears them. React Compiler skips a component that writes a ref before its declaration, and the move counts +5/−5.Perf
useStateupdate and one ref write. No DraftStore write and no backend save. The testa reload during an edit keeps the unsent draft and its files (#5672)(T1) spies onDraftStore.setTextandsetAttachmentswhile it types into an edit and expects 0 calls.appendToVisibleInput,isEditVisibleand theattachFileslanding check run per action only.make check-react-compiler:Review round 1
Codex found one valid P2 defect that this PR introduced (thread at
index.tsx:2137). An editing slash command that clears only its text, such as/vimor/model, dropped the edit's file. The command takes the edit's text and files when it starts (plan step 9), and on "consume" nothing put the files back. On main the edit keeps them.Fix (
a12dad2c3f, +13 lines): a command consumes only what it clears.clear-inputclears its text, andclear-attachmentsclears its files. On "consume", and on "restore-if-empty" when the edit already holds new text, the parts the command did not clear go back once: into the same edit, or after the unsent draft.Tests: T24 "a text-only command typed into an edit keeps the edit's file" (real
/vim; it failed before the fix), T25 (5 cases, "a held text-only command (, ) returns the edit's file once") and T26 (2 cases, "a held command that clears attachments () brings no file back"). Mutations M28-M32 each make at least one of them fail.Known limit (untested): no command sends
clear-attachmentstogether with "restore-if-empty". The 3 commands that clear files all end with "consume". If one ever did, and the edit text was empty, the whole snapshot would come back, files included. On main the files would stay cleared.Validation
.catchrestore on both/compactphase calls, because the fix catches and rethrows instead oftry/finally.e1cddc014a) reranmake static-check,make check-react-compilerand the edit test files, all exit 0:make static-check: exit 0.make check-react-compiler: exit 0 (output under Perf).editKeepsUnsentDraft.test.tsandcomposerDraftsFormalRepro.test.ts: 48/48, exit 0.tests/ui/chatplustests/ui/layout/rightSidebar.test.ts: 134 passed, 1 failed, 23 skipped. The failure issendModeDropdown"send after step waits for every tool call emitted in the current step" with "Expected the source turn to be streaming before queueing", the 1 s stream-start wait tracked in 🤖 tests: sendModeDropdown 'send after step' waits only 1 s for the stream to start #5807. Reruns of that file alone: all tests passed in 3 of 3 runs on this branch and 3 of 3 runs on main.src/browser/features/ChatInput,src/browser/components/ChatPaneanduseAIViewKeybinds: 215 passed, 1 failed. The failure (useAIViewKeybinds"desktop canvas owns capture-phase resume, chat focus, editor, and terminal shortcuts") also fails on main's code locally.BUGBASH_AI=mock make test-bugbash-repros: exit 0, 50 tests selected across 2 phases.tests/ui/chat/newChatStreamingFlash.test.tsfails locally on main too (cd2837ab91: 2 of 3 tests fail with "Created workspace not found yet"). On this branch it failed 2 of 3 in each of two reruns before the final gate run, with the same message, and passed in the final gate run. Main CI passed it in its last 5 runs, so CI is the check of record for this file.tests/bugbash/repros/editCancelKeepsDraft.e2e.ts, phone target,--repeat-each 20 --retries 0 --workers 1, mock AI):cd2837ab91: 20 executed, 20 passed, 0 failed, 0 flaky.tests/bugbash/repros/knownFailureReloadDuringEdit.e2e.tsloses itsknown-failuretag and header, and passes in the repro run above.Test notes
tests/ui/chat/editKeepsUnsentDraft.test.tsscenarios T1-T26 (40 cases). T18 is insrc/browser/features/ChatInput/useComposerDraft.test.ts.typeIntoEditinstead ofgetDraftStore().setText. Tests 402, 534 and 572 change their assertions for D3-A. Two tests incomposerDraftsFormalRepro.test.tsare replaced by T2 and T5, because an edit can no longer merge with edit text that another window saved.renderAppandcreateAppHarnessgain astrictModeoption (T11).runWithFinally. jest's sandboxedprocessdoes not see real unhandled rejections.DOMRect, which the context menu needs and happy-dom lacks.Risks
Medium. The change touches the composer's edit lifecycle, which #5801 found hard to get right. Affected areas: edit mode, queued-message restores during an edit,
/compactedits, and composer inserts. Failure modes are lost or duplicated draft text. The tests cover every move of the edit text, and each mutation proves its test. Notes keep main's behavior (D1-A), and no persisted format changes, so an upgrade or downgrade needs no migration.Follow-ups
The implementation plan (sha256
427d0c55411aae85140272a71ce2a9e48710d95d94ee6d452682071864d2f865) is in this comment: #5893 (comment) (GitHub's body size limit no longer fits it here).Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$154.71