brought modelpicker and validate up to speed with the refactor

webview-ui/src/components/settings/ModelPicker.tsx

@@ -38,6 +38,7 @@ interface ModelPickerProps {
 	apiConfiguration: ProviderSettings
 	setApiConfigurationField: <K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K]) => void
 	organizationAllowList: OrganizationAllowList
+	onOpenRefetch?: () => void
 }
 
 export const ModelPicker = ({
@@ -49,6 +50,7 @@ export const ModelPicker = ({
 	apiConfiguration,
 	setApiConfigurationField,
 	organizationAllowList,
+	onOpenRefetch,
 }: ModelPickerProps) => {
 	const { t } = useAppTranslation()
 
@@ -57,64 +59,78 @@ export const ModelPicker = ({
 	const searchInputRef = useRef<HTMLInputElement>(null)
 
 	const currentConfiguredModelId = apiConfiguration[modelIdKey]
+	const [searchValue, setSearchValue] = useState(currentConfiguredModelId || "")
 
 	const modelIdsForDropdown = useMemo(() => {
 		const filteredModels = filterModels(models, apiConfiguration.apiProvider, organizationAllowList)
 		return Object.keys(filteredModels ?? {}).sort((a, b) => a.localeCompare(b))
 	}, [models, apiConfiguration.apiProvider, organizationAllowList])
 
-	const { id: selectedModelIdForInfo, info: selectedModelInfo } = useSelectedModel(apiConfiguration)
-
-	const [searchValue, setSearchValue] = useState(currentConfiguredModelId || "")
-
+	// Synchronize apiConfiguration and searchValue when models/selection changes
+	const currentIdInSettings = apiConfiguration[modelIdKey]
 	useEffect(() => {
-		const currentIdInSettings = apiConfiguration[modelIdKey]
-
-		if (!models || Object.keys(models).length === 0) {
+		if (!models || modelIdsForDropdown.length === 0) {
+			// Use modelIdsForDropdown for check after filtering
 			if (currentIdInSettings !== undefined) {
 				setApiConfigurationField(modelIdKey, undefined)
 			}
-			if (searchValue !== "") setSearchValue("")
 		} else {
-			const availableIds = Object.keys(models)
 			let newIdToSet: string | undefined = undefined
-
-			if (currentIdInSettings && availableIds.includes(currentIdInSettings)) {
+			if (currentIdInSettings && modelIdsForDropdown.includes(currentIdInSettings)) {
 				newIdToSet = currentIdInSettings
-			} else if (availableIds.includes(defaultModelId)) {
+			} else if (modelIdsForDropdown.includes(defaultModelId)) {
 				newIdToSet = defaultModelId
-			} else if (availableIds.length > 0) {
-				newIdToSet = availableIds[0]
+			} else {
+				newIdToSet = modelIdsForDropdown[0] // Fallback to the first available model
 			}
 
 			if (currentIdInSettings !== newIdToSet) {
 				setApiConfigurationField(modelIdKey, newIdToSet)
 			}
-			const targetSearchValue = newIdToSet || ""
-			if (searchValue !== targetSearchValue) setSearchValue(targetSearchValue)
 		}
-	}, [models, apiConfiguration, searchValue, defaultModelId, modelIdKey, setApiConfigurationField])
+		// This effect primarily ensures the configured ID is valid against the available models.
+		// SearchValue will be synced by another effect or callbacks.
+	}, [models, modelIdsForDropdown, currentIdInSettings, defaultModelId, modelIdKey, setApiConfigurationField])
+
+	// Effect to sync searchValue with currentConfiguredModelId.
+	// Primarily handles changes when the popover is closed.
+	// When open, user input and specific actions (onSelect, onClearSearch) manage searchValue.
+	// onOpenChange handles resetting searchValue when the popover closes.
+	useEffect(() => {
+		if (!open) {
+			// Only act if the popover is closed
+			// If currentConfiguredModelId has changed and searchValue is out of sync, update it.
+			// Also handles if searchValue somehow changed while closed.
+			if (searchValue !== (currentConfiguredModelId || "")) {
+				setSearchValue(currentConfiguredModelId || "")
+			}
+		}
+		// When 'open' is true, do nothing here to allow user input to control searchValue.
+	}, [currentConfiguredModelId, open, searchValue]) // Rerun if currentConfiguredModelId changes or popover opens/closes
+
+	const { id: selectedModelIdForInfo, info: selectedModelInfo } = useSelectedModel(apiConfiguration)
 
 	const onSelect = useCallback(
 		(modelId: string) => {
-			if (!modelId) {
-				return
-			}
+			if (!modelId) return
+			setApiConfigurationField(modelIdKey, modelId) // This will trigger currentConfiguredModelId update
+			setSearchValue(modelId) // Directly set search for immediate feedback in closed popover
 			setOpen(false)
-			setApiConfigurationField(modelIdKey, modelId)
-			setSearchValue(modelId)
 		},
 		[modelIdKey, setApiConfigurationField],
 	)
 
 	const onOpenChange = useCallback(
-		(open: boolean) => {
-			setOpen(open)
-			if (!open) {
-				setSearchValue(apiConfiguration[modelIdKey] || "")
+		(newOpenState: boolean) => {
+			setOpen(newOpenState)
+			if (newOpenState && onOpenRefetch) {
+				onOpenRefetch()
+			}
+			if (!newOpenState) {
+				setSearchValue(currentConfiguredModelId || "")
 			}
 		},
-		[apiConfiguration, modelIdKey],
+		[currentConfiguredModelId, onOpenRefetch],
 	)
 
 	const onClearSearch = useCallback(() => {
@@ -132,8 +148,13 @@ export const ModelPicker = ({
 							variant="combobox"
 							role="combobox"
 							aria-expanded={open}
-							className="w-full justify-between">
-							<div>{currentConfiguredModelId ?? t("settings:common.select")}</div>
+							className="w-full justify-between"
+							disabled={modelIdsForDropdown.length === 0}>
+							<div>
+								{modelIdsForDropdown.length === 0
+									? ""
+									: (currentConfiguredModelId ?? t("settings:common.select"))}
+							</div>
 							<ChevronsUpDown className="opacity-50" />
 						</Button>
 					</PopoverTrigger>
@@ -142,8 +163,8 @@ export const ModelPicker = ({
 							<div className="relative">
 								<CommandInput
 									ref={searchInputRef}
-									value={searchValue}
-									onValueChange={setSearchValue}
+									value={searchValue} // Controlled input
+									onValueChange={setSearchValue} // User types, updates searchValue directly
 									placeholder={t("settings:modelPicker.searchPlaceholder")}
 									className="h-9 mr-4"
 									data-testid="model-input"
@@ -167,7 +188,7 @@ export const ModelPicker = ({
 								</CommandEmpty>
 								<CommandGroup>
 									{modelIdsForDropdown.map((model) => (
-										<CommandItem key={model} value={model} onSelect={onSelect}>
+										<CommandItem key={model} value={model} onSelect={() => onSelect(model)}>
 											{model}
 											<Check
 												className={cn(
@@ -181,7 +202,10 @@ export const ModelPicker = ({
 							</CommandList>
 							{searchValue && !modelIdsForDropdown.includes(searchValue) && (
 								<div className="p-1 border-t border-vscode-input-border">
-									<CommandItem data-testid="use-custom-model" value={searchValue} onSelect={onSelect}>
+									<CommandItem
+										data-testid="use-custom-model"
+										value={searchValue}
+										onSelect={() => onSelect(searchValue)}>
 										{t("settings:modelPicker.useCustomModel", { modelId: searchValue })}
 									</CommandItem>
 								</div>

webview-ui/src/components/settings/providers/LiteLLM.tsx

@@ -33,6 +33,7 @@ export const LiteLLM = ({ apiConfiguration, setApiConfigurationField, organizati
 		models: litellmModelsData,
 		isLoading: isLoadingModels,
 		error: modelsError,
+		refetch: refetchLiteLLMModels,
 	} = useProviderModels("litellm", providerModelsOptions)
 	console.log("litellmModelsData1212", litellmModelsData, isLoadingModels, modelsError)
 
@@ -87,6 +88,7 @@ export const LiteLLM = ({ apiConfiguration, setApiConfigurationField, organizati
 				serviceUrl="https://docs.litellm.ai/"
 				setApiConfigurationField={setApiConfigurationField}
 				organizationAllowList={organizationAllowList}
+				onOpenRefetch={refetchLiteLLMModels}
 			/>
 		</>
 	)

webview-ui/src/utils/validate.ts

@@ -2,11 +2,10 @@ import i18next from "i18next"
 
 import type { ProviderSettings, OrganizationAllowList } from "@roo-code/types"
 
-import { isRouterName, RouterModels } from "@roo/api"
+// import { isRouterName } from "@roo/api" // Removed as it's no longer used
 
 export function validateApiConfiguration(
 	apiConfiguration: ProviderSettings,
-	routerModels?: RouterModels,
 	organizationAllowList?: OrganizationAllowList,
 ): string | undefined {
 	const keysAndIdsPresentErrorMessage = validateModelsAndKeysProvided(apiConfiguration)
@@ -22,88 +21,67 @@ export function validateApiConfiguration(
 		return organizationAllowListErrorMessage
 	}
 
-	return validateModelId(apiConfiguration, routerModels)
+	return undefined
 }
 
 function validateModelsAndKeysProvided(apiConfiguration: ProviderSettings): string | undefined {
-	switch (apiConfiguration.apiProvider) {
+	const { apiProvider } = apiConfiguration
+
+	switch (apiProvider) {
 		case "openrouter":
-			if (!apiConfiguration.openRouterApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
+			if (!apiConfiguration.openRouterApiKey) return i18next.t("settings:validation.apiKey")
+			if (!apiConfiguration.openRouterModelId) return i18next.t("settings:validation.modelId")
 			break
 		case "glama":
-			if (!apiConfiguration.glamaApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
+			if (!apiConfiguration.glamaApiKey) return i18next.t("settings:validation.apiKey")
+			if (!apiConfiguration.glamaModelId) return i18next.t("settings:validation.modelId")
 			break
 		case "unbound":
-			if (!apiConfiguration.unboundApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
+			if (!apiConfiguration.unboundApiKey) return i18next.t("settings:validation.apiKey")
+			if (!apiConfiguration.unboundModelId) return i18next.t("settings:validation.modelId")
 			break
 		case "requesty":
-			if (!apiConfiguration.requestyApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
+			if (!apiConfiguration.requestyApiKey) return i18next.t("settings:validation.apiKey")
+			if (!apiConfiguration.requestyModelId) return i18next.t("settings:validation.modelId")
 			break
 		case "litellm":
-			if (!apiConfiguration.litellmApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
+			if (!apiConfiguration.litellmApiKey) return i18next.t("settings:validation.apiKey")
+			if (!apiConfiguration.litellmModelId) return i18next.t("settings:validation.modelId")
 			break
-		case "anthropic":
-			if (!apiConfiguration.apiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
-			break
-		case "bedrock":
-			if (!apiConfiguration.awsRegion) {
-				return i18next.t("settings:validation.awsRegion")
-			}
-			break
-		case "vertex":
-			if (!apiConfiguration.vertexProjectId || !apiConfiguration.vertexRegion) {
-				return i18next.t("settings:validation.googleCloud")
-			}
-			break
-		case "gemini":
-			if (!apiConfiguration.geminiApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
-			break
-		case "openai-native":
-			if (!apiConfiguration.openAiNativeApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
-			break
-		case "mistral":
-			if (!apiConfiguration.mistralApiKey) {
-				return i18next.t("settings:validation.apiKey")
-			}
-			break
-		case "openai":
+		case "openai": // This is openai-compatible router
 			if (!apiConfiguration.openAiBaseUrl || !apiConfiguration.openAiApiKey || !apiConfiguration.openAiModelId) {
 				return i18next.t("settings:validation.openAi")
 			}
 			break
 		case "ollama":
-			if (!apiConfiguration.ollamaModelId) {
-				return i18next.t("settings:validation.modelId")
-			}
+			if (!apiConfiguration.ollamaModelId) return i18next.t("settings:validation.modelId")
 			break
 		case "lmstudio":
-			if (!apiConfiguration.lmStudioModelId) {
-				return i18next.t("settings:validation.modelId")
-			}
+			if (!apiConfiguration.lmStudioModelId) return i18next.t("settings:validation.modelId")
 			break
 		case "vscode-lm":
-			if (!apiConfiguration.vsCodeLmModelSelector) {
-				return i18next.t("settings:validation.modelSelector")
-			}
+			if (!apiConfiguration.vsCodeLmModelSelector) return i18next.t("settings:validation.modelSelector")
+			break
+		case "anthropic":
+			if (!apiConfiguration.apiKey) return i18next.t("settings:validation.apiKey")
+			break
+		case "bedrock":
+			if (!apiConfiguration.awsRegion) return i18next.t("settings:validation.awsRegion")
+			break
+		case "vertex":
+			if (!apiConfiguration.vertexProjectId || !apiConfiguration.vertexRegion)
+				return i18next.t("settings:validation.googleCloud")
+			break
+		case "gemini":
+			if (!apiConfiguration.geminiApiKey) return i18next.t("settings:validation.apiKey")
+			break
+		case "openai-native":
+			if (!apiConfiguration.openAiNativeApiKey) return i18next.t("settings:validation.apiKey")
+			break
+		case "mistral":
+			if (!apiConfiguration.mistralApiKey) return i18next.t("settings:validation.apiKey")
 			break
 	}
-
 	return undefined
 }
 
@@ -132,6 +110,7 @@ function validateProviderAgainstOrganizationSettings(
 			}
 		}
 	}
+	return undefined
 }
 
 function getModelIdForProvider(apiConfiguration: ProviderSettings, provider: string): string | undefined {
@@ -147,26 +126,19 @@ function getModelIdForProvider(apiConfiguration: ProviderSettings, provider: str
 		case "litellm":
 			return apiConfiguration.litellmModelId
 		case "openai":
-			return apiConfiguration.openAiModelId
+			return apiConfiguration.openAiModelId // openai-compatible
 		case "ollama":
 			return apiConfiguration.ollamaModelId
 		case "lmstudio":
 			return apiConfiguration.lmStudioModelId
 		case "vscode-lm":
-			// vsCodeLmModelSelector is an object, not a string
 			return apiConfiguration.vsCodeLmModelSelector?.id
 		default:
 			return apiConfiguration.apiModelId
 	}
 }
-/**
- * Validates an Amazon Bedrock ARN format and optionally checks if the region in the ARN matches the provided region
- * @param arn The ARN string to validate
- * @param region Optional region to check against the ARN's region
- * @returns An object with validation results: { isValid, arnRegion, errorMessage }
- */
+
 export function validateBedrockArn(arn: string, region?: string) {
-	// Validate ARN format
 	const arnRegex = /^arn:aws:(?:bedrock|sagemaker):([^:]+):([^:]*):(?:([^/]+)\/([\w.\-:]+)|([^/]+))$/
 	const match = arn.match(arnRegex)
 
@@ -178,10 +150,8 @@ export function validateBedrockArn(arn: string, region?: string) {
 		}
 	}
 
-	// Extract region from ARN
 	const arnRegion = match[1]
 
-	// Check if region in ARN matches provided region (if specified)
 	if (region && arnRegion !== region) {
 		return {
 			isValid: true,
@@ -190,46 +160,5 @@ export function validateBedrockArn(arn: string, region?: string) {
 		}
 	}
 
-	// ARN is valid and region matches (or no region was provided to check against)
 	return { isValid: true, arnRegion, errorMessage: undefined }
 }
-
-export function validateModelId(apiConfiguration: ProviderSettings, routerModels?: RouterModels): string | undefined {
-	const provider = apiConfiguration.apiProvider ?? ""
-
-	if (!isRouterName(provider)) {
-		return undefined
-	}
-
-	let modelId: string | undefined
-
-	switch (provider) {
-		case "openrouter":
-			modelId = apiConfiguration.openRouterModelId
-			break
-		case "glama":
-			modelId = apiConfiguration.glamaModelId
-			break
-		case "unbound":
-			modelId = apiConfiguration.unboundModelId
-			break
-		case "requesty":
-			modelId = apiConfiguration.requestyModelId
-			break
-		case "litellm":
-			modelId = apiConfiguration.litellmModelId
-			break
-	}
-
-	if (!modelId) {
-		return i18next.t("settings:validation.modelId")
-	}
-
-	const models = routerModels?.[provider]
-
-	if (models && Object.keys(models).length > 1 && !Object.keys(models).includes(modelId)) {
-		return i18next.t("settings:validation.modelAvailability", { modelId })
-	}
-
-	return undefined
-}