Better encapsulation for API config

Author: mrubens

src/core/__tests__/contextProxy.test.ts

@@ -2,11 +2,20 @@ import * as vscode from "vscode"
 import { ContextProxy } from "../contextProxy"
 import { logger } from "../../utils/logging"
 import { GLOBAL_STATE_KEYS, SECRET_KEYS } from "../../shared/globalState"
+import { ApiConfiguration } from "../../shared/api"
 
 // Mock shared/globalState
 jest.mock("../../shared/globalState", () => ({
 	GLOBAL_STATE_KEYS: ["apiProvider", "apiModelId", "mode"],
 	SECRET_KEYS: ["apiKey", "openAiApiKey"],
+	GlobalStateKey: {},
+	SecretKey: {},
+}))
+
+// Mock shared/api
+jest.mock("../../shared/api", () => ({
+	API_CONFIG_KEYS: ["apiProvider", "apiModelId"],
+	ApiConfiguration: {},
 }))
 
 // Mock VSCode API
@@ -153,5 +162,82 @@ describe("ContextProxy", () => {
 			const storedValue = await proxy.getSecret("api-key")
 			expect(storedValue).toBeUndefined()
 		})
+
+		describe("getApiConfiguration", () => {
+			it("should combine global state and secrets into a single ApiConfiguration object", async () => {
+				// Mock data in state cache
+				await proxy.updateGlobalState("apiProvider", "anthropic")
+				await proxy.updateGlobalState("apiModelId", "test-model")
+				// Mock data in secrets cache
+				await proxy.storeSecret("apiKey", "test-api-key")
+
+				const config = proxy.getApiConfiguration()
+
+				// Should contain values from global state
+				expect(config.apiProvider).toBe("anthropic")
+				expect(config.apiModelId).toBe("test-model")
+				// Should contain values from secrets
+				expect(config.apiKey).toBe("test-api-key")
+			})
+
+			it("should handle special case for apiProvider defaulting", async () => {
+				// Clear apiProvider but set apiKey
+				await proxy.updateGlobalState("apiProvider", undefined)
+				await proxy.storeSecret("apiKey", "test-api-key")
+
+				const config = proxy.getApiConfiguration()
+
+				// Should default to anthropic when apiKey exists
+				expect(config.apiProvider).toBe("anthropic")
+
+				// Clear both apiProvider and apiKey
+				await proxy.updateGlobalState("apiProvider", undefined)
+				await proxy.storeSecret("apiKey", undefined)
+
+				const configWithoutKey = proxy.getApiConfiguration()
+
+				// Should default to openrouter when no apiKey exists
+				expect(configWithoutKey.apiProvider).toBe("openrouter")
+			})
+		})
+
+		describe("updateApiConfiguration", () => {
+			it("should update both global state and secrets", async () => {
+				const apiConfig: ApiConfiguration = {
+					apiProvider: "anthropic",
+					apiModelId: "claude-latest",
+					apiKey: "test-api-key",
+				}
+
+				await proxy.updateApiConfiguration(apiConfig)
+
+				// Should update global state
+				expect(mockGlobalState.update).toHaveBeenCalledWith("apiProvider", "anthropic")
+				expect(mockGlobalState.update).toHaveBeenCalledWith("apiModelId", "claude-latest")
+				// Should update secrets
+				expect(mockSecrets.store).toHaveBeenCalledWith("apiKey", "test-api-key")
+
+				// Check that values are in cache
+				expect(proxy.getGlobalState("apiProvider")).toBe("anthropic")
+				expect(proxy.getGlobalState("apiModelId")).toBe("claude-latest")
+				expect(proxy.getSecret("apiKey")).toBe("test-api-key")
+			})
+
+			it("should ignore keys that aren't in either GLOBAL_STATE_KEYS or SECRET_KEYS", async () => {
+				// Use type assertion to add an invalid key
+				const apiConfig = {
+					apiProvider: "anthropic",
+					invalidKey: "should be ignored",
+				} as ApiConfiguration & { invalidKey: string }
+
+				await proxy.updateApiConfiguration(apiConfig)
+
+				// Should update keys in GLOBAL_STATE_KEYS
+				expect(mockGlobalState.update).toHaveBeenCalledWith("apiProvider", "anthropic")
+				// Should not call update/store for invalid keys
+				expect(mockGlobalState.update).not.toHaveBeenCalledWith("invalidKey", expect.anything())
+				expect(mockSecrets.store).not.toHaveBeenCalledWith("invalidKey", expect.anything())
+			})
+		})
 	})
 })

src/core/contextProxy.ts

@@ -1,6 +1,7 @@
 import * as vscode from "vscode"
 import { logger } from "../utils/logging"
-import { GLOBAL_STATE_KEYS, SECRET_KEYS } from "../shared/globalState"
+import { ApiConfiguration, API_CONFIG_KEYS } from "../shared/api"
+import { GLOBAL_STATE_KEYS, SECRET_KEYS, GlobalStateKey, SecretKey } from "../shared/globalState"
 
 export class ContextProxy {
 	private readonly originalContext: vscode.ExtensionContext
@@ -82,7 +83,6 @@ export class ContextProxy {
 	getSecret(key: string): string | undefined {
 		return this.secretCache.get(key)
 	}
-
 	storeSecret(key: string, value?: string): Thenable<void> {
 		// Update cache
 		this.secretCache.set(key, value)
@@ -93,4 +93,62 @@ export class ContextProxy {
 			return this.originalContext.secrets.store(key, value)
 		}
 	}
+
+	/**
+	 * Gets a complete ApiConfiguration object by fetching values
+	 * from both global state and secrets storage
+	 */
+	getApiConfiguration(): ApiConfiguration {
+		// Create an empty ApiConfiguration object
+		const config: ApiConfiguration = {}
+
+		// Add all API-related keys from global state
+		for (const key of API_CONFIG_KEYS) {
+			const value = this.getGlobalState(key)
+			if (value !== undefined) {
+				// Use type assertion to avoid TypeScript error
+				;(config as any)[key] = value
+			}
+		}
+
+		// Add all secret values
+		for (const key of SECRET_KEYS) {
+			const value = this.getSecret(key)
+			if (value !== undefined) {
+				// Use type assertion to avoid TypeScript error
+				;(config as any)[key] = value
+			}
+		}
+
+		// Handle special case for apiProvider if needed (same logic as current implementation)
+		if (!config.apiProvider) {
+			if (config.apiKey) {
+				config.apiProvider = "anthropic"
+			} else {
+				config.apiProvider = "openrouter"
+			}
+		}
+
+		return config
+	}
+
+	/**
+	 * Updates an ApiConfiguration by persisting each property
+	 * to the appropriate storage (global state or secrets)
+	 */
+	async updateApiConfiguration(apiConfiguration: ApiConfiguration): Promise<void> {
+		const promises: Array<Thenable<void>> = []
+
+		// For each property, update the appropriate storage
+		Object.entries(apiConfiguration).forEach(([key, value]) => {
+			if (SECRET_KEYS.includes(key as SecretKey)) {
+				promises.push(this.storeSecret(key, value))
+			} else if (API_CONFIG_KEYS.includes(key as GlobalStateKey)) {
+				promises.push(this.updateGlobalState(key, value))
+			}
+			// Ignore keys that aren't in either list
+		})
+
+		await Promise.all(promises)
+	}
 }

src/core/webview/ClineProvider.ts

@@ -1659,20 +1659,8 @@ export class ClineProvider implements vscode.WebviewViewProvider {
 			}
 		}
 
-		// Create an array of promises to update state
-		const promises: Promise<any>[] = []
-
-		// For each property in apiConfiguration, update the appropriate state
-		Object.entries(apiConfiguration).forEach(([key, value]) => {
-			// Check if this key is a secret
-			if (SECRET_KEYS.includes(key as SecretKey)) {
-				promises.push(this.storeSecret(key as SecretKey, value))
-			} else {
-				promises.push(this.updateGlobalState(key as GlobalStateKey, value))
-			}
-		})
-
-		await Promise.all(promises)
+		// Update all configuration values through the contextProxy
+		await this.contextProxy.updateApiConfiguration(apiConfiguration)
 
 		if (this.cline) {
 			this.cline.api = buildApiHandler(apiConfiguration)
@@ -2073,62 +2061,32 @@ export class ClineProvider implements vscode.WebviewViewProvider {
 	*/
 
 	async getState() {
-		// Create an object to store all fetched values
-		const stateValues: Record<GlobalStateKey | SecretKey, any> = {} as Record<GlobalStateKey | SecretKey, any>
-		const secretValues: Record<SecretKey, any> = {} as Record<SecretKey, any>
+		// Get ApiConfiguration directly from contextProxy
+		const apiConfiguration = this.contextProxy.getApiConfiguration()
+
+		// Create an object to store all fetched values (excluding API config which we already have)
+		const stateValues: Record<GlobalStateKey, any> = {} as Record<GlobalStateKey, any>
 
-		// Create promise arrays for global state and secrets
-		const statePromises = GLOBAL_STATE_KEYS.map((key) => this.getGlobalState(key))
-		const secretPromises = SECRET_KEYS.map((key) => this.getSecret(key))
+		// Create promise arrays for global state
+		const statePromises = GLOBAL_STATE_KEYS
+			// Filter out API config keys since we already have them
+			.filter((key) => !API_CONFIG_KEYS.includes(key))
+			.map((key) => this.getGlobalState(key))
 
 		// Add promise for custom modes which is handled separately
 		const customModesPromise = this.customModesManager.getCustomModes()
 
 		let idx = 0
-		const valuePromises = await Promise.all([...statePromises, ...secretPromises, customModesPromise])
+		const valuePromises = await Promise.all([...statePromises, customModesPromise])
 
-		// Populate stateValues and secretValues
-		GLOBAL_STATE_KEYS.forEach((key, _) => {
+		// Populate stateValues
+		GLOBAL_STATE_KEYS.filter((key) => !API_CONFIG_KEYS.includes(key)).forEach((key) => {
 			stateValues[key] = valuePromises[idx]
 			idx = idx + 1
 		})
 
-		SECRET_KEYS.forEach((key, index) => {
-			secretValues[key] = valuePromises[idx]
-			idx = idx + 1
-		})
-
 		let customModes = valuePromises[idx] as ModeConfig[] | undefined
 
-		// Determine apiProvider with the same logic as before
-		let apiProvider: ApiProvider
-		if (stateValues.apiProvider) {
-			apiProvider = stateValues.apiProvider
-		} else {
-			// Either new user or legacy user that doesn't have the apiProvider stored in state
-			// (If they're using OpenRouter or Bedrock, then apiProvider state will exist)
-			if (secretValues.apiKey) {
-				apiProvider = "anthropic"
-			} else {
-				// New users should default to openrouter
-				apiProvider = "openrouter"
-			}
-		}
-
-		// Build the apiConfiguration object combining state values and secrets
-		// Using the dynamic approach with API_CONFIG_KEYS
-		const apiConfiguration: ApiConfiguration = {
-			// Dynamically add all API-related keys from stateValues
-			...Object.fromEntries(API_CONFIG_KEYS.map((key) => [key, stateValues[key]])),
-			// Add all secrets
-			...secretValues,
-		}
-
-		// Ensure apiProvider is set properly if not already in state
-		if (!apiConfiguration.apiProvider) {
-			apiConfiguration.apiProvider = apiProvider
-		}
-
 		// Return the same structure as before
 		return {
 			apiConfiguration,

src/core/webview/__tests__/ClineProvider.test.ts

@@ -34,6 +34,21 @@ jest.mock("../../contextProxy", () => {
 				.mockImplementation((key, value) =>
 					value ? context.secrets.store(key, value) : context.secrets.delete(key),
 				),
+			getApiConfiguration: jest.fn().mockImplementation(() => ({
+				apiProvider: "openrouter",
+				// Add other common properties
+			})),
+			updateApiConfiguration: jest.fn().mockImplementation(async (apiConfiguration) => {
+				// Mock implementation that simulates updating state and secrets
+				for (const [key, value] of Object.entries(apiConfiguration)) {
+					if (key === "apiKey" || key === "openAiApiKey") {
+						context.secrets.store(key, value)
+					} else {
+						context.globalState.update(key, value)
+					}
+				}
+				return Promise.resolve()
+			}),
 			saveChanges: jest.fn().mockResolvedValue(undefined),
 			dispose: jest.fn().mockResolvedValue(undefined),
 			hasPendingChanges: jest.fn().mockReturnValue(false),
@@ -1579,5 +1594,51 @@ describe("ContextProxy integration", () => {
 		expect(mockContextProxy.getGlobalState).toBeDefined()
 		expect(mockContextProxy.updateGlobalState).toBeDefined()
 		expect(mockContextProxy.storeSecret).toBeDefined()
+		expect(mockContextProxy.getApiConfiguration).toBeDefined()
+		expect(mockContextProxy.updateApiConfiguration).toBeDefined()
+	})
+
+	test("getState uses contextProxy.getApiConfiguration", async () => {
+		// Setup mock API configuration
+		const mockApiConfig = {
+			apiProvider: "anthropic",
+			apiModelId: "claude-latest",
+			apiKey: "test-api-key",
+		}
+		mockContextProxy.getApiConfiguration.mockReturnValue(mockApiConfig)
+
+		// Get state
+		const state = await provider.getState()
+
+		// Verify getApiConfiguration was called
+		expect(mockContextProxy.getApiConfiguration).toHaveBeenCalled()
+		// Verify state has the API configuration from contextProxy
+		expect(state.apiConfiguration).toBe(mockApiConfig)
+	})
+
+	test("updateApiConfiguration uses contextProxy.updateApiConfiguration", async () => {
+		// Setup test config
+		const testApiConfig = {
+			apiProvider: "anthropic",
+			apiModelId: "claude-latest",
+			apiKey: "test-api-key",
+		}
+
+		// Mock methods needed for the test
+		provider.configManager = {
+			listConfig: jest.fn().mockResolvedValue([]),
+			setModeConfig: jest.fn(),
+		} as any
+
+		// Mock getState for mode
+		jest.spyOn(provider, "getState").mockResolvedValue({
+			mode: "code",
+		} as any)
+
+		// Call the private method - need to use any to access it
+		await (provider as any).updateApiConfiguration(testApiConfig)
+
+		// Verify contextProxy.updateApiConfiguration was called with the right config
+		expect(mockContextProxy.updateApiConfiguration).toHaveBeenCalledWith(testApiConfig)
 	})
 })