Skip to content

Commit 7ac9316

Browse files
TyriarTyriar
authored
Merge pull request microsoft#272060 from microsoft/tyriar/hover_auto_approve
Move auto approval messages into hover
2 parents 111fec7 + 7a29528 commit 7ac9316

File tree

10 files changed

+83
-43
lines changed

10 files changed

+83
-43
lines changed

‎src/vs/workbench/contrib/chat/browser/chatContentParts/chatMcpServersInteractionContentPart.ts‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -141,7 +141,8 @@ export class ChatMcpServersInteractionContentPart extends Disposable implements
141141
this.context,
142142
true, // forceShowSpinner
143143
true, // forceShowMessage
144-
undefined // icon
144+
undefined, // icon
145+
undefined, // toolInvocation
145146
));
146147
this.domNode.appendChild(this.workingProgressPart.domNode);
147148
}

‎src/vs/workbench/contrib/chat/browser/chatContentParts/chatProgressContentPart.ts‎

Lines changed: 57 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -6,21 +6,23 @@
66
import { $, append } from '../../../../../base/browser/dom.js';
77
import { alert } from '../../../../../base/browser/ui/aria/aria.js';
88
import { Codicon } from '../../../../../base/common/codicons.js';
9-
import { MarkdownString } from '../../../../../base/common/htmlContent.js';
9+
import { markdownCommandLink, MarkdownString, type IMarkdownString } from '../../../../../base/common/htmlContent.js';
1010
import { Disposable, MutableDisposable } from '../../../../../base/common/lifecycle.js';
1111
import { ThemeIcon } from '../../../../../base/common/themables.js';
1212
import { IMarkdownRenderer } from '../../../../../platform/markdown/browser/markdownRenderer.js';
1313
import { IRenderedMarkdown } from '../../../../../base/browser/markdownRenderer.js';
1414
import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js';
1515
import { localize } from '../../../../../nls.js';
16-
import { IChatProgressMessage, IChatTask, IChatTaskSerialized } from '../../common/chatService.js';
16+
import { IChatProgressMessage, IChatTask, IChatTaskSerialized, IChatToolInvocation, IChatToolInvocationSerialized, ToolConfirmKind } from '../../common/chatService.js';
1717
import { IChatRendererContent, isResponseVM } from '../../common/chatViewModel.js';
1818
import { ChatTreeItem } from '../chat.js';
1919
import { renderFileWidgets } from '../chatInlineAnchorWidget.js';
2020
import { IChatContentPart, IChatContentPartRenderContext } from './chatContentParts.js';
2121
import { IChatMarkdownAnchorService } from './chatMarkdownAnchorService.js';
2222
import { IConfigurationService } from '../../../../../platform/configuration/common/configuration.js';
2323
import { AccessibilityWorkbenchSettingId } from '../../../accessibility/browser/accessibilityConfiguration.js';
24+
import { IHoverService } from '../../../../../platform/hover/browser/hover.js';
25+
import { HoverStyle } from '../../../../../base/browser/ui/hover/hover.js';
2426

2527
export class ChatProgressContentPart extends Disposable implements IChatContentPart {
2628
public readonly domNode: HTMLElement;
@@ -36,6 +38,7 @@ export class ChatProgressContentPart extends Disposable implements IChatContentP
3638
forceShowSpinner: boolean | undefined,
3739
forceShowMessage: boolean | undefined,
3840
icon: ThemeIcon | undefined,
41+
private readonly toolInvocation: IChatToolInvocation | IChatToolInvocationSerialized | undefined,
3942
@IInstantiationService private readonly instantiationService: IInstantiationService,
4043
@IChatMarkdownAnchorService private readonly chatMarkdownAnchorService: IChatMarkdownAnchorService,
4144
@IConfigurationService private readonly configurationService: IConfigurationService
@@ -61,11 +64,9 @@ export class ChatProgressContentPart extends Disposable implements IChatContentP
6164
result.element.classList.add('progress-step');
6265
renderFileWidgets(result.element, this.instantiationService, this.chatMarkdownAnchorService, this._store);
6366

64-
this.domNode = $('.progress-container');
65-
const iconElement = $('div');
66-
iconElement.classList.add(...ThemeIcon.asClassNameArray(codicon));
67-
append(this.domNode, iconElement);
68-
append(this.domNode, result.element);
67+
const tooltip: IMarkdownString | undefined = this.createApprovalMessage();
68+
const progressPart = this._register(instantiationService.createInstance(ChatProgressSubPart, result.element, codicon, tooltip));
69+
this.domNode = progressPart.domNode;
6970
this.renderedMessage.value = result;
7071
}
7172

@@ -100,23 +101,70 @@ export class ChatProgressContentPart extends Disposable implements IChatContentP
100101
const showSpinner = shouldShowSpinner(followingContent, element);
101102
return other.kind === 'progressMessage' && this.showSpinner === showSpinner;
102103
}
104+
105+
private createApprovalMessage(): IMarkdownString | undefined {
106+
if (!this.toolInvocation) {
107+
return undefined;
108+
}
109+
110+
const reason = IChatToolInvocation.executionConfirmedOrDenied(this.toolInvocation);
111+
if (!reason || typeof reason === 'boolean') {
112+
return undefined;
113+
}
114+
115+
let md: string;
116+
switch (reason.type) {
117+
case ToolConfirmKind.Setting:
118+
md = localize('chat.autoapprove.setting', 'Auto approved by {0}', markdownCommandLink({ title: '`' + reason.id + '`', id: 'workbench.action.openSettings', arguments: [reason.id] }, false));
119+
break;
120+
case ToolConfirmKind.LmServicePerTool:
121+
md = reason.scope === 'session'
122+
? localize('chat.autoapprove.lmServicePerTool.session', 'Auto approved for this session')
123+
: reason.scope === 'workspace'
124+
? localize('chat.autoapprove.lmServicePerTool.workspace', 'Auto approved for this workspace')
125+
: localize('chat.autoapprove.lmServicePerTool.profile', 'Auto approved for this profile');
126+
md += ' (' + markdownCommandLink({ title: localize('edit', 'Edit'), id: 'workbench.action.chat.editToolApproval', arguments: [this.toolInvocation.toolId] }) + ')';
127+
break;
128+
case ToolConfirmKind.UserAction:
129+
case ToolConfirmKind.Denied:
130+
case ToolConfirmKind.ConfirmationNotNeeded:
131+
default:
132+
return;
133+
}
134+
135+
if (!md) {
136+
return undefined;
137+
}
138+
139+
return new MarkdownString(md, { isTrusted: true });
140+
}
103141
}
104142

105143
function shouldShowSpinner(followingContent: IChatRendererContent[], element: ChatTreeItem): boolean {
106144
return isResponseVM(element) && !element.isComplete && followingContent.length === 0;
107145
}
108146

109147

110-
export class ChatCustomProgressPart {
148+
export class ChatProgressSubPart extends Disposable {
111149
public readonly domNode: HTMLElement;
112150

113151
constructor(
114152
messageElement: HTMLElement,
115153
icon: ThemeIcon,
154+
tooltip: IMarkdownString | string | undefined,
155+
@IHoverService hoverService: IHoverService,
116156
) {
157+
super();
158+
117159
this.domNode = $('.progress-container');
118160
const iconElement = $('div');
119161
iconElement.classList.add(...ThemeIcon.asClassNameArray(icon));
162+
if (tooltip) {
163+
this._register(hoverService.setupDelayedHover(iconElement, {
164+
content: tooltip,
165+
style: HoverStyle.Pointer,
166+
}));
167+
}
120168
append(this.domNode, iconElement);
121169

122170
messageElement.classList.add('progress-step');
@@ -137,7 +185,7 @@ export class ChatWorkingProgressContentPart extends ChatProgressContentPart impl
137185
kind: 'progressMessage',
138186
content: new MarkdownString().appendText(localize('workingMessage', "Working..."))
139187
};
140-
super(progressMessage, chatContentMarkdownRenderer, context, undefined, undefined, undefined, instantiationService, chatMarkdownAnchorService, configurationService);
188+
super(progressMessage, chatContentMarkdownRenderer, context, undefined, undefined, undefined, undefined, instantiationService, chatMarkdownAnchorService, configurationService);
141189
}
142190

143191
override hasSameContent(other: IChatRendererContent, followingContent: IChatRendererContent[], element: ChatTreeItem): boolean {

‎src/vs/workbench/contrib/chat/browser/chatContentParts/chatTaskContentPart.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -41,7 +41,7 @@ export class ChatTaskContentPart extends Disposable implements IChatContentPart
4141
true;
4242
this.isSettled = isSettled;
4343
const showSpinner = !isSettled && !context.element.isComplete;
44-
const progressPart = this._register(instantiationService.createInstance(ChatProgressContentPart, task, chatContentMarkdownRenderer, context, showSpinner, true, undefined));
44+
const progressPart = this._register(instantiationService.createInstance(ChatProgressContentPart, task, chatContentMarkdownRenderer, context, showSpinner, true, undefined, undefined));
4545
this.domNode = progressPart.domNode;
4646
this.onDidChangeHeight = Event.None;
4747
}

‎src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolConfirmationSubPart.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -284,9 +284,9 @@ export class ChatTerminalToolConfirmationSubPart extends BaseChatToolInvocationS
284284
}
285285
};
286286
if (newRules.length === 1) {
287-
terminalData.autoApproveInfo = new MarkdownString(`_${localize('newRule', 'Auto approve rule {0} added', formatRuleLinks(newRules))}_`, mdTrustSettings);
287+
terminalData.autoApproveInfo = new MarkdownString(localize('newRule', 'Auto approve rule {0} added', formatRuleLinks(newRules)), mdTrustSettings);
288288
} else if (newRules.length > 1) {
289-
terminalData.autoApproveInfo = new MarkdownString(`_${localize('newRule.plural', 'Auto approve rules {0} added', formatRuleLinks(newRules))}_`, mdTrustSettings);
289+
terminalData.autoApproveInfo = new MarkdownString(localize('newRule.plural', 'Auto approve rules {0} added', formatRuleLinks(newRules)), mdTrustSettings);
290290
}
291291
toolConfirmKind = ToolConfirmKind.UserAction;
292292
break;

‎src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatTerminalToolProgressPart.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,7 +17,7 @@ import { IChatCodeBlockInfo } from '../../chat.js';
1717
import { ChatQueryTitlePart } from '../chatConfirmationWidget.js';
1818
import { IChatContentPartRenderContext } from '../chatContentParts.js';
1919
import { ChatMarkdownContentPart, EditorPool } from '../chatMarkdownContentPart.js';
20-
import { ChatCustomProgressPart } from '../chatProgressContentPart.js';
20+
import { ChatProgressSubPart } from '../chatProgressContentPart.js';
2121
import { BaseChatToolInvocationSubPart } from './chatToolInvocationSubPart.js';
2222
import '../media/chatTerminalToolProgressPart.css';
2323
import { TerminalContribSettingId } from '../../../../terminal/terminalContribExports.js';
@@ -106,7 +106,7 @@ export class ChatTerminalToolProgressPart extends BaseChatToolInvocationSubPart
106106
this._register(this.markdownPart.onDidChangeHeight(() => this._onDidChangeHeight.fire()));
107107

108108
elements.message.append(this.markdownPart.domNode);
109-
const progressPart = _instantiationService.createInstance(ChatCustomProgressPart, elements.container, this.getIcon());
109+
const progressPart = this._register(_instantiationService.createInstance(ChatProgressSubPart, elements.container, this.getIcon(), terminalData.autoApproveInfo));
110110
this.domNode = progressPart.domNode;
111111
}
112112

‎src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatToolInvocationPart.ts‎

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -5,10 +5,8 @@
55

66
import * as dom from '../../../../../../base/browser/dom.js';
77
import { Emitter } from '../../../../../../base/common/event.js';
8-
import { markdownCommandLink, MarkdownString } from '../../../../../../base/common/htmlContent.js';
98
import { Disposable, DisposableStore, IDisposable } from '../../../../../../base/common/lifecycle.js';
109
import { IMarkdownRenderer } from '../../../../../../platform/markdown/browser/markdownRenderer.js';
11-
import { localize } from '../../../../../../nls.js';
1210
import { IInstantiationService } from '../../../../../../platform/instantiation/common/instantiation.js';
1311
import { IChatToolInvocation, IChatToolInvocationSerialized, ToolConfirmKind } from '../../../common/chatService.js';
1412
import { IChatRendererContent } from '../../../common/chatViewModel.js';
@@ -29,6 +27,8 @@ import { ChatToolOutputSubPart } from './chatToolOutputPart.js';
2927
import { ChatToolPostExecuteConfirmationPart } from './chatToolPostExecuteConfirmationPart.js';
3028
import { ChatToolProgressSubPart } from './chatToolProgressPart.js';
3129
import { autorun } from '../../../../../../base/common/observable.js';
30+
import { localize } from '../../../../../../nls.js';
31+
import { markdownCommandLink, MarkdownString } from '../../../../../../base/common/htmlContent.js';
3232

3333
export class ChatToolInvocationPart extends Disposable implements IChatContentPart {
3434
public readonly domNode: HTMLElement;
@@ -94,8 +94,8 @@ export class ChatToolInvocationPart extends Disposable implements IChatContentPa
9494
partStore.add(this.subPart.onDidChangeHeight(() => this._onDidChangeHeight.fire()));
9595
partStore.add(this.subPart.onNeedsRerender(render));
9696

97-
// todo@connor4312/tyriar: standardize how these are displayed
98-
if (!(this.subPart instanceof ChatTerminalToolProgressPart)) {
97+
// todo@connor4312: Move MCP spinner to left to get consistent auto approval presentation
98+
if (this.subPart instanceof ChatInputOutputMarkdownProgressPart) {
9999
const approval = this.createApprovalMessage();
100100
if (approval) {
101101
this.domNode.appendChild(approval);
@@ -107,6 +107,7 @@ export class ChatToolInvocationPart extends Disposable implements IChatContentPa
107107
render();
108108
}
109109

110+
/** @deprecated Approval should be centrally managed by passing tool invocation ChatProgressContentPart */
110111
private get autoApproveMessageContent() {
111112
const reason = IChatToolInvocation.executionConfirmedOrDenied(this.toolInvocation);
112113
if (!reason || typeof reason === 'boolean') {
@@ -137,6 +138,7 @@ export class ChatToolInvocationPart extends Disposable implements IChatContentPa
137138
return md;
138139
}
139140

141+
/** @deprecated Approval should be centrally managed by passing tool invocation ChatProgressContentPart */
140142
private createApprovalMessage(): HTMLElement | undefined {
141143
const md = this.autoApproveMessageContent;
142144
if (!md) {

‎src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatToolOutputPart.ts‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { IToolResultOutputDetails } from '../../../common/languageModelToolsServ
1818
import { IChatCodeBlockInfo, IChatWidgetService } from '../../chat.js';
1919
import { IChatOutputRendererService } from '../../chatOutputItemRenderer.js';
2020
import { IChatContentPartRenderContext } from '../chatContentParts.js';
21-
import { ChatCustomProgressPart } from '../chatProgressContentPart.js';
21+
import { ChatProgressSubPart } from '../chatProgressContentPart.js';
2222
import { BaseChatToolInvocationSubPart } from './chatToolInvocationSubPart.js';
2323

2424
interface OutputState {
@@ -104,7 +104,7 @@ export class ChatToolOutputSubPart extends BaseChatToolInvocationSubPart {
104104

105105
const progressMessage = dom.$('span');
106106
progressMessage.textContent = localize('loading', 'Rendering tool output...');
107-
const progressPart = this.instantiationService.createInstance(ChatCustomProgressPart, progressMessage, ThemeIcon.modify(Codicon.loading, 'spin'));
107+
const progressPart = this._register(this.instantiationService.createInstance(ChatProgressSubPart, progressMessage, ThemeIcon.modify(Codicon.loading, 'spin'), undefined));
108108
parent.appendChild(progressPart.domNode);
109109

110110
// TODO: we also need to show the tool output in the UI

‎src/vs/workbench/contrib/chat/browser/chatContentParts/toolInvocationParts/chatToolProgressPart.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@ export class ChatToolProgressSubPart extends BaseChatToolInvocationSubPart {
7777
this.provideScreenReaderStatus(content);
7878
}
7979

80-
return this.instantiationService.createInstance(ChatProgressContentPart, progressMessage, this.renderer, this.context, undefined, true, this.getIcon());
80+
return this.instantiationService.createInstance(ChatProgressContentPart, progressMessage, this.renderer, this.context, undefined, true, this.getIcon(), this.toolInvocation);
8181
}
8282

8383
private getAnnouncementKey(kind: 'progress' | 'complete'): string {

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

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1265,7 +1265,7 @@ export class ChatListItemRenderer extends Disposable implements ITreeRenderer<Ch
12651265
} else if (content.kind === 'multiDiffData') {
12661266
return this.renderMultiDiffData(content, templateData, context);
12671267
} else if (content.kind === 'progressMessage') {
1268-
return this.instantiationService.createInstance(ChatProgressContentPart, content, this.chatContentMarkdownRenderer, context, undefined, undefined, undefined);
1268+
return this.instantiationService.createInstance(ChatProgressContentPart, content, this.chatContentMarkdownRenderer, context, undefined, undefined, undefined, undefined);
12691269
} else if (content.kind === 'working') {
12701270
return this.instantiationService.createInstance(ChatWorkingProgressContentPart, content, this.chatContentMarkdownRenderer, context);
12711271
} else if (content.kind === 'progressTask' || content.kind === 'progressTaskSerialized') {

‎src/vs/workbench/contrib/terminalContrib/chatAgentTools/browser/tools/runInTerminalTool.ts‎

Lines changed: 9 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -582,16 +582,14 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
582582
resultText += `\n\ The command is still running, with output:\n${pollingResult.output}`;
583583
}
584584

585-
const toolResultMessage = toolSpecificData.autoApproveInfo;
586585
return {
587-
toolResultMessage: toolResultMessage,
588586
toolMetadata: {
589587
exitCode: undefined // Background processes don't have immediate exit codes
590588
},
591589
content: [{
592590
kind: 'text',
593591
value: resultText,
594-
}]
592+
}],
595593
};
596594
} catch (e) {
597595
if (termId) {
@@ -723,17 +721,8 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
723721
}
724722
resultText.push(terminalResult);
725723

726-
let resolvedToolResultMessage: IMarkdownString | undefined;
727-
if (toolSpecificData.autoApproveInfo) {
728-
if (toolResultMessage) {
729-
resolvedToolResultMessage = new MarkdownString(`${toolSpecificData.autoApproveInfo.value}\n\n${toolResultMessage}`, toolSpecificData.autoApproveInfo);
730-
} else {
731-
resolvedToolResultMessage = toolSpecificData.autoApproveInfo;
732-
}
733-
}
734-
735724
return {
736-
toolResultMessage: resolvedToolResultMessage,
725+
toolResultMessage,
737726
toolMetadata: {
738727
exitCode: exitCode
739728
},
@@ -936,23 +925,23 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
936925
const isGlobalAutoApproved = config?.value ?? config.defaultValue;
937926
if (isGlobalAutoApproved) {
938927
const settingsUri = createCommandUri(openTerminalSettingsLinkCommandId, 'global');
939-
return new MarkdownString(`*${localize('autoApprove.global', 'Auto approved by setting {0}', `[\`${ChatConfiguration.GlobalAutoApprove}\`](${settingsUri.toString()} "${localize('ruleTooltip.global', 'View settings')}")`)}*`, mdTrustSettings);
928+
return new MarkdownString(`${localize('autoApprove.global', 'Auto approved by setting {0}', `[\`${ChatConfiguration.GlobalAutoApprove}\`](${settingsUri.toString()} "${localize('ruleTooltip.global', 'View settings')}")`)}`, mdTrustSettings);
940929
}
941930

942931
if (isAutoApproved) {
943932
switch (autoApproveReason) {
944933
case 'commandLine': {
945934
if (commandLineResult.rule) {
946-
return new MarkdownString(`*${localize('autoApprove.rule', 'Auto approved by rule {0}', formatRuleLinks(commandLineResult))}*`, mdTrustSettings);
935+
return new MarkdownString(localize('autoApprove.rule', 'Auto approved by rule {0}', formatRuleLinks(commandLineResult)), mdTrustSettings);
947936
}
948937
break;
949938
}
950939
case 'subCommand': {
951940
const uniqueRules = dedupeRules(subCommandResults);
952941
if (uniqueRules.length === 1) {
953-
return new MarkdownString(`*${localize('autoApprove.rule', 'Auto approved by rule {0}', formatRuleLinks(uniqueRules))}*`, mdTrustSettings);
942+
return new MarkdownString(localize('autoApprove.rule', 'Auto approved by rule {0}', formatRuleLinks(uniqueRules)), mdTrustSettings);
954943
} else if (uniqueRules.length > 1) {
955-
return new MarkdownString(`*${localize('autoApprove.rules', 'Auto approved by rules {0}', formatRuleLinks(uniqueRules))}*`, mdTrustSettings);
944+
return new MarkdownString(localize('autoApprove.rules', 'Auto approved by rules {0}', formatRuleLinks(uniqueRules)), mdTrustSettings);
956945
}
957946
break;
958947
}
@@ -961,16 +950,16 @@ export class RunInTerminalTool extends Disposable implements IToolImpl {
961950
switch (autoApproveReason) {
962951
case 'commandLine': {
963952
if (commandLineResult.rule) {
964-
return new MarkdownString(`*${localize('autoApproveDenied.rule', 'Auto approval denied by rule {0}', formatRuleLinks(commandLineResult))}*`, mdTrustSettings);
953+
return new MarkdownString(localize('autoApproveDenied.rule', 'Auto approval denied by rule {0}', formatRuleLinks(commandLineResult)), mdTrustSettings);
965954
}
966955
break;
967956
}
968957
case 'subCommand': {
969958
const uniqueRules = dedupeRules(subCommandResults.filter(e => e.result === 'denied'));
970959
if (uniqueRules.length === 1) {
971-
return new MarkdownString(`*${localize('autoApproveDenied.rule', 'Auto approval denied by rule {0}', formatRuleLinks(uniqueRules))}*`);
960+
return new MarkdownString(localize('autoApproveDenied.rule', 'Auto approval denied by rule {0}', formatRuleLinks(uniqueRules)));
972961
} else if (uniqueRules.length > 1) {
973-
return new MarkdownString(`*${localize('autoApproveDenied.rules', 'Auto approval denied by rules {0}', formatRuleLinks(uniqueRules))}*`);
962+
return new MarkdownString(localize('autoApproveDenied.rules', 'Auto approval denied by rules {0}', formatRuleLinks(uniqueRules)));
974963
}
975964
break;
976965
}

0 commit comments

Comments
 (0)