From 654d65afba44ee03adec617195e51a4a81d4a1d4 Mon Sep 17 00:00:00 2001 From: davion-knight Date: Tue, 21 Jul 2026 10:50:03 -0500 Subject: [PATCH] fix(review): paginate check-runs in findPreviewUrlFromChecks 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 --- src/review/visual/preview-url.ts | 23 +++++++++----- test/unit/preview-url.test.ts | 53 +++++++++++++++++++++++++++++++- 2 files changed, 68 insertions(+), 8 deletions(-) diff --git a/src/review/visual/preview-url.ts b/src/review/visual/preview-url.ts index 99713fc1b0..fadc335e24 100644 --- a/src/review/visual/preview-url.ts +++ b/src/review/visual/preview-url.ts @@ -243,15 +243,24 @@ export async function findPreviewUrlFromChecks(params: { const url = extractPreviewUrl(status.target_url); if (url) return url; } - const checks = await githubJson<{ check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> }>( + // Walk every page of check-runs (#7779): a head SHA with >100 check-runs can push the Cloudflare Workers + // Builds check-run onto page 2+, and a page-1-only read would then miss it -- the same failure the file's + // header reasons about, already handled by getPreviewBuildState/findPreviewUrlFromPrComments for this + // identical endpoint via findAcrossPages. + const checkUrl = await findAcrossPages<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }, string>( `${base}/commits/${encodeURIComponent(params.sha)}/check-runs?per_page=100`, opts, - ).catch(() => null); - for (const run of checks?.check_runs ?? []) { - if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue; - const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text); - if (url) return url; - } + (payload) => (payload as { check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> })?.check_runs ?? [], + (runs) => { + for (const run of runs) { + if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue; + const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text); + if (url) return url; + } + return null; + }, + ); + if (checkUrl) return checkUrl; } catch (error) { console.log(JSON.stringify({ event: "preview_from_checks_error", repo: `${params.repo.owner}/${params.repo.repo}`, message: String(error).slice(0, 200) })); } diff --git a/test/unit/preview-url.test.ts b/test/unit/preview-url.test.ts index 9232d66ae1..aef94cd744 100644 --- a/test/unit/preview-url.test.ts +++ b/test/unit/preview-url.test.ts @@ -1,6 +1,6 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { clearGitHubResponseCacheForTest, githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client"; -import { extractPreviewUrl, findPreviewUrlFromPrComments, getLatestDeploymentStatus, getPreviewBuildState } from "../../src/review/visual/preview-url"; +import { extractPreviewUrl, findPreviewUrlFromChecks, findPreviewUrlFromPrComments, getLatestDeploymentStatus, getPreviewBuildState } from "../../src/review/visual/preview-url"; /** GitHub's `Link` header for a page that advertises a next page (the exact shape findAcrossPages walks). */ const NEXT_LINK = '; rel="next", ; rel="last"'; @@ -52,6 +52,57 @@ describe("preview-url GitHub reads", () => { }); }); +describe("findPreviewUrlFromChecks pagination (#7779)", () => { + const isStatus = (input: RequestInfo | URL) => /\/status\b/.test(String(input)); + const isCheckRuns = (input: RequestInfo | URL) => /\/check-runs\b/.test(String(input)); + + it("follows Link: rel=next and finds a preview check-run on page 2", async () => { + // Page 1 carries only an unrelated check-run; the Cloudflare Workers Builds check-run (with the preview + // link) is on page 2 -- the exact >100-check-runs case a page-1-only read used to miss. + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + if (isStatus(input)) return Response.json({ statuses: [] }); + if (isCheckRuns(input) && isPage2(input)) { + return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://pr-1.app.workers.dev" }] }); + } + // page 1: an unrelated passing check with no preview link, advertising a next page + return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://ci.example.com/run/1" }] }, { headers: { link: NEXT_LINK } }); + }); + vi.stubGlobal("fetch", fetchMock); + + await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBe("https://pr-1.app.workers.dev"); + // Proves it actually walked to page 2 (the whole point of the fix) rather than stopping at page 1. + expect(fetchMock.mock.calls.some((c) => isCheckRuns(c[0] as RequestInfo | URL) && isPage2(c[0] as RequestInfo | URL))).toBe(true); + }); + + it("returns null when every page advertises more but none carries check_runs (bounded walk)", async () => { + // Payloads with no `check_runs` field exercise the empty-page fallback; the always-next Link can't loop + // unboundedly (findAcrossPages caps the walk), so this resolves to null instead of hanging. + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + if (isStatus(input)) return Response.json({ statuses: [] }); + return Response.json({}, { headers: { link: NEXT_LINK } }); + }); + vi.stubGlobal("fetch", fetchMock); + + await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBeNull(); + }); + + it("skips a failed check-run and still finds the preview link on page 1 without an extra request", async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + if (isStatus(input)) return Response.json({ statuses: [] }); + return Response.json({ + check_runs: [ + { status: "completed", conclusion: "failure", details_url: "https://pr-9.app.workers.dev" }, // failed → skipped, its URL must NOT win + { status: "completed", conclusion: "success", details_url: "https://pr-real.app.workers.dev" }, + ], + }); + }); + vi.stubGlobal("fetch", fetchMock); + + await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBe("https://pr-real.app.workers.dev"); + expect(fetchMock.mock.calls.some((c) => isPage2(c[0] as RequestInfo | URL))).toBe(false); + }); +}); + describe("preview-url pagination (#7450)", () => { it("findPreviewUrlFromPrComments follows Link: rel=next and finds the bot comment on page 2", async () => { const page1 = Array.from({ length: 100 }, (_v, i) => ({ user: { login: `user${i}` }, body: "just chatter" }));