ui: fix MCP server display name conflicts in tools lists (#26011)

* ui: fix MCP server display name conflicts in tools lists Tool groups were keyed by display label so two servers reporting the same name broke the keyed each blocks and only one was visible. Key rendering, expand state and toggles by the stable server id instead, and suffix duplicate labels with a counter in config order. * ui: customizable MCP server display name with autofill Add a display name field to the MCP server form, add and edit alike. The custom name takes precedence over the server-reported one, so two servers reporting the same name can be told apart; clearing the field returns to the automatic label. In the add dialog a debounced preview handshake prefills the field with the server-reported name: a manual edit freezes the autofill, stale responses are discarded, failures stay silent, and an unedited prefill is not persisted so the label keeps following the server. * ui: fix recursive fetch passthrough in the client test setup The original fetch was captured inside beforeEach, where it is the previous test's spy since vi.spyOn returns the existing one, so the default passthrough recursed on itself for any URL outside the mocked set. Capture the real fetch once at module load.
2026-07-24 19:28:14 +02:00
parent 27209a598d
commit 95a923a64c
15 changed files with 257 additions and 35 deletions
@@ -299,7 +299,7 @@

 						<Collapsible.Content>
 							<div class="flex flex-col gap-0.5 pl-4">
-								{#each toolsPanel.activeGroups as group (group.label)}
+								{#each toolsPanel.activeGroups as group (group.key)}
 									{@const checked = toolsPanel.isGroupChecked(group)}
 									{@const enabledCount = toolsPanel.getEnabledToolCount(group)}
 									{@const favicon = toolsPanel.getFavicon(group)}
@@ -307,7 +307,7 @@
 									<button
 										type="button"
 										class={sheetItemRowClass}
-										onclick={() => toolsPanel.toggleGroupByLabel(group.label)}
+										onclick={() => toolsPanel.toggleGroupByKey(group.key)}
 									>
 										{#if favicon}
 											<img
@@ -330,7 +330,7 @@
 											{checked}
 											class="{ICON_CLASS_DEFAULT} shrink-0"
 											onclick={(e) => e.stopPropagation()}
-											onCheckedChange={() => toolsPanel.toggleGroupByLabel(group.label)}
+											onCheckedChange={() => toolsPanel.toggleGroupByKey(group.key)}
 										/>
 									</button>
 								{/each}
@@ -64,14 +64,14 @@
 			{/if}
 		{:else}
 			<div class="max-h-80 overflow-y-auto p-2 pr-1">
-				{#each toolsPanel.activeGroups as group (group.label)}
-					{@const isExpanded = toolsPanel.expandedGroups.has(group.label)}
+				{#each toolsPanel.activeGroups as group (group.key)}
+					{@const isExpanded = toolsPanel.expandedGroups.has(group.key)}
 					{@const checked = toolsPanel.isGroupChecked(group)}
 					{@const favicon = toolsPanel.getFavicon(group)}

 					<Collapsible.Root
 						open={isExpanded}
-						onOpenChange={() => toolsPanel.toggleGroupExpanded(group.label)}
+						onOpenChange={() => toolsPanel.toggleGroupExpanded(group.key)}
 					>
 						<div class="flex items-center gap-1">
 							<Collapsible.Trigger
@@ -109,7 +109,7 @@
 										<Checkbox
 											{...props}
 											{checked}
-											onCheckedChange={() => toolsPanel.toggleGroupByLabel(group.label)}
+											onCheckedChange={() => toolsPanel.toggleGroupByKey(group.key)}
 											class="mr-2 {ICON_CLASS_DEFAULT} shrink-0"
 										/>
 									{/snippet}
@@ -15,6 +15,7 @@
 		REDACTED_HEADERS
 	} from '$lib/constants';
 	import { browser } from '$app/environment';
+	import { HealthCheckStatus } from '$lib/enums';

 	interface Props {
 		open: boolean;
@@ -24,6 +25,16 @@
 	let { open = $bindable(), onOpenChange }: Props = $props();

 	let newServerUrl = $state('');
+	let newServerName = $state('');
+	let nameAutoFilled = $state('');
+	let nameTouched = $state(false);
+
+	let previewRun = 0;
+
+	function handleNameChange(value: string) {
+		newServerName = value;
+		nameTouched = true;
+	}
 	let newServerHeaders = $state('');
 	let newServerUseProxy = $state(false);

@@ -115,6 +126,48 @@
 		}
 	});

+	// Debounced preview handshake: once the URL is valid and stable, fetch the
+	// server-reported name to prefill the display name field. A manual edit
+	// freezes the autofill for good, and failures stay silent.
+	$effect(() => {
+		const url = newServerUrl.trim();
+		const headers = newServerHeaders.trim();
+		const useProxy = newServerUseProxy;
+
+		if (!open || newServerUrlError || !url) return;
+
+		const run = ++previewRun;
+		// One throwaway id per run: concurrent previews (URL typed, then the
+		// bearer token pasted) would poison each other's shared health state.
+		const previewId = `${MCP_SERVER_ID_PREFIX}-preview-${run}`;
+		const timer = setTimeout(async () => {
+			await mcpStore.runHealthCheck({
+				id: previewId,
+				enabled: false,
+				url,
+				headers: headers || undefined,
+				useProxy
+			});
+
+			const state = mcpStore.getHealthCheckState(previewId);
+
+			mcpStore.clearHealthCheck(previewId);
+
+			if (run !== previewRun) return;
+
+			if (state.status !== HealthCheckStatus.SUCCESS) return;
+
+			const autoName = state.serverInfo?.title || state.serverInfo?.name || '';
+
+			if (autoName && !nameTouched) {
+				newServerName = autoName;
+				nameAutoFilled = autoName;
+			}
+		}, 600);
+
+		return () => clearTimeout(timer);
+	});
+
 	let hasSelection = $derived(selectedRecommendationId !== null);

 	let unconfiguredRecommendations = $derived.by(() => {
@@ -146,6 +199,10 @@
 	function handleOpenChange(value: boolean) {
 		if (!value) {
 			newServerUrl = '';
+			newServerName = '';
+			nameAutoFilled = '';
+			nameTouched = false;
+			previewRun++;
 			newServerHeaders = '';
 			newServerUseProxy = false;
 			newServerWantsAuthorization = false;
@@ -163,6 +220,12 @@
 			id: newServerId,
 			enabled: true,
 			url: newServerUrl.trim(),
+			// A name equal to the autofilled server-reported one is not a
+			// customization: keep following the automatic label.
+			displayName:
+				newServerName.trim() && newServerName.trim() !== nameAutoFilled.trim()
+					? newServerName.trim()
+					: undefined,
 			headers: newServerHeaders.trim() || undefined,
 			useProxy: newServerUseProxy
 		});
@@ -210,6 +273,8 @@
 			<div class="space-y-4 py-4">
 				<McpServerForm
 					url={newServerUrl}
+					name={newServerName}
+					onNameChange={handleNameChange}
 					headers={newServerHeaders}
 					useProxy={newServerUseProxy}
 					onUrlChange={(v) => (newServerUrl = v)}
@@ -70,7 +70,12 @@
 	async function startEditing() {
 		isEditing = true;
 		await tick();
-		editFormRef?.setInitialValues(server.url, server.headers || '', server.useProxy || false);
+		editFormRef?.setInitialValues(
+			server.url,
+			server.headers || '',
+			server.useProxy || false,
+			displayName
+		);
 	}

 	function cancelEditing() {
@@ -81,9 +86,12 @@
 		}
 	}

-	function saveEditing(url: string, headers: string, useProxy: boolean) {
+	function saveEditing(url: string, headers: string, useProxy: boolean, name?: string) {
 		onUpdate({
 			url: url,
+			// undefined = prefill untouched, keep any existing custom name;
+			// empty string = field cleared, back to the automatic label
+			displayName: name === undefined ? server.displayName : name.trim() || undefined,
 			headers: headers || undefined,
 			useProxy: useProxy
 		});
@@ -106,6 +114,7 @@
 			serverId={server.id}
 			serverUrl={server.url}
 			serverUseProxy={server.useProxy}
+			serverLabel={displayName}
 			onSave={saveEditing}
 			onCancel={cancelEditing}
 		/>
@@ -7,13 +7,23 @@
 		serverId: string;
 		serverUrl: string;
 		serverUseProxy?: boolean;
-		onSave: (url: string, headers: string, useProxy: boolean) => void;
+		/** Current automatic label, prefilled so the user can customize it. */
+		serverLabel?: string;
+		onSave: (url: string, headers: string, useProxy: boolean, name?: string) => void;
 		onCancel: () => void;
 	}

-	let { serverId, serverUrl, serverUseProxy = false, onSave, onCancel }: Props = $props();
+	let {
+		serverId,
+		serverUrl,
+		serverUseProxy = false,
+		serverLabel = '',
+		onSave,
+		onCancel
+	}: Props = $props();

 	let editUrl = $derived(serverUrl);
+	let editName = $derived(serverLabel);
 	let editHeaders = $state('');
 	let editUseProxy = $derived(serverUseProxy);

@@ -34,7 +44,12 @@

 	function handleSave() {
 		if (!canSave) return;
-		onSave(editUrl.trim(), editHeaders.trim(), editUseProxy);
+
+		// An unchanged prefill keeps following the automatic label; only an
+		// actual edit becomes a persisted custom display name.
+		const name = editName.trim() !== serverLabel.trim() ? editName.trim() : undefined;
+
+		onSave(editUrl.trim(), editHeaders.trim(), editUseProxy, name);
 	}

 	function handleSubmit(event: SubmitEvent) {
@@ -42,10 +57,11 @@
 		handleSave();
 	}

-	export function setInitialValues(url: string, headers: string, useProxy: boolean) {
+	export function setInitialValues(url: string, headers: string, useProxy: boolean, name = '') {
 		editUrl = url;
 		editHeaders = headers;
 		editUseProxy = useProxy;
+		editName = name;
 	}
 </script>

@@ -55,6 +71,8 @@

 		<McpServerForm
 			url={editUrl}
+			name={editName}
+			onNameChange={(v) => (editName = v)}
 			headers={editHeaders}
 			useProxy={editUseProxy}
 			onUrlChange={(v) => (editUrl = v)}
@@ -17,6 +17,10 @@
 	interface Props {
 		url: string;
 		headers: string;
+		name?: string;
+		onNameChange?: (name: string) => void;
+		/** Shown in the empty display name field, e.g. the current automatic label. */
+		namePlaceholder?: string;
 		useProxy?: boolean;
 		onUrlChange: (url: string) => void;
 		onHeadersChange: (headers: string) => void;
@@ -44,6 +48,9 @@
 	let {
 		url,
 		headers,
+		name = '',
+		onNameChange,
+		namePlaceholder = 'Name reported by the server',
 		useProxy = false,
 		onUrlChange,
 		onHeadersChange,
@@ -156,6 +163,20 @@
 		{/if}
 	</div>

+	<div class="mb-4">
+		<label for="server-name-{id}" class="mb-2 block text-xs font-medium select-none">
+			Display name
+		</label>
+
+		<Input
+			id="server-name-{id}"
+			type="text"
+			placeholder={namePlaceholder}
+			value={name}
+			oninput={(e) => onNameChange?.(e.currentTarget.value)}
+		/>
+	</div>
+
 	<label class="flex items-center gap-2 cursor-pointer select-none">
 		<Switch
 			id="use-authorization-{id}"
@@ -14,11 +14,11 @@
 	let expandedGroups = new SvelteSet<string>();
 	let groups = $derived(toolsStore.toolGroups);

-	function toggleExpanded(label: string) {
-		if (expandedGroups.has(label)) {
-			expandedGroups.delete(label);
+	function toggleExpanded(key: string) {
+		if (expandedGroups.has(key)) {
+			expandedGroups.delete(key);
 		} else {
-			expandedGroups.add(label);
+			expandedGroups.add(key);
 		}
 	}
 </script>
@@ -27,9 +27,9 @@
 	<div class="py-8 text-center text-sm text-muted-foreground">No tools available</div>
 {:else}
 	<div class="space-y-2">
-		{#each groups as group (group.label)}
-			{@const isExpanded = expandedGroups.has(group.label)}
-			<Collapsible.Root open={isExpanded} onOpenChange={() => toggleExpanded(group.label)}>
+		{#each groups as group (group.key)}
+			{@const isExpanded = expandedGroups.has(group.key)}
+			<Collapsible.Root open={isExpanded} onOpenChange={() => toggleExpanded(group.key)}>
 				<Collapsible.Trigger
 					class="flex w-full items-center gap-2 rounded-lg px-3 py-2 text-sm hover:bg-muted/50"
 				>
@@ -16,9 +16,9 @@ export interface UseToolsPanelReturn {
 	getEnabledToolCount(group: ToolGroup): number;
 	getFavicon(group: ToolGroup): string | null;
 	isGroupDisabled(group: ToolGroup): boolean;
-	toggleGroupExpanded(label: string): void;
-	/** Toggle all tools in a group by label (avoids stale group object references). */
-	toggleGroupByLabel(label: string): void;
+	toggleGroupExpanded(key: string): void;
+	/** Toggle all tools in a group by its stable key (avoids stale group object references). */
+	toggleGroupByKey(key: string): void;
 	handleOpen(): void;
 }

@@ -76,17 +76,17 @@ export function useToolsPanel(): UseToolsPanelReturn {
 		);
 	}

-	function toggleGroupExpanded(label: string): void {
-		if (expandedGroups.has(label)) {
-			expandedGroups.delete(label);
+	function toggleGroupExpanded(key: string): void {
+		if (expandedGroups.has(key)) {
+			expandedGroups.delete(key);
 		} else {
-			expandedGroups.add(label);
+			expandedGroups.add(key);
 		}
 	}

-	function toggleGroupByLabel(label: string): void {
-		// Find current group by label to get up-to-date tool references
-		const group = activeGroups.find((g) => g.label === label);
+	function toggleGroupByKey(key: string): void {
+		// Find current group by key to get up-to-date tool references
+		const group = activeGroups.find((g) => g.key === key);
 		if (!group) return;
 		toolsStore.toggleGroup(group);
 	}
@@ -117,7 +117,7 @@ export function useToolsPanel(): UseToolsPanelReturn {
 		getFavicon,
 		isGroupDisabled,
 		toggleGroupExpanded,
-		toggleGroupByLabel,
+		toggleGroupByKey,
 		handleOpen
 	};
 }
@@ -148,6 +148,7 @@ class MCPStore {
 				enabled: Boolean((entry as { enabled?: unknown })?.enabled),
 				url,
 				name: (entry as { name?: string })?.name,
+				displayName: (entry as { displayName?: string })?.displayName,
 				headers: headers || undefined,
 				useProxy: Boolean((entry as { useProxy?: unknown })?.useProxy)
 			} satisfies MCPServerSettingsEntry;
@@ -375,14 +376,38 @@ class MCPStore {
 		return this.connections;
 	}

-	getServerLabel(server: MCPServerDisplayInfo): string {
+	/**
+	 * Resolves the raw label for a server: user-defined display name first,
+	 * then server-reported title or name when the health check succeeded,
+	 * then the configured name (admin baseline or legacy data), then URL.
+	 */
+	#serverBaseLabel(server: MCPServerDisplayInfo): string {
+		if (server.displayName) return server.displayName;
+
 		const healthState = this.getHealthCheckState(server.id);

 		if (healthState?.status === HealthCheckStatus.SUCCESS)
 			return (
 				healthState.serverInfo?.title || healthState.serverInfo?.name || server.name || server.url
 			);
-		return server.url;
+		return server.name || server.url;
+	}
+
+	/**
+	 * Returns the display label for a server, suffixed with a positional
+	 * counter when several configured servers resolve to the same base label
+	 * (e.g. two endpoints of the same host reporting an identical name).
+	 * Numbering follows config order, so it is stable across renders.
+	 */
+	getServerLabel(server: MCPServerDisplayInfo): string {
+		const label = this.#serverBaseLabel(server);
+		const twins = this.getServers().filter((s) => this.#serverBaseLabel(s) === label);
+
+		if (twins.length < 2) return label;
+
+		const position = twins.findIndex((s) => s.id === server.id);
+
+		return position < 0 ? label : `${label} (${position + 1})`;
 	}

 	getServerById(serverId: string): MCPServerSettingsEntry | undefined {
@@ -517,6 +542,7 @@ class MCPStore {
 			enabled: serverData.enabled,
 			url: serverData.url.trim(),
 			name: serverData.name,
+			displayName: serverData.displayName,
 			headers: serverData.headers?.trim() || undefined,
 			useProxy: serverData.useProxy
 		};
@@ -282,6 +282,7 @@ class ToolsStore {
 			if (!group) {
 				group = {
 					source: entry.source,
+					key: groupKey,
 					label: this.groupLabel(entry),
 					serverId: entry.serverId,
 					tools: []
@@ -214,6 +214,8 @@ export type MCPToolCall = {
 export interface MCPServerDisplayInfo {
 	id: string;
 	name?: string;
+	/** User-defined display name, takes precedence over every automatic label. */
+	displayName?: string;
 	url: string;
 }

@@ -14,6 +14,8 @@ export interface ToolEntry {

 export interface ToolGroup {
 	source: ToolSource;
+	/** Stable identity for keyed rendering and toggles, unique per group */
+	key: string;
 	label: string;
 	/** For MCP groups, the server ID */
 	serverId?: string;
@@ -101,6 +101,7 @@ export function parseMcpServerSettings(rawServers: unknown): MCPServerSettingsEn
 			enabled: Boolean((entry as { enabled?: unknown })?.enabled),
 			url,
 			name: (entry as { name?: string })?.name,
+			displayName: (entry as { displayName?: string })?.displayName,
 			headers: headers || undefined,
 			useProxy: Boolean((entry as { useProxy?: unknown })?.useProxy)
 		} satisfies MCPServerSettingsEntry;
@@ -0,0 +1,73 @@
+import { beforeEach, describe, expect, it } from 'vitest';
+import { render } from 'vitest-browser-svelte';
+import { McpServerForm } from '$lib/components/app/mcp';
+import { mcpStore } from '$lib/stores/mcp.svelte';
+import { settingsStore } from '$lib/stores/settings.svelte';
+
+describe('mcp server display name', () => {
+	beforeEach(() => {
+		settingsStore.updateConfig('mcpServers', '[]');
+	});
+
+	it('custom display name wins over the url fallback', () => {
+		const server = mcpStore.addServer({
+			enabled: false,
+			url: 'https://mcp.example.com/a',
+			displayName: 'My Tools'
+		});
+		expect(mcpStore.getServerLabel(server)).toBe('My Tools');
+	});
+
+	it('without a custom name the url is the label', () => {
+		const server = mcpStore.addServer({ enabled: false, url: 'https://mcp.example.com/a' });
+		expect(mcpStore.getServerLabel(server)).toBe('https://mcp.example.com/a');
+	});
+
+	it('identical labels get positional suffixes', () => {
+		const a = mcpStore.addServer({
+			enabled: false,
+			url: 'https://mcp.example.com/a',
+			displayName: 'GitHub'
+		});
+		const b = mcpStore.addServer({
+			enabled: false,
+			url: 'https://mcp.example.com/b',
+			displayName: 'GitHub'
+		});
+		expect(mcpStore.getServerLabel(a)).toBe('GitHub (1)');
+		expect(mcpStore.getServerLabel(b)).toBe('GitHub (2)');
+	});
+
+	it('renaming one twin dissolves the suffixes', () => {
+		const a = mcpStore.addServer({
+			enabled: false,
+			url: 'https://mcp.example.com/a',
+			displayName: 'GitHub'
+		});
+		const b = mcpStore.addServer({
+			enabled: false,
+			url: 'https://mcp.example.com/b',
+			displayName: 'GitHub'
+		});
+		mcpStore.updateServer(b.id, { displayName: 'GitHub Work' });
+		expect(mcpStore.getServerLabel(a)).toBe('GitHub');
+		expect(mcpStore.getServerLabel(mcpStore.getServerById(b.id)!)).toBe('GitHub Work');
+	});
+
+	it('the form exposes an editable display name field', async () => {
+		let captured = '';
+		const screen = await render(McpServerForm, {
+			url: 'https://mcp.example.com/a',
+			headers: '',
+			name: '',
+			onUrlChange: () => {},
+			onHeadersChange: () => {},
+			onNameChange: (v: string) => (captured = v)
+		});
+
+		const input = screen.getByLabelText('Display name');
+		await expect.element(input).toBeVisible();
+		await input.fill('My Custom Server');
+		expect(captured).toBe('My Custom Server');
+	});
+});
@@ -6,9 +6,13 @@ import { beforeEach, vi } from 'vitest';
 // Mock fetch for API calls during client tests.
 // In test environment there is no backend server, so we intercept
 // the specific endpoints the app uses and return valid mock data.
-beforeEach(() => {
-	const originalFetch = globalThis.fetch;
+// The passthrough target is captured once at module load: capturing it
+// inside beforeEach grabs the previous test's spy (vi.spyOn returns the
+// existing spy), making the default branch recurse on itself as soon as
+// a test fetches a URL outside the mocked set.
+const originalFetch = globalThis.fetch.bind(globalThis);

+beforeEach(() => {
 	vi.spyOn(globalThis, 'fetch').mockImplementation(
 		async (input: RequestInfo | URL, init?: RequestInit) => {
 			const url = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url;