[change] Replace custom popup handling with nodePopup config#796
[change] Replace custom popup handling with nodePopup config#796dee077 wants to merge 4 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR refactors popup handling in device-map.js and floorplan.js to use NetJSONGraph's built-in nodePopup configuration. The changes move popup content generation, table rendering, and DOM event binding from custom Leaflet lifecycle management into reusable callback functions (loadPopUpContent, renderRows, bindPopupEventListener). Device map popups now fetch and paginate location devices asynchronously with search and status filtering, while floorplan popups render a simpler device summary. Both files update label visibility config naming from showLabelsAtZoomLevel to showMapLabelsAtZoom, disable click-driven popup opening, and refactor realtime location updates to read popup state via optional chaining. Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Line 368: The map config option was renamed to showMapLabelsAtZoom but onReady
still reads map.config.showLabelsAtZoomLevel, causing initialZoom >= undefined
to always be false; update the onReady logic (the onReady function that checks
initialZoom against map.config.showLabelsAtZoomLevel) to read
map.config.showMapLabelsAtZoom (or guard if undefined) so the comparison uses
the renamed option and re-enables emphasis labels correctly.
- Around line 233-235: Remove the leftover debugger statement inside the
success(data) callback in device-map.js (the success function that handles popup
pagination when append is true); simply delete the line containing "debugger;"
so the success(data) handler no longer pauses execution when DevTools are open
and pagination/automated tests run uninterrupted.
- Around line 223-227: The infinite-scroll branch uses the base `url` instead of
the pagination path; when `append` is true set `fetchUrl` to `nextUrl` (the next
page URL) instead of `url` so the request fetches the next page rather than
reloading page 1; update the `if (append)` branch that assigns `fetchUrl` (and
ensure `nextUrl` is the variable produced by pagination logic) to use `nextUrl`
and leave the else branch using `queryString`/`url` unchanged.
- Around line 157-169: The function renderRows currently writes the "No devices
found" markup to the DOM via popup.find(...) and then returns an empty string,
which fails when .map-detail doesn't exist; change renderRows (and the branch
where deviceList.length === 0) to construct and return the empty-state HTML
string (e.g. the same <tr> template) instead of returning "", and only perform
popup.find("tbody").html(template) if popup.length > 0 (so DOM writes are
conditional). Keep references to renderRows,
netjsongraphInstance.leaflet._popupState.devices and the popup variable to
locate the code.
- Around line 345-348: Remove the manual call to loadPopUpContent inside the
geoOptions.onEachFeature click handler so NetJSONGraph's nodePopup.content
handles popup population exclusively; locate the click handler in
geoOptions.onEachFeature (where loadPopUpContent is currently invoked manually)
and delete that invocation, leaving nodePopup: { show: true, content:
loadPopUpContent, onOpen: bindPopupEventListener } intact and ensuring
_popupState is no longer raced by the legacy manual call.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1443db79-0039-4de2-a2e3-17132d3a584a
⛔ Files ignored due to path filters (1)
openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (2)
openwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/device/static/monitoring/js/device-map.js
🔇 Additional comments (1)
openwisp_monitoring/device/static/monitoring/js/floorplan.js (1)
380-380: LGTM!Also applies to: 387-410, 416-417, 531-531
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
dee077
force-pushed
the
change/795-use-nodepopup-config-for-geo-and-indoor-maps
branch
from
e70d3a1 to
4128198
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_monitoring/device/static/monitoring/js/floorplan.js (1)
387-417:⚠️ Potential issue | 🔴 CriticalFix reference to renamed property in device-map.js.
The configuration property
showLabelsAtZoomLevelwas renamed toshowMapLabelsAtZoom(as seen in the config at line 368), but device-map.js line 455 still references the old property name:const showLabel = initialZoom >= map.config.showLabelsAtZoomLevel;This should be:
const showLabel = initialZoom >= map.config.showMapLabelsAtZoom;The
nodePopupconfiguration withshow,content, andconfigproperties is correctly structured and matches the NetJSONGraph pattern for both floorplan.js and device-map.js (with the latter also including anonOpencallback). However, the property reference bug will cause incorrect label visibility logic due to accessing an undefined config value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js` around lines 387 - 417, The label-visibility check in device-map.js still uses the old config key showLabelsAtZoomLevel causing undefined reads; update the condition that computes showLabel (the expression using initialZoom and map.config) to reference the renamed property showMapLabelsAtZoom instead of showLabelsAtZoomLevel so label visibility uses the actual map.config value; verify the change affects the code path where showLabel is used to toggle node labels (search for the showLabel variable and its comparison against initialZoom/map.config).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Around line 387-417: The label-visibility check in device-map.js still uses
the old config key showLabelsAtZoomLevel causing undefined reads; update the
condition that computes showLabel (the expression using initialZoom and
map.config) to reference the renamed property showMapLabelsAtZoom instead of
showLabelsAtZoomLevel so label visibility uses the actual map.config value;
verify the change affects the code path where showLabel is used to toggle node
labels (search for the showLabel variable and its comparison against
initialZoom/map.config).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8c258196-2cf2-46e8-8e98-8770847f6156
⛔ Files ignored due to path filters (1)
openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (2)
openwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/device/static/monitoring/js/device-map.js
🔇 Additional comments (10)
openwisp_monitoring/device/static/monitoring/js/floorplan.js (2)
380-380: LGTM!
531-531: LGTM!openwisp_monitoring/device/static/monitoring/js/device-map.js (8)
157-169: Return the empty-state row fromrenderRows.On initial render from
loadPopUpContent,.map-detaildoes not exist in the DOM yet, sopopup.find("tbody").html(...)is a no-op. Returning""leaves the table body empty instead of showing the "No devices found" message.
223-227: UsenextUrlfor infinite-scroll fetches.The
appendbranch relies on the caller passing the pagination URL as theurlparameter, but using the outer-scopenextUrldirectly would be clearer and less error-prone.
233-235: Remove the leftoverdebuggerstatement.Line 234 will pause popup pagination whenever DevTools are open and can interfere with QA or automated browser runs.
345-355: Remove the manualloadPopUpContentcall from the click handler.With
nodePopup.contentset toloadPopUpContent, the manual invocation at line 428 insidegeoOptions.onEachFeaturecreates a race condition. Both handlers fire on the same click, potentially causing duplicate async calls that race_popupState.Also applies to: 406-434
368-368: Update the consumer of the renamed label-zoom option.Line 368 renames to
showMapLabelsAtZoom, butonReadyat line 455 still readsmap.config.showLabelsAtZoomLevel. The comparisoninitialZoom >= undefinedis always false, so emphasis labels never re-enable.Also applies to: 455-455
59-155: LGTM!
437-437: LGTM!
574-575: LGTM!
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
Refactored geo and indoor maps to use the configurable NetJSONGraph nodePopup config, reducing duplicated popup handling logic. Fixes #795
dee077
force-pushed
the
change/795-use-nodepopup-config-for-geo-and-indoor-maps
branch
2 times, most recently
from
3d4fd13 to
9cef6c3
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_monitoring/device/static/monitoring/js/device-map.js (1)
419-433: 🧹 Nitpick | 🔵 Trivial | ⚖️ Poor tradeoffClarify the dual popup-triggering architecture or consolidate.
The click handler in
geoOptions.onEachFeature(lines 419-433) manually invokesloadPopUpContent, whilenodePopup.contentat line 346 also references the same function. Combined withonClickElement: function () {}(no-op at line 436), this suggests:
- ECharts scatter nodes rely on
nodePopup- Leaflet GeoJSON features rely on manual handling
If this dual-path design is intentional, add a brief comment explaining why both mechanisms coexist. Otherwise, consider consolidating to avoid maintenance burden and the
this-binding issues noted above.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_monitoring/device/static/monitoring/js/device-map.js` around lines 419 - 433, The click handling currently uses two different popup paths (geoOptions.onEachFeature's manual click handler calling loadPopUpContent and the nodePopup.content/onClickElement used by ECharts), which is confusing and can cause duplicate triggers and this-binding issues; either consolidate to a single popup entry point by removing the manual call in geoOptions.onEachFeature and delegating to nodePopup/onClickElement, or keep both but add a short comment in geoOptions.onEachFeature explaining why GeoJSON features must call loadPopUpContent directly (e.g., ECharts nodes use nodePopup while Leaflet features cannot rely on it), and if consolidating, ensure to wire the same loadPopUpContent call through nodePopup.onClickElement and remove the no-op onClickElement; reference geoOptions.onEachFeature, loadPopUpContent, nodePopup.content and onClickElement when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Around line 59-61: loadPopUpContent expects its `this` to be the NetJSONGraph
instance but is invoked as a plain function from geoOptions.onEachFeature, so
`this` is undefined; fix by invoking it with the correct context (e.g. use
Function.prototype.call/apply or a bound function) where you currently call
loadPopUpContent(feature, map) inside geoOptions.onEachFeature so that `this`
points to the NetJSONGraph instance, and do the same for the other manual
invocation area (the block affecting nodePopup.content / lines 76-81) instead of
leaving it as a plain call.
- Line 193: Fix the typo in the console error message in device-map.js where the
string currently reads "Popup not found, cann't bind event listeners" — update
it to "Popup not found, can't bind event listeners" (search for the
console.error call that logs "Popup not found" to locate the exact spot).
- Line 159: Remove the dead commented-out statement "// deviceList = deviceList
|| devices;" from the device-map.js snippet; it's redundant with the active code
on the previous line and should be deleted so only the live assignment using
deviceList/devices remains.
- Around line 229-241: The success callback currently uses jQuery's dynamic this
(ajax settings object) when calling renderRows(this, devices), causing
NetJSONGraph lookups to fail; fix by capturing the correct NetJSONGraph
reference before the $.ajax call (e.g., const instanceRef = this or a named
variable holding the NetJSONGraph) or convert the success callback to an arrow
function to preserve lexical this, then call renderRows(instanceRef, devices)
(or renderRows(this, devices) if using arrow) so renderRows receives the real
NetJSONGraph instance.
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Line 532: The empty onClickElement: function () {} is an intentional no-op
because popup handling has been moved to the nodePopup configuration; add a
concise inline comment above or inside the onClickElement function (referencing
onClickElement and nodePopup) stating that the handler is intentionally empty
and popups are handled via nodePopup so future maintainers do not remove or
repurpose it.
---
Outside diff comments:
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Around line 419-433: The click handling currently uses two different popup
paths (geoOptions.onEachFeature's manual click handler calling loadPopUpContent
and the nodePopup.content/onClickElement used by ECharts), which is confusing
and can cause duplicate triggers and this-binding issues; either consolidate to
a single popup entry point by removing the manual call in
geoOptions.onEachFeature and delegating to nodePopup/onClickElement, or keep
both but add a short comment in geoOptions.onEachFeature explaining why GeoJSON
features must call loadPopUpContent directly (e.g., ECharts nodes use nodePopup
while Leaflet features cannot rely on it), and if consolidating, ensure to wire
the same loadPopUpContent call through nodePopup.onClickElement and remove the
no-op onClickElement; reference geoOptions.onEachFeature, loadPopUpContent,
nodePopup.content and onClickElement when making changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 432a6034-c214-4587-b7b3-b7525d545151
⛔ Files ignored due to path filters (1)
openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (3)
openwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/tests/test_selenium.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/tests/test_selenium.pyopenwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.js
🧠 Learnings (5)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
🔇 Additional comments (7)
openwisp_monitoring/tests/test_selenium.py (1)
325-325: LGTM!openwisp_monitoring/device/static/monitoring/js/device-map.js (1)
344-354: LGTM!Also applies to: 367-367, 454-454, 522-522, 573-574
openwisp_monitoring/device/static/monitoring/js/floorplan.js (5)
355-381: LGTM!
387-387: LGTM!
408-417: LGTM!
419-424: LGTM!
234-234: LGTM!Also applies to: 543-543
dee077
force-pushed
the
change/795-use-nodepopup-config-for-geo-and-indoor-maps
branch
from
9cef6c3 to
ea5a7c0
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_monitoring/device/static/monitoring/js/device-map.js (1)
202-210:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMove the
loadingflip to request start, not debounce scheduling.
loading = trueis set before thesetTimeoutruns, so the next search/filter change immediately hitsif (!url || loading) returnand never replaces the pending request. In practice, fast typing leaves the popup showing results for an older query instead of the latest one. Track “debounce scheduled” separately, or only markloadingonce the AJAX request is actually about to fire.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@openwisp_monitoring/device/static/monitoring/js/device-map.js` around lines 202 - 210, The code sets loading = true immediately in fetchDevices(), preventing newer calls from replacing the pending debounce; instead introduce a separate debounce flag (e.g. debounceScheduled) or only set loading when the AJAX request actually starts: remove the top-level loading = true, set debounceScheduled = true when you call setTimeout and clear it inside the timeout before starting the request, then set loading = true just before initiating the AJAX call in the setTimeout callback (also ensure fetchDevicesTimeout is cleared and debounceScheduled/loading are reset on completion/error); reference function fetchDevices, variables loading and fetchDevicesTimeout, and the setTimeout callback where the spinner/table and the AJAX request are handled.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Around line 547-548: The websocket update handler accesses
window._owGeoMap.leaflet and window._owGeoMap.leaflet?._popupState without
guarding window._owGeoMap itself, which can throw if messages arrive before
window._owGeoMap is assigned; update the two reads to start the optional chain
on window._owGeoMap (e.g. window._owGeoMap?.leaflet and
window._owGeoMap?._popupState) so currentPopup and currentPopupLocationId are
only read when the global map reference exists.
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Around line 231-235: The close handler is only removing the "nodeId" key,
leaving the bookmarkable fragment id in the URL; update
removeUrlFragment(locationId) to call
maps[currentFloor].utils.removeUrlFragment(id) with no second argument (omit the
"nodeId" key) so the utils method removes the entire fragment associated with
that fragmentId; locate the function removeUrlFragment and replace
maps[currentFloor].utils.removeUrlFragment(id, "nodeId") with
maps[currentFloor].utils.removeUrlFragment(id).
In `@openwisp_monitoring/tests/test_selenium.py`:
- Line 325: Remove or document the unused class attribute `retry_max = 1` in the
test case defined in openwisp_monitoring/tests/test_selenium.py: either delete
the `retry_max` line if no local code reads it, or add a brief inline comment on
the test class explaining what it was intended to control and where it would be
consumed (e.g., that Django Channels' ChannelsLiveServerTestCase does not
support `retry_max`), so future readers know why it exists; locate the attribute
on the test class and apply the chosen change.
---
Outside diff comments:
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Around line 202-210: The code sets loading = true immediately in
fetchDevices(), preventing newer calls from replacing the pending debounce;
instead introduce a separate debounce flag (e.g. debounceScheduled) or only set
loading when the AJAX request actually starts: remove the top-level loading =
true, set debounceScheduled = true when you call setTimeout and clear it inside
the timeout before starting the request, then set loading = true just before
initiating the AJAX call in the setTimeout callback (also ensure
fetchDevicesTimeout is cleared and debounceScheduled/loading are reset on
completion/error); reference function fetchDevices, variables loading and
fetchDevicesTimeout, and the setTimeout callback where the spinner/table and the
AJAX request are handled.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: b0cf8511-24ed-4909-9f55-b452a4d40479
⛔ Files ignored due to path filters (1)
openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (3)
openwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/tests/test_selenium.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
🧰 Additional context used
📓 Path-based instructions (1)
{**/*.sh,**/*.{js,ts,tsx,jsx},**/*.py,**/*.rb,**/*.go,**/*.java,**/*.cs,**/*.cpp,**/*.c}
📄 CodeRabbit inference engine (Custom checks)
Cryptic or non-obvious code (regex, complex bash commands, or hard-to-read code) must include a concise comment explaining why it is needed and why the complexity is acceptable
Files:
openwisp_monitoring/tests/test_selenium.pyopenwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.js
🧠 Learnings (5)
📚 Learning: 2026-02-21T01:03:37.822Z
Learnt from: nemesifier
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:827-859
Timestamp: 2026-02-21T01:03:37.822Z
Learning: In Selenium tests (e.g., in openwisp_monitoring/tests/test_selenium.py and similar test files), when testing JS animations on dashboards or elements driven by JavaScript (such as real-time location updates), insert a short sleep (e.g., sleep(0.3)) before WebDriverWait assertions to allow animations to complete and reduce flakiness. Note: use this as a targeted workaround and prefer explicit waits or animation-end checks where possible to avoid relying on fixed delays.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-02-21T18:44:28.852Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/api/views.py:263-281
Timestamp: 2026-02-21T18:44:28.852Z
Learning: In openwisp-monitoring, MonitoringIndoorCoordinatesList inherits organization scoping from the parent IndoorCoordinatesList (from openwisp-controller), which uses FilterByParentManaged mixin and filters by location_id in get_queryset(). The child class only overrides the queryset attribute to add monitoring-specific select_related fields; this pattern is safe as long as get_queryset() from the parent is not bypassed. During reviews, verify that MonitoringIndoorCoordinatesList continues to rely on the parent's get_queryset() and that any added select_related fields in the child do not alter the parent's filtering logic.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-02-25T18:42:08.825Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/tests/test_selenium.py:309-317
Timestamp: 2026-02-25T18:42:08.825Z
Learning: In Selenium tests that use ChannelsLiveServerTestCase (e.g., openwisp_monitoring/tests/test_selenium.py), override settings to configure CHANNEL_LAYERS with channels_redis.core.RedisChannelLayer instead of InMemoryChannelLayer. This is required because the live server runs in a separate process from the test process, and InMemoryChannelLayer is per-process only and cannot handle cross-process WebSocket broadcasting needed for real-time location update tests.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In this repository (openwisp/openwisp-monitoring), the project targets Python 3.10–3.13 as defined by CI. Do not flag backports.zoneinfo as a missing dependency; zoneinfo is a built-in module in all supported Python versions. When reviewing Python code, assume zoneinfo is available and avoid suggesting installation of backports.zoneinfo. If a file imports zoneinfo or uses it for time zone handling, treat it as standard library usage compatible with the supported CI matrix.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-03-14T18:39:04.626Z
Learnt from: UltraBot05
Repo: openwisp/openwisp-monitoring PR: 766
File: openwisp_monitoring/utils.py:59-68
Timestamp: 2026-03-14T18:39:04.626Z
Learning: In the openwisp-monitoring project, targets are Linux-based environments. Do not flag a Windows-specific tzdata package as a missing dependency in code reviews for Python files (e.g., openwisp_monitoring/utils.py). If a platform-specific dependency is truly required, document the exception in review guidelines and ensure CI/packaging checks enforce platform constraints rather than manual review.
Applied to files:
openwisp_monitoring/tests/test_selenium.py
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
dee077
force-pushed
the
change/795-use-nodepopup-config-for-geo-and-indoor-maps
branch
from
ea5a7c0 to
42069a1
Compare
dee077
force-pushed
the
change/795-use-nodepopup-config-for-geo-and-indoor-maps
branch
from
42069a1 to
e46815d
Compare
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Please link the upstream netjsongraph.js PR/release or the generated source used for openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.js. This bundle adds nodePopup, onOpen, preserveFragment, and the new removeUrlFragment(id, key, preserveFragment) behavior, but the minified diff is not reviewable or reproducible as-is.
| loadingOverlay.hide(); | ||
| alert(gettext("Error while retrieving data")); | ||
| console.error(error); | ||
| return null; |
There was a problem hiding this comment.
Returning null here is still not enough: nodePopup treats it as content, opens an empty popup, and then calls onOpen. Since _popupState is only set after a successful response, the destructuring in bindPopupEventListener can throw on the error path. Please abort popup creation on failure, for example by rethrowing after showing the alert, and guard _popupState before destructuring.
There was a problem hiding this comment.
Yes, removed return null with throw error
| statusFilterButtons += `<span | ||
| let nextUrl = data.next; | ||
| let statusFilterButtons = ""; | ||
| map._popupState = { |
There was a problem hiding this comment.
This state is written before netjsongraph knows whether this async request is still the current one. A slower request for location A can finish after location B has opened and overwrite _popupState, while the websocket handler then reads the wrong locationId. Please only store this for the popup that is actually opened, or keep it scoped to the popup instance.
There was a problem hiding this comment.
Added an additional on currentPopup remove event listener to make _popupState = null
| }, | ||
| // Popup handling is delegated to nodePopup.content, | ||
| // so disable the default onClickElement popup behavior. | ||
| onClickElement: function () {}, |
There was a problem hiding this comment.
Removing onEachFeature also removed the hover tooltip that showed feature.properties.name. Since ECharts and map tooltips are disabled, locations no longer show their name on hover. If that is intentional please call it out in the PR; otherwise please restore the hover label while keeping popup ownership in nodePopup.
There was a problem hiding this comment.
It was repetitive
With showMapLabelsAtZoom: 8, it works as expected
github-project-automation
Bot
moved this from Reviewer approved
to In progress
in OpenWISP Priorities for next releases
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@dee077 When I pass the mouse over a point, the label disappears, but this happens before I click on the point. Is this on purpose?
Screencast.from.2026-05-25.20-22-04.mp4
I think the label should disappear only when the point is clicked.
2 participants
Refactored geo and indoor maps to use the configurable NetJSONGraph nodePopup config, reducing duplicated popup handling logic.
Fixes #795
Checklist
Reference to Existing Issue
Closes #795 .
On top of openwisp/netjsongraph.js#547 and openwisp/netjsongraph.js#542
Description of Changes
Refactored the geo and indoor map implementations to use the configurable mapOptions.nodePopup functionality added in NetJSONGraph. This removes duplicated custom popup handling logic, reuses the shared popup lifecycle implementation, and keeps popup behavior configurable through the graph configuration.
Todo