Bug: Do not display non-existing shortcuts in the settings page #32711
Conversation
📝 WalkthroughWalkthroughrenderKeyInput in settings/shortcuts now filters Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Settings as Settings/Shortcuts UI
participant Renderer as renderKeyInput
participant Labels as shortcutLabels / addonsShortcutLabels
User->>Settings: open shortcuts UI
Settings->>Renderer: call renderKeyInput(shortcutKeys)
Renderer->>Labels: filter keys -> availableShortcuts
alt key has label
Renderer-->>Settings: render Row for key
else no label
Renderer-->>Settings: skip key (no Row)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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/core/src/manager/settings/shortcuts.tsx (2)
233-233: Minor formatting change with no behavioral impact.This empty line addition is a whitespace-only change. Ensure it aligns with the project's ESLint and Prettier configuration.
291-317: Consider using.filter().map()or.flatMap()for cleaner code.The current implementation uses
.map()with a conditionalreturnstatement, which creates an array containingundefinedelements. While React correctly skipsundefinedvalues during rendering, a more idiomatic approach would be to filter first, then map.Option 1: Use
.filter().map()- const arr = Object.entries(shortcutKeys).map(([feature, { shortcut }]: [Feature, any]) => { - if (shortcutLabels[feature] === undefined && (!addonsShortcutLabels || !addonsShortcutLabels[feature])) { - return; - } - return ( + const arr = Object.entries(shortcutKeys) + .filter(([feature]: [Feature, any]) => { + return shortcutLabels[feature] !== undefined || (addonsShortcutLabels && addonsShortcutLabels[feature]); + }) + .map(([feature, { shortcut }]: [Feature, any]) => ( <Row key={feature}> {/* @ts-expect-error (non strict) */} <Description>{shortcutLabels[feature] || addonsShortcutLabels[feature]}</Description> <TextInput spellCheck="false" valid={this.displayError(feature)} className="modalInput" onBlur={this.onBlur} onFocus={this.onFocus(feature)} // @ts-expect-error (Converted from ts-ignore) onKeyDown={this.onKeyDown} value={shortcut ? shortcutToHumanString(shortcut) : ''} placeholder="Type keys" readOnly /> {/* @ts-expect-error (non strict) */} <SuccessIcon valid={this.displaySuccessMessage(feature)} /> </Row> - ); - }); + ));Option 2: Use
.flatMap()- const arr = Object.entries(shortcutKeys).map(([feature, { shortcut }]: [Feature, any]) => { + const arr = Object.entries(shortcutKeys).flatMap(([feature, { shortcut }]: [Feature, any]) => { if (shortcutLabels[feature] === undefined && (!addonsShortcutLabels || !addonsShortcutLabels[feature])) { - return; + return []; } return ( <Row key={feature}> {/* @ts-expect-error (non strict) */} <Description>{shortcutLabels[feature] || addonsShortcutLabels[feature]}</Description> <TextInput spellCheck="false" valid={this.displayError(feature)} className="modalInput" onBlur={this.onBlur} onFocus={this.onFocus(feature)} // @ts-expect-error (Converted from ts-ignore) onKeyDown={this.onKeyDown} value={shortcut ? shortcutToHumanString(shortcut) : ''} placeholder="Type keys" readOnly /> {/* @ts-expect-error (non strict) */} <SuccessIcon valid={this.displaySuccessMessage(feature)} /> </Row> ); });Both options eliminate
undefinedelements from the array and make the intent clearer. Option 1 (.filter().map()) is generally preferred for readability, while Option 2 (.flatMap()) is more concise if you prefer the early return pattern.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
code/core/src/manager/settings/shortcuts.tsx(2 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/core/src/manager/settings/shortcuts.tsx
**/*.{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/core/src/manager/settings/shortcuts.tsx
🧬 Code graph analysis (1)
code/core/src/manager/settings/shortcuts.tsx (1)
code/core/src/manager-api/lib/shortcut.ts (1)
shortcutToHumanString(172-174)
|
View your CI Pipeline Execution ↗ for commit 78ca2f1
☁️ Nx Cloud last updated this comment at |
There was a problem hiding this comment.
Thanks for your contribution @DKER2!
I think something is wrong with the applied logic unfortunately. I am consistently missing Toggle Outline on both my existing sets of browsers with expired shortcuts and on a fresh port with the default shortcuts.
I think it would be wiser to figure out what is defined in the code, and filter out the settings in local storage that don't match the list in the code. It might also be useful to enforce the sorting present in the code rather than in local storage, as that can result in a messy ordering.
Co-authored-by: Steve Dodier-Lazaro <[email protected]>
Hi @Sidnioulz, based on this code storybook/code/core/src/outline/OutlineSelector.tsx Lines 25 to 33 in 4a0464b storybook/code/core/src/manager-api/modules/shortcuts.ts Lines 412 to 414 in 4a0464b |
Oh wow, good catch. I definitely didn't expect that. |
Sidnioulz
changed the title
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (1)
722-734: Test assertion may not match the test intent.The test description states "should bail transformation and not warn when file is already transformed," but:
- Line 733 checks
logger.loginstead oflogger.warn- An already-transformed file would have
transformedStoryExports.size === 0, triggering the bailout at line 244 of the main code, which emitslogger.warn- This means the test might pass while still emitting warnings
Consider either:
- Changing the assertion to check
logger.warnto match the test intent, OR- Updating the bailout logic to detect already-transformed files and skip the warning (as suggested in the previous file review)
- expect(logger.log).not.toHaveBeenCalled(); + expect(logger.warn).not.toHaveBeenCalled();code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
107-107: Remove unsafe non-null assertions onpreviewConfigPath.The non-null assertion operator (
!) at lines 107 and 122 masks a real safety issue. The parameterpreviewConfigPathis typed as optional (string | undefined) inAutofixOptions, but it's passed directly to functions expecting a required string without any validation. Additionally, line 72 usespreviewConfigPathin a template literal without assertion, which would expose "undefined" in the user-facing message if the value is actually undefined.Add a guard clause at the start of the
run()function to ensurepreviewConfigPathis provided:if (!previewConfigPath) { throw new Error('previewConfigPath is required for CSF Factories migration'); }Then remove the
!operators at lines 107 and 122 once the validation is in place.code/frameworks/nextjs/src/preset.ts (1)
172-172: UseisNextVersionGteconsistently for all version checks.Line 172 still uses the old pattern with
semver.gtedirectly, while the v16 check at line 54 was migrated to use the newisNextVersionGteutility. The new utility appliessemver.coerceto normalize version strings with prerelease tags or metadata, making it more robust. This inconsistency could cause the v14 check to behave differently from the v16 check when Next.js versions have non-standard formatting.Apply this diff to align with the refactoring:
- const isNext14orNewer = semver.gte(nextjsVersion, '14.0.0'); + const isNext14orNewer = isNextVersionGte('14.0.0');Note: After this change, you can also remove the direct
getNextjsVersion()call at line 169 if it's only used for this comparison. TheisNextVersionGteutility calls it internally.code/core/src/common/utils/load-main-config.ts (1)
53-55: Bug: temp filename has a double dot before the extension.
${name}.tmp.${ext}yieldsmain.tmp..ts. Use${name}.tmp${ext}.- const { ext, name, dir } = parse(mainPath); - const modifiedMainPath = join(dir, `${name}.tmp.${ext}`); + const { ext, name, dir } = parse(mainPath); + const modifiedMainPath = join(dir, `${name}.tmp${ext}`);code/core/src/manager-api/modules/stories.ts (2)
371-387: Avoid bypassing the empty-hash guard in jumpToComponentDefaulting to {} makes the
if (!hash)check ineffective and can pass an empty index to sibling lookup. Remove the fallback and/or guard for emptiness.Apply one of:
- const hash = refId ? refs[refId].filteredIndex || {} : filteredIndex; - if (!hash) { + const hash = refId ? refs[refId].filteredIndex : filteredIndex; + if (!hash || Object.keys(hash).length === 0) { return; }Also applies to: 379-383
476-484: findLeafEntry can recurse into missing entries; fix and align types
- If no child exists in
index, recursion uses a non-existent ID and may crash.- The docstring says “or null if no leaf entry was found”, but the signature returns only
API_LeafEntry; the callers already use optional chaining.Recommend:
- Return
API_LeafEntry | undefined.- Stop recursion when there’s no existing child in
index.- findLeafEntry(index: API_IndexHash, storyId: StoryId): API_LeafEntry; + findLeafEntry(index: API_IndexHash, storyId: StoryId): API_LeafEntry | undefined; @@ const entry = index[storyId]; - if (entry.type === 'docs' || entry.type === 'story') { + if (!entry) return undefined; + if (entry.type === 'docs' || entry.type === 'story') { return entry; } - const childStoryId = entry.children.find((childId) => index[childId]) || entry.children[0]; - return api.findLeafEntry(index, childStoryId); + const existingChildId = entry.children.find((childId) => !!index[childId]); + if (!existingChildId) return undefined; + return api.findLeafEntry(index, existingChildId);No other changes needed; call sites already use optional chaining or pass result into a guarded navigator.
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
74-87: Fix function signature mismatch for isValidWorkspaceConfigFileThe declared type takes
(fileContents: string, babel: any)but the implementation only accepts one arg and uses the importedbabel. Align the signature to avoid confusion.-export const isValidWorkspaceConfigFile: (fileContents: string, babel: any) => boolean = ( - fileContents -) => { +export const isValidWorkspaceConfigFile = (fileContents: string): boolean => {And update call sites to pass only
fileContents.
🧹 Nitpick comments (14)
code/lib/eslint-plugin/package.json (1)
61-61: Consider updating ts-dedent to the latest stable version.The
ts-dedentdependency is currently pinned to^2.0.0. According to recent information, version 2.2.0 is the latest stable release with improved indentation handling for multiline values. Updating to^2.2.0would be optional but would unlock these improvements.code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.ts (1)
242-258: Early bailout logic looks correct, but may produce warnings for already-transformed files.The early bailout when
transformedStoryExports.size === 0appropriately prevents broken indexer scenarios. However, already-transformed files (which containmeta.story()calls instead of object literals) would trigger this bailout and emit a warning, even though they don't need transformation.Consider detecting already-transformed files before the bailout to avoid unnecessary warnings. For example, check if the source already contains
preview.meta(or.story(patterns.Example detection:
+ // Skip transformation if file is already in CSF4 format + if (info.source.includes('.story(') || info.source.includes('preview.meta(')) { + return info.source; + } + // If no stories were transformed, bail early to avoid having a mixed CSF syntax and therefore a broken indexer. if (transformedStoryExports.size === 0) {code/core/src/node-logger/prompts/prompt-provider-clack.ts (1)
56-59: Redundant property assignment after spread.The explicit
required: options.requiredis redundant since the spread...optionsalready includes therequiredfield. This change doesn't alter behavior but adds unnecessary code.Consider simplifying to:
- const result = await clack.multiselect<T>({ - ...options, - required: options.required, - }); + const result = await clack.multiselect<T>(options);code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
70-74: Improve grammar in user-facing message.The phrase "absolute imports of your...file" is grammatically awkward. Consider revising to "absolute imports to reference your preview config file" or "absolute imports for your preview configuration" for better clarity.
Apply this diff to improve the message:
- The CSF Factories format can benefit from using absolute imports of your ${picocolors.cyan(previewConfigPath)} file. We can configure that for you, using subpath imports (a node standard), by adjusting the imports property of your package.json. + The CSF Factories format can benefit from using absolute imports to reference your ${picocolors.cyan(previewConfigPath)} file. We can configure that for you, using subpath imports (a node standard), by adjusting the imports property of your package.json.code/core/src/common/utils/load-main-config.ts (4)
14-22: Default and document skipCache param.Make intent explicit and TS-friendly.
-export async function loadMainConfig({ - configDir = '.storybook', - cwd, - skipCache, -}: { +export async function loadMainConfig({ + configDir = '.storybook', + cwd, + skipCache = false, +}: { configDir: string; cwd?: string; - skipCache?: boolean; + /** Bypass module cache when importing main config */ + skipCache?: boolean; }):Ensure all call sites either rely on default or pass a boolean.
34-37: Broaden ESM error detection to cover older messages.Some Node/bundler variants still emit “require is not defined”.
- if (e.message.includes('not defined in ES module scope')) { + if ( + e.message.includes('not defined in ES module scope') || + e.message.includes('require is not defined') + ) {
58-61: Honor skipCache for the temporary import and make cleanup resilient.
- Pass through skipCache.
- Use
rm(..., { force: true })to avoid races.- out = await importModule(modifiedMainPath); + out = await importModule(modifiedMainPath, { skipCache }); } finally { - await rm(modifiedMainPath); + await rm(modifiedMainPath, { force: true }); }
35-37: Consider elevating to warn level.This path indicates config incompatibility;
logger.warnmay be more appropriate thaninfo.CHANGELOG.prerelease.md (1)
3-5: Fix grammar in beta.13 entries ("it's" → "its").User-visible docs polish.
-- Move: Addon jest into it's own repository - [#32646](https://github.com/storybookjs/storybook/pull/32646), thanks @ndelangen! +- Move: Addon jest into its own repository - [#32646](https://github.com/storybookjs/storybook/pull/32646), thanks @ndelangen!code/core/src/manager-api/modules/stories.ts (1)
441-444: Requiring filteredHash may block navigation early
selectStorynow returns if eitherhashorfilteredHashis falsy. During initial load (or for refs before filters are applied),filteredHashcan be absent. Consider falling back tohashfor leaf resolution.- const hash = ref ? refs[ref].index : index; - const filteredHash = ref ? refs[ref].filteredIndex : filteredIndex; - if (!hash || !filteredHash) { + const hash = ref ? refs[ref].index : index; + const filteredHash = ref ? refs[ref].filteredIndex : filteredIndex; + if (!hash) { return; } + const leafIndex = filteredHash || hash;And below:
- gotoStory(api.findLeafEntry(filteredHash, entry.id)); + gotoStory(api.findLeafEntry(leafIndex, entry.id));Please confirm there’s no regression when reloading directly on the shortcuts/settings routes before filters are computed. Based on learnings.
code/core/src/manager-api/tests/stories.test.ts (1)
79-98: Add edge-case tests: no visible children and missing filteredIndexTo guard regressions:
- When a component exists in
filteredIndexbut all of its children are filtered out,selectStory('componentId')should no-op (no navigate, no crash).- When
filteredIndexis undefined (early load),selectStory('componentId')should still work by falling back toindex.I can provide test snippets if helpful.
Also applies to: 607-707, 1360-1386
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
61-73: Broaden isSafeToExtendWorkspace param type to match call sitesYou call
isSafeToExtendWorkspace(arg as CallExpression)but the guard inside already checksisCallExpression. Type it asDeclarationto reflect usage.-const isSafeToExtendWorkspace = (path: CallExpression) => +const isSafeToExtendWorkspace = (path: Declaration) => isCallExpression(path) && path.arguments.length > 0 && isObjectExpression(path.arguments[0]) && ...code/addons/vitest/src/updateVitestFile.ts (1)
82-139: Confirm minimum Node runtime for findLastIndexThis relies on
Array.prototype.findLastIndex. Ensure our supported Node version in this package/CLI is >=18 (ideally >=20 per Vitest 3.x). If not guaranteed, replace with a backward-compatible scan.code/addons/vitest/src/updateVitestFile.test.ts (1)
18-20: Adopt Vitest mocking conventions (spy: true, vi.mocked) per repo guidelinesPer guidelines, prefer
vi.mock(..., { spy: true })andvi.mocked()for typed access. Update the mocks (logger, module resolver, events, global) to use spy mode; you’re already usingvi.mockedin places—make it consistent.Based on coding guidelines.
Also applies to: 24-31, 46-56, 120-131, 195-203, 227-238, 302-313, 375-386, 447-458, 498-509, 581-592, 651-662, 675-686, 745-756, 807-818
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
code/addons/jest/docs/storybook-addon-jest.gifis excluded by!**/*.gifcode/yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (106)
CHANGELOG.md(1 hunks)CHANGELOG.prerelease.md(1 hunks)MIGRATION.md(3 hunks)code/addons/a11y/package.json(1 hunks)code/addons/docs/package.json(1 hunks)code/addons/jest/README.md(0 hunks)code/addons/jest/build-config.ts(0 hunks)code/addons/jest/manager.js(0 hunks)code/addons/jest/package.json(0 hunks)code/addons/jest/project.json(0 hunks)code/addons/jest/src/components/Message.tsx(0 hunks)code/addons/jest/src/components/Panel.tsx(0 hunks)code/addons/jest/src/components/Result.tsx(0 hunks)code/addons/jest/src/hoc/provideJestResult.tsx(0 hunks)code/addons/jest/src/index.ts(0 hunks)code/addons/jest/src/manager.tsx(0 hunks)code/addons/jest/src/shared.test.ts(0 hunks)code/addons/jest/src/shared.ts(0 hunks)code/addons/jest/src/types.ts(0 hunks)code/addons/jest/tsconfig.json(0 hunks)code/addons/jest/vitest.config.ts(0 hunks)code/addons/links/package.json(1 hunks)code/addons/onboarding/package.json(1 hunks)code/addons/pseudo-states/package.json(1 hunks)code/addons/themes/package.json(1 hunks)code/addons/vitest/package.json(1 hunks)code/addons/vitest/src/updateVitestFile.test.ts(11 hunks)code/addons/vitest/src/updateVitestFile.ts(2 hunks)code/builders/builder-vite/package.json(1 hunks)code/builders/builder-webpack5/package.json(1 hunks)code/core/package.json(1 hunks)code/core/src/cli/detect.ts(1 hunks)code/core/src/common/utils/get-addon-annotations.ts(2 hunks)code/core/src/common/utils/load-main-config.ts(1 hunks)code/core/src/common/versions.ts(1 hunks)code/core/src/core-server/__for-testing__/main.ts(0 hunks)code/core/src/manager-api/modules/stories.ts(4 hunks)code/core/src/manager-api/tests/stories.test.ts(1 hunks)code/core/src/manager-api/version.ts(1 hunks)code/core/src/manager/components/sidebar/TagsFilterPanel.tsx(1 hunks)code/core/src/node-logger/prompts/prompt-provider-clack.ts(1 hunks)code/core/src/shared/utils/module.ts(2 hunks)code/frameworks/angular/package.json(1 hunks)code/frameworks/ember/package.json(1 hunks)code/frameworks/html-vite/package.json(1 hunks)code/frameworks/nextjs-vite/package.json(1 hunks)code/frameworks/nextjs/package.json(1 hunks)code/frameworks/nextjs/src/config/webpack.ts(1 hunks)code/frameworks/nextjs/src/preset.ts(2 hunks)code/frameworks/nextjs/src/utils.ts(2 hunks)code/frameworks/preact-vite/package.json(1 hunks)code/frameworks/react-native-web-vite/package.json(1 hunks)code/frameworks/react-vite/package.json(1 hunks)code/frameworks/react-webpack5/package.json(1 hunks)code/frameworks/server-webpack5/package.json(1 hunks)code/frameworks/svelte-vite/package.json(1 hunks)code/frameworks/sveltekit/package.json(1 hunks)code/frameworks/vue3-vite/package.json(1 hunks)code/frameworks/web-components-vite/package.json(1 hunks)code/lib/cli-sb/package.json(1 hunks)code/lib/cli-storybook/package.json(1 hunks)code/lib/cli-storybook/src/add.ts(2 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts(6 hunks)code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(1 hunks)code/lib/cli-storybook/src/automigrate/multi-project.test.ts(2 hunks)code/lib/cli-storybook/src/automigrate/multi-project.ts(1 hunks)code/lib/cli-storybook/src/codemod/csf-factories.ts(2 hunks)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)code/lib/codemod/package.json(1 hunks)code/lib/core-webpack/package.json(1 hunks)code/lib/create-storybook/package.json(1 hunks)code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts(2 hunks)code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx(2 hunks)code/lib/csf-plugin/package.json(1 hunks)code/lib/eslint-plugin/package.json(1 hunks)code/lib/react-dom-shim/package.json(1 hunks)code/package.json(1 hunks)code/presets/create-react-app/package.json(1 hunks)code/presets/react-webpack/package.json(1 hunks)code/presets/server-webpack/package.json(1 hunks)code/renderers/html/package.json(1 hunks)code/renderers/preact/package.json(1 hunks)code/renderers/react/package.json(1 hunks)code/renderers/server/package.json(1 hunks)code/renderers/svelte/package.json(1 hunks)code/renderers/vue3/package.json(1 hunks)code/renderers/web-components/package.json(1 hunks)docs/_snippets/addon-a11y-config-in-preview.md(4 hunks)docs/_snippets/vite-includeStorybookNextjsPlugin.md(1 hunks)docs/releases/migration-guide.mdx(1 hunks)docs/versions/next.json(1 hunks)docs/writing-tests/integrations/stories-in-unit-tests.mdx(1 hunks)scripts/.eslintignore(0 hunks)scripts/build/entry-configs.ts(0 hunks)test-storybooks/ember-cli/package.json(0 hunks)test-storybooks/external-docs/package.json(0 hunks)test-storybooks/portable-stories-kitchen-sink/nextjs/package.json(0 hunks)test-storybooks/portable-stories-kitchen-sink/react/package.json(0 hunks)test-storybooks/portable-stories-kitchen-sink/svelte/package.json(0 hunks)test-storybooks/portable-stories-kitchen-sink/vue3/package.json(1 hunks)test-storybooks/server-kitchen-sink/package.json(0 hunks)test-storybooks/standalone-preview/package.json(0 hunks)test-storybooks/yarn-pnp/package.json(0 hunks)
💤 Files with no reviewable changes (27)
- code/addons/jest/src/types.ts
- code/core/src/core-server/for-testing/main.ts
- code/addons/jest/src/components/Result.tsx
- code/addons/jest/src/shared.test.ts
- code/addons/jest/vitest.config.ts
- test-storybooks/portable-stories-kitchen-sink/nextjs/package.json
- code/addons/jest/package.json
- test-storybooks/portable-stories-kitchen-sink/react/package.json
- test-storybooks/server-kitchen-sink/package.json
- code/addons/jest/src/index.ts
- code/addons/jest/src/hoc/provideJestResult.tsx
- scripts/build/entry-configs.ts
- test-storybooks/standalone-preview/package.json
- test-storybooks/ember-cli/package.json
- code/addons/jest/project.json
- test-storybooks/yarn-pnp/package.json
- code/addons/jest/tsconfig.json
- test-storybooks/external-docs/package.json
- code/addons/jest/src/manager.tsx
- code/addons/jest/README.md
- test-storybooks/portable-stories-kitchen-sink/svelte/package.json
- code/addons/jest/src/components/Message.tsx
- code/addons/jest/manager.js
- code/addons/jest/src/shared.ts
- scripts/.eslintignore
- code/addons/jest/build-config.ts
- code/addons/jest/src/components/Panel.tsx
✅ Files skipped from review due to trivial changes (35)
- code/frameworks/nextjs-vite/package.json
- code/frameworks/react-native-web-vite/package.json
- code/frameworks/svelte-vite/package.json
- code/core/package.json
- code/lib/react-dom-shim/package.json
- code/frameworks/vue3-vite/package.json
- code/frameworks/react-vite/package.json
- code/addons/docs/package.json
- code/renderers/server/package.json
- code/lib/csf-plugin/package.json
- code/presets/react-webpack/package.json
- code/addons/themes/package.json
- code/lib/codemod/package.json
- code/renderers/html/package.json
- code/frameworks/web-components-vite/package.json
- code/renderers/web-components/package.json
- code/addons/vitest/package.json
- code/core/src/manager/components/sidebar/TagsFilterPanel.tsx
- code/renderers/react/package.json
- code/builders/builder-vite/package.json
- code/frameworks/angular/package.json
- code/addons/links/package.json
- code/builders/builder-webpack5/package.json
- code/lib/cli-sb/package.json
- code/renderers/svelte/package.json
- code/lib/cli-storybook/package.json
- code/presets/server-webpack/package.json
- code/renderers/preact/package.json
- code/frameworks/react-webpack5/package.json
- code/addons/a11y/package.json
- code/frameworks/nextjs/package.json
- code/core/src/common/utils/get-addon-annotations.ts
- CHANGELOG.md
- code/lib/create-storybook/package.json
- code/addons/pseudo-states/package.json
🧰 Additional context used
📓 Path-based instructions (9)
**/*.{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/frameworks/ember/package.jsoncode/frameworks/nextjs/src/preset.tscode/package.jsoncode/lib/core-webpack/package.jsoncode/core/src/common/versions.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/cli/detect.tscode/frameworks/html-vite/package.jsoncode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/addons/vitest/src/updateVitestFile.tscode/addons/onboarding/package.jsoncode/core/src/manager-api/version.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/presets/create-react-app/package.jsoncode/renderers/vue3/package.jsoncode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/frameworks/server-webpack5/package.jsontest-storybooks/portable-stories-kitchen-sink/vue3/package.jsoncode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/frameworks/nextjs/src/utils.tscode/core/src/common/utils/load-main-config.tscode/core/src/node-logger/prompts/prompt-provider-clack.tsdocs/versions/next.jsoncode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/eslint-plugin/package.jsoncode/frameworks/sveltekit/package.jsoncode/frameworks/preact-vite/package.jsoncode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/core/src/manager-api/modules/stories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/frameworks/nextjs/src/config/webpack.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/frameworks/nextjs/src/preset.tscode/core/src/common/versions.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/cli/detect.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/addons/vitest/src/updateVitestFile.tscode/core/src/manager-api/version.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/frameworks/nextjs/src/utils.tscode/core/src/common/utils/load-main-config.tscode/core/src/node-logger/prompts/prompt-provider-clack.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/core/src/manager-api/modules/stories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/frameworks/nextjs/src/config/webpack.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/frameworks/nextjs/src/preset.tscode/core/src/common/versions.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/cli/detect.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/addons/vitest/src/updateVitestFile.tscode/core/src/manager-api/version.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/frameworks/nextjs/src/utils.tscode/core/src/common/utils/load-main-config.tscode/core/src/node-logger/prompts/prompt-provider-clack.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/core/src/manager-api/modules/stories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/frameworks/nextjs/src/config/webpack.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/frameworks/nextjs/src/preset.tscode/core/src/common/versions.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsxcode/core/src/cli/detect.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/multi-project.tscode/addons/vitest/src/updateVitestFile.tscode/core/src/manager-api/version.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/frameworks/nextjs/src/utils.tscode/core/src/common/utils/load-main-config.tscode/core/src/node-logger/prompts/prompt-provider-clack.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.tscode/core/src/shared/utils/module.tscode/lib/cli-storybook/src/add.tscode/lib/cli-storybook/src/codemod/csf-factories.tscode/core/src/manager-api/modules/stories.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.tscode/frameworks/nextjs/src/config/webpack.ts
docs/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Update documentation under
docs/for significant changes, including migration guides for breaking changes
Files:
docs/writing-tests/integrations/stories-in-unit-tests.mdxdocs/releases/migration-guide.mdxdocs/versions/next.jsondocs/_snippets/vite-includeStorybookNextjsPlugin.mddocs/_snippets/addon-a11y-config-in-preview.md
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/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/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/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/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/automigrate/helpers/mainConfigFile.test.tscode/core/src/manager-api/tests/stories.test.tscode/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/lib/cli-storybook/src/automigrate/multi-project.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
test-storybooks/**
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Maintain test Storybook configurations under
test-storybooks/for E2E and visual testing scenarios
Files:
test-storybooks/portable-stories-kitchen-sink/vue3/package.json
🧠 Learnings (7)
📚 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/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.tscode/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/addons/vitest/src/updateVitestFile.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 **/*.@(test|spec).{ts,tsx,js,jsx} : Mock external dependencies in tests using `vi.mock()` (e.g., filesystem, loggers)
Applied to files:
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.tscode/addons/vitest/src/updateVitestFile.test.tscode/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 test-storybooks/** : Maintain test Storybook configurations under `test-storybooks/` for E2E and visual testing scenarios
Applied to files:
test-storybooks/portable-stories-kitchen-sink/vue3/package.json
📚 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/addons/vitest/src/updateVitestFile.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 type and access mocked functions
Applied to files:
code/addons/vitest/src/updateVitestFile.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 scripts/**/*.{ts,js,mjs} : In Node.js scripts, use `storybook/internal/node-logger` instead of `console.*`
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/**/*.{ts,tsx,js,jsx,mjs} : In application code, use Storybook loggers instead of `console.*` (client code: `storybook/internal/client-logger`; server code: `storybook/internal/node-logger`)
Applied to files:
code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts
🧬 Code graph analysis (13)
code/frameworks/nextjs/src/preset.ts (1)
code/frameworks/nextjs/src/utils.ts (1)
isNextVersionGte(28-32)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (3)
containsPatternUsage(259-271)containsDirnameUsage(274-276)hasDirnameDefined(283-287)
code/core/src/manager-api/tests/stories.test.ts (2)
code/core/src/manager-api/tests/mockStoriesEntries.ts (1)
navigationEntries(71-120)code/core/src/manager-api/tests/url.test.js (1)
store(143-151)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (1)
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.tsx (1)
vitestConfigFiles(95-146)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (2)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (1)
fixFauxEsmRequire(67-216)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
containsDirnameUsage(274-276)
code/lib/cli-storybook/src/automigrate/multi-project.test.ts (1)
code/lib/cli-storybook/src/automigrate/multi-project.ts (1)
promptForAutomigrations(197-250)
code/core/src/common/utils/load-main-config.ts (1)
code/core/src/shared/utils/module.ts (1)
importModule(54-89)
code/addons/vitest/src/updateVitestFile.test.ts (1)
code/addons/vitest/src/updateVitestFile.ts (3)
updateConfigFile(79-301)loadTemplate(9-16)updateWorkspaceFile(303-364)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (2)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (6)
containsRequireUsage(251-256)containsFilenameUsage(278-280)containsDirnameUsage(274-276)updateMainConfig(207-230)bannerComment(298-299)BannerConfig(292-296)code/lib/cli-storybook/src/automigrate/types.ts (1)
Fix(52-60)
code/lib/cli-storybook/src/add.ts (2)
code/core/src/common/utils/load-main-config.ts (1)
loadMainConfig(14-71)code/core/src/common/utils/sync-main-preview-addons.ts (1)
syncStorybookAddons(16-25)
code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
scripts/utils/tools.ts (1)
picocolors(117-117)
code/core/src/manager-api/modules/stories.ts (2)
code/core/src/manager-api/root.tsx (1)
API_HashEntry(308-308)code/core/src/csf/index.ts (2)
sanitize(8-16)toId(27-28)
code/frameworks/nextjs/src/config/webpack.ts (1)
code/frameworks/nextjs/src/utils.ts (1)
isNextVersionGte(28-32)
🪛 LanguageTool
docs/_snippets/vite-includeStorybookNextjsPlugin.md
[grammar] ~1-~1: Hier könnte ein Fehler sein.
Context: ts filename="vitest.config.ts" renderer="react" language="ts" import { defineConfig } from "vite"; import { storybookNextJsPlugin } from '@storybook/nextjs-vite/vite-plugin' export default defineConfig({ // only necessary when not using @storybook/addon-vitest, otherwise the plugin is loaded automatically plugins: [storybookNextJsPlugin()], });
(QB_NEW_DE)
🔇 Additional comments (46)
code/lib/core-webpack/package.json (1)
3-3: Version bump is well-formed and consistent.The version update from
10.0.0-beta.10to10.0.0-beta.13follows valid semver conventions. Dependencies (notablyts-dedentat^2.0.0) remain stable and correctly specified. No functional or API changes detected.code/frameworks/html-vite/package.json (1)
3-3: Version bump to 10.0.0-beta.13 looks good.Straightforward version update aligned with repository-wide release. No functional changes and the version string is properly formatted.
code/frameworks/preact-vite/package.json (1)
3-3: Version bump to 10.0.0-beta.13 looks good.Straightforward version update consistent with html-vite and aligned with repository-wide release. No functional changes, dependencies, or structural modifications.
code/frameworks/sveltekit/package.json (1)
3-3: Version bump is a straightforward patch update to align with coordinated release.The version bump from 10.0.0-beta.10 to 10.0.0-beta.13 is consistent with the monorepo's beta release cycle. Since this is part of a larger coordinated version bump across multiple packages (as noted in the PR summary), no concerns here.
code/frameworks/ember/package.json (1)
3-3: Version bump is appropriate.The version bump from 10.0.0-beta.10 to 10.0.0-beta.13 is consistent with the broader version refresh mentioned in the PR context across Storybook packages. The change is isolated and follows semantic versioning for beta releases.
code/presets/create-react-app/package.json (1)
3-3: Version bump is consistent with coordinated release updates.The version update to
10.0.0-beta.13aligns with the broader set of version updates across Storybook packages mentioned in the PR context. The preset's version alignment with the core storybook package (specified asworkspace:^in peerDependencies) is appropriate.test-storybooks/portable-stories-kitchen-sink/vue3/package.json (1)
17-53: LGTM!The removal of the
"@storybook/addon-jest"resolution is consistent with the PR's objective to remove addon-jest across test-storybooks, and the file structure remains valid. The trailing newline (line 72) aligns with Prettier formatting conventions.code/lib/eslint-plugin/package.json (1)
3-3: Version bump is straightforward.The package version has been updated from 10.0.0-beta.10 to 10.0.0-beta.13, which is consistent with a beta release cycle. No functional changes to the package.
code/addons/onboarding/package.json (1)
3-3: Version bump looks good; routine monorepo version sync.The version has been updated from "10.0.0-beta.10" to "10.0.0-beta.13", consistent with the PR's broader version bump across packages. JSON formatting is clean and matches Prettier standards.
docs/_snippets/addon-a11y-config-in-preview.md (1)
25-25: Update the oldglobalsAPI reference in accessibility testing documentation.The property rename from
globalstoinitialGlobalsis confirmed as a breaking change with an automigration system in place. However, the documentation update indocs/_snippets/addon-a11y-config-in-preview.mdis inconsistent withdocs/writing-tests/accessibility-testing.mdx, which still references the oldglobals.a11y.manualproperty at line 56.Update the accessibility testing doc to use the new
initialGlobalsnaming and consider adding a deprecation notice or migration section todocs/writing-tests/accessibility-testing.mdxexplaining thatglobalshas been replaced withinitialGlobals. The automigration system will handle code updates, but explicit documentation guidance helps users understand the change.code/renderers/vue3/package.json (2)
3-3: Mismatch between PR objectives and file contents.The PR objectives describe changes to keyboard shortcuts filtering in
renderKeyInputwithin the settings module, but the file provided only shows a version bump. The keyboard shortcuts bug fix code is not included in this review.Verify that the actual keyboard shortcuts fix (described in the PR title and objectives) has been provided for review in a separate file.
3-3: Version bump is valid and consistently applied across the repository.Verification confirms the update to
10.0.0-beta.13is part of a coordinated repository-wide version bump. All 43 packages across renderers, addons, frameworks, and related modules have been consistently updated to the same version with no inconsistencies.code/frameworks/server-webpack5/package.json (1)
3-3: Version bump is consistent across the monorepo—approved.Verification confirms 43 active packages (frameworks, core, addons, builders, presets) are all correctly synchronized at
10.0.0-beta.13. Test fixtures use0.0.0(expected), and utility packages (scripts/tools) maintain independent versions by design. No issues found.code/lib/cli-storybook/src/codemod/helpers/story-to-csf-factory.test.ts (2)
11-16: LGTM! Mock setup follows coding guidelines.The mock correctly includes both
logandwarnmethods withvi.fn(), and uses thespy: truepattern as required by the coding guidelines.Based on coding guidelines.
681-697: Good test coverage for the new bailout scenario.The test appropriately verifies that files with no transformable stories are skipped and emit the expected warning message.
code/lib/cli-storybook/src/automigrate/multi-project.ts (1)
246-246: LGTM: Makes automigration selection optional.Setting
required: falseallows users to skip all automigrations if desired, which is the correct behavior for an optional selection prompt. The test coverage confirms this behavior.code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.test.ts (1)
223-332: LGTM: Comprehensive test coverage for pattern detection.The test suite thoroughly covers the new helper functions with edge cases including:
- Detection in actual code vs. strings/comments
- All quote types (double, single, backtick)
- Escaped quotes and mixed contexts
- Different variable declaration keywords (const, let, var)
code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.test.ts (3)
79-143: LGTM: Tests updated to match BannerConfig return type.The test expectations correctly verify the new return type with
hasRequireUsage,hasUnderscoreDirname, andhasUnderscoreFilenamefields instead of a simple boolean.
146-228: LGTM: Thorough edge-case coverage for dirname detection.The test suite comprehensively validates the
containsDirnameUsagefunction across various contexts:
- Strings (all quote types)
- Comments (single-line and multi-line)
- Escaped quotes
- Mixed contexts
259-434: LGTM: Run tests validate AST transformations correctly.The tests verify:
- Correct import and declaration injection
- Proper ordering of declarations
- Avoidance of duplicates
- Inline snapshots ensure exact output matches expectations
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (2)
258-287: LGTM: Robust pattern detection with proper string/comment stripping.The
containsPatternUsageimplementation correctly:
- Strips strings using a comprehensive regex that handles all quote types and escape sequences
- Removes block and line comments
- Tests the pattern only on actual code
The approach prevents false positives from strings and comments.
289-303: LGTM: Clean API for banner configuration.The
BannerConfiginterface and related utilities provide a clear contract for tracking what ESM compatibility features are needed. ThebannerCommentandhasRequireBannerare straightforward and well-defined.code/lib/cli-storybook/src/automigrate/fixes/fix-faux-esm-require.ts (4)
19-65: LGTM: Comprehensive declaration detection.The
hasExistingDeclarationfunction correctly checks for existing declarations across:
- Variable declarations (const/let/var)
- Function declarations
- Exported versions of both
This prevents duplicate declarations from being injected.
92-106: LGTM: Improved return type provides granular control.Returning a
BannerConfigobject instead of a boolean allows therunmethod to selectively inject only the needed compatibility features, avoiding unnecessary imports and declarations.
120-210: LGTM: Careful AST manipulation with correct ordering.The implementation correctly:
- Adds only necessary imports
- Inserts declarations after imports
- Ensures
__filenameis declared before__dirname(sincedirname()depends on it)- Calculates
requireinsertion position accounting for previously added declarations- Avoids duplicates via
hasExistingDeclarationchecksThe insertion logic is well-thought-out and correct.
212-214: Banner comment placement is confirmed as intentional.The code deliberately prepends the banner comment before all file content, including imports. The flow is: (1) AST modifications are written via
updateMainConfig, (2) the modified file is read, (3) the banner is prepended using[bannerComment, content].join('\n'), and (4) the result is written to disk. Since the banner is a comment line, appearing before imports is a valid design choice for a migration notice.code/lib/cli-storybook/src/codemod/csf-factories.ts (1)
80-84: Helpful option labels with clear examples.The updated option labels with inline import examples make it very clear to users what each choice means. Good UX improvement.
code/frameworks/nextjs/src/utils.ts (1)
12-12: LGTM!The semver import is correctly added to support the new version comparison utility.
code/frameworks/nextjs/src/config/webpack.ts (1)
6-8: LGTM!The refactor correctly replaces the local version check with the new centralized
isNextVersionGteutility, improving code maintainability and consistency.code/frameworks/nextjs/src/preset.ts (1)
18-18: LGTM!The import and usage of
isNextVersionGtecorrectly centralizes the version comparison logic for the Next.js 16 check.Also applies to: 54-54
code/package.json (1)
3-3: LGTM: Version bump to beta.13.The version increment is consistent with the coordinated beta.13 release across the monorepo.
code/core/src/manager-api/version.ts (1)
1-1: LGTM: Version constant updated.The version export correctly reflects the beta.13 release.
docs/releases/migration-guide.mdx (1)
32-32: LGTM: Improved documentation clarity.The updated wording accurately clarifies that both the main configuration and other presets must be valid ESM, not just the main file alone.
docs/writing-tests/integrations/stories-in-unit-tests.mdx (1)
83-91: LGTM: Helpful troubleshooting guidance added.The new troubleshooting section provides clear guidance for Next.js Vite users encountering module resolution errors.
code/core/src/common/versions.ts (1)
3-44: LGTM: Coordinated version updates.All package versions correctly updated to 10.0.0-beta.13 in this auto-generated file.
code/lib/cli-storybook/src/add.ts (1)
183-184: LGTM: Proper config reloading after addon addition.Loading a fresh main config with
skipCache: trueensures thatsyncStorybookAddonsoperates on the most recent configuration after the addon has been added to the file (line 178). This prevents stale config issues.docs/_snippets/vite-includeStorybookNextjsPlugin.md (1)
1-9: LGTM: Clear configuration example.The snippet provides a concise example for configuring the Storybook Next.js plugin in Vite, with a helpful comment explaining when it's needed.
code/lib/create-storybook/src/ink/steps/checks/vitestConfigFiles.test.ts (2)
16-42: LGTM: Mock configs for mergeConfig scenarios.The new mock configurations appropriately test both simple mergeConfig usage and multi-argument mergeConfig scenarios, expanding test coverage for the vitest config validation logic.
189-203: LGTM: Comprehensive test coverage for mergeConfig.The new test cases verify that the validation logic correctly handles mergeConfig usage patterns, including scenarios where defineConfig appears in different argument positions.
code/core/src/shared/utils/module.ts (3)
54-57: API change looks good.Signature with
{ skipCache = false }is sensible and backwards compatible.
75-77: Nice: clear require cache when skipping cache.Prevents stale loads in CJS fallback.
67-70: No issues found—query parameter cache-busting is supported on file:// URLs.Node's ESM loader accepts query parameters on file:// URLs across Node 18, 20, and 22, with modules loaded separately when query or fragment parameters differ. The
?${Date.now()}cache-buster at lines 67–70 is a supported and effective pattern.code/core/src/manager-api/tests/stories.test.ts (1)
1109-1147: Nice coverage for filtered child selectionTest correctly asserts fallback to first visible child when the first is hidden.
code/addons/vitest/src/updateVitestFile.ts (1)
190-296: mergeConfig handling looks solid; good precedence for test-bearing defineConfigThe logic to locate
defineConfigargs, prefer the one withtest, and relocate existing test into workspace/projects before merging is clear.Please add a small fixture covering a
mergeConfig(vite, defineConfig(objA), defineConfig(objB))where neither hastestto assert we still inject the Storybook project undertest.code/addons/vitest/src/updateVitestFile.test.ts (1)
7-9: Good move to diff-based assertions with getDiffSwitching to
getDiffmakes the tests resilient and focused. LGTM.code/core/src/manager-api/modules/stories.ts (1)
428-437: Safeguard settings spread in gotoStory
settingsmay be undefined; spreading it can throw. Use a default.- store.setState({ settings: { ...settings, lastTrackedStoryId: entry.id } }); + const prev = settings || {}; + store.setState({ settings: { ...prev, lastTrackedStoryId: entry.id } });Likely an incorrect or invalid review comment.
| case ProjectType.REACT_NATIVE_AND_RNW: | ||
| case ProjectType.REACT_NATIVE_WEB: | ||
| return CoreBuilder.Vite; |
There was a problem hiding this comment.
Critical: This change is unrelated to the PR objectives.
This file modifies CLI builder detection logic for React Native projects, but the PR is titled "Bug: Do not display non-existing shortcuts in the settings" and the objectives describe fixing keyboard shortcuts display in the settings UI. The enriched AI summary confirms this adds a React Native + RNW → Vite builder mapping.
While the code change itself is technically correct (adding ProjectType.REACT_NATIVE_AND_RNW case with appropriate fallthrough to CoreBuilder.Vite), it appears to be completely unrelated to keyboard shortcuts.
Please clarify:
- Why is this builder detection change included in a keyboard shortcuts PR?
- Should this be in a separate PR?
- Was this file included accidentally?
🤖 Prompt for AI Agents
In code/core/src/cli/detect.ts around lines 136-138, the added mapping of
ProjectType.REACT_NATIVE_AND_RNW / ProjectType.REACT_NATIVE_WEB to
CoreBuilder.Vite is unrelated to the keyboard-shortcuts bugfix PR; remove this
change from the current branch (revert these case additions) and either open a
separate PR containing the builder-detection change with its own
description/tests/docs or, if it must remain here, document why it belongs in
this PR by updating the PR description and adding a brief rationale and tests
demonstrating the new mapping.
| if (!gotoStory(entry)) { | ||
| // If the entry is not a story or docs, find the first descendant entry that is | ||
| gotoStory(api.findLeafEntry(filteredHash, entry.id)); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Selecting first visible child should handle “no visible children”
If a component has zero visible children in the current filter, findLeafEntry(filteredHash, entry.id) can fail. Pair with a safe findLeafEntry (see below) to avoid errors and just no-op when nothing is visible.
🤖 Prompt for AI Agents
In code/core/src/manager-api/modules/stories.ts around lines 454 to 457, calling
gotoStory(api.findLeafEntry(filteredHash, entry.id)) can throw when
findLeafEntry returns nothing for components with zero visible children; change
this to call findLeafEntry once into a variable, check it for truthiness, and
only call gotoStory if a valid leaf was returned (no-op otherwise), and ensure
findLeafEntry itself returns null/undefined when no descendant is visible.
| export const isNextVersionGte = (version: string): boolean => { | ||
| const currentVersion = getNextjsVersion(); | ||
| const coercedVersion = semver.coerce(currentVersion); | ||
| return coercedVersion ? semver.gte(coercedVersion, version) : false; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Document the public utility and verify edge case handling.
The function correctly uses semver.coerce to normalize the current Next.js version before comparison. However:
- Add JSDoc documentation explaining the purpose, parameters, and return behavior (especially that it returns
falsewhen the version cannot be parsed). - Consider logging a warning when
coercereturnsnull, as this indicates a version detection issue that could cause unexpected behavior downstream.
Apply this diff to add documentation:
+/**
+ * Checks if the current Next.js version is greater than or equal to the specified version.
+ * @param version - The version string to compare against (e.g., '16.0.0')
+ * @returns true if current version >= specified version, false if less than or if version cannot be determined
+ */
export const isNextVersionGte = (version: string): boolean => {
const currentVersion = getNextjsVersion();
const coercedVersion = semver.coerce(currentVersion);
return coercedVersion ? semver.gte(coercedVersion, version) : false;
};Verify edge cases with this script:
🏁 Script executed:
#!/bin/bash
# Description: Check for any unusual Next.js version formats in package.json that might fail coercion
echo "Checking Next.js version format..."
fd -t f "package.json" --exec jq -r 'select(.dependencies.next != null or .devDependencies.next != null) | .dependencies.next // .devDependencies.next' | head -5Length of output: 237
Now let me search the codebase for the actual implementation:
🏁 Script executed:
#!/bin/bash
# Find getNextjsVersion function implementation
rg -n "getNextjsVersion" --type ts --type tsx -A 5 -B 2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Find isNextVersionGte function usage
rg -n "isNextVersionGte" --type ts --type tsx -B 2 -A 2Length of output: 27
🏁 Script executed:
#!/bin/bash
# Check the full utils.ts file to see imports and structure
cat code/frameworks/nextjs/src/utils.tsLength of output: 5608
🏁 Script executed:
#!/bin/bash
# Find where isNextVersionGte is called
rg -n "isNextVersionGte" --type ts -B 3 -A 3Length of output: 2599
🏁 Script executed:
#!/bin/bash
# Check if the file has any existing documentation for exported functions
rg -n "^\s*\*\/" --type ts code/frameworks/nextjs/src/utils.ts | head -10Length of output: 80
Add JSDoc documentation to the exported utility function.
The function correctly handles edge cases by returning false when version coercion fails. However, add JSDoc to document the purpose, parameters, and return behavior—consistent with other exported utilities in this file like scopedResolve and getNodeModulesExcludeRegex.
+/**
+ * Checks if the current Next.js version is greater than or equal to the specified version.
+ * @param version - The version string to compare against (e.g., '16.0.0')
+ * @returns true if current version >= specified version, false if less than or version cannot be parsed
+ */
export const isNextVersionGte = (version: string): boolean => {
const currentVersion = getNextjsVersion();
const coercedVersion = semver.coerce(currentVersion);
return coercedVersion ? semver.gte(coercedVersion, version) : false;
};Note: The null coercion scenario is unlikely in practice since getNextjsVersion() reads from Next.js's package.json which should contain valid semver. The graceful fallback to false is appropriate.
📝 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.
| export const isNextVersionGte = (version: string): boolean => { | |
| const currentVersion = getNextjsVersion(); | |
| const coercedVersion = semver.coerce(currentVersion); | |
| return coercedVersion ? semver.gte(coercedVersion, version) : false; | |
| }; | |
| /** | |
| * Checks if the current Next.js version is greater than or equal to the specified version. | |
| * @param version - The version string to compare against (e.g., '16.0.0') | |
| * @returns true if current version >= specified version, false if less than or version cannot be parsed | |
| */ | |
| export const isNextVersionGte = (version: string): boolean => { | |
| const currentVersion = getNextjsVersion(); | |
| const coercedVersion = semver.coerce(currentVersion); | |
| return coercedVersion ? semver.gte(coercedVersion, version) : false; | |
| }; |
🤖 Prompt for AI Agents
In code/frameworks/nextjs/src/utils.ts around lines 28 to 32, add a JSDoc block
above the exported isNextVersionGte function that documents the function's
purpose (checks whether the current Next.js version is greater than or equal to
a provided semver), the parameter (version: string — the semver string to
compare against), and the return value (boolean — true if current Next.js
version coerced via semver is >= the provided version; false if coercion fails
or the comparison is not satisfied). Also mention the edge-case behavior in the
JSDoc: that coercion may return null and in that case the function returns false
as a graceful fallback.
| describe('promptForAutomigrations', () => { | ||
| it('should call multiselect with required: false', async () => { | ||
| const { prompt } = await import('storybook/internal/node-logger'); | ||
| const multiselectMock = vi.mocked(prompt.multiselect); | ||
| multiselectMock.mockResolvedValue(['fix1']); | ||
|
|
||
| const fix1 = createMockFix('fix1', { needsFix: true }); | ||
| const project1 = createMockProject('/project1/.storybook'); | ||
|
|
||
| const automigrations = [ | ||
| { | ||
| fix: fix1, | ||
| reports: [ | ||
| { | ||
| result: { needsFix: true }, | ||
| status: 'check_succeeded' as const, | ||
| project: project1, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
|
|
||
| await promptForAutomigrations(automigrations, { dryRun: false, yes: false }); | ||
|
|
||
| expect(multiselectMock).toHaveBeenCalledWith( | ||
| expect.objectContaining({ | ||
| message: 'Select automigrations to run', | ||
| required: false, | ||
| }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should return empty array when user selects nothing', async () => { | ||
| const { prompt } = await import('storybook/internal/node-logger'); | ||
| const multiselectMock = vi.mocked(prompt.multiselect); | ||
| multiselectMock.mockResolvedValue([]); | ||
|
|
||
| const fix1 = createMockFix('fix1', { needsFix: true }); | ||
| const project1 = createMockProject('/project1/.storybook'); | ||
|
|
||
| const automigrations = [ | ||
| { | ||
| fix: fix1, | ||
| reports: [ | ||
| { | ||
| result: { needsFix: true }, | ||
| status: 'check_succeeded' as const, | ||
| project: project1, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
|
|
||
| const result = await promptForAutomigrations(automigrations, { | ||
| dryRun: false, | ||
| yes: false, | ||
| }); | ||
|
|
||
| expect(result).toEqual([]); | ||
| }); | ||
|
|
||
| it('should return all automigrations when yes option is true', async () => { | ||
| const { logger } = await import('storybook/internal/node-logger'); | ||
| const logSpy = vi.spyOn(logger, 'log'); | ||
|
|
||
| const fix1 = createMockFix('fix1', { needsFix: true }); | ||
| const fix2 = createMockFix('fix2', { needsFix: true }); | ||
| const project1 = createMockProject('/project1/.storybook'); | ||
|
|
||
| const automigrations = [ | ||
| { | ||
| fix: fix1, | ||
| reports: [ | ||
| { | ||
| result: { needsFix: true }, | ||
| status: 'check_succeeded' as const, | ||
| project: project1, | ||
| }, | ||
| ], | ||
| }, | ||
| { | ||
| fix: fix2, | ||
| reports: [ | ||
| { | ||
| result: { needsFix: true }, | ||
| status: 'check_succeeded' as const, | ||
| project: project1, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
|
|
||
| const result = await promptForAutomigrations(automigrations, { dryRun: false, yes: true }); | ||
|
|
||
| expect(result).toEqual(automigrations); | ||
| expect(logSpy).toHaveBeenCalledWith('Running all detected automigrations:'); | ||
| }); | ||
|
|
||
| it('should return empty array when dryRun is true', async () => { | ||
| const { logger } = await import('storybook/internal/node-logger'); | ||
| const logSpy = vi.spyOn(logger, 'log'); | ||
|
|
||
| const fix1 = createMockFix('fix1', { needsFix: true }); | ||
| const project1 = createMockProject('/project1/.storybook'); | ||
|
|
||
| const automigrations = [ | ||
| { | ||
| fix: fix1, | ||
| reports: [ | ||
| { | ||
| result: { needsFix: true }, | ||
| status: 'check_succeeded' as const, | ||
| project: project1, | ||
| }, | ||
| ], | ||
| }, | ||
| ]; | ||
|
|
||
| const result = await promptForAutomigrations(automigrations, { dryRun: true, yes: false }); | ||
|
|
||
| expect(result).toEqual([]); | ||
| expect(logSpy).toHaveBeenCalledWith( | ||
| 'Detected automigrations (dry run - no changes will be made):' | ||
| ); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Move mock implementations to beforeEach blocks.
Per coding guidelines, mock implementations should be placed in beforeEach blocks rather than inline within test cases. This ensures consistent mock setup and makes tests more maintainable.
Based on coding guidelines.
Apply this pattern:
describe('promptForAutomigrations', () => {
+ let multiselectMock: any;
+ let logSpy: any;
+
+ beforeEach(async () => {
+ const { prompt, logger } = await import('storybook/internal/node-logger');
+ multiselectMock = vi.mocked(prompt.multiselect);
+ logSpy = vi.spyOn(logger, 'log');
+ });
+
it('should call multiselect with required: false', async () => {
- const { prompt } = await import('storybook/internal/node-logger');
- const multiselectMock = vi.mocked(prompt.multiselect);
multiselectMock.mockResolvedValue(['fix1']);
// ... rest of test
});📝 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('promptForAutomigrations', () => { | |
| it('should call multiselect with required: false', async () => { | |
| const { prompt } = await import('storybook/internal/node-logger'); | |
| const multiselectMock = vi.mocked(prompt.multiselect); | |
| multiselectMock.mockResolvedValue(['fix1']); | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| await promptForAutomigrations(automigrations, { dryRun: false, yes: false }); | |
| expect(multiselectMock).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| message: 'Select automigrations to run', | |
| required: false, | |
| }) | |
| ); | |
| }); | |
| it('should return empty array when user selects nothing', async () => { | |
| const { prompt } = await import('storybook/internal/node-logger'); | |
| const multiselectMock = vi.mocked(prompt.multiselect); | |
| multiselectMock.mockResolvedValue([]); | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| const result = await promptForAutomigrations(automigrations, { | |
| dryRun: false, | |
| yes: false, | |
| }); | |
| expect(result).toEqual([]); | |
| }); | |
| it('should return all automigrations when yes option is true', async () => { | |
| const { logger } = await import('storybook/internal/node-logger'); | |
| const logSpy = vi.spyOn(logger, 'log'); | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const fix2 = createMockFix('fix2', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| { | |
| fix: fix2, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| const result = await promptForAutomigrations(automigrations, { dryRun: false, yes: true }); | |
| expect(result).toEqual(automigrations); | |
| expect(logSpy).toHaveBeenCalledWith('Running all detected automigrations:'); | |
| }); | |
| it('should return empty array when dryRun is true', async () => { | |
| const { logger } = await import('storybook/internal/node-logger'); | |
| const logSpy = vi.spyOn(logger, 'log'); | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| const result = await promptForAutomigrations(automigrations, { dryRun: true, yes: false }); | |
| expect(result).toEqual([]); | |
| expect(logSpy).toHaveBeenCalledWith( | |
| 'Detected automigrations (dry run - no changes will be made):' | |
| ); | |
| }); | |
| }); | |
| describe('promptForAutomigrations', () => { | |
| let multiselectMock: any; | |
| let logSpy: any; | |
| beforeEach(async () => { | |
| const { prompt, logger } = await import('storybook/internal/node-logger'); | |
| multiselectMock = vi.mocked(prompt.multiselect); | |
| logSpy = vi.spyOn(logger, 'log'); | |
| }); | |
| it('should call multiselect with required: false', async () => { | |
| multiselectMock.mockResolvedValue(['fix1']); | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| await promptForAutomigrations(automigrations, { dryRun: false, yes: false }); | |
| expect(multiselectMock).toHaveBeenCalledWith( | |
| expect.objectContaining({ | |
| message: 'Select automigrations to run', | |
| required: false, | |
| }) | |
| ); | |
| }); | |
| it('should return empty array when user selects nothing', async () => { | |
| multiselectMock.mockResolvedValue([]); | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| const result = await promptForAutomigrations(automigrations, { | |
| dryRun: false, | |
| yes: false, | |
| }); | |
| expect(result).toEqual([]); | |
| }); | |
| it('should return all automigrations when yes option is true', async () => { | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const fix2 = createMockFix('fix2', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| { | |
| fix: fix2, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| const result = await promptForAutomigrations(automigrations, { dryRun: false, yes: true }); | |
| expect(result).toEqual(automigrations); | |
| expect(logSpy).toHaveBeenCalledWith('Running all detected automigrations:'); | |
| }); | |
| it('should return empty array when dryRun is true', async () => { | |
| const fix1 = createMockFix('fix1', { needsFix: true }); | |
| const project1 = createMockProject('/project1/.storybook'); | |
| const automigrations = [ | |
| { | |
| fix: fix1, | |
| reports: [ | |
| { | |
| result: { needsFix: true }, | |
| status: 'check_succeeded' as const, | |
| project: project1, | |
| }, | |
| ], | |
| }, | |
| ]; | |
| const result = await promptForAutomigrations(automigrations, { dryRun: true, yes: false }); | |
| expect(result).toEqual([]); | |
| expect(logSpy).toHaveBeenCalledWith( | |
| 'Detected automigrations (dry run - no changes will be made):' | |
| ); | |
| }); | |
| }); |
🤖 Prompt for AI Agents
In code/lib/cli-storybook/src/automigrate/multi-project.test.ts around lines
125-250, the tests define mock implementations (e.g.,
vi.mocked(prompt.multiselect).mockResolvedValue and vi.spyOn(logger, 'log'))
inline in each it block; move those mock setups into a shared beforeEach block
so all tests get a consistent mock environment. Create a top-level
describe('promptForAutomigrations') beforeEach that imports prompt and logger,
creates the multiselectMock and logSpy (and resets/clears mocks between tests),
and sets default mockResolvedValue/spy behavior; remove the in-test mock
implementations and only override specific behavior inside individual tests when
needed. Ensure to call vi.clearAllMocks() or reset mocks in an
afterEach/beforeEach to avoid cross-test leakage.
| This change aligns with Node.js ESM requirements, where relative imports must specify the full file extension. This applies to `.storybook/main.js` and any custom preset files. While TypeScript-based files (`.storybook/main.ts`) will continue to work with extensionless imports for now through automatic resolution, we recommend migrating to explicit extensions for consistency and better compatibility. | ||
|
|
There was a problem hiding this comment.
TypeScript still requires explicit extensions in NodeNext/Bundler mode
TypeScript configs that follow our earlier guidance (moduleResolution: "bundler" or "nodenext") no longer allow importing ./my-file without the .js suffix; the compiler raises TS2691 unless you keep the legacy node resolver or flip on allowImportingTsExtensions. Telling readers that .storybook/main.ts “will continue to work with extensionless imports” is misleading and will regress anyone who updated their tsconfig per the preceding section. Please adjust the wording (or call out the exact resolver setting that keeps this working) so the docs don’t direct people toward a configuration that TypeScript immediately rejects.
🤖 Prompt for AI Agents
In MIGRATION.md around lines 547 to 548, the guidance incorrectly implies
.storybook/main.ts will continue to work with extensionless imports under
NodeNext/Bundler—this is misleading because TypeScript with moduleResolution
"bundler" or "nodenext" requires explicit .js/.ts extensions (TS2691) unless the
legacy "node" resolver or allowImportingTsExtensions is enabled; update the
wording to state that extensionless imports are rejected by those resolvers and
either recommend adding explicit extensions or explicitly call out the exact
resolver setting (or allowImportingTsExtensions flag) required to keep
extensionless imports working.
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
Sidnioulz
changed the title
4 participants
Closes #32464
addonsShortcutLabels and addonsShortcut is initilized as empty dict on start of programs, but because of localstorage from previous session, it retrieve the list of shortcuts with previously initialized addon shortcut. This PR will hide any addon-shortcut if it have not been initiliazed in new start
Thank you for contributing to Storybook! Please submit all PRs to the
nextbranch unless they are specific to the current release. Storybook maintainers cherry-pick bug and documentation fixes into themainbranch as part of the release process, so you shouldn't need to worry about this. For additional guidance: https://storybook.js.org/docs/contribute-->
What I did
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
Removed
Documentation
Behavior