From 82f5a352953906fa2faad9e50f4335635480d41b Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 17:45:36 -0700 Subject: [PATCH 1/3] feat(review): viewport x theme completeness matrix for the screenshot-table gate (#4540) The deterministic screenshot-table gate only checked that a PR body contained some markdown table with any image inside it, with no concept of viewport or color-mode coverage -- even though metagraphed's own contributor skill file documents an exact 3-viewport x 2-theme x before/after = 12-image contract. metagraphed PR #4661 shipped 4/12 images and passed AI review, which had no way to know the completeness requirement existed. screenshotTableGate gains requireViewports/requireThemes (string arrays, empty by default -- opt-in per repo, byte-identical otherwise). When set, the evaluator matches each required (viewport, theme) pair against a labeled table row and requires two image-bearing cells (before + after) in that row; missing pairs are named in the rejection reason. action gains a real advisory value, distinct from the request_changes/comment values #4110 removed as dead/unwired config -- this one is actually wired: an advisory violation is computed but never reaches the close-triggering planner match. Full config-as-code wiring: DB migration + Drizzle schema + settings resolver + .gittensory.yml manifest parser (both the Worker copy and the hand-duplicated gittensory-engine package copy) + OpenAPI. Closes #4540 --- apps/gittensory-ui/public/openapi.json | 19 +- .../0130_screenshot_table_gate_matrix.sql | 7 + .../gittensory-engine/src/focus-manifest.ts | 2 + .../src/review/screenshot-table-gate.ts | 126 +++++++++- .../src/types/manifest-deps-types.ts | 10 +- src/db/repositories.ts | 8 +- src/db/schema.ts | 5 + src/openapi/schemas.ts | 4 +- src/review/screenshot-table-gate.ts | 126 +++++++++- src/settings/agent-actions.ts | 5 +- src/signals/focus-manifest.ts | 2 + src/types.ts | 19 +- test/unit/focus-manifest.test.ts | 26 +- ...ory-settings-screenshot-table-gate.test.ts | 12 +- .../unit/screenshot-table-gate-engine.test.ts | 235 +++++++++++++++++- test/unit/screenshot-table-gate.test.ts | 235 +++++++++++++++++- 16 files changed, 804 insertions(+), 37 deletions(-) create mode 100644 migrations/0130_screenshot_table_gate_matrix.sql diff --git a/apps/gittensory-ui/public/openapi.json b/apps/gittensory-ui/public/openapi.json index c5e537058b..0b04a70ed9 100644 --- a/apps/gittensory-ui/public/openapi.json +++ b/apps/gittensory-ui/public/openapi.json @@ -9480,18 +9480,33 @@ "action": { "type": "string", "enum": [ - "close" + "close", + "advisory" ] }, "message": { "type": "string" + }, + "requireViewports": { + "type": "array", + "items": { + "type": "string" + } + }, + "requireThemes": { + "type": "array", + "items": { + "type": "string" + } } }, "required": [ "enabled", "whenLabels", "whenPaths", - "action" + "action", + "requireViewports", + "requireThemes" ] }, "createdAt": { diff --git a/migrations/0130_screenshot_table_gate_matrix.sql b/migrations/0130_screenshot_table_gate_matrix.sql new file mode 100644 index 0000000000..0034080a1e --- /dev/null +++ b/migrations/0130_screenshot_table_gate_matrix.sql @@ -0,0 +1,7 @@ +-- Viewport x theme completeness matrix for the screenshot-table gate (#4535, #4540). Empty (default) JSON +-- arrays keep the original presence-only check byte-identical for every repo that hasn't opted in; a +-- non-empty screenshot_table_gate_require_viewports_json switches the evaluator into matrix mode, requiring a +-- labeled before/after row per configured viewport (x theme, when the themes column is also set). Mirrors the +-- existing when_labels_json / when_paths_json JSON-array-column shape from #2006 (migration 0117). +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_require_viewports_json TEXT NOT NULL DEFAULT '[]'; +ALTER TABLE repository_settings ADD COLUMN screenshot_table_gate_require_themes_json TEXT NOT NULL DEFAULT '[]'; diff --git a/packages/gittensory-engine/src/focus-manifest.ts b/packages/gittensory-engine/src/focus-manifest.ts index a2c2ffb83a..fbfca523a9 100644 --- a/packages/gittensory-engine/src/focus-manifest.ts +++ b/packages/gittensory-engine/src/focus-manifest.ts @@ -1881,6 +1881,8 @@ function parseSettingsOverride(value: JsonValue | undefined, warnings: string[], if (Array.isArray(rawGate.whenPaths)) sparseGate.whenPaths = validated.whenPaths; if (isScreenshotTableGateAction(rawGate.action)) sparseGate.action = validated.action; if (typeof rawGate.message === "string" && rawGate.message.trim().length > 0) sparseGate.message = validated.message; + if (Array.isArray(rawGate.requireViewports)) sparseGate.requireViewports = validated.requireViewports; + if (Array.isArray(rawGate.requireThemes)) sparseGate.requireThemes = validated.requireThemes; out.screenshotTableGate = sparseGate; } else if (r.screenshotTableGate !== undefined) { warnings.push(`Manifest "settings.screenshotTableGate" must be an object; ignoring it and keeping any existing policy.`); diff --git a/packages/gittensory-engine/src/review/screenshot-table-gate.ts b/packages/gittensory-engine/src/review/screenshot-table-gate.ts index 66eb033b4b..0edf7fc054 100644 --- a/packages/gittensory-engine/src/review/screenshot-table-gate.ts +++ b/packages/gittensory-engine/src/review/screenshot-table-gate.ts @@ -14,6 +14,8 @@ const MAX_LABELS = 50; const MAX_PATHS = 50; const MAX_LABEL_CHARS = 100; const MAX_PATH_CHARS = 300; +const MAX_MATRIX_DIMENSION = 12; +const MAX_MATRIX_TOKEN_CHARS = 40; // Extensions treated as "an image file" for the committed-image-file check below. Deliberately excludes SVG: // an SVG can embed script/foreign-object content, so it is never accepted as review evidence anywhere in this @@ -26,9 +28,11 @@ export const DEFAULT_SCREENSHOT_TABLE_GATE: ScreenshotTableGateConfig = { whenLabels: [], whenPaths: [], action: "close", + requireViewports: [], + requireThemes: [], }; -const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close"]; +const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "advisory"]; export function isScreenshotTableGateAction(value: unknown): value is ScreenshotTableGateAction { return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value); @@ -59,10 +63,10 @@ function normalizeStringList(value: unknown, field: string, max: number, maxChar * throws: malformed fields fall back to the default (disabled/empty), matching every other settings normalizer * in this codebase. */ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: string[]): ScreenshotTableGateConfig { - if (input === undefined || input === null) return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }; + if (input === undefined || input === null) return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] }; if (typeof input !== "object" || Array.isArray(input)) { warnings.push("settings.requireScreenshotTable must be an object; using the default (disabled)."); - return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }; + return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] }; } const record = input as Record; const enabled = typeof record.enabled === "boolean" ? record.enabled : DEFAULT_SCREENSHOT_TABLE_GATE.enabled; @@ -72,7 +76,7 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str const action = isScreenshotTableGateAction(record.action) ? record.action : (() => { - if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" (the only supported value; #4110 removed request_changes/comment as dead config surface); using the default "close".`); + if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" or "advisory" (#4110 removed request_changes/comment as dead config surface); using the default "close".`); return DEFAULT_SCREENSHOT_TABLE_GATE.action; })(); const message = typeof record.message === "string" && record.message.trim().length > 0 ? record.message.trim() : undefined; @@ -84,6 +88,8 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str whenLabels: normalizeStringList(record.whenLabels, "whenLabels", MAX_LABELS, MAX_LABEL_CHARS, warnings), whenPaths: normalizeStringList(record.whenPaths, "whenPaths", MAX_PATHS, MAX_PATH_CHARS, warnings), action, + requireViewports: normalizeStringList(record.requireViewports, "requireViewports", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), + requireThemes: normalizeStringList(record.requireThemes, "requireThemes", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), ...(message !== undefined ? { message } : {}), }; } @@ -156,6 +162,96 @@ export function hasCommittedImageFile(changedFiles: string[], scopedPaths: strin }); } +const IMAGE_CELL_PATTERN = /!\[[^\]]*\]\([^)]+\)|]*>/i; + +/** One data row of a detected markdown table: the cell texts in source order (leading/trailing pipes and + * whitespace stripped). Deliberately a SEPARATE table-detection pass from {@link hasImageBearingMarkdownTable} + * rather than a shared refactor of it -- that function's exact behavior is pinned by existing tests, and this + * one needs actual cell contents (not just "does some cell have an image"), so duplicating its short + * header+separator detection loop keeps both independently simple instead of risking a regression in either + * from a shared-code change. */ +function extractTableRows(body: string | null | undefined): string[][] { + if (!body) return []; + const lines = body.split(/\r?\n/); + const tableRowPattern = /^\s*\|.*\|\s*$/; + const rows: string[][] = []; + for (let i = 0; i < lines.length - 1; i += 1) { + /* v8 ignore next -- defensive: the loop bound above guarantees lines[i] always exists here. */ + const header = lines[i] ?? ""; + /* v8 ignore next -- defensive: the loop bound above guarantees lines[i + 1] always exists here. */ + const separator = lines[i + 1] ?? ""; + if (!tableRowPattern.test(header) || !isMarkdownTableSeparatorRow(separator)) continue; + let j = i + 2; + /* v8 ignore next -- defensive: the `j < lines.length` guard above guarantees lines[j] always exists here. */ + while (j < lines.length && tableRowPattern.test(lines[j] ?? "")) { + /* v8 ignore next -- defensive: same loop-bound guarantee as above. */ + const line = lines[j] ?? ""; + const cells = line + .trim() + .replace(/^\|/, "") + .replace(/\|$/, "") + .split("|") + .map((cell) => cell.trim()); + rows.push(cells); + j += 1; + } + } + return rows; +} + +/** One (viewport, theme) combination the matrix must cover. `theme: null` means the theme dimension isn't + * required at all (a repo can require viewport coverage without color-mode coverage). */ +export type ScreenshotMatrixPair = { viewport: string; theme: string | null }; + +/** The full set of (viewport, theme) pairs `config` requires, or `[]` when matrix mode is off. Matrix mode + * turns on via `requireViewports` alone -- `requireThemes` with an empty `requireViewports` has no effect, + * since there is no viewport to cross it against. */ +export function requiredScreenshotMatrixPairs(config: ScreenshotTableGateConfig): ScreenshotMatrixPair[] { + if (config.requireViewports.length === 0) return []; + if (config.requireThemes.length === 0) return config.requireViewports.map((viewport) => ({ viewport, theme: null })); + const pairs: ScreenshotMatrixPair[] = []; + for (const viewport of config.requireViewports) { + for (const theme of config.requireThemes) pairs.push({ viewport, theme }); + } + return pairs; +} + +/** True when some row's first cell (the row LABEL, e.g. "Desktop · Light") mentions both `pair.viewport` and + * `pair.theme` (case-insensitive substring match -- tolerant of whatever separator character the contributor + * used between them) AND that row has at least two image-bearing cells among the rest (before + after). */ +function rowSatisfiesMatrixPair(row: string[], pair: ScreenshotMatrixPair): boolean { + const label = (row[0] ?? "").toLowerCase(); + if (!label.includes(pair.viewport.toLowerCase())) return false; + if (pair.theme !== null && !label.includes(pair.theme.toLowerCase())) return false; + const imageCells = row.slice(1).filter((cell) => IMAGE_CELL_PATTERN.test(cell)).length; + return imageCells >= 2; +} + +/** The subset of `pairs` with NO satisfying row anywhere in `body`'s tables. Empty ⇒ full coverage. */ +export function missingScreenshotMatrixPairs(body: string | null | undefined, pairs: ScreenshotMatrixPair[]): ScreenshotMatrixPair[] { + if (pairs.length === 0) return []; + const rows = extractTableRows(body); + return pairs.filter((pair) => !rows.some((row) => rowSatisfiesMatrixPair(row, pair))); +} + +function formatMatrixPair(pair: ScreenshotMatrixPair): string { + return pair.theme === null ? pair.viewport : `${pair.viewport} · ${pair.theme}`; +} + +/** Build the rejection reason for a matrix violation, naming exactly which viewport/theme combinations are + * still missing a real before+after pair -- so the contributor knows precisely what to add, not just that + * "something" is missing. */ +export function buildScreenshotMatrixMessage(missing: ScreenshotMatrixPair[]): string { + const list = missing.map(formatMatrixPair).join(", "); + const dimensionLabel = missing.some((pair) => pair.theme !== null) ? "viewport × theme" : "viewport"; + return ( + "This pull request changes UI/visual code but its screenshot evidence is incomplete. Every required " + + `${dimensionLabel} combination needs its own before/after image pair in a labeled table row (e.g. ` + + '"Desktop · Light | before | after"). Still missing: ' + + `${list}.\n\nPlease resubmit with the remaining rows filled in.` + ); +} + /** True when the PR is IN SCOPE for the gate: it carries one of `config.whenLabels` OR touches a path matching * one of `config.whenPaths`. Both empty ⇒ every PR is in scope (an operator who enables the gate with no * scoping at all wants it enforced everywhere). Only one non-empty list configured ⇒ that list alone decides @@ -182,10 +278,16 @@ export type ScreenshotTableGateResult = { const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null }; -/** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. In - * scope AND (no image-bearing table in the body OR an image pasted outside a table OR a committed image file - * under a scoped path), UNLESS `botCaptureSatisfied` ⇒ violated, with the configured (or default) templated - * message as the reason. */ +/** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. + * `botCaptureSatisfied` ⇒ no violation regardless of mode (an automated capture is equivalent to a + * hand-authored table, and the bot doesn't (yet) shoot a full viewport/theme matrix -- see #4535's scope note). + * + * Two modes, chosen by whether `config.requireViewports` is non-empty (#4535): + * - MATRIX mode: every required (viewport, theme) pair (`requiredScreenshotMatrixPairs`) must have a labeled + * before/after row. Violated ⇒ the reason names exactly which pairs are still missing. + * - PRESENCE mode (the original #2006 behavior, unchanged): in scope AND (no image-bearing table in the body + * OR an image pasted outside a table OR a committed image file under a scoped path) ⇒ violated, with the + * configured (or default) templated message as the reason. */ export function evaluateScreenshotTableGate(input: { config: ScreenshotTableGateConfig; prBody: string | null | undefined; @@ -203,6 +305,14 @@ export function evaluateScreenshotTableGate(input: { if (!config.enabled) return NO_VIOLATION; if (!isScreenshotTableGateInScope(config, input.prLabels, input.changedFiles)) return NO_VIOLATION; if (input.botCaptureSatisfied === true) return NO_VIOLATION; + + const matrixPairs = requiredScreenshotMatrixPairs(config); + if (matrixPairs.length > 0) { + const missing = missingScreenshotMatrixPairs(input.prBody, matrixPairs); + if (missing.length === 0) return NO_VIOLATION; + return { violated: true, reason: config.message ?? buildScreenshotMatrixMessage(missing) }; + } + const hasTable = hasImageBearingMarkdownTable(input.prBody); const outsideTable = hasImageOutsideTable(input.prBody); const committedImage = hasCommittedImageFile(input.changedFiles, config.whenPaths); diff --git a/packages/gittensory-engine/src/types/manifest-deps-types.ts b/packages/gittensory-engine/src/types/manifest-deps-types.ts index 292d5a4771..f0db2655ac 100644 --- a/packages/gittensory-engine/src/types/manifest-deps-types.ts +++ b/packages/gittensory-engine/src/types/manifest-deps-types.ts @@ -18,9 +18,9 @@ export type CombineStrategy = "single" | "consensus" | "synthesis"; export type OnMerge = "either" | "both"; -// #4110: `request_changes`/`comment` were REMOVED (see src/types.ts's mirror of this type for why) -- `"close"` -// is the only value this gate has ever enforced. -export type ScreenshotTableGateAction = "close"; +// #4110: `request_changes`/`comment` were REMOVED (see src/types.ts's mirror of this type for why). +// `"advisory"` (#4535) is a NEW, actually-wired value -- see src/types.ts's mirror for the full rationale. +export type ScreenshotTableGateAction = "close" | "advisory"; export type ScreenshotTableGateConfig = { enabled: boolean; @@ -28,6 +28,10 @@ export type ScreenshotTableGateConfig = { whenPaths: string[]; action: ScreenshotTableGateAction; message?: string | undefined; + // Viewport x theme completeness matrix (#4535) -- see src/types.ts's mirror of this type for the full + // rationale. + requireViewports: string[]; + requireThemes: string[]; }; export type CommandAuthorizationRole = "maintainer" | "collaborator" | "pr_author" | "confirmed_miner"; diff --git a/src/db/repositories.ts b/src/db/repositories.ts index 74440dcad6..c8957c5ad5 100644 --- a/src/db/repositories.ts +++ b/src/db/repositories.ts @@ -570,7 +570,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise reviewEvasionLabel: DEFAULT_REVIEW_EVASION_LABEL, reviewEvasionComment: true, mergeTrainMode: "off", - screenshotTableGate: { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }, + screenshotTableGate: { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] }, }; } return { @@ -857,6 +857,8 @@ export async function upsertRepositorySettings(env: Env, settings: Partial nowIso()), updatedAt: text("updated_at").notNull().$defaultFn(() => nowIso()), }); diff --git a/src/openapi/schemas.ts b/src/openapi/schemas.ts index 7dca0848e1..65c86d0b00 100644 --- a/src/openapi/schemas.ts +++ b/src/openapi/schemas.ts @@ -788,7 +788,9 @@ export const RepositorySettingsSchema = z enabled: z.boolean(), whenLabels: z.array(z.string()), whenPaths: z.array(z.string()), - action: z.enum(["close"]), + action: z.enum(["close", "advisory"]), + requireViewports: z.array(z.string()), + requireThemes: z.array(z.string()), message: z.string().optional(), }) .optional(), diff --git a/src/review/screenshot-table-gate.ts b/src/review/screenshot-table-gate.ts index 0c1c21c26a..415588bcfd 100644 --- a/src/review/screenshot-table-gate.ts +++ b/src/review/screenshot-table-gate.ts @@ -14,6 +14,8 @@ const MAX_LABELS = 50; const MAX_PATHS = 50; const MAX_LABEL_CHARS = 100; const MAX_PATH_CHARS = 300; +const MAX_MATRIX_DIMENSION = 12; +const MAX_MATRIX_TOKEN_CHARS = 40; // Extensions treated as "an image file" for the committed-image-file check below. Deliberately excludes SVG: // an SVG can embed script/foreign-object content, so it is never accepted as review evidence anywhere in this @@ -26,9 +28,11 @@ export const DEFAULT_SCREENSHOT_TABLE_GATE: ScreenshotTableGateConfig = { whenLabels: [], whenPaths: [], action: "close", + requireViewports: [], + requireThemes: [], }; -const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close"]; +const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "advisory"]; export function isScreenshotTableGateAction(value: unknown): value is ScreenshotTableGateAction { return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value); @@ -59,10 +63,10 @@ function normalizeStringList(value: unknown, field: string, max: number, maxChar * throws: malformed fields fall back to the default (disabled/empty), matching every other settings normalizer * in this codebase. */ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: string[]): ScreenshotTableGateConfig { - if (input === undefined || input === null) return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }; + if (input === undefined || input === null) return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] }; if (typeof input !== "object" || Array.isArray(input)) { warnings.push("settings.requireScreenshotTable must be an object; using the default (disabled)."); - return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [] }; + return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] }; } const record = input as Record; const enabled = typeof record.enabled === "boolean" ? record.enabled : DEFAULT_SCREENSHOT_TABLE_GATE.enabled; @@ -72,7 +76,7 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str const action = isScreenshotTableGateAction(record.action) ? record.action : (() => { - if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" (the only supported value; #4110 removed request_changes/comment as dead config surface); using the default "close".`); + if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" or "advisory" (#4110 removed request_changes/comment as dead config surface); using the default "close".`); return DEFAULT_SCREENSHOT_TABLE_GATE.action; })(); const message = typeof record.message === "string" && record.message.trim().length > 0 ? record.message.trim() : undefined; @@ -84,6 +88,8 @@ export function normalizeScreenshotTableGateConfig(input: unknown, warnings: str whenLabels: normalizeStringList(record.whenLabels, "whenLabels", MAX_LABELS, MAX_LABEL_CHARS, warnings), whenPaths: normalizeStringList(record.whenPaths, "whenPaths", MAX_PATHS, MAX_PATH_CHARS, warnings), action, + requireViewports: normalizeStringList(record.requireViewports, "requireViewports", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), + requireThemes: normalizeStringList(record.requireThemes, "requireThemes", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), ...(message !== undefined ? { message } : {}), }; } @@ -156,6 +162,96 @@ export function hasCommittedImageFile(changedFiles: string[], scopedPaths: strin }); } +const IMAGE_CELL_PATTERN = /!\[[^\]]*\]\([^)]+\)|]*>/i; + +/** One data row of a detected markdown table: the cell texts in source order (leading/trailing pipes and + * whitespace stripped). Deliberately a SEPARATE table-detection pass from {@link hasImageBearingMarkdownTable} + * rather than a shared refactor of it -- that function's exact behavior is pinned by existing tests, and this + * one needs actual cell contents (not just "does some cell have an image"), so duplicating its short + * header+separator detection loop keeps both independently simple instead of risking a regression in either + * from a shared-code change. */ +function extractTableRows(body: string | null | undefined): string[][] { + if (!body) return []; + const lines = body.split(/\r?\n/); + const tableRowPattern = /^\s*\|.*\|\s*$/; + const rows: string[][] = []; + for (let i = 0; i < lines.length - 1; i += 1) { + /* v8 ignore next -- defensive: the loop bound above guarantees lines[i] always exists here. */ + const header = lines[i] ?? ""; + /* v8 ignore next -- defensive: the loop bound above guarantees lines[i + 1] always exists here. */ + const separator = lines[i + 1] ?? ""; + if (!tableRowPattern.test(header) || !isMarkdownTableSeparatorRow(separator)) continue; + let j = i + 2; + /* v8 ignore next -- defensive: the `j < lines.length` guard above guarantees lines[j] always exists here. */ + while (j < lines.length && tableRowPattern.test(lines[j] ?? "")) { + /* v8 ignore next -- defensive: same loop-bound guarantee as above. */ + const line = lines[j] ?? ""; + const cells = line + .trim() + .replace(/^\|/, "") + .replace(/\|$/, "") + .split("|") + .map((cell) => cell.trim()); + rows.push(cells); + j += 1; + } + } + return rows; +} + +/** One (viewport, theme) combination the matrix must cover. `theme: null` means the theme dimension isn't + * required at all (a repo can require viewport coverage without color-mode coverage). */ +export type ScreenshotMatrixPair = { viewport: string; theme: string | null }; + +/** The full set of (viewport, theme) pairs `config` requires, or `[]` when matrix mode is off. Matrix mode + * turns on via `requireViewports` alone -- `requireThemes` with an empty `requireViewports` has no effect, + * since there is no viewport to cross it against. */ +export function requiredScreenshotMatrixPairs(config: ScreenshotTableGateConfig): ScreenshotMatrixPair[] { + if (config.requireViewports.length === 0) return []; + if (config.requireThemes.length === 0) return config.requireViewports.map((viewport) => ({ viewport, theme: null })); + const pairs: ScreenshotMatrixPair[] = []; + for (const viewport of config.requireViewports) { + for (const theme of config.requireThemes) pairs.push({ viewport, theme }); + } + return pairs; +} + +/** True when some row's first cell (the row LABEL, e.g. "Desktop · Light") mentions both `pair.viewport` and + * `pair.theme` (case-insensitive substring match -- tolerant of whatever separator character the contributor + * used between them) AND that row has at least two image-bearing cells among the rest (before + after). */ +function rowSatisfiesMatrixPair(row: string[], pair: ScreenshotMatrixPair): boolean { + const label = (row[0] ?? "").toLowerCase(); + if (!label.includes(pair.viewport.toLowerCase())) return false; + if (pair.theme !== null && !label.includes(pair.theme.toLowerCase())) return false; + const imageCells = row.slice(1).filter((cell) => IMAGE_CELL_PATTERN.test(cell)).length; + return imageCells >= 2; +} + +/** The subset of `pairs` with NO satisfying row anywhere in `body`'s tables. Empty ⇒ full coverage. */ +export function missingScreenshotMatrixPairs(body: string | null | undefined, pairs: ScreenshotMatrixPair[]): ScreenshotMatrixPair[] { + if (pairs.length === 0) return []; + const rows = extractTableRows(body); + return pairs.filter((pair) => !rows.some((row) => rowSatisfiesMatrixPair(row, pair))); +} + +function formatMatrixPair(pair: ScreenshotMatrixPair): string { + return pair.theme === null ? pair.viewport : `${pair.viewport} · ${pair.theme}`; +} + +/** Build the rejection reason for a matrix violation, naming exactly which viewport/theme combinations are + * still missing a real before+after pair -- so the contributor knows precisely what to add, not just that + * "something" is missing. */ +export function buildScreenshotMatrixMessage(missing: ScreenshotMatrixPair[]): string { + const list = missing.map(formatMatrixPair).join(", "); + const dimensionLabel = missing.some((pair) => pair.theme !== null) ? "viewport × theme" : "viewport"; + return ( + "This pull request changes UI/visual code but its screenshot evidence is incomplete. Every required " + + `${dimensionLabel} combination needs its own before/after image pair in a labeled table row (e.g. ` + + '"Desktop · Light | before | after"). Still missing: ' + + `${list}.\n\nPlease resubmit with the remaining rows filled in.` + ); +} + /** True when the PR is IN SCOPE for the gate: it carries one of `config.whenLabels` OR touches a path matching * one of `config.whenPaths`. Both empty ⇒ every PR is in scope (an operator who enables the gate with no * scoping at all wants it enforced everywhere). Only one non-empty list configured ⇒ that list alone decides @@ -182,10 +278,16 @@ export type ScreenshotTableGateResult = { const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null }; -/** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. In - * scope AND (no image-bearing table in the body OR an image pasted outside a table OR a committed image file - * under a scoped path), UNLESS `botCaptureSatisfied` ⇒ violated, with the configured (or default) templated - * message as the reason. */ +/** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. + * `botCaptureSatisfied` ⇒ no violation regardless of mode (an automated capture is equivalent to a + * hand-authored table, and the bot doesn't (yet) shoot a full viewport/theme matrix -- see #4535's scope note). + * + * Two modes, chosen by whether `config.requireViewports` is non-empty (#4535): + * - MATRIX mode: every required (viewport, theme) pair (`requiredScreenshotMatrixPairs`) must have a labeled + * before/after row. Violated ⇒ the reason names exactly which pairs are still missing. + * - PRESENCE mode (the original #2006 behavior, unchanged): in scope AND (no image-bearing table in the body + * OR an image pasted outside a table OR a committed image file under a scoped path) ⇒ violated, with the + * configured (or default) templated message as the reason. */ export function evaluateScreenshotTableGate(input: { config: ScreenshotTableGateConfig; prBody: string | null | undefined; @@ -203,6 +305,14 @@ export function evaluateScreenshotTableGate(input: { if (!config.enabled) return NO_VIOLATION; if (!isScreenshotTableGateInScope(config, input.prLabels, input.changedFiles)) return NO_VIOLATION; if (input.botCaptureSatisfied === true) return NO_VIOLATION; + + const matrixPairs = requiredScreenshotMatrixPairs(config); + if (matrixPairs.length > 0) { + const missing = missingScreenshotMatrixPairs(input.prBody, matrixPairs); + if (missing.length === 0) return NO_VIOLATION; + return { violated: true, reason: config.message ?? buildScreenshotMatrixMessage(missing) }; + } + const hasTable = hasImageBearingMarkdownTable(input.prBody); const outsideTable = hasImageOutsideTable(input.prBody); const committedImage = hasCommittedImageFile(input.changedFiles, config.whenPaths); diff --git a/src/settings/agent-actions.ts b/src/settings/agent-actions.ts index ca11b9b2b0..a22385ec9e 100644 --- a/src/settings/agent-actions.ts +++ b/src/settings/agent-actions.ts @@ -358,8 +358,9 @@ export type AgentActionPlanInput = { // satisfies the gate, so `matched` here is already false whenever the bot capture succeeded (see // evaluateScreenshotTableGate's `botCaptureSatisfied` input). Same zero-hallucination short-circuit shape as // blacklistMatch — fires ahead of ALL merit/CI/AI analysis, for a CONTRIBUTOR only, so its close is tagged - // `closeKind: "screenshot_table"`. Absent / not-violated ⇒ no effect. `"close"` is the gate's only - // enforcement action (#4110 removed the dead request_changes/comment surface — see ScreenshotTableGateAction). + // `closeKind: "screenshot_table"`. Absent / not-violated ⇒ no effect. The caller (processors.ts) only ever + // populates this field when the gate's configured `action` is `"close"` — an `"advisory"` violation (#4535) + // never reaches the planner at all, by construction (see ScreenshotTableGateAction). screenshotTableMatch?: { matched: boolean; reason: string | null } | undefined; pr: { mergeableState?: string | null | undefined; diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 9e8704a6ea..ca34682799 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -566,6 +566,8 @@ export function resolveEffectiveSettings( whenLabels: screenshotTableGateOverride.whenLabels ?? base.whenLabels, whenPaths: screenshotTableGateOverride.whenPaths ?? base.whenPaths, action: screenshotTableGateOverride.action ?? base.action, + requireViewports: screenshotTableGateOverride.requireViewports ?? base.requireViewports, + requireThemes: screenshotTableGateOverride.requireThemes ?? base.requireThemes, message: screenshotTableGateOverride.message ?? base.message, }; } diff --git a/src/types.ts b/src/types.ts index 9822278a3c..2e2106d690 100644 --- a/src/types.ts +++ b/src/types.ts @@ -1101,9 +1101,14 @@ export type RepositorySettings = { /** #4110: `request_changes`/`comment` were REMOVED (not just left unused) -- they were fully typed/validated * but `src/queue/processors.ts` only ever branched on `=== "close"`, so setting either in `.gittensory.yml` - * silently did nothing. `"close"` is the only value this gate has ever enforced; a legacy config with either - * removed value normalizes to the default ("close") with a warning, exactly like any other invalid value. */ -export type ScreenshotTableGateAction = "close"; + * silently did nothing. A legacy config with either removed value normalizes to the default ("close") with a + * warning, exactly like any other invalid value. + * `"advisory"` (#4535) is a NEW, actually-wired value, not a resurrection of either removed one: the gate + * still computes the violation and its reason, but `src/queue/processors.ts` only ever folds the result into + * the close-triggering `screenshotTableMatch` when `action === "close"` -- so `"advisory"` is a real no-op on + * merge/close by construction, with visibility left to the AI reviewer's own commentary (its context is + * expected to mention the same completeness requirement -- see the review-context sync in the #4540 PR). */ +export type ScreenshotTableGateAction = "close" | "advisory"; /** Per-repo config for the before/after screenshot-table gate (#2006). See {@link RepositorySettings.screenshotTableGate} * and `review/screenshot-table-gate.ts` for the normalizer + pure evaluator. */ @@ -1113,6 +1118,14 @@ export type ScreenshotTableGateConfig = { whenPaths: string[]; action: ScreenshotTableGateAction; message?: string | undefined; + /** Viewport x theme completeness matrix (#4535). Both empty (the default) ⇒ byte-identical to the original + * presence-only check (some image-bearing table, anywhere). A non-empty `requireViewports` switches the + * evaluator into matrix mode: every configured viewport (crossed with every configured theme, when + * `requireThemes` is also non-empty) must have its own labeled before/after row in the PR body's table -- + * see `review/screenshot-table-gate.ts` for the row-matching heuristic. `requireThemes` alone (viewports + * empty) has no effect -- the viewport dimension is what turns matrix mode on. */ + requireViewports: string[]; + requireThemes: string[]; }; export type CommandAuthorizationRole = "maintainer" | "collaborator" | "pr_author" | "confirmed_miner"; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index 490819391d..0ba3417878 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -2903,7 +2903,7 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = it("resolveEffectiveSettings falls back to the built-in default when the DB layer has no screenshotTableGate at all (#2006)", () => { const db = {} as unknown as RepositorySettings; const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { enabled: true } } })); - expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close" }); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] }); }); it("resolveEffectiveSettings keeps the DB layer's enabled/action when the manifest override omits them (#2006)", () => { @@ -2924,6 +2924,30 @@ describe("parseFocusManifest settings override + resolveEffectiveSettings", () = expect(parsed.warnings).toContain(`Manifest "settings.screenshotTableGate" must be an object; ignoring it and keeping any existing policy.`); }); + it("wires settings.screenshotTableGate.requireViewports/requireThemes into the manifest parser as a sparse override (#4535)", () => { + const parsed = parseFocusManifest({ settings: { screenshotTableGate: { requireViewports: ["Desktop", "Tablet", "Mobile"], requireThemes: ["Light", "Dark"] } } }); + expect(parsed.settings.screenshotTableGate).toEqual({ requireViewports: ["Desktop", "Tablet", "Mobile"], requireThemes: ["Light", "Dark"] }); + }); + + it("omits requireViewports/requireThemes from the sparse override when the raw manifest doesn't name them (#4535)", () => { + const parsed = parseFocusManifest({ settings: { screenshotTableGate: { enabled: true } } }); + expect(parsed.settings.screenshotTableGate).toEqual({ enabled: true }); + expect(parsed.settings.screenshotTableGate).not.toHaveProperty("requireViewports"); + expect(parsed.settings.screenshotTableGate).not.toHaveProperty("requireThemes"); + }); + + it("resolveEffectiveSettings merges requireViewports/requireThemes without clearing the DB layer's other fields (#4535)", () => { + const db = { screenshotTableGate: { enabled: true, whenLabels: ["frontend"], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] } } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { requireViewports: ["Desktop"], requireThemes: ["Light", "Dark"] } } })); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: ["frontend"], whenPaths: [], action: "close", requireViewports: ["Desktop"], requireThemes: ["Light", "Dark"] }); + }); + + it("resolveEffectiveSettings keeps the DB layer's requireViewports/requireThemes when the manifest override omits them (#4535)", () => { + const db = { screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: ["Desktop"], requireThemes: ["Light"] } } as unknown as RepositorySettings; + const eff = resolveEffectiveSettings(db, parseFocusManifest({ settings: { screenshotTableGate: { enabled: true } } })); + expect(eff.screenshotTableGate).toEqual({ enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: ["Desktop"], requireThemes: ["Light"] }); + }); + it("wires settings.advisoryAiRouting into the manifest parser as a sparse override (#4364)", () => { const parsed = parseFocusManifest({ settings: { advisoryAiRouting: { slop: true, summaries: true } } }); expect(parsed.settings.advisoryAiRouting).toEqual({ slop: true, summaries: true }); diff --git a/test/unit/repository-settings-screenshot-table-gate.test.ts b/test/unit/repository-settings-screenshot-table-gate.test.ts index caf94336aa..65c458f2cd 100644 --- a/test/unit/repository-settings-screenshot-table-gate.test.ts +++ b/test/unit/repository-settings-screenshot-table-gate.test.ts @@ -9,7 +9,7 @@ describe("repository_settings: screenshotTableGate (#2006)", () => { it("getRepositorySettings returns the disabled default for a repo with no DB row at all", async () => { const env = createTestEnv(); const settings = await getRepositorySettings(env, "acme/brand-new-repo"); - expect(settings.screenshotTableGate).toEqual({ enabled: false, whenLabels: [], whenPaths: [], action: "close" }); + expect(settings.screenshotTableGate).toEqual({ enabled: false, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] }); }); it("upsertRepositorySettings persists the disabled default when the caller omits screenshotTableGate entirely", async () => { @@ -28,6 +28,8 @@ describe("repository_settings: screenshotTableGate (#2006)", () => { whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", + requireViewports: [], + requireThemes: [], message: "Custom contract text", }, }); @@ -37,22 +39,24 @@ describe("repository_settings: screenshotTableGate (#2006)", () => { whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", + requireViewports: [], + requireThemes: [], message: "Custom contract text", }); }); it("a true read-modify-write caller carries the persisted value forward explicitly (no DB merge)", async () => { const env = createTestEnv(); - await upsertRepositorySettings(env, { repoFullName: "acme/round-trip", screenshotTableGate: { enabled: true, whenLabels: ["visual"], whenPaths: [], action: "close" } }); + await upsertRepositorySettings(env, { repoFullName: "acme/round-trip", screenshotTableGate: { enabled: true, whenLabels: ["visual"], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] } }); const settings = await getRepositorySettings(env, "acme/round-trip"); await upsertRepositorySettings(env, { ...settings, repoFullName: "acme/round-trip" }); const after = await getRepositorySettings(env, "acme/round-trip"); - expect(after.screenshotTableGate).toEqual({ enabled: true, whenLabels: ["visual"], whenPaths: [], action: "close" }); + expect(after.screenshotTableGate).toEqual({ enabled: true, whenLabels: ["visual"], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] }); }); it("omits `message` entirely when unset (never persists an empty string)", async () => { const env = createTestEnv(); - await upsertRepositorySettings(env, { repoFullName: "acme/no-message", screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close" } }); + await upsertRepositorySettings(env, { repoFullName: "acme/no-message", screenshotTableGate: { enabled: true, whenLabels: [], whenPaths: [], action: "close", requireViewports: [], requireThemes: [] } }); const settings = await getRepositorySettings(env, "acme/no-message"); expect(settings.screenshotTableGate?.message).toBeUndefined(); }); diff --git a/test/unit/screenshot-table-gate-engine.test.ts b/test/unit/screenshot-table-gate-engine.test.ts index 0fea5845f4..559a62cf3d 100644 --- a/test/unit/screenshot-table-gate-engine.test.ts +++ b/test/unit/screenshot-table-gate-engine.test.ts @@ -1,6 +1,7 @@ // Mirror of the app suite pointed at the gittensory-engine copy so the extracted module owns its branch coverage (#2280). import { describe, expect, it } from "vitest"; import { + buildScreenshotMatrixMessage, DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, @@ -9,7 +10,10 @@ import { hasImageOutsideTable, isScreenshotTableGateAction, isScreenshotTableGateInScope, + missingScreenshotMatrixPairs, normalizeScreenshotTableGateConfig, + requiredScreenshotMatrixPairs, + type ScreenshotMatrixPair, } from "../../packages/gittensory-engine/src/review/screenshot-table-gate"; import type { ScreenshotTableGateConfig } from "../../packages/gittensory-engine/src/types/manifest-deps-types"; @@ -20,8 +24,9 @@ function config(overrides: Partial = {}): ScreenshotT const TABLE_BODY = ["| Before | After |", "| --- | --- |", "| ![before](https://x/before.png) | ![after](https://x/after.png) |"].join("\n"); describe("isScreenshotTableGateAction", () => { - it("accepts the only valid action", () => { + it("accepts both valid actions", () => { expect(isScreenshotTableGateAction("close")).toBe(true); + expect(isScreenshotTableGateAction("advisory")).toBe(true); }); it("rejects a non-string or unknown value", () => { @@ -190,7 +195,7 @@ describe("normalizeScreenshotTableGateConfig", () => { { enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }, [], ); - expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }); + expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", requireViewports: [], requireThemes: [], message: "custom text" }); }); it("rejects a non-boolean enabled with a warning, falling back to false", () => { @@ -246,6 +251,145 @@ describe("normalizeScreenshotTableGateConfig", () => { expect(result.whenLabels.length).toBe(50); expect(warnings.some((w) => w.includes("capped"))).toBe(true); }); + + it("accepts the new advisory action", () => { + expect(normalizeScreenshotTableGateConfig({ action: "advisory" }, []).action).toBe("advisory"); + }); + + it("parses requireViewports/requireThemes, trimming and defaulting to empty (#4535)", () => { + expect(normalizeScreenshotTableGateConfig({}, []).requireViewports).toEqual([]); + expect(normalizeScreenshotTableGateConfig({}, []).requireThemes).toEqual([]); + const result = normalizeScreenshotTableGateConfig({ requireViewports: [" Desktop ", "Tablet", "Mobile"], requireThemes: [" Light ", "Dark"] }, []); + expect(result.requireViewports).toEqual(["Desktop", "Tablet", "Mobile"]); + expect(result.requireThemes).toEqual(["Light", "Dark"]); + }); + + it("rejects a non-array requireViewports/requireThemes with a warning, falling back to []", () => { + const warnings: string[] = []; + const result = normalizeScreenshotTableGateConfig({ requireViewports: "desktop", requireThemes: "light" }, warnings); + expect(result.requireViewports).toEqual([]); + expect(result.requireThemes).toEqual([]); + expect(warnings.length).toBe(2); + }); + + it("caps requireViewports/requireThemes at their max entry count", () => { + const warnings: string[] = []; + const many = Array.from({ length: 20 }, (_, i) => `viewport-${i}`); + const result = normalizeScreenshotTableGateConfig({ requireViewports: many }, warnings); + expect(result.requireViewports.length).toBe(12); + expect(warnings.some((w) => w.includes("capped"))).toBe(true); + }); +}); + +describe("requiredScreenshotMatrixPairs (#4535)", () => { + it("returns [] (matrix mode off) when requireViewports is empty, regardless of requireThemes", () => { + expect(requiredScreenshotMatrixPairs(config({ requireViewports: [], requireThemes: ["light", "dark"] }))).toEqual([]); + expect(requiredScreenshotMatrixPairs(config())).toEqual([]); + }); + + it("returns one theme:null pair per viewport when requireThemes is empty (viewport-only mode)", () => { + expect(requiredScreenshotMatrixPairs(config({ requireViewports: ["Desktop", "Mobile"], requireThemes: [] }))).toEqual([ + { viewport: "Desktop", theme: null }, + { viewport: "Mobile", theme: null }, + ]); + }); + + it("returns the full cartesian product when both dimensions are configured", () => { + expect(requiredScreenshotMatrixPairs(config({ requireViewports: ["Desktop", "Mobile"], requireThemes: ["Light", "Dark"] }))).toEqual([ + { viewport: "Desktop", theme: "Light" }, + { viewport: "Desktop", theme: "Dark" }, + { viewport: "Mobile", theme: "Light" }, + { viewport: "Mobile", theme: "Dark" }, + ]); + }); +}); + +describe("missingScreenshotMatrixPairs (#4535)", () => { + const FULL_MATRIX_BODY = [ + "| Viewport · Theme | Before | After |", + "| --- | --- | --- |", + "| Desktop · Light | ![b](x.png) | ![a](y.png) |", + "| Desktop · Dark | ![b](x.png) | ![a](y.png) |", + "| Mobile · Light | ![b](x.png) | ![a](y.png) |", + "| Mobile · Dark | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + + it("returns [] when pairs is empty, without scanning the body", () => { + expect(missingScreenshotMatrixPairs("anything", [])).toEqual([]); + }); + + it("returns [] when every required pair has a satisfying labeled row", () => { + const pairs: ScreenshotMatrixPair[] = [ + { viewport: "Desktop", theme: "Light" }, + { viewport: "Mobile", theme: "Dark" }, + ]; + expect(missingScreenshotMatrixPairs(FULL_MATRIX_BODY, pairs)).toEqual([]); + }); + + it("matches viewport/theme case-insensitively and tolerates any separator between them", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| tablet - light | ![b](x.png) | ![a](y.png) |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Tablet", theme: "Light" }])).toEqual([]); + }); + + it("reports a pair missing when no row's label mentions the viewport at all", () => { + const pairs: ScreenshotMatrixPair[] = [{ viewport: "Tablet", theme: "Light" }]; + expect(missingScreenshotMatrixPairs(FULL_MATRIX_BODY, pairs)).toEqual(pairs); + }); + + it("reports a pair missing when the row matches the viewport but not the theme", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| Desktop · Light | ![b](x.png) | ![a](y.png) |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Desktop", theme: "Dark" }])).toEqual([{ viewport: "Desktop", theme: "Dark" }]); + }); + + it("reports a pair missing when the labeled row only has ONE image cell (before, no after)", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| Desktop · Light | ![b](x.png) | no image here |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Desktop", theme: "Light" }])).toEqual([{ viewport: "Desktop", theme: "Light" }]); + }); + + it("REGRESSION (PR #4661 shape): desktop-only 2x2 table is missing every tablet/mobile pair", () => { + const body = [ + "| Theme | Before | After |", + "| --- | --- | --- |", + "| Dark | ![before dark](x.png) | ![after dark](y.png) |", + "| Light | ![before light](x.png) | ![after light](y.png) |", + ].join("\n"); + const pairs = requiredScreenshotMatrixPairs(config({ requireViewports: ["Desktop", "Tablet", "Mobile"], requireThemes: ["Light", "Dark"] })); + const missing = missingScreenshotMatrixPairs(body, pairs); + // Desktop rows never mention "Desktop" in their label (just "Dark"/"Light"), so ALL SIX pairs are + // missing -- the row-label contract requires naming the viewport, not just the theme. + expect(missing).toEqual(pairs); + }); + + it("a viewport:null-theme pair (viewport-only mode) is satisfied by any theme label, or none at all", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| Desktop | ![b](x.png) | ![a](y.png) |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Desktop", theme: null }])).toEqual([]); + }); + + it("handles a null/undefined body (no rows at all) -- every pair is missing", () => { + const pairs: ScreenshotMatrixPair[] = [{ viewport: "Desktop", theme: "Light" }]; + expect(missingScreenshotMatrixPairs(null, pairs)).toEqual(pairs); + expect(missingScreenshotMatrixPairs(undefined, pairs)).toEqual(pairs); + }); +}); + +describe("buildScreenshotMatrixMessage (#4535)", () => { + it("names the missing viewport x theme pairs and uses the 'viewport × theme' dimension label", () => { + const message = buildScreenshotMatrixMessage([ + { viewport: "Tablet", theme: "Light" }, + { viewport: "Mobile", theme: "Dark" }, + ]); + expect(message).toContain("Tablet · Light"); + expect(message).toContain("Mobile · Dark"); + expect(message).toContain("viewport × theme"); + }); + + it("uses the plain 'viewport' dimension label when no missing pair has a theme", () => { + const message = buildScreenshotMatrixMessage([{ viewport: "Tablet", theme: null }]); + expect(message).toContain("Tablet"); + expect(message).not.toContain("Tablet · "); + expect(message).toContain("viewport combination"); + expect(message).not.toContain("viewport × theme"); + }); }); describe("evaluateScreenshotTableGate", () => { @@ -367,4 +511,91 @@ describe("evaluateScreenshotTableGate", () => { expect(result).toEqual({ violated: false, reason: null }); }); }); + + describe("matrix mode (#4535, requireViewports/requireThemes)", () => { + const FULL_MATRIX_BODY = [ + "| Viewport · Theme | Before | After |", + "| --- | --- | --- |", + "| Desktop · Light | ![b](x.png) | ![a](y.png) |", + "| Desktop · Dark | ![b](x.png) | ![a](y.png) |", + "| Tablet · Light | ![b](x.png) | ![a](y.png) |", + "| Tablet · Dark | ![b](x.png) | ![a](y.png) |", + "| Mobile · Light | ![b](x.png) | ![a](y.png) |", + "| Mobile · Dark | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + + function matrixConfig(overrides: Partial = {}) { + return config({ enabled: true, requireViewports: ["Desktop", "Tablet", "Mobile"], requireThemes: ["Light", "Dark"], ...overrides }); + } + + it("no violation when every required viewport x theme pair has a labeled before/after row", () => { + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: FULL_MATRIX_BODY, prLabels: [], changedFiles: [] }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("REGRESSION (metagraphed PR #4661 shape): desktop-only before/after (4/12 images) still violates matrix mode", () => { + const desktopOnlyBody = [ + "| Theme | Before | After |", + "| --- | --- | --- |", + "| Dark | ![before dark](x.png) | ![after dark](y.png) |", + "| Light | ![before light](x.png) | ![after light](y.png) |", + ].join("\n"); + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: desktopOnlyBody, prLabels: [], changedFiles: [] }); + expect(result.violated).toBe(true); + expect(result.reason).toContain("Desktop · Light"); + expect(result.reason).toContain("Tablet · Light"); + expect(result.reason).toContain("Mobile · Dark"); + }); + + it("violates and names only the still-missing pairs when some (not all) rows are present", () => { + const partialBody = [ + "| Viewport · Theme | Before | After |", + "| --- | --- | --- |", + "| Desktop · Light | ![b](x.png) | ![a](y.png) |", + "| Desktop · Dark | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: partialBody, prLabels: [], changedFiles: [] }); + expect(result.violated).toBe(true); + const missingList = (result.reason ?? "").split("Still missing: ")[1] ?? ""; + expect(missingList).not.toContain("Desktop · Light"); + expect(missingList).toContain("Tablet · Light"); + expect(missingList).toContain("Mobile · Dark"); + }); + + it("viewport-only matrix mode (requireThemes empty) is satisfied by one before/after row per viewport", () => { + const body = [ + "| Viewport | Before | After |", + "| --- | --- | --- |", + "| Desktop | ![b](x.png) | ![a](y.png) |", + "| Mobile | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + const result = evaluateScreenshotTableGate({ config: matrixConfig({ requireViewports: ["Desktop", "Mobile"], requireThemes: [] }), prBody: body, prLabels: [], changedFiles: [] }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("a configured message override still wins over the auto-generated matrix message", () => { + const result = evaluateScreenshotTableGate({ config: matrixConfig({ message: "Custom matrix rejection text" }), prBody: "no table", prLabels: [], changedFiles: [] }); + expect(result.reason).toBe("Custom matrix rejection text"); + }); + + it("botCaptureSatisfied short-circuits matrix mode too, even with zero rows", () => { + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: "no table at all", prLabels: [], changedFiles: [], botCaptureSatisfied: true }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("an out-of-scope PR is never violated by matrix mode either", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig({ whenLabels: ["frontend"] }), + prBody: "no table", + prLabels: ["backend"], + changedFiles: [], + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("falls through to presence-only mode (unchanged #2006 behavior) when requireViewports is empty", () => { + const result = evaluateScreenshotTableGate({ config: config({ enabled: true, requireViewports: [] }), prBody: TABLE_BODY, prLabels: [], changedFiles: [] }); + expect(result).toEqual({ violated: false, reason: null }); + }); + }); }); diff --git a/test/unit/screenshot-table-gate.test.ts b/test/unit/screenshot-table-gate.test.ts index e9fd31ca19..c845e515fc 100644 --- a/test/unit/screenshot-table-gate.test.ts +++ b/test/unit/screenshot-table-gate.test.ts @@ -1,5 +1,6 @@ import { describe, expect, it } from "vitest"; import { + buildScreenshotMatrixMessage, DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, DEFAULT_SCREENSHOT_TABLE_GATE, evaluateScreenshotTableGate, @@ -8,7 +9,10 @@ import { hasImageOutsideTable, isScreenshotTableGateAction, isScreenshotTableGateInScope, + missingScreenshotMatrixPairs, normalizeScreenshotTableGateConfig, + requiredScreenshotMatrixPairs, + type ScreenshotMatrixPair, } from "../../src/review/screenshot-table-gate"; import type { ScreenshotTableGateConfig } from "../../src/types"; @@ -19,8 +23,9 @@ function config(overrides: Partial = {}): ScreenshotT const TABLE_BODY = ["| Before | After |", "| --- | --- |", "| ![before](https://x/before.png) | ![after](https://x/after.png) |"].join("\n"); describe("isScreenshotTableGateAction", () => { - it("accepts the only valid action", () => { + it("accepts both valid actions", () => { expect(isScreenshotTableGateAction("close")).toBe(true); + expect(isScreenshotTableGateAction("advisory")).toBe(true); }); it("rejects a non-string or unknown value", () => { @@ -189,7 +194,7 @@ describe("normalizeScreenshotTableGateConfig", () => { { enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }, [], ); - expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", message: "custom text" }); + expect(result).toEqual({ enabled: true, whenLabels: ["frontend", "visual"], whenPaths: ["apps/ui/**"], action: "close", requireViewports: [], requireThemes: [], message: "custom text" }); }); it("rejects a non-boolean enabled with a warning, falling back to false", () => { @@ -245,6 +250,145 @@ describe("normalizeScreenshotTableGateConfig", () => { expect(result.whenLabels.length).toBe(50); expect(warnings.some((w) => w.includes("capped"))).toBe(true); }); + + it("accepts the new advisory action", () => { + expect(normalizeScreenshotTableGateConfig({ action: "advisory" }, []).action).toBe("advisory"); + }); + + it("parses requireViewports/requireThemes, trimming and defaulting to empty (#4535)", () => { + expect(normalizeScreenshotTableGateConfig({}, []).requireViewports).toEqual([]); + expect(normalizeScreenshotTableGateConfig({}, []).requireThemes).toEqual([]); + const result = normalizeScreenshotTableGateConfig({ requireViewports: [" Desktop ", "Tablet", "Mobile"], requireThemes: [" Light ", "Dark"] }, []); + expect(result.requireViewports).toEqual(["Desktop", "Tablet", "Mobile"]); + expect(result.requireThemes).toEqual(["Light", "Dark"]); + }); + + it("rejects a non-array requireViewports/requireThemes with a warning, falling back to []", () => { + const warnings: string[] = []; + const result = normalizeScreenshotTableGateConfig({ requireViewports: "desktop", requireThemes: "light" }, warnings); + expect(result.requireViewports).toEqual([]); + expect(result.requireThemes).toEqual([]); + expect(warnings.length).toBe(2); + }); + + it("caps requireViewports/requireThemes at their max entry count", () => { + const warnings: string[] = []; + const many = Array.from({ length: 20 }, (_, i) => `viewport-${i}`); + const result = normalizeScreenshotTableGateConfig({ requireViewports: many }, warnings); + expect(result.requireViewports.length).toBe(12); + expect(warnings.some((w) => w.includes("capped"))).toBe(true); + }); +}); + +describe("requiredScreenshotMatrixPairs (#4535)", () => { + it("returns [] (matrix mode off) when requireViewports is empty, regardless of requireThemes", () => { + expect(requiredScreenshotMatrixPairs(config({ requireViewports: [], requireThemes: ["light", "dark"] }))).toEqual([]); + expect(requiredScreenshotMatrixPairs(config())).toEqual([]); + }); + + it("returns one theme:null pair per viewport when requireThemes is empty (viewport-only mode)", () => { + expect(requiredScreenshotMatrixPairs(config({ requireViewports: ["Desktop", "Mobile"], requireThemes: [] }))).toEqual([ + { viewport: "Desktop", theme: null }, + { viewport: "Mobile", theme: null }, + ]); + }); + + it("returns the full cartesian product when both dimensions are configured", () => { + expect(requiredScreenshotMatrixPairs(config({ requireViewports: ["Desktop", "Mobile"], requireThemes: ["Light", "Dark"] }))).toEqual([ + { viewport: "Desktop", theme: "Light" }, + { viewport: "Desktop", theme: "Dark" }, + { viewport: "Mobile", theme: "Light" }, + { viewport: "Mobile", theme: "Dark" }, + ]); + }); +}); + +describe("missingScreenshotMatrixPairs (#4535)", () => { + const FULL_MATRIX_BODY = [ + "| Viewport · Theme | Before | After |", + "| --- | --- | --- |", + "| Desktop · Light | ![b](x.png) | ![a](y.png) |", + "| Desktop · Dark | ![b](x.png) | ![a](y.png) |", + "| Mobile · Light | ![b](x.png) | ![a](y.png) |", + "| Mobile · Dark | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + + it("returns [] when pairs is empty, without scanning the body", () => { + expect(missingScreenshotMatrixPairs("anything", [])).toEqual([]); + }); + + it("returns [] when every required pair has a satisfying labeled row", () => { + const pairs: ScreenshotMatrixPair[] = [ + { viewport: "Desktop", theme: "Light" }, + { viewport: "Mobile", theme: "Dark" }, + ]; + expect(missingScreenshotMatrixPairs(FULL_MATRIX_BODY, pairs)).toEqual([]); + }); + + it("matches viewport/theme case-insensitively and tolerates any separator between them", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| tablet - light | ![b](x.png) | ![a](y.png) |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Tablet", theme: "Light" }])).toEqual([]); + }); + + it("reports a pair missing when no row's label mentions the viewport at all", () => { + const pairs: ScreenshotMatrixPair[] = [{ viewport: "Tablet", theme: "Light" }]; + expect(missingScreenshotMatrixPairs(FULL_MATRIX_BODY, pairs)).toEqual(pairs); + }); + + it("reports a pair missing when the row matches the viewport but not the theme", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| Desktop · Light | ![b](x.png) | ![a](y.png) |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Desktop", theme: "Dark" }])).toEqual([{ viewport: "Desktop", theme: "Dark" }]); + }); + + it("reports a pair missing when the labeled row only has ONE image cell (before, no after)", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| Desktop · Light | ![b](x.png) | no image here |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Desktop", theme: "Light" }])).toEqual([{ viewport: "Desktop", theme: "Light" }]); + }); + + it("REGRESSION (PR #4661 shape): desktop-only 2x2 table is missing every tablet/mobile pair", () => { + const body = [ + "| Theme | Before | After |", + "| --- | --- | --- |", + "| Dark | ![before dark](x.png) | ![after dark](y.png) |", + "| Light | ![before light](x.png) | ![after light](y.png) |", + ].join("\n"); + const pairs = requiredScreenshotMatrixPairs(config({ requireViewports: ["Desktop", "Tablet", "Mobile"], requireThemes: ["Light", "Dark"] })); + const missing = missingScreenshotMatrixPairs(body, pairs); + // Desktop rows never mention "Desktop" in their label (just "Dark"/"Light"), so ALL SIX pairs are + // missing -- the row-label contract requires naming the viewport, not just the theme. + expect(missing).toEqual(pairs); + }); + + it("a viewport:null-theme pair (viewport-only mode) is satisfied by any theme label, or none at all", () => { + const body = ["| Row | Before | After |", "| --- | --- | --- |", "| Desktop | ![b](x.png) | ![a](y.png) |"].join("\n"); + expect(missingScreenshotMatrixPairs(body, [{ viewport: "Desktop", theme: null }])).toEqual([]); + }); + + it("handles a null/undefined body (no rows at all) -- every pair is missing", () => { + const pairs: ScreenshotMatrixPair[] = [{ viewport: "Desktop", theme: "Light" }]; + expect(missingScreenshotMatrixPairs(null, pairs)).toEqual(pairs); + expect(missingScreenshotMatrixPairs(undefined, pairs)).toEqual(pairs); + }); +}); + +describe("buildScreenshotMatrixMessage (#4535)", () => { + it("names the missing viewport x theme pairs and uses the 'viewport × theme' dimension label", () => { + const message = buildScreenshotMatrixMessage([ + { viewport: "Tablet", theme: "Light" }, + { viewport: "Mobile", theme: "Dark" }, + ]); + expect(message).toContain("Tablet · Light"); + expect(message).toContain("Mobile · Dark"); + expect(message).toContain("viewport × theme"); + }); + + it("uses the plain 'viewport' dimension label when no missing pair has a theme", () => { + const message = buildScreenshotMatrixMessage([{ viewport: "Tablet", theme: null }]); + expect(message).toContain("Tablet"); + expect(message).not.toContain("Tablet · "); + expect(message).toContain("viewport combination"); + expect(message).not.toContain("viewport × theme"); + }); }); describe("evaluateScreenshotTableGate", () => { @@ -366,4 +510,91 @@ describe("evaluateScreenshotTableGate", () => { expect(result).toEqual({ violated: false, reason: null }); }); }); + + describe("matrix mode (#4535, requireViewports/requireThemes)", () => { + const FULL_MATRIX_BODY = [ + "| Viewport · Theme | Before | After |", + "| --- | --- | --- |", + "| Desktop · Light | ![b](x.png) | ![a](y.png) |", + "| Desktop · Dark | ![b](x.png) | ![a](y.png) |", + "| Tablet · Light | ![b](x.png) | ![a](y.png) |", + "| Tablet · Dark | ![b](x.png) | ![a](y.png) |", + "| Mobile · Light | ![b](x.png) | ![a](y.png) |", + "| Mobile · Dark | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + + function matrixConfig(overrides: Partial = {}) { + return config({ enabled: true, requireViewports: ["Desktop", "Tablet", "Mobile"], requireThemes: ["Light", "Dark"], ...overrides }); + } + + it("no violation when every required viewport x theme pair has a labeled before/after row", () => { + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: FULL_MATRIX_BODY, prLabels: [], changedFiles: [] }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("REGRESSION (metagraphed PR #4661 shape): desktop-only before/after (4/12 images) still violates matrix mode", () => { + const desktopOnlyBody = [ + "| Theme | Before | After |", + "| --- | --- | --- |", + "| Dark | ![before dark](x.png) | ![after dark](y.png) |", + "| Light | ![before light](x.png) | ![after light](y.png) |", + ].join("\n"); + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: desktopOnlyBody, prLabels: [], changedFiles: [] }); + expect(result.violated).toBe(true); + expect(result.reason).toContain("Desktop · Light"); + expect(result.reason).toContain("Tablet · Light"); + expect(result.reason).toContain("Mobile · Dark"); + }); + + it("violates and names only the still-missing pairs when some (not all) rows are present", () => { + const partialBody = [ + "| Viewport · Theme | Before | After |", + "| --- | --- | --- |", + "| Desktop · Light | ![b](x.png) | ![a](y.png) |", + "| Desktop · Dark | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: partialBody, prLabels: [], changedFiles: [] }); + expect(result.violated).toBe(true); + const missingList = (result.reason ?? "").split("Still missing: ")[1] ?? ""; + expect(missingList).not.toContain("Desktop · Light"); + expect(missingList).toContain("Tablet · Light"); + expect(missingList).toContain("Mobile · Dark"); + }); + + it("viewport-only matrix mode (requireThemes empty) is satisfied by one before/after row per viewport", () => { + const body = [ + "| Viewport | Before | After |", + "| --- | --- | --- |", + "| Desktop | ![b](x.png) | ![a](y.png) |", + "| Mobile | ![b](x.png) | ![a](y.png) |", + ].join("\n"); + const result = evaluateScreenshotTableGate({ config: matrixConfig({ requireViewports: ["Desktop", "Mobile"], requireThemes: [] }), prBody: body, prLabels: [], changedFiles: [] }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("a configured message override still wins over the auto-generated matrix message", () => { + const result = evaluateScreenshotTableGate({ config: matrixConfig({ message: "Custom matrix rejection text" }), prBody: "no table", prLabels: [], changedFiles: [] }); + expect(result.reason).toBe("Custom matrix rejection text"); + }); + + it("botCaptureSatisfied short-circuits matrix mode too, even with zero rows", () => { + const result = evaluateScreenshotTableGate({ config: matrixConfig(), prBody: "no table at all", prLabels: [], changedFiles: [], botCaptureSatisfied: true }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("an out-of-scope PR is never violated by matrix mode either", () => { + const result = evaluateScreenshotTableGate({ + config: matrixConfig({ whenLabels: ["frontend"] }), + prBody: "no table", + prLabels: ["backend"], + changedFiles: [], + }); + expect(result).toEqual({ violated: false, reason: null }); + }); + + it("falls through to presence-only mode (unchanged #2006 behavior) when requireViewports is empty", () => { + const result = evaluateScreenshotTableGate({ config: config({ enabled: true, requireViewports: [] }), prBody: TABLE_BODY, prLabels: [], changedFiles: [] }); + expect(result).toEqual({ violated: false, reason: null }); + }); + }); }); From 4988e49ebbce88b499dffd02adbdd3d27eaccbb2 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 17:47:11 -0700 Subject: [PATCH 2/3] fix(review): renumber migration to 0131 (0130 was claimed by another PR mid-rebase) --- ...able_gate_matrix.sql => 0131_screenshot_table_gate_matrix.sql} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename migrations/{0130_screenshot_table_gate_matrix.sql => 0131_screenshot_table_gate_matrix.sql} (100%) diff --git a/migrations/0130_screenshot_table_gate_matrix.sql b/migrations/0131_screenshot_table_gate_matrix.sql similarity index 100% rename from migrations/0130_screenshot_table_gate_matrix.sql rename to migrations/0131_screenshot_table_gate_matrix.sql From 5a34d0034885255e666482c8d3d6030c58a9c2b8 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Thu, 9 Jul 2026 18:01:26 -0700 Subject: [PATCH 3/3] fix(review): mark the defensive row[0] fallback unreachable for coverage (codecov/patch) --- packages/gittensory-engine/src/review/screenshot-table-gate.ts | 3 +++ src/review/screenshot-table-gate.ts | 3 +++ 2 files changed, 6 insertions(+) diff --git a/packages/gittensory-engine/src/review/screenshot-table-gate.ts b/packages/gittensory-engine/src/review/screenshot-table-gate.ts index 0edf7fc054..4d02e30957 100644 --- a/packages/gittensory-engine/src/review/screenshot-table-gate.ts +++ b/packages/gittensory-engine/src/review/screenshot-table-gate.ts @@ -220,6 +220,9 @@ export function requiredScreenshotMatrixPairs(config: ScreenshotTableGateConfig) * `pair.theme` (case-insensitive substring match -- tolerant of whatever separator character the contributor * used between them) AND that row has at least two image-bearing cells among the rest (before + after). */ function rowSatisfiesMatrixPair(row: string[], pair: ScreenshotMatrixPair): boolean { + // `?? ""` only exists to satisfy noUncheckedIndexedAccess -- `extractTableRows`'s `.split("|")` always + // produces at least one cell, even for an empty-string row, so `row[0]` is never actually undefined here. + /* v8 ignore next -- defensive: see the comment above. */ const label = (row[0] ?? "").toLowerCase(); if (!label.includes(pair.viewport.toLowerCase())) return false; if (pair.theme !== null && !label.includes(pair.theme.toLowerCase())) return false; diff --git a/src/review/screenshot-table-gate.ts b/src/review/screenshot-table-gate.ts index 415588bcfd..b232d4c2f1 100644 --- a/src/review/screenshot-table-gate.ts +++ b/src/review/screenshot-table-gate.ts @@ -220,6 +220,9 @@ export function requiredScreenshotMatrixPairs(config: ScreenshotTableGateConfig) * `pair.theme` (case-insensitive substring match -- tolerant of whatever separator character the contributor * used between them) AND that row has at least two image-bearing cells among the rest (before + after). */ function rowSatisfiesMatrixPair(row: string[], pair: ScreenshotMatrixPair): boolean { + // `?? ""` only exists to satisfy noUncheckedIndexedAccess -- `extractTableRows`'s `.split("|")` always + // produces at least one cell, even for an empty-string row, so `row[0]` is never actually undefined here. + /* v8 ignore next -- defensive: see the comment above. */ const label = (row[0] ?? "").toLowerCase(); if (!label.includes(pair.viewport.toLowerCase())) return false; if (pair.theme !== null && !label.includes(pair.theme.toLowerCase())) return false;