-
Notifications
You must be signed in to change notification settings - Fork 2.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: change set support in chat input and chat model #14750
Conversation
Very nice, looks really great. I believe this is a very powerful feature! Couple of high level comments/questions:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just a first highlevel look for now
examples/api-samples/src/browser/chat/change-set-chat-agent-contribution.ts
Show resolved
Hide resolved
} | ||
} | ||
|
||
async reject(): Promise<void> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
'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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, so I would propose to add a remove
action which is always visible and show a discard
if the change has already been applied. WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, that makes sense. I would also add a "clear" action next to accept to clear the complete changeset
0817dcb
to
2d6e9f9
Compare
* 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]>
Signed-off-by: Jonas Helming <[email protected]>
57071ea
to
32e3fab
Compare
* Add delete change set * Add delete change set element * Reintroduce latestRequest ref to have proper value in functions * Change "reject" to "undo" * Refactor change set file element to use a service for file logic
I've addressed the review feedback and added delete functionality. @sdirix I've reintroduce the |
Looks great! I found one additional issue, either to fix now or as a follow up:
Once we merge this, we should create a follow up epic with all the open topics => I can do this. |
- streamline parameters for change set deletion - remove the ref for latestRequest again
What it does
Other fixes in Chat Input UI:
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:
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:
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
Attribution
None
Review checklist
Reminder for reviewers