-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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.
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
Conversation
This PR addresses issue RooCodeInc#2579, where the auto-approve toast notification would reappear even after the user selected "Don't show again." The root cause was that the user's preference was not being persisted across sessions.
daniel-lxs
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Mnehmos, Thank you for working on this fix!
I took a look at your PR and left a couple of suggestions that are worth taking a look at.
Let me know if you have any questions!
| // 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. | ||
| } |
There was a problem hiding this comment.
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".
| if (value === true) { | ||
| setAutoApprovalEnabled(true) | ||
| vscode.postMessage({ type: "autoApprovalEnabled", bool: true }) | ||
| } else { |
There was a problem hiding this comment.
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 })
}
}There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Were these changes accidental?
There was a problem hiding this comment.
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.
…ssues - 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 RooCodeInc#4395
|
Hey @Mnehmos, Thank you for taking a look at the suggestions! I was testing the auto-approve buttons and noticed a weird behavior: 2025-06-11_08-01-55.mp4It seems like the buttons are enabled or disabled randomly, can you take a look? |
Related GitHub Issue
Closes: #2579
Description
This pull request re-implements the changes from the original PR #4371.
This PR addresses issue #2579, which pointed out confusing UI behavior in the auto-approve feature. The changes ensure that the UI state is always clear and unambiguous to the user.
The primary changes are:
webview-ui/src/components/chat/AutoApproveMenu.tsx:webview-ui/src/components/settings/AutoApproveToggle.tsx:.gitignore:webview-ui/node_modulesandwebview-ui/distto the ignore list.These changes ensure that the user's preference is respected, improving the overall user experience.
Test Procedure
Type of Change
srcor test files.Pre-Submission Checklist
npm run lint).console.log) has been removed.npm test).mainbranch.npm run changesetif this PR includes user-facing changes or dependency updates.Screenshots / Videos
Documentation Updates
Additional Notes
This PR is a recreation of #4371 after the original branch was accidentally deleted.
Important
Fixes UI behavior in
AutoApproveMenu.tsxandAutoApproveToggle.tsxto ensure clear auto-approve state indication and updates.gitignore.AutoApproveMenu.tsx, the main "Auto-Approve" checkbox is disabled when no actions are approved and is automatically enabled when any action is selected.AutoApproveToggle.tsx, toggle buttons reflect their individual state and indicate if the overall auto-approve setting is enabled.webview-ui/node_modulesandwebview-ui/distto.gitignore.This description was created by
for 629aee0. You can customize this summary. It will automatically update as commits are pushed.