Merge pull request #853 from RooVetGit/add_profile_modal

Better UX for adding new API config profiles

.changeset/dirty-coins-exist.md

@@ -0,0 +1,5 @@
+---
+"roo-cline": patch
+---
+
+Improve the user experience for adding a new configuration profile

webview-ui/src/components/settings/ApiConfigManager.tsx

@@ -3,6 +3,7 @@ import { memo, useEffect, useRef, useState } from "react"
 import { ApiConfigMeta } from "../../../../src/shared/ExtensionMessage"
 import { Dropdown } from "vscrui"
 import type { DropdownOption } from "vscrui"
+import { Dialog, DialogContent } from "../ui/dialog"
 
 interface ApiConfigManagerProps {
 	currentApiConfigName?: string
@@ -21,55 +22,113 @@ const ApiConfigManager = ({
 	onRenameConfig,
 	onUpsertConfig,
 }: ApiConfigManagerProps) => {
-	const [editState, setEditState] = useState<"new" | "rename" | null>(null)
+	const [isRenaming, setIsRenaming] = useState(false)
+	const [isCreating, setIsCreating] = useState(false)
 	const [inputValue, setInputValue] = useState("")
-	const inputRef = useRef<HTMLInputElement>()
+	const [newProfileName, setNewProfileName] = useState("")
+	const [error, setError] = useState<string | null>(null)
+	const inputRef = useRef<any>(null)
+	const newProfileInputRef = useRef<any>(null)
 
-	// Focus input when entering edit mode
-	useEffect(() => {
-		if (editState) {
-			setTimeout(() => inputRef.current?.focus(), 0)
+	const validateName = (name: string, isNewProfile: boolean): string | null => {
+		const trimmed = name.trim()
+		if (!trimmed) return "Name cannot be empty"
+
+		const nameExists = listApiConfigMeta?.some((config) => config.name.toLowerCase() === trimmed.toLowerCase())
+
+		// For new profiles, any existing name is invalid
+		if (isNewProfile && nameExists) {
+			return "A profile with this name already exists"
 		}
-	}, [editState])
 
-	// Reset edit state when current profile changes
-	useEffect(() => {
-		setEditState(null)
+		// For rename, only block if trying to rename to a different existing profile
+		if (!isNewProfile && nameExists && trimmed.toLowerCase() !== currentApiConfigName?.toLowerCase()) {
+			return "A profile with this name already exists"
+		}
+
+		return null
+	}
+
+	const resetCreateState = () => {
+		setIsCreating(false)
+		setNewProfileName("")
+		setError(null)
+	}
+
+	const resetRenameState = () => {
+		setIsRenaming(false)
 		setInputValue("")
+		setError(null)
+	}
+
+	// Focus input when entering rename mode
+	useEffect(() => {
+		if (isRenaming) {
+			const timeoutId = setTimeout(() => inputRef.current?.focus(), 0)
+			return () => clearTimeout(timeoutId)
+		}
+	}, [isRenaming])
+
+	// Focus input when opening new dialog
+	useEffect(() => {
+		if (isCreating) {
+			const timeoutId = setTimeout(() => newProfileInputRef.current?.focus(), 0)
+			return () => clearTimeout(timeoutId)
+		}
+	}, [isCreating])
+
+	// Reset state when current profile changes
+	useEffect(() => {
+		resetCreateState()
+		resetRenameState()
 	}, [currentApiConfigName])
 
 	const handleAdd = () => {
-		const newConfigName = currentApiConfigName + " (copy)"
-		onUpsertConfig(newConfigName)
+		resetCreateState()
+		setIsCreating(true)
 	}
 
 	const handleStartRename = () => {
-		setEditState("rename")
+		setIsRenaming(true)
 		setInputValue(currentApiConfigName || "")
+		setError(null)
 	}
 
 	const handleCancel = () => {
-		setEditState(null)
-		setInputValue("")
+		resetRenameState()
 	}
 
 	const handleSave = () => {
 		const trimmedValue = inputValue.trim()
-		if (!trimmedValue) return
+		const error = validateName(trimmedValue, false)
 
-		if (editState === "new") {
-			onUpsertConfig(trimmedValue)
-		} else if (editState === "rename" && currentApiConfigName) {
+		if (error) {
+			setError(error)
+			return
+		}
+
+		if (isRenaming && currentApiConfigName) {
 			if (currentApiConfigName === trimmedValue) {
-				setEditState(null)
-				setInputValue("")
+				resetRenameState()
 				return
 			}
 			onRenameConfig(currentApiConfigName, trimmedValue)
 		}
 
-		setEditState(null)
-		setInputValue("")
+		resetRenameState()
+	}
+
+	const handleNewProfileSave = () => {
+		const trimmedValue = newProfileName.trim()
+		const error = validateName(trimmedValue, true)
+
+		if (error) {
+			setError(error)
+			return
+		}
+
+		onUpsertConfig(trimmedValue)
+		resetCreateState()
 	}
 
 	const handleDelete = () => {
@@ -93,49 +152,63 @@ const ApiConfigManager = ({
 					<span style={{ fontWeight: "500" }}>Configuration Profile</span>
 				</label>
 
-				{editState ? (
-					<div style={{ display: "flex", gap: "4px", alignItems: "center" }}>
-						<VSCodeTextField
-							ref={inputRef as any}
-							value={inputValue}
-							onInput={(e: any) => setInputValue(e.target.value)}
-							placeholder={editState === "new" ? "Enter profile name" : "Enter new name"}
-							style={{ flexGrow: 1 }}
-							onKeyDown={(e: any) => {
-								if (e.key === "Enter" && inputValue.trim()) {
-									handleSave()
-								} else if (e.key === "Escape") {
-									handleCancel()
-								}
-							}}
-						/>
-						<VSCodeButton
-							appearance="icon"
-							disabled={!inputValue.trim()}
-							onClick={handleSave}
-							title="Save"
-							style={{
-								padding: 0,
-								margin: 0,
-								height: "28px",
-								width: "28px",
-								minWidth: "28px",
-							}}>
-							<span className="codicon codicon-check" />
-						</VSCodeButton>
-						<VSCodeButton
-							appearance="icon"
-							onClick={handleCancel}
-							title="Cancel"
-							style={{
-								padding: 0,
-								margin: 0,
-								height: "28px",
-								width: "28px",
-								minWidth: "28px",
-							}}>
-							<span className="codicon codicon-close" />
-						</VSCodeButton>
+				{isRenaming ? (
+					<div
+						data-testid="rename-form"
+						style={{ display: "flex", gap: "4px", alignItems: "center", flexDirection: "column" }}>
+						<div style={{ display: "flex", gap: "4px", alignItems: "center", width: "100%" }}>
+							<VSCodeTextField
+								ref={inputRef}
+								value={inputValue}
+								onInput={(e: unknown) => {
+									const target = e as { target: { value: string } }
+									setInputValue(target.target.value)
+									setError(null)
+								}}
+								placeholder="Enter new name"
+								style={{ flexGrow: 1 }}
+								onKeyDown={(e: unknown) => {
+									const event = e as { key: string }
+									if (event.key === "Enter" && inputValue.trim()) {
+										handleSave()
+									} else if (event.key === "Escape") {
+										handleCancel()
+									}
+								}}
+							/>
+							<VSCodeButton
+								appearance="icon"
+								disabled={!inputValue.trim()}
+								onClick={handleSave}
+								title="Save"
+								style={{
+									padding: 0,
+									margin: 0,
+									height: "28px",
+									width: "28px",
+									minWidth: "28px",
+								}}>
+								<span className="codicon codicon-check" />
+							</VSCodeButton>
+							<VSCodeButton
+								appearance="icon"
+								onClick={handleCancel}
+								title="Cancel"
+								style={{
+									padding: 0,
+									margin: 0,
+									height: "28px",
+									width: "28px",
+									minWidth: "28px",
+								}}>
+								<span className="codicon codicon-close" />
+							</VSCodeButton>
+						</div>
+						{error && (
+							<p className="text-red-500 text-sm mt-2" data-testid="error-message">
+								{error}
+							</p>
+						)}
 					</div>
 				) : (
 					<>
@@ -211,6 +284,63 @@ const ApiConfigManager = ({
 						</p>
 					</>
 				)}
+
+				<Dialog
+					open={isCreating}
+					onOpenChange={(open: boolean) => {
+						if (open) {
+							setIsCreating(true)
+							setNewProfileName("")
+							setError(null)
+						} else {
+							resetCreateState()
+						}
+					}}
+					aria-labelledby="new-profile-title">
+					<DialogContent className="p-4 max-w-sm">
+						<h2 id="new-profile-title" className="text-lg font-semibold mb-4">
+							New Configuration Profile
+						</h2>
+						<button className="absolute right-4 top-4" aria-label="Close dialog" onClick={resetCreateState}>
+							<span className="codicon codicon-close" />
+						</button>
+						<VSCodeTextField
+							ref={newProfileInputRef}
+							value={newProfileName}
+							onInput={(e: unknown) => {
+								const target = e as { target: { value: string } }
+								setNewProfileName(target.target.value)
+								setError(null)
+							}}
+							placeholder="Enter profile name"
+							style={{ width: "100%" }}
+							onKeyDown={(e: unknown) => {
+								const event = e as { key: string }
+								if (event.key === "Enter" && newProfileName.trim()) {
+									handleNewProfileSave()
+								} else if (event.key === "Escape") {
+									resetCreateState()
+								}
+							}}
+						/>
+						{error && (
+							<p className="text-red-500 text-sm mt-2" data-testid="error-message">
+								{error}
+							</p>
+						)}
+						<div className="flex justify-end gap-2 mt-4">
+							<VSCodeButton appearance="secondary" onClick={resetCreateState}>
+								Cancel
+							</VSCodeButton>
+							<VSCodeButton
+								appearance="primary"
+								disabled={!newProfileName.trim()}
+								onClick={handleNewProfileSave}>
+								Create Profile
+							</VSCodeButton>
+						</div>
+					</DialogContent>
+				</Dialog>
 			</div>
 		</div>
 	)

webview-ui/src/components/settings/__tests__/ApiConfigManager.test.tsx

@@ -1,4 +1,4 @@
-import { render, screen, fireEvent } from "@testing-library/react"
+import { render, screen, fireEvent, within } from "@testing-library/react"
 import ApiConfigManager from "../ApiConfigManager"
 
 // Mock VSCode components
@@ -8,11 +8,12 @@ jest.mock("@vscode/webview-ui-toolkit/react", () => ({
 			{children}
 		</button>
 	),
-	VSCodeTextField: ({ value, onInput, placeholder }: any) => (
+	VSCodeTextField: ({ value, onInput, placeholder, onKeyDown }: any) => (
 		<input
 			value={value}
 			onChange={(e) => onInput(e)}
 			placeholder={placeholder}
+			onKeyDown={onKeyDown}
 			ref={undefined} // Explicitly set ref to undefined to avoid warning
 		/>
 	),
@@ -32,6 +33,16 @@ jest.mock("vscrui", () => ({
 	),
 }))
 
+// Mock Dialog component
+jest.mock("@/components/ui/dialog", () => ({
+	Dialog: ({ children, open, onOpenChange }: any) => (
+		<div role="dialog" aria-modal="true" style={{ display: open ? "block" : "none" }} data-testid="dialog">
+			{children}
+		</div>
+	),
+	DialogContent: ({ children }: any) => <div data-testid="dialog-content">{children}</div>,
+}))
+
 describe("ApiConfigManager", () => {
 	const mockOnSelectConfig = jest.fn()
 	const mockOnDeleteConfig = jest.fn()
@@ -54,34 +65,74 @@ describe("ApiConfigManager", () => {
 		jest.clearAllMocks()
 	})
 
-	it("immediately creates a copy when clicking add button", () => {
+	const getRenameForm = () => screen.getByTestId("rename-form")
+	const getDialogContent = () => screen.getByTestId("dialog-content")
+
+	it("opens new profile dialog when clicking add button", () => {
 		render(<ApiConfigManager {...defaultProps} />)
 
-		// Find and click the add button
 		const addButton = screen.getByTitle("Add profile")
 		fireEvent.click(addButton)
 
-		// Verify that onUpsertConfig was called with the correct name
-		expect(mockOnUpsertConfig).toHaveBeenCalledTimes(1)
-		expect(mockOnUpsertConfig).toHaveBeenCalledWith("Default Config (copy)")
+		expect(screen.getByTestId("dialog")).toBeVisible()
+		expect(screen.getByText("New Configuration Profile")).toBeInTheDocument()
 	})
 
-	it("creates copy with correct name when current config has spaces", () => {
-		render(<ApiConfigManager {...defaultProps} currentApiConfigName="My Test Config" />)
+	it("creates new profile with entered name", () => {
+		render(<ApiConfigManager {...defaultProps} />)
 
+		// Open dialog
 		const addButton = screen.getByTitle("Add profile")
 		fireEvent.click(addButton)
 
-		expect(mockOnUpsertConfig).toHaveBeenCalledWith("My Test Config (copy)")
+		// Enter new profile name
+		const input = screen.getByPlaceholderText("Enter profile name")
+		fireEvent.input(input, { target: { value: "New Profile" } })
+
+		// Click create button
+		const createButton = screen.getByText("Create Profile")
+		fireEvent.click(createButton)
+
+		expect(mockOnUpsertConfig).toHaveBeenCalledWith("New Profile")
 	})
 
-	it("handles empty current config name gracefully", () => {
-		render(<ApiConfigManager {...defaultProps} currentApiConfigName="" />)
+	it("shows error when creating profile with existing name", () => {
+		render(<ApiConfigManager {...defaultProps} />)
 
+		// Open dialog
 		const addButton = screen.getByTitle("Add profile")
 		fireEvent.click(addButton)
 
-		expect(mockOnUpsertConfig).toHaveBeenCalledWith(" (copy)")
+		// Enter existing profile name
+		const input = screen.getByPlaceholderText("Enter profile name")
+		fireEvent.input(input, { target: { value: "Default Config" } })
+
+		// Click create button to trigger validation
+		const createButton = screen.getByText("Create Profile")
+		fireEvent.click(createButton)
+
+		// Verify error message
+		const dialogContent = getDialogContent()
+		const errorMessage = within(dialogContent).getByTestId("error-message")
+		expect(errorMessage).toHaveTextContent("A profile with this name already exists")
+		expect(mockOnUpsertConfig).not.toHaveBeenCalled()
+	})
+
+	it("prevents creating profile with empty name", () => {
+		render(<ApiConfigManager {...defaultProps} />)
+
+		// Open dialog
+		const addButton = screen.getByTitle("Add profile")
+		fireEvent.click(addButton)
+
+		// Enter empty name
+		const input = screen.getByPlaceholderText("Enter profile name")
+		fireEvent.input(input, { target: { value: "   " } })
+
+		// Verify create button is disabled
+		const createButton = screen.getByText("Create Profile")
+		expect(createButton).toBeDisabled()
+		expect(mockOnUpsertConfig).not.toHaveBeenCalled()
 	})
 
 	it("allows renaming the current config", () => {
@@ -102,6 +153,45 @@ describe("ApiConfigManager", () => {
 		expect(mockOnRenameConfig).toHaveBeenCalledWith("Default Config", "New Name")
 	})
 
+	it("shows error when renaming to existing config name", () => {
+		render(<ApiConfigManager {...defaultProps} />)
+
+		// Start rename
+		const renameButton = screen.getByTitle("Rename profile")
+		fireEvent.click(renameButton)
+
+		// Find input and enter existing name
+		const input = screen.getByDisplayValue("Default Config")
+		fireEvent.input(input, { target: { value: "Another Config" } })
+
+		// Save to trigger validation
+		const saveButton = screen.getByTitle("Save")
+		fireEvent.click(saveButton)
+
+		// Verify error message
+		const renameForm = getRenameForm()
+		const errorMessage = within(renameForm).getByTestId("error-message")
+		expect(errorMessage).toHaveTextContent("A profile with this name already exists")
+		expect(mockOnRenameConfig).not.toHaveBeenCalled()
+	})
+
+	it("prevents renaming to empty name", () => {
+		render(<ApiConfigManager {...defaultProps} />)
+
+		// Start rename
+		const renameButton = screen.getByTitle("Rename profile")
+		fireEvent.click(renameButton)
+
+		// Find input and enter empty name
+		const input = screen.getByDisplayValue("Default Config")
+		fireEvent.input(input, { target: { value: "   " } })
+
+		// Verify save button is disabled
+		const saveButton = screen.getByTitle("Save")
+		expect(saveButton).toBeDisabled()
+		expect(mockOnRenameConfig).not.toHaveBeenCalled()
+	})
+
 	it("allows selecting a different config", () => {
 		render(<ApiConfigManager {...defaultProps} />)
 
@@ -149,4 +239,42 @@ describe("ApiConfigManager", () => {
 		// Verify we're back to normal view
 		expect(screen.queryByDisplayValue("New Name")).not.toBeInTheDocument()
 	})
+
+	it("handles keyboard events in new profile dialog", () => {
+		render(<ApiConfigManager {...defaultProps} />)
+
+		// Open dialog
+		const addButton = screen.getByTitle("Add profile")
+		fireEvent.click(addButton)
+
+		const input = screen.getByPlaceholderText("Enter profile name")
+
+		// Test Enter key
+		fireEvent.input(input, { target: { value: "New Profile" } })
+		fireEvent.keyDown(input, { key: "Enter" })
+		expect(mockOnUpsertConfig).toHaveBeenCalledWith("New Profile")
+
+		// Test Escape key
+		fireEvent.keyDown(input, { key: "Escape" })
+		expect(screen.getByTestId("dialog")).not.toBeVisible()
+	})
+
+	it("handles keyboard events in rename mode", () => {
+		render(<ApiConfigManager {...defaultProps} />)
+
+		// Start rename
+		const renameButton = screen.getByTitle("Rename profile")
+		fireEvent.click(renameButton)
+
+		const input = screen.getByDisplayValue("Default Config")
+
+		// Test Enter key
+		fireEvent.input(input, { target: { value: "New Name" } })
+		fireEvent.keyDown(input, { key: "Enter" })
+		expect(mockOnRenameConfig).toHaveBeenCalledWith("Default Config", "New Name")
+
+		// Test Escape key
+		fireEvent.keyDown(input, { key: "Escape" })
+		expect(screen.queryByDisplayValue("New Name")).not.toBeInTheDocument()
+	})
 })