Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Bug]: Changing FlatList data causes bottomsheet to close #2068

Open
geraintf opened this issue Dec 3, 2024 · 17 comments
Open

[Bug]: Changing FlatList data causes bottomsheet to close #2068

geraintf opened this issue Dec 3, 2024 · 17 comments
Labels
bug Something isn't working

Comments

@geraintf
Copy link

geraintf commented Dec 3, 2024

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 geraintf added the bug Something isn't working label Dec 3, 2024
@Simonas-Juska
Copy link

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

@isekovanic
Copy link

isekovanic commented Dec 9, 2024

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.

@r-obeen
Copy link

r-obeen commented Dec 10, 2024

Same issue here... on iOS too

@MianIbrarHussain
Copy link

same issue on ios...

@arsotech
Copy link

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 🙏

@Ben-WD
Copy link

Ben-WD commented Dec 11, 2024

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

Can we have a new release?

@arsotech
Copy link

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.

@isekovanic
Copy link

@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

@arsotech
Copy link

@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.

@ustuncem
Copy link

Same issue happening on iOS only. android is fine

@ustuncem
Copy link

ustuncem commented Dec 11, 2024

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.

@arsotech
Copy link

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

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

@r-obeen
Copy link

r-obeen commented Dec 11, 2024

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 !

@arsotech
Copy link

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

@ustuncem
Copy link

ustuncem commented Dec 11, 2024

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

@khanwilson
Copy link

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!

@MrNapcae
Copy link

MrNapcae commented Dec 14, 2024

Я думаю, проблема в том, как 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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

10 participants