From d4607ae63bcf7b6f0c33c2581308db70d61f1cda Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 29 Jul 2026 01:46:08 -0700 Subject: [PATCH] fix(visual): stop silently dropping mobile screenshots of long pages MAX_SCREENSHOT_HEIGHT was 10000, derived for the 1440-wide DESKTOP viewport -- MAX_SCREENSHOT_PIXELS is literally 1440 x 10000. Reusing that height bound for the 390-wide MOBILE viewport 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. The shot was discarded and the function returned null. On the ORB that was 64 mobile screenshots dropped in a single hour, weakening the visual gate on exactly the viewport most likely to show a responsive regression. The renderer was never the constraint. Verified against this deployment's own browserless (v2 / Chrome 149): a 390 x 20000 full-page capture returns 200 in ~157KB. Raise the height bound to 20000 and let MAX_SCREENSHOT_PIXELS remain the real cost ceiling, which judges a narrow-tall page by what it actually costs. Desktop is unchanged: 1440 x 10847 is 15.6M pixels and still fails the pixel cap. --- src/review/visual/shot.ts | 19 +++++++++++++++++-- test/unit/visual-shot.test.ts | 31 ++++++++++++++++++++++++++++++- 2 files changed, 47 insertions(+), 3 deletions(-) diff --git a/src/review/visual/shot.ts b/src/review/visual/shot.ts index f0a9acbe4..593d1d033 100644 --- a/src/review/visual/shot.ts +++ b/src/review/visual/shot.ts @@ -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; diff --git a/test/unit/visual-shot.test.ts b/test/unit/visual-shot.test.ts index ac3230ef2..67aafb5df 100644 --- a/test/unit/visual-shot.test.ts +++ b/test/unit/visual-shot.test.ts @@ -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(() => ({ @@ -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); + }); +});