Addon-A11y: Fix a11y postinstall #33060
Conversation
valentinpalkovic
added
the
patch:yes
|
View your CI Pipeline Execution ↗ for commit 763a269
☁️ Nx Cloud last updated this comment at |
valentinpalkovic
force-pushed
the
valentin/addon-a11y-fix-postinstall-automigration
branch
from
2b8c69f to
763a269
Compare
📝 WalkthroughWalkthroughReplaces direct shell execution in a11y postinstall with a JsPackageManagerFactory-based runPackageCommand, and adds an auto-approve branch in automigrate runFixes to skip confirmation when Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Postinstall as postinstall.ts
participant PM as JsPackageManager
participant SB as storybook CLI
Postinstall->>PM: getPackageManager({ force, configDir, packageManager })
PM-->>Postinstall: packageManager instance
Postinstall->>PM: runPackageCommand('storybook', args)
PM->>SB: execute 'storybook automigrate' with args
SB-->>PM: exit/status
PM-->>Postinstall: result
sequenceDiagram
autonumber
participant CLI as runFixes
participant User as UserPrompt
CLI->>CLI: iterate fixes
alt fix.promptType == 'auto' and flags.yes == true
CLI->>CLI: shouldRun = true
else
CLI->>User: prompt to confirm
User-->>CLI: user decision (yes/no)
CLI->>CLI: set shouldRun per response
end
CLI->>CLI: run or skip fix based on shouldRun
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes
Possibly related PRs
✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/addons/a11y/src/postinstall.ts (1)
23-28: Implementation correctly uses JsPackageManagerFactory.The refactor from direct shell execution to using
JsPackageManagerFactoryis implemented correctly. The package manager is properly configured with theforce(packageManager) andconfigDiroptions, andrunPackageCommandis called with the appropriate arguments.Consider whether explicit error handling should be added for the
runPackageCommandcall. Currently, any errors will bubble up to the caller, which may be acceptable depending on how postinstall errors are handled at the addon installation level.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/addons/a11y/src/postinstall.ts(1 hunks)code/lib/cli-storybook/src/automigrate/index.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Tabs/Tabs.stories.tsx:222-227
Timestamp: 2025-11-05T09:36:55.944Z
Learning: Repo: storybookjs/storybook PR: 32458 — In code/core/src/components/components/Button/Button.tsx (React/TypeScript), ButtonProps includes ariaLabel?: string | false and the component maps it to the DOM aria-label. Convention: ariaLabel is mandatory on all Button usages — provide a descriptive string for icon-only buttons; set ariaLabel=false when the button’s children already serve as the accessible name. Do not suggest using a raw aria-label prop on Button call sites.
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/addons/a11y/src/postinstall.ts
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Applied to files:
code/addons/a11y/src/postinstall.ts
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/addons/a11y/src/postinstall.ts
📚 Learning: 2025-10-01T15:24:01.060Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32594
File: code/core/src/components/components/Popover/WithPopover.tsx:7-9
Timestamp: 2025-10-01T15:24:01.060Z
Learning: In the Storybook repository, "react-aria-components/patched-dist/*" (e.g., "react-aria-components/patched-dist/Dialog", "react-aria-components/patched-dist/Popover", "react-aria-components/patched-dist/Tooltip") are valid import paths created by a patch applied to the react-aria-components package. These imports should not be flagged as broken or invalid until a maintainer explicitly states they are no longer acceptable.
Applied to files:
code/addons/a11y/src/postinstall.ts
📚 Learning: 2025-09-24T09:39:39.233Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32507
File: code/core/src/manager/globals/globals-module-info.ts:25-33
Timestamp: 2025-09-24T09:39:39.233Z
Learning: In Storybook, storybook/actions/decorator is a preview-only entrypoint and should not be included in manager globals configuration. The duplicatedKeys array in code/core/src/manager/globals/globals-module-info.ts is specifically for manager-side externalization, not preview entrypoints.
Applied to files:
code/addons/a11y/src/postinstall.ts
🧬 Code graph analysis (1)
code/addons/a11y/src/postinstall.ts (2)
code/lib/cli-storybook/src/add.ts (1)
PostinstallOptions(21-26)code/core/src/common/js-package-manager/JsPackageManagerFactory.ts (1)
JsPackageManagerFactory(29-195)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/lib/cli-storybook/src/automigrate/index.ts (1)
327-339: LGTM! Auto-approval logic correctly implemented.The conditional auto-approval for 'auto' promptType migrations when the
yesflag is provided is implemented correctly and follows the same pattern used for manual migrations earlier in the function.code/addons/a11y/src/postinstall.ts (2)
1-1: LGTM! Import path is correct.The import of
JsPackageManagerFactoryfromstorybook/internal/commonaligns with the internal Storybook API structure.
6-6: Let me verify the actual migration file to confirm this is the correct and intentional migration ID:Perfect! The web search confirms that'addon-a11y-addon-test'is the actual, legitimate migration ID, not a placeholder. The automigration inaddon-a11y-addon-test.tshelps users transition to the new parameter system, and it configures accessibility tests with theparameters.a11y.testparameter for Vitest addon or test-runner.Likely an incorrect or invalid review comment.
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 2 | 2 | 0 |
| Self size | 414 KB | 181 KB | 🎉 -232 KB 🎉 |
| Dependency size | 2.97 MB | 2.97 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
Closes #32242
What I did
Fixes an issue of calling an automigration while running the addon-a11y's postinstall script.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
yesflag, bypassing interactive confirmation prompts for streamlined, non-interactive workflows.