fix(mship): return the chat connect flow to the tab that started it - #6403
Merged
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit f41e66c. Configure here. |
Contributor
Greptile SummaryThe PR moves chat credential OAuth into per-attempt popups and routes successful returns through a self-closing completion page, preserving the initiating chat tab and its in-progress state.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/use-oauth-chip-connection.ts | Introduces the popup lifecycle, attempt-correlated completion handling, retry protection, credential corroboration, and bounded settlement without leaving a blocking prior issue outstanding. |
| apps/sim/lib/credentials/oauth-chat-attempt.ts | Adds safe completion-URL construction and makes attempt persistence failures non-throwing so the handoff can still release its window. |
| apps/sim/app/oauth/chat-complete/chat-complete-handoff.tsx | Publishes the callback verdict, closes the OAuth window, and uses a sanitized same-origin fallback when closure is refused. |
| apps/sim/next.config.ts | Extends permissive opener-policy routing to the OAuth completion and error pages needed by the popup lifecycle. |
| apps/sim/app/workspace/[workspaceId]/home/components/message-content/components/special-tags/special-tags.test.tsx | Adds regression coverage for popup ownership, completion, terminal settlement, remount deadlines, retry behavior, and credential-backed locking. |
Sequence Diagram
sequenceDiagram
participant Chat as Chat tab
participant Popup as OAuth popup
participant Provider as OAuth provider
participant Complete as /oauth/chat-complete
Chat->>Chat: Create pending attempt
Chat->>Popup: Open per-attempt authorize URL
Popup->>Provider: Run authorization
Provider-->>Complete: Return with attempt identifier
Complete->>Chat: Publish verdict through shared attempt storage/event
Complete->>Popup: Request window.close()
Chat->>Chat: Refetch credentials and update row
alt Popup cannot close
Complete->>Complete: Redirect to sanitized same-origin returnTo
end
Reviews (8): Last reviewed commit: "chore(mship): tighten the comments on th..." | Re-trigger Greptile
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
6 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
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.
Collaborator
Collaborator
|
@cursor review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
Collaborator
Collaborator
|
@cursor review |
… 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.
… 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.
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.
Collaborator
|
@greptileai review bugbot run |
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.
Collaborator
|
@greptileai review bugbot run |
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.
Collaborator
|
@greptileai review bugbot run |
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.
Collaborator
Collaborator
|
@cursor review |
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.
Collaborator
Collaborator
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit f41e66c. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/oauth/chat-completepage, so the chat tab updates in place instead of the flow landing on a duplicate copy of the app in a new tabrel="noopener") instead of taking the popup path/oauth-erroror the workspace error exit as finished rather than waiting on itDesktop
Unaffected — the desktop app never reaches the popup path.
Desktop can't run OAuth in an embedded window: Better Auth binds the flow's state to the initiating browser's cookies, the Sim browser panel's partition isn't signed in to Sim, and Google/Microsoft reject embedded user agents outright. So the chip hands the whole flow to the system browser through the desktop bridge (
beginOAuthConnect), which opens/desktop/connect, runs the authorize route there, and bounces/desktop/connect/completeto the app's127.0.0.1loopback. The app refocuses itself and refreshes credentials off that push.Why this PR can't touch it:
onConnectClicktakes the bridge branch andreturns beforebuildOAuthChatCompleteAuthorizeUrlorwindow.openis ever called, sopopupRefis never set on desktop. Every guard added here keys off that ref —isPopupStillOpen(null)exits on its first line — which makes the terminal-path check and the per-attempt popup naming unreachable. Completion still arrives throughuseDesktopOAuthConnectListenerinuse-oauth-return.ts, untouched by this PR, and desktop keeps its own attempt correlation viasetActiveDesktopOAuthChatAttempt/resolveDesktopOAuthChatAttempt.Type of Change
Testing
Tested manually.
bun run lint,check:audits(22 audits),check:api-validation, and type-check all pass. 173 test files / 1800 tests green, including new coverage for the close-refused redirect and its cross-originreturnTorejection, the per-attempt popup name, the terminal-page settle, and the retryable-lock behavior.Not verified in a real browser: whether
COOP: same-originon/oauth/chat-completeever refuseswindow.close(). It closed fine in manual testing; the 400ms redirect is the fallback if it doesn't.Checklist