Skip to content

fix: open Windows file links in chat - #5283

Closed
thiscallnet wants to merge 1 commit into
pingdotgg:mainfrom
thiscallnet:fix/windows-markdown-file-links
Closed

fix: open Windows file links in chat#5283
thiscallnet wants to merge 1 commit into
pingdotgg:mainfrom
thiscallnet:fix/windows-markdown-file-links

Conversation

@thiscallnet

@thiscallnet thiscallnet commented Aug 3, 2026

Copy link
Copy Markdown

Summary

Make chat file links in Windows native app clickable
Clicking opens sidebar with the file
Adds a relevant file icon too

Why

Chat file links in Windows native apps look clickable, act clickable, but arent, its confusing, would be nice to add clickable behavior

Validation

Have AI reply to you with file links in chat on Windows app:

Before:

image
before.mp4

After ( clickable, opens sidebar with file ):

image
after.mp4

Checklist

  • This PR is small and focused
  • I explained what changed and why
  • I included before/after screenshots for any UI changes
  • I included a video for animation/interaction changes

Note

Low Risk
Localized markdown link normalization and sanitize allowlist for chat rendering; no auth or server changes, with added unit tests.

Overview
Windows drive-letter paths in chat markdown (e.g. C:/Users/... or file:///D:/...) now survive sanitization and are normalized so the existing file-link UI can treat them as workspace files instead of broken or external links.

rehype-sanitize is extended so single-letter href protocols are allowed before urlTransform runs—otherwise C: is stripped as an unknown scheme. rewriteMarkdownFileUriHref then canonicalizes backslashes, encoded %5C, and duplicate slashes to a leading-slash form like /C:/path, which keeps the drive letter from being parsed as a URL scheme downstream.

Tests cover file:// Windows URIs, angle-bracketed destinations, and several slash/encoding variants; non-file hrefs like https:// and x:command stay unchanged.

Reviewed by Cursor Bugbot for commit 76625da. Bugbot is set up for automated code reviews on this repo. Configure here.

Note

[!NOTE]

Fix Windows drive letter file links opening in chat

  • Extends the rehype-sanitize schema in ChatMarkdown.tsx to allow Windows drive-letter pseudo-protocols (e.g. C:) so they are not stripped before urlTransform runs.
  • Updates rewriteMarkdownFileUriHref in markdown-links.ts to canonicalize Windows drive-style hrefs (including backslash, URL-encoded backslash, and duplicate-slash variants) to a leading-slash form like /C:/path.
  • Adds tests in markdown-links.test.ts covering normalization of file:// URIs, angle-bracketed paths, and various slash/encoding variants.

Macroscope summarized 76625da.

@coderabbitai

coderabbitai Bot commented Aug 3, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 87977cab-9de8-4fbe-a6ba-7757776415ab

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list. labels Aug 3, 2026

@cursor cursor Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Want fixes drafted automatically? Bugbot Autofix can create code changes for findings. A team admin can enable Autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit b63420e. Configure here.

Comment thread apps/web/src/markdown-links.ts
@macroscopeapp

macroscopeapp Bot commented Aug 3, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved e1752e5

Straightforward bug fix for Windows file path handling in chat markdown. Changes are self-contained to link processing logic, include comprehensive test coverage, and have no security or infrastructure implications.

You can customize Macroscope's approvability policy. Learn more.

@thiscallnet
thiscallnet force-pushed the fix/windows-markdown-file-links branch from e1752e5 to 3b9f4f9 Compare August 3, 2026 19:38
@thiscallnet
thiscallnet force-pushed the fix/windows-markdown-file-links branch from 3b9f4f9 to 76625da Compare August 3, 2026 21:56
@bts-cssi

Copy link
Copy Markdown

Second Windows repro, independently confirming this, plus one consequence that I don't think has been spelled out yet.

Environment: T3 Code Nightly 0.0.33, Windows 11 (26220) x64, react-markdown 10.1.0, rehype-sanitize 6.0.0.

Repro: ask an agent where a build artifact landed. Both Codex and Claude routinely emit a bare drive path as the link destination rather than a file:// URI:

[Download the local installer](C:\projects\app\dist\App_1.12.0_x64-setup.exe)

That renders as an ordinary blue link, and clicking it does nothing at all.

Mechanism. Two independent layers each kill it, which lines up with the two halves of this PR:

  1. rehype-sanitize runs before react-markdown's urlTransform, and CHAT_MARKDOWN_SANITIZE_SCHEMA allowlists file: but not drive letters — so C: reads as an unknown protocol and the href attribute is dropped outright. Running main's actual schema through the pipeline:

    destination href after sanitize
    C:\projects\...\setup.exe undefined
    C:/projects/.../setup.exe undefined
    file:///C:/projects/.../setup.exe preserved
    /Users/x/project/AGENTS.md preserved
  2. Even if it survived sanitize, defaultUrlTransform returns "" for any X:/… — the first colon precedes the first slash and C isn't in safeProtocol. Verified directly against react-markdown 10.1.0: C:/… and D:/… both → "", while POSIX and relative destinations pass through untouched.

The part worth calling out: it isn't only the anchor that dies. The file-chip lookup is keyed on the transformed href — markdownFileLinkMetaByHref.get(normalizeMarkdownLinkHrefKey(href)) — so an empty href also misses the map and the destination never becomes a file chip. That costs the chip's right-click menu as well, so there's no "Copy full path" and no "Open in editor" to fall back on. On Windows the link is completely inert rather than merely pointing somewhere useless, which is why this reads as broken rather than as a minor papercut. POSIX destinations are unaffected throughout, which is what makes this Windows-only.

I read the diff, and allowing drive letters at the sanitize layer plus canonicalizing to /C:/… before urlTransform closes both layers — and because the lookup key now matches, it restores the chip and its context menu too, not just the click. Would be good to see this land.

@CDVolvik CDVolvik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Heads up that #6237 is open against the same bug and touches the same two places, CHAT_MARKDOWN_SANITIZE_SCHEMA and rewriteMarkdownFileUriHref. The approaches are not just different, they produce different href shapes, so whichever lands second will not merge cleanly and its tests will contradict the first. Worth a maintainer picking one before either goes further.

The comparison, since it is not obvious from either description alone:

This PR widens the sanitizer to accept every single letter as a scheme:

const WINDOWS_DRIVE_LINK_PROTOCOLS = Array.from("ABC...xyz");
href: [...(defaultSchema.protocols?.href ?? []), "file", ...WINDOWS_DRIVE_LINK_PROTOCOLS],

#6237 instead runs a remark plugin before sanitization so drive paths never reach it looking like a scheme, and leaves the allowlist at file. Both end up safe today, because urlTransform runs afterwards and falls back to defaultUrlTransform, which drops anything that is not http/https/mailto/tel. But this version's safety depends on that second gate staying correct forever, for 52 schemes rather than one. Sanitizer allowlists are the thing people audit and trust, so widening it to a: through z: is a bigger claim than the bug needs. Your own x:command test proves the backstop works rather than that the allowlist is tight.

The other thing worth weighing is the leading-slash canonical form:

if (/^\/[A-Za-z]:\//.test(windowsDriveHref)) return windowsDriveHref;

WINDOWS_DRIVE_PATH_PATTERN is /^[A-Za-z]:[\\/]/, anchored, so /C:/Users/... stops matching it. Everything downstream that keys off that pattern therefore stops seeing these as Windows paths, and they get routed into the POSIX branch instead:

if (path.startsWith("/")) return looksLikePosixFilesystemPath(path);

That still works for the cases in your tests, but only by accident: /C:/ is not in POSIX_FILE_ROOT_PREFIXES, so it survives on the trailing-extension check or the :42 position suffix. A drive link with neither, say a directory link like /C:/Users/mike/project, fails all three and stops resolving as a file link. #6237 keeps C:/Users/... intact and so keeps matching the existing pattern.

Also note this changes the return shape of an already-exported function, which is why the two existing expectations had to be edited:

-).toBe("D:/Programme/t3code/apps/web/src/components/chat/OpenInPicker.tsx#L69");
+).toBe(canonicalHref);   // now "/D:/Programme/..."

Only two call sites today, both in ChatMarkdown.tsx, so the blast radius is small right now. But an exported helper whose contract is "no leading slash" quietly becoming "leading slash on Windows" is the kind of thing that bites the third caller.

The %5C and C:// normalization here is genuinely good and I did not see an equivalent in #6237. If the maintainers go with the remark-plugin approach, those cases are worth porting across rather than losing.

@CDVolvik CDVolvik left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Formalizing my 8/14 review as changes-requested — the head commit is unchanged (76625da) and both findings still hold:

  1. The leading-slash form this PR produces (/C:/...) no longer matches WINDOWS_DRIVE_PATH_PATTERN = /^[A-Za-z]:[\/]/ — the pattern is anchored, so every downstream consumer of that check stops recognizing the path as a Windows drive path. #6237 fixes the same bug with the C:/... shape, which keeps the pattern matching.

  2. The sanitizer widening catches all 52 single-letter prefixes, not just real drive letters in path position — that over-matches.

Full detail with repro in my earlier review on this commit.

@t3dotgg

t3dotgg commented Aug 25, 2026

Copy link
Copy Markdown
Member

Note

🤖 GPT-5.6 Sol responding on behalf of Theo

Closing in favor of merged #8081, which fixes Windows drive-path file links in chat without weakening Markdown URL sanitization. Your work is credited in #8081.

@t3dotgg t3dotgg closed this Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants