diff --git a/src/review/visual/preview-url.ts b/src/review/visual/preview-url.ts index d3b9c85a4b..3f3a18c985 100644 --- a/src/review/visual/preview-url.ts +++ b/src/review/visual/preview-url.ts @@ -38,12 +38,12 @@ class PreviewGitHubError extends Error { } } -/** Minimal fetch→JSON helper (mirrors reviewbot's core/github.ts githubJson). Throws PreviewGitHubError on a - * non-2xx so callers can distinguish a 404 ("no deployments") from a transient outage. */ -async function githubJson( - url: string, - init: { token?: string | undefined; apiVersion?: string | undefined; rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined } = {}, -): Promise { +type GithubJsonInit = { token?: string | undefined; apiVersion?: string | undefined; rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined }; + +/** Minimal fetch→JSON helper that also surfaces the response's `Link` header for pagination (mirrors + * reviewbot's core/github.ts githubJson). Throws PreviewGitHubError on a non-2xx so callers can distinguish + * a 404 ("no deployments") from a transient outage. */ +async function githubJsonWithLink(url: string, init: GithubJsonInit = {}): Promise<{ payload: T; link: string | null }> { const headers = new Headers(); headers.set("accept", "application/vnd.github+json"); headers.set("user-agent", PRODUCT_USER_AGENT); @@ -68,7 +68,50 @@ async function githubJson( const message = typeof (payload as { message?: string })?.message === "string" ? (payload as { message: string }).message : `GitHub ${response.status}`; throw new PreviewGitHubError(response.status, message); } - return payload as T; + return { payload: payload as T, link: response.headers.get("link") }; +} + +async function githubJson(url: string, init: GithubJsonInit = {}): Promise { + return (await githubJsonWithLink(url, init)).payload; +} + +// GitHub caps list endpoints at 100 items/page, so a single `per_page=100` read silently truncates: a PR with +// >100 discussion comments, or a commit with >100 check-runs, would push the Cloudflare Workers Builds bot's +// comment / check-run onto page 2+ and this discovery would then return null/"absent" as if it genuinely +// didn't exist (a truncated page-1 response is indistinguishable from an empty one). Walk the `Link: rel="next"` +// header instead, bounded so a pathological PR/commit (or a mock that always advertises a next page) can't turn +// one read into an unbounded fetch loop -- mirrors src/github/backfill.ts's githubPaginatedList/PR_DETAIL_MAX_PAGES +// and src/github/app.ts's workflow-run listing (MAX_WORKFLOW_RUN_LIST_PAGES), both bounded to 10. +const PREVIEW_LIST_MAX_PAGES = 10; + +function hasNextPage(link: string | null): boolean { + return Boolean(link?.split(",").some((part) => /rel="next"/.test(part))); +} + +/** + * Walk a GitHub list endpoint's `Link: rel="next"` pages, probing each page's items as it arrives and + * returning the first non-null probe result. Bounded to PREVIEW_LIST_MAX_PAGES (see the note above) so a + * pathological resource can never spin. A page fetch/parse failure propagates to the caller, whose own + * try/catch degrades it to null/"absent" -- earlier pages were already probed, so a mid-pagination failure + * falls back to what they yielded (nothing usable) rather than dropping a successful first page, mirroring + * githubPaginatedList's own "a later-page failure keeps the pages already fetched" contract. + */ +async function findAcrossPages( + firstPageUrl: string, + init: GithubJsonInit, + selectItems: (payload: unknown) => TItem[], + probe: (items: TItem[]) => TResult | null, +): Promise { + for (let page = 1; page <= PREVIEW_LIST_MAX_PAGES; page += 1) { + // Callers pass a `per_page=100` first-page URL; append the 1-based page cursor for page 2+ only (page 1 is + // GitHub's default, so leaving it bare keeps that request byte-identical to the pre-pagination read). + const url = page === 1 ? firstPageUrl : `${firstPageUrl}&page=${page}`; + const { payload, link } = await githubJsonWithLink(url, init); + const found = probe(selectItems(payload)); + if (found !== null) return found; + if (!hasNextPage(link)) return null; + } + return null; } export type DeploymentLookup = { url: string | null; failed: boolean; error?: boolean }; @@ -215,22 +258,26 @@ export async function findPreviewUrlFromPrComments(params: { rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined; }): Promise { const base = `https://api.github.com/repos/${params.repo.owner}/${params.repo.repo}`; + const opts = { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey }; try { - const comments = await githubJson>( + return await findAcrossPages<{ user?: { login?: string }; body?: string }, string>( `${base}/issues/${params.prNumber}/comments?per_page=100`, - { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey }, - ).catch(() => null); - if (!Array.isArray(comments)) return null; - // Newest first (the bot edits one comment in place). - for (const c of [...comments].reverse()) { - if ((c.user?.login ?? "").toLowerCase() !== "cloudflare-workers-and-pages[bot]") continue; - const url = extractPreviewUrl(c.body); - if (url) return url; - } + opts, + (payload) => (Array.isArray(payload) ? (payload as Array<{ user?: { login?: string }; body?: string }>) : []), + (comments) => { + // Newest first (the bot edits one comment in place). + for (const c of [...comments].reverse()) { + if ((c.user?.login ?? "").toLowerCase() !== "cloudflare-workers-and-pages[bot]") continue; + const url = extractPreviewUrl(c.body); + if (url) return url; + } + return null; + }, + ); } catch (error) { console.log(JSON.stringify({ event: "preview_from_comments_error", repo: `${params.repo.owner}/${params.repo.repo}`, message: String(error).slice(0, 200) })); + return null; } - return null; } /** @@ -247,15 +294,20 @@ export async function getPreviewBuildState(params: { rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined; }): Promise<"building" | "succeeded" | "failed" | "absent"> { const base = `https://api.github.com/repos/${params.repo.owner}/${params.repo.repo}`; + const opts = { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey }; try { - const checks = await githubJson<{ check_runs?: Array<{ name?: string; status?: string; conclusion?: string }> }>( + const state = await findAcrossPages<{ name?: string; status?: string; conclusion?: string }, "building" | "succeeded" | "failed">( `${base}/commits/${encodeURIComponent(params.sha)}/check-runs?per_page=100`, - { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey }, - ).catch(() => null); - const build = (checks?.check_runs ?? []).find((r) => /workers builds|cloudflare/i.test(r.name ?? "")); - if (!build) return "absent"; - if (build.status !== "completed") return "building"; // queued / in_progress → the preview is coming - return build.conclusion === "success" ? "succeeded" : "failed"; + opts, + (payload) => (payload as { check_runs?: Array<{ name?: string; status?: string; conclusion?: string }> })?.check_runs ?? [], + (runs) => { + const build = runs.find((r) => /workers builds|cloudflare/i.test(r.name ?? "")); + if (!build) return null; // not on this page — keep walking until found, Link exhausts, or the page bound + if (build.status !== "completed") return "building"; // queued / in_progress → the preview is coming + return build.conclusion === "success" ? "succeeded" : "failed"; + }, + ); + return state ?? "absent"; } catch { return "absent"; } diff --git a/test/unit/preview-url.test.ts b/test/unit/preview-url.test.ts index 55cec16272..19b91c8e8d 100644 --- a/test/unit/preview-url.test.ts +++ b/test/unit/preview-url.test.ts @@ -1,6 +1,11 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { clearGitHubResponseCacheForTest, githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client"; -import { extractPreviewUrl, getPreviewBuildState } from "../../src/review/visual/preview-url"; +import { extractPreviewUrl, findPreviewUrlFromPrComments, 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"'; +const REPO = { owner: "o", repo: "r" }; +const isPage2 = (input: RequestInfo | URL) => /[?&]page=2\b/.test(String(input)); afterEach(() => { clearGitHubResponseCacheForTest(); @@ -47,6 +52,122 @@ describe("preview-url GitHub reads", () => { }); }); +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" })); + const page2 = [{ user: { login: "cloudflare-workers-and-pages[bot]" }, body: "Preview ready: https://pr-9.app.workers.dev/route" }]; + const fetchMock = vi.fn(async (input: RequestInfo | URL) => + isPage2(input) ? Response.json(page2) : Response.json(page1, { headers: { link: NEXT_LINK } }), + ); + vi.stubGlobal("fetch", fetchMock); + + await expect(findPreviewUrlFromPrComments({ token: "t", repo: REPO, prNumber: 9 })).resolves.toBe("https://pr-9.app.workers.dev"); + expect(fetchMock).toHaveBeenCalledTimes(2); + expect(String(fetchMock.mock.calls[0]![0])).toContain("/issues/9/comments?per_page=100"); + expect(String(fetchMock.mock.calls[0]![0])).not.toContain("&page="); // page 1 stays the bare pre-pagination read + expect(String(fetchMock.mock.calls[1]![0])).toContain("&page=2"); + }); + + it("findPreviewUrlFromPrComments stops as soon as the bot comment is found, without fetching further pages", async () => { + const fetchMock = vi.fn(async () => + Response.json([{ user: { login: "cloudflare-workers-and-pages[bot]" }, body: "https://pr-1.app.workers.dev" }], { headers: { link: NEXT_LINK } }), + ); + vi.stubGlobal("fetch", fetchMock); + await expect(findPreviewUrlFromPrComments({ token: "t", repo: REPO, prNumber: 1 })).resolves.toBe("https://pr-1.app.workers.dev"); + expect(fetchMock).toHaveBeenCalledTimes(1); // early exit despite the advertised next page + }); + + it("findPreviewUrlFromPrComments returns null when no bot comment exists and there is no next page", async () => { + vi.stubGlobal("fetch", async () => Response.json([{ user: { login: "someone" }, body: "hi" }])); + await expect(findPreviewUrlFromPrComments({ token: "t", repo: REPO, prNumber: 2 })).resolves.toBeNull(); + }); + + it("findPreviewUrlFromPrComments treats a non-array comments payload as empty", async () => { + vi.stubGlobal("fetch", async () => Response.json({ message: "unexpected shape" })); + await expect(findPreviewUrlFromPrComments({ token: "t", repo: REPO, prNumber: 5 })).resolves.toBeNull(); + }); + + it("findPreviewUrlFromPrComments degrades to null when a later-page fetch fails, never throwing", async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + if (isPage2(input)) throw new Error("network down"); + return Response.json([{ user: { login: "x" }, body: "hi" }], { headers: { link: NEXT_LINK } }); + }); + vi.stubGlobal("fetch", fetchMock); + await expect(findPreviewUrlFromPrComments({ token: "t", repo: REPO, prNumber: 3 })).resolves.toBeNull(); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("findPreviewUrlFromPrComments is bounded: a pathological always-Link:next response can't loop unboundedly", async () => { + const fetchMock = vi.fn(async () => Response.json([{ user: { login: "x" }, body: "hi" }], { headers: { link: NEXT_LINK } })); + vi.stubGlobal("fetch", fetchMock); + await expect(findPreviewUrlFromPrComments({ token: "t", repo: REPO, prNumber: 4 })).resolves.toBeNull(); + expect(fetchMock).toHaveBeenCalledTimes(10); // PREVIEW_LIST_MAX_PAGES + }); + + it("findPreviewUrlFromPrComments skips a user-less comment and a bot comment with no preview link, then returns the real one", async () => { + // Order matters: the scan reverses each page (newest first), so the url-bearing bot comment (index 0) is + // examined LAST -- the user-less comment and the link-less bot comment are examined first. + vi.stubGlobal("fetch", async () => + Response.json([ + { user: { login: "cloudflare-workers-and-pages[bot]" }, body: "Preview: https://pr-7.app.workers.dev" }, + { user: { login: "cloudflare-workers-and-pages[bot]" }, body: "build started, no link yet" }, // bot, no URL -> if(url) is false + { body: "a comment with no user object at all" }, // user absent -> `c.user?.login ?? ""` is "" + ]), + ); + await expect(findPreviewUrlFromPrComments({ token: "t", repo: REPO, prNumber: 7 })).resolves.toBe("https://pr-7.app.workers.dev"); + }); + + it("getPreviewBuildState ignores a nameless check-run and still classifies the Workers Builds one", async () => { + vi.stubGlobal("fetch", async () => + Response.json({ + check_runs: [ + { status: "completed", conclusion: "success" }, // no name -> `r.name ?? ""` -> regex miss + { name: "Cloudflare Workers Builds", status: "completed", conclusion: "success" }, + ], + }), + ); + await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "nameless" })).resolves.toBe("succeeded"); + }); + + it("getPreviewBuildState follows Link: rel=next and finds the Workers Builds check on page 2", async () => { + const page1 = { check_runs: Array.from({ length: 100 }, () => ({ name: "unit tests", status: "completed", conclusion: "success" })) }; + const page2 = { check_runs: [{ name: "Cloudflare Workers Builds", status: "in_progress" }] }; + const fetchMock = vi.fn(async (input: RequestInfo | URL) => + isPage2(input) ? Response.json(page2) : Response.json(page1, { headers: { link: NEXT_LINK } }), + ); + vi.stubGlobal("fetch", fetchMock); + await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "abc" })).resolves.toBe("building"); + expect(fetchMock).toHaveBeenCalledTimes(2); + }); + + it("getPreviewBuildState classifies a completed Workers Builds check as succeeded or failed", async () => { + vi.stubGlobal("fetch", async () => Response.json({ check_runs: [{ name: "cloudflare pages", status: "completed", conclusion: "success" }] })); + await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "s1" })).resolves.toBe("succeeded"); + vi.stubGlobal("fetch", async () => Response.json({ check_runs: [{ name: "cloudflare pages", status: "completed", conclusion: "failure" }] })); + await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "s2" })).resolves.toBe("failed"); + }); + + it("getPreviewBuildState treats a payload without a check_runs array as absent", async () => { + vi.stubGlobal("fetch", async () => Response.json({})); + await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "s3" })).resolves.toBe("absent"); + }); + + it("getPreviewBuildState is bounded and degrades to absent on a later-page failure", async () => { + const spin = vi.fn(async () => Response.json({ check_runs: [] }, { headers: { link: NEXT_LINK } })); + vi.stubGlobal("fetch", spin); + await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "spin" })).resolves.toBe("absent"); + expect(spin).toHaveBeenCalledTimes(10); // PREVIEW_LIST_MAX_PAGES + + const failLater = vi.fn(async (input: RequestInfo | URL) => { + if (isPage2(input)) throw new Error("boom"); + return Response.json({ check_runs: [] }, { headers: { link: NEXT_LINK } }); + }); + vi.stubGlobal("fetch", failLater); + await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "fail" })).resolves.toBe("absent"); + expect(failLater).toHaveBeenCalledTimes(2); + }); +}); + describe("extractPreviewUrl", () => { it.each([ ["null", null],