feat: add GitHub contributors graph#1445
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
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
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
Tobbe
force-pushed
the
tobbe-feat-gh-contrib-graph
branch
from
752f464 to
dc34dc7
Compare
|
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:
📝 WalkthroughWalkthroughThis PR adds per-package repository resolution and a new "contributors" metric to the package trends chart. It introduces a Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 1 | ❌ 1❌ Failed checks (1 warning)
✅ 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}
| <PackageWeeklyDownloadStats | ||
| :packageName | ||
| :createdIso="pkg?.time?.created ?? null" | ||
| :repoRef="repoRef" | ||
| /> |
There was a problem hiding this comment.
Fix repoRef nullability for PackageWeeklyDownloadStats.
repoRef can be null from useRepoMeta, but the child prop expects RepoRef | undefined, causing the reported type check failure. Normalise null to undefined (or widen the prop type).
🛠️ Suggested fix
- :repoRef="repoRef"
+ :repoRef="repoRef ?? undefined"📝 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.
| <PackageWeeklyDownloadStats | |
| :packageName | |
| :createdIso="pkg?.time?.created ?? null" | |
| :repoRef="repoRef" | |
| /> | |
| <PackageWeeklyDownloadStats | |
| :packageName | |
| :createdIso="pkg?.time?.created ?? null" | |
| :repoRef="repoRef ?? undefined" | |
| /> |
🧰 Tools
🪛 GitHub Check: 💪 Type check
[failure] 1371-1371:
Type 'RepoRef | null' is not assignable to type 'RepoRef | undefined'.
Tobbe
changed the title
|
When switching to monthly or yearly, the component is designed to display estimations for incomplete periods. The facet does not appear to be working on the compare page. I would also move the fetching to the useCharts composable |
Yes, I did find it a bit weird that it was estimating how many contributors a package will have 😀 How should we handle the incomplete period though? 🤔 |
We are dealing with absolutes. n contributors today does not add up with n yesterday. So weekly, monthly, yearly should be averages. |
Yeah, I hadn't gotten around to fully testing that one yet. But I also need to find a package that doesn't have a GitHub repo in its package.json to test with. |
|
I faked one not having a github link, just to be able to test how it'd work. 
|
Likes also isn't additive, right? So maybe likes and contributors should be handled differently compared to downloads. |
|
@graphieros Can we mark this as ready to review again? |
|
I don't think it should be shipped with estimations for contributors. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/components/Package/TrendsChart.vue (1)
1662-1667:⚠️ Potential issue | 🟡 MinorRemove inline focus-visible utility on the reset button.
Please rely on the global button focus-visible styling and drop the per‑element
focus-visible:outline-accent/70class here.🛠️ Suggested fix
- class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border focus-visible:outline-accent/70 sm:mb-0" + class="self-end flex items-center justify-center px-2.5 py-1.75 border border-transparent rounded-md text-fg-subtle hover:text-fg transition-colors hover:border-border sm:mb-0"Based on learnings: "In the npmx.dev project, ensure that focus-visible styling for button and select elements is implemented globally in app/assets/main.css ... Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements."
🧹 Nitpick comments (1)
app/pages/compare.vue (1)
253-258: Edge case: fallback may show a loading spinner when status is'success'but data is absent.The
v-elsebranch displays a loading spinner for any state not explicitly handled. Ifstatusis'success'yetpackagesDataremainsnullor all-null (an unlikely but possible race or upstream bug), users would see an indefinite spinner rather than an informative message.Consider whether a "No data available" state or an additional explicit check for
status === 'success'would provide clearer feedback.
| const METRICS = computed<MetricDef[]>(() => { | ||
| const metrics: MetricDef[] = [ | ||
| { | ||
| id: 'downloads', | ||
| label: $t('package.trends.items.downloads'), | ||
| fetch: ({ packageName }, opts) => | ||
| fetchPackageDownloadEvolution( | ||
| packageName, | ||
| props.createdIso ?? null, | ||
| opts, | ||
| ) as Promise<EvolutionData>, | ||
| supportsMulti: true, | ||
| }, | ||
| { | ||
| id: 'likes', | ||
| label: $t('package.trends.items.likes'), | ||
| fetch: ({ packageName }, opts) => fetchPackageLikesEvolution(packageName, opts), | ||
| supportsMulti: true, | ||
| }, | ||
| ] | ||
|
|
||
| if (hasContributorsFacet.value) { | ||
| metrics.push({ | ||
| id: 'contributors', | ||
| label: $t('package.trends.items.contributors'), | ||
| fetch: ({ repoRef }, opts) => fetchRepoContributorsEvolution(repoRef, opts), | ||
| supportsMulti: true, | ||
| }) | ||
| } | ||
|
|
||
| return metrics | ||
| }) | ||
|
|
||
| const selectedMetric = usePermalink<MetricId>('facet', DEFAULT_METRIC_ID, { | ||
| permanent: props.permalink, | ||
| }) | ||
|
|
||
| const effectivePackageNamesForMetric = computed<string[]>(() => { | ||
| if (!isMultiPackageMode.value) return effectivePackageNames.value | ||
| if (selectedMetric.value !== 'contributors') return effectivePackageNames.value | ||
| return effectivePackageNames.value.filter( | ||
| name => repoRefsByPackage.value[name]?.provider === 'github', | ||
| ) | ||
| }) | ||
|
|
||
| const skippedPackagesWithoutGitHub = computed(() => { | ||
| if (!isMultiPackageMode.value) return [] | ||
| if (selectedMetric.value !== 'contributors') return [] | ||
| if (!effectivePackageNames.value.length) return [] | ||
|
|
||
| return effectivePackageNames.value.filter( | ||
| name => repoRefsByPackage.value[name]?.provider !== 'github', | ||
| ) | ||
| }) | ||
|
|
||
| const availableGranularities = computed<ChartTimeGranularity[]>(() => { | ||
| if (selectedMetric.value === 'contributors') { | ||
| return ['weekly', 'monthly', 'yearly'] | ||
| } | ||
|
|
||
| return ['daily', 'weekly', 'monthly', 'yearly'] | ||
| }) |
There was a problem hiding this comment.
Disable estimation/extrapolation for contributors.
Contributors are absolute counts, so the monthly/yearly extrapolation and estimation overlay will inflate the latest period and mislead. Gate those behaviours on selectedMetric !== 'contributors'.
🛠️ Suggested fix
-const isEstimationGranularity = computed(
- () => displayedGranularity.value === 'monthly' || displayedGranularity.value === 'yearly',
-)
+const isEstimationGranularity = computed(
+ () =>
+ selectedMetric.value !== 'contributors'
+ && (displayedGranularity.value === 'monthly' || displayedGranularity.value === 'yearly'),
+) function extrapolateLastValue(lastValue: number) {
+ if (selectedMetric.value === 'contributors') return lastValue
if (displayedGranularity.value !== 'monthly' && displayedGranularity.value !== 'yearly')
return lastValue
I also agree that it's wrong. You said
So just to make sure I understand what you mean, if I select "yearly" as the period, it should show the average so far for 2026 as the value for the full year? And same for "monthly", it should show the average so far for February as the value for this month? |
ghostdevv
left a comment
ghostdevv
left a comment
There was a problem hiding this comment.
I think I agree with @graphieros we should fix the data prediction issue before merging, especially as there isn't a lot of time before recharging begins - will defer to him on that since he is the graph guy :p
but very exciting!!
Yes that's right. An average of unique contributors, if this data is available of course. |
|
@graphieros I removed the extrapolation for Contributors. |
|
@ghostdevv you requested changes. Is this good enough? I.e. no estimation, just current total 
|
|
Looks good to me now ! |
|
@graphieros Thanks for working with me on this one 🙏 |
|
@Tobbe I recommend you merge main in your branch, since vue-data-ui version was bumped and minor config details updated on the chart file |
|
Ok, looks mergeable to me |
Yeah, I was just going to say it looks good :) 
|
|
@graphieros What's the setup like here, do we need to wait for @ghostdevv to review again? |
|
Yep ;) |
3 participants
I started working on this new graph. WDYT?
I wanted to be able to see how "healthy", from a number-of-contibutors perspective a package is.
Like, even if a package has hundreds of contributors, maybe those are all from years past, and the package is now basically only kept alive by one person, which isn't too good from a reliability/bus factor point of view
It uses the GitHub API, so only works for packages that have a github repo linked in the package metadata