From d14cda778eb67989932ff73a46cd59825efb2f04 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jos=C3=A9=20Almeida?= <57680069+zeval@users.noreply.github.com> Date: Thu, 21 May 2026 22:14:37 +0100 Subject: [PATCH] fix(web): close clarification overlay even when WS event is lost (#997) --- .../session/use-clarification-group.test.ts | 164 ++++++++++++++++++ .../session/use-clarification-group.ts | 64 ++++++- 2 files changed, 224 insertions(+), 4 deletions(-) diff --git a/apps/web/hooks/domains/session/use-clarification-group.test.ts b/apps/web/hooks/domains/session/use-clarification-group.test.ts index b45829fae1..ad1a4fe434 100644 --- a/apps/web/hooks/domains/session/use-clarification-group.test.ts +++ b/apps/web/hooks/domains/session/use-clarification-group.test.ts @@ -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((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); diff --git a/apps/web/hooks/domains/session/use-clarification-group.ts b/apps/web/hooks/domains/session/use-clarification-group.ts index 46efc4c358..65093c2241 100644 --- a/apps/web/hooks/domains/session/use-clarification-group.ts +++ b/apps/web/hooks/domains/session/use-clarification-group.ts @@ -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, + 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, + 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 { 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>({}); 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([]); + submitBundleRef.current = messages ?? []; + const submitCollected = useCallback( async (override?: Record) => { 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 {