Skip to content

Commit f41e66c

Browse files
committed
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.
1 parent 8d3c519 commit f41e66c

2 files changed

Lines changed: 38 additions & 65 deletions

File tree

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

Lines changed: 32 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -34,32 +34,22 @@ const OAUTH_POPUP_FEATURES = 'width=560,height=720,resizable=yes,scrollbars=yes'
3434
* polling the handle is the only way to observe it.
3535
*/
3636
const OAUTH_POPUP_POLL_INTERVAL_MS = 400
37-
/**
38-
* How long to keep waiting on a popup whose outcome became unobservable before
39-
* deciding from the credential list anyway. Matches the MCP OAuth popup's
40-
* safety timeout — long enough not to cut a slow consent short, short enough
41-
* that an abandoned flow does not leave the row waiting forever.
42-
*/
37+
/** Matches the MCP OAuth popup's safety timeout: bounds a flow whose outcome never becomes observable. */
4338
const OAUTH_POPUP_UNOBSERVABLE_TIMEOUT_MS = 10 * 60 * 1000
4439

4540
/**
46-
* Same-origin pages an OAuth flow can die on without ever reaching the return
47-
* leg. Better Auth sends pre-state failures — a denied consent, most often —
48-
* to its global error page rather than this flow's callback, and the
49-
* custom-provider callbacks exit to the workspace root with an `error` param.
50-
* Neither publishes a verdict, so a popup sitting on one is finished, not
51-
* in flight.
41+
* Same-origin pages an OAuth flow can die on without reaching the return leg —
42+
* Better Auth sends pre-state failures (usually a denied consent) to its global
43+
* error page, and the custom-provider callbacks exit to the workspace root.
44+
* Neither publishes a verdict, so a popup sitting on one is finished.
5245
*/
5346
const OAUTH_POPUP_TERMINAL_PATHS = new Set(['/oauth-error', '/workspace'])
5447

5548
/**
56-
* What the opener can actually prove about a popup it launched.
57-
*
58-
* `ended` is reserved for positive evidence — a same-origin page we can read
59-
* that is known to publish no verdict. A handle that reports closed is only
60-
* `unobservable`: a provider page with COOP `same-origin` disowns the window,
61-
* and the disowned handle reports `closed` for a consent screen that is still
62-
* running. Treating that as an ending would fail a live flow.
49+
* What the opener can actually prove about a popup it launched. `ended` needs
50+
* positive evidence: a same-origin page we can read that publishes no verdict.
51+
* A handle reporting closed is only `unobservable` — COOP disowns the window
52+
* and the disowned handle reports closed for a consent screen still running.
6353
*/
6454
type PopupObservation = 'live' | 'ended' | 'unobservable'
6555

@@ -208,12 +198,10 @@ export function useOAuthChipConnection({
208198
)
209199
const verdictConnected = connectionStatus === 'connected'
210200
const connected = verdictConnected || connectedFromWorkspaceChange
211-
// The label trusts the return leg's verdict; the lock does not. A failure to
212-
// create the credential from its draft is swallowed server-side, so a flow
213-
// can report success with nothing in the workspace to show for it — locking
214-
// on the verdict alone would strand the row saying "Connected" with no way
215-
// to retry. A reconnect legitimately produces no new credential, so it has
216-
// nothing to corroborate against and keeps trusting the verdict.
201+
// The label trusts the verdict; the lock does not. A swallowed credential-draft
202+
// failure lets a flow report success with nothing in the workspace, and locking
203+
// on that would strand the row saying "Connected" with no way to retry. A
204+
// reconnect produces no new credential, so it has nothing to corroborate against.
217205
const connectedFromAttempt =
218206
verdictConnected && (reconnectCredentialId ? true : connectedFromWorkspaceChange)
219207

@@ -222,15 +210,13 @@ export function useOAuthChipConnection({
222210
}, [onConnected])
223211

224212
/**
225-
* A credential for this row can also appear without the row launching it —
226-
* the integrations page in another tab, or a desktop flow that never comes
227-
* back through the return URL. Diffing the workspace list against the
228-
* baseline captured for this scope surfaces that.
213+
* A credential can appear without this row launching it — the integrations
214+
* page in another tab, or a desktop flow that never returns through the URL.
215+
* Diffing the workspace list against this scope's baseline surfaces that.
229216
*
230-
* This signal is workspace-wide, so it cannot be attributed to one row:
231-
* sibling chips for the same provider all see the same change. It therefore
232-
* only ever *shows* the row as satisfied — {@link connectedFromAttempt} is
233-
* what locks it, so a second same-provider row stays clickable.
217+
* Workspace-wide, so it cannot be attributed to one row: it only ever *shows*
218+
* the row satisfied. {@link connectedFromAttempt} is what locks it, so a
219+
* sibling chip for the same provider stays clickable.
234220
*/
235221
useEffect(() => {
236222
if (!isFetched) return
@@ -268,12 +254,10 @@ export function useOAuthChipConnection({
268254
])
269255

270256
/**
271-
* This row's attempt: the one named by the return URL when we came back from
272-
* the provider, else the last one stored for this exact row. The stored
273-
* lookup is what survives a reload — and what covers the transcript
274-
* rendering only after the return hook has already stripped the URL param.
275-
* Both are scoped to the row, so a sibling chip for the same provider can
276-
* never claim this one's result.
257+
* This row's attempt: the one named by the return URL, else the last one
258+
* stored for this exact row. The stored lookup survives a reload, and covers
259+
* a transcript rendered after the return hook stripped the URL param. Both
260+
* are row-scoped, so a sibling chip can never claim this one's result.
277261
*/
278262
const readRowAttempt = useCallback((): OAuthChatAttempt | null => {
279263
const active = activeAttemptId ? readOAuthChatAttempt(activeAttemptId) : null
@@ -378,20 +362,17 @@ export function useOAuthChipConnection({
378362
}, [settleFromCredentials])
379363

380364
/**
381-
* Watches a live popup for the endings that publish no verdict — a provider
382-
* interstitial exiting to the workspace root, a denied consent landing on the
383-
* OAuth error page, or the user closing the window. None reach the completion
384-
* page or fire an event here, and the user need never leave this tab for a
385-
* focus event either, so polling is the only way the row learns it is over.
365+
* Watches a live popup for the endings that publish no verdict. None fire an
366+
* event here, and the user need never leave this tab for a focus event
367+
* either, so polling is the only way the row learns the flow is over.
386368
*/
387369
useEffect(() => {
388370
if (connectionStatus !== 'pending' || !popupRef.current) return
389371
const poll = window.setInterval(() => {
390372
const observation = observePopup(popupRef.current)
391373
if (observation === 'live') return
392374
// Stop before settling: the refetch leaves the status `pending` for its
393-
// duration, so a running interval would keep firing and a later tick
394-
// could resolve an attempt a retry had since replaced.
375+
// duration, so a later tick could resolve an attempt a retry replaced.
395376
window.clearInterval(poll)
396377
popupRef.current = null
397378
// Only `ended` is proof the flow finished with nothing published. A
@@ -402,15 +383,11 @@ export function useOAuthChipConnection({
402383
}, [connectionStatus, settleFromCredentials])
403384

404385
/**
405-
* Backstop deadline for a pending attempt whose outcome never becomes
406-
* observable — a closed-or-disowned popup this tab cannot read, with no blur
407-
* for the focus verification to work from.
408-
*
409-
* Bounds every pending attempt rather than only one this mount launched: the
410-
* transcript virtualizes, so a row can remount onto an attempt restored from
411-
* storage with no window handle at all. The deadline comes from the attempt's
412-
* own `requestedAt`, so remounting re-arms with the time remaining instead of
413-
* restarting the clock or dropping the bound entirely.
386+
* Backstop for a pending attempt whose outcome never becomes observable.
387+
* Bounds every pending attempt, not just one this mount launched — the
388+
* transcript virtualizes, so a row can remount with no window handle at all.
389+
* Dated from the attempt's `requestedAt`, so a remount inherits the time
390+
* remaining rather than restarting the clock.
414391
*/
415392
useEffect(() => {
416393
if (connectionStatus !== 'pending') return

apps/sim/next.config.ts

Lines changed: 6 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -319,13 +319,9 @@ const nextConfig: NextConfig = {
319319
{
320320
// Exclude Vercel internal resources and static assets from strict COOP, Google Drive Picker
321321
// and the /demo Cal.com booking embed to prevent 'refused to connect' / slow-load issues.
322-
// The OAuth popup pages are excluded because `same-origin` moves a document into its own
323-
// browsing-context group, disowning it from the opener that launched it: the popup stops
324-
// being reliably script-closable and its opener sees `closed` report true for a live window.
325-
// That covers the return leg (`oauth/chat-complete`) and both pages a flow can exit on
326-
// without reaching it — `oauth-error` and the `workspace` root, which the custom-provider
327-
// callbacks bounce to. Bare `workspace` also matches `/workspace` itself, which previously
328-
// fell here while every `/workspace/...` route got the permissive policy below.
322+
// The pages an OAuth popup can land on are excluded too: `same-origin` would disown the
323+
// popup from its opener, leaving it not reliably script-closable and reporting `closed`
324+
// for a live window.
329325
source:
330326
'/((?!_next|_vercel|api|favicon.ico|w/.*|workspace|api/tools/drive|demo|oauth-error|oauth/chat-complete).*)',
331327
headers: [
@@ -350,9 +346,9 @@ const nextConfig: NextConfig = {
350346
],
351347
},
352348
{
353-
// For main app routes, Google Drive Picker, the /demo Cal.com embed, the pages an OAuth
354-
// popup can land on, and Vercel resources - use permissive policies. The popup pages match
355-
// their opener's value so the two stay in one browsing-context group.
349+
// For main app routes, Google Drive Picker, the /demo Cal.com embed, the OAuth popup pages,
350+
// and Vercel resources - use permissive policies. The popup pages match their opener's
351+
// value so the two stay in one browsing-context group.
356352
source:
357353
'/(w/.*|workspace.*|api/tools/drive|demo.*|oauth-error|oauth/chat-complete|_next/.*|_vercel/.*)',
358354
headers: [

0 commit comments

Comments
 (0)