react-native-bottom-sheet icon indicating copy to clipboard operation
react-native-bottom-sheet copied to clipboard

[Bug]: Changing FlatList data causes bottomsheet to close

Open geraintf opened this issue 1 year ago • 25 comments

Version

v5

Reanimated Version

v3

Gesture Handler Version

v2

Platforms

iOS

What happened?

If enableDynamicSizing is true and index is set to -1 then when the data for a BottomSheetFlatList changes the open index gets set to -1 ie. closes the bottom sheet.

  • This does no occur with dynamic sizing off
  • This does not occur if the index prop is missing/set to something other than -1

Reproduced on ios, untested on web/android

Reproduction steps

Using the reproduction example:

  • Press 'open' to open the bottom sheet
  • Press 'add' to modify the flatlist data
  • See the bottom sheet closes

Reproduction sample

https://snack.expo.dev/@geraintfisher/bottom-sheet-bug

Relevant log output

No response

geraintf avatar Dec 03 '24 20:12 geraintf

Same issue here, although not specifically related to FlatList. Also happens with BottomSheetView when component re-renders. iOS only.

Simonas-Juska avatar Dec 09 '24 13:12 Simonas-Juska

Having the exact same issue that @Simonas-Juska mentioned. Whenever a rerender happens (for example, if we do something like onChange={setSnapIndex} or something similar) the bottom sheet closes the first time it's opened and afterwards it works correctly. After some in depth debugging of the flow I've found that doing the following patch:

diff --git a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
index c79181a..857f980 100644
--- a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
+++ b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
@@ -240,7 +240,7 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
     const animatedCurrentIndex = useReactiveSharedValue(
       animateOnMount ? -1 : _providedIndex
     );
-    const animatedPosition = useSharedValue(INITIAL_POSITION);
+    const animatedPosition = useSharedValue(0);
     const animatedNextPosition = useSharedValue(INITIAL_VALUE);
     const animatedNextPositionIndex = useSharedValue(INITIAL_VALUE);

fixes this issue. Not entirely sure on which commit introduces it, but it's likely a side-effect of something else. I believe the underlying reason is related to the fact that upon first opening, this line gets hit and as a (probably) unwanted consequence of this change (or one of the related ones to it) we get an automatic close as the position is not yet updated.

I unfortunately did not have the time to debug in more depth but I hope this might be helpful for you @gorhom - please let me know if I can provide any additional help !

Note: In my circumstance, it does not at all matter whether enableDynamicSizing is set to true or false - but rather the rerender causes the issue.

Note 2: The issue is reproducible for all 5.x.x versions.

isekovanic avatar Dec 09 '24 16:12 isekovanic

Same issue here... on iOS too

r-obeen avatar Dec 10 '24 00:12 r-obeen

same issue on ios...

MianIbrarHussain avatar Dec 11 '24 07:12 MianIbrarHussain

Same issue. Whenever a prop changes on first mount, it closes the sheet. @gorhom be great if we can fix this in the next version 🙏

arsotech avatar Dec 11 '24 09:12 arsotech

Same issue here, patch from @isekovanic is working, thanks :-)

Can we have a new release?

Ben-WD avatar Dec 11 '24 10:12 Ben-WD

Same issue here, patch from @isekovanic is working, thanks :-)

Can we have a new release?

Patch is not working for me unfortunately. New architecture / ios. Patch causes the sheet (in close mode index = -1) to render momentarily over the screen before it closes to it's default position.

arsotech avatar Dec 11 '24 10:12 arsotech

@arsotech Yeah, I'm not at all confident that it's the exact root cause of the issue sadly - just something I found helps in my specific scenario while debugging; I can give it another go tonight and try to dig a bit deeper

isekovanic avatar Dec 11 '24 10:12 isekovanic

@arsotech Yeah, I'm not at all confident that it's the exact root cause of the issue sadly - just something I found helps in my specific scenario while debugging; I can give it another go tonight and try to dig a bit deeper

Thanks good sir. Appreciate that.

arsotech avatar Dec 11 '24 10:12 arsotech

Same issue happening on iOS only. android is fine

ustuncem avatar Dec 11 '24 18:12 ustuncem

I think the problem lies in how evaluatePosition calculates the bottom sheet's next position during re-renders. The logic inadvertently snaps the sheet to -1 (closed state) but it should snap to currentIndex. Fixed it by adding a check:

index c79181a..1ac7d7f 100644
--- a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
+++ b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
@@ -191,6 +191,7 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
     const _animatedContainerHeight = useReactiveSharedValue(
       _providedContainerHeight ?? INITIAL_CONTAINER_HEIGHT
     );
+
     /**
      * This is a conditional variable, where if the `BottomSheet` is used
      * in a modal, then it will subset vertical insets (top+bottom) from
@@ -789,6 +790,10 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
       function getEvaluatedPosition(source: ANIMATION_SOURCE) {
         'worklet';
         const currentIndex = animatedCurrentIndex.value;
+        if (source !== ANIMATION_SOURCE.MOUNT && currentIndex >= 0) {
+          return animatedSnapPoints.value[currentIndex];
+        }
+
         const snapPoints = animatedSnapPoints.value;
         const keyboardState = animatedKeyboardState.value;
         const highestSnapPoint = animatedHighestSnapPoint.value;

Might help those that are having problems on iOS.

ustuncem avatar Dec 11 '24 19:12 ustuncem

if (source !== ANIMATION_SOURCE.MOUNT && currentIndex >= 0) {
+          return animatedSnapPoints.value[currentIndex];
+        }

That indeed fixed it. Thanks @ustuncem for looking into it.

arsotech avatar Dec 11 '24 21:12 arsotech

Yep, it did fix it for me too. How would I implement that ? Would I need to clone the package and modify it ? I guess I'll loose the future updates aren't I ?

Or would it be better to wait a fix from the official repo here ? Thanks !

r-obeen avatar Dec 11 '24 22:12 r-obeen

Yep, it did fix it for me too. How would I implement that ? Would I need to clone the package and modify it ? I guess I'll loose the future updates aren't I ?

Or would it be better to wait a fix from the official repo here ? Thanks !

I am using patch-package for this

arsotech avatar Dec 11 '24 22:12 arsotech

Yep, it did fix it for me too. How would I implement that ? Would I need to clone the package and modify it ? I guess I'll loose the future updates aren't I ?

Or would it be better to wait a fix from the official repo here ? Thanks !

Install patch-package and postinstall-postinstall with npm or yarn etc.

yarn add patch-package postinstall-postinstall

Then create a folder called patches at the root of your project.

Add a script to your package.json postinstall: patch-package.

Then create a file under the patches folder called "@gorhom+bottom-sheet+5.0.6.patch"

Copy paste the patch that I posted earlier

Lastly, run yarn install. Now that patch should apply every time you run yarn install or npm install. When there is an official fix, you can remove that patch.

Alternatively, just apply this patch manually in node_modules and then run npx patch-package @gorhom/bottom-sheet this will create the file automatically for you

ustuncem avatar Dec 11 '24 22:12 ustuncem

Having the exact same issue that @Simonas-Juska mentioned. Whenever a rerender happens (for example, if we do something like onChange={setSnapIndex} or something similar) the bottom sheet closes the first time it's opened and afterwards it works correctly. After some in depth debugging of the flow I've found that doing the following patch:

diff --git a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
index c79181a..857f980 100644
--- a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
+++ b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
@@ -240,7 +240,7 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
     const animatedCurrentIndex = useReactiveSharedValue(
       animateOnMount ? -1 : _providedIndex
     );
-    const animatedPosition = useSharedValue(INITIAL_POSITION);
+    const animatedPosition = useSharedValue(0);
     const animatedNextPosition = useSharedValue(INITIAL_VALUE);
     const animatedNextPositionIndex = useSharedValue(INITIAL_VALUE);

fixes this issue. Not entirely sure on which commit introduces it, but it's likely a side-effect of something else. I believe the underlying reason is related to the fact that upon first opening, this line gets hit and as a (probably) unwanted consequence of this change (or one of the related ones to it) we get an automatic close as the position is not yet updated.

I unfortunately did not have the time to debug in more depth but I hope this might be helpful for you @gorhom - please let me know if I can provide any additional help !

Note: In my circumstance, it does not at all matter whether enableDynamicSizing is set to true or false - but rather the rerender causes the issue.

Note 2: The issue is reproducible for all 5.x.x versions.

i was follow this. Go in to /node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx and using patch-packages to save edit in nodemoules and i work for me! @arsotech thanks bro!

khanwilson avatar Dec 12 '24 02:12 khanwilson

Я думаю, проблема в том, как estimatePosition вычисляет следующую позицию нижнего листа во время повторных рендеров. Логика непреднамеренно привязывает лист к -1 (закрытое состояние), хотя он должен привязываться к currentIndex. Исправил это, добавив проверку:

index c79181a..1ac7d7f 100644
--- a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
+++ b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
@@ -191,6 +191,7 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
     const _animatedContainerHeight = useReactiveSharedValue(
       _providedContainerHeight ?? INITIAL_CONTAINER_HEIGHT
     );
+
     /**
      * This is a conditional variable, where if the `BottomSheet` is used
      * in a modal, then it will subset vertical insets (top+bottom) from
@@ -789,6 +790,10 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
       function getEvaluatedPosition(source: ANIMATION_SOURCE) {
         'worklet';
         const currentIndex = animatedCurrentIndex.value;
+        if (source !== ANIMATION_SOURCE.MOUNT && currentIndex >= 0) {
+          return animatedSnapPoints.value[currentIndex];
+        }
+
         const snapPoints = animatedSnapPoints.value;
         const keyboardState = animatedKeyboardState.value;
         const highestSnapPoint = animatedHighestSnapPoint.value;

Может помочь тем, у кого возникли проблемы на iOS.

work!! @arsotech thanks!

MrNapcae avatar Dec 14 '24 07:12 MrNapcae

still exist for me

Nader-CS avatar Jan 08 '25 13:01 Nader-CS

I spend a lot of hours to debug, this helped me

diff --git a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
index c79181a..b8d7dee 100644
--- a/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
+++ b/node_modules/@gorhom/bottom-sheet/src/components/bottomSheet/BottomSheet.tsx
@@ -792,6 +792,24 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
         const snapPoints = animatedSnapPoints.value;
         const keyboardState = animatedKeyboardState.value;
         const highestSnapPoint = animatedHighestSnapPoint.value;
+        const currentPosition = animatedPosition.value;
+        const closedPosition = animatedClosedPosition.value;
+
+        /**
+         * Handle initial mount and dynamic content changes during mount
+         */
+        if (!isAnimatedOnMount.value || source === ANIMATION_SOURCE.MOUNT) {
+          const position = snapPoints[_providedIndex];
+          return position;
+        }
+
+        // Only maintain closed state if we're actually closed and not trying to open
+        const isCurrentlyClosed = currentPosition >= closedPosition;
+        if (source === ANIMATION_SOURCE.SNAP_POINT_CHANGE && 
+            isCurrentlyClosed && 
+            _providedIndex === -1) {
+          return closedPosition;
+        }
 
         /**
          * if the keyboard blur behavior is restore and keyboard is hidden,
@@ -849,7 +867,8 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
           isInTemporaryPosition.value = true;
           const keyboardHeightInContainer =
             animatedKeyboardHeightInContainer.value;
-          return Math.max(0, highestSnapPoint - keyboardHeightInContainer);
+          const nextPosition = Math.max(0, highestSnapPoint - keyboardHeightInContainer);
+          return nextPosition;
         }
 
         /**
@@ -857,23 +876,65 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
          * the current position.
          */
         if (isInTemporaryPosition.value) {
-          return animatedPosition.value;
+          return currentPosition;
         }
 
         /**
-         * if the bottom sheet did not animate on mount,
-         * then we return the provided index or the closed position.
+         * Handle dynamic content changes after mount
          */
-        if (!isAnimatedOnMount.value) {
-          return _providedIndex === -1
-            ? animatedClosedPosition.value
-            : snapPoints[_providedIndex];
+        if (source === ANIMATION_SOURCE.SNAP_POINT_CHANGE) {
+          // If we're trying to open the sheet, use the target snap point
+          if (_providedIndex !== -1) {
+            const targetPosition = snapPoints[_providedIndex];
+            return targetPosition;
+          }
+
+          // Find the closest snap point to current position
+          let closestPoint = highestSnapPoint;
+          let minDistance = Math.abs(currentPosition - highestSnapPoint);
+          
+          for (const point of snapPoints) {
+            const distance = Math.abs(currentPosition - point);
+            if (distance < minDistance) {
+              minDistance = distance;
+              closestPoint = point;
+            }
+          }
+          
+          return closestPoint;
         }
 
         /**
-         * return the current index position.
+         * return the current index position or fallback to closest point
          */
-        return snapPoints[currentIndex];
+        const position = snapPoints[currentIndex];
+        
+        if (position === undefined) {
+          // If sheet was closed and should stay closed
+          if (currentPosition >= closedPosition && _providedIndex === -1) {
+            return closedPosition;
+          }
+          
+          // Use provided index if available
+          if (_providedIndex !== -1) {
+            return snapPoints[_providedIndex];
+          }
+          
+          // Find closest snap point as fallback
+          let closestPoint = highestSnapPoint;
+          let minDistance = Math.abs(currentPosition - highestSnapPoint);
+          
+          for (const point of snapPoints) {
+            const distance = Math.abs(currentPosition - point);
+            if (distance < minDistance) {
+              minDistance = distance;
+              closestPoint = point;
+            }
+          }
+          return closestPoint;
+        }
+        
+        return position;
       },
       [
         animatedContentGestureState,
@@ -884,6 +945,7 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
         animatedKeyboardState,
         animatedPosition,
         animatedSnapPoints,
+        animatedClosedPosition,
         isInTemporaryPosition,
         isAnimatedOnMount,
         keyboardBehavior,
@@ -931,9 +993,16 @@ const BottomSheetComponent = forwardRef<BottomSheet, BottomSheetProps>(
               undefined,
               animationConfigs
             );
+            // Only set isAnimatedOnMount to true after animation starts
+            if (source === ANIMATION_SOURCE.MOUNT) {
+              isAnimatedOnMount.value = true;
+            }
           } else {
             setToPosition(proposedPosition);
-            isAnimatedOnMount.value = true;
+            // Only set isAnimatedOnMount to true after position is set
+            if (source === ANIMATION_SOURCE.MOUNT) {
+              isAnimatedOnMount.value = true;
+            }
           }
           return;
         }

ryskin avatar Jan 09 '25 14:01 ryskin

I created a PR https://github.com/gorhom/react-native-bottom-sheet/pull/2108

Let me know if it helps! I’ve tried various approaches. There are several reasons for the issue, and I accidentally encountered a closing bug during the open animation in version 4.x.

ryskin avatar Jan 10 '25 04:01 ryskin

looking at the issue at the moment

gorhom avatar Feb 03 '25 17:02 gorhom

im not able to repo the issue,, could you someone provide a repo sample code base so i could investigate the issue ?

https://github.com/user-attachments/assets/939e7db2-b7d6-4b63-84bd-f9904543386f

gorhom avatar Feb 03 '25 17:02 gorhom

@gorhom I think this only occurs when enableDynamicSizing is on

michael-pingo-ai avatar May 20 '25 20:05 michael-pingo-ai

Okay i added the suggested patch here and it fixed this issue for me as well however - it seems to have broken the keyboard avoiding view functionality of text input components.

I can also attest to this being extremely hard to reproduce as I'm unable to reproduce it myself and have to rely on pushing updates and seeing if users report the behavior as fixed.

billyjacoby avatar Jun 17 '25 11:06 billyjacoby

Adding to this - i'm only able to reliably reproduce in release mode on iOS.

billyjacoby avatar Jun 17 '25 20:06 billyjacoby