Automigration: Improve the viewport/backgrounds automigration #32619
Conversation
…d renaming `disable` to `disabled` in viewport stories
…r expression references for `defaultViewport`
valentinpalkovic
added
bug
patch:yes
📝 WalkthroughWalkthroughExported transformStoryFile, removed transformStoryFileSync, integrated printCsf into the transform pipeline used by tests, and updated viewport and backgrounds migration logic; tests now invoke transformStoryFile + printCsf and assert printed CSF output. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Dev as Developer
participant CLI as Automigrate CLI
participant TF as transformStoryFile
participant CSF as printCsf
participant FS as File System
Dev->>CLI: Run automigration (addon-globals-api)
CLI->>TF: Parse source -> produce CsfFile | null
rect rgba(52,152,219,0.08)
TF->>TF: Apply migrations:
note right of TF #eef9ff: viewport: handle literals/member-exprs,\nderive isRotated from defaultOrientation,\nrename disable→disabled\nbackgrounds: values → options
end
TF->>CSF: If CsfFile, print CSF string
CSF-->>CLI: Return printed CSF (string) or null
CLI->>FS: Write updated file if string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
|
View your CI Pipeline Execution ↗ for commit 7a4ae9e
☁️ Nx Cloud last updated this comment at |
…nd enhance viewport handling in story files
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (1)
333-337: Verify the isRotated derivation logic.The logic sets
isRotated = truewhendefaultOrientation.value === 'portrait'. As mentioned in the test file review, this seems counterintuitive since portrait typically represents a non-rotated state. Please confirm this matches the Storybook viewport API semantics.
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (1)
241-242: Consider improving type safety for the options parameter.The
optionsparameter uses theanytype in multiple functions. Consider defining a more specific type based on theAddonGlobalsApiOptionsinterface (lines 25-43) to improve type safety and catch potential issues at compile time.For example:
type TransformOptions = Pick< AddonGlobalsApiOptions, 'needsViewportMigration' | 'needsBackgroundsMigration' | 'viewportsOptions' | 'backgroundsOptions' >;Also applies to: 255-256, 290-291
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts(4 hunks)code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts(3 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/automigrate/fixes/addon-globals-api.tscode/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.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/automigrate/fixes/addon-globals-api.tscode/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.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/automigrate/fixes/addon-globals-api.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/automigrate/fixes/addon-globals-api.test.ts
🧬 Code graph analysis (2)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/ast-utils.ts (4)
getObjectProperty(7-23)removeProperty(26-41)addProperty(44-50)transformValuesToOptions(78-103)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (1)
scripts/utils/tools.ts (1)
dedent(118-118)
⏰ 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 (5)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (2)
858-889: LGTM!The test correctly validates that member expression references (e.g.,
MINIMAL_VIEWPORTS.mobile2) are preserved in the transformed output, matching the PR objective to support imported viewport constants.
891-932: LGTM!The test correctly validates the transformation of
backgrounds.valuestobackgrounds.optionsand the migration ofdefaulttoglobals.backgrounds.value, fulfilling the PR objectives for backgrounds migration.code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (3)
313-325: LGTM!The implementation correctly handles both string literals and member expressions for
defaultViewport, enabling support for imported viewport constants (e.g.,MINIMAL_VIEWPORTS.mobile2) as specified in the PR objectives.
354-366: LGTM!The code correctly removes the deprecated
defaultOrientationproperty after extracting its value and properly renamesdisabletodisabledwhile preserving the boolean value, addressing the PR objectives.
376-387: LGTM!The implementation correctly transforms the
backgrounds.valuesarray into anoptionsobject using thetransformValuesToOptionshelper, fulfilling the PR objective to migrate the backgrounds structure.
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts
Outdated
Show resolved
Hide resolved
…Csf and streamline story transformations
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (1)
265-273: LGTM with a suggestion for type safety.Exporting
transformStoryFilemakes it testable and reusable, which aligns with the PR objectives. However, theviewportsOptionsandbackgroundsOptionsparameters are typed asany, which reduces type safety.Consider defining explicit interfaces for these options:
+interface ViewportOptions { + defaultViewport?: string; + viewports?: Expression; + defaultOrientation?: string; + disable?: boolean; +} + +interface BackgroundsOptions { + default?: string; + values?: Expression; + disable?: boolean; +} + export function transformStoryFile( source: string, options: { needsViewportMigration: boolean; needsBackgroundsMigration: boolean; - viewportsOptions: any; - backgroundsOptions: any; + viewportsOptions: ViewportOptions | undefined; + backgroundsOptions: BackgroundsOptions | undefined; } ): CsfFile | null {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts(17 hunks)code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts
🧰 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/fixes/addon-globals-api.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/fixes/addon-globals-api.ts
🧬 Code graph analysis (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/ast-utils.ts (4)
getObjectProperty(7-23)removeProperty(26-41)addProperty(44-50)transformValuesToOptions(78-103)
⏰ 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 (5)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (5)
293-305: LGTM! Handles imported viewport constants.The code correctly handles
defaultViewportas both string literals (e.g.,'mobile1') and member expressions (e.g.,MINIMAL_VIEWPORTS.mobile2), which addresses one of the key objectives from the linked issue.
334-346: LGTM! Correctly handles defaultOrientation removal and disable→disabled rename.The code properly:
- Removes
defaultOrientationafter processing it intoisRotated(line 336)- Renames
disabletodisabledwhile preserving the boolean value (lines 342-344)These transformations align with the PR objectives for viewport parameter migration.
356-367: LGTM! Correctly transforms backgrounds values to options.The code properly transforms the
valuesarray into anoptionsobject using thetransformValuesToOptionshelper, which addresses the PR objective for preserving and migrating the values array.
375-378: No action needed:getKeyFromNamealready guards against an undefinedvaluesArrayand falls back to returning the generated key.
313-318: isRotated logic is correct.defaultOrientation: 'portrait'intentionally setsisRotated: true, matching the viewport add-on’s semantics and existing tests.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (1)
834-868: Verify the test expectation aligns with implementation.A past review flagged the
isRotated: truefordefaultOrientation: 'portrait'(line 863) as incorrect, stating that portrait should useisRotated: falseand landscape should usetrue. That comment was marked as addressed in commit c56a9aa.However, the test still expects
isRotated: truefor portrait, which matches the implementation inaddon-globals-api.ts(lines 309-314). Please confirm this semantic is correct per Storybook's viewport globals API documentation.Storybook viewport globals API: when should isRotated be true vs false for defaultOrientation portrait vs landscape?
🧹 Nitpick comments (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (1)
15-26: Mock setup violates coding guidelines.The mock calls at lines 15 and 17 don't use the
spy: trueoption, and mock implementations are at the top level rather than inbeforeEachblocks. As per coding guidelines, all mocks should usevi.mock(..., { spy: true })and implementations should be inbeforeEach.However, these mocks appear to be intentionally static (the fs mock is configured per-test via
__setMockFiles, and the recast mock forces quote normalization globally). If this is the intended pattern, consider documenting why these mocks are static rather than per-test.Based on coding guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts(17 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
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/automigrate/fixes/addon-globals-api.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/automigrate/fixes/addon-globals-api.test.ts
**/*.{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/fixes/addon-globals-api.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/automigrate/fixes/addon-globals-api.test.ts
🧠 Learnings (5)
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in beforeEach blocks
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mock() with the spy: true option for all package and file mocks in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Use vi.mocked() to access and implement mock behaviors
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts
📚 Learning: 2025-09-17T08:11:47.197Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.197Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Document complex mock behaviors
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts
📚 Learning: 2025-09-17T08:11:47.196Z
Learnt from: CR
PR: storybookjs/storybook#0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-09-17T08:11:47.196Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required properties and methods that the test subject uses
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts
🧬 Code graph analysis (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (2)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (3)
transformStoryFile(265-467)transformStoryFileSync(236-247)transformStoryFile(285-441)code/core/src/csf-tools/CsfFile.ts (1)
printCsf(1040-1042)
⏰ 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/automigrate/fixes/addon-globals-api.test.ts (2)
62-71: LGTM!The transform function correctly uses
transformStoryFile+printCsfto replace the deprecatedtransformStoryFileSync. The implementation properly handles the null case and extracts the.codeproperty from the print result.
870-936: LGTM!The new test cases correctly verify:
- Member expression preservation for viewport references (lines 870-898) - ensures expressions like
MINIMAL_VIEWPORTS.mobile2are preserved in the migrated globals- Backgrounds values → options transformation (lines 900-936) - confirms the migration properly transforms the values array to an options object with kebab-cased keys
Both tests align with the PR objectives to handle viewport constants and background values migration.
| t.objectProperty(t.identifier('value'), t.stringLiteral(defaultViewport.value)), | ||
| t.objectProperty(t.identifier('isRotated'), t.booleanLiteral(false)), | ||
| t.objectProperty(t.identifier('value'), viewportValue), | ||
| t.objectProperty(t.identifier('isRotated'), t.booleanLiteral(isRotated)), |
There was a problem hiding this comment.
if there was no defaultOrientation ever defined, does it make sense to add isRotated to every story?
valentinpalkovic
deleted the
valentin/improve-viewport-background-automigration
branch
…background-automigration Automigration: Improve the viewport/backgrounds automigration (cherry picked from commit a47e297)
github-actions
bot
added
the
patch:done
ndelangen
removed
the
patch:yes
Closes #32605
What I did
I fixed the first and the third issue, which were reported. The second one is too complex to support it and is also considered rather edge-casey. The rest of the problems were wrongly reported, because the outcome of the transformations is actually okay!
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 pull request has been released as version
0.0.0-pr-32619-sha-7a4ae9e3. 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-32619-sha-7a4ae9e3valentin/improve-viewport-background-automigration7a4ae9e31759915936)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=32619Summary by CodeRabbit
New Features
Bug Fixes
Tests