Automigrations: Add automigration for viewport and backgrounds #31614
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
3 files reviewed, 5 comments
Edit PR Review Bot Settings | Greptile
| { name: 'Light', value: '#F8F8F8' }, | ||
| { name: 'Dark', value: '#F8F8F8' } | ||
| ], |
There was a problem hiding this comment.
logic: Both Light and Dark backgrounds have the same value '#F8F8F8' - likely a test data error
| { name: 'Light', value: '#F8F8F8' }, | |
| { name: 'Dark', value: '#F8F8F8' } | |
| ], | |
| { name: 'Light', value: '#F8F8F8' }, | |
| { name: 'Dark', value: '#333333' } | |
| ], |
| export const NoParams = {}; | ||
| export const ExistingGlobals = { globals: { backgrounds: { value: 'dark' } } }; | ||
| export const ExistingDisabled = { parameters: { backgrounds: { disabled: true } } }; |
There was a problem hiding this comment.
style: Extra spaces at the start of these lines create inconsistent formatting
| export const NoParams = {}; | |
| export const ExistingGlobals = { globals: { backgrounds: { value: 'dark' } } }; | |
| export const ExistingDisabled = { parameters: { backgrounds: { disabled: true } } }; | |
| export const NoParams = {}; | |
| export const ExistingGlobals = { globals: { backgrounds: { value: 'dark' } } }; | |
| export const ExistingDisabled = { parameters: { backgrounds: { disabled: true } } }; |
| const optionKey = addonName === 'viewport' ? field : field; | ||
| options[optionKey] = value; |
There was a problem hiding this comment.
logic: Redundant field name assignment. optionKey = field doesn't transform the field name as the comment suggests
| const optionKey = addonName === 'viewport' ? field : field; | |
| options[optionKey] = value; | |
| const optionKey = field; | |
| options[optionKey] = value; |
| ['initialGlobals', 'backgrounds', 'value'], | ||
| getKeyFromName(backgroundsOptions.values as ArrayExpression, backgroundsOptions.default) | ||
| ); |
There was a problem hiding this comment.
logic: Potential error if backgroundsOptions.values is undefined but default exists
|
|
||
| if (needsViewportMigration) { | ||
| // Get the viewport parameter object | ||
| const viewports = getFieldNode(['parameters', 'viewport', 'viewports']) as ObjectExpression; |
There was a problem hiding this comment.
logic: Check if viewports exists before casting to ObjectExpression to prevent runtime errors
| const viewports = getFieldNode(['parameters', 'viewport', 'viewports']) as ObjectExpression; | |
| const viewportsNode = getFieldNode(['parameters', 'viewport', 'viewports']); | |
| if (!viewportsNode || !t.isObjectExpression(viewportsNode)) return; | |
| const viewports = viewportsNode as ObjectExpression; |
|
View your CI Pipeline Execution ↗ for commit fc539b3
☁️ Nx Cloud last updated this comment at |
| previewConfig.removeField(['parameters', 'backgrounds', 'values']); | ||
| addProperty( |
There was a problem hiding this comment.
This could be done differently, by just renaming the key instead of removing everything and re-adding like it's done here:
…s-backgrounds-automigrations
|
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. Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 187 | 187 | 0 |
| Self size | 905 KB | 920 KB | 🚨 +15 KB 🚨 |
| Dependency size | 79.78 MB | 79.78 MB | 0 B |
| Bundle Size Analyzer | Link | Link |
…tion logic with storiesPaths parameter
…ns special characters in value name
… improve formatting for better readability
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (2)
101-105: Redundant field remap — simplify assignment
optionKey = addonName === 'viewport' ? field : fieldis a no-op. Useoptions[field] = value;.- // Convert field names if necessary (maintaining the expected output structure) - const optionKey = addonName === 'viewport' ? field : field; - options[optionKey] = value; + options[field] = value;
146-158: Avoid unsafe cast; support identifiers/dynamic values for viewport options
viewportsmay be an Identifier (e.g.,INITIAL_VIEWPORTS), not an ObjectExpression. Guard and type asExpression.- // Get the viewport parameter object - const viewports = getFieldNode(['parameters', 'viewport', 'viewports']) as ObjectExpression; + // Get the viewport parameter expression (can be object literal or identifier) + const viewportsExpr = getFieldNode(['parameters', 'viewport', 'viewports']) as Expression | undefined; ... - if (viewportsOptions?.viewports) { + if (viewportsOptions?.viewports && viewportsExpr) { // Remove the old viewports property previewConfig.removeField(['parameters', 'viewport', 'viewports']); addProperty( getFieldNode(['parameters', 'viewport']) as ObjectExpression, 'options', - viewports + viewportsExpr ); }
🧹 Nitpick comments (14)
code/lib/cli-storybook/src/automigrate/helpers/ast-utils.ts (4)
49-50: Safer property insertion: choose identifier vs string key based on validity
addPropertyalways usest.identifier(propertyName). If a caller passes a non-identifier (e.g., "background-color"), this generates invalid AST. Prefer identifier only when valid, else string literal.- obj.properties.push(t.objectProperty(t.identifier(propertyName), value)); + const isValidId = /^[A-Za-z_$][A-Za-z0-9_$]*$/.test(propertyName); + const key = isValidId ? t.identifier(propertyName) : t.stringLiteral(propertyName); + obj.properties.push(t.objectProperty(key, value));
105-122: Deduplicate key-normalization logic into a shared helperBoth
transformValuesToOptionsandgetKeyFromNameimplementtoLowerCase().replace(/\s+/g, '_'). Extract anormalizeKey(name: string)helper and reuse it to avoid drift.+function normalizeKey(name: string) { + return name.toLowerCase().replace(/\s+/g, '_'); +} ... - const key = nameProperty.value.toLowerCase().replace(/\s+/g, '_'); + const key = normalizeKey(nameProperty.value); ... - return name.toLowerCase().replace(/\s+/g, '_'); + return normalizeKey(name);
197-202: Add a type guard before treatingparametersas an object
getObjectPropertyreturnsExpression | undefined, but the code casts tot.ObjectExpression. Guard witht.isObjectExpressionto avoid mis-handling identifiers or spreads.- const parameters = getObjectProperty(storyObject, 'parameters') as t.ObjectExpression; - if (parameters) { - return transformParameters(parameters, storyObject, storyName, csf); + const parameters = getObjectProperty(storyObject, 'parameters'); + if (parameters && t.isObjectExpression(parameters)) { + return transformParameters(parameters, storyObject, storyName, csf); }
155-186: Access to CSF internals: consider public accessors to avoid tight couplingUsing
csf._storyExportsandcsf._metaPathcouples this helper to private internals. IfCsfFileevolves, this may break migrations.
- Expose stable accessors in
CsfFile(e.g.,getStoryExportEntries(),getMetaNode()), and consume them here. As per coding guidelines.code/lib/cli-storybook/src/automigrate/helpers/addon-a11y-parameters.ts (3)
15-21: Harden type checks and support string-literal keysAdd guards for
parameters/a11ybeing object expressions. Also handle'element'when expressed as a string-literal key.- const parametersValue = getObjectProperty(obj, 'parameters') as t.ObjectExpression | undefined; - if (parametersValue) { - const a11yValue = getObjectProperty(parametersValue, 'a11y') as t.ObjectExpression | undefined; - if (a11yValue) { + const parametersValue = getObjectProperty(obj, 'parameters'); + if (parametersValue && t.isObjectExpression(parametersValue)) { + const a11yValue = getObjectProperty(parametersValue, 'a11y'); + if (a11yValue && t.isObjectExpression(a11yValue)) { const elementProp = a11yValue.properties.find( - (prop) => - t.isObjectProperty(prop) && t.isIdentifier(prop.key) && prop.key.name === 'element' + (prop) => + t.isObjectProperty(prop) && + ((t.isIdentifier(prop.key) && prop.key.name === 'element') || + (t.isStringLiteral(prop.key) && prop.key.value === 'element')) ); if (elementProp && t.isObjectProperty(elementProp)) { elementProp.key = t.identifier('context'); return true; }
38-41: Remove unused callback params to satisfy strict lint rules
storyNameandcsfare unused. Either omit or prefix with_.- let hasChanges = transformStories(parsed, (storyObject, storyName, csf) => { + let hasChanges = transformStories(parsed, (storyObject) => { return migrateA11yParameters(storyObject); });
44-67: HandleStory.parameters.a11y = {...}assignments in CSF2The CSF2 loop only rewrites when the RHS is the whole
parametersobject. It missesStory.parameters.a11y = { element: ... }. Extend detection to nested member expressions and renameelement→contextin that RHS object.- if ( - isStoryAnnotation(statement, parsed._storyExports) && - t.isExpressionStatement(statement) && - t.isAssignmentExpression(statement.expression) && - t.isObjectExpression(statement.expression.right) - ) { - const parameters = statement.expression.right.properties; - parameters.forEach((param) => { - if (t.isObjectProperty(param) && t.isIdentifier(param.key) && param.key.name === 'a11y') { - const a11yValue = param.value as t.ObjectExpression; - const elementProp = a11yValue.properties.find( - (prop) => - t.isObjectProperty(prop) && t.isIdentifier(prop.key) && prop.key.name === 'element' - ); - if (elementProp && t.isObjectProperty(elementProp)) { - elementProp.key = t.identifier('context'); - hasChanges = true; - } - } - }); - } + if (isStoryAnnotation(statement, parsed._storyExports) && t.isExpressionStatement(statement)) { + const assign = statement.expression; + if (t.isAssignmentExpression(assign)) { + // Case 1: Story.parameters = { a11y: { element: ... } } + if (t.isObjectExpression(assign.right)) { + assign.right.properties.forEach((param) => { + if ( + t.isObjectProperty(param) && + ((t.isIdentifier(param.key) && param.key.name === 'a11y') || + (t.isStringLiteral(param.key) && param.key.value === 'a11y')) && + t.isObjectExpression(param.value) + ) { + const elementProp = param.value.properties.find( + (prop) => + t.isObjectProperty(prop) && + ((t.isIdentifier(prop.key) && prop.key.name === 'element') || + (t.isStringLiteral(prop.key) && prop.key.value === 'element')) + ); + if (elementProp && t.isObjectProperty(elementProp)) { + elementProp.key = t.identifier('context'); + hasChanges = true; + } + } + }); + } + // Case 2: Story.parameters.a11y = { element: ... } + if ( + t.isMemberExpression(assign.left) && + t.isMemberExpression(assign.left.object) && + ((t.isIdentifier(assign.left.property) && assign.left.property.name === 'a11y') || + (t.isStringLiteral(assign.left.property) && assign.left.property.value === 'a11y')) && + t.isObjectExpression(assign.right) + ) { + const elementProp = assign.right.properties.find( + (prop) => + t.isObjectProperty(prop) && + ((t.isIdentifier(prop.key) && prop.key.name === 'element') || + (t.isStringLiteral(prop.key) && prop.key.value === 'element')) + ); + if (elementProp && t.isObjectProperty(elementProp)) { + elementProp.key = t.identifier('context'); + hasChanges = true; + } + } + } + }code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (4)
214-232: Preview-level cleanup: remove empty objects after migrationAfter moving fields, remove now-empty
parameters.viewport/backgrounds(andparametersif empty) to match the “cleanup empty objects” goal.if (!dryRun) { await writeFile(result.previewConfigPath, formatConfig(previewConfig)); } // Update stories if (needsViewportMigration || needsBackgroundsMigration) { + // Cleanup: drop empty groups + const paramsNode = getFieldNode(['parameters']) as ObjectExpression | undefined; + const viewportNode = getFieldNode(['parameters', 'viewport']) as ObjectExpression | undefined; + const backgroundsNode = getFieldNode(['parameters', 'backgrounds']) as ObjectExpression | undefined; + if (viewportNode && t.isObjectExpression(viewportNode) && viewportNode.properties.length === 0) { + previewConfig.removeField(['parameters', 'viewport']); + } + if (backgroundsNode && t.isObjectExpression(backgroundsNode) && backgroundsNode.properties.length === 0) { + previewConfig.removeField(['parameters', 'backgrounds']); + } + if (paramsNode && t.isObjectExpression(paramsNode) && paramsNode.properties.length === 0) { + previewConfig.removeField(['parameters']); + } + await transformStoryFiles( storiesPaths, { needsViewportMigration, needsBackgroundsMigration, viewportsOptions, backgroundsOptions, }, dryRun ); }
235-247: Tighten helper typingsAvoid
anyin public helpers; reuse option shapes for clarity and safety.export function transformStoryFileSync( source: string, options: { needsViewportMigration: boolean; needsBackgroundsMigration: boolean; - viewportsOptions: any; - backgroundsOptions: any; + viewportsOptions: AddonGlobalsApiOptions['viewportsOptions']; + backgroundsOptions: AddonGlobalsApiOptions['backgroundsOptions']; } ) {
250-259: Propagate types into batch transformerSame here; replace
anywith the specific option shapes.async function transformStoryFiles( files: string[], options: { needsViewportMigration: boolean; needsBackgroundsMigration: boolean; - viewportsOptions: any; - backgroundsOptions: any; + viewportsOptions: AddonGlobalsApiOptions['viewportsOptions']; + backgroundsOptions: AddonGlobalsApiOptions['backgroundsOptions']; }, dryRun: boolean ): Promise<Array<{ file: string; error: Error }>> {
74-90: Consider migrating defaults to initialGlobals even when options already existIf
parameters.backgrounds.optionsis present but legacydefault/disableare also present, you currently skip migration entirely. Consider still liftingdefaulttoinitialGlobalsand renamingdisabletodisabledto normalize mixed configs.code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (3)
12-14: Enable spy mode on fs mock to comply with mocking guidelinesAdd
{ spy: true }.-vi.mock('node:fs/promises', async () => import('../../../../../__mocks__/fs/promises')); +vi.mock('node:fs/promises', async () => import('../../../../../__mocks__/fs/promises'), { spy: true });As per coding guidelines.
304-331: Add test for preview rename when disable is falsePreview migration should rename
disable: falsetodisabled: false, mirroring story-level behavior.@@ it('should rename backgrounds disable property to disabled', async () => { @@ }); + + it('should rename backgrounds disable: false to disabled: false (preview)', async () => { + const { previewFileContent } = await runMigrationAndGetTransformFn(dedent` + export default { + parameters: { + backgrounds: { + values: [{ name: 'Light', value: '#F8F8F8' }], + disable: false + } + } + } + `); + expect(previewFileContent).toContain('disabled: false'); + expect(previewFileContent).not.toContain('disable:'); + });
28-41: Minor: simplify fs mock setupYou can assign mock files without the verbose generic;
vi.mocked(fsp as any).__setMockFiles({...})is sufficient.- vi.mocked<typeof import('../../../../../__mocks__/fs/promises')>(fsp as any).__setMockFiles({ + vi.mocked(fsp as any).__setMockFiles({ [previewConfigPath]: previewContents, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts(1 hunks)code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts(1 hunks)code/lib/cli-storybook/src/automigrate/fixes/index.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/addon-a11y-parameters.ts(4 hunks)code/lib/cli-storybook/src/automigrate/helpers/ast-utils.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/automigrate/fixes/addon-globals-api.tscode/lib/cli-storybook/src/automigrate/helpers/ast-utils.tscode/lib/cli-storybook/src/automigrate/fixes/index.tscode/lib/cli-storybook/src/automigrate/helpers/addon-a11y-parameters.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/helpers/ast-utils.tscode/lib/cli-storybook/src/automigrate/fixes/index.tscode/lib/cli-storybook/src/automigrate/helpers/addon-a11y-parameters.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 (5)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (3)
code/core/src/csf-tools/ConfigFile.ts (5)
ConfigFile(147-1160)loadConfig(1162-1165)getFieldNode(333-341)getFieldValue(353-362)formatConfig(1167-1169)code/lib/cli-storybook/src/automigrate/helpers/ast-utils.ts (6)
addProperty(44-50)removeProperty(26-41)transformValuesToOptions(78-103)getKeyFromName(106-122)transformStoryParameters(189-205)getObjectProperty(7-23)code/core/src/csf-tools/CsfFile.ts (4)
formatCsf(1027-1037)writeCsf(1049-1056)CsfFile(277-1006)loadCsf(1021-1025)
code/lib/cli-storybook/src/automigrate/helpers/ast-utils.ts (4)
code/core/src/manager/components/sidebar/mockdata.large.ts (1)
index(12-26726)code/core/src/core-server/mocking-utils/esmWalker.ts (1)
Node(45-45)code/core/src/storybook-error.ts (1)
name(56-60)code/core/src/csf-tools/CsfFile.ts (1)
CsfFile(277-1006)
code/lib/cli-storybook/src/automigrate/fixes/index.ts (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (1)
addonGlobalsApi(51-233)
code/lib/cli-storybook/src/automigrate/helpers/addon-a11y-parameters.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/ast-utils.ts (2)
getObjectProperty(7-23)transformStories(156-186)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (1)
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts (3)
check(55-129)addonGlobalsApi(51-233)transformStoryFileSync(236-247)
🔇 Additional comments (3)
code/lib/cli-storybook/src/automigrate/helpers/addon-a11y-parameters.ts (1)
69-70: LGTM on return semanticsReturning
parsedonly whenhasChangesis true is correct and side‑effect free.code/lib/cli-storybook/src/automigrate/fixes/index.ts (1)
24-31: Confirm fix ordering: addonGlobalsApi vs initialGlobals
addonGlobalsApiwritesinitialGlobals.viewport/backgrounds. SinceinitialGlobalsruns earlier, ensure it doesn’t reformat/overwrite fields set byaddonGlobalsApiin the same run order.
- Please confirm
initialGlobalsis idempotent and won’t stomp the structures created byaddonGlobalsApi. If needed, moveaddonGlobalsApibeforeinitialGlobalsor guard writes ininitialGlobals.code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.test.ts (1)
15-24: Invalid mock signature for vitest — use module id string and spy mode
vi.mock(import('storybook/internal/babel'), ...)is incorrect. Pass the module path string and enablespy: trueper guidelines.-vi.mock(import('storybook/internal/babel'), async (actualImport) => { - const actual = await actualImport(); +vi.mock('storybook/internal/babel', async (importOriginal) => { + const actual = await importOriginal(); return { ...actual, recast: { ...actual.recast, print: (ast, options) => actual.recast.print(ast, { ...options, quote: 'single' }), }, }; -}); +}, { spy: true });As per coding guidelines.
⛔ Skipped due to learnings
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 testsLearnt 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 behaviorsLearnt 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} : Avoid direct function mocking without vi.mocked()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 type and access mocked functionsLearnt 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 behaviorsLearnt 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} : Avoid mocking without the spy: true optionLearnt 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} : Use type-safe mocking with vi.mocked()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} : Avoid inline mock implementations within test casesLearnt 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} : Keep mock implementations simple and focusedLearnt 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} : Avoid mocking only a subset of required dependencies
| describe('run - preview file', () => { | ||
| it('should migrate viewport configuration correctly', async () => { | ||
| const { previewFileContent } = await runMigrationAndGetTransformFn(dedent` | ||
| export default { | ||
| parameters: { | ||
| viewport: { | ||
| viewports: { | ||
| mobile: { name: 'Mobile', width: '320px', height: '568px' }, | ||
| tablet: { name: 'Tablet', width: '768px', height: '1024px' } | ||
| }, | ||
| defaultViewport: 'mobile' | ||
| } | ||
| } | ||
| } | ||
| `); | ||
|
|
||
| expect(previewFileContent).toMatchInlineSnapshot(dedent` | ||
| "export default { | ||
| parameters: { | ||
| viewport: { | ||
| options: { | ||
| mobile: { name: 'Mobile', width: '320px', height: '568px' }, | ||
| tablet: { name: 'Tablet', width: '768px', height: '1024px' } | ||
| } | ||
| } | ||
| }, | ||
|
|
||
| initialGlobals: { | ||
| viewport: { | ||
| value: 'mobile', | ||
| isRotated: false | ||
| } | ||
| } | ||
| };" | ||
| `); | ||
| }); |
There was a problem hiding this comment.
Add coverage for dynamic backgrounds values (Identifier) to prevent data loss
Current tests don’t cover parameters.backgrounds.values = backgroundValues. Add a preview migration test asserting we do not drop values or generate an empty options object when values aren’t a literal array.
@@
describe('run - preview file', () => {
+ it('should not drop dynamic backgrounds values (identifier)', async () => {
+ const { previewFileContent } = await runMigrationAndGetTransformFn(dedent`
+ const backgroundValues = [
+ { name: 'Light', value: '#F8F8F8' },
+ { name: 'Dark', value: '#333333' }
+ ];
+ export default {
+ parameters: {
+ backgrounds: {
+ values: backgroundValues,
+ default: 'Light'
+ }
+ }
+ }
+ `);
+ expect(previewFileContent).toContain('backgrounds: {');
+ // Keep values as-is (no empty options), but lift default to initialGlobals
+ expect(previewFileContent).toContain('values: backgroundValues');
+ expect(previewFileContent).toContain('initialGlobals');
+ expect(previewFileContent).toContain("value: 'light'");
+ });📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| describe('run - preview file', () => { | |
| it('should migrate viewport configuration correctly', async () => { | |
| const { previewFileContent } = await runMigrationAndGetTransformFn(dedent` | |
| export default { | |
| parameters: { | |
| viewport: { | |
| viewports: { | |
| mobile: { name: 'Mobile', width: '320px', height: '568px' }, | |
| tablet: { name: 'Tablet', width: '768px', height: '1024px' } | |
| }, | |
| defaultViewport: 'mobile' | |
| } | |
| } | |
| } | |
| `); | |
| expect(previewFileContent).toMatchInlineSnapshot(dedent` | |
| "export default { | |
| parameters: { | |
| viewport: { | |
| options: { | |
| mobile: { name: 'Mobile', width: '320px', height: '568px' }, | |
| tablet: { name: 'Tablet', width: '768px', height: '1024px' } | |
| } | |
| } | |
| }, | |
| initialGlobals: { | |
| viewport: { | |
| value: 'mobile', | |
| isRotated: false | |
| } | |
| } | |
| };" | |
| `); | |
| }); | |
| describe('run - preview file', () => { | |
| it('should not drop dynamic backgrounds values (identifier)', async () => { | |
| const { previewFileContent } = await runMigrationAndGetTransformFn(dedent` | |
| const backgroundValues = [ | |
| { name: 'Light', value: '#F8F8F8' }, | |
| { name: 'Dark', value: '#333333' } | |
| ]; | |
| export default { | |
| parameters: { | |
| backgrounds: { | |
| values: backgroundValues, | |
| default: 'Light' | |
| } | |
| } | |
| } | |
| `); | |
| expect(previewFileContent).toContain('backgrounds: {'); | |
| // Keep values as-is (no empty options), but lift default to initialGlobals | |
| expect(previewFileContent).toContain('values: backgroundValues'); | |
| expect(previewFileContent).toContain('initialGlobals'); | |
| expect(previewFileContent).toContain("value: 'light'"); | |
| }); | |
| it('should migrate viewport configuration correctly', async () => { | |
| const { previewFileContent } = await runMigrationAndGetTransformFn(dedent` | |
| export default { | |
| parameters: { | |
| viewport: { | |
| viewports: { | |
| mobile: { name: 'Mobile', width: '320px', height: '568px' }, | |
| tablet: { name: 'Tablet', width: '768px', height: '1024px' } | |
| }, | |
| defaultViewport: 'mobile' | |
| } | |
| } | |
| } | |
| `); | |
| expect(previewFileContent).toMatchInlineSnapshot(dedent` | |
| "export default { | |
| parameters: { | |
| viewport: { | |
| options: { | |
| mobile: { name: 'Mobile', width: '320px', height: '568px' }, | |
| tablet: { name: 'Tablet', width: '768px', height: '1024px' } | |
| } | |
| } | |
| }, | |
| initialGlobals: { | |
| viewport: { | |
| value: 'mobile', | |
| isRotated: false | |
| } | |
| } | |
| };" | |
| `); | |
| }); |
| if (backgroundsOptions?.values) { | ||
| // Transform values array to options object | ||
| const optionsObject = transformValuesToOptions( | ||
| backgroundsOptions.values as ArrayExpression | ||
| ); | ||
|
|
||
| // Remove the old values property | ||
| previewConfig.removeField(['parameters', 'backgrounds', 'values']); | ||
| addProperty( | ||
| getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, | ||
| 'options', | ||
| optionsObject | ||
| ); | ||
| } |
There was a problem hiding this comment.
Don’t destructure dynamic backgrounds values into an empty options object
When backgrounds.values is not a literal array (e.g., an Identifier), transformValuesToOptions() returns {}, and you remove the original values, losing all options. Only transform and remove values when it’s an ArrayExpression; otherwise, keep values as-is.
- if (backgroundsOptions?.values) {
- // Transform values array to options object
- const optionsObject = transformValuesToOptions(
- backgroundsOptions.values as ArrayExpression
- );
-
- // Remove the old values property
- previewConfig.removeField(['parameters', 'backgrounds', 'values']);
- addProperty(
- getFieldNode(['parameters', 'backgrounds']) as ObjectExpression,
- 'options',
- optionsObject
- );
- }
+ if (backgroundsOptions?.values) {
+ const valuesNode = backgroundsOptions.values as Expression;
+ if (t.isArrayExpression(valuesNode)) {
+ // Literal array: convert to keyed options
+ const optionsObject = transformValuesToOptions(valuesNode);
+ previewConfig.removeField(['parameters', 'backgrounds', 'values']);
+ addProperty(
+ getFieldNode(['parameters', 'backgrounds']) as ObjectExpression,
+ 'options',
+ optionsObject
+ );
+ }
+ // Non-array expressions (identifiers, calls) are left untouched to avoid data loss.
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (backgroundsOptions?.values) { | |
| // Transform values array to options object | |
| const optionsObject = transformValuesToOptions( | |
| backgroundsOptions.values as ArrayExpression | |
| ); | |
| // Remove the old values property | |
| previewConfig.removeField(['parameters', 'backgrounds', 'values']); | |
| addProperty( | |
| getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, | |
| 'options', | |
| optionsObject | |
| ); | |
| } | |
| if (backgroundsOptions?.values) { | |
| const valuesNode = backgroundsOptions.values as Expression; | |
| if (t.isArrayExpression(valuesNode)) { | |
| // Literal array: convert to keyed options | |
| const optionsObject = transformValuesToOptions(valuesNode); | |
| previewConfig.removeField(['parameters', 'backgrounds', 'values']); | |
| addProperty( | |
| getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, | |
| 'options', | |
| optionsObject | |
| ); | |
| } | |
| // Non-array expressions (identifiers, calls) are left untouched to avoid data loss. | |
| } |
🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts around
lines 175 to 188, the code unconditionally transforms and removes
backgrounds.values which loses non-literal values (e.g., Identifiers); change
the guard to only transform and remove when backgroundsOptions.values is an
ArrayExpression (use the AST check you already use elsewhere, e.g.,
isArrayExpression or node.type === 'ArrayExpression'); otherwise leave the
original values field intact (do not call transformValuesToOptions, do not
remove the values property, and do not add options).
| // If disable exists, rename to disabled | ||
| if (backgroundsOptions?.disable === true) { | ||
| // Remove the old disable property | ||
| removeProperty(getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, 'disable'); | ||
|
|
||
| addProperty( | ||
| getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, | ||
| 'disabled', | ||
| t.booleanLiteral(true) | ||
| ); | ||
| } |
There was a problem hiding this comment.
Rename backgrounds.disable for both true and false
Preview migration currently handles only disable === true. Rename regardless of boolean value to keep shape consistent with stories.
- // If disable exists, rename to disabled
- if (backgroundsOptions?.disable === true) {
- // Remove the old disable property
- removeProperty(getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, 'disable');
-
- addProperty(
- getFieldNode(['parameters', 'backgrounds']) as ObjectExpression,
- 'disabled',
- t.booleanLiteral(true)
- );
- }
+ // If disable exists, rename to disabled (preserve value)
+ if (typeof backgroundsOptions?.disable === 'boolean') {
+ removeProperty(getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, 'disable');
+ addProperty(
+ getFieldNode(['parameters', 'backgrounds']) as ObjectExpression,
+ 'disabled',
+ t.booleanLiteral(Boolean(backgroundsOptions.disable))
+ );
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // If disable exists, rename to disabled | |
| if (backgroundsOptions?.disable === true) { | |
| // Remove the old disable property | |
| removeProperty(getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, 'disable'); | |
| addProperty( | |
| getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, | |
| 'disabled', | |
| t.booleanLiteral(true) | |
| ); | |
| } | |
| // If disable exists, rename to disabled (preserve value) | |
| if (typeof backgroundsOptions?.disable === 'boolean') { | |
| // Remove the old disable property | |
| removeProperty( | |
| getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, | |
| 'disable' | |
| ); | |
| // Add the new 'disabled' property with the original boolean value | |
| addProperty( | |
| getFieldNode(['parameters', 'backgrounds']) as ObjectExpression, | |
| 'disabled', | |
| t.booleanLiteral(Boolean(backgroundsOptions.disable)) | |
| ); | |
| } |
🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.ts around
lines 201 to 211, the migration only renames backgrounds.disable when it is
true; change it to rename for both true and false by checking for the presence
of the disable property (e.g., backgroundsOptions?.disable !== undefined or
typeof === 'boolean'), remove the old 'disable' property and add 'disabled' with
the same boolean value (use t.booleanLiteral(Boolean(value)) or the actual
boolean) so the shape matches stories regardless of the disable value.
| if (t.isStringLiteral(nameProperty)) { | ||
| const key = nameProperty.value.toLowerCase().replace(/\s+/g, '_'); | ||
|
|
||
| // For complex names with dots, brackets, or other special characters, use string literal | ||
| // For simple names, use identifier | ||
| const keyNode = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(key) | ||
| ? t.identifier(key) | ||
| : t.stringLiteral(nameProperty.value); | ||
|
|
||
| optionsObject.properties.push(t.objectProperty(keyNode, element)); | ||
| } |
There was a problem hiding this comment.
Backgrounds key mismatch: use normalized key for both identifier and string-literal paths
When name is not a valid identifier, you build key but then use t.stringLiteral(nameProperty.value), causing options to use the original name (e.g., "Light Blue") while getKeyFromName returns light_blue. This breaks lookups for initialGlobals.backgrounds.value.
Fix: use the normalized key in both branches.
- const keyNode = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(key)
- ? t.identifier(key)
- : t.stringLiteral(nameProperty.value);
+ const keyNode = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(key)
+ ? t.identifier(key)
+ : t.stringLiteral(key);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (t.isStringLiteral(nameProperty)) { | |
| const key = nameProperty.value.toLowerCase().replace(/\s+/g, '_'); | |
| // For complex names with dots, brackets, or other special characters, use string literal | |
| // For simple names, use identifier | |
| const keyNode = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(key) | |
| ? t.identifier(key) | |
| : t.stringLiteral(nameProperty.value); | |
| optionsObject.properties.push(t.objectProperty(keyNode, element)); | |
| } | |
| if (t.isStringLiteral(nameProperty)) { | |
| const key = nameProperty.value.toLowerCase().replace(/\s+/g, '_'); | |
| // For complex names with dots, brackets, or other special characters, use string literal | |
| // For simple names, use identifier | |
| const keyNode = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/.test(key) | |
| ? t.identifier(key) | |
| : t.stringLiteral(key); | |
| optionsObject.properties.push(t.objectProperty(keyNode, element)); | |
| } |
🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/helpers/ast-utils.ts around lines 87
to 97, the code builds a normalized key (key) but uses nameProperty.value when
creating the string literal branch, causing mismatched keys; change the
string-literal branch to use the normalized key (i.e., t.stringLiteral(key)) so
both identifier and string-literal paths use the same normalized key, and ensure
the objectProperty is created with that normalized keyNode.
valentinpalkovic
deleted the
valentin/add-viewports-backgrounds-automigrations
branch
valentinpalkovic
added
the
patch:yes
…kgrounds-automigrations Automigrations: Add automigration for viewport and backgrounds (cherry picked from commit 346ccac)
|
Here are a few issues from QA, some might be edge case 1- The properties before: export const Mobile: Story = {
parameters: {
viewport: {
defaultOrientation: 'portrait',
defaultViewport: 'iphonex',
disable: true,
},
},
}after: export const Mobile: Story = {
parameters: {
viewport: {
defaultOrientation: 'portrait',
disable: true
},
},
globals: {
viewport: {
value: "iphonex",
isRotated: false
}
}
}2- const defined viewport parameter does not get transformed const viewport = {
defaultViewport: 'iphonex',
}
export const Mobile: Story = {
parameters: {
viewport
},
}3- importing a value from viewports addon to a story does not get transformed import { MINIMAL_VIEWPORTS } from 'storybook/viewport'
export const Mobile: Story = {
parameters: {
viewport: {
defaultViewport: MINIMAL_VIEWPORTS.mobile2
},
},
} |
github-actions
bot
added
the
patch:done
ndelangen
removed
the
patch:yes
Closes #
What I did
Introduced an automigration to update Storybook projects from deprecated
viewportandbackgroundsaddon configuration to the new globals API. The migration converts oldparameterssuch asviewports/defaultViewportandvalues/defaultto their respectiveoptionsandinitialGlobalsproperties. The tool also renames thedisableproperty for backgrounds todisabled, ensuring a consistent experience when switching between stories. Comprehensive tests cover a variety of migration scenarios including merges, dynamic values, cleanup of empty objects, and handling both meta and story export styles.Transformation Examples
Viewport Configuration Example
Before:
After:
Backgrounds Configuration Example
Before:
After:
Story-Level Migration Example
Before:
After:
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-31614-sha-fc539b3c. 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-31614-sha-fc539b3cvalentin/add-viewports-backgrounds-automigrationsfc539b3c1759239487)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 canary-release-pr.yml --field pr=31614Greptile Summary
Introduces an automigration tool for Storybook 9 to convert viewport and backgrounds addon configurations to use the new globals API, ensuring consistent experience while navigating between stories.
code/lib/cli-storybook/src/automigrate/fixes/addon-globals-api.tsto handle migration of viewport and backgrounds configurationsviewports→optionsanddefaultViewport→initialGlobals.viewportvalues→optionsanddefault→initialGlobals.backgroundsdisableproperty todisabledfor backgrounds configurationaddon-globals-api.test.tscovering various migration scenariosSummary by CodeRabbit