[chores] Improvements to the dashboard map#809
Open
dee077 wants to merge 13 commits into
Open
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 centralizes popup and floorplan state, converts popup content loading to an async AJAX flow with abortable requests, refactors popup rendering and event handling to support replace-vs-append loading and infinite scroll, scopes floorplan overlay state with session guards and delegated navigation handlers, adjusts CSS for loading states and spinner placement, replaces hardcoded admin map URLs with a runtime window._owMapPageUrl, and updates Selenium tests to wait on the new loading-state classes. Sequence Diagram(s)sequenceDiagram
participant NetJSONGraph
participant MapLeaflet
participant Client
participant Server
participant PopupTable
NetJSONGraph->>MapLeaflet: nodePopup triggers loadPopUpContent(node, netjson)
MapLeaflet->>Client: invoke loadPopUpContent
Client->>Server: AJAX request for devices (debounced, abortable)
Server-->>Client: returns devices + nextUrl
Client->>PopupTable: renderRows(devices) / update DOM (replace or append)
PopupTable-->>Client: hide spinner / remove loading classes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
dee077
changed the title
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![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`:
- Around line 202-266: The popup teardown currently only clears _popupState but
doesn't cancel a queued fetchDevices timeout or abort an in-flight $.ajax;
update fetchDevices to store the timeout id (fetchDevicesTimeout) and the jqXHR
(e.g., fetchDevicesRequest) when starting the request, add logic to the error
handler to ignore aborts, and in the popup teardown routine where _popupState is
cleared also call clearTimeout(fetchDevicesTimeout) and if (fetchDevicesRequest)
fetchDevicesRequest.abort(); additionally clear/reset fetchDevicesRequest in the
ajax complete() callback so subsequent teardowns are safe.
- Around line 141-142: renderRows is performing global DOM reads/writes (e.g.,
using $(".map-detail") and updating "`#status-filter`") which causes one popup to
overwrite another; change renderRows and the popup helper used by nodePopup to
accept (or locate) the current popup element and scope all DOM queries/updates
to that element (use popupElement.querySelector or $(popupElement).find(...)
instead of global selectors), replace global id usage like "`#status-filter`" with
scoped selectors inside the popup (or convert to a class and scope it) and
update all occurrences referenced in renderRows, nodePopup and the ranges called
out (lines ~158-187, ~251, ~271-285) to use the scoped popup element.
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Around line 279-293: removeUrlFragment currently requires an initialized map
instance to clear the indoor URL fragment, which leaves a stale hash if the
overlay is closed before renderIndoorMap sets maps[currentFloor]; update
removeUrlFragment (using getFloorplanState, floorplanState.state.locationId and
floorplanState.state.currentFloor) to fall back to removing the URL fragment
even when floorplanState.maps[currentFloor] is undefined: if the map exists keep
using maps[currentFloor].utils.removeUrlFragment(id), otherwise clear the
fragment directly (e.g. via history.replaceState to remove location.hash) when
state.locationId is present so the bookmarkable overlay hash is reliably
removed.
- Around line 143-155: openFloorPlan currently allows id = null but
setFloorplanState expects floorplanState.state.locationId to be non-null; update
openFloorPlan to ensure a valid locationId before calling setFloorplanState by
either deriving a proper id (from url or floor) or throwing an error if id is
absent. Locate the openFloorPlan function and the call to
setFloorplanState(buildInitialFloorplanState(url, id, floor)) and add a guard
that computes/validates a non-null id (or fails fast) and pass that canonical id
into buildInitialFloorplanState so the persisted state never contains
locationId=null.
- Around line 476-568: The onReady() continuation mutates map/ECharts/DOM after
awaiting img.decode() without checking if the floorplan session was closed or
replaced; to fix, capture the current session identifier before awaiting (e.g.,
read getFloorplanState()._sessionId or other unique session token), then
immediately after await img.decode() re-read getFloorplanState() and compare the
session id and state; if the state is missing or the session id differs, return
early and avoid touching map, this.data, DOM, or calling
pushIndoorMapIdFragment; update logic around the img.decode() await in onReady()
to perform this guard so destroyed/replaced sessions don't get mutated.
🪄 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: 28b6dc1c-c1b0-4b82-b21c-8437f66cbe0a
⛔ 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/css/device-map.cssopenwisp_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.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.11 | 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/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.js
🪛 GitHub Check: CodeQL
openwisp_monitoring/device/static/monitoring/js/floorplan.js
[failure] 34-34: Insecure randomness
This uses a cryptographically insecure random number generated at Math.random() in a security context.
🔇 Additional comments (1)
openwisp_monitoring/device/static/monitoring/css/device-map.css (1)
150-162: LGTM!Also applies to: 189-189
nemesifier
changed the title
dee077
force-pushed
the
feature/705-improvements-to-dashboard-maps
branch
from
May 31, 2026 13:29
5531f9d to
0ab579f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/css/device-map.css`:
- Around line 192-195: The spinner is horizontally off-center because transform
only translates vertically; update the rule for
.table-container.is-loading-append .table-spinner to translate both axes (e.g.,
use translate(-50%, -50%) or combine translateX(-50%) and translateY(-50%)) so
the spinner's center is aligned at left: 50%; keep the existing top and left
declarations and the !important flags intact.
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Around line 199-267: The fetchDevices function currently uses a single loading
flag that blocks any new fetch while one is active; change it so only concurrent
append requests are blocked and non-append (replace) requests cancel/overtake
in-flight append work: at the top of fetchDevices, keep the early-return for
missing url but replace the blanket "if (loading) return" with logic that (a) if
append and loading (or loadingAppend) is true, return to block concurrent
appends, and (b) if not append and activeRequest exists (and especially if the
activeRequest was an append), call activeRequest.abort() and clear any
append-specific flags before proceeding; introduce/track a distinct append flag
(e.g., loadingAppend) or inspect the append parameter when setting/clearing
flags so success/complete handlers still update devices/nextUrl correctly; use
activeRequest.abort() to ensure replace requests overtake in-flight appends and
then start the new replace fetch.
🪄 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: a82f7863-336a-4c97-8b42-c481fff5fd28
📒 Files selected for processing (4)
openwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_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.11 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.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.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | 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/floorplan.jsopenwisp_monitoring/device/static/monitoring/js/device-map.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 (20)
openwisp_monitoring/device/static/monitoring/js/floorplan.js (19)
7-38: LGTM!
40-52: LGTM!
84-116: LGTM!
118-127: LGTM!
129-141: LGTM!
143-168: LGTM!
170-232: LGTM!
234-257: LGTM!
259-280: LGTM!
282-317: LGTM!
319-346: LGTM!
348-386: LGTM!
388-405: LGTM!
407-433: LGTM!
435-609: LGTM!
610-643: LGTM!
645-662: LGTM!
664-700: LGTM!
702-703: LGTM!openwisp_monitoring/tests/test_selenium.py (1)
325-350: LGTM!Also applies to: 390-440, 475-477
|
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
feature/705-improvements-to-dashboard-maps
branch
from
May 31, 2026 13:47
0ab579f to
ee83d0d
Compare
Member
Author
Updates
Infinite.scroll.webm
|
dee077
marked this pull request as ready for review
May 31, 2026 13:55
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
openwisp_monitoring/device/static/monitoring/js/floorplan.js (2)
506-518:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRollback failed image initialization so the floor can be retried.
maps[floor]is registered beforeimg.decode(). If the image load fails, this catch exits without clearing that entry or the floor container, andshowFloor()will treat the floor as already rendered on the next visit instead of retryingrenderIndoorMap().Suggested fix
try { await img.decode(); } catch (e) { console.error("Failed to load floorplan image:", e); + const latestState = getFloorplanState(); + if (latestState?.maps?.[floor] === indoorMap) { + delete latestState.maps[floor]; + setFloorplanState(latestState); + } + $(`#floor-content-${floor}`).remove(); $(".floorplan-loading-spinner").hide(); return; }🤖 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 506 - 518, The code registers floorplanState.maps[floor] before the image is confirmed loaded, so on img.decode() failure the entry remains and showFloor() will skip re-rendering; fix by deferring the assignment of floorplanState.maps[floor] (and the setFloorplanState call) until after await img.decode() succeeds, or if you prefer to keep the assignment early then in the img.decode() catch remove the stale entry (delete floorplanState.maps[floor] and call setFloorplanState) and clear the floor container so renderIndoorMap()/showFloor() can retry; update the catch to also hide the loading spinner as already done.
151-167:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle open-time failures before leaving the overlay mounted.
The overlay and backdrop are created before the first awaited fetch, but this path does not catch
fetchData()/showFloor()failures. Current callers fire-and-forgetwindow.openFloorPlan(...), so a rejected promise here leaves a half-built overlay on screen and surfaces as an unhandled rejection.Suggested fix
- await fetchData(url, floor); - const floorplanState = getFloorplanState(); - if (!floorplanState?.state) return; - calculateNavigationState(floorplanState.state.currentFloor); - - addFloorButtons(); - await showFloor(url, floorplanState.state.currentFloor); + try { + await fetchData(url, floor); + const floorplanState = getFloorplanState(); + if (!floorplanState?.state) return; + calculateNavigationState(floorplanState.state.currentFloor); + + addFloorButtons(); + await showFloor(url, floorplanState.state.currentFloor); + } catch (error) { + destroyFloorplan(); + return; + }🤖 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 151 - 167, The overlay and backdrop are mounted before awaiting fetchData/showFloor, so any rejection leaves a partial UI and an unhandled promise; wrap the async sequence starting from setFloorplanState(...) through await showFloor(...) in a try/catch (or add .catch) that on error reverses the DOM mutations (remove the created $floorPlanContainer/$floorNavigation from `#dashboard-map-overlay` and `#floorplan-overlay` and remove the "active" class added to .menu-backdrop), clears or resets state via setFloorplanState(null) or similar, and rethrows or handle/log the error to avoid an unhandled rejection; locate the code around createFloorPlanContainer(), createFloorNavigation(), fetchData(url, floor), showFloor(url, ...), getFloorplanState() to implement the cleanup in the catch block so callers of window.openFloorPlan(...) won't leave the overlay mounted on failure.
♻️ Duplicate comments (1)
openwisp_monitoring/device/static/monitoring/js/device-map.js (1)
203-205:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent aborted requests from resetting loading/UI state
fetchDevices()aborts the previousactiveRequestwhen!append, but the jqXHRcomplete()handler unconditionally setsactiveRequest = null,loading = false, hides the spinner, and removes loading classes. Since the replacement request is started later insidesetTimeout(ms), the aborted request’scomplete()can run during the delay and make the popup look idle while the next fetch is pending/in-flight.if (!append && activeRequest) { activeRequest.abort(); }Suggested fix
let fetchDevicesTimeout; let activeRequest = null; + let activeRequestId = 0; let loading = false; function fetchDevices(url, ms = 0, append) { 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"); spinner.show(); @@ - fetchDevicesTimeout = setTimeout(() => { + const requestId = ++activeRequestId; + fetchDevicesTimeout = setTimeout(() => { let params = new URLSearchParams(); @@ - activeRequest = $.ajax({ + const request = $.ajax({ dataType: "json", url: fetchUrl, xhrFields: { withCredentials: true }, @@ 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() { + if (requestId !== activeRequestId || activeRequest !== request) return; activeRequest = null; loading = false; spinner.hide(); container.removeClass("is-loading"); container.removeClass("is-loading-append"); container.css("min-height", ""); container.css("--table-spinner-top", ""); }, }); + activeRequest = request; }, ms); }🤖 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 203 - 205, The abort of activeRequest in fetchDevices() can cause the aborted jqXHR complete() handler to clear UI/loading state while a replacement request is scheduled inside setTimeout(ms); update the logic so the complete() handler only clears activeRequest/loading/spinner when the completing request is still the current activeRequest (or use a per-request id token) instead of unconditionally nulling them—modify the complete handler in device-map.js to check xhr === activeRequest (or compare requestId) before changing loading, hiding the spinner, or removing loading classes so aborted or stale completions do not reset the UI for the new fetch.
🤖 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/tests/test_selenium.py`:
- Around line 334-347: The `_wait_for_popup_table_ready` method contains a
non-obvious predicate that polls for the absence of specific CSS classes
(`is-loading` and `is-loading-append`) to determine when the popup table is
ready. Add a concise comment above the WebDriverWait call explaining why
monitoring these CSS classes is used instead of directly checking spinner
visibility, and why this approach provides a stable readiness signal. This
clarifies the tight coupling to the popup's JS/CSS loading contract for future
maintainers.
---
Outside diff comments:
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Around line 506-518: The code registers floorplanState.maps[floor] before the
image is confirmed loaded, so on img.decode() failure the entry remains and
showFloor() will skip re-rendering; fix by deferring the assignment of
floorplanState.maps[floor] (and the setFloorplanState call) until after await
img.decode() succeeds, or if you prefer to keep the assignment early then in the
img.decode() catch remove the stale entry (delete floorplanState.maps[floor] and
call setFloorplanState) and clear the floor container so
renderIndoorMap()/showFloor() can retry; update the catch to also hide the
loading spinner as already done.
- Around line 151-167: The overlay and backdrop are mounted before awaiting
fetchData/showFloor, so any rejection leaves a partial UI and an unhandled
promise; wrap the async sequence starting from setFloorplanState(...) through
await showFloor(...) in a try/catch (or add .catch) that on error reverses the
DOM mutations (remove the created $floorPlanContainer/$floorNavigation from
`#dashboard-map-overlay` and `#floorplan-overlay` and remove the "active" class
added to .menu-backdrop), clears or resets state via setFloorplanState(null) or
similar, and rethrows or handle/log the error to avoid an unhandled rejection;
locate the code around createFloorPlanContainer(), createFloorNavigation(),
fetchData(url, floor), showFloor(url, ...), getFloorplanState() to implement the
cleanup in the catch block so callers of window.openFloorPlan(...) won't leave
the overlay mounted on failure.
---
Duplicate comments:
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Around line 203-205: The abort of activeRequest in fetchDevices() can cause
the aborted jqXHR complete() handler to clear UI/loading state while a
replacement request is scheduled inside setTimeout(ms); update the logic so the
complete() handler only clears activeRequest/loading/spinner when the completing
request is still the current activeRequest (or use a per-request id token)
instead of unconditionally nulling them—modify the complete handler in
device-map.js to check xhr === activeRequest (or compare requestId) before
changing loading, hiding the spinner, or removing loading classes so aborted or
stale completions do not reset the UI for the new fetch.
🪄 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: 0c1963a1-44d9-47b7-a3b0-cb964e406865
📒 Files selected for processing (4)
openwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_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.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | 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
- 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/floorplan.jsopenwisp_monitoring/device/static/monitoring/js/device-map.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 (4)
openwisp_monitoring/device/static/monitoring/css/device-map.css (1)
192-195: LGTM!openwisp_monitoring/device/static/monitoring/js/device-map.js (1)
123-147: LGTM!Also applies to: 158-184
openwisp_monitoring/tests/test_selenium.py (1)
390-440: LGTM!Also applies to: 475-477
openwisp_monitoring/device/static/monitoring/js/floorplan.js (1)
33-35: LGTM!Also applies to: 143-146, 299-316, 521-529
Test Failures in
|
dee077
force-pushed
the
feature/705-improvements-to-dashboard-maps
branch
2 times, most recently
from
May 31, 2026 14:04
b9c6d2a to
5aa2e02
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
143-168:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTear down the overlay when the initial floorplan load fails.
openFloorPlan()now mounts the backdrop/overlay before the first request, but iffetchData()rejects the function exits without cleanup. That leaves a blank overlay and active backdrop behind after API errors.💡 Minimal fix
setFloorplanState(buildInitialFloorplanState(url, id, floor)); setupNavigationHandlers(); - await fetchData(url, floor); - const floorplanState = getFloorplanState(); - if (!floorplanState?.state) return; - calculateNavigationState(floorplanState.state.currentFloor); - - addFloorButtons(); - await showFloor(url, floorplanState.state.currentFloor); + try { + await fetchData(url, floor); + const floorplanState = getFloorplanState(); + if (!floorplanState?.state) return; + calculateNavigationState(floorplanState.state.currentFloor); + addFloorButtons(); + await showFloor(url, floorplanState.state.currentFloor); + } catch { + destroyFloorplan(); + } }🤖 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 143 - 168, openFloorPlan mounts UI elements before calling fetchData, so if fetchData rejects the overlay/backdrop are left behind; wrap the await fetchData(url, floor) call in a try/catch (inside openFloorPlan) and on any error call destroyFloorplan() (or otherwise remove the backdrop/overlay and clear state via destroyFloorplan) before rethrowing or returning to ensure cleanup, and also ensure the subsequent early return after getFloorplanState() failure also triggers destroyFloorplan() so no orphaned UI remains; reference functions: openFloorPlan, fetchData, destroyFloorplan, buildInitialFloorplanState, setFloorplanState.
🤖 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 203-205: The cleanup code for XHR callbacks currently clears
activeRequest, loading, and the loading CSS classes unconditionally, which lets
an older aborted request hide the spinner for a newer request; fix by making
each request capture its own XHR reference and only perform cleanup if that
reference still equals the global activeRequest (e.g. store the created XHR in a
local const xhrRef, assign activeRequest = xhrRef, then in success/fail/done
callbacks wrap the logic that clears activeRequest, sets loading=false and
removes loading classes with if (activeRequest === xhrRef) { ... }); also avoid
nulling activeRequest when aborting a previous request so the newer request
remains the authoritative handle.
---
Outside diff comments:
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Around line 143-168: openFloorPlan mounts UI elements before calling
fetchData, so if fetchData rejects the overlay/backdrop are left behind; wrap
the await fetchData(url, floor) call in a try/catch (inside openFloorPlan) and
on any error call destroyFloorplan() (or otherwise remove the backdrop/overlay
and clear state via destroyFloorplan) before rethrowing or returning to ensure
cleanup, and also ensure the subsequent early return after getFloorplanState()
failure also triggers destroyFloorplan() so no orphaned UI remains; reference
functions: openFloorPlan, fetchData, destroyFloorplan,
buildInitialFloorplanState, setFloorplanState.
🪄 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: bbb3149c-e824-43b7-8ba6-8c286fbdd16c
📒 Files selected for processing (4)
openwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_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.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.10 | 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~=4.2.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.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/device/static/monitoring/js/device-map.jsopenwisp_monitoring/tests/test_selenium.pyopenwisp_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 (1)
openwisp_monitoring/device/static/monitoring/js/floorplan.js (1)
34-35: LGTM!Also applies to: 299-316, 502-529
Test Failures in OpenWISP MonitoringHello @dee077, There are 4 test failures in the CI logs:
|
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/floorplan.js`:
- Around line 521-529: The current guard in the onReady()/continuation path
checks for missing state, sessionId mismatch, or map change but misses the case
where navigateToFloor() changed the current floor while the previous floor's
decode continued; update the condition so UI sync (hiding
.floorplan-loading-spinner and pushing the fragment) only runs when
getFloorplanState().state.currentFloor === floor. Concretely, within the same
block around getFloorplanState(), add a check referencing
latestState.state.currentFloor and the local floor variable (and keep the
existing checks for _sessionId and maps[floor]) so that if the currentFloor
differs you return early and do not hide the spinner or update the URL fragment.
In `@openwisp_monitoring/device/static/monitoring/js/location-inline.js`:
- Line 16: The code reads window._owMapPageUrl into mapPageUrl without
validation, which can produce broken hrefs like "undefined#..."; update the
logic around the map link generation (where mapPageUrl is used) to first verify
window._owMapPageUrl is a non-empty string (e.g., const mapPageUrl = typeof
window._owMapPageUrl === 'string' && window._owMapPageUrl ? window._owMapPageUrl
: null), then handle the null case by either skipping/disabled link creation or
using a safe default base URL before concatenating hashes so no href contains
"undefined"; ensure all uses of mapPageUrl (the variable) check for
null/validity before building links.
🪄 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: ae48b85b-0cfc-4018-8a31-45a93a26a9a5
📒 Files selected for processing (6)
openwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/device/static/monitoring/js/location-inline.jsopenwisp_monitoring/device/templates/admin/monitoring/device/change_form.htmlopenwisp_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.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | 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/location-inline.jsopenwisp_monitoring/tests/test_selenium.pyopenwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/device/static/monitoring/js/device-map.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 (8)
openwisp_monitoring/device/static/monitoring/js/location-inline.js (1)
22-22: LGTM!Also applies to: 46-46
openwisp_monitoring/device/templates/admin/monitoring/device/change_form.html (2)
2-2: LGTM!
611-616: LGTM!openwisp_monitoring/device/static/monitoring/js/device-map.js (2)
199-208: Older aborted requests can still clear a newer popup fetch.After Line 207 aborts the previous jqXHR, the unguarded
complete()block at Lines 268-276 still resetsactiveRequest,loading, and the loading classes for whichever request finishes. If a newer debounced search/filter fetch is pending or already in flight, the older aborted callback can hide the spinner and drop the newer handle. Guard cleanup with a per-fetch token or request identity tied to the latest scheduled fetch.Also applies to: 248-276
123-147: LGTM!Also applies to: 158-184, 296-312
openwisp_monitoring/device/static/monitoring/css/device-map.css (1)
192-195: LGTM!openwisp_monitoring/tests/test_selenium.py (1)
333-352: LGTM!Also applies to: 392-442, 477-479
openwisp_monitoring/device/static/monitoring/js/floorplan.js (1)
34-35: LGTM!Also applies to: 143-146, 299-316
|
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
feature/705-improvements-to-dashboard-maps
branch
from
May 31, 2026 20:48
1a3b6d7 to
ab7a1a2
Compare
nemesifier
reviewed
Jun 1, 2026
dee077
force-pushed
the
feature/705-improvements-to-dashboard-maps
branch
from
June 2, 2026 12:41
7c81439 to
c1b54f0
Compare
|
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
feature/705-improvements-to-dashboard-maps
branch
from
June 3, 2026 23:59
d6bd4fc to
55a3615
Compare
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
|
The CI is failing due to transient infrastructure issues (not related to your code). I have restarted the failed jobs automatically (1/3). |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Checklist
Reference to Existing Issue
Closes #705.
Description of Changes
Screenshot
Screencast.from.2026-05-28.03-49-01.webm
Note: This PR shows changes of #796, as it has been built on top of it. For now, we can just wait for the above PR to get merged, and then we can move forward with this