Uh oh!
There was an error while loading. Please reload this page.
feat(ui): generalize downloads chart and add social likes option - #1310
Conversation
Generalize the download analytics chart into a trends chart that supports both downloads and likes, extensibly allowing for more facets in the future. - Add a facet selector (Downloads / Likes) shown on Compare page and in the single-package chart modal - Add server endpoint and utility for fetching per-package likes evolution from ATProto
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
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
It's kinda nice to see this as a diff in PRs actually 😁
📝 WalkthroughWalkthroughThis PR replaces DownloadAnalytics with a new TrendsChart component (enabled with Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 1✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
No actionable comments were generated in the recent review. 🎉 Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
i18n/locales/fr-FR.json (1)
317-446:⚠️ Potential issue | 🟠 MajorDuplicate
package.trendskeys will override earlier translations.
JSON allows only onetrendsobject at this level; the later block wins and the earlier block is ignored by parsers that accept duplicates. Please merge them into a single block so translations are deterministic.🛠️ Suggested fix (merge into a single package.trends block)
"trends": { "granularity": "Granularité", "granularity_daily": "Quotidien", "granularity_weekly": "Hebdomadaire", "granularity_monthly": "Mensuel", "granularity_yearly": "Annuel", "start_date": "Début", "end_date": "Fin", "loading": "Chargement...", "date_range": "{start} au {end}", "date_range_multiline": "{start}\nau {end}", "download_file": "Télécharger {fileType}", "toggle_annotator": "Afficher/Masquer l'annotateur", "legend_estimation": "Estimation", "no_data": "Données non disponibles", "y_axis_label": "{facet} {granularity}", - "items": {- "downloads": "Téléchargements"- }+ "facet": "Facette",+ "title": "Tendances",+ "items": {+ "downloads": "Téléchargements",+ "likes": "J'aime"+ } }, @@ - "trends": {- "granularity": "Granularité",- "granularity_daily": "Quotidien",- "granularity_weekly": "Hebdomadaire",- "granularity_monthly": "Mensuel",- "granularity_yearly": "Annuel",- "start_date": "Début",- "end_date": "Fin",- "loading": "Chargement...",- "date_range": "{start} au {end}",- "date_range_multiline": "{start}\nau {end}",- "download_file": "Télécharger {fileType}",- "toggle_annotator": "Afficher/Masquer l'annotateur",- "legend_estimation": "Estimation",- "no_data": "Données non disponibles",- "y_axis_label": "{facet} {granularity}",- "facet": "Facette",- "title": "Tendances",- "items": {- "downloads": "Téléchargements",- "likes": "J'aime"- }- },
🧹 Nitpick comments (3)
app/components/Package/TrendsChart.vue (2)
602-604: Non-null assertion onfind()result may cause runtime error.
METRICS.value.find(m => m.id === selectedMetric.value)can returnundefinedif no metric matches. Using!here assumes the invariant always holds, but ifselectedMetricwere ever set to an invalid value (e.g., via external input or a bug), this would throw at runtime.Consider adding a fallback or guard:
🛡️ Proposed defensive fix
const activeMetricState = computed(() => metricStates[selectedMetric.value]) -const activeMetricDef = computed(() => METRICS.value.find(m => m.id === selectedMetric.value)!)+const activeMetricDef = computed(() => {+ const metric = METRICS.value.find(m => m.id === selectedMetric.value)+ if (!metric) {+ throw new Error(`Unknown metric: ${selectedMetric.value}`)+ }+ return metric+})
674-676: Repeated non-null assertion onfind()result.Same concern as
activeMetricDef— ifmetricIddoesn't match any entry inMETRICS, the!assertion will throw.🛡️ Proposed defensive fix
const state = metricStates[metricId] - const metric = METRICS.value.find(m => m.id === metricId)!+ const metric = METRICS.value.find(m => m.id === metricId)+ if (!metric) {+ console.error(`Unknown metric: ${metricId}`)+ state.pending = false+ return+ } const currentToken = ++state.requestTokenapp/composables/useCharts.ts (1)
376-394: Consider adding TTL-based invalidation to match caching patterns used elsewhere.The
likesEvolutionCachelacks expiration logic and persists for the entire session. Once cached, new likes won't appear until the page is refreshed. Whilst this is acceptable for brief sessions, the codebase already establishes TTL patterns in other composables (e.g.useRepoMeta.tsuses a 10-minute TTL for repository metadata). A similar TTL here would ensure data freshness aligns with how frequently likes data changes.
Uh oh!
There was an error while loading. Please reload this page.
| export default defineEventHandler(async event => { | ||
| const packageName = getRouterParam(event, 'pkg') | ||
| if (!packageName) { | ||
| throw createError({ | ||
| status: 400, | ||
| message: 'package name not provided', | ||
| }) | ||
| } | ||
| const likesUtil = new PackageLikesUtils() | ||
| return await likesUtil.getLikesEvolution(packageName) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Check the codebase structure and find similar patternsecho"=== File structure ==="
ls -la server/api/social/ 2>/dev/null ||echo"Directory not found"
find server -type f -name "[...*.ts"2>/dev/null | head -20
echo -e "\n=== Checking other catch-all routes ==="
rg -l "\[\.\.\." server/api --type=ts 2>/dev/null | head -10Repository: npmx-dev/npmx.dev
Length of output: 1313
🏁 Script executed:
#!/bin/bash# Check how PackageLikesUtils is used elsewhereecho"=== PackageLikesUtils usage ==="
rg -n "PackageLikesUtils" server --type=ts -A 2 -B 1
echo -e "\n=== Check if getLikesEvolution is used elsewhere ==="
rg -n "getLikesEvolution" server --type=ts -B 2 -A 2Repository: npmx-dev/npmx.dev
Length of output: 2721
🏁 Script executed:
#!/bin/bash# Check similar route handlers to see if they normalize router paramsecho"=== Other handlers with getRouterParam ==="
rg -n "getRouterParam" server/api --type=ts -B 1 -A 1 | head -50
echo -e "\n=== Check for Array.isArray patterns with router params ==="
rg -n "Array\.isArray.*getRouterParam|Array\.isArray.*router" server --type=tsRepository: npmx-dev/npmx.dev
Length of output: 3893
🏁 Script executed:
#!/bin/bash# Look at the actual file in context and search for related utilitiesecho"=== Content of the likes-evolution handler ==="
cat server/api/social/likes-evolution/\[...pkg\].get.ts 2>/dev/null || find server -name "*likes*evolution*" -type f
echo -e "\n=== Search for how routes handle scoped packages ==="
rg -n "@.*/" server/api --type=ts | head -20Repository: npmx-dev/npmx.dev
Length of output: 2767
🏁 Script executed:
#!/bin/bash# Find parsePackageParams definition and usageecho"=== parsePackageParams definition ==="
rg -n "parsePackageParams" server --type=ts -B 2 -A 10 | head -80
echo -e "\n=== Check if parsePackageParams is used elsewhere ==="
rg -l "parsePackageParams" server --type=tsRepository: npmx-dev/npmx.dev
Length of output: 6692
🏁 Script executed:
#!/bin/bash# Compare the two handlers side by sideecho"=== Likes handler (full content) ==="
cat server/api/social/likes/\[...pkg\].get.ts 2>/dev/null
echo -e "\n=== Likes-evolution handler (full content) ==="
cat server/api/social/likes-evolution/\[...pkg\].get.ts 2>/dev/nullRepository: npmx-dev/npmx.dev
Length of output: 1391
Use consistent package parameter parsing like the likes handler.
The handler should match the pattern in server/api/social/likes/[...pkg].get.ts: split the parameter, parse with parsePackageParams, validate with PackageRouteParamsSchema, and decode URI components. This ensures proper handling of scoped packages (e.g. @scope/package), versioning patterns (e.g. package/v/1.0.0), and URL-encoded characters.
✅ Suggested fix
+import * as v from 'valibot'+import { PackageRouteParamsSchema } from '#shared/schemas/package'+import { parsePackageParams } from '~/server/utils/parse-package-params'+
export default defineEventHandler(async event => {
- const packageName = getRouterParam(event, 'pkg')- if (!packageName) {+ const pkgParamSegments = getRouterParam(event, 'pkg')?.split('/') ?? []+ const { rawPackageName } = parsePackageParams(pkgParamSegments)++ if (!rawPackageName) {
throw createError({
status: 400,
message: 'package name not provided',
})
}
- const likesUtil = new PackageLikesUtils()- return await likesUtil.getLikesEvolution(packageName)+ const { packageName } = v.parse(PackageRouteParamsSchema, {+ packageName: decodeURIComponent(rawPackageName),+ })++ const likesUtil = new PackageLikesUtils()+ return await likesUtil.getLikesEvolution(packageName)
})
graphieros
left a comment
There was a problem hiding this comment.
This is beautiful refactoring ✨
Huge fan
| @@ -1858,21 +1945,17 @@ const chartConfig = computed(() => { | |||
| <div class="min-h-[260px]" /> | |||
There was a problem hiding this comment.
I forgot about this placeholder.
We can probably have it fit the resolved size, to mitigate CLS when switching facets for the first time
There was a problem hiding this comment.
would you like to follow up on that? 😁
| createdIso?: string | null | ||
| /** When true, shows facet selector (e.g. Downloads / Likes). */ | ||
| showFacetSelector?: boolean |
There was a problem hiding this comment.
Is this needed? Seems like it's never false
Uh oh!
There was an error while loading. Please reload this page.
Generalize the download analytics chart into a trends chart that supports both downloads and likes, extensibly allowing for more facets in the future.
npmx.chart.package.likes.mp4
Note
Full disclosure: I don't really know what I'm doing with the ATProto stuff. I tried to follow existing patterns. It seems to work.