fix(review): bound scroll GIF page operations

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<T>(operation: Promise<T>, label: string): Promise<T> {
+  let timeoutId: ReturnType<typeof setTimeout> | undefined;
+  const timeout = new Promise<never>((_, 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<typeof setTimeout>);
+  }
+}
+
+async function waitForScrollSettle(): Promise<void> {
+  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 };

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<never>(() => 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);
+});