From 92d81d7be858febf268b58cffbc6ca1eff0ec8c6 Mon Sep 17 00:00:00 2001 From: Abbey Campbell Date: Mon, 4 May 2026 11:40:10 -0700 Subject: [PATCH] Mob 1239 obs sheet not dismissing (#3576) * use bottom sheet api instead of returning null when hidden * dedupe inside modal dismiss logic * rename handleSnapPress -> openSheet * ts fixes and split render into two branches * fix integration test so we check for BottomSheet behavior rather than child absence * snapshot updates + comments * prevent hidden-state BottomSheet dismiss from triggering onPressClose * port changes from BottomSheet to BottomSheetV2 --- .../SharedComponents/Sheets/BottomSheet.tsx | 175 ++-- .../SharedComponents/Sheets/BottomSheetV2.tsx | 29 +- .../navigation/AddObsButton.test.js | 8 +- .../AddObsBottomSheet/AddObsButton.test.js | 4 +- .../__snapshots__/AddObsButton.test.js.snap | 888 ++++++++++++++++++ .../BottomTabNavigator/CustomTabBar.test.js | 4 +- .../__snapshots__/CustomTabBar.test.js.snap | 888 ++++++++++++++++++ 7 files changed, 1918 insertions(+), 78 deletions(-) diff --git a/src/components/SharedComponents/Sheets/BottomSheet.tsx b/src/components/SharedComponents/Sheets/BottomSheet.tsx index b3f3b9696..41e082cb4 100644 --- a/src/components/SharedComponents/Sheets/BottomSheet.tsx +++ b/src/components/SharedComponents/Sheets/BottomSheet.tsx @@ -1,10 +1,12 @@ +import type { + BottomSheetBackdropProps, +} from "@gorhom/bottom-sheet"; import BottomSheet, { BottomSheetModal, BottomSheetScrollView, } from "@gorhom/bottom-sheet"; import classnames from "classnames"; import { BottomSheetStandardBackdrop, Heading4, INatIconButton } from "components/SharedComponents"; import { View } from "components/styledComponents"; -import type { Node } from "react"; import React, { useCallback, useEffect, @@ -45,6 +47,8 @@ interface Props { enableContentPanningGesture?: boolean; } +type SheetHandle = BottomSheet & Partial>; + const StandardBottomSheet = ( { children, hidden, @@ -60,110 +64,147 @@ const StandardBottomSheet = ( { scrollEnabled = true, enablePanDownToClose = true, enableContentPanningGesture = true, -}: Props ): Node => { +}: Props ) => { if ( snapPoints ) { throw new Error( "BottomSheet does not accept snapPoints as a prop." ); } const { t } = useTranslation( ); - const sheetRef = useRef( null ); + const sheetRef = useRef( null ); + const skipNextOnPressCloseRef = useRef( false ); const insets = useSafeAreaInsets( ); - const handleClose = useCallback( ( ) => { - if ( onPressClose ) onPressClose( ); - + // The optional `sheet` arg lets the unmount cleanup pass a captured handle; + // see the cleanup effect below for why that matters. + const dismissSheet = useCallback( ( sheet = sheetRef.current ) => { if ( insideModal ) { - sheetRef.current?.collapse( ); + sheet?.close( ); } else { - sheetRef.current?.dismiss( ); + sheet?.dismiss?.( ); } - }, [insideModal, onPressClose] ); + }, [insideModal] ); - const renderBackdrop = props => ( + const handleClose = useCallback( ( ) => { + if ( skipNextOnPressCloseRef.current ) { + skipNextOnPressCloseRef.current = false; + } else if ( onPressClose ) { + onPressClose( ); + } + dismissSheet( ); + }, [dismissSheet, onPressClose] ); + + const renderBackdrop = ( props: BottomSheetBackdropProps ) => ( {} )} /> ); - const handleSnapPress = useCallback( ( ) => { + const openSheet = useCallback( ( ) => { if ( insideModal ) { sheetRef.current?.expand( ); } else { - sheetRef.current?.present( ); + sheetRef.current?.present?.( ); } }, [insideModal] ); useEffect( ( ) => { - if ( hidden ) { return; } - handleSnapPress( ); - }, [hidden, handleSnapPress] ); + if ( hidden ) { + skipNextOnPressCloseRef.current = true; + dismissSheet( ); + return; + } + skipNextOnPressCloseRef.current = false; + openSheet( ); + }, [hidden, openSheet, dismissSheet] ); - // To me, this implies this is a good candidate for splitting into 2 components - const BottomSheetComponent = insideModal - ? BottomSheet - : BottomSheetModal; + // Capture sheetRef.current now: it's null by the time this cleanup runs on unmount, + // so the captured handle is what actually dismisses the sheet. + useEffect( ( ) => { + const sheet = sheetRef.current; + return ( ) => dismissSheet( sheet ); + }, [dismissSheet] ); - if ( hidden ) { - return null; + const content = ( + + 0 + ? "pb-7" + : null, + containerClass, + )} + onLayout={onLayout} + // Not ideal, but @gorhom/bottom-sheet components don't support + // testID + testID={testID} + > + {!headerText + ? null + : ( + + + {headerText} + + + )} + {children} + {!hideCloseButton && ( + + )} + + + ); + + // Consider splitting into separate files/components or removing `insideModal` usage + if ( insideModal ) { + return ( + + {content} + + ); } return ( - } style={marginOnWide} accessible={false} onDismiss={handleClose} enablePanDownToClose={enablePanDownToClose} enableContentPanningGesture={enableContentPanningGesture} > - - 0 - ? "pb-7" - : null, - containerClass, - )} - onLayout={onLayout} - // Not ideal, but @gorhom/bottom-sheet components don't support - // testID - testID={testID} - > - {!headerText - ? null - : ( - - - {headerText} - - - )} - {children} - {!hideCloseButton && ( - - - + {content} + ); }; diff --git a/src/components/SharedComponents/Sheets/BottomSheetV2.tsx b/src/components/SharedComponents/Sheets/BottomSheetV2.tsx index 02d4dc3b5..34fd5ed71 100644 --- a/src/components/SharedComponents/Sheets/BottomSheetV2.tsx +++ b/src/components/SharedComponents/Sheets/BottomSheetV2.tsx @@ -51,6 +51,7 @@ const BottomSheetV2 = ( { }: Props ): React.JSX.Element | null => { const { t } = useTranslation( ); const sheetRef = useRef( null ); + const skipNextOnDismissRef = useRef( false ); const insets = useSafeAreaInsets( ); const handlePressClose = useCallback( ( ) => { @@ -59,6 +60,16 @@ const BottomSheetV2 = ( { } }, [onPressClose] ); + const handleDismiss = useCallback( ( ) => { + if ( skipNextOnDismissRef.current ) { + skipNextOnDismissRef.current = false; + return; + } + if ( onDismiss ) { + onDismiss( ); + } + }, [onDismiss] ); + const renderBackdrop = ( props: BottomSheetBackdropProps ) => ( { - if ( hidden ) { return; } + if ( hidden ) { + skipNextOnDismissRef.current = true; + sheetRef.current?.dismiss( ); + return; + } + skipNextOnDismissRef.current = false; sheetRef.current?.present( ); }, [hidden] ); - if ( hidden ) { - return null; - } + useEffect( ( ) => { + const sheet = sheetRef.current; + return ( ) => { + sheet?.dismiss( ); + }; + }, [] ); return ( { } ); it( "does not open model on long press", async ( ) => { + const presentSpy = jest.spyOn( BottomSheetModal.prototype, "present" ); renderComponent( ); await longPress( ); - const noEvidenceButton = screen.queryByLabelText( - i18next.t( "Observation-with-no-evidence" ), - ); - expect( noEvidenceButton ).toBeFalsy( ); + expect( presentSpy ).not.toHaveBeenCalled( ); + presentSpy.mockRestore( ); } ); describe( "with advanced AICamera-only setting", ( ) => { diff --git a/tests/unit/components/AddObsBottomSheet/AddObsButton.test.js b/tests/unit/components/AddObsBottomSheet/AddObsButton.test.js index 022719550..8aac9b3ae 100644 --- a/tests/unit/components/AddObsBottomSheet/AddObsButton.test.js +++ b/tests/unit/components/AddObsBottomSheet/AddObsButton.test.js @@ -30,7 +30,9 @@ describe( "AddObsButton", () => { it( "renders correctly", () => { renderComponent( ); - // Snapshot test + // Snapshot includes AddObs BottomSheet subtree due @gorhom/bottom-sheet test mock + // rendering children unconditionally. Hidden/open behavior is asserted in + // integration tests. expect( screen ).toMatchSnapshot(); } ); it( "does not render tooltip in default state", () => { diff --git a/tests/unit/components/AddObsBottomSheet/__snapshots__/AddObsButton.test.js.snap b/tests/unit/components/AddObsBottomSheet/__snapshots__/AddObsButton.test.js.snap index d30374464..6294c3917 100644 --- a/tests/unit/components/AddObsBottomSheet/__snapshots__/AddObsButton.test.js.snap +++ b/tests/unit/components/AddObsBottomSheet/__snapshots__/AddObsButton.test.js.snap @@ -18,6 +18,894 @@ exports[`AddObsButton renders correctly 1`] = ` } } > + + + + + + + + + +  + + + + + Take photos + + + + + + + + +  + + + + + Record a sound + + + + + + +  + + + + + Upload photos + + + + + + +  + + + + Create observation with no evidence + + + + + + { it( "should render correctly", async () => { renderComponent( ); - + // Snapshot includes AddObs BottomSheet subtree due @gorhom/bottom-sheet test mock + // rendering children unconditionally. Hidden/open behavior is asserted in + // integration tests. await expect( screen ).toMatchSnapshot(); } ); diff --git a/tests/unit/components/BottomTabNavigator/__snapshots__/CustomTabBar.test.js.snap b/tests/unit/components/BottomTabNavigator/__snapshots__/CustomTabBar.test.js.snap index 214492cf4..14355cf8e 100644 --- a/tests/unit/components/BottomTabNavigator/__snapshots__/CustomTabBar.test.js.snap +++ b/tests/unit/components/BottomTabNavigator/__snapshots__/CustomTabBar.test.js.snap @@ -355,6 +355,894 @@ exports[`CustomTabBar with advanced user layout should render correctly 1`] = ` ] } > + + + + + + + + + +  + + + + + Take photos + + + + + + + + +  + + + + + Record a sound + + + + + + +  + + + + + Upload photos + + + + + + +  + + + + Create observation with no evidence + + + + + +