From b958720bd52aa80d33f1eaa4ca39ce970c1f18c6 Mon Sep 17 00:00:00 2001 From: Joyce Er Date: Wed, 20 Nov 2024 14:22:45 -0500 Subject: [PATCH] fix: manually attached files should take precedence over suggested files (#234291) --- .../browser/chatEditing/chatEditingActions.ts | 4 +-- .../browser/chatEditing/chatEditingSession.ts | 6 ++-- .../contrib/chat/browser/chatInputPart.ts | 31 ++++++++++++++----- .../contrib/chat/browser/chatWidget.ts | 4 +-- .../contrib/chatInputRelatedFilesContrib.ts | 4 +-- .../contrib/chat/common/chatEditingService.ts | 7 ++++- 6 files changed, 38 insertions(+), 18 deletions(-) diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts index 73f41a5bc166d..8ae6967755b61 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingActions.ts @@ -22,7 +22,7 @@ import { GroupsOrder, IEditorGroupsService } from '../../../../services/editor/c import { IEditorService } from '../../../../services/editor/common/editorService.js'; import { ChatAgentLocation } from '../../common/chatAgents.js'; import { ChatContextKeys } from '../../common/chatContextKeys.js'; -import { applyingChatEditsFailedContextKey, CHAT_EDITING_MULTI_DIFF_SOURCE_RESOLVER_SCHEME, chatEditingResourceContextKey, chatEditingWidgetFileStateContextKey, decidedChatEditingResourceContextKey, hasAppliedChatEditsContextKey, hasUndecidedChatEditingResourceContextKey, IChatEditingService, IChatEditingSession, WorkingSetEntryState } from '../../common/chatEditingService.js'; +import { applyingChatEditsFailedContextKey, CHAT_EDITING_MULTI_DIFF_SOURCE_RESOLVER_SCHEME, chatEditingResourceContextKey, chatEditingWidgetFileStateContextKey, decidedChatEditingResourceContextKey, hasAppliedChatEditsContextKey, hasUndecidedChatEditingResourceContextKey, IChatEditingService, IChatEditingSession, WorkingSetEntryRemovalReason, WorkingSetEntryState } from '../../common/chatEditingService.js'; import { IChatService } from '../../common/chatService.js'; import { isRequestVM, isResponseVM } from '../../common/chatViewModel.js'; import { CHAT_CATEGORY } from '../actions/chatActions.js'; @@ -96,7 +96,7 @@ registerAction2(class RemoveFileFromWorkingSet extends WorkingSetAction { async runWorkingSetAction(accessor: ServicesAccessor, currentEditingSession: IChatEditingSession, chatWidget: IChatWidget, ...uris: URI[]): Promise { // Remove from working set - currentEditingSession.remove(...uris); + currentEditingSession.remove(WorkingSetEntryRemovalReason.User, ...uris); // Remove from chat input part const resourceSet = new ResourceSet(uris); diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts index dab8496790a7d..339674d6674f5 100644 --- a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts +++ b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts @@ -30,7 +30,7 @@ import { IEditorService } from '../../../../services/editor/common/editorService import { MultiDiffEditor } from '../../../multiDiffEditor/browser/multiDiffEditor.js'; import { MultiDiffEditorInput } from '../../../multiDiffEditor/browser/multiDiffEditorInput.js'; import { ChatAgentLocation, IChatAgentService } from '../../common/chatAgents.js'; -import { ChatEditingSessionChangeType, ChatEditingSessionState, ChatEditKind, IChatEditingSession, WorkingSetDisplayMetadata, WorkingSetEntryState } from '../../common/chatEditingService.js'; +import { ChatEditingSessionChangeType, ChatEditingSessionState, ChatEditKind, IChatEditingSession, WorkingSetDisplayMetadata, WorkingSetEntryRemovalReason, WorkingSetEntryState } from '../../common/chatEditingService.js'; import { IChatResponseModel } from '../../common/chatModel.js'; import { IChatWidgetService } from '../chat.js'; import { ChatEditingMultiDiffSourceResolver } from './chatEditingService.js'; @@ -328,7 +328,7 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio this._entriesObs.set(entriesArr, undefined); } - remove(...uris: URI[]): void { + remove(reason: WorkingSetEntryRemovalReason, ...uris: URI[]): void { this._assertNotDisposed(); let didRemoveUris = false; @@ -338,7 +338,7 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio continue; } didRemoveUris = this._workingSet.delete(uri) || didRemoveUris; - if (state.state === WorkingSetEntryState.Transient || state.state === WorkingSetEntryState.Suggested) { + if (reason === WorkingSetEntryRemovalReason.User && (state.state === WorkingSetEntryState.Transient || state.state === WorkingSetEntryState.Suggested)) { this._removedTransientEntries.add(uri); } } diff --git a/src/vs/workbench/contrib/chat/browser/chatInputPart.ts b/src/vs/workbench/contrib/chat/browser/chatInputPart.ts index 8aee0aca746a2..e2e699a5545f6 100644 --- a/src/vs/workbench/contrib/chat/browser/chatInputPart.ts +++ b/src/vs/workbench/contrib/chat/browser/chatInputPart.ts @@ -1085,22 +1085,36 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge // The user tried to attach too many files, we have to drop anything after the limit const entriesToPreserve: IChatCollapsibleListItem[] = []; const newEntries: IChatCollapsibleListItem[] = []; + const suggestedFiles: IChatCollapsibleListItem[] = []; for (let i = 0; i < entries.length; i += 1) { const entry = entries[i]; - // If this entry was here earlier and is still here, we should prioritize preserving it - // so that nothing existing gets evicted - const currentEntryUri = entry.kind === 'reference' && URI.isUri(entry.reference) ? entry.reference : undefined; - if (this._combinedChatEditWorkingSetEntries.find((e) => e.toString() === currentEntryUri?.toString()) && remainingFileEntriesBudget > 0) { - entriesToPreserve.push(entry); - remainingFileEntriesBudget -= 1; + if (entry.kind !== 'reference' || !URI.isUri(entry.reference)) { + continue; + } + const currentEntryUri = entry.reference; + if (entry.state === WorkingSetEntryState.Suggested) { + // Keep track of suggested files for now, they should not take precedence over newly added files + suggestedFiles.push(entry); + } else if (this._combinedChatEditWorkingSetEntries.find((e) => e.toString() === currentEntryUri?.toString())) { + // If this entry was here earlier and is still here, we should prioritize preserving it + // so that nothing existing gets evicted + if (remainingFileEntriesBudget > 0) { + entriesToPreserve.push(entry); + remainingFileEntriesBudget -= 1; + } } else { newEntries.push(entry); } } - const newEntriesThatFit = newEntries.slice(0, remainingFileEntriesBudget); - entries = [...entriesToPreserve, ...newEntriesThatFit]; + const newEntriesThatFit = remainingFileEntriesBudget > 0 ? newEntries.slice(0, remainingFileEntriesBudget) : []; remainingFileEntriesBudget -= newEntriesThatFit.length; + const suggestedFilesThatFit = remainingFileEntriesBudget > 0 ? suggestedFiles.slice(0, remainingFileEntriesBudget) : []; + // Intentional: to make bad suggestions less annoying, + // here we don't count the suggested files against the budget, + // so that the Add Files button remains enabled and the user can easily + // override the suggestions with their own manual file selections + entries = [...entriesToPreserve, ...newEntriesThatFit, ...suggestedFilesThatFit]; } if (entries.length > 1) { overviewText.textContent += ' ' + localize('chatEditingSession.manyFiles', '({0} files)', entries.length); @@ -1184,6 +1198,7 @@ export class ChatInputPart extends Disposable implements IHistoryNavigationWidge supportIcons: true, secondary: true })); + // Disable the button if the entries that are not suggested exceed the budget button.enabled = remainingFileEntriesBudget > 0; button.label = localize('chatAddFiles', '{0} Add Files...', '$(add)'); button.setTitle(button.enabled ? localize('addFiles.label', 'Add files to your working set') : localize('addFilesDisabled.label', 'You have reached the maximum number of files that can be added to the working set.')); diff --git a/src/vs/workbench/contrib/chat/browser/chatWidget.ts b/src/vs/workbench/contrib/chat/browser/chatWidget.ts index 9e5cb763ed637..75eaaa6669736 100644 --- a/src/vs/workbench/contrib/chat/browser/chatWidget.ts +++ b/src/vs/workbench/contrib/chat/browser/chatWidget.ts @@ -35,7 +35,7 @@ import { asCssVariable } from '../../../../platform/theme/common/colorUtils.js'; import { IThemeService } from '../../../../platform/theme/common/themeService.js'; import { ChatAgentLocation, IChatAgentCommand, IChatAgentData, IChatAgentService, IChatWelcomeMessageContent, isChatWelcomeMessageContent } from '../common/chatAgents.js'; import { ChatContextKeys } from '../common/chatContextKeys.js'; -import { IChatEditingService, IChatEditingSession, WorkingSetEntryState } from '../common/chatEditingService.js'; +import { IChatEditingService, IChatEditingSession, WorkingSetEntryRemovalReason, WorkingSetEntryState } from '../common/chatEditingService.js'; import { IChatModel, IChatRequestVariableEntry, IChatResponseModel } from '../common/chatModel.js'; import { ChatRequestAgentPart, IParsedChatRequest, chatAgentLeader, chatSubcommandLeader, formatChatQuestion } from '../common/chatParserTypes.js'; import { ChatRequestParser } from '../common/chatRequestParser.js'; @@ -1052,7 +1052,7 @@ export class ChatWidget extends Disposable implements IChatWidget { actualSize: number; }; this.telemetryService.publicLog2('chatEditing/workingSetSize', { originalSize: this.inputPart.attemptedWorkingSetEntriesCount, actualSize: uniqueWorkingSetEntries.size }); - currentEditingSession?.remove(...unconfirmedSuggestions); + currentEditingSession?.remove(WorkingSetEntryRemovalReason.Programmatic, ...unconfirmedSuggestions); } const result = await this.chatService.sendRequest(this.viewModel.sessionId, input, { diff --git a/src/vs/workbench/contrib/chat/browser/contrib/chatInputRelatedFilesContrib.ts b/src/vs/workbench/contrib/chat/browser/contrib/chatInputRelatedFilesContrib.ts index 33813d81543a4..5d4768232e25f 100644 --- a/src/vs/workbench/contrib/chat/browser/contrib/chatInputRelatedFilesContrib.ts +++ b/src/vs/workbench/contrib/chat/browser/contrib/chatInputRelatedFilesContrib.ts @@ -10,7 +10,7 @@ import { ResourceSet } from '../../../../../base/common/map.js'; import { URI } from '../../../../../base/common/uri.js'; import { localize } from '../../../../../nls.js'; import { IWorkbenchContribution } from '../../../../common/contributions.js'; -import { ChatEditingSessionChangeType, IChatEditingService, WorkingSetEntryState } from '../../common/chatEditingService.js'; +import { ChatEditingSessionChangeType, IChatEditingService, WorkingSetEntryRemovalReason, WorkingSetEntryState } from '../../common/chatEditingService.js'; import { IChatWidgetService } from '../chat.js'; export class ChatRelatedFilesContribution extends Disposable implements IWorkbenchContribution { @@ -80,7 +80,7 @@ export class ChatRelatedFilesContribution extends Disposable implements IWorkben existingSuggestedEntriesToRemove.push(entry[0]); } } - currentEditingSession?.remove(...existingSuggestedEntriesToRemove); + currentEditingSession?.remove(WorkingSetEntryRemovalReason.Programmatic, ...existingSuggestedEntriesToRemove); // Add the new related file suggestions to the working set for (const file of newSuggestions) { diff --git a/src/vs/workbench/contrib/chat/common/chatEditingService.ts b/src/vs/workbench/contrib/chat/common/chatEditingService.ts index 4cdc8f3fb38a1..731d6d0bfb172 100644 --- a/src/vs/workbench/contrib/chat/common/chatEditingService.ts +++ b/src/vs/workbench/contrib/chat/common/chatEditingService.ts @@ -79,7 +79,7 @@ export interface IChatEditingSession { readonly isVisible: boolean; addFileToWorkingSet(uri: URI, description?: string, kind?: WorkingSetEntryState.Transient | WorkingSetEntryState.Suggested): void; show(): Promise; - remove(...uris: URI[]): void; + remove(reason: WorkingSetEntryRemovalReason, ...uris: URI[]): void; accept(...uris: URI[]): Promise; reject(...uris: URI[]): Promise; /** @@ -91,6 +91,11 @@ export interface IChatEditingSession { redoInteraction(): Promise; } +export const enum WorkingSetEntryRemovalReason { + User, + Programmatic +} + export const enum WorkingSetEntryState { Modified, Accepted,