feat: add right-click context menu to sidebar plugin icons order to be able to remove a provider without going to the settings.#197
Conversation
…d and Remove actions
MariosPapadakis
requested review from
davidarny,
robinebers and
validatedev
as code owners
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey! 👋 This is Rob's AI reviewer. Thanks for the contribution!
Nice feature and clean wiring. The new right-click flow feels great, and Remove matching Settings behavior is the right call. One thing to fix before merge:
- Please clean up menu resources after use — each right-click creates native menu objects (
Menu,IconMenuItem,PredefinedMenuItem). Right now they are not closed, so memory can grow over time. Please close them in afinallyblock aftermenu.popup().
Example shape:
try {
await menu.popup()
} finally {
await Promise.allSettled([
menu.close(),
reloadItem.close(),
separator.close(),
removeItem.close(),
])
}Everything else in this PR looks good to me.
There was a problem hiding this comment.
Pull request overview
This PR adds a right-click context menu to sidebar plugin icons, allowing users to reload or remove (disable) providers without navigating to the Settings page. The implementation uses Tauri's Menu API to create native context menus with "Reload" and "Remove" actions.
Changes:
- Added context menu functionality to plugin icons in the sidebar with Reload and Remove options
- Integrated the remove action with existing plugin toggle logic and analytics tracking
- Added
core:menu:defaultcapability to enable Tauri Menu API usage
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| src/components/side-nav.tsx | Implements context menu creation and handling for plugin icons using Tauri Menu API |
| src/App.tsx | Adds handlePluginContextAction callback to process reload and remove actions from the context menu |
| src-tauri/capabilities/default.json | Enables core:menu:default permission for native menu functionality |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
| } | ||
| }, | ||
| [handleRetryPlugin, pluginSettings, scheduleTrayIconUpdate, activeView] |
There was a problem hiding this comment.
The dependency array is missing setPluginSettings and setActiveView. These state setters should be included in the dependency array even though they're stable in React, to maintain consistency with other callbacks in the codebase and follow React best practices. The existing handleToggle callback at line 904 follows the same pattern and includes all dependencies.
| [handleRetryPlugin, pluginSettings, scheduleTrayIconUpdate, activeView] | |
| [handleRetryPlugin, pluginSettings, scheduleTrayIconUpdate, activeView, setPluginSettings, setActiveView] |
| ;(async () => { | ||
| const reloadItem = await IconMenuItem.new({ | ||
| id: `ctx-reload-${pluginId}`, | ||
| text: "Reload", | ||
| icon: NativeIcon.Refresh, | ||
| action: () => onPluginContextAction(pluginId, "reload"), | ||
| }) | ||
| const separator = await PredefinedMenuItem.new({ item: "Separator" }) | ||
| const removeItem = await IconMenuItem.new({ | ||
| id: `ctx-remove-${pluginId}`, | ||
| text: "Remove", | ||
| icon: NativeIcon.Remove, | ||
| action: () => onPluginContextAction(pluginId, "remove"), | ||
| }) | ||
| const menu = await Menu.new({ | ||
| items: [reloadItem, separator, removeItem], | ||
| }) | ||
| await menu.popup() | ||
| })().catch(console.error) |
There was a problem hiding this comment.
The menu creation happens on every context menu event but doesn't clean up the created menu items. This could lead to memory leaks over time. Consider cleaning up menu items after the menu is closed, or memoize the menu creation to reuse the same instances. Other parts of the codebase handle cleanup carefully (e.g., ResizeObserver and MutationObserver cleanup in App.tsx lines 951-955).
There was a problem hiding this comment.
@copilot open a new pull request to apply changes based on this feedback
| const handlePluginContextMenu = useCallback( | ||
| (e: React.MouseEvent, pluginId: string) => { | ||
| e.preventDefault() | ||
| if (!onPluginContextAction) return | ||
|
|
||
| ;(async () => { | ||
| const reloadItem = await IconMenuItem.new({ | ||
| id: `ctx-reload-${pluginId}`, | ||
| text: "Reload", | ||
| icon: NativeIcon.Refresh, | ||
| action: () => onPluginContextAction(pluginId, "reload"), | ||
| }) | ||
| const separator = await PredefinedMenuItem.new({ item: "Separator" }) | ||
| const removeItem = await IconMenuItem.new({ | ||
| id: `ctx-remove-${pluginId}`, | ||
| text: "Remove", | ||
| icon: NativeIcon.Remove, | ||
| action: () => onPluginContextAction(pluginId, "remove"), | ||
| }) | ||
| const menu = await Menu.new({ | ||
| items: [reloadItem, separator, removeItem], | ||
| }) | ||
| await menu.popup() | ||
| })().catch(console.error) | ||
| }, | ||
| [onPluginContextAction] | ||
| ) |
There was a problem hiding this comment.
The new context menu functionality lacks test coverage. The existing side-nav.test.tsx file tests other SideNav behaviors but doesn't include tests for the new onPluginContextAction callback. Consider adding tests to verify that the context menu handler is called correctly when right-clicking a plugin icon. The codebase follows comprehensive testing practices as evidenced by the existing test suite.
| const handlePluginContextAction = useCallback( | ||
| (pluginId: string, action: PluginContextAction) => { | ||
| if (action === "reload") { | ||
| handleRetryPlugin(pluginId) | ||
| } else if (action === "remove") { | ||
| // Disable the plugin (same as unchecking in settings) | ||
| if (!pluginSettings) return | ||
| const alreadyDisabled = pluginSettings.disabled.includes(pluginId) | ||
| if (alreadyDisabled) return | ||
|
|
||
| track("provider_toggled", { provider_id: pluginId, enabled: "false" }) | ||
| const nextSettings: PluginSettings = { | ||
| ...pluginSettings, | ||
| disabled: [...pluginSettings.disabled, pluginId], | ||
| } | ||
| setPluginSettings(nextSettings) | ||
| scheduleTrayIconUpdate("settings", TRAY_SETTINGS_DEBOUNCE_MS) | ||
| void savePluginSettings(nextSettings).catch((error) => { | ||
| console.error("Failed to save plugin toggle:", error) | ||
| }) | ||
|
|
||
| // If we were viewing this plugin, switch to home | ||
| if (activeView === pluginId) { | ||
| setActiveView("home") | ||
| } | ||
| } | ||
| }, | ||
| [handleRetryPlugin, pluginSettings, scheduleTrayIconUpdate, activeView] | ||
| ) |
There was a problem hiding this comment.
The new handlePluginContextAction callback lacks test coverage. The existing App.test.tsx file extensively tests plugin toggling behavior (e.g., "enables disabled plugin and starts batch" at line 865), but there are no tests for the new context menu actions. Consider adding tests for both "reload" and "remove" actions, including edge cases like removing an already disabled plugin or removing the currently active plugin.
validatedev
left a comment
validatedev
left a comment
There was a problem hiding this comment.
I agree with @davidarny, need some testcov.
4 participants
Description
Adds a second action to the native right-click context menu provider icons in the sidebar:
Testing
Screenshots
Before:
After:
Related Issue
New feature.
Type of Change
Testing
bun run buildand it succeededbun run testand all tests passbun tauri devScreenshots
Checklist
mainbranchSummary by cubic
Added a right-click context menu to sidebar provider icons with Reload and Remove, so you can disable a provider without opening Settings. Remove mirrors the Settings toggle and auto-returns to Home if the provider was active.
Written for commit c4c7736. Summary will update on new commits.