Skip to content

Commit 1d84f3c

Browse files
waleedlatif1waleedlatif1
committed
fix(mcp): decouple OAuth result correlation from popup.closed
Cursor flagged two defects in the previous gate, both rooted in keying the BroadcastChannel filter on `popupIntervalsRef` (the popup.closed poll map): - A genuine completion was dropped whenever the poll had already removed the server's entry — and COOP can make `popup.closed` misreport, which is exactly the case this PR targets, so the fix could silently fail to apply. - A result without a serverId fell back to "any in-flight popup", waking unrelated same-origin tabs with spurious toasts/refetches. Introduce `pendingFlowsRef` (serverId -> safety timeout) as the correlation source of truth: cleared only on completion or a 10-min timeout (matching the server OAuth start TTL), never by popup.closed. The popup.closed poll now only clears the spinner (best-effort abandon UX) and never touches correlation, so a real completion is always processed. Results without a serverId are ignored; the initiating tab's safety timeout clears its own "Connecting…".
1 parent 4c57c51 commit 1d84f3c

1 file changed

Lines changed: 84 additions & 64 deletions

File tree

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

Lines changed: 84 additions & 64 deletions
Original file line numberDiff line numberDiff line change
@@ -37,18 +37,64 @@ interface UseMcpOauthPopupProps {
3737
workspaceId: string
3838
}
3939

40+
/**
41+
* Bounds how long a row shows "Connecting…" without a result. Matches the server-side OAuth
42+
* start TTL: once it lapses the authorization state has expired and the flow can no longer
43+
* complete, so a still-pending flow is safe to drop.
44+
*/
45+
const OAUTH_FLOW_TIMEOUT_MS = 10 * 60 * 1000
46+
4047
export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
4148
const queryClient = useQueryClient()
4249
const { mutateAsync: startOauth } = useStartMcpOauth()
4350

4451
const [connectingServers, setConnectingServers] = useState<Set<string>>(() => new Set())
45-
const popupIntervalsRef = useRef<Map<string, number>>(new Map())
52+
// serverId -> safety timeout. This is the correlation source of truth for the
53+
// BroadcastChannel gate: it is cleared only when the flow completes or times out, never by
54+
// popup.closed polling — COOP can make popup.closed misreport, and clearing it early would
55+
// drop the genuine completion this fix exists to deliver.
56+
const pendingFlowsRef = useRef<Map<string, number>>(new Map())
57+
// serverId -> popup.closed poll. Best-effort fast "Connecting…" clear when the user
58+
// abandons the popup; never used to correlate a result.
59+
const popupPollsRef = useRef<Map<string, number>>(new Map())
60+
61+
const stopConnecting = useCallback((serverId: string) => {
62+
setConnectingServers((prev) => {
63+
if (!prev.has(serverId)) return prev
64+
const next = new Set(prev)
65+
next.delete(serverId)
66+
return next
67+
})
68+
}, [])
69+
70+
const stopPopupPoll = useCallback((serverId: string) => {
71+
const poll = popupPollsRef.current.get(serverId)
72+
if (poll !== undefined) {
73+
window.clearInterval(poll)
74+
popupPollsRef.current.delete(serverId)
75+
}
76+
}, [])
77+
78+
/** End a flow entirely: stop the spinner, its safety timeout, and its popup poll. */
79+
const settleFlow = useCallback(
80+
(serverId: string) => {
81+
const timeout = pendingFlowsRef.current.get(serverId)
82+
if (timeout !== undefined) window.clearTimeout(timeout)
83+
pendingFlowsRef.current.delete(serverId)
84+
stopPopupPoll(serverId)
85+
stopConnecting(serverId)
86+
},
87+
[stopConnecting, stopPopupPoll]
88+
)
4689

4790
useEffect(() => {
48-
const intervals = popupIntervalsRef.current
91+
const pending = pendingFlowsRef.current
92+
const polls = popupPollsRef.current
4993
return () => {
50-
for (const id of intervals.values()) window.clearInterval(id)
51-
intervals.clear()
94+
for (const t of pending.values()) window.clearTimeout(t)
95+
for (const p of polls.values()) window.clearInterval(p)
96+
pending.clear()
97+
polls.clear()
5298
}
5399
}, [])
54100

@@ -61,90 +107,64 @@ export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
61107
channel.onmessage = (event) => {
62108
const data = event.data as Partial<McpOauthCallbackMessage> | null
63109
if (data?.type !== 'mcp-oauth') return
64-
// A BroadcastChannel reaches every same-origin tab, so only the tab that actually
65-
// opened this flow's popup should react — otherwise an unrelated tab (any workspace)
66-
// would clear state, refetch, and show a spurious toast. `popupIntervalsRef` holds
67-
// this tab's in-flight popups; a result for a server it never opened is not ours.
68-
const initiatedHere = data.serverId
69-
? popupIntervalsRef.current.has(data.serverId)
70-
: popupIntervalsRef.current.size > 0
71-
if (!initiatedHere) return
72-
if (data.serverId) {
73-
const serverId = data.serverId
74-
const interval = popupIntervalsRef.current.get(serverId)
75-
if (interval !== undefined) {
76-
window.clearInterval(interval)
77-
popupIntervalsRef.current.delete(serverId)
78-
}
79-
setConnectingServers((prev) => {
80-
if (!prev.has(serverId)) return prev
81-
const next = new Set(prev)
82-
next.delete(serverId)
83-
return next
84-
})
85-
} else if (!data.ok) {
86-
// Early callback failures (missing params, invalid state) post back
87-
// without a serverId, so we can't target a specific row — clear all
88-
// in-flight popups instead of leaving the UI stuck on "Connecting…".
89-
for (const id of popupIntervalsRef.current.values()) window.clearInterval(id)
90-
popupIntervalsRef.current.clear()
91-
setConnectingServers((prev) => (prev.size === 0 ? prev : new Set()))
92-
}
110+
// A BroadcastChannel reaches every same-origin tab, so react only to a result for a
111+
// flow THIS tab started. A result without a serverId can't be correlated to a flow, so
112+
// it's ignored here — the initiating tab's safety timeout clears its own "Connecting…".
113+
if (!data.serverId || !pendingFlowsRef.current.has(data.serverId)) return
114+
settleFlow(data.serverId)
93115
if (data.ok) {
94116
queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(workspaceId) })
95-
if (data.serverId) {
96-
queryClient.invalidateQueries({
97-
queryKey: mcpKeys.serverToolsList(workspaceId, data.serverId),
98-
})
99-
} else {
100-
queryClient.invalidateQueries({
101-
queryKey: mcpKeys.serverToolsWorkspace(workspaceId),
102-
})
103-
}
117+
queryClient.invalidateQueries({
118+
queryKey: mcpKeys.serverToolsList(workspaceId, data.serverId),
119+
})
104120
queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) })
105121
toast.success('Server authorized')
106122
} else {
107123
toast.error(reasonToMessage(data.reason))
108124
}
109125
}
110126
return () => channel.close()
111-
}, [queryClient, workspaceId])
127+
}, [queryClient, workspaceId, settleFlow])
112128

113129
const startOauthForServer = useCallback(
114130
async (serverId: string) => {
115131
setConnectingServers((prev) => new Set(prev).add(serverId))
116-
const clear = () => {
117-
const existing = popupIntervalsRef.current.get(serverId)
118-
if (existing !== undefined) {
119-
window.clearInterval(existing)
120-
popupIntervalsRef.current.delete(serverId)
121-
}
122-
setConnectingServers((prev) => {
123-
const next = new Set(prev)
124-
next.delete(serverId)
125-
return next
126-
})
127-
}
128132
try {
129133
const result = await startOauth({ serverId, workspaceId })
130134
if (result.status === 'already_authorized') {
131-
clear()
135+
settleFlow(serverId)
132136
return
133137
}
134138
const { popup } = result
135-
const existing = popupIntervalsRef.current.get(serverId)
136-
if (existing !== undefined) window.clearInterval(existing)
137-
const interval = window.setInterval(() => {
138-
if (popup.closed) clear()
139-
}, 500)
140-
popupIntervalsRef.current.set(serverId, interval)
139+
// Track this in-flight flow for the BroadcastChannel gate, bounded by a safety
140+
// timeout in case no result ever arrives (popup abandoned, or a callback failure
141+
// that couldn't carry a serverId).
142+
const existingTimeout = pendingFlowsRef.current.get(serverId)
143+
if (existingTimeout !== undefined) window.clearTimeout(existingTimeout)
144+
pendingFlowsRef.current.set(
145+
serverId,
146+
window.setTimeout(() => settleFlow(serverId), OAUTH_FLOW_TIMEOUT_MS)
147+
)
148+
// Best-effort: clear "Connecting…" quickly when the user closes the popup without
149+
// finishing. popup.closed can misreport under COOP, so this only stops the spinner —
150+
// it never touches `pendingFlowsRef`, so it can't drop a real result.
151+
stopPopupPoll(serverId)
152+
popupPollsRef.current.set(
153+
serverId,
154+
window.setInterval(() => {
155+
if (popup.closed) {
156+
stopPopupPoll(serverId)
157+
stopConnecting(serverId)
158+
}
159+
}, 500)
160+
)
141161
} catch (e) {
142-
clear()
162+
settleFlow(serverId)
143163
logger.error('Failed to start MCP OAuth', e)
144164
toast.error(toError(e).message || 'Failed to start authorization')
145165
}
146166
},
147-
[startOauth, workspaceId]
167+
[startOauth, workspaceId, settleFlow, stopConnecting, stopPopupPoll]
148168
)
149169

150170
return { connectingServers, startOauthForServer }

0 commit comments

Comments
 (0)