Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -463,16 +463,19 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl
// previously only present as the invisible `alt` attribute, never rendered as visible text. <br> (not a
// literal newline, which would break the GFM table row) keeps the caption inside the same cell; <sub> 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 ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(url)}"></a><br><sub>${attr(label)}</sub>` : "—";
// `imgUrl` (defaults to `url`) is what the <img src> loads; `url` is ALWAYS what the <a href> 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 ? `<a href="${attr(url)}" target="_blank" rel="noopener"><img width="360" alt="${attr(label)}" src="${attr(imgUrl)}"></a><br><sub>${attr(label)}</sub>` : "—";
const rows: string[] = [];
let hasAnyDiff = false;
for (const route of routes) {
const path = markdownCode(route.path);
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;
Expand Down
49 changes: 43 additions & 6 deletions src/review/visual/capture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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";
Expand Down Expand Up @@ -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 <img> 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;
Expand Down Expand Up @@ -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
Expand All @@ -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"
Expand All @@ -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 };
Expand All @@ -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);
Expand Down Expand Up @@ -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 } : {}),
Expand Down
21 changes: 20 additions & 1 deletion src/review/visual/image-downscale.ts
Original file line number Diff line number Diff line change
@@ -1,11 +1,30 @@
// 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
// directly. Mirrors the pixel-diff.ts seam exactly: `scripts/build-selfhost.mjs`'s esbuild plugin swaps
// 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 <img>, 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<Uint8Array> {
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<Uint8Array> {
return png;
}
31 changes: 31 additions & 0 deletions src/selfhost/stubs/image-downscale.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,34 @@ export async function downscaleForVision(png: Uint8Array): Promise<Uint8Array> {
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<Uint8Array> {
try {
const resized = await sharp(png)
.resize({ width: DISPLAY_MAX_WIDTH_PX, withoutEnlargement: true })
.png()
.toBuffer();
return new Uint8Array(resized);
} catch {
return png;
}
}
18 changes: 17 additions & 1 deletion test/unit/image-downscale.test.ts
Original file line number Diff line number Diff line change
@@ -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 () => {
Expand All @@ -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);
});
});
31 changes: 30 additions & 1 deletion test/unit/selfhost-image-downscale-stub.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<Uint8Array> {
const buf = await sharp({ create: { width, height, channels: 3, background: { r: 10, g: 20, b: 30 } } })
Expand Down Expand Up @@ -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);
});
});
Loading
Loading