fix(chat): keep the optimistic user bubble across a slot refetch - #6825
fix(chat): keep the optimistic user bubble across a slot refetch#6825rnoack1 wants to merge 1 commit into
Conversation
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
1 similar comment
|
👋 Hi! This PR's description is missing some required sections from our PR template. Workflow runs won't be auto-approved until the description is updated. Missing sections:
Please update your PR description to include these sections, then push or re-save the description. The workflows will be approved on the next cycle. |
UX Review (Fable 5.1, fork) — 🟡 CONCERNSUX-level review of Confirmed the fork lane: the two screenshots this PR adds ( UX-Verdict: CONCERNS New delivery-warning surface (dimmed bubble, composer caption, three action buttons) is unseen by any cold reader in this fork lane, so comprehension is unverified. Watch
Evidence gaps
[UX-REVIEWED] e4923de |
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 1 of 1 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands. BLOCKING -- website/src/utils/chatPaneRecovery.ts:158 -- slot-only marker lets another tab clear the duplicate-send warning Adjudication (Fable 5.1) — is blocking on each finding proportionate?API Error: 400 Claude Code 2.1.240 does not support this model; version 2.1.255 or newer is required. Run 'claude update', or update the Claude desktop app, then try again. |
First Principles Review (Fable 5.1, fork) — 🟡 CONCERNSPremise-level review of I have enough to complete the review. The First-Principles-Verdict: CONCERNS A What this change shipsIntent: keep the user's just-sent bubble on screen when a slot refetch resolves before the server persists it. Fundamentally a FIX.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] e4923de |
Design Review (Fable 5.1, fork) — 🟡 CONCERNSDesign-level review of Design is sound and I've confirmed the mechanism (retention marker, precedence in one accessor, spec updated in-commit). My concerns are scope and one declared product trade. Design-Verdict: CONCERNS Sound fix for a real bug, but it bundles several independent logical changes into one commit — including an admitted second defect — and ships a product trade a human should ratify. Watch
[DESIGN-REVIEWED] e4923de |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
787e318 to
5bd4536
Compare
5bd4536 to
b75621c
Compare
b75621c to
7970b0c
Compare
Head branch was pushed to by a user without write access
1faf116 to
ce55670
Compare
ce55670 to
b8dd77f
Compare
b8dd77f to
9fdd5e0
Compare
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
Position an unanchored send past the server-identified rows rather than by timestamp, so a client clock behind the server cannot seat a new prompt above the history it followed. Hand the payload back on every page arm that ends without a receipt, the 10s abort included, since the bubble is store-only and a reload keeps neither it nor the draft cleared before the POST; name the unconfirmed state in one shared string, and retire it on edit history rather than value equality, re-checked whenever a restoration stages, so an echo that confirms before the abort cannot strand a delivered prompt where a resend duplicates the turn.
Problem / Motivation
A slot-detail refetch rebuilds
messagesfrom the fetched page and drops the user's just-sent message.The composer appends the user's row client-side at send time (
appendMessage, flaggedmeta.optimisticbecause it carries asendId), so it is on screen before the server has persisted anything. Both refetch reducers then rebuild the list, and both preserve some local rows the page may predate:switchSlot.fulfilledre-attaches a trailing local reply only when it is anassistant/streamingrow, otherwise falls through tonext = preservedrefreshSlot.fulfilledre-injects onlypermissioncards over the fetched pageNeither preserves a trailing
userrow. So a refetch resolving in the window between the optimistic append and the server's own append silently discards the bubble.Why it matters
isWelcomeStaterequires!slotLoading, so oncemessagesis emptied the pane renders blank with a spinner rather than the welcome hero. The composer has already cleared. The message reappears only when the turn's first reply lands, which on a first turn can be 10+ seconds later.The net effect is that a slow turn is indistinguishable from a lost message — the user has no evidence their send was accepted.
What changed
One shared guard, applied at both call sites, re-attaching sent user rows the fetched page cannot legitimately contain.
Retention is set where the composer renders a bubble ahead of the server (
meta.pendingServerRow) and is released by exactly two things: the row's own identity appearing in a fetched page, or an explicit outcome dispatched throughclearPendingServerRow. Nothing else retires it — not a wall clock, not a row count, and not dispatch order. That last exclusion is deliberate and was the subject of two earlier iterations here: neither a timestamp nor the client's dispatch sequence says when the SERVER took its snapshot, so a refetch issued after the send can still have read the transcript before the POST committed, and treating its page as proof of absence deletes a delivered prompt. The explicit outcomes are exactly two, and both come from the SERVER: a refusal, and a queued acceptance (whose queued twin owns the message). A transport failure never retires the row, in either direction — no browser signal proves the POST did not leave,navigator.onLineleast of all, since it is read when the exception surfaces and so reads false on a connection that dropped after the bytes went out. The cost is stated rather than hidden: a send that genuinely never left leaves a bubble that no refetch clears, which is the deliberate trade against deleting a delivered prompt. A superseded same-slot response is discarded outright by the shared dispatch clock, so an older refetch cannot rebuild the transcript without a row a newer one already persisted.Four further changes ride here and are called out rather than left implicit:
state.slotLoading = falsemoved above the supersede guard inswitchSlot.fulfilled— a real timing change. It is placed deliberately between the guards: BELOWactiveSlot !== key(where a newer switch owns the flag and must clear it itself, so clearing here would lie about that switch) and ABOVE the superseded return (where nothing else ever clears it —refreshSlot.fulfillednever touches the flag — so a discarded response would leave a permanent spinner). The remaining early return,isUnsafeKey(key), did not clear the flag before this change either, so its behaviour is unchanged.ChatPagesites (refusal, abort, transport) plus the session-create failure. This is a SECOND defect fixed here, beyond the stated bug, and it changes WHERE a failure row lands: previously the row was appended to whichever slot was active when the exception surfaced, so switching slots mid-send filed the error in the wrong transcript. The sending slot is captured before the first await and used for every failure arm.meta.deliveryConfirmedmarker (absence cannot carry this: a DROPPED row is absent too, and clearing on absence would destroy the text the restore just rescued) plusmeta.confirmedSendId, naming WHICH send the row confirms, and both composers READ that identity to swap the caption from "resending may send it twice" to "Message delivered - sending it again will send it twice". Confirmation deliberately clears NOTHING: the composer's ownonChangeis only one mutation entry point, and a dropped session ref, a pasted block or a picked file change no text at all, so neither a touched flag nor a text comparison can see them and an automatic clear would delete work it cannot prove is a duplicate. The caption is released only when the user edits or clears the payload, and the marker is PERSISTED alongside the draft so a reload cannot bring the payload back unmarked. Content is deliberately NOT compared: two sends of the same short prompt ("continue") are indistinguishable by text, so a content match let an EARLIER confirmed row retire a LATER still-unconfirmed resend and delete the only copy of it.reconcileOptimisticEchostrips the one-shotsendId(fix(chat): pipelined sends cause duplicate bubbles and orphaned optimistic rows #3898 item 2), which is why the durable key is needed rather than reusing it; the receipt path stamps the same key so neither reader has to know which path confirmed. A composer the user has since edited is left alone.nextSeq). Needed by the supersede guard, which requires a monotonic token; a second counter beside the existing warm one would let two orderings interleave incomparably. It is minted in thunks only — no reducer reads the counter.ChatPane's abort arm now restores unconditionally, matching its own transport arm andChatPage: abort is the WEAKEST delivery state and the bubble is store-only, so a reload keeps neither it nor the composer cleared before the POST.Three review items are declared here rather than folded in, because each changes a contract wider than the identity defect this commit fixes. (a) Refining the retire to strip only a recovered suffix when the restore MERGED, instead of clearing, alters the merge/clear contract. (b) The four-flag delivery lifecycle (
optimistic,pendingServerRow,deliveryUnknown,deliveryConfirmed) is choreographed by hand in two surfaces; extracting the shared receipt/echo->caption handshake into one owner is a follow-up, not a bug fix. (c) The retention trade below — a send that genuinely never left leaves a bubble no refetch clears — is a declared product-behaviour trade a human should ratify; it is stated, not changed.Re-attachment keys on
meta.sendId, which the server preserves on the row it appends, so a page that already carries the row reconciles instead of gaining a duplicate.Re-attachment composes over the existing
tailNotInPage/rowIdentitiesidentity pair rather than hand-rolling a secondsendIdset, and each row is reinserted at its prior relative position rather than concatenated: athinkingrow can arrive after the optimistic append and the refetch re-seats it, so a tail append would render reasoning above the prompt that caused it. Anchoring on the nearest preceding identifiable neighbour also keeps pipelined sends in order.Retention is a POSITIVE
meta.pendingServerRowmarker, set only where the composer renders a bubble ahead of the server.clearPendingServerRowretires it on every outcome meaning the transcript must not get the row back from this client: a refused or errored send, and a QUEUED acceptance — keyed onsendId, because the slot-detail queue redacts its content for display, so no content join can pair a queued twin. It deliberately does NOT clearmeta.optimistic: that flag is delivery state, andChatPageSendConfirm.test.tsxpins that a refusal is not a receipt.Retention otherwise holds until a fetched page actually CONTAINS the row, so two concurrent pre-append refetches cannot delete an unacknowledged bubble between them. There is no other release: a row count cannot distinguish a rewind from a stale page for a send the server has not acknowledged, so no
totalcomparison is made — the earlier shrink wiring was unreachable once retention requiredoptimistic, and has been deleted rather than left as dead code.ChatPane's three failure arms get the same treatment through their sharedreportFailedSend, and the send-timeout arm is left preserved rather than collected, so a timed-out send cannot lose the user's text silently.Tests
chatSlice.optimisticBubblePreserved.test.tsdrives the real thunks against a real store with a mocked slot-detail response. Both drop-tests fail without the fix:messagesemptying to length 0 is precisely the blank-pane state, so the test pins the symptom and not just the merge.Also pinned:
setPagingCursorearly-returns while a switch claim is open, so a refresh resolving first installs no cursor, and then supersedes the switch that would have. The slot was left withslotCursorKey === null, whichloadOlder's own condition reads as "paging mid-switch" forever. The superseded branch installs NOTHING and returns: applying an older response's ownhasMore/nextBeforewould set the paging anchor from a page a newer response has already superseded, which skips rows. The residual is stated rather than hidden --switchSlot.pendinghas already de-keyedslotCursorKey, andloadOlder's condition (slotCursorKey === activeSlot) therefore refuses paging until the next successfulswitchSlot/refreshSlotinstalls a cursor. The testinstalls NO cursor from a superseded switch, even with none landedpins exactly that. The refresh itself is deliberately NOT suppressed: skipping it would let a switch that snapshottedrunning: trueoutlive thechat_donethat ended the turn, leaving the slot stuck streaming.setPagingCursorwrites unconditionally, so after two same-slot bounded switches resolved newest-first the stale response's oldernext_beforereplaced the newer one and the rows between them became unreachable. There is noslotCursorKeycomparison at the call site. The protection issetPagingCursor's own early return while a switch claim is open for the active slot, together withswitchSlot.pendingde-keying the cursor, so a stale response landing second writes nothing over the newer one. The testkeeps the newer cursor when the stale response lands secondpins it.markDeliveryUnknownmarks that row and the bubble renders at reduced opacity with a dashed outline, so the transcript stops vouching for a delivery the code calls unknown.pages.chatPage.delivery_unconfirmed, "Delivery unconfirmed — resending may send it twice.") is used by BOTH unknown-delivery arms, so the pane and the page describe one state identically instead of borrowing the refusal copy that asserts a non-delivery the code does not hold. The bubble carries a visible caption and an accessible label, not just reduced opacity and a dashed outline.confirmOptimisticSendandreconcileOptimisticEchoboth cleardeliveryUnknown, so a row whose receipt or WS echo already proved delivery stops rendering as unconfirmed instead of waiting for the next refetch to replace it.restoreComposerAfterFailedSendmerges rather than overwrites and callssaveDrafts(), so the recovery is the durable one. The duplicate-resend risk is carried by the caption instead ("resending may send it twice"), which is a warning the user can act on, where silent data loss is not. The unconfirmed bubble also drops itstitletooltip, which merely restated the caption rendered directly beneath it.ChatPageif (!_busy || forceNew),ChatPaneif (!busy && (text || files.length))), somarkDeliveryUnknownmarks nothing and the composer, already cleared before the POST, was the only copy. Both arms capture that gate's own result and restore when no bubble took the text; the transport arm restores regardless, since no bubble outlives a reload.appendMessagewrites to whichever slot is on screen, so switching away while the POST was in flight filed the failure in the wrong transcript. Both the refused and the transport arm now useappendSlotMessagewith the captured slot.AbortErrorarms dispatchmarkDeliveryUnknown, so a POST that stalled before any receipt no longer leaves a normal-looking phantom prompt that later refetches preserve indefinitely.pendingServerRowonto ANY user row carrying asendId, while release stayed hand-wired per outcome in two surfaces. A third send surface would have inherited retention for free and the release choreography not at all, turning its failed sends into tab-lifetime phantoms captioned "delivery unconfirmed". The reducers now set onlyoptimistic; retention comes from an explicitretainedSend(meta)whose contract names the releases it obliges. A surface that does not ask gets no retention, so the trap cannot be inherited by construction. The helper returns a new object because the caller's meta also goes over the wire and must not gain a client-only flag.AbortErrorarms marked delivery unknown but restored nothing. On the page the restore is now unconditional: the bubble is store-only, so when one WAS appended a reload still lost the text, the draft having been cleared and persisted as cleared before the POST. The pane arm keeps its no-bubble guard, becauserestoreIntoComposerwrites React state only and the pane persists no draft, so restoring there cannot make a payload outlive a reload.pages.chatPage.send_no_response, "The server did not respond to this send."). That is a transport fact rather than a delivery claim, so it stays true and cannot contradict the row once a receipt or echo makes the caption definite.retainedSendopt-in at the pane's append site fails four component tests, proving the opt-in sites are covered rather than only the fixtures; reverting each of the earlier three fixes fails its own test (four in total, incl.expected 'chat/appendMessage' to be 'chat/appendSlotMessage'); suppressing the composer restore fails five tests, one of which asserts the PERSISTED draft after a transport rejection and fails withexpected undefined to be 'survives a reload'-- the reload-loss the restore exists to prevent; reverting the unanchored walk fails both clock-skew tests, and removing either proven-delivery clear fails its own test; removing the cursor guard fails the stale-overwrite test (cursor 100 instead of 300) and over-reaching it to skip the install entirely fails two others; removing the unconfirmed-marker dispatch fails the marker test; suppressing the refresh on a superseded switch fails the stuck-running test; reverting the warm retention fails the warm test; disabling the page-identity release makes 7 tests fail (an implementation that retains unconditionally is rejected); reinstating any transport-error clear fails the offline-retain test; and reverting the timestamp advance, the retention hold or the ordering guard each fails exactly its own test, so none passes vacuouslyOrdering and the failure path are pinned too, and both were negative-controlled: reverting the reinsertion to a tail append fails
stays ABOVE a thinking row that arrived after it, and forcing the front-insertion fallback failslands after the history it followed, not at the very front.Ordering, the page-identity release and the two-concurrent-refetch case are each negative-controlled: reverting the timestamp advance, the identity filter, or the retention hold makes the corresponding tests fail.
Verification
src/store/: 6 files, 109 passedsrc/pages/: 14 files, 215 passedswitchSlot/refreshSlot: 1268 tests passed, 0 test failurestsc --noEmitclean,eslintclean on both changed files11 of those 52 files fail at collection in my sandbox. I ran the identical set on an unmodified baseline: also exactly 11 failures, 1263 tests passed (the 5-test delta is precisely this PR's new tests). Pre-existing and environmental to my local
node_modulessymlink, not introduced here — but worth a second look in CI.Screenshots
The unknown-delivery bubble: dimmed, dashed, and captioned, with a confirmed row above and a still-pending row below as controls that must look identical. Captured from the real component via
website/capture/user-message-delivery-unknown.html; the capture script fails unless exactly onerole="status"caption renders, so a checkout that lost the caption cannot produce these images.Pattern harvest
Rule candidate: semgrep
Pattern: a reducer rebuilding a locally-accumulated list wholesale from a fetched server page,
without routing the result through a preservation step for the rows that page cannot yet contain.
Evidence the class recurs rather than being a one-off: the same omission was present at THREE
independent rebuild sites in this one file -- both slot-refetch reducers and the warm reconcile.
Each site reads as locally correct, because the fetched page genuinely IS the authoritative
history; what it silently drops is the one row the server could not have known about yet.
Limit of the mechanical rule: the shape is syntactically detectable, so semgrep can flag the site
and require the author to say how in-flight local rows survive. It cannot judge whether a given
row is still unacknowledged -- that residue belongs in a review-prompt line, not in a gate.
Note on the shared localStorage budget: the two existing draft stores KEEP their 2 MiB cap -- an earlier revision derived it by dividing the shared budget, which lowered it to about 1.07 MiB and would have LRU-evicted unsent drafts a user had already typed, purely because the constant moved. The recovery store is budgeted separately at 512 KiB on top, so 2 + 2 + 0.5 = 4.5 MiB against the ~5 MB an origin gives. That cap bounds marker records only: a record still carrying a prompt is never evicted for budget, because this store is that prompt's only copy.