UsefulSoftwareCo · RhysSullivan · Jun 25, 2026 · Jun 25, 2026
diff --git a/.claude/skills/self-contained-modals/SKILL.md b/.claude/skills/self-contained-modals/SKILL.md
@@ -0,0 +1,77 @@
+---
+name: self-contained-modals
+description: "Build modals/dialogs self-contained: form and in-flight state lives inside, closing unmounts it. Use when writing or reviewing a modal/dialog, especially one that owns async work (OAuth popups, timers, subscriptions, AbortControllers). Catches the stuck-on-Connecting class of lifecycle-leak bugs."
+---
+
+# Self-contained modals
+
+A modal's **form state and in-flight work live INSIDE the modal**, and **closing
+the modal UNMOUNTS that state** so it is destroyed, not hand-reset. Only the
+**open/route intent** belongs to the parent (deep links, programmatic open,
+reconnect handoffs need it).
+
+## Why
+
+A hand-written `reset()` has to enumerate every field, and it silently drifts out
+of sync with state owned by **child hooks the parent can't see**. Unmounting
+resets everything for free, including child-hook cleanup effects (cancelling a
+dangling server session, clearing a timer, aborting a fetch).
+
+Concrete bug this prevents (executor, add-account-modal): the modal was mounted
+unconditionally, so `useOAuthPopupFlow`'s `busy` survived close. `reset()` zeroed
+its own booleans but never called `oauthPopup.cancel()`. Abandon the OAuth popup,
+close, reopen, and `oauthBusy = false || busy(true) = true`, so the footer is
+wedged on "Connecting…" with Close disabled. Unmounting would have cleared `busy`
+AND run the hook's cleanup that cancels the server OAuth session.
+
+## How to apply
+
+Default to **genuine conditional unmount**, state inside. When closed the
+component returns `null`, so React destroys all of it and runs every child
+hook's cleanup. This is the cleanest fix and the one to reach for first:
+
+```tsx
+function Parent() {
+  const [open, setOpen] = useState(false);
+  return open ? <Modal onClose={() => setOpen(false)} /> : null;
+}
+```
+
+A key bump (`<Body key={openCount} />`) is **still a manual reset**, just
+spelled as a remount. Prefer real unmount; only reach for keyed remount in the
+one case below.
+
+That case: **Radix Dialog** (this repo's `components/dialog.tsx`) Content/Overlay
+use `data-[state=closed]:animate-out` exit animations, so unmounting the whole
+`Dialog` drops the close animation. If you must keep that animation, keep the
+`Dialog` + `DialogContent` shell mounted and remount only the state-bearing
+**body** per open:
+
+```tsx
+function Parent() {
+  const [open, setOpen] = useState(false);
+  const [openCount, setOpenCount] = useState(0);
+  return (
+    <Dialog open={open} onOpenChange={setOpen}>
+      <DialogContent>{open ? <ModalBody key={openCount} /> : null}</DialogContent>
+    </Dialog>
+  );
+}
+```
+
+This only works when `DialogContent` is **independent of body state**. If the
+shell depends on the body (e.g. a width className driven by the body's current
+sub-view), the body must own `DialogContent`, so there is no stable shell to
+keep, and genuine unmount (losing the exit animation) is the right call. That is
+exactly the executor add-account-modal: it genuinely unmounts and accepts the
+lost animation rather than plumbing body state up to a shell.
+
+## Reviewing: flag these smells
+
+1. A hand-written `reset()` exists. Its presence means state outlives the modal;
+   ask why the modal isn't just unmounted.
+2. An always-mounted dialog (rendered unconditionally with an `open` prop) that
+   owns async/in-flight state: popups, timers, subscriptions, AbortControllers,
+   server sessions.
+3. A busy/loading flag composed from a child hook (e.g. `ccBusy || someHook.busy`)
+   where `reset()` clears only part of it.
diff --git a/e2e/cloud/connection-modal-oauth-abandon.test.ts b/e2e/cloud/connection-modal-oauth-abandon.test.ts
@@ -0,0 +1,157 @@
+// Cloud (browser): abandoning an OAuth connection must not wedge the
+// add-connection modal. A user picks a registered OAuth app, clicks "Connect
+// with OAuth" (the modal opens the provider popup and flips to a busy
+// "Connecting…" state), then bails by closing the popup without granting consent.
+// The popup-closed signal is intentionally not polled (providers' COOP headers
+// make `popup.closed` unreliable), so the modal can't detect the abandonment on
+// its own. The guarantee under test: closing the modal afterwards RESETS it, so
+// reopening offers a fresh attempt instead of staying stuck on "Connecting…".
+//
+// Repro for the user report: "I bailed on finishing the OAuth connection … the
+// Executor app can't detect [it]. But closing the modal should reset the state
+// so I can try again."
+import { randomBytes } from "node:crypto";
+
+import { expect } from "@effect/vitest";
+import { Effect } from "effect";
+import { composePluginApi } from "@executor-js/api/server";
+import { openApiHttpPlugin } from "@executor-js/plugin-openapi/api";
+import { IntegrationSlug, OAuthClientSlug } from "@executor-js/sdk/shared";
+import { serveOAuthTestServer } from "@executor-js/sdk/testing";
+
+import { scenario } from "../src/scenario";
+import { Api, Browser, Target } from "../src/services";
+
+const api = composePluginApi([openApiHttpPlugin()] as const);
+
+const unique = (prefix: string) => `${prefix}_${randomBytes(4).toString("hex")}`;
+
+scenario(
+  "Connections · closing the add-connection modal after abandoning OAuth lets you try again (not stuck on Connecting)",
+  { timeout: 120_000 },
+  Effect.scoped(
+    Effect.gen(function* () {
+      const target = yield* Target;
+      const browser = yield* Browser;
+      const { client: makeClient } = yield* Api;
+      // A real authorization server on 127.0.0.1: the modal's popup navigates to
+      // its authorize page, which we abandon by closing the window.
+      const oauth = yield* serveOAuthTestServer();
+      const identity = yield* target.newIdentity();
+      const client = yield* makeClient(api, identity);
+
+      // An integration that declares an OAuth auth method (no DCR: it carries
+      // explicit endpoints, no registration/discovery URL), so the modal shows
+      // the bring-your-own app picker.
+      const integration = IntegrationSlug.make(unique("oauthint"));
+      yield* client.openapi.addSpec({
+        payload: {
+          spec: {
+            kind: "blob",
+            value: JSON.stringify({
+              openapi: "3.0.3",
+              info: { title: "OAuth-protected API", version: "1.0.0" },
+              paths: {
+                "/me": {
+                  get: {
+                    operationId: "getMe",
+                    tags: ["default"],
+                    responses: { "200": { description: "the caller" } },
+                  },
+                },
+              },
+            }),
+          },
+          slug: integration,
+          baseUrl: "http://127.0.0.1:59999",
+          authenticationTemplate: [
+            {
+              slug: "oauth",
+              kind: "oauth2",
+              authorizationUrl: oauth.authorizationEndpoint,
+              tokenUrl: oauth.tokenEndpoint,
+              scopes: ["read"],
+            },
+          ],
+        },
+      });
+
+      // A registered OAuth app whose endpoints match the integration's, so the
+      // picker auto-selects it and the footer offers "Connect with OAuth".
+      const clientSlug = OAuthClientSlug.make(unique("oauthc"));
+      yield* client.oauth.createClient({
+        payload: {
+          owner: "org",
+          slug: clientSlug,
+          authorizationUrl: oauth.authorizationEndpoint,
+          tokenUrl: oauth.tokenEndpoint,
+          grant: "authorization_code",
+          clientId: "test-client",
+          clientSecret: "test-secret",
+        },
+      });
+
+      yield* browser.session(identity, async ({ page, step }) => {
+        const dialog = page.getByRole("dialog");
+        const addConnection = page.getByRole("button", { name: "Add connection", exact: true });
+        const connectWithOAuth = dialog.getByRole("button", { name: "Connect with OAuth" });
+        const connecting = dialog.getByRole("button", { name: "Connecting…" });
+
+        await step("Open the integration and start a new connection", async () => {
+          await page.goto(`/integrations/${integration}`, { waitUntil: "networkidle" });
+          await addConnection.click();
+          // The registered app is auto-selected, so the OAuth connect button is
+          // present and enabled.
+          await connectWithOAuth.waitFor({ state: "visible", timeout: 15_000 });
+          expect(
+            await connectWithOAuth.isDisabled(),
+            "the auto-selected app makes Connect with OAuth actionable",
+          ).toBe(false);
+        });
+
+        await step("Begin OAuth, then bail by closing the provider popup", async () => {
+          const [popup] = await Promise.all([page.waitForEvent("popup"), connectWithOAuth.click()]);
+          // The footer flips to the busy "Connecting…" state while the popup is
+          // open; the flow is genuinely in flight.
+          await connecting.waitFor({ state: "visible", timeout: 15_000 });
+          // Let the popup actually reach the authorize page so the OAuth session
+          // is live, then abandon it: the user closes the window without
+          // granting consent.
+          await popup.waitForURL((url) => !url.href.startsWith("about:"), { timeout: 15_000 });
+          await popup.close();
+        });
+
+        await step("Close the modal", async () => {
+          // The Close button is disabled while busy, so the user backs out with
+          // Escape, exactly the "bail" path from the report.
+          await page.keyboard.press("Escape");
+          await dialog.waitFor({ state: "hidden", timeout: 15_000 });
+        });
+
+        await step(
+          "Reopen the modal: it offers a fresh attempt, not a stuck Connecting",
+          async () => {
+            await addConnection.click();
+            await dialog.waitFor({ state: "visible", timeout: 15_000 });
+            await page.waitForLoadState("networkidle");
+
+            // The guarantee: the reopened modal is reset. Before the fix it stays
+            // wedged on "Connecting…" (the abandoned flow's busy state survived the
+            // close), so this count is 1 and the test fails, reproducing the bug.
+            expect(
+              await connecting.count(),
+              "the reopened modal must not be stuck in the Connecting state",
+            ).toBe(0);
+
+            // And a fresh OAuth attempt is actually offered and actionable again.
+            await connectWithOAuth.waitFor({ state: "visible", timeout: 15_000 });
+            expect(
+              await connectWithOAuth.isDisabled(),
+              "the reopened modal lets the user start OAuth again",
+            ).toBe(false);
+          },
+        );
+      });
+    }),
+  ),
+);
diff --git a/packages/react/src/components/add-account-modal.tsx b/packages/react/src/components/add-account-modal.tsx
@@ -588,7 +588,7 @@ function OAuthAppRadioRow(props: {
   );
 }
 
-export function AddAccountModal(props: {
+interface AddAccountModalProps {
   readonly integration: IntegrationSlug;
   readonly integrationName: string;
   readonly methods: readonly AuthMethod[];
@@ -601,7 +601,20 @@ export function AddAccountModal(props: {
    *  plugin whose auth is fixed (MCP) omits this, hiding the row. */
   readonly createCustomMethod?: CreateCustomMethod;
   readonly removeCustomMethod?: (method: AuthMethod) => Promise<boolean>;
-}) {
+}
+
+/** The add-connection modal is self-contained: every transient bit of state
+ *  (form fields, the in-flight OAuth popup flow) lives in `AddAccountModalView`,
+ *  so closing the modal genuinely unmounts that view and React destroys all of
+ *  it, never hand-reset. Unmounting also runs `useOAuthPopupFlow`'s cleanup,
+ *  which cancels a dangling server OAuth session. That is why abandoning an
+ *  OAuth popup can't wedge a later open: the stuck flow died with its instance.
+ *  The parent owns only open/route intent (deep links, the reconnect handoff). */
+export function AddAccountModal(props: AddAccountModalProps) {
+  return props.open ? <AddAccountModalView {...props} /> : null;
+}
+
+function AddAccountModalView(props: AddAccountModalProps) {
   const {
     integration,
     integrationName,
@@ -855,27 +868,6 @@ export function AddAccountModal(props: {
   const showSavedToPicker = !oauthRegistering && savedToOptions.length > 1;
   const callableName = connectionNameFrom(label, savedToOwner, integrationName, organizationId);
 
-  const reset = () => {
-    setMethodId(methods[0]?.id ?? "");
-    setValues({});
-    setCredentialOrigin("paste");
-    setOnePasswordItemId("");
-    setLabel("");
-    setOwner(defaultOwner);
-    setSubmitting(false);
-    setPickedApp(null);
-    setRegisteringOAuthClient(false);
-    setCcBusy(false);
-    setDcrBusy(false);
-    setDcrFailed(false);
-    setShowOtherApps(false);
-    setEditingClient(null);
-    setRemovingClient(null);
-    setCreatedMethods([]);
-    setRemovedMethodIds(new Set());
-    setAddingMethod(false);
-  };
-
   // Build the picker row's Edit/Remove menu for an app, but only once its full
   // summary has loaded (the picker option lacks endpoints/resource). Until then
   // the row shows no actions menu rather than a broken one.
@@ -953,10 +945,10 @@ export function AddAccountModal(props: {
     }
   };
 
-  const close = () => {
-    onOpenChange(false);
-    reset();
-  };
+  // Just ask the parent to close. Reopening remounts this whole component (see
+  // AddAccountModal), so there is nothing to hand-reset: the form fields and the
+  // OAuth popup flow's busy state die with this instance.
+  const close = () => onOpenChange(false);
 
   const credentialPayloadOrigin = createCredentialPayloadOrigin({
     origin: credentialOrigin,
@@ -1169,13 +1161,7 @@ export function AddAccountModal(props: {
   };
 
   return (
-    <Dialog
-      open={open}
-      onOpenChange={(next: boolean) => {
-        if (!next) close();
-        else onOpenChange(true);
-      }}
-    >
+    <Dialog open={open} onOpenChange={onOpenChange}>
       <DialogContent
         className={cn(
           "max-h-[85vh] overflow-x-hidden overflow-y-auto",