Skip to content

fix(generate-pdf): silent no-op on Windows - build file:// URLs with pathToFileURL#949

Open
shuhia wants to merge 1 commit into
santifer:mainfrom
shuhia:fix/948-windows-pathtofileurl
Open

fix(generate-pdf): silent no-op on Windows - build file:// URLs with pathToFileURL#949
shuhia wants to merge 1 commit into
santifer:mainfrom
shuhia:fix/948-windows-pathtofileurl

Conversation

@shuhia

@shuhia shuhia commented Jun 11, 2026

Copy link
Copy Markdown

Fixes #948

What

On Windows, node generate-pdf.mjs <input.html> <output.pdf> was a silent no-op: no output, no PDF, exit code 0. The main-module guard hand-built the comparison URL:

const isMain = process.argv[1] && import.meta.url === `file://${resolve(process.argv[1])}`;

On Windows resolve() returns backslash paths (C:\..., or \\wsl.localhost\... UNC when the repo lives in WSL), so the string never equals import.meta.url (file:///C:/... forward-slash form). isMain stayed false, the CLI entry never ran, and the script exited 0 after merely being imported.

This PR replaces the hand-built string with pathToFileURL() — the same pattern the repo already uses in scan.mjs, scan-ats-full.mjs, update-system.mjs, and followup-cadence.mjs. generate-pdf.mjs was the last script with the hand-built guard.

Per the issue, I grepped for all remaining `file://${...}` interpolations and fixed the other two in the same file, which produced invalid backslash URLs on Windows:

  • the fonts injection (url('file://${fontsDir}/) → broken font loading, wrong typography in the PDF
  • the Playwright baseURL (file://${baseDir}/) → broken relative resources

4 lines changed (3 sites + the pathToFileURL import). No behavior change on Linux/macOS: for POSIX absolute paths pathToFileURL().href produces the same file:///... string the template literal did (and additionally percent-encodes special characters correctly).

Verification (Windows 11, Node v24.14.0)

Before (clean clone of main @ 0d57994):

> node generate-pdf.mjs batch\does-not-exist.html out.pdf
> echo $LASTEXITCODE
0          # no output at all, no PDF

After:

> node generate-pdf.mjs batch\does-not-exist.html out.pdf
📄 Input:  ...\batch\does-not-exist.html
📁 Output: ...\out.pdf
📏 Format: A4
❌ PDF generation failed: ENOENT: no such file or directory, open '...'
> echo $LASTEXITCODE
1

End-to-end render on Windows with templates/cv-template.html (exercises the fonts rewrite + baseURL):

✅ PDF generated: ...\output\test-windows.pdf
📊 Pages: 1
📦 Size: 33.3 KB

Test suite (node test-all.mjs): 240 passed, 3 failed — and the identical 3 failures occur on unmodified main on this machine, all Windows-environment issues unrelated to this change (Go toolchain missing for the dashboard build, .claude/.../SKILL.md symlink checked out as a plain file, spawnSync chmod ENOENT). Linux CI should be fully green.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Improved PDF generation reliability and cross-platform compatibility through enhanced file URL handling for fonts and resources.

The main-module guard compared import.meta.url against a hand-built
`file://${resolve(process.argv[1])}` string. On Windows, resolve()
returns backslash paths (C:\... or \\wsl.localhost\... UNC), so the
comparison never matched: the CLI entry point never ran and the script
exited 0 silently without generating anything.

Use pathToFileURL() like the other CLI scripts already do (scan.mjs,
scan-ats-full.mjs, update-system.mjs, followup-cadence.mjs), and fix
the two other hand-built file:// URLs in the same file (fonts
injection and the Playwright baseURL), which produced invalid
backslash URLs on Windows.

Fixes santifer#948

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 302a780f-1fd4-444c-88eb-31518c92b76c

📥 Commits

Reviewing files that changed from the base of the PR and between 0d57994 and 14e65cc.

📒 Files selected for processing (1)
  • generate-pdf.mjs

📝 Walkthrough

Walkthrough

The PR fixes URL handling in generate-pdf.mjs by replacing three instances of hand-built file:// URL strings with standard pathToFileURL calls. Windows paths with backslashes were causing the main-module guard to fail silently, broken font loading, and invalid baseURL format. The fix imports pathToFileURL and applies it consistently across font injection, Playwright configuration, and CLI detection.

Changes

Cross-platform URL path handling

Layer / File(s) Summary
Replace hand-built file:// URLs with pathToFileURL
generate-pdf.mjs
Import pathToFileURL from node:url and replace three instances of string-interpolated file://${...} URLs: font path rewriting now uses pathToFileURL(fontsDir).href, Playwright baseURL uses pathToFileURL(baseDir).href, and main-module detection compares import.meta.url against pathToFileURL(resolve(process.argv[1])).href. Fixes Windows silent no-op where backslash paths never matched, and broken fonts and resource paths.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related PRs

  • santifer/career-ops#929: Modifies the same page.setContent Playwright options in generate-pdf.mjs; one updates baseURL URL formatting via pathToFileURL, the other switches waitUntil strategy.

Suggested labels

🔧 scripts

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title accurately describes the main change: fixing a Windows-specific bug by replacing hand-built file:// URL construction with pathToFileURL().
Linked Issues check ✅ Passed The code changes fully address all objectives from #948: replacing hand-built file:// URLs with pathToFileURL() in the main-module guard, fonts injection, and baseURL, ensuring the CLI runs on Windows.
Out of Scope Changes check ✅ Passed All changes are directly scoped to fixing the Windows file:// URL bug; no unrelated modifications are present.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

generate-pdf.mjs is a silent no-op on Windows (main-module guard never matches)

1 participant