Improve copying chat output to external apps - #328466
Conversation
There was a problem hiding this comment.
Pull request overview
Improves chat output portability when copying to external applications and reduces incorrect file/symbol links.
Changes:
- Converts local/internal links and images to inline code during copy or paste.
- Adds reusable Markdown link rewriting and copy sanitization.
- Avoids ambiguous or false-positive Copilot linkification.
Show a summary per file
| File | Description |
|---|---|
src/vs/workbench/contrib/chat/test/common/model/chatModel.test.ts |
Tests portable inline references. |
src/vs/workbench/contrib/chat/test/browser/widget/chatClipboard.test.ts |
Tests clipboard sanitization. |
src/vs/workbench/contrib/chat/test/browser/widget/__snapshots__/ChatMarkdownRenderer_supportHtml_with_one-line_markdown.1.snap |
Updates external-link rendering snapshot. |
src/vs/workbench/contrib/chat/common/model/chatModel.ts |
Formats file references as code. |
src/vs/workbench/contrib/chat/browser/widget/chatListWidget.ts |
Sanitizes rich-text copy selections. |
src/vs/workbench/contrib/chat/browser/widget/chatClipboard.ts |
Implements portable HTML and Markdown conversion. |
src/vs/workbench/contrib/chat/browser/agentSessions/agentHost/stateToProgressAdapter.ts |
Reuses Markdown rewriting utility. |
src/vs/workbench/contrib/chat/browser/actions/chatCopyActions.ts |
Sanitizes chat copy actions. |
src/vs/base/test/common/markdownLinks.test.ts |
Tests source-aware link rewriting. |
src/vs/base/test/browser/markdownRenderer.test.ts |
Tests copy-safe rendered links. |
src/vs/base/test/browser/htmlToMarkdown.test.ts |
Tests internal-link conversion. |
src/vs/base/common/markdownLinks.ts |
Adds Markdown rewriting infrastructure. |
src/vs/base/common/htmlContent.ts |
Classifies portable link targets. |
src/vs/base/browser/markdownRenderer.ts |
Restores safe external href values. |
src/vs/base/browser/htmlToMarkdown.ts |
Converts internal HTML links to code. |
extensions/copilot/src/extension/linkify/vscode-node/symbolLinkifier.ts |
Rejects ambiguous symbol resources. |
extensions/copilot/src/extension/linkify/vscode-node/findWord.ts |
Tightens qualified-symbol matching. |
extensions/copilot/src/extension/linkify/vscode-node/findSymbol.ts |
Parses qualified symbol names. |
extensions/copilot/src/extension/linkify/test/vscode-node/findSymbol.test.ts |
Tests qualified-name parsing. |
extensions/copilot/src/extension/linkify/test/node/util.ts |
Adds multi-root test setup. |
extensions/copilot/src/extension/linkify/test/node/filePathLinkifier.spec.ts |
Tests false-positive and ambiguous paths. |
extensions/copilot/src/extension/linkify/common/modelFilePathLinkifier.ts |
Rejects ambiguous model paths. |
extensions/copilot/src/extension/linkify/common/linkifiedText.ts |
Adds single-resource matching. |
extensions/copilot/src/extension/linkify/common/filePathLinkifier.ts |
Tightens file and directory linkification. |
Review details
Suppressed comments (1)
src/vs/workbench/contrib/chat/browser/widget/chatClipboard.ts:115
- This fallback searches the entire source for every matching destination, including code spans and fenced blocks. When an unlocatable multiline link shares a target with a literal example in code, both occurrences are changed to
](), contradicting the helper's guarantee that code remains byte-for-byte intact. Exclude matches overlappingcodeRanges.
for (let at = source.indexOf(`](${target})`); at >= 0; at = source.indexOf(`](${target})`, at + 1)) {
edits.push({ start: at, end: at + `](${target})`.length, replacement: ']()' });
}
- Files reviewed: 24/24 changed files
- Comments generated: 4
- Review effort level: Balanced
|
Base:
|
There was a problem hiding this comment.
Review details
Suppressed comments (6)
src/vs/base/common/markdownLinks.ts:139
findDefinitionRangesonly examines gaps between top-level tokens. Marked can consume a reference definition inside a blockquote or list item within the enclosing token'sraw, so that definition is never reported here. For example,> [x][r]\n>\n> [r]: /Users/me/private.tsrewrites the reference but leaves the private definition on the clipboard. Definition discovery needs to account for nested container source ranges while still excluding code.
function findDefinitionRanges(tokens: readonly marked.Token[], markdown: string): { start: number; end: number }[] {
const ranges: { start: number; end: number }[] = [];
let cursor = 0;
for (const token of tokens) {
src/vs/workbench/contrib/chat/browser/widget/chatClipboard.ts:113
- This fallback assumes the destination appears exactly as
](${target}), buttoken.hrefis parser-normalized and omits source syntax such as angle brackets, escapes, and titles. An unlocatable link such as a multiline blockquote label followed by(/Users/me/a.ts "title")is therefore left unchanged and still exposes the local path. Locate the original destination suffix/range rather than synthesizing it fromhref.
const needle = `](${target})`;
for (let at = source.indexOf(needle); at >= 0; at = source.indexOf(needle, at + 1)) {
if (!overlapsCode(at, at + needle.length, codeRanges)) {
edits.push({ start: at, end: at + needle.length, replacement: ']()' });
}
}
src/vs/workbench/contrib/chat/browser/widget/chatClipboard.ts:92
- These parser-decoded fields are interpolated back into Markdown without escaping. For example, a portable reference label such as
[a\]b][r]loses its source escape and becomes malformed[a]b](https://...); escaped quotes in a definition title have the same problem. Serialize the label, destination, and title with Markdown-safe escaping instead of inserting normalized token fields directly.
if (!token.raw.includes(target)) {
const title = token.title ? ` "${token.title}"` : '';
return `${token.type === 'image' ? '!' : ''}[${token.text}](${target}${title})`;
src/vs/base/common/markdownLinks.ts:104
- When an opaque code token cannot be located, it is silently skipped and never added to
codeRanges. This occurs for fenced code nested in a blockquote because marked strips the>prefixes from the child token'sraw; the caller's fallback source scan can then rewrite a matching](local-path)inside that fence. Track unlocatable code regions or avoid source-wide fallback edits so code remains byte-for-byte intact as promised.
This issue also appears on line 136 of the same file.
const start = markdown.indexOf(raw, cursor);
if (start < 0) {
if (token.type === 'link' || token.type === 'image') {
unlocatable.push(token as marked.Tokens.Link | marked.Tokens.Image);
}
continue;
src/vs/workbench/contrib/chat/browser/widget/chatClipboard.ts:119
- Dropping every definition breaks portable reference links whose token could not be located. A multiline reference label inside a blockquote/list is added to
unlocatable; itshttpstarget is intentionally not scrubbed, but this line still deletes the definition, leaving an unresolved literal reference after copy. Preserve definitions that are still needed by unlocatable portable links.
// A definition holds its target in source the reader never saw, so leaving one behind
// would publish the very path this strips. The links that leaned on them now spell
// their targets out, so the definitions have nothing left to say.
edits.push(...definitionRanges.map(range => ({ ...range, replacement: '' })));
extensions/copilot/src/extension/linkify/common/linkifiedText.ts:54
- Using
Uri.toString()makes the uniqueness check path-case-sensitive. On case-insensitive file systems, candidates or references that differ only in path casing identify the same file but are treated as ambiguous, so linkification is incorrectly suppressed. Use a platform-appropriate comparison key fromextensions/copilot/src/util/vs/base/common/resources.ts(seeextUriBiasedIgnorePathCaseat lines 342-345).
return new Set(matches.map(match => match.toString())).size === 1 ? matches[0] : undefined;
- Files reviewed: 24/24 changed files
- Comments generated: 0 new
- Review effort level: Balanced
|
I appreciate the work done to address this issue. At first I wasn't sure why it was so large, but a review indicated that it touches when chat text is turned into links, how those links are rendered, and how they are converted during copy and paste. I did an independent review (which was quite extensive):
As mentioned there was one bug that cropped up. I provide it below, courtesy of Claude Open 5 (in conjunction with GPT-5 Sol). Otherwise they tell me this looks good — thanks! Bug: Nested images inside non-portable links are dropped entirely.
toPortableMarkdown('[](/repo/a.ts)') // '`img`'The two HTML paths added here don't. Both derive the replacement label from // src/vs/base/test/browser/htmlToMarkdown.test.ts
convertHtmlToMarkdown('<a href="file:///repo/a.ts"><img alt="diagram" src="https://example.com/d.png"></a>')
// actual: ''
// expected: ''
// src/vs/workbench/contrib/chat/test/browser/widget/chatClipboard.test.ts
sanitizeToHtml('<a href="" data-href="file:///repo/a.ts"><img alt="diagram" src="https://example.com/d.png"></a>')
// actual: ''
// expected: '<img alt="diagram" src="https://example.com/d.png">'Note the image target is portable in both cases — a shareable In if (!target || !isPortableMarkdownTarget(target)) {
return plainText ? appendEscapedMarkdownInlineCode(plainText) : text;
}In if (!label.trim()) {
element.replaceWith(...element.childNodes);
return;
}Neither breaks an existing test — an empty anchor has no children, so Narrow in practice, since chat disallows remote images and browsers usually resolve hrefs before the clipboard — the realistic trigger is pasting from a local HTML file or the markdown preview. |
|
Here is a status of how this PR relates to other ticket, courtesy of Claude Opus 5 (with the help of GPT-5.6 Sol): Link-issue status after PR #328466As of 2026-08-05. PR merged, milestone 1.133.0, not yet released. Fixed by this PR
Both open entries are candidates for closing, or at least for verification against Narrowed, still open
Contained, not fixed
Partially addressed
Untouched
PatternLinks leaking out broken is now handled comprehensively. Links existing when they Open defect in this PRAn image nested inside a non-portable link is discarded along with its alt text, in |
Fix #328212