diff --git a/canvas/src/components/mobile/MobileInbox.tsx b/canvas/src/components/mobile/MobileInbox.tsx index 7676aea1..bd337fa8 100644 --- a/canvas/src/components/mobile/MobileInbox.tsx +++ b/canvas/src/components/mobile/MobileInbox.tsx @@ -33,6 +33,11 @@ export function MobileInbox({ dark }: { dark: boolean }) { const [items, setItems] = useState([]); const [loading, setLoading] = useState(true); const [acting, setActing] = useState(null); + // Audit F1: track backend fetch failure distinctly so the list can render + // a retry affordance instead of the silent "No pending approvals" empty + // state. A genuine backend outage during a destructive-approvals review + // must NOT look like a clean inbox. + const [loadError, setLoadError] = useState(null); // Responder identity — the same resolution RequestsInbox uses (session // user_id, "admin" placeholder when unauthenticated). @@ -49,22 +54,42 @@ export function MobileInbox({ dark }: { dark: boolean }) { // in-flight fetch for the old kind must not overwrite the list after the // new tab's load clears it (CR2 #11478). const loadSeqRef = useRef(0); + // Audit F3: track rows the user has just optimistically acted on. A + // REQUEST_RESPONDED WS event fired by OUR server (in response to our own + // POST) can race with the optimistic removal — the WS-triggered load() + // may re-fetch a list that still contains the row (the POST hasn't + // landed server-side yet), and a naive setItems would briefly re-render + // the just-approved row (the "flicker" report). Filtering the just-acted + // set on load() return closes the race; the set is cleared on POST + // completion (success drops it because the server no longer returns the + // row; failure drops it because the catch re-loads from server truth). + const justActedRef = useRef>(new Set()); const load = useCallback(() => { const seq = ++loadSeqRef.current; // core#2766 / CR2 #11478: clear stale rows the moment the kind changes // so the user never sees approval cards under the Tasks tab (or // vice-versa) while the new list is still fetching. setItems([]); + setLoadError(null); setLoading(true); api .get(`/requests/pending?kind=${kind}`) .then((rows) => { if (seq !== loadSeqRef.current) return; // stale response - setItems(Array.isArray(rows) ? rows : []); + const arr = Array.isArray(rows) ? rows : []; + // F3: filter out rows the user has just acted on so a racing + // WS-triggered load doesn't re-surface them in the brief window + // between optimistic-removal and POST completion. + setItems(arr.filter((r) => !justActedRef.current.has(r.id))); }) .catch(() => { if (seq !== loadSeqRef.current) return; // stale response setItems([]); + // F1: surface the failure distinctly. The render path below + // shows a retry affordance + role="alert" instead of the + // "No pending approvals" copy that would falsely suggest a + // clean inbox during a backend outage. + setLoadError("Could not load pending requests."); }) .finally(() => { if (seq !== loadSeqRef.current) return; // stale response @@ -82,8 +107,11 @@ export function MobileInbox({ dark }: { dark: boolean }) { async (r: RequestRow, action: "done" | "rejected" | "approved") => { if (acting) return; setActing(r.id); - // Optimistic: drop the row immediately; restore on failure. - const prev = items; + // Optimistic: drop the row immediately. The server is the source of + // truth, so on failure we re-load() rather than restoring a stale + // `items` snapshot (Audit F2: a `setItems(prev)` snapshot would wipe + // any rows that arrived via WS during the in-flight POST). + justActedRef.current.add(r.id); setItems((cur) => cur.filter((x) => x.id !== r.id)); try { await api.post(`/requests/${r.id}/respond`, { @@ -91,13 +119,22 @@ export function MobileInbox({ dark }: { dark: boolean }) { responder_type: "user", responder_id: responderIdRef.current, }); + // F3: POST succeeded — the server now treats r as responded, so + // future loads won't return it. Drop from justActedRef. + justActedRef.current.delete(r.id); } catch { - setItems(prev); // restore on failure + // F2: re-fetch from server truth (preserves any rows that arrived + // via WS during the in-flight POST — restoring a stale `items` + // snapshot here would wipe them). Also drops the row from + // justActedRef so the server's re-fetched list (with the row + // still pending) is rendered as-is. + justActedRef.current.delete(r.id); + load(); } finally { setActing(null); } }, - [acting, items], + [acting, load], ); const subTabs: { id: InboxKind; label: string }[] = useMemo( @@ -154,6 +191,44 @@ export function MobileInbox({ dark }: { dark: boolean }) {
{loading && items.length === 0 ? (
Loading…
+ ) : loadError && items.length === 0 ? ( + // F1: backend fetch failure renders a distinct error/retry state + // (NOT the silent "No pending approvals" empty copy). Critical for + // destructive-approvals review — a clean-looking inbox during a + // backend outage would let destructive actions go unreviewed. +
+
{loadError}
+ +
) : items.length === 0 ? (
{kind === "approval" diff --git a/canvas/src/components/mobile/__tests__/MobileInbox.test.tsx b/canvas/src/components/mobile/__tests__/MobileInbox.test.tsx index 8eb5152d..c27f88b9 100644 --- a/canvas/src/components/mobile/__tests__/MobileInbox.test.tsx +++ b/canvas/src/components/mobile/__tests__/MobileInbox.test.tsx @@ -15,6 +15,7 @@ vi.mock("@/lib/auth", () => ({ fetchSession: vi.fn().mockResolvedValue({ user_id vi.mock("@/hooks/useSocketEvent", () => ({ useSocketEvent: vi.fn() })); import { api } from "@/lib/api"; +import { useSocketEvent } from "@/hooks/useSocketEvent"; import type { RequestRow } from "@/components/concierge/RequestsInbox"; import { MobileInbox } from "../MobileInbox"; @@ -105,4 +106,129 @@ describe("MobileInbox", () => { expect(getByText(/No pending tasks/i)).toBeTruthy(); }); }); + + // ───────────────────────────────────────────────────────────────────── + // Audit F1 (HIGH): MobileInbox.tsx:155-162 — backend fetch FAILURE must + // render a distinct error/retry state, NOT the silent "No pending + // approvals" empty copy. Critical for destructive-approvals review: + // a clean-looking inbox during a backend outage would let destructive + // actions go unreviewed. + // ───────────────────────────────────────────────────────────────────── + it("F1: backend fetch failure renders error/retry, not silent 'No pending approvals'", async () => { + vi.mocked(api.get).mockRejectedValue(new Error("network down")); + const { getByTestId, queryByText } = render(); + + // The error state + retry button must render. + await waitFor(() => { + expect(getByTestId("inbox-load-error")).toBeTruthy(); + }); + expect(getByTestId("inbox-retry")).toBeTruthy(); + + // The silent "No pending approvals" copy MUST NOT render during a + // backend fetch failure — that's the bug F1 fixes. + expect(queryByText(/No pending approvals/i)).toBeNull(); + + // The retry button must call load() — second GET also rejects, but + // we assert the click is wired (no throw, error state persists). + await act(async () => { + fireEvent.click(getByTestId("inbox-retry")); + }); + await waitFor(() => { + expect(getByTestId("inbox-load-error")).toBeTruthy(); + }); + }); + + // ───────────────────────────────────────────────────────────────────── + // Audit F2 (HIGH): MobileInbox.tsx:81-101 — `respond` prev-closure must + // NOT wipe a row that arrived via WS during the in-flight POST on + // failure. The server is the source of truth, so the fix re-loads from + // the server rather than restoring a stale `items` snapshot. + // ───────────────────────────────────────────────────────────────────── + it("F2: POST failure re-loads from server (preserves a row that arrived via WS mid-POST)", async () => { + const rowA: RequestRow = { ...approval, id: "req-A", title: "Row A: delete prod secret?" }; + const rowB: RequestRow = { ...approval, id: "req-B", title: "Row B: rotate prod key?" }; + + // First GET (initial load): server returns [A]. The user clicks + // Approve on A; the POST fails. The post-failure re-load (the + // second GET) simulates the server returning [A, B] — A is still + // pending (POST failed), and B arrived via WS during the in-flight + // POST. The OLD snapshot-restore would wipe B; the new load() + // re-fetch must preserve it. + let getCallCount = 0; + vi.mocked(api.get).mockImplementation(() => { + getCallCount++; + if (getCallCount === 1) return Promise.resolve([rowA]); + return Promise.resolve([rowA, rowB]); + }); + vi.mocked(api.post).mockRejectedValue(new Error("server down")); + + const { getByText } = render(); + await waitFor(() => expect(getByText("Row A: delete prod secret?")).toBeTruthy()); + + // User clicks Approve on A → optimistic removal + POST in flight. + await act(async () => { + fireEvent.click(getByText("Approve")); + }); + + // After the post-failure re-load, BOTH A (server still has it) AND + // B (WS-arrived during the in-flight POST) must be visible. B + // surviving is the load-bearing assertion for F2. + await waitFor(() => { + expect(getByText("Row B: rotate prod key?")).toBeTruthy(); + }); + expect(getByText("Row A: delete prod secret?")).toBeTruthy(); + }); + + // ───────────────────────────────────────────────────────────────────── + // Audit F3 (HIGH): MobileInbox.tsx:77-79 — WS REQUEST_RESPONDED fired + // during the optimistic-removal / in-flight-POST window must NOT cause + // the just-acted row to briefly reappear (flicker). The fix filters + // justActedRef out of the load() response. + // ───────────────────────────────────────────────────────────────────── + it("F3: WS REQUEST_RESPONDED during optimistic removal does not flicker the row", async () => { + // Capture the useSocketEvent callback so we can fire a synthetic + // WS event from the test. + let socketCb: ((msg: { event?: string }) => void) | null = null; + vi.mocked(useSocketEvent).mockImplementation((cb) => { + socketCb = cb as typeof socketCb; + }); + + const rowA: RequestRow = { ...approval, id: "req-A", title: "Row A: delete prod secret?" }; + vi.mocked(api.get).mockResolvedValue([rowA]); + // Hold the POST open so the WS race has a window to fire in. + const postDeferred = deferred(); + vi.mocked(api.post).mockReturnValue(postDeferred.promise as Promise); + + const { getByText, queryByText } = render(); + await waitFor(() => expect(getByText("Row A: delete prod secret?")).toBeTruthy()); + + // User clicks Approve → optimistic removal; POST is in flight. + await act(async () => { + fireEvent.click(getByText("Approve")); + }); + await waitFor(() => expect(queryByText("Row A: delete prod secret?")).toBeNull()); + + // Race: a WS REQUEST_RESPONDED fires before the POST lands. The + // load() triggered by it must re-fetch (the server still has A as + // pending because the POST hasn't returned). The OLD code would + // re-surface A → flicker. The NEW code filters justActedRef so A + // stays gone. + vi.mocked(api.get).mockResolvedValueOnce([rowA]); + await act(async () => { + socketCb?.({ event: "REQUEST_RESPONDED" }); + }); + // Give the re-fetch + setItems microtask queue a tick to settle. + await act(async () => { + await Promise.resolve(); + }); + + // Critical: A must NOT have reappeared. Flicker prevented. + expect(queryByText("Row A: delete prod secret?")).toBeNull(); + + // Now resolve the POST (success) — A is permanently responded. + await act(async () => { + postDeferred.resolve({}); + }); + expect(queryByText("Row A: delete prod secret?")).toBeNull(); + }); });