[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.
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds tooltip.hideDelay = 0. Sets baseGraphLabel.silent = true and ensures geo/map series labels are created by merging existing label config with silent: true. Introduces showMapLabelsAtZoom (default 13, falls back from legacy showLabelsAtZoomLevel) and computes currentZoom and showLabel in both graph and map render paths (including zoomend and cluster-click). When showLabel is false, labels and emphasis are hidden by setting label.silent = true. Cluster-click and zoom handlers reuse currentZoom to compute targetZoom and propagate label.silent updates. Tests updated to reflect additional echarts.setOption calls and label/tooltip emphasis behavior; config default property renamed. Sequence Diagram(s)sequenceDiagram
participant User
participant LeafletMap
participant ClusterManager
participant EChartsGeoLayer
User->>LeafletMap: zoom or click cluster
LeafletMap->>LeafletMap: compute currentZoom
alt cluster clicked
LeafletMap->>ClusterManager: request target zoom for cluster
ClusterManager->>LeafletMap: respond with targetZoom
LeafletMap->>LeafletMap: setView(targetZoom)
LeafletMap->>LeafletMap: recompute currentZoom
end
LeafletMap->>EChartsGeoLayer: evaluate (currentZoom >= showMapLabelsAtZoom?)
alt threshold met
EChartsGeoLayer->>EChartsGeoLayer: show labels & emphasis (label.silent = false)
else threshold not met
EChartsGeoLayer->>EChartsGeoLayer: hide labels & emphasis (label.silent = true)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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.
| // 4. Resolve label visibility threshold | ||
| let {showMapLabelsAtZoom} = self.config; | ||
| if (showMapLabelsAtZoom === undefined) { | ||
| if (self.config.showMapLabelsAtZoom !== undefined) { | ||
| showMapLabelsAtZoom = self.config.showMapLabelsAtZoom; | ||
| } else { | ||
| showMapLabelsAtZoom = 13; | ||
| } | ||
| } |
There was a problem hiding this comment.
Redundant condition: backward compatibility fallback is broken.
The inner if on line 548 checks self.config.showMapLabelsAtZoom !== undefined, but this is the same property just extracted on line 546. When line 547's condition is true (the value is undefined), line 548's condition will always be false, making lines 548-549 dead code.
Based on the PR objectives to "preserve backward compatibility with the existing showLabelsAtZoomLevel configuration", 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;
}
}📝 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.
| // 4. Resolve label visibility threshold | |
| let {showMapLabelsAtZoom} = self.config; | |
| if (showMapLabelsAtZoom === undefined) { | |
| if (self.config.showMapLabelsAtZoom !== undefined) { | |
| showMapLabelsAtZoom = self.config.showMapLabelsAtZoom; | |
| } else { | |
| showMapLabelsAtZoom = 13; | |
| } | |
| } | |
| // 4. Resolve label visibility threshold | |
| let {showMapLabelsAtZoom} = self.config; | |
| if (showMapLabelsAtZoom === undefined) { | |
| if (self.config.showLabelsAtZoomLevel !== undefined) { | |
| showMapLabelsAtZoom = self.config.showLabelsAtZoomLevel; | |
| } else { | |
| showMapLabelsAtZoom = 13; | |
| } | |
| } |
🤖 Prompt for AI Agents
In @src/js/netjsongraph.render.js around lines 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.
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?
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