diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c2aa9c9..a301635e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -92,6 +92,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 no longer froze on the previous step's authoritative usage; the current step's estimate is combined per-component with `max`, so the count rises smoothly and never jumps backwards. (#163) +- **AI chat: "New chat" pressed during the first turn's stream now resets the + thread instead of leaving the old turn streaming.** While a brand-new, + not-yet-adopted chat streamed its first turn, hitting "New chat" left + `activeChatId === null` (a no-op for the atom), so the reconciler never + remounted and the in-flight thread kept streaming behind the fresh one — and a + late refetch / late `onFinish` from that abandoned thread could yank the user + back into the chat they just left. "New chat" now forces a fresh empty thread + unconditionally and the finished thread's mount key is checked so a late + callback from an abandoned thread no longer adopts or re-arms the fallback. + (#161) ## [0.93.0] - 2026-06-21 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 de0b9923..d37c3c54 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 @@ -195,6 +195,7 @@ export default function AiChatWindow() { waitingForHistory, onTurnFinished, onServerChatId, + startFreshThread, cancelPendingAdoption, } = useChatSession({ activeChatId, @@ -209,18 +210,26 @@ export default function AiChatWindow() { // 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. + // pressing "New chat" while already in a not-yet-adopted new chat leaves + // activeChatId === null (a no-op for the atom), so the reconciler never fires — + // startFreshThread forces the remount unconditionally (and disarms any armed + // error-path fallback) so a late refetch can't yank the user into a just-failed + // chat after they chose a fresh one (#161). const startNewChat = useCallback((): void => { - cancelPendingAdoption(); + // Force a fresh thread UNCONDITIONALLY. On a brand-new, not-yet-adopted chat + // (activeChatId === null) whose first turn is still streaming, setActiveChatId + // (null) is a no-op and the render-phase reconciler would not remount — leaving + // the streaming thread in place (#161). startFreshThread guarantees a clean + // remount and already disarms any armed error-path fallback (so a separate + // cancelPendingAdoption() call here is redundant); the abandoned thread's late + // finish is rejected by the threadKey guard in the session hook. + startFreshThread(); setActiveChatId(null); setHistoryOpen(false); setDraft(""); // Default the picker back to "Universal assistant" for the fresh chat. setSelectedRoleId(null); - }, [cancelPendingAdoption, setActiveChatId, setDraft, setSelectedRoleId]); + }, [startFreshThread, setActiveChatId, setDraft, setSelectedRoleId]); const selectChat = useCallback( (chatId: string): void => { @@ -633,6 +642,7 @@ export default function AiChatWindow() { onRolePicked={(role) => setSelectedRoleId(role.id)} assistantName={currentRole?.name} onTurnFinished={onTurnFinished} + threadKey={threadKey} onServerChatId={onServerChatId} onLiveTurnTokens={setLiveTurnTokens} /> diff --git a/apps/client/src/features/ai-chat/components/chat-thread.test.tsx b/apps/client/src/features/ai-chat/components/chat-thread.test.tsx new file mode 100644 index 00000000..d1c71bc3 --- /dev/null +++ b/apps/client/src/features/ai-chat/components/chat-thread.test.tsx @@ -0,0 +1,94 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { render, act } from "@testing-library/react"; +import { MantineProvider } from "@mantine/core"; + +// Capture the options ChatThread passes to useChat so the test can drive the +// hook's terminal callbacks (here: onError) directly, without a real stream. The +// box is created via vi.hoisted so the hoisted vi.mock factory below can close +// over it. +const { useChatBox } = vi.hoisted(() => ({ + useChatBox: { options: null as unknown as Record | null }, +})); + +// Mock the AI SDK hook: record the options and return an inert, ready store so +// ChatThread renders without any network/streaming machinery. +vi.mock("@ai-sdk/react", () => ({ + useChat: (options: Record) => { + useChatBox.options = options; + return { + messages: [], + sendMessage: vi.fn(), + status: "ready", + stop: vi.fn(), + error: null, + }; + }, +})); + +// Stub react-i18next so `t` returns the key (other component tests use the same +// pattern); ChatThread's rendered chrome is irrelevant to this wiring test. +vi.mock("react-i18next", () => ({ + useTranslation: () => ({ t: (key: string) => key }), +})); + +// Mock the heavy presentational children to trivial stubs — this test only +// exercises the onError → onTurnFinished wiring, not their rendering. +vi.mock("@/features/ai-chat/components/message-list.tsx", () => ({ + default: () => null, +})); +vi.mock("@/features/ai-chat/components/chat-input.tsx", () => ({ + default: () => null, +})); +vi.mock("@/features/ai-chat/components/role-cards.tsx", () => ({ + default: () => null, +})); +vi.mock("@/features/ai-chat/components/chat-error-alert.tsx", () => ({ + default: () => null, +})); +vi.mock("@/features/ai-chat/components/chat-stopped-notice.tsx", () => ({ + default: () => null, +})); + +import ChatThread from "./chat-thread"; + +// matchMedia (read by MantineProvider) is stubbed globally in vitest.setup.ts. + +describe("ChatThread onError wiring (#161)", () => { + beforeEach(() => { + useChatBox.options = null; + vi.clearAllMocks(); + }); + + it("onError calls onTurnFinished with (undefined, threadKey) so a late error from an abandoned thread is rejected", () => { + const onTurnFinished = vi.fn(); + // Silence the deliberate console.error ChatThread logs for devtools. + const consoleError = vi + .spyOn(console, "error") + .mockImplementation(() => {}); + + render( + + + , + ); + + const options = useChatBox.options; + expect(options).not.toBeNull(); + expect(typeof options?.onError).toBe("function"); + + // Drive the captured onError exactly as the AI SDK would on a stream error. + act(() => { + (options!.onError as (e: Error) => void)(new Error("stream blew up")); + }); + + // The thread's own mount key must be forwarded with NO server id, so the + // session hook can reject this finish if the thread has been abandoned. + expect(onTurnFinished).toHaveBeenCalledWith(undefined, "thread-key-1"); + + consoleError.mockRestore(); + }); +}); 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 c906a940..54418541 100644 --- a/apps/client/src/features/ai-chat/components/chat-thread.tsx +++ b/apps/client/src/features/ai-chat/components/chat-thread.tsx @@ -60,7 +60,12 @@ interface ChatThreadProps { * 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; + onTurnFinished: (serverChatId?: string, finishingThreadKey?: string) => void; + /** This thread's mount key (the same value the parent uses as React `key`). + * Forwarded back through onTurnFinished so the session hook can tell a finish + * from THIS still-mounted thread from a late finish of an abandoned thread the + * user already left via New chat / switch (#161). */ + threadKey?: string; /** Called EARLY (at the stream's `start` chunk) with the authoritative server * chat id streamed on the assistant message metadata, so a brand-new chat * adopts its real id WHILE the first turn is still streaming (#174 — makes the @@ -116,6 +121,7 @@ export default function ChatThread({ onRolePicked, assistantName, onTurnFinished, + threadKey, onServerChatId, onLiveTurnTokens, }: ChatThreadProps) { @@ -258,7 +264,7 @@ export default function ChatThread({ // 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)); + onTurnFinished(extractServerChatId(message), threadKey); // 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); @@ -279,7 +285,7 @@ export default function ChatThread({ // Surface the raw failure in the browser console (devtools) for debugging; // the UI separately shows a friendly classified banner (see errorView). console.error("AI chat stream error:", streamError); - onTurnFinished(); + onTurnFinished(undefined, threadKey); }, }); 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 0080cc80..164f3976 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,5 +1,5 @@ import { describe, it, expect, vi, beforeEach } from "vitest"; -import { renderHook } from "@testing-library/react"; +import { renderHook, act } from "@testing-library/react"; import { useChatSession } from "./use-chat-session"; import type { UseChatSessionOptions } from "./use-chat-session"; @@ -120,16 +120,40 @@ 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. + it("cancelPendingAdoption (selectChat) disarms a late refetch from adopting the just-failed chat", () => { + // cancelPendingAdoption is the explicit disarm the window calls from + // selectChat: switching to a chat whose id == null is a no-op for the atom, so + // the render-phase reconciler never fires and only this call disarms an armed + // error-path fallback. (startNewChat no longer routes through here — it calls + // startFreshThread, covered by the next test — but cancelPendingAdoption still + // backs selectChat, so this guard must hold.) 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 + result.current.cancelPendingAdoption(); // window calls this from selectChat + // 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("#161: startFreshThread disarms the armed error-path fallback (New chat during the first turn)", () => { + // Pressing "New chat" while already in a not-yet-adopted new chat keeps + // activeChatId === null, so the render-phase reconciler never fires. The + // window now calls startFreshThread() (NOT cancelPendingAdoption) to force a + // fresh thread; this test pins the load-bearing fact that startFreshThread + // ALSO nulls pendingNewChatRef, so a late refetch of the just-failed row can't + // yank the user back into the abandoned chat. + const { result, rerender, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "x" }] }, + }); + result.current.onTurnFinished(undefined); // first turn failed → arm (before=["x"]) + act(() => result.current.startFreshThread()); // "New chat" → fresh thread + disarm // The just-failed row lands in a late refetch; it must NOT be adopted. rerender({ activeChatId: null, @@ -227,6 +251,48 @@ describe("useChatSession", () => { expect(result.current.threadKey).toBe("C"); }); + it("#161: startFreshThread remounts even when activeChatId stays null (New chat mid-stream)", () => { + // The bug: pressing New chat while still on a brand-new, not-yet-adopted chat + // leaves activeChatId === null, so the render-phase reconciler never fires. + // startFreshThread must remount UNCONDITIONALLY (a new mount key). + const { result } = setup({ activeChatId: null, chats: { items: [] } }); + const keyBefore = result.current.threadKey; + act(() => result.current.startFreshThread()); + expect(result.current.threadKey).not.toBe(keyBefore); + }); + + it("#161: a late finish from an ABANDONED thread does not adopt or invalidate messages", () => { + const { + result, + setActiveChatId, + onInvalidateChatList, + onInvalidateChatMessages, + } = setup({ activeChatId: null, chats: { items: [{ id: "x" }] } }); + const abandonedKey = result.current.threadKey; + // User pressed New chat mid-stream → fresh thread (new mount key). + act(() => result.current.startFreshThread()); + expect(result.current.threadKey).not.toBe(abandonedKey); + // The left-behind thread's onFinish fires late, carrying ITS (now stale) key + // and the server id of the chat the user just left. It must NOT be adopted. + act(() => result.current.onTurnFinished("A", abandonedKey)); + expect(setActiveChatId).not.toHaveBeenCalled(); + expect(onInvalidateChatMessages).not.toHaveBeenCalled(); + // The abandoned chat should still surface in the history list. + expect(onInvalidateChatList).toHaveBeenCalled(); + }); + + it("#161: a finish from the CURRENT thread (matching key) still adopts", () => { + const { result, setActiveChatId } = setup({ + activeChatId: null, + chats: { items: [{ id: "x" }] }, + }); + // Same thread that is mounted reports its finish with the matching key. + act(() => + result.current.onTurnFinished("A", result.current.threadKey), + ); + expect(setActiveChatId).toHaveBeenCalledWith("A"); + }); + 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({ 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 d21ebd11..83cc14ad 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 @@ -32,8 +32,13 @@ export interface UseChatSessionResult { /** 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; + * (undefined on a failed turn). `finishingThreadKey` is the mount key of the + * thread that produced this callback — when it no longer matches the mounted + * thread (the user pressed New chat / switched mid-stream), the call is from an + * abandoned thread and must NOT adopt or re-arm the fallback. Omitting it (old + * callers / tests) treats the call as belonging to the current thread. Handles + * new-chat id adoption + invalidations. */ + onTurnFinished: (serverChatId?: string, finishingThreadKey?: string) => void; /** Call EARLY (at the stream's `start` chunk) with the authoritative streamed * chat id so a brand-new chat adopts its real id WHILE its first turn is still * streaming — making `activeChatId`-gated affordances (e.g. the Copy/export @@ -41,6 +46,13 @@ export interface UseChatSessionResult { * no list/messages invalidation — that is left to onTurnFinished at the end). * Idempotent and a no-op once the chat already has an id. */ onServerChatId: (serverChatId?: string) => void; + /** Force a brand-new, empty thread (new mount key, no chat id) UNCONDITIONALLY. + * The render-phase reconciler only remounts when `activeChatId` actually + * changes; pressing "New chat" while already in a not-yet-adopted new chat + * leaves `activeChatId === null` (a no-op for the atom), so the reconciler + * never fires and the stale streaming thread stays mounted (#161). The window + * calls this from startNewChat to guarantee a fresh thread regardless. */ + startFreshThread: () => 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. */ @@ -85,6 +97,14 @@ export function useChatSession( const activeChatIdRef = useRef(activeChatId); activeChatIdRef.current = activeChatId; + // Live mirror of the mounted thread's key, read by onTurnFinished to tell a + // current-thread finish from an ABANDONED one. ai@6's useChat does not abort + // its request on unmount, and its callbacks are proxied so onFinish/onError of + // a thread the user already left (via New chat / switch) still fire AFTER that + // thread unmounts. By then this ref holds the NEW thread's key, so comparing it + // to the key the finishing thread reports rejects the abandoned turn (#161). + const threadKeyRef = useRef(""); + // 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 @@ -98,6 +118,10 @@ export function useChatSession( : switchThread(activeChatId), ); + // Keep the live mirror pointed at the currently-mounted thread's key so a late + // onTurnFinished can be matched against it (see threadKeyRef above). + threadKeyRef.current = thread.key; + // 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 @@ -115,7 +139,21 @@ export function useChatSession( // 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) => { + (serverChatId?: string, finishingThreadKey?: string) => { + // Reject a finish from an ABANDONED thread. After the user pressed New chat + // (or switched chats) mid-stream, the left-behind thread's onFinish/onError + // still fire (ai@6 does not abort on unmount). Adopting/arming off that late + // callback would yank the user back into the chat they just left (#161). + // `undefined` (legacy callers/tests) is treated as the current thread. + const isCurrentThread = + finishingThreadKey === undefined || + finishingThreadKey === threadKeyRef.current; + if (!isCurrentThread) { + // Still surface the abandoned chat in the history list, but do NOT adopt, + // arm the fallback, or invalidate per-chat messages (no thread shows it). + onInvalidateChatList(); + return; + } // 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. @@ -258,11 +296,29 @@ export function useChatSession( pendingNewChatRef.current = null; }, []); + // Force a fresh, empty thread regardless of the current `activeChatId`. The + // render-phase reconciler only remounts on an activeChatId CHANGE, so "New chat" + // pressed while already in a not-yet-adopted new chat (activeChatId stays null) + // would otherwise leave the in-flight streaming thread mounted (#161). Dispatch + // `reconcile` to chatId:null with a brand-new key so React remounts ChatThread + // (a fresh useChat store). Disarm any armed fallback too. After this dispatch + // thread.chatId is null; the window also sets activeChatId to null, so the + // render-phase reconciler then finds them equal and does not double-remount. + const startFreshThread = useCallback(() => { + pendingNewChatRef.current = null; + dispatch({ + type: "reconcile", + chatId: null, + newKey: `new-${generateId()}`, + }); + }, []); + return { threadKey: thread.key, waitingForHistory, onTurnFinished, onServerChatId, + startFreshThread, cancelPendingAdoption, }; }