Skip to content

fix(chat): keep the optimistic user bubble across a slot refetch - #6825

Open
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/preserve-optimistic-user-bubble-on-slot-refetch
Open

fix(chat): keep the optimistic user bubble across a slot refetch#6825
rnoack1 wants to merge 1 commit into
kirodotdev:mainfrom
rnoack1:fix/preserve-optimistic-user-bubble-on-slot-refetch

Conversation

@rnoack1

@rnoack1 rnoack1 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

A slot-detail refetch rebuilds messages from the fetched page and drops the user's just-sent message.

The composer appends the user's row client-side at send time (appendMessage, flagged meta.optimistic because it carries a sendId), 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.fulfilled re-attaches a trailing local reply only when it is an assistant/streaming row, otherwise falls through to next = preserved
  • refreshSlot.fulfilled re-injects only permission cards over the fetched page
  • both additionally carry pastes, thinking blocks and the queue

Neither preserves a trailing user row. So a refetch resolving in the window between the optimistic append and the server's own append silently discards the bubble.

Why it matters

isWelcomeState requires !slotLoading, so once messages is 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 through clearPendingServerRow. 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.onLine least 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 = false moved above the supersede guard in switchSlot.fulfilled — a real timing change. It is placed deliberately between the guards: BELOW activeSlot !== 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.fulfilled never 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.
  • Failure rows are addressed to the SENDING slot, at three ChatPage sites (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.
  • A receipt or echo makes the caption DEFINITE; it does not clear the composer. The failure arms stage the payload back so a reload cannot lose it, but confirmation retires the "resending may send it twice" caption, which previously left the identical payload staged with the warning gone — one reflexive Enter duplicated an answered turn. Both clear sites now set a positive meta.deliveryConfirmed marker (absence cannot carry this: a DROPPED row is absent too, and clearing on absence would destroy the text the restore just rescued) plus meta.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 own onChange is 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. reconcileOptimisticEcho strips the one-shot sendId (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.
  • Warm and slot-detail ordering share one dispatch clock (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 and ChatPage: 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 / rowIdentities identity pair rather than hand-rolling a second sendId set, and each row is reinserted at its prior relative position rather than concatenated: a thinking row 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.pendingServerRow marker, set only where the composer renders a bubble ahead of the server. clearPendingServerRow retires 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 on sendId, because the slot-detail queue redacts its content for display, so no content join can pair a queued twin. It deliberately does NOT clear meta.optimistic: that flag is delivery state, and ChatPageSendConfirm.test.tsx pins 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 total comparison is made — the earlier shrink wiring was unreachable once retention required optimistic, and has been deleted rather than left as dead code. ChatPane's three failure arms get the same treatment through their shared reportFailedSend, 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.ts drives the real thunks against a real store with a mocked slot-detail response. Both drop-tests fail without the fix:

AssertionError: expected [] to have a length of 1 but got +0   (switchSlot)
AssertionError: expected [] to have a length of 1 but got +0   (refreshSlot)

messages emptying to length 0 is precisely the blank-pane state, so the test pins the symptom and not just the merge.

Also pinned:

  • duplicate-safety per path — a page already containing the row must not gain a second copy, so a fix that blindly re-appends cannot pass
  • a superseded switch discards its paging cursor along with the rebuildsetPagingCursor early-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 with slotCursorKey === null, which loadOlder's own condition reads as "paging mid-switch" forever. The superseded branch installs NOTHING and returns: applying an older response's own hasMore/nextBefore would set the paging anchor from a page a newer response has already superseded, which skips rows. The residual is stated rather than hidden -- switchSlot.pending has already de-keyed slotCursorKey, and loadOlder's condition (slotCursorKey === activeSlot) therefore refuses paging until the next successful switchSlot/refreshSlot installs a cursor. The test installs NO cursor from a superseded switch, even with none landed pins exactly that. The refresh itself is deliberately NOT suppressed: skipping it would let a switch that snapshotted running: true outlive the chat_done that ended the turn, leaving the slot stuck streaming.
  • the warm reconcile retains a pending send too — it is the third rebuild site: with no identity overlap its base is the warm page alone, which dropped a pane send before the user switched in.
  • a superseded switch cannot overwrite a newer paging cursorsetPagingCursor writes unconditionally, so after two same-slot bounded switches resolved newest-first the stale response's older next_before replaced the newer one and the rows between them became unreachable. There is no slotCursorKey comparison at the call site. The protection is setPagingCursor's own early return while a switch claim is open for the active slot, together with switchSlot.pending de-keying the cursor, so a stale response landing second writes nothing over the newer one. The test keeps the newer cursor when the stale response lands second pins it.
  • an unconfirmed bubble no longer reads as delivered — a transport error keeps retention (no signal proves non-delivery), so the row survives every refetch while the adjacent error row says the send failed. markDeliveryUnknown marks 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.
  • an unanchored send is positioned structurally, not by clock — with an empty cache the send is the only prior row, so no preceding identity can anchor it, and a client clock running BEHIND the server made every fetched row look later, seating the prompt ABOVE the fetched history. An unanchored send now walks past every SERVER-IDENTIFIED row instead of consulting a timestamp, so it lands after the history while later local rows stay after it.
  • the unconfirmed state is stated in words, not only in styling — one shared catalog string (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.
  • proven delivery retires the markingconfirmOptimisticSend and reconcileOptimisticEcho both clear deliveryUnknown, 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.
  • a failed send is recoverable after a RELOAD, not just on screen — the composer restore runs on every page arm that ends without a receipt: an explicit refusal, a dropped queued send, a transport error, and the 10s abort. The abort is the weakest state of the four -- delivery is unknown -- so it is the arm that most needs the payload handed back, and it now restores unconditionally like its siblings. It still reports nothing in the transcript, because the request was probably received and only the reply is late; the duplicate-resend risk is carried by the caption rather than by withholding the text. An earlier iteration here restored only where no bubble had taken the text, reasoning that a retained bubble already held it and that re-staging invited a duplicate resend. That was wrong about durability: the bubble lives only in the store, and the draft was cleared and PERSISTED as cleared before the POST, so a reload lost the text and its attachments with no copy anywhere. restoreComposerAfterFailedSend merges rather than overwrites and calls saveDrafts(), 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 its title tooltip, which merely restated the caption rendered directly beneath it.
  • a BUSY send keeps its text — a busy send appends no optimistic bubble at all (ChatPage if (!_busy || forceNew), ChatPane if (!busy && (text || files.length))), so markDeliveryUnknown marks 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.
  • a failure row is addressed to the SENDING slotappendMessage writes 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 use appendSlotMessage with the captured slot.
  • a timed-out send stops reading as delivered — both AbortError arms dispatch markDeliveryUnknown, so a POST that stalled before any receipt no longer leaves a normal-looking phantom prompt that later refetches preserve indefinitely.
  • retention is now OPT-IN, closing a set/release asymmetry — the reducers used to stamp pendingServerRow onto ANY user row carrying a sendId, 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 only optimistic; retention comes from an explicit retainedSend(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.
  • an aborted send keeps its text — both AbortError arms 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, because restoreIntoComposer writes React state only and the pane persists no draft, so restoring there cannot make a payload outlive a reload.
  • the warning is no longer printed twice — the bubble caption owns the delivery state, so the error row carries a distinct string (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.
  • negative controls — removing the retainedSend opt-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 with expected 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 vacuously

Ordering 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 fails lands 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

  • new file: 5 passed
  • src/store/: 6 files, 109 passed
  • src/pages/: 14 files, 215 passed
  • all 52 files referencing switchSlot/refreshSlot: 1268 tests passed, 0 test failures
  • tsc --noEmit clean, eslint clean on both changed files

11 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_modules symlink, 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 one role="status" caption renders, so a checkout that lost the caption cannot produce these images.

Unknown-delivery bubble, dark theme

Unknown-delivery bubble, light theme

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.

@rnoack1
rnoack1 requested a review from a team August 29, 2026 18:28
@rnoack1
rnoack1 requested a review from a team as a code owner August 29, 2026 18:28
@rnoack1
rnoack1 requested a review from krishdhasmana August 29, 2026 18:28
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@iamwhatever

Copy link
Copy Markdown
Collaborator

👋 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:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed

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
@dwu96

dwu96 commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

👋 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:

  • ## Problem / Motivation
  • ## Why it matters
  • ## What changed

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.

@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5.1, fork) — 🟡 CONCERNS

UX-level review of e4923de99bfee9f673d895b415a5333179760d0f via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Confirmed the fork lane: the two screenshots this PR adds (temp-screenshots/optimistic-bubble-delivery/*.png) exist only as binary markers (patch lines 292–299) and are not on disk; no blind read ran. Every user-visible control this PR adds is therefore an evidence gap. The copy and handlers themselves are well-formed (labels trace to their actions; the "may"/"if it never reached the server" hedging reflects genuinely-unknowable delivery state the code does not hold, per the PR's stated design — so no false-hedge exit fires).

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

  • Dense composer caption: in the non-delivered arm the role="status" span concatenates two clauses — "Delivery unconfirmed — your text is back in the composer; resending may send it twice." + "Dismissing the warning also removes this message if it never reached the server." (ChatPage.tsx L2421-2423, ChatPane.tsx L1286). Two sentences of 12px warn text in composer chrome; a stressed user re-reads. Frequency low × persistence per-failure. Smallest fix: keep the resend clause in the caption, move the dismiss consequence into the button's title/aria-describedby beside "Dismiss and remove message".

Evidence gaps

  • Unconfirmed/spent bubble state — dimmed opacity-70 + dashed outline + caption delivery_unconfirmed_short ("Delivery unconfirmed") / delivery_unconfirmed_spent ("A later message was delivered; this one was never confirmed"), UserMessage.tsx L2484-2491 — in no materialized screenshot; needs the two added PNGs pushed to this repo or a blind read.
  • Composer action buttons delivery_discard "Discard message", delivery_clear "Clear composer", delivery_dismiss "Dismiss and remove message", app.dismiss "Dismiss", and caption delivery_delivered/delivery_unconfirmed_resend (ChatPage.tsx L2416-2445) — no screenshot shows the composer state; comprehension and whether a user dares click is unknown.
  • Notice/error rows send_no_response "No confirmation came back for this message." and the delivery_unconfirmed_resend warning notice (ChatPage.tsx L1080, L2108) — unshown.

[UX-REVIEWED] e4923de

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed e4923de99bfee9f673d895b415a5333179760d0f via the fork AI-review pipeline; updated in place on each push.

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
const pageKey = (slot: string): string => \page:${slot}``
Two tabs fail sends in one slot -> one tab dismisses its warning -> shared marker is cleared -> the other reloads and can resend an already-delivered turn without warning.
Anchor: residual/crash-data-loss-corruption
Fix: Key page markers by tab owner and send ID; load and clear only the current tab’s marker.
[BLOCK-MERGE] e4923de
[GPT-REVIEWED] e4923de

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.

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5.1, fork) — 🟡 CONCERNS

Premise-level review of e4923de99bfee9f673d895b415a5333179760d0f via the fork AI-review pipeline — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

I have enough to complete the review. The website/capture/, website/scripts/capture-*.mjs, and temp-screenshots/ files match long-established repo conventions (227 capture components, 426 capture scripts, 1213 committed screenshots), so they are not improper riders. The core change is a genuine fix bundled with several separable defect fixes and a new delivery-doubt UI, all of which the author declares.

First-Principles-Verdict: CONCERNS

A fix(chat) for one dropped-bubble bug that also ships ~5 separable defect fixes plus a new delivery-doubt marker/caption system — each has a named harm, but they are independently landable.

What this change ships

Intent: keep the user's just-sent bubble on screen when a slot refetch resolves before the server persists it. Fundamentally a FIX.

  1. Just-sent user bubble survives a mid-send refetch (pendingServerRow retention + preserveOptimisticSends) — justified, this is the fix.
  2. Unknown/unconfirmed bubble renders muted+dashed with a caption — rides along (new visibility).
  3. Composer caption "resending may send it twice" → "delivered", with Discard/Dismiss — rides along.
  4. A stranded send survives a full page reload (localStorage chatPaneRecovery) — rides along.
  5. Failure/error rows land in the SENDING slot, not the active one — rides along (declared 2nd defect).
  6. Cancelling a queued card restores attachments after a missed queue_push (sendId/edited plumbing + preSendStash) — rides along (3rd defect).
  7. Superseded same-slot refetch discarded (slotDetailSeq) — rides along.
  8. slotLoading=false moved above supersede guard — justified (required by 7).
  9. Cross-tab ownership so a parked send only returns to its origin tab (tabId) — rides along.
  10. Spent-doubt state (deliveryUnresolved/demotePriorDoubt, past-tense caption) — rides along. (>10 items total; also applyQueueEdit, sendIds merge-matching.)

Watch

  • The staged-warn caption + Discard/Dismiss + onComposerInput containment is hand-choreographed twice (ChatPane.tsx:874-966, ChatPage.tsx:2166-2203) — a second spelling that will diverge. The author declares the shared-owner extraction as a follow-up; land it before the two copies drift.
  • Item 5 (failure-row-slot) is a distinct defect with its own transcript-correctness harm; it shares no mechanism with the retention fix and could land alone.

Subtractions

  • Drop the deliveryUnresolved/spent third state and delivery_unconfirmed_spent caption. Its only harm is a permanent nag on a never-confirmed row; deleting deliveryUnknown in demotePriorDoubt (chatSlice.ts:4346) removes that harm without a new marker, precedence rule, and render branch (1 consumer: UserMessage.tsx:2491). Keeping the row "visible in past tense" is a product choice, not part of bubble retention.
  • Defer the cross-tab tabId/reachedByNavigation/timeOrigin ownership arbitration in chatPaneRecovery.ts:9328-9387 to its own change — the stated retention bug is single-tab and does not require multi-tab park ownership to be fixed.

[FIRST-PRINCIPLES-REVIEWED] e4923de

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5.1, fork) — 🟡 CONCERNS

Design-level review of e4923de99bfee9f673d895b415a5333179760d0f via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Scope / reviewability. Beyond the stated bubble-preservation fix, the change also lands: failure rows re-addressed to the sending slot (the author's own words: "a SECOND defect fixed here, beyond the stated bug"), the deliveryConfirmed/confirmedSendId caption contract, the composer-restore-on-failure behavior, the shared nextSeq dispatch clock, and two cursor-guard fixes. AGENTS.md pins "one logical change per commit, at most two commits per PR." The retention/clock/supersede pieces genuinely interlock, but the sending-slot failure-row fix is cleanly separable and unrelated to the optimistic-bubble identity defect — it should be its own PR. As shipped, these changes cannot be reverted independently if one regresses.
  • Declared retention trade needs human sign-off. By design a send that truly never left the browser leaves a bubble no refetch ever clears (item (c), and the retainedSend opt-in). This is a deliberate, correct-for-the-threat choice (no browser signal proves non-delivery), but it is a lasting product behavior — a tab-lifetime phantom captioned "delivery unconfirmed" — that the author explicitly asks a human to ratify. Confirm before merge.

[DESIGN-REVIEWED] e4923de

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed e4923de99bfee9f673d895b415a5333179760d0f via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] e4923de

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/preserve-optimistic-user-bubble-on-slot-refetch branch from 787e318 to 5bd4536 Compare August 29, 2026 19:35
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/preserve-optimistic-user-bubble-on-slot-refetch branch from 5bd4536 to b75621c Compare August 29, 2026 20:11
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@rnoack1
rnoack1 force-pushed the fix/preserve-optimistic-user-bubble-on-slot-refetch branch from b75621c to 7970b0c Compare August 29, 2026 20:33
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 30, 2026
auto-merge was automatically disabled August 30, 2026 00:39

Head branch was pushed to by a user without write access

@rnoack1
rnoack1 force-pushed the fix/preserve-optimistic-user-bubble-on-slot-refetch branch from 1faf116 to ce55670 Compare August 30, 2026 00:39
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 30, 2026
@rnoack1
rnoack1 force-pushed the fix/preserve-optimistic-user-bubble-on-slot-refetch branch from ce55670 to b8dd77f Compare August 30, 2026 01:50
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 30, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Aug 30, 2026
@rnoack1
rnoack1 force-pushed the fix/preserve-optimistic-user-bubble-on-slot-refetch branch from b8dd77f to 9fdd5e0 Compare August 30, 2026 02:26
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Aug 30, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #5909 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #5909: MERGE_DISCUSSION. A sequencing decision is needed, not a closure. 5909's own description names PR #6825 as the follow-up that must rebase its markSendFailed/delivery-unknown behavior onto this transport caller; the conflict is real and the two encode opposite abort-path policies. Files: website/src/components/ChatPane.tsx, website/src/test/ChatPane.dirSend.test.tsx.
  • This PR is OVERLAPPING with PR #4180. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6825: MERGE_DISCUSSION. Reinstating a surface a merged PR deliberately removed needs an explicit human ratification, specifically for the 10s-abort trigger; the transport-failure trigger is defensible on its own terms. Files: website/src/pages/chat/UserMessage.tsx.
  • This PR is OVERLAPPING with PR #7255. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6825: MERGE_DISCUSSION. A behaviour change and a wholesale relocation of the same ~200 lines. Sequencing this (fix first, refactor rebases) is far cheaper than the reverse, and should be agreed rather than discovered at merge time. Files: website/src/pages/ChatPage.tsx.
  • This PR is OVERLAPPING with PR #7911. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6825: MERGE_DISCUSSION. Independent features on one line; no behavioural contradiction and no goal overlap. Files: website/src/pages/chat/UserMessage.tsx.
  • This PR is OVERLAPPING with PR #7916. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6825: MERGE_DISCUSSION. Two open PRs rewrite the same switchSlot thunk for unrelated reasons and already conflict. Land order needs to be decided explicitly, and whoever rebases second must carry detailSeq onto BOTH of 7916's return paths. Files: website/src/store/chatSlice.ts, website/src/store/chatSlice.olderHistoryCursor.test.ts.
  • This PR is OVERLAPPING with PR #7997. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #6825: MERGE_DISCUSSION. Complementary halves of the same honesty principle on different flags; no code collision. Files: website/src/pages/chat/UserMessage.tsx.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants