feat(ui): adds social likes to compare page#940
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
|
📝 WalkthroughWalkthroughAdds a new numeric facet 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)
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: 1
🧹 Nitpick comments (1)
app/composables/usePackageComparison.ts (1)
109-120: Consider surfacing likes fetch failures as “unknown” rather than “0”.A failed likes request currently renders as 0 likes, which can be misleading. Aligning with downloads (null/undefined on failure) keeps comparisons honest.
♻️ Suggested adjustment
export interface PackageComparisonData { package: ComparisonPackage downloads?: number /** Total likes from atproto */ - totalLikes: number + totalLikes?: number /** Package's own unpacked size (from dist.unpackedSize) */ packageSize?: number @@ - $fetch<PackageLikes>(`/api/social/likes/${name}`).catch(() => ({ - totalLikes: 0, - userHasLiked: false, - })), + $fetch<PackageLikes>(`/api/social/likes/${name}`).catch(() => null), @@ - totalLikes: likes.totalLikes, + totalLikes: likes?.totalLikes,Also applies to: 168-168
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
The verdict is in, sorry Daniel 
|
|
ok ok never mind, you do win this round: 
|
serhalp
left a comment
serhalp
left a comment
There was a problem hiding this comment.
Awesome, thanks! 🚀
When you merge main in, please test this feature with the new "no dep" column from #801. I think you'll just need to add a field to this 'fake' column.
serhalp
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/composables/usePackageComparison.ts (1)
361-368: Consider handling theisNoDependencycase for consistency withdownloads.The
downloadsfacet (lines 351-354) has special handling for the "no dependency" column, returning{ raw: 0, display: '–', status: 'neutral' }. ThetotalLikescase doesn't have this handling, so the no-dependency column will show nothing for Likes whilst showing "–" for Downloads.If the intent is to show "–" for likes in the no-dependency column as well, consider adding the same pattern:
♻️ Proposed change for consistency
case 'totalLikes': { - if (data.totalLikes === undefined) return null + if (data.totalLikes === undefined) { + if (isNoDependency) return { raw: 0, display: '–', status: 'neutral' } + return null + } return { raw: data.totalLikes, display: formatCompactNumber(data.totalLikes), status: 'neutral', } }
| $fetch<VulnerabilityTreeResult>( | ||
| `/api/registry/vulnerabilities/${encodePackageName(name)}`, | ||
| ).catch(() => null), | ||
| $fetch<PackageLikes>(`/api/social/likes/${name}`).catch(() => null), |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n app/composables/usePackageComparison.ts | sed -n '130,150p'Repository: npmx-dev/npmx.dev
Length of output: 1254
🏁 Script executed:
# Search for the API route definition for /api/social/likes
rg -n "social/likes" --type ts --type js -B 2 -A 5Repository: npmx-dev/npmx.dev
Length of output: 1906
🏁 Script executed:
# Find where encodePackageName is defined and used
rg -n "encodePackageName" --type ts --type js | head -20Repository: npmx-dev/npmx.dev
Length of output: 2075
🏁 Script executed:
# Check the broader context of lines around 143 to see other API calls
cat -n app/composables/usePackageComparison.ts | sed -n '125,160p'Repository: npmx-dev/npmx.dev
Length of output: 1890
🏁 Script executed:
# Find the API route definition for social/likes
fd -type f -name "*.ts" -o -name "*.js" | xargs rg -l "social/likes" | head -10Repository: npmx-dev/npmx.dev
Length of output: 358
🏁 Script executed:
# Search for the server-side route handler
rg -n "api/social/likes" --type ts --type js -B 3 -A 10Repository: npmx-dev/npmx.dev
Length of output: 3146
🏁 Script executed:
# Look for likes route in server/api
fd -path "*/server/api/*" -name "*likes*" -o -path "*/server/api/*" -name "*social*"Repository: npmx-dev/npmx.dev
Length of output: 229
🏁 Script executed:
# Find server API routes for social/likes
find server -name "*.ts" -o -name "*.js" | xargs grep -l "social" 2>/dev/null | head -10Repository: npmx-dev/npmx.dev
Length of output: 124
🏁 Script executed:
# Search more broadly for likes route handler
rg "likes" server/api --type-list | head -5Repository: npmx-dev/npmx.dev
Length of output: 450
🏁 Script executed:
# List the server/api directory structure
find server/api -type f -name "*.ts" | sortRepository: npmx-dev/npmx.dev
Length of output: 1035
🏁 Script executed:
cat -n server/api/social/likes/[...pkg].get.tsRepository: npmx-dev/npmx.dev
Length of output: 520
Package name should be encoded for consistency and scoped package support.
The likes API call uses the raw name without encodePackageName(), unlike the other API calls in this block. This causes issues with scoped packages (e.g., @vue/core) because the forward slash in the scope becomes a path separator. The same issue exists in two other locations: app/pages/package/[...package].vue line 402 and app/components/OgImage/Package.vue line 49.
🐛 Proposed fix
- $fetch<PackageLikes>(`/api/social/likes/${name}`).catch(() => null),
+ $fetch<PackageLikes>(`/api/social/likes/${encodePackageName(name)}`).catch(() => null),Apply the same fix to app/pages/package/[...package].vue line 402 and app/components/OgImage/Package.vue line 49.
📝 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.
| $fetch<PackageLikes>(`/api/social/likes/${name}`).catch(() => null), | |
| $fetch<PackageLikes>(`/api/social/likes/${encodePackageName(name)}`).catch(() => null), |
There was a problem hiding this comment.
Whoops thought i did it my bad coderabbit
There was a problem hiding this comment.
@Sukiiu No worries at all! It's an easy thing to miss, especially since the API route uses [...pkg] which handles it server-side. Thanks for taking a look! 🐰
🐰✨
( •_•)
/>🔧
|
Oh a small issue here, CodeRabbit is right about the consistency of So for now the proposed solution is to remove @serhalp what do you think ? |
Sukiiu
force-pushed
the
feat/like-comparaison
branch
from
e891e26 to
3526acf
Compare
|
Is encoding it even necessary because the like endpoint seems to capture the entire path greedily @Sukiiu? Regardless, go for it! Consistency is good. Will mark this as draft until you're ready 🙏 |
No not needed, it works without the encoding, but it was more to keep consistency. Will keep it like that for now, so it's ready to go then |
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
Thanks for your first contribution, @Sukiiu! 💫 We'd love to welcome you to the npmx community. Come and say hi on Discord! And once you've joined, visit npmx.wamellow.com to claim the contributor role. |
4 participants
Following this msg on discord :
This PR add like as a metric (I don't really know if it's a relevant metric)