feat: add charts to compare page#846
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
📝 WalkthroughWalkthroughAdds a new Vue component Compare/LineChart.vue that renders DownloadAnalytics for selected packages. Extends Package/DownloadAnalytics.vue to support multi-package mode (per-package datasets, legend, custom tooltips, coordinated loading, date-range handling and dynamic export filenames) while preserving single-package behaviour. Inserts the line chart into the compare page. Adds app/utils/frameworks.ts (SHOWCASED_FRAMEWORKS, getFrameworkColor, isListedFramework) and updates index.vue to use it. Adds tests for frameworks and accessibility tests for the new chart. Bumps vue-data-ui from 3.14.3 to 3.14.5. 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)
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Package/DownloadAnalytics.vue (1)
634-639:⚠️ Potential issue | 🟡 MinorX‑axis label is suppressed even for a single package passed via
packageNames.
props.packageNamesis truthy for empty arrays and single‑item arrays, so the x‑axis label is hidden even when it should show. Consider gating on multi‑package and length > 1.🔧 Suggested fix
- xLabel: props.packageNames ? '' : xAxisLabel.value, // for multiple series, names are displayed in the chart's legend + xLabel: + isMultiPackageMode.value && effectivePackageNames.value.length > 1 + ? '' + : xAxisLabel.value, // for multiple series, names are displayed in the chart's legend
🧹 Nitpick comments (1)
app/composables/useFrameworks.ts (1)
3-20: Tighten framework typing and avoid the unsafe non‑null assertion.
FrameworkNamecurrently widens tostring, soisListedFrameworkdoesn’t narrow andgetFrameworkColorcan still throw. Consider freezing the list withas constand returning a nullable colour (or a safe fallback) instead of!.♻️ Suggested adjustment
- const frameworks = ref([ + const frameworks = ref([ { name: 'nuxt', package: 'nuxt', color: 'oklch(0.7862 0.192 155.63)' }, { name: 'vue', package: 'vue', color: 'oklch(0.7025 0.132 160.37)' }, { name: 'nitro', package: 'nitro', color: 'oklch(70.4% 0.191 22.216)' }, { name: 'react', package: 'react', color: 'oklch(0.832 0.1167 218.69)' }, { name: 'svelte', package: 'svelte', color: 'oklch(0.6917 0.1865 35.04)' }, { name: 'vite', package: 'vite', color: 'oklch(0.7484 0.1439 294.03)' }, { name: 'next', package: 'next', color: 'oklch(71.7% .1648 250.794)' }, { name: 'astro', package: 'astro', color: 'oklch(0.5295 0.2434 270.23)' }, { name: 'typescript', package: 'typescript', color: 'oklch(0.5671 0.1399 253.3)' }, { name: 'angular', package: '@angular/core', color: 'oklch(0.626 0.2663 310.4)' }, - ]) + ] as const) - function getFrameworkColor(framework: FrameworkName): string { - return frameworks.value.find(f => f.name === framework)!.color - } + function getFrameworkColor(framework: FrameworkName): string | undefined { + return frameworks.value.find(f => f.name === framework)?.color + }As per coding guidelines: Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index.
| <h2 | ||
| id="comparison-heading" | ||
| class="text-xs text-fg-subtle uppercase tracking-wider mb-4 mt-10" | ||
| > | ||
| downloads history | ||
| </h2> | ||
|
|
||
| <CompareLineChart :packages /> | ||
| </div> |
There was a problem hiding this comment.
Avoid duplicate heading IDs and localise the new title.
Line 113 already uses comparison-heading, so reusing it here breaks aria-labelledby and can confuse assistive tech. The new heading text is also hard‑coded. Consider giving the downloads history block its own section + i18n key.
🔧 Suggested fix
- <h2
- id="comparison-heading"
- class="text-xs text-fg-subtle uppercase tracking-wider mb-4 mt-10"
- >
- downloads history
- </h2>
-
- <CompareLineChart :packages />
+ <section class="mt-10" aria-labelledby="downloads-history-heading">
+ <h2
+ id="downloads-history-heading"
+ class="text-xs text-fg-subtle uppercase tracking-wider mb-4"
+ >
+ {{ $t('compare.packages.section_downloads_history') }}
+ </h2>
+ <CompareLineChart :packages="packages" />
+ </section>📝 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.
| <h2 | |
| id="comparison-heading" | |
| class="text-xs text-fg-subtle uppercase tracking-wider mb-4 mt-10" | |
| > | |
| downloads history | |
| </h2> | |
| <CompareLineChart :packages /> | |
| </div> | |
| <section class="mt-10" aria-labelledby="downloads-history-heading"> | |
| <h2 | |
| id="downloads-history-heading" | |
| class="text-xs text-fg-subtle uppercase tracking-wider mb-4" | |
| > | |
| {{ $t('compare.packages.section_downloads_history') }} | |
| </h2> | |
| <CompareLineChart :packages="packages" /> | |
| </section> | |
| </div> |
There was a problem hiding this comment.
@wojtekmaj Thank you! 🙏
🧠 Learnings used
Learnt from: alexdln
Repo: npmx-dev/npmx.dev PR: 838
File: app/pages/package/[...package].vue:445-449
Timestamp: 2026-02-03T13:59:33.392Z
Learning: The copy button pattern in app/pages/package/[...package].vue may be made into a reusable component or pattern in the future, but currently it's acceptable to keep it inline with the CSS-only approach for smooth animations.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
app/components/Package/DownloadAnalytics.vue (2)
27-27: Consider removing or documenting the always-true computed.
shouldFetchis a computed that always returnstrue. If this is a placeholder for future conditional logic, consider adding a brief comment explaining intent. Otherwise, the indirection adds no value and the usages could be simplified.
454-455: Avoidas anycast; define a proper type for options.Casting
options.valuetoanybypasses TypeScript's type checking forstartDateandendDateproperties. Consider extending theoptionstype union to include these optional fields explicitly.🔧 Suggested improvement
The
optionstype already includesDateRangeFieldsviaapplyDateRange, but accessing those fields requires a type guard or a widened base type. One approach:- const o = options.value - const hasExplicitRange = Boolean((o as any).startDate || (o as any).endDate) + const o = options.value as typeof options.value & DateRangeFields + const hasExplicitRange = Boolean(o.startDate || o.endDate)Alternatively, define a common base type that includes the optional date fields.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Package/DownloadAnalytics.vue (1)
454-456: Avoidanycasts onoptionsto keep type‑safety.
All union variants already sharestartDate/endDate, so the casts can be removed.🔧 Suggested refactor
- const o = options.value - const hasExplicitRange = Boolean((o as any).startDate || (o as any).endDate) + const o = options.value + const hasExplicitRange = Boolean(o.startDate || o.endDate)- const o = options.value as any + const o = options.valueAs per coding guidelines: Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index.
Also applies to: 480-493
| :disabled="pending" | ||
| class="w-full px-2.5 py-1.75 bg-bg-subtle font-mono text-sm text-fg outline-none appearance-none focus-visible:outline-accent/70" | ||
| > |
There was a problem hiding this comment.
Remove inline focus-visible utilities on select/button.
Project guidance applies focus-visible styling globally for buttons and selects; these per‑element utilities should be removed for consistency.
Proposed diff
- class="w-full px-2.5 py-1.75 bg-bg-subtle font-mono text-sm text-fg outline-none appearance-none focus-visible:outline-accent/70"
+ class="w-full px-2.5 py-1.75 bg-bg-subtle font-mono text-sm text-fg outline-none appearance-none"- class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border focus-visible:outline-accent/70 sm:mb-0"
+ class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border sm:mb-0"Based on learnings, focus-visible styling for buttons/selects is global via app/assets/main.css and inline utilities should not be added.
Also applies to: 844-849
| // VueUiXy is imported directly in <script setup>, so global stubs cannot override it. | ||
| // We mock the module itself to prevent vue-data-ui from mounting charts during tests | ||
| // (it relies on DOM measurements and causes runtime errors in Vitest / Playwright). | ||
| // This render-function stub avoids the Vue runtime-compiler warning and keeps slots working. | ||
| vi.mock('vue-data-ui/vue-ui-xy', () => { | ||
| return { | ||
| VueUiXy: defineComponent({ | ||
| name: 'VueUiXy', | ||
| inheritAttrs: false, | ||
| setup(_, { attrs, slots }) { | ||
| return () => | ||
| h('div', { ...attrs, 'data-test-id': 'vue-ui-xy-stub' }, slots.default?.() ?? []) | ||
| }, | ||
| }), | ||
| } | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the current imports in the test file
cat -n test/nuxt/a11y.spec.ts | head -30Repository: npmx-dev/npmx.dev
Length of output: 1517
🏁 Script executed:
# Also check if there's a nuxt config file that might enable auto-imports
fd -e config.ts -e config.js | grep -E "(nuxt|vite)" | head -10Repository: npmx-dev/npmx.dev
Length of output: 112
🏁 Script executed:
# Check for any auto-import configuration in the repository
rg -t ts -t js "auto.?import|unplugin" --max-count 5Repository: npmx-dev/npmx.dev
Length of output: 290
🏁 Script executed:
# Check nuxt.config.ts for auto-import configuration
cat -n nuxt.config.ts | head -80Repository: npmx-dev/npmx.dev
Length of output: 2617
🏁 Script executed:
# Check vitest.config.ts for test environment setup
cat vitest.config.tsRepository: npmx-dev/npmx.dev
Length of output: 2292
🏁 Script executed:
# Search for other uses of defineComponent or h in test files
rg "defineComponent|^import.*from\s+['\"]vue['\"]" test/nuxt/ --type ts -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 3697
🏁 Script executed:
# Check if there are any runtime errors or issues mentioned in the test setup
cat -n test/nuxt/setup.tsRepository: npmx-dev/npmx.dev
Length of output: 1101
Add explicit imports for defineComponent and h from Vue.
The Nuxt test environment provides auto-imports, but other test files in this codebase (e.g. use-colors.spec.ts) explicitly import Vue utilities. This makes the mock self-contained and improves type safety consistency.
Proposed diff
import { afterEach, describe, expect, it, vi } from 'vitest'
+import { defineComponent, h } from 'vue'
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/utils/frameworks.ts (1)
58-60: Non-null assertion bypasses type safety.The
!operator assumes.find()always succeeds. IfFrameworkPackageNameisn't properly constrained (see above), any string could be passed, causing a runtime error when accessing.coloronundefined.Consider adding a fallback or explicit error handling for robustness.
🛡️ Proposed defensive handling
export function getFrameworkColor(framework: FrameworkPackageName): string { - return SHOWCASED_FRAMEWORKS.find(f => f.package === framework)!.color + const found = SHOWCASED_FRAMEWORKS.find(f => f.package === framework) + if (!found) { + throw new Error(`Unknown framework: ${framework}`) + } + return found.color }Alternatively, with proper
as consttyping, the!becomes acceptable since only valid frameworks can be passed.As per coding guidelines: "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index".
| export const SHOWCASED_FRAMEWORKS = [ | ||
| { | ||
| name: 'nuxt', | ||
| package: 'nuxt', | ||
| color: 'oklch(0.7862 0.192 155.63)', | ||
| }, | ||
| { name: 'vue', package: 'vue', color: 'oklch(0.7025 0.132 160.37)' }, | ||
| { | ||
| name: 'nitro', | ||
| package: 'nitro', | ||
| color: 'oklch(70.4% 0.191 22.216)', | ||
| }, | ||
| { | ||
| name: 'react', | ||
| package: 'react', | ||
| color: 'oklch(0.832 0.1167 218.69)', | ||
| }, | ||
| { | ||
| name: 'svelte', | ||
| package: 'svelte', | ||
| color: 'oklch(0.6917 0.1865 35.04)', | ||
| }, | ||
| { | ||
| name: 'vite', | ||
| package: 'vite', | ||
| color: 'oklch(0.7484 0.1439 294.03)', | ||
| }, | ||
| { | ||
| name: 'next', | ||
| package: 'next', | ||
| color: 'oklch(71.7% .1648 250.794)', | ||
| }, | ||
| { | ||
| name: 'astro', | ||
| package: 'astro', | ||
| color: 'oklch(0.5295 0.2434 270.23)', | ||
| }, | ||
| { | ||
| name: 'typescript', | ||
| package: 'typescript', | ||
| color: 'oklch(0.5671 0.1399 253.3)', | ||
| }, | ||
| { | ||
| name: 'angular', | ||
| package: '@angular/core', | ||
| color: 'oklch(0.626 0.2663 310.4)', | ||
| }, | ||
| ] | ||
|
|
||
| export type FrameworkPackageName = (typeof SHOWCASED_FRAMEWORKS)[number]['package'] |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add as const to derive literal union types for FrameworkPackageName.
Without as const, TypeScript widens the array element types, causing FrameworkPackageName to resolve to string rather than the intended union type ('nuxt' | 'vue' | 'react' | ...). This undermines the type safety of getFrameworkColor.
🔧 Proposed fix
-export const SHOWCASED_FRAMEWORKS = [
+export const SHOWCASED_FRAMEWORKS = [
{
name: 'nuxt',
package: 'nuxt',
color: 'oklch(0.7862 0.192 155.63)',
},
// ... rest of frameworks
{
name: 'angular',
package: '@angular/core',
color: 'oklch(0.626 0.2663 310.4)',
},
-]
+] as const satisfies readonly ShowcasedFramework[]As per coding guidelines: "Ensure you write strictly type-safe code".
| } from '../../../../app/utils/frameworks' | ||
|
|
||
| describe('getFrameworkColor', () => { | ||
| it('returns the color a listed framework', () => { |
There was a problem hiding this comment.
Minor typo in test description.
Missing "of" in the test description.
📝 Proposed fix
- it('returns the color a listed framework', () => {
+ it('returns the color of a listed framework', () => {📝 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.
| it('returns the color a listed framework', () => { | |
| it('returns the color of a listed framework', () => { |
3 participants
closes #803