diff --git a/.github/scripts/web-shell-visuals-publish.mjs b/.github/scripts/web-shell-visuals-publish.mjs index 626a21358d4..e7e4b991ea4 100644 --- a/.github/scripts/web-shell-visuals-publish.mjs +++ b/.github/scripts/web-shell-visuals-publish.mjs @@ -17,7 +17,7 @@ * `buildComment`) are exported and tested. The file also runs as a CLI for the * workflow: * node web-shell-visuals-publish.mjs stage - * node web-shell-visuals-publish.mjs comment + * node web-shell-visuals-publish.mjs comment [changedPathsFile] */ import { @@ -26,6 +26,7 @@ import { mkdirSync, openSync, readdirSync, + readFileSync, readSync, statSync, writeFileSync, @@ -115,6 +116,60 @@ export function selectImages(candidates, opts = {}) { return { accepted, warnings }; } +/** + * Directories that feed the rendered bundle. Kept in sync with the `paths:` + * trigger in .github/workflows/web-shell-visuals.yml — that trigger decides + * whether we render at all; this decides whether a "nothing changed" RESULT + * deserves a second look. + */ +const RENDER_SHAPING_PREFIXES = [ + 'packages/web-shell/client/', + 'packages/webui/src/', +]; + +/** + * Extensions that change what a view LOOKS like. Deliberately narrow: a `.ts` + * hook/util/type edit routinely lands with no visual delta, and flagging those + * would train everyone to ignore the prompt. `.css` covers `.module.css`; every + * `.svg` under the render surface is a bundled UI icon (client/assets/icons), + * so a changed icon that moves no pixel is the same coverage signal. + */ +const RENDER_SHAPING_EXT = /\.(tsx|css|svg)$/i; + +/** Test + scenario code DRIVES the preview; it is not the UI under preview. */ +const NOT_PRODUCT_UI = + /(^|\/)(__tests__|__mocks__|e2e)\/|\.(test|spec)\.[jt]sx?$/i; + +/** How many paths to name before collapsing the rest into a count. */ +export const MAX_LISTED_PATHS = 8; + +/** + * Pick the changed paths that shape rendering, from the PR's full file list. + * Returns `{ files, total }` — `files` capped at `maxListed`, `total` the full + * count, so the caller can say "and N more" without re-deriving it. + * + * This exists because "no view changed" is ambiguous: it means either "this + * change genuinely has no visual effect" or "no scenario renders this UI at + * all". The second is a COVERAGE gap that has shipped silently three times + * (#7035 primary label, #7221 worktree badge, #7365 empty-state toggle), each + * time caught only because a human happened to notice the missing image. + */ +export function selectRenderShapingFiles(paths, opts = {}) { + const maxListed = opts.maxListed ?? MAX_LISTED_PATHS; + const matched = []; + for (const raw of paths ?? []) { + const p = String(raw).trim(); + if (!p) continue; + if (!RENDER_SHAPING_PREFIXES.some((prefix) => p.startsWith(prefix))) + continue; + if (!RENDER_SHAPING_EXT.test(p)) continue; + if (NOT_PRODUCT_UI.test(p)) continue; + matched.push(p); + } + matched.sort(); + return { files: matched.slice(0, maxListed), total: matched.length }; +} + /** Self-defending HTML escaping for interpolated values. */ export const esc = (s) => String(s) @@ -126,9 +181,18 @@ export const esc = (s) => export const pretty = (s) => s.replace(/[-_]+/g, ' ').replace(/\b\w/g, (c) => c.toUpperCase()); +/** + * Render a path inside a code span without letting it escape. Backticks would + * close the span (and let the rest of the path inject markdown/HTML), so they + * go; `esc` then neutralises the remainder. Both are no-ops for real paths. + */ +const codePath = (p) => `\`${esc(String(p).replace(/[`\r\n]/g, ''))}\``; + /** * Pure comment builder. `files` is the list of staged filenames (png + gif). - * `ctx` is `{ rawBase, shortSha, runUrl }`. Returns the markdown body. + * `ctx` is `{ rawBase, shortSha, runUrl, changedPaths }`, where `changedPaths` + * is the PR's full changed-file list (used only to triage an empty preview). + * Returns the markdown body. */ export function buildComment(files, ctx = {}) { const rawBase = ctx.rawBase ?? ''; @@ -161,8 +225,31 @@ export function buildComment(files, ctx = {}) { out.push(''); } } else { - out.push('✅ _No screenshot changes against the PR base._'); - out.push(''); + // An empty preview is ambiguous, so say WHICH of the two things it means. + // "No view changed" is only a clean bill of health if nothing that shapes a + // view was touched; when render-shaping files DID change, the same result + // may instead mean no scenario renders them — a coverage gap that reads as + // reassurance if we print a bare green check (see selectRenderShapingFiles). + const shaping = selectRenderShapingFiles(ctx.changedPaths); + if (shaping.total > 0) { + const noun = shaping.total === 1 ? 'file' : 'files'; + out.push( + `ℹ️ _No screenshot changed against the PR base_ — but this PR edits ${shaping.total} render-shaping ${noun}:`, + ); + out.push(''); + for (const f of shaping.files) out.push(`- ${codePath(f)}`); + if (shaping.total > shaping.files.length) { + out.push(`- _…and ${shaping.total - shaping.files.length} more._`); + } + out.push(''); + out.push( + 'Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or **no scenario renders this UI** — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to `packages/web-shell/client/e2e/visuals/screenshots.spec.ts` that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.', + ); + out.push(''); + } else { + out.push('✅ _No screenshot changes against the PR base._'); + out.push(''); + } } if (gifs.length > 0) { @@ -246,14 +333,26 @@ function stageCli(screenshotsDir, gifsDir, stageDir) { process.stdout.write(`${accepted.length}\n`); } -function commentCli(stageDir, rawBase, shortSha, runUrl, bodyFile) { +function commentCli(stageDir, rawBase, shortSha, runUrl, bodyFile, pathsFile) { let files = []; try { files = readdirSync(stageDir); } catch { // Missing stage dir → empty preview body. } - const body = buildComment(files, { rawBase, shortSha, runUrl }); + // Newline-delimited changed paths, via a file rather than argv: a PR can + // change thousands of files, and paths are attacker-influenced (fork PRs). + // Best-effort — if the workflow's API call failed the file is absent/empty, + // and the comment falls back to the plain "no screenshot changes" line. + let changedPaths = []; + if (pathsFile) { + try { + changedPaths = readFileSync(pathsFile, 'utf8').split('\n'); + } catch { + // Unreadable → treat as "unknown", not as "nothing changed". + } + } + const body = buildComment(files, { rawBase, shortSha, runUrl, changedPaths }); writeFileSync(bodyFile, body); process.stderr.write(`Comment body: ${body.split('\n').length} lines.\n`); } @@ -263,7 +362,7 @@ if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { if (cmd === 'stage') { stageCli(rest[0], rest[1], rest[2]); } else if (cmd === 'comment') { - commentCli(rest[0], rest[1], rest[2], rest[3], rest[4]); + commentCli(rest[0], rest[1], rest[2], rest[3], rest[4], rest[5]); } else { process.stderr.write(`unknown command: ${cmd ?? '(none)'}\n`); process.exit(2); diff --git a/.github/scripts/web-shell-visuals-publish.test.mjs b/.github/scripts/web-shell-visuals-publish.test.mjs index 83da97ac114..d496cc65a67 100644 --- a/.github/scripts/web-shell-visuals-publish.test.mjs +++ b/.github/scripts/web-shell-visuals-publish.test.mjs @@ -16,6 +16,7 @@ import { MAX_SCREENSHOTS, sanitizeName, selectImages, + selectRenderShapingFiles, } from './web-shell-visuals-publish.mjs'; const PNG = '89504e470d0a1a0a'; @@ -160,3 +161,125 @@ test('buildComment lists a lone composite as one wide image (no light/dark table // A lone light shot no longer needs a dark-pair placeholder cell. assert.doesNotMatch(body, //); }); + +// --- Empty-preview triage (coverage gap vs. genuinely no visual effect) --- + +test('selectRenderShapingFiles keeps rendered .tsx/.css/.svg and drops logic/test/other-package edits', () => { + const { files, total } = selectRenderShapingFiles([ + 'packages/web-shell/client/components/WelcomeScreen.tsx', + 'packages/web-shell/client/components/worktree.module.css', + 'packages/webui/src/ui/button.tsx', + 'packages/web-shell/client/assets/icons/plan.svg', + // Dropped: not a rendered extension... + 'packages/web-shell/client/hooks/useWorktree.ts', + 'packages/web-shell/client/types.d.ts', + // ...not the rendered surface... + 'packages/core/src/utils/gitDiff.ts', + 'packages/web-shell/server/routes.tsx', + 'docs/web-shell.md', + // ...or test/scenario code, which DRIVES the preview rather than being it. + 'packages/web-shell/client/e2e/visuals/screenshots.spec.ts', + 'packages/web-shell/client/components/Sidebar.test.tsx', + 'packages/web-shell/client/components/__tests__/Chip.tsx', + // Blank lines from a trailing newline in the paths file. + '', + ' ', + ]); + assert.deepEqual(files, [ + 'packages/web-shell/client/assets/icons/plan.svg', + 'packages/web-shell/client/components/WelcomeScreen.tsx', + 'packages/web-shell/client/components/worktree.module.css', + 'packages/webui/src/ui/button.tsx', + ]); + assert.equal(total, 4); +}); + +test('selectRenderShapingFiles caps the listed paths but reports the true total', () => { + const many = Array.from( + { length: 12 }, + (_, i) => `packages/web-shell/client/c/F${String(i).padStart(2, '0')}.tsx`, + ); + const { files, total } = selectRenderShapingFiles(many, { maxListed: 3 }); + assert.equal(total, 12); + assert.equal(files.length, 3); + assert.equal(files[0], 'packages/web-shell/client/c/F00.tsx'); +}); + +test('selectRenderShapingFiles tolerates a missing/undefined list', () => { + assert.deepEqual(selectRenderShapingFiles(undefined), { + files: [], + total: 0, + }); + assert.deepEqual(selectRenderShapingFiles([]), { files: [], total: 0 }); +}); + +test('buildComment flags a possible COVERAGE GAP when UI changed but no view did', () => { + const body = buildComment([], { + shortSha: 'abc1234', + changedPaths: [ + 'packages/web-shell/client/components/WelcomeScreen.tsx', + 'packages/web-shell/client/hooks/useWorktree.ts', // logic — not listed + ], + }); + // The bare green check would read as "nothing broke"; it must not appear. + assert.doesNotMatch(body, /✅/); + assert.match(body, /1 render-shaping file:/); // singular + assert.match( + body, + /`packages\/web-shell\/client\/components\/WelcomeScreen\.tsx`/, + ); + assert.doesNotMatch(body, /useWorktree\.ts/); + assert.match(body, /no scenario renders this UI/); + assert.match(body, /screenshots\.spec\.ts/); // tells you where to fix it +}); + +test('buildComment keeps the green check when only non-rendering files changed', () => { + const body = buildComment([], { + shortSha: 'abc1234', + changedPaths: [ + 'packages/web-shell/client/hooks/useWorktree.ts', + 'packages/core/src/index.ts', + ], + }); + // A logic-only PR with no visual delta is EXPECTED — prompting here would + // train everyone to ignore the prompt when it matters. + assert.match(body, /✅ _No screenshot changes against the PR base\._/); + assert.doesNotMatch(body, /coverage gap/); +}); + +test('buildComment does not triage when screenshots DID change', () => { + const body = buildComment(['home-dark.png'], { + rawBase: 'r', + changedPaths: ['packages/web-shell/client/components/WelcomeScreen.tsx'], + }); + assert.match(body, / { + const body = buildComment([], { + changedPaths: Array.from( + { length: 10 }, + (_, i) => + `packages/web-shell/client/c/F${String(i).padStart(2, '0')}.tsx`, + ), + }); + assert.match(body, /10 render-shaping files:/); // plural + assert.match(body, /_…and 2 more\._/); // 10 - MAX_LISTED_PATHS(8) + assert.equal(body.split('\n').filter((l) => /^- `/.test(l)).length, 8); +}); + +test('buildComment neutralises a path that tries to break out of its code span', () => { + const body = buildComment([], { + changedPaths: [ + 'packages/web-shell/client/``.tsx', + ], + }); + assert.doesNotMatch(body, / /^- `/.test(l)); + assert.equal(bullets.length, 1); + assert.equal((bullets[0].match(/`/g) ?? []).length, 2); +}); diff --git a/.github/workflows/web-shell-visuals-publish.yml b/.github/workflows/web-shell-visuals-publish.yml index 6d086ef35ac..489adc1dac4 100644 --- a/.github/workflows/web-shell-visuals-publish.yml +++ b/.github/workflows/web-shell-visuals-publish.yml @@ -228,6 +228,19 @@ jobs: RAW_BASE="https://raw.githubusercontent.com/${GITHUB_REPOSITORY}/${ASSET_SHA}/imgs" fi + # --- Changed paths, for triaging an EMPTY preview ------------------ + # "No view changed" means either "no visual effect" or "no scenario + # renders this UI"; the script tells those apart by looking at which + # files the PR touched (see selectRenderShapingFiles). Read here rather + # than in the render job because that one runs untrusted PR code. + # Best-effort: on API failure the list stays empty and the comment + # falls back to the plain "no screenshot changes" line. + CHANGED_PATHS_FILE="${RUNNER_TEMP}/visuals-changed-paths.txt" + : > "${CHANGED_PATHS_FILE}" + gh api "repos/${GITHUB_REPOSITORY}/pulls/${PR}/files" \ + --paginate --jq '.[].filename' > "${CHANGED_PATHS_FILE}" 2>/dev/null \ + || echo "::warning::Could not list changed files for PR #${PR}; the preview will not flag coverage gaps." + # --- Build the comment body --------------------------------------- # Delegated to the same unit-tested script (light/dark pairing, flow # labels, and HTML escaping live in web-shell-visuals-publish.mjs). @@ -235,7 +248,8 @@ jobs: node .github/scripts/web-shell-visuals-publish.mjs comment \ "${STAGE}" \ "${RAW_BASE}" \ - "${SHORT_SHA}" "${RUN_URL}" "${BODY_FILE}" + "${SHORT_SHA}" "${RUN_URL}" "${BODY_FILE}" \ + "${CHANGED_PATHS_FILE}" # --- Post or update the PR comment -------------------------------- # Dedup only against OUR OWN prior comment (bot author + marker), and