Skip to content

fix(chat): remove the unconfirmed-message notice and its timeout sweep - #4180

Merged
bolichen97 merged 1 commit into
mainfrom
fix/remove-unconfirmed-indicator
Aug 18, 2026
Merged

fix(chat): remove the unconfirmed-message notice and its timeout sweep#4180
bolichen97 merged 1 commit into
mainfrom
fix/remove-unconfirmed-indicator

Conversation

@CrysisDeu

@CrysisDeu CrysisDeu commented Aug 17, 2026

Copy link
Copy Markdown
Collaborator

Problem / Motivation

Message not confirmed — may not have been delivered appears under a sent chat
bubble ~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.tsx appends an error bubble when the server accepts neither ok nor
queued (Couldn't send — check your connection and try again.) and calls
restoreComposerAfterFailedSend(), 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.doSend cleared the composer on the way out, appended the
optimistic bubble, and then swallowed the outcome entirely:
api.sendChat(...).catch(() => undefined), with the .then branch returning
early 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 ChatPage has
always 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, main landed confirmOptimisticSend with the finding that the
chat_message echo reconcileOptimisticEcho waits for is never broadcast for
a dashboard send
DashboardState.append suppresses it by design, precisely
because 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: confirmOptimisticSend
is 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.stale are kept
deleted: 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 either
remains 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, no
ws.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.ts documents one twice — "the
UI 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 an
English 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:

  • UX Review — CONCERNS: "A delivery-failure warning that ships in English
    for 11 locales, promises no recovery action, and won't fire when the
    connection actually drops."
  • Design Review — CONCERNS: "rides the server heartbeat, so it goes silent
    in exactly the failure it was built to surface."
  • First Principles — CONCERNS, with the subtraction spelled out: "Defer
    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 the
real fix for #3898 as well as the rider, and reverting it wholesale would
reinstate the duplicate-bubble regression. Items 1–3 stay untouched:

  • the breakcontinue scan fix that lets pipelined sends reconcile,
  • the reconcileOptimisticEcho() helper that de-duplicated two inline scans,
  • the one-shot sendId strip after a successful match.

Items 4–8 are removed:

Removed Where
the stale && optimistic indicator + its AlertCircle website/src/pages/chat/UserMessage.tsx
sweepStaleOptimistic reducer and its export website/src/store/chatSlice.ts
OPTIMISTIC_TIMEOUT_MS (exported, zero consumers outside the module) website/src/store/chatSlice.ts
the per-send optimisticTs write, in both append paths website/src/store/chatSlice.ts
the 10s setInterval dispatch added by #3973 website/src/hooks/useWebSocket.ts
the message_unconfirmed key 13 locale catalogs

optimistic: true stays. It is the reconcile scan's marker and is
load-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.doSend now reports a
failed send the way ChatPage always has — an error row addressed to the slot
that 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 ok
nor queued, and one shape that claims acceptance without it: chat_handlers
queues if message: but returns {ok, queued} unconditionally, so an
attachment-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 trusting queued alone. The send is
also bounded by the same 10s AbortController
ChatPage.send uses, and carries the same meaning it does there: reaching the
bound 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's restoreComposerAfterFailedSend exactly, and the
first 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 failed
doSend and by the question-card onFallbackSend that previously carried its
own 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 to pages.chatPage.send_failed only on the
transport-reject path where no body exists — a 409 slot agent mismatch is
actionable, "check your connection" is not (Opus 4.8). No new string ships.

Two other api.sendChat callers still swallow a failed send — App.tsx:1696
(feature-request flow) and useSceneInteraction.tsx:290 (fetch resolves on
4xx/5xx, so a refused send still reaches setSendState('sent')). First
Principles Review found them and confirmed the scope: neither ever carried
sendId meta, so neither was covered by the notice this PR removes and nothing
regresses. 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 optimisticTs also closes a defect First Principles flagged in the
same review: reconcileOptimisticEcho strips sendId and optimistic but
left optimisticTs (and a late-echo stale: true) in the persisted transcript
forever — 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.tssweepStaleOptimistic marks timed-out bubbles as stale and … does not mark fresh optimistic bubbles as stale are
    replaced by records no wall-clock timestamp on an optimistic bubble, which
    asserts optimistic === true and both optimisticTs and stale undefined.

  • website/src/test/UseWebSocketCoverage.test.tsx — the
    client-side optimistic timeout sweep (#3973) describe block (the interval
    dispatch test and its unmount-cleanup sibling) is deleted along with its
    imports.

  • website/src/test/ChatPane.dirSend.test.tsx — five new tests for the signal
    that 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), not
    replaced and not dropped; and retyping the same text mid-flight does not come
    back doubled; and a refused body surfaces the server's own error string while
    a transport reject falls back to the catalog entry. Only the fallback case is
    asserted loosely (non-empty role: 'error'), so no assertion pins catalog
    wording.

Net: 2 tests deleted, 7 added, 1 describe block removed.

Run locally: npx tsc -b clean; npm run lint 0 errors (627 pre-existing
warnings, unchanged); all i18n gates OK (i18n:check — 540 untranslated across
115 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 wider
chatSlice|UserMessage|useWebSocket|ChatPage|i18n|locale sweep passed
124 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 -b type-checks the whole project, eslint
covers all of src, and a repo-wide grep for sweepStaleOptimistic,
OPTIMISTIC_TIMEOUT_MS, optimisticTs and message_unconfirmed returns no
hits 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.ts retains the #3898 coverage (pipelined-send
reconciliation, the steer boundary, sendId strip), and those 225 tests pass
unchanged, 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 UserMessage against the real stylesheet and theme tokens
and hands it the exact meta the reducer used to write. The state being
photographed 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 shotsbefore from the base commit,
after from 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.

before (base commit) after (this branch)
before dark after dark
Light theme
before (base commit) after (this branch)
before light after light

The capture script (website/scripts/capture-user-message-unconfirmed.mjs) is
self-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 the expect
argument it was given. So a before shot cannot be taken from a checkout that
no longer draws the notice, and an after shot cannot be taken from one that
still does — a mislabelled pair fails instead of emitting a misleading image.
Verified in both directions locally: expect=present against this branch fails
with 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 (the
defect #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

  • Single commit with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable) — N/A, no spec documents this notice
  • No secrets, credentials, or internal references in the diff

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 recovery
strips 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.

@CrysisDeu
CrysisDeu requested a review from a team August 17, 2026 19:09
@CrysisDeu
CrysisDeu requested a review from a team as a code owner August 17, 2026 19:09
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Aug 17, 2026
@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

Advisory design-level review of 6bc78312f948868f8f3383548be10e2fb657d0bc — updated in place on each push; does not block merge.

Design-Verdict: CONCERNS

Sound subtraction with real parity fix, but it patches a server-side lie ({ok, queued} returned unconditionally) in one client caller.

Watch

  • The body.queued && !llm.trim() guard encodes chat_handlers.py:366's internals — the server answers {ok: true, queued: true} even when if message: skipped the queue — into one frontend caller. Root cause is the server reporting an acceptance that didn't happen; until it answers truthfully (or queues attachment-only sends), every other sendChat caller, including ChatPage which has no such guard, still trusts the lie. chat send: an unreadable receipt is reported as a refused send, in both send paths #4217 doesn't list this; file the server fix explicitly.
  • The abort path assumes "reaching the bound means the request WAS received" — a POST black-holed before the gateway (dead proxy, wedged connection) also aborts at 10s and is now silently ignored, with composer cleared and no error row. That is the one loss case the removed 30s notice covered and the replacement deliberately doesn't; the mitigation is only the visible absence of a reply. Acceptable as parity with ChatPage, but it's a shared hole, not a closed one.

[DESIGN-REVIEWED] 6bc7831

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 6bc78312f948868f8f3383548be10e2fb657d0bc — this comment is updated in place on each push.

Review details

Both candidates' premises are verifiable, and both are falsified:

CANDIDATE 2 (attachment-only queued send false-report): I confirmed the backend at chat_handlers.py:366-380 — the queue path guards queue_append with if message: and returns {ok, queued} unconditionally. Attachments ride via meta, not the message body. So a file-only send (empty wire text) while busy is genuinely not queued or broadcast by the backend — nothing carries the attachment. The frontend's if (body.queued && !llm.trim()) therefore correctly detects a real drop; reporting it is accurate, not a false report. The candidate's worry ("if the server ever queues files") is exactly the case that does not occur. Falsified — (c) has no observable wrong outcome.

CANDIDATE 1 (dead-socket lost signal): The removed sweep fired on every dashboard send (no chat_message echo ever comes back for dashboard sends), so it was a false-positive indicator on delivered messages — its removal is a deliberate, documented tradeoff. On a dead connection, a fetch that rejects promptly hits .catch → not AbortError → reportFailedSend() (error row + composer restore) — a stronger signal than the removed notice. Only the narrow hung-until-10s-abort case reports nothing, and that path deliberately mirrors ChatPage.send's established "received, WS will deliver" rule to avoid duplicating an in-flight turn. The candidate's own outcome is speculative ("may not have been delivered", "could not confirm"). Does not clear the 80+ bar for a defect in the changed lines.

No dangling references to the removed sweepStaleOptimistic / OPTIMISTIC_TIMEOUT_MS / optimisticTs symbols remain outside the updated tests, so nothing is broken by the removal.

No grounded Step 2 finding survives falsification.

No findings.

[OPUS-REVIEWED] 6bc7831

Verdict parsed from the review's SHA-scoped output markers for commit 6bc78312f948868f8f3383548be10e2fb657d0bc.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 6bc78312f948868f8f3383548be10e2fb657d0bc: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Advisory premise-level review of 6bc78312f948868f8f3383548be10e2fb657d0bc — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push; does not block merge.

I've read the review contract, the intent file, and the full patch, and verified the surrounding code: ChatPage.tsx's existing failure path (restoreComposerAfterFailedSend, the inline 10s abort), the backend handler at src/kiro_crew/dashboard/chat_handlers.py:366-380 (queues if message: yet returns {ok, queued} unconditionally), prepareSendPayload in fileTokens.ts (ChatPage always emits [attached_file N]/image markdown, so only the pane can produce an empty wire text), the capture-harness convention (151 sibling capture-*.mjs scripts, temp-screenshots/README.md), and that no reader of optimisticTs/stale/message_unconfirmed remains anywhere in website/src. Here is the review.

First-Principles-Verdict: CONCERNS

A well-earned deletion, but the queued && !llm.trim() guard patches on the client a receipt the server is still lying about to every other caller.

What this change ships

Intent: 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).

  1. "Message not confirmed" notice gone from under sent bubbles — justified (false by construction on every dashboard send, Dashboard messages show "Message not confirmed" after 30s: the confirmation signal is a server echo the composer path never emits #4131)
  2. Its English-only string removed from all 13 locale catalogs — justified
  3. 30s wall-clock sweep, 10s interval, and per-send timestamp deleted — justified (zero remaining readers, counted)
  4. A failed pane send now shows an error row and hands text/files back — rides along, declared; replaces the signal item 1 removed
  5. Pane send now bounded at 10s, abort deliberately not reported — declared; mirrors ChatPage's existing bound
  6. Attachment-only send a busy server silently dropped is now reported — symptom-level
  7. Question-card fallback recovery now dedupes and merges instead of blind-appending — rides along, declared
  8. Capture harness plus 4 committed review screenshots — per the documented temp-screenshots/ convention

Watch

  • Item 6 sits at SYMPTOM level with the cause named and in-repo: chat_handlers.py:366-380 queues if message: but answers {ok: True, queued: True} unconditionally — a false receipt. The guard at ChatPane.tsx:348 covers the only shipped client that produces empty wire text (ChatPage cannot: prepareSendPayload always emits attachment tokens — 0 sibling guards needed client-side), but app-token callers of the same endpoint still receive the lie. The description quotes the cause and fixes the symptom.
  • Two spellings of the merge-don't-clobber composer-restore rule now exist: restoreIntoComposer (ChatPane.tsx:253-261) and the textBack join in ChatPage.tsx:4027-4031 (count: 2). They will diverge.

Subtractions

  • Fix the cause server-side — stop returning queued: True for a message the handler dropped — then delete the body.queued && !llm.trim() guard at ChatPane.tsx:348: the existing !body.ok && !body.queued branch already reports it.

[FIRST-PRINCIPLES-REVIEWED] 6bc7831

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @CrysisDeu overrides the GPT 5.6 finding for 6bc78312f948868f8f3383548be10e2fb657d0bc; the recorded reason is authoritative for this commit.

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:
/ai-review override gpt 6bc78312f948868f8f3383548be10e2fb657d0bc: <one-sentence reason>

@github-actions

github-actions Bot commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

Advisory UX-level review of 6bc78312f948868f8f3383548be10e2fb657d0bc — updated in place on each push; does not block merge.

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

  • ChatPane.tsx if (body.queued && !llm.trim()) { reportFailedSend() } renders "Couldn't send — check your connection and try again" for a failure whose cause is a busy slot, not the network; a user who obediently checks their connection and retries into the same busy state gets the same false blame. Use a branch-specific string (e.g. "Couldn't attach the file while the agent is busy — try again when the turn finishes") instead of the generic pages.chatPage.send_failed.

[UX-REVIEWED] 6bc7831

@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 17, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/remove-unconfirmed-indicator branch from 76c96e4 to 56dbdff Compare August 17, 2026 19:21
@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 17, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/remove-unconfirmed-indicator branch from 56dbdff to dd870d2 Compare August 17, 2026 19:40
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Review disposition — GPT 5.6, BLOCKING

  • Split-pane send failures lose their only failure signal (website/src/hooks/useWebSocket.ts:1554)

    Offline ChatPane.doSend -> composer clears and appends an optimistic row -> rejected fetch is swallowed -> the undelivered message now appears sent forever. Fix: Restore the timeout metadata, sweep, and UserMessage status for unreconciled optimistic rows.

    fixed in dd870d270, though not by the remedy proposed — the defect is
    real and I had it wrong in the description.

    Verified: ChatPane.doSend calls setInput(''), appends the optimistic row,
    then api.sendChat(...).then(...).catch(() => undefined) — and the .then
    branch returns early on if (!cardAtSend && !askAtSend) before ever reading
    the body. So on that path a rejected fetch and a refused body were both
    silent, and the PR description's claim that "a genuinely failed send never
    reaches" the notice was true only of ChatPage. Corrected in the body.

    Not restoring the timeout metadata, sweep and status, because that reinstates
    a 30s wall-clock guess as the answer to a failure the code already knows about
    synchronously — and it would bring back the three properties this PR removes
    it for: it fires on delivered messages (slow first turn, dropped socket on a
    completed turn, a queued message), it offers no action, and it ships in
    English to 13 catalogs. Instead ChatPane.doSend now reports the failure the
    way ChatPage always has: an error row addressed to the slot that 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, on both the rejected
    fetch and a body the server accepted as neither ok nor queued. Recovery
    MERGES rather than clobbers, so a message typed during the in-flight window is
    not lost to restore an older one.

    This is strictly stronger than the notice on the axis the finding cares about:
    it fires immediately instead of after 30s, only when the send actually failed
    instead of on any unreconciled row, and it returns the payload instead of
    leaving the user to guess whether resending duplicates a delivered turn. It
    reuses the existing pages.chatPage.send_failed entry, so no new string ships.

    Pinned by four tests in website/src/test/ChatPane.dirSend.test.tsx: rejected
    send, refused body, accepted send (no row), and the no-clobber case.

@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 17, 2026
@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 17, 2026
@CrysisDeu
CrysisDeu force-pushed the fix/remove-unconfirmed-indicator branch from dd870d2 to 001ac08 Compare August 17, 2026 20:21
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Aug 17, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Review disposition — GPT 5.6 (advisory FINDING, head dd870d2)

  • "prev.trim() ? prev : text" discards the failed payload when a newer draft exists, so recovery does not merge as promised (website/src/components/ChatPane.tsx:291)

    Fix: append failed text and merge/deduplicate file arrays.

    fixed in 001ac0833, exactly as prescribed. The finding is correct and the
    comment above the code was the tell: it said "MERGE, never clobber" while the
    expression implemented newer-wins, which drops the very message the error row
    tells the user to try again. Worse, my own test asserted the dropping behaviour,
    so the defect was pinned rather than caught.

    Now mirrors ChatPage's restoreComposerAfterFailedSend semantics: empty
    composer takes the failed text; identical text is left alone; otherwise the
    failed text is APPENDED below the kept draft joined by a blank line. Files
    merge as a set union ([...prev, ...files.filter(f => !prev.includes(f))]) so a
    file the user re-picked mid-flight is not double-attached.

    Two tests replace the one that pinned the defect: the in-flight case now
    asserts newer work\n\nthe failing one, and a new sibling asserts that
    retyping the same text mid-flight does not come back doubled. 9/9 in that file.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Review disposition — UX Review, PASS with one suggestion (head dd870d2)

  • reportFailedSend's merge silently drops the failed payload when the user typed during the in-flight window, while the error row still says "try again"

    ChatPage's mirrored path appends the failed text; match it (same for pendingFiles, whose attachments have the same problem).

    fixed in 001ac0833. Taken as a defect rather than a suggestion: the copy
    and the behaviour contradicted each other, which is the same class of dishonesty
    this PR removes the notice for. The composer now APPENDS the failed text below
    the kept draft (blank-line separated, identical text not duplicated) and merges
    attachments as a set union, matching ChatPage. So "try again" now refers to
    something the composer actually still holds, in every ordering.

    GPT 5.6 reached the same finding independently on the same line; both are
    answered by this change, and each lane gets its own comment rather than one
    covering both.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Review disposition — First Principles Review, advisory CONCERNS (head dd870d2)

  • Item 6 is a point patch: of 6 production api.sendChat callers, two still leave a failed send looking sent — App.tsx:1696 and useSceneInteraction.tsx:290.

    Neither was covered by the removed notice (no sendId meta), so this PR regresses nothing — but the cause it names is left standing in 2 places.

    accepted-and-deferred, filed as Two api.sendChat callers still leave a failed send looking sent #4198. Both sites verified as described:
    App.tsx paints a user bubble, sets the slot running, then swallows the outcome
    in catch { /* WS will handle response */ } with no body check; and
    useSceneInteraction.tsx relies on fetch rejecting, which it does not do for
    an HTTP error status, so a refused send reaches setSendState('sent') — a state
    name asserting the opposite of what happened.

    Deferred rather than folded in, on the lane's own scoping: neither ever carried
    sendId, so neither was covered by the notice this PR removes, and the removal
    regresses nothing there. What this PR fixed is the one path where removing the
    notice would have taken away the only signal. Extending it means designing
    failure UX for a modal flow with no transcript to append to and for a scene
    interaction — two unrelated surfaces, each needing its own decision about where
    the error appears. Doing that inside a subtractive PR is the rider pattern this
    lane exists to catch.

    Two api.sendChat callers still leave a failed send looking sent #4198 is a task rather than a question: it names both sites with line numbers,
    the four-step shape to copy from the two existing references, and the extra
    response.ok check useSceneInteraction specifically needs.

@CrysisDeu
CrysisDeu force-pushed the fix/remove-unconfirmed-indicator branch from 001ac08 to 97d1778 Compare August 17, 2026 20:38
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Review disposition — Opus 4.8 (advisory FINDING, head 001ac08)

  • Send failures on a pane drop the server's specific reason and show a generic connectivity message (website/src/components/ChatPane.tsx:286)

    Fix: pass the parsed body into reportFailedSend and use body.error || i18nT('pages.chatPage.send_failed') for the .then path, keeping the generic string only for the .catch transport-reject path where no body exists.

    fixed in 97d1778d5, exactly as prescribed including the split between the
    two paths. The finding is right and the reference it cites is the one I was
    already claiming to mirror — ChatPage.tsx:4045 does body.error || i18nT(...)
    and I had copied only the fallback half. A 409 slot agent mismatch or a 503
    crew mode unavailable is something the caller can act on; telling them to
    check a connection that is fine is both wrong and useless.

    reportFailedSend now takes an optional reason, the .then path passes
    body.error, and the .catch path passes nothing so the connectivity copy is
    used only where there genuinely is no body to read. Two tests cover the split:
    a refused body asserts the exact server string (slot is stopping), and the
    transport-reject case asserts a non-empty row without pinning catalog wording.

    Also noted and agreed, no action: the observation that the optimistic bubble is
    not removed on failure matches ChatPage's established behaviour, and the
    removed notice was never the real failure signal since it fired on delivered
    rows too.

@CrysisDeu
CrysisDeu force-pushed the fix/remove-unconfirmed-indicator branch from d4f4fdd to e533831 Compare August 17, 2026 22:10
@github-actions github-actions Bot removed the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Aug 17, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — GPT 5.6 Review @ e533831d1

Both findings are accepted as real and deferred, with a human override recorded above and tracking in #4217. Neither is dismissed as a false positive.

The deferral has one reason, and it is checkable: both flagged patterns are copied verbatim from this repo's reference send path, ChatPage. Changing them in ChatPane alone would leave the two paths behaving differently on the same server response — the divergence the First Principles lane asked to collapse two rounds ago, and which this PR collapsed by making the pane mirror the reference.

Finding 1 — ChatPane.tsx:337, missing response mistaken for a refused send

Accepted. The mechanism is exactly as described: r.json().catch(() => ({})) turns an unreadable receipt from an accepted POST into {}, which fails the !body.queued && !body.ok test, so the user is told the send failed and handed the payload back to retry.

It is not this PR's pattern. ChatPage.tsx:4043-4049:

const body = await r.json().catch(() => ({}))
if (!body.queued && !body.ok) {
  dispatch(setSlotRunning(false))
  dispatch(appendMessage({ role: 'error', content: body.error || i18nT('pages.chatPage.send_failed'), cls: '' }))
  // The server explicitly accepted neither (`ok` nor `queued`), so nothing
  // was sent — recovering the composer cannot duplicate a delivered turn.
  restoreComposerAfterFailedSend()
}

The finding is in fact sharper than stated: that comment asserts the invariant the .catch breaks, so the reference documents a guarantee it does not have. Worth fixing — and worth fixing where the claim lives, not only in the mirror.

The proposed remedy ("report and restore only for a parsed response explicitly refusing the send; ignore unavailable or unreadable receipts") is the right direction but not a one-line guard: it introduces a third outcome — delivered-unknown — and what that state owes the user is undecided. Silence loses a genuinely failed send; a distinct row needs a translated string this PR has no catalog entry for; a delivery probe is new behaviour. That is the contract change, and it is what #4217 is for.

Finding 2 — ChatPane.tsx:255, recovery corrupts the newer draft

Accepted. Two trailing spaces are a Markdown hard break and prev.replace(/\s+$/, '') discards them. The remedy is correct: keep prev intact and use trim() only for the emptiness and equality checks.

Also not this PR's pattern — ChatPage.tsx:3851 and :4031 both do keepText.replace(/\s+$/, ''). This one is small and uncontroversial on its own; it is bundled into #4217 only because splitting it would touch the same three call sites twice.

What this PR does keep

This PR is subtractive: it removes the Message not confirmed — may not have been delivered indicator and its wall-clock sweep. The failure signal it adds to the pane exists because the removal would otherwise have left that path with none, and it is spelled the way the reference spells it — deliberately, so the two paths can be fixed together rather than drifting apart again.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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.

@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 17, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for e533831d1b53fa24c68c21330a42101187f47be3.

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.

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.
@CrysisDeu
CrysisDeu force-pushed the fix/remove-unconfirmed-indicator branch from e533831 to 6bc7831 Compare August 17, 2026 23:05
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

/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 6bc78312f

The only change from e533831d1 is a test-file syntax fix: ChatPane.dirSend.test.tsx was missing the two closing lines of the ChatPane pane boundary — data-chat-pane contract describe (an it's }) and the describe's })), dropped when the last rebase merged that block alongside this branch's. tsc -b did not catch it because the build project excludes tests, so the file was unparseable on the previous head and none of its 19 tests ran in CI. Frontend Lint & Type Check was the gate that caught it (449:0 Parsing error: '}' expected). No production file changed.

Both GPT findings therefore stand exactly as dispositioned for the previous head, and both remain accepted as real rather than dismissed:

  1. ChatPane.tsx:337 — an unreadable or truncated response body from an accepted POST collapses to {} and is then reported as a refused send, so a retry can duplicate a delivered turn. Same shape as ChatPage.tsx:4043-4049, whose own comment asserts the invariant the .catch breaks. The remedy introduces a third outcome (delivered-unknown) whose user-facing contract is undecided; that is chat send: an unreadable receipt is reported as a refused send, in both send paths #4217.
  2. ChatPane.tsx:255prev.replace(/\s+$/, '') discards a Markdown hard break. Same as ChatPage.tsx:3851 and :4031. Small and uncontroversial, bundled into chat send: an unreadable receipt is reported as a refused send, in both send paths #4217 only to avoid touching the same three call sites twice.

Gates on this head: tsc -b clean, lint 0 errors, i18n:check 0 untranslated added lines, and 1083 frontend tests pass across 52 files — including, for the first time on this branch, the 19 in the file above.

@github-actions

Copy link
Copy Markdown
Contributor

Human judgment recorded

@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 6bc78312f948868f8f3383548be10e2fb657d0bc.

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.

This decision applies only to this commit. A new push requires a new judgment.

@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 17, 2026
@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — Design Review @ 6bc78312f

Both Watch items are accepted as accurate. One produced an action in this cycle; the other is a real gap I am not going to paper over.

Watch 1 — the guard encodes the server's internals; file the server fix explicitly

Done: #4223. The lane is right that #4217 did not cover it — that issue is scoped to the frontend send-failure contract, and the false receipt is a backend defect with its own blast radius. I re-read the handler before filing rather than relying on the review text: if message: guards both queue_append and the queue_push broadcast, return web.json_response({"ok": True, "queued": True}) sits outside it, and the if not message: return 400 is after this branch, so it is unreachable for a busy slot.

The characterization "patches a server-side lie in one client caller" is correct and I am not defending the guard as the right long-term shape. #4223 states the intended end state explicitly: once the receipt is truthful, the guard is deleted, because !body.ok && !body.queued already covers that case. It is in this PR because the alternative was to ship the removal while leaving a silent attachment drop with no signal at all.

Watch 2 — a POST black-holed before the gateway aborts at 10s and is silently ignored

Accepted, and this is the sharpest thing said about the change. It is true that the removed 30s notice was the only surface that said anything in that case, and that the replacement deliberately does not. The distinction that makes it defensible is narrower than "parity": the notice fired on every optimistic message including delivered ones, because the chat_message echo it waited for is never broadcast for a dashboard send (#4131) — so it was not covering the black-hole case, it was firing indiscriminately and happened to include it. Trading an always-on false claim for an uncovered case is a net gain in honesty, not a wash.

But "shared hole, not a closed one" is the accurate summary and I am recording it as such rather than claiming otherwise. The abort case is exactly the delivered-unknown state #4217 has to define: a request that may or may not have arrived, which the current code resolves by assuming arrival. That assumption is why .catch returns early on AbortError — reporting failure there would invite a duplicate turn — and it is the assumption #4217 must replace with something better than a guess.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — First Principles Review @ 6bc78312f

The item-by-item read of what ships is accurate, including that items 4–8 ride along and are declared. Both Watch items and the Subtraction are accepted.

Watch 1 — item 6 is at symptom level, cause named and in-repo

Accepted without qualification, and filed as #4223. The lane's evidence matches what I verified in the handler myself: if message: guards queue_append and the queue_push broadcast, the {"ok": True, "queued": True} response does not, and the if not message 400 is unreachable for the busy path.

The "0 sibling guards needed client-side" count is also right and worth restating, because it is what keeps the guard from being a pattern: prepareSendPayload always emits [attached_file N] / image markdown, so the main chat path cannot produce an empty wire text and does not need the same branch. The pane is the only shipped UI that can. App-token callers of the same endpoint remain misinformed, which is precisely why the fix has to be server-side.

Subtraction — fix the cause, then delete the guard

Accepted as the intended end state, and #4223 says so in its own text rather than leaving it implied: once the receipt is truthful, the guard at ChatPane.tsx goes away because !body.ok && !body.queued already reports that case. I am not carrying the guard as permanent design.

Not done in this PR for one reason, stated plainly: this is a frontend removal, and changing what the chat endpoint returns is a backend contract change affecting every caller, with its own tests and its own failure modes. Bundling it here would make a subtractive PR into a cross-cutting one — the same objection that split the indicator removal out of #4168 in the first place.

Watch 2 — two spellings of merge-don't-clobber now exist

Accepted, with the exact locations the lane gives: restoreIntoComposer (ChatPane.tsx:253-261) and the textBack join (ChatPage.tsx:4027-4031), count 2. "They will diverge" is the right prediction — this PR itself is evidence, since the pane's version had to be corrected twice during review before it matched the reference's behaviour.

Tracked in #4217, which now also carries the two GPT findings that arise from exactly this duplication: the refusal test and the trailing-whitespace strip are shared verbatim with the reference, so fixing them in one spelling only would widen the divergence rather than close it. Collapsing the two spellings and fixing the shared defects is one change, not two.

@CrysisDeu

Copy link
Copy Markdown
Collaborator Author

Disposition — UX Review @ 6bc78312f

One suggestion, accepted as a real defect and deferred with a reason I would rather state than hide.

Suggestion — the busy-slot drop renders network advice

The criticism is correct and the mechanism is exactly as described: reportFailedSend() renders pages.chatPage.send_failed"Couldn't send — check your connection and try again" — for a failure whose cause is a busy slot. A user who follows that advice checks a working connection, retries into the same busy state, and gets the same false blame. The proposed copy ("Couldn't attach the file while the agent is busy — try again when the turn finishes") is better in substance: it names the real cause and gives an action that actually resolves it.

It is not in this PR because of what shipping it would require. I checked the catalog for an existing key that fits this case and there is none, so the change means adding a new user-facing string. This PR's entire purpose is removing a string that was English-only in all 13 locale catalogs — adding another untranslated one to fix the copy would reintroduce, in the same file, the defect being removed. That is not a scheduling excuse; it is the reason the fix belongs with a batch that carries translations.

Tracked in #4217 alongside the other send-failure copy and contract work.

Two things worth recording so the deferral is not mistaken for disagreement:

  • The imprecise advice is strictly better than what this path did before, which was nothing — the composer cleared, the optimistic bubble stayed, and the attachment vanished with no row at all. Wrong advice about a real failure beats silence about it, but it is still wrong advice.
  • The deeper fix removes the branch entirely rather than rewording it. chat API: busy slot returns queued:true for an attachment-only send it dropped #4223 (filed this cycle, from the Design and First Principles lanes) covers the server returning queued: True for a send it dropped; once the receipt is truthful, this branch and its copy problem both disappear, since the generic failure path already handles an honest refusal.

@github-actions github-actions Bot added readiness: passed Eligible automated validation passed for the current revision and removed readiness: checking Automated validation is still running labels Aug 17, 2026
@bolichen97
bolichen97 merged commit 8354057 into main Aug 18, 2026
65 of 68 checks passed
@bolichen97
bolichen97 deleted the fix/remove-unconfirmed-indicator branch August 18, 2026 19:26
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Aug 18, 2026
encomjp pushed a commit to encomjp/kirocrew-customapi that referenced this pull request Aug 22, 2026
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>
@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 #6825 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 #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.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants