[feature] Added configurable popup on node click #402#542
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds mapOptions.nodePopup defaults and implements NetJSONGraphGUI.loadNodePopup (async content resolution, Leaflet popup creation, staleness checks, close handling) plus createDefaultPopupContent. Integrates popups into ECharts click handling (skip clusters, open popup then forward node click) and into applyUrlFragmentState restore. Adds CSS for the default popup and updates the indoor-map example to use custom popup content, hash-based auto-open, instance caching/clearing, an index-based projection fix that writes projected locations back into node data, and test adjustments across unit and browser suites. Sequence Diagram(s)sequenceDiagram
participant User
participant ECharts
participant Render
participant GUI
participant Leaflet
User->>ECharts: click node
ECharts->>Render: emit click event
Render->>Render: if params.data.cluster then return null
alt not clustered and nodePopup.show
Render->>GUI: loadNodePopup(node)
GUI->>Leaflet: create & open popup at node location (content resolved)
Leaflet->>GUI: popup opened
GUI->>Render: forward clickElement("node", node)
else clustered
Render->>Render: no popup, return null
end
Leaflet->>GUI: popup close event
GUI->>GUI: cleanup URL fragment if bookmarkable and matches nodeId
sequenceDiagram
participant URLUtil
participant GUI
participant Leaflet
URLUtil->>GUI: applyUrlFragmentState restores node selection
GUI->>GUI: resolve node and call loadNodePopup(node) if nodePopup.show
GUI->>Leaflet: open popup at node location
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/css/netjsongraph.cssParsing error: Unexpected token . Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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
added
enhancement
javascript
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Changes Since Last ReviewThe incremental changes since commit Bug Fix in
New CI Workflow:
Enhanced Test Coverage:
All changes follow defensive programming practices with comprehensive test coverage. Files Reviewed (5 files changed since last review)
Reviewed by kimi-k2.5-0127 · 221,338 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
public/example_templates/netjsonmap-indoormap-overlay.html (1)
271-277:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPrevent re-initializing the indoor graph when the overlay is already open.
openIndoorMap()creates a newNetJSONGraphand listeners on every call, even when#indoormap-containeralready exists. This can stack handlers and leak state.Suggested fix
function openIndoorMap() { let indoorMapContainer = document.getElementById("indoormap-container"); - if (!indoorMapContainer) { - indoorMapContainer = createIndoorMapContainer(); - } + if (indoorMapContainer) { + return; + } + indoorMapContainer = createIndoorMapContainer(); const mapContainer = document.getElementById("map-container"); mapContainer.appendChild(indoorMapContainer); const indoor = new NetJSONGraph( @@ closeButtonHandler(indoorMapContainer, indoor, () => { window.removeEventListener("popstate", popstateHandler); + window._indoorMap = null; }); }Also applies to: 278-447
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/example_templates/netjsonmap-indoormap-overlay.html` around lines 271 - 277, openIndoorMap currently recreates a NetJSONGraph and event listeners every time even if `#indoormap-container` exists, causing stacked handlers and leaks; modify openIndoorMap to check for an existing container and existing graph instance before initializing (e.g., if document.getElementById("indoormap-container") exists then reuse the existing graph/listeners and skip creating a new NetJSONGraph), or store the graph on the container (container._indoorGraph or dataset) when createIndoorMapContainer runs and use that to avoid re-initialization; ensure you also avoid appending the container again to map-container when it already exists so createIndoorMapContainer and NetJSONGraph instantiation run only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Around line 222-225: The popup currently injects node-derived strings using
innerHTML on the elements keyLabel and valueLabel which risks XSS; change both
assignments to use textContent instead (update the code that creates keyLabel
and valueLabel—the elements with class "njg-tooltip-value" and the key label
creation logic—to set textContent = key and textContent = value rather than
using innerHTML) so node data is rendered as plain text.
- Around line 187-196: The getIndoorMapFragment function currently uses
decodeURIComponent(window.location.hash.replace(/^#/, "")) which can throw a
URIError for malformed percent-encodings; wrap the decode step in a safe
try/catch (or a small safeDecode helper) and fall back to an empty string (or
raw hash) on error before splitting into fragments, so fragments/finder logic
(fragments.find, new URLSearchParams) never breaks; update references in
getIndoorMapFragment and any local variables (fragments, params) accordingly.
In `@src/css/netjsongraph.css`:
- Around line 398-401: The CSS rule for the selector ".njg-container
.default-popup .njg-tooltip-value" uses the deprecated "word-break: break-word";
replace it by removing that declaration and adding the modern equivalent
"overflow-wrap: anywhere" (or "overflow-wrap: break-word" if you prefer legacy
behaviour) so long values still break correctly; update the rule in the same
selector (njg-tooltip-value) and remove the deprecated property to satisfy
stylelint.
- Around line 369-378: The selectors ".njg-container .default-popup
a.leaflet-popup-close-button" and its :hover variant don't match Leaflet's DOM
because the close button isn't nested inside the element with class
"default-popup"; update the selectors to target the actual popup close button
placement such as ".njg-container .leaflet-popup .leaflet-popup-close-button"
(or simply ".njg-container
a.leaflet-popup-close-button"/".leaflet-popup-close-button" if you want global
scope) and apply the same positioning, font-size and hover color rules there so
the styles actually take effect.
In `@src/js/netjsongraph.gui.js`:
- Around line 357-360: The popup currently uses innerHTML to render untrusted
node data (see keyLabel and valueLabel creation where keyLabel.innerHTML = key
and valueLabel.innerHTML = value), which enables XSS; replace uses of innerHTML
with textContent for both keyLabel and valueLabel (and any other popup fields in
the same tooltip/renderer function that assign user data) so the values are
treated as plain text and not parsed as HTML; locate the tooltip-generation code
that creates elements with class "njg-tooltip-value" (and the surrounding
function that handles key/value rendering) and update assignments to use
textContent.
- Around line 303-305: The async call to the custom popup callback (popupContent
/ nodePopup.content) can throw or reject and must be guarded; wrap the await
popupContent.call(this, node, this.self) in a try/catch, on error log or handle
the exception (e.g., via console.warn or the module logger) and set popupContent
to a safe fallback (null/empty string) so the popup flow continues without
unhandled promise rejections; reference the popupContent variable and the call
site where popupContent.call(this, node, this.self) is invoked.
- Around line 323-330: The remove handler for this.self.leaflet.currentPopup
unconditionally deletes the bookmarkable nodeId from the URL, which can remove a
newly-selected node; change the handler so it only clears the hash when the
popup being removed actually corresponds to the nodeId currently stored in the
fragments: inside the currentPopup.on("remove", ...) callback, obtain the
removed popup's node identifier (e.g. from the popup instance/options or its
stored metadata), compare it to fragments[id].get("nodeId") and only call
fragments[id].delete("nodeId") and this.self.utils.updateUrlFragments(fragments)
when they match; keep references to parseUrlFragments, updateUrlFragments,
bookmarkableActions/id, and currentPopup to locate and update the code.
In `@src/js/netjsongraph.util.js`:
- Around line 1384-1386: The auto-open currently calls
self.gui.loadNodePopup(node) whenever self.config.mapOptions.nodePopup.show is
true, but node bookmarks can be link fragments (format "source~target") which
are not real nodes; change the guard to only call loadNodePopup for actual node
fragments by detecting and skipping link fragments—e.g., check the fragment/id
for the link separator (tilde) or another node-vs-link indicator on the passed
node object before invoking self.gui.loadNodePopup(node). Ensure the check
references the same symbols: self.config.mapOptions.nodePopup.show and
self.gui.loadNodePopup(node), and only call loadNodePopup when the fragment
represents a node.
---
Outside diff comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Around line 271-277: openIndoorMap currently recreates a NetJSONGraph and
event listeners every time even if `#indoormap-container` exists, causing stacked
handlers and leaks; modify openIndoorMap to check for an existing container and
existing graph instance before initializing (e.g., if
document.getElementById("indoormap-container") exists then reuse the existing
graph/listeners and skip creating a new NetJSONGraph), or store the graph on the
container (container._indoorGraph or dataset) when createIndoorMapContainer runs
and use that to avoid re-initialization; ensure you also avoid appending the
container again to map-container when it already exists so
createIndoorMapContainer and NetJSONGraph instantiation run only once.
🪄 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: f4762176-7728-49fd-8f89-affcebe04f0b
📒 Files selected for processing (6)
public/example_templates/netjsonmap-indoormap-overlay.htmlsrc/css/netjsongraph.csssrc/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.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). (4)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: QA-Checks
- GitHub Check: auto-assign-issue / run-bot
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
src/css/netjsongraph.csssrc/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.js
🧠 Learnings (4)
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.js
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.js
📚 Learning: 2026-01-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
📚 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
🪛 OpenGrep (1.20.0)
src/js/netjsongraph.gui.js
[WARNING] 357-357: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
[WARNING] 360-360: Setting innerHTML with dynamic content can lead to XSS. Use textContent or createElement with proper escaping instead.
(coderabbit.xss.innerhtml-assignment)
🪛 Stylelint (17.11.0)
src/css/netjsongraph.css
[error] 400-400: Deprecated keyword "break-word" for property "word-break" (declaration-property-value-keyword-no-deprecated)
(declaration-property-value-keyword-no-deprecated)
🔇 Additional comments (3)
public/example_templates/netjsonmap-indoormap-overlay.html (1)
76-132: LGTM!Also applies to: 151-151, 198-200, 298-307, 350-358, 400-400
src/js/netjsongraph.config.js (1)
248-256: LGTM!src/js/netjsongraph.render.js (1)
111-119: LGTM!
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
You are on the right path with the mapOptions.nodePopup approach, and the feature direction matches #402. It still needs more polishing before we can merge it safely.
Linting Fails With The Current Project Setup
yarn lint fails because the new code uses optional chaining in src/js/netjsongraph.gui.js, but the current ESLint parser setup does not accept it. There is also a consistent-return error in the new click handler in src/js/netjsongraph.render.js.
Please either rewrite the new optional chaining with explicit guards, or update the parser/lint setup intentionally for the whole project.
Focused Tests Are Failing
yarn test test/netjsongraph.util.test.js test/netjsongraph.spec.js --runInBand fails.
The config fixture in test/netjsongraph.spec.js needs to be updated for the new mapOptions.nodePopup default. More importantly, applyUrlFragmentState currently crashes when mapOptions.nodePopup is missing from a custom test config, so the new code needs a guard before reading .show.
Popup Restoration Runs For Link Fragments
applyUrlFragmentState calls loadNodePopup for every restored fragment when popups are enabled. Link fragments use the source~target format and resolve to link data, not node data, so popup restoration should only run for real node fragments.
Replacing A Popup Can Remove The Newly Selected URL Fragment
The URL is updated before the old popup is removed. The old popup's remove handler then deletes nodeId unconditionally, so clicking node A and then node B can remove node B from the hash.
The remove handler should only clear the fragment if the popup being removed belongs to the same node currently stored in the URL.
Custom Popup Content Needs Failure Handling
nodePopup.content is awaited without error handling. If a user callback throws or rejects, the click flow can fail with an unhandled rejection.
There is also a possible race: since loadNodePopup is async and the click handler does not await it, a slower callback from an earlier click can open after a newer click and show stale popup content.
Indoor Overlay Can Be Initialized More Than Once
openIndoorMap() creates a new NetJSONGraph and new event listeners each time it is called, even when the indoor container already exists. This can stack graph instances and listeners.
Please make repeated calls reuse the existing overlay or return early when it is already open.
Documentation And Browser Tests Need Updates
mapOptions.nodePopup is a new public option and should be documented in README.md.
The browser tests still assume that clicking a geo node opens the indoor map directly. With this PR, the first click opens a popup and the floorplan is opened from the popup button, so those tests need to be updated. Please also add coverage for default popup content, custom popup content, URL restoration, link-fragment restoration, and popup close/replacement behavior.
Remove Debug And Dead Code From The Example
The indoor overlay example still has a console.log("Node", node), an empty onClickElement, and a commented-out onClickElement. These should be removed unless they are needed for a specific test or behavior.
CodeRabbit And KiloBot Issues To Address
The important bot findings are valid and should be fixed, but I am not repeating every detail here.
- Use
textContentinstead ofinnerHTMLfor node-derived popup fields; this is the main security issue. - Guard malformed URL hash decoding in the example page.
- Fix the Leaflet popup close-button CSS selector and replace deprecated
word-break: break-word. - Guard the custom popup callback against thrown/rejected errors.
- Do not clear
nodeIdfrom the hash unless the removed popup matches the current fragment. - Do not auto-open node popups for restored link fragments.
- Avoid repeated indoor-map initialization.
- Add null guards for
nodePopup, but avoid optional chaining unless the project lint setup is updated.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (4)
public/example_templates/netjsonmap-indoormap-overlay.html (2)
403-403:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty
onClickElementoverride — same concern as Line 151.Same situation as the main graph: an empty handler silences the indoor graph’s default click behavior. Either drop it or document why it’s required now that
nodePopupowns the click path, and confirm tests still cover the indoor click flow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/example_templates/netjsonmap-indoormap-overlay.html` at line 403, The empty onClickElement override in the indoor overlay (onClickElement) is suppressing the default click behavior; either remove this empty function so the indoor graph falls back to its default click handler, or replace it with a short clarifying comment and a passthrough that delegates to nodePopup (or calls the original handler) so clicks still invoke nodePopup; after changing, run/adjust tests to ensure the indoor click flow is covered.
151-151:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEmpty
onClickElementoverride needs justification or removal.The empty handler suppresses default click behavior on the main graph. Either remove it (and let the default fire), or add a brief comment explaining that the new
nodePopupflow is now the canonical click action so the override is intentional — and update the related browser tests accordingly.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/example_templates/netjsonmap-indoormap-overlay.html` at line 151, The empty onClickElement(type, data) {} override suppresses the graph's default click behavior; either remove this override so the default handler runs, or keep it but add a short explanatory comment stating that nodePopup is now the canonical click flow and that the empty override is intentional, and then update the related browser tests to expect the nodePopup behavior instead of the original default click handling; locate the onClickElement definition to apply the change.src/css/netjsongraph.css (1)
369-378:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDefault-popup close-button selectors still won’t match Leaflet’s DOM.
The close button is rendered as a sibling of
.leaflet-popup-content-wrapper, not inside.default-popup(which lives within.leaflet-popup-content), so these rules remain inert. To preserve the “only style the default popup” intent without breaking custom popups, tag the popup wrapper itself (e.g.popup.getElement()?.classList.add("njg-default-popup")inloadNodePopup) and scope the rules to that wrapper class.Suggested fix
-.njg-container .default-popup a.leaflet-popup-close-button { +.njg-container .njg-default-popup .leaflet-popup-close-button { position: absolute; top: 4px; right: 4px; font-size: 18px !important; } -.njg-container .default-popup a.leaflet-popup-close-button:hover { +.njg-container .njg-default-popup .leaflet-popup-close-button:hover { color: black !important; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/css/netjsongraph.css` around lines 369 - 378, The CSS selectors targeting the close button use .njg-container .default-popup a.leaflet-popup-close-button but Leaflet renders the close button as a sibling of .leaflet-popup-content-wrapper, so those rules never apply; update the code that creates the default popup (loadNodePopup) to add a distinct class to the popup wrapper (e.g. call popup.getElement()?.classList.add("njg-default-popup") in loadNodePopup) and change the CSS to target the wrapper class (e.g. .njg-container .njg-default-popup .leaflet-popup-close-button) so only the default popup is styled while leaving custom popups unchanged.src/js/netjsongraph.gui.js (1)
302-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard synchronous throws from
nodePopup.contentcallback invocation.
popupContent.call(...)is outsidetry, so a synchronous throw escapes and leaves an unhandled rejection path.Proposed fix
- } else if (popupContent && typeof popupContent === "function") { - const popupRequest = popupContent.call(this, node, this.self); + } else if (popupContent && typeof popupContent === "function") { + let popupRequest; + try { + popupRequest = Promise.resolve(popupContent.call(this, node, this.self)); + } catch (error) { + console.error("Failed to build node popup content:", error); + return; + } this.self.leaflet.currentPopupRequest = popupRequest; try { popupContent = await popupRequest;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.gui.js` around lines 302 - 307, Wrap the synchronous invocation of the popup factory so sync throws are caught: move the call to popupContent.call(this, node, this.self) inside the existing try so both synchronous exceptions and promise rejections are handled by the same catch; assign popupRequest and set this.self.leaflet.currentPopupRequest only after the call succeeds (i.e., inside the try) and then await popupRequest to get popupContent. Target symbols: popupContent, popupRequest, this.self.leaflet.currentPopupRequest, and the surrounding try/catch block in netjsongraph.gui.js.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Around line 202-204: The auto-open of the indoor overlay runs synchronously
and can race the geo map's async restoration; change the logic so that instead
of calling openIndoorMap() immediately when getIndoorMapFragment() is truthy,
defer that call until after the geo map has finished its async restore (i.e.,
after netjsonmap.render() completes and the geo map's onReady/restore hook
fires). Concretely: wire the getIndoorMapFragment() check into the geo map's
ready/restore completion callback (or promise resolution from
netjsonmap.render()), and then call openIndoorMap() there to ensure proper
ordering and popstate/focus consistency.
- Around line 76-78: The global rule ".njg-container .njg-sideBar { display:
none }" hides sidebars for both the main geo map and the indoor overlay; change
it so it only targets the main map container (e.g., narrow the selector to the
main map root instead of the generic ".njg-container") so the indoor overlay
selectors "#netjson-indoormap .njg-sideBar" and "#indoormap-container
.njg-container .sideBarHandle" remain effective and the indoor sidebar can be
shown when desired.
- Around line 206-243: createCustomPopup currently calls
location.lat.toFixed(8)/location.lng.toFixed(8) without validating types and
sets the button label with innerHTML; update createCustomPopup to first
coerce/validate location (the local variable location inside createCustomPopup)
by checking that location?.lat and location?.lng are finite numbers (e.g.,
Number(location.lat) and Number.isFinite) before calling toFixed, and fall back
to null or a safe string when invalid so formatting doesn't throw; also replace
the button.innerHTML assignment with button.textContent to avoid inconsistent
HTML injection handling; references: createCustomPopup, the local variable
location, fields.location, and the button that calls openIndoorMap.
In `@src/js/netjsongraph.gui.js`:
- Around line 318-321: The code assumes
this.self.config.mapOptions.nodePopup.config exists before calling
Object.entries, causing a runtime throw when nodePopup.show is enabled but
nodePopup.config is missing; update the logic around popupConfig (the variable
assigned from this.self.config.mapOptions.nodePopup.config) to guard against
undefined/null by defaulting to an empty object (or using optional
chaining/nullish coalescing) before calling Object.entries so the subsequent
Object.fromEntries filter operates on an object, not undefined.
- Around line 331-354: Destructure values instead of indexing and use strict
equality, and avoid direct DOM queries by wiring Leaflet popup lifecycle:
replace direct accesses like this.self.config.mapOptions.nodePopup.onOpen and
this.self.config.bookmarkableActions.id with destructured bindings (e.g., const
{ mapOptions, bookmarkableActions } = this.self.config; const { onOpen } =
mapOptions.nodePopup; const { id } = bookmarkableActions), call onOpen if it is
a function inside try/catch, then remove the querySelector logic and instead
listen for popup.on("remove", ...) to handle the popup close lifecycle; inside
that handler compare fragments[id].get("nodeId") to the popup node id using ===
and coerce popup node id to String(popupNodeId) (or ensure currentNodeId is
parsed to the same type) before comparing, and update fragments via
this.self.utils.updateUrlFragments as before.
---
Duplicate comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Line 403: The empty onClickElement override in the indoor overlay
(onClickElement) is suppressing the default click behavior; either remove this
empty function so the indoor graph falls back to its default click handler, or
replace it with a short clarifying comment and a passthrough that delegates to
nodePopup (or calls the original handler) so clicks still invoke nodePopup;
after changing, run/adjust tests to ensure the indoor click flow is covered.
- Line 151: The empty onClickElement(type, data) {} override suppresses the
graph's default click behavior; either remove this override so the default
handler runs, or keep it but add a short explanatory comment stating that
nodePopup is now the canonical click flow and that the empty override is
intentional, and then update the related browser tests to expect the nodePopup
behavior instead of the original default click handling; locate the
onClickElement definition to apply the change.
In `@src/css/netjsongraph.css`:
- Around line 369-378: The CSS selectors targeting the close button use
.njg-container .default-popup a.leaflet-popup-close-button but Leaflet renders
the close button as a sibling of .leaflet-popup-content-wrapper, so those rules
never apply; update the code that creates the default popup (loadNodePopup) to
add a distinct class to the popup wrapper (e.g. call
popup.getElement()?.classList.add("njg-default-popup") in loadNodePopup) and
change the CSS to target the wrapper class (e.g. .njg-container
.njg-default-popup .leaflet-popup-close-button) so only the default popup is
styled while leaving custom popups unchanged.
In `@src/js/netjsongraph.gui.js`:
- Around line 302-307: Wrap the synchronous invocation of the popup factory so
sync throws are caught: move the call to popupContent.call(this, node,
this.self) inside the existing try so both synchronous exceptions and promise
rejections are handled by the same catch; assign popupRequest and set
this.self.leaflet.currentPopupRequest only after the call succeeds (i.e., inside
the try) and then await popupRequest to get popupContent. Target symbols:
popupContent, popupRequest, this.self.leaflet.currentPopupRequest, and the
surrounding try/catch block in netjsongraph.gui.js.
🪄 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: ea239778-8b33-4501-85a6-d130847cdb68
📒 Files selected for processing (8)
.eslintrc.jspublic/example_templates/netjsonmap-indoormap-overlay.htmlsrc/css/netjsongraph.csssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jstest/netjsongraph.spec.jstest/netjsongraph.util.test.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). (4)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: Tests and Coverage (dev)
- GitHub Check: Tests and Coverage (echarts-only)
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
src/js/netjsongraph.render.jstest/netjsongraph.util.test.jstest/netjsongraph.spec.jssrc/css/netjsongraph.csssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.util.test.js
🧠 Learnings (4)
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
src/js/netjsongraph.render.jstest/netjsongraph.util.test.jstest/netjsongraph.spec.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
📚 Learning: 2026-01-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
📚 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
🪛 ESLint
src/js/netjsongraph.gui.js
[error] 331-331: Use object destructuring.
(prefer-destructuring)
[error] 347-347: Use object destructuring.
(prefer-destructuring)
[error] 350-350: Expected '===' and instead saw '=='.
(eqeqeq)
🪛 GitHub Check: QA-Checks
src/js/netjsongraph.gui.js
[failure] 350-350:
Expected '===' and instead saw '=='
[failure] 347-347:
Use object destructuring
[failure] 331-331:
Use object destructuring
🔇 Additional comments (8)
src/css/netjsongraph.css (1)
365-368: LGTM!Also applies to: 380-403
public/example_templates/netjsonmap-indoormap-overlay.html (1)
353-364: LGTM!.eslintrc.js (1)
3-6: LGTM!test/netjsongraph.spec.js (1)
203-211: LGTM!test/netjsongraph.util.test.js (1)
317-320: LGTM!src/js/netjsongraph.gui.js (1)
357-387: LGTM!src/js/netjsongraph.render.js (1)
111-120: LGTM!src/js/netjsongraph.util.js (1)
1384-1386: LGTM!
dee077
force-pushed
the
feature/402-node-popup-configurable-on-click
branch
3 times, most recently
from
0aced28 to
7b32b3d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/js/netjsongraph.gui.js (1)
350-352:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize popup node ID before fragment comparison.
currentNodeIdfromURLSearchParamsis always a string, whilenode.idmay be numeric. With strict equality, close cleanup can silently fail and leave stalenodeIdin the hash.Suggested fix
- const popupNodeId = node?.id || node?.properties?.id; - if (currentNodeId === popupNodeId) { + const popupNodeId = node?.id ?? node?.properties?.id; + if (popupNodeId != null && currentNodeId === String(popupNodeId)) { fragments[id].delete("nodeId"); this.self.utils.updateUrlFragments(fragments); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.gui.js` around lines 350 - 352, currentNodeId (from URLSearchParams) is a string while node.id may be numeric, causing strict equality to fail and leave stale nodeId fragments; normalize types before comparing by converting popupNodeId and/or currentNodeId to the same type (e.g., String(popupNodeId) or Number(currentNodeId)) so the comparison currentNodeId === popupNodeId succeeds when IDs match, then proceed to call fragments[id].delete("nodeId") as before (refer to popupNodeId, currentNodeId and fragments[id].delete("nodeId") in the diff).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/js/netjsongraph.gui.js`:
- Around line 361-368: The popup location formatting can throw if location.lat
or location.lng are non-numeric; update the block that builds fields.location
(where location is derived from node?.location || node?.properties?.location and
the fields object is constructed) to validate coordinates with Number.isFinite
(or safe parseFloat + isFinite) before calling toFixed(8), and if either
coordinate is not finite fallback to null or a safe placeholder string so
malformed nodes won't break popup rendering.
---
Duplicate comments:
In `@src/js/netjsongraph.gui.js`:
- Around line 350-352: currentNodeId (from URLSearchParams) is a string while
node.id may be numeric, causing strict equality to fail and leave stale nodeId
fragments; normalize types before comparing by converting popupNodeId and/or
currentNodeId to the same type (e.g., String(popupNodeId) or
Number(currentNodeId)) so the comparison currentNodeId === popupNodeId succeeds
when IDs match, then proceed to call fragments[id].delete("nodeId") as before
(refer to popupNodeId, currentNodeId and fragments[id].delete("nodeId") in the
diff).
🪄 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: 18453421-c859-4f79-826f-1efb0daf0069
📒 Files selected for processing (8)
.eslintrc.jspublic/example_templates/netjsonmap-indoormap-overlay.htmlsrc/css/netjsongraph.csssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jstest/netjsongraph.spec.jstest/netjsongraph.util.test.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). (3)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: Tests and Coverage (dev)
- GitHub Check: Tests and Coverage (echarts-only)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
test/netjsongraph.spec.jstest/netjsongraph.util.test.jssrc/js/netjsongraph.util.jssrc/css/netjsongraph.csssrc/js/netjsongraph.render.jssrc/js/netjsongraph.gui.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.util.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.util.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.gui.js
🧠 Learnings (4)
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
test/netjsongraph.spec.jstest/netjsongraph.util.test.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.gui.js
📚 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
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/netjsongraph.util.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.gui.js
📚 Learning: 2026-01-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
🔇 Additional comments (7)
.eslintrc.js (1)
3-6: LGTM!test/netjsongraph.spec.js (1)
203-211: LGTM!test/netjsongraph.util.test.js (1)
317-320: LGTM!src/css/netjsongraph.css (1)
365-403: LGTM!src/js/netjsongraph.render.js (1)
111-120: LGTM!src/js/netjsongraph.util.js (1)
1384-1386: LGTM!public/example_templates/netjsonmap-indoormap-overlay.html (1)
123-131: LGTM!Also applies to: 187-205, 207-246, 274-275, 278-280, 308-316, 360-368, 410-410
dee077
force-pushed
the
feature/402-node-popup-configurable-on-click
branch
from
7b32b3d to
47a3113
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
public/example_templates/netjsonmap-indoormap-overlay.html (1)
360-371: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueRemove redundant mutations of
nodeobject.In lines 368–369, mutations to
node.locationandnode.properties.locationare dead code. Thenodeobject (fromdata.node) is the same reference asthis.data.nodes[index], so these properties were already updated on lines 366–367. Delete lines 368–369 for clarity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/example_templates/netjsonmap-indoormap-overlay.html` around lines 360 - 371, The code mutates the same object twice: inside mapOptions.series[0].data.forEach the properties are already assigned to this.data.nodes[index].location and .properties.location, so the subsequent assignments to node.location and node.properties.location are redundant; remove those two lines (the assignments to node.location and node.properties.location) from the callback in mapOptions.series[0].data.forEach to keep a single update path and avoid dead code.test/netjsongraph.browser.test.js (1)
272-301: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winReplace fixed sleep with explicit wait for indoor overlay readiness.
The
await driver.sleep(500)before waiting for#indoormap-containeris redundant—getElementByCss()already performs an explicit wait usingdriver.wait(until.elementLocated(...))with a 2000ms timeout. On fast systems, the sleep wastes time; on slow systems, it provides no guarantee the element loads within 500ms anyway.Instead, remove the sleep and let the subsequent
getElementByCss()call handle the wait. For stronger guarantees, wait forwindow._indoorMapto be populated using a custom condition:await driver.wait(async () => { return await driver.executeScript('return !!window._indoorMap?.utils'); }, 2000);Then immediately fetch the container element without the sleep.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/netjsongraph.browser.test.js` around lines 272 - 301, Remove the fixed await driver.sleep(500) before querying `#indoormap-container` and instead add an explicit wait that checks for the indoor overlay readiness (e.g., wait for window._indoorMap.utils to be populated) using driver.wait and driver.executeScript, then call getElementByCss(driver, "#indoormap-container", 2000) as before; update the test around getElementByCss, floorplanBtn click, and the indoor map checks (references: driver.sleep, getElementByCss, window._indoorMap, driver.wait) to rely on this explicit readiness condition rather than a hard sleep.
♻️ Duplicate comments (4)
public/example_templates/netjsonmap-indoormap-overlay.html (1)
241-241: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueUse
textContentfor the button label for consistency and safety.The surrounding key/value labels were switched to
textContent(Lines 228, 231). Settingbutton.innerHTML = "Open Floorplan"is a plain literal so it is not exploitable here, but keepinginnerHTMLon a static label is inconsistent with the surrounding hardening and trains future contributors to reach forinnerHTMLreflexively.🔧 Suggested fix
- button.innerHTML = "Open Floorplan"; + button.textContent = "Open Floorplan";🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/example_templates/netjsonmap-indoormap-overlay.html` at line 241, The button label uses innerHTML which is inconsistent with the surrounding use of textContent; change the assignment on the DOM element referenced as button (the "Open Floorplan" label) to use button.textContent = "Open Floorplan" to match the hardening of other labels and avoid encouraging innerHTML usage.src/js/netjsongraph.gui.js (3)
366-368:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate coordinates before calling
toFixed.Line 367 can throw when
location.lat/location.lngare not finite numbers.Proposed fix
const location = node?.location || node?.properties?.location; + const lat = Number(location?.lat); + const lng = Number(location?.lng); + const hasCoords = Number.isFinite(lat) && Number.isFinite(lng); const fields = { name: node?.name, id: node?.id, label: node?.label, - location: location - ? `${location.lat.toFixed(8)}, ${location.lng.toFixed(8)}` - : null, + location: hasCoords ? `${lat.toFixed(8)}, ${lng.toFixed(8)}` : null, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.gui.js` around lines 366 - 368, The location formatting code calls toFixed on location.lat/location.lng without validation; update the location mapping (the object property building the "location" string) to first check that location exists and both Number.isFinite(location.lat) and Number.isFinite(location.lng) are true, and only then produce `${location.lat.toFixed(8)}, ${location.lng.toFixed(8)}`; otherwise return null (or an empty string) to avoid runtime errors when lat/lng are NaN/Infinity/undefined.
302-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winCatch synchronous errors from custom popup content callbacks.
Line 303 executes
popupContent.call(...)before thetryblock, so a synchronous throw escapes and can cause an unhandled rejection because callers do not awaitloadNodePopup.Proposed fix
- } else if (popupContent && typeof popupContent === "function") { - const popupRequest = popupContent.call(this, node, this.self); - this.self.leaflet.currentPopupRequest = popupRequest; - try { - popupContent = await popupRequest; + } else if (popupContent && typeof popupContent === "function") { + let popupRequest; + try { + popupRequest = popupContent.call(this, node, this.self); + this.self.leaflet.currentPopupRequest = popupRequest; + popupContent = await popupRequest; } catch (error) { - if (this.self.leaflet.currentPopupRequest !== popupRequest) { + if (popupRequest && this.self.leaflet.currentPopupRequest !== popupRequest) { return; } console.error("Failed to build node popup content:", error); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.gui.js` around lines 302 - 307, The synchronous call to the custom popup callback (popupContent.call(this, node, this.self)) happens outside the try/catch so any immediate throw escapes; move the invocation and assignment to popupRequest (and setting this.self.leaflet.currentPopupRequest) inside the try block (or wrap the call in its own try) so both synchronous and asynchronous errors are caught, then await popupRequest and handle errors in the catch as currently implemented; reference popupContent, popupRequest, and this.self.leaflet.currentPopupRequest when making the change.
340-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard close-button lookup before attaching listeners.
Lines 340-344 assume
.leaflet-popup-close-buttonexists. If popup config disables the close button, this throws at runtime.Proposed fix
const popupElement = popup .getElement() .querySelector(".leaflet-popup-close-button"); - popupElement.addEventListener("click", () => { + if (!popupElement) { + return; + } + popupElement.addEventListener("click", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.gui.js` around lines 340 - 344, The code assumes popup.getElement().querySelector(".leaflet-popup-close-button") returns an element before calling popupElement.addEventListener, which can throw when the close button is disabled; update the block around popupElement (result of popup.getElement().querySelector(".leaflet-popup-close-button")) to guard for a null/undefined result and only call addEventListener if popupElement is truthy, preserving the existing behavior that checks this.self.config.bookmarkableActions?.enabled inside the handler (i.e., locate the popupElement lookup and addEventListener call and wrap the listener attachment in a null-check).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Around line 443-458: The popstate handler registered as popstateHandler leaks
when the overlay is closed via back navigation because the handler is never
removed; update the fragment-falsy branch inside popstateHandler so that when
you remove the container and clear window._indoorMap you also call
window.removeEventListener("popstate", popstateHandler) (matching the
close-button path), and additionally ensure openIndoorMap does not register
multiple handlers (e.g. check for an existing listener/flag or that
window._indoorMap is falsy before adding the popstate listener) so
popstateHandler (which closes over indoor and uses
indoor.utils.parseUrlFragments and indoor.config.bookmarkableActions.id) is not
added multiple times.
In `@test/netjsongraph.browser.test.js`:
- Line 465: Remove the stray trailing whitespace on the blank line reported in
the test file by deleting the trailing spaces so the line is truly empty (run an
editor trim or run `yarn lint:fix`/Prettier to normalize), ensuring the file no
longer contains trailing spaces that will fail Prettier/ESLint checks.
- Line 619: The closing of the describe block has an extra leading space before
the closing token " });"; locate the describe block in the test (the closing
that currently reads " });") and remove the extra space so the line reads "});"
(i.e., ensure the closing brace and parenthesis are adjacent with no leading
space) to match project formatting and Prettier expectations.
- Line 393: The test call that sets a 10000ms timeout (the third argument to
test()) should be annotated with a one-line comment explaining why it exceeds
the default (the multi-step popstate flow plus three driver.sleep(500) waits can
exceed 5s) and that 10s was chosen as a safe margin; locate the test(...)
invocation in netjsongraph.browser.test.js and add a concise comment referencing
the popstate flow and the three driver.sleep(500) waits so future maintainers
know this is intentional and note that the timeout was validated locally (or
adjust if you confirm a different value is sufficient).
- Around line 272-277: The test block around getElementByCss/floorplanBtn is
missing semicolons and contains trailing whitespace which breaks the repo
Prettier/ESLint rules; update the statements (e.g., the const floorplanBtn
assignment, the expect(floorplanBtn).not.toBeNull(), await floorplanBtn.click(),
and await driver.sleep(500)) to include trailing semicolons and remove any
trailing spaces, then run the required formatter commands (openwisp-qa-format in
the Python venv and yarn lint:fix) to apply consistent Prettier/ESLint fixes
before pushing.
- Around line 306-318: The variable indoorContainer is misnamed because
getElementByCss(driver, ".njg-popup-button", 2000) returns the popup button, not
the indoor overlay element; rename the variable (e.g., popupButton or
indoorPopupButton) and update the assertion to reflect that it checks for the
popup button presence, or alternatively change the selector to target the actual
indoor overlay (`#indoormap-container`) if the intent was to verify the overlay
mounting; update references to indoorContainer in this test block (the
getElementByCss call and the expect(...) assertion) to use the new name or
correct selector so the test semantics match the variable name.
---
Outside diff comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Around line 360-371: The code mutates the same object twice: inside
mapOptions.series[0].data.forEach the properties are already assigned to
this.data.nodes[index].location and .properties.location, so the subsequent
assignments to node.location and node.properties.location are redundant; remove
those two lines (the assignments to node.location and node.properties.location)
from the callback in mapOptions.series[0].data.forEach to keep a single update
path and avoid dead code.
In `@test/netjsongraph.browser.test.js`:
- Around line 272-301: Remove the fixed await driver.sleep(500) before querying
`#indoormap-container` and instead add an explicit wait that checks for the indoor
overlay readiness (e.g., wait for window._indoorMap.utils to be populated) using
driver.wait and driver.executeScript, then call getElementByCss(driver,
"#indoormap-container", 2000) as before; update the test around getElementByCss,
floorplanBtn click, and the indoor map checks (references: driver.sleep,
getElementByCss, window._indoorMap, driver.wait) to rely on this explicit
readiness condition rather than a hard sleep.
---
Duplicate comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Line 241: The button label uses innerHTML which is inconsistent with the
surrounding use of textContent; change the assignment on the DOM element
referenced as button (the "Open Floorplan" label) to use button.textContent =
"Open Floorplan" to match the hardening of other labels and avoid encouraging
innerHTML usage.
In `@src/js/netjsongraph.gui.js`:
- Around line 366-368: The location formatting code calls toFixed on
location.lat/location.lng without validation; update the location mapping (the
object property building the "location" string) to first check that location
exists and both Number.isFinite(location.lat) and Number.isFinite(location.lng)
are true, and only then produce `${location.lat.toFixed(8)},
${location.lng.toFixed(8)}`; otherwise return null (or an empty string) to avoid
runtime errors when lat/lng are NaN/Infinity/undefined.
- Around line 302-307: The synchronous call to the custom popup callback
(popupContent.call(this, node, this.self)) happens outside the try/catch so any
immediate throw escapes; move the invocation and assignment to popupRequest (and
setting this.self.leaflet.currentPopupRequest) inside the try block (or wrap the
call in its own try) so both synchronous and asynchronous errors are caught,
then await popupRequest and handle errors in the catch as currently implemented;
reference popupContent, popupRequest, and this.self.leaflet.currentPopupRequest
when making the change.
- Around line 340-344: The code assumes
popup.getElement().querySelector(".leaflet-popup-close-button") returns an
element before calling popupElement.addEventListener, which can throw when the
close button is disabled; update the block around popupElement (result of
popup.getElement().querySelector(".leaflet-popup-close-button")) to guard for a
null/undefined result and only call addEventListener if popupElement is truthy,
preserving the existing behavior that checks
this.self.config.bookmarkableActions?.enabled inside the handler (i.e., locate
the popupElement lookup and addEventListener call and wrap the listener
attachment in a null-check).
🪄 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: e233ad36-2cf8-40ab-acad-03507fac23df
📒 Files selected for processing (9)
.eslintrc.jspublic/example_templates/netjsonmap-indoormap-overlay.htmlsrc/css/netjsongraph.csssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jstest/netjsongraph.browser.test.jstest/netjsongraph.spec.jstest/netjsongraph.util.test.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). (3)
- GitHub Check: Tests and Coverage (echarts-only)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
test/netjsongraph.util.test.jstest/netjsongraph.spec.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/css/netjsongraph.csssrc/js/netjsongraph.gui.jstest/netjsongraph.browser.test.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.util.test.jstest/netjsongraph.browser.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
test/netjsongraph.browser.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Write browser tests in test/netjsongraph.browser.test.js using Chrome/ChromeDriver and run with
yarn test test/netjsongraph.browser.test.js
Files:
test/netjsongraph.browser.test.js
🧠 Learnings (4)
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
test/netjsongraph.util.test.jstest/netjsongraph.spec.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.jstest/netjsongraph.browser.test.js
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
📚 Learning: 2026-01-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
📚 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
🔇 Additional comments (7)
public/example_templates/netjsonmap-indoormap-overlay.html (1)
308-316: ⚡ Quick winConfirm the default popup renderer kicks in when
content: null.The code at line 300 uses
if (popupContent == null)which correctly handles bothnullandundefinedvalues, falling back tothis.createDefaultPopupContent(node)on line 301. The function invocation at line 303 is guarded by anelse ifblock that only executes whenpopupContentexists and is a function, preventing any throw whencontent: nullis set..eslintrc.js (1)
3-6: LGTM!test/netjsongraph.spec.js (1)
203-211: LGTM!test/netjsongraph.util.test.js (1)
317-320: LGTM!src/css/netjsongraph.css (1)
365-403: LGTM!src/js/netjsongraph.render.js (1)
111-120: LGTM!src/js/netjsongraph.util.js (1)
1384-1386: LGTM!
dee077
force-pushed
the
feature/402-node-popup-configurable-on-click
branch
2 times, most recently
from
87e867b to
488b25a
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
1. Does the PR address the issue properly? Is this the best approach?
Mostly yes. The mapOptions.nodePopup config wired through gui.loadNodePopup matches the direction suggested in #402, and show: false by default keeps backward compatibility, so existing consumers are not affected.
A potentially cleaner alternative worth considering: keep the nodePopup config, but also expose loadNodePopup and createDefaultPopupContent as supported utils methods, so downstream consumers (e.g. openwisp-monitoring) can trigger and extend popups explicitly without reaching into self.gui.
That said, the PR is not yet merge-ready:
- The Coveralls check is failing (see M0 below).
- The PR description's own Todo list still calls out three open items: "Add docs", "Fix/Add tests and CI", and a URL-fragment bug ("when pasting the URL on the current tab, the URL is reverting, and the newly pasted URL fragment state is not applied"). None of these are resolved on the current branch.
- The PR checklist marks documentation and tests as not done.
- Several earlier review comments are answered with "Updated!" but the fix was either incomplete or never landed on the branch (see below for more details)
2. Outstanding issues
Critical
C.0 UX issues found during manual testing
Overlapping popup and labels:
This should be avoided: the two UI elements are redundant because the provide the same info, therefore when the popup is open, the overlay shouldn't be shown at all.
We already worked on this topic extensively in #474, can you reuse the existing logic to prevent the overlapping of these two elements?
To save you some time, here is what I would do. In #474 we introduced updateLabelVisibility in src/js/netjsongraph.render.js:609-629. It is a local closure inside mapRender, and right now it is the single point that toggles series[0].label.show based on showMapLabelsAtZoom and the current zoom level. It is already called from four places: the initial render (:631), the zoomend handler (:634), the post-cluster re-render (:732), and the cluster-threshold change (:772).
The cleanest way to extend it for the popup case is:
-
Promote
updateLabelVisibilityfrom a local closure to a method on the render utility (or attach it toself, e.g.self.updateMapLabelVisibility), so it can be called fromgui.jsas well. -
Add a third condition to the visibility check, so labels are hidden while a popup is open:
const showLabel = self.config.showMapLabelsAtZoom !== false && self.leaflet.getZoom() >= self.config.showMapLabelsAtZoom && !self.leaflet.currentPopup;
-
In
loadNodePopup(src/js/netjsongraph.gui.js:286-356), call this method right afteropenOn(...)setsself.leaflet.currentPopup, and again on the popup close path so labels come back. The existingdispatchAction({type: "hideTip"})at line 292 is already an example of the same pattern (coordinating echarts state with popup state), so this fits naturally next to it. -
Bind a
popup.on('remove', ...)listener (in addition to whatever close-button handling you keep), soself.leaflet.currentPopupis reset andupdateMapLabelVisibilityis re-run for every close path (close button, programmatic close, replacement by another popup). Otherwise labels can stay hidden after the popup is gone.
This way the label-vs-popup overlap is fixed by the same mechanism we already use for label-vs-zoom and label-vs-cluster, instead of introducing a different one.
C1. CSS rules for the default-popup close button never match the DOM.
src/css/netjsongraph.css:366-378.
.njg-container .default-popup a.leaflet-popup-close-button { ... }
.njg-container .default-popup a.leaflet-popup-close-button:hover { ... }Leaflet renders the close button as a sibling of .leaflet-popup-content-wrapper, not as a descendant of .default-popup (which lives inside the content wrapper), so position, top, right, font-size and the !important hover color are silently inert. The scoping intent is fine, but the selector needs to match. A working fix is to mark the popup container itself:
popup.getElement()?.classList.add("njg-default-popup");and then target .njg-container .njg-default-popup .leaflet-popup-close-button. Please apply this before merging.
C2. The null-safety check on nodePopup.show is inconsistent between the two call sites.
src/js/netjsongraph.render.js:115:if (configs.mapOptions.nodePopup.show)(no guard).src/js/netjsongraph.util.js:1384:if (target == null && self.config.mapOptions?.nodePopup?.show)(guarded).
If a downstream consumer sets mapOptions.nodePopup to null, false, or an object without show, the click handler in render.js throws TypeError. Now that .eslintrc.js was bumped to ecmaVersion: "latest" in this same PR, please align both call sites to optional chaining.
C3. The consistent-return shape of the new click handler still looks wrong.
src/js/netjsongraph.render.js:102-121. The handler was reshaped to add return null in the cluster-without-popup branch to satisfy the lint rule, but the earlier branches still return whatever clickElement(...) returns (often undefined). The project does not consume the return value of onClickElement, so all of these return statements are misleading. Please confirm yarn lint actually passes on 488b25a, and ideally drop the explicit returns (onClickElement(...) then return), so the intent is clear.
Major
M0. Coverage drops sharply and Coveralls fails the PR.
The coverage/coveralls check reports coverage decreased -3.6% to 78.47% (build 79484188). Diff coverage is 1.37%, which means only 1 out of 73 new relevant lines is covered.
The drop is concentrated in:
| File | Coverage change |
|---|---|
src/js/netjsongraph.gui.js (around L287, loadNodePopup / createDefaultPopupContent) |
-28.56% |
src/js/netjsongraph.render.js (around L111, new click branches) |
-1.09% |
src/js/netjsongraph.util.js (around L1385, the auto-popup branch) |
-0.09% |
The popup is only exercised by Selenium browser tests, which Jest's coverage instrumentation does not see. There are no Jest specs for loadNodePopup, createDefaultPopupContent, the popupConfigInput null-filtering, the onOpen callback, the request-token async-ordering logic, or the new branches in render.js and util.js. Please add Jest specs that cover at least:
createDefaultPopupContent(node)returning a<div class="default-popup">with the expected children.loadNodePopupbeing a no-op (and logging) whenleafletis unset ornode.locationis missing.popupConfigInputnull-filtering and thecloseOnClickdefault precedence.- The async-ordering token in
loadNodePopup(function-content path). - The
onOpencallback being invoked, and its errors being swallowed. - The new branches in
render.js:111-120andutil.js:1384(including the link-fragment guard).
M1. Empty onClickElement overrides left in the example.
public/example_templates/netjsonmap-indoormap-overlay.html:150 (onClickElement(type, data) {},) and :410 (onClickElement: function () {},). The tests were updated to click through the popup button, so these empty overrides are dead code. Please remove them.
M2. popupElement is dereferenced without a null check.
src/js/netjsongraph.gui.js:340-343:
const popupElement = popup
.getElement()
.querySelector(".leaflet-popup-close-button");
popupElement.addEventListener("click", ...);If a consumer passes mapOptions.nodePopup.config.closeButton: false, Leaflet does not render the close button, so querySelector returns null and addEventListener throws TypeError. Please null-check popupElement before binding the listener, and ideally skip the bookmark-cleanup setup entirely when there is no close button.
M3. Async-ordering protection only covers the function-content path, and URL bookmark is not unwound on failure.
src/js/netjsongraph.gui.js:286-330. The currentPopupRequest token guards against an out-of-order await of two function callbacks, which is good, but:
- Function side effects (e.g. server fetches) still run for the discarded click. Please at least document this in the new config, or expose an
AbortControllerto the callback so user code can cancel cleanly. addActionToUrlruns synchronously inrender.js:104beforeloadNodePopup, so a failed or late popup leaves the URL pointing at a node whose popup never opened. Please unwind the URL bookmark in the failure branch (thecatchinloadNodePopupand the early-return paths at lines 287-298).
Minor
m1. PR description shows offset: [0, 0], actual default is offset: null.
src/js/netjsongraph.config.js:252. The Leaflet default is [0, 7]. Using null (filtered out by Object.fromEntries(... filter(value != null)) in loadNodePopup) means Leaflet's default applies. That is fine, but please make the PR description match the code.
m2. Mixed innerHTML / textContent in the example custom popup.
public/example_templates/netjsonmap-indoormap-overlay.html:241 still uses button.innerHTML = "Open Floorplan";. The string is static, so the risk is zero, but for consistency with the rest of this PR (which moved to textContent for security) please switch this one too.
m3. console.error in production paths silently swallows errors.
src/js/netjsongraph.gui.js:288, 296, 311, 337. Acceptable for UI code, but please consider routing through this.self.event so embedders can hook in.
m4. mapOptions.nodePopup config is not documented.
README.md is unchanged. Without docs, downstream consumers will have to read the source to discover the new option, its defaults, and the closeOnClick: false override. Please add a section in the README.
m5. createDefaultPopupContent is duplicated in the example.
The example's createCustomPopup (HTML lines 207-246) is essentially a copy-paste of createDefaultPopupContent (gui.js:358-388) plus a button. Please either export the default builder so user code can extend it, or document that the example deliberately re-implements it.
m6. closeOnClick: false default differs from Leaflet's default.
src/js/netjsongraph.gui.js:323-326. Spreading popupConfig after closeOnClick: false makes it overridable, which is good. Please mention this in the README, since it is not Leaflet's default.
m7. Redundant truthiness check.
src/js/netjsongraph.gui.js:302: else if (popupContent && typeof popupContent === "function"). The earlier if (popupContent == null) already filters null and undefined, so the leading popupContent && is dead. Please remove it.
m8. createDefaultPopupContent shows raw name, id, label, location keys.
src/js/netjsongraph.gui.js:362-369. The neighbouring getNodeLinkInfo uses a formatKeyLabel helper to humanize keys (Local Addresses, Client, etc.); the default popup shows the raw keys instead. Please reuse the helper for consistency.
m9. PR's own remaining Todo.
"Fix bug when pasting the URL on the current tab, the URL is reverting, and the newly pasted URL fragment state is not applied." Please either resolve it in this PR or split it into a follow-up issue before merge.
3. Status of earlier review threads (only items not fully addressed)
Maintainer (@nemesifier)
consistent-returninrender.jsclick handler. Partially addressed; see C3.- Async click ordering / rejected callbacks. Partially addressed; see M3.
- Empty
onClickElementoverride in the example. Not applied; see M1.
CodeRabbit
- Close-button CSS selector does not match the Leaflet DOM. Not applied; see C1.
- Auto-open should skip link fragments. Logic is correct, but missing test coverage; see M0.
- Custom-popup callback failure guard. Addressed, but the surrounding async semantics still need work; see M3.
4. Recommendation
Please address the following before merge:
- The Coveralls failure (M0). Jest specs for the new
gui.js,render.js, andutil.jsbranches need to land in this PR. - C1 (inert CSS) and C2 (null-safety inconsistency). Both are quick fixes.
- M1, M2, M3 close real bugs and are short fixes.
- Documentation for
mapOptions.nodePopupin the README (m4 / m6).
| #indoormap-container .njg-container .hidden .sideBarHandle { | ||
| left: 35px; | ||
| } | ||
| .njg-container .njg-sideBar { |
There was a problem hiding this comment.
If I comment out this, I see no difference. What does this do?
| .njg-container .njg-sideBar { | ||
| display: none; | ||
| } | ||
| .njg-container .leaflet-popup-tip-container { |
There was a problem hiding this comment.
If I comment this out, I see:
Is this rule something that would be always needed? If so, move it to the main CSS.
Not sure why it's here?
I think 3.5% works better visually.
| display: flex; | ||
| justify-content: center; |
There was a problem hiding this comment.
Wouldn't this simpler alternative obtain the same effect?
| display: flex; | |
| justify-content: center; | |
| text-align: center |
| } | ||
| .njg-container .leaflet-popup-tip { | ||
| box-shadow: none; | ||
| } |
There was a problem hiding this comment.
Same as .njg-container .leaflet-popup-tip-container and should be listed inmediately after that rule.
Added support for configuring node popup behavior through mapOptions.nodePopup, allowing users to customize popup settings and click behavior through configuration. Fixes #402
dee077
force-pushed
the
feature/402-node-popup-configurable-on-click
branch
from
488b25a to
8119346
Compare
ESLint Failures in JavaScript FilesHello @dee077, The following ESLint errors were detected:
To fix these issues, please run the following command in your local environment: |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
public/example_templates/netjsonmap-indoormap-overlay.html (1)
450-455:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRemove stale
popstatelistener when closing via browser Back.Line 454 clears
window._indoorMap, but this branch leaves thepopstatelistener attached, so reopen cycles can stack stale handlers.🔧 Minimal fix
} else { const container = document.getElementById("indoormap-container"); if (container) { container.remove(); window._indoorMap = null; + window.removeEventListener("popstate", popstateHandler); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@public/example_templates/netjsonmap-indoormap-overlay.html` around lines 450 - 455, The close branch removes the DOM container and clears window._indoorMap but leaves the popstate listener attached, causing duplicate handlers on reopen; update the close logic to remove the popstate listener (call window.removeEventListener('popstate', handler)) and clear any saved handler reference (e.g. window._indoorMap.popstateHandler) before nulling window._indoorMap, and ensure when the listener is originally added you save the handler function onto window._indoorMap (or a known property) so removeEventListener can target the exact same function.src/js/netjsongraph.render.js (1)
115-116:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard
nodePopupaccess in the click path.
configs.mapOptions.nodePopup.showcan throw whenmapOptionsis overridden withoutnodePopup.Proposed fix
- if (configs.mapOptions.nodePopup.show) { + const nodePopupEnabled = + configs.mapOptions && + configs.mapOptions.nodePopup && + configs.mapOptions.nodePopup.show; + if (nodePopupEnabled) { self.gui.loadNodePopup(params.data.node); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.render.js` around lines 115 - 116, Guard access to nodePopup before calling the popup loader: replace the direct check of configs.mapOptions.nodePopup.show with a safe existence check (e.g. use configs.mapOptions && configs.mapOptions.nodePopup && configs.mapOptions.nodePopup.show or optional chaining like configs.mapOptions?.nodePopup?.show) and ensure self.gui.loadNodePopup is only invoked when that guard passes; locate the click handler containing configs.mapOptions.nodePopup.show and the call to self.gui.loadNodePopup(params.data.node) and wrap the call with the described guard.src/js/netjsongraph.gui.js (2)
302-307:⚠️ Potential issue | 🟠 Major | ⚡ Quick winWrap custom popup callback invocation in
try/catch.
popupContent.call(...)can throw synchronously before theawait, which currently escapes and breaks click handling.Proposed fix
- } else if (popupContent && typeof popupContent === "function") { - const popupRequest = popupContent.call(this, node, this.self); + } else if (popupContent && typeof popupContent === "function") { + let popupRequest; + try { + popupRequest = popupContent.call(this, node, this.self); + } catch (error) { + console.error("Failed to build node popup content:", error); + return; + } this.self.leaflet.currentPopupRequest = popupRequest; try { popupContent = await popupRequest;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.gui.js` around lines 302 - 307, The invocation of the custom popup callback (popupContent.call(this, node, this.self)) can throw synchronously; wrap that call in an outer try/catch so synchronous exceptions are caught and handled the same way as rejected popupRequest promises. Specifically, move the creation of popupRequest and the assignment this.self.leaflet.currentPopupRequest into a try block, and add a catch that handles synchronous errors (e.g., set popupContent to undefined/null and run the same error handling/logging as the existing inner catch). Keep the inner await popupRequest try/catch as-is so promise rejections are still handled.
340-344:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGuard close-button lookup before binding click listener.
If popup close button is disabled (
nodePopup.config.closeButton = false),querySelector(...)returnsnulland this throws onaddEventListener.Proposed fix
- const popupElement = popup - .getElement() - .querySelector(".leaflet-popup-close-button"); - popupElement.addEventListener("click", () => { + const popupElement = popup + .getElement() + ?.querySelector(".leaflet-popup-close-button"); + if (!popupElement) { + return; + } + popupElement.addEventListener("click", () => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/js/netjsongraph.gui.js` around lines 340 - 344, The code assumes popup.getElement().querySelector(".leaflet-popup-close-button") always returns an element before calling addEventListener; if nodePopup.config.closeButton is false this is null and throws. Fix by null-guarding the lookup: assign const popupElement = popup.getElement().querySelector(".leaflet-popup-close-button"); then only call popupElement.addEventListener(...) when popupElement is truthy (e.g., if (popupElement) { popupElement.addEventListener(...); }), keeping the existing check for this.self.config.bookmarkableActions?.enabled inside that handler.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/netjsongraph.render.test.js`:
- Around line 1856-1858: Remove the conditional guard so the test always
verifies the popup loader: replace the "if (graph.gui) {
expect(graph.gui.loadNodePopup).toBeInstanceOf(Function); }" block with
unconditional assertions such as "expect(graph.gui).toBeDefined();" followed by
"expect(graph.gui.loadNodePopup).toBeInstanceOf(Function);" so the test fails if
graph.gui or its loadNodePopup method is missing.
- Line 1837: The tests shadow the module-scope variable graph and use a dynamic
require for the GUI; rename the inner test variables from graph to localGraph
(update all declarations that currently shadow the top-level graph, e.g., in
NetJSONGraphCore test cases) and add a static import for NetJSONGraphGUI at the
top of the file, then replace the dynamic require(...) usage with the imported
NetJSONGraphGUI class so all references use localGraph and NetJSONGraphGUI
consistently.
---
Duplicate comments:
In `@public/example_templates/netjsonmap-indoormap-overlay.html`:
- Around line 450-455: The close branch removes the DOM container and clears
window._indoorMap but leaves the popstate listener attached, causing duplicate
handlers on reopen; update the close logic to remove the popstate listener (call
window.removeEventListener('popstate', handler)) and clear any saved handler
reference (e.g. window._indoorMap.popstateHandler) before nulling
window._indoorMap, and ensure when the listener is originally added you save the
handler function onto window._indoorMap (or a known property) so
removeEventListener can target the exact same function.
In `@src/js/netjsongraph.gui.js`:
- Around line 302-307: The invocation of the custom popup callback
(popupContent.call(this, node, this.self)) can throw synchronously; wrap that
call in an outer try/catch so synchronous exceptions are caught and handled the
same way as rejected popupRequest promises. Specifically, move the creation of
popupRequest and the assignment this.self.leaflet.currentPopupRequest into a try
block, and add a catch that handles synchronous errors (e.g., set popupContent
to undefined/null and run the same error handling/logging as the existing inner
catch). Keep the inner await popupRequest try/catch as-is so promise rejections
are still handled.
- Around line 340-344: The code assumes
popup.getElement().querySelector(".leaflet-popup-close-button") always returns
an element before calling addEventListener; if nodePopup.config.closeButton is
false this is null and throws. Fix by null-guarding the lookup: assign const
popupElement = popup.getElement().querySelector(".leaflet-popup-close-button");
then only call popupElement.addEventListener(...) when popupElement is truthy
(e.g., if (popupElement) { popupElement.addEventListener(...); }), keeping the
existing check for this.self.config.bookmarkableActions?.enabled inside that
handler.
In `@src/js/netjsongraph.render.js`:
- Around line 115-116: Guard access to nodePopup before calling the popup
loader: replace the direct check of configs.mapOptions.nodePopup.show with a
safe existence check (e.g. use configs.mapOptions &&
configs.mapOptions.nodePopup && configs.mapOptions.nodePopup.show or optional
chaining like configs.mapOptions?.nodePopup?.show) and ensure
self.gui.loadNodePopup is only invoked when that guard passes; locate the click
handler containing configs.mapOptions.nodePopup.show and the call to
self.gui.loadNodePopup(params.data.node) and wrap the call with the described
guard.
🪄 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: 204758f5-9267-4474-b4a1-3de7cedd7eb2
📒 Files selected for processing (12)
.eslintrc.jspublic/example_templates/netjsonmap-indoormap-overlay.htmlsrc/css/netjsongraph.csssrc/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jstest/netjsongraph.browser.test.jstest/netjsongraph.dom.test.jstest/netjsongraph.render.test.jstest/netjsongraph.spec.jstest/netjsongraph.util.test.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). (5)
- GitHub Check: Tests and Coverage (echarts-only)
- GitHub Check: Tests and Coverage (dev)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jstest/netjsongraph.dom.test.jssrc/js/netjsongraph.util.jstest/netjsongraph.spec.jssrc/css/netjsongraph.csstest/netjsongraph.util.test.jstest/netjsongraph.render.test.jssrc/js/netjsongraph.gui.jstest/netjsongraph.browser.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.dom.test.jstest/netjsongraph.util.test.jstest/netjsongraph.render.test.jstest/netjsongraph.browser.test.js
test/netjsongraph.browser.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Write browser tests in test/netjsongraph.browser.test.js using Chrome/ChromeDriver and run with
yarn test test/netjsongraph.browser.test.js
Files:
test/netjsongraph.browser.test.js
🧠 Learnings (4)
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jstest/netjsongraph.dom.test.jssrc/js/netjsongraph.util.jstest/netjsongraph.spec.jstest/netjsongraph.util.test.jstest/netjsongraph.render.test.jssrc/js/netjsongraph.gui.jstest/netjsongraph.browser.test.js
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
📚 Learning: 2026-01-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
📚 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
🪛 ESLint
test/netjsongraph.render.test.js
[error] 1837-1837: 'graph' is already declared in the upper scope on line 37 column 7.
(no-shadow)
[error] 1875-1875: 'graph' is already declared in the upper scope on line 37 column 7.
(no-shadow)
[error] 1893-1893: 'graph' is already declared in the upper scope on line 37 column 7.
(no-shadow)
[error] 1899-1899: Unexpected require().
(global-require)
[error] 1899-1899: A constructor name should not start with a lowercase letter.
(new-cap)
[error] 1931-1931: 'graph' is already declared in the upper scope on line 37 column 7.
(no-shadow)
🪛 GitHub Check: QA-Checks
test/netjsongraph.render.test.js
[failure] 1931-1931:
'graph' is already declared in the upper scope on line 37 column 7
[failure] 1899-1899:
A constructor name should not start with a lowercase letter
[failure] 1899-1899:
Unexpected require()
[failure] 1893-1893:
'graph' is already declared in the upper scope on line 37 column 7
[failure] 1875-1875:
'graph' is already declared in the upper scope on line 37 column 7
[failure] 1837-1837:
'graph' is already declared in the upper scope on line 37 column 7
🔇 Additional comments (9)
src/css/netjsongraph.css (1)
365-403: LGTM!public/example_templates/netjsonmap-indoormap-overlay.html (1)
76-100: LGTM!Also applies to: 123-131, 150-150, 187-246, 274-280, 308-317, 360-367, 410-410, 446-449
test/netjsongraph.browser.test.js (1)
172-172: LGTM!Also applies to: 197-198, 218-218, 244-245, 271-276, 305-312, 317-317, 327-339, 345-345, 353-359, 367-373, 381-387, 465-465
src/js/netjsongraph.config.js (1)
248-256: LGTM!.eslintrc.js (1)
3-6: LGTM!test/netjsongraph.spec.js (1)
203-211: LGTM!test/netjsongraph.util.test.js (1)
317-320: LGTM!Also applies to: 436-644
src/js/netjsongraph.util.js (1)
1384-1386: LGTM!test/netjsongraph.dom.test.js (1)
446-585: LGTM!
| const container = document.createElement("div"); | ||
| container.setAttribute("id", "map"); | ||
| document.body.appendChild(container); | ||
| const graph = new NetJSONGraphCore(data); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify lint-triggering patterns in the changed block
rg -n '^\s*const graph = new NetJSONGraphCore\(' test/netjsongraph.render.test.js
rg -n 'new \(require\("../src/js/netjsongraph\.gui"\)\.default\)' test/netjsongraph.render.test.jsRepository: openwisp/netjsongraph.js
Length of output: 409
🏁 Script executed:
#!/bin/bash
# Check the file structure and context around the problematic lines
echo "=== Checking file size ==="
wc -l test/netjsongraph.render.test.js
echo -e "\n=== Context around line 1837 (first issue) ==="
sed -n '1830,1845p' test/netjsongraph.render.test.js
echo -e "\n=== Context around line 1899 (dynamic require) ==="
sed -n '1892,1910p' test/netjsongraph.render.test.js
echo -e "\n=== Check for global graph variable or imports ==="
head -50 test/netjsongraph.render.test.js | grep -E "^(import|const|let|var|global)" | head -20Repository: openwisp/netjsongraph.js
Length of output: 1619
🏁 Script executed:
#!/bin/bash
# Check ESLint configuration
echo "=== ESLint configuration ==="
if [ -f .eslintrc ]; then
cat .eslintrc
elif [ -f .eslintrc.js ]; then
cat .eslintrc.js
elif [ -f .eslintrc.json ]; then
cat .eslintrc.json
elif [ -f package.json ]; then
jq '.eslintConfig' package.json 2>/dev/null || echo "No eslintConfig in package.json"
fi
echo -e "\n=== Check for 'no-shadow' rule configuration ==="
rg -A 2 'no-shadow' .eslintrc* package.json 2>/dev/nullRepository: openwisp/netjsongraph.js
Length of output: 534
🏁 Script executed:
#!/bin/bash
# Verify the context of all mentioned lines to understand shadowing scope
echo "=== Line 37 context ==="
sed -n '30,45p' test/netjsongraph.render.test.js
echo -e "\n=== Lines 1875-1880 context ==="
sed -n '1870,1885p' test/netjsongraph.render.test.js
echo -e "\n=== Lines 1893-1910 context ==="
sed -n '1890,1915p' test/netjsongraph.render.test.js
echo -e "\n=== Lines 1931-1945 context ==="
sed -n '1925,1950p' test/netjsongraph.render.test.jsRepository: openwisp/netjsongraph.js
Length of output: 2714
Resolve shadowing conflicts and replace dynamic require with static import.
The variable graph at module scope (line 37) is shadowed by graph declarations in nested test scopes at lines 1837, 1875, 1893, and 1931, triggering no-shadow. Line 1899 uses dynamic require instead of a static import, triggering global-require. Rename all shadowing variables to localGraph, add a static import for NetJSONGraphGUI, and replace the dynamic require with the imported class.
Suggested fix
import L from "leaflet";
import NetJSONGraphCore from "../src/js/netjsongraph.core";
import NetJSONGraphRender from "../src/js/netjsongraph.render";
+import NetJSONGraphGUI from "../src/js/netjsongraph.gui";
...
- const graph = new NetJSONGraphCore(data);
- graph.event = graph.utils.createEvent();
- graph.setConfig({
+ const localGraph = new NetJSONGraphCore(data);
+ localGraph.event = localGraph.utils.createEvent();
+ localGraph.setConfig({
el: container,
mapOptions: {
nodePopup: {
...
- graph.setUtils();
- expect(graph.config.mapOptions.nodePopup).toBeDefined();
- expect(graph.config.mapOptions.nodePopup.show).toBe(true);
- expect(graph.config.mapOptions.nodePopup.config.autoPan).toBe(true);
- if (graph.gui) {
- expect(graph.gui.loadNodePopup).toBeInstanceOf(Function);
- }
+ localGraph.setUtils();
+ expect(localGraph.config.mapOptions.nodePopup).toBeDefined();
+ expect(localGraph.config.mapOptions.nodePopup.show).toBe(true);
+ expect(localGraph.config.mapOptions.nodePopup.config.autoPan).toBe(true);
+ expect(localGraph.gui).toBeDefined();
+ expect(localGraph.gui.loadNodePopup).toBeInstanceOf(Function);
...
- const graph = new NetJSONGraphCore(data);
- graph.event = graph.utils.createEvent();
- graph.setConfig({
+ const localGraph = new NetJSONGraphCore(data);
+ localGraph.event = localGraph.utils.createEvent();
+ localGraph.setConfig({
...
- graph.setUtils();
- expect(graph.config.mapOptions.nodePopup.show).toBe(false);
+ localGraph.setUtils();
+ expect(localGraph.config.mapOptions.nodePopup.show).toBe(false);
...
- const graph = new NetJSONGraphCore({
+ const localGraph = new NetJSONGraphCore({
type: "NetworkGraph",
nodes: [],
links: [],
});
- graph.event = graph.utils.createEvent();
- graph.gui = new (require("../src/js/netjsongraph.gui").default)(graph);
+ localGraph.event = localGraph.utils.createEvent();
+ localGraph.gui = new NetJSONGraphGUI(localGraph);
...
- const popupContent = graph.gui.createDefaultPopupContent(node);
+ const popupContent = localGraph.gui.createDefaultPopupContent(node);
...
- const graph = new NetJSONGraphCore(data);
- graph.event = graph.utils.createEvent();
- graph.setConfig({
+ const localGraph = new NetJSONGraphCore(data);
+ localGraph.event = localGraph.utils.createEvent();
+ localGraph.setConfig({
...
- graph.setUtils();
- expect(graph.config.mapOptions.nodePopup.content).toBe(customContentHandler);
- expect(graph.config.mapOptions.nodePopup.onOpen).toBe(onOpenHandler);
- expect(graph.config.mapOptions.nodePopup.config.autoPan).toBe(false);
- expect(graph.config.mapOptions.nodePopup.config.offset).toEqual([10, 20]);
+ localGraph.setUtils();
+ expect(localGraph.config.mapOptions.nodePopup.content).toBe(customContentHandler);
+ expect(localGraph.config.mapOptions.nodePopup.onOpen).toBe(onOpenHandler);
+ expect(localGraph.config.mapOptions.nodePopup.config.autoPan).toBe(false);
+ expect(localGraph.config.mapOptions.nodePopup.config.offset).toEqual([10, 20]);Also applies to: 1875-1875, 1893-1893, 1899-1899, 1931-1931
🧰 Tools
🪛 ESLint
[error] 1837-1837: 'graph' is already declared in the upper scope on line 37 column 7.
(no-shadow)
🪛 GitHub Check: QA-Checks
[failure] 1837-1837:
'graph' is already declared in the upper scope on line 37 column 7
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/netjsongraph.render.test.js` at line 1837, The tests shadow the
module-scope variable graph and use a dynamic require for the GUI; rename the
inner test variables from graph to localGraph (update all declarations that
currently shadow the top-level graph, e.g., in NetJSONGraphCore test cases) and
add a static import for NetJSONGraphGUI at the top of the file, then replace the
dynamic require(...) usage with the imported NetJSONGraphGUI class so all
references use localGraph and NetJSONGraphGUI consistently.
| if (graph.gui) { | ||
| expect(graph.gui.loadNodePopup).toBeInstanceOf(Function); | ||
| } |
There was a problem hiding this comment.
Make the popup-loader assertion unconditional.
This assertion currently becomes a no-op when graph.gui is missing, so the test can pass without verifying the intended behavior.
Suggested fix
- if (graph.gui) {
- expect(graph.gui.loadNodePopup).toBeInstanceOf(Function);
- }
+ expect(graph.gui).toBeDefined();
+ expect(graph.gui.loadNodePopup).toBeInstanceOf(Function);📝 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.
| if (graph.gui) { | |
| expect(graph.gui.loadNodePopup).toBeInstanceOf(Function); | |
| } | |
| expect(graph.gui).toBeDefined(); | |
| expect(graph.gui.loadNodePopup).toBeInstanceOf(Function); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/netjsongraph.render.test.js` around lines 1856 - 1858, Remove the
conditional guard so the test always verifies the popup loader: replace the "if
(graph.gui) { expect(graph.gui.loadNodePopup).toBeInstanceOf(Function); }" block
with unconditional assertions such as "expect(graph.gui).toBeDefined();"
followed by "expect(graph.gui.loadNodePopup).toBeInstanceOf(Function);" so the
test fails if graph.gui or its loadNodePopup method is missing.
2 participants
Added support for configuring node popup behavior through mapOptions.nodePopup, allowing users to customize popup settings and click behavior through configuration.
Fixes #402
Checklist
Reference to Existing Issue
Closes #402.
Description of Changes
Screenrecording
Screencast.from.2026-05-14.03-49-22.webm
Todo