improvement(mcp): let users re-open OAuth authorization anytime (#5809)

* improvement(mcp): let users re-open OAuth authorization anytime

A server stuck on 'OAuth Authorization required' had no discoverable way to
re-trigger the auth popup once it was closed or abandoned: the connect button
lived only in the server detail view and hard-disabled while 'Connecting…',
a state cleared by polling popup.closed (unreliable under COOP), so it could
stay locked until the 10-minute safety timeout.

- Add an 'Authorize'/'Reopen authorization' action to the server list row menu
  for unconnected OAuth servers, so re-auth is reachable without drilling in.
- Make the detail-view button always clickable (reopens a fresh popup) instead
  of locking on an unverifiable 'Connecting…' state.

* fix(mcp): guard OAuth start against double-click opening two popups

Now that the connect button stays clickable, block re-entry while the
/oauth/start request is in flight; cleared once it settles so a later reopen
still starts a fresh flow.

* test(mcp): cover OAuth popup double-click guard and reopen-after-settle

* fix(mcp): kill prior popup poll before reopening so it can't clear the new flow

Addresses a stale-poll race: an abandoned attempt's popup.closed interval could
fire mid-reopen and clear 'Connecting…' for the fresh flow.

* fix(mcp): retire prior OAuth flow's pending entry on reopen, not just its poll

Drop the previous attempt's pending-flow map entry and safety timeout up front
too, so a late BroadcastChannel result (settleFlow) can't clear the reopened
flow's connecting state during the new /oauth/start await.

* fix(mcp): preserve prior OAuth flow until reopen's start succeeds

Reworks reopen concurrency so a superseded or failed reopen can't lose a real
authorization:
- Retire the prior flow only after the replacement /oauth/start succeeds; a
  failed reopen keeps it so its popup can still complete and be honored.
- Guard the connecting-clear (settleFlow + popup poll) behind the in-flight
  start set, so a stale settle can't flicker the reopened flow's label.
- Invalidate server queries on already_authorized so the UI reflects a server
  that authorized out from under the client.
Adds a test for the already_authorized invalidation.

* refactor(mcp): make OAuth connecting state deterministic via reference counting

Replaces the scattered, race-prone stopConnecting/set-based tracking with a
per-server attempt count: each start increments once and clears exactly once
(fail, already_authorized, settle, or supersede), so the 'Connecting…' /
'Reopen authorization' label can never get stuck or flicker across concurrent
reopens. Retire prior flows whenever a replacement start succeeds (including
already_authorized); a failed reopen keeps the prior flow so it can still
complete. Drops the unreliable popup.closed poll entirely — the label already
invites reopening and the safety timeout guarantees clearing. Adds a test for
the failed-reopen-preserves-prior-flow case.

Author: waleedlatif1

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

@@ -68,21 +68,25 @@ interface ServerListItemProps {
   server: McpServer
   tools: McpTool[]
   isDeleting: boolean
+  isConnecting: boolean
   isLoadingTools?: boolean
   isRefreshing?: boolean
   onRemove: () => void
   onViewDetails: () => void
+  onAuthorize: () => void
 }
 
 function ServerListItem({
   canManage,
   server,
   tools,
   isDeleting,
+  isConnecting,
   isLoadingTools = false,
   isRefreshing = false,
   onRemove,
   onViewDetails,
+  onAuthorize,
 }: ServerListItemProps) {
   const transportLabel = formatTransportLabel(server.transport || 'http')
   const toolsLabel = getServerToolsLabel(
@@ -121,6 +125,14 @@ function ServerListItem({
           label='Server actions'
           actions={[
             { label: 'Details', onSelect: onViewDetails },
+            ...(canManage && server.authType === 'oauth' && server.connectionStatus !== 'connected'
+              ? [
+                  {
+                    label: isConnecting ? 'Reopen authorization' : 'Authorize',
+                    onSelect: onAuthorize,
+                  },
+                ]
+              : []),
             ...(canManage
               ? [
                   {
@@ -450,12 +462,13 @@ export function MCP() {
                 <div>
                   <Chip
                     variant='primary'
-                    disabled={connectingOauthServers.has(server.id)}
                     onClick={async () => {
                       await startOauthForServer(server.id)
                     }}
                   >
-                    {connectingOauthServers.has(server.id) ? 'Connecting…' : 'Connect with OAuth'}
+                    {connectingOauthServers.has(server.id)
+                      ? 'Reopen authorization window'
+                      : 'Connect with OAuth'}
                   </Chip>
                 </div>
               </div>
@@ -660,13 +673,15 @@ export function MCP() {
                   server={server}
                   tools={tools}
                   isDeleting={deletingServers.has(server.id)}
+                  isConnecting={connectingOauthServers.has(server.id)}
                   isLoadingTools={isLoadingTools}
                   isRefreshing={
                     refreshServerMutation.isPending &&
                     refreshServerMutation.variables?.serverId === server.id
                   }
                   onRemove={() => handleRemoveServer(server.id)}
                   onViewDetails={() => handleViewDetails(server.id)}
+                  onAuthorize={() => startOauthForServer(server.id)}
                 />
               )
             })}

apps/sim/hooks/mcp/use-mcp-oauth-popup.test.tsx

@@ -0,0 +1,191 @@
+/**
+ * @vitest-environment jsdom
+ */
+import { act, type ReactNode } from 'react'
+import { sleep } from '@sim/utils/helpers'
+import { QueryClient, QueryClientProvider } from '@tanstack/react-query'
+import { createRoot, type Root } from 'react-dom/client'
+import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
+
+const { mockStartOauth } = vi.hoisted(() => ({ mockStartOauth: vi.fn() }))
+
+vi.mock('@sim/emcn', () => ({
+  toast: { success: vi.fn(), error: vi.fn() },
+}))
+
+vi.mock('@/hooks/queries/mcp', () => ({
+  useStartMcpOauth: () => ({ mutateAsync: mockStartOauth }),
+  mcpKeys: {
+    serversList: (workspaceId: string) => ['mcp', 'servers', workspaceId],
+    serverToolsList: (workspaceId: string, serverId: string) => [
+      'mcp',
+      'server-tools',
+      workspaceId,
+      serverId,
+    ],
+    storedToolsList: (workspaceId: string) => ['mcp', 'stored-tools', workspaceId],
+  },
+}))
+
+import { useMcpOauthPopup } from '@/hooks/mcp/use-mcp-oauth-popup'
+
+/**
+ * Minimal dependency-free hook harness (the repo has no `@testing-library/react`).
+ * Mounts the hook in a real React 19 root under jsdom, wrapped in a real
+ * `QueryClientProvider`, so query/mutation lifecycles run exactly as in the app.
+ */
+function renderHookWithClient<T>(useHook: () => T): {
+  result: () => T
+  queryClient: QueryClient
+  unmount: () => void
+} {
+  ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
+  const queryClient = new QueryClient({
+    defaultOptions: { queries: { retry: false }, mutations: { retry: false } },
+  })
+  const container = document.createElement('div')
+  const root: Root = createRoot(container)
+  let latest: T
+
+  function Probe() {
+    latest = useHook()
+    return null
+  }
+
+  function Wrapper({ children }: { children: ReactNode }) {
+    return <QueryClientProvider client={queryClient}>{children}</QueryClientProvider>
+  }
+
+  act(() => {
+    root.render(
+      <Wrapper>
+        <Probe />
+      </Wrapper>
+    )
+  })
+
+  return { result: () => latest, queryClient, unmount: () => act(() => root.unmount()) }
+}
+
+async function flush() {
+  await act(async () => {
+    for (let i = 0; i < 5; i++) {
+      await Promise.resolve()
+      await sleep(0)
+    }
+  })
+}
+
+describe('useMcpOauthPopup', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    // jsdom has no BroadcastChannel; the hook opens one on mount.
+    class FakeBroadcastChannel {
+      onmessage: ((event: MessageEvent) => void) | null = null
+      constructor(public name: string) {}
+      postMessage(): void {}
+      close(): void {}
+    }
+    ;(globalThis as unknown as { BroadcastChannel: unknown }).BroadcastChannel =
+      FakeBroadcastChannel
+  })
+
+  afterEach(() => {
+    vi.restoreAllMocks()
+  })
+
+  it('ignores a concurrent second start for the same server (no double popup)', async () => {
+    let resolveStart: (value: unknown) => void = () => {}
+    mockStartOauth.mockImplementation(
+      () =>
+        new Promise((res) => {
+          resolveStart = res
+        })
+    )
+
+    const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
+    await flush()
+
+    // Two clicks before /oauth/start resolves — the guard must collapse them to one request.
+    await act(async () => {
+      void hook.result().startOauthForServer('s1')
+      void hook.result().startOauthForServer('s1')
+    })
+    expect(mockStartOauth).toHaveBeenCalledTimes(1)
+
+    // Settle the first flow so the guard clears.
+    await act(async () => {
+      resolveStart({ status: 'redirect', popup: { closed: false }, state: 'state-1' })
+    })
+    await flush()
+
+    hook.unmount()
+  })
+
+  it('allows a fresh start after the previous one settles (reopen after abandon)', async () => {
+    mockStartOauth.mockResolvedValue({ status: 'redirect', popup: { closed: false }, state: 'st' })
+
+    const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
+    await flush()
+
+    await act(async () => {
+      await hook.result().startOauthForServer('s1')
+    })
+    await flush()
+
+    await act(async () => {
+      await hook.result().startOauthForServer('s1')
+    })
+    await flush()
+
+    // Both distinct clicks reached the mutation — the guard only blocks concurrent re-entry.
+    expect(mockStartOauth).toHaveBeenCalledTimes(2)
+
+    hook.unmount()
+  })
+
+  it('invalidates server queries when start reports already_authorized', async () => {
+    mockStartOauth.mockResolvedValue({ status: 'already_authorized' })
+
+    const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
+    await flush()
+    const invalidateSpy = vi.spyOn(hook.queryClient, 'invalidateQueries')
+
+    await act(async () => {
+      await hook.result().startOauthForServer('s1')
+    })
+    await flush()
+
+    // The server is already connected — the UI must still refresh so it reflects that.
+    expect(invalidateSpy).toHaveBeenCalledWith({ queryKey: ['mcp', 'servers', 'w1'] })
+
+    hook.unmount()
+  })
+
+  it('keeps the row connecting when a reopen fails but a prior flow is still live', async () => {
+    mockStartOauth.mockResolvedValueOnce({
+      status: 'redirect',
+      popup: { closed: false },
+      state: 'st-a',
+    })
+    const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
+    await flush()
+
+    await act(async () => {
+      await hook.result().startOauthForServer('s1')
+    })
+    await flush()
+    expect(hook.result().connectingServers.has('s1')).toBe(true)
+
+    // Reopen fails (e.g. popup blocked). The still-live prior flow must keep the row connecting
+    // rather than the failed attempt clearing it (deterministic reference counting).
+    mockStartOauth.mockRejectedValueOnce(new Error('Popup blocked'))
+    await act(async () => {
+      await hook.result().startOauthForServer('s1')
+    })
+    await flush()
+    expect(hook.result().connectingServers.has('s1')).toBe(true)
+
+    hook.unmount()
+  })
+})