From 1b19b374f3ca4abe958bbb3334efcb3a79fbc81a Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 18 Sep 2026 11:13:09 -0700 Subject: [PATCH 1/2] Make OAuth sign-in easy to cancel and retry --- .changeset/connection-oauth-cancel.md | 5 + e2e/scenarios/connection-setup-ux.test.ts | 128 ++++++++++++++++++ .../src/components/add-account-modal.test.ts | 26 ++++ .../src/components/add-account-modal.tsx | 100 +++++++++++--- packages/react/src/plugins/oauth-sign-in.tsx | 120 ++++++++++------ 5 files changed, 321 insertions(+), 58 deletions(-) create mode 100644 .changeset/connection-oauth-cancel.md diff --git a/.changeset/connection-oauth-cancel.md b/.changeset/connection-oauth-cancel.md new file mode 100644 index 0000000000..631cd2b3d7 --- /dev/null +++ b/.changeset/connection-oauth-cancel.md @@ -0,0 +1,5 @@ +--- +"executor": patch +--- + +Show a clear, cancellable sign-in state and prevent cancelled OAuth requests from disrupting a retry. diff --git a/e2e/scenarios/connection-setup-ux.test.ts b/e2e/scenarios/connection-setup-ux.test.ts index fa87ea93d6..c11ea0e757 100644 --- a/e2e/scenarios/connection-setup-ux.test.ts +++ b/e2e/scenarios/connection-setup-ux.test.ts @@ -288,3 +288,131 @@ scenario( }), ), ); + +scenario( + "Connection setup · an interrupted OAuth popup can be cancelled and retried", + {}, + Effect.scoped( + Effect.gen(function* () { + const { browser, identity, slug, client, emulator } = yield* fixture; + yield* browser.session(identity, async ({ page, step }) => { + await step("Start provider sign-in without entering a name", async () => { + await visit(page, `/integrations/${slug}?addAccount=1`); + await page.getByRole("tab", { name: "OAuth2", exact: true }).click(); + const opened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const popup = await opened; + await popup.waitForURL(/oauth\/v2\/authorize/); + await popup.close(); + }); + await step("Cancel the waiting sign-in and retry", async () => { + const cancel = page.getByRole("button", { name: "Cancel sign-in", exact: true }); + expect( + await cancel.isVisible(), + "an interrupted provider window must not strand Connecting forever", + ).toBe(true); + expect(await cancel.isEnabled()).toBe(true); + await page.getByText("Continue in the sign-in window", { exact: true }).waitFor(); + expect( + await page.getByRole("button", { name: "Connecting…", exact: true }).count(), + "waiting for provider consent must not leave a dead Connecting button", + ).toBe(0); + await cancel.click(); + expect( + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).isEnabled(), + ).toBe(true); + const opened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const popup = await opened; + await popup.waitForURL(/oauth\/v2\/authorize/); + await popup.route("https://emulators.dev/oauth/v2/authorize/callback", (route) => + route.continue({ url: `${emulator.baseUrl}/oauth/v2/authorize/callback` }), + ); + await popup.getByRole("button", { name: /admin/ }).click(); + await page.getByRole("dialog").waitFor({ state: "hidden", timeout: 30_000 }); + }); + }); + const connections = yield* client.connections.list({ query: { integration: slug } }); + expect(connections, "retry finishes and persists exactly one connection").toHaveLength(1); + }), + ), +); + +scenario( + "Connection setup · cancelling a pending OAuth start ignores its late response", + {}, + Effect.scoped( + Effect.gen(function* () { + const { browser, identity, slug, client, emulator } = yield* fixture; + yield* browser.session(identity, async ({ page, step }) => { + const released = Promise.withResolvers(); + const started = Promise.withResolvers(); + const completed = Promise.withResolvers(); + await page.route( + "**/api/oauth/start", + async (route) => { + const response = await route.fetch(); + const body: unknown = await response.json(); + if ( + typeof body !== "object" || + body === null || + !("state" in body) || + typeof body.state !== "string" + ) { + expect.fail("the held start response must contain its OAuth session state"); + } + started.resolve(body.state); + await released.promise; + await route.fulfill({ response }); + completed.resolve(); + }, + { times: 1 }, + ); + try { + await step("Cancel while the authorization response is still in flight", async () => { + await visit(page, `/integrations/${slug}?addAccount=1`); + const opened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const popup = await opened; + const oldState = await started.promise; + await page.getByRole("button", { name: "Cancel sign-in", exact: true }).click(); + await expect.poll(() => popup.isClosed()).toBe(true); + const cancelled = page.waitForResponse((response) => + response.url().endsWith("/api/oauth/cancel"), + ); + const retryOpened = page.waitForEvent("popup"); + await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); + const retryPopup = await retryOpened; + await retryPopup.waitForURL(/oauth\/v2\/authorize/); + released.resolve(); + await completed.promise; + const cancellation = await cancelled; + expect( + cancellation.request().postDataJSON(), + "only the old session is cancelled", + ).toEqual({ state: oldState }); + expect( + retryPopup.isClosed(), + "the old response cannot close the new sign-in window", + ).toBe(false); + expect( + page.context().pages(), + "only the replacement sign-in window remains", + ).toHaveLength(2); + await page.getByText("Continue in the sign-in window", { exact: true }).waitFor(); + await retryPopup.route("https://emulators.dev/oauth/v2/authorize/callback", (route) => + route.continue({ url: `${emulator.baseUrl}/oauth/v2/authorize/callback` }), + ); + await retryPopup.getByRole("button", { name: /admin/ }).click(); + await page.getByRole("dialog").waitFor({ state: "hidden", timeout: 30_000 }); + }); + } finally { + released.resolve(); + await page.unrouteAll({ behavior: "wait" }); + } + }); + const connections = yield* client.connections.list({ query: { integration: slug } }); + expect(connections, "the newer attempt survives and completes consent").toHaveLength(1); + }), + ), +); diff --git a/packages/react/src/components/add-account-modal.test.ts b/packages/react/src/components/add-account-modal.test.ts index 90558590e6..8be53d3d2c 100644 --- a/packages/react/src/components/add-account-modal.test.ts +++ b/packages/react/src/components/add-account-modal.test.ts @@ -366,6 +366,7 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + isActive: () => true, reserve: (): OAuthPopupReservation => RESERVED, release: (): void => {}, createClient: (args: CimdCreateArgs): Promise => { @@ -410,6 +411,7 @@ describe("runCimdConnect", () => { const outcome = await runCimdConnect( { + isActive: () => true, reserve: (): OAuthPopupReservation => RESERVED, release: (): void => {}, createClient: (): Promise => { @@ -1076,6 +1078,7 @@ describe("runCimdConnect popup reservation", () => { const popup = popupSpy(); const outcome = await runCimdConnect( { + isActive: () => true, ...popup, createClient: (args: CimdCreateArgs): Promise => { popup.calls.push("createClient"); @@ -1096,6 +1099,7 @@ describe("runCimdConnect popup reservation", () => { const popup = popupSpy(); const outcome = await runCimdConnect( { + isActive: () => true, ...popup, createClient: (): Promise => Promise.resolve(null), start: (): void => {}, @@ -1107,10 +1111,32 @@ describe("runCimdConnect popup reservation", () => { expect(popup.calls).toEqual(["reserve", "release"]); }); + it("does not start sign-in after cancellation during client setup", async () => { + const created = Promise.withResolvers(); + const popup = popupSpy(); + let active = true; + const connecting = runCimdConnect( + { + ...popup, + isActive: () => active, + createClient: () => created.promise, + start: () => { + popup.calls.push("start"); + }, + }, + cimdInput, + ); + active = false; + created.resolve(OAuthClientSlug.make("cancelled-client")); + expect(await connecting).toEqual({ kind: "aborted" }); + expect(popup.calls).toEqual(["reserve", "release"]); + }); + it("never claims a window when the method is missing its endpoints", async () => { const popup = popupSpy(); const outcome = await runCimdConnect( { + isActive: () => true, ...popup, createClient: (): Promise => Promise.resolve(null), start: (): void => {}, diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 222dd0f682..20fb43df8c 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -100,7 +100,15 @@ import { DropdownMenuItem, DropdownMenuTrigger, } from "./dropdown-menu"; -import { ChevronDown, EyeIcon, EyeOffIcon, PlusIcon, XIcon } from "lucide-react"; +import { + ChevronDown, + EyeIcon, + EyeOffIcon, + LoaderCircleIcon, + PanelsTopLeftIcon, + PlusIcon, + XIcon, +} from "lucide-react"; import { Dialog, DialogContent, @@ -741,6 +749,7 @@ type CimdCreateClientArgs = { }; type RunCimdConnectDeps = { + readonly isActive: () => boolean; readonly createClient: (args: CimdCreateClientArgs) => Promise; readonly start: (args: CimdStartArgs) => void; /** Claim the sign-in window before any await. See `useOAuthPopupFlow.reserve`. */ @@ -767,6 +776,7 @@ type RunCimdConnectInput = { type CimdOutcome = | { readonly kind: "started"; readonly client: OAuthClientSlug; readonly reused: boolean } + | { readonly kind: "aborted" } | { readonly kind: "popup-blocked" } | { readonly kind: "failed"; readonly reason: "missing-endpoints" | "create-failed" }; @@ -830,6 +840,10 @@ export async function runCimdConnect( if (reservation.kind === "blocked") return { kind: "popup-blocked" }; const resolved = await resolveCimdClient(deps, input); + if (!deps.isActive()) { + deps.release(); + return { kind: "aborted" }; + } if (resolved.kind === "failed") { deps.release(); return resolved; @@ -1866,7 +1880,14 @@ function AddAccountModalView(props: AddAccountModalProps) { const oauthBusy = ccBusy || oauthPopup.busy; const cimdConnecting = cimdBusy || oauthPopup.busy; const dcrConnecting = dcrBusy || oauthPopup.busy; - const automaticOAuthConnecting = cimdConnecting || dcrConnecting; + const signInPending = cimdBusy || dcrBusy || oauthPopup.busy; + const automaticAttemptRef = useRef(0); + const cancelSignIn = () => { + automaticAttemptRef.current += 1; + oauthPopup.cancel(); + setCimdBusy(false); + setDcrBusy(false); + }; // "Connection saved to" for a PICKED BYO OAuth app. Cloud: a Workspace (`org`) // app can mint Personal or Workspace connections; a Personal (`user`) app can @@ -2359,11 +2380,16 @@ function AddAccountModalView(props: AddAccountModalProps) { const cimdOwner = owner; const connectionName = previewConnectionName(label, cimdOwner); const identityLabel = typedIdentityLabel(label); + const attempt = ++automaticAttemptRef.current; + const isActive = () => viewMountedRef.current && attempt === automaticAttemptRef.current; setCimdBusy(true); const outcome = await runCimdConnect( { reserve: oauthPopup.reserve, - release: oauthPopup.releaseReservation, + release: () => { + if (attempt === automaticAttemptRef.current) oauthPopup.releaseReservation(); + }, + isActive, createClient: createCimdClient, start: (args: CimdStartArgs): void => { void oauthPopup.start({ @@ -2398,6 +2424,7 @@ function AddAccountModalView(props: AddAccountModalProps) { existingClients: clientSummaries, }, ); + if (!isActive()) return; setCimdBusy(false); trackEvent("connection_oauth_started", { integration_slug: String(integration), @@ -2442,16 +2469,18 @@ function AddAccountModalView(props: AddAccountModalProps) { setDcrFailed(true); return; } + const attempt = ++automaticAttemptRef.current; + const isActive = () => viewMountedRef.current && attempt === automaticAttemptRef.current; setDcrBusy(true); const outcome = await runAutomaticOAuthConnect( { reserve: oauthPopup.reserve, - release: oauthPopup.releaseReservation, - // Closing the modal genuinely unmounts this view (see - // `AddAccountModal`), so "still mounted" is exactly "still open". - // The sequence checks it between round trips: a close mid-flight - // must not register a client or launch the popup afterwards. - isActive: () => viewMountedRef.current, + release: () => { + if (attempt === automaticAttemptRef.current) oauthPopup.releaseReservation(); + }, + // A closed modal or cancelled attempt cannot register a client or + // launch sign-in after the next round trip. A retry owns a new attempt. + isActive, probe: async (url: string): Promise => { const exit = await doProbe({ payload: { url }, reactivityKeys: [] }); if (Exit.isFailure(exit)) return null; @@ -2560,7 +2589,7 @@ function AddAccountModalView(props: AddAccountModalProps) { // The modal closed mid-flight: this view is unmounted, so no state may // be written at all — not the fallback below, and not even the busy // flag, which belongs to the surface that is gone. - if (outcome.kind === "aborted") return; + if (!isActive() || outcome.kind === "aborted") return; setDcrBusy(false); // `connection_oauth_started` measures the connect funnel; a reconnect // reports through `connection_reconnected` on the popup callbacks above, @@ -2720,7 +2749,49 @@ function AddAccountModalView(props: AddAccountModalProps) { : "sm:max-w-xl", )} > - {addingMethod && createCustomMethod ? ( + {signInPending ? ( + <> + + Add connection · {integrationName} + +
+
+ {oauthPopup.phase === "authorizing" ? ( +
+

+ {oauthPopup.phase === "authorizing" + ? "Continue in the sign-in window" + : oauthPopup.phase === "saving" + ? "Finishing connection" + : "Preparing sign-in"} +

+ + {oauthPopup.phase === "authorizing" + ? "If the window closed or sign-in stalled, cancel and try again." + : oauthPopup.phase === "saving" + ? "Sign-in complete. Updating your connections." + : "The sign-in window will be ready shortly. You can cancel at any time."} + +
+ + + {oauthPopup.phase !== "saving" ? ( + + ) : null} + + + ) : addingMethod && createCustomMethod ? ( <> Add authentication method @@ -3371,12 +3442,7 @@ function AddAccountModalView(props: AddAccountModalProps) {

) : null} - {/* Footer action, in precedence order: diff --git a/packages/react/src/plugins/oauth-sign-in.tsx b/packages/react/src/plugins/oauth-sign-in.tsx index f46d6378e5..2197f06a2f 100644 --- a/packages/react/src/plugins/oauth-sign-in.tsx +++ b/packages/react/src/plugins/oauth-sign-in.tsx @@ -1,10 +1,12 @@ -import { useCallback, useEffect, useRef, useState } from "react"; -import { useAtomSet } from "@effect/atom-react"; +import { useCallback, useContext, useEffect, useRef, useState } from "react"; +import { RegistryContext, useAtomSet } from "@effect/atom-react"; import * as Data from "effect/Data"; import * as Effect from "effect/Effect"; import * as Exit from "effect/Exit"; +import * as AtomRegistry from "effect/unstable/reactivity/AtomRegistry"; -import { cancelOAuth, oauthConnectionCompleted, startOAuth } from "../api/atoms"; +import { oauthConnectionCompleted } from "../api/atoms"; +import { ExecutorApiClient } from "../api/client"; import { trackEvent } from "../api/analytics"; import { messageFromExit, messageFromUnknown, useReportHandledError } from "../api/error-reporting"; import { @@ -186,12 +188,11 @@ export function useOAuthPopupFlow< popupName, startErrorMessage, } = options; - const doStartOAuth = useAtomSet(startOAuth, { mode: "promiseExit" }); - const doCancelOAuth = useAtomSet(cancelOAuth, { mode: "promiseExit" }); + const registry = useContext(RegistryContext); const doOAuthConnectionCompleted = useAtomSet(oauthConnectionCompleted, { mode: "promiseExit" }); const reportHandledError = useReportHandledError(); const blockedMessage = popupBlockedMessage ?? POPUP_BLOCKED_MESSAGE; - const [busy, setBusy] = useState(false); + const [phase, setPhase] = useState<"idle" | "preparing" | "authorizing" | "saving">("idle"); const [error, setError] = useState(null); const cleanupRef = useRef<(() => void) | null>(null); const sessionRef = useRef<{ readonly state: string } | null>(null); @@ -200,6 +201,7 @@ export function useOAuthPopupFlow< // exists. Hold it here so cancel and unmount can close it, or abandoning a // connect mid-probe strands a blank popup on screen. const reservationRef = useRef(null); + const generationRef = useRef(0); const releaseReservation = useCallback(() => { reservationRef.current?.popup.close(); @@ -208,23 +210,33 @@ export function useOAuthPopupFlow< const cancelSession = useCallback( (state: string) => { - void doCancelOAuth({ payload: { state: OAuthState.make(state) } }); + // Each cancellation owns its request; cancelling an older attempt must + // not interrupt cleanup of another session. + const request = ExecutorApiClient.runtime.atom( + ExecutorApiClient.use((client) => + client.oauth.cancel({ payload: { state: OAuthState.make(state) } }), + ), + ); + void Effect.runPromiseExit(AtomRegistry.getResult(registry, request)); }, - [doCancelOAuth], + [registry], ); const cancel = useCallback(() => { + generationRef.current += 1; + setError(null); const session = sessionRef.current; cleanupRef.current?.(); cleanupRef.current = null; sessionRef.current = null; releaseReservation(); if (session) cancelSession(session.state); - setBusy(false); + setPhase("idle"); }, [cancelSession, releaseReservation]); useEffect( () => () => { + generationRef.current += 1; const session = sessionRef.current; cleanupRef.current?.(); cleanupRef.current = null; @@ -267,21 +279,20 @@ export function useOAuthPopupFlow< // `reserve`; cancelling again here would close the window it reserved. const reservation = input.reservation ?? reserve(); if (reservation.kind === "blocked") { - setBusy(false); + setPhase("idle"); setError(blockedMessage); input.onError?.(blockedMessage); return; } - setBusy(true); + setPhase("preparing"); setError(null); // Desktop hosts open the auth URL in the user's real browser, so they // reserve no in-page window and rely on the polling channel for the // result. const desktopBridge = reservation.kind === "desktop" ? reservation.bridge : null; const reservedPopup = reservation.kind === "window" ? reservation.popup : null; - // The window's lifetime now belongs to this flow's teardown, which closes - // it on every failure path below. - reservationRef.current = null; + // Cancel/unmount owns this window even while the start request is pending. + const generation = generationRef.current; const startExit = await Effect.runPromiseExit( Effect.tryPromise({ try: input.run, @@ -292,6 +303,14 @@ export function useOAuthPopupFlow< }), }), ); + if (generation !== generationRef.current) { + reservedPopup?.popup.close(); + if (Exit.isSuccess(startExit) && startExit.value.authorizationUrl !== null) { + cancelSession(startExit.value.state); + } + return; + } + reservationRef.current = null; if (Exit.isFailure(startExit)) { const message = messageFromExit(startExit, startErrorMessage ?? "Failed to start sign-in"); reportHandledError(startExit.cause, { @@ -301,7 +320,7 @@ export function useOAuthPopupFlow< metadata: input.reportMetadata, }); reservedPopup?.popup.close(); - setBusy(false); + setPhase("idle"); setError(message); input.onError?.(message); return; @@ -311,29 +330,33 @@ export function useOAuthPopupFlow< const message = noAuthorizationUrlMessage ?? "OAuth start did not produce an authorization URL"; reservedPopup?.popup.close(); - setBusy(false); + setPhase("idle"); setError(message); input.onError?.(message); return; } + setPhase("authorizing"); sessionRef.current = { state: response.state }; input.onAuthorizationStarted?.(response); const handleResult = async (result: OAuthPopupResult) => { + if (generation !== generationRef.current) return; cleanupRef.current = null; sessionRef.current = null; if (!result.ok) { trackEvent("oauth_completed", { success: false }); - setBusy(false); + setPhase("idle"); setError(result.error); input.onError?.(result.error, result.errorDetails); return; } + setPhase("saving"); const refreshExit = await doOAuthConnectionCompleted({ reactivityKeys: connectionWriteKeys, }); + if (generation !== generationRef.current) return; if (Exit.isFailure(refreshExit)) { const message = messageFromExit(refreshExit, "Failed to refresh connection"); reportHandledError(refreshExit.cause, { @@ -343,7 +366,7 @@ export function useOAuthPopupFlow< metadata: input.reportMetadata, }); trackEvent("oauth_completed", { success: false }); - setBusy(false); + setPhase("idle"); setError(message); input.onError?.(message); return; @@ -362,15 +385,18 @@ export function useOAuthPopupFlow< metadata: input.reportMetadata, }); trackEvent("oauth_completed", { success: false }); - setBusy(false); - setError(message); - input.onError?.(message); + if (generation === generationRef.current) { + setPhase("idle"); + setError(message); + input.onError?.(message); + } return; } trackEvent("oauth_completed", { success: true }); - setBusy(false); + if (generation === generationRef.current) setPhase("idle"); }; const handleClosed = () => { + if (generation !== generationRef.current) return; cleanupRef.current = null; sessionRef.current = null; // `popup.closed` is advisory: COOP redirects can make a live popup @@ -379,16 +405,17 @@ export function useOAuthPopupFlow< const message = popupClosedMessage ?? "Sign-in cancelled - popup was closed before completing the flow."; trackEvent("oauth_completed", { success: false }); - setBusy(false); + setPhase("idle"); setError(message); input.onError?.(message); }; const handleOpenFailed = () => { + if (generation !== generationRef.current) return; cleanupRef.current = null; sessionRef.current = null; cancelSession(response.state); trackEvent("oauth_completed", { success: false }); - setBusy(false); + setPhase("idle"); setError(blockedMessage); input.onError?.(blockedMessage); }; @@ -442,20 +469,29 @@ export function useOAuthPopupFlow< name: String(input.payload.name), owner: input.payload.owner, }, - run: () => - doStartOAuth({ - payload: { - client: input.payload.client, - clientOwner: input.payload.clientOwner, - owner: input.payload.owner, - name: input.payload.name, - integration: input.payload.integration, - template: input.payload.template, - identityLabel: input.payload.identityLabel, - newConnection: input.payload.newConnection, - redirectUri: input.payload.redirectUri ?? oauthCallbackUrl(callbackPath), - }, - }).then((exit) => + run: () => { + // A shared mutation atom returns its latest result to every waiter. + // A fast retry could give the cancelled attempt the NEW session's + // state and make it cancel that session. Keep one atom per attempt; + // its subscription lives until this request settles, even on cancel. + const request = ExecutorApiClient.runtime.atom( + ExecutorApiClient.use((client) => + client.oauth.start({ + payload: { + client: input.payload.client, + clientOwner: input.payload.clientOwner, + owner: input.payload.owner, + name: input.payload.name, + integration: input.payload.integration, + template: input.payload.template, + identityLabel: input.payload.identityLabel, + newConnection: input.payload.newConnection, + redirectUri: input.payload.redirectUri ?? oauthCallbackUrl(callbackPath), + }, + }), + ), + ); + return Effect.runPromiseExit(AtomRegistry.getResult(registry, request)).then((exit) => Exit.isSuccess(exit) ? // The redirect branch carries `authorizationUrl` + `state`; the // inline "connected" (client_credentials) branch has no URL to @@ -469,14 +505,16 @@ export function useOAuthPopupFlow< message: messageFromExit(exit, startErrorMessage ?? "Failed to start sign-in"), }), ), - ), + ); + }, }); }, - [callbackPath, doStartOAuth, openAuthorization, startErrorMessage], + [callbackPath, registry, openAuthorization, startErrorMessage], ); return { - busy, + busy: phase !== "idle", + phase, error, setError, start, From 2fbc1fb93db77835c757f69c175e84e9ad112aa9 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan <39114868+RhysSullivan@users.noreply.github.com> Date: Fri, 18 Sep 2026 14:46:24 -0700 Subject: [PATCH 2/2] Keep the connection form visible during sign-in --- .changeset/connection-oauth-cancel.md | 2 +- e2e/scenarios/connection-setup-ux.test.ts | 22 ++++++ .../src/components/add-account-modal.tsx | 76 ++++++------------- 3 files changed, 46 insertions(+), 54 deletions(-) diff --git a/.changeset/connection-oauth-cancel.md b/.changeset/connection-oauth-cancel.md index 631cd2b3d7..59028ef5a1 100644 --- a/.changeset/connection-oauth-cancel.md +++ b/.changeset/connection-oauth-cancel.md @@ -2,4 +2,4 @@ "executor": patch --- -Show a clear, cancellable sign-in state and prevent cancelled OAuth requests from disrupting a retry. +Let users cancel sign-in from the connection form and prevent cancelled OAuth requests from disrupting a retry. diff --git a/e2e/scenarios/connection-setup-ux.test.ts b/e2e/scenarios/connection-setup-ux.test.ts index c11ea0e757..4900b302db 100644 --- a/e2e/scenarios/connection-setup-ux.test.ts +++ b/e2e/scenarios/connection-setup-ux.test.ts @@ -122,6 +122,11 @@ scenario( await step("Sign in with a provider account without naming the connection", async () => { await visit(page, `/integrations/${slug}?addAccount=1`); await page.getByRole("tab", { name: "OAuth2", exact: true }).click(); + // Attached previews can contain other compatible apps. Use this + // scenario's provider instance for the full callback and token exchange. + await page + .getByRole("radio", { name: new RegExp(slug.replace("setup-", ""), "i") }) + .check(); const opened = page.waitForEvent("popup"); await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); const popup = await opened; @@ -299,6 +304,11 @@ scenario( await step("Start provider sign-in without entering a name", async () => { await visit(page, `/integrations/${slug}?addAccount=1`); await page.getByRole("tab", { name: "OAuth2", exact: true }).click(); + // Attached previews can contain other compatible apps. Use this + // scenario's provider instance for the full callback and token exchange. + await page + .getByRole("radio", { name: new RegExp(slug.replace("setup-", ""), "i") }) + .check(); const opened = page.waitForEvent("popup"); await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); const popup = await opened; @@ -313,6 +323,11 @@ scenario( ).toBe(true); expect(await cancel.isEnabled()).toBe(true); await page.getByText("Continue in the sign-in window", { exact: true }).waitFor(); + expect( + await page.getByRole("tab", { name: "OAuth2", exact: true }).isVisible(), + "the authentication form stays visible during sign-in", + ).toBe(true); + expect(await page.getByRole("textbox", { name: /Display name/ }).isVisible()).toBe(true); expect( await page.getByRole("button", { name: "Connecting…", exact: true }).count(), "waiting for provider consent must not leave a dead Connecting button", @@ -371,10 +386,17 @@ scenario( try { await step("Cancel while the authorization response is still in flight", async () => { await visit(page, `/integrations/${slug}?addAccount=1`); + await page + .getByRole("radio", { name: new RegExp(slug.replace("setup-", ""), "i") }) + .check(); const opened = page.waitForEvent("popup"); await page.getByRole("button", { name: "Connect with OAuth", exact: true }).click(); const popup = await opened; const oldState = await started.promise; + expect( + await page.getByRole("textbox", { name: /Display name/ }).isVisible(), + "the connection form stays visible while sign-in is being prepared", + ).toBe(true); await page.getByRole("button", { name: "Cancel sign-in", exact: true }).click(); await expect.poll(() => popup.isClosed()).toBe(true); const cancelled = page.waitForResponse((response) => diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx index 20fb43df8c..9f93803071 100644 --- a/packages/react/src/components/add-account-modal.tsx +++ b/packages/react/src/components/add-account-modal.tsx @@ -100,15 +100,7 @@ import { DropdownMenuItem, DropdownMenuTrigger, } from "./dropdown-menu"; -import { - ChevronDown, - EyeIcon, - EyeOffIcon, - LoaderCircleIcon, - PanelsTopLeftIcon, - PlusIcon, - XIcon, -} from "lucide-react"; +import { ChevronDown, EyeIcon, EyeOffIcon, PlusIcon, XIcon } from "lucide-react"; import { Dialog, DialogContent, @@ -2749,49 +2741,7 @@ function AddAccountModalView(props: AddAccountModalProps) { : "sm:max-w-xl", )} > - {signInPending ? ( - <> - - Add connection · {integrationName} - -
-
- {oauthPopup.phase === "authorizing" ? ( -
-

- {oauthPopup.phase === "authorizing" - ? "Continue in the sign-in window" - : oauthPopup.phase === "saving" - ? "Finishing connection" - : "Preparing sign-in"} -

- - {oauthPopup.phase === "authorizing" - ? "If the window closed or sign-in stalled, cancel and try again." - : oauthPopup.phase === "saving" - ? "Sign-in complete. Updating your connections." - : "The sign-in window will be ready shortly. You can cancel at any time."} - -
- - - {oauthPopup.phase !== "saving" ? ( - - ) : null} - - - ) : addingMethod && createCustomMethod ? ( + {addingMethod && createCustomMethod ? ( <> Add authentication method @@ -3442,10 +3392,20 @@ function AddAccountModalView(props: AddAccountModalProps) {

) : null} + {signInPending ? ( +

+ {oauthPopup.phase === "authorizing" + ? "Continue in the sign-in window" + : oauthPopup.phase === "saving" + ? "Finishing connection…" + : "Preparing sign-in…"} +

+ ) : null} {/* Footer action, in precedence order: + - pending sign-in: cancel, or wait for an authorized connection to save; - transparent CIMD (no app registration): create/reuse a public metadata-document client and start OAuth; - transparent DCR (no picker): a single Connect that runs @@ -3453,7 +3413,17 @@ function AddAccountModalView(props: AddAccountModalProps) { - registering a BYO app: the form owns its own submit, no footer; - picked BYO OAuth app: Connect with OAuth / Connect (client creds); - credential/no-auth method: Add connection. */} - {cimdActive ? ( + {signInPending ? ( + oauthPopup.phase === "saving" ? ( + + ) : ( + + ) + ) : cimdActive ? (