fix(console-ai): a failed re-resolve must not drop the conversation it is re-reading (#2627) - #4183
Merged
Merged
Conversation
…t is re-reading (#2627) The AI build flow re-resolves the conversation it already holds at the worst possible moment: ADR-0057 A1.b bind-on-create waits for the build turn to end, then re-keys to `app:<pkg>:build` and navigates to `?package=`, and that scope flip fires one GET at the instant the server is still finishing the heaviest turn of the session. `useChatConversation` ended in a blanket `catch { setConversationId(undefined); setInitialMessages([]) }`. For a FIRST resolve that is right — nothing is held. For a re-resolve it is destructive: `AiChatPage` keys its chat pane on `${chatApi}:${conversationId ?? 'pending'}` and the thread lives inside the chat hook's instance, so `undefined` REPLACES the pane and the blueprint card, build summary and Publish button leave with the discarded instance — the reported blank conversation that only came back after switching threads and back. A failed resolve now keeps what it was re-reading when that is the conversation already held. This is the other half of the guard that already refused an EMPTY re-read the right to wipe hydrated messages; only the failing case was open. A resolve aimed at a DIFFERENT conversation, and a first resolve with nothing held, still clear — both pinned negatively. Also pins the rollback half of #2632's optimistic plan-card approve, which the fix promised but never covered: a `notSent` approval rolls the "Building…" badge back so the card is actionable again, while a streamed-response error and a superseding later send both leave it alone. Fixes the second half of #2627; the first half (the ~10s dead window) was already fixed by #2632 and is reported, not re-fixed.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
…finitive 404 (#2627) The guard's subject is a conversation the hook is still holding and re-reading. Once the server answers 404/403 the id is dead — the code has already cleared both its caches and is falling through to create a replacement — so a failure of THAT create must not preserve the dead id on screen. Narrows the guard to what it was argued for and pins the boundary.
Contributor
✅ Console Performance Budget
📦 Bundle Size Report
Size Limits
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Part of #2627
Two independent halves, premise-checked separately. Half 1 was already fixed and is reported, not re-fixed; half 2 reproduced and is fixed here — hence
Part of, notFixes.Half 1 — the ~10s dead window after 开始搭建: premise expired
Already fixed, by #2632 (commit
c0bd483, landed 2026-07-17 — the same day the staging observation was filed, so the observation predates the fix rather than surviving it).ChatbotEnhancednow flips the clicked plan card to aBuilding…badge with a spinner on submit, and the durableBuiltstate still derives from the message stream. The dead window's source was measured as stated in that PR's own comment and confirmed here: the approval's visible effect — a new user bubble plus a streaming turn — lands at the BOTTOM of the thread, outside the viewport when the plan card is what the user is looking at, so nothing at the card changed untilapply_blueprintbegan. It was never a slow approve request and never an ignored server event.What was NOT pinned was the rollback path the fix's comment promises, so this PR adds it (tests only, no behaviour change):
notSent— 429 / offline) rolls the badge back so the card is actionable again. Without it the badge is both a lie and a dead end: it replaces the buttons, so the user cannot re-approve at all and the build silently never starts.Reverse-verified individually: deleting the rollback limb reds only the first (
Unable to find … proposed-plan-approve— the card stays stuck onBuilding…); deleting the supersede line inhandleSubmitreds only the third.Half 2 — the history-clearing race: reproduced, fixed at the source
Reproduced through the real page before touching anything, driving the reported sequence: build turn streams →
apply_blueprint's draft lands and the Live Canvas opens (full-screen chat becomes the chat|preview split) → the turn ends → ADR-0057 A1.b bind-on-create re-keys the conversation toapp:<pkg>:buildand navigates to?package=.That last step is the one that matters: the scope flip re-resolves the SAME conversation, one GET fired at the instant the server is still finishing the heaviest turn of the session. On the happy path the measured pane-mount count across the whole transition is unchanged and the thread is intact — the existing guards hold. When that one GET fails, the pane remounts and the thread is gone.
Source:
useChatConversation's resolve effect ended in a blanketFor a FIRST resolve that is right — nothing is held, nothing is lost. For a re-resolve of the conversation the hook is ALREADY holding it is destructive, because of what the host does with the result:
AiChatPagekeys its chat pane on${chatApi}:${conversationId ?? 'pending'}, and the thread lives inside the chat hook's instance (useObjectChatseeds frominitialMessagesonce per mount —aiInitialMessageshas[]deps and useChat'sChatobject is created once). Soundefineddoes not re-render the pane, it REPLACES it, and the blueprint card, the build summary and the Publish button leave with the discarded instance — matching the report down to the recovery step, since switching threads and back re-hydrates from a server that has since persisted the turn.The fix keeps what a failed re-read was re-reading, when that is the conversation already held. This is deliberately the other half of a guard that was already in the same function: the same re-resolve returning NO messages mid-turn was already refused the right to wipe hydrated history (#2466's "never let that empty read wipe hydrated messages"); only the FAILING case was still open.
Fixed at the producer rather than by making the consumer tolerant: the hook was reporting "no conversation" while still holding a perfectly valid one, so a
?? lastKnownIdinAiChatPage's key would have papered over a wrong answer instead of correcting it.The guard's boundary (second commit)
The guard's subject is a conversation the hook is still holding and re-reading, and it stops being that the moment the server is DEFINITIVE. On a 404/403 the code has already cleared both of that id's caches and is falling through to mint a replacement, so the guard drops its claim there — a failure of that create clears, exactly as before. Without this the guard would have quietly widened into "keep the id whatever goes wrong", including keeping one the server had just declared dead.
Three negative halves are pinned alongside the positive one, so the guard cannot drift into that: a resolve aimed at a DIFFERENT conversation (a sidebar switch — leaving the old thread on screen under a URL naming another one would be worse), a first resolve with nothing held, and the 404-then-failing-create case above.
Reverse verification
Two levels, both run:
AiChatPage.buildHistorySurvives.test.tsx's failing-refetch case reds asexpected 3 to be 2: one extra pane mount, i.e. the remount that discards the thread. The happy-path case stays green, because it is not the defect.useChatConversation.test.tsxreds2 failed | 36 passed, both asexpected undefined to be 'conv-a'— the dropped id itself. The four negative pins stay green, so the reds are the guard and not collateral. Restored:38 passed.Verification
Local, from the repo root:
pnpm exec vitest run packages/app-shell/src/hooks/__tests__/useChatConversation.test.tsx— 38 passed (and the reverse-verification pair above).pnpm exec vitest run packages/app-shell/src/console/ai/__tests__/AiChatPage.buildHistorySurvives.test.tsx packages/plugin-chatbot/src/__tests__/ChatbotEnhanced.sendError.test.tsx— 9 passed.check:control-bytes,check:i18n-keys,check:i18n-drift— all green. No user-facing strings were added or changed, so there is no ten-pack work: the pins assert ondata-testids and on label props the test supplies itself.The full suites, type-check and lint were deliberately left to CI rather than duplicated on a shared container: Lint, Type Check, Build & E2E and all four Test shards are green on
6a533c39a.Generated by Claude Code