Merge pull request #1394 from RooVetGit/fix_context_proxy

Revert "Better encapsulation for API config"

Author: mrubens

src/core/__tests__/contextProxy.test.ts

@@ -2,20 +2,11 @@ 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
@@ -162,82 +153,5 @@ 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,7 +1,6 @@
 import * as vscode from "vscode"
 import { logger } from "../utils/logging"
-import { ApiConfiguration, API_CONFIG_KEYS } from "../shared/api"
-import { GLOBAL_STATE_KEYS, SECRET_KEYS, GlobalStateKey, SecretKey } from "../shared/globalState"
+import { GLOBAL_STATE_KEYS, SECRET_KEYS } from "../shared/globalState"
 
 export class ContextProxy {
 	private readonly originalContext: vscode.ExtensionContext
@@ -83,6 +82,7 @@ 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,62 +93,4 @@ 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,8 +1659,20 @@ export class ClineProvider implements vscode.WebviewViewProvider {
 			}
 		}
 
-		// Update all configuration values through the contextProxy
-		await this.contextProxy.updateApiConfiguration(apiConfiguration)
+		// 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)
 
 		if (this.cline) {
 			this.cline.api = buildApiHandler(apiConfiguration)
@@ -2061,32 +2073,62 @@ export class ClineProvider implements vscode.WebviewViewProvider {
 	*/
 
 	async getState() {
-		// 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 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>
 
-		// 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))
+		// 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))
 
 		// Add promise for custom modes which is handled separately
 		const customModesPromise = this.customModesManager.getCustomModes()
 
 		let idx = 0
-		const valuePromises = await Promise.all([...statePromises, customModesPromise])
+		const valuePromises = await Promise.all([...statePromises, ...secretPromises, customModesPromise])
 
-		// Populate stateValues
-		GLOBAL_STATE_KEYS.filter((key) => !API_CONFIG_KEYS.includes(key)).forEach((key) => {
+		// Populate stateValues and secretValues
+		GLOBAL_STATE_KEYS.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,21 +34,6 @@ 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),
@@ -1594,51 +1579,5 @@ 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)
 	})
 })