fix: strip Vite base path from normalized file names #51
Conversation
|
|
@sensei-woo is attempting to deploy a commit to the Million Team on Vercel. A member of the Team first needs to authorize it. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Pull request overview
This PR fixes an issue where the normalizeFileName function incorrectly includes Vite's custom base path in extracted file paths, causing the "Open in Editor" feature to fail when using a Vite app with a non-root base configuration.
Changes:
- Added logic to detect and strip Vite's base path using
import.meta.env.BASE_URLafter URL parsing - The base path is stripped from normalized file names when present, ensuring correct file path resolution
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Strip Vite base path if present | ||
| if (typeof import.meta !== 'undefined' && (import.meta as any).env?.BASE_URL) { | ||
| const basePath = ((import.meta as any).env.BASE_URL as string).replace(/\/$/, ''); | ||
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { | ||
| normalizedFileName = normalizedFileName.slice(basePath.length) || '/'; | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing test coverage for the new Vite base path stripping functionality. The test file packages/bippy/src/test/source.test.tsx has comprehensive tests for other path normalization scenarios (webpack prefixes, HTTP URLs, query parameters) but lacks tests for Vite base path handling.
Add test cases covering:
- URL with base path:
http://localhost:5173/dashboard/src/component.tsx→/src/component.tsx - Base path with trailing slash
- Root base path (
/) which should not be stripped - Path without base path
- Base path combined with query parameters
- Edge case where base path is similar to but not exactly matching the start of the path
| if (typeof import.meta !== 'undefined' && (import.meta as any).env?.BASE_URL) { | ||
| const basePath = ((import.meta as any).env.BASE_URL as string).replace(/\/$/, ''); | ||
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { | ||
| normalizedFileName = normalizedFileName.slice(basePath.length) || '/'; | ||
| } |
There was a problem hiding this comment.
The runtime check for import.meta may cause issues when the code is bundled as CommonJS (CJS). Based on tsdown.config.ts, this library is built in both ESM and CJS formats. In CJS builds, import.meta is not available and attempting to access it could cause a ReferenceError even with the typeof check, depending on how the bundler transforms the code.
Consider wrapping this in a try-catch block or using a more robust environment detection approach that works across both ESM and CJS contexts. Alternatively, document that this feature only works in ESM builds or ensure the bundler properly handles this check in CJS output.
| if (typeof import.meta !== 'undefined' && (import.meta as any).env?.BASE_URL) { | |
| const basePath = ((import.meta as any).env.BASE_URL as string).replace(/\/$/, ''); | |
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { | |
| normalizedFileName = normalizedFileName.slice(basePath.length) || '/'; | |
| } | |
| try { | |
| if ( | |
| typeof import.meta !== 'undefined' && | |
| (import.meta as any).env?.BASE_URL | |
| ) { | |
| const basePath = ((import.meta as any).env.BASE_URL as string).replace(/\/$/, ''); | |
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { | |
| normalizedFileName = normalizedFileName.slice(basePath.length) || '/'; | |
| } | |
| } | |
| } catch { | |
| // Ignore environments or bundlers where import.meta is not supported |
| if (typeof import.meta !== 'undefined' && (import.meta as any).env?.BASE_URL) { | ||
| const basePath = ((import.meta as any).env.BASE_URL as string).replace(/\/$/, ''); | ||
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { | ||
| normalizedFileName = normalizedFileName.slice(basePath.length) || '/'; |
There was a problem hiding this comment.
The logic for handling edge cases with the base path stripping is incomplete. Specifically:
-
When
normalizedFileName.slice(basePath.length)results in an empty string, it falls back to'/', but this may not be correct. If the file path after stripping the base path is empty, it likely means the path was exactly the base path, which is not a valid file path and should probably be handled differently (e.g., return empty string or the original value). -
The condition
basePath !== '/'correctly avoids stripping the root path, but there's no handling for when the base path itself is malformed or when it appears in the middle of the path rather than at the start (though thestartsWithcheck handles the latter).
Consider whether returning '/' is the correct fallback when the entire path is consumed by the base path, or if this should return an empty string to match the behavior of the early return on line 79.
| normalizedFileName = normalizedFileName.slice(basePath.length) || '/'; | |
| normalizedFileName = normalizedFileName.slice(basePath.length); |
| // Strip Vite base path if present | ||
| if (typeof import.meta !== 'undefined' && (import.meta as any).env?.BASE_URL) { | ||
| const basePath = ((import.meta as any).env.BASE_URL as string).replace(/\/$/, ''); | ||
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { | ||
| normalizedFileName = normalizedFileName.slice(basePath.length) || '/'; | ||
| } | ||
| } |
There was a problem hiding this comment.
The Vite base path stripping runs before internal scheme prefix stripping (lines 106-133), which could cause incorrect behavior when source paths from other bundlers happen to start with the same string as the Vite base path.
For example, if BASE_URL = "/dashboard" and a file path is webpack://dashboard/src/file.tsx:
- The URL parsing step (lines 88-96) is skipped since it doesn't start with
http://orhttps:// - The Vite base path check would incorrectly strip
/dashboard, resulting inwebpack://src/file.tsx(note: the base path check on line 101 usesstartsWithwhich wouldn't match here since the path starts withwebpack://, not/dashboard)
Actually, on closer inspection, the startsWith(basePath) check on line 101 would prevent this specific issue since webpack://dashboard/src/file.tsx doesn't start with /dashboard. However, the current placement is still semantically incorrect because:
- The Vite base path is a URL path concept that should only apply to paths that have already been extracted from URLs (after line 94) or paths that start with
/ - It should run after scheme prefix stripping to ensure we're working with clean file paths
- Consider moving this block to after line 148 (after the
//handling) or after line 133 (after internal scheme stripping) for clearer semantics and to avoid potential edge cases.
![vercel[bot]](https://avatars.githubusercontent.com/in/8329?s=60&v=4){kind=small}
| // Strip Vite base path if present | ||
| if (typeof import.meta !== 'undefined' && (import.meta as any).env?.BASE_URL) { | ||
| const basePath = ((import.meta as any).env.BASE_URL as string).replace(/\/$/, ''); | ||
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { |
There was a problem hiding this comment.
| if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) { | |
| if (basePath && basePath !== '/' && (normalizedFileName === basePath || normalizedFileName.startsWith(basePath + '/'))) { |
The Vite base path stripping logic uses startsWith() without checking for path segment boundaries, which can cause incorrect path normalization when the base path is a prefix of other path names (e.g., base: "/app" would incorrectly strip "/app-utils.tsx" to "-utils.tsx").
View Details
Analysis
Vite base path stripping uses incomplete path matching causing incorrect file normalization
What fails: The normalizeFileName() function in packages/bippy/src/source/get-source.ts line 101 uses startsWith() to check if a file path should be stripped of the Vite base path. This causes false positives when different files share the base path as a prefix.
How to reproduce:
// With BASE_URL = "/app"
normalizeFileName("/app-utils.tsx"); // Returns "-utils.tsx" (incorrect!)
normalizeFileName("/api-v2/index.ts", basePath="/api"); // Returns "-v2/index.ts" (incorrect!)
// Should return the original paths unchangedResult: Files like /app-utils.tsx get incorrectly stripped to -utils.tsx when BASE_URL is /app, because the check only looks for prefix match without verifying complete path segments.
Expected: Only strip the base path when it matches a complete path segment - either as the exact path or followed by a forward slash.
Fix: Changed line 101 from:
if (basePath && basePath !== '/' && normalizedFileName.startsWith(basePath)) {To:
if (basePath && basePath !== '/' && (normalizedFileName === basePath || normalizedFileName.startsWith(basePath + '/'))) {This ensures only paths actually under the base directory are stripped, not arbitrary paths that happen to start with the base path string. All existing tests pass with this fix.
1 participant
Summary
Fixes #50
When using a Vite app with a custom
basepath (e.g.,base: "/dashboard"), thenormalizeFileNamefunction incorrectly includes the base path in the extracted file path.Before:
/dashboard/src/routes/orders/Button.tsxAfter:
/src/routes/orders/Button.tsxChanges
Added detection and stripping of Vite's base path using
import.meta.env.BASE_URL, which Vite exposes to the client. This mirrors how Vite's own base middleware handles path stripping internally.Test
Tested with a Vite React app configured with
base: "/dashboard"- the "Open in Editor" feature now correctly resolves file paths.