fix: support windows file links in messages#570
fix: support windows file links in messages#570Reekin wants to merge 6 commits intoDimillian:mainfrom
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. |
2 participants
Summary
Validation