From 1858a5800da48143a73f00d0be299d7ab5fc4524 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Mon, 22 Jun 2026 23:46:50 +0300 Subject: [PATCH 1/7] fix(ai-chat): adopt the server-returned chat id, not the newest in the list MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../ai-chat/components/ai-chat-window.tsx | 92 +++++++++---------- .../ai-chat/components/chat-thread.tsx | 18 +++- .../src/core/ai-chat/ai-chat.service.ts | 10 ++ 3 files changed, 67 insertions(+), 53 deletions(-) diff --git a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx index 6d543289..52732e79 100644 --- a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx +++ b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx @@ -101,7 +101,8 @@ function clampGeom(g: { left: number; top: number; width: number; height: number /** * Floating, draggable, resizable, minimizable AI chat window. Replaces the * former right-aside `AiChatPanel`: it owns ALL chat orchestration (active - * chat, new chat, adopt-new-chat, open-page context, token sum) and wraps the + * chat, new chat, in-place id adoption from streamed metadata, open-page + * context, token sum) and wraps the * reused inner components (ConversationList + ChatThread) in window chrome * ported from the GitmostAgent.jsx design. */ @@ -132,10 +133,6 @@ export default function AiChatWindow() { // left partly off-screen). const [geom, setGeom] = useAtom(aiChatWindowGeomAtom); - // Track whether we are awaiting the id of a just-created (new) chat, so we - // can adopt it once the chat list refreshes after the first turn finishes. - const adoptNewChat = useRef(false); - // Latch: the chat id whose full persisted history has finished loading while // its thread is mounted. Used so a later BACKGROUND refetch (the post-turn // messages invalidation) never tears the live thread back down to the loader. @@ -197,9 +194,6 @@ export default function AiChatWindow() { : null; const startNewChat = useCallback((): void => { - // Cancel any pending adoption so a just-finished new chat can't yank the user - // back here after they explicitly started a fresh one. - adoptNewChat.current = false; setActiveChatId(null); setHistoryOpen(false); setDraft(""); @@ -209,8 +203,6 @@ export default function AiChatWindow() { const selectChat = useCallback( (chatId: string): void => { - // Cancel any pending adoption so it can't override an explicit selection. - adoptNewChat.current = false; setActiveChatId(chatId); setHistoryOpen(false); setDraft(""); @@ -222,25 +214,42 @@ export default function AiChatWindow() { ); // After a turn finishes, refresh the chat list. For a brand-new chat (no id - // yet), the server has just created the row; adopt the newest chat id so the - // thread switches from "new" to the persisted chat (and loads its history on - // later opens). - const onTurnFinished = useCallback(() => { - if (activeChatId === null) adoptNewChat.current = true; - queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }); - // Re-sync the persisted message rows for the active chat so the Markdown - // export and the token counters reflect the turn that just finished. The - // live thread renders from its own useChat store (stable threadKey / store - // id), so refetching these rows never re-seeds or tears down the open - // thread. For a brand-new chat activeChatId is still null here; that chat's - // first row load happens right after id adoption, and every later turn hits - // this invalidation with the adopted id. - if (activeChatId) { - queryClient.invalidateQueries({ - queryKey: AI_CHAT_MESSAGES_RQ_KEY(activeChatId), - }); - } - }, [activeChatId, queryClient]); + // yet), the server has just created the row and reported its AUTHORITATIVE id + // via the streamed assistant message metadata (`serverChatId`); we adopt THAT + // exact id so the thread switches from "new" to its OWN persisted chat — never + // guessing the newest chat in the list, which would adopt a SIBLING chat that + // a second tab created in the same moment and leak this tab's later turns into + // it (the two-tab adoption race, #137). + const onTurnFinished = useCallback( + (serverChatId?: string) => { + if (activeChatId === null && serverChatId) { + // In-place adoption: move the active chat AND the live-thread marker to + // the real id together, so the render-phase switch check below sees no + // "switch" and keeps the SAME mounted thread (its useChat already holds + // the just-finished turn, and its chatIdRef now mirrors the real id so + // the SECOND turn sends the correct chatId) instead of remounting and + // re-seeding from not-yet-persisted history. React's automatic batching + // inside this callback lands both updates in one render, so the guard + // never observes the new activeChatId with a stale liveThreadChatId. + setLiveThreadChatId(serverChatId); + setActiveChatId(serverChatId); + } + queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }); + // Re-sync the persisted message rows for the active chat so the Markdown + // export and the token counters reflect the turn that just finished. The + // live thread renders from its own useChat store (stable threadKey / store + // id), so refetching these rows never re-seeds or tears down the open + // thread. For a brand-new chat activeChatId is still null here; that chat's + // first row load happens right after id adoption, and every later turn hits + // this invalidation with the adopted id. + if (activeChatId) { + queryClient.invalidateQueries({ + queryKey: AI_CHAT_MESSAGES_RQ_KEY(activeChatId), + }); + } + }, + [activeChatId, queryClient, setActiveChatId], + ); // The active chat object (for its title) and an export gate: only enable the // export button when an existing chat with loaded persisted rows is active. @@ -294,26 +303,11 @@ export default function AiChatWindow() { notifications.show({ message: t("Copied") }); }, [activeChatId, messageRows, activeChat, clipboard, t]); - // When awaiting a new chat's id, adopt the most-recent chat (the list is - // ordered newest-first) once it appears. - useEffect(() => { - if (!adoptNewChat.current) return; - const newest = chats?.items?.[0]; - if (newest) { - adoptNewChat.current = false; - // In-place adoption: move the active chat AND the live-thread marker to the - // new id together, so the threadKey derivation below sees no "switch" and - // keeps the SAME mounted thread (its useChat already holds the finished - // turn) instead of remounting and re-seeding from not-yet-persisted history. - // ASSUMPTION: these two updates (jotai atom + useState) must land in ONE - // render so the render-phase guard never observes the new activeChatId with - // a stale liveThreadChatId (which would wrongly remount). React 18 automatic - // batching inside this effect callback guarantees that; if the store/atom - // mechanism ever changes, gate adoption on an explicit flag instead. - setLiveThreadChatId(newest.id); - setActiveChatId(newest.id); - } - }, [chats, setActiveChatId]); + // NOTE: new-chat id adoption is no longer a heuristic effect. The server now + // reports the authoritative created chat id on the streamed assistant message + // metadata, and `onTurnFinished(serverChatId)` adopts THAT id in place — see + // above. The old "newest chat in the list" guess was removed because it raced + // a second tab's simultaneous new chat (#137). // Adjust the derived thread state during render when the active chat genuinely // changes — the React-sanctioned alternative to an effect (it re-renders before diff --git a/apps/client/src/features/ai-chat/components/chat-thread.tsx b/apps/client/src/features/ai-chat/components/chat-thread.tsx index 9ee92377..29fad43c 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.tsx @@ -58,8 +58,12 @@ interface ChatThreadProps { * forwarded to MessageList. Absent => the generic "AI agent". */ assistantName?: string; /** Called when a turn finishes; the parent refreshes the chat list and, for - * a new chat, adopts the freshly created chat id. */ - onTurnFinished: () => void; + * a new chat, adopts the freshly created chat id. `serverChatId` is the + * authoritative id the server attached to the streamed assistant message + * metadata (see the server's `messageMetadata`); the parent adopts THIS for a + * new chat instead of guessing the newest chat in the list (fixes the two-tab + * adoption race, #137). Undefined on a failed turn that produced no metadata. */ + onTurnFinished: (serverChatId?: string) => void; /** Parent-owned ref that this thread keeps updated with its live useChat * snapshot (full message list + streaming flag), so the header's * "Copy chat" export can include the in-progress, not-yet-persisted @@ -246,8 +250,14 @@ export default function ChatThread({ // sending after the user hit Stop — or blindly retrying after a failure — // would be wrong, so on Stop/disconnect/error the queue is left intact for // the user to decide. - onFinish: ({ isAbort, isDisconnect, isError }) => { - onTurnFinished(); + onFinish: ({ message, isAbort, isDisconnect, isError }) => { + // The server attaches the authoritative chatId to the streamed assistant + // message metadata (see `messageMetadata` in ai-chat.service.ts). Forward it + // so the parent adopts the REAL created chat id for a new chat, rather than + // guessing the newest chat in the list (which races a second tab — #137). + const serverChatId = (message?.metadata as { chatId?: string } | undefined) + ?.chatId; + onTurnFinished(serverChatId); // Show a neutral "stopped" marker for an aborted turn; the red error banner // (via `error`) already covers isError, and a clean finish clears any marker. if (isError) setStopNotice(null); diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index d44a9abd..6e3751e9 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -504,6 +504,16 @@ export class AiChatService { // does not buffer responses by default. result.pipeUIMessageStreamToResponse(res.raw, { headers: { 'X-Accel-Buffering': 'no' }, + // Surface the authoritative chatId on the streamed assistant UI message so + // the client adopts the REAL id of the row we created, instead of guessing + // the newest chat in its list. `messageMetadata` is invoked by the AI SDK + // on the `start` and `finish` stream parts (ai@6); we attach `chatId` on the + // `start` part so it reaches the client (as message.metadata.chatId) at the + // very first chunk — before any second tab can race a newer chat into the + // list. This fixes the two-tab "adoption race" (#137) where a new chat in + // tab A could adopt tab B's id and leak its turns into the wrong row. + messageMetadata: ({ part }) => + part.type === 'start' ? { chatId } : undefined, onError: (error: unknown) => { // Reuse the shared formatter so provider error formatting stays // unified between the log line and the streamed error message. -- 2.49.1 From 0edc5aeda8c9a53b8ecb6034e6ff65e514ee4ee2 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Mon, 22 Jun 2026 23:46:50 +0300 Subject: [PATCH 2/7] docs(agents): clarify where DB migrations auto-apply (prod) vs not (dev) 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 --- AGENTS.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/AGENTS.md b/AGENTS.md index 333445a9..48a282ad 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -210,7 +210,7 @@ pnpm --filter @docmost/mcp test # node --test (unit + mock) pnpm --filter @docmost/mcp test:e2e # MCP end-to-end against a live instance ``` -**Database migrations** (Kysely, run from `apps/server`; they auto-run on server startup too): +**Database migrations** (Kysely, run from `apps/server`). **Where they auto-apply:** in **production** (the built image / `start:prod`) pending migrations run automatically on server boot. In **local dev** (the `pnpm dev` stand / `nest start --watch`) they do **NOT** auto-run — after you pull or switch branches you must apply them yourself with `pnpm --filter server migration:latest`, or any endpoint touching a new column/table 500s (e.g. a freshly-added `ai_chats.page_id` blanket-500s all of AI chat until migrated). ```bash pnpm --filter server migration:create --name=my_change # new empty migration pnpm --filter server migration:latest # apply all pending -- 2.49.1 From 580f3442b818c83eae46922c2ec4346b3de33a9c Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 23 Jun 2026 01:17:30 +0300 Subject: [PATCH 3/7] fix(ai-chat): prevent duplicate chat row on first-turn error; add adoption tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the PR #138 review. Blocker 1 — duplicate chat row: a brand-new chat whose first turn errors BEFORE the SSE 'start' chunk never receives the authoritative chatId, so metadata adoption can't run; a retry then sent chatId:null and the server inserted a SECOND chat row, orphaning the first turn. Keep metadata adoption as the primary path (resolveAdoptedChatId) and add a bounded, unambiguous fallback: on a new-chat finish with no server id, snapshot the known chat ids and, once the list refetch lands, adopt the SINGLE newly-appeared id (pickNewlyCreatedChatId). Zero or >1 new ids (e.g. two tabs racing) → no adoption — no items[0] guessing, so #137 stays fixed. The wait-for-refetch guard compares set membership (robust to a concurrent delete), and the diff dedupes so a repeated id from a paginated list never reads as ambiguous. Blocker 2 — tests: new adopt-chat-id.test.ts covers both pure helpers (adopt decision + newly-created-id diff incl. dedupe/reorder); the server messageMetadata callback is extracted to chatStreamStartMetadata and unit-tested (start -> {chatId}, otherwise undefined). Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/components/ai-chat-window.tsx | 91 +++++++++++++++---- .../ai-chat/components/chat-thread.tsx | 8 +- .../ai-chat/utils/adopt-chat-id.test.ts | 43 +++++++++ .../features/ai-chat/utils/adopt-chat-id.ts | 40 ++++++++ .../src/core/ai-chat/ai-chat.service.spec.ts | 18 ++++ .../src/core/ai-chat/ai-chat.service.ts | 18 +++- 6 files changed, 196 insertions(+), 22 deletions(-) create mode 100644 apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts create mode 100644 apps/client/src/features/ai-chat/utils/adopt-chat-id.ts diff --git a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx index 52732e79..7a8ea4b7 100644 --- a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx +++ b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx @@ -42,6 +42,10 @@ import { import ConversationList from "@/features/ai-chat/components/conversation-list.tsx"; import ChatThread from "@/features/ai-chat/components/chat-thread.tsx"; import { buildChatMarkdown } from "@/features/ai-chat/utils/chat-markdown.ts"; +import { + resolveAdoptedChatId, + pickNewlyCreatedChatId, +} from "@/features/ai-chat/utils/adopt-chat-id.ts"; import { shouldCollapseOnOutsidePointer, isHeaderClick, @@ -138,6 +142,15 @@ export default function AiChatWindow() { // messages invalidation) never tears the live thread back down to the loader. const historyLoadedKeyRef = useRef(null); + // Error-path fallback for new-chat id adoption. When a brand-new chat's first + // turn errors BEFORE the server's `start` chunk, no authoritative chatId ever + // reaches the client, so the primary metadata adoption cannot run. We then + // ARM this ref with a snapshot of the currently-known chat ids; once the list + // refetch lands with the just-created row, the fallback effect below adopts + // the SINGLE newly-appeared id (unambiguous — unlike the old items[0] guess). + // `null` = not armed. + const pendingNewChatRef = useRef(null); + // Mount key for ChatThread + the chat the currently-mounted thread represents. // `threadKey` normally tracks the active chat, so selecting a different chat // (incl. from page history) remounts and re-seeds. The ONE exception is @@ -199,6 +212,8 @@ export default function AiChatWindow() { setDraft(""); // Default the picker back to "Universal assistant" for the fresh chat. setSelectedRoleId(null); + // The user moved on — disarm any pending error-path adoption fallback. + pendingNewChatRef.current = null; }, [setActiveChatId, setDraft, setSelectedRoleId]); const selectChat = useCallback( @@ -209,6 +224,8 @@ export default function AiChatWindow() { // Reset the card-picked role so a stale pick can't leak into the existing // chat's header/assistant-name (which prefers the chat's persisted role). setSelectedRoleId(null); + // The user moved on — disarm any pending error-path adoption fallback. + pendingNewChatRef.current = null; }, [setActiveChatId, setDraft, setSelectedRoleId], ); @@ -222,17 +239,28 @@ export default function AiChatWindow() { // it (the two-tab adoption race, #137). const onTurnFinished = useCallback( (serverChatId?: string) => { - if (activeChatId === null && serverChatId) { - // In-place adoption: move the active chat AND the live-thread marker to - // the real id together, so the render-phase switch check below sees no - // "switch" and keeps the SAME mounted thread (its useChat already holds - // the just-finished turn, and its chatIdRef now mirrors the real id so - // the SECOND turn sends the correct chatId) instead of remounting and - // re-seeding from not-yet-persisted history. React's automatic batching - // inside this callback lands both updates in one render, so the guard - // never observes the new activeChatId with a stale liveThreadChatId. - setLiveThreadChatId(serverChatId); - setActiveChatId(serverChatId); + const adopted = resolveAdoptedChatId(activeChatId, serverChatId); + if (adopted) { + // PRIMARY path. In-place adoption: move the active chat AND the + // live-thread marker to the real id together, so the render-phase switch + // check below sees no "switch" and keeps the SAME mounted thread (its + // useChat already holds the just-finished turn, and its chatIdRef now + // mirrors the real id so the SECOND turn sends the correct chatId) + // instead of remounting and re-seeding from not-yet-persisted history. + // React's automatic batching inside this callback lands both updates in + // one render, so the guard never observes the new activeChatId with a + // stale liveThreadChatId. + setLiveThreadChatId(adopted); + setActiveChatId(adopted); + // Primary adoption won — disarm any previously-armed fallback. + pendingNewChatRef.current = null; + } else if (activeChatId === null) { + // FALLBACK path: a brand-new chat finished with NO server id (the first + // turn errored before the `start` chunk). Arm the bounded list-refetch + // fallback by snapshotting the currently-known chat ids. `chats` is still + // the pre-refetch list here, so the just-created row is NOT yet in it; the + // effect below adopts the single id that newly appears after the refetch. + pendingNewChatRef.current = chats?.items?.map((c) => c.id) ?? []; } queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }); // Re-sync the persisted message rows for the active chat so the Markdown @@ -248,9 +276,34 @@ export default function AiChatWindow() { }); } }, - [activeChatId, queryClient, setActiveChatId], + [activeChatId, chats, queryClient, setActiveChatId], ); + // FALLBACK resolver. Armed only by onTurnFinished when a brand-new chat's + // first turn errored before the `start` chunk (no authoritative id streamed). + // Once the per-user list refetch lands with the just-created row, adopt the + // SINGLE id that newly appeared relative to the pre-refetch snapshot. Adoption + // is IN PLACE (move liveThreadChatId + activeChatId together) exactly like the + // primary path, so the render-phase switch guard does not remount the thread. + useEffect(() => { + const before = pendingNewChatRef.current; + if (before === null || activeChatId !== null) return; // not armed / already adopted + const after = chats?.items?.map((c) => c.id) ?? []; + // Keep waiting until the list membership actually CHANGES (the refetch + // landed). A length compare would miss the change if another session + // concurrently deleted a chat in this same window (length stays equal while + // a new row was added); a set-membership compare is robust to that. + const beforeSet = new Set(before); + if (after.length === before.length && after.every((id) => beforeSet.has(id))) + return; // list not refetched yet — keep waiting + const adopted = pickNewlyCreatedChatId(before, after); // single new id, or null if ambiguous + pendingNewChatRef.current = null; // give up after one resolved refetch either way + if (adopted) { + setLiveThreadChatId(adopted); + setActiveChatId(adopted); + } + }, [chats, activeChatId, setActiveChatId]); + // The active chat object (for its title) and an export gate: only enable the // export button when an existing chat with loaded persisted rows is active. const activeChat = useMemo( @@ -303,11 +356,15 @@ export default function AiChatWindow() { notifications.show({ message: t("Copied") }); }, [activeChatId, messageRows, activeChat, clipboard, t]); - // NOTE: new-chat id adoption is no longer a heuristic effect. The server now - // reports the authoritative created chat id on the streamed assistant message - // metadata, and `onTurnFinished(serverChatId)` adopts THAT id in place — see - // above. The old "newest chat in the list" guess was removed because it raced - // a second tab's simultaneous new chat (#137). + // NOTE: new-chat id adoption has two paths, neither of which is the old + // "newest chat in the list" heuristic that raced a second tab (#137). + // PRIMARY: the server reports the authoritative created chat id on the streamed + // assistant message metadata, and `onTurnFinished(serverChatId)` adopts THAT id + // in place — see above. FALLBACK (only when the first turn errors before the + // `start` chunk, so no metadata id is streamed): adopt the SINGLE chat that + // newly appeared in the refetched per-user list relative to a pre-refetch + // snapshot taken when the failed turn finished — unambiguous, and it does not + // race a sibling chat the way `items[0]` did (see the fallback effect above). // Adjust the derived thread state during render when the active chat genuinely // changes — the React-sanctioned alternative to an effect (it re-renders before diff --git a/apps/client/src/features/ai-chat/components/chat-thread.tsx b/apps/client/src/features/ai-chat/components/chat-thread.tsx index 29fad43c..b295faec 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.tsx @@ -269,9 +269,11 @@ export default function ChatThread({ }, // `onError` runs in addition to `onFinish` (which ai@6 also calls on error). // Log the raw failure here for devtools; the UI shows a friendly classified - // banner via `error` below. We still call `onTurnFinished()` (idempotent with - // the onFinish call) so a brand-new chat that fails its first turn is adopted - // and the chat list refreshes immediately rather than after a manual refresh. + // banner via `error` below. We still call `onTurnFinished()` with NO server id + // (idempotent with the onFinish call): for a brand-new chat that ARMS the + // bounded list-refetch fallback (adopt the single newly-appeared chat once the + // refetch lands); for an existing chat it just refreshes the chat list + // immediately rather than after a manual refresh. onError: (streamError) => { // Surface the raw failure in the browser console (devtools) for debugging; // the UI separately shows a friendly classified banner (see errorView). diff --git a/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts b/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts new file mode 100644 index 00000000..d5ba12ec --- /dev/null +++ b/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts @@ -0,0 +1,43 @@ +import { describe, it, expect } from "vitest"; +import { resolveAdoptedChatId, pickNewlyCreatedChatId } from "./adopt-chat-id"; + +describe("resolveAdoptedChatId", () => { + it("adopts the server id for a brand-new chat (activeChatId null + id)", () => { + expect(resolveAdoptedChatId(null, "chat-1")).toBe("chat-1"); + }); + + it("returns null for an existing chat even with a server id", () => { + expect(resolveAdoptedChatId("chat-existing", "chat-1")).toBeNull(); + }); + + it("returns null for a new chat with no server id", () => { + expect(resolveAdoptedChatId(null, undefined)).toBeNull(); + expect(resolveAdoptedChatId(null, null)).toBeNull(); + }); +}); + +describe("pickNewlyCreatedChatId", () => { + it("returns the single newly-appeared id", () => { + expect(pickNewlyCreatedChatId(["a", "b"], ["c", "a", "b"])).toBe("c"); + }); + + it("returns null when no new id appeared", () => { + expect(pickNewlyCreatedChatId(["a", "b"], ["a", "b"])).toBeNull(); + }); + + it("returns null when more than one new id appeared (ambiguous)", () => { + expect(pickNewlyCreatedChatId(["a"], ["a", "b", "c"])).toBeNull(); + }); + + it("returns the single after id when before is empty", () => { + expect(pickNewlyCreatedChatId([], ["only"])).toBe("only"); + }); + + it("treats a duplicated new id as one (deduped, not ambiguous)", () => { + expect(pickNewlyCreatedChatId(["a"], ["a", "new", "new"])).toBe("new"); + }); + + it("returns null when membership is unchanged but reordered", () => { + expect(pickNewlyCreatedChatId(["a", "b"], ["b", "a"])).toBeNull(); + }); +}); diff --git a/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts b/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts new file mode 100644 index 00000000..16bebc04 --- /dev/null +++ b/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts @@ -0,0 +1,40 @@ +/** + * Pure helpers for adopting a brand-new chat's authoritative server id. + * + * PRIMARY path: the server streams the real chat id on the assistant message + * metadata (see `chatStreamStartMetadata` server-side); `resolveAdoptedChatId` + * turns that into the id to adopt for a new chat. + * + * FALLBACK path (only when a new chat's first turn errors BEFORE the `start` + * chunk, so no metadata id ever reached the client): adopt the single chat that + * NEWLY appeared in the per-user list relative to a pre-refetch snapshot — + * `pickNewlyCreatedChatId`. This is unambiguous and does not race a second tab + * the way the old "newest chat in the list" guess did (#137). + */ + +/** + * Resolve the id to adopt from the server-streamed metadata. Returns + * `serverChatId` only for a brand-new chat (`activeChatId === null`) that + * received a truthy id; otherwise null (existing chat, or no id streamed). + */ +export function resolveAdoptedChatId( + activeChatId: string | null, + serverChatId: string | null | undefined, +): string | null { + return activeChatId === null && serverChatId ? serverChatId : null; +} + +/** + * Return the single id present in `afterIds` but not in `beforeIds`. Returns + * null when zero or more-than-one such id exists (ambiguous — do not adopt). + */ +export function pickNewlyCreatedChatId( + beforeIds: readonly string[], + afterIds: readonly string[], +): string | null { + const before = new Set(beforeIds); + // Dedupe the new ids: a paginated/flatMapped list can repeat the same id, and + // one genuinely-new chat must not read as "ambiguous" (>1) from a duplicate. + const added = new Set(afterIds.filter((id) => !before.has(id))); + return added.size === 1 ? [...added][0] : null; +} diff --git a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts index d6f333d0..8f6e48d5 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.spec.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.spec.ts @@ -5,6 +5,7 @@ import { rowToUiMessage, prepareAgentStep, buildPartialAssistantRecord, + chatStreamStartMetadata, MAX_AGENT_STEPS, FINAL_STEP_INSTRUCTION, } from './ai-chat.service'; @@ -295,3 +296,20 @@ describe('buildPartialAssistantRecord', () => { expect(rec.text).toBe('half'); }); }); + +/** + * chatStreamStartMetadata: attach the authoritative chatId to the streamed + * assistant UI message ONLY on the `start` part (so the client adopts the real + * created chat id at the first chunk — see #137). Any non-start part adds none. + */ +describe('chatStreamStartMetadata', () => { + it('returns { chatId } for the start part', () => { + expect(chatStreamStartMetadata({ type: 'start' }, 'chat-1')).toEqual({ + chatId: 'chat-1', + }); + }); + + it('returns undefined for a finish part (any non-start part)', () => { + expect(chatStreamStartMetadata({ type: 'finish' }, 'chat-1')).toBeUndefined(); + }); +}); diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index 6e3751e9..b8d925e4 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -512,8 +512,7 @@ export class AiChatService { // very first chunk — before any second tab can race a newer chat into the // list. This fixes the two-tab "adoption race" (#137) where a new chat in // tab A could adopt tab B's id and leak its turns into the wrong row. - messageMetadata: ({ part }) => - part.type === 'start' ? { chatId } : undefined, + messageMetadata: ({ part }) => chatStreamStartMetadata(part, chatId), onError: (error: unknown) => { // Reuse the shared formatter so provider error formatting stays // unified between the log line and the streamed error message. @@ -562,6 +561,21 @@ export class AiChatService { } } +/** + * Compute the `messageMetadata` payload for the streamed assistant UI message. + * The AI SDK invokes `messageMetadata` on each stream part (ai@6); we attach the + * authoritative `chatId` ONLY on the `start` part so it reaches the client at the + * very first chunk (as `message.metadata.chatId`). The client adopts THAT id for a + * new chat instead of guessing the newest chat in its list — fixing the two-tab + * adoption race (#137). Returning undefined for any non-start part adds no metadata. + */ +export function chatStreamStartMetadata( + part: { type: string }, + chatId: string, +): { chatId: string } | undefined { + return part.type === 'start' ? { chatId } : undefined; +} + /** The last message with role 'user' from a useChat payload, if any. */ function lastUserMessage( messages: UIMessage[] | undefined, -- 2.49.1 From ba901477497f158720f1aa6e74657c56400e569e Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 23 Jun 2026 01:46:15 +0300 Subject: [PATCH 4/7] refactor(ai-chat): consolidate thread id lifecycle into one atomic identity MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the PR #138 review's architecture note (the deferred 'non-blocking' item). The brand-new -> persisted chat identity was spread across two separate useState slots (threadKey + liveThreadChatId) plus a render-phase guard, so the mount key and the live thread's chat id could in principle diverge. Consolidate them into ONE atomic state object { key, chatId } with pure, unit-tested transitions in thread-identity.ts: - newThread(newKey) — a brand-new id-less chat (fresh session key); - switchThread(chatId) — switch to an existing chat (key := chat id) -> remount; - adoptThread(prev, id) — a new chat learns its real id IN PLACE (key unchanged -> no remount, live useChat store preserved). The 'key vs chatId diverged' state is now unrepresentable. The render-phase reconciliation stays (the atom is also set externally, e.g. page-history's history-item opening a referenced chat), but adoption vs switch is now explicit. Behavior is unchanged; verified live: new-chat adopt + 2nd turn in the same row, no mid-conversation remount, the two-tab race stays leak-free, switch-to-existing remounts + reseeds, and reload restores full history. Adds thread-identity.test.ts. Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/components/ai-chat-window.tsx | 88 +++++++++++-------- .../ai-chat/utils/thread-identity.test.ts | 53 +++++++++++ .../features/ai-chat/utils/thread-identity.ts | 47 ++++++++++ 3 files changed, 151 insertions(+), 37 deletions(-) create mode 100644 apps/client/src/features/ai-chat/utils/thread-identity.test.ts create mode 100644 apps/client/src/features/ai-chat/utils/thread-identity.ts diff --git a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx index 7a8ea4b7..a2418110 100644 --- a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx +++ b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx @@ -46,6 +46,12 @@ import { resolveAdoptedChatId, pickNewlyCreatedChatId, } from "@/features/ai-chat/utils/adopt-chat-id.ts"; +import { + type ThreadIdentity, + newThread, + switchThread, + adoptThread, +} from "@/features/ai-chat/utils/thread-identity.ts"; import { shouldCollapseOnOutsidePointer, isHeaderClick, @@ -151,19 +157,21 @@ export default function AiChatWindow() { // `null` = not armed. const pendingNewChatRef = useRef(null); - // Mount key for ChatThread + the chat the currently-mounted thread represents. - // `threadKey` normally tracks the active chat, so selecting a different chat - // (incl. from page history) remounts and re-seeds. The ONE exception is - // in-place adoption of a brand-new chat's server id: the adopt effect moves - // `liveThreadChatId` to the new id TOGETHER with `activeChatId`, so the switch - // check below does not fire and the SAME thread stays mounted (its useChat - // already holds the just-finished turn) instead of being re-seeded from - // not-yet-persisted history. - const [threadKey, setThreadKey] = useState( - () => activeChatId ?? `new-${generateId()}`, - ); - const [liveThreadChatId, setLiveThreadChatId] = useState( - activeChatId, + // The mounted thread's identity: ONE atomic value tying ChatThread's mount + // key (`thread.key`) to the chat id that mounted thread holds (`thread.chatId`). + // Consolidating these makes the "key vs chat id diverged" state unrepresentable + // — every change goes through an explicit transition (see thread-identity.ts): + // • switchThread — a genuine switch (key := chat id) → remount + reseed; + // • adoptThread — a brand-new chat learns its real id (key UNCHANGED, chat + // id null→realId) → NO remount, so the live useChat store (holding the + // just-finished turn) is preserved and the 2nd turn sends the real chatId. + // A non-null initial activeChatId yields {key: id, chatId: id}; the `""` newKey + // is unused on that (non-null) path. A null initial chat gets a fresh session + // key with no chat id yet. + const [thread, setThread] = useState(() => + activeChatId === null + ? newThread(`new-${generateId()}`) + : switchThread(activeChatId), ); const { data: chats } = useAiChatsQuery(); @@ -241,17 +249,15 @@ export default function AiChatWindow() { (serverChatId?: string) => { const adopted = resolveAdoptedChatId(activeChatId, serverChatId); if (adopted) { - // PRIMARY path. In-place adoption: move the active chat AND the - // live-thread marker to the real id together, so the render-phase switch - // check below sees no "switch" and keeps the SAME mounted thread (its - // useChat already holds the just-finished turn, and its chatIdRef now - // mirrors the real id so the SECOND turn sends the correct chatId) + // PRIMARY path. In-place adoption: set the public selection and the + // thread identity to the real id together. `adoptThread` keeps the SAME + // mount key (chat id null→realId), so the render-phase reconciler below + // sees `activeChatId === thread.chatId` and keeps the SAME mounted thread + // (its useChat already holds the just-finished turn, and its chatIdRef + // now mirrors the real id so the SECOND turn sends the correct chatId) // instead of remounting and re-seeding from not-yet-persisted history. - // React's automatic batching inside this callback lands both updates in - // one render, so the guard never observes the new activeChatId with a - // stale liveThreadChatId. - setLiveThreadChatId(adopted); setActiveChatId(adopted); + setThread((prev) => adoptThread(prev, adopted)); // Primary adoption won — disarm any previously-armed fallback. pendingNewChatRef.current = null; } else if (activeChatId === null) { @@ -265,7 +271,7 @@ export default function AiChatWindow() { queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }); // Re-sync the persisted message rows for the active chat so the Markdown // export and the token counters reflect the turn that just finished. The - // live thread renders from its own useChat store (stable threadKey / store + // live thread renders from its own useChat store (stable thread.key / store // id), so refetching these rows never re-seeds or tears down the open // thread. For a brand-new chat activeChatId is still null here; that chat's // first row load happens right after id adoption, and every later turn hits @@ -283,8 +289,8 @@ export default function AiChatWindow() { // first turn errored before the `start` chunk (no authoritative id streamed). // Once the per-user list refetch lands with the just-created row, adopt the // SINGLE id that newly appeared relative to the pre-refetch snapshot. Adoption - // is IN PLACE (move liveThreadChatId + activeChatId together) exactly like the - // primary path, so the render-phase switch guard does not remount the thread. + // is IN PLACE (set activeChatId + adoptThread the identity together) exactly + // like the primary path, so the render-phase reconciler does not remount. useEffect(() => { const before = pendingNewChatRef.current; if (before === null || activeChatId !== null) return; // not armed / already adopted @@ -299,8 +305,8 @@ export default function AiChatWindow() { const adopted = pickNewlyCreatedChatId(before, after); // single new id, or null if ambiguous pendingNewChatRef.current = null; // give up after one resolved refetch either way if (adopted) { - setLiveThreadChatId(adopted); setActiveChatId(adopted); + setThread((prev) => adoptThread(prev, adopted)); } }, [chats, activeChatId, setActiveChatId]); @@ -366,14 +372,22 @@ export default function AiChatWindow() { // snapshot taken when the failed turn finished — unambiguous, and it does not // race a sibling chat the way `items[0]` did (see the fallback effect above). - // Adjust the derived thread state during render when the active chat genuinely - // changes — the React-sanctioned alternative to an effect (it re-renders before - // paint, no extra commit, and converges since the next render finds them equal). - // In-place adoption of a new chat's id never reaches here because the adopt - // effect moves liveThreadChatId in lockstep with activeChatId. - if (activeChatId !== liveThreadChatId) { - setLiveThreadChatId(activeChatId); - setThreadKey(activeChatId ?? `new-${generateId()}`); + // Reconcile the thread identity against the active-chat atom during render when + // they diverge — the React-sanctioned alternative to an effect (it re-renders + // before paint, no extra commit, and converges since the next render finds them + // equal). This reconciliation MUST remain: `activeChatId` is the public + // selection and is ALSO set from OUTSIDE this component (e.g. page-history's + // history-item calls setActiveChatId to open a referenced chat). A divergence + // here is a genuine SWITCH (external atom change OR user switch via + // selectChat/startNewChat), so `switchThread` remounts + reseeds. In-place + // adoption never reaches this branch: it set activeChatId and the thread's + // chatId to the same value, so `activeChatId === thread.chatId` here. + if (activeChatId !== thread.chatId) { + setThread( + activeChatId === null + ? newThread(`new-${generateId()}`) + : switchThread(activeChatId), + ); } // Latch the active chat once its full history has loaded and its thread is // mounted, so a later background refetch (the post-turn messages @@ -382,7 +396,7 @@ export default function AiChatWindow() { // the live thread down to a loader and lose its in-progress useChat state. if ( activeChatId !== null && - threadKey === activeChatId && + thread.key === activeChatId && !messagesLoading && historyLoadedKeyRef.current !== activeChatId ) { @@ -398,7 +412,7 @@ export default function AiChatWindow() { const waitingForHistory = activeChatId !== null && messagesLoading && - threadKey === activeChatId && + thread.key === activeChatId && historyLoadedKeyRef.current !== activeChatId; // Current context size for the active chat: how much the conversation now @@ -725,7 +739,7 @@ export default function AiChatWindow() { ) : ( { + it("uses the supplied key and has no chat id yet", () => { + expect(newThread("new-abc")).toEqual({ key: "new-abc", chatId: null }); + }); +}); + +describe("switchThread", () => { + it("switches to an existing chat: key becomes the chat id", () => { + expect(switchThread("chat-1")).toEqual({ + key: "chat-1", + chatId: "chat-1", + }); + }); +}); + +describe("adoptThread", () => { + it("adopts in place for a new chat: keeps the key, sets the chat id", () => { + const prev = newThread("new-abc"); + expect(adoptThread(prev, "chat-1")).toEqual({ + key: "new-abc", + chatId: "chat-1", + }); + }); + + it("is a no-op for an already-persisted chat", () => { + const prev: { key: string; chatId: string | null } = { + key: "chat-1", + chatId: "chat-1", + }; + expect(adoptThread(prev, "chat-2")).toBe(prev); + }); + + it("INVARIANT: adoption never remounts (key unchanged) while chatId changes", () => { + const prev = newThread("new-abc"); + const next = adoptThread(prev, "chat-1"); + // The mount key is preserved (no remount) ... + expect(next.key).toBe(prev.key); + // ... while the chat id moved from null to the real id. + expect(prev.chatId).toBeNull(); + expect(next.chatId).toBe("chat-1"); + }); + + it("INVARIANT: after adoption thread.chatId equals the adopted id, so the window's render-phase reconciler (activeChatId !== thread.chatId) does NOT fire and remount the live thread", () => { + const adoptedId = "chat-1"; + const next = adoptThread(newThread("new-abc"), adoptedId); + // The window sets activeChatId to the same adoptedId; this asserts they match + // so the reconciler treats it as already-in-sync, not a switch. + expect(next.chatId).toBe(adoptedId); + }); +}); diff --git a/apps/client/src/features/ai-chat/utils/thread-identity.ts b/apps/client/src/features/ai-chat/utils/thread-identity.ts new file mode 100644 index 00000000..4b289615 --- /dev/null +++ b/apps/client/src/features/ai-chat/utils/thread-identity.ts @@ -0,0 +1,47 @@ +/** + * Pure transitions for the AI-chat thread's identity: the single source of + * truth tying ChatThread's mount key to the chat id that mounted thread holds. + * + * The window keeps exactly ONE of these in state. Consolidating the mount key + * and the live thread's chat id into one atomic value makes the "stale chat id + * vs key" state unrepresentable: every change goes through one of the explicit + * transitions below, so the key and chatId can never silently diverge. + * + * - `newThread`/`switchThread` produce a key that forces a remount (+ reseed): + * `newThread` for a brand-new (id-less) chat, `switchThread` for an existing + * one. The caller picks which based on whether there is a chat id. + * - `adoptThread` keeps the SAME key so a brand-new chat learns its real id + * WITHOUT remounting (the live useChat store, holding the just-finished turn, + * is preserved and the next turn sends the real chatId). + * + * `newThread` takes the session key from the impure `generateId()` at the call + * site so these stay pure and unit-testable. + */ +export type ThreadIdentity = { key: string; chatId: string | null }; + +/** + * A brand-new chat: a fresh session key and no chat id yet. `newKey` is + * supplied by the caller (generateId() is impure) so this stays pure/testable. + */ +export function newThread(newKey: string): ThreadIdentity { + return { key: newKey, chatId: null }; +} + +/** + * Switch to an EXISTING chat: the mount key becomes the chat id, forcing a + * remount + reseed from the persisted history. (A switch to a brand-new chat + * goes through `newThread` instead — there is no id to key on.) + */ +export function switchThread(chatId: string): ThreadIdentity { + return { key: chatId, chatId }; +} + +/** + * In-place adoption: a brand-new chat (`prev.chatId === null`) learns its real + * id WITHOUT remounting — keep the SAME key, set the chat id. If `prev` already + * has a chatId (not a new chat), this is a no-op (returns `prev`): adoption only + * applies to an as-yet-unadopted new thread. + */ +export function adoptThread(prev: ThreadIdentity, chatId: string): ThreadIdentity { + return prev.chatId === null ? { key: prev.key, chatId } : prev; +} -- 2.49.1 From f59ca3cb0d548799b53bbc35c5d072b73b462307 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 23 Jun 2026 02:25:52 +0300 Subject: [PATCH 5/7] refactor(ai-chat): extract useChatSession hook + lock the id lifecycle with tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the 2nd PR #138 review (test debt + the Variant-B architecture ask). The new→persisted chat id lifecycle (mount key, both adoption paths, the history-load latch, the render-phase reconciler, onTurnFinished) is moved out of the 768-line window into a new useChatSession hook driven by a pure threadSessionReducer (reconcile/adopt), so adopt-vs-switch is one explicit dispatch point and the scattering the review flagged is gone (window: 768→~620). Tests (the blockers): - use-chat-session.test.tsx — hook-level locks incl. the #137 regression (adopts the authoritative streamed id 'A', NOT chats.items[0]='B' — fails on the old heuristic), the error-path fallback (arm/adopt/ambiguous/add+delete), the disarm-on-reconcile lock (a fallback armed then switched away must not be adopted by a late refetch), in-place-adopt-keeps-key vs external-switch-remount, and the waitingForHistory latch. - extractServerChatId (reading message.metadata.chatId) and newlyAddedChatIds extracted as pure helpers with unit tests; threadSessionReducer tested. Cleanups: single canonical #137 explanation in adopt-chat-id.ts (other sites reference it); fallback effect computes the set diff once; invalidate callbacks memoized; redundant invariant tests folded. Behavior preserved — re-verified live (z.ai glm-5.2): new-chat adopt + 2nd turn in the same row, no mid-conversation remount, two-tab race leak-free, switch to an existing chat reseeds full history, reload restores history. Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/components/ai-chat-window.tsx | 196 +++--------------- .../ai-chat/components/chat-thread.tsx | 22 +- .../ai-chat/hooks/use-chat-session.test.tsx | 149 +++++++++++++ .../ai-chat/hooks/use-chat-session.ts | 196 ++++++++++++++++++ .../ai-chat/utils/adopt-chat-id.test.ts | 58 +++++- .../features/ai-chat/utils/adopt-chat-id.ts | 57 ++++- .../ai-chat/utils/thread-identity.test.ts | 56 +++-- .../features/ai-chat/utils/thread-identity.ts | 26 +++ .../src/core/ai-chat/ai-chat.service.ts | 9 +- 9 files changed, 556 insertions(+), 213 deletions(-) create mode 100644 apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx create mode 100644 apps/client/src/features/ai-chat/hooks/use-chat-session.ts diff --git a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx index a2418110..83200747 100644 --- a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx +++ b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx @@ -6,7 +6,6 @@ import { useRef, useState, } from "react"; -import { generateId } from "ai"; import { type UIMessage } from "@ai-sdk/react"; import { Group, Loader, Tooltip } from "@mantine/core"; import { @@ -42,16 +41,7 @@ import { import ConversationList from "@/features/ai-chat/components/conversation-list.tsx"; import ChatThread from "@/features/ai-chat/components/chat-thread.tsx"; import { buildChatMarkdown } from "@/features/ai-chat/utils/chat-markdown.ts"; -import { - resolveAdoptedChatId, - pickNewlyCreatedChatId, -} from "@/features/ai-chat/utils/adopt-chat-id.ts"; -import { - type ThreadIdentity, - newThread, - switchThread, - adoptThread, -} from "@/features/ai-chat/utils/thread-identity.ts"; +import { useChatSession } from "@/features/ai-chat/hooks/use-chat-session.ts"; import { shouldCollapseOnOutsidePointer, isHeaderClick, @@ -143,37 +133,6 @@ export default function AiChatWindow() { // left partly off-screen). const [geom, setGeom] = useAtom(aiChatWindowGeomAtom); - // Latch: the chat id whose full persisted history has finished loading while - // its thread is mounted. Used so a later BACKGROUND refetch (the post-turn - // messages invalidation) never tears the live thread back down to the loader. - const historyLoadedKeyRef = useRef(null); - - // Error-path fallback for new-chat id adoption. When a brand-new chat's first - // turn errors BEFORE the server's `start` chunk, no authoritative chatId ever - // reaches the client, so the primary metadata adoption cannot run. We then - // ARM this ref with a snapshot of the currently-known chat ids; once the list - // refetch lands with the just-created row, the fallback effect below adopts - // the SINGLE newly-appeared id (unambiguous — unlike the old items[0] guess). - // `null` = not armed. - const pendingNewChatRef = useRef(null); - - // The mounted thread's identity: ONE atomic value tying ChatThread's mount - // key (`thread.key`) to the chat id that mounted thread holds (`thread.chatId`). - // Consolidating these makes the "key vs chat id diverged" state unrepresentable - // — every change goes through an explicit transition (see thread-identity.ts): - // • switchThread — a genuine switch (key := chat id) → remount + reseed; - // • adoptThread — a brand-new chat learns its real id (key UNCHANGED, chat - // id null→realId) → NO remount, so the live useChat store (holding the - // just-finished turn) is preserved and the 2nd turn sends the real chatId. - // A non-null initial activeChatId yields {key: id, chatId: id}; the `""` newKey - // is unused on that (non-null) path. A null initial chat gets a fresh session - // key with no chat id yet. - const [thread, setThread] = useState(() => - activeChatId === null - ? newThread(`new-${generateId()}`) - : switchThread(activeChatId), - ); - const { data: chats } = useAiChatsQuery(); // Roles for the new-chat picker (any member may list them). Only fetched while // the window is open. @@ -214,14 +173,14 @@ export default function AiChatWindow() { ? { id: openPageData.id, title: openPageData.title } : null; + // startNewChat/selectChat only set the public atom; useChatSession's render- + // phase reconciler does the remount AND disarms any pending new-chat fallback. const startNewChat = useCallback((): void => { setActiveChatId(null); setHistoryOpen(false); setDraft(""); // Default the picker back to "Universal assistant" for the fresh chat. setSelectedRoleId(null); - // The user moved on — disarm any pending error-path adoption fallback. - pendingNewChatRef.current = null; }, [setActiveChatId, setDraft, setSelectedRoleId]); const selectChat = useCallback( @@ -232,83 +191,33 @@ export default function AiChatWindow() { // Reset the card-picked role so a stale pick can't leak into the existing // chat's header/assistant-name (which prefers the chat's persisted role). setSelectedRoleId(null); - // The user moved on — disarm any pending error-path adoption fallback. - pendingNewChatRef.current = null; }, [setActiveChatId, setDraft, setSelectedRoleId], ); - // After a turn finishes, refresh the chat list. For a brand-new chat (no id - // yet), the server has just created the row and reported its AUTHORITATIVE id - // via the streamed assistant message metadata (`serverChatId`); we adopt THAT - // exact id so the thread switches from "new" to its OWN persisted chat — never - // guessing the newest chat in the list, which would adopt a SIBLING chat that - // a second tab created in the same moment and leak this tab's later turns into - // it (the two-tab adoption race, #137). - const onTurnFinished = useCallback( - (serverChatId?: string) => { - const adopted = resolveAdoptedChatId(activeChatId, serverChatId); - if (adopted) { - // PRIMARY path. In-place adoption: set the public selection and the - // thread identity to the real id together. `adoptThread` keeps the SAME - // mount key (chat id null→realId), so the render-phase reconciler below - // sees `activeChatId === thread.chatId` and keeps the SAME mounted thread - // (its useChat already holds the just-finished turn, and its chatIdRef - // now mirrors the real id so the SECOND turn sends the correct chatId) - // instead of remounting and re-seeding from not-yet-persisted history. - setActiveChatId(adopted); - setThread((prev) => adoptThread(prev, adopted)); - // Primary adoption won — disarm any previously-armed fallback. - pendingNewChatRef.current = null; - } else if (activeChatId === null) { - // FALLBACK path: a brand-new chat finished with NO server id (the first - // turn errored before the `start` chunk). Arm the bounded list-refetch - // fallback by snapshotting the currently-known chat ids. `chats` is still - // the pre-refetch list here, so the just-created row is NOT yet in it; the - // effect below adopts the single id that newly appears after the refetch. - pendingNewChatRef.current = chats?.items?.map((c) => c.id) ?? []; - } - queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }); - // Re-sync the persisted message rows for the active chat so the Markdown - // export and the token counters reflect the turn that just finished. The - // live thread renders from its own useChat store (stable thread.key / store - // id), so refetching these rows never re-seeds or tears down the open - // thread. For a brand-new chat activeChatId is still null here; that chat's - // first row load happens right after id adoption, and every later turn hits - // this invalidation with the adopted id. - if (activeChatId) { - queryClient.invalidateQueries({ - queryKey: AI_CHAT_MESSAGES_RQ_KEY(activeChatId), - }); - } - }, - [activeChatId, chats, queryClient, setActiveChatId], + // The AI-chat thread-identity lifecycle (mount key, both new-chat id adoption + // paths, the history-loaded latch, the render-phase reconciler) lives in this + // hook. See adopt-chat-id.ts for the canonical #137 two-tab race explanation. + // Memoized so the hook's `onTurnFinished` useCallback keeps a stable identity + // across renders (it is read live by useChat's onFinish, so this is cosmetic, + // not load-bearing — but avoids needless re-creation). + const invalidateChatList = useCallback( + () => queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }), + [queryClient], ); - - // FALLBACK resolver. Armed only by onTurnFinished when a brand-new chat's - // first turn errored before the `start` chunk (no authoritative id streamed). - // Once the per-user list refetch lands with the just-created row, adopt the - // SINGLE id that newly appeared relative to the pre-refetch snapshot. Adoption - // is IN PLACE (set activeChatId + adoptThread the identity together) exactly - // like the primary path, so the render-phase reconciler does not remount. - useEffect(() => { - const before = pendingNewChatRef.current; - if (before === null || activeChatId !== null) return; // not armed / already adopted - const after = chats?.items?.map((c) => c.id) ?? []; - // Keep waiting until the list membership actually CHANGES (the refetch - // landed). A length compare would miss the change if another session - // concurrently deleted a chat in this same window (length stays equal while - // a new row was added); a set-membership compare is robust to that. - const beforeSet = new Set(before); - if (after.length === before.length && after.every((id) => beforeSet.has(id))) - return; // list not refetched yet — keep waiting - const adopted = pickNewlyCreatedChatId(before, after); // single new id, or null if ambiguous - pendingNewChatRef.current = null; // give up after one resolved refetch either way - if (adopted) { - setActiveChatId(adopted); - setThread((prev) => adoptThread(prev, adopted)); - } - }, [chats, activeChatId, setActiveChatId]); + const invalidateChatMessages = useCallback( + (id: string) => + queryClient.invalidateQueries({ queryKey: AI_CHAT_MESSAGES_RQ_KEY(id) }), + [queryClient], + ); + const { threadKey, waitingForHistory, onTurnFinished } = useChatSession({ + activeChatId, + setActiveChatId, + chats, + messagesLoading, + onInvalidateChatList: invalidateChatList, + onInvalidateChatMessages: invalidateChatMessages, + }); // The active chat object (for its title) and an export gate: only enable the // export button when an existing chat with loaded persisted rows is active. @@ -362,59 +271,6 @@ export default function AiChatWindow() { notifications.show({ message: t("Copied") }); }, [activeChatId, messageRows, activeChat, clipboard, t]); - // NOTE: new-chat id adoption has two paths, neither of which is the old - // "newest chat in the list" heuristic that raced a second tab (#137). - // PRIMARY: the server reports the authoritative created chat id on the streamed - // assistant message metadata, and `onTurnFinished(serverChatId)` adopts THAT id - // in place — see above. FALLBACK (only when the first turn errors before the - // `start` chunk, so no metadata id is streamed): adopt the SINGLE chat that - // newly appeared in the refetched per-user list relative to a pre-refetch - // snapshot taken when the failed turn finished — unambiguous, and it does not - // race a sibling chat the way `items[0]` did (see the fallback effect above). - - // Reconcile the thread identity against the active-chat atom during render when - // they diverge — the React-sanctioned alternative to an effect (it re-renders - // before paint, no extra commit, and converges since the next render finds them - // equal). This reconciliation MUST remain: `activeChatId` is the public - // selection and is ALSO set from OUTSIDE this component (e.g. page-history's - // history-item calls setActiveChatId to open a referenced chat). A divergence - // here is a genuine SWITCH (external atom change OR user switch via - // selectChat/startNewChat), so `switchThread` remounts + reseeds. In-place - // adoption never reaches this branch: it set activeChatId and the thread's - // chatId to the same value, so `activeChatId === thread.chatId` here. - if (activeChatId !== thread.chatId) { - setThread( - activeChatId === null - ? newThread(`new-${generateId()}`) - : switchThread(activeChatId), - ); - } - // Latch the active chat once its full history has loaded and its thread is - // mounted, so a later background refetch (the post-turn messages - // invalidation, which can transiently flip hasNextPage for a chat whose - // message count is an exact multiple of the server page size) does not tear - // the live thread down to a loader and lose its in-progress useChat state. - if ( - activeChatId !== null && - thread.key === activeChatId && - !messagesLoading && - historyLoadedKeyRef.current !== activeChatId - ) { - historyLoadedKeyRef.current = activeChatId; - } - - // Show the history loader only when freshly OPENING an existing chat (the key - // equals the chat id) whose history has not been fully loaded yet. For a live - // in-place thread that adopted its id, the key is still the "new-…" session - // key, so we keep showing the live thread instead of unmounting it behind a - // loader; and once a chat's history has loaded, a later background refetch no - // longer tears the thread back down (see the latch above). - const waitingForHistory = - activeChatId !== null && - messagesLoading && - thread.key === activeChatId && - historyLoadedKeyRef.current !== activeChatId; - // Current context size for the active chat: how much the conversation now // occupies in the model's context window — NOT the cumulative tokens spent. // We read the most recent assistant row that carries a context figure: @@ -739,7 +595,7 @@ export default function AiChatWindow() { ) : ( the generic "AI agent". */ assistantName?: string; - /** Called when a turn finishes; the parent refreshes the chat list and, for - * a new chat, adopts the freshly created chat id. `serverChatId` is the - * authoritative id the server attached to the streamed assistant message - * metadata (see the server's `messageMetadata`); the parent adopts THIS for a - * new chat instead of guessing the newest chat in the list (fixes the two-tab - * adoption race, #137). Undefined on a failed turn that produced no metadata. */ + /** Called when a turn finishes; the parent refreshes the chat list and, for a + * new chat, adopts the freshly created chat id. `serverChatId` is the + * authoritative id the server streamed on the assistant message metadata, or + * undefined on a failed turn — see adopt-chat-id.ts for the full #137 design. */ onTurnFinished: (serverChatId?: string) => void; /** Parent-owned ref that this thread keeps updated with its live useChat * snapshot (full message list + streaming flag), so the header's @@ -251,13 +250,10 @@ export default function ChatThread({ // would be wrong, so on Stop/disconnect/error the queue is left intact for // the user to decide. onFinish: ({ message, isAbort, isDisconnect, isError }) => { - // The server attaches the authoritative chatId to the streamed assistant - // message metadata (see `messageMetadata` in ai-chat.service.ts). Forward it - // so the parent adopts the REAL created chat id for a new chat, rather than - // guessing the newest chat in the list (which races a second tab — #137). - const serverChatId = (message?.metadata as { chatId?: string } | undefined) - ?.chatId; - onTurnFinished(serverChatId); + // Forward the authoritative server chatId (streamed on the assistant + // message metadata) so the parent adopts the REAL created chat id for a new + // chat — see adopt-chat-id.ts for the full #137 design. + onTurnFinished(extractServerChatId(message)); // Show a neutral "stopped" marker for an aborted turn; the red error banner // (via `error`) already covers isError, and a clean finish clears any marker. if (isError) setStopNotice(null); diff --git a/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx b/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx new file mode 100644 index 00000000..e7b8695c --- /dev/null +++ b/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx @@ -0,0 +1,149 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { renderHook } from "@testing-library/react"; +import { useChatSession } from "./use-chat-session"; + +// Drive the hook the way the window does: the parent owns `activeChatId` and +// passes it back in. `setActiveChatId` is a spy so we can assert the EXACT id the +// hook adopts (the #137 regression: it must be the authoritative streamed id, not +// the newest chat in the list). +function setup(initial: { + activeChatId: string | null; + chats: { items?: { id: string }[] } | undefined; + messagesLoading?: boolean; +}) { + const setActiveChatId = vi.fn(); + const onInvalidateChatList = vi.fn(); + const onInvalidateChatMessages = vi.fn(); + const { result, rerender } = renderHook( + (props: { + activeChatId: string | null; + chats: { items?: { id: string }[] } | undefined; + messagesLoading?: boolean; + }) => + useChatSession({ + activeChatId: props.activeChatId, + setActiveChatId, + chats: props.chats, + messagesLoading: props.messagesLoading ?? false, + onInvalidateChatList, + onInvalidateChatMessages, + }), + { initialProps: initial }, + ); + return { + result, + rerender, + setActiveChatId, + onInvalidateChatList, + onInvalidateChatMessages, + }; +} + +describe("useChatSession", () => { + beforeEach(() => vi.clearAllMocks()); + + it("#137 REGRESSION LOCK: adopts the authoritative streamed id, NOT items[0]", () => { + // Brand-new chat, list already holds a SIBLING chat B as items[0] (a second + // tab just created it). The server streams the real id "A" for THIS chat. + const { result, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "B" }] }, + }); + result.current.onTurnFinished("A"); + // Must adopt the authoritative id, not the newest-in-list guess. + expect(setActiveChatId).toHaveBeenCalledWith("A"); + expect(setActiveChatId).not.toHaveBeenCalledWith("B"); + }); + + it("fallback adopt: arms on a server-id-less finish, adopts the single new id after refetch", () => { + const { result, rerender, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "x" }] }, + }); + // No server id => arm the fallback (no adoption yet). + result.current.onTurnFinished(undefined); + expect(setActiveChatId).not.toHaveBeenCalled(); + // The refetch lands with the new row => adopt it. + rerender({ activeChatId: null, chats: { items: [{ id: "x" }, { id: "new" }] } }); + expect(setActiveChatId).toHaveBeenCalledWith("new"); + }); + + it("fallback ambiguous: two new ids appear => no adoption", () => { + const { result, rerender, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "x" }] }, + }); + result.current.onTurnFinished(undefined); + rerender({ + activeChatId: null, + chats: { items: [{ id: "x" }, { id: "n1" }, { id: "n2" }] }, + }); + expect(setActiveChatId).not.toHaveBeenCalled(); + }); + + it("fallback add+delete in one window: adopts the new id (membership compare)", () => { + const { result, rerender, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "a" }, { id: "b" }] }, + }); + result.current.onTurnFinished(undefined); + // a was deleted, new was added — same length, but membership changed. + rerender({ activeChatId: null, chats: { items: [{ id: "b" }, { id: "new" }] } }); + expect(setActiveChatId).toHaveBeenCalledWith("new"); + }); + + it("disarm on reconcile: a fallback armed then switched away is NOT adopted by a late refetch", () => { + // Arm the error-path fallback on a brand-new chat (snapshot before=["x"]). + const { result, rerender, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "x" }] }, + }); + result.current.onTurnFinished(undefined); + // The user switches to an existing chat C BEFORE the refetch lands; the + // render-phase reconciler must DISARM the pending fallback. + rerender({ activeChatId: "C", chats: { items: [{ id: "x" }] } }); + // ...then starts a fresh new chat again (back to null), without re-arming. + rerender({ activeChatId: null, chats: { items: [{ id: "x" }] } }); + // A late refetch now brings a new row. Because the earlier fallback was + // disarmed on the switch (not left armed with the stale ["x"] snapshot), it + // must NOT be adopted. (Without the disarm this would wrongly adopt "new".) + rerender({ + activeChatId: null, + chats: { items: [{ id: "x" }, { id: "new" }] }, + }); + expect(setActiveChatId).not.toHaveBeenCalledWith("new"); + }); + + it("in-place adopt keeps threadKey stable; an external switch remounts", () => { + const chats = { items: [{ id: "B" }] }; + const { result, rerender } = setup({ activeChatId: null, chats }); + const keyBefore = result.current.threadKey; + // Adopt the streamed id; the PARENT then reflects activeChatId="A" back in. + result.current.onTurnFinished("A"); + rerender({ activeChatId: "A", chats }); + // In-place adoption: SAME mount key (the live useChat store is preserved). + expect(result.current.threadKey).toBe(keyBefore); + + // An EXTERNAL switch (not via adopt) to a different chat must remount: the + // key becomes the chat id. + rerender({ activeChatId: "C", chats }); + expect(result.current.threadKey).toBe("C"); + }); + + it("waitingForHistory gates the loader only while opening an unloaded existing chat", () => { + // Open an existing chat whose history is still loading => loader on. + const { result, rerender } = setup({ + activeChatId: "chat-1", + chats: { items: [{ id: "chat-1" }] }, + messagesLoading: true, + }); + expect(result.current.waitingForHistory).toBe(true); + // Once loading finishes, the latch flips and the loader is off. + rerender({ + activeChatId: "chat-1", + chats: { items: [{ id: "chat-1" }] }, + messagesLoading: false, + }); + expect(result.current.waitingForHistory).toBe(false); + }); +}); diff --git a/apps/client/src/features/ai-chat/hooks/use-chat-session.ts b/apps/client/src/features/ai-chat/hooks/use-chat-session.ts new file mode 100644 index 00000000..65392f62 --- /dev/null +++ b/apps/client/src/features/ai-chat/hooks/use-chat-session.ts @@ -0,0 +1,196 @@ +import { useCallback, useEffect, useReducer, useRef } from "react"; +import { generateId } from "ai"; +import { + resolveAdoptedChatId, + newlyAddedChatIds, +} from "@/features/ai-chat/utils/adopt-chat-id.ts"; +import { + newThread, + switchThread, + threadSessionReducer, +} from "@/features/ai-chat/utils/thread-identity.ts"; + +/** What the window needs from a chat session: the ChatThread mount key, the + * history-loader gate, and the turn-finished callback. */ +export interface ChatSession { + /** ChatThread mount key (was `thread.key`). */ + threadKey: string; + /** Show the history loader instead of the live thread. */ + waitingForHistory: boolean; + /** Call when a turn finishes; `serverChatId` is the authoritative streamed id + * (undefined on a failed turn). Handles new-chat id adoption + invalidations. */ + onTurnFinished: (serverChatId?: string) => void; +} + +/** + * Owns the AI-chat thread-identity lifecycle: the single atomic thread identity, + * both new-chat id adoption paths (primary streamed-metadata + bounded error-path + * fallback), the history-loaded latch, and the render-phase reconciler that keeps + * the thread's mount key in sync with the public `activeChatId` atom. + * + * This is the twice-bugged area for the #137 two-tab adoption race; the canonical + * explanation of the adoption design lives in adopt-chat-id.ts. + */ +export function useChatSession(params: { + activeChatId: string | null; + setActiveChatId: (id: string | null) => void; + chats: { items?: { id: string }[] } | undefined; + messagesLoading: boolean; + /** Wraps queryClient.invalidateQueries(AI_CHATS_RQ_KEY). */ + onInvalidateChatList: () => void; + /** Wraps the per-chat messages invalidation. */ + onInvalidateChatMessages: (chatId: string) => void; +}): ChatSession { + const { + activeChatId, + setActiveChatId, + chats, + messagesLoading, + onInvalidateChatList, + onInvalidateChatMessages, + } = params; + + // The mounted thread's identity: ONE atomic value tying ChatThread's mount key + // (`thread.key`) to the chat id that mounted thread holds (`thread.chatId`). + // Consolidating these makes the "key vs chat id diverged" state unrepresentable + // — every change goes through an explicit transition (see thread-identity.ts): + // `newThread`/`switchThread` to (re)mount, `adoptThread` for in-place adoption. + // Initial: a non-null activeChatId switches to it; a null one gets a fresh + // session key with no chat id yet. + const [thread, dispatch] = useReducer( + threadSessionReducer, + undefined, + () => + activeChatId === null + ? newThread(`new-${generateId()}`) + : switchThread(activeChatId), + ); + + // Error-path fallback for new-chat id adoption. When a brand-new chat's first + // turn errors BEFORE the server's `start` chunk, no authoritative chatId ever + // reaches the client, so the primary metadata adoption cannot run. We then ARM + // this ref with a snapshot of the currently-known chat ids; once the list + // refetch lands with the just-created row, the fallback effect below adopts the + // SINGLE newly-appeared id. `null` = not armed. See adopt-chat-id.ts (#137). + const pendingNewChatRef = useRef(null); + + // Latch: the chat id whose full persisted history has finished loading while + // its thread is mounted. Used so a later BACKGROUND refetch (the post-turn + // messages invalidation) never tears the live thread back down to the loader. + const historyLoadedKeyRef = useRef(null); + + // After a turn finishes, refresh the chat list. For a brand-new chat (no id + // yet) we adopt the server's AUTHORITATIVE streamed id (never the newest in the + // list, which races a second tab — #137; see adopt-chat-id.ts). + const onTurnFinished = useCallback( + (serverChatId?: string) => { + const adopted = resolveAdoptedChatId(activeChatId, serverChatId); + if (adopted) { + // PRIMARY path. In-place adoption: set the public selection and the + // thread identity to the real id together. `adopt` keeps the SAME mount + // key, so the render-phase reconciler sees `activeChatId === thread.chatId` + // and keeps the SAME mounted thread (its useChat already holds the + // just-finished turn) instead of remounting + re-seeding from + // not-yet-persisted history. + setActiveChatId(adopted); + dispatch({ type: "adopt", chatId: adopted }); + // Primary adoption won — disarm any previously-armed fallback. + pendingNewChatRef.current = null; + } else if (activeChatId === null) { + // FALLBACK path: a brand-new chat finished with NO server id (the first + // turn errored before the `start` chunk). Arm the bounded list-refetch + // fallback by snapshotting the currently-known chat ids. `chats` is still + // the pre-refetch list here, so the just-created row is NOT yet in it; the + // effect below adopts the single id that newly appears after the refetch. + pendingNewChatRef.current = chats?.items?.map((c) => c.id) ?? []; + } + onInvalidateChatList(); + // Re-sync the persisted message rows for the active chat so the Markdown + // export and token counters reflect the just-finished turn. The live thread + // renders from its own useChat store (stable thread.key), so this never + // re-seeds or tears down the open thread. For a brand-new chat activeChatId + // is still null here; later turns hit this with the adopted id. + if (activeChatId) { + onInvalidateChatMessages(activeChatId); + } + }, + [ + activeChatId, + chats, + setActiveChatId, + onInvalidateChatList, + onInvalidateChatMessages, + ], + ); + + // FALLBACK resolver. Armed only by onTurnFinished when a brand-new chat's first + // turn errored before the `start` chunk (no authoritative id streamed). Once + // the per-user list refetch lands with the just-created row, adopt the SINGLE + // id that newly appeared relative to the pre-refetch snapshot. Adoption is IN + // PLACE (set activeChatId + `adopt` together) like the primary path, so the + // render-phase reconciler does not remount. + useEffect(() => { + const before = pendingNewChatRef.current; + if (before === null || activeChatId !== null) return; // not armed / already adopted + const after = chats?.items?.map((c) => c.id) ?? []; + const added = newlyAddedChatIds(before, after); + // Keep waiting until a genuinely-new id appears. Set-based, so it is robust + // to an add+delete in the same window (a length compare would miss it), and + // it deliberately keeps waiting through an unrelated deletion (no new id yet) + // until the just-created row actually lands, rather than giving up early. + if (added.size === 0) return; // list not refetched yet — keep waiting + pendingNewChatRef.current = null; // resolved — disarm + if (added.size === 1) { + // single unambiguous new id; >1 = ambiguous → give up + const adopted = [...added][0]; + setActiveChatId(adopted); + dispatch({ type: "adopt", chatId: adopted }); + } + }, [chats, activeChatId, setActiveChatId]); + + // Reconcile the thread identity against the active-chat atom during render when + // they diverge — the React-sanctioned alternative to an effect (re-renders + // before paint, no extra commit, and converges since the next render finds them + // equal). This reconciliation MUST remain: `activeChatId` is the public + // selection and is ALSO set from OUTSIDE this component (e.g. page-history opens + // a referenced chat via setActiveChatId). A divergence here is a genuine SWITCH + // (external atom change OR user switch via selectChat/startNewChat), so + // `reconcile` remounts + reseeds. In-place adoption never reaches this branch: + // it set activeChatId and thread.chatId to the same value. + if (activeChatId !== thread.chatId) { + // A genuine switch makes any pending error-path new-chat fallback moot. + pendingNewChatRef.current = null; + dispatch({ + type: "reconcile", + chatId: activeChatId, + newKey: `new-${generateId()}`, + }); + } + + // Latch the active chat once its full history has loaded and its thread is + // mounted, so a later background refetch (the post-turn messages invalidation, + // which can transiently flip hasNextPage for a chat whose message count is an + // exact multiple of the server page size) does not tear the live thread down to + // a loader and lose its in-progress useChat state. + if ( + activeChatId !== null && + thread.key === activeChatId && + !messagesLoading && + historyLoadedKeyRef.current !== activeChatId + ) { + historyLoadedKeyRef.current = activeChatId; + } + + // Show the history loader only when freshly OPENING an existing chat (the key + // equals the chat id) whose history has not been fully loaded yet. For a live + // in-place thread that adopted its id, the key is still the "new-…" session + // key, so the live thread keeps rendering; and once a chat's history has loaded, + // a later background refetch no longer tears it down (see the latch above). + const waitingForHistory = + activeChatId !== null && + messagesLoading && + thread.key === activeChatId && + historyLoadedKeyRef.current !== activeChatId; + + return { threadKey: thread.key, waitingForHistory, onTurnFinished }; +} diff --git a/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts b/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts index d5ba12ec..39a40984 100644 --- a/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts +++ b/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from "vitest"; -import { resolveAdoptedChatId, pickNewlyCreatedChatId } from "./adopt-chat-id"; +import { + resolveAdoptedChatId, + pickNewlyCreatedChatId, + newlyAddedChatIds, + extractServerChatId, +} from "./adopt-chat-id"; describe("resolveAdoptedChatId", () => { it("adopts the server id for a brand-new chat (activeChatId null + id)", () => { @@ -41,3 +46,54 @@ describe("pickNewlyCreatedChatId", () => { expect(pickNewlyCreatedChatId(["a", "b"], ["b", "a"])).toBeNull(); }); }); + +describe("newlyAddedChatIds", () => { + it("returns the single new id", () => { + expect([...newlyAddedChatIds(["a", "b"], ["a", "b", "c"])]).toEqual(["c"]); + }); + + it("returns an empty set when nothing was added", () => { + expect(newlyAddedChatIds(["a", "b"], ["b", "a"]).size).toBe(0); + }); + + it("returns both new ids when two were added", () => { + expect(newlyAddedChatIds(["a"], ["a", "b", "c"])).toEqual( + new Set(["b", "c"]), + ); + }); + + it("keeps only the new id across an add+delete in the same window", () => { + // before [a,b] -> after [b,new]: a was deleted, new was added. + expect([...newlyAddedChatIds(["a", "b"], ["b", "new"])]).toEqual(["new"]); + }); + + it("dedupes a repeated new id to a single entry", () => { + expect(newlyAddedChatIds(["a"], ["a", "new", "new"])).toEqual( + new Set(["new"]), + ); + }); +}); + +describe("extractServerChatId", () => { + it("returns the chatId when present on metadata", () => { + expect(extractServerChatId({ metadata: { chatId: "chat-1" } })).toBe( + "chat-1", + ); + }); + + it("returns undefined when the message has no metadata", () => { + expect(extractServerChatId({})).toBeUndefined(); + }); + + it("returns undefined when metadata lacks chatId", () => { + expect(extractServerChatId({ metadata: { other: 1 } })).toBeUndefined(); + }); + + it("returns undefined for a non-string chatId", () => { + expect(extractServerChatId({ metadata: { chatId: 42 } })).toBeUndefined(); + }); + + it("returns undefined for an undefined message", () => { + expect(extractServerChatId(undefined)).toBeUndefined(); + }); +}); diff --git a/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts b/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts index 16bebc04..57d2388b 100644 --- a/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts +++ b/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts @@ -1,15 +1,33 @@ /** * Pure helpers for adopting a brand-new chat's authoritative server id. * + * ============================ CANONICAL #137 NOTE ============================ + * This docblock is the single authoritative explanation of the new-chat id + * adoption design and the #137 two-tab race it fixes. Other call sites + * (use-chat-session.ts, the server's `chatStreamStartMetadata`) reference here + * rather than restating it. + * + * When a user sends the first turn of a BRAND-NEW chat, the client has no chat + * id yet (`activeChatId === null`). The server creates the row and the client + * must "adopt" that row's real id so the SECOND turn targets the same chat. + * + * The OLD heuristic adopted `items[0]` — the newest chat in the refetched list. + * That races a second tab: if another tab created a chat in the same moment, + * its row could be `items[0]`, so this tab would adopt the SIBLING chat and + * leak its later turns into it (#137). We adopt by IDENTITY instead, two ways: + * * PRIMARY path: the server streams the real chat id on the assistant message - * metadata (see `chatStreamStartMetadata` server-side); `resolveAdoptedChatId` - * turns that into the id to adopt for a new chat. + * metadata's `start` part (see `chatStreamStartMetadata` server-side); + * `extractServerChatId` reads it off the finished message and + * `resolveAdoptedChatId` turns it into the id to adopt for a new chat. This is + * authoritative and immune to the race. * * FALLBACK path (only when a new chat's first turn errors BEFORE the `start` * chunk, so no metadata id ever reached the client): adopt the single chat that * NEWLY appeared in the per-user list relative to a pre-refetch snapshot — - * `pickNewlyCreatedChatId`. This is unambiguous and does not race a second tab - * the way the old "newest chat in the list" guess did (#137). + * `newlyAddedChatIds` / `pickNewlyCreatedChatId`. This is unambiguous and does + * not race a second tab the way the old "newest chat in the list" guess did. + * ============================================================================ */ /** @@ -24,6 +42,32 @@ export function resolveAdoptedChatId( return activeChatId === null && serverChatId ? serverChatId : null; } +/** + * Read the authoritative server chat id off a finished assistant message. The + * server attaches it as `message.metadata.chatId` on the `start` part (see + * `chatStreamStartMetadata`). Returns it only when it is a string; undefined for + * a missing message, missing metadata, or a non-string `chatId`. + */ +export function extractServerChatId( + message: { metadata?: unknown } | undefined, +): string | undefined { + const m = message?.metadata as { chatId?: string } | undefined; + return typeof m?.chatId === "string" ? m.chatId : undefined; +} + +/** + * The deduped set of ids present in `afterIds` but not in `beforeIds`. A + * paginated/flatMapped list can repeat the same id, so dedupe: one genuinely-new + * chat must not read as multiple from a duplicate. + */ +export function newlyAddedChatIds( + beforeIds: readonly string[], + afterIds: readonly string[], +): Set { + const before = new Set(beforeIds); + return new Set(afterIds.filter((id) => !before.has(id))); +} + /** * Return the single id present in `afterIds` but not in `beforeIds`. Returns * null when zero or more-than-one such id exists (ambiguous — do not adopt). @@ -32,9 +76,6 @@ export function pickNewlyCreatedChatId( beforeIds: readonly string[], afterIds: readonly string[], ): string | null { - const before = new Set(beforeIds); - // Dedupe the new ids: a paginated/flatMapped list can repeat the same id, and - // one genuinely-new chat must not read as "ambiguous" (>1) from a duplicate. - const added = new Set(afterIds.filter((id) => !before.has(id))); + const added = newlyAddedChatIds(beforeIds, afterIds); return added.size === 1 ? [...added][0] : null; } diff --git a/apps/client/src/features/ai-chat/utils/thread-identity.test.ts b/apps/client/src/features/ai-chat/utils/thread-identity.test.ts index 3ab65a11..eab1eadb 100644 --- a/apps/client/src/features/ai-chat/utils/thread-identity.test.ts +++ b/apps/client/src/features/ai-chat/utils/thread-identity.test.ts @@ -1,5 +1,10 @@ import { describe, it, expect } from "vitest"; -import { newThread, switchThread, adoptThread } from "./thread-identity"; +import { + newThread, + switchThread, + adoptThread, + threadSessionReducer, +} from "./thread-identity"; describe("newThread", () => { it("uses the supplied key and has no chat id yet", () => { @@ -17,6 +22,10 @@ describe("switchThread", () => { }); describe("adoptThread", () => { + // Key UNCHANGED (no remount) + chatId moved null->realId. The unchanged key is + // what keeps the live useChat store alive; the matching chatId is what makes the + // window's render-phase reconciler (activeChatId !== thread.chatId) treat the + // adopted thread as already-in-sync rather than a switch. it("adopts in place for a new chat: keeps the key, sets the chat id", () => { const prev = newThread("new-abc"); expect(adoptThread(prev, "chat-1")).toEqual({ @@ -32,22 +41,39 @@ describe("adoptThread", () => { }; expect(adoptThread(prev, "chat-2")).toBe(prev); }); +}); - it("INVARIANT: adoption never remounts (key unchanged) while chatId changes", () => { - const prev = newThread("new-abc"); - const next = adoptThread(prev, "chat-1"); - // The mount key is preserved (no remount) ... - expect(next.key).toBe(prev.key); - // ... while the chat id moved from null to the real id. - expect(prev.chatId).toBeNull(); - expect(next.chatId).toBe("chat-1"); +describe("threadSessionReducer", () => { + it("reconcile to an existing id switches (key becomes the id)", () => { + const next = threadSessionReducer(newThread("new-abc"), { + type: "reconcile", + chatId: "chat-1", + newKey: "new-xyz", + }); + expect(next).toEqual({ key: "chat-1", chatId: "chat-1" }); }); - it("INVARIANT: after adoption thread.chatId equals the adopted id, so the window's render-phase reconciler (activeChatId !== thread.chatId) does NOT fire and remount the live thread", () => { - const adoptedId = "chat-1"; - const next = adoptThread(newThread("new-abc"), adoptedId); - // The window sets activeChatId to the same adoptedId; this asserts they match - // so the reconciler treats it as already-in-sync, not a switch. - expect(next.chatId).toBe(adoptedId); + it("reconcile to null starts a fresh new thread with the supplied key", () => { + const next = threadSessionReducer(switchThread("chat-1"), { + type: "reconcile", + chatId: null, + newKey: "new-xyz", + }); + expect(next).toEqual({ key: "new-xyz", chatId: null }); + }); + + it("adopt on a new thread keeps the key and sets the id", () => { + const next = threadSessionReducer(newThread("new-abc"), { + type: "adopt", + chatId: "chat-1", + }); + expect(next).toEqual({ key: "new-abc", chatId: "chat-1" }); + }); + + it("adopt on a persisted thread is a no-op", () => { + const prev = switchThread("chat-1"); + expect(threadSessionReducer(prev, { type: "adopt", chatId: "chat-2" })).toBe( + prev, + ); }); }); diff --git a/apps/client/src/features/ai-chat/utils/thread-identity.ts b/apps/client/src/features/ai-chat/utils/thread-identity.ts index 4b289615..f5408fce 100644 --- a/apps/client/src/features/ai-chat/utils/thread-identity.ts +++ b/apps/client/src/features/ai-chat/utils/thread-identity.ts @@ -45,3 +45,29 @@ export function switchThread(chatId: string): ThreadIdentity { export function adoptThread(prev: ThreadIdentity, chatId: string): ThreadIdentity { return prev.chatId === null ? { key: prev.key, chatId } : prev; } + +/** + * Thread-identity transitions as a reducer action. See `threadSessionReducer`. + */ +export type ThreadSessionAction = + | { type: "reconcile"; chatId: string | null; newKey: string } + | { type: "adopt"; chatId: string }; + +/** + * Single source of truth for thread-identity transitions. `reconcile` handles a + * genuine switch (user OR external atom write) -> remount; `adopt` moves a brand- + * new chat to its real id in place (no remount). + */ +export function threadSessionReducer( + state: ThreadIdentity, + action: ThreadSessionAction, +): ThreadIdentity { + switch (action.type) { + case "reconcile": + return action.chatId === null + ? newThread(action.newKey) + : switchThread(action.chatId); + case "adopt": + return adoptThread(state, action.chatId); + } +} diff --git a/apps/server/src/core/ai-chat/ai-chat.service.ts b/apps/server/src/core/ai-chat/ai-chat.service.ts index b8d925e4..71550f6c 100644 --- a/apps/server/src/core/ai-chat/ai-chat.service.ts +++ b/apps/server/src/core/ai-chat/ai-chat.service.ts @@ -562,12 +562,9 @@ export class AiChatService { } /** - * Compute the `messageMetadata` payload for the streamed assistant UI message. - * The AI SDK invokes `messageMetadata` on each stream part (ai@6); we attach the - * authoritative `chatId` ONLY on the `start` part so it reaches the client at the - * very first chunk (as `message.metadata.chatId`). The client adopts THAT id for a - * new chat instead of guessing the newest chat in its list — fixing the two-tab - * adoption race (#137). Returning undefined for any non-start part adds no metadata. + * Attach the authoritative `chatId` to the streamed assistant message's `start` + * part (as `message.metadata.chatId`) so the client can adopt the real id for a + * new chat. See the client's adopt-chat-id.ts for the full #137 design. */ export function chatStreamStartMetadata( part: { type: string }, -- 2.49.1 From 8f9a218c682a1173554237b2e415e0011830a70a Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 23 Jun 2026 03:04:40 +0300 Subject: [PATCH 6/7] fix(ai-chat): disarm pending adoption on New chat/select; drop dead helper Closes the 3rd PR #138 review. Warning fix: the render-phase reconciler only disarms the error-path adoption fallback when activeChatId actually changes. Pressing 'New chat' while ALREADY in a new chat keeps activeChatId === null (a no-op atom write), so the reconciler never fired and a stale armed fallback could adopt the just-failed chat from a late refetch, yanking the user out of their fresh chat. useChatSession now returns cancelPendingAdoption(); the window calls it from startNewChat AND selectChat. (The hook call moved above those callbacks so they can reference it.) Added a hook test that fails without the explicit disarm, plus a test for the existing-chat onTurnFinished branch (no adoption + per-chat invalidation). Cleanups: removed the dead pickNewlyCreatedChatId (the fallback effect uses newlyAddedChatIds directly with the 0/1/>1 decision inline) and its tests/doc mention; inlined the two invalidation closures (onTurnFinished is read live by useChat's onFinish, never in an effect dep array, so memoizing them was needless ceremony). Verified: tsc clean, 127 ai-chat tests green; live (z.ai glm-5.2) new chat + 2nd turn recalled the number in the SAME row (1 chat / 4 messages), no page errors. Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/components/ai-chat-window.tsx | 56 +++++++++---------- .../ai-chat/hooks/use-chat-session.test.tsx | 31 ++++++++++ .../ai-chat/hooks/use-chat-session.ts | 21 ++++++- .../ai-chat/utils/adopt-chat-id.test.ts | 27 --------- .../features/ai-chat/utils/adopt-chat-id.ts | 17 +----- 5 files changed, 82 insertions(+), 70 deletions(-) diff --git a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx index 83200747..5ec80874 100644 --- a/apps/client/src/features/ai-chat/components/ai-chat-window.tsx +++ b/apps/client/src/features/ai-chat/components/ai-chat-window.tsx @@ -173,18 +173,42 @@ export default function AiChatWindow() { ? { id: openPageData.id, title: openPageData.title } : null; - // startNewChat/selectChat only set the public atom; useChatSession's render- - // phase reconciler does the remount AND disarms any pending new-chat fallback. + // The AI-chat thread-identity lifecycle (mount key, both new-chat id adoption + // paths, the history-loaded latch, the render-phase reconciler) lives in this + // hook. See adopt-chat-id.ts for the canonical #137 two-tab race explanation. + // The invalidate closures are passed inline: `onTurnFinished` is read live by + // useChat's onFinish (never in an effect dep array), so their identity does not + // matter — no memoization ceremony needed. + const { threadKey, waitingForHistory, onTurnFinished, cancelPendingAdoption } = + useChatSession({ + activeChatId, + setActiveChatId, + chats, + messagesLoading, + onInvalidateChatList: () => + queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }), + onInvalidateChatMessages: (id) => + queryClient.invalidateQueries({ queryKey: AI_CHAT_MESSAGES_RQ_KEY(id) }), + }); + + // startNewChat/selectChat set the public atom; the hook's render-phase + // reconciler handles the remount when activeChatId actually CHANGES. But + // pressing "New chat" while already in a new chat leaves activeChatId === null + // (a no-op for the atom), so the reconciler never fires — explicitly disarm any + // armed error-path fallback here so a late refetch can't yank the user into a + // just-failed chat after they chose a fresh one. const startNewChat = useCallback((): void => { + cancelPendingAdoption(); setActiveChatId(null); setHistoryOpen(false); setDraft(""); // Default the picker back to "Universal assistant" for the fresh chat. setSelectedRoleId(null); - }, [setActiveChatId, setDraft, setSelectedRoleId]); + }, [cancelPendingAdoption, setActiveChatId, setDraft, setSelectedRoleId]); const selectChat = useCallback( (chatId: string): void => { + cancelPendingAdoption(); setActiveChatId(chatId); setHistoryOpen(false); setDraft(""); @@ -192,33 +216,9 @@ export default function AiChatWindow() { // chat's header/assistant-name (which prefers the chat's persisted role). setSelectedRoleId(null); }, - [setActiveChatId, setDraft, setSelectedRoleId], + [cancelPendingAdoption, setActiveChatId, setDraft, setSelectedRoleId], ); - // The AI-chat thread-identity lifecycle (mount key, both new-chat id adoption - // paths, the history-loaded latch, the render-phase reconciler) lives in this - // hook. See adopt-chat-id.ts for the canonical #137 two-tab race explanation. - // Memoized so the hook's `onTurnFinished` useCallback keeps a stable identity - // across renders (it is read live by useChat's onFinish, so this is cosmetic, - // not load-bearing — but avoids needless re-creation). - const invalidateChatList = useCallback( - () => queryClient.invalidateQueries({ queryKey: AI_CHATS_RQ_KEY }), - [queryClient], - ); - const invalidateChatMessages = useCallback( - (id: string) => - queryClient.invalidateQueries({ queryKey: AI_CHAT_MESSAGES_RQ_KEY(id) }), - [queryClient], - ); - const { threadKey, waitingForHistory, onTurnFinished } = useChatSession({ - activeChatId, - setActiveChatId, - chats, - messagesLoading, - onInvalidateChatList: invalidateChatList, - onInvalidateChatMessages: invalidateChatMessages, - }); - // The active chat object (for its title) and an export gate: only enable the // export button when an existing chat with loaded persisted rows is active. const activeChat = useMemo( diff --git a/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx b/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx index e7b8695c..fdd13f91 100644 --- a/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx +++ b/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx @@ -114,6 +114,37 @@ describe("useChatSession", () => { expect(setActiveChatId).not.toHaveBeenCalledWith("new"); }); + it("startNewChat while already in a new chat: cancelPendingAdoption stops a late refetch adopting the failed chat", () => { + // The Warning path the render-phase reconciler can't catch: pressing "New + // chat" while already in a new chat keeps activeChatId === null (a no-op for + // the atom), so only the explicit cancelPendingAdoption() disarms. + const { result, rerender, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "x" }] }, + }); + result.current.onTurnFinished(undefined); // first turn failed → arm (before=["x"]) + result.current.cancelPendingAdoption(); // window calls this from startNewChat + // The just-failed row lands in a late refetch; it must NOT be adopted. + rerender({ + activeChatId: null, + chats: { items: [{ id: "x" }, { id: "failed" }] }, + }); + expect(setActiveChatId).not.toHaveBeenCalledWith("failed"); + }); + + it("onTurnFinished for an existing chat: no adoption, invalidates that chat's messages", () => { + const { + result, + setActiveChatId, + onInvalidateChatList, + onInvalidateChatMessages, + } = setup({ activeChatId: "chat-1", chats: { items: [{ id: "chat-1" }] } }); + result.current.onTurnFinished("chat-1"); + expect(setActiveChatId).not.toHaveBeenCalled(); // existing chat is never re-adopted + expect(onInvalidateChatList).toHaveBeenCalled(); + expect(onInvalidateChatMessages).toHaveBeenCalledWith("chat-1"); + }); + it("in-place adopt keeps threadKey stable; an external switch remounts", () => { const chats = { items: [{ id: "B" }] }; const { result, rerender } = setup({ activeChatId: null, chats }); diff --git a/apps/client/src/features/ai-chat/hooks/use-chat-session.ts b/apps/client/src/features/ai-chat/hooks/use-chat-session.ts index 65392f62..46283ee6 100644 --- a/apps/client/src/features/ai-chat/hooks/use-chat-session.ts +++ b/apps/client/src/features/ai-chat/hooks/use-chat-session.ts @@ -20,6 +20,10 @@ export interface ChatSession { /** Call when a turn finishes; `serverChatId` is the authoritative streamed id * (undefined on a failed turn). Handles new-chat id adoption + invalidations. */ onTurnFinished: (serverChatId?: string) => void; + /** Disarm any pending error-path new-chat fallback. The window calls this from + * startNewChat/selectChat so a late refetch can't yank the user back into a + * just-failed chat after they explicitly moved on. */ + cancelPendingAdoption: () => void; } /** @@ -192,5 +196,20 @@ export function useChatSession(params: { thread.key === activeChatId && historyLoadedKeyRef.current !== activeChatId; - return { threadKey: thread.key, waitingForHistory, onTurnFinished }; + // Explicit disarm for startNewChat/selectChat. The render-phase reconciler only + // disarms when activeChatId actually changes, but "New chat" pressed while the + // user is ALREADY in a new chat is a no-op for the atom (activeChatId stays + // null), so the reconciler never fires — without this an armed fallback could + // adopt the just-failed chat from a late refetch and yank the user out of their + // fresh chat. Stable identity (writes a ref). + const cancelPendingAdoption = useCallback(() => { + pendingNewChatRef.current = null; + }, []); + + return { + threadKey: thread.key, + waitingForHistory, + onTurnFinished, + cancelPendingAdoption, + }; } diff --git a/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts b/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts index 39a40984..c9ff117a 100644 --- a/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts +++ b/apps/client/src/features/ai-chat/utils/adopt-chat-id.test.ts @@ -1,7 +1,6 @@ import { describe, it, expect } from "vitest"; import { resolveAdoptedChatId, - pickNewlyCreatedChatId, newlyAddedChatIds, extractServerChatId, } from "./adopt-chat-id"; @@ -21,32 +20,6 @@ describe("resolveAdoptedChatId", () => { }); }); -describe("pickNewlyCreatedChatId", () => { - it("returns the single newly-appeared id", () => { - expect(pickNewlyCreatedChatId(["a", "b"], ["c", "a", "b"])).toBe("c"); - }); - - it("returns null when no new id appeared", () => { - expect(pickNewlyCreatedChatId(["a", "b"], ["a", "b"])).toBeNull(); - }); - - it("returns null when more than one new id appeared (ambiguous)", () => { - expect(pickNewlyCreatedChatId(["a"], ["a", "b", "c"])).toBeNull(); - }); - - it("returns the single after id when before is empty", () => { - expect(pickNewlyCreatedChatId([], ["only"])).toBe("only"); - }); - - it("treats a duplicated new id as one (deduped, not ambiguous)", () => { - expect(pickNewlyCreatedChatId(["a"], ["a", "new", "new"])).toBe("new"); - }); - - it("returns null when membership is unchanged but reordered", () => { - expect(pickNewlyCreatedChatId(["a", "b"], ["b", "a"])).toBeNull(); - }); -}); - describe("newlyAddedChatIds", () => { it("returns the single new id", () => { expect([...newlyAddedChatIds(["a", "b"], ["a", "b", "c"])]).toEqual(["c"]); diff --git a/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts b/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts index 57d2388b..1993dccc 100644 --- a/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts +++ b/apps/client/src/features/ai-chat/utils/adopt-chat-id.ts @@ -25,8 +25,9 @@ * FALLBACK path (only when a new chat's first turn errors BEFORE the `start` * chunk, so no metadata id ever reached the client): adopt the single chat that * NEWLY appeared in the per-user list relative to a pre-refetch snapshot — - * `newlyAddedChatIds` / `pickNewlyCreatedChatId`. This is unambiguous and does - * not race a second tab the way the old "newest chat in the list" guess did. + * `newlyAddedChatIds` (the fallback effect adopts only when exactly one id is + * new). This is unambiguous and does not race a second tab the way the old + * "newest chat in the list" guess did. * ============================================================================ */ @@ -67,15 +68,3 @@ export function newlyAddedChatIds( const before = new Set(beforeIds); return new Set(afterIds.filter((id) => !before.has(id))); } - -/** - * Return the single id present in `afterIds` but not in `beforeIds`. Returns - * null when zero or more-than-one such id exists (ambiguous — do not adopt). - */ -export function pickNewlyCreatedChatId( - beforeIds: readonly string[], - afterIds: readonly string[], -): string | null { - const added = newlyAddedChatIds(beforeIds, afterIds); - return added.size === 1 ? [...added][0] : null; -} -- 2.49.1 From 870df458eda3a4c35421687addc5a0f97e7726c1 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Tue, 23 Jun 2026 03:33:15 +0300 Subject: [PATCH 7/7] test(ai-chat): cover double onTurnFinished; name hook option/result types MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the 4th PR #138 review (3 suggestions, no blockers). - Double-call safety: a failed turn fires both useChat onFinish AND onError, so onTurnFinished can run twice in one turn (streamed id, then no id) before a re-render. onTurnFinished now reads the live id from a ref (set imperatively on primary adoption) instead of the stale closure, so the 2nd no-id call cannot re-arm the error-path fallback at the source; the render-phase reconciler is the second layer. Added a hook test for the sequence — verified it fails only if BOTH layers are removed (non-tautological). - Conventions: extracted named UseChatSessionOptions / UseChatSessionResult interfaces (was an inline param literal + ChatSession); the test derives its driver props from them. - Simplification: extracted the chatIdSnapshot(chats) projection used at both the fallback arm site and the resolver effect. Architecture notes from the review (caller-driven disarm contract; cross-process {chatId} type) intentionally left as Variant A per the reviewer's recommendation. tsc clean; 128 ai-chat tests green. Co-Authored-By: Claude Opus 4.8 --- .../ai-chat/hooks/use-chat-session.test.tsx | 46 +++++++++--- .../ai-chat/hooks/use-chat-session.ts | 75 ++++++++++++------- 2 files changed, 85 insertions(+), 36 deletions(-) diff --git a/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx b/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx index fdd13f91..8104d1e6 100644 --- a/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx +++ b/apps/client/src/features/ai-chat/hooks/use-chat-session.test.tsx @@ -1,25 +1,25 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; import { renderHook } from "@testing-library/react"; import { useChatSession } from "./use-chat-session"; +import type { UseChatSessionOptions } from "./use-chat-session"; + +// The props the test drives: the parent-owned subset of UseChatSessionOptions +// (the spies are injected by setup, not per-render). messagesLoading is optional +// here (defaulted to false in setup) for terser test call sites. +type DriverProps = Pick & { + messagesLoading?: boolean; +}; // Drive the hook the way the window does: the parent owns `activeChatId` and // passes it back in. `setActiveChatId` is a spy so we can assert the EXACT id the // hook adopts (the #137 regression: it must be the authoritative streamed id, not // the newest chat in the list). -function setup(initial: { - activeChatId: string | null; - chats: { items?: { id: string }[] } | undefined; - messagesLoading?: boolean; -}) { +function setup(initial: DriverProps) { const setActiveChatId = vi.fn(); const onInvalidateChatList = vi.fn(); const onInvalidateChatMessages = vi.fn(); const { result, rerender } = renderHook( - (props: { - activeChatId: string | null; - chats: { items?: { id: string }[] } | undefined; - messagesLoading?: boolean; - }) => + (props: DriverProps) => useChatSession({ activeChatId: props.activeChatId, setActiveChatId, @@ -145,6 +145,32 @@ describe("useChatSession", () => { expect(onInvalidateChatMessages).toHaveBeenCalledWith("chat-1"); }); + it("double onTurnFinished on a failed-after-start turn: primary adopt, 2nd no-id call does NOT re-arm the fallback", () => { + // ai@6 fires onFinish AND onError on a failed turn. If the failure happened + // AFTER the `start` chunk, onFinish carries the streamed id and onError does + // not — so onTurnFinished runs twice in one turn (id, then no-id) before any + // re-render. The 2nd call must NOT re-arm the fallback off the still-null + // closure; otherwise a late refetch (parent hasn't reflected the adoption yet) + // would wrongly adopt a sibling row. + const { result, rerender, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "x" }] }, + }); + result.current.onTurnFinished("A"); // onFinish: primary adoption + expect(setActiveChatId).toHaveBeenCalledWith("A"); + result.current.onTurnFinished(undefined); // onError: same turn, no id + // Even in the worst case (the parent has NOT yet reflected activeChatId="A" + // and a late refetch lands a new row), the just-failed sibling must NOT be + // adopted. Two layers guarantee this: the ref guard keeps the 2nd call from + // re-arming at the source, and the render-phase reconciler disarms anything + // stale once thread.chatId ("A") diverges from the still-null activeChatId. + rerender({ + activeChatId: null, + chats: { items: [{ id: "x" }, { id: "late" }] }, + }); + expect(setActiveChatId).not.toHaveBeenCalledWith("late"); + }); + it("in-place adopt keeps threadKey stable; an external switch remounts", () => { const chats = { items: [{ id: "B" }] }; const { result, rerender } = setup({ activeChatId: null, chats }); diff --git a/apps/client/src/features/ai-chat/hooks/use-chat-session.ts b/apps/client/src/features/ai-chat/hooks/use-chat-session.ts index 46283ee6..998f2631 100644 --- a/apps/client/src/features/ai-chat/hooks/use-chat-session.ts +++ b/apps/client/src/features/ai-chat/hooks/use-chat-session.ts @@ -10,9 +10,23 @@ import { threadSessionReducer, } from "@/features/ai-chat/utils/thread-identity.ts"; +/** Inputs to {@link useChatSession}. `activeChatId`/`setActiveChatId` are the + * public selection atom (also written from outside the window, e.g. page + * history); the rest is read-only context the hook needs. */ +export interface UseChatSessionOptions { + activeChatId: string | null; + setActiveChatId: (id: string | null) => void; + chats: { items?: { id: string }[] } | undefined; + messagesLoading: boolean; + /** Wraps queryClient.invalidateQueries(AI_CHATS_RQ_KEY). */ + onInvalidateChatList: () => void; + /** Wraps the per-chat messages invalidation. */ + onInvalidateChatMessages: (chatId: string) => void; +} + /** What the window needs from a chat session: the ChatThread mount key, the * history-loader gate, and the turn-finished callback. */ -export interface ChatSession { +export interface UseChatSessionResult { /** ChatThread mount key (was `thread.key`). */ threadKey: string; /** Show the history loader instead of the live thread. */ @@ -26,6 +40,14 @@ export interface ChatSession { cancelPendingAdoption: () => void; } +/** Project a chat list to its id array (the before/after snapshot for the + * error-path fallback). */ +function chatIdSnapshot( + chats: { items?: { id: string }[] } | undefined, +): string[] { + return chats?.items?.map((c) => c.id) ?? []; +} + /** * Owns the AI-chat thread-identity lifecycle: the single atomic thread identity, * both new-chat id adoption paths (primary streamed-metadata + bounded error-path @@ -35,16 +57,9 @@ export interface ChatSession { * This is the twice-bugged area for the #137 two-tab adoption race; the canonical * explanation of the adoption design lives in adopt-chat-id.ts. */ -export function useChatSession(params: { - activeChatId: string | null; - setActiveChatId: (id: string | null) => void; - chats: { items?: { id: string }[] } | undefined; - messagesLoading: boolean; - /** Wraps queryClient.invalidateQueries(AI_CHATS_RQ_KEY). */ - onInvalidateChatList: () => void; - /** Wraps the per-chat messages invalidation. */ - onInvalidateChatMessages: (chatId: string) => void; -}): ChatSession { +export function useChatSession( + params: UseChatSessionOptions, +): UseChatSessionResult { const { activeChatId, setActiveChatId, @@ -54,6 +69,15 @@ export function useChatSession(params: { onInvalidateChatMessages, } = params; + // Live mirror of `activeChatId`, read by onTurnFinished. ai@6 fires both + // onFinish AND onError on a failed turn, so onTurnFinished can run twice in one + // turn (once with the streamed id, once without) BEFORE a re-render. Reading + // the ref — which the primary-adoption branch updates imperatively — makes that + // second call see the just-adopted id, so it cannot re-arm the fallback. (A + // plain closure over `activeChatId` would still read null on the second call.) + const activeChatIdRef = useRef(activeChatId); + activeChatIdRef.current = activeChatId; + // The mounted thread's identity: ONE atomic value tying ChatThread's mount key // (`thread.key`) to the chat id that mounted thread holds (`thread.chatId`). // Consolidating these makes the "key vs chat id diverged" state unrepresentable @@ -88,7 +112,11 @@ export function useChatSession(params: { // list, which races a second tab — #137; see adopt-chat-id.ts). const onTurnFinished = useCallback( (serverChatId?: string) => { - const adopted = resolveAdoptedChatId(activeChatId, serverChatId); + // Read the live id from the ref, not the closure: on a failed turn this can + // run twice in one turn (onFinish + onError) before any re-render, and the + // primary branch below updates the ref so the second call sees the adopted id. + const current = activeChatIdRef.current; + const adopted = resolveAdoptedChatId(current, serverChatId); if (adopted) { // PRIMARY path. In-place adoption: set the public selection and the // thread identity to the real id together. `adopt` keeps the SAME mount @@ -96,35 +124,30 @@ export function useChatSession(params: { // and keeps the SAME mounted thread (its useChat already holds the // just-finished turn) instead of remounting + re-seeding from // not-yet-persisted history. + activeChatIdRef.current = adopted; // a same-turn 2nd call now sees the id setActiveChatId(adopted); dispatch({ type: "adopt", chatId: adopted }); // Primary adoption won — disarm any previously-armed fallback. pendingNewChatRef.current = null; - } else if (activeChatId === null) { + } else if (current === null) { // FALLBACK path: a brand-new chat finished with NO server id (the first // turn errored before the `start` chunk). Arm the bounded list-refetch // fallback by snapshotting the currently-known chat ids. `chats` is still // the pre-refetch list here, so the just-created row is NOT yet in it; the // effect below adopts the single id that newly appears after the refetch. - pendingNewChatRef.current = chats?.items?.map((c) => c.id) ?? []; + pendingNewChatRef.current = chatIdSnapshot(chats); } onInvalidateChatList(); // Re-sync the persisted message rows for the active chat so the Markdown // export and token counters reflect the just-finished turn. The live thread // renders from its own useChat store (stable thread.key), so this never - // re-seeds or tears down the open thread. For a brand-new chat activeChatId - // is still null here; later turns hit this with the adopted id. - if (activeChatId) { - onInvalidateChatMessages(activeChatId); + // re-seeds or tears down the open thread. For a brand-new chat `current` is + // still null here; later turns hit this with the adopted id. + if (current) { + onInvalidateChatMessages(current); } }, - [ - activeChatId, - chats, - setActiveChatId, - onInvalidateChatList, - onInvalidateChatMessages, - ], + [chats, setActiveChatId, onInvalidateChatList, onInvalidateChatMessages], ); // FALLBACK resolver. Armed only by onTurnFinished when a brand-new chat's first @@ -136,7 +159,7 @@ export function useChatSession(params: { useEffect(() => { const before = pendingNewChatRef.current; if (before === null || activeChatId !== null) return; // not armed / already adopted - const after = chats?.items?.map((c) => c.id) ?? []; + const after = chatIdSnapshot(chats); const added = newlyAddedChatIds(before, after); // Keep waiting until a genuinely-new id appears. Set-based, so it is robust // to an add+delete in the same window (a length compare would miss it), and -- 2.49.1