Core: Join framework preset path with slash #32838

brandonroberts · 2025-10-25T17:23:53Z

Closes #

What I did

Fixes an issue for scoped npm packages on Windows with Storybook 10 RC. When@analogjs/storybook-angular is resolved on Windows, it is joined as @analogjs\\storybook-angular\\preset which fails the validation check in exsolve for resolving valid package names.

PR with workaround for Storybook 10 RC

brandonroberts/angular-vite-storybook#22

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

stories
unit tests
integration tests
end-to-end tests

Manual testing

This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!

Documentation

Add or update documentation reflecting your changes
If you are deprecating/removing a feature, make sure to update
MIGRATION.MD

Checklist for Maintainers

When this PR is ready for testing, make sure to add ci:normal, ci:merged or ci:daily GH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found in code/lib/cli-storybook/src/sandbox-templates.ts
Make 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/core team 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
- Improved framework preset resolution and validation to correctly handle community and scoped framework names, reducing false-positive failures.
Tests
- Stabilized validation tests by centralizing resolution behavior in mocks, making tests reliably simulate resolvable and unresolvable scenarios and preventing intermittent flakiness.

coderabbitai · 2025-10-25T17:24:17Z

📝 Walkthrough

Walkthrough

Replaces construction of framework preset module specifier from path.join(frameworkName, 'preset') to a template literal ${frameworkName}/preset, and updates tests to mock exsolve.resolveModulePath instead of require.resolve, toggling resolution success/failure via mocked implementations.

Changes

Cohort / File(s)	Summary
Framework validation logic `code/core/src/common/utils/validate-config.ts`	Replace `path.join(frameworkName, 'preset')` with string template `${frameworkName}/preset`; remove `path.join` import; keep same resolve options when calling module resolution.
Validation tests `code/core/src/common/utils/validate-config.test.ts`	Replace per-test `require.resolve` mocks with mocked `exsolve.resolveModulePath`; tests toggle mocked implementation to simulate resolvable/unresolvable modules and adjust expectations accordingly; reset mocks between tests.

Sequence Diagram(s)

sequenceDiagram
  participant Validator as validate-config
  participant Exsolve as exsolve.resolveModulePath
  participant Node as Node module resolver

  rect rgba(200,230,255,0.35)
    Validator->>Exsolve: call resolveModulePath("${frameworkName}/preset", options)
    Exsolve-->>Validator: resolved path or throw
  end

  alt resolved
    Validator->>Validator: accept framework as community/known
  else throw
    Validator->>Validator: treat as unresolved -> throw error
  end

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Areas to inspect:
- validate-config.ts: confirm ${frameworkName}/preset is equivalent to previous path.join(...) for all target environments and resolver semantics (scoped names, platform path separators).
- validate-config.test.ts: ensure mocked exsolve.resolveModulePath covers all prior scenarios and that mocks are reset between tests.

Possibly related PRs

Core: Use exsolve resolveModulePath for safeResolveModule #32477 — Similar change replacing path.join(...) with template-literal module paths and moving tests to exsolve.resolveModulePath-based resolution.

✨ Finishing touches

📝 Generate docstrings

🧪 Generate unit tests (beta)

Create PR with unit tests
Post copyable unit tests in a comment

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 26438fc and c144ed0.

📒 Files selected for processing (1)

code/core/src/common/utils/validate-config.test.ts (2 hunks)

🚧 Files skipped from review as they are similar to previous changes (1)

code/core/src/common/utils/validate-config.test.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). (1)

GitHub Check: normal

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

coderabbitai

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 (1)

code/core/src/common/utils/validate-config.test.ts (1)
22-26: Pre-existing: Test description contradicts expectation.

The test description states "should not throw" but expects the validation to throw (.toThrow()). This logical inconsistency should be corrected.

Apply this diff to fix the description:
-  it('should not throw if framework is unknown (community) but can be resolved', () => {
+  it('should throw if framework is unknown (community) and mocking require.resolve does not help', () => {
     // mock require.resolve to return a value
     vi.spyOn(require, 'resolve').mockReturnValue('some-community-framework');
     expect(() => validateFrameworkName('some-community-framework')).toThrow();
Or fix the test to properly mock resolveModulePath from exsolve if the intent is to test successful resolution.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between dbf7c18 and a5402e1.

📒 Files selected for processing (2)

code/core/src/common/utils/validate-config.test.ts (1 hunks)
code/core/src/common/utils/validate-config.ts (1 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

**/*.{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 (use yarn lint:js:cmd <file>)

Files:

code/core/src/common/utils/validate-config.ts
code/core/src/common/utils/validate-config.test.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/core/src/common/utils/validate-config.ts
code/core/src/common/utils/validate-config.test.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/core/src/common/utils/validate-config.ts
code/core/src/common/utils/validate-config.test.ts

{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

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/common/utils/validate-config.ts
code/core/src/common/utils/validate-config.test.ts

code/**/*.{test,spec}.{ts,tsx}

📄 CodeRabbit inference engine (.cursorrules)

code/**/*.{test,spec}.{ts,tsx}: Place all test files under the code/ directory
Name test files as *.test.ts, *.test.tsx, *.spec.ts, or *.spec.tsx

Files:

code/core/src/common/utils/validate-config.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/core/src/common/utils/validate-config.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 using vi.mock() (e.g., filesystem, loggers)

Files:

code/core/src/common/utils/validate-config.test.ts

🧬 Code graph analysis (1)

code/core/src/common/utils/validate-config.test.ts (1)

code/core/src/common/utils/validate-config.ts (1)

validateFrameworkName (15-43)

🔇 Additional comments (1)

code/core/src/common/utils/validate-config.ts (1)

36-36: LGTM! Template literal ensures cross-platform compatibility.

Using a template literal instead of path.join() guarantees forward slashes on all platforms, which correctly resolves the Windows issue where backslashes in scoped package names (e.g., @analogjs\\storybook-angular\\preset) would fail validation in exsolve.

code/core/src/common/utils/validate-config.test.ts

storybook-pr-benchmarking · 2025-10-27T14:25:20Z

Package Benchmarks

^{Commit: 1aaa3a3, ran on 28 October 2025 at 08:13:32 UTC}

No significant changes detected, all good. 👏

coderabbitai

Actionable comments posted: 1

🧹 Nitpick comments (1)

code/core/src/common/utils/validate-config.test.ts (1)
36-52: Tests work correctly but would be clearer with vi.mocked() pattern.

While the global flag pattern functions correctly, using vi.mocked() to override mock behavior per-test (as shown in the refactor above) makes the test intent more explicit and keeps test setup isolated within each test case.

Based on coding guidelines.

With the refactored mock setup, these tests become:
  it('should not throw if framework is unknown (community) but can be resolved', () => {
    // No setup needed - beforeEach already mocks successful resolution
    expect(() => validateFrameworkName('some-community-framework')).not.toThrow();
  });

  it('should not throw if scoped framework is unknown (community) but can be resolved', () => {
    // No setup needed - beforeEach already mocks successful resolution
    expect(() => validateFrameworkName('@some-community/framework')).not.toThrow();
  });

  it('should throw if framework is unknown and cannot be resolved', () => {
    vi.mocked(resolveModulePath).mockImplementation(() => {
      throw new Error('Cannot resolve');
    });
    expect(() => validateFrameworkName('foo')).toThrow();
  });

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a5402e1 and 26438fc.

📒 Files selected for processing (1)

code/core/src/common/utils/validate-config.test.ts (2 hunks)

🧰 Additional context used

📓 Path-based instructions (7)

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/core/src/common/utils/validate-config.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/core/src/common/utils/validate-config.test.ts

**/*.{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 (use yarn lint:js:cmd <file>)

Files:

code/core/src/common/utils/validate-config.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 using vi.mock() (e.g., filesystem, loggers)

Files:

code/core/src/common/utils/validate-config.test.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/core/src/common/utils/validate-config.test.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/core/src/common/utils/validate-config.test.ts

{code/**,scripts/**}/**/*.{ts,tsx,js,jsx,mjs}

📄 CodeRabbit inference engine (.github/copilot-instructions.md)

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/common/utils/validate-config.test.ts

🧠 Learnings (1)

📚 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} : Avoid direct function mocking without vi.mocked()

Applied to files:

code/core/src/common/utils/validate-config.test.ts

🧬 Code graph analysis (1)

code/core/src/common/utils/validate-config.test.ts (1)

code/core/src/common/utils/validate-config.ts (1)

validateFrameworkName (15-43)

⏰ 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). (1)

GitHub Check: normal

code/core/src/common/utils/validate-config.test.ts

nx-cloud · 2025-10-28T08:10:00Z

View your CI Pipeline Execution ↗ for commit 1aaa3a3

Command	Status	Duration	Result
`nx run-many -t build --parallel=3`	✅ Succeeded	44s	View ↗

☁️ Nx Cloud last updated this comment at 2025-10-28 08:20:44 UTC

fix(core): remove usage of node:path to join framework preset path

a5402e1

coderabbitai bot reviewed Oct 25, 2025

View reviewed changes

code/core/src/common/utils/validate-config.test.ts Outdated Show resolved Hide resolved

valentinpalkovic assigned ndelangen Oct 27, 2025

valentinpalkovic added bug core windows ci:normal labels Oct 27, 2025

coderabbitai bot reviewed Oct 27, 2025

View reviewed changes

code/core/src/common/utils/validate-config.test.ts Outdated Show resolved Hide resolved

chore: fix unit tests

c144ed0

brandonroberts force-pushed the fix-core-resolve-scoped-framework branch from 26438fc to c144ed0 Compare October 27, 2025 17:59

ndelangen approved these changes Oct 28, 2025

View reviewed changes

Merge branch 'next' into fix-core-resolve-scoped-framework

1aaa3a3

ndelangen changed the title ~~fix(core): remove usage of node:path to join framework preset path~~ Core: Join framework preset path with slash Oct 28, 2025

ndelangen merged commit df41f2e into storybookjs:next Oct 28, 2025
54 of 55 checks passed

github-actions bot mentioned this pull request Oct 28, 2025

Release: Prerelease 10.0.0-rc.4 #32861

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

Uh oh!

Core: Join framework preset path with slash #32838

Core: Join framework preset path with slash #32838

brandonroberts commented Oct 25, 2025 •

edited by coderabbitai bot

Loading

Uh oh!

coderabbitai bot commented Oct 25, 2025 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

storybook-pr-benchmarking bot commented Oct 27, 2025 •

edited

Loading

Uh oh!

coderabbitai bot left a comment

Uh oh!

Uh oh!

nx-cloud bot commented Oct 28, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Uh oh!

Core: Join framework preset path with slash #32838

Core: Join framework preset path with slash #32838

Conversation

brandonroberts commented Oct 25, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What I did

Checklist for Contributors

Testing

The changes in this PR are covered in the following automated tests:

Manual testing

Documentation

Checklist for Maintainers

🦋 Canary release

Summary by CodeRabbit

Uh oh!

coderabbitai bot commented Oct 25, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Estimated code review effort

Possibly related PRs

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

storybook-pr-benchmarking bot commented Oct 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Package Benchmarks

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

nx-cloud bot commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

brandonroberts commented Oct 25, 2025 •

edited by coderabbitai bot

Loading

coderabbitai bot commented Oct 25, 2025 •

edited

Loading

storybook-pr-benchmarking bot commented Oct 27, 2025 •

edited

Loading

nx-cloud bot commented Oct 28, 2025 •

edited

Loading