diff --git a/.github/scripts/generate-pr-comment.js b/.github/scripts/generate-pr-comment.js index c82d11c77a6fb..6aceefc3ee745 100644 --- a/.github/scripts/generate-pr-comment.js +++ b/.github/scripts/generate-pr-comment.js @@ -4,12 +4,13 @@ /** * @description Generates and posts PR comment with analysis results - * @input --analysis --a11y --storybook-url --run-url + * @input --analysis --a11y --visual --storybook-url --run-url * @output Formatted markdown comment body to stdout */ const fs = require('node:fs'); const { buildA11ySection } = require('./lib/a11y-format'); +const { buildVisualSection } = require('./lib/visual-format'); const args = process.argv.slice(2); const getArg = (name) => { @@ -19,6 +20,7 @@ const getArg = (name) => { const analysisFile = getArg('analysis') || 'analysis.json'; const a11yFile = getArg('a11y') || 'a11y-report.json'; +const visualFile = getArg('visual'); const runUrl = getArg('run-url') || ''; const prNumber = getArg('pr-number') || ''; const storybookUrl = getArg('storybook-url') || ''; @@ -115,6 +117,18 @@ if (analysis.modifiedComponents && analysis.modifiedComponents.length > 0) { // Build accessibility section using shared module const a11ySection = buildA11ySection(a11yReport); +// Visual regression is optional: the pr-visual job is skipped when no +// components changed, and absent entirely on older runs. +let visualVerdict = null; +if (visualFile) { + try { + visualVerdict = JSON.parse(fs.readFileSync(visualFile, 'utf8')); + } catch { + visualVerdict = null; + } +} +const visualSection = buildVisualSection(visualVerdict, getArg('visual-report-url')); + // Build bundle size section — one row per package the PR actually touched. let bundleSection = '### Bundle Size Summary\n\n'; // Prefer the multi-package list; fall back to the legacy single-core shape so @@ -187,7 +201,7 @@ const body = `## PR Analysis Report ${diffModeCaveat}${storybookSection}${sandboxSection}${componentSection || '_No new or modified components detected._\n\n'} ${bundleSection} ${a11ySection} ---- +${visualSection}--- Generated by PR Enrichment workflow${footerLinksStr} `; diff --git a/.github/scripts/lib/visual-format.js b/.github/scripts/lib/visual-format.js index e0a3b34366043..96bd6445bd57b 100644 --- a/.github/scripts/lib/visual-format.js +++ b/.github/scripts/lib/visual-format.js @@ -31,10 +31,15 @@ function buildVisualSection(verdict, reportUrl) { return `### Visual Regression\n\n**Status:** ${verdict.counts.failed} shot(s) could not be captured.${link}\n\n`; } if (!verdict.changes || verdict.changes.length === 0) { - const added = verdict.counts?.added - ? ` ${verdict.counts.added} new shot(s) had no baseline to compare against.` + const compared = verdict.counts.total - (verdict.counts.added ?? 0); + // A PR-scoped run shoots every story of the touched component in every + // theme that styles it, which is deeper than the daily gate's baseline + // reaches — so some shots legitimately have nothing to compare against. + // Saying "added" there reads as a problem; saying it plainly does not. + const unbaselined = verdict.counts?.added + ? ` ${verdict.counts.added} shot(s) have no baseline yet and were not compared.` : ''; - return `### Visual Regression\n\n**Status:** No visual change across ${verdict.counts.total} shot(s).${added}\n\n`; + return `### Visual Regression\n\n**Status:** No visual change across ${compared} compared shot(s).${unbaselined}\n\n`; } const rows = verdict.changes diff --git a/.github/scripts/lib/visual-format.test.mjs b/.github/scripts/lib/visual-format.test.mjs new file mode 100644 index 0000000000000..703d845eff56b --- /dev/null +++ b/.github/scripts/lib/visual-format.test.mjs @@ -0,0 +1,99 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +import {describe, expect, it} from 'vitest'; +import {createRequire} from 'node:module'; + +const {buildVisualSection} = createRequire(import.meta.url)('./visual-format.js'); + +const verdict = (overrides = {}) => ({ + status: 'pass', + counts: {total: 16, unchanged: 16, changed: 0, added: 0, removed: 0, failed: 0}, + changes: [], + ...overrides, +}); + +describe('buildVisualSection', () => { + it('renders nothing when the job did not run', () => { + expect(buildVisualSection(null)).toBe(''); + }); + + it('says so plainly when nothing moved', () => { + expect(buildVisualSection(verdict())).toContain('No visual change across 16 compared shot(s)'); + }); + + it('calls an unbaselined shot exactly that, not an addition', () => { + const section = buildVisualSection( + verdict({counts: {total: 20, unchanged: 14, changed: 0, added: 6, removed: 0, failed: 0}}), + ); + expect(section).toContain('No visual change across 14 compared shot(s)'); + expect(section).toContain('6 shot(s) have no baseline yet'); + }); + + it('states the reason for a skip, so a broad PR does not look like a pass', () => { + const section = buildVisualSection( + verdict({status: 'skipped', reason: '900 shots exceeds the 240-shot budget'}), + ); + expect(section).toContain('Skipped'); + expect(section).toContain('900 shots exceeds the 240-shot budget'); + }); + + it('lists each changed shot with the theme and mode it changed in', () => { + const section = buildVisualSection( + verdict({ + status: 'changed', + counts: {total: 16, unchanged: 14, changed: 2, added: 0, removed: 0, failed: 0}, + changes: [ + {key: 'a', component: 'Button', name: 'Primary', theme: 'y2k', mode: 'light', diffPixels: 1126}, + {key: 'b', component: 'Button', name: 'Primary', theme: 'y2k', mode: 'dark', diffPixels: 1401}, + ], + }), + ); + expect(section).toContain('2 of 16 shot(s) changed'); + expect(section).toContain('| Button | Primary | y2k | light | 1,126 |'); + }); + + it('frames a change as a question rather than a failure', () => { + const section = buildVisualSection( + verdict({ + status: 'changed', + counts: {total: 2, unchanged: 1, changed: 1, added: 0, removed: 0, failed: 0}, + changes: [{key: 'a', component: 'B', name: 'S', theme: 't', mode: 'light', diffPixels: 5}], + }), + ); + expect(section).toMatch(/question, not a failure/); + }); + + it('caps the table and says how many were left out', () => { + const changes = Array.from({length: 25}, (_, index) => ({ + key: `k${index}`, + component: 'C', + name: 'S', + theme: 't', + mode: 'light', + diffPixels: index, + })); + const section = buildVisualSection( + verdict({status: 'changed', counts: {total: 25, changed: 25}, changes}), + ); + expect(section).toContain('and 5 more'); + }); + + it('reports a capture failure distinctly from a change', () => { + const section = buildVisualSection( + verdict({status: 'failed', counts: {total: 4, failed: 2, changed: 0}}), + ); + expect(section).toContain('2 shot(s) could not be captured'); + }); + + it('links the report when one was published', () => { + const section = buildVisualSection( + verdict({ + status: 'changed', + counts: {total: 1, changed: 1}, + changes: [{key: 'a', component: 'B', name: 'S', theme: 't', mode: 'light', diffPixels: 1}], + }), + 'https://example.com/report/', + ); + expect(section).toContain('https://example.com/report/'); + }); +}); diff --git a/.github/scripts/visual-gate/README.md b/.github/scripts/visual-gate/README.md index f78c397cb0e12..171ce2105d334 100644 --- a/.github/scripts/visual-gate/README.md +++ b/.github/scripts/visual-gate/README.md @@ -77,6 +77,39 @@ runner would read as "everything changed". The gate refuses that comparison instead of showing you 500 false diffs, and the shared baseline is only ever written by CI, from the pinned runner label. +## Two different questions + +The gate asks two things, and only one of them is a screenshot. + +**Did anything move?** — the shot tiers, compared against an accepted baseline. +Catches any visual regression, in any theme. + +**Did each theming target's override actually reach the pixels?** — `gate.mjs +reach`, and no baseline is involved. A pixel diff cannot answer this: when an +override stops applying, the frame is captured broken and promoted as correct, +and every later run agrees with it forever. The probe theme gives every +selector a unique deterministic colour, so this is an equality test — compute +the colour that selector should have produced, read the element, compare. It +names the target instead of a rectangle, and it cannot flake. + +Three outcomes, because the difference matters: + +| | meaning | +| -------- | -------------------------------------------------------------------------------- | +| reached | the override painted | +| shadowed | another target on the _same element_ won — a fact about the markup, not a defect | +| missed | nothing probe-coloured won | + +```bash +node .github/scripts/visual-gate/gate.mjs reach +``` + +It runs in the daily gate and is **reported, not enforced**. Today 50 targets +miss, from one systemic cause: StyleX emits into `@layer priority1-4`, which +sort _after_ `astryx-theme`, so wherever a component sets a property the theme +override loses. Failing the gate on a known systemic issue would only teach +everyone to ignore it — enforce it once that is fixed and the count is zero. + ## How the release cut uses it The daily cut (08:00 PT) reads the gate's verdict before it merges the version diff --git a/.github/scripts/visual-gate/gate.mjs b/.github/scripts/visual-gate/gate.mjs index fa1e8c4ed20b0..3b59503e98823 100644 --- a/.github/scripts/visual-gate/gate.mjs +++ b/.github/scripts/visual-gate/gate.mjs @@ -30,6 +30,7 @@ import {fileURLToPath} from 'node:url'; import {capture, scout} from './lib/capture.mjs'; import {analyzeTargeting, buildVerdict, compareCaptures} from './lib/compare.mjs'; import {buildPlan, readStoryIndex} from './lib/plan.mjs'; +import {READ_TARGETS, emptyAccumulator, fold} from './lib/probe-reach.mjs'; import {renderReport} from './lib/report.mjs'; import {accept, incomparable, readBaseline} from './lib/baseline.mjs'; import {loadConfig, loadThemeOverrides, loadThemingTargets} from './lib/sources.mjs'; @@ -54,6 +55,19 @@ const tiers = (flag('tiers') ?? config.tiers.join(',')).split(',').filter(Boolea const sample = flag('sample') ? Number(flag('sample')) : null; /** Restrict the plan to story ids containing any of these — for debugging a shot, never for a gate run. */ const only = (flag('only') ?? '').split(',').filter(Boolean); +/** For the `component` tier: the components a PR touched. */ +const components = (flag('components') ?? '').split(',').filter(Boolean); +/** + * Above this many shots the run declines instead of capturing. + * + * A sweeping PR — a token change, a shared hook, a rename across the system — + * would put hundreds of diffs in front of a reviewer who has no way to judge + * them one by one, and the honest answer is that a per-PR check is the wrong + * instrument for that change: the daily gate reviews it against the whole + * baseline instead. Declining loudly beats either timing out or dumping a + * report nobody can read. + */ +const maxShots = flag('max-shots') ? Number(flag('max-shots')) : Infinity; /** * Which stories the scout needs to look at: every story of a component some @@ -77,17 +91,19 @@ function storiesToScout(stories, targets, themeOverrides) { async function plan() { const [targets, themeOverrides] = await Promise.all([ loadThemingTargets(REPO_ROOT), - loadThemeOverrides(REPO_ROOT), + loadThemeOverrides(REPO_ROOT, config.probeTheme), ]); const stories = readStoryIndex(storybookDir, Object.keys(config.excludeStories)); let observations; - if (!has('no-scout') && tiers.includes('theme-matrix')) { + if (!has('no-scout') && (tiers.includes('theme-matrix') || tiers.includes('probe'))) { const cachePath = flag('observations'); if (cachePath && fs.existsSync(cachePath)) { observations = JSON.parse(fs.readFileSync(cachePath, 'utf8')); } else { - const storyIds = storiesToScout(stories, targets, themeOverrides); + const storyIds = tiers.includes('probe') + ? stories.map(story => story.id) + : storiesToScout(stories, targets, themeOverrides); process.stderr.write(`Scouting ${storyIds.length} stories for theming targets…\n`); observations = await scout({ storyIds, @@ -106,6 +122,8 @@ async function plan() { observations, defaultTheme: config.defaultTheme, tiers, + components, + probeTheme: config.probeTheme, }); // A sample is for trying the rig out, never for a gate run: it is taken // evenly across the plan so it spans components rather than the first few. @@ -163,6 +181,34 @@ function stageReportImages({reportDir, keys, currentDir, baselinePath}) { async function check() { const shots = await plan(); + + // Over budget: say so in the verdict rather than capturing. The report and + // the PR comment both render this, so a skipped check is visible as a + // decision, never as a silent pass. + if (shots.length > maxShots) { + const verdict = { + version: 1, + status: 'skipped', + generatedAt: new Date().toISOString(), + reason: `${shots.length} shots exceeds the ${maxShots}-shot budget${components.length ? ` (${components.length} components touched)` : ''} — too broad to review shot by shot here. The daily release gate covers this change against the full baseline.`, + counts: {total: shots.length, unchanged: 0, changed: 0, added: 0, removed: 0, failed: 0}, + components, + changes: [], + }; + fs.mkdirSync(outDir, {recursive: true}); + fs.writeFileSync(path.join(outDir, 'verdict.json'), `${JSON.stringify(verdict, null, 2)}\n`); + const summary = `## Visual gate: skipped\n\n${verdict.reason}\n`; + process.stdout.write(summary); + if (flag('summary-output')) fs.writeFileSync(flag('summary-output'), summary); + if (process.env.GITHUB_OUTPUT) { + fs.appendFileSync( + process.env.GITHUB_OUTPUT, + `status=skipped\nchanged=0\nadded=0\nfailed=0\ntotal=${shots.length}\n`, + ); + } + return EXIT.clean; + } + process.stderr.write(`Visual gate: ${shots.length} shots (${tiers.join(', ')})\n`); const {manifest, failures} = await runCapture(shots); @@ -182,11 +228,12 @@ async function check() { diffDir: path.join(reportDir, 'diff'), threshold: config.threshold, maxDiffPixels: config.maxDiffPixels, + scoped: components.length > 0, }); const [targets, themeOverrides] = await Promise.all([ loadThemingTargets(REPO_ROOT), - loadThemeOverrides(REPO_ROOT), + loadThemeOverrides(REPO_ROOT, config.probeTheme), ]); const targeting = analyzeTargeting({ observedTargets: manifest.observedTargets, @@ -200,7 +247,7 @@ async function check() { baselineManifest, targeting, failures, - context: {...manifest.context, tiers, baselineExists: exists}, + context: {...manifest.context, tiers, baselineExists: exists, scoped: components.length > 0, components}, }); stageReportImages({ @@ -301,6 +348,68 @@ async function main() { } case 'check': return check(); + case 'reach': { + // The assertion a pixel diff cannot make: did each target's override + // actually arrive? No baseline, no images — the probe theme's unique + // per-selector colour is the fingerprint, so this is an equality test. + const {chromium} = await import('playwright'); + const {serveDirectory} = await import('./lib/capture.mjs'); + const stories = readStoryIndex(storybookDir, Object.keys(config.excludeStories)); + const subject = only.length + ? stories.filter(story => only.some(f => story.storyId ?? story.id.includes(f))) + : stories; + const server = await serveDirectory(storybookDir); + const origin = `http://127.0.0.1:${server.port}`; + const browser = await chromium.launch(); + const page = await browser.newPage({viewport: config.viewport}); + const acc = emptyAccumulator(); + let done = 0; + for (const story of subject) { + try { + await page.goto( + `${origin}/iframe.html?id=${encodeURIComponent(story.id)}&viewMode=story&globals=astryxTheme:${config.probeTheme};colorMode:light`, + {waitUntil: 'load', timeout: 30000}, + ); + await page.waitForSelector('#storybook-root > *', {timeout: 20000}); + await page.evaluate(() => document.fonts.ready); + fold(acc, await page.evaluate(READ_TARGETS), story.id); + } catch { + // A story that will not render contributes no readings; the visual + // tier reports it as a capture failure. + } + if (++done % 100 === 0) process.stderr.write(` read ${done}/${subject.length}\n`); + } + await browser.close(); + await server.close(); + + const declared = (await loadThemingTargets(REPO_ROOT)).map(t => t.key); + const unseen = [...new Set(declared)].filter( + key => !acc.verified.has(key) && !acc.failures.has(key) && !acc.shadowed.has(key), + ); + const out = { + version: 1, + generatedAt: new Date().toISOString(), + verified: [...acc.verified].sort(), + failures: Object.fromEntries([...acc.failures].sort()), + shadowed: Object.fromEntries([...acc.shadowed].sort()), + neverRendered: unseen.sort(), + }; + fs.mkdirSync(outDir, {recursive: true}); + fs.writeFileSync(path.join(outDir, 'reach.json'), `${JSON.stringify(out, null, 2)}\n`); + + process.stdout.write( + `reached the pixels: ${out.verified.length}\n` + + `shares an element with another: ${Object.keys(out.shadowed).length}\n` + + `override did NOT arrive: ${Object.keys(out.failures).length}\n` + + `no story renders it: ${out.neverRendered.length}\n`, + ); + for (const [key, info] of Object.entries(out.failures).slice(0, 30)) { + process.stdout.write( + ` ${key.padEnd(30)} got ${String(info.got).padEnd(22)} want ${info.expected} (${info.storyId})\n`, + ); + } + return Object.keys(out.failures).length > 0 ? EXIT.changed : EXIT.clean; + } case 'flaky': { // A gate that cries wolf gets ignored, so the exclusion list is // evidence, not guesswork: capture the same build twice and name every @@ -369,7 +478,7 @@ async function main() { } default: process.stderr.write( - 'Usage: gate.mjs [--storybook-dir dir] [--baseline dir] [--out dir] [--tiers a,b] [--sample n]\n', + 'Usage: gate.mjs [--storybook-dir dir] [--baseline dir] [--out dir] [--tiers a,b] [--sample n]\n', ); return EXIT.crashed; } diff --git a/.github/scripts/visual-gate/generate-probe-theme.mjs b/.github/scripts/visual-gate/generate-probe-theme.mjs new file mode 100644 index 0000000000000..d4eeb6b96e756 --- /dev/null +++ b/.github/scripts/visual-gate/generate-probe-theme.mjs @@ -0,0 +1,172 @@ +#!/usr/bin/env node +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file Generate (or verify) the probe theme from the component docs. + * + * @input the repo's component docs + * @output packages/themes/probe/src/probeTheme.ts, and a coverage summary + * + * `--check` makes it a CI guard: adding a theming target without regenerating + * fails the build, which is the whole point — a target that nobody remembered + * to cover is exactly the target that silently stops working. + * + * Usage: + * node .github/scripts/visual-gate/generate-probe-theme.mjs [--check] + */ + +import * as fs from 'node:fs'; +import * as path from 'node:path'; +import {fileURLToPath, pathToFileURL} from 'node:url'; + +import {buildProbeComponents, renderProbeTheme} from './lib/probe-theme.mjs'; +import {loadThemingTargets} from './lib/sources.mjs'; + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '../../..'); +const OUT = path.join(REPO_ROOT, 'packages/themes/probe/src/probeTheme.ts'); +const check = process.argv.includes('--check'); + +/** + * Documented props per component, for resolving a visual prop's value set. + * @returns {Promise>>} + */ +async function loadProps() { + const {loadComponentDoc} = await import( + pathToFileURL(path.join(REPO_ROOT, 'packages/cli/foundation/discovery/component-loader.mjs')) + .href + ); + /** @type {Record>} */ + const byComponent = {}; + const root = path.join(REPO_ROOT, 'packages/core/src'); + + /** @param {string} dir */ + const scan = async dir => { + for (const entry of fs.readdirSync(dir, {withFileTypes: true})) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === 'node_modules' || entry.name === '__tests__') continue; + await scan(full); + continue; + } + if (!entry.name.endsWith('.doc.mjs')) continue; + try { + const doc = await loadComponentDoc(full); + const name = doc?.name || path.basename(path.dirname(full)); + byComponent[name] = [...(byComponent[name] ?? []), ...(doc?.props ?? [])]; + } catch { + // An unreadable doc contributes no props; the target enumeration skips + // it too, so the two stay consistent. + } + } + }; + await scan(root); + return byComponent; +} + +/** + * Exported string-union type aliases across core, so a doc that says + * `size: AvatarSize` still yields its values. + * + * A regex over source, not the TypeScript compiler: this runs in a `--check` + * on every PR, and the cost of a full type-check is not worth it for what is + * a one-line declaration in practice. A union it cannot parse falls through to + * the skipped list, where it is reported rather than silently dropped. + * + * @returns {Record} + */ +function loadTypeAliases() { + /** @type {Record} */ + const raw = {}; + const root = path.join(REPO_ROOT, 'packages/core/src'); + const pattern = /(?:export\s+)?type\s+(\w+)\s*=\s*([^;{]+);/g; + + /** @param {string} dir */ + const scan = dir => { + for (const entry of fs.readdirSync(dir, {withFileTypes: true})) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === 'node_modules' || entry.name === '__tests__') continue; + scan(full); + continue; + } + if (!/\.tsx?$/.test(entry.name) || entry.name.endsWith('.test.ts')) continue; + for (const match of fs.readFileSync(full, 'utf8').matchAll(pattern)) { + raw[match[1]] ??= match[2]; + } + } + }; + scan(root); + + // Aliases compose (`AvatarSize = AvatarNamedSize | AvatarNumericSize`), so + // resolve transitively. The depth bound is a cycle guard, not a real limit. + /** @param {string} type @param {number} depth @returns {string[]} */ + const resolve = (type, depth = 0) => { + if (depth > 4) return []; + /** @type {string[]} */ + const values = []; + for (const part of type.split('|').map(piece => piece.trim())) { + const literal = part.match(/^'([^']+)'$/); + if (literal) { + values.push(literal[1]); + continue; + } + if (raw[part]) values.push(...resolve(raw[part], depth + 1)); + } + return values; + }; + + /** @type {Record} */ + const aliases = {}; + for (const name of Object.keys(raw)) { + const values = resolve(raw[name]); + if (values.length > 1) aliases[name] = [...new Set(values)]; + } + return aliases; +} + +const targets = await loadThemingTargets(REPO_ROOT); +const built = buildProbeComponents(targets, await loadProps(), loadTypeAliases()); + +// Format with the repo's own prettier config. The pre-commit hook formats +// every staged .ts, so a generator emitting anything else would produce a file +// that is reformatted the moment it is committed — and `--check` would then +// fail on every run afterwards, for a difference in whitespace. +const prettier = await import('prettier'); +const options = await prettier.resolveConfig(OUT); +const source = await prettier.format(renderProbeTheme(built), { + ...options, + filepath: OUT, +}); + +if (check) { + const current = fs.existsSync(OUT) ? fs.readFileSync(OUT, 'utf8') : ''; + if (current !== source) { + process.stderr.write( + '::error::The probe theme is out of date — a theming target changed without regenerating it.\n' + + 'A target the probe theme does not cover is a target the visual gate cannot verify.\n' + + 'Run: pnpm visual:probe-theme\n', + ); + process.exit(1); + } + process.stdout.write( + `✅ probe theme current — ${built.coverage.targets} targets, ${built.coverage.selectors} selectors.\n`, + ); + process.exit(0); +} + +fs.mkdirSync(path.dirname(OUT), {recursive: true}); +fs.writeFileSync(OUT, source); +process.stdout.write( + `Wrote ${path.relative(REPO_ROOT, OUT)} — ${built.coverage.targets} targets, ${built.coverage.selectors} selectors.\n`, +); +if (built.coverage.skipped.length > 0) { + process.stdout.write( + `\n${built.coverage.skipped.length} visual prop(s) could not be enumerated (no string-union type), so their values are unprobed:\n`, + ); + for (const entry of built.coverage.skipped.slice(0, 15)) { + process.stdout.write(` ${entry.key}.${entry.prop} — ${entry.reason}\n`); + } + if (built.coverage.skipped.length > 15) { + process.stdout.write(` … and ${built.coverage.skipped.length - 15} more\n`); + } +} diff --git a/.github/scripts/visual-gate/lib/compare.mjs b/.github/scripts/visual-gate/lib/compare.mjs index f57d1685ef57f..3fadcc3ac6bd3 100644 --- a/.github/scripts/visual-gate/lib/compare.mjs +++ b/.github/scripts/visual-gate/lib/compare.mjs @@ -55,6 +55,7 @@ function pad(png, width, height, PNG) { * @param {string} options.diffDir - where diff PNGs are written * @param {number} options.threshold - pixelmatch per-pixel colour threshold * @param {number} options.maxDiffPixels - pixels allowed to differ before a shot counts as changed + * @param {boolean} [options.scoped] - the plan covers only part of the baseline * @returns {Promise<{changes: Change[], added: string[], removed: string[], unchanged: string[]}>} */ export async function compareCaptures({ @@ -65,6 +66,7 @@ export async function compareCaptures({ diffDir, threshold, maxDiffPixels, + scoped = false, }) { const {PNG} = await import('pngjs'); const pixelmatch = (await import('pixelmatch')).default; @@ -123,7 +125,13 @@ export async function compareCaptures({ }); } - const removed = [...baselineKeys].filter(key => !currentManifest.shots[key]); + // A scoped run (a PR shooting only the components it touched) deliberately + // captures a fraction of the baseline, so "in the baseline, not in this run" + // means out of scope — not removed. Reporting it as removal would put a + // five-hundred-shot deletion on every PR. + const removed = scoped + ? [] + : [...baselineKeys].filter(key => !currentManifest.shots[key]); changes.sort((a, b) => b.diffPixels - a.diffPixels); return {changes, added, removed, unchanged}; } diff --git a/.github/scripts/visual-gate/lib/plan.mjs b/.github/scripts/visual-gate/lib/plan.mjs index cfec7ab1456c5..d51b97937ebb8 100644 --- a/.github/scripts/visual-gate/lib/plan.mjs +++ b/.github/scripts/visual-gate/lib/plan.mjs @@ -145,10 +145,21 @@ function rank(name) { * @param {Record>} options.themeOverrides - theme → component key → override selectors * @param {Record>} [options.observations] - story id → targets it rendered, from a scout pass * @param {string} options.defaultTheme - * @param {string[]} options.tiers - any of 'theme-matrix', 'surface', 'full' + * @param {string[]} options.tiers - any of 'theme-matrix', 'surface', 'full', 'component', 'probe' + * @param {string[]} [options.components] - for the 'component' tier: the components to cover + * @param {string} [options.probeTheme] - name of the generated coverage theme * @returns {Shot[]} */ -export function buildPlan({stories, targets, themeOverrides, observations, defaultTheme, tiers}) { +export function buildPlan({ + stories, + targets, + themeOverrides, + observations, + defaultTheme, + tiers, + components = [], + probeTheme = 'probe', +}) { /** @type {Map} */ const shots = new Map(); const representatives = representativeStories(stories); @@ -173,15 +184,104 @@ export function buildPlan({stories, targets, themeOverrides, observations, defau } } + if (tiers.includes('component')) { + // The PR tier: every story of the named components, in the default theme + // and in every theme that styles them. Deeper than `surface` (which shoots + // one story per component), and narrow enough to run per PR. + const themesByComponent = new Map(); + for (const target of targets) { + for (const [theme, keys] of Object.entries(themeOverrides)) { + if (!Object.hasOwn(keys, target.key)) continue; + if (!themesByComponent.has(target.component)) { + themesByComponent.set(target.component, new Set()); + } + themesByComponent.get(target.component).add(theme); + } + } + for (const story of stories) { + if (!components.includes(story.component)) continue; + for (const mode of MODES) { + add({...toShotBase(story), theme: defaultTheme, mode}, 'component'); + for (const theme of themesByComponent.get(story.component) ?? []) { + if (theme === defaultTheme) continue; + add({...toShotBase(story), theme, mode}, `theme:${theme}`); + } + } + } + } + if (tiers.includes('theme-matrix')) { for (const shot of themeMatrix({stories, targets, themeOverrides, observations})) { add(shot.shot, shot.reason); } } + if (tiers.includes('probe')) { + // The coverage tier. The probe theme styles every declared target, so + // "which story shows this target" is the only question left — and the + // scout already answered it. One shot per target, on the story that + // renders it, which is what makes a newly added target verified from the + // day its doc lands instead of whenever a designer happens to style it. + for (const {shot, reason} of probeShots({ + stories, + targets, + observations, + probeTheme, + })) { + add(shot, reason); + } + } + return [...shots.values()].sort((a, b) => a.key.localeCompare(b.key)); } +/** + * One shot per theming target, in the probe theme, on a story that renders it. + * + * Targets are grouped so a story covering twenty of them costs one shot, not + * twenty: the probe theme colours every target differently, so a single frame + * verifies all of them at once. Without observations there is nothing to aim + * at — the probe tier needs the scout. + * + * @param {object} options + * @param {ReturnType} options.stories + * @param {Array<{key: string, component: string}>} options.targets + * @param {Record>} [options.observations] + * @param {string} options.probeTheme + */ +function probeShots({stories, targets, observations, probeTheme}) { + if (!observations) return []; + + const wanted = new Set(targets.map(target => target.key)); + const byStory = new Map(); + for (const story of stories) { + const rendered = Object.keys(observations[story.id] ?? {}).filter(key => wanted.has(key)); + if (rendered.length > 0) byStory.set(story, new Set(rendered)); + } + + // Greedy set cover: fewest stories that between them render every target. + const planned = []; + const uncovered = new Set(wanted); + while (uncovered.size > 0) { + let best = null; + let bestCount = 0; + for (const [story, rendered] of byStory) { + const count = [...rendered].filter(key => uncovered.has(key)).length; + if (count > bestCount) { + best = story; + bestCount = count; + } + } + if (!best) break; + for (const key of byStory.get(best)) uncovered.delete(key); + for (const mode of MODES) { + planned.push({shot: {...toShotBase(best), theme: probeTheme, mode}, reason: 'probe'}); + } + byStory.delete(best); + } + return planned; +} + /** * The targeted net: for every selector a theme overrides, one story that * actually renders it. diff --git a/.github/scripts/visual-gate/lib/probe-reach.mjs b/.github/scripts/visual-gate/lib/probe-reach.mjs new file mode 100644 index 0000000000000..3420511323b4a --- /dev/null +++ b/.github/scripts/visual-gate/lib/probe-reach.mjs @@ -0,0 +1,140 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file Does each theming target's override actually reach the pixels? + * + * @input a built Storybook, the probe theme + * @output per target: the override arrived, or it did not and what won instead + * + * This is the assertion the screenshot tier cannot make. A visual baseline + * proves a component looks the same as last week — including when the reason + * it looks the same is that a theme override stopped applying and the shot was + * captured broken and promoted as correct. Nothing in a pixel diff can tell + * those apart. + * + * The probe theme gives every selector a unique deterministic colour, so + * "did this override reach this element" is an equality test rather than a + * diff: compute the colour the selector should have produced, read the + * element's computed style, compare. No baseline, no images, no flake, and it + * names the failing target instead of a rectangle of moved pixels. + * + * An element is checked against EVERY selector that legitimately addresses it + * — `base`, each reflected prop, each reflected state — because a + * `variant:info` override beating `base` is the cascade working, not a miss. + */ + +import {paint} from './probe-theme.mjs'; + +/** + * `hsl(H S% L%)` → the `rgb(r, g, b)` string getComputedStyle returns. + * @param {string} hsl + * @returns {string} + */ +export function hslToRgb(hsl) { + const [h, s, l] = hsl.match(/[\d.]+/g).map(Number); + const saturation = s / 100; + const lightness = l / 100; + const k = n => (n + h / 30) % 12; + const a = saturation * Math.min(lightness, 1 - lightness); + const f = n => lightness - a * Math.max(-1, Math.min(k(n) - 3, Math.min(9 - k(n), 1))); + const to255 = n => Math.round(f(n) * 255); + return `rgb(${to255(0)}, ${to255(8)}, ${to255(4)})`; +} + +/** + * Every colour a correctly-applied probe theme could legitimately produce on + * this element. + * @param {string} key - target key (class minus the `astryx-` prefix) + * @param {string[]} data - reflected `data-*` as `name` or `name:value` + * @returns {string[]} + */ +export function expectedColors(key, data) { + const seeds = [key, ...data.map(entry => `${key}.${entry}`)]; + // Every property the generator paints, for every selector that addresses + // this element — an element with no background can still prove the override + // arrived through its text or border colour. + return seeds.flatMap(seed => Object.values(paint(seed))).map(hslToRgb); +} + +/** + * Read every themed element on the page, with the data a theme can address it + * by. Runs in the browser. + * @returns {string} + */ +export const READ_TARGETS = `(() => { + const out = []; + for (const el of document.querySelectorAll('[class*="astryx-"]')) { + const data = []; + for (const a of el.attributes) { + if (!a.name.startsWith('data-')) continue; + const n = a.name.slice(5); + data.push(a.value === '' || a.value === 'true' ? n : n + ':' + a.value); + } + const cs = getComputedStyle(el); + // Several targets can sit on ONE element (a date input's toggle icon is + // also an icon). Whichever rule wins, only one colour can be there, so a + // reading carries every co-located target and the caller decides. + const keys = [...el.classList].filter(c => c.startsWith('astryx-')).map(c => c.slice(7)); + if (keys.length === 0) continue; + out.push({keys, data, bg: cs.backgroundColor, color: cs.color, border: cs.borderTopColor}); + } + return out; +})()`; + +/** + * Fold a page's readings into the running verdict. + * + * A target is verified as soon as ONE element proves the override arrived; + * later elements of the same target that legitimately show something else + * (a state the probe does not colour, an inherited surface) must not + * un-verify it. + * + * @param {{verified: Set, failures: Map}} acc + * @param {Array<{key: string, data: string[], bg: string}>} readings + * @param {string} storyId + */ +export function fold(acc, readings, storyId) { + for (const {keys, data, bg, color, border} of readings) { + // The probe paints background, text and border from independent hashes, so + // an element that cannot show a background (an inline glyph, a + // display:contents wrapper) can still prove the override arrived. + const painted = [bg, color, border]; + + for (const key of keys) { + if (acc.verified.has(key)) continue; + const mine = expectedColors(key, data); + if (painted.some(value => mine.includes(value))) { + acc.verified.add(key); + acc.failures.delete(key); + acc.shadowed.delete(key); + continue; + } + // Another target on this same element won. That is a fact about the + // markup, not a broken override — the two targets are the same element, + // so only one colour can be there. Reported separately, because + // "these two targets are one element" is worth knowing and is NOT the + // same finding as "this override reaches nothing". + const sibling = keys.some( + other => other !== key && painted.some(v => expectedColors(other, data).includes(v)), + ); + if (sibling) { + if (!acc.failures.has(key) && !acc.shadowed.has(key)) { + acc.shadowed.set(key, {storyId, sharesElementWith: keys.filter(k => k !== key)}); + } + continue; + } + if (!acc.failures.has(key)) { + // First failing story wins: the report needs one stable place to + // point, and re-pointing it at whichever story was walked last makes + // the same failure read differently between runs. + acc.failures.set(key, {storyId, got: bg, expected: mine[0], data}); + } + } + } + return acc; +} + +/** @returns {{verified: Set, failures: Map, shadowed: Map}} */ +export function emptyAccumulator() { + return {verified: new Set(), failures: new Map(), shadowed: new Map()}; +} diff --git a/.github/scripts/visual-gate/lib/probe-reach.test.mjs b/.github/scripts/visual-gate/lib/probe-reach.test.mjs new file mode 100644 index 0000000000000..4fea606d75f97 --- /dev/null +++ b/.github/scripts/visual-gate/lib/probe-reach.test.mjs @@ -0,0 +1,124 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +import {describe, expect, it} from 'vitest'; + +import {emptyAccumulator, expectedColors, fold, hslToRgb} from './probe-reach.mjs'; +import {paint, probeColor} from './probe-theme.mjs'; + +const rgbOf = seed => hslToRgb(probeColor(seed)); +const textOf = seed => hslToRgb(paint(seed).color); + +describe('hslToRgb', () => { + it('matches the rgb() form getComputedStyle returns', () => { + expect(hslToRgb('hsl(0 100% 50%)')).toBe('rgb(255, 0, 0)'); + expect(hslToRgb('hsl(120 100% 25%)')).toBe('rgb(0, 128, 0)'); + }); + + it('round-trips a probe colour', () => { + expect(rgbOf('badge')).toMatch(/^rgb\(\d+, \d+, \d+\)$/); + }); +}); + +describe('expectedColors', () => { + it('accepts the base colour when the element reflects no data', () => { + expect(expectedColors('badge', [])).toContain(rgbOf('badge')); + }); + + it('covers every property the generator paints, not just the background', () => { + expect(expectedColors('badge', [])).toContain(textOf('badge')); + }); + + it('also accepts a variant colour — a variant beating base is the cascade working', () => { + const colors = expectedColors('badge', ['variant:info']); + expect(colors).toContain(rgbOf('badge.variant:info')); + expect(colors).toContain(rgbOf('badge')); + }); +}); + +describe('fold', () => { + it('accepts proof from text or border, so an element with no background still counts', () => { + const acc = fold( + emptyAccumulator(), + [{keys: ['icon'], data: [], bg: 'rgba(0, 0, 0, 0)', color: textOf('icon')}], + 's', + ); + expect([...acc.verified]).toEqual(['icon']); + }); + + it('calls a target shadowed — not failed — when another target on the SAME element won', () => { + const acc = fold( + emptyAccumulator(), + [{keys: ['date-input-toggle-icon', 'icon'], data: [], bg: rgbOf('icon')}], + 's', + ); + expect([...acc.verified]).toEqual(['icon']); + expect(acc.failures.size).toBe(0); + expect(acc.shadowed.get('date-input-toggle-icon')).toMatchObject({ + sharesElementWith: ['icon'], + }); + }); + + it('promotes a shadowed target to verified once it wins somewhere else', () => { + const acc = emptyAccumulator(); + fold(acc, [{keys: ['a', 'b'], data: [], bg: rgbOf('b')}], 'one'); + expect(acc.shadowed.has('a')).toBe(true); + fold(acc, [{keys: ['a'], data: [], bg: rgbOf('a')}], 'two'); + expect(acc.shadowed.has('a')).toBe(false); + expect(acc.verified.has('a')).toBe(true); + }); + + it('still fails a target when NOTHING probe-coloured won on its element', () => { + const acc = fold( + emptyAccumulator(), + [{keys: ['card', 'surface'], data: [], bg: 'rgb(255, 255, 255)'}], + 's', + ); + expect(acc.failures.has('card')).toBe(true); + expect(acc.shadowed.size).toBe(0); + }); + + it('verifies a target whose override arrived', () => { + const acc = fold(emptyAccumulator(), [{keys: ['badge'], data: [], bg: rgbOf('badge')}], 's'); + expect([...acc.verified]).toEqual(['badge']); + expect(acc.failures.size).toBe(0); + }); + + it('fails a target showing the component colour instead of the override', () => { + const acc = fold(emptyAccumulator(), [{keys: ['badge'], data: [], bg: 'rgb(0, 100, 224)'}], 's'); + expect(acc.verified.size).toBe(0); + expect(acc.failures.get('badge')).toMatchObject({got: 'rgb(0, 100, 224)', storyId: 's'}); + }); + + it('credits the variant colour on a variant element', () => { + const acc = fold( + emptyAccumulator(), + [{keys: ['badge'], data: ['variant:info'], bg: rgbOf('badge.variant:info')}], + 's', + ); + expect([...acc.verified]).toEqual(['badge']); + }); + + it('one proof is enough — a later element cannot un-verify a target', () => { + const acc = emptyAccumulator(); + fold(acc, [{keys: ['badge'], data: [], bg: rgbOf('badge')}], 'a'); + fold(acc, [{keys: ['badge'], data: [], bg: 'rgb(1, 2, 3)'}], 'b'); + expect([...acc.verified]).toEqual(['badge']); + expect(acc.failures.size).toBe(0); + }); + + it('clears an earlier failure once the target is proven elsewhere', () => { + const acc = emptyAccumulator(); + fold(acc, [{keys: ['badge'], data: [], bg: 'rgb(1, 2, 3)'}], 'a'); + expect(acc.failures.has('badge')).toBe(true); + fold(acc, [{keys: ['badge'], data: [], bg: rgbOf('badge')}], 'b'); + expect(acc.failures.has('badge')).toBe(false); + expect([...acc.verified]).toEqual(['badge']); + }); + + it('keeps the first failing story, so the report can point somewhere real', () => { + const acc = emptyAccumulator(); + fold(acc, [{keys: ['card'], data: [], bg: 'rgb(255, 255, 255)'}], 'first'); + fold(acc, [{keys: ['card'], data: [], bg: 'rgb(255, 255, 255)'}], 'second'); + expect(acc.failures.get('card').storyId).toBe('first'); + }); +}); diff --git a/.github/scripts/visual-gate/lib/probe-theme.mjs b/.github/scripts/visual-gate/lib/probe-theme.mjs new file mode 100644 index 0000000000000..f0ade098fa3cd --- /dev/null +++ b/.github/scripts/visual-gate/lib/probe-theme.mjs @@ -0,0 +1,177 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file The probe theme: a theme that styles every declared theming target. + * + * @input the component docs (via the CLI's own target enumeration) + * @output a `defineTheme` config covering every target, variant and state the + * system documents as themeable + * + * Real themes style what their designer cared about, so most of the themeable + * surface is never exercised by any of them — on this repo the gate found 49 + * such overrides, and a newly added target starts life in that unverified set + * by default. Nothing tells you when a target stops working, because nothing + * was ever styling it. + * + * The probe theme closes that by construction: it is GENERATED from the target + * enumeration, so a target added tomorrow is covered the moment its doc lands — + * no one has to remember to write a case for it. It is a test fixture, never + * shipped, and it is deliberately garish: every override is a loud, unmistakable + * value, because the question it answers is "did this override reach the pixels + * at all", not "does this look good". + * + * Each selector gets a DISTINCT colour, derived from a hash of its name. Two + * targets that are supposed to be different elements but actually resolve to + * the same element show up as one colour instead of two — which is exactly the + * bug ("this sub-target isn't really separate") that a uniform hot-pink theme + * would hide. + */ + +/** + * A stable, well-separated colour per selector name. Deterministic: the same + * selector is the same colour in every run, so a baseline stays comparable. + * @param {string} seed + * @param {{lightness?: number}} [options] + * @returns {string} + */ +export function probeColor(seed, options = {}) { + let hash = 0; + for (let index = 0; index < seed.length; index += 1) { + hash = (Math.imul(hash, 31) + seed.charCodeAt(index)) | 0; + } + // Golden-angle hue stepping keeps adjacent selectors far apart in hue. + const hue = Math.abs(hash * 137.508) % 360; + const saturation = 70 + (Math.abs(hash >> 8) % 25); + const lightness = options.lightness ?? 45 + (Math.abs(hash >> 16) % 20); + return `hsl(${hue.toFixed(1)} ${saturation}% ${lightness}%)`; +} + +/** + * Prop values from a doc's `type` union — `'a' | 'b' | 'c'` → ['a','b','c']. + * + * A named alias (`AvatarSize`) is not a union at this point; `aliases` carries + * the ones resolved from source, because a doc that says `size: AvatarSize` + * documents just as real a variant axis as one that spells the union inline, + * and skipping those left a third of the surface unprobed. + * + * A prop whose type is neither (a number, a boolean, an object) contributes + * nothing: there is no enumerable value set to probe. + * + * @param {string | undefined} type + * @param {Record} [aliases] + * @returns {string[]} + */ +export function unionValues(type, aliases = {}) { + if (typeof type !== 'string') return []; + const inline = [...type.matchAll(/'([^']+)'/g)].map(match => match[1]); + // A union of one is a literal type, not a variant axis. + if (inline.length > 1) return inline; + const named = aliases[type.trim()]; + return named && named.length > 1 ? named : []; +} + +/** + * Build the probe theme's `components` map. + * + * @param {Array<{key: string, component: string, props: string[], states: string[]}>} targets + * @param {Record>} propsByComponent + * @param {Record} [aliases] - named type aliases resolved from source + * @returns {{components: Record>>, coverage: {targets: number, selectors: number, skipped: Array<{key: string, prop: string, reason: string}>}}} + */ +export function buildProbeComponents(targets, propsByComponent, aliases = {}) { + /** @type {Record>>} */ + const components = {}; + /** @type {Array<{key: string, prop: string, reason: string}>} */ + const skipped = []; + let selectors = 0; + + for (const target of targets) { + const styles = (components[target.key] ??= {}); + + if (!styles.base) { + styles.base = paint(`${target.key}`); + selectors += 1; + } + + for (const prop of target.props) { + const declared = propsByComponent[target.component]?.find(entry => entry.name === prop); + const values = unionValues(declared?.type, aliases); + if (values.length === 0) { + skipped.push({ + key: target.key, + prop, + reason: declared + ? `type "${declared.type}" is not an enumerable string union` + : 'not a documented prop of the owning component (usually a sub-element derived from another prop)', + }); + continue; + } + for (const value of values) { + const selector = `${prop}:${value}`; + if (styles[selector]) continue; + styles[selector] = paint(`${target.key}.${selector}`); + selectors += 1; + } + } + + for (const state of target.states) { + if (styles[state]) continue; + styles[state] = paint(`${target.key}.${state}`); + selectors += 1; + } + } + + return {components, coverage: {targets: Object.keys(components).length, selectors, skipped}}; +} + +/** + * Paint a selector so that each property is independently verifiable. + * + * Every property gets its own hue derived from the same seed, rather than one + * flat colour: a single colour for both `backgroundColor` and `color` renders + * the text invisible, which hides a text-colour regression behind a working + * background — and makes the diff report unreadable for the human who has to + * judge it. Lightness is pinned so the text always contrasts with the fill. + * + * Exported so the reach check computes expectations from the SAME function + * that generates the theme — two copies of this mapping would drift, and the + * check would then report the drift as a broken override. + * + * @param {string} seed + * @returns {{backgroundColor: string, color: string, borderColor: string, outlineColor: string}} + */ +export function paint(seed) { + return { + backgroundColor: probeColor(seed), + color: probeColor(`${seed}/text`, {lightness: 12}), + borderColor: probeColor(`${seed}/border`, {lightness: 25}), + outlineColor: probeColor(`${seed}/outline`, {lightness: 25}), + }; +} + +/** + * The generated theme source. Written to disk rather than built in memory so + * the coverage it claims is reviewable in a diff — when a target is added, the + * probe theme's diff is the record that it became covered. + * + * @param {ReturnType} built + * @returns {string} + */ +export function renderProbeTheme({components, coverage}) { + return `// Copyright (c) Meta Platforms, Inc. and affiliates. +// @generated by .github/scripts/visual-gate/generate-probe-theme.mjs — do not edit. +// +// A theme that styles EVERY declared theming target, so the visual gate can +// prove each one still reaches the pixels. Not shipped; not published; a test +// fixture. Regenerate with: pnpm visual:probe-theme +// +// Coverage: ${coverage.targets} targets, ${coverage.selectors} selectors. + +import {defineTheme} from '@astryxdesign/core/theme'; + +export const probeTheme = defineTheme({ + name: 'probe', + components: ${JSON.stringify(components, null, 2).replace(/\n/g, '\n ')}, +}); +`; +} diff --git a/.github/scripts/visual-gate/lib/probe-theme.test.mjs b/.github/scripts/visual-gate/lib/probe-theme.test.mjs new file mode 100644 index 0000000000000..15fa8b68e7746 --- /dev/null +++ b/.github/scripts/visual-gate/lib/probe-theme.test.mjs @@ -0,0 +1,87 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +import {describe, expect, it} from 'vitest'; + +import {buildProbeComponents, probeColor, unionValues} from './probe-theme.mjs'; + +describe('unionValues', () => { + it('reads an inline string union', () => { + expect(unionValues("'sm' | 'md' | 'lg'")).toEqual(['sm', 'md', 'lg']); + }); + + it('resolves a named alias, so a doc that says `size: AvatarSize` is still probed', () => { + expect(unionValues('AvatarSize', {AvatarSize: ['sm', 'lg']})).toEqual(['sm', 'lg']); + }); + + it('ignores a single-literal type — that is a constant, not a variant axis', () => { + expect(unionValues("'only'")).toEqual([]); + }); + + it('ignores a type with no enumerable values', () => { + expect(unionValues('number')).toEqual([]); + expect(unionValues(undefined)).toEqual([]); + }); +}); + +describe('probeColor', () => { + it('is deterministic, so a baseline stays comparable across runs', () => { + expect(probeColor('badge.base')).toBe(probeColor('badge.base')); + }); + + it('gives different selectors different colours, so two targets that collapse into one element show it', () => { + expect(probeColor('badge.base')).not.toBe(probeColor('badge.variant:error')); + }); + + it('honours a pinned lightness, so text stays readable against its own fill', () => { + expect(probeColor('x', {lightness: 12})).toMatch(/12%\)$/); + }); +}); + +describe('buildProbeComponents', () => { + const targets = [ + {key: 'badge', component: 'Badge', props: ['variant'], states: []}, + {key: 'switch', component: 'Switch', props: [], states: ['checked', 'disabled']}, + ]; + const props = {Badge: [{name: 'variant', type: "'info' | 'error'"}]}; + + it('covers every target with a base selector', () => { + const {components} = buildProbeComponents(targets, props); + expect(Object.keys(components).sort()).toEqual(['badge', 'switch']); + expect(components.badge.base).toBeDefined(); + expect(components.switch.base).toBeDefined(); + }); + + it('expands a variant prop into one selector per documented value', () => { + const {components} = buildProbeComponents(targets, props); + expect(Object.keys(components.badge).sort()).toEqual(['base', 'variant:error', 'variant:info']); + }); + + it('covers every declared state', () => { + const {components} = buildProbeComponents(targets, props); + expect(Object.keys(components.switch).sort()).toEqual(['base', 'checked', 'disabled']); + }); + + it('paints text and background differently, so an invisible-text regression is still visible', () => { + const {components} = buildProbeComponents(targets, props); + expect(components.badge.base.color).not.toBe(components.badge.base.backgroundColor); + }); + + it('reports a visual prop it cannot enumerate instead of dropping it silently', () => { + const {coverage} = buildProbeComponents( + [{key: 'stack', component: 'Stack', props: ['gap'], states: []}], + {Stack: []}, + ); + expect(coverage.skipped).toEqual([ + {key: 'stack', prop: 'gap', reason: expect.stringContaining('not a documented prop')}, + ]); + }); + + it('counts what it covered, which is what the CI guard asserts', () => { + const {coverage} = buildProbeComponents(targets, props); + expect(coverage).toMatchObject({targets: 2, selectors: 6}); + }); + + it('is deterministic — same docs, same theme, so regeneration is a no-op diff', () => { + expect(buildProbeComponents(targets, props)).toEqual(buildProbeComponents(targets, props)); + }); +}); diff --git a/.github/scripts/visual-gate/lib/sources.mjs b/.github/scripts/visual-gate/lib/sources.mjs index 354cad354b286..3695985fec1fd 100644 --- a/.github/scripts/visual-gate/lib/sources.mjs +++ b/.github/scripts/visual-gate/lib/sources.mjs @@ -42,10 +42,17 @@ export async function loadThemingTargets(repoRoot) { * `defineTheme`, not a literal in its source, so the built artifact is the * only honest answer. * + * The probe theme is deliberately EXCLUDED. It styles every target by + * construction, so feeding it to the theme matrix would ask for a shot per + * (target x story that renders it) — 614 shots here — to answer a question the + * probe tier answers in 128 with a set cover. It is a coverage instrument, not + * a theme someone ships. + * * @param {string} repoRoot + * @param {string} [probeTheme] - name of the coverage fixture to leave out * @returns {Promise>>} */ -export async function loadThemeOverrides(repoRoot) { +export async function loadThemeOverrides(repoRoot, probeTheme = 'probe') { const themesDir = path.join(repoRoot, 'packages/themes'); /** @type {Record>} */ const overrides = {}; @@ -60,7 +67,7 @@ export async function loadThemeOverrides(repoRoot) { } const module = await import(pathToFileURL(built).href); const theme = Object.values(module).find(value => value?.name && value?.components); - if (!theme) continue; + if (!theme || theme.name === probeTheme) continue; overrides[theme.name] = Object.fromEntries( Object.entries(theme.components).map(([key, styles]) => [key, Object.keys(styles ?? {})]), ); @@ -71,7 +78,7 @@ export async function loadThemeOverrides(repoRoot) { /** * @param {string} repoRoot - * @returns {{excludeStories: Record, viewport: {width: number, height: number}, settleMs: number, threshold: number, maxDiffPixels: number, defaultTheme: string, tiers: string[]}} + * @returns {{excludeStories: Record, viewport: {width: number, height: number}, settleMs: number, threshold: number, maxDiffPixels: number, defaultTheme: string, probeTheme: string, tiers: string[]}} */ export function loadConfig(repoRoot) { const defaults = { @@ -81,7 +88,8 @@ export function loadConfig(repoRoot) { threshold: 0.1, maxDiffPixels: 0, defaultTheme: 'neutral', - tiers: ['surface', 'theme-matrix'], + probeTheme: 'probe', + tiers: ['surface', 'theme-matrix', 'probe'], }; const configPath = path.join(repoRoot, '.github/scripts/visual-gate/visual-gate.config.json'); if (!fs.existsSync(configPath)) return defaults; diff --git a/.github/scripts/visual-gate/visual-gate.config.json b/.github/scripts/visual-gate/visual-gate.config.json index 00e034e74dc84..a5400f5aa8807 100644 --- a/.github/scripts/visual-gate/visual-gate.config.json +++ b/.github/scripts/visual-gate/visual-gate.config.json @@ -1,7 +1,9 @@ { "$comment": "Configuration for .github/scripts/visual-gate. Every excludeStories entry needs the reason the story cannot reproduce itself — the list stays reviewable instead of growing quietly, and `gate.mjs flaky` is how entries are found (run it, do not guess). A key ending in * covers a whole story file.", "defaultTheme": "neutral", - "tiers": ["surface", "theme-matrix"], + "$probeTheme": "A generated fixture styling every declared target (packages/themes/probe). The gate treats it as the coverage instrument, not a design: it is what makes a NEW theming target verified from the day its doc lands.", + "probeTheme": "probe", + "tiers": ["surface", "theme-matrix", "probe"], "viewport": {"width": 1024, "height": 768}, "settleMs": 50, "threshold": 0.1, diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 78b4af1f44416..0c09eeaec2f36 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -516,6 +516,104 @@ jobs: # job summary but don't block the merge, while the suite is observed for # flake. To promote: drop continue-on-error and add `pr-rtl` to required # checks. See apps/storybook/rtl-audit/README.md. + # Visual regression, scoped to the components a PR touched. Sibling to + # pr-a11y and pr-rtl: same build-storybook artifact, same analysis.json + # scoping, so a PR pays only for what it changed — a median PR is ~16 shots + # (~10s) against the 642 the daily release gate takes. + # + # Deeper than the daily gate where it looks: the release gate shoots ONE + # representative story per component, this shoots EVERY story of the touched + # component in EVERY theme that styles it. + # + # SKIPS ITSELF WHEN A PR IS TOO BROAD (--max-shots). A token or shared-hook + # change puts hundreds of diffs in front of a reviewer who cannot judge them + # one by one, and a per-PR check is the wrong instrument for that change — + # the daily gate reviews it against the whole baseline instead. The skip + # states its reason in the PR comment rather than vanishing. + # + # SOFT / NON-BLOCKING (continue-on-error) while the suite earns trust: the + # verdict surfaces in the job summary and the PR comment, and the release + # gate is the blocking one. To promote: drop continue-on-error. + pr-visual: + needs: [build-storybook, check-components] + if: github.event_name == 'pull_request' && needs.check-components.outputs.has_components == 'true' + runs-on: 2-core-ubuntu-arm + continue-on-error: true + permissions: + contents: read + steps: + - name: Checkout + uses: actions/checkout@v7 + + - name: Setup Node and pnpm + uses: ./.github/actions/setup + with: + install: 'false' + + - name: Download analysis artifact + uses: actions/download-artifact@v8 + with: + name: pr-analysis + + - name: Download Storybook artifact + uses: actions/download-artifact@v8 + with: + name: storybook-${{ needs.build-storybook.outputs.short_hash }} + path: apps/storybook/dist + + - name: Install Playwright + run: pnpm install --frozen-lockfile && npx playwright install chromium + + - name: Fetch visual baseline from gh-pages + run: | + set -eu + rm -rf /tmp/gh-pages + git clone --depth=1 --filter=blob:none --sparse --single-branch --branch gh-pages \ + "https://github.com/${GITHUB_REPOSITORY}.git" /tmp/gh-pages + git -C /tmp/gh-pages sparse-checkout set visual-gate/baseline + mkdir -p .visual-baseline + if [ -d /tmp/gh-pages/visual-gate/baseline ]; then + cp -r /tmp/gh-pages/visual-gate/baseline/. .visual-baseline/ + fi + + - name: Run the visual gate for the touched components + id: visual + run: | + set -eu + COMPONENTS=$(jq -r '(.newComponents + .modifiedComponents) | join(",")' analysis.json) + if [ -z "$COMPONENTS" ]; then + echo "No component changes resolved from analysis.json — nothing to shoot." + exit 0 + fi + echo "Components: $COMPONENTS" + set +e + node .github/scripts/visual-gate/gate.mjs check \ + --storybook-dir apps/storybook/dist \ + --baseline .visual-baseline \ + --out .visual-run \ + --tiers component \ + --components "$COMPONENTS" \ + --max-shots 240 \ + --no-scout \ + --summary-output visual-summary.md + code=$? + [ -f visual-summary.md ] && cat visual-summary.md >> "$GITHUB_STEP_SUMMARY" + # Exit 2 is "changed", which is a question for the reviewer, not a + # broken job. Only a crash (1) is a failure. + [ "$code" -eq 1 ] && exit 1 + exit 0 + + - name: Upload visual verdict and report + if: always() + uses: actions/upload-artifact@v7 + with: + name: visual-pr-report + path: | + .visual-run/verdict.json + .visual-run/report/ + retention-days: 7 + if-no-files-found: ignore + pr-rtl: needs: [build-storybook, check-components] if: github.event_name == 'pull_request' && needs.check-components.outputs.has_components == 'true' diff --git a/.github/workflows/pr-comment.yml b/.github/workflows/pr-comment.yml index 22aa81690d068..0fdc411ec93af 100644 --- a/.github/workflows/pr-comment.yml +++ b/.github/workflows/pr-comment.yml @@ -57,6 +57,15 @@ jobs: github-token: ${{ secrets.GITHUB_TOKEN }} continue-on-error: true + - name: Download visual verdict from the CI run + uses: actions/download-artifact@v8 + with: + name: visual-pr-report + path: visual + run-id: ${{ github.event.workflow_run.id }} + github-token: ${{ secrets.GITHUB_TOKEN }} + continue-on-error: true + - name: Generate and post PR comment uses: actions/github-script@v9 with: @@ -94,6 +103,9 @@ jobs: '.github/scripts/generate-pr-comment.js', '--analysis', 'pr-analysis/analysis.json', '--a11y', a11yPath, + ...(fs.existsSync('visual/verdict.json') + ? ['--visual', 'visual/verdict.json'] + : []), '--storybook-url', meta.storybookUrl || '', '--sandbox-url', meta.sandboxUrl || '', '--run-url', meta.runUrl || '', diff --git a/.github/workflows/release-gate.yml b/.github/workflows/release-gate.yml index 6a659b58d8423..5d499612f35d7 100644 --- a/.github/workflows/release-gate.yml +++ b/.github/workflows/release-gate.yml @@ -136,6 +136,46 @@ jobs: retention-days: 30 if-no-files-found: warn + # The assertion a pixel diff cannot make. A baseline proves a component + # looks like it did last week — including when the reason is that a theme + # override stopped applying and the broken frame was promoted as correct. + # This asserts each declared target's override actually reached the + # pixels, by equality against the probe theme's unique per-selector + # colour. No baseline, no images, and it names the target. + # + # Reported, not enforced, until the standing failures are worked down: + # today 50 targets lose to component styles because StyleX emits into + # @layer priority1-4, which sort AFTER astryx-theme. Failing the gate on + # a known systemic issue would just teach everyone to ignore it. + - name: Check theme overrides reach the pixels + id: reach + continue-on-error: true + run: | + set +e + node .github/scripts/visual-gate/gate.mjs reach \ + --storybook-dir apps/storybook/dist \ + --out .visual-run > reach-summary.txt 2>&1 + code=$? + { + echo "## Theme override reach" + echo "" + echo '```' + head -40 reach-summary.txt + echo '```' + } >> "$GITHUB_STEP_SUMMARY" + cat reach-summary.txt + [ "$code" -eq 1 ] && exit 1 + exit 0 + + - name: Upload reach report + if: always() + uses: actions/upload-artifact@v7 + with: + name: visual-reach + path: .visual-run/reach.json + retention-days: 30 + if-no-files-found: warn + a11y: name: Accessibility runs-on: 2-core-ubuntu-arm diff --git a/apps/storybook/.storybook/main.ts b/apps/storybook/.storybook/main.ts index 3c5800ea2ecae..4d9054fb9b3fb 100644 --- a/apps/storybook/.storybook/main.ts +++ b/apps/storybook/.storybook/main.ts @@ -113,6 +113,9 @@ const config: StorybookConfig = { '@astryxdesign/theme-neutral/*': [ path.join(rootDir, 'packages/themes/neutral/src/*'), ], + '@astryxdesign/theme-probe/*': [ + path.join(rootDir, 'packages/themes/probe/src/*'), + ], '@astryxdesign/theme-stone/*': [ path.join(rootDir, 'packages/themes/stone/src/*'), ], @@ -162,6 +165,10 @@ const config: StorybookConfig = { rootDir, 'packages/themes/neutral/src/source.ts', ), + '@astryxdesign/theme-probe': path.resolve( + rootDir, + 'packages/themes/probe/src/source.ts', + ), '@astryxdesign/theme-stone': path.resolve( rootDir, 'packages/themes/stone/src/source.ts', diff --git a/apps/storybook/.storybook/preview.tsx b/apps/storybook/.storybook/preview.tsx index 61e6231134016..54460eda9f4ed 100644 --- a/apps/storybook/.storybook/preview.tsx +++ b/apps/storybook/.storybook/preview.tsx @@ -12,6 +12,7 @@ import {chocolateTheme} from '@astryxdesign/theme-chocolate'; import {gothicTheme} from '@astryxdesign/theme-gothic'; import {matchaTheme} from '@astryxdesign/theme-matcha'; import {neutralTheme} from '@astryxdesign/theme-neutral'; +import {probeTheme} from '@astryxdesign/theme-probe'; import {stoneTheme} from '@astryxdesign/theme-stone'; import {y2kTheme} from '@astryxdesign/theme-y2k'; // Import the base reset stylesheet @@ -28,6 +29,10 @@ const themes = { gothic: gothicTheme, matcha: matchaTheme, neutral: neutralTheme, + // A generated test fixture, not a design: it styles every declared theming + // target so the visual gate can prove each one still paints. See + // packages/themes/probe/README.md. + probe: probeTheme, stone: stoneTheme, y2k: y2kTheme, }; @@ -117,6 +122,7 @@ const preview: Preview = { {value: 'chocolate', title: 'Chocolate', icon: 'circle'}, {value: 'gothic', title: 'Gothic', icon: 'moon'}, {value: 'matcha', title: 'Matcha', icon: 'circlehollow'}, + {value: 'probe', title: 'Probe (test fixture)', icon: 'beaker'}, {value: 'y2k', title: 'Y2K', icon: 'lightning'}, ], dynamicTitle: true, diff --git a/apps/storybook/.storybook/story-tree.test.ts b/apps/storybook/.storybook/story-tree.test.ts new file mode 100644 index 0000000000000..9b817db08c305 --- /dev/null +++ b/apps/storybook/.storybook/story-tree.test.ts @@ -0,0 +1,114 @@ +// Copyright (c) Meta Platforms, Inc. and affiliates. + +/** + * @file The story tree has one shape, and this holds it. + * + * ``` + * //(Default | Theme Sheet | …) + * /Hooks/ + * /Themes/ + * ``` + * + * Conventions that live only in a README drift: `Hooks/useClipboard` and + * `Components/ChatComposer/Custom Input` both sat outside the pattern for + * months because nothing but a reader could notice. This reads the built + * index, so it judges what Storybook actually renders rather than what the + * source appears to say. + * + * Requires a built Storybook; skips itself when there is none, because a + * fresh clone has no dist and this must not be the test that fails there. + */ + +import fs from 'node:fs'; +import path from 'node:path'; +import {fileURLToPath} from 'node:url'; + +import {describe, expect, it} from 'vitest'; + +const ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..'); +const INDEX = path.join(ROOT, 'dist', 'index.json'); + +/** The packages a story can ship from. Not categories — packages. */ +const PACKAGES = ['Core', 'Lab', 'Charts', 'Vega', 'RichText']; + +/** Groups that are deliberately not components. */ +const NON_COMPONENT_GROUPS = ['Hooks', 'Themes']; + +type StoryIndex = {entries: Record}; + +const built = fs.existsSync(INDEX); +const titles = built + ? [ + ...new Set( + Object.values( + (JSON.parse(fs.readFileSync(INDEX, 'utf8')) as StoryIndex).entries, + ).map(entry => entry.title), + ), + ].sort() + : []; + +describe.skipIf(!built)('story tree shape', () => { + it('has stories to check', () => { + expect(titles.length).toBeGreaterThan(0); + }); + + it('files every story under a package, never a category', () => { + const stray = titles.filter( + title => !PACKAGES.includes(title.split('/')[0]), + ); + expect(stray, `not under one of ${PACKAGES.join(', ')}`).toEqual([]); + }); + + it('keeps hooks under /Hooks/, not beside components', () => { + const misfiled = titles.filter(title => { + const segments = title.split('/'); + const isHook = /^use[A-Z]/.test(segments[segments.length - 1]); + return isHook && segments[1] !== 'Hooks'; + }); + expect( + misfiled, + 'a hook is not a component — file it under /Hooks/', + ).toEqual([]); + }); + + it('keeps theme-level features under /Themes/', () => { + // A component may legitimately carry "Theme" in its name, so this looks + // for the theme FEATURES the repo actually ships rather than a substring. + const features = [ + 'Theme', + 'MediaTheme', + 'MediaTheme Auto', + 'CodeTheme', + 'CodeEditorTheme', + ]; + const misfiled = titles.filter(title => { + const segments = title.split('/'); + return ( + features.includes(segments[segments.length - 1]) && + segments[1] !== 'Themes' + ); + }); + expect( + misfiled, + 'theme features are not components — file them under /Themes/', + ).toEqual([]); + }); + + it('never nests a component deeper than /', () => { + // Charts/Chrome/* and Lab/3DChart/* are deliberate sub-grouping; anything + // else three deep is a story name that leaked into the title. + const allowedGroups = [ + ...NON_COMPONENT_GROUPS, + 'Chrome', + '3DChart', + 'Chart Interactions', + ]; + const tooDeep = titles.filter(title => { + const segments = title.split('/'); + return segments.length > 2 && !allowedGroups.includes(segments[1]); + }); + expect(tooDeep, 'put the story name in the story, not the title').toEqual( + [], + ); + }); +}); diff --git a/apps/storybook/README.md b/apps/storybook/README.md index e0ee08525292a..cb8f88784f441 100644 --- a/apps/storybook/README.md +++ b/apps/storybook/README.md @@ -4,6 +4,54 @@ Storybook application for component development and visual documentation. +## How stories are organized + +One shape, so a reader can guess where anything lives and the visual gate can +find it: + +``` +//(Default | Theme Sheet | …) +/Hooks/ +/Themes/ +``` + +- **``** is `Core`, `Lab`, `Charts`, `Vega` or `RichText` — the package + the thing ships from, never a category like "Components". +- **`Default` comes first.** The simplest honest use, and the first thing a + builder sees. +- **`Theme Sheet` comes second** — every themeable target of that component, in + every variant and state its `.doc.mjs` declares, on one page. It is the + reference for theme authors and the surface the visual gate photographs under + the probe theme, which is how a theming target is proven to still reach the + pixels. A component whose sheet is missing a target has a target nothing can + verify. +- **Hooks and theme-level features are not components** and do not sit beside + them. Icon and indicator registries, `MediaTheme`, `CodeTheme` and the like + live under `Themes/`. + +### A Theme Sheet must not pin its own theme + +Render the component plainly and let the toolbar drive the theme. A story that +wraps itself in `` overrides the global, so the toolbar cannot +switch it and the visual gate can never probe it — the story becomes invisible +to exactly the testing it looks like it is helping with. + +```tsx +// Good — the toolbar (and the gate) control the theme +export const ThemeSheet: Story = { + name: 'Theme Sheet', + render: () => ( + <> + {VARIANTS.map(v => ( + + {v} + + ))} + + ), +}; +``` + | File | Role | Purpose | | -------------------------------- | ------------- | ------------------------------------------------------------- | | `.storybook/main.ts` | Config | Storybook Vite integration, preview build target, and aliases | diff --git a/apps/storybook/package.json b/apps/storybook/package.json index 7713d6612bba5..87673773834ed 100644 --- a/apps/storybook/package.json +++ b/apps/storybook/package.json @@ -18,6 +18,7 @@ "@astryxdesign/theme-gothic": "*", "@astryxdesign/theme-matcha": "*", "@astryxdesign/theme-neutral": "*", + "@astryxdesign/theme-probe": "*", "@astryxdesign/theme-stone": "*", "@astryxdesign/theme-y2k": "*", "@astryxdesign/vega": "*", diff --git a/apps/storybook/stories/ChatComposerCustomInput.stories.tsx b/apps/storybook/stories/ChatComposerCustomInput.stories.tsx index 0b81b404208b8..2c501ef2b2ee2 100644 --- a/apps/storybook/stories/ChatComposerCustomInput.stories.tsx +++ b/apps/storybook/stories/ChatComposerCustomInput.stories.tsx @@ -16,7 +16,7 @@ import {OnChangePlugin} from '@lexical/react/LexicalOnChangePlugin'; import {$getRoot, type EditorState} from 'lexical'; const meta: Meta = { - title: 'Components/ChatComposer/Custom Input', + title: 'Core/ChatComposer', component: ChatComposer, parameters: { docs: { diff --git a/apps/storybook/stories/CodeEditorTheme.stories.tsx b/apps/storybook/stories/CodeEditorTheme.stories.tsx index e1be1be9500c3..667ad978cad3d 100644 --- a/apps/storybook/stories/CodeEditorTheme.stories.tsx +++ b/apps/storybook/stories/CodeEditorTheme.stories.tsx @@ -86,7 +86,7 @@ function ThemedEditor({ } const meta: Meta = { - title: 'Lab/CodeEditorTheme', + title: 'Lab/Themes/CodeEditorTheme', parameters: { docs: { description: { diff --git a/apps/storybook/stories/CodeTheme.stories.tsx b/apps/storybook/stories/CodeTheme.stories.tsx index 560de3f332c23..c66e8bf8bee8f 100644 --- a/apps/storybook/stories/CodeTheme.stories.tsx +++ b/apps/storybook/stories/CodeTheme.stories.tsx @@ -64,7 +64,7 @@ const sampleCode = [ ].join('\n'); const meta: Meta = { - title: 'Core/CodeTheme', + title: 'Core/Themes/CodeTheme', tags: ['autodocs'], parameters: { docs: { diff --git a/apps/storybook/stories/MediaTheme.stories.tsx b/apps/storybook/stories/MediaTheme.stories.tsx index f99dc0c820ab6..9a6598d14d7b3 100644 --- a/apps/storybook/stories/MediaTheme.stories.tsx +++ b/apps/storybook/stories/MediaTheme.stories.tsx @@ -19,7 +19,7 @@ import {Card} from '@astryxdesign/core/Card'; // ============================================================================= const meta: Meta = { - title: 'Core/MediaTheme', + title: 'Core/Themes/MediaTheme', parameters: { docs: { description: { @@ -109,11 +109,7 @@ function OnLightDemo() { Content on a light surface in dark mode: text and icons become dark. - +