CLI: Avoid mixed CSF in files with unconventional stories #32716
Conversation
- Added early exit condition in `storyToCsfFactory` to skip transformation when no stories are detected, preventing mixed CSF syntax. - Updated tests to verify the new behavior, ensuring proper logging when no stories are transformed. - Mocked logger's log method for improved test coverage and validation of warning messages.
📝 WalkthroughWalkthroughAdds an early bail-out in story-to-csf-factory to return the original source when no stories are transformed and updates tests accordingly: logger mock now includes log(), expectations adjusted, new test for bail-out behavior, and snapshots updated to include/export A and its transformation to meta.story(). Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Caller
participant Factory as story-to-csf-factory
participant Parser as Parser/Analyzer
participant Logger as logger
Dev->>Factory: transform(source)
Factory->>Parser: parse + detect stories
Parser-->>Factory: detectedStoryNames, AST
rect rgba(200, 230, 255, 0.3)
note over Factory: Transform pass
Factory->>Factory: attempt to transform stories<br/>(collect transformedStoryExports)
end
alt No stories transformed
Factory->>Logger: warn("No stories transformed... returning original")
Factory-->>Dev: original source
else Some transformed
alt Inconsistent: transformed count != detected count
Factory->>Logger: warn("Inconsistent transformation... returning original")
Factory-->>Dev: original source
else Consistent
Factory-->>Dev: transformed source
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit b1b26aa
☁️ Nx Cloud last updated this comment at |
![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 (2)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts (1)
242-248: Early bail is correct; avoid warning on already-transformed files (reduce noise).Current behavior warns even when a file is already fully CSF-factory–converted. Consider silently returning in that case (or logging at info/debug), while keeping warnings for unsupported/partial cases.
Suggested refinement:
// If no stories were transformed, bail early to avoid having a mixed CSF syntax and therefore a broken indexer. if (transformedStoryExports.size === 0) { - logger.warn( - `Skipping codemod for ${info.path}: no stories were transformed. Either there are no stories, file has been already transformed or some stories are written in an unsupported format.` - ); - return info.source; + // Detect "already transformed": every detected story initializer is a `*.story(...)` call. + const alreadyTransformedCount = Object.values(csf._storyExports).reduce((acc, decl) => { + const init = t.isVariableDeclarator(decl as t.VariableDeclarator) + ? (decl as t.VariableDeclarator).init + : undefined; + return acc + + (t.isCallExpression(init) && + t.isMemberExpression(init.callee) && + t.isIdentifier(init.callee.property, { name: 'story' }) + ? 1 + : 0); + }, 0); + + if (detectedStoryNames.length > 0 && alreadyTransformedCount === detectedStoryNames.length) { + // File appears already transformed; return source silently. + return info.source; + } + + logger.warn( + `Skipping codemod for ${info.path}: no stories were transformed. Either there are no stories, file has been already transformed or some stories are written in an unsupported format.` + ); + return info.source; }If you prefer not to add the detection, alternatively lower the log level for the “already transformed” case, and keep warn for “unsupported”/partial conversions.
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (1)
681-698: Strengthen logger assertions and leverage global mock clearing
- Replace
vi.mocked(logger.warn).mock.calls[0][0]withexpect(logger.warn).toHaveBeenLastCalledWith(...)in both tests.- In the “already transformed” case, add
expect(logger.warn).not.toHaveBeenCalled().- Remove the manual
beforeEach(() => vi.clearAllMocks())—clearMocks: trueis already enabled in your Vitest config.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts(9 hunks)code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.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/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
code/**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx
Files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Use vi.mocked() to type and access mocked functions
Implement mock behaviors in beforeEach blocks
Mock all required dependencies that the test subject uses
Mock implementations should be placed in beforeEach blocks
Each mock implementation should return a Promise for async functions
Mock implementations should match the expected return type of the original function
Use vi.mocked() to access and implement mock behaviors
Mock all required properties and methods that the test subject uses
Avoid direct function mocking without vi.mocked()
Avoid mock implementations outside of beforeEach blocks
Avoid mocking without the spy: true option
Avoid inline mock implementations within test cases
Avoid mocking only a subset of required dependencies
Mock at the highest level of abstraction needed
Keep mock implementations simple and focused
Use type-safe mocking with vi.mocked()
Document complex mock behaviors
Group related mocks together
Files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
⏰ 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). (3)
- GitHub Check: Danger JS
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (3)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts (1)
251-258: Consistency check looks goodThe simplified condition precisely guards against mixed CSF by bailing when some detected stories couldn’t be transformed.
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (2)
11-16: Logger mock extension is fineAdding logger.log alongside warn is OK and aligns with production usage.
34-45: Snapshot churn introducing A is expectedAdding a simple story export (A) to exercise transformation paths in snapshots is reasonable and improves coverage.
Also applies to: 50-62, 66-77, 85-98, 107-120, 129-142, 151-165, 193-218, 250-275, 292-301, 368-372, 390-394, 410-412, 431-438, 457-462, 481-486, 506-511, 533-536, 558-561, 583-586, 616-623
Closes #32680
What I did
This PR adds an early exit condition in
storyToCsfFactoryto skip transformation when no stories are detected, preventing mixed CSF syntax.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
Bug Fixes
Refactor
Tests