Skip to content

Commit 2d58a39

Browse files
roblourensroblourens
authored
Merge pull request microsoft#210281 from microsoft/roblou/unacceptable-swift
Fix chat MarkdownString merging
2 parents d30b358 + cebce51 commit 2d58a39

File tree

8 files changed

+86
-27
lines changed

8 files changed

+86
-27
lines changed

‎src/vs/workbench/api/common/extHostTypes.ts

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4272,6 +4272,10 @@ export enum ChatResultFeedbackKind {
42724272
export class ChatResponseMarkdownPart {
42734273
value: vscode.MarkdownString;
42744274
constructor(value: string | vscode.MarkdownString) {
4275+
if (typeof value !== 'string' && value.isTrusted === true) {
4276+
throw new Error('The boolean form of MarkdownString.isTrusted is NOT supported for chat participants.');
4277+
}
4278+
42754279
this.value = typeof value === 'string' ? new MarkdownString(value) : value;
42764280
}
42774281
}
@@ -4284,6 +4288,10 @@ export class ChatResponseMarkdownWithVulnerabilitiesPart {
42844288
value: vscode.MarkdownString;
42854289
vulnerabilities: vscode.ChatVulnerability[];
42864290
constructor(value: string | vscode.MarkdownString, vulnerabilities: vscode.ChatVulnerability[]) {
4291+
if (typeof value !== 'string' && value.isTrusted === true) {
4292+
throw new Error('The boolean form of MarkdownString.isTrusted is NOT supported for chat participants.');
4293+
}
4294+
42874295
this.value = typeof value === 'string' ? new MarkdownString(value) : value;
42884296
this.vulnerabilities = vulnerabilities;
42894297
}

‎src/vs/workbench/contrib/chat/browser/chatListRenderer.ts

Lines changed: 0 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -953,14 +953,6 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
953953
private renderMarkdown(markdown: IMarkdownString, element: ChatTreeItem, templateData: IChatListItemTemplate, fillInIncompleteTokens = false): IMarkdownRenderResult {
954954
const disposables = new DisposableStore();
955955

956-
markdown = new MarkdownString(markdown.value, {
957-
isTrusted: {
958-
// Disable all other config options except isTrusted
959-
enabledCommands: typeof markdown.isTrusted === 'object' ? markdown.isTrusted?.enabledCommands : [] ?? []
960-
},
961-
supportHtml: true
962-
});
963-
964956
// We release editors in order so that it's more likely that the same editor will be assigned if this element is re-rendered right away, like it often is during progressive rendering
965957
const orderedDisposablesList: IDisposable[] = [];
966958
const codeblocks: IChatCodeBlockInfo[] = [];

‎src/vs/workbench/contrib/chat/common/annotations.ts

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ import { MarkdownString } from 'vs/base/common/htmlContent';
66
import { basename } from 'vs/base/common/resources';
77
import { URI } from 'vs/base/common/uri';
88
import { IRange } from 'vs/editor/common/core/range';
9-
import { IChatProgressRenderableResponseContent, IChatProgressResponseContent } from 'vs/workbench/contrib/chat/common/chatModel';
9+
import { IChatProgressRenderableResponseContent, IChatProgressResponseContent, canMergeMarkdownStrings } from 'vs/workbench/contrib/chat/common/chatModel';
1010
import { IChatAgentMarkdownContentWithVulnerability, IChatAgentVulnerabilityDetails, IChatContentInlineReference, IChatMarkdownContent } from 'vs/workbench/contrib/chat/common/chatService';
1111

1212
export const contentRefUrl = 'http://_vscodecontentref_'; // must be lowercase for URI
@@ -20,16 +20,17 @@ export function annotateSpecialMarkdownContent(response: ReadonlyArray<IChatProg
2020
const printUri = URI.parse(contentRefUrl).with({ fragment: JSON.stringify(location) });
2121
const markdownText = `[${item.name || basename(location.uri)}](${printUri.toString()})`;
2222
if (previousItem?.kind === 'markdownContent') {
23-
result[result.length - 1] = { content: new MarkdownString(previousItem.content.value + markdownText, { isTrusted: previousItem.content.isTrusted }), kind: 'markdownContent' };
23+
result[result.length - 1] = { content: new MarkdownString(previousItem.content.value + markdownText), kind: 'markdownContent' };
2424
} else {
2525
result.push({ content: new MarkdownString(markdownText), kind: 'markdownContent' });
2626
}
27-
} else if (item.kind === 'markdownContent' && previousItem?.kind === 'markdownContent') {
28-
result[result.length - 1] = { content: new MarkdownString(previousItem.content.value + item.content.value, { isTrusted: previousItem.content.isTrusted }), kind: 'markdownContent' };
27+
} else if (item.kind === 'markdownContent' && previousItem?.kind === 'markdownContent' && canMergeMarkdownStrings(previousItem.content, item.content)) {
28+
result[result.length - 1] = { content: new MarkdownString(previousItem.content.value + item.content.value), kind: 'markdownContent' };
2929
} else if (item.kind === 'markdownVuln') {
3030
const vulnText = encodeURIComponent(JSON.stringify(item.vulnerabilities));
3131
const markdownText = `<vscode_annotation details='${vulnText}'>${item.content.value}</vscode_annotation>`;
3232
if (previousItem?.kind === 'markdownContent') {
33+
// Since this is inside a codeblock, it needs to be merged into the previous markdown content.
3334
result[result.length - 1] = { content: new MarkdownString(previousItem.content.value + markdownText, { isTrusted: previousItem.content.isTrusted }), kind: 'markdownContent' };
3435
} else {
3536
result.push({ content: new MarkdownString(markdownText), kind: 'markdownContent' });
@@ -101,4 +102,3 @@ export function extractVulnerabilitiesFromText(text: string): { newText: string;
101102

102103
return { newText, vulnerabilities };
103104
}
104-

‎src/vs/workbench/contrib/chat/common/chatModel.ts

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { Emitter, Event } from 'vs/base/common/event';
99
import { IMarkdownString, MarkdownString, isMarkdownString } from 'vs/base/common/htmlContent';
1010
import { Disposable } from 'vs/base/common/lifecycle';
1111
import { revive } from 'vs/base/common/marshalling';
12+
import { equals } from 'vs/base/common/objects';
1213
import { basename, isEqual } from 'vs/base/common/resources';
1314
import { ThemeIcon } from 'vs/base/common/themables';
1415
import { URI, UriComponents, UriDto, isUriComponents } from 'vs/base/common/uri';
@@ -164,22 +165,19 @@ export class Response implements IResponse {
164165
const responsePartLength = this._responseParts.length - 1;
165166
const lastResponsePart = this._responseParts[responsePartLength];
166167

167-
if (!lastResponsePart || lastResponsePart.kind !== 'markdownContent') {
168-
// The last part can't be merged with
168+
if (!lastResponsePart || lastResponsePart.kind !== 'markdownContent' || !canMergeMarkdownStrings(lastResponsePart.content, progress.content)) {
169+
// The last part can't be merged with- not markdown, or markdown with different permissions
169170
this._responseParts.push(progress);
170171
} else {
171-
// Merge all enabled commands TODO
172-
const lastPartEnabledCommands = typeof lastResponsePart.content.isTrusted === 'object' ?
173-
lastResponsePart.content.isTrusted.enabledCommands :
174-
[];
175-
const thisPartEnabledCommands = typeof progress.content.isTrusted === 'object' ?
176-
progress.content.isTrusted.enabledCommands :
177-
[];
178-
const enabledCommands = [...lastPartEnabledCommands, ...thisPartEnabledCommands];
179-
this._responseParts[responsePartLength] = { content: new MarkdownString(lastResponsePart.content.value + progress.content.value, { isTrusted: { enabledCommands } }), kind: 'markdownContent' };
172+
lastResponsePart.content = {
173+
value: lastResponsePart.content.value + progress.content.value,
174+
isTrusted: lastResponsePart.content.isTrusted,
175+
supportThemeIcons: lastResponsePart.content.supportThemeIcons,
176+
supportHtml: lastResponsePart.content.supportHtml,
177+
baseUri: lastResponsePart.content.baseUri
178+
} satisfies IMarkdownString;
180179
}
181180
this._updateRepr(quiet);
182-
183181
} else if (progress.kind === 'textEdit') {
184182
if (progress.edits.length > 0) {
185183
// merge text edits for the same file no matter when they come in
@@ -923,3 +921,22 @@ export function updateRanges(variableData: IChatRequestVariableData, diff: numbe
923921
}))
924922
};
925923
}
924+
925+
export function canMergeMarkdownStrings(md1: IMarkdownString, md2: IMarkdownString): boolean {
926+
if (md1.baseUri && md2.baseUri) {
927+
const baseUriEquals = md1.baseUri.scheme === md2.baseUri.scheme
928+
&& md1.baseUri.authority === md2.baseUri.authority
929+
&& md1.baseUri.path === md2.baseUri.path
930+
&& md1.baseUri.query === md2.baseUri.query
931+
&& md1.baseUri.fragment === md2.baseUri.fragment;
932+
if (!baseUriEquals) {
933+
return false;
934+
}
935+
} else if (md1.baseUri || md2.baseUri) {
936+
return false;
937+
}
938+
939+
return equals(md1.isTrusted, md2.isTrusted) &&
940+
md1.supportHtml === md2.supportHtml &&
941+
md1.supportThemeIcons === md2.supportThemeIcons;
942+
}

‎src/vs/workbench/contrib/chat/test/common/__snapshots__/Response_mergeable_markdown.0.snap

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
[
2+
{
3+
content: {
4+
value: "markdown1markdown2",
5+
isTrusted: false,
6+
supportThemeIcons: false,
7+
supportHtml: false,
8+
baseUri: undefined
9+
},
10+
kind: "markdownContent"
11+
}
12+
]

‎src/vs/workbench/contrib/chat/test/common/__snapshots__/Response_not_mergeable_markdown.0.snap

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
[
2+
{
3+
content: {
4+
value: "markdown1",
5+
isTrusted: false,
6+
supportThemeIcons: false,
7+
supportHtml: true
8+
},
9+
kind: "markdownContent"
10+
},
11+
{
12+
content: {
13+
value: "markdown2",
14+
isTrusted: false,
15+
supportThemeIcons: false,
16+
supportHtml: false
17+
},
18+
kind: "markdownContent"
19+
}
20+
]

‎src/vs/workbench/contrib/chat/test/common/chatModel.test.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -126,7 +126,7 @@ suite('ChatModel', () => {
126126
suite('Response', () => {
127127
ensureNoDisposablesAreLeakedInTestSuite();
128128

129-
test('markdown, markdown', async () => {
129+
test('mergeable markdown', async () => {
130130
const response = new Response([]);
131131
response.updateContent({ content: new MarkdownString('markdown1'), kind: 'markdownContent' });
132132
response.updateContent({ content: new MarkdownString('markdown2'), kind: 'markdownContent' });
@@ -135,6 +135,15 @@ suite('Response', () => {
135135
assert.strictEqual(response.asString(), 'markdown1markdown2');
136136
});
137137

138+
test('not mergeable markdown', async () => {
139+
const response = new Response([]);
140+
const md1 = new MarkdownString('markdown1');
141+
md1.supportHtml = true;
142+
response.updateContent({ content: md1, kind: 'markdownContent' });
143+
response.updateContent({ content: new MarkdownString('markdown2'), kind: 'markdownContent' });
144+
await assertSnapshot(response.value);
145+
});
146+
138147
test('inline reference', async () => {
139148
const response = new Response([]);
140149
response.updateContent({ content: new MarkdownString('text before'), kind: 'markdownContent' });

‎src/vscode-dts/vscode.proposed.chatParticipant.d.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -309,7 +309,7 @@ declare module 'vscode' {
309309
* `push(new ChatResponseMarkdownPart(value))`.
310310
*
311311
* @see {@link ChatResponseStream.push}
312-
* @param value A markdown string or a string that should be interpreted as markdown.
312+
* @param value A markdown string or a string that should be interpreted as markdown. The boolean form of {@link MarkdownString.isTrusted} is NOT supported.
313313
* @returns This stream.
314314
*/
315315
markdown(value: string | MarkdownString): ChatResponseStream;
@@ -373,6 +373,7 @@ declare module 'vscode' {
373373
}
374374

375375
export class ChatResponseMarkdownPart {
376+
/** Note: The boolean form of {@link MarkdownString.isTrusted} is NOT supported. */
376377
value: MarkdownString;
377378
constructor(value: string | MarkdownString);
378379
}

0 commit comments

Comments
 (0)