Issues/720 theme colors from openwisp utils#760
Issues/720 theme colors from openwisp utils#760shakurah wants to merge 20 commits intoopenwisp:masterfrom
Conversation
…izer in DeviceMetricView inside api views to display correct response schema in API documentation
…self.serializer_class
…penwisp#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Fixes openwisp#716
Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Fixes openwisp#716
Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Fixes openwisp#716
…p#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
…p#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
…p#716 Replaces direct use of MonitoringDeviceDetailSerializer with self.serializer_class to improve flexibility, allow overrides, and maintain consistent serializer usage across the view. Related to openwisp#716
- Updated the GitHub Actions `Install Dependencies` step to install Django before the project package in editable mode.
Replaced hardcoded color values in CSS files with the theme color variables defined in openwisp-utils, ensuring consistent styling across the OpenWISP project. Related to openwisp#720
|
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 the OpenWISP monitoring UI to use CSS variables from the openwisp-utils theme palette instead of hardcoded color values. Changes include replacing hex colors like Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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: 9
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/apps.py (1)
352-358:⚠️ Potential issue | 🔴 CriticalCSS variable
var(--ow-color-black)will not render in Plotly.js dashboard charts.The
colorsdictionary is passed toregister_dashboard_chart()and consumed by Plotly.js for rendering (lines 137, 300 inchart.js). Plotly.js only accepts hex, rgb/rgba, hsl/hsv, or named colors—not CSS custom properties. The string"var(--ow-color-black)"will be passed unchanged to Plotly, resulting in the "deactivated" status failing to render with a valid color.Use a hex value instead:
Proposed fix
"colors": { "ok": "#267126", "problem": "#ffb442", "critical": "#a72d1d", "unknown": "#353c44", - "deactivated": "var(--ow-color-black)", + "deactivated": "#000000", },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/apps.py` around lines 352 - 358, The "deactivated" color value in the colors dict passed to register_dashboard_chart() is using a CSS custom property ("var(--ow-color-black)") which Plotly.js won't accept; replace that value with the actual hex color used for --ow-color-black (e.g. "#000000" or the project's exact hex) so Plotly receives a valid color string. Update the colors dict in apps.py (the "deactivated" key) to the hex literal and ensure no other entries use CSS variables before calling register_dashboard_chart(); no changes are needed in chart.js.openwisp_monitoring/device/static/monitoring/js/device-map.js (1)
8-14:⚠️ Potential issue | 🟠 MajorResolve the CSS custom property before storing it in
STATUS_COLORS.
STATUS_COLORSvalues are consumed directly by ECharts for map rendering. The string"var(--ow-color-black)000"will be passed literally to the library and is not a valid color format—CSS variables are not resolved in JavaScript contexts. The "000" suffix suggests an attempt at string concatenation that results in invalid syntax.Suggested fix
window._owGeoMapConfig.STATUS_COLORS = { ok: "#267126", problem: "#ffb442", critical: "#a72d1d", unknown: "#353c44", - deactivated: "var(--ow-color-black)000", + deactivated: + getComputedStyle(document.documentElement) + .getPropertyValue("--ow-color-black") + .trim() || "#000000", };Note: The same pattern appears in
floorplan.jsand should be corrected there as well.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/static/monitoring/js/device-map.js` around lines 8 - 14, The deactivated color entry in window._owGeoMapConfig.STATUS_COLORS currently stores the literal string "var(--ow-color-black)000" which ECharts will receive as an invalid color; replace this by resolving the CSS custom property at runtime (e.g., read getComputedStyle(document.documentElement).getPropertyValue('--ow-color-black') or similar) and then build a valid color string (concatenate the resolved hex or convert to rgba with the intended alpha) and assign that result to the deactivated key in window._owGeoMapConfig.STATUS_COLORS; apply the same fix for the identical pattern in floorplan.js where a CSS variable is being inlined as a literal string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/developer/utils.rst`:
- Around line 69-73: The colors in the example use CSS variables that Plotly
cannot resolve, so update the code that builds/uses config.scale and
mapped.color (the places referenced as colorscale: config.scale and
options.marker.color.push(mapped.color)) to replace any "var(--...)" tokens with
concrete color values before passing them to Plotly; either hardcode hex/RGB
values in the "map" entries or resolve CSS variables at runtime (e.g., via
getComputedStyle on :root) and substitute those computed values into
config.scale and the mapped.color array so Plotly receives actual color strings
(the custom legend CSS can remain as-is).
In `@docs/user/settings.rst`:
- Line 519: The OPENWISP_MONITORING_CHARTS setting currently uses an invalid
color string "var(--ow-color-black)000" which concatenates a CSS variable and
suffix and is unsupported by Plotly; replace that entry under the "traffic" key
with a valid hex color (for example "#000000" or another concrete hex like
"#111111") and remove any CSS variable usage so Plotly receives only concrete
color values.
In `@openwisp_monitoring/device/static/monitoring/css/daterangepicker.css`:
- Line 35: Three CSS declarations use the invalid token "solidvar(...)" (e.g.,
the border-bottom and other border rules) which breaks the parser; locate the
occurrences of "solidvar(" in daterangepicker.css (search for "solidvar(" and
the related rules like the border-bottom declaration) and change them to "solid
var(...)" so the border and popup arrow fill styles are correctly applied.
In `@openwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.css`:
- Line 2: Update the background declaration to include a space after the colon
and wrap the url() argument in quotes: find the background property that
currently reads background:var(--ow-color-white) url(fullscreen.png) no-repeat 0
0; (the background property using url(fullscreen.png)) and change it to use a
space after the colon and quoted URL, e.g. background: var(--ow-color-white)
url("fullscreen.png") no-repeat 0 0; to satisfy stylelint and maintain
consistency.
In `@openwisp_monitoring/device/static/monitoring/css/netjsongraph.css`:
- Around line 47-50: The CSS rule for selector ".njg-container .marker-cluster
div" has inconsistent formatting for the "color" property; add a single space
after the colon so "color:var(--ow-color-white);" becomes "color:
var(--ow-color-white);" to match the project's spacing convention and keep the
other property "background-color: ..." unchanged.
- Around line 76-78: Update the CSS rule for the selector ".njg-container
.njg-tooltip" to add a space after the background colon for consistent
formatting; specifically change the "background:var(--ow-color-white)
!important;" declaration to include a space ("background: var(--ow-color-white)
!important;") while keeping the border rule and !important unchanged.
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Around line 353-357: The label.backgroundColor currently uses an unresolved
CSS var string ("var(--ow-color-black)000") which Canvas cannot parse; update
the chart config in floorplan.js to resolve the CSS custom property before
assigning backgroundColor: use
getComputedStyle(document.documentElement).getPropertyValue('--ow-color-black')
(trim the result) and convert it to a literal color string (e.g., append an
alpha channel correctly as "#rrggbbaa" or convert to "rgba(...)") and assign
that resolved string to label.backgroundColor in the ECharts option object so
the canvas receives a concrete color value.
In `@openwisp_monitoring/monitoring/static/monitoring/css/chart.css`:
- Around line 133-137: The CSS rule for the .tooltip class has an inconsistent
declaration "color:var(--ow-color-white)"; update the color property in the
.tooltip selector to include a space after the colon (i.e., "color:
var(--ow-color-white)") to match the formatting of other declarations in the
file and keep style consistency with the .tooltip rule.
- Around line 110-116: In the CSS rule for selector "#ow-chart-contents .circle
> span" remove the duplicated font-size declaration so only the intended value
remains (choose either the relative 0.15em or the absolute 13px) and ensure
there is a space after the colon in declarations (e.g., "color:
var(--ow-overlay-60) !important;") for consistent formatting.
---
Outside diff comments:
In `@openwisp_monitoring/device/apps.py`:
- Around line 352-358: The "deactivated" color value in the colors dict passed
to register_dashboard_chart() is using a CSS custom property
("var(--ow-color-black)") which Plotly.js won't accept; replace that value with
the actual hex color used for --ow-color-black (e.g. "#000000" or the project's
exact hex) so Plotly receives a valid color string. Update the colors dict in
apps.py (the "deactivated" key) to the hex literal and ensure no other entries
use CSS variables before calling register_dashboard_chart(); no changes are
needed in chart.js.
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Around line 8-14: The deactivated color entry in
window._owGeoMapConfig.STATUS_COLORS currently stores the literal string
"var(--ow-color-black)000" which ECharts will receive as an invalid color;
replace this by resolving the CSS custom property at runtime (e.g., read
getComputedStyle(document.documentElement).getPropertyValue('--ow-color-black')
or similar) and then build a valid color string (concatenate the resolved hex or
convert to rgba with the intended alpha) and assign that result to the
deactivated key in window._owGeoMapConfig.STATUS_COLORS; apply the same fix for
the identical pattern in floorplan.js where a CSS variable is being inlined as a
literal string.
🪄 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: c9f78acd-7700-4ce2-8290-64caca1aef24
⛔ Files ignored due to path filters (2)
openwisp_monitoring/device/static/monitoring/js/lib/netjsongraph.min.jsis excluded by!**/*.min.jsopenwisp_monitoring/monitoring/static/monitoring/js/lib/plotly-cartesian.min.jsis excluded by!**/*.min.js
📒 Files selected for processing (13)
.github/workflows/ci.ymldocs/developer/utils.rstdocs/user/settings.rstopenwisp_monitoring/device/apps.pyopenwisp_monitoring/device/static/monitoring/css/daterangepicker.cssopenwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.cssopenwisp_monitoring/device/static/monitoring/css/monitoring.cssopenwisp_monitoring/device/static/monitoring/css/netjsongraph.cssopenwisp_monitoring/device/static/monitoring/css/percircle.min.cssopenwisp_monitoring/device/static/monitoring/js/device-map.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/monitoring/static/monitoring/css/chart.css
📜 Review details
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2026-02-25T17:20:05.287Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/templates/admin/map/base_map.html:25-26
Timestamp: 2026-02-25T17:20:05.287Z
Learning: In openwisp-monitoring, base_map.html unconditionally applies the `dedicated-map-page` class to `#device-map-container`, but CSS conflicts are avoided by conditionally loading `monitoring.css`. MapPageAdmin includes `monitoring.css` (which contains `.dedicated-map-page` rules), while the dashboard map does not load `monitoring.css`, so the class has no effect on the dashboard.
Applied to files:
openwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.cssopenwisp_monitoring/device/apps.pyopenwisp_monitoring/device/static/monitoring/css/monitoring.cssopenwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_monitoring/device/static/monitoring/js/device-map.js
📚 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/device/apps.py
📚 Learning: 2026-03-10T22:15:52.380Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 755
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:45-47
Timestamp: 2026-03-10T22:15:52.380Z
Learning: In openwisp-monitoring PR `#755`, the indoor map URL fragment separator was intentionally changed from ":" to "_" (format: `<locationId>_<floor>`). This is a new feature with no existing saved/bookmarked URLs, so no backward compatibility with the old ":" separator format is required. The `_` separator is the canonical format going forward.
Applied to files:
openwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/device/static/monitoring/css/device-map.css
📚 Learning: 2026-02-21T18:29:38.883Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:253-258
Timestamp: 2026-02-21T18:29:38.883Z
Learning: In openwisp-monitoring, the `floor` field in the indoor coordinates API is always an integer representing floor numbers, so it doesn't require escaping when used in jQuery attribute selectors.
Applied to files:
openwisp_monitoring/device/static/monitoring/js/floorplan.js
🪛 Biome (2.4.6)
openwisp_monitoring/monitoring/static/monitoring/css/chart.css
[error] 114-114: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
openwisp_monitoring/device/static/monitoring/css/daterangepicker.css
[error] 35-35: Unexpected unknown function: solidvar
(lint/correctness/noUnknownFunction)
[error] 77-77: Unexpected unknown function: solidvar
(lint/correctness/noUnknownFunction)
[error] 141-141: Unexpected unknown function: solidvar
(lint/correctness/noUnknownFunction)
openwisp_monitoring/device/static/monitoring/css/percircle.min.css
[error] 1-1: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
🪛 Stylelint (17.4.0)
openwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.css
[error] 2-2: Expected quotes around "url" function argument (function-url-quotes)
(function-url-quotes)
openwisp_monitoring/device/static/monitoring/css/percircle.min.css
[error] 1-1: Unexpected deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
[error] 1-1: Unexpected deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
[error] 1-1: Unexpected deprecated property "clip" (property-no-deprecated)
(property-no-deprecated)
🔇 Additional comments (5)
openwisp_monitoring/device/static/monitoring/css/netjsongraph.css (1)
87-101: LGTM!The CSS variable substitutions for
.njg-tooltip-keyand.njg-tooltip-valueare correctly applied for CSS styling.openwisp_monitoring/device/static/monitoring/css/monitoring.css (3)
4-9: LGTM!The CSS variable substitutions for
#ow-device-status h2correctly replace hardcoded colors with theme variables.
32-35: LGTM!The background color substitution is correctly applied.
41-43: LGTM!The
.health-deactivatedcolor correctly uses the CSS variable for consistent theming..github/workflows/ci.yml (1)
84-86: LGTM!The simplified single-step Django installation with
-Uflag is cleaner than the previous uninstall/reinstall approach and correctly handles version upgrades.
| label: { | ||
| show: true, | ||
| color: "#ffffff", | ||
| backgroundColor: "#000000", | ||
| backgroundColor: "var(--ow-color-black)000", | ||
| borderWidth: 1, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check the exact content at lines 353-357 in the flagged file
cat -n openwisp_monitoring/device/static/monitoring/js/floorplan.js | sed -n '350,360p'Repository: openwisp/openwisp-monitoring
Length of output: 457
🌐 Web query:
ECharts label backgroundColor CSS variables resolution
💡 Result:
Apache ECharts label.backgroundColor (including rich-text blocks) expects a real color value (e.g. #rrggbb, rgba(...)) because labels are rendered by ECharts’ graphics layer (typically Canvas), and Canvas doesn’t resolve CSS var(--token) at draw time. ECharts maintainers have explicitly declined adding CSS-variable support for chart colors for this reason, recommending you resolve the variable yourself via getComputedStyle(...).getPropertyValue(...) before setOption. [1][2]
Use this pattern:
const cssVar = (name, el = document.documentElement) =>
getComputedStyle(el).getPropertyValue(name).trim();
option = {
series: [{
label: {
show: true,
backgroundColor: cssVar('--label-bg') // <- resolved to '#...' / 'rgb(...)'
}
}]
};
chart.setOption(option);If your CSS variables can change at runtime (theme switch), re-run setOption() with freshly resolved values. [2][3]
Sources:
[1] ECharts rich-text label docs showing label.backgroundColor usage: (echarts.apache.org)
[2] ECharts issue reference noting Canvas won’t recognize CSS variables and recommending getComputedStyle(...).getPropertyValue(...): (erkobridee.com)
[3] Canvas fillStyle / CSS variable resolution discussion (use getComputedStyle): (stackoverflow.com)
Citations:
- 1: https://echarts.apache.org/handbook/en/how-to/label/rich-text/?utm_source=openai
- 2: https://erkobridee.com/astro-ts-tw-svelte/echarts/references/
- 3: https://stackoverflow.com/questions/71368314/can-i-set-a-canvas-fillstyle-with-a-variable-from-css?utm_source=openai
🌐 Web query:
ECharts ZRender color format support CSS custom properties
💡 Result:
ZRender (and therefore ECharts’ canvas renderer) only parses “literal” CSS color strings (e.g. #f00/#ff0000, rgb()/rgba(), hsl()/hsla(), named colors) and does not list var(--token) as a supported input format. [1]
As a result, passing CSS custom properties like:
color: 'var(--brand)'is not reliably supported in ECharts—there’s a documented bug where bars can disappear on hover when colors are provided as CSS variables, and the issue was closed “not planned”. [2]
Workaround: resolve the variable yourself and pass a real color string into ECharts (e.g. getComputedStyle(el).getPropertyValue('--brand').trim()), then call setOption(...). [3]
Sources
[1] https://ecomfe.github.io/zrender-doc/public/api.html
[2] apache/echarts#19743
[3] https://erkobridee.com/astro-ts-tw-svelte/echarts/references/
Resolve CSS variable outside ECharts config before setting the label background color.
ECharts (ZRender) renders labels to Canvas and does not resolve CSS custom properties like var(--ow-color-black)000 at draw time. Canvas accepts only literal color strings (#rrggbb, rgb(...), named colors). This causes the background to fail silently or render unexpectedly. ECharts maintainers have documented this limitation and recommend resolving the variable yourself via getComputedStyle() before passing it to the chart config.
Proposed fix
nodeConfig: {
label: {
show: true,
color: "#ffffff",
- backgroundColor: "var(--ow-color-black)000",
+ backgroundColor:
+ getComputedStyle(document.documentElement)
+ .getPropertyValue("--ow-color-black")
+ .trim() || "#000000",
borderWidth: 1,
borderRadius: 8,
opacity: 1,
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| label: { | |
| show: true, | |
| color: "#ffffff", | |
| backgroundColor: "#000000", | |
| backgroundColor: "var(--ow-color-black)000", | |
| borderWidth: 1, | |
| label: { | |
| show: true, | |
| color: "#ffffff", | |
| backgroundColor: | |
| getComputedStyle(document.documentElement) | |
| .getPropertyValue("--ow-color-black") | |
| .trim() || "#000000", | |
| borderWidth: 1, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js` around lines
353 - 357, The label.backgroundColor currently uses an unresolved CSS var string
("var(--ow-color-black)000") which Canvas cannot parse; update the chart config
in floorplan.js to resolve the CSS custom property before assigning
backgroundColor: use
getComputedStyle(document.documentElement).getPropertyValue('--ow-color-black')
(trim the result) and convert it to a literal color string (e.g., append an
alpha channel correctly as "#rrggbbaa" or convert to "rgba(...)") and assign
that resolved string to label.backgroundColor in the ECharts option object so
the canvas receives a concrete color value.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
openwisp_monitoring/device/static/monitoring/js/floorplan.js (1)
421-421:⚠️ Potential issue | 🔴 CriticalMalformed CSS variable string and ECharts Canvas limitation.
The value
"var(--ow-color-black)000"appears to be a faulty find-replace of#000000, leaving a trailing000suffix. Even if corrected tovar(--ow-color-black), ECharts renders labels to Canvas, which cannot resolve CSS custom properties at draw time.As noted in the previous review, resolve the variable via JavaScript before passing to the chart config:
Proposed fix
nodeConfig: { label: { show: true, color: "#ffffff", - backgroundColor: "var(--ow-color-black)000", + backgroundColor: + getComputedStyle(document.documentElement) + .getPropertyValue("--ow-color-black") + .trim() || "#000000", borderWidth: 1, borderRadius: 8, opacity: 1, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js` at line 421, The chart option is using a malformed CSS string ("var(--ow-color-black)000") and ECharts draws to Canvas so CSS variables must be resolved in JS before usage; update the code that builds the ECharts options (where backgroundColor is set in floorplan.js) to read the actual CSS custom property (e.g., via getComputedStyle(document.documentElement).getPropertyValue('--ow-color-black') with a safe fallback like '#000000'), strip/normalize whitespace, and then assign that resolved color to the backgroundColor field passed to the chart config instead of the literal "var(...)" string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 85-88: Remove the duplicate Django installation in the CI workflow
by keeping only the later pip install -U ${{ matrix.django-version }}
invocation; specifically, delete the earlier pip install -U ${{
matrix.django-version }} that appears before pip install -r
requirements-test.txt so the step sequence is: pip install -r
requirements-test.txt, pip install -U -I -e ., pip install -U ${{
matrix.django-version }} (or alternatively keep only the first install and
remove the later one if you need Django available before installing
requirements), and do not bundle this CI tweak with the color-to-theme PR—if
this change is unrelated, split it into a dedicated PR.
---
Duplicate comments:
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Line 421: The chart option is using a malformed CSS string
("var(--ow-color-black)000") and ECharts draws to Canvas so CSS variables must
be resolved in JS before usage; update the code that builds the ECharts options
(where backgroundColor is set in floorplan.js) to read the actual CSS custom
property (e.g., via
getComputedStyle(document.documentElement).getPropertyValue('--ow-color-black')
with a safe fallback like '#000000'), strip/normalize whitespace, and then
assign that resolved color to the backgroundColor field passed to the chart
config instead of the literal "var(...)" string.
🪄 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: 9e3e20a5-2947-412b-b06d-8292615eb045
📒 Files selected for processing (2)
.github/workflows/ci.ymlopenwisp_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.11 | django~=5.1.0
- GitHub Check: Python==3.10 | 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.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- 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.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-03-10T22:15:52.380Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 755
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:45-47
Timestamp: 2026-03-10T22:15:52.380Z
Learning: In openwisp-monitoring PR `#755`, the indoor map URL fragment separator was intentionally changed from ":" to "_" (format: `<locationId>_<floor>`). This is a new feature with no existing saved/bookmarked URLs, so no backward compatibility with the old ":" separator format is required. The `_` separator is the canonical format going forward.
Applied to files:
openwisp_monitoring/device/static/monitoring/js/floorplan.js
📚 Learning: 2026-02-21T18:29:38.883Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:253-258
Timestamp: 2026-02-21T18:29:38.883Z
Learning: In openwisp-monitoring, the `floor` field in the indoor coordinates API is always an integer representing floor numbers, so it doesn't require escaping when used in jQuery attribute selectors.
Applied to files:
openwisp_monitoring/device/static/monitoring/js/floorplan.js
| pip install -U ${{ matrix.django-version }} | ||
| pip install -r requirements-test.txt | ||
| pip install -U -I -e . | ||
| pip install -U ${{ matrix.django-version }} |
There was a problem hiding this comment.
Duplicate Django installation and unrelated change to PR scope.
Line 85 installs the Django matrix version, but line 88 performs the same installation again. This creates redundancy:
- If the intent is to ensure the correct Django version after other packages run, only line 88 is necessary.
- If the intent is to have Django available before
requirements-test.txtruns, line 85 alone would suffice (assumingrequirements-test.txtdoesn't pin a conflicting Django version).
Additionally, this CI change appears unrelated to the PR's stated objective of replacing hardcoded color values with theme variables. Consider either:
- Removing the duplicate installation, or
- Splitting this CI change into a separate PR with appropriate context
Suggested fix (keep only the later install)
pip install -U pip wheel setuptools
- pip install -U ${{ matrix.django-version }}
pip install -r requirements-test.txt
pip install -U -I -e .
pip install -U ${{ matrix.django-version }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pip install -U ${{ matrix.django-version }} | |
| pip install -r requirements-test.txt | |
| pip install -U -I -e . | |
| pip install -U ${{ matrix.django-version }} | |
| pip install -r requirements-test.txt | |
| pip install -U -I -e . | |
| pip install -U ${{ matrix.django-version }} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/ci.yml around lines 85 - 88, Remove the duplicate Django
installation in the CI workflow by keeping only the later pip install -U ${{
matrix.django-version }} invocation; specifically, delete the earlier pip
install -U ${{ matrix.django-version }} that appears before pip install -r
requirements-test.txt so the step sequence is: pip install -r
requirements-test.txt, pip install -U -I -e ., pip install -U ${{
matrix.django-version }} (or alternatively keep only the first install and
remove the later one if you need Django available before installing
requirements), and do not bundle this CI tweak with the color-to-theme PR—if
this change is unrelated, split it into a dedicated PR.
Replaced hardcoded color values in CSS files with the theme color variables defined in openwisp-utils, ensuring consistent styling across the OpenWISP project. Related to openwisp#720
shakurah
force-pushed
the
issues/720-theme-colors-from-openwisp-utils
branch
from
3f7d960 to
75fb376
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_monitoring/tests/test_selenium.py (1)
807-847:⚠️ Potential issue | 🟠 MajorAdd a short animation-settle delay before map-coordinate assertions.
These two assertions query ECharts state immediately after coordinate updates and can still flake in CI due to animation timing. Add the same targeted pause used elsewhere in this file before each
WebDriverWait.Proposed stability patch
location.geometry = Point(12.513124, 41.898903) location.full_clean() location.save() + sleep(0.3) # Wait for JS animation try: series_value = WebDriverWait(self.web_driver, 5).until( lambda d: d.execute_script( @@ location.geometry = Point(12.511124, 41.898903) location.full_clean() location.save() + sleep(0.3) # Wait for JS animation try: series_value = WebDriverWait(self.web_driver, 5).until( lambda d: d.execute_script(Based on learnings: In Selenium tests with JS animations, insert a short sleep (e.g.,
sleep(0.3)) beforeWebDriverWaitassertions to reduce flakiness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/tests/test_selenium.py` around lines 807 - 847, The assertions read ECharts options immediately after updating coordinates and can flake due to animation timing; before each WebDriverWait call in this test (the two lambda d: d.execute_script(...) invocations that obtain series_value), insert the same short animation-settle pause used elsewhere in this file (e.g., sleep(0.3) or the project's helper pause) so the chart finishes animating; place the sleep just after saving the updated location (after location.save() and before the WebDriverWait) and also before the first WebDriverWait earlier in the test so both series_value reads use the pause.
♻️ Duplicate comments (3)
openwisp_monitoring/device/static/monitoring/css/daterangepicker.css (2)
77-77:⚠️ Potential issue | 🔴 CriticalCritical: Missing space breaks CSS border declaration.
The CSS parser will treat
solidvaras an unknown function and drop this entire declaration, removing the popup arrow styling.🐛 Proposed fix
- border-top: 6px solidvar(--ow-color-white); + border-top: 6px solid var(--ow-color-white);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/static/monitoring/css/daterangepicker.css` at line 77, The CSS rule for the popup arrow's border-top is malformed: change the value in the declaration that currently reads "border-top: 6px solidvar(--ow-color-white);" to include a space between "solid" and the CSS variable so it becomes "border-top: 6px solid var(--ow-color-white);" (update the declaration in daterangepicker.css where the border-top property is set).
141-141:⚠️ Potential issue | 🔴 CriticalCritical: Missing space breaks CSS border declaration.
Same issue as line 77 —
solidvaris not a valid CSS function. The calendar table border will not render.🐛 Proposed fix
- border: 1px solidvar(--ow-color-white); + border: 1px solid var(--ow-color-white);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/static/monitoring/css/daterangepicker.css` at line 141, The CSS border declaration in daterangepicker.css uses "solidvar(--ow-color-white)" which is invalid; update the border property to separate the keyword and the CSS variable (i.e., change the value to "1px solid var(--ow-color-white)") in the rule containing the border property and fix the identical instance referenced earlier (the one similar to line 77) so the calendar table border renders correctly.openwisp_monitoring/device/static/monitoring/js/floorplan.js (1)
420-421:⚠️ Potential issue | 🔴 CriticalCritical: ECharts/Canvas cannot resolve CSS variables.
ECharts renders labels to Canvas, and when using CSS variables like
var(--ow-color-white)for colors, "when you hover over each bar on the chart, each bar disappears instead of remain the same color." The ECharts team confirmed "This feature won't be added since Canvas doesn't recognize the CSS variables currently. It's suggested to usewindow.getComputedStyle(DOM).getPropertyValue('--the-css-var')to get real color values before callingsetOption."The label colors will fail silently or render unexpectedly. You must resolve the CSS variables to literal color values before passing them to the ECharts config.
🐛 Proposed fix — resolve CSS variables
+ // Helper to resolve CSS variables for Canvas-based rendering + const getCssVar = (name) => + getComputedStyle(document.documentElement).getPropertyValue(name).trim() || null; + function renderIndoorMap(allResults, imageUrl, divId, floor) { + const labelColor = getCssVar("--ow-color-white") || "#ffffff"; + const labelBgColor = getCssVar("--ow-color-black") || "#000000"; + const indoorMap = new NetJSONGraph(allResults, { el: `#${divId}`, render: "map", showLabelsAtZoomLevel: 0, mapOptions: { // ... nodeConfig: { label: { show: true, - color: "var(--ow-color-white)", - backgroundColor: "var(--ow-color-black)", + color: labelColor, + backgroundColor: labelBgColor, borderWidth: 1, borderRadius: 8, opacity: 1, }, },🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js` around lines 420 - 421, ECharts can't use CSS vars for canvas colors—replace usages of color: "var(--...)" and backgroundColor: "var(--...)" by reading the resolved values via getComputedStyle on the chart container before calling chart.setOption; specifically, locate where floorplan.js builds the ECharts option (references to color and backgroundColor in the options and the call to setOption) and compute literal colors like const white = getComputedStyle(chartContainer).getPropertyValue('--ow-color-white').trim() and similar for --ow-color-black, then assign those literal color strings into the option object before invoking chart.setOption so canvas rendering uses real color values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/device/static/monitoring/js/chart.css`:
- Around line 56-63: Remove the duplicate CSS rule for the .chart-heading
selector: locate both .chart-heading blocks (the one at lines shown in the diff
and the repeated block later) and delete the redundant copy so only a single
.chart-heading rule remains, preserving the existing properties (margin,
font-size, text-align, font-weight, color) in the remaining declaration.
- Around line 1-3: There are two duplicate CSS rule blocks for the selector
`#ow-chart-fallback`; merge them into one rule by locating both occurrences of the
selector (`#ow-chart-fallback`) in chart.css and combining their properties into a
single block (preserve all distinct declarations and remove the duplicate
block), ensuring no conflicting properties are left unresolved and ordering
stays logical.
- Line 49: The background declaration in chart.css uses url(...) without quotes
which Stylelint flags; update the background rule that currently reads
background: url(../../admin/img/icon-unknown.svg) 0 0 no-repeat; to wrap the
path in quotes (e.g. url("...")) so it becomes background:
url("..../icon-unknown.svg") 0 0 no-repeat; ensure you use the project's
preferred quote style (single or double) consistently for other url() usages.
In `@openwisp_monitoring/tests/test_selenium.py`:
- Around line 767-773: Duplicate tab-cleanup code that checks
self.web_driver.window_handles and closes additional tabs should be extracted
into a single helper method (e.g., cleanup_extra_tabs or _close_extra_tabs) on
the test class used in openwisp_monitoring/tests/test_selenium.py; implement the
helper to capture primary_tab = self.web_driver.window_handles[0], iterate over
self.web_driver.window_handles[1:], switch_to.window(tab) and close(), then
switch back to primary_tab, and replace both duplicated blocks (the one at the
shown location and the one at the other occurrence) with a single call to that
helper to ensure consistent teardown behavior.
---
Outside diff comments:
In `@openwisp_monitoring/tests/test_selenium.py`:
- Around line 807-847: The assertions read ECharts options immediately after
updating coordinates and can flake due to animation timing; before each
WebDriverWait call in this test (the two lambda d: d.execute_script(...)
invocations that obtain series_value), insert the same short animation-settle
pause used elsewhere in this file (e.g., sleep(0.3) or the project's helper
pause) so the chart finishes animating; place the sleep just after saving the
updated location (after location.save() and before the WebDriverWait) and also
before the first WebDriverWait earlier in the test so both series_value reads
use the pause.
---
Duplicate comments:
In `@openwisp_monitoring/device/static/monitoring/css/daterangepicker.css`:
- Line 77: The CSS rule for the popup arrow's border-top is malformed: change
the value in the declaration that currently reads "border-top: 6px
solidvar(--ow-color-white);" to include a space between "solid" and the CSS
variable so it becomes "border-top: 6px solid var(--ow-color-white);" (update
the declaration in daterangepicker.css where the border-top property is set).
- Line 141: The CSS border declaration in daterangepicker.css uses
"solidvar(--ow-color-white)" which is invalid; update the border property to
separate the keyword and the CSS variable (i.e., change the value to "1px solid
var(--ow-color-white)") in the rule containing the border property and fix the
identical instance referenced earlier (the one similar to line 77) so the
calendar table border renders correctly.
In `@openwisp_monitoring/device/static/monitoring/js/floorplan.js`:
- Around line 420-421: ECharts can't use CSS vars for canvas colors—replace
usages of color: "var(--...)" and backgroundColor: "var(--...)" by reading the
resolved values via getComputedStyle on the chart container before calling
chart.setOption; specifically, locate where floorplan.js builds the ECharts
option (references to color and backgroundColor in the options and the call to
setOption) and compute literal colors like const white =
getComputedStyle(chartContainer).getPropertyValue('--ow-color-white').trim() and
similar for --ow-color-black, then assign those literal color strings into the
option object before invoking chart.setOption so canvas rendering uses real
color values.
🪄 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: ac8c3bdb-37cd-4e14-b4cb-080f5c3d9c5b
📒 Files selected for processing (10)
.github/workflows/ci.ymlopenwisp_monitoring/device/static/monitoring/css/daterangepicker.cssopenwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.cssopenwisp_monitoring/device/static/monitoring/css/netjsongraph.cssopenwisp_monitoring/device/static/monitoring/js/chart.cssopenwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/device/static/monitoring/js/location-inline.jsopenwisp_monitoring/monitoring/static/monitoring/css/chart.cssopenwisp_monitoring/tests/test_selenium.py
📜 Review details
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 755
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:45-47
Timestamp: 2026-03-10T22:16:11.815Z
Learning: In openwisp-monitoring PR `#755`, the indoor map URL fragment separator was intentionally changed from ":" to "_" (format: `<locationId>_<floor>`). This is a new feature with no existing saved/bookmarked URLs, so no backward compatibility with the old ":" separator format is required. The `_` separator is the canonical format going forward.
📚 Learning: 2026-03-10T22:16:11.815Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 755
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:45-47
Timestamp: 2026-03-10T22:16:11.815Z
Learning: In openwisp-monitoring PR `#755`, the indoor map URL fragment separator was intentionally changed from ":" to "_" (format: `<locationId>_<floor>`). This is a new feature with no existing saved/bookmarked URLs, so no backward compatibility with the old ":" separator format is required. The `_` separator is the canonical format going forward.
Applied to files:
openwisp_monitoring/device/static/monitoring/js/location-inline.jsopenwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_monitoring/device/static/monitoring/js/floorplan.jsopenwisp_monitoring/tests/test_selenium.py
📚 Learning: 2026-02-21T18:29:38.883Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:253-258
Timestamp: 2026-02-21T18:29:38.883Z
Learning: In openwisp-monitoring, the `floor` field in the indoor coordinates API is always an integer representing floor numbers, so it doesn't require escaping when used in jQuery attribute selectors.
Applied to files:
openwisp_monitoring/device/static/monitoring/js/location-inline.jsopenwisp_monitoring/device/static/monitoring/js/floorplan.js
📚 Learning: 2026-02-25T17:20:05.287Z
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 738
File: openwisp_monitoring/device/templates/admin/map/base_map.html:25-26
Timestamp: 2026-02-25T17:20:05.287Z
Learning: In openwisp-monitoring, base_map.html unconditionally applies the `dedicated-map-page` class to `#device-map-container`, but CSS conflicts are avoided by conditionally loading `monitoring.css`. MapPageAdmin includes `monitoring.css` (which contains `.dedicated-map-page` rules), while the dashboard map does not load `monitoring.css`, so the class has no effect on the dashboard.
Applied to files:
openwisp_monitoring/device/static/monitoring/css/device-map.cssopenwisp_monitoring/device/static/monitoring/js/chart.cssopenwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.css
📚 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
🪛 Biome (2.4.6)
openwisp_monitoring/device/static/monitoring/js/chart.css
[error] 60-60: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
[error] 114-114: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
[error] 124-124: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
openwisp_monitoring/monitoring/static/monitoring/css/chart.css
[error] 114-114: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
openwisp_monitoring/device/static/monitoring/css/daterangepicker.css
[error] 77-77: Unexpected unknown function: solidvar
(lint/correctness/noUnknownFunction)
[error] 141-141: Unexpected unknown function: solidvar
(lint/correctness/noUnknownFunction)
🪛 Stylelint (17.4.0)
openwisp_monitoring/device/static/monitoring/js/chart.css
[error] 58-58: Unexpected duplicate "font-size" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
[error] 111-111: Unexpected duplicate "font-size" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
[error] 122-122: Unexpected duplicate "font-size" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
[error] 49-49: Expected quotes around "url" function argument (function-url-quotes)
(function-url-quotes)
🔇 Additional comments (20)
.github/workflows/ci.yml (1)
85-88: Duplicate Django installation and scope concern already flagged.The previous review comment correctly identified:
- Lines 85 and 88 both install
${{ matrix.django-version }}, creating redundancy- This CI modification is unrelated to the PR's objective of replacing hardcoded colors with theme variables
If the early install (line 85) is intentional to ensure Django is available for packages that import Django during setup, consider adding a clarifying comment. Otherwise, remove the duplicate or split this change into a dedicated CI PR.
openwisp_monitoring/device/static/monitoring/css/leaflet.fullscreen.css (1)
2-2: LGTM!The hardcoded
#fffhas been correctly replaced withvar(--ow-color-white), and the URL is properly quoted. This aligns with the PR objective of using centralized theme variables.openwisp_monitoring/device/static/monitoring/js/location-inline.js (1)
39-39: LGTM!The indoor map ID format correctly uses the underscore separator (
${locationId}_${floor}), aligning with the canonical format established in PR#755.openwisp_monitoring/monitoring/static/monitoring/css/chart.css (2)
113-118: LGTM!The color values have been correctly updated to use CSS variables (
var(--ow-overlay-60)andvar(--ow-color-black)), aligning with the centralized theming effort.
136-136: LGTM!The tooltip color correctly uses
var(--ow-color-white).openwisp_monitoring/device/static/monitoring/css/netjsongraph.css (3)
47-50: LGTM!The marker cluster text color correctly uses
var(--ow-color-white)with proper formatting.
76-78: LGTM!The tooltip background correctly uses
var(--ow-color-white).
87-100: LGTM!The tooltip key and value colors correctly use
var(--ow-color-black), replacing hardcoded#000.openwisp_monitoring/device/static/monitoring/css/device-map.css (5)
1-5: LGTM!The deactivated health status color correctly uses
var(--ow-color-black).
62-67: LGTM!The button text color correctly uses
var(--ow-color-white).
234-244: LGTM!The floorplan container correctly uses CSS variables for
box-shadow(var(--ow-overlay-40)) andbackground(var(--ow-color-white)).
262-269: LGTM!The floorplan title color correctly uses
var(--ow-color-fg-darker).
303-310: LGTM!The navigation body box-shadow correctly uses
var(--ow-overlay-40).openwisp_monitoring/device/static/monitoring/css/daterangepicker.css (1)
4-4: LGTM on the remaining color variable replacements.The other CSS variable substitutions throughout the file correctly replace hardcoded colors with theme variables.
Also applies to: 35-35, 110-110, 143-143, 166-166, 173-173, 189-189, 293-293, 344-344, 352-352, 364-365, 369-369, 378-378, 381-381, 385-385, 391-391, 395-395
openwisp_monitoring/device/static/monitoring/js/chart.css (1)
113-113: LGTM on the CSS variable usage.The color values correctly use CSS variables (
var(--ow-overlay-60),var(--ow-color-black),var(--ow-color-white)).Also applies to: 117-118, 136-136
openwisp_monitoring/device/static/monitoring/js/floorplan.js (4)
45-46: LGTM!The fragment parsing correctly uses the underscore separator format (
<locationId>_<floor>), aligning with the canonical format established in PR#755. Based on learnings, this is a new feature with no existing saved/bookmarked URLs requiring backward compatibility.
62-99: LGTM on navigation handlers.The
setupPopstateHandlerandsetupHashChangeHandlerfunctions correctly manage browser navigation and URL fragment changes. The handlers properly clean up previous listeners before registering new ones, preventing memory leaks.
338-353: LGTM!The
pushIndoorMapIdFragmentfunction correctly constructs and persists the indoor map fragment using the underscore format.
392-400: LGTM!The popup removal handler correctly removes the
nodeIdfrom URL fragments when the popup is closed.openwisp_monitoring/tests/test_selenium.py (1)
714-735: Fragment-format migration and coverage expansion look correct.The underscore-based indoor map fragment assertions and the new no-node-click fragment test coverage are aligned with the intended canonical format and improve confidence in hash sync behavior.
Based on learnings: the indoor map fragment separator was intentionally standardized to
_(<locationId>_<floor>) and is canonical going forward.Also applies to: 1011-1090, 1095-1279
| #ow-chart-fallback { | ||
| text-align: center; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate #ow-chart-fallback rule blocks.
This selector appears twice (lines 1-3 and 37-40) with different properties. These should be merged into a single rule block.
♻️ Proposed fix — merge into single rule
`#ow-chart-fallback` {
text-align: center;
+ display: none;
+ border: 0 none;
}
`#ow-chart-utils` {
display: flex;
...
-#ow-chart-fallback {
- display: none;
- border: 0 none;
-}Also applies to: 37-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/static/monitoring/js/chart.css` around lines 1 -
3, There are two duplicate CSS rule blocks for the selector `#ow-chart-fallback`;
merge them into one rule by locating both occurrences of the selector
(`#ow-chart-fallback`) in chart.css and combining their properties into a single
block (preserve all distinct declarations and remove the duplicate block),
ensuring no conflicting properties are left unresolved and ordering stays
logical.
| width: 22px; | ||
| height: 21px; | ||
| text-indent: -9999px; | ||
| background: url(../../admin/img/icon-unknown.svg) 0 0 no-repeat; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add quotes around URL function argument.
Stylelint expects quotes around the url() argument for consistency.
✨ Proposed fix
- background: url(../../admin/img/icon-unknown.svg) 0 0 no-repeat;
+ background: url("../../admin/img/icon-unknown.svg") 0 0 no-repeat;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| background: url(../../admin/img/icon-unknown.svg) 0 0 no-repeat; | |
| background: url("../../admin/img/icon-unknown.svg") 0 0 no-repeat; |
🧰 Tools
🪛 Stylelint (17.4.0)
[error] 49-49: Expected quotes around "url" function argument (function-url-quotes)
(function-url-quotes)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/static/monitoring/js/chart.css` at line 49, The
background declaration in chart.css uses url(...) without quotes which Stylelint
flags; update the background rule that currently reads background:
url(../../admin/img/icon-unknown.svg) 0 0 no-repeat; to wrap the path in quotes
(e.g. url("...")) so it becomes background: url("..../icon-unknown.svg") 0 0
no-repeat; ensure you use the project's preferred quote style (single or double)
consistently for other url() usages.
| .chart-heading { | ||
| margin: 0; | ||
| font-size: 2em; | ||
| text-align: center; | ||
| font-size: 1.22em; | ||
| font-weight: normal; | ||
| color: #434b62; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate .chart-heading rule blocks.
The .chart-heading selector is defined twice (lines 56-63 and 120-127) with identical content. Remove the duplicate.
♻️ Proposed fix — remove duplicate
`#ow-chart-contents` b {
background-color: var(--ow-color-black) !important;
}
-.chart-heading {
- margin: 0;
- font-size: 2em;
- text-align: center;
- font-size: 1.22em;
- font-weight: normal;
- color: `#434b62`;
-}
`#chart-loading-overlay` {Also applies to: 120-127
🧰 Tools
🪛 Biome (2.4.6)
[error] 60-60: Duplicate properties can lead to unexpected behavior and may override previous declarations unintentionally.
(lint/suspicious/noDuplicateProperties)
🪛 Stylelint (17.4.0)
[error] 58-58: Unexpected duplicate "font-size" (declaration-block-no-duplicate-properties)
(declaration-block-no-duplicate-properties)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/static/monitoring/js/chart.css` around lines 56 -
63, Remove the duplicate CSS rule for the .chart-heading selector: locate both
.chart-heading blocks (the one at lines shown in the diff and the repeated block
later) and delete the redundant copy so only a single .chart-heading rule
remains, preserving the existing properties (margin, font-size, text-align,
font-weight, color) in the remaining declaration.
| # Cleanup in case of failure to ensure the it does not end up with multiple tabs open | ||
| if len(self.web_driver.window_handles) > 1: | ||
| primary_tab = self.web_driver.window_handles[0] | ||
| for tab in self.web_driver.window_handles[1:]: | ||
| self.web_driver.switch_to.window(tab) | ||
| self.web_driver.close() | ||
| self.web_driver.switch_to.window(primary_tab) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract duplicated tab-cleanup logic into a helper.
The same cleanup block appears twice. A helper keeps teardown behavior consistent and reduces drift.
Refactor sketch
+ def _close_extra_tabs(self):
+ if len(self.web_driver.window_handles) <= 1:
+ return
+ primary_tab = self.web_driver.window_handles[0]
+ for tab in self.web_driver.window_handles[1:]:
+ self.web_driver.switch_to.window(tab)
+ self.web_driver.close()
+ self.web_driver.switch_to.window(primary_tab)
@@
- if len(self.web_driver.window_handles) > 1:
- primary_tab = self.web_driver.window_handles[0]
- for tab in self.web_driver.window_handles[1:]:
- self.web_driver.switch_to.window(tab)
- self.web_driver.close()
- self.web_driver.switch_to.window(primary_tab)
+ self._close_extra_tabs()
@@
- if len(self.web_driver.window_handles) > 1:
- primary_tab = self.web_driver.window_handles[0]
- for tab in self.web_driver.window_handles[1:]:
- self.web_driver.switch_to.window(tab)
- self.web_driver.close()
- self.web_driver.switch_to.window(primary_tab)
+ self._close_extra_tabs()Also applies to: 1302-1308
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/tests/test_selenium.py` around lines 767 - 773, Duplicate
tab-cleanup code that checks self.web_driver.window_handles and closes
additional tabs should be extracted into a single helper method (e.g.,
cleanup_extra_tabs or _close_extra_tabs) on the test class used in
openwisp_monitoring/tests/test_selenium.py; implement the helper to capture
primary_tab = self.web_driver.window_handles[0], iterate over
self.web_driver.window_handles[1:], switch_to.window(tab) and close(), then
switch back to primary_tab, and replace both duplicated blocks (the one at the
shown location and the one at the other occurrence) with a single call to that
helper to ensure consistent teardown behavior.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
.github/workflows/ci.yml (1)
85-88: 🧹 Nitpick | 🔵 TrivialDuplicate Django installation—consider clarifying intent or removing.
This change adds Django installation at line 85, but line 88 already installs the same version. The final installation at line 88 ensures the matrix Django version is used regardless of what
requirements-test.txttransitively installs (e.g., viaopenwisp-utils).If the intent is to have Django available before installing test requirements (some packages may need it during setup), add a comment explaining this. Otherwise, line 85 is redundant since line 88 already ensures the correct version.
Additionally, this CI change appears unrelated to the PR's stated objective of replacing hardcoded colors with theme variables—consider splitting it into a separate PR with appropriate context.
,
Option 1: Keep only line 88 (simpler, ensures final version)
pip install -U pip wheel setuptools - pip install -U ${{ matrix.django-version }} pip install -r requirements-test.txt pip install -U -I -e . pip install -U ${{ matrix.django-version }}Option 2: Keep both with explanatory comment
pip install -U pip wheel setuptools + # Install Django early so packages in requirements-test.txt can use it during setup pip install -U ${{ matrix.django-version }} pip install -r requirements-test.txt pip install -U -I -e . + # Re-install to override any Django version pulled transitively by openwisp-utils pip install -U ${{ matrix.django-version }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 85 - 88, The CI workflow duplicates the Django install (the pip install -U ${{ matrix.django-version }} command appears both before and after pip install -r requirements-test.txt), so either remove the earlier pip install and keep only the final pip install -U ${{ matrix.django-version }} to guarantee the matrix version, or if Django must be present before installing requirements (some packages may need it at setup time), keep both but add a clear comment above the first pip install explaining that intent (referencing the two commands: pip install -U ${{ matrix.django-version }} and pip install -r requirements-test.txt); also consider moving this CI change into a separate PR since it is orthogonal to the theme-variable refactor.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/ci.yml:
- Around line 85-88: The CI workflow duplicates the Django install (the pip
install -U ${{ matrix.django-version }} command appears both before and after
pip install -r requirements-test.txt), so either remove the earlier pip install
and keep only the final pip install -U ${{ matrix.django-version }} to guarantee
the matrix version, or if Django must be present before installing requirements
(some packages may need it at setup time), keep both but add a clear comment
above the first pip install explaining that intent (referencing the two
commands: pip install -U ${{ matrix.django-version }} and pip install -r
requirements-test.txt); also consider moving this CI change into a separate PR
since it is orthogonal to the theme-variable refactor.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8ec4044a-fce1-40f8-83ac-341cf1cadd0b
📒 Files selected for processing (1)
.github/workflows/ci.yml
📜 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~=5.1.0
- GitHub Check: Python==3.12 | 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.13 | django~=5.2.0
- GitHub Check: Python==3.13 | 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~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 755
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:45-47
Timestamp: 2026-03-10T22:16:11.815Z
Learning: In openwisp-monitoring PR `#755`, the indoor map URL fragment separator was intentionally changed from ":" to "_" (format: `<locationId>_<floor>`). This is a new feature with no existing saved/bookmarked URLs, so no backward compatibility with the old ":" separator format is required. The `_` separator is the canonical format going forward.
Replaced hardcoded color values in CSS files with the theme color variables defined in openwisp-utils, ensuring consistent styling across the OpenWISP project. Related to openwisp#720
…com:shakurah/openwisp-monitoring into issues/720-theme-colors-from-openwisp-utils
CI Failure AnalysisHello @shakurah, The CI run failed due to a Test Failure. Explanation: The logs indicate a "Fatal Python error: Aborted" with a "double free or corruption (fasttop)" message. This type of error often points to memory corruption issues within the Python interpreter or its underlying libraries, which can be triggered by complex interactions during test execution, especially those involving concurrency or resource management. The stack trace shows threads involved in Django's database operations and concurrent futures, suggesting the issue might be related to how these components handle resources or threads during the test suite. Remediation: The "double free or corruption" error is notoriously difficult to pinpoint without more specific debugging information. However, given the context of a test suite failure, the following steps are recommended:
|
Replaced hardcoded color values in CSS files with the theme color variables defined in openwisp-utils, ensuring consistent styling across the OpenWISP project. Related to openwisp#720
…com:shakurah/openwisp-monitoring into issues/720-theme-colors-from-openwisp-utils
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js`:
- Line 13: STATUS_COLORS contains an invalid hex for the deactivated key
("000"); update the deactivated color to a valid hex string by adding the "#"
prefix (e.g., "#000000" or the intended short form "#000"), or, to follow the PR
theming goal, replace the hardcoded value with the theme CSS variable (resolve
via getComputedStyle if ECharts needs a concrete color) — locate STATUS_COLORS
and the deactivated property in device-map.js and either change deactivated:
"000" to deactivated: "#000" (or a full hex) or read the variable from the
openwisp-utils theme and pass the computed value to ECharts.
🪄 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: 9b2dcecf-3532-4958-ba31-acb3cfe4ea1e
📒 Files selected for processing (1)
openwisp_monitoring/device/static/monitoring/js/device-map.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.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: dee077
Repo: openwisp/openwisp-monitoring PR: 755
File: openwisp_monitoring/device/static/monitoring/js/floorplan.js:45-47
Timestamp: 2026-03-10T22:16:11.815Z
Learning: In openwisp-monitoring PR `#755`, the indoor map URL fragment separator was intentionally changed from ":" to "_" (format: `<locationId>_<floor>`). This is a new feature with no existing saved/bookmarked URLs, so no backward compatibility with the old ":" separator format is required. The `_` separator is the canonical format going forward.
| critical: "#a72d1d", | ||
| unknown: "#353c44", | ||
| deactivated: "#000000", | ||
| deactivated: "000", |
There was a problem hiding this comment.
Invalid hex color value: missing "#" prefix.
The value "000" is not a valid CSS/ECharts color. All other colors in STATUS_COLORS use the # prefix (e.g., "#267126"). This will cause deactivated device markers to render with incorrect or fallback colors on the map.
Given the PR objective is to use theme color variables from openwisp-utils, consider using the appropriate CSS variable instead. If a hardcoded value is required here (e.g., for ECharts which may not support CSS variables), restore the valid hex format.
🐛 Proposed fix
If using a hardcoded hex value:
- deactivated: "000",
+ deactivated: "#000",Or if ECharts supports CSS variable resolution via getComputedStyle, consider aligning with the PR's theming objective by using the appropriate variable.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| deactivated: "000", | |
| deactivated: "#000", |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@openwisp_monitoring/device/static/monitoring/js/device-map.js` at line 13,
STATUS_COLORS contains an invalid hex for the deactivated key ("000"); update
the deactivated color to a valid hex string by adding the "#" prefix (e.g.,
"#000000" or the intended short form "#000"), or, to follow the PR theming goal,
replace the hardcoded value with the theme CSS variable (resolve via
getComputedStyle if ECharts needs a concrete color) — locate STATUS_COLORS and
the deactivated property in device-map.js and either change deactivated: "000"
to deactivated: "#000" (or a full hex) or read the variable from the
openwisp-utils theme and pass the computed value to ECharts.
1 participant
Checklist
Reference to Existing Issue
Closes #720 .
Description of Changes
Please describe these changes.
Replaced hardcoded color values in CSS files with the theme color
variables defined in openwisp-utils, ensuring consistent styling
across the OpenWISP project.