diff --git a/src/review/visual/preview-url.ts b/src/review/visual/preview-url.ts index 3f3a18c985..99661fc94d 100644 --- a/src/review/visual/preview-url.ts +++ b/src/review/visual/preview-url.ts @@ -100,14 +100,16 @@ async function findAcrossPages( firstPageUrl: string, init: GithubJsonInit, selectItems: (payload: unknown) => TItem[], - probe: (items: TItem[]) => TResult | null, + // The probe may be async (e.g. it fetches a nested list per item, #7805); a synchronous probe still works + // unchanged, since `await` on a non-promise is a no-op. + probe: (items: TItem[]) => TResult | null | Promise, ): 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)); + const found = await probe(selectItems(payload)); if (found !== null) return found; if (!hasNextPage(link)) return null; } @@ -138,13 +140,51 @@ 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>(`${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. Mirrors getPreviewBuildState. + const findDeploymentUrl = (id: number): Promise => + findAcrossPages<{ state?: string; environment_url?: string }, string>( + `${base}/deployments/${id}/statuses?per_page=100`, + 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=100`, + 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. @@ -152,30 +192,7 @@ export async function getLatestDeploymentStatus(params: { 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>(`${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 }; } diff --git a/test/unit/preview-url.test.ts b/test/unit/preview-url.test.ts index 19b91c8e8d..8eca5e3ed9 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, 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 = '; rel="next", ; rel="last"'; @@ -166,6 +166,190 @@ describe("preview-url pagination (#7450)", () => { await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "fail" })).resolves.toBe("absent"); expect(failLater).toHaveBeenCalledTimes(2); }); + + it("getLatestDeploymentStatus finds a deployment whose environment_url sits on page 2 of the deployments list (#7805)", async () => { + // Page 1: 100 deployments none of which carry a usable status; page 2: the deployment with the real URL. + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/deployments?")) { + if (isPage2(input)) return Response.json([{ id: 777 }]); + return Response.json( + Array.from({ length: 100 }, (_v, i) => ({ id: i + 1 })), + { headers: { link: NEXT_LINK } }, + ); + } + // Deployment 777's statuses carry the preview URL; every page-1 deployment's statuses are empty. + if (url.includes("/deployments/777/statuses")) { + return Response.json([{ state: "success", environment_url: "https://p2.pages.dev/" }]); + } + return Response.json([]); + }); + vi.stubGlobal("fetch", fetchMock); + await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({ + url: "https://p2.pages.dev/", + failed: false, + }); + // Page 1 + page 2 of deployments were both fetched (the bug was that page 2 was never read). + expect(fetchMock.mock.calls.some((c) => isPage2(c[0]))).toBe(true); + }); + + it("getLatestDeploymentStatus finds a usable status on page 2 of a deployment's statuses (#7805)", async () => { + const fetchMock = vi.fn(async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/deployments?")) return Response.json([{ id: 42 }]); + if (url.includes("/deployments/42/statuses")) { + if (isPage2(input)) return Response.json([{ state: "success", environment_url: "https://s2.workers.dev/" }]); + // Page 1: 100 older statuses, none usable, advertising a next page. + return Response.json( + Array.from({ length: 100 }, () => ({ state: "queued" })), + { headers: { link: NEXT_LINK } }, + ); + } + return Response.json([]); + }); + vi.stubGlobal("fetch", fetchMock); + await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({ + url: "https://s2.workers.dev/", + failed: false, + }); + expect(fetchMock.mock.calls.some((c) => String(c[0]).includes("/statuses") && isPage2(c[0]))).toBe(true); + }); + + it("getLatestDeploymentStatus reports failed when a deployment's latest status is a failure and none are pending (#7805)", async () => { + // No usable environment_url anywhere, and the single deployment's latest (statuses[0]) is a hard failure: + // sawFailure is set, sawPending stays false, so the terminal verdict is failed:true. + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/deployments?")) return Response.json([{ id: 5 }]); + if (url.includes("/deployments/5/statuses")) return Response.json([{ state: "failure" }, { state: "success" }]); + return Response.json([]); + }); + await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({ + url: null, + failed: true, + }); + }); + + it("getLatestDeploymentStatus is NOT failed when another deployment is still pending, even if one failed (#7805)", async () => { + // Two deployments: one failed, one still in_progress. sawPending being set suppresses the failed verdict, so + // the caller keeps polling rather than declaring a false terminal failure. + 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: "abc" })).resolves.toEqual({ + url: null, + failed: false, + }); + }); + + it("getLatestDeploymentStatus swallows a per-deployment statuses-fetch error and treats that deployment as URL-less (#7805)", async () => { + // A statuses read throwing must not abort the whole lookup: findDeploymentUrl's .catch degrades that one + // deployment to null, so the overall result is a clean "no URL yet, not failed". + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/deployments?")) return Response.json([{ id: 9 }]); + if (url.includes("/deployments/9/statuses")) throw new Error("statuses network down"); + return Response.json([]); + }); + await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({ + url: null, + failed: false, + }); + }); + + it("getLatestDeploymentStatus skips a deployment whose id is absent, leaving no statuses to inspect (#7805)", async () => { + // The id filter drops an id-less deployment entry, so there is nothing to fetch statuses for -> no URL, not + // failed. (A statuses fetch for a real id is never issued here.) + 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: "abc" })).resolves.toEqual({ + url: null, + failed: false, + }); + }); + + it("getLatestDeploymentStatus treats a non-array deployments payload as an empty page (#7805)", async () => { + // Array.isArray(payload) ? ... : [] -- the deployments list read returning a non-array object degrades to an + // empty page rather than throwing. + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/deployments?")) return Response.json({ message: "unexpected non-array deployments shape" }); + return Response.json([]); + }); + await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({ + url: null, + failed: false, + }); + }); + + it("getLatestDeploymentStatus treats a non-array statuses payload for a deployment as empty (#7805)", async () => { + // The inner Array.isArray(payload) ? ... : [] guard: a deployment's /statuses returning a non-array object is + // treated as "no statuses", so that deployment contributes no URL and no failed/pending signal. + 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: "abc" })).resolves.toEqual({ + url: null, + failed: false, + }); + }); + + it("getLatestDeploymentStatus builds a ref-scoped deployments query when given a ref instead of a sha (#7805)", async () => { + // The selector ternary's ref arm: with params.ref (and no sha) the deployments list is queried by ref=..., and + // a usable status still resolves to its environment_url. + 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, + }); + // The deployments query was scoped by ref=..., not sha=... + expect(fetchMock.mock.calls.some((c) => String(c[0]).includes("/deployments?ref=feature-branch"))).toBe(true); + }); + + it("getLatestDeploymentStatus returns error:true when the deployments read fails with a non-404 status (#7805)", async () => { + // A 403/5xx (not a 404 "genuinely no deployments") must surface error:true so the caller keeps polling rather + // than showing a false terminal "no preview" state. + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/deployments?")) return new Response("forbidden", { status: 403 }); + return Response.json([]); + }); + await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({ + url: null, + failed: false, + error: true, + }); + }); + + it("getLatestDeploymentStatus returns no-preview (not error) when the deployments read 404s (#7805)", async () => { + // A 404 means the ref genuinely has no deployments -> a clean {url:null, failed:false}, distinct from the + // non-404 error path above. + vi.stubGlobal("fetch", async (input: RequestInfo | URL) => { + const url = String(input); + if (url.includes("/deployments?")) return new Response("not found", { status: 404 }); + return Response.json([]); + }); + await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({ + url: null, + failed: false, + }); + }); }); describe("extractPreviewUrl", () => {