Skip to content

Commit 745656a

Browse files
easonLiangWorldedtecheasonliang28edelauna
authored
fix(settings): preserve configured LiteLLM model ID in model picker (#1368)
* 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). * fix(settings): resolve LiteLLM selection once router fetch settles When the router-models payload lacks a litellm provider entry (partial listing, failed fetch, renamed deployment), hasValidRouterData stayed false and useSelectedModel substituted the provider default, silently replacing the user-configured litellmModelId. LiteLLM now only needs the fetch to settle, since a configured ID is an explicit selection; other dynamic providers still require a populated provider entry. Add a hook-level regression test for a payload without the litellm entry and a configured custom ID, and convert the affected test doubles to the typed createRouterModelsResult helper, removing the as any / as never casts. * test(useSelectedModel): cover LiteLLM isError path after hasValidRouterData change --------- Co-authored-by: Eason Liang <easonliang28@gmail.com> Co-authored-by: Elliott de Launay <edelauna@gmail.com>
1 parent d5f7795 commit 745656a

3 files changed

Lines changed: 254 additions & 129 deletions

File tree

‎webview-ui/src/components/settings/__tests__/ModelPicker.spec.tsx‎

Lines changed: 114 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,18 +1,40 @@
11
// npx vitest src/components/settings/__tests__/ModelPicker.spec.tsx
22

33
import { screen, fireEvent, renderWithExtensionState } from "@/utils/test-utils"
4-
import { act } from "react"
4+
import { act, type ReactNode } from "react"
55
import { QueryClient } from "@tanstack/react-query"
6+
import { type Mock } from "vitest"
67

7-
import { ModelInfo, providerIdentifiers } from "@roo-code/types"
8+
import {
9+
litellmDefaultModelId,
10+
type ModelInfo,
11+
type ProviderSettings,
12+
type RouterModels,
13+
providerIdentifiers,
14+
} from "@roo-code/types"
815

916
import { ModelPicker } from "../ModelPicker"
17+
import { useRouterModels } from "@src/components/ui/hooks/useRouterModels"
18+
19+
type SetApiConfigurationField = <K extends keyof ProviderSettings>(
20+
field: K,
21+
value: ProviderSettings[K],
22+
isUserAction?: boolean,
23+
) => void
24+
25+
// useRouterModels returns a react-query observable result; these tests only need the stable state fields.
26+
const createRouterModelsResult = (data: Partial<RouterModels>): ReturnType<typeof useRouterModels> =>
27+
({ data, isLoading: false, isError: false }) as ReturnType<typeof useRouterModels>
1028

1129
vi.mock("@src/context/ExtensionStateContext", () => ({
12-
ExtensionStateContextProvider: ({ children }: any) => children,
30+
ExtensionStateContextProvider: ({ children }: { children: ReactNode }) => children,
1331
useExtensionState: vi.fn(),
1432
}))
1533

34+
vi.mock("@src/components/ui/hooks/useRouterModels")
35+
36+
const mockUseRouterModels = useRouterModels as Mock<typeof useRouterModels>
37+
1638
Element.prototype.scrollIntoView = vi.fn()
1739

1840
describe("ModelPicker", () => {
@@ -34,8 +56,9 @@ describe("ModelPicker", () => {
3456
model2: { name: "Model 2", description: "Test model 2", ...modelInfo },
3557
}
3658

59+
const apiConfiguration: ProviderSettings = {}
3760
const defaultProps = {
38-
apiConfiguration: {},
61+
apiConfiguration,
3962
defaultModelId: "model1",
4063
modelIdKey: "openRouterModelId" as const,
4164
serviceName: "Test Service",
@@ -55,6 +78,8 @@ describe("ModelPicker", () => {
5578
beforeEach(() => {
5679
vi.clearAllMocks()
5780
vi.useFakeTimers()
81+
// Default: no router models available. Provider-specific tests override per test.
82+
mockUseRouterModels.mockReturnValue(createRouterModelsResult({}))
5883
})
5984

6085
afterEach(() => {
@@ -254,4 +279,89 @@ describe("ModelPicker", () => {
254279
expect(screen.getByTestId("automatic-fetch-hint")).toBeInTheDocument()
255280
})
256281
})
282+
283+
describe("LiteLLM custom model selection", () => {
284+
const litellmModels: Record<string, ModelInfo> = {
285+
"gpt-4o-mini": { description: "LiteLLM proxy model", ...modelInfo },
286+
}
287+
288+
const renderLiteLLMPicker = (apiConfiguration: ProviderSettings, setField: SetApiConfigurationField) =>
289+
renderWithExtensionState(
290+
<ModelPicker
291+
apiConfiguration={apiConfiguration}
292+
defaultModelId={litellmDefaultModelId}
293+
models={litellmModels}
294+
modelIdKey="litellmModelId"
295+
serviceName="LiteLLM"
296+
serviceUrl="https://docs.litellm.ai/"
297+
setApiConfigurationField={setField}
298+
organizationAllowList={{ allowAll: true, providers: {} }}
299+
/>,
300+
{ queryClient },
301+
)
302+
303+
beforeEach(() => {
304+
mockUseRouterModels.mockReturnValue(createRouterModelsResult({ litellm: litellmModels }))
305+
})
306+
307+
it("keeps a custom model ID in the picker instead of reverting to the default", async () => {
308+
// Regression: on the LiteLLM settings screen the user could not change the
309+
// model ID to a value absent from the fetched /models list -- the picker
310+
// silently reverted to the hardcoded default model after the selection.
311+
const customModelId = "my-litellm-alias"
312+
let apiConfiguration: ProviderSettings = { apiProvider: providerIdentifiers.litellm }
313+
const setField = vi.fn(function <K extends keyof ProviderSettings>(field: K, value: ProviderSettings[K]) {
314+
apiConfiguration = { ...apiConfiguration, [field]: value }
315+
})
316+
317+
const { rerender } = await act(async () => {
318+
return renderLiteLLMPicker(apiConfiguration, setField)
319+
})
320+
321+
// Before any selection the picker shows the provider default.
322+
expect(screen.getByTestId("model-picker-button")).toHaveTextContent(litellmDefaultModelId)
323+
324+
// Open the popover and type a model ID that is not in the fetched list.
325+
await act(async () => {
326+
fireEvent.click(screen.getByTestId("model-picker-button"))
327+
})
328+
await act(async () => {
329+
vi.advanceTimersByTime(100)
330+
})
331+
await act(async () => {
332+
fireEvent.input(screen.getByTestId("model-input"), { target: { value: customModelId } })
333+
})
334+
await act(async () => {
335+
vi.advanceTimersByTime(100)
336+
})
337+
await act(async () => {
338+
fireEvent.click(screen.getByTestId("use-custom-model"))
339+
})
340+
await act(async () => {
341+
vi.advanceTimersByTime(100)
342+
})
343+
344+
expect(setField).toHaveBeenCalledWith("litellmModelId", customModelId)
345+
346+
// Re-render with the updated configuration (as SettingsView does after the
347+
// setter runs) and assert the selection is kept, not reset to the default.
348+
await act(async () => {
349+
rerender(
350+
<ModelPicker
351+
apiConfiguration={apiConfiguration}
352+
defaultModelId={litellmDefaultModelId}
353+
models={litellmModels}
354+
modelIdKey="litellmModelId"
355+
serviceName="LiteLLM"
356+
serviceUrl="https://docs.litellm.ai/"
357+
setApiConfigurationField={setField}
358+
organizationAllowList={{ allowAll: true, providers: {} }}
359+
/>,
360+
)
361+
})
362+
363+
expect(screen.getByTestId("model-picker-button")).toHaveTextContent(customModelId)
364+
expect(screen.getByTestId("model-picker-button")).not.toHaveTextContent(litellmDefaultModelId)
365+
})
366+
})
257367
})

0 commit comments

Comments
 (0)