fix(review): paginate preview-url.ts's PR-comment and check-run GitHub reads

xfodev · xfodev · commit 7b8beb3fba2a · 2026-07-20T13:09:00.000+02:00
findPreviewUrlFromPrComments and getPreviewBuildState each read only one per_page=100 page, so on a PR with >100 comments or a commit with >100 check-runs the Cloudflare Workers Builds bot's preview comment/check-run could land on page 2+ and be silently missed (null/absent as if it did not exist). Both now walk the Link: rel="next" header, bounded to PREVIEW_LIST_MAX_PAGES=10, mirroring the existing githubPaginatedList (backfill.ts) and workflow-run listing (app.ts) precedents. The fail-safe contract is preserved: each function still degrades to null/absent on any failure and a mid-pagination failure falls back to earlier pages, never throwing. Adds regression tests for the page-2 case, early-exit, the page bound against a pathological always-next mock, and mid-page failure. Closes #7450
diff --git 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<T>(
-  url: string,
-  init: { token?: string | undefined; apiVersion?: string | undefined; rateLimitAdmissionKey?: GitHubRateLimitAdmissionKey | undefined } = {},
-): Promise<T> {
+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<T>(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<T>(
     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<T>(url: string, init: GithubJsonInit = {}): Promise<T> {
+  return (await githubJsonWithLink<T>(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<TItem, TResult>(
+  firstPageUrl: string,
+  init: GithubJsonInit,
+  selectItems: (payload: unknown) => TItem[],
+  probe: (items: TItem[]) => TResult | null,
+): Promise<TResult | null> {
+  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<unknown>(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<string | null> {
   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<Array<{ user?: { login?: string }; body?: string }>>(
+    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
@@ -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 = '<https://api.github.com/resource?per_page=100&page=99>; rel="next", <https://api.github.com/resource?per_page=100&page=99>; 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],
