Skip to content

Commit 31fd2b9

Browse files
committed
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
1 parent 67d9d14 commit 31fd2b9

File tree

4 files changed

+383
-39
lines changed

4 files changed

+383
-39
lines changed

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

Lines changed: 27 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -1,22 +1,9 @@
1-
import { useEffect, useCallback, useMemo } from "react"
1+
import { useEffect, useCallback, useMemo, useRef } from "react"
22
import { useExtensionState } from "@src/context/ExtensionStateContext"
33
import { vscode } from "@src/utils/vscode"
44
import { AutoApproveSetting } from "../settings/AutoApproveToggle"
55
import { useAutoApprovalToggles } from "@src/hooks/useAutoApprovalToggles"
6-
7-
// Keyboard shortcuts mapping for auto-approve options
8-
const KEYBOARD_SHORTCUTS: Record<string, AutoApproveSetting> = {
9-
"1": "alwaysAllowReadOnly",
10-
"2": "alwaysAllowWrite",
11-
"3": "alwaysAllowBrowser",
12-
"4": "alwaysAllowExecute",
13-
"5": "alwaysAllowMcp",
14-
"6": "alwaysAllowModeSwitch",
15-
"7": "alwaysAllowSubtasks",
16-
"8": "alwaysAllowFollowupQuestions",
17-
"9": "alwaysAllowUpdateTodoList",
18-
"0": "alwaysApproveResubmit",
19-
}
6+
import { KEYBOARD_SHORTCUTS, DEFAULT_KEYBOARD_CONFIG } from "@src/constants/autoApproveConstants"
207

218
export const AutoApproveKeyboardShortcuts = () => {
229
const {
@@ -99,23 +86,39 @@ export const AutoApproveKeyboardShortcuts = () => {
9986
],
10087
)
10188

89+
// Store the handleToggle function in a ref to avoid re-registrations
90+
const handleToggleRef = useRef(handleToggle)
10291
useEffect(() => {
103-
const handleKeyDown = (event: KeyboardEvent) => {
104-
// Check if Alt/Option key is pressed along with a number key
105-
if (event.altKey && !event.ctrlKey && !event.metaKey && !event.shiftKey) {
106-
const shortcut = KEYBOARD_SHORTCUTS[event.key]
107-
if (shortcut) {
108-
event.preventDefault()
109-
handleToggle(shortcut)
110-
}
92+
handleToggleRef.current = handleToggle
93+
}, [handleToggle])
94+
95+
// Stable event handler that uses the ref
96+
const handleKeyDown = useCallback((event: KeyboardEvent) => {
97+
// Check if keyboard shortcuts are enabled
98+
if (!DEFAULT_KEYBOARD_CONFIG.enabled) {
99+
return
100+
}
101+
102+
// Support both Alt key and Ctrl+Shift key combinations based on configuration
103+
const isValidModifier = DEFAULT_KEYBOARD_CONFIG.useCtrlShiftKey
104+
? event.ctrlKey && event.shiftKey && !event.altKey && !event.metaKey
105+
: event.altKey && !event.ctrlKey && !event.metaKey && !event.shiftKey
106+
107+
if (isValidModifier) {
108+
const shortcut = KEYBOARD_SHORTCUTS[event.key]
109+
if (shortcut) {
110+
event.preventDefault()
111+
handleToggleRef.current(shortcut)
111112
}
112113
}
114+
}, [])
113115

116+
useEffect(() => {
114117
window.addEventListener("keydown", handleKeyDown)
115118
return () => {
116119
window.removeEventListener("keydown", handleKeyDown)
117120
}
118-
}, [handleToggle])
121+
}, [handleKeyDown])
119122

120123
return null // This component doesn't render anything
121124
}

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

Lines changed: 9 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ import { useAppTranslation } from "@/i18n/TranslationContext"
33
import { cn } from "@/lib/utils"
44
import { StandardTooltip } from "@/components/ui"
55
import { autoApproveSettingsConfig, AutoApproveSetting } from "../settings/AutoApproveToggle"
6+
import { KEYBOARD_SHORTCUTS_DISPLAY, DEFAULT_KEYBOARD_CONFIG } from "@/constants/autoApproveConstants"
67

78
type AutoApproveToggles = Pick<
89
GlobalSettings,
@@ -22,20 +23,6 @@ type AutoApproveToggleDropdownProps = AutoApproveToggles & {
2223
onToggle: (key: AutoApproveSetting, value: boolean) => void
2324
}
2425

25-
// Keyboard shortcuts mapping
26-
const KEYBOARD_SHORTCUTS: Record<AutoApproveSetting, string> = {
27-
alwaysAllowReadOnly: "Alt+1",
28-
alwaysAllowWrite: "Alt+2",
29-
alwaysAllowBrowser: "Alt+3",
30-
alwaysAllowExecute: "Alt+4",
31-
alwaysAllowMcp: "Alt+5",
32-
alwaysAllowModeSwitch: "Alt+6",
33-
alwaysAllowSubtasks: "Alt+7",
34-
alwaysAllowFollowupQuestions: "Alt+8",
35-
alwaysAllowUpdateTodoList: "Alt+9",
36-
alwaysApproveResubmit: "Alt+0",
37-
}
38-
3926
export const AutoApproveToggleDropdown = ({ onToggle, ...props }: AutoApproveToggleDropdownProps) => {
4027
const { t } = useAppTranslation()
4128

@@ -52,7 +39,14 @@ export const AutoApproveToggleDropdown = ({ onToggle, ...props }: AutoApproveTog
5239
icon,
5340
testId,
5441
}: (typeof autoApproveSettingsConfig)[AutoApproveSetting]) => {
55-
const tooltipContent = `${t(descriptionKey || "")} (${KEYBOARD_SHORTCUTS[key]})`
42+
// Get the appropriate keyboard shortcut display based on configuration
43+
const shortcutDisplay = DEFAULT_KEYBOARD_CONFIG.useCtrlShiftKey
44+
? KEYBOARD_SHORTCUTS_DISPLAY[key].replace("Alt+", "Ctrl+Shift+")
45+
: KEYBOARD_SHORTCUTS_DISPLAY[key]
46+
47+
const tooltipContent = DEFAULT_KEYBOARD_CONFIG.enabled
48+
? `${t(descriptionKey || "")} (${shortcutDisplay})`
49+
: t(descriptionKey || "")
5650
return (
5751
<StandardTooltip key={key} content={tooltipContent}>
5852
<button

0 commit comments

Comments
 (0)