fix osm data warning and fix infinite loop

maproulette · Feb 15, 2025 · c14736a · c14736a
1 parent 3eceec6
commit c14736a
Show file tree

Hide file tree

Showing 4 changed files with 91 additions and 28 deletions.
diff --git a/src/components/EnhancedMap/OSMDataLayer/OSMDataLayer.jsx b/src/components/EnhancedMap/OSMDataLayer/OSMDataLayer.jsx
@@ -83,6 +83,19 @@ const generateLayer = (props, map, _leaflet) => {
     pane: props.leaflet?.pane,
   });
 
+  // Clear existing layers to ensure proper ordering
+  layerGroup.clearLayers();
+
+  // Get all features and sort them by type to ensure consistent ordering
+  const features = layerGroup.buildFeatures(props.xmlData);
+  const sortedFeatures = features.sort((a, b) => {
+    const typeOrder = { way: 1, area: 2, node: 3, changeset: 4 };
+    return typeOrder[a.type] - typeOrder[b.type];
+  });
+
+  // Add features in the sorted order
+  layerGroup.addData(sortedFeatures);
+
   layerGroup.eachLayer((layer) => {
     layer.options.mrLayerLabel = props.intl.formatMessage(layerMessages.showOSMDataLabel);
     layer.options.fill = false;
@@ -166,7 +179,8 @@ const OSMDataLayerComponent = createPathComponent(
     if (!_isEqual(prevProps.showOSMElements, props.showOSMElements)) {
       instance.clearLayers();
       const newLayers = generateLayer(props, instance);
-      newLayers.eachLayer((layer) => instance.addLayer(layer));
+      // Transfer all layers at once to maintain ordering
+      instance.addLayer(newLayers);
     }
   },
 );

diff --git a/src/components/HOCs/WithCurrentTask/WithCurrentTask.jsx b/src/components/HOCs/WithCurrentTask/WithCurrentTask.jsx
@@ -324,6 +324,7 @@ export const mapDispatchToProps = (dispatch, ownProps) => {
     fetchOSMData: (bbox) => {
       return fetchOSMData(bbox).catch((error) => {
         dispatch(addError(error));
+        throw error;
       });
     },
 

diff --git a/src/components/TaskPane/TaskMap/Messages.js b/src/components/TaskPane/TaskMap/Messages.js
@@ -8,4 +8,21 @@ export default defineMessages({
     id: "Map.layerSelectionList.header",
     defaultMessage: "Select Desired Feature",
   },
+  osmDataAreaTooLarge: {
+    id: "Task.osmData.areaTooLarge",
+    defaultMessage:
+      "The selected area is too large to load OSM data. Please zoom in further to view OSM features.",
+  },
+  osmDataTooLarge: {
+    id: "Task.map.osmData.tooLarge",
+    defaultMessage: "OSM Data Area Too Large",
+  },
+  zoomInForOSMData: {
+    id: "Task.map.osmData.zoomInRequired",
+    defaultMessage: "Please zoom in closer to view OSM data for this area",
+  },
+  osmDataError: {
+    id: "Task.map.osmData.error",
+    defaultMessage: "Error Loading OSM Data",
+  },
 });
diff --git a/src/components/TaskPane/TaskMap/TaskMap.jsx b/src/components/TaskPane/TaskMap/TaskMap.jsx
@@ -79,7 +79,11 @@ export const TaskMapContent = (props) => {
   const [showTaskFeatures, setShowTaskFeatures] = useState(true);
   const [osmData, setOsmData] = useState(null);
   const [showOSMData, setShowOSMData] = useState(false);
-  const [showOSMElements, setShowOSMElements] = useState({ nodes: true, ways: true, areas: true });
+  const [showOSMElements, setShowOSMElements] = useState({
+    nodes: true,
+    ways: true,
+    areas: true,
+  });
   const [osmDataLoading, setOsmDataLoading] = useState(false);
   const [mapillaryViewerImage, setMapillaryViewerImage] = useState(null);
   const [openStreetCamViewerImage, setOpenStreetCamViewerImage] = useState(null);
@@ -264,43 +268,62 @@ export const TaskMapContent = (props) => {
     setShowTaskFeatures((prevState) => !prevState);
   };
 
-  const fetchOSMData = () => {
-    setOsmDataLoading(true);
-    props.fetchOSMData(map.getBounds().toBBoxString()).then((xmlData) => {
-      // Indicate the map should skip fitting to bounds as the OSM data could
-      // extend beyond the current view and we don't want the map to zoom out
-      setOsmData(xmlData);
+  const fetchOSMData = async () => {
+    try {
+      if (showOSMData) {
+        const bounds = map.getBounds()?.toBBoxString();
+        if (!bounds) {
+          throw new Error("Invalid map bounds");
+        }
+
+        const xmlData = await props.fetchOSMData(bounds);
+        setOsmData(xmlData);
+        setShowOSMData(true);
+      } else {
+        setOsmData(null);
+        setShowOSMData(false);
+      }
+    } catch (error) {
+      console.error("Error handling OSM data:", error);
+      setOsmData(null);
+      setShowOSMData(false);
+    } finally {
       setOsmDataLoading(false);
-    });
+    }
   };
 
   /**
    * Invoked by LayerToggle when the user wishes to toggle visibility of
    * OSM data on or off.
    */
-  const toggleOSMDataVisibility = () => {
+  const toggleOSMDataVisibility = async () => {
+    // Prevent multiple requests while loading
     if (osmDataLoading) {
       return;
     }
 
     const loadOSMData = !showOSMData;
     setOsmDataLoading(true);
-    setShowOSMData(loadOSMData);
-
-    if (loadOSMData) {
-      props
-        .fetchOSMData(map.getBounds().toBBoxString())
-        .then((xmlData) => {
-          setOsmData(xmlData);
-        })
-        .catch((error) => {
-          console.error("Error fetching OSM data:", error);
-        })
-        .finally(() => {
-          setOsmDataLoading(false);
-        });
-    } else {
+
+    try {
+      if (loadOSMData) {
+        const bounds = map.getBounds()?.toBBoxString();
+        if (!bounds) {
+          throw new Error("Invalid map bounds");
+        }
+
+        const xmlData = await props.fetchOSMData(bounds);
+        setOsmData(xmlData);
+        setShowOSMData(true);
+      } else {
+        setOsmData(null);
+        setShowOSMData(false);
+      }
+    } catch (error) {
+      console.error("Error handling OSM data:", error);
       setOsmData(null);
+      setShowOSMData(false);
+    } finally {
       setOsmDataLoading(false);
     }
   };
@@ -426,7 +449,7 @@ export const TaskMapContent = (props) => {
     if (showOSMData && !osmData) {
       fetchOSMData();
     }
-  }, [props, osmData]);
+  }, [showOSMData, osmData]);
 
   useEffect(() => {
     if (features.length !== 0) {
@@ -625,7 +648,11 @@ export const TaskMapContent = (props) => {
   }
 
   return (
-    <div className={classNames("task-map task", { "full-screen-map": props.isMobile })}>
+    <div
+      className={classNames("task-map task", {
+        "full-screen-map": props.isMobile,
+      })}
+    >
       <LayerToggle
         {...props}
         showTaskFeatures={showTaskFeatures}
@@ -681,7 +708,11 @@ const TaskMap = (props) => {
   };
 
   return (
-    <div className={classNames("task-map task", { "full-screen-map": props.isMobile })}>
+    <div
+      className={classNames("task-map task", {
+        "full-screen-map": props.isMobile,
+      })}
+    >
       <MapContainer
         taskBundle={props.taskBundle}
         center={props.centerPoint}