ci: add ESLint and tsc type checking for JavaScript files#771
Open
cv wants to merge 2 commits intoNVIDIA:mainfrom
Open
ci: add ESLint and tsc type checking for JavaScript files#771cv wants to merge 2 commits intoNVIDIA:mainfrom
cv wants to merge 2 commits intoNVIDIA:mainfrom
Conversation
Add lint and type-check coverage for all JS files outside the nemoclaw/ TypeScript sub-project (bin/, test/, scripts/, docs/_ext/). - eslint.config.mjs: flat config with environment-specific settings (CJS for bin/scripts, ESM for test, browser for docs) - jsconfig.json: tsc --checkJs with bundler resolution, no strict - prek hooks: eslint-js (pre-commit, priority 6) and tsc-check-js (pre-push, priority 10) - Fix 68 existing ESLint violations (empty catches, unused vars, regex spaces, dead imports, useless try/catch) - Fix 11 tsc type errors via JSDoc casts and @ts-expect-error pragmas Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds ESLint and JS type-checking configuration and pre-commit/pre-push hooks; introduces eslint.config.mjs and jsconfig.json; updates package.json scripts and devDependencies; standardizes empty catch blocks with explicit ignored comments; renames unused variables to underscore-prefixed names; small test and import cleanups; minor regex and JSDoc/TS assertion tweaks. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
docs/_ext/search_assets/modules/ResultRenderer.js (1)
132-132: Remove the unused_resultbinding entirely.Renaming avoids lint noise, but the variable is still dead code and adds confusion.
Proposed cleanup
- bindResultEvents(container, results) { - container.querySelectorAll('.search-result-item').forEach((item, index) => { - const _result = results[index]; + bindResultEvents(container) { + container.querySelectorAll('.search-result-item').forEach((item) => {- this.bindResultEvents(container, results); + this.bindResultEvents(container);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/_ext/search_assets/modules/ResultRenderer.js` at line 132, Remove the dead binding "_result" created by "const _result = results[index];" — delete that entire declaration so there is no unused variable; if the intention was to reference the item later, instead use "results[index]" inline where needed or rename the existing usage to a used variable, but do not keep "_result" as an unused binding.bin/lib/nim.js (1)
149-149: Pre-existing dead code:_intervalis never used.The variable is declared but the sleep call on line 164 uses a hardcoded
"5"instead of_interval / 1000. The underscore prefix acknowledges this, but consider either using the variable or removing it entirely to avoid confusion.♻️ Option: Use the interval constant or remove it
- const _interval = 5000; + const intervalSec = 5; const safePort = Number(port); console.log(` Waiting for NIM health on port ${safePort} (timeout: ${timeout}s)...`); while ((Date.now() - start) / 1000 < timeout) { ... // Synchronous sleep via spawnSync - require("child_process").spawnSync("sleep", ["5"]); + require("child_process").spawnSync("sleep", [String(intervalSec)]); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@bin/lib/nim.js` at line 149, The code declares a constant named _interval but never uses it; update the sleep invocation (the call currently using the hardcoded "5") to use _interval by converting milliseconds to seconds (e.g., use _interval / 1000) or remove the unused _interval constant entirely—ensure you update or delete the _interval declaration and replace the hardcoded sleep argument so there is no leftover dead variable.docs/_ext/search_assets/main.js (1)
161-164: Consider removing unused parameters entirely or documenting the interface contract.The
renderResultsmethod ignores both parameters and returns an empty string. If this is a public API placeholder for future implementation, consider adding a JSDoc comment explaining the interface contract. If it's truly dead code, consider removing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/_ext/search_assets/main.js` around lines 161 - 164, The renderResults(_results, _query) method currently ignores its parameters and returns an empty string; either remove the unused parameters and delete the method if it is dead code, or add a JSDoc describing the interface contract and intended behavior (including expected types for results and query and that it returns an HTML string) so callers know it’s a placeholder for future implementation; update any references to SearchPageManager.renderResults to match the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@bin/lib/onboard.js`:
- Around line 540-542: Remove the unused destructured import _mkdtempSync and
the redundant local os require: delete the line that does const { mkdtempSync:
_mkdtempSync } = require("fs") and the line const os = require("os"), and keep
using the module-level fs and os imports so buildCtx =
fs.mkdtempSync(path.join(os.tmpdir(), "nemoclaw-build-")) continues to work
unchanged; verify there are no other local references to _mkdtempSync or the
local os after removal.
In `@docs/_ext/search_assets/modules/SearchPageManager.js`:
- Around line 136-143: The computed variables _audienceOptions and
_difficultyOptions are dead code because their option HTML is never inserted
into the template; either remove them or render dropdowns and wire them into the
filter grid. Fix by updating the render/template method that builds the filter
grid to include select elements for audience and difficulty (using the existing
_audienceOptions and _difficultyOptions), ensure their select IDs/classes match
the event handler selectors referenced in the change handler, and reuse
escapeHtml, formatPersonaName, and formatDifficultyName when populating the
options; alternatively, delete the declarations of _audienceOptions and
_difficultyOptions and remove any related null-checked DOM references in the
event handling code to eliminate the unused variables.
---
Nitpick comments:
In `@bin/lib/nim.js`:
- Line 149: The code declares a constant named _interval but never uses it;
update the sleep invocation (the call currently using the hardcoded "5") to use
_interval by converting milliseconds to seconds (e.g., use _interval / 1000) or
remove the unused _interval constant entirely—ensure you update or delete the
_interval declaration and replace the hardcoded sleep argument so there is no
leftover dead variable.
In `@docs/_ext/search_assets/main.js`:
- Around line 161-164: The renderResults(_results, _query) method currently
ignores its parameters and returns an empty string; either remove the unused
parameters and delete the method if it is dead code, or add a JSDoc describing
the interface contract and intended behavior (including expected types for
results and query and that it returns an HTML string) so callers know it’s a
placeholder for future implementation; update any references to
SearchPageManager.renderResults to match the chosen approach.
In `@docs/_ext/search_assets/modules/ResultRenderer.js`:
- Line 132: Remove the dead binding "_result" created by "const _result =
results[index];" — delete that entire declaration so there is no unused
variable; if the intention was to reference the item later, instead use
"results[index]" inline where needed or rename the existing usage to a used
variable, but do not keep "_result" as an unused binding.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f4add669-0c02-44a5-af3f-8b8142371814
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (21)
.pre-commit-config.yamlbin/lib/credentials.jsbin/lib/local-inference.jsbin/lib/nim.jsbin/lib/onboard.jsbin/lib/policies.jsbin/lib/preflight.jsbin/lib/registry.jsbin/lib/resolve-openshell.jsbin/nemoclaw.jsdocs/_ext/search_assets/main.jsdocs/_ext/search_assets/modules/ResultRenderer.jsdocs/_ext/search_assets/modules/SearchEngine.jsdocs/_ext/search_assets/modules/SearchPageManager.jseslint.config.mjsjsconfig.jsonpackage.jsonscripts/telegram-bridge.jstest/policies.test.jstest/preflight.test.jstest/runner.test.js
💤 Files with no reviewable changes (1)
- test/policies.test.js
- bin/lib/onboard.js: remove redundant fs/os requires (already imported at module level) - docs/_ext/search_assets/modules/SearchPageManager.js: remove unused audienceOptions and difficultyOptions (never wired into template) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
19 tasks
wscurran
added
CI/CD
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.
Summary
nemoclaw/TypeScript sub-project (bin/,test/,scripts/,docs/_ext/) with environment-specific configs (CJS, ESM, browser)tsc --checkJsviajsconfig.jsonfor type checking JS without requiring migration to TypeScriptlint/typechecknpm scriptsTest plan
npx eslint .passes cleannpx tsc -p jsconfig.jsonpasses cleannpx vitest run— all tests pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Documentation
Tests