diff --git a/scripts/build-selfhost.mjs b/scripts/build-selfhost.mjs index f1a17654ae..5fa03e44a4 100644 --- a/scripts/build-selfhost.mjs +++ b/scripts/build-selfhost.mjs @@ -47,6 +47,11 @@ await esbuild.build({ build.onResolve({ filter: /^cloudflare:workers$/ }, () => ({ path: resolve(root, "src/selfhost/cf-workers-shim.ts") })); build.onResolve({ filter: /^@cloudflare\/puppeteer$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/puppeteer.ts") })); build.onResolve({ filter: /^agents\/mcp$/ }, () => ({ path: resolve(root, "src/selfhost/stubs/agents-mcp.ts") })); + // Worker-safe no-op → real pixel-diff (pixelmatch/pngjs need Node Buffer, forbidden in the Worker + // bundle by test/unit/worker-entry-boundary.test.ts). Exact match: capture.ts is the only value + // 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") })); }, }, ], diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 272a041e77..27d98e5ab5 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -338,10 +338,13 @@ export type UnifiedCommentBridgeArgs = { * CLICKABLE THUMBNAILS: a small `` (GitHub caps it to the column width) wrapped in an `` to the * SAME full-resolution shot, so a click opens the screenshot full-size. One row per route per viewport * (desktop / mobile), with the route path as the caption and a before (production) vs after (this PR's preview) - * column. Emitted as TRUSTED raw HTML (`rawHtml: true`) so the `/` survive — public-safe by - * construction: every value is a first-party minted /gittensory/shot URL or a route path (no private rubric / - * scoring terms), and a stray `"` in a URL is neutralized so it can't break out of the attribute. Returns null - * when nothing is renderable (no route has any shot URL), so the section is omitted rather than shown empty. + * column, plus a Diff column (#3674, self-host only) highlighting exactly what changed when a pixel-diff + * provider is available and finds a real visual difference — absent on hosted builds and any unchanged/no-diff + * cell, which render as a dash like every other missing shot. Emitted as TRUSTED raw HTML (`rawHtml: true`) so + * the `/` survive — public-safe by construction: every value is a first-party minted /gittensory/shot + * URL or a route path (no private rubric / scoring terms), and a stray `"` in a URL is neutralized so it can't + * break out of the attribute. Returns null when nothing is renderable (no route has any shot URL), so the + * section is omitted rather than shown empty. */ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedCollapsible | null { const attr = (value: string): string => @@ -355,22 +358,27 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl const cell = (url: string | undefined, label: string): string => url ? `${attr(label)}` : "—"; const rows: string[] = []; + let hasAnyDiff = false; for (const route of routes) { const path = markdownCode(route.path); if (route.beforeUrl || route.afterUrl) { - rows.push(`| ${path} | desktop | ${cell(route.beforeUrl, `before ${route.path}`)} | ${cell(route.afterUrl, `after ${route.path}`)} |`); + if (route.diffUrl) hasAnyDiff = true; + rows.push(`| ${path} | desktop | ${cell(route.beforeUrl, `before ${route.path}`)} | ${cell(route.afterUrl, `after ${route.path}`)} | ${cell(route.diffUrl, `diff ${route.path}`)} |`); } if (route.beforeUrlMobile || route.afterUrlMobile) { - rows.push(`| ${path} | mobile | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)`)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)`)} |`); + if (route.diffUrlMobile) hasAnyDiff = true; + rows.push(`| ${path} | mobile | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)`)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)`)} | ${cell(route.diffUrlMobile, `diff ${route.path} (mobile)`)} |`); } } if (rows.length === 0) return null; const body = [ - "| Route | Viewport | Before (production) | After (this PR's preview) |", - "| --- | --- | --- | --- |", + "| Route | Viewport | Before (production) | After (this PR's preview) | Diff |", + "| --- | --- | --- | --- | --- |", ...rows, "", - "_Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy._", + hasAnyDiff + ? "_Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy · Diff highlights exactly what changed._" + : "_Click any thumbnail to open the full-size screenshot. Before = production · After = this PR's preview deploy._", ].join("\n"); return { title: "Visual preview", body, rawHtml: true }; } diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 3c965f1800..8bd86a38a1 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -20,6 +20,7 @@ import { parseRepo, } from "./preview-url"; import { captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type Viewport } from "./shot"; +import { compareCapturedScreenshots, isVisualDiffAvailable, type VisualDiffOutcome } from "./pixel-diff"; const NAMESPACE = "gittensory"; const DEFAULT_ROUTES = ["/"]; @@ -28,13 +29,17 @@ const DEFAULT_ROUTE_FILE = /apps\/gittensory-ui\/src\/routes\/(.+?)\.(?:tsx|jsx) // wall-clock — Browser Rendering is the costliest binding. const MAX_ROUTES = 2; -/** A single captured route's before/after shot URLs (desktop + mobile). undefined slot ⇒ a dash cell. */ +/** A single captured route's before/after shot URLs (desktop + mobile), plus an optional pixel-diff overlay + * 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. */ export interface CaptureRoute { path: string; beforeUrl?: string | undefined; beforeUrlMobile?: string | undefined; afterUrl?: string | undefined; afterUrlMobile?: string | undefined; + diffUrl?: string | undefined; + diffUrlMobile?: string | undefined; } /** The capture pipeline's result: the rendered routes, plus whether a preview build is still pending. */ @@ -143,7 +148,12 @@ async function capturePage( slot: "before" | "after", viewportName: "desktop" | "mobile", viewport: Viewport, -): Promise<{ url?: string | undefined }> { + // #3674: when true, ALSO resolve the raw PNG bytes (not just the URL) so the caller can pixel-diff + // before+after — including on a cache hit, which is the COMMON case for "before" (the same production + // shot is reused across many PR reviews). Costs one extra read on a cache hit; false (every existing + // caller) skips it entirely, so this is zero-cost unless a caller opts in. + includeBytes = false, +): Promise<{ url?: string | undefined; png?: Uint8Array | undefined }> { if (!page) return {}; const shotBase = env.PUBLIC_API_ORIGIN; // this worker's public origin (serves /gittensory/shot) const onDemand = shotBase ? `${shotBase}/${NAMESPACE}/shot?url=${encodeURIComponent(page)}&w=${viewport.width}&h=${viewport.height}` : page; @@ -154,7 +164,11 @@ async function capturePage( const key = `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}.png`; const url = shotBase ? `${shotBase}/${NAMESPACE}/shot?key=${encodeURIComponent(key)}` : onDemand; const cached = await env.REVIEW_AUDIT.get(key).catch(() => null); - if (cached) return { url }; + if (cached) { + if (!includeBytes) return { url }; + const bytes = await new Response(cached.body).arrayBuffer().then((buf) => new Uint8Array(buf)).catch(() => undefined); + return { url, ...(bytes ? { png: bytes } : {}) }; + } const { png, authWalled } = await captureShot(env, page, viewport).catch(() => ({ png: null, authWalled: false })); // A protected route that redirected to a sign-in wall: show an honest "requires authentication" // placeholder rather than caching/serving a screenshot of the login screen. @@ -163,12 +177,31 @@ async function capturePage( } if (png) { await env.REVIEW_AUDIT.put(key, png, { httpMetadata: { contentType: "image/png" } }).catch(() => undefined); - return { url }; + return { url, ...(includeBytes ? { png } : {}) }; } } return { url: onDemand }; } +/** Upload a computed diff-overlay PNG to the same store `capturePage` uses, returning its shot URL — or + * undefined when there's no diff image (unchanged/new/removed/no-diff-provider), storage is unavailable, or + * the upload fails. Mirrors capturePage's own key/URL scheme so the diff shares its caching story. */ +async function uploadDiffImage( + env: Env, + target: CaptureTarget, + path: string, + viewportName: "desktop" | "mobile", + diff: VisualDiffOutcome | null, +): Promise { + if (!diff?.diffImagePng) return undefined; + const shotBase = env.PUBLIC_API_ORIGIN; + if (!env.REVIEW_AUDIT || !shotBase) return undefined; + const fingerprint = await sha256Hex(`${target.headSha ?? target.prNumber}:diff:${viewportName}:${path}`); + const key = `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}-diff.png`; + await env.REVIEW_AUDIT.put(key, diff.diffImagePng, { httpMetadata: { contentType: "image/png" } }).catch(() => undefined); + return `${shotBase}/${NAMESPACE}/shot?key=${encodeURIComponent(key)}`; +} + /** Per-repo `review.visual` config, as resolved by the caller from the manifest (#3609 / #3610). Absent ⇒ * byte-identical to today (GitHub-native discovery, automatic route inference, built-in route cap). */ export type VisualCaptureConfig = { preview?: VisualPreviewInput | null | undefined; routes?: VisualRoutesInput | null | undefined }; @@ -229,6 +262,9 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge const failedPlaceholder = shotBase ? `${shotBase}/${NAMESPACE}/shot?placeholder=failed` : undefined; const afterPlaceholder = previewFailed ? failedPlaceholder : loadingPlaceholder; + // #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(); const routes = resolveVisualRoutes(visualFiles, visualConfig?.routes); const captureRoutes: CaptureRoute[] = []; for (const path of routes) { @@ -236,10 +272,22 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge const afterPage = previewBase ? joinUrl(previewBase, path) : ""; // Render desktop + mobile for each slot in parallel (4 PNGs/route) to bound wall-clock. const [beforeShot, beforeMobileShot, afterShot, afterMobileShot] = await Promise.all([ - capturePage(env, target, beforePage, "before", "desktop", DESKTOP_VIEWPORT), - capturePage(env, target, beforePage, "before", "mobile", MOBILE_VIEWPORT), - afterPage ? capturePage(env, target, afterPage, "after", "desktop", DESKTOP_VIEWPORT) : Promise.resolve<{ url?: string | undefined }>({ url: afterPlaceholder }), - afterPage ? capturePage(env, target, afterPage, "after", "mobile", MOBILE_VIEWPORT) : Promise.resolve<{ url?: string | undefined }>({ url: afterPlaceholder }), + capturePage(env, target, beforePage, "before", "desktop", DESKTOP_VIEWPORT, diffAvailable), + capturePage(env, target, beforePage, "before", "mobile", MOBILE_VIEWPORT, diffAvailable), + afterPage ? capturePage(env, target, afterPage, "after", "desktop", DESKTOP_VIEWPORT, diffAvailable) : Promise.resolve<{ url?: string | undefined; png?: Uint8Array | undefined }>({ url: afterPlaceholder }), + afterPage ? capturePage(env, target, afterPage, "after", "mobile", MOBILE_VIEWPORT, diffAvailable) : Promise.resolve<{ url?: string | undefined; png?: Uint8Array | undefined }>({ url: afterPlaceholder }), + ]); + // A diff needs BOTH sides' real bytes — a placeholder/dash slot (no preview yet, auth-walled, render + // failure) has no `png`, so compareCapturedScreenshots degrades to null exactly like a missing shot does. + const [desktopDiff, mobileDiff] = diffAvailable + ? await Promise.all([ + compareCapturedScreenshots(beforeShot.png, afterShot.png), + compareCapturedScreenshots(beforeMobileShot.png, afterMobileShot.png), + ]) + : [null, null]; + const [diffUrl, diffUrlMobile] = await Promise.all([ + uploadDiffImage(env, target, path, "desktop", desktopDiff), + uploadDiffImage(env, target, path, "mobile", mobileDiff), ]); captureRoutes.push({ path, @@ -247,6 +295,8 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge beforeUrlMobile: beforeMobileShot.url, afterUrl: afterShot.url, afterUrlMobile: afterMobileShot.url, + ...(diffUrl ? { diffUrl } : {}), + ...(diffUrlMobile ? { diffUrlMobile } : {}), }); } return { routes: captureRoutes, previewPending }; diff --git a/src/review/visual/pixel-diff.ts b/src/review/visual/pixel-diff.ts new file mode 100644 index 0000000000..42bd6c27ad --- /dev/null +++ b/src/review/visual/pixel-diff.ts @@ -0,0 +1,33 @@ +// Pixel-comparison provider seam for the before/after capture pipeline (#3674). WORKER-SAFE DEFAULT: a no-op. +// +// The real screenshot-comparison logic (the self-host-only module under `src/visual-agent/`) depends on +// Node's `Buffer` and a native-leaning PNG-decode step, which the Cloudflare Workers runtime doesn't +// guarantee — that's why `test/unit/worker-entry-boundary.test.ts` forbids importing (or even naming, in +// worker-reachable file content) that module from the Worker entry (`src/index.ts`). This file is the seam: +// `capture.ts` (which IS 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 +// `@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 cost, until a Workers-compatible +// pixel-comparison path exists. +export type VisualDiffOutcome = { + status: "changed" | "unchanged" | "new" | "removed"; + changedPixelPercent: number | null; + diffImagePng: Uint8Array | null; +}; + +/** True when this build can actually compute a pixel diff (self-host only, see module header). Callers use + * this to decide whether it's worth paying the extra cost of holding/fetching screenshot bytes at all — + * always false here, so nothing about the existing capture path changes in hosted mode. */ +export function isVisualDiffAvailable(): boolean { + return false; +} + +/** Compare two screenshots. Always null in the Worker-safe default — self-host's swapped-in implementation + * does the real comparison. Callers must treat null as "no diff available for this cell", never a failure. */ +export async function compareCapturedScreenshots( + _before: Uint8Array | null | undefined, + _after: Uint8Array | null | undefined, +): Promise { + return null; +} diff --git a/src/selfhost/stubs/pixel-diff.ts b/src/selfhost/stubs/pixel-diff.ts new file mode 100644 index 0000000000..993a7dd6fb --- /dev/null +++ b/src/selfhost/stubs/pixel-diff.ts @@ -0,0 +1,33 @@ +// Self-host replacement for src/review/visual/pixel-diff.ts (#3674). Swapped in by +// scripts/build-selfhost.mjs's esbuild plugin, the same mechanism used for @cloudflare/puppeteer — this +// file is only ever bundled into dist/server.mjs, never the Worker entry, so it's safe to depend on +// pixelmatch/pngjs (Node `Buffer` + PNG decode) here. Unlike puppeteer-core, pixelmatch/pngjs are +// unconditional package.json dependencies (no INSTALL_VISUAL_REVIEW-style opt-in), so a plain static +// import is fine — no lazy runtime import needed. +import { compareRouteScreenshots } from "../../visual-agent/visual-diff"; +import type { VisualDiffOutcome } from "../../review/visual/pixel-diff"; + +export function isVisualDiffAvailable(): boolean { + return true; +} + +export async function compareCapturedScreenshots( + before: Uint8Array | null | undefined, + after: Uint8Array | null | undefined, +): Promise { + if (!before && !after) return null; + try { + const result = compareRouteScreenshots({ + route: "", + before: before ? Buffer.from(before) : null, + after: after ? Buffer.from(after) : null, + }); + return { + status: result.status, + changedPixelPercent: result.changedPixelPercent, + diffImagePng: result.diffImagePng ? new Uint8Array(result.diffImagePng) : null, + }; + } catch { + return null; + } +} diff --git a/test/unit/pixel-diff.test.ts b/test/unit/pixel-diff.test.ts new file mode 100644 index 0000000000..90692c7d45 --- /dev/null +++ b/test/unit/pixel-diff.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, it } from "vitest"; +import { compareCapturedScreenshots, isVisualDiffAvailable } from "../../src/review/visual/pixel-diff"; + +describe("pixel-diff Worker-safe default (#3674)", () => { + it("reports diffing as unavailable", () => { + expect(isVisualDiffAvailable()).toBe(false); + }); + + it("always resolves to null regardless of input, since the real implementation is self-host only", async () => { + const before = new Uint8Array([1, 2, 3]); + const after = new Uint8Array([4, 5, 6]); + await expect(compareCapturedScreenshots(before, after)).resolves.toBeNull(); + await expect(compareCapturedScreenshots(before, undefined)).resolves.toBeNull(); + await expect(compareCapturedScreenshots(null, after)).resolves.toBeNull(); + await expect(compareCapturedScreenshots(null, null)).resolves.toBeNull(); + }); +}); diff --git a/test/unit/selfhost-pixel-diff-stub.test.ts b/test/unit/selfhost-pixel-diff-stub.test.ts new file mode 100644 index 0000000000..246821cce0 --- /dev/null +++ b/test/unit/selfhost-pixel-diff-stub.test.ts @@ -0,0 +1,68 @@ +// Tests for the self-host pixel-diff stub (#3674). 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/visual-diff.test.ts's own fixture style. +import { PNG } from "pngjs"; +import { describe, expect, it } from "vitest"; +import { compareCapturedScreenshots, isVisualDiffAvailable } from "../../src/selfhost/stubs/pixel-diff"; + +function createSolidPng(width: number, height: number, rgba: [number, number, number, number]): Buffer { + 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 PNG.sync.write(png); +} + +describe("selfhost pixel-diff stub (#3674)", () => { + it("reports diffing as available", () => { + expect(isVisualDiffAvailable()).toBe(true); + }); + + it("returns null when both screenshots are missing", async () => { + await expect(compareCapturedScreenshots(null, null)).resolves.toBeNull(); + await expect(compareCapturedScreenshots(undefined, undefined)).resolves.toBeNull(); + }); + + it("flags a real visual change with a diff image and changed-pixel percentage", async () => { + const before = new Uint8Array(createSolidPng(40, 30, [255, 255, 255, 255])); + const after = new Uint8Array(createSolidPng(40, 30, [0, 0, 0, 255])); + const result = await compareCapturedScreenshots(before, after); + expect(result?.status).toBe("changed"); + expect(result?.changedPixelPercent).toBe(100); + expect(result?.diffImagePng).toBeInstanceOf(Uint8Array); + expect(result?.diffImagePng?.length).toBeGreaterThan(0); + }); + + it("marks identical screenshots unchanged without a diff image", async () => { + const png = new Uint8Array(createSolidPng(32, 24, [10, 20, 30, 255])); + const result = await compareCapturedScreenshots(png, png); + expect(result).toEqual({ status: "unchanged", changedPixelPercent: 0, diffImagePng: null }); + }); + + it("treats a missing before (new page) as status 'new' with no diff image", async () => { + const after = new Uint8Array(createSolidPng(10, 10, [1, 2, 3, 255])); + const result = await compareCapturedScreenshots(null, after); + expect(result?.status).toBe("new"); + expect(result?.diffImagePng).toBeNull(); + }); + + it("treats a missing after (removed page) as status 'removed' with no diff image", async () => { + const before = new Uint8Array(createSolidPng(10, 10, [1, 2, 3, 255])); + const result = await compareCapturedScreenshots(before, null); + expect(result?.status).toBe("removed"); + expect(result?.diffImagePng).toBeNull(); + }); + + it("degrades to null when the input bytes aren't a valid PNG (never throws)", async () => { + const garbage = new Uint8Array([1, 2, 3, 4, 5]); + const valid = new Uint8Array(createSolidPng(10, 10, [1, 2, 3, 255])); + await expect(compareCapturedScreenshots(garbage, valid)).resolves.toBeNull(); + }); +}); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index e57e432583..2ad61b49cc 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -5,9 +5,56 @@ import { latestGitHubRestRateLimitObservation, } from "../../src/github/client"; 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 shotModule from "../../src/review/visual/shot"; +import { sha256Hex } from "../../src/utils/crypto"; import { createTestEnv } from "../helpers/d1"; +/** Minimal in-memory R2Bucket-compatible store (mirrors the self-host filesystem blob-store's get/put + * 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 { + const store = new Map(); + return { + async get(key: string) { + const bytes = store.get(key); + return bytes ? ({ body: new Response(bytes).body } as unknown as R2ObjectBody) : null; + }, + async put(key: string, value: unknown) { + if (options.failPut) throw new Error("simulated storage failure"); + const bytes = new Uint8Array(await new Response(value as BodyInit).arrayBuffer()); + store.set(key, bytes); + return { key } as unknown as R2Object; + }, + } as unknown as R2Bucket; +} + +/** An R2Bucket whose get() resolves to a cached object whose body stream ERRORS when read — for testing + * capturePage's "cache hit but reading the bytes back fails" degrade-gracefully path. */ +function reviewAuditWithBrokenCachedBody(key: string): R2Bucket { + return { + async get(requestedKey: string) { + if (requestedKey !== key) return null; + const body = new ReadableStream({ + start(controller) { + controller.error(new Error("simulated read failure")); + }, + }); + return { body } as unknown as R2ObjectBody; + }, + async put() { + return { key } as unknown as R2Object; + }, + } as unknown as R2Bucket; +} + +async function shotKey(prNumber: number, slot: "before" | "after", viewportName: "desktop" | "mobile", page: string): Promise { + const fingerprint = await sha256Hex(`${prNumber}:${slot}:${viewportName}:${page}`); + return `gittensory/shots/${fingerprint.slice(0, 40)}.png`; +} + afterEach(() => { clearGitHubResponseCacheForTest(); vi.unstubAllGlobals(); @@ -322,3 +369,250 @@ describe("mapFilesToRoutes maxRoutes parameter", () => { expect(mapFilesToRoutes(manyFiles, undefined, 3)).toEqual(["/app", "/app/analytics", "/app/billing"]); }); }); + +describe("buildCapture pixel-diff wiring (#3674)", () => { + it("never calls the diff provider when diffing is unavailable (the real, unmocked default) — byte-identical to pre-#3674", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable"); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots"); + 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: 1, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(availableSpy).toHaveBeenCalled(); + expect(compareSpy).not.toHaveBeenCalled(); + expect(result.routes[0]?.diffUrl).toBeUndefined(); + expect(result.routes[0]?.diffUrlMobile).toBeUndefined(); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); + + it("uploads a diff image and threads diffUrl when the provider reports a real change", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue({ + status: "changed", + changedPixelPercent: 12.5, + diffImagePng: new Uint8Array([1, 2, 3, 4]), + }); + 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: 2, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(result.routes[0]?.diffUrl).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.diffUrlMobile).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.diffUrl).not.toBe(result.routes[0]?.diffUrlMobile); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); + + it("does not attach a diffUrl when the provider reports no visible change (no diff image)", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue({ + status: "unchanged", + changedPixelPercent: 0, + diffImagePng: null, + }); + try { + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 3, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(result.routes[0]?.diffUrl).toBeUndefined(); + expect(result.routes[0]?.diffUrlMobile).toBeUndefined(); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); + + it("skips the diff upload gracefully when REVIEW_AUDIT/PUBLIC_API_ORIGIN aren't configured, even with a real diff image", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue({ + status: "changed", + changedPixelPercent: 40, + diffImagePng: new Uint8Array([9, 9, 9]), + }); + try { + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "", PUBLIC_SITE_ORIGIN: "https://prod.example.com" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 4, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(result.routes[0]?.diffUrl).toBeUndefined(); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); + + it("passes cached screenshot bytes (not just the URL) to the diff provider on a cache hit — the common case for a reused 'before' shot", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue(null); + try { + const env = createTestEnv({ + PUBLIC_API_ORIGIN: "https://worker.example", + PUBLIC_SITE_ORIGIN: "https://prod.example.com", + REVIEW_AUDIT: memoryReviewAudit(), + }); + const beforeBytes = new Uint8Array([10, 20, 30]); + const afterBytes = new Uint8Array([40, 50, 60]); + const beforeKey = await shotKey(5, "before", "desktop", "https://prod.example.com/app"); + const afterKey = await shotKey(5, "after", "desktop", "https://preview.example.com/app"); + await env.REVIEW_AUDIT!.put(beforeKey, beforeBytes, {} as R2PutOptions); + await env.REVIEW_AUDIT!.put(afterKey, afterBytes, {} as R2PutOptions); + + await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 5, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + const desktopCall = compareSpy.mock.calls.find(([before, after]) => before !== undefined || after !== undefined); + expect(desktopCall?.[0]).toEqual(beforeBytes); + expect(desktopCall?.[1]).toEqual(afterBytes); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); + + it("returns just the URL (no bytes) on a cache hit when diffing is unavailable — the real default, includeBytes stays false", async () => { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const beforeKey = await shotKey(6, "before", "desktop", "https://prod.example.com/app"); + await env.REVIEW_AUDIT!.put(beforeKey, new Uint8Array([1, 2, 3]), {} as R2PutOptions); + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 6, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(result.routes[0]?.beforeUrl).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.diffUrl).toBeUndefined(); + }); + + it("degrades to no bytes (never throws) when reading a cached screenshot's body fails", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue(null); + try { + const beforeKey = await shotKey(7, "before", "desktop", "https://prod.example.com/app"); + const env = createTestEnv({ + PUBLIC_API_ORIGIN: "https://worker.example", + PUBLIC_SITE_ORIGIN: "https://prod.example.com", + REVIEW_AUDIT: reviewAuditWithBrokenCachedBody(beforeKey), + }); + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 7, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(result.routes[0]?.beforeUrl).toContain("/gittensory/shot?key="); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); + + it("returns just the URL (no bytes) for a fresh successful render when diffing is unavailable — the real default", async () => { + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: new Uint8Array([5, 5, 5]), authWalled: false }); + 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: 11, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(captureShotSpy).toHaveBeenCalled(); + expect(result.routes[0]?.beforeUrl).toContain("/gittensory/shot?key="); + expect(result.routes[0]?.diffUrl).toBeUndefined(); + } finally { + captureShotSpy.mockRestore(); + } + }); + + it("threads fresh screenshot bytes to the diff provider right after a successful render, not just on a cache hit", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue(null); + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: new Uint8Array([9, 9, 9]), authWalled: false }); + try { + const env = createTestEnv({ + PUBLIC_API_ORIGIN: "https://worker.example", + PUBLIC_SITE_ORIGIN: "https://prod.example.com", + REVIEW_AUDIT: memoryReviewAudit(), + }); + + await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 8, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(captureShotSpy).toHaveBeenCalled(); + const bothSidesFresh = compareSpy.mock.calls.find(([before, after]) => before !== undefined && after !== undefined); + expect(bothSidesFresh?.[0]).toEqual(new Uint8Array([9, 9, 9])); + expect(bothSidesFresh?.[1]).toEqual(new Uint8Array([9, 9, 9])); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("still returns a diff URL even when persisting the diff image fails (fire-and-forget put, mirrors capturePage's own pattern)", async () => { + const availableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue({ + status: "changed", + changedPixelPercent: 30, + diffImagePng: new Uint8Array([7, 7, 7]), + }); + 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: 10, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + + expect(result.routes[0]?.diffUrl).toContain("/gittensory/shot?key="); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); +}); diff --git a/test/unit/visual-collapsible.test.ts b/test/unit/visual-collapsible.test.ts index 165f412bcb..7ab482c0d5 100644 --- a/test/unit/visual-collapsible.test.ts +++ b/test/unit/visual-collapsible.test.ts @@ -71,6 +71,42 @@ describe("buildBeforeAfterCollapsible", () => { expect(c?.body).not.toContain("

✅ FORGED APPROVAL

"); expect(c?.body).not.toContain("maintainer click here"); }); + + it("renders a dash in the Diff column and the plain caption when no route has a diff image (#3674, e.g. hosted builds)", () => { + const c = buildBeforeAfterCollapsible(routes); + expect(c?.body).toContain("| Route | Viewport | Before (production) | After (this PR's preview) | Diff |"); + expect(c?.body).toContain("| `/app/analytics` | desktop | { + const c = buildBeforeAfterCollapsible([ + { + 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", + diffUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/abc-diff.png", + }, + ]); + expect(c?.body).toContain(' { + const c = buildBeforeAfterCollapsible([ + { + path: "/app/analytics", + beforeUrlMobile: "https://api.example.dev/gittensory/shot?key=gittensory/shots/abc-m.png", + afterUrlMobile: "https://api.example.dev/gittensory/shot?key=gittensory/shots/def-m.png", + diffUrlMobile: "https://api.example.dev/gittensory/shot?key=gittensory/shots/abc-diff-m.png", + }, + ]); + expect(c?.body).toContain("| `/app/analytics` | mobile |"); + expect(c?.body).toContain(' {