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
5 changes: 5 additions & 0 deletions .gittensory.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,11 @@ settings:
# # Overrides the built-in cap (2) on how many routes get screenshotted per PR, whether they come from
# # `paths` above or automatic inference. Positive integer or null. Default: null (built-in default).
# max_routes: 3
# # Which `prefers-color-scheme` variants to capture (#3678). List of "light"/"dark", each rendered as a
# # separate before/after row. Empty/default ⇒ a single light-theme capture, byte-identical to today.
# themes:
# - light
# - dark
# # 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.
Expand Down
5 changes: 5 additions & 0 deletions config/examples/gittensory.full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -824,6 +824,11 @@ settings:
# # Overrides the built-in cap (2) on how many routes get screenshotted per PR, whether they come from
# # `paths` above or automatic inference. Positive integer or null. Default: null (built-in default).
# max_routes: 3
# # Which `prefers-color-scheme` variants to capture (#3678). List of "light"/"dark", each rendered as a
# # separate before/after row. Empty/default ⇒ a single light-theme capture, byte-identical to today.
# themes:
# - light
# - dark
# # 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.
Expand Down
22 changes: 12 additions & 10 deletions src/review/unified-comment-bridge.ts
Original file line number Diff line number Diff line change
Expand Up @@ -342,14 +342,15 @@ export type UnifiedCommentBridgeArgs = {
* Build the "Visual preview" collapsible from the before/after capture routes — a clean table whose cells are
* CLICKABLE THUMBNAILS: a small `<img>` (GitHub caps it to the column width) wrapped in an `<a href>` 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, 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 `<a>/<img>` 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.
* (desktop / mobile) per captured theme (#3678, e.g. "desktop (dark)" — unlabeled when a route has no theme,
* exactly like today), with the route path as the caption and a before (production) vs after (this PR's
* preview) 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 `<a>/<img>` 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 =>
Expand All @@ -366,13 +367,14 @@ export function buildBeforeAfterCollapsible(routes: CaptureRoute[]): UnifiedColl
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 | ${cell(route.beforeUrl, `before ${route.path}`)} | ${cell(route.afterUrl, `after ${route.path}`)} | ${cell(route.diffUrl, `diff ${route.path}`)} |`);
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}`)} |`);
}
if (route.beforeUrlMobile || route.afterUrlMobile) {
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)`)} |`);
rows.push(`| ${path} | mobile${themeSuffix} | ${cell(route.beforeUrlMobile, `before ${route.path} (mobile)${themeSuffix}`)} | ${cell(route.afterUrlMobile, `after ${route.path} (mobile)${themeSuffix}`)} | ${cell(route.diffUrlMobile, `diff ${route.path} (mobile)${themeSuffix}`)} |`);
}
}
if (rows.length === 0) return null;
Expand Down
102 changes: 61 additions & 41 deletions src/review/visual/capture.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ import {
getPreviewBuildState,
parseRepo,
} from "./preview-url";
import { captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type Viewport } from "./shot";
import { captureShot, DESKTOP_VIEWPORT, MOBILE_VIEWPORT, type ShotTheme, type Viewport } from "./shot";
import { compareCapturedScreenshots, isVisualDiffAvailable, type VisualDiffOutcome } from "./pixel-diff";

const NAMESPACE = "gittensory";
Expand All @@ -31,9 +31,12 @@ const MAX_ROUTES = 2;

/** 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. */
* 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. */
export interface CaptureRoute {
path: string;
theme?: ShotTheme | undefined;
beforeUrl?: string | undefined;
beforeUrlMobile?: string | undefined;
afterUrl?: string | undefined;
Expand Down Expand Up @@ -153,14 +156,21 @@ async function capturePage(
// 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,
// #3678: emulate prefers-color-scheme before rendering. Undefined (every pre-#3678 caller) ⇒ no emulation
// call and an UNCHANGED cache key — byte-identical to today.
theme?: ShotTheme | undefined,
): 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;
// Carries the theme (#3678) so a LATER on-demand fetch of this exact URL (e.g. a failed/never-persisted
// render retried by GitHub's image proxy) still requests the matching prefers-color-scheme, not the
// default — handleShot's Mode B reads this same &theme= param. Omitted when unset, unchanged from today.
const onDemand = shotBase ? `${shotBase}/${NAMESPACE}/shot?url=${encodeURIComponent(page)}&w=${viewport.width}&h=${viewport.height}${theme ? `&theme=${theme}` : ""}` : page;

if (env.REVIEW_AUDIT) {
// Key includes the viewport so desktop + mobile of the same page don't collide in R2.
const fingerprint = await sha256Hex(`${target.headSha ?? target.prNumber}:${slot}:${viewportName}:${page}`);
// Key includes the viewport (and, when set, the theme) so desktop/mobile and light/dark shots of the
// same page don't collide in R2.
const fingerprint = await sha256Hex(`${target.headSha ?? target.prNumber}:${slot}:${viewportName}:${page}${theme ? `:${theme}` : ""}`);
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);
Expand All @@ -169,7 +179,7 @@ async function capturePage(
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 }));
const { png, authWalled } = await captureShot(env, page, viewport, theme ? { theme } : {}).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.
if (authWalled) {
Expand All @@ -192,19 +202,21 @@ async function uploadDiffImage(
path: string,
viewportName: "desktop" | "mobile",
diff: VisualDiffOutcome | null,
theme?: ShotTheme | undefined,
): Promise<string | undefined> {
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 fingerprint = await sha256Hex(`${target.headSha ?? target.prNumber}:diff:${viewportName}:${path}${theme ? `:${theme}` : ""}`);
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 };
/** 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 };

/**
* Build the before/after capture for a PR: resolve the preview URL, derive routes from the changed UI files,
Expand Down Expand Up @@ -266,38 +278,46 @@ export async function buildCapture(env: Env, token: string, target: CaptureTarge
// 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);
// #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 —
// capturePage/captureShot already treat an undefined theme as "no emulation call at all", so this one
// iteration is byte-identical to every pre-#3678 call.
const themes: readonly (ShotTheme | undefined)[] = visualConfig?.themes && visualConfig.themes.length > 0 ? visualConfig.themes : [undefined];
const captureRoutes: CaptureRoute[] = [];
for (const path of routes) {
const beforePage = prodBase ? joinUrl(prodBase, path) : "";
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, 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,
beforeUrl: beforeShot.url,
beforeUrlMobile: beforeMobileShot.url,
afterUrl: afterShot.url,
afterUrlMobile: afterMobileShot.url,
...(diffUrl ? { diffUrl } : {}),
...(diffUrlMobile ? { diffUrlMobile } : {}),
});
for (const theme of themes) {
for (const path of routes) {
const beforePage = prodBase ? joinUrl(prodBase, path) : "";
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, diffAvailable, theme),
capturePage(env, target, beforePage, "before", "mobile", MOBILE_VIEWPORT, diffAvailable, theme),
afterPage ? capturePage(env, target, afterPage, "after", "desktop", DESKTOP_VIEWPORT, diffAvailable, theme) : Promise.resolve<{ url?: string | undefined; png?: Uint8Array | undefined }>({ url: afterPlaceholder }),
afterPage ? capturePage(env, target, afterPage, "after", "mobile", MOBILE_VIEWPORT, diffAvailable, theme) : 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, theme),
uploadDiffImage(env, target, path, "mobile", mobileDiff, theme),
]);
captureRoutes.push({
path,
...(theme ? { theme } : {}),
beforeUrl: beforeShot.url,
beforeUrlMobile: beforeMobileShot.url,
afterUrl: afterShot.url,
afterUrlMobile: afterMobileShot.url,
...(diffUrl ? { diffUrl } : {}),
...(diffUrlMobile ? { diffUrlMobile } : {}),
});
}
}
return { routes: captureRoutes, previewPending };
}
14 changes: 12 additions & 2 deletions src/review/visual/shot.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,13 @@ import puppeteer from "@cloudflare/puppeteer";
import { isSafeHttpUrl } from "../content-lane/safe-url";

export type Viewport = { width: number; height: number };
/** A `prefers-color-scheme` value the renderer can emulate before capture (#3678). */
export type ShotTheme = "light" | "dark";
export interface CaptureShotOptions {
isAllowedUrl?: (targetUrl: string) => boolean;
/** Emulate `prefers-color-scheme: <theme>` before navigation (#3678). Omitted (every existing caller) ⇒
* no emulation call at all — Chromium's own unconfigured default, byte-identical to today. */
theme?: ShotTheme;
}
type ScreenshotRequest = {
url(): string;
Expand Down Expand Up @@ -149,6 +154,7 @@ export async function captureShot(env: Env, url: string, viewport: Viewport = VI
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_screenshot_redirect_blocked", url, final: page.url().slice(0, 200) }));
Expand Down Expand Up @@ -212,14 +218,18 @@ export async function handleShot(request: Request, env: Env, opts: ShotOptions =
});
}

// Mode B: render on demand (host-allowlisted + SSRF-guarded). Optional &w=&h= selects the viewport.
// Mode B: render on demand (host-allowlisted + SSRF-guarded). Optional &w=&h= selects the viewport;
// optional &theme= (#3678) emulates prefers-color-scheme — an unrecognized value is ignored (falls back to
// no emulation) rather than rejecting the whole request over a cosmetic param.
const target = params.get("url");
if (!target || !isSafeHttpUrl(target)) return new Response("bad url", { status: 400 });
if (!isAllowedHost(target, env, opts.productionUrl)) return new Response("forbidden host", { status: 403 });
const w = Number(params.get("w"));
const h = Number(params.get("h"));
const viewport: Viewport = Number.isFinite(w) && w > 0 && Number.isFinite(h) && h > 0 ? { width: Math.min(w, 2560), height: Math.min(h, 2560) } : DESKTOP_VIEWPORT;
const png = await renderScreenshot(env, target, viewport, { isAllowedUrl: (candidate) => isAllowedHost(candidate, env, opts.productionUrl) });
const requestedTheme = params.get("theme");
const theme: ShotTheme | undefined = requestedTheme === "light" || requestedTheme === "dark" ? requestedTheme : undefined;
const png = await renderScreenshot(env, target, viewport, { isAllowedUrl: (candidate) => isAllowedHost(candidate, env, opts.productionUrl), ...(theme ? { theme } : {}) });
if (!png) return new Response("screenshot unavailable", { status: 502 });
return new Response(png, {
headers: { "content-type": "image/png", "cache-control": "public, max-age=300" },
Expand Down
Loading
Loading