[fix] Remove unused dependencies, add check to run-qa-checks #484#485
[fix] Remove unused dependencies, add check to run-qa-checks #484#485nemesifier merged 5 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR introduces an automated unused dependency detection system to the CI/CD pipeline using the Knip tool. It adds a Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (1)📓 Common learnings🔇 Additional comments (1)
✏️ Tip: You can disable this entire section by setting Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
package.json (1)
13-21: Remove orphanedlint-stagedconfiguration andprecommitscript.
huskyhas been removed fromdevDependenciesand there is no.huskydirectory, so thelint-stagedconfiguration (lines 18-21) andprecommitscript (line 13) will not execute. Either:
- Re-add
huskyand configure.husky/pre-committo runlint-staged, or- Remove the
lint-stagedconfiguration block and theprecommitscript entry.
🤖 Fix all issues with AI agents
In `@package.json`:
- Around line 81-84: Update package.json by removing "knip" and "typescript"
from the top-level "dependencies" object and add them to the "devDependencies"
object instead; ensure the exact package names "knip" and "typescript" remain
with their versions (e.g., "^5.81.0" and "^5.9.3") and run npm/yarn install to
verify lockfile updates.
In `@run-qa-checks`:
- Around line 15-16: Remove the trailing whitespace at the end of the "node
scripts/ci-knip.js" command in the run-qa-checks script so the line ends exactly
with "node scripts/ci-knip.js" (no extra spaces); update the file to strip that
trailing space and commit the change.
In `@scripts/ci-knip.js`:
- Around line 49-61: The Array.from(...).sort() calls are sorting dependency
objects by string coercion instead of their name property; update both
Array.from(issues.dependencies).sort() and
Array.from(issues.devDependencies).sort() to use a comparator that compares the
objects' name properties (e.g. (a,b) => a.name.localeCompare(b.name) with a safe
fallback if name is missing) so the entries are sorted by dependency name before
forEach logging.
🧹 Nitpick comments (1)
scripts/ci-knip.js (1)
100-114: Consider logging stderr for debugging non-JSON failures.The error handling correctly attempts to parse JSON from
error.stdoutwhen knip finds issues (exits non-zero). However, when JSON parsing fails, additional context fromerror.stderrcould help diagnose the root cause.Optional enhancement
} catch (parseError) { console.error("❌ Knip failed (could not parse JSON output):"); console.error(error.message); + if (error.stderr) { + console.error("stderr:", error.stderr); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
lib/js/echarts-gl.min.jsis excluded by!**/*.min.jsyarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (29)
.github/workflows/ci.yml.nvmrcCHANGELOG.rstexamples/load_data_geo_extent/index.htmlindex.htmlknip.jsonpackage.jsonpublic/example_templates/netjson-clustering.htmlpublic/example_templates/netjson-dateParse.htmlpublic/example_templates/netjson-multipleInterfaces.htmlpublic/example_templates/netjson-searchElements.htmlpublic/example_templates/netjson-switchGraphMode.htmlpublic/example_templates/netjson-switchRenderMode.htmlpublic/example_templates/netjsongraph-graphGL.htmlpublic/example_templates/netjsongraph-multipleLinks.htmlpublic/example_templates/netjsongraph-nodeExpand.htmlpublic/example_templates/netjsongraph-wifi-clients.htmlpublic/example_templates/netjsongraph.htmlpublic/example_templates/netjsonmap-animation.htmlpublic/example_templates/netjsonmap-appendData.htmlpublic/example_templates/netjsonmap-appendData2.htmlpublic/example_templates/netjsonmap-indoormap.htmlpublic/example_templates/netjsonmap-multipleTiles.htmlpublic/example_templates/netjsonmap-nodeTiles.htmlpublic/example_templates/netjsonmap-plugins.htmlpublic/example_templates/netjsonmap.htmlpublic/example_templates/njg-geojson.htmlrun-qa-checksscripts/ci-knip.js
🧰 Additional context used
🧠 Learnings (1)
📚 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:
public/example_templates/netjson-switchRenderMode.htmlpublic/example_templates/netjsonmap-multipleTiles.htmlpublic/example_templates/netjson-searchElements.htmlpublic/example_templates/netjsongraph-nodeExpand.htmlpublic/example_templates/netjson-switchGraphMode.htmlpublic/example_templates/netjson-multipleInterfaces.htmlpublic/example_templates/netjsonmap-nodeTiles.htmlpublic/example_templates/netjsongraph-multipleLinks.htmlindex.htmlpublic/example_templates/netjson-dateParse.htmlpublic/example_templates/netjsongraph-graphGL.htmlpublic/example_templates/netjsonmap-appendData.htmlpublic/example_templates/netjsonmap.htmlpublic/example_templates/netjsonmap-appendData2.htmlpublic/example_templates/netjsongraph.html
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Tests and Coverage
🔇 Additional comments (31)
public/example_templates/netjsonmap-animation.html (1)
1-1: LGTM — Doctype casing normalization.The HTML5 DOCTYPE declaration is case-insensitive, so this lowercase normalization is valid and maintains consistency across template files.
public/example_templates/netjsonmap-plugins.html (1)
1-1: LGTM! DOCTYPE normalization for consistency.The lowercase DOCTYPE declaration is valid HTML5 and aligns with the consistency improvements applied across multiple example templates.
public/example_templates/netjsonmap-appendData2.html (1)
1-1: LGTM!The DOCTYPE normalization to lowercase is valid and contributes to consistency across the HTML templates. Both forms are functionally equivalent in HTML5.
public/example_templates/netjsongraph-nodeExpand.html (1)
1-1: LGTM! Valid HTML5 doctype normalization.The change from uppercase to lowercase doctype declaration is valid—both forms are equivalent in HTML5. This normalization improves consistency across template files.
public/example_templates/netjsongraph.html (1)
1-1: LGTM! Doctype normalization applied.The DOCTYPE declaration has been normalized to lowercase, which is valid HTML5 syntax. This formatting change has no functional impact.
examples/load_data_geo_extent/index.html (1)
1-1: LGTM! Doctype normalization applied.The DOCTYPE declaration has been normalized to lowercase, consistent with the broader formatting updates across HTML templates in this PR.
public/example_templates/netjsonmap-nodeTiles.html (1)
1-1: LGTM! Doctype normalization applied.The DOCTYPE declaration has been normalized to lowercase. This formatting change aligns with the HTML template consistency updates throughout the repository.
public/example_templates/netjson-switchRenderMode.html (1)
1-1: LGTM! Doctype normalization applied.The DOCTYPE declaration has been normalized to lowercase, maintaining consistency with other HTML templates in this PR.
public/example_templates/netjsongraph-graphGL.html (1)
1-1: LGTM! Doctype normalization applied.The DOCTYPE declaration has been normalized to lowercase. This formatting change is consistent with the HTML template normalization across the repository.
public/example_templates/netjson-switchGraphMode.html (1)
1-1: LGTM!The lowercase DOCTYPE declaration is valid per HTML5 spec and aligns with modern conventions. This stylistic normalization is consistent across the template files.
public/example_templates/netjsonmap-indoormap.html (1)
1-1: LGTM!Consistent DOCTYPE normalization matching the other template files.
public/example_templates/netjson-dateParse.html (1)
1-1: LGTM!Consistent DOCTYPE normalization.
public/example_templates/netjson-multipleInterfaces.html (1)
1-1: LGTM!Consistent DOCTYPE normalization.
public/example_templates/netjsonmap-appendData.html (1)
1-1: LGTM!Consistent DOCTYPE normalization completing the standardization across example templates.
public/example_templates/netjsonmap-multipleTiles.html (1)
1-1: LGTM!Lowercase
<!doctype html>is the canonical HTML5 form and functionally equivalent to the uppercase variant.public/example_templates/netjson-searchElements.html (1)
1-1: LGTM!Consistent doctype normalization across templates.
public/example_templates/njg-geojson.html (1)
1-1: LGTM!Consistent doctype normalization.
public/example_templates/netjsonmap.html (1)
1-1: LGTM!Consistent doctype normalization.
public/example_templates/netjsongraph-multipleLinks.html (1)
1-1: LGTM!Consistent doctype normalization.
.nvmrc (1)
1-1: LGTM!Using
lts/krypton(Node.js 22.x) is a valid and recent LTS choice. The.nvmrcfile correctly specifies the Node.js version for tooling and CI..github/workflows/ci.yml (2)
21-25: LGTM!The Node.js setup step correctly references the
.nvmrcfile and is properly placed before the yarn cache retrieval step.
58-62: LGTM!Consistent Node.js setup in the build job, mirroring the qa-checks job configuration.
knip.json (1)
1-6: The knip.json configuration is sufficient as-is. Knip automatically detects entry points from package.json fields (main/bin/exports/scripts), and this project's entry point is already defined via"main": "src/js/netjsongraph.js". Explicitly adding anentryfield would be redundant and contradicts Knip's documented best practice of relying on automatic detection before manual configuration.Likely an incorrect or invalid review comment.
public/example_templates/netjson-clustering.html (1)
73-95: LGTM!The reformatted NetJSONGraph constructor maintains identical functionality while improving readability. The configuration options are well-organized and the commented examples for
nodeCategoriesprovide helpful customization guidance.CHANGELOG.rst (1)
1-97: LGTM!The changelog formatting is now consistent with standard reStructuredText conventions. Section headers use proper decoration characters, and version entries are well-organized with clear feature, changes, and bugfix sections.
index.html (3)
1-1: LGTM!Lowercase
<!doctype html>is the recommended HTML5 standard form.
36-42: LGTM!CSS formatting improvements: multi-line transition declaration and double-quoted font-family value improve readability and maintain consistency.
248-269: LGTM!The theme toggle logic is correct. String quote standardization to double quotes is consistent throughout the file.
scripts/ci-knip.js (2)
3-10: LGTM!Good use of Sets for deduplication of dependencies, devDependencies, unresolved imports, and unused files. The array for
unusedExportsappropriately preserves file context for each export.
89-99: LGTM!The execution flow correctly runs knip with JSON reporter and handles the success path. Using
execSyncwith explicit stdio pipes ensures proper output capture.public/example_templates/netjsongraph-wifi-clients.html (1)
122-129: TheattachClientsOverlaymethod is properly defined and the configuration parameters are valid.The new
onReadyhook correctly integrates the clients overlay plugin. The method is properly bound to the NetJSONGraph instance and accepts all the provided options:radius: 5,gap: 3, andminZoomLevel: 1. These values are all valid and compatible with the function's implementation.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks good, I will test asap.
|
@nemesifier I think you have merged this PR, why has it not been marked as merged, shall I close this PR myself? |
I haven't merged this yet. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@PRoIISHAAN could you please rebase this on the latest master?
nemesifier
added
enhancement
dependencies
github-project-automation
bot
moved this to In progress
in OpenWISP Contributor's Board
|
rebase done |
github-project-automation
bot
moved this from In progress
to Done
in OpenWISP Contributor's Board
|
Thanks @PRoIISHAAN 🙏 |
2 participants
…equired knip files
Checklist
Reference to Existing Issue
Closes #484 .
Description of Changes
Removed unused dependencies
added knip
added knip.json and scripts/ci-knip.js