Skip to content

Commit 717fb3c

Browse files
committed
fix(review): paginate findPreviewUrlFromChecks's check-runs read
findPreviewUrlFromChecks fetched only page 1 of a commit's check-runs and never followed pagination, unlike its siblings getPreviewBuildState and findPreviewUrlFromPrComments which walk the same endpoint via the file's own findAcrossPages helper. On a commit with >100 check-runs the Cloudflare Workers Builds check-run can land on page 2+, so this discovery returned null even though the check existed -- the PR then showed a permanent loading spinner instead of the real preview screenshot. Reuse findAcrossPages (as the issue requires) so the check-runs scan walks every page, stopping as soon as a page yields a usable preview URL, and preserve the pre-existing best-effort degrade-to-null on a read error.
1 parent 0ff0b8f commit 717fb3c

2 files changed

Lines changed: 98 additions & 7 deletions

File tree

src/review/visual/preview-url.ts

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -228,15 +228,28 @@ export async function findPreviewUrlFromChecks(params: {
228228
const url = extractPreviewUrl(status.target_url);
229229
if (url) return url;
230230
}
231-
const checks = await githubJson<{ check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> }>(
231+
// Walk EVERY page of check-runs (#7779): a commit with >100 check-runs can push the preview-deploy
232+
// check-run onto page 2+, which a single per_page=100 read would miss -- exactly the truncation the file
233+
// header warns about, and the same walk getPreviewBuildState/findPreviewUrlFromPrComments already do for
234+
// this endpoint. Reuses findAcrossPages so the scan stops as soon as a page yields a usable URL.
235+
const urlFromChecks = await findAcrossPages<
236+
{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } },
237+
string
238+
>(
232239
`${base}/commits/${encodeURIComponent(params.sha)}/check-runs?per_page=100`,
233240
opts,
241+
(payload) =>
242+
(payload as { check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> })?.check_runs ?? [],
243+
(runs) => {
244+
for (const run of runs) {
245+
if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue;
246+
const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text);
247+
if (url) return url;
248+
}
249+
return null;
250+
},
234251
).catch(() => null);
235-
for (const run of checks?.check_runs ?? []) {
236-
if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue;
237-
const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text);
238-
if (url) return url;
239-
}
252+
if (urlFromChecks) return urlFromChecks;
240253
} catch (error) {
241254
console.log(JSON.stringify({ event: "preview_from_checks_error", repo: `${params.repo.owner}/${params.repo.repo}`, message: String(error).slice(0, 200) }));
242255
}

test/unit/preview-url.test.ts

Lines changed: 79 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { afterEach, describe, expect, it, vi } from "vitest";
22
import { clearGitHubResponseCacheForTest, githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client";
3-
import { extractPreviewUrl, findPreviewUrlFromPrComments, getPreviewBuildState } from "../../src/review/visual/preview-url";
3+
import { extractPreviewUrl, findPreviewUrlFromChecks, findPreviewUrlFromPrComments, getPreviewBuildState } from "../../src/review/visual/preview-url";
44

55
/** GitHub's `Link` header for a page that advertises a next page (the exact shape findAcrossPages walks). */
66
const NEXT_LINK = '<https://api.github.com/resource?per_page=100&page=99>; rel="next", <https://api.github.com/resource?per_page=100&page=99>; rel="last"';
@@ -166,6 +166,84 @@ describe("preview-url pagination (#7450)", () => {
166166
await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "fail" })).resolves.toBe("absent");
167167
expect(failLater).toHaveBeenCalledTimes(2);
168168
});
169+
170+
// findPreviewUrlFromChecks scans the combined commit-status first, then walks check-runs. These stub the
171+
// status endpoint to an empty/no-preview payload so control reaches the check-runs walk under test (#7779).
172+
const emptyStatus = () => Response.json({ statuses: [] });
173+
174+
it("findPreviewUrlFromChecks follows Link: rel=next and finds the preview check-run on page 2 (#7779)", async () => {
175+
// The bug: a commit with >100 check-runs pushes the Cloudflare Workers Builds check onto page 2+, which the
176+
// old single-page read missed. Page 1 is 100 non-preview runs advertising a next page; page 2 carries it.
177+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
178+
const url = String(input);
179+
if (url.includes("/status")) return emptyStatus();
180+
if (url.includes("/check-runs")) {
181+
if (isPage2(input)) return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://pr-9.pages.dev/" }] });
182+
return Response.json({ check_runs: Array.from({ length: 100 }, () => ({ status: "completed", conclusion: "success", details_url: "https://example.com/ci" })) }, { headers: { link: NEXT_LINK } });
183+
}
184+
return Response.json({});
185+
});
186+
vi.stubGlobal("fetch", fetchMock);
187+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc" })).resolves.toBe("https://pr-9.pages.dev");
188+
// Page 2 of check-runs was actually fetched (the pre-#7779 bug never read it).
189+
expect(fetchMock.mock.calls.some((c) => String(c[0]).includes("/check-runs") && isPage2(c[0]))).toBe(true);
190+
});
191+
192+
it("findPreviewUrlFromChecks skips a completed non-success check-run and reads the preview from output.summary/text (#7779)", async () => {
193+
// Exercises the `continue` (completed && conclusion !== success) skip and the details_url ?? summary ?? text
194+
// fallback chain: the first run is a hard failure (skipped), the second carries the URL only in output.text.
195+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
196+
const url = String(input);
197+
if (url.includes("/status")) return emptyStatus();
198+
if (url.includes("/check-runs")) {
199+
return Response.json({
200+
check_runs: [
201+
{ status: "completed", conclusion: "failure", details_url: "https://should-be-skipped.pages.dev/" },
202+
{ status: "completed", conclusion: "success", output: { text: "preview at https://from-text.workers.dev/" } },
203+
],
204+
});
205+
}
206+
return Response.json({});
207+
});
208+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc" })).resolves.toBe("https://from-text.workers.dev");
209+
});
210+
211+
it("findPreviewUrlFromChecks returns null when no check-run carries a preview URL and there is no next page (#7779)", async () => {
212+
// The probe finds nothing on the only page and the payload has no `check_runs` array (treated as []): the
213+
// whole discovery degrades to null rather than throwing.
214+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
215+
const url = String(input);
216+
if (url.includes("/status")) return emptyStatus();
217+
if (url.includes("/check-runs")) return Response.json({});
218+
return Response.json({});
219+
});
220+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc" })).resolves.toBeNull();
221+
});
222+
223+
it("findPreviewUrlFromChecks degrades to null when the check-runs read fails, without throwing (#7779)", async () => {
224+
// The .catch(() => null) around the paginated walk preserves the pre-#7779 best-effort behavior: a check-runs
225+
// fetch error must not throw out of the function, just yield no URL from that source.
226+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
227+
const url = String(input);
228+
if (url.includes("/status")) return emptyStatus();
229+
if (url.includes("/check-runs")) throw new Error("check-runs network down");
230+
return Response.json({});
231+
});
232+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc" })).resolves.toBeNull();
233+
});
234+
235+
it("findPreviewUrlFromChecks returns a preview URL straight from the combined commit-status, before touching check-runs (#7779)", async () => {
236+
// The status-first path: a success status whose target_url is a preview host short-circuits before the
237+
// check-runs walk runs at all.
238+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
239+
const url = String(input);
240+
if (url.includes("/status")) return Response.json({ statuses: [{ state: "success", target_url: "https://from-status.pages.dev/" }] });
241+
return Response.json({});
242+
});
243+
vi.stubGlobal("fetch", fetchMock);
244+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc" })).resolves.toBe("https://from-status.pages.dev");
245+
expect(fetchMock.mock.calls.some((c) => String(c[0]).includes("/check-runs"))).toBe(false);
246+
});
169247
});
170248

171249
describe("extractPreviewUrl", () => {

0 commit comments

Comments
 (0)