refactor: address PR review feedback for auto-approve keyboard shortcuts

- Extract KEYBOARD_SHORTCUTS to shared constants file to avoid duplication
- Fix event listener cleanup using useRef and stable callback to prevent memory leaks
- Add configuration support for keyboard shortcuts (enabled/disabled, Alt vs Ctrl+Shift)
- Add comprehensive tests for keyboard shortcut functionality
- Improve code organization and maintainability

webview-ui/src/components/chat/AutoApproveKeyboardShortcuts.tsx

@@ -1,22 +1,9 @@
-import { useEffect, useCallback, useMemo } from "react"
+import { useEffect, useCallback, useMemo, useRef } from "react"
 import { useExtensionState } from "@src/context/ExtensionStateContext"
 import { vscode } from "@src/utils/vscode"
 import { AutoApproveSetting } from "../settings/AutoApproveToggle"
 import { useAutoApprovalToggles } from "@src/hooks/useAutoApprovalToggles"
-
-// Keyboard shortcuts mapping for auto-approve options
-const KEYBOARD_SHORTCUTS: Record<string, AutoApproveSetting> = {
-	"1": "alwaysAllowReadOnly",
-	"2": "alwaysAllowWrite",
-	"3": "alwaysAllowBrowser",
-	"4": "alwaysAllowExecute",
-	"5": "alwaysAllowMcp",
-	"6": "alwaysAllowModeSwitch",
-	"7": "alwaysAllowSubtasks",
-	"8": "alwaysAllowFollowupQuestions",
-	"9": "alwaysAllowUpdateTodoList",
-	"0": "alwaysApproveResubmit",
-}
+import { KEYBOARD_SHORTCUTS, DEFAULT_KEYBOARD_CONFIG } from "@src/constants/autoApproveConstants"
 
 export const AutoApproveKeyboardShortcuts = () => {
 	const {
@@ -99,23 +86,39 @@ export const AutoApproveKeyboardShortcuts = () => {
 		],
 	)
 
+	// Store the handleToggle function in a ref to avoid re-registrations
+	const handleToggleRef = useRef(handleToggle)
 	useEffect(() => {
-		const handleKeyDown = (event: KeyboardEvent) => {
-			// Check if Alt/Option key is pressed along with a number key
-			if (event.altKey && !event.ctrlKey && !event.metaKey && !event.shiftKey) {
-				const shortcut = KEYBOARD_SHORTCUTS[event.key]
-				if (shortcut) {
-					event.preventDefault()
-					handleToggle(shortcut)
-				}
-			}
+		handleToggleRef.current = handleToggle
+	}, [handleToggle])
+
+	// Stable event handler that uses the ref
+	const handleKeyDown = useCallback((event: KeyboardEvent) => {
+		// Check if keyboard shortcuts are enabled
+		if (!DEFAULT_KEYBOARD_CONFIG.enabled) {
+			return
 		}
 
+		// Support both Alt key and Ctrl+Shift key combinations based on configuration
+		const isValidModifier = DEFAULT_KEYBOARD_CONFIG.useCtrlShiftKey
+			? event.ctrlKey && event.shiftKey && !event.altKey && !event.metaKey
+			: event.altKey && !event.ctrlKey && !event.metaKey && !event.shiftKey
+
+		if (isValidModifier) {
+			const shortcut = KEYBOARD_SHORTCUTS[event.key]
+			if (shortcut) {
+				event.preventDefault()
+				handleToggleRef.current(shortcut)
+			}
+		}
+	}, [])
+
+	useEffect(() => {
 		window.addEventListener("keydown", handleKeyDown)
 		return () => {
 			window.removeEventListener("keydown", handleKeyDown)
 		}
-	}, [handleToggle])
+	}, [handleKeyDown])
 
 	return null // This component doesn't render anything
 }

webview-ui/src/components/chat/AutoApproveToggleDropdown.tsx

@@ -3,6 +3,7 @@ import { useAppTranslation } from "@/i18n/TranslationContext"
 import { cn } from "@/lib/utils"
 import { StandardTooltip } from "@/components/ui"
 import { autoApproveSettingsConfig, AutoApproveSetting } from "../settings/AutoApproveToggle"
+import { KEYBOARD_SHORTCUTS_DISPLAY, DEFAULT_KEYBOARD_CONFIG } from "@/constants/autoApproveConstants"
 
 type AutoApproveToggles = Pick<
 	GlobalSettings,
@@ -22,20 +23,6 @@ type AutoApproveToggleDropdownProps = AutoApproveToggles & {
 	onToggle: (key: AutoApproveSetting, value: boolean) => void
 }
 
-// Keyboard shortcuts mapping
-const KEYBOARD_SHORTCUTS: Record<AutoApproveSetting, string> = {
-	alwaysAllowReadOnly: "Alt+1",
-	alwaysAllowWrite: "Alt+2",
-	alwaysAllowBrowser: "Alt+3",
-	alwaysAllowExecute: "Alt+4",
-	alwaysAllowMcp: "Alt+5",
-	alwaysAllowModeSwitch: "Alt+6",
-	alwaysAllowSubtasks: "Alt+7",
-	alwaysAllowFollowupQuestions: "Alt+8",
-	alwaysAllowUpdateTodoList: "Alt+9",
-	alwaysApproveResubmit: "Alt+0",
-}
-
 export const AutoApproveToggleDropdown = ({ onToggle, ...props }: AutoApproveToggleDropdownProps) => {
 	const { t } = useAppTranslation()
 
@@ -52,7 +39,14 @@ export const AutoApproveToggleDropdown = ({ onToggle, ...props }: AutoApproveTog
 		icon,
 		testId,
 	}: (typeof autoApproveSettingsConfig)[AutoApproveSetting]) => {
-		const tooltipContent = `${t(descriptionKey || "")} (${KEYBOARD_SHORTCUTS[key]})`
+		// Get the appropriate keyboard shortcut display based on configuration
+		const shortcutDisplay = DEFAULT_KEYBOARD_CONFIG.useCtrlShiftKey
+			? KEYBOARD_SHORTCUTS_DISPLAY[key].replace("Alt+", "Ctrl+Shift+")
+			: KEYBOARD_SHORTCUTS_DISPLAY[key]
+
+		const tooltipContent = DEFAULT_KEYBOARD_CONFIG.enabled
+			? `${t(descriptionKey || "")} (${shortcutDisplay})`
+			: t(descriptionKey || "")
 		return (
 			<StandardTooltip key={key} content={tooltipContent}>
 				<button

webview-ui/src/components/chat/__tests__/AutoApproveKeyboardShortcuts.spec.tsx

@@ -0,0 +1,292 @@
+import { render, fireEvent, waitFor } from "@/utils/test-utils"
+import { useExtensionState } from "@src/context/ExtensionStateContext"
+import { vscode } from "@src/utils/vscode"
+import { AutoApproveKeyboardShortcuts } from "../AutoApproveKeyboardShortcuts"
+import { DEFAULT_KEYBOARD_CONFIG } from "@src/constants/autoApproveConstants"
+
+// Mock vscode API
+vi.mock("@src/utils/vscode", () => ({
+	vscode: {
+		postMessage: vi.fn(),
+	},
+}))
+
+// Mock ExtensionStateContext
+vi.mock("@src/context/ExtensionStateContext")
+
+// Mock the constants to control keyboard config
+vi.mock("@src/constants/autoApproveConstants", () => ({
+	KEYBOARD_SHORTCUTS: {
+		"1": "alwaysAllowReadOnly",
+		"2": "alwaysAllowWrite",
+		"3": "alwaysAllowBrowser",
+		"4": "alwaysAllowExecute",
+		"5": "alwaysAllowMcp",
+		"6": "alwaysAllowModeSwitch",
+		"7": "alwaysAllowSubtasks",
+		"8": "alwaysAllowFollowupQuestions",
+		"9": "alwaysAllowUpdateTodoList",
+		"0": "alwaysApproveResubmit",
+	},
+	KEYBOARD_SHORTCUTS_DISPLAY: {
+		alwaysAllowReadOnly: "Alt+1",
+		alwaysAllowWrite: "Alt+2",
+		alwaysAllowBrowser: "Alt+3",
+		alwaysAllowExecute: "Alt+4",
+		alwaysAllowMcp: "Alt+5",
+		alwaysAllowModeSwitch: "Alt+6",
+		alwaysAllowSubtasks: "Alt+7",
+		alwaysAllowFollowupQuestions: "Alt+8",
+		alwaysAllowUpdateTodoList: "Alt+9",
+		alwaysApproveResubmit: "Alt+0",
+	},
+	DEFAULT_KEYBOARD_CONFIG: {
+		enabled: true,
+		useAltKey: true,
+		useCtrlShiftKey: false,
+	},
+}))
+
+// Get the mocked postMessage function
+const mockPostMessage = vscode.postMessage as ReturnType<typeof vi.fn>
+
+describe("AutoApproveKeyboardShortcuts", () => {
+	const defaultExtensionState = {
+		autoApprovalEnabled: true,
+		alwaysAllowReadOnly: false,
+		alwaysAllowReadOnlyOutsideWorkspace: false,
+		alwaysAllowWrite: false,
+		alwaysAllowWriteOutsideWorkspace: false,
+		alwaysAllowExecute: false,
+		alwaysAllowBrowser: false,
+		alwaysAllowMcp: false,
+		alwaysAllowModeSwitch: false,
+		alwaysAllowSubtasks: false,
+		alwaysApproveResubmit: false,
+		alwaysAllowFollowupQuestions: false,
+		alwaysAllowUpdateTodoList: false,
+		writeDelayMs: 3000,
+		allowedMaxRequests: undefined,
+		setAutoApprovalEnabled: vi.fn(),
+		setAlwaysAllowReadOnly: vi.fn(),
+		setAlwaysAllowWrite: vi.fn(),
+		setAlwaysAllowExecute: vi.fn(),
+		setAlwaysAllowBrowser: vi.fn(),
+		setAlwaysAllowMcp: vi.fn(),
+		setAlwaysAllowModeSwitch: vi.fn(),
+		setAlwaysAllowSubtasks: vi.fn(),
+		setAlwaysApproveResubmit: vi.fn(),
+		setAlwaysAllowFollowupQuestions: vi.fn(),
+		setAlwaysAllowUpdateTodoList: vi.fn(),
+		setAllowedMaxRequests: vi.fn(),
+	}
+
+	beforeEach(() => {
+		vi.clearAllMocks()
+		;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue(defaultExtensionState)
+	})
+
+	describe("Keyboard shortcut handling", () => {
+		it("should toggle read-only with Alt+1", async () => {
+			const mockSetAlwaysAllowReadOnly = vi.fn()
+			;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue({
+				...defaultExtensionState,
+				setAlwaysAllowReadOnly: mockSetAlwaysAllowReadOnly,
+			})
+
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Alt+1 keypress
+			fireEvent.keyDown(window, { key: "1", altKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).toHaveBeenCalledWith({
+					type: "alwaysAllowReadOnly",
+					bool: true,
+				})
+				expect(mockSetAlwaysAllowReadOnly).toHaveBeenCalledWith(true)
+			})
+		})
+
+		it("should toggle write with Alt+2", async () => {
+			const mockSetAlwaysAllowWrite = vi.fn()
+			;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue({
+				...defaultExtensionState,
+				setAlwaysAllowWrite: mockSetAlwaysAllowWrite,
+			})
+
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Alt+2 keypress
+			fireEvent.keyDown(window, { key: "2", altKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).toHaveBeenCalledWith({
+					type: "alwaysAllowWrite",
+					bool: true,
+				})
+				expect(mockSetAlwaysAllowWrite).toHaveBeenCalledWith(true)
+			})
+		})
+
+		it("should toggle resubmit with Alt+0", async () => {
+			const mockSetAlwaysApproveResubmit = vi.fn()
+			;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue({
+				...defaultExtensionState,
+				setAlwaysApproveResubmit: mockSetAlwaysApproveResubmit,
+			})
+
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Alt+0 keypress
+			fireEvent.keyDown(window, { key: "0", altKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).toHaveBeenCalledWith({
+					type: "alwaysApproveResubmit",
+					bool: true,
+				})
+				expect(mockSetAlwaysApproveResubmit).toHaveBeenCalledWith(true)
+			})
+		})
+
+		it("should not trigger with Ctrl+1", async () => {
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Ctrl+1 keypress (should not trigger)
+			fireEvent.keyDown(window, { key: "1", ctrlKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).not.toHaveBeenCalled()
+			})
+		})
+
+		it("should not trigger with Shift+1", async () => {
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Shift+1 keypress (should not trigger)
+			fireEvent.keyDown(window, { key: "1", shiftKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).not.toHaveBeenCalled()
+			})
+		})
+
+		it("should not trigger with Meta+1", async () => {
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Meta+1 keypress (should not trigger)
+			fireEvent.keyDown(window, { key: "1", metaKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).not.toHaveBeenCalled()
+			})
+		})
+
+		it("should not trigger with Alt+Ctrl+1", async () => {
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Alt+Ctrl+1 keypress (should not trigger)
+			fireEvent.keyDown(window, { key: "1", altKey: true, ctrlKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).not.toHaveBeenCalled()
+			})
+		})
+
+		it("should toggle off when already enabled", async () => {
+			const mockSetAlwaysAllowReadOnly = vi.fn()
+			;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue({
+				...defaultExtensionState,
+				alwaysAllowReadOnly: true, // Already enabled
+				setAlwaysAllowReadOnly: mockSetAlwaysAllowReadOnly,
+			})
+
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Alt+1 keypress
+			fireEvent.keyDown(window, { key: "1", altKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).toHaveBeenCalledWith({
+					type: "alwaysAllowReadOnly",
+					bool: false, // Should toggle off
+				})
+				expect(mockSetAlwaysAllowReadOnly).toHaveBeenCalledWith(false)
+			})
+		})
+	})
+
+	describe("Configuration support", () => {
+		it("should not trigger when keyboard shortcuts are disabled", async () => {
+			// Mock the config to disable shortcuts
+			DEFAULT_KEYBOARD_CONFIG.enabled = false
+
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Simulate Alt+1 keypress
+			fireEvent.keyDown(window, { key: "1", altKey: true })
+
+			await waitFor(() => {
+				expect(mockPostMessage).not.toHaveBeenCalled()
+			})
+
+			// Reset config
+			DEFAULT_KEYBOARD_CONFIG.enabled = true
+		})
+
+		it("should use Ctrl+Shift when configured", async () => {
+			// Mock the config to use Ctrl+Shift
+			DEFAULT_KEYBOARD_CONFIG.useAltKey = false
+			DEFAULT_KEYBOARD_CONFIG.useCtrlShiftKey = true
+
+			const mockSetAlwaysAllowReadOnly = vi.fn()
+			;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue({
+				...defaultExtensionState,
+				setAlwaysAllowReadOnly: mockSetAlwaysAllowReadOnly,
+			})
+
+			render(<AutoApproveKeyboardShortcuts />)
+
+			// Alt+1 should not work
+			fireEvent.keyDown(window, { key: "1", altKey: true })
+			await waitFor(() => {
+				expect(mockPostMessage).not.toHaveBeenCalled()
+			})
+
+			// Ctrl+Shift+1 should work
+			fireEvent.keyDown(window, { key: "1", ctrlKey: true, shiftKey: true })
+			await waitFor(() => {
+				expect(mockPostMessage).toHaveBeenCalledWith({
+					type: "alwaysAllowReadOnly",
+					bool: true,
+				})
+			})
+
+			// Reset config
+			DEFAULT_KEYBOARD_CONFIG.useAltKey = true
+			DEFAULT_KEYBOARD_CONFIG.useCtrlShiftKey = false
+		})
+	})
+
+	describe("Event listener cleanup", () => {
+		it("should clean up event listeners on unmount", () => {
+			const addEventListenerSpy = vi.spyOn(window, "addEventListener")
+			const removeEventListenerSpy = vi.spyOn(window, "removeEventListener")
+
+			const { unmount } = render(<AutoApproveKeyboardShortcuts />)
+
+			// Check that event listener was added
+			expect(addEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function))
+
+			// Unmount the component
+			unmount()
+
+			// Check that event listener was removed
+			expect(removeEventListenerSpy).toHaveBeenCalledWith("keydown", expect.any(Function))
+
+			addEventListenerSpy.mockRestore()
+			removeEventListenerSpy.mockRestore()
+		})
+	})
+})

webview-ui/src/constants/autoApproveConstants.ts

@@ -0,0 +1,55 @@
+import { AutoApproveSetting } from "../components/settings/AutoApproveToggle"
+
+/**
+ * Keyboard shortcuts mapping for auto-approve options
+ * Maps keyboard keys (1-9, 0) to their corresponding auto-approve settings
+ */
+export const KEYBOARD_SHORTCUTS: Record<string, AutoApproveSetting> = {
+	"1": "alwaysAllowReadOnly",
+	"2": "alwaysAllowWrite",
+	"3": "alwaysAllowBrowser",
+	"4": "alwaysAllowExecute",
+	"5": "alwaysAllowMcp",
+	"6": "alwaysAllowModeSwitch",
+	"7": "alwaysAllowSubtasks",
+	"8": "alwaysAllowFollowupQuestions",
+	"9": "alwaysAllowUpdateTodoList",
+	"0": "alwaysApproveResubmit",
+}
+
+/**
+ * Keyboard shortcuts display mapping
+ * Maps auto-approve settings to their display shortcut strings
+ */
+export const KEYBOARD_SHORTCUTS_DISPLAY: Record<AutoApproveSetting, string> = {
+	alwaysAllowReadOnly: "Alt+1",
+	alwaysAllowWrite: "Alt+2",
+	alwaysAllowBrowser: "Alt+3",
+	alwaysAllowExecute: "Alt+4",
+	alwaysAllowMcp: "Alt+5",
+	alwaysAllowModeSwitch: "Alt+6",
+	alwaysAllowSubtasks: "Alt+7",
+	alwaysAllowFollowupQuestions: "Alt+8",
+	alwaysAllowUpdateTodoList: "Alt+9",
+	alwaysApproveResubmit: "Alt+0",
+}
+
+/**
+ * Configuration for keyboard shortcuts
+ * Can be extended in the future to support user preferences
+ */
+export interface KeyboardShortcutConfig {
+	enabled: boolean
+	useAltKey: boolean
+	useCtrlShiftKey: boolean
+}
+
+/**
+ * Default keyboard shortcut configuration
+ * In the future, this can be loaded from user settings
+ */
+export const DEFAULT_KEYBOARD_CONFIG: KeyboardShortcutConfig = {
+	enabled: true,
+	useAltKey: true,
+	useCtrlShiftKey: false,
+}