Repository navigation
ci: the PR report reads the touched-components check's per-component results and wide-change flag, once #9178 writes them - #9217
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
…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)
…ents check, and names the labels for a wide change
|
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 2aa736e |
![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 stacked compact-report redesign drops the package stylesheet gzip size that AST-080 FR8 requires, and the new wide-change path can hide results from suites that ran. Please address these before this moves forward:
- Restore a base-against-head gzip figure for each changed package stylesheet, with a test.
- On a wide change, keep marks from the accessibility report and visual verdict, treat a
not-runtouched result as absent, and name only the suites that did not run. - Build the
ci:run-*labels fromSUITE_LABELSinrun-labels.cjs, and match touched results bykey(core/Chatandlab/Chatcollide today). - Retarget the base to #9171's branch (the patch is 2,074 lines against
main), and say in the description that the report changes only once #9178 lands.
[Automated review]
| stories ? `${name} · [Storybook](${stories})` : name, | ||
| sizeCell(row.head, row.base, row.baseMeasured), | ||
| locCell(...row.loc, row.baseMeasured), | ||
| ...(checks ? [checksCell(wide ? {} : row.results)] : []), |
There was a problem hiding this comment.
On a wide change this blanks marks that came from the accessibility report and visual verdict too, so a labelled suite's result disappears (a Card visual change renders — · — · — above the visual section that lists it). Drop only the touched-components results here and keep the fallbacks.
| if (build && ['failure', 'cancelled'].includes(build.conclusion)) return ''; | ||
| // Too wide for the touched-components check: no suite ran per component. | ||
| if (touched?.wideChange === true) | ||
| return `Wide change (${num(touched.componentCount)} components): add \`ci:run-visual\`, \`ci:run-a11y\` or \`ci:run-rtl\` to run that suite.\n\n`; |
There was a problem hiding this comment.
This line asks for all three labels even when ci:run-visual or ci:run-a11y was applied and its job ran. Name only the suites whose jobs did not run (suiteRunState), and take the labels from SUITE_LABELS in run-labels.cjs.
| const stats = analysis.componentStats || {}; | ||
| const baseStats = analysis.baseComponentStats || {}; | ||
| const byName = new Map( | ||
| (touched?.components || []).map(component => [component.name, component]), |
There was a problem hiding this comment.
Keyed by bare name: core/Chat and lab/Chat both exist, so one's results show on the other. The producer writes key; key the map by component.key.
| : ''; | ||
|
|
||
| // Build the full comment | ||
| const report = buildCompactReport({ |
There was a problem hiding this comment.
The compact report carries no package stylesheet gzip size, which AST-080 FR8 requires for every changed package stylesheet (base against head). analysis.bundlePackages[].stylesheetGzipBytes is still measured; render it and add a test.
|
/approve-spec 3590cb3 |
There was a problem hiding this comment.
Changes requested: the new reader misreports several states of the report #9178 writes, so a few fixes are needed before this lands.
- On a wide change, keep the marks of suites that ran under their labels and name only the suites that did not run (
lib/pr-status.js:296,:434-435). - Treat
'not-run'as absent so the other sources still apply, match touched results bykey(core/Chatandlab/Chatboth exist), and give the new test the producer's report shape. - Build the wide-change labels from
SUITE_LABELSinrun-labels.cjs, and reword the title so it does not promise a report change before #9178 lands.
[Automated review]
| : 'pass' | ||
| : null; | ||
| const a11y = | ||
| reported.a11y ?? |
There was a problem hiding this comment.
#9178's report writes 'not-run' under every suite key it did not cover, so this ?? never falls back to the accessibility report (the same holds for reported.visual). Treating 'not-run' as absent restores the fallback the description promises.
…wide change keeps label-run marks and names only suites that did not run; labels come from run-labels.cjs
cixzhang
changed the title
|
/approve-spec 227f46c |
There was a problem hiding this comment.
Changes requested: the earlier findings are fixed, but the not-run line still misreports two states of the report #9178 writes.
- On a narrow change the table shows the touched check's A11y and RTL marks while the line beneath says those suites did not run and asks for their labels (
lib/pr-status.js:453-457); say the touched components were checked and the labels run the full suites. - #9178 now runs
Stable visual regressionwithoutci:run-visual, sosuiteRunStatecounts visual as run and a wide change's line dropsci:run-visual(lib/pr-status.js:32); decide visual from its label-gated scout job, or keeppr-visualbehind the label in #9178.
[Automated review]
| const lead = | ||
| touched?.wideChange === true | ||
| ? `Wide change (${num(touched.componentCount)} components)` | ||
| : 'Not run on this PR'; | ||
| return `${lead}: add ${list} to run ${labels.length === 1 ? 'it' : 'them'}.\n\n`; |
There was a problem hiding this comment.
On a narrow touched report this keeps "Not run on this PR" under the touched check's A11y and RTL marks, so the comment says a suite did not run right beneath its ✓. When touched carries results and is not wide, a lead such as "Touched components only" with "to run the full suites" would match what ran.
…-gated scout and says touched components were checked on a narrow change
|
/approve-spec 765021e |
There was a problem hiding this comment.
Recommend approval: both not-run line issues from the last round are fixed, and the reader matches the report #9178 writes. The one red CI job, test-node (4/4), fails a packages/cli watch-mode test this change does not touch, so a re-run should clear it. One optional follow-up: a narrow report whose every result is not-run (for example when the touched check's audits did not run) still reads "Touched components checked"; keeping "Not run on this PR" unless some touched component has a result would avoid that.
[Automated review]
| // checked, so the labels run the full suites. | ||
| if (touched?.wideChange === true) | ||
| return `Wide change (${num(touched.componentCount)} components): add ${list} to run ${labels.length === 1 ? 'it' : 'them'}.\n\n`; | ||
| if (touched) |
There was a problem hiding this comment.
Non-blocking: this also fires for a narrow report whose every result is not-run (#9178 still uploads one when its audits did not run), so the line says the touched components were checked while the table shows no marks. Saying it only when some touched component has a result other than not-run would cover that.
|
/approve-spec 0c27793 |
There was a problem hiding this comment.
Changes requested: once #9178 lands, the Modified components table can show a visual ✓ for components nothing compared. When the touched report says visual not-run, lib/pr-status.js:222-230 falls back to pr-visual's touched-only verdict and renders a pass. That happens for a new component with no baseline and for a Lab or Charts component, and reverse-dependent rows get the same ✓. Skipping that fallback when the verdict carries context.scope: 'touched', with a test for it, fixes this; the rest matches the report #9178 writes.
[Automated review]
| own('visual') ?? | ||
| (visualVerdict && visualVerdict.status !== 'skipped' | ||
| ? // A run that failed compared nothing: never a pass. | ||
| visualVerdict.status === 'failed' | ||
| ? 'fail' | ||
| : visualChanges.some(entry => entry.component === name) | ||
| ? 'changed' | ||
| : 'pass' | ||
| : null; | ||
| : null); |
There was a problem hiding this comment.
After #9178, an unlabelled pull request's verdict here is pr-visual's touched-only run (context.scope: 'touched'). So a component the touched report marks visual not-run falls back to it and shows ✓, though it was never compared: a new component with no baseline, or a Lab or Charts component. Reverse-dependent rows the report does not list get the same ✓. Adding visualVerdict.context?.scope !== 'touched' to this condition, with a test, keeps those rows at —.
…heck did not compare
|
/approve-spec f9605af |
There was a problem hiding this comment.
Changes requested: the reader matches the report #9178 writes, and the earlier visual ✓ issue is fixed, but the wide-change line misreports shared-style changes. #9178 marks a theme or token change wide whatever its component count and records why in foundations. lib/pr-status.js:488-489 prints only the count, so such a change reads "Wide change (0 components)". Please name the shared-style reason when foundations is non-empty, and add a test with that report shape.
[Automated review]
| if (touched?.wideChange === true) | ||
| return `Wide change (${num(touched.componentCount)} components): add ${list} to run ${labels.length === 1 ? 'it' : 'them'}.\n\n`; |
There was a problem hiding this comment.
#9178 marks a change wide when it touches shared styles (themes, tokens, a shared .stylex.ts), whatever its folder count, and its wide report records those files in foundations. This line prints only componentCount, so a token-only change reads "Wide change (0 components)" and one with a single touched folder reads "(1 components)". When touched.foundations is non-empty, saying the change reaches shared styles (for example "Wide change (shared styles): add …") would keep the reason, with a test using that report shape.
|
/approve-spec c90f83f |
There was a problem hiding this comment.
Recommend approval: the wide-change line now names the shared-style files that made a change wide, and the reader matches the report #9178 writes. Two optional follow-ups: when the CI job list cannot be read, a touched-only visual verdict still drops ci:run-visual from the line (lib/pr-status.js:475), and the description's wide-line bullet still shows only the component-count form. #9178 also edits componentRows and notRunLine, so whichever of the two lands second needs a manual merge.
[Automated review]
Maintainer impact
The touched-components check (#9178) runs the visual, accessibility and RTL suites for the components a pull request touches, and records each component's result. With this, the PR report's Modified components table shows those results in its Visual · A11y · RTL marks. When a change is too wide for the check, the report says so in one line and names the labels that run the suites.
Intended invariant
touched-componentsreport for this exact head, each component's marks come from it first, matched by key (core/Chatandlab/Chatare different components). Anot-runresult counts as no result, so the accessibility report and the visual verdict still apply.wideChange), a suite run by itsci:run-*label keeps its marks. The not-run line reads "Wide change (N components): add … to run it/them" and names only the suites that did not run. Labels come fromSUITE_LABELSinrun-labels.cjs.Change
pr-comment.yml: the comment job downloads thetouched-componentsartifact.lib/pr-preview.mjs: passes the report to the generator only when itsheadShais this head.lib/pr-status.js: reads results and the wide-change flag from it; drops theperfsuite, which has no label or job.generate-pr-comment.js: a--touchedinput.Evidence
Scope
Testing
pr-status.test.mjs(fixtures use the report shape ci: a touched-components check runs visual, a11y and RTL for the components a pull request changes #9178 writes):not-runfalls back to the other reports;pr-preview.test.mjs: the report is passed only when it names the exact head.