CLI: Fix 'beforeVersion' evaluation for Storybook package #33141
Conversation
valentinpalkovic
changed the title
|
View your CI Pipeline Execution ↗ for commit 4dcfdc4
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughPackage manager proxies now resolve module package.json lookups from each package's Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Main as getStorybookData
participant Info as getStorybookInfo
participant PM as PackageManager
Note over Caller,PM: New flow — split spec vs installed
Caller->>Main: getStorybookData({ configDir })
Main->>Info: getStorybookInfo(dirname(configDir || process.cwd()))
Info-->>Main: { versionSpecifier }
Main->>PM: getModulePackageJSON('storybook', /* implicit cwd */)
PM-->>Main: { versionInstalled }
Main-->>Caller: { versionSpecifier, versionInstalled }
sequenceDiagram
participant Caller
participant PMBase as JsPackageManager
participant Proxy as Proxy (BUN/NPM/Yarn/PNPM)
Note over Caller,Proxy: Old lookup used `this.cwd`
PMBase->>Proxy: getModulePackageJSON('pkg')
Proxy->>Proxy: find.up('package.json', { cwd: this.cwd })
Proxy-->>PMBase: PackageJson | null
Note over Caller,Proxy: New lookup uses `operationDir`
PMBase->>Proxy: getModulePackageJSON('pkg', cwd?)
Proxy->>Proxy: find.up('package.json', { cwd: this.primaryPackageJson.operationDir })
Proxy-->>PMBase: PackageJson | null
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (4)**/*.{ts,tsx,js,jsx,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
**/*.{ts,tsx,js,jsx,json,html,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
code/**/!(*.test).{ts,tsx,js,mjs}📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Files:
🧠 Learnings (1)📓 Common learnings🧬 Code graph analysis (1)code/core/src/common/js-package-manager/PNPMProxy.ts (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
🔇 Additional comments (1)
Comment |
valentinpalkovic
force-pushed
the
valentin/fix-storybook-version-evaluation
branch
from
528d942 to
9f09c7e
Compare
Package BenchmarksCommit: No significant changes detected, all good. 👏 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
code/lib/cli-storybook/src/doctor/index.ts (1)
194-204: OptionalstorybookVersionis handled safely, but consider simplifying the error path
- Declaring
storybookVersionasstring | undefinedand making the parameter togetDoctorDiagnosticsoptional is consistent withProjectDoctorDataand is safely guarded by the earlyif (!storybookVersion) { … return results; }check, so the new typing is sound.- Independently of this change,
doctor()’stry/catchcurrently pushes a configuration error intodiagnosticResultsbut then proceeds to callcollectDoctorResultsByProjectwith potentially uninitializedconfigDir/packageManager/mainConfig. BecausegetDoctorDiagnosticsreturns early whenstorybookVersionis falsy, this doesn’t crash, but it does ignore the more specific configuration error you built.You might want to early-return from
doctor()inside thecatch(or routediagnosticResultsintodisplayDoctorResults) to avoid carrying partially initialized state and to surface the more precise configuration error.Also applies to: 243-268
code/lib/cli-storybook/src/automigrate/fixes/remove-essentials.ts (1)
129-139: Updated destructuring matches actual usage; consider cleaning up unused fieldsRestricting the destructuring in
run()to{ hasEssentials, hasDocsDisabled, additionalAddonsToRemove, essentialsOptions }matches what the function actually uses and avoids unused locals.Given that
AddonDocsOptionsstill includeshasDocsAddonandallDepsthatrun()never reads, you might either:
- Use them to avoid re-adding
@storybook/addon-docswhen it’s already present, or- Remove them from
AddonDocsOptionsand thecheckresult if they’re intentionally unused.Not urgent, but tightening the options shape would make this migration easier to follow.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
code/core/src/common/utils/cli.ts(0 hunks)code/lib/cli-storybook/src/add.test.ts(0 hunks)code/lib/cli-storybook/src/automigrate/fixes/remove-essentials.ts(1 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(2 hunks)code/lib/cli-storybook/src/automigrate/index.test.ts(3 hunks)code/lib/cli-storybook/src/automigrate/index.ts(0 hunks)code/lib/cli-storybook/src/doctor/index.ts(2 hunks)code/lib/cli-storybook/src/doctor/types.ts(1 hunks)
💤 Files with no reviewable changes (3)
- code/lib/cli-storybook/src/add.test.ts
- code/lib/cli-storybook/src/automigrate/index.ts
- code/core/src/common/utils/cli.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/lib/cli-storybook/src/doctor/types.tscode/lib/cli-storybook/src/automigrate/index.test.tscode/lib/cli-storybook/src/automigrate/fixes/remove-essentials.tscode/lib/cli-storybook/src/doctor/index.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/lib/cli-storybook/src/doctor/types.tscode/lib/cli-storybook/src/automigrate/index.test.tscode/lib/cli-storybook/src/automigrate/fixes/remove-essentials.tscode/lib/cli-storybook/src/doctor/index.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/lib/cli-storybook/src/doctor/types.tscode/lib/cli-storybook/src/automigrate/index.test.tscode/lib/cli-storybook/src/automigrate/fixes/remove-essentials.tscode/lib/cli-storybook/src/doctor/index.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/lib/cli-storybook/src/doctor/types.tscode/lib/cli-storybook/src/automigrate/fixes/remove-essentials.tscode/lib/cli-storybook/src/doctor/index.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/lib/cli-storybook/src/automigrate/index.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.test.{ts,tsx}: Write meaningful unit tests that import and call functions being tested, not just verify syntax patterns
Use 'yarn vitest run --coverage ' to run tests with coverage reports and aim for 75%+ coverage of statements/lines
Focus test coverage on all branches, conditions, edge cases, error paths, and different input variations
Use 'vi.mock()' to mock external dependencies like file system and loggers in unit tests
Files:
code/lib/cli-storybook/src/automigrate/index.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/lib/cli-storybook/src/automigrate/index.test.ts
🧠 Learnings (9)
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/lib/cli-storybook/src/doctor/types.tscode/lib/cli-storybook/src/automigrate/fixes/remove-essentials.tscode/lib/cli-storybook/src/doctor/index.ts
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
code/lib/cli-storybook/src/doctor/types.tscode/lib/cli-storybook/src/doctor/index.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: The Storybook repository is a TypeScript/React monorepo with main codebase in 'code/' directory and tooling in 'scripts/' directory
Applied to files:
code/lib/cli-storybook/src/doctor/types.tscode/lib/cli-storybook/src/doctor/index.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid inline mock implementations within test cases in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: The useGlobals hook from storybook/manager-api returns a tuple where the third element (storyGlobals) is typed as Globals, not Globals | undefined. This means TypeScript guarantees it's always defined, making the `in` operator safe to use without additional null checks.
Applied to files:
code/lib/cli-storybook/src/doctor/index.ts
📚 Learning: 2025-09-18T20:51:06.618Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/viewport/components/Tool.tsx:38-39
Timestamp: 2025-09-18T20:51:06.618Z
Learning: In viewport tool code, when using the `useGlobals` hook from storybook/manager-api, the third returned value `storyGlobals` is guaranteed by TypeScript to be defined (not undefined/null), making the `in` operator safe to use without additional null checks.
Applied to files:
code/lib/cli-storybook/src/doctor/index.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/lib/cli-storybook/src/doctor/types.ts (1)
9-14: OptionalstorybookVersiononProjectDoctorDatais correctMaking
storybookVersionoptional matches howgetDoctorDiagnosticshandles missing versions and allows callers to represent “version unknown” without casting. No issues from this change alone.code/lib/cli-storybook/src/automigrate/index.test.ts (1)
116-121: Test wrapper now correctly relies on detected “from” version instead of passingbeforeVersionUpdating
runFixWrapperto accept onlystorybookVersionand adjusting the call sites to pass{ storybookVersion: '7.0.0' }aligns the tests with the newrunFixescontract, where the “from” version is derived viagetModulePackageJSON(mocked tobeforeVersion). This matches the desired behavior for upgrade flows and avoids reintroducing the old mis-threadedbeforeVersionparameter.Also applies to: 156-183
…er and update related references across utils, telemetry, and automigration modules.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
code/lib/cli-storybook/src/util.ts (1)
285-352: UsingversionInstalledto drivebeforeVersion/upgrade logic correctly fixes the “from 9.1.1 to 9.1.1” bugSwitching
processProjectto:
- read
versionInstalledfromgetStorybookData,- validate and compare against
currentCLIVersion, and- surface
beforeVersion: versionInstalled,means the upgrade flow now reflects the project’s actual installed Storybook version when computing
isUpgradeand when displaying the “from → to” message. This directly addresses the misreported “from” version in the linked issue.Two follow‑ups to consider:
Robustness when
storybookmeta package is missing
versionInstalledcurrently comes frompackageManager.getModulePackageJSON('storybook')?.version. In projects that only depend on view‑layer packages (e.g.@storybook/react) and not thestorybookmeta package, this may beundefined, causingvalidateVersionto throwUpgradeStorybookUnknownCurrentVersionError. If such setups are still meant to be supported byupgrade, you may want to:// Sketch only; adapt to existing helpers const versionInstalled = (await packageManager.getModulePackageJSON('storybook'))?.version ?? (await getInstalledStorybookVersion(packageManager));so you fall back to the more generic resolution logic when the meta package isn’t present.
Clarify the meaning of
beforeVersionNow that
beforeVersionis always derived fromversionInstalled, its semantics are “installed version before running this CLI”, not “CLI’s own version”. That seems desired, but it may be worth double‑checking any downstream consumers (logging/UI, doctor, automigrate) for assumptions aboutbeforeVersionbeing a CLI version rather than a project version.To verify behavior across edge cases (meta package absent, canary installs), you can run targeted upgrades in a few minimal projects (with and without
storybookindependencies) and confirm that:
- the CLI prints the correct “Upgrading from X to Y” message, and
- non‑canary flows don’t unexpectedly throw
UpgradeStorybookUnknownCurrentVersionError.code/lib/cli-storybook/src/automigrate/index.test.ts (1)
51-193: Tests correctly adapted toversionInstalledflow and cleaner Fix typingThe updates in this block look sound:
const fixes: Fix[] = [...]relies onFix’s default generic and removes the explicitany, improving type safety without changing behavior.PackageManagerimplementinggetModulePackageJSON(packageName, basePath?)and the castnew PackageManager() as unknown as JsPackageManagerremain compatible with the updatedJsPackageManagersignature and keep the mock simple for these tests.runFixWrappernow only takes{ storybookVersion }, reflecting thatbeforeVersionis derived from the mockedgetModulePackageJSONrather than being passed explicitly.runAutomigrateWrapper’sgetStorybookData.mockResolvedValue({ ...common, beforeVersion, versionInstalled: storybookVersion, isLatest: true })aligns with the realgetStorybookDatareturn shape after the refactor, whiledoAutomigrate({ configDir, fixes })exercises the new fixes‑injection path.Optionally, you might move more of the mock configuration into
beforeEach(e.g. thegetStorybookData.mockResolvedValuefor specific scenarios) to more closely follow the repo’s spy‑mocking conventions, but the current structure is functionally correct.code/core/src/common/utils/get-storybook-info.ts (1)
75-100: NewgetStorybookVersionSpecifiercleanly exposes the dependency range, with a small path refinement to considerThe new flow:
- scans all
package.jsonfiles underdirname(configDir)viaJsPackageManager.listAllPackageJsonPaths,- finds the first
storybookdependency acrossdependencies/devDependencies/peerDependencies, and- threads the resulting version string through
getStorybookInfoasversionSpecifier,nicely decouples the declared Storybook range (specifier) from the installed version used elsewhere. This aligns with the type change on
CoreCommon_StorybookInfoand the telemetry update.One small improvement to consider:
getConfigInfo(configDir)may adjust the effective config directory (e.g. by parsing thestorybookscript), butgetStorybookVersionSpecifieris still called with the originalconfigDir. Using the resolved path would make the lookup more robust in non‑standard setups:const configInfo = getConfigInfo(configDir); const versionSpecifier = getStorybookVersionSpecifier(configInfo.configDir);This keeps behavior unchanged for the common case while better matching how the rest of
getStorybookInfoalready respectsconfigInfo.configDir.Also applies to: 153-159, 181-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
code/core/src/common/utils/get-storybook-info.ts(2 hunks)code/core/src/telemetry/storybook-metadata.ts(1 hunks)code/core/src/types/modules/core-common.ts(1 hunks)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts(3 hunks)code/lib/cli-storybook/src/automigrate/index.test.ts(6 hunks)code/lib/cli-storybook/src/automigrate/index.ts(3 hunks)code/lib/cli-storybook/src/automigrate/types.ts(0 hunks)code/lib/cli-storybook/src/doctor/index.ts(3 hunks)code/lib/cli-storybook/src/util.ts(3 hunks)
💤 Files with no reviewable changes (1)
- code/lib/cli-storybook/src/automigrate/types.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
- code/lib/cli-storybook/src/doctor/index.ts
- code/lib/cli-storybook/src/automigrate/index.ts
🧰 Additional context used
📓 Path-based instructions (7)
**/*.{ts,tsx,js,jsx,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use camelCase for variable and function names
Files:
code/core/src/telemetry/storybook-metadata.tscode/core/src/types/modules/core-common.tscode/lib/cli-storybook/src/util.tscode/core/src/common/utils/get-storybook-info.tscode/lib/cli-storybook/src/automigrate/index.test.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes
Files:
code/core/src/telemetry/storybook-metadata.tscode/core/src/types/modules/core-common.tscode/lib/cli-storybook/src/util.tscode/core/src/common/utils/get-storybook-info.tscode/lib/cli-storybook/src/automigrate/index.test.ts
**/*.{ts,tsx,js,jsx,json,html,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.{ts,tsx,js,jsx,json,html,mjs}: Use ESLint and Prettier for code style enforcement
Run 'yarn prettier --write ' to format code after making changes
Run 'yarn lint:js:cmd ' to check for ESLint issues after making changes
Files:
code/core/src/telemetry/storybook-metadata.tscode/core/src/types/modules/core-common.tscode/lib/cli-storybook/src/util.tscode/core/src/common/utils/get-storybook-info.tscode/lib/cli-storybook/src/automigrate/index.test.ts
code/**/!(*.test).{ts,tsx,js,mjs}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
code/**/!(*.test).{ts,tsx,js,mjs}: Use 'logger' from 'storybook/internal/node-logger' for server-side (Node.js) logging, not console.log/console.warn/console.error
Use 'logger' from 'storybook/internal/client-logger' for client-side (browser) logging, not console.log/console.warn/console.error
Do not use console.log, console.warn, or console.error directly unless in isolated files where importing loggers would significantly increase bundle size
Files:
code/core/src/telemetry/storybook-metadata.tscode/core/src/types/modules/core-common.tscode/lib/cli-storybook/src/util.tscode/core/src/common/utils/get-storybook-info.ts
**/*.{test,spec}.{ts,tsx}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{test,spec}.{ts,tsx}: Test files should follow the naming pattern*.test.ts,*.test.tsx,*.spec.ts, or*.spec.tsx
Follow the spy mocking rules defined in.cursor/rules/spy-mocking.mdcfor consistent mocking patterns with Vitest
Files:
code/lib/cli-storybook/src/automigrate/index.test.ts
**/*.test.{ts,tsx}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.test.{ts,tsx}: Write meaningful unit tests that import and call functions being tested, not just verify syntax patterns
Use 'yarn vitest run --coverage ' to run tests with coverage reports and aim for 75%+ coverage of statements/lines
Focus test coverage on all branches, conditions, edge cases, error paths, and different input variations
Use 'vi.mock()' to mock external dependencies like file system and loggers in unit tests
Files:
code/lib/cli-storybook/src/automigrate/index.test.ts
**/*.test.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (.cursor/rules/spy-mocking.mdc)
**/*.test.{ts,tsx,js,jsx}: Usevi.mock()with thespy: trueoption for all package and file mocks in Vitest tests
Place all mocks at the top of the test file before any test cases
Usevi.mocked()to type and access the mocked functions in Vitest tests
Implement mock behaviors inbeforeEachblocks in Vitest tests
Mock all required dependencies that the test subject uses
Each mock implementation should return a Promise for async functions in Vitest
Mock implementations should match the expected return type of the original function
Mock all required properties and methods that the test subject uses in Vitest tests
Avoid direct function mocking withoutvi.mocked()in Vitest tests
Avoid mock implementations outside ofbeforeEachblocks in Vitest tests
Avoid mocking without thespy: trueoption in Vitest tests
Avoid inline mock implementations within test cases in Vitest tests
Avoid mocking only a subset of required dependencies in Vitest tests
Mock at the highest level of abstraction needed in Vitest tests
Keep mock implementations simple and focused in Vitest tests
Use type-safe mocking withvi.mocked()in Vitest tests
Document complex mock behaviors in Vitest tests
Group related mocks together in Vitest tests
Files:
code/lib/cli-storybook/src/automigrate/index.test.ts
🧠 Learnings (10)
📓 Common learnings
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
📚 Learning: 2025-11-05T09:38:47.712Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/Select/Select.tsx:200-204
Timestamp: 2025-11-05T09:38:47.712Z
Learning: Repo: storybookjs/storybook — Guidance: Until Storybook 11 is released, do not suggest using React.useId anywhere (e.g., in code/core/src/components/components/Select/Select.tsx) to maintain compatibility with React 17 runtimes. Prefer advising: accept a caller-provided props.id and, if needed, generate a client-only fallback id to minimize SSR hydration issues — but avoid useId. Resume prompting for useId after Storybook 11.
Applied to files:
code/core/src/telemetry/storybook-metadata.tscode/lib/cli-storybook/src/util.ts
📚 Learning: 2025-10-02T09:22:13.215Z
Learnt from: JReinhold
Repo: storybookjs/storybook PR: 32607
File: code/package.json:243-243
Timestamp: 2025-10-02T09:22:13.215Z
Learning: The Storybook repository uses Yarn v^4 (any 4.x version) as the package manager, configured via .yarnrc.yml and package.json packageManager field. Specific patch versions within v4 can be upgraded as needed.
Applied to files:
code/lib/cli-storybook/src/util.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mocking only a subset of required dependencies in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: Applies to **/*.test.{ts,tsx} : Focus test coverage on all branches, conditions, edge cases, error paths, and different input variations
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:47.649Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.649Z
Learning: Applies to **/*.test.{ts,tsx} : Write meaningful unit tests that import and call functions being tested, not just verify syntax patterns
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Implement mock behaviors in `beforeEach` blocks in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Mock all required dependencies that the test subject uses
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Avoid mock implementations outside of `beforeEach` blocks in Vitest tests
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
📚 Learning: 2025-11-24T17:49:59.268Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .cursor/rules/spy-mocking.mdc:0-0
Timestamp: 2025-11-24T17:49:59.268Z
Learning: Applies to **/*.test.{ts,tsx,js,jsx} : Each mock implementation should return a Promise for async functions in Vitest
Applied to files:
code/lib/cli-storybook/src/automigrate/index.test.ts
🧬 Code graph analysis (2)
code/lib/cli-storybook/src/util.ts (1)
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
getStorybookData(102-159)
code/lib/cli-storybook/src/automigrate/index.test.ts (3)
code/lib/cli-storybook/src/automigrate/types.ts (1)
Fix(52-60)code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts (1)
getStorybookData(102-159)code/lib/cli-storybook/src/automigrate/index.ts (1)
doAutomigrate(43-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: normal
- GitHub Check: Core Unit Tests, windows-latest
🔇 Additional comments (2)
code/core/src/types/modules/core-common.ts (1)
685-693: CoreCommon_StorybookInfo field rename aligns with new versionSpecifier semanticsRenaming
versiontoversionSpecifierhere matches howgetStorybookInfoand telemetry now expose Storybook’s dependency range, not the installed version. This keeps the types consistent across the codepaths usingCoreCommon_StorybookInfo.code/core/src/telemetry/storybook-metadata.ts (1)
238-249: Telemetry now pullsstorybookVersionSpecifierfromgetStorybookInfoWiring
storybookVersionSpecifiertostorybookInfo.versionSpecifier ?? ''correctly aligns telemetry with the newCoreCommon_StorybookInfoshape and ensures you report the user’s dependency specifier (e.g.^9.0.0orworkspace:*) rather than the CLI’s own version, while preserving a safe empty‑string fallback.
code/lib/cli-storybook/src/automigrate/helpers/mainConfigFile.ts
Outdated
Show resolved
Hide resolved
…onPath resolution
valentinpalkovic
deleted the
valentin/fix-storybook-version-evaluation
branch
Closes #32211
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 pull request has been released as version
0.0.0-pr-33141-sha-42b6c68a. 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-33141-sha-42b6c68avalentin/fix-storybook-version-evaluation42b6c68a1764059276)To request a new release of this pull request, mention the
@storybookjs/coreteam.core team members can create a new canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=33141Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.