CLI: Skip vitest transform for CSF Factories in a11y-addon-test automigration #31941
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 files reviewed, 1 comment
Edit PR Review Bot Settings | Greptile
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts
Outdated
Show resolved
Hide resolved
|
View your CI Pipeline Execution ↗ for commit 2ffb1e1
☁️ Nx Cloud last updated this comment at |
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts
Outdated
Show resolved
Hide resolved
|
@mrginglymus could you please let me know how feasible it'd be to use an AST-based approach to detect preview? We have a few examples using Babel's AST tools in other migrations. Also, in #32083, I set up a test bed with the few scenarios/edge cases to handle spread over branches. This immensely speeds up testing and review, as reviewers otherwise have to build a sandbox from scratch for every iteration of the PR. It would be lovely (but not required :) ) if you could set one up too. |
|
@Sidnioulz I've pushed the AST approach - there was already a utility for csf4 sniffing so I've used that in the project gathering step and propagated it to all migrations. That does mean we have no automated tests now because the migration tests only test code after the gather phase; I'll need to sniff around for tests that capture an earlier phase - or, as you say, set up a repro. |
I feel that should be acceptable. You could use the SB logger and add debug messages in your migration code, or even assertions that would fail when your script finds an illogical/incoherent situation. If your script fails when it can't produce a coherent output, then users know when to switch to manual migration. Part of the deal is we need to point to the right documentation resource for them. Besides, if we have an E2E test somewhere, then everything works at the time the script is most needed. It might break later (as NPM deps evolve), and at the moment, we don't run any repro case in CI for any automigration. It might be something we'd want to automate, but the ROI isn't as clear or high as other things the team is working on, so it hasn't happened yet. |
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
Outdated
Show resolved
Hide resolved
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a new boolean csf4 flag computed from preview config, threads it through project collection and automigration pipelines, updates types and data structures, and uses it in the addon-a11y check to gate vitest setup transformation behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Dev as CLI User
participant CLI as CLI Automigrate
participant Util as util.processProject
participant CFG as getStorybookData
participant CSF as csf-tools
Dev->>CLI: run automigrate
CLI->>Util: processProject(projectPath)
Util->>CFG: getStorybookData(configDir)
CFG->>CFG: locate previewConfigPath
alt previewConfigPath found
CFG->>CSF: readConfig(previewPath)
CSF-->>CFG: previewConfig
CFG->>CSF: isCsfFactoryPreview(previewConfig)
CSF-->>CFG: csf4:boolean
else
CFG->>CFG: csf4 = false
end
CFG-->>Util: { ..., csf4 }
Util-->>CLI: CollectProjectsSuccessResult { ..., csf4 }
note over CLI: Automigration phase
CLI->>CLI: build ProjectAutomigrationData with csf4
CLI->>Fixes: check(options{ ..., csf4 })
alt addon-a11y-addon-test
Fixes->>Fixes: skipVitestSetupTransformation = csf4
end
CLI->>Fixes: run(options{ ..., csf4 })
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@mrginglymus I've updated your branch to trigger a fresh Windows test build as I think it was outdated, but FYI there are TS errors showing up in the CircleCI jobs, and you have failing tests for your automigration too. |
![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/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
182-183: Clarify the log message and add error handling.Two concerns:
The log message 'Getting csf version...' is misleading—it's actually detecting CSF factory usage in the preview file, not retrieving a version number. Consider changing to 'Detecting CSF factory usage...' for clarity.
If
readConfig(previewConfigPath)throws (e.g., due to syntax errors in the preview file), it will crash the entire data collection. Consider wrapping in try-catch:logger.debug('Detecting CSF factory usage...'); - const csf4 = previewConfigPath ? isCsfFactoryPreview(await readConfig(previewConfigPath)) : false; + let csf4 = false; + if (previewConfigPath) { + try { + csf4 = isCsfFactoryPreview(await readConfig(previewConfigPath)); + } catch (err) { + logger.debug(`Failed to read preview config for CSF factory detection: ${err}`); + } + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(3 hunks)code/lib/cli-storybook/src/automigrate/multi-project.ts(3 hunks)code/lib/cli-storybook/src/automigrate/types.ts(1 hunks)code/lib/cli-storybook/src/util.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Adhere to ESLint and Prettier rules across all JS/TS source files
Files:
code/lib/cli-storybook/src/automigrate/types.tscode/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/lib/cli-storybook/src/util.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Fix type errors and prefer precise typings instead of using any or suppressions, consistent with strict mode
Files:
code/lib/cli-storybook/src/automigrate/types.tscode/lib/cli-storybook/src/automigrate/fixes/addon-a11y-addon-test.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/lib/cli-storybook/src/util.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
🧬 Code graph analysis (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
code/core/src/csf-tools/ConfigFile.ts (2)
isCsfFactoryPreview(1189-1204)readConfig(1175-1178)
⏰ 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/fixes/addon-a11y-addon-test.ts (2)
48-48: LGTM! csf4 parameter correctly added to check signature.This enables the migration to detect CSF factory usage and adjust behavior accordingly.
74-74: LGTM! Correctly skips vitest setup transformation when CSF factories are detected.Initializing
skipVitestSetupTransformationwithcsf4ensures that projects using CSF factories (which usepreview.composed.beforeAllpattern) will skip the vitest setup transformation, addressing the issue described in #31940.code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
194-194: LGTM! csf4 correctly added to return value.The csf4 field is properly computed and propagated through the automigration pipeline.
mrginglymus
force-pushed
the
addon-a11y-migrate-fix
branch
from
cb0c21b to
2fff816
Compare
mrginglymus
force-pushed
the
addon-a11y-migrate-fix
branch
from
678ac26 to
e36148b
Compare
|
You didn't provide manual testing instructions, but I trust you have tested against a real use case @mrginglymus :) |
Sidnioulz
changed the title
addon-a11y-addon-test migration - do not transform vitest.setup file if using csf factories
|
Actually I could do with a canary for this one... |
Valentin triggered the canary creation, it should appear in your PR description shortly! |
|
Sad trombone, it doesn't work. |
Do you have leads to investigate, or would you need some time with the team to talk it out? |
Simple logic bug, but the test case wasn't realistic enough to capture it. Fixed now. |
6 participants
Closes #31940
What I did
Sniff for usage of csf factories in
vitest.setupand skip transformationChecklist 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 pull request has been released as version
0.0.0-pr-31941-sha-498c84dc. Try it out in a new sandbox by runningnpx [email protected] sandboxor in an existing project withnpx [email protected] upgrade.More information
0.0.0-pr-31941-sha-498c84dcaddon-a11y-migrate-fix498c84dc1764763383)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=31941Greptile Summary
Modified the a11y addon migration logic to prevent incorrect transformations of
vitest.setupfiles when CSF factories are detected. This fixes an issue where the standard transformation would fail for projects using Component Story Format factories.preview.composed.beforeAllin setup files to identify CSF factory usageaddonA11yAddonTest.check()to setskipVitestSetupTransformation=truewhen factories are detectedSummary by CodeRabbit
New Features
Bug Fixes