Skip to content
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(ai): Improve agent history recording #14378

Merged

Conversation

planger
Copy link
Contributor

@planger planger commented Oct 30, 2024

What it does

  • Simplify recording for chat requests/responses by providing a util
  • Introduce explicit optional system message field
  • Show messages and system message in history view as a context
  • A few minor typos and nitpicks

How to test

Use all agents and check whether their history:

  • They should show a system message
  • They should show the context messages (if available)

Follow-ups

None

Review checklist

Reminder for reviewers

* Simplify recording for chat requests/responses by providing a util
* Introduce explicit optional system message field
* Show messages and system message in history view as a context
* A few minor typos and nitpicks
@planger planger requested a review from JonasHelming October 30, 2024 20:09
Copy link
Contributor

@JonasHelming JonasHelming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks great, it is a shame how often I misspelled Orchestrator :-)

I believe the Orchestrator and the Terminal Assistant do not list the messages in the context, is this expected?

On a fresh start, when the history is not open, the drop down of the history view is empty and I get this error when clicking it. Not sure, whether this is related to your change, though, probably rather to: #14236

root ERROR The above error occurred in the component:

at SelectComponent (http://127.0.0.1:3000/bundle.js:342317:9)
at div

Consider adding an error boundary to your tree to customize error handling behavior.
Visit https://reactjs.org/link/error-boundaries to learn more about error boundaries.

I have to reload to recover from this.

@JonasHelming
Copy link
Contributor

One question: Why do you use the fork, it would be simple to create a branch in the Theia repo.

@planger
Copy link
Contributor Author

planger commented Nov 1, 2024

@JonasHelming Thank you for your feedback!

Right, the Orchestrator didn't include the messages, even though they are technically part of the context. So I added it.
I didn't change the Terminal Assistant as it isn't a chat agent and therefore doesn't have any additional chat messages as context.

The react error is unrelated imho, but I pushed a fix for it too.

@planger planger requested a review from JonasHelming November 1, 2024 10:54
Copy link
Contributor

@JonasHelming JonasHelming left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, LGTM!

@planger planger merged commit 050d6c8 into eclipse-theia:master Nov 1, 2024
10 of 11 checks passed
@planger planger deleted the planger/ai/improve-agent-history branch November 1, 2024 22:26
@sgraband sgraband added this to the 1.56.0 milestone Nov 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants