Skip to content

Commit fa21f3e

Browse files
kai392RealDiligentcursoragent
authored
fix(review): paginate getLatestDeploymentStatus deployments and statuses (#7805) (#7843)
* fix(review): paginate getLatestDeploymentStatus deployments and statuses (#7805) Reuse findAcrossPages for both GitHub list reads so preview URL discovery does not silently miss deployments or statuses beyond page 1. Closes #7805 Co-authored-by: Cursor <cursoragent@cursor.com> * fix(review): capture latest deployment status from first statuses page only Avoid a redundant page-1 refetch after pagination and satisfy exactOptionalPropertyTypes for latestState. Co-authored-by: Cursor <cursoragent@cursor.com> * test(review): expect deployments + two status pages in pagination case Co-authored-by: Cursor <cursoragent@cursor.com> * test(review): cover getLatestDeploymentStatus failure and error paths (#7805) Co-authored-by: Cursor <cursoragent@cursor.com> * test(review): cover remaining getLatestDeploymentStatus patch branches Co-authored-by: Cursor <cursoragent@cursor.com> * fix(review): align deployment pagination with findDeploymentUrl helper (#7805) Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: RealDiligent <brave.challenge007@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
1 parent 6ea4e28 commit fa21f3e

2 files changed

Lines changed: 258 additions & 33 deletions

File tree

‎src/review/visual/preview-url.ts‎

Lines changed: 47 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -100,14 +100,14 @@ async function findAcrossPages<TItem, TResult>(
100100
firstPageUrl: string,
101101
init: GithubJsonInit,
102102
selectItems: (payload: unknown) => TItem[],
103-
probe: (items: TItem[]) => TResult | null,
103+
probe: (items: TItem[]) => TResult | null | Promise<TResult | null>,
104104
): Promise<TResult | null> {
105105
for (let page = 1; page <= PREVIEW_LIST_MAX_PAGES; page += 1) {
106106
// Callers pass a `per_page=100` first-page URL; append the 1-based page cursor for page 2+ only (page 1 is
107107
// GitHub's default, so leaving it bare keeps that request byte-identical to the pre-pagination read).
108108
const url = page === 1 ? firstPageUrl : `${firstPageUrl}&page=${page}`;
109109
const { payload, link } = await githubJsonWithLink<unknown>(url, init);
110-
const found = probe(selectItems(payload));
110+
const found = await probe(selectItems(payload));
111111
if (found !== null) return found;
112112
if (!hasNextPage(link)) return null;
113113
}
@@ -138,44 +138,59 @@ export async function getLatestDeploymentStatus(params: {
138138
? `ref=${encodeURIComponent(params.ref)}`
139139
: "";
140140
if (!selector) return { url: null, failed: false };
141-
let deployments: Array<{ id?: number }>;
142-
try {
143-
deployments = await githubJson<Array<{ id?: number }>>(`${base}/deployments?${selector}&per_page=10`, {
144-
token: params.token,
145-
apiVersion: params.apiVersion,
146-
rateLimitAdmissionKey: params.rateLimitAdmissionKey,
141+
const opts = { token: params.token, apiVersion: params.apiVersion, rateLimitAdmissionKey: params.rateLimitAdmissionKey };
142+
143+
// sawFailure/sawPending accumulate across every deployment (and every page of them), so the final
144+
// failed-vs-still-coming verdict reflects all deployments, not just the first page (#7805).
145+
let sawFailure = false;
146+
let sawPending = false;
147+
148+
// Scan one deployment's statuses across ALL pages (#7805): return its environment_url when a usable status is
149+
// found, else null after recording whether its latest status looked failed/pending.
150+
const findDeploymentUrl = (id: number): Promise<string | null> =>
151+
findAcrossPages<{ state?: string; environment_url?: string }, string>(
152+
`${base}/deployments/${id}/statuses?per_page=10`,
153+
opts,
154+
(payload) => (Array.isArray(payload) ? (payload as Array<{ state?: string; environment_url?: string }>) : []),
155+
(statuses) => {
156+
for (const status of statuses) {
157+
const ok = status.state === "success" || status.state === "in_progress";
158+
if (ok && status.environment_url) return status.environment_url;
159+
}
160+
// Only the first page's first entry is GitHub's "latest" status; later pages are older, so the
161+
// failed/pending bookkeeping keys off statuses[0] exactly as the pre-pagination single-page read did.
162+
const latest = statuses[0]?.state;
163+
if (latest === "failure" || latest === "error") sawFailure = true;
164+
else if (latest === "in_progress" || latest === "queued" || latest === "pending") sawPending = true;
165+
return null;
166+
},
167+
).catch((error) => {
168+
console.log(JSON.stringify({ event: "deployment_status_error", deployment: id, message: String(error).slice(0, 200) }));
169+
return null;
147170
});
171+
172+
let url: string | null;
173+
try {
174+
// Walk every page of deployments, and on each page scan each deployment's statuses; return the first usable
175+
// environment_url found, letting findAcrossPages stop as soon as a page yields one.
176+
url = await findAcrossPages<{ id?: number }, string>(
177+
`${base}/deployments?${selector}&per_page=10`,
178+
opts,
179+
(payload) => (Array.isArray(payload) ? (payload as Array<{ id?: number }>) : []),
180+
async (deployments) => {
181+
const ids = deployments.map((d) => d.id).filter((id): id is number => id != null);
182+
const statusUrls = await Promise.all(ids.map((id) => findDeploymentUrl(id)));
183+
return statusUrls.find((found): found is string => found !== null) ?? null;
184+
},
185+
);
148186
} catch (error) {
149187
// 404 → the ref genuinely has no deployments. Any other failure (403 missing scope, rate limit, 5xx) is
150188
// NOT "no preview"; report `error` so the caller keeps polling rather than showing a false terminal state.
151189
if (error instanceof PreviewGitHubError && error.status === 404) return { url: null, failed: false };
152190
console.log(JSON.stringify({ event: "deployment_lookup_error", repo: `${params.repo.owner}/${params.repo.repo}`, selector, message: String(error).slice(0, 200) }));
153191
return { url: null, failed: false, error: true };
154192
}
155-
const ids = deployments.map((d) => d.id).filter((id): id is number => id != null);
156-
const statusLists = await Promise.all(
157-
ids.map((id) =>
158-
githubJson<Array<{ state?: string; environment_url?: string }>>(`${base}/deployments/${id}/statuses?per_page=10`, {
159-
token: params.token,
160-
apiVersion: params.apiVersion,
161-
rateLimitAdmissionKey: params.rateLimitAdmissionKey,
162-
}).catch((error) => {
163-
console.log(JSON.stringify({ event: "deployment_status_error", deployment: id, message: String(error).slice(0, 200) }));
164-
return [] as Array<{ state?: string; environment_url?: string }>;
165-
}),
166-
),
167-
);
168-
let sawFailure = false;
169-
let sawPending = false;
170-
for (const statuses of statusLists) {
171-
for (const status of statuses) {
172-
const ok = status.state === "success" || status.state === "in_progress";
173-
if (ok && status.environment_url) return { url: status.environment_url, failed: false };
174-
}
175-
const latest = statuses[0]?.state;
176-
if (latest === "failure" || latest === "error") sawFailure = true;
177-
else if (latest === "in_progress" || latest === "queued" || latest === "pending") sawPending = true;
178-
}
193+
if (url !== null) return { url, failed: false };
179194
return { url: null, failed: sawFailure && !sawPending };
180195
}
181196

‎test/unit/preview-url.test.ts‎

Lines changed: 211 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { afterEach, describe, expect, it, vi } from "vitest";
22
import { clearGitHubResponseCacheForTest, githubRateLimitAdmissionKeyForInstallation, latestGitHubRestRateLimitObservation } from "../../src/github/client";
3-
import { extractPreviewUrl, findPreviewUrlFromPrComments, getPreviewBuildState } from "../../src/review/visual/preview-url";
3+
import { extractPreviewUrl, findPreviewUrlFromPrComments, getLatestDeploymentStatus, getPreviewBuildState } from "../../src/review/visual/preview-url";
44

55
/** GitHub's `Link` header for a page that advertises a next page (the exact shape findAcrossPages walks). */
66
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)", () => {
166166
await expect(getPreviewBuildState({ token: "t", repo: REPO, sha: "fail" })).resolves.toBe("absent");
167167
expect(failLater).toHaveBeenCalledTimes(2);
168168
});
169+
170+
it("getLatestDeploymentStatus follows Link: rel=next on deployments and finds the preview URL on page 2 (#7805)", async () => {
171+
const page1Deployments = Array.from({ length: 10 }, (_v, i) => ({ id: i + 1 }));
172+
const page2Deployments = [{ id: 99 }];
173+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
174+
const url = String(input);
175+
if (url.includes("/deployments?") && url.includes("sha=abc")) {
176+
return isPage2(input)
177+
? Response.json(page2Deployments)
178+
: Response.json(page1Deployments, { headers: { link: NEXT_LINK } });
179+
}
180+
if (url.includes("/deployments/99/statuses")) {
181+
return Response.json([{ state: "success", environment_url: "https://pr-99.app.workers.dev" }]);
182+
}
183+
if (url.includes("/deployments/") && url.includes("/statuses")) {
184+
return Response.json([{ state: "failure" }]);
185+
}
186+
throw new Error(`unexpected fetch: ${url}`);
187+
});
188+
vi.stubGlobal("fetch", fetchMock);
189+
190+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "abc" })).resolves.toEqual({
191+
url: "https://pr-99.app.workers.dev",
192+
failed: false,
193+
});
194+
expect(fetchMock.mock.calls.some((c) => /\/deployments\?.*page=2/.test(String(c[0])))).toBe(true);
195+
expect(String(fetchMock.mock.calls.find((c) => String(c[0]).includes("/deployments?"))![0])).not.toContain("&page=");
196+
});
197+
198+
it("getLatestDeploymentStatus follows Link: rel=next on deployment statuses and finds environment_url on page 2 (#7805)", async () => {
199+
const page1Statuses = Array.from({ length: 10 }, () => ({ state: "pending" }));
200+
const page2Statuses = [{ state: "success", environment_url: "https://deep-status.app.workers.dev" }];
201+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
202+
const url = String(input);
203+
if (url.includes("/deployments?")) {
204+
return Response.json([{ id: 7 }]);
205+
}
206+
if (url.includes("/deployments/7/statuses")) {
207+
return isPage2(input) ? Response.json(page2Statuses) : Response.json(page1Statuses, { headers: { link: NEXT_LINK } });
208+
}
209+
throw new Error(`unexpected fetch: ${url}`);
210+
});
211+
vi.stubGlobal("fetch", fetchMock);
212+
213+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "deep" })).resolves.toEqual({
214+
url: "https://deep-status.app.workers.dev",
215+
failed: false,
216+
});
217+
expect(fetchMock).toHaveBeenCalledTimes(3); // deployments list + statuses pages 1 and 2
218+
expect(fetchMock.mock.calls.some((c) => /\/deployments\/7\/statuses.*page=2/.test(String(c[0])))).toBe(true);
219+
});
220+
221+
it("getLatestDeploymentStatus returns failed:true when the latest status errored and none are pending", async () => {
222+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
223+
const url = String(input);
224+
if (url.includes("/deployments?")) return Response.json([{ id: 1 }]);
225+
return Response.json([{ state: "failure" }]);
226+
});
227+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "fail" })).resolves.toEqual({ url: null, failed: true });
228+
});
229+
230+
it("getLatestDeploymentStatus keeps failed:false while a deployment is still pending", async () => {
231+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
232+
const url = String(input);
233+
if (url.includes("/deployments?")) return Response.json([{ id: 1 }]);
234+
return Response.json([{ state: "pending" }]);
235+
});
236+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "pending" })).resolves.toEqual({ url: null, failed: false });
237+
});
238+
239+
it("getLatestDeploymentStatus treats a 404 deployments list as absent", async () => {
240+
vi.stubGlobal("fetch", async () => Response.json({ message: "Not Found" }, { status: 404 }));
241+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "missing" })).resolves.toEqual({ url: null, failed: false });
242+
});
243+
244+
it("getLatestDeploymentStatus reports error:true on a non-404 deployment lookup failure", async () => {
245+
vi.stubGlobal("fetch", async () => Response.json({ message: "rate limited" }, { status: 403 }));
246+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "rl" })).resolves.toEqual({ url: null, failed: false, error: true });
247+
});
248+
249+
it("getLatestDeploymentStatus skips the GitHub read when neither sha nor ref is provided", async () => {
250+
const fetchMock = vi.fn();
251+
vi.stubGlobal("fetch", fetchMock);
252+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO })).resolves.toEqual({ url: null, failed: false });
253+
expect(fetchMock).not.toHaveBeenCalled();
254+
});
255+
256+
it("getLatestDeploymentStatus degrades when a deployment statuses fetch throws", async () => {
257+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
258+
const url = String(input);
259+
if (url.includes("/deployments?")) return Response.json([{ id: 1 }]);
260+
throw new Error("status read down");
261+
});
262+
vi.stubGlobal("fetch", fetchMock);
263+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "status-down" })).resolves.toEqual({ url: null, failed: false });
264+
});
265+
266+
it("getLatestDeploymentStatus skips deployments without an id and still finds a preview URL", async () => {
267+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
268+
const url = String(input);
269+
if (url.includes("/deployments?")) return Response.json([{}, { id: 2 }]);
270+
return Response.json([{ state: "success", environment_url: "https://valid-id.app.workers.dev" }]);
271+
});
272+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "skip-id" })).resolves.toEqual({
273+
url: "https://valid-id.app.workers.dev",
274+
failed: false,
275+
});
276+
});
277+
278+
it("getLatestDeploymentStatus accepts in_progress statuses with an environment_url", async () => {
279+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
280+
const url = String(input);
281+
if (url.includes("/deployments?")) return Response.json([{ id: 3 }]);
282+
return Response.json([{ state: "in_progress", environment_url: "https://building.app.workers.dev" }]);
283+
});
284+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "building" })).resolves.toEqual({
285+
url: "https://building.app.workers.dev",
286+
failed: false,
287+
});
288+
});
289+
290+
it("getLatestDeploymentStatus treats a latest error status as failed when nothing is pending", async () => {
291+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
292+
const url = String(input);
293+
if (url.includes("/deployments?")) return Response.json([{ id: 4 }]);
294+
return Response.json([{ state: "error" }]);
295+
});
296+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "error-state" })).resolves.toEqual({ url: null, failed: true });
297+
});
298+
299+
it("getLatestDeploymentStatus keeps failed:false when the latest status is still in_progress without a URL", async () => {
300+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
301+
const url = String(input);
302+
if (url.includes("/deployments?")) return Response.json([{ id: 5 }]);
303+
return Response.json([{ state: "in_progress" }]);
304+
});
305+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "in-progress" })).resolves.toEqual({ url: null, failed: false });
306+
});
307+
308+
it("getLatestDeploymentStatus treats non-array deployment and status payloads as empty", async () => {
309+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
310+
const url = String(input);
311+
if (url.includes("/deployments?")) return Response.json({ message: "unexpected" });
312+
return Response.json({ message: "unexpected" });
313+
});
314+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "shape" })).resolves.toEqual({ url: null, failed: false });
315+
});
316+
317+
it("getLatestDeploymentStatus keeps failed:false when one deployment failed but another is still pending", async () => {
318+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
319+
const url = String(input);
320+
if (url.includes("/deployments?")) return Response.json([{ id: 1 }, { id: 2 }]);
321+
if (url.includes("/deployments/1/statuses")) return Response.json([{ state: "error" }]);
322+
if (url.includes("/deployments/2/statuses")) return Response.json([{ state: "in_progress" }]);
323+
return Response.json([]);
324+
});
325+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "mixed" })).resolves.toEqual({ url: null, failed: false });
326+
});
327+
328+
it("getLatestDeploymentStatus skips deployments without an id", async () => {
329+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
330+
const url = String(input);
331+
if (url.includes("/deployments?")) return Response.json([{ notAnId: true }]);
332+
return Response.json([]);
333+
});
334+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "no-id" })).resolves.toEqual({ url: null, failed: false });
335+
});
336+
337+
it("getLatestDeploymentStatus builds a ref-scoped deployments query when given a ref instead of a sha", async () => {
338+
const fetchMock = vi.fn(async (input: RequestInfo | URL) => {
339+
const url = String(input);
340+
if (url.includes("/deployments?")) return Response.json([{ id: 8 }]);
341+
if (url.includes("/deployments/8/statuses")) return Response.json([{ state: "success", environment_url: "https://ref.pages.dev/" }]);
342+
return Response.json([]);
343+
});
344+
vi.stubGlobal("fetch", fetchMock);
345+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, ref: "feature-branch" })).resolves.toEqual({
346+
url: "https://ref.pages.dev/",
347+
failed: false,
348+
});
349+
expect(fetchMock.mock.calls.some((c) => String(c[0]).includes("/deployments?ref=feature-branch"))).toBe(true);
350+
});
351+
352+
it("getLatestDeploymentStatus treats a non-array statuses payload for a deployment as empty", async () => {
353+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
354+
const url = String(input);
355+
if (url.includes("/deployments?")) return Response.json([{ id: 3 }]);
356+
if (url.includes("/deployments/3/statuses")) return Response.json({ message: "unexpected non-array statuses shape" });
357+
return Response.json([]);
358+
});
359+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "bad-statuses" })).resolves.toEqual({ url: null, failed: false });
360+
});
361+
362+
it("getLatestDeploymentStatus keeps failed:false when the latest status is queued", async () => {
363+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
364+
const url = String(input);
365+
if (url.includes("/deployments?")) return Response.json([{ id: 6 }]);
366+
return Response.json([{ state: "queued" }]);
367+
});
368+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, ref: "feature/x" })).resolves.toEqual({ url: null, failed: false });
369+
});
370+
371+
it("getLatestDeploymentStatus treats an empty statuses page as absent without a latest state", async () => {
372+
vi.stubGlobal("fetch", async (input: RequestInfo | URL) => {
373+
const url = String(input);
374+
if (url.includes("/deployments?")) return Response.json([{ id: 8 }]);
375+
return Response.json([]);
376+
});
377+
await expect(getLatestDeploymentStatus({ token: "t", repo: REPO, sha: "empty-statuses" })).resolves.toEqual({ url: null, failed: false });
378+
});
169379
});
170380

171381
describe("extractPreviewUrl", () => {

0 commit comments

Comments
 (0)