Skip to content

Conversation

@mrginglymus
Copy link
Contributor

@mrginglymus mrginglymus commented Nov 27, 2025

What I did

Fixed another windows path separator bug that was swallowing all type errors.

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
    • Fixed path separator handling in type diagnostics reporting for better cross-platform compatibility.
    • Improved type checking output to clearly indicate when no type errors are present.

✏️ Tip: You can customize this high-level summary in your review settings.

@nx-cloud
Copy link

nx-cloud bot commented Nov 27, 2025

View your CI Pipeline Execution ↗ for commit 0e21324

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

☁️ Nx Cloud last updated this comment at 2025-11-27 12:32:33 UTC

@valentinpalkovic valentinpalkovic self-assigned this Nov 27, 2025
@valentinpalkovic valentinpalkovic added ci:normal build Internal-facing build tooling & test updates labels Nov 27, 2025
@valentinpalkovic valentinpalkovic changed the title Fix type checking on windows Build: Fix type checking on windows Nov 27, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Nov 27, 2025

📝 Walkthrough

Walkthrough

This change normalizes file path separators in TypeScript diagnostics lookup to forward slashes, ensuring consistent path handling across platforms. Additionally, it adds diagnostic output feedback when no type errors are detected.

Changes

Cohort / File(s) Summary
Path normalization and diagnostics output
scripts/check/check-package.ts
Added node:path import; normalized diagnostic paths by replacing OS-specific separators with forward slashes via replaceAll(path.sep, '/'); added else branch to print "no type errors" feedback when diagnostics are empty

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~5 minutes

  • Path normalization is a straightforward fix for cross-platform consistency
  • Addition of else branch for diagnostic output is a minor logic change
  • Single-file modification with minimal scope

Possibly related PRs

  • storybookjs/storybook#32838: Addresses Windows backslash vs forward-slash path issues for module resolution, similar cross-platform path normalization concern
✨ Finishing touches
  • 📝 Generate docstrings

📜 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 b90e26e and 0e21324.

📒 Files selected for processing (1)
  • scripts/check/check-package.ts (1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx,js,jsx,mjs}

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

Use camelCase for variable and function names

Files:

  • scripts/check/check-package.ts
**/*.{ts,tsx}

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

**/*.{ts,tsx}: Enable TypeScript strict mode
Export functions from modules for testing purposes

Files:

  • scripts/check/check-package.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:

  • scripts/check/check-package.ts
🧠 Learnings (3)
📓 Common learnings
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.659Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode
📚 Learning: 2025-11-24T17:49:47.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.659Z
Learning: Run 'yarn task --task check' to verify TypeScript types across all packages before submitting PRs

Applied to files:

  • scripts/check/check-package.ts
📚 Learning: 2025-11-24T17:49:47.659Z
Learnt from: CR
Repo: storybookjs/storybook PR: 0
File: .github/copilot-instructions.md:0-0
Timestamp: 2025-11-24T17:49:47.659Z
Learning: Applies to **/*.{ts,tsx} : Enable TypeScript strict mode

Applied to files:

  • scripts/check/check-package.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
🔇 Additional comments (3)
scripts/check/check-package.ts (3)

1-1: LGTM!

The import of the path module is necessary for accessing path.sep to normalize path separators.


14-16: LGTM!

Adding feedback when no type errors are detected improves the script's user experience by confirming successful completion.


10-10: Cannot fully verify—repository clone failed; however, fix aligns with cross-platform best practices.

The normalization pattern process.cwd().replaceAll(path.sep, '/') is sound and follows industry best practices for TypeScript/Node.js path handling. Forward slash conversion ensures consistent module specifier handling across Windows and Unix systems.

However, I was unable to verify the original review comment's action items due to a repository clone failure:

  • Cannot confirm Windows testing
  • Cannot search for similar path separator issues elsewhere in the codebase
  • Cannot execute yarn task --task check

The fix itself appears correct in principle, but complete verification requires direct repository access.

Tip

📝 Customizable high-level summaries are now available in beta!

You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.

  • Provide your own instructions using the high_level_summary_instructions setting.
  • Format the summary however you like (bullet lists, tables, multi-section layouts, contributor stats, etc.).
  • Use high_level_summary_in_walkthrough to move the summary from the description to the walkthrough section.

Example instruction:

"Divide the high-level summary into five sections:

  1. 📝 Description — Summarize the main change in 50–60 words, explaining what was done.
  2. 📓 References — List relevant issues, discussions, documentation, or related PRs.
  3. 📦 Dependencies & Requirements — Mention any new/updated dependencies, environment variable changes, or configuration updates.
  4. 📊 Contributor Summary — Include a Markdown table showing contributions:
    | Contributor | Lines Added | Lines Removed | Files Changed |
  5. ✔️ Additional Notes — Add any extra reviewer context.
    Keep each section concise (under 200 words) and use bullet or numbered lists for clarity."

Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later.


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

@mrginglymus
Copy link
Contributor Author

I think this was probably fixed as a side effect of kaspergeddon

@mrginglymus mrginglymus deleted the fix-type-check-windows branch November 28, 2025 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

build Internal-facing build tooling & test updates ci:normal

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants