Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 16 additions & 7 deletions src/review/visual/preview-url.ts
Original file line number Diff line number Diff line change
Expand Up @@ -243,15 +243,24 @@ export async function findPreviewUrlFromChecks(params: {
const url = extractPreviewUrl(status.target_url);
if (url) return url;
}
const checks = await githubJson<{ check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> }>(
// Walk every page of check-runs (#7779): a head SHA with >100 check-runs can push the Cloudflare Workers
// Builds check-run onto page 2+, and a page-1-only read would then miss it -- the same failure the file's
// header reasons about, already handled by getPreviewBuildState/findPreviewUrlFromPrComments for this
// identical endpoint via findAcrossPages.
const checkUrl = await findAcrossPages<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }, string>(
`${base}/commits/${encodeURIComponent(params.sha)}/check-runs?per_page=100`,
opts,
).catch(() => null);
for (const run of checks?.check_runs ?? []) {
if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue;
const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text);
if (url) return url;
}
(payload) => (payload as { check_runs?: Array<{ status?: string; conclusion?: string; details_url?: string; output?: { summary?: string; text?: string } }> })?.check_runs ?? [],
(runs) => {
for (const run of runs) {
if (run.status === "completed" && run.conclusion && run.conclusion !== "success") continue;
const url = extractPreviewUrl(run.details_url) ?? extractPreviewUrl(run.output?.summary) ?? extractPreviewUrl(run.output?.text);
if (url) return url;
}
return null;
},
);
if (checkUrl) return checkUrl;
} catch (error) {
console.log(JSON.stringify({ event: "preview_from_checks_error", repo: `${params.repo.owner}/${params.repo.repo}`, message: String(error).slice(0, 200) }));
}
Expand Down
53 changes: 52 additions & 1 deletion test/unit/preview-url.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import { afterEach, describe, expect, it, vi } from "vitest";
import { clearGitHubResponseCacheForTest, githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client";
import { extractPreviewUrl, findPreviewUrlFromPrComments, getLatestDeploymentStatus, getPreviewBuildState } from "../../src/review/visual/preview-url";
import { extractPreviewUrl, findPreviewUrlFromChecks, 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"';
Expand Down Expand Up @@ -52,6 +52,57 @@ describe("preview-url GitHub reads", () => {
});
});

describe("findPreviewUrlFromChecks pagination (#7779)", () => {
const isStatus = (input: RequestInfo | URL) => /\/status\b/.test(String(input));
const isCheckRuns = (input: RequestInfo | URL) => /\/check-runs\b/.test(String(input));

it("follows Link: rel=next and finds a preview check-run on page 2", async () => {
// Page 1 carries only an unrelated check-run; the Cloudflare Workers Builds check-run (with the preview
// link) is on page 2 -- the exact >100-check-runs case a page-1-only read used to miss.
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
if (isStatus(input)) return Response.json({ statuses: [] });
if (isCheckRuns(input) && isPage2(input)) {
return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://pr-1.app.workers.dev" }] });
}
// page 1: an unrelated passing check with no preview link, advertising a next page
return Response.json({ check_runs: [{ status: "completed", conclusion: "success", details_url: "https://ci.example.com/run/1" }] }, { headers: { link: NEXT_LINK } });
});
vi.stubGlobal("fetch", fetchMock);

await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBe("https://pr-1.app.workers.dev");
// Proves it actually walked to page 2 (the whole point of the fix) rather than stopping at page 1.
expect(fetchMock.mock.calls.some((c) => isCheckRuns(c[0] as RequestInfo | URL) && isPage2(c[0] as RequestInfo | URL))).toBe(true);
});

it("returns null when every page advertises more but none carries check_runs (bounded walk)", async () => {
// Payloads with no `check_runs` field exercise the empty-page fallback; the always-next Link can't loop
// unboundedly (findAcrossPages caps the walk), so this resolves to null instead of hanging.
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
if (isStatus(input)) return Response.json({ statuses: [] });
return Response.json({}, { headers: { link: NEXT_LINK } });
});
vi.stubGlobal("fetch", fetchMock);

await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBeNull();
});

it("skips a failed check-run and still finds the preview link on page 1 without an extra request", async () => {
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
if (isStatus(input)) return Response.json({ statuses: [] });
return Response.json({
check_runs: [
{ status: "completed", conclusion: "failure", details_url: "https://pr-9.app.workers.dev" }, // failed → skipped, its URL must NOT win
{ status: "completed", conclusion: "success", details_url: "https://pr-real.app.workers.dev" },
],
});
});
vi.stubGlobal("fetch", fetchMock);

await expect(findPreviewUrlFromChecks({ token: "t", repo: REPO, sha: "abc123" })).resolves.toBe("https://pr-real.app.workers.dev");
expect(fetchMock.mock.calls.some((c) => isPage2(c[0] as RequestInfo | URL))).toBe(false);
});
});

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" }));
Expand Down