fix(chat): remove the unconfirmed-message notice and its timeout sweep - #4180
Conversation
Design Review (Fable 5) — 🟡 CONCERNSAdvisory design-level review of Design-Verdict: CONCERNS Sound subtraction with real parity fix, but it patches a server-side lie ( Watch
[DESIGN-REVIEWED] 6bc7831 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsBoth candidates' premises are verifiable, and both are falsified: CANDIDATE 2 (attachment-only queued send false-report): I confirmed the backend at CANDIDATE 1 (dead-socket lost signal): The removed sweep fired on every dashboard send (no No dangling references to the removed No grounded Step 2 finding survives falsification. No findings. [OPUS-REVIEWED] 6bc7831 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — 🟡 CONCERNSAdvisory premise-level review of I've read the review contract, the intent file, and the full patch, and verified the surrounding code: First-Principles-Verdict: CONCERNS A well-earned deletion, but the What this change shipsIntent: stop telling users a delivered message "may not have been delivered," and give the split-pane the real failure signal instead — a FIX (a removal executing a prior review's subtraction).
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 6bc7831 |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @CrysisDeu overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
UX Review (Fable 5) — ✅ PASSAdvisory UX-level review of UX-Verdict: PASS Removes a hedging, action-less notice and replaces silence on pane-send failure with an error row plus composer restore — the user keeps their work and gets the truth. Suggestions
[UX-REVIEWED] 6bc7831 |
76c96e4 to
56dbdff
Compare
56dbdff to
dd870d2
Compare
Review disposition — GPT 5.6, BLOCKING
|
dd870d2 to
001ac08
Compare
Review disposition — GPT 5.6 (advisory FINDING, head dd870d2)
|
Review disposition — UX Review, PASS with one suggestion (head dd870d2)
|
Review disposition — First Principles Review, advisory CONCERNS (head dd870d2)
|
001ac08 to
97d1778
Compare
Review disposition — Opus 4.8 (advisory FINDING, head 001ac08)
|
d4f4fdd to
e533831
Compare
Disposition — GPT 5.6 Review @
|
|
/ai-review override gpt e533831: Both findings are real but not specific to this PR — the flagged refusal test and the trailing-whitespace strip are shared verbatim with the reference send path (ChatPage.tsx:4043 and :3851/:4031), so fixing them only in the pane would re-diverge the two spellings the First Principles lane asked to collapse; both are tracked for a change that fixes BOTH paths together in #4217. Rationale per finding, and the evidence that each pattern is the reference's own, is in the disposition comment below. Neither finding is dismissed: #4217 carries both. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
"Message not confirmed - may not have been delivered" could only render on a send the server had ACCEPTED. A genuinely failed send takes a different path: ChatPage appends its own error bubble and restores the composer. So the notice asserted "may not have been delivered" in the one case where delivery had almost certainly succeeded. Its judgement was a client wall clock and nothing else -- no HTTP result, no ws.readyState, no queue state -- so it also fired on a slow first turn, on a tab whose socket dropped while the server finished the turn, and on a queued message. It offered no action either: chatSlice documented a "retry affordance" twice and what shipped was a static label, while resending is unsafe against an agent that runs real turns. It reached every user in English, including zh-CN. Removes items 4-8 of #3963 as First Principles Review recorded at the time ("Defer items 4-8 -- the reported defect is fully removed by items 1-3"), alongside two UX findings answered by the same blanket disposition. NOT a revert of that commit: items 1-3 are the real #3898 fix and stay -- the break-to-continue scan fix, reconcileOptimisticEcho, and the one-shot sendId strip. `optimistic: true` also stays; it is the reconcile marker. Dropping optimisticTs additionally closes a defect flagged in the same review: reconcile stripped sendId and optimistic but left optimisticTs in the persisted transcript forever, reintroducing the exact harm item 3 of that PR existed to remove. Two tests pinning the removed behaviour are replaced by one pinning its absence, so the timestamp cannot return unnoticed.
e533831 to
6bc7831
Compare
|
/ai-review override gpt 6bc7831: Re-issued for the new head; the two findings are unchanged and unchanged in disposition — both patterns are shared verbatim with the reference send path (ChatPage.tsx:4043-4049, :3851, :4031), so fixing them only in the pane would re-diverge the two spellings, and both are tracked for a both-paths change in #4217. Disposition — GPT 5.6 Review, carried to
|
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Disposition — Design Review @
|
Disposition — First Principles Review @
|
Disposition — UX Review @
|
kirodotdev#4180) "Message not confirmed - may not have been delivered" could only render on a send the server had ACCEPTED. A genuinely failed send takes a different path: ChatPage appends its own error bubble and restores the composer. So the notice asserted "may not have been delivered" in the one case where delivery had almost certainly succeeded. Its judgement was a client wall clock and nothing else -- no HTTP result, no ws.readyState, no queue state -- so it also fired on a slow first turn, on a tab whose socket dropped while the server finished the turn, and on a queued message. It offered no action either: chatSlice documented a "retry affordance" twice and what shipped was a static label, while resending is unsafe against an agent that runs real turns. It reached every user in English, including zh-CN. Removes items 4-8 of kirodotdev#3963 as First Principles Review recorded at the time ("Defer items 4-8 -- the reported defect is fully removed by items 1-3"), alongside two UX findings answered by the same blanket disposition. NOT a revert of that commit: items 1-3 are the real kirodotdev#3898 fix and stay -- the break-to-continue scan fix, reconcileOptimisticEcho, and the one-shot sendId strip. `optimistic: true` also stays; it is the reconcile marker. Dropping optimisticTs additionally closes a defect flagged in the same review: reconcile stripped sendId and optimistic but left optimisticTs in the persisted transcript forever, reintroducing the exact harm item 3 of that PR existed to remove. Two tests pinning the removed behaviour are replaced by one pinning its absence, so the timestamp cannot return unnoticed. Co-authored-by: t <t@t>
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. |
Problem / Motivation
Message not confirmed — may not have been deliveredappears under a sent chatbubble ~30–40s after send. It is wrong in the one thing it asserts, and it
cannot be acted on.
A genuinely failed send is reported separately — on the single-chat path.
ChatPage.tsxappends an error bubble when the server accepts neitheroknorqueued(Couldn't send — check your connection and try again.) and callsrestoreComposerAfterFailedSend(), which hands the text back to the composer.So on that path the stale notice can only render on a send the server
accepted — the case where delivery almost certainly succeeded. It says "may
not have been delivered" precisely when that is least likely to be true.
The split-pane path was different, and GPT 5.6 Review was right to block on
it.
ChatPane.doSendcleared the composer on the way out, appended theoptimistic bubble, and then swallowed the outcome entirely:
api.sendChat(...).catch(() => undefined), with the.thenbranch returningearly unless a question card was pending. No error row, no composer restore, no
signal of any kind — an undelivered message stayed on screen looking sent. On
that path the 30s notice genuinely was the only thing that ever spoke.
That does not make the notice the right instrument, and it is not restored here.
A 30s wall-clock guess is a bad answer to a failure the code already knows
about at the moment it happens. This PR gives that path the real signal
instead: the same immediate error row and composer recovery
ChatPagehasalways had. Assert what happened, hand back the payload — which is what the
notice failed to do on either path.
#4131 has since made this worse, and its fix proves the point. While this PR
was in review,
mainlandedconfirmOptimisticSendwith the finding that thechat_messageechoreconcileOptimisticEchowaits for is never broadcast fora dashboard send —
DashboardState.appendsuppresses it by design, preciselybecause the composer already rendered the bubble. So the pending flag survived
on every composer message and, in that PR's own words, "the 30s sweep flags
every message the user sends". The notice was not an occasional false positive;
it was a false positive by construction on the dashboard's primary send path.
This PR is rebased onto that fix and keeps all of it:
confirmOptimisticSendis the honest half — it retires the pending state at the moment the server's own
response accepts the send, which is the only confirmation a dashboard send ever
gets. What this PR removes is the other half, the wall-clock guess that read the
state. The reducer's
delete meta.optimisticTs/delete meta.staleare keptdeleted: I kept them at first with a legacy-transcript rationale, and First
Principles checked the reachability I had not — the confirm only ever matches a
just-sent
sendId, which never carries those fields, and no reader of eitherremains anywhere. The scrub could not reach the rows it claimed to clean.
The judgement behind the notice was a client wall clock and nothing else:
Date.now() - optimisticTs > 30_000. It consults no HTTP result, nows.readyState, and no queue state. So it also fires on a slow first turn(kiro-cli cold start plus first MCP startup regularly exceeds 30s), on a tab
whose socket dropped while the server ran the turn to completion, and on a
message sitting in the queue behind a running turn. All three are delivered.
There is no recovery action either.
chatSlice.tsdocuments one twice — "theUI renders a retry affordance for stale messages", "so the UI can show a
'retry' affordance" — and what ships is a static label with an
AlertCircle.The one thing a reader could do, resend, is unsafe against an agent that runs
real turns and edits files: nothing tells them whether the first one landed.
And the notice reached every user in English. The key was pasted verbatim into
all 13 catalogs including
zh-CN.json, so a Chinese dashboard renders anEnglish sentence with an em dash.
Why it matters
Every one of those points was raised on #3963 before it merged, by three
separate advisory lanes:
for 11 locales, promises no recovery action, and won't fire when the
connection actually drops."
in exactly the failure it was built to surface."
items 4–8 … the reported defect is fully removed by items 1–3."
All four findings were answered with one disposition comment carrying one
rationale —
additive scope beyond the core bug fix— and the PR merged.This PR executes First Principles' subtraction as written. It is the same
verdict, 30 hours later, with the intervening evidence that deferring it made
the surface worse rather than idle: #3973 moved the sweep from the server
heartbeat to a client-side 10s interval, which raised the trigger rate (the
indicator now appears reliably while offline, which is the delivered case) and
addressed none of the three findings.
What changed (motivation → approach → change)
This is deliberately not
git revert 1fa394ecd. That commit carries thereal fix for #3898 as well as the rider, and reverting it wholesale would
reinstate the duplicate-bubble regression. Items 1–3 stay untouched:
break→continuescan fix that lets pipelined sends reconcile,reconcileOptimisticEcho()helper that de-duplicated two inline scans,sendIdstrip after a successful match.Items 4–8 are removed:
stale && optimisticindicator + itsAlertCirclewebsite/src/pages/chat/UserMessage.tsxsweepStaleOptimisticreducer and its exportwebsite/src/store/chatSlice.tsOPTIMISTIC_TIMEOUT_MS(exported, zero consumers outside the module)website/src/store/chatSlice.tsoptimisticTswrite, in both append pathswebsite/src/store/chatSlice.tssetIntervaldispatch added by #3973website/src/hooks/useWebSocket.tsmessage_unconfirmedkeyoptimistic: truestays. It is the reconcile scan's marker and isload-bearing for the #3898 fix; only the wall-clock timestamp beside it goes.
And one thing is added, because removing the notice took away the split-pane
path's only signal (GPT 5.6 Review, round 1):
ChatPane.doSendnow reports afailed send the way
ChatPagealways has — an error row addressed to the slotthat owns the message (not the active one; the user can switch panes while the
POST is in flight) plus the composer text and files handed back. Both outcomes
are covered, the rejected fetch and a body the server accepted as neither
oknor
queued, and one shape that claims acceptance without it:chat_handlersqueues
if message:but returns{ok, queued}unconditionally, so anattachment-only send that raced the slot into the busy state was neither queued
nor broadcast — the file was discarded while the composer cleared. That is now
reported too (GPT 5.6, blocking), which is why the guard reads
body.queued && !llm.trim()rather than trustingqueuedalone. The send isalso bounded by the same 10s
AbortControllerChatPage.senduses, and carries the same meaning it does there: reaching thebound means the request WAS received and only the reply is late, so the abort is
ignored rather than reported. Treating it as a failure — which an earlier
revision of this PR did — hands the payload back and invites a retry that
duplicates a turn already running, side effects included (GPT 5.6, blocking).
The recovery APPENDS rather than replaces — the failed text goes
below whatever the composer now holds, separated by a blank line, and identical
text is not duplicated; attachments merge as a set union. Neither payload may
win: preferring the newer one silently discards the message the error row is
telling the user to try again, and preferring the older one loses work they just
did. This mirrors
ChatPage'srestoreComposerAfterFailedSendexactly, and thefirst revision of this PR got it wrong in the newer-wins direction — caught by
GPT 5.6 and UX Review independently.
The recovery lives in ONE place:
restoreIntoComposer, shared by the faileddoSendand by the question-cardonFallbackSendthat previously carried itsown single-newline, no-dedupe spelling. First Principles flagged the divergence
the moment this PR created it; collapsing the two is a net reduction rather than
a second copy. The error row also surfaces the server's own reason when there
is one (
body.error), falling back topages.chatPage.send_failedonly on thetransport-reject path where no body exists — a 409
slot agent mismatchisactionable, "check your connection" is not (Opus 4.8). No new string ships.
Two other
api.sendChatcallers still swallow a failed send —App.tsx:1696(feature-request flow) and
useSceneInteraction.tsx:290(fetchresolves on4xx/5xx, so a refused send still reaches
setSendState('sent')). FirstPrinciples Review found them and confirmed the scope: neither ever carried
sendIdmeta, so neither was covered by the notice this PR removes and nothingregresses. Filed as #4198 rather than fixed here — each needs its own decision
about where the error surfaces (the feature-request modal has no transcript to
append to), which is design work that does not belong in a subtractive PR.
Removing
optimisticTsalso closes a defect First Principles flagged in thesame review:
reconcileOptimisticEchostripssendIdandoptimisticbutleft
optimisticTs(and a late-echostale: true) in the persisted transcriptforever — the exact harm item 3 of that PR existed to remove, reintroduced one
line below it.
Behaviour after this change: an unconfirmed bubble looks like an unconfirmed
bubble — no reply, no streaming, no tool rows under it. That absence was
already visible, earlier and more legibly than a 12px line of warning text, and
it does not assert anything false.
Tests
Two tests that pinned the removed behaviour are replaced by one that pins its
absence, so a future change cannot quietly re-add the timestamp:
website/src/test/chatSlice.test.ts—sweepStaleOptimistic marks timed-out bubbles as staleand… does not mark fresh optimistic bubbles as stalearereplaced by
records no wall-clock timestamp on an optimistic bubble, whichasserts
optimistic === trueand bothoptimisticTsandstaleundefined.website/src/test/UseWebSocketCoverage.test.tsx— theclient-side optimistic timeout sweep (#3973)describe block (the intervaldispatch test and its unmount-cleanup sibling) is deleted along with its
imports.
website/src/test/ChatPane.dirSend.test.tsx— five new tests for the signalthat replaced the notice on the split-pane path: a rejected send and a refused
body each produce exactly one error row and hand the text back; an accepted
send produces none; a message typed while the failing send was in flight gets
the failed payload APPENDED below it (
newer work\n\nthe failing one), notreplaced and not dropped; and retyping the same text mid-flight does not come
back doubled; and a refused body surfaces the server's own
errorstring whilea transport reject falls back to the catalog entry. Only the fallback case is
asserted loosely (non-empty
role: 'error'), so no assertion pins catalogwording.
Net: 2 tests deleted, 7 added, 1 describe block removed.
Run locally:
npx tsc -bclean;npm run lint0 errors (627 pre-existingwarnings, unchanged); all i18n gates OK (
i18n:check— 540 untranslated across115 files, at/below the 1015 baseline; DNT 22 terms intact; manifest-sync 21/179
exact; unit-literals
[added-lines] 0,[vs-base] 0); all 13 catalogs parse;51 test files / 1065 tests pass across every file matching
ChatPane|chatSlice|UserMessage|useWebSocket|QueueStack, and a widerchatSlice|UserMessage|useWebSocket|ChatPage|i18n|localesweep passed124 files / 1870 tests on the previous head.
The full 877-file vitest run was not completed locally — CI runs it. The
residual risk is bounded:
tsc -btype-checks the whole project,eslintcovers all of
src, and a repo-wide grep forsweepStaleOptimistic,OPTIMISTIC_TIMEOUT_MS,optimisticTsandmessage_unconfirmedreturns nohits outside this diff's own assertions.
Manual verification
N/A for the removal itself — a deleted render branch has no runtime path, and
the vitest assertion above proves the state that gated it is never written.
What is not covered by unit tests: the reconcile path this PR deliberately
leaves alone.
chatSlice.test.tsretains the #3898 coverage (pipelined-sendreconciliation, the steer boundary,
sendIdstrip), and those 225 tests passunchanged, which is the evidence that items 1–3 survive intact.
Screenshots / video
Captured from an isolated harness (
website/capture/user-message-unconfirmed.tsx)that mounts the real
UserMessageagainst the real stylesheet and theme tokensand hands it the exact
metathe reducer used to write. The state beingphotographed was reachable in the running app only ~30–40s after a send whose
echo never arrived, so shooting it live means stalling the WebSocket for half a
minute; the harness reaches the same render with no gateway and no timing.
The same harness file produces both shots —
beforefrom the base commit,afterfrom this branch — so nothing about the scene differs between them.Rows 1 and 3 are the control: a confirmed bubble and a still-pending one that
has not timed out. They must look identical in both images, so any difference a
reviewer sees is attributable to row 2 alone.
Light theme
The capture script (
website/scripts/capture-user-message-unconfirmed.mjs) isself-checking, and that is the point of the pair: it counts the indicator's
role="status"node and exits non-zero unless the count matches theexpectargument it was given. So a
beforeshot cannot be taken from a checkout thatno longer draws the notice, and an
aftershot cannot be taken from one thatstill does — a mislabelled pair fails instead of emitting a misleading image.
Verified in both directions locally:
expect=presentagainst this branch failswith
expected 1 unconfirmed notice(s), saw 0.Related Issues
Executes the subtraction recorded on #3963 by First Principles Review
(
Defer items 4–8) and the UX Review findings on the same PR. Leaves #3898 (thedefect #3963 legitimately fixed) fixed. Supersedes #3973, which changed this
code path's trigger without addressing the findings.
Review-process counterpart: #4168, which fixes the verdict→authority mapping
that let these three findings be deferred with one blanket rationale.
Checklist
Known and deferred
The GPT lane found two real defects in the pane send path that this PR does not fix, because both
patterns are shared verbatim with the reference send path in
ChatPage(ChatPage.tsx:4043-4049,:3851,:4031): an unreadable response body is reported as a refused send, and composer recoverystrips trailing whitespace that Markdown treats as a hard line break. Fixing either only here would
make the two paths disagree on the same server response. Both are tracked in #4217 together with the
other deferred send-failure items, and a human override is recorded on this PR for that lane.