From 6f535356b452871a8e6cf76837ad64a4b2481e39 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Mon, 10 Aug 2026 19:56:38 -0400 Subject: [PATCH] fix: key tool rows by source position so duplicate names filter correctly `ToolControls` keyed both the main list and the SEP-2243 excluded list by `tool.name`. A server may legitimately return the same tool name more than once, and colliding React keys let a filtered-out row survive reconciliation: searching narrows the array, React matches the surviving rows against the stale keys, and an unrelated row stays mounted (plus a "two children with the same key" console warning). Stamp each row's position in the unfiltered list before filtering and use `:` as the key, on both lists. The position is captured before the filter, so the key stays stable as the search narrows. No deduplication, no change to what `tools/list` returns, and the name/title search behavior is unchanged. Adds regression tests for both lists using the reporter's fixture. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt --- .../groups/ToolControls/ToolControls.test.tsx | 85 +++++++++++++++++++ .../groups/ToolControls/ToolControls.tsx | 45 ++++++---- 2 files changed, 112 insertions(+), 18 deletions(-) diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx index 1ce40d1f..ef3d0210 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx @@ -204,6 +204,91 @@ describe("ToolControls", () => { expect(onRefreshList).toHaveBeenCalledTimes(1); }); + // A server may legitimately return the same tool name twice; keying rows by + // name alone collides, and React then reuses the stale row instead of + // unmounting it when the filter narrows (#1957). + const duplicateNameTools: Tool[] = [ + { + name: "get_record", + title: "Get Record First", + inputSchema: { type: "object" }, + }, + { + name: "duplicate_tool", + title: "First Duplicate", + inputSchema: { type: "object" }, + }, + { + name: "unrelated_tool", + title: "Unrelated Tool First", + inputSchema: { type: "object" }, + }, + { + name: "duplicate_tool", + title: "Second Duplicate", + inputSchema: { type: "object" }, + }, + { + name: "get_record", + title: "Get Record Second", + inputSchema: { type: "object" }, + }, + { + name: "unrelated_tool", + title: "Unrelated Tool Second", + inputSchema: { type: "object" }, + }, + ]; + + it("removes every non-matching row when tool names repeat (#1957)", async () => { + const user = userEvent.setup(); + renderWithMantine(); + + await user.type(screen.getByPlaceholderText("Search tools..."), "get"); + + expect(screen.getByText("Get Record First")).toBeInTheDocument(); + expect(screen.getByText("Get Record Second")).toBeInTheDocument(); + for (const stale of [ + "First Duplicate", + "Second Duplicate", + "Unrelated Tool First", + "Unrelated Tool Second", + ]) { + expect(screen.queryByText(stale)).not.toBeInTheDocument(); + } + }); + + it("removes every non-matching excluded row when names repeat (#1957)", async () => { + const user = userEvent.setup(); + const duplicateExcluded = [ + { + tool: { name: "get_thing", inputSchema: { type: "object" as const } }, + reason: "a", + }, + { + tool: { name: "dupe", inputSchema: { type: "object" as const } }, + reason: "b", + }, + { + tool: { name: "dupe", inputSchema: { type: "object" as const } }, + reason: "c", + }, + { + tool: { name: "get_other", inputSchema: { type: "object" as const } }, + reason: "d", + }, + ]; + renderWithMantine( + , + ); + + await user.type(screen.getByPlaceholderText("Search tools..."), "get"); + + expect(screen.getByText("get_thing")).toBeInTheDocument(); + expect(screen.getByText("get_other")).toBeInTheDocument(); + expect(screen.queryByText("dupe")).not.toBeInTheDocument(); + }); + it("renders no load error by default", () => { renderWithMantine(); expect(screen.queryByText(/Couldn't load/)).not.toBeInTheDocument(); diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.tsx index 1806b0c3..4ad70d85 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.tsx @@ -108,6 +108,18 @@ const ExcludedTooltip = Tooltip.withProps({ position: "right", }); +// A server may return the same tool name more than once, so the name alone is +// not a unique React key — colliding keys let a filtered-out row survive +// reconciliation instead of unmounting (#1957). The tool's position in the +// unfiltered list disambiguates duplicates and stays stable while the search +// narrows, since it is captured before filtering. +const rowKey = (name: string, sourceIndex: number) => `${sourceIndex}:${name}`; + +/** Matches a tool against the (already lower-cased) search query by name or title. */ +const matchesQuery = (tool: Tool, query: string) => + tool.name.toLowerCase().includes(query) || + (tool.title?.toLowerCase().includes(query) ?? false); + export function ToolControls({ tools, excludedTools = [], @@ -122,22 +134,19 @@ export function ToolControls({ }: ToolControlsProps) { const viewportRef = useScrollMemory("tools-sidebar"); const query = searchText.toLowerCase(); - const filteredTools = searchText - ? tools.filter( - (tool) => - tool.name.toLowerCase().includes(query) || - (tool.title?.toLowerCase().includes(query) ?? false), - ) - : tools; + // Stamp each row's source position before filtering, so the key survives the + // list narrowing (#1957). + const filteredTools = tools + .map((tool, sourceIndex) => ({ tool, key: rowKey(tool.name, sourceIndex) })) + .filter(({ tool }) => !searchText || matchesQuery(tool, query)); // Excluded tools are searchable too, matching name AND title like the main // list above, so a filtered view stays consistent. - const filteredExcluded = searchText - ? excludedTools.filter( - ({ tool }) => - tool.name.toLowerCase().includes(query) || - (tool.title?.toLowerCase().includes(query) ?? false), - ) - : excludedTools; + const filteredExcluded = excludedTools + .map((excluded, sourceIndex) => ({ + ...excluded, + key: rowKey(excluded.tool.name, sourceIndex), + })) + .filter(({ tool }) => !searchText || matchesQuery(tool, query)); return ( @@ -156,9 +165,9 @@ export function ToolControls({ - {filteredTools.map((tool) => ( + {filteredTools.map(({ tool, key }) => ( { @@ -169,8 +178,8 @@ export function ToolControls({ {filteredExcluded.length > 0 && ( <> - {filteredExcluded.map(({ tool, reason }) => ( - + {filteredExcluded.map(({ tool, reason, key }) => ( +