-
Notifications
You must be signed in to change notification settings - Fork 2.8k
refactor: flatten image generation settings structure #7536
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
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 several issues that need attention before this can be merged. The refactoring to flatten the image generation settings structure is a good improvement, but there are some critical issues to address.
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? Adding SecretState directly to RooCodeSettings creates a type intersection that might cause confusion. The SecretState type now includes both provider secrets and global secrets. Consider documenting this change or using a more explicit type composition:
| export type RooCodeSettings = GlobalSettings & ProviderSettings & SecretState | |
| export type RooCodeSettings = GlobalSettings & ProviderSettings & { openRouterImageApiKey?: string } |
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.
Could we add JSDoc comments here to explain the distinction between GLOBAL_SECRET_KEYS and SECRET_STATE_KEYS? This would help future maintainers understand why we need two separate arrays for secrets.
src/core/webview/ClineProvider.ts
Outdated
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 cast to any here breaks type safety. Could we update the type definitions to properly include openRouterImageApiKey in the state values type? This suggests the types aren't properly aligned with the actual data structure.
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.
For consistency with other model ID fields in the codebase, should this be named imageGenerationModelId instead of imageGenerationSelectedModel?
Additional Review NotesI noticed a couple of additional issues that need attention: 1. Merge ConflictsThe PR currently has merge conflicts that need to be resolved before it can be merged. Please rebase or merge the latest changes from the main branch. 2. Migration LogicSince this PR removes the nested
This could be done in the 3. Test CoverageWhile the tests have been updated to work with the new structure, consider adding specific test cases for:
Overall, this is a good refactoring that simplifies the codebase! Once these issues are addressed, it should be ready to merge. |
- Move openRouterImageApiKey and imageGenerationSelectedModel to top-level - Remove nested imageGenerationSettings object - Update all dependent components and tests - Handle secrets properly with GLOBAL_SECRET_KEYS - Simplify ContextProxy to handle flattened structure
This fixes TypeScript error when setting global secret keys
5e042fd to
1744d47
Compare
- Added openRouterImageApiKey to GlobalSettings schema instead of including all SecretState - Fixed compilation errors by properly typing the settings - Updated tests to account for GLOBAL_SECRET_KEYS - Added migration logic to convert old nested settings to flattened structure - Added comprehensive tests for migration functionality
- Adjusted test expectations for GLOBAL_SECRET_KEYS.length - Updated initialize test to check for migration call - Fixed mock counts to reflect new openRouterImageApiKey secret
|
|
||
| // Image generation settings (experimental) - flattened for simplicity | ||
| openRouterImageApiKey: z.string().optional(), | ||
| imageGenerationSelectedModel: z.string().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.
Should this be openRouterImageGenerationSelectedModel?
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.
Should we leave this generic in case we want to add other models from other providers?
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.
Yeah I'm just not sure the right structure to support multiple providers. If we don't care about preserving the key when you switch between them, we might want imageApiProvider, imageApiKey, imageApiModelId?
…SelectedModel for consistency
Summary
This PR refactors the image generation settings to use a flattened structure instead of nested objects, simplifying the codebase and improving maintainability.
Changes
Core Changes
openRouterImageApiKeyandimageGenerationSelectedModelfrom nestedimageGenerationSettingsobject to top-level propertiesimageGenerationSettingsnested object entirelyGLOBAL_SECRET_KEYSarray to handle global secrets properlySecretStatetype to include global secret keysContextProxy Simplification
NESTED_SECRET_PATHS,hasNestedSecrets, etc.)getValue,setValue, andgetValuesmethodsextractAndStoreNestedSecretsandmergeNestedSecretshelper functionsComponent Updates
ImageGenerationSettingscomponent to accept flattened propsExperimentalSettingsto pass flattened propsSettingsViewto handle flattened state structureType Fixes
SecretStatetoRooCodeSettingstype definition to properly handle global secretsExtensionStatetype to includeopenRouterImageApiKeyas an optional propertyBenefits
Testing
Important
Refactor image generation settings to a flattened structure, simplifying code and improving maintainability across components and types.
openRouterImageApiKeyandimageGenerationSelectedModelto top-level properties inglobal-settings.ts.imageGenerationSettingsnested object.GLOBAL_SECRET_KEYSfor global secrets.SecretStatetype to include global secret keys.ContextProxy.ts.getValue,setValue, andgetValuesmethods.extractAndStoreNestedSecretsandmergeNestedSecretsfunctions.ImageGenerationSettingsandExperimentalSettingscomponents to use flattened props.SettingsViewto handle flattened state structure.ImageGenerationSettings.spec.tsx.ExtensionStatetype inExtensionMessage.tsto includeopenRouterImageApiKeyandopenRouterImageGenerationSelectedModel.webviewMessageHandler.tsto handle new message types for image generation settings.This description was created by
for bcc0c25. You can customize this summary. It will automatically update as commits are pushed.