Move diff editing config to provider settings (#2655)

* Move diff editing config to provider settings

* Fix tests
This commit is contained in:
Matt Rubens 2025-04-15 14:59:27 -04:00 committed by GitHub
parent a3b2ebc026
commit 648c6e7d28
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
11 changed files with 179 additions and 98 deletions

View file

@ -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<boolean>("diffEnabled")
fuzzyMatchThreshold = await this.context.globalState.get<number>("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.
*/

View file

@ -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()
})

View file

@ -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
}

View file

@ -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
}

View file

@ -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,
}

View file

@ -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<HTMLDivElement> & {
diffEnabled?: boolean
fuzzyMatchThreshold?: number
setCachedStateField: SetCachedStateField<"diffEnabled" | "fuzzyMatchThreshold">
}
export const AdvancedSettings = ({
diffEnabled,
fuzzyMatchThreshold,
setCachedStateField,
className,
...props
}: AdvancedSettingsProps) => {
const { t } = useAppTranslation()
return (
<div className={cn("flex flex-col gap-2", className)} {...props}>
<SectionHeader>
<div className="flex items-center gap-2">
<Cog className="w-4" />
<div>{t("settings:sections.advanced")}</div>
</div>
</SectionHeader>
<Section>
<div>
<VSCodeCheckbox
checked={diffEnabled}
onChange={(e: any) => {
setCachedStateField("diffEnabled", e.target.checked)
}}>
<span className="font-medium">{t("settings:advanced.diff.label")}</span>
</VSCodeCheckbox>
<div className="text-vscode-descriptionForeground text-sm">
{t("settings:advanced.diff.description")}
</div>
</div>
{diffEnabled && (
<div className="flex flex-col gap-3 pl-3 border-l-2 border-vscode-button-background">
<div>
<label className="block font-medium mb-1">
{t("settings:advanced.diff.matchPrecision.label")}
</label>
<div className="flex items-center gap-2">
<Slider
min={0.8}
max={1}
step={0.005}
value={[fuzzyMatchThreshold ?? 1.0]}
onValueChange={([value]) => setCachedStateField("fuzzyMatchThreshold", value)}
/>
<span className="w-10">{Math.round((fuzzyMatchThreshold || 1) * 100)}%</span>
</div>
<div className="text-vscode-descriptionForeground text-sm mt-1">
{t("settings:advanced.diff.matchPrecision.description")}
</div>
</div>
</div>
)}
</Section>
</div>
)
}

View file

@ -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 && (
<>
<DiffSettingsControl
diffEnabled={apiConfiguration.diffEnabled}
fuzzyMatchThreshold={apiConfiguration.fuzzyMatchThreshold}
onChange={(field, value) => setApiConfigurationField(field, value)}
/>
<TemperatureControl
value={apiConfiguration?.modelTemperature}
onChange={handleInputChange("modelTemperature", noTransform)}

View file

@ -0,0 +1,68 @@
import React, { useCallback } from "react"
import { Slider } from "@/components/ui"
import { useAppTranslation } from "@/i18n/TranslationContext"
import { VSCodeCheckbox } from "@vscode/webview-ui-toolkit/react"
interface DiffSettingsControlProps {
diffEnabled?: boolean
fuzzyMatchThreshold?: number
onChange: (field: "diffEnabled" | "fuzzyMatchThreshold", value: any) => void
}
export const DiffSettingsControl: React.FC<DiffSettingsControlProps> = ({
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 (
<div className="flex flex-col gap-1">
<div>
<VSCodeCheckbox checked={diffEnabled} onChange={handleDiffEnabledChange}>
<span className="font-medium">{t("settings:advanced.diff.label")}</span>
</VSCodeCheckbox>
<div className="text-vscode-descriptionForeground text-sm">
{t("settings:advanced.diff.description")}
</div>
</div>
{diffEnabled && (
<div className="flex flex-col gap-3 pl-3 border-l-2 border-vscode-button-background">
<div>
<label className="block font-medium mb-1">
{t("settings:advanced.diff.matchPrecision.label")}
</label>
<div className="flex items-center gap-2">
<Slider
min={0.8}
max={1}
step={0.005}
value={[fuzzyMatchThreshold]}
onValueChange={handleThresholdChange}
/>
<span className="w-10">{Math.round(fuzzyMatchThreshold * 100)}%</span>
</div>
<div className="text-vscode-descriptionForeground text-sm mt-1">
{t("settings:advanced.diff.matchPrecision.description")}
</div>
</div>
</div>
)}
</div>
)
}

View file

@ -12,9 +12,7 @@ import { Section } from "./Section"
import { ExperimentalFeature } from "./ExperimentalFeature"
type ExperimentalSettingsProps = HTMLAttributes<HTMLDivElement> & {
setCachedStateField: SetCachedStateField<
"terminalOutputLineLimit" | "maxOpenTabsContext" | "diffEnabled" | "fuzzyMatchThreshold"
>
setCachedStateField: SetCachedStateField<"terminalOutputLineLimit" | "maxOpenTabsContext">
experiments: Record<ExperimentId, boolean>
setExperimentEnabled: SetExperimentEnabled
}

View file

@ -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<SettingsViewRef, SettingsViewProps>(({ onDone, t
const notificationsRef = useRef<HTMLDivElement>(null)
const contextManagementRef = useRef<HTMLDivElement>(null)
const terminalRef = useRef<HTMLDivElement>(null)
const advancedRef = useRef<HTMLDivElement>(null)
const experimentalRef = useRef<HTMLDivElement>(null)
const languageRef = useRef<HTMLDivElement>(null)
const aboutRef = useRef<HTMLDivElement>(null)
@ -314,7 +310,6 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ 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<SettingsViewRef, SettingsViewProps>(({ onDone, t
notificationsRef,
contextManagementRef,
terminalRef,
advancedRef,
experimentalRef,
],
)
@ -515,14 +509,6 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
/>
</div>
<div ref={advancedRef}>
<AdvancedSettings
diffEnabled={diffEnabled}
fuzzyMatchThreshold={fuzzyMatchThreshold}
setCachedStateField={setCachedStateField}
/>
</div>
<div ref={experimentalRef}>
<ExperimentalSettings
setCachedStateField={setCachedStateField}

View file

@ -96,6 +96,33 @@ jest.mock("../ThinkingBudget", () => ({
) : null,
}))
// Mock DiffSettingsControl for tests
jest.mock("../DiffSettingsControl", () => ({
DiffSettingsControl: ({ diffEnabled, fuzzyMatchThreshold, onChange }: any) => (
<div data-testid="diff-settings-control">
<label>
Enable editing through diffs
<input
type="checkbox"
checked={diffEnabled}
onChange={(e) => onChange("diffEnabled", e.target.checked)}
/>
</label>
<div>
Fuzzy match threshold
<input
type="range"
value={fuzzyMatchThreshold || 1.0}
onChange={(e) => onChange("fuzzyMatchThreshold", parseFloat(e.target.value))}
min={0.8}
max={1}
step={0.005}
/>
</div>
</div>
),
}))
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()
})