fix: show correct dependencies count #787
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
gameroman
force-pushed
the
fix-compare-deps
branch
from
c4d0d0f to
ef62441
Compare
gameroman
force-pushed
the
fix-compare-deps
branch
from
ef62441 to
f05a7fc
Compare
|
would you update the tests? 🙏 |
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
|
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughA new numeric field Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
app/composables/usePackageComparison.ts (2)
370-397:⚠️ Potential issue | 🟠 MajorWrap switch cases in blocks and correctly handle zero dependencies.
The case statements lack blocks, triggering Biome's
noSwitchDeclarationsrule. Additionally,if (!data.directDeps)incorrectly filters out valid0values; useif (data.directDeps == null)to check only for null/undefined. Wrap'dependencies','deprecated', and'totalDependencies'cases in blocks.🧩 Proposed fix
- case 'dependencies': - if (!data.directDeps) return null - const depCount = data.directDeps - return { - raw: depCount, - display: String(depCount), - status: depCount > 10 ? 'warning' : 'neutral', - } + case 'dependencies': { + if (data.directDeps == null) return null + const depCount = data.directDeps + return { + raw: depCount, + display: String(depCount), + status: depCount > 10 ? 'warning' : 'neutral', + } + } - case 'deprecated': - const isDeprecated = !!data.metadata?.deprecated - return { - raw: isDeprecated, - display: isDeprecated - ? t('compare.facets.values.deprecated') - : t('compare.facets.values.not_deprecated'), - status: isDeprecated ? 'bad' : 'good', - } + case 'deprecated': { + const isDeprecated = !!data.metadata?.deprecated + return { + raw: isDeprecated, + display: isDeprecated + ? t('compare.facets.values.deprecated') + : t('compare.facets.values.not_deprecated'), + status: isDeprecated ? 'bad' : 'good', + } + } - case 'totalDependencies': - if (!data.installSize) return null - const totalDepCount = data.installSize.dependencyCount - return { - raw: totalDepCount, - display: String(totalDepCount), - status: totalDepCount > 50 ? 'warning' : 'neutral', - } + case 'totalDependencies': { + if (!data.installSize) return null + const totalDepCount = data.installSize.dependencyCount + return { + raw: totalDepCount, + display: String(totalDepCount), + status: totalDepCount > 50 ? 'warning' : 'neutral', + } + }
116-150:⚠️ Potential issue | 🟠 MajorRemove the
usePackagecall; compute direct deps from the already-fetchedversionData.Calling
usePackageinside an async callback violates Nuxt 3's requirement that composables be called synchronously in setup context. As a result,pkg.data.valueisundefinedhere, collapsingrequestedVersiontoundefined, and you lose the dependency count. You already have the version data from the registry response, so passversionDatadirectly togetDependencyCountinstead.Proposed fix
- const pkg = usePackage(name, latestVersion) - const requestedVersion = pkg.data.value?.requestedVersion - - const versionData = pkgData.versions[latestVersion] + const versionData = pkgData.versions[latestVersion] const packageSize = versionData?.dist?.unpackedSize @@ - directDeps: getDependencyCount(requestedVersion ?? null), + directDeps: getDependencyCount(versionData ?? null),
|
Thank you for coderabbit |
|
I think it is ready now |
|
would you also see whether there are any improvements to include from #868 ? I plan to merge this PR with a credit to @shamilkotta as they missed that you had already submitted this... |
|
I was thinking if |
Yes, will check and apply any improvements from that PR |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/composables/usePackageComparison.ts (1)
235-240:⚠️ Potential issue | 🟡 MinorFix loading state for dependency facets.
dependenciesnow uses fastdirectDeps, butisFacetLoadingstill treats it as slow and doesn’t includetotalDependencies, which is slow. This will show a misleading loading state.🔧 Proposed fix
- return facet === 'installSize' || facet === 'dependencies' + return facet === 'installSize' || facet === 'totalDependencies'Also applies to: 368-395
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
awesome, thank you for fixing this! 🙌🏼 |
3 participants
fixes #857
closes #868