Make OAuth sign-in easy to cancel and retry

Author: RhysSullivan

.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.

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<void>();
+        const started = Promise.withResolvers<string>();
+        const completed = Promise.withResolvers<void>();
+        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);
+    }),
+  ),
+);

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<OAuthClientSlug | null> => {
@@ -410,6 +411,7 @@ describe("runCimdConnect", () => {
 
     const outcome = await runCimdConnect(
       {
+        isActive: () => true,
         reserve: (): OAuthPopupReservation => RESERVED,
         release: (): void => {},
         createClient: (): Promise<OAuthClientSlug | null> => {
@@ -1076,6 +1078,7 @@ describe("runCimdConnect popup reservation", () => {
     const popup = popupSpy();
     const outcome = await runCimdConnect(
       {
+        isActive: () => true,
         ...popup,
         createClient: (args: CimdCreateArgs): Promise<OAuthClientSlug> => {
           popup.calls.push("createClient");
@@ -1096,6 +1099,7 @@ describe("runCimdConnect popup reservation", () => {
     const popup = popupSpy();
     const outcome = await runCimdConnect(
       {
+        isActive: () => true,
         ...popup,
         createClient: (): Promise<OAuthClientSlug | null> => 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<OAuthClientSlug | null>();
+    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<OAuthClientSlug | null> => Promise.resolve(null),
         start: (): void => {},

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<OAuthClientSlug | null>;
   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<OAuthProbeResult | null> => {
             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 ? (
+          <>
+            <DialogHeader>
+              <DialogTitle>Add connection · {integrationName}</DialogTitle>
+            </DialogHeader>
+            <div role="status" className="flex flex-col items-center px-4 py-8 text-center">
+              <div className="mb-5 flex size-12 items-center justify-center rounded-xl border border-border/70 bg-muted/30 text-foreground shadow-sm">
+                {oauthPopup.phase === "authorizing" ? (
+                  <PanelsTopLeftIcon aria-hidden="true" className="size-5" />
+                ) : (
+                  <LoaderCircleIcon
+                    aria-hidden="true"
+                    className="size-5 animate-spin motion-reduce:animate-none"
+                  />
+                )}
+              </div>
+              <p className="text-sm font-medium text-foreground">
+                {oauthPopup.phase === "authorizing"
+                  ? "Continue in the sign-in window"
+                  : oauthPopup.phase === "saving"
+                    ? "Finishing connection"
+                    : "Preparing sign-in"}
+              </p>
+              <DialogDescription className="mt-2 max-w-xs text-sm leading-relaxed">
+                {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."}
+              </DialogDescription>
+            </div>
+            <DialogFooter className="border-t border-border/60 pt-4 sm:justify-between">
+              <Button type="button" variant="ghost" onClick={close}>
+                Close
+              </Button>
+              {oauthPopup.phase !== "saving" ? (
+                <Button type="button" variant="outline" onClick={cancelSignIn}>
+                  Cancel sign-in
+                </Button>
+              ) : null}
+            </DialogFooter>
+          </>
+        ) : addingMethod && createCustomMethod ? (
           <>
             <DialogHeader className="border-b border-border/60 px-5 py-4">
               <DialogTitle className="text-base">Add authentication method</DialogTitle>
@@ -3371,12 +3442,7 @@ function AddAccountModalView(props: AddAccountModalProps) {
               </p>
             ) : null}
             <DialogFooter>
-              <Button
-                type="button"
-                variant="ghost"
-                onClick={close}
-                disabled={submitting || oauthBusy || automaticOAuthConnecting}
-              >
+              <Button type="button" variant="ghost" onClick={close} disabled={submitting || ccBusy}>
                 {isOAuth ? "Close" : "Cancel"}
               </Button>
               {/* Footer action, in precedence order: