Closed
Conversation
dosubot
bot
added
size:XXL
Contributor
Re-review in progress. Outstanding items: 4.
Mention @roomote in a comment to request specific changes to this pull request or fix all unresolved issues. |
![roomote-v0[bot]](https://avatars.githubusercontent.com/in/1546624?s=60&v=4){kind=small}
hannesrudolph
force-pushed
the
settings-refactor
branch
from
86a98a3 to
88e9777
Compare
Comment on lines
+164
to
+166
| export function applySettingDefaults<T extends Partial<Record<SettingWithDefault, unknown>>>( | ||
| settings: T, | ||
| ): T & typeof settingDefaults { |
Contributor
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.
Suggested change
| 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.
Comment on lines
+164
to
+176
| 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 | ||
| } |
Contributor
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.
Suggested change
| 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.
|
|
||
| // Don't show the button if the current value matches the default | ||
| // undefined is treated as "using default" | ||
| const isDefault = currentValue === undefined || currentValue === defaultValue |
Contributor
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.
| {/* 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>{" "} |
Contributor
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.
- 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)
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.
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
- 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.
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
- Add 10 missing translation keys to all 17 supported locales: - common.true, common.false, common.empty, common.resetToDefault - sections.indexing - codeIndex.stopIndexingButton, codeIndex.configureInSettings - codeIndex.enableInSettings - codeIndex.apiKeyNote.title, codeIndex.apiKeyNote.description - Fix TerminalSettings component props to use the correct numeric settings (terminalOutputLineLimit, terminalOutputCharacterLimit, terminalCompressProgressBar) instead of the old enum-based terminalOutputPreviewSize - Remove unused Select component imports
- Add terminalOutputLineLimit, terminalOutputCharacterLimit to GlobalSettings schema - Add terminalCompressProgressBar to GlobalSettings schema - Keep terminalOutputPreviewSize for backward compatibility with OutputInterceptor - Add new terminal settings to ExtensionState type - Add terminalOutputLineLimit/CharacterLimit to ClineProvider getState destructuring
- Add terminalOutputLineLimit, terminalOutputCharacterLimit to SettingsView destructuring - Add terminalCompressProgressBar to SettingsView destructuring - Update TerminalSettings props to pass numeric settings instead of enum - Fix ExtensionStateContext setters for numeric terminal settings
hannesrudolph
force-pushed
the
settings-refactor
branch
from
4a71b09 to
9eafcc5
Compare
| @@ -399,32 +429,37 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t | |||
| terminalZdotdir, | |||
| terminalOutputPreviewSize: terminalOutputPreviewSize ?? "medium", | |||
Contributor
There was a problem hiding this comment.
terminalOutputPreviewSize is currently coerced to "medium" at save-time, which breaks the reset-to-default pattern (it will persist a value even when the user never set it, and prevents future default changes from flowing through). This should be passed through as-is and defaulted at read-time (similar to DEFAULT_TERMINAL_OUTPUT_PREVIEW_SIZE).
Suggested change
| terminalOutputPreviewSize: terminalOutputPreviewSize ?? "medium", | |
| terminalOutputPreviewSize, |
Fix it with Roo Code or mention @roomote and request a fix.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.