From 28ab2e77f208adbf202f16741fb55a2ae3d3a935 Mon Sep 17 00:00:00 2001 From: Hannes Rudolph Date: Fri, 23 Jan 2026 18:35:36 -0700 Subject: [PATCH] feat: clear settings matching defaults on every startup 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. --- src/core/config/ContextProxy.ts | 6 +- .../__tests__/settingsMigrations.spec.ts | 141 +++++++++++++++++- src/utils/settingsMigrations.ts | 69 ++++++++- 3 files changed, 207 insertions(+), 9 deletions(-) diff --git a/src/core/config/ContextProxy.ts b/src/core/config/ContextProxy.ts index 02af97144a..de48fa640d 100644 --- a/src/core/config/ContextProxy.ts +++ b/src/core/config/ContextProxy.ts @@ -21,7 +21,7 @@ import { TelemetryService } from "@roo-code/telemetry" import { logger } from "../../utils/logging" import { supportPrompt } from "../../shared/support-prompt" -import { runSettingsMigrations } from "../../utils/settingsMigrations" +import { runStartupSettingsMaintenance } from "../../utils/settingsMigrations" type GlobalStateKey = keyof GlobalState type SecretStateKey = keyof SecretState @@ -100,8 +100,8 @@ export class ContextProxy { // Migration: Clear old default condensing prompt so users get the improved v2 default await this.migrateOldDefaultCondensingPrompt() - // Migration: Clear hardcoded defaults so users can benefit from future default changes - await runSettingsMigrations(this) + // Settings maintenance: Run migrations and clear settings that match defaults + await runStartupSettingsMaintenance(this) this._isInitialized = true } diff --git a/src/utils/__tests__/settingsMigrations.spec.ts b/src/utils/__tests__/settingsMigrations.spec.ts index 6d8cb2b64c..8a8dfa38eb 100644 --- a/src/utils/__tests__/settingsMigrations.spec.ts +++ b/src/utils/__tests__/settingsMigrations.spec.ts @@ -1,6 +1,13 @@ -import { runSettingsMigrations, migrations, CURRENT_MIGRATION_VERSION } from "../settingsMigrations" +import { + runSettingsMigrations, + migrations, + CURRENT_MIGRATION_VERSION, + clearDefaultSettings, + runStartupSettingsMaintenance, +} from "../settingsMigrations" import type { ContextProxy } from "../../core/config/ContextProxy" import type { GlobalState } from "@roo-code/types" +import { settingDefaults } from "@roo-code/types" // Mock the logger vi.mock("../logging", () => ({ @@ -296,4 +303,136 @@ describe("settingsMigrations", () => { expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("codebaseIndexConfig", undefined) }) }) + + describe("clearDefaultSettings", () => { + it("should clear settings that match current defaults", async () => { + // Setup: user has settings that match current defaults + mockContextProxy.getGlobalState.mockImplementation((key: keyof GlobalState) => { + if (key === "browserToolEnabled") return settingDefaults.browserToolEnabled + if (key === "soundVolume") return settingDefaults.soundVolume + if (key === "maxWorkspaceFiles") return settingDefaults.maxWorkspaceFiles + return undefined + }) + + const clearedCount = await clearDefaultSettings(mockContextProxy as unknown as ContextProxy) + + // All matching defaults should be cleared + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("browserToolEnabled", undefined) + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("soundVolume", undefined) + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("maxWorkspaceFiles", undefined) + expect(clearedCount).toBe(3) + }) + + it("should preserve custom values that don't match defaults", async () => { + // Setup: user has custom values that don't match defaults + mockContextProxy.getGlobalState.mockImplementation((key: keyof GlobalState) => { + if (key === "browserToolEnabled") return false // default is true + if (key === "soundVolume") return 0.8 // default is 0.5 + if (key === "maxWorkspaceFiles") return 500 // default is 200 + return undefined + }) + + const clearedCount = await clearDefaultSettings(mockContextProxy as unknown as ContextProxy) + + // No settings should be cleared + expect(mockContextProxy.updateGlobalState).not.toHaveBeenCalled() + expect(clearedCount).toBe(0) + }) + + it("should not clear already undefined values", async () => { + // Setup: all settings are undefined + mockContextProxy.getGlobalState.mockReturnValue(undefined) + + const clearedCount = await clearDefaultSettings(mockContextProxy as unknown as ContextProxy) + + // No settings should be cleared (already undefined) + expect(mockContextProxy.updateGlobalState).not.toHaveBeenCalled() + expect(clearedCount).toBe(0) + }) + + it("should only clear settings in settingDefaults", async () => { + // Setup: user has settings - some in defaults, some not + mockContextProxy.getGlobalState.mockImplementation((key: keyof GlobalState) => { + if (key === "browserToolEnabled") return settingDefaults.browserToolEnabled + if (key === "customInstructions") return "my instructions" // not in settingDefaults + return undefined + }) + + await clearDefaultSettings(mockContextProxy as unknown as ContextProxy) + + // browserToolEnabled should be cleared + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("browserToolEnabled", undefined) + + // customInstructions should NOT be touched (not in settingDefaults) + expect(mockContextProxy.updateGlobalState).not.toHaveBeenCalledWith("customInstructions", undefined) + }) + + it("should handle string settings correctly", async () => { + mockContextProxy.getGlobalState.mockImplementation((key: keyof GlobalState) => { + if (key === "enterBehavior") return "send" // matches default + if (key === "language") return "en" // matches default + return undefined + }) + + await clearDefaultSettings(mockContextProxy as unknown as ContextProxy) + + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("enterBehavior", undefined) + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("language", undefined) + }) + + it("should return the count of cleared settings", async () => { + mockContextProxy.getGlobalState.mockImplementation((key: keyof GlobalState) => { + if (key === "browserToolEnabled") return true // matches default + if (key === "soundEnabled") return true // matches default + if (key === "soundVolume") return 0.8 // does NOT match default (0.5) + return undefined + }) + + const clearedCount = await clearDefaultSettings(mockContextProxy as unknown as ContextProxy) + + expect(clearedCount).toBe(2) // Only browserToolEnabled and soundEnabled match + }) + }) + + describe("runStartupSettingsMaintenance", () => { + it("should run both migrations and default clearing", async () => { + // Setup: migration not run, and has a setting matching default + mockContextProxy.getGlobalState.mockImplementation((key: keyof GlobalState) => { + if (key === "settingsMigrationVersion") return 0 + if (key === "browserToolEnabled") return true // matches both historical and current default + return undefined + }) + + await runStartupSettingsMaintenance(mockContextProxy as unknown as ContextProxy) + + // Should have updated migration version + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith( + "settingsMigrationVersion", + CURRENT_MIGRATION_VERSION, + ) + + // browserToolEnabled should be cleared (by migration or clearDefaults) + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("browserToolEnabled", undefined) + }) + + it("should run clearDefaultSettings even after migrations are complete", async () => { + // Setup: migrations already complete, but has setting matching default + mockContextProxy.getGlobalState.mockImplementation((key: keyof GlobalState) => { + if (key === "settingsMigrationVersion") return CURRENT_MIGRATION_VERSION + if (key === "soundVolume") return 0.5 // matches current default + return undefined + }) + + await runStartupSettingsMaintenance(mockContextProxy as unknown as ContextProxy) + + // Migration version should NOT be updated (already current) + expect(mockContextProxy.updateGlobalState).not.toHaveBeenCalledWith( + "settingsMigrationVersion", + expect.anything(), + ) + + // soundVolume should be cleared by clearDefaultSettings + expect(mockContextProxy.updateGlobalState).toHaveBeenCalledWith("soundVolume", undefined) + }) + }) }) diff --git a/src/utils/settingsMigrations.ts b/src/utils/settingsMigrations.ts index 500a580e74..8ebe6044e6 100644 --- a/src/utils/settingsMigrations.ts +++ b/src/utils/settingsMigrations.ts @@ -1,16 +1,21 @@ /** - * Settings migrations for version-gated migration of hardcoded defaults. + * Settings migrations and defaults cleanup. * - * This module tracks which migrations have been applied and runs any pending - * migrations when the extension starts. Each migration targets specific - * historical default values that were being hardcoded in storage before - * the "reset to default" pattern fix. + * This module provides two mechanisms for managing settings: + * + * 1. **Version-gated migrations**: Run once per version to handle specific + * migration scenarios (e.g., flattening nested configs). + * + * 2. **Every-startup defaults clearing**: Clears settings that exactly match + * their current default values on every startup. This ensures users always + * benefit from default value improvements. * * See plans/reset-to-default-ideal-pattern.md for the full design. */ import type { ContextProxy } from "../core/config/ContextProxy" import type { GlobalState, CodebaseIndexConfig } from "@roo-code/types" +import { settingDefaults, type SettingWithDefault } from "@roo-code/types" import { logger } from "./logging" @@ -175,3 +180,57 @@ export async function runSettingsMigrations(contextProxy: ContextProxy): Promise await contextProxy.updateGlobalState("settingsMigrationVersion", CURRENT_MIGRATION_VERSION) logger.info(`Settings migration complete. Now at version ${CURRENT_MIGRATION_VERSION}`) } + +/** + * Clears settings that exactly match their current default values. + * + * This function runs on every startup to ensure users always benefit from + * default value improvements. When a setting's stored value exactly matches + * the current default, it's cleared (set to undefined) so the default is + * applied at read time. + * + * Note: This approach means users cannot "lock in" a value that happens to + * match the default. If they explicitly set browserToolEnabled=true (the default), + * it will be cleared and they'll use whatever the default is in the future. + * + * @param contextProxy - The ContextProxy instance for reading/writing state + * @returns The number of settings that were cleared + */ +export async function clearDefaultSettings(contextProxy: ContextProxy): Promise { + let clearedCount = 0 + + for (const key of Object.keys(settingDefaults) as SettingWithDefault[]) { + const storedValue = contextProxy.getGlobalState(key as keyof GlobalState) + const defaultValue = settingDefaults[key] + + // Only clear if stored value exactly matches the current default + // undefined values are already "default" so skip them + if (storedValue !== undefined && storedValue === defaultValue) { + await contextProxy.updateGlobalState(key as keyof GlobalState, undefined) + logger.info(`Cleared default setting: ${key} (was ${JSON.stringify(storedValue)})`) + clearedCount++ + } + } + + if (clearedCount > 0) { + logger.info(`Cleared ${clearedCount} settings that matched their defaults`) + } + + return clearedCount +} + +/** + * Runs all startup settings maintenance tasks. + * + * This is the main entry point that should be called on extension startup. + * It runs both migrations (once per version) and defaults clearing (every startup). + * + * @param contextProxy - The ContextProxy instance for reading/writing state + */ +export async function runStartupSettingsMaintenance(contextProxy: ContextProxy): Promise { + // First run any pending migrations + await runSettingsMigrations(contextProxy) + + // Then clear any settings that match current defaults + await clearDefaultSettings(contextProxy) +}