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)".
|
It seems some logic is goofy with the 'old versions' toggle? For example: Unchecked: i see 2.x and 6.x. Which seems backwards? Idk Lookin' good so far! |
Just tried this and it worked for me, have you opened this link before by any chance? I have fixed caching related bugs on this branch that may require you to clear your browser cache |
|
Hmm -- I experienced this in a private browser window 🤔 |
|
@NullVoxPopuli oh! I read your message wrong. I actually flipped this toggle from "hide" to "show" recently, so now it defaults to hiding the old versions, so what you described is expected. By default it will show versions younger than 1 year ago (older ones are hidden), and checked the box reveals them (shows you more versions) |
|
It's time based? I don't think that is as effective a signal, because in the example above, we had an enterprise client who hadn't upgraded in over a decade pay for a security fix, so now 2.x shows up even though most users aren't on 2.x And the whole major-release schedule is every year and a half for this particular project - and some other ecosystems have even longer between majors - perhaps a current version to current version minus 5 or some other range could be used? Was time chosen because of frequent major releasors? (Angular, Next, etc?) |
Yea the old versions feature is time based. It defaults to hiding versions older than 1 years, and enabling the toggle displays those old versions. This was just something I wanted personally, but if you'd prefer it behave differently I am open to changing this / the defaults / etc. |
|
To me, it seems like 1 year only makes sense if the whole ecosystem is super on top of upgrades and every library is doing multiple majors per year -- which would be quite disruptive if it were the default, i think |
should the labels be more left-aligned -- closer to the toggles? <3 |
|
Interesting, I didn't know they had tooltips on the toggles, I added my own to the labels instead when I first started this, not sure how new that is, but it definitely wasn't there before (maybe a bug fix?) |
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.