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
This commit is contained in:
Roo Code 2025-07-22 09:29:04 +00:00
parent df6c57d293
commit cdb9e41cee
2 changed files with 106 additions and 5 deletions

View file

@ -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>

View file

@ -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()
})
})
})