Fix Windows local browse paths - #212
Open
Nul-led wants to merge 2 commits into
Open
Conversation
Contributor
📝 WalkthroughWalkthroughWindows local browse handling now normalizes Windows drive-letter paths during decoding and route generation. Unit tests and a documented end-to-end test cover file links, directory navigation, MP4 range responses, Unix paths, and UNC paths. ChangesWindows browse path handling
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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 |
Author
|
/review |
Code Review by Qodo
1. Missing TestChat hrefOk checks
|
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+81
to
+86
| // Browse URLs keep an absolute-path slash after the route prefix. On Windows, | ||
| // that turns `C:/path` into `/C:/path`, which Node resolves as `C:\C:\path`. | ||
| // Remove only that synthetic slash; Unix and UNC absolute paths stay intact. | ||
| if (platform === 'win32' && /^\/[A-Za-z]:[\\/]/u.test(decoded)) { | ||
| return decoded.slice(1) | ||
| } |
There was a problem hiding this comment.
1. Windows browse hrefs broken 🐞 Bug ≡ Correctness
On Windows, decodeBrowsePath now returns drive paths as C:/... (no leading /), but toBrowseHref/toEditHref concatenate paths directly after /codex-local-browse//codex-local-edit without inserting a separator slash. This can produce URLs like /codex-local-browseC:%5CUsers%5Cfile that won’t match the server routes (expecting /codex-local-browse/*path), breaking directory navigation/back/edit links on Windows.
Agent Prompt
## Issue description
`decodeBrowsePath()` now correctly normalizes Windows drive-letter paths from `/C:/...` to `C:/...`, but URL generation (`toBrowseHref` / `toEditHref`) assumes the path already begins with `/` and concatenates it directly after the route prefix. On Windows this yields malformed URLs (missing the route separator `/`) that don’t match `/codex-local-browse/*path` and `/codex-local-edit/*path`.
## Issue Context
- Server routes require `/codex-local-browse/<path>` and `/codex-local-edit/<path>`.
- After this PR, browse/edit link generation must not rely on the filesystem path having a leading `/`.
## Fix Focus Areas
- Update `toBrowseHref` and `toEditHref` to always insert exactly one `/` after the route prefix, regardless of whether `pathValue` starts with `/`.
- Normalize Windows separators for URL paths (e.g., replace `\\` with `/`) before encoding, so generated hrefs are stable and readable.
- Add a small unit test by factoring URL building into an exported helper (or otherwise testing via existing exported functions) to cover Windows drive paths.
### Code locations
- src/server/localBrowseUi.ts[144-154]
- src/server/localBrowseUi.ts[253-269]
- src/server/httpServer.ts[152-178]
- src/server/localBrowseUi.test.ts[1-19]
## Suggested implementation sketch
- In `toBrowseHref` / `toEditHref`:
- `const urlPath = pathValue.replace(/\\/gu, '/')`
- `const encoded = encodeURI(urlPath)`
- `const suffix = encoded.startsWith('/') ? encoded.slice(1) : encoded`
- `return `/codex-local-browse/${suffix}${query}`` (and similarly for edit)
- Add tests that assert a Windows input like `C:\\Users\\Me\\file.txt` (and `C:/Users/Me/file.txt`) yields `/codex-local-browse/C:/Users/Me/file.txt` (or equivalent encoding) and matches the route prefix with `/codex-local-browse/`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Nul-led
marked this pull request as ready for review
July 21, 2026 07:42
PR Summary by QodoFix Windows local browse decoding for drive-letter paths
AI Description
Diagram
High-Level Assessment
Files changed (4)
|
|
Code review by qodo was updated up to the latest commit ef41deb |
This was referenced Jul 26, 2026
Author
|
/review |
Comment on lines
+8
to
+15
| ## Steps | ||
|
|
||
| 1. Send a message containing Markdown links to `C:/path/to/file.ps1` and `C:/path/to/video.mp4`. | ||
| 2. Inspect both rendered links and confirm their `href`, title, and visible text preserve the complete drive-letter paths. | ||
| 3. Open the text-file link and confirm the request returns the existing file instead of a 404 response. | ||
| 4. Open the MP4 link and seek within the video. | ||
| 5. Inspect the MP4 request and confirm byte-range requests return `206 Partial Content` with `Accept-Ranges: bytes`. | ||
| 6. Open a directory through local browse and click a nested folder plus the parent (`..`) link. |
There was a problem hiding this comment.
1. Missing testchat hrefok checks 📘 Rule violation ☼ Reliability
This PR changes file-link/local-browse behavior, but the new manual test entry does not include the mandatory TestChat validation outputs (hrefOk, titleOk, textOk) nor the required saved screenshot under output/playwright/testchat-<feature>-cjs.png. Without those standardized checks and evidence, regressions in chat parsing/link rendering may go undetected.
Agent Prompt
## Issue description
The manual verification entry for Windows absolute file links does not include the mandatory TestChat validation outputs required for link rendering changes: a unique marker, explicit `hrefOk`/`titleOk`/`textOk` assertions, and a screenshot saved under `output/playwright/testchat-<feature>-cjs.png`.
## Issue Context
This PR modifies browse/edit link generation and browse-path decoding, which is within the scope of chat parsing/link rendering changes that require standardized TestChat validation evidence.
## Fix Focus Areas
- tests/chat-composer-rendering/windows-absolute-file-links-open-local-browse.md[8-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+8
to
+23
| ## Steps | ||
|
|
||
| 1. Send a message containing Markdown links to `C:/path/to/file.ps1` and `C:/path/to/video.mp4`. | ||
| 2. Inspect both rendered links and confirm their `href`, title, and visible text preserve the complete drive-letter paths. | ||
| 3. Open the text-file link and confirm the request returns the existing file instead of a 404 response. | ||
| 4. Open the MP4 link and seek within the video. | ||
| 5. Inspect the MP4 request and confirm byte-range requests return `206 Partial Content` with `Accept-Ranges: bytes`. | ||
| 6. Open a directory through local browse and click a nested folder plus the parent (`..`) link. | ||
|
|
||
| ## Expected Results | ||
|
|
||
| - Windows drive paths are decoded as `C:/...`, not `/C:/...` or `C:\\C:\\...`. | ||
| - Existing files return successfully through `/codex-local-browse/C:/...`. | ||
| - MP4 files use the correct media content type and support browser range requests. | ||
| - Directory, parent, and edit links use `/codex-local-browse/C:/...` or `/codex-local-edit/C:/...` with forward slashes and a separator after the route prefix. | ||
| - Unix absolute paths and UNC paths retain their existing behavior. |
There was a problem hiding this comment.
2. Missing light/dark verification step 📘 Rule violation ☼ Reliability
The added manual test case for the changed link/browse UI does not require validation in both light and dark themes. This increases the risk of theme-specific regressions going unnoticed.
Agent Prompt
## Issue description
The manual test entry for the Windows local-browse link behavior does not include explicit verification in both light and dark themes.
## Issue Context
PR Compliance requires changed UI surfaces to be validated in both themes to prevent dark-mode regressions.
## Fix Focus Areas
- tests/chat-composer-rendering/windows-absolute-file-links-open-local-browse.md[8-23]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
|
Code review by qodo was updated up to the latest commit 06442fb |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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 `@src/server/localBrowseUi.ts`:
- Around line 144-158: Update normalizeLocalRoutePath, toBrowseHref, and
toEditHref so each normalized filesystem path segment is encoded with
encodeURIComponent while preserving slash separators; do not rely on encodeURI,
which leaves # and ? unescaped. Add regression coverage verifying filenames
containing # and ? remain part of the routed path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: a65b4fe7-753c-46a1-a290-9b0519a0e440
📒 Files selected for processing (3)
src/server/localBrowseUi.test.tssrc/server/localBrowseUi.tstests/chat-composer-rendering/windows-absolute-file-links-open-local-browse.md
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/chat-composer-rendering/windows-absolute-file-links-open-local-browse.md
Comment on lines
+144
to
+158
| function normalizeLocalRoutePath(pathValue: string): string { | ||
| const normalized = pathValue.replace(/\\/gu, '/') | ||
| return normalized.startsWith('/') ? normalized : `/${normalized}` | ||
| } | ||
|
|
||
| export function toBrowseHref(pathValue: string, newProjectName = ''): string { | ||
| const normalizedName = normalizeNewProjectName(newProjectName) | ||
| const query = normalizedName ? `?newProjectName=${encodeURIComponent(normalizedName)}` : '' | ||
| return `/codex-local-browse${encodeURI(pathValue)}${query}` | ||
| return `/codex-local-browse${encodeURI(normalizeLocalRoutePath(pathValue))}${query}` | ||
| } | ||
|
|
||
| function toEditHref(pathValue: string, newProjectName = ''): string { | ||
| export function toEditHref(pathValue: string, newProjectName = ''): string { | ||
| const normalizedName = normalizeNewProjectName(newProjectName) | ||
| const query = normalizedName ? `?newProjectName=${encodeURIComponent(normalizedName)}` : '' | ||
| return `/codex-local-edit${encodeURI(pathValue)}${query}` | ||
| return `/codex-local-edit${encodeURI(normalizeLocalRoutePath(pathValue))}${query}` |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repo files around localBrowseUi:"
fd -a 'localBrowseUi\.(ts|js|tsx)$' . || true
file="$(fd 'localBrowseUi\.(ts|js|tsx)$' . | head -n1 || true)"
if [ -n "${file:-}" ]; then
echo "FILE=$file"
wc -l "$file"
sed -n '1,240p' "$file" | cat -n
fi
echo
echo "Search usages of toBrowseHref/toEditHref/export:"
rg -n "toBrowseHref|toEditHref|normalizeLocalRoutePath|codex-local-(browse|edit)" . || true
echo
echo "Node browser URL behavior:"
node - <<'JS'
const paths = ['/tmp/report#1.txt', 'tmp/a?b.txt', '/tmp/a/b#b.txt?query=1'];
for (const pathValue of paths) {
const normalized = pathValue.replace(/\\/g, '/')
const href = `/codex-local-browse${encodeURI(normalized.startsWith('/') ? normalized : `/${normalized}`)}`
const u = new URL(`http://example.com${href}`)
console.log({ pathValue, href, pathname: u.pathname, search: u.search, hash: u.hash })
}
console.log("imports localBrowseUi?:", (() => { try { import('./src/server/localBrowseUi.ts').then(()=>{}, e=>console.log('async import:', e.message)); import('./src/server/localBrowseUi.js').then(()=>{}, e=>console.log('cjs async import:', e.message)); } catch(e){ console.log('top import error', e.message) } })());
JSRepository: friuns2/codex-mobile
Length of output: 17011
Encode filesystem path segments before constructing the route URL.
encodeURI leaves ? and # unescaped. A filename like report#1.txt becomes /codex-local-browse/tmp/report#1.txt; the browser treats #1.txt as a fragment, so /codex-local-browse/*path receives only /tmp/report. Encode each normalized path segment with encodeURIComponent while preserving / separators, and add regression coverage for # and ? filenames.
🤖 Prompt for 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.
In `@src/server/localBrowseUi.ts` around lines 144 - 158, Update
normalizeLocalRoutePath, toBrowseHref, and toEditHref so each normalized
filesystem path segment is encoded with encodeURIComponent while preserving
slash separators; do not rely on encodeURI, which leaves # and ? unescaped. Add
regression coverage verifying filenames containing # and ? remain part of the
routed path.
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
/C:/...browse paths back toC:/...on WindowsRoot cause
CodexApp deliberately generates
/codex-local-browse/C:/...URLs for Windows file links. The server decoded the path as/C:/..., which Node resolves on Windows asC:\\C:\\..., so existing files returned 404.User impact
Agent-generated links to Windows files now open through local browse. Media files continue to use the existing
sendFilepath, including byte-range support for MP4 playback and seeking.Verification
pnpm exec vitest run src/server/localBrowseUi.test.ts(3 passed)pnpm run build(passed).ps1URL returned 200.mp4URL returnedvideo/mp4,Accept-Ranges: bytes, and206 Partial Contentfor a 100-byte rangehrefOk,titleOk, andtextOkoutput/playwright/testchat-windows-local-browse-cjs.pngThe full unit suite is 148/150 on Windows. The two unrelated failures require unprivileged symlink creation and POSIX
0600mode reporting.Performance audit
The change adds one bounded regular-expression check to path decoding. It does not add requests, filesystem operations, polling, fanout, payload copies, or cache invalidation; successful links continue through the existing single
statplussendFilerequest path.Summary by CodeRabbit
/C:/...style paths.Directory navigation follow-up
mammoth-v1link, child HTTP 200 response, and parent navigation with PlaywrightPerformance remains one bounded string replacement and prefix check per generated href, with no additional requests, filesystem operations, polling, fanout, payload copying, or cache invalidation.