Skip to content

Commit cdb9e41

Browse files
committed
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
1 parent df6c57d commit cdb9e41

File tree

2 files changed

+106
-5
lines changed

2 files changed

+106
-5
lines changed

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

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -129,10 +129,11 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
129129
setIsExpanded((prev) => !prev)
130130
}, [])
131131

132-
// Disable main checkbox while menu is open or no options selected
132+
// Disable main checkbox only when no options are selected
133+
// Allow toggling even when menu is expanded for better UX and safety
133134
const isCheckboxDisabled = useMemo(() => {
134-
return !hasEnabledOptions || isExpanded
135-
}, [hasEnabledOptions, isExpanded])
135+
return !hasEnabledOptions
136+
}, [hasEnabledOptions])
136137

137138
const enabledActionsList = Object.entries(toggles)
138139
.filter(([_key, value]) => !!value)
@@ -184,14 +185,14 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
184185
? t("chat:autoApprove.toggleAriaLabel")
185186
: t("chat:autoApprove.disabledAriaLabel")
186187
}
187-
onChange={() => {
188+
onChange={useCallback(() => {
188189
if (hasEnabledOptions) {
189190
const newValue = !(autoApprovalEnabled ?? false)
190191
setAutoApprovalEnabled(newValue)
191192
vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue })
192193
}
193194
// If no options enabled, do nothing
194-
}}
195+
}, [hasEnabledOptions, autoApprovalEnabled, setAutoApprovalEnabled])}
195196
/>
196197
</StandardTooltip>
197198
</div>

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

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,35 @@ vi.mock("@src/utils/vscode", () => ({
1313
// Mock ExtensionStateContext
1414
vi.mock("@src/context/ExtensionStateContext")
1515

16+
// Mock VSCode webview UI toolkit
17+
vi.mock("@vscode/webview-ui-toolkit/react", () => ({
18+
VSCodeCheckbox: ({ children, checked, disabled, onChange, ...props }: any) => (
19+
<label>
20+
<input
21+
type="checkbox"
22+
role="checkbox"
23+
checked={checked}
24+
disabled={disabled}
25+
{...props}
26+
onChange={(e: any) => {
27+
if (!disabled && onChange) {
28+
onChange(e)
29+
}
30+
}}
31+
/>
32+
{children}
33+
</label>
34+
),
35+
VSCodeLink: ({ children, onClick, ...props }: any) => (
36+
<a onClick={onClick} {...props}>
37+
{children}
38+
</a>
39+
),
40+
VSCodeTextField: ({ value, onInput, ...props }: any) => (
41+
<input type="text" value={value} onInput={onInput} {...props} />
42+
),
43+
}))
44+
1645
// Mock translation hook
1746
vi.mock("@src/i18n/TranslationContext", () => ({
1847
useAppTranslation: () => ({
@@ -303,5 +332,76 @@ describe("AutoApproveMenu", () => {
303332
bool: false,
304333
})
305334
})
335+
336+
it("should keep master checkbox responsive when menu is expanded", async () => {
337+
const mockSetAutoApprovalEnabled = vi.fn()
338+
339+
;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue({
340+
...defaultExtensionState,
341+
autoApprovalEnabled: true,
342+
alwaysAllowReadOnly: true,
343+
setAutoApprovalEnabled: mockSetAutoApprovalEnabled,
344+
})
345+
346+
render(<AutoApproveMenu />)
347+
348+
// Expand the menu
349+
const menuContainer = screen.getByText("Auto-approve").parentElement
350+
fireEvent.click(menuContainer!)
351+
352+
// Wait for the menu to expand
353+
await waitFor(() => {
354+
expect(screen.getByTestId("always-allow-readonly-toggle")).toBeInTheDocument()
355+
})
356+
357+
// The master checkbox should still be enabled and clickable
358+
const masterCheckbox = screen.getByRole("checkbox")
359+
expect(masterCheckbox).not.toBeDisabled()
360+
361+
// Click the master checkbox while menu is expanded
362+
fireEvent.click(masterCheckbox)
363+
364+
// Should successfully toggle the master checkbox
365+
expect(mockPostMessage).toHaveBeenCalledWith({
366+
type: "autoApprovalEnabled",
367+
bool: false,
368+
})
369+
expect(mockSetAutoApprovalEnabled).toHaveBeenCalledWith(false)
370+
})
371+
372+
it("should not flicker when state changes rapidly", async () => {
373+
const mockSetAutoApprovalEnabled = vi.fn()
374+
375+
// Start with enabled state
376+
const enabledState = {
377+
...defaultExtensionState,
378+
autoApprovalEnabled: true,
379+
alwaysAllowReadOnly: true,
380+
setAutoApprovalEnabled: mockSetAutoApprovalEnabled,
381+
}
382+
383+
;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue(enabledState)
384+
385+
const { rerender } = render(<AutoApproveMenu />)
386+
387+
const masterCheckbox = screen.getByRole("checkbox")
388+
expect(masterCheckbox).toBeChecked()
389+
390+
// Simulate rapid state changes
391+
for (let i = 0; i < 5; i++) {
392+
const newState = {
393+
...defaultExtensionState,
394+
autoApprovalEnabled: i % 2 === 0,
395+
alwaysAllowReadOnly: true,
396+
setAutoApprovalEnabled: mockSetAutoApprovalEnabled,
397+
}
398+
;(useExtensionState as ReturnType<typeof vi.fn>).mockReturnValue(newState)
399+
rerender(<AutoApproveMenu />)
400+
}
401+
402+
// Checkbox should reflect the final state without issues (i=4, so 4 % 2 === 0, which is true)
403+
expect(masterCheckbox).toBeChecked()
404+
expect(masterCheckbox).not.toBeDisabled()
405+
})
306406
})
307407
})

0 commit comments

Comments
 (0)