Uh oh!
There was an error while loading. Please reload this page.
fix(app): preserve native path separators in file path helpers - #14912
Conversation
79ee9d3 to
8a4ef00Compare8a4ef00 to
e57fe7dCompareThere was a problem hiding this comment.
Pull request overview
This pull request fixes a UI regression introduced in PR #13659 where Windows file paths were displayed with forward slashes (/) instead of native backslashes (\) in the web UI. The fix preserves native path separators while maintaining compatibility with Cygwin's mixed-separator paths.
Changes:
- Modified path normalization to canonicalize paths only for comparison, then slice from the original path to preserve native separators
- Updated path helper functions to handle both forward and backward slashes when stripping prefixes
- Updated test expectations to verify that native separators are preserved in output paths
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| packages/app/src/context/file/path.ts | Modified normalize() to canonicalize paths only for prefix comparison while preserving original separators in the output |
| packages/app/src/context/file/path.test.ts | Updated test expectations to verify Windows paths preserve backslashes while Unix paths preserve forward slashes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| expect(path.normalize("C:/repo/src/app.ts")).toBe("src/app.ts") | ||
| expect(path.normalize("file://C:/repo/src/app.ts")).toBe("src/app.ts") | ||
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src/app.ts") | ||
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src\\app.ts") |
There was a problem hiding this comment.
The PR description mentions that Cygwin paths like "/c/Users/..." or "C:/Users/..." should match against native roots like "C:\Users...", but there are no test cases verifying this behavior. Consider adding test cases to ensure Cygwin path compatibility works as intended, for example testing paths starting with "/c/" or "/cygdrive/c/" against a Windows root.
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src\\app.ts") | |
| expect(path.normalize("c:\\repo\\src\\app.ts")).toBe("src\\app.ts") | |
| // Cygwin-style paths should also normalize against the Windows root | |
| expect(path.normalize("/c/repo/src/app.ts")).toBe("src/app.ts") | |
| expect(path.normalize("/cygdrive/c/repo/src/app.ts")).toBe("src/app.ts") |
| // Only case-insensitive on Windows (drive letter or UNC paths) | ||
| const windows = /^[A-Za-z]:/.test(root) || root.startsWith("\\\\") |
There was a problem hiding this comment.
The canonicalization always lowercases both the root and path, even on non-Windows systems. This could cause issues on case-sensitive filesystems (Linux/Mac) where "/repo/Src/app.ts" and "/repo/src/app.ts" are different files. The original code only applied case-insensitive matching on Windows (when root started with a drive letter). Consider adding back the Windows detection to only lowercase on Windows systems.
Uh oh!
There was an error while loading. Please reload this page.
Summary
The Cygwin fix (a74fedd) normalized all paths to
/in the frontend path helpers to fix change detection with mixed separators. This caused the web UI to show/instead of\on Windows — a UI regression.Fix: Canonicalize to
/only for the prefix-stripping comparison, but slice from the original path to preserve native separators./c/Users/...,C:/Users/...) still match against native roots (C:\Users\...)\in file paths (matching native behavior and the TUI)Reverts the UI regression from #13659 while keeping the Cygwin compatibility.