From 86401faa37a56c0b6de706c862823601b39350f7 Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Wed, 5 Mar 2025 10:04:48 -0500 Subject: [PATCH] Better encapsulation for API config --- src/core/__tests__/contextProxy.test.ts | 86 +++++++++++++++++++ src/core/contextProxy.ts | 62 ++++++++++++- src/core/webview/ClineProvider.ts | 72 ++++------------ .../webview/__tests__/ClineProvider.test.ts | 61 +++++++++++++ 4 files changed, 222 insertions(+), 59 deletions(-) diff --git a/src/core/__tests__/contextProxy.test.ts b/src/core/__tests__/contextProxy.test.ts index 9f0c20b0c4..ef0c4333e0 100644 --- a/src/core/__tests__/contextProxy.test.ts +++ b/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()) + }) + }) }) }) diff --git a/src/core/contextProxy.ts b/src/core/contextProxy.ts index 7c429c86cf..8d3f9a4b7c 100644 --- a/src/core/contextProxy.ts +++ b/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 { // 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 { + const promises: Array> = [] + + // 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) + } } diff --git a/src/core/webview/ClineProvider.ts b/src/core/webview/ClineProvider.ts index a09f18d278..121505672d 100644 --- a/src/core/webview/ClineProvider.ts +++ b/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[] = [] - - // 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 = {} as Record - const secretValues: Record = {} as Record + // Get ApiConfiguration directly from contextProxy + const apiConfiguration = this.contextProxy.getApiConfiguration() - // 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 an object to store all fetched values (excluding API config which we already have) + const stateValues: Record = {} as Record + + // 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, diff --git a/src/core/webview/__tests__/ClineProvider.test.ts b/src/core/webview/__tests__/ClineProvider.test.ts index 3ef024afb3..ed557c8838 100644 --- a/src/core/webview/__tests__/ClineProvider.test.ts +++ b/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) }) })