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 `<index>:<name>` 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) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01SW1p8E2uiyyLx4RKwrwSrt
This commit is contained in:
parent
a09f2dcf1d
commit
6f535356b4
2 changed files with 112 additions and 18 deletions
|
|
@ -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(<ControlledToolControls tools={duplicateNameTools} />);
|
||||
|
||||
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(
|
||||
<ControlledToolControls tools={[]} excludedTools={duplicateExcluded} />,
|
||||
);
|
||||
|
||||
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(<ToolControls {...baseProps} />);
|
||||
expect(screen.queryByText(/Couldn't load/)).not.toBeInTheDocument();
|
||||
|
|
|
|||
|
|
@ -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 (
|
||||
<SidebarStack>
|
||||
|
|
@ -156,9 +165,9 @@ export function ToolControls({
|
|||
<ListLoadError error={loadError} what="tools" onRetry={onRefreshList} />
|
||||
<SidebarScroll viewportRef={viewportRef}>
|
||||
<Stack gap="xs">
|
||||
{filteredTools.map((tool) => (
|
||||
{filteredTools.map(({ tool, key }) => (
|
||||
<ToolListItem
|
||||
key={tool.name}
|
||||
key={key}
|
||||
tool={tool}
|
||||
selected={tool.name === selectedName}
|
||||
onClick={() => {
|
||||
|
|
@ -169,8 +178,8 @@ export function ToolControls({
|
|||
{filteredExcluded.length > 0 && (
|
||||
<>
|
||||
<ExcludedDivider />
|
||||
{filteredExcluded.map(({ tool, reason }) => (
|
||||
<ExcludedTooltip key={tool.name} label={reason}>
|
||||
{filteredExcluded.map(({ tool, reason, key }) => (
|
||||
<ExcludedTooltip key={key} label={reason}>
|
||||
<ExcludedRow>
|
||||
<ExcludedWarningIcon>
|
||||
<RiErrorWarningLine />
|
||||
|
|
|
|||
Loading…
Reference in a new issue