feat(desktop): make file paths in chat messages clickable - #9677
feat(desktop): make file paths in chat messages clickable#9677bdeenyy wants to merge 53 commits into
Conversation
![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: b4f4f91aea
ℹ️ 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".
|
|
||
| ipcMain.handle('open-path-in-explorer', async (_event, path: string) => { | ||
| try { | ||
| shell.showItemInFolder(path); |
There was a problem hiding this comment.
When the clicked link is a ~/... path, which the new linkifier explicitly recognizes in ui/desktop/src/utils/linkifyPaths.ts, this passes the literal tilde to Electron. Electron/Node file APIs do not perform shell expansion, and this app already uses expandTilde before other filesystem operations, so links like ~/.config/app/settings.json will fail to reveal the actual file. Please expand the path before calling showItemInFolder.
Useful? React with 👍 / 👎.
| const UNIX_PATH_RE = /(?:^|[\s('"`\[(,;]|\/\*.*?\*\/)()(\/(?:[a-zA-Z0-9._+-]+\/){1,}[a-zA-Z0-9._+-]+)/g; | ||
| const TILDE_PATH_RE = /(?:^|[\s('"`\[(,;]|\/\*.*?\*\/)()(~\/(?:[a-zA-Z0-9._+-]+\/)*[a-zA-Z0-9._+-]+)/g; |
There was a problem hiding this comment.
For paths followed by sentence punctuation, e.g. Created /tmp/result.txt., the final period is included in the generated link because . is allowed as the last path character. That makes the click try to reveal /tmp/result.txt. instead of /tmp/result.txt, so common assistant prose produces broken file links. Please strip terminal sentence punctuation or require a non-punctuation final character when matching these prose paths.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cecfaa628c
ℹ️ 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".
| ExtensionConfig::Frontend { .. } | ||
| | ExtensionConfig::InlinePython { .. } | ||
| | ExtensionConfig::Sse { .. } => return Ok(None), |
There was a problem hiding this comment.
When a user already has a configured sse, inline_python, or frontend extension, this branch returns None and on_get_config_extensions drops it with flatten(). The desktop now populates configured extensions through this ACP endpoint (ui/desktop/src/acp/extensions.ts), so those existing config entries disappear from Settings and bulk operations even though they remain in the config, leaving users unable to see, disable, or remove legacy SSE/custom entries. Please keep returning a list entry for unsupported-but-configured extensions instead of filtering them out.
Useful? React with 👍 / 👎.
| let bytes = response | ||
| .bytes() | ||
| .await | ||
| .map_err(|error| format!("failed to read image response: {error}"))?; |
There was a problem hiding this comment.
For read_image URLs where the server omits or understates Content-Length, this buffers the entire response with response.bytes() before the 20 MB limit is enforced back in load_image. A large or unbounded response can therefore consume memory despite the advertised MAX_IMAGE_BYTES guard. Please stream the body and stop once the cap is exceeded, or otherwise apply a hard download-size limit before collecting the bytes.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e0601f6dee
ℹ️ 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 localRe = new RegExp(re.source, 'g'); | ||
| while ((m = localRe.exec(text)) !== null) { | ||
| const prefixLen = m[1].length; | ||
| const path = m[2]; |
There was a problem hiding this comment.
When any message contains a recognized path, the regexes above only define one capture group (the path itself), but this reads m[2] as the path. That makes path undefined for normal inputs like /home/user/file.txt, so the later path.length access throws while Markdown is rendering instead of producing a clickable link. Either add a separate prefix capture group or read the path from m[1] and compute the prefix length differently.
Useful? React with 👍 / 👎.
e0601f6 to
d6ddeaa
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6ddeaa65b
ℹ️ 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".
| env_keys, | ||
| timeout, | ||
| bundled, | ||
| .. | ||
| } => GooseExtension::Mcp { | ||
| server: McpServer::Stdio(McpServerStdio::new(name, cmd).args(args.clone())), |
There was a problem hiding this comment.
When a configured stdio extension still uses the legacy envs map, this arm discards it (..) and only returns env_keys; the analogous streamable_http arm does the same. The desktop now builds its Settings list from this ACP response and round-trips those entries through addExtension for toggles/edits, so a user with literal legacy env values can lose the actual environment values from config instead of having them migrated or preserved. Please either include/migrate the legacy env values or avoid round-tripping this lossy representation for config updates.
Useful? React with 👍 / 👎.
28d68ad to
e2a7475
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2a74753ee
ℹ️ 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".
| visit(tree, 'text', (node: Text, index: number | undefined, parent: Parent | undefined) => { | ||
| if (index === undefined || !parent) return; | ||
| linkifyNode(node, index, parent); |
There was a problem hiding this comment.
When a message contains a plain path such as See /etc/hosts, this visitor replaces the text node with a file-link node, but traversal then continues into the newly inserted link's text child and linkifies the same path again. That produces nested file links/anchors, which React/HTML cannot represent reliably and can break the click target; skip traversal inside link nodes or advance the visitor past the inserted nodes after splicing.
Useful? React with 👍 / 👎.
|
Thanks for the contribution, and congrats on landing #9601! 🎉 We'd like to take this one but there are a few things to sort out first:
Also note that, per our contribution guide, for a new feature like this it's best to open an issue/discussion first so we can align on direction. Could you address the Codex comments (fix or a one-line reason per AI Code Reviews)? We'll revisit in a few days. Thanks! |
Add remark plugin (linkifyPaths) that detects Unix, tilde, and Windows file paths in markdown text and converts them to open-file:// links. Clicking a path calls shell.showItemInFolder() to reveal the file in Finder/Explorer. Signed-off-by: Denis <i@deenyy.ru>
Detect Unix, tilde, and Windows file paths in chat text and inline code nodes, and render them as open-file:// links so users can click through to the referenced file. Signed-off-by: Denis <i@deenyy.ru>
Fix regex capture group indexing in findPaths, strip trailing sentence punctuation, skip URL-like matches, and avoid re-linkifying text inside links. Test the exported findPaths implementation and expand tilde paths before revealing files in the system file manager. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
e2a7475 to
e310220
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e310220557
ℹ️ 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 UNIX_PATH_RE = /(?:^|[\s('"`[(,;]|\/\*.*?\*\/)?(\/(?:[a-zA-Z0-9._+-]+\/){1,}[a-zA-Z0-9._+-]+)/g; | ||
| const TILDE_PATH_RE = /(?:^|[\s('"`[(,;]|\/\*.*?\*\/)?(~\/(?:[a-zA-Z0-9._+-]+\/)*[a-zA-Z0-9._+-]+)/g; | ||
| const WIN_PATH_RE = /(?:^|[\s('"`[(,;]|\/\*.*?\*\/)?([A-Za-z]:[\\/](?:[a-zA-Z0-9._+-]+[\\/])+[a-zA-Z0-9._+-]+)/g; |
There was a problem hiding this comment.
When a valid local path contains a space, these regexes stop the match at the first space and still create a link for the truncated prefix; for example Saved /Users/me/My Project/result.txt becomes a clickable /Users/me/My, which reveals the wrong or nonexistent item instead of the actual file. This affects common folder/file names with spaces, so the matcher should either include spaces with a reliable terminator or avoid linkifying partial segment matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks for the review and for merging #9601!
I've addressed the feedback in e310220:
P1 — regex group indexing: findPaths now reads the path from m[1] and computes prefixLen as m[0].length - m[1].length.
Tests: linkifyPaths.test.ts imports and tests the exported findPaths from the module instead of duplicating the logic inline.
Trailing punctuation: sentence-ending punctuation (e.g. /tmp/result.txt.) is stripped before creating the link.
Tilde paths: open-path-in-explorer now calls expandTilde() before shell.showItemInFolder().
Other Codex comments:
Nested links (P2): text/inlineCode nodes inside existing link parents are skipped during traversal.
URL false positives: paths immediately after a URL scheme (https://...) are ignored.
Also rebased onto latest main and dropped the duplicate scrollbar commit (already landed in #9601).
Re: opening an issue/discussion first — understood for future features. Happy to open one retroactively if that helps track this.
Let me know if anything else needs adjusting!
There was a problem hiding this comment.
Addressed the P2 "paths with spaces" feedback in 8ed41e8:
Replaced greedy regex with a segment parser that allows spaces inside folder/file names but stops before trailing prose.
open-file:// hrefs now use encodeURI() so spaces survive markdown URL handling.
bdeenyy
changed the title
Replace greedy regex matching with a path parser that allows spaces inside segment names while stopping before trailing prose. Encode spaces in open-file:// URLs so markdown hrefs remain valid. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ed41e8fec
ℹ️ 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 after = text[j]; | ||
| if (after === separator) return j; | ||
| if (after === undefined || /[.,;:!?)'\]"]/.test(after)) return j; |
There was a problem hiding this comment.
When a normal path is followed by a word and sentence punctuation, this branch treats that word as a spaced path continuation, so text like Created /tmp/result successfully. is linkified as /tmp/result successfully instead of /tmp/result. That makes common assistant prose reveal the wrong or nonexistent file; only include the spaced continuation when there is stronger evidence it is part of the path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 692f479: spaced continuations before punctuation now require path-like evidence (capitalized/dotted segment name, or short preceding token for multi-word folder names like my documents). Trailing sentence punctuation is stripped while scanning continuations so . is not absorbed via isPathChar.
Do not treat lowercase words after a complete path segment as part of the path when they precede sentence punctuation. Also strip trailing punctuation while scanning spaced continuations so dots are not absorbed into the word via isPathChar. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 692f479c4e
ℹ️ 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".
| } | ||
|
|
||
| function isUrlPath(text: string, index: number): boolean { | ||
| const before = text.slice(0, index); |
There was a problem hiding this comment.
When a message contains a long plain-text node with no paths, findPaths calls isValidPathStart for every character, and this prefix slice plus the later prefix slice/comment regex scan reprocess all preceding text each time. Since remarkLinkifyPaths now runs on every rendered chat message, a large assistant paragraph or pasted log outside a fenced code block can stall the desktop UI even though there is nothing to linkify; keep the scan linear by checking the immediate context/protocol state without slicing the whole prefix at each index.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 692f479c4e
ℹ️ 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".
| } | ||
|
|
||
| function isUrlPath(text: string, index: number): boolean { | ||
| const before = text.slice(0, index); |
There was a problem hiding this comment.
When a message contains a long plain-text node with no paths, findPaths calls isValidPathStart for every character, and this prefix slice plus the later prefix slice/comment regex scan reprocess all preceding text each time. Since remarkLinkifyPaths now runs on every rendered chat message, a large assistant paragraph or pasted log outside a fenced code block can stall the desktop UI even though there is nothing to linkify; keep the scan linear by checking the immediate context/protocol state without slicing the whole prefix at each index.
Useful? React with 👍 / 👎.
Scan text in a single pass with O(1) URL and block-comment checks instead of re-slicing prefixes at every index. Skip visiting children of generated open-file links to avoid re-linkifying path text. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 18923abb4e
ℹ️ 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".
| function isPathLikeSpacedWord(word: string, prevToken: string): boolean { | ||
| if (/[A-Z0-9._+-]/.test(word)) return true; | ||
| return ( | ||
| /^[a-z][a-z0-9]*$/.test(word) && prevToken.length <= 3 && !prevToken.includes('.') |
There was a problem hiding this comment.
The fresh case not covered by the earlier prose fix is when the matched path ends with a short basename: with Created /tmp/a successfully. this condition treats successfully as a spaced path segment because prevToken.length <= 3, so the link becomes /tmp/a successfully and the click opens a nonexistent item rather than /tmp/a. The prose guard should require stronger evidence than a short previous token at the end of the match.
Useful? React with 👍 / 👎.
| 'firefox:', | ||
| 'safari:', | ||
| 'goose:', | ||
| 'open-file:', |
There was a problem hiding this comment.
I checked the other isProtocolSafe call sites in MCPUIResourceRenderer and McpAppRenderer: safe protocols are opened directly via window.electron.openExternal without the confirmation path. Since open-file:// is only handled specially by MarkdownContent, marking it globally safe lets an MCP link action with this app-internal scheme bypass confirmation and get forwarded by main's open-external handler to the OS as an external protocol. Keep this scheme local to generated markdown file links instead of adding it to the shared safe list.
Useful? React with 👍 / 👎.
| a: (props) => { | ||
| const href = props.href; | ||
| if (href && href.startsWith(OPEN_FILE_PROTOCOL)) { | ||
| const filePath = decodeURIComponent(href.slice(OPEN_FILE_PROTOCOL.length)); |
There was a problem hiding this comment.
Generated file links are encoded, but arbitrary markdown can also include an open-file:// href now; if it contains malformed percent escapes such as [x](open-file:///%E0%A4%A), this decodeURIComponent runs during render and throws URIError, breaking the whole MarkdownContent render. Please catch decode failures or ignore invalid file-link hrefs before rendering the anchor.
Useful? React with 👍 / 👎.
Require multi-char basenames before spaced continuations, remove open-file from global safe protocols, guard decodeURIComponent for authored links, and return explicitly from visit callback branches. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd6b368ca5
ℹ️ 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".
| function isPathChar(char: string): boolean { | ||
| return /[a-zA-Z0-9._+-]/.test(char); |
There was a problem hiding this comment.
When a valid path contains a Unicode directory or file name after enough ASCII segments, for example /Users/me/デスクトップ/out.txt, isPathChar stops at the first non-ASCII character while parsePathAt has already counted a valid prefix, so the generated link points to /Users/me/ instead of the actual file. This misdirects clicks for localized filenames; either allow Unicode filename characters or avoid linkifying partial prefixes when parsing stops on an unsupported path character.
Useful? React with 👍 / 👎.
| return ( | ||
| <a | ||
| {...props} | ||
| href={undefined} |
There was a problem hiding this comment.
Because the generated file link renders an <a> without an href, it is no longer focusable or activatable via normal keyboard link semantics, so users tabbing through chat cannot open these file paths even though mouse clicks work. Keep a safe href or render a button-like control with the appropriate focus and key handling.
Useful? React with 👍 / 👎.
Allow Unicode letters and numbers in path segments, reject partial matches when a later segment contains unsupported characters, and restore href on file-path links so they remain keyboard accessible. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc5c4d0ed0
ℹ️ 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".
| if (!/^[a-z0-9]+$/.test(word)) return true; | ||
| return ( | ||
| prevToken.length >= 2 && | ||
| prevToken.length <= 3 && | ||
| !prevToken.includes('.') |
There was a problem hiding this comment.
Fresh evidence beyond the earlier spaces fix: when a valid path ends with a numeric/lowercase suffix after a longer token, e.g. Saved /Users/me/Project 2026., this predicate rejects 2026 because Project is longer than 3 chars, so the generated link points at /Users/me/Project instead of the actual path. That makes common year/version-suffixed folders reveal the wrong item; include these continuations with a stronger terminator or avoid emitting a partial link.
Useful? React with 👍 / 👎.
| if (node.type !== 'text' && node.type !== 'inlineCode') { | ||
| return undefined; | ||
| } | ||
| if (index === undefined || !parent || parent.type === 'link') { |
There was a problem hiding this comment.
When authored Markdown uses a reference-style link whose visible text contains a path, such as [log /tmp/out][log], the text node's parent is linkReference, not link, so this guard still replaces that child with an open-file:// link. The result is nested/broken anchors and the original reference link no longer behaves as authored; skip all link-like parents before calling linkifyNode.
Useful? React with 👍 / 👎.
Allow digit-only spaced continuations for folders like "Project 2026" and skip linkification inside linkReference parents to preserve reference-style markdown. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 435732b872
ℹ️ 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".
| if (node.type !== 'text' && node.type !== 'inlineCode') { | ||
| return undefined; | ||
| } | ||
| if (index === undefined || !parent || isLinkLikeParent(parent)) { |
There was a problem hiding this comment.
Fresh evidence beyond the direct-child reference-link case: when an authored link label wraps the path in formatting, e.g. [**/tmp/out**](https://example.com) or [**/tmp/out**][log], the text node's parent is strong, so this immediate-parent check misses the link ancestor and linkifyNode inserts an open-file:// link inside the original link. That produces nested/broken anchors and changes the user's authored target; skip traversal for the whole link/linkReference subtree instead of only checking the immediate parent.
Useful? React with 👍 / 👎.
| while (i < text.length && text[i] === ' ') { | ||
| const continuationEnd = readSpacedContinuation(text, i, start, separator); | ||
| if (continuationEnd === i) break; |
There was a problem hiding this comment.
Fresh evidence beyond the earlier spaces fix: for common duplicate/download names like /Users/me/Downloads/report (1).pdf, readSpacedContinuation stops at the space before (, so the path emitted is only /Users/me/Downloads/report and the click reveals the wrong item while (1).pdf remains plain text. Either support these filename characters in spaced continuations or suppress the partial link when the continuation looks path-like.
Useful? React with 👍 / 👎.
Skip the entire link and linkReference subtrees during traversal so formatted link labels are not linkified, and include download-style suffixes like "report (1).pdf" in matched paths. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7ccad017e
ℹ️ 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".
| if (!segment) { | ||
| if (segmentCount >= minSegments && i < text.length) { | ||
| const next = text[i]; | ||
| if (next !== separator && next !== ' ' && !/[.,;:!?)'\]"]/.test(next)) { |
There was a problem hiding this comment.
When a valid filename contains punctuation that is also treated as prose punctuation, e.g. Saved /tmp/report,final.txt, readSegment stops at the comma and this condition accepts it as a path terminator. The emitted link then targets only /tmp/report while ,final.txt remains plain text, so clicking opens the wrong or nonexistent item; please include such punctuation when more path characters follow, or reject the partial match.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f31ed2a41a
ℹ️ 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".
| if (i >= text.length) return spaceIndex; | ||
|
|
||
| const content = text.slice(contentStart, i); | ||
| if (!/^[a-zA-Z0-9]+$/.test(content)) return spaceIndex; |
There was a problem hiding this comment.
When a filename has a parenthesized suffix containing a space or separator, such as /Users/me/Downloads/report (final copy).pdf or report (final-draft).pdf, this alphanumeric-only guard returns before the following .pdf can prove the parenthetical is part of the filename. readSegment then stops at the space and linkifies only /Users/me/Downloads/report, so clicking reveals the wrong prefix while the actual filename remains plain text.
Useful? React with 👍 / 👎.
Allow spaced and hyphenated parenthetical filename suffixes when an extension follows, while still rejecting short prose parentheticals like (temp) after paths. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3a3484e6f
ℹ️ 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".
| if (/\.[A-Za-z0-9]+$/.test(word) && /^[a-z]/.test(prevToken)) { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
When a spaced filename starts with a capitalized token and the next token is lowercase but carries the extension, such as Saved /tmp/Project notes.txt or Saved /tmp/API response.json, this condition rejects the continuation because prevToken is not lowercase and the fallback only allows very short previous tokens. parsePathAt then treats the space as a boundary and emits /tmp/Project or /tmp/API, so clicking opens the wrong prefix instead of the actual file; an extension-bearing continuation should count as path evidence regardless of the previous token's case.
Useful? React with 👍 / 👎.
| if (/^\d+$/.test(word)) { | ||
| return word.length >= 4; |
There was a problem hiding this comment.
When a complete path is followed by a 4+ digit count or year in prose, for example Created /tmp/out 2026 files, this branch accepts 2026 as a spaced filename segment solely because of its length. The generated link targets /tmp/out 2026 instead of /tmp/out, so common status/count text after paths opens the wrong or nonexistent item; require stronger path evidence before appending bare numeric continuations.
Useful? React with 👍 / 👎.
Accept extension-bearing spaced filename segments regardless of basename case, and only allow bare 4+ digit continuations after capitalized basenames when the path ends or hits sentence punctuation. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f71168a9e8
ℹ️ 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".
| } | ||
|
|
||
| function isFilenamePunctuation(char: string): boolean { | ||
| return char === ',' || char === "'" || char === ':' || char === '?'; |
There was a problem hiding this comment.
When a real filename uses parentheses without a preceding space, such as /tmp/report(1).pdf or /tmp/foo(bar).txt, readSegment stops at ( because this punctuation whitelist does not hand it off to any balanced-parentheses reader. Since ( is not a path boundary, parsePathAt rejects the whole match, leaving these common file paths unclickable even though the spaced parenthetical form is supported.
Useful? React with 👍 / 👎.
Support filenames like report(1).pdf and foo(bar).txt in path detection while rejecting inline prose parentheticals without extensions. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d69f581ef1
ℹ️ 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".
|
|
||
| function isParenthesizedFilenameContent(content: string): boolean { | ||
| if (/^\d+$/.test(content)) return true; | ||
| return /^[a-zA-Z0-9]+(?:[ -][a-zA-Z0-9]+)*$/.test(content); |
There was a problem hiding this comment.
When a filename has a versioned parenthetical suffix such as /tmp/report (v1.2).pdf or /tmp/report (final_v2).pdf, this content check rejects the parenthetical before the following .pdf can prove it belongs to the filename. readSegment then stops at the space and the generated link targets only /tmp/report, leaving the real filename as plain text; allow filename characters like . and _ here when the closing parenthesis is followed by extension/path characters.
Useful? React with 👍 / 👎.
…ffixes Versioned download names like (v1.2) or (final_v2) were rejected by isParenthesizedFilenameContent, so spaced paths stopped at the basename. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c7ac42887
ℹ️ 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".
| } | ||
|
|
||
| function stripLineColumnSuffix(path: string): string { | ||
| const match = path.match(/\.[A-Za-z0-9]+(:\d+(?::\d+)?)$/); |
There was a problem hiding this comment.
Fresh evidence beyond the extension-bearing cases is common diagnostics for extensionless files such as /workspace/Dockerfile:12 or /workspace/Makefile:8:1: readSegment consumes the numeric suffix via colon punctuation, but this regex only strips :line[:column] when the basename has a dot extension. The generated link therefore targets a nonexistent Dockerfile:12-style path instead of the real file, so line suffix handling needs to cover extensionless diagnostic filenames too.
Useful? React with 👍 / 👎.
| if (i > afterParen) { | ||
| return i; |
There was a problem hiding this comment.
Fresh evidence in the current code is that a sentence period after a word parenthetical counts as isPathChar, so Created /tmp/out (temporary). is accepted as /tmp/out (temporary) and the final period is stripped later. That makes ordinary explanatory parentheticals at sentence boundaries open the wrong/nonexistent path; only treat the parenthetical as part of the filename when the following characters prove an actual extension or filename continuation.
Useful? React with 👍 / 👎.
Strip line:column suffixes from extensionless diagnostic filenames like Dockerfile and Makefile, and require real extension evidence after word parentheticals so prose like "(temporary)." is not absorbed. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 032955f478
ℹ️ 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".
| while (i >= 0 && /[a-zA-Z0-9_.$-]/.test(text[i])) { | ||
| i--; | ||
| } | ||
| return !(i >= 0 && (text[i] === '?' || text[i] === '&')); |
There was a problem hiding this comment.
When a local-looking path appears inside inline code or other non-autolinked URL text such as https://host/page#file=/tmp/out or https://host/download;file=/tmp/out, this guard accepts the slash after = because it only rejects ? and &. The path linkifier then inserts an open-file link into the URL text, corrupting the URL and making the embedded value reveal /tmp/out; treat fragment/path-parameter delimiters like query delimiters before allowing assignment-style starts.
Useful? React with 👍 / 👎.
|
|
||
| const lastSep = Math.max(path.lastIndexOf('/'), path.lastIndexOf('\\')); | ||
| const basename = path.slice(lastSep + 1); | ||
| const lineColMatch = basename.match(/^([A-Z][A-Za-z0-9_-]*)(:\d+(?::\d+)?)$/); |
There was a problem hiding this comment.
Fresh evidence in this revision is that the extensionless fallback only matches basenames starting with an uppercase letter. Diagnostics for lowercase extensionless files, for example this repo's crates/goose-sdk/justfile:12, keep :12 in the matched path after readSegment consumes the colon digits, so the generated link targets a nonexistent justfile:12 instead of the actual file.
Useful? React with 👍 / 👎.
…ic suffixes Treat # and ; like query delimiters before assignment-style path starts so paths in URL fragments and path parameters are not linkified. Strip :line suffixes from known lowercase extensionless diagnostic filenames such as justfile and dockerfile without affecting colon-in-filename cases like snapshot:1. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a321ea3042
ℹ️ 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".
| if (after !== undefined && /\s/.test(after)) { | ||
| if (after === ' ') { | ||
| const rest = text.slice(j).trimStart(); | ||
| if (rest.startsWith(separator)) return spaceIndex; | ||
| } | ||
| return isSpacedFilenameContinuation(word, prevToken, text, j) ? j : spaceIndex; |
There was a problem hiding this comment.
For common paths such as C:\Program Files (x86)\Goose, the continuation after Files reaches this whitespace branch with rest starting (x86)\Goose, not the separator, so it falls back to the capitalization heuristics and rejects Files because the previous token Program is longer than three characters. The match is then emitted as C:\Program, so clicking the generated link reveals the wrong prefix; treat a following parenthesized directory suffix before the next separator as evidence to continue the segment.
Useful? React with 👍 / 👎.
Accept spaced path continuations when a parenthesized directory suffix like (x86) or (Beta) appears before the next separator, so Windows and Unix paths such as C:\Program Files (x86)\Goose linkify fully. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc1dd15100
ℹ️ 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".
| while (i >= 0 && /[a-zA-Z0-9_.$-]/.test(text[i])) { | ||
| i--; | ||
| } | ||
| return !(i >= 0 && URL_PARAM_DELIMITERS.has(text[i])); |
There was a problem hiding this comment.
Fresh evidence in the current delimiter check is that URL query keys containing common bracket/encoding characters are still treated as assignments. In inline code like https://host/download?file[]=/tmp/out or https://host/download?file%5B%5D=/tmp/out, this scan stops on ] or % instead of reaching the preceding ?, so isAssignmentEqualsStart returns true and the linkifier inserts an open-file:// link into the URL value. This is the same URL-query context the tests are trying to suppress; keep scanning valid parameter-key characters or otherwise detect URL context before allowing the = case.
Useful? React with 👍 / 👎.
| } | ||
| if (j === i) return false; | ||
| const word = text.slice(i, j).replace(TRAILING_PUNCTUATION_RE, ''); | ||
| if (hasFileExtension(word) && /^[a-z]/.test(word)) return true; |
There was a problem hiding this comment.
When a multi-word filename has an intermediate lowercase segment and the extension-bearing segment starts uppercase, e.g. Saved /tmp/project notes Draft.txt, this condition rejects the lookahead solely because Draft.txt is not lowercase. readSpacedContinuation then stops at notes and parsePathAt emits only /tmp/project, so clicks reveal the wrong prefix even though the .txt extension proves the remaining words are part of the filename.
Useful? React with 👍 / 👎.
…ahead Extend query-param key scanning in isAssignmentEqualsStart to include brackets and percent-encoding so paths in values like file[]=/tmp/out are not linkified. Accept extension-bearing spaced words regardless of case in readExtensionWordAhead to match isPathLikeSpacedWord behavior. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c9823ef9ec
ℹ️ 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".
| /^[a-z][a-z0-9]*$/.test(word) && | ||
| /^[a-z]/.test(prevToken) && | ||
| readExtensionWordAhead(text, endIndex) |
There was a problem hiding this comment.
When a spaced filename starts with a capitalized token and the extension appears on a later word, e.g. Saved /tmp/Project notes draft.txt or Saved /tmp/API response data.json, this lookahead is the only branch that can use the later .txt/.json as evidence, but it requires the previous token to be lowercase. The first continuation word is rejected, so parsePathAt emits only /tmp/Project or /tmp/API; clicking the generated link opens the wrong prefix even though the later extension proves the full filename.
Useful? React with 👍 / 👎.
Let lowercase spaced filename words continue after titlecase or acronym basenames when readExtensionWordAhead proves a later extension, matching two-word cases already handled via hasFileExtension. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ad9db5af90
ℹ️ 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".
| } | ||
|
|
||
| function isQueryParamKeyChar(char: string): boolean { | ||
| return /[a-zA-Z0-9_.$\[\]%-]/.test(char); |
There was a problem hiding this comment.
Fresh evidence in the current URL guard is that hyphens are not treated as query-key characters, so a URL such as https://host/download?file-name=/tmp/out stops scanning at - and isAssignmentEqualsStart allows the /tmp/out value to become an open-file:// link. Hyphenated parameter names are common, and this corrupts inline URL text in the same query-value context the surrounding checks are trying to suppress; include - in the backwards scan or otherwise detect the URL delimiter before accepting the assignment case.
Useful? React with 👍 / 👎.
Move hyphen to the start of the isQueryParamKeyChar character class so it cannot be misread as a range operator, and add tests for hyphenated query keys like file-name= and content-type=. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d5619ded04
ℹ️ 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".
|
|
||
| function isParenthesizedFilenameContent(content: string): boolean { | ||
| if (/^\d+$/.test(content)) return true; | ||
| return /^[a-zA-Z0-9._]+(?:[ -][a-zA-Z0-9._]+)*$/.test(content); |
There was a problem hiding this comment.
When a parenthesized filename suffix contains characters the parser already supports elsewhere, such as an apostrophe or non-ASCII text, this ASCII-only guard rejects the suffix before the following extension can prove it is part of the filename. For example, Saved /tmp/report (John's copy).pdf or Saved /tmp/report (最終).pdf gets linkified only as /tmp/report, so clicking opens the wrong prefix while the actual file path remains plain text.
Useful? React with 👍 / 👎.
Expand isParenthesizedFilenameContent to accept characters already valid in path segments via isPathChar, plus spaces, hyphens, and apostrophes. Extension evidence in readParenthesizedContent still blocks prose like (temporary) without a real filename suffix. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
Remove unnecessary \[ escape in isQueryParamKeyChar regex and extend parenthesized filename detection for apostrophes and Unicode content. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7aad701d84
ℹ️ 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".
| prevToken.length >= 2 && | ||
| prevToken.length <= 2 && |
There was a problem hiding this comment.
Fresh evidence in this revision is that this fallback still accepts any 4+ letter lowercase word when the previous path token is exactly two characters. In messages like Created /tmp/ui successfully. or Check /tmp/go output, the generated link targets /tmp/ui successfully or /tmp/go output instead of the completed short path, so common status prose after short directories opens the wrong item; require stronger filename evidence such as an extension or following separator before taking this branch.
Useful? React with 👍 / 👎.
Require extension, separator, or extension-ahead evidence before the two-char basename spaced-word fallback, and keep determiner-led folder names like "my documents" working via an explicit rule. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 535be0762a
ℹ️ 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".
| while (i >= 0 && isQueryParamKeyChar(text[i])) { | ||
| i--; | ||
| } | ||
| return !(i >= 0 && URL_PARAM_DELIMITERS.has(text[i])); |
There was a problem hiding this comment.
When a non-autolinked URL has an equals-valued path segment, such as inline code https://host/download/file=/tmp/out or bare text example.com/download/file=/tmp/out, this scan stops at the preceding / and returns true because / is not treated as URL context. The path linkifier then inserts an open-file:// link into the URL text, corrupting the displayed URL and making the value look like a local file path; detect slash/domain URL context before accepting assignment-style starts.
Useful? React with 👍 / 👎.
| return true; | ||
| } | ||
| return ( | ||
| /^[a-z][a-z0-9]*$/.test(word) && |
There was a problem hiding this comment.
When a spaced filename has a supported filename character in an intermediate word and the extension appears later, e.g. Saved /tmp/project release-notes draft.txt or Saved /tmp/project release_notes draft.txt, this alphanumeric-only check rejects the first continuation before the later .txt evidence can be used. findPaths then emits only /tmp/project, so the generated link opens the wrong prefix for valid filenames that otherwise use characters this parser already accepts.
Useful? React with 👍 / 👎.
Reject linkifying paths after = when the slash belongs to a URL path segment, and allow hyphens/underscores in intermediate spaced filename words when extension lookahead succeeds. Signed-off-by: Denis <i@deenyy.ru> Co-authored-by: Cursor <cursoragent@cursor.com>
|
Thanks for all the work on this, and genuinely for being so responsive to the review — you fixed the P1 and the test issue quickly, and stuck with it through a lot of codex rounds. That said, watching what codex keeps surfacing on every commit (line:col suffixes, If you (or anyone) wants to take another run at it, here is a direction that sidesteps almost all of this:
Because the extension anchors the right edge, you never have to guess where the path ends — which is what causes 80% of the edge cases here. The only remaining judgement is how far left to go across spaces, and even there a simple default (don't cross spaces, or only cross while the previous token is path-chars-only) terminates cleanly with no per-case heuristics. That should collapse most of this ~600-line file and make it robust by construction. Please feel free to open a fresh PR (or reopen) if you land on a more scalable solution along those lines — I'd be happy to look. Thanks again! 🙏 |
2 participants
Summary
Make file paths in chat messages clickable in the desktop app. A remark plugin detects Unix (
/home/user/file.rs), tilde (~/.config/app/settings.json), and Windows (C:\Users\dev\project\index.ts) paths in markdown text and inline code, and renders them asopen-file://links. Clicking a path reveals the file in Finder/Explorer viashell.showItemInFolder().Rebased onto latest
main; duplicate scrollbar commit from #9601 removed.Testing
pnpm test -- src/utils/linkifyPaths.test.ts— 14 unit tests (path detection, edge cases, trailing punctuation)Related Issues
N/A — happy to open a discussion/issue if you'd prefer to track this separately.
Screenshots/Demos (for UX changes)
Before: file paths in chat are plain text, not clickable.
After: paths render as links; clicking opens the file location in the system file manager.
Screenshots/Demos (for UX changes)
Before:
After: