Keep a New chat from being undone by the check it raced - #116
Conversation
`useBotThread` verifies a remembered thread on mount before handing it out, a network round-trip during which the chat renders. `startNew` — the New chat button — runs from an event handler, and its own mint is not held behind that check: `checkKnown` is a read rather than a mint, so the `mintingRef` guard that serialises the two mints is clear while it runs. So the two race. Press New chat while the mount-time check is still in flight, and if the mint resolves first, the check's resolution then runs `setThreadId(existing)` and puts the old thread back: the person asked for a new conversation, watched it appear, and landed in the previous one, with nothing on screen to say why. The `current` flag that resolution already reads only covers the agent changing or the component unmounting, not this. `startNew` now sets a ref the mount-time resolution reads, so a check about the thread the person just left cannot overwrite the one they just asked for. It is reset at the top of the effect, so a new agent resolves from scratch. This is the same shape as the `current` and `mountedRef` guards beside it, and like them it lives in the hook's async coordination — which is why `threadToUse`, the pure decision it wraps, is where the test is and stays green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Setting it at the press means a mint that fails leaves the mount-time check disarmed with nothing having replaced the thread, and the chat pane renders only when there is one: an empty screen, no message, no way back but a reload. Moving it after the guard costs nothing, since assignment and check both run to completion on one thread.
davidmckayv
left a comment
There was a problem hiding this comment.
The race is real and this closes it. Driven in the browser rather than argued from the code, because the interleaving is a timing bug and the app has no React test harness.
Setup: /api/threads/mint stubbed to 500, /api/threads/<id> delayed 6s, a remembered thread in localStorage, then a soft remount so the mount-time check is genuinely in flight when New chat is pressed.
- With the latch at the press:
chatPresent: false. The mint fails, the check finds itself disarmed, nothing replaces the thread, andbot.tsxrenders the chat only when there is one. Empty pane, no message, no way back but a reload. - With the latch moved after the
if (!minted) returnguard:chatPresent: true. The remembered conversation is restored.
Same script, same stubs, only the line moved. Pushed as 0b3614f.
It is not a weaker guard: the assignment and the check both run to completion on one thread, so a check resolving after it sees it, and one resolving before it has restored a thread this is about to replace, which is what was asked for.
typecheck, lint, format:check and bun test app (121 pass) are clean.
What this changes
A race in
useBotThread(from #81). The hook verifies a remembered thread on mount —checkKnown, a network round-trip — before handing the id out.startNew(the New chat button) runs from an event handler and its mint is not held behind that check:checkKnownis a read, not a mint, so themintingRefguard that serialises the two mints is clear while it runs.So the two race:
checkKnown(existing)is in flight.startNewmints a fresh thread.startNew's mint resolves first →setThreadId(minted).checkKnownresolves (known: true) → its handler runssetThreadId(existing).The new chat is silently replaced by the old one — the person asked for a fresh conversation, watched it appear, and landed back in the previous one, with nothing on screen to explain it. Exactly the "pressing New chat should never leave you worse off" the hook already reasons about, in the one window it did not cover: the
currentflag the resolution reads only catches the agent changing or the component unmounting.The fix mirrors the guards already there.
startNewsets astartedNewRef; the mount-time resolution reads it, so a check about the thread the person just left cannot overwrite the one they just asked for. It resets at the top of the effect, so a new agent resolves from scratch.Where it runs
useRefguard in the same hook, alongside the existingcurrentandmountedRef. Nothing crosses to another process.localStorage) is unchanged; the fix stops a stale write to React state, not to storage.Boundary and audit
Proof
bun run typecheck(app) — clean.bun test app/tests app/src— 121 pass, unchanged. The hook's async coordination is not unit-tested here (the existingcurrent/mountedRefguards are not either); the pure decision it wraps,threadToUse, is, and stays green. The fix is the same guard shape as those two, verified by the interleaving above.biome check— clean.One
useRefand one condition, matching the guards beside it; no storage, network, or decision logic changed.