Skip to content

Commit 8c6166e

Browse files
j15zwaleedlatif1
andauthored
fix(mship): return the chat connect flow to the tab that started it (#6403)
* fix(mship): return the chat connect flow to the tab that started it Connecting an integration from a chat credential chip opened OAuth in a new tab and returned there, so the user landed on a second copy of the app while the conversation they started from sat stale behind it. The flow now runs in a popup and returns through a new self-closing page at /oauth/chat-complete, which publishes its verdict to the shared attempt record and closes. The chat tab picks that up over its storage listener and updates in place, so it never navigates. A blocked popup takes the same route in a new tab and still lands on the completion page, so both paths share one verdict source. That verdict is now the server's: reaching the completion page means Better Auth routed the flow to its success callback. The previous check diffed the workspace credential list, which reported failure whenever a user re-authorized an account they had already linked -- that path updates the account row and creates no new credential. The lock is stricter than the label. A failure to create the credential from its draft is swallowed server-side, so a flow can report success with nothing in the workspace; the row stays retryable unless the credential actually appears. Also: the popup is named per attempt so sibling rows cannot renavigate each other's window; a cross-origin connect URL keeps the anchor's noopener instead of taking the popup path; the focus verifier reads the attempt after its refetch so a verdict published mid-flight is not overwritten; and the verifier treats a popup parked on a terminal page (/oauth-error, the workspace error exit) as finished rather than waiting on it forever. * 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. * fix(mship): never read a disowned popup handle as a finished flow A provider page with COOP same-origin disowns the popup, and the disowned handle reports closed for a consent screen still running. The watcher took that as an ending and published 'failed' against a live flow. - Replace the boolean with a three-state observation. Only a same-origin terminal page counts as 'ended'; a closed-or-disowned handle is 'unobservable' and publishes no verdict. Closing a popup hands focus back to this tab anyway, so the focus verification settles that case. - Stop the interval before settling. The refetch leaves the status pending for its duration, so a running interval could fire again and resolve an attempt a retry had since replaced. - Gate the success toast on a launched-attempt ref rather than the window handle, which the watcher clears before React applies the verdict. * fix(oauth): keep the chat connect return leg in its opener's browsing context /oauth/chat-complete runs as a popup but fell into the strict COOP rule, so same-origin moved it into its own browsing-context group the moment it loaded — disowning it from the tab that opened it. That is the documented cause of a popup that is not reliably script-closable and whose opener sees window.closed report true for a live window. Matches it to its opener's same-origin-allow-popups instead, which is the directive the platform provides for exactly this case. * fix(oauth): keep every page an OAuth popup lands on observable to its opener The popup watcher settles on a same-origin terminal page, but both entries in OAUTH_POPUP_TERMINAL_PATHS were served strict same-origin COOP, which moves the popup into its own browsing-context group. The opener could then neither read its location nor trust window.closed, so the terminal-page branch could never fire in production and a flow exiting through one of those pages left the row waiting until the user happened to refocus the tab. Serves /oauth-error and the /workspace root the same same-origin-allow-popups their opener uses. The workspace root previously fell under the strict rule while every /workspace/... route already got the permissive one. * test(mship): cover the announcement surviving an early popup release The success toast is gated on the launched-attempt ref rather than the window handle; nothing pinned that. Adds the regression test, and trims the comment duplication the fix left behind. * fix(mship): bound the wait on a popup whose outcome became unobservable A closed handle and a COOP-disowned one are indistinguishable, so the watcher published no verdict for either and relied on the focus verification to settle it. That recovers the normal case — closing a popup hands focus back — but not one where the opener was never blurred, leaving the row waiting indefinitely. Arms the same safety timeout the MCP OAuth popup uses for the same reason: past it, the row decides from the credential list rather than waiting on a verdict that is never going to arrive. Cleared as soon as a real verdict lands. * fix(mship): survive a remount while an attempt is still pending The unobservable deadline lived in the watcher effect's closure, so it was armed only by the mount that launched the popup. The transcript virtualizes: a row scrolled away mid-connect came back with no window handle and no blur behind it, and nothing re-armed the bound. Derives the deadline from the attempt's own requestedAt and arms it for any pending attempt, so a remount inherits the time remaining rather than restarting the clock or losing it. A demonstrably live popup still owns the flow and is left to the watcher. * fix(mship): bind a settle to its own attempt and keep the deadline armed Two races the previous rounds left behind. A settle read the attempt only after its refetch, so a retry landing during that window was resolved by a run it never triggered — failing a replacement whose popup was still going. The attempt id is now captured before the await and the verdict only lands if it still matches; the status is still re-read after, so a verdict published mid-refetch is not overwritten. The safety deadline was one-shot. A consent screen that outlived it consumed the timeout while still live, leaving nothing to catch the popup dying unobservably afterwards. It now re-checks at the poll interval instead of expiring against a live window. * chore(mship): tighten the comments on the OAuth popup flow Trims the COOP rationale in next.config.ts to the point, and condenses the longest blocks in the connect hook without dropping the reasoning a reader needs to keep the invariants. --------- Co-authored-by: Waleed Latif <walif6@gmail.com>
1 parent b5e5ca5 commit 8c6166e

8 files changed

Lines changed: 1205 additions & 63 deletions

File tree

Lines changed: 135 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,135 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { act } from 'react'
5+
import { createRoot, type Root } from 'react-dom/client'
6+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
7+
import {
8+
createOAuthChatAttempt,
9+
type OAuthChatAttempt,
10+
readOAuthChatAttempt,
11+
} from '@/lib/credentials/oauth-chat-attempt'
12+
import { ChatCompleteHandoff } from '@/app/oauth/chat-complete/chat-complete-handoff'
13+
14+
function renderAt(search: string): { root: Root } {
15+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
16+
window.history.replaceState({}, '', `/oauth/chat-complete${search}`)
17+
const root: Root = createRoot(document.createElement('div'))
18+
act(() => root.render(<ChatCompleteHandoff />))
19+
return { root }
20+
}
21+
22+
describe('ChatCompleteHandoff', () => {
23+
let attempt: OAuthChatAttempt
24+
25+
beforeEach(() => {
26+
vi.clearAllMocks()
27+
window.localStorage.clear()
28+
vi.spyOn(window, 'close').mockImplementation(() => {})
29+
attempt = createOAuthChatAttempt({
30+
workspaceId: 'workspace-1',
31+
providerId: 'google-email',
32+
baseProviderId: 'google',
33+
displayName: 'Gmail',
34+
controlId: 'message-1:0:0',
35+
baselineCredentialIds: ['existing-gmail'],
36+
})
37+
})
38+
39+
it('publishes success on arrival, without requiring a new credential to appear', () => {
40+
// Re-authorizing an already-linked account updates the account row rather
41+
// than creating one, so no new credential lands — reaching this page is
42+
// still the server telling us the flow succeeded.
43+
const { root } = renderAt(`?oauthAttempt=${attempt.id}`)
44+
45+
expect(readOAuthChatAttempt(attempt.id)?.status).toBe('connected')
46+
expect(window.close).toHaveBeenCalledOnce()
47+
act(() => root.unmount())
48+
})
49+
50+
it('publishes failure when the provider returned an error', () => {
51+
const { root } = renderAt(`?oauthAttempt=${attempt.id}&error=access_denied`)
52+
53+
expect(readOAuthChatAttempt(attempt.id)?.status).toBe('failed')
54+
act(() => root.unmount())
55+
})
56+
57+
it('leaves an unrelated attempt untouched when no attempt is named', () => {
58+
const { root } = renderAt('')
59+
60+
expect(readOAuthChatAttempt(attempt.id)?.status).toBe('pending')
61+
expect(window.close).toHaveBeenCalledOnce()
62+
act(() => root.unmount())
63+
})
64+
65+
it('still publishes the verdict when the attempt store rejects the write', () => {
66+
const setItem = vi.spyOn(Storage.prototype, 'setItem').mockImplementation(() => {
67+
throw new DOMException('quota', 'QuotaExceededError')
68+
})
69+
70+
// The verdict is lost, but the window must still be released — an
71+
// unguarded throw would strand the popup open on this page.
72+
expect(() => renderAt(`?oauthAttempt=${attempt.id}`)).not.toThrow()
73+
expect(window.close).toHaveBeenCalledOnce()
74+
setItem.mockRestore()
75+
})
76+
77+
describe('close-refused fallback', () => {
78+
const realLocation = window.location
79+
80+
afterEach(() => {
81+
vi.useRealTimers()
82+
Object.defineProperty(window, 'location', { configurable: true, value: realLocation })
83+
})
84+
85+
/**
86+
* jsdom performs no navigation and forbids redefining `location.replace`,
87+
* so the whole location is swapped for a stub carrying only what the
88+
* handoff reads: the current href, the origin, and the redirect sink.
89+
*/
90+
function renderWithStubbedLocation(search: string): { calls: string[]; root: Root } {
91+
const calls: string[] = []
92+
Object.defineProperty(window, 'location', {
93+
configurable: true,
94+
value: {
95+
href: `https://sim.test/oauth/chat-complete${search}`,
96+
origin: 'https://sim.test',
97+
replace: (url: string) => calls.push(url),
98+
},
99+
})
100+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
101+
const root: Root = createRoot(document.createElement('div'))
102+
act(() => root.render(<ChatCompleteHandoff />))
103+
return { calls, root }
104+
}
105+
106+
it('redirects to a same-origin returnTo once the close is refused', () => {
107+
vi.useFakeTimers()
108+
const returnTo = 'https://sim.test/workspace/workspace-1/chat/chat-1'
109+
110+
const { calls, root } = renderWithStubbedLocation(
111+
`?oauthAttempt=${attempt.id}&returnTo=${encodeURIComponent(returnTo)}`
112+
)
113+
act(() => {
114+
vi.advanceTimersByTime(400)
115+
})
116+
117+
expect(calls).toEqual([returnTo])
118+
act(() => root.unmount())
119+
})
120+
121+
it('refuses a cross-origin returnTo and falls back to the workspace', () => {
122+
vi.useFakeTimers()
123+
124+
const { calls, root } = renderWithStubbedLocation(
125+
`?oauthAttempt=${attempt.id}&returnTo=${encodeURIComponent('https://evil.example/steal')}`
126+
)
127+
act(() => {
128+
vi.advanceTimersByTime(400)
129+
})
130+
131+
expect(calls).toEqual(['/workspace'])
132+
act(() => root.unmount())
133+
})
134+
})
135+
})
Lines changed: 66 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,66 @@
1+
'use client'
2+
3+
import { useEffect, useRef } from 'react'
4+
import {
5+
OAUTH_CHAT_ATTEMPT_PARAM,
6+
OAUTH_CHAT_RETURN_TO_PARAM,
7+
setOAuthChatAttemptStatus,
8+
} from '@/lib/credentials/oauth-chat-attempt'
9+
10+
const CLOSE_FALLBACK_DELAY_MS = 400
11+
12+
/**
13+
* The fallback redirect must never leave this origin — the target rides in a
14+
* query param the user could have tampered with.
15+
*/
16+
function sanitizeReturnTo(raw: string | null): string | null {
17+
if (!raw) return null
18+
try {
19+
const url = new URL(raw, window.location.origin)
20+
return url.origin === window.location.origin ? url.toString() : null
21+
} catch {
22+
return null
23+
}
24+
}
25+
26+
/**
27+
* Behavior half of the chat OAuth return leg: publishes the verdict to the
28+
* attempt record — which the chat tab's chip picks up over its storage
29+
* listener — then closes the window. Renders nothing, so the page's frame
30+
* paints as server markup before this hydrates.
31+
*
32+
* Reaching this page IS the verdict: Better Auth routes a flow here only as its
33+
* success `callbackURL`, sending failures to `/oauth-error` or back here with
34+
* an `error` code. That beats diffing the credential list, which calls a
35+
* re-authorized account a failure — that path updates the existing account row
36+
* and creates nothing for a diff to find.
37+
*
38+
* A window the browser refuses to close redirects to the chat instead (the
39+
* popup-blocked path opens this leg in a tab, which no script may close). That
40+
* URL carries no attempt id on purpose: the verdict is already published, and
41+
* the destination would otherwise re-decide it by the very diff above.
42+
*/
43+
export function ChatCompleteHandoff() {
44+
const ranRef = useRef(false)
45+
46+
useEffect(() => {
47+
if (ranRef.current) return
48+
ranRef.current = true
49+
50+
const params = new URL(window.location.href).searchParams
51+
const attemptId = params.get(OAUTH_CHAT_ATTEMPT_PARAM)
52+
const returnTo = sanitizeReturnTo(params.get(OAUTH_CHAT_RETURN_TO_PARAM))
53+
54+
if (attemptId) {
55+
setOAuthChatAttemptStatus(attemptId, params.has('error') ? 'failed' : 'connected')
56+
}
57+
58+
window.close()
59+
const timer = window.setTimeout(() => {
60+
window.location.replace(returnTo ?? '/workspace')
61+
}, CLOSE_FALLBACK_DELAY_MS)
62+
return () => window.clearTimeout(timer)
63+
}, [])
64+
65+
return null
66+
}
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
import type { Metadata } from 'next'
2+
import { DesktopHandoffShell } from '@/app/desktop/components/desktop-handoff-shell'
3+
import { ChatCompleteHandoff } from '@/app/oauth/chat-complete/chat-complete-handoff'
4+
5+
export const metadata: Metadata = {
6+
title: 'Returning to Sim',
7+
robots: { index: false },
8+
}
9+
10+
/**
11+
* Post-OAuth return leg for the chat credential chips. The chip rewrites the
12+
* authorize URL's return param to land here, so the OAuth window finishes on
13+
* this page — which signals the chat tab and closes — instead of loading a
14+
* second copy of the app.
15+
*
16+
* Shown for a few hundred milliseconds in a popup, or briefly in the original
17+
* tab when the popup was blocked, so it wears the same handoff frame as the
18+
* other minimal-chrome gates (the 404, the desktop connect screens) rather
19+
* than styling of its own.
20+
*/
21+
export default function ChatCompletePage() {
22+
return (
23+
<>
24+
<ChatCompleteHandoff />
25+
<DesktopHandoffShell title='Finishing the connection' description='Returning you to Sim.' />
26+
</>
27+
)
28+
}

0 commit comments

Comments
 (0)