Repository navigation
ci: a touched-components check runs visual, a11y and RTL for the components a pull request changes - #9178
Conversation
|
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🔎 Preview ·
|
| Package | Where | Signature | Usage | |
|---|---|---|---|---|
| + | core |
. · Types · interface SearchContext |
interface SearchContext<T extends SearchableItem = SearchableItem> |
import type {SearchContext} from '@astryxdesign/core' |
| + | core |
./Typeahead · Also exported from an earlier entry point · SearchContext |
SearchContext (.) |
import {SearchContext} from '@astryxdesign/core/Typeahead' |
| ~ | core |
. · Types · interface SearchSource |
search: (query: string) => Promise<T[]> | T[] → search: (query: string, context?: SearchContext<T>) => Promise<T[]> | T[] |
import type {SearchSource} from '@astryxdesign/core'; const x: SearchSource = {search: …} |
Not run on this PR: add ci:run-visual, ci:run-a11y or ci:run-rtl to run them.
CI impact: +0.2 job-min per update · pool +0.3% · time to green +1.4 min · some runners chosen by event or unknown · 11 CI action or script files changed (not measured)
| Before | After | Change | |
|---|---|---|---|
| Job-minutes per PR update | 78.6 | 78.9 | +0.2 |
| Runner pool, runner-minutes a day | 36,679 | 36,792 | +113 (+0.3%) |
| Average concurrent jobs | 25.5 | 25.5 | +0.1 |
| Time to green (median → this run) | 14.6 min | 15.9 min | +1.4 min |
| Runs a day | 466 | 466 | 0 |
Changed jobs: Touched components added: standard · Stable visual regression changed: standard or non-standard by event (2-core-ubuntu-arm)
Not measured (the jobs that use these keep their recent minutes above): .github/scripts/a11y-pr-scope.mjs, .github/scripts/ci-heavy-lanes.test.mjs, .github/scripts/ci-test-routing.test.mjs, .github/scripts/run-labels.test.mjs, .github/scripts/touched-components.mjs, .github/scripts/touched-components.test.mjs, .github/scripts/visual-gate/README.md, .github/scripts/visual-gate/gate.mjs, .github/scripts/visual-gate/publish-pr-report.mjs, .github/scripts/visual-gate/publish-pr-report.test.mjs, and 1 more.
Assumptions: before is each job's average over the last 20 pull-request runs; after keeps that for unchanged jobs and uses this run's minutes for added or changed ones, weighted by how often each changed job ran in those runs; runs a day averages the seven whole days before today; time to green compares this run with the median of those runs that ran the same jobs.
Generated by PR Enrichment workflow | View full report
|
/approve-spec 4aa2ed3 |
4aa2ed3 to
07ccaba
Compare
github-actions
Bot
removed
the
needs:spec-owner-review
07ccaba to
472d86c
Compare
github-actions
Bot
added
the
needs:spec-owner-review
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision. This pull request rewrites spec:AST-030 FR12 and DEC-7 to add a label-free pull-request check that the current FR12 and DEC-7 rule out, and no /approve-spec covers the current head. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA>, and should say whether DEC-6, DEC-4, FR12's single-owner sentence and the RTL audit evidence record's INV7 are amended to match. Whatever the decision, these need fixing:
- The
pr-analysisdownload has noif, so the check fails every spec-only, docsite-only, tooling-only and release-branch pull request. - A change inside a multi-component folder such as Table, Chat or DropdownMenu is reported as "No component changed" and nothing is audited.
- A visual capture failure reads as
passand skips the accessibility and RTL steps, and a pull request that only adds a Core component makes the visual gate exit 1 on an empty plan. - The RTL loop stops at the first package with a finding, and curated failures or all-N/A results read as
pass. - The visual step builds Core and every theme inside the 15-minute job because it does not download the built
distsartifact the waypr-visualdoes.
[Automated review]
| - name: Download analysis artifact | ||
| uses: actions/download-artifact@v8 | ||
| with: | ||
| name: pr-analysis |
There was a problem hiding this comment.
This download has no if. For spec-only, docsite-only, tooling-only and release-branch pull requests build-storybook uploads no pr-analysis, so the job fails on pull requests with no component. Gate it on steps.scope.outputs.has_components == 'true', the way pr-a11y-guards does.
| const touched = [ | ||
| ...new Set([ | ||
| ...strings(analysis.newComponentOwners), | ||
| ...strings(analysis.modifiedComponentOwners), | ||
| ]), | ||
| ].filter(owner => owner.includes('/')); |
There was a problem hiding this comment.
analyze-pr.js leaves multi-component folders (Table, Chat, DropdownMenu) in unresolvedComponentSources, so a change there gives an empty touched set and "No component changed". a11y-pr-scope.mjs already expands those sources; the touched set needs the same members.
| const mine = entry => | ||
| namesOwner( | ||
| typeof entry === 'string' ? shots[entry]?.component : entry?.component, | ||
| identity, | ||
| ); | ||
| if ((verdict.failures ?? []).some(mine)) return {result: 'fail'}; |
There was a problem hiding this comment.
The gate writes failures as {key, error} and keeps failed shots out of the manifest, so mine never matches a failure and a failed visual check reads pass. Treat a failure with no manifest component as a failure for every audited Core component.
| for PACKAGE in "${PACKAGE_LIST[@]}"; do | ||
| FILTER=$(tr ',' '\n' <<< "$OWNERS" | grep "^$PACKAGE/" | paste -sd, -) | ||
| [ -n "$FILTER" ] || continue | ||
| node apps/storybook/rtl-audit/rtl-audit.mjs \ | ||
| --storybook-dir apps/storybook/dist \ | ||
| --output "rtl-reports/$PACKAGE.json" \ | ||
| --packages "$PACKAGE" \ | ||
| --concurrency 8 \ | ||
| --filter "$FILTER" | ||
| done |
There was a problem hiding this comment.
rtl-audit.mjs exits 1 on any finding (that is why pr-rtl-shard is continue-on-error), so under bash -e the first package with a finding ends this loop and the remaining packages are never audited.
472d86c to
d6da10b
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this pull request intentionally changes spec:AST-030 FR12 and DEC-7 to add a label-free touched-components check that the current FR12 and DEC-7 rule out, and no /approve-spec covers the current head. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA>, and should settle whether FR12's retained label sentence, FR13, DEC-6, DEC-7's title, FR11/DEC-4 and the RTL audit evidence record's INV7 are amended to match. Whatever the decision, these need fixing:
pr-visual's default step sets noASTRYX_PR_HEAD_SHAorASTRYX_PR_BASE_SHA, so the PR comment rejects its verdict and thePR Commentstatus fails on unlabelled component pull requests.- The
pr-analysisdownload inpr-touched-componentshas noif, so the check fails every spec-only, docsite-only and tooling-only pull request. - The default visual step exits 1 when no touched Core component has an accepted baseline shot, as in a pull request that only adds one, which turns Stable visual regression red.
- RTL rows read
passfor all-N/A components and curated failures, and the per-package loop stops at the first package with a finding. - A visual failure is recorded as
not-run, and a change inside a multi-component folder such as Table or Chat is reported as "No component changed".
[Automated review]
| - name: Visual check for the default touched components | ||
| if: steps.touched.outputs.has_components == 'true' && steps.touched.outputs.visual_components != '' | ||
| env: | ||
| COMPONENTS: ${{ steps.touched.outputs.visual_components }} |
There was a problem hiding this comment.
This step sets no ASTRYX_PR_HEAD_SHA or ASTRYX_PR_BASE_SHA (the labelled step does at L2410-L2411), so the verdict records null head and base and publish-pr-report.mjs refuses it; pr-comment.yml then reports the PR Comment status as failed. Pass the same two variables here.
| set -e | ||
| case "$code" in | ||
| 0|2) ;; | ||
| 3) rm -f .visual-run/verdict.json ;; |
There was a problem hiding this comment.
The gate exits 3 only when --dependents is passed (gate.mjs:243-253), so this branch never runs. When no touched Core component has an accepted baseline shot, as in a pull request that only adds one, the plan is empty and the gate exits 1 here, failing pr-visual.
| ref: ${{ github.event.pull_request.head.sha }} | ||
| persist-credentials: false | ||
|
|
||
| - name: Download analysis artifact |
There was a problem hiding this comment.
This download has no if. Spec-only, docsite-only and tooling-only pull requests upload no pr-analysis, so the job fails on them. Gate it on steps.scope.outputs.has_components == 'true', as every other analysis download in this file is gated.
| # The visual result comes from pr-visual, the one visual owner: its | ||
| # default touched check, or its full check under ci:run-visual. | ||
| - name: Download pr-visual's verdict | ||
| if: steps.touched.outputs.has_components == 'true' && needs.pr-visual.result == 'success' |
There was a problem hiding this comment.
A failed visual verdict makes pr-visual exit 1, so this never downloads a failure and every row reads visual not-run. With no always(), an RTL finding in the step above also skips it.
| const mine = results.filter(result => namesOwner(result.component, identity)); | ||
| if (mine.length === 0) return {result: 'not-run'}; | ||
| const failed = mine.filter( | ||
| result => result.verdict !== 'pass' && result.verdict !== 'N-A', |
There was a problem hiding this comment.
N-A counts as passing and the curated section is never read, so an all-N/A component or a curated not-RTL card reads pass (rtl-audit-evidence INV3, INV4). report.coverage already carries each component's measured, verified-N/A or gap status.
| const touched = [ | ||
| ...new Set([ | ||
| ...strings(analysis.newComponentOwners), | ||
| ...strings(analysis.modifiedComponentOwners), |
There was a problem hiding this comment.
analyze-pr.js leaves multi-component folders (Table, Chat, DropdownMenu) in unresolvedComponentSources, so a change there gives an empty touched set and "No component changed". resolvePrA11yComponents in a11y-pr-scope.mjs already expands those folders.
…the components a pull request changes Every pull request without a ci:run-<suite> label runs one default check for only the components it touches, in every package, against its Storybook build. The visual part is pr-visual's new default mode, a focused check of the touched Core components that never escalates to the release plan, so pr-visual stays the one visual owner (AST-030 FR12). The accessibility and RTL parts run in pr-touched-components on one standard arm runner, which folds in pr-visual's verdict and writes touched-components.json: one row per component with each suite's pass/fail/changed/not-run result, or the wide flag and count. A wide change (more than five components, or shared styles: themes, tokens, shared .stylex.ts) skips it, and nothing starts a full suite on its own. AST-030 FR12 and DEC-7 record the check.
99a69ae to
fb1d0e3
Compare
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this pull request intentionally changes spec:AST-030 FR12 and DEC-7 to add a label-free touched-components check that the current FR12 and DEC-7 rule out, and no /approve-spec covers the current head. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA>, and should settle whether FR12's retained label sentence, FR13, DEC-6, DEC-7's title, FR11/DEC-4 and the RTL audit evidence record's INV7 are amended to match. Whatever the decision, these need fixing:
pr-visual's default step sets noASTRYX_PR_HEAD_SHAorASTRYX_PR_BASE_SHA, so the PR comment rejects its verdict and thePR Commentstatus fails on unlabelled component pull requests.- The
pr-analysisdownload inpr-touched-componentshas noif, so the check fails every spec-only, docsite-only and tooling-only pull request. - The default visual step exits 1 when no touched Core component has an accepted baseline shot, as in a pull request that only adds one.
- A visual failure is recorded as
not-run, a curated RTL failure aspass, and a change inside Chat or Table as "No component changed". - With
pr-visualrunning on every pull request, the PR comment no longer tells anyone to addci:run-visual, wide changes included.
[Automated review]
| if: >- | ||
| (github.event_name != 'pull_request' || !github.event.pull_request.draft && (github.event.pull_request.head.repo.full_name == github.repository || contains(github.event.pull_request.labels.*.name, 'ci:run'))) && (!cancelled() && | ||
| (((github.event_name == 'pull_request' && needs.run-labels.outputs.visual == 'true') && | ||
| ((github.event_name == 'pull_request' && |
There was a problem hiding this comment.
With the label clause gone, Stable visual regression now succeeds on every unlabelled pull request, including a wide change or one with no Core component where nothing is compared. On main, pr-status.js counts visual as run whenever this job is not skipped (suiteRunState), so the PR comment's "Not run on this PR" line stops naming ci:run-visual. Have the comment treat visual as not run when no visual verdict was published.
| # The visual result comes from pr-visual, the one visual owner: its | ||
| # default touched check, or its full check under ci:run-visual. | ||
| - name: Download pr-visual's verdict | ||
| if: always() && steps.touched.outputs.has_components == 'true' && needs.pr-visual.result == 'success' |
There was a problem hiding this comment.
A failed verdict makes the gate exit 1 (gate.mjs:629), so pr-visual fails and this download never runs: the row reads not-run. Download when needs.pr-visual.result != 'skipped', and treat status failed as fail in visualResult, since the gate writes failures as {key, error} with no component.
| that suite from the next run. When the accessibility audit did not run, the | ||
| pull-request comment names `ci:run-a11y`. | ||
| - **Every pull request runs a touched-components check** (visual, | ||
| accessibility and RTL for only the components it changes) on one standard |
There was a problem hiding this comment.
The visual part runs in pr-visual, on its own runner, before pr-touched-components starts, so the check uses two runners; "on one standard runner" describes only the accessibility and RTL job.
pr-visual's default step sets the PR head and base and marks its verdict scope: touched, so the PR comment accepts it and names ci:run-visual; a wide change writes a skipped verdict that does the same. Only touched components with an accepted baseline shot are compared. pr-touched-components skips the analysis for spec-only, docsite-only and tooling-only changes, reads a failed pr-visual's verdict (and reports a failed visual job as fail), counts curated RTL cards, and expands a changed multi-component folder to its members, counted as one component. AST-030 FR11/DEC-4 record the advisory job; FR12, FR13, DEC-6, the DEC-7 title and rtl-audit-evidence INV7 match.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this pull request intentionally rewrites spec:AST-030 FR11, FR12, FR13, DEC-4, DEC-6 and DEC-7 and the RTL audit evidence record's INV7 to add a label-free touched-components check that the current FR12 and DEC-7 rule out, and no /approve-spec covers the current head. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA>, and should say whether DEC-7's opening sentence and DEC-5's guard sentence are amended to match. Whatever the decision, these need fixing:
- The branch conflicts with
main, which deletedlib/visual-format.js;main's comment builder (lib/pr-status.js) would then mark every sized component visual ✓ after a touched-only check and stop namingci:run-visual, so it needs to learn the touched scope. - The RTL audit step has no
always(), so a failed accessibility audit skips it and both suites readnot-runinstead offail. - CONTRIBUTING and DEC-7 say the whole check runs on one standard runner, but its visual part runs in
pr-visualon a second runner.
[Automated review]
| --components "$COMPONENTS" | ||
|
|
||
| - name: RTL audit for the touched components | ||
| if: steps.touched.outputs.has_components == 'true' && steps.touched.outputs.rtl_packages != '' |
There was a problem hiding this comment.
Without always() this step is skipped whenever the accessibility step fails (for example accessibility-audit.js throwing "No owned Storybook stories resolved"), and the report then records both accessibility and RTL as not-run. Prefix it with always() &&, and pass the accessibility step's outcome to report so a crashed audit reads fail.
| verdict?.context?.scope === 'touched' && | ||
| verdict.status !== 'skipped' | ||
| ) { | ||
| return `${section}Only the touched components were checked; add \`ci:run-visual\` to run the full suite.\n\n`; |
There was a problem hiding this comment.
main deleted this file (#9201) and now builds the comment in lib/pr-status.js, so this line never reaches a reader and the branch conflicts. On main, notRunLine stops naming ci:run-visual once pr-visual runs on every pull request, and componentRows marks every sized component visual ✓ after a touched-only pass; the touched scope has to be read there.
| that suite from the next run. When the accessibility audit did not run, the | ||
| pull-request comment names `ci:run-a11y`. | ||
| - **Every pull request runs a touched-components check** (visual, | ||
| accessibility and RTL for only the components it changes) on one standard |
There was a problem hiding this comment.
The visual part runs in pr-visual, a separate job on its own runner, before pr-touched-components starts, so this is two standard runners, as FR12 already says.
| @@ -479,7 +507,10 @@ the same exact-commit `release-check` the release gate uses, and before a cut; | |||
| a pull request runs each only when it opts in with that suite's `ci:run-visual`, `ci:run-a11y` or `ci:run-rtl` label. Main-push checks are throttled: one runs at a time, a | |||
There was a problem hiding this comment.
This opening still says a pull request runs each suite only with its label, beside FR12's label-free pr-visual mode; it needs "at its opted-in scope" like FR12. DEC-5's "PR accessibility … retain[s] the … guards" sentence has the same gap for the touched job's guard-less audit.
…-check # Conflicts: # .github/scripts/lib/visual-format.js # .github/scripts/lib/visual-format.test.mjs
…er a failed a11y audit Main's status table (pr-status.js) replaced visual-format.js. A touched-only visual verdict now gives a result only to the components it compared (its published context carries them) and the table names ci:run-visual, with its own line for a wide change. The touched job's RTL audit runs even when the accessibility audit failed. CONTRIBUTING and AST-030 DEC-7 say where each part runs; DEC-7's opening and DEC-5's guard sentence match the check.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this pull request intentionally rewrites spec:AST-030 FR11, FR12, FR13 and DEC-4 to DEC-7 and the RTL audit evidence record's INV7 to add a label-free touched-components check, with its five-component and shared-style skip rule, that the current FR12 and DEC-7 rule out, and no /approve-spec covers the current head. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA>. Whatever the decision, these need fixing:
- When
pr-visual's default mode compares nothing, as for a behaviour-only hook change, the PR comment stops namingci:run-visualand reads as if visual ran. touched-components.jsonrecords a crashed accessibility audit, and an RTL coverage gap, asnot-run("no RTL check applies") instead of a failure or a gap.expandFolderOwnerscopies the folder expansion ina11y-pr-scope.mjsand has already drifted from it; both should call one shared helper.- The description still says
pr-touched-componentsruns the visual check in every package on one runner, but visual runs inpr-visual, for Core only.
[Automated review]
| if (notRun.length === 0) return ''; | ||
| // pr-visual's default check ran, but compared only the touched components | ||
| // (or nothing, for a wide change). | ||
| const touchedVisual = |
There was a problem hiding this comment.
pr-visual now runs on every pull request, so suiteRunState counts visual as run even when the default mode published no verdict (a behaviour-only hook change, or no touched Core component with a baseline shot). Then neither this line nor the not-run list names ci:run-visual. Keep naming it when no visual verdict was published, or have the default step publish a touched-scope verdict that says nothing was compared.
|
|
||
| /** The accessibility result for one owner: new violations fail it. */ | ||
| export function a11yResult(identity, report, newViolations) { | ||
| if (report == null) return {result: 'not-run'}; |
There was a problem hiding this comment.
A thrown accessibility-audit.js or a11y-pr-scope.mjs leaves no report, so a crashed audit reads not-run while the job goes red. Pass the accessibility step's outcome to report and read a failure with no report as fail, as visualResult already does for a failed visual job.
| ]); | ||
| const mine = results.filter(result => namesOwner(result.component, identity)); | ||
| if (mine.length === 0) return {result: 'not-run'}; | ||
| if (mine.every(result => result.verdict === 'N-A')) { |
There was a problem hiding this comment.
The report's coverage rollup already says whether an all-N/A component is verified-na or a coverage-gap (and rtl-audit.mjs exits 1 for a gap). Reading only the sections labels an unverified gap "no RTL check applies" (rtl-audit-evidence INV4, INV5). Read coverage.results[].status and keep this detail for verified-na.
| * expands only the changed folder, as the accessibility scope does, without | ||
| * reading Storybook. | ||
| */ | ||
| export function expandFolderOwners(sources, repoRoot = process.cwd()) { |
There was a problem hiding this comment.
This repeats resolvePrA11yComponents' folder expansion (a11y-pr-scope.mjs:108-157) and has already drifted: it accepts ./.. and skips a source the original rejects. Export the member enumeration from one module and call it from both; the accessibility scope adds its route check on top.
…s; one folder expansion An accessibility or RTL audit that failed before writing its report now fails its owners instead of reading not-run, and an RTL coverage gap fails rather than reading "no RTL check applies". The PR status table says visual did not run (and names ci:run-visual) when pr-visual's default mode compared nothing. The changed-folder expansion is one helper, changedFolderMembers in a11y-pr-scope.mjs, used by both the accessibility scope and this check.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this pull request intentionally rewrites spec:AST-030 FR11 to FR13 and DEC-4 to DEC-7 and the RTL audit evidence record's INV7 to add a label-free touched-components check, with its five-component and shared-style skip rule, which the current FR12 and DEC-7 rule out. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA> for this head. Whatever the decision, these need fixing:
- The branch conflicts with #9217 in
.github/scripts/lib/pr-status.jsand its test, so CI has not run; mergemainand keep #9217's comment reader as the one path, dropping this branch's overlapping touched-scope edits there. - The new
suiteSucceededclause tells a pull request that already hasci:run-visualto add it whenever its visual run published no verdict. - The description's Change and Evidence sections still say
pr-touched-componentsruns the focused visual check; that ispr-visual's default step.
[Automated review]
| if ( | ||
| suite.id === 'visual' && | ||
| state === 'ran' && | ||
| !visualVerdict && | ||
| suiteSucceeded(suite, jobs) | ||
| ) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
On a pull request that already carries ci:run-visual and whose visual run published no verdict (no stable story in scope), this returns true and the comment says "add ci:run-visual"; the base does not. This clause sits outside the conflict hunks with #9217, so a merge that takes main's side keeps it. Drop it with the rest of the touched-scope reading and rely on #9217's scout-based check.
…-check # Conflicts: # .github/scripts/lib/pr-status.js # .github/scripts/lib/pr-status.test.mjs
Main's PR report (#9217) reads the touched-components record for per-component results; this branch's own status-table edits were dropped in the merge, so the verdict's compared-components list has no reader.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this pull request intentionally rewrites spec:AST-030 FR11 to FR13 and DEC-4 to DEC-7 and the RTL audit evidence record's INV7 to add a label-free touched-components check, with its five-component and shared-style skip rule, which the current FR12 and DEC-7 rule out. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA> for this head. Whatever the decision, one fix is needed:
a11yResultintouched-components.mjsdrops new accessibility violations the audit files under a multi-component folder's umbrella story (such ascore/Chat) or a member's pattern story (such asChatSendButton), so a Chat change with a new axe violation reads pass or not-run and the job stays green; take audited owners fromauditedStories, count umbrella violations against the folder's touched members, and test with the real report shape.
[Automated review]
| } | ||
| : {result: 'not-run'}; | ||
| } | ||
| const audited = Object.keys(report.components ?? {}).some(component => |
There was a problem hiding this comment.
report.components is keyed by story-title segment (Chat, ChatComposer, Button Pattern), while new violations are named by the audit's owner (core/Chat for the umbrella story, core/ChatSendButton for its pattern story). Neither matches a touched member here, so a Chat change with a new axe violation reads pass/not-run and hasFailure stays false. Read audited owners from report.auditedStories[].owner, count a <package>/<Folder> umbrella's violations against that folder's touched members, and test with the real report shape.
…y violations The audit files a multi-component folder's violations under its umbrella story owner (core/Chat) or a member's pattern story owner (core/ChatSendButton). Audited owners now come from the report's auditedStories; an umbrella violation fails every touched member of its folder, and an audited owner outside the touched set gets its own row, so a new violation can never read pass or not-run. Tested with the audit's real report shape through diffAgainstBaseline.
There was a problem hiding this comment.
Approval waits on an engineering owner's spec decision: this pull request intentionally rewrites spec:AST-030 FR11 to FR13 and DEC-4 to DEC-7 and the RTL audit evidence record's INV7 to add a label-free touched-components check, with its five-component and shared-style skip rule, which the current FR12 and DEC-7 rule out. Any engineering owner, including the author, records that decision by commenting /approve-spec <full head SHA> for this head. The accessibility fix for multi-component folders is in and tested, so no other change is needed.
[Automated review]
|
/approve-spec 555e6f3 |
github-actions
Bot
removed
the
needs:spec-owner-review
…-check # Conflicts: # CONTRIBUTING.md
|
/approve-spec ea20f24 |
There was a problem hiding this comment.
Changes requested: the owner-approved spec rewrite and the CI impact section hold, but three small fixes remain, and the new commit will need a fresh /approve-spec <full head SHA> from an engineering owner (the author may record it).
rtlResulttreats a curated RTL card whose rollup ispartial(for example, prev and next ordered correctly in LTR but not mirrored in RTL) as N/A, so the component reads RTLpassor "no RTL check applies"; mappartialtofailwith a test..github/scripts/visual-gate/README.mdstill says a pull request runspr-visualonly withci:run-visualand that PR accessibility always includes the modal-close, theme-var and story-play guards; update both sentences for the default touched check.- The new job was inserted between
release-check:and its "Release-time projection only" comment inci.yml; move that comment back aboverelease-check.
[Automated review]
| if: >- | ||
| (github.event_name != 'pull_request' || !github.event.pull_request.draft && (github.event.pull_request.head.repo.full_name == github.repository || contains(github.event.pull_request.labels.*.name, 'ci:run'))) && (!cancelled() && | ||
| (((github.event_name == 'pull_request' && needs.run-labels.outputs.visual == 'true') && | ||
| ((github.event_name == 'pull_request' && |
There was a problem hiding this comment.
Without the label clause this job starts on every unlabelled pull request, and its Storybook download and Playwright install (L2361, L2388-L2390) run whenever has_stable_visual is true: this pull request's own run took 43 s and compared nothing, where the base skipped the job. The description's CI impact: line (AST-030 FR19) should state that cost; gating those steps on the touched decision would also avoid most of it.
…ase-check comment match A curated card whose rollup is partial (some dimensions hold, others do not) now fails its owner instead of reading N/A. The visual-gate README says a pull request runs pr-visual's full check with ci:run-visual and its default touched check otherwise, and that the touched accessibility audit runs without the guards. The release-check comment moves back above its job.
|
/approve-spec 1b51112 |
There was a problem hiding this comment.
Approval recommended: an engineering owner approved the rewritten spec:AST-030 and rtl-audit-evidence INV7 at this head, and the partial-RTL, visual-gate README and release-check comment fixes are in, the first with a test. Nothing further is needed from the contributor beyond required checks finishing green.
[Automated review]
Maintainer impact
A pull request update runs no visual, accessibility or RTL check unless someone adds a
ci:run-*label, so a component change usually reachesmainwith no browser evidence. The PR comment can't link a reviewer to the affected components' docsite and Storybook pages with a result beside each. A narrow default check gives every component pull request that evidence for a few standard-runner minutes per update, and leaves the full suites behind their labels.Intended invariant
pr-visual's default mode compares the touched Core components (the only stable visual stories).pr-touched-componentsaudits the touched components in every package (core/…,lab/…,charts/…) on one standard arm runner..stylex.tsfile in a package's sharedtheme,utils,hooks,internalorLayerfolder). Behaviour-only shared code is not a wide change. Nothing starts a full suite on its own;ci:run-visual|a11y|rtldo.CI impact (standard
ubuntu-24.04-armrunners for both jobs; no non-standard runner):Stable visual regression(pr-visual) now starts on every unlabelled pull request whose Storybook builds.ci:run-visual.Touched components (visual, accessibility, RTL)(new) runs on every pull request whose Storybook builds, labelled or not.ci:run-a11yandci:run-rtl.Change
pr-visual's default mode (the visual part; Core only, since only Core has stable visual stories):ci:run-visual,touched-components.mjs scopedecides the touched set and wide change, keeping only components with an accepted baseline shot.gate.mjs check --tiers component --no-scoutcompares them, with the PR head and base SHA andASTRYX_VISUAL_SCOPE=touched, so the published verdict carriesscope: touched.ci:run-visual. A scope with nothing to compare leaves no verdict.ci:run-visual.pr-visualstays the one visual owner (FR12 guard).pr-touched-componentsjob (accessibility and RTL, every package, one standard arm runner):pr-analysis, skipped for spec-only, docsite-only and tooling-only changes.wide_changeandcomponent_countas job outputs.a11y-pr-scope.mjsfilters) and the RTL audit (--filterper package). RTL runs even after a failed accessibility audit.pr-visual's verdict, including after a failedpr-visual.touched-components.mjs reportwritestouched-components.json:{headSha, wideChange, componentCount, components: [{key, package, name, results: {visual, a11y, rtl}}]}, with each resultpass,fail,changedornot-run. A wide change writeswideChange: true,componentCount,foundations, and the components without results. The file is uploaded as thetouched-componentsartifact, which the PR report reads (ci: the PR report reads the touched-components check's per-component results and wide-change flag, once #9178 writes them #9217).fail, and a failed capture is matched to its component through the manifest. A component with no accepted baseline isnot-run.partialcurated card fails. A coverage gap fails, and an audit that failed before its report fails.changedFolderMembersina11y-pr-scope.mjs, the accessibility scope's own helper. A folder counts as one component for the wide rule, as the threshold's data counted folders.Wide-change threshold: N = 5
The coordinator's analysis of 600 merged PRs since 10-17: 327 touched at least one component, with a median of 1, p90 of 2, p95 of 3 and a maximum of 21. Only 2.1% touched more than 5 and 0.9% more than 10.
My count over 908 merged PRs since 10-17 agrees. 454 touched at least one component, with a p90 of 3 and a p95 of 3. Examples above the line: #9055 (21 components), #8367 (20), #9071 (16).
Evidence
touched-components.test.mjs:.stylex.ts) are wide while behaviour-only paths aren't.pr-touched-componentsruns on every PR after the Storybook build andpr-visual, onubuntu-24.04-armwith a ≤15-minute timeout; exposes the outputs; is in no join; skips visibly; uploads.pr-visual's default step runsgate.mjs check --tiers component, never the release plan, with the PR head and base andscope: touched.ci-test-routing.test.mjs: the job asks the scope action forcomponent-audit.pr-status.test.mjsandpublish-pr-report.test.mjs: the published verdict keeps only thetouchedscope marker, which the PR report (ci: the PR report reads the touched-components check's per-component results and wide-change flag, once #9178 writes them #9217) reads.Suites:
.github/scriptspasses 1,727 tests.actionlintshows no new findings andpnpm check:knowledgeis aligned.Measured replay (a do-not-merge replay pull request, now closed, on this design; standard GitHub-hosted arm runners; wall-clock minutes per job):
pr-touched-componentspr-visual(default touched mode)pr-rtl-shard.not-run, notpass.Scope
Writing rubric results
R1 narration: none. FR11, FR12, FR13, DEC-4, DEC-6, DEC-7 and rtl-audit-evidence INV7 state the check, the wide rule and the advisory job in the present tense.
R2 research: none. The threshold data lives in this description; the record keeps only the rule.
R3 evidence: none changed.
R4 rejected alternatives: none added.
R5 change references: none.
R6 open questions: none.
Testing
pnpm vitest run --project node .github/scripts/;actionlint;pnpm check:knowledge.