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

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

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":

src/exports/api.ts

@@ -6,7 +6,14 @@ import * as path from "path"
 import { getWorkspacePath } from "../utils/path"
 import { ClineProvider } from "../core/webview/ClineProvider"
 import { openClineInNewTab } from "../activate/registerCommands"
-import { RooCodeSettings, RooCodeEvents, RooCodeEventName } from "../schemas"
+import {
+	RooCodeSettings,
+	RooCodeEvents,
+	RooCodeEventName,
+	ProviderSettings,
+	ProviderSettingsEntry,
+	isSecretStateKey,
+} from "../schemas"
 import { IpcOrigin, IpcMessageType, TaskCommandName, TaskEvent } from "../schemas/ipc"
 
 import { RooCodeAPI } from "./interface"
@@ -246,8 +253,10 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI {
 
 	// Global Settings Management
 
-	public getConfiguration() {
-		return this.sidebarProvider.getValues()
+	public getConfiguration(): RooCodeSettings {
+		return Object.fromEntries(
+			Object.entries(this.sidebarProvider.getValues()).filter(([key]) => !isSecretStateKey(key)),
+		)
 	}
 
 	public async setConfiguration(values: RooCodeSettings) {
@@ -258,77 +267,86 @@ export class API extends EventEmitter<RooCodeEvents> implements RooCodeAPI {
 
 	// Provider Profile Management
 
-	private getProfilesMeta() {
-		return this.getConfiguration().listApiConfigMeta || []
+	public getProfiles(): string[] {
+		return this.sidebarProvider.getProviderProfileEntries().map(({ name }) => name)
 	}
 
-	public getProfiles() {
-		return this.getProfilesMeta().map((profile) => profile.name)
+	public getProfileEntry(name: string): ProviderSettingsEntry | undefined {
+		return this.sidebarProvider.getProviderProfileEntry(name)
 	}
 
-	public hasProfile(name: string): boolean {
-		return !!(this.getConfiguration().listApiConfigMeta || []).find((profile) => profile.name === name)
-	}
+	public async createProfile(name: string, profile?: ProviderSettings, activate: boolean = true) {
+		const entry = this.getProfileEntry(name)
 
-	public async createProfile(name: string) {
-		if (!name || !name.trim()) {
-			throw new Error("Profile name cannot be empty")
+		if (entry) {
+			throw new Error(`Profile with name "${name}" already exists`)
 		}
 
-		const currentSettings = this.getConfiguration()
-		const profiles = currentSettings.listApiConfigMeta || []
+		const id = await this.sidebarProvider.upsertProviderProfile(name, profile ?? {}, activate)
 
-		if (profiles.some((profile) => profile.name === name)) {
-			throw new Error(`A profile with the name "${name}" already exists`)
+		if (!id) {
+			throw new Error(`Failed to create profile with name "${name}"`)
 		}
 
-		const id = this.sidebarProvider.providerSettingsManager.generateId()
-
-		await this.setConfiguration({
-			...currentSettings,
-			listApiConfigMeta: [
-				...profiles,
-				{
-					id,
-					name: name.trim(),
-					apiProvider: "openai" as const,
-				},
-			],
-		})
-
 		return id
 	}
 
-	public async deleteProfile(name: string) {
-		const currentSettings = this.getConfiguration()
-		const listApiConfigMeta = this.getProfilesMeta()
-		const targetIndex = listApiConfigMeta.findIndex((p) => p.name === name)
+	public async updateProfile(
+		name: string,
+		profile: ProviderSettings,
+		activate: boolean = true,
+	): Promise<string | undefined> {
+		const entry = this.getProfileEntry(name)
 
-		if (targetIndex === -1) {
+		if (!entry) {
 			throw new Error(`Profile with name "${name}" does not exist`)
 		}
 
-		const profileToDelete = listApiConfigMeta[targetIndex]
-		listApiConfigMeta.splice(targetIndex, 1)
+		const id = await this.sidebarProvider.upsertProviderProfile(name, profile, activate)
+
+		if (!id) {
+			throw new Error(`Failed to update profile with name "${name}"`)
+		}
+
+		return id
+	}
+
+	public async upsertProfile(
+		name: string,
+		profile: ProviderSettings,
+		activate: boolean = true,
+	): Promise<string | undefined> {
+		const id = await this.sidebarProvider.upsertProviderProfile(name, profile, activate)
+
+		if (!id) {
+			throw new Error(`Failed to upsert profile with name "${name}"`)
+		}
+
+		return id
+	}
 
-		// If we're deleting the active profile, clear the currentApiConfigName.
-		const currentApiConfigName =
-			currentSettings.currentApiConfigName === profileToDelete.name
-				? undefined
-				: currentSettings.currentApiConfigName
+	public async deleteProfile(name: string): Promise<void> {
+		const entry = this.getProfileEntry(name)
 
-		await this.setConfiguration({ ...currentSettings, listApiConfigMeta, currentApiConfigName })
+		if (!entry) {
+			throw new Error(`Profile with name "${name}" does not exist`)
+		}
+
+		await this.sidebarProvider.deleteProviderProfile(entry)
 	}
 
 	public getActiveProfile(): string | undefined {
 		return this.getConfiguration().currentApiConfigName
 	}
 
-	public async setActiveProfile(name: string) {
-		if (!this.hasProfile(name)) {
+	public async setActiveProfile(name: string): Promise<string | undefined> {
+		const entry = this.getProfileEntry(name)
+
+		if (!entry) {
 			throw new Error(`Profile with name "${name}" does not exist`)
 		}
 
 		await this.sidebarProvider.activateProviderProfile({ name })
+		return this.getActiveProfile()
 	}
 }

src/exports/interface.ts

@@ -113,13 +113,41 @@ export interface RooCodeAPI extends EventEmitter<RooCodeEvents> {
 	 */
 	getProfiles(): string[]
 
+	/**
+	 * Returns the profile entry for a given name
+	 * @param name The name of the profile
+	 * @returns The profile entry, or undefined if the profile does not exist
+	 */
+	getProfileEntry(name: string): ProviderSettingsEntry | undefined
+
 	/**
 	 * Creates a new API configuration profile
 	 * @param name The name of the profile
+	 * @param profile The profile to create; defaults to an empty object
+	 * @param activate Whether to activate the profile after creation; defaults to true
 	 * @returns The ID of the created profile
 	 * @throws Error if the profile already exists
 	 */
-	createProfile(name: string, profile?: ProviderSettings): Promise<string>
+	createProfile(name: string, profile?: ProviderSettings, activate?: boolean): Promise<string>
+
+	/**
+	 * Updates an existing API configuration profile
+	 * @param name The name of the profile
+	 * @param profile The profile to update
+	 * @param activate Whether to activate the profile after update; defaults to true
+	 * @returns The ID of the updated profile
+	 * @throws Error if the profile does not exist
+	 */
+	updateProfile(name: string, profile: ProviderSettings, activate?: boolean): Promise<string | undefined>
+
+	/**
+	 * Creates a new API configuration profile or updates an existing one
+	 * @param name The name of the profile
+	 * @param profile The profile to create or update; defaults to an empty object
+	 * @param activate Whether to activate the profile after upsert; defaults to true
+	 * @returns The ID of the upserted profile
+	 */
+	upsertProfile(name: string, profile: ProviderSettings, activate?: boolean): Promise<string | undefined>
 
 	/**
 	 * Deletes a profile by name
@@ -139,5 +167,5 @@ export interface RooCodeAPI extends EventEmitter<RooCodeEvents> {
 	 * @param name The name of the profile to activate
 	 * @throws Error if the profile does not exist
 	 */
-	setActiveProfile(name: string): Promise<void>
+	setActiveProfile(name: string): Promise<string | undefined>
 }

src/exports/roo-code.d.ts

@@ -632,13 +632,38 @@ interface RooCodeAPI extends EventEmitter<RooCodeEvents> {
 	 * @returns Array of profile names
 	 */
 	getProfiles(): string[]
+	/**
+	 * Returns the profile entry for a given name
+	 * @param name The name of the profile
+	 * @returns The profile entry, or undefined if the profile does not exist
+	 */
+	getProfileEntry(name: string): ProviderSettingsEntry | undefined
 	/**
 	 * Creates a new API configuration profile
 	 * @param name The name of the profile
+	 * @param profile The profile to create; defaults to an empty object
+	 * @param activate Whether to activate the profile after creation; defaults to true
 	 * @returns The ID of the created profile
 	 * @throws Error if the profile already exists
 	 */
-	createProfile(name: string, profile?: ProviderSettings): Promise<string>
+	createProfile(name: string, profile?: ProviderSettings, activate?: boolean): Promise<string>
+	/**
+	 * Updates an existing API configuration profile
+	 * @param name The name of the profile
+	 * @param profile The profile to update
+	 * @param activate Whether to activate the profile after update; defaults to true
+	 * @returns The ID of the updated profile
+	 * @throws Error if the profile does not exist
+	 */
+	updateProfile(name: string, profile: ProviderSettings, activate?: boolean): Promise<string | undefined>
+	/**
+	 * Creates a new API configuration profile or updates an existing one
+	 * @param name The name of the profile
+	 * @param profile The profile to create or update; defaults to an empty object
+	 * @param activate Whether to activate the profile after upsert; defaults to true
+	 * @returns The ID of the upserted profile
+	 */
+	upsertProfile(name: string, profile: ProviderSettings, activate?: boolean): Promise<string | undefined>
 	/**
 	 * Deletes a profile by name
 	 * @param name The name of the profile to delete
@@ -655,7 +680,7 @@ interface RooCodeAPI extends EventEmitter<RooCodeEvents> {
 	 * @param name The name of the profile to activate
 	 * @throws Error if the profile does not exist
 	 */
-	setActiveProfile(name: string): Promise<void>
+	setActiveProfile(name: string): Promise<string | undefined>
 }
 
 export {

src/shared/WebviewMessage.ts

@@ -11,7 +11,6 @@ export type AudioType = "notification" | "celebration" | "progress_loop"
 
 export interface WebviewMessage {
 	type:
-		| "apiConfiguration"
 		| "deleteMultipleTasksWithIds"
 		| "currentApiConfigName"
 		| "saveApiConfiguration"