CLI: Fix Vitest v3 installs and refactor AddonVitestService; align create‑storybook usage #33131
Conversation
|
View your CI Pipeline Execution ↗ for commit 329b471
☁️ Nx Cloud last updated this comment at |
📝 WalkthroughWalkthroughConstructor-injects Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Command
participant Service
participant PM as PackageManager
rect rgb(240,240,255)
Note over User,PM: Old flow (param-passing)
User->>Command: new Command()
User->>Command: execute(packageManager, opts)
Command->>Service: new Service()
Command->>Service: methodCall(packageManager, ...)
Service->>PM: perform actions
end
rect rgb(235,255,235)
Note over User,PM: New flow (constructor injection)
User->>Command: new Command(packageManager)
Command->>Command: this.packageManager = packageManager
Command->>Service: new Service(packageManager)
User->>Command: execute(opts)
Command->>Service: methodCall(...)
Service->>PM: perform actions (via this.packageManager)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
✨ Finishing touches
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🧰 Additional context used🧬 Code graph analysis (2)code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts (2)
code/core/src/cli/AddonVitestService.test.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 (7)
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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts (1)
19-27: FeatureCompatibilityService test is wired to the old API and wrong constructor typesThis test still reflects the pre‑refactor API:
FeatureCompatibilityServiceis constructed withmockAddonVitestService, but the implementation now expects aJsPackageManager(and internally creates / accepts anAddonVitestService).validateTestFeatureCompatibilityis called withmockPackageManageras the first argument and assertsvalidateCompatibilityreceives a{ packageManager, ... }object, while the implementation now only forwards{ framework, builder, projectRoot }.As written, this will no longer type‑check or correctly exercise the new behavior.
A minimal way to realign the test with the updated API is:
describe('FeatureCompatibilityService', () => { - let service: FeatureCompatibilityService; - let mockAddonVitestService: AddonVitestService; - - beforeEach(() => { - // @ts-expect-error accept old constructor in mock context - mockAddonVitestService = new AddonVitestService({} as any); - service = new FeatureCompatibilityService(mockAddonVitestService); - }); + let service: FeatureCompatibilityService; + let mockAddonVitestService: AddonVitestService; + let mockPackageManager: JsPackageManager; + let mockValidateCompatibility: ReturnType<typeof vi.fn>; @@ - describe('validateTestFeatureCompatibility', () => { - let mockPackageManager: JsPackageManager; - let mockValidateCompatibility: ReturnType<typeof vi.fn>; - - beforeEach(() => { - mockPackageManager = { - getInstalledVersion: vi.fn(), - } as Partial<JsPackageManager> as JsPackageManager; - - // Get the mocked validateCompatibility method - mockValidateCompatibility = vi.mocked(mockAddonVitestService.validateCompatibility); - }); + beforeEach(() => { + mockPackageManager = { + getInstalledVersion: vi.fn(), + } as Partial<JsPackageManager> as JsPackageManager; + + // Local mock for AddonVitestService, injected into the service + mockAddonVitestService = { + validateCompatibility: vi.fn(), + } as unknown as AddonVitestService; + + service = new FeatureCompatibilityService(mockPackageManager, mockAddonVitestService); + mockValidateCompatibility = vi.mocked(mockAddonVitestService.validateCompatibility); + }); + + describe('validateTestFeatureCompatibility', () => { @@ - it('should return compatible when all checks pass', async () => { + it('should return compatible when all checks pass', async () => { mockValidateCompatibility.mockResolvedValue({ compatible: true }); const result = await service.validateTestFeatureCompatibility( - mockPackageManager, SupportedFramework.REACT_VITE, SupportedBuilder.VITE, '/test' ); @@ - expect(mockValidateCompatibility).toHaveBeenCalledWith({ - packageManager: mockPackageManager, - framework: 'react-vite', - builder: SupportedBuilder.VITE, - projectRoot: '/test', - }); + expect(mockValidateCompatibility).toHaveBeenCalledWith({ + framework: 'react-vite', + builder: SupportedBuilder.VITE, + projectRoot: '/test', + }); @@ - const result = await service.validateTestFeatureCompatibility( - mockPackageManager, - SupportedFramework.REACT_VITE, - SupportedBuilder.VITE, - '/test' - ); + const result = await service.validateTestFeatureCompatibility( + SupportedFramework.REACT_VITE, + SupportedBuilder.VITE, + '/test' + );This makes the test match:
- The new constructor signature (
new FeatureCompatibilityService(packageManager, addonVitestService?)).- The updated
validateTestFeatureCompatibility(framework, builder, directory)signature.- The new
AddonVitestService.validateCompatibilitycall shape.Also applies to: 45-56, 58-75, 77-93
code/core/src/cli/AddonVitestService.test.ts (2)
21-43: Fix initialization order inbeforeEach: service is created beforemockPackageManageris assignedIn
beforeEach,service = new AddonVitestService(mockPackageManager);runs whilemockPackageManageris stillundefined(first test) or before it’s reassigned (subsequent tests). As a result,this.packageManagerinside the service is eitherundefinedor points at a stale instance, and any method that callsthis.packageManager.*will fail or use the wrong mock.Reorder setup so the mock is created first and then passed into the service:
- beforeEach(() => { - vi.clearAllMocks(); - service = new AddonVitestService(mockPackageManager); - vi.mocked(getProjectRoot).mockReturnValue('/test/project'); - - mockPackageManager = { - getAllDependencies: vi.fn(), - getInstalledVersion: vi.fn(), - runPackageCommand: vi.fn(), - } as Partial<JsPackageManager> as JsPackageManager; + beforeEach(() => { + vi.clearAllMocks(); + + mockPackageManager = { + getAllDependencies: vi.fn(), + getInstalledVersion: vi.fn(), + runPackageCommand: vi.fn(), + } as Partial<JsPackageManager> as JsPackageManager; + + service = new AddonVitestService(mockPackageManager); + vi.mocked(getProjectRoot).mockReturnValue('/test/project');(Keep the logger/prompt mocks as they are after this block.)
45-246: Align tests with constructor-injectedAddonVitestServiceAPI (dropmockPackageManagerarguments)Now that
AddonVitestServicetakesJsPackageManagervia its constructor and methods usethis.packageManagerinternally, update all instance method calls to remove themockPackageManagerparameter:
collectDependencies: lines 124, 136, 148validatePackageVersions: lines 208, 220, 230, 240installPlaywright: lines 384, 412, 427, 438, 447, 455, 466- const deps = await service.collectDependencies(mockPackageManager); + const deps = await service.collectDependencies(); - const result = await service.validatePackageVersions(mockPackageManager); + const result = await service.validatePackageVersions(); - const { errors } = await service.installPlaywright(mockPackageManager); + const { errors } = await service.installPlaywright();
🧹 Nitpick comments (4)
code/addons/vitest/src/postinstall.ts (1)
61-61: AddonVitestService DI and usage look correct; minor duplication of Vitest version logicConstructing a single
AddonVitestService(packageManager)and then usingvalidateCompatibility(),collectDependencies(), andinstallPlaywright({ yes })matches the new API and keeps package-manager concerns centralized. The only minor nit is that Vitest version detection now lives both here (isVitest3_2To4/isVitest4OrNewer) and insideAddonVitestService.collectDependencies(), so if version rules change in future it might be worth pulling that into one shared helper/service to avoid divergence.Also applies to: 99-99, 141-143
code/lib/create-storybook/src/services/FeatureCompatibilityService.ts (1)
23-26: Constructor DI forAddonVitestServiceis sound; JSDoc still mentions removedpackageManagerparamInjecting
JsPackageManagervia theFeatureCompatibilityServiceconstructor and constructingAddonVitestServicewith it is a clean, testable pattern;validateTestFeatureCompatibilitycorrectly delegates tothis.addonVitestService.validateCompatibility({ framework, builder, projectRoot: directory }).The JSDoc above
validateTestFeatureCompatibilitystill lists@param packageManager, but the method signature no longer takes that argument. It would be good to drop or update that tag to avoid confusion.Also applies to: 45-49
code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (1)
24-27: Constructor‑injectedpackageManagerusage and Vitest dependency collection look correctInjecting
JsPackageManagerintoDependencyInstallationCommandand usingthis.packageManagerforaddDependencies/installDependencieslines up with the rest of the refactor, andcollectAddonDependenciescorrectly delegates Vitest devDeps tonew AddonVitestService(this.packageManager).collectDependencies().If you want to tighten intent slightly, you could mark the constructor fields as readonly:
- constructor( - private dependencyCollector: DependencyCollector, - private packageManager: JsPackageManager - ) {} + constructor( + private readonly dependencyCollector: DependencyCollector, + private readonly packageManager: JsPackageManager + ) {}but functionally this looks good.
Also applies to: 33-34, 49-52, 60-63, 71-71, 81-90, 93-103
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (1)
36-39: Addon configuration DI and Playwright installation integration look consistentPassing
JsPackageManagerintoAddonConfigurationCommandand constructing a singleAddonVitestService(packageManager)aligns with the new API, andconfigureAddonscorrectly forwardsthis.packageManager.typeintopostinstallAddonwhile keepingskipInstall/skipDependencyManagementtrue. The follow‑upaddonVitestService.installPlaywright({ yes: options.yes })hook is a clear place to centralize Playwright binary installation.If you later want more visibility into Playwright failures, you could capture and act on the
{ errors }returned frominstallPlaywright, but the current behavior matches the previous pattern of ignoring that detail here.Also applies to: 60-67, 106-107, 123-131, 162-167
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
code/addons/vitest/src/postinstall.ts(3 hunks)code/core/src/cli/AddonVitestService.test.ts(9 hunks)code/core/src/cli/AddonVitestService.ts(9 hunks)code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts(2 hunks)code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts(5 hunks)code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts(1 hunks)code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts(6 hunks)code/lib/create-storybook/src/commands/UserPreferencesCommand.test.ts(8 hunks)code/lib/create-storybook/src/commands/UserPreferencesCommand.ts(3 hunks)code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts(1 hunks)code/lib/create-storybook/src/services/FeatureCompatibilityService.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Applied to files:
code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.ts
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.tscode/core/src/cli/AddonVitestService.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.ts
🧬 Code graph analysis (10)
code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts (2)
code/lib/create-storybook/src/dependency-collector.ts (1)
DependencyCollector(19-185)code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (1)
DependencyInstallationCommand(23-91)
code/lib/create-storybook/src/commands/UserPreferencesCommand.test.ts (1)
code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (1)
UserPreferencesCommand(45-219)
code/lib/create-storybook/src/services/FeatureCompatibilityService.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
code/core/src/cli/AddonVitestService.test.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (2)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (1)
AddonConfigurationCommand(35-160)
code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (2)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/dependency-collector.ts (1)
DependencyCollector(19-185)
code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
code/addons/vitest/src/postinstall.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (2)
code/lib/create-storybook/src/services/FeatureCompatibilityService.ts (1)
FeatureCompatibilityService(22-62)code/lib/create-storybook/src/generators/types.ts (1)
CommandOptions(118-137)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (2)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/generators/types.ts (1)
CommandOptions(118-137)
⏰ 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 (8)
code/lib/create-storybook/src/commands/UserPreferencesCommand.test.ts (1)
35-45: Tests correctly follow constructor‑injectedpackageManagerand newexecuteAPI
UserPreferencesCommandis now instantiated withmockPackageManager, and all calls toexecuteuse a single options argument. This matches the refactored command signature and keeps the tests focused on behavior rather than wiring.Also applies to: 87-203
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (1)
20-27: AddonConfigurationCommand tests correctly adopt constructor‑injectedpackageManagerInstantiating
AddonConfigurationCommandwithmockPackageManagerand assertingpostinstallAddonreceivespackageManager: 'npm'matches the refactored command that readsthis.packageManager.type. The AddonVitestService mock keepsinstallPlaywrightisolated from these tests.Also applies to: 36-50
code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts (1)
11-20: DependencyInstallationCommand tests now match constructor‑injectedpackageManagerUsing a shared
mockPackageManagerand constructingDependencyInstallationCommandwith(dependencyCollector, mockPackageManager)aligns with the new command API. The existing expectations aroundaddDependencies/installDependenciesbehavior remain valid.code/core/src/cli/AddonVitestService.ts (5)
39-39: LGTM! Dependency injection simplifies the API.The constructor-based injection of
packageManagerremoves parameter threading across all methods, improving both testability and the public API surface.
82-113: LGTM! Conditional package selection prevents v3 install failures.The conditional selection between
@vitest/browser-playwright(v4+) and@vitest/browser(v3) correctly addresses the compatibility issue. Applying the version specifier to all vitest-related packages ensures ecosystem consistency.
127-175: LGTM! Method signature aligned with constructor injection.The simplified signature removes the
packageManagerparameter while maintaining all existing functionality throughthis.packageManager.
190-252: LGTM! Validation methods correctly use injected dependency.Both
validateCompatibilityandvalidatePackageVersionscorrectly referencethis.packageManagerand maintain their original validation logic.
65-79: Verify pre-release version handling and confirm edge case severity.The version detection logic has two edge cases that warrant verification:
Pre-release versions: @vitest/browser has versions ranging from v2.x through v3.x (latest 3.1.3), and in Vitest v4, @vitest/browser is now included in every provider package automatically. The semver library's
satisfies()function returns false for pre-release versions when using range comparisons like>=4.0.0—meaning a project withvitest: "4.0.0-beta.0"would be classified as v3 and install@vitest/browserinstead of@vitest/browser-playwright.Unparseable versions: If a version string cannot be parsed by
validRange/minVersionorcoerce, the code defaults toisVitest4OrNewer = true. While this is a reasonable defensive choice for the common case (no vitest installed → install latest), it could fail if a project has a malformed version string but is actually on v3.In Vitest v4, you need to install a separate package: @vitest/browser-playwright, @vitest/browser-webdriverio, or @vitest/browser-preview, and the npm data confirms these packages exist only for v4. Confirm whether pre-release versions or unparseable version strings are expected in your environment, and consider adding an explicit fallback or logging when version detection fails to handle these cases defensively.
valentinpalkovic
force-pushed
the
valentin/cleanup-and-vitest-4-fix
branch
from
44bc707 to
dacacab
Compare
valentinpalkovic
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts (1)
93-101: Fix assertion ongetAllPackages()to actually check for Vitest absence
dependencyCollector.getAllPackages()returns an object{ dependencies, devDependencies }, buttoContain('vitest')expects an array or string, so this assertion never meaningfully inspects the underlying lists.Consider asserting on the flattened dependency arrays instead, for example:
const { dependencies, devDependencies } = dependencyCollector.getAllPackages(); expect([...dependencies, ...devDependencies]).not.toContain( expect.stringContaining('vitest') );This will correctly guarantee that no Vitest packages were added when
Feature.TESTis not selected.Also applies to: 103-110
code/core/src/cli/AddonVitestService.test.ts (2)
117-156: Align tests with constructor-injected API and correct the coverage/istanbul scenarioNow that
AddonVitestServiceholds theJsPackageManagervia its constructor:
collectDependenciesandvalidatePackageVersionsno longer need a package manager parameter, yet some tests still call them withmockPackageManager(e.g., Lines 124, 136, 148, 208, 220, 230, 240). Those extra arguments are ignored, but they obscure the fact that the injectedthis.packageManageris what matters.- Similarly,
installPlaywrighttakes an options object ({ yes?: boolean }) but is still being invoked asservice.installPlaywright(mockPackageManager). This works only because the extra properties are ignored.For clarity and to better reflect the public API, consider:
- Calling
service.collectDependencies()andservice.validatePackageVersions()without arguments.- Updating the Playwright tests to call
service.installPlaywright()orservice.installPlaywright({ yes: true })and keep all assertions aroundpromptandrunPackageCommandas they are.Additionally, in
it('skips coverage if istanbul', ...), the mockedgetInstalledVersionsequence:vi.mocked(mockPackageManager.getInstalledVersion) .mockResolvedValueOnce(null) // currently used for vitest .mockResolvedValueOnce('3.0.0') // currently used for @vitest/coverage-v8 .mockResolvedValueOnce(null); // currently used for @vitest/coverage-istanbulactually simulates "
@vitest/coverage-v8installed" again rather than "istanbul-only". To truly exercise the "istanbul installed, v8 absent" path with the current implementation order (vitest→@vitest/coverage-v8→@vitest/coverage-istanbul), you likely want:vi.mocked(mockPackageManager.getInstalledVersion) .mockResolvedValueOnce(null) // vitest version .mockResolvedValueOnce(null) // @vitest/coverage-v8 .mockResolvedValueOnce('3.0.0'); // @vitest/coverage-istanbulThis will give you distinct coverage for both v8-installed and istanbul-installed branches.
Also applies to: 181-245, 373-471
21-35: FixbeforeEachordering: service is constructed with an undefined/stale packageManagerIn
beforeEach,serviceis created beforemockPackageManageris initialized, andmockPackageManageris then reassigned:beforeEach(() => { vi.clearAllMocks(); service = new AddonVitestService(mockPackageManager); // mockPackageManager is undefined on first run vi.mocked(getProjectRoot).mockReturnValue('/test/project'); mockPackageManager = { getAllDependencies: vi.fn(), getInstalledVersion: vi.fn(), runPackageCommand: vi.fn(), } as Partial<JsPackageManager> as JsPackageManager; });This means:
- The first test constructs
AddonVitestServicewithundefinedaspackageManager, causing runtime failures when methods hitthis.packageManager.*.- Subsequent tests construct
servicewith the previousmockPackageManagerinstance, while stubbing methods on the newly assigned one, so mocks don't line up with whatserviceactually uses.Reorder to initialize the mock before constructing the service:
beforeEach(() => { vi.clearAllMocks(); + + mockPackageManager = { + getAllDependencies: vi.fn(), + getInstalledVersion: vi.fn(), + runPackageCommand: vi.fn(), + } as Partial<JsPackageManager> as JsPackageManager; + service = new AddonVitestService(mockPackageManager); vi.mocked(getProjectRoot).mockReturnValue('/test/project'); - - mockPackageManager = { - getAllDependencies: vi.fn(), - getInstalledVersion: vi.fn(), - runPackageCommand: vi.fn(), - } as Partial<JsPackageManager> as JsPackageManager; });
♻️ Duplicate comments (1)
code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (1)
46-51: Constructor parameter default usesthisbefore instance initialization.In constructor parameter initializers,
thisproperties are not yet assigned when default values are evaluated. Usingthis.packageManagerin the default value forfeatureServicewill result inundefinedbeing passed toFeatureCompatibilityService.Apply this diff to reference the raw parameter instead:
export class UserPreferencesCommand { constructor( private commandOptions: CommandOptions, private packageManager: JsPackageManager, - private featureService = new FeatureCompatibilityService(this.packageManager), + private featureService = new FeatureCompatibilityService(packageManager), private telemetryService = new TelemetryService(commandOptions.disableTelemetry) ) {}
🧹 Nitpick comments (1)
code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts (1)
11-22:DependencyInstallationCommand.test.tsis missing AddonVitestService mock used by sibling testsVerification confirms the concern: other tests in the same directory (notably
AddonConfigurationCommand.test.tsandUserPreferencesCommand.test.ts) properly mockAddonVitestServiceusingvi.fn().mockImplementation(), butDependencyInstallationCommand.test.tsdoes not.When
execute()callscollectAddonDependencies()(which instantiatesAddonVitestServicewith the injectedmockPackageManagerat line 84 of the implementation), the service will attempt to callgetAllDependencies()andgetInstalledVersion()on the mock—methods that do not exist. Although errors are caught and logged internally, this creates unnecessary noise and inconsistency with the established test pattern.Consider either extending
mockPackageManagerto include no-op stubs for those methods, or add avi.mock('storybook/internal/cli')block with an implementation matching the pattern inAddonConfigurationCommand.test.ts(lines 41–47).Also applies to: 25-38, 40-48
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
code/addons/vitest/src/postinstall.ts(3 hunks)code/core/src/cli/AddonVitestService.test.ts(9 hunks)code/core/src/cli/AddonVitestService.ts(9 hunks)code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts(2 hunks)code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts(5 hunks)code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts(1 hunks)code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts(6 hunks)code/lib/create-storybook/src/commands/UserPreferencesCommand.test.ts(8 hunks)code/lib/create-storybook/src/commands/UserPreferencesCommand.ts(3 hunks)code/lib/create-storybook/src/initiate.ts(1 hunks)code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts(1 hunks)code/lib/create-storybook/src/services/FeatureCompatibilityService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- code/lib/create-storybook/src/commands/UserPreferencesCommand.test.ts
- code/lib/create-storybook/src/services/FeatureCompatibilityService.ts
- code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts
- code/addons/vitest/src/postinstall.ts
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/core/src/cli/AddonVitestService.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Applied to files:
code/core/src/cli/AddonVitestService.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.tscode/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts
📚 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/create-storybook/src/commands/UserPreferencesCommand.ts
🧬 Code graph analysis (7)
code/lib/create-storybook/src/initiate.ts (1)
code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (1)
executeUserPreferences(221-227)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (3)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/generators/types.ts (1)
CommandOptions(118-137)code/lib/create-storybook/src/commands/index.ts (1)
executeAddonConfiguration(24-24)
code/core/src/cli/AddonVitestService.test.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (2)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/dependency-collector.ts (1)
DependencyCollector(19-185)
code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (3)
code/lib/create-storybook/src/services/FeatureCompatibilityService.ts (1)
FeatureCompatibilityService(22-62)code/lib/create-storybook/src/services/TelemetryService.ts (1)
TelemetryService(10-105)code/lib/create-storybook/src/generators/types.ts (1)
CommandOptions(118-137)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (2)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (1)
AddonConfigurationCommand(35-160)
code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts (2)
code/lib/create-storybook/src/dependency-collector.ts (1)
DependencyCollector(19-185)code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (1)
DependencyInstallationCommand(23-91)
⏰ 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 (10)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (1)
20-27: AddonConfigurationCommand DI and mocks are aligned with the new APIInjecting
mockPackageManagerintoAddonConfigurationCommandand asserting thatpostinstallAddonreceivespackageManager: 'npm'matches the updated constructor andconfigureAddonsimplementation. TheAddonVitestServicemock andpostinstallAddonspy setup look consistent and should keep these tests decoupled from real CLI behavior.Also applies to: 36-49, 129-164
code/lib/create-storybook/src/initiate.ts (1)
60-67: UpdatedexecuteUserPreferencescall correctly matches the new object-based APIPassing a single options object including
packageManager,options,framework,builder, andprojectTypeis consistent with the refactoredexecuteUserPreferenceshelper. This keepsdoInitiatein sync with the new constructor/execute signature ofUserPreferencesCommandand ensures project type is available for preference handling.code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (1)
23-33: DependencyInstallationCommand DI refactor and Vitest integration look soundInjecting
JsPackageManagervia the constructor and usingthis.packageManagerthroughoutexecutesimplifies the call sites and cleanly supports the newAddonVitestServiceAPI. ThecollectAddonDependencieshelper correctly:
- Checks
selectedFeaturesforFeature.TEST,- Uses
new AddonVitestService(this.packageManager).collectDependencies()to derive devDeps, and- Safely degrades by logging and continuing on errors.
The
executeDependencyInstallationwrapper now passes bothdependencyCollectorandpackageManagerinto the command and forwards only the relevantskipInstall/selectedFeaturesfields intoexecute, which keeps responsibilities well separated.Also applies to: 46-52, 55-64, 68-77, 80-90, 93-102
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (3)
36-39: LGTM! Constructor injection pattern applied correctly.The constructor now accepts
packageManagerand properly initializesaddonVitestServicewith it, centralizing dependency management within the service.
61-66: LGTM! Method calls updated to use injected dependencies.Both
configureAddonsandaddonVitestService.installPlaywrightcalls correctly rely on the injectedpackageManagerinstance rather than passing it as a parameter.
162-166: LGTM! Wrapper function correctly implements new pattern.The
executeAddonConfigurationwrapper properly extractspackageManager, instantiates the command with it, and forwards the remaining options toexecute.code/core/src/cli/AddonVitestService.ts (3)
65-79: LGTM! Version detection prevents v3 incompatibility issues.The logic correctly determines the Vitest version by checking both installed and declared dependencies, then uses semver helpers (
validRange,minVersion,coerce,satisfies) to conditionally install@vitest/browser-playwright(v4+) or@vitest/browser(v3).The default of
isVitest4OrNewer = trueis pragmatic: if vitest is uninstalled/undeclared, the latest version (likely v4+) will be installed anyway. If parsing fails on a declared version, assuming newer is safer than blocking installation.
82-86: LGTM! Conditional package selection addresses the PR objective.This change directly solves the install failure for Vitest v3 projects by selecting the correct browser package based on the detected version.
39-39: LGTM! Constructor injection consistently applied.All method signatures correctly updated to rely on
this.packageManagerinstead of accepting it as a parameter, simplifying the API surface.Also applies to: 61-61, 127-127, 228-228
code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (1)
221-226: LGTM! Wrapper function correctly implements constructor injection.The
executeUserPreferenceswrapper properly extractspackageManagerandoptions, instantiates the command with them, and forwards the remaining options toexecute.
…eManager; align create-storybook and postinstall call sites; fix Vitest v3 projects to avoid adding @vitest/browser-playwright by default
valentinpalkovic
force-pushed
the
valentin/cleanup-and-vitest-4-fix
branch
from
dacacab to
01e3d54
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts (1)
24-25: Consider using a proper mock packageManager.The
@ts-expect-errorwith{} as anybypasses type checking. While this works because the vi.mock provides method implementations, a proper mock packageManager would be more maintainable:beforeEach(() => { - // @ts-expect-error accept old constructor in mock context - mockAddonVitestService = new AddonVitestService({} as any); + const mockPackageManager = { + getAllDependencies: vi.fn().mockReturnValue({}), + getInstalledVersion: vi.fn().mockResolvedValue(undefined), + } as Partial<JsPackageManager> as JsPackageManager; + mockAddonVitestService = new AddonVitestService(mockPackageManager); service = new FeatureCompatibilityService(mockAddonVitestService); });This makes the test more explicit and avoids potential issues if AddonVitestService's constructor logic changes.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
code/addons/vitest/src/postinstall.ts(3 hunks)code/core/src/cli/AddonVitestService.test.ts(9 hunks)code/core/src/cli/AddonVitestService.ts(9 hunks)code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts(2 hunks)code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts(5 hunks)code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts(1 hunks)code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts(6 hunks)code/lib/create-storybook/src/commands/UserPreferencesCommand.test.ts(8 hunks)code/lib/create-storybook/src/commands/UserPreferencesCommand.ts(3 hunks)code/lib/create-storybook/src/initiate.ts(1 hunks)code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts(1 hunks)code/lib/create-storybook/src/services/FeatureCompatibilityService.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- code/lib/create-storybook/src/commands/DependencyInstallationCommand.test.ts
- code/lib/create-storybook/src/initiate.ts
- code/core/src/cli/AddonVitestService.test.ts
- code/lib/create-storybook/src/commands/UserPreferencesCommand.test.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-17T07:31:04.432Z
Learnt from: ndelangen
Repo: storybookjs/storybook PR: 32484
File: code/core/package.json:326-326
Timestamp: 2025-09-17T07:31:04.432Z
Learning: In Storybook's core package, dependencies like `open` are bundled into the final distribution during the build process, so they should remain in devDependencies rather than being moved to dependencies. End users don't need these packages as separate runtime dependencies since they're included in the bundled code.
Applied to files:
code/lib/create-storybook/src/commands/DependencyInstallationCommand.tscode/core/src/cli/AddonVitestService.ts
📚 Learning: 2025-09-29T13:20:23.346Z
Learnt from: mrginglymus
Repo: storybookjs/storybook PR: 32556
File: code/core/package.json:309-313
Timestamp: 2025-09-29T13:20:23.346Z
Learning: The `fast-printf` dependency in Storybook's core package is bundled into the final distribution during the build process, so it should remain in devDependencies rather than being moved to dependencies, following the same pattern as other bundled dependencies like `open`.
Applied to files:
code/lib/create-storybook/src/commands/DependencyInstallationCommand.tscode/core/src/cli/AddonVitestService.ts
📚 Learning: 2025-11-05T09:37:25.920Z
Learnt from: Sidnioulz
Repo: storybookjs/storybook PR: 32458
File: code/core/src/components/components/tooltip/WithTooltip.tsx:54-96
Timestamp: 2025-11-05T09:37:25.920Z
Learning: Repo: storybookjs/storybook — In code/core/src/components/components/tooltip/WithTooltip.tsx, the legacy WithTooltip implementation is intentionally reintroduced for backward compatibility and is deprecated; maintainers (per Sidnioulz) do not want maintenance or improvements on it. Prefer WithTooltipNew/Popover; avoid suggesting changes to WithTooltip.* going forward.
Applied to files:
code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts
📚 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/create-storybook/src/commands/UserPreferencesCommand.ts
🧬 Code graph analysis (7)
code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (3)
code/lib/create-storybook/src/dependency-collector.ts (1)
DependencyCollector(19-185)code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/commands/index.ts (1)
executeDependencyInstallation(26-26)
code/lib/create-storybook/src/services/FeatureCompatibilityService.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
code/lib/create-storybook/src/services/FeatureCompatibilityService.test.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (2)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (1)
AddonConfigurationCommand(35-160)
code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (2)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)code/lib/create-storybook/src/generators/types.ts (1)
CommandOptions(118-137)
code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (3)
code/lib/create-storybook/src/generators/types.ts (1)
CommandOptions(118-137)code/lib/create-storybook/src/services/FeatureCompatibilityService.ts (1)
FeatureCompatibilityService(22-62)code/lib/create-storybook/src/services/TelemetryService.ts (1)
TelemetryService(10-105)
code/addons/vitest/src/postinstall.ts (1)
code/core/src/cli/AddonVitestService.ts (1)
AddonVitestService(38-384)
⏰ 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 (18)
code/core/src/cli/AddonVitestService.ts (4)
39-39: LGTM: Constructor properly injects packageManager.The constructor now accepts packageManager as a required dependency, enabling all methods to use
this.packageManagerinternally. This eliminates parameter threading.
85-86: Correct browser package selection based on Vitest version.The conditional logic correctly installs:
@vitest/browser-playwrightfor Vitest v4+@vitest/browserfor Vitest v3.xThis addresses the PR's main goal of preventing v3 install failures.
96-99: LGTM: Refactored methods use injected packageManager.All methods (
collectDependencies,installPlaywright,validatePackageVersions) now usethis.packageManagerinstead of accepting it as a parameter. Signatures simplified accordingly.Also applies to: 127-149
61-79: The version detection logic is correct and well-designed.After examining the implementation, the dual-detection mechanism is intentional and robust:
getInstalledVersion()(JsPackageManager.ts:615-649) usesfindInstallations()which delegates to package manager CLIs (npm ls,yarn list,pnpm list), covering installed packages in node_modules or pnp zips.
getAllDependencies()(JsPackageManager.ts:221-232) explicitly supports monorepos by iterating throughpackageJsonPathsand merging alldependencies,devDependencies, andpeerDependenciesfrom every package.json file.Tests confirm the design (AddonVitestService.test.ts:58) verify expected behavior when
getInstalledVersionreturns null, and this pattern is consistently used elsewhere (e.g.,helpers.ts).For both checks to fail on an existing Vitest v3 installation would require a severely corrupted installation or an unusual monorepo misconfiguration. The default
isVitest4OrNewer = trueis appropriate for fresh setups and acts as a safe fallback.code/addons/vitest/src/postinstall.ts (2)
61-61: LGTM: AddonVitestService instantiated with packageManager.Correctly constructs the service with the required packageManager dependency.
99-99: LGTM: Method calls updated to new signatures.
collectDependencies()andinstallPlaywright({ yes })called without packageManager parameter, matching the refactored API.Also applies to: 141-143
code/lib/create-storybook/src/commands/AddonConfigurationCommand.test.ts (1)
22-26: LGTM: Test setup updated for constructor injection.The shared
mockPackageManagerand command construction align with the new dependency injection pattern.Also applies to: 49-49
code/lib/create-storybook/src/commands/DependencyInstallationCommand.ts (3)
24-28: LGTM: Constructor properly initializes dependencies.The command now accepts
packageManagerand uses it to initializeaddonVitestService, following the same pattern as other commands in this refactor.
85-85: LGTM: Dependency collection updated to new API.
collectDependencies()correctly called without the packageManager parameter.
94-101: LGTM: Wrapper function updated for dependency injection.The
executeDependencyInstallationwrapper now accepts and passespackageManagerto the constructor, maintaining a clean separation between construction and execution.code/lib/create-storybook/src/services/FeatureCompatibilityService.ts (2)
23-26: LGTM: Service initialized with packageManager.The constructor properly accepts
packageManagerand uses it to initializeaddonVitestService, consistent with the refactoring pattern.
45-54: LGTM: Method signature simplified.
validateTestFeatureCompatibilityno longer needs packageManager as a parameter since it's available viathis.addonVitestService.code/lib/create-storybook/src/commands/AddonConfigurationCommand.ts (3)
36-39: LGTM: Constructor dependency injection implemented.The command now receives
packageManageras a constructor parameter and uses it to initializeaddonVitestService.
106-106: LGTM: Internal methods use injected packageManager.
configureAddonsno longer needs packageManager as a parameter. Line 124 correctly usesthis.packageManager.type.Also applies to: 124-124
162-167: LGTM: Wrapper function follows injection pattern.
executeAddonConfigurationextractspackageManagerfrom params and constructs the command with it, then passes remaining options toexecute().code/lib/create-storybook/src/commands/UserPreferencesCommand.ts (3)
46-51: LGTM: Constructor properly initializes services with packageManager.The constructor correctly uses the
packageManagerparameter (notthis.packageManager) in the default initializer forfeatureService. This addresses the previous review concern about accessingthisbefore instance creation.
54-54: LGTM: Method signature simplified.
execute()no longer needs packageManager as a parameter since it's available via the injectedfeatureService.
221-227: LGTM: Wrapper follows consistent injection pattern.
executeUserPreferencesextractspackageManagerandoptions, constructs the command with both, then executes with remaining parameters.
Closes #
What I did
4+ is detected.
threading.
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
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.