diff --git a/src/review/visual/shot.ts b/src/review/visual/shot.ts index 9a24aa248e..5123fcb1e2 100644 --- a/src/review/visual/shot.ts +++ b/src/review/visual/shot.ts @@ -198,6 +198,23 @@ const MAX_SCROLL_STEPS = 6; // Lets a scroll-linked CSS transition/JS listener finish reacting before the frame is captured — short enough // that 6 steps stays a quick "evidence" clip, long enough that a typical transition (150–300ms) has settled. const SCROLL_SETTLE_MS = 350; +const SCROLL_EVALUATE_TIMEOUT_MS = 2_000; + +async function withScrollOperationTimeout(operation: Promise, label: string): Promise { + let timeoutId: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timeoutId = setTimeout(() => reject(new Error(`scroll ${label} timed out after ${SCROLL_EVALUATE_TIMEOUT_MS}ms`)), SCROLL_EVALUATE_TIMEOUT_MS); + }); + try { + return await Promise.race([operation, timeout]); + } finally { + clearTimeout(timeoutId as ReturnType); + } +} + +async function waitForScrollSettle(): Promise { + await new Promise((resolve) => setTimeout(resolve, SCROLL_SETTLE_MS)); +} /** * Capture a short sequence of viewport-cropped frames while scrolling `url` from top to bottom, for assembly @@ -253,14 +270,20 @@ export async function captureScrollFrames(env: Env, url: string, viewport: Viewp // browser realm, not this Worker/Node one) — this project's `lib` deliberately excludes `dom` (it would // shadow the Workers-runtime `Request`/`Response` globals used everywhere else), so these two reach the // browser globals via `globalThis` instead of the bare identifiers, which don't resolve at compile time. - const scrollHeight = await page.evaluate(() => (globalThis as unknown as { document: { documentElement: { scrollHeight: number } } }).document.documentElement.scrollHeight); + const scrollHeight = await withScrollOperationTimeout( + page.evaluate(() => (globalThis as unknown as { document: { documentElement: { scrollHeight: number } } }).document.documentElement.scrollHeight), + "height", + ); const maxScroll = Math.max(0, scrollHeight - viewport.height); const stepCount = maxScroll === 0 ? 1 : MAX_SCROLL_STEPS; const frames: Uint8Array[] = []; for (let step = 0; step < stepCount; step++) { const position = stepCount === 1 ? 0 : Math.round((maxScroll * step) / (stepCount - 1)); - await page.evaluate((y) => (globalThis as unknown as { window: { scrollTo: (x: number, yPos: number) => void } }).window.scrollTo(0, y), position); - await page.evaluate((ms) => new Promise((resolve) => setTimeout(resolve, ms)), SCROLL_SETTLE_MS); + await withScrollOperationTimeout( + page.evaluate((y) => (globalThis as unknown as { window: { scrollTo: (x: number, yPos: number) => void } }).window.scrollTo(0, y), position), + "scroll", + ); + await waitForScrollSettle(); frames.push((await page.screenshot({ type: "png", fullPage: false })) as Uint8Array); } return { frames, authWalled: false }; diff --git a/test/unit/visual-shot-scroll-timeout.test.ts b/test/unit/visual-shot-scroll-timeout.test.ts new file mode 100644 index 0000000000..83687d0b10 --- /dev/null +++ b/test/unit/visual-shot-scroll-timeout.test.ts @@ -0,0 +1,71 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { captureScrollFrames } from "../../src/review/visual/shot"; + +const mocks = vi.hoisted(() => ({ + launch: vi.fn(), +})); + +vi.mock("@cloudflare/puppeteer", () => ({ + default: { launch: mocks.launch }, +})); + +function envWithBrowser(): Env { + return { BROWSER: {} } as Env; +} + +afterEach(() => { + mocks.launch.mockReset(); + vi.useRealTimers(); +}); + +describe("captureScrollFrames operation timeout", () => { + it("captures frames when page-realm scroll operations settle normally", async () => { + const close = vi.fn().mockResolvedValue(undefined); + const screenshot = new Uint8Array([1, 2, 3]); + const page = { + setRequestInterception: vi.fn().mockResolvedValue(undefined), + on: vi.fn(), + setViewport: vi.fn().mockResolvedValue(undefined), + goto: vi.fn().mockResolvedValue(undefined), + url: vi.fn().mockReturnValue("https://preview.example.com/app"), + evaluate: vi + .fn() + .mockResolvedValueOnce(700) + .mockResolvedValueOnce(undefined), + screenshot: vi.fn().mockResolvedValue(screenshot), + }; + mocks.launch.mockResolvedValue({ newPage: vi.fn().mockResolvedValue(page), close }); + + const result = await captureScrollFrames(envWithBrowser(), "https://preview.example.com/app", { width: 100, height: 900 }); + + expect(result).toEqual({ frames: [screenshot], authWalled: false }); + expect(page.evaluate).toHaveBeenCalledTimes(2); + expect(page.screenshot).toHaveBeenCalledWith({ type: "png", fullPage: false }); + expect(close).toHaveBeenCalledTimes(1); + }); + + it("REGRESSION: bounds a contributor-controlled page-realm scroll hang and closes the browser", async () => { + const close = vi.fn().mockResolvedValue(undefined); + const page = { + setRequestInterception: vi.fn().mockResolvedValue(undefined), + on: vi.fn(), + setViewport: vi.fn().mockResolvedValue(undefined), + goto: vi.fn().mockResolvedValue(undefined), + url: vi.fn().mockReturnValue("https://preview.example.com/app"), + evaluate: vi + .fn() + .mockResolvedValueOnce(1_800) + .mockReturnValueOnce(new Promise(() => undefined)), + screenshot: vi.fn(), + }; + mocks.launch.mockResolvedValue({ newPage: vi.fn().mockResolvedValue(page), close }); + + const started = Date.now(); + const result = await captureScrollFrames(envWithBrowser(), "https://preview.example.com/app", { width: 100, height: 900 }); + + expect(Date.now() - started).toBeLessThan(5_000); + expect(result).toEqual({ frames: [], authWalled: false }); + expect(page.screenshot).not.toHaveBeenCalled(); + expect(close).toHaveBeenCalledTimes(1); + }, 7_000); +});