Skip to content

Commit 932bf69

Browse files
MnehmosMnehmos
committed
Remove settings page master auto-approve toggle and fix performance issues
- Remove master toggle UI from AutoApproveSettings component - Remove master toggle translation keys from English locale - Update tests to remove master toggle functionality - Fix useDeepCompareEffect warning in ChatView.tsx by using useEffect - Optimize useAutoApproveState hook for better performance: - Improve memoization dependencies - Batch state updates in handleMasterToggle - Optimize callback dependencies - Optimize AutoApproveMenu with memoized setters object - Resolves language check failure in PR #4395
1 parent 273e8ac commit 932bf69

File tree

6 files changed

+72
-148
lines changed

6 files changed

+72
-148
lines changed

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

Lines changed: 29 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -44,14 +44,14 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
4444

4545
const toggles = useMemo(
4646
() => ({
47-
alwaysAllowReadOnly: alwaysAllowReadOnly,
48-
alwaysAllowWrite: alwaysAllowWrite,
49-
alwaysAllowExecute: alwaysAllowExecute,
50-
alwaysAllowBrowser: alwaysAllowBrowser,
51-
alwaysAllowMcp: alwaysAllowMcp,
52-
alwaysAllowModeSwitch: alwaysAllowModeSwitch,
53-
alwaysAllowSubtasks: alwaysAllowSubtasks,
54-
alwaysApproveResubmit: alwaysApproveResubmit,
47+
alwaysAllowReadOnly,
48+
alwaysAllowWrite,
49+
alwaysAllowExecute,
50+
alwaysAllowBrowser,
51+
alwaysAllowMcp,
52+
alwaysAllowModeSwitch,
53+
alwaysAllowSubtasks,
54+
alwaysApproveResubmit,
5555
}),
5656
[
5757
alwaysAllowReadOnly,
@@ -65,10 +65,9 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
6565
],
6666
)
6767

68-
// Use the centralized auto-approve state hook
69-
const { hasAnyAutoApprovedAction, updateAutoApprovalState, handleMasterToggle } = useAutoApproveState({
70-
toggles,
71-
setters: {
68+
// Memoize setters object to prevent recreating it on every render
69+
const setters = useMemo(
70+
() => ({
7271
setAlwaysAllowReadOnly,
7372
setAlwaysAllowWrite,
7473
setAlwaysAllowExecute,
@@ -78,7 +77,24 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
7877
setAlwaysAllowSubtasks,
7978
setAlwaysApproveResubmit,
8079
setAutoApprovalEnabled,
81-
},
80+
}),
81+
[
82+
setAlwaysAllowReadOnly,
83+
setAlwaysAllowWrite,
84+
setAlwaysAllowExecute,
85+
setAlwaysAllowBrowser,
86+
setAlwaysAllowMcp,
87+
setAlwaysAllowModeSwitch,
88+
setAlwaysAllowSubtasks,
89+
setAlwaysApproveResubmit,
90+
setAutoApprovalEnabled,
91+
],
92+
)
93+
94+
// Use the centralized auto-approve state hook
95+
const { hasAnyAutoApprovedAction, updateAutoApprovalState, handleMasterToggle } = useAutoApproveState({
96+
toggles,
97+
setters,
8298
})
8399

84100
const displayedAutoApproveText = useMemo(() => {

‎webview-ui/src/components/chat/ChatView.tsx‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { forwardRef, useCallback, useEffect, useImperativeHandle, useMemo, useRef, useState } from "react"
2-
import { useDeepCompareEffect, useEvent, useMount } from "react-use"
2+
import { useEvent, useMount } from "react-use"
33
import debounce from "debounce"
44
import { Virtuoso, type VirtuosoHandle } from "react-virtuoso"
55
import removeMd from "remove-markdown"
@@ -215,7 +215,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
215215
vscode.postMessage({ type: "playTts", text })
216216
}
217217

218-
useDeepCompareEffect(() => {
218+
useEffect(() => {
219219
// if last message is an ask, show user ask UI
220220
// if user finished a task, then start a new task with a new conversation history since in this moment that the extension is waiting for user response, the user could close the extension and the conversation history would be lost.
221221
// basically as long as a task is active, the conversation history will be persisted
@@ -390,7 +390,8 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
390390
break
391391
}
392392
}
393-
}, [lastMessage, secondLastMessage])
393+
// eslint-disable-next-line react-hooks/exhaustive-deps -- isAutoApproved is defined later in the file
394+
}, [lastMessage, secondLastMessage, t, playSound])
394395

395396
useEffect(() => {
396397
if (messages.length === 0) {

‎webview-ui/src/components/settings/AutoApproveSettings.tsx‎

Lines changed: 2 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,7 @@ export const AutoApproveSettings = ({
7676
}
7777

7878
// Use the centralized auto-approve state hook
79-
const { hasAnyAutoApprovedAction, updateAutoApprovalState, handleMasterToggle } = useAutoApproveState({
79+
const { updateAutoApprovalState } = useAutoApproveState({
8080
toggles,
8181
setCachedStateField,
8282
})
@@ -102,28 +102,7 @@ export const AutoApproveSettings = ({
102102
</SectionHeader>
103103

104104
<Section>
105-
{/* Master Auto-Approval Checkbox */}
106-
<div className="flex flex-col gap-4 mb-6">
107-
<div className="flex items-center gap-3">
108-
<VSCodeCheckbox
109-
checked={hasAnyAutoApprovedAction}
110-
onChange={() => handleMasterToggle()}
111-
data-testid="master-auto-approve-checkbox">
112-
<span className="font-medium text-base">
113-
{t("settings:autoApprove.masterToggle.label")}
114-
</span>
115-
</VSCodeCheckbox>
116-
</div>
117-
<div className="text-vscode-descriptionForeground text-sm pl-6">
118-
{t("settings:autoApprove.masterToggle.description")}
119-
</div>
120-
</div>
121-
122-
<AutoApproveToggle
123-
{...toggles}
124-
onToggle={updateAutoApprovalState}
125-
isOverallApprovalEnabled={hasAnyAutoApprovedAction}
126-
/>
105+
<AutoApproveToggle {...toggles} onToggle={updateAutoApprovalState} />
127106

128107
{/* ADDITIONAL SETTINGS */}
129108

‎webview-ui/src/components/settings/__tests__/AutoApproveSettings.spec.tsx‎

Lines changed: 0 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -34,8 +34,6 @@ jest.mock("@/i18n/TranslationContext", () => ({
3434
const translations: Record<string, string> = {
3535
"settings:sections.autoApprove": "Auto Approve",
3636
"settings:autoApprove.description": "Auto-approve certain actions",
37-
"settings:autoApprove.masterToggle.label": "Enable Auto-Approval",
38-
"settings:autoApprove.masterToggle.description": "Enable or disable all auto-approval settings at once",
3937
"settings:autoApprove.readOnly.label": "Read-only",
4038
"settings:autoApprove.write.label": "Write files",
4139
"settings:autoApprove.execute.label": "Execute",
@@ -84,55 +82,6 @@ describe("AutoApproveSettings", () => {
8482
jest.clearAllMocks()
8583
})
8684

87-
describe("Master Auto-Approval Checkbox", () => {
88-
it("should render master checkbox when no settings are enabled", () => {
89-
render(<AutoApproveSettings {...defaultProps} />)
90-
91-
const masterCheckbox = screen.getByTestId("master-auto-approve-checkbox")
92-
expect(masterCheckbox).toBeInTheDocument()
93-
expect(masterCheckbox).not.toBeChecked()
94-
})
95-
96-
it("should render master checkbox as checked when any setting is enabled", () => {
97-
render(<AutoApproveSettings {...defaultProps} alwaysAllowReadOnly={true} />)
98-
99-
const masterCheckbox = screen.getByTestId("master-auto-approve-checkbox")
100-
expect(masterCheckbox).toBeChecked()
101-
})
102-
103-
it("should render master checkbox as checked when multiple settings are enabled", () => {
104-
render(
105-
<AutoApproveSettings
106-
{...defaultProps}
107-
alwaysAllowReadOnly={true}
108-
alwaysAllowWrite={true}
109-
alwaysAllowBrowser={true}
110-
/>,
111-
)
112-
113-
const masterCheckbox = screen.getByTestId("master-auto-approve-checkbox")
114-
expect(masterCheckbox).toBeChecked()
115-
})
116-
})
117-
118-
describe("Individual toggles with master state", () => {
119-
it("should show individual toggles with full opacity when master is enabled", () => {
120-
render(<AutoApproveSettings {...defaultProps} alwaysAllowReadOnly={true} />)
121-
122-
const readOnlyToggle = screen.getByTestId("always-allow-readonly-toggle")
123-
expect(readOnlyToggle).toBeInTheDocument()
124-
expect(readOnlyToggle).not.toHaveClass("opacity-50")
125-
})
126-
127-
it("should show individual toggles with reduced opacity when master is disabled", () => {
128-
render(<AutoApproveSettings {...defaultProps} />)
129-
130-
const readOnlyToggle = screen.getByTestId("always-allow-readonly-toggle")
131-
expect(readOnlyToggle).toBeInTheDocument()
132-
expect(readOnlyToggle).toHaveClass("opacity-50")
133-
})
134-
})
135-
13685
describe("Additional settings visibility", () => {
13786
it("should show read-only additional settings when read-only is enabled", () => {
13887
render(<AutoApproveSettings {...defaultProps} alwaysAllowReadOnly={true} />)

‎webview-ui/src/hooks/useAutoApproveState.ts‎

Lines changed: 37 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -35,25 +35,16 @@ interface UseAutoApproveStateProps {
3535

3636
export const useAutoApproveState = ({ toggles, setters, setCachedStateField }: UseAutoApproveStateProps) => {
3737
// Calculate if any auto-approve action is enabled
38-
const hasAnyAutoApprovedAction = useMemo(() => Object.values(toggles).some((value) => !!value), [toggles])
38+
const hasAnyAutoApprovedAction = useMemo(() => {
39+
return Object.values(toggles).some((value) => !!value)
40+
}, [toggles])
3941

4042
// Update individual auto-approval setting
4143
const updateAutoApprovalState = useCallback(
4244
(key: AutoApproveSetting, value: boolean) => {
43-
// Calculate updated toggles state
44-
const updatedToggles = { ...toggles, [key]: value }
45-
const hasAnyEnabled = Object.values(updatedToggles).some((v) => !!v)
46-
4745
// Send vscode message for individual setting
4846
vscode.postMessage({ type: key, bool: value })
4947

50-
// Update main auto-approval setting based on new state if setter available
51-
if (setters?.setAutoApprovalEnabled) {
52-
const shouldEnableAutoApproval = hasAnyEnabled
53-
setters.setAutoApprovalEnabled(shouldEnableAutoApproval)
54-
vscode.postMessage({ type: "autoApprovalEnabled", bool: shouldEnableAutoApproval })
55-
}
56-
5748
// Update the specific setting state using appropriate setter
5849
if (setters) {
5950
switch (key) {
@@ -82,6 +73,15 @@ export const useAutoApproveState = ({ toggles, setters, setCachedStateField }: U
8273
setters.setAlwaysApproveResubmit?.(value)
8374
break
8475
}
76+
77+
// Update main auto-approval setting after state update
78+
if (setters.setAutoApprovalEnabled) {
79+
// Calculate if any will be enabled after this update
80+
const updatedToggles = { ...toggles, [key]: value }
81+
const hasAnyEnabled = Object.values(updatedToggles).some((v) => !!v)
82+
setters.setAutoApprovalEnabled(hasAnyEnabled)
83+
vscode.postMessage({ type: "autoApprovalEnabled", bool: hasAnyEnabled })
84+
}
8585
} else if (setCachedStateField) {
8686
// Fallback to setCachedStateField for settings page
8787
setCachedStateField(key, value)
@@ -95,51 +95,34 @@ export const useAutoApproveState = ({ toggles, setters, setCachedStateField }: U
9595
(enabled?: boolean) => {
9696
const newValue = enabled !== undefined ? enabled : !hasAnyAutoApprovedAction
9797

98-
// Set all individual toggles to the new value
99-
Object.keys(autoApproveSettingsConfig).forEach((key) => {
100-
const settingKey = key as AutoApproveSetting
101-
// Send vscode message for individual setting
102-
vscode.postMessage({ type: settingKey, bool: newValue })
98+
// Batch all updates to reduce re-renders
99+
if (setters) {
100+
// Update all individual settings in one batch
101+
setters.setAlwaysAllowReadOnly?.(newValue)
102+
setters.setAlwaysAllowWrite?.(newValue)
103+
setters.setAlwaysAllowExecute?.(newValue)
104+
setters.setAlwaysAllowBrowser?.(newValue)
105+
setters.setAlwaysAllowMcp?.(newValue)
106+
setters.setAlwaysAllowModeSwitch?.(newValue)
107+
setters.setAlwaysAllowSubtasks?.(newValue)
108+
setters.setAlwaysApproveResubmit?.(newValue)
103109

104-
// Update individual setting state using appropriate setter
105-
if (setters) {
106-
switch (settingKey) {
107-
case "alwaysAllowReadOnly":
108-
setters.setAlwaysAllowReadOnly?.(newValue)
109-
break
110-
case "alwaysAllowWrite":
111-
setters.setAlwaysAllowWrite?.(newValue)
112-
break
113-
case "alwaysAllowExecute":
114-
setters.setAlwaysAllowExecute?.(newValue)
115-
break
116-
case "alwaysAllowBrowser":
117-
setters.setAlwaysAllowBrowser?.(newValue)
118-
break
119-
case "alwaysAllowMcp":
120-
setters.setAlwaysAllowMcp?.(newValue)
121-
break
122-
case "alwaysAllowModeSwitch":
123-
setters.setAlwaysAllowModeSwitch?.(newValue)
124-
break
125-
case "alwaysAllowSubtasks":
126-
setters.setAlwaysAllowSubtasks?.(newValue)
127-
break
128-
case "alwaysApproveResubmit":
129-
setters.setAlwaysApproveResubmit?.(newValue)
130-
break
131-
}
132-
} else if (setCachedStateField) {
133-
// Fallback to setCachedStateField for settings page
134-
setCachedStateField(settingKey, newValue)
110+
// Update main auto-approval setting
111+
if (setters.setAutoApprovalEnabled) {
112+
setters.setAutoApprovalEnabled(newValue)
135113
}
136-
})
137-
138-
// Update main auto-approval setting once at the end
139-
if (setters?.setAutoApprovalEnabled) {
140-
setters.setAutoApprovalEnabled(newValue)
141-
vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue })
114+
} else if (setCachedStateField) {
115+
// Fallback to setCachedStateField for settings page
116+
Object.keys(autoApproveSettingsConfig).forEach((key) => {
117+
setCachedStateField(key as AutoApproveSetting, newValue)
118+
})
142119
}
120+
121+
// Send all vscode messages in one batch
122+
Object.keys(autoApproveSettingsConfig).forEach((key) => {
123+
vscode.postMessage({ type: key as AutoApproveSetting, bool: newValue })
124+
})
125+
vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue })
143126
},
144127
[hasAnyAutoApprovedAction, setters, setCachedStateField],
145128
)

‎webview-ui/src/i18n/locales/en/settings.json‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -68,10 +68,6 @@
6868
},
6969
"autoApprove": {
7070
"description": "Allow Roo to automatically perform operations without requiring approval. Enable these settings only if you fully trust the AI and understand the associated security risks.",
71-
"masterToggle": {
72-
"label": "Enable Auto-Approval",
73-
"description": "Enable or disable all auto-approval settings at once. When disabled, all individual auto-approval toggles are visually disabled but their saved states are preserved."
74-
},
7571
"readOnly": {
7672
"label": "Read",
7773
"description": "When enabled, Roo will automatically view directory contents and read files without requiring you to click the Approve button.",

0 commit comments

Comments
 (0)