diff --git a/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx b/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx index 06d149b20c..d2d81ad49b 100644 --- a/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx @@ -3,16 +3,22 @@ import { screen, fireEvent, renderWithExtensionState } from "@/utils/test-utils" import { act } from "react" import { QueryClient } from "@tanstack/react-query" +import { type Mock } from "vitest" -import { ModelInfo, providerIdentifiers } from "@roo-code/types" +import { litellmDefaultModelId, ModelInfo, providerIdentifiers } from "@roo-code/types" import { ModelPicker } from "../ModelPicker" +import { useRouterModels } from "@src/components/ui/hooks/useRouterModels" vi.mock("@src/context/ExtensionStateContext", () => ({ ExtensionStateContextProvider: ({ children }: any) => children, useExtensionState: vi.fn(), })) +vi.mock("@src/components/ui/hooks/useRouterModels") + +const mockUseRouterModels = useRouterModels as Mock + Element.prototype.scrollIntoView = vi.fn() describe("ModelPicker", () => { @@ -55,6 +61,8 @@ describe("ModelPicker", () => { beforeEach(() => { vi.clearAllMocks() vi.useFakeTimers() + // Default: no router models available. Provider-specific tests override per test. + mockUseRouterModels.mockReturnValue({ data: {}, isLoading: false, isError: false } as any) }) afterEach(() => { @@ -254,4 +262,96 @@ 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: Record, + setField: (field: string, value: unknown) => void, + ) => + renderWithExtensionState( + , + { queryClient }, + ) + + beforeEach(() => { + mockUseRouterModels.mockReturnValue({ + data: { litellm: litellmModels }, + isLoading: false, + isError: false, + } as any) + }) + + 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: Record = { apiProvider: providerIdentifiers.litellm } + const setField = vi.fn((field: string, value: unknown) => { + 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 3558d47e38..04b26345a2 100644 --- a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts +++ b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts @@ -899,7 +899,12 @@ describe("useSelectedModel", () => { expect(result.current.id).toBe("my-custom-model") }) - it("should use litellmDefaultModelInfo when selected model not found in routerModels", () => { + 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({ data: { openrouter: {}, @@ -926,9 +931,40 @@ 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({ + data: { + openrouter: {}, + requesty: {}, + litellm: { + "existing-model": { + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + }, + }, + }, + isLoading: false, + isError: false, + } as any) + + 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) }) diff --git a/webview-ui/src/components/ui/hooks/useSelectedModel.ts b/webview-ui/src/components/ui/hooks/useSelectedModel.ts index b7ad9e87e5..b8a85c9ad1 100644 --- a/webview-ui/src/components/ui/hooks/useSelectedModel.ts +++ b/webview-ui/src/components/ui/hooks/useSelectedModel.ts @@ -191,22 +191,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: {