fix(ui): don't evict a user message sent during local-model warm-up (#11670) - #11698
Conversation
…11670) The post-turn history reload (loadConversationMessages) full-replaces the thread with server truth. When the server never persisted the user's turn — a send during local-model warm-up where the runtime-ready hold expired (503 "Agent is not running"), or a stream that completed with nothing persisted — that reload returned a thread WITHOUT the user's message and silently evicted the optimistic bubble: the user's words vanished with no failed state and no way to recover them. Fix (root cause, in useChatSend): - restoreEvictedUserTurn(): after every post-send reconcile reload, detect that the just-sent user turn is missing from server truth (text match among user turns no older than the send minus clock-skew slack) and re-attach the user's bubble plus a retryable failed assistant turn (failureKind: provider_issue -> the overlay's existing Retry chip). The send now fails loudly and one tap re-delivers it once the model is ready. - Wired into both reload sites of runQueuedChatSend (success path + non-404 error path) and both sites of sendActionMessage. Validation-4xx rejects are excluded (their draft is restored to the composer, re-attaching would duplicate it); no-op when the server persisted the turn or the reload never replaced local state. - handleChatRetry fallback now also drops the optimistic (temp-) user turn it retries, so the resend's fresh optimistic bubble doesn't transiently duplicate the message. Tests: - 8 new regression tests in useChatSend.test.tsx that emulate the REAL full-replace reload semantics (the previous no-op reload mock is exactly why this eviction never showed up in the suite). 6 of them red on develop's useChatSend, green with the fix; 2 guard the no-false-positive side (persisted turn -> no duplicate, validation reject -> composer restore only). - New real-browser e2e (run-warmup-eviction-e2e.mjs): the REAL useChatSend pipeline + real ContinuousChatOverlay, warm-up 503 simulated at the client-API boundary; real typing/clicks; asserts bubble survival, Retry chip, and exactly-once delivery after retry. Screenshots committed. Closes #11670 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…cript-only) Develop moved again mid-fix: #11698 added run-warmup-eviction-e2e.mjs with a packages/ui test:warmup-eviction-e2e script but no workflow leg, so the ui-e2e-runner-coverage ratchet stayed red. Add the gate leg next to its shell siblings + the artifact upload path. Runner verified green locally via the exact CI invocation: 8/8 checks, exit 0. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cript-only) (#11704) Develop moved again mid-fix: #11698 added run-warmup-eviction-e2e.mjs with a packages/ui test:warmup-eviction-e2e script but no workflow leg, so the ui-e2e-runner-coverage ratchet stayed red. Add the gate leg next to its shell siblings + the artifact upload path. Runner verified green locally via the exact CI invocation: 8/8 checks, exit 0. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
Closes #11670
Root cause
useChatSend's post-turn reconcile (loadConversationMessages) full-replaces the thread with server truth. When the server never persisted the user's turn — a send during local-model warm-up where the runtime-ready hold expired (503 Agent is not runningfromconversation-routes.ts; the user message is only persisted after runtime resolution), or a stream that completed empty with nothing persisted — the reload returned a thread without the user's message and silently evicted the optimistic bubble. No failed state, no notice on the empty-success variant, text unrecoverable. The suite never caught it because itsloadConversationMessagesmock was a no-op{ ok: true }instead of the real full-replace.Fix (root cause, no fallback sludge)
restoreEvictedUserTurn()— after every post-send reconcile, detect that the just-sent user turn is missing from server truth (text match among user turns no older than the send minus 60s clock-skew slack, so an identical message from an earlier exchange can't mask an eviction) and re-attach the user's bubble plus a retryable failed assistant turn (failureKind: "provider_issue"→ the overlay's existing Retry chip). The send fails loudly; one tap re-delivers once the model is ready — mirroring the existingreattachInterruptedPartialpattern for assistant partials.runQueuedChatSend(success + non-404 error path) and both sites ofsendActionMessage. Validation-4xx rejects excluded (their draft goes back to the composer — re-attaching would duplicate). No-op when the server persisted the turn or the reload never replaced local state.handleChatRetryfallback now also drops the optimistic (temp-) user turn it retries, so the resend's fresh optimistic bubble doesn't transiently duplicate the message.Evidence
useChatSend.test.tsxthat emulate the REAL full-replace reload. 6 red against develop'suseChatSend.ts(verified by swapping it in:6 failed | 2 passed), all green with the fix. 2 guard the no-false-positive side (persisted silent turn → no duplicate/no spurious failed turn; validation reject → composer restore only). Full file: 40/40 green. Two pre-existing fixtures updated from toytimestamp: 1to realistic epoch-ms (a persisted turn's real timestamp) — intent unchanged.bun run --cwd packages/ui test:warmup-eviction-e2e— bundles the REALuseChatSendpipeline + realContinuousChatOverlay, warm-up 503 simulated at the client-API boundary, real typing/Enter/click in headless chromium. 8/8 assertions green (screenshots + console log committed undersrc/components/shell/__e2e__/output-warmup-eviction/). Same harness against develop'suseChatSend: reds exactly at "user bubble SURVIVES the warm-up 503 + reconcile reload".output-warmup-eviction/console.logshows the real path: send → 503 notice →reload conv-1: server holds 0 messages(the eviction moment) → retry →server holds 2 messages.bunx vitest run src/state/ src/components/shell/→ 95 files, 1166 passed.bun run --cwd packages/ui typecheckclean. Biome clean on touched files. Re-verified after rebase ontoorigin/develop(cdd87af).resolveRuntimeForChatTurn) is unchanged and is what the boundary simulation reproduces.🤖 Generated with Claude Code