Improve behavior of AI Changeset diff editor (#14786)

tsmaeder · web-flow · commit 95f4d49d949d · 2025-01-30T14:11:49.000+01:00
partially fixes #14785 Contributed on behalf of STMicroelectronics Signed-off-by: Thomas Mäder <t.s.maeder@gmail.com>
diff --git a/dependency-check-baseline.json b/dependency-check-baseline.json
@@ -1,3 +1,2 @@
 {
-  "npm/npmjs/-/unicode-property-aliases-ecmascript/2.1.0": "Manually approved"
 }
diff --git a/examples/api-samples/src/browser/chat/change-set-chat-agent-contribution.ts b/examples/api-samples/src/browser/chat/change-set-chat-agent-contribution.ts
@@ -24,6 +24,7 @@ import {
 } from '@theia/ai-chat';
 import { ChangeSetFileElementFactory } from '@theia/ai-chat/lib/browser/change-set-file-element';
 import { Agent, PromptTemplate } from '@theia/ai-core';
+import { URI } from '@theia/core';
 import { inject, injectable, interfaces } from '@theia/core/shared/inversify';
 import { FileService } from '@theia/filesystem/lib/browser/file-service';
 import { WorkspaceService } from '@theia/workspace/lib/browser';
@@ -94,13 +95,14 @@ export class ChangeSetChatAgent extends AbstractStreamParsingChatAgent implement
                 chatSessionId
             })
         );
+
         if (fileToChange && fileToChange.resource) {
             changeSet.addElement(
                 this.fileChangeFactory({
                     uri: fileToChange.resource,
                     type: 'modify',
                     state: 'pending',
-                    targetState: 'Hello World Modify!',
+                    targetState: await this.computeTargetState(fileToChange.resource),
                     changeSet,
                     chatSessionId
                 })
@@ -124,6 +126,30 @@ export class ChangeSetChatAgent extends AbstractStreamParsingChatAgent implement
         ));
         request.response.complete();
     }
+    async computeTargetState(resource: URI): Promise<string> {
+        const content = await this.fileService.read(resource);
+        if (content.value.length < 20) {
+            return 'HelloWorldModify';
+        }
+        let readLocation = Math.random() * 0.1 * content.value.length;
+        let oldLocation = 0;
+        let output = '';
+        while (readLocation < content.value.length) {
+            output += content.value.substring(oldLocation, readLocation);
+            oldLocation = readLocation;
+            const type = Math.random();
+            if (type < 0.33) {
+                // insert
+                output += `this is an insert at ${readLocation}`;
+            } else {
+                // delete
+                oldLocation += 20;
+            }
+
+            readLocation += Math.random() * 0.1 * content.value.length;
+        }
+        return output;
+    }
 
     protected override async getSystemMessageDescription(): Promise<SystemMessageDescription | undefined> {
         return undefined;
diff --git a/packages/ai-chat/src/browser/change-set-file-element.ts b/packages/ai-chat/src/browser/change-set-file-element.ts
@@ -64,6 +64,10 @@ export class ChangeSetFileElement implements ChangeSetElement {
         return this.elementProps.uri;
     }
 
+    get changedUri(): URI {
+        return createChangeSetFileUri(this.elementProps.chatSessionId, this.uri);
+    }
+
     get name(): string {
         return this.elementProps.name ?? this.changeSetFileService.getName(this.uri);
     }
@@ -98,25 +102,25 @@ export class ChangeSetFileElement implements ChangeSetElement {
     }
 
     async open(): Promise<void> {
-        this.changeSetFileService.open(this.uri, this.targetState);
+        this.changeSetFileService.open(this);
     }
 
     async openChange(): Promise<void> {
         this.changeSetFileService.openDiff(
             this.uri,
-            createChangeSetFileUri(this.elementProps.chatSessionId, this.uri)
+            this.changedUri
         );
     }
 
-    async accept(): Promise<void> {
+    async accept(contents?: string): Promise<void> {
         this.state = 'applied';
         if (this.type === 'delete') {
             await this.changeSetFileService.delete(this.uri);
             this.state = 'applied';
             return;
         }
 
-        await this.changeSetFileService.write(this.uri, this.targetState);
+        await this.changeSetFileService.write(this.uri, contents !== undefined ? contents : this.targetState);
     }
 
     async discard(): Promise<void> {
diff --git a/packages/ai-chat/src/browser/change-set-file-resource.ts b/packages/ai-chat/src/browser/change-set-file-resource.ts
@@ -14,7 +14,7 @@
 // SPDX-License-Identifier: EPL-2.0 OR GPL-2.0-only WITH Classpath-exception-2.0
 // *****************************************************************************
 
-import { Resource, ResourceResolver, URI } from '@theia/core';
+import { Resource, ResourceResolver, ResourceSaveOptions, URI } from '@theia/core';
 import { inject, injectable } from '@theia/core/shared/inversify';
 import { ChatService } from '../common';
 import { ChangeSetFileElement } from './change-set-file-element';
@@ -23,7 +23,7 @@ export const CHANGE_SET_FILE_RESOURCE_SCHEME = 'changeset-file';
 const QUERY = 'uri=';
 
 export function createChangeSetFileUri(chatSessionId: string, elementUri: URI): URI {
-    return new URI(CHANGE_SET_FILE_RESOURCE_SCHEME + ':/' + chatSessionId).withQuery(QUERY + encodeURIComponent(elementUri.path.toString()));
+    return new URI(CHANGE_SET_FILE_RESOURCE_SCHEME + '://' + chatSessionId + '/' + elementUri.path).withQuery(QUERY + encodeURIComponent(elementUri.path.toString()));
 }
 
 /**
@@ -41,7 +41,7 @@ export class ChangeSetFileResourceResolver implements ResourceResolver {
             throw new Error('The given uri is not a change set file uri: ' + uri);
         }
 
-        const chatSessionId = uri.path.name;
+        const chatSessionId = uri.authority;
         const session = this.chatService.getSession(chatSessionId);
         if (!session) {
             throw new Error('Chat session not found: ' + chatSessionId);
@@ -60,8 +60,12 @@ export class ChangeSetFileResourceResolver implements ResourceResolver {
 
         return {
             uri,
-            readOnly: true,
+            readOnly: false,
+            initiallyDirty: true,
             readContents: async () => element.targetState ?? '',
+            saveContents: async (content: string, options?: ResourceSaveOptions): Promise<void> => {
+                element.accept(content);
+            },
             dispose: () => { }
         };
     }
diff --git a/packages/ai-chat/src/browser/change-set-file-service.ts b/packages/ai-chat/src/browser/change-set-file-service.ts
@@ -21,6 +21,7 @@ import { EditorManager } from '@theia/editor/lib/browser';
 import { FileService } from '@theia/filesystem/lib/browser/file-service';
 import { MonacoWorkspace } from '@theia/monaco/lib/browser/monaco-workspace';
 import { WorkspaceService } from '@theia/workspace/lib/browser/workspace-service';
+import { ChangeSetFileElement } from './change-set-file-element';
 
 @injectable()
 export class ChangeSetFileService {
@@ -82,28 +83,15 @@ export class ChangeSetFileService {
         return this.labelProvider.getLongName(uri.parent);
     }
 
-    async open(uri: URI, targetState: string): Promise<void> {
-        const exists = await this.fileService.exists(uri);
+    async open(element: ChangeSetFileElement): Promise<void> {
+        const exists = await this.fileService.exists(element.uri);
         if (exists) {
-            open(this.openerService, uri);
+            await open(this.openerService, element.uri);
             return;
         }
-        const editor = await this.editorManager.open(uri.withScheme(UNTITLED_SCHEME), {
+        await this.editorManager.open(element.changedUri, {
             mode: 'reveal'
         });
-        editor.editor.executeEdits([{
-            newText: targetState,
-            range: {
-                start: {
-                    character: 1,
-                    line: 1,
-                },
-                end: {
-                    character: 1,
-                    line: 1,
-                },
-            }
-        }]);
     }
 
     async openDiff(originalUri: URI, suggestedUri: URI): Promise<void> {
@@ -112,7 +100,7 @@ export class ChangeSetFileService {
         // Currently we don't have a great way to show the suggestions in a diff editor with accept/reject buttons
         // So we just use plain diffs with the suggestions as original and the current state as modified, so users can apply changes in their current state
         // But this leads to wrong colors and wrong label (revert change instead of accept change)
-        const diffUri = DiffUris.encode(suggestedUri, openedUri,
+        const diffUri = DiffUris.encode(openedUri, suggestedUri,
             `AI Changes: ${this.labelProvider.getName(originalUri)}`,
         );
         open(this.openerService, diffUri);
@@ -139,7 +127,7 @@ export class ChangeSetFileService {
             await this.monacoWorkspace.applyBackgroundEdit(document, [{
                 range: document.textEditorModel.getFullModelRange(),
                 text
-            }], true);
+            }], (editor, wasDirty) => editor === undefined || !wasDirty);
         } else {
             await this.fileService.write(uri, text);
         }
diff --git a/packages/core/src/browser/saveable.ts b/packages/core/src/browser/saveable.ts
@@ -392,4 +392,7 @@ export class ShouldSaveDialog extends AbstractDialog<boolean> {
         return this.shouldSave;
     }
 
+    override async open(disposeOnResolve?: boolean): Promise<boolean | undefined> {
+        return super.open(disposeOnResolve);
+    }
 }
diff --git a/packages/core/src/common/resource.ts b/packages/core/src/common/resource.ts
@@ -60,6 +60,8 @@ export interface Resource extends Disposable {
     readonly onDidChangeReadOnly?: Event<boolean | MarkdownString>;
 
     readonly readOnly?: boolean | MarkdownString;
+
+    readonly initiallyDirty?: boolean;
     /**
      * Reads latest content of this resource.
      *
@@ -378,11 +380,13 @@ export class UntitledResourceResolver implements ResourceResolver {
 export class UntitledResource implements Resource {
 
     protected readonly onDidChangeContentsEmitter = new Emitter<void>();
+    initiallyDirty: boolean;
     get onDidChangeContents(): Event<void> {
         return this.onDidChangeContentsEmitter.event;
     }
 
     constructor(private resources: Map<string, UntitledResource>, public uri: URI, private content?: string) {
+        this.initiallyDirty = (content !== undefined && content.length > 0);
         this.resources.set(this.uri.toString(), this);
     }
 
diff --git a/packages/monaco/src/browser/monaco-editor-model.ts b/packages/monaco/src/browser/monaco-editor-model.ts
@@ -19,7 +19,7 @@ import { TextEditorDocument, EncodingMode, FindMatchesOptions, FindMatch, Editor
 import { DisposableCollection, Disposable } from '@theia/core/lib/common/disposable';
 import { Emitter, Event } from '@theia/core/lib/common/event';
 import { CancellationTokenSource, CancellationToken } from '@theia/core/lib/common/cancellation';
-import { Resource, ResourceError, ResourceVersion, UNTITLED_SCHEME } from '@theia/core/lib/common/resource';
+import { Resource, ResourceError, ResourceVersion } from '@theia/core/lib/common/resource';
 import { Saveable, SaveOptions } from '@theia/core/lib/browser/saveable';
 import { MonacoToProtocolConverter } from './monaco-to-protocol-converter';
 import { ProtocolToMonacoConverter } from './protocol-to-monaco-converter';
@@ -202,7 +202,7 @@ export class MonacoEditorModel implements IResolvedTextEditorModel, TextEditorDo
             const languageSelection = StandaloneServices.get(ILanguageService).createByFilepathOrFirstLine(uri, firstLine);
             this.model = StandaloneServices.get(IModelService).createModel(value, languageSelection, uri);
             this.resourceVersion = this.resource.version;
-            this.setDirty(this._dirty || (this.resource.uri.scheme === UNTITLED_SCHEME && this.model.getValueLength() > 0));
+            this.setDirty(this._dirty || (!!this.resource.initiallyDirty));
             this.updateSavedVersionId();
             this.toDispose.push(this.model);
             this.toDispose.push(this.model.onDidChangeContent(event => this.fireDidChangeContent(event)));
@@ -553,7 +553,7 @@ export class MonacoEditorModel implements IResolvedTextEditorModel, TextEditorDo
         }
 
         const changes = [...this.contentChanges];
-        if (changes.length === 0 && !overwriteEncoding && reason !== TextDocumentSaveReason.Manual) {
+        if ((changes.length === 0 && !this.resource.initiallyDirty) && !overwriteEncoding && reason !== TextDocumentSaveReason.Manual) {
             return;
         }
 
diff --git a/packages/monaco/src/browser/monaco-workspace.ts b/packages/monaco/src/browser/monaco-workspace.ts
@@ -221,14 +221,16 @@ export class MonacoWorkspace {
      * Applies given edits to the given model.
      * The model is saved if no editors is opened for it.
      */
-    applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[], shouldSave = true): Promise<void> {
+    applyBackgroundEdit(model: MonacoEditorModel, editOperations: monaco.editor.IIdentifiedSingleEditOperation[],
+        shouldSave?: boolean | ((openEditor: MonacoEditor | undefined, wasDirty: boolean) => boolean)): Promise<void> {
         return this.suppressOpenIfDirty(model, async () => {
             const editor = MonacoEditor.findByDocument(this.editorManager, model)[0];
+            const wasDirty = !!editor?.document.dirty;
             const cursorState = editor && editor.getControl().getSelections() || [];
             model.textEditorModel.pushStackElement();
             model.textEditorModel.pushEditOperations(cursorState, editOperations, () => cursorState);
             model.textEditorModel.pushStackElement();
-            if (!editor && shouldSave) {
+            if ((typeof shouldSave === 'function' && shouldSave(editor, wasDirty)) || (!editor && shouldSave)) {
                 await model.save();
             }
         });

Original file line number	Diff line number	Diff line change
`@@ -1,3 +1,2 @@`
`1`	`1`	`{`
`2`		`- "npm/npmjs/-/unicode-property-aliases-ecmascript/2.1.0": "Manually approved"`
`3`	`2`	`}`
Original file line number	Diff line number	Diff line change
`@@ -64,6 +64,10 @@ export class ChangeSetFileElement implements ChangeSetElement {`
`64`	`64`	`return this.elementProps.uri;`
`65`	`65`	`}`
`66`	`66`
	`67`	`+ get changedUri(): URI {`
	`68`	`+ return createChangeSetFileUri(this.elementProps.chatSessionId, this.uri);`
	`69`	`+ }`
	`70`	`+`
`67`	`71`	`get name(): string {`
`68`	`72`	`return this.elementProps.name ?? this.changeSetFileService.getName(this.uri);`
`69`	`73`	`}`
`@@ -98,25 +102,25 @@ export class ChangeSetFileElement implements ChangeSetElement {`
`98`	`102`	`}`
`99`	`103`
`100`	`104`	`async open(): Promise<void> {`
`101`		`- this.changeSetFileService.open(this.uri, this.targetState);`
	`105`	`+ this.changeSetFileService.open(this);`
`102`	`106`	`}`
`103`	`107`
`104`	`108`	`async openChange(): Promise<void> {`
`105`	`109`	`this.changeSetFileService.openDiff(`
`106`	`110`	`this.uri,`
`107`		`- createChangeSetFileUri(this.elementProps.chatSessionId, this.uri)`
	`111`	`+ this.changedUri`
`108`	`112`	`);`
`109`	`113`	`}`
`110`	`114`
`111`		`- async accept(): Promise<void> {`
	`115`	`+ async accept(contents?: string): Promise<void> {`
`112`	`116`	`this.state = 'applied';`
`113`	`117`	`if (this.type === 'delete') {`
`114`	`118`	`await this.changeSetFileService.delete(this.uri);`
`115`	`119`	`this.state = 'applied';`
`116`	`120`	`return;`
`117`	`121`	`}`
`118`	`122`
`119`		`- await this.changeSetFileService.write(this.uri, this.targetState);`
	`123`	`+ await this.changeSetFileService.write(this.uri, contents !== undefined ? contents : this.targetState);`
`120`	`124`	`}`
`121`	`125`
`122`	`126`	`async discard(): Promise<void> {`
Original file line number	Diff line number	Diff line change
`@@ -392,4 +392,7 @@ export class ShouldSaveDialog extends AbstractDialog<boolean> {`
`392`	`392`	`return this.shouldSave;`
`393`	`393`	`}`
`394`	`394`
	`395`	`+ override async open(disposeOnResolve?: boolean): Promise<boolean \| undefined> {`
	`396`	`+ return super.open(disposeOnResolve);`
	`397`	`+ }`
`395`	`398`	`}`
Original file line number	Diff line number	Diff line change
`@@ -60,6 +60,8 @@ export interface Resource extends Disposable {`
`60`	`60`	`readonly onDidChangeReadOnly?: Event<boolean \| MarkdownString>;`
`61`	`61`
`62`	`62`	`readonly readOnly?: boolean \| MarkdownString;`
	`63`	`+`
	`64`	`+ readonly initiallyDirty?: boolean;`
`63`	`65`	`/**`
`64`	`66`	`* Reads latest content of this resource.`
`65`	`67`	`*`
`@@ -378,11 +380,13 @@ export class UntitledResourceResolver implements ResourceResolver {`
`378`	`380`	`export class UntitledResource implements Resource {`
`379`	`381`
`380`	`382`	`protected readonly onDidChangeContentsEmitter = new Emitter<void>();`
	`383`	`+ initiallyDirty: boolean;`
`381`	`384`	`get onDidChangeContents(): Event<void> {`
`382`	`385`	`return this.onDidChangeContentsEmitter.event;`
`383`	`386`	`}`
`384`	`387`
`385`	`388`	`constructor(private resources: Map<string, UntitledResource>, public uri: URI, private content?: string) {`
	`389`	`+ this.initiallyDirty = (content !== undefined && content.length > 0);`
`386`	`390`	`this.resources.set(this.uri.toString(), this);`
`387`	`391`	`}`
`388`	`392`