-
Notifications
You must be signed in to change notification settings - Fork 2.6k
Wrangle our settings-related types and add support for settings import / export #1997
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
🦋 Changeset detectedLatest commit: 316bfce The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This pull request is quite large, with 26 changed files, 1622 lines added, and 769 lines removed. It includes various refactors, feature additions, and test updates across multiple files and modules. To make it easier to review and manage, consider splitting the changes into smaller, more focused pull requests. For example, you could separate the feature additions related to settings import/export from the refactoring of type definitions and state management. This would help reviewers focus on specific areas and ensure thorough review of each part. |
| awsRegion: true, | ||
| awsUseCrossRegionInference: true, | ||
| awsUsePromptCache: true, | ||
| awspromptCacheId: true, |
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 key awspromptCacheId appears to have inconsistent camel casing compared to other AWS keys (e.g., awsAccessKey). Consider renaming it to awsPromptCacheId for consistency.
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.
Yes; unfortunate that we named it like this. We need a playbook for settings migrations.
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
Co-authored-by: ellipsis-dev[bot] <65095814+ellipsis-dev[bot]@users.noreply.github.com>
|
|
||
| export type Values<T> = T[keyof T] | ||
|
|
||
| export type Equals<X, Y> = (<T>() => T extends X ? 1 : 2) extends <T>() => T extends Y ? 1 : 2 ? true : false |
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 Equals utility type is doing a lot of heavy lifting in this PR to ensure that our zod schemas match the roo-code.d.ts type definitions. Ideally we'd infer the types from the schema and nicely DRY things up, but the type definition file can only have type declarations in it.
| } | ||
|
|
||
| export interface ExtensionState { | ||
| export type ExtensionState = Pick< |
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 ExtensionState can mostly be generate by Picking from GlobalSettings (this is better since it ensures that all the types match between the two), but there are some annoying differences that we can eventually clean up and make this look tidy.
| const providerNamesEnum: [ProviderName, ...ProviderName[]] = [ | ||
| PROVIDER_NAMES[0], | ||
| ...PROVIDER_NAMES.slice(1).map((p) => p), | ||
| ] |
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.
You'll see this technique employed in a few places. This a trick that allows you to convert a typescript string union type to an array that can be passed to zod.enum. I've named all of these to reflect this.
| modes: true, | ||
| } | ||
|
|
||
| const TOOL_GROUPS = Object.keys(toolGroups) as ToolGroup[] |
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 pattern converting Object.keys(...) to a tuple (used for z.enum) is repeated (also seen with languages and telemetry settings). Consider extracting this logic into a reusable helper function to reduce duplication and improve maintainability.
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.
@ellipsis-dev Can you respond to this comment with a quick example of how you might do that in a strongly-typed and fully type-safe way?
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.
Here's a quick example of converting Object.keys to a tuple for z.enum in a strongly-typed and type-safe way:
const myEnumValues = {
value1: true,
value2: true,
value3: true,
};
const MY_ENUM = Object.keys(myEnumValues) as (keyof typeof myEnumValues)[];
const myEnumSchema = z.enum(MY_ENUM);This approach ensures that the keys are strongly typed and can be used safely with z.enum. Let me know if you need further clarification!
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.
@ellipsis-dev This isn't exactly what you mentioned; you said this can be done with a reusable helper. Where's the helper?
src/core/webview/ClineProvider.ts
Outdated
| const uri = await vscode.window.showSaveDialog({ | ||
| filters: { JSON: ["json"] }, | ||
| defaultUri: vscode.Uri.file( | ||
| path.join(os.homedir(), "Documents", `roo-code-${message.text}.json`), |
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.
Does this default make sense in all OSes? What happens if we don't set a default?
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.
If you don't set a default then it defaults to the last directory you saved something to (if any), and the named is "Untitled.json".
| "custom_storage_path_set": "自定義存儲路徑已設置:{{path}}", | ||
| "default_storage_path": "已恢復使用默認存儲路徑" | ||
| "default_storage_path": "已恢復使用默認存儲路徑", | ||
| "settings_imported": "設置已成功導入。" |
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 newly added translation for settings_imported ends with a Chinese full stop (。) which is inconsistent with similar entries in this file (e.g., default_storage_path does not end with punctuation). Consider making the punctuation consistent across these messages.
| "settings_imported": "設置已成功導入。" | |
| "settings_imported": "設置已成功導入" |
…iles - Backfill `modeApiConfigs` on initialize if absent, seeding with the current profile - Ensure `setModeConfig()` always mutates and persists the real modeApiConfigs map - Addresses regression introduced in PR RooCodeInc#1997
Context
roo-code.d.ts.ApiConfigurationwas strongly typed previously).ProviderSettingsandGlobalSettings). Provider settings are specific to a given API provider profile and change when you select a different profile. Global settings apply irrespective of your current profile.Possible TODOs:
roo-code.d.tsand make the zod schemas the source of truth? I think that would significantly reduce the amount of typing boilerplate.Implementation
How to Test
Get in Touch
Important
Refactor settings management by introducing
ProviderSettingsandGlobalSettings, adding import/export functionality, and updating related tests and UI components.roo-code.d.ts.ProviderSettingsandGlobalSettingsto differentiate between provider-specific and global settings.globalState.ts.importExport.ts.ConfigManagerwithProviderSettingsManagerinClineProvider.ts.ContextProxyfor managing global and secret state.ContextProxy.test.tsandProviderSettingsManager.test.tsto reflect new settings management.importExport.test.tsfor testing import/export functionality.About.tsx.This description was created by
for 316bfce. It will automatically update as commits are pushed.