[chores:fix] Preserve indoor overlay URL fragment on popup close #547
Merged
Conversation
Added configurable fragment preservation support for bookmarkable actions so indoor map overlays retain their URL fragment when only the popup nodeId is removed. Fixes #546
📝 WalkthroughWalkthroughIntroduces a new bookmarkableActions.preserveFragment option (default false) and sets it true in the indoor map example. updateUrlFragments now removes the hash when empty. removeUrlFragment gains a preserveFragment parameter so removing nodeId can retain an id-only fragment when requested. NetJSONGraphGUI.loadNodePopup reads preserveFragment and passes it to removeUrlFragment in failure and popup-remove paths. Tests and README updated to reflect the new option and method signature. Sequence Diagram(s)sequenceDiagram
participant Client as User / Browser
participant GUI as NetJSONGraphGUI.loadNodePopup
participant Config as config.bookmarkableActions
participant Utils as utils.removeUrlFragment
participant History as Browser History
Client->>GUI: open node popup (may set fragment nodeId)
GUI->>Config: read { id, preserveFragment }
alt Content build fails
GUI->>Utils: removeUrlFragment(id, "nodeId", preserveFragment)
Utils->>History: updateUrlFragments / pushState
end
alt Popup removed and fragment nodeId matches popup
GUI->>Utils: removeUrlFragment(id, "nodeId", preserveFragment)
Utils->>History: updateUrlFragments / pushState
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Overview
Incremental Review (fa3071b..HEAD)The latest commit adds: Documentation (src/js/netjsongraph.util.js lines 1348-1349):
Test Coverage (test/netjsongraph.util.test.js lines 736-751):
Previous Review ContextThe PR implements a well-structured fix for issue #546 that allows indoor map overlays to preserve their URL fragment ID when popups close. The core implementation was reviewed in previous commits and found to be sound. Files Reviewed (2 incremental + 6 previous)
Existing Inline Comments (3 active)
Reviewed by kimi-k2.5 · 95,532 tokens |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/netjsongraph.dom.test.js`:
- Around line 740-744: Add a test that sets bookmarkableActions.preserveFragment
= true before triggering the popup cleanup and assert that
testGraph.utils.removeUrlFragment was called with the third argument true (i.e.
expect(testGraph.utils.removeUrlFragment).toHaveBeenCalledWith("id", "nodeId",
true)); locate the existing popup cleanup test that currently asserts the
false/default path and duplicate/extend it to set preserveFragment true, trigger
the same cleanup flow (the same setup that calls popup cleanup), and add the new
expectation referencing testGraph.utils.removeUrlFragment so the true-path is
covered.
🪄 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: f5041171-f887-41e7-9c45-04850a4bdbc5
📒 Files selected for processing (5)
public/example_templates/netjsonmap-indoormap-overlay.htmlsrc/js/netjsongraph.config.jssrc/js/netjsongraph.gui.jssrc/js/netjsongraph.util.jstest/netjsongraph.dom.test.js
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Tests and Coverage (echarts-only)
- GitHub Check: Tests and Coverage (prod)
- GitHub Check: Tests and Coverage (dev)
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.jstest/netjsongraph.dom.test.js
src/**/*.js
📄 CodeRabbit inference engine (AGENTS.md)
Husky pre-commit hooks automatically run lint-staged to format JavaScript files in
src/**/*.jswith prettier
Files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
**/*.test.{js,ts}
📄 CodeRabbit inference engine (AGENTS.md)
Write unit tests using Jest with jsdom and run with
yarn test
Files:
test/netjsongraph.dom.test.js
🧠 Learnings (4)
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.jstest/netjsongraph.dom.test.js
📚 Learning: 2026-01-20T16:50:56.414Z
Learnt from: codesankalp
Repo: openwisp/netjsongraph.js PR: 425
File: src/js/netjsongraph.render.js:1-26
Timestamp: 2026-01-20T16:50:56.414Z
Learning: In netjsongraph.js, prefer importing from echarts/lib/chart/*/install, echarts/lib/component/*/install, and echarts/lib/renderer/install* rather than the public entry points (echarts/charts, echarts/components, echarts/renderers) to improve tree-shaking and reduce bundle size. Apply this pattern across JS files in src (update imports accordingly) and verify by comparing bundle sizes between the install-path imports and the public-entry imports.
Applied to files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.util.jssrc/js/netjsongraph.gui.js
📚 Learning: 2026-01-04T08:00:36.595Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.util.js:1302-1341
Timestamp: 2026-01-04T08:00:36.595Z
Learning: In the netjsongraph.js codebase, specifically in src/js/netjsongraph.util.js, the pattern '== null' is intentionally used to detect both null and undefined in a single comparison. Do not flag or replace these checks with strict equality checks (=== null or === undefined) for this file; preserve the established idiom.
Applied to files:
src/js/netjsongraph.util.js
📚 Learning: 2026-05-19T23:25:39.778Z
Learnt from: nemesifier
Repo: openwisp/netjsongraph.js PR: 542
File: src/js/netjsongraph.util.js:1478-1481
Timestamp: 2026-05-19T23:25:39.778Z
Learning: When reviewing netjsongraph label rendering logic in src/js/netjsongraph.util.js and src/js/netjsongraph.render.js, follow the documented contract for showMapLabelsAtZoom: (1) if showMapLabelsAtZoom is false, map labels must be completely disabled; (2) if it is 0, labels must always be shown regardless of zoom (do not treat 0 as disabled); (3) if it is a positive number N, labels must be shown only when zoom >= N. Ensure comparisons implement the intended semantics (i.e., zoom >= 0 is intentionally always true when the setting is 0).
Applied to files:
src/js/netjsongraph.util.js
🔇 Additional comments (4)
src/js/netjsongraph.config.js (1)
304-304: LGTM!public/example_templates/netjsonmap-indoormap-overlay.html (1)
317-317: LGTM!src/js/netjsongraph.util.js (1)
1273-1281: LGTM!Also applies to: 1349-1362
src/js/netjsongraph.gui.js (1)
328-329: LGTM!Also applies to: 357-357, 400-404
nemesifier
changed the title
nemesifier
approved these changes
May 22, 2026
nemesifier
changed the title
github-project-automation
Bot
moved this to In progress
in OpenWISP Priorities for next releases
github-project-automation
Bot
moved this from In progress
to Reviewer approved
in OpenWISP Priorities for next releases
github-project-automation
Bot
moved this from Reviewer approved
to Done
in OpenWISP Priorities for next releases
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow up of #542.
Added configurable fragment preservation support for bookmarkable actions so indoor map overlays retain their URL fragment when only the popup nodeId is removed.
Fixes #546
Checklist
Reference to Existing Issue
Closes #546.
Description of Changes
preserveFragmentto support removing the nodeId param based on the config