fix(ai-chat): adopt the server-returned chat id (two-tab adoption race #137) #138

Ghost · 2026-06-22T23:47:21+03:00

Ghost commented

2026-06-22 23:47:21 +03:00

Fixes the AI-chat adoption race (#137), reproduced live with DB proof, plus a docs clarification.

The bug

A brand-new chat (activeChatId === null) had no way to learn the id of the row the server created — the SSE stream never returned it — so the client adopted the newest chat in the per-user list (chats.items[0]). With two tabs open, a second tab creating a chat at ~the same time made its row the newest, so the first tab adopted the wrong id: its later turns persisted into the other chat and the agent rebuilt history from it (commands "leaked" between chats), while the live UI still showed the original conversation.

The fix

Server (ai-chat.service.ts): attach the authoritative chatId to the streamed assistant message via the AI SDK messageMetadata on the start part, so it reaches the client on the very first chunk (before any racing tab).
Client (chat-thread.tsx, ai-chat-window.tsx): read message.metadata.chatId in useChat's onFinish and adopt that id in place (no remount → the live turn and the thread's chatIdRef follow the real id, so the next turn targets the right chat). Removed the chats.items[0] guess and the adoptNewChat ref.

Verified live (the two-tab race that confirmed the bug)

Tab A new chat "codeword FIG" → while A streams, Tab B new chat "codeword CIRCUS" → A asks again. Run 3×: A's both turns now stay in A's own chat row (the FIG one); A's 2nd turn no longer lands in B's chat. Single-tab new-chat (2 turns + reload → full history in the correct chat) and existing-chat flows unaffected. tsc clean (client + server).

Also: AGENTS.md migration note

Clarifies that DB migrations auto-apply only in production (built image / start:prod); the local dev stand (pnpm dev / nest start --watch) does not auto-run them — you must pnpm --filter server migration:latest after pulling/switching branches. (During this investigation, the unapplied 20260622…-ai-chat-page-origin migration blanket-500'd all of AI chat on the dev stand — an env gotcha, not a code bug.)

🤖 Generated with Claude Code

Fixes the AI-chat **adoption race** (#137), reproduced live with DB proof, plus a docs clarification. ### The bug A brand-new chat (`activeChatId === null`) had no way to learn the id of the row the server created — the SSE stream never returned it — so the client adopted the **newest** chat in the per-user list (`chats.items[0]`). With two tabs open, a second tab creating a chat at ~the same time made its row the newest, so the first tab adopted the **wrong** id: its later turns persisted into the other chat and the agent rebuilt history from it (commands "leaked" between chats), while the live UI still showed the original conversation. ### The fix - **Server** (`ai-chat.service.ts`): attach the authoritative `chatId` to the streamed assistant message via the AI SDK `messageMetadata` on the `start` part, so it reaches the client on the very first chunk (before any racing tab). - **Client** (`chat-thread.tsx`, `ai-chat-window.tsx`): read `message.metadata.chatId` in `useChat`'s `onFinish` and adopt that id **in place** (no remount → the live turn and the thread's `chatIdRef` follow the real id, so the next turn targets the right chat). Removed the `chats.items[0]` guess and the `adoptNewChat` ref. ### Verified live (the two-tab race that confirmed the bug) Tab A new chat "codeword FIG" → while A streams, Tab B new chat "codeword CIRCUS" → A asks again. Run 3×: A's **both** turns now stay in A's own chat row (the FIG one); A's 2nd turn no longer lands in B's chat. Single-tab new-chat (2 turns + reload → full history in the correct chat) and existing-chat flows unaffected. `tsc` clean (client + server). ### Also: AGENTS.md migration note Clarifies that DB migrations auto-apply **only in production** (built image / `start:prod`); the **local dev** stand (`pnpm dev` / `nest start --watch`) does **not** auto-run them — you must `pnpm --filter server migration:latest` after pulling/switching branches. (During this investigation, the unapplied `20260622…-ai-chat-page-origin` migration blanket-500'd all of AI chat on the dev stand — an env gotcha, not a code bug.) 🤖 Generated with [Claude Code](https://claude.com/claude-code)

Ghost added 2 commits 2026-06-22 23:47:21 +03:00

fix(ai-chat): adopt the server-returned chat id, not the newest in the list 1858a5800d

A brand-new chat (activeChatId === null) had no way to learn the id of the row
the server created: the SSE stream never returned it, so the client adopted the
NEWEST chat in the per-user list (chats.items[0]). With two tabs open, a second
tab creating a chat at ~the same time made its row the newest, so the first tab
adopted the wrong id — its later turns persisted into the other chat and the
agent rebuilt history from it (commands leaked between chats), while the live UI
still showed the original conversation. (#137)

The server now attaches the authoritative chatId to the streamed assistant
message via the AI SDK messageMetadata on the 'start' part, so it reaches the
client on the first chunk. The client reads message.metadata.chatId in useChat's
onFinish and adopts that id in place (no remount, so the live turn and the
thread's chatIdRef follow the real id and the next turn targets the right chat).
The chats.items[0] guess and the adoptNewChat ref are removed.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs(agents): clarify where DB migrations auto-apply (prod) vs not (dev) 0edc5aeda8

Migrations auto-run on boot only in production (the built image / start:prod);
the local dev stand (pnpm dev / nest start --watch) does NOT auto-run them, so
after pulling or switching branches you must apply them with
'pnpm --filter server migration:latest' or endpoints touching new columns 500
(e.g. a freshly-added ai_chats.page_id blanket-500s all of AI chat).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

vvzvlad commented

2026-06-23 00:01:58 +03:00

Code review — adoption race fix (#137)

Verdict: Request changes. The core fix is correct and genuinely closes #137 — streaming the authoritative chatId in message metadata and adopting it in place (instead of guessing chats.items[0]) is the right approach. Security / conventions / documentation / regressions / simplification all came back clean. Two things to address before merge, plus one non-blocking architecture note.

⚠️ Warning — a new-chat first turn that fails before the `start` chunk can create a duplicate chat row

Adoption is now gated on a truthy serverChatId, which only exists once the AI SDK has processed the stream's start part. But the server creates the chat row and persists the user message before the stream is wired (apps/server/src/core/ai-chat/ai-chat.service.ts ~232–261). If anything fails in that window — a history DB read, convertToModelMessages, or any synchronous throw before the first chunk (all re-thrown by the try/catch at ~520 → the controller writes a bare 500 with no SSE) — the client never receives a chatId.

On that path both onFinish({ isError }) and onError call onTurnFinished(undefined), so adoption is skipped and chatIdRef.current stays null. The composer is still enabled, so the user retries → the next send carries chatId: null → the server inserts a second chat row, orphaning the first turn in the first row.

The old code adopted unconditionally (via the adoptNewChat ref + the chat-list refetch), so it didn't have this gap — the base onError comment even states the intent: "so a brand-new chat that fails its first turn is adopted." The PR silently drops that (and that comment is now stale).

Fix: for the activeChatId === null && !serverChatId branch, don't drop adoption silently — e.g. gate the next send so a failed first turn can't create a second row, or fall back to the list refetch only when there is exactly one unambiguous candidate. Also update the now-stale onError comment.

⚠️ Test coverage — the fix ships with no tests

This is a concurrency bug fix with no regression test; nothing locks #137 in.

Client adoption decision (onTurnFinished) has three branches — adopt / don't-adopt / skip — and none is tested, including the don't-adopt branch where the warning above lives. The project already extracts orchestration logic out of chat-thread into utils/queue-helpers.ts with a queue-helpers.test.ts; the same pattern fits a pure adoptedChatId(activeChatId, serverChatId) helper. A test that returns serverChatId (not "newest in list") is the regression guard for #137.
Server messageMetadata (attaching chatId only on the start part) has no test — ai-chat.service.spec.ts is untouched. If it ever regresses (wrong part / wrong field), #137 quietly returns. Extract the callback to an exported helper and assert start → { chatId }, finish → undefined.

💡 Architecture (non-blocking, forward-looking)

This is the 2nd id-handling fix in this exact spot. The new→persisted chat identity is spread across ~5 cooperating state slots (activeChatId atom, liveThreadChatId, threadKey, chatIdRef, chatStoreId) plus a render-phase guard and a load-bearing "both updates batch in one render" assumption. This PR improves the trigger (drops the heuristic useEffect + the adoptNewChat ref) but leaves the scattering in place. When this area is next touched, worth considering either letting ChatThread own its own id transition, or modeling the lifecycle as a small { new | adopting | persisted } state machine so the "stale liveThreadChatId" state becomes unrepresentable. Not a merge blocker.

_{Multi-aspect review: security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture. The stability finding was verified against the controller/service error path.}

## Code review — adoption race fix (#137) **Verdict: Request changes.** The core fix is correct and genuinely closes #137 — streaming the authoritative `chatId` in message metadata and adopting it in place (instead of guessing `chats.items[0]`) is the right approach. Security / conventions / documentation / regressions / simplification all came back clean. Two things to address before merge, plus one non-blocking architecture note. ### :warning: Warning — a new-chat first turn that fails before the `start` chunk can create a **duplicate chat row** Adoption is now gated on a truthy `serverChatId`, which only exists once the AI SDK has processed the stream's `start` part. But the server creates the chat row and persists the user message **before** the stream is wired (`apps/server/src/core/ai-chat/ai-chat.service.ts` ~232–261). If anything fails in that window — a history DB read, `convertToModelMessages`, or any synchronous throw before the first chunk (all re-thrown by the `try/catch` at ~520 → the controller writes a bare 500 with no SSE) — the client never receives a `chatId`. On that path both `onFinish({ isError })` and `onError` call `onTurnFinished(undefined)`, so adoption is skipped and `chatIdRef.current` stays `null`. The composer is still enabled, so the user retries → the next send carries `chatId: null` → the server inserts a **second** chat row, orphaning the first turn in the first row. The old code adopted unconditionally (via the `adoptNewChat` ref + the chat-list refetch), so it didn't have this gap — the base `onError` comment even states the intent: *"so a brand-new chat that fails its first turn is adopted."* The PR silently drops that (and that comment is now stale). **Fix:** for the `activeChatId === null && !serverChatId` branch, don't drop adoption silently — e.g. gate the next send so a failed first turn can't create a second row, or fall back to the list refetch only when there is exactly one unambiguous candidate. Also update the now-stale `onError` comment. ### :warning: Test coverage — the fix ships with no tests This is a concurrency bug fix with no regression test; nothing locks #137 in. - **Client adoption decision (`onTurnFinished`)** has three branches — adopt / don't-adopt / skip — and none is tested, including the don't-adopt branch where the warning above lives. The project already extracts orchestration logic out of `chat-thread` into `utils/queue-helpers.ts` with a `queue-helpers.test.ts`; the same pattern fits a pure `adoptedChatId(activeChatId, serverChatId)` helper. A test that returns `serverChatId` (not "newest in list") is the regression guard for #137. - **Server `messageMetadata`** (attaching `chatId` only on the `start` part) has no test — `ai-chat.service.spec.ts` is untouched. If it ever regresses (wrong part / wrong field), #137 quietly returns. Extract the callback to an exported helper and assert `start → { chatId }`, `finish → undefined`. ### :bulb: Architecture (non-blocking, forward-looking) This is the 2nd id-handling fix in this exact spot. The new→persisted chat identity is spread across ~5 cooperating state slots (`activeChatId` atom, `liveThreadChatId`, `threadKey`, `chatIdRef`, `chatStoreId`) plus a render-phase guard and a load-bearing "both updates batch in one render" assumption. This PR improves the *trigger* (drops the heuristic `useEffect` + the `adoptNewChat` ref) but leaves the scattering in place. When this area is next touched, worth considering either letting `ChatThread` own its own id transition, or modeling the lifecycle as a small `{ new | adopting | persisted }` state machine so the "stale `liveThreadChatId`" state becomes unrepresentable. Not a merge blocker. --- <sub>Multi-aspect review: security · stability · conventions · documentation · regressions · test-coverage · simplification · architecture. The stability finding was verified against the controller/service error path.</sub>