Skip to content

Commit 656c7ba

Browse files
committed
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.
1 parent 86b57e3 commit 656c7ba

3 files changed

Lines changed: 52 additions & 19 deletions

File tree

apps/sim/app/oauth/chat-complete/chat-complete-handoff.tsx

Lines changed: 11 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -26,24 +26,19 @@ function sanitizeReturnTo(raw: string | null): string | null {
2626
/**
2727
* Behavior half of the chat OAuth return leg: publishes the verdict to the
2828
* 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 is
30-
* plain server-rendered markup that paints before this hydrates.
29+
* listener — then closes the window. Renders nothing, so the page's frame
30+
* paints as server markup before this hydrates.
3131
*
32-
* Reaching this page IS the verdict. Better Auth routes a flow here only as
33-
* its success `callbackURL`, sending failures to `onAPIError.errorURL`
34-
* (`/oauth-error`) or back here with an `error` code, so the server has
35-
* already decided by the time this runs. That is a strictly better signal than
36-
* the credential diffing the generic-page return does: re-authorizing an
37-
* already-linked account updates the account row instead of creating one, so
38-
* no new credential appears and a diff would call a perfectly good connect a
39-
* failure.
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.
4037
*
41-
* A window the browser refuses to close redirects on to the chat surface
42-
* instead. That is the popup-blocked path: the anchor's `target='_blank'`
43-
* opens this leg in a new tab, which no script may close. That URL
44-
* deliberately carries no attempt id — the verdict is already published, and
45-
* the destination's return router would otherwise re-decide it by the very
46-
* diff this page exists to avoid.
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.
4742
*/
4843
export function ChatCompleteHandoff() {
4944
const ranRef = useRef(false)

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

Lines changed: 41 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -582,6 +582,47 @@ describe('CredentialDisplay link tag', () => {
582582
act(() => root.unmount())
583583
})
584584

585+
it('still announces the connection when the watcher released the popup first', async () => {
586+
// The watcher drops the window handle as soon as it stops being observable,
587+
// which routinely happens before React applies the storage-driven verdict.
588+
// The announcement has to survive that, so it cannot be gated on the handle.
589+
vi.useFakeTimers()
590+
const toastSuccess = vi.spyOn(toast, 'success').mockImplementation(() => '')
591+
const popup = { focus: vi.fn(), closed: false }
592+
const openSpy = vi
593+
.spyOn(window, 'open')
594+
.mockReturnValue(popup as unknown as ReturnType<typeof window.open>)
595+
const { container, root } = renderCredentialLink({
596+
type: 'link',
597+
provider: 'google-email',
598+
value:
599+
'https://sim.test/api/auth/oauth2/authorize?providerId=google-email&callbackURL=https%3A%2F%2Fsim.test%2Fworkspace%2Fworkspace-1%2Fchat%2Fchat-1',
600+
})
601+
602+
await act(async () => {
603+
container
604+
.querySelector('a')
605+
?.dispatchEvent(new MouseEvent('click', { bubbles: true, cancelable: true }))
606+
})
607+
const attemptId = new URL(
608+
new URL(openSpy.mock.calls[0][0] as string).searchParams.get('callbackURL') ?? ''
609+
).searchParams.get('oauthAttempt') as string
610+
611+
popup.closed = true
612+
await act(async () => {
613+
await vi.advanceTimersByTimeAsync(2000)
614+
})
615+
await act(async () => {
616+
setOAuthChatAttemptStatus(attemptId, 'connected')
617+
})
618+
619+
expect(toastSuccess).toHaveBeenCalledWith('Gmail connected successfully.')
620+
vi.useRealTimers()
621+
openSpy.mockRestore()
622+
toastSuccess.mockRestore()
623+
act(() => root.unmount())
624+
})
625+
585626
it('focuses the live popup instead of starting a rival attempt on a repeat click', async () => {
586627
const toastSuccess = vi.spyOn(toast, 'success').mockImplementation(() => '')
587628
const popup = {

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

Lines changed: 0 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -406,9 +406,6 @@ export function useOAuthChipConnection({
406406
const launchedAttemptId = launchedAttemptIdRef.current
407407
if (connectionStatus !== 'connected' || !launchedAttemptId) return
408408
const attempt = readOAuthChatAttempt(launchedAttemptId)
409-
// Tracked separately from `popupRef`, which the watcher clears the moment
410-
// the window looks finished — often before React has applied the
411-
// storage-driven verdict, which would swallow the announcement.
412409
launchedAttemptIdRef.current = null
413410
popupRef.current = null
414411
// The verdict settles the label, but the lock also waits on the credential

0 commit comments

Comments
 (0)