fix: windows compatibility across packages#184
Merged
Conversation
- bundler: emit file:// URL specifiers for autoload imports so Windows paths don't trigger PARSE_ERROR when backslashes are interpreted as escape sequences in generated string literals - bundler: append .exe to compiled binary names for Windows targets so CompiledBinary.path matches the file bun produces on disk; simplify clean() which previously had to enumerate both variants - cli: normalize template paths to forward slashes in renderTemplate so the gitignore -> .gitignore rename and --no-example/--no-config filters in kidd init work on Windows (where path.relative returns native \) - utils: pass shell:true on Windows in proc.exec/proc.spawn so .cmd shims (npm/pnpm-installed CLIs like tsx) can be launched via cmd.exe - core (breaking): remove unused git submodule detection. Drops isInSubmodule, getParentRepoRoot, and the ProjectRoot type. findProjectRoot now returns string | null. The submodule code was inherited from a project skeleton, had no internal callers, and was silently disabled on Windows. Co-Authored-By: Claude <noreply@anthropic.com>
🦋 Changeset detectedLatest commit: 6f15480 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Merging this PR will not alter performance
Comparing 
Footnotes
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR standardizes generated and runtime ESM import specifiers to file:// URLs via new path utilities, normalizes template-relative paths to POSIX form, adds Windows .exe handling for compiled binaries and cleanup, enables shell execution for child processes on Windows, and simplifies findProjectRoot to return a string path (removing submodule/parent-repo APIs and types). Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
…tils/node/path Centralize the two cross-platform path helpers that previously lived as private one-liners in the bundler and the CLI: - `path.toImportUrl(p)` wraps `pathToFileURL(p).href` for converting filesystem paths into ESM `file://` import specifiers. - `path.toPosixPath(p)` normalizes native paths to forward slashes. Updated call sites: - packages/bundler/src/autoloader/generate-autoloader.ts (3 usages) - packages/core/src/autoload.ts (1 usage) - packages/cli/src/lib/render.ts (1 usage) Co-Authored-By: Claude <noreply@anthropic.com>
…at module load Capturing process.platform at module load freezes the value before any test stub or platform mock can apply. Convert NEEDS_SHELL constant into a needsShell() function so each spawn/exec call re-reads the value. Co-Authored-By: Claude <noreply@anthropic.com>
Replace `match(targets.length > 0)` with `match(targets).with([], ...)` and `match(isMultiTarget)` with `match(resolvedTargets.length).with(1, ...)` so ts-pattern matches on the underlying value rather than a boolean intermediary. Drops the now-redundant isMultiTarget local. Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bundler/src/utils/clean.ts`:
- Around line 98-101: Replace the imperative if-check that appends ".exe" with a
ts-pattern match expression: use match(target).with('windows-*', () =>
`${base}.exe`).otherwise(() => base) (or the equivalent pattern used elsewhere),
ensuring you reference the same target and base variables and keep consistent
ts-pattern usage as in the surrounding code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ea0588e4-d53c-4867-8d35-7525027c18ad
📒 Files selected for processing (1)
packages/bundler/src/utils/clean.ts
Sweep the codebase for the match(boolean) antipattern where ts-pattern
was being fed a derived comparison or a bare boolean variable. Replaced
with one of:
- match on the underlying value (e.g. match(parts.length > 1) becomes
inlined if/else; match(targets.length > 0) becomes match(targets).with([], ...))
- match on the parent object via property pattern
(e.g. match(params.compile) becomes match(params).with({ compile: true }, ...))
- straight if/else for simple boolean variables
Files touched (14): bundler/build/config, bundler/utils/clean,
bundler/utils/format-error, cli/commands/build, cli/lib/template-versions,
core/autoload, core/lib/format/code-frame, core/middleware/config/config,
core/middleware/icons/context, core/middleware/icons/install,
core/runtime/register, core/stories/check, core/ui/keys.
Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/stories/check.ts`:
- Around line 133-143: Replace the two-branch if returning an error or empty
array with a ts-pattern match: import { match } from 'ts-pattern' and use
match(params.editableFieldCount).with(value => value > MAX_EDITABLE_FIELDS, ()
=> [Object.freeze({ storyName: params.name, severity: 'error' as const, message:
`Too many editable fields: ${String(params.editableFieldCount)} (max
${String(MAX_EDITABLE_FIELDS)}). Move fields to \\`defaults\\` to reduce.`,
})]).otherwise(() => []) so the logic uses match() instead of the if; ensure you
reference params.editableFieldCount and MAX_EDITABLE_FIELDS and return the same
frozen object shape.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ef41fa41-68c8-4cc8-8ac7-63d35f862f16
📒 Files selected for processing (13)
packages/bundler/src/build/config.tspackages/bundler/src/utils/clean.tspackages/bundler/src/utils/format-error.tspackages/cli/src/commands/build.tspackages/cli/src/lib/template-versions.tspackages/core/src/autoload.tspackages/core/src/lib/format/code-frame.tspackages/core/src/middleware/config/config.tspackages/core/src/middleware/icons/context.tspackages/core/src/middleware/icons/install.tspackages/core/src/runtime/register.tspackages/core/src/stories/check.tspackages/core/src/ui/keys.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Resolves multiple Windows compatibility issues across
bundler,cli,utils, andcore. Some are concrete bugs (PARSE_ERROR onkidd build,kidd run --engine=tsxENOENT), others are paths-as-strings assumptions that worked on POSIX but failed silently on Windows. Also drops dead submodule-detection code that was already disabled on Windows via aprocess.platform === 'win32'guard.Changes
@kidd-cli/bundler(patch)importstatements now usefile://URL specifiers (pathToFileURL(p).href) instead of raw absolute paths. On Windows, paths likeC:\joggr\update.tswere being parsed as JavaScript string literals where\j,\u,\wtriggeredPARSE_ERROR: Invalid escape sequence. The runtime autoloader was already fixed in fix(core): use file:// URLs for command autoload imports #180; this extends the same fix to the static autoloader plugin.resolveBinaryName()now appends.exeforwindows-*targets soCompiledBinary.pathmatches the file bun actually produces on disk. Bun auto-appends.exefor Windows targets, so we were recording a path that didn't exist on disk.clean()previously enumerated bothcliandcli.exeto compensate; now it produces a single canonical name.@kidd-cli/cli(patch)renderTemplatenow normalizespath.relative()output to forward slashes viatoPosixPath. This fixes two latent bugs inkidd initon Windows:/(^|\/)gitignore($|\/)/gdidn't match\gitignoresogitignore.liquidwasn't renamed to.gitignorefile.relativePath.includes('commands/hello.ts')didn't matchcommands\hello.ts, so--no-exampleand--no-configflags silently no-op'd@kidd-cli/utils(patch)proc.execandproc.spawnnow passshell: trueon Windows. Without this, Node'sCreateProcesscannot launch.cmdshim files — which is how npm/pnpm install most CLI binaries (tsx,prettier, etc.). Affectskidd run --engine=tsxon Windows.nodeandbunare unaffected (they're real.exefiles).@kidd-cli/core(breaking, minor)isInSubmodule,getParentRepoRoot, and theProjectRoottype from the public API.findProjectRoot()now returnsstring | null(the path) instead ofProjectRoot | null.if (process.platform === 'win32') return null. Same pattern of "we punted on Windows here" — opted to delete rather than maintain.Tests
packages/bundler/src/compile/compile.test.tscovering.exenaming for single-target, multi-target, and mixed-target builds.packages/bundler/src/autoloader/generate-autoloader.test.tsto assertfile://URL specifiers.packages/core/src/lib/project/root.test.tsfor the simplified API.tests/helpers.ts(integration test runner) to append.exefor the Windows host so the path matches reality and stops relying on Windows'CreateProcessWextension auto-append.Investigation notes
The compile output
.exechange was the only finding from this round of audit that wasn't immediately obvious. I dispatched two adversarial reviewers — one argued NOT to fix (Windows masks the spawn case via auto-extension; recording "what user requested" is defensible), one argued to fix (clean.tsalready had a workaround enumerating both variants, which is evidence the contract leak was already biting). The smoking gun waspackages/bundler/src/utils/clean.ts:101-102which had to special-case Windows becausebinary.pathcouldn't be trusted. Verdict: real bug.Caveat
Even with these fixes, none of the Windows code paths have been executed on a Windows machine in this session. CI runs on
windows-latest(.github/workflows/ci-integration.yml) but builds the example artifacts on macOS and runs the pre-built CLIs directly —kidd run,kidd init, and the bundler's autoload generation are not exercised end-to-end on Windows. Recommend monitoring the Windows job on this PR closely.Test plan
pnpm check(typecheck + lint + format) — cleanpnpm test— 1075 core + 158 cli + 135 bundler + 101 utils tests passwindows-latestpassesmacos-latest,macos-15-intel,ubuntu-latestpass