Repository navigation
Feat: HTML Report Design Updates - #10
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
📝 WalkthroughWalkthroughReplaced the inline header SVG and added an inline-SVG favicon; extended dependency metadata with Changes
Sequence Diagram(s)sequenceDiagram
rect rgba(200,200,255,0.5)
participant CLI
end
rect rgba(200,255,200,0.5)
participant Collectors as "Per-package Collectors"
end
rect rgba(255,200,200,0.5)
participant Aggregator
end
rect rgba(255,255,200,0.5)
participant ReportUI as "Report UI"
end
CLI->>Collectors: detect workspaces (detectYarnPnP) & run collectors (audit/outdated/graph)
Collectors-->>Aggregator: persist per-package raw outputs
Aggregator->>Aggregator: merge deps (incl. peers), compute packageStats (fileCount/hasBin), determineIntroduction(scope)
Aggregator-->>ReportUI: emit aggregated model
ReportUI->>ReportUI: resolve dep keys (keysByName), apply filters/forced visibility, render links and scroll to details
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 4
🤖 Fix all issues with AI agents
In `@report-ui/main.ts`:
- Around line 263-268: The anchor generation uses getDepDomId(pkg) but
resolveDepKey returns null for name-only dep keys (strings without “@”), making
the link a no-op; update the code paths that build package links (the snippet
using escapeHtml(getDepDomId(pkg)) and the other occurrences around the
getDepDomId/resolveDepKey usage) to detect when resolveDepKey(pkg) is null and,
in that case, try resolving by name: call resolveDepKeyByName(pkgName) or filter
the dependency index for a single candidate and use that resolved key to compute
getDepDomId(resolvedKey); if multiple candidates exist keep the non-link
behavior, and ensure aria-label and data-dep-key are updated to use the resolved
key when present (apply same fix for the other occurrences noted).
In `@src/aggregator.ts`:
- Around line 865-882: The current regexes in isTestFile, isToolingFile, and
isBuildFile miss dotfile-style configs (e.g. .eslintrc, .prettierrc, .babelrc)
so those files are misclassified; update each function's regex to include an
alternative branch that matches leading-dot config filenames (e.g.
/(^|\/)\.(eslint|prettier|babel|stylelint|commitlint|husky|renovate|swc|tsconfig|postcss|tailwind|storybook|babel|eslintrc|prettierrc|babelrc)(rc|config)?(\..*)?$/
or equivalent) so files like .eslintrc, .prettierrc.json, .babelrc.js, and
.tsconfig (dot-prefixed or extensionless) are correctly detected by
isToolingFile and isBuildFile (and add any test-tool dotfiles to isTestFile as
needed).
In `@test-fixtures/execution-signals/packages/scripted/package.json`:
- Around line 6-8: The "install" script under scripts.install currently uses the
ambiguous `&& ||` pattern with scripts/install.js; replace that with an explicit
retry behavior: either change the package.json entry for scripts.install to run
scripts/install.js once and then retry on failure using a clear OR-based
sequence (script A || script A) or, preferably, create a small wrapper (e.g.,
scripts/install-with-retries.js) that attempts scripts/install.js N times and
call that from scripts.install; update references to scripts/install.js in
package.json accordingly so intent is unambiguous.
In `@test-fixtures/execution-signals/packages/scripted/scripts/install.js`:
- Around line 8-10: Replace the hard-coded dead branch and global eval by gating
these calls behind an environment flag and explicitly suppressing the
global-eval lint rule; specifically, change the constant-conditional block that
contains cp.execSync and eval so it runs only when a guard like
process.env.RUN_EXECUTION_SIGNALS === 'true' is set, and add an inline lint
suppression for the eval call (e.g., a comment to disable
lint/security/noGlobalEval) or remove the eval entirely if not required; update
the block referencing the if (false) conditional, cp.execSync, and eval to
implement this guard and suppression.
🧹 Nitpick comments (4)
src/types.ts (1)
97-100: Document presence-only semantics or switch tobooleanfor clarity.
Line 100 typeshasBinastrue(literal-true only), which communicates a presence-only flag: hasBin present means the package has a bin, and hasBin absent means we're not asserting it. This design is intentional (see line 233 in aggregator.ts where onlytrueis set), but the unconventional literal-true type may confuse maintainers. Either add a clarifying comment thathasBinpresence indicatestrueand absence means unknown, or switch tobooleanfor conventional clarity.package.json (1)
27-43: Align fixture scan output extensions for consistency.The new scans write
.jsonwhile existing fixture scans still output.htmlwith--json, sofixtures:scannow mixes extensions. Consider standardizing on one convention to keep tooling predictable.README.md (1)
172-178: Reduce repeated “Yarn” phrasing in support list.Three consecutive bullets start with “Yarn”; a grouped bullet reads cleaner.
✍️ Possible rewrite
-- Yarn Classic (v1, node_modules linker): Supported for dependency tree, audit, outdated, and workspaces. -- Yarn Berry (v2+, node-modules linker): Dependency tree and audit work; outdated support depends on available Yarn commands/plugins and may be unavailable. -- Yarn Plug'n'Play (`nodeLinker: pnp`): Not supported yet. +- Yarn: + - Classic (v1, node_modules linker): Supported for dependency tree, audit, outdated, and workspaces. + - Berry (v2+, node-modules linker): Dependency tree and audit work; outdated support depends on available Yarn commands/plugins and may be unavailable. + - Plug'n'Play (`nodeLinker: pnp`): Not supported yet.test-fixtures/usage-classification/src/tests/app.test.ts (1)
2-3: Duplicate import oftransitive-mixed.The module
transitive-mixedis imported twice. If this is intentional (e.g., to test deduplication or import counting behavior), consider adding a comment explaining the purpose. Otherwise, remove the duplicate.🔧 If unintentional, remove the duplicate
import 'transitive-testing'; import 'transitive-mixed'; -import 'transitive-mixed';
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/aggregator.ts`:
- Around line 1039-1050: determineIntroduction currently classifies any
peer-scoped dependency as 'tooling'; change the logic so only dev scope is
always 'tooling' and peer scope is treated as tooling only when its
runtimeImpact is not 'runtime' (e.g., runtimeImpact === 'tooling' or 'testing'
or undefined), e.g. replace the condition "if (scope === 'dev' || scope ===
'peer') return 'tooling';" with separate checks: "if (scope === 'dev') return
'tooling';" and then "if (scope === 'peer' && runtimeImpact !== 'runtime')
return 'tooling';" so peer deps with runtimeImpact === 'runtime' are not
misclassified.
🧹 Nitpick comments (2)
report-ui/main.ts (1)
266-285: Consider caching name-to-key mappings for better performance.
resolveDepKeyByNameFromSetiterates and parses the entirelinkableKeysset on each call. For reports with many dependencies, this could become noticeable when rendering large dependency lists.You already maintain
depKeysByNamein theinitscope. Consider passing it (or a lookup function) to these resolution helpers, or memoizing results.♻️ Example approach using a pre-built map
-function resolveDepKeyByNameFromSet(name: string, linkableKeys: Set<string>): string | null { - const candidates = Array.from(linkableKeys).filter((candidate) => { - const parsed = parseDepKey(candidate); - return parsed?.name === name; - }); - if (candidates.length === 1) return candidates[0]; - return null; -} +function resolveDepKeyByNameFromSet( + name: string, + linkableKeys: Set<string>, + keysByName?: Map<string, string[]> +): string | null { + if (keysByName) { + const candidates = keysByName.get(name)?.filter(k => linkableKeys.has(k)) || []; + return candidates.length === 1 ? candidates[0] : null; + } + // Fallback to iteration if no map provided + const candidates = Array.from(linkableKeys).filter((candidate) => { + const parsed = parseDepKey(candidate); + return parsed?.name === name; + }); + return candidates.length === 1 ? candidates[0] : null; +}src/aggregator.ts (1)
754-775: Optional: avoid re-classifying file categories during runtime impact weighting.
Entries already carrycategory, so you can reuse it instead of re-running regex classification.♻️ Suggested tweak
-function determineRuntimeImpactFromFiles( - files: Array<{ file: string; count: number }> -): DependencyRecord['usage']['runtimeImpact'] { +function determineRuntimeImpactFromFiles( + files: Array<{ file: string; count: number; category?: RuntimeCategory }> +): DependencyRecord['usage']['runtimeImpact'] { const weights: Record<RuntimeCategory, number> = { runtime: 0, build: 0, testing: 0, tooling: 0 }; let total = 0; for (const entry of files) { - const category = classifyFileCategory(entry.file); + const category = entry.category ?? classifyFileCategory(entry.file); const weight = Number.isFinite(entry.count) && entry.count > 0 ? entry.count : 1; weights[category] += weight; total += weight; }Also applies to: 889-935
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@report-ui/main.ts`:
- Around line 257-309: parseDepKey incorrectly splits scoped npm-alias keys;
update parseDepKey to first look for the last occurrence of the substring
"@npm:" (use lastIndexOf('@npm:')) and, if found, split at that '@' so
parsed.version begins with "npm:..."; otherwise fall back to the existing last-@
split. Also adjust resolveDepLinkTarget's npm alias construction (the branch
using parsed.version.startsWith('npm:')) to avoid producing a double '@' when
the aliased target starts with '@' — concatenate parsed.name and the aliased
target trimmed of its 'npm:' prefix, inserting an '@' only when that trimmed
target does not already start with '@'. Ensure changes touch parseDepKey and
resolveDepLinkTarget accordingly.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@report-ui/main.ts`:
- Around line 704-709: The UI renders two consecutive items with the same label
"Direct parents" — one via renderKvItemHtml('Direct parents',
renderDependencyIdList(..., dep.usage.origins.topParentPackages, ...)) and the
other via renderKvItem('Direct parents', dep.usage.origins.parentPackageCount ??
0); change the second item's label to something distinct (e.g., 'Parent count'
or 'Direct parents (count)') so the numeric parentPackageCount is not shown with
the same label; update the call to renderKvItem that references
dep.usage.origins.parentPackageCount accordingly.
🧹 Nitpick comments (1)
README.md (1)
209-211: Consider varying sentence structure for readability.Three successive bullet points begin with "Yarn", which can feel repetitive. A minor rewording could improve flow.
📝 Suggested adjustment
- npm: Supported for dependency tree, audit, outdated, single-package, and workspaces. - pnpm: Supported for dependency tree, audit, outdated, and workspaces (with ls depth fallbacks for large projects). -- Yarn Classic (v1, node_modules linker): Supported for dependency tree, audit, outdated, and workspaces. -- Yarn Berry (v2+, node-modules linker): Dependency tree and audit work; outdated support depends on available Yarn commands/plugins and may be unavailable. -- Yarn Plug'n'Play (`nodeLinker: pnp`): Not supported yet. +- Yarn Classic (v1, node_modules linker): Supported for dependency tree, audit, outdated, and workspaces. +- Yarn Berry (v2+, node-modules linker): Dependency tree and audit work; outdated support depends on available commands/plugins and may be unavailable. +- Plug'n'Play mode (`nodeLinker: pnp`): Not yet supported.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@report-ui/main.ts`:
- Around line 1128-1136: resolveDepKey builds the npm-alias key incorrectly for
scoped packages by inserting an extra '@' (producing '@scope/name@@1.2.3');
update the npmAliasKey construction in resolveDepKey to match the logic in
resolveDepLinkTarget (concatenate parsed.name with
parsed.version.slice('npm:'.length) without adding an extra '@' or otherwise use
the same expression used in resolveDepLinkTarget) so scoped aliases resolve
correctly; keep the check using depByKey.has(npmAliasKey) as-is.
🧹 Nitpick comments (2)
report-ui/main.ts (1)
971-979: Handle npm alias keys when buildingdepKeysByName.Using
lastIndexOf('@')will mis-parse keys containing@npm:(especially scoped aliases), which can break name-based resolution later. Consider reusingparseDepKey/getDepKeysByNameIndexto keep alias handling consistent.♻️ Suggested fix
- const depKeysByName = new Map<string, string[]>(); - depByKey.forEach((_dep, depKey) => { - const lastAt = depKey.lastIndexOf('@'); - if (lastAt <= 0) return; - const name = depKey.slice(0, lastAt); - const keys = depKeysByName.get(name) || []; - keys.push(depKey); - depKeysByName.set(name, keys); - }); + const depKeysByName = getDepKeysByNameIndex(knownDepKeys);src/report-assets.ts (1)
16-16: Rename exported asset constants to camelCase (or document an exception).
JS_CONTENT(andCSS_CONTENTabove) are exported variables; guidelines call forcamelCasenaming. Since this file is generated, consider updating the generator (scripts/build-report.ts) and any import sites, or explicitly documenting an exception for generated asset exports.As per coding guidelines, use
camelCasefor variables and functions.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@report-ui/main.ts`:
- Around line 970-979: The current loop building depKeysByName uses manual
lastIndexOf('@') parsing which misparses keys with `@npm`: and scoped aliases;
replace this with the parse-aware indexer: call
getDepKeysByNameIndex(knownDepKeys) to produce the mapping (or iterate depByKey
and use parseDepKey(depKey) to extract the canonical name) and populate
depKeysByName accordingly so keys like `@npm`:... and scoped packages are indexed
correctly; update usages of knownDepKeys/depKeysByName to use the new map.
Comment on lines
+970
to
+979
| const knownDepKeys = new Set(depByKey.keys()); | ||
| const depKeysByName = new Map<string, string[]>(); | ||
| depByKey.forEach((_dep, depKey) => { | ||
| const lastAt = depKey.lastIndexOf('@'); | ||
| if (lastAt <= 0) return; | ||
| const name = depKey.slice(0, lastAt); | ||
| const keys = depKeysByName.get(name) || []; | ||
| keys.push(depKey); | ||
| depKeysByName.set(name, keys); | ||
| }); |
Contributor
There was a problem hiding this comment.
Use parseDepKey-aware indexing for depKeysByName. The manual lastIndexOf('@') parsing will mis-index dep keys containing @npm: (especially scoped aliases), which can break name-only resolution for those deps. Prefer getDepKeysByNameIndex(knownDepKeys) or parseDepKey here.
🛠️ Suggested fix
- const depKeysByName = new Map<string, string[]>();
- depByKey.forEach((_dep, depKey) => {
- const lastAt = depKey.lastIndexOf('@');
- if (lastAt <= 0) return;
- const name = depKey.slice(0, lastAt);
- const keys = depKeysByName.get(name) || [];
- keys.push(depKey);
- depKeysByName.set(name, keys);
- });
+ const depKeysByName = getDepKeysByNameIndex(knownDepKeys);🤖 Prompt for AI Agents
In `@report-ui/main.ts` around lines 970 - 979, The current loop building
depKeysByName uses manual lastIndexOf('@') parsing which misparses keys with
`@npm`: and scoped aliases; replace this with the parse-aware indexer: call
getDepKeysByNameIndex(knownDepKeys) to produce the mapping (or iterate depByKey
and use parseDepKey(depKey) to extract the canonical name) and populate
depKeysByName accordingly so keys like `@npm`:... and scoped packages are indexed
correctly; update usages of knownDepKeys/depKeysByName to use the new map.
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
Bug Fixes
Documentation
Chores