settings refactor #10928
settings refactor #10928
Conversation
dosubot
bot
added
size:XXL
Re-review in progress. Outstanding items: 3.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
| const { codebaseIndexModels, apiConfiguration } = useExtensionState() | ||
|
|
||
| // Extract values from nested config | ||
| const codebaseIndexEnabled = codebaseIndexConfig?.codebaseIndexEnabled ?? true |
There was a problem hiding this comment.
codebaseIndexEnabled defaults to true here, but the new centralized defaults set indexing to disabled by default. This makes the indexing UI render as enabled for users who have never configured it, and can also lead to persisting a non-default config just by saving settings.
| const codebaseIndexEnabled = codebaseIndexConfig?.codebaseIndexEnabled ?? true | |
| const codebaseIndexEnabled = codebaseIndexConfig?.codebaseIndexEnabled ?? false |
Fix it with Roo Code or mention @roomote and request a fix.
packages/types/src/defaults.ts
Outdated
| // Prompt enhancement settings | ||
| condensingApiConfigId: "", | ||
| includeTaskHistoryInEnhance: true, |
There was a problem hiding this comment.
settingDefaults still includes condensingApiConfigId, but the PR removes this setting from the global settings schema and extension state. Keeping it in the defaults registry makes it easy for future code/tests to accidentally reintroduce a removed setting or assert on a key that should no longer exist.
| // Prompt enhancement settings | |
| condensingApiConfigId: "", | |
| includeTaskHistoryInEnhance: true, | |
| // Prompt enhancement settings | |
| includeTaskHistoryInEnhance: true, |
Fix it with Roo Code or mention @roomote and request a fix.
- Add settingDefaults registry in packages/types/src/defaults.ts - Add settings migration framework with versioned migrations - Flatten codebaseIndexConfig to top-level keys for consistency - Create new IndexingSettings UI component in Settings view - Update SettingsView to pass undefined instead of coerced defaults - Update ExtensionStateContext types to support optional settings - Add comprehensive tests for defaults and migrations
- Change codebaseIndexEnabled default from true to false in IndexingSettings.tsx - Remove condensingApiConfigId from settingDefaults (setting removed from schema)
hannesrudolph
force-pushed
the
settings-refactor
branch
from
86a98a3 to
88e9777
Compare
This implements 'Option 2' - every-startup clearing of default values. - Add clearDefaultSettings() function that checks all settings in settingDefaults and clears any that exactly match the default - Add runStartupSettingsMaintenance() as the main entry point that runs both migrations (once) and default clearing (every startup) - Update ContextProxy to use runStartupSettingsMaintenance - Add comprehensive tests for the new functionality This ensures users always benefit from default value improvements. Note: Users cannot 'lock in' a value that matches the default.
IMPORTANT: Every setting with a default value is now in settingDefaults. This enables clearDefaultSettings() to properly clear ALL default values from storage on every startup, ensuring defaults are never persisted. Added settings: - Auto-approval: autoApprovalEnabled, alwaysAllowReadOnly, alwaysAllowWrite, alwaysAllowBrowser, alwaysAllowMcp, alwaysAllowModeSwitch, alwaysAllowSubtasks, alwaysAllowExecute, requestDelaySeconds, followupAutoApproveTimeoutMs, etc. - Terminal: terminalShellIntegrationDisabled, terminalCommandDelay, terminalPowershellCounter, terminalZsh*, terminalCompressProgressBar - Other: diagnosticsEnabled, historyPreviewCollapsed, hasOpenedModeSelector, enableMcpServerCreation, rateLimitSeconds Removed settings: - diffEnabled, fuzzyMatchThreshold (removed from schema) Updated test expectations to match new comprehensive defaults list.
| export function applySettingDefaults<T extends Partial<Record<SettingWithDefault, unknown>>>( | ||
| settings: T, | ||
| ): T & typeof settingDefaults { |
There was a problem hiding this comment.
Minor API mismatch: the PR summary mentions an applySettingsDefaults() helper, but the exported helper is applySettingDefaults() (singular) and currently unused. If any callers import the plural name, it will fail at build time; consider renaming or adding a compat alias.
| export function applySettingDefaults<T extends Partial<Record<SettingWithDefault, unknown>>>( | |
| settings: T, | |
| ): T & typeof settingDefaults { | |
| export function applySettingsDefaults<T extends Partial<Record<SettingWithDefault, unknown>>>( | |
| settings: T, | |
| ): T & typeof settingDefaults { |
Fix it with Roo Code or mention @roomote and request a fix.
EMBEDDING_MODEL_PROFILES was being stored in globalState on every ClineProvider initialization, but this is unnecessary - it's static reference data that should be passed directly to the webview. Changes: - Remove line that stored EMBEDDING_MODEL_PROFILES in globalState - The webview still receives the data via the ?? fallback in getState() - Add migration v3 to clean up existing codebaseIndexModels keys - Add tests for migration v3
| export function applySettingDefaults<T extends Partial<Record<SettingWithDefault, unknown>>>( | ||
| settings: T, | ||
| ): T & typeof settingDefaults { | ||
| const result = { ...settings } as T & typeof settingDefaults | ||
|
|
||
| for (const key of Object.keys(settingDefaults) as SettingWithDefault[]) { | ||
| if (result[key] === undefined) { | ||
| ;(result as Record<SettingWithDefault, unknown>)[key] = settingDefaults[key] | ||
| } | ||
| } | ||
|
|
||
| return result | ||
| } |
There was a problem hiding this comment.
Minor naming mismatch: defaults.ts exports applySettingDefaults() (singular) but the PR summary and the existing top-level TODO refer to applySettingsDefaults() (plural). Consider renaming to plural or adding a compat alias to avoid a future import typo.
| export function applySettingDefaults<T extends Partial<Record<SettingWithDefault, unknown>>>( | |
| settings: T, | |
| ): T & typeof settingDefaults { | |
| const result = { ...settings } as T & typeof settingDefaults | |
| for (const key of Object.keys(settingDefaults) as SettingWithDefault[]) { | |
| if (result[key] === undefined) { | |
| ;(result as Record<SettingWithDefault, unknown>)[key] = settingDefaults[key] | |
| } | |
| } | |
| return result | |
| } | |
| export function applySettingsDefaults<T extends Partial<Record<SettingWithDefault, unknown>>>( | |
| settings: T, | |
| ): T & typeof settingDefaults { | |
| const result = { ...settings } as T & typeof settingDefaults | |
| for (const key of Object.keys(settingDefaults) as SettingWithDefault[]) { | |
| if (result[key] === undefined) { | |
| ;(result as Record<SettingWithDefault, unknown>)[key] = settingDefaults[key] | |
| } | |
| } | |
| return result | |
| } |
Fix it with Roo Code or mention @roomote and request a fix.
- Add ResetToDefault component with ↺ icon that shows when a setting differs from its default value - Tooltip shows the default value (e.g., 'Reset to default (true)') - Component only renders when current value !== default - Add translation keys for boolean/empty/resetToDefault formatting - Integrate with BrowserSettings as proof of concept: - browserToolEnabled checkbox - browserViewportSize dropdown - screenshotQuality slider - Add comprehensive tests for visibility, functionality, and styling
- Use settingDefaults fallback for checkbox checked state - Use settingDefaults fallback for conditional rendering - Use settingDefaults fallback for Select value This ensures that when a setting is reset to undefined (meaning 'use default'), the UI displays the actual default value instead of showing undefined/empty state.
|
|
||
| // Don't show the button if the current value matches the default | ||
| // undefined is treated as "using default" | ||
| const isDefault = currentValue === undefined || currentValue === defaultValue |
There was a problem hiding this comment.
ResetToDefault currently treats undefined as "using default" and will hide the button even when a caller passes undefined while the stored value differs from the default (e.g. before saving). If you want a true "reset-from-dirty" indicator in the UI, this component likely needs either the stored value or an explicit isUsingDefault/storedValue signal instead of inferring from undefined.
Fix it with Roo Code or mention @roomote and request a fix.
The webviewMessageHandler was passing section as 'tab' property but App.tsx expects it in 'values.section'. This fixes the navigation from CodeIndexPopover 'Configure in Settings' link to the indexing tab.
- Replace hardcoded English text with i18n translation keys - Add apiKeyNote.title and apiKeyNote.description to settings.json - Note explains that API keys are shared with provider configuration
| {/* Note about API keys */} | ||
| <div className="mt-4 p-3 bg-vscode-inputValidation-infoBackground border border-vscode-inputValidation-infoBorder rounded"> | ||
| <p className="text-sm text-vscode-inputValidation-infoForeground m-0"> | ||
| <strong>{t("settings:codeIndex.apiKeyNote.title")}</strong>{" "} |
There was a problem hiding this comment.
IndexingSettings now uses settings:codeIndex.apiKeyNote.*, but this key only exists in English; other locales under webview-ui/src/i18n/locales/*/settings.json will likely show the raw key or fallback unexpectedly. Consider adding codeIndex.apiKeyNote.title/description to the other locale files (even as English placeholders) to avoid missing-translation UI.
Fix it with Roo Code or mention @roomote and request a fix.
Summary
Implements a reset-to-default pattern for settings management, enabling users to benefit from future default improvements without losing their customizations.
Changes
Centralized defaults registry (packages/types/src/defaults.ts): New
settingDefaultsobject as source of truth for all setting defaults, with helper functionsgetSettingWithDefault()andapplySettingsDefaults()Settings migration framework (src/utils/settingsMigrations.ts): Versioned migration system to clear hardcoded defaults from storage
codebaseIndexConfigto top-level keysFlattened indexing settings (packages/types/src/global-settings.ts): 12 new top-level schema fields for codebase indexing configuration
New IndexingSettings UI (webview-ui/src/components/settings/IndexingSettings.tsx): Dedicated settings tab for codebase indexing with full provider configuration (OpenAI, Ollama, Gemini, Mistral, Bedrock, OpenRouter)
SettingsView pattern change (webview-ui/src/components/settings/SettingsView.tsx): Values passed directly without coercion;
undefinedremoves setting from storageType updates (webview-ui/src/context/ExtensionStateContext.tsx): Several settings made optional to support the
undefined= "use default" patternBackend updates (src/services/code-index/config-manager.ts): Reads flat keys from globalState with defaults applied at read time
Comprehensive tests: New test suites for defaults module, migrations, and IndexingSettings component
Important
Refactor settings management to support reset-to-default pattern, centralize defaults, and update UI and tests accordingly.
settingDefaults.getSettingWithDefault()andapplySettingsDefaults()indefaults.ts.settingsMigrations.tsto clear hardcoded defaults and flattencodebaseIndexConfig.IndexingSettingscomponent inIndexingSettings.tsxfor codebase indexing configuration.SettingsView.tsxto pass values directly without coercion.ResetToDefaultcomponent for UI reset functionality.ResetToDefaultandIndexingSettingscomponents.This description was created by
for 4a71b09. You can customize this summary. It will automatically update as commits are pushed.