-
-
Notifications
You must be signed in to change notification settings - Fork 172
Fix Windows local browse paths #212
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,42 @@ | ||
| import { describe, expect, it } from 'vitest' | ||
| import { decodeBrowsePath, toBrowseHref, toEditHref } from './localBrowseUi' | ||
|
|
||
| describe('decodeBrowsePath', () => { | ||
| it('removes the browse-route slash before Windows drive paths', () => { | ||
| expect(decodeBrowsePath('/C:/Users/Nulled/video.mp4', 'win32')).toBe('C:/Users/Nulled/video.mp4') | ||
| expect(decodeBrowsePath('/%43%3A/Users/Nulled/file.ps1', 'win32')).toBe('C:/Users/Nulled/file.ps1') | ||
| }) | ||
|
|
||
| it('preserves Unix, UNC, and already normalized paths', () => { | ||
| expect(decodeBrowsePath('/home/codex/file.txt', 'linux')).toBe('/home/codex/file.txt') | ||
| expect(decodeBrowsePath('//server/share/file.txt', 'win32')).toBe('//server/share/file.txt') | ||
| expect(decodeBrowsePath('C:/Users/Nulled/file.txt', 'win32')).toBe('C:/Users/Nulled/file.txt') | ||
| }) | ||
|
|
||
| it('leaves malformed URL encoding usable for the normal validation path', () => { | ||
| expect(decodeBrowsePath('/tmp/100%/file.txt', 'linux')).toBe('/tmp/100%/file.txt') | ||
| }) | ||
| }) | ||
|
|
||
| describe('local browse route hrefs', () => { | ||
| it('adds the route separator and converts Windows path separators', () => { | ||
| const path = String.raw`C:\Users\Nulled\Documents\invasion\artifacts\autonomous-weaponry\mammoth-v1` | ||
| expect(toBrowseHref(path)).toBe( | ||
| '/codex-local-browse/C:/Users/Nulled/Documents/invasion/artifacts/autonomous-weaponry/mammoth-v1', | ||
| ) | ||
| expect(toEditHref(String.raw`C:\Users\Nulled\file.ps1`)).toBe( | ||
| '/codex-local-edit/C:/Users/Nulled/file.ps1', | ||
| ) | ||
| }) | ||
|
|
||
| it('preserves Unix and UNC absolute paths', () => { | ||
| expect(toBrowseHref('/home/codex/folder')).toBe('/codex-local-browse/home/codex/folder') | ||
| expect(toBrowseHref(String.raw`\\server\share\folder`)).toBe('/codex-local-browse//server/share/folder') | ||
| }) | ||
|
|
||
| it('keeps project picker query parameters encoded', () => { | ||
| expect(toBrowseHref(String.raw`C:\Users\Nulled\Parent Folder`, 'My Project')).toBe( | ||
| '/codex-local-browse/C:/Users/Nulled/Parent%20Folder?newProjectName=My%20Project', | ||
| ) | ||
| }) | ||
| }) |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -69,13 +69,23 @@ export function normalizeLocalPath(rawPath: string): string { | |
| return trimmed | ||
| } | ||
|
|
||
| export function decodeBrowsePath(rawPath: string): string { | ||
| export function decodeBrowsePath(rawPath: string, platform: NodeJS.Platform = process.platform): string { | ||
| if (!rawPath) return '' | ||
| let decoded: string | ||
| try { | ||
| return decodeURIComponent(rawPath) | ||
| decoded = decodeURIComponent(rawPath) | ||
| } catch { | ||
| return rawPath | ||
| decoded = rawPath | ||
| } | ||
|
|
||
| // 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) | ||
| } | ||
|
|
||
| return decoded | ||
| } | ||
|
|
||
| export function isTextEditablePath(pathValue: string): boolean { | ||
|
|
@@ -131,16 +141,21 @@ function normalizeNewProjectName(value: string): string { | |
| return value.trim().replace(/[\\/]+/gu, '').trim() | ||
| } | ||
|
|
||
| function toBrowseHref(pathValue: string, newProjectName = ''): string { | ||
| 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}` | ||
|
Comment on lines
+144
to
+158
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 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.
🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
| function escapeForInlineScriptString(value: string): string { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| ### Feature: Windows absolute file links open through local browse | ||
|
|
||
| ## Prerequisites | ||
|
|
||
| - Run CodexApp on Windows. | ||
| - Open TestChat with a project that contains an existing text file and MP4 file on a drive-letter path. | ||
|
|
||
| ## 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. | ||
|
Comment on lines
+8
to
+15
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 1. Missing testchat hrefok checks 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
|
||
|
|
||
| ## 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. | ||
|
Comment on lines
+8
to
+23
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 2. Missing light/dark verification step 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
|
||
|
|
||
| ## Rollback / Cleanup | ||
|
|
||
| - Close the opened file and video tabs. No files are modified by this test. | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1. Windows browse hrefs broken
🐞 Bug≡ CorrectnessAgent Prompt
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools