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,