[fix] Resolve redundant node labels and overlays #454#474
[fix] Resolve redundant node labels and overlays #454#474Dhruv-ub wants to merge 12 commits intoopenwisp:masterfrom
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.
|
|
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 change addresses label and overlay redundancy in geographic maps by renaming the configuration option Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error)
✅ Passed checks (4 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 Tip CodeRabbit can generate a title for your PR based on the changes with custom instructions.Set the |
![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.
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: |
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
fb6a175 to
ebf25bf
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/js/netjsongraph.render.js`:
- Line 928: The export statement exports undefined module-scope symbols
`echarts` and `L`; remove them and export only the defined module symbol
`NetJSONGraphRender`. Edit the export line that currently reads `export
{NetJSONGraphRender, echarts, L};` to export just `NetJSONGraphRender`, and
ensure there are no remaining top-level references expecting `echarts` or `L`
(note `echarts` is not instantiated from the imported `{use}` and `L` is local
to `mapRender()`); this fixes the webpack build by eliminating undefined
exports.
In `@test/netjsongraph.browser.test.js`:
- Around line 364-390: The test is passing coincidentally because mapRender
captures const {showMapLabelsAtZoom} from self.config at render time so changing
window.map.config.showMapLabelsAtZoom after render doesn't affect the zoomend
handler; update the test to either (A) change
window.map.config.showMapLabelsAtZoom before re-rendering the map (call the
module's render method or trigger the same code path that runs mapRender) so the
destructured value in mapRender reflects the new threshold, or (B) keep the
existing flow but choose discriminating boundary values (e.g., set
window.map.config.showMapLabelsAtZoom to 12 and assert that zoom 12 shows while
zoom 13 hides, then repeat with threshold 13) so the test actually verifies the
override; locate mapRender and the zoomend handler and adjust the test to
re-render or use boundary zooms accordingly.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.nvmrcREADME.mdsrc/js/netjsongraph.config.jssrc/js/netjsongraph.jssrc/js/netjsongraph.render.jstest/netjsongraph.browser.test.jstest/netjsongraph.render.test.js
🚧 Files skipped from review as they are similar to previous changes (3)
- src/js/netjsongraph.config.js
- .nvmrc
- src/js/netjsongraph.js
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
test/netjsongraph.render.test.jssrc/js/netjsongraph.render.jstest/netjsongraph.browser.test.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.render.test.jstest/netjsongraph.browser.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.render.js
test/netjsongraph.browser.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Write browser tests in test/netjsongraph.browser.test.js using Chrome/ChromeDriver and run with
yarn test test/netjsongraph.browser.test.js
Files:
test/netjsongraph.browser.test.js
🧠 Learnings (11)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 0
File: :0-0
Timestamp: 2026-02-06T16:33:48.468Z
Learning: In the openwisp/netjsongraph.js project, prioritize pragmatic reviews that focus on critical matters with real user impact (functionality, performance, security, maintainability) over minor code quality details that require significant effort for no noticeable benefit. Avoid perfectionist suggestions when tests pass and functionality is correct.
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:21.783Z
Learning: Project preference: treat showMapLabelsAtZoom = 0 as “disabled” (not a valid threshold) and keep falsy checks in src/js/netjsongraph.render.js; document or normalize this if needed.
📚 Learning: 2026-01-30T08:31:21.783Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:21.783Z
Learning: Project preference: treat showMapLabelsAtZoom = 0 as “disabled” (not a valid threshold) and keep falsy checks in src/js/netjsongraph.render.js; document or normalize this if needed.
Applied to files:
test/netjsongraph.render.test.jstest/netjsongraph.browser.test.jsREADME.md
📚 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.jstest/netjsongraph.browser.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.jstest/netjsongraph.browser.test.js
📚 Learning: 2026-02-06T16:33:48.468Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 0
File: :0-0
Timestamp: 2026-02-06T16:33:48.468Z
Learning: In the openwisp/netjsongraph.js project, prioritize pragmatic reviews that focus on critical matters with real user impact (functionality, performance, security, maintainability) over minor code quality details that require significant effort for no noticeable benefit. Avoid perfectionist suggestions when tests pass and functionality is correct.
Applied to files:
test/netjsongraph.render.test.jssrc/js/netjsongraph.render.jstest/netjsongraph.browser.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.jstest/netjsongraph.browser.test.js
📚 Learning: 2026-01-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
📚 Learning: 2026-01-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
📚 Learning: 2026-01-04T08:00:40.936Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:40.936Z
Learning: In the netjsongraph.js codebase, the `== null` pattern is intentionally used to check for both null and undefined values in a single comparison, which is a well-established JavaScript idiom and should not be flagged for replacement with strict equality checks.
Applied to files:
src/js/netjsongraph.render.js
🧬 Code graph analysis (2)
src/js/netjsongraph.render.js (2)
test/netjsongraph.calc.test.js (1)
configs(3-32)src/js/netjsongraph.clients.js (1)
currentZoom(21-21)
test/netjsongraph.browser.test.js (1)
test/browser.test.utils.js (6)
getRenderedNodesAndLinksCount(72-88)getRenderedNodesAndLinksCount(72-88)urls(32-43)urls(32-43)getElementByCss(45-52)getElementByCss(45-52)
🪛 GitHub Actions: netjsongraph.js CI BUILD
src/js/netjsongraph.render.js
[error] 928-928: Module parse failed: Export 'echarts' is not defined (928:28). You may need an appropriate loader to handle this file type.
🔇 Additional comments (11)
README.md (1)
311-317: Documentation accurately reflects the newshowMapLabelsAtZoomoption.The description of
falseto disable and numeric threshold behavior aligns with the implementation innetjsongraph.render.js.src/js/netjsongraph.render.js (5)
60-60: Good addition —hideDelay: 0ensures tooltips disappear predictably.This eliminates the delayed tooltip hide that contributed to visual overlap with labels.
177-178: Correct: making graph labelssilentprevents hover event conflicts.This addresses the core PR objective of avoiding interaction conflicts between labels and tooltips.
334-339: Map label config properly merges existing label, conditional visibility, andsilent: true.The previous duplicate-
label-property issue (flagged in past reviews) has been resolved — the secondlabel:line is removed, so this merged object is now the effective config.
606-627: Initial label visibility and zoom-threshold gating look correct.The falsy check on
showMapLabelsAtZoomcorrectly treatsfalse,0,null, andundefinedas "disabled" — consistent with the project preference. Based on learnings,showMapLabelsAtZoom = 0is treated as "disabled" (not a valid threshold).
786-798: Cluster click zoom logic looks correct.Zooming to
currentZoom + 2(capped at max) on cluster click is a reasonable UX improvement.test/netjsongraph.browser.test.js (1)
30-31: Cosmetic spacing changes in destructuring — no behavioral impact.test/netjsongraph.render.test.js (4)
981-981: Correct:echarts.onmock is needed for the new mouseover/mouseout handler registration.
1275-1285: Updated call count expectations correctly reflect the new label visibilitysetOptioncalls.The annotation explaining initial render + moveend = 2 calls is helpful.
1506-1563: Well-structured test setup for label/tooltip interaction scenarios.The mock correctly includes
silent: trueingenerateMapOptionreturn value to match the actual implementation, and theechartsSetOptiondelegate toecharts.setOptionenables proper call tracking.
1565-1611: Good coverage of the core label behaviors: silent enforcement, zoomend preservation, and hover toggling.These tests directly exercise the fix described in the PR objectives.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
I just have to fix merge conflicts now. Will try to fix it asap |
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
7751626 to
9096d11
Compare
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
9096d11 to
01e8856
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)
606-627:⚠️ Potential issue | 🟠 Major
showMapLabelsAtZoomis captured once at render time — runtime config changes won't take effect.The destructuring on line 606 creates a local constant. Any post-render mutation of
self.config.showMapLabelsAtZoom(e.g., from the browser test at line 376) won't affect thezoomend,mouseover, ormouseouthandlers. This is the root cause of the browser test's false-positive pass (it works coincidentally because the default threshold 13 already satisfies the assertions).If dynamic config changes need to be supported, read from
self.config.showMapLabelsAtZoomdirectly in each handler instead of closing over the destructured value.
🤖 Fix all issues with AI agents
In `@src/js/netjsongraph.js`:
- Around line 39-49: The backward-compatibility aliasing currently reads from
this.config but writes the alias back to the wrapper instead of the core graph
config used at render time; change the block that checks
this.config.showMapLabelsAtZoom and this.config.showLabelsAtZoomLevel to assign
the aliased value into the core config object
(this.graph.config.showMapLabelsAtZoom = this.config.showLabelsAtZoomLevel) so
the value survives the setConfig/deepMergeObj merge and is what rendering code
(self.config / this.graph.config) reads; keep the warning message but ensure the
check still guards undefined values before assigning into this.graph.config.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.nvmrcREADME.mdsrc/js/netjsongraph.config.jssrc/js/netjsongraph.jssrc/js/netjsongraph.render.jstest/netjsongraph.browser.test.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 (4)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
test/netjsongraph.browser.test.jssrc/js/netjsongraph.jssrc/js/netjsongraph.config.jstest/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.browser.test.jstest/netjsongraph.render.test.js
test/netjsongraph.browser.test.js
📄 CodeRabbit inference engine (AGENTS.md)
Write browser tests in test/netjsongraph.browser.test.js using Chrome/ChromeDriver and run with
yarn test test/netjsongraph.browser.test.js
Files:
test/netjsongraph.browser.test.js
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.jssrc/js/netjsongraph.config.jssrc/js/netjsongraph.render.js
🧠 Learnings (12)
📓 Common learnings
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 0
File: :0-0
Timestamp: 2026-02-06T16:33:48.468Z
Learning: In the openwisp/netjsongraph.js project, prioritize pragmatic reviews that focus on critical matters with real user impact (functionality, performance, security, maintainability) over minor code quality details that require significant effort for no noticeable benefit. Avoid perfectionist suggestions when tests pass and functionality is correct.
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:21.783Z
Learning: Project preference: treat showMapLabelsAtZoom = 0 as “disabled” (not a valid threshold) and keep falsy checks in src/js/netjsongraph.render.js; document or normalize this if needed.
📚 Learning: 2026-01-30T08:31:21.783Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:21.783Z
Learning: Project preference: treat showMapLabelsAtZoom = 0 as “disabled” (not a valid threshold) and keep falsy checks in src/js/netjsongraph.render.js; document or normalize this if needed.
Applied to files:
test/netjsongraph.browser.test.jssrc/js/netjsongraph.jssrc/js/netjsongraph.config.jstest/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.browser.test.jstest/netjsongraph.render.test.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.browser.test.jssrc/js/netjsongraph.config.jstest/netjsongraph.render.test.jssrc/js/netjsongraph.render.js
📚 Learning: 2026-01-04T08:00:40.936Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:40.936Z
Learning: In the netjsongraph.js codebase, the `== null` pattern is intentionally used to check for both null and undefined values in a single comparison, which is a well-established JavaScript idiom and should not be flagged for replacement with strict equality checks.
Applied to files:
src/js/netjsongraph.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.jssrc/js/netjsongraph.config.jssrc/js/netjsongraph.render.js
📚 Learning: 2026-02-06T16:33:48.468Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 0
File: :0-0
Timestamp: 2026-02-06T16:33:48.468Z
Learning: In the openwisp/netjsongraph.js project, prioritize pragmatic reviews that focus on critical matters with real user impact (functionality, performance, security, maintainability) over minor code quality details that require significant effort for no noticeable benefit. Avoid perfectionist suggestions when tests pass and functionality is correct.
Applied to files:
src/js/netjsongraph.jstest/netjsongraph.render.test.jssrc/js/netjsongraph.render.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:
src/js/netjsongraph.jstest/netjsongraph.render.test.jssrc/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.jstest/netjsongraph.render.test.jssrc/js/netjsongraph.render.js
📚 Learning: 2026-01-20T00:42:32.369Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:42:32.369Z
Learning: In the netjsongraph.js codebase, node IDs are guaranteed not to contain the "~" character, so using "source~target" as a composite key for links in nodeLinkIndex will not collide with node ID keys.
Applied to files:
src/js/netjsongraph.js
📚 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-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
🧬 Code graph analysis (2)
test/netjsongraph.browser.test.js (1)
test/browser.test.utils.js (4)
urls(32-43)urls(32-43)getElementByCss(45-52)getElementByCss(45-52)
test/netjsongraph.render.test.js (3)
test/netjsongraph.util.test.js (1)
mockLeaflet(4-13)webpack-plugins/inject-leaflet-loader.js (1)
JSON(16-16)test/netjsongraph.duplicateNodes.test.js (1)
NetJSONGraphRender(2-2)
🪛 GitHub Actions: netjsongraph.js CI BUILD
src/js/netjsongraph.render.js
[error] 928-928: Module parse failed: Export 'echarts' is not defined (928:28). You may need an appropriate loader to handle this file type. export {NetJSONGraphRender, echarts, L};
🔇 Additional comments (6)
src/js/netjsongraph.config.js (1)
45-45: LGTM!Clean rename from
showLabelsAtZoomLeveltoshowMapLabelsAtZoomwith the same default value. Backward compatibility is handled in the constructor.src/js/netjsongraph.render.js (3)
60-60: LGTM —hideDelay: 0for predictable tooltip behavior.This eliminates the stale tooltip lingering on mouseout, directly addressing the visual clutter issue.
177-178: LGTM — marking graph labels assilentprevents hover conflicts.This ensures labels don't intercept mouse events, letting tooltips remain the primary interactive overlay.
334-339: LGTM — map labels now merge config, conditionally hide, and are silent.The duplicate
labelproperty from the previous iteration has been resolved. The spread merges existing label config, conditionally disables viashowMapLabelsAtZoom, and addssilent: true.test/netjsongraph.render.test.js (2)
1506-1563: Good test coverage for the label/tooltip interaction behavior.The mock setup correctly wires
echarts.onandechartsSetOption, and the test suite covers the key scenarios: silent labels, zoomend behavior, hover toggling, and disabled labels. Well-structured.
1613-1685: Thorough verification that disabled labels stay disabled across all interactions.This test correctly validates that when
showMapLabelsAtZoomis falsy, neither zoom changes nor hover events re-enable labels. The assertion pattern (tracking call counts before/after) is clean.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
netjsongraph.js_.Leaflet.plugins.mp4
Almost everything is addressed. The only outstanding item is the .nvmrc missing trailing newline.
|
@Adityashandilya555 I think you added a video of showMapLabelsAtZoom being set to false. |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai is this ready to merge? |
|
Tip For best results, initiate chat on the files or code changes. The Comment image shows a geographic map with mountain labels (e.g., “Südliche Wösterspitze 2537 m”, “Ruggles 2270 m”, “Trittwankopf 2482 m”) and a blue node under the cursor. A white tooltip is visible reading:
Why I think it’s ready
One small follow-up (optional pre-merge, trivial post-merge)
If you want, I can open a quick follow-up issue for that README tweak. 🧠 Learnings used |
Will update as per its requirements and push asap |
|
The Comment image shows a geographic map with mountain POIs labeled (e.g., “Südliche Wösterspitze 2537 m”, “Rüggle 2270 m”). A blue node is under the cursor, and a white tooltip is visible with:
What’s green right now
What still blocks merge
Nice-to-have (non-blocking)
Ping me after your push; I’ll re-run the review immediately. 🧠 Learnings used✅ Actions performedReview triggered.
|
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I am sorry it took long for me to review this, it's been a tough period.
It looks mostly good, but on a closer look I still found some issues, see below.
I would appreciate a screen recording that shows how the UI issues have been addressed.
1. Performance: setOption called on every mouseover/mouseout (High)
File: src/js/netjsongraph.render.js:629-667
The mouseover and mouseout handlers each call self.echarts.setOption()
on every single hover event. setOption is not a cheap operation -- it triggers
ECharts' internal diff/merge and re-render pipeline. On a map with hundreds of
nodes, rapidly moving the cursor across multiple nodes will trigger this
repeatedly, potentially causing jank or frame drops.
Suggestion: Debounce the handlers, or track a boolean state flag and skip
the setOption call if the label visibility already matches the desired state.
let labelsCurrentlyHidden = false;
self.echarts.on("mouseover", () => {
if (!labelsCurrentlyHidden && showMapLabelsAtZoom && ...) {
labelsCurrentlyHidden = true;
self.echarts.setOption(/* hide */);
}
});
self.echarts.on("mouseout", () => {
if (labelsCurrentlyHidden && showMapLabelsAtZoom && ...) {
labelsCurrentlyHidden = false;
self.echarts.setOption(/* show */);
}
});2. Event handler accumulation on re-render (Medium)
File: src/js/netjsongraph.render.js:629, 649
self.echarts.on("mouseover", ...) and self.echarts.on("mouseout", ...) are
registered every time mapRender is called. If the user toggles between map
and graph mode via switchMode (see netjsongraph.js:150-176), mapRender
is called again, and new handlers are stacked on top of old ones. Over several
toggles this leads to duplicate handler invocations.
Suggestion: Call self.echarts.off("mouseover") /
self.echarts.off("mouseout") before registering, or use named functions and
off them first.
3. All labels hide on any node hover (Low/UX)
Screencast.from.2026-03-11.16-54-19.mp4
File: src/js/netjsongraph.render.js:629-667
When hovering any node, all labels on the map disappear. The issue
description says "the labels should disappear if there's another overlay
showing up on hover." The current implementation is broader than necessary --
it hides every label rather than just the one overlapping with the tooltip.
It looks quite weird on a busy map. Can we only hide the specific node label being hovered? That's what I was expecting, I wasn't expecting that all labels were hidden.
While testing for this issue, I found another issue: if you change the zoom level, labels are hidden until mouseover, see:
Screencast.from.2026-03-11.16-54-34.mp4
4. Config value captured at render time won't reflect dynamic changes (Low)
File: src/js/netjsongraph.render.js:606
const {showMapLabelsAtZoom} = self.config;This destructures the value once at render time. The mouseover, mouseout,
and zoomend handlers all close over this captured value. If a consumer
changes self.config.showMapLabelsAtZoom after initial render (e.g., via a
settings UI), the handlers won't see the updated value.
Suggestion: Read from self.config.showMapLabelsAtZoom directly in the
handlers instead of destructuring.
5. animationDuration: 1000 added without clear reason (Low)
File: src/js/netjsongraph.render.js:334
The scatter series now has animationDuration: 1000 added. This wasn't
mentioned in the issue or any commit message. It adds a 1-second animation to
node rendering which could impact perceived performance on large datasets.
6. silent: true also set on graph mode labels (Low)
File: src/js/netjsongraph.render.js:178
baseGraphLabel.silent = true;The issue is specifically about map mode label/tooltip overlap. Setting
silent: true on graph-mode labels means graph labels also won't respond to
mouse events. This may or may not be desired -- the issue description didn't
mention graph mode labels having overlap problems. If graph labels should remain interactive, this line should be removed.
Question: can you explain why this is added and what effect it has?
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
6fedf7c to
7d0bdcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/js/netjsongraph.render.js`:
- Line 602: Remove the commented-out dead code line referencing
showMapLabelsAtZoom in the rendering module (the line "// const
{showMapLabelsAtZoom} = self.config;") inside src/js/netjsongraph.render.js;
simply delete this unused commented declaration so the file contains no leftover
commented variable destructuring and keep related code (e.g., any uses of
self.config or map label logic) intact.
🪄 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: 019d870c-b5f6-4c49-8801-61ce226ee353
📒 Files selected for processing (2)
src/js/netjsongraph.render.jstest/netjsongraph.render.test.js
📜 Review details
🧰 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 (11)
📓 Common learnings
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:21.783Z
Learning: Project preference: treat showMapLabelsAtZoom = 0 as “disabled” (not a valid threshold) and keep falsy checks in src/js/netjsongraph.render.js; document or normalize this if needed.
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 0
File: :0-0
Timestamp: 2026-02-06T16:33:48.468Z
Learning: In the openwisp/netjsongraph.js project, prioritize pragmatic reviews that focus on critical matters with real user impact (functionality, performance, security, maintainability) over minor code quality details that require significant effort for no noticeable benefit. Avoid perfectionist suggestions when tests pass and functionality is correct.
📚 Learning: 2026-01-30T08:31:21.783Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:21.783Z
Learning: Project preference: treat showMapLabelsAtZoom = 0 as “disabled” (not a valid threshold) and keep falsy checks in src/js/netjsongraph.render.js; document or normalize this if needed.
Applied to files:
test/netjsongraph.render.test.js
📚 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-02-06T16:33:48.468Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 0
File: :0-0
Timestamp: 2026-02-06T16:33:48.468Z
Learning: In the openwisp/netjsongraph.js project, prioritize pragmatic reviews that focus on critical matters with real user impact (functionality, performance, security, maintainability) over minor code quality details that require significant effort for no noticeable benefit. Avoid perfectionist suggestions when tests pass and functionality is correct.
Applied to files:
test/netjsongraph.render.test.jssrc/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:
test/netjsongraph.render.test.jssrc/js/netjsongraph.render.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-30T08:31:13.746Z
Learnt from: Dhruv-ub
Repo: openwisp/netjsongraph.js PR: 474
File: src/js/netjsongraph.render.js:347-351
Timestamp: 2026-01-30T08:31:13.746Z
Learning: In src/js/netjsongraph.render.js, treat showMapLabelsAtZoom = 0 as the explicit 'disabled' state (not a valid threshold). Do not rely on generic falsy checks for this flag. Update logic to distinguish between undefined/null (not set), 0 (disabled), and >0 (enabled at a given zoom). For example, use explicit checks such as: if (showMapLabelsAtZoom === 0) { /* labels hidden */ } else if (typeof showMapLabelsAtZoom === 'number' && showMapLabelsAtZoom > 0) { /* show labels at/above this zoom */ } else { /* default behavior or undefined */ }. If needed, document this convention or normalize input (e.g., set undefined to a sensible default) to avoid ambiguity in future reviews.
Applied to files:
src/js/netjsongraph.render.js
📚 Learning: 2026-01-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-04T08:00:40.936Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:40.936Z
Learning: In the netjsongraph.js codebase, the `== null` pattern is intentionally used to check for both null and undefined values in a single comparison, which is a well-established JavaScript idiom and should not be flagged for replacement with strict equality checks.
Applied to files:
src/js/netjsongraph.render.js
🔇 Additional comments (15)
src/js/netjsongraph.render.js (6)
60-60: LGTM!Setting
hideDelay: 0ensures the tooltip disappears immediately, improving predictability and aligning with the PR objective of making tooltip behavior explicit.
331-335: LGTM!The label configuration correctly merges existing config, conditionally hides labels when
showMapLabelsAtZoomis falsy, and setssilent: trueto prevent hover event capture. This addresses the PR objective of making labels non-interactive.
603-623: LGTM!The initial label visibility logic correctly hides labels when
showMapLabelsAtZoomis falsy or when the current zoom is below the threshold. Thesilent: trueflag ensures labels don't capture hover events.
625-633: LGTM!The empty handlers are appropriately retained for test compatibility. ECharts natively handles label visibility through the
emphasis: { label: { show: false } }configuration, making explicit setOption calls unnecessary.
635-655: LGTM!The
zoomendhandler correctly evaluates label visibility dynamically, maintainingsilent: trueand keeping emphasis labels hidden to prevent redundancy with tooltips.
754-766: LGTM!The cluster click handler now zooms to
currentZoom + 2(capped at max zoom) instead of just expanding, providing better user interaction for navigating into clusters.test/netjsongraph.render.test.js (9)
981-981: LGTM!Adding the
on: jest.fn()mock correctly supports testing the new echarts event handler registration.
1081-1086: LGTM!The config field rename to
showMapLabelsAtZoomand echarts mock update align with the implementation changes.
1224-1239: LGTM!The config update and the
echartsSetOptiondelegation tomockSelf.echarts.setOptionallow proper tracking of setOption calls in tests.
1275-1285: LGTM!The comments clearly explain the expected call count logic: 1 (initial render) + 1 (moveend update) = 2. This makes the test intention clear.
1473-1479: LGTM!Consistent updates to use the renamed config field and mock echarts event registration.
1506-1563: LGTM!The test setup properly configures mocks for testing label/tooltip interaction, including the key
silent: trueproperty in the mock return value.
1565-1584: LGTM!These tests verify that labels remain
silent: trueboth on initial render and after zoomend events, ensuring labels don't capture hover events.
1586-1606: LGTM!This test verifies that the mouseover/mouseout handlers are registered and callable without errors. Since the handlers are intentionally no-ops (ECharts handles label visibility natively via emphasis config), no assertions are needed beyond verifying registration.
1608-1670: LGTM!Comprehensive test coverage for the "labels completely disabled" scenario when
showMapLabelsAtZoomisfalse. The test correctly verifies:
- Initial label hiding via setOption
- Labels remain hidden at high zoom (zoomend)
- Labels remain hidden at low zoom (zoomend)
- Hover/unhover handlers don't error when labels are disabled
- Optimize ECharts setOption to prevent hover frame drops - Clean up event handlers on re-render to avoid duplication - Hide only hovered node label and fix zoom visibility bug - Read config state dynamically - Disable cluster animation and remove graph label silent flag
Dhruv-ub
force-pushed
the
issues/454-fix-redundant-labels
branch
from
7d0bdcf to
2699d01
Compare
Node.Label.Testing.mp4Here is the testing video for the issue |
4 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