Skip to content

Commit bce497d

Browse files
shin-coreshin-core
committed
fix(ui): guard the 4 repo-picker panels against out-of-order fetch responses
Each of these panels binds a free-text owner/repo <input> 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.
1 parent 2364ad4 commit bce497d

5 files changed

Lines changed: 211 additions & 108 deletions

File tree

‎apps/loopover-ui/src/components/site/app-panels/activation-preview.test.tsx‎

Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,4 +133,52 @@ describe("ActivationPreview", () => {
133133
});
134134
expect(screen.getByText(/Settings are unavailable for this repository\./i)).toBeTruthy();
135135
});
136+
137+
it("ignores a stale earlier response that resolves after a newer repo was typed (#7784)", async () => {
138+
// Per-repo deferred responses keyed off the request URL, so we can resolve them out of order: the FIRST
139+
// repo's (slow) request is resolved LAST, after the SECOND repo's request already landed. The stale first
140+
// response must not overwrite the second repo's rendered preview.
141+
const resolvers: Record<string, (value: unknown) => void> = {};
142+
apiFetch.mockImplementation(
143+
(url: string) =>
144+
new Promise((resolve) => {
145+
const repo = url.includes("/acme/first/")
146+
? "first"
147+
: url.includes("/acme/second/")
148+
? "second"
149+
: "other";
150+
resolvers[repo] = resolve;
151+
}),
152+
);
153+
render(<ActivationPreview reviewability={[{ pr: "acme/first#1" }]} />);
154+
155+
// Type the first repo (its request is now pending, unresolved).
156+
fireEvent.change(screen.getByPlaceholderText("owner/repo"), {
157+
target: { value: "acme/first" },
158+
});
159+
await waitFor(() => expect(resolvers.first).toBeTruthy());
160+
161+
// Type a second repo before the first resolves; its request is pending too.
162+
fireEvent.change(screen.getByPlaceholderText("owner/repo"), {
163+
target: { value: "acme/second" },
164+
});
165+
await waitFor(() => expect(resolvers.second).toBeTruthy());
166+
167+
// The SECOND (newest) request resolves first with the second repo's summary.
168+
resolvers.second({
169+
ok: true,
170+
data: { ...BASE_PREVIEW, repoFullName: "acme/second", summary: "SECOND repo summary." },
171+
});
172+
await waitFor(() => expect(screen.getByText("SECOND repo summary.")).toBeTruthy());
173+
174+
// Now the STALE first request finally resolves. The cancelled-flag guard must drop it so the second repo's
175+
// preview stays on screen rather than being clobbered by the first repo's now-outdated data.
176+
resolvers.first({
177+
ok: true,
178+
data: { ...BASE_PREVIEW, repoFullName: "acme/first", summary: "FIRST repo summary (stale)." },
179+
});
180+
await Promise.resolve();
181+
await waitFor(() => expect(screen.getByText("SECOND repo summary.")).toBeTruthy());
182+
expect(screen.queryByText("FIRST repo summary (stale).")).toBeNull();
183+
});
136184
});

‎apps/loopover-ui/src/components/site/app-panels/activation-preview.tsx‎

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -65,31 +65,42 @@ export function ActivationPreview({ reviewability }: { reviewability: Array<{ pr
6565
const base = repoApiBase(repoFullName);
6666
const hasRepos = repoOptions.length > 0;
6767

68-
const load = useCallback(async () => {
69-
const apiBase = repoApiBase(repoFullName);
70-
if (!apiBase) {
71-
setPreview(null);
68+
// isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the
69+
// effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's state
70+
// (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- the flag is flipped in the effect cleanup.
71+
const load = useCallback(
72+
async (isCancelled: () => boolean = () => false) => {
73+
const apiBase = repoApiBase(repoFullName);
74+
if (!apiBase) {
75+
setPreview(null);
76+
setLoadError(null);
77+
return;
78+
}
7279
setLoadError(null);
73-
return;
74-
}
75-
setLoadError(null);
76-
setLoading(true);
77-
const result = await apiFetch<ActivationPreviewResponse>(`${apiBase}/activation-preview`, {
78-
label: "Activation preview",
79-
credentials: "include",
80-
silentStatus: true,
81-
});
82-
if (result.ok) {
83-
setPreview(result.data);
84-
} else {
85-
setPreview(null);
86-
setLoadError(result.message);
87-
}
88-
setLoading(false);
89-
}, [repoFullName]);
80+
setLoading(true);
81+
const result = await apiFetch<ActivationPreviewResponse>(`${apiBase}/activation-preview`, {
82+
label: "Activation preview",
83+
credentials: "include",
84+
silentStatus: true,
85+
});
86+
if (isCancelled()) return;
87+
if (result.ok) {
88+
setPreview(result.data);
89+
} else {
90+
setPreview(null);
91+
setLoadError(result.message);
92+
}
93+
setLoading(false);
94+
},
95+
[repoFullName],
96+
);
9097

9198
useEffect(() => {
92-
void load();
99+
let cancelled = false;
100+
void load(() => cancelled);
101+
return () => {
102+
cancelled = true;
103+
};
93104
}, [load]);
94105

95106
return (

‎apps/loopover-ui/src/components/site/app-panels/ai-review-settings.tsx‎

Lines changed: 38 additions & 27 deletions
Original file line numberDiff line numberDiff line change
@@ -61,35 +61,46 @@ export function AiReviewSettings({ reviewability }: { reviewability: Array<{ pr:
6161
const base = repoApiBase(repoFullName);
6262
const hasRepos = repoOptions.length > 0;
6363

64-
const load = useCallback(async () => {
65-
const apiBase = repoApiBase(repoFullName);
66-
if (!apiBase) return;
67-
setMessage(null);
68-
setLoading(true);
69-
const [settings, key] = await Promise.all([
70-
apiFetch<RepoSettingsResponse>(`${apiBase}/settings`, {
71-
label: "AI review settings",
72-
credentials: "include",
73-
silentStatus: true,
74-
}),
75-
apiFetch<AiKeyStatus>(`${apiBase}/ai-key`, {
76-
label: "AI key status",
77-
credentials: "include",
78-
silentStatus: true,
79-
}),
80-
]);
81-
if (settings.ok) {
82-
setMode(settings.data.aiReviewMode ?? "off");
83-
setByok(settings.data.aiReviewByok ?? false);
84-
setProvider(settings.data.aiReviewProvider ?? "anthropic");
85-
setModel(settings.data.aiReviewModel ?? "");
86-
}
87-
setKeyStatus(key.ok ? key.data : null);
88-
setLoading(false);
89-
}, [repoFullName]);
64+
// isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the
65+
// effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's settings
66+
// and key status (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- flipped in the effect cleanup.
67+
const load = useCallback(
68+
async (isCancelled: () => boolean = () => false) => {
69+
const apiBase = repoApiBase(repoFullName);
70+
if (!apiBase) return;
71+
setMessage(null);
72+
setLoading(true);
73+
const [settings, key] = await Promise.all([
74+
apiFetch<RepoSettingsResponse>(`${apiBase}/settings`, {
75+
label: "AI review settings",
76+
credentials: "include",
77+
silentStatus: true,
78+
}),
79+
apiFetch<AiKeyStatus>(`${apiBase}/ai-key`, {
80+
label: "AI key status",
81+
credentials: "include",
82+
silentStatus: true,
83+
}),
84+
]);
85+
if (isCancelled()) return;
86+
if (settings.ok) {
87+
setMode(settings.data.aiReviewMode ?? "off");
88+
setByok(settings.data.aiReviewByok ?? false);
89+
setProvider(settings.data.aiReviewProvider ?? "anthropic");
90+
setModel(settings.data.aiReviewModel ?? "");
91+
}
92+
setKeyStatus(key.ok ? key.data : null);
93+
setLoading(false);
94+
},
95+
[repoFullName],
96+
);
9097

9198
useEffect(() => {
92-
void load();
99+
let cancelled = false;
100+
void load(() => cancelled);
101+
return () => {
102+
cancelled = true;
103+
};
93104
}, [load]);
94105

95106
async function saveKey() {

‎apps/loopover-ui/src/components/site/app-panels/ams-miner-cohort-card.tsx‎

Lines changed: 33 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -98,31 +98,42 @@ export function AmsMinerCohortCard({ reviewability }: { reviewability: Array<{ p
9898
const base = repoApiBase(repoFullName);
9999
const hasRepos = repoOptions.length > 0;
100100

101-
const load = useCallback(async () => {
102-
const apiBase = repoApiBase(repoFullName);
103-
if (!apiBase) {
104-
setComparison(null);
101+
// isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the
102+
// effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's state
103+
// (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- the flag is flipped in the effect cleanup.
104+
const load = useCallback(
105+
async (isCancelled: () => boolean = () => false) => {
106+
const apiBase = repoApiBase(repoFullName);
107+
if (!apiBase) {
108+
setComparison(null);
109+
setLoadError(null);
110+
return;
111+
}
105112
setLoadError(null);
106-
return;
107-
}
108-
setLoadError(null);
109-
setLoading(true);
110-
const result = await apiFetch<AmsMinerCohortComparison>(`${apiBase}/ams-miner-cohort`, {
111-
label: "AMS miner cohort comparison",
112-
credentials: "include",
113-
silentStatus: true,
114-
});
115-
if (result.ok) {
116-
setComparison(result.data);
117-
} else {
118-
setComparison(null);
119-
setLoadError(result.message);
120-
}
121-
setLoading(false);
122-
}, [repoFullName]);
113+
setLoading(true);
114+
const result = await apiFetch<AmsMinerCohortComparison>(`${apiBase}/ams-miner-cohort`, {
115+
label: "AMS miner cohort comparison",
116+
credentials: "include",
117+
silentStatus: true,
118+
});
119+
if (isCancelled()) return;
120+
if (result.ok) {
121+
setComparison(result.data);
122+
} else {
123+
setComparison(null);
124+
setLoadError(result.message);
125+
}
126+
setLoading(false);
127+
},
128+
[repoFullName],
129+
);
123130

124131
useEffect(() => {
125-
void load();
132+
let cancelled = false;
133+
void load(() => cancelled);
134+
return () => {
135+
cancelled = true;
136+
};
126137
}, [load]);
127138

128139
return (

‎apps/loopover-ui/src/components/site/app-panels/maintainer-settings.tsx‎

Lines changed: 59 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -145,32 +145,43 @@ export function MaintainerSettings({ reviewability }: { reviewability: Array<{ p
145145
const base = repoApiBase(repoFullName);
146146
const hasRepos = repoOptions.length > 0;
147147

148-
const load = useCallback(async () => {
149-
const apiBase = repoApiBase(repoFullName);
150-
if (!apiBase) return;
151-
setMessage(null);
152-
setLoading(true);
153-
const result = await apiFetch<MaintainerSettings>(`${apiBase}/settings`, {
154-
label: "Repository settings",
155-
credentials: "include",
156-
silentStatus: true,
157-
});
158-
// Default the agent-layer fields defensively so the editor renders even against an older response shape.
159-
setSettings(
160-
result.ok
161-
? {
162-
...result.data,
163-
autonomy: result.data.autonomy ?? {},
164-
agentPaused: result.data.agentPaused ?? false,
165-
agentDryRun: result.data.agentDryRun ?? false,
166-
}
167-
: null,
168-
);
169-
setLoading(false);
170-
}, [repoFullName]);
148+
// isCancelled guards against an out-of-order response: a keystroke replaces repoFullName (and re-runs the
149+
// effect) before an earlier request resolves, so an older fetch must not overwrite the newer repo's settings
150+
// (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- flipped in the effect cleanup.
151+
const load = useCallback(
152+
async (isCancelled: () => boolean = () => false) => {
153+
const apiBase = repoApiBase(repoFullName);
154+
if (!apiBase) return;
155+
setMessage(null);
156+
setLoading(true);
157+
const result = await apiFetch<MaintainerSettings>(`${apiBase}/settings`, {
158+
label: "Repository settings",
159+
credentials: "include",
160+
silentStatus: true,
161+
});
162+
if (isCancelled()) return;
163+
// Default the agent-layer fields defensively so the editor renders even against an older response shape.
164+
setSettings(
165+
result.ok
166+
? {
167+
...result.data,
168+
autonomy: result.data.autonomy ?? {},
169+
agentPaused: result.data.agentPaused ?? false,
170+
agentDryRun: result.data.agentDryRun ?? false,
171+
}
172+
: null,
173+
);
174+
setLoading(false);
175+
},
176+
[repoFullName],
177+
);
171178

172179
useEffect(() => {
173-
void load();
180+
let cancelled = false;
181+
void load(() => cancelled);
182+
return () => {
183+
cancelled = true;
184+
};
174185
}, [load]);
175186

176187
function setField<K extends keyof MaintainerSettings>(key: K, value: MaintainerSettings[K]) {
@@ -520,21 +531,32 @@ function FocusManifestEditor({ base }: { base: string | null }) {
520531
const [busy, setBusy] = useState(false);
521532
const [message, setMessage] = useState<Message | null>(null);
522533

523-
const load = useCallback(async () => {
524-
if (!base) return;
525-
setLoading(true);
526-
setMessage(null);
527-
const result = await apiFetch<FocusManifestResponse>(`${base}/focus-manifest`, {
528-
label: "Focus manifest",
529-
credentials: "include",
530-
silentStatus: true,
531-
});
532-
setText(result.ok ? JSON.stringify(result.data.manifest, null, 2) : "");
533-
setLoading(false);
534-
}, [base]);
534+
// isCancelled guards against an out-of-order response: `base` changes as the parent's repoFullName is typed
535+
// (and re-runs the effect) before an earlier request resolves, so an older fetch must not overwrite the newer
536+
// repo's manifest text (#7784). Same cancelled-flag idiom as use-polled-fetch.ts -- flipped in the cleanup.
537+
const load = useCallback(
538+
async (isCancelled: () => boolean = () => false) => {
539+
if (!base) return;
540+
setLoading(true);
541+
setMessage(null);
542+
const result = await apiFetch<FocusManifestResponse>(`${base}/focus-manifest`, {
543+
label: "Focus manifest",
544+
credentials: "include",
545+
silentStatus: true,
546+
});
547+
if (isCancelled()) return;
548+
setText(result.ok ? JSON.stringify(result.data.manifest, null, 2) : "");
549+
setLoading(false);
550+
},
551+
[base],
552+
);
535553

536554
useEffect(() => {
537-
void load();
555+
let cancelled = false;
556+
void load(() => cancelled);
557+
return () => {
558+
cancelled = true;
559+
};
538560
}, [load]);
539561

540562
async function save() {

0 commit comments

Comments
 (0)