Skip to content

Commit e84b4e1

Browse files
fix(review): paginate check-runs in findPreviewUrlFromChecks (#7881)
findPreviewUrlFromChecks fetched only page 1 of a head SHA's check-runs (?per_page=100) and never followed pagination, while its two siblings in the same file -- getPreviewBuildState and findPreviewUrlFromPrComments -- already walk this identical endpoint via findAcrossPages. The file's own header comment reasons about exactly this failure: a commit with >100 check-runs pushes the Cloudflare Workers Builds check-run onto page 2+, and a page-1-only read then returns null as if the preview didn't exist -- so the PR comment shows a permanent loading spinner even though the deploy succeeded. Route the check-runs fetch through findAcrossPages, reusing the exact pattern getPreviewBuildState already applies to this endpoint (no new pagination loop); the per-run preview-URL scan moves into the page probe unchanged. Adds regression tests: a preview check-run on page 2 is still found (asserting the walk reaches page 2), a page-1 hit takes no extra request, a failed check-run is skipped, and an all-next-Link/no-check_runs response terminates bounded at null. Closes #7779 Co-authored-by: davion-knight <marko.j0158@gmail.com>
1 parent 1454d22 commit e84b4e1

2 files changed

Lines changed: 68 additions & 8 deletions

File tree

src/review/visual/preview-url.ts

Lines changed: 16 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -243,15 +243,24 @@ export async function findPreviewUrlFromChecks(params: {
243243
const url = extractPreviewUrl(status.target_url);
244244
if (url) return url;
245245
}
246-
const checks = await githubJson<{ check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> }>(
246+
// Walk every page of check-runs (#7779): a head SHA with >100 check-runs can push the Cloudflare Workers
247+
// Builds check-run onto page 2+, and a page-1-only read would then miss it -- the same failure the file's
248+
// header reasons about, already handled by getPreviewBuildState/findPreviewUrlFromPrComments for this
249+
// identical endpoint via findAcrossPages.
250+
const checkUrl = await findAcrossPages<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }, string>(
247251
`${base}/commits/${encodeURIComponent(params.sha)}/check-runs?per_page=100`,
248252
opts,
249-
).catch(() => null);
250-
for (const run of checks?.check_runs ?? []) {
251-
if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue;
252-
const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text);
253-
if (url) return url;
254-
}
253+
(payload) => (payload as { check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> })?.check_runs ?? [],
254+
(runs) => {
255+
for (const run of runs) {
256+
if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue;
257+
const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text);
258+
if (url) return url;
259+
}
260+
return null;
261+
},
262+
);
263+
if (checkUrl) return checkUrl;
255264
} catch (error) {
256265
console.log(JSON.stringify({ event: "preview_from_checks_error", repo: `${params.repo.owner}/${params.repo.repo}`, message: String(error).slice(0, 200) }));
257266
}

test/unit/preview-url.test.ts

Lines changed: 52 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, getLatestDeploymentStatus, getPreviewBuildState } from "../../src/review/visual/preview-url";
3+
import { extractPreviewUrl, findPreviewUrlFromChecks, findPreviewUrlFromPrComments, getLatestDeploymentStatus, 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"';
@@ -52,6 +52,57 @@ describe("preview-url GitHub reads", () => {
5252
});
5353
});
5454

55+
describe("findPreviewUrlFromChecks pagination (#7779)", () => {
56+
const isStatus = (input: RequestInfo | URL) => /\/status\b/.test(String(input));
57+
const isCheckRuns = (input: RequestInfo | URL) => /\/check-runs\b/.test(String(input));
58+
59+
it("follows Link: rel=next and finds a preview check-run on page 2", async () => {
60+
// Page 1 carries only an unrelated check-run; the Cloudflare Workers Builds check-run (with the preview
61+
// link) is on page 2 -- the exact >100-check-runs case a page-1-only read used to miss.
62+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
63+
if (isStatus(input)) return Response.json({ statuses: [] });
64+
if (isCheckRuns(input) && isPage2(input)) {
65+
return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://pr-1.app.workers.dev" }] });
66+
}
67+
// page 1: an unrelated passing check with no preview link, advertising a next page
68+
return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://ci.example.com/run/1" }] }, { headers: { link: NEXT_LINK } });
69+
});
70+
vi.stubGlobal("fetch", fetchMock);
71+
72+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBe("https://pr-1.app.workers.dev");
73+
// Proves it actually walked to page 2 (the whole point of the fix) rather than stopping at page 1.
74+
expect(fetchMock.mock.calls.some((c) => isCheckRuns(c[0] as RequestInfo | URL) && isPage2(c[0] as RequestInfo | URL))).toBe(true);
75+
});
76+
77+
it("returns null when every page advertises more but none carries check_runs (bounded walk)", async () => {
78+
// Payloads with no `check_runs` field exercise the empty-page fallback; the always-next Link can't loop
79+
// unboundedly (findAcrossPages caps the walk), so this resolves to null instead of hanging.
80+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
81+
if (isStatus(input)) return Response.json({ statuses: [] });
82+
return Response.json({}, { headers: { link: NEXT_LINK } });
83+
});
84+
vi.stubGlobal("fetch", fetchMock);
85+
86+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBeNull();
87+
});
88+
89+
it("skips a failed check-run and still finds the preview link on page 1 without an extra request", async () => {
90+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
91+
if (isStatus(input)) return Response.json({ statuses: [] });
92+
return Response.json({
93+
check_runs: [
94+
{ status: "completed", conclusion: "failure", details_url: "https://pr-9.app.workers.dev" }, // failed → skipped, its URL must NOT win
95+
{ status: "completed", conclusion: "success", details_url: "https://pr-real.app.workers.dev" },
96+
],
97+
});
98+
});
99+
vi.stubGlobal("fetch", fetchMock);
100+
101+
await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBe("https://pr-real.app.workers.dev");
102+
expect(fetchMock.mock.calls.some((c) => isPage2(c[0] as RequestInfo | URL))).toBe(false);
103+
});
104+
});
105+
55106
describe("preview-url pagination (#7450)", () => {
56107
it("findPreviewUrlFromPrComments follows Link: rel=next and finds the bot comment on page 2", async () => {
57108
const page1 = Array.from({ length: 100 }, (_v, i) => ({ user: { login: `user${i}` }, body: "just chatter" }));

0 commit comments

Comments
 (0)