[change] Reduce library size #392#425
Conversation
codesankalp
force-pushed
the
fix/issue-392
branch
3 times, most recently
from
7b1d3ca to
5236b0e
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
The main problem I see is that now yarn build produces multiple files, which doesn't make it easy for us to drop the resulting work in OpenWISP. Is this on purpose?
Yes, there are several reasons for this approach:
The contenthash in filenames will optimize CDN cache management when using cdn (if we are using cdn). Integration Considerations:
let me know your thoughts! |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@codesankalp what do you think about the following?
- a result set providing the current output: all separate files
- a result set providing the library + echarts
- a result set providing the library + echarts + leaflet
Can you add basic explanation about the above?
codesankalp
force-pushed
the
fix/issue-392
branch
4 times, most recently
from
68ed674 to
17ffba3
Compare
|
New Buid Sizes:
|
codesankalp
force-pushed
the
fix/issue-392
branch
2 times, most recently
from
e43988e to
5355c03
Compare
codesankalp
force-pushed
the
fix/issue-392
branch
2 times, most recently
from
0c99ec8 to
b4bbfa9
Compare
There was a problem hiding this comment.
I have the following problem:
yarn install
yarn install v1.22.22
[1/4] Resolving packages...
[2/4] Fetching packages...
error Error: https://registry.yarnpkg.com/error-ex/-/error-ex-1.3.3.tgz: Request failed "404 Not Found"
at ResponseError.ExtendableBuiltin (/usr/share/yarn/lib/cli.js:696:66)
at new ResponseError (/usr/share/yarn/lib/cli.js:802:124)
at Request.<anonymous> (/usr/share/yarn/lib/cli.js:66750:16)
at Request.emit (node:events:519:28)
at module.exports.Request.onRequestResponse (/usr/share/yarn/lib/cli.js:142287:10)
at ClientRequest.emit (node:events:519:28)
at HTTPParser.parserOnIncomingClient (node:_http_client:716:27)
at HTTPParser.parserOnHeadersComplete (node:_http_common:117:17)
at TLSSocket.socketOnData (node:_http_client:558:22)
at TLSSocket.emit (node:events:519:28)
info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
I see the following line in yarn.lock:
debug@4, debug@^4.1.0, debug@^4.1.1, debug@^4.3.1, debug@^4.3.2, debug@^4.3.4, debug@^4.4.1:
version "4.4.2"
resolved "https://registry.yarnpkg.com/debug/-/debug-4.4.2.tgz#96b480a7fe47cc04fe57f0bf56e058dba2a001fa"
integrity sha512-IQeXCZhGRpFiLI3MYlCGLjNssUBiE8G21RMyNH35KFsxIvhrMeh5jXuG82woDZrYX9pgqHs+GF5js2Ducn4y4A==
dependencies:
ms "^2.1.3"
The URL https://registry.yarnpkg.com/debug/-/debug-4.4.2.tgz#96b480a7fe47cc04fe57f0bf56e058dba2a001fa returns 404.
I am not sure why the CI build is not complaining about this, but it feels wrong to me to reference a 404 URL in yarn.lock, what do you think?
codesankalp
force-pushed
the
fix/issue-392
branch
from
b4bbfa9 to
4d026ce
Compare
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
I did some changes to the README.
The save on the complete bundle is only ~300KB.
In OpenWISP dashboard we need to load the complete bundle, so reducing 300KB is good but I wonder if we really need 1.3 MB to show geographic and indoor maps.
Is there a way to reduce the imports of echarts so that we only import exactly what we use and let the javascript pruning tools do the work of removing all the JS code that we don't use?
codesankalp
force-pushed
the
fix/issue-392
branch
2 times, most recently
from
bc058f2 to
e6c4b28
Compare
Signed-off-by: Sankalp <sankalp.nex@gmail.com>
codesankalp
force-pushed
the
fix/issue-392
branch
from
d8082d5 to
7af27bb
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 5-7: The package.json currently points "main" to
dist/netjsongraph.min.js but builds are ignored by git and not included in the
published package; add a "prepare" script (e.g., add "prepare": "yarn
build:full" to the scripts object so npm/yarn runs the build on publish) and/or
add a "files" array (e.g., include "dist/") to package.json to ensure the dist/
directory is packaged; update the scripts and/or files keys accordingly so that
the "main" entry exists in the published package.
In `@src/js/netjsongraph.config.js`:
- Around line 366-375: The crs getter on config calls getLeaflet() without
suppressing warnings, causing repeated console warnings in echarts-only builds;
update the getter to call getLeaflet(true) so it suppresses the warning when
Leaflet is absent and keep the rest of the logic (return L ? L.CRS.EPSG3857 :
null) intact—modify the Object.defineProperty for config.crs that currently
references getLeaflet() to use getLeaflet(true) instead.
In `@webpack-plugins/inject-leaflet-loader.js`:
- Around line 62-70: The onerror handler for the created script element
(leafletJS) only logs to console and can leave the page stuck; update
leafletJS.onerror to both surface a visible error and recover: create or update
a DOM error element (e.g., show a banner/message) and then call initMap() (or
loadLeafletPlugins if hasPlugins is true) so the page proceeds in a degraded
state; locate the leafletJS creation block and modify leafletJS.onerror to
display an in-page error message and invoke the appropriate initializer (initMap
or loadLeafletPlugins) instead of only console.error.
In `@webpack.config.js`:
- Around line 207-211: The webpack performance config currently sets
maxEntrypointSize and maxAssetSize too low for the full build; update the
performance block (performance, maxEntrypointSize, maxAssetSize) to either raise
maxAssetSize (and optionally maxEntrypointSize) to values that cover the full
"Library + ECharts + Leaflet" bundle (e.g., bump maxAssetSize to ~450000 or
similar) or add a comment documenting that production builds will generate
warnings for the full-build entrypoint and those warnings are expected; change
the numeric thresholds in the performance config or add the explanatory comment
accordingly.
🧹 Nitpick comments (4)
src/js/netjsongraph.core.js (1)
80-80:async render()neverawaits the data-loading promise chain — consider returning or awaiting it.The method is marked
asyncbut thepaginatedDataParse.then()chain (Line 105) is fire-and-forget. This meansawait someInstance.render()resolves immediately, before data is loaded and the view is painted. If callers rely on the event system (onReady/onLoad) this works, but theasyncsignature is misleading — it suggests awaiting it guarantees a rendered state.Consider either:
return this.utils.paginatedDataParse.call(…).then(…)so the caller can actually await completion, or- Documenting clearly that render is fire-and-forget and callers should listen for lifecycle events.
src/js/leaflet-loader.js (1)
1-37: Clean runtime loader pattern — well structured.The caching, build-time branching via
BUNDLE_LEAFLET, and graceful fallback for the echarts-only build are solid. One tiny nit:Line 1's
/* eslint-disable import/no-mutable-exports */appears unnecessary — the only export is thegetLeafletfunction declaration (an immutable binding); the mutablelet Lis module-private and not exported.src/js/echarts-leaflet/LeafletCoordSys.js (1)
151-155: Dead code:typeof L === "undefined"can never be true here.
Lis a closure variable captured from line 12. SincecreateLeafletCoordSystem()throws on line 14 ifLis falsy, by the timeLeafletCoordSys.createexecutes,Lis guaranteed to be defined and truthy. This guard was meaningful whenLwas a global, but is now dead code.♻️ Suggested cleanup
if (leafletCoordSys) { throw new Error("Only one leaflet component can exist"); } - if (typeof L === "undefined") { - throw new Error("Leaflet api is not loaded"); - } if (!leafletModel.__map) {webpack.config.js (1)
186-202: Consider extractingzlibconstants to reduce repetition.
require("zlib")is called four times inline within the Brotli compression options. Extracting it to a local constant improves readability.♻️ Suggested refactor
+const zlib = require("zlib"); + // ...inside the production plugins array: new CompressionPlugin({ filename: "[path][base].br", algorithm: "brotliCompress", test: /\.(js|css|html|svg|json)$/, compressionOptions: { params: { - [require("zlib").constants.BROTLI_PARAM_QUALITY]: 11, - [require("zlib").constants.BROTLI_PARAM_SIZE_HINT]: 0, - [require("zlib").constants.BROTLI_PARAM_MODE]: - require("zlib").constants.BROTLI_MODE_TEXT, - [require("zlib").constants.BROTLI_PARAM_LGWIN]: 22, - [require("zlib").constants.BROTLI_PARAM_LGBLOCK]: 0, + [zlib.constants.BROTLI_PARAM_QUALITY]: 11, + [zlib.constants.BROTLI_PARAM_SIZE_HINT]: 0, + [zlib.constants.BROTLI_PARAM_MODE]: zlib.constants.BROTLI_MODE_TEXT, + [zlib.constants.BROTLI_PARAM_LGWIN]: 22, + [zlib.constants.BROTLI_PARAM_LGBLOCK]: 0, }, },
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
lib/css/assets/leaflet-measure.pngis excluded by!**/*.pngpublic/assets/images/floorplan.pngis excluded by!**/*.pngyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (27)
.eslintrc.js.github/workflows/ci.ymlREADME.mdindex.htmlknip.jsonpackage.jsonpublic/assets/data/airplaneRouteMap.jsonpublic/assets/data/netjsongraph-graphGL.jsonpublic/example_templates/netjsongraph-graphGL.htmlpublic/example_templates/netjsonmap-animation.htmlpublic/example_templates/netjsonmap-indoormap.htmlsrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.clients.jssrc/js/netjsongraph.config.jssrc/js/netjsongraph.core.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.jssrc/js/netjsongraph.render.jstest/browser.test.utils.jstest/netjsongraph.duplicateNodes.test.jstest/netjsongraph.render.test.jswebpack-plugins/inject-leaflet-loader.jswebpack.config.js
💤 Files with no reviewable changes (4)
- test/browser.test.utils.js
- public/example_templates/netjsongraph-graphGL.html
- public/assets/data/airplaneRouteMap.json
- public/example_templates/netjsonmap-animation.html
🚧 Files skipped from review as they are similar to previous changes (5)
- src/js/netjsongraph.clients.js
- knip.json
- .eslintrc.js
- test/netjsongraph.duplicateNodes.test.js
- README.md
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,css,scss,json}
📄 CodeRabbit inference engine (AGENTS.md)
Format code using
openwisp-qa-formatcommand with Python virtualenv enabled andyarn lint:fix(runs eslint --fix and prettier via lint-staged)
Files:
webpack.config.jspackage.jsonwebpack-plugins/inject-leaflet-loader.jssrc/js/netjsongraph.jssrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/netjsongraph.config.jstest/netjsongraph.render.test.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.core.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/echarts-leaflet/LeafletCoordSys.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/netjsongraph.config.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.core.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
🧠 Learnings (11)
📓 Common learnings
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.
📚 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:
webpack.config.jspackage.jsonwebpack-plugins/inject-leaflet-loader.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:
package.jsontest/netjsongraph.render.test.js.github/workflows/ci.yml
📚 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.{js,ts} : Write unit tests using Jest with jsdom and run with `yarn test`
Applied to files:
package.json.github/workflows/ci.yml
📚 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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.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/echarts-leaflet/LeafletCoordSys.jssrc/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/echarts-leaflet/LeafletModel.jssrc/js/echarts-leaflet/LeafletView.jssrc/js/echarts-leaflet/index.jssrc/js/netjsongraph.config.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.core.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.jssrc/js/echarts-leaflet/LeafletCoordSys.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:
src/js/netjsongraph.geojson.jssrc/js/netjsongraph.render.jssrc/js/netjsongraph.config.jstest/netjsongraph.render.test.jssrc/js/netjsongraph.core.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-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: Run QA checks using `./run-qa-checks` before committing code
Applied to files:
.github/workflows/ci.yml
🧬 Code graph analysis (7)
webpack.config.js (1)
webpack-plugins/inject-leaflet-loader.js (1)
path(5-5)
src/js/echarts-leaflet/LeafletCoordSys.js (2)
src/js/leaflet-loader.js (2)
L(12-12)getLeaflet(14-37)src/js/echarts-leaflet/index.js (1)
L(11-11)
src/js/netjsongraph.geojson.js (4)
src/js/leaflet-loader.js (2)
L(12-12)getLeaflet(14-37)src/js/echarts-leaflet/index.js (1)
L(11-11)src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/netjsongraph.config.js (1)
L(370-370)
src/js/echarts-leaflet/LeafletView.js (1)
src/js/echarts-leaflet/LeafletCoordSys.js (3)
mapOffset(126-126)mapOffset(132-132)api(113-113)
src/js/netjsongraph.config.js (4)
src/js/leaflet-loader.js (2)
L(12-12)getLeaflet(14-37)src/js/echarts-leaflet/index.js (1)
L(11-11)src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/netjsongraph.geojson.js (2)
L(136-136)L(140-140)
test/netjsongraph.render.test.js (1)
test/netjsongraph.duplicateNodes.test.js (1)
NetJSONGraphRender(2-2)
src/js/leaflet-loader.js (4)
src/js/echarts-leaflet/index.js (1)
L(11-11)src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/netjsongraph.config.js (1)
L(370-370)src/js/netjsongraph.geojson.js (2)
L(136-136)L(140-140)
🪛 Biome (2.3.13)
webpack-plugins/inject-leaflet-loader.js
[error] 160-160: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 GitHub Check: CodeQL
.github/workflows/ci.yml
[warning] 52-129: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{contents: read}}
⏰ 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 (dev)
🔇 Additional comments (28)
.github/workflows/ci.yml (1)
83-97: CI matrix covers all build variants with browser tests — good setup.Conditional build/server steps cleanly separate the three paths, and browser tests (Line 121) run unconditionally across all matrix entries, which matches the reviewer request to exercise echarts-only builds in CI.
src/js/echarts-leaflet/LeafletView.js (1)
1-7: Clean refactor to directecharts/coreimports and parameterless factory.The shift from dependency-injected
echarts/Lparameters to direct imports fromecharts/coreis consistent across all echarts-leaflet modules and removes unnecessary coupling. Theconsttightening onpartsandmapOffsetis a nice touch.src/js/echarts-leaflet/LeafletModel.js (1)
1-7: Consistent refactor mirroring the LeafletView.js pattern — LGTM.Direct import of
extendComponentModelfromecharts/coreand parameterless factory signature align with the other echarts-leaflet module changes.Also applies to: 17-17
src/js/netjsongraph.render.js (4)
1-26: Modular echarts imports via/lib/*/installpaths — consistent with project learnings.The
use()registration pattern with individual install imports aligns with the tested approach that yields smaller bundles in this project's webpack configuration. Addition ofCanvasRendereralongsideSVGRendererensures both renderers are available. Based on learnings, the/lib/*/installpattern produces better tree-shaking results than the public entry points in this project's webpack configuration.
515-520: Runtime Leaflet resolution with fail-fast error — solid approach.Destructuring
{circleMarker, latLngBounds}from the loaded Leaflet instance at the top ofmapRenderkeeps the rest of the method clean and avoids repeatedL.references. The early throw ensures a clear error message when Leaflet is genuinely missing.
206-207: Simplified to always usetype: "graph"— completes the graphGL removal.Hardcoding
type: "graph"with a"force"layout fallback removes the last graphGL code path. The|| "force"default is a safe choice for the standard graph layout.
871-871: Narrowed public API to a single default export.Removing the named
echartsandLexports in favor ofexport default NetJSONGraphRenderis a clean API surface reduction. Consumers that need echarts or Leaflet directly should useecharts/coreor the leaflet-loader respectively.webpack-plugins/inject-leaflet-loader.js (1)
1-21: Build-time integrity computation looks good.The approach of reading the Leaflet version and computing SRI hashes from the local
node_modulescopy at build time is solid — it ensures the CDN-served assets match what was tested locally.public/example_templates/netjsonmap-indoormap.html (1)
13-17: LGTM — Leaflet JS CDN tag added alongside existing CSS.The added script tag correctly matches the existing Leaflet CSS version (1.8.0) and includes an SRI integrity hash. The
InjectLeafletLoaderPluginwill strip these static references during echarts-only builds and replace them with the dynamic loader, while full builds will overridewindow.Lwith the bundled copy.src/js/netjsongraph.geojson.js (1)
1-1: Clean migration to runtime Leaflet loading.The pattern is consistent with other modules:
getLeaflet()→ early return if unavailable → destructure needed API. ThegeoJSONfactory destructuring fromLis correct for Leaflet 1.8.0.Also applies to: 136-140, 167-167
test/netjsongraph.render.test.js (3)
1-3: Import changes align with the new default-export pattern.
Lis now imported directly from"leaflet"andNetJSONGraphRenderuses a default import, consistent with the refactored module exports.
477-481: Async render test properly awaits the now-asyncrender().The switch from a synchronous throw expectation to
await map.render()+console.errorassertion correctly reflects the new async render path.
707-715: Good addition: prototype isolation prevents test pollution.The
beforeEach/afterEachsnapshot-and-restore ofNetJSONGraphRender.prototype's prototype chain prevents theObject.setPrototypeOfcall insetUp(line 718) from leaking across tests.package.json (1)
90-101: Dependency restructuring looks correct.Leaflet as an optional peer dependency, echarts and kdbush as direct dependencies — this properly supports both build variants.
zrenderremoval is fine since it's a transitive dependency ofecharts.src/js/netjsongraph.js (2)
8-11: Conditional Leaflet bundling for full builds.The
BUNDLE_LEAFLETguard correctly ensures Leaflet is onlyrequired and assigned towindow.Lin the full build variant. This runs before any component initialization, sogetLeaflet()calls in downstream modules will find it.
176-178: Default export added alongsidewindow.NetJSONGraph.This properly supports both ESM bundler consumers (via
import) and script-tag consumers (viawindow.NetJSONGraph).src/js/echarts-leaflet/LeafletCoordSys.js (2)
11-24: Clean refactoring to loader-based Leaflet access.The parameterless factory that obtains Leaflet via
getLeaflet()and destructures the needed APIs is well-structured. Thethrowon line 14 ensures no code path proceeds without a valid Leaflet instance.
175-197: Destructured Leaflet API usage is correct.
lMap,lTileLayer, andcontrol.layersproperly replace the formerL.map,L.tileLayer, andL.control.layerscalls. The aliased names (lMap,lTileLayer) avoid conflicts with local variables.src/js/netjsongraph.config.js (2)
1-1: LGTM on the import change.Clean switch from direct Leaflet import to the runtime loader pattern, consistent with the rest of the codebase.
377-378: Named + default exports look good.Exporting
prepareDatafrom the original config object (not the spread copy) preserves the original reference, and the default export is the enhanced config with the dynamiccrsgetter.webpack.config.js (5)
22-59: Aggressive Terser settings look appropriate for a production library bundle.The multi-pass compression with
pure_getters,collapse_vars, andtoplevelis well-suited for reducing the final output.hoist_vars: falseavoids potential issues with variable hoisting across modules.
89-103:sideEffects: trueis correctly set now—good.This properly enables webpack's side-effects optimization, consistent with the past review feedback.
83-88: Leaflet externalization for echarts-only build is correctly configured.Mapping
leafletto the globalLensures Leaflet is excluded from the bundle while remaining available at runtime viawindow.L.
169-171:DefinePluginforBUNDLE_LEAFLETcorrectly drives the runtime loader.
JSON.stringify(!isEchartsOnly)ensures the flag is properly injected as a boolean literal, which theleaflet-loader.jsmodule checks at runtime.
68-81: The review comment is incorrect. Thebuildscript in package.json is defined as"yarn build:full"only, not as a combined script that runs bothbuild:fullandbuild:echarts-onlysequentially. The two build variants are separate scripts that must be invoked independently. Whileclean: truewould erase the first build's output if both scripts were run one after another manually, this is not an automated workflow issue in the current package.json configuration.Likely an incorrect or invalid review comment.
src/js/echarts-leaflet/index.js (3)
1-5: Imports are well-structured.Using
echarts/corefor the registration APIs (registerCoordinateSystem,registerAction) is correct—the tree-shaking learning aboutecharts/lib/*/installpaths applies to chart/component/renderer modules, not the core registration surface.
10-16: Clean early-exit for missing Leaflet.
getLeaflet(true)correctly suppresses the warning, and bailing out before any Leaflet-dependent registration ensures the echarts-only build works without errors. This is the right pattern.
18-36: Registration flow looks correct.The parameterless calls to
extendLeafletModel(),extendLeafletView(), andcreateLeafletCoordSystem()align with the refactored modules that now source their dependencies internally. TheregisterActioncallback logic is unchanged.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
I thought you were facing issues in echarts build, and i was testing that only. After making the change, all the tests are passing. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 5-10: The package.json exposes "module": "src/js/netjsongraph.js"
but the "files" array only publishes "dist/", so consumers cannot resolve the
module entry; fix by either adding the source path to the published files (add
"src/" or "src/js/netjsongraph.js" to the "files" array) so the module field
remains valid, or remove/update the "module" field to point to a file inside
"dist/" (e.g., the bundled ES module) if only pre-built artifacts are intended
to be published.
🧹 Nitpick comments (8)
src/js/netjsongraph.core.js (3)
77-83:asynckeyword is unnecessary — no top-levelawaitin the method body.
render()is markedasyncbut never usesawaitin its own scope. All asynchronous work is kicked off via.then()chains (line 108) andevent.oncecallbacks. Theasynckeyword only wraps the return value in aPromise<undefined>that resolves immediately — before any data is loaded or rendered — which is inconsistent with what callers might expect from anasyncmethod.The JSDoc correctly says "fire-and-forget," but if callers genuinely should not
awaitthis, droppingasyncmakes that contract explicit and avoids a misleading return type.Suggested fix
- async render() { + render() {
100-107: Deadcatchblock —onReadyDonenever rejects.
onReadyDone(lines 89-97) always callsresolve()unconditionally, even when theonReadycallback throws (the error is logged, thenresolve()runs). That meansawait onReadyDoneon line 102 can never reject, so thecatchon lines 103-105 is dead code.Either remove the dead
catch, or change theonReadyDonepromise to actually reject on error so the catch serves a purpose.Option A — remove dead catch
this.event.once("applyUrlFragmentState", async () => { - try { - await onReadyDone; - } catch (e) { - console.error("onReady failed:", e); - } + await onReadyDone; this.utils.applyUrlFragmentState.call(this, this); });
89-97:onReadyerrors are silently swallowed — consider propagating.When
this.config.onReadythrows, the error is logged (line 94), butresolve()is still called, so downstream code (e.g.applyUrlFragmentState) proceeds as if nothing went wrong. This may be intentional for resilience, but it means a brokenonReadyhook will silently degrade the application state.If this is deliberate, a brief inline comment explaining why errors are swallowed (rather than rejecting) would help future maintainers.
webpack-plugins/inject-leaflet-loader.js (3)
9-21: Module-level I/O reads Leaflet files at require-time — acceptable but fragile.The integrity hash computation reads from
node_modules/leaflet/dist/when the plugin is first required. This will throw with a clear error if Leaflet isn't installed (e.g., missing devDependency). Since Leaflet is now a devDependency, this should always be available during builds.Consider wrapping the file reads in a try/catch with a descriptive error message to fail gracefully.
80-96:waitForLeafletpolls even on confirmed CDN failure — 5s delay before proceeding.When
leafletJS.onerrorfires (line 72),loadLeafletPlugins()is called, which enterswaitForLeafletand polls forLup to 50 × 100ms = 5 seconds. Since the CDN load already failed,Lwill never appear, so this always burns through the full timeout before callinginitMap().Consider passing a flag or skipping the poll when the error path is known:
♻️ Sketch — skip polling on known failure
leafletJS.onerror = function() { console.error('Failed to load Leaflet from CDN. Please check your internet connection.'); const errorDiv = document.createElement('div'); errorDiv.style.cssText = 'position:fixed;top:0;left:0;right:0;background:`#f44336`;color:white;padding:10px;text-align:center;z-index:9999;'; errorDiv.textContent = 'Error: Failed to load Leaflet. Please check your internet connection.'; document.body.appendChild(errorDiv); - ${hasPlugins ? "loadLeafletPlugins();" : "initMap();"} + initMap(); };When Leaflet fails to load, calling
loadLeafletPlugins()(which waits forL) adds an unnecessary 5-second delay. CallinginitMap()directly allows the page to render in its degraded state immediately.
201-207: Remove thedo/whileloop —String.replace()with the global regex flag replaces all matches in a single call.The
gflag inleafletJSRegexcausesreplace()to match and remove all occurrences at once. The loop executes twice (once to remove all matches, once to verify nothing changed), making it redundant. The CSS removal on line ~199 correctly omits this pattern.♻️ Suggested simplification
// Remove main leaflet.js script if present const leafletJSRegex = /<script[^>]*leaflet\.js[^>]*>[\s\S]*?<\/script>/gi; - let prev; - do { - prev = html; - html = html.replace(leafletJSRegex, ""); - } while (html !== prev); + html = html.replace(leafletJSRegex, "");webpack.config.js (2)
69-82:clean: truewill wipe the other variant's output if both builds are run in sequence.If a user runs
yarn build:fullfollowed byyarn build:echarts-only(or vice versa), the second build deletes the first build's output because both targetdist/withclean: true. This is likely the intended behavior (one variant at a time), but worth documenting or noting.If both outputs should coexist,
clean: truewould need to befalseor scoped to specific filenames.
142-206: Plugin composition is well structured.Good use of conditional spreads for production-only compression and optional bundle analysis. The
DefinePluginforBUNDLE_LEAFLETcorrectly drives the runtime Leaflet loading behavior inleaflet-loader.js.One minor note:
BundleAnalyzerPlugin(line 9) is eagerlyrequire()-d at module load even whenANALYZEis unset. A lazy require would shave off startup time for normal builds.♻️ Lazy require
-const BundleAnalyzerPlugin = require("webpack-bundle-analyzer").BundleAnalyzerPlugin; // ... later in plugins array: - ...(process.env.ANALYZE ? [new BundleAnalyzerPlugin()] : []), + ...(process.env.ANALYZE + ? [new (require("webpack-bundle-analyzer").BundleAnalyzerPlugin)()] + : []),
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
package.jsonsrc/js/echarts-leaflet/LeafletCoordSys.jssrc/js/leaflet-loader.jssrc/js/netjsongraph.config.jssrc/js/netjsongraph.core.jswebpack-plugins/inject-leaflet-loader.jswebpack.config.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/js/leaflet-loader.js
- src/js/echarts-leaflet/LeafletCoordSys.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{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:
webpack.config.jssrc/js/netjsongraph.config.jspackage.jsonsrc/js/netjsongraph.core.jswebpack-plugins/inject-leaflet-loader.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.core.js
🧠 Learnings (9)
📓 Common learnings
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.
📚 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:
webpack.config.jspackage.jsonwebpack-plugins/inject-leaflet-loader.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.core.js
📚 Learning: 2026-01-20T00:56:36.062Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:36.062Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same source and target). Ensure nodeLinkIndex is keyed by a single 'source~target' pair and that adding a link does not overwrite existing links for that pair. If parallel links are ever required, the data model must be changed (e.g., allow an array of links per 'source~target' key) rather than storing only one link.
Applied to files:
src/js/netjsongraph.config.jssrc/js/netjsongraph.core.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:
package.json
📚 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.{js,ts} : Write unit tests using Jest with jsdom and run with `yarn test`
Applied to files:
package.json
📚 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 **/*.{js,ts,css,scss,json} : Format code using `openwisp-qa-format` command with Python virtualenv enabled and `yarn lint:fix` (runs eslint --fix and prettier via lint-staged)
Applied to files:
package.json
📚 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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.js
🧬 Code graph analysis (3)
webpack.config.js (1)
webpack-plugins/inject-leaflet-loader.js (1)
path(5-5)
src/js/netjsongraph.config.js (4)
src/js/echarts-leaflet/LeafletCoordSys.js (1)
L(12-12)src/js/leaflet-loader.js (2)
L(11-11)getLeaflet(13-36)src/js/echarts-leaflet/index.js (1)
L(11-11)src/js/netjsongraph.geojson.js (2)
L(136-136)L(140-140)
webpack-plugins/inject-leaflet-loader.js (2)
webpack.config.js (2)
InjectLeafletLoaderPlugin(11-11)HtmlWebpackPlugin(6-6)test/browser.test.utils.js (1)
options(20-20)
🪛 Biome (2.3.13)
webpack-plugins/inject-leaflet-loader.js
[error] 165-165: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (6)
src/js/netjsongraph.core.js (1)
14-30: Unchanged segments look correct.The constructor,
setConfig, data-loading pipeline (paginatedDataParse→ node/link indexing → render), andsetUtilsare unchanged and consistent with the existing patterns and retrieved learnings (singlesource~targetkey fornodeLinkIndex, no parallel-link support).Also applies to: 42-71, 108-182, 194-220
package.json (2)
15-17: Build scripts and dual build variants look good.Clean separation of
build:fullandbuild:echarts-onlywithbuilddefaulting tobuild:full. The--env BUILD_TYPEflag correctly propagates the build variant to webpack.
94-105: Leaflet dependency structure is correct.Leaflet is properly in
devDependencies(for local development/builds),peerDependencies(to signal consumers), and markedoptionalinpeerDependenciesMeta. This allows echarts-only consumers to skip Leaflet entirely.webpack-plugins/inject-leaflet-loader.js (1)
43-76: CDN error handling now shows a visible error — good improvement.The
onerrorhandler (lines 66-72) now appends a visible error banner and still callsinitMap()/loadLeafletPlugins()to allow degraded functionality. This addresses the prior review concern about silent failures.webpack.config.js (1)
62-104: Build variant logic and optimization settings look solid.Clean separation of
isProduction/isEchartsOnlyflags, correct Leaflet externalization for echarts-only builds, andsideEffects: trueproperly enables tree-shaking. The TerserPlugin configuration is thorough with multi-pass compression.src/js/netjsongraph.config.js (1)
366-378: Shallow copy is mitigated by deep copy on instantiation.While
{...NetJSONGraphDefaultConfig}creates a shallow copy where nested objects are shared by reference, this is not a concern in practice. EachNetJSONGraphinstance performs a deep copy of the config innetjsongraph.core.js:24(this.config = this.utils.deepCopy(NetJSONGraphDefaultConfig)), ensuring nested objects are isolated per instance. Mutations to one instance's config do not affect others.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@webpack-plugins/inject-leaflet-loader.js`:
- Around line 218-225: The current logic runs a fresh regex against the entire
html to compute scriptType (scriptTypeMatch / scriptType), which can pick up a
different <script> tag; instead, derive the type from the previously captured
script tag match (the variable produced by scriptRegex / match) rather than
running a new regex on html—use the existing capture group that holds the type
(if present) to set scriptType, falling back to "text/javascript" when that
capture is undefined.
🧹 Nitpick comments (2)
webpack-plugins/inject-leaflet-loader.js (2)
9-21: Module-level I/O runs unconditionally, even when the plugin is a no-op.Lines 9–21 read and hash files from
node_modules/leafletatrequire()-time, before the constructor'sisEchartsOnlycheck. If Leaflet isn't installed (e.g., treated as a peer dependency in some downstream setup), this crashes the entire build with a rawENOENT, even for full builds where the plugin does nothing.Consider deferring these reads into
apply()(or a lazy getter) so the work only happens whenisEchartsOnlyis true.♻️ Suggested approach — lazy initialization
-const leafletPkgPath = path.resolve(__dirname, "../node_modules/leaflet/package.json"); -const leafletPkg = JSON.parse(fs.readFileSync(leafletPkgPath, "utf8")); -const LEAFLET_VERSION = leafletPkg.version; - -function getIntegrity(filePath) { - const fullPath = path.resolve(__dirname, "../node_modules/leaflet/dist", filePath); - const content = fs.readFileSync(fullPath); - const hash = crypto.createHash("sha512").update(content).digest("base64"); - return `sha512-${hash}`; -} - -const LEAFLET_CSS_INTEGRITY = getIntegrity("leaflet.css"); -const LEAFLET_JS_INTEGRITY = getIntegrity("leaflet.js"); +let _leafletMeta; +function getLeafletMeta() { + if (!_leafletMeta) { + const leafletPkgPath = path.resolve(__dirname, "../node_modules/leaflet/package.json"); + const leafletPkg = JSON.parse(fs.readFileSync(leafletPkgPath, "utf8")); + const version = leafletPkg.version; + const integrity = (file) => { + const fullPath = path.resolve(__dirname, "../node_modules/leaflet/dist", file); + const content = fs.readFileSync(fullPath); + const hash = crypto.createHash("sha512").update(content).digest("base64"); + return `sha512-${hash}`; + }; + _leafletMeta = { + version, + cssIntegrity: integrity("leaflet.css"), + jsIntegrity: integrity("leaflet.js"), + }; + } + return _leafletMeta; +}Then use
getLeafletMeta()insidegetLeafletLoaderSnippet().
195-200: Redundantdo-whileloop —String.replacewith/galready replaces all matches.
String.prototype.replace()with a global regex replaces every occurrence in a single call. The loop adds complexity for no benefit.♻️ Simplification
// Remove main leaflet.js script if present const leafletJSRegex = /<script[^>]*leaflet\.js[^>]*>[\s\S]*?<\/script>/gi; - let prev; - do { - prev = html; - html = html.replace(leafletJSRegex, ""); - } while (html !== prev); // remove all occurrences + html = html.replace(leafletJSRegex, "");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webpack-plugins/inject-leaflet-loader.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
webpack-plugins/inject-leaflet-loader.js
🧠 Learnings (6)
📓 Common learnings
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.
📚 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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.js
📚 Learning: 2026-01-20T00:56:47.084Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:47.084Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same pair of nodes), so storing a single link per "source~target" key in nodeLinkIndex is appropriate and won't overwrite any links.
Applied to files:
webpack-plugins/inject-leaflet-loader.js
🪛 Biome (2.3.13)
webpack-plugins/inject-leaflet-loader.js
[error] 165-165: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🔇 Additional comments (1)
webpack-plugins/inject-leaflet-loader.js (1)
43-155: Loader snippet generation is well-structured.The CDN loader correctly handles: Leaflet-already-loaded fast path, integrity-checked resource injection, graceful degradation with a visible error banner on CDN failure, parallel plugin loading with proper coordination, and polling as a safety net. The
initMap()function left open at line 154 (closed by the caller) is a code-generation pattern that works but is somewhat non-obvious — a brief comment at line 154 noting// closed by apply()would help future readers.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@webpack-plugins/inject-leaflet-loader.js`:
- Around line 230-231: The regex used to extract the script type in the inject
logic is incorrect (it uses /type="([^"])"/i which only captures one char), so
typeMatch is always null and scriptType falls back to "text/javascript"; update
the pattern used where match and typeMatch are computed (the code that assigns
typeMatch from match[0]) to use a quantifier that captures one-or-more non-quote
characters (e.g., change to a pattern equivalent to /type="([^"]+)"/i), then
leave the scriptType assignment (scriptType = typeMatch ? typeMatch[1] :
"text/javascript") unchanged so real type attributes like "module" or
"text/javascript" are preserved.
- Around line 9-17: Rename the private variable _leafletMeta to leafletMeta
everywhere (declaration and all references, e.g., in the getLeafletMeta function
and any other uses) to remove the leading underscore and satisfy
no-underscore-dangle, and replace the property access of leafletPkg.version with
destructuring: const { version } = leafletPkg; ensuring any subsequent uses
reference the new version variable and the renamed leafletMeta.
🧹 Nitpick comments (1)
webpack-plugins/inject-leaflet-loader.js (1)
206-211: Thedo…whileloop is redundant with ag-flag regex.
String.prototype.replacewith a global (g) regex already replaces all matches in a single call. The loop will always exit after one iteration.♻️ Simplification
// Remove main leaflet.js script if present const leafletJSRegex = /<script[^>]*leaflet\.js[^>]*>[\s\S]*?<\/script>/gi; - let prev; - do { - prev = html; - html = html.replace(leafletJSRegex, ""); - } while (html !== prev); // remove all occurrences + html = html.replace(leafletJSRegex, "");
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
webpack-plugins/inject-leaflet-loader.js
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{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:
webpack-plugins/inject-leaflet-loader.js
🧠 Learnings (6)
📓 Common learnings
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.
📚 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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.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:
webpack-plugins/inject-leaflet-loader.js
📚 Learning: 2026-01-20T00:56:47.084Z
Learnt from: dee077
Repo: openwisp/netjsongraph.js PR: 417
File: src/js/netjsongraph.core.js:132-145
Timestamp: 2026-01-20T00:56:47.084Z
Learning: In the netjsongraph.js codebase, the data model does not support parallel links (multiple links between the same pair of nodes), so storing a single link per "source~target" key in nodeLinkIndex is appropriate and won't overwrite any links.
Applied to files:
webpack-plugins/inject-leaflet-loader.js
🧬 Code graph analysis (1)
webpack-plugins/inject-leaflet-loader.js (1)
webpack.config.js (3)
fs(2-2)InjectLeafletLoaderPlugin(10-10)HtmlWebpackPlugin(6-6)
🪛 Biome (2.3.13)
webpack-plugins/inject-leaflet-loader.js
[error] 176-176: Do not shadow the global "constructor" property.
Consider renaming this variable. It's easy to confuse the origin of variables when they're named after a known global.
(lint/suspicious/noShadowRestrictedNames)
🪛 ESLint
webpack-plugins/inject-leaflet-loader.js
[error] 9-9: Unexpected dangling '_' in '_leafletMeta'.
(no-underscore-dangle)
[error] 17-17: Use object destructuring.
(prefer-destructuring)
🪛 GitHub Actions: netjsongraph.js CI BUILD
webpack-plugins/inject-leaflet-loader.js
[error] 9-9: ESLint: Unexpected dangling '_' in '_leafletMeta' (no-underscore-dangle).
🪛 GitHub Check: QA-Checks
webpack-plugins/inject-leaflet-loader.js
[failure] 17-17:
Use object destructuring
[failure] 9-9:
Unexpected dangling '_' in '_leafletMeta'
🔇 Additional comments (1)
webpack-plugins/inject-leaflet-loader.js (1)
53-165: Loader snippet structure looks solid overall.The CDN-loading IIFE, SRI integrity attributes from local hashes, plugin synchronization pattern, error banners, and graceful degradation are well thought out. The
waitForLeafletpolling is defensive but reasonable for robustness.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
force-pushed
the
fix/issue-392
branch
from
7ff15ec to
f5b6f89
Compare
|
@coderabbitai Your suggestion:
Triggers CodeQL: Incomplete multi-character sanitization This string may still contain |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 880 🏁 Script executed: Length of output: 922 🏁 Script executed: Length of output: 405
Why CodeQL flagged it: Regex-based HTML sanitization is inherently vulnerable to nested or overlapping tags. For example:
The do...while loop was attempting to handle this by replacing iteratively, but this pattern is still incomplete and can be bypassed with clever nesting. Recommended approach:
That said, in this specific context (a webpack plugin processing HTML it generated during build), the risk is somewhat mitigated since you control the input. However, CodeQL is correctly warning about a pattern that's generally unsafe. I apologize for the flawed suggestion. Security should always take precedence over minor code simplifications. 🐰💭 🧠 Learnings used |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@codesankalp good for me! I did some changes to make @coderabbitai happy, I think is good enough now, can you double check the changes do not break anything? I checked on my end but it's better if you double check before we merge.
|
@coderabbitai is there any other issue which was not addressed yet? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 278234
Proposal:
Illustrative change (using node-html-parser, but cheerio works too):
// Remove hardcoded Leaflet assets via structured DOM removal (no regex sanitization) // Remove leaflet.css // Remove main leaflet.js // For plugin page, remove Leaflet plugin assets we load dynamically html = root.toString(); Why this helps
If you prefer zero new deps, we can alternatively switch to HtmlWebpackPlugin’s tag hooks (alterAssetTags/alterAssetTagGroups) and filter out the script/link tag objects before templating, which also avoids regex and satisfies CodeQL. I can draft that version as well. Would you like me to proceed with the parser-based change (and add the devDependency), or should I wire this into HtmlWebpackPlugin’s tag hooks instead? 🧠 Learnings used |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 922 🏁 Script executed: Length of output: 221905 🏁 Script executed: Length of output: 1049 🏁 Script executed: Length of output: 2852
✅ Resolved Issues
|
|
@coderabbitai let's not waste more time on little details that are not going to have any real impact on the project and let's focus on critical matters that bring real benefits to our users, as long as there's no real issues being introduced, minor things which require significant more effort for no noticeable benefits can be skipped. |
|
Tip For best results, initiate chat on the files or code changes.
Status: No critical issues blocking this PR. ✅ The HTML sanitization pattern and minor linting items have no real-world impact in this build-time context. The PR is ready to merge from a functional and quality perspective. I'll focus future reviews on issues that materially impact users, performance, security, or maintainability—not perfectionist code style tweaks. Thank you for the clear guidance! ✏️ Learnings added
🧠 Learnings used |
github-project-automation
Bot
moved this from In progress
to Done
in OpenWISP Priorities for next releases
Checklist
Reference to Existing Issue
Closes #392
Description of Changes
Use chunk splitting, dynamic loading and removed unused examples to reduce library size.
#425 (comment)