fix(web): an edit saved during dispatch no longer disappears (#40 A5) - #82
Merged
Conversation
The drain consults the editing hold only when it PICKS a head (`useThreadOutboxDrain.ts` — the `editingQueuedMessageIds` check sits in the selection loop, above `beginDispatchingQueuedMessage`). A hold taken after that point does not stop the send already under way, and `threadOutboxManager.update` correctly returns `false` for a message that has left the queue — but `saveEditing` ignored the return and closed the editor as if it had saved. Net effect: the message went out with its ORIGINAL text, the user's revision was gone from the queue, the editor, and the composer, and nothing anywhere said so. Closed from both sides: * `beginEditing` refuses to open the editor for the row currently being dispatched. Selection and `beginDispatchingQueuedMessage` are synchronous within the same effect, so the atom covers the whole async send with no gap. * `saveEditing` honours the boolean. On `false` it logs through the module's own `[thread-outbox]` breadcrumb helper and raises a warning toast saying the message already went out with its original text. The row unmounts either way — it renders from the queue — so the news has to leave the component. `logOutboxMutationFailure` gains `"update"` beside `"reorder"`; a no-op update is exactly as invisible to the user as a rejected reorder, which is what that helper already exists for. Both changed files are fork-owned; the upstream-owned file count and every deletion count are unchanged. Verified: `vp test run apps/web/src/outbox` 41 passed; `vp lint` clean; `tsgo --noEmit -p apps/web` 0 errors. Not unit-tested at the component level — apps/web has 34 component tests and every one is `renderToStaticMarkup`, so there is no way to drive a click here without adding an interaction-test dependency. The contract this leans on (`update` resolving `false` once the message has left the queue) is already pinned by threadOutbox.logic.test.ts:442.
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Partial for #40 — section A item 5. Independent of #81 (verified: the two branches auto-merge, only
outboxDiagnostics.tsoverlaps and the hunks are ~20 lines apart).The race
The drain consults the editing hold only when it picks a head — the
editingQueuedMessageIdscheck sits in the selection loop inuseThreadOutboxDrain.ts, abovebeginDispatchingQueuedMessage. A hold taken after that point does not stop the send already under way.threadOutboxManager.updatehandles its side correctly: it is a documented no-op for a message that has left the queue and returnsfalseto say so. What was missing is thatsaveEditingignored the return and closed the editor as if it had saved.Net effect: the message went out with its original text, the user's revision was gone from the queue, the editor, and the composer, and nothing anywhere said so.
Closed from both sides
beginEditingrefuses to open the editor for the row currently being dispatched. Checked the ordering: selection andbeginDispatchingQueuedMessagehappen synchronously within the same effect, sodispatchingQueuedMessageIdAtomcovers the whole async send with no gap on either end.saveEditinghonours the boolean. Onfalseit logs through the module's own[thread-outbox]breadcrumb helper and raises a warning toast saying the message already went out with its original text, so the revision can be re-sent. The row unmounts either way — it renders from the queue — so keeping the editor open is not an option; the news has to leave the component.logOutboxMutationFailuregains"update"beside"reorder". A no-op update is exactly as invisible to the user as a rejected reorder, which is the case that helper already exists for.Seam cost
None. Both changed files are fork-created (
git cat-file -e <merge-base>:apps/web/src/components/chat/ThreadOutboxQueueList.tsxfails;apps/web/src/outbox/does not exist upstream at all). The upstream-owned file count stays at 37 and no deletion count moves, soSEAMS.mdneeds no edit.Verification, and one honest gap
vp test run apps/web/src/outbox— 41 passed.vp linton both files — clean.tsgo --noEmit -p apps/web— 0 errors.Not unit-tested at the component level.
apps/webhas 34 component tests and every one of them isrenderToStaticMarkup— there is no@testing-library/react, nofireEvent, nouserEventanywhere in the package, so there is no way to drive a click on the Edit button without adding an interaction-test dependency. That is a call for you, not something to slip into a cleanup PR. I did not manufacture a pure-helper wrapper arounddispatchingId === messageIdjust to have something to assert on.The contract this change leans on is already pinned:
threadOutbox.logic.test.ts:442assertsmanager.update(...)resolvesfalseonce the message has left the queue ("stale flush"). What was untested was the caller honouring it, and that is what this PR fixes.Still open on #40
A6 (a refused steer loses the composer text) is deliberately not here.
recoverTurnStartFailurelives inapps/server/src/orchestration/Layers/ProviderCommandReactor.ts— an upstream-owned server file — and the server cannot put text back in a composer anyway. A real fix means the client reacting toprovider.turn.start.failedby re-populating the composer and draft store, which is a new client behaviour rather than a leftover, and wants its own design. Section B and C1 also remain.