Repository navigation
Feat: Fix more PNPM Workspace Issues and Add Tests - #11
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughAdds PNPM-aware install-state checks and local node_modules crawling, centralizes fixture orchestration and Vitest-based tests, introduces unit tests for aggregation/license/npmLs/utils, updates report UI and dynamic CTA handling, and expands README with PNPM workspace crawling and hardening details. Changes
Sequence Diagram(s)sequenceDiagram
participant CLI as "CLI (dist/cli.js)"
participant Runner as "npmLs runner"
participant FS as "Filesystem (node_modules, .pnpm)"
participant Store as "PNPM virtual store"
participant Aggregator as "Aggregator / Normalizer"
CLI->>Runner: run scan --project /path --json
Runner->>FS: createPnpmInstallState(projectPath)
FS->>Store: read .pnpm virtual store entries & node_modules roots
Store-->>FS: store index & roots
FS-->>Runner: installState (store index, roots, caches)
Runner->>Runner: isPnpmPackageInstalled(name, version, installState)
alt package installed via virtual store
Runner->>Store: lookup store entry
else package linked / workspace
Runner->>FS: check node_modules/workspace links
end
Runner->>Aggregator: normalized dependency nodes (installed-only filter applied)
Aggregator->>FS: read local package.json & license artifacts
Aggregator-->>CLI: write dependency-radar.json / dependency-radar.html
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/aggregator.test.ts`:
- Around line 31-37: The test is passing an unrecognized field packageJson to
aggregateData so the override is ignored; change the call to pass the correct
property name projectPackageJson (or use pkgOverride if you only mean to
override dependencies) so AggregateInput receives the intended override. Update
the aggregateData invocation in the test to replace packageJson with
projectPackageJson (or use pkgOverride for dependency-only overrides) so the
test uses the provided metadata instead of the on-disk package.json.
In `@src/runners/npmLs.test.ts`:
- Around line 36-57: The helper buildPnpmTreePayload currently relies on type
inference; add an explicit TypeScript return type for buildPnpmTreePayload
(e.g., a small typed payload/interface or a precise inline type describing the
outer array and nested dependencies objects) and update its signature to include
that return type and keep 2-space indentation, single quotes and semicolons;
ensure the declared type covers the nested dependency shape (name, version,
optional dependencies) so the test compiles under strict mode.
In `@test-fixtures/scripts/assert-suite.js`:
- Around line 148-153: The predicate in hasOutdated uses an invalid status check
(status === 'outdated') against targetDeps' outdatedStatus; update the condition
to treat documented update statuses as outdated by checking that status is one
of 'patch', 'minor' or 'major' (e.g.,
['patch','minor','major'].includes(status)) so that entries with no
latestVersion but with an update status are correctly flagged; modify the logic
in the hasOutdated computation (the arrow function that inspects
entry.package.version, entry.upgrade.latestVersion, and
entry.upgrade.outdatedStatus) to use that status membership check.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test-fixtures/scripts/assert-suite.js (1)
23-40: Consider guarding JSON parse errors to preserve aggregated failures.A malformed report will currently throw and bypass the consolidated failure summary. Capturing parse failures keeps the suite behavior consistent with other assertions.
Suggested change
function loadReport(fixtureName) { const reportPath = path.join(fixturesRoot, fixtureName, 'dependency-radar.json'); if (!fs.existsSync(reportPath)) { failures.push(`[${fixtureName}] missing report: ${reportPath}`); return null; } - return JSON.parse(fs.readFileSync(reportPath, 'utf8')); + try { + return JSON.parse(fs.readFileSync(reportPath, 'utf8')); + } catch (error) { + failures.push(`[${fixtureName}] failed to parse report: ${reportPath} (${error.message})`); + return null; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test-fixtures/scripts/assert-suite.js` around lines 23 - 40, The loadReport function can throw if JSON.parse fails, which aborts the test run; wrap the file read/parse in a try/catch so malformed JSON doesn't throw but instead pushes a descriptive failure into the failures array (include fixtureName and the parse error message) and return null; update loadReport to catch errors from JSON.parse (and fs.readFileSync) and push e.g. `[${fixtureName}] invalid report: ${error.message}` to failures to preserve aggregated failures.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test-fixtures/scripts/assert-suite.js`:
- Around line 23-40: The loadReport function can throw if JSON.parse fails,
which aborts the test run; wrap the file read/parse in a try/catch so malformed
JSON doesn't throw but instead pushes a descriptive failure into the failures
array (include fixtureName and the parse error message) and return null; update
loadReport to catch errors from JSON.parse (and fs.readFileSync) and push e.g.
`[${fixtureName}] invalid report: ${error.message}` to failures to preserve
aggregated failures.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/report.ts (1)
6-6: CTA URL constant and builder are duplicated across files.
CTA_BASE_URLis defined identically in bothsrc/report.ts(line 6) andreport-ui/main.ts(line 15). The URL construction logic is also duplicated inreport-ui/main.ts(lines 1582-1584).Consider extracting this constant and the URL building logic into a shared module to maintain a single source of truth.
Also applies to: 14-19
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/report.ts` at line 6, Extract the duplicated CTA_BASE_URL constant and the URL-construction logic into a new shared module (e.g., export const CTA_BASE_URL and export function buildCtaUrl(...)) and replace the duplicated declarations in src/report.ts and report-ui/main.ts by importing those exports; update references to CTA_BASE_URL and the inline builder in both places to use the shared buildCtaUrl function so there is a single source of truth for the CTA URL logic.
🤖 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/report.ts`:
- Line 6: Extract the duplicated CTA_BASE_URL constant and the URL-construction
logic into a new shared module (e.g., export const CTA_BASE_URL and export
function buildCtaUrl(...)) and replace the duplicated declarations in
src/report.ts and report-ui/main.ts by importing those exports; update
references to CTA_BASE_URL and the inline builder in both places to use the
shared buildCtaUrl function so there is a single source of truth for the CTA URL
logic.
This was referenced Mar 1, 2026
Merged
Merged
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
Documentation
Tests