fix: support windows file links in messages#570
Conversation
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c3616b379f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac232a3205
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b10b4c77ef
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 158bba5602
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review
Overall: Thorough fix for Windows file link handling in the Markdown renderer. The refactoring extracts file link parsing into a dedicated utility module, which is a clear improvement over the scattered regex patterns. Excellent test coverage.
Strengths
-
Centralized parsing via fileLinks utility. Moving parseFileLocation, normalizeFileLinkPath, and fromFileUrl into a shared utility eliminates duplicated regex patterns (FILE_LINE_SUFFIX_PATTERN, FILE_HASH_LINE_SUFFIX_PATTERN, etc.) and makes the logic testable in isolation.
-
Windows path support is comprehensive. Handles drive letters (I:\path), UNC paths (\server\share), namespace-prefixed paths (\?\C:), and the #L anchor normalization (converting #L12 to :12). The test matrix covers all these variants.
-
Test coverage is excellent. 147+ new lines of Markdown.test.tsx covering Windows absolute paths in plain text, #L anchors in hrefs, workspace route exclusions, file:// URL edge cases, and percent-encoding corner cases. The useFileLinkOpener tests for Copy Link with namespace-prefixed paths are a nice addition.
Potential Issues
-
resolveHrefFilePath candidate loop: The new approach iterates over [url, safeDecodeURIComponent(url)] and checks isLikelyFileHref for each. This means double-decoding could occur if the URL is already decoded. The Set-based dedup handles the common case, but a URL like %2525 (double-encoded %) would decode to %25, then to %, creating a different path than intended. Low risk in practice since file paths rarely contain percent signs.
-
urlTransform early return for file links: Adding the resolveHrefFilePath check before the scheme-based checks in urlTransform is important. Without this, react-markdown's default URL sanitizer would strip Windows absolute paths (they look like unknown schemes). This is correct but worth a comment explaining why it's needed.
-
isKnownLocalWorkspaceRouteFilePath guard in getLinkablePath: Good catch preventing /workspace/settings#L12 from being treated as a file link. The test for inline code workspace routes confirms this works correctly.
-
title attribute addition on file links: Adding title={hrefFilePath} to the anchor element is a nice UX touch, showing the full resolved path on hover. This also serves as documentation of what path will be opened on click.
No bugs or security issues found. Clean refactoring with strong test coverage.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 08f6339091
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
@Dimillian I think this is ready to merge. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21a0a65e3d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9206362248
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: Windows File Links in Messages
Quick check - can you share the diff?
xkonjin
left a comment
xkonjin
left a comment
There was a problem hiding this comment.
Code Review: Windows File Links in Messages
Overall: Thorough work handling Windows paths (drive letters, backslashes, UNC paths) in the Markdown file-link resolver. Test coverage is excellent with 15+ new test cases covering edge cases.
Items:
-
The test 'keeps generic workspace routes as normal markdown links' was renamed to 'keeps exact workspace routes' and the test path changed from '/workspace/reviews/overview' to '/workspace/reviews'. This changes the test semantics - the original tested that nested routes under reserved names were preserved as local routes. Make sure the nested-route behavior is still covered (the new 'keeps nested reviews routes local' test covers the /workspaces/ variant but not the /workspace/ singular form with sub-paths).
-
Windows path linkification with unescaped percent signs (the '100%.tsx' tests) is handled gracefully, which is good. However, verify that the percent-encoding roundtrip is consistent - the href shows '100%25.tsx' but onOpenFileLink receives '100%.tsx'. If any downstream code re-encodes the path, you could end up with double-encoding.
-
The file:// URL handling strips non-line fragments (#overview becomes no suffix) but preserves #L fragments (#L12 becomes :12). This is the right behavior. However, the test 'ignores non-line file URL fragments' verifies the callback receives '/tmp/report.md' without the fragment. Make sure this is consistent with how the editor actually opens files - some editors support anchor-based navigation.
-
UNC path support (file://server/share/...) properly resolves to '//server/share/...' which is correct for Windows. No issues.
Tests: Exceptional coverage - drive letters, UNC paths, percent-encoded filenames, #L anchors, workspace route disambiguation, inline code exclusion. Well structured.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eb3f373f1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 136b706c38
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10369de2c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
src/utils/fileLinks.ts
Outdated
| if (routeInfo.tailSegments.length === 0) { | ||
| return true; | ||
| } | ||
| if (routeInfo.tailSegments.length !== 1) { | ||
| return false; |
There was a problem hiding this comment.
/workspace/settings:12 as a local route
Because isKnownLocalWorkspaceRoutePath() now derives routeInfo from parseFileLocation(rawPath), exact mounted paths like /workspace/settings:12 and /workspaces/team/reviews:9 are reduced to /workspace/settings/.../reviews and then classified as app routes here. In those cases the link is no longer passed to resolveMountedWorkspacePath() or onOpenFileLink, so extensionless root files/directories named settings or reviews become impossible to open when a line suffix is present, even though :12 is only meaningful for file links.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ead51f7667
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const allowedTailSegments = | ||
| LOCAL_WORKSPACE_ROUTE_TAIL_SEGMENTS[ | ||
| routeInfo.routeSegment as keyof typeof LOCAL_WORKSPACE_ROUTE_TAIL_SEGMENTS | ||
| ]; | ||
| return (allowedTailSegments as ReadonlySet<string>).has(routeInfo.tailSegments[0]); |
There was a problem hiding this comment.
For workspaces actually named settings or reviews, this narrower allowlist now lets paths like /workspace/settings/.. and /workspace/settings/../secret fall through as “files”. resolveMountedWorkspacePath() then treats settings as the mounted workspace name and joins the remaining .. segments onto the active workspace, so clicking the message link can open the parent directory (or a sibling outside the repo) instead of staying on the local app route. The previous blanket guard for settings/* and reviews/* did not allow that escape hatch.
Useful? React with 👍 / 👎.
| const parsedLocation = parseFileLocation(trimmed); | ||
| const pathOnly = parsedLocation.path.trim(); | ||
| if (/[?#]/.test(pathOnly)) { | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The new ?[#] rejection blocks href targets that use Windows namespace prefixes, because paths like \\?\C:\repo\App.tsx:12 and \\?\UNC\server\share\App.tsx:12 contain a literal ? before the drive/share. Plain-text file references still parse via parseInlineFileTarget(), and toFileUrl() now explicitly round-trips these prefixes, so markdown links with the same paths regress: resolveMessageFileHref() returns null, the click falls back to the generic local-link handler, and onOpenFileLink is never invoked.
Useful? React with 👍 / 👎.
3 participants
Summary
Validation