Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mrubens merged 3 commits into from
Aug 14, 2025
Merged

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
68de733
fix: enable save button for provider dropdown and checkbox changes
daniel-lxs Aug 14, 2025
a07ac9a
fix: remove incorrect isUserAction=false from apiModelId sync
daniel-lxs Aug 14, 2025
55f7792
test: fix ThinkingBudget test to expect isUserAction parameter
daniel-lxs Aug 14, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 7 additions & 3 deletions webview-ui/src/components/settings/ApiOptions.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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>>
Expand Down Expand Up @@ -186,7 +190,7 @@ const ApiOptions = ({
// Update `apiModelId` whenever `selectedModelId` changes.
useEffect(() => {
if (selectedModelId && apiConfiguration.apiModelId !== selectedModelId) {
setApiConfigurationField("apiModelId", selectedModelId)
setApiConfigurationField("apiModelId", selectedModelId, false)
}
}, [selectedModelId, setApiConfigurationField, apiConfiguration.apiModelId])

Expand Down Expand Up @@ -280,7 +284,7 @@ const ApiOptions = ({
const shouldSetDefault = !modelId

if (shouldSetDefault) {
setApiConfigurationField(field, defaultValue)
setApiConfigurationField(field, defaultValue, false)
}
}

Expand Down
8 changes: 6 additions & 2 deletions webview-ui/src/components/settings/ModelPicker.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
Copy link
Contributor

@roomote roomote bot Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good use of the comment here to clarify that this is automatic initialization! This pattern makes the intent very clear.

}

isInitialized.current = true
Expand Down
8 changes: 4 additions & 4 deletions webview-ui/src/components/settings/SettingsView.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,17 +219,17 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
}, [])

const setApiConfigurationField = useCallback(
Copy link
Contributor

@roomote roomote bot Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding JSDoc documentation for this function to explain the purpose of the isUserAction parameter. This would help future developers understand when to pass false. For example:

Suggested change
const setApiConfigurationField = useCallback(
/**
* Updates a field in the API configuration
* @param field - The field to update
* @param value - The new value
* @param isUserAction - Whether this change is from user interaction (true) or automatic initialization (false).
* User actions will mark the form as dirty and enable the save button.
*/
const setApiConfigurationField = useCallback(
<K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K], isUserAction: boolean = true) => {

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

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
Copy link
Contributor

@roomote roomote bot Aug 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here is correct, but consider adding a comment to explain why we skip change detection for automatic initialization. This would help future maintainers understand the pattern.


if (!isInitialSync) {
setChangeDetected(true)
Expand Down
10 changes: 7 additions & 3 deletions webview-ui/src/components/settings/ThinkingBudget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down Expand Up @@ -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])

Expand All @@ -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])

Expand Down
8 changes: 6 additions & 2 deletions webview-ui/src/components/settings/providers/HuggingFace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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) => {
Expand Down Expand Up @@ -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
}
}
}
Expand Down
8 changes: 6 additions & 2 deletions webview-ui/src/components/settings/providers/Unbound.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}
}

Expand Down
Loading