Skip to content

Commit dbd391e

Browse files
roblourensroblourens
authored
Don't allow picking the same variable multiple times (microsoft#203975)
* Cache agent subCommands, make some async code sync * Fix lastSlashCommands * Cache parsed chat query in one place so we don't have to parse the same thing over and over * Don't allow picking the same variable multiple times Fix microsoft/vscode-copilot-release#733 * Fix tests
1 parent dcea438 commit dbd391e

17 files changed

+154
-106
lines changed

‎src/vs/workbench/api/browser/mainThreadChatAgents2.ts‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ import { ExtHostChatAgentsShape2, ExtHostContext, IChatProgressDto, IExtensionCh
1818
import { IChatWidgetService } from 'vs/workbench/contrib/chat/browser/chat';
1919
import { ChatInputPart } from 'vs/workbench/contrib/chat/browser/chatInputPart';
2020
import { AddDynamicVariableAction, IAddDynamicVariableContext } from 'vs/workbench/contrib/chat/browser/contrib/chatDynamicVariables';
21-
import { IChatAgentService } from 'vs/workbench/contrib/chat/common/chatAgents';
21+
import { IChatAgentCommand, IChatAgentService } from 'vs/workbench/contrib/chat/common/chatAgents';
2222
import { ChatRequestAgentPart } from 'vs/workbench/contrib/chat/common/chatParserTypes';
2323
import { ChatRequestParser } from 'vs/workbench/contrib/chat/common/chatRequestParser';
2424
import { IChatFollowup, IChatProgress, IChatService } from 'vs/workbench/contrib/chat/common/chatService';
@@ -75,6 +75,7 @@ export class MainThreadChatAgents2 extends Disposable implements MainThreadChatA
7575
}
7676

7777
$registerAgent(handle: number, name: string, metadata: IExtensionChatAgentMetadata): void {
78+
let lastSlashCommands: IChatAgentCommand[] | undefined;
7879
const d = this._chatAgentService.registerAgent({
7980
id: name,
8081
metadata: revive(metadata),
@@ -93,11 +94,15 @@ export class MainThreadChatAgents2 extends Disposable implements MainThreadChatA
9394

9495
return this._proxy.$provideFollowups(handle, sessionId, token);
9596
},
97+
get lastSlashCommands() {
98+
return lastSlashCommands;
99+
},
96100
provideSlashCommands: async (token) => {
97101
if (!this._agents.get(handle)?.hasSlashCommands) {
98102
return []; // save an IPC call
99103
}
100-
return this._proxy.$provideSlashCommands(handle, token);
104+
lastSlashCommands = await this._proxy.$provideSlashCommands(handle, token);
105+
return lastSlashCommands;
101106
}
102107
});
103108
this._agents.set(handle, {
@@ -141,7 +146,7 @@ export class MainThreadChatAgents2 extends Disposable implements MainThreadChatA
141146
return;
142147
}
143148

144-
const parsedRequest = (await this._instantiationService.createInstance(ChatRequestParser).parseChatRequest(widget.viewModel.sessionId, model.getValue())).parts;
149+
const parsedRequest = this._instantiationService.createInstance(ChatRequestParser).parseChatRequest(widget.viewModel.sessionId, model.getValue()).parts;
145150
const agentPart = parsedRequest.find((part): part is ChatRequestAgentPart => part instanceof ChatRequestAgentPart);
146151
const thisAgentName = this._agents.get(handle)?.name;
147152
if (agentPart?.agent.id !== thisAgentName) {

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

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import { ICodeEditor } from 'vs/editor/browser/editorBrowser';
99
import { Selection } from 'vs/editor/common/core/selection';
1010
import { createDecorator } from 'vs/platform/instantiation/common/instantiation';
1111
import { IChatWidgetContrib } from 'vs/workbench/contrib/chat/browser/chatWidget';
12+
import { IParsedChatRequest } from 'vs/workbench/contrib/chat/common/chatParserTypes';
1213
import { IChatRequestViewModel, IChatResponseViewModel, IChatViewModel, IChatWelcomeMessageViewModel } from 'vs/workbench/contrib/chat/common/chatViewModel';
1314

1415
export const IChatWidgetService = createDecorator<IChatWidgetService>('chatWidgetService');
@@ -105,6 +106,7 @@ export interface IChatWidget {
105106
readonly inputEditor: ICodeEditor;
106107
readonly providerId: string;
107108
readonly supportsFileReferences: boolean;
109+
readonly parsedInput: IParsedChatRequest;
108110

109111
getContrib<T extends IChatWidgetContrib>(id: string): T | undefined;
110112
reveal(item: ChatTreeItem): void;

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

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,8 @@ import { ChatModelInitState, IChatModel } from 'vs/workbench/contrib/chat/common
3333
import { IChatReplyFollowup, IChatService } from 'vs/workbench/contrib/chat/common/chatService';
3434
import { ChatViewModel, IChatResponseViewModel, isRequestVM, isResponseVM, isWelcomeVM } from 'vs/workbench/contrib/chat/common/chatViewModel';
3535
import { IThemeService } from 'vs/platform/theme/common/themeService';
36+
import { IParsedChatRequest } from 'vs/workbench/contrib/chat/common/chatParserTypes';
37+
import { ChatRequestParser } from 'vs/workbench/contrib/chat/common/chatRequestParser';
3638

3739
const $ = dom.$;
3840

@@ -127,6 +129,15 @@ export class ChatWidget extends Disposable implements IChatWidget {
127129
return this._viewModel;
128130
}
129131

132+
private parsedChatRequest: IParsedChatRequest | undefined;
133+
get parsedInput() {
134+
if (this.parsedChatRequest === undefined) {
135+
this.parsedChatRequest = this.instantiationService.createInstance(ChatRequestParser).parseChatRequest(this.viewModel!.sessionId, this.getInput());
136+
}
137+
138+
return this.parsedChatRequest;
139+
}
140+
130141
constructor(
131142
readonly viewContext: IChatWidgetViewContext,
132143
private readonly viewOptions: IChatWidgetViewOptions,
@@ -436,6 +447,9 @@ export class ChatWidget extends Disposable implements IChatWidget {
436447
});
437448
}));
438449
this._register(this.inputPart.onDidChangeHeight(() => this.bodyDimension && this.layout(this.bodyDimension.height, this.bodyDimension.width)));
450+
this._register(this.inputEditor.onDidChangeModelContent(() => {
451+
this.parsedChatRequest = undefined;
452+
}));
439453
}
440454

441455
private onDidStyleChange(): void {
@@ -553,8 +567,6 @@ export class ChatWidget extends Disposable implements IChatWidget {
553567
const lastResponse = responses?.[responses.length - 1];
554568
this._chatAccessibilityService.acceptResponse(lastResponse, requestId);
555569
});
556-
} else {
557-
this._chatAccessibilityService.acceptResponse(undefined, requestId);
558570
}
559571
}
560572
}

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

Lines changed: 37 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,6 @@ import { CompletionContext, CompletionItem, CompletionItemKind, CompletionList }
1515
import { ITextModel } from 'vs/editor/common/model';
1616
import { ILanguageFeaturesService } from 'vs/editor/common/services/languageFeatures';
1717
import { localize } from 'vs/nls';
18-
import { IConfigurationService } from 'vs/platform/configuration/common/configuration';
1918
import { IInstantiationService } from 'vs/platform/instantiation/common/instantiation';
2019
import { Registry } from 'vs/platform/registry/common/platform';
2120
import { inputPlaceholderForeground } from 'vs/platform/theme/common/colorRegistry';
@@ -33,7 +32,6 @@ import { ChatRequestParser } from 'vs/workbench/contrib/chat/common/chatRequestP
3332
import { IChatService } from 'vs/workbench/contrib/chat/common/chatService';
3433
import { IChatSlashCommandService } from 'vs/workbench/contrib/chat/common/chatSlashCommands';
3534
import { IChatVariablesService } from 'vs/workbench/contrib/chat/common/chatVariables';
36-
import { isResponseVM } from 'vs/workbench/contrib/chat/common/chatViewModel';
3735
import { LifecyclePhase } from 'vs/workbench/services/lifecycle/common/lifecycle';
3836

3937
const decorationDescription = 'chat';
@@ -55,7 +53,6 @@ class InputEditorDecorations extends Disposable {
5553

5654
constructor(
5755
private readonly widget: IChatWidget,
58-
@IInstantiationService private readonly instantiationService: IInstantiationService,
5956
@ICodeEditorService private readonly codeEditorService: ICodeEditorService,
6057
@IThemeService private readonly themeService: IThemeService,
6158
@IChatService private readonly chatService: IChatService,
@@ -154,7 +151,7 @@ class InputEditorDecorations extends Disposable {
154151
return;
155152
}
156153

157-
const parsedRequest = (await this.instantiationService.createInstance(ChatRequestParser).parseChatRequest(viewModel.sessionId, inputValue)).parts;
154+
const parsedRequest = this.widget.parsedInput.parts;
158155

159156
let placeholderDecoration: IDecorationOptions[] | undefined;
160157
const agentPart = parsedRequest.find((p): p is ChatRequestAgentPart => p instanceof ChatRequestAgentPart);
@@ -294,7 +291,6 @@ class SlashCommandCompletions extends Disposable {
294291
constructor(
295292
@ILanguageFeaturesService private readonly languageFeaturesService: ILanguageFeaturesService,
296293
@IChatWidgetService private readonly chatWidgetService: IChatWidgetService,
297-
@IInstantiationService private readonly instantiationService: IInstantiationService,
298294
@IChatSlashCommandService private readonly chatSlashCommandService: IChatSlashCommandService
299295
) {
300296
super();
@@ -313,7 +309,7 @@ class SlashCommandCompletions extends Disposable {
313309
return null;
314310
}
315311

316-
const parsedRequest = (await this.instantiationService.createInstance(ChatRequestParser).parseChatRequest(widget.viewModel.sessionId, model.getValue())).parts;
312+
const parsedRequest = widget.parsedInput.parts;
317313
const usedAgent = parsedRequest.find(p => p instanceof ChatRequestAgentPart);
318314
if (usedAgent) {
319315
// No (classic) global slash commands when an agent is used
@@ -351,7 +347,6 @@ class AgentCompletions extends Disposable {
351347
@ILanguageFeaturesService private readonly languageFeaturesService: ILanguageFeaturesService,
352348
@IChatWidgetService private readonly chatWidgetService: IChatWidgetService,
353349
@IChatAgentService private readonly chatAgentService: IChatAgentService,
354-
@IInstantiationService private readonly instantiationService: IInstantiationService,
355350
) {
356351
super();
357352

@@ -364,7 +359,7 @@ class AgentCompletions extends Disposable {
364359
return null;
365360
}
366361

367-
const parsedRequest = (await this.instantiationService.createInstance(ChatRequestParser).parseChatRequest(widget.viewModel.sessionId, model.getValue())).parts;
362+
const parsedRequest = widget.parsedInput.parts;
368363
const usedAgent = parsedRequest.find(p => p instanceof ChatRequestAgentPart);
369364
if (usedAgent && !Range.containsPosition(usedAgent.editorRange, position)) {
370365
// Only one agent allowed
@@ -407,7 +402,7 @@ class AgentCompletions extends Disposable {
407402
return null;
408403
}
409404

410-
const parsedRequest = (await this.instantiationService.createInstance(ChatRequestParser).parseChatRequest(widget.viewModel.sessionId, model.getValue())).parts;
405+
const parsedRequest = widget.parsedInput.parts;
411406
const usedAgentIdx = parsedRequest.findIndex((p): p is ChatRequestAgentPart => p instanceof ChatRequestAgentPart);
412407
if (usedAgentIdx < 0) {
413408
return;
@@ -428,7 +423,7 @@ class AgentCompletions extends Disposable {
428423
}
429424

430425
const usedAgent = parsedRequest[usedAgentIdx] as ChatRequestAgentPart;
431-
const commands = await usedAgent.agent.provideSlashCommands(token);
426+
const commands = await usedAgent.agent.provideSlashCommands(token); // Refresh the cache here
432427

433428
return <CompletionList>{
434429
suggestions: commands.map((c, i) => {
@@ -576,7 +571,6 @@ class VariableCompletions extends Disposable {
576571
@ILanguageFeaturesService private readonly languageFeaturesService: ILanguageFeaturesService,
577572
@IChatWidgetService private readonly chatWidgetService: IChatWidgetService,
578573
@IChatVariablesService private readonly chatVariablesService: IChatVariablesService,
579-
@IConfigurationService private readonly configurationService: IConfigurationService,
580574
) {
581575
super();
582576

@@ -595,33 +589,24 @@ class VariableCompletions extends Disposable {
595589
return null;
596590
}
597591

598-
const history = widget.viewModel!.getItems()
599-
.filter(isResponseVM);
600-
601-
// TODO@roblourens work out a real API for this- maybe it can be part of the two-step flow that @file will probably use
602-
const historyVariablesEnabled = this.configurationService.getValue('chat.experimental.historyVariables');
603-
const historyItems = historyVariablesEnabled ? history.map((h, i): CompletionItem => ({
604-
label: `${chatVariableLeader}response:${i + 1}`,
605-
detail: h.response.asString(),
606-
insertText: `${chatVariableLeader}response:${String(i + 1).padStart(String(history.length).length, '0')} `,
607-
kind: CompletionItemKind.Text,
608-
range,
609-
})) : [];
610-
611-
const variableItems = Array.from(this.chatVariablesService.getVariables()).map(v => {
612-
const withLeader = `${chatVariableLeader}${v.name}`;
613-
return <CompletionItem>{
614-
label: withLeader,
615-
range,
616-
insertText: withLeader + ' ',
617-
detail: v.description,
618-
kind: CompletionItemKind.Text, // The icons are disabled here anyway
619-
sortText: 'z'
620-
};
621-
});
592+
const usedVariables = widget.parsedInput.parts.filter((p): p is ChatRequestVariablePart => p instanceof ChatRequestVariablePart);
593+
const variableItems = Array.from(this.chatVariablesService.getVariables())
594+
// This doesn't look at dynamic variables like `file`, where multiple makes sense.
595+
.filter(v => !usedVariables.some(usedVar => usedVar.variableName === v.name))
596+
.map(v => {
597+
const withLeader = `${chatVariableLeader}${v.name}`;
598+
return <CompletionItem>{
599+
label: withLeader,
600+
range,
601+
insertText: withLeader + ' ',
602+
detail: v.description,
603+
kind: CompletionItemKind.Text, // The icons are disabled here anyway
604+
sortText: 'z'
605+
};
606+
});
622607

623608
return <CompletionList>{
624-
suggestions: [...variableItems, ...historyItems]
609+
suggestions: variableItems
625610
};
626611
}
627612
}));
@@ -655,22 +640,22 @@ class ChatTokenDeleter extends Disposable {
655640

656641
// If this was a simple delete, try to find out whether it was inside a token
657642
if (!change.text && this.widget.viewModel) {
658-
parser.parseChatRequest(this.widget.viewModel.sessionId, previousInputValue).then(previousParsedValue => {
659-
// For dynamic variables, this has to happen in ChatDynamicVariableModel with the other bookkeeping
660-
const deletableTokens = previousParsedValue.parts.filter(p => p instanceof ChatRequestAgentPart || p instanceof ChatRequestAgentSubcommandPart || p instanceof ChatRequestSlashCommandPart || p instanceof ChatRequestVariablePart);
661-
deletableTokens.forEach(token => {
662-
const deletedRangeOfToken = Range.intersectRanges(token.editorRange, change.range);
663-
// Part of this token was deleted, and the deletion range doesn't go off the front of the token, for simpler math
664-
if ((deletedRangeOfToken && !deletedRangeOfToken.isEmpty()) && Range.compareRangesUsingStarts(token.editorRange, change.range) < 0) {
665-
// Assume single line tokens
666-
const length = deletedRangeOfToken.endColumn - deletedRangeOfToken.startColumn;
667-
const rangeToDelete = new Range(token.editorRange.startLineNumber, token.editorRange.startColumn, token.editorRange.endLineNumber, token.editorRange.endColumn - length);
668-
this.widget.inputEditor.executeEdits(this.id, [{
669-
range: rangeToDelete,
670-
text: '',
671-
}]);
672-
}
673-
});
643+
const previousParsedValue = parser.parseChatRequest(this.widget.viewModel.sessionId, previousInputValue);
644+
645+
// For dynamic variables, this has to happen in ChatDynamicVariableModel with the other bookkeeping
646+
const deletableTokens = previousParsedValue.parts.filter(p => p instanceof ChatRequestAgentPart || p instanceof ChatRequestAgentSubcommandPart || p instanceof ChatRequestSlashCommandPart || p instanceof ChatRequestVariablePart);
647+
deletableTokens.forEach(token => {
648+
const deletedRangeOfToken = Range.intersectRanges(token.editorRange, change.range);
649+
// Part of this token was deleted, and the deletion range doesn't go off the front of the token, for simpler math
650+
if ((deletedRangeOfToken && !deletedRangeOfToken.isEmpty()) && Range.compareRangesUsingStarts(token.editorRange, change.range) < 0) {
651+
// Assume single line tokens
652+
const length = deletedRangeOfToken.endColumn - deletedRangeOfToken.startColumn;
653+
const rangeToDelete = new Range(token.editorRange.startLineNumber, token.editorRange.startColumn, token.editorRange.endLineNumber, token.editorRange.endColumn - length);
654+
this.widget.inputEditor.executeEdits(this.id, [{
655+
range: rangeToDelete,
656+
text: '',
657+
}]);
658+
}
674659
});
675660
}
676661

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

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -17,20 +17,21 @@ import { IChatRequestVariableValue } from 'vs/workbench/contrib/chat/common/chat
1717

1818
//#region agent service, commands etc
1919

20-
export interface IChatAgentData {
21-
id: string;
22-
metadata: IChatAgentMetadata;
23-
}
24-
2520
export interface IChatAgentHistoryEntry {
2621
request: IChatAgentRequest;
2722
response: ReadonlyArray<IChatProgressResponseContent>;
2823
result: IChatAgentResult;
2924
}
3025

26+
export interface IChatAgentData {
27+
id: string;
28+
metadata: IChatAgentMetadata;
29+
}
30+
3131
export interface IChatAgent extends IChatAgentData {
3232
invoke(request: IChatAgentRequest, progress: (part: IChatProgress) => void, history: IChatAgentHistoryEntry[], token: CancellationToken): Promise<IChatAgentResult>;
3333
provideFollowups?(sessionId: string, token: CancellationToken): Promise<IChatFollowup[]>;
34+
lastSlashCommands?: IChatAgentCommand[];
3435
provideSlashCommands(token: CancellationToken): Promise<IChatAgentCommand[]>;
3536
}
3637

@@ -146,6 +147,7 @@ export class ChatAgentService extends Disposable implements IChatAgentService {
146147
throw new Error(`No agent with id ${id} registered`);
147148
}
148149
data.agent.metadata = { ...data.agent.metadata, ...updateMetadata };
150+
data.agent.provideSlashCommands(CancellationToken.None); // Update the cached slash commands
149151
this._onDidChangeAgents.fire();
150152
}
151153

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

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,6 @@
33
* Licensed under the MIT License. See License.txt in the project root for license information.
44
*--------------------------------------------------------------------------------------------*/
55

6-
import { CancellationToken } from 'vs/base/common/cancellation';
76
import { OffsetRange } from 'vs/editor/common/core/offsetRange';
87
import { IPosition, Position } from 'vs/editor/common/core/position';
98
import { Range } from 'vs/editor/common/core/range';
@@ -23,7 +22,7 @@ export class ChatRequestParser {
2322
@IChatSlashCommandService private readonly slashCommandService: IChatSlashCommandService
2423
) { }
2524

26-
async parseChatRequest(sessionId: string, message: string): Promise<IParsedChatRequest> {
25+
parseChatRequest(sessionId: string, message: string): IParsedChatRequest {
2726
const parts: IParsedChatRequestPart[] = [];
2827
const references = this.variableService.getDynamicVariables(sessionId); // must access this list before any async calls
2928

@@ -39,8 +38,7 @@ export class ChatRequestParser {
3938
} else if (char === chatAgentLeader) {
4039
newPart = this.tryToParseAgent(message.slice(i), message, i, new Position(lineNumber, column), parts);
4140
} else if (char === chatSubcommandLeader) {
42-
// TODO try to make this sync
43-
newPart = await this.tryToParseSlashCommand(sessionId, message.slice(i), message, i, new Position(lineNumber, column), parts);
41+
newPart = this.tryToParseSlashCommand(message.slice(i), message, i, new Position(lineNumber, column), parts);
4442
}
4543

4644
if (!newPart) {
@@ -140,7 +138,7 @@ export class ChatRequestParser {
140138
return;
141139
}
142140

143-
private async tryToParseSlashCommand(sessionId: string, remainingMessage: string, fullMessage: string, offset: number, position: IPosition, parts: ReadonlyArray<IParsedChatRequestPart>): Promise<ChatRequestSlashCommandPart | ChatRequestAgentSubcommandPart | undefined> {
141+
private tryToParseSlashCommand(remainingMessage: string, fullMessage: string, offset: number, position: IPosition, parts: ReadonlyArray<IParsedChatRequestPart>): ChatRequestSlashCommandPart | ChatRequestAgentSubcommandPart | undefined {
144142
const nextSlashMatch = remainingMessage.match(slashReg);
145143
if (!nextSlashMatch) {
146144
return;
@@ -169,8 +167,8 @@ export class ChatRequestParser {
169167
return;
170168
}
171169

172-
const subCommands = await usedAgent.agent.provideSlashCommands(CancellationToken.None);
173-
const subCommand = subCommands.find(c => c.name === command);
170+
const subCommands = usedAgent.agent.lastSlashCommands;
171+
const subCommand = subCommands?.find(c => c.name === command);
174172
if (subCommand) {
175173
// Valid agent subcommand
176174
return new ChatRequestAgentSubcommandPart(slashRange, slashEditorRange, subCommand);

0 commit comments

Comments
 (0)