[fix] Resolve redundant node labels and overlays #454 #474
Conversation
Test.mp4 |
Dhruv-ub
changed the title
|
Fixed the no-shadow linting error in src/js/netjsongraph.render.js and finalized the parameter rename in src/js/netjsongraph.config.js. Could you please run the checks again? |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The patch contains changes to the formatting which are polluting the actual changes. If formatting needs to be updated let's open a PR to master with only formatting changes.
| clusterSeparation: 20, | ||
| showMetaOnNarrowScreens: false, | ||
| showLabelsAtZoomLevel: 13, | ||
| showMapLabelsAtZoom: 13, |
There was a problem hiding this comment.
This is an unsolicited backward incompatible renaming
There was a problem hiding this comment.
Acknowledged. I am working on the fixes now. I will revert showMapLabelsAtZoom back to showLabelsAtZoomLevel and remove the unrelated formatting changes. I will force push the updates shortly to clean up the commit history.
There was a problem hiding this comment.
I think I was wrong about this comment, we need to read the issue description again more closely. I am sorry for this mistake, I have too many things on my mind right now and I can forget some details, let's follow the issue description and reply to the issues brought up there and use it as a guide to resolve the issue. That's the right way to do it.
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
5648e36 to
a4054d7
Compare
Dhruv-ub
changed the title
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
5 times, most recently
from
c6c106d to
465e1e7
Compare
|
I'm working in a WSL (Ubuntu) environment and have run the Prettier formatters, but the QA checks are still failing locally on my machine. I've tried to resolve the formatting/linting issues, but I'm stuck on why the checks persist in failing. Could you please check the logs and advise what might be missing?" |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I need to insist, please only send the bugfix change, do not send any other change, if QA checks fail, we'll look into it. The PR in the current state contains a lot of other changes unrelated to the bugfix which is time consuming for maintaienrs to review.
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
5 times, most recently
from
a685e69 to
1f239ae
Compare
|
Done please have a look on why ci build failing. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds tooltip.hideDelay = 0 and makes graph/map labels silent by default (label.silent = true). Introduces showMapLabelsAtZoom (backwards-compatible fallback from showLabelsAtZoomLevel, default 13, can be false) and computes currentZoom and showLabel in both graph and map render paths. Zoomend, cluster-click, roam and hover handlers recompute visibility and toggle labels while preserving silent: true when labels are hidden. Tests and README updated to reflect the renamed config and the new zoom/hover label behavior. Sequence Diagram(s)sequenceDiagram
participant User
participant LeafletMap as "LeafletMap\n(rgba(70,130,180,0.5))"
participant ClusterManager as "ClusterManager\n(rgba(34,139,34,0.5))"
participant EChartsGeoLayer as "EChartsGeoLayer\n(rgba(255,165,0,0.5))"
User->>LeafletMap: zoom or click cluster
LeafletMap->>LeafletMap: compute currentZoom
alt cluster clicked
LeafletMap->>ClusterManager: request targetZoom
ClusterManager->>LeafletMap: return targetZoom
LeafletMap->>LeafletMap: setView(targetZoom)
LeafletMap->>LeafletMap: recompute currentZoom
end
LeafletMap->>EChartsGeoLayer: evaluate (currentZoom >= showMapLabelsAtZoom?)
alt threshold met
EChartsGeoLayer->>EChartsGeoLayer: show labels (label.silent = false)
else threshold not met or disabled
EChartsGeoLayer->>EChartsGeoLayer: hide labels (label.silent = true)
end
User->>EChartsGeoLayer: hover node
alt labels hidden by zoom
EChartsGeoLayer->>EChartsGeoLayer: temporarily show label on mouseover (keep silent true when not shown)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Fix all issues with AI Agents 🤖
In @src/js/netjsongraph.render.js:
- Around line 538-546: The code currently only branches on showMapLabelsAtZoom
=== undefined, so passing showMapLabelsAtZoom: false isn't treated as an
explicit "disable labels" intent; update the resolution logic to explicitly
handle showMapLabelsAtZoom === false as the disabled case (e.g., check if
showMapLabelsAtZoom === false and preserve/flag that state), otherwise run the
existing undefined fallback to self.config.showLabelsAtZoomLevel or 13; ensure
subsequent code that validates numeric zoom treats false as the explicit disable
option rather than an invalid type.
- Line 694: The variable NewZoom uses PascalCase; rename it to camelCase
(newZoom) by changing the declaration "const NewZoom = self.leaflet.getZoom();"
to "const newZoom = self.leaflet.getZoom();" and update every usage of NewZoom
in this module (e.g., any comparisons, assignments, or calls that reference
NewZoom) to newZoom so identifiers remain consistent with JavaScript naming
conventions.
🧹 Nitpick comments (1)
src/js/netjsongraph.render.js (1)
572-572: Consider preserving descriptive variable names.The variable renames (
currentZoom→cZoom,showLabel→sLabel) reduce readability without clear benefit. The original names were more descriptive and self-documenting.🔎 Suggested improvement
- const cZoom = self.leaflet.getZoom(); - const sLabel = cZoom >= self.config.showLabelsAtZoomLevel; + const currentZoom = self.leaflet.getZoom(); + const showLabel = typeof showMapLabelsAtZoom === "number" && currentZoom >= showMapLabelsAtZoom; self.echarts.setOption({ series: [ { id: "geo-map", label: { - show: sLabel, + show: showLabel, },Also applies to: 579-579
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/js/netjsongraph.render.js
🧰 Additional context used
🪛 GitHub Actions: netjsongraph.js CI BUILD
src/js/netjsongraph.render.js
[warning] 1-1: Code style issues found. Run Prettier with --write to fix.
[error] 1-1: Prettier check failed. Hint: run 'openwisp-qa-format' or 'Prettier --write' to fix code style issues in this file.
🔇 Additional comments (2)
src/js/netjsongraph.render.js (2)
581-585: Good: Emphasis labels remain hidden to prevent overlay overlap.Setting
emphasis.label.showtofalseunconditionally aligns with the PR objective to prevent label overlap with hover overlays. This ensures that when a user hovers over a node, the overlay tooltip appears without competing with the label.
538-610: Theopenwisp-qa-formatscript does not exist in this repository's package.json. Additionally, the code is already properly formatted—Prettier reports no changes needed. The review comment is based on a non-existent issue.Likely an incorrect or invalid review comment.
src/js/netjsongraph.render.js
Outdated
| // Zoom into the clicked cluster instead of expanding it | ||
| const currentZoom = self.leaflet.getZoom(); | ||
| const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom()); | ||
| const NewZoom = self.leaflet.getZoom(); |
There was a problem hiding this comment.
Use camelCase for the zoom variable.
NewZoom uses PascalCase, which is conventionally reserved for constructors and classes in JavaScript. Use camelCase for consistency with language conventions.
🔎 Proposed fix
- const NewZoom = self.leaflet.getZoom();
- const targetZoom = Math.min(NewZoom + 2, self.leaflet.getMaxZoom());
+ const currentZoom = self.leaflet.getZoom();
+ const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom());Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In @src/js/netjsongraph.render.js around line 694, The variable NewZoom uses
PascalCase; rename it to camelCase (newZoom) by changing the declaration "const
NewZoom = self.leaflet.getZoom();" to "const newZoom = self.leaflet.getZoom();"
and update every usage of NewZoom in this module (e.g., any comparisons,
assignments, or calls that reference NewZoom) to newZoom so identifiers remain
consistent with JavaScript naming conventions.
There was a problem hiding this comment.
why are you renaming this variable? What's wrong with currentZoom?
There was a problem hiding this comment.
@nemesifier You're absolutely right to question this. There's nothing wrong with currentZoom — in fact, it was the correct name. The rename to NewZoom appears to be an unintended change introduced in this PR.
The original currentZoom followed proper JavaScript camelCase conventions and was semantically clear. The rename to NewZoom (PascalCase) actually violates naming conventions, which is why I flagged it.
This change should be reverted back to currentZoom to maintain both naming consistency and the original, more appropriate variable name.
Hey, @Dhruv-ub try running |
@stktyagi Thanks for suggestion but I already ran that command, still I am having issue. |
This worked, you might want to recheck your setup environment. |
Thanks for resolving it, will check it |
|
@Dhruv-ub try: |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I see progress but I also see more quirks:
Placing the mouse on the small label causes it start blinking:
When the mouse leaves, there's still overlap:
Please perform rigorous manual testing of your changes to ensure they're not adding quirks before asking for further reviews.
If we get to something that works, it would be nice to add at least 1 browser test for this.
Can you reply to the questions listed in the issue descirption and how you intend to solve each?
I don't have a clear directive now, but I think the whole situation is a mess, here's the problems I see, I make suggestions at each point but I am open for better ideas:
- I am not sure where the hover overlay is coming from, whether is enabled by default or not
- It seems to me there's not an explicit ways to disable labels on the geographic map (they appears automatically after a certain level of zoom)
- If both are enabled, the overlap covers the label and they're quite redundant, ideally the labels should disappear if there's another overlay showing up on hover and by default only one of the two should be enabled (probably the label as it's not invasive)
- The current config options for controlling this behavior are:
showLabelsAtZoomLevelfor geo map andshowGraphLabelsAtZoom, which is not consistent, I think we should renameshowLabelsAtZoomLeveltoshowMapLabelsAtZoom(in the code we can continue supportingshowLabelsAtZoomLevelfor backward compatibility) and allow this to befalseto disable lables altogether
src/js/netjsongraph.render.js
Outdated
| self.leaflet.on("zoomend", () => { | ||
| const currentZoom = self.leaflet.getZoom(); | ||
| const showLabel = currentZoom >= self.config.showLabelsAtZoomLevel; | ||
| const cZoom = self.leaflet.getZoom(); |
There was a problem hiding this comment.
I wouldn't rename these variables.
|
There was a problem hiding this comment.
Actionable comments posted: 1
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)
578-597: Inconsistent threshold reference in zoomend handler.Line 580 uses
self.config.showMapLabelsAtZoomdirectly, bypassing the fallback logic resolved earlier. If a user configures the legacyshowLabelsAtZoomLevelproperty, this comparison will fail becauseself.config.showMapLabelsAtZoomwould be the default value (13) from the config, not the user's intended value.Consider storing the resolved threshold value for consistent use:
🔧 Suggested approach
+ // Store resolved threshold for consistent use in event handlers + const resolvedMapLabelsThreshold = showMapLabelsAtZoom; + self.leaflet.on("zoomend", () => { currentZoom = self.leaflet.getZoom(); - showLabel = currentZoom >= self.config.showMapLabelsAtZoom; + showLabel = currentZoom >= resolvedMapLabelsThreshold;
🤖 Fix all issues with AI agents
In @src/js/netjsongraph.render.js:
- Around line 545-553: The fallback wrongly checks
self.config.showMapLabelsAtZoom twice; change the logic in the
showMapLabelsAtZoom resolution so that when showMapLabelsAtZoom is undefined it
next checks the legacy property self.config.showLabelsAtZoomLevel (not
self.config.showMapLabelsAtZoom) and only if that is also undefined set the
default 13; update the block around the showMapLabelsAtZoom variable assignment
to reference self.config.showLabelsAtZoomLevel for backward compatibility.
🧹 Nitpick comments (2)
src/js/netjsongraph.render.js (2)
174-176: Good fix for label-tooltip interaction.Setting
silent: truemakes labels non-interactive, giving tooltips interaction priority as intended. Consider making the comment slightly more descriptive.📝 Optional: More descriptive comment
- // Added this for label hover issue - baseGraphLabel.silent = true; + // Make labels non-interactive to prevent label-tooltip conflicts on hover + baseGraphLabel.silent = true;
702-707: Consider clearer variable naming.
NewZoomis misleading since it holds the current zoom level, not the new/target zoom. The namecurrentZoomorclickZoomwould be more accurate, withtargetZoomfor the computed destination.📝 Optional: Clearer naming
- const NewZoom = self.leaflet.getZoom(); - const targetZoom = Math.min(NewZoom + 2, self.leaflet.getMaxZoom()); + const currentZoom = self.leaflet.getZoom(); + const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/netjsongraph.render.js (1)
src/js/netjsongraph.clients.js (1)
currentZoom(21-21)
🔇 Additional comments (4)
src/js/netjsongraph.config.js (1)
44-45: LGTM on the config rename.The rename from
showLabelsAtZoomLeveltoshowMapLabelsAtZoomaligns naming withshowGraphLabelsAtZoomand improves consistency. The backward compatibility handling for the old property name should be verified in the render file.src/js/netjsongraph.render.js (3)
48-51: LGTM on tooltip configuration.Adding
hideDelay: 0ensures tooltips disappear immediately, preventing overlap with labels. Combined withconfine: true, this creates predictable tooltip behavior as intended by the PR.
339-342: LGTM on map label configuration.The spread pattern properly preserves existing label config while adding
silent: trueto prevent hover conflicts, consistent with the graph label handling.
559-576: LGTM on initial label visibility handling.Correctly hides labels and emphasis labels when zoom is below threshold, with
silent: trueto prevent interaction conflicts.
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
94bc1c2 to
2a630fc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
578-596: Inconsistent handling ofshowMapLabelsAtZoomin zoomend handler.The initial visibility logic (line 557) checks
typeof showMapLabelsAtZoom === "number"before comparing, but the zoomend handler (line 580) directly compares without this check. If the user setsshowMapLabelsAtZoom: falseto disable labels entirely, the comparisoncurrentZoom >= falsewill produce incorrect results.🐛 Proposed fix
self.leaflet.on("zoomend", () => { currentZoom = self.leaflet.getZoom(); - showLabel = currentZoom >= self.config.showMapLabelsAtZoom; + showLabel = + typeof showMapLabelsAtZoom === "number" && + currentZoom >= showMapLabelsAtZoom; self.echarts.setOption({ series: [ {Note: Use the local
showMapLabelsAtZoomvariable (which captures the resolved value including legacy fallback) instead ofself.config.showMapLabelsAtZoom.
🤖 Fix all issues with AI agents
In @src/js/netjsongraph.render.js:
- Around line 545-553: The current logic for resolving showMapLabelsAtZoom
incorrectly re-checks self.config.showMapLabelsAtZoom; change the fallback to
look for the legacy key self.config.showLabelsAtZoomLevel instead and assign it
to the local variable showMapLabelsAtZoom if present, otherwise default to 13;
update the block that references showMapLabelsAtZoom and
self.config.showMapLabelsAtZoom to instead check
self.config.showLabelsAtZoomLevel as the backward-compatible fallback.
🧹 Nitpick comments (1)
src/js/netjsongraph.render.js (1)
702-703: Variable naming doesn't follow camelCase convention and is semantically misleading.
NewZoomshould use camelCase (newZoom) per JavaScript conventions. Additionally, the name is misleading since it captures the current zoom level, not a "new" one.♻️ Suggested fix
- const NewZoom = self.leaflet.getZoom(); - const targetZoom = Math.min(NewZoom + 2, self.leaflet.getMaxZoom()); + const currentZoom = self.leaflet.getZoom(); + const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jstest/netjsongraph.render.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- src/js/netjsongraph.config.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/netjsongraph.render.js (2)
test/netjsongraph.calc.test.js (1)
configs(3-32)src/js/netjsongraph.clients.js (1)
currentZoom(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (4)
src/js/netjsongraph.render.js (3)
48-50: LGTM!Adding
hideDelay: 0ensures tooltips disappear immediately when the mouse leaves, making behavior predictable and giving tooltips interaction priority as intended.
174-176: LGTM!Setting
silent: trueon graph labels ensures they don't interfere with tooltip interactions, addressing the label-tooltip conflict issue.
339-342: LGTM!Spreading the existing label config while adding
silent: truecorrectly applies non-interactive behavior to map labels, consistent with the graph label treatment.test/netjsongraph.render.test.js (1)
1255-1255: LGTM!The updated expectation correctly reflects the additional
setOptioncall introduced by the new label visibility logic during map rendering.
Demo.Test.mp4Here is a demo of the fix in action. The labels now handle zooming correctly and do not overlap with tooltips. |
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
2a630fc to
655e48d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
578-597: Inconsistent threshold usage in zoomend handler.Line 580 uses
self.config.showMapLabelsAtZoomdirectly, while the initial logic (lines 545-553) computes a fallback to13when undefined. If the config option is not set,currentZoom >= undefinedevaluates tofalse, causing labels to never show on zoom regardless of zoom level.The handler should use the resolved threshold value consistently.
Proposed fix
Store the resolved threshold on
selffor consistent access:// 4. Resolve label visibility threshold let {showMapLabelsAtZoom} = self.config; if (showMapLabelsAtZoom === undefined) { - if (self.config.showMapLabelsAtZoom !== undefined) { - showMapLabelsAtZoom = self.config.showMapLabelsAtZoom; + if (self.config.showLabelsAtZoomLevel !== undefined) { + showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; } else { showMapLabelsAtZoom = 13; } } + // Store resolved threshold for event handlers + self._mapLabelZoomThreshold = showMapLabelsAtZoom;Then in the zoomend handler:
self.leaflet.on("zoomend", () => { currentZoom = self.leaflet.getZoom(); - showLabel = currentZoom >= self.config.showMapLabelsAtZoom; + showLabel = typeof self._mapLabelZoomThreshold === "number" + && currentZoom >= self._mapLabelZoomThreshold; self.echarts.setOption({
🤖 Fix all issues with AI agents
In @src/js/netjsongraph.render.js:
- Around line 545-553: The fallback logic around showMapLabelsAtZoom is
redundant and never reaches the inner self.config check; update the block that
initializes showMapLabelsAtZoom to first use self.config.showMapLabelsAtZoom (as
you already destructured), and if that is undefined fall back to the legacy
option self.config.showLabelsAtZoomLevel before finally defaulting to 13. Remove
the unreachable inner if and ensure the code references the destructured
showMapLabelsAtZoom only for the initial value, then assign from
self.config.showLabelsAtZoomLevel when needed, with 13 as the final default.
🧹 Nitpick comments (1)
src/js/netjsongraph.render.js (1)
702-703: Use camelCase for variable naming.
NewZoomshould benewZoomto follow JavaScript naming conventions. PascalCase is typically reserved for classes and constructors.Proposed fix
- const NewZoom = self.leaflet.getZoom(); - const targetZoom = Math.min(NewZoom + 2, self.leaflet.getMaxZoom()); + const newZoom = self.leaflet.getZoom(); + const targetZoom = Math.min(newZoom + 2, self.leaflet.getMaxZoom());
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jstest/netjsongraph.render.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/netjsongraph.config.js
- test/netjsongraph.render.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/netjsongraph.render.js (2)
test/netjsongraph.calc.test.js (1)
configs(3-32)src/js/netjsongraph.clients.js (1)
currentZoom(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (3)
src/js/netjsongraph.render.js (3)
48-51: LGTM!Adding
hideDelay: 0makes tooltip dismissal immediate and explicit, which aligns with the PR objective of predictable tooltip behavior.
174-176: LGTM!Setting
silent: trueon graph labels prevents them from capturing mouse events, ensuring tooltips retain interaction priority as intended.
339-342: LGTM!The spread pattern correctly preserves user-defined label configuration while ensuring labels remain non-interactive.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Can you add a browser test for this behavior?
src/js/netjsongraph.render.js
Outdated
| // Zoom into the clicked cluster instead of expanding it | ||
| const currentZoom = self.leaflet.getZoom(); | ||
| const targetZoom = Math.min(currentZoom + 2, self.leaflet.getMaxZoom()); | ||
| const NewZoom = self.leaflet.getZoom(); |
There was a problem hiding this comment.
why are you renaming this variable? What's wrong with currentZoom?
This looks a lot better, thanks 👍 |
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
655e48d to
b19a10d
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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)
578-596: Inconsistent fallback logic in zoomend handler.Line 580 uses
self.config.showMapLabelsAtZoomdirectly instead of the localshowMapLabelsAtZoomvariable that was intended to handle backward compatibility fallback. This means:
- Initial render: applies fallback to
showLabelsAtZoomLevel(if the fix above is applied)- On zoom: ignores the fallback and reads
showMapLabelsAtZoomdirectlyConsider storing the resolved threshold value on
selffor consistent access, or extract the fallback logic into a helper.🔧 Suggested approach: store resolved value
// 4. Resolve label visibility threshold let {showMapLabelsAtZoom} = self.config; if (showMapLabelsAtZoom === undefined) { if (self.config.showLabelsAtZoomLevel !== undefined) { showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; } else { showMapLabelsAtZoom = 13; } } + // Store resolved threshold for consistent access in event handlers + self._resolvedMapLabelsAtZoom = showMapLabelsAtZoom; // ... later in zoomend handler: self.leaflet.on("zoomend", () => { currentZoom = self.leaflet.getZoom(); - showLabel = currentZoom >= self.config.showMapLabelsAtZoom; + showLabel = currentZoom >= self._resolvedMapLabelsAtZoom;
🤖 Fix all issues with AI agents
In @src/js/netjsongraph.render.js:
- Around line 545-553: The code extracts let {showMapLabelsAtZoom} = self.config
then redundantly re-checks self.config.showMapLabelsAtZoom inside the undefined
branch, which is always false and prevents falling back to the legacy option;
update the undefined branch in the showMapLabelsAtZoom resolution logic to check
for the legacy config key self.config.showLabelsAtZoomLevel and assign that
value if present, otherwise set the default 13 (i.e., when showMapLabelsAtZoom
is undefined, use self.config.showLabelsAtZoomLevel if defined, else 13), and
remove the dead inner check of self.config.showMapLabelsAtZoom.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jstest/netjsongraph.render.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- test/netjsongraph.render.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/netjsongraph.render.js (1)
src/js/netjsongraph.clients.js (1)
currentZoom(21-21)
🔇 Additional comments (5)
src/js/netjsongraph.config.js (1)
44-44: LGTM!The configuration property rename from
showLabelsAtZoomLeveltoshowMapLabelsAtZoomaligns with the naming convention (showGraphLabelsAtZoom) and the default value of 13 is preserved.src/js/netjsongraph.render.js (4)
48-51: LGTM!Adding
hideDelay: 0ensures tooltips disappear immediately when the cursor leaves, which addresses the reported blinking and overlap behavior.
174-176: LGTM!Setting
silent: trueon graph labels prevents them from intercepting hover events, ensuring tooltips remain the primary interactive overlay as intended.
339-342: LGTM!The label configuration correctly spreads the existing config and adds
silent: true, maintaining consistency with the graph label approach.
702-707: LGTM!The cluster click handler correctly updates
currentZoombefore calculating the target zoom level for the cluster expansion.
Sure, I will add the browser test shortly. Regarding the variable rename: I originally changed it because the linter flagged a redeclaration error with currentZoom. I have resolved this now. |
- Fixed hover overlay origin to be confined to viewport. - Added logic to disable map labels completely if configured. - Resolved overlap between labels and hover overlays (labels hide on hover). - Ensured labels remain non-invasive while tooltips take priority. - Renamed configuration options for consistency. - Added regression tests for label visibility and hover interaction.
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
b19a10d to
3da29d0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
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)
611-630: Use resolved threshold in zoomend handler.Line 613 directly references
self.config.showMapLabelsAtZoom, bypassing the threshold resolution logic at lines 545-553. If a user configured the legacyshowLabelsAtZoomLevelproperty, this comparison would useundefined, resulting in incorrect label visibility.🐛 Proposed fix
Store the resolved threshold where
zoomendcan access it, or re-resolve it consistently:- showLabel = currentZoom >= self.config.showMapLabelsAtZoom; + showLabel = typeof showMapLabelsAtZoom === "number" && currentZoom >= showMapLabelsAtZoom;Note: The
showMapLabelsAtZoomvariable from line 546 is already in scope for this handler due to closure, so this should work directly.
♻️ Duplicate comments (1)
src/js/netjsongraph.render.js (1)
545-553: Backward compatibility fallback is broken — redundant condition creates dead code.The inner
ifon line 548 checksself.config.showMapLabelsAtZoom, but this is the same property destructured on line 546. When line 547's condition is true (value is undefined), line 548's condition will always be false, making lines 548-549 unreachable.Per PR objectives to "preserve backward compatibility with
showLabelsAtZoomLevel", this should fall back to the legacy property name:🐛 Proposed fix for backward compatibility
// 4. Resolve label visibility threshold let {showMapLabelsAtZoom} = self.config; if (showMapLabelsAtZoom === undefined) { - if (self.config.showMapLabelsAtZoom !== undefined) { - showMapLabelsAtZoom = self.config.showMapLabelsAtZoom; + if (self.config.showLabelsAtZoomLevel !== undefined) { + showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; } else { showMapLabelsAtZoom = 13; } }
🧹 Nitpick comments (1)
test/netjsongraph.render.test.js (1)
1529-1537: Test validates mock return value rather than actual implementation.This test verifies that
generateMapOptionreturnslabel.silent: true, but sincegenerateMapOptionis mocked (line 1518), it's testing the mock setup rather than the actual implementation innetjsongraph.render.js. The actual implementation at lines 339-342 does setsilent: true, so the behavior is correct, but this test doesn't exercise that code path.Consider either:
- Using the real
generateMapOptionmethod instead of mocking it, or- Renaming this test to clarify it's testing the integration/wiring rather than the option generation logic.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/js/netjsongraph.config.jssrc/js/netjsongraph.render.jstest/netjsongraph.render.test.js
🧰 Additional context used
🧬 Code graph analysis (1)
src/js/netjsongraph.render.js (3)
test/netjsongraph.calc.test.js (1)
configs(3-32)test/netjsongraph.render.test.js (1)
self(484-484)src/js/netjsongraph.clients.js (1)
currentZoom(21-21)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (11)
src/js/netjsongraph.config.js (1)
44-44: LGTM — Configuration rename aligns with PR objectives.The rename from
showLabelsAtZoomLeveltoshowMapLabelsAtZoomimproves naming consistency between graph and map label zoom settings. Backward compatibility is handled via fallback logic innetjsongraph.render.js.src/js/netjsongraph.render.js (6)
50-50: LGTM — Tooltip stabilization.Setting
hideDelay: 0ensures tooltips disappear immediately when the mouse leaves, preventing lingering overlays that could conflict with labels.
174-176: LGTM — Graph labels marked as silent.Setting
silent: trueon graph labels prevents them from interfering with hover interactions, allowing tooltips to take priority as the primary interactive overlay.
339-342: LGTM — Map labels marked as silent.The spread operator correctly merges existing label configuration while adding
silent: true, ensuring map labels don't capture hover events.
555-576: LGTM — Initial label visibility logic.The initial label visibility is correctly computed using the local
showMapLabelsAtZoomvariable (which includes fallback handling once the above issue is fixed), and appropriately hides labels and emphasis when below the zoom threshold.
578-609: LGTM — Hover handlers prevent label/tooltip overlap.The mouseover/mouseout handlers correctly hide labels when tooltips appear and restore them when tooltips disappear, but only when labels are currently visible (
showLabelis true). This prevents the redundancy issue described in the PR objectives.
735-740: LGTM — Cluster click zoom logic.The cluster click handler correctly uses
currentZoomto calculate the target zoom level, maintaining consistent variable naming.test/netjsongraph.render.test.js (4)
968-968: LGTM — Mock updated for echarts event handling.Adding
echarts.on: jest.fn()allows tests to verify that event handlers are properly registered.
1065-1065: LGTM — Tests use renamed configuration property.Tests correctly reference
showMapLabelsAtZoommatching the renamed configuration.
1253-1263: LGTM — Updated test expectations for setOption calls.The test correctly expects 2
setOptioncalls: one for initial render and one aftermoveendtriggers a data update.
1556-1588: LGTM — Comprehensive hover interaction test.This test properly verifies that:
- Labels hide when mouse hovers over a node (tooltip appears)
- Labels restore when mouse leaves (tooltip disappears)
This directly validates the fix for the redundant label/tooltip overlap issue.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
changed the title
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Good progress @Dhruv-ub I like the result! 👍
Here's a few problems found during my review and testing:
- I tried setting
showMapLabelsAtZoomto false in one of the examples but I don't see any difference - the README needs to be updated
- it doesn't look like we are maintaining backward compatibility with the old config option
- 1 test in the CI build is failing
I think with the issues above resolved we can merge.
|
|
||
| expect(mouseOverCall).toBeDefined(); | ||
| expect(mouseOutCall).toBeDefined(); | ||
|
|
| const mouseOutCall = mockSelf.echarts.on.mock.calls.find( | ||
| (c) => c[0] === "mouseout", | ||
| ); | ||
|
|
|
|
||
| // 3. Simulate Mouse Over (Tooltip appears) -> Labels should HIDE | ||
| onHover(); | ||
|
|
|
|
||
| // 4. Simulate Mouse Out (Tooltip gone) -> Labels should SHOW | ||
| onUnhover(); | ||
|
|
|
|
||
| test("zoomend keeps labels silent when zoom remains above threshold", () => { | ||
| renderInstance.mapRender(mockSelf.data, mockSelf); | ||
|
|
There was a problem hiding this comment.
please avoid adding too many blank lines as they don't add any benefit
| }); | ||
| }); | ||
|
|
||
| describe("mapRender label and tooltip interaction (emphasis behavior)", () => { |
There was a problem hiding this comment.
+1 for grouping these tests together!
| clusterSeparation: 20, | ||
| showMetaOnNarrowScreens: false, | ||
| showLabelsAtZoomLevel: 13, | ||
| showMapLabelsAtZoom: 13, |
There was a problem hiding this comment.
From #454:
(in the code we can continue supporting showLabelsAtZoomLevel for backward compatibility)
Is this being honored and if yes how?
| const showCall = mockSelf.echarts.setOption.mock.calls.at(-1)[0]; | ||
| const shownSeries = showCall.series.find((s) => s.id === "geo-map"); | ||
| expect(shownSeries.label.show).toBe(true); | ||
| }); |
There was a problem hiding this comment.
can you add a test for the case in which labels are disabled alltogether?
- Fixed logic so 'showMapLabelsAtZoom: false' correctly disables labels. - Restored backward compatibility for 'showLabelsAtZoomLevel'. - Updated README documentation. - Fixed CI test failure by updating mocks with 'fastDeepCopy'. - Added regression test for disabled labels.
There was a problem hiding this comment.
Actionable comments posted: 0
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)
337-356: Backward compatibility gap:generateMapOptionwon't detect legacyshowLabelsAtZoomLevelconfig.The
shouldDisableLabelscheck on line 339 readsconfigs.showMapLabelsAtZoomdirectly, but the backward compatibility fallback for the legacyshowLabelsAtZoomLevelis only resolved locally withinmapRenderat lines 618-626. This creates two problems:
- The initial
generateMapOptioncall at line 554 happens before the backward compat resolution (line 618), so it will read an undefined value if the user only provided the legacyshowLabelsAtZoomLevel.- Even after backward compat resolution, the fallback is stored in a local variable at line 618 and is never written back to
self.config, so subsequentgenerateMapOptioncalls from event handlers also won't benefit from it.Users who rely on the legacy
showLabelsAtZoomLevelconfig will find that label disabling doesn't work as expected.Consider either resolving backward compatibility once at the start of
mapRenderand updatingself.configaccordingly, or moving the resolution logic directly intogenerateMapOption.
🧹 Nitpick comments (2)
src/js/netjsongraph.render.js (1)
657-688: Consider debouncing or guarding against rapid toggles.The
mouseover/mouseouthandlers callsetOptionon every event. While ECharts handles this reasonably well, rapid mouse movements could trigger many consecutive updates.This is a minor consideration—if manual testing shows acceptable performance, no changes are needed.
test/netjsongraph.render.test.js (1)
1494-1704: Make the “silent labels” assertion validate the applied option (not just the mock).
Right now the test inspects the mocked return value, which can be self‑fulfilling. Consider asserting on the lastsetOptionpayload instead.♻️ Suggested tweak
- const option = mockSelf.utils.generateMapOption.mock.results[0].value; - const series = option.series.find((s) => s.id === "geo-map"); + const lastCall = mockSelf.echarts.setOption.mock.calls.at(-1)[0]; + const series = lastCall.series.find((s) => s.id === "geo-map");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.nvmrcREADME.mdsrc/js/netjsongraph.render.jstest/netjsongraph.render.test.js
✅ Files skipped from review due to trivial changes (1)
- .nvmrc
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🧰 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.render.test.jssrc/js/netjsongraph.render.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.render.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.js
🧠 Learnings (6)
📚 Learning: 2026-01-20T16:51:06.213Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:51:06.213Z
Learning: In the netjsongraph.js project, imports from `echarts/lib/chart/*/install`, `echarts/lib/component/*/install`, and `echarts/lib/renderer/install*` paths produce better tree-shaking results than the public entry points (`echarts/charts`, `echarts/components`, `echarts/renderers`) in their webpack configuration. The project maintainer has tested both approaches and confirmed the `/lib/*/install` pattern yields smaller bundles.
Applied to files:
test/netjsongraph.render.test.js
📚 Learning: 2026-01-22T22:37:11.271Z
Learnt from: CR
Repo: openwisp/netjsongraph.js PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-22T22:37:11.271Z
Learning: Applies to test/netjsongraph.browser.test.js : Write browser tests in test/netjsongraph.browser.test.js using Chrome/ChromeDriver and run with `yarn test test/netjsongraph.browser.test.js`
Applied to files:
test/netjsongraph.render.test.js
📚 Learning: 2026-01-27T19:09:23.929Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 473
File: src/js/netjsongraph.render.js:433-468
Timestamp: 2026-01-27T19:09:23.929Z
Learning: In the netjsongraph.js project, when implementing wheel zoom propagation from the graph container to the canvas: ECharts intercepts wheel events directly on the canvas with preventDefault(), so those events don't bubble to the container. A wheel listener on the container only catches events that occur outside the canvas (in the empty container space), making checks for e.target === canvas unnecessary since canvas events never reach the container listener.
Applied to files:
test/netjsongraph.render.test.jssrc/js/netjsongraph.render.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:
test/netjsongraph.render.test.jssrc/js/netjsongraph.render.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.js
📚 Learning: 2026-01-24T23:09:24.928Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 444
File: src/js/netjsongraph.util.js:1404-1442
Timestamp: 2026-01-24T23:09:24.928Z
Learning: In the `moveNodeInRealTime` function in `src/js/netjsongraph.util.js`, it is recommended that `node.properties` be set, and the function should return early when `properties` is not provided rather than initializing it or handling it gracefully.
Applied to files:
src/js/netjsongraph.render.js
🔇 Additional comments (10)
src/js/netjsongraph.render.js (5)
58-60: LGTM!Setting
hideDelay: 0ensures tooltips hide immediately, which aligns with the PR objective to make tooltip behavior explicit and predictable.
183-185: LGTM!Setting
silent: trueon graph labels prevents them from capturing hover events, ensuring tooltips remain the primary interactive overlay as intended.
617-655: LGTM!The backward compatibility resolution correctly falls back from
showMapLabelsAtZoomtoshowLabelsAtZoomLeveland defaults to 13. The initial label visibility logic properly handles the disabled state and appliessilent: true.
690-733: LGTM!The zoomend handler correctly updates
showLabelbased on the new zoom level and consistently appliessilent: true. The zoom control button state management is also properly implemented.
817-822: LGTM!Using the shared
currentZoomvariable maintains consistency with the rest of the code. Thezoomendhandler will properly recalculate visibility after the zoom animation completes.test/netjsongraph.render.test.js (5)
968-972: Mockedecharts.onhookup looks good.
Helps align the test double with new hover/zoom handlers.
1070-1076: Good update to includeshowMapLabelsAtZoom+echarts.onin the mock.
Keeps the zoom-control tests consistent with the new label logic.
1213-1229: Nice wiring ofshowMapLabelsAtZoomandechartsSetOptiondelegation.
This makes the polygon/moveend test expectations much clearer.
1264-1274: Clearer setOption expectations after moveend.
The explicit “1 initial + 1 update” check makes the test intent obvious.
1462-1469: LGTM for the map label zoom toggle setup.
Good to seeshowMapLabelsAtZoomreflected in the zoomend test scaffold.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| } | ||
|
|
||
| if (self.leaflet.getZoom() < self.config.showLabelsAtZoomLevel) { | ||
| // 4. Resolve label visibility threshold |
There was a problem hiding this comment.
This entire block of changes is very unreadable and seems too unreadable for such a simple fix to labels.
It's a ball of spaghetti code. I don't feel comfortable in merging.
Why do we need two different variables to hide labels: showLabel and labelsDisabled? Whether it's disabled or not zoomed enough, why not just use one var? There's far too many if/else statements.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I took a closer look and I have a number of doubts. See below.
| if (self.config.showLabelsAtZoomLevel !== undefined) { | ||
| showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; | ||
| } else { | ||
| showMapLabelsAtZoom = 13; |
There was a problem hiding this comment.
why? isn't the default config definition in netjsongraph.config.js enough?
|
|
||
| // Check if labels should be disabled completely | ||
| const shouldDisableLabels = | ||
| configs.showMapLabelsAtZoom === false || configs.showMapLabelsAtZoom === null; |
There was a problem hiding this comment.
I don't think we really need this.. just check if it's falsy
| const baseGraphSeries = {...configs.graphConfig.series}; | ||
| const baseGraphLabel = {...(baseGraphSeries.label || {})}; | ||
|
|
||
| // Added this for label hover issue |
There was a problem hiding this comment.
This comment is too vague... what issue? We have lots of issues with labels. What does this solve exactly?
| let {showMapLabelsAtZoom} = self.config; | ||
| // Backward Compatibility: Check old name if new one is missing | ||
| if (showMapLabelsAtZoom === undefined) { | ||
| if (self.config.showLabelsAtZoomLevel !== undefined) { |
There was a problem hiding this comment.
I don't think we should do this during rendering. Can't we do this somewhere earlier during initialization? For example in the construtctor of NetJSONGraphCore, when we initialize the config object, we can check for the the presence of showLabelsAtZoomLevel and assign it to showMapLabelsAtZoom if showMapLabelsAtZoom is not defined.
| typeof showMapLabelsAtZoom === "number" && | ||
| currentZoom >= showMapLabelsAtZoom; | ||
|
|
||
| if (labelsDisabled || !showLabel) { |
There was a problem hiding this comment.
I think we should strive to end up with something similar as we had before, eg:
const {showMapLabelsAtZoom} = self.config;
if (showMapLabelsAtZoom && self.leaflet.getZoom() < showMapLabelsAtZoom) {The rest of the code is far too complex with no benefits. The handling of backward compatibility should be done elsewhere, not during the map rendering, it feels wrong to me, it's too late here.
| // When a user hovers over a node, we hide the static label so the Tooltip | ||
| self.echarts.on("mouseover", () => { | ||
| if (!labelsDisabled && showLabel) { | ||
| self.echarts.setOption({ |
There was a problem hiding this comment.
Wouldn't this have to change based on the current zoom level? Shouldn't the zoom level be evaluated in this function? What if the mouseover event happens after map rendering and the user has zoomed in or out?
|
|
||
| self.echarts.on("mouseout", () => { | ||
| if (!labelsDisabled && showLabel) { | ||
| self.echarts.setOption({ |
3 participants
To improve the interaction between map labels and hover overlays to avoid visual clutter and interaction conflicts, especially at higher zoom levels.
The hover overlay is confirmed to be the default ECharts tooltip. Its behavior is made more predictable, while label rendering is adjusted so labels no longer interfere with hover interactions.
Fixes #454
Checklist
Reference to Existing Issue
Closes #454.
Description of Changes
Screenshot