From 1a17293a3043ef87420e01de54016b03a0cd9268 Mon Sep 17 00:00:00 2001 From: Rhys Sullivan Date: Wed, 24 Jun 2026 23:19:28 -0700 Subject: [PATCH] Make the add-connection modal self-contained so abandoned OAuth can't wedge it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The add-account modal stays mounted across open/close (its parent passes an `open` prop rather than conditionally mounting it), so transient state survived a close. In particular the OAuth popup flow's busy state lives in `useOAuthPopupFlow`, not the modal's own useState, and a hand-written `reset()` cleared the modal's local booleans but never touched it. So if a user began OAuth and then abandoned the provider popup (closed it without granting consent, which we intentionally do not poll for because provider COOP headers make `popup.closed` unreliable), the footer stayed wedged on "Connecting…" with Close disabled. Reopening the modal showed the same stuck state with no way to retry. The fix is structural rather than another setter in `reset()`: the modal is now self-contained. The whole body lives in `AddAccountModalView`, and the thin `AddAccountModal` wrapper mounts it only while open (`open ? : null`). Closing flips `open` to false, which genuinely unmounts the view, so React destroys the form fields and the OAuth popup flow's busy state and runs the hook's unmount cleanup that cancels the dangling server OAuth session. `reset()` is deleted: a closed modal owns no state to reset. Reconnect and deep-link handoffs still prefill, because the view mounts with its `initialState` already set and applies it on mount. The view owns the dynamic-width DialogContent (its width tracks the body's current sub-view), so there is no body-independent shell to keep mounted while the body remounts; unmounting the whole Dialog forgoes the Radix close animation. That is the deliberate tradeoff for correct teardown. Adds a cloud e2e scenario driving the real abandon-then-reopen path: it starts OAuth against a local authorization server, closes the popup mid-flow, closes the modal with Escape, reopens, and asserts the modal offers a fresh attempt instead of a stuck "Connecting…". Also adds a self-contained-modals skill codifying the underlying rule (form and in-flight state live inside the modal; closing unmounts it) so this lifecycle-leak class gets flagged in review. --- .claude/skills/self-contained-modals/SKILL.md | 77 +++++++++ .../connection-modal-oauth-abandon.test.ts | 157 ++++++++++++++++++ .../src/components/add-account-modal.tsx | 54 +++--- 3 files changed, 254 insertions(+), 34 deletions(-) create mode 100644 .claude/skills/self-contained-modals/SKILL.md create mode 100644 e2e/cloud/connection-modal-oauth-abandon.test.ts diff --git a/.claude/skills/self-contained-modals/SKILL.md b/.claude/skills/self-contained-modals/SKILL.md new file mode 100644 index 000000000..6ff660805 --- /dev/null +++ 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 ? setOpen(false)} /> : null; +} +``` + +A key bump (``) 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 ( + + {open ? : null} + + ); +} +``` + +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 new file mode 100644 index 000000000..2b33bb82a --- /dev/null +++ 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 index f7927a230..bb314fd6b 100644 --- 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; -}) { +} + +/** 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 ? : 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 ( - { - if (!next) close(); - else onOpenChange(true); - }} - > +