fix(mcp): open the OAuth popup synchronously and bound the start request

Two bugs behind 'pressed Connect/Reopen and nothing happened':
- window.open ran AFTER awaiting /oauth/start, outside the browser's user
  activation, so the popup was silently blocked (worst on the add-server flow).
  Popup-first now: open about:blank synchronously in the click (named window,
  so a re-click focuses/reuses an existing authorization window), navigate it
  once the start returns; close it on failure/already_authorized; clear toast
  when genuinely blocked (and skip the request entirely).
- /oauth/start had no client timeout, so a stalled start held the re-entrancy
  guard and the connecting label indefinitely. Bounded at 30s; on timeout the
  popup closes, the label resets, and retry is immediately available.

Author: waleedlatif1

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

@@ -79,6 +79,16 @@ async function flush() {
 describe('useMcpOauthPopup', () => {
   beforeEach(() => {
     vi.clearAllMocks()
+    // Popup-first: startOauthForServer opens a blank window synchronously.
+    window.open = vi.fn(
+      () =>
+        ({
+          close: vi.fn(),
+          focus: vi.fn(),
+          location: { replace: vi.fn() },
+          closed: false,
+        }) as unknown as Window
+    )
     // jsdom has no BroadcastChannel; the hook opens one on mount.
     class FakeBroadcastChannel {
       onmessage: ((event: MessageEvent) => void) | null = null
@@ -115,15 +125,23 @@ describe('useMcpOauthPopup', () => {
 
     // Settle the first flow so the guard clears.
     await act(async () => {
-      resolveStart({ status: 'redirect', popup: { closed: false }, state: 'state-1' })
+      resolveStart({
+        status: 'redirect',
+        authorizationUrl: 'https://as.example/a?state=state-1',
+        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' })
+    mockStartOauth.mockResolvedValue({
+      status: 'redirect',
+      authorizationUrl: 'https://as.example/a?state=st',
+      state: 'st',
+    })
 
     const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
     await flush()
@@ -165,7 +183,7 @@ describe('useMcpOauthPopup', () => {
   it('keeps the row connecting when a reopen fails but a prior flow is still live', async () => {
     mockStartOauth.mockResolvedValueOnce({
       status: 'redirect',
-      popup: { closed: false },
+      authorizationUrl: 'https://as.example/a?state=st-a',
       state: 'st-a',
     })
     const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
@@ -188,4 +206,40 @@ describe('useMcpOauthPopup', () => {
 
     hook.unmount()
   })
+
+  it('does not issue the start request when the popup is blocked', async () => {
+    ;(window.open as ReturnType<typeof vi.fn>).mockReturnValue(null)
+    const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
+    await flush()
+
+    await act(async () => {
+      await hook.result().startOauthForServer('s1')
+    })
+    await flush()
+
+    expect(mockStartOauth).not.toHaveBeenCalled()
+    expect(hook.result().connectingServers.has('s1')).toBe(false)
+    hook.unmount()
+  })
+
+  it('opens the popup before the start request resolves (popup-first)', async () => {
+    const order: string[] = []
+    ;(window.open as ReturnType<typeof vi.fn>).mockImplementation(() => {
+      order.push('open')
+      return { close: vi.fn(), focus: vi.fn(), location: { replace: vi.fn() } } as unknown as Window
+    })
+    mockStartOauth.mockImplementation(async () => {
+      order.push('start')
+      return { status: 'redirect', authorizationUrl: 'https://as.example/a?state=st', state: 'st' }
+    })
+    const hook = renderHookWithClient(() => useMcpOauthPopup({ workspaceId: 'w1' }))
+    await flush()
+
+    await act(async () => {
+      await hook.result().startOauthForServer('s1')
+    })
+
+    expect(order).toEqual(['open', 'start'])
+    hook.unmount()
+  })
 })

apps/sim/hooks/mcp/use-mcp-oauth-popup.ts

@@ -153,27 +153,55 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
       const starting = (startingRef.current ??= new Set())
       if (starting.has(serverId)) return
       starting.add(serverId)
+      // Popup-first: open (or, via the shared window name, reuse+focus) the popup
+      // SYNCHRONOUSLY inside the user's click so the browser's activation gate sees
+      // it. Opening after the /oauth/start await loses the activation and gets
+      // silently popup-blocked — the "pressed Connect and nothing happened" bug.
+      const popup = window.open(
+        'about:blank',
+        `mcp-oauth-${serverId}`,
+        'width=560,height=720,resizable=yes,scrollbars=yes'
+      )
+      if (!popup) {
+        starting.delete(serverId)
+        toast.error('Popup blocked. Please allow popups for this site and retry.')
+        return
+      }
+      popup.focus?.()
       incConnecting(serverId) // this attempt begins
       try {
         const result = await startOauth({ serverId, workspaceId })
-        // The replacement start succeeded (already-authorized, or a fresh popup opened), so retire
-        // any prior attempt for this server now — its result is moot and the server-side `state`
-        // it depended on has been overwritten. A *failed* start (below) leaves prior flows intact.
+        // The replacement start succeeded (already-authorized, or a fresh authorization is
+        // underway), so retire any prior attempt for this server now — its result is moot and
+        // the server-side `state` it depended on has been overwritten. A *failed* start
+        // (below) leaves prior flows intact.
         retireFlows(serverId)
         if (result.status === 'already_authorized') {
+          try {
+            popup.close()
+          } catch {}
           invalidateServer(serverId)
           decConnecting(serverId) // this attempt ends
           return
         }
+        const { authorizationUrl, state } = result
+        try {
+          popup.location.replace(authorizationUrl)
+        } catch {
+          // A COOP-severed reused window can refuse scripted navigation; reopen by name.
+          window.open(authorizationUrl, `mcp-oauth-${serverId}`)
+        }
         // 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).
-        const { state } = result
         pendingFlowsRef.current.set(state, {
           serverId,
           timeout: window.setTimeout(() => settleFlow(state), OAUTH_FLOW_TIMEOUT_MS),
         })
       } catch (e) {
+        try {
+          popup.close()
+        } catch {}
         decConnecting(serverId) // this attempt ends; any prior flow keeps its own count
         logger.error('Failed to start MCP OAuth', e)
         toast.error(toError(e).message || 'Failed to start authorization')

apps/sim/hooks/queries/mcp.ts

@@ -310,7 +310,7 @@ export function useCreateMcpServer() {
  * correlate the eventual result back to this exact flow.
  */
 export type StartMcpOauthMutationResult =
-  | { status: 'redirect'; popup: Window; state: string }
+  | { status: 'redirect'; authorizationUrl: string; state: string }
   | { status: 'already_authorized' }
 
 export function useStartMcpOauth() {
@@ -319,6 +319,9 @@ export function useStartMcpOauth() {
       mutationFn: async ({ serverId, workspaceId }) => {
         const result = await requestJson(startMcpOauthContract, {
           query: { serverId, workspaceId },
+          // A stalled /oauth/start must settle so the caller can reset the connecting
+          // state and close its pre-opened popup instead of appearing bricked.
+          signal: AbortSignal.timeout(30_000),
         })
         if (result.status === 'already_authorized') return { status: 'already_authorized' }
 
@@ -332,15 +335,10 @@ export function useStartMcpOauth() {
         if (!state) {
           throw new Error('Authorization URL is missing the OAuth state parameter')
         }
-        const popup = window.open(
-          result.authorizationUrl,
-          `mcp-oauth-${serverId}`,
-          'width=560,height=720,resizable=yes,scrollbars=yes'
-        )
-        if (!popup) {
-          throw new Error('Popup blocked. Please allow popups for this site and retry.')
-        }
-        return { status: 'redirect', popup, state }
+        // The popup itself is opened SYNCHRONOUSLY by the caller inside the user's
+        // click (popup-first) — opening it here, after the network await, loses the
+        // user activation and gets silently popup-blocked.
+        return { status: 'redirect', authorizationUrl: result.authorizationUrl, state }
       },
     }
   )