From bce497d86b2e4414235fde386c18c9d63886ab66 Mon Sep 17 00:00:00 2001 From: shin-core <153108882+shin-core@users.noreply.github.com> Date: Wed, 22 Jul 2026 01:27:19 +0900 Subject: [PATCH] fix(ui): guard the 4 repo-picker panels against out-of-order fetch responses Each of these panels binds a free-text owner/repo to a load callback run in an effect keyed on repoFullName, with no cancellation guard: typing fires a fetch per keystroke, and an earlier (shorter, partially-typed) request resolving after a later one silently overwrites the newer repo's state -- e.g. showing the wrong repo's BYOK key status / AI-review mode. Apply the same cancelled-flag idiom use-polled-fetch.ts already uses (flag flipped in the effect cleanup, checked before any post-await setState) to all five load functions: activation-preview, ams-miner-cohort-card, ai-review-settings, and maintainer-settings (its main load plus FocusManifestEditor). Adds a representative regression test that resolves two repos' requests out of order and asserts the stale one is dropped. --- .../app-panels/activation-preview.test.tsx | 48 ++++++++++ .../site/app-panels/activation-preview.tsx | 55 ++++++----- .../site/app-panels/ai-review-settings.tsx | 65 +++++++------ .../site/app-panels/ams-miner-cohort-card.tsx | 55 ++++++----- .../site/app-panels/maintainer-settings.tsx | 96 ++++++++++++------- 5 files changed, 211 insertions(+), 108 deletions(-) diff --git a/apps/loopover-ui/src/components/site/app-panels/activation-preview.test.tsx b/apps/loopover-ui/src/components/site/app-panels/activation-preview.test.tsx index 7bbabfbe4b..401fdcb58b 100644 --- a/apps/loopover-ui/src/components/site/app-panels/activation-preview.test.tsx +++ b/apps/loopover-ui/src/components/site/app-panels/activation-preview.test.tsx @@ -133,4 +133,52 @@ describe("ActivationPreview", () => { }); expect(screen.getByText(/Settings are unavailable for this repository\./i)).toBeTruthy(); }); + + it("ignores a stale earlier response that resolves after a newer repo was typed (#7784)", async () => { + // Per-repo deferred responses keyed off the request URL, so we can resolve them out of order: the FIRST + // repo's (slow) request is resolved LAST, after the SECOND repo's request already landed. The stale first + // response must not overwrite the second repo's rendered preview. + const resolvers: Record void> = {}; + apiFetch.mockImplementation( + (url: string) => + new Promise((resolve) => { + const repo = url.includes("/acme/first/") + ? "first" + : url.includes("/acme/second/") + ? "second" + : "other"; + resolvers[repo] = resolve; + }), + ); + render(); + + // Type the first repo (its request is now pending, unresolved). + fireEvent.change(screen.getByPlaceholderText("owner/repo"), { + target: { value: "acme/first" }, + }); + await waitFor(() => expect(resolvers.first).toBeTruthy()); + + // Type a second repo before the first resolves; its request is pending too. + fireEvent.change(screen.getByPlaceholderText("owner/repo"), { + target: { value: "acme/second" }, + }); + await waitFor(() => expect(resolvers.second).toBeTruthy()); + + // The SECOND (newest) request resolves first with the second repo's summary. + resolvers.second({ + ok: true, + data: { ...BASE_PREVIEW, repoFullName: "acme/second", summary: "SECOND repo summary." }, + }); + await waitFor(() => expect(screen.getByText("SECOND repo summary.")).toBeTruthy()); + + // Now the STALE first request finally resolves. The cancelled-flag guard must drop it so the second repo's + // preview stays on screen rather than being clobbered by the first repo's now-outdated data. + resolvers.first({ + ok: true, + data: { ...BASE_PREVIEW, repoFullName: "acme/first", summary: "FIRST repo summary (stale)." }, + }); + await Promise.resolve(); + await waitFor(() => expect(screen.getByText("SECOND repo summary.")).toBeTruthy()); + expect(screen.queryByText("FIRST repo summary (stale).")).toBeNull(); + }); }); diff --git a/apps/loopover-ui/src/components/site/app-panels/activation-preview.tsx b/apps/loopover-ui/src/components/site/app-panels/activation-preview.tsx index cc3c66d232..844e09f18c 100644 --- a/apps/loopover-ui/src/components/site/app-panels/activation-preview.tsx +++ b/apps/loopover-ui/src/components/site/app-panels/activation-preview.tsx @@ -65,31 +65,42 @@ export function ActivationPreview({ reviewability }: { reviewability: Array<{ pr const base = repoApiBase(repoFullName); const hasRepos = repoOptions.length > 0; - const load = useCallback(async () => { - const apiBase = repoApiBase(repoFullName); - if (!apiBase) { - setPreview(null); + // isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the + // effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's state + // (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- the flag is flipped in the effect cleanup. + const load = useCallback( + async (isCancelled: () => boolean = () => false) => { + const apiBase = repoApiBase(repoFullName); + if (!apiBase) { + setPreview(null); + setLoadError(null); + return; + } setLoadError(null); - return; - } - setLoadError(null); - setLoading(true); - const result = await apiFetch(`${apiBase}/activation-preview`, { - label: "Activation preview", - credentials: "include", - silentStatus: true, - }); - if (result.ok) { - setPreview(result.data); - } else { - setPreview(null); - setLoadError(result.message); - } - setLoading(false); - }, [repoFullName]); + setLoading(true); + const result = await apiFetch(`${apiBase}/activation-preview`, { + label: "Activation preview", + credentials: "include", + silentStatus: true, + }); + if (isCancelled()) return; + if (result.ok) { + setPreview(result.data); + } else { + setPreview(null); + setLoadError(result.message); + } + setLoading(false); + }, + [repoFullName], + ); useEffect(() => { - void load(); + let cancelled = false; + void load(() => cancelled); + return () => { + cancelled = true; + }; }, [load]); return ( diff --git a/apps/loopover-ui/src/components/site/app-panels/ai-review-settings.tsx b/apps/loopover-ui/src/components/site/app-panels/ai-review-settings.tsx index b6c9a193bd..366e6076eb 100644 --- a/apps/loopover-ui/src/components/site/app-panels/ai-review-settings.tsx +++ b/apps/loopover-ui/src/components/site/app-panels/ai-review-settings.tsx @@ -61,35 +61,46 @@ export function AiReviewSettings({ reviewability }: { reviewability: Array<{ pr: const base = repoApiBase(repoFullName); const hasRepos = repoOptions.length > 0; - const load = useCallback(async () => { - const apiBase = repoApiBase(repoFullName); - if (!apiBase) return; - setMessage(null); - setLoading(true); - const [settings, key] = await Promise.all([ - apiFetch(`${apiBase}/settings`, { - label: "AI review settings", - credentials: "include", - silentStatus: true, - }), - apiFetch(`${apiBase}/ai-key`, { - label: "AI key status", - credentials: "include", - silentStatus: true, - }), - ]); - if (settings.ok) { - setMode(settings.data.aiReviewMode ?? "off"); - setByok(settings.data.aiReviewByok ?? false); - setProvider(settings.data.aiReviewProvider ?? "anthropic"); - setModel(settings.data.aiReviewModel ?? ""); - } - setKeyStatus(key.ok ? key.data : null); - setLoading(false); - }, [repoFullName]); + // isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the + // effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's settings + // and key status (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- flipped in the effect cleanup. + const load = useCallback( + async (isCancelled: () => boolean = () => false) => { + const apiBase = repoApiBase(repoFullName); + if (!apiBase) return; + setMessage(null); + setLoading(true); + const [settings, key] = await Promise.all([ + apiFetch(`${apiBase}/settings`, { + label: "AI review settings", + credentials: "include", + silentStatus: true, + }), + apiFetch(`${apiBase}/ai-key`, { + label: "AI key status", + credentials: "include", + silentStatus: true, + }), + ]); + if (isCancelled()) return; + if (settings.ok) { + setMode(settings.data.aiReviewMode ?? "off"); + setByok(settings.data.aiReviewByok ?? false); + setProvider(settings.data.aiReviewProvider ?? "anthropic"); + setModel(settings.data.aiReviewModel ?? ""); + } + setKeyStatus(key.ok ? key.data : null); + setLoading(false); + }, + [repoFullName], + ); useEffect(() => { - void load(); + let cancelled = false; + void load(() => cancelled); + return () => { + cancelled = true; + }; }, [load]); async function saveKey() { diff --git a/apps/loopover-ui/src/components/site/app-panels/ams-miner-cohort-card.tsx b/apps/loopover-ui/src/components/site/app-panels/ams-miner-cohort-card.tsx index 96e290c979..a840e55b44 100644 --- a/apps/loopover-ui/src/components/site/app-panels/ams-miner-cohort-card.tsx +++ b/apps/loopover-ui/src/components/site/app-panels/ams-miner-cohort-card.tsx @@ -98,31 +98,42 @@ export function AmsMinerCohortCard({ reviewability }: { reviewability: Array<{ p const base = repoApiBase(repoFullName); const hasRepos = repoOptions.length > 0; - const load = useCallback(async () => { - const apiBase = repoApiBase(repoFullName); - if (!apiBase) { - setComparison(null); + // isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the + // effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's state + // (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- the flag is flipped in the effect cleanup. + const load = useCallback( + async (isCancelled: () => boolean = () => false) => { + const apiBase = repoApiBase(repoFullName); + if (!apiBase) { + setComparison(null); + setLoadError(null); + return; + } setLoadError(null); - return; - } - setLoadError(null); - setLoading(true); - const result = await apiFetch(`${apiBase}/ams-miner-cohort`, { - label: "AMS miner cohort comparison", - credentials: "include", - silentStatus: true, - }); - if (result.ok) { - setComparison(result.data); - } else { - setComparison(null); - setLoadError(result.message); - } - setLoading(false); - }, [repoFullName]); + setLoading(true); + const result = await apiFetch(`${apiBase}/ams-miner-cohort`, { + label: "AMS miner cohort comparison", + credentials: "include", + silentStatus: true, + }); + if (isCancelled()) return; + if (result.ok) { + setComparison(result.data); + } else { + setComparison(null); + setLoadError(result.message); + } + setLoading(false); + }, + [repoFullName], + ); useEffect(() => { - void load(); + let cancelled = false; + void load(() => cancelled); + return () => { + cancelled = true; + }; }, [load]); return ( diff --git a/apps/loopover-ui/src/components/site/app-panels/maintainer-settings.tsx b/apps/loopover-ui/src/components/site/app-panels/maintainer-settings.tsx index ebef4c74af..9e6c114dca 100644 --- a/apps/loopover-ui/src/components/site/app-panels/maintainer-settings.tsx +++ b/apps/loopover-ui/src/components/site/app-panels/maintainer-settings.tsx @@ -145,32 +145,43 @@ export function MaintainerSettings({ reviewability }: { reviewability: Array<{ p const base = repoApiBase(repoFullName); const hasRepos = repoOptions.length > 0; - const load = useCallback(async () => { - const apiBase = repoApiBase(repoFullName); - if (!apiBase) return; - setMessage(null); - setLoading(true); - const result = await apiFetch(`${apiBase}/settings`, { - label: "Repository settings", - credentials: "include", - silentStatus: true, - }); - // Default the agent-layer fields defensively so the editor renders even against an older response shape. - setSettings( - result.ok - ? { - ...result.data, - autonomy: result.data.autonomy ?? {}, - agentPaused: result.data.agentPaused ?? false, - agentDryRun: result.data.agentDryRun ?? false, - } - : null, - ); - setLoading(false); - }, [repoFullName]); + // isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the + // effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's settings + // (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- flipped in the effect cleanup. + const load = useCallback( + async (isCancelled: () => boolean = () => false) => { + const apiBase = repoApiBase(repoFullName); + if (!apiBase) return; + setMessage(null); + setLoading(true); + const result = await apiFetch(`${apiBase}/settings`, { + label: "Repository settings", + credentials: "include", + silentStatus: true, + }); + if (isCancelled()) return; + // Default the agent-layer fields defensively so the editor renders even against an older response shape. + setSettings( + result.ok + ? { + ...result.data, + autonomy: result.data.autonomy ?? {}, + agentPaused: result.data.agentPaused ?? false, + agentDryRun: result.data.agentDryRun ?? false, + } + : null, + ); + setLoading(false); + }, + [repoFullName], + ); useEffect(() => { - void load(); + let cancelled = false; + void load(() => cancelled); + return () => { + cancelled = true; + }; }, [load]); function setField(key: K, value: MaintainerSettings[K]) { @@ -520,21 +531,32 @@ function FocusManifestEditor({ base }: { base: string | null }) { const [busy, setBusy] = useState(false); const [message, setMessage] = useState(null); - const load = useCallback(async () => { - if (!base) return; - setLoading(true); - setMessage(null); - const result = await apiFetch(`${base}/focus-manifest`, { - label: "Focus manifest", - credentials: "include", - silentStatus: true, - }); - setText(result.ok ? JSON.stringify(result.data.manifest, null, 2) : ""); - setLoading(false); - }, [base]); + // isCancelled guards against an out-of-order response: `base` changes as the parent's repoFullName is typed + // (and re-runs the effect) before an earlier request resolves, so an older fetch must not overwrite the newer + // repo's manifest text (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- flipped in the cleanup. + const load = useCallback( + async (isCancelled: () => boolean = () => false) => { + if (!base) return; + setLoading(true); + setMessage(null); + const result = await apiFetch(`${base}/focus-manifest`, { + label: "Focus manifest", + credentials: "include", + silentStatus: true, + }); + if (isCancelled()) return; + setText(result.ok ? JSON.stringify(result.data.manifest, null, 2) : ""); + setLoading(false); + }, + [base], + ); useEffect(() => { - void load(); + let cancelled = false; + void load(() => cancelled); + return () => { + cancelled = true; + }; }, [load]); async function save() {