From 51b4468bcb5f323b6271c224f92181e6940d3e2d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 17:09:38 -0700 Subject: [PATCH 1/2] improvement(mcp): make the OAuth experience visible and verbally consistent MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From a full UX-consistency audit of the MCP settings surfaces: - One name for one action: 'Authorize' / 'Reopen authorization' everywhere (list chip, row, detail) — was three different labels across surfaces. - The connecting state is now visible: the row subtitle shows a muted 'Waiting for authorization...' instead of continuing to shout the red 'OAuth authorization required' mid-flow. - The Authorize affordance is a visible chip on the list row (was buried in the overflow menu), so a blocked popup's 'retry' has an obvious target. - Removed the redundant aggregate discovery banner — every failing row already reports its specific error; two red messages for one failure. - The header Refresh chip no longer renders an error sentence as its label (short 'Failed'; the Status field carries the explanation). - Sentence-case sweep (Unnamed server, Not connected, Server name, Add MCP server / Edit MCP server, Add server, Test connection, Edit form, Delete MCP server), 'Search servers...' placeholder, row-subtitle/error text on the canonical tokens, and a Loading empty state instead of a blank flash. --- .../mcp-server-form-modal.tsx | 8 +- .../settings/components/mcp/mcp.tsx | 79 ++++++++----------- .../mcp/refresh-action-state.test.ts | 4 +- .../components/mcp/refresh-action-state.ts | 4 +- .../components/mcp/server-tools-label.test.ts | 2 +- .../components/mcp/server-tools-label.ts | 2 +- 6 files changed, 43 insertions(+), 56 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/components/mcp-server-form-modal/mcp-server-form-modal.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/components/mcp-server-form-modal/mcp-server-form-modal.tsx index fe6d59e0967..f3afefe0820 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/components/mcp-server-form-modal/mcp-server-form-modal.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/components/mcp-server-form-modal/mcp-server-form-modal.tsx @@ -116,7 +116,7 @@ function getTestButtonLabel( if (testResult?.success) return 'Connection success' if (testResult?.authRequired) return 'Requires OAuth' if (testResult && !testResult.success) return 'No connection: retry' - return 'Test Connection' + return 'Test connection' } interface FormattedInputProps { @@ -609,8 +609,8 @@ export function McpServerFormModal({ const isSubmitDisabled = isSubmitting || !isFormValid || isDomainBlocked || (mode === 'edit' && !hasChanges) - const title = mode === 'add' ? 'Add New MCP Server' : 'Edit MCP Server' - const submitLabel = mode === 'add' ? 'Add MCP' : 'Save' + const title = mode === 'add' ? 'Add MCP server' : 'Edit MCP server' + const submitLabel = mode === 'add' ? 'Add server' : 'Save' const handleToggleJsonMode = () => { if (testResult) clearTestResult() @@ -622,7 +622,7 @@ export function McpServerFormModal({ const secondaryAction: ChipModalFooterAction | undefined = mode === 'add' ? { - label: formMode === 'form' ? 'Edit JSON' : 'Edit Form', + label: formMode === 'form' ? 'Edit JSON' : 'Edit form', onClick: handleToggleJsonMode, } : formMode === 'form' diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx index 97d64680a7d..0ce7078929a 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx @@ -114,38 +114,37 @@ function ServerListItem({
- {server.name || 'Unnamed Server'} + {server.name || 'Unnamed server'} ({transportLabel})

- {isRefreshing - ? 'Refreshing...' - : isLoadingTools && tools.length === 0 - ? 'Loading...' - : showDiscoveryError - ? discoveryError - : toolsLabel} + {isConnecting + ? 'Waiting for authorization...' + : isRefreshing + ? 'Refreshing...' + : isLoadingTools && tools.length === 0 + ? 'Loading...' + : showDiscoveryError + ? discoveryError + : toolsLabel}

+ {canManage && server.authType === 'oauth' && server.connectionStatus !== 'connected' && ( + {isConnecting ? 'Reopen authorization' : 'Authorize'} + )} state.error != null) const hasServers = servers && servers.length > 0 const showNoResults = searchTerm.trim() && filteredServers.length === 0 && servers.length > 0 @@ -426,7 +416,7 @@ export function MCP() { return (
- Server Name -

{server.name || 'Unnamed Server'}

+ Server name +

{server.name || 'Unnamed server'}

@@ -487,9 +477,7 @@ export function MCP() { await startOauthForServer(server.id) }} > - {connectingOauthServers.has(server.id) - ? 'Reopen authorization window' - : 'Connect with OAuth'} + {connectingOauthServers.has(server.id) ? 'Reopen authorization' : 'Authorize'}
@@ -651,7 +639,7 @@ export function MCP() { search={{ value: searchTerm, onChange: setSearchTerm, - placeholder: 'Search MCPs...', + placeholder: 'Search servers...', }} actions={ canEdit @@ -669,21 +657,18 @@ export function MCP() { > {listError ? (
-

+

{getErrorMessage(listError, 'Failed to load MCP servers')}

- ) : serversLoading ? null : !hasServers ? ( + ) : serversLoading ? ( + Loading... + ) : !hasServers ? ( {canEdit ? 'Click "Add server" above to get started' : 'No MCP servers configured'} ) : (
- {hasDiscoveryError && ( -

- {getErrorMessage(toolsError, 'Some tools could not be discovered')} -

- )} {filteredServers.map((server) => { if (!server?.id) return null const tools = toolsByServer[server.id] || [] @@ -749,8 +734,8 @@ export function MCP() { onOpenChange={(open) => { if (!open) setServerToDeleteId(null) }} - srTitle='Delete MCP Server' - title='Delete MCP Server' + srTitle='Delete MCP server' + title='Delete MCP server' text={[ 'Are you sure you want to delete ', { diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts index 4a036e8827b..ea47df2042c 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts @@ -31,7 +31,7 @@ describe('getRefreshActionState', () => { }) }) - it('shows OAuth authorization required when an OAuth refresh finishes disconnected', () => { + it('shows a short Failed result when an OAuth refresh finishes disconnected (status field explains)', () => { expect( getRefreshActionState({ mutationStatus: 'success', @@ -40,7 +40,7 @@ describe('getRefreshActionState', () => { workflowsUpdated: 0, }) ).toEqual({ - text: 'OAuth authorization required', + text: 'Failed', textTone: 'error', disabled: false, }) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts index 8b4ae5da87d..d54c0d376de 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts @@ -29,7 +29,9 @@ export function getRefreshActionState({ authType === 'oauth' && !error?.trim() ) { - return { text: 'OAuth authorization required', textTone: 'error', disabled: false } + // The detail view's Status field carries the "OAuth authorization required" + // explanation; the header chip stays a short action-shaped result. + return { text: 'Failed', textTone: 'error', disabled: false } } if ( diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts index 373ae480af4..43facb9bceb 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.test.ts @@ -19,7 +19,7 @@ describe('getServerToolsLabel', () => { }) it('keeps the generic disconnected state for non-OAuth servers', () => { - expect(getServerToolsLabel([], 'disconnected', null, 'headers')).toBe('Not Connected') + expect(getServerToolsLabel([], 'disconnected', null, 'headers')).toBe('Not connected') }) it('shows the persisted error for disconnected connections', () => { diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts index 495cea5f9ff..2aae09d5537 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/server-tools-label.ts @@ -16,7 +16,7 @@ export function getServerToolsLabel( if (connectionStatus === 'disconnected') { return ( - lastError?.trim() || (authType === 'oauth' ? 'OAuth authorization required' : 'Not Connected') + lastError?.trim() || (authType === 'oauth' ? 'OAuth authorization required' : 'Not connected') ) } From 57c6ee42b2d772e5bdf8207009c022c59fca0f51 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Tue, 21 Jul 2026 17:19:28 -0700 Subject: [PATCH 2/2] fix(mcp): show stale-discovery failures, best-effort popup-close label clear, drop redundant branch - A failing latest discovery now shows its error even when cached tools exist (the stale tool count silently hid it). - Best-effort popup.closed poll clears only the 'Waiting for authorization...' label share; the flow entry stays registered so a completion that still arrives over the BroadcastChannel is honored (settleFlow skips the double-decrement). Under COOP misreport the worst case is an early label reset, never a dropped completion. - Remove the OAuth refresh-state branch made redundant by the 'Failed' change, plus its now-unused authType/error inputs. --- .../settings/components/mcp/mcp.tsx | 5 +- .../mcp/refresh-action-state.test.ts | 17 ------ .../components/mcp/refresh-action-state.ts | 17 +----- .../hooks/mcp/use-mcp-oauth-popup.test.tsx | 58 +++++++++++++++++++ apps/sim/hooks/mcp/use-mcp-oauth-popup.ts | 37 ++++++++++-- 5 files changed, 93 insertions(+), 41 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx index 0ce7078929a..32cc51cc2f9 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx @@ -99,9 +99,10 @@ function ServerListItem({ ) // A live discovery failure whose stored status hasn't caught up yet would otherwise read as // "0 tools"; surface it directly so a failed row reads as failed, not empty. + // Shown even when cached tools exist: a present discoveryError means the LATEST + // discovery failed, and silently showing the stale tool count would hide that. const showDiscoveryError = Boolean(discoveryError) && - tools.length === 0 && server.connectionStatus !== 'error' && server.connectionStatus !== 'disconnected' const hasConnectionIssue = @@ -408,8 +409,6 @@ export function MCP() { const refreshAction = getRefreshActionState({ mutationStatus: isCurrentRefresh ? refreshServerMutation.status : 'idle', connectionStatus: isCurrentRefresh ? refreshServerMutation.data?.status : undefined, - authType: server.authType, - error: isCurrentRefresh ? refreshServerMutation.data?.error : undefined, workflowsUpdated: isCurrentRefresh ? refreshServerMutation.data?.workflowsUpdated : undefined, }) diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts index ea47df2042c..3186429218a 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.test.ts @@ -31,28 +31,11 @@ describe('getRefreshActionState', () => { }) }) - it('shows a short Failed result when an OAuth refresh finishes disconnected (status field explains)', () => { - expect( - getRefreshActionState({ - mutationStatus: 'success', - connectionStatus: 'disconnected', - authType: 'oauth', - workflowsUpdated: 0, - }) - ).toEqual({ - text: 'Failed', - textTone: 'error', - disabled: false, - }) - }) - it('keeps Failed when a disconnected OAuth refresh has a concrete error', () => { expect( getRefreshActionState({ mutationStatus: 'success', connectionStatus: 'disconnected', - authType: 'oauth', - error: 'The MCP server took too long to respond and timed out', workflowsUpdated: 0, }) ).toEqual({ diff --git a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts index d54c0d376de..28a6869921e 100644 --- a/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts +++ b/apps/sim/app/workspace/[workspaceId]/settings/components/mcp/refresh-action-state.ts @@ -1,12 +1,10 @@ import type { MutationStatus } from '@tanstack/react-query' -import type { McpServer, RefreshMcpServerResult } from '@/lib/api/contracts/mcp' +import type { RefreshMcpServerResult } from '@/lib/api/contracts/mcp' import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header' interface RefreshActionStateInput { mutationStatus: MutationStatus connectionStatus?: RefreshMcpServerResult['status'] - authType?: McpServer['authType'] - error?: RefreshMcpServerResult['error'] workflowsUpdated?: number } @@ -15,25 +13,12 @@ type RefreshActionState = Pick export function getRefreshActionState({ mutationStatus, connectionStatus, - authType, - error, workflowsUpdated, }: RefreshActionStateInput): RefreshActionState { if (mutationStatus === 'pending') { return { text: 'Refreshing...', textTone: undefined, disabled: true } } - if ( - mutationStatus === 'success' && - connectionStatus === 'disconnected' && - authType === 'oauth' && - !error?.trim() - ) { - // The detail view's Status field carries the "OAuth authorization required" - // explanation; the header chip stays a short action-shaped result. - return { text: 'Failed', textTone: 'error', disabled: false } - } - if ( mutationStatus === 'error' || (mutationStatus === 'success' && connectionStatus !== 'connected') diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx b/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx index 4825584d657..d7720a3971c 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx @@ -243,4 +243,62 @@ describe('useMcpOauthPopup', () => { expect(order).toEqual(['open', 'start']) hook.unmount() }) + + it('clears the label when the popup closes but still honors a late completion', async () => { + vi.useFakeTimers() + try { + const popup = { + close: vi.fn(), + focus: vi.fn(), + location: { replace: vi.fn() }, + closed: false, + } + ;(window.open as ReturnType).mockReturnValue(popup as unknown as Window) + let channelHandler: ((e: { data: unknown }) => void) | null = null + class CapturingChannel { + constructor(public name: string) {} + postMessage(): void {} + close(): void {} + } + Object.defineProperty(CapturingChannel.prototype, 'onmessage', { + set(h) { + channelHandler = h + }, + get() { + return channelHandler + }, + configurable: true, + }) + ;(globalThis as unknown as { BroadcastChannel: unknown }).BroadcastChannel = CapturingChannel + mockStartOauth.mockResolvedValue({ + status: 'redirect', + authorizationUrl: 'https://as.example/a?state=st', + state: 'st', + }) + const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' })) + await act(async () => { + await hook.result().startOauthForServer('s1') + }) + expect(hook.result().connectingServers.has('s1')).toBe(true) + + // User closes the popup — the label share clears within a poll tick... + popup.closed = true + await act(async () => { + vi.advanceTimersByTime(1500) + }) + expect(hook.result().connectingServers.has('s1')).toBe(false) + + // ...but the flow stays registered: a late BroadcastChannel completion is still honored. + const invalidateSpy = vi.spyOn(hook.queryClient, 'invalidateQueries') + await act(async () => { + channelHandler?.({ data: { type: 'mcp-oauth', ok: true, serverId: 's1', state: 'st' } }) + }) + expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ['mcp', 'servers', 'w1'] }) + // And the count did not go negative / re-clear twice. + expect(hook.result().connectingServers.has('s1')).toBe(false) + hook.unmount() + } finally { + vi.useRealTimers() + } + }) }) diff --git a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts index f9768a39bd5..d1e76921b48 100644 --- a/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts +++ b/apps/sim/hooks/mcp/use-mcp-oauth-popup.ts @@ -58,7 +58,9 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { // correlation: the callback echoes it on every result (even failures that resolve no serverId), // so the tab that started this exact flow matches it while other same-origin tabs — and // unrelated flows in this tab — ignore the broadcast. - const pendingFlowsRef = useRef>(new Map()) + const pendingFlowsRef = useRef< + Map + >(new Map()) // serverIds with an in-flight `/oauth/start` request — guards a fast double-click from opening // two popups. Cleared once the request settles, so a later click (to reopen an abandoned // popup) still starts a fresh flow. @@ -98,8 +100,10 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { const flow = pendingFlowsRef.current.get(state) if (!flow) return window.clearTimeout(flow.timeout) + if (flow.poll !== undefined) window.clearInterval(flow.poll) pendingFlowsRef.current.delete(state) - decConnecting(flow.serverId) + // The popup-closed poll may have already cleared this flow's label share. + if (!flow.labelCleared) decConnecting(flow.serverId) }, [decConnecting] ) @@ -119,7 +123,10 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { useEffect(() => { const pending = pendingFlowsRef.current return () => { - for (const { timeout } of pending.values()) window.clearTimeout(timeout) + for (const { timeout, poll } of pending.values()) { + window.clearTimeout(timeout) + if (poll !== undefined) window.clearInterval(poll) + } pending.clear() } }, []) @@ -204,10 +211,30 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) { // Track the in-flight flow by its `state` nonce for the BroadcastChannel gate, bounded by // a safety timeout in case no result ever arrives (popup abandoned, or a callback the // client can't otherwise observe under COOP). - pendingFlowsRef.current.set(state, { + const flow: { serverId: string; timeout: number; poll?: number; labelCleared?: boolean } = { serverId, timeout: window.setTimeout(() => settleFlow(state), OAUTH_FLOW_TIMEOUT_MS), - }) + } + pendingFlowsRef.current.set(state, flow) + // Best-effort label clear when the user closes the popup: under COOP `closed` can + // misreport true, so this only clears the "Waiting for authorization..." share of + // the count — the flow entry stays registered, and a completion that still arrives + // over the BroadcastChannel is honored (settleFlow skips the double-decrement). + flow.poll = window.setInterval(() => { + let closed = false + try { + closed = popup.closed + } catch { + closed = true + } + if (!closed) return + if (flow.poll !== undefined) window.clearInterval(flow.poll) + flow.poll = undefined + if (!flow.labelCleared && pendingFlowsRef.current.get(state) === flow) { + flow.labelCleared = true + decConnecting(serverId) + } + }, 1000) } catch (e) { try { popup.close()