Skip to content

Make the add-connection modal self-contained so abandoned OAuth can't wedge it - #1126

Merged
RhysSullivan merged 1 commit into
mainfrom
claude/nice-banzai-534ae2
Jun 25, 2026
Merged

Make the add-connection modal self-contained so abandoned OAuth can't wedge it#1126
RhysSullivan merged 1 commit into
mainfrom
claude/nice-banzai-534ae2

Conversation

@RhysSullivan

@RhysSullivan RhysSullivan commented Jun 25, 2026

Copy link
Copy Markdown
Collaborator

Problem

A user reported the add-connection modal getting stuck on "Connecting…": they began an OAuth connection, abandoned the provider popup (closed it without granting consent), then closed the modal. Reopening showed the footer still wedged on "Connecting…" with the Close button disabled, and no way to retry.

Root cause

The add-account modal was kept mounted across open/close (its parent renders it with an open prop rather than conditionally mounting it), so transient state survived a close. The OAuth popup flow's busy state lives in the useOAuthPopupFlow hook, not in the modal's own useState, and the modal's hand-written reset() zeroed its local booleans but never touched the hook.

Because we intentionally do not poll popup.closed (provider COOP headers make it unreliable), abandoning the popup leaves the hook's busy true. With the modal always mounted, that state survived the close, so:

oauthBusy = ccBusy(false) || oauthPopup.busy(true) = true

and the reopened footer stayed on "Connecting…".

Fix

Rather than adding another setter to reset(), the modal is now self-contained and genuinely unmounts on close. The whole body lives in AddAccountModalView, and the thin AddAccountModal wrapper mounts it only while open:

export function AddAccountModal(props: AddAccountModalProps) {
  return props.open ? <AddAccountModalView {...props} /> : null;
}
  • reset() is deleted. Closing flips open to false, React unmounts the view, and that destroys the form fields and the OAuth popup flow's busy state for free, and runs the hook's unmount cleanup that cancels the dangling server-side OAuth session. A closed modal owns no state to reset.
  • Reconnect and deep-link handoffs still prefill: the view mounts with its initialState already set and applies it on mount.
  • Tradeoff: 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, the deliberate cost of correct teardown here.

Evidence

New cloud e2e scenario e2e/cloud/connection-modal-oauth-abandon.test.ts drives the real path against a local authorization server:

  1. Open the integration, start a connection, click Connect with OAuth (footer flips to "Connecting…").
  2. Let the popup reach the authorize page, then close it (the abandon).
  3. Close the modal with Escape (Close is disabled while busy, exactly the reported bail path).
  4. Reopen and assert there is no stuck "Connecting…" button and a fresh OAuth attempt is offered.

Before the fix this scenario fails (expected 1 to be 0); with the fix it passes. The existing connect-panel browser scenario still passes, covering the happy-path open/connect flow under mount-on-open.

Also included

A self-contained-modals skill that codifies the underlying rule (a modal's form and in-flight state live inside it and are destroyed on close by unmounting; a hand-written reset() is a review smell) so this lifecycle-leak class gets caught in review rather than re-introduced.

An audit of the rest of the codebase surfaced two more low-severity instances of the same pattern (a stale-on-reopen saving flag in the connection/integration edit sheets, and an invite-dialog reducer left in an error state); those are out of scope here and can be tracked separately.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 25, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
executor-marketing 1a17293 Commit Preview URL

Branch Preview URL
Jun 25 2026, 06:54 AM

@github-actions

github-actions Bot commented Jun 25, 2026

Copy link
Copy Markdown
Contributor

Cloudflare preview

Torn down — the PR is closed.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Jun 25, 2026

Copy link
Copy Markdown

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Updated (UTC)
✅ Deployment successful!
View logs
executor-cloud 1a17293 Jun 25 2026, 06:54 AM

@pkg-pr-new

pkg-pr-new Bot commented Jun 25, 2026

Copy link
Copy Markdown

Open in StackBlitz

@executor-js/cli

npm i https://pkg.pr.new/@executor-js/cli@1126

@executor-js/config

npm i https://pkg.pr.new/@executor-js/config@1126

@executor-js/execution

npm i https://pkg.pr.new/@executor-js/execution@1126

@executor-js/sdk

npm i https://pkg.pr.new/@executor-js/sdk@1126

@executor-js/codemode-core

npm i https://pkg.pr.new/@executor-js/codemode-core@1126

@executor-js/runtime-quickjs

npm i https://pkg.pr.new/@executor-js/runtime-quickjs@1126

@executor-js/plugin-file-secrets

npm i https://pkg.pr.new/@executor-js/plugin-file-secrets@1126

@executor-js/plugin-graphql

npm i https://pkg.pr.new/@executor-js/plugin-graphql@1126

@executor-js/plugin-keychain

npm i https://pkg.pr.new/@executor-js/plugin-keychain@1126

@executor-js/plugin-mcp

npm i https://pkg.pr.new/@executor-js/plugin-mcp@1126

@executor-js/plugin-onepassword

npm i https://pkg.pr.new/@executor-js/plugin-onepassword@1126

@executor-js/plugin-openapi

npm i https://pkg.pr.new/@executor-js/plugin-openapi@1126

executor

npm i https://pkg.pr.new/executor@1126

commit: 1a17293

@RhysSullivan
RhysSullivan force-pushed the claude/nice-banzai-534ae2 branch from 1b38390 to 10f2383 Compare June 25, 2026 06:35
@RhysSullivan RhysSullivan changed the title Reset add-connection modal OAuth busy state on close Make the add-connection modal self-contained so abandoned OAuth can't wedge it Jun 25, 2026
… wedge it

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 ? <View/> : 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.
@RhysSullivan
RhysSullivan force-pushed the claude/nice-banzai-534ae2 branch from 10f2383 to 1a17293 Compare June 25, 2026 06:52
@RhysSullivan
RhysSullivan marked this pull request as ready for review June 25, 2026 07:07
@RhysSullivan
RhysSullivan merged commit d958b31 into main Jun 25, 2026
15 checks passed
@greptile-apps

greptile-apps Bot commented Jun 25, 2026

Copy link
Copy Markdown

Greptile Summary

This PR makes the add-connection modal tear down its own transient state on close. The main changes are:

  • AddAccountModal now only mounts the stateful view while open is true.
  • The manual reset() path was removed in favor of React unmount cleanup.
  • A cloud e2e scenario covers abandoning an OAuth popup, closing, and reopening.
  • A new modal lifecycle skill documents the self-contained modal pattern.

Confidence Score: 5/5

This looks safe to merge.

  • No blocking issues found in the changed code.
  • The modal close path now tears down the stateful view that owns the OAuth popup flow.
  • The new e2e test covers the reported stuck Connecting state.

Important Files Changed

Filename Overview
packages/react/src/components/add-account-modal.tsx Moves the stateful modal body behind a conditional wrapper and relies on unmount cleanup instead of manual reset calls.
e2e/cloud/connection-modal-oauth-abandon.test.ts Adds a browser scenario for the abandoned OAuth popup flow and verifies reopening starts from a clean modal state.
.claude/skills/self-contained-modals/SKILL.md Adds guidance for keeping modal form and in-flight state inside the mounted dialog body.

Reviews (1): Last reviewed commit: "Make the add-connection modal self-conta..." | Re-trigger Greptile

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant