diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index e3ffcc9222..e4baf45af4 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -463,8 +463,11 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl // previously only present as the invisible `alt` attribute, never rendered as visible text.
(not a // literal newline, which would break the GFM table row) keeps the caption inside the same cell; is // the same de-emphasized styling this table already uses for its own footer legend line below. - const cell = (url: string | undefined, label: string): string => - url ? `${attr(label)}
${attr(label)}` : "—"; + // `imgUrl` (defaults to `url`) is what the loads; `url` is ALWAYS what the points at, so + // "click to open full-size" keeps resolving to the true original even when a smaller downscaled copy + // (route.beforeThumbUrl/afterThumbUrl, self-host only) is embedded inline instead. + const cell = (url: string | undefined, label: string, imgUrl: string = url ?? ""): string => + url ? `${attr(label)}
${attr(label)}` : "—"; const rows: string[] = []; let hasAnyDiff = false; for (const route of routes) { @@ -472,7 +475,7 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl const themeSuffix = route.theme ? ` (${route.theme})` : ""; if (route.beforeUrl || route.afterUrl) { if (route.diffUrl) hasAnyDiff = true; - rows.push(`| ${path} | desktop${themeSuffix} | ${cell(route.beforeUrl, `before ${route.path}${themeSuffix}`)} | ${cell(route.afterUrl, `after ${route.path}${themeSuffix}`)} | ${cell(route.diffUrl, `diff ${route.path}${themeSuffix}`)} |`); + rows.push(`| ${path} | desktop${themeSuffix} | ${cell(route.beforeUrl, `before ${route.path}${themeSuffix}`, route.beforeThumbUrl)} | ${cell(route.afterUrl, `after ${route.path}${themeSuffix}`, route.afterThumbUrl)} | ${cell(route.diffUrl, `diff ${route.path}${themeSuffix}`)} |`); } if (route.beforeUrlMobile || route.afterUrlMobile) { if (route.diffUrlMobile) hasAnyDiff = true; diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 3c89673502..c110562066 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -17,7 +17,7 @@ import { base64Encode, sha256Hex } from "../../utils/crypto"; import type { AiContentBlock } from "../../types"; import { isSafeHttpUrl } from "../content-lane/safe-url"; -import { downscaleForVision } from "./image-downscale"; +import { downscaleForDisplay, downscaleForVision, isDisplayDownscaleAvailable } from "./image-downscale"; import type { GitHubRateLimitAdmissionKey } from "../../github/client"; import { dispatchVisualCaptureFallback, fallbackShotR2Key, isFallbackDispatchInFlight, markFallbackDispatched } from "./actions-fallback"; import { MAX_PREVIEW_POLL_ATTEMPTS, previewPollAttemptCount, recordPreviewPollAttempt } from "./preview-poll-budget"; @@ -62,6 +62,13 @@ export interface CaptureRoute { beforeUrlMobile?: string | undefined; afterUrl?: string | undefined; afterUrlMobile?: string | undefined; + // #6324: a separate, downscaled DISPLAY copy of the desktop shot -- self-host only, desktop-only (see + // capturePage's own doc comment for why). beforeUrl/afterUrl above are UNCHANGED in meaning (still the + // full-resolution original, still what "click to open full-size" resolves to); these are additive fields + // the comment table prefers for the embedded when present, falling back to beforeUrl/afterUrl when + // absent (hosted mode, or a resize that didn't actually shrink anything). + beforeThumbUrl?: string | undefined; + afterThumbUrl?: string | undefined; diffUrl?: string | undefined; diffUrlMobile?: string | undefined; beforeGifUrl?: string | undefined; @@ -335,7 +342,7 @@ async function capturePage( // theming ignores prefers-color-scheme (see shot.ts's CaptureShotOptions.theme doc). Only takes effect // together with `theme`; undefined (every pre-#4109 caller) ⇒ byte-identical to today. themeStorageKey?: string | undefined, -): Promise<{ url?: string | undefined; png?: Uint8Array | undefined }> { +): Promise<{ url?: string | undefined; thumbUrl?: string | undefined; png?: Uint8Array | undefined }> { if (!page) return {}; const shotBase = env.PUBLIC_API_ORIGIN; // this worker's public origin (serves /loopover/shot) // Carries the theme (#3678) and, when set, the storage key (#4109) so a LATER on-demand fetch of this @@ -354,11 +361,22 @@ async function capturePage( ); const key = `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}.png`; const url = resolveShotUrl(env, key) || onDemand; + // #6324: a downscaled DISPLAY copy, stored at a SIBLING key so the original at `key` never changes -- + // diffing (compareCapturedScreenshots, via includeBytes below) always reads the true original on both a + // fresh render AND a cache hit, and "click to open full-size" keeps resolving to it unchanged. Self-host + // only (isDisplayDownscaleAvailable) and desktop-only: shot.ts's mobile viewport (390px) is already + // close enough to the table's own 360px display width that a third resized copy would save little. + const thumbKey = viewportName === "desktop" && isDisplayDownscaleAvailable() ? `${NAMESPACE}/shots/${fingerprint.slice(0, 40)}-thumb.png` : undefined; const cached = await env.REVIEW_AUDIT.get(key).catch(() => null); if (cached) { - if (!includeBytes) return { url }; + // Verified via a real read, not assumed from the original's own existence -- the sibling write below + // is best-effort and can independently fail, so a stale/missing thumb must fall back to the full-res + // URL rather than ever risk embedding a broken image link. + const thumbCached = thumbKey ? await env.REVIEW_AUDIT.get(thumbKey).catch(() => null) : null; + const thumbUrl = thumbCached && thumbKey ? resolveShotUrl(env, thumbKey) || undefined : undefined; + if (!includeBytes) return { url, ...(thumbUrl ? { thumbUrl } : {}) }; const bytes = await new Response(cached.body).arrayBuffer().then((buf) => new Uint8Array(buf)).catch(() => undefined); - return { url, ...(bytes ? { png: bytes } : {}) }; + return { url, ...(thumbUrl ? { thumbUrl } : {}), ...(bytes ? { png: bytes } : {}) }; } const { png, authWalled } = await captureShot(env, page, viewport, theme ? { theme, ...(themeStorageKey ? { themeStorageKey } : {}) } : {}).catch(() => ({ png: null, authWalled: false })); // A protected route that redirected to a sign-in wall: show an honest "requires authentication" @@ -368,7 +386,21 @@ async function capturePage( } if (png) { await env.REVIEW_AUDIT.put(key, png, { httpMetadata: { contentType: "image/png" } }).catch(() => undefined); - return { url, ...(includeBytes ? { png } : {}) }; + let thumbUrl: string | undefined; + if (thumbKey) { + const displayPng = await downscaleForDisplay(png).catch(() => png); + // Skip storing/using the thumb entirely when downscaling genuinely didn't shrink anything (an + // already-narrow capture, or a decode/resize failure that degraded to the original bytes) -- a + // byte-identical copy at a second key is pure waste, not an optimization. + if (displayPng.byteLength < png.byteLength) { + // thumbUrl must only be set once the write is CONFIRMED to have succeeded -- a bare + // `.catch(() => undefined)` here would swallow a write failure and still fall through to embed a + // URL for an object that was never actually stored, a broken image link for every viewer. + const stored = await env.REVIEW_AUDIT.put(thumbKey, displayPng, { httpMetadata: { contentType: "image/png" } }).then(() => true).catch(() => false); + if (stored) thumbUrl = resolveShotUrl(env, thumbKey) || undefined; + } + } + return { url, ...(thumbUrl ? { thumbUrl } : {}), ...(includeBytes ? { png } : {}) }; } } return { url: onDemand }; @@ -387,7 +419,10 @@ async function resolveFallbackAfterShot( viewportName: "desktop" | "mobile", actionsFallbackEnabled: boolean, placeholder: string | undefined, -): Promise<{ url?: string | undefined; png?: Uint8Array | undefined }> { +): Promise<{ url?: string | undefined; thumbUrl?: string | undefined; png?: Uint8Array | undefined }> { + // #6324: never produces a thumbUrl (the actions_fallback artifact is stored as-is, no display downscale + // applied) -- typed here purely so this function's return shape matches capturePage's, since buildCapture + // uses both interchangeably for the "after" desktop slot. if (!actionsFallbackEnabled || !env.REVIEW_AUDIT || !target.headSha) return { url: placeholder }; const key = await fallbackShotR2Key(target.headSha, path, viewportName); const cached = await env.REVIEW_AUDIT.get(key).catch(() => null); @@ -644,6 +679,8 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge beforeUrlMobile: beforeMobileShot.url, afterUrl: afterShot.url, afterUrlMobile: afterMobileShot.url, + ...(beforeShot.thumbUrl ? { beforeThumbUrl: beforeShot.thumbUrl } : {}), + ...(afterShot.thumbUrl ? { afterThumbUrl: afterShot.thumbUrl } : {}), ...(diffUrl ? { diffUrl } : {}), ...(diffUrlMobile ? { diffUrlMobile } : {}), ...(beforeGifUrl ? { beforeGifUrl } : {}), diff --git a/src/review/visual/image-downscale.ts b/src/review/visual/image-downscale.ts index 7f9242a768..e507c3cb81 100644 --- a/src/review/visual/image-downscale.ts +++ b/src/review/visual/image-downscale.ts @@ -1,4 +1,4 @@ -// Vision-image downscale provider seam (#4370). WORKER-SAFE DEFAULT: a no-op. +// Image downscale provider seam (#4370, extended #6324). WORKER-SAFE DEFAULT: a no-op. // // The real downscale uses a native image-resizing binding that can't run on the Cloudflare Workers runtime // — that's why `capture.ts` (which IS Worker-reachable) imports ONLY this file, never the real dependency @@ -6,6 +6,25 @@ // this specifier for a real implementation (`src/selfhost/stubs/image-downscale.ts`) when bundling the // self-host entry (`src/server.ts`). The Worker's own (wrangler) bundle never applies that swap, so hosted // mode always returns the input unchanged — zero behavior change, zero added cost. +// +// #6324 added downscaleForDisplay/isDisplayDownscaleAvailable alongside the original downscaleForVision: +// distinct purpose (a real, smaller thumbnail copy stored for the PR-comment table's , not a +// vision-call-only resize), distinct target size, but the identical hosted-no-op/self-host-real seam shape. export async function downscaleForVision(png: Uint8Array): Promise { return png; } + +/** True when this build can actually produce a downscaled DISPLAY copy (self-host only, see module header). + * Callers use this to decide whether generating + storing a separate thumbnail is worth it at all — always + * false here, so hosted mode never pays for a second R2 write or resize attempt that would be a no-op + * anyway. */ +export function isDisplayDownscaleAvailable(): boolean { + return false; +} + +/** Downscale `png` for DISPLAY (the PR-comment table's embedded thumbnail) — distinct from + * downscaleForVision's AI-call-sized resize above. A no-op here; callers must gate on + * isDisplayDownscaleAvailable() rather than assume this changed anything. */ +export async function downscaleForDisplay(png: Uint8Array): Promise { + return png; +} diff --git a/src/selfhost/stubs/image-downscale.ts b/src/selfhost/stubs/image-downscale.ts index c323120e20..6ef8579979 100644 --- a/src/selfhost/stubs/image-downscale.ts +++ b/src/selfhost/stubs/image-downscale.ts @@ -30,3 +30,34 @@ export async function downscaleForVision(png: Uint8Array): Promise { return png; } } + +/** True in self-host mode -- see image-downscale.ts's module header for why hosted mode can never do this. */ +export function isDisplayDownscaleAvailable(): boolean { + return true; +} + +/** Width cap for the DISPLAY thumbnail embedded in the PR-comment table (#6324) -- distinct from + * VISION_MAX_DIMENSION_PX above (a different caller, a different constraint). shot.ts's DESKTOP_VIEWPORT is + * 1440px wide; the table embeds the image at `width="360"` (a GitHub-rendered thumbnail), so every viewer's + * browser previously downloaded the full native-resolution capture just to display it shrunk 4x. 720px is + * 2x the display width -- sharp enough for a HiDPI/retina viewer, still a real reduction from 1440px (and a + * much larger one for a tall full-page capture, since height scales down proportionally too). */ +const DISPLAY_MAX_WIDTH_PX = 720; + +/** Downscale `png` so its width is at most {@link DISPLAY_MAX_WIDTH_PX}, preserving aspect ratio and never + * enlarging an already-narrow image (a mobile-viewport capture, already close to display width, passes + * through unchanged rather than being upscaled). Any decode/resize failure degrades to the ORIGINAL bytes, + * matching downscaleForVision's own "a full-size image beats no image" contract -- capturePage's caller + * falls back to the original URL entirely when this genuinely can't produce a smaller copy, so a failure + * here is never user-visible as a broken image, only as a missed optimization. */ +export async function downscaleForDisplay(png: Uint8Array): Promise { + try { + const resized = await sharp(png) + .resize({ width: DISPLAY_MAX_WIDTH_PX, withoutEnlargement: true }) + .png() + .toBuffer(); + return new Uint8Array(resized); + } catch { + return png; + } +} diff --git a/test/unit/image-downscale.test.ts b/test/unit/image-downscale.test.ts index 93699dd049..2965d9218d 100644 --- a/test/unit/image-downscale.test.ts +++ b/test/unit/image-downscale.test.ts @@ -1,5 +1,5 @@ import { describe, expect, it } from "vitest"; -import { downscaleForVision } from "../../src/review/visual/image-downscale"; +import { downscaleForDisplay, downscaleForVision, isDisplayDownscaleAvailable } from "../../src/review/visual/image-downscale"; describe("image-downscale Worker-safe default (#4370)", () => { it("returns the input bytes unchanged, since the real implementation is self-host only", async () => { @@ -12,3 +12,19 @@ describe("image-downscale Worker-safe default (#4370)", () => { await expect(downscaleForVision(empty)).resolves.toBe(empty); }); }); + +describe("image-downscale display-copy Worker-safe default (#6324)", () => { + it("isDisplayDownscaleAvailable is false -- the real implementation is self-host only", () => { + expect(isDisplayDownscaleAvailable()).toBe(false); + }); + + it("downscaleForDisplay returns the input bytes unchanged", async () => { + const png = new Uint8Array([137, 80, 78, 71, 4, 5, 6]); + await expect(downscaleForDisplay(png)).resolves.toBe(png); + }); + + it("downscaleForDisplay returns an empty input unchanged", async () => { + const empty = new Uint8Array([]); + await expect(downscaleForDisplay(empty)).resolves.toBe(empty); + }); +}); diff --git a/test/unit/selfhost-image-downscale-stub.test.ts b/test/unit/selfhost-image-downscale-stub.test.ts index 041a260ac4..a275f68a31 100644 --- a/test/unit/selfhost-image-downscale-stub.test.ts +++ b/test/unit/selfhost-image-downscale-stub.test.ts @@ -4,7 +4,7 @@ // / real PNG fixtures here, mirroring test/unit/selfhost-pixel-diff-stub.test.ts's own fixture style. import sharp from "sharp"; import { describe, expect, it } from "vitest"; -import { downscaleForVision } from "../../src/selfhost/stubs/image-downscale"; +import { downscaleForDisplay, downscaleForVision, isDisplayDownscaleAvailable } from "../../src/selfhost/stubs/image-downscale"; async function solidPng(width: number, height: number): Promise { const buf = await sharp({ create: { width, height, channels: 3, background: { r: 10, g: 20, b: 30 } } }) @@ -49,3 +49,32 @@ describe("selfhost image-downscale stub (#4370)", () => { expect(result).toBe(garbage); }); }); + +describe("selfhost image-downscale stub, display copy (#6324)", () => { + it("isDisplayDownscaleAvailable is true -- this IS the real self-host implementation", () => { + expect(isDisplayDownscaleAvailable()).toBe(true); + }); + + it("downscales a desktop-width capture (1440px, shot.ts's DESKTOP_VIEWPORT) so width is capped at 720, preserving aspect ratio", async () => { + const desktopShot = await solidPng(1440, 2397); // the exact dimensions observed live on JSONbored/metagraphed#6036 + const result = await downscaleForDisplay(desktopShot); + const { width, height } = await dimensionsOf(result); + expect(width).toBe(720); + expect(height).toBe(1199); // round(2397/1440 * 720) = round(1198.5) = 1199 + expect(result.byteLength).toBeLessThan(desktopShot.byteLength); + }); + + it("leaves an already-narrow image's dimensions unchanged (withoutEnlargement) -- e.g. a mobile-width capture", async () => { + const mobileShot = await solidPng(390, 844); + const result = await downscaleForDisplay(mobileShot); + const { width, height } = await dimensionsOf(result); + expect(width).toBe(390); + expect(height).toBe(844); + }); + + it("degrades to the ORIGINAL bytes (never drops the image) when the input isn't a valid image", async () => { + const garbage = new Uint8Array([1, 2, 3, 4, 5]); + const result = await downscaleForDisplay(garbage); + expect(result).toBe(garbage); + }); +}); diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 22bb42aafc..3fc75cd721 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -7,6 +7,7 @@ import { import { fallbackShotR2Key, markFallbackDispatched } from "../../src/review/visual/actions-fallback"; import { buildCapture, fetchExternalScreenshotContentBlock, fetchShotContentBlock, hasSuccessfulBotCapture, mapFilesToRoutes, resolvePreviewUrlTemplate, resolveVisualRoutes } from "../../src/review/visual/capture"; import type { CaptureRoute } from "../../src/review/visual/capture"; +import * as imageDownscaleModule from "../../src/review/visual/image-downscale"; import * as pixelDiffModule from "../../src/review/visual/pixel-diff"; import { MAX_PREVIEW_POLL_ATTEMPTS, previewPollAttemptCount, recordPreviewPollAttempt } from "../../src/review/visual/preview-poll-budget"; import * as previewUrlModule from "../../src/review/visual/preview-url"; @@ -17,18 +18,22 @@ 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; failGet?: boolean } = {}): R2Bucket { + * without needing a real browser binding to produce fresh bytes. `failPut`/`failGet: true` makes EVERY + * put()/get() reject, for testing the caller's own `.catch(() => undefined)` degrade-gracefully path. + * `failPutKeys`/`failGetKeys` (#6324) instead fail ONLY the listed keys, leaving every other key's + * read/write to behave normally -- needed to simulate a failure on ONE of the two sibling writes + * capturePage's thumbnail logic makes (the original succeeds, the thumb independently fails, or vice + * versa) without breaking the rest of the flow that has to succeed for the test to reach that code at all. */ +function memoryReviewAudit(options: { failPut?: boolean; failGet?: boolean; failPutKeys?: string[]; failGetKeys?: string[] } = {}): R2Bucket { const store = new Map(); return { async get(key: string) { - if (options.failGet) throw new Error("simulated storage read failure"); + if (options.failGet || options.failGetKeys?.includes(key)) throw new Error("simulated storage read failure"); 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"); + if (options.failPut || options.failPutKeys?.includes(key)) 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; @@ -60,6 +65,13 @@ async function shotKey(prNumber: number, slot: "before" | "after", viewportName: return `loopover/shots/${fingerprint.slice(0, 40)}.png`; } +/** #6324: the sibling key a downscaled DISPLAY copy is stored under -- same fingerprint as shotKey, `-thumb` + * suffix before the extension. */ +async function thumbKey(prNumber: number, slot: "before" | "after", viewportName: "desktop" | "mobile", page: string): Promise { + const fingerprint = await sha256Hex(`${prNumber}:${slot}:${viewportName}:${page}`); + return `loopover/shots/${fingerprint.slice(0, 40)}-thumb.png`; +} + afterEach(() => { clearGitHubResponseCacheForTest(); vi.unstubAllGlobals(); @@ -756,6 +768,262 @@ describe("buildCapture pixel-diff wiring (#3674)", () => { }); }); +describe("buildCapture display-thumbnail wiring (#6324)", () => { + it("never attempts a thumbnail when display downscaling is unavailable (the real, unmocked default) — byte-identical to pre-#6324", async () => { + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay"); + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: new Uint8Array([9, 9, 9]), authWalled: false }); + 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: 40, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + expect(downscaleSpy).not.toHaveBeenCalled(); + expect(result.routes[0]?.beforeThumbUrl).toBeUndefined(); + expect(result.routes[0]?.afterThumbUrl).toBeUndefined(); + } finally { + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("generates + stores a thumbnail and threads beforeThumbUrl/afterThumbUrl when downscaling is available and genuinely shrinks the image", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockResolvedValue(new Uint8Array([1])); + 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() }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 41, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + expect(result.routes[0]?.beforeThumbUrl).toContain("/loopover/shot?key="); + expect(result.routes[0]?.beforeThumbUrl).toContain("-thumb.png"); + expect(result.routes[0]?.afterThumbUrl).toContain("-thumb.png"); + // The full-resolution URL is UNCHANGED -- still what "click to open full-size" resolves to. + expect(result.routes[0]?.beforeUrl).not.toContain("-thumb.png"); + const storedThumb = await env.REVIEW_AUDIT!.get(await thumbKey(41, "before", "desktop", "https://prod.example.com/app")); + expect(storedThumb).not.toBeNull(); + } finally { + availableSpy.mockRestore(); + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("never generates a thumbnail for the mobile viewport, even when downscaling is available — 390px is already close to the table's 360px display width", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockResolvedValue(new Uint8Array([1])); + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: new Uint8Array([9, 9, 9]), authWalled: false }); + 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: 42, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + // downscaleForDisplay was called for the desktop slots only -- CaptureRoute has no mobile thumb field + // at all (by design), so there's nothing to assert false on the route itself; the real assertion is + // that the call count matches "desktop before + desktop after" (2), not 4 (every slot). + expect(downscaleSpy).toHaveBeenCalledTimes(2); + } finally { + availableSpy.mockRestore(); + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("skips storing/using a thumbnail when downscaling didn't actually shrink the image (already narrow, or a decode failure that degraded to the original bytes)", async () => { + const same = new Uint8Array([9, 9, 9]); + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockResolvedValue(same); + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: same, 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: 43, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + expect(result.routes[0]?.beforeThumbUrl).toBeUndefined(); + const storedThumb = await env.REVIEW_AUDIT!.get(await thumbKey(43, "before", "desktop", "https://prod.example.com/app")); + expect(storedThumb).toBeNull(); + } finally { + availableSpy.mockRestore(); + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("degrades to the original bytes (never throws) when downscaleForDisplay itself rejects", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockRejectedValue(new Error("simulated decode failure")); + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: new Uint8Array([9, 9, 9]), authWalled: false }); + 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: 44, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + expect(result.routes[0]?.beforeUrl).toContain("/loopover/shot?key="); + expect(result.routes[0]?.beforeThumbUrl).toBeUndefined(); + } finally { + availableSpy.mockRestore(); + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("re-verifies the thumbnail actually exists on a cache hit rather than assuming it from the original's own presence", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + // Pre-seed ONLY the original -- simulates the sibling thumb write having failed on a PRIOR render. + const beforeKey = await shotKey(45, "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: 45, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + + expect(result.routes[0]?.beforeUrl).toContain("/loopover/shot?key="); + expect(result.routes[0]?.beforeThumbUrl).toBeUndefined(); + } finally { + availableSpy.mockRestore(); + } + }); + + it("finds a genuinely-cached thumbnail on a cache hit and threads its URL", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + try { + const env = createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com", REVIEW_AUDIT: memoryReviewAudit() }); + const beforeKey = await shotKey(46, "before", "desktop", "https://prod.example.com/app"); + const beforeThumbKey = await thumbKey(46, "before", "desktop", "https://prod.example.com/app"); + await env.REVIEW_AUDIT!.put(beforeKey, new Uint8Array([1, 2, 3]), {} as R2PutOptions); + await env.REVIEW_AUDIT!.put(beforeThumbKey, new Uint8Array([1]), {} as R2PutOptions); + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 46, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + + expect(result.routes[0]?.beforeThumbUrl).toContain(encodeURIComponent(beforeThumbKey)); + } finally { + availableSpy.mockRestore(); + } + }); + + it("#6324 CORRECTNESS: the pixel-diff provider always receives the ORIGINAL full-resolution bytes, never the downscaled display copy — even though a thumbnail was generated in the SAME call", async () => { + const diffAvailableSpy = vi.spyOn(pixelDiffModule, "isVisualDiffAvailable").mockReturnValue(true); + const compareSpy = vi.spyOn(pixelDiffModule, "compareCapturedScreenshots").mockResolvedValue(null); + const downscaleAvailableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockResolvedValue(new Uint8Array([1])); + const originalBefore = new Uint8Array([10, 20, 30]); + const originalAfter = new Uint8Array([40, 50, 60]); + const captureShotSpy = vi + .spyOn(shotModule, "captureShot") + .mockImplementation(async (_env, url: string) => ({ png: url.includes("preview.example.com") ? originalAfter : originalBefore, authWalled: false })); + try { + 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: 47, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + const desktopCall = compareSpy.mock.calls.find(([before, after]) => before !== undefined || after !== undefined); + expect(desktopCall?.[0]).toEqual(originalBefore); + expect(desktopCall?.[1]).toEqual(originalAfter); + } finally { + diffAvailableSpy.mockRestore(); + compareSpy.mockRestore(); + downscaleAvailableSpy.mockRestore(); + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("falls back to no thumbUrl (never throws) when the thumb-key WRITE itself fails on a fresh render, even though the original write succeeded", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockResolvedValue(new Uint8Array([1])); + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: new Uint8Array([9, 9, 9]), authWalled: false }); + try { + const beforeThumb = await thumbKey(49, "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: memoryReviewAudit({ failPutKeys: [beforeThumb] }) }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 49, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + // The ORIGINAL still saved fine and is still usable -- only the thumb-specific optimization is absent. + expect(result.routes[0]?.beforeUrl).toContain("/loopover/shot?key="); + expect(result.routes[0]?.beforeThumbUrl).toBeUndefined(); + } finally { + availableSpy.mockRestore(); + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); + + it("falls back to no thumbUrl (never throws) when re-verifying the thumb's existence on a cache hit itself fails to read", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + try { + const beforeKey = await shotKey(50, "before", "desktop", "https://prod.example.com/app"); + const beforeThumb = await thumbKey(50, "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: memoryReviewAudit({ failGetKeys: [beforeThumb] }) }); + await env.REVIEW_AUDIT!.put(beforeKey, new Uint8Array([1, 2, 3]), {} as R2PutOptions); + + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 50, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + + expect(result.routes[0]?.beforeUrl).toContain("/loopover/shot?key="); + expect(result.routes[0]?.beforeThumbUrl).toBeUndefined(); + } finally { + availableSpy.mockRestore(); + } + }); + + it("links a thumbnail directly at the bucket instead of this instance's /loopover/shot proxy when REVIEW_AUDIT_S3_PUBLIC_URL is configured", async () => { + const availableSpy = vi.spyOn(imageDownscaleModule, "isDisplayDownscaleAvailable").mockReturnValue(true); + const downscaleSpy = vi.spyOn(imageDownscaleModule, "downscaleForDisplay").mockResolvedValue(new Uint8Array([1])); + 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(), + REVIEW_AUDIT_S3_PUBLIC_URL: "https://pub-abc123.r2.dev", + }); + const result = await buildCapture( + env, + "installation-token", + { repoFullName: "owner/repo", prNumber: 48, previewUrl: "https://preview.example.com" }, + ["apps/loopover-ui/src/routes/app.index.tsx"], + ); + const expectedThumbKey = await thumbKey(48, "before", "desktop", "https://prod.example.com/app"); + expect(result.routes[0]?.beforeThumbUrl).toBe(`https://pub-abc123.r2.dev/${expectedThumbKey}`); + } finally { + availableSpy.mockRestore(); + downscaleSpy.mockRestore(); + captureShotSpy.mockRestore(); + } + }); +}); + describe("buildCapture with REVIEW_AUDIT_S3_PUBLIC_URL configured (direct bucket links)", () => { it("links an already-cached shot directly at the bucket instead of this instance's /loopover/shot proxy", async () => { const env = createTestEnv({ diff --git a/test/unit/visual-collapsible.test.ts b/test/unit/visual-collapsible.test.ts index 5ad3955434..1188d97b49 100644 --- a/test/unit/visual-collapsible.test.ts +++ b/test/unit/visual-collapsible.test.ts @@ -63,6 +63,31 @@ describe("buildBeforeAfterCollapsible", () => { expect(c?.body).not.toContain("—
"); }); + it("#6324: the prefers a route's beforeThumbUrl/afterThumbUrl over the full-resolution beforeUrl/afterUrl, but ALWAYS points at the full-resolution original", () => { + const c = buildBeforeAfterCollapsible([ + { + path: "/app/analytics", + beforeUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/full-before.png", + beforeThumbUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/thumb-before.png", + afterUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/full-after.png", + afterThumbUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/thumb-after.png", + }, + ]); + // href = full-resolution (click to open full-size); img src = the smaller thumb. + expect(c?.body).toContain('before /app/analytics'); + expect(c?.body).toContain('after /app/analytics'); + // The full-resolution URLs never appear as an img src (only inside an href). + expect(c?.body).not.toContain('src="https://api.example.dev/gittensory/shot?key=gittensory/shots/full-before.png"'); + expect(c?.body).not.toContain('src="https://api.example.dev/gittensory/shot?key=gittensory/shots/full-after.png"'); + }); + + it("#6324: falls back to the full-resolution URL for the img src when no thumb URL is present (hosted mode, or mobile rows, which never get one)", () => { + const c = buildBeforeAfterCollapsible(routes); + // routes (the shared fixture below) has no beforeThumbUrl/afterThumbUrl -- src and href must be identical. + expect(c?.body).toContain('before /app/analytics'); + expect(c?.body).toContain('after /app/analytics'); + }); + it("returns null when no route has any shot URL (no empty table)", () => { expect(buildBeforeAfterCollapsible([])).toBeNull(); expect(buildBeforeAfterCollapsible([{ path: "/" }])).toBeNull();