feat: Implement dark mode support for map and UI #483
Conversation
WalkthroughThis PR adds dark-mode support across the NetJSONGraph UI and map rendering. It introduces a new Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant GUI as NetJSONGraphGUI
participant Storage as localStorage
participant Doc as Document (CSS classes)
participant Render as NetJSONGraphRender
participant Leaflet as Leaflet Map
Note over GUI,Render: Initialization flow
User->>GUI: init()
GUI->>GUI: createThemeControl()
GUI->>Storage: read "map_theme"
Storage-->>GUI: theme value (light/dark/null)
GUI->>Doc: add/remove "dark-mode" class
GUI-->>User: show theme control
Note over User,GUI: User toggles theme
User->>GUI: click theme toggle
GUI->>Doc: toggle "dark-mode" class
GUI->>Storage: write "map_theme"
GUI->>Render: trigger re-render
Note over Render,Leaflet: Map tile selection and update
Render->>Doc: isDarkMode() checks classes
alt Dark mode detected
Render->>Render: select mapTileConfigDark
else Light mode
Render->>Render: select mapTileConfig
end
Render->>Leaflet: remove old TileLayer(s), add new TileLayer(s)
Leaflet-->>User: render tiles with selected theme
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 3
🤖 Fix all issues with AI Agents
In @src/js/netjsongraph.gui.js:
- Around line 54-56: Replace the truthy-function check with a comparison against
the active render mode: instead of testing `this.self.utils.mapRender` directly,
verify `this.self.config.render === this.self.utils.mapRender` and only then
call `this.self.utils.render()` so re-rendering occurs only when map mode is
active; update the conditional around `this.self.utils.render` accordingly.
In @src/js/netjsongraph.render.js:
- Around line 401-407: Extract the duplicated dark-mode detection into a single
helper method (e.g., isDarkMode()) on the same renderer class used in this file;
implement it to consistently check document.body.classList,
document.documentElement.classList, and self.el (if present) for "dark-mode" and
return a boolean, then replace both inline checks (the const isDarkMode = ...
block around lines ~401 and the similar block around lines ~498) with calls to
this.new helper to remove duplication and ensure identical behavior.
In @test/netjsongraph.render.test.js:
- Around line 945-947: The mock Leaflet object defines addLayer twice (duplicate
of addLayer alongside eachLayer and removeLayer); remove the redundant addLayer
entry so the mock only declares addLayer once (keep the intended
implementation), update any references if needed, and run the tests to confirm
no behavioral change from the deduped mock in netjsongraph.render.test.js.
🧹 Nitpick comments (3)
src/js/netjsongraph.render.js (3)
469-471: Validate dark mode tile config to improve error messaging.The validation at line 469 only checks
mapTileConfig[0], but when dark mode is active, the code usesmapTileConfigDarkif available. If a developer only configuresmapTileConfigDarkand enables dark mode by default, they'll get a misleading error aboutmapTileConfig.🔎 Proposed validation enhancement
- if (!self.config.mapTileConfig[0]) { - throw new Error(`You must add the tiles via the "mapTileConfig" param!`); + const isDarkMode = this.isDarkModeActive(self); + const activeTileConfig = isDarkMode && self.config.mapTileConfigDark + ? self.config.mapTileConfigDark + : self.config.mapTileConfig; + + if (!activeTileConfig || !activeTileConfig[0]) { + const configName = isDarkMode && self.config.mapTileConfigDark + ? "mapTileConfigDark" + : "mapTileConfig"; + throw new Error(`You must add the tiles via the "${configName}" param!`); }
508-518: Manual tile layer management may cause visual flicker during theme toggle.The approach of removing all
TileLayerinstances and re-adding them (lines 509-517) is a workaround for echarts-leaflet not updating tiles onsetOption. This can cause a brief flash where no tiles are visible during the transition.Consider these alternatives:
- Check if the current tile layer URL matches the target before removing/re-adding
- Add new layers before removing old ones to maintain continuous coverage
- Investigate if echarts-leaflet has a supported API for updating tile configurations
🔎 Alternative approach to minimize flicker
if (self.leaflet) { + // Collect existing tile layers + const existingLayers = []; self.leaflet.eachLayer((layer) => { if (layer instanceof L.TileLayer) { - self.leaflet.removeLayer(layer); + existingLayers.push(layer); } }); + // Add new tile layers first targetTiles.forEach((tile) => { L.tileLayer(tile.urlTemplate, tile.options).addTo(self.leaflet); }); + + // Then remove old layers to maintain continuous coverage + existingLayers.forEach(layer => { + self.leaflet.removeLayer(layer); + }); }
493-495: Extensive Leaflet guards in map rendering function.The code adds numerous guards checking
if (self.leaflet)before accessing Leaflet methods throughout themapRenderfunction. While defensive programming is good, this function inherently requires Leaflet to work (map tiles, coordinates, zoom, etc.).The extensive guards suggest one of two scenarios:
- Leaflet might not be initialized at certain points during the render lifecycle
- The code is overly defensive for a dependency that should always be present
If Leaflet can legitimately be absent at these points, consider failing fast at the beginning of
mapRenderwith a clear error. If these guards are purely defensive against edge cases, they're well-implemented but may indicate an architectural issue worth investigating.Also applies to: 589-630, 634-685, 693-754
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
public/example_templates/netjsonmap.htmlsrc/css/netjsongraph-theme.csssrc/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jstest/darkmode.test.jstest/netjsongraph.dom.test.jstest/netjsongraph.render.test.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T08:00:40.936Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:40.936Z
Learning: In the netjsongraph.js codebase, the `== null` pattern is intentionally used to check for both null and undefined values in a single comparison, which is a well-established JavaScript idiom and should not be flagged for replacement with strict equality checks.
Applied to files:
test/netjsongraph.dom.test.js
🧬 Code graph analysis (1)
test/darkmode.test.js (2)
test/netjsongraph.render.test.js (3)
JSONData(5-9)graph(36-36)graph(299-299)test/netjsongraph.dom.test.js (1)
graph(5-13)
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (6)
test/netjsongraph.dom.test.js (1)
306-307: LGTM!The test expectations correctly validate that the GUI initialization now creates a controls container and includes the new theme control element. This aligns well with the dark mode feature implementation.
test/darkmode.test.js (2)
13-33: Test setup looks reasonable.The beforeEach setup correctly initializes the graph and render instances with mock tile configurations. The assignment of
rendertograph.utils(line 32) enables the tests to callrender.generateMapOptionwith the graph context, which is appropriate for isolated unit testing of the render logic.
35-72: Comprehensive dark mode test coverage.The test suite effectively validates all key scenarios:
- Default light mode behavior
- Dark mode activation via body or html element classes
- Graceful fallback when dark configuration is absent
Each test properly cleans up DOM state, preventing side effects between tests.
test/netjsongraph.render.test.js (1)
500-502: Test mocks properly updated for new tile configuration structure.The updates to
mapTileConfigand Leaflet mock methods correctly align test scaffolding with the new dark mode tile configuration structure. The consistent use of the OpenStreetMap tile URL template with proper placeholder syntax ensures tests remain valid.Also applies to: 968-970, 1070-1072, 1177-1179, 1214-1216, 1387-1389, 1439-1441, 1449-1451
src/js/netjsongraph.config.js (1)
260-269: CartoDB tile configuration is valid for production.The Dark Matter tiles from
basemaps.cartocdn.comare publicly available and do not require API keys. The URL endpoint is accessible, the attribution is correct, and the subdomain distribution is properly configured.src/css/netjsongraph-theme.css (1)
91-157: Dark mode styles are comprehensive and well-structured.The dark mode implementation covers all necessary UI elements (sidebar, tooltips, search button, switches, theme control) with appropriate color choices and includes responsive behavior for small screens. The styling is clean and follows a consistent dark theme pattern.
src/js/netjsongraph.render.js
Outdated
| const isDarkMode = | ||
| document.body.classList.contains("dark-mode") || | ||
| document.documentElement.classList.contains("dark-mode"); | ||
| const tiles = | ||
| isDarkMode && configs.mapTileConfigDark | ||
| ? configs.mapTileConfigDark | ||
| : configs.mapTileConfig; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate dark mode detection logic with inconsistent element checks.
Dark mode detection is duplicated in two places with slightly different implementations:
- Lines 401-403 check
document.bodyanddocument.documentElement - Lines 498-501 additionally check
self.el
This duplication creates maintenance burden and the inconsistency could lead to different behavior in edge cases.
🔎 Proposed refactor to extract dark mode detection
Add a helper method to the class:
+ isDarkModeActive(self) {
+ return (
+ (self.el && self.el.classList.contains("dark-mode")) ||
+ document.body.classList.contains("dark-mode") ||
+ document.documentElement.classList.contains("dark-mode")
+ );
+ }
+
generateMapOption(JSONData, self, clusters = []) {
// ... existing code ...
- const isDarkMode =
- document.body.classList.contains("dark-mode") ||
- document.documentElement.classList.contains("dark-mode");
+ const isDarkMode = this.isDarkModeActive(self);
const tiles =
isDarkMode && configs.mapTileConfigDark
? configs.mapTileConfigDark
: configs.mapTileConfig;Then use the same helper at lines 498-506:
- const isDarkMode =
- (self.el && self.el.classList.contains("dark-mode")) ||
- document.body.classList.contains("dark-mode") ||
- document.documentElement.classList.contains("dark-mode");
+ const isDarkMode = this.isDarkModeActive(self);
const targetTiles =
isDarkMode && self.config.mapTileConfigDark
? self.config.mapTileConfigDark
: self.config.mapTileConfig;Also applies to: 498-506
🤖 Prompt for AI Agents
In @src/js/netjsongraph.render.js around lines 401 - 407, Extract the duplicated
dark-mode detection into a single helper method (e.g., isDarkMode()) on the same
renderer class used in this file; implement it to consistently check
document.body.classList, document.documentElement.classList, and self.el (if
present) for "dark-mode" and return a boolean, then replace both inline checks
(the const isDarkMode = ... block around lines ~401 and the similar block around
lines ~498) with calls to this.new helper to remove duplication and ensure
identical behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In @src/js/netjsongraph.gui.js:
- Around line 43-46: Wrap the localStorage access for theme in try-catch: when
reading the theme (the current localStorage.getItem("map_theme") usage that
assigns savedTheme and conditions calling
this.self.el.classList.add("dark-mode")) wrap the getItem in try/catch and treat
failures as no saved theme (don’t throw), and when persisting the theme (the
localStorage.setItem call around line 51) wrap setItem in try/catch and silently
ignore storage errors (optionally log to console.debug) so the UI initialization
and theme toggling (the savedTheme variable, localStorage.getItem,
localStorage.setItem, and this.self.el.classList.add calls) won’t break if
storage is disabled.
- Around line 32-34: The theme toggle icon span (variable "icon") currently uses
an emoji which may not be announced by screen readers; update the span to
improve accessibility by adding an appropriate ARIA attribute: if the emoji is
purely decorative set aria-hidden="true" and role="img" as needed, or provide an
explicit aria-label (e.g., aria-label="Toggle theme" or aria-hidden="true" if
decorative) so assistive tech gets the correct intent; locate the code that
creates the span ("const icon = document.createElement('span')", "icon.innerHTML
= '🌓'") and add the chosen ARIA attribute and/or role accordingly.
🧹 Nitpick comments (1)
src/js/netjsongraph.gui.js (1)
315-316: Consider making theme control creation configurable.The comment suggests making the theme control configurable. While the current unconditional creation works, consider adding a config option (e.g.,
showThemeToggle: true) for consistency with other UI controls likeswitchModeand to allow users to hide it if they implement custom theme switching.🔧 Proposed refactor for configurability
In your default config (likely in
netjsongraph.config.js):showThemeToggle: true,Then update the init method:
init() { this.sideBar = this.createSideBar(); - // Always create theme control for now, or make it configurable - this.createThemeControl(); + if (this.self.config.showThemeToggle) { + this.createThemeControl(); + } if (this.self.config.switchMode) { if (!this.controls) { this.controls = this.createControls(); } this.renderModeSelector = this.createRenderModeSelector(); } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jstest/netjsongraph.render.test.js
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-01-04T08:00:40.936Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:40.936Z
Learning: In the netjsongraph.js codebase, the `== null` pattern is intentionally used to check for both null and undefined values in a single comparison, which is a well-established JavaScript idiom and should not be flagged for replacement with strict equality checks.
Applied to files:
src/js/netjsongraph.gui.js
⏰ 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). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (4)
test/netjsongraph.render.test.js (1)
500-502: LGTM! Test scaffolding properly updated for dark mode support.The test mocks have been consistently updated to support the new dark-mode feature:
mapTileConfignow uses proper array structure with tile configuration objectsisDarkModemocks added throughout to simulate theme-aware behavior- Leaflet instance mocks include
eachLayerandremoveLayerfor tile layer managementAll changes align with the dark-mode implementation in the production code.
Also applies to: 506-506, 946-948, 969-971, 981-981, 1072-1074, 1085-1085, 1217-1219, 1231-1231, 1291-1291, 1327-1327, 1393-1395, 1399-1399, 1446-1448, 1456-1458, 1470-1470
src/js/netjsongraph.render.js (3)
41-47: LGTM! Dark mode detection properly consolidated.The
isDarkMode(self)helper consolidates dark-mode detection logic into a single reusable method that checks the element, body, and document classes. This addresses the previous duplication concern and ensures consistent behavior throughout the codebase.
411-419: Dark-mode tile selection implemented correctly.The dark-mode aware tile selection properly uses
isDarkMode(self)to choose betweenmapTileConfigDarkandmapTileConfig. The manual tile layer management inmapRender(lines 506-523) is a necessary workaround because echarts-leaflet doesn't automatically handle tile updates onsetOption. The implementation correctly:
- Removes existing TileLayer instances
- Adds new tiles based on current dark mode state
- Guards all operations with Leaflet availability checks
Also applies to: 506-523
501-503: LGTM! Leaflet operations properly guarded.All Leaflet-dependent operations are now correctly guarded with
if (self.leaflet)checks, preventing runtime errors when Leaflet is unavailable. This defensive programming approach ensures the code gracefully handles mixed rendering environments while preserving existing behavior when Leaflet is present.Key improvements:
- Tile layer management guarded (lines 513-523)
- Bounds fitting guarded (line 571)
- Zoom-based label toggling guarded (lines 576-592, 594-635)
- Move-end data fetching guarded (lines 639-690)
- Clustering zoom handlers guarded (lines 698-759)
Also applies to: 513-523, 571-573, 576-592, 594-635, 639-690, 698-759
| const icon = document.createElement("span"); | ||
| icon.innerHTML = "🌓"; | ||
| icon.style.marginRight = "5px"; |
There was a problem hiding this comment.
Improve accessibility of the theme toggle icon.
The emoji "🌓" may not be announced properly by screen readers. Consider adding an aria-label attribute to the icon span, or use an SVG/icon font with proper ARIA attributes for better accessibility.
♿ Proposed fix for better accessibility
const icon = document.createElement("span");
- icon.innerHTML = "🌓";
+ icon.innerHTML = "🌓";
+ icon.setAttribute("aria-hidden", "true");
icon.style.marginRight = "5px";Mark the emoji as decorative since the text label already conveys the meaning. Alternatively, if you want the icon to be semantic:
const icon = document.createElement("span");
icon.innerHTML = "🌓";
+ icon.setAttribute("role", "img");
+ icon.setAttribute("aria-label", "Theme toggle");
icon.style.marginRight = "5px";📝 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.
| const icon = document.createElement("span"); | |
| icon.innerHTML = "🌓"; | |
| icon.style.marginRight = "5px"; | |
| const icon = document.createElement("span"); | |
| icon.innerHTML = "🌓"; | |
| icon.setAttribute("aria-hidden", "true"); | |
| icon.style.marginRight = "5px"; |
🤖 Prompt for AI Agents
In @src/js/netjsongraph.gui.js around lines 32 - 34, The theme toggle icon span
(variable "icon") currently uses an emoji which may not be announced by screen
readers; update the span to improve accessibility by adding an appropriate ARIA
attribute: if the emoji is purely decorative set aria-hidden="true" and
role="img" as needed, or provide an explicit aria-label (e.g.,
aria-label="Toggle theme" or aria-hidden="true" if decorative) so assistive tech
gets the correct intent; locate the code that creates the span ("const icon =
document.createElement('span')", "icon.innerHTML = '🌓'") and add the chosen
ARIA attribute and/or role accordingly.
| const savedTheme = localStorage.getItem("map_theme"); | ||
| if (savedTheme === "dark") { | ||
| this.self.el.classList.add("dark-mode"); | ||
| } |
There was a problem hiding this comment.
Add error handling for localStorage access.
The localStorage.getItem and localStorage.setItem calls can throw a SecurityError in private browsing mode or when storage is disabled. Wrap these calls in try-catch blocks to prevent the theme control from breaking the entire UI initialization.
🛡️ Proposed fix to add error handling
- const savedTheme = localStorage.getItem("map_theme");
- if (savedTheme === "dark") {
- this.self.el.classList.add("dark-mode");
- }
+ try {
+ const savedTheme = localStorage.getItem("map_theme");
+ if (savedTheme === "dark") {
+ this.self.el.classList.add("dark-mode");
+ }
+ } catch (e) {
+ // Silently fall back to light mode if localStorage is unavailable
+ }And for the setter:
themeControl.onclick = () => {
this.self.el.classList.toggle("dark-mode");
const isDark = this.self.el.classList.contains("dark-mode");
- localStorage.setItem("map_theme", isDark ? "dark" : "light");
+ try {
+ localStorage.setItem("map_theme", isDark ? "dark" : "light");
+ } catch (e) {
+ // Theme will still toggle visually, just won't persist
+ }
// If map is active, re-render to update tiles
if (this.self.config.render === this.self.utils.mapRender) {
this.self.utils.render();
}
};Also applies to: 51-51
🤖 Prompt for AI Agents
In @src/js/netjsongraph.gui.js around lines 43 - 46, Wrap the localStorage
access for theme in try-catch: when reading the theme (the current
localStorage.getItem("map_theme") usage that assigns savedTheme and conditions
calling this.self.el.classList.add("dark-mode")) wrap the getItem in try/catch
and treat failures as no saved theme (don’t throw), and when persisting the
theme (the localStorage.setItem call around line 51) wrap setItem in try/catch
and silently ignore storage errors (optionally log to console.debug) so the UI
initialization and theme toggling (the savedTheme variable,
localStorage.getItem, localStorage.setItem, and this.self.el.classList.add
calls) won’t break if storage is disabled.
1 participant
Checklist
Reference to Existing Issue
Closes #475
Description of Changes
This PR implements a comprehensive Dark Mode feature for the NetJSONGraph map interface.
Key Changes:
netjsongraph-theme.cssto provide dark mode styles for the sidebar, tooltips, metadata container, and the new toggle button.localStorage(key:map_theme), persisting across page reloads.test/darkmode.test.jsto verify theme toggling and persistence.test/netjsongraph.render.test.jsandtest/netjsongraph.dom.test.jsto cover new logic and fix existing test gaps.