Closed
Conversation
📝 WalkthroughWalkthroughAdds module-level complexity analysis: Halstead metrics, module Maintainability Index scoring, and aggregation of per-function metrics. Introduces a standalone analyzeModule API, Halstead visitor and utilities, module analysis visitor, updated rule wiring/options, package exports, docs, and comprehensive tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Input as Code Input
participant Parser as oxc-parser
participant AST as AST
participant Walker as estree-walker
participant Visitor as Module Analysis Visitor
participant Halstead as Halstead Counter
participant Complexity as Complexity Calculator
participant Result as ModuleAnalysisResult
Input->>Parser: parseSync(code, filename)
Parser->>AST: return Program AST
AST->>Walker: traverse (attach loc/parent)
Walker->>Visitor: enter/leave node events
Visitor->>Halstead: increment operators/operands per node
Visitor->>Complexity: update cyclomatic/cognitive per-function
Halstead->>Visitor: provide module & function counts on exits
Complexity->>Visitor: aggregate per-function metrics
Visitor->>Result: compute module MI score & decomposition
Result-->>Input: return ModuleAnalysisResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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)
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
🧹 Nitpick comments (1)
src/analyze.ts (1)
73-84: Stub context returns empty string forgetText()— may affect extraction analysis.The
createLibraryContext()stub returnsgetText: () => '', butcontext.sourceCode.getText()is called incombined-visitor.ts(see context snippet 1, line 170). While this shouldn't break module complexity calculation, it may cause extraction analysis or other features relying on source text to silently produce incorrect results.Consider documenting this limitation or storing the actual
codeparameter in the stub.💡 Optional: Include source text in stub context
-function createLibraryContext(): Context { +function createLibraryContext(code: string): Context { return { sourceCode: { - text: '', - getText: () => '', + text: code, + getText: (node?: ESTreeNode) => { + if (!node?.loc) return code; + // Simplified: would need proper offset calculation for node ranges + return code; + }, scopeManager: null, getScope: () => null, },Then update the call site:
const visitor = createModuleAnalysisVisitor( - createLibraryContext(), + createLibraryContext(code),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/analyze.ts` around lines 73 - 84, The stub createLibraryContext() returns getText() as an empty string which can cause extraction/analysis that depends on source text to misbehave; update createLibraryContext to accept and store the real source text (e.g., add a parameter like code or sourceText) and set sourceCode.text and sourceCode.getText to return that provided string, ensuring Context.sourceCode.getText() returns the actual file contents used by combined-visitor.ts and other analyzers; update any call sites that construct the stub to pass the code string or, if not possible, add a comment documenting the limitation and a TODO to wire real text later.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@package.json`:
- Around line 65-77: The package marks oxc-parser and estree-walker as optional
peer deps but src/analyze.ts imports them at module top-level and src/index.ts
re-exports analyzeModule, causing MODULE_NOT_FOUND on any import; fix by
changing analyzeModule in src/analyze.ts to lazily load these optional libs via
dynamic import() inside the function (or helper) so they are only required when
analyzeModule is invoked, update any type/return handling accordingly, and keep
src/index.ts re-exporting analyzeModule unchanged so other imports no longer
trigger missing-module errors.
In `@README.md`:
- Around line 187-193: The fenced code block that starts with "Module is too
complex (score: 81.5/100, maximum: 80)." is missing a language token and
triggers markdownlint MD040; fix it by adding a language identifier (e.g., text)
after the opening fence (change ``` to ```text) so the block is explicitly
marked (the block containing "Estimated bug risk: ~2.3 defects..." through
"cognitive total: 52, maximum: 40.").
In `@src/rules/complexity.ts`:
- Line 45: The guard currently treats opts.moduleComplexity == 0 as "disabled"
which prevents reporting when a user explicitly sets 0; change the early-return
to only disable for negative values (i.e., treat negative thresholds as
disabled) so the check becomes opts.moduleComplexity < 0 ||
result.moduleComplexity <= opts.moduleComplexity, keeping the existing
comparison against result.moduleComplexity so a configured 0 will still trigger
violations when result.moduleComplexity > 0; update the condition in the same
location where opts.moduleComplexity and result.moduleComplexity are used.
---
Nitpick comments:
In `@src/analyze.ts`:
- Around line 73-84: The stub createLibraryContext() returns getText() as an
empty string which can cause extraction/analysis that depends on source text to
misbehave; update createLibraryContext to accept and store the real source text
(e.g., add a parameter like code or sourceText) and set sourceCode.text and
sourceCode.getText to return that provided string, ensuring
Context.sourceCode.getText() returns the actual file contents used by
combined-visitor.ts and other analyzers; update any call sites that construct
the stub to pass the code string or, if not possible, add a comment documenting
the limitation and a TODO to wire real text later.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15e213e3-7530-424a-a92c-51e7e8f223b3
📒 Files selected for processing (15)
.oxlintrc.jsonREADME.mdpackage.jsonsrc/analyze.tssrc/index.tssrc/module/halstead-visitor.tssrc/module/halstead.tssrc/module/visitor.tssrc/rules/complexity.tssrc/rules/shared.tstests/analyze.test.tstests/fixtures/js/module-level.jstests/halstead.test.tstests/module-analysis.test.tstests/module-complexity.test.ts
Comment on lines
65
to
77
| "peerDependencies": { | ||
| "@oxlint/plugins": ">=1.43.0" | ||
| "@oxlint/plugins": ">=1.43.0", | ||
| "oxc-parser": ">=0.60.0", | ||
| "estree-walker": ">=3.0.0" | ||
| }, | ||
| "peerDependenciesMeta": { | ||
| "oxc-parser": { | ||
| "optional": true | ||
| }, | ||
| "estree-walker": { | ||
| "optional": true | ||
| } | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
echo "=== Checking package.json peerDependencies ==="
sed -n '65,77p' package.json
echo ""
echo "=== Checking src/analyze.ts imports (first 15 lines) ==="
head -15 src/analyze.ts
echo ""
echo "=== Checking for all imports of oxc-parser ==="
rg "from ['\"]oxc-parser['\"]|import.*oxc-parser" src/ -A 2 -B 2
echo ""
echo "=== Checking for all imports of estree-walker ==="
rg "from ['\"]estree-walker['\"]|import.*estree-walker" src/ -A 2 -B 2Repository: itaymendel/oxlint-plugin-complexity
Length of output: 1531
🏁 Script executed:
#!/bin/bash
echo "=== Checking package.json main/exports field ==="
rg '"main"|"exports"' package.json -A 5
echo ""
echo "=== Checking how analyzeModule is exported ==="
rg "export.*analyzeModule|function analyzeModule" src/ -B 2 -A 5
echo ""
echo "=== Checking where analyzeModule is defined ==="
grep -n "analyzeModule" src/analyze.ts | head -5
echo ""
echo "=== Checking if there are any try-catch or dynamic imports ==="
rg "try\s*\{|catch|dynamic.*import|import\(" src/analyze.tsRepository: itaymendel/oxlint-plugin-complexity
Length of output: 1237
🏁 Script executed:
#!/bin/bash
echo "=== Checking module/visitor.js for oxc-parser/estree-walker deps ==="
rg "from ['\"]oxc-parser['\"]|import.*oxc-parser|from ['\"]estree-walker['\"]|import.*estree-walker" src/module/visitor.ts
echo ""
echo "=== Checking all imports in module/visitor.ts (first 20 lines) ==="
head -20 src/module/visitor.ts
echo ""
echo "=== Summary: Does main entry point require optional deps? ==="
echo "Main entry exports from: src/index.ts"
echo "Exports from src/index.ts:"
grep "export" src/index.tsRepository: itaymendel/oxlint-plugin-complexity
Length of output: 1772
Optional peer dependencies will cause import-time failure if not installed.
oxc-parser and estree-walker are marked as optional peerDependencies, but src/analyze.ts imports them unconditionally at the top level. Since src/index.ts re-exports analyzeModule from src/analyze.ts, importing from the main package entry point will trigger a MODULE_NOT_FOUND error if these optional dependencies are not installed, even when accessing other exports.
Consider one of:
- Use dynamic
import()inanalyzeModuleso the dependency is only required when the function is called - Move
analyzeModuleto a separate entry point (e.g.,oxlint-plugin-complexity/analyze) to avoid loading optional deps from the main entry - Remove
optional: trueif these dependencies are required for core functionality
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@package.json` around lines 65 - 77, The package marks oxc-parser and
estree-walker as optional peer deps but src/analyze.ts imports them at module
top-level and src/index.ts re-exports analyzeModule, causing MODULE_NOT_FOUND on
any import; fix by changing analyzeModule in src/analyze.ts to lazily load these
optional libs via dynamic import() inside the function (or helper) so they are
only required when analyzeModule is invoked, update any type/return handling
accordingly, and keep src/index.ts re-exporting analyzeModule unchanged so other
imports no longer trigger missing-module errors.
| result: ModuleAnalysisResult, | ||
| opts: ParsedModuleOptions | ||
| ): string[] { | ||
| if (opts.moduleComplexity <= 0 || result.moduleComplexity <= opts.moduleComplexity) return []; |
There was a problem hiding this comment.
moduleComplexity: 0 currently disables score violations instead of enforcing the strictest threshold.
Line 45 returns early when opts.moduleComplexity <= 0, so a configured value of 0 can never trigger the module complexity score report even when result.moduleComplexity > 0.
Suggested fix
- if (opts.moduleComplexity <= 0 || result.moduleComplexity <= opts.moduleComplexity) return [];
+ if (result.moduleComplexity <= opts.moduleComplexity) return [];📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (opts.moduleComplexity <= 0 || result.moduleComplexity <= opts.moduleComplexity) return []; | |
| if (result.moduleComplexity <= opts.moduleComplexity) return []; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/rules/complexity.ts` at line 45, The guard currently treats
opts.moduleComplexity == 0 as "disabled" which prevents reporting when a user
explicitly sets 0; change the early-return to only disable for negative values
(i.e., treat negative thresholds as disabled) so the check becomes
opts.moduleComplexity < 0 || result.moduleComplexity <= opts.moduleComplexity,
keeping the existing comparison against result.moduleComplexity so a configured
0 will still trigger violations when result.moduleComplexity > 0; update the
condition in the same location where opts.moduleComplexity and
result.moduleComplexity are used.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/analyze.ts (1)
73-84: Consider documenting the minimal context limitations.The
createLibraryContextprovides a stub implementation wheregetText()always returns the full source code rather than a node's text, andscopeManager/getScopereturn null. This works for the current module analysis visitor but may cause issues if visitors are extended to rely on node-specific text extraction or scope analysis.Consider adding a brief comment documenting these limitations for future maintainers.
💡 Optional: Add documentation comment
+/** + * Creates a minimal Context for standalone analysis (no linting context). + * Limitations: getText() returns full source, scopeManager is null. + */ function createLibraryContext(code: string): Context { return { sourceCode: { text: code, getText: () => code, scopeManager: null, getScope: () => null, }, options: [], report: () => {}, } as unknown as Context; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/analyze.ts` around lines 73 - 84, Add a short documentation comment above the createLibraryContext function explaining its minimal/stub behavior: that sourceCode.getText() always returns the whole file (not node-specific text), scopeManager is null and getScope() returns null, and that this is sufficient only for current module analysis visitors but will break visitors that require node text or scope analysis; mention callers should replace with a full ESLint Context when needed (reference createLibraryContext and the Context type).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/analyze.ts`:
- Around line 73-84: Add a short documentation comment above the
createLibraryContext function explaining its minimal/stub behavior: that
sourceCode.getText() always returns the whole file (not node-specific text),
scopeManager is null and getScope() returns null, and that this is sufficient
only for current module analysis visitors but will break visitors that require
node text or scope analysis; mention callers should replace with a full ESLint
Context when needed (reference createLibraryContext and the Context type).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: bf4acafa-3871-4e3d-af65-d250658b80ba
📒 Files selected for processing (4)
README.mdpackage.jsonsrc/analyze.tssrc/index.ts
Owner
Author
|
not shipping. module level stuff are redundant. don't add anything valuable over function level complexity |
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
1 participant
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 by CodeRabbit
New Features
Configuration
Documentation
Tests