Browse Source

Fix: Enable save button for provider dropdown and checkbox changes (#7113)

* fix: enable save button for provider dropdown and checkbox changes

The save button wasn't enabling when users changed provider settings like
dropdowns and checkboxes because setApiConfigurationField was treating all
changes from undefined to a defined value as 'initial sync' and not marking
the form as dirty.

Added an optional isUserAction parameter (defaults to true) to distinguish:
- User actions (should enable save button) - the default
- Automatic initialization (shouldn't enable save button) - pass false

This fixes the issue where changing provider dropdowns, checkboxes with
default values, and other settings wouldn't enable the save button.

* fix: remove incorrect isUserAction=false from apiModelId sync

The useEffect that syncs apiModelId with selectedModelId was incorrectly
passing isUserAction=false, which prevented the save button from enabling
when users selected a different model. Since this effect responds to all
selectedModelId changes (including user selections), it should use the
default isUserAction=true behavior.

* test: fix ThinkingBudget test to expect isUserAction parameter

The test now correctly expects setApiConfigurationField to be called
with three arguments including the isUserAction=false parameter for
automatic thinking token adjustments.
Daniel 6 months ago
parent
commit
bfdd158129

+ 6 - 2
webview-ui/src/components/settings/ApiOptions.tsx

@@ -105,7 +105,11 @@ import { buildDocLink } from "@src/utils/docLinks"
 export interface ApiOptionsProps {
 	uriScheme: string | undefined
 	apiConfiguration: ProviderSettings
-	setApiConfigurationField: <K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K]) => void
+	setApiConfigurationField: <K extends keyof ProviderSettings>(
+		field: K,
+		value: ProviderSettings[K],
+		isUserAction?: boolean,
+	) => void
 	fromWelcomeView?: boolean
 	errorMessage: string | undefined
 	setErrorMessage: React.Dispatch<React.SetStateAction<string | undefined>>
@@ -280,7 +284,7 @@ const ApiOptions = ({
 				const shouldSetDefault = !modelId
 
 				if (shouldSetDefault) {
-					setApiConfigurationField(field, defaultValue)
+					setApiConfigurationField(field, defaultValue, false)
 				}
 			}
 

+ 6 - 2
webview-ui/src/components/settings/ModelPicker.tsx

@@ -46,7 +46,11 @@ interface ModelPickerProps {
 	serviceName: string
 	serviceUrl: string
 	apiConfiguration: ProviderSettings
-	setApiConfigurationField: <K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K]) => void
+	setApiConfigurationField: <K extends keyof ProviderSettings>(
+		field: K,
+		value: ProviderSettings[K],
+		isUserAction?: boolean,
+	) => void
 	organizationAllowList: OrganizationAllowList
 	errorMessage?: string
 }
@@ -124,7 +128,7 @@ export const ModelPicker = ({
 	useEffect(() => {
 		if (!selectedModelId && !isInitialized.current) {
 			const initialValue = modelIds.includes(selectedModelId) ? selectedModelId : defaultModelId
-			setApiConfigurationField(modelIdKey, initialValue)
+			setApiConfigurationField(modelIdKey, initialValue, false) // false = automatic initialization
 		}
 
 		isInitialized.current = true

+ 4 - 4
webview-ui/src/components/settings/SettingsView.tsx

@@ -219,7 +219,7 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
 	}, [])
 
 	const setApiConfigurationField = useCallback(
-		<K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K]) => {
+		<K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K], isUserAction: boolean = true) => {
 			setCachedState((prevState) => {
 				if (prevState.apiConfiguration?.[field] === value) {
 					return prevState
@@ -227,9 +227,9 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
 
 				const previousValue = prevState.apiConfiguration?.[field]
 
-				// Don't treat initial sync from undefined to a defined value as a user change
-				// This prevents the dirty state when the component initializes and auto-syncs the model ID
-				const isInitialSync = previousValue === undefined && value !== undefined
+				// Only skip change detection for automatic initialization (not user actions)
+				// This prevents the dirty state when the component initializes and auto-syncs values
+				const isInitialSync = !isUserAction && previousValue === undefined && value !== undefined
 
 				if (!isInitialSync) {
 					setChangeDetected(true)

+ 7 - 3
webview-ui/src/components/settings/ThinkingBudget.tsx

@@ -20,7 +20,11 @@ import { useSelectedModel } from "@src/components/ui/hooks/useSelectedModel"
 
 interface ThinkingBudgetProps {
 	apiConfiguration: ProviderSettings
-	setApiConfigurationField: <K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K]) => void
+	setApiConfigurationField: <K extends keyof ProviderSettings>(
+		field: K,
+		value: ProviderSettings[K],
+		isUserAction?: boolean,
+	) => void
 	modelInfo?: ModelInfo
 }
 
@@ -71,7 +75,7 @@ export const ThinkingBudget = ({ apiConfiguration, setApiConfigurationField, mod
 	// Set default reasoning effort when model supports it and no value is set
 	useEffect(() => {
 		if (isReasoningEffortSupported && !apiConfiguration.reasoningEffort && defaultReasoningEffort) {
-			setApiConfigurationField("reasoningEffort", defaultReasoningEffort)
+			setApiConfigurationField("reasoningEffort", defaultReasoningEffort, false)
 		}
 	}, [isReasoningEffortSupported, apiConfiguration.reasoningEffort, defaultReasoningEffort, setApiConfigurationField])
 
@@ -91,7 +95,7 @@ export const ThinkingBudget = ({ apiConfiguration, setApiConfigurationField, mod
 	// appropriately.
 	useEffect(() => {
 		if (isReasoningBudgetSupported && customMaxThinkingTokens > modelMaxThinkingTokens) {
-			setApiConfigurationField("modelMaxThinkingTokens", modelMaxThinkingTokens)
+			setApiConfigurationField("modelMaxThinkingTokens", modelMaxThinkingTokens, false)
 		}
 	}, [isReasoningBudgetSupported, customMaxThinkingTokens, modelMaxThinkingTokens, setApiConfigurationField])
 

+ 1 - 1
webview-ui/src/components/settings/__tests__/ThinkingBudget.spec.tsx

@@ -112,7 +112,7 @@ describe("ThinkingBudget", () => {
 		)
 
 		// Effect should trigger and cap the value
-		expect(setApiConfigurationField).toHaveBeenCalledWith("modelMaxThinkingTokens", 8000) // 80% of 10000
+		expect(setApiConfigurationField).toHaveBeenCalledWith("modelMaxThinkingTokens", 8000, false) // 80% of 10000
 	})
 
 	it("should use default thinking tokens if not provided", () => {

+ 6 - 2
webview-ui/src/components/settings/providers/HuggingFace.tsx

@@ -34,7 +34,11 @@ type HuggingFaceModel = {
 
 type HuggingFaceProps = {
 	apiConfiguration: ProviderSettings
-	setApiConfigurationField: (field: keyof ProviderSettings, value: ProviderSettings[keyof ProviderSettings]) => void
+	setApiConfigurationField: (
+		field: keyof ProviderSettings,
+		value: ProviderSettings[keyof ProviderSettings],
+		isUserAction?: boolean,
+	) => void
 }
 
 export const HuggingFace = ({ apiConfiguration, setApiConfigurationField }: HuggingFaceProps) => {
@@ -93,7 +97,7 @@ export const HuggingFace = ({ apiConfiguration, setApiConfigurationField }: Hugg
 					// Set to "auto" as default
 					const defaultProvider = "auto"
 					setSelectedProvider(defaultProvider)
-					setApiConfigurationField("huggingFaceInferenceProvider", defaultProvider)
+					setApiConfigurationField("huggingFaceInferenceProvider", defaultProvider, false) // false = automatic default
 				}
 			}
 		}

+ 6 - 2
webview-ui/src/components/settings/providers/Unbound.tsx

@@ -17,7 +17,11 @@ import { ModelPicker } from "../ModelPicker"
 
 type UnboundProps = {
 	apiConfiguration: ProviderSettings
-	setApiConfigurationField: (field: keyof ProviderSettings, value: ProviderSettings[keyof ProviderSettings]) => void
+	setApiConfigurationField: (
+		field: keyof ProviderSettings,
+		value: ProviderSettings[keyof ProviderSettings],
+		isUserAction?: boolean,
+	) => void
 	routerModels?: RouterModels
 	organizationAllowList: OrganizationAllowList
 	modelValidationError?: string
@@ -110,7 +114,7 @@ export const Unbound = ({
 
 			if (!currentModelId || !modelExists) {
 				const firstAvailableModelId = Object.keys(updatedModels)[0]
-				setApiConfigurationField("unboundModelId", firstAvailableModelId)
+				setApiConfigurationField("unboundModelId", firstAvailableModelId, false) // false = automatic model selection
 			}
 		}