[fix] Address coderabbit comments and fix tests

Author: dee077

openwisp_monitoring/device/static/monitoring/css/device-map.css

@@ -192,7 +192,7 @@
 .table-container.is-loading-append .table-spinner {
   top: var(--table-spinner-top) !important;
   left: 50% !important;
-  transform: translateY(-50%);
+  transform: translate(-50%, -50%);
 }
 .no-devices {
   font-weight: bold;

openwisp_monitoring/device/static/monitoring/js/device-map.js

@@ -121,30 +121,30 @@
       }
 
       popupContent = `
-          <div class="map-detail">
-            <h2>${escapeHtml(popupTitle)} (${data.count})</h2>
-            <div class="input-container">
-              <input id="device-search" placeholder="${gettext("Search for devices")}" />
-            </div>
-            <div class="label-container">
-              ${statusFilterButtons}
-              <input id="status-filter" type="hidden" />
-            </div>
-            <div class="table-container">
-              <table>
-                <thead>
-                  <tr>
-                    <th>${gettext("name")}</th>
-                    <th class="th-status"><span class ="health-status-heading">${gettext("status")}</span></th>
-                  </tr>
-                </thead>
-                <tbody>${renderRows(netjsongraphInstance)}</tbody>
-              </table>
-              <div class="ow-loading-spinner table-spinner"></div>
-            </div>
-            ${floorplan_btn}
-          </div>
-        `;
+           <div class="map-detail">
+             <h2>${escapeHtml(popupTitle)} (${data.count})</h2>
+             <div class="input-container">
+               <input id="device-search" placeholder="${gettext("Search for devices")}" />
+             </div>
+             <div class="label-container">
+               ${statusFilterButtons}
+               <input id="status-filter" type="hidden" />
+             </div>
+             <div class="table-container">
+               <table>
+                 <thead>
+                   <tr>
+                     <th>${gettext("name")}</th>
+                     <th class="th-status"><span class ="health-status-heading">${gettext("status")}</span></th>
+                   </tr>
+                 </thead>
+                 <tbody>${renderRows(devices)}</tbody>
+               </table>
+               <div class="ow-loading-spinner table-spinner"></div>
+             </div>
+             ${floorplan_btn}
+           </div>
+         `;
       loadingOverlay.hide();
       return popupContent;
     } catch (error) {
@@ -155,9 +155,7 @@
     }
   }
 
-  function renderRows(netjsongraphInstance, deviceList) {
-    deviceList = deviceList || netjsongraphInstance.leaflet._popupState.devices;
-    const popup = $(".map-detail");
+  function renderRows(deviceList) {
     if (deviceList.length === 0) {
       const emptyRow = `
         <tr>
@@ -166,7 +164,6 @@
           </td>
         </tr>
       `;
-      popup.find("tbody").html(emptyRow);
       return emptyRow;
     }
     const rows = deviceList
@@ -183,7 +180,6 @@
     `,
       )
       .join("");
-    popup.find("tbody").html(rows);
     return rows;
   }
 
@@ -198,10 +194,15 @@
       netjsongraphInstance?.leaflet?._popupState;
     const el = $(currentPopup.getElement());
     let fetchDevicesTimeout;
+    let activeRequest = null;
     let loading = false;
     function fetchDevices(url, ms = 0, append) {
-      if (!url || loading) return;
+      if (!url) return;
+      if (append && loading) return;
       clearTimeout(fetchDevicesTimeout);
+      if (!append && activeRequest) {
+        activeRequest.abort();
+      }
       loading = true;
       const container = el.find(".table-container");
       const spinner = el.find(".table-spinner");
@@ -239,7 +240,7 @@
         } else {
           fetchUrl = queryString ? `${url}?${queryString}` : url;
         }
-        $.ajax({
+        activeRequest = $.ajax({
           dataType: "json",
           url: fetchUrl,
           xhrFields: { withCredentials: true },
@@ -250,14 +251,16 @@
               devices = data.results;
             }
             nextUrl = data.next;
-            renderRows(netjsongraphInstance, devices);
+            el.find("tbody").html(renderRows(devices));
             if (!append) container.scrollTop(0);
           },
-          error() {
+          error(_jqXHR, textStatus) {
+            if (textStatus === "abort") return;
             console.error(gettext("Could not load more devices from"), url);
             alert(gettext("Could not load more devices."));
           },
           complete() {
+            activeRequest = null;
             loading = false;
             spinner.hide();
             container.removeClass("is-loading");
@@ -284,7 +287,7 @@
         btn.addClass("active");
         activeStatuses.push(status);
       }
-      $(`#status-filter`).val(activeStatuses.join(","));
+      el.find("#status-filter").val(activeStatuses.join(","));
       fetchDevices(url);
     });
     el.find(".table-container").on("scroll", function () {
@@ -297,6 +300,8 @@
       window.openFloorPlan(floorplanUrl, locationId);
     });
     currentPopup.on("remove", () => {
+      clearTimeout(fetchDevicesTimeout);
+      activeRequest?.abort();
       netjsongraphInstance.leaflet._popupState = null;
     });
   }
@@ -441,7 +446,6 @@
           map.leaflet.addControl(new L.control.scale(scale));
         }
 
-
         try {
           const features = (map.data && map.data.features) || [];
           if (features.length) {

openwisp_monitoring/device/static/monitoring/js/floorplan.js

@@ -31,7 +31,7 @@
       selectedIndex: 0,
       isFullScreen: false,
       // Used to ignore late ajax responses from older sessions.
-      _sessionId: `${Date.now()}_${Math.random()}`,
+      _sessionId: window.crypto.randomUUID(),
       popstateHandler: null,
       hashchangeHandler: null,
     };
@@ -140,7 +140,10 @@
     setFloorplanState(floorplanState);
   }
 
-  async function openFloorPlan(url, id = null, floor = null) {
+  async function openFloorPlan(url, id, floor = null) {
+    if (id == null) {
+      throw new Error("openFloorPlan requires a locationId");
+    }
     if (document.getElementById("floorplan-overlay")) {
       destroyFloorplan();
     }
@@ -290,7 +293,27 @@
       floorplanState.maps[floorplanState.state.currentFloor].utils.removeUrlFragment(
         id,
       );
+      return;
     }
+
+    // Fallback: if the overlay is closed before the indoor map instance is
+    // initialized, ensure any indoor fragment is removed from the URL so a
+    // reload/back-navigation doesn't reopen the overlay unexpectedly.
+    const raw = window.location.hash.replace(/^#/, "");
+    if (!raw) return;
+    const fragments = decodeURIComponent(raw)
+      .split(";")
+      .map((f) => f.trim())
+      .filter(Boolean);
+    if (!fragments.length) return;
+    const kept = fragments.filter((fragment) => {
+      const params = new URLSearchParams(fragment);
+      const id = params.get("id");
+      return id === "dashboard-geo-map";
+    });
+    const nextHash = kept.length ? `#${encodeURIComponent(kept.join(";"))}` : "";
+    const nextUrl = `${window.location.pathname}${window.location.search}${nextHash}`;
+    window.history.replaceState(null, "", nextUrl);
   }
 
   function addFloorButtons() {
@@ -476,6 +499,9 @@
         async onReady() {
           const floorplanState = getFloorplanState();
           if (!floorplanState?.state) return;
+          // Guard against stale async continuation if the overlay is closed or a
+          // newer floorplan session replaces the current one while awaiting.
+          const sessionId = floorplanState._sessionId;
           const map = this.leaflet;
           floorplanState.maps[floor] = indoorMap;
           setFloorplanState(floorplanState);
@@ -491,6 +517,16 @@
             $(".floorplan-loading-spinner").hide();
             return;
           }
+
+          const latestState = getFloorplanState();
+          if (
+            !latestState?.state ||
+            latestState._sessionId !== sessionId ||
+            latestState.maps[floor] !== indoorMap
+          ) {
+            // Don't touch map/echarts/DOM: this continuation is stale.
+            return;
+          }
           let initialZoom;
           const h = img.height;
           const w = h * (img.width / img.height);

openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.js

@@ -322,6 +322,7 @@ class TestDashboardMap(
     floorplan_model = Floorplan
     object_location_model = DeviceLocation
     config_app_label = "config"
+    retry_max = 1
 
     def _open_popup(self, mapType, id):
         self.web_driver.execute_script(
@@ -330,6 +331,27 @@ def _open_popup(self, mapType, id):
             str(id),
         )
 
+    def _wait_for_popup_table_ready(self, timeout=5):
+        # The popup keeps the table mounted during replace/append fetches, so
+        # these loading-state classes are the most reliable signal that row
+        # updates have finished.
+        try:
+            WebDriverWait(self.web_driver, timeout).until(
+                lambda d: all(
+                    cls
+                    not in (
+                        d.find_element(
+                            By.CSS_SELECTOR, ".map-detail .table-container"
+                        ).get_attribute("class")
+                        or ""
+                    )
+                    for cls in ("is-loading", "is-loading-append")
+                )
+            )
+        except TimeoutException as e:
+            print(self.get_browser_logs())
+            self.fail(f"Popup table did not finish loading within {timeout}s: {e}")
+
     def test_features_on_device_popup(self):
         org = self._get_org()
         d1 = self._create_device(
@@ -368,9 +390,7 @@ def test_features_on_device_popup(self):
             self.assertFalse(status_ok_close_btn.is_displayed())
             status_ok.click()
             self.assertTrue(status_ok_close_btn.is_displayed())
-            self.wait_for_invisibility(
-                By.CSS_SELECTOR, ".map-detail .ow-loading-spinner"
-            )
+            self._wait_for_popup_table_ready()
             table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
             self.assertEqual(len(table_entries), 1)
             self.assertIn(d2.name, table_entries[0].text)
@@ -385,35 +405,27 @@ def test_features_on_device_popup(self):
             self.assertFalse(status_unknown_close_btn.is_displayed())
             status_unknown.click()
             self.assertTrue(status_unknown_close_btn.is_displayed())
-            self.wait_for_invisibility(
-                By.CSS_SELECTOR, ".map-detail .ow-loading-spinner"
-            )
+            self._wait_for_popup_table_ready()
             table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
             self.assertEqual(len(table_entries), 2)
 
         with self.subTest("Test removing filters by clicking close button"):
             status_ok_close_btn.click()
             self.assertFalse(status_ok_close_btn.is_displayed())
-            self.wait_for_invisibility(
-                By.CSS_SELECTOR, ".map-detail .ow-loading-spinner", timeout=5
-            )
+            self._wait_for_popup_table_ready()
             table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
             self.assertEqual(len(table_entries), 1)
             self.assertIn(d1.name, table_entries[0].text)
             status_unknown.click()
             self.assertFalse(status_unknown_close_btn.is_displayed())
-            self.wait_for_invisibility(
-                By.CSS_SELECTOR, ".map-detail .ow-loading-spinner"
-            )
+            self._wait_for_popup_table_ready()
             table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
             self.assertEqual(len(table_entries), 2)
 
         with self.subTest("Test search field"):
             input_field = self.find_element(By.CSS_SELECTOR, "#device-search")
             input_field.send_keys("device1")
-            self.wait_for_invisibility(
-                By.CSS_SELECTOR, ".map-detail .ow-loading-spinner"
-            )
+            self._wait_for_popup_table_ready()
             table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
             self.assertEqual(len(table_entries), 1)
             self.assertIn(d1.name, table_entries[0].text)
@@ -422,17 +434,13 @@ def test_features_on_device_popup(self):
             input_field.clear()
             # Just clearing the input field does not trigger the event listeners
             input_field.send_keys(" ")
-            self.wait_for_invisibility(
-                By.CSS_SELECTOR, ".map-detail .ow-loading-spinner"
-            )
+            self._wait_for_popup_table_ready()
             table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
             self.assertEqual(len(table_entries), 2)
 
         with self.subTest("Test filtering to get no results"):
             input_field.send_keys("Non-Existent-Device")
-            self.wait_for_invisibility(
-                By.CSS_SELECTOR, ".map-detail .ow-loading-spinner"
-            )
+            self._wait_for_popup_table_ready()
             table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
             self.assertEqual(len(table_entries), 1)
             self.assertIn("No devices found", table_entries[0].text)
@@ -467,7 +475,9 @@ def test_infinite_scroll_on_popup(self):
         self.web_driver.execute_script(
             "arguments[0].scrollTop = arguments[0].scrollHeight", table_container
         )
-        self.wait_for_invisibility(By.CSS_SELECTOR, ".map-detail .ow-loading-spinner")
+        # Allow scroll animation to trigger infinite scroll fetch.
+        sleep(0.3)
+        self._wait_for_popup_table_ready()
         table_entries = self.find_elements(By.CSS_SELECTOR, ".map-detail tbody tr")
         self.assertEqual(len(table_entries), 20)