From 8266ff46a8c54c20366df6ced81c8f28ea32e31a Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Sun, 5 Jul 2026 19:23:55 -0700 Subject: [PATCH] feat(review): add review.visual.themes config for dark-mode capture Adds review.visual.themes: string[] (validated against the "light" / "dark" enum, following the same list-parsing idiom as review.visual.routes.paths) so a repo can request before/after evidence for both color schemes instead of only whatever a page defaults to. - shot.ts: captureShot accepts a theme option and calls page.emulateMediaFeatures([{name: "prefers-color-scheme", value}]) before navigation. Undefined (every existing caller) means no emulation call at all. handleShot's on-demand render (?url=) also reads a matching &theme= query param, since an on-demand fallback URL needs to carry the same information a persisted capture's cache key does. - capture.ts: capturePage's cache-key fingerprint and on-demand fallback URL both include the theme (only when set, so the format is byte-identical when it's not); buildCapture resolves the configured theme list (default: a single implicit undefined pass, identical to today) and produces one CaptureRoute per (route, theme) pair, tagging it with `theme` only when explicitly requested. - unified-comment-bridge.ts: the viewport column gets a theme suffix ("desktop (dark)") when a route has one, unlabeled otherwise. Verified with a real headless-Chromium render (browserless) that emulating "light" produces byte-identical output to no emulation at all (confirming the default is unaffected), and that "dark" produces a genuinely different, visually-inverted render via a real prefers-color-scheme media query -- not just a passing mocked test. Part of #3607. Closes #3678. --- .gittensory.yml.example | 5 ++ config/examples/gittensory.full.yml | 5 ++ src/review/unified-comment-bridge.ts | 22 ++--- src/review/visual/capture.ts | 102 ++++++++++++++--------- src/review/visual/shot.ts | 14 +++- src/signals/focus-manifest.ts | 43 ++++++++-- test/unit/focus-manifest.test.ts | 52 +++++++++++- test/unit/signals-coverage.test.ts | 2 +- test/unit/visual-capture.test.ts | 110 +++++++++++++++++++++++++ test/unit/visual-collapsible.test.ts | 42 ++++++++++ test/unit/visual-config-wiring.test.ts | 1 + test/unit/visual-shot.test.ts | 35 ++++++++ 12 files changed, 373 insertions(+), 60 deletions(-) diff --git a/.gittensory.yml.example b/.gittensory.yml.example index 2ae050901a..7c34ba44c6 100644 --- a/.gittensory.yml.example +++ b/.gittensory.yml.example @@ -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. diff --git a/config/examples/gittensory.full.yml b/config/examples/gittensory.full.yml index e899fd5893..3743e257f0 100644 --- a/config/examples/gittensory.full.yml +++ b/config/examples/gittensory.full.yml @@ -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. diff --git a/src/review/unified-comment-bridge.ts b/src/review/unified-comment-bridge.ts index 95559a2229..b91d8f5e5f 100644 --- a/src/review/unified-comment-bridge.ts +++ b/src/review/unified-comment-bridge.ts @@ -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 `` (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, 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. + * (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 `/` 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 => @@ -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; diff --git a/src/review/visual/capture.ts b/src/review/visual/capture.ts index 8bd86a38a1..afe134bb3c 100644 --- a/src/review/visual/capture.ts +++ b/src/review/visual/capture.ts @@ -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"; @@ -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; @@ -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); @@ -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) { @@ -192,19 +202,21 @@ async function uploadDiffImage( path: string, viewportName: "desktop" | "mobile", diff: VisualDiffOutcome | null, + theme?: ShotTheme | undefined, ): 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 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, @@ -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 }; } diff --git a/src/review/visual/shot.ts b/src/review/visual/shot.ts index 0dc538643c..8fa3c29025 100644 --- a/src/review/visual/shot.ts +++ b/src/review/visual/shot.ts @@ -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: ` 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; @@ -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) })); @@ -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" }, diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index aef7af9452..8144754bb4 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -457,8 +457,12 @@ export const EMPTY_SELF_HOST_AI_MODEL_CONFIG: SelfHostAiModelConfig = { export type VisualConfig = { preview: VisualPreviewConfig; routes: VisualRoutesConfig; + themes: VisualTheme[]; }; +/** A `prefers-color-scheme` value the capture pipeline can emulate before rendering (#3678). */ +export type VisualTheme = "light" | "dark"; + export type VisualPreviewConfig = { /** `review.visual.preview.url_template`: the repo's "after" preview URL, with `{number}` (PR number), * `{head_sha}` (full commit SHA), and `{head_sha_short}` (first 7 chars) placeholders substituted at @@ -489,6 +493,7 @@ export type VisualRoutesConfig = { export const EMPTY_VISUAL_CONFIG: VisualConfig = { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, + themes: [], }; /** One `review.path_instructions[]` entry: a manifest path glob + the public-safe instructions to apply when a @@ -1819,7 +1824,31 @@ function parseSelfHostAiModelConfig(value: JsonValue | undefined, warnings: stri } function visualConfigPresent(config: VisualConfig): boolean { - return config.preview.urlTemplate !== null || config.routes.paths.length > 0 || config.routes.maxRoutes !== null; + return config.preview.urlTemplate !== null || config.routes.paths.length > 0 || config.routes.maxRoutes !== null || config.themes.length > 0; +} + +const VISUAL_THEME_VALUES: readonly VisualTheme[] = ["light", "dark"]; + +/** Parse `review.visual.themes` — which `prefers-color-scheme` variants to capture (#3678). Empty/default ⇒ + * the capture pipeline falls back to a single light-theme render, byte-identical to today. Unlike + * `routes.paths` (an open-ended glob list), this is a closed 2-value enum, so entries are validated against + * it directly rather than reusing the generic glob-list parser. */ +function parseVisualThemes(value: JsonValue | undefined, warnings: string[]): VisualTheme[] { + if (value === undefined || value === null) return []; + if (!Array.isArray(value)) { + warnings.push(`Manifest "review.visual.themes" must be a list of "light"/"dark"; ignoring it.`); + return []; + } + const out: VisualTheme[] = []; + for (const [index, entry] of value.entries()) { + const theme = typeof entry === "string" ? (entry.trim().toLowerCase() as VisualTheme) : undefined; + if (!theme || !VISUAL_THEME_VALUES.includes(theme)) { + warnings.push(`Manifest "review.visual.themes[${index}]" must be "light" or "dark"; ignoring it.`); + continue; + } + if (!out.includes(theme)) out.push(theme); + } + return out; } // `{number}`/`{head_sha}`/`{head_sha_short}` are GitHub-controlled facts about the PR (never attacker-supplied @@ -1851,12 +1880,13 @@ function parseVisualUrlTemplate(value: JsonValue | undefined, warnings: string[] return template; } -/** Parse `review.visual` — per-repo before/after screenshot-capture config (#3609 preview / #3610 routes). */ +/** Parse `review.visual` — per-repo before/after screenshot-capture config (#3609 preview / #3610 routes / + * #3678 themes). */ function parseVisualConfig(value: JsonValue | undefined, warnings: string[]): VisualConfig { - if (value === undefined || value === null) return { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } }; + if (value === undefined || value === null) return { ...EMPTY_VISUAL_CONFIG }; if (typeof value !== "object" || Array.isArray(value)) { warnings.push(`Manifest field "review.visual" must be a mapping; ignoring it.`); - return { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } }; + return { ...EMPTY_VISUAL_CONFIG }; } const record = value as Record; @@ -1873,7 +1903,9 @@ function parseVisualConfig(value: JsonValue | undefined, warnings: string[]): Vi const paths = routesRecord ? parseManifestGlobList(routesRecord.paths, "review.visual.routes.paths", warnings) : []; const maxRoutes = routesRecord ? normalizeOptionalPositiveInteger(routesRecord.max_routes, "review.visual.routes.max_routes", warnings) : null; - return { preview: { urlTemplate }, routes: { paths, maxRoutes } }; + const themes = parseVisualThemes(record.themes, warnings); + + return { preview: { urlTemplate }, routes: { paths, maxRoutes }, themes }; } function parseAutoReviewTitleKeywords(value: JsonValue | undefined, warnings: string[]): string[] { @@ -2135,6 +2167,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue if (review.visual.routes.maxRoutes !== null) routes.max_routes = review.visual.routes.maxRoutes; visual.routes = routes; } + if (review.visual.themes.length > 0) visual.themes = [...review.visual.themes]; out.visual = visual; } if (review.linkedIssueSatisfaction !== null) out.linkedIssueSatisfaction = review.linkedIssueSatisfaction; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index f1c99b1678..db3dfae653 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -3270,6 +3270,7 @@ describe("review.visual (#3609 preview.url_template / #3610 routes)", () => { expect(m.review.visual).toEqual({ preview: { urlTemplate: "https://pr-{number}.preview.example.com" }, routes: { paths: ["/pricing", "/docs"], maxRoutes: 3 }, + themes: [], }); expect(m.review.present).toBe(true); expect(parseFocusManifest({ review: reviewConfigToJson(m.review) }).review.visual).toEqual(m.review.visual); @@ -3358,7 +3359,56 @@ describe("review.visual (#3609 preview.url_template / #3610 routes)", () => { it("resolveReviewVisualConfig: null manifest yields empty defaults; a set manifest passes through", () => { expect(resolveReviewVisualConfig(null)).toEqual({ ...EMPTY_VISUAL_CONFIG }); const manifest = parseFocusManifest({ review: { visual: { routes: { paths: ["/app"] } } } }); - expect(resolveReviewVisualConfig(manifest)).toEqual({ preview: { urlTemplate: null }, routes: { paths: ["/app"], maxRoutes: null } }); + expect(resolveReviewVisualConfig(manifest)).toEqual({ preview: { urlTemplate: null }, routes: { paths: ["/app"], maxRoutes: null }, themes: [] }); + }); +}); + +describe("review.visual.themes (#3678 dark-mode capture)", () => { + it("parses a light+dark list, marks present, and round-trips", () => { + const m = parseFocusManifest({ review: { visual: { themes: ["light", "dark"] } } }); + expect(m.review.visual.themes).toEqual(["light", "dark"]); + expect(m.review.present).toBe(true); + expect(reviewConfigToJson(m.review)).toEqual({ visual: { themes: ["light", "dark"] } }); + }); + + it("absent/empty themes yields [] and does not mark review present on its own", () => { + expect(parseFocusManifest({}).review.visual.themes).toEqual([]); + expect(parseFocusManifest({ review: { visual: { themes: [] } } }).review.visual.themes).toEqual([]); + expect(parseFocusManifest({ review: { visual: {} } }).review.present).toBe(false); + }); + + it("lowercases + dedupes entries, preserving first-seen order", () => { + const m = parseFocusManifest({ review: { visual: { themes: ["DARK", "light", "dark", "Light"] } } }); + expect(m.review.visual.themes).toEqual(["dark", "light"]); + }); + + it("drops an unrecognized theme value with a warning but keeps the valid ones", () => { + const bad = parseFocusManifest({ review: { visual: { themes: ["light", "sepia", "dark"] } } }); + expect(bad.review.visual.themes).toEqual(["light", "dark"]); + expect(bad.warnings.some((w) => /review\.visual\.themes\[1\].*"light" or "dark"/.test(w))).toBe(true); + }); + + it("warns and drops the whole list when it's not an array", () => { + const bad = parseFocusManifest({ review: { visual: { themes: "dark" } } }); + expect(bad.review.visual.themes).toEqual([]); + expect(bad.warnings.some((w) => /review\.visual\.themes.*must be a list/.test(w))).toBe(true); + }); + + it("drops a non-string entry with a warning naming its index", () => { + const bad = parseFocusManifest({ review: { visual: { themes: ["light", 42, "dark"] } } }); + expect(bad.review.visual.themes).toEqual(["light", "dark"]); + expect(bad.warnings.some((w) => /review\.visual\.themes\[1\]/.test(w))).toBe(true); + }); + + it("marks present via themes alone (preview + routes both empty)", () => { + const m = parseFocusManifest({ review: { visual: { themes: ["dark"] } } }); + expect(m.review.present).toBe(true); + expect(reviewConfigToJson(m.review)).toEqual({ visual: { themes: ["dark"] } }); + }); + + it("resolveReviewVisualConfig passes a configured theme list through", () => { + const manifest = parseFocusManifest({ review: { visual: { themes: ["dark"] } } }); + expect(resolveReviewVisualConfig(manifest).themes).toEqual(["dark"]); }); }); diff --git a/test/unit/signals-coverage.test.ts b/test/unit/signals-coverage.test.ts index 316d594511..3084f004c1 100644 --- a/test/unit/signals-coverage.test.ts +++ b/test/unit/signals-coverage.test.ts @@ -1127,7 +1127,7 @@ describe("signal coverage edge cases", () => { collisions: buildCollisionReport(directRepo.fullName, [], [currentPr]), preflight: buildPreflightResult({ repoFullName: directRepo.fullName, title: "Fix isolated issue", body: "Fixes #99", linkedIssues: [99] }, directRepo, [], [currentPr]), settings: gateSettings, - review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null } }, linkedIssueSatisfaction: null }, + review: { present: true, footerText: "Reviewed by the Acme maintainer bot.", note: "Run npm test before pushing.", fields: { relatedWork: false }, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, suggestions: null, changedFilesSummary: null, effortScore: null, findingCategories: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { skipDrafts: null, ignoreAuthors: [], ignoreTitleKeywords: [], baseBranches: [], autoPauseAfterReviewedCommits: null }, labelingRules: [], aiModel: { claudeModel: null, claudeEffort: null, codexModel: null, codexEffort: null }, visual: { preview: { urlTemplate: null }, routes: { paths: [], maxRoutes: null }, themes: [] }, linkedIssueSatisfaction: null }, aiReview: { notes: "The change is focused.\n\n**Nits (2)**\n- Add a test for the edge case.\n- Keep the validator helper scoped." }, }); expect(customizedComment).toContain("Reviewed by the Acme maintainer bot."); // custom footer lead diff --git a/test/unit/visual-capture.test.ts b/test/unit/visual-capture.test.ts index 2ad61b49cc..5518d50d03 100644 --- a/test/unit/visual-capture.test.ts +++ b/test/unit/visual-capture.test.ts @@ -616,3 +616,113 @@ describe("buildCapture pixel-diff wiring (#3674)", () => { } }); }); + +describe("buildCapture theme matrix (#3678)", () => { + it("produces exactly one untagged route per path when no themes are configured — byte-identical to pre-#3678", async () => { + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 20, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(result.routes).toHaveLength(1); + expect(result.routes[0]?.theme).toBeUndefined(); + }); + + it("produces one tagged route per (path, theme) pair when themes are configured, with distinct shot URLs per theme", async () => { + 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: 21, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { themes: ["light", "dark"] }, + ); + expect(result.routes).toHaveLength(2); + expect(result.routes.map((r) => r.theme)).toEqual(["light", "dark"]); + expect(result.routes[0]?.path).toBe(result.routes[1]?.path); + // Different themes must never collide on the same cache key/URL. + expect(result.routes[0]?.beforeUrl).not.toBe(result.routes[1]?.beforeUrl); + }); + + it("tags the single route with its theme even when only one theme is explicitly configured", async () => { + const result = await buildCapture( + createTestEnv({ PUBLIC_API_ORIGIN: "https://worker.example", PUBLIC_SITE_ORIGIN: "https://prod.example.com" }), + "installation-token", + { repoFullName: "owner/repo", prNumber: 22, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { themes: ["dark"] }, + ); + expect(result.routes).toHaveLength(1); + expect(result.routes[0]?.theme).toBe("dark"); + }); + + it("passes the configured theme through to captureShot's render options", async () => { + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: null, 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: 23, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { themes: ["dark"] }, + ); + expect(captureShotSpy).toHaveBeenCalled(); + const themedCall = captureShotSpy.mock.calls.find(([, , , opts]) => opts?.theme === "dark"); + expect(themedCall).toBeDefined(); + } finally { + captureShotSpy.mockRestore(); + } + }); + + it("never passes a theme option to captureShot when no themes are configured", async () => { + const captureShotSpy = vi.spyOn(shotModule, "captureShot").mockResolvedValue({ png: null, 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: 24, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + ); + expect(captureShotSpy).toHaveBeenCalled(); + expect(captureShotSpy.mock.calls.every(([, , , opts]) => !opts?.theme)).toBe(true); + } finally { + captureShotSpy.mockRestore(); + } + }); + + it("threads the theme into the diff-image fingerprint too, so a themed and untagged diff never collide", 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: 25, previewUrl: "https://preview.example.com" }, + ["apps/gittensory-ui/src/routes/app.index.tsx"], + undefined, + { themes: ["dark"] }, + ); + expect(result.routes[0]?.theme).toBe("dark"); + expect(result.routes[0]?.diffUrl).toContain("/gittensory/shot?key="); + // Same path/PR, but tagged "dark" — must not reuse the untagged diff's fingerprint (theme is part of the key). + const untaggedFingerprint = await sha256Hex(`25:diff:desktop:/app`); + expect(result.routes[0]?.diffUrl).not.toContain(untaggedFingerprint.slice(0, 40)); + } finally { + availableSpy.mockRestore(); + compareSpy.mockRestore(); + } + }); +}); diff --git a/test/unit/visual-collapsible.test.ts b/test/unit/visual-collapsible.test.ts index 7ab482c0d5..affd4cfd6b 100644 --- a/test/unit/visual-collapsible.test.ts +++ b/test/unit/visual-collapsible.test.ts @@ -107,6 +107,48 @@ describe("buildBeforeAfterCollapsible", () => { expect(c?.body).toContain(' { + const c = buildBeforeAfterCollapsible([ + { + path: "/app/analytics", + theme: "dark", + beforeUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/abc.png", + afterUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/def.png", + }, + ]); + expect(c?.body).toContain("| `/app/analytics` | desktop (dark) |"); + expect(c?.body).toContain('alt="before /app/analytics (dark)"'); + expect(c?.body).toContain('alt="after /app/analytics (dark)"'); + }); + + it("leaves the viewport column unlabeled when a route has no theme — byte-identical to pre-#3678", () => { + const c = buildBeforeAfterCollapsible(routes); + expect(c?.body).toContain("| `/app/analytics` | desktop |"); + expect(c?.body).not.toContain("desktop ("); + }); + + it("combines the theme and mobile labels on the mobile row", () => { + const c = buildBeforeAfterCollapsible([ + { + path: "/app/analytics", + theme: "dark", + 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", + }, + ]); + expect(c?.body).toContain("| `/app/analytics` | mobile (dark) |"); + expect(c?.body).toContain('alt="before /app/analytics (mobile) (dark)"'); + }); + + it("renders one row set per theme when the same route appears twice with different themes", () => { + const c = buildBeforeAfterCollapsible([ + { path: "/", theme: "light", beforeUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/light.png" }, + { path: "/", theme: "dark", beforeUrl: "https://api.example.dev/gittensory/shot?key=gittensory/shots/dark.png" }, + ]); + expect(c?.body).toContain("| `/` | desktop (light) |"); + expect(c?.body).toContain("| `/` | desktop (dark) |"); + }); }); describe("buildUnifiedCommentBody beforeAfter wiring", () => { diff --git a/test/unit/visual-config-wiring.test.ts b/test/unit/visual-config-wiring.test.ts index b2b11ea223..b381dc2dde 100644 --- a/test/unit/visual-config-wiring.test.ts +++ b/test/unit/visual-config-wiring.test.ts @@ -18,6 +18,7 @@ describe("review.visual wiring (#3609 / #3610)", () => { await expect(resolveVisualCaptureConfig({} as Env, "acme/widgets")).resolves.toEqual({ preview: { urlTemplate: "https://pr-{number}.preview.example.com" }, routes: { paths: ["/pricing"], maxRoutes: 3 }, + themes: [], }); expect(loadSpy).toHaveBeenCalledWith(expect.anything(), "acme/widgets"); loadSpy.mockRestore(); diff --git a/test/unit/visual-shot.test.ts b/test/unit/visual-shot.test.ts index f6381f7742..df7345e3ab 100644 --- a/test/unit/visual-shot.test.ts +++ b/test/unit/visual-shot.test.ts @@ -8,6 +8,7 @@ const mocks = vi.hoisted(() => ({ continue: vi.fn(async () => undefined), close: vi.fn(async () => undefined), launch: vi.fn(), + emulateMediaFeatures: vi.fn(async () => undefined), })); vi.mock("@cloudflare/puppeteer", () => ({ @@ -60,6 +61,7 @@ describe("visual screenshot on-demand SSRF guard", () => { if (event === "request") onRequest = callback; }), setViewport: vi.fn(async () => undefined), + emulateMediaFeatures: mocks.emulateMediaFeatures, goto: vi.fn(async (url: string) => { onRequest?.(makeRequest(url)); if (mocks.finalUrl !== url) onRequest?.(makeRequest(mocks.finalUrl)); @@ -120,6 +122,39 @@ describe("visual screenshot on-demand SSRF guard", () => { expect(mocks.screenshot).toHaveBeenCalledWith({ type: "png", fullPage: true }); }); + it("never emulates a color scheme when no theme is requested — every existing caller, byte-identical to today", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + await captureShot(env(), "https://preview.pages.dev/page"); + expect(mocks.emulateMediaFeatures).not.toHaveBeenCalled(); + }); + + it("emulates prefers-color-scheme when a theme is requested (#3678)", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + await captureShot(env(), "https://preview.pages.dev/page", undefined, { theme: "dark" }); + expect(mocks.emulateMediaFeatures).toHaveBeenCalledWith([{ name: "prefers-color-scheme", value: "dark" }]); + }); + + it("handleShot's on-demand render reads &theme= and emulates it (#3678)", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + const response = await handleShot(shotRequest(`url=${encodeURIComponent("https://preview.pages.dev/page")}&theme=dark`), env()); + expect(response.status).toBe(200); + expect(mocks.emulateMediaFeatures).toHaveBeenCalledWith([{ name: "prefers-color-scheme", value: "dark" }]); + }); + + it("handleShot ignores an unrecognized &theme= value instead of rejecting the request", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + const response = await handleShot(shotRequest(`url=${encodeURIComponent("https://preview.pages.dev/page")}&theme=sepia`), env()); + expect(response.status).toBe(200); + expect(mocks.emulateMediaFeatures).not.toHaveBeenCalled(); + }); + + it("handleShot never emulates a color scheme when &theme= is absent — byte-identical to pre-#3678", async () => { + mocks.finalUrl = "https://preview.pages.dev/page"; + const response = await handleShot(request("https://preview.pages.dev/page"), env()); + expect(response.status).toBe(200); + expect(mocks.emulateMediaFeatures).not.toHaveBeenCalled(); + }); + it("captureShot rejects an unsafe target before launching the browser (defense-in-depth)", async () => { const result = await captureShot(env(), "http://127.0.0.1/admin"); expect(result).toEqual({ png: null, authWalled: false });