diff --git a/.gittensory.yml.example b/.gittensory.yml.example index ea54610708..db79f856f2 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -831,6 +831,12 @@ settings: # themes: # - light # - dark +# # Also capture a short scroll-through GIF per route (desktop only) — evidence for scroll-linked behavior +# # (parallax, reveal-on-scroll, a sticky header) that a static screenshot can't show (#3612). Rendered as +# # a separate "Scroll preview" section alongside the static before/after table, never replacing it. +# # SELF-HOST ONLY (no effect on the hosted service) and the heaviest capture mode here — up to 6 extra +# # renders per side, ~4s wall-clock per side measured in practice. Bool. Default: false (no scroll capture). +# gif: false # # Maintainer overrides for the public review-panel CONTENT (not what gittensory measures). The # # Gittensor attribution + register link is always appended to the footer regardless; maintainer text # # failing the public-safe filter is dropped, never published. diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index e287bab71b..a24cdd2048 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -844,6 +844,12 @@ settings: # themes: # - light # - dark +# # Also capture a short scroll-through GIF per route (desktop only) — evidence for scroll-linked behavior +# # (parallax, reveal-on-scroll, a sticky header) that a static screenshot can't show (#3612). Rendered as +# # a separate "Scroll preview" section alongside the static before/after table, never replacing it. +# # SELF-HOST ONLY (no effect on the hosted service) and the heaviest capture mode here — up to 6 extra +# # renders per side, ~4s wall-clock per side measured in practice. Bool. Default: false (no scroll capture). +# gif: false # # Maintainer overrides for the public review-panel CONTENT (not what gittensory measures). The # # Gittensor attribution + register link is always appended to the footer regardless; maintainer text # # failing the public-safe filter is dropped, never published. diff --git a/package-lock.json b/package-lock.json index a6cccb20d2..6534d3f0c5 100644 --- a/package-lock.json +++ b/package-lock.json @@ -41,6 +41,7 @@ "@types/pngjs": "^6.0.5", "@vitest/coverage-v8": "^4.1.9", "drizzle-kit": "^0.31.10", + "gifenc": "^1.0.3", "git-cliff": "^2.13.1", "github-actionlint": "^1.7.12", "node-addon-api": "^8.8.0", @@ -59,6 +60,7 @@ }, "apps/gittensory-ui": { "name": "@jsonbored/gittensory-ui", + "version": "0.0.0", "dependencies": { "@hookform/resolvers": "^5.4.0", "@radix-ui/react-accordion": "^1.2.14", @@ -9890,6 +9892,13 @@ "node": ">= 14" } }, + "node_modules/gifenc": { + "version": "1.0.3", + "resolved": "https://registry.npmjs.org/gifenc/-/gifenc-1.0.3.tgz", + "integrity": "sha512-xdr6AdrfGBcfzncONUOlXMBuc5wJDtOueE3c5rdG0oNgtINLD+f2iFZltrBRZYzACRbKr+mSVU/x98zv2u3jmw==", + "dev": true, + "license": "MIT" + }, "node_modules/git-cliff": { "version": "2.13.1", "resolved": "https://registry.npmjs.org/git-cliff/-/git-cliff-2.13.1.tgz", diff --git a/package.json b/package.json index a68ad41a95..4008e85f49 100644 --- a/package.json +++ b/package.json @@ -109,6 +109,7 @@ "@types/pngjs": "^6.0.5", "@vitest/coverage-v8": "^4.1.9", "drizzle-kit": "^0.31.10", + "gifenc": "^1.0.3", "git-cliff": "^2.13.1", "github-actionlint": "^1.7.12", "node-addon-api": "^8.8.0", diff --git a/scripts/build-selfhost.mjs b/scripts/build-selfhost.mjs index 5fa03e44a4..f056faa418 100644 --- a/scripts/build-selfhost.mjs +++ b/scripts/build-selfhost.mjs @@ -52,6 +52,8 @@ await esbuild.build({ // importer, always as "./pixel-diff" (same-directory sibling) — the stub's own `import type` back to // the original is erased before bundling and never reaches this resolver. build.onResolve({ filter: /^\.\/pixel-diff$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/pixel-diff.ts") })); + // Same pattern for scroll-through GIF assembly (#3612) — pngjs decode + gifenc encode need Node Buffer. + build.onResolve({ filter: /^\.\/scroll-gif$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/scroll-gif.ts") })); }, }, ], diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index b91d8f5e5f..5b7faf1f48 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -390,6 +390,45 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl return { title: "Visual preview", body, rawHtml: true }; } +/** + * Build the "Scroll preview" collapsible from the same before/after capture routes (#3612) — rendered + * ALONGSIDE "Visual preview", never replacing it, since a scroll-through GIF is evidence for scroll-linked + * behavior (parallax, reveal-on-scroll, a sticky header) that a single static screenshot can't show, not a + * substitute for the static before/after comparison. Self-host only (`review.visual.gif`, off by default — + * see capture.ts's `gifWanted`) and desktop-viewport only in this first cut, so there is no Viewport column + * here (unlike "Visual preview"'s desktop/mobile rows). Same clickable-thumbnail markup and public-safety + * argument as `buildBeforeAfterCollapsible`. Returns null when no route has a GIF, so the section is omitted + * entirely for every repo that hasn't opted in — byte-identical to pre-#3612 for everyone else. + */ +export function buildScrollPreviewCollapsible(routes: CaptureRoute[]): UnifiedCollapsible | null { + const attr = (value: string): string => + value.replace(/[&"<>]/g, (char) => ({ "&": "&", '"': """, "<": "<", ">": ">" })[char] as string); + const markdownCode = (value: string): string => + `\`${value + .replace(/\\/g, "\\\\") + .replace(/`/g, "\\`") + .replace(/\|/g, "\\|") + .replace(/[<>]/g, (char) => (char === "<" ? "<" : ">"))}\``; + const cell = (url: string | undefined, label: string): string => + url ? `${attr(label)}` : "—"; + const rows: string[] = []; + for (const route of routes) { + if (!route.beforeGifUrl && !route.afterGifUrl) continue; + const path = markdownCode(route.path); + const themeSuffix = route.theme ? ` (${route.theme})` : ""; + rows.push(`| ${path}${themeSuffix} | ${cell(route.beforeGifUrl, `before ${route.path}${themeSuffix} (scroll)`)} | ${cell(route.afterGifUrl, `after ${route.path}${themeSuffix} (scroll)`)} |`); + } + if (rows.length === 0) return null; + const body = [ + "| Route | Before (production) | After (this PR's preview) |", + "| --- | --- | --- |", + ...rows, + "", + "_A short scroll-through clip (desktop) — click either thumbnail to open the full animation. Evidence for scroll-linked behavior a single screenshot can't show._", + ].join("\n"); + return { title: "Scroll preview", body, rawHtml: true }; +} + /** A changed file's path + line deltas — everything `buildChangedFilesSummaryCollapsible` needs to group and * total. Deliberately narrower than `PullRequestFileRecord` (path/additions/deletions only) so the bridge * doesn't drag GitHub's full file-record shape into its pure-rendering surface. */ @@ -552,8 +591,11 @@ export function buildUnifiedCommentBody(args: UnifiedCommentBridgeArgs): string // Visual-capture port: when before/after routes are present, append a "Visual preview" collapsible to the // extra sections. Flag-OFF (the processor passes no beforeAfter) ⇒ extraCollapsibles is unchanged. const visualCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildBeforeAfterCollapsible(args.beforeAfter) : null; - const extraCollapsibles = - visualCollapsible !== null ? [...(withFindingCategories ?? []), visualCollapsible] : withFindingCategories; + const withVisual = visualCollapsible !== null ? [...(withFindingCategories ?? []), visualCollapsible] : withFindingCategories; + // #3612: "Scroll preview" renders ALONGSIDE "Visual preview" (never replacing it) — self-host + gif:true + // only, so this is null (no section, no behavior change) for every repo that hasn't opted in. + const scrollCollapsible = args.beforeAfter && args.beforeAfter.length > 0 ? buildScrollPreviewCollapsible(args.beforeAfter) : null; + const extraCollapsibles = scrollCollapsible !== null ? [...(withVisual ?? []), scrollCollapsible] : withVisual; const body = renderUnifiedReviewComment(input, { brand: args.brand ?? "Gittensory review", diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 4238b43b35..5a330bace8 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -19,8 +19,9 @@ import { getPreviewBuildState, parseRepo, } from "./preview-url"; -import { captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type ShotTheme, type Viewport } from "./shot"; +import { captureScrollFrames, captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type ShotTheme, type Viewport } from "./shot"; import { compareCapturedScreenshots, isVisualDiffAvailable, type VisualDiffOutcome } from "./pixel-diff"; +import { encodeScrollGif, isScrollGifAvailable } from "./scroll-gif"; const NAMESPACE = "gittensory"; const DEFAULT_ROUTES = ["/"]; @@ -34,7 +35,9 @@ const MAX_CONFIGURED_ROUTES = 5; * per viewport (#3674) — self-host only (isVisualDiffAvailable), and only when the diff clears the visual- * diff module's own noise threshold; undefined slot ⇒ a dash cell either way. `theme` is set only when * `review.visual.themes` (#3678) configured more than the implicit single default capture — undefined means - * "the one, un-emulated default render", exactly like today. */ + * "the one, un-emulated default render", exactly like today. `beforeGifUrl`/`afterGifUrl` (#3612) are a + * short scroll-through animation — self-host only (isScrollGifAvailable) and only when `review.visual.gif` + * opts in; desktop viewport only in this first cut (see buildCapture's scoping note). */ export interface CaptureRoute { path: string; theme?: ShotTheme | undefined; @@ -44,6 +47,8 @@ export interface CaptureRoute { afterUrlMobile?: string | undefined; diffUrl?: string | undefined; diffUrlMobile?: string | undefined; + beforeGifUrl?: string | undefined; + afterGifUrl?: string | undefined; } /** The capture pipeline's result: the rendered routes, plus whether a preview build is still pending. */ @@ -214,10 +219,54 @@ async function uploadDiffImage( return `${shotBase}/${NAMESPACE}/shot?key=${encodeURIComponent(key)}`; } -/** Per-repo `review.visual` config, as resolved by the caller from the manifest (#3609 / #3610 / #3678). - * Absent ⇒ byte-identical to today (GitHub-native discovery, automatic route inference, single default- - * theme capture, built-in route cap). */ -export type VisualCaptureConfig = { preview?: VisualPreviewInput | null | undefined; routes?: VisualRoutesInput | null | undefined; themes?: readonly ShotTheme[] | null | undefined }; +// How long each frame shows when the assembled GIF plays back (#3612) — a quick "evidence clip" pace: the +// full MAX_SCROLL_STEPS (6, see shot.ts) loop takes ~3s, long enough to read, short enough to stay a glance. +const GIF_FRAME_DELAY_MS = 500; + +/** + * Capture a scroll-through sequence for `page` and assemble it into a GIF (#3612), or undefined when there's + * no page, the render fails/auth-walls, storage is unavailable, or this build can't assemble GIFs at all + * (isScrollGifAvailable — hosted mode; see scroll-gif.ts). Caches on the same fingerprint scheme as + * `capturePage`/`uploadDiffImage` — a scroll capture is the most expensive thing this pipeline does (up to 6 + * extra renders plus a full encode), so a re-review of the same head must never redo it. + */ +async function captureScrollGif( + env: Env, + target: CaptureTarget, + page: string, + slot: "before" | "after", + viewportName: "desktop" | "mobile", + viewport: Viewport, + theme?: ShotTheme | undefined, +): Promise { + if (!page) return undefined; + const shotBase = env.PUBLIC_API_ORIGIN; + if (!env.REVIEW_AUDIT || !shotBase) return undefined; + const fingerprint = await sha256Hex(`${target.headSha ?? target.prNumber}:scrollgif:${slot}:${viewportName}:${page}${theme ? `:${theme}` : ""}`); + const key = `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}.gif`; + const url = `${shotBase}/${NAMESPACE}/shot?key=${encodeURIComponent(key)}`; + const cached = await env.REVIEW_AUDIT.get(key).catch(() => null); + if (cached) return url; + const { frames, authWalled } = await captureScrollFrames(env, page, viewport, theme ? { theme } : {}).catch(() => ({ frames: [] as Uint8Array[], authWalled: false })); + if (authWalled || frames.length === 0) return undefined; + const gifBytes = await encodeScrollGif( + frames.map((png) => ({ png })), + GIF_FRAME_DELAY_MS, + ); + if (!gifBytes) return undefined; + await env.REVIEW_AUDIT.put(key, gifBytes, { httpMetadata: { contentType: "image/gif" } }).catch(() => undefined); + return url; +} + +/** Per-repo `review.visual` config, as resolved by the caller from the manifest (#3609 / #3610 / #3678 / + * #3612). Absent ⇒ byte-identical to today (GitHub-native discovery, automatic route inference, single + * default-theme capture, built-in route cap, no scroll-GIF). */ +export type VisualCaptureConfig = { + preview?: VisualPreviewInput | null | undefined; + routes?: VisualRoutesInput | null | undefined; + themes?: readonly ShotTheme[] | null | undefined; + gif?: boolean | null | undefined; +}; /** * Build the before/after capture for a PR: resolve the preview URL, derive routes from the changed UI files, @@ -278,6 +327,12 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge // #3674: resolved ONCE per call, not per route/viewport — false in every hosted build (see pixel-diff.ts), // so capturePage never pays the extra cached-bytes-read cost unless self-host's real diff module is active. const diffAvailable = isVisualDiffAvailable(); + // #3612: gated on BOTH the opt-in config AND isScrollGifAvailable — hosted mode can never assemble a GIF + // (see scroll-gif.ts), so this must short-circuit before capturing a single scroll frame there, not just + // before encoding one. Desktop-viewport only in this first cut: a scroll-through GIF is already the + // heaviest capture mode (up to 6 extra renders per side), and doubling it for mobile is a narrower-scope + // call deferred to a follow-up rather than shipped speculatively (matches #3674's hosted-diff deferral). + const gifWanted = visualConfig?.gif === true && isScrollGifAvailable(); const routes = resolveVisualRoutes(visualFiles, visualConfig?.routes); // #3678: an explicit, non-empty theme list captures the SAME routes once per theme, each tagged on its // CaptureRoute entry. [undefined] (the default, absent config) renders the single un-emulated default — @@ -308,6 +363,12 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge uploadDiffImage(env, target, path, "desktop", desktopDiff, theme), uploadDiffImage(env, target, path, "mobile", mobileDiff, theme), ]); + const [beforeGifUrl, afterGifUrl] = gifWanted + ? await Promise.all([ + captureScrollGif(env, target, beforePage, "before", "desktop", DESKTOP_VIEWPORT, theme), + afterPage ? captureScrollGif(env, target, afterPage, "after", "desktop", DESKTOP_VIEWPORT, theme) : Promise.resolve(undefined), + ]) + : [undefined, undefined]; captureRoutes.push({ path, ...(theme ? { theme } : {}), @@ -317,6 +378,8 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge afterUrlMobile: afterMobileShot.url, ...(diffUrl ? { diffUrl } : {}), ...(diffUrlMobile ? { diffUrlMobile } : {}), + ...(beforeGifUrl ? { beforeGifUrl } : {}), + ...(afterGifUrl ? { afterGifUrl } : {}), }); } } diff --git a/src/review/visual/scroll-gif.ts b/src/review/visual/scroll-gif.ts new file mode 100644 index 0000000000..5f21de6610 --- /dev/null +++ b/src/review/visual/scroll-gif.ts @@ -0,0 +1,29 @@ +// Scroll-through GIF assembly seam for the before/after capture pipeline (#3612). WORKER-SAFE DEFAULT: a no-op. +// +// A scroll-linked interaction (parallax, reveal-on-scroll, a sticky header) isn't visible in a single static +// screenshot — this assembles a short sequence of viewport-cropped frames (captured while scrolling down the +// page, see `captureScrollFrames` in ./shot) into one animated image. Turning those frames into a real +// animated image needs decoding each captured frame back to raw pixels, which — like the pixel-diff provider +// in ./pixel-diff — depends on Node's `Buffer` and a native-leaning image-decode step the Cloudflare Workers +// runtime doesn't guarantee. `test/unit/worker-entry-boundary.test.ts` enforces the same boundary here as it +// does for the pixel-diff module. `capture.ts` (Worker-reachable) imports ONLY this file; never the self-host +// module directly. `scripts/build-selfhost.mjs`'s esbuild plugin swaps this exact specifier for a real +// implementation when bundling the self-host entry (`src/server.ts`) — the same module-substitution pattern +// already used for pixel-diff and `@cloudflare/puppeteer` in that same build. The Worker's own (wrangler) +// bundle never applies that swap, so hosted mode always uses this no-op — zero behavior change, zero added +// capture cost, until a Workers-compatible image-decode path exists. +export type ScrollGifFrame = { png: Uint8Array }; + +/** True when this build can actually assemble a scroll-through GIF (self-host only, see module header). + * Callers use this to decide whether it's worth paying the extra cost of capturing stepped scroll frames at + * all — always false here, so nothing about the existing capture path changes in hosted mode. */ +export function isScrollGifAvailable(): boolean { + return false; +} + +/** Assemble captured frames into an animated image. Always null in the Worker-safe default — self-host's + * swapped-in implementation does the real encode. Callers must treat null as "no GIF available", never a + * failure — a missing GIF degrades the collapsible section to omitting that route, not an error. */ +export async function encodeScrollGif(_frames: readonly ScrollGifFrame[], _frameDelayMs: number): Promise { + return null; +} diff --git a/src/review/visual/shot.ts b/src/review/visual/shot.ts index e6f9065a6e..9a24aa248e 100644 --- a/src/review/visual/shot.ts +++ b/src/review/visual/shot.ts @@ -190,6 +190,88 @@ export async function renderScreenshot(env: Env, url: string, viewport: Viewport return (await captureShot(env, url, viewport, opts)).png; } +// A scroll-through capture is deliberately narrow (#3612): a fixed number of viewport-cropped frames taken +// while scrolling straight down the page, not a general "record any interaction" system. This is sufficient +// evidence for scroll-linked behavior (parallax, reveal-on-scroll, a sticky header) without the much harder, +// speculative problem of inferring WHICH interaction a change actually affects. +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; + +/** + * Capture a short sequence of viewport-cropped frames while scrolling `url` from top to bottom, for assembly + * into a scroll-through GIF (#3612) — evidence for scroll-linked behavior that a single static screenshot + * can't show. Mirrors `captureShot`'s SSRF guard, sub-request interception, and auth-wall detection exactly + * (duplicated rather than shared: this is security-sensitive code, and the two functions diverge only in + * what they do with the page once navigation succeeds). A page shorter than one viewport yields a single + * frame — nothing to scroll through, so no point animating a static page. `frames` is empty on any failure + * (callers degrade gracefully, same contract as `captureShot` returning a null `png`). + */ +export async function captureScrollFrames(env: Env, url: string, viewport: Viewport = VIEWPORT, opts: CaptureShotOptions = {}): Promise<{ frames: Uint8Array[]; authWalled: boolean }> { + if (!url || !isSafeHttpUrl(url) || (opts.isAllowedUrl && !opts.isAllowedUrl(url))) { + console.log(JSON.stringify({ ev: "render_scroll_frames_blocked", url: String(url).slice(0, 120) })); + return { frames: [], authWalled: false }; + } + if (!env.BROWSER) return { frames: [], authWalled: false }; + let browser: Awaited> | null = null; + try { + browser = await puppeteer.launch(env.BROWSER as unknown as Parameters[0]); + const page = await browser.newPage(); + await page.setRequestInterception(true); + page.on("request", (request: ScreenshotRequest) => { + const requestUrl = request.url(); + let protocol = ""; + try { + protocol = new URL(requestUrl).protocol; + } catch { + request.abort().catch(() => undefined); + return; + } + if (protocol === "http:" || protocol === "https:") { + const isAllowedNavigation = !request.isNavigationRequest() || !opts.isAllowedUrl || opts.isAllowedUrl(requestUrl); + if (!isSafeHttpUrl(requestUrl) || !isAllowedNavigation) { + console.log(JSON.stringify({ ev: "render_scroll_frames_request_blocked", url: requestUrl.slice(0, 120) })); + request.abort().catch(() => undefined); + return; + } + } + request.continue().catch(() => undefined); + }); + await page.setViewport(viewport); + if (opts.theme) await page.emulateMediaFeatures([{ name: "prefers-color-scheme", value: opts.theme }]); + await page.goto(url, { waitUntil: "networkidle0", timeout: 20000 }); + if (!isSafeHttpUrl(page.url()) || (opts.isAllowedUrl && !opts.isAllowedUrl(page.url()))) { + console.log(JSON.stringify({ ev: "render_scroll_frames_redirect_blocked", url, final: page.url().slice(0, 200) })); + return { frames: [], authWalled: false }; + } + if (isAuthWallUrl(page.url()) && !isAuthWallUrl(url)) { + console.log(JSON.stringify({ ev: "render_scroll_frames_auth_walled", url, final: page.url().slice(0, 200) })); + return { frames: [], authWalled: true }; + } + // `document`/`window` below run inside the real page (the callback is serialized and executed in the + // 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 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); + frames.push((await page.screenshot({ type: "png", fullPage: false })) as Uint8Array); + } + return { frames, authWalled: false }; + } catch (error) { + console.log(JSON.stringify({ ev: "render_scroll_frames_error", mode: "binding", url, message: String(error).slice(0, 200) })); + return { frames: [], authWalled: false }; + } finally { + if (browser) await browser.close().catch(() => undefined); + } +} + export async function handleShot(request: Request, env: Env, opts: ShotOptions = {}): Promise { const params = new URL(request.url).searchParams; const r2Prefix = `${opts.namespace ?? "gittensory"}/shots/`; @@ -213,8 +295,12 @@ export async function handleShot(request: Request, env: Env, opts: ShotOptions = } const object = await env.REVIEW_AUDIT?.get(key); if (!object) return new Response("not found", { status: 404 }); + // By extension, not stored httpMetadata: the self-host filesystem blob store never round-trips it (see + // src/selfhost/blob-store.ts), so a GIF (#3612) served with a hardcoded image/png content-type would + // fail to animate in most viewers even though the bytes themselves are a perfectly valid GIF. + const contentType = key.endsWith(".gif") ? "image/gif" : "image/png"; return new Response(object.body, { - headers: { "content-type": "image/png", "cache-control": "public, max-age=86400, immutable" }, + headers: { "content-type": contentType, "cache-control": "public, max-age=86400, immutable" }, }); } diff --git a/src/selfhost/stubs/gifenc.d.ts b/src/selfhost/stubs/gifenc.d.ts new file mode 100644 index 0000000000..2da3de583b --- /dev/null +++ b/src/selfhost/stubs/gifenc.d.ts @@ -0,0 +1,28 @@ +// Minimal ambient types for the `gifenc` package (#3612), which ships no TypeScript declarations of its own +// and has no @types package. Scoped to exactly the surface `./scroll-gif.ts` uses. +declare module "gifenc" { + export type GifPalette = number[][]; + + export interface GifEncoderFrameOptions { + transparent?: boolean; + transparentIndex?: number; + delay?: number; + palette?: GifPalette | null; + repeat?: number; + colorDepth?: number; + dispose?: number; + first?: boolean; + } + + export interface GifEncoderInstance { + writeFrame(index: Uint8Array, width: number, height: number, opts?: GifEncoderFrameOptions): void; + finish(): void; + bytes(): Uint8Array; + bytesView(): Uint8Array; + reset(): void; + } + + export function GIFEncoder(opts?: { initialCapacity?: number; auto?: boolean }): GifEncoderInstance; + export function quantize(rgba: Uint8Array | Uint8ClampedArray, maxColors: number, opts?: Record): GifPalette; + export function applyPalette(rgba: Uint8Array | Uint8ClampedArray, palette: GifPalette, format?: string): Uint8Array; +} diff --git a/src/selfhost/stubs/scroll-gif.ts b/src/selfhost/stubs/scroll-gif.ts new file mode 100644 index 0000000000..926b80abbf --- /dev/null +++ b/src/selfhost/stubs/scroll-gif.ts @@ -0,0 +1,38 @@ +// Self-host replacement for src/review/visual/scroll-gif.ts (#3612). Swapped in by +// scripts/build-selfhost.mjs's esbuild plugin, the same mechanism used for pixel-diff and +// @cloudflare/puppeteer — this file is only ever bundled into dist/server.mjs, never the Worker entry, so +// it's safe to depend on pngjs (Node `Buffer` + PNG decode) and gifenc (pure-JS GIF encode, no ffmpeg/native +// dependency — Workers-safe by itself, but useless here without the PNG-decode step next to it) here. +import { PNG } from "pngjs"; +import { GIFEncoder, quantize, applyPalette } from "gifenc"; +import type { ScrollGifFrame } from "../../review/visual/scroll-gif"; + +export function isScrollGifAvailable(): boolean { + return true; +} + +export async function encodeScrollGif(frames: readonly ScrollGifFrame[], frameDelayMs: number): Promise { + if (frames.length === 0) return null; + try { + const decoded = frames.map((frame) => PNG.sync.read(Buffer.from(frame.png))); + const { width, height } = decoded[0]!; + // Every frame comes from the same viewport-cropped capture loop, so dimensions should always match — + // treat a mismatch as a decode/capture inconsistency and degrade to null rather than emit a corrupt GIF. + if (decoded.some((image) => image.width !== width || image.height !== height)) return null; + + // One shared palette across every frame (quantized over ALL frames' pixels, not just the first) avoids a + // per-frame palette switch flickering colors that are consistent across the real page. + const allPixels = Buffer.concat(decoded.map((image) => image.data)); + const palette = quantize(allPixels, 256); + + const gif = GIFEncoder(); + decoded.forEach((image, index) => { + const indexed = applyPalette(image.data, palette); + gif.writeFrame(indexed, width, height, { palette, delay: frameDelayMs, first: index === 0, repeat: 0 }); + }); + gif.finish(); + return gif.bytes(); + } catch { + return null; + } +} diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 0d86d7a219..3c54768edf 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -472,6 +472,12 @@ export type VisualConfig = { preview: VisualPreviewConfig; routes: VisualRoutesConfig; themes: VisualTheme[]; + /** `review.visual.gif`: capture a short scroll-through GIF (#3612) alongside the static before/after + * screenshots — evidence for scroll-linked behavior (parallax, reveal-on-scroll, a sticky header) that a + * single static shot can't show. Self-host only (see src/review/visual/scroll-gif.ts) and the heaviest + * capture mode this pipeline has (up to 6 extra renders per side) — false (default, every existing + * manifest) ⇒ byte-identical to today, no scroll frames captured at all. */ + gif: boolean; }; /** A `prefers-color-scheme` value the capture pipeline can emulate before rendering (#3678). */ @@ -508,6 +514,7 @@ export const EMPTY_VISUAL_CONFIG: VisualConfig = { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], + gif: false, }; /** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a @@ -1866,7 +1873,7 @@ function parseSelfHostAiModelConfig(value: JsonValue | undefined, warnings: stri } function visualConfigPresent(config: VisualConfig): boolean { - return config.preview.urlTemplate !== null || config.routes.paths.length > 0 || config.routes.maxRoutes !== null || config.themes.length > 0; + return config.preview.urlTemplate !== null || config.routes.paths.length > 0 || config.routes.maxRoutes !== null || config.themes.length > 0 || config.gif; } const VISUAL_THEME_VALUES: readonly VisualTheme[] = ["light", "dark"]; @@ -1946,8 +1953,9 @@ function parseVisualConfig(value: JsonValue | undefined, warnings: string[]): Vi const maxRoutes = routesRecord ? normalizeOptionalVisualMaxRoutes(routesRecord.max_routes, warnings) : null; const themes = parseVisualThemes(record.themes, warnings); + const gif = normalizeOptionalBoolean(record.gif, "review.visual.gif", warnings) === true; - return { preview: { urlTemplate }, routes: { paths, maxRoutes }, themes }; + return { preview: { urlTemplate }, routes: { paths, maxRoutes }, themes, gif }; } function parseAutoReviewTitleKeywords(value: JsonValue | undefined, warnings: string[]): string[] { @@ -2242,6 +2250,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue visual.routes = routes; } if (review.visual.themes.length > 0) visual.themes = [...review.visual.themes]; + if (review.visual.gif) visual.gif = true; out.visual = visual; } if (review.linkedIssueSatisfaction !== null) out.linkedIssueSatisfaction = review.linkedIssueSatisfaction; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 8cd9cd8dbc..9833eb065c 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3351,6 +3351,7 @@ describe("review.visual (#3609 preview.url_template / #3610 routes)", () => { preview: { urlTemplate: "https://pr-{number}.preview.example.com" }, routes: { paths: ["/pricing", "/docs"], maxRoutes: 3 }, themes: [], + gif: false, }); expect(m.review.present).toBe(true); expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.visual).toEqual(m.review.visual); @@ -3446,7 +3447,7 @@ describe("review.visual (#3609 preview.url_template / #3610 routes)", () => { it("resolveReviewVisualConfig: null manifest yields empty defaults; a set manifest passes through", () => { expect(resolveReviewVisualConfig(null)).toEqual({ ...EMPTY_VISUAL_CONFIG }); const manifest = parseFocusManifest({ review: { visual: { routes: { paths: ["/app"] } } } }); - expect(resolveReviewVisualConfig(manifest)).toEqual({ preview: { urlTemplate: null }, routes: { paths: ["/app"], maxRoutes: null }, themes: [] }); + expect(resolveReviewVisualConfig(manifest)).toEqual({ preview: { urlTemplate: null }, routes: { paths: ["/app"], maxRoutes: null }, themes: [], gif: false }); }); }); @@ -3499,6 +3500,48 @@ describe("review.visual.themes (#3678 dark-mode capture)", () => { }); }); +describe("review.visual.gif (#3612 scroll-through GIF capture)", () => { + it("parses gif: true, marks present, and round-trips", () => { + const m = parseFocusManifest({ review: { visual: { gif: true } } }); + expect(m.review.visual.gif).toBe(true); + expect(m.review.present).toBe(true); + expect(reviewConfigToJson(m.review)).toEqual({ visual: { gif: true } }); + }); + + it("absent gif defaults to false and does not mark review present on its own", () => { + expect(parseFocusManifest({}).review.visual.gif).toBe(false); + expect(parseFocusManifest({ review: { visual: {} } }).review.present).toBe(false); + }); + + it("gif: false does not mark review present, so the whole review block round-trips to null", () => { + const m = parseFocusManifest({ review: { visual: { gif: false } } }); + expect(m.review.visual.gif).toBe(false); + expect(reviewConfigToJson(m.review)).toBeNull(); + }); + + it("warns and defaults to false when gif is not a boolean", () => { + const bad = parseFocusManifest({ review: { visual: { gif: "yes" } } }); + expect(bad.review.visual.gif).toBe(false); + expect(bad.warnings.some((w) => /review\.visual\.gif.*must be a boolean/.test(w))).toBe(true); + }); + + it("marks present via gif alone (preview + routes + themes all empty)", () => { + const m = parseFocusManifest({ review: { visual: { gif: true } } }); + expect(m.review.present).toBe(true); + }); + + it("composes with themes — both configured independently and both round-trip", () => { + const m = parseFocusManifest({ review: { visual: { gif: true, themes: ["dark"] } } }); + expect(m.review.visual).toEqual({ preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: ["dark"], gif: true }); + expect(reviewConfigToJson(m.review)).toEqual({ visual: { themes: ["dark"], gif: true } }); + }); + + it("resolveReviewVisualConfig passes a configured gif: true through", () => { + const manifest = parseFocusManifest({ review: { visual: { gif: true } } }); + expect(resolveReviewVisualConfig(manifest).gif).toBe(true); + }); +}); + describe("review.pre_merge_checks (#review-pre-merge-checks)", () => { it("parses checks (name + assertions + when_paths + enforce), marks present, and round-trips", () => { const m = parseFocusManifest({ diff --git a/test/unit/scroll-gif.test.ts b/test/unit/scroll-gif.test.ts new file mode 100644 index 0000000000..b5477b8ad5 --- /dev/null +++ b/test/unit/scroll-gif.test.ts @@ -0,0 +1,14 @@ +import { describe, expect, it } from "vitest"; +import { encodeScrollGif, isScrollGifAvailable } from "../../src/review/visual/scroll-gif"; + +describe("scroll-gif Worker-safe default (#3612)", () => { + it("reports scroll-GIF assembly as unavailable", () => { + expect(isScrollGifAvailable()).toBe(false); + }); + + it("always resolves to null regardless of input, since the real implementation is self-host only", async () => { + const frames = [{ png: new Uint8Array([1, 2, 3]) }, { png: new Uint8Array([4, 5, 6]) }]; + await expect(encodeScrollGif(frames, 500)).resolves.toBeNull(); + await expect(encodeScrollGif([], 500)).resolves.toBeNull(); + }); +}); diff --git a/test/unit/selfhost-scroll-gif-stub.test.ts b/test/unit/selfhost-scroll-gif-stub.test.ts new file mode 100644 index 0000000000..819e30d85e --- /dev/null +++ b/test/unit/selfhost-scroll-gif-stub.test.ts @@ -0,0 +1,62 @@ +// Tests for the self-host scroll-through-GIF stub (#3612). This module is never bundled into the Worker +// entry (scripts/build-selfhost.mjs swaps it in only when building src/server.ts — see +// test/unit/worker-entry-boundary.test.ts for the enforced side of that), so it's safe to depend on real +// PNG fixtures / Buffer here, mirroring test/unit/selfhost-pixel-diff-stub.test.ts's own fixture style. +import { PNG } from "pngjs"; +import { describe, expect, it } from "vitest"; +import { encodeScrollGif, isScrollGifAvailable } from "../../src/selfhost/stubs/scroll-gif"; + +function createSolidPng(width: number, height: number, rgba: [number, number, number, number]): Uint8Array { + const png = new PNG({ width, height }); + for (let y = 0; y < height; y += 1) { + for (let x = 0; x < width; x += 1) { + const idx = (width * y + x) << 2; + png.data[idx] = rgba[0]; + png.data[idx + 1] = rgba[1]; + png.data[idx + 2] = rgba[2]; + png.data[idx + 3] = rgba[3]; + } + } + return new Uint8Array(PNG.sync.write(png)); +} + +function gifHeaderOf(bytes: Uint8Array): string { + return String.fromCharCode(...bytes.slice(0, 6)); +} + +describe("selfhost scroll-gif stub (#3612)", () => { + it("reports scroll-GIF assembly as available", () => { + expect(isScrollGifAvailable()).toBe(true); + }); + + it("returns null for an empty frame list", async () => { + await expect(encodeScrollGif([], 300)).resolves.toBeNull(); + }); + + it("encodes a real GIF89a stream from multiple same-size frames", async () => { + const frames = [ + { png: createSolidPng(20, 15, [255, 0, 0, 255]) }, + { png: createSolidPng(20, 15, [0, 255, 0, 255]) }, + { png: createSolidPng(20, 15, [0, 0, 255, 255]) }, + ]; + const gif = await encodeScrollGif(frames, 300); + expect(gif).toBeInstanceOf(Uint8Array); + expect(gifHeaderOf(gif!)).toBe("GIF89a"); + expect(gif!.length).toBeGreaterThan(0); + }); + + it("encodes a single-frame input into a valid (non-animated) GIF", async () => { + const gif = await encodeScrollGif([{ png: createSolidPng(10, 10, [1, 2, 3, 255]) }], 300); + expect(gifHeaderOf(gif!)).toBe("GIF89a"); + }); + + it("degrades to null when frames have mismatched dimensions (a decode/capture inconsistency)", async () => { + const frames = [{ png: createSolidPng(20, 15, [255, 0, 0, 255]) }, { png: createSolidPng(30, 15, [0, 255, 0, 255]) }]; + await expect(encodeScrollGif(frames, 300)).resolves.toBeNull(); + }); + + it("degrades to null when a frame isn't a valid PNG (never throws)", async () => { + const frames = [{ png: createSolidPng(10, 10, [1, 2, 3, 255]) }, { png: new Uint8Array([1, 2, 3, 4, 5]) }]; + await expect(encodeScrollGif(frames, 300)).resolves.toBeNull(); + }); +}); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 7d7eaa879c..4a052340ca 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [] }, linkedIssueSatisfaction: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], skipLabels: [], skipDocsOnly: null, maxAddedLines: 0, maxFiles: 0, baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [], gif: false }, linkedIssueSatisfaction: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 0d8cf4a3e6..8806b38a36 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -7,6 +7,7 @@ import { import { buildCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; import * as pixelDiffModule from "../../src/review/visual/pixel-diff"; import * as previewUrlModule from "../../src/review/visual/preview-url"; +import * as scrollGifModule from "../../src/review/visual/scroll-gif"; import * as shotModule from "../../src/review/visual/shot"; import { sha256Hex } from "../../src/utils/crypto"; import { createTestEnv } from "../helpers/d1"; @@ -15,10 +16,11 @@ import { createTestEnv } from "../helpers/d1"; * surface) — lets a test pre-seed a "cached" screenshot at the exact fingerprinted key capturePage derives, * without needing a real browser binding to produce fresh bytes. `failPut: true` makes every put() reject, * for testing the caller's own `.catch(() => undefined)` degrade-gracefully path. */ -function memoryReviewAudit(options: { failPut?: boolean } = {}): R2Bucket { +function memoryReviewAudit(options: { failPut?: boolean; failGet?: boolean } = {}): R2Bucket { const store = new Map(); return { async get(key: string) { + if (options.failGet) throw new Error("simulated storage read failure"); const bytes = store.get(key); return bytes ? ({ body: new Response(bytes).body } as unknown as R2ObjectBody) : null; }, @@ -743,3 +745,333 @@ describe("buildCapture theme matrix (#3678)", () => { } }); }); + +describe("buildCapture scroll-GIF wiring (#3612)", () => { + it("never captures scroll frames when review.visual.gif is unset, even when isScrollGifAvailable is true", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames"); + try { + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 30, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(captureScrollSpy).not.toHaveBeenCalled(); + expect(result.routes[0]?.beforeGifUrl).toBeUndefined(); + expect(result.routes[0]?.afterGifUrl).toBeUndefined(); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + } + }); + + it("never captures scroll frames when gif:true is configured but this build can't assemble GIFs (hosted mode)", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(false); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames"); + try { + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 31, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(captureScrollSpy).not.toHaveBeenCalled(); + expect(result.routes[0]?.beforeGifUrl).toBeUndefined(); + expect(result.routes[0]?.afterGifUrl).toBeUndefined(); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + } + }); + + it("captures + uploads both a before and after scroll GIF when gif:true and isScrollGifAvailable are both true", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3]), new Uint8Array([4, 5, 6])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(new Uint8Array([7, 8, 9])); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 32, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(captureScrollSpy).toHaveBeenCalledTimes(2); // before + after + expect(encodeSpy).toHaveBeenCalledTimes(2); + expect(result.routes[0]?.beforeGifUrl).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.afterGifUrl).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.beforeGifUrl).not.toBe(result.routes[0]?.afterGifUrl); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("does not attempt an after-GIF when there is no preview URL yet (afterPage is empty)", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(new Uint8Array([7, 8, 9])); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 33 }, // no previewUrl -> afterPage is "" + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(captureScrollSpy).toHaveBeenCalledTimes(1); // before only + expect(result.routes[0]?.beforeGifUrl).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.afterGifUrl).toBeUndefined(); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("reuses a cached scroll GIF without re-capturing frames on the next review of the same head", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(new Uint8Array([7, 8, 9])); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const target = { repoFullName: "owner/repo", prNumber: 34, previewUrl: "https://preview.example.com" }; + const files = ["apps/gittensory-ui/src/routes/app.index.tsx"]; + const first = await buildCapture(env, "installation-token", target, files, undefined, { gif: true }); + expect(captureScrollSpy).toHaveBeenCalledTimes(2); + const second = await buildCapture(env, "installation-token", target, files, undefined, { gif: true }); + expect(captureScrollSpy).toHaveBeenCalledTimes(2); // no NEW calls — both slots served from cache + expect(second.routes[0]?.beforeGifUrl).toBe(first.routes[0]?.beforeGifUrl); + expect(second.routes[0]?.afterGifUrl).toBe(first.routes[0]?.afterGifUrl); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("does not upload a GIF when the frames come back empty (auth-walled or render failure)", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ frames: [], authWalled: false }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif"); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 35, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(encodeSpy).not.toHaveBeenCalled(); + expect(result.routes[0]?.beforeGifUrl).toBeUndefined(); + expect(result.routes[0]?.afterGifUrl).toBeUndefined(); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("does not upload a GIF when the encoder degrades to null", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(null); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 36, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(result.routes[0]?.beforeGifUrl).toBeUndefined(); + expect(result.routes[0]?.afterGifUrl).toBeUndefined(); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("does not attempt a before-GIF when there is no production URL configured (beforePage is empty)", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(new Uint8Array([7, 8, 9])); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "", REVIEW_AUDIT: memoryReviewAudit() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 37, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(captureScrollSpy).toHaveBeenCalledTimes(1); // after only — before has no page to capture + expect(result.routes[0]?.beforeGifUrl).toBeUndefined(); + expect(result.routes[0]?.afterGifUrl).toContain("/gittensory/shot?key="); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("does not capture scroll frames when there is no REVIEW_AUDIT storage, even with gif:true configured", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames"); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com" }); // no REVIEW_AUDIT + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 38, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(captureScrollSpy).not.toHaveBeenCalled(); + expect(result.routes[0]?.beforeGifUrl).toBeUndefined(); + expect(result.routes[0]?.afterGifUrl).toBeUndefined(); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + } + }); + + it("threads the theme into the scroll-GIF fingerprint too, so a themed and untagged GIF never collide", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(new Uint8Array([7, 8, 9])); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 39, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true, themes: ["dark"] }, + ); + expect(result.routes[0]?.theme).toBe("dark"); + expect(result.routes[0]?.afterGifUrl).toContain("/gittensory/shot?key="); + const untaggedFingerprint = await sha256Hex(`39:scrollgif:after:desktop:https://preview.example.com/app`); + expect(result.routes[0]?.afterGifUrl).not.toContain(untaggedFingerprint.slice(0, 40)); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("degrades to a fresh capture (never throws) when the GIF cache lookup itself fails", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(new Uint8Array([7, 8, 9])); + try { + const env = createTestEnv({ + PUBLIC_API_ORIGIN: "https://worker.example", + PUBLIC_SITE_ORIGIN: "https://prod.example.com", + REVIEW_AUDIT: memoryReviewAudit({ failGet: true }), + }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 40, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(captureScrollSpy).toHaveBeenCalled(); // cache lookup failed -> falls through to a fresh capture + expect(result.routes[0]?.beforeGifUrl).toContain("/gittensory/shot?key="); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); + + it("degrades to no GIF (never throws) when captureScrollFrames itself rejects", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockRejectedValue(new Error("browser binding exhausted")); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 41, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(result.routes[0]?.beforeGifUrl).toBeUndefined(); + expect(result.routes[0]?.afterGifUrl).toBeUndefined(); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + } + }); + + it("still returns the GIF URL even when persisting it fails (fire-and-forget put, mirrors uploadDiffImage's own pattern)", async () => { + const gifAvailableSpy = vi.spyOn(scrollGifModule, "isScrollGifAvailable").mockReturnValue(true); + const captureScrollSpy = vi.spyOn(shotModule, "captureScrollFrames").mockResolvedValue({ + frames: [new Uint8Array([1, 2, 3])], + authWalled: false, + }); + const encodeSpy = vi.spyOn(scrollGifModule, "encodeScrollGif").mockResolvedValue(new Uint8Array([7, 8, 9])); + try { + const env = createTestEnv({ + PUBLIC_API_ORIGIN: "https://worker.example", + PUBLIC_SITE_ORIGIN: "https://prod.example.com", + REVIEW_AUDIT: memoryReviewAudit({ failPut: true }), + }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 42, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { gif: true }, + ); + expect(result.routes[0]?.beforeGifUrl).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.afterGifUrl).toContain("/gittensory/shot?key="); + } finally { + gifAvailableSpy.mockRestore(); + captureScrollSpy.mockRestore(); + encodeSpy.mockRestore(); + } + }); +}); diff --git a/test/unit/visual-collapsible.test.ts b/test/unit/visual-collapsible.test.ts index affd4cfd6b..1177bf35b2 100644 --- a/test/unit/visual-collapsible.test.ts +++ b/test/unit/visual-collapsible.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { buildBeforeAfterCollapsible, buildUnifiedCommentBody } from "../../src/review/unified-comment-bridge"; +import { buildBeforeAfterCollapsible, buildScrollPreviewCollapsible, buildUnifiedCommentBody } from "../../src/review/unified-comment-bridge"; import type { GateCheckEvaluation } from "../../src/rules/advisory"; import type { PublicPrPanelSignalRow } from "../../src/signals/engine"; import type { CaptureRoute } from "../../src/review/visual/capture"; @@ -151,6 +151,108 @@ describe("buildBeforeAfterCollapsible", () => { }); }); +describe("buildScrollPreviewCollapsible (#3612)", () => { + it("renders a 'Scroll preview' table (no Viewport column) when a route has a scroll GIF", () => { + const c = buildScrollPreviewCollapsible([ + { + path: "/app/analytics", + beforeGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/before.gif", + afterGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/after.gif", + }, + ]); + expect(c).not.toBeNull(); + expect(c?.title).toBe("Scroll preview"); + expect(c?.rawHtml).toBe(true); + expect(c?.body).toContain("| Route | Before (production) | After (this PR's preview) |"); + expect(c?.body).not.toContain("Viewport"); + expect(c?.body).toContain("`/app/analytics`"); + expect(c?.body).toContain(' { + expect(buildScrollPreviewCollapsible([])).toBeNull(); + expect(buildScrollPreviewCollapsible([{ path: "/" }])).toBeNull(); + expect( + buildScrollPreviewCollapsible([{ path: "/", beforeUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.png" }]), + ).toBeNull(); + }); + + it("renders a dash when only one side has a GIF", () => { + const c = buildScrollPreviewCollapsible([{ path: "/", afterGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.gif" }]); + expect(c?.body).toContain("| `/` | — | { + const c = buildScrollPreviewCollapsible([ + { path: "/", theme: "dark", afterGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.gif" }, + ]); + expect(c?.body).toContain("| `/` (dark) |"); + expect(c?.body).toContain('alt="after / (dark) (scroll)"'); + }); + + it("escapes a pipe in the route path so it can't break the markdown table", () => { + const c = buildScrollPreviewCollapsible([{ path: "/a|b", afterGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.gif" }]); + expect(c?.body).toContain("`/a\\|b`"); + }); + + it("escapes route captions before embedding them in the trusted raw HTML table", () => { + const c = buildScrollPreviewCollapsible([ + { + path: "/p`

✅ FORGED APPROVAL

maintainer click here", + afterGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.gif", + }, + ]); + expect(c?.body).toContain("`/p\\`<h2>✅ FORGED APPROVAL</h2><a href=x>maintainer click here</a>`"); + expect(c?.body).not.toContain("

✅ FORGED APPROVAL

"); + expect(c?.body).not.toContain("maintainer click here"); + }); +}); + +describe("buildUnifiedCommentBody scroll-GIF wiring (#3612)", () => { + const base = { + gate: gate(), + panelRows, + readinessTotal: 90, + changedFiles: 3, + footerMarkdown: footer, + }; + const gifRoutes: CaptureRoute[] = [ + { + path: "/app/analytics", + beforeUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/abc.png", + afterUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/def.png", + beforeGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/before.gif", + afterGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/after.gif", + }, + ]; + + it("appends 'Scroll preview' ALONGSIDE 'Visual preview' when a route has a GIF", () => { + const body = buildUnifiedCommentBody({ ...base, beforeAfter: gifRoutes }); + expect(body).toContain("Visual preview"); + expect(body).toContain("Scroll preview"); + const visualIndex = body.indexOf("Visual preview"); + const scrollIndex = body.indexOf("Scroll preview"); + expect(scrollIndex).toBeGreaterThan(visualIndex); + }); + + it("does NOT add a Scroll preview section when no route has a GIF (flag-OFF parity)", () => { + const body = buildUnifiedCommentBody({ ...base, beforeAfter: routes }); + expect(body).toContain("Visual preview"); + expect(body).not.toContain("Scroll preview"); + }); + + it("still appends 'Scroll preview' when a route has a GIF but no static before/after shot (no Visual preview section)", () => { + const body = buildUnifiedCommentBody({ + ...base, + beforeAfter: [{ path: "/x", afterGifUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/x.gif" }], + }); + expect(body).not.toContain("Visual preview"); + expect(body).toContain("Scroll preview"); + }); +}); + describe("buildUnifiedCommentBody beforeAfter wiring", () => { const base = { gate: gate(), diff --git a/test/unit/visual-config-wiring.test.ts b/test/unit/visual-config-wiring.test.ts index b381dc2dde..43b0ccdf57 100644 --- a/test/unit/visual-config-wiring.test.ts +++ b/test/unit/visual-config-wiring.test.ts @@ -19,6 +19,7 @@ describe("review.visual wiring (#3609 / #3610)", () => { preview: { urlTemplate: "https://pr-{number}.preview.example.com" }, routes: { paths: ["/pricing"], maxRoutes: 3 }, themes: [], + gif: false, }); expect(loadSpy).toHaveBeenCalledWith(expect.anything(), "acme/widgets"); loadSpy.mockRestore(); diff --git a/test/unit/visual-shot.test.ts b/test/unit/visual-shot.test.ts index df7345e3ab..7f93edaf2b 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 { captureShot, handleShot } from "../../src/review/visual/shot"; +import { captureScrollFrames, captureShot, handleShot } from "../../src/review/visual/shot"; const mocks = vi.hoisted(() => ({ finalUrl: "https://preview.pages.dev/page", @@ -9,6 +9,12 @@ const mocks = vi.hoisted(() => ({ close: vi.fn(async () => undefined), launch: vi.fn(), emulateMediaFeatures: vi.fn(async () => undefined), + evaluate: vi.fn(), + // captureScrollFrames' FIRST page.evaluate() call queries scrollHeight; every later call (scrollTo, the + // settle delay) discards its return value — so only the first call's resolved value matters to the code + // under test, regardless of exactly how many scroll/settle evaluate() calls happen after it. + scrollHeight: 900, + evaluateCallCount: 0, })); vi.mock("@cloudflare/puppeteer", () => ({ @@ -52,6 +58,20 @@ describe("visual screenshot on-demand SSRF guard", () => { beforeEach(() => { vi.clearAllMocks(); mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.scrollHeight = 900; + mocks.evaluateCallCount = 0; + mocks.evaluate.mockImplementation(async (fn: (...fnArgs: unknown[]) => unknown, ...fnArgs: unknown[]) => { + mocks.evaluateCallCount++; + // The real callback runs inside the browser's own realm (document/window), which this Node test + // environment doesn't have — invoking it anyway and swallowing the inevitable throw is enough to + // exercise its body (real coverage, not just "the mock was configured") without needing a real DOM. + try { + fn(...fnArgs); + } catch { + // expected — see above. + } + return mocks.evaluateCallCount === 1 ? mocks.scrollHeight : undefined; + }); mocks.launch.mockImplementation(async () => { let onRequest: ((request: ReturnType) => void) | undefined; return { @@ -68,6 +88,7 @@ describe("visual screenshot on-demand SSRF guard", () => { }), url: vi.fn(() => mocks.finalUrl), screenshot: mocks.screenshot, + evaluate: mocks.evaluate, }), close: mocks.close, }; @@ -194,6 +215,165 @@ describe("visual screenshot on-demand SSRF guard", () => { }); }); +describe("captureScrollFrames (#3612 scroll-through GIF evidence)", () => { + beforeEach(() => { + vi.clearAllMocks(); + mocks.finalUrl = "https://preview.pages.dev/page"; + mocks.scrollHeight = 900; + mocks.evaluateCallCount = 0; + mocks.evaluate.mockImplementation(async (fn: (...fnArgs: unknown[]) => unknown, ...fnArgs: unknown[]) => { + mocks.evaluateCallCount++; + // The real callback runs inside the browser's own realm (document/window), which this Node test + // environment doesn't have — invoking it anyway and swallowing the inevitable throw is enough to + // exercise its body (real coverage, not just "the mock was configured") without needing a real DOM. + try { + fn(...fnArgs); + } catch { + // expected — see above. + } + return mocks.evaluateCallCount === 1 ? mocks.scrollHeight : undefined; + }); + mocks.launch.mockImplementation(async () => { + let onRequest: ((request: ReturnType) => void) | undefined; + return { + newPage: async () => ({ + setRequestInterception: vi.fn(async () => undefined), + on: vi.fn((event: string, callback: (request: ReturnType) => void) => { + if (event === "request") onRequest = callback; + }), + setViewport: vi.fn(async () => undefined), + emulateMediaFeatures: mocks.emulateMediaFeatures, + goto: vi.fn(async (url: string) => { + onRequest?.(makeRequest(url)); + if (mocks.finalUrl !== url) onRequest?.(makeRequest(mocks.finalUrl)); + }), + url: vi.fn(() => mocks.finalUrl), + screenshot: mocks.screenshot, + evaluate: mocks.evaluate, + }), + close: mocks.close, + }; + }); + }); + + it("rejects an unsafe target before launching the browser", async () => { + const result = await captureScrollFrames(env(), "http://127.0.0.1/admin", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); + expect(mocks.launch).not.toHaveBeenCalled(); + }); + + it("captures MAX_SCROLL_STEPS viewport-cropped frames for a page much taller than one viewport", async () => { + mocks.scrollHeight = 900 * 10; // a 10-viewport-tall page + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result.authWalled).toBe(false); + expect(result.frames).toHaveLength(6); + expect(mocks.screenshot).toHaveBeenCalledTimes(6); + expect(mocks.screenshot).toHaveBeenCalledWith({ type: "png", fullPage: false }); + }); + + it("captures exactly one frame for a page that fits within a single viewport (nothing to scroll through)", async () => { + mocks.scrollHeight = 500; // shorter than the 900px viewport + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result.frames).toHaveLength(1); + expect(mocks.screenshot).toHaveBeenCalledTimes(1); + }); + + it("emulates prefers-color-scheme when a theme is requested, same as captureShot", async () => { + await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }, { theme: "dark" }); + expect(mocks.emulateMediaFeatures).toHaveBeenCalledWith([{ name: "prefers-color-scheme", value: "dark" }]); + }); + + it("returns no frames when a redirect leads to a private endpoint", async () => { + mocks.finalUrl = "http://127.0.0.1/admin"; + const result = await captureScrollFrames(env(), "https://attacker.workers.dev/redirect", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); + expect(mocks.screenshot).not.toHaveBeenCalled(); + expect(mocks.close).toHaveBeenCalled(); + }); + + it("flags authWalled and captures no frames on a login-page redirect", async () => { + mocks.finalUrl = "https://preview.pages.dev/login"; + const result = await captureScrollFrames(env(), "https://preview.pages.dev/dashboard", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: true }); + expect(mocks.screenshot).not.toHaveBeenCalled(); + }); + + it("returns no frames when there is no BROWSER binding", async () => { + const result = await captureScrollFrames({} as Env, "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); + expect(mocks.launch).not.toHaveBeenCalled(); + }); + + it("degrades to no frames when the browser throws mid-capture", async () => { + mocks.launch.mockRejectedValueOnce(new Error("binding exhausted")); + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); + }); + + it("aborts a sub-request whose URL fails to parse", async () => { + mocks.finalUrl = "::::not-a-url"; + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); + expect(mocks.abort).toHaveBeenCalled(); + expect(mocks.screenshot).not.toHaveBeenCalled(); + }); + + it("swallows continue() and abort() rejections on the allowed + unparseable sub-requests", async () => { + mocks.continue.mockRejectedValueOnce(new Error("continue failed")); + mocks.abort.mockRejectedValueOnce(new Error("abort failed")); + mocks.finalUrl = "::::not-a-url"; + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); + }); + + it("swallows an abort() rejection on an unsafe-host sub-request", async () => { + mocks.abort.mockRejectedValueOnce(new Error("abort failed")); + mocks.finalUrl = "http://127.0.0.1/admin"; + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); + }); + + it("swallows a close() rejection in the finally block", async () => { + mocks.close.mockRejectedValueOnce(new Error("close failed")); + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result.authWalled).toBe(false); + expect(mocks.close).toHaveBeenCalled(); + }); + + it("does not apply the http SSRF check to a non-http(s) sub-request protocol", async () => { + mocks.finalUrl = "ftp://files.example.com/x"; + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }); + expect(result).toEqual({ frames: [], authWalled: false }); // final url is non-http(s) -> redirect-blocked downstream + expect(mocks.continue).toHaveBeenCalled(); + }); + + it("honors a caller-supplied isAllowedUrl on both the initial target and each sub-request", async () => { + const isAllowedUrl = vi.fn((candidate: string) => candidate === "https://preview.pages.dev/page"); + mocks.finalUrl = "https://preview.pages.dev/page"; + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }, { isAllowedUrl }); + expect(result.authWalled).toBe(false); + expect(result.frames.length).toBeGreaterThan(0); + expect(mocks.continue).toHaveBeenCalled(); + expect(isAllowedUrl).toHaveBeenCalledWith("https://preview.pages.dev/page"); + }); + + it("rejects the target up front when isAllowedUrl disallows it, before launching the browser", async () => { + const isAllowedUrl = vi.fn(() => false); + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }, { isAllowedUrl }); + expect(result).toEqual({ frames: [], authWalled: false }); + expect(mocks.launch).not.toHaveBeenCalled(); + }); + + it("aborts a sub-request whose navigation isAllowedUrl disallows, even though the host itself is otherwise safe", async () => { + const isAllowedUrl = vi.fn((candidate: string) => candidate === "https://preview.pages.dev/page"); + mocks.finalUrl = "https://preview.pages.dev/other-page"; // safe host, but not the one isAllowedUrl accepts + const result = await captureScrollFrames(env(), "https://preview.pages.dev/page", { width: 1440, height: 900 }, { isAllowedUrl }); + expect(result).toEqual({ frames: [], authWalled: false }); + expect(mocks.abort).toHaveBeenCalled(); + expect(mocks.screenshot).not.toHaveBeenCalled(); + }); +}); + describe("visual screenshot placeholder cards", () => { it("serves the loading spinner SVG for placeholder=loading", async () => { const response = await handleShot(shotRequest("placeholder=loading"), {} as Env); @@ -240,6 +420,16 @@ describe("visual screenshot R2 key serve + traversal guard", () => { expect(new Uint8Array(await response.arrayBuffer())).toEqual(png); }); + it("serves a .gif key with an image/gif content-type (#3612) — extension-derived, not stored httpMetadata", async () => { + const gif = new Uint8Array([1, 2, 3, 4]); + const key = "gittensory/shots/abc.gif"; + const response = await handleShot(shotRequest(`key=${encodeURIComponent(key)}`), r2Env({ [key]: gif })); + + expect(response.status).toBe(200); + expect(response.headers.get("content-type")).toBe("image/gif"); + expect(new Uint8Array(await response.arrayBuffer())).toEqual(gif); + }); + it("returns 404 for a valid key that is absent from R2", async () => { const response = await handleShot( shotRequest(`key=${encodeURIComponent("gittensory/shots/missing.png")}`), diff --git a/test/unit/worker-entry-boundary.test.ts b/test/unit/worker-entry-boundary.test.ts index 443c52e38f..efc58ad0c0 100644 --- a/test/unit/worker-entry-boundary.test.ts +++ b/test/unit/worker-entry-boundary.test.ts @@ -10,7 +10,7 @@ const WORKER_ENTRY = join(srcRoot, "index.ts"); const MCP_BIN = join(root, "packages/gittensory-mcp/bin/gittensory-mcp.js"); const FORBIDDEN_PATH = /(?:^|\/)visual-agent\//; -const FORBIDDEN_IDENTIFIERS = /\b(?:pixelmatch|pngjs|visual-diff)\b/; +const FORBIDDEN_IDENTIFIERS = /\b(?:pixelmatch|pngjs|visual-diff|gifenc)\b/; function resolveLocalImport(fromFile: string, specifier: string): string | null { if (!specifier.startsWith(".")) return null; @@ -72,17 +72,17 @@ describe("worker entry boundary", () => { expect(forbidden, `worker entry must not reach agent-only modules: ${forbidden.join(", ")}`).toEqual([]); }); - it("does not reference pixelmatch, pngjs, or visual-diff in worker-reachable source", () => { + it("does not reference pixelmatch, pngjs, visual-diff, or gifenc in worker-reachable source", () => { const hits = collectReachableSources(WORKER_ENTRY) .map((file) => { const content = readFileSync(file, "utf8"); return FORBIDDEN_IDENTIFIERS.test(content) ? relativeToRoot(file) : null; }) .filter((entry): entry is string => entry !== null); - expect(hits, `worker-reachable files must not mention Node-only visual diff deps: ${hits.join(", ")}`).toEqual([]); + expect(hits, `worker-reachable files must not mention Node-only visual diff/GIF deps: ${hits.join(", ")}`).toEqual([]); }); - it("does not reference visual diff modules in the published MCP bin bundle", () => { + it("does not reference visual diff or GIF modules in the published MCP bin bundle", () => { const content = readFileSync(MCP_BIN, "utf8"); expect(content).not.toMatch(FORBIDDEN_IDENTIFIERS); expect(content).not.toMatch(/visual-agent/);