From 1366ba01155ed17a98a70868d583759ef3ec749a Mon Sep 17 00:00:00 2001 From: Chris Estreich Date: Fri, 23 May 2025 23:22:41 -0700 Subject: [PATCH] Preserve model settings when selecting a specific OpenRouter provider (#3915) --- .changeset/twelve-pigs-reply.md | 5 + src/extension.ts | 12 +- webview-ui/eslint.config.mjs | 11 +- .../common/__tests__/CodeBlock.test.tsx | 2 +- .../prompts/__tests__/PromptsView.test.tsx | 2 +- .../components/settings/CodeIndexSettings.tsx | 1 - .../settings/__tests__/SettingsView.test.tsx | 2 +- .../hooks/__tests__/useSelectedModel.test.ts | 373 ++++++++++++++++++ .../components/ui/hooks/useSelectedModel.ts | 7 +- .../src/components/ui/markdown/CodeBlock.tsx | 2 +- webview-ui/src/utils/TelemetryClient.ts | 4 +- 11 files changed, 407 insertions(+), 14 deletions(-) create mode 100644 .changeset/twelve-pigs-reply.md create mode 100644 webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.test.ts diff --git a/.changeset/twelve-pigs-reply.md b/.changeset/twelve-pigs-reply.md new file mode 100644 index 0000000000..41d961bae7 --- /dev/null +++ b/.changeset/twelve-pigs-reply.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Preserve model settings when selecting a specific OpenRouter provider diff --git a/src/extension.ts b/src/extension.ts index 5037027424..0244e425f8 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -145,17 +145,19 @@ export async function activate(context: vscode.ExtensionContext) { const socketPath = process.env.ROO_CODE_IPC_SOCKET_PATH const enableLogging = typeof socketPath === "string" - // Watch the core files and automatically reload the extension host - const enableCoreAutoReload = process.env?.NODE_ENV === "development" - if (enableCoreAutoReload) { - console.log(`♻️♻️♻️ Core auto-reloading is ENABLED!`) + // Watch the core files and automatically reload the extension host. + if (process.env.NODE_ENV === "development") { + console.log(`♻️♻️♻️ Core auto-reloading is ENABLED! Watching for changes in ${context.extensionPath}/**/*.ts`) + const watcher = vscode.workspace.createFileSystemWatcher( - new vscode.RelativePattern(context.extensionPath, "src/**/*.ts"), + new vscode.RelativePattern(context.extensionPath, "**/*.ts"), ) + watcher.onDidChange((uri) => { console.log(`♻️ File changed: ${uri.fsPath}. Reloading host…`) vscode.commands.executeCommand("workbench.action.reloadWindow") }) + context.subscriptions.push(watcher) } diff --git a/webview-ui/eslint.config.mjs b/webview-ui/eslint.config.mjs index 208700018b..db76f49211 100644 --- a/webview-ui/eslint.config.mjs +++ b/webview-ui/eslint.config.mjs @@ -5,7 +5,16 @@ export default [ ...reactConfig, { rules: { - "@typescript-eslint/no-unused-vars": "off", + "@typescript-eslint/no-unused-vars": [ + "error", + { + args: "all", + ignoreRestSiblings: true, + varsIgnorePattern: "^_", + argsIgnorePattern: "^_", + caughtErrorsIgnorePattern: "^_", + }, + ], "@typescript-eslint/no-explicit-any": "off", "react/prop-types": "off", "react/display-name": "off", diff --git a/webview-ui/src/components/common/__tests__/CodeBlock.test.tsx b/webview-ui/src/components/common/__tests__/CodeBlock.test.tsx index 5c84587110..3430a09ef0 100644 --- a/webview-ui/src/components/common/__tests__/CodeBlock.test.tsx +++ b/webview-ui/src/components/common/__tests__/CodeBlock.test.tsx @@ -33,7 +33,7 @@ jest.mock("lucide-react", () => { return new Proxy( {}, { - get: function (obj, prop) { + get: function (_obj, prop) { // Return a component factory for any icon that's requested if (prop === "__esModule") { return true diff --git a/webview-ui/src/components/prompts/__tests__/PromptsView.test.tsx b/webview-ui/src/components/prompts/__tests__/PromptsView.test.tsx index 2015ffd0fb..3a92e4d3e1 100644 --- a/webview-ui/src/components/prompts/__tests__/PromptsView.test.tsx +++ b/webview-ui/src/components/prompts/__tests__/PromptsView.test.tsx @@ -17,7 +17,7 @@ jest.mock("lucide-react", () => { return new Proxy( {}, { - get: function (obj, prop) { + get: function (_obj, prop) { // Return a component factory for any icon that's requested if (prop === "__esModule") { return true diff --git a/webview-ui/src/components/settings/CodeIndexSettings.tsx b/webview-ui/src/components/settings/CodeIndexSettings.tsx index be65a2456c..cd45a6921b 100644 --- a/webview-ui/src/components/settings/CodeIndexSettings.tsx +++ b/webview-ui/src/components/settings/CodeIndexSettings.tsx @@ -17,7 +17,6 @@ import { } from "@/components/ui/alert-dialog" import { vscode } from "@/utils/vscode" -import { ExtensionStateContextType } from "@/context/ExtensionStateContext" import { CodebaseIndexConfig, CodebaseIndexModels, ProviderSettings } from "../../../../src/schemas" import { EmbedderProvider } from "../../../../src/shared/embeddingModels" import { z } from "zod" diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.test.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.test.tsx index 072296a754..f53eb633b2 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.test.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.test.tsx @@ -15,7 +15,7 @@ jest.mock("lucide-react", () => { return new Proxy( {}, { - get: function (obj, prop) { + get: function (_obj, prop) { // Return a component factory for any icon that's requested if (prop === "__esModule") { return true diff --git a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.test.ts b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.test.ts new file mode 100644 index 0000000000..df7add4d25 --- /dev/null +++ b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.test.ts @@ -0,0 +1,373 @@ +// npx jest src/components/ui/hooks/__tests__/useSelectedModel.test.ts + +import React from "react" +import { QueryClient, QueryClientProvider } from "@tanstack/react-query" +import { renderHook } from "@testing-library/react" + +import { ProviderSettings, ModelInfo } from "@roo/shared/api" + +import { useSelectedModel } from "../useSelectedModel" +import { useRouterModels } from "../useRouterModels" +import { useOpenRouterModelProviders } from "../useOpenRouterModelProviders" + +jest.mock("../useRouterModels") +jest.mock("../useOpenRouterModelProviders") + +const mockUseRouterModels = useRouterModels as jest.MockedFunction +const mockUseOpenRouterModelProviders = useOpenRouterModelProviders as jest.MockedFunction< + typeof useOpenRouterModelProviders +> + +const createWrapper = () => { + const queryClient = new QueryClient({ + defaultOptions: { + queries: { + retry: false, + }, + }, + }) + return ({ children }: { children: React.ReactNode }) => + React.createElement(QueryClientProvider, { client: queryClient }, children) +} + +describe("useSelectedModel", () => { + describe("OpenRouter provider merging", () => { + it("should merge base model info with specific provider info when both exist", () => { + const baseModelInfo: ModelInfo = { + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + } + + const specificProviderInfo: ModelInfo = { + maxTokens: 8192, // Different value that should override + contextWindow: 16384, // Different value that should override + supportsImages: true, // Different value that should override + supportsPromptCache: true, // Different value that should override + inputPrice: 0.001, + outputPrice: 0.002, + description: "Provider-specific description", + } + + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: { + "test-model": baseModelInfo, + }, + requesty: {}, + glama: {}, + unbound: {}, + litellm: {}, + }, + isLoading: false, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: { + "test-provider": specificProviderInfo, + }, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "openrouter", + openRouterModelId: "test-model", + openRouterSpecificProvider: "test-provider", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.id).toBe("test-model") + expect(result.current.info).toEqual({ + maxTokens: 8192, // From specific provider (overrides base) + contextWindow: 16384, // From specific provider (overrides base) + supportsImages: true, // From specific provider (overrides base) + supportsPromptCache: true, // From specific provider (overrides base) + inputPrice: 0.001, + outputPrice: 0.002, + description: "Provider-specific description", + }) + }) + + it("should use only specific provider info when base model info is missing", () => { + const specificProviderInfo: ModelInfo = { + maxTokens: 8192, + contextWindow: 16384, + supportsImages: true, + supportsPromptCache: true, + inputPrice: 0.001, + outputPrice: 0.002, + description: "Provider-specific description", + } + + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: {}, + requesty: {}, + glama: {}, + unbound: {}, + litellm: {}, + }, + isLoading: false, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: { + "test-provider": specificProviderInfo, + }, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "openrouter", + openRouterModelId: "test-model", + openRouterSpecificProvider: "test-provider", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.id).toBe("test-model") + expect(result.current.info).toEqual(specificProviderInfo) + }) + + it("should demonstrate the merging behavior validates the comment about missing fields", () => { + const baseModelInfo: ModelInfo = { + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + supportsComputerUse: true, + cacheWritesPrice: 0.1, + cacheReadsPrice: 0.01, + } + + const specificProviderInfo: Partial = { + inputPrice: 0.001, + outputPrice: 0.002, + description: "Provider-specific description", + maxTokens: 8192, // Override this one + supportsImages: true, // Override this one + } + + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: { + "test-model": baseModelInfo, + }, + requesty: {}, + glama: {}, + unbound: {}, + litellm: {}, + }, + isLoading: false, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: { "test-provider": specificProviderInfo as ModelInfo }, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "openrouter", + openRouterModelId: "test-model", + openRouterSpecificProvider: "test-provider", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.id).toBe("test-model") + expect(result.current.info).toEqual({ + // Fields from base model that provider doesn't have + contextWindow: 8192, // From base (provider doesn't override) + supportsPromptCache: false, // From base (provider doesn't override) + supportsComputerUse: true, // From base (provider doesn't have) + cacheWritesPrice: 0.1, // From base (provider doesn't have) + cacheReadsPrice: 0.01, // From base (provider doesn't have) + + // Fields overridden by provider + maxTokens: 8192, // From provider (overrides base) + supportsImages: true, // From provider (overrides base) + + // Fields only in provider + inputPrice: 0.001, // From provider (base doesn't have) + outputPrice: 0.002, // From provider (base doesn't have) + description: "Provider-specific description", // From provider (base doesn't have) + }) + }) + + it("should use base model info when no specific provider is configured", () => { + const baseModelInfo: ModelInfo = { + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + } + + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: { "test-model": baseModelInfo }, + requesty: {}, + glama: {}, + unbound: {}, + litellm: {}, + }, + isLoading: false, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: {}, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "openrouter", + openRouterModelId: "test-model", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.id).toBe("test-model") + expect(result.current.info).toEqual(baseModelInfo) + }) + + it("should fall back to default when both base and specific provider info are missing", () => { + mockUseRouterModels.mockReturnValue({ + data: { + openrouter: { + "anthropic/claude-3.7-sonnet": { + // Default model + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + }, + }, + requesty: {}, + glama: {}, + unbound: {}, + litellm: {}, + }, + isLoading: false, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: {}, + isLoading: false, + isError: false, + } as any) + + const apiConfiguration: ProviderSettings = { + apiProvider: "openrouter", + openRouterModelId: "non-existent-model", + openRouterSpecificProvider: "non-existent-provider", + } + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(apiConfiguration), { wrapper }) + + expect(result.current.id).toBe("anthropic/claude-3.7-sonnet") + expect(result.current.info).toEqual({ + maxTokens: 4096, + contextWindow: 8192, + supportsImages: false, + supportsPromptCache: false, + }) + }) + }) + + describe("loading and error states", () => { + it("should return loading state when router models are loading", () => { + mockUseRouterModels.mockReturnValue({ + data: undefined, + isLoading: true, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: undefined, + isLoading: false, + isError: false, + } as any) + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(), { wrapper }) + + expect(result.current.isLoading).toBe(true) + }) + + it("should return loading state when open router model providers are loading", () => { + mockUseRouterModels.mockReturnValue({ + data: { openrouter: {}, requesty: {}, glama: {}, unbound: {}, litellm: {} }, + isLoading: false, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: undefined, + isLoading: true, + isError: false, + } as any) + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(), { wrapper }) + + expect(result.current.isLoading).toBe(true) + }) + + it("should return error state when either hook has an error", () => { + mockUseRouterModels.mockReturnValue({ + data: undefined, + isLoading: false, + isError: true, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: {}, + isLoading: false, + isError: false, + } as any) + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(), { wrapper }) + + expect(result.current.isError).toBe(true) + }) + }) + + describe("default behavior", () => { + it("should return anthropic default when no configuration is provided", () => { + mockUseRouterModels.mockReturnValue({ + data: undefined, + isLoading: false, + isError: false, + } as any) + + mockUseOpenRouterModelProviders.mockReturnValue({ + data: undefined, + isLoading: false, + isError: false, + } as any) + + const wrapper = createWrapper() + const { result } = renderHook(() => useSelectedModel(), { wrapper }) + + expect(result.current.provider).toBe("anthropic") + expect(result.current.id).toBe("claude-3-7-sonnet-20250219") + expect(result.current.info).toBeUndefined() + }) + }) +}) diff --git a/webview-ui/src/components/ui/hooks/useSelectedModel.ts b/webview-ui/src/components/ui/hooks/useSelectedModel.ts index 0bfa136b39..e28b24824b 100644 --- a/webview-ui/src/components/ui/hooks/useSelectedModel.ts +++ b/webview-ui/src/components/ui/hooks/useSelectedModel.ts @@ -82,7 +82,12 @@ function getSelectedModel({ const specificProvider = apiConfiguration.openRouterSpecificProvider if (specificProvider && openRouterModelProviders[specificProvider]) { - info = openRouterModelProviders[specificProvider] + // Overwrite the info with the specific provider info. Some + // fields are missing the model info for `openRouterModelProviders` + // so we need to merge the two. + info = info + ? { ...info, ...openRouterModelProviders[specificProvider] } + : openRouterModelProviders[specificProvider] } return info diff --git a/webview-ui/src/components/ui/markdown/CodeBlock.tsx b/webview-ui/src/components/ui/markdown/CodeBlock.tsx index 5342e6e7a6..b1fa0fbd86 100644 --- a/webview-ui/src/components/ui/markdown/CodeBlock.tsx +++ b/webview-ui/src/components/ui/markdown/CodeBlock.tsx @@ -44,7 +44,7 @@ export const CodeBlock: FC = memo(({ language, value, className, }) setHighlightedCode(html) - } catch (e) { + } catch (_e) { setHighlightedCode(value) } } diff --git a/webview-ui/src/utils/TelemetryClient.ts b/webview-ui/src/utils/TelemetryClient.ts index dd8c664754..8b502da5a4 100644 --- a/webview-ui/src/utils/TelemetryClient.ts +++ b/webview-ui/src/utils/TelemetryClient.ts @@ -35,8 +35,8 @@ class TelemetryClient { if (TelemetryClient.telemetryEnabled) { try { posthog.capture(eventName, properties) - } catch (error) { - // Silently fail if there's an error capturing an event + } catch (_error) { + // Silently fail if there's an error capturing an event. } } }