chat: invoke save participants only when accepting changes (#238628)

- Don't trigger save participants in edits from the edit file tool. Fixes the bad undo stack we discussed this morning. - When modifications are accepted, if the file is not dirty, then trigger save participants. This lets you still get nice formatting.
microsoft · Jan 24, 2025 · c53aa1b · c53aa1b
1 parent b94947f
commit c53aa1b
Show file tree

Hide file tree

Showing 2 changed files with 54 additions and 13 deletions.
diff --git a/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts b/src/vs/workbench/contrib/chat/browser/chatEditing/chatEditingSession.ts
@@ -4,44 +4,46 @@
  *--------------------------------------------------------------------------------------------*/
 
 import { ITask, Sequencer, timeout } from '../../../../../base/common/async.js';
+import { VSBuffer } from '../../../../../base/common/buffer.js';
 import { BugIndicatingError } from '../../../../../base/common/errors.js';
 import { Emitter } from '../../../../../base/common/event.js';
-import { Disposable, dispose } from '../../../../../base/common/lifecycle.js';
+import { StringSHA1 } from '../../../../../base/common/hash.js';
+import { Disposable, DisposableMap, dispose } from '../../../../../base/common/lifecycle.js';
 import { ResourceMap, ResourceSet } from '../../../../../base/common/map.js';
 import { autorun, derived, IObservable, IReader, ITransaction, observableValue, transaction } from '../../../../../base/common/observable.js';
+import { autorunDelta, autorunIterableDelta } from '../../../../../base/common/observableInternal/autorun.js';
+import { isEqual, joinPath } from '../../../../../base/common/resources.js';
 import { URI } from '../../../../../base/common/uri.js';
 import { isCodeEditor, isDiffEditor } from '../../../../../editor/browser/editorBrowser.js';
 import { IBulkEditService } from '../../../../../editor/browser/services/bulkEditService.js';
+import { IOffsetEdit, ISingleOffsetEdit, OffsetEdit } from '../../../../../editor/common/core/offsetEdit.js';
 import { TextEdit } from '../../../../../editor/common/languages.js';
 import { ILanguageService } from '../../../../../editor/common/languages/language.js';
 import { ITextModel } from '../../../../../editor/common/model.js';
 import { IModelService } from '../../../../../editor/common/services/model.js';
 import { ITextModelService } from '../../../../../editor/common/services/resolverService.js';
 import { localize } from '../../../../../nls.js';
 import { EditorActivation } from '../../../../../platform/editor/common/editor.js';
+import { IEnvironmentService } from '../../../../../platform/environment/common/environment.js';
 import { IFileService } from '../../../../../platform/files/common/files.js';
 import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js';
+import { ILogService } from '../../../../../platform/log/common/log.js';
 import { IWorkspaceContextService } from '../../../../../platform/workspace/common/workspace.js';
-import { IEditorCloseEvent } from '../../../../common/editor.js';
+import { IEditorCloseEvent, SaveReason } from '../../../../common/editor.js';
 import { DiffEditorInput } from '../../../../common/editor/diffEditorInput.js';
 import { IEditorGroupsService } from '../../../../services/editor/common/editorGroupsService.js';
 import { IEditorService } from '../../../../services/editor/common/editorService.js';
+import { ITextFileService } from '../../../../services/textfile/common/textfiles.js';
 import { MultiDiffEditor } from '../../../multiDiffEditor/browser/multiDiffEditor.js';
 import { MultiDiffEditorInput } from '../../../multiDiffEditor/browser/multiDiffEditorInput.js';
+import { isNotebookEditorInput } from '../../../notebook/common/notebookEditorInput.js';
+import { INotebookService } from '../../../notebook/common/notebookService.js';
 import { ChatEditingSessionChangeType, ChatEditingSessionState, ChatEditKind, getMultiDiffSourceUri, IChatEditingSession, IModifiedFileEntry, WorkingSetDisplayMetadata, WorkingSetEntryRemovalReason, WorkingSetEntryState } from '../../common/chatEditingService.js';
 import { IChatResponseModel } from '../../common/chatModel.js';
-import { ChatEditingModifiedFileEntry, IModifiedEntryTelemetryInfo, ISnapshotEntry } from './chatEditingModifiedFileEntry.js';
-import { ChatEditingTextModelContentProvider } from './chatEditingTextModelContentProviders.js';
-import { isEqual, joinPath } from '../../../../../base/common/resources.js';
-import { StringSHA1 } from '../../../../../base/common/hash.js';
-import { IEnvironmentService } from '../../../../../platform/environment/common/environment.js';
-import { VSBuffer } from '../../../../../base/common/buffer.js';
-import { IOffsetEdit, ISingleOffsetEdit, OffsetEdit } from '../../../../../editor/common/core/offsetEdit.js';
-import { ILogService } from '../../../../../platform/log/common/log.js';
 import { IChatService } from '../../common/chatService.js';
-import { INotebookService } from '../../../notebook/common/notebookService.js';
+import { ChatEditingModifiedFileEntry, IModifiedEntryTelemetryInfo, ISnapshotEntry } from './chatEditingModifiedFileEntry.js';
 import { ChatEditingModifiedNotebookEntry } from './chatEditingModifiedNotebookEntry.js';
-import { isNotebookEditorInput } from '../../../notebook/common/notebookEditorInput.js';
+import { ChatEditingTextModelContentProvider } from './chatEditingTextModelContentProviders.js';
 
 const STORAGE_CONTENTS_FOLDER = 'contents';
 const STORAGE_STATE_FILE = 'state.json';
@@ -174,6 +176,7 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio
 		@IEditorService private readonly _editorService: IEditorService,
 		@IChatService private readonly _chatService: IChatService,
 		@INotebookService private readonly _notebookService: INotebookService,
+		@ITextFileService private readonly _textFileService: ITextFileService,
 	) {
 		super();
 	}
@@ -193,6 +196,7 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio
 
 		// Add the currently active editors to the working set
 		this._trackCurrentEditorsInWorkingSet();
+		this._triggerSaveParticipantsOnAccept();
 		this._register(this._editorService.onDidVisibleEditorsChange(() => {
 			this._trackCurrentEditorsInWorkingSet();
 		}));
@@ -225,6 +229,39 @@ export class ChatEditingSession extends Disposable implements IChatEditingSessio
 		return storage.storeState(state);
 	}
 
+	private _triggerSaveParticipantsOnAccept() {
+		const im = this._register(new DisposableMap<ChatEditingModifiedFileEntry>());
+		const attachToEntry = (entry: ChatEditingModifiedFileEntry) => {
+			return autorunDelta(entry.state, ({ lastValue, newValue }) => {
+				if (newValue === WorkingSetEntryState.Accepted && lastValue === WorkingSetEntryState.Modified) {
+					// Don't save a file if there's still pending changes. If there's not (e.g.
+					// the agentic flow with autosave) then save again to trigger participants.
+					if (!this._textFileService.isDirty(entry.modifiedURI)) {
+						this._textFileService.save(entry.modifiedURI, {
+							reason: SaveReason.EXPLICIT,
+							force: true,
+							ignoreErrorHandler: true,
+						}).catch(() => {
+							// ignored
+						});
+					}
+				}
+			});
+		};
+
+		this._register(autorunIterableDelta(
+			reader => this._entriesObs.read(reader),
+			({ addedValues, removedValues }) => {
+				for (const entry of addedValues) {
+					im.set(entry, attachToEntry(entry));
+				}
+				for (const entry of removedValues) {
+					im.deleteAndDispose(entry);
+				}
+			}
+		));
+	}
+
 	private _trackCurrentEditorsInWorkingSet(e?: IEditorCloseEvent) {
 		const existingTransientEntries = new ResourceSet();
 		for (const file of this._workingSet.keys()) {

diff --git a/src/vs/workbench/contrib/chat/browser/tools/tools.ts b/src/vs/workbench/contrib/chat/browser/tools/tools.ts
@@ -12,6 +12,7 @@ import { localize } from '../../../../../nls.js';
 import { IInstantiationService } from '../../../../../platform/instantiation/common/instantiation.js';
 import { IWorkspaceContextService } from '../../../../../platform/workspace/common/workspace.js';
 import { IWorkbenchContribution } from '../../../../common/contributions.js';
+import { SaveReason } from '../../../../common/editor.js';
 import { ITextFileService } from '../../../../services/textfile/common/textfiles.js';
 import { ICodeMapperService } from '../../common/chatCodeMapperService.js';
 import { IChatEditingService } from '../../common/chatEditingService.js';
@@ -171,7 +172,10 @@ class EditTool implements IToolImpl {
 			dispose.dispose();
 		});
 
-		await this.textFileService.save(uri);
+		await this.textFileService.save(uri, {
+			reason: SaveReason.AUTO,
+			skipSaveParticipants: true,
+		});
 
 		return {
 			content: [{ kind: 'text', value: 'The file was edited successfully' }]