Skip to content

Commit 38c5d8c

Browse files
committed
fix: address PR review comments
- Fix tooltip rendering to conditionally show only when needed - Reuse useAutoApprovalState hook in ChatView to avoid code duplication - Improve code quality and consistency across auto-approve components
1 parent e734ec7 commit 38c5d8c

File tree

3 files changed

+62
-34
lines changed

3 files changed

+62
-34
lines changed

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

Lines changed: 15 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -198,26 +198,30 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
198198
}}
199199
onClick={toggleExpanded}>
200200
<div onClick={(e) => e.stopPropagation()}>
201-
<StandardTooltip content={!hasEnabledOptions ? t("chat:autoApprove.selectOptionsFirst") : ""}>
201+
{!hasEnabledOptions ? (
202+
<StandardTooltip content={t("chat:autoApprove.selectOptionsFirst")}>
203+
<VSCodeCheckbox
204+
checked={effectiveAutoApprovalEnabled}
205+
disabled={isCheckboxDisabled}
206+
aria-label={t("chat:autoApprove.disabledAriaLabel")}
207+
onChange={() => {
208+
// Show a message or do nothing
209+
return
210+
}}
211+
/>
212+
</StandardTooltip>
213+
) : (
202214
<VSCodeCheckbox
203215
checked={effectiveAutoApprovalEnabled}
204216
disabled={isCheckboxDisabled}
205-
aria-label={
206-
hasEnabledOptions
207-
? t("chat:autoApprove.toggleAriaLabel")
208-
: t("chat:autoApprove.disabledAriaLabel")
209-
}
217+
aria-label={t("chat:autoApprove.toggleAriaLabel")}
210218
onChange={() => {
211-
if (!hasEnabledOptions) {
212-
// Show a message or do nothing
213-
return
214-
}
215219
const newValue = !(autoApprovalEnabled ?? false)
216220
setAutoApprovalEnabled(newValue)
217221
vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue })
218222
}}
219223
/>
220-
</StandardTooltip>
224+
)}
221225
</div>
222226
<div
223227
style={{

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

Lines changed: 32 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ import { useSelectedModel } from "@src/components/ui/hooks/useSelectedModel"
3838
import RooHero from "@src/components/welcome/RooHero"
3939
import RooTips from "@src/components/welcome/RooTips"
4040
import { StandardTooltip } from "@src/components/ui"
41+
import { useAutoApprovalState } from "@src/hooks/useAutoApprovalState"
4142

4243
import TelemetryBanner from "../common/TelemetryBanner"
4344
import VersionIndicator from "../common/VersionIndicator"
@@ -959,26 +960,43 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
959960
[deniedCommands],
960961
)
961962

963+
// Create toggles object for useAutoApprovalState hook
964+
const autoApprovalToggles = useMemo(
965+
() => ({
966+
alwaysAllowReadOnly,
967+
alwaysAllowWrite,
968+
alwaysAllowExecute,
969+
alwaysAllowBrowser,
970+
alwaysAllowMcp,
971+
alwaysAllowModeSwitch,
972+
alwaysAllowSubtasks,
973+
alwaysAllowFollowupQuestions,
974+
alwaysAllowUpdateTodoList,
975+
}),
976+
[
977+
alwaysAllowReadOnly,
978+
alwaysAllowWrite,
979+
alwaysAllowExecute,
980+
alwaysAllowBrowser,
981+
alwaysAllowMcp,
982+
alwaysAllowModeSwitch,
983+
alwaysAllowSubtasks,
984+
alwaysAllowFollowupQuestions,
985+
alwaysAllowUpdateTodoList,
986+
],
987+
)
988+
989+
const { hasEnabledOptions } = useAutoApprovalState(autoApprovalToggles, autoApprovalEnabled)
990+
962991
const isAutoApproved = useCallback(
963992
(message: ClineMessage | undefined) => {
964993
// First check if auto-approval is enabled AND we have at least one permission
965994
if (!autoApprovalEnabled || !message || message.type !== "ask") {
966995
return false
967996
}
968997

969-
// Check if ANY auto-approve option is enabled
970-
const hasAnyAutoApproveEnabled =
971-
alwaysAllowReadOnly ||
972-
alwaysAllowWrite ||
973-
alwaysAllowBrowser ||
974-
alwaysAllowExecute ||
975-
alwaysAllowMcp ||
976-
alwaysAllowModeSwitch ||
977-
alwaysAllowSubtasks ||
978-
alwaysAllowFollowupQuestions ||
979-
alwaysAllowUpdateTodoList
980-
981-
if (!hasAnyAutoApproveEnabled) {
998+
// Use the hook's result instead of duplicating the logic
999+
if (!hasEnabledOptions) {
9821000
return false
9831001
}
9841002

@@ -1055,6 +1073,7 @@ const ChatViewComponent: React.ForwardRefRenderFunction<ChatViewRef, ChatViewPro
10551073
},
10561074
[
10571075
autoApprovalEnabled,
1076+
hasEnabledOptions,
10581077
alwaysAllowBrowser,
10591078
alwaysAllowReadOnly,
10601079
alwaysAllowReadOnlyOutsideWorkspace,

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

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -136,25 +136,30 @@ export const AutoApproveSettings = ({
136136
<div {...props}>
137137
<SectionHeader description={t("settings:autoApprove.description")}>
138138
<div className="flex items-center gap-2">
139-
<StandardTooltip content={!hasEnabledOptions ? t("settings:autoApprove.selectOptionsFirst") : ""}>
139+
{!hasEnabledOptions ? (
140+
<StandardTooltip content={t("settings:autoApprove.selectOptionsFirst")}>
141+
<VSCodeCheckbox
142+
checked={effectiveAutoApprovalEnabled}
143+
disabled={!hasEnabledOptions}
144+
aria-label={t("settings:autoApprove.disabledAriaLabel")}
145+
onChange={() => {
146+
// Do nothing when no options are enabled
147+
return
148+
}}
149+
/>
150+
</StandardTooltip>
151+
) : (
140152
<VSCodeCheckbox
141153
checked={effectiveAutoApprovalEnabled}
142154
disabled={!hasEnabledOptions}
143-
aria-label={
144-
hasEnabledOptions
145-
? t("settings:autoApprove.toggleAriaLabel")
146-
: t("settings:autoApprove.disabledAriaLabel")
147-
}
155+
aria-label={t("settings:autoApprove.toggleAriaLabel")}
148156
onChange={() => {
149-
if (!hasEnabledOptions) {
150-
return
151-
}
152157
const newValue = !(autoApprovalEnabled ?? false)
153158
setAutoApprovalEnabled(newValue)
154159
vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue })
155160
}}
156161
/>
157-
</StandardTooltip>
162+
)}
158163
<span className="codicon codicon-check w-4" />
159164
<div>{t("settings:sections.autoApprove")}</div>
160165
</div>

0 commit comments

Comments
 (0)