fix(astro): normalize file URL paths on Windows#67
Conversation
|
@mhanelia is attempting to deploy a commit to the Cytonic Team on Vercel. A member of the Team first needs to authorize it. |
Greptile SummaryThis PR fixes Windows path handling in the Astro integration by replacing direct
Confidence Score: 4/5Safe to merge — the core fix is correct and the changed call sites all receive Astro's guaranteed file: URL objects. The toFileSystemPath helper correctly handles both URL objects (via fileURLToPath) and string pathnames (via the drive-letter regex). The three replaced .pathname accesses are the right fix for the Windows bug. The only gaps are that the new test does not exercise the build command paths or the astro:build:done hook, and the helper type signature does not guard against non-file: URLs — both are unlikely to matter given Astro internals but worth tightening. No files require special attention; both changed files are straightforward. The test file could benefit from additional coverage of the build-command paths. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["astro:config:setup\n(config.publicDir / config.outDir)"] --> B{toFileSystemPath}
C["astro:build:done\n(dir || astroConfig.outDir)"] --> B
B --> D{pathOrUrl instanceof URL?}
D -- Yes --> E["fileURLToPath(pathOrUrl)\n→ correct OS path"]
D -- No --> F["regex: strip leading /C: prefix\n→ correct OS path"]
E --> G["Use in fs.mkdirSync /\nresolveConfig / scanBuiltPages"]
F --> G
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
src/plugins/astro.test.ts:16-31
**Limited test coverage for the fix**
The test only exercises the `dev` command path (checking `mkdirSync`). The same `toFileSystemPath` helper is also called in the `build` branch of `astro:config:setup` (`toFileSystemPath(config.outDir)`) and in the `astro:build:done` hook (`toFileSystemPath(dir || astroConfig.outDir)`). A Windows `file://` URL passed as `dir` in the build hook — the original crash site described in the PR — is not covered by any test here, so a future refactor could re-introduce the regression without failing CI.
### Issue 2 of 2
src/plugins/astro.ts:100-106
**String-branch regex is silently skipped for non-`file:` URLs**
When `pathOrUrl` is a `URL` whose protocol is not `file:` (e.g. `http:` or `https:`), `fileURLToPath` will throw a `TypeError: The URL must be of scheme file`. Astro guarantees `file:` URLs for `publicDir`, `outDir`, and the build `dir` parameter, so this won't trigger today. However, since the helper accepts a generic `string | URL`, callers could inadvertently pass a non-`file:` URL and receive an unhandled exception rather than a graceful error. Narrowing the accepted type or adding a protocol guard would make the contract explicit.
Reviews (1): Last reviewed commit: "fix(astro): normalize file URL paths on ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| describe('aeoAstroIntegration', () => { | ||
| it('uses native filesystem paths for Astro file URLs in dev', () => { | ||
| fsMocks.existsSync.mockReturnValue(false); | ||
|
|
||
| const publicDir = new URL('file:///C:/mock/project/public'); | ||
| const outDir = new URL('file:///C:/mock/project/dist'); | ||
| const integration = aeoAstroIntegration({ widget: { enabled: false } }); | ||
|
|
||
| integration.hooks['astro:config:setup']({ | ||
| config: { publicDir, outDir }, | ||
| command: 'dev', | ||
| }); | ||
|
|
||
| expect(fsMocks.mkdirSync).toHaveBeenCalledWith(fileURLToPath(publicDir), { recursive: true }); | ||
| }); | ||
| }); |
There was a problem hiding this comment.
The test only exercises the dev command path (checking mkdirSync). The same toFileSystemPath helper is also called in the build branch of astro:config:setup (toFileSystemPath(config.outDir)) and in the astro:build:done hook (toFileSystemPath(dir || astroConfig.outDir)). A Windows file:// URL passed as dir in the build hook — the original crash site described in the PR — is not covered by any test here, so a future refactor could re-introduce the regression without failing CI.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/astro.test.ts
Line: 16-31
Comment:
**Limited test coverage for the fix**
The test only exercises the `dev` command path (checking `mkdirSync`). The same `toFileSystemPath` helper is also called in the `build` branch of `astro:config:setup` (`toFileSystemPath(config.outDir)`) and in the `astro:build:done` hook (`toFileSystemPath(dir || astroConfig.outDir)`). A Windows `file://` URL passed as `dir` in the build hook — the original crash site described in the PR — is not covered by any test here, so a future refactor could re-introduce the regression without failing CI.
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| function toFileSystemPath(pathOrUrl: string | URL): string { | ||
| if (pathOrUrl instanceof URL) { | ||
| return fileURLToPath(pathOrUrl); | ||
| } | ||
|
|
||
| return pathOrUrl.replace(/^\/([A-Za-z]:)(?=\/|\\)/, '$1'); | ||
| } |
There was a problem hiding this comment.
file: URLs
When pathOrUrl is a URL whose protocol is not file: (e.g. http: or https:), fileURLToPath will throw a TypeError: The URL must be of scheme file. Astro guarantees file: URLs for publicDir, outDir, and the build dir parameter, so this won't trigger today. However, since the helper accepts a generic string | URL, callers could inadvertently pass a non-file: URL and receive an unhandled exception rather than a graceful error. Narrowing the accepted type or adding a protocol guard would make the contract explicit.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/plugins/astro.ts
Line: 100-106
Comment:
**String-branch regex is silently skipped for non-`file:` URLs**
When `pathOrUrl` is a `URL` whose protocol is not `file:` (e.g. `http:` or `https:`), `fileURLToPath` will throw a `TypeError: The URL must be of scheme file`. Astro guarantees `file:` URLs for `publicDir`, `outDir`, and the build `dir` parameter, so this won't trigger today. However, since the helper accepts a generic `string | URL`, callers could inadvertently pass a non-`file:` URL and receive an unhandled exception rather than a graceful error. Narrowing the accepted type or adding a protocol guard would make the contract explicit.
How can I resolve this? If you propose a fix, please make it concise.1 participant
Summary
Fixes Windows path handling in the Astro integration when Astro provides
publicDirandoutDirasfile://URLs.What changed
Converted Astro
file://URLs to native filesystem paths using Node'sfileURLToPath().Updated dev
publicDircreation to use the normalized filesystem path.Updated build output scanning to normalize the Astro build output URL before reading generated HTML.
Added a regression test with mocked Windows
publicDirandoutDirfile URLs.Why
On Windows, using
URL.pathnamedirectly can produce paths like/C:/Users/....When passed to filesystem APIs, this can be interpreted incorrectly and result in duplicated drive paths like
C:\C:\Users\...\public.Using
fileURLToPath()preserves correct behavior across Windows, Linux, and macOS.Validation
npx.cmd tsc --noEmit✅npm.cmd run build✅npx.cmd vitest run src/plugins/astro.test.ts✅npx.cmd vitest runsrc/plugins/angular.test.ts(should scan Angular routes from source)