Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -97,12 +97,12 @@ function ServerListItem({
server.lastError,
server.authType
)
// 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.
// Only hard-red when there are no last-known tools to show. A populated, connected server
// stays on its tool count through a transient probe failure; a persistent failure flips
// `connectionStatus` to error/disconnected and reads as failed through that path instead.
const showDiscoveryError =
Boolean(discoveryError) &&
tools.length === 0 &&
server.connectionStatus !== 'error' &&
server.connectionStatus !== 'disconnected'
const hasConnectionIssue =
Expand Down
92 changes: 90 additions & 2 deletions apps/sim/hooks/queries/mcp.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
*/
import { act, type ReactNode } from 'react'
import { sleep } from '@sim/utils/helpers'
import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
import { QueryClient, QueryClientProvider, useQueryClient } from '@tanstack/react-query'
import { createRoot, type Root } from 'react-dom/client'
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'

Expand All @@ -20,7 +20,12 @@ import {
listMcpServersContract,
type McpServer,
} from '@/lib/api/contracts/mcp'
import { useForceRefreshMcpTools, useMcpServers, useMcpToolsQuery } from '@/hooks/queries/mcp'
import {
mcpKeys,
useForceRefreshMcpTools,
useMcpServers,
useMcpToolsQuery,
} from '@/hooks/queries/mcp'

const WORKSPACE_ID = 'workspace-1'

Expand Down Expand Up @@ -181,6 +186,89 @@ describe('useMcpToolsQuery', () => {
unmount()
})

it('keeps last-known-good tools when a later discovery refetch fails', async () => {
let discoverCalls = 0
mockRequestJson.mockImplementation(async (contract) => {
if (contract === listMcpServersContract) {
return {
success: true,
data: { servers: [server('s1', { authType: 'headers', connectionStatus: 'connected' })] },
}
}
if (contract === discoverMcpToolsContract) {
discoverCalls++
if (discoverCalls === 1) {
return { success: true, data: { tools: [{ name: 'tool-a', serverId: 's1' }] } }
}
throw new Error('transient stall')
}
throw new Error('Unexpected MCP request')
})

const { getResult, unmount } = renderHookWithClient(() => ({
tools: useMcpToolsQuery(WORKSPACE_ID),
queryClient: useQueryClient(),
}))
await flush()
expect(getResult().tools.data).toHaveLength(1)

// Force a refetch that fails; the last successful tools must survive.
await act(async () => {
await getResult().queryClient.invalidateQueries({
queryKey: mcpKeys.serverToolsList(WORKSPACE_ID, 's1'),
})
})
await flush()

expect(getResult().tools.data).toHaveLength(1)
expect(getResult().tools.toolsStateByServer.get('s1')?.error).toBeInstanceOf(Error)

unmount()
})

it('drops stale tools once a non-OAuth server is persistently failed', async () => {
let discoverCalls = 0
let listCalls = 0
mockRequestJson.mockImplementation(async (contract) => {
if (contract === listMcpServersContract) {
listCalls++
// Healthy on first read, error state after the failed refetch invalidates the list.
const connectionStatus = listCalls === 1 ? 'connected' : 'error'
return {
success: true,
data: { servers: [server('s1', { authType: 'headers', connectionStatus })] },
}
}
if (contract === discoverMcpToolsContract) {
discoverCalls++
if (discoverCalls === 1) {
return { success: true, data: { tools: [{ name: 'tool-a', serverId: 's1' }] } }
}
throw new Error('persistent failure')
}
throw new Error('Unexpected MCP request')
})

const { getResult, unmount } = renderHookWithClient(() => ({
tools: useMcpToolsQuery(WORKSPACE_ID),
queryClient: useQueryClient(),
}))
await flush()
expect(getResult().tools.data).toHaveLength(1)

await act(async () => {
await getResult().queryClient.invalidateQueries({
queryKey: mcpKeys.serverToolsList(WORKSPACE_ID, 's1'),
})
})
await flush()

// Stored status is now 'error' → the dead server's stale tools are dropped from the aggregate.
expect(getResult().tools.data).toHaveLength(0)

unmount()
})

it('does not force-refresh disconnected OAuth servers', async () => {
mockServers([
server('oauth-disconnected', { authType: 'oauth', connectionStatus: 'disconnected' }),
Expand Down
14 changes: 10 additions & 4 deletions apps/sim/hooks/queries/mcp.ts
Original file line number Diff line number Diff line change
Expand Up @@ -182,21 +182,27 @@ export function useMcpToolsQuery(workspaceId: string) {
let hasData = false
let anyServerLoading = false
let firstError: Error | null = null
const statusById = new Map(servers?.map((s) => [s.id, s.connectionStatus]))
const toolsStateByServer = new Map<
string,
{ isLoading: boolean; isFetching: boolean; error: Error | null }
>()
for (let index = 0; index < results.length; index++) {
const result = results[index]
// Drop stale data from servers whose latest refetch errored.
if (result.data && !result.isError) {
const serverId = serverIds[index]
const status = serverId ? statusById.get(serverId) : undefined
const persistentlyFailed = status === 'error' || status === 'disconnected'
// Keep last-known-good tools through a transient failure (React Query retains `data`, the
// stored status is still healthy) so a populated server doesn't blank — but drop them once
// the stored status crosses its failure threshold, so the workflow editor stops offering a
// dead server's stale tools. Matches how reference MCP clients treat transient vs. closed.
if (result.data && (!result.isError || !persistentlyFailed)) {
tools.push(...result.data)
hasData = true
}
if (result.isLoading) anyServerLoading = true
if (!firstError && result.error instanceof Error) firstError = result.error

const serverId = serverIds[index]
if (serverId) {
toolsStateByServer.set(serverId, {
isLoading: result.isLoading,
Expand All @@ -213,7 +219,7 @@ export function useMcpToolsQuery(workspaceId: string) {
error: hasData ? null : firstError,
toolsStateByServer,
}
}, [results, serversLoading, serverIds])
}, [results, serversLoading, serverIds, servers])
}

export function useForceRefreshMcpTools() {
Expand Down
Loading