[feature] Update URL with node click of map interactions #238

[dee077]

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Closes #238 .

Description of Changes

• Update URL query params on map interactions (zoom, move, bounds change)
• Add mode in URL to differentiate indoor map and geo-map
• Open floorplan overlay when in indoor map mode

[nemesifier]

As discussed in our weekly meeting, the result doesn't seem to match the specs described in #238.

Please read again the specs and ask any question if not clear.

[dee077]

Screencast.from.2025-09-12.02-47-39.webm

[pandafy]

Good work @Deepanshu! This is going to be a very user feature in the library. 💪🏼

I found a corner case while going back through the history of the browser using the back button. I expected the map to update when the URL fragment updates, but that is not the case here.

I also explored about enabling this feature by default. If ID is not provided in the config, we can generate a hash based on the HTML element. This can have side-effects, so let's think before going ahead in this direction.

Maybe, auto-generating ID is a bad idea. If the hash will change if the user changed the HTML element. This will render old URLs useless.

diff --git a/src/js/netjsongraph.config.js b/src/js/netjsongraph.config.js
index b4fcc67..444d813 100644
--- a/src/js/netjsongraph.config.js
+++ b/src/js/netjsongraph.config.js
@@ -285,7 +285,7 @@ const NetJSONGraphDefaultConfig = {
   ],
   linkCategories: [],
   urlFragments: {
-    show: false,
+    show: true,
     id: null,
   },
 
diff --git a/src/js/netjsongraph.core.js b/src/js/netjsongraph.core.js
index 1d448d1..1758e0b 100644
--- a/src/js/netjsongraph.core.js
+++ b/src/js/netjsongraph.core.js
@@ -57,6 +57,19 @@ class NetJSONGraph {
       console.error("Can't change el again!");
     }
 
+    if (!this.config.urlFragments.id) {
+      // The config does not specify a an ID, we assign one by creating
+      // a hash of the HTML element using FNV-1a algorithm
+      let str = this.el.outerHTML
+      let hash = 0x811c9dc5; // FNV offset basis
+      for (let i = 0; i < str.length; i++) {
+        hash ^= str.charCodeAt(i);
+        hash = (hash * 0x01000193) >>> 0; // FNV prime
+
+      }
+      this.config.urlFragments.id = hash.toString();
+    }
+
     return this.config;
   }

[pandafy]

I also explored about enabling this feature by default. If ID is not provided in the config, we can generate a hash based on the HTML element. This can have side-effects, so let's think before going ahead in this direction

Maybe, this is a bad idea. What if the user updates the HTML element? Then, the old URLs will no longer work.

[pandafy]

Good work @dee077! 👏🏼 👏🏼 👏🏼

It works as expected.

We need to add a lot of comments in the code to explain our design decision. Otherwise, we will forget about why we chose to implement the feature this way and then search for the details in issues.

[dee077]

Updates

• Added a new object in the netjsongraph instance this.nodeIndex, which contains the key as node.id or sourceNodeId-targetNodeId in case of a link, so that it can be looked afterwards by this.data.nodes[index] or this.data.links[index].
• Updated the tests with these new changes and added new unit and browser tests for case of links.
• Did not add browser tests for adding nodes to url on click on the node or link, as we currently don't have resilient logic to trigger the click event of the node or link in selenium tests. Moving the mouse programmatically and triggering a click will cause unexpected failure on different window sizes.
• Adding the value for nodeIndex as index of the array present this.data.nodes or this.data.nodes was a bad idea, as it cannot be sure the array will be in the same sequence every time. On monitoring found that the index is getting changed on readering a url in a new tab.
• So in netjsongraph.core.js we are already traversing the nodes and links to set the this.data, so I created a shallow copy of it in this.nodeLinkIndex as key of node id or source~target for further looks without need for a traversal.
• Used ~ as a separator in case of setting link key in dict as source~target, uuid can have - and give unexpected results

[nemesifier]

@coderabbitai review

[nemesifier]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Caution

Review failed

An error occurred during the review process. Please try again later.

Walkthrough

This PR adds a bookmarkable actions feature to NetJSONGraph via a new bookmarkableActions config. Clicks on nodes/links persist an id in the URL hash (nodeId or source~target) using history.pushState; on load or popstate the library parses fragments, validates against an internal node/link index, optionally centers/zooms the view, and triggers the element click handler. Render now awaits async onReady before applying URL fragments. New util methods, example pages, README docs, and test coverage were added.

Sequence Diagram(s)

sequenceDiagram
    participant User
    participant Graph as NetJSONGraph
    participant Util as URLUtil
    participant Browser as Browser History
    participant URL as Page URL

    rect rgb(240,248,255)
    note over User,URL: Bookmark Action Flow
    User->>Graph: Click node/link
    Graph->>Util: addActionToUrl(params)
    Util->>Util: parseUrlFragments()
    Util->>Util: encode fragment (id or source~target)
    Util->>URL: update location.hash (semicolon-delimited)
    Util->>Browser: pushState(...)
    Graph->>Graph: emit applyUrlFragmentState
    end

Loading

sequenceDiagram
    participant Browser
    participant Graph as NetJSONGraph
    participant Util as URLUtil
    participant Renderer as Map/Chart Renderer

    rect rgb(240,255,240)
    note over Browser,Renderer: Restore from URL Fragment Flow
    Browser->>Graph: Page load (URL contains fragments)
    Graph->>Graph: start render()
    Graph->>Graph: await onReady (onReadyDone)
    Graph->>Util: applyUrlFragmentState(self)
    Util->>Util: parseUrlFragments()
    Util->>Renderer: compute target coords / zoom
    Util->>Graph: trigger onClickElement / open details
    Renderer->>Browser: update view (pan/zoom)
    Browser->>Util: popstate (back/forward)
    Util->>Graph: applyUrlFragmentState(self) (on popstate)
    end

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Pre-merge checks and finishing touches

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	Title clearly indicates adding URL updates on map interactions and references issue #238, directly matching the primary changeset objective.
Description check	✅ Passed	Description covers main changes (URL updates on interactions, mode parameter, floorplan overlay), references issue #238, but marks tests and documentation as incomplete.
Linked Issues check	✅ Passed	Changes comprehensively implement issue #238 requirements: URL fragments for node/link clicks, restoration on page visits, browser history support, geo-map and indoor map support, and extensible architecture.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue #238 objectives; example templates and tests support the bookmarkable actions feature without introducing unrelated modifications.

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 10

🧹 Nitpick comments (6)

src/js/netjsongraph.util.js (1)

1217-1228: Use strict equality for null checks.

Line 1223 uses loose equality (!=). Consider using strict equality (!==) for consistency with modern JavaScript best practices.

🔎 Proposed fix

-      if (id != null) {
+      if (id !== null && id !== undefined) {
         fragments[id] = params;
       }

test/netjsongraph.browser.test.js (1)

144-161: Test coverage looks good, but consider robustness of XPath selectors.

The test correctly verifies basic URL fragment functionality for nodes. However, the XPath selector "//span[@class='njg-valueLabel' and text()='10.149.3.3']" is tightly coupled to the current DOM structure. If the class name or element hierarchy changes, this test will break even if the functionality works correctly.

💡 Consider adding a data attribute for testing

To make tests more resilient to UI changes, consider adding data-testid attributes to key elements in the sidebar:

// In sidebar rendering code:
<span class="njg-valueLabel" data-testid="node-id-value">10.149.3.3</span>

// In test:
const node = await getElementByXpath(
  driver,
  "//span[@data-testid='node-id-value' and text()='10.149.3.3']",
  2000,
);

This approach is more maintainable and follows testing best practices.

README.md (1)

472-479: Consider documenting default values for all properties.

The documentation clearly shows the configuration structure and types, but only mentions that the feature is "disabled by default" (Line 481). For better clarity, consider documenting default values for all properties in the code block.

💡 Enhanced documentation with defaults

   ```javascript
       bookmarkableActions: {
-        enabled: boolean,
+        enabled: boolean,  // default: false
-        id: string,
+        id: string,        // default: null
-        zoomOnRestore: boolean,
+        zoomOnRestore: boolean,  // default: true
-        zoomLevel: number,
+        zoomLevel: number,       // default: null
       }

This makes it immediately clear to users what the default behavior is for each option.
</details>

</blockquote></details>
<details>
<summary>src/js/netjsongraph.render.js (1)</summary><blockquote>

`156-156`: **Remove unused index parameters.**

The `index` parameter is added to the callbacks on lines 156, 246, and 295 but is never used within the callback bodies. Based on past review comments, this appears to be leftover from a previous implementation. Removing these unused parameters will improve code clarity.



<details>
<summary>🔎 Proposed cleanup</summary>

```diff
-    const links = JSONData.links.map((link, index) => {
+    const links = JSONData.links.map((link) => {
       const linkResult = JSON.parse(JSON.stringify(link));

       const {linkStyleConfig, linkEmphasisConfig} = self.utils.getLinkStyle(

-    nodes.forEach((node, index) => {
+    nodes.forEach((node) => {
       if (node.properties) {

-    links.forEach((link, index) => {
+    links.forEach((link) => {
       if (!flatNodes[link.source]) {

Also applies to: 246-246, 295-295

test/netjsongraph.util.test.js (1)

330-375: Conceptual ordering test, but doesn't test actual implementation.

This test verifies the concept that applyUrlFragmentState should run after onReady completes, but uses a custom event emitter rather than the actual NetJSONGraph event system. While the test demonstrates the expected behavior pattern, it doesn't directly validate the production code's sequencing logic in netjsongraph.core.js.

Consider adding an integration test that exercises the actual NetJSONGraph instance to validate the sequencing end-to-end.

src/js/netjsongraph.core.js (1)

114-135: Add validation or documentation to prevent node ID collision with link key separator.

The indexing strategy is sound, and the ~ separator is documented in the README as the link format in URL fragments. However, there's no validation preventing node IDs from containing ~, which could cause a collision in nodeLinkIndex. For example, a node with ID "nodeA~nodeB" would overwrite the link key "nodeA~nodeB" if those nodes are linked.

While this is unlikely in practice and no existing test data violates this constraint, consider either:

• Adding validation to reject node IDs containing ~, or
• Documenting this constraint as a requirement for node ID format.

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e503883 and ccfd2d1.

📒 Files selected for processing (15)

• README.md
• index.html
• public/example_templates/netjson-clustering.html
• public/example_templates/netjsongraph.html
• public/example_templates/netjsonmap-indoormap-overlay.html
• public/example_templates/netjsonmap-indoormap.html
• public/example_templates/netjsonmap.html
• src/js/netjsongraph.config.js
• src/js/netjsongraph.core.js
• src/js/netjsongraph.render.js
• src/js/netjsongraph.util.js
• test/browser.test.utils.js
• test/netjsongraph.browser.test.js
• test/netjsongraph.render.test.js
• test/netjsongraph.util.test.js

🧰 Additional context used

🧬 Code graph analysis (3)

src/js/netjsongraph.util.js (1)

examples/load_data_geo_extent/index.js (1)

• node (406-413)

src/js/netjsongraph.render.js (3)

test/netjsongraph.render.test.js (1)

• JSONData (5-9)

src/js/netjsongraph.geojson.js (2)

• links (17-17)
• nodes (16-16)

examples/load_data_geo_extent/index.js (5)

• links (444-446)
• JSONData (23-403)
• nodes (417-425)
• nodes (442-442)
• node (406-413)

test/netjsongraph.browser.test.js (1)

test/browser.test.utils.js (10)

• urls (33-39)
• urls (33-39)
• getElementByCss (41-48)
• getElementByCss (41-48)
• captureConsoleErrors (131-136)
• captureConsoleErrors (131-136)
• getElementByXpath (50-57)
• getElementByXpath (50-57)
• printConsoleErrors (138-145)
• printConsoleErrors (138-145)

🪛 LanguageTool

README.md

[style] ~484-~484: Try moving the adverb to make the sentence clearer.
Context: ...re added to the URL: 1. id – A prefix used to uniquely identify the map. 2. nodeId – The id of the selected n...

(SPLIT_INFINITIVE)

---

[grammar] ~491-~491: Ensure spelling is correct
Context: ... clicking a node updates the URL and on apllying the state from url it automatically cen...

(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)

🔇 Additional comments (20)

test/browser.test.utils.js (1)

50-57: LGTM! XPath helper follows existing patterns.

The new getElementByXpath function mirrors the existing getElementByCss utility with consistent error handling and timeout defaults.

src/js/netjsongraph.util.js (3)

1239-1248: LGTM! State management implementation is correct.

The function correctly serializes fragments and uses history.pushState to maintain browser history.

---

1281-1288: LGTM! Fragment removal logic is straightforward.

The function correctly checks for fragment existence before removal and updates the URL/history state.

---

1329-1333: LGTM! Event handler uses the correct event.

The popstate event is appropriate for handling browser back/forward navigation with history state.

public/example_templates/netjson-clustering.html (1)

83-86: LGTM! Configuration follows the expected pattern.

The bookmarkableActions configuration is properly structured with a unique identifier for the clustering map.

public/example_templates/netjsonmap.html (1)

99-103: LGTM! Configuration includes appropriate zoom level.

The bookmarkableActions configuration is properly structured with a unique identifier and a sensible default zoom level for the geographic map.

test/netjsongraph.render.test.js (2)

512-513: LGTM! Proper test isolation.

The added mocks for parseUrlFragments and setupHashChangeHandler correctly isolate the rendering tests from URL fragment side effects introduced by the bookmarkableActions feature.

---

985-986: LGTM! Consistent mock pattern across test suites.

All remaining mock additions follow the same correct pattern, properly isolating each test suite from URL fragment operations. The duplication across multiple beforeEach blocks is appropriate as each test suite requires independent setup.

Also applies to: 1083-1084, 1226-1227, 1286-1287, 1323-1324, 1354-1355, 1399-1400, 1463-1464

test/netjsongraph.browser.test.js (3)

3-3: LGTM! Necessary import for new tests.

The getElementByXpath utility is correctly imported to support locating sidebar elements by their text content in the new URL fragment tests.

---

163-187: Link test coverage is appropriate.

The test correctly verifies URL fragment functionality for links using the source~target format. The same consideration about XPath selector robustness applies here as mentioned in the node test.

---

189-234: Good coverage across render modes.

The tests appropriately verify URL fragment functionality for both nodes and links in geographic map mode, complementing the basic usage tests. This ensures the feature works consistently across different visualization types.

Consider adding browser tests for the indoor map mode mentioned in the PR objectives (netjsonmap-indoormap-overlay.html example). The current tests cover basic graph and geographic map but not the indoor floorplan scenario.

public/example_templates/netjsongraph.html (1)

87-90: LGTM! Proper feature configuration for the example.

The bookmarkableActions configuration is correctly structured and enables the bookmarkable URL fragment feature for this example. The id: "basicUsage" matches the URL fragments used in the browser tests, ensuring consistency.

src/js/netjsongraph.render.js (3)

96-96: LGTM! URL bookmarking integrated correctly.

The call to self.utils.addActionToUrl(self, params) appropriately updates the URL with the clicked element's identifier before executing the click handler. This ensures the URL is updated for bookmarking purposes while still allowing the user's custom onClickElement callback to execute.

---

447-447: LGTM! Hash change handlers properly positioned.

The setupHashChangeHandler(self) calls are correctly placed in both graphRender and mapRender after the visualization setup is complete but before emitting the "onLoad" event. This ensures browser back/forward navigation is handled appropriately for URL fragments in both render modes.

Also applies to: 716-716

---

452-452: LGTM! URL fragment state application correctly sequenced.

The applyUrlFragmentState event emissions are correctly placed after the onReady event in both render methods. This ensures the visualization is fully initialized before attempting to restore state from URL fragments, preventing race conditions.

Also applies to: 721-721

src/js/netjsongraph.config.js (1)

289-294: Well-structured configuration block for bookmarkable actions.

The configuration provides sensible defaults: enabled: false ensures backward compatibility, zoomOnRestore: true provides expected UX when restoring state, and zoomLevel: null allows automatic zoom level detection. The id property enables multiple graph instances per page.

test/netjsongraph.util.test.js (1)

295-328: Good test coverage for applyUrlFragmentState.

The test properly mocks setView, onClickElement, and verifies both the map positioning and click handler are invoked with correct parameters.

src/js/netjsongraph.core.js (1)

74-86: Well-designed async sequencing for URL fragment state restoration.

The Promise-based coordination ensures applyUrlFragmentState only executes after onReady completes, even when onReady performs async operations. The once event pattern prevents duplicate handler registration.

public/example_templates/netjsonmap-indoormap-overlay.html (1)

140-147: Good cleanup logic for URL fragments on overlay close.

The close handler correctly removes the indoor map's URL fragment before removing the container, ensuring the URL stays in sync with the UI state.

public/example_templates/netjsonmap-indoormap.html (1)

98-128: Excellent documentation of coordinate transformation logic.

The comments thoroughly explain the Leaflet coordinate system differences and the transformation approach. This will be valuable for future maintainers working with indoor map coordinate systems.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

README.md (1)

468-496: Documentation looks good overall.

The bookmarkableActions documentation clearly explains the feature, its configuration options, and behavior across different visualization types. The structure is user-friendly with the config block, notes about default state, and explanations for different element types.

One minor issue: Line 491 contains a grammatical issue - "In ECharts graphs, only triggers" should be "In ECharts graphs, it only triggers" for proper sentence structure.

🔎 Suggested improvement

-  For nodes, the behavior depends on the type of visualization in Leaflet maps, clicking a node updates the URL and on applying the state from url it automatically centers the map on that node, in addition to triggering its click event. In ECharts graphs, only triggers the click event for the node.
+  For nodes, the behavior depends on the type of visualization. In Leaflet maps, clicking a node updates the URL and on applying the state from the URL it automatically centers the map on that node, in addition to triggering its click event. In ECharts graphs, it only triggers the click event for the node.

src/js/netjsongraph.util.js (1)

1343-1347: Consider returning a cleanup function for the event listener.

The popstate listener is added but never removed. If multiple NetJSONGraph instances are created and destroyed dynamically, this could lead to stale handlers or unexpected behavior. For typical single-instance usage this is fine, but returning a cleanup function would improve reusability.

🔎 Suggested improvement

   setupHashChangeHandler(self) {
-    window.addEventListener("popstate", () => {
+    const handler = () => {
       this.applyUrlFragmentState(self);
-    });
+    };
+    window.addEventListener("popstate", handler);
+    // Return cleanup function for potential future use
+    return () => window.removeEventListener("popstate", handler);
   }

📜 Review details

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ccfd2d1 and 59ad0e4.

📒 Files selected for processing (6)

• README.md
• index.html
• public/example_templates/netjsonmap-indoormap-overlay.html
• public/example_templates/netjsonmap-indoormap.html
• src/js/netjsongraph.util.js
• test/netjsongraph.util.test.js

🚧 Files skipped from review as they are similar to previous changes (1)

• test/netjsongraph.util.test.js

🧰 Additional context used

🧠 Learnings (1)

📚 Learning: 2026-01-04T08:00:36.595Z

Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:36.595Z
Learning: In the netjsongraph.js codebase, specifically in src/js/netjsongraph.util.js, the pattern '== null' is intentionally used to detect both null and undefined in a single comparison. Do not flag or replace these checks with strict equality checks (=== null or === undefined) for this file; preserve the established idiom.

Applied to files:

• src/js/netjsongraph.util.js

🪛 LanguageTool

README.md

[style] ~484-~484: Try moving the adverb to make the sentence clearer.
Context: ...re added to the URL: 1. id – A prefix used to uniquely identify the map. 2. nodeId – The id of the selected n...

(SPLIT_INFINITIVE)

🔇 Additional comments (11)

src/js/netjsongraph.util.js (4)

1217-1228: LGTM!

The parseUrlFragments function correctly parses semicolon-delimited URL fragments into a map of IDs to URLSearchParams. The docstring clearly explains the fragment structure and expected output. The != null check on line 1223 is consistent with the established codebase pattern for detecting both null and undefined.

---

1239-1248: LGTM!

The function correctly serializes the fragments object back to the URL hash and stores the node data in browser history state for efficient retrieval during back/forward navigation. The inline comment clearly explains the rationale.

---

1250-1291: LGTM!

The defensive checks have been properly implemented. The function correctly:

• Guards against missing/disabled config and cluster clicks
• Validates nodeLinkIndex exists before use
• Determines nodeId based on render mode (graph vs map) and element type (node vs link)
• Validates the nodeId exists in the lookup before updating URL

---

1293-1300: LGTM!

Simple and correct implementation for removing a fragment from the URL.

index.html (1)

184-188: LGTM!

The new card for the indoor map overlay example follows the existing pattern and links to the new example page correctly. The previous "Ovelay" typo has been fixed to "Overlay".

public/example_templates/netjsonmap-indoormap-overlay.html (3)

116-118: Verify intentional behavior: indoor map opens on any element click.

The onClickElement callback opens the indoor map overlay regardless of which node or link was clicked. If this is intentional demo behavior, consider adding a comment to clarify. If it should only trigger for specific nodes (e.g., those representing indoor locations), the logic would need adjustment.

---

186-245: LGTM!

The async onReady implementation correctly:

• Removes default tile layers
• Loads and decodes the floorplan image asynchronously
• Calculates proper bounds centered at (0,0)
• Transforms node and link coordinates from image space to lat/lng
• Sets up the image overlay with proper bounds constraints

The comment at line 208 referencing the workaround explanation in netjsonmap-indoormap.html is helpful for maintainability.

---

139-146: LGTM!

The close button handler properly cleans up by removing the URL fragment for the indoor map before removing the container from the DOM. This ensures the URL stays in sync with the UI state.

public/example_templates/netjsonmap-indoormap.html (3)

55-71: LGTM!

The updated mapOptions with proper zoom configuration and the new bookmarkableActions config are correctly structured. The zoomOnRestore: false is appropriate for indoor maps where the view is constrained to the floorplan bounds.

---

82-156: LGTM!

The async onReady implementation is well-documented with clear comments explaining:

• The workaround for issue #397
• The coordinate system differences between Leaflet and cartesian systems
• The transformation logic for nodes and links

The past issues (missing const for imageUrl and soruce typo) have been fixed.

---

155-157: No issues found. The brace structure is correct. Line 155 closes the onReady function, line 156 closes the config object parameter, and line 157 closes the NetJSONGraph constructor call. The nesting is properly balanced and the syntax is valid.