feat: version history page display download count#2178
feat: version history page display download count#2178btea wants to merge 14 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
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 server and client support to fetch and display per-version npm download counts. A new cached server API route 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)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![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 (1)
app/pages/package/[[org]]/[name]/versions.vue (1)
295-302: Consider a shared download-count component.The same lookup/format/label block now exists in five places, and the copies already differ slightly (
divvsspan, width classes, etc.). Pulling this into a tiny component or shared helper would make later styling or accessibility tweaks much easier to keep consistent.Also applies to: 352-359, 443-450, 515-522, 563-570
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 15baadc8-c5b4-4bfe-827c-2d8460ab9ee2
📒 Files selected for processing (3)
app/pages/package/[[org]]/[name]/versions.vueserver/api/registry/npmjs-versions/[...pkg].get.tsserver/utils/npm-website-versions.ts
|
I personally found it hard to glance at these numbers without feeling a bit confused. What do you think about adding a "v" prefix to the version, or maybe in a separate tag? (The tag might be too bold, so I’m not sure; I would need to experiment with it.) 
perhaps even keeping the count always before the date, so that it's also visually consistent with the top block?
|
|
I would do something like this as it's a pretty common pattern for other places on npmx when we show version resolution. Also it allows to have right section less busy with numbers. 
|
|
Thank you for your suggestions, they all seem good. 👍 |
|
I'll mark this as draft while you work on it, can you mark it as ready when you want a review? |
|
@ghostdevv Thank you for your comment. Actually, the feature should already be complete. It's just that people have different suggestions regarding the information display location, and I'm not sure where would be most suitable. What do you think? |
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f04786c2-991f-4bb2-bb3b-7acd57fa06cb
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name]/versions.vueserver/api/registry/npmjs-versions/[...pkg].get.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- server/api/registry/npmjs-versions/[...pkg].get.ts
There was a problem hiding this comment.
🧹 Nitpick comments (5)
test/unit/server/utils/npm-website-versions.spec.ts (2)
12-14: Consider usingvi.unstubAllGlobals()for proper cleanup.
vi.restoreAllMocks()restores mocked functions but does not unstub globals set viavi.stubGlobal(). This may cause test pollution if other tests in the suite rely on the original global values.♻️ Proposed fix
afterEach(() => { vi.restoreAllMocks() + vi.unstubAllGlobals() })
16-50: Good test coverage for scoped packages and 404 handling.The tests verify URL encoding for scoped packages and the 404 error path. Consider adding coverage for:
- Non-scoped package names (simpler encoding path)
- The 502 error scenario when
versionsResponse.okis false but status is not 404🧪 Additional test cases
it('handles non-scoped package names', async () => { const fetchMock = vi.fn().mockResolvedValue({ ok: true, json: async () => ({ downloads: { '2.0.0': 456 }, }), }) vi.stubGlobal('fetch', fetchMock) const result = await fetchNpmVersionDownloadsFromApi('lodash') expect(fetchMock).toHaveBeenCalledWith('https://api.npmjs.org/versions/lodash/last-week') expect(result).toEqual([{ version: '2.0.0', downloads: 456 }]) }) it('throws a 502 error when npm API returns a non-404 failure', async () => { const fetchMock = vi.fn().mockResolvedValue({ ok: false, status: 500, }) vi.stubGlobal('fetch', fetchMock) await expect(fetchNpmVersionDownloadsFromApi('some-package')).rejects.toMatchObject({ statusCode: 502, message: 'Failed to fetch version download data from npm API', }) })app/pages/package/[[org]]/[name]/versions.vue (3)
89-101: Consider memoising group download totals.
getGroupDownloadsiterates over all versions in a group each time it's called. For packages with many versions per group (e.g., 216 versions in2.x), this runs on every render. A memoised computed or a precomputed map keyed bygroupKeywould improve performance.♻️ Proposed optimisation
+const groupDownloadsMap = computed(() => { + const map = new Map<string, number>() + for (const group of versionGroups.value) { + let total = 0 + let hasValue = false + for (const version of group.versions) { + const downloads = versionDownloadsMap.value.get(version) + if (downloads !== undefined) { + total += downloads + hasValue = true + } + } + if (hasValue) map.set(group.groupKey, total) + } + return map +}) -function getGroupDownloads(versions: string[]): number | undefined { - let total = 0 - let hasValue = false - - for (const version of versions) { - const downloads = getVersionDownloads(version) - if (downloads === undefined) continue - total += downloads - hasValue = true - } - - return hasValue ? total : undefined -} +function getGroupDownloads(groupKey: string): number | undefined { + return groupDownloadsMap.value.get(groupKey) +}Then update template calls from
getGroupDownloads(item.versions)togetGroupDownloads(item.groupKey).
582-583: Redundant fallback in:datetimebinding.The
v-ifguard already ensuresitem.versions[0]is truthy, making the?? ''fallback unnecessary.♻️ Suggested simplification
-:datetime="getVersionTime(item.versions[0] ?? '')!" +:datetime="getVersionTime(item.versions[0])!"
20-27: Type definitions align with server response.The interfaces correctly match the structure returned by the
/api/registry/npmjs-versions/{package}endpoint. Consider importingNpmWebsiteVersionDownloaddirectly from#server/utils/npm-website-versionsto avoid duplication and ensure type consistency.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a950388d-c7ee-40a8-b8ae-bff668e7318b
📒 Files selected for processing (2)
app/pages/package/[[org]]/[name]/versions.vuetest/unit/server/utils/npm-website-versions.spec.ts
MatteoGabriele
left a comment
MatteoGabriele
left a comment
There was a problem hiding this comment.
A lot of non-null assertions in this file; some of them are from previous work, but I wonder if they could be avoided, as it seems to me that they defeat the purpose of using TypeScript.
I like the adjustment of placing the count on the right side only; it looks much cleaner. However, could we add a brief explanation of what those numbers represent? It's not immediately clear to me. I understand they are downloads, given the PR title, but the UI doesn't specify this, and it might not be obvious to users either.
Perhaps 100,400 downloads?
thank you so much and let me know what you think 🙏
|
Thank you for reviewing. Adding "downloads" after each download number seems a bit redundant. I've added a description column at the top; do you think that's feasible? |
|
The issue is that the label isn't visible throughout the entire navigation; it disappears when I scroll. It also looks somewhat odd to me, especially with lower numbers and near the date, which I find confusing. 
Also found another issue: the download count in the header appears to be misplaced. 
For me, these numbers without a label don't work, but maybe we could check if someone else has the same thoughts. |
I think a description in the header is sufficient; it's not really necessary to display it all the time.
Yeah, that was intentional. The top row feels spacious enough; I think it looks better in the middle. Do you think we should put it on the right, like the bottom row?
That's fine. My main concern is that there might not be enough space to include the relevant labels on the mobile app. |
|
My final input. Thank you for being patient while I experiment with the UI here 🙏 
Let's maybe call out @ghostdevv to see a third pair of eyes. |
|
Adding an icon looks great, and we can also add a title hint. 👍 |
Yeah, also thinking about the a11y side of this: just a number wouldn't make much sense for a screen reader. |
|
@btea, can we center the downloads, icon, and date? 🙏 
|
|
I also wonder if we could use a grid so that everything is aligned? This is what it looks like currently for vite: 
|
|
Adjustments have been made as suggested. |
5 participants
🔗 Linked issue
related to #2025
🧭 Context
The version history page displays the download count for each version over the past 7 days, consistent with the npmjs functionality, which seems very useful.
📚 Description