refactor: simplify mcp tool editor connection flow around 401 signal

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

frontend/src/lib/components/flows/content/McpOAuthConnect.svelte

@@ -17,26 +17,19 @@
 	interface Props {
 		onConnected: (resourcePath: string, resourceName: string) => void
 		onCancel?: () => void
-		onDiscoveryStatus?: (
-			status: 'discovering' | 'supported' | 'unsupported',
-			error?: string
-		) => void
 		initialServerUrl?: string
 		initialResourceName?: string
 		initialResourcePath?: string
 		updateExistingResource?: boolean
-		hideUntilDiscovered?: boolean
 	}
 
 	let {
 		onConnected,
 		onCancel,
-		onDiscoveryStatus,
 		initialServerUrl = '',
 		initialResourceName = '',
 		initialResourcePath = '',
-		updateExistingResource = false,
-		hideUntilDiscovered = false
+		updateExistingResource = false
 	}: Props = $props()
 
 	let serverUrl = $state('')
@@ -53,9 +46,6 @@
 	let connectDisabled = $derived(
 		!resourcePath || (!updateExistingResource && pathError !== '')
 	)
-	let shouldRender = $derived(
-		!hideUntilDiscovered || status === 'discovered' || status === 'connecting'
-	)
 
 	initializeFromProps()
 
@@ -77,7 +67,6 @@
 	async function discoverOAuth() {
 		status = 'discovering'
 		error = null
-		onDiscoveryStatus?.('discovering')
 		try {
 			discoveryResult = await McpOauthService.discoverMcpOauth({
 				requestBody: { mcp_server_url: serverUrl }
@@ -92,13 +81,11 @@
 				}
 			}
 			status = 'discovered'
-			onDiscoveryStatus?.('supported')
 		} catch (e: any) {
 			console.error('Error discovering OAuth settings', e)
 			const errorMessage = e.body?.message || e.body || e.message || 'Unknown error'
 			error = `Failed to discover OAuth settings: ${errorMessage}`
 			status = 'unsupported'
-			onDiscoveryStatus?.('unsupported', error)
 		}
 	}
 
@@ -289,105 +276,103 @@
 	onDestroy(cleanup)
 </script>
 
-{#if shouldRender}
-	<div class="border rounded p-4 bg-surface-secondary flex flex-col gap-4">
-		<div class="flex justify-between items-center">
-			<span class="font-semibold text-sm">
-				{updateExistingResource
-					? 'Connect selected MCP resource with OAuth'
-					: 'Connect MCP Server with OAuth'}
-			</span>
-			{#if onCancel}
-				<Button size="xs" color="light" onClick={onCancel}>Cancel</Button>
+<div class="border rounded p-4 bg-surface-secondary flex flex-col gap-4">
+	<div class="flex justify-between items-center">
+		<span class="font-semibold text-sm">
+			{updateExistingResource
+				? 'Connect selected MCP resource with OAuth'
+				: 'Connect MCP Server with OAuth'}
+		</span>
+		{#if onCancel}
+			<Button size="xs" color="light" onClick={onCancel}>Cancel</Button>
+		{/if}
+	</div>
+
+	<Label label="MCP Server URL">
+		<input
+			type="url"
+			bind:value={serverUrl}
+			placeholder="https://mcp.example.com"
+			class="text-sm w-full"
+			disabled={status === 'connecting'}
+		/>
+	</Label>
+
+	{#if status === 'idle'}
+		<Button size="sm" onClick={discoverOAuth} disabled={!serverUrl}>Discover OAuth Settings</Button>
+	{:else if status === 'discovering'}
+		<div class="text-sm text-secondary">Discovering OAuth settings...</div>
+	{:else if status === 'unsupported'}
+		<div class="flex flex-col gap-1 text-xs text-red-600 dark:text-red-400">
+			<div>OAuth is not available for this MCP server.</div>
+			{#if error}
+				<div>{error}</div>
+			{/if}
+		</div>
+	{:else if status === 'discovered' && discoveryResult}
+		<div class="text-xs text-green-600 dark:text-green-400">
+			&#10003; OAuth supported
+			{#if discoveryResult.supports_dynamic_registration}
+				(Dynamic Client Registration available)
 			{/if}
 		</div>
 
-		<Label label="MCP Server URL">
+		{#if discoveryResult.scopes_supported && discoveryResult.scopes_supported.length > 0}
+			<Label label="Select Scopes">
+				<div class="flex flex-col flex-wrap gap-2">
+					{#each discoveryResult.scopes_supported as scope (scope)}
+						<label class="flex flex-row items-center gap-2 text-xs cursor-pointer">
+							<input
+								type="checkbox"
+								checked={selectedScopes.includes(scope)}
+								onchange={(e) => {
+									const target = e.target as HTMLInputElement
+									if (target.checked) {
+										selectedScopes = [...selectedScopes, scope]
+									} else {
+										selectedScopes = selectedScopes.filter((s) => s !== scope)
+									}
+								}}
+								class="!w-4 !h-4"
+							/>
+							{scope}
+						</label>
+					{/each}
+				</div>
+			</Label>
+		{/if}
+
+		<Label label="Resource Name">
 			<input
-				type="url"
-				bind:value={serverUrl}
-				placeholder="https://mcp.example.com"
+				type="text"
+				bind:value={resourceName}
+				placeholder="my-mcp-server"
 				class="text-sm w-full"
-				disabled={status === 'connecting'}
 			/>
 		</Label>
 
-		{#if status === 'idle'}
-			<Button size="sm" onClick={discoverOAuth} disabled={!serverUrl}>Discover OAuth Settings</Button>
-		{:else if status === 'discovering'}
-			<div class="text-sm text-secondary">Discovering OAuth settings...</div>
-		{:else if status === 'unsupported'}
-			<div class="flex flex-col gap-1 text-xs text-red-600 dark:text-red-400">
-				<div>OAuth is not available for this MCP server.</div>
-				{#if error}
-					<div>{error}</div>
-				{/if}
-			</div>
-		{:else if status === 'discovered' && discoveryResult}
-			<div class="text-xs text-green-600 dark:text-green-400">
-				&#10003; OAuth supported
-				{#if discoveryResult.supports_dynamic_registration}
-					(Dynamic Client Registration available)
-				{/if}
-			</div>
-
-			{#if discoveryResult.scopes_supported && discoveryResult.scopes_supported.length > 0}
-				<Label label="Select Scopes">
-					<div class="flex flex-col flex-wrap gap-2">
-						{#each discoveryResult.scopes_supported as scope (scope)}
-							<label class="flex flex-row items-center gap-2 text-xs cursor-pointer">
-								<input
-									type="checkbox"
-									checked={selectedScopes.includes(scope)}
-									onchange={(e) => {
-										const target = e.target as HTMLInputElement
-										if (target.checked) {
-											selectedScopes = [...selectedScopes, scope]
-										} else {
-											selectedScopes = selectedScopes.filter((s) => s !== scope)
-										}
-									}}
-									class="!w-4 !h-4"
-								/>
-								{scope}
-							</label>
-						{/each}
-					</div>
-				</Label>
-			{/if}
-
-			<Label label="Resource Name">
-				<input
-					type="text"
-					bind:value={resourceName}
-					placeholder="my-mcp-server"
-					class="text-sm w-full"
-				/>
+		{#if updateExistingResource}
+			<Label label="Resource Path">
+				<div class="text-sm w-full rounded border bg-surface px-2 py-1">{resourcePath}</div>
 			</Label>
-
-			{#if updateExistingResource}
-				<Label label="Resource Path">
-					<div class="text-sm w-full rounded border bg-surface px-2 py-1">{resourcePath}</div>
-				</Label>
-			{:else}
-				<Path
-					bind:path={resourcePath}
-					bind:error={pathError}
-					initialPath=""
-					namePlaceholder={resourceName}
-					kind="resource"
-				/>
-			{/if}
-
-			<Button size="sm" onClick={startOAuth} disabled={connectDisabled}>
-				Connect with OAuth
-			</Button>
-		{:else if status === 'connecting'}
-			<div class="text-sm text-secondary">Complete authentication in popup window...</div>
+		{:else}
+			<Path
+				bind:path={resourcePath}
+				bind:error={pathError}
+				initialPath=""
+				namePlaceholder={resourceName}
+				kind="resource"
+			/>
 		{/if}
 
-		{#if error && status !== 'unsupported'}
-			<div class="text-xs text-red-600 dark:text-red-400">{error}</div>
-		{/if}
-	</div>
-{/if}
+		<Button size="sm" onClick={startOAuth} disabled={connectDisabled}>
+			Connect with OAuth
+		</Button>
+	{:else if status === 'connecting'}
+		<div class="text-sm text-secondary">Complete authentication in popup window...</div>
+	{/if}
+
+	{#if error && status !== 'unsupported'}
+		<div class="text-xs text-red-600 dark:text-red-400">{error}</div>
+	{/if}
+</div>

frontend/src/lib/components/flows/content/McpToolEditor.svelte

@@ -34,36 +34,6 @@
 		tool: McpTool
 	}
 
-	type McpResourceValue = {
-		name?: string
-		url: string
-		token?: string | null
-	}
-
-	type OAuthDiscoveryStatus = 'none' | 'discovering' | 'supported' | 'unsupported'
-
-	function getMcpResourceValue(value: unknown): McpResourceValue | undefined {
-		if (!value || typeof value !== 'object') {
-			return undefined
-		}
-
-		const candidate = value as Record<string, unknown>
-		const url = typeof candidate.url === 'string' ? candidate.url : undefined
-
-		if (!url) {
-			return undefined
-		}
-
-		return {
-			name: typeof candidate.name === 'string' ? candidate.name : undefined,
-			url,
-			token:
-				typeof candidate.token === 'string' || candidate.token === null
-					? candidate.token
-					: undefined
-		}
-	}
-
 	function getFallbackMcpResourceName(path: string, url?: string): string {
 		if (path) {
 			const parts = path.split('/')
@@ -85,23 +55,10 @@
 		return error?.body?.message || error?.body || error?.message
 	}
 
-	function isUnauthorizedError(error: any): boolean {
-		const status = error?.status
-		const message = String(getErrorMessage(error) ?? '')
-
-		return (
-			status === 401 ||
-			status === 403 ||
-			/\b(unauthorized|unauthorised|401|403)\b/i.test(message)
-		)
-	}
-
 	let { tool = $bindable() }: Props = $props()
 
 	let refreshCount = $state(0)
 	let resourcePicker: ResourcePicker | undefined = $state()
-	let lastResourcePath = $state<string | undefined>()
-	let oauthDiscoveryStatus = $state<OAuthDiscoveryStatus>('none')
 
 	let resourcePath = $derived(tool.value.resource_path)
 
@@ -128,45 +85,12 @@
 
 	let toolOptions = $derived(safeSelectItems((tools.value ?? []).map((t) => t.name)))
 	let error = $derived(getErrorMessage(tools.error))
-	let selectedMcpResourceValue = $derived(getMcpResourceValue(selectedResource.value?.value))
-	let selectedMcpHasToken = $derived(
-		typeof selectedMcpResourceValue?.token === 'string' &&
-			selectedMcpResourceValue.token.trim().length > 0
-	)
-	let selectedMcpUrlOnly = $derived(Boolean(selectedMcpResourceValue?.url && !selectedMcpHasToken))
-	let selectedMcpServerUrl = $derived(selectedMcpResourceValue?.url)
-	let selectedMcpResourceName = $derived(
-		selectedMcpResourceValue?.name ||
-			getFallbackMcpResourceName(resourcePath, selectedMcpServerUrl)
-	)
-	let selectedMcpUnauthorized = $derived(
-		Boolean(selectedMcpUrlOnly && isUnauthorizedError(tools.error))
-	)
-	let connectionEstablished = $derived(
-		Boolean(resourcePath?.length > 0 && tools.status !== 'loading' && !tools.error && tools.value)
-	)
-	let oauthDiscoveryPending = $derived(
-		selectedMcpUnauthorized &&
-			(oauthDiscoveryStatus === 'none' || oauthDiscoveryStatus === 'discovering')
-	)
-	let showConnecting = $derived(
-		Boolean(
-			resourcePath?.length > 0 &&
-				!connectionEstablished &&
-				(tools.status === 'loading' || selectedResource.status === 'loading' || oauthDiscoveryPending)
-		)
-	)
-	let showOAuthFlow = $derived(
-		selectedMcpUnauthorized && oauthDiscoveryStatus !== 'unsupported'
-	)
-	let connectionError = $derived(
-		resourcePath?.length > 0 &&
-			!connectionEstablished &&
-			!showConnecting &&
-			(!selectedMcpUnauthorized || oauthDiscoveryStatus === 'unsupported')
-			? error
-			: undefined
-	)
+	// The backend returns 401 (NotAuthorized) when the MCP server requires authentication.
+	let unauthorized = $derived(tools.error?.status === 401)
+
+	let resourceValue = $derived(selectedResource.value?.value as { url?: string; name?: string } | undefined)
+	let serverUrl = $derived(resourceValue?.url)
+	let resourceName = $derived(resourceValue?.name || getFallbackMcpResourceName(resourcePath, serverUrl))
 
 	$effect(() => {
 		resourcePath
@@ -191,22 +115,6 @@
 		})
 	})
 
-	$effect(() => {
-		const currentPath = resourcePath
-		untrack(() => {
-			if (currentPath !== lastResourcePath) {
-				lastResourcePath = currentPath
-				oauthDiscoveryStatus = 'none'
-			}
-		})
-	})
-
-	$effect(() => {
-		if (!selectedMcpUnauthorized && oauthDiscoveryStatus !== 'none') {
-			oauthDiscoveryStatus = 'none'
-		}
-	})
-
 	$effect(() => {
 		if (!tool.value.include_tools) {
 			tool.value.include_tools = []
@@ -222,15 +130,10 @@
 		}
 	})
 
-	function handleOAuthDiscoveryStatus(status: Exclude<OAuthDiscoveryStatus, 'none'>) {
-		oauthDiscoveryStatus = status
-	}
-
-	async function handleOAuthConnected(connectedResourcePath: string, resourceName: string) {
+	async function handleOAuthConnected(connectedResourcePath: string, connectedResourceName: string) {
 		await resourcePicker?.refreshResources()
 		tool.value.resource_path = connectedResourcePath
-		tool.summary = `MCP: ${resourceName}`
-		oauthDiscoveryStatus = 'none'
+		tool.summary = `MCP: ${connectedResourceName}`
 		selectedResource.refresh()
 		refreshCount += 1
 	}
@@ -257,95 +160,89 @@
 		</Label>
 	</div>
 
-	{#if showConnecting}
-		<div class="text-xs text-secondary italic">connecting to mcp server...</div>
-	{/if}
-
-	{#if showOAuthFlow}
-		{#key `${resourcePath ?? ''}:${selectedMcpServerUrl ?? ''}`}
-			<McpOAuthConnect
-				onConnected={handleOAuthConnected}
-				onDiscoveryStatus={handleOAuthDiscoveryStatus}
-				initialServerUrl={selectedMcpServerUrl}
-				initialResourceName={selectedMcpResourceName}
-				initialResourcePath={resourcePath}
-				updateExistingResource={true}
-				hideUntilDiscovered={true}
-			/>
-		{/key}
-	{/if}
-
-	{#if connectionError}
-		<div class="text-xs text-red-600 dark:text-red-400">{`Failed to connect to MCP server: ${connectionError}`}</div>
-	{/if}
-
-	{#if connectionEstablished}
-		<div class="w-full">
-			<Label label="Summary">
-				<input
-					type="text"
-					bind:value={tool.summary}
-					placeholder="e.g., GitHub MCP"
-					class="text-sm w-full"
+	{#if resourcePath?.length > 0}
+		{#if tools.status === 'loading' || selectedResource.status === 'loading'}
+			<div class="text-xs text-secondary italic">connecting to mcp server...</div>
+		{:else if unauthorized}
+			{#key `${resourcePath}:${serverUrl ?? ''}`}
+				<McpOAuthConnect
+					onConnected={handleOAuthConnected}
+					initialServerUrl={serverUrl}
+					initialResourceName={resourceName}
+					initialResourcePath={resourcePath}
+					updateExistingResource={true}
 				/>
-			</Label>
-		</div>
-
-		<Section label="Available Tools">
-			{#snippet action()}
-				<Button
-					size="xs"
-					color="light"
-					onClick={() => (refreshCount += 1)}
-					startIcon={{ icon: RefreshCw }}
-					disabled={tools.status === 'loading'}
-				>
-					{tools.status === 'loading' ? 'Loading...' : 'Refresh Tools'}
-				</Button>
-			{/snippet}
-			<div class="w-full flex flex-col gap-2">
-				<div class="max-h-48 overflow-y-auto border rounded p-2 bg-surface-secondary">
-					{#if (tools.value ?? []).length === 0}
-						<div class="text-xs text-secondary italic">No tools returned by this MCP server.</div>
-					{:else}
-						<div class="flex flex-col gap-1">
-							{#each tools.value ?? [] as mcpTool (mcpTool.name)}
-								<div class="text-xs">
-									<span class="font-semibold">{mcpTool.name}</span>
-									{#if mcpTool.description}
-										<span class="text-secondary">— {mcpTool.description}</span>
-									{/if}
-								</div>
-							{/each}
-						</div>
-					{/if}
-				</div>
+			{/key}
+		{:else if error}
+			<div class="text-xs text-red-600 dark:text-red-400">{`Failed to connect to MCP server: ${error}`}</div>
+		{:else if tools.status === 'ok'}
+			<div class="w-full">
+				<Label label="Summary">
+					<input
+						type="text"
+						bind:value={tool.summary}
+						placeholder="e.g., GitHub MCP"
+						class="text-sm w-full"
+					/>
+				</Label>
 			</div>
-		</Section>
 
-		{#if tool.value.include_tools && tool.value.exclude_tools}
-			<Section label="Tool Filtering">
-				<div class="w-full flex flex-col gap-3">
-					<div class="flex flex-col gap-2">
-						<Label label="Only include specified tools">
-							<MultiSelect
-								bind:value={tool.value.include_tools}
-								items={toolOptions}
-								placeholder="Choose tools to include..."
-							/>
-						</Label>
-					</div>
-					<div class="flex flex-col gap-2">
-						<Label label="Exclude specified tools">
-							<MultiSelect
-								bind:value={tool.value.exclude_tools}
-								items={toolOptions}
-								placeholder="Choose tools to exclude..."
-							/>
-						</Label>
+			<Section label="Available Tools">
+				{#snippet action()}
+					<Button
+						size="xs"
+						color="light"
+						onClick={() => (refreshCount += 1)}
+						startIcon={{ icon: RefreshCw }}
+						disabled={tools.status === 'loading'}
+					>
+						{tools.status === 'loading' ? 'Loading...' : 'Refresh Tools'}
+					</Button>
+				{/snippet}
+				<div class="w-full flex flex-col gap-2">
+					<div class="max-h-48 overflow-y-auto border rounded p-2 bg-surface-secondary">
+						{#if (tools.value ?? []).length === 0}
+							<div class="text-xs text-secondary italic">No tools returned by this MCP server.</div>
+						{:else}
+							<div class="flex flex-col gap-1">
+								{#each tools.value ?? [] as mcpTool (mcpTool.name)}
+									<div class="text-xs">
+										<span class="font-semibold">{mcpTool.name}</span>
+										{#if mcpTool.description}
+											<span class="text-secondary">— {mcpTool.description}</span>
+										{/if}
+									</div>
+								{/each}
+							</div>
+						{/if}
 					</div>
 				</div>
 			</Section>
+
+			{#if tool.value.include_tools && tool.value.exclude_tools}
+				<Section label="Tool Filtering">
+					<div class="w-full flex flex-col gap-3">
+						<div class="flex flex-col gap-2">
+							<Label label="Only include specified tools">
+								<MultiSelect
+									bind:value={tool.value.include_tools}
+									items={toolOptions}
+									placeholder="Choose tools to include..."
+								/>
+							</Label>
+						</div>
+						<div class="flex flex-col gap-2">
+							<Label label="Exclude specified tools">
+								<MultiSelect
+									bind:value={tool.value.exclude_tools}
+									items={toolOptions}
+									placeholder="Choose tools to exclude..."
+								/>
+							</Label>
+						</div>
+					</div>
+				</Section>
+			{/if}
 		{/if}
 	{/if}
 </div>