CLUE-731: AI chats should time out

src/components/chat-tutor/chat-highlights.test.tsx

@@ -13,7 +13,7 @@ const turn: ChatTurn = {
 };
 
 const chatResult = (turns: ChatTurn[]): UseChatResult => ({
-  turns, status: "idle", error: null, pending: false,
+  turns, status: "idle", error: null, pending: false, timedOut: false,
   sendMessage: jest.fn(async () => undefined), header: "Doc · 1.1"
 });
 

src/components/chat-tutor/chat-sidebar.tsx

@@ -58,15 +58,22 @@ export const ChatTutorSidebar: React.FC<IProps> = observer((props) => {
   // path. Rebuilding on documentKey/problemPath change is the hard conversation swap;
   // the unit's authored prompt overrides and the effective unit-summary slice are both
   // mixed into the conversation id so a change to either starts a fresh conversation.
-  const transport: ChatTransport = useMemo(() => {
+  // The conversation id comes back with the transport because it is derived from values that only
+  // exist in here. It travels into the failure logs, where it joins a log row to the conversation
+  // a report of one stuck chat points at. The debug transport has none.
+  const { transport, conversationId } = useMemo((): {
+    transport: ChatTransport; conversationId?: string;
+  } => {
     const getLeftContext = () => problemSectionsLoaded(problem) ? buildLeftContext(problem) : undefined;
     // Rides the same install-eligible sends as LEFT (see unit-context.ts). Computed fresh per
     // transport, same as getLeftContext -- unit.config is authored data, not expected to change
     // without a reload.
     const getUnitContext = () => buildUnitContext(unit, problem);
     const tutorPrompts = normalizeTutorPrompts(appConfig.chatTutorPrompts);
     if (urlParams.chatDebug) {
-      return new DebugTransport({ getLeftContext, getUnitContext, getRightSummary, tutorPrompts });
+      return {
+        transport: new DebugTransport({ getLeftContext, getUnitContext, getRightSummary, tutorPrompts })
+      };
     }
     const promptsKey = tutorPrompts && tutorPromptsKey(tutorPrompts);
     // Resolved once per transport. Undefined for the default provider, which is what keeps it
@@ -78,11 +85,12 @@ export const ChatTutorSidebar: React.FC<IProps> = observer((props) => {
     // never fork on it either.
     const unitContext = providerInstallsProblem(provider) ? getUnitContext() : undefined;
     const unitKey = unitContext ? hashString(unitContext) : undefined;
-    return new FirestoreTransport({
+    const docId = conversationDocId(
+      user.id, documentKey, user.network, problemPath, promptsKey, provider, unitKey
+    );
+    return { conversationId: docId, transport: new FirestoreTransport({
       firestore: db.firestore,
-      conversationId: conversationDocId(
-        user.id, documentKey, user.network, problemPath, promptsKey, provider, unitKey
-      ),
+      conversationId: docId,
       uid: user.id,
       contextId: user.classHash,
       problemPath,
@@ -95,13 +103,17 @@ export const ChatTutorSidebar: React.FC<IProps> = observer((props) => {
       getRightContent: () => content ? serializeRight(content) : undefined,
       tutorPrompts,
       provider,
-    });
+    }) };
   }, [documentKey, problemPath, problem, getRightSummary, content, appConfig, db, user, unit]);
 
+  const logContext = useMemo(
+    () => conversationId ? { conversationId, documentKey } : undefined,
+    [conversationId, documentKey]);
+
   // The drawer header makes the conversation scope legible: this conversation is bound
   // to one workspace document within one problem, and swaps when either changes.
   const header = `${documentTitle} · ${problemPath}`;
-  const chat = useChat({ transport, header });
+  const chat = useChat({ transport, header, logContext });
 
   // Display-only persona intro (never part of the AI context). An unset value uses the built-in
   // default; an authored empty string suppresses the intro entirely (?? keeps "" distinct from unset).

src/components/chat-tutor/chat-timeout.test.tsx

@@ -0,0 +1,90 @@
+import React from "react";
+import { act, fireEvent, render, screen } from "@testing-library/react";
+import { Chat } from "./chat";
+import { ChatTurn } from "./transport";
+import { kReplyDeadlineMs, kTimeoutMessage, UseChatResult } from "./use-chat";
+
+const chatResult = (over: Partial<UseChatResult> = {}): UseChatResult => ({
+  turns: [], status: "idle", error: null, pending: false, timedOut: false,
+  sendMessage: jest.fn(async () => undefined), header: "Doc · 1.1", ...over
+});
+
+const reply: ChatTurn = { id: "a1", sender: "assistant", text: "Try the sensor block." };
+
+describe("Chat reply timeout", () => {
+  it("shows the timeout message when the deadline has passed", () => {
+    render(<Chat chat={chatResult({ timedOut: true })} />);
+    expect(screen.getByTestId("chat-error")).toHaveTextContent(kTimeoutMessage);
+  });
+
+  // A real send failure must not be masked by the deadline that follows it.
+  it("prefers a send error over the timeout message", () => {
+    render(<Chat chat={chatResult({ timedOut: true, error: "Failed to send your message." })} />);
+    expect(screen.getByTestId("chat-error")).toHaveTextContent("Failed to send your message.");
+  });
+
+  // The live region keys on `pending` going false to announce a completed reply, and the deadline
+  // flips `pending` false too — so without a guard a timeout announces the PREVIOUS reply as if it
+  // had just arrived, telling a screen-reader user Ada answered at the moment Ada failed to.
+  it("does not announce the previous reply when the deadline fires", () => {
+    render(<Chat chat={chatResult({ timedOut: true, turns: [reply] })} />);
+    expect(screen.getByTestId("chat-live")).toHaveTextContent("");
+  });
+
+  it("announces a reply normally when nothing has timed out", () => {
+    render(<Chat chat={chatResult({ turns: [reply] })} />);
+    expect(screen.getByTestId("chat-live")).toHaveTextContent("Ada said: Try the sensor block.");
+  });
+
+  it("shows no message row when there is neither an error nor a timeout", () => {
+    render(<Chat chat={chatResult()} />);
+    expect(screen.queryByTestId("chat-error")).not.toBeInTheDocument();
+  });
+});
+
+// Firestore resolves add() only once the backend acknowledges it, so an offline write stays
+// pending indefinitely. The deadline therefore has to bound the composer's own sending state, not
+// just the wait for a reply, or the dots run on past it with the input disabled.
+describe("Chat with a send that never resolves", () => {
+  beforeEach(() => jest.useFakeTimers());
+  afterEach(() => jest.useRealTimers());
+
+  it("releases the composer and stops the dots on the deadline", () => {
+    const chat = chatResult({ sendMessage: jest.fn(() => new Promise<void>(() => undefined)) });
+    render(<Chat chat={chat} />);
+
+    fireEvent.change(screen.getByTestId("chat-input"), { target: { value: "how do I start?" } });
+    fireEvent.submit(screen.getByTestId("chat-composer"));
+
+    expect(screen.getByTestId("chat-typing")).toBeInTheDocument();
+    expect(screen.getByTestId("chat-input")).toBeDisabled();
+
+    act(() => { jest.advanceTimersByTime(kReplyDeadlineMs); });
+
+    expect(screen.queryByTestId("chat-typing")).not.toBeInTheDocument();
+    expect(screen.getByTestId("chat-input")).not.toBeDisabled();
+  });
+
+  // Once the composer is released the student can send again, and the first send may still settle.
+  it("does not let a superseded send re-disable the composer or overwrite the new draft", () => {
+    let failFirst: (e: Error) => void = () => undefined;
+    const sendMessage = jest.fn()
+      .mockImplementationOnce(() => new Promise<void>((_r, rej) => { failFirst = rej; }))
+      .mockImplementationOnce(() => new Promise<void>(() => undefined));
+    render(<Chat chat={chatResult({ sendMessage })} />);
+    const input = () => screen.getByTestId("chat-input") as HTMLInputElement;
+
+    fireEvent.change(input(), { target: { value: "first message" } });
+    fireEvent.submit(screen.getByTestId("chat-composer"));
+    act(() => { jest.advanceTimersByTime(kReplyDeadlineMs); });
+
+    fireEvent.change(input(), { target: { value: "second message" } });
+    fireEvent.submit(screen.getByTestId("chat-composer"));
+    expect(input()).toBeDisabled();
+
+    act(() => { failFirst(new Error("offline")); });
+
+    expect(input()).toBeDisabled();          // the second send is still in flight
+    expect(input().value).toBe("");          // and the student's composer is not stuffed with the first
+  });
+});

src/components/chat-tutor/chat.tsx

@@ -1,7 +1,7 @@
 import React, { FormEvent, useEffect, useMemo, useRef, useState } from "react";
 import classNames from "classnames";
 import { v4 as uuidv4 } from "uuid";
-import { UseChatResult } from "./use-chat";
+import { kReplyDeadlineMs, kTimeoutMessage, UseChatResult } from "./use-chat";
 import { ChatTurn } from "./transport";
 
 import "./chat.scss";
@@ -119,9 +119,13 @@ const DebugTurn: React.FC<{ turn: ChatTurn }> = ({ turn }) => {
 
 export const Chat: React.FC<IProps> = ({ chat, onClose, closeLabel, transcriptTitle, introText,
     onHighlightHover, onHighlightToggle, activeHighlightKey, enableHighlights = false }) => {
-  const { turns, error, pending, sendMessage, header } = chat;
+  const { turns, error, pending, timedOut, sendMessage, header } = chat;
   const [input, setInput] = useState("");
   const [sending, setSending] = useState(false);
+  // Identifies the newest submit. Once the release timer below frees the composer, an earlier send
+  // can still settle: without this its finally would re-enable the composer mid-send, and its
+  // catch would overwrite whatever the student has since typed with the old message.
+  const sendGenRef = useRef(0);
   const [copied, setCopied] = useState(false);
   const listRef = useRef<HTMLDivElement>(null);
   const inputRef = useRef<HTMLInputElement>(null);
@@ -224,12 +228,21 @@ export const Chat: React.FC<IProps> = ({ chat, onClose, closeLabel, transcriptTi
     if (!text || sending) return;
     setSending(true);
     setInput("");
+    // An offline add() stays pending indefinitely, so awaiting it for the UI would hold the
+    // composer disabled past the deadline meant to end exactly that. This bounds the UI only: the
+    // write keeps running and still reconciles if it lands.
+    const gen = ++sendGenRef.current;
+    const isNewest = () => gen === sendGenRef.current;
+    const release = setTimeout(() => { if (isNewest()) setSending(false); }, kReplyDeadlineMs);
     try {
       await sendMessage(text);
     } catch {
-      setInput(text); // restore so the student can retry (error surfaced by the hook)
+      // Restore so the student can retry (the error itself is surfaced by the hook). Never over
+      // text they have typed since: a superseded send's message is not what they are writing now.
+      if (isNewest()) setInput(prev => prev || text);
     } finally {
-      setSending(false);
+      clearTimeout(release);
+      if (isNewest()) setSending(false);
     }
   };
 
@@ -338,7 +351,7 @@ export const Chat: React.FC<IProps> = ({ chat, onClose, closeLabel, transcriptTi
 
       {/* single polite live region announcing the completed assistant reply */}
       <div className="visually-hidden" aria-live="polite" data-testid="chat-live">
-        {!pending && lastAssistantText ? `Ada said: ${lastAssistantText}` : ""}
+        {!pending && !timedOut && lastAssistantText ? `Ada said: ${lastAssistantText}` : ""}
       </div>
 
       {/* Pinning a highlight moves a ring onto an object elsewhere in the document, where nothing
@@ -360,7 +373,8 @@ export const Chat: React.FC<IProps> = ({ chat, onClose, closeLabel, transcriptTi
           {activeHighlightLabel ? `Highlighting ${activeHighlightLabel}` : ""}
         </div>}
 
-      {error && <div className="chat-error" role="alert" data-testid="chat-error">{error}</div>}
+      {(error || timedOut) &&
+        <div className="chat-error" role="alert" data-testid="chat-error">{error ?? kTimeoutMessage}</div>}
 
       <form className="chat-composer" onSubmit={onSubmit} data-testid="chat-composer">
         <label htmlFor={inputId} className="visually-hidden">Message Ada</label>

src/components/chat-tutor/firestore-transport.test.ts

@@ -285,3 +285,165 @@ describe("FirestoreTransport measures the workspace in bytes", () => {
     expect(added[0]).not.toHaveProperty("rightContent");
   });
 });
+
+
+// The composer stays open while a reply is pending, so two sends can be in flight at once.
+// `lastSentRightHash` means "acknowledged by the server", recorded in send order.
+describe("FirestoreTransport workspace bookkeeping under overlapping sends", () => {
+  // Unlike fakeFirestore above, each add() hands back a promise the test settles by hand, which is
+  // the only way to express one send resolving while another is still open.
+  function deferredFirestore() {
+    const added: Record<string, unknown>[] = [];
+    const settle: Array<{ resolve: () => void; reject: (e: unknown) => void }> = [];
+    const messages = {
+      add: (doc: Record<string, unknown>) => {
+        added.push(doc);
+        return new Promise<void>((resolve, reject) => settle.push({ resolve, reject }));
+      }
+    };
+    const firestore = { collection: () => ({ doc: () => ({ collection: () => messages }) }) };
+    return { added, settle, firestore: firestore as any };
+  }
+
+  function transportWithMutableWorkspace() {
+    const { added, settle, firestore } = deferredFirestore();
+    let summary = { markdown: "W1", hash: "H1" };
+    const transport = new FirestoreTransport({
+      firestore, conversationId: "conv1", uid: "123", contextId: "class1",
+      problemPath: "sas/1/2", getLeftContext: () => "{}",
+      getRightSummary: () => summary,
+    });
+    const edit = (markdown: string, hash: string) => { summary = { markdown, hash }; };
+    return { added, settle, transport, edit };
+  }
+
+  it("records the hash in send order, not completion order", async () => {
+    const { added, settle, transport, edit } = transportWithMutableWorkspace();
+
+    const first = transport.sendUserMessage("one");
+    edit("W2", "H2");
+    const second = transport.sendUserMessage("two");
+
+    // B's write lands before A's, which is what put the client's record behind the server's.
+    settle[1].resolve();
+    settle[0].resolve();
+    await Promise.all([first, second]);
+
+    const third = transport.sendUserMessage("three");
+    settle[2].resolve();
+    await third;
+
+    expect(added[1].rightContext).toBe("W2");
+    expect(added[2]).not.toHaveProperty("rightContext");
+  });
+
+  it("attaches again when the write that carried the workspace failed", async () => {
+    const { added, settle, transport } = transportWithMutableWorkspace();
+
+    const first = transport.sendUserMessage("one");
+    settle[0].reject(new Error("offline"));
+    await expect(first).rejects.toThrow("offline");
+
+    const second = transport.sendUserMessage("two");
+    settle[1].resolve();
+    await second;
+
+    expect(added[1].rightContext).toBe("W1");
+  });
+
+  // An older send failing must not unsettle what a newer, successful one delivered.
+  it("keeps a newer send's acknowledged workspace when an older send fails", async () => {
+    const { added, settle, transport, edit } = transportWithMutableWorkspace();
+
+    const first = transport.sendUserMessage("one");
+    edit("W2", "H2");
+    const second = transport.sendUserMessage("two");
+
+    settle[1].resolve();
+    await second;
+    settle[0].reject(new Error("offline"));
+    await expect(first).rejects.toThrow("offline");
+
+    const third = transport.sendUserMessage("three");
+    settle[2].resolve();
+    await third;
+
+    expect(added[2]).not.toHaveProperty("rightContext");
+  });
+
+  // This send attached nothing, so its failure says nothing about what the server already has.
+  it("leaves the acknowledged hash in place when an unchanged send fails", async () => {
+    const { added, settle, transport } = transportWithMutableWorkspace();
+
+    const first = transport.sendUserMessage("one");
+    settle[0].resolve();
+    await first;
+
+    const second = transport.sendUserMessage("two"); // attaches nothing: workspace unchanged
+    settle[1].reject(new Error("offline"));
+    await expect(second).rejects.toThrow("offline");
+
+    const third = transport.sendUserMessage("three");
+    settle[2].resolve();
+    await third;
+
+    expect(added[0].rightContext).toBe("W1");
+    expect(added[2]).not.toHaveProperty("rightContext");
+  });
+
+  // Both sends failing means the student was offline; neither message landed. Restoring the
+  // earlier send's hash would claim a workspace the server never received, so the next send that
+  // DID land would carry no workspace and the tutor would answer blind to the student's work.
+  it("records nothing when two overlapping sends both fail", async () => {
+    const { added, settle, transport, edit } = transportWithMutableWorkspace();
+
+    const first = transport.sendUserMessage("one");          // carries W1
+    edit("W2", "H2");
+    const second = transport.sendUserMessage("two");         // carries W2
+
+    settle[0].reject(new Error("offline"));
+    await expect(first).rejects.toThrow("offline");
+    settle[1].reject(new Error("offline"));
+    await expect(second).rejects.toThrow("offline");
+
+    // The student undoes the edit, so the document is back at the workspace the first (also
+    // failed) send carried. Nothing was acknowledged, so this send must carry it again.
+    edit("W1", "H1");
+    const third = transport.sendUserMessage("three");
+    settle[2].resolve();
+    await third;
+
+    expect(added[2].rightContext).toBe("W1");
+  });
+
+  // The send that carries the workspace can fail while a later one that skipped it succeeds. If
+  // the hash were claimed before delivery, that later turn would reach the backend with no
+  // workspace at all and the tutor would answer it blind to the student's work.
+  it("still carries the workspace when the send that first attached it fails", async () => {
+    const { added, settle, transport } = transportWithMutableWorkspace();
+
+    const first = transport.sendUserMessage("one");   // attaches W1
+    const second = transport.sendUserMessage("two");  // document unchanged
+
+    settle[0].reject(new Error("offline"));
+    await expect(first).rejects.toThrow("offline");
+    settle[1].resolve();
+    await second;
+
+    expect(added[1].rightContext).toBe("W1");
+  });
+
+  // Nothing retroactively tells an in-flight turn about an edit: the payload is computed before
+  // the write. Pinned here because the guarantee is load-bearing and invisible from the call site.
+  it("does not change what a message carried when the document is edited mid-send", async () => {
+    const { added, settle, transport, edit } = transportWithMutableWorkspace();
+
+    const first = transport.sendUserMessage("one");
+    edit("W2", "H2");
+    settle[0].resolve();
+    await first;
+
+    expect(added).toHaveLength(1);
+    expect(added[0].rightContext).toBe("W1");
+  });
+});