fix(ai-chat): prevent duplicate chat row on first-turn error; add adoption tests

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 <noreply@anthropic.com>

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<string | null>(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<string[] | null>(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

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).

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();
+  });
+});

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;
+}