Skip to content

Commit 58f5e88

Browse files
committed
fix(mship): settle the connect row from the popup, not from focus alone
Addresses the review findings on the chat OAuth return leg. - Watch the popup on an interval. A provider interstitial bouncing to the workspace root, a denied consent on /oauth-error, or a closed window all end the flow without publishing a verdict or firing any event in this tab, so the row waited forever. The focus handler also no longer consumes the away flag when it defers to a live popup. - Focus an already-running popup on a repeat click instead of starting a rival attempt, which orphaned the first flow's verdict on an attempt id the row had stopped reading. - Settle from the refetched credentials on the popup success path, so the row's lock is corroborated and a connected row stops being clickable. Extracts the shared refetch-then-decide step into settleFromCredentials, used by the focus handler, the popup watcher, and the success path.
1 parent e625525 commit 58f5e88

2 files changed

Lines changed: 286 additions & 44 deletions

File tree

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.tsx

Lines changed: 197 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -505,6 +505,133 @@ describe('CredentialDisplay link tag', () => {
505505
act(() => root.unmount())
506506
})
507507

508+
it('settles the row from the popup watcher when the flow ends with no event in this tab', async () => {
509+
// A provider interstitial (Trello parks on one for ~3s) bounces to the
510+
// workspace root without ever reaching the completion page. Neither page
511+
// publishes a verdict, and a user who never leaves this tab gets no focus
512+
// event either — only the watcher can end the wait.
513+
vi.useFakeTimers()
514+
const popup = {
515+
focus: vi.fn(),
516+
closed: false,
517+
location: { origin: 'https://sim.test', pathname: '/api/auth/trello/callback' },
518+
}
519+
const openSpy = vi
520+
.spyOn(window, 'open')
521+
.mockReturnValue(popup as unknown as ReturnType<typeof window.open>)
522+
const { container, root } = renderCredentialLink({
523+
type: 'link',
524+
provider: 'google-email',
525+
value:
526+
'https://sim.test/api/auth/oauth2/authorize?providerId=google-email&callbackURL=https%3A%2F%2Fsim.test%2Fworkspace%2Fworkspace-1%2Fchat%2Fchat-1',
527+
})
528+
529+
await act(async () => {
530+
container
531+
.querySelector('a')
532+
?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }))
533+
})
534+
await act(async () => {
535+
await vi.advanceTimersByTimeAsync(2000)
536+
})
537+
538+
// The interstitial is not terminal, so the row is still deferring.
539+
expect(container.textContent).toContain('Waiting for Gmail connection')
540+
541+
popup.location.pathname = '/workspace'
542+
await act(async () => {
543+
await vi.advanceTimersByTimeAsync(2000)
544+
})
545+
546+
expect(container.textContent).toContain('Not connected — connect Gmail')
547+
vi.useRealTimers()
548+
openSpy.mockRestore()
549+
act(() => root.unmount())
550+
})
551+
552+
it('focuses the live popup instead of starting a rival attempt on a repeat click', async () => {
553+
const toastSuccess = vi.spyOn(toast, 'success').mockImplementation(() => '')
554+
const popup = {
555+
focus: vi.fn(),
556+
closed: false,
557+
// Still on the provider's pages: reading location across origins throws.
558+
get location(): Location {
559+
throw new DOMException('cross-origin', 'SecurityError')
560+
},
561+
}
562+
const openSpy = vi
563+
.spyOn(window, 'open')
564+
.mockReturnValue(popup as unknown as ReturnType<typeof window.open>)
565+
const { container, root } = renderCredentialLink({
566+
type: 'link',
567+
provider: 'google-email',
568+
value:
569+
'https://sim.test/api/auth/oauth2/authorize?providerId=google-email&callbackURL=https%3A%2F%2Fsim.test%2Fworkspace%2Fworkspace-1%2Fchat%2Fchat-1',
570+
})
571+
const link = container.querySelector('a')
572+
573+
await act(async () => {
574+
link?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }))
575+
})
576+
const attemptId = new URL(
577+
new URL(openSpy.mock.calls[0][0] as string).searchParams.get('callbackURL') ?? ''
578+
).searchParams.get('oauthAttempt') as string
579+
await act(async () => {
580+
link?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }))
581+
})
582+
583+
// No second window, and no second attempt to strand the first one's verdict.
584+
expect(openSpy).toHaveBeenCalledOnce()
585+
expect(popup.focus).toHaveBeenCalledTimes(2)
586+
587+
await act(async () => {
588+
setOAuthChatAttemptStatus(attemptId, 'connected')
589+
})
590+
591+
expect(container.textContent).toContain('Connected Gmail')
592+
openSpy.mockRestore()
593+
toastSuccess.mockRestore()
594+
act(() => root.unmount())
595+
})
596+
597+
it('locks the row once the popup verdict is corroborated by a refetched credential', async () => {
598+
const toastSuccess = vi.spyOn(toast, 'success').mockImplementation(() => '')
599+
const popup = { focus: vi.fn(), closed: false }
600+
const openSpy = vi
601+
.spyOn(window, 'open')
602+
.mockReturnValue(popup as unknown as ReturnType<typeof window.open>)
603+
mockRefetchWorkspaceCredentials.mockResolvedValue({
604+
data: [{ id: 'new-gmail', providerId: 'google', updatedAt: '2026-08-07T10:05:00Z' }],
605+
})
606+
const { container, root } = renderCredentialLink({
607+
type: 'link',
608+
provider: 'google-email',
609+
value:
610+
'https://sim.test/api/auth/oauth2/authorize?providerId=google-email&callbackURL=https%3A%2F%2Fsim.test%2Fworkspace%2Fworkspace-1%2Fchat%2Fchat-1',
611+
})
612+
613+
await act(async () => {
614+
container
615+
.querySelector('a')
616+
?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }))
617+
})
618+
const attemptId = new URL(
619+
new URL(openSpy.mock.calls[0][0] as string).searchParams.get('callbackURL') ?? ''
620+
).searchParams.get('oauthAttempt') as string
621+
await act(async () => {
622+
setOAuthChatAttemptStatus(attemptId, 'connected')
623+
})
624+
625+
// The popup flow never blurs this tab, so this refetch is the only one that
626+
// can corroborate the verdict and let the control lock.
627+
expect(mockRefetchWorkspaceCredentials).toHaveBeenCalled()
628+
expect(container.textContent).toContain('Connected Gmail')
629+
expect(container.querySelector('a')?.getAttribute('aria-disabled')).toBe('true')
630+
openSpy.mockRestore()
631+
toastSuccess.mockRestore()
632+
act(() => root.unmount())
633+
})
634+
508635
it('navigates the tab through the same completion page when the popup is blocked', () => {
509636
const openSpy = vi.spyOn(window, 'open').mockReturnValue(null)
510637
const { container, root } = renderCredentialLink({
@@ -816,6 +943,76 @@ describe('CredentialDisplay link tag', () => {
816943
expect(container.querySelector('input')).toBeNull()
817944
act(() => root.unmount())
818945
})
946+
947+
it('keeps a typed secret while a sibling row runs its OAuth connect', async () => {
948+
// The card's secret drafts live in component state until its Submit, so a
949+
// connect that navigates this tab away discards whatever the user already
950+
// typed into a sibling row. Running the flow in a popup is what makes the
951+
// mixed card safe: this tab is never unloaded, so the draft outlives the
952+
// connect — including the re-render its verdict and refetch trigger.
953+
const toastSuccess = vi.spyOn(toast, 'success').mockImplementation(() => '')
954+
const popup = { focus: vi.fn(), closed: false }
955+
const openSpy = vi
956+
.spyOn(window, 'open')
957+
.mockReturnValue(popup as unknown as ReturnType<typeof window.open>)
958+
const container = document.createElement('div')
959+
const root: Root = createRoot(container)
960+
const onOptionSelect = vi.fn()
961+
const data: CredentialItemData[] = [
962+
{
963+
type: 'link',
964+
provider: 'google-email',
965+
value:
966+
'https://sim.test/api/auth/oauth2/authorize?providerId=google-email&callbackURL=https%3A%2F%2Fsim.test%2Fworkspace%2Fworkspace-1%2Fchat%2Fchat-1',
967+
},
968+
{ type: 'secret_input', name: 'OPENAI_API_KEY' },
969+
]
970+
act(() => {
971+
root.render(
972+
<SpecialTags segment={{ type: 'credential', data }} onOptionSelect={onOptionSelect} />
973+
)
974+
})
975+
976+
const secretInput = container.querySelector('input')
977+
act(() => {
978+
if (!secretInput) return
979+
const valueSetter = Object.getOwnPropertyDescriptor(
980+
window.HTMLInputElement.prototype,
981+
'value'
982+
)?.set
983+
valueSetter?.call(secretInput, 'sk-test-key')
984+
secretInput.dispatchEvent(new Event('input', { bubbles: true }))
985+
})
986+
987+
await act(async () => {
988+
container
989+
.querySelector('a')
990+
?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }))
991+
})
992+
const attemptId = new URL(
993+
new URL(openSpy.mock.calls[0][0] as string).searchParams.get('callbackURL') ?? ''
994+
).searchParams.get('oauthAttempt') as string
995+
await act(async () => {
996+
setOAuthChatAttemptStatus(attemptId, 'connected')
997+
})
998+
999+
// The field masks while unfocused, so assert on what the draft is actually
1000+
// for: submitting it. A mask of the right length is not proof it survived.
1001+
const submitButton = Array.from(container.querySelectorAll('button')).find(
1002+
(button) => button.textContent === 'Submit'
1003+
)
1004+
await act(async () => {
1005+
submitButton?.click()
1006+
})
1007+
1008+
expect(mockUpsertWorkspaceEnvironment).toHaveBeenCalledWith({
1009+
workspaceId: 'workspace-1',
1010+
variables: { OPENAI_API_KEY: 'sk-test-key' },
1011+
})
1012+
openSpy.mockRestore()
1013+
toastSuccess.mockRestore()
1014+
act(() => root.unmount())
1015+
})
8191016
})
8201017

8211018
describe('parseSpecialTags sim_key placeholder', () => {

apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/use-oauth-chip-connection.ts

Lines changed: 89 additions & 44 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,12 @@ import { useWorkspaceCredentials } from '@/hooks/queries/credentials'
2828
const OAUTH_POPUP_WINDOW_NAME = 'sim-oauth-connect'
2929
/** Matches the MCP OAuth popup (`hooks/mcp/use-mcp-oauth-popup.ts`) so the two consent windows open alike. */
3030
const OAUTH_POPUP_FEATURES = 'width=560,height=720,resizable=yes,scrollbars=yes'
31+
/**
32+
* How often to re-examine a live popup. Matches the MCP OAuth popup's own
33+
* watcher: a closed or terminal window fires no event in the opener, so
34+
* polling the handle is the only way to observe it.
35+
*/
36+
const OAUTH_POPUP_POLL_INTERVAL_MS = 400
3137

3238
/**
3339
* Same-origin pages an OAuth flow can die on without ever reaching the return
@@ -266,6 +272,44 @@ export function useOAuthChipConnection({
266272
})
267273
}, [activeAttemptId, controlId, providerId, reconnectCredentialId, workspaceId])
268274

275+
/**
276+
* Refetches the workspace credentials and settles this row's pending attempt
277+
* against them. Shared by the two endings the completion page cannot cover: a
278+
* return to this tab, and a popup seen closed or parked on a terminal page.
279+
* Idempotent — an attempt is only rewritten while still `pending`.
280+
*/
281+
const settleFromCredentials = useCallback(async () => {
282+
const result = await refetchWorkspaceOAuthCredentials()
283+
const credentials = result.data ?? []
284+
285+
// Also covers a credential connected from another tab, which this row never
286+
// launched and query state alone would surface only on its next fetch.
287+
const storedBaseline = workspaceCredentialBaselineRef.current
288+
if (!reconnectCredentialId && storedBaseline?.scope === credentialScope) {
289+
setConnectedFromWorkspaceChange(
290+
hasOAuthCredentialChanged({ ...credentialTarget, ...storedBaseline.baseline }, credentials)
291+
)
292+
}
293+
294+
// Read after the await: a snapshot taken before it goes stale the moment the
295+
// popup publishes its verdict mid-refetch, and that stale 'pending' would
296+
// license the very overwrite the guard below exists to prevent.
297+
const attempt = readRowAttempt()
298+
if (!attempt || attempt.status !== 'pending') return
299+
// Only the callback path can prove a reconnect returned — an unrelated edit
300+
// to the credential is indistinguishable from one here.
301+
const attemptConnected = reconnectCredentialId
302+
? false
303+
: hasOAuthCredentialChanged(attempt, credentials)
304+
setOAuthChatAttemptStatus(attempt.id, attemptConnected ? 'connected' : 'failed')
305+
}, [
306+
credentialScope,
307+
credentialTarget,
308+
readRowAttempt,
309+
reconnectCredentialId,
310+
refetchWorkspaceOAuthCredentials,
311+
])
312+
269313
useEffect(() => {
270314
const syncStatus = () => setConnectionStatus(readRowAttempt()?.status ?? null)
271315
window.addEventListener(OAUTH_CHAT_ATTEMPT_EVENT, syncStatus)
@@ -281,47 +325,19 @@ export function useOAuthChipConnection({
281325
const markAway = () => {
282326
oauthWindowWasAwayRef.current = true
283327
}
284-
const verifyAfterReturn = async () => {
328+
const verifyAfterReturn = () => {
285329
if (!oauthWindowWasAwayRef.current || document.visibilityState !== 'visible') return
286-
oauthWindowWasAwayRef.current = false
287-
const result = await refetchWorkspaceOAuthCredentials()
288-
const credentials = result.data ?? []
289-
290-
// Refetching on every return closes the other-tab gap even when this row
291-
// did not launch the connection. Query state normally drives the effect
292-
// above; updating here as well makes the result immediate and deterministic.
293-
const storedBaseline = workspaceCredentialBaselineRef.current
294-
if (!reconnectCredentialId && storedBaseline?.scope === credentialScope) {
295-
setConnectedFromWorkspaceChange(
296-
hasOAuthCredentialChanged(
297-
{ ...credentialTarget, ...storedBaseline.baseline },
298-
credentials
299-
)
300-
)
301-
}
302-
303-
// Read the attempt only after the await. A snapshot taken before it goes
304-
// stale the moment the popup publishes its verdict mid-refetch, and the
305-
// stale 'pending' would license the overwrite this check exists to stop —
306-
// deterministically to 'failed' on the reconnect branch below.
307-
const attempt = readRowAttempt()
308-
if (!attempt || attempt.status !== 'pending') return
309-
// Clicking back to this tab while the popup is still on the provider's
310-
// consent screen is not a verdict — the flow it owns has not returned
311-
// yet, and settling here would flash "not connected" under a connect the
312-
// user is midway through. Its own return leg publishes the result.
330+
// A live popup still owns the flow; settling now would flash "not
331+
// connected" mid-connect. The away flag deliberately survives this bail —
332+
// consuming it would spend the only signal a later return has to work
333+
// with, and no second focus event is guaranteed to arrive.
313334
if (isPopupStillOpen(popupRef.current)) return
314-
// A reconnect is verified by the callback path, which has proof that the
315-
// OAuth flow returned. A plain focus event cannot distinguish it from an
316-
// unrelated edit to the same credential.
317-
const attemptConnected = reconnectCredentialId
318-
? false
319-
: hasOAuthCredentialChanged(attempt, credentials)
320-
setOAuthChatAttemptStatus(attempt.id, attemptConnected ? 'connected' : 'failed')
335+
oauthWindowWasAwayRef.current = false
336+
void settleFromCredentials()
321337
}
322338
const handleVisibilityChange = () => {
323339
if (document.visibilityState === 'hidden') markAway()
324-
else void verifyAfterReturn()
340+
else verifyAfterReturn()
325341
}
326342

327343
window.addEventListener('blur', markAway)
@@ -332,13 +348,24 @@ export function useOAuthChipConnection({
332348
window.removeEventListener('focus', verifyAfterReturn)
333349
document.removeEventListener('visibilitychange', handleVisibilityChange)
334350
}
335-
}, [
336-
credentialScope,
337-
credentialTarget,
338-
readRowAttempt,
339-
reconnectCredentialId,
340-
refetchWorkspaceOAuthCredentials,
341-
])
351+
}, [settleFromCredentials])
352+
353+
/**
354+
* Watches a live popup for the endings that publish no verdict — a provider
355+
* interstitial exiting to the workspace root, a denied consent landing on the
356+
* OAuth error page, or the user closing the window. None reach the completion
357+
* page or fire an event here, and the user need never leave this tab for a
358+
* focus event either, so polling is the only way the row learns it is over.
359+
*/
360+
useEffect(() => {
361+
if (connectionStatus !== 'pending' || !popupRef.current) return
362+
const poll = window.setInterval(() => {
363+
if (isPopupStillOpen(popupRef.current)) return
364+
popupRef.current = null
365+
void settleFromCredentials()
366+
}, OAUTH_POPUP_POLL_INTERVAL_MS)
367+
return () => window.clearInterval(poll)
368+
}, [connectionStatus, settleFromCredentials])
342369

343370
useEffect(() => {
344371
if (connected) onConnectedRef.current?.()
@@ -356,10 +383,14 @@ export function useOAuthChipConnection({
356383
if (connectionStatus !== 'connected' || !popup) return
357384
const attempt = readOAuthChatAttempt(popup.attemptId)
358385
popupRef.current = null
386+
// The verdict settles the label, but the lock also waits on the credential
387+
// being observable. A popup flow never blurs this tab, so nothing else
388+
// refetches here and the row would read "Connected" yet stay clickable.
389+
void settleFromCredentials()
359390
if (attempt?.status === 'connected') {
360391
toast.success(`${attempt.displayName} connected successfully.`)
361392
}
362-
}, [connectionStatus])
393+
}, [connectionStatus, settleFromCredentials])
363394

364395
/**
365396
* Desktop app: OAuth cannot run in an embedded window — not in the app
@@ -376,6 +407,20 @@ export function useOAuthChipConnection({
376407
event.preventDefault()
377408
return
378409
}
410+
// A second click would replace both the active attempt and the window
411+
// handle, orphaning the first flow — its verdict lands on an attempt id
412+
// the row no longer reads, so a successful connect waits forever. The
413+
// live window is the flow; surface it instead of starting a rival one.
414+
const livePopup = popupRef.current
415+
if (isPopupStillOpen(livePopup)) {
416+
event.preventDefault()
417+
try {
418+
livePopup?.window.focus?.()
419+
} catch {
420+
// A COOP-severed handle refuses focus, but still owns the flow.
421+
}
422+
return
423+
}
379424
const attempt = createOAuthChatAttempt({
380425
workspaceId,
381426
providerId,

0 commit comments

Comments
 (0)