feat: replace quadrant with scatter with selectable axes#2472
feat: replace quadrant with scatter with selectable axes#2472graphieros merged 22 commits intomainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
graphieros
changed the title
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
|
|
| 📦 Package | 📋 Versions |
|---|---|
| h3 | 5 versions
h3@2.0.1-rc.11 h3@2.0.1-rc.16 h3@2.0.1-rc.20 |
| glob | 5 versions
|
| @rolldown/pluginutils | 5 versions
@rolldown/pluginutils@1.0.0-rc.9 |
| @oxc-project/types | 6 versions
|
💡 To find out what depends on a specific package, run: pnpm -r why example-package
|
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:
📝 WalkthroughWalkthroughThis PR removes the quadrant chart and its scoring pipeline, replacing it with a selectable-axis scatter chart. It adds Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/pages/compare.vue (1)
429-456:⚠️ Potential issue | 🟡 MinorDon't show the empty-state copy for scatter-only facet sets.
This branch still keys off
facet.chartable, which now only covers the bar charts. If the user selects only scatter-capable facets such asvulnerabilitiesorlastUpdated, the page tells them “No chartable data available” even thoughFacetScatterChartbelow can still render. Gate this message onchartable_scatteras well, or scope it to the bar-chart section only.💡 Suggested tweak
- <p v-else class="py-12 text-center text-fg-subtle"> + <p + v-else-if="!selectedFacets.some(facet => facet.chartable_scatter)" + class="py-12 text-center text-fg-subtle" + > {{ $t('compare.packages.no_chartable_data') }} </p>test/nuxt/a11y.spec.ts (1)
1020-1127:⚠️ Potential issue | 🟠 MajorFixture does not exercise the populated scatter render path.
Since
FacetScatterChartdefaults to Downloads/wk (X-axis) × Install Size (Y-axis), andgetNumericFacetValuereturnsnullwheninstallSizeis missing, both items in this fixture will be filtered out by theif (x === null || y === null)check inbuildCompareScatterChartDataset. The chart will render empty, so the test only covers the empty-state path. AddinstallSizeto the fixture to exercise the actual populated scatter render.
🧹 Nitpick comments (1)
app/utils/charts.ts (1)
700-705: Rename the typoed export before it spreads further.
coopyAltTextForCompareScatterChartlooks accidental and makes the helper harder to discover by grep/autocomplete.Proposed fix
-export async function coopyAltTextForCompareScatterChart({ +export async function copyAltTextForCompareScatterChart({ dataset, config, }: AltCopyArgs<VueUiScatterSeries[], CompareScatterChartConfig>) { const altText = createAltTextForCompareScatterChart({ dataset, config }) await config.copy(altText) }
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6257ac32-ed17-41e5-adc4-4084e3970520
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (21)
app/components/Compare/FacetQuadrantChart.vueapp/components/Compare/FacetScatterChart.vueapp/components/Package/TrendsChart.vueapp/composables/useChartWatermark.tsapp/composables/useFacetSelection.tsapp/pages/compare.vueapp/utils/charts.tsapp/utils/compare-quadrant-chart.tsapp/utils/compare-scatter-chart.tsi18n/locales/cs-CZ.jsoni18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/locales/ru-RU.jsoni18n/locales/zh-CN.jsoni18n/locales/zh-TW.jsoni18n/schema.jsonnuxt.config.tspackage.jsontest/nuxt/a11y.spec.tstest/unit/app/utils/compare-quadrant-chart.spec.tstest/unit/app/utils/compare-scatter-chart.spec.ts
💤 Files with no reviewable changes (7)
- i18n/locales/cs-CZ.json
- i18n/locales/zh-CN.json
- app/components/Compare/FacetQuadrantChart.vue
- i18n/locales/zh-TW.json
- i18n/locales/ru-RU.json
- test/unit/app/utils/compare-quadrant-chart.spec.ts
- app/utils/compare-quadrant-chart.ts
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Compare/FacetScatterChart.vue (2)
125-269: Split the chart config builder into smaller helpers.This computed now mixes export callbacks, alt-copy wiring, axis formatting, and style/layout in one block. Pulling those sections into dedicated helpers will make future chart changes much easier to review and safer to extend.
As per coding guidelines,
**/*.{ts,tsx,vue}should “Keep functions focused and manageable (generally under 50 lines)”.
82-89: Refactor to avoid fragile index-based fallback logic.The scatter chart function iterates
packagesDataby index and accessespackages[index]for fallback names. When NO_DEPENDENCY_ID is in the packages list,packagesDataincludes its entry, but the filteredpackagespassed to the component does not. This creates an array length mismatch: ifpackages.value = ["react", "vue", "__no_dependency__"], thenpackagesDatahas 3 items but filteredpackageshas 2. Accessingpackages[2]would be undefined.This latent misalignment is currently masked because NO_DEPENDENCY_ID entries have null x/y facet values and filter out at line 98 before the fallback is used. However, the code is fragile and depends on external data shape to avoid the actual index-out-of-bounds risk.
Instead of relying on index-based fallbacks, pass the filtered
packagesDatadirectly (already filtered alongsidepackages), or refactorbuildCompareScatterChartDatasetto not assume index alignment between the two arrays.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94752aba-4f36-4762-91b0-4dab9eacee01
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (14)
app/components/Compare/FacetScatterChart.vueapp/utils/charts.tsapp/utils/compare-scatter-chart.tsi18n/locales/cs-CZ.jsoni18n/locales/en.jsoni18n/locales/fr-FR.jsoni18n/locales/nb-NO.jsoni18n/locales/ru-RU.jsoni18n/locales/zh-CN.jsoni18n/locales/zh-TW.jsoni18n/schema.jsonnuxt.config.tspackage.jsontest/unit/app/utils/compare-scatter-chart.spec.ts
💤 Files with no reviewable changes (4)
- i18n/locales/cs-CZ.json
- i18n/locales/zh-TW.json
- i18n/locales/zh-CN.json
- i18n/locales/ru-RU.json
✅ Files skipped from review due to trivial changes (2)
- package.json
- test/unit/app/utils/compare-scatter-chart.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- nuxt.config.ts
- i18n/schema.json
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Compare/FacetScatterChart.vue (1)
488-493: Please remove the commented-out CSS block.Keeping dead positioning rules in a brand-new component makes it harder to see which scatter-specific tweaks are still intentional.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e96ba54d-a497-4122-844f-227c3fc5d91e
📒 Files selected for processing (1)
app/components/Compare/FacetScatterChart.vue
graphieros
added
the
needs review
| </template> | ||
| <template #skeleton> | ||
| <!-- This empty div overrides the default built-in scanning animation on load --> | ||
| <div /> |
There was a problem hiding this comment.
| <div /> | |
| <div></div> |
2 participants
Resolves #2449
This replaces the controversial quadrant chart with a scatter plot, where 2 facets can be compared.
No weights, no debates.