Skip to content

fix(ui): guard the repo-picker panels against out-of-order fetch responses #7896

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
shin-core wants to merge 1 commit into from
Closed

fix(ui): guard the repo-picker panels against out-of-order fetch responses #7896

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 48 additions & 0 deletions apps/loopover-ui/src/components/site/app-panels/activation-preview.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<string, (value: unknown) => 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(<ActivationPreview reviewability={[{ pr: "acme/first#1" }]} />);

// 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();
});
});
55 changes: 33 additions & 22 deletions apps/loopover-ui/src/components/site/app-panels/activation-preview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<ActivationPreviewResponse>(`${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<ActivationPreviewResponse>(`${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 (
Expand Down
65 changes: 38 additions & 27 deletions apps/loopover-ui/src/components/site/app-panels/ai-review-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<RepoSettingsResponse>(`${apiBase}/settings`, {
label: "AI review settings",
credentials: "include",
silentStatus: true,
}),
apiFetch<AiKeyStatus>(`${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<RepoSettingsResponse>(`${apiBase}/settings`, {
label: "AI review settings",
credentials: "include",
silentStatus: true,
}),
apiFetch<AiKeyStatus>(`${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() {
Expand Down
55 changes: 33 additions & 22 deletions apps/loopover-ui/src/components/site/app-panels/ams-miner-cohort-card.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<AmsMinerCohortComparison>(`${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<AmsMinerCohortComparison>(`${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 (
Expand Down
96 changes: 59 additions & 37 deletions apps/loopover-ui/src/components/site/app-panels/maintainer-settings.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<MaintainerSettings>(`${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<MaintainerSettings>(`${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<K extends keyof MaintainerSettings>(key: K, value: MaintainerSettings[K]) {
Expand Down Expand Up @@ -520,21 +531,32 @@ function FocusManifestEditor({ base }: { base: string | null }) {
const [busy, setBusy] = useState(false);
const [message, setMessage] = useState<Message | null>(null);

const load = useCallback(async () => {
if (!base) return;
setLoading(true);
setMessage(null);
const result = await apiFetch<FocusManifestResponse>(`${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<FocusManifestResponse>(`${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() {
Expand Down
Loading