feat(ui): community version distribution #1245
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
|
|
Oooo fancy ! |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
|
Need to change the title to feat(ui) oops |
danielroe
changed the title
|
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 version download distribution feature: new VersionDistribution Vue component and a useVersionDistribution composable; server API route /api/registry/downloads/:slug/versions and server utils to group/filter per-version downloads; shared types and re-export added; Versions.vue now opens a modal and defers mounting the chart until the dialog transition finishes; ChartModal accepts a Possibly related PRs
Suggested labels
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)
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: 2
🧹 Nitpick comments (4)
server/api/registry/downloads/[...slug].get.ts (2)
83-89:totalDownloadsis computed after filtering, which may not match expected semantics.The
totalDownloadson line 89 is calculated fromdata.downloads(all versions), butgroupson line 83 has already been created fromgroupVersionDownloads. If the intention is fortotalDownloadsto represent the sum of the returned groups only (after threshold filtering on line 86), the current calculation is inconsistent.Consider whether:
totalDownloadsshould reflect the pre-filter total (current behaviour) — this is likely correct for percentage context.- Or if it should match the sum of filtered groups.
If the current behaviour is intentional, a brief comment would clarify this.
116-117: Consider defensive access when readingversionData.versions.The cast
as { versions: string[] }assumes the external API always returns this shape. If the response structure changes or is malformed, accessingversionData.versionscould throw or assignundefined.🛡️ Suggested defensive access
- const versionData = (await fastMetaResponse.json()) as { versions: string[] } - apiResponse.recentVersions = versionData.versions + const versionData = (await fastMetaResponse.json()) as { versions?: string[] } + if (Array.isArray(versionData.versions)) { + apiResponse.recentVersions = versionData.versions + }app/components/Package/Versions.vue (1)
336-344: Remove inlinefocus-visibleutility class from button.The
focus-visible:outline-accent/70class on line 338 should be removed. Per project conventions, focus-visible styling for buttons is applied globally viamain.css, and individual buttons should not use inline focus-visible utility classes.♻️ Suggested fix
<ButtonBase variant="secondary" - class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 focus-visible:outline-accent/70 rounded" + class="text-fg-subtle hover:text-fg transition-colors duration-200 inline-flex items-center justify-center min-w-6 min-h-6 -m-1 p-1 rounded" :title="$t('package.downloads.community_distribution')" classicon="i-carbon:load-balancer-network" `@click`="openDistributionModal"Based on learnings: "focus-visible styling for buttons and selects is applied globally via main.css... individual buttons or selects in Vue components should not rely on inline focus-visible utility classes".
server/utils/version-downloads.ts (1)
120-129: Comment is misleading — the grouping logic is identical for all versions.The comment at lines 123-124 suggests special handling for 0.x versions, but the actual implementation groups all versions by
major.minoruniformly. The comment should be updated to reflect that this function always groups by minor version, which happens to align with semver conventions where 0.x minor bumps are breaking changes.📝 Suggested comment update
// Group by major.minor const groups = new Map<string, ProcessedVersion[]>() for (const version of processed) { - // For 0.x versions, treat each minor as significant (0.9.x, 0.10.x are different) - // For 1.x+, group by major.minor normally + // Group all versions by major.minor (e.g., 1.2.x, 1.3.x) + // This aligns with semver conventions where 0.x minor bumps indicate breaking changes const groupKey = `${version.major}.${version.minor}`
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/components/Package/VersionDistribution.vue (1)
66-175: SplitchartConfiginto smaller helpers to keep functions manageable.
chartConfigis well over the guideline’s 50‑line threshold. Consider extracting sub-builders (e.g., grid/tooltip/zoom config) to keep the computed function focused and easier to test.As per coding guidelines: Keep functions focused and manageable (generally under 50 lines).
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/components/Package/VersionDistribution.vue (1)
66-175: Consider splittingchartConfiginto smaller computed blocks.This computed exceeds the “generally under 50 lines” guidance. Extract grid/tooltip/zoom config builders (or separate computeds) to keep the logic manageable.
As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
| :deep(.vue-ui-xy) svg rect { | ||
| transition: none !important; | ||
| } |
There was a problem hiding this comment.
@graphieros I tried to disable the vue-ui-xy animations that I saw in the modal, as they looked a bit odd in this context (bars on the graph flying in from bottom left into position).
I couldn't figure out what the reasonable way to disable this was, so this is what I came up with for now. If there was something obvious I was missing, please let me know 🙏🏼
There was a problem hiding this comment.
What you did is fine!
It's part of the things to improve on the library side.
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
server/api/registry/downloads/[...slug].get.ts (1)
37-138: Consider extracting helper functions to keep the handler under ~50 lines.The handler is now ~100 lines, making it harder to scan and test. Splitting fetch/parsing, filtering, and enrichment into helpers would keep it focused and easier to maintain.
As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
Double encoding broke the requests here
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api/registry/downloads/[...slug].get.ts (1)
44-45: Consider extracting helper functions to keep the handler focused.The handler is well over ~50 lines; splitting query parsing, npm fetch, and recent-versions enrichment into helpers would improve readability and testability.
As per coding guidelines, "Keep functions focused and manageable (generally under 50 lines)".
| const query = getQuery(event) | ||
| const parsed = v.parse(QuerySchema, query) | ||
| const mode = parsed.mode | ||
| const filterThreshold = parsed.filterThreshold ?? 1 | ||
| const filterOldVersionsBool = parsed.filterOldVersions === 'true' |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full file to understand the try/catch structure
cat -n server/api/registry/downloads/\[...\]slug\].get.ts | head -100Repository: npmx-dev/npmx.dev
Length of output: 142
🏁 Script executed:
# Also search for where the try/catch block is located
grep -n "try\|catch" server/api/registry/downloads/\[...\]slug\].get.tsRepository: npmx-dev/npmx.dev
Length of output: 141
🏁 Script executed:
# Check valibot library usage and whether it throws on validation failure
rg "v\.parse|import.*valibot" server/api/registry/downloads/ -A 2 -B 2Repository: npmx-dev/npmx.dev
Length of output: 817
🏁 Script executed:
# Look for error handling patterns in the codebase - how other handlers deal with validation
rg "v\.parse" server/ -l --type ts --type tsx | head -10Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Check if createError is used elsewhere and what status codes it supports
rg "createError.*400|statusCode.*400" server/ --type ts --type tsx | head -5Repository: npmx-dev/npmx.dev
Length of output: 88
🏁 Script executed:
# Find the exact file
fd -t f "slug\]\.get\.ts" server/api/registry/downloads/Repository: npmx-dev/npmx.dev
Length of output: 107
🏁 Script executed:
# Read using the path from the fd output, using find + cat
find server/api/registry/downloads -name "*slug*.get.ts" -exec cat -n {} \;Repository: npmx-dev/npmx.dev
Length of output: 6523
🏁 Script executed:
# Check valibot documentation or examples in the codebase for error handling
rg "v\.parse" --type ts -B 5 -A 5 | head -50Repository: npmx-dev/npmx.dev
Length of output: 3328
🏁 Script executed:
# Find and examine the handleApiError implementation
fd -t f "error" server/utils --type ts | xargs grep -l "handleApiError"Repository: npmx-dev/npmx.dev
Length of output: 232
🏁 Script executed:
# Search for handleApiError implementation
rg "export.*handleApiError|function handleApiError" server/ -A 15Repository: npmx-dev/npmx.dev
Length of output: 1161
🏁 Script executed:
# Check test file to understand how handleApiError handles valibot errors
cat -n test/unit/server/utils/error-handler.spec.ts | grep -A 20 "throws a 404"Repository: npmx-dev/npmx.dev
Length of output: 939
🏁 Script executed:
# Get the full handleApiError implementation to see complete ValiError handling
cat -n server/utils/error-handler.tsRepository: npmx-dev/npmx.dev
Length of output: 1414
🏁 Script executed:
# Check if there's a global error handler in the framework that catches unhandled errors
rg "unhandledRejection|error.*handler|middleware" server/middleware --type ts -lRepository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Check other API routes to see if they wrap v.parse in try/catch when it's the first validation
rg -B 5 -A 5 "v\.parse" server/api --type ts | head -80Repository: npmx-dev/npmx.dev
Length of output: 5219
🏁 Script executed:
# Check if there's a global error handler or middleware that catches unhandled errors
rg "defineEventHandler|defineEventHandler.*error" server/middleware --type ts -A 5 | head -40Repository: npmx-dev/npmx.dev
Length of output: 501
🏁 Script executed:
# Search for global error handling in Nitro config or middleware
fd -t f "nitro\|error\|middleware" -E node_modules | xargs grep -l "unhandled\|catch.*error" 2>/dev/null | head -5Repository: npmx-dev/npmx.dev
Length of output: 43
🏁 Script executed:
# Verify the pattern: check if the downloads route is unique in having parse outside try/catch
rg "v\.parse.*QuerySchema|v\.parse.*query" server/api -B 3 -A 3Repository: npmx-dev/npmx.dev
Length of output: 2565
Move query validation inside the try/catch block to ensure consistent error handling.
The v.parse() call at line 69 throws a ValiError on invalid input but occurs before the try/catch block that starts at line 74. This means validation errors will surface as unhandled 500s rather than being caught and converted to a 4xx response by handleApiError. All other routes in the codebase place validation inside try/catch. Wrap the parse call in the existing try/catch to align with established error handling patterns.
|
Looks great !
|
| }, | ||
| }, | ||
| }, | ||
| timeTag: { |
There was a problem hiding this comment.
time tag can be removed (false by default)
| xAxisLabels: { | ||
| show: xAxisLabels.value.length <= 25, | ||
| values: xAxisLabels.value, | ||
| fontSize: isMobile.value ? 14 : 12, |
There was a problem hiding this comment.
I think the font size could be bigger
| padding: { | ||
| top: 24, | ||
| right: 24, | ||
| bottom: xAxisLabels.value.length > 10 ? 100 : 72, // More space for rotated labels |
There was a problem hiding this comment.
When labels rotate the chart resizes to avoid overflow automatically, so you don't need that much bottom padding
| }, | ||
| }, | ||
| }, | ||
| userOptions: { |
There was a problem hiding this comment.
userOptions is correctly declared on L.95, this can be removed
| class="chart-container w-full h-[400px] sm:h-[400px]" | ||
| :class="{ 'h-[500px]': isMobile }" | ||
| > | ||
| <VueUiXy :dataset="xyDataset" :config="chartConfig" class="[direction:ltr]" /> |
There was a problem hiding this comment.
To be added:
- svg scoped slot (see implementation in prod of DownloadAnalytics.vue, to inject a custom legend for SVG print, and npmx watermark)
- legend scoped slot for a single series
- all the scoped slots for the contextual menu:
- menuIcon
- optionCsv
- optionImg
- optionSvg
- all the annotator-action-xxx scoped slots
| :class="{ 'h-[500px]': isMobile }" | ||
| > | ||
| <VueUiXy :dataset="xyDataset" :config="chartConfig" class="[direction:ltr]" /> | ||
| </div> |
There was a problem hiding this comment.
You can use the fallback slot provided by ClientOnly (see DownloadAnalytics.vue)
3 participants
Built on the backs of @graphieros's vue-ui-xy and @NullVoxPopuli's majors to add a npmx themed version distributions feature.
Also added hide old here because that's something I personally wanted for this. Idk if anyone else was working on a related feature here, so if this crosses paths with some ongoing work, just let me know.