fix: callouts no longer crash when selection is filtered out

add helper functions to callout system to make initializing state more concise and mapping state to logic more consistent; as a result, if the user's selection is filtered out, the app gracefully hides the callout rather than crashing. further refactor SiteMapCallout for general clarity.
techmatters · Apr 11, 2024 · 7002670 · 7002670
1 parent 70c47d8
commit 7002670
Show file tree

Hide file tree

Showing 5 changed files with 169 additions and 81 deletions.
diff --git a/dev-client/src/screens/HomeScreen/HomeScreen.tsx b/dev-client/src/screens/HomeScreen/HomeScreen.tsx
@@ -48,7 +48,12 @@ import {selectSitesAndUserRoles} from 'terraso-client-shared/selectors';
 import {ListFilterProvider} from 'terraso-mobile-client/components/ListFilter';
 import {useGeospatialContext} from 'terraso-mobile-client/context/GeospatialContext';
 import {Box} from 'terraso-mobile-client/components/NativeBaseAdapters';
-import {CalloutState} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
+import {
+  CalloutState,
+  noneCallout,
+  siteCallout,
+  locationCallout,
+} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
 
 type HomeScreenRef = {
   showSiteOnMap: (site: Site) => void;
@@ -71,9 +76,7 @@ export const HomeScreen = memo(() => {
   const infoBottomSheetRef = useRef<BottomSheetModal>(null);
   const siteListBottomSheetRef = useRef<BottomSheet>(null);
   const [mapStyleURL, setMapStyleURL] = useState(Mapbox.StyleURL.Street);
-  const [calloutState, setCalloutState] = useState<CalloutState>({
-    kind: 'none',
-  });
+  const [calloutState, setCalloutState] = useState<CalloutState>(noneCallout());
   const currentUserID = useSelector(
     state => state.account.currentUser?.data?.id,
   );
@@ -87,7 +90,7 @@ export const HomeScreen = memo(() => {
   const showSiteOnMap = useCallback(
     (targetSite: Site) => {
       mapRef.current?.moveToPoint(targetSite);
-      setCalloutState({kind: 'site', siteId: targetSite.id});
+      setCalloutState(siteCallout(targetSite.id));
       siteListBottomSheetRef.current?.collapse();
     },
     [setCalloutState],
@@ -130,7 +133,7 @@ export const HomeScreen = memo(() => {
 
   const searchFunction = useCallback(
     (coords: Coords) => {
-      setCalloutState({kind: 'location', coords});
+      setCalloutState(locationCallout(coords));
       mapRef.current?.moveToPoint(coords);
     },
     [setCalloutState, mapRef],

diff --git a/dev-client/src/screens/HomeScreen/HomeScreenCallout.ts b/dev-client/src/screens/HomeScreen/HomeScreenCallout.ts
@@ -15,22 +15,83 @@
  * along with state program. If not, see https://www.gnu.org/licenses/.
  */
 
-import { Coords } from 'terraso-mobile-client/model/map/mapSlice';
-
+import {Site} from 'terraso-client-shared/site/siteSlice';
+import {Coords} from 'terraso-mobile-client/model/map/mapSlice';
 
 export type CalloutState =
   | {
-    kind: 'site';
-    siteId: string;
-  }
+      kind: 'site';
+      siteId: string;
+    }
   | {
-    kind: 'location';
-    coords: Coords;
-  }
+      kind: 'location';
+      coords: Coords;
+    }
   | {
-    kind: 'site_cluster';
-    siteIds: string[];
-    coords: Coords;
+      kind: 'site_cluster';
+      siteIds: string[];
+      coords: Coords;
+    }
+  | {kind: 'none'};
+
+export function siteCallout(siteId: string): CalloutState {
+  return {kind: 'site', siteId};
+}
+
+export function siteClusterCallout(
+  coords: Coords,
+  siteIds: Iterable<string>,
+): CalloutState {
+  return {kind: 'site_cluster', coords, siteIds: Array.from(siteIds)};
+}
+
+export function locationCallout(coords: Coords): CalloutState {
+  return {kind: 'location', coords};
+}
+
+export function noneCallout(): CalloutState {
+  return {kind: 'none'};
+}
+
+export function getCalloutCoords(
+  state: CalloutState,
+  sites: Record<string, Site>,
+): Coords | null {
+  switch (state.kind) {
+    case 'site':
+      return getCalloutSite(state, sites);
+    case 'location':
+      return state.coords;
+    case 'site_cluster':
+      return state.coords;
+    case 'none':
+      return null;
   }
-  | { kind: 'none' };
+}
 
+export function getCalloutSite(
+  state: CalloutState,
+  sites: Record<string, Site>,
+): Site | null {
+  return state.kind === 'site' && state.siteId in sites
+    ? sites[state.siteId]
+    : null;
+}
+
+export function getCalloutSites(
+  state: CalloutState,
+  sites: Record<string, Site>,
+): Record<string, Site> {
+  switch (state.kind) {
+    case 'site':
+      const site = getCalloutSite(state, sites);
+      return site == null ? {} : Object.fromEntries([[state.siteId, site]]);
+    case 'site_cluster':
+      const presentSiteIds = state.siteIds.filter(siteId => siteId in sites);
+      return Object.fromEntries(
+        presentSiteIds.map(siteId => [siteId, sites[siteId]]),
+      );
+    default:
+      return {};
+  }
+}
diff --git a/dev-client/src/screens/HomeScreen/components/SiteClusterCalloutListItem.tsx b/dev-client/src/screens/HomeScreen/components/SiteClusterCalloutListItem.tsx
@@ -18,7 +18,10 @@
 import {useCallback} from 'react';
 import {Pressable} from 'react-native';
 import {Site} from 'terraso-client-shared/site/siteSlice';
-import {CalloutState} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
+import {
+  CalloutState,
+  siteCallout,
+} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
 import {useSelector} from 'terraso-mobile-client/store';
 import {
   Column,
@@ -41,7 +44,7 @@ export const SiteClusterCalloutListItem = ({
       : state.project.projects[site.projectId],
   );
   const onPress = useCallback(() => {
-    setState({kind: 'site', siteId: site.id});
+    setState(siteCallout(site.id));
   }, [site.id, setState]);
 
   return (

diff --git a/dev-client/src/screens/HomeScreen/components/SiteMap.tsx b/dev-client/src/screens/HomeScreen/components/SiteMap.tsx
@@ -28,7 +28,14 @@ import {
 import {useTheme} from 'native-base';
 import {Keyboard, PixelRatio, StyleSheet} from 'react-native';
 import {Site} from 'terraso-client-shared/site/siteSlice';
-import {CalloutState} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
+import {
+  CalloutState,
+  noneCallout,
+  siteCallout,
+  locationCallout,
+  siteClusterCallout,
+  getCalloutSite,
+} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
 import {
   coordsToPosition,
   positionToCoords,
@@ -82,13 +89,10 @@ export const SiteMap = memo(
       }));
 
       const {filteredItems: filteredSites} = useListFilter<Site>();
-
       const sites = Object.fromEntries(
         filteredSites.map(site => [site.id, site]),
       );
-
-      const selectedSite =
-        calloutState.kind === 'site' ? sites[calloutState.siteId] : null;
+      const selectedSite = getCalloutSite(calloutState, sites);
 
       const {colors} = useTheme();
 
@@ -145,13 +149,14 @@ export const SiteMap = memo(
               0,
             )) as GeoJSON.FeatureCollection;
 
-            setCalloutState({
-              kind: 'site_cluster',
-              coords: positionToCoords(
-                (feature.geometry as GeoJSON.Point).coordinates,
+            setCalloutState(
+              siteClusterCallout(
+                positionToCoords(
+                  (feature.geometry as GeoJSON.Point).coordinates,
+                ),
+                leafFeatures.features.map(feat => feat.id as string),
               ),
-              siteIds: leafFeatures.features.map(feat => feat.id as string),
-            });
+            );
           }
         },
         [shapeSourceRef, setCalloutState],
@@ -180,17 +185,12 @@ export const SiteMap = memo(
               paddingBottom: 0,
               cameraRef: cameraRef,
             });
-            setCalloutState({kind: 'site', siteId: feature.id as string});
+            setCalloutState(siteCallout(feature.id as string));
           }
         },
         [setCalloutState, handleClusterPress],
       );
 
-      const onTempSitePress = useCallback(
-        () => setCalloutState(calloutState),
-        [calloutState, setCalloutState],
-      );
-
       const onPress = useCallback(
         async (feature: GeoJSON.Feature) => {
           if (
@@ -210,13 +210,12 @@ export const SiteMap = memo(
               paddingBottom: 320,
               cameraRef: cameraRef,
             });
-            setCalloutState({
-              kind: 'location',
-              coords: positionToCoords(feature.geometry.coordinates),
-            });
+            setCalloutState(
+              locationCallout(positionToCoords(feature.geometry.coordinates)),
+            );
           } else {
             Keyboard.dismiss();
-            setCalloutState({kind: 'none'});
+            setCalloutState(noneCallout());
           }
         },
         [calloutState, setCalloutState],
@@ -313,8 +312,7 @@ export const SiteMap = memo(
           </Mapbox.ShapeSource>
           <Mapbox.ShapeSource
             id="temporarySitesSource"
-            shape={temporarySitesFeature}
-            onPress={onTempSitePress}>
+            shape={temporarySitesFeature}>
             <Mapbox.SymbolLayer
               id="temporarySitesLayer"
               style={mapStyles.temporarySiteLayer}

diff --git a/dev-client/src/screens/HomeScreen/components/SiteMapCallout.tsx b/dev-client/src/screens/HomeScreen/components/SiteMapCallout.tsx
@@ -19,13 +19,20 @@ import React, {useCallback} from 'react';
 import Mapbox from '@rnmapbox/maps';
 import {Divider, FlatList} from 'native-base';
 import {Site} from 'terraso-client-shared/site/siteSlice';
-import {CalloutState} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
+import {
+  CalloutState,
+  noneCallout,
+  getCalloutCoords,
+  getCalloutSite,
+  getCalloutSites,
+} from 'terraso-mobile-client/screens/HomeScreen/HomeScreenCallout';
 import {coordsToPosition} from 'terraso-mobile-client/components/StaticMapView';
 import {Card} from 'terraso-mobile-client/components/Card';
 import {CardCloseButton} from 'terraso-mobile-client/components/CardCloseButton';
 import {SiteCard} from 'terraso-mobile-client/components/SiteCard';
 import {SiteClusterCalloutListItem} from 'terraso-mobile-client/screens/HomeScreen/components/SiteClusterCalloutListItem';
 import {TemporarySiteCallout} from 'terraso-mobile-client/screens/HomeScreen/components/TemporarySiteCallout';
+import {Coords} from 'terraso-mobile-client/model/map/mapSlice';
 
 type Props = {
   sites: Record<string, Site>;
@@ -34,45 +41,13 @@ type Props = {
 };
 
 export const SiteMapCallout = ({sites, state, setState}: Props) => {
-  const closeCallout = useCallback(() => setState({kind: 'none'}), [setState]);
-
-  if (state.kind === 'none') {
+  const coords = getCalloutCoords(state, sites);
+  if (!coords) {
     return null;
   }
 
-  const coords = state.kind === 'site' ? sites[state.siteId] : state.coords;
-
-  let child: React.ComponentProps<typeof Mapbox.MarkerView>['children'];
-
-  if (state.kind === 'site') {
-    child = (
-      <SiteCard
-        site={sites[state.siteId]}
-        buttons={<CardCloseButton onPress={closeCallout} />}
-        isPopover={true}
-      />
-    );
-  } else if (state.kind === 'site_cluster') {
-    child = (
-      <Card
-        width="270px"
-        buttons={<CardCloseButton onPress={closeCallout} />}
-        isPopover={true}>
-        <FlatList
-          data={state.siteIds}
-          keyExtractor={id => id}
-          renderItem={({item: id}) => (
-            <SiteClusterCalloutListItem site={sites[id]} setState={setState} />
-          )}
-          ItemSeparatorComponent={() => <Divider my="10px" />}
-        />
-      </Card>
-    );
-  } else if (state.kind === 'location') {
-    child = (
-      <TemporarySiteCallout coords={coords} closeCallout={closeCallout} />
-    );
-  } else {
+  let child = CalloutChild(coords, {sites, state, setState});
+  if (!child) {
     return null;
   }
 
@@ -85,3 +60,51 @@ export const SiteMapCallout = ({sites, state, setState}: Props) => {
     </Mapbox.MarkerView>
   );
 };
+
+const CalloutChild = (coords: Coords, {sites, state, setState}: Props) => {
+  const closeCallout = useCallback(() => setState(noneCallout()), [setState]);
+
+  switch (state.kind) {
+    case 'site':
+      const site = getCalloutSite(state, sites);
+      if (!site) {
+        return null;
+      }
+
+      return (
+        <SiteCard
+          site={site}
+          buttons={<CardCloseButton onPress={closeCallout} />}
+          isPopover={true}
+        />
+      );
+    case 'site_cluster':
+      const clusterSites = getCalloutSites(state, sites);
+      if (!clusterSites) {
+        return null;
+      }
+
+      return (
+        <Card
+          width="270px"
+          buttons={<CardCloseButton onPress={closeCallout} />}
+          isPopover={true}>
+          <FlatList
+            data={Object.keys(clusterSites)}
+            keyExtractor={id => id}
+            renderItem={({item: id}) => (
+              <SiteClusterCalloutListItem
+                site={clusterSites[id]}
+                setState={setState}
+              />
+            )}
+            ItemSeparatorComponent={() => <Divider my="10px" />}
+          />
+        </Card>
+      );
+    default:
+      return (
+        <TemporarySiteCallout coords={coords} closeCallout={closeCallout} />
+      );
+  }
+};