Core: Normalize file paths in ChangeDetectionService and trace-changed for Windows support#34445
Conversation
|
View your CI Pipeline Execution ↗ for commit a09cdf6
☁️ Nx Cloud last updated this comment at |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChanged path handling in change-detection: replaced absolute resolution with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
code/core/src/core-server/change-detection/trace-changed.ts (1)
5-21: Well-structured fallback lookup for cross-platform compatibility.The two-phase approach is sound: direct O(1) lookup first, then O(n) fallback only when needed. This handles module graphs where keys might have inconsistent path formats (e.g., Windows backslashes vs. forward slashes).
Consider adding a brief inline comment explaining when the fallback is expected to trigger (e.g., Windows paths in moduleGraph keys).
📝 Optional: Add explanatory comment
function getModuleNodesByNormalizedPath( moduleGraph: ModuleGraph, normalizedPath: string ): Set<ModuleNode> | undefined { const directMatch = moduleGraph.get(normalizedPath); if (directMatch) { return directMatch; } + // Fallback: moduleGraph keys may use OS-native separators (e.g., backslashes on Windows) for (const [modulePath, nodes] of moduleGraph.entries()) { if (normalizePath(modulePath) === normalizedPath) { return nodes; } } return undefined; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@code/core/src/core-server/change-detection/trace-changed.ts` around lines 5 - 21, Add a short inline comment in getModuleNodesByNormalizedPath explaining that after the O(1) direct lookup the O(n) fallback scans moduleGraph entries to handle mismatched path formats (e.g., Windows backslashes vs POSIX forward slashes) and will only run when keys in moduleGraph are not already normalized; place the comment above the fallback for clarity so future readers understand when and why the fallback path is expected to trigger.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@code/core/src/core-server/change-detection/trace-changed.ts`:
- Around line 5-21: Add a short inline comment in getModuleNodesByNormalizedPath
explaining that after the O(1) direct lookup the O(n) fallback scans moduleGraph
entries to handle mismatched path formats (e.g., Windows backslashes vs POSIX
forward slashes) and will only run when keys in moduleGraph are not already
normalized; place the comment above the fallback for clarity so future readers
understand when and why the fallback path is expected to trigger.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e02881a4-04ee-44dc-9bd3-3a481498b7a6
📒 Files selected for processing (2)
code/core/src/core-server/change-detection/ChangeDetectionService.tscode/core/src/core-server/change-detection/trace-changed.ts
What I did
Normalize paths in change detection and module tracing logic to support Windows.
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
Caution
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
Documentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli-storybook/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook publish.yml --field pr=<PR_NUMBER>Summary by CodeRabbit
Bug Fixes
Tests