Skip to content

CLI: Skip a11y addon automigration prompting when not applicable#35376

Merged
Sidnioulz merged 1 commit into
nextfrom
yann/a11y-automigration-fix
Jul 9, 2026
Merged

CLI: Skip a11y addon automigration prompting when not applicable#35376
Sidnioulz merged 1 commit into
nextfrom
yann/a11y-automigration-fix

Conversation

@yannbf

@yannbf yannbf commented Jul 6, 2026

Copy link
Copy Markdown
Member

Closes #

What I did

The addon-a11y-addon-test automigration was prompted for projects where it had nothing to do. Its check() only evaluated the "already migrated" skip conditions when both .storybook/vitest.setup.* and .storybook/preview.* existed:

if (vitestSetupFile && previewFile) {
  ...
  if (skipVitestSetupTransformation && skipPreviewTransformation) {
    return null; // the only "don't prompt" exit
  }
}

Projects without a vitest.setup file — the norm since the vitest plugin auto-provisions preview annotations (10.3+) — skipped that block entirely, so the migration was always prompted even when preview already had parameters.a11y.test configured. Selecting it was a complete no-op (it rewrote preview with identical content).

This PR computes the two skip flags independently per file:

  • No vitest.setup file → nothing to transform on the setup side (the plugin auto-provisions annotations, including addon-a11y's). Projects that keep the optional setup file are still checked and transformed exactly as before.
  • The preview file is checked on its own, regardless of whether a setup file exists.
  • The early return null now runs whenever both flags are set.

This also fixes a second latent bug the old gating had: a project with an already-migrated setup file but no preview file skipped the "already has @storybook/addon-a11y" detection and re-transformed the setup file, injecting a duplicate a11yAddonAnnotations import.

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

  • stories
  • unit tests
  • integration tests
  • end-to-end tests

Includes a regression test for the false-positive scenario (no setup file + preview already configured → check() returns null), and un-skips the previously skipped "preview file only" test with a corrected expectation.

Manual testing

  1. Take a project with @storybook/addon-a11y and @storybook/addon-vitest in main.ts, no .storybook/vitest.setup.* file, and parameters: { a11y: { test: 'todo' } } already set in .storybook/preview.* (e.g. https://github.com/yannbf/mealdrop).
  2. Run npx storybook@next upgrade (or yarn automigrate from a linked CLI build).
  3. Before this fix: the addon-a11y-addon-test automigration is prompted and does nothing when run. After: it is not prompted.
  4. Also verify a legacy project with a .storybook/vitest.setup.ts lacking the a11y annotations still gets prompted and transformed as before.

Documentation

  • Add or update documentation reflecting your changes
  • If you are deprecating/removing a feature, make sure to update
    MIGRATION.MD

Checklist for Maintainers

  • When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts
  • Declare whether manual QA will be needed for this PR during the next release, through qa:needed or qa:skip

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved automigration checks for Storybook preview and Vitest setup files so updates are only suggested when needed.
    • Better handles cases where the preview file already includes the required changes, avoiding unnecessary transformations.
    • More accurately skips setup changes when transformation isn’t possible or no longer required.

…ng to do

The check only evaluated the "already migrated" skip conditions when both
.storybook/vitest.setup.* and .storybook/preview.* existed. Projects without
a vitest.setup file (the norm since the vitest plugin auto-provisions preview
annotations in 10.3+) were always prompted, even when preview already had
parameters.a11y.test configured — and running the migration was a no-op.

The skip flags are now computed independently per file: a missing setup file
counts as nothing-to-transform on the setup side, and the preview file is
checked on its own. This also fixes re-transforming an already-migrated setup
file (duplicate a11yAddonAnnotations import) when no preview file existed.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@yannbf yannbf added bug ci:normal Run our default set of CI jobs (choose this for most PRs). qa:needed Pull Requests that will need manual QA prior to release. cli labels Jul 6, 2026
@yannbf yannbf changed the title CLI: Fix addon-a11y-addon-test automigration prompting when there is nothing to do CLI: Skip a11y addon automigration prompting when not applicable Jul 6, 2026
@coderabbitai

coderabbitai Bot commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The automigration for the a11y addon test now computes skipVitestSetupTransformation and skipPreviewTransformation independently rather than requiring both files to exist together, with earlier null returns when both are skipped. Catch clauses in two helper functions were simplified. Corresponding tests were enabled, updated, and extended.

Changes

Independent Skip-Flag Migration Logic

Layer / File(s) Summary
Independent transformation flags
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts
Replaces combined vitest setup/preview existence check with independently initialized and computed skipVitestSetupTransformation/skipPreviewTransformation flags, returns null earlier when both are skipped, and simplifies catch (e) to catch in two helper functions.
Updated test expectations and coverage
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.test.ts
Enables a previously skipped test, updates expected skipVitestSetupTransformation values across scenarios, and adds a case asserting check returns null when vitest.setup is missing but preview already contains required transformations.

Estimated code review effort: 2 (Simple) | ~10 minutes

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts (1)

80-96: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value

Files are read twice (once here, once in getTransformedSetupCode/getTransformedPreviewCode).

vitestSetupFile/previewFile are each read via readFileSync here to compute skip flags, then read again inside getTransformedSetupCode/getTransformedPreviewCode (Lines 108, 121) if not skipped. Since this pattern predates this diff, it's not a regression, but caching the source read once would avoid redundant I/O and reduce inconsistency risk (e.g., file changing between reads).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts` around
lines 80 - 96, The `add`/skip-flag logic in `addon-a11y-addon-test.ts` reads
`vitestSetupFile` and `previewFile` twice, once in this precheck block and again
in `getTransformedSetupCode`/`getTransformedPreviewCode`. Refactor the flow so
the file contents are read once, cached, and reused to compute
`skipVitestSetupTransformation`/`skipPreviewTransformation` and to feed the
transformation helpers, avoiding duplicate `readFileSync` calls and inconsistent
reads.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.test.ts`:
- Around line 198-232: The current coverage only exercises the case where
vitest.setup is missing and the preview file is already transformed, but it
misses the mirrored path where previewFile is absent and vitest.setup is already
migrated. Add a symmetric test in addonA11yAddonTest.check that mocks the
preview file as missing while providing a migrated vitest.setup, and assert it
returns null; this will cover the skipPreviewTransformation default behavior in
the same check flow.

In `@code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts`:
- Around line 77-100: The default handling for preview transformation is
incomplete in check(), causing a migration result even when previewFile is
absent. Update add-on-a11y addon test logic so skipPreviewTransformation is
initialized from !previewFile, and keep the existing preview-file
read/shouldPreviewFileBeTransformed flow only when previewFile exists. Verify
the early return in check() now skips the auto flow when both transformations
are unavailable, and add a test covering the missing-preview case.

---

Nitpick comments:
In `@code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts`:
- Around line 80-96: The `add`/skip-flag logic in `addon-a11y-addon-test.ts`
reads `vitestSetupFile` and `previewFile` twice, once in this precheck block and
again in `getTransformedSetupCode`/`getTransformedPreviewCode`. Refactor the
flow so the file contents are read once, cached, and reused to compute
`skipVitestSetupTransformation`/`skipPreviewTransformation` and to feed the
transformation helpers, avoiding duplicate `readFileSync` calls and inconsistent
reads.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 7c36610b-bcd6-430d-b1f5-3c41b996b152

📥 Commits

Reviewing files that changed from the base of the PR and between fac05a5 and 774298e.

📒 Files selected for processing (2)
  • code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.test.ts
  • code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts

@storybook-app-bot

storybook-app-bot Bot commented Jul 6, 2026

Copy link
Copy Markdown

Package Benchmarks

Commit: 774298e, ran on 9 July 2026 at 10:56:37 UTC

The following packages have significant changes to their size or dependencies:

storybook

Before After Difference
Dependency count 73 72 🎉 -1 🎉
Self size 21.95 MB 21.94 MB 🎉 -16 KB 🎉
Dependency size 36.65 MB 36.44 MB 🎉 -213 KB 🎉
Bundle Size Analyzer Link Link

@storybook/cli

Before After Difference
Dependency count 205 204 🎉 -1 🎉
Self size 825 KB 821 KB 🎉 -4 KB 🎉
Dependency size 92.16 MB 91.93 MB 🎉 -229 KB 🎉
Bundle Size Analyzer Link Link

@storybook/codemod

Before After Difference
Dependency count 198 197 🎉 -1 🎉
Self size 32 KB 32 KB 🚨 +4 B 🚨
Dependency size 90.64 MB 90.41 MB 🎉 -229 KB 🎉
Bundle Size Analyzer Link Link

create-storybook

Before After Difference
Dependency count 74 73 🎉 -1 🎉
Self size 1.09 MB 1.09 MB 🎉 -153 B 🎉
Dependency size 58.61 MB 58.38 MB 🎉 -229 KB 🎉
Bundle Size Analyzer node node

@Sidnioulz Sidnioulz added the upgrade:10.5.0 Issues/PRs found during 10.5 upgrade QA and post-release regressions label Jul 9, 2026
@Sidnioulz Sidnioulz enabled auto-merge July 9, 2026 11:03
@Sidnioulz Sidnioulz disabled auto-merge July 9, 2026 11:19
@Sidnioulz Sidnioulz merged commit 732e899 into next Jul 9, 2026
167 of 178 checks passed
@Sidnioulz Sidnioulz deleted the yann/a11y-automigration-fix branch July 9, 2026 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug ci:normal Run our default set of CI jobs (choose this for most PRs). cli qa:needed Pull Requests that will need manual QA prior to release. upgrade:10.5.0 Issues/PRs found during 10.5 upgrade QA and post-release regressions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants