From ddedab101b02c5ba7a78f92b79579a40f2e558b9 Mon Sep 17 00:00:00 2001 From: Eason Liang Date: Tue, 25 Aug 2026 19:32:39 +0800 Subject: [PATCH] fix(settings): preserve configured LiteLLM model ID in model picker The LiteLLM case in useSelectedModel validated the configured model ID against the fetched /models list and silently substituted the hardcoded default (claude-3-7-sonnet-20250219) whenever the configured ID was absent. LiteLLM is a proxy that fronts arbitrary models and aliases, so a configured ID is the user's explicit selection even when it is not in the fetched list (custom aliases, incomplete or stale listings, renamed deployments). On the settings screen the picker reverted to the default after every selection, making the model ID appear unchangeable; the saved custom ID was also not displayed after reopening settings. Only fall back to the default when nothing is configured and a populated list exists; keep the empty-ID behavior for the empty-list case. Adds a hook-level regression test and a ModelPicker component test covering the full user flow (open picker, use-custom-model, re-render with updated config). --- .../settings/__tests__/ModelPicker.spec.tsx | 102 +++++++++++++++++- .../hooks/__tests__/useSelectedModel.spec.ts | 42 +++++++- .../components/ui/hooks/useSelectedModel.ts | 30 +++--- 3 files changed, 154 insertions(+), 20 deletions(-) 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: {