fix(scripts): asset generation cannot spawn vite on Windows - #130
fix(scripts): asset generation cannot spawn vite on Windows#130MohammedAlkindi wants to merge 1 commit into
Conversation
generate-assets.mjs builds the vite argv with URL.pathname, which on Windows yields a leading-slash path such as /C:/repo/node_modules/vite/ bin/vite.js. Node resolves that against the drive root, so the spawn fails with Cannot find module 'C:\C:\repo\node_modules\vite\bin\vite.js' and pnpm check cannot get past the asset build. Use fileURLToPath, which is the documented file: URL to path conversion and is a no-op difference on POSIX. scripts/render-brand.mjs has the same URL.pathname pattern, but it is referenced by no package script or workflow and needs rsvg, so it is left alone here rather than changed without a way to exercise it.
|
🦞👀 Pull request received. I will update this pull request when review starts. ClawSweeper review completeClawSweeper finished reviewing this revision. The review result is being finalized. |
|
Codex review: needs maintainer review before merge. Reviewed September 8, 2026, 3:30 AM ET / 07:30 UTC. ClawSweeper reviewWhat this changesConvert Vite’s executable and configuration file URLs into native filesystem paths so asset generation works on Windows. Merge readiness✅ Ready for maintainer review This PR remains necessary: current main and v0.3.1 retain the Windows path defect. The focused patch is correct, and the contributor describes successful asset generation on Windows after applying it. Priority: P2 Review scores
Verification
How this fits togetherCrabfleet’s asset generator invokes Vite to bundle the browser application, then packages application and documentation assets for the Worker. Build, check, and several tests enter through this generator. flowchart LR
A[Build or check command] --> B[Asset generator]
C[Vite and config file URLs] --> D[Native filesystem paths]
D --> B
B --> E[Vite browser bundle]
E --> F[Generated Worker assets]
Before mergeNone. Agent review detailsSecurityNone. Review metricsNone. Technical reviewBest possible solution: Use Node’s native file-URL conversion at both subprocess argument boundaries, keeping unrelated checkout line-ending behavior outside this fix. Do we have a high-confidence way to reproduce the issue? Yes: running the current generator from a Windows drive checkout passes malformed drive paths to Node, matching the supplied failure trace and Node’s documented behavior; this review did not execute the build. Is this the best way to solve the issue? Yes: converting both file URLs with the standard Node API is the narrow repair and also handles encoded path characters correctly. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning medium; reviewed against bdd5083b0d3d. LabelsLabel changes:
Label justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
|
What Problem This Solves
Resolves a problem where
pnpm checkcannot get past asset generation on Windows.generate-assets.mjsbuilds the vite argv withURL.pathname, which yields a leading-slash path suchas
/C:/repo/node_modules/vite/bin/vite.js. Node resolves that against the drive root, so the spawnfails before any asset is written.
Why This Change Was Made
fileURLToPathis the documentedfile:URL to path conversion and returns the same string on POSIX,so this is a no-op for Linux and macOS contributors and unblocks Windows ones.
User Impact
A Windows contributor can run
pnpm checkthrough the asset build instead of failing at the first step.Evidence
On this branch with the change reverted:
With it applied,
node scripts/generate-assets.mjsexits cleanly andpnpm checkruns through the vitebuild to
oxfmt.Could not verify:
pnpm checkstill does not finish here, for an unrelated reason.oxfmt --checkreports format issues in 427 files on a stock Windows clone: the repo has no
.gitattributes, socore.autocrlf=truechecks out CRLF while oxfmt expects LF.scripts/generate-assets.mjsis not amongthose 427. That is a checkout-wide question rather than something this PR should decide, so I have left
it alone.
scripts/render-brand.mjs:16has the sameURL.pathnamepattern. It is referenced by no package scriptor workflow and needs
rsvg, so I left it rather than change code I cannot exercise.