-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Add profile management functions to API #2251
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
Conversation
|
| } | ||
|
|
||
| private generateId() { | ||
| public generateId() { |
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.
Exposing generateId() as public may allow external misuse. Consider documenting its intended use and, if IDs are security-sensitive, using a more robust method (e.g., crypto.randomUUID()).
| public async createProfile(name: string): Promise<string> { | ||
| // Input validation | ||
| if (!name || !name.trim()) { | ||
| throw new Error("Profile name cannot be empty") |
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.
Error messages in createProfile are user-facing. Consider internationalizing these strings (e.g., using i18next) to adhere to our standards.
| throw new Error("Profile name cannot be empty") | |
| throw new Error(i18next.t('error.profileNameEmpty')) |
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.
Not sure we need to worry about this for the API
| public async createProfile(name: string): Promise<string> { | ||
| // Input validation | ||
| if (!name || !name.trim()) { | ||
| throw new Error("Profile name cannot be empty") |
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.
When checking for existing profiles, an error is thrown using a literal string. Please internationalize this message as well.
| throw new Error("Profile name cannot be empty") | |
| throw new Error(i18n.t('error.profileNameEmpty')) |
|
|
||
| const profile = profiles.find((p) => p.name === name) | ||
| if (!profile) { | ||
| throw new Error(`Profile with name "${name}" does not exist`) |
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 setActiveProfile, the error message is hard-coded. Consider using internationalized strings for consistency with our development standards.
| return this.getConfiguration().currentApiConfigName | ||
| } | ||
|
|
||
| public async deleteProfile(name: string): 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.
The deleteProfile method throws user-facing errors and resets the active profile without fallback. Consider internationalizing the error and, if appropriate, selecting a different active profile rather than setting it to undefined.
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 the API case is it better to have no active profile after deleting than to switch to another one like we do in the extension?
mrubens
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.
Looks good to me. What do you think @cte?
| const currentSettings = this.getConfiguration() | ||
| const profiles = currentSettings.listApiConfigMeta || [] | ||
|
|
||
| if (profiles.some((profile) => profile.name === name)) { |
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.
Not a huge deal, but we might want to improve / expand the ProviderSettingsManager interface and just proxy these calls through to that object. It already has a few things you could use like fetching a profile by name or id. In general the current implementation seems fine though.
Left one comment about pushing more of the logic into |
Context
Continuing the work outlined in the How to configure Roo from outside of Roo? Discord thread, this PR adds several profile management functions to the Roo extension's exported API:
createProfile(name: string)getProfiles(): string[]setActiveProfile(name: string): Promise<void>getActiveProfile(): string | undefineddeleteProfile(name: string): Promise<void>I've tested all of these functions against our WIP Roo Configurator extension.
Implementation
Most of the changes of substance are in api.ts and build on the recent work to improve the configuration APIs.
I'm using names instead of profile IDs since the settings JSON seems to use them for specifying current active profile.
How to Test
Create a custom extension that calls these APIs and do something like this:
Important
Adds profile management functions to the API, enabling creation, retrieval, activation, and deletion of profiles, with updates to
RooCodeAPIinterface andgenerateIdmethod.APIclass inapi.ts:createProfile,getProfiles,setActiveProfile,getActiveProfile,deleteProfile.RooCodeAPIinterface ininterface.tsandroo-code.d.tsto include new profile management functions.generateIdmethod inProviderSettingsManager.tsto public to support profile ID generation.This description was created by
for d299739. It will automatically update as commits are pushed.