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
19 changes: 17 additions & 2 deletions src/review/visual/shot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,23 @@ type ScreenshotPage = {
export const DESKTOP_VIEWPORT: Viewport = { width: 1440, height: 900 };
export const MOBILE_VIEWPORT: Viewport = { width: 390, height: 844 }; // iPhone-class portrait
const VIEWPORT = DESKTOP_VIEWPORT;
export const MAX_SCREENSHOT_HEIGHT = 10000;
export const MAX_SCREENSHOT_PIXELS = 14_400_000; // 1440 × 10000, matching the full-page cap.
// A sanity bound against a pathological infinite-scroll page, NOT a cost proxy -- cost is bounded by
// MAX_SCREENSHOT_PIXELS below, and separately by MAX_SCREENSHOT_BYTES.
//
// It used to be 10000, which was derived for the 1440-wide DESKTOP viewport (see the pixel cap's own
// comment: 1440 × 10000). Applied unchanged to the 390-wide MOBILE viewport it rejected captures costing a
// third as much: an ordinary long docs page renders ~10850px tall at 390 wide, which is 4.2M pixels against
// a 14.4M budget -- comfortably affordable, silently dropped. On the ORB that was 64 mobile screenshots
// discarded in a single hour, weakening the visual gate precisely on the viewport most likely to reveal a
// responsive regression.
//
// 20000 is empirically verified against this deployment's own renderer (browserless v2 / Chrome 149):
// a 390 × 20000 full-page capture returns 200 in ~157KB. The renderer was never the binding constraint.
// Desktop is unaffected -- 1440 × 20000 is 28.8M pixels and still fails the pixel cap.
export const MAX_SCREENSHOT_HEIGHT = 20000;
// The real cost ceiling: width × height, so a narrow-tall page is judged by what it actually costs to
// render rather than by height alone.
export const MAX_SCREENSHOT_PIXELS = 14_400_000; // 1440 × 10000 — one full desktop-width page.
export const MAX_SCREENSHOT_BYTES = 5 * 1024 * 1024;
const SCREENSHOT_TIMEOUT_MS = 10000;
const SCREENSHOT_HEIGHT_PROBE_TIMEOUT_MS = 2_000;
Expand Down
31 changes: 30 additions & 1 deletion test/unit/visual-shot.test.ts
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
import { beforeEach, describe, expect, it, vi } from "vitest";
import { captureInteractionFrames, captureScrollFrames, captureShot, handleShot } from "../../src/review/visual/shot";
import { captureInteractionFrames, captureScrollFrames, captureShot, handleShot, MAX_SCREENSHOT_HEIGHT, MAX_SCREENSHOT_PIXELS, MOBILE_VIEWPORT, DESKTOP_VIEWPORT } from "../../src/review/visual/shot";
import { counterValue, resetMetrics } from "../../src/selfhost/metrics";

const mocks = vi.hoisted(() => ({
Expand Down Expand Up @@ -1113,3 +1113,32 @@ describe("visual capture result metric (#9487)", () => {
});

});

describe("a narrow-tall mobile page is judged by cost, not height alone", () => {
// Observed on the ORB: 64 mobile captures discarded in one hour, every one at width 390. An ordinary long
// docs page is ~10850px tall at that width -- 4.2M pixels against a 14.4M budget. The old 10000 height cap
// was derived for the 1440-wide DESKTOP viewport (MAX_SCREENSHOT_PIXELS is literally 1440 × 10000), so
// reusing it for mobile rejected captures costing a third as much, weakening the visual gate on exactly
// the viewport most likely to reveal a responsive regression.
it("REGRESSION: the real observed mobile page (390 × 10847) is now within both caps", () => {
const width = MOBILE_VIEWPORT.width;
const height = 10847;
expect(width).toBe(390);
expect(height).toBeLessThanOrEqual(MAX_SCREENSHOT_HEIGHT);
expect(width * height).toBeLessThanOrEqual(MAX_SCREENSHOT_PIXELS);
});

it("INVARIANT: desktop is unaffected — the pixel cap still rejects a wide page of the same height", () => {
// The cost ceiling must not have been widened by raising the height bound.
const area = DESKTOP_VIEWPORT.width * 10847;
expect(area).toBeGreaterThan(MAX_SCREENSHOT_PIXELS);
});

it("INVARIANT: the height cap is a sanity bound the renderer can actually satisfy", () => {
// 20000 is verified against this deployment's renderer (browserless v2 / Chrome 149): a 390 × 20000
// full-page capture returns 200 in ~157KB. If this is ever raised past what the renderer can produce,
// captures fail at request time instead of being cheaply rejected here.
expect(MAX_SCREENSHOT_HEIGHT).toBe(20000);
expect(MOBILE_VIEWPORT.width * MAX_SCREENSHOT_HEIGHT).toBeLessThanOrEqual(MAX_SCREENSHOT_PIXELS);
});
});