fix(web): make Windows file links clickable - #6237
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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 |
a1e59dc to
f3024d6
Compare
ApprovabilityVerdict: Approved 2e03244 Straightforward bug fix making Windows file paths clickable in markdown by converting them to file:// URIs before HTML sanitization. Changes are self-contained with good test coverage, including verification that unsafe schemes remain blocked. You can customize Macroscope's approvability policy. Learn more. |
f3024d6 to
6717343
Compare
Dismissing prior approval to re-evaluate 6717343
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 71371ee. Configure here.
Dismissing prior approval to re-evaluate 2e03244
|
Confirmed on Windows. We reproduced drive-letter Windows Markdown links failing to open, then tested the current PR head (2e03244) in our downstream build. The links now resolve and open correctly, including the chat rendering path with line breaks. Thanks for the fix. |
|
I reproduced one remaining gap with the current PR head: valid angle-bracketed destinations containing spaces or balanced parentheses survive the Markdown pipeline, but I opened a small, tested follow-up against this branch: peculiarnewbie#1 It keeps the precomputed map as the fast path and falls back to resolving the parsed/normalized |
CDVolvik
left a comment
There was a problem hiding this comment.
46/46 across markdown-links.test.ts, markdown-links-rendering.test.tsx and filePathDisplay.test.ts on Linux (Node 24).
I went looking for a mismatch between the rendering test's pipeline and the real one, since markdown-links-rendering.test.tsx builds its own ReactMarkdown rather than rendering ChatMarkdown. There isn't one today. The test uses
protocols: { ...defaultSchema.protocols, href: [...(defaultSchema.protocols?.href ?? []), "file"] }
urlTransform={(href) => rewriteMarkdownFileUriHref(href) ?? defaultUrlTransform(href)}and ChatMarkdown uses exactly the same protocol list in CHAT_MARKDOWN_SANITIZE_SCHEMA, plus
const markdownUrlTransform = useCallback((href: string) => {
return rewriteMarkdownFileUriHref(href) ?? defaultUrlTransform(href);
}, []);So the behaviour is faithfully mirrored. Worth flagging anyway, because the mirroring is by hand and there are now three things that have to stay in step: the file protocol entry, remarkRewriteWindowsFileLinks sitting in the remark list, and that urlTransform. Drop any one of them from ChatMarkdown and every test in the new file still passes, because the test supplies its own. CHAT_MARKDOWN_SANITIZE_SCHEMA and markdownUrlTransform are both module-local, so exporting them and importing them into the test would turn this from a parallel implementation into an actual guard, without changing what is asserted.
The still removes unsafe schemes case is the right instinct given this widens what survives sanitization. One more worth adding next to it: a single-letter scheme that is not a drive path. WINDOWS_DRIVE_PATH_PATTERN keys off [A-Za-z]:, and the reason javascript: is safe is that it is many letters, not one. Something like [x](c:/Users/x) versus a genuine one-letter scheme would pin that the widening is limited to drive paths rather than to any short scheme.
isAbsolutePath in filePathDisplay.ts only matches a forward slash after the drive letter:
return path.startsWith("/") || /^[A-Za-z]:\//.test(path);That is fine where it is called, since the value has already been through normalizeMarkdownLinkDestination, but the function name reads general and the next caller will not necessarily normalize first. C:\Users\... returns false from a function called isAbsolutePath. Either accepting both separators or naming it for the normalized input would stop that being a trap later.
|
we need this merged |

Problem
On Windows, Markdown links with direct drive paths such as
[artifact](C:/Users/.../artifact.mp4)rendered as blue text with no usable target.rehype-sanitizeinterpreted the drive letter as a URL scheme and removed thehrefbefore React Markdown's URL transform could normalize it.Fix
Rewrite only Windows drive-path link destinations to the already-allowed
file:///form while the document is still Markdown AST. The existing URL transform then restores the drive path and sends it through T3 Code's existing file-link renderer. Forward- and backslash drive paths, including encoded and unencoded Unicode segments, are canonicalized to the same lookup key, while unsafe schemes remain subject to the normal sanitizer.Absolute Windows paths outside the workspace also remain absolute in tooltips and copied paths. The composer's existing mention-chip behavior is unchanged and out of scope.
Before
The label looked like a link, but it had no target and could not be clicked.
After
The same destination reaches the existing file-link component; hovering reveals the full path and the link is actionable.
Verification
vp test run apps/web/src/markdown-links.test.ts apps/web/src/markdown-links-rendering.test.tsx apps/web/src/filePathDisplay.test.ts— 46 tests passedvp run --filter @t3tools/web typecheckvp lintandvp fmt --checkfor all six changed filesImplemented with GPT-5.6-Sol in the Codex harness via T3 Code.
Note
Medium Risk
Touches markdown href sanitization and file-link resolution, which are security-adjacent URL handling paths. Scope is narrow and unsafe schemes still go through the existing sanitizer.
Overview
Fixes Windows drive-path markdown links (e.g.
C:/...orC:\\...) that rendered as non-clickable text becauserehype-sanitizetreated the drive letter as a URL scheme and stripped thehref.Adds
remarkRewriteWindowsFileLinks, which rewrites only Windows drive destinations to allowedfile:///form in mdast before sanitization. The existing URL transform then restores the drive path for the normal file-link renderer. Also canonicalizes backslash/unicode variants into a shared lookup key vianormalizeMarkdownFileLinkHrefKey.Separately,
formatWorkspaceRelativePathnow treats Windows drive paths as absolute, so paths outside the workspace stay absolute in tooltips/copied paths instead of getting a workspace label prefix.Reviewed by Cursor Bugbot for commit 2e03244. Bugbot is set up for automated code reviews on this repo. Configure here.
Note
Fix Windows file path links to be clickable in chat markdown
remarkRewriteWindowsFileLinksremark plugin to convert Windows drive-path links (e.g.C:\foo) intofile:///C:/...URIs before HTML sanitization, preventing the sanitizer from stripping them.normalizeMarkdownFileLinkHrefKeyto produce stable canonical keys for Windows file links regardless of backslashes or percent-encoding, replacing the local helper in ChatMarkdown.tsx.formatWorkspaceRelativePathin filePathDisplay.ts to avoid prefixing absolute Windows drive paths with the workspace label.Macroscope summarized 2e03244.