-
Notifications
You must be signed in to change notification settings - Fork 2.7k
16466: clarify role of Orchestrator agent in Theia AI chat interactions #16663
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
base: master
Are you sure you want to change the base?
Conversation
- ensure at least one language model is configured (e.g. API Key present) - report configuration errors for language models, to help the user identify what they need to do - unset the Ollama default models (not relevant defaults; also makes it impossible to distinguish whether ollama is actually configured) - remove the fallback "Default Agent" - make sure the user explicitly selects its own "Default Agent" (with some recommendations) - improve error messages when using an invalid/disabled Agent
5b21066 to
d6bb543
Compare
|
I've rebased the PR on the current master, which included a refactoring of the Welcome Page from regular React to (Localized)Markdown: issues/16470 |
ndoschek
left a comment
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.
Hi @CamilleLetavernier, thanks for this improvement, the flow works great and will guide new users much better now! 🙌
I added a few minor comments inline and have one general thought:
The views could be aligned in a follow-up so they all use consistent headings and layout. The first and default view weren't touched by this PR, so handling it separately should be fine imo. When testing the new user flow, the views switch between different styles, fonts, sizes, and alignments, which feels a bit jumpy to the eye.
| {nls.localize('theia/ai/ide/continueAnyway', 'Continue Anyway')} | ||
| </button> | ||
| </div> | ||
| <small className="theia-WelcomeMessage-Hint"> | ||
| {nls.localize('theia/ai/ide/bypassHint', 'Some agents like Claude Code don\'t require Theia Language Models')} |
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.
A little more info here would be nice, that this sets the preference Bypass Model Requirement.
Maybe a tooltip would also be good here.
| **Common fixes:** | ||
| - Check that your API key is valid and not expired | ||
| - Verify that custom servers (Ollama, LlamaFile) are running | ||
| - Check your network connection and firewall settings |
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.
This should also be left-aligned. If the UL content is centered, it looks off.
| <button | ||
| key={agent.id} | ||
| className="theia-WelcomeMessage-AgentButton" | ||
| onClick={() => this.setDefaultAgent(agent.id)} | ||
| title={agent.description}> | ||
| <span className="theia-WelcomeMessage-AgentButton-Icon">@</span> | ||
| <span className="theia-WelcomeMessage-AgentButton-Label">{agent.label}</span> | ||
| </button> |
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.
This is a little big, would be nice if we could get the same button size as on the other screen (open ui settings) to have a little consistency between the views.
| <LocalizedMarkdown | ||
| localizationKey="theia/ai/ide/moreAgentsAvailable" | ||
| defaultMarkdown={recommendedAgents.length > 0 | ||
| ? 'More agents are available. Use @AgentName to try others or configure a different default in [preferences]({0}).' |
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.
This box looks a little off, we should have a least the same padding as the input box on the bottom.
In general it would be great if we could reuse another theia concept, like the classes used in the AlertMessage.
E.g. in the scm view we have a warning alert message, but in this case we could use info level, 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.
In general I would like to double check the font sizes, margins and so on and use theia css variables where possible, but as mentioned in my general comment, it would also be fine for me to rework the styling in a follow up together with the other views we have.
|
@JonasHelming could you also take a look at the flow in general and let us know if you’re fine with it or have any additional feedback? TIA! |
|
Thanks, this is a nice improvement for initial users!
I think we should make the message feel less like an exception, as users will very likely run into it. Suggestion: Please configure at least one language model providerIf you want to use OpenAI, Anthropic or GoogleAI hosted models, please enter an API key in the settings. (maybe link the provider names to their settings page) Current Configuration Statemessage from providers |
What it does
How to test
Screen 1: AI Features are disabled (Not change for this screen)
Screen 2: AI Models are not configured (New screen 🌟 )
Screen 3: Default Agent has not been selected (New screen 🌟 )
Screen 4: Everything is (probably) fine, happy chatting! (No change for this screen)
Follow-ups
Breaking changes
Attribution
Review checklist
nlsservice (for details, please see the Internationalization/Localization section in the Coding Guidelines)Reminder for reviewers