Skip to content

fix: Fix Auto Approve UI Closing #2579 #4395

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 3 commits into from
Closed

fix: Fix Auto Approve UI Closing #2579 #4395

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
629aee0
fix: Prevent auto-approve toast from showing after dismissal
Mnehmos Jun 6, 2025
273e8ac
feat: Implement AutoApprove settings and menu
Mnehmos Jun 10, 2025
932bf69
Remove settings page master auto-approve toggle and fix performance i…
Mnehmos Jun 10, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion .gitignore
View file
Open in desktop
Copy link
Member

@daniel-lxs daniel-lxs Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Were these changes accidental?

Copy link
Contributor Author

@Mnehmos Mnehmos Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes yes and yes. This was my first PR attempt, I'll review all of these and come with a much more solid approach. Thank you for reviewing these changes.

Original file line number Diff line number Diff line change
Expand Up @@ -4,10 +4,13 @@ out
out-*
node_modules
coverage/
mock/

.DS_Store

# Webview UI specific ignores
webview-ui/node_modules
webview-ui/dist

# IDEs
.idea

Expand Down
53 changes: 45 additions & 8 deletions webview-ui/src/components/chat/AutoApproveMenu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
(key: AutoApproveSetting, value: boolean) => {
vscode.postMessage({ type: key, bool: value })

// Update the specific setting
switch (key) {
case "alwaysAllowReadOnly":
setAlwaysAllowReadOnly(value)
Expand All @@ -69,6 +70,29 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
setAlwaysApproveResubmit(value)
break
}

// After updating the specific setting, check if any action is now enabled.
// If so, ensure autoApprovalEnabled is true.
// This needs to be done after the state updates, so we'll use a temporary check
// or re-evaluate the `toggles` object.
// For simplicity, we'll assume the `toggles` state will reflect the change
// in the next render cycle, and we can force autoApprovalEnabled to true
// if any action is being enabled.
if (value === true) {
setAutoApprovalEnabled(true)
vscode.postMessage({ type: "autoApprovalEnabled", bool: true })
} else {
Copy link
Member

@daniel-lxs daniel-lxs Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic here only handles enabling the main checkbox when an individual action is enabled, but doesn't handle the reverse case. When the last enabled action is disabled, the main checkbox should also be disabled automatically.

Consider adding logic to check if all toggles are now false and update autoApprovalEnabled accordingly:

if (value === true) {
  setAutoApprovalEnabled(true)
  vscode.postMessage({ type: "autoApprovalEnabled", bool: true })
} else {
  // Check if this was the last enabled toggle
  const updatedToggles = { ...toggles, [key]: false }
  const hasAnyEnabled = Object.values(updatedToggles).some(v => !!v)
  if (!hasAnyEnabled) {
    setAutoApprovalEnabled(false)
    vscode.postMessage({ type: "autoApprovalEnabled", bool: false })
  }
}

// If an action is being disabled, check if all are now disabled.
// If so, set autoApprovalEnabled to false.
// This requires re-evaluating the state of all toggles *after* the current one is set.
// A more robust solution would involve passing the updated `toggles` object
// or re-calculating `hasAnyAutoApprovedAction` here.
// For now, let's rely on the `hasAnyAutoApprovedAction` memoized value
// which will update on the next render.
// If the user unchecks the last enabled option, autoApprovalEnabled should become false.
// This logic is already handled by the main checkbox's disabled state in the collapsed view.
// So, we only need to ensure it turns ON when an individual is turned ON.
}
Copy link
Member

@daniel-lxs daniel-lxs Jun 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The state synchronization logic here seems complex and the extensive comments suggest uncertainty about the approach. Could we simplify this by creating a more robust state update mechanism?

Specifically, the else branch (lines 84-95) acknowledges that it doesn't handle the case where disabling an action should potentially disable the main checkbox. This could lead to inconsistent UI states.

Consider extracting this logic into a separate function that can properly calculate the new state based on all current toggle values, rather than relying on "future render cycles".

},
[
setAlwaysAllowReadOnly,
Expand All @@ -79,6 +103,7 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
setAlwaysAllowModeSwitch,
setAlwaysAllowSubtasks,
setAlwaysApproveResubmit,
setAutoApprovalEnabled, // Added setAutoApprovalEnabled to dependencies
],
)

Expand Down Expand Up @@ -107,10 +132,17 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
],
)

const enabledActionsList = Object.entries(toggles)
.filter(([_key, value]) => !!value)
.map(([key]) => t(autoApproveSettingsConfig[key as AutoApproveSetting].labelKey))
.join(", ")
const hasAnyAutoApprovedAction = useMemo(() => Object.values(toggles).some((value) => !!value), [toggles])

const displayedAutoApproveText = useMemo(() => {
if (autoApprovalEnabled && hasAnyAutoApprovedAction) {
return Object.entries(toggles)
.filter(([_key, value]) => !!value)
.map(([key]) => t(autoApproveSettingsConfig[key as AutoApproveSetting].labelKey))
.join(", ")
}
return t("chat:autoApprove.none")
}, [autoApprovalEnabled, hasAnyAutoApprovedAction, toggles, t])

const handleOpenSettings = useCallback(
() =>
Expand Down Expand Up @@ -140,9 +172,10 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
onClick={toggleExpanded}>
<div onClick={(e) => e.stopPropagation()}>
<VSCodeCheckbox
checked={autoApprovalEnabled ?? false}
checked={autoApprovalEnabled && hasAnyAutoApprovedAction}
disabled={!hasAnyAutoApprovedAction}
onChange={() => {
const newValue = !(autoApprovalEnabled ?? false)
const newValue = !(autoApprovalEnabled && hasAnyAutoApprovedAction)
setAutoApprovalEnabled(newValue)
vscode.postMessage({ type: "autoApprovalEnabled", bool: newValue })
}}
Expand Down Expand Up @@ -172,7 +205,7 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
flex: 1,
minWidth: 0,
}}>
{enabledActionsList || t("chat:autoApprove.none")}
{displayedAutoApproveText}
</span>
<span
className={`codicon codicon-chevron-${isExpanded ? "down" : "right"}`}
Expand All @@ -199,7 +232,11 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
/>
</div>

<AutoApproveToggle {...toggles} onToggle={onAutoApproveToggle} />
<AutoApproveToggle
{...toggles}
onToggle={onAutoApproveToggle}
isOverallApprovalEnabled={autoApprovalEnabled}
/>

{/* Auto-approve API request count limit input row inspired by Cline */}
<div
Expand Down
41 changes: 24 additions & 17 deletions webview-ui/src/components/settings/AutoApproveToggle.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,9 +87,10 @@ export const autoApproveSettingsConfig: Record<AutoApproveSetting, AutoApproveCo

type AutoApproveToggleProps = AutoApproveToggles & {
onToggle: (key: AutoApproveSetting, value: boolean) => void
isOverallApprovalEnabled?: boolean // New prop
}

export const AutoApproveToggle = ({ onToggle, ...props }: AutoApproveToggleProps) => {
export const AutoApproveToggle = ({ onToggle, isOverallApprovalEnabled, ...props }: AutoApproveToggleProps) => {
const { t } = useAppTranslation()

return (
Expand All @@ -99,22 +100,28 @@ export const AutoApproveToggle = ({ onToggle, ...props }: AutoApproveToggleProps
"[@media(min-width:600px)]:gap-4",
"[@media(min-width:800px)]:max-w-[800px]",
)}>
{Object.values(autoApproveSettingsConfig).map(({ key, descriptionKey, labelKey, icon, testId }) => (
<Button
key={key}
variant={props[key] ? "default" : "outline"}
onClick={() => onToggle(key, !props[key])}
title={t(descriptionKey || "")}
aria-label={t(labelKey)}
aria-pressed={!!props[key]}
data-testid={testId}
className={cn(" aspect-square h-[80px]", !props[key] && "opacity-50")}>
<span className={cn("flex flex-col items-center gap-1")}>
<span className={`codicon codicon-${icon}`} />
<span className="text-sm text-center">{t(labelKey)}</span>
</span>
</Button>
))}
{Object.values(autoApproveSettingsConfig).map(({ key, descriptionKey, labelKey, icon, testId }) => {
const isButtonActive = props[key] // This reflects the actual state of the individual toggle
const isButtonVisuallyEnabled = isOverallApprovalEnabled === false ? false : isButtonActive

return (
<Button
key={key}
variant={isButtonActive ? "default" : "outline"} // Variant always reflects its own state
onClick={() => onToggle(key, !props[key])}
title={t(descriptionKey || "")}
aria-label={t(labelKey)}
aria-pressed={!!isButtonActive}
data-testid={testId}
className={cn(" aspect-square h-[80px]", !isButtonVisuallyEnabled && "opacity-50")} // Opacity based on overall state
>
<span className={cn("flex flex-col items-center gap-1")}>
<span className={`codicon codicon-${icon}`} />
<span className="text-sm text-center">{t(labelKey)}</span>
</span>
</Button>
)
})}
</div>
)
}