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

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(

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

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

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"]);

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