-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: add organization default provider settings support #6221
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
|
|
||
| // Only apply defaults if the current value is undefined or empty | ||
| const currentValue = apiConfiguration[key as keyof ProviderSettings] | ||
| if (!currentValue || (typeof currentValue === "string" && currentValue.trim() === "")) { |
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.
Avoid using '!currentValue' to check if a field is empty. This check will override valid falsy values (like 0 or false). Instead, explicitly check for undefined or null (e.g., currentValue === undefined || currentValue === null || (typeof currentValue === 'string' && currentValue.trim() === '')).
| if (!currentValue || (typeof currentValue === "string" && currentValue.trim() === "")) { | |
| if (currentValue === undefined || currentValue === null || (typeof currentValue === "string" && currentValue.trim() === "")) { |
|
|
||
| // Only apply defaults if the current value is undefined or empty | ||
| const currentValue = apiConfiguration[key as keyof ProviderSettings] | ||
| if (!currentValue || (typeof currentValue === "string" && currentValue.trim() === "")) { |
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 falsy value check will incorrectly override valid falsy values. For example, if an organization sets temperature: 0 or a boolean setting to false, these valid values would be replaced with defaults.
Consider using a more explicit check:
if (currentValue === undefined || currentValue === null || (typeof currentValue === "string" && currentValue.trim() === "")) {This issue was already raised by ellipsis-dev[bot] but hasn't been addressed yet.
| // Get organization settings including default provider settings | ||
| let organizationDefaultProviderSettings: Record<string, any> = {} | ||
| try { | ||
| const orgSettings = await CloudService.instance.getOrganizationSettings() |
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 getOrganizationSettings() fails, the error is logged but organizationDefaultProviderSettings remains an empty object. This silent failure might be confusing for users who expect defaults to be applied.
Consider:
- Adding a user-visible notification when organization defaults fail to load
- Or propagating partial data if some defaults are available
- Or adding a flag to indicate whether defaults were successfully loaded
This would help users understand why their expected defaults aren't being applied.
|
|
||
| if (orgDefaults) { | ||
| // Apply each default setting from the organization | ||
| Object.entries(orgDefaults).forEach(([key, defaultValue]) => { |
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.
There's a potential race condition here. When the provider changes, defaults are applied immediately in the same function, but the provider change might trigger other async operations (like fetching available models).
Consider using a useEffect hook to apply defaults after the provider change has been processed:
useEffect(() => {
const orgDefaults = organizationDefaultProviderSettings?.[apiConfiguration.apiProvider];
if (orgDefaults && isNewProfile) { // You'd need to track if this is a new profile
// Apply defaults here
}
}, [apiConfiguration.apiProvider, organizationDefaultProviderSettings]);This would ensure defaults are applied at the right time in the component lifecycle.
This PR adds support for organizations to define default provider settings that are automatically applied when users switch to a provider for the first time.
Changes
defaultProviderSettingsfield to organization settings schemaHow it works
Testing
Important
Adds support for organization-defined default provider settings, applied when users switch providers, with schema, state, and UI updates.
defaultProviderSettingsto organization settings schema incloud.ts.CloudServiceinCloudService.tsto expose organization settings including defaults.webviewMessageHandler.tsto apply defaults when creating new profiles or switching providers.ApiOptions.tsxapplies organization defaults when provider changes.cloud-schema.test.tsfor schema validation.webviewMessageHandler-orgDefaults.spec.tsfor message handler behavior.ExtensionStateContext.tsxto includeorganizationDefaultProviderSettingsin state.ExtensionStateContext.This description was created by
for 83b3cc3. You can customize this summary. It will automatically update as commits are pushed.