diff --git a/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx b/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx index 06d149b20c..9a21ff678f 100644 --- a/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx @@ -1,18 +1,40 @@ // npx vitest src/components/settings/__tests__/ModelPicker.spec.tsx import { screen, fireEvent, renderWithExtensionState } from "@/utils/test-utils" -import { act } from "react" +import { act, type ReactNode } from "react" import { QueryClient } from "@tanstack/react-query" +import { type Mock } from "vitest" -import { ModelInfo, providerIdentifiers } from "@roo-code/types" +import { + litellmDefaultModelId, + type ModelInfo, + type ProviderSettings, + type RouterModels, + providerIdentifiers, +} from "@roo-code/types" import { ModelPicker } from "../ModelPicker" +import { useRouterModels } from "@src/components/ui/hooks/useRouterModels" + +type SetApiConfigurationField = ( + field: K, + value: ProviderSettings[K], + isUserAction?: boolean, +) => void + +// useRouterModels returns a react-query observable result; these tests only need the stable state fields. +const createRouterModelsResult = (data: Partial): ReturnType => + ({ data, isLoading: false, isError: false }) as ReturnType vi.mock("@src/context/ExtensionStateContext", () => ({ - ExtensionStateContextProvider: ({ children }: any) => children, + ExtensionStateContextProvider: ({ children }: { children: ReactNode }) => children, useExtensionState: vi.fn(), })) +vi.mock("@src/components/ui/hooks/useRouterModels") + +const mockUseRouterModels = useRouterModels as Mock + Element.prototype.scrollIntoView = vi.fn() describe("ModelPicker", () => { @@ -34,8 +56,9 @@ describe("ModelPicker", () => { model2: { name: "Model 2", description: "Test model 2", ...modelInfo }, } + const apiConfiguration: ProviderSettings = {} const defaultProps = { - apiConfiguration: {}, + apiConfiguration, defaultModelId: "model1", modelIdKey: "openRouterModelId" as const, serviceName: "Test Service", @@ -55,6 +78,8 @@ describe("ModelPicker", () => { beforeEach(() => { vi.clearAllMocks() vi.useFakeTimers() + // Default: no router models available. Provider-specific tests override per test. + mockUseRouterModels.mockReturnValue(createRouterModelsResult({})) }) afterEach(() => { @@ -254,4 +279,89 @@ describe("ModelPicker", () => { expect(screen.getByTestId("automatic-fetch-hint")).toBeInTheDocument() }) }) + + describe("LiteLLM custom model selection", () => { + const litellmModels: Record = { + "gpt-4o-mini": { description: "LiteLLM proxy model", ...modelInfo }, + } + + const renderLiteLLMPicker = (apiConfiguration: ProviderSettings, setField: SetApiConfigurationField) => + renderWithExtensionState( + , + { queryClient }, + ) + + beforeEach(() => { + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ litellm: litellmModels })) + }) + + it("keeps a custom model ID in the picker instead of reverting to the default", async () => { + // Regression: on the LiteLLM settings screen the user could not change the + // model ID to a value absent from the fetched /models list -- the picker + // silently reverted to the hardcoded default model after the selection. + const customModelId = "my-litellm-alias" + let apiConfiguration: ProviderSettings = { apiProvider: providerIdentifiers.litellm } + const setField = vi.fn(function (field: K, value: ProviderSettings[K]) { + apiConfiguration = { ...apiConfiguration, [field]: value } + }) + + const { rerender } = await act(async () => { + return renderLiteLLMPicker(apiConfiguration, setField) + }) + + // Before any selection the picker shows the provider default. + expect(screen.getByTestId("model-picker-button")).toHaveTextContent(litellmDefaultModelId) + + // Open the popover and type a model ID that is not in the fetched list. + await act(async () => { + fireEvent.click(screen.getByTestId("model-picker-button")) + }) + await act(async () => { + vi.advanceTimersByTime(100) + }) + await act(async () => { + fireEvent.input(screen.getByTestId("model-input"), { target: { value: customModelId } }) + }) + await act(async () => { + vi.advanceTimersByTime(100) + }) + await act(async () => { + fireEvent.click(screen.getByTestId("use-custom-model")) + }) + await act(async () => { + vi.advanceTimersByTime(100) + }) + + expect(setField).toHaveBeenCalledWith("litellmModelId", customModelId) + + // Re-render with the updated configuration (as SettingsView does after the + // setter runs) and assert the selection is kept, not reset to the default. + await act(async () => { + rerender( + , + ) + }) + + expect(screen.getByTestId("model-picker-button")).toHaveTextContent(customModelId) + expect(screen.getByTestId("model-picker-button")).not.toHaveTextContent(litellmDefaultModelId) + }) + }) }) diff --git a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts index 33a9df16da..cf29cbe6fa 100644 --- a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts +++ b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts @@ -685,15 +685,7 @@ describe("useSelectedModel", () => { describe("bedrock provider with 1M context", () => { beforeEach(() => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) mockUseOpenRouterModelProviders.mockReturnValue({ data: {}, @@ -747,15 +739,7 @@ describe("useSelectedModel", () => { describe("bedrock provider with custom ARN", () => { beforeEach(() => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) mockUseOpenRouterModelProviders.mockReturnValue({ data: {}, @@ -801,15 +785,7 @@ describe("useSelectedModel", () => { }) it("should use litellmDefaultModelInfo as fallback when routerModels.litellm is empty", () => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) const apiConfiguration: ProviderSettings = { apiProvider: providerIdentifiers.litellm, @@ -827,15 +803,7 @@ describe("useSelectedModel", () => { }) it("should return an empty model ID when the list is empty and no model is configured", () => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) const apiConfiguration: ProviderSettings = { apiProvider: providerIdentifiers.litellm, @@ -855,8 +823,8 @@ describe("useSelectedModel", () => { // Primary user-visible scenario: a "Sync Models" click momentarily empties the // router-models list before the refreshed list arrives. The selection must be held // across that transition rather than reset. - mockUseRouterModels.mockReturnValue({ - data: { + mockUseRouterModels.mockReturnValue( + createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: { @@ -867,10 +835,8 @@ describe("useSelectedModel", () => { supportsPromptCache: false, }, }, - }, - isLoading: false, - isError: false, - } as any) + }), + ) const apiConfiguration: ProviderSettings = { apiProvider: providerIdentifiers.litellm, @@ -884,24 +850,21 @@ describe("useSelectedModel", () => { expect(result.current.id).toBe("my-custom-model") // Simulate the list emptying mid-sync. - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) rerender() // Selection is preserved through the empty window. expect(result.current.id).toBe("my-custom-model") }) - it("should use litellmDefaultModelInfo when selected model not found in routerModels", () => { - mockUseRouterModels.mockReturnValue({ - data: { + it("preserves a configured model ID that is absent from the populated list", () => { + // Regression: LiteLLM is a proxy whose users may configure aliases or models + // that the fetched /models list does not include (custom aliases, incomplete or + // stale listings). The configured ID is the user's explicit selection and must + // not be replaced by a hardcoded default, which made the settings screen appear + // to ignore model ID changes. + mockUseRouterModels.mockReturnValue( + createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: { @@ -912,10 +875,8 @@ describe("useSelectedModel", () => { supportsPromptCache: false, }, }, - }, - isLoading: false, - isError: false, - } as any) + }), + ) const apiConfiguration: ProviderSettings = { apiProvider: providerIdentifiers.litellm, @@ -926,9 +887,59 @@ describe("useSelectedModel", () => { const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) expect(result.current.provider).toBe(providerIdentifiers.litellm) - // Falls back to default model ID + // The configured ID is preserved even though it is absent from the fetched list + expect(result.current.id).toBe("non-existing-model") + // Model info falls back to litellmDefaultModelInfo since the model is not in router models + expect(result.current.info).toEqual(litellmDefaultModelInfo) + }) + + it("falls back to the default model ID only when nothing is configured but a list exists", () => { + mockUseRouterModels.mockReturnValue( + createRouterModelsResult({ + openrouter: {}, + requesty: {}, + litellm: { + "existing-model": { + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + }, + }, + }), + ) + + const apiConfiguration: ProviderSettings = { + apiProvider: providerIdentifiers.litellm, + // litellmModelId intentionally omitted + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + // Nothing configured: fall back to the provider default so the picker shows a selection expect(result.current.id).toBe("claude-3-7-sonnet-20250219") - // Should use litellmDefaultModelInfo as fallback since default model also not in router models + expect(result.current.info).toEqual(litellmDefaultModelInfo) + }) + + it("preserves a configured model ID when the router payload has no litellm entry", () => { + // Regression: when the router-models payload lacks the litellm provider entry + // (partial or failed response), the hook must not substitute the provider + // default, which would silently replace the user's configured ID. + mockUseRouterModels.mockReturnValue(createRouterModelsResult({})) + + const apiConfiguration: ProviderSettings = { + apiProvider: providerIdentifiers.litellm, + litellmModelId: "my-litellm-alias", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.provider).toBe(providerIdentifiers.litellm) + // The configured ID survives even though the payload has no litellm entry at all + expect(result.current.id).toBe("my-litellm-alias") + // No router info is available for the configured ID, so the fallback info applies expect(result.current.info).toEqual(litellmDefaultModelInfo) }) @@ -941,17 +952,13 @@ describe("useSelectedModel", () => { description: "Custom LiteLLM model", } - mockUseRouterModels.mockReturnValue({ - data: { + mockUseRouterModels.mockReturnValue( + createRouterModelsResult({ openrouter: {}, requesty: {}, - litellm: { - "custom-model": customModelInfo, - }, - }, - isLoading: false, - isError: false, - } as any) + litellm: { "custom-model": customModelInfo }, + }), + ) const apiConfiguration: ProviderSettings = { apiProvider: providerIdentifiers.litellm, @@ -965,6 +972,30 @@ describe("useSelectedModel", () => { expect(result.current.id).toBe("custom-model") expect(result.current.info).toEqual(customModelInfo) }) + + it("resolves the configured model ID even when the router fetch errors", () => { + // Regression guard: hasValidRouterData for LiteLLM now only requires + // !isLoading, so a failed fetch (isError=true, isLoading=false) must + // still resolve the hook and preserve the configured ID rather than + // resetting to the provider default. + mockUseRouterModels.mockReturnValue( + createRouterModelsResult(undefined, { isLoading: false, isError: true }), + ) + + const apiConfiguration: ProviderSettings = { + apiProvider: providerIdentifiers.litellm, + litellmModelId: "my-litellm-alias", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + // Configured ID preserved — a fetch failure must not reset the user's selection. + expect(result.current.id).toBe("my-litellm-alias") + expect(result.current.info).toEqual(litellmDefaultModelInfo) + // Callers that surface error banners still see the flag. + expect(result.current.isError).toBe(true) + }) }) describe("kenari provider", () => { @@ -1083,15 +1114,7 @@ describe("useSelectedModel", () => { describe("openai provider", () => { beforeEach(() => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) mockUseOpenRouterModelProviders.mockReturnValue({ data: {}, @@ -1164,15 +1187,7 @@ describe("useSelectedModel", () => { describe("minimax provider", () => { beforeEach(() => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) mockUseOpenRouterModelProviders.mockReturnValue({ data: {}, @@ -1211,15 +1226,7 @@ describe("useSelectedModel", () => { describe("vscode-lm provider", () => { beforeEach(() => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) mockUseOpenRouterModelProviders.mockReturnValue({ data: {}, @@ -1282,15 +1289,7 @@ describe("useSelectedModel", () => { describe("friendli provider", () => { beforeEach(() => { - mockUseRouterModels.mockReturnValue({ - data: { - openrouter: {}, - requesty: {}, - litellm: {}, - }, - isLoading: false, - isError: false, - } as any) + mockUseRouterModels.mockReturnValue(createRouterModelsResult({ openrouter: {}, requesty: {}, litellm: {} })) mockUseOpenRouterModelProviders.mockReturnValue({ data: {}, diff --git a/webview-ui/src/components/ui/hooks/useSelectedModel.ts b/webview-ui/src/components/ui/hooks/useSelectedModel.ts index b7ad9e87e5..8d4b70ad4a 100644 --- a/webview-ui/src/components/ui/hooks/useSelectedModel.ts +++ b/webview-ui/src/components/ui/hooks/useSelectedModel.ts @@ -57,6 +57,17 @@ function getValidatedModelId( return configuredId && availableModels?.[configuredId] ? configuredId : defaultModelId } +/** + * Resolves the model currently selected for the active API provider. + * + * Dynamic providers validate the configured model ID against the fetched + * router-model list and only resolve the selection once that list is + * available. LiteLLM is the exception: it fronts arbitrary models and + * aliases, so a configured `litellmModelId` is the user's explicit + * selection and is preserved as soon as the fetch settles, even when the + * router payload has no LiteLLM entry (partial listing, failed fetch, or + * renamed deployment). + */ export const useSelectedModel = (apiConfiguration?: ProviderSettings) => { const provider = apiConfiguration?.apiProvider || providerIdentifiers.openrouter const activeProvider: ProviderName | undefined = isRetiredProvider(provider) ? undefined : provider @@ -84,12 +95,19 @@ export const useSelectedModel = (apiConfiguration?: ProviderSettings) => { const needLmStudio = typeof lmStudioModelId !== "undefined" const needOllama = typeof ollamaModelId !== "undefined" + // LiteLLM may legitimately have no entry in the router payload (partial + // listing, failed fetch, renamed deployment) even though the configured + // ID is a valid selection, so it only needs the fetch to settle. Other + // dynamic providers require a populated provider entry before the + // selection is resolved. const hasValidRouterData = needRouterModels && dynamicProvider - ? routerModels.data && - routerModels.data[dynamicProvider] !== undefined && - typeof routerModels.data[dynamicProvider] === "object" && - !routerModels.isLoading + ? dynamicProvider === providerIdentifiers.litellm + ? !routerModels.isLoading + : routerModels.data && + routerModels.data[dynamicProvider] !== undefined && + typeof routerModels.data[dynamicProvider] === "object" && + !routerModels.isLoading : true const isReady = @@ -191,22 +209,20 @@ function getSelectedModel({ return { id, info: routerInfo } } case providerIdentifiers.litellm: { - // When the model list is empty (not yet loaded or still loading), - // preserve the configured model ID. LiteLLM is a proxy with no inherent - // default model, so we never substitute a hardcoded default here -- when - // nothing is configured we return an empty ID so the picker shows "no - // selection" rather than a phantom model that does not exist on the server. - const hasModels = - routerModels[providerIdentifiers.litellm] && - Object.keys(routerModels[providerIdentifiers.litellm]).length > 0 - const id = hasModels - ? getValidatedModelId( - apiConfiguration.litellmModelId, - routerModels[providerIdentifiers.litellm], - defaultModelId, - ) - : (apiConfiguration.litellmModelId ?? "") - const routerInfo = routerModels[providerIdentifiers.litellm]?.[id] + // LiteLLM is a proxy that fronts arbitrary models and aliases, so a + // configured model ID is the user's explicit selection even when it is + // absent from the fetched list (custom aliases, incomplete or stale + // listings, renamed deployments). Never substitute a hardcoded default + // over a configured ID -- doing so silently discards the user's model + // choice on the settings screen. Only fall back to the default when + // nothing is configured and a populated list exists; when the list is + // empty we return an empty ID so the picker shows "no selection" rather + // than a phantom model that does not exist on the server. + const litellmModels = routerModels[providerIdentifiers.litellm] + const id = + apiConfiguration.litellmModelId ?? + (litellmModels && Object.keys(litellmModels).length > 0 ? defaultModelId : "") + const routerInfo = litellmModels?.[id] return { id, info: routerInfo ?? litellmDefaultModelInfo } } case providerIdentifiers.xai: {