fix(web): close clarification overlay even when WS event is lost (#997)

2026-05-21 22:14:37 +01:00
parent 76143f8159
commit d14cda778e
2 changed files with 224 additions and 4 deletions
@@ -6,6 +6,13 @@ vi.mock("@/lib/config", () => ({
  getBackendConfig: () => ({ apiBaseUrl: "https://api.test" }),
 }));

+const mockUpdateMessage = vi.fn();
+vi.mock("@/components/state-provider", () => ({
+  useAppStoreApi: () => ({
+    getState: () => ({ updateMessage: mockUpdateMessage }),
+  }),
+}));
+
 import { useClarificationGroup } from "./use-clarification-group";

 function clarMessage(opts: {
@@ -38,6 +45,7 @@ const fetchMock = vi.fn();

 function setupFetchMock() {
  fetchMock.mockReset();
+  mockUpdateMessage.mockReset();
  fetchMock.mockResolvedValue(new Response(null, { status: 200 }));
  globalThis.fetch = fetchMock as unknown as typeof globalThis.fetch;
 }
@@ -211,6 +219,162 @@ describe("useClarificationGroup — submit + skip", () => {
  });
 });

+// Silent WS dead-socket scenario: when the question has been hanging long enough
+// that the underlying WebSocket has gone half-dead (NAT timeout, browser throttle),
+// the answer submit still completes via HTTP but the backend's session.message.updated
+// broadcast never reaches the dead socket — so the overlay would stay stuck on the
+// pending bundle until the user refreshes. To stay robust against that we mark
+// each bundle message as answered/rejected in the store the moment the HTTP POST
+// resolves, mirroring the backend update the WS event would have delivered.
+describe("useClarificationGroup — optimistic store update on resolve", () => {
+  beforeEach(setupFetchMock);
+
+  it("submitCollected marks every bundle message as answered in the store", async () => {
+    const msgs = [
+      clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 2 }),
+      clarMessage({ id: "m2", pendingId: "p1", questionId: "q2", index: 1, total: 2 }),
+    ];
+    const { result } = renderHook(() => useClarificationGroup(msgs));
+
+    const answer1 = { question_id: "q1", selected_options: ["o1"] };
+    const answer2 = { question_id: "q2", custom_text: "free" };
+    await act(async () => {
+      result.current.recordAnswer("q1", answer1);
+      result.current.recordAnswer("q2", answer2);
+    });
+    await act(async () => {
+      await result.current.submitCollected();
+    });
+
+    expect(result.current.submitState).toBe("ok");
+    expect(mockUpdateMessage).toHaveBeenCalledTimes(2);
+    const firstCall = mockUpdateMessage.mock.calls[0][0];
+    const secondCall = mockUpdateMessage.mock.calls[1][0];
+    expect(firstCall.id).toBe("m1");
+    expect(firstCall.metadata.status).toBe("answered");
+    expect(firstCall.metadata.response).toEqual(answer1);
+    expect(firstCall.metadata.pending_id).toBe("p1");
+    expect(secondCall.id).toBe("m2");
+    expect(secondCall.metadata.status).toBe("answered");
+    expect(secondCall.metadata.response).toEqual(answer2);
+  });
+
+  it("skipAll marks every bundle message as rejected in the store", async () => {
+    const msgs = [
+      clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 2 }),
+      clarMessage({ id: "m2", pendingId: "p1", questionId: "q2", index: 1, total: 2 }),
+    ];
+    const { result } = renderHook(() => useClarificationGroup(msgs));
+
+    await act(async () => {
+      await result.current.skipAll("Too vague");
+    });
+
+    expect(result.current.submitState).toBe("ok");
+    expect(mockUpdateMessage).toHaveBeenCalledTimes(2);
+    const calls = mockUpdateMessage.mock.calls.map((c) => c[0]);
+    expect(calls.map((m) => m.id).sort()).toEqual(["m1", "m2"]);
+    for (const m of calls) {
+      expect(m.metadata.status).toBe("rejected");
+      expect(m.metadata.pending_id).toBe("p1");
+    }
+  });
+
+  it("submitCollected does NOT touch the store when the HTTP request fails", async () => {
+    fetchMock.mockResolvedValueOnce(new Response("nope", { status: 400 }));
+    const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })];
+    const { result } = renderHook(() => useClarificationGroup(msgs));
+
+    await act(async () => {
+      await result.current.submitCollected({
+        q1: { question_id: "q1", selected_options: ["o1"] },
+      });
+    });
+
+    expect(result.current.submitState).toBe("error");
+    expect(mockUpdateMessage).not.toHaveBeenCalled();
+  });
+});
+
+describe("useClarificationGroup — optimistic store update edge cases", () => {
+  beforeEach(setupFetchMock);
+
+  // Race guard (cubic P1): if the parent re-renders the hook with a different
+  // bundle after the POST is in flight (e.g. the next clarification has
+  // already streamed in), the optimistic update must still target the bundle
+  // that was *submitted* — not whatever the live messages prop points at when
+  // the await resolves. We capture a snapshot at submit time.
+  it("submitCollected applies the optimistic update to the submit-time bundle, not the latest one", async () => {
+    let resolveFetch: ((res: Response) => void) | null = null;
+    fetchMock.mockImplementationOnce(
+      () =>
+        new Promise<Response>((resolve) => {
+          resolveFetch = resolve;
+        }),
+    );
+
+    const initial = [
+      clarMessage({ id: "m-old", pendingId: "p-old", questionId: "q-old", index: 0, total: 1 }),
+    ];
+    const next = [
+      clarMessage({ id: "m-new", pendingId: "p-new", questionId: "q-new", index: 0, total: 1 }),
+    ];
+
+    const { result, rerender } = renderHook(({ msgs }) => useClarificationGroup(msgs), {
+      initialProps: { msgs: initial },
+    });
+
+    await act(async () => {
+      // Submit kicks off the POST; rerender swaps the bundle while in flight;
+      // resolveFetch unblocks the POST and the optimistic update runs.
+      const pending = result.current.submitCollected({
+        "q-old": { question_id: "q-old", selected_options: ["o1"] },
+      });
+      rerender({ msgs: next });
+      resolveFetch?.(new Response(null, { status: 200 }));
+      await pending;
+    });
+
+    expect(mockUpdateMessage).toHaveBeenCalledTimes(1);
+    expect(mockUpdateMessage.mock.calls[0][0].id).toBe("m-old");
+    expect(mockUpdateMessage.mock.calls[0][0].metadata.pending_id).toBe("p-old");
+  });
+
+  // Failure-isolation guard (greptile P1): the optimistic store update is a
+  // best-effort UI nicety — if it blows up (e.g. the store action is missing,
+  // immer throws on a frozen object), the HTTP submit still succeeded and the
+  // user must see submitState === "ok", not "error".
+  it("submitCollected stays 'ok' when the optimistic store update throws", async () => {
+    mockUpdateMessage.mockImplementationOnce(() => {
+      throw new Error("store update boom");
+    });
+    const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })];
+    const { result } = renderHook(() => useClarificationGroup(msgs));
+
+    await act(async () => {
+      await result.current.submitCollected({
+        q1: { question_id: "q1", selected_options: ["o1"] },
+      });
+    });
+
+    expect(result.current.submitState).toBe("ok");
+  });
+
+  it("skipAll stays 'ok' when the optimistic store update throws", async () => {
+    mockUpdateMessage.mockImplementationOnce(() => {
+      throw new Error("store update boom");
+    });
+    const msgs = [clarMessage({ id: "m1", pendingId: "p1", questionId: "q1", index: 0, total: 1 })];
+    const { result } = renderHook(() => useClarificationGroup(msgs));
+
+    await act(async () => {
+      await result.current.skipAll();
+    });
+
+    expect(result.current.submitState).toBe("ok");
+  });
+});
+
 describe("useClarificationGroup — inflight guard", () => {
  beforeEach(setupFetchMock);

@@ -3,6 +3,7 @@
 import { useCallback, useEffect, useMemo, useRef, useState } from "react";
 import type { ClarificationAnswer, ClarificationRequestMetadata, Message } from "@/lib/types/http";
 import { getBackendConfig } from "@/lib/config";
+import { useAppStoreApi } from "@/components/state-provider";

 type SubmitState = "idle" | "submitting" | "ok" | "error";

@@ -57,6 +58,44 @@ async function postClarificationBatch(
  return "error";
 }

+// Mark each bundle message as resolved so the overlay closes regardless of
+// whether the backend's WS confirmation event arrives. A long-idle tab can
+// leave the WebSocket half-dead (NAT/throttle); the HTTP POST still succeeds
+// but the session.message.updated broadcast never lands, which would otherwise
+// strand the carousel on "pending" until the user refreshes.
+function applyResolvedStatusToBundle(
+  bundle: readonly Message[],
+  status: "answered" | "rejected",
+  answersByQuestionId: Record<string, ClarificationAnswer>,
+  update: (message: Message) => void,
+) {
+  for (const msg of bundle) {
+    const meta = (msg.metadata ?? {}) as ClarificationRequestMetadata;
+    const questionId = meta.question_id ?? meta.question?.id ?? "";
+    const nextMeta: ClarificationRequestMetadata = { ...meta, status };
+    const matched = questionId ? answersByQuestionId[questionId] : undefined;
+    if (matched) nextMeta.response = matched;
+    update({ ...msg, metadata: nextMeta });
+  }
+}
+
+// Best-effort optimistic update — isolated from the submit's own try/catch so
+// a thrown store action (missing handler, immer freeze, etc.) can't downgrade
+// a successful HTTP submit to submitState === "error".
+function safeApplyResolvedStatus(
+  bundle: readonly Message[],
+  status: "answered" | "rejected",
+  answersByQuestionId: Record<string, ClarificationAnswer>,
+  update: (message: Message) => void,
+) {
+  if (bundle.length === 0) return;
+  try {
+    applyResolvedStatusToBundle(bundle, status, answersByQuestionId, update);
+  } catch (err) {
+    console.error("Clarification optimistic update threw:", err);
+  }
+}
+
 async function postClarificationSkip(pendingId: string, reason: string): Promise<SubmitState> {
  const { apiBaseUrl } = getBackendConfig();
  const res = await fetch(`${apiBaseUrl}/api/v1/clarification/${pendingId}/respond`, {
@@ -83,6 +122,7 @@ async function postClarificationSkip(pendingId: string, reason: string): Promise
 export function useClarificationGroup(
  messages: readonly Message[] | null | undefined,
 ): ClarificationGroupApi {
+  const storeApi = useAppStoreApi();
  const [answers, setAnswers] = useState<Record<string, ClarificationAnswer>>({});
  const answersRef = useRef(answers);
  useEffect(() => {
@@ -121,6 +161,12 @@ export function useClarificationGroup(
    });
  }, []);

+  // Snapshot the bundle at submit time so a re-render that swaps `messages`
+  // mid-flight (e.g. the next clarification streaming in) can't make the
+  // optimistic update target the wrong messages once the await resolves.
+  const submitBundleRef = useRef<readonly Message[]>([]);
+  submitBundleRef.current = messages ?? [];
+
  const submitCollected = useCallback(
    async (override?: Record<string, ClarificationAnswer>) => {
      if (!pendingId) return;
@@ -128,13 +174,18 @@ export function useClarificationGroup(
      const current = { ...answersRef.current, ...(override ?? {}) };
      const haveAll = questionIds.every((id) => Boolean(current[id]));
      if (!haveAll) return;
+      const bundle = submitBundleRef.current.slice();
      inflightRef.current = true;
      setSubmitState("submitting");
      const ordered = questionIds
        .map((id) => current[id])
        .filter((a): a is ClarificationAnswer => Boolean(a));
      try {
-        setSubmitState(await postClarificationBatch(pendingId, ordered));
+        const next = await postClarificationBatch(pendingId, ordered);
+        setSubmitState(next);
+        if (next === "ok") {
+          safeApplyResolvedStatus(bundle, "answered", current, storeApi.getState().updateMessage);
+        }
      } catch (err) {
        console.error("Clarification submit threw:", err);
        setSubmitState("error");
@@ -142,17 +193,22 @@ export function useClarificationGroup(
        inflightRef.current = false;
      }
    },
-    [pendingId, questionIds],
+    [pendingId, questionIds, storeApi],
  );

  const skipAll = useCallback(
    async (reason?: string) => {
      if (!pendingId) return;
      if (inflightRef.current) return;
+      const bundle = submitBundleRef.current.slice();
      inflightRef.current = true;
      setSubmitState("submitting");
      try {
-        setSubmitState(await postClarificationSkip(pendingId, reason ?? "User skipped"));
+        const next = await postClarificationSkip(pendingId, reason ?? "User skipped");
+        setSubmitState(next);
+        if (next === "ok") {
+          safeApplyResolvedStatus(bundle, "rejected", {}, storeApi.getState().updateMessage);
+        }
      } catch (err) {
        console.error("Clarification skip threw:", err);
        setSubmitState("error");
@@ -160,7 +216,7 @@ export function useClarificationGroup(
        inflightRef.current = false;
      }
    },
-    [pendingId],
+    [pendingId, storeApi],
  );

  return {