From f683e4530f8a55c7c9a8a2a8903cbcf9a4c1c87c Mon Sep 17 00:00:00 2001 From: Matt Rubens Date: Wed, 5 Mar 2025 11:05:05 -0500 Subject: [PATCH] Revert "Better encapsulation for API config" This reverts commit 86401faa37a56c0b6de706c862823601b39350f7. --- 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, 59 insertions(+), 222 deletions(-) diff --git a/src/core/__tests__/contextProxy.test.ts b/src/core/__tests__/contextProxy.test.ts index ef0c4333e0..9f0c20b0c4 100644 --- a/src/core/__tests__/contextProxy.test.ts +++ b/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()) - }) - }) }) }) diff --git a/src/core/contextProxy.ts b/src/core/contextProxy.ts index 8d3f9a4b7c..7c429c86cf 100644 --- a/src/core/contextProxy.ts +++ b/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 { // 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 { - 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 121505672d..a09f18d278 100644 --- a/src/core/webview/ClineProvider.ts +++ b/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[] = [] + + // 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 + const stateValues: Record = {} as Record + const secretValues: Record = {} as Record - // 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)) + // 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, diff --git a/src/core/webview/__tests__/ClineProvider.test.ts b/src/core/webview/__tests__/ClineProvider.test.ts index ed557c8838..3ef024afb3 100644 --- a/src/core/webview/__tests__/ClineProvider.test.ts +++ b/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) }) })