Skip to content

fix(modal): make dismiss idempotent when modal is not presented - #2711

Open
janicduplessis wants to merge 1 commit into
gorhom:masterfrom
janicduplessis:fix/modal-dismiss-before-present
Open

fix(modal): make dismiss idempotent when modal is not presented#2711
janicduplessis wants to merge 1 commit into
gorhom:masterfrom
janicduplessis:fix/modal-dismiss-before-present

Conversation

@janicduplessis

@janicduplessis janicduplessis commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

Motivation

Calling dismiss() on a modal whose status is INITIAL — never presented, or already self-dismissed and reset — permanently breaks it: every subsequent present() silently no-ops. handleDismiss falls through the early-exit and sets statusRef to DISMISSING, nothing ever resets it (no BottomSheet is mounted to fire onClose), and handlePortalRender bails on DISMISSING forever after.

This breaks the common declarative wrapper pattern:

useEffect(() => {
  if (visible) modalRef.current?.present();
  else modalRef.current?.dismiss();
}, [visible]);

A backdrop/swipe close resets the status to INITIAL, so the wrapper's next dismiss() hits exactly this case.

This PR treats INITIAL as already-closed in handleDismiss, mirroring CLOSED/MINIMIZED, so dismiss() is idempotent. Full analysis and a Snack repro are in #2669.

Verified in a real app on iOS after upgrading to 5.2.14: with this change, present → user close → dismiss() → present cycles work repeatedly.

Fixes #2669

Before / after

Minimal repro (scripted): dismiss() on a never-presented modal, then present() twice.

Before — present() never works after the initial dismiss():

upstream-before2.mp4

After — dismiss() is a safe no-op and present() opens the sheet:

upstream-after2.mp4

Calling dismiss() while the modal status is INITIAL (never presented, or
already fully dismissed and reset) fell through the early-exit and left
statusRef stuck at DISMISSING, after which every present() silently
no-oped in handlePortalRender. Treat INITIAL as already-closed so
dismiss() is safe to call from declarative wrappers that mirror an
'open' prop.

Fixes gorhom#2669
@github-actions

github-actions Bot commented Aug 1, 2026

Copy link
Copy Markdown

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@janicduplessis

Copy link
Copy Markdown
Contributor Author

Not stale

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown

This PR is stale because it has been open 30 days with no activity. Remove stale label or comment or this will be closed in 10 days.

david-mobile1 added a commit to david-mobile1/react-native-bottom-sheet that referenced this pull request Sep 4, 2026
An interrupted close animation leaves `statusRef` at `DISMISSING` forever,
because `handleBottomSheetOnClose` is its only exit. From there the failure
is self-sustaining: `handlePortalRender` returns early, so the inner sheet
is never mounted and `bottomSheetRef` stays null, so the only line that
clears the status never runs. `present()` becomes a permanent, silent no-op
and no caller can recover it.

`handleForceDismiss` escapes via `DISMISSING && currentIndexRef === -1`, but
`handleDismiss` has no such branch and the index never reached -1 here.

Distinct from the INITIAL-status wedges in gorhom#2669 / gorhom#2723 and the fixes in
 gorhom#2670, gorhom#2711 and gorhom#2725: this path starts from a legitimate dismissal of a
presented modal, so every INITIAL guard is bypassed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: dismiss() before first present() corrupts statusRef, blocking subsequent present()

1 participant