Skip to content

Commit e72a777

Browse files
committed
fix(mcp): improve OAuth failure recovery
1 parent d165712 commit e72a777

12 files changed

Lines changed: 587 additions & 109 deletions

File tree

apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx

Lines changed: 19 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,8 @@ import { useWorkflowRegistry } from '@/stores/workflows/registry/store'
4444
import { useSubBlockStore } from '@/stores/workflows/subblock/store'
4545
import type { BlockState } from '@/stores/workflows/workflow/types'
4646
import { McpServerFormModal } from './components'
47+
import { getRefreshActionState } from './refresh-action-state'
48+
import { getServerToolsLabel } from './server-tools-label'
4749

4850
const logger = createLogger('McpSettings')
4951

@@ -58,16 +60,6 @@ function formatTransportLabel(transport: string): string {
5860
.join('-')
5961
}
6062

61-
function formatToolsLabel(tools: McpTool[], connectionStatus?: string): string {
62-
if (connectionStatus === 'error') {
63-
return 'Unable to connect'
64-
}
65-
const count = tools.length
66-
const plural = count !== 1 ? 's' : ''
67-
const names = count > 0 ? `: ${tools.map((t) => t.name).join(', ')}` : ''
68-
return `${count} tool${plural}${names}`
69-
}
70-
7163
interface ServerListItemProps {
7264
server: McpServer
7365
tools: McpTool[]
@@ -88,7 +80,7 @@ function ServerListItem({
8880
onViewDetails,
8981
}: ServerListItemProps) {
9082
const transportLabel = formatTransportLabel(server.transport || 'http')
91-
const toolsLabel = formatToolsLabel(tools, server.connectionStatus)
83+
const toolsLabel = getServerToolsLabel(tools, server.connectionStatus, server.lastError)
9284
const isError = server.connectionStatus === 'error'
9385

9486
return (
@@ -295,19 +287,11 @@ export function MCP() {
295287
}
296288

297289
useEffect(() => {
298-
if (!refreshServerMutation.isSuccess) return
290+
if (!refreshServerMutation.isSuccess && !refreshServerMutation.isError) return
299291
const timeout = window.setTimeout(() => refreshServerMutation.reset(), 3000)
300292
return () => window.clearTimeout(timeout)
301-
// eslint-disable-next-line react-hooks/exhaustive-deps -- mutation object is unstable; isSuccess flag is the trigger
302-
}, [refreshServerMutation.isSuccess])
303-
304-
const refreshingServerId = refreshServerMutation.isPending
305-
? refreshServerMutation.variables?.serverId
306-
: null
307-
const refreshedServerId = refreshServerMutation.isSuccess
308-
? refreshServerMutation.variables?.serverId
309-
: null
310-
const refreshedWorkflowsUpdated = refreshServerMutation.data?.workflowsUpdated
293+
// eslint-disable-next-line react-hooks/exhaustive-deps -- mutation object is unstable; status flags are the triggers
294+
}, [refreshServerMutation.isSuccess, refreshServerMutation.isError])
311295

312296
const editingServer = editingServerId
313297
? (servers.find((s) => s.id === editingServerId) as McpServer | undefined)
@@ -372,25 +356,23 @@ export function MCP() {
372356
if (selectedServer) {
373357
const { server, tools } = selectedServer
374358
const transportLabel = formatTransportLabel(server.transport || 'http')
375-
376-
const refreshLabel =
377-
refreshingServerId === server.id
378-
? 'Refreshing...'
379-
: refreshedServerId === server.id
380-
? refreshedWorkflowsUpdated
381-
? `Synced (${refreshedWorkflowsUpdated} workflow${refreshedWorkflowsUpdated === 1 ? '' : 's'})`
382-
: 'Refreshed'
383-
: 'Refresh tools'
359+
const isCurrentRefresh = refreshServerMutation.variables?.serverId === server.id
360+
const refreshAction = getRefreshActionState({
361+
mutationStatus: isCurrentRefresh ? refreshServerMutation.status : 'idle',
362+
connectionStatus: isCurrentRefresh ? refreshServerMutation.data?.status : undefined,
363+
workflowsUpdated: isCurrentRefresh ? refreshServerMutation.data?.workflowsUpdated : undefined,
364+
})
384365

385366
return (
386367
<SettingsPanel
387368
back={{ text: 'MCP tools', icon: ArrowLeft, onSelect: handleBackToList }}
388369
title={server.name || 'Unnamed Server'}
389370
actions={[
390371
{
391-
text: refreshLabel,
372+
text: refreshAction.text,
373+
textTone: refreshAction.textTone,
392374
onSelect: () => handleRefreshServer(server.id),
393-
disabled: refreshingServerId === server.id || refreshedServerId === server.id,
375+
disabled: refreshAction.disabled,
394376
},
395377
{
396378
text: 'Edit',
@@ -634,7 +616,10 @@ export function MCP() {
634616
tools={tools}
635617
isDeleting={deletingServers.has(server.id)}
636618
isLoadingTools={isLoadingTools}
637-
isRefreshing={refreshingServerId === server.id}
619+
isRefreshing={
620+
refreshServerMutation.isPending &&
621+
refreshServerMutation.variables?.serverId === server.id
622+
}
638623
onRemove={() => handleRemoveServer(server.id)}
639624
onViewDetails={() => handleViewDetails(server.id)}
640625
/>
Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { getRefreshActionState } from './refresh-action-state'
3+
4+
describe('getRefreshActionState', () => {
5+
it.each(['error', 'disconnected'] as const)(
6+
'shows a retryable red-text Failed state when refresh returns %s',
7+
(status) => {
8+
expect(
9+
getRefreshActionState({
10+
mutationStatus: 'success',
11+
connectionStatus: status,
12+
workflowsUpdated: 0,
13+
})
14+
).toEqual({
15+
text: 'Failed',
16+
textTone: 'error',
17+
disabled: false,
18+
})
19+
}
20+
)
21+
22+
it('shows Failed when the refresh request itself rejects', () => {
23+
expect(
24+
getRefreshActionState({
25+
mutationStatus: 'error',
26+
})
27+
).toEqual({
28+
text: 'Failed',
29+
textTone: 'error',
30+
disabled: false,
31+
})
32+
})
33+
34+
it('preserves successful refresh feedback', () => {
35+
expect(
36+
getRefreshActionState({
37+
mutationStatus: 'success',
38+
connectionStatus: 'connected',
39+
workflowsUpdated: 2,
40+
})
41+
).toEqual({
42+
text: 'Synced (2 workflows)',
43+
textTone: undefined,
44+
disabled: true,
45+
})
46+
})
47+
})
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
import type { MutationStatus } from '@tanstack/react-query'
2+
import type { RefreshMcpServerResult } from '@/lib/api/contracts/mcp'
3+
import type { SettingsAction } from '@/app/workspace/[workspaceId]/settings/components/settings-header/settings-header'
4+
5+
interface RefreshActionStateInput {
6+
mutationStatus: MutationStatus
7+
connectionStatus?: RefreshMcpServerResult['status']
8+
workflowsUpdated?: number
9+
}
10+
11+
type RefreshActionState = Pick<SettingsAction, 'text' | 'textTone' | 'disabled'>
12+
13+
export function getRefreshActionState({
14+
mutationStatus,
15+
connectionStatus,
16+
workflowsUpdated,
17+
}: RefreshActionStateInput): RefreshActionState {
18+
if (mutationStatus === 'pending') {
19+
return { text: 'Refreshing...', textTone: undefined, disabled: true }
20+
}
21+
22+
if (
23+
mutationStatus === 'error' ||
24+
(mutationStatus === 'success' && connectionStatus !== 'connected')
25+
) {
26+
return { text: 'Failed', textTone: 'error', disabled: false }
27+
}
28+
29+
if (mutationStatus === 'success') {
30+
const text = workflowsUpdated
31+
? `Synced (${workflowsUpdated} workflow${workflowsUpdated === 1 ? '' : 's'})`
32+
: 'Refreshed'
33+
return { text, textTone: undefined, disabled: true }
34+
}
35+
36+
return { text: 'Refresh tools', textTone: undefined, disabled: false }
37+
}
Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
import { describe, expect, it } from 'vitest'
2+
import { getServerToolsLabel } from './server-tools-label'
3+
4+
describe('getServerToolsLabel', () => {
5+
it('shows the persisted server error for errored connections', () => {
6+
expect(getServerToolsLabel([], 'error', 'MCP error -32001: Request timed out')).toBe(
7+
'MCP error -32001: Request timed out'
8+
)
9+
})
10+
11+
it('falls back when an errored connection has no persisted error', () => {
12+
expect(getServerToolsLabel([], 'error', null)).toBe('Unable to connect')
13+
})
14+
15+
it('continues showing discovered tools for healthy connections', () => {
16+
expect(getServerToolsLabel([{ name: 'search' }], 'connected', null)).toBe('1 tool: search')
17+
})
18+
})
Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,20 @@
1+
import type { McpServer } from '@/lib/api/contracts/mcp'
2+
3+
interface NamedTool {
4+
name: string
5+
}
6+
7+
export function getServerToolsLabel(
8+
tools: NamedTool[],
9+
connectionStatus?: McpServer['connectionStatus'],
10+
lastError?: McpServer['lastError']
11+
): string {
12+
if (connectionStatus === 'error') {
13+
return lastError?.trim() || 'Unable to connect'
14+
}
15+
16+
const count = tools.length
17+
const plural = count !== 1 ? 's' : ''
18+
const names = count > 0 ? `: ${tools.map((tool) => tool.name).join(', ')}` : ''
19+
return `${count} tool${plural}${names}`
20+
}

apps/sim/app/workspace/[workspaceId]/settings/components/settings-header/settings-header.tsx

Lines changed: 7 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ const useIsomorphicLayoutEffect = typeof window === 'undefined' ? useEffect : us
2121
/** The strict contract for a settings header action — rendered as a {@link Chip}, data only. */
2222
export interface SettingsAction {
2323
text: string
24+
textTone?: 'error'
2425
icon?: ComponentType<{ className?: string }>
2526
variant?: 'primary' | 'destructive'
2627
active?: boolean
@@ -83,6 +84,7 @@ function computeSignature(c: SettingsHeaderConfig): string {
8384
b: c.back ? [c.back.text, c.back.icon ? 1 : 0] : null,
8485
a: c.actions?.map((x) => [
8586
x.text,
87+
x.textTone ?? '',
8688
x.variant ?? '',
8789
x.active ?? false,
8890
x.disabled ?? false,
@@ -173,7 +175,11 @@ export function SettingsHeaderShell({ children }: { children: ReactNode }) {
173175
}
174176
disabled={action.disabled}
175177
>
176-
{action.text}
178+
{action.textTone === 'error' ? (
179+
<span className='text-[var(--text-error)]'>{action.text}</span>
180+
) : (
181+
action.text
182+
)}
177183
</Chip>
178184
)
179185
return action.tooltip ? (

0 commit comments

Comments
 (0)