[Agent Builder] base management UI for plugins#257211
Conversation
|
/ci |
|
/ci |
|
/ci |
📝 WalkthroughWalkthroughIntroduces a comprehensive plugin management system for the Agent Builder, including UI components for listing, viewing, installing, and uploading plugins; React hooks for managing plugin data and operations; a PluginsService for API communication; and integration into the application routing, breadcrumbs, and navigation flows. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant UI as Plugin Install UI
participant Hook as useInstallPluginFromUrl
participant Service as PluginsService
participant API as Backend API
participant Toast as Toast Notification
User->>UI: Clicks "Install from URL"
UI->>UI: Opens InstallFromUrlModal
User->>UI: Enters URL, clicks Submit
UI->>Hook: Calls installFromUrl({url})
Hook->>Service: Calls installFromUrl({url})
Service->>API: POST /api/internal/plugins/install-from-url
activate API
API-->>Service: InstallPluginResponse
deactivate API
alt Success
Service-->>Hook: Success response
Hook->>Toast: Shows success toast
Hook->>UI: Closes modal
Toast-->>User: "Plugin installed successfully"
else Error
Service-->>Hook: Error
Hook->>Toast: Shows error toast
Toast-->>User: "Failed to install plugin"
end
sequenceDiagram
participant User as User
participant Table as AgentBuilderPluginsTable
participant Hook as useDeletePlugin
participant Service as useDeletePluginService
participant API as Backend API
participant Toast as Toast Notification
User->>Table: Clicks delete on plugin row
Table->>Hook: Calls deletePlugin(pluginId, pluginName)
Hook->>Hook: Opens delete confirmation modal
User->>Hook: Clicks "Delete" in modal
Hook->>Service: Calls mutateAsync({pluginId})
Service->>API: DELETE /api/plugins/{pluginId}
activate API
API-->>Service: DeletePluginResponse
deactivate API
alt Success
Service-->>Hook: Success
Hook->>Toast: Shows success toast
Hook->>Table: Invalidates plugins list query
Toast-->>User: "Plugin deleted"
else Error
Service-->>Hook: Error
Hook->>Toast: Shows error toast
Toast-->>User: "Failed to delete plugin"
end
Hook->>Hook: Closes modal, resets state
sequenceDiagram
participant User as User
participant Page as PluginDetailsPage
participant Hook as usePlugin
participant Service as usePluginService
participant API as Backend API
participant Component as PluginDetails Component
User->>Page: Navigates to /plugins/:pluginId
Page->>Hook: Calls usePlugin({pluginId})
Hook->>Service: Fetches plugin data
Service->>API: GET /api/plugins/{pluginId}
activate API
API-->>Service: PluginDefinition
deactivate API
alt Loading
Component->>User: Shows spinner
else Loaded
Service-->>Hook: Returns plugin data
Hook-->>Component: Passes PluginDefinition
Component->>Component: Renders identity, about, skills sections
Component-->>User: Displays plugin details
else Error
Hook->>Hook: Shows error toast
Page->>Page: Redirects to plugins list
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Comment |
chrisbmar
left a comment
There was a problem hiding this comment.
This is excellent. Super easy to review and follows similar structure to all other services and CRUD pages with some magic breadcrumb manipulation going on.
FWIW, I ran it locally and installed the plugin via the URL and all worked well. I tried to do it via zip too and that never worked.
When I downloaded the plugin from Github, it's stripping out the .claude-plugin folder and the plugin.json file so it makes sense that it's failing. LMK how I can test the zip option 🙂
| width: '40px', | ||
| align: 'right' as const, | ||
| render: (plugin: PluginDefinition) => ( | ||
| <PluginContextMenu plugin={plugin} onDelete={onDelete} canManage={manageTools} /> |
There was a problem hiding this comment.
Fine for now, and maybe in the future too. But, if we anticipate granular access for managePlugins, can we please ensure there is a ticket for it so it doesn't get lost?
There was a problem hiding this comment.
| ]; | ||
| }, [parentPlugin, skillId]); | ||
|
|
||
| useBreadcrumb(breadcrumbs); |
There was a problem hiding this comment.
The breadcrumbs are a bit jumpy in the UI, I wonder if it's because plugins or skills are initially loading? If that's the case you could return an empty array whilst they're loading to avoid the flash.
No changes necessary here though. The breadcrumb manipulation logic looks good and is nice in the video.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
x-pack/platform/plugins/shared/agent_builder/public/application/components/agents/list/agents.tsx (1)
70-75: Consider addingdata-test-subjfor testability.The new "Manage plugins" button follows the same pattern as the "Manage skills" button, which is good for consistency. However, neither button has a
data-test-subjattribute. Consider adding test attributes to improve testability of these navigation buttons.♻️ Suggested improvement
<EuiButtonEmpty aria-label={managePluginsLabel} href={createAgentBuilderUrl(appPaths.plugins.list)} + data-test-subj="agentBuilderManagePluginsButton" > <EuiText size="s">{managePluginsLabel}</EuiText> </EuiButtonEmpty>,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/platform/plugins/shared/agent_builder/public/application/components/agents/list/agents.tsx` around lines 70 - 75, The EuiButtonEmpty for the "Manage plugins" navigation lacks a data-test-subj which hurts automated testability; update the EuiButtonEmpty component (where managePluginsLabel and createAgentBuilderUrl(appPaths.plugins.list) are used) to include a descriptive data-test-subj (e.g. data-test-subj="managePluginsButton") for consistency with other navigation controls (and mirror the same change for the "Manage skills" button if present) so tests can reliably select this element.x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugins.ts (1)
35-44: Potential stale closure or infinite loop ifonLoadingErroris not memoized.If callers pass an inline arrow function for
onLoadingError, the dependency array will see a new reference each render, potentially retriggering the effect. Consider documenting this requirement or using a ref to store the callback.♻️ Alternative using ref pattern
+import { useEffect, useRef } from 'react'; + export const usePlugins = ({ onLoadingError }: UsePluginsProps = {}) => { const { addErrorToast } = useToasts(); const { plugins, isLoading, error, isError } = usePluginsService(); + const onLoadingErrorRef = useRef(onLoadingError); + onLoadingErrorRef.current = onLoadingError; useEffect(() => { if (isError) { const formattedError = formatAgentBuilderErrorMessage(error); addErrorToast({ title: labels.plugins.loadPluginsErrorToast, text: formattedError, }); - onLoadingError?.(new Error(formattedError)); + onLoadingErrorRef.current?.(new Error(formattedError)); } - }, [isError, error, addErrorToast, onLoadingError]); + }, [isError, error, addErrorToast]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugins.ts` around lines 35 - 44, The effect using useEffect that calls onLoadingError (in use_plugins.ts) can retrigger on every render if callers pass an inline onLoadingError (stale closure/infinite loop); fix by capturing the callback in a ref (e.g., onLoadingErrorRef.current) and updating that ref in a separate effect, then use the ref inside the existing effect that calls formatAgentBuilderErrorMessage, addErrorToast, and labels.plugins.loadPluginsErrorToast so the main effect's dependency array can omit the changing onLoadingError reference and only include stable deps (isError, error, addErrorToast).x-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/plugins_table.tsx (1)
75-83: Minor: Redundant page index reset when size changes.Line 80 sets
pageIndexto 0 after line 77 already set it topage.index. While functionally correct (size change should reset to page 0), the double setState could be combined.♻️ Cleaner pagination handling
onTableChange={({ page }: CriteriaWithPagination<PluginDefinition>) => { if (page) { - setTablePageIndex(page.index); if (page.size !== tablePageSize) { setTablePageSize(page.size); setTablePageIndex(0); + } else { + setTablePageIndex(page.index); } } }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/plugins_table.tsx` around lines 75 - 83, The onTableChange handler currently calls setTablePageIndex(page.index) then, if page.size changed, calls setTablePageSize(page.size) and again setTablePageIndex(0), causing redundant state updates; update the logic in onTableChange (the CriteriaWithPagination<PluginDefinition> callback) to first check if page.size !== tablePageSize and, if so, call setTablePageSize(page.size) and setTablePageIndex(0), otherwise call setTablePageIndex(page.index) — this removes the double setTablePageIndex call and keeps pagination behavior correct.x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugin.ts (1)
25-32: Consider avoiding non-null assertion and redundant type cast.Line 28 uses
pluginId!which is safe due to theenabledguard, but violates the guideline to avoid non-null assertions. Line 32's type assertion may be unnecessary if the query is properly typed.♻️ Cleaner approach avoiding assertions
const { data: plugin, isLoading, error, isError, - } = useQuery({ + } = useQuery<PluginDefinition | undefined>({ enabled: !!pluginId, queryKey: queryKeys.plugins.byId(pluginId), - queryFn: () => pluginsService.get({ pluginId: pluginId! }), + queryFn: () => { + if (!pluginId) throw new Error('pluginId is required'); + return pluginsService.get({ pluginId }); + }, }); return { - plugin: plugin as PluginDefinition | undefined, + plugin, isLoading, error, isError, };As per coding guidelines: "Avoid non-null assertions (
!) unless locally justified in TypeScript."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugin.ts` around lines 25 - 32, Replace the non-null assertion and redundant return cast by narrowing the param type instead: in the useQuery call, avoid pluginId! by casting pluginId to string in the queryFn argument (e.g., pass pluginId as string to pluginsService.get) and ensure the useQuery generic is typed to return PluginDefinition so you can return plugin directly without "as PluginDefinition | undefined"; update references to useQuery, queryFn, pluginsService.get and pluginId and remove the redundant cast on the returned plugin variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_delete_plugin.ts`:
- Around line 86-101: The error path in handleSuccess currently shows an error
toast but never closes the confirmation modal; update the failure branch in the
DeletePluginSuccessCallback (handleSuccess) to call setDeletePluginState(null)
before returning so the modal is reset when data.success is false; ensure you
call setDeletePluginState(null) alongside addErrorToast (in the same
conditional) in the handleSuccess function.
---
Nitpick comments:
In
`@x-pack/platform/plugins/shared/agent_builder/public/application/components/agents/list/agents.tsx`:
- Around line 70-75: The EuiButtonEmpty for the "Manage plugins" navigation
lacks a data-test-subj which hurts automated testability; update the
EuiButtonEmpty component (where managePluginsLabel and
createAgentBuilderUrl(appPaths.plugins.list) are used) to include a descriptive
data-test-subj (e.g. data-test-subj="managePluginsButton") for consistency with
other navigation controls (and mirror the same change for the "Manage skills"
button if present) so tests can reliably select this element.
In
`@x-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/plugins_table.tsx`:
- Around line 75-83: The onTableChange handler currently calls
setTablePageIndex(page.index) then, if page.size changed, calls
setTablePageSize(page.size) and again setTablePageIndex(0), causing redundant
state updates; update the logic in onTableChange (the
CriteriaWithPagination<PluginDefinition> callback) to first check if page.size
!== tablePageSize and, if so, call setTablePageSize(page.size) and
setTablePageIndex(0), otherwise call setTablePageIndex(page.index) — this
removes the double setTablePageIndex call and keeps pagination behavior correct.
In
`@x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugin.ts`:
- Around line 25-32: Replace the non-null assertion and redundant return cast by
narrowing the param type instead: in the useQuery call, avoid pluginId! by
casting pluginId to string in the queryFn argument (e.g., pass pluginId as
string to pluginsService.get) and ensure the useQuery generic is typed to return
PluginDefinition so you can return plugin directly without "as PluginDefinition
| undefined"; update references to useQuery, queryFn, pluginsService.get and
pluginId and remove the redundant cast on the returned plugin variable.
In
`@x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugins.ts`:
- Around line 35-44: The effect using useEffect that calls onLoadingError (in
use_plugins.ts) can retrigger on every render if callers pass an inline
onLoadingError (stale closure/infinite loop); fix by capturing the callback in a
ref (e.g., onLoadingErrorRef.current) and updating that ref in a separate
effect, then use the ref inside the existing effect that calls
formatAgentBuilderErrorMessage, addErrorToast, and
labels.plugins.loadPluginsErrorToast so the main effect's dependency array can
omit the changing onLoadingError reference and only include stable deps
(isError, error, addErrorToast).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 78ab2591-6536-4345-8d85-5c01d51f2a6d
📒 Files selected for processing (24)
x-pack/platform/plugins/shared/agent_builder/public/application/components/agents/list/agents.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/conversations/conversation_header/more_actions_button.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/install_from_url_modal.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/install_plugin_button.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/plugin_details.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/plugins.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/plugins_table.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/plugins_table_context_menu.tsxx-pack/platform/plugins/shared/agent_builder/public/application/components/plugins/upload_plugin_modal.tsxx-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_delete_plugin.tsx-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_install_plugin.tsx-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugin.tsx-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_plugins.tsx-pack/platform/plugins/shared/agent_builder/public/application/pages/plugin_details.tsxx-pack/platform/plugins/shared/agent_builder/public/application/pages/plugins.tsxx-pack/platform/plugins/shared/agent_builder/public/application/pages/skill_details.tsxx-pack/platform/plugins/shared/agent_builder/public/application/query_keys.tsx-pack/platform/plugins/shared/agent_builder/public/application/routes.tsxx-pack/platform/plugins/shared/agent_builder/public/application/utils/app_paths.tsx-pack/platform/plugins/shared/agent_builder/public/application/utils/i18n.tsx-pack/platform/plugins/shared/agent_builder/public/plugin.tsxx-pack/platform/plugins/shared/agent_builder/public/services/index.tsx-pack/platform/plugins/shared/agent_builder/public/services/plugins/plugins_service.tsx-pack/platform/plugins/shared/agent_builder/public/services/types.ts
| const handleSuccess: DeletePluginSuccessCallback = (data, { pluginName }) => { | ||
| if (!data.success) { | ||
| addErrorToast({ | ||
| title: labels.plugins.deletePluginErrorToast(pluginName), | ||
| text: formatAgentBuilderErrorMessage( | ||
| new Error('Delete operation failed. API returned: { success: false }') | ||
| ), | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| addSuccessToast({ | ||
| title: labels.plugins.deletePluginSuccessToast(pluginName), | ||
| }); | ||
| setDeletePluginState(null); | ||
| }; |
There was a problem hiding this comment.
Modal state not reset when data.success is false.
When the delete API returns { success: false }, an error toast is shown but deletePluginState is not reset to null, leaving the confirmation modal open. The user would need to manually cancel.
🐛 Proposed fix to reset state on failure
const handleSuccess: DeletePluginSuccessCallback = (data, { pluginName }) => {
if (!data.success) {
addErrorToast({
title: labels.plugins.deletePluginErrorToast(pluginName),
text: formatAgentBuilderErrorMessage(
new Error('Delete operation failed. API returned: { success: false }')
),
});
+ setDeletePluginState(null);
return;
}
addSuccessToast({
title: labels.plugins.deletePluginSuccessToast(pluginName),
});
setDeletePluginState(null);
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@x-pack/platform/plugins/shared/agent_builder/public/application/hooks/plugins/use_delete_plugin.ts`
around lines 86 - 101, The error path in handleSuccess currently shows an error
toast but never closes the confirmation modal; update the failure branch in the
DeletePluginSuccessCallback (handleSuccess) to call setDeletePluginState(null)
before returning so the modal is reset when data.success is false; ensure you
call setDeletePluginState(null) alongside addErrorToast (in the same
conditional) in the handleSuccess function.
|
@elasticmachine merge upstream |
|
/ci |
💔 Build Failed
Failed CI Steps
Test Failures
Metrics [docs]Module Count
Async chunks
Page load bundle
History
|
|
@elasticmachine merge upstream |
## Summary Fix elastic/search-team#13249 Add a base management interface for plugins - list plugins page - plugin view page (read only - plugins can only be installed/uninstalled) - action to install a plugin from url or zip - action to uninstall an existing plugin **Note:** - it's still not possible to associate plugins to agents, it's the next step (elastic/search-team#13250) - safe deletion is out of scope (elastic/search-team#13254) ### Demo https://github.com/user-attachments/assets/f9464936-4465-423d-9c2e-a27a502993d9 ### How to test valid plugin urls: - https://github.com/anthropics/financial-services-plugins/tree/main/financial-analysis - https://github.com/anthropics/financial-services-plugins/blob/main/financial-analysis/.claude-plugin/plugin.json <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added plugin management system with ability to install from URL or upload plugin files * Added plugin list view featuring search, pagination, sorting, and per-plugin actions * Added plugin details page displaying metadata, descriptions, and associated skills * Added Manage Plugins navigation option accessible from agent workspace and conversation header <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
## Summary Fix elastic/search-team#13249 Add a base management interface for plugins - list plugins page - plugin view page (read only - plugins can only be installed/uninstalled) - action to install a plugin from url or zip - action to uninstall an existing plugin **Note:** - it's still not possible to associate plugins to agents, it's the next step (elastic/search-team#13250) - safe deletion is out of scope (elastic/search-team#13254) ### Demo https://github.com/user-attachments/assets/f9464936-4465-423d-9c2e-a27a502993d9 ### How to test valid plugin urls: - https://github.com/anthropics/financial-services-plugins/tree/main/financial-analysis - https://github.com/anthropics/financial-services-plugins/blob/main/financial-analysis/.claude-plugin/plugin.json <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes * **New Features** * Added plugin management system with ability to install from URL or upload plugin files * Added plugin list view featuring search, pagination, sorting, and per-plugin actions * Added plugin details page displaying metadata, descriptions, and associated skills * Added Manage Plugins navigation option accessible from agent workspace and conversation header <!-- end of auto-generated comment: release notes by coderabbit.ai --> --------- Co-authored-by: Elastic Machine <elasticmachine@users.noreply.github.com>
Summary
Fix https://github.com/elastic/search-team/issues/13249
Add a base management interface for plugins
Note:
Demo
Screen.Recording.2026-03-11.at.20.54.02.mov
How to test
valid plugin urls:
Summary by CodeRabbit
Release Notes