diff --git a/.changeset/dirty-coins-exist.md b/.changeset/dirty-coins-exist.md new file mode 100644 index 0000000000..d01a3ba76e --- /dev/null +++ b/.changeset/dirty-coins-exist.md @@ -0,0 +1,5 @@ +--- +"roo-cline": patch +--- + +Improve the user experience for adding a new configuration profile diff --git a/webview-ui/src/components/settings/ApiConfigManager.tsx b/webview-ui/src/components/settings/ApiConfigManager.tsx index b10adf4a49..652803fe76 100644 --- a/webview-ui/src/components/settings/ApiConfigManager.tsx +++ b/webview-ui/src/components/settings/ApiConfigManager.tsx @@ -1,8 +1,9 @@ import { VSCodeButton, VSCodeTextField } from "@vscode/webview-ui-toolkit/react" -import { memo, useEffect, useRef, useState } from "react" +import { memo, useEffect, useReducer, useRef } 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 @@ -13,6 +14,86 @@ interface ApiConfigManagerProps { onUpsertConfig: (configName: string) => void } +type State = { + isRenaming: boolean + isCreating: boolean + inputValue: string + newProfileName: string + error: string | null +} + +type Action = + | { type: "START_RENAME"; payload: string } + | { type: "CANCEL_EDIT" } + | { type: "SET_INPUT"; payload: string } + | { type: "SET_NEW_NAME"; payload: string } + | { type: "START_CREATE" } + | { type: "CANCEL_CREATE" } + | { type: "SET_ERROR"; payload: string | null } + | { type: "RESET_STATE" } + +const initialState: State = { + isRenaming: false, + isCreating: false, + inputValue: "", + newProfileName: "", + error: null, +} + +const reducer = (state: State, action: Action): State => { + switch (action.type) { + case "START_RENAME": + return { + ...state, + isRenaming: true, + inputValue: action.payload, + error: null, + } + case "CANCEL_EDIT": + return { + ...state, + isRenaming: false, + inputValue: "", + error: null, + } + case "SET_INPUT": + return { + ...state, + inputValue: action.payload, + error: null, + } + case "SET_NEW_NAME": + return { + ...state, + newProfileName: action.payload, + error: null, + } + case "START_CREATE": + return { + ...state, + isCreating: true, + newProfileName: "", + error: null, + } + case "CANCEL_CREATE": + return { + ...state, + isCreating: false, + newProfileName: "", + error: null, + } + case "SET_ERROR": + return { + ...state, + error: action.payload, + } + case "RESET_STATE": + return initialState + default: + return state + } +} + const ApiConfigManager = ({ currentApiConfigName = "", listApiConfigMeta = [], @@ -21,55 +102,93 @@ const ApiConfigManager = ({ onRenameConfig, onUpsertConfig, }: ApiConfigManagerProps) => { - const [editState, setEditState] = useState<"new" | "rename" | null>(null) - const [inputValue, setInputValue] = useState("") - const inputRef = useRef() + const [state, dispatch] = useReducer(reducer, initialState) + const inputRef = useRef(null) + const newProfileInputRef = useRef(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 + // 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 + } + + // Focus input when entering rename mode useEffect(() => { - setEditState(null) - setInputValue("") + if (state.isRenaming) { + const timeoutId = setTimeout(() => inputRef.current?.focus(), 0) + return () => clearTimeout(timeoutId) + } + }, [state.isRenaming]) + + // Focus input when opening new dialog + useEffect(() => { + if (state.isCreating) { + const timeoutId = setTimeout(() => newProfileInputRef.current?.focus(), 0) + return () => clearTimeout(timeoutId) + } + }, [state.isCreating]) + + // Reset state when current profile changes + useEffect(() => { + dispatch({ type: "RESET_STATE" }) }, [currentApiConfigName]) const handleAdd = () => { - const newConfigName = currentApiConfigName + " (copy)" - onUpsertConfig(newConfigName) + dispatch({ type: "START_CREATE" }) } const handleStartRename = () => { - setEditState("rename") - setInputValue(currentApiConfigName || "") + dispatch({ type: "START_RENAME", payload: currentApiConfigName || "" }) } const handleCancel = () => { - setEditState(null) - setInputValue("") + dispatch({ type: "CANCEL_EDIT" }) } const handleSave = () => { - const trimmedValue = inputValue.trim() - if (!trimmedValue) return + const trimmedValue = state.inputValue.trim() + const error = validateName(trimmedValue, false) - if (editState === "new") { - onUpsertConfig(trimmedValue) - } else if (editState === "rename" && currentApiConfigName) { + if (error) { + dispatch({ type: "SET_ERROR", payload: error }) + return + } + + if (state.isRenaming && currentApiConfigName) { if (currentApiConfigName === trimmedValue) { - setEditState(null) - setInputValue("") + dispatch({ type: "CANCEL_EDIT" }) return } onRenameConfig(currentApiConfigName, trimmedValue) } - setEditState(null) - setInputValue("") + dispatch({ type: "CANCEL_EDIT" }) + } + + const handleNewProfileSave = () => { + const trimmedValue = state.newProfileName.trim() + const error = validateName(trimmedValue, true) + + if (error) { + dispatch({ type: "SET_ERROR", payload: error }) + return + } + + onUpsertConfig(trimmedValue) + dispatch({ type: "CANCEL_CREATE" }) } const handleDelete = () => { @@ -93,49 +212,62 @@ const ApiConfigManager = ({ Configuration Profile - {editState ? ( -
- 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() - } - }} - /> - - - - - - + {state.isRenaming ? ( +
+
+ { + const target = e as { target: { value: string } } + dispatch({ type: "SET_INPUT", payload: target.target.value }) + }} + placeholder="Enter new name" + style={{ flexGrow: 1 }} + onKeyDown={(e: unknown) => { + const event = e as { key: string } + if (event.key === "Enter" && state.inputValue.trim()) { + handleSave() + } else if (event.key === "Escape") { + handleCancel() + } + }} + /> + + + + + + +
+ {state.error && ( +

+ {state.error} +

+ )}
) : ( <> @@ -211,6 +343,57 @@ const ApiConfigManager = ({

)} + + dispatch({ type: open ? "START_CREATE" : "CANCEL_CREATE" })} + aria-labelledby="new-profile-title"> + +

+ New Configuration Profile +

+ + { + const target = e as { target: { value: string } } + dispatch({ type: "SET_NEW_NAME", payload: target.target.value }) + }} + placeholder="Enter profile name" + style={{ width: "100%" }} + onKeyDown={(e: unknown) => { + const event = e as { key: string } + if (event.key === "Enter" && state.newProfileName.trim()) { + handleNewProfileSave() + } else if (event.key === "Escape") { + dispatch({ type: "CANCEL_CREATE" }) + } + }} + /> + {state.error && ( +

+ {state.error} +

+ )} +
+ dispatch({ type: "CANCEL_CREATE" })}> + Cancel + + + Create Profile + +
+ +
) diff --git a/webview-ui/src/components/settings/__tests__/ApiConfigManager.test.tsx b/webview-ui/src/components/settings/__tests__/ApiConfigManager.test.tsx index ac6245d6d1..24e62215ec 100644 --- a/webview-ui/src/components/settings/__tests__/ApiConfigManager.test.tsx +++ b/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} ), - VSCodeTextField: ({ value, onInput, placeholder }: any) => ( + VSCodeTextField: ({ value, onInput, placeholder, onKeyDown }: any) => ( 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) => ( +
+ {children} +
+ ), + DialogContent: ({ children }: any) =>
{children}
, +})) + 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() - // 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() + it("creates new profile with entered name", () => { + render() + // 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() + it("shows error when creating profile with existing name", () => { + render() + // 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() + + // 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() + + // 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() + + // 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() @@ -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() + + // 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() + + // 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() + }) })