Fix Windows path normalization in target plans#443
Conversation
|
Opened the matching bug report as #444 for the ugig bug-fix trail. This PR fixes that Windows path normalization issue. |
Greptile SummaryThis PR fixes cross-platform path separator bugs exposed when running the target adapter test suite on Windows. Two distinct strategies are applied: file listing paths in
Confidence Score: 5/5Safe to merge — changes are well-scoped to path normalization helpers with no side effects on real build/ship logic. The two fix strategies (sep-split normalization for file lists; isWindowsPath-gated posix.join for path builders) are both correct. All previously-flagged native-join call sites from earlier review rounds have been addressed. The remaining native-join calls in browser-safari's real build path are macOS-only filesystem operations where native separators are appropriate. No logic changes outside of path construction. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Path joining needed] --> B{Which file?}
B -->|tv-roku / web-static| C[Use native join for I/O traversal]
C --> D["relative(root, path)"]
D --> E[".split(sep).join('/')"]
E --> F[POSIX-normalized string in manifest]
B -->|pkg-jsr / plugin-vscode / browser-safari| G["isWindowsPath(base)?"]
G -->|"includes('\\') OR /^[A-Za-z]:\\//"| H[Windows path detected]
G -->|neither condition| I[POSIX path detected]
H --> J["native join / resolve"]
I --> K["posix.join / posix.resolve"]
J --> L[Correct Windows path]
K --> M[POSIX path preserved]
Reviews (4): Last reviewed commit: "Address path normalization review" | Re-trigger Greptile |
|
Fixed the review items in a54d0ea: �rowser-safari now uses the path-preserving helper for the dry-run plan path, plugin-vscode uses it for the dry-run plan file path, and the Windows-path heuristic no longer treats // POSIX paths as Windows. Re-ran the five targeted test files (30 passed) and typechecks for the touched target packages. |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
a54d0ea to
3935725
Compare
|
Rebased this branch onto current upstream master after the auto-rebase bot could not push to the fork. New head is 3935725.\n\nVerification after rebase:\n- npx --yes pnpm@9.12.0 exec vitest run packages/targets/tv-roku/src/index.test.ts packages/targets/web-static/src/index.test.ts packages/targets/pkg-jsr/src/index.test.ts packages/targets/plugin-vscode/src/index.test.ts packages/targets/browser-safari/src/index.test.ts -> 30 passed\n- typecheck passed for @profullstack/sh1pt-target-tv-roku, @profullstack/sh1pt-target-web-static, @profullstack/sh1pt-target-pkg-jsr, @profullstack/sh1pt-target-plugin-vscode, and @profullstack/sh1pt-target-browser-safari |
|
🤖 Auto-rebase: The branch was rebased successfully locally but could not be pushed to the fork. Please enable 'Allow edits from maintainers' in the PR settings, or rebase manually: |
3935725 to
5a346f7
Compare
|
Rebased again onto current upstream master after the latest auto-rebase bot comment. New head is 5a346f7.\n\nVerification after rebase:\n- npx --yes pnpm@9.12.0 exec vitest run packages/targets/tv-roku/src/index.test.ts packages/targets/web-static/src/index.test.ts packages/targets/pkg-jsr/src/index.test.ts packages/targets/plugin-vscode/src/index.test.ts packages/targets/browser-safari/src/index.test.ts -> 30 passed\n- typecheck passed for @profullstack/sh1pt-target-tv-roku, @profullstack/sh1pt-target-web-static, @profullstack/sh1pt-target-pkg-jsr, @profullstack/sh1pt-target-plugin-vscode, and @profullstack/sh1pt-target-browser-safari |
Summary
/separators on Windows for Roku and static web outputs/repo/...to\repo\...on WindowsWhy
Running the target adapter tests on Windows exposed cross-platform path handling bugs: generated manifests and mocked command cwd values contained backslashes even where the package contract/tests expect portable POSIX-style paths.
Validation
pnpm exec vitest run packages/targets/tv-roku/src/index.test.ts packages/targets/web-static/src/index.test.ts packages/targets/pkg-jsr/src/index.test.ts packages/targets/plugin-vscode/src/index.test.ts packages/targets/browser-safari/src/index.test.ts-> 30 passedpnpm --filter @profullstack/sh1pt-target-tv-roku typecheckpnpm --filter @profullstack/sh1pt-target-web-static typecheckpnpm --filter @profullstack/sh1pt-target-pkg-jsr typecheckpnpm --filter @profullstack/sh1pt-target-plugin-vscode typecheckpnpm --filter @profullstack/sh1pt-target-browser-safari typecheckFor the ugig bug-fix bounty: this PR fixes Windows-only path separator failures found while running the test suite locally.