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

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()
@@ -162,10 +180,10 @@ describe('useMcpOauthPopup', () => {
  hook.unmount()
  })
 
- it('keeps the row connecting when a reopen fails but a prior flow is still live', async () => {
+ it('clears the row when a reopen fails (the reopen blanked the prior window)', 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' }))
@@ -177,15 +195,52 @@ describe('useMcpOauthPopup', () => {
  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'))
+ // Reopen fails after the named-window open already navigated the prior auth window to
+ // about:blank — the prior flow is moot (windowless), so BOTH it and the failed attempt
+ // clear, leaving the row idle with 'Connect with OAuth' immediately available.
+ mockStartOauth.mockRejectedValueOnce(new Error('start failed'))
  await act(async () => {
  await hook.result().startOauthForServer('s1')
  })
  await flush()
- expect(hook.result().connectingServers.has('s1')).toBe(true)
+ expect(hook.result().connectingServers.has('s1')).toBe(false)
+
+ 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

@@ -50,9 +50,9 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
 
  // Per-server count of live authorization attempts; a row shows "Connecting…" / "Reopen
  // authorization" while its count > 0. Reference counting (not a boolean set) keeps the label
- // deterministic across concurrent attempts: a reopen increments before the superseded flow
- // decrements, so the count never dips to 0 mid-reopen (no flicker), and every attempt clears
- // exactly once (never stuck).
+ // deterministic across concurrent attempts: a reopen retires the superseded flow and starts
+ // its own count within one batched update (no flicker), and every attempt clears exactly
+ // once (never stuck).
  const [connectingCounts, setConnectingCounts] = useState<Map<string, number>>(() => new Map())
  // OAuth `state` nonce -> { serverId, safety timeout }. `state` keys the BroadcastChannel
  // correlation: the callback echoes it on every result (even failures that resolve no serverId),
@@ -153,27 +153,65 @@ 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?.()
+ // Opening the shared named window just navigated ANY prior authorization window to
+ // about:blank, so prior flows for this server are moot regardless of how this attempt
+ // ends — retire them now (a failed start must not leave a windowless flow "connecting"
+ // for the 10-minute safety timeout).
+ retireFlows(serverId)
  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.
- retireFlows(serverId)
  if (result.status === 'already_authorized') {
+ try {
+ popup.close()
+ } catch {}
  invalidateServer(serverId)
  decConnecting(serverId) // this attempt ends
  return
  }
+ const { authorizationUrl, state } = result
+ let navigated = true
+ try {
+ popup.location.replace(authorizationUrl)
+ } catch {
+ // A COOP-severed reused window can refuse scripted navigation; reopen by name.
+ // This runs after the await, so it can itself be popup-blocked — check it.
+ navigated = window.open(authorizationUrl, `mcp-oauth-${serverId}`) !== null
+ }
+ if (!navigated) {
+ try {
+ popup.close()
+ } catch {}
+ decConnecting(serverId)
+ toast.error('Popup blocked. Please allow popups for this site and retry.')
+ return
+ }
  // 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,16 +310,35 @@ 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() {
  return useMutation<StartMcpOauthMutationResult, Error, { serverId: string; workspaceId: string }>(
  {
  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.
+ // Feature-detect AbortSignal.timeout (Safari <16 lacks it) with a plain
+ // controller fallback.
+ let timeoutSignal: AbortSignal | undefined
+ let timeoutId: ReturnType<typeof setTimeout> | undefined
+ if (typeof AbortSignal.timeout === 'function') {
+ timeoutSignal = AbortSignal.timeout(30_000)
+ } else {
+ const controller = new AbortController()
+ timeoutId = setTimeout(() => controller.abort(new Error('Request timed out')), 30_000)
+ timeoutSignal = controller.signal
+ }
+ let result: Awaited<ReturnType<typeof requestJson<typeof startMcpOauthContract>>>
+ try {
+ result = await requestJson(startMcpOauthContract, {
+ query: { serverId, workspaceId },
+ signal: timeoutSignal,
+ })
+ } finally {
+ if (timeoutId !== undefined) clearTimeout(timeoutId)
+ }
  if (result.status === 'already_authorized') return { status: 'already_authorized' }
 
  const parsedUrl = new URL(result.authorizationUrl)
@@ -332,15 +351,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 }
  },
  }
  )