fix(review): align deployment pagination with findDeploymentUrl helper (#7805)

Co-authored-by: Cursor <cursoragent@cursor.com>

Author: RealDiligent

src/review/visual/preview-url.ts

@@ -139,62 +139,58 @@ export async function getLatestDeploymentStatus(params: {
       : "";
   if (!selector) return { url: null, failed: false };
   const opts = { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey };
-  type DeploymentStatus = { state?: string; environment_url?: string };
-  const selectStatuses = (payload: unknown) => (Array.isArray(payload) ? (payload as DeploymentStatus[]) : []);
-  const probeStatusesForUrl = (statuses: DeploymentStatus[]) => {
-    for (const status of statuses) {
-      const ok = status.state === "success" || status.state === "in_progress";
-      if (ok && status.environment_url) return status.environment_url;
-    }
-    return null;
-  };
-  const inspectDeploymentStatuses = async (deploymentId: number): Promise<{ url: string | null; latestState?: string }> => {
-    let latestState: string | undefined;
-    let capturedLatest = false;
-    const url = await findAcrossPages<DeploymentStatus, string>(
-      `${base}/deployments/${deploymentId}/statuses?per_page=10`,
+
+  // 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,
-      selectStatuses,
+      (payload) => (Array.isArray(payload) ? (payload as Array<{ state?: string; environment_url?: string }>) : []),
       (statuses) => {
-        if (!capturedLatest) {
-          latestState = statuses[0]?.state;
-          capturedLatest = true;
+        for (const status of statuses) {
+          const ok = status.state === "success" || status.state === "in_progress";
+          if (ok && status.environment_url) return status.environment_url;
         }
-        return probeStatusesForUrl(statuses);
+        // 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: deploymentId, message: String(error).slice(0, 200) }));
+      console.log(JSON.stringify({ event: "deployment_status_error", deployment: id, message: String(error).slice(0, 200) }));
       return null;
     });
-    if (url) return { url };
-    return latestState !== undefined ? { url: null, latestState } : { url: null };
-  };
-  let sawFailure = false;
-  let sawPending = false;
+
+  let url: string | null;
   try {
-    const url = await findAcrossPages<{ id?: number }, string>(
+    // 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) => {
-        for (const deployment of deployments) {
-          if (deployment.id == null) continue;
-          const { url: foundUrl, latestState } = await inspectDeploymentStatuses(deployment.id);
-          if (foundUrl) return foundUrl;
-          if (latestState === "failure" || latestState === "error") sawFailure = true;
-          else if (latestState === "in_progress" || latestState === "queued" || latestState === "pending") sawPending = true;
-        }
-        return null;
+        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;
       },
     );
-    if (url) return { url, failed: false };
   } 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 };
   }
+  if (url !== null) return { url, failed: false };
   return { url: null, failed: sawFailure && !sawPending };
 }
 

test/unit/preview-url.test.ts

@@ -313,6 +313,69 @@ describe("preview-url pagination (#7450)", () => {
     });
     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", () => {