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