-
Notifications
You must be signed in to change notification settings - Fork 2.6k
feat: replace auto-approve text labels with icons in collapsed view #6500
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
- Updated AutoApproveMenu component to display icons instead of text labels when collapsed - Added enabledIcons computed value to extract icon names from enabled toggles - Modified the display span to render icons with proper styling - Updated tests to check for icon presence instead of text content - Removed unused displayText and enabledActionsList variables - Maintains the same functionality while providing a more compact visual representation
| ? t("chat:autoApprove.none") | ||
| : enabledIcons.map((icon, index) => ( | ||
| <span | ||
| key={index} |
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.
Consider using the toggle key (e.g., the key string) as the unique key instead of the index, and add an aria-label attribute to each icon span for accessibility.
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.
Thank you for this UI improvement! The refactoring successfully replaces text labels with icons for a more compact interface. I've identified a few areas that could be enhanced for better robustness and accessibility.
| ? t("chat:autoApprove.none") | ||
| : enabledIcons.map((icon, index) => ( | ||
| <span | ||
| key={index} |
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 enabledIcons array could contain duplicate icons if multiple settings share the same icon, causing React key warnings. Consider using the setting key + index as the key instead of just index, or filter out duplicates if that's the intended behavior.
| {!effectiveAutoApprovalEnabled || !hasEnabledOptions | ||
| ? t("chat:autoApprove.none") | ||
| : enabledIcons.map((icon, index) => ( | ||
| <span |
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.
For better accessibility, consider adding aria-label attributes to these icon spans. Screen readers won't be able to interpret the meaning of these icons without text labels or proper ARIA attributes.
| {displayText} | ||
| {!effectiveAutoApprovalEnabled || !hasEnabledOptions | ||
| ? t("chat:autoApprove.none") | ||
| : enabledIcons.map((icon, index) => ( |
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.
With many enabled options, icons could overflow the container. Consider adding a maximum display count (e.g., show first 3-4 icons + '...' indicator) or implementing horizontal scrolling for better UX.
| gap: "6px", | ||
| }}> | ||
| {displayText} | ||
| {!effectiveAutoApprovalEnabled || !hasEnabledOptions |
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 inline conditional logic could be extracted to a helper function for better readability. Something like renderEnabledIcons() would make the JSX cleaner.
| return Object.entries(toggles) | ||
| .filter(([_key, value]) => !!value) | ||
| .map(([key]) => autoApproveSettingsConfig[key as AutoApproveSetting].icon) | ||
| }, [toggles]) |
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 dependency array includes the entire toggles object. For better performance, consider memoizing based on just the enabled state values or using a more specific dependency array.
|
@roomote-agent Add tooltips to the icons so they are more easily identifiable, do not create a new PR, push to this one |
|
I see your request to add tooltips to the icons in the collapsed view. I'll work on implementing this now and push the changes to this PR branch. |
- Wrapped each icon in StandardTooltip component - Tooltips display the label for each auto-approve option - Added test to verify tooltips appear on hover - Helps users identify icons more easily in collapsed state
|
I've successfully added tooltips to the icons in the collapsed auto-approve menu! 🎉 Changes made:
The implementation uses the existing The changes have been pushed to this PR branch and CI checks are currently running. |
|
@hannesrudolph This looks a bit weird |
|
| const tooltipContent = config ? t(config.labelKey) : "" | ||
|
|
||
| return ( | ||
| <StandardTooltip key={index} content={tooltipContent}> |
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.
Consider using a stable unique key (e.g. the icon name) instead of the array index in the map() to avoid potential key collisions if the order changes.
| <StandardTooltip key={index} content={tooltipContent}> | |
| <StandardTooltip key={icon} content={tooltipContent}> |
…ighlight active ones; update tests
…irectly - Icons in collapsed view are now clickable buttons that toggle their respective options - Added hover effect for better UX feedback - Clicking an icon directly toggles the auto-approve option without expanding the menu - Updated tests to verify the new clickable behavior - Maintains tooltips on all icon buttons
|
Related Documentation No published documentation to review for changes on this repository. |
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.
Pull Request Overview
This PR modernizes the auto-approve menu interface by replacing text labels with icons in the collapsed view, creating a more compact and visually appealing design. The functionality remains unchanged while improving the visual representation.
- Replace text labels with corresponding icons (eye, edit, terminal, etc.) in collapsed view
- Add interactive tooltips and hover states for better user experience
- Update test suite to verify icon presence and tooltip functionality
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| AutoApproveMenu.tsx | Replaces text display with icon buttons, adds tooltips and hover effects |
| AutoApproveMenu.spec.tsx | Updates tests to check for icon presence, highlighting, and tooltip functionality |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| alignItems: "center", | ||
| gap: "6px", | ||
| }} | ||
| onClick={(e) => e.stopPropagation()}> |
Copilot
AI
Aug 20, 2025
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 onClick handler e.stopPropagation() on the span element is redundant since each individual button already handles its own click events. This could be removed to simplify the code.
| onClick={(e) => e.stopPropagation()}> | |
| > |
| }} | ||
| onMouseLeave={(e) => { | ||
| e.currentTarget.style.backgroundColor = "transparent" | ||
| }} |
Copilot
AI
Aug 20, 2025
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.
[nitpick] The inline mouse event handlers for hover effects could be extracted to improve readability and maintainability. Consider using CSS hover pseudo-selectors or extracting these into named functions.
| }} |
| const isEnabled = !!toggles[key] | ||
| return ( | ||
| <StandardTooltip key={key} content={t(labelKey)}> | ||
| <button |
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.
Consider adding an accessible aria-label to the icon button (e.g., aria-label={t(labelKey)}) to ensure screen reader users can understand its purpose.
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
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.
LGTM

This PR replaces the text labels in the collapsed auto-approve menu with their corresponding icons, providing a more compact and visually consistent interface.
FocuSee.Project.2025-08-19.18-17-55.mp4
Changes
enabledIconscomputed value to extract icon names from enabled togglesdisplayTextandenabledActionsListvariablesVisual Changes
Testing
Important
Replaces text labels with icons in the collapsed AutoApproveMenu for a more compact UI, updating tests accordingly.
AutoApproveMenunow displays icons instead of text labels in collapsed view.enabledIconscomputed value to extract icon names from enabled toggles.displayTextandenabledActionsListvariables.AutoApproveMenu.spec.tsxto check for icon presence instead of text content.This description was created by
for e5b41c8. You can customize this summary. It will automatically update as commits are pushed.