feat: change set support in chat input and chat model #14750

planger · 2025-01-20T20:14:11Z

What it does

Add the concept of a change set to chat model and input UI
Add an implementation of change set elements for files
Add an agent for testing: @Changeset

Other fixes in Chat Input UI:

The inProgress state of the chat input was actually unsafely managed. This change addresses the proper management of the inProgress state.
The positioning, e.g. of the placeholder is now more adaptive.

As the change set feature directly relates to another feature (context, work in progress), this change also already prepares for those changes in the chat UI:

Prepare chat input for adding context to requests
Add context in the form of variables to chat model

Fixes #14749

How to test

To demonstrate the usage and simplify testing, this PR includes a test agent named @ChangeSet. This statically sets a somewhat random change set into the chat model, with one new, one modified, and one deleted file change set element.

change-set.mp4

Follow-ups

Next steps include:

Hook up the Coder Agent to allow for testing this change set support in more breadth.
The Coder Agent should be switched to use the monaco model, if it exists, to capture potentially dirty states -- otherwise the AI sees a different version than the user.
Improve the diff editor we use here: currently there seems to be no simple way to show a diff between the workspace (editable) and a suggested modified version, while allowing to "accept" changes from the suggested version to the workspace version. Therefore, we take a somewhat ugly shortcut in this PR to show a diff between the suggested changes as readonly original version left and the editable workspace version on the right. This works, however, the colors are wrong (red for additions) and the labels are wrong (reject, even though it is actually an accept).
Potentially optimize the ChangeSetFileElement how it applies and reverts changes, taking the monaco text model into account. I aimed at reusing the functionality to modify the monaco model if an editor is open without saving, and saving to disk directly if no editor is opened. But this should be tested for user experience and consistency.

Breaking changes

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

Attribution

None

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-01-20T22:45:04Z

Very nice, looks really great. I believe this is a very powerful feature!

Couple of high level comments/questions:

How do we combine this with the existing FileChangeSetService, as there is quite some overlap.
- Do we keep both data structures? This is the biggest overlap. In particular it is:
  Coder: FileChangeSetService|ChangeSet|FileChange vs. this PR ChangeSet|ChangeSetElement|ChangeSetFileElement
- Where are changes applied to the content (currently only in the FileChangeSetService)
- Where are file created, deleted, written (currently in both)
- How can the LLM check whether changes are applicable
- How can the LLM look at the new state after the change would be applied before they are actually applied
How do we hand the chatID around. We need it at least in functions for Coder to work in its current state
What is our story to close a changeset or to clear it?

JonasHelming

Just a first highlevel look for now

JonasHelming · 2025-01-20T22:24:17Z

examples/api-samples/src/browser/chat/change-set-chat-agent-contribution.ts

+        request.response.response.addContent(new MarkdownChatResponseContentImpl(
+            'I have created a change set for you. You can now review and apply it.'
+        ));
+        request.response.complete();


I wonder when we should update the change set in a real scenario. Simple solution would be when a response is there, but if more then one file is changed, it looks too late from a user POV. Unfortunately, the agent currently does not know which functions are called, therefore, we have no good trigger.

JonasHelming · 2025-01-20T22:45:18Z

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

+        }
+    }
+
+    async reject(): Promise<void> {


'reject' is rather 'undo' or 'discard', two comments:

I would rename this action and only show it, if the change has already been applied

I believe we need something to remove a change from a changeset without applying it.

packages/ai-chat/src/common/chat-model.ts

JonasHelming · 2025-01-20T22:49:23Z

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

+};
+
+@injectable()
+export class ChangeSetFileElement implements ChangeSetElement {


I am not 100% sure if I like the mixture between behavior and state in this class. It relies heavily on DI and Theia services and seems hard to test. Maybe we should extract the behavior to a separate interface?

Yeah, I'm certainly open to extract e.g. an abstract super class. I considered it when I wrote it, but the only behavior related to changing the generic ChangeElement state is switching the _state flag on accept and reject. So I thought it might not be worth it. Except for that, all behavior is file specific.

My suggestion would be more to extract the behavior into a separate interface (delegation), I am not 100% sure myself, though.

JonasHelming · 2025-01-20T22:58:21Z

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

Is this used somewhere?

Yes, the resource resolver is registered here:
https://github.com/eclipse-theia/theia/pull/14750/files/0817dcbf334ccfe0a430798ffcbce1dbcce9baa9#diff-444a8731310f8ef30095775299ab7f86f4c392bdd312ac603b9b795f9b3759d4R103

This is needed to obtain the suggested content of a file via the Theia URIs resolution, enabling to reuse the DiffUris in Theia.

With this change we enable passing optional context to tool calls. This is then used for tool calls in the context of a chat to pass on the chat request model. This way tool call handlers can retrieve additional information as a context or update the chat model directly.

sdirix · 2025-01-22T10:10:07Z

packages/ai-chat-ui/src/browser/chat-input-widget.tsx

+    React.useEffect(() => {
+        const onChatModelUpdate = (event: ChatChangeEvent) => {
+            if (ChatChangeEvent.isChangeSetEvent(event)) {
+                forceUpdate();


It seems the forceUpdate is used to update the UI for changes in the changeSet. A bit more elegant would be to maintain a "view model" abstraction in useState for the ChangeSetBox and update that one whenever needed. Then we would not need a forceUpdate

sdirix · 2025-01-22T10:13:50Z

packages/ai-chat-ui/src/browser/chat-input-widget.tsx

+        const onResponseUpdate = () => {
+            setInProgress(ChatRequestModel.isInProgress(lastRequest));
+        };
+        const listener = lastRequest?.response.onDidChange(onResponseUpdate);
        return () => listener?.dispose();
    }, [lastRequest]);


This was not introduced by your PR but as I am just noticing it: We should never build useEffect based transformations for state which is under our control. At best this introduces unnecessary rerenderings and confusing state updates, at worst this ends up in endless rerender cycles.

As lastRequest is maintained by us, we should perform any side effects we want to see at the same time as lastRrequest is changed, i.e. by the code calling the setter.

sdirix · 2025-01-22T10:26:47Z

packages/ai-chat-ui/src/browser/chat-input-widget.tsx

+    const leftOptions = React.useMemo<Option[]>(() => (
+        props.showContext ? [{
+            title: 'Attach elements to context',
+            handler: () => { /* TODO */ },
+            className: 'codicon-add'
+        }] : []
+    ), [props.showContext]);
+
+    const rightOptions = React.useMemo<Option[]>(() => (


At the moment the memos are not worth it. The only thing we save are the construction of the arrays, however the ChatInputOptions will be rerendered anyway.

Only if we would memo the whole ChatInputOptions, then the useMemos are needed. Not sure whether all these complications are worth it, I would imagine that the ChatInputOptions are cheap to render.

@Changeset

* Add the concept of a change set to chat model and input UI * Add an implementation of change set elements for files * Add an agent for testing: @Changeset Other fixes in Chat Input UI: * The inProgress state of the chat input was actually unsafely managed. This change addresses the proper management of the inProgress state. * The positioning, e.g. of the placeholder is now more adaptive. As the change set feature directly relates to another feature (context, work in progress), this change also already prepares for those changes in the chat UI: * Prepare chat input for adding context to requests * Add context in the form of variables to chat model Fixes #14749 Signed-off-by: Jonas Helming <[email protected]>

JonasHelming requested a review from sdirix January 20, 2025 22:21

JonasHelming reviewed Jan 20, 2025

View reviewed changes

JonasHelming marked this pull request as draft January 22, 2025 07:51

sdirix reviewed Jan 22, 2025

View reviewed changes

JonasHelming force-pushed the planger/issues/14749 branch from 0817dcb to 2d6e9f9 Compare January 22, 2025 10:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: change set support in chat input and chat model #14750

feat: change set support in chat input and chat model #14750

planger commented Jan 20, 2025

JonasHelming commented Jan 20, 2025 •

edited

Loading

JonasHelming left a comment

JonasHelming Jan 20, 2025

JonasHelming Jan 20, 2025

JonasHelming Jan 20, 2025

planger Jan 21, 2025

JonasHelming Jan 21, 2025

JonasHelming Jan 20, 2025

planger Jan 21, 2025

sdirix Jan 22, 2025

sdirix Jan 22, 2025

sdirix Jan 22, 2025

feat: change set support in chat input and chat model #14750

Are you sure you want to change the base?

feat: change set support in chat input and chat model #14750

Conversation

planger commented Jan 20, 2025

What it does

How to test

Follow-ups

Breaking changes

Attribution

Review checklist

Reminder for reviewers

JonasHelming commented Jan 20, 2025 • edited Loading

JonasHelming left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JonasHelming commented Jan 20, 2025 •

edited

Loading