[feature] Node/link visual highlighting #163#577
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR implements node and link highlighting for graph and map views. It adds highlight state tracking, action resolution, hover handling, and ECharts dispatch logic in Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant NetJSONGraphRender
participant NetJSONGraphUtil
participant Echarts
User->>NetJSONGraphRender: click node or link
NetJSONGraphRender->>NetJSONGraphUtil: highlightNode/highlightLink
NetJSONGraphUtil->>NetJSONGraphUtil: resolve highlight actions
NetJSONGraphUtil->>Echarts: dispatchAction(highlight/showTip)
NetJSONGraphUtil->>NetJSONGraphUtil: update activeHighlightedElements
User->>NetJSONGraphRender: hover element
NetJSONGraphUtil->>Echarts: dispatchAction(highlight/downplay/hideTip)
Related issue:
Suggested labels: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (3 errors, 2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
enhancement
javascript
Dhruv-ub
force-pushed
the
feature/163-Node-link-visual-highlighting
branch
from
0a96c98 to
4cdebca
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/netjsongraph.render.js (1)
171-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStray blank line inside the node-mapping callback.
Line 176 introduces an unnecessary blank line inside the function body.
🧹 Fix
const seriesEmphasis = (configs.graphConfig.series || {}).emphasis || {}; - nodeResult.itemStyle = nodeStyleConfig;As per coding guidelines, "Avoid unnecessary blank lines inside functions and methods."
🤖 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 171 - 201, Remove the unnecessary blank line inside the node-mapping callback in netjsongraph.render.js; the nodes.map handler that builds nodeResult should stay compact and follow the coding guideline against extra blank lines inside functions. Keep the existing logic in the callback around getNodeStyle, resolvedName assignment, and _source preservation, but delete the stray empty line so the function body flows without gaps.Source: Coding guidelines
🤖 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.render.js`:
- Around line 99-151: The click listener registration in netjsongraph.render.js
uses echartsLayer.on with a third argument that is not a passive option in
ECharts, so remove the {passive: true} argument from the
self.echartsClickHandler registration. Keep the handler setup in the same place
as the echartsLayer.off/on logic, and preserve the existing
self.echartsClickHandler arrow function behavior without implying DOM-style
listener options.
In `@test/netjsongraph.render.test.js`:
- Around line 1413-1544: The test bodies in NetJSONGraphRender’s highlight
emphasis option generation spec contain unnecessary blank lines between arrange,
act, and assert sections. Remove the extra blank lines inside the two test
functions so they follow the code style guideline of keeping function bodies
compact, while preserving the same assertions on generateGraphOption and
generateMapOption.
In `@test/netjsongraph.util.test.js`:
- Line 1149: There are unnecessary blank lines inside several test bodies in
netjsongraph.util.test.js, which breaks the preferred arrange/act/assert
spacing. Clean up the affected test cases by removing the extra empty lines
within the bodies around the referenced blocks so each test stays compact and
follows the same formatting pattern. Use the nearby test functions in the file
as the targets for the cleanup.
- Around line 1126-1175: highlightNode is expected to highlight connected links
too, but resolveNodeHighlightActions only matches the node against
graph/scatter/effectScatter series and ignores this.data.links. Update
resolveNodeHighlightActions (and any helper it uses) to also inspect the util’s
link data and add highlight actions for links whose source/target connect to the
selected node, so highlightNode can dispatch the extra link highlight and track
both activeHighlightedElements entries.
---
Outside diff comments:
In `@src/js/netjsongraph.render.js`:
- Around line 171-201: Remove the unnecessary blank line inside the node-mapping
callback in netjsongraph.render.js; the nodes.map handler that builds nodeResult
should stay compact and follow the coding guideline against extra blank lines
inside functions. Keep the existing logic in the callback around getNodeStyle,
resolvedName assignment, and _source preservation, but delete the stray empty
line so the function body flows without gaps.
🪄 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: fd72cd63-0337-4455-88b3-acd24fe4819a
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (7)
src/js/netjsongraph.config.jssrc/js/netjsongraph.core.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.jstest/netjsongraph.render.test.jstest/netjsongraph.spec.jstest/netjsongraph.util.test.js
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Avoid unnecessary blank lines inside functions and methods
Write comments only when they explain why code is shaped a certain way, and place comments before the relevant block instead of scattering them inside it
Files:
src/js/netjsongraph.core.jssrc/js/netjsongraph.config.jstest/netjsongraph.spec.jstest/netjsongraph.render.test.jstest/netjsongraph.util.test.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.util.js
🧠 Learnings (5)
📚 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.core.jssrc/js/netjsongraph.config.jstest/netjsongraph.spec.jstest/netjsongraph.render.test.jstest/netjsongraph.util.test.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.core.jssrc/js/netjsongraph.config.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-05-19T23:25:39.778Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 542
File: src/js/netjsongraph.util.js:1478-1481
Timestamp: 2026-05-19T23:25:39.778Z
Learning: When reviewing netjsongraph label rendering logic in src/js/netjsongraph.util.js and src/js/netjsongraph.render.js, follow the documented contract for showMapLabelsAtZoom: (1) if showMapLabelsAtZoom is false, map labels must be completely disabled; (2) if it is 0, labels must always be shown regardless of zoom (do not treat 0 as disabled); (3) if it is a positive number N, labels must be shown only when zoom >= N. Ensure comparisons implement the intended semantics (i.e., zoom >= 0 is intentionally always true when the setting is 0).
Applied to files:
src/js/netjsongraph.render.jssrc/js/netjsongraph.util.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 (16)
src/js/netjsongraph.util.js (7)
1375-1386: 🎯 Functional Correctness | 💤 Low valueMinor:
highlightLinklacks the same invalid-input guard ashighlightNode.If
linkDatais falsy,onClickElementis still invoked with it (sinceresolveLinkHighlightActionsjust returns[]rather than short-circuitinghighlightLink). Current call sites always pass valid link data, so this is low-impact today.
1144-1160: LGTM!
1230-1268: 🎯 Functional Correctness | 🏗️ Heavy liftNode highlighting doesn't resolve connected links, but tests expect it to.
The full-series-search branch here only matches the node itself in
graph/scatter/effectScatterseries. There is no lookup ofgraph.links/edgesorlinesseries data for entries whosesource/targetmatchnodeData.id. However, the associated test (highlightNode highlights the node and connected links...) expects a secondhighlightdispatch against a matchinglinesentry andactiveHighlightedElementsto have length 2. As written, only one action would be produced/dispatched, so the test would fail.Could you confirm whether connected-link highlighting is an intended part of `highlightNode`'s public-API contract? If so, this needs the additional lookup above.🐛 Suggested addition
series.forEach((serie, seriesIndex) => { if (!serie) { return; } if (serie.type === "graph") { const nodes = serie.nodes || serie.data || []; const dataIndex = nodes.findIndex((item) => item && item.id === nodeData.id); if (dataIndex !== -1) { actions.push({seriesIndex, dataIndex, dataType: "node"}); } + const links = serie.links || serie.edges || []; + links.forEach((edge, edgeIndex) => { + if (edge && (edge.source === nodeData.id || edge.target === nodeData.id)) { + actions.push({seriesIndex, dataIndex: edgeIndex, dataType: "edge", showTip: false}); + } + }); } else if (serie.type === "scatter" || serie.type === "effectScatter") { const data = serie.data || []; const dataIndex = data.findIndex( (item) => item && item.node && item.node.id === nodeData.id, ); if (dataIndex !== -1) { actions.push({seriesIndex, dataIndex}); } + } else if (serie.type === "lines") { + const data = serie.data || []; + data.forEach((item, dataIndex) => { + if ( + item && + item.link && + (item.link.source === nodeData.id || item.link.target === nodeData.id) + ) { + actions.push({seriesIndex, dataIndex, showTip: false}); + } + }); } });
1270-1323: LGTM!
1388-1427: LGTM!
1470-1501: 🩺 Stability & Availability | ⚡ Quick winVerify lifecycle cleanup for the global
keyuplistener.
window.addEventListener("keyup", keyup)is only ever removed by a subsequent call tosetupHighlightEventHandlers(on re-render), not on full widget disposal. If instances ofNetJSONGraphCoreare created/destroyed repeatedly (e.g., in an SPA), this closure keepsself(and everything it references) alive. This mirrors other unmanaged global handlers already in the file (e.g.window.onresizeingraphRender), so it may be an accepted pattern, but worth confirming there's a teardown path elsewhere in the codebase.
1162-1468: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winRemove the dead
this.utils ? ... : ...dual-mode branching.Nearly every new helper repeats
this.utils ? this.utils.fn.call(this, ...) : this.fn(...). Given the codebase's established convention (util methods are always invoked withthisbound to the core instance, e.g.this.utils.JSONParamParse(...)inpaginatedDataParse),this.utilsis always defined when these helpers run — theelsefallback is unreachable in every call site and in the test mocks shown. This adds a lot of noise across ~15 methods for no behavioral benefit.♻️ Example simplification (repeat pattern across the section)
- isMultiSelectHighlight(options = {}) { - const event = this.utils - ? this.utils.getHighlightEvent(options) - : this.getHighlightEvent(options); - return Boolean(options.multiSelect || options.append || (event && event.ctrlKey)); - } + isMultiSelectHighlight(options = {}) { + const event = this.utils.getHighlightEvent(options); + return Boolean(options.multiSelect || options.append || (event && event.ctrlKey)); + }> Likely an incorrect or invalid review comment.src/js/netjsongraph.config.js (1)
92-103: LGTM!Also applies to: 194-199, 216-218
src/js/netjsongraph.core.js (1)
215-219: LGTM!src/js/netjsongraph.render.js (2)
202-255: LGTM!
291-467: LGTM!test/netjsongraph.render.test.js (2)
1414-1473: LGTM!generateGraphOptionemphasis assertions match the merge logic shown in the render.js contract snippet.
1475-1543: LGTM!generateMapOptionemphasis assertions are consistent with the map option merge logic.test/netjsongraph.spec.js (1)
48-57: LGTM!Also applies to: 147-150, 167-169
test/netjsongraph.util.test.js (2)
1177-1220: LGTM! Ctrl-key multi-select behavior forhighlightLinktraces correctly againstisMultiSelectHighlight/dispatchHighlightActions.
1222-1245: LGTM!clearHighlightdownplay +hideTip+ state-clear assertions match the implementation.
| echartsLayer.setOption(self.utils.deepMergeObj(commonOption, customOption)); | ||
| echartsLayer.on( | ||
| "click", | ||
| (params) => { | ||
| const clickElement = configs.onClickElement.bind(self); | ||
| self.utils.addActionToUrl(self, params); | ||
| if (params.componentSubType === "graph") { | ||
| clickElement(params.dataType === "edge" ? "link" : "node", params.data); | ||
| return; | ||
| } | ||
| if (params.componentSubType === "lines") { | ||
| clickElement("link", params.data.link); | ||
| return; | ||
| if (self.echartsClickHandler && typeof echartsLayer.off === "function") { | ||
| echartsLayer.off("click", self.echartsClickHandler); | ||
| } | ||
| self.echartsClickHandler = (params) => { | ||
| self.utils.addActionToUrl(self, params); | ||
| if (params.componentSubType === "graph") { | ||
| if (params.dataType === "edge") { | ||
| self.utils.highlightLink(params.data, { | ||
| openTooltip: true, | ||
| showInfo: true, | ||
| event: params.event, | ||
| seriesIndex: params.seriesIndex, | ||
| dataIndex: params.dataIndex, | ||
| dataType: "edge", | ||
| }); | ||
| } else { | ||
| self.utils.highlightNode(params.data, { | ||
| openTooltip: true, | ||
| showInfo: true, | ||
| event: params.event, | ||
| seriesIndex: params.seriesIndex, | ||
| dataIndex: params.dataIndex, | ||
| dataType: "node", | ||
| }); | ||
| } | ||
| if (params.data && !params.data.cluster) { | ||
| if (configs.mapOptions?.nodePopup?.show) { | ||
| self.gui.loadNodePopup(params.data.node); | ||
| } | ||
| clickElement("node", params.data.node); | ||
| return; | ||
| } | ||
| if (params.componentSubType === "lines") { | ||
| self.utils.highlightLink(params.data.link, { | ||
| openTooltip: false, | ||
| showInfo: true, | ||
| event: params.event, | ||
| seriesIndex: params.seriesIndex, | ||
| dataIndex: params.dataIndex, | ||
| }); | ||
| return; | ||
| } | ||
| if (params.data && !params.data.cluster) { | ||
| if (configs.mapOptions?.nodePopup?.show) { | ||
| self.gui.loadNodePopup(params.data.node); | ||
| } | ||
| }, | ||
| {passive: true}, | ||
| ); | ||
| self.utils.highlightNode(params.data.node, { | ||
| openTooltip: true, | ||
| showInfo: true, | ||
| event: params.event, | ||
| seriesIndex: params.seriesIndex, | ||
| dataIndex: params.dataIndex, | ||
| }); | ||
| } | ||
| }; | ||
| echartsLayer.on("click", self.echartsClickHandler, {passive: true}); | ||
| self.utils.setupHighlightEventHandlers(self); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
{passive: true} third argument to echartsLayer.on() has no effect.
ECharts' on(eventName, handler, context) third parameter is a this-binding context, not a passive-listener option — this isn't the DOM addEventListener API. Since self.echartsClickHandler is an arrow function, this argument does nothing either way; consider removing it to avoid implying non-existent passive-listener behavior.
🤖 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 99 - 151, The click listener
registration in netjsongraph.render.js uses echartsLayer.on with a third
argument that is not a passive option in ECharts, so remove the {passive: true}
argument from the self.echartsClickHandler registration. Keep the handler setup
in the same place as the echartsLayer.off/on logic, and preserve the existing
self.echartsClickHandler arrow function behavior without implying DOM-style
listener options.
| test("highlightNode highlights the node and connected links with node tooltip", () => { | ||
| const util = new NetJSONGraphUtil(); | ||
| const node = {id: "node-1"}; | ||
| const link = {source: "node-1", target: "node-2"}; | ||
| const mockSelf = { | ||
| utils: util, | ||
| data: {nodes: [node, {id: "node-2"}], links: [link]}, | ||
| config: {onClickElement: jest.fn()}, | ||
| echarts: { | ||
| getOption: jest.fn(() => ({ | ||
| series: [ | ||
| { | ||
| type: "scatter", | ||
| data: [{node}, {node: {id: "node-2"}}], | ||
| }, | ||
| { | ||
| type: "lines", | ||
| data: [{link}], | ||
| }, | ||
| ], | ||
| })), | ||
| dispatchAction: jest.fn(), | ||
| }, | ||
| }; | ||
|
|
||
| util.highlightNode.call(mockSelf, node, {openTooltip: true, showInfo: true}); | ||
|
|
||
| expect(mockSelf.echarts.dispatchAction).toHaveBeenCalledWith({ | ||
| type: "highlight", | ||
| seriesIndex: 0, | ||
| dataIndex: 0, | ||
| }); | ||
| expect(mockSelf.echarts.dispatchAction).toHaveBeenCalledWith({ | ||
| type: "highlight", | ||
| seriesIndex: 1, | ||
| dataIndex: 0, | ||
| }); | ||
| expect(mockSelf.echarts.dispatchAction).toHaveBeenCalledWith({ | ||
| type: "showTip", | ||
| seriesIndex: 0, | ||
| dataIndex: 0, | ||
| }); | ||
| expect(mockSelf.echarts.dispatchAction).not.toHaveBeenCalledWith({ | ||
| type: "showTip", | ||
| seriesIndex: 1, | ||
| dataIndex: 0, | ||
| }); | ||
| expect(mockSelf.config.onClickElement).toHaveBeenCalledWith("node", node); | ||
| expect(mockSelf.activeHighlightedElements).toHaveLength(2); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the actual highlightNode/resolveNodeHighlightActions implementation
# to confirm whether connected links are resolved via this.data.links.
fd -i 'netjsongraph.util.js' -x cat -n {}Repository: openwisp/netjsongraph.js
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="test/netjsongraph.util.test.js"
# Show the highlight-related implementation near the relevant methods only.
sed -n '1230,1365p' netjsongraph.util.js
printf '\n--- TEST CONTEXT ---\n'
sed -n '1126,1175p' "$file"Repository: openwisp/netjsongraph.js
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '1230,1365p' netjsongraph.util.jsRepository: openwisp/netjsongraph.js
Length of output: 226
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect only the highlight-specific methods and a compact call graph.
rg -n "^( )?(highlightNode|resolveNodeHighlightActions|resolveLinkHighlightActions|addActiveHighlight|dispatchHighlightAction|clearHighlight)\b|highlightNode\(" netjsongraph.util.js
sed -n '1230,1385p' netjsongraph.util.js | cat -nRepository: openwisp/netjsongraph.js
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect only the highlight-related methods and their call sites.
rg -n "highlightNode|resolveNodeHighlightActions|resolveLinkHighlightActions|addActiveHighlight|dispatchHighlightAction|clearHighlight" netjsongraph.util.js
printf '\n---\n'
sed -n '1230,1385p' netjsongraph.util.jsRepository: openwisp/netjsongraph.js
Length of output: 223
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate the utility source file and inspect only the highlight-related methods.
fd -a -i 'netjsongraph.util.js' .
printf '\n---\n'
fd -a -i 'netjsongraph.util.*' .
printf '\n---\n'
fd -a -i 'util.js' .Repository: openwisp/netjsongraph.js
Length of output: 363
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="/home/jailuser/git/src/js/netjsongraph.util.js"
# Read the highlight-related section only.
sed -n '1230,1385p' "$file" | cat -nRepository: openwisp/netjsongraph.js
Length of output: 6474
highlightNode doesn’t resolve connected links
This test assumes the node highlight also dispatches a link highlight, but resolveNodeHighlightActions() only matches the node itself in graph/scatter/effectScatter series. It never looks at this.data.links, so the seriesIndex: 1 and activeHighlightedElements.length === 2 expectations don’t line up with the current implementation.
🤖 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.util.test.js` around lines 1126 - 1175, highlightNode is
expected to highlight connected links too, but resolveNodeHighlightActions only
matches the node against graph/scatter/effectScatter series and ignores
this.data.links. Update resolveNodeHighlightActions (and any helper it uses) to
also inspect the util’s link data and add highlight actions for links whose
source/target connect to the selected node, so highlightNode can dispatch the
extra link highlight and track both activeHighlightedElements entries.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/js/netjsongraph.util.js (1)
1378-1391: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winGuard
highlightLinkagainst missing link data.
resolveLinkHighlightActions()returns[]for invalid input, buthighlightLink()still clears current highlights and can callonClickElement("link", undefined)whenshowInfois set. Mirror the node path and no-op early.🐛 Proposed fix
highlightLink(linkData, options = {}) { + if (!linkData) { + return; + } const resolveLink = this.utils🤖 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.util.js` around lines 1378 - 1391, highlightLink currently proceeds even when linkData is missing or invalid, unlike the node path. Update highlightLink in netjsongraph.util.js to mirror the node guard by returning early when linkData is absent/invalid so it does not clear existing highlights or call onClickElement("link", undefined); keep the existing resolveLinkHighlightActions and dispatchHighlightActions flow only for valid link data.
🤖 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.
Outside diff comments:
In `@src/js/netjsongraph.util.js`:
- Around line 1378-1391: highlightLink currently proceeds even when linkData is
missing or invalid, unlike the node path. Update highlightLink in
netjsongraph.util.js to mirror the node guard by returning early when linkData
is absent/invalid so it does not clear existing highlights or call
onClickElement("link", undefined); keep the existing resolveLinkHighlightActions
and dispatchHighlightActions flow only for valid link data.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: fb1f45a1-2f5a-40c1-80b9-683c8a137061
📒 Files selected for processing (2)
CHANGELOG.rstsrc/js/netjsongraph.util.js
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: Tests and Coverage (echarts-only)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Avoid unnecessary blank lines inside functions and methods
Write comments only when they explain why code is shaped a certain way, and place comments before the relevant block instead of scattering them inside it
Files:
src/js/netjsongraph.util.js
🧠 Learnings (4)
📚 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-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.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.js
📚 Learning: 2026-05-19T23:25:39.778Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 542
File: src/js/netjsongraph.util.js:1478-1481
Timestamp: 2026-05-19T23:25:39.778Z
Learning: When reviewing netjsongraph label rendering logic in src/js/netjsongraph.util.js and src/js/netjsongraph.render.js, follow the documented contract for showMapLabelsAtZoom: (1) if showMapLabelsAtZoom is false, map labels must be completely disabled; (2) if it is 0, labels must always be shown regardless of zoom (do not treat 0 as disabled); (3) if it is a positive number N, labels must be shown only when zoom >= N. Ensure comparisons implement the intended semantics (i.e., zoom >= 0 is intentionally always true when the setting is 0).
Applied to files:
src/js/netjsongraph.util.js
🔇 Additional comments (2)
CHANGELOG.rst (1)
1-10: LGTM!Also applies to: 25-41, 57-67, 79-79, 89-96
src/js/netjsongraph.util.js (1)
1283-1283: LGTM!Also applies to: 1312-1314, 1369-1373
coderabbitai
Bot
added
enhancement
javascript
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/netjsongraph.util.test.js (1)
1193-1265: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winBlank lines inside newly added test bodies.
highlightLink keeps existing highlight when Ctrl is pressed(lines 1213, 1218) andclearHighlight downplays active elements and clears tooltip(lines 1251, 1253) separate arrange/act/assert sections with blank lines. As per coding guidelines: "Avoid unnecessary blank lines inside functions and methods."🧹 Proposed fix
}; - util.highlightLink.call(mockSelf, firstLink); util.highlightLink.call(mockSelf, secondLink, { event: {event: {ctrlKey: true}}, }); - expect(mockSelf.echarts.dispatchAction).toHaveBeenCalledWith({util.clearHighlight.call(mockSelf); - expect(mockSelf.echarts.dispatchAction).toHaveBeenCalledWith({🤖 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.util.test.js` around lines 1193 - 1265, The newly added test bodies in NetJSONGraphUtil should not contain unnecessary blank lines between arrange/act/assert sections. Update the tests in the highlightLink and clearHighlight cases to keep the statements contiguous, following the project guideline to avoid blank lines inside functions and methods.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/netjsongraph.util.test.js`:
- Around line 1193-1265: The newly added test bodies in NetJSONGraphUtil should
not contain unnecessary blank lines between arrange/act/assert sections. Update
the tests in the highlightLink and clearHighlight cases to keep the statements
contiguous, following the project guideline to avoid blank lines inside
functions and methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f679a41d-1ec6-4c80-a5aa-47393de6cec9
📒 Files selected for processing (3)
test/netjsongraph.render.test.jstest/netjsongraph.spec.jstest/netjsongraph.util.test.js
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Tests and Coverage (dev)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: Tests and Coverage (echarts-only)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,ts,jsx,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,ts,jsx,tsx}: Avoid unnecessary blank lines inside functions and methods
Write comments only when they explain why code is shaped a certain way, and place comments before the relevant block instead of scattering them inside it
Files:
test/netjsongraph.spec.jstest/netjsongraph.render.test.jstest/netjsongraph.util.test.js
🧠 Learnings (1)
📚 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.render.test.jstest/netjsongraph.util.test.js
🔇 Additional comments (5)
test/netjsongraph.render.test.js (3)
1470-1470: Blank lines inside test bodies.Same arrange/act/assert spacing pattern flagged previously for this exact block (lines 1428-1559). As per coding guidelines: "Avoid unnecessary blank lines inside functions and methods."
Also applies to: 1478-1478, 1537-1537, 1548-1548
7-10: LGTM!Also applies to: 311-318, 494-496, 1251-1264, 1712-1714, 1745-1761, 1821-1823, 1842-1846, 1932-1934, 1951-1953, 2142-2147
1490-1536: 🎯 Functional CorrectnessNo
echartsmock needed here.generateMapOptiononly usesJSONData,self.config, andself.utils; it does not readself.echarts, so this test setup is sufficient.> Likely an incorrect or invalid review comment.test/netjsongraph.spec.js (1)
48-59: LGTM!Also applies to: 149-154, 171-173
test/netjsongraph.util.test.js (1)
198-203: LGTM!Also applies to: 715-719, 747-751, 1128-1130, 1139-1192
coderabbitai
Bot
removed
enhancement
javascript
Dhruv-ub
force-pushed
the
feature/163-Node-link-visual-highlighting
branch
from
8d67a63 to
9f871bd
Compare
Dhruv-ub
changed the title
Dhruv-ub
force-pushed
the
feature/163-Node-link-visual-highlighting
branch
2 times, most recently
from
a795f0f to
060c33e
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
In the videos you shared in the PR description, the elements stay highlighted also after the mouse moves out of their area, that doesn't look right.
The element should be highlighted when hovered/focused/clicked, but canceling the operation should bring it back to normal, eg:
- mouse over highlights, mouse out brings back to normal;
- focus highlights, unfocus brings back to normal;
- openeing details of the element highlights, closing brings back to normal.
For unfocusing and closing I mean also focusing or opening a different element.
Can you please clarify the discrepancy?
github-project-automation
Bot
moved this to To do (general)
in OpenWISP Contributor's Board
github-project-automation
Bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
Dhruv-ub
force-pushed
the
feature/163-Node-link-visual-highlighting
branch
from
060c33e to
c3f049a
Compare
Actually, they don't stay highlighted in the video I have kept ctrl pressed that's why they are being highlighted and at last I have left ctrl, so it got unhighlighted. |
Also, the node/edge being highlighted even when leaving their area gets unhighlighted once we press ctrl. |
Dhruv-ub
changed the title
2 participants
Checklist
Reference to Existing Issue
Closes #163.
Description of Changes
Implemented native ECharts-based visual highlighting for NetJSONGraph nodes and links.
Changes include:
highlightNode(nodeData, options)highlightLink(linkData, options)clearHighlight()dispatchActionwithhighlight,downplay,showTip, andhideTipinstead of direct DOM manipulation.focus: "none"where needed.Test Video
Standard Graph
1.mp4
Geographical Graph
2.mp4