diff --git a/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx b/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx index b95c48ed8..5f644c2eb 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.test.tsx @@ -399,6 +399,54 @@ describe("ToolControls", () => { expect(screen.queryByText("dupe")).not.toBeInTheDocument(); }); + // Asserts the given texts appear in this order in the rendered DOM. + const expectInDomOrder = (texts: string[]) => { + const nodes = texts.map((text) => screen.getByText(text)); + for (let i = 1; i < nodes.length; i++) { + expect( + nodes[i - 1]!.compareDocumentPosition(nodes[i]!) & + Node.DOCUMENT_POSITION_FOLLOWING, + ).toBeTruthy(); + } + }; + + it("lists tools alphabetically by their displayed label (#2782)", () => { + // sampleTools arrives as List Files, Query DB, git_status, git_commit. + renderWithMantine(); + expectInDomOrder(["git_commit", "git_status", "List Files", "Query DB"]); + }); + + it("keeps the sorted order while a search narrows the list (#2782)", () => { + renderWithMantine(); + expectInDomOrder(["git_commit", "git_status"]); + }); + + it("reports the source-position row key for a sorted row (#2782)", async () => { + // Sorting moves git_commit (source index 3) to the top; its key must still + // name its position in the server's list, which is what the parent resolves. + const user = userEvent.setup(); + const onSelectTool = vi.fn(); + renderWithMantine( + , + ); + await user.click(screen.getByText("git_commit")); + expect(onSelectTool).toHaveBeenCalledWith("3:git_commit"); + }); + + it("lists excluded tools alphabetically by the name they show (#2782)", () => { + // Titles that would invert the order: an excluded row shows only `name`, + // so sorting by the resolved title would read as unsorted. + const excluded = [ + { name: "zed_tool", title: "Alpha" }, + { name: "alpha_tool", title: "Zulu" }, + ].map((tool) => ({ + tool: { ...tool, inputSchema: { type: "object" as const } }, + reason: "invalid x-mcp-header", + })); + renderWithMantine(); + expectInDomOrder(["alpha_tool", "zed_tool"]); + }); + 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 e7b5f557a..9364df639 100644 --- a/clients/web/src/components/groups/ToolControls/ToolControls.tsx +++ b/clients/web/src/components/groups/ToolControls/ToolControls.tsx @@ -23,7 +23,11 @@ import { } from "../../elements/ListPaginationControls/ListPaginationControls"; import { ToolListItem } from "../ToolListItem/ToolListItem"; import { useScrollMemory } from "../../../hooks/useScrollMemory"; -import { toolRowKey } from "../../../utils/toolUtils"; +import { + compareToolLabels, + compareToolsByLabel, + toolRowKey, +} from "../../../utils/toolUtils"; import { formatCountedLabel, listCountOf } from "../../../utils/listCount"; export interface ToolControlsProps { @@ -151,22 +155,25 @@ export function ToolControls({ }: ToolControlsProps) { const viewportRef = useScrollMemory("tools-sidebar"); const query = searchText.toLowerCase(); - // Stamp each row's source position before filtering, so the key survives the - // list narrowing (#1957). + // Stamp each row's source position before filtering and sorting, so the key + // survives the list narrowing (#1957) and the reordering (#2782). const filteredTools = tools .map((tool, sourceIndex) => ({ tool, key: toolRowKey(tool.name, sourceIndex), })) - .filter(({ tool }) => !searchText || matchesQuery(tool, query)); + .filter(({ tool }) => !searchText || matchesQuery(tool, query)) + .sort((a, b) => compareToolsByLabel(a.tool, b.tool)); // Excluded tools are searchable too, matching name AND title like the main - // list above, so a filtered view stays consistent. + // list above, so a filtered view stays consistent. They sort by `name`, not + // the resolved label, because `name` is all an excluded row shows. const filteredExcluded = excludedTools .map((excluded, sourceIndex) => ({ ...excluded, key: excludedRowKey(excluded.tool.name, sourceIndex), })) - .filter(({ tool }) => !searchText || matchesQuery(tool, query)); + .filter(({ tool }) => !searchText || matchesQuery(tool, query)) + .sort((a, b) => compareToolLabels(a.tool.name, b.tool.name)); // The server's total, not the search-filtered view: the header answers "how // many tools does this server expose" (#2607). Omitted until the first load diff --git a/clients/web/src/utils/toolUtils.test.ts b/clients/web/src/utils/toolUtils.test.ts index 5f7ee4184..483e022e1 100644 --- a/clients/web/src/utils/toolUtils.test.ts +++ b/clients/web/src/utils/toolUtils.test.ts @@ -1,6 +1,8 @@ import { describe, it, expect } from "vitest"; import type { Tool } from "@modelcontextprotocol/client"; import { + compareToolLabels, + compareToolsByLabel, findToolByRowKey, hasInputFields, offersRunAsTask, @@ -26,6 +28,46 @@ describe("resolveDisplayLabel", () => { }); }); +describe("compareToolsByLabel (#2782)", () => { + const tool = (name: string, title?: string): Tool => ({ + name, + ...(title === undefined ? {} : { title }), + inputSchema: { type: "object" }, + }); + const labels = (tools: Tool[]) => + [...tools] + .sort(compareToolsByLabel) + .map((t) => resolveDisplayLabel(t.name, t.title)); + + it("orders by the displayed label: the title when present, else the name", () => { + expect( + labels([tool("zeta", "Alpha"), tool("beta"), tool("aaa", "Gamma")]), + ).toEqual(["Alpha", "beta", "Gamma"]); + }); + + it("ignores case and compares digit runs numerically", () => { + expect( + labels([tool("tool_10"), tool("Tool_2"), tool("apple"), tool("Banana")]), + ).toEqual(["apple", "Banana", "Tool_2", "tool_10"]); + }); + + it("treats labels differing only by case as equal, so a stable sort keeps source order", () => { + expect(compareToolsByLabel(tool("Echo"), tool("echo"))).toBe(0); + expect(compareToolsByLabel(tool("echo"), tool("echo"))).toBe(0); + }); +}); + +describe("compareToolLabels (#2782)", () => { + it("compares raw labels with the same collation the tool comparator uses", () => { + expect(["tool_10", "Tool_2", "apple"].sort(compareToolLabels)).toEqual([ + "apple", + "Tool_2", + "tool_10", + ]); + expect(compareToolLabels("Echo", "echo")).toBe(0); + }); +}); + describe("hasInputFields", () => { const baseTool = (inputSchema: Tool["inputSchema"]): Tool => ({ name: "t", diff --git a/clients/web/src/utils/toolUtils.ts b/clients/web/src/utils/toolUtils.ts index 77afb7df9..fa0fb0593 100644 --- a/clients/web/src/utils/toolUtils.ts +++ b/clients/web/src/utils/toolUtils.ts @@ -16,6 +16,37 @@ export function resolveDisplayLabel(name: string, title?: string): string { return title ?? name; } +// Case-insensitive, with digit runs compared numerically, so `tool_2` sorts +// before `tool_10` and `Git` beside `git` — the order a person scanning the +// list expects, rather than raw code-unit order. +const toolLabelCollator = new Intl.Collator(undefined, { + sensitivity: "base", + numeric: true, +}); + +/** + * Orders two row labels the way the Tools sidebar sorts (#2782). Exported for + * rows that show something other than the resolved label — the excluded rows + * show the bare `name` — so each list sorts by the text it actually displays. + */ +export function compareToolLabels(a: string, b: string): number { + return toolLabelCollator.compare(a, b); +} + +/** + * Orders tools by the label their row shows ({@link resolveDisplayLabel}: the + * `title` when present, else the `name`), so the Tools sidebar reads + * alphabetically however the server ordered `tools/list` (#2782). Ties return + * 0, and `Array.prototype.sort` is stable, so duplicates keep their source + * order. + */ +export function compareToolsByLabel(a: Tool, b: Tool): number { + return compareToolLabels( + resolveDisplayLabel(a.name, a.title), + resolveDisplayLabel(b.name, b.title), + ); +} + /** * True when the tool's input schema declares at least one property — used by * App-flow callers to decide whether to render a form or auto-launch. Kept in