-
Notifications
You must be signed in to change notification settings - Fork 2.6k
fix: properly handle Gemini settings with schema-level defaults #6741
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
- Move enableUrlContext and enableGrounding to base schema as optional - Make these fields required (non-optional) for Gemini provider specifically - Add migration to set default values (true) for existing Gemini configs - Clean up these fields from non-Gemini providers - Ensure Gemini configs always have these fields defined This eliminates the undefined state that was causing the Save button issue, fixing it at the data layer rather than working around it in the UI. Fixes #6616
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.
Thank you for your contribution! I've reviewed the changes and found some issues that need attention.
| } | ||
| } | ||
|
|
||
| private async migrateEnableUrlContextAndGrounding(providerProfiles: ProviderProfiles) { |
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.
Missing test coverage for this new migration. This is a critical migration that affects data integrity and should be thoroughly tested. Could we add tests similar to the other migrations (like migrateTodoListEnabled at line 190)?
| } | ||
|
|
||
| // Only run migration if either flag is false | ||
| if ( |
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.
Is this intentional? The migration checks and updates are done separately for each flag. If an error occurs between updating the flags, it could leave the migration in an inconsistent state. Consider combining both flag updates into a single operation:
| if ( | |
| if ( | |
| !providerProfiles.migrations.enableUrlContextMigrated || | |
| !providerProfiles.migrations.enableGroundingMigrated | |
| ) { | |
| await this.migrateEnableUrlContextAndGrounding(providerProfiles) | |
| providerProfiles.migrations.enableUrlContextMigrated = true | |
| providerProfiles.migrations.enableGroundingMigrated = true | |
| isDirty = true | |
| } |
| // Only apply migration to gemini provider settings | ||
| if (apiConfig.apiProvider === "gemini") { | ||
| // Type assertion to access gemini-specific properties | ||
| const geminiConfig = apiConfig as any |
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 use of any type assertions here bypasses TypeScript's type checking. Could we consider using proper type guards or creating a more specific type for Gemini configurations? This would improve type safety and make the code more maintainable.
| providerProfiles.apiConfigs[name] = { ...filteredConfig, id } | ||
|
|
||
| // Handle gemini-specific fields - only ensure they exist for Gemini configs | ||
| if (filteredConfig.apiProvider === "gemini") { |
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 logic for ensuring Gemini fields have defaults is duplicated in export (lines 536-547) and in the load method (lines 599-610). Could we extract this into a helper function to reduce duplication? Something like:
| if (filteredConfig.apiProvider === "gemini") { | |
| private ensureGeminiDefaults(config: any): void { | |
| if (config.apiProvider === "gemini") { | |
| config.enableUrlContext = config.enableUrlContext ?? true | |
| config.enableGrounding = config.enableGrounding ?? true | |
| } else { | |
| delete config.enableUrlContext | |
| delete config.enableGrounding | |
| } | |
| } |
| modelMaxThinkingTokens: z.number().optional(), | ||
|
|
||
| // URL context and grounding (optional for most providers, required for Gemini) | ||
| enableUrlContext: z.boolean().optional(), |
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.
Good approach moving these to the base schema as optional fields! This provides flexibility while maintaining backward compatibility. The comment clearly explains the intent.
|
Closing this PR as the schema-level approach introduces TypeScript compilation issues with the cloud package. The fix needs more comprehensive testing and coordination with the types system. |
Description
This PR provides a proper fix for the Gemini settings checkbox issue by addressing it at the schema/data layer rather than working around it in the UI.
Problem
When the Gemini settings checkboxes (
enableUrlContextandenableGrounding) don't exist in the saved configuration, they have anundefinedvalue. This causes the Save button to not activate when toggling these checkboxes.Solution
This PR takes a comprehensive approach similar to commit 6aea8990:
Schema Changes:
enableUrlContextandenableGroundingto the base schema as optional fields (available to all providers)Migration System:
true) for existing Gemini configsData Layer Handling:
ProviderSettingsManagerto properly handle these fieldsBenefits
Alternative Approach
This supersedes PR #6617 which attempted to fix this at the UI layer. The schema-level approach is more robust and prevents the issue from occurring in the first place.
Testing
trueby defaultFixes #6616
Important
Fix Gemini settings handling by updating schema and data layer to ensure required fields are always defined for Gemini and cleaned up for non-Gemini providers.
enableUrlContextandenableGroundingto base schema as optional fields inprovider-settings.ts.geminiSchema.ProviderSettingsManager.tsto set default values for existing Gemini configs.ProviderSettingsManagerto ensure Gemini configs have these fields with defaults.This description was created by
for e15ec38. You can customize this summary. It will automatically update as commits are pushed.