-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: Redesign Auto-Approve UI with dropdown and keyboard shortcuts #7421
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
- Move AutoApproveMenu below text area with dropdown trigger similar to ModeSelector - Add Stamp icon for dropdown trigger - Implement text ellipsis for long text in dropdown trigger - Change to two-column layout for better organization - Add Select All/Select None functionality with Lucide icons (ListChecks, LayoutList) - Update tests to work with new dropdown UI
- Add Alt+1 through Alt+0 keyboard shortcuts to toggle auto-approve options - Display keyboard shortcuts in tooltips for each option - Create AutoApproveKeyboardShortcuts component to handle global keyboard events
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.
Reviewing my own code is like debugging in a mirror - everything looks backwards but the bugs are still mine.
| "7": "alwaysAllowSubtasks", | ||
| "8": "alwaysAllowFollowupQuestions", | ||
| "9": "alwaysAllowUpdateTodoList", | ||
| "0": "alwaysApproveResubmit", |
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.
This KEYBOARD_SHORTCUTS constant is duplicated in AutoApproveToggleDropdown.tsx. We should extract this to a shared constants file to maintain a single source of truth. Consider creating a shared constants file like autoApproveConstants.ts:
| "0": "alwaysApproveResubmit", | |
| import { KEYBOARD_SHORTCUTS } from '@/constants/autoApproveConstants' |
| return () => { | ||
| window.removeEventListener("keydown", handleKeyDown) | ||
| } | ||
| }, [handleToggle]) |
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.
Is this intentional? The event listener cleanup depends on handleToggle which could cause unnecessary re-registrations. Consider using a ref to store the callback or wrapping it in useCallback with stable dependencies to prevent potential memory leaks.
| useEffect(() => { | ||
| const handleKeyDown = (event: KeyboardEvent) => { | ||
| // Check if Alt/Option key is pressed along with a number key | ||
| if (event.altKey && !event.ctrlKey && !event.metaKey && !event.shiftKey) { |
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 Alt+Number shortcuts might conflict with browser or screen reader shortcuts. Should we consider adding a setting to disable keyboard shortcuts or use different key combinations (e.g., Ctrl+Shift+Number)?
| if (toggles[key]) { | ||
| onAutoApproveToggle(key, 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.
Good addition of Select All/Select None functionality! These handlers are well-implemented and will improve the user experience when managing multiple auto-approve options.
|
@roomote-agent can you incorporate the feedback from the PR review? |
|
Hi @brunobergher! I see the review feedback and will incorporate the following improvements:
I'll get started on these changes right away! |
- Extract KEYBOARD_SHORTCUTS to shared constants file to avoid duplication - Fix event listener cleanup using useRef and stable callback to prevent memory leaks - Add configuration support for keyboard shortcuts (enabled/disabled, Alt vs Ctrl+Shift) - Add comprehensive tests for keyboard shortcut functionality - Improve code organization and maintainability
|
✅ @brunobergher All review feedback has been successfully incorporated! Changes implemented:1. ✨ Extracted KEYBOARD_SHORTCUTS to shared constants file
2. 🔧 Fixed event listener cleanup issue
3. ⌨️ Added keyboard shortcut configuration support
4. ✅ Added comprehensive test coverage
CI StatusAll checks have passed successfully:
The code is ready for review! 🚀 |
Summary
This PR redesigns the Auto-Approve functionality UI based on the Slack request to improve the user experience.
Changes
UI Redesign (First Commit)
Keyboard Shortcuts (Second Commit)
Testing
Screenshots
The implementation follows the design guidance from the attached image in the Slack request, with:
Notes
Important
Redesigns Auto-Approve UI with a dropdown, adds keyboard shortcuts, and updates tests for improved user experience.
AutoApproveMenubelow text area with dropdown trigger inChatView.tsx.AutoApproveMenu.tsx.AutoApproveToggleDropdown.tsx.AutoApproveMenu.tsx.AutoApproveKeyboardShortcutscomponent for Alt+1 to Alt+0 shortcuts inAutoApproveKeyboardShortcuts.tsx.AutoApproveToggleDropdown.tsx.AutoApproveMenu.spec.tsx.AutoApproveKeyboardShortcuts.spec.tsx.autoApproveConstants.ts.This description was created by
for 31fd2b9. You can customize this summary. It will automatically update as commits are pushed.