Repository navigation
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
AI review status for this pull request.
|
meta-cla
Bot
added
the
CLA Signed
github-actions
Bot
added
the
needs:spec-owner-review
PR Analysis Report
📚 Storybook Preview🧪 Sandbox PreviewModified ComponentsToast (@astryxdesign/core) · View in Storybook
Component Size
Sizes are gzip. Import counts the JavaScript the import loads eagerly; CSS counts the package stylesheet rules referenced by the modules it can load from its own package, lazily loaded ones included. Bundle Size SummaryGzip figures measure each package's entry file, not the code an import pulls in.
Published Package Size
10 other published packages are unchanged. 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 |
8356639 to
8dfc6b0
Compare
![astracat-bot[bot]](https://avatars.githubusercontent.com/in/4796231?s=60&v=4){kind=small}
There was a problem hiding this comment.
This replay can't be approved yet: its AST-030 FR12/DEC-7 rewrite adds an every-PR touched-components check that the current FR12 and DEC-7 keep behind the ci:run-* labels. An engineering owner records whether that requirement stands by commenting /approve-spec <full head SHA>, and the author can do so as an engineering owner. Whatever that decision, the job needs these fixes before it carries #9178's minutes:
- The repository's FR12 guard (
scripts/check-visual-workflow-owner.mjs) rejectspr-touched-components, so the requiredtestfails; FR11, FR12's single-owner sentence, DEC-4 and the RTL audit architecture's INV7 need amending alongside it. - Spec-, docsite- and tooling-only pull requests would fail at the ungated
pr-analysisdownload, and a new Core component would crash the visual step. - Failed visual captures, curated RTL failures and all-N/A RTL results show as
passintouched-components.json, and a change inside a multi-component folder such as Chat reports no components. - The branch predates
main's package-size step, sobuild-storybookfailed and the touched-components job was skipped; rebase before the next scenario.
[Automated review]
| ref: ${{ github.event.pull_request.head.sha }} | ||
| persist-credentials: false | ||
|
|
||
| - name: Download analysis artifact |
There was a problem hiding this comment.
build-storybook succeeds without uploading pr-analysis on spec-only, docsite-only and tooling-only lanes (ci.yml:1089-1090), so this ungated download fails the job on every such pull request. The other downloads of this artifact are gated (ci.yml:1493, 1622, 2595).
| COMPONENTS: ${{ steps.touched.outputs.visual_components }} | ||
| run: | | ||
| set +e | ||
| node .github/scripts/visual-gate/gate.mjs check \ |
There was a problem hiding this comment.
scripts/check-visual-workflow-owner.mjs:94-96 admits gate.mjs check only in ci.yml → pr-visual, so this line fails test-node (2/4) and the required test: "ci.yml → pr-touched-components: visual regression belongs in ci.yml → pr-visual (AST-030 FR12)".
| if: steps.touched.outputs.has_components == 'true' | ||
| run: pnpm install --frozen-lockfile && npx playwright install chromium | ||
|
|
||
| # Exit 0 is clean, 2 is a change for review and 3 means nothing touched |
There was a problem hiding this comment.
gate.mjs exits 3 only inside dependents.length > 0 (gate.mjs:243-253), and this step passes no --dependents. A touched Core component with no accepted baseline produces an empty plan, which throws and exits 1 (gate.mjs:380-381), so the step fails instead of reporting not-run.
| 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} (capture.mjs:623), and failed shots are absent from the manifest, so this never matches: a failed capture reports pass. The test fixture {component: 'Button'} is a shape the gate never writes.
| ); | ||
| const mine = results.filter(result => namesOwner(result.component, identity)); | ||
| if (mine.length === 0) return {result: 'not-run'}; | ||
| const failed = mine.filter( |
There was a problem hiding this comment.
This treats an all-N-A component as pass, and RTL_SECTIONS leaves out curated.results, so a curated not-RTL rollup also reports pass. That contradicts the rtl-audit-evidence architecture's INV3 and INV4.
| * the rest. | ||
| */ | ||
| export function resolveTouchedScope(analysis) { | ||
| const touched = [ |
There was a problem hiding this comment.
This re-derives the changed-component set that resolvePrA11yComponents (a11y-pr-scope.mjs:68-158) owns. It drops unresolvedComponentSources, so a change only inside core/Chat reports "No component changed", and it skips the fail-closed analysis checks that DEC-5 requires.
…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.
8dfc6b0 to
1c84d7a
Compare
There was a problem hiding this comment.
This replay can't be approved yet. Its AST-030 FR12/DEC-7 rewrite adds a default touched-components check on every unlabelled pull request, which the current FR12 and DEC-7 keep behind the ci:run-* labels. An engineering owner records whether that requirement stands by commenting /approve-spec <full head SHA>; the author can do that as an engineering owner. Whatever the decision, these need fixing before #9178 lands:
- The required
testfails:.github/scripts/run-labels.test.mjsstill expectspr-visualto run only withci:run-visual. - FR12's retained label sentence, DEC-7's opening, DEC-6, FR13 and the RTL audit architecture's INV7 still say these owners run on a pull request only with a label.
pr-touched-componentsdownloadspr-analysiswith no condition, so every spec-, docsite- and tooling-only pull request fails it.- A new Core component fails
pr-visual's default step, because the gate never exits 3 without--dependents. rtlResultreports curated RTL failures and all-N/A components aspass, and the touched set drops multi-component folders such as Chat that the shared resolvers expand.
[Automated review]
| ref: ${{ github.event.pull_request.head.sha }} | ||
| persist-credentials: false | ||
|
|
||
| - name: Download analysis artifact |
There was a problem hiding this comment.
build-storybook succeeds on spec-only, docsite-only and tooling-only lanes but uploads pr-analysis only outside them (ci.yml:1161), so this ungated download fails the job on every such pull request. The other downloads of this artifact are gated (ci.yml:1556, :1685, :2347, :2699).
| set -e | ||
| case "$code" in | ||
| 0|2) ;; | ||
| 3) rm -f .visual-run/verdict.json ;; |
There was a problem hiding this comment.
gate.mjs exits 3 only inside dependents.length > 0 (gate.mjs:243-253), and this step passes no --dependents, so this arm never runs. A touched component with no stable story or no accepted baseline throws instead (plan.mjs:485-493, gate.mjs:377-382) and exits 1, failing pr-visual.
| 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.
An all-N-A component reports pass here, and RTL_SECTIONS leaves out curated.results, so a curated not-RTL card also reports pass; that contradicts the RTL audit architecture's INV3 and INV4. In this head's own run Toast's automatic results were all N/A and only the unread curated D4 card held its evidence.
| * scope audits its touched owners, and the comment points to the labels for | ||
| * the rest. | ||
| */ | ||
| export function resolveTouchedScope(analysis) { |
There was a problem hiding this comment.
This re-derives the changed-component set that resolvePrA11yComponents (a11y-pr-scope.mjs:68-158) and resolveRtlShardScope own. It ignores unresolvedComponentSources, so a change only inside core/Chat reports "No component changed", and it skips their fail-closed analysis checks.
| confirm that runs nothing. A label runs only its own suite; a pull request | ||
| update without one runs none of them, and no required context depends on | ||
| them. An opted-in PR's a11y MUST remain scoped to changed components | ||
| confirm that runs nothing. A label runs only its own suite. Without a label, a pull request update |
There was a problem hiding this comment.
Lines 192-198 still say pr-visual "MUST ... run only when the pull request carries that suite's label", while the new text from here runs its default mode on every pull request; DEC-7's opening (494), DEC-6 (523-525) and FR13 (253) say the same and need amending with it.
There was a problem hiding this comment.
This replay can't be approved yet. Its AST-030 FR12/DEC-7 rewrite adds a default touched-components check on every unlabelled pull request, which the current FR12 and DEC-7 keep behind the ci:run-* labels. An engineering owner records whether that requirement stands by commenting /approve-spec <full head SHA>, and the author can do that as an engineering owner. Whatever the decision, scenario C's own run shows these need fixing before #9178 lands:
- The required
testfails because.github/scripts/run-labels.test.mjsstill expectspr-visualto run only withci:run-visual. touched-components.jsonrecords Lab MobileTokenizer's RTL coverage gap aspassand the three componentspr-visualpassed as visualnot-run, because the verdict download is skipped after any earlier failure.- Selector has no row: the touched set ignores
unresolvedComponentSources, which the shared accessibility resolver expands, so scenario C measured four components, not five. - FR12's retained label sentence, DEC-7's opening, DEC-6, FR13 and the RTL audit architecture's INV7 still say these owners run only with a label.
- Spec-, docsite- and tooling-only pull requests fail the new job at its ungated
pr-analysisdownload.
[Automated review]
| # 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.
With no status function GitHub adds success() here, so this download is skipped whenever an earlier step failed. In this head's run the RTL step failed on the Lab coverage gap. Toast, TextArea and MultiSelector were then recorded as visual not-run, although pr-visual passed their 6 shots. !cancelled() && (or always() &&, as on the report step below) keeps the verdict.
| */ | ||
| export function resolveTouchedScope(analysis) { | ||
| const touched = [ | ||
| ...new Set([ |
There was a problem hiding this comment.
In this head's run the analysis listed core/Selector under unresolvedComponentSources. The accessibility step's shared resolver audited Selector, but this set, pr-visual's list and the RTL owners left it out, and the record has no Selector row.
|
Replay finished; minutes are in #9178. |
Do not merge. Times the touched-components check proposed in #9178 on component sets from recent pull requests: A one component (Toast, like #9016), B three (Selector, MultiSelector, BottomSheet, like #9018/#9020), C five including a Lab component, D six (a wide change, which skips). Each push is one scenario; the minutes go to #9178. Closed when the replay finishes.