Skip to content

fix(chat): resolve the optimistic steer bubble against the steer receipt - #7658

Merged
bolichen97 merged 1 commit into
kirodotdev:mainfrom
rnoack1:fix/drop-optimistic-steer-bubble-when-queued
Sep 3, 2026
Merged

fix(chat): resolve the optimistic steer bubble against the steer receipt#7658
bolichen97 merged 1 commit into
kirodotdev:mainfrom
rnoack1:fix/drop-optimistic-steer-bubble-when-queued

Conversation

@rnoack1

@rnoack1 rnoack1 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Pressing Enter mid-turn renders the message twice, and one of the two copies
claims something that did not happen.

steer() optimistically appends the bubble with meta.steer, which draws the
"Steered into the running turn" badge. The mutation was explicitly
fire-and-forget (// fire-and-forget: no onSuccess), so nothing ever compared
that claim against the server's answer — and the backend does not always inject.
Two accepted receipt shapes contradict the badge:

  • {ok, queued} — the busy branch fell through to the queue, because the steer
    RPC was unavailable or the turn's teardown requeued the text. The backend
    broadcasts queue_push, so the same message is also drawn as an interactive
    queue card above the composer. The user sees their text twice: once as a card
    waiting for the next turn, once as a badge asserting it was injected into the
    current one.
  • {ok, slot, mid} — the POST raced chat_done, missed the busy branch entirely
    and started a new turn. The text ran, but it was not steered into anything.

The queued case has a second, more confusing symptom: switching away from the
session and back makes the badge disappear. That is the correct state showing
through — the transcript refetch has no steer row to rebuild, because the text is
in the queue and was never injected. The badge was the lie, and its vanishing
looked like a second, separate bug.

Why it matters

The badge is load-bearing for a decision the user makes constantly: whether their
correction landed in time to change the turn in flight, or is parked behind it.
Getting that wrong in the optimistic direction is the harmful direction — the
user believes the agent has been redirected and stops watching, while the text is
actually sitting in the queue and the turn continues on its original course.

The duplicate rendering compounds it: cancelling the queue card removes the card
and leaves the badge behind, so the transcript then shows an injection for a
message the user explicitly cancelled.

What changed (motivation → approach → change)

The receipt already carries everything needed to tell the three outcomes apart;
it was simply discarded. confirmedDelivered in utils/sendDelivery.ts had
already reasoned this through for the ordinary send path — "when it does queue,
it broadcasts queue_push, and that card is the server-owned representation of
the message" — but the steer path had no equivalent, and unlike a plain bubble a
steer bubble carries an affirmative claim rather than merely a pending one.

So the fix reads the answer and resolves the bubble against it, one remedy per
shape:

  • steered: true — the claim is true, nothing to do.
  • queued: true — the queue card owns the text, so the bubble is removed.
    Demoting it instead would leave a duplicate, and the queue drain later appends
    the real row on top of that.
  • accepted with neither flag (new turn) — the text ran, so the row stays and only
    meta.steer is dropped, demoting it to a plain user message.

Both mutating modes are gated on the bubble still being optimistic: once a
steer_push echo or confirmOptimisticSend has cleared that flag the server owns
the row, and a late receipt must not delete or rewrite it. The slot travels in the
mutation variables rather than being read off activeSlot inside the callback,
because the POST outlives the render that started it and the user can switch
sessions while it is in flight — the same reason the steer echo and
confirmOptimisticSend are slot-addressed.

An unreadable body (ok absent) is deliberately left alone rather than treated as
a failure: rewriting a rendered row on an answer we could not read would assert an
outcome that was never measured.

Tests

ChatPage.steerQueuedReceipt.test.tsx drives the real path — mount mid-turn,
type, press Enter (Steer is the default busy action), mock the receipt — and
asserts store state rather than the badge, since the flag is the input the badge
is derived from:

  • a queued receipt leaves no user row for the steered text (the queue card owns
    it),
  • a new-turn receipt keeps the row but with meta.steer cleared,
  • a steered receipt leaves the row and its flag untouched.

Verified as a negative control by reverting only the two source files and re-running:
the first two fail for exactly the intended reason (expected [ { role: 'user' } ] to have a length of +0 but got 1, and expected true to be falsy) while the third
still passes — so the tests discriminate rather than failing wholesale.

chatSlice.resolveOptimisticSteer.test.ts covers the guards a single send cannot
reach: a row the server already claimed is untouched, a non-matching sendId is a
no-op, a bubble living in slotMessages after a session switch still resolves, an
unsafe slot key is refused, and demotion preserves the row's sendId (the
reconciliation key for any later echo).

Full website suite: 1736 files / 27311 tests pass. tsc --noEmit clean; eslint
reports 0 errors and the same 8 pre-existing warnings as the base commit.

Manual verification

The frames below were captured in a real browser (Playwright-driven Chromium,
980px viewport at 2x) rather than asserted in jsdom, so the duplicate is visible
rather than inferred.

Screenshots / video

Both frames render the real UserMessage and QueueStack components against
store state produced by the real reducers: the optimistic steer bubble
(appendMessage with meta.steer), plus the queue card the backend's
queue_push broadcast creates (appendQueuedMessage). The only difference
between them is the receipt dispatch — before omits it, which is exactly what
main does since the mutation was fire-and-forget; after dispatches
resolveOptimisticSteer with the queued outcome the server actually returned.

Before — the same message twice: the badge asserting it was injected into the
running turn, and the queue card saying it is waiting for the next one.

Before: a steer bubble badged "Steered into the running turn" above a queue card holding the identical text

After — the queue card alone, which is where the text actually is.

After: only the queue card remains above the composer, with no steer bubble or badge

A DOM probe run on each frame in the same pass, so the images are not the only
evidence: before reports badge: true with one [data-role="user"] row and the
transcript reading "Steered into the running turn / <text>"; after reports
badge: false, zero user rows, and an empty transcript. The queue card is present
in both.

Related Issues

None — reported directly from a live session, with the duplicate render and the
badge disappearing on a session switch observed as two symptoms of this one cause.

Pattern harvest

Rule candidate: review-prompt
Pattern: an optimistic UI element that asserts a specific outcome (not merely
"pending") while its request is fire-and-forget — the assertion can never be
retracted when the server reports a different outcome, and the contradicting
server-owned representation renders alongside it.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

@rnoack1
rnoack1 requested a review from a team September 1, 2026 15:30
@rnoack1
rnoack1 requested a review from a team as a code owner September 1, 2026 15:30
@rnoack1
rnoack1 requested a review from pepmach September 1, 2026 15:30
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — ✅ PASS

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

The base code confirms the pieces: the badge is "Steered into the running turn" (en.json:6061, rendered in UserMessage.tsx), and the queue card renders in QueueStack directly above the composer — so on the queued outcome, the removed bubble's text is still visible, editable, and cancellable at the attention locus. The change removes a false affirmative claim and a duplicate rendering; the demote/remove semantics match the honest server state in each receipt shape, and the message text is never lost (the queue card owns it in the removal case).

One residual gap sits on a changed line: onError (ChatPage.tsx, comment rewritten in this diff) still only logs to console, so a steer POST that fails outright leaves the optimistic bubble wearing the same affirmative badge this PR exists to correct — low frequency, same impact class.

UX-Verdict: PASS

Removes a badge that lied about injection and a duplicate rendering; every receipt shape now resolves to the honest state, and no user text is lost.

Watch

  • A failed steer POST still keeps the "Steered into the running turn" badge: onError: (e) => { console.error('steer failed', e) } never touches the bubble, so a network/5xx failure leaves the exact affirmative lie this PR fixes for accepted receipts. Low frequency × same impact × persists until refetch. Smallest fix: on error, drop meta.steer from the still-optimistic bubble (the later steer_push echo re-confirms if it actually landed).

[UX-REVIEWED] bfb24a2

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — ✅ PASS

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

All claims in the description check out against the base tree: the three receipt shapes exist at chat_handlers.py:549-567 and :854-863, the new-turn JSON receipt is indeed gated on ws=1 (chat_handlers.py:637), and the reducer mirrors the established confirmOptimisticSend reconciliation pattern (slot-addressed, RECONCILE_WINDOW, both-arrays scan, unsafe-key guard). The fix is root-cause (the receipt was discarded; now it is read), the optimistic gate prevents late-receipt destruction of server-owned rows, and the tests were negative-controlled. The ws=1 addition also fixes the raced-new-turn arm streaming into an unread response — documented in the client comment, within scope.

Design-Verdict: PASS

Reads the receipt the fire-and-forget path discarded, reusing the codebase's existing optimistic-reconciliation pattern — root-cause fix, correctly guarded against late/racing receipts.

Suggestions

  • Drop temp-screenshots/ from the merge — attach the images to the PR instead of committing PR-evidence binaries into main's history.
  • On the turn outcome the receipt carries mid; also confirming the row (as confirmOptimisticSend does for plain sends) would light the pin control before the chat_done rebuild.

[DESIGN-REVIEWED] bfb24a2

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — ✅ PASS

Premise-level review of bfb24a25440f981afc2118b86ac027d06cf5beb0 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.

All evidence checked against the trusted base. Composing the review.

First-Principles-Verdict: PASS

Retracting an optimistic badge the server contradicted is a real, verified defect; every item in the diff serves that one retraction.

What this change ships

Intent: stop the steer bubble asserting "steered into the running turn" when the server actually queued the text or started a new turn — a FIX.

  1. Queued steer: badge bubble removed, queue card alone remains — justified
  2. New-turn race: bubble demoted to a plain user message, badge dropped — justified
  3. Steer POST now sends ws=1, so the raced arm answers JSON instead of SSE — justified (the new-turn receipt is unreachable without it; sendChat already uses it, client.ts:2990)
  4. In-flight steer resolves against its sending slot, surviving a session switch — justified
  5. New resolveOptimisticSteer reducer exported — 1 consumer (ChatPage onSuccess), both outcome variants constructed
  6. Two PNGs committed under temp-screenshots/steer-queued/ — rides along, but conventional (~200 sibling dirs; .gitignore names temp-screenshots/ as committed deliverables)

Verified against base: the three receipt shapes exist (chat_handlers.py:550,554,567,634; {ok, slot, mid} at :860-863 gated on ws_mode at :637); every queued: true arm has a queue_push broadcast (chat_delivery.py:394, chat_runner.py:4119, chat_handlers.py:626); confirmedDelivered deliberately excludes queued and cannot remove or demote a row, so this is not a second spelling of it. The subtractive alternative — never assert meta.steer optimistically and let the steer_push echo own the badge — is already weighed in-tree (ChatPage.tsx:6375 documents the echo lag that optimistic append exists to hide).

[FIRST-PRINCIPLES-REVIEWED] bfb24a2

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

Both queued: true arms of the steer endpoint broadcast a queue_push before returning the receipt — the fall-through queue path via queue_for_next_turn (chat_delivery.py:393) and the STEER_REQUEUED teardown path via _requeue_unconsumed_steers (chat_runner.py:4118). So the candidate's "persistently invisible" outcome cannot occur; what remains is at most a sub-frame flicker dependent on client-side WS-vs-HTTP arrival order, and the reducer's if (!m.meta?.steer || !m.meta?.optimistic) return true guard (chatSlice.ts:203) already skips the splice for any row the server has already echoed/claimed. Outcome (c) is speculative timing only — below the bar.

No findings.

[OPUS-REVIEWED] bfb24a2

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] bfb24a2

@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 Sep 1, 2026
@rnoack1
rnoack1 force-pushed the fix/drop-optimistic-steer-bubble-when-queued branch from 9a969c5 to b86da28 Compare September 1, 2026 18:52
@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 Sep 1, 2026
@rnoack1
rnoack1 force-pushed the fix/drop-optimistic-steer-bubble-when-queued branch from b86da28 to 590bca1 Compare September 1, 2026 21:38
@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 Sep 1, 2026
@rnoack1
rnoack1 force-pushed the fix/drop-optimistic-steer-bubble-when-queued branch from 590bca1 to 08fee21 Compare September 1, 2026 23:11
@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 Sep 2, 2026
@rnoack1
rnoack1 force-pushed the fix/drop-optimistic-steer-bubble-when-queued branch from 08fee21 to 2fc48aa Compare September 2, 2026 00:53
@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 Sep 2, 2026
@rnoack1
rnoack1 force-pushed the fix/drop-optimistic-steer-bubble-when-queued branch from e263ecf to 5e4195c Compare September 2, 2026 06:23
@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 Sep 2, 2026
@rnoack1
rnoack1 force-pushed the fix/drop-optimistic-steer-bubble-when-queued branch from 5e4195c to 964d95a Compare September 2, 2026 07:24
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision merge conflict Branch has merge conflicts with its base — author must resolve before merge and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
A steer the server queued instead of injecting kept its "Steered into the running turn" badge beside the queue card for the same text; before/after capture committed as screenshot evidence.
@rnoack1
rnoack1 force-pushed the fix/drop-optimistic-steer-bubble-when-queued branch from 964d95a to bfb24a2 Compare September 3, 2026 03:51
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: passed Eligible automated validation passed for the current revision readiness: action required A blocking check or review needs attention merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 3, 2026
@bolichen97
bolichen97 enabled auto-merge (squash) September 3, 2026 09:24

@bolichen97 bolichen97 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving on the strength of a full readiness audit of every open PR against main, not a
line-by-line reading of this diff — recording that plainly so the next reader knows what this
stamp does and does not cover.

Verified against this exact head SHA:

  • readiness: passed present, and PR Readiness — the one required status context on main
    (ruleset protected-branches) — is success on this head.
  • No check run on this head is failure, cancelled, timed_out or still in flight. Skipped
    jobs are path-filtered conditionals, none of them required.
  • mergeable: true, and the head is not far enough behind main for its green CI to describe a
    base that no longer exists.
  • No surviving reviewer CHANGES_REQUESTED: any such review is on an older commit and therefore
    already dismissed by dismiss_stale_reviews_on_push.
  • Every issue comment, inline review comment and review thread was read and classified. Nothing
    left is an unresolved human change request — the remainder is bot review-lane output, resolved
    or outdated threads, explicitly non-blocking suggestions, and author status notes.

Auto-merge (squash) is armed, so this lands once every other ruleset requirement is met.

@bolichen97
bolichen97 merged commit 6ade333 into kirodotdev:main Sep 3, 2026
68 checks passed
@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 3, 2026
@rnoack1
rnoack1 deleted the fix/drop-optimistic-steer-bubble-when-queued branch September 3, 2026 13:08
@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 #7997 is PARTIALLY_COVERED relative to this PR. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #7997: CONTINUE_DEVELOPMENT. Main covers the sibling routes only. The steered-then-never-consumed route this PR exists for is not implemented anywhere in current origin/main, so closure as completed is not available; the PR should land (after the open premise decision below) rather than be superseded. Files: website/src/pages/ChatPage.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

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants