Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a quadrant comparison chart to the Compare page: a new Vue Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be4b4196-422e-437b-89ff-c11f23cf7b05
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (11)
app/components/Compare/FacetQuadrantChart.vueapp/composables/useChartWatermark.tsapp/pages/compare.vueapp/utils/charts.tsapp/utils/compare-quadrant-chart.tsi18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.jsonpackage.jsontest/nuxt/a11y.spec.tstest/unit/app/utils/compare-quadrant-chart.spec.ts
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/utils/charts.ts (1)
708-714: 🛠️ Refactor suggestion | 🟠 MajorUse the proper config type instead of
any.
copyAltTextForCompareQuadrantChartstill usesanyfor the config type parameter, whilst its siblingcreateAltTextForCompareQuadrantChartcorrectly usesCompareQuadrantChartConfig. This is inconsistent and violates type-safety. As per coding guidelines: "Ensure you write strictly type-safe code".Suggested fix
export async function copyAltTextForCompareQuadrantChart({ dataset, config, -}: AltCopyArgs<VueUiQuadrantDatapoint[], any>) { +}: AltCopyArgs<VueUiQuadrantDatapoint[], CompareQuadrantChartConfig>) { const altText = createAltTextForCompareQuadrantChart({ dataset, config }) await config.copy(altText) }
🧹 Nitpick comments (2)
i18n/locales/en.json (1)
1127-1150: Terminology mismatch between quadrant labels and alt-text descriptions.The quadrant labels use different terminology than the alt-text side analysis:
label_top_left: "promising" vsside_analysis_top_left: "efficient"label_bottom_right: "popular but heavy" vsside_analysis_bottom_right: "resource-heavy"Consider aligning these for consistency so that users encountering the alt-text receive the same terminology as shown on the chart.
Suggested alignment
- "side_analysis_top_left": "The following packages are positioned on the top-left quadrant (efficient): {packages}", + "side_analysis_top_left": "The following packages are positioned on the top-left quadrant (promising): {packages}", - "side_analysis_bottom_right": "The following packages are positioned on the bottom-right quadrant (resource-heavy): {packages}", + "side_analysis_bottom_right": "The following packages are positioned on the bottom-right quadrant (popular but heavy): {packages}",app/components/Compare/FacetQuadrantChart.vue (1)
107-125: Use proper type instead ofanyand consider avoiding spread in map.Line 109 uses
anytype forel, butrawQuadrant.valuereturnsPackageQuadrantPoint[]. Using the proper type improves type safety. Additionally, the static analysis tool flagged the spread operator inmapas inefficient.Suggested fix
+import type { PackageQuadrantPoint } from '~/utils/compare-quadrant-chart' const dataset = computed<VueUiQuadrantDatasetItem[]>(() => { - return rawQuadrant.value.map((el: any) => { - return { - ...el, + return rawQuadrant.value.map((el: PackageQuadrantPoint): VueUiQuadrantDatasetItem => { + const ellipsizedName = applyEllipsis(el.name, 20) + return { + adoptionScore: el.adoptionScore, + efficiencyScore: el.efficiencyScore, + id: el.id, + license: el.license, + name: ellipsizedName, + metrics: el.metrics, + quadrant: el.quadrant, + x: el.x, + y: el.y, fullname: el.name, - name: applyEllipsis(el.name, 20), shape: 'circle', color: isListedFramework(el.name) ? getFrameworkColor(el.name) : undefined, series: [ { - name: applyEllipsis(el.name, 20), + name: ellipsizedName, x: el.x, y: el.y, }, ], } }) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2853caa5-78c1-4501-b5f9-45d323ec9044
📒 Files selected for processing (8)
app/components/Compare/FacetBarChart.vueapp/components/Compare/FacetQuadrantChart.vueapp/utils/charts.tsapp/utils/compare-quadrant-chart.tsi18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.jsontest/unit/app/utils/compare-quadrant-chart.spec.ts
✅ Files skipped from review due to trivial changes (1)
- test/unit/app/utils/compare-quadrant-chart.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- i18n/locales/fr-FR.json
- i18n/schema.json
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/Compare/FacetQuadrantChart.vue (1)
136-183: Extract the tooltip renderer out ofconfig.This computed now mixes chart options, export wiring, and a large HTML template string. Pulling the tooltip formatter into a helper (or small dedicated component) would keep the config declarative and make the metric layout much easier to test.
As per coding guidelines "Keep functions focused and manageable (generally under 50 lines)".
Also applies to: 249-320
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4c1378e-e4d8-49e2-9f38-20701313fbe0
📒 Files selected for processing (4)
app/components/Compare/FacetQuadrantChart.vueapp/utils/compare-quadrant-chart.tsi18n/locales/en.jsoni18n/locales/fr-FR.json
✅ Files skipped from review due to trivial changes (2)
- i18n/locales/fr-FR.json
- app/utils/compare-quadrant-chart.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- i18n/locales/en.json
There was a problem hiding this comment.
🧹 Nitpick comments (2)
app/components/Compare/FacetQuadrantChart.vue (2)
113-128: Consider avoiding object spread in map for slight efficiency gain.Static analysis flags the spread operator in
.map()as inefficient since it creates intermediate objects. For the typical small number of packages being compared, the impact is negligible, but if you wish to address the lint warning:♻️ Suggested refactor to avoid spread
const dataset = computed<VueUiQuadrantDatasetItem[]>(() => { return rawQuadrant.value.map((el: PackageQuadrantPoint) => { - return { - ...el, - fullname: el.name, - name: applyEllipsis(el.name, 20), - shape: 'circle', - color: isListedFramework(el.name) ? getFrameworkColor(el.name) : undefined, - series: [ - { - name: applyEllipsis(el.name, 20), - x: el.x, - y: el.y, - }, - ], - } + const item: VueUiQuadrantDatasetItem = { + id: el.id, + license: el.license, + name: applyEllipsis(el.name, 20), + fullname: el.name, + x: el.x, + y: el.y, + adoptionScore: el.adoptionScore, + efficiencyScore: el.efficiencyScore, + quadrant: el.quadrant, + metrics: el.metrics, + shape: 'circle', + color: isListedFramework(el.name) ? getFrameworkColor(el.name) : undefined, + series: [ + { + name: applyEllipsis(el.name, 20), + x: el.x, + y: el.y, + }, + ], + } + return item }) })
287-316: Add fallback values for consistency and robustness.Some metrics use
?? 0fallbacks (lines 281, 285) while others don't. Ifdatapoint?.category?.metricsis unexpectedly undefined, this could display "NaN%" or "undefined" in the tooltip.♻️ Suggested fix to add consistent fallbacks
<div class="flex flex-row items-baseline gap-2"> <span class="text-fg-subtle">${$t('compare.quadrant_chart.label_freshness_score')}</span> - <span class="text-fg text-sm">${Math.round(datapoint?.category?.metrics.freshnessPercent)}%</span> + <span class="text-fg text-sm">${Math.round(datapoint?.category?.metrics?.freshnessPercent ?? 0)}%</span> </div> <div class="text-fg text-xs mt-4">${$t('compare.quadrant_chart.label_y_axis')}</div> <div class="flex flex-row items-baseline gap-2"> <span class="text-fg-subtle">${$t('compare.facets.items.installSize.label')}</span> - <span class="text-fg text-sm">${bytesFormatter.format(datapoint?.category?.metrics.installSize)}</span> + <span class="text-fg text-sm">${bytesFormatter.format(datapoint?.category?.metrics?.installSize ?? 0)}</span> </div> <div class="flex flex-row items-baseline gap-2"> <span class="text-fg-subtle">${$t('compare.facets.items.packageSize.label')}</span> - <span class="text-fg text-sm">${bytesFormatter.format(datapoint?.category?.metrics.packageSize)}</span> + <span class="text-fg text-sm">${bytesFormatter.format(datapoint?.category?.metrics?.packageSize ?? 0)}</span> </div> <div class="flex flex-row items-baseline gap-2"> <span class="text-fg-subtle">${$t('compare.facets.items.dependencies.label')}</span> - <span class="text-fg text-sm">${datapoint?.category?.metrics.dependencies}</span> + <span class="text-fg text-sm">${datapoint?.category?.metrics?.dependencies ?? 0}</span> </div> <div class="flex flex-row items-baseline gap-2"> <span class="text-fg-subtle">${$t('compare.facets.items.totalDependencies.label')}</span> - <span class="text-fg text-sm">${datapoint?.category?.metrics.totalDependencies}</span> + <span class="text-fg text-sm">${datapoint?.category?.metrics?.totalDependencies ?? 0}</span> </div> <div class="flex flex-row items-baseline gap-2"> <span class="text-fg-subtle">${$t('compare.facets.items.vulnerabilities.label')}</span> - <span class="text-fg text-sm">${datapoint?.category?.metrics.vulnerabilities}</span> + <span class="text-fg text-sm">${datapoint?.category?.metrics?.vulnerabilities ?? 0}</span> </div>
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 043b74b5-3b22-4b95-acb5-71e2dc39cb09
📒 Files selected for processing (4)
app/components/Compare/FacetQuadrantChart.vuei18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/schema.json
🚧 Files skipped from review as they are similar to previous changes (3)
- i18n/locales/fr-FR.json
- i18n/locales/en.json
- i18n/schema.json
There was a problem hiding this comment.
🧹 Nitpick comments (1)
app/components/Compare/FacetQuadrantChart.vue (1)
113-128: Consider explicit property assignment to avoid spread in map.The static analyser flags spreading inside
.map()as inefficient. SincePackageQuadrantPointhas a known shape, you could destructure and reassign explicitly. However, if the utility may add properties in future that the chart relies on at runtime, keeping the spread preserves forward compatibility.♻️ Optional: explicit property assignment
const dataset = computed<VueUiQuadrantDatasetItem[]>(() => { - return rawQuadrant.value.map((el: PackageQuadrantPoint) => { - return { - ...el, - fullname: el.name, - name: applyEllipsis(el.name, 20), - shape: 'circle', - color: isListedFramework(el.name) ? getFrameworkColor(el.name) : undefined, - series: [ - { - name: applyEllipsis(el.name, 20), - x: el.x, - y: el.y, - }, - ], - } + return rawQuadrant.value.map((el: PackageQuadrantPoint) => ({ + x: el.x, + y: el.y, + quadrant: el.quadrant, + category: el.category, + fullname: el.name, + name: applyEllipsis(el.name, 20), + shape: 'circle' as const, + color: isListedFramework(el.name) ? getFrameworkColor(el.name) : undefined, + series: [{ name: applyEllipsis(el.name, 20), x: el.x, y: el.y }], + })) - }) })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 35dff1bd-c07b-42e0-9c66-353be24e2794
📒 Files selected for processing (1)
app/components/Compare/FacetQuadrantChart.vue
|
|
||
| const rawQuadrant = computed(() => createQuadrantDataset(source.value)) | ||
| const dataset = computed<VueUiQuadrantDatasetItem[]>(() => { | ||
| return rawQuadrant.value.map((el: PackageQuadrantPoint) => { |
There was a problem hiding this comment.
could you fix the oxlint warning on this line
oxc(no-map-spread)
Spreading to modify object properties in `map` calls is inefficient
app/utils/compare-quadrant-chart.ts
Outdated
| if (!packages.length) return [] | ||
| return packages.map(packageItem => createQuadrantPoint(packageItem)) |
There was a problem hiding this comment.
isn't this if kinda redundant? we can keep it if you want, just checking!
| } from '~/utils/charts' | ||
| import { drawNpmxLogoAndTaglineWatermark } from '~/composables/useChartWatermark' | ||
|
|
||
| import('vue-data-ui/style.css') |
There was a problem hiding this comment.
| import('vue-data-ui/style.css') | |
| import 'vue-data-ui/style.css'; |
would this also work?
There was a problem hiding this comment.
import('vue-data-ui/style.css') is a dynamic css import (was refactored to this by Daniel on TrendsChart.vue last month)
3 participants
Resolves #2387
This adds a quadrant chart to the compare page, to visualise comparisons along 2 axes:
The chart is displayed below the bar charts, and includes:
Data processing:
Some signals are inverted (so lower is better): size, deps, vulnerabilities
Deprecation is a hard override and forces min efficiency
The weights I have chosen can be subject to discussion, because of their arbitrary nature:
Other