diff --git a/webview-ui/src/__tests__/SearchableSelect.spec.tsx b/webview-ui/src/__tests__/SearchableSelect.spec.tsx index bcc0a2c8b7..a05acbf914 100644 --- a/webview-ui/src/__tests__/SearchableSelect.spec.tsx +++ b/webview-ui/src/__tests__/SearchableSelect.spec.tsx @@ -253,4 +253,67 @@ describe("SearchableSelect", () => { expect(within(dropdown).queryByText("Option 3")).not.toBeInTheDocument() }) }) + + it("closes the dropdown when ESC key is pressed", async () => { + const user = userEvent.setup() + render() + + // Open the dropdown + const trigger = screen.getByRole("combobox") + await user.click(trigger) + + // Verify dropdown is open + expect(trigger).toHaveAttribute("aria-expanded", "true") + expect(screen.getByRole("listbox")).toBeInTheDocument() + + // Press ESC key + fireEvent.keyDown(window, { key: "Escape" }) + + // Verify dropdown is closed + await waitFor(() => { + expect(trigger).toHaveAttribute("aria-expanded", "false") + expect(screen.queryByRole("listbox")).not.toBeInTheDocument() + }) + }) + + it("does not close the dropdown when ESC is pressed while dropdown is closed", async () => { + render() + + const trigger = screen.getByRole("combobox") + + // Ensure dropdown is closed + expect(trigger).toHaveAttribute("aria-expanded", "false") + + // Press ESC key + fireEvent.keyDown(window, { key: "Escape" }) + + // Verify dropdown remains closed + expect(trigger).toHaveAttribute("aria-expanded", "false") + expect(screen.queryByRole("listbox")).not.toBeInTheDocument() + }) + + it("prevents default and stops propagation when ESC is pressed", async () => { + const user = userEvent.setup() + render() + + // Open the dropdown + const trigger = screen.getByRole("combobox") + await user.click(trigger) + + // Create a mock event to track preventDefault and stopPropagation + const escapeEvent = new KeyboardEvent("keydown", { + key: "Escape", + bubbles: true, + cancelable: true, + }) + const preventDefaultSpy = vi.spyOn(escapeEvent, "preventDefault") + const stopPropagationSpy = vi.spyOn(escapeEvent, "stopPropagation") + + // Dispatch the event + window.dispatchEvent(escapeEvent) + + // Verify preventDefault and stopPropagation were called + expect(preventDefaultSpy).toHaveBeenCalled() + expect(stopPropagationSpy).toHaveBeenCalled() + }) }) diff --git a/webview-ui/src/components/chat/CodeIndexPopover.tsx b/webview-ui/src/components/chat/CodeIndexPopover.tsx index d7683e8c7e..c85aaf6ea5 100644 --- a/webview-ui/src/components/chat/CodeIndexPopover.tsx +++ b/webview-ui/src/components/chat/CodeIndexPopover.tsx @@ -38,6 +38,7 @@ import { } from "@src/components/ui" import { AlertTriangle } from "lucide-react" import { useRooPortal } from "@src/components/ui/hooks/useRooPortal" +import { useEscapeKey } from "@src/hooks/useEscapeKey" import type { EmbedderProvider } from "@roo/embeddingModels" import type { IndexingStatus } from "@roo/ExtensionMessage" import { CODEBASE_INDEX_DEFAULTS } from "@roo-code/types" @@ -440,6 +441,9 @@ export const CodeIndexPopover: React.FC = ({ }) }, [checkUnsavedChanges]) + // Use the shared ESC key handler hook - respects unsaved changes logic + useEscapeKey(open, handlePopoverClose) + const handleSaveSettings = () => { // Validate settings before saving if (!validateSettings()) { diff --git a/webview-ui/src/components/chat/__tests__/CodeIndexPopover.validation.spec.tsx b/webview-ui/src/components/chat/__tests__/CodeIndexPopover.validation.spec.tsx deleted file mode 100644 index 60399865f3..0000000000 --- a/webview-ui/src/components/chat/__tests__/CodeIndexPopover.validation.spec.tsx +++ /dev/null @@ -1,381 +0,0 @@ -import React from "react" -import { render, screen, fireEvent, waitFor } from "@testing-library/react" -import { describe, it, expect, vi, beforeEach } from "vitest" -import { CodeIndexPopover } from "../CodeIndexPopover" - -// Mock the vscode API -vi.mock("@src/utils/vscode", () => ({ - vscode: { - postMessage: vi.fn(), - }, -})) - -// Mock the extension state context -vi.mock("@src/context/ExtensionStateContext", () => ({ - useExtensionState: vi.fn(), -})) - -// Mock the translation context -vi.mock("@src/i18n/TranslationContext", () => ({ - useAppTranslation: () => ({ t: vi.fn((key: string) => key) }), -})) - -// Mock react-i18next -vi.mock("react-i18next", () => ({ - Trans: ({ children }: { children: React.ReactNode }) =>
{children}
, -})) - -// Mock the doc links utility -vi.mock("@src/utils/docLinks", () => ({ - buildDocLink: vi.fn(() => "https://docs.roocode.com"), -})) - -// Mock the portal hook -vi.mock("@src/components/ui/hooks/useRooPortal", () => ({ - useRooPortal: () => ({ portalContainer: document.body }), -})) - -// Mock Radix UI components to avoid portal issues -vi.mock("@src/components/ui", () => ({ - Popover: ({ children }: { children: React.ReactNode }) =>
{children}
, - PopoverContent: ({ children }: { children: React.ReactNode }) =>
{children}
, - PopoverTrigger: ({ children }: { children: React.ReactNode }) =>
{children}
, - Select: ({ children }: { children: React.ReactNode }) =>
{children}
, - SelectContent: ({ children }: { children: React.ReactNode }) =>
{children}
, - SelectItem: ({ children, value }: { children: React.ReactNode; value: string }) => ( -
- {children} -
- ), - SelectTrigger: ({ children }: { children: React.ReactNode }) =>
{children}
, - SelectValue: ({ placeholder }: { placeholder?: string }) => {placeholder}, - AlertDialog: ({ children }: { children: React.ReactNode }) =>
{children}
, - AlertDialogAction: ({ children }: { children: React.ReactNode }) => , - AlertDialogCancel: ({ children }: { children: React.ReactNode }) => , - AlertDialogContent: ({ children }: { children: React.ReactNode }) =>
{children}
, - AlertDialogDescription: ({ children }: { children: React.ReactNode }) =>
{children}
, - AlertDialogFooter: ({ children }: { children: React.ReactNode }) =>
{children}
, - AlertDialogHeader: ({ children }: { children: React.ReactNode }) =>
{children}
, - AlertDialogTitle: ({ children }: { children: React.ReactNode }) =>
{children}
, - AlertDialogTrigger: ({ children }: { children: React.ReactNode }) =>
{children}
, - Slider: ({ value, onValueChange }: { value: number[]; onValueChange: (value: number[]) => void }) => ( - onValueChange([parseInt(e.target.value)])} /> - ), - StandardTooltip: ({ children }: { children: React.ReactNode }) =>
{children}
, - cn: (...classes: string[]) => classes.join(" "), -})) - -// Mock VSCode web components to behave like regular HTML inputs -vi.mock("@vscode/webview-ui-toolkit/react", () => ({ - VSCodeTextField: ({ value, onInput, placeholder, className, ...rest }: any) => ( - onInput && onInput(e)} - placeholder={placeholder} - className={className} - aria-label="Text field" - {...rest} - /> - ), - VSCodeButton: ({ children, onClick, ...rest }: any) => ( - - ), - VSCodeDropdown: ({ value, onChange, children, className, ...rest }: any) => ( - - ), - VSCodeOption: ({ value, children, ...rest }: any) => ( - - ), - VSCodeLink: ({ href, children, ...rest }: any) => ( - - {children} - - ), - VSCodeCheckbox: ({ checked, onChange, children, ...rest }: any) => ( - - ), -})) - -// Helper function to simulate input on form elements -const simulateInput = (element: Element, value: string) => { - // Now that we're mocking VSCode components as regular HTML inputs, - // we can use standard fireEvent.change - fireEvent.change(element, { target: { value } }) -} - -describe("CodeIndexPopover Validation", () => { - let mockUseExtensionState: any - - beforeEach(async () => { - vi.clearAllMocks() - - // Get the mocked function - const { useExtensionState } = await import("@src/context/ExtensionStateContext") - mockUseExtensionState = vi.mocked(useExtensionState) - - // Setup default extension state - mockUseExtensionState.mockReturnValue({ - codebaseIndexConfig: { - codebaseIndexEnabled: false, - codebaseIndexQdrantUrl: "", - codebaseIndexEmbedderProvider: "openai", - codebaseIndexEmbedderBaseUrl: "", - codebaseIndexEmbedderModelId: "", - codebaseIndexSearchMaxResults: 10, - codebaseIndexSearchMinScore: 0.7, - codebaseIndexOpenAiCompatibleBaseUrl: "", - codebaseIndexEmbedderModelDimension: undefined, - }, - codebaseIndexModels: { - openai: [{ dimension: 1536 }], - }, - }) - }) - - const renderComponent = () => { - return render( - - - , - ) - } - - const openPopover = async () => { - const trigger = screen.getByText("Test Trigger") - fireEvent.click(trigger) - - // Wait for popover to open - await waitFor(() => { - expect(screen.getByRole("dialog")).toBeInTheDocument() - }) - } - - const expandSetupSection = async () => { - const setupButton = screen.getByText("settings:codeIndex.setupConfigLabel") - fireEvent.click(setupButton) - - // Wait for section to expand - look for vscode-text-field elements - await waitFor(() => { - const textFields = screen.getAllByLabelText("Text field") - expect(textFields.length).toBeGreaterThan(0) - }) - } - - describe("OpenAI Provider Validation", () => { - it("should show validation error when OpenAI API key is empty", async () => { - renderComponent() - await openPopover() - await expandSetupSection() - - // First, make a change to enable the save button by modifying the Qdrant URL - const qdrantUrlField = screen.getByPlaceholderText(/settings:codeIndex.qdrantUrlPlaceholder/i) - fireEvent.change(qdrantUrlField, { target: { value: "http://localhost:6333" } }) - - // Wait for the save button to become enabled - await waitFor(() => { - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - expect(saveButton).not.toBeDisabled() - }) - - // Now clear the OpenAI API key to create a validation error - const apiKeyField = screen.getByPlaceholderText(/settings:codeIndex.openAiKeyPlaceholder/i) - fireEvent.change(apiKeyField, { target: { value: "" } }) - - // Click the save button to trigger validation - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - fireEvent.click(saveButton) - - // Should show specific field error - await waitFor(() => { - expect(screen.getByText("settings:codeIndex.validation.openaiApiKeyRequired")).toBeInTheDocument() - }) - }) - - it("should show validation error when model is not selected", async () => { - renderComponent() - await openPopover() - await expandSetupSection() - - // First, make a change to enable the save button - const qdrantUrlField = screen.getByPlaceholderText(/settings:codeIndex.qdrantUrlPlaceholder/i) - fireEvent.change(qdrantUrlField, { target: { value: "http://localhost:6333" } }) - - // Set API key but leave model empty - const apiKeyField = screen.getByPlaceholderText(/settings:codeIndex.openAiKeyPlaceholder/i) - fireEvent.change(apiKeyField, { target: { value: "test-api-key" } }) - - // Wait for the save button to become enabled - await waitFor(() => { - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - expect(saveButton).not.toBeDisabled() - }) - - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - fireEvent.click(saveButton) - - await waitFor(() => { - expect(screen.getByText("settings:codeIndex.validation.modelSelectionRequired")).toBeInTheDocument() - }) - }) - }) - - describe("Qdrant URL Validation", () => { - it("should show validation error when Qdrant URL is empty", async () => { - renderComponent() - await openPopover() - await expandSetupSection() - - // First, make a change to enable the save button by setting API key - const apiKeyField = screen.getByPlaceholderText(/settings:codeIndex.openAiKeyPlaceholder/i) - fireEvent.change(apiKeyField, { target: { value: "test-api-key" } }) - - // Clear the Qdrant URL to create validation error - const qdrantUrlField = screen.getByPlaceholderText(/settings:codeIndex.qdrantUrlPlaceholder/i) - fireEvent.change(qdrantUrlField, { target: { value: "" } }) - - // Wait for the save button to become enabled - await waitFor(() => { - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - expect(saveButton).not.toBeDisabled() - }) - - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - fireEvent.click(saveButton) - - await waitFor(() => { - expect(screen.getByText("settings:codeIndex.validation.invalidQdrantUrl")).toBeInTheDocument() - }) - }) - - it("should show validation error when Qdrant URL is invalid", async () => { - renderComponent() - await openPopover() - await expandSetupSection() - - // First, make a change to enable the save button by setting API key - const apiKeyField = screen.getByPlaceholderText(/settings:codeIndex.openAiKeyPlaceholder/i) - fireEvent.change(apiKeyField, { target: { value: "test-api-key" } }) - - // Set invalid Qdrant URL - const qdrantUrlField = screen.getByPlaceholderText(/settings:codeIndex.qdrantUrlPlaceholder/i) - fireEvent.change(qdrantUrlField, { target: { value: "invalid-url" } }) - - // Wait for the save button to become enabled - await waitFor(() => { - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - expect(saveButton).not.toBeDisabled() - }) - - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - fireEvent.click(saveButton) - - await waitFor(() => { - expect(screen.getByText("settings:codeIndex.validation.invalidQdrantUrl")).toBeInTheDocument() - }) - }) - }) - - describe("Common Field Validation", () => { - it("should not show validation error for optional Qdrant API key", async () => { - renderComponent() - await openPopover() - await expandSetupSection() - - // Set required fields to make form valid - const qdrantUrlField = screen.getByPlaceholderText(/settings:codeIndex.qdrantUrlPlaceholder/i) - fireEvent.change(qdrantUrlField, { target: { value: "http://localhost:6333" } }) - - const apiKeyField = screen.getByPlaceholderText(/settings:codeIndex.openAiKeyPlaceholder/i) - fireEvent.change(apiKeyField, { target: { value: "test-api-key" } }) - - // Select a model - this is required (get the select element specifically) - const modelSelect = screen.getAllByRole("combobox").find((el) => el.tagName === "SELECT") - if (modelSelect) { - fireEvent.change(modelSelect, { target: { value: "0" } }) - } - - // Leave Qdrant API key empty (it's optional) - const qdrantApiKeyField = screen.getByPlaceholderText(/settings:codeIndex.qdrantApiKeyPlaceholder/i) - fireEvent.change(qdrantApiKeyField, { target: { value: "" } }) - - // Wait for the save button to become enabled - await waitFor(() => { - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - expect(saveButton).not.toBeDisabled() - }) - - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - fireEvent.click(saveButton) - - // Should not show validation errors since Qdrant API key is optional - }) - - it("should clear validation errors when fields are corrected", async () => { - renderComponent() - await openPopover() - await expandSetupSection() - - // First make an invalid change to enable the save button and trigger validation - const textFields = screen.getAllByLabelText("Text field") - const qdrantField = textFields.find((field) => - field.getAttribute("placeholder")?.toLowerCase().includes("qdrant"), - ) - - if (qdrantField) { - simulateInput(qdrantField, "invalid-url") // Invalid URL to trigger validation - } - - // Wait for save button to be enabled - const saveButton = screen.getByText("settings:codeIndex.saveSettings") - await waitFor(() => { - expect(saveButton).not.toBeDisabled() - }) - - // Click save to trigger validation errors - fireEvent.click(saveButton) - - // Now fix the errors with valid values - const apiKeyField = textFields.find( - (field) => - field.getAttribute("placeholder")?.toLowerCase().includes("openai") || - field.getAttribute("placeholder")?.toLowerCase().includes("key"), - ) - - // Set valid Qdrant URL - if (qdrantField) { - simulateInput(qdrantField, "http://localhost:6333") - } - - // Set API key - if (apiKeyField) { - simulateInput(apiKeyField, "test-api-key") - } - - // Select a model - this is required (get the select element specifically) - const modelSelect = screen.getAllByRole("combobox").find((el) => el.tagName === "SELECT") - if (modelSelect) { - fireEvent.change(modelSelect, { target: { value: "0" } }) - } - - // Try to save again - fireEvent.click(saveButton) - - // Validation errors should be cleared (specific field errors are checked elsewhere) - }) - }) -}) diff --git a/webview-ui/src/components/modes/ModesView.tsx b/webview-ui/src/components/modes/ModesView.tsx index 170d03b0e4..c2b67bc450 100644 --- a/webview-ui/src/components/modes/ModesView.tsx +++ b/webview-ui/src/components/modes/ModesView.tsx @@ -48,6 +48,7 @@ import { StandardTooltip, } from "@src/components/ui" import { DeleteModeDialog } from "@src/components/modes/DeleteModeDialog" +import { useEscapeKey } from "@src/hooks/useEscapeKey" // Get all available groups that should show in prompts view const availableGroups = (Object.keys(TOOL_GROUPS) as ToolGroup[]).filter((group) => !TOOL_GROUPS[group].alwaysAvailable) @@ -194,6 +195,9 @@ const ModesView = ({ onDone }: ModesViewProps) => { } }, []) + // Use the shared ESC key handler hook + useEscapeKey(open, () => setOpen(false)) + // Handler for clearing search input const onClearSearch = useCallback(() => { setSearchValue("") diff --git a/webview-ui/src/components/modes/__tests__/ModesView.spec.tsx b/webview-ui/src/components/modes/__tests__/ModesView.spec.tsx index e202114bbb..56ab791360 100644 --- a/webview-ui/src/components/modes/__tests__/ModesView.spec.tsx +++ b/webview-ui/src/components/modes/__tests__/ModesView.spec.tsx @@ -231,4 +231,37 @@ describe("PromptsView", () => { text: undefined, }) }) + + it("closes the mode selection popover when ESC key is pressed", async () => { + renderPromptsView() + const selectTrigger = screen.getByTestId("mode-select-trigger") + + // Open the popover + fireEvent.click(selectTrigger) + await waitFor(() => { + expect(selectTrigger).toHaveAttribute("aria-expanded", "true") + }) + + // Press ESC key + fireEvent.keyDown(window, { key: "Escape" }) + + // Verify popover is closed + await waitFor(() => { + expect(selectTrigger).toHaveAttribute("aria-expanded", "false") + }) + }) + + it("does not close the popover when ESC is pressed while popover is closed", async () => { + renderPromptsView() + const selectTrigger = screen.getByTestId("mode-select-trigger") + + // Ensure popover is closed + expect(selectTrigger).toHaveAttribute("aria-expanded", "false") + + // Press ESC key + fireEvent.keyDown(window, { key: "Escape" }) + + // Verify popover remains closed + expect(selectTrigger).toHaveAttribute("aria-expanded", "false") + }) }) diff --git a/webview-ui/src/components/settings/ModelPicker.tsx b/webview-ui/src/components/settings/ModelPicker.tsx index 5882d4f3f3..c8445766f1 100644 --- a/webview-ui/src/components/settings/ModelPicker.tsx +++ b/webview-ui/src/components/settings/ModelPicker.tsx @@ -21,6 +21,7 @@ import { PopoverTrigger, Button, } from "@src/components/ui" +import { useEscapeKey } from "@src/hooks/useEscapeKey" import { ModelInfoView } from "./ModelInfoView" import { ApiErrorMessage } from "./ApiErrorMessage" @@ -133,6 +134,9 @@ export const ModelPicker = ({ } }, []) + // Use the shared ESC key handler hook + useEscapeKey(open, () => setOpen(false)) + return ( <>
diff --git a/webview-ui/src/components/ui/searchable-select.tsx b/webview-ui/src/components/ui/searchable-select.tsx index 7317dda781..ec4067c6ef 100644 --- a/webview-ui/src/components/ui/searchable-select.tsx +++ b/webview-ui/src/components/ui/searchable-select.tsx @@ -13,6 +13,7 @@ import { PopoverContent, PopoverTrigger, } from "@/components/ui" +import { useEscapeKey } from "@/hooks/useEscapeKey" export interface SearchableSelectOption { value: string @@ -79,6 +80,9 @@ export function SearchableSelect({ return () => clearTimeout(timeoutId) }, [value]) + // Use the shared ESC key handler hook + useEscapeKey(open, () => setOpen(false)) + const handleOpenChange = (open: boolean) => { setOpen(open) // Reset search when closing diff --git a/webview-ui/src/hooks/useEscapeKey.spec.ts b/webview-ui/src/hooks/useEscapeKey.spec.ts new file mode 100644 index 0000000000..057235b7cf --- /dev/null +++ b/webview-ui/src/hooks/useEscapeKey.spec.ts @@ -0,0 +1,168 @@ +import { renderHook } from "@testing-library/react" +import { vi } from "vitest" +import { useEscapeKey } from "./useEscapeKey" + +describe("useEscapeKey", () => { + let mockOnEscape: ReturnType + + beforeEach(() => { + mockOnEscape = vi.fn() + }) + + afterEach(() => { + vi.clearAllMocks() + }) + + it("should call onEscape when Escape key is pressed and isOpen is true", () => { + renderHook(() => useEscapeKey(true, mockOnEscape)) + + const event = new KeyboardEvent("keydown", { key: "Escape" }) + window.dispatchEvent(event) + + expect(mockOnEscape).toHaveBeenCalledTimes(1) + }) + + it("should not call onEscape when Escape key is pressed and isOpen is false", () => { + renderHook(() => useEscapeKey(false, mockOnEscape)) + + const event = new KeyboardEvent("keydown", { key: "Escape" }) + window.dispatchEvent(event) + + expect(mockOnEscape).not.toHaveBeenCalled() + }) + + it("should not call onEscape when a different key is pressed", () => { + renderHook(() => useEscapeKey(true, mockOnEscape)) + + const event = new KeyboardEvent("keydown", { key: "Enter" }) + window.dispatchEvent(event) + + expect(mockOnEscape).not.toHaveBeenCalled() + }) + + it("should prevent default when preventDefault option is true", () => { + renderHook(() => useEscapeKey(true, mockOnEscape, { preventDefault: true })) + + const event = new KeyboardEvent("keydown", { key: "Escape" }) + const preventDefaultSpy = vi.spyOn(event, "preventDefault") + window.dispatchEvent(event) + + expect(preventDefaultSpy).toHaveBeenCalled() + }) + + it("should not prevent default when preventDefault option is false", () => { + renderHook(() => useEscapeKey(true, mockOnEscape, { preventDefault: false })) + + const event = new KeyboardEvent("keydown", { key: "Escape" }) + const preventDefaultSpy = vi.spyOn(event, "preventDefault") + window.dispatchEvent(event) + + expect(preventDefaultSpy).not.toHaveBeenCalled() + }) + + it("should stop propagation when stopPropagation option is true", () => { + renderHook(() => useEscapeKey(true, mockOnEscape, { stopPropagation: true })) + + const event = new KeyboardEvent("keydown", { key: "Escape" }) + const stopPropagationSpy = vi.spyOn(event, "stopPropagation") + window.dispatchEvent(event) + + expect(stopPropagationSpy).toHaveBeenCalled() + }) + + it("should not stop propagation when stopPropagation option is false", () => { + renderHook(() => useEscapeKey(true, mockOnEscape, { stopPropagation: false })) + + const event = new KeyboardEvent("keydown", { key: "Escape" }) + const stopPropagationSpy = vi.spyOn(event, "stopPropagation") + window.dispatchEvent(event) + + expect(stopPropagationSpy).not.toHaveBeenCalled() + }) + + it("should remove event listener on unmount", () => { + const addEventListenerSpy = vi.spyOn(window, "addEventListener") + const removeEventListenerSpy = vi.spyOn(window, "removeEventListener") + + const { unmount } = renderHook(() => useEscapeKey(true, mockOnEscape)) + + expect(addEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function)) + + unmount() + + expect(removeEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function)) + }) + + it("should always add event listener regardless of isOpen state", () => { + const addEventListenerSpy = vi.spyOn(window, "addEventListener") + const removeEventListenerSpy = vi.spyOn(window, "removeEventListener") + + // Test with isOpen = false + const { rerender } = renderHook(({ isOpen }) => useEscapeKey(isOpen, mockOnEscape), { + initialProps: { isOpen: false }, + }) + + expect(addEventListenerSpy).toHaveBeenCalledTimes(1) + + // Change to isOpen = true + rerender({ isOpen: true }) + + // The listener is re-added because handleKeyDown changes when isOpen changes + // This is expected behavior - the old listener is removed and a new one is added + expect(addEventListenerSpy).toHaveBeenCalledTimes(2) + expect(removeEventListenerSpy).toHaveBeenCalledTimes(1) + }) + + it("should handle rapid isOpen state changes without memory leaks", () => { + const addEventListenerSpy = vi.spyOn(window, "addEventListener") + const removeEventListenerSpy = vi.spyOn(window, "removeEventListener") + + const { rerender, unmount } = renderHook(({ isOpen }) => useEscapeKey(isOpen, mockOnEscape), { + initialProps: { isOpen: false }, + }) + + // Initial render + expect(addEventListenerSpy).toHaveBeenCalledTimes(1) + + // Rapid state changes + rerender({ isOpen: true }) + rerender({ isOpen: false }) + rerender({ isOpen: true }) + + // Each rerender causes the effect to re-run because handleKeyDown changes + expect(addEventListenerSpy).toHaveBeenCalledTimes(4) + // Each re-run also removes the previous listener + expect(removeEventListenerSpy).toHaveBeenCalledTimes(3) + + // Unmount while isOpen is true + unmount() + + // Should properly clean up the final listener + expect(removeEventListenerSpy).toHaveBeenCalledTimes(4) + + // Verify that all listeners were properly cleaned up + expect(addEventListenerSpy).toHaveBeenCalledTimes(removeEventListenerSpy.mock.calls.length) + }) + + it("should update callback when dependencies change", () => { + const { rerender } = renderHook(({ isOpen, onEscape }) => useEscapeKey(isOpen, onEscape), { + initialProps: { isOpen: true, onEscape: mockOnEscape }, + }) + + const event = new KeyboardEvent("keydown", { key: "Escape" }) + window.dispatchEvent(event) + + expect(mockOnEscape).toHaveBeenCalledTimes(1) + + // Change the callback + const newMockOnEscape = vi.fn() + rerender({ isOpen: true, onEscape: newMockOnEscape }) + + window.dispatchEvent(event) + + // Old callback should not be called again + expect(mockOnEscape).toHaveBeenCalledTimes(1) + // New callback should be called + expect(newMockOnEscape).toHaveBeenCalledTimes(1) + }) +}) diff --git a/webview-ui/src/hooks/useEscapeKey.ts b/webview-ui/src/hooks/useEscapeKey.ts new file mode 100644 index 0000000000..9ca3bc206e --- /dev/null +++ b/webview-ui/src/hooks/useEscapeKey.ts @@ -0,0 +1,44 @@ +import { useEffect, useCallback } from "react" + +/** + * Custom hook for handling ESC key press events + * @param isOpen - Whether the component is currently open/visible + * @param onEscape - Callback function to execute when ESC is pressed + * @param options - Additional options for the hook + */ +export function useEscapeKey( + isOpen: boolean, + onEscape: () => void, + options: { + preventDefault?: boolean + stopPropagation?: boolean + } = {}, +) { + const { preventDefault = true, stopPropagation = true } = options + + const handleKeyDown = useCallback( + (event: KeyboardEvent) => { + // Check isOpen inside the handler to ensure proper cleanup + if (event.key === "Escape" && isOpen) { + if (preventDefault) { + event.preventDefault() + } + if (stopPropagation) { + event.stopPropagation() + } + onEscape() + } + }, + [isOpen, onEscape, preventDefault, stopPropagation], + ) + + useEffect(() => { + // Always add the event listener to ensure proper cleanup on unmount + // The isOpen check is now inside the handler + window.addEventListener("keydown", handleKeyDown) + + return () => { + window.removeEventListener("keydown", handleKeyDown) + } + }, [handleKeyDown]) +}