Improve AI diff UX #14910

colin-grant-work · 2025-02-12T23:05:36Z

Updates editors when new suggestions proposed
Separates accept and saving actions to avoid unnecessary prompts
Avoids use of untitled editors to avoid prompts for them

What it does

Fixes parts of #14768. In particular:

Eliminates autosaving for AI chat generated editors. Here, I chose to pipe a new autosavable field from the Resource interface to the Saveable interface and then not perform autosaves if the Saveable says it doesn't want to be autosaved. @msujew, you've recently worked on the autosaving infrastructure, so I'd be grateful to get your thoughts. Another alternative I considered was to pipe the SaveReason through to the Resource's save options so that some resources could just reject automatic saves, but that feels less clean than having the Saveable declare itself exempt from that system.
Switches to using query-based in-memory resources for the left side of chat diffs. That makes them read-only, which I think makes sense: the user's focus is (or should be) on the right side, where the AI has suggested things. This avoids some synching issues, but arguably introduces others. @Reviewers, please see what you think of the way the diff behaves when you save the right side. It will still show the diff relative to original state, without showing that the underlying left-side file has actually been updated. I can fix that by using mutable in-memory resources, but then the lifecycle gets a little trickier.
Separates the save cycle of the resource from the apply cycle of the ChangeSetFileElement. This allows us to avoid hanging dirty state or prompts to save when the user believes they have saved / applied the changes.
Changes the names of methods on ChangeSetElements from accept and discard to apply and revert and removes the 'discarded' state option. The practical reason for doing so was that we were inconsistent in our handling of 'reverted' changes: we let them be reapplied on the element level, but we didn't reactivate the global Activate button on the ChangeSet level. Now if the user reverts a change, it returns to 'pending' state: I don't think we should stand in the way of the user having third thoughts. Semantically, discard also sounded more like delete than it behaved. I'm open to discussion about this point.
Speaking of delete, I added optional dispose to the ChangeSetElement interface to be called when the element is deleted. This lets me do cleanup like closing diff editors when a change element is deleted. I have some misgivings about the lifecycle still: we currently replace elements in the change set, but we don't want everything associated with a given proposal (e.g. diff editors) to die when it gets replaced.
And finally (I think), I adapted my code from Update editors when ChangeElement added or replaced #14901 to update an editor when a new suggestion is made for the same file in the same session.

How to test

Fairly thorough testing of all the interactions you've gotten used to re: LLM suggestions.
Turn on autosave and observe that your AI editors aren't affected.
Open a diff editor and close it in a dirty state -> you should be prompted to save. I thought about getting rid of this if the user hasn't actually interacted with the editor, since then the state is saved as part of the LLM interaction. Let me know your thoughts.
Open a diff editor and save it. The corresponding node should be marked as accepted in the UI.
Open a diff and accept it in the chat UI. The diff should close without a prompt re: saving.
Have the LLM generate a new file. No matter what you do, you should never be prompted for the path at which to save something.
Open a diff editor and then delete the corresponding node. The editor should close. It should prompt you whether you want to save. Same question as re: (3).
Open a diff editor, then ask the agent to do something else in the same file (see instructions for Update editors when ChangeElement added or replaced #14901, e.g.). The editor should be updated with the new content.

Follow-ups

Breaking changes

This PR introduces breaking changes and requires careful review.
If yes, the breaking changes section in the changelog has been updated before merge.

Attribution

Review checklist

As an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

As a reviewer, I agree to behave in accordance with the review guidelines

JonasHelming · 2025-02-12T23:24:15Z

Question: The PR description says the change log was updated (due to breaking changes), but i do not see the update in the PR?

- Updates editors when new suggestions proposed - Separates accept and saving actions to avoid unnecessary prompts - Avoids use of untitled editors to avoid prompts for them

colin-grant-work · 2025-02-13T15:02:31Z

Question: The PR description says the change log was updated (due to breaking changes), but i do not see the update in the PR?

Fair point. I'd say that some of the breakage is optional, and I'd prefer to update the changelog once I know which bits have survived review (or been added because of it). But I can write an entry now and then try to keep it up to date if anything changes, if you prefer.

planger

Awesome, this improves the current state already immensely!

Asking to Save

Open a diff editor and close it in a dirty state -> you should be prompted to save. I thought about getting rid of this if the user hasn't actually interacted with the editor, since then the state is saved as part of the LLM interaction. Let me know your thoughts.

Open a diff editor and then delete the corresponding node. The editor should close. It should prompt you whether you want to save. Same question as re: (3).

I personally would prefer, if we don't ask the user whether they would like to save, if the right side of the suggestion-diff-editor hasn't been changed by the user. Feels unexpected. But its tricky and I can happily live with both.

Accept vs Apply, etc.

I like the new naming much better than what we had before! Thanks!
Shouldn't we also propagate that to the UI too? The labels of the button and hovers still use the term accept. I think it'll be better to keep the naming in the code and UI consistent, and change "Accept" to "Apply" and "Undo" to "Revert" and "Delete" to "Discard"?

Dirty State When Applied

If the state of a suggestion is "Applied" and we open the diff editor, shouldn't we avoid the dirty state in the diff editor? It is already applied and the dirty state suggests that it is not, in my view.

The Left Side of the Suggestion Diff Editor

Currently the left side is the state of the file at the time where the change set element has been created and remains in this state.

Shouldn't we keep the left side of the diff editor always up to date with what's currently in the file system or Monaco workspace? If I made changes in between, and hit save in the diff editor, I wouldn't even see that I'm overwriting my changes. If we would show the actual current state left, I'd at least see that as a diff.

On the other hand, it currently reflects what the LLM was using as its basis; also, if the user applied the suggestions, the diff editor now still shows the actual original suggestions. Makes sense too.

However, overall, it feels more logical to me, if the left side is always the current workspace state. This would also mean that if I applied the suggestions, there are no further AI suggestions to review -- so the diff editor becomes clean (no changes).

In conclusion, I lean towards syncing the left side with the current Monaco workspace state (or file system state if not open). To me the benefits outweigh the single advantage of the current approach --- ie that I still can see what the LLM originally suggested. But to the user this is not really relevant information, once they already reviewed and accepted the changes. And they can always revert in the change set UI.

planger · 2025-02-13T19:21:10Z

packages/ai-chat/src/browser/change-set-file-element.ts

-        this.state = 'applied';
-        if (this.type === 'delete') {
+    async apply(contents?: string): Promise<void> {
+        if (await this.changeSetFileService.trySave(this.changedUri)) { /** Continue */ } else if (this.type === 'delete') {


Optional nitpick: Just a matter of taste, but I personally find /** Continue */ less clear. You might consider negating the condition and either duplicating the closeDiffsFor() call or nesting the if-statement inside the negated condition.

planger · 2025-02-13T20:31:04Z

Oh, one more observation:
It looks like with the @ChangeSet agent, the diff editor content isn't replaced (even if it is the same file), while it works with the @Coder. I think, this is because the @ChangeSet agent always creates a new change set, while the ChangeSetFileResourceResolver listens to the specific change set for updates.

We should probably change the @ChangeSet agent to ensure the example conforms to best practice.

JonasHelming requested a review from planger February 12, 2025 23:24

Improve AI diff UX

5a11c4b

- Updates editors when new suggestions proposed - Separates accept and saving actions to avoid unnecessary prompts - Avoids use of untitled editors to avoid prompts for them

colin-grant-work force-pushed the bugfix/no-change-resource-change-event branch from b07a34a to 5a11c4b Compare February 13, 2025 14:59

colin-grant-work mentioned this pull request Feb 13, 2025

[Theia AI] Coder and ChangeSet Epic #14768

Open

19 tasks

planger reviewed Feb 13, 2025

View reviewed changes

JonasHelming mentioned this pull request Feb 14, 2025

Theia AI G&A - AI-powered Theia IDE alpha - Release epic #14923

Open

31 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve AI diff UX #14910

Improve AI diff UX #14910

colin-grant-work commented Feb 12, 2025 •

edited

Loading

JonasHelming commented Feb 12, 2025

colin-grant-work commented Feb 13, 2025

planger left a comment •

edited

Loading

planger Feb 13, 2025 •

edited

Loading

planger commented Feb 13, 2025 •

edited

Loading

Improve AI diff UX #14910

Are you sure you want to change the base?

Improve AI diff UX #14910

Conversation

colin-grant-work commented Feb 12, 2025 • edited Loading

What it does

How to test

Follow-ups

Breaking changes

Attribution

Review checklist

Reminder for reviewers

JonasHelming commented Feb 12, 2025

colin-grant-work commented Feb 13, 2025

planger left a comment • edited Loading

Choose a reason for hiding this comment

Asking to Save

Accept vs Apply, etc.

Dirty State When Applied

The Left Side of the Suggestion Diff Editor

planger Feb 13, 2025 • edited Loading

Choose a reason for hiding this comment

planger commented Feb 13, 2025 • edited Loading

colin-grant-work commented Feb 12, 2025 •

edited

Loading

planger left a comment •

edited

Loading

planger Feb 13, 2025 •

edited

Loading

planger commented Feb 13, 2025 •

edited

Loading