Repository navigation
ci: the PR report becomes one compact view: this head's preview, a modified-components table, collapsed visual changes, package size, and one not-run line - #9171
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
|
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.
|
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
📚 Storybook Preview🧪 Sandbox PreviewNo new or modified components detected. Bundle Size SummaryNo component packages changed. Published Package SizeNo published package changed size (11 measured). Download is the gzip tarball npm fetches; unpacked is what lands in Accessibility AuditStatus: Accessibility did not run on this pull request; add Generated by PR Enrichment workflow | Storybook | Sandbox | View full report |
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: the diff rewrites AST-030 FR12 (and the matching INV7 in rtl-audit-evidence) to replace ci:heavy-lanes with per-suite ci:run-visual / ci:run-a11y / ci:run-rtl labels that count only when applied with write access, and the new status table and advisory package budget have no spec claim yet. Any engineering owner, the author included, can record the decision by commenting /approve-spec <full head SHA>, and should say whether AST-030 must state the status table and package budget before they land. Independently of that decision, please:
- Land #9161 and #9165 first so this pull request carries its own ~1,300 lines instead of ~3,100.
- Stop the CI jobs table telling every pull request to add
ci:run-a11yfor the release-only Accessibility spec-test contracts, and telling a labelled pull request to add the label it already has whenbuild-storybookfailed. - Reuse one byte formatter instead of the new copies in
pr-status.jsandpackage-size.js, and drop the Perf row, which no CI job can produce (ci:run-perfdoes not exist).
[Automated review]
| { | ||
| id: 'a11y', | ||
| label: 'Accessibility', | ||
| jobs: /^(pr-a11y|Accessibility spec-test)/i, |
There was a problem hiding this comment.
Accessibility spec-test contracts (and its shards) run only for release-check (ci.yml a11y-contracts), so on every pull request this pattern makes the job table say the suite did not run and to add ci:run-a11y, even when pr-a11y ran. Dropping it here moves those jobs into the hidden count.
| jobs: /^(pr-a11y|Accessibility spec-test)/i, | ||
| }, | ||
| {id: 'rtl', label: 'RTL', jobs: /^pr-rtl/i}, | ||
| {id: 'perf', label: 'Perf', jobs: /^(perf|pr-perf)/i}, |
There was a problem hiding this comment.
No job in the CI workflow matches ^(perf|pr-perf), so the Perf row can never render, and its not-run sentence names a ci:run-perf label nothing honours. Consider removing Perf until a job and label exist.
| return parts.length > 0 ? parts.join(' · ') : 'no published package changed'; | ||
| } | ||
|
|
||
| function bytes(value) { |
There was a problem hiding this comment.
This is a third copy of the byte formatter (sizeText in generate-pr-comment.js and package-size.js), and the copies already disagree past 1 MB. One export from lib/report-text.js would keep them in 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.
cixzhang
changed the title
…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
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: no current record covers the compact PR report, publishing visual frames to an assets/pr-<n> branch with contents: write (and pruning those branches), the advisory package budget, or reading draft #9178's touched-components report. Any engineering owner, the author included, can record that decision by commenting /approve-spec <full head SHA> and saying whether AST-030 must state these first; the per-suite label rewording is already on main and needs nothing more. Independently of that decision, please:
- Rebase onto
mainand keep itsrun-labels.cjs(the branch now conflicts with the merged #9165), land #9161 first, and split out the frame publishing so this pull request carries only its own change. - Show a missing merge-base size as unmeasured instead of
(new), and render only the frames publish-frames actually committed. - Drop "full suite" from the not-run and wide-change lines, since a labelled pull request's accessibility stays scoped to changed components.
- Check the frame URL against this pull request's number and head, keep
contents: writeto a frames-only job, and share one byte formatter.
[Automated review]
|
|
||
| /** A change against main; growth in bold, so it stands out. */ | ||
| function delta(value, format) { | ||
| if (value === null) return 'new'; |
There was a problem hiding this comment.
null here also means the merge base was not measured (no size-base report, or one for another head), so every modified component then reads (new); the base report said N/A. Consider new only for analysis.newComponents or a key missing from a measured base, and — otherwise.
| if (!row.kinds.includes(kind)) return '—'; | ||
| if (!base) return 'frame unavailable'; | ||
| const key = encodeURIComponent(String(row.key ?? '')); | ||
| return `<img src="${base}${kind}/${key}.png" height="120" alt="${kind} frame">`; |
There was a problem hiding this comment.
publish-frames skips a frame that is missing, not PNG/WebP, or over 500 KB, but this still links it, so the thumbnail renders broken. Could publish-frames output the paths it committed, and this render only those?
| labels.length === 1 | ||
| ? labels[0] | ||
| : `${labels.slice(0, -1).join(', ')} or ${labels.at(-1)}`; | ||
| return `Not run on this PR: add ${list} to run ${labels.length === 1 ? 'it' : 'a full suite'}.\n\n`; |
There was a problem hiding this comment.
A label runs the suite scoped to what the change reaches, and a labelled PR's accessibility stays scoped to changed components (AST-030 FR12, FR13), so "a full suite" overstates it; "to run them" would be accurate. The same applies to the wide-change line above.
| ? ['--preview-state', previewState(storybook, sandbox)] | ||
| // Frames publish-frames.mjs put on this PR's assets branch, and only | ||
| // from raw.githubusercontent.com. | ||
| ...(/^https:\/\/raw\.githubusercontent\.com\/[A-Za-z0-9_.-]+\/[A-Za-z0-9_.-]+\/assets\/pr-\d+\/visual\/[0-9a-f]{40}\/$/.test( |
There was a problem hiding this comment.
This accepts any owner, repository, PR number and SHA, while the description says the frame URL is passed only for this head and this PR's assets branch. Comparing against assets/pr-${identity.prNumber}/visual/${identity.headSha}/ in this repository would match the claim.
| # on the pull request's assets branch (publish-frames.mjs writes only | ||
| # that branch). | ||
| actions: read | ||
| contents: write |
There was a problem hiding this comment.
Only the frames step writes, but job-level contents: write also reaches the artifact downloads and the reconcile script that parse pull-request artifacts. A separate frames job holding only this scope would keep the rest read-only.
github-actions
Bot
removed
the
needs:spec-owner-review
There was a problem hiding this comment.
This needs an engineering owner's spec decision before it can be approved: no current record covers the compact PR report (including reading the exact head's Vercel deployment), publishing visual frames to an assets/pr-<n> branch with contents: write and pruning those branches, or reading draft #9178's touched-components report. An engineering owner records that decision by commenting /approve-spec <full head SHA> on this pull request, and the author may do so as an engineering owner, saying whether AST-030 must state these first. Independently of that decision, please:
- Split out the frame publishing and pruner (and ideally the exact-head preview read) so this change stays under 2,000 authored lines.
- Give
contents: writeto a frames-only job whose checkout does not persist credentials. - Never show ✓ for a visual run that failed, and show a missing merge-base size as unmeasured rather than
(new). - Render only the frames publish-frames committed, and drop "a full suite" from the not-run line.
- Check the frame URL against this pull request's number and head, and reuse
sizeTextfromreport-text.jsinstead of new byte formatters.
[Automated review]
| (visualVerdict && visualVerdict.status !== 'skipped' | ||
| ? visualChanges.some(entry => entry.component === name) | ||
| ? 'changed' | ||
| : 'pass' |
There was a problem hiding this comment.
When the visual verdict's status is failed (shots could not be captured), this marks every component without a changed shot ✓, including one whose own shot is in failures. Could a failed verdict mark components named in failures ✗ and the rest —, so the table never shows a pass the run did not produce?
| return typeof value === 'number' && Number.isFinite(value); | ||
| } | ||
|
|
||
| function kb(bytes) { |
There was a problem hiding this comment.
report-text.js already exports sizeText, documented as shared by every size the PR report shows, and package-size.js still uses it for the job summary. Adding kb, signedSize and packed here means the same package reads 97.7KB in the summary and 97.7 kB in the comment; extending sizeText keeps one formatter.
…move to their own PR); a failed visual run never shows a pass; an unmeasured base says so; shared sizeText; shorter not-run line
|
/approve-spec 7271897 |
There was a problem hiding this comment.
Changes requested: the compact report's direction is approved at this head, but a few things need fixing before it can land.
- The pull request is 62 lines over the 2,000 authored-line limit; moving the
touched-componentsreading into #9178, which produces that artifact, brings it under. headPreviewreports "no preview" for every fork, but Vercel does build fork previews here (#9102 has one), so fork contributors lose their preview link.- The title promises collapsed visual thumbnails that this head no longer renders, so retitle it and drop the frame-URL claims and stale comments.
- The fallback report keeps its own preview wording, the wide-change line says the labels "run the full suite", and a failed Storybook build is reported as a missing
ci:run-visuallabel.
[Automated review]
| /** One line for the browser suites that did not run, with the labels that run them. */ | ||
| function notRunLine({jobs, a11yReport, visualVerdict, touched = null}) { | ||
| if (touched?.wideChange === true) | ||
| return `Wide change (${num(touched.componentCount)} components): add \`ci:run-visual\` / \`ci:run-a11y\` / \`ci:run-rtl\` to run the full suite.\n\n`; |
There was a problem hiding this comment.
AST-030 FR12 keeps a labelled pull request's accessibility scoped to changed components (and FR13 scopes labelled visual and RTL to the affected set), so "to run the full suite" overstates what these labels do. "… to run them", as the not-run line now says?
| /** | ||
| * Changed shots shown with frames, largest change first; publish-frames.mjs | ||
| * publishes the same shots in the same order. | ||
| */ |
There was a problem hiding this comment.
publish-frames.mjs is not in this head and no frames are shown any more; this comment, the @output above (lines 12-13) and the test file's header still describe thumbnails.
| * jobs was skipped by its run condition (the label was absent). A suite with | ||
| * no jobs in the run is unknown (null). | ||
| */ | ||
| function suiteRunState(suite, jobs) { |
There was a problem hiding this comment.
pr-a11y and pr-rtl also skip when build-storybook fails (needs: [build-storybook, run-labels]), and visual requires build-storybook success, so a labelled pull request with a failed Storybook build is told to add the label it already has. Name the labels only when build-storybook succeeded in jobs?
| return 'none'; | ||
| } | ||
|
|
||
| // The same preview wording as the generator's report: the branch alias |
There was a problem hiding this comment.
This is no longer the generator's wording: the fallback report still links the branch alias (and a fork-… alias for forks, which headPreview says Vercel never builds). Could the fallback read headPreview and render previewLine too, so the two report paths share one preview renderer?
…its fallback, leaves the not-run line out when the Storybook build failed, and drops the touched-components reading (it moves to its own PR)
cixzhang
changed the title
|
/approve-spec ef9b4bc |
There was a problem hiding this comment.
This looks ready to merge: an engineering owner approved the report's direction at this head, and every earlier finding is fixed (fork previews, the fallback's preview line, the not-run line after a failed Storybook build, the title, and the size). A few leftovers are worth a small follow-up and do not block:
pr-comment.yml:13-14andreport-text.js:43still describe branch-alias links and the old1.5KBformat.pr-status.js:40keeps aperfsuite that no CI job matches, andpr-preview.test.mjsstill writes atouched-components.jsonfixture nothing reads.
[Automated review]
| jobs: /^(pr-a11y|Accessibility spec-test)/i, | ||
| }, | ||
| {id: 'rtl', label: 'RTL', short: 'RTL', jobs: /^pr-rtl/i}, | ||
| {id: 'perf', label: 'Perf', short: 'Perf', jobs: /^(perf|pr-perf)/i}, |
There was a problem hiding this comment.
No CI job matches ^(perf|pr-perf), so this entry never renders today; if one ever did, the not-run line would ask for ci:run-perf, which run-labels.cjs (SUITE_LABELS) does not define. Consider dropping it until a perf job and label exist. Non-blocking.
| const touchedPath = path.join( | ||
| path.dirname(paths.analysis), | ||
| 'touched-components.json', | ||
| ); | ||
| if (touched) fs.writeFileSync(touchedPath, JSON.stringify(touched)); |
There was a problem hiding this comment.
reconcilePrComment no longer reads a touched-components report, so this fixture (and touched/touchedPath at lines 205 and 245) is dead now that the reading moved to its own PR. Non-blocking.
| * visible and the rest collapsed, most important first; the visual deltas | ||
| * with frames for the published shots only; the package line; and the one | ||
| * not-run or wide-change line. |
There was a problem hiding this comment.
This header still mentions frames for published shots and a wide-change line; neither exists at this head. Non-blocking.
Maintainer impact
Reviewers and contributors open the PR report to answer: where is this head's preview, what did the change do to each component it touches, did any browser suite flag it, and did anything people download grow. Today those answers are spread across long sections, the preview links can show an earlier push, and every suite that didn't run takes a paragraph. GitHub already lists the CI checks and CodeBunny posts its own review, so the report can be one compact view of what ran on the pull request.
Intended invariant
The report is, in order and with nothing expanded by default:
### 🔎 Preview · <sha>): Storybook and Sandbox links to this head's own Vercel deployment when Vercel reports it ready. Until then it says "⏳ Building…" and links the branch alias (the previous ready deployment). A failed build says so, and a draft shows nothing. A fork's head is looked up the same way, since Vercel builds fork previews too. The fallback report (no trusted analysis) keeps the same preview line.| Component | Size | LoC | Visual · A11y · RTL |.lab/…,richtext/…).ci:run-<suite>labels. It is left out when the Storybook build failed, because GitHub's check already shows that. Per-component results from the touched-components check, and its wide-change line, come in a follow-up PR.Change
lib/pr-status.js(new) renders all of the above.generate-pr-comment.jsis reduced to reading its inputs and composing the report; the old size, bundle, component-stat and job tables are gone.lib/pr-preview.mjs:headPreviewreads Vercel's deployment for the exact head SHA. Onlyvercel[bot]'sPreviewdeployments and*.vercel.appaddresses count.sourceRunJobsreads which suites ran.pr-comment.ymlchanges in the comment job:deployments: read, to read the preview state.Permissions: the comment job gains
deployments: read(this head's preview state) only. It writes no repository contents.Evidence
Scope
Testing
vitest run --project node .github/scripts: all pass except one Chromium-launch test this machine cannot run (rtl-audit-cli, unrelated).lib/pr-status.test.mjscovers:pr-preview.test.mjscovers: