Repository navigation
Conversation
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: 76c52aa2bb
ℹ️ 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".
76c52aa to
a0ca200
Compare
|
Parked at
Generated with |
a0ca200 to
72a15a9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72a15a919c
ℹ️ 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".
|
Parked at
Generated with |
…(transcript-only, second Edit)
…aft, after the unsent draft
72a15a9 to
e8acc69
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e8acc69cd8
ℹ️ 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".
| const editSessionRef = useRef<EditSession | null>( | ||
| workspaceId ? (editSessions.get(workspaceId) ?? null) : null | ||
| ); |
There was a problem hiding this comment.
When the user switches away and back while an edit send is still resolving, the keyed ChatInput remounts and restores this session with sendInFlight: true, but its local sendingCount resets and canSend never checks the restored session flag. Because the original component deliberately continues submitted sends after unmount, the resumed edit can enable Send with the original buffer and launch a second edit request before the first settles, producing competing replacements or a spurious history conflict. The remounted composer needs to derive its send-disabled state from the persisted edit session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid on e8acc69. The edit session survives the composer remount with sendInFlight: true, but sendingCount is local useState (index.tsx ~467), and canSend (~1024) never reads the restored session flag. So the remounted composer can send the same edit a second time. Not fixed: this is a new defect in the edit-preservation mechanism, and the hard stop applies. #5801 is parked, and the thread stays open.
| if (!session || session.settled) return false; | ||
| session.settled = true; | ||
| const { preEditDraft, preEditReviews } = session; | ||
| settleEditSession(session); | ||
| const { preEditReviews } = session; | ||
| if (dropEditReviews) setDraftReviews(null); |
There was a problem hiding this comment.
If an edit send starts, the user switches away and back before its reply, and the original send then succeeds, this runs in the now-unmounted composer: it marks the shared session settled and applies setDraftReviews only to that dead instance. The live composer subsequently sees the cleared edit target, but releaseEndedEdit refuses the already-settled session, leaving the edited message's review override active; the next ordinary draft can therefore send those notes again instead of restoring the pre-edit reviews. The successful completion must publish the review restoration to the currently mounted composer before settling the shared session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Valid on e8acc69. A send accepted after a switch away and back settles the shared session in the unmounted composer, which applies setDraftReviews only there. The live composer's releaseEndedEdit then skips the settled session, so the edit's notes stay as the override. Not fixed: hard stop. #5801 is parked, and the thread stays open.
| const editingByWorkspace = useSyncExternalStore(subscribeEditTargets, readEditTargets); | ||
| const editingMessage = editingByWorkspace[workspaceId]; |
There was a problem hiding this comment.
When a history-changed transcript refresh returns failed, its retry precondition is kept only in the current ChatInput's editRefreshRetry state, while this change preserves the invalidated edit target across workspace switches. Switching away and back after that failure remounts the composer, discards the retry state, and restores an edit with preconditionInvalidated: true; Send remains disabled, the indicator incorrectly says the refresh is still running, and no retry button is available. Persist the retry data with the edit target or restart the refresh when restoring the edit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
|
Parked at
Generated with |
This branch has not been deployed
1 participant
Summary
Editing a sent message no longer writes the edit text into the workspace's shared, persisted draft. The edit text now lives only in the editing window's memory. A reload during an edit keeps the unsent draft (#5672), and an edit in one window no longer shows up in another window's composer (#5571). An open edit now also survives a workspace switch: switching away and back shows the same edit with its typed text and attachment changes, and the unsent draft stays intact (#5808, item 1). A reload drops the open edit itself. That is the chosen tradeoff (Option A).
Fixes #5672
Fixes #5571
Part of #5808 (item 1 only; items 2 and 3 existed on main and stay open)
Background
Before this change, entering edit mode saved a snapshot of the unsent draft (
preEditDraft) and then replaced the shared draft with the message text. The shared draft is persisted and synced across windows. So a reload during an edit restored the edit text as the draft and lost the unsent draft. A second window showed the edit text in its normal composer.Implementation
useComposerDraftholds a memory-only edit buffer, kept per workspace in module memory and read withuseSyncExternalStore. While an edit is open,setInputandsetAttachmentswrite to that buffer, and the composer renders it. The shared draft inDraftStoreis not touched. A buffer left behind by an edit that ended without settling is ignored.ChatInputInnerfills the buffer withbeginEditDraftwhen an edit starts, and drops it withendEditDrafton cancel or accepted send.EditSession.preEditDraftand its restore code are gone, and edit entry no longer waits for draft attachment payloads.history-changedrefresh found no target.releaseEndedEditthen keeps the edit's text, attachments and notes as a normal draft, after the unsent draft. On main they became the draft and replaced the unsent draft. An edit whose send is in flight is left to that send, because an accepted edit replaces its row before the reply.EditSession.sendInFlight(memory only) marks that state.tests/ui/chat/composerDraftsFormalRepro.test.ts› "keeps a staged attachment in the edit once across a refused send, out of the shared draft" shows it: a refused edit send keeps its text and its staged file once (staged one time, one chip), and both the in-memory and the backend shared draft hold only the unsent draft's attachment. It replaces the deleted test "keeps the staged copy of an attachment another window saved again as pending", whose premise (another window holds the edit) no longer exists.useComposerDraftcopieseditMessageIdinto a ref in auseLayoutEffect, not in the edit-open handler. The two are not equivalent. An edit can end without any composer handler running (ChatPane clears it when the row is replaced or deleted). Then the ref must follow the prop, or later keystrokes go to the stale edit buffer and vanish. With the copy moved intobeginEditDraft, typing after the row is deleted is lost (the row-deleted test fails). A render-time assignment would also expose an uncommitted render's ID, so this matches the existingeditingMessageIdReflayout effect inChatInputInner.ChatPane.tsx's[workspaceId]effect cleared the edit). The edit text then survived only because it had overwritten the unsent draft. This PR now keeps the edit's identity and content across a switch without touching the unsent draft:editTargets, read withuseSyncExternalStore), instead of oneuseStateslot that a switch cleared. It is not React state, because WorkspaceShell shows a loading placeholder while a returning workspace loads, and that unmounts ChatPane. A no-op update returns the same object and notifies nobody. An entry goes when its edit ends.editDrafts) and session (editSessions) per workspace in module memory too, so the remounted composer resumes the same edit. The edit's own notes are kept in the session when the composer unmounts.hasOlderHistoryis true. Its send still carries the rows precondition, so a row that really changed is refused withhistory-changedand handled as before./compact) clears only its own edit buffer. It replaces its row before it clears, and the clear used to reach the unsent draft's files.setEditingByWorkspace) is the only writer of edit targets. It callskeepUnsettledEditInDraft(useComposerDraft.ts) for every target it clears or replaces. The rule takes the buffer first, so it runs at most once per edit. It adds no store and no persisted field. Audited target-clearing paths:setEditingMessage(undefined)from Cancel/Escape (handleCancelEdit): already settled, no buffer left.setEditingMessage(undefined)after an accepted send, including/compact: already settled.releaseEndedEditcovers the composer's own session when it is mounted).history-changedfollowed bytarget-not-found(onCancelEdit): the same, through the rule.transcriptOnlyeffect, andsetEditingMessagewhile transcript-only): the rule keeps the contents. This was a review finding.beginEditingMessage→setEditingMessage): the first edit's contents are kept before the second fills its buffer. This was a review finding.src/browser/utils/chatEditing.ts(thecanEditDisplayedUserMessageguard from 🤖 fix: Token Budget warning rows are never turn triggers #5792 is unchanged), and edit target row selection. Assistant and streaming paths are unchanged.Validation
tests/ui/chat/editKeepsUnsentDraft.test.ts: "a reload during an edit keeps the unsent draft (🤖 Reloading during a message edit replaces the unsent draft with the edit text #5672)" and "an edit in one window does not reach another window's composer (🤖 Composer: Edit in one window also loads the message into another window's composer #5571)". Both failed on main and pass here.DraftStore.setTextandsetAttachmentsare never called. A mutation that writes the store on each edit keystroke fails it.editKeepsUnsentDraft.test.ts), each failing before the change:hasOlderHistorytrue. The edit stays.editKeepsUnsentDraft.test.ts): "an edit keeps its text and files in the draft when the workspace turns transcript-only" and "a second Edit keeps the first edit's text and files in the draft". Mutation check: both fail with production at 72a15a9, and both fail when only thekeepUnsettledEditInDraftcall is removed.tests/bugbash/repros/editCancelKeepsDraft.e2e.ts, phone target, 20 runs each (mock app AI,--retries 0). main (9bdcf1e): 20/20 passed. This branch: 20/20 passed on 2b0734913c. The pushed head 72a15a9 is that commit rebased onto main 752f962, whose only overlap is an unrelated keybind change in ChatInput; the repair files are identical. main did not fail, so this PR does not claim to fix 🤖 tests: phone repro editCancelKeepsDraft once showed an empty composer after Cancel #5810.releaseEndedEdit.knownFailureReloadDuringEdit.e2e.tsis renamed toreloadDuringEditKeepsDraft.e2e.tsand loses itsknown-failuretag.make static-checkpasses.make check-react-compiler: 23/24 hot components compile (1 known skipped), the same as main.Before, window B's composer read "message to edit 81" and the reloaded window lost "my unsent draft". After, both kept "my unsent draft".
formal/composer-draftsdoes not model edit mode. Edit mode now never writes the shared draft, so the modeled behavior is unchanged, and I did not reruncheck.sh.Size
The PR is +738/-169, above the usual 500-line review size. Most of it is tests (+466/-94). The workspace-switch repair adds +363/-44 (production +166/-43 in ChatPane.tsx, ChatInput/index.tsx and useComposerDraft.ts). It repairs a loss that this PR introduced, so it belongs here. The round-2 review fixes (
releaseEndedEdit) repair defects that this PR introduced (an edit that ChatPane ends on its own lost its edit buffer), so they belong here.EditSession.sendInFlightis a memory-only field on the in-memory edit session. It is not persisted and adds no on-disk state.Risks
Medium, limited to the composer's edit mode. A reload or crash now drops an open edit's text instead of keeping it. A workspace switch keeps it. The module-level edit stores are memory-only and keyed by workspace. An entry left by a workspace removed mid-edit is small and stays until reload. The unsent draft is the data this change protects. Send-failure put-back for edits now targets the edit buffer. The tests above cover refusal, cancel during a pending send,
/compactedits, and replaced rows.Generated with
xum• Model:anthropic:claude-opus-5-5• Thinking:high• Cost:$26.66