diff --git a/ui/src/components/create/SelectToolsDialog.tsx b/ui/src/components/create/SelectToolsDialog.tsx index 2a5265130..04cf7f491 100644 --- a/ui/src/components/create/SelectToolsDialog.tsx +++ b/ui/src/components/create/SelectToolsDialog.tsx @@ -36,7 +36,7 @@ import { isAgentTool, isAgentResponse, isMcpTool, - toolResponseToAgentTool, + mergeToolIntoServerEntry, groupMcpToolsByServer, serverNamesMatch, } from "@/lib/toolUtils"; @@ -326,37 +326,7 @@ export const SelectToolsDialog: React.FC = ({ setLocalSelectedTools((prev) => [...prev, toolToAdd]); } else { const tool = item as ToolsResponse; - - const existingServerToolIndex = localSelectedTools.findIndex( - (t) => - isMcpTool(t) && - serverNamesMatch(t.mcpServer?.name || "", tool.server_name), - ); - - if (existingServerToolIndex >= 0) { - const existingTool = localSelectedTools[existingServerToolIndex]; - - if (existingTool.mcpServer?.toolNames?.includes(tool.id)) { - return; - } - - const updatedTool = { - ...existingTool, - mcpServer: { - ...existingTool.mcpServer!, - toolNames: [...(existingTool.mcpServer!.toolNames || []), tool.id], - }, - }; - - setLocalSelectedTools((prev) => - prev.map((t, idx) => - idx === existingServerToolIndex ? updatedTool : t, - ), - ); - } else { - toolToAdd = toolResponseToAgentTool(tool, tool.server_name); - setLocalSelectedTools((prev) => [...prev, toolToAdd]); - } + setLocalSelectedTools((prev) => mergeToolIntoServerEntry(prev, tool)); } }; diff --git a/ui/src/components/onboarding/steps/ReviewStep.tsx b/ui/src/components/onboarding/steps/ReviewStep.tsx index c95cf026f..23bcad6df 100644 --- a/ui/src/components/onboarding/steps/ReviewStep.tsx +++ b/ui/src/components/onboarding/steps/ReviewStep.tsx @@ -6,6 +6,7 @@ import { ScrollArea } from "@/components/ui/scroll-area"; import { Separator } from "@/components/ui/separator"; import { Badge } from "@/components/ui/badge"; import { Loader2, FunctionSquare } from 'lucide-react'; +import { isMcpTool, isAgentTool } from "@/lib/toolUtils"; import type { Tool } from "@/types"; interface OnboardingDataForReview { @@ -74,12 +75,25 @@ export function ReviewStep({ onboardingData, isLoading, onBack, onSubmit }: Revi {onboardingData.selectedTools && onboardingData.selectedTools.length > 0 ? (
- {onboardingData.selectedTools.map((tool, index) => ( - - - {tool.mcpServer?.name} - - ))} + {onboardingData.selectedTools.flatMap((tool) => { + if (isMcpTool(tool)) { + return tool.mcpServer.toolNames.map((toolName) => ( + + + {toolName} + + )); + } + if (isAgentTool(tool)) { + return [( + + + {tool.agent.name} + + )]; + } + return []; + })}
) : ( diff --git a/ui/src/components/onboarding/steps/ToolSelectionStep.tsx b/ui/src/components/onboarding/steps/ToolSelectionStep.tsx index 0ccebc47e..8b4c94418 100644 --- a/ui/src/components/onboarding/steps/ToolSelectionStep.tsx +++ b/ui/src/components/onboarding/steps/ToolSelectionStep.tsx @@ -7,7 +7,7 @@ import { Alert, AlertDescription, AlertTitle } from "@/components/ui/alert"; import { Info, ChevronDown, ChevronRight, FunctionSquare, Search } from 'lucide-react'; import { LoadingState } from "@/components/LoadingState"; import { ErrorState } from "@/components/ErrorState"; -import { getToolResponseDisplayName, getToolResponseDescription, getToolResponseIdentifier, getToolResponseCategory, toolResponseToAgentTool } from "@/lib/toolUtils"; +import { getToolResponseDisplayName, getToolResponseDescription, getToolResponseIdentifier, getToolResponseCategory, isMcpTool, serverNamesMatch, mergeToolIntoServerEntry } from "@/lib/toolUtils"; import type { Tool, ToolsResponse } from "@/types"; import { Input } from "@/components/ui/input"; @@ -39,7 +39,7 @@ export function ToolSelectionStep({ if (tool.type === "Agent" && tool.agent) { return false; // Agents don't match ToolResponse objects } else if (tool.type === "McpServer" && tool.mcpServer) { - return tool.mcpServer.name === toolResponse.server_name && + return serverNamesMatch(tool.mcpServer.name, toolResponse.server_name) && tool.mcpServer.toolNames.includes(toolResponse.id); } return false; @@ -125,12 +125,16 @@ export function ToolSelectionStep({ "k8s_get_resources", ]; - const initialSelection: Tool[] = []; + // Merge desired tools that share a server into one entry instead of + // pushing one per tool, so preselection doesn't itself produce + // duplicate-looking rows for the same server. + let initialSelection: Tool[] = []; availableTools.forEach((tool) => { const toolId = getToolResponseDisplayName(tool); - if (desiredIds.includes(toolId)) { - initialSelection.push(toolResponseToAgentTool(tool, tool.server_name)); + if (!desiredIds.includes(toolId)) { + return; } + initialSelection = mergeToolIntoServerEntry(initialSelection, tool); }); if (initialSelection.length > 0) { @@ -141,10 +145,30 @@ export function ToolSelectionStep({ }, [availableTools, initialSelectedTools, selectedTools.length]); const handleToolToggle = (toolResponse: ToolsResponse) => { - const agentTool = toolResponseToAgentTool(toolResponse, toolResponse.server_name); setSelectedTools(prev => { const isSelected = prev.some(t => toolResponseMatchesTool(toolResponse, t)); - return isSelected ? prev.filter(t => !toolResponseMatchesTool(toolResponse, t)) : [...prev, agentTool]; + + if (isSelected) { + // Drop just this tool id from whichever entry holds it, and drop + // the entry entirely once it references no tools. + return prev + .map(t => { + if (!isMcpTool(t) || !serverNamesMatch(t.mcpServer.name, toolResponse.server_name)) { + return t; + } + const remainingToolNames = t.mcpServer.toolNames.filter((id) => id !== toolResponse.id); + return remainingToolNames.length > 0 + ? { ...t, mcpServer: { ...t.mcpServer, toolNames: remainingToolNames } } + : null; + }) + .filter((t): t is Tool => t !== null); + } + + // Merge into the existing entry for this server instead of adding a + // second entry for the same server, which otherwise produced + // duplicate-looking rows downstream (Review step, agent edit page) + // and duplicate live MCP connections at agent runtime. + return mergeToolIntoServerEntry(prev, toolResponse); }); }; @@ -217,11 +241,20 @@ export function ToolSelectionStep({ {expandedCategories[category] && (
- {categoryTools.map((tool: ToolsResponse) => ( -
+ {categoryTools.map((tool: ToolsResponse) => { + const selected = isToolSelected(tool); + return ( +
handleToolToggle(tool)} className="mt-1" /> @@ -233,7 +266,8 @@ export function ToolSelectionStep({

{getToolResponseDescription(tool)}

- ))} + ); + })}
)} diff --git a/ui/src/components/onboarding/steps/__tests__/ReviewStep.test.tsx b/ui/src/components/onboarding/steps/__tests__/ReviewStep.test.tsx new file mode 100644 index 000000000..ce14af680 --- /dev/null +++ b/ui/src/components/onboarding/steps/__tests__/ReviewStep.test.tsx @@ -0,0 +1,100 @@ +/** + * @jest-environment jsdom + * + * Bug: the "Selected Tools" badges rendered `tool.mcpServer?.name` (the MCP + * server name, e.g. "kagent-tool-server") - one badge per array entry. Now + * that ToolSelectionStep merges same-server picks into a single Tool entry's + * mcpServer.toolNames array, this collapsed to a single, unhelpful badge no + * matter how many/which tools were actually selected. + * + * Fix: flatMap over each entry's toolNames (for MCP tools) so Review shows + * one badge per selected tool name instead of per array entry. + */ +import React from "react"; +import { describe, it, expect } from "@jest/globals"; +import { render, screen } from "@testing-library/react"; +import { ReviewStep } from "@/components/onboarding/steps/ReviewStep"; +import type { Tool } from "@/types"; + +describe("ReviewStep Selected Tools", () => { + it("shows one badge per tool name, not one per (merged) server entry", () => { + const mergedServerTool: Tool = { + type: "McpServer", + mcpServer: { + kind: "RemoteMCPServer", + apiGroup: "kagent.dev", + name: "kagent-tool-server", + namespace: "kagent", + toolNames: ["k8s_get_pods", "k8s_get_events"], + }, + }; + + render( + {}} + onSubmit={() => {}} + />, + ); + + expect(screen.getByText("k8s_get_pods")).toBeInTheDocument(); + expect(screen.getByText("k8s_get_events")).toBeInTheDocument(); + expect(screen.queryByText("kagent-tool-server")).not.toBeInTheDocument(); + }); + + it("shows tools from two different servers as separate entries, not merged", () => { + const serverATool: Tool = { + type: "McpServer", + mcpServer: { + kind: "RemoteMCPServer", + apiGroup: "kagent.dev", + name: "kagent-tool-server", + namespace: "kagent", + toolNames: ["k8s_get_pods"], + }, + }; + const serverBTool: Tool = { + type: "McpServer", + mcpServer: { + kind: "RemoteMCPServer", + apiGroup: "kagent.dev", + name: "context-forge", + namespace: "kagent", + toolNames: ["argocd-get-application"], + }, + }; + + render( + {}} + onSubmit={() => {}} + />, + ); + + expect(screen.getByText("k8s_get_pods")).toBeInTheDocument(); + expect(screen.getByText("argocd-get-application")).toBeInTheDocument(); + expect(screen.queryByText("kagent-tool-server")).not.toBeInTheDocument(); + expect(screen.queryByText("context-forge")).not.toBeInTheDocument(); + }); + + it("shows the agent name for Agent-type selections", () => { + const agentTool: Tool = { + type: "Agent", + agent: { name: "researcher", namespace: "kagent" }, + }; + + render( + {}} + onSubmit={() => {}} + />, + ); + + expect(screen.getByText("researcher")).toBeInTheDocument(); + }); +}); diff --git a/ui/src/components/onboarding/steps/__tests__/ToolSelectionStep.test.tsx b/ui/src/components/onboarding/steps/__tests__/ToolSelectionStep.test.tsx new file mode 100644 index 000000000..e9c8a126a --- /dev/null +++ b/ui/src/components/onboarding/steps/__tests__/ToolSelectionStep.test.tsx @@ -0,0 +1,123 @@ +/** + * @jest-environment jsdom + * + * Bug: handleToolToggle pushed a brand-new Tool object per checkbox click + * instead of merging into an existing entry for the same MCP server (unlike + * SelectToolsDialog.handleAddItem, which merges same-server picks into one + * mcpServer.toolNames array). Selecting multiple tools from the same server + * during onboarding produced several Tool entries that all shared the same + * server identity - the data shape that made downstream duplicate-entry + * rendering bugs possible (see ToolsSection's duplicate-key issue). + * + * Fix: handleToolToggle now merges same-server picks into a single entry on + * select, and removes just that tool id (dropping the entry only once it + * references no tools) on deselect. + */ +import React from "react"; +import { describe, it, expect, jest } from "@jest/globals"; +import { render, screen } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { ToolSelectionStep } from "@/components/onboarding/steps/ToolSelectionStep"; +import type { Tool, ToolsResponse } from "@/types"; + +const makeTool = ( + id: string, + serverName = "kagent/kagent-tool-server", +): ToolsResponse => ({ + id, + server_name: serverName, + created_at: "", + updated_at: "", + deleted_at: "", + description: `${id} description`, + group_kind: "Tool", +}); + +const renderStep = (tools: ToolsResponse[], onNext = jest.fn<(tools: Tool[]) => void>()) => { + render( + , + ); + return { onNext }; +}; + +describe("ToolSelectionStep duplicate-selection prevention", () => { + it("merges multiple tool picks from the same server into a single Tool entry", async () => { + const user = userEvent.setup(); + const tools = [makeTool("k8s_get_pods"), makeTool("k8s_get_events")]; + const { onNext } = renderStep(tools); + + await user.click(await screen.findByRole("checkbox", { name: /k8s_get_pods/i })); + await user.click(screen.getByRole("checkbox", { name: /k8s_get_events/i })); + await user.click(screen.getByRole("button", { name: /next: review/i })); + + expect(onNext).toHaveBeenCalledTimes(1); + const submitted = onNext.mock.calls[0][0]; + expect(submitted).toHaveLength(1); + expect([...submitted[0]!.mcpServer!.toolNames].sort()).toEqual([ + "k8s_get_events", + "k8s_get_pods", + ]); + }); + + it("unchecking one of two same-server selections keeps the other tool selected", async () => { + const user = userEvent.setup(); + const tools = [makeTool("k8s_get_pods"), makeTool("k8s_get_events")]; + const { onNext } = renderStep(tools); + + await user.click(await screen.findByRole("checkbox", { name: /k8s_get_pods/i })); + await user.click(screen.getByRole("checkbox", { name: /k8s_get_events/i })); + await user.click(screen.getByRole("checkbox", { name: /k8s_get_pods/i })); + await user.click(screen.getByRole("button", { name: /next: review/i })); + + expect(onNext).toHaveBeenCalledTimes(1); + const submitted = onNext.mock.calls[0][0]; + expect(submitted).toHaveLength(1); + expect(submitted[0]!.mcpServer!.toolNames).toEqual(["k8s_get_events"]); + }); + + it("unchecking the only selected tool for a server removes the entry entirely", async () => { + const user = userEvent.setup(); + const tools = [makeTool("k8s_get_pods")]; + const { onNext } = renderStep(tools); + + await user.click(await screen.findByRole("checkbox", { name: /k8s_get_pods/i })); + await user.click(screen.getByRole("checkbox", { name: /k8s_get_pods/i })); + await user.click(screen.getByRole("button", { name: /next: review/i })); + + expect(onNext).toHaveBeenCalledWith([]); + }); + + it("keeps tools from different servers as separate Tool entries instead of merging them", async () => { + // Both server names contain "kagent-tool-server" so ToolSelectionStep's + // own K8s-only filter (server_name?.includes("kagent-tool-server")) + // lets both through - but they have different parsed names, so + // serverNamesMatch must NOT treat them as the same server. + const user = userEvent.setup(); + const tools = [ + makeTool("k8s_get_pods", "kagent/kagent-tool-server"), + makeTool("k8s_get_events", "kagent/kagent-tool-server-extra"), + ]; + const { onNext } = renderStep(tools); + + await user.click(await screen.findByRole("checkbox", { name: /k8s_get_pods/i })); + await user.click(screen.getByRole("checkbox", { name: /k8s_get_events/i })); + await user.click(screen.getByRole("button", { name: /next: review/i })); + + expect(onNext).toHaveBeenCalledTimes(1); + const submitted = onNext.mock.calls[0][0]; + expect(submitted).toHaveLength(2); + + const byServer = new Map( + submitted.map((t) => [t!.mcpServer!.name, t!.mcpServer!.toolNames]), + ); + expect(byServer.get("kagent-tool-server")).toEqual(["k8s_get_pods"]); + expect(byServer.get("kagent-tool-server-extra")).toEqual(["k8s_get_events"]); + }); +}); diff --git a/ui/src/lib/toolUtils.ts b/ui/src/lib/toolUtils.ts index 485e69169..40cd44879 100644 --- a/ui/src/lib/toolUtils.ts +++ b/ui/src/lib/toolUtils.ts @@ -276,3 +276,25 @@ export const getDiscoveredToolCategory = (tool: DiscoveredTool, serverRef: strin export const getDiscoveredToolIdentifier = (tool: DiscoveredTool, serverRef: string): string => { return `${serverRef}-${tool.name}`; }; + +// Adds toolResponse to an existing entry for its server (deduping +// toolNames), or appends a new entry if none exists yet. +export const mergeToolIntoServerEntry = (tools: Tool[], toolResponse: ToolsResponse): Tool[] => { + const existing = tools + .filter(isMcpTool) + .find((t) => serverNamesMatch(t.mcpServer.name, toolResponse.server_name)); + if (!existing) { + return [...tools, toolResponseToAgentTool(toolResponse, toolResponse.server_name)]; + } + const merged: Tool = { + ...existing, + mcpServer: { + ...existing.mcpServer, + // Callers may seed this from an existing agent's persisted tools + // (e.g. hand-edited YAML), which isn't guaranteed to satisfy + // toolNames: string[] at runtime despite the type - fall back to []. + toolNames: Array.from(new Set([...(existing.mcpServer.toolNames || []), toolResponse.id])), + }, + }; + return tools.map((t) => (t === existing ? merged : t)); +}; \ No newline at end of file