From 10515505a4ff3095b5fa5336cb20a6003b33f11e Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 15 Jul 2026 20:34:52 +0800 Subject: [PATCH 1/7] ci(web-shell): before/after visual previews, showing only changed views MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The visual-preview bot posted the same fixed set of canned screenshots on every web-shell PR, so it could not show what a PR actually changed (a mermaid/split feature was invisible) and added noise on PRs that touch the UI only trivially. Render each scenario against BOTH the PR base (`main`) and the PR head, pixel-diff them, and post a stitched "main | this PR" composite for only the views that CHANGED. A PR with no visual impact composites nothing → "no visual change". This makes the preview feature-aware with no per-PR understanding: the diff finds exactly the surface the PR moved (and it subsumes the backend-PR noise #6959 pre-filtered, at the content level). - web-shell-visuals-compose.mjs: pixel-diff (canvas) + stitch a labelled composite; pure helpers (parseShot/isChanged/planWork) unit-tested. - web-shell-visuals.yml: also render the base — trusted `main` via pull_request.base.sha, so no secret exposure in the untrusted-PR job — then compose; composites replace the raw after-shots (same `-.png` name the publisher already expects). - publish buildComment: list composites; "no visual change" when none. Verified locally by overlaying #6881's real changes onto main + a new mermaid scenario: the compositor flagged the mermaid view 6.5% changed (its new zoom controls) and correctly skipped the unchanged transcript. --- .github/scripts/web-shell-visuals-compose.mjs | 220 ++++++++++++++++++ .../web-shell-visuals-compose.test.mjs | 78 +++++++ .github/scripts/web-shell-visuals-publish.mjs | 39 ++-- .../web-shell-visuals-publish.test.mjs | 28 ++- .github/workflows/web-shell-visuals.yml | 60 ++++- 5 files changed, 386 insertions(+), 39 deletions(-) create mode 100644 .github/scripts/web-shell-visuals-compose.mjs create mode 100644 .github/scripts/web-shell-visuals-compose.test.mjs diff --git a/.github/scripts/web-shell-visuals-compose.mjs b/.github/scripts/web-shell-visuals-compose.mjs new file mode 100644 index 00000000000..64e85e6184d --- /dev/null +++ b/.github/scripts/web-shell-visuals-compose.mjs @@ -0,0 +1,220 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +/** + * Before/after compositor for the web-shell visuals preview. + * + * Given a `before/` (rendered from the PR's merge-base, i.e. `main`) and an + * `after/` (rendered from the PR head) set of `-.png` screenshots, + * pixel-diff each pair and, for the views that actually CHANGED, stitch a + * single labelled "main (before) | this PR (after)" composite into `outDir` + * (reusing the `-.png` name so the publish step lists it as-is). + * + * Unchanged views are dropped — so a PR with no visual impact produces no + * composites (the comment then says "no visual change") and a feature PR shows + * exactly the surface it moved, with no per-PR understanding required. A view + * present only in `after/` (a scenario the PR adds) is emitted after-only and + * tagged NEW. + * + * The pure helpers (`parseShot`, `isChanged`, `planWork`) are exported and unit + * -tested in web-shell-visuals-compose.test.mjs. The browser diff+stitch runs + * as a CLI: + * node web-shell-visuals-compose.mjs + */ + +import { chromium } from '@playwright/test'; +import { mkdirSync, readdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { basename, join } from 'node:path'; +import { pathToFileURL } from 'node:url'; + +// % of pixels that must differ for a view to count as "changed". Small but +// non-zero so sub-pixel/AA jitter between two identical renders is ignored. +export const CHANGED_PCT_THRESHOLD = 0.1; +// A pixel "differs" when |ΔR|+|ΔG|+|ΔB| exceeds this (ignores imperceptible AA). +export const PER_PIXEL_DELTA = 30; +// Display width of each panel in the stitched composite. +export const PANEL_WIDTH = 560; + +/** Parse `-.png` → `{ view, theme }` or null (ignore others). */ +export function parseShot(name) { + const m = basename(String(name)).match(/^(.*)-(light|dark)\.png$/i); + return m ? { view: m[1], theme: m[2].toLowerCase() } : null; +} + +/** + * Decide whether a view changed. A view with no baseline (added by the PR) + * counts as changed so it is shown (after-only, NEW). Otherwise it is changed + * only when the differing-pixel fraction reaches the threshold. + */ +export function isChanged( + { hasBefore, changedPct }, + threshold = CHANGED_PCT_THRESHOLD, +) { + if (!hasBefore) return true; + return changedPct >= threshold; +} + +/** + * Pure plan over filenames: for each real `-.png` in `after`, + * whether a same-named baseline exists in `before`. Sorted, deterministic. + */ +export function planWork(afterNames, beforeNames) { + const before = new Set( + (beforeNames ?? []).filter((n) => parseShot(n)).map((n) => basename(n)), + ); + return (afterNames ?? []) + .filter((n) => parseShot(n)) + .map((n) => basename(n)) + .sort() + .map((name) => ({ name, hasBefore: before.has(name) })); +} + +const dataUri = (path) => + `data:image/png;base64,${readFileSync(path).toString('base64')}`; + +// Self-describing composite: the before/after labels and the diff magnitude are +// burned into the image, so it reads correctly even opened on its own. +function compositeHtml({ + view, + theme, + hasBefore, + changedPct, + beforeUri, + afterUri, +}) { + const badge = hasBefore + ? `${changedPct.toFixed(1)}% changed` + : 'new scenario'; + const beforePanel = hasBefore + ? `
main (before)
` + : ''; + return ` +
+
+ ${view} · ${theme}${hasBefore ? '' : ' · NEW'} + — ${badge} +
+
+ ${beforePanel} +
this PR (after)
+
+
`; +} + +// Differing-pixel fraction (%) between two same-size PNG data URIs, measured on +// a canvas (no native image deps). Compares over the overlapping region so a +// height change between before/after still yields a sane number. +async function diffPct(page, beforeUri, afterUri) { + return page.evaluate( + // This arrow runs in the BROWSER (page.evaluate), where Image/document are + // defined; declare them so eslint's node env doesn't flag no-undef. + /* global Image, document */ + async ([a, b, delta]) => { + const load = (src) => + new Promise((res) => { + const img = new Image(); + img.onload = () => res(img); + img.src = src; + }); + const [ia, ib] = await Promise.all([load(a), load(b)]); + const w = Math.min(ia.width, ib.width); + const h = Math.min(ia.height, ib.height); + const c = document.createElement('canvas'); + c.width = w; + c.height = h; + const ctx = c.getContext('2d', { willReadFrequently: true }); + ctx.drawImage(ia, 0, 0); + const da = ctx.getImageData(0, 0, w, h).data; + ctx.clearRect(0, 0, w, h); + ctx.drawImage(ib, 0, 0); + const db = ctx.getImageData(0, 0, w, h).data; + let diff = 0; + const n = w * h; + for (let i = 0; i < n; i++) { + const j = i * 4; + if ( + Math.abs(da[j] - db[j]) + + Math.abs(da[j + 1] - db[j + 1]) + + Math.abs(da[j + 2] - db[j + 2]) > + delta + ) + diff++; + } + return n ? (diff / n) * 100 : 0; + }, + [beforeUri, afterUri, PER_PIXEL_DELTA], + ); +} + +async function composeCli(beforeDir, afterDir, outDir) { + mkdirSync(outDir, { recursive: true }); + const list = (d) => { + try { + return readdirSync(d); + } catch { + return []; + } + }; + const work = planWork(list(afterDir), list(beforeDir)); + + const browser = await chromium.launch(); + // deviceScaleFactor:2 keeps the burned-in labels crisp on retina/GitHub zoom. + const page = await browser.newPage({ deviceScaleFactor: 2 }); + const manifest = []; + for (const { name, hasBefore } of work) { + const { view, theme } = parseShot(name); + const afterUri = dataUri(join(afterDir, name)); + const beforeUri = hasBefore ? dataUri(join(beforeDir, name)) : null; + const changedPct = hasBefore + ? await diffPct(page, beforeUri, afterUri) + : 100; + const changed = isChanged({ hasBefore, changedPct }); + manifest.push({ + name, + view, + theme, + hasBefore, + changedPct: Number(changedPct.toFixed(2)), + changed, + }); + if (!changed) continue; + await page.setContent( + compositeHtml({ + view, + theme, + hasBefore, + changedPct, + beforeUri, + afterUri, + }), + ); + await page.locator('#cap').screenshot({ path: join(outDir, name) }); + } + await browser.close(); + + writeFileSync( + join(outDir, 'manifest.json'), + JSON.stringify(manifest, null, 2), + ); + const composited = manifest.filter((e) => e.changed).length; + for (const e of manifest) { + const tag = e.changed ? (e.hasBefore ? 'CHANGED' : 'NEW ') : 'skip '; + process.stderr.write(` ${tag} ${e.name} (${e.changedPct}% diff)\n`); + } + // stdout = composited count (the workflow reads it to decide whether to post). + process.stdout.write(`${composited}\n`); +} + +if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { + const [beforeDir, afterDir, outDir] = process.argv.slice(2); + if (!beforeDir || !afterDir || !outDir) { + process.stderr.write( + 'usage: web-shell-visuals-compose.mjs \n', + ); + process.exit(2); + } + composeCli(beforeDir, afterDir, outDir); +} diff --git a/.github/scripts/web-shell-visuals-compose.test.mjs b/.github/scripts/web-shell-visuals-compose.test.mjs new file mode 100644 index 00000000000..0044ebed035 --- /dev/null +++ b/.github/scripts/web-shell-visuals-compose.test.mjs @@ -0,0 +1,78 @@ +/** + * @license + * Copyright 2025 Qwen + * SPDX-License-Identifier: Apache-2.0 + */ + +import assert from 'node:assert/strict'; +import test from 'node:test'; + +import { + CHANGED_PCT_THRESHOLD, + isChanged, + parseShot, + planWork, +} from './web-shell-visuals-compose.mjs'; + +test('parseShot extracts view + theme, is case-insensitive, and strips dirs', () => { + assert.deepEqual(parseShot('mermaid-diagram-light.png'), { + view: 'mermaid-diagram', + theme: 'light', + }); + assert.deepEqual(parseShot('permission-panel-DARK.png'), { + view: 'permission-panel', + theme: 'dark', + }); + // A view name may itself contain a dash — only the final -light/-dark splits. + assert.deepEqual(parseShot('a/b/session-transcript-dark.png'), { + view: 'session-transcript', + theme: 'dark', + }); +}); + +test('parseShot rejects non-screenshot names (gifs, manifest, themeless)', () => { + assert.equal(parseShot('model-switch.gif'), null); + assert.equal(parseShot('manifest.json'), null); + assert.equal(parseShot('mermaid-diagram.png'), null); // no -light/-dark + assert.equal(parseShot('random.png'), null); +}); + +test('isChanged: a view with no baseline (PR-added) always counts as changed', () => { + assert.equal(isChanged({ hasBefore: false, changedPct: 0 }), true); +}); + +test('isChanged: with a baseline, only meets threshold counts as changed', () => { + assert.equal(isChanged({ hasBefore: true, changedPct: 0 }), false); + assert.equal(isChanged({ hasBefore: true, changedPct: 0.05 }), false); + assert.equal( + isChanged({ hasBefore: true, changedPct: CHANGED_PCT_THRESHOLD }), + true, + ); + assert.equal(isChanged({ hasBefore: true, changedPct: 6.5 }), true); +}); + +test('planWork pairs after-shots with baseline presence, sorted, ignoring non-shots', () => { + const plan = planWork( + [ + 'mermaid-diagram-light.png', + 'mermaid-diagram-dark.png', + 'session-transcript-light.png', + 'model-switch.gif', // ignored (not a shot) + 'manifest.json', // ignored + ], + ['session-transcript-light.png', 'session-transcript-dark.png'], + ); + assert.deepEqual(plan, [ + { name: 'mermaid-diagram-dark.png', hasBefore: false }, // PR-added → NEW + { name: 'mermaid-diagram-light.png', hasBefore: false }, + { name: 'session-transcript-light.png', hasBefore: true }, // has baseline + ]); +}); + +test('planWork is robust to a missing before set (first-ever PR → all NEW)', () => { + const plan = planWork(['home-light.png', 'home-dark.png'], []); + assert.deepEqual( + plan.map((p) => p.hasBefore), + [false, false], + ); +}); diff --git a/.github/scripts/web-shell-visuals-publish.mjs b/.github/scripts/web-shell-visuals-publish.mjs index 730ed23e873..2e78f2a5fcb 100644 --- a/.github/scripts/web-shell-visuals-publish.mjs +++ b/.github/scripts/web-shell-visuals-publish.mjs @@ -136,43 +136,32 @@ export function buildComment(files, ctx = {}) { const runUrl = ctx.runUrl ?? ''; const url = (name) => `${rawBase}/${encodeURIComponent(name)}`; - const shots = files.filter((f) => /\.png$/i.test(f)); - const views = new Map(); - for (const f of shots) { - const m = f.match(/^(.*)-(light|dark)\.png$/i); - if (!m) continue; - const [, view, theme] = m; - const entry = views.get(view) || {}; - entry[theme.toLowerCase()] = f; - views.set(view, entry); - } + // Screenshots are before/after COMPOSITES (`-.png`), one per + // changed view+theme. The compositor already dropped unchanged views, so an + // empty set means "no visual change vs main". The before/after labels and the + // view name are burned into each image, so we just list them. + const shots = files.filter((f) => /\.png$/i.test(f)).sort(); const gifs = files.filter((f) => /\.gif$/i.test(f)).sort(); const out = []; out.push(''); out.push('### 🖼️ web-shell visual preview'); out.push( - `Auto-rendered from this PR head \`${esc(shortSha)}\` against a mock daemon (no real backend). Refreshes on every push.`, + `Rendered against a mock daemon (no real backend): \`main\` vs this PR head \`${esc(shortSha)}\`. Only views that **changed** are shown — refreshes on every push.`, ); out.push(''); - if (views.size > 0) { - out.push('#### Screenshots · light / dark'); - out.push(''); - out.push(''); - out.push(''); - for (const [view, pair] of [...views.entries()].sort()) { - const light = pair.light - ? `${esc(view)} light` - : '—'; - const dark = pair.dark - ? `${esc(view)} dark` - : '—'; + out.push('#### Screenshots · before / after'); + out.push(''); + if (shots.length > 0) { + for (const f of shots) { out.push( - ``, + `${esc(f.replace(/\.png$/i, ''))} before/after`, ); + out.push(''); } - out.push('
viewlightdark
${esc(pretty(view))}${light}${dark}
'); + } else { + out.push('✅ _No visual changes against `main`._'); out.push(''); } diff --git a/.github/scripts/web-shell-visuals-publish.test.mjs b/.github/scripts/web-shell-visuals-publish.test.mjs index 1eb50b9e99f..38e78da391d 100644 --- a/.github/scripts/web-shell-visuals-publish.test.mjs +++ b/.github/scripts/web-shell-visuals-publish.test.mjs @@ -109,7 +109,7 @@ test('selectImages bounds EXAMINED candidates so a junk flood cannot run forever assert.ok(warnings.some((w) => w.includes('candidate files'))); }); -test('buildComment pairs light/dark, lists gifs, labels flows, escapes, links the run', () => { +test('buildComment lists before/after composites, labels flows, escapes, links the run', () => { const body = buildComment( [ 'session-transcript-light.png', @@ -125,13 +125,17 @@ test('buildComment pairs light/dark, lists gifs, labels flows, escapes, links th assert.match(body, //); assert.match(body, /session-transcript-light\.png/); assert.match(body, /session-transcript-dark\.png/); + assert.match(body, /Only views that \*\*changed\*\* are shown/); // before/after framing assert.match(body, /model-switch\.gif/); assert.match(body, /Open the slash menu and switch model/); // flow label assert.match(body, /abc1234/); assert.match(body, /https:\/\/run\.example\/1/); - // Exactly one screenshot row: the single view with light+dark paired. - const rows = body.split('\n').filter((l) => l.startsWith(' /^/); // composites are a list, not a table }); test('buildComment does not leak Object.prototype members as flow labels', () => { @@ -142,11 +146,17 @@ test('buildComment does not leak Object.prototype members as flow labels', () => assert.match(body, /\*\*ToString\*\*/); // falls back to the prettified filename }); -test('buildComment is empty-safe and marks a missing pair with an em dash', () => { - const empty = buildComment([], {}); +test('buildComment says "no visual changes" when there are no composites', () => { + const empty = buildComment([], { shortSha: 'abc1234' }); assert.match(empty, /web-shell visual preview/); - assert.doesNotMatch(empty, //); // no screenshots section + assert.doesNotMatch(empty, /—<\/td>/); // the missing dark cell +test('buildComment lists a lone composite as one wide image (no light/dark table)', () => { + const body = buildComment(['home-light.png'], { rawBase: 'r' }); + assert.match(body, //); // composites are a flat list now + // A lone light shot no longer needs a dark-pair placeholder cell. + assert.doesNotMatch(body, /
/); }); diff --git a/.github/workflows/web-shell-visuals.yml b/.github/workflows/web-shell-visuals.yml index 0139e540cb7..c5870702d76 100644 --- a/.github/workflows/web-shell-visuals.yml +++ b/.github/workflows/web-shell-visuals.yml @@ -1,9 +1,11 @@ name: 'Web-shell Visuals' -# Auto-capture web-shell screenshots (light + dark) and short flow recordings -# for PRs that touch the web-shell UI, then hand the images to the companion -# `web-shell-visuals-publish.yml` (workflow_run) which posts them inline on the -# PR. +# For PRs that touch the web-shell UI, render its screenshots (light + dark) +# against BOTH the PR base (`main`) and the PR head, diff them, and stitch a +# "main | this PR" composite for the views that CHANGED (plus short flow +# recordings). The images are handed to the companion +# `web-shell-visuals-publish.yml` (workflow_run), which posts them inline on the +# PR. A PR with no visual impact composites nothing → "no visual change". # # Security model: this workflow BUILDS AND RENDERS untrusted PR code, so it runs # on the `pull_request` trigger (fork PRs get a read-only token and NO secrets), @@ -40,6 +42,8 @@ on: # rather than the whole tree. # The capture pipeline itself, so a workflow-only change is exercised. - '.github/workflows/web-shell-visuals.yml' + # The before/after compositor, so a compose-only change is exercised. + - '.github/scripts/web-shell-visuals-compose.mjs' permissions: contents: 'read' @@ -93,11 +97,57 @@ jobs: echo "PLAYWRIGHT_PORT=${port}" >> "${GITHUB_ENV}" echo "Using web-shell Playwright port ${port}" - - name: 'Capture screenshots and flow recordings' + - name: 'Capture screenshots and flow recordings (after / PR head)' env: WEB_SHELL_VISUALS_OUTPUT_DIR: '${{ runner.temp }}/web-shell-visuals' run: 'npm run test:e2e:visuals --workspace=packages/web-shell' + # --- Before/after: render the PR base too, then keep only CHANGED views. + # The base is trusted `main` (github.event.pull_request.base.sha), so this + # adds no secret exposure to this untrusted-PR job — it only produces more + # image bytes. A diff against the base is what makes the preview + # feature-aware without understanding the PR: a PR with no visual impact + # composites nothing, a feature PR shows exactly the surface it moved. + - name: 'Check out the PR base (before render)' + uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 + with: + ref: '${{ github.event.pull_request.base.sha }}' + path: 'base' + fetch-depth: 1 + persist-credentials: false + + - name: 'Install base dependencies' + working-directory: 'base' + run: 'npm ci --prefer-offline --no-audit --progress=false' + + # Best-effort: if the base fails to render, compose treats every after-shot + # as NEW (after-only) rather than sinking the whole preview. Playwright + # browsers are already installed on the runner and shared across checkouts. + - name: 'Capture before (base) screenshots' + working-directory: 'base' + continue-on-error: true + env: + WEB_SHELL_VISUALS_OUTPUT_DIR: '${{ runner.temp }}/web-shell-before' + run: 'npm run test:e2e:visuals --workspace=packages/web-shell' + + - name: 'Compose before/after and keep only changed views' + env: + BEFORE_DIR: '${{ runner.temp }}/web-shell-before/screenshots' + AFTER_DIR: '${{ runner.temp }}/web-shell-visuals/screenshots' + OUT_DIR: '${{ runner.temp }}/web-shell-visuals' + run: |- + set -euo pipefail + # Diff each -.png against the base render and stitch a + # labelled "main | this PR" composite for the views that changed. The + # composites REPLACE the raw after-shots as the published screenshots, + # reusing the -.png name the publish step already expects. + node .github/scripts/web-shell-visuals-compose.mjs \ + "${BEFORE_DIR}" "${AFTER_DIR}" "${OUT_DIR}/composite" + rm -f "${OUT_DIR}/composite/manifest.json" + rm -rf "${OUT_DIR}/screenshots" + mv "${OUT_DIR}/composite" "${OUT_DIR}/screenshots" + echo "Changed views composited: $(find "${OUT_DIR}/screenshots" -name '*.png' | wc -l | tr -d ' ')" + - name: 'Convert flow recordings to inline GIFs' env: OUT_DIR: '${{ runner.temp }}/web-shell-visuals' From b97515b74520fa9c6a9de1e8d3ffa6a9588cb706 Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 15 Jul 2026 22:35:40 +0800 Subject: [PATCH 2/7] =?UTF-8?q?ci(web-shell):=20address=20before/after=20r?= =?UTF-8?q?eview=20=E2=80=94=20lazy=20import,=20merge-base,=20robustness?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses the /review findings on the before/after preview: - Lazy the @playwright/test import in the compositor so the pure exports (parseShot/isChanged/planWork) load dependency-free, and wire web-shell-visuals-compose.test.mjs into the github_ci_only test step — it was never actually running in CI. (finding 1) - Diff against the MERGE-BASE, not the base-branch tip, so a PR branch behind main doesn't render others' already-landed changes reversed as this PR's diff. (finding 2) - continue-on-error on the base checkout + install so a flaky base degrades to after-only instead of sinking the job; compose likewise degrades to the raw after-shots on failure. (finding 3) - timeout 20->30 (the job ~doubled) and scope the base render to screenshots.spec.ts, skipping the discarded flow videos. (finding 4) - diffPct: add img.onerror so a corrupt/truncated baseline PNG can't hang page.evaluate to the job timeout. (finding 5) - Lower CHANGED_PCT_THRESHOLD 0.1 -> 0.02 (~205px at 1280x800) so an icon swap or one-word label change isn't classified "no change". (finding 6) - Nits: correct the stdout comment, esc() the burned-in labels, and scope the comment wording to "screenshots" (flows are always head-only). --- .github/scripts/web-shell-visuals-compose.mjs | 39 ++++++++++-- .../web-shell-visuals-compose.test.mjs | 6 +- .github/scripts/web-shell-visuals-publish.mjs | 4 +- .../web-shell-visuals-publish.test.mjs | 4 +- .github/workflows/ci.yml | 2 +- .github/workflows/web-shell-visuals.yml | 63 +++++++++++++------ 6 files changed, 87 insertions(+), 31 deletions(-) diff --git a/.github/scripts/web-shell-visuals-compose.mjs b/.github/scripts/web-shell-visuals-compose.mjs index 64e85e6184d..1181533ef96 100644 --- a/.github/scripts/web-shell-visuals-compose.mjs +++ b/.github/scripts/web-shell-visuals-compose.mjs @@ -25,14 +25,16 @@ * node web-shell-visuals-compose.mjs */ -import { chromium } from '@playwright/test'; import { mkdirSync, readdirSync, readFileSync, writeFileSync } from 'node:fs'; import { basename, join } from 'node:path'; import { pathToFileURL } from 'node:url'; -// % of pixels that must differ for a view to count as "changed". Small but -// non-zero so sub-pixel/AA jitter between two identical renders is ignored. -export const CHANGED_PCT_THRESHOLD = 0.1; +// % of pixels that must differ for a view to count as "changed". Kept low so a +// small-but-real change (an icon swap ~24×24, a one-word label) still registers +// — this is the exact failure mode the preview exists to catch. before/after +// render on the same runner + fonts with `animations: 'disabled'`, so AA jitter +// between two identical renders is ~0. At 1280×800, 0.02% ≈ 205 px. +export const CHANGED_PCT_THRESHOLD = 0.02; // A pixel "differs" when |ΔR|+|ΔG|+|ΔB| exceeds this (ignores imperceptible AA). export const PER_PIXEL_DELTA = 30; // Display width of each panel in the stitched composite. @@ -75,6 +77,17 @@ export function planWork(afterNames, beforeNames) { const dataUri = (path) => `data:image/png;base64,${readFileSync(path).toString('base64')}`; +// Minimal HTML-escape for values interpolated into the composite markup. The +// view/theme come from PR-controlled filenames and render only in the capture +// job's own Chromium (which already runs PR code — no boundary crossed), so +// this is purely hygiene, keeping that safety argument obvious. +const esc = (s) => + String(s) + .replace(/&/g, '&') + .replace(//g, '>') + .replace(/"/g, '"'); + // Self-describing composite: the before/after labels and the diff magnitude are // burned into the image, so it reads correctly even opened on its own. function compositeHtml({ @@ -94,7 +107,7 @@ function compositeHtml({ return `
- ${view} · ${theme}${hasBefore ? '' : ' · NEW'} + ${esc(view)} · ${esc(theme)}${hasBefore ? '' : ' · NEW'} — ${badge}
@@ -117,9 +130,16 @@ async function diffPct(page, beforeUri, afterUri) { new Promise((res) => { const img = new Image(); img.onload = () => res(img); + // A truncated/corrupt PNG (a best-effort base render killed + // mid-write) must still settle, or page.evaluate would hang to the + // job timeout and take the artifacts with it. + img.onerror = () => res(null); img.src = src; }); const [ia, ib] = await Promise.all([load(a), load(b)]); + // An undecodable image can't be compared → treat the view as fully + // changed (so it is shown, not silently dropped). + if (!ia || !ib) return 100; const w = Math.min(ia.width, ib.width); const h = Math.min(ia.height, ib.height); const c = document.createElement('canvas'); @@ -160,6 +180,11 @@ async function composeCli(beforeDir, afterDir, outDir) { }; const work = planWork(list(afterDir), list(beforeDir)); + // Lazy import so the pure exports (parseShot / isChanged / planWork) load + // WITHOUT @playwright/test — the dependency-free `github_ci_only` test step + // (no npm ci) imports this module for those helpers and would otherwise die + // with ERR_MODULE_NOT_FOUND on a top-level Playwright import. + const { chromium } = await import('@playwright/test'); const browser = await chromium.launch(); // deviceScaleFactor:2 keeps the burned-in labels crisp on retina/GitHub zoom. const page = await browser.newPage({ deviceScaleFactor: 2 }); @@ -204,7 +229,9 @@ async function composeCli(beforeDir, afterDir, outDir) { const tag = e.changed ? (e.hasBefore ? 'CHANGED' : 'NEW ') : 'skip '; process.stderr.write(` ${tag} ${e.name} (${e.changedPct}% diff)\n`); } - // stdout = composited count (the workflow reads it to decide whether to post). + // stdout = composited count, for logs only. Nothing parses it: the workflow + // re-counts the written composites via `find`, and the post/no-post decision + // lives in the publish workflow. process.stdout.write(`${composited}\n`); } diff --git a/.github/scripts/web-shell-visuals-compose.test.mjs b/.github/scripts/web-shell-visuals-compose.test.mjs index 0044ebed035..ebabdd9748a 100644 --- a/.github/scripts/web-shell-visuals-compose.test.mjs +++ b/.github/scripts/web-shell-visuals-compose.test.mjs @@ -43,7 +43,11 @@ test('isChanged: a view with no baseline (PR-added) always counts as changed', ( test('isChanged: with a baseline, only meets threshold counts as changed', () => { assert.equal(isChanged({ hasBefore: true, changedPct: 0 }), false); - assert.equal(isChanged({ hasBefore: true, changedPct: 0.05 }), false); + // Below the threshold (relative, so it survives a threshold retune). + assert.equal( + isChanged({ hasBefore: true, changedPct: CHANGED_PCT_THRESHOLD / 2 }), + false, + ); assert.equal( isChanged({ hasBefore: true, changedPct: CHANGED_PCT_THRESHOLD }), true, diff --git a/.github/scripts/web-shell-visuals-publish.mjs b/.github/scripts/web-shell-visuals-publish.mjs index 2e78f2a5fcb..8ad04e78a8e 100644 --- a/.github/scripts/web-shell-visuals-publish.mjs +++ b/.github/scripts/web-shell-visuals-publish.mjs @@ -147,7 +147,7 @@ export function buildComment(files, ctx = {}) { out.push(''); out.push('### 🖼️ web-shell visual preview'); out.push( - `Rendered against a mock daemon (no real backend): \`main\` vs this PR head \`${esc(shortSha)}\`. Only views that **changed** are shown — refreshes on every push.`, + `Rendered against a mock daemon (no real backend): \`main\` vs this PR head \`${esc(shortSha)}\`. Only **screenshots** that changed are shown (flows below, if any, are head-only) — refreshes on every push.`, ); out.push(''); @@ -161,7 +161,7 @@ export function buildComment(files, ctx = {}) { out.push(''); } } else { - out.push('✅ _No visual changes against `main`._'); + out.push('✅ _No screenshot changes against `main`._'); out.push(''); } diff --git a/.github/scripts/web-shell-visuals-publish.test.mjs b/.github/scripts/web-shell-visuals-publish.test.mjs index 38e78da391d..ddf099c5a87 100644 --- a/.github/scripts/web-shell-visuals-publish.test.mjs +++ b/.github/scripts/web-shell-visuals-publish.test.mjs @@ -125,7 +125,7 @@ test('buildComment lists before/after composites, labels flows, escapes, links t assert.match(body, //); assert.match(body, /session-transcript-light\.png/); assert.match(body, /session-transcript-dark\.png/); - assert.match(body, /Only views that \*\*changed\*\* are shown/); // before/after framing + assert.match(body, /Only \*\*screenshots\*\* that changed are shown/); // before/after framing assert.match(body, /model-switch\.gif/); assert.match(body, /Open the slash menu and switch model/); // flow label assert.match(body, /abc1234/); @@ -150,7 +150,7 @@ test('buildComment says "no visual changes" when there are no composites', () => const empty = buildComment([], { shortSha: 'abc1234' }); assert.match(empty, /web-shell visual preview/); assert.doesNotMatch(empty, / { diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index c5dd2843a5c..47257763592 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -212,7 +212,7 @@ jobs: node scripts/lint.js --setup node scripts/lint.js --actionlint node scripts/lint.js --yamllint - node --test .github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs + node --test .github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs # Self-hosted can't reach nodejs.org reliably; reuse the machine's Node. - name: 'Set up Node.js 22.x (hosted)' diff --git a/.github/workflows/web-shell-visuals.yml b/.github/workflows/web-shell-visuals.yml index c5870702d76..afb6429cca9 100644 --- a/.github/workflows/web-shell-visuals.yml +++ b/.github/workflows/web-shell-visuals.yml @@ -61,7 +61,8 @@ jobs: name: 'Capture web-shell visuals (ubuntu-latest, Node 22.x)' if: "${{ github.repository == 'QwenLM/qwen-code' }}" runs-on: 'ubuntu-latest' - timeout-minutes: 20 + # ~2x the original: the job now also builds + renders the merge-base arm. + timeout-minutes: 30 steps: - name: 'Checkout PR head' uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 @@ -102,33 +103,52 @@ jobs: WEB_SHELL_VISUALS_OUTPUT_DIR: '${{ runner.temp }}/web-shell-visuals' run: 'npm run test:e2e:visuals --workspace=packages/web-shell' - # --- Before/after: render the PR base too, then keep only CHANGED views. - # The base is trusted `main` (github.event.pull_request.base.sha), so this - # adds no secret exposure to this untrusted-PR job — it only produces more - # image bytes. A diff against the base is what makes the preview - # feature-aware without understanding the PR: a PR with no visual impact - # composites nothing, a feature PR shows exactly the surface it moved. - - name: 'Check out the PR base (before render)' + # --- Before/after: render the PR's MERGE-BASE too, then keep only CHANGED + # views. The merge-base — NOT the base-branch tip — is the correct "before" + # for a `head.sha` render: a PR branch behind `main` would otherwise show + # other people's already-landed web-shell changes, reversed, as this PR's + # diff. The merge-base is trusted history, so this adds no secret exposure + # to the untrusted-PR job — it only produces more image bytes. Every base + # step is best-effort (continue-on-error): a flaky base build/render + # degrades to after-only (each view tagged NEW), never sinking the preview. + - name: 'Resolve the merge-base' + id: 'mergebase' + env: + GH_TOKEN: '${{ secrets.GITHUB_TOKEN }}' + BASE_SHA: '${{ github.event.pull_request.base.sha }}' + HEAD_SHA: '${{ github.event.pull_request.head.sha }}' + run: |- + set -euo pipefail + # Read token suffices and works for fork PRs; fall back to the base tip + # if the compare API is briefly unavailable. + MB="$(gh api "repos/${GITHUB_REPOSITORY}/compare/${BASE_SHA}...${HEAD_SHA}" --jq '.merge_base_commit.sha' 2>/dev/null || true)" + echo "sha=${MB:-$BASE_SHA}" >> "${GITHUB_OUTPUT}" + echo "before (merge-base) = ${MB:-$BASE_SHA}; base tip = ${BASE_SHA}" + + - name: 'Check out the merge-base (before render)' + continue-on-error: true uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: - ref: '${{ github.event.pull_request.base.sha }}' + ref: '${{ steps.mergebase.outputs.sha }}' path: 'base' fetch-depth: 1 persist-credentials: false - name: 'Install base dependencies' + continue-on-error: true working-directory: 'base' run: 'npm ci --prefer-offline --no-audit --progress=false' - # Best-effort: if the base fails to render, compose treats every after-shot - # as NEW (after-only) rather than sinking the whole preview. Playwright - # browsers are already installed on the runner and shared across checkouts. + # Screenshots only — the base flow videos are discarded (before-videos are + # never used), so rendering flows here would just burn CI minutes. `--` + # passes the spec through to `playwright test`. Playwright browsers are + # already installed on the runner and shared across checkouts. - name: 'Capture before (base) screenshots' working-directory: 'base' continue-on-error: true env: WEB_SHELL_VISUALS_OUTPUT_DIR: '${{ runner.temp }}/web-shell-before' - run: 'npm run test:e2e:visuals --workspace=packages/web-shell' + run: 'npm run test:e2e:visuals --workspace=packages/web-shell -- screenshots.spec.ts' - name: 'Compose before/after and keep only changed views' env: @@ -141,12 +161,17 @@ jobs: # labelled "main | this PR" composite for the views that changed. The # composites REPLACE the raw after-shots as the published screenshots, # reusing the -.png name the publish step already expects. - node .github/scripts/web-shell-visuals-compose.mjs \ - "${BEFORE_DIR}" "${AFTER_DIR}" "${OUT_DIR}/composite" - rm -f "${OUT_DIR}/composite/manifest.json" - rm -rf "${OUT_DIR}/screenshots" - mv "${OUT_DIR}/composite" "${OUT_DIR}/screenshots" - echo "Changed views composited: $(find "${OUT_DIR}/screenshots" -name '*.png' | wc -l | tr -d ' ')" + if node .github/scripts/web-shell-visuals-compose.mjs \ + "${BEFORE_DIR}" "${AFTER_DIR}" "${OUT_DIR}/composite"; then + rm -f "${OUT_DIR}/composite/manifest.json" + rm -rf "${OUT_DIR}/screenshots" + mv "${OUT_DIR}/composite" "${OUT_DIR}/screenshots" + echo "Changed views composited: $(find "${OUT_DIR}/screenshots" -name '*.png' | wc -l | tr -d ' ')" + else + # Degrade rather than sink: leave the raw after-shots in + # ${OUT_DIR}/screenshots so the PR still gets a (head-only) preview. + echo "::warning::compose failed; publishing raw after-shots without a before/after." + fi - name: 'Convert flow recordings to inline GIFs' env: From ffbb52299f44aa50daa3b9e1595d3383d4a6000b Mon Sep 17 00:00:00 2001 From: wenshao Date: Wed, 15 Jul 2026 23:40:50 +0800 Subject: [PATCH 3/7] ci(web-shell): address before/after review round 2 (yiliang114) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - diffPct: a dimension change IS a visual change — comparing only the overlapping rectangle hid it (a taller viewport with unchanged top pixels read 0%). Short-circuit any size mismatch to changed. (Critical) - Composite/comment label: "PR base (before)" not "main" — the workflow also runs for release/**, whose base is not main. (Critical) - Merge-base resolve: retry the compare API, then emit an EMPTY sha and SKIP the base render (after-only) rather than falling back to the base-branch tip, which reintroduces the reversed-diff bug. (Critical) - Base steps get ids; the before render runs only when the base checkout AND install both succeeded — else base/ (nested under head) resolves node_modules up to head's and produces a hybrid before. (Critical) - Publisher: a zero-change run now UPDATES the marker comment (image-less "no screenshot changes") instead of exiting, so a prior preview's stale images + SHA do not linger. (Critical) Finding 6 (helper tests skip full CI) is a pre-existing repo-wide gap for every .github/scripts test; left for a focused follow-up. --- .github/scripts/web-shell-visuals-compose.mjs | 16 ++-- .github/scripts/web-shell-visuals-publish.mjs | 4 +- .../web-shell-visuals-publish.test.mjs | 2 +- .../workflows/web-shell-visuals-publish.yml | 81 +++++++++++-------- .github/workflows/web-shell-visuals.yml | 36 ++++++--- 5 files changed, 87 insertions(+), 52 deletions(-) diff --git a/.github/scripts/web-shell-visuals-compose.mjs b/.github/scripts/web-shell-visuals-compose.mjs index 1181533ef96..ae8c825e723 100644 --- a/.github/scripts/web-shell-visuals-compose.mjs +++ b/.github/scripts/web-shell-visuals-compose.mjs @@ -102,7 +102,7 @@ function compositeHtml({ ? `${changedPct.toFixed(1)}% changed` : 'new scenario'; const beforePanel = hasBefore - ? `
main (before)
` + ? `
PR base (before)
` : ''; return `
@@ -117,9 +117,9 @@ function compositeHtml({
`; } -// Differing-pixel fraction (%) between two same-size PNG data URIs, measured on -// a canvas (no native image deps). Compares over the overlapping region so a -// height change between before/after still yields a sane number. +// Differing-pixel fraction (%) between two PNG data URIs, measured on a canvas +// (no native image deps). A size mismatch short-circuits to 100% (a dimension +// change is itself a visual change); equal-size images compare pixel-for-pixel. async function diffPct(page, beforeUri, afterUri) { return page.evaluate( // This arrow runs in the BROWSER (page.evaluate), where Image/document are @@ -140,8 +140,12 @@ async function diffPct(page, beforeUri, afterUri) { // An undecodable image can't be compared → treat the view as fully // changed (so it is shown, not silently dropped). if (!ia || !ib) return 100; - const w = Math.min(ia.width, ib.width); - const h = Math.min(ia.height, ib.height); + // A dimension change IS a visual change: comparing only the overlapping + // rectangle would hide it (a taller viewport whose top pixels are + // unchanged would read 0%). Treat any size mismatch as fully changed. + if (ia.width !== ib.width || ia.height !== ib.height) return 100; + const w = ia.width; + const h = ia.height; const c = document.createElement('canvas'); c.width = w; c.height = h; diff --git a/.github/scripts/web-shell-visuals-publish.mjs b/.github/scripts/web-shell-visuals-publish.mjs index 8ad04e78a8e..626a21358d4 100644 --- a/.github/scripts/web-shell-visuals-publish.mjs +++ b/.github/scripts/web-shell-visuals-publish.mjs @@ -147,7 +147,7 @@ export function buildComment(files, ctx = {}) { out.push(''); out.push('### 🖼️ web-shell visual preview'); out.push( - `Rendered against a mock daemon (no real backend): \`main\` vs this PR head \`${esc(shortSha)}\`. Only **screenshots** that changed are shown (flows below, if any, are head-only) — refreshes on every push.`, + `Rendered against a mock daemon (no real backend): the PR base vs this PR head \`${esc(shortSha)}\`. Only **screenshots** that changed are shown (flows below, if any, are head-only) — refreshes on every push.`, ); out.push(''); @@ -161,7 +161,7 @@ export function buildComment(files, ctx = {}) { out.push(''); } } else { - out.push('✅ _No screenshot changes against `main`._'); + out.push('✅ _No screenshot changes against the PR base._'); out.push(''); } diff --git a/.github/scripts/web-shell-visuals-publish.test.mjs b/.github/scripts/web-shell-visuals-publish.test.mjs index ddf099c5a87..83da97ac114 100644 --- a/.github/scripts/web-shell-visuals-publish.test.mjs +++ b/.github/scripts/web-shell-visuals-publish.test.mjs @@ -150,7 +150,7 @@ test('buildComment says "no visual changes" when there are no composites', () => const empty = buildComment([], { shortSha: 'abc1234' }); assert.match(empty, /web-shell visual preview/); assert.doesNotMatch(empty, / { diff --git a/.github/workflows/web-shell-visuals-publish.yml b/.github/workflows/web-shell-visuals-publish.yml index fbf171cdbaf..6d086ef35ac 100644 --- a/.github/workflows/web-shell-visuals-publish.yml +++ b/.github/workflows/web-shell-visuals-publish.yml @@ -162,11 +162,17 @@ jobs: STAGE="${RUNNER_TEMP}/visuals-stage" rm -rf "${STAGE}" count="$(node .github/scripts/web-shell-visuals-publish.mjs stage "${ART}/screenshots" "${ART}/gifs" "${STAGE}")" + # A zero count is a MEANINGFUL "no visual change" state, not "nothing + # to do": still update the marker comment (so a prior preview's stale + # images + SHA don't linger — the comment promises to refresh on every + # push), just without hosting a new asset commit. if [ -z "${count}" ] || [ "${count}" = "0" ]; then - echo "::notice::No valid images in the artifact; nothing to publish." - exit 0 + echo "::notice::No changed screenshots; updating the marker with a no-change note (no asset commit)." + HAS_IMAGES=0 + else + echo "Staged ${count} image(s) for PR #${PR}." + HAS_IMAGES=1 fi - echo "Staged ${count} image(s) for PR #${PR}." # Re-validate right before the force-push too: a close/new-head during # download+staging must not force-push a stale snapshot (which would @@ -182,38 +188,45 @@ jobs: # (a bare `pr-assets` branch would D/F-conflict with pr-assets/*), and # the per-PR concurrency group serializes same-PR runs so the # force-push never drops a concurrent snapshot. - BRANCH="pr-assets/web-shell-visuals-${PR}" - AUTH_URL="https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" - WORK="${RUNNER_TEMP}/pr-assets" - rm -rf "${WORK}" - mkdir -p "${WORK}/imgs" - cp "${STAGE}"/* "${WORK}/imgs/" - cd "${WORK}" - git init -q - git config user.name 'qwen-code-bot' - git config user.email 'qwen-code-bot@users.noreply.github.com' - git checkout -q --orphan snapshot - git add imgs - git commit -q -m "web-shell visuals: PR #${PR} (run ${RUN_ID})" - # Bounded retry for transient push failures (network / brief lock). - # Same-PR runs are serialized by concurrency, so this never needs to - # reconcile a concurrent snapshot — a plain retry suffices. - pushed=0 - for attempt in 1 2 3; do - if git push -q --force "${AUTH_URL}" "HEAD:${BRANCH}"; then - pushed=1 - break + # Host the images ONLY when there are changed views. A no-change run + # skips the asset commit entirely; RAW_BASE stays empty and the + # comment below is image-less ("no screenshot changes"). + RAW_BASE='' + if [ "${HAS_IMAGES}" = "1" ]; then + BRANCH="pr-assets/web-shell-visuals-${PR}" + AUTH_URL="https://x-access-token:${GH_TOKEN}@github.com/${GITHUB_REPOSITORY}.git" + WORK="${RUNNER_TEMP}/pr-assets" + rm -rf "${WORK}" + mkdir -p "${WORK}/imgs" + cp "${STAGE}"/* "${WORK}/imgs/" + cd "${WORK}" + git init -q + git config user.name 'qwen-code-bot' + git config user.email 'qwen-code-bot@users.noreply.github.com' + git checkout -q --orphan snapshot + git add imgs + git commit -q -m "web-shell visuals: PR #${PR} (run ${RUN_ID})" + # Bounded retry for transient push failures (network / brief lock). + # Same-PR runs are serialized by concurrency, so this never needs to + # reconcile a concurrent snapshot — a plain retry suffices. + pushed=0 + for attempt in 1 2 3; do + if git push -q --force "${AUTH_URL}" "HEAD:${BRANCH}"; then + pushed=1 + break + fi + echo "::notice::force-push attempt ${attempt} failed; retrying." + sleep 2 + done + if [ "${pushed}" -ne 1 ]; then + echo "::error::Failed to push web-shell visuals to ${BRANCH} after retries." + exit 1 fi - echo "::notice::force-push attempt ${attempt} failed; retrying." - sleep 2 - done - if [ "${pushed}" -ne 1 ]; then - echo "::error::Failed to push web-shell visuals to ${BRANCH} after retries." - exit 1 + ASSET_SHA="$(git rev-parse HEAD)" + cd "${GITHUB_WORKSPACE}" + echo "web-shell visuals hosted on ${BRANCH} at ${ASSET_SHA}." + RAW_BASE="https://raw.githubusercontent.com/${GITHUB_REPOSITORY}/${ASSET_SHA}/imgs" fi - ASSET_SHA="$(git rev-parse HEAD)" - cd "${GITHUB_WORKSPACE}" - echo "web-shell visuals hosted on ${BRANCH} at ${ASSET_SHA}." # --- Build the comment body --------------------------------------- # Delegated to the same unit-tested script (light/dark pairing, flow @@ -221,7 +234,7 @@ jobs: BODY_FILE="${RUNNER_TEMP}/visuals-comment.md" node .github/scripts/web-shell-visuals-publish.mjs comment \ "${STAGE}" \ - "https://raw.githubusercontent.com/${GITHUB_REPOSITORY}/${ASSET_SHA}/imgs" \ + "${RAW_BASE}" \ "${SHORT_SHA}" "${RUN_URL}" "${BODY_FILE}" # --- Post or update the PR comment -------------------------------- diff --git a/.github/workflows/web-shell-visuals.yml b/.github/workflows/web-shell-visuals.yml index afb6429cca9..6dc52756372 100644 --- a/.github/workflows/web-shell-visuals.yml +++ b/.github/workflows/web-shell-visuals.yml @@ -119,13 +119,26 @@ jobs: HEAD_SHA: '${{ github.event.pull_request.head.sha }}' run: |- set -euo pipefail - # Read token suffices and works for fork PRs; fall back to the base tip - # if the compare API is briefly unavailable. - MB="$(gh api "repos/${GITHUB_REPOSITORY}/compare/${BASE_SHA}...${HEAD_SHA}" --jq '.merge_base_commit.sha' 2>/dev/null || true)" - echo "sha=${MB:-$BASE_SHA}" >> "${GITHUB_OUTPUT}" - echo "before (merge-base) = ${MB:-$BASE_SHA}; base tip = ${BASE_SHA}" + # Read token suffices and works for fork PRs. Retry a few times; if it + # STILL fails, emit an EMPTY sha so the before render is skipped + # entirely (after-only). Falling back to the base-branch tip would + # reintroduce the exact reversed-diff bug the merge-base prevents. + MB='' + for attempt in 1 2 3; do + MB="$(gh api "repos/${GITHUB_REPOSITORY}/compare/${BASE_SHA}...${HEAD_SHA}" --jq '.merge_base_commit.sha' 2>/dev/null || true)" + [ -n "${MB}" ] && break + sleep 2 + done + echo "sha=${MB}" >> "${GITHUB_OUTPUT}" + if [ -n "${MB}" ]; then + echo "before (merge-base) = ${MB}" + else + echo "::warning::could not resolve the merge-base; skipping the before render (after-only)." + fi - name: 'Check out the merge-base (before render)' + id: 'base_checkout' + if: "${{ steps.mergebase.outputs.sha != '' }}" continue-on-error: true uses: 'actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10' # v6.0.3 with: @@ -135,15 +148,20 @@ jobs: persist-credentials: false - name: 'Install base dependencies' + id: 'base_install' + if: "${{ steps.base_checkout.outcome == 'success' }}" continue-on-error: true working-directory: 'base' run: 'npm ci --prefer-offline --no-audit --progress=false' - # Screenshots only — the base flow videos are discarded (before-videos are - # never used), so rendering flows here would just burn CI minutes. `--` - # passes the spec through to `playwright test`. Playwright browsers are - # already installed on the runner and shared across checkouts. + # Run ONLY when the checkout AND install both succeeded. `base/` is nested + # under the head checkout, so if the base's own node_modules is absent (a + # failed npm ci) the render would resolve Playwright/Vite UP to the head's + # node_modules and produce a HYBRID before. A skip here → absent baseline → + # after-only. Screenshots only (base flow videos are discarded); `--` + # passes the spec through to `playwright test`; browsers are shared. - name: 'Capture before (base) screenshots' + if: "${{ steps.base_checkout.outcome == 'success' && steps.base_install.outcome == 'success' }}" working-directory: 'base' continue-on-error: true env: From cd0bdd275be7482d260c72da1e9034a69568a357 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 16 Jul 2026 00:45:56 +0800 Subject: [PATCH 4/7] ci(web-shell): close the compositor browser in a finally (leak on error) A mid-loop rejection in diffPct (evaluate timeout / CDP disconnect on a corrupt or oversized PNG) exited composeCli via the exception and skipped browser.close(), leaking a ~200 MB Chromium child for the rest of the CI job. Wrap the page + loop in try/finally so the browser always closes. Also drop the stale "main (before)" labels from the docstring (the composite/comment say "PR base" now, since the workflow also runs for release/**). --- .github/scripts/web-shell-visuals-compose.mjs | 66 ++++++++++--------- 1 file changed, 36 insertions(+), 30 deletions(-) diff --git a/.github/scripts/web-shell-visuals-compose.mjs b/.github/scripts/web-shell-visuals-compose.mjs index ae8c825e723..ec425be334c 100644 --- a/.github/scripts/web-shell-visuals-compose.mjs +++ b/.github/scripts/web-shell-visuals-compose.mjs @@ -7,10 +7,10 @@ /** * Before/after compositor for the web-shell visuals preview. * - * Given a `before/` (rendered from the PR's merge-base, i.e. `main`) and an + * Given a `before/` (rendered from the PR's merge-base — the PR base) and an * `after/` (rendered from the PR head) set of `-.png` screenshots, * pixel-diff each pair and, for the views that actually CHANGED, stitch a - * single labelled "main (before) | this PR (after)" composite into `outDir` + * single labelled "PR base (before) | this PR (after)" composite into `outDir` * (reusing the `-.png` name so the publish step lists it as-is). * * Unchanged views are dropped — so a PR with no visual impact produces no @@ -190,39 +190,45 @@ async function composeCli(beforeDir, afterDir, outDir) { // with ERR_MODULE_NOT_FOUND on a top-level Playwright import. const { chromium } = await import('@playwright/test'); const browser = await chromium.launch(); - // deviceScaleFactor:2 keeps the burned-in labels crisp on retina/GitHub zoom. - const page = await browser.newPage({ deviceScaleFactor: 2 }); const manifest = []; - for (const { name, hasBefore } of work) { - const { view, theme } = parseShot(name); - const afterUri = dataUri(join(afterDir, name)); - const beforeUri = hasBefore ? dataUri(join(beforeDir, name)) : null; - const changedPct = hasBefore - ? await diffPct(page, beforeUri, afterUri) - : 100; - const changed = isChanged({ hasBefore, changedPct }); - manifest.push({ - name, - view, - theme, - hasBefore, - changedPct: Number(changedPct.toFixed(2)), - changed, - }); - if (!changed) continue; - await page.setContent( - compositeHtml({ + // Always close the browser, even if a diff/screenshot rejects mid-loop (an + // evaluate timeout or CDP disconnect on a corrupt/oversized PNG) — otherwise + // a ~200 MB Chromium child would leak for the rest of the CI job. + try { + // deviceScaleFactor:2 keeps the burned-in labels crisp on retina/GitHub zoom. + const page = await browser.newPage({ deviceScaleFactor: 2 }); + for (const { name, hasBefore } of work) { + const { view, theme } = parseShot(name); + const afterUri = dataUri(join(afterDir, name)); + const beforeUri = hasBefore ? dataUri(join(beforeDir, name)) : null; + const changedPct = hasBefore + ? await diffPct(page, beforeUri, afterUri) + : 100; + const changed = isChanged({ hasBefore, changedPct }); + manifest.push({ + name, view, theme, hasBefore, - changedPct, - beforeUri, - afterUri, - }), - ); - await page.locator('#cap').screenshot({ path: join(outDir, name) }); + changedPct: Number(changedPct.toFixed(2)), + changed, + }); + if (!changed) continue; + await page.setContent( + compositeHtml({ + view, + theme, + hasBefore, + changedPct, + beforeUri, + afterUri, + }), + ); + await page.locator('#cap').screenshot({ path: join(outDir, name) }); + } + } finally { + await browser.close(); } - await browser.close(); writeFileSync( join(outDir, 'manifest.json'), From 6fad8a770dbedd08dece30af982ea49d98b17f90 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 16 Jul 2026 00:48:15 +0800 Subject: [PATCH 5/7] ci(web-shell): catch the compositor CLI promise for a clean exit An unhandled composeCli() rejection (e.g. a missing @playwright/test) printed an UnhandledPromiseRejectionWarning and exited without a meaningful code; add a .catch that writes the error and exits 1. --- .github/scripts/web-shell-visuals-compose.mjs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/.github/scripts/web-shell-visuals-compose.mjs b/.github/scripts/web-shell-visuals-compose.mjs index ec425be334c..ccf600dbcae 100644 --- a/.github/scripts/web-shell-visuals-compose.mjs +++ b/.github/scripts/web-shell-visuals-compose.mjs @@ -253,5 +253,10 @@ if (import.meta.url === pathToFileURL(process.argv[1] ?? '').href) { ); process.exit(2); } - composeCli(beforeDir, afterDir, outDir); + composeCli(beforeDir, afterDir, outDir).catch((err) => { + // Surface a clean diagnostic + non-zero exit instead of an + // UnhandledPromiseRejectionWarning (e.g. a missing @playwright/test). + process.stderr.write(`${err?.stack ?? err}\n`); + process.exit(1); + }); } From afc64583c56d639129cb49e8cecd47c6ded688df Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 16 Jul 2026 06:06:30 +0800 Subject: [PATCH 6/7] ci: run .github/scripts helper tests in full CI + test planWork nullish guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Finding 6: the compositor/publisher helper tests ran only in the github_ci_only profile, which a `full` PR skips (and vitest test:ci doesn't collect node:test files) — so a compositor change could pass CI without its regression tests. Run them in the full ubuntu Test job too. - Cover planWork's `?? []` guards with null/undefined inputs. --- .github/scripts/web-shell-visuals-compose.test.mjs | 9 +++++++++ .github/workflows/ci.yml | 9 +++++++++ 2 files changed, 18 insertions(+) diff --git a/.github/scripts/web-shell-visuals-compose.test.mjs b/.github/scripts/web-shell-visuals-compose.test.mjs index ebabdd9748a..8c9bb82d3e7 100644 --- a/.github/scripts/web-shell-visuals-compose.test.mjs +++ b/.github/scripts/web-shell-visuals-compose.test.mjs @@ -80,3 +80,12 @@ test('planWork is robust to a missing before set (first-ever PR → all NEW)', ( [false, false], ); }); + +test('planWork tolerates null/undefined args (exercises the ?? [] guards)', () => { + assert.deepEqual(planWork(null, null), []); + assert.deepEqual(planWork(undefined, undefined), []); + // after present, before nullish → every shot is NEW. + assert.deepEqual(planWork(['a-light.png'], null), [ + { name: 'a-light.png', hasBefore: false }, + ]); +}); diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 47257763592..d1326c57058 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -333,6 +333,15 @@ jobs: if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}" run: 'npm run check:serve-fast-path-bundle' + # The `github_ci_only` profile runs the .github/scripts helper tests, but a + # `full` PR that touches those scripts skips that path and `npm run + # test:ci` (vitest) does not collect `node:test` files — so run them here + # too, or a compositor/publisher change could pass CI without its + # regression tests. Linux-only (they're platform-independent). + - name: 'Run .github/scripts helper tests' + if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}" + run: 'node --test .github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs' + - name: 'Run tests and generate reports' id: 'unit_tests' if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}" From 2efcbc85885074dac7bf714f03741a063f4f75b5 Mon Sep 17 00:00:00 2001 From: wenshao Date: Thu, 16 Jul 2026 07:41:11 +0800 Subject: [PATCH 7/7] ci: extract HELPER_TESTS list so both CI profiles share one source of truth Round-3 F6 fix duplicated the .github/scripts node:test list across the github_ci_only and full-profile steps; a missed edit would silently drop coverage in one path. Hoist it to a workflow-level env var both reference. --- .github/workflows/ci.yml | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index d1326c57058..6b650d63263 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -46,6 +46,11 @@ env: ACTIONLINT_VERSION: '1.7.12' SHELLCHECK_VERSION: '0.11.0' YAMLLINT_VERSION: '1.35.1' + # Single source of truth for the .github/scripts node:test suites — run by + # BOTH the github_ci_only helper step and the full-profile Test step, so a + # new helper test can't be added to one path and silently dropped from the + # other. + HELPER_TESTS: '.github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs' jobs: classify_pr: @@ -212,7 +217,7 @@ jobs: node scripts/lint.js --setup node scripts/lint.js --actionlint node scripts/lint.js --yamllint - node --test .github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs + node --test ${{ env.HELPER_TESTS }} # Self-hosted can't reach nodejs.org reliably; reuse the machine's Node. - name: 'Set up Node.js 22.x (hosted)' @@ -340,7 +345,7 @@ jobs: # regression tests. Linux-only (they're platform-independent). - name: 'Run .github/scripts helper tests' if: "${{ needs.classify_pr.outputs.skip_ci != 'true' && steps.ci_profile.outputs.ci_profile == 'full' }}" - run: 'node --test .github/scripts/pr-safety-precheck.test.mjs .github/scripts/ci/classify-profile.test.mjs .github/scripts/resolve-sandbox-image.test.mjs .github/scripts/web-shell-visuals-publish.test.mjs .github/scripts/web-shell-visuals-compose.test.mjs' + run: 'node --test ${{ env.HELPER_TESTS }}' - name: 'Run tests and generate reports' id: 'unit_tests'