From cdb9e41cee75f7c0d6f7b4ad1b1a1e6c97a21b73 Mon Sep 17 00:00:00 2001
From: Roo Code <roomote@roocode.com>
Date: Tue, 22 Jul 2025 09:29:04 +0000
Subject: [PATCH] fix: keep auto-approve checkbox responsive when menu is
 expanded

- Remove isExpanded from checkbox disabled condition to allow toggling while menu is open
- Wrap onChange handler in useCallback to prevent unnecessary re-renders
- Add tests to verify checkbox remains responsive when menu is expanded
- Add test to verify no flickering during rapid state changes

This fixes the critical safety issue where users could not disable auto-approval
during model responses when they realized unintended actions were about to occur.

Fixes #6060
---
 .../src/components/chat/AutoApproveMenu.tsx   |  11 +-
 .../chat/__tests__/AutoApproveMenu.spec.tsx   | 100 ++++++++++++++++++
 2 files changed, 106 insertions(+), 5 deletions(-)

diff --git a/webview-ui/src/components/chat/AutoApproveMenu.tsx b/webview-ui/src/components/chat/AutoApproveMenu.tsx
index 2e987b7c497..9a0b7677e61 100644
--- a/webview-ui/src/components/chat/AutoApproveMenu.tsx
+++ b/webview-ui/src/components/chat/AutoApproveMenu.tsx
@@ -129,10 +129,11 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
 		setIsExpanded((prev) => !prev)
 	}, [])
 
-	// Disable main checkbox while menu is open or no options selected
+	// Disable main checkbox only when no options are selected
+	// Allow toggling even when menu is expanded for better UX and safety
 	const isCheckboxDisabled = useMemo(() => {
-		return !hasEnabledOptions || isExpanded
-	}, [hasEnabledOptions, isExpanded])
+		return !hasEnabledOptions
+	}, [hasEnabledOptions])
 
 	const enabledActionsList = Object.entries(toggles)
 		.filter(([_key, value]) => !!value)
@@ -184,14 +185,14 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
 									? t("chat:autoApprove.toggleAriaLabel")
 									: t("chat:autoApprove.disabledAriaLabel")
 							}
-							onChange={() => {
+							onChange={useCallback(() => {
 								if (hasEnabledOptions) {
 									const newValue = !(autoApprovalEnabled ?? false)
 									setAutoApprovalEnabled(newValue)
 									vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue })
 								}
 								// If no options enabled, do nothing
-							}}
+							}, [hasEnabledOptions, autoApprovalEnabled, setAutoApprovalEnabled])}
 						/>
 					</StandardTooltip>
 				</div>
diff --git a/webview-ui/src/components/chat/__tests__/AutoApproveMenu.spec.tsx b/webview-ui/src/components/chat/__tests__/AutoApproveMenu.spec.tsx
index 185e5eeec6e..9c98a1d839e 100644
--- a/webview-ui/src/components/chat/__tests__/AutoApproveMenu.spec.tsx
+++ b/webview-ui/src/components/chat/__tests__/AutoApproveMenu.spec.tsx
@@ -13,6 +13,35 @@ vi.mock("@src/utils/vscode", () => ({
 // Mock ExtensionStateContext
 vi.mock("@src/context/ExtensionStateContext")
 
+// Mock VSCode webview UI toolkit
+vi.mock("@vscode/webview-ui-toolkit/react", () => ({
+	VSCodeCheckbox: ({ children, checked, disabled, onChange, ...props }: any) => (
+		<label>
+			<input
+				type="checkbox"
+				role="checkbox"
+				checked={checked}
+				disabled={disabled}
+				{...props}
+				onChange={(e: any) => {
+					if (!disabled && onChange) {
+						onChange(e)
+					}
+				}}
+			/>
+			{children}
+		</label>
+	),
+	VSCodeLink: ({ children, onClick, ...props }: any) => (
+		<a onClick={onClick} {...props}>
+			{children}
+		</a>
+	),
+	VSCodeTextField: ({ value, onInput, ...props }: any) => (
+		<input type="text" value={value} onInput={onInput} {...props} />
+	),
+}))
+
 // Mock translation hook
 vi.mock("@src/i18n/TranslationContext", () => ({
 	useAppTranslation: () => ({
@@ -303,5 +332,76 @@ describe("AutoApproveMenu", () => {
 				bool: false,
 			})
 		})
+
+		it("should keep master checkbox responsive when menu is expanded", async () => {
+			const mockSetAutoApprovalEnabled = vi.fn()
+
+			;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue({
+				...defaultExtensionState,
+				autoApprovalEnabled: true,
+				alwaysAllowReadOnly: true,
+				setAutoApprovalEnabled: mockSetAutoApprovalEnabled,
+			})
+
+			render(<AutoApproveMenu />)
+
+			// Expand the menu
+			const menuContainer = screen.getByText("Auto-approve").parentElement
+			fireEvent.click(menuContainer!)
+
+			// Wait for the menu to expand
+			await waitFor(() => {
+				expect(screen.getByTestId("always-allow-readonly-toggle")).toBeInTheDocument()
+			})
+
+			// The master checkbox should still be enabled and clickable
+			const masterCheckbox = screen.getByRole("checkbox")
+			expect(masterCheckbox).not.toBeDisabled()
+
+			// Click the master checkbox while menu is expanded
+			fireEvent.click(masterCheckbox)
+
+			// Should successfully toggle the master checkbox
+			expect(mockPostMessage).toHaveBeenCalledWith({
+				type: "autoApprovalEnabled",
+				bool: false,
+			})
+			expect(mockSetAutoApprovalEnabled).toHaveBeenCalledWith(false)
+		})
+
+		it("should not flicker when state changes rapidly", async () => {
+			const mockSetAutoApprovalEnabled = vi.fn()
+
+			// Start with enabled state
+			const enabledState = {
+				...defaultExtensionState,
+				autoApprovalEnabled: true,
+				alwaysAllowReadOnly: true,
+				setAutoApprovalEnabled: mockSetAutoApprovalEnabled,
+			}
+
+			;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue(enabledState)
+
+			const { rerender } = render(<AutoApproveMenu />)
+
+			const masterCheckbox = screen.getByRole("checkbox")
+			expect(masterCheckbox).toBeChecked()
+
+			// Simulate rapid state changes
+			for (let i = 0; i < 5; i++) {
+				const newState = {
+					...defaultExtensionState,
+					autoApprovalEnabled: i % 2 === 0,
+					alwaysAllowReadOnly: true,
+					setAutoApprovalEnabled: mockSetAutoApprovalEnabled,
+				}
+				;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue(newState)
+				rerender(<AutoApproveMenu />)
+			}
+
+			// Checkbox should reflect the final state without issues (i=4, so 4 % 2 === 0, which is true)
+			expect(masterCheckbox).toBeChecked()
+			expect(masterCheckbox).not.toBeDisabled()
+		})
 	})
 })