Skip to content

fix: keep a sheet presented when snapToPosition targets a temporary position - #2738

Open
KurnosovNikita wants to merge 1 commit into
gorhom:masterfrom
KurnosovNikita:fix/snap-to-position-dismisses-modal
Open

fix: keep a sheet presented when snapToPosition targets a temporary position#2738
KurnosovNikita wants to merge 1 commit into
gorhom:masterfrom
KurnosovNikita:fix/snap-to-position-dismisses-modal

Conversation

@KurnosovNikita

@KurnosovNikita KurnosovNikita commented Aug 13, 2026

Copy link
Copy Markdown

Motivation

Fixes #2739snapToPosition() dismisses a BottomSheetModal.

Repro Snack: https://snack.expo.dev/Bfj1FfiQhXKRNduNipoJN — present the modal, bump the counter,
tap Reposition 1px; onChange index=-1 then onDismiss, and the sheet's state is gone.

animateToPosition resolves the index of its target by looking the position up in the detents
array:

let index = detents?.indexOf(position + offset) ?? -1;

A snapToPosition() target is by definition not a detent — that is what the API is for, and
handleSnapToPosition marks it with isInTemporaryPosition.value = true. The -1 is written to
nextIndex, and when the animation completes:

if (nextIndex === -1) {
  runOnJS(handleOnClose)();
}

On a BottomSheetModal that is handleBottomSheetOnClose → status DISMISSEDunmount()
onDismiss. The sheet unmounts and everything in its content is destroyed, with nothing having
asked for a dismissal. The keyboard recovery immediately above is the only thing that ever
prevents it, and only while KEYBOARD_STATUS.SHOWN.

We hit this in production: an Android-only repair that nudges a presented sheet a pixel with
snapToPosition() after returning from the system photo picker. Every nudge dismissed the sheet —
five for five in one user's diagnostic bundle, each onChange {index: -1} landing at exactly the
nudged position, taking a filled-in form with it.

The change

One branch, next to the keyboard recovery it mirrors:

if (
  index === -1 &&
  source === ANIMATION_SOURCE.USER &&
  position !== closedDetentPosition &&
  animatedCurrentIndex.get() !== -1
) {
  index = animatedCurrentIndex.get();
}

A temporary position is not part of the detents array either, and unlike the closed position it is
not a dismissal. Reporting the index the sheet already holds mirrors animatedIndex, which
returns animatedCurrentIndex rather than -1 while isInTemporaryPosition — and it keeps the
reposition invisible to onChange as well, since animateToPositionCompleted fires it only when
the index actually changed.

It cannot swallow a genuine dismissal. Every close — close(), forceClose(), pan-down —
animates to closedDetentPosition, which the branch excludes; gesture, keyboard,
snap-point-change and mount animations carry a source other than USER; and a sheet already at
-1 is left alone. The gesture path already makes exactly this distinction
(nextPositionIndex === -1 && !isInTemporaryPosition.value), so this only brings
animateToPosition in line with it.

Why source and not isInTemporaryPosition. handleSnapToPosition sets that flag wherever it
is called from — commonly the JS thread — and then schedules animateToPosition on the UI thread
via runOnUI. A fix depending on that write landing first would be silently ineffective if the
ordering ever changed. source is an argument: it is already on the UI thread when the branch
reads it. Happy to switch to the flag (or to both) if you'd rather keep the concept in one place.

animatedCurrentIndex is added to the callback's dependency list in the same edit.

Test plan

  • yarn typescript — clean.
  • yarn lint — the changed file is clean; the one warning biome reports
    (src/hooks/useBoundingClientRect.ts:54, useOptionalChain) is pre-existing on master and
    untouched here. There is no test script at the root, so nothing else to run.
  • Carried in a production React Native app (RN 0.86 / Expo 57 / reanimated 4) as a patch-package
    patch on 5.2.14: applies clean, that app's suite is green (195 files / 1774 tests), and its own
    guard asserts the branch is present in both shipped entry points.
  • Not yet device-verified, stating it plainly: the on-device pass is scheduled but has not run,
    so I have not personally watched a repositioned sheet stay presented on hardware. The evidence I
    do have is the source path, the Snack, and five-for-five dismissals in a production diagnostic
    bundle that stop being possible with this branch in place. I'll report back here once the device
    pass is done — and if you'd rather wait for that before reviewing, that's fair.

I can add the Snack's repro to the example app if you'd like one in the tree — tell me where you'd want it.

…osition

`animateToPosition` resolves the index of its target by looking the position up
in the detents array, and a `snapToPosition` target is deliberately not one of
them. The resulting -1 is written to `nextIndex`, and on completion
`animateToPositionCompleted` fires `onClose` for that index — which on a
`BottomSheetModal` reaches `unmount()` and `onDismiss`. So `snapToPosition`
dismisses a modal and destroys its content, unless the keyboard happens to be
SHOWN and the recovery above catches it first.

Add the same recovery for a temporary position: report the index the sheet
already holds, mirroring `animatedIndex`, which returns `animatedCurrentIndex`
rather than -1 while `isInTemporaryPosition`. Reporting the current index also
keeps the reposition invisible to `onChange`, which fires only when the index
changed.

The branch cannot swallow a real dismissal: every close — `close()`,
`forceClose()`, pan-down — animates to `closedDetentPosition`, which is excluded,
and gesture, keyboard, snap-point-change and mount animations carry a source
other than USER. A sheet already at -1 is left alone.

Keyed on `source` rather than on `isInTemporaryPosition` because
`handleSnapToPosition` sets that flag wherever it is called from — often the JS
thread — and then schedules `animateToPosition` on the UI thread; `source` is an
argument and is already there.

Fixes gorhom#2736
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.

[Bug]: snapToPosition() dismisses a BottomSheetModal — a temporary position resolves to index -1 and fires onClose

1 participant