Skip to content

fix: settle openAsync results when reopening the same overlay - #234

Open
chungyo wants to merge 1 commit into
toss:mainfrom
chungyo:fix/open-async-reuse
Open

chungyo wants to merge 1 commit into
toss:mainfrom
chungyo:fix/open-async-reuse

Conversation

@chungyo

@chungyo chungyo commented Sep 9, 2026

Copy link
Copy Markdown

Description

Reopening a closed overlay with the same overlayId currently leaves the new openAsync promise pending. For example, openAsync → close('first') → openAsync → close('second') resolves only the first call, even though the second overlay closes.

This happens because reopening deliberately retains the original controller and component key to preserve component state, including the controller wrapper that captured the first promise. This change keeps that state-preserving behavior and supplies the result callbacks belonging to each new invocation.

Changes

  • Store the current invocation's resolve/reject callbacks on the overlay item, and update them when reopening it.
  • Supply those callbacks to the retained controller without capturing a promise in the controller wrapper. Preserve the original controller kind so synchronous controllers keep their existing props, while retained async controllers still receive reject when reopened synchronously.
  • Ignore close actions from callbacks belonging to an older invocation, so delayed callbacks cannot close the reopened overlay.
  • Add eleven regression/control tests and a patch changeset. No dependencies are added.

Motivation and Context

The FAQ documents close as retaining state for reuse, whereas unmount resets it. Requiring an unmount between openAsync calls would resolve the promise problem by discarding the very state that close is meant to retain.

How Has This Been Tested?

  • The first six regressions failed against unmodified main before the source fix. A separate main snapshot comparison reproduces eight reuse regressions while the unmount control and all 57 pre-existing tests pass. The synchronous-props compatibility regression was also observed failing before its fix.
  • All 68 tests pass after the fix, including synchronous controller props, close/reject reuse under StrictMode, stale callbacks, original-controller preservation, mixed open/openAsync calls, ID/context isolation, and unmount/reset behavior. A separate tsc --noEmit check passes. The full tests, typecheck, and package build also pass on the repository-pinned Node 22.20.0 with Yarn 4.5.1.
  • Package build, repository lint, attw --pack, publint, and the React 16/17/18/19 example builds pass.
  • The built CommonJS package passes a React DOM/jsdom harness with React 16.14.0, 17.0.2, 18.3.1, and 19.0.0 under StrictMode: repeated results settle, state remains at 7 across close/reopen, stale callbacks do not close or settle a later invocation, and unmount resets state to 0.
  • A native Chromium 152 / React 19.2.4 StrictMode run preserves input value 7 on reopening, resolves the next call with 8, and rejects a third call with 8 without page errors. The baseline leaves the second call pending.
  • The main test suite reports 15 existing React act warnings both before and after this change; the added regression file does not add warnings.

Types of changes

  • Bug fix
  • New feature
  • Breaking change
  • Documentation update

Checklist

  • I have performed a self-review of my own code.
  • My code is commented where needed.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective.
  • New and existing unit tests pass locally with my changes.

The existing FAQ already describes the intended state preservation; no documentation or downstream dependency change is required.

Further Comments

This does not settle promises closed externally with overlay.close/unmount; that is the separate behavior addressed by #215. The implementation touches some of the same files, so merging both changes will require an integration check. Animation-frame scheduling and overlay ordering are unchanged.

The public open/openAsync signatures and return types are preserved, as are the props supplied to each original controller kind. useOverlayData exposes the additional optional asyncResult and isAsyncController fields stored on the item. Reusing an ID still retains the original controller, so callers must keep that controller's result contract consistent; replacing the controller or changing its result type is outside this fix.

@chungyo
chungyo requested a review from jungpaeng as a code owner September 9, 2026 01:19
@changeset-bot

changeset-bot Bot commented Sep 9, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: ff68d5e

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
overlay-kit Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@vercel

vercel Bot commented Sep 9, 2026

Copy link
Copy Markdown

@chungyo is attempting to deploy a commit to the Toss Team on Vercel.

A member of the Team first needs to authorize it.

This branch has not been deployed

No deployments
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