From f3e6ded5fedd1c678177d937271d4838f3895a5c Mon Sep 17 00:00:00 2001 From: jaylfc Date: Mon, 17 Aug 2026 03:12:31 +0000 Subject: [PATCH 1/2] DecisionsApp load(): guard state updates with request sequence to prevent stale overwrites - Add latestSeq ref that increments on each load() entry - Guard setPending, setAnswered, setAuthRequests, and setLoading(false) with seq === latestSeq.current so only the last-started load wins - Add test that holds the first load's fetches pending, lets a second load land with newer data, then releases the first load with older data and asserts the stale response did not overwrite --- changelog.d/tsk-ycfglg-decisions-load-race.md | 2 + desktop/src/apps/DecisionsApp.test.tsx | 64 +++++++++++++++++++ desktop/src/apps/DecisionsApp.tsx | 12 ++-- 3 files changed, 74 insertions(+), 4 deletions(-) create mode 100644 changelog.d/tsk-ycfglg-decisions-load-race.md diff --git a/changelog.d/tsk-ycfglg-decisions-load-race.md b/changelog.d/tsk-ycfglg-decisions-load-race.md new file mode 100644 index 000000000..eb1b6cc6e --- /dev/null +++ b/changelog.d/tsk-ycfglg-decisions-load-race.md @@ -0,0 +1,2 @@ +### Fixed +- DecisionsApp `load()` now guards each state update with a monotonically increasing request sequence, so a stale in-flight response can no longer overwrite newer data when mount, focus refresh, or SSE-driven reloads overlap diff --git a/desktop/src/apps/DecisionsApp.test.tsx b/desktop/src/apps/DecisionsApp.test.tsx index 5f5e68d36..a91461a66 100644 --- a/desktop/src/apps/DecisionsApp.test.tsx +++ b/desktop/src/apps/DecisionsApp.test.tsx @@ -433,4 +433,68 @@ describe("DecisionsApp", () => { await new Promise((r) => setTimeout(r, 0)); }); }); + + it("last-started load wins over an older stale response", async () => { + const olderPending = [singleSelect]; + const newerPending: Decision[] = []; + let callCount = 0; + const heldResolvers: Array<() => void> = []; + + const fetchMock = vi.fn().mockImplementation((input: string) => { + callCount++; + if (callCount <= 3) { + return new Promise((resolve) => { + heldResolvers.push(() => + resolve({ + ok: true, + status: 200, + json: () => { + if (input.includes("status=pending") && !input.includes("auth-requests")) + return Promise.resolve(olderPending); + if (input.includes("status=answered")) + return Promise.resolve([]); + return Promise.resolve({ requests: [] }); + }, + }), + ); + }); + } + return Promise.resolve({ + ok: true, + status: 200, + json: () => { + if (input.includes("status=pending") && !input.includes("auth-requests")) + return Promise.resolve(newerPending); + if (input.includes("status=answered")) + return Promise.resolve([]); + return Promise.resolve({ requests: [] }); + }, + }); + }); + + vi.stubGlobal("fetch", fetchMock); + render(); + + // Mount load A starts but its fetches are held pending. + await act(async () => { + await new Promise((r) => setTimeout(r, 50)); + }); + + // Focus refresh triggers load B after the 1 s debounce. + window.dispatchEvent(new Event("focus")); + + // Wait for load B to land. + await act(async () => { + await new Promise((r) => setTimeout(r, 1200)); + }); + + // Now release the held load A with older data. + await act(async () => { + heldResolvers.forEach((r) => r()); + await new Promise((r) => setTimeout(r, 0)); + }); + + // The rendered list must still reflect load B's newer data, not load A's stale data. + expect(screen.queryByText(singleSelect.question)).toBeNull(); + }); }); diff --git a/desktop/src/apps/DecisionsApp.tsx b/desktop/src/apps/DecisionsApp.tsx index 552e1bb8e..e1b4295f2 100644 --- a/desktop/src/apps/DecisionsApp.tsx +++ b/desktop/src/apps/DecisionsApp.tsx @@ -589,8 +589,10 @@ export function DecisionsApp({ windowId: _windowId }: { windowId: string }) { const [answered, setAnswered] = useState([]); const [authRequests, setAuthRequests] = useState([]); const [loading, setLoading] = useState(true); + const latestSeq = useRef(0); const load = useCallback(async (opts?: { silent?: boolean }) => { + const seq = ++latestSeq.current; if (!opts?.silent) setLoading(true); try { const [pRes, aRes, rRes] = await Promise.all([ @@ -600,9 +602,11 @@ export function DecisionsApp({ windowId: _windowId }: { windowId: string }) { ]); // Only overwrite a list when its request actually succeeded; a transient // failure must not blank out decisions the user can still act on. - if (pRes.ok) setPending(asDecisionList(await pRes.json())); - if (aRes.ok) setAnswered(asDecisionList(await aRes.json())); - if (rRes.ok) { + if (pRes.ok && seq === latestSeq.current) + setPending(asDecisionList(await pRes.json())); + if (aRes.ok && seq === latestSeq.current) + setAnswered(asDecisionList(await aRes.json())); + if (rRes.ok && seq === latestSeq.current) { const data = await rRes.json(); const reqs = (data?.requests ?? data ?? []) as AuthRequest[]; setAuthRequests(Array.isArray(reqs) ? reqs : []); @@ -610,7 +614,7 @@ export function DecisionsApp({ windowId: _windowId }: { windowId: string }) { } catch { // Network error: keep whatever was last loaded in place. } finally { - if (!opts?.silent) setLoading(false); + if (!opts?.silent && seq === latestSeq.current) setLoading(false); } }, []); From 0157a6b2dab6d824cd737c457c0f1c19ef19094a Mon Sep 17 00:00:00 2001 From: jaylfc Date: Mon, 17 Aug 2026 04:03:28 +0000 Subject: [PATCH 2/2] fix(decisions): close the json()-parse race window and un-stick loading The seq guard ran before each awaited json() parse, so a newer load starting mid-parse still let the stale body land (CR finding). Parse first, re-check seq immediately before each setter. The seq-guarded finally left loading stuck true forever when a silent focus refresh outraced the mount load (only non-silent caller), which also made both race regression tests vacuous: they asserted against the Loading placeholder, not the rendered list. finally now always clears its own non-silent loading; both tests assert the placeholder is gone before asserting stale data is absent. Red (pre-fix): 2 failed (both race tests). Green: 14 passed. --- desktop/src/apps/DecisionsApp.test.tsx | 67 ++++++++++++++++++++++++++ desktop/src/apps/DecisionsApp.tsx | 25 +++++++--- 2 files changed, 85 insertions(+), 7 deletions(-) diff --git a/desktop/src/apps/DecisionsApp.test.tsx b/desktop/src/apps/DecisionsApp.test.tsx index a91461a66..d279373b8 100644 --- a/desktop/src/apps/DecisionsApp.test.tsx +++ b/desktop/src/apps/DecisionsApp.test.tsx @@ -494,7 +494,74 @@ describe("DecisionsApp", () => { await new Promise((r) => setTimeout(r, 0)); }); + // Guard against a vacuous pass: if the mount load's loss of the race left + // `loading` stuck true, the list is not rendered at all and the stale-data + // assert below would pass no matter what state holds. + expect(screen.queryByText("Loading...")).toBeNull(); // The rendered list must still reflect load B's newer data, not load A's stale data. expect(screen.queryByText(singleSelect.question)).toBeNull(); }); + + it("stale response held at json() parse must not overwrite a newer load", async () => { + // Unlike the test above, load A's FETCHES resolve immediately — it is the + // awaited json() parse that is held. The seq guard has already been + // evaluated by then, so a check placed before the await cannot catch this. + const olderPending = [singleSelect]; + const newerPending: Decision[] = []; + let callCount = 0; + const heldJsonResolvers: Array<() => void> = []; + + const fetchMock = vi.fn().mockImplementation((input: string) => { + callCount++; + const bodyFor = (data: Decision[] | never[]) => { + if (input.includes("status=pending") && !input.includes("auth-requests")) return data; + if (input.includes("status=answered")) return []; + return { requests: [] }; + }; + if (callCount <= 3) { + return Promise.resolve({ + ok: true, + status: 200, + json: () => + new Promise((resolve) => { + heldJsonResolvers.push(() => resolve(bodyFor(olderPending))); + }), + }); + } + return Promise.resolve({ + ok: true, + status: 200, + json: () => Promise.resolve(bodyFor(newerPending)), + }); + }); + + vi.stubGlobal("fetch", fetchMock); + render(); + + // Load A's fetches resolve; A is now suspended inside await json(). + await act(async () => { + await new Promise((r) => setTimeout(r, 50)); + }); + + // Focus refresh triggers load B after the 1 s debounce; B completes fully. + window.dispatchEvent(new Event("focus")); + await act(async () => { + await new Promise((r) => setTimeout(r, 1200)); + }); + + // Release A's held json() bodies with the older data. A awaits the three + // bodies sequentially, so each release lets it register the next held + // json(); drain until none remain so A runs to completion (incl. finally). + await act(async () => { + while (heldJsonResolvers.length > 0) { + heldJsonResolvers.splice(0).forEach((r) => r()); + await new Promise((r) => setTimeout(r, 0)); + } + }); + + // Same vacuity guard as above: the list must actually be rendered. + expect(screen.queryByText("Loading...")).toBeNull(); + // B's newer (empty) list must survive; A's stale parse must not land. + expect(screen.queryByText(singleSelect.question)).toBeNull(); + }); }); diff --git a/desktop/src/apps/DecisionsApp.tsx b/desktop/src/apps/DecisionsApp.tsx index e1b4295f2..e9fc7efee 100644 --- a/desktop/src/apps/DecisionsApp.tsx +++ b/desktop/src/apps/DecisionsApp.tsx @@ -602,19 +602,30 @@ export function DecisionsApp({ windowId: _windowId }: { windowId: string }) { ]); // Only overwrite a list when its request actually succeeded; a transient // failure must not blank out decisions the user can still act on. - if (pRes.ok && seq === latestSeq.current) - setPending(asDecisionList(await pRes.json())); - if (aRes.ok && seq === latestSeq.current) - setAnswered(asDecisionList(await aRes.json())); - if (rRes.ok && seq === latestSeq.current) { + // The seq re-check must sit AFTER each awaited json() parse — a newer + // load can start while a body is still streaming, and a check placed + // before the await would let the stale body land anyway. + if (pRes.ok) { + const next = asDecisionList(await pRes.json()); + if (seq === latestSeq.current) setPending(next); + } + if (aRes.ok) { + const next = asDecisionList(await aRes.json()); + if (seq === latestSeq.current) setAnswered(next); + } + if (rRes.ok) { const data = await rRes.json(); const reqs = (data?.requests ?? data ?? []) as AuthRequest[]; - setAuthRequests(Array.isArray(reqs) ? reqs : []); + if (seq === latestSeq.current) setAuthRequests(Array.isArray(reqs) ? reqs : []); } } catch { // Network error: keep whatever was last loaded in place. } finally { - if (!opts?.silent && seq === latestSeq.current) setLoading(false); + // Deliberately NOT seq-guarded: loading tracks this non-silent call's + // own lifecycle. Only the mount load is non-silent; if a silent focus + // refresh outraces it, a guarded clear would leave "Loading..." stuck + // forever with data already on screen. + if (!opts?.silent) setLoading(false); } }, []);