fix(review): paginate getLatestDeploymentStatus deployments and statuses (#7805)

src/review/visual/preview-url.ts

@@ -100,14 +100,14 @@ async function findAcrossPages<TItem, TResult>(
   firstPageUrl: string,
   init: GithubJsonInit,
   selectItems: (payload: unknown) => TItem[],
-  probe: (items: TItem[]) => TResult | null,
+  probe: (items: TItem[]) => TResult | null | Promise<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));
+    const found = await probe(selectItems(payload));
     if (found !== null) return found;
     if (!hasNextPage(link)) return null;
   }
@@ -138,44 +138,59 @@ export async function getLatestDeploymentStatus(params: {
       ? `ref=${encodeURIComponent(params.ref)}`
       : "";
   if (!selector) return { url: null, failed: false };
-  let deployments: Array<{ id?: number }>;
-  try {
-    deployments = await githubJson<Array<{ id?: number }>>(`${base}/deployments?${selector}&per_page=10`, {
-      token: params.token,
-      apiVersion: params.apiVersion,
-      rateLimitAdmissionKey: params.rateLimitAdmissionKey,
+  const opts = { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey };
+
+  // sawFailure/sawPending accumulate across every deployment (and every page of them), so the final
+  // failed-vs-still-coming verdict reflects all deployments, not just the first page (#7805).
+  let sawFailure = false;
+  let sawPending = false;
+
+  // Scan one deployment's statuses across ALL pages (#7805): return its environment_url when a usable status is
+  // found, else null after recording whether its latest status looked failed/pending.
+  const findDeploymentUrl = (id: number): Promise<string | null> =>
+    findAcrossPages<{ state?: string; environment_url?: string }, string>(
+      `${base}/deployments/${id}/statuses?per_page=10`,
+      opts,
+      (payload) => (Array.isArray(payload) ? (payload as Array<{ state?: string; environment_url?: string }>) : []),
+      (statuses) => {
+        for (const status of statuses) {
+          const ok = status.state === "success" || status.state === "in_progress";
+          if (ok && status.environment_url) return status.environment_url;
+        }
+        // Only the first page's first entry is GitHub's "latest" status; later pages are older, so the
+        // failed/pending bookkeeping keys off statuses[0] exactly as the pre-pagination single-page read did.
+        const latest = statuses[0]?.state;
+        if (latest === "failure" || latest === "error") sawFailure = true;
+        else if (latest === "in_progress" || latest === "queued" || latest === "pending") sawPending = true;
+        return null;
+      },
+    ).catch((error) => {
+      console.log(JSON.stringify({ event: "deployment_status_error", deployment: id, message: String(error).slice(0, 200) }));
+      return null;
     });
+
+  let url: string | null;
+  try {
+    // Walk every page of deployments, and on each page scan each deployment's statuses; return the first usable
+    // environment_url found, letting findAcrossPages stop as soon as a page yields one.
+    url = await findAcrossPages<{ id?: number }, string>(
+      `${base}/deployments?${selector}&per_page=10`,
+      opts,
+      (payload) => (Array.isArray(payload) ? (payload as Array<{ id?: number }>) : []),
+      async (deployments) => {
+        const ids = deployments.map((d) => d.id).filter((id): id is number => id != null);
+        const statusUrls = await Promise.all(ids.map((id) => findDeploymentUrl(id)));
+        return statusUrls.find((found): found is string => found !== null) ?? null;
+      },
+    );
   } catch (error) {
     // 404 → the ref genuinely has no deployments. Any other failure (403 missing scope, rate limit, 5xx) is
     // NOT "no preview"; report `error` so the caller keeps polling rather than showing a false terminal state.
     if (error instanceof PreviewGitHubError && error.status === 404) return { url: null, failed: false };
     console.log(JSON.stringify({ event: "deployment_lookup_error", repo: `${params.repo.owner}/${params.repo.repo}`, selector, message: String(error).slice(0, 200) }));
     return { url: null, failed: false, error: true };
   }
-  const ids = deployments.map((d) => d.id).filter((id): id is number => id != null);
-  const statusLists = await Promise.all(
-    ids.map((id) =>
-      githubJson<Array<{ state?: string; environment_url?: string }>>(`${base}/deployments/${id}/statuses?per_page=10`, {
-        token: params.token,
-        apiVersion: params.apiVersion,
-        rateLimitAdmissionKey: params.rateLimitAdmissionKey,
-      }).catch((error) => {
-        console.log(JSON.stringify({ event: "deployment_status_error", deployment: id, message: String(error).slice(0, 200) }));
-        return [] as Array<{ state?: string; environment_url?: string }>;
-      }),
-    ),
-  );
-  let sawFailure = false;
-  let sawPending = false;
-  for (const statuses of statusLists) {
-    for (const status of statuses) {
-      const ok = status.state === "success" || status.state === "in_progress";
-      if (ok && status.environment_url) return { url: status.environment_url, failed: false };
-    }
-    const latest = statuses[0]?.state;
-    if (latest === "failure" || latest === "error") sawFailure = true;
-    else if (latest === "in_progress" || latest === "queued" || latest === "pending") sawPending = true;
-  }
+  if (url !== null) return { url, failed: false };
   return { url: null, failed: sawFailure && !sawPending };
 }
 

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, getPreviewBuildState } from "../../src/review/visual/preview-url";
+import { extractPreviewUrl, 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 = '<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,216 @@ describe("preview-url pagination (#7450)", () => {
     await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "fail" })).resolves.toBe("absent");
     expect(failLater).toHaveBeenCalledTimes(2);
   });
+
+  it("getLatestDeploymentStatus follows Link: rel=next on deployments and finds the preview URL on page 2 (#7805)", async () => {
+    const page1Deployments = Array.from({ length: 10 }, (_v, i) => ({ id: i + 1 }));
+    const page2Deployments = [{ id: 99 }];
+    const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?") && url.includes("sha=abc")) {
+        return isPage2(input)
+          ? Response.json(page2Deployments)
+          : Response.json(page1Deployments, { headers: { link: NEXT_LINK } });
+      }
+      if (url.includes("/deployments/99/statuses")) {
+        return Response.json([{ state: "success", environment_url: "https://pr-99.app.workers.dev" }]);
+      }
+      if (url.includes("/deployments/") && url.includes("/statuses")) {
+        return Response.json([{ state: "failure" }]);
+      }
+      throw new Error(`unexpected fetch: ${url}`);
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({
+      url: "https://pr-99.app.workers.dev",
+      failed: false,
+    });
+    expect(fetchMock.mock.calls.some((c) => /\/deployments\?.*page=2/.test(String(c[0])))).toBe(true);
+    expect(String(fetchMock.mock.calls.find((c) => String(c[0]).includes("/deployments?"))![0])).not.toContain("&page=");
+  });
+
+  it("getLatestDeploymentStatus follows Link: rel=next on deployment statuses and finds environment_url on page 2 (#7805)", async () => {
+    const page1Statuses = Array.from({ length: 10 }, () => ({ state: "pending" }));
+    const page2Statuses = [{ state: "success", environment_url: "https://deep-status.app.workers.dev" }];
+    const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) {
+        return Response.json([{ id: 7 }]);
+      }
+      if (url.includes("/deployments/7/statuses")) {
+        return isPage2(input) ? Response.json(page2Statuses) : Response.json(page1Statuses, { headers: { link: NEXT_LINK } });
+      }
+      throw new Error(`unexpected fetch: ${url}`);
+    });
+    vi.stubGlobal("fetch", fetchMock);
+
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "deep" })).resolves.toEqual({
+      url: "https://deep-status.app.workers.dev",
+      failed: false,
+    });
+    expect(fetchMock).toHaveBeenCalledTimes(3); // deployments list + statuses pages 1 and 2
+    expect(fetchMock.mock.calls.some((c) => /\/deployments\/7\/statuses.*page=2/.test(String(c[0])))).toBe(true);
+  });
+
+  it("getLatestDeploymentStatus returns failed:true when the latest status errored and none are pending", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 1 }]);
+      return Response.json([{ state: "failure" }]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "fail" })).resolves.toEqual({ url: null, failed: true });
+  });
+
+  it("getLatestDeploymentStatus keeps failed:false while a deployment is still pending", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 1 }]);
+      return Response.json([{ state: "pending" }]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "pending" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus treats a 404 deployments list as absent", async () => {
+    vi.stubGlobal("fetch", async () => Response.json({ message: "Not Found" }, { status: 404 }));
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "missing" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus reports error:true on a non-404 deployment lookup failure", async () => {
+    vi.stubGlobal("fetch", async () => Response.json({ message: "rate limited" }, { status: 403 }));
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "rl" })).resolves.toEqual({ url: null, failed: false, error: true });
+  });
+
+  it("getLatestDeploymentStatus skips the GitHub read when neither sha nor ref is provided", async () => {
+    const fetchMock = vi.fn();
+    vi.stubGlobal("fetch", fetchMock);
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO })).resolves.toEqual({ url: null, failed: false });
+    expect(fetchMock).not.toHaveBeenCalled();
+  });
+
+  it("getLatestDeploymentStatus degrades when a deployment statuses fetch throws", async () => {
+    const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 1 }]);
+      throw new Error("status read down");
+    });
+    vi.stubGlobal("fetch", fetchMock);
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "status-down" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus skips deployments without an id and still finds a preview URL", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{}, { id: 2 }]);
+      return Response.json([{ state: "success", environment_url: "https://valid-id.app.workers.dev" }]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "skip-id" })).resolves.toEqual({
+      url: "https://valid-id.app.workers.dev",
+      failed: false,
+    });
+  });
+
+  it("getLatestDeploymentStatus accepts in_progress statuses with an environment_url", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 3 }]);
+      return Response.json([{ state: "in_progress", environment_url: "https://building.app.workers.dev" }]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "building" })).resolves.toEqual({
+      url: "https://building.app.workers.dev",
+      failed: false,
+    });
+  });
+
+  it("getLatestDeploymentStatus treats a latest error status as failed when nothing is pending", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 4 }]);
+      return Response.json([{ state: "error" }]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "error-state" })).resolves.toEqual({ url: null, failed: true });
+  });
+
+  it("getLatestDeploymentStatus keeps failed:false when the latest status is still in_progress without a URL", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 5 }]);
+      return Response.json([{ state: "in_progress" }]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "in-progress" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus treats non-array deployment and status payloads as empty", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json({ message: "unexpected" });
+      return Response.json({ message: "unexpected" });
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "shape" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus keeps failed:false when one deployment failed but another is still pending", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 1 }, { id: 2 }]);
+      if (url.includes("/deployments/1/statuses")) return Response.json([{ state: "error" }]);
+      if (url.includes("/deployments/2/statuses")) return Response.json([{ state: "in_progress" }]);
+      return Response.json([]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "mixed" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus skips deployments without an id", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ notAnId: true }]);
+      return Response.json([]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "no-id" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus builds a ref-scoped deployments query when given a ref instead of a sha", async () => {
+    const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 8 }]);
+      if (url.includes("/deployments/8/statuses")) return Response.json([{ state: "success", environment_url: "https://ref.pages.dev/" }]);
+      return Response.json([]);
+    });
+    vi.stubGlobal("fetch", fetchMock);
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, ref: "feature-branch" })).resolves.toEqual({
+      url: "https://ref.pages.dev/",
+      failed: false,
+    });
+    expect(fetchMock.mock.calls.some((c) => String(c[0]).includes("/deployments?ref=feature-branch"))).toBe(true);
+  });
+
+  it("getLatestDeploymentStatus treats a non-array statuses payload for a deployment as empty", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 3 }]);
+      if (url.includes("/deployments/3/statuses")) return Response.json({ message: "unexpected non-array statuses shape" });
+      return Response.json([]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "bad-statuses" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus keeps failed:false when the latest status is queued", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 6 }]);
+      return Response.json([{ state: "queued" }]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, ref: "feature/x" })).resolves.toEqual({ url: null, failed: false });
+  });
+
+  it("getLatestDeploymentStatus treats an empty statuses page as absent without a latest state", async () => {
+    vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
+      const url = String(input);
+      if (url.includes("/deployments?")) return Response.json([{ id: 8 }]);
+      return Response.json([]);
+    });
+    await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "empty-statuses" })).resolves.toEqual({ url: null, failed: false });
+  });
 });
 
 describe("extractPreviewUrl", () => {