Repository navigation
ci: the PR report shows before/after/diff thumbnails for visual changes, from frames a frames-only job publishes - #9201
Conversation
…h PR, in the build job Measures every published package's npm download and unpacked size in build-storybook, after its build, and compares it with the merge base's measurement that Main now keeps for 14 days. Adds a Published Package Size section to the PR report and the job summary, and fails the job when a package's download grows more than 5% (and at least 1 KB) unless the PR description has a 'Size budget:' line.
…e ci:heavy-lanes Each label opts a pull request into one heavy suite. A run-labels gate job reads the event's labels and, for each suite label, the newest labeler's role (write, maintain or admin; fail closed), from a decision script in the merge commit's sparse checkout; each suite's owners need it and run only on their own suite's output. A label still takes effect on the next push or reopen, ci:run stays the fork gate, and the release check is unchanged. The pull-request comment names the label for a suite that did not run. AST-030, CONTRIBUTING and the visual and RTL docs name the new labels.
…ize, visual, accessibility, RTL, perf when it ran, and the CI jobs; touched components link their docs and stories
…helper; the check also follows root build inputs - A package over its budget is flagged (job summary, PR report, a workflow warning) and fails nothing, like the component touch-only flags; the 'Size budget:' line and the enforce step are gone. - The package-size merge-base step deepens once when the 50-commit fetch holds no merge base, as size-base does. - Report counts render through num(). - tsconfig.json and scripts/ are in the check's paths scope. - size-base and the package-size lookup share lib/main-artifact.cjs.
…ip reason is its own skip step
…ied-components table (name links docs, Storybook, size, LoC, Visual · A11y · RTL marks), collapsed visual thumbnails, package size lines, one not-run hint Changed shots' frames are published to the PR's assets branch by publish-frames.mjs so the report can show before/after/diff thumbnails; the touched-components report (wide-change flag included) is read from its own artifact.
…s/pr-<n>, and the daily pruner removes that branch once its PR closes
…rked .pr-report-frames); hand-pushed assets branches keep the existing rule
…move to their own PR); a failed visual run never shows a pass; an unmeasured base says so; shared sizeText; shorter not-run line
…ames-only job publishes to assets/pr-<n>; the pruner removes only branches it created once their PR closes
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
meta-cla
Bot
added
the
CLA Signed
|
AI review status for this pull request.
|
PR Analysis Report🔎 Preview ·
|
|
/approve-spec 4557523 |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Changes requested. The frames job and its trust checks look sound, but the stacked compact report needs these fixes, and this pull request should be rebased once #9171 merges so it carries only its own 830 lines.
- The report no longer shows each changed package stylesheet's gzip size before and after, which
spec:AST-080FR8 requires;analyze-pr.jsstill measures it (stylesheetGzipBytes). pr-status.jsadds a second Before / After / Diff renderer besidevisual-format.js, which is left with no caller; keep one of them.- The not-run line rebuilds the suite labels instead of using
run-labels.cjs, and already lists aci:run-perflabel that does not exist.
[Automated review]
| const baseStats = readOptionalJson('base-stats', baseStatsFile); | ||
| if (baseStats) { | ||
| analysis.baseComponentStats = baseStats.baseComponentStats || undefined; | ||
| analysis.baseBundlePackages = baseStats.baseBundlePackages || undefined; |
There was a problem hiding this comment.
baseBundlePackages is still read here, but nothing in lib/pr-status.js renders the package-stylesheet figure (bundlePackages[].stylesheetGzipBytes against baseBundlePackages). spec:AST-080 FR8 requires each changed package stylesheet's gzip size, base against head, beside the per-component CSS delta. The base report printed | @astryxdesign/core | 58.6KB | 59.6KB | +1000B | for a stylesheet growing 60,000 to 61,000 bytes; the head prints nothing. Could packagesLine add one stylesheet line per changed package?
| if (!row.kinds.includes(kind)) return '—'; | ||
| const file = `${kind}/${row.key}.png`; | ||
| if (!published.has(file)) return 'not published'; | ||
| return `<img src="${base}${kind}/${encodeURIComponent(String(row.key))}.png" height="120" alt="${kind} frame">`; |
There was a problem hiding this comment.
This is a second Before / After / Diff renderer: lib/visual-format.js buildVisualSection already builds these <img> cells from an image base (lines 83-113), and after this change nothing calls it except its own test. Please keep one: extend buildVisualSection for the compact layout and call it here, or delete visual-format.js and its test.
| return false; | ||
| }); | ||
| if (notRun.length === 0) return ''; | ||
| const labels = notRun.map(suite => `\`ci:run-${suite.id}\``); |
There was a problem hiding this comment.
The labels and display names here copy run-labels.cjs (SUITE_LABELS, SUITE_NAMES, notRunSentence, which exist for this comment's not-run sentence). The copy has already drifted: SUITES adds perf, whose ci:run-perf label run-labels.cjs does not define. Could run-labels.cjs export the combined line, or its maps, for this function to call?
|
/approve-spec 077507f |
There was a problem hiding this comment.
Changes requested. The frames job and its trust checks look sound, but two duplicated code paths need one owner each before this lands.
framesToPublishinpublish-frames.mjsre-implements the report's shot selection frombuildVisualDeltas(the order and the 12-shot cut, held in three places); export one selector frompr-status.jsand call it, so thumbnails cannot drift from the rows.- The new thumbnail cells duplicate the Before / After / Diff renderer in
visual-format.js, which now has no caller; keep one of them. - The
publish-frames.mjsheader says it runs in the comment job; it runs in theframesjob.
[Automated review]
| export function framesToPublish(verdict, limit) { | ||
| if (!verdict || verdict.status === 'skipped' || verdict.status === 'failed') | ||
| return []; | ||
| const changes = [...(verdict.changes || [])].sort( | ||
| (a, b) => Number(b.diffPixels ?? 0) - Number(a.diffPixels ?? 0), | ||
| ); | ||
| const shots = [ | ||
| ...changes.map(change => ({key: change.key, kinds: KINDS})), | ||
| ...(verdict.added || []).map(key => ({key, kinds: ['after']})), | ||
| ...(verdict.removed || []).map(key => ({key, kinds: ['before']})), | ||
| ].filter(shot => KEY.test(String(shot.key ?? ''))); | ||
| return shots.slice(0, Math.min(Number(limit) || 0, MAX_SHOTS)); |
There was a problem hiding this comment.
This repeats the row choice in buildVisualDeltas (lib/pr-status.js:295-307: changes by diffPixels, then added, then removed, cut at VISUAL_FRAME_LIMIT). The thumbnails line up only while both copies agree, and the cut now lives in three places (VISUAL_FRAME_LIMIT, MAX_SHOTS, --limit 12). Could pr-status.js export one selector that this calls?
| * already use) under `visual/<head sha>/`, marking a branch it creates with | ||
| * MARKER so only such branches are pruned, and writes `image_base` (their | ||
| * raw URL prefix) to GITHUB_OUTPUT. Writes nothing when no shot changed. | ||
| * @position The PR Comment workflow's comment job, after the visual report is |
There was a problem hiding this comment.
@position says the comment job, but this runs in the frames job, the only one with contents: write. @output also leaves out the published output (line 219).
…sual-format renderer
|
/approve-spec b7c3bb4 |
There was a problem hiding this comment.
Recommending approval: an engineering owner's /approve-spec on this head settles the direction of the frame thumbnails, the write-scoped frames job and the pruner's marker rule, and the earlier findings (the copied shot selection, the second renderer and the @position header) are fixed. Three optional cleanups remain:
--limit 12inpr-comment.ymlrestatesVISUAL_FRAME_LIMIT; dropping it leaves one owner for the 12-shot cut.- In
pr-status.js, the oldbuildVisualDeltasJSDoc now sits abovevisualShots, and the file header does not mention the thumbnails. ci-tooling-routing.test.mjschecks thatcommentskips tooling-only runs but notframes; one moreexpectpins it.
[Automated review]
| --pr "$PR_NUMBER" \ | ||
| --head-sha "$HEAD_SHA" \ | ||
| --repository "$GITHUB_REPOSITORY" \ | ||
| --limit 12 |
There was a problem hiding this comment.
Optional: this restates VISUAL_FRAME_LIMIT (pr-status.js:46). framesToPublish already clamps to it, so dropping --limit here (and the ?? 12 default in publish-frames.mjs) leaves one owner for the cut; today the two agree.
| * without it, each row names its change. | ||
| */ | ||
| function buildVisualDeltas({verdict}) { | ||
| /** |
There was a problem hiding this comment.
Optional: the JSDoc just above (lines 285-291) describes buildVisualDeltas but now sits orphaned above visualShots's own, so buildVisualDeltas (line 314) has none. Moving it down, and adding the published frames and thumbnails to the file header's @input / @output, keeps the docs accurate.
Maintainer impact
A reviewer of a visual change today opens the run's visual report to see what moved. With this, the PR report's collapsed "Visual changes" section shows Before / After / Diff thumbnails for the 12 largest changes, so the reviewer sees the change inline.
Intended invariant
framesjob is the only job in the workflow withcontents: write. It runs trusted code frommain, never checks out or runs pull-request code, and does not persist credentials in its checkout.publish-frames.mjswrites onlyrefs/heads/assets/pr-<n>. It publishes at most 12 shots, and only PNG or WebP frames of at most 500 KB each, checked by their bytes, with sanitized names..pr-report-framesmarker. The daily branch pruner deletes a markedassets/pr-<n>branch once PR #n is merged or closed. Hand-pushed assets branches (screenshots) have no marker and keep the existing rule, so their links don't break.Change
visual-gate/publish-frames.mjs(new): takes the report's own rows (visualShotsinlib/pr-status.js, so thumbnails can't drift from the rows), validates the frames, and commits them in one commit undervisual/<head sha>/. It writesimage_baseand the published frame paths to the job outputs.pr-comment.yml: the newframesjob (download, re-encode as the comment job does, publish). The comment job waits for it and passes its outputs on, and the outcome report counts it.lib/pr-preview.mjs:trustedFrameschecks the URL against this PR's number and head, and each path against<before|after|diff>/<key>.png.lib/pr-status.js:visualShots, shared by the rows and the frames, and thumbnails for published frames only.lib/visual-format.jsand its test are removed. Its Before / After / Diff renderer has had no caller since the compact report.scripts/prune-branches.sh: the marker rule forassets/pr-<n>.Evidence
Scope
Testing
vitest run --project node .github/scripts scripts/prune-branches.test.mjs: all pass except the known Chromium-launch and load-timeout tests on this machine (rtl-audit-cli,analyze-pr), which are unrelated.publish-frames.test.mjs:assets/pr-<n>, and marks only a branch it creates;pr-preview.test.mjs:<before|after|diff>/<key>.pngpaths;framesjob hascontents: write, and its checkout persists no credentials.pr-status.test.mjs: thumbnails only for published frames.prune-branches.test.mjs: a merged PR's marked branch is deleted, an open PR's is kept, and an unmarked one is not deleted even though its PR is closed.