CLI: Make relative imports default in csf-factories codemod#32610
Conversation
|
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. 📝 WalkthroughWalkthroughUpdated CLI codemod messaging and options related to import types (relative vs subpath) and documentation links to v10. Adjusted test expectations by toggling useSubPathImports flags to reflect the new guidance. No control flow or exported API changes. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
…ts the default Co-authored-by: yannbf <1671563+yannbf@users.noreply.github.com>
|
@copilot make the suggested changes and resolve the comments after |
…n logic Co-authored-by: yannbf <1671563+yannbf@users.noreply.github.com>
|
@copilot I need you to update the text and the options again. |
Co-authored-by: yannbf <1671563+yannbf@users.noreply.github.com>
... Updated the text and options as requested in commit 7f79e3a. Changes include:
|
|
View your CI Pipeline Execution ↗ for commit 5065e09
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
91-101: Critical: Condition logic is inverted.The condition at line 91 adds an imports map when
useSubPathImportsistrue. However, with the inverted semantics (wheretruemeans relative imports), this adds the imports map when the user chooses relative imports—but the imports map (#*) is only needed for subpath imports.This causes the package.json to be modified unnecessarily when users select relative imports, and NOT modified when they actually need it for subpath imports.
Apply this diff to fix the inverted condition:
- if (useSubPathImports && !packageJson.imports?.['#*']) { + if (!useSubPathImports && !packageJson.imports?.['#*']) { logger.log( `🗺️ Adding imports map in ${picocolors.cyan(packageManager.primaryPackageJson.packageJsonPath)}` );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/lib/cli-storybook/src/codemod/csf-factories.ts(2 hunks)code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{js,jsx,json,html,ts,tsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{js,jsx,json,html,ts,tsx,mjs}: Run Prettier formatting on changed files before committing
Run ESLint on changed files and fix all errors/warnings before committing (useyarn lint:js:cmd <file>)
Files:
code/lib/cli-storybook/src/codemod/csf-factories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Export functions from modules when they need to be unit-tested
Files:
code/lib/cli-storybook/src/codemod/csf-factories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
code/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
In application code, use Storybook loggers instead of
console.*(client code:storybook/internal/client-logger; server code:storybook/internal/node-logger)
Files:
code/lib/cli-storybook/src/codemod/csf-factories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Do not use
console.log,console.warn, orconsole.errordirectly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/codemod/csf-factories.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
**/*.@(test|spec).{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.@(test|spec).{ts,tsx,js,jsx}: Unit tests should import and execute the functions under test rather than only asserting on syntax patterns
Mock external dependencies in tests usingvi.mock()(e.g., filesystem, loggers)
Files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
🧠 Learnings (2)
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
📚 Learning: 2025-10-13T13:33:14.659Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-10-13T13:33:14.659Z
Learning: Applies to code/.storybook/** : Place internal UI Storybook configuration in `code/.storybook/` and maintain it there
Applied to 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). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (1)
29-29: Test flag inversions correctly reflect the new semantics.The boolean flag flips in the test expectations are consistent with the inverted semantics described in the PR objectives, where
useSubPathImports: truenow generates relative imports anduseSubPathImports: falsegenerates subpath imports. The test at lines 348-398 explicitly validates this behavior:
useSubPathImports: false→ output is'#.storybook/preview'(subpath)useSubPathImports: true→ output is'../../preview'(relative)While the variable name
useSubPathImportsis now counterintuitive, the tests correctly enforce the intended behavior.Also applies to: 364-364, 386-386, 348-398
code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
70-74: LGTM on messaging and documentation updates.The updated user guidance, prompt message, and documentation links correctly reference the v10 CSF next docs and provide balanced information about both import types.
Also applies to: 78-78, 129-129
Problem
The csf-factories automigration had several issues that needed to be addressed:
csf-factories) instead of the correct Storybook 10 documentation (csf-next)Changes
Updated
code/lib/cli-storybook/src/codemod/csf-factories.ts,code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts, and tests:1. Fixed Documentation Links
https://storybook.js.org/docs/10/api/csf/csf-next#subpath-imports2. Made Relative Imports the Default
useSubPathImports = true(with inverted logic, this means relative imports)3. Inverted Boolean Logic for Better Semantics
useSubPathImports = truemeans use relative imports (the default)useSubPathImports = falsemeans use subpath importsif (useSubPathImports)calculates relative paths4. Improved Messaging and Presentation
"Relative imports (import preview from '../../.storybook/preview')"and"Subpath imports (import preview from '#.storybook/preview')"Result
Users running the csf-factories migration will now:
This ensures a better migration experience that aligns with the updated documentation and provides users with the information they need to make an informed choice.
Fixes #29836
Original prompt
💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.
Summary by CodeRabbit
Refactor
Documentation
Tests