From b4ebacc012cceb1295909a6eb752f065ed9f4b8a Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Tue, 15 Apr 2025 14:00:30 -0400 Subject: [PATCH 1/2] Move diff editing config to provider settings --- src/core/config/ProviderSettingsManager.ts | 48 +++++++++++- src/exports/roo-code.d.ts | 2 + src/exports/types.ts | 2 + src/schemas/index.ts | 4 + .../components/settings/AdvancedSettings.tsx | 75 ------------------- .../src/components/settings/ApiOptions.tsx | 6 ++ .../settings/DiffSettingsControl.tsx | 68 +++++++++++++++++ .../settings/ExperimentalSettings.tsx | 4 +- .../src/components/settings/SettingsView.tsx | 14 ---- 9 files changed, 130 insertions(+), 93 deletions(-) delete mode 100644 webview-ui/src/components/settings/AdvancedSettings.tsx create mode 100644 webview-ui/src/components/settings/DiffSettingsControl.tsx diff --git a/src/core/config/ProviderSettingsManager.ts b/src/core/config/ProviderSettingsManager.ts index 212a673b95..9956c4b095 100644 --- a/src/core/config/ProviderSettingsManager.ts +++ b/src/core/config/ProviderSettingsManager.ts @@ -16,6 +16,7 @@ export const providerProfilesSchema = z.object({ migrations: z .object({ rateLimitSecondsMigrated: z.boolean().optional(), + diffSettingsMigrated: z.boolean().optional(), }) .optional(), }) @@ -36,6 +37,7 @@ export class ProviderSettingsManager { modeApiConfigs: this.defaultModeApiConfigs, migrations: { rateLimitSecondsMigrated: true, // Mark as migrated on fresh installs + diffSettingsMigrated: true, // Mark as migrated on fresh installs }, } @@ -85,7 +87,10 @@ export class ProviderSettingsManager { // Ensure migrations field exists if (!providerProfiles.migrations) { - providerProfiles.migrations = { rateLimitSecondsMigrated: false } // Initialize with default values + providerProfiles.migrations = { + rateLimitSecondsMigrated: false, + diffSettingsMigrated: false, + } // Initialize with default values isDirty = true } @@ -95,6 +100,12 @@ export class ProviderSettingsManager { isDirty = true } + if (!providerProfiles.migrations.diffSettingsMigrated) { + await this.migrateDiffSettings(providerProfiles) + providerProfiles.migrations.diffSettingsMigrated = true + isDirty = true + } + if (isDirty) { await this.store(providerProfiles) } @@ -129,6 +140,41 @@ export class ProviderSettingsManager { } } + private async migrateDiffSettings(providerProfiles: ProviderProfiles) { + try { + let diffEnabled: boolean | undefined + let fuzzyMatchThreshold: number | undefined + + try { + diffEnabled = await this.context.globalState.get("diffEnabled") + fuzzyMatchThreshold = await this.context.globalState.get("fuzzyMatchThreshold") + } catch (error) { + console.error("[MigrateDiffSettings] Error getting global diff settings:", error) + } + + if (diffEnabled === undefined) { + // Failed to get the existing value, use the default. + diffEnabled = true + } + + if (fuzzyMatchThreshold === undefined) { + // Failed to get the existing value, use the default. + fuzzyMatchThreshold = 1.0 + } + + for (const [name, apiConfig] of Object.entries(providerProfiles.apiConfigs)) { + if (apiConfig.diffEnabled === undefined) { + apiConfig.diffEnabled = diffEnabled + } + if (apiConfig.fuzzyMatchThreshold === undefined) { + apiConfig.fuzzyMatchThreshold = fuzzyMatchThreshold + } + } + } catch (error) { + console.error(`[MigrateDiffSettings] Failed to migrate diff settings:`, error) + } + } + /** * List all available configs with metadata. */ diff --git a/src/exports/roo-code.d.ts b/src/exports/roo-code.d.ts index eb778c80ae..066dd2cdc2 100644 --- a/src/exports/roo-code.d.ts +++ b/src/exports/roo-code.d.ts @@ -182,6 +182,8 @@ type ProviderSettings = { modelTemperature?: (number | null) | undefined reasoningEffort?: ("low" | "medium" | "high") | undefined rateLimitSeconds?: number | undefined + diffEnabled?: boolean | undefined + fuzzyMatchThreshold?: number | undefined fakeAi?: unknown | undefined } diff --git a/src/exports/types.ts b/src/exports/types.ts index 3a53a2f9ff..931e07fc25 100644 --- a/src/exports/types.ts +++ b/src/exports/types.ts @@ -183,6 +183,8 @@ type ProviderSettings = { modelTemperature?: (number | null) | undefined reasoningEffort?: ("low" | "medium" | "high") | undefined rateLimitSeconds?: number | undefined + diffEnabled?: boolean | undefined + fuzzyMatchThreshold?: number | undefined fakeAi?: unknown | undefined } diff --git a/src/schemas/index.ts b/src/schemas/index.ts index 80b6bbe197..6c30b6334b 100644 --- a/src/schemas/index.ts +++ b/src/schemas/index.ts @@ -401,6 +401,8 @@ export const providerSettingsSchema = z.object({ modelTemperature: z.number().nullish(), reasoningEffort: reasoningEffortsSchema.optional(), rateLimitSeconds: z.number().optional(), + diffEnabled: z.boolean().optional(), + fuzzyMatchThreshold: z.number().optional(), // Fake AI fakeAi: z.unknown().optional(), }) @@ -490,6 +492,8 @@ const providerSettingsRecord: ProviderSettingsRecord = { modelTemperature: undefined, reasoningEffort: undefined, rateLimitSeconds: undefined, + diffEnabled: undefined, + fuzzyMatchThreshold: undefined, // Fake AI fakeAi: undefined, } diff --git a/webview-ui/src/components/settings/AdvancedSettings.tsx b/webview-ui/src/components/settings/AdvancedSettings.tsx deleted file mode 100644 index b6e3435418..0000000000 --- a/webview-ui/src/components/settings/AdvancedSettings.tsx +++ /dev/null @@ -1,75 +0,0 @@ -import { HTMLAttributes } from "react" -import { useAppTranslation } from "@/i18n/TranslationContext" -import { VSCodeCheckbox } from "@vscode/webview-ui-toolkit/react" -import { Cog } from "lucide-react" - -import { cn } from "@/lib/utils" -import { Slider } from "@/components/ui" - -import { SetCachedStateField } from "./types" -import { SectionHeader } from "./SectionHeader" -import { Section } from "./Section" - -type AdvancedSettingsProps = HTMLAttributes & { - diffEnabled?: boolean - fuzzyMatchThreshold?: number - setCachedStateField: SetCachedStateField<"diffEnabled" | "fuzzyMatchThreshold"> -} -export const AdvancedSettings = ({ - diffEnabled, - fuzzyMatchThreshold, - setCachedStateField, - className, - ...props -}: AdvancedSettingsProps) => { - const { t } = useAppTranslation() - - return ( -
- -
- -
{t("settings:sections.advanced")}
-
- - -
-
- { - setCachedStateField("diffEnabled", e.target.checked) - }}> - {t("settings:advanced.diff.label")} - -
- {t("settings:advanced.diff.description")} -
-
- - {diffEnabled && ( -
-
- -
- setCachedStateField("fuzzyMatchThreshold", value)} - /> - {Math.round((fuzzyMatchThreshold || 1) * 100)}% -
-
- {t("settings:advanced.diff.matchPrecision.description")} -
-
-
- )} -
-
- ) -} diff --git a/webview-ui/src/components/settings/ApiOptions.tsx b/webview-ui/src/components/settings/ApiOptions.tsx index 21f40c92af..0fe4332212 100644 --- a/webview-ui/src/components/settings/ApiOptions.tsx +++ b/webview-ui/src/components/settings/ApiOptions.tsx @@ -53,6 +53,7 @@ import { ModelInfoView } from "./ModelInfoView" import { ModelPicker } from "./ModelPicker" import { TemperatureControl } from "./TemperatureControl" import { RateLimitSecondsControl } from "./RateLimitSecondsControl" +import { DiffSettingsControl } from "./DiffSettingsControl" import { ApiErrorMessage } from "./ApiErrorMessage" import { ThinkingBudget } from "./ThinkingBudget" import { R1FormatSetting } from "./R1FormatSetting" @@ -1681,6 +1682,11 @@ const ApiOptions = ({ {!fromWelcomeView && ( <> + setApiConfigurationField(field, value)} + /> void +} + +export const DiffSettingsControl: React.FC = ({ + diffEnabled = true, + fuzzyMatchThreshold = 1.0, + onChange, +}) => { + const { t } = useAppTranslation() + + const handleDiffEnabledChange = useCallback( + (e: any) => { + onChange("diffEnabled", e.target.checked) + }, + [onChange], + ) + + const handleThresholdChange = useCallback( + (newValue: number[]) => { + onChange("fuzzyMatchThreshold", newValue[0]) + }, + [onChange], + ) + + return ( +
+
+ + {t("settings:advanced.diff.label")} + +
+ {t("settings:advanced.diff.description")} +
+
+ + {diffEnabled && ( +
+
+ +
+ + {Math.round(fuzzyMatchThreshold * 100)}% +
+
+ {t("settings:advanced.diff.matchPrecision.description")} +
+
+
+ )} +
+ ) +} diff --git a/webview-ui/src/components/settings/ExperimentalSettings.tsx b/webview-ui/src/components/settings/ExperimentalSettings.tsx index a2d6fbd274..51ce36b408 100644 --- a/webview-ui/src/components/settings/ExperimentalSettings.tsx +++ b/webview-ui/src/components/settings/ExperimentalSettings.tsx @@ -12,9 +12,7 @@ import { Section } from "./Section" import { ExperimentalFeature } from "./ExperimentalFeature" type ExperimentalSettingsProps = HTMLAttributes & { - setCachedStateField: SetCachedStateField< - "terminalOutputLineLimit" | "maxOpenTabsContext" | "diffEnabled" | "fuzzyMatchThreshold" - > + setCachedStateField: SetCachedStateField<"terminalOutputLineLimit" | "maxOpenTabsContext"> experiments: Record setExperimentEnabled: SetExperimentEnabled } diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 35b78cfc83..2e32630341 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -8,7 +8,6 @@ import { Bell, Database, SquareTerminal, - Cog, FlaskConical, AlertTriangle, Globe, @@ -52,7 +51,6 @@ import { InterfaceSettings } from "./InterfaceSettings" import { NotificationSettings } from "./NotificationSettings" import { ContextManagementSettings } from "./ContextManagementSettings" import { TerminalSettings } from "./TerminalSettings" -import { AdvancedSettings } from "./AdvancedSettings" import { ExperimentalSettings } from "./ExperimentalSettings" import { LanguageSettings } from "./LanguageSettings" import { About } from "./About" @@ -71,7 +69,6 @@ const sectionNames = [ "notifications", "contextManagement", "terminal", - "advanced", "experimental", "language", "about", @@ -299,7 +296,6 @@ const SettingsView = forwardRef(({ onDone, t const notificationsRef = useRef(null) const contextManagementRef = useRef(null) const terminalRef = useRef(null) - const advancedRef = useRef(null) const experimentalRef = useRef(null) const languageRef = useRef(null) const aboutRef = useRef(null) @@ -314,7 +310,6 @@ const SettingsView = forwardRef(({ onDone, t { id: "notifications", icon: Bell, ref: notificationsRef }, { id: "contextManagement", icon: Database, ref: contextManagementRef }, { id: "terminal", icon: SquareTerminal, ref: terminalRef }, - { id: "advanced", icon: Cog, ref: advancedRef }, { id: "experimental", icon: FlaskConical, ref: experimentalRef }, { id: "language", icon: Globe, ref: languageRef }, { id: "about", icon: Info, ref: aboutRef }, @@ -328,7 +323,6 @@ const SettingsView = forwardRef(({ onDone, t notificationsRef, contextManagementRef, terminalRef, - advancedRef, experimentalRef, ], ) @@ -515,14 +509,6 @@ const SettingsView = forwardRef(({ onDone, t /> -
- -
-
Date: Tue, 15 Apr 2025 14:46:12 -0400 Subject: [PATCH 2/2] Fix tests --- .../__tests__/ProviderSettingsManager.test.ts | 12 +++++- .../settings/__tests__/ApiOptions.test.tsx | 42 +++++++++++++++++-- 2 files changed, 49 insertions(+), 5 deletions(-) diff --git a/src/core/config/__tests__/ProviderSettingsManager.test.ts b/src/core/config/__tests__/ProviderSettingsManager.test.ts index 91f5adbdf9..ade40c29d3 100644 --- a/src/core/config/__tests__/ProviderSettingsManager.test.ts +++ b/src/core/config/__tests__/ProviderSettingsManager.test.ts @@ -41,7 +41,7 @@ describe("ProviderSettingsManager", () => { expect(mockSecrets.store).not.toHaveBeenCalled() }) - it("should not initialize config if it exists", async () => { + it("should not initialize config if it exists and migrations are complete", async () => { mockSecrets.get.mockResolvedValue( JSON.stringify({ currentApiConfigName: "default", @@ -49,10 +49,13 @@ describe("ProviderSettingsManager", () => { default: { config: {}, id: "default", + diffEnabled: true, + fuzzyMatchThreshold: 1.0, }, }, migrations: { rateLimitSecondsMigrated: true, + diffSettingsMigrated: true, }, }), ) @@ -75,6 +78,10 @@ describe("ProviderSettingsManager", () => { apiProvider: "anthropic", }, }, + migrations: { + rateLimitSecondsMigrated: true, + diffSettingsMigrated: true, + }, }), ) @@ -82,7 +89,8 @@ describe("ProviderSettingsManager", () => { // Should have written the config with new IDs expect(mockSecrets.store).toHaveBeenCalled() - const storedConfig = JSON.parse(mockSecrets.store.mock.calls[0][1]) + const calls = mockSecrets.store.mock.calls + const storedConfig = JSON.parse(calls[calls.length - 1][1]) // Get the latest call expect(storedConfig.apiConfigs.default.id).toBeTruthy() expect(storedConfig.apiConfigs.test.id).toBeTruthy() }) diff --git a/webview-ui/src/components/settings/__tests__/ApiOptions.test.tsx b/webview-ui/src/components/settings/__tests__/ApiOptions.test.tsx index 9310f4cdf2..bf407cfffe 100644 --- a/webview-ui/src/components/settings/__tests__/ApiOptions.test.tsx +++ b/webview-ui/src/components/settings/__tests__/ApiOptions.test.tsx @@ -96,6 +96,33 @@ jest.mock("../ThinkingBudget", () => ({ ) : null, })) +// Mock DiffSettingsControl for tests +jest.mock("../DiffSettingsControl", () => ({ + DiffSettingsControl: ({ diffEnabled, fuzzyMatchThreshold, onChange }: any) => ( +
+ +
+ Fuzzy match threshold + onChange("fuzzyMatchThreshold", parseFloat(e.target.value))} + min={0.8} + max={1} + step={0.005} + /> +
+
+ ), +})) + const renderApiOptions = (props = {}) => { const queryClient = new QueryClient() @@ -116,14 +143,23 @@ const renderApiOptions = (props = {}) => { } describe("ApiOptions", () => { - it("shows temperature and rate limit controls by default", () => { - renderApiOptions() + it("shows diff settings, temperature and rate limit controls by default", () => { + renderApiOptions({ + apiConfiguration: { + diffEnabled: true, + fuzzyMatchThreshold: 0.95, + }, + }) + // Check for DiffSettingsControl by looking for text content + expect(screen.getByText(/enable editing through diffs/i)).toBeInTheDocument() expect(screen.getByTestId("temperature-control")).toBeInTheDocument() expect(screen.getByTestId("rate-limit-seconds-control")).toBeInTheDocument() }) - it("hides temperature and rate limit controls when fromWelcomeView is true", () => { + it("hides all controls when fromWelcomeView is true", () => { renderApiOptions({ fromWelcomeView: true }) + // Check for absence of DiffSettingsControl text + expect(screen.queryByText(/enable editing through diffs/i)).not.toBeInTheDocument() expect(screen.queryByTestId("temperature-control")).not.toBeInTheDocument() expect(screen.queryByTestId("rate-limit-seconds-control")).not.toBeInTheDocument() })