From 6e5246a05f7a84eb5d50085e3e38bf4da624e669 Mon Sep 17 00:00:00 2001 From: Povilas Kanapickas Date: Thu, 29 Oct 2026 12:16:16 +0000 Subject: [PATCH] [refactor] Guarantee a defined apiProvider for runtime consumers Every runtime consumer now receives an API configuration whose `apiProvider` is always defined, while the stored and partial `ProviderSettings` shape stays unchanged so storage, import/export, partial updates and fixtures are unaffected. - Add `DEFAULT_API_PROVIDER`, `ResolvedProviderSettings`, `resolveProviderSettings()` and `getActiveProvider()` to the types package. Resolution only fills a missing provider and passes retired names through, because the webview needs them for its warnings. - Type `ExtensionState.apiConfiguration` as `ResolvedProviderSettings`; inbound message payloads stay `ProviderSettings`. - Resolve in `ClineProvider.getState()`, `getStateToPostToWebview()` (including the task configuration) and `updateProfileModel()`, and replace the repeated retired-provider narrowing with `getActiveProvider()`. - Seed the webview's initial state with the default provider and re-resolve after `setApiConfiguration` merges, so an explicit `apiProvider: undefined` cannot erase it. - Replace the scattered OpenRouter/Anthropic fallbacks in `useSelectedModel`, `ModelSelector` and `validate` with `DEFAULT_API_PROVIDER`. Unset drafts and tests now resolve to Anthropic, matching the host. - Base the welcome flow's "not configured" check on `checkExistKey` instead of provider presence, so a resolved but keyless Anthropic configuration still shows the OpenRouter suggestion. --- .../__tests__/resolveProviderSettings.spec.ts | 56 +++++++++++++++++++ packages/types/src/provider-settings.ts | 50 +++++++++++++++++ packages/types/src/vscode-extension-host.ts | 4 +- src/core/task/__tests__/Task.spec.ts | 13 +++-- .../Task.streaming-tool-calls.spec.ts | 4 +- src/core/webview/ClineProvider.ts | 35 +++++------- .../webview/__tests__/ClineProvider.spec.ts | 52 +++++++++++++++++ .../__tests__/checkExistApiConfig.spec.ts | 13 ++++- webview-ui/playwright/gallery/stories.tsx | 4 +- .../src/components/chat/ModelSelector.tsx | 3 +- .../chat/__tests__/ChatView.spec.tsx | 28 +++++++++- .../chat/__tests__/CodeIndexPopover.test.tsx | 8 +-- .../chat/__tests__/ModelSelector.spec.tsx | 8 ++- .../settings/__tests__/SettingsView.spec.tsx | 6 +- .../SettingsView.unsaved-changes.spec.tsx | 4 +- .../hooks/__tests__/useSelectedModel.spec.ts | 24 ++++---- .../components/ui/hooks/useSelectedModel.ts | 3 +- .../welcome/WelcomeViewProvider.tsx | 6 +- .../__tests__/WelcomeViewProvider.spec.tsx | 19 ++++++- .../src/context/ExtensionStateContext.tsx | 14 +++-- .../__tests__/ExtensionStateContext.spec.tsx | 48 ++++++++++++++-- webview-ui/src/utils/validate.ts | 3 +- 22 files changed, 333 insertions(+), 72 deletions(-) create mode 100644 packages/types/src/__tests__/resolveProviderSettings.spec.ts diff --git a/packages/types/src/__tests__/resolveProviderSettings.spec.ts b/packages/types/src/__tests__/resolveProviderSettings.spec.ts new file mode 100644 index 0000000000..b0f01d9c32 --- /dev/null +++ b/packages/types/src/__tests__/resolveProviderSettings.spec.ts @@ -0,0 +1,56 @@ +import { providerIdentifiers, retiredProviderIdentifiers } from "../provider-identifiers.js" +import { + DEFAULT_API_PROVIDER, + getActiveProvider, + isRetiredProvider, + resolveProviderSettings, + type ProviderSettings, +} from "../provider-settings.js" + +describe("resolveProviderSettings", () => { + it("uses the shared default provider when none is set", () => { + expect(DEFAULT_API_PROVIDER).toBe(providerIdentifiers.anthropic) + expect(resolveProviderSettings({}).apiProvider).toBe(DEFAULT_API_PROVIDER) + }) + + it.each([ + { name: "explicit provider", settings: { apiProvider: providerIdentifiers.openrouter } }, + { name: "retired provider", settings: { apiProvider: retiredProviderIdentifiers.roo } }, + ] as const)("keeps an $name unchanged", ({ settings }) => { + expect(resolveProviderSettings(settings).apiProvider).toBe(settings.apiProvider) + }) + + it("keeps retired providers instead of replacing them", () => { + const resolved = resolveProviderSettings({ apiProvider: retiredProviderIdentifiers.groq }) + + expect(resolved.apiProvider).toBe(retiredProviderIdentifiers.groq) + expect(isRetiredProvider(resolved.apiProvider)).toBe(true) + }) + + it("preserves the remaining settings", () => { + const resolved = resolveProviderSettings({ apiModelId: "claude-sonnet-4-5", apiKey: "test-key" }) + + expect(resolved).toEqual({ + apiProvider: DEFAULT_API_PROVIDER, + apiModelId: "claude-sonnet-4-5", + apiKey: "test-key", + }) + }) + + it("returns a new object and does not mutate the input", () => { + const settings: ProviderSettings = {} + const resolved = resolveProviderSettings(settings) + + expect(resolved).not.toBe(settings) + expect(settings.apiProvider).toBeUndefined() + }) +}) + +describe("getActiveProvider", () => { + it.each([ + { name: "active provider", provider: providerIdentifiers.openrouter, expected: providerIdentifiers.openrouter }, + { name: "retired provider", provider: retiredProviderIdentifiers.roo, expected: undefined }, + ] as const)("returns $expected for an $name", ({ provider, expected }) => { + expect(getActiveProvider(resolveProviderSettings({ apiProvider: provider }))).toBe(expected) + }) +}) diff --git a/packages/types/src/provider-settings.ts b/packages/types/src/provider-settings.ts index ebb48b041b..f1884d2150 100644 --- a/packages/types/src/provider-settings.ts +++ b/packages/types/src/provider-settings.ts @@ -283,6 +283,56 @@ export const providerSettingsSchema = z.object({ export type ProviderSettings = z.infer +/** + * The provider used when a configuration has no explicit `apiProvider`. + * + * This value is supplied at read time only: `resolveProviderSettings()` fills it + * in for a configuration (or profile) that has no stored `apiProvider`, so an + * unsaved profile follows whatever the default is at that moment. + * + * Resolution does not make the value permanent. Once the resolved configuration + * is saved (for example when the settings view persists the profile it was + * given), `apiProvider` is stored explicitly, and that profile keeps the value + * it was saved with. Later changes to this constant therefore do not affect + * already-saved profiles. + * + * Saves must keep writing `apiProvider` rather than omitting it to "stay on the + * default": the discriminated stored-profile schema requires a provider for + * active providers, and omitting it would silently re-open the profile to future + * default changes. + */ +export const DEFAULT_API_PROVIDER = providerIdentifiers.anthropic satisfies ProviderName + +/** + * A `ProviderSettings` whose `apiProvider` is guaranteed to be defined. + * + * The value may still be a retired provider name: resolution only fills in a + * *missing* provider, it does not filter retired ones, because the webview + * needs to see them to show the retired-provider warnings. + */ +export type ResolvedProviderSettings = ProviderSettings & { + apiProvider: NonNullable // ProviderName | RetiredProviderName +} + +/** + * Returns a new configuration with `apiProvider` filled in when missing. + * + * Does not mutate the input and passes retired provider names through + * unchanged. Consumers that need an *active* provider should narrow the + * result with `isRetiredProvider()` or use `getActiveProvider()`. + */ +export const resolveProviderSettings = (settings: ProviderSettings): ResolvedProviderSettings => ({ + ...settings, + apiProvider: settings.apiProvider ?? DEFAULT_API_PROVIDER, +}) + +/** + * Returns the provider when it is still active, or `undefined` for retired + * providers. + */ +export const getActiveProvider = (settings: ResolvedProviderSettings): ProviderName | undefined => + isRetiredProvider(settings.apiProvider) ? undefined : settings.apiProvider + export const providerSettingsWithIdSchema = providerSettingsSchema.extend({ id: z.string().optional() }) export const discriminatedProviderSettingsWithIdSchema = providerSettingsSchemaDiscriminated.and( diff --git a/packages/types/src/vscode-extension-host.ts b/packages/types/src/vscode-extension-host.ts index 55cf302722..5c0165d0e6 100644 --- a/packages/types/src/vscode-extension-host.ts +++ b/packages/types/src/vscode-extension-host.ts @@ -1,7 +1,7 @@ import { z } from "zod" import type { GlobalSettings, RooCodeSettings } from "./global-settings.js" -import type { ProviderSettings, ProviderSettingsEntry } from "./provider-settings.js" +import type { ProviderSettings, ProviderSettingsEntry, ResolvedProviderSettings } from "./provider-settings.js" import type { HistoryItem } from "./history.js" import type { ModeConfig, PromptComponent } from "./mode.js" import type { Experiments } from "./experiment.js" @@ -353,7 +353,7 @@ export type ExtensionState = Pick< currentTaskId?: string currentTaskItem?: HistoryItem currentTaskTodos?: TodoItem[] // Initial todos for the current task - apiConfiguration: ProviderSettings + apiConfiguration: ResolvedProviderSettings uriScheme?: string shouldShowAnnouncement: boolean diff --git a/src/core/task/__tests__/Task.spec.ts b/src/core/task/__tests__/Task.spec.ts index f52f29174c..abe7c571ee 100644 --- a/src/core/task/__tests__/Task.spec.ts +++ b/src/core/task/__tests__/Task.spec.ts @@ -18,6 +18,7 @@ import { type HistoryItem, type ModeConfig, type ProviderSettings, + type ResolvedProviderSettings, type ModelInfo, type TaskLike, } from "@roo-code/types" @@ -76,7 +77,7 @@ type MockedClineProvider = ClineProvider & { mockResolvedValue: (value: Partial) => void } } -type RateLimitedProviderSettings = ProviderSettings & { +type RateLimitedProviderSettings = ResolvedProviderSettings & { rateLimitSeconds: number } @@ -321,7 +322,7 @@ const stubModelInfo: ModelInfo = { describe("Cline", () => { let mockProvider: ClineProvider - let mockApiConfig: ProviderSettings + let mockApiConfig: ResolvedProviderSettings let mockOutputChannel: vscode.OutputChannel let mockExtensionContext: vscode.ExtensionContext @@ -3884,7 +3885,7 @@ describe("Cline", () => { describe("Dynamic Strategy Selection", () => { let mockProvider: MockedClineProvider - let mockApiConfig: ProviderSettings + let mockApiConfig: ResolvedProviderSettings beforeEach(() => { vi.clearAllMocks() @@ -4093,7 +4094,7 @@ describe("Cline", () => { }) it("stores a provider profile selected through submitUserMessage", async () => { - const selectedConfiguration: ProviderSettings = { + const selectedConfiguration: ResolvedProviderSettings = { ...mockApiConfig, apiModelId: "selected-model", } @@ -4825,10 +4826,10 @@ describe("Cline", () => { }) it("configures tool restrictions for Gemini requests", async () => { - const apiConfiguration = { + const apiConfiguration: ResolvedProviderSettings = { ...mockApiConfig, apiProvider: providerIdentifiers.gemini, - } as ProviderSettings + } const task = new Task({ provider: mockProvider, apiConfiguration, diff --git a/src/core/task/__tests__/Task.streaming-tool-calls.spec.ts b/src/core/task/__tests__/Task.streaming-tool-calls.spec.ts index bff09e9cab..1a42e28ded 100644 --- a/src/core/task/__tests__/Task.streaming-tool-calls.spec.ts +++ b/src/core/task/__tests__/Task.streaming-tool-calls.spec.ts @@ -6,7 +6,7 @@ import { NativeToolCallParser } from "../../assistant-message/NativeToolCallPars import { ClineProvider } from "../../webview/ClineProvider" import { WebviewFocusTracker } from "../../webview/WebviewFocusTracker" import { providerIdentifiers } from "@roo-code/types/provider-identifiers" -import type { ProviderSettings } from "@roo-code/types" +import type { ResolvedProviderSettings } from "@roo-code/types" import { ApiStreamChunk } from "../../../api/transform/stream" import { ContextProxy } from "../../config/ContextProxy" import { TelemetryService } from "@roo-code/telemetry" @@ -175,7 +175,7 @@ vi.mock("../../../utils/fs", () => ({ describe("Task - Streaming Tool Call Handling", () => { let mockProvider: ClineProvider - let mockApiConfig: ProviderSettings + let mockApiConfig: ResolvedProviderSettings let mockOutputChannel: vscode.OutputChannel let scope: object let mockExtensionContext: vscode.ExtensionContext diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index 636c022f62..295c79de45 100644 --- a/src/core/webview/ClineProvider.ts +++ b/src/core/webview/ClineProvider.ts @@ -29,7 +29,6 @@ import { type TaskProviderLike, type TaskProviderEvents, type GlobalState, - type ProviderName, type ProviderSettings, type RooCodeSettings, type ProviderSettingsEntry, @@ -69,10 +68,11 @@ import { ORGANIZATION_ALLOW_ALL, DEFAULT_MODES, DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, + getActiveProvider, getModelId, - isRetiredProvider, modelIdKeys, providerIdentifiers, + resolveProviderSettings, } from "@roo-code/types" import { RateLimitClock, createRateLimitClock } from "../task/RateLimitClock" import { TaskRegistry } from "../task/TaskRegistry" @@ -2070,8 +2070,9 @@ export class ClineProvider async (signal) => { const { name: _name, ...stored } = await this.providerSettingsManager.getProfile({ name }) - // Unset provider (e.g. the seeded default profile) resolves to anthropic, matching getState(). - const storedProvider = stored.apiProvider ?? providerIdentifiers.anthropic + // Unset provider (e.g. the seeded default profile) resolves to the shared default, + // matching getState(). + const storedProvider = resolveProviderSettings(stored).apiProvider if (storedProvider !== expectedProvider) { throw new Error( @@ -3067,7 +3068,11 @@ export class ClineProvider return { version: this.context.extension?.packageJSON?.version ?? "", - apiConfiguration: currentTaskApiConfiguration ?? apiConfiguration, + // The task configuration is a separate source and needs the same "provider is defined" + // guarantee as the persisted settings. + apiConfiguration: currentTaskApiConfiguration + ? resolveProviderSettings(currentTaskApiConfiguration) + : apiConfiguration, customInstructions, alwaysAllowReadOnly: alwaysAllowReadOnly ?? false, alwaysAllowReadOnlyOutsideWorkspace: alwaysAllowReadOnlyOutsideWorkspace ?? false, @@ -3242,19 +3247,7 @@ export class ClineProvider const stateValues = this.contextProxy.getValues() const customModes = await this.customModesManager.getCustomModes() - // Determine apiProvider with the same logic as before, while filtering retired providers. - const apiProvider: ProviderName = - stateValues.apiProvider && !isRetiredProvider(stateValues.apiProvider) - ? stateValues.apiProvider - : providerIdentifiers.anthropic - - // Build the apiConfiguration object combining state values and secrets. - const providerSettings = this.contextProxy.getProviderSettings() - - // Ensure apiProvider is set properly if not already in state - if (!providerSettings.apiProvider) { - providerSettings.apiProvider = apiProvider - } + const apiConfiguration = resolveProviderSettings(this.contextProxy.getProviderSettings()) let organizationAllowList = ORGANIZATION_ALLOW_ALL @@ -3307,7 +3300,7 @@ export class ClineProvider // Return the same structure as before. return { - apiConfiguration: providerSettings, + apiConfiguration, lastShownAnnouncementId: stateValues.lastShownAnnouncementId, customInstructions: stateValues.customInstructions, apiModelId: stateValues.apiModelId, @@ -4434,14 +4427,12 @@ export class ClineProvider } } - const apiProvider = apiConfiguration?.apiProvider - return { language, mode, taskId: task?.taskId, parentTaskId: task?.parentTaskId, - apiProvider: apiProvider && !isRetiredProvider(apiProvider) ? apiProvider : undefined, + apiProvider: getActiveProvider(apiConfiguration), modelId: task?.api?.getModel().id, diffStrategy: task?.diffStrategy?.getName(), isSubtask: task ? !!task.parentTaskId : undefined, diff --git a/src/core/webview/__tests__/ClineProvider.spec.ts b/src/core/webview/__tests__/ClineProvider.spec.ts index d0155955d0..d5be8354c5 100644 --- a/src/core/webview/__tests__/ClineProvider.spec.ts +++ b/src/core/webview/__tests__/ClineProvider.spec.ts @@ -20,7 +20,9 @@ import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, DEFAULT_DIFF_FUZZY_THRESHOLD, DEFAULT_WRITE_DELAY_MS, + DEFAULT_API_PROVIDER, providerIdentifiers, + retiredProviderIdentifiers, openAiModelInfoSaneDefaults, } from "@roo-code/types" import { TelemetryService } from "@roo-code/telemetry" @@ -3960,6 +3962,45 @@ describe("ClineProvider", () => { expect(state).toHaveProperty("writeDelayMs") }) + describe("resolved apiConfiguration", () => { + test("getState and getStateToPostToWebview resolve a missing provider to the shared default", async () => { + await provider.contextProxy.setProviderSettings({ apiModelId: "claude-sonnet-4-5" }) + + expect((await provider.getState()).apiConfiguration.apiProvider).toBe(DEFAULT_API_PROVIDER) + expect((await provider.getStateToPostToWebview()).apiConfiguration.apiProvider).toBe(DEFAULT_API_PROVIDER) + }) + + test("keeps a stored provider", async () => { + await provider.contextProxy.setProviderSettings({ apiProvider: providerIdentifiers.openrouter }) + + expect((await provider.getState()).apiConfiguration.apiProvider).toBe(providerIdentifiers.openrouter) + expect((await provider.getStateToPostToWebview()).apiConfiguration.apiProvider).toBe( + providerIdentifiers.openrouter, + ) + }) + + test("passes a stored retired provider through so the webview can warn", async () => { + // `roo` is stripped by the legacy-router downgrade, so use another retired provider. + await provider.contextProxy.setProviderSettings({ apiProvider: retiredProviderIdentifiers.groq }) + + expect((await provider.getState()).apiConfiguration.apiProvider).toBe(retiredProviderIdentifiers.groq) + expect((await provider.getStateToPostToWebview()).apiConfiguration.apiProvider).toBe( + retiredProviderIdentifiers.groq, + ) + }) + + test("resolves the task configuration when it has no provider", async () => { + vi.spyOn(provider, "getCurrentTask").mockReturnValue({ + apiConfiguration: { apiModelId: "task-local-model" }, + } as unknown as Task) + + const postedState = await provider.getStateToPostToWebview() + + expect(postedState.apiConfiguration.apiProvider).toBe(DEFAULT_API_PROVIDER) + expect(postedState.apiConfiguration.apiModelId).toBe("task-local-model") + }) + }) + test("getState and getStateToPostToWebview return the complete NanoGPT configuration", async () => { await provider.resolveWebviewView(mockWebviewView) await provider.contextProxy.setProviderSettings({ @@ -6256,6 +6297,17 @@ describe("getTelemetryProperties", () => { expect(properties).toHaveProperty("modelId", "claude-sonnet-4-20250514") }) + it.each([ + { name: "active provider", apiProvider: providerIdentifiers.openrouter, expected: "openrouter" }, + { name: "retired provider", apiProvider: retiredProviderIdentifiers.groq, expected: undefined }, + ] as const)("reports $name in telemetry as $expected", async ({ apiProvider, expected }) => { + await provider.contextProxy.setProviderSettings({ apiProvider }) + + const properties = await provider.getTelemetryProperties() + + expect(properties.apiProvider).toBe(expected) + }) + describe("cloud authentication telemetry", () => { beforeEach(() => { // Reset all mocks before each test diff --git a/src/shared/__tests__/checkExistApiConfig.spec.ts b/src/shared/__tests__/checkExistApiConfig.spec.ts index 570e5c8223..52585b475b 100644 --- a/src/shared/__tests__/checkExistApiConfig.spec.ts +++ b/src/shared/__tests__/checkExistApiConfig.spec.ts @@ -1,6 +1,11 @@ // npx vitest run src/shared/__tests__/checkExistApiConfig.spec.ts -import { providerIdentifiers, retiredProviderIdentifiers, type ProviderSettings } from "@roo-code/types" +import { + DEFAULT_API_PROVIDER, + providerIdentifiers, + retiredProviderIdentifiers, + type ProviderSettings, +} from "@roo-code/types" import { checkExistKey } from "../checkExistApiConfig" @@ -162,4 +167,10 @@ describe("checkExistKey", () => { } expect(checkExistKey(config, true)).toBe(false) }) + + it("should return false for a resolved but keyless default provider", () => { + // The host resolves `apiProvider` before posting state, so the welcome gate must + // decide from keys rather than from provider presence. + expect(checkExistKey({ apiProvider: DEFAULT_API_PROVIDER })).toBe(false) + }) }) diff --git a/webview-ui/playwright/gallery/stories.tsx b/webview-ui/playwright/gallery/stories.tsx index 85d6919db4..824d6326d0 100644 --- a/webview-ui/playwright/gallery/stories.tsx +++ b/webview-ui/playwright/gallery/stories.tsx @@ -1,5 +1,7 @@ import React, { useState } from "react" +import { resolveProviderSettings } from "@roo-code/types" + type StoryProps = Record type Story = (props: StoryProps) => React.ReactNode | Promise @@ -433,7 +435,7 @@ export const stories: Record = { import("@/components/welcome/WelcomeLanding"), ]) return ( - + undefined} onImportSettings={() => undefined} /> ) diff --git a/webview-ui/src/components/chat/ModelSelector.tsx b/webview-ui/src/components/chat/ModelSelector.tsx index 0e5b92b59b..1f3e638900 100644 --- a/webview-ui/src/components/chat/ModelSelector.tsx +++ b/webview-ui/src/components/chat/ModelSelector.tsx @@ -8,6 +8,7 @@ import { type ModelInfo, providerIdentifiers, isRetiredProvider, + DEFAULT_API_PROVIDER, } from "@roo-code/types" import { cn } from "@/lib/utils" @@ -90,7 +91,7 @@ export const ModelSelector = ({ const { t } = useAppTranslation() const { organizationAllowList, currentApiConfigName } = useExtensionState() - const providerRaw = apiConfiguration.apiProvider || providerIdentifiers.openrouter + const providerRaw = apiConfiguration.apiProvider ?? DEFAULT_API_PROVIDER const provider: ProviderName | undefined = isRetiredProvider(providerRaw) ? undefined : providerRaw const { id: selectedModelId, info: selectedModelInfo } = useSelectedModel(apiConfiguration) diff --git a/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx b/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx index 42edc94b9f..c11d272613 100644 --- a/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ChatView.spec.tsx @@ -12,7 +12,7 @@ import { } from "@/utils/test-utils" import { vscode } from "@src/utils/vscode" -import type { SuggestionItem } from "@roo-code/types" +import { retiredProviderIdentifiers, type SuggestionItem } from "@roo-code/types" import ChatView, { ChatViewProps } from "../ChatView" @@ -1823,3 +1823,29 @@ describe("ChatView - Context Condensing Indicator Tests", () => { ) }) }) + +describe("ChatView - Retired provider warning", () => { + beforeEach(() => vi.clearAllMocks()) + + it("warns instead of sending when the resolved configuration keeps a retired provider", async () => { + // The host resolves `apiProvider` but deliberately passes retired names through, + // so this warning path must keep working on a resolved configuration. + const { getByTestId, getByText } = renderChatView() + + mockPostMessage({ + apiConfiguration: { apiProvider: retiredProviderIdentifiers.roo }, + clineMessages: [{ type: "say", say: "task", ts: Date.now() - 1000, text: "Initial task" }], + }) + + await waitFor(() => expect(getByTestId("chat-textarea")).toBeInTheDocument()) + + const input = getByTestId("chat-textarea").querySelector("input")! + fireEvent.change(input, { target: { value: "hello" } }) + fireEvent.keyDown(input, { key: "Enter", code: "Enter" }) + + await waitFor(() => expect(getByText("chat:retiredProvider.title")).toBeInTheDocument()) + expect(vscode.postMessage).not.toHaveBeenCalledWith( + expect.objectContaining({ type: "askResponse", askResponse: "messageResponse" }), + ) + }) +}) diff --git a/webview-ui/src/components/chat/__tests__/CodeIndexPopover.test.tsx b/webview-ui/src/components/chat/__tests__/CodeIndexPopover.test.tsx index e27a10d51c..7656ebd5c5 100644 --- a/webview-ui/src/components/chat/__tests__/CodeIndexPopover.test.tsx +++ b/webview-ui/src/components/chat/__tests__/CodeIndexPopover.test.tsx @@ -1,7 +1,7 @@ import React from "react" import type { EmbedderProvider, ExtensionState } from "@roo-code/types" -import { providerIdentifiers } from "@roo-code/types" +import { providerIdentifiers, resolveProviderSettings } from "@roo-code/types" import { fireEvent, renderWithExtensionState, screen } from "@/utils/test-utils" import { useOpenRouterModelProviders } from "@src/components/ui/hooks/useOpenRouterModelProviders" @@ -119,7 +119,7 @@ describe("CodeIndexPopover OpenRouter provider lookup", () => { const renderPopover = ( provider: EmbedderProvider, modelId: string, - apiProvider?: ExtensionState["apiConfiguration"], + apiProvider: ExtensionState["apiConfiguration"] = resolveProviderSettings({}), ) => renderWithExtensionState( @@ -204,8 +204,8 @@ describe("CodeIndexPopover OpenRouter provider lookup", () => { expectedProfile: "", }, { - name: "unset API provider", - apiConfiguration: {}, + name: "default API provider", + apiConfiguration: resolveProviderSettings({}), expectedRegion: "", expectedProfile: "", }, diff --git a/webview-ui/src/components/chat/__tests__/ModelSelector.spec.tsx b/webview-ui/src/components/chat/__tests__/ModelSelector.spec.tsx index d4dd381015..9e6e72675f 100644 --- a/webview-ui/src/components/chat/__tests__/ModelSelector.spec.tsx +++ b/webview-ui/src/components/chat/__tests__/ModelSelector.spec.tsx @@ -1,6 +1,6 @@ import { render, screen, fireEvent } from "@/utils/test-utils" -import { providerIdentifiers, type ProviderSettings } from "@roo-code/types" +import { anthropicDefaultModelId, providerIdentifiers, type ProviderSettings } from "@roo-code/types" import { ModelSelector } from "../ModelSelector" @@ -373,6 +373,12 @@ describe("ModelSelector", () => { }) }) + test("falls back to the shared default provider when the configuration has no provider", () => { + render() + + expect(screen.getByTestId("model-selector-trigger")).toHaveTextContent(anthropicDefaultModelId) + }) + test("falls back to a degraded trigger for providers without a simple model list, and jumps to Settings on click", () => { render( { vi.clearAllMocks() }) - const renderWithConfig = (apiConfiguration: ProviderSettings) => { + const renderWithConfig = (apiConfiguration: Partial) => { const onDone = vi.fn() renderWithExtensionState(, { - state: { currentApiConfigName: "test-config", apiConfiguration }, + state: { currentApiConfigName: "test-config", apiConfiguration: resolveProviderSettings(apiConfiguration) }, }) } diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx index 1bc4ab1afb..88609defaa 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.unsaved-changes.spec.tsx @@ -1,4 +1,4 @@ -import { providerIdentifiers, openAiModelInfoSaneDefaults, type ProviderSettings } from "@roo-code/types" +import { providerIdentifiers, openAiModelInfoSaneDefaults, type ResolvedProviderSettings } from "@roo-code/types" import { screen, fireEvent, waitFor } from "@testing-library/react" import { renderWithExtensionState } from "@/utils/test-utils" @@ -679,7 +679,7 @@ describe("SettingsView - Unsaved Changes Detection", () => { }) it("keeps OpenAI-compatible reasoning edits cached until Save despite a live state refresh", async () => { - const configuration: ProviderSettings = { + const configuration: ResolvedProviderSettings = { apiProvider: providerIdentifiers.openai, openAiModelId: "custom-model", enableReasoningEffort: false, 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 4955c3c031..8066d711f5 100644 --- a/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts +++ b/webview-ui/src/components/ui/hooks/__tests__/useSelectedModel.spec.ts @@ -13,6 +13,8 @@ import { type ModelRecord, type RouterModels, anthropicModels, + anthropicDefaultModelId, + DEFAULT_API_PROVIDER, BEDROCK_1M_CONTEXT_MODEL_IDS, BEDROCK_DEFAULT_CONTEXT, BEDROCK_MAX_TOKENS, @@ -599,7 +601,9 @@ describe("useSelectedModel", () => { }) describe("loading and error states", () => { - it("should set loading when router models are loading for the default OpenRouter provider", () => { + const openRouterConfiguration = { apiProvider: providerIdentifiers.openrouter } + + it("should set loading when router models are loading for OpenRouter", () => { mockUseRouterModels.mockReturnValue({ data: undefined, isLoading: true, @@ -613,12 +617,12 @@ describe("useSelectedModel", () => { } as any) const wrapper = createWrapper() - const { result } = renderHook(() => useSelectedModel({}), { wrapper }) + const { result } = renderHook(() => useSelectedModel(openRouterConfiguration), { wrapper }) expect(result.current.isLoading).toBe(true) }) - it("should set loading when OpenRouter provider metadata is loading for the default provider", () => { + it("should set loading when OpenRouter provider metadata is loading", () => { mockUseRouterModels.mockReturnValue({ data: { openrouter: {}, requesty: {}, litellm: {} }, isLoading: false, @@ -632,12 +636,12 @@ describe("useSelectedModel", () => { } as any) const wrapper = createWrapper() - const { result } = renderHook(() => useSelectedModel({}), { wrapper }) + const { result } = renderHook(() => useSelectedModel(openRouterConfiguration), { wrapper }) expect(result.current.isLoading).toBe(true) }) - it("should set error when router models error for the default OpenRouter provider", () => { + it("should set error when router models error for OpenRouter", () => { mockUseRouterModels.mockReturnValue({ data: undefined, isLoading: false, @@ -651,14 +655,14 @@ describe("useSelectedModel", () => { } as any) const wrapper = createWrapper() - const { result } = renderHook(() => useSelectedModel({}), { wrapper }) + const { result } = renderHook(() => useSelectedModel(openRouterConfiguration), { wrapper }) expect(result.current.isError).toBe(true) }) }) describe("default behavior", () => { - it("should return OpenRouter default when no configuration is provided", () => { + it("falls back to the shared default provider when no configuration is provided", () => { mockUseRouterModels.mockReturnValue({ data: undefined, isLoading: false, @@ -674,9 +678,9 @@ describe("useSelectedModel", () => { const wrapper = createWrapper() const { result } = renderHook(() => useSelectedModel({}), { wrapper }) - expect(result.current.provider).toBe(providerIdentifiers.openrouter) - expect(result.current.id).toBe(openRouterDefaultModelId) - expect(result.current.info).toBeUndefined() + expect(result.current.provider).toBe(DEFAULT_API_PROVIDER) + expect(result.current.id).toBe(anthropicDefaultModelId) + expect(result.current.info).toEqual(anthropicModels[anthropicDefaultModelId]) }) }) diff --git a/webview-ui/src/components/ui/hooks/useSelectedModel.ts b/webview-ui/src/components/ui/hooks/useSelectedModel.ts index 9eca0cc0f3..e0d9b6b9cd 100644 --- a/webview-ui/src/components/ui/hooks/useSelectedModel.ts +++ b/webview-ui/src/components/ui/hooks/useSelectedModel.ts @@ -43,6 +43,7 @@ import { providerIdentifiers, githubCopilotLanguageModel, vscodeLlmBaselineModelInfo, + DEFAULT_API_PROVIDER, } from "@roo-code/types" import { useRouterModels } from "./useRouterModels" @@ -97,7 +98,7 @@ function getStaticCatalogModel( * renamed deployment). */ export const useSelectedModel = (apiConfiguration: ProviderSettings) => { - const provider = apiConfiguration.apiProvider || providerIdentifiers.openrouter + const provider = apiConfiguration.apiProvider ?? DEFAULT_API_PROVIDER const activeProvider: ProviderName | undefined = isRetiredProvider(provider) ? undefined : provider const dynamicProvider = activeProvider && isDynamicProvider(activeProvider) ? activeProvider : undefined const openRouterModelId = diff --git a/webview-ui/src/components/welcome/WelcomeViewProvider.tsx b/webview-ui/src/components/welcome/WelcomeViewProvider.tsx index b038146fd9..81f57784f6 100644 --- a/webview-ui/src/components/welcome/WelcomeViewProvider.tsx +++ b/webview-ui/src/components/welcome/WelcomeViewProvider.tsx @@ -4,6 +4,8 @@ import { ArrowLeft, Brain } from "lucide-react" import { openRouterDefaultModelId, providerIdentifiers, type ProviderSettings } from "@roo-code/types" +import { checkExistKey } from "@roo/checkExistApiConfig" + import { useExtensionState } from "@src/context/ExtensionStateContext" import { validateApiConfiguration } from "@src/utils/validate" import { vscode } from "@src/utils/vscode" @@ -24,9 +26,7 @@ const getWelcomeApiConfiguration = ( apiConfiguration: ProviderSettings, zooCodeIsAuthenticated?: boolean, ): ProviderSettings => { - // validateApiConfiguration treats a missing apiProvider as valid (no switch case matches), - // so we explicitly fall back here before delegating to it for incomplete-but-set configs. - if (!apiConfiguration.apiProvider) { + if (!checkExistKey(apiConfiguration, zooCodeIsAuthenticated)) { return DEFAULT_WELCOME_API_CONFIGURATION } diff --git a/webview-ui/src/components/welcome/__tests__/WelcomeViewProvider.spec.tsx b/webview-ui/src/components/welcome/__tests__/WelcomeViewProvider.spec.tsx index c15c0608ec..e02d62de8c 100644 --- a/webview-ui/src/components/welcome/__tests__/WelcomeViewProvider.spec.tsx +++ b/webview-ui/src/components/welcome/__tests__/WelcomeViewProvider.spec.tsx @@ -1,4 +1,4 @@ -import { providerIdentifiers } from "@roo-code/types" +import { DEFAULT_API_PROVIDER, providerIdentifiers } from "@roo-code/types" // npx vitest src/components/welcome/__tests__/WelcomeViewProvider.spec.tsx import React from "react" @@ -170,6 +170,23 @@ describe("WelcomeViewProvider", () => { }) }) + it("still suggests OpenRouter for a resolved but keyless configuration", () => { + // The host now always resolves `apiProvider`, so "no provider" is no longer the + // signal for an unconfigured profile. A keyless default must still onboard. + const { setApiConfiguration } = renderWelcomeViewProvider({ + apiConfiguration: { apiProvider: DEFAULT_API_PROVIDER }, + }) + + fireEvent.click(screen.getByTestId("button-primary")) + + expect(screen.getByTestId("api-options")).toHaveAttribute("data-provider", "openrouter") + expect(screen.getByTestId("api-options")).toHaveAttribute("data-model", openRouterDefaultModelId) + expect(setApiConfiguration).toHaveBeenCalledWith({ + apiProvider: providerIdentifiers.openrouter, + openRouterModelId: openRouterDefaultModelId, + }) + }) + it("keeps the landing screen centered", () => { renderWelcomeViewProvider() diff --git a/webview-ui/src/context/ExtensionStateContext.tsx b/webview-ui/src/context/ExtensionStateContext.tsx index 41f94b5ff5..c96ad7dbb0 100644 --- a/webview-ui/src/context/ExtensionStateContext.tsx +++ b/webview-ui/src/context/ExtensionStateContext.tsx @@ -25,6 +25,8 @@ import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, DEFAULT_DIFF_FUZZY_THRESHOLD, DEFAULT_WRITE_DELAY_MS, + DEFAULT_API_PROVIDER, + resolveProviderSettings, } from "@roo-code/types" import { findLastIndex } from "@roo/array" @@ -66,7 +68,7 @@ export interface ExtensionStateContextType extends ExtensionState { marketplaceInstalledMetadata?: MarketplaceInstalledMetadata profileThresholds: Record setProfileThresholds: (value: Record) => void - setApiConfiguration: (config: ProviderSettings) => void + setApiConfiguration: (config: Partial) => void setCustomInstructions: (value?: string) => void setAlwaysAllowReadOnly: (value: boolean) => void setAlwaysAllowReadOnlyOutsideWorkspace: (value: boolean) => void @@ -232,7 +234,9 @@ export const mergeExtensionState = (prevState: ExtensionState, newState: Partial } export const createInitialExtensionState = (): ExtensionState => ({ - apiConfiguration: {}, + // Never rendered: App waits for `didHydrateState`, and the first state message + // replaces this object. It exists so the type contract holds from the start. + apiConfiguration: { apiProvider: DEFAULT_API_PROVIDER }, version: "", clineMessages: [], taskHistory: [], @@ -377,13 +381,13 @@ export const ExtensionStateContextProvider: React.FC<{ [], ) - const setApiConfiguration = useCallback((value: ProviderSettings) => { + const setApiConfiguration = useCallback((value: Partial) => { setState((prevState) => ({ ...prevState, - apiConfiguration: { + apiConfiguration: resolveProviderSettings({ ...prevState.apiConfiguration, ...value, - }, + }), })) }, []) diff --git a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx index 424255245a..99f36615f7 100644 --- a/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx +++ b/webview-ui/src/context/__tests__/ExtensionStateContext.spec.tsx @@ -3,7 +3,6 @@ import { render, screen, act } from "@/utils/test-utils" import React from "react" import { - type ProviderSettings, type ExperimentId, type ExtensionState, type ClineMessage, @@ -14,6 +13,7 @@ import { DEFAULT_CHECKPOINT_TIMEOUT_SECONDS, DEFAULT_DIFF_FUZZY_THRESHOLD, DEFAULT_WRITE_DELAY_MS, + DEFAULT_API_PROVIDER, } from "@roo-code/types" import { @@ -108,6 +108,9 @@ const ApiConfigTestComponent = () => { + ) } @@ -699,6 +702,32 @@ describe("ExtensionStateContext", () => { expect(state.writeDelayMs).toBe(DEFAULT_WRITE_DELAY_MS) }) + it("initializes apiConfiguration with the shared default provider", () => { + // The pre-hydration placeholder must satisfy the resolved-state contract so + // consumers never have to handle an undefined provider. + expect(createInitialExtensionState().apiConfiguration.apiProvider).toBe(DEFAULT_API_PROVIDER) + }) + + it("keeps a defined provider when setApiConfiguration receives an undefined provider", () => { + render( + + + , + ) + + act(() => { + screen.getByTestId("update-api-config-button").click() + }) + + act(() => { + screen.getByTestId("clear-provider-button").click() + }) + + const config = JSON.parse(screen.getByTestId("api-configuration").textContent || "{}") + + expect(config.apiProvider).toBe(DEFAULT_API_PROVIDER) + }) + it("initializes shadowed context fields from initialState", () => { const routerModels = {} as RouterModels const marketplaceItems: MarketplaceItem[] = [ @@ -902,7 +931,7 @@ describe("mergeExtensionState", () => { customModes: [], maxOpenTabsContext: 20, maxWorkspaceFiles: 100, - apiConfiguration: { providerId: providerIdentifiers.openrouter } as ProviderSettings, + apiConfiguration: { apiProvider: providerIdentifiers.openrouter }, telemetrySetting: "unset", showRooIgnoredFiles: true, enableSubfolderRules: false, @@ -926,14 +955,22 @@ describe("mergeExtensionState", () => { const prevState: ExtensionState = { ...baseState, - apiConfiguration: { modelMaxTokens: 1234, modelMaxThinkingTokens: 123 }, + apiConfiguration: { + apiProvider: providerIdentifiers.anthropic, + modelMaxTokens: 1234, + modelMaxThinkingTokens: 123, + }, experiments: {} as Record, checkpointTimeout: DEFAULT_CHECKPOINT_TIMEOUT_SECONDS - 5, } const newState: ExtensionState = { ...baseState, - apiConfiguration: { modelMaxThinkingTokens: 456, modelTemperature: 0.3 }, + apiConfiguration: { + apiProvider: providerIdentifiers.anthropic, + modelMaxThinkingTokens: 456, + modelTemperature: 0.3, + }, experiments: { preventFocusDisruption: false, imageGeneration: false, @@ -946,6 +983,7 @@ describe("mergeExtensionState", () => { const result = mergeExtensionState(prevState, newState) expect(result.apiConfiguration).toEqual({ + apiProvider: providerIdentifiers.anthropic, modelMaxThinkingTokens: 456, modelTemperature: 0.3, }) @@ -974,7 +1012,7 @@ describe("mergeExtensionState", () => { customModes: [], maxOpenTabsContext: 20, maxWorkspaceFiles: 100, - apiConfiguration: {}, + apiConfiguration: { apiProvider: DEFAULT_API_PROVIDER }, telemetrySetting: "unset", showRooIgnoredFiles: true, enableSubfolderRules: false, diff --git a/webview-ui/src/utils/validate.ts b/webview-ui/src/utils/validate.ts index 9e4addf7b1..d2525c3869 100644 --- a/webview-ui/src/utils/validate.ts +++ b/webview-ui/src/utils/validate.ts @@ -9,6 +9,7 @@ import { parseOpenAiExtraBody, providerIdentifiers, githubCopilotLanguageModel, + DEFAULT_API_PROVIDER, } from "@roo-code/types" type OpenAiExtraBodyParseResult = ReturnType @@ -292,7 +293,7 @@ function validateDynamicProviderModelId( apiConfiguration: ProviderSettings, routerModels?: RouterModels, ): string | undefined { - const provider = apiConfiguration.apiProvider ?? "" + const provider = apiConfiguration.apiProvider ?? DEFAULT_API_PROVIDER // We only validate model ids from dynamic providers. if (!isDynamicProvider(provider)) {