Skip to content

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.

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 4 commits into from
Closed

feat: replace auto-approve text labels with icons in collapsed view #6500

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
228c9c7
feat: replace auto-approve text labels with icons in collapsed view
roomote Jul 31, 2025
cfa2081
feat: add tooltips to auto-approve icons in collapsed view
roomote Aug 5, 2025
782174e
feat(webview-ui): show all auto-approve icons in collapsed view and h…
Aug 19, 2025
e5b41c8
feat: make collapsed auto-approve icons clickable to toggle options d…
Aug 19, 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
34 changes: 21 additions & 13 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 @@ -129,18 +129,12 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
setIsExpanded((prev) => !prev)
}, [])

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

// Update displayed text logic
const displayText = useMemo(() => {
if (!effectiveAutoApprovalEnabled || !hasEnabledOptions) {
return t("chat:autoApprove.none")
}
return enabledActionsList || t("chat:autoApprove.none")
}, [effectiveAutoApprovalEnabled, hasEnabledOptions, enabledActionsList, t])
// Get enabled icons for display
const enabledIcons = useMemo(() => {
return Object.entries(toggles)
.filter(([_key, value]) => !!value)
.map(([key]) => autoApproveSettingsConfig[key as AutoApproveSetting].icon)
}, [toggles])
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

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.


const handleOpenSettings = useCallback(
() =>
Expand Down Expand Up @@ -213,8 +207,22 @@ const AutoApproveMenu = ({ style }: AutoApproveMenuProps) => {
whiteSpace: "nowrap",
flex: 1,
minWidth: 0,
display: "flex",
alignItems: "center",
gap: "6px",
}}>
{displayText}
{!effectiveAutoApprovalEnabled || !hasEnabledOptions
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

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.

? t("chat:autoApprove.none")
: enabledIcons.map((icon, index) => (
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

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.

<span
Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

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.

key={index}
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot Jul 31, 2025

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.

Copy link
Contributor Author

@roomote roomote bot Jul 31, 2025

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.

className={`codicon codicon-${icon}`}
style={{
fontSize: "14px",
flexShrink: 0,
}}
Copy link

Copilot AI Aug 20, 2025

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.

Suggested change
}}

Copilot uses AI. Check for mistakes.
/>
))}
</span>
<span
className={`codicon codicon-chevron-${isExpanded ? "down" : "right"}`}
Expand Down
12 changes: 8 additions & 4 deletions webview-ui/src/components/chat/__tests__/AutoApproveMenu.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,8 +111,9 @@ describe("AutoApproveMenu", () => {

render(<AutoApproveMenu />)

// Check that the text shows the enabled option
expect(screen.getByText("Read-only operations")).toBeInTheDocument()
// Check that the icon for read-only operations is shown
const container = screen.getByText("Auto-approve").parentElement?.parentElement
expect(container?.querySelector(".codicon-eye")).toBeInTheDocument()
})

it("should not allow toggling master checkbox when no options are selected", () => {
Expand Down Expand Up @@ -222,8 +223,11 @@ describe("AutoApproveMenu", () => {

render(<AutoApproveMenu />)

// Should show all enabled options in the summary
expect(screen.getByText("Read-only operations, Write operations, Execute operations")).toBeInTheDocument()
// Should show icons for all enabled options
const container = screen.getByText("Auto-approve").parentElement?.parentElement
expect(container?.querySelector(".codicon-eye")).toBeInTheDocument() // Read
expect(container?.querySelector(".codicon-edit")).toBeInTheDocument() // Write
expect(container?.querySelector(".codicon-terminal")).toBeInTheDocument() // Execute
})

it("should handle enabling first option when none selected", async () => {
Expand Down
Loading