fix(lint): resolve snippet typechecking failures on Windows due to mi…#147
Conversation
…xed path separators
|
Warning Review limit reached
Next review available in: 59 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe snippet typechecker normalizes Windows-style paths for virtual file storage, compiler-host lookups, source retrieval, and diagnostic mapping. Tests cover the exported helper and Windows path-separator regression behavior. ChangesVirtual path normalization
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — the fix is correct and mergeable; one coverage gap and one naming nit worth a look.
Reviewed changes — normalizes virtual-file paths to POSIX separators so snippet:types typechecking actually runs on Windows instead of silently passing.
- Normalize compiler-host path keys —
virtualFilesstore/lookup and thehost.fileExists/host.readFile/host.directoryExists/host.getSourceFile/ diagnostics-loop lookups are now wrapped innormalizeDocsPath, so the backslash-containing keys produced byresolve(projectRoot)on Windows match the forward-slashed paths TypeScript's compiler host queries with.
The fix is sound: virtualDirs is derived from the now-normalized virtualFiles keys so it stays consistent, createProgram root names come from those keys, and on POSIX normalizeDocsPath is an identity transform so there's no behavior change. I found no store/lookup site that was missed.
ℹ️ No regression test exercises the Windows path-separator scenario
The bug only reproduces when resolve(projectRoot) yields backslashes, i.e. on Windows. The existing suite builds projects with mkdtemp, which returns OS-native (forward-slashed) paths on CI, so the mismatch never occurs there and a future regression on this code path would pass unnoticed.
Technical details
# No regression test for the Windows path-separator fix
## Affected sites
- `packages/leadtype/src/lint/snippet-typecheck.test.ts` — all cases use `mkdtemp`; none force a backslash-containing `projectRoot`.
- `packages/leadtype/src/lint/snippet-typecheck.ts:231` — `virtualDir` interpolates `resolve(projectRoot)`; the separator mix is the whole bug.
## Required outcome
- A test that fails on the pre-fix code and passes on the post-fix code, without depending on the host OS being Windows.
## Suggested approach (optional)
- Directly unit-test the normalization contract: assert `typecheckSnippets` still reports a snippet error when virtual keys would be stored backslashed — e.g. by exercising `normalizeDocsPath` round-tripping, or by refactoring the `virtualDir` construction to accept an injectable path-join so a `\\`-style root can be simulated on POSIX.
- If neither is worth the churn, a one-line comment near line 231 noting *why* normalization is required would at least protect the invariant from a well-meaning future simplification.
## Open questions for the human
- Is a POSIX-runnable regression test worth the refactor here, or is documenting the invariant sufficient given Windows isn't in CI?ℹ️ Nitpicks
normalizeDocsPath(from../internal/docs-url) is a docs-URL helper that happens to be a generic\\→/replace. Reusing it for compiler-host paths is functionally correct but semantically off; a neutrally-namedtoPosixPath/normalizeSeparatorshelper would read truer to intent. Not blocking.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in the fix itself — the refactor is correct and mergeable. One minor suggestion inline about the added test.
Reviewed changes — the delta since the prior pullfrog review (59986ae): a refactor from the reused normalizeDocsPath helper to a purpose-named local one, plus a unit test.
- Introduced local
toPosixPathhelper —snippet-typecheck.tsnow defines and exportstoPosixPath(filePath.replace(/\\/g, "/")) and uses it at everyvirtualFiles/virtualDirsstore and lookup site, replacing thenormalizeDocsPathimport. This cleanly resolves the prior naming nit — the compiler-host path normalization no longer borrows a docs-URL helper. - Added a
toPosixPathunit test —snippet-typecheck.test.tsasserts backslash paths convert to forward slashes.
The refactor is behavior-preserving: toPosixPath is an equivalent generic \\→/ replace, all store/lookup keys stay consistent, and on POSIX it remains an identity transform.
Claude Opus ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — the delta since the prior pullfrog review (e1062ea): a POSIX-runnable regression test that closes the last open coverage gap.
- Added Windows path-separator integration regression test —
snippet-typecheck.test.tsnow drivestypecheckSnippetsend-to-end throughlintWithTypecheckwith a backslash-containingprojectRoot(${projectRoot}${path.sep}subdir\subsubdir) and asserts asnippet:typeserror is reported. On POSIX the injected backslash makes the stored virtual-file keys mismatch TypeScript's forward-slashed host queries pre-fix (the diagnostic is treated as a real-project file and skipped, so the test fails), while post-fixtoPosixPathnormalizes both store and lookup so the error surfaces. This is precisely the "fails on pre-fix, passes on post-fix without requiring Windows" outcome the prior review requested.
The test is self-contained: the snippet (const x: number = "this is a string";) has no imports, so the fabricated non-existent projectRoot doesn't perturb tsconfig or module resolution — the assignment is a type error regardless of config. Prior naming and coverage concerns are both now resolved; the fix is mergeable.
Claude Opus ｜ 𝕏
2 participants
closes:#145
Description
On Windows, the
snippet:typeslint checks were silently passing even when snippets contained syntax or compilation errors. This was caused by a path separator mismatch between the virtual file cache key storage and the TypeScript compiler host lookups.This PR resolves the issue by normalizing all paths to POSIX-style (forward slashes
/) when storing and querying the virtual files in the compiler host callbacks.Root Cause
snippet-typecheck.ts,virtualDirwas resolved usingprojectRoot(using Windows backslashes\), and the file name was joined using/. This stored paths likeC:\path\to\project/.leadtype-snippet-0/main.tsin thevirtualFilesMap.C:/path/to/project/.leadtype-snippet-0/main.ts).host.fileExists,host.getSourceFile) queried files using the forward-slashed version, causingvirtualFiles.get(fileName)to returnundefinedand skip typechecking.Changes
normalizeDocsPathfrom../internal/docs-url.host.fileExists,host.readFile,host.directoryExists,host.getSourceFile, and the diagnostics loop) withnormalizeDocsPath().snippet-typecheck.tswith Biome/Ultracite.Verification
Ran the vitest suite for snippet typechecking on Windows:
bun test snippet-typecheck.test.ts