Improve provider profile management in the external API (#3386)

Co-authored-by: John Richmond <[email protected]>

Author: 2 people

src/core/webview/ClineProvider.ts

@@ -9,7 +9,7 @@ import axios from "axios"
 import pWaitFor from "p-wait-for"
 import * as vscode from "vscode"
 
-import type { GlobalState, ProviderName, ProviderSettings, RooCodeSettings } from "../../schemas"
+import type { GlobalState, ProviderName, ProviderSettings, RooCodeSettings, ProviderSettingsEntry } from "../../schemas"
 import { t } from "../../i18n"
 import { setPanel } from "../../activate/registerCommands"
 import { requestyDefaultModelId, openRouterDefaultModelId, glamaDefaultModelId } from "../../shared/api"
@@ -246,7 +246,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 			return false
 		}
 
-		// check if there is a cline instance in the stack (if this provider has an active task)
+		// Check if there is a cline instance in the stack (if this provider has an active task)
 		if (visibleProvider.getCurrentCline()) {
 			return true
 		}
@@ -795,39 +795,127 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 		await this.postStateToWebview()
 	}
 
+	// Provider Profile Management
+
+	getProviderProfileEntries(): ProviderSettingsEntry[] {
+		return this.contextProxy.getValues().listApiConfigMeta || []
+	}
+
+	getProviderProfileEntry(name: string): ProviderSettingsEntry | undefined {
+		return this.getProviderProfileEntries().find((profile) => profile.name === name)
+	}
+
+	public hasProviderProfileEntry(name: string): boolean {
+		return !!this.getProviderProfileEntry(name)
+	}
+
+	async upsertProviderProfile(
+		name: string,
+		providerSettings: ProviderSettings,
+		activate: boolean = true,
+	): Promise<string | undefined> {
+		try {
+			// TODO: Do we need to be calling `activateProfile`? It's not
+			// clear to me what the source of truth should be; in some cases
+			// we rely on the `ContextProxy`'s data store and in other cases
+			// we rely on the `ProviderSettingsManager`'s data store. It might
+			// be simpler to unify these two.
+			const id = await this.providerSettingsManager.saveConfig(name, providerSettings)
+
+			if (activate) {
+				const { mode } = await this.getState()
+
+				// These promises do the following:
+				// 1. Adds or updates the list of provider profiles.
+				// 2. Sets the current provider profile.
+				// 3. Sets the current mode's provider profile.
+				// 4. Copies the provider settings to the context.
+				//
+				// Note: 1, 2, and 4 can be done in one `ContextProxy` call:
+				// this.contextProxy.setValues({ ...providerSettings, listApiConfigMeta: ..., currentApiConfigName: ... })
+				// We should probably switch to that and verify that it works.
+				// I left the original implementation in just to be safe.
+				await Promise.all([
+					this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig()),
+					this.updateGlobalState("currentApiConfigName", name),
+					this.providerSettingsManager.setModeConfig(mode, id),
+					this.contextProxy.setProviderSettings(providerSettings),
+				])
+
+				// Change the provider for the current task.
+				// TODO: We should rename `buildApiHandler` for clarity (e.g. `getProviderClient`).
+				const task = this.getCurrentCline()
+
+				if (task) {
+					task.api = buildApiHandler(providerSettings)
+				}
+			} else {
+				await this.updateGlobalState("listApiConfigMeta", await this.providerSettingsManager.listConfig())
+			}
+
+			await this.postStateToWebview()
+			return id
+		} catch (error) {
+			this.log(
+				`Error create new api configuration: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,
+			)
+
+			vscode.window.showErrorMessage(t("common:errors.create_api_config"))
+			return undefined
+		}
+	}
+
+	async deleteProviderProfile(profileToDelete: ProviderSettingsEntry) {
+		const globalSettings = this.contextProxy.getValues()
+		let profileToActivate: string | undefined = globalSettings.currentApiConfigName
+
+		if (profileToDelete.name === profileToActivate) {
+			profileToActivate = this.getProviderProfileEntries().find(({ name }) => name !== profileToDelete.name)?.name
+		}
+
+		if (!profileToActivate) {
+			throw new Error("You cannot delete the last profile")
+		}
+
+		const entries = this.getProviderProfileEntries().filter(({ name }) => name !== profileToDelete.name)
+
+		await this.contextProxy.setValues({
+			...globalSettings,
+			currentApiConfigName: profileToActivate,
+			listApiConfigMeta: entries,
+		})
+
+		await this.postStateToWebview()
+	}
+
 	async activateProviderProfile(args: { name: string } | { id: string }) {
-		const { name, ...providerSettings } = await this.providerSettingsManager.activateProfile(args)
+		const { name, id, ...providerSettings } = await this.providerSettingsManager.activateProfile(args)
 
+		// See `upsertProviderProfile` for a description of what this is doing.
 		await Promise.all([
 			this.contextProxy.setValue("listApiConfigMeta", await this.providerSettingsManager.listConfig()),
 			this.contextProxy.setValue("currentApiConfigName", name),
+			this.contextProxy.setProviderSettings(providerSettings),
 		])
 
-		await this.updateApiConfiguration(providerSettings)
-		await this.postStateToWebview()
-	}
-
-	async updateApiConfiguration(providerSettings: ProviderSettings) {
-		// Update mode's default config.
 		const { mode } = await this.getState()
 
-		if (mode) {
-			const currentApiConfigName = this.getGlobalState("currentApiConfigName")
-			const listApiConfig = await this.providerSettingsManager.listConfig()
-			const config = listApiConfig?.find((c) => c.name === currentApiConfigName)
-
-			if (config?.id) {
-				await this.providerSettingsManager.setModeConfig(mode, config.id)
-			}
+		if (id) {
+			await this.providerSettingsManager.setModeConfig(mode, id)
 		}
 
-		await this.contextProxy.setProviderSettings(providerSettings)
+		// Change the provider for the current task.
+		const task = this.getCurrentCline()
 
-		if (this.getCurrentCline()) {
-			this.getCurrentCline()!.api = buildApiHandler(providerSettings)
+		if (task) {
+			task.api = buildApiHandler(providerSettings)
 		}
+
+		await this.postStateToWebview()
 	}
 
+	// Task Management
+
 	async cancelTask() {
 		const cline = this.getCurrentCline()
 
@@ -943,7 +1031,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 			openRouterModelId: apiConfiguration?.openRouterModelId || openRouterDefaultModelId,
 		}
 
-		await this.upsertApiConfiguration(currentApiConfigName, newConfiguration)
+		await this.upsertProviderProfile(currentApiConfigName, newConfiguration)
 	}
 
 	// Glama
@@ -973,7 +1061,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 			glamaModelId: apiConfiguration?.glamaModelId || glamaDefaultModelId,
 		}
 
-		await this.upsertApiConfiguration(currentApiConfigName, newConfiguration)
+		await this.upsertProviderProfile(currentApiConfigName, newConfiguration)
 	}
 
 	// Requesty
@@ -988,29 +1076,7 @@ export class ClineProvider extends EventEmitter<ClineProviderEvents> implements
 			requestyModelId: apiConfiguration?.requestyModelId || requestyDefaultModelId,
 		}
 
-		await this.upsertApiConfiguration(currentApiConfigName, newConfiguration)
-	}
-
-	// Save configuration
-
-	async upsertApiConfiguration(configName: string, apiConfiguration: ProviderSettings) {
-		try {
-			await this.providerSettingsManager.saveConfig(configName, apiConfiguration)
-			const listApiConfig = await this.providerSettingsManager.listConfig()
-
-			await Promise.all([
-				this.updateGlobalState("listApiConfigMeta", listApiConfig),
-				this.updateApiConfiguration(apiConfiguration),
-				this.updateGlobalState("currentApiConfigName", configName),
-			])
-
-			await this.postStateToWebview()
-		} catch (error) {
-			this.log(
-				`Error create new api configuration: ${JSON.stringify(error, Object.getOwnPropertyNames(error), 2)}`,
-			)
-			vscode.window.showErrorMessage(t("common:errors.create_api_config"))
-		}
+		await this.upsertProviderProfile(currentApiConfigName, newConfiguration)
 	}
 
 	// Task history

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

@@ -612,7 +612,7 @@ describe("ClineProvider", () => {
 		expect(mockContext.globalState.update).toHaveBeenCalledWith("currentApiConfigName", "test-config")
 	})
 
-	test("saves current config when switching to mode without config", async () => {
+	it("saves current config when switching to mode without config", async () => {
 		await provider.resolveWebviewView(mockWebviewView)
 		const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as jest.Mock).mock.calls[0][0]
 
@@ -633,15 +633,15 @@ describe("ClineProvider", () => {
 		expect(provider.providerSettingsManager.setModeConfig).toHaveBeenCalledWith("architect", "current-id")
 	})
 
-	test("saves config as default for current mode when loading config", async () => {
+	it("saves config as default for current mode when loading config", async () => {
 		await provider.resolveWebviewView(mockWebviewView)
 		const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as jest.Mock).mock.calls[0][0]
 
+		const profile: ProviderSettingsEntry = { apiProvider: "anthropic", id: "new-id", name: "new-config" }
+
 		;(provider as any).providerSettingsManager = {
-			activateProfile: jest
-				.fn()
-				.mockResolvedValue({ config: { apiProvider: "anthropic", id: "new-id" }, name: "new-config" }),
-			listConfig: jest.fn().mockResolvedValue([{ name: "new-config", id: "new-id", apiProvider: "anthropic" }]),
+			activateProfile: jest.fn().mockResolvedValue(profile),
+			listConfig: jest.fn().mockResolvedValue([profile]),
 			setModeConfig: jest.fn(),
 			getModeConfigId: jest.fn().mockResolvedValue(undefined),
 		} as any
@@ -656,18 +656,19 @@ describe("ClineProvider", () => {
 		expect(provider.providerSettingsManager.setModeConfig).toHaveBeenCalledWith("architect", "new-id")
 	})
 
-	test("load API configuration by ID works and updates mode config", async () => {
+	it("load API configuration by ID works and updates mode config", async () => {
 		await provider.resolveWebviewView(mockWebviewView)
 		const messageHandler = (mockWebviewView.webview.onDidReceiveMessage as jest.Mock).mock.calls[0][0]
 
+		const profile: ProviderSettingsEntry = {
+			name: "config-by-id",
+			id: "config-id-123",
+			apiProvider: "anthropic",
+		}
+
 		;(provider as any).providerSettingsManager = {
-			activateProfile: jest.fn().mockResolvedValue({
-				config: { apiProvider: "anthropic", id: "config-id-123" },
-				name: "config-by-id",
-			}),
-			listConfig: jest
-				.fn()
-				.mockResolvedValue([{ name: "config-by-id", id: "config-id-123", apiProvider: "anthropic" }]),
+			activateProfile: jest.fn().mockResolvedValue(profile),
+			listConfig: jest.fn().mockResolvedValue([profile]),
 			setModeConfig: jest.fn(),
 			getModeConfigId: jest.fn().mockResolvedValue(undefined),
 		} as any
@@ -881,7 +882,7 @@ describe("ClineProvider", () => {
 		})
 	})
 
-	test("saves mode config when updating API configuration", async () => {
+	it("saves mode config when updating API configuration", async () => {
 		// Setup mock context with mode and config name
 		mockContext = {
 			...mockContext,
@@ -907,12 +908,14 @@ describe("ClineProvider", () => {
 
 		;(provider as any).providerSettingsManager = {
 			listConfig: jest.fn().mockResolvedValue([{ name: "test-config", id: "test-id", apiProvider: "anthropic" }]),
+			saveConfig: jest.fn().mockResolvedValue("test-id"),
 			setModeConfig: jest.fn(),
 		} as any
 
 		// Update API configuration
 		await messageHandler({
-			type: "apiConfiguration",
+			type: "upsertApiConfiguration",
+			text: "test-config",
 			apiConfiguration: { apiProvider: "anthropic" },
 		})
 
@@ -1675,14 +1678,11 @@ describe("ClineProvider", () => {
 				currentApiConfigName: "test-config",
 			} as any)
 
-			// Trigger updateApiConfiguration
+			// Trigger upsertApiConfiguration
 			await messageHandler({
 				type: "upsertApiConfiguration",
 				text: "test-config",
-				apiConfiguration: {
-					apiProvider: "anthropic",
-					apiKey: "test-key",
-				},
+				apiConfiguration: { apiProvider: "anthropic", apiKey: "test-key" },
 			})
 
 			// Verify error was logged and user was notified

src/core/webview/webviewMessageHandler.ts

@@ -126,12 +126,6 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We
 			// task. This essentially creates a fresh slate for the new task.
 			await provider.initClineWithTask(message.text, message.images)
 			break
-		case "apiConfiguration":
-			if (message.apiConfiguration) {
-				await provider.updateApiConfiguration(message.apiConfiguration)
-			}
-			await provider.postStateToWebview()
-			break
 		case "customInstructions":
 			await provider.updateCustomInstructions(message.text)
 			break
@@ -1068,7 +1062,7 @@ export const webviewMessageHandler = async (provider: ClineProvider, message: We
 			break
 		case "upsertApiConfiguration":
 			if (message.text && message.apiConfiguration) {
-				await provider.upsertApiConfiguration(message.text, message.apiConfiguration)
+				await provider.upsertProviderProfile(message.text, message.apiConfiguration)
 			}
 			break
 		case "renameApiConfiguration":