feat: sort contributors list by role #1369
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
📝 WalkthroughWalkthroughThe PR adds role-aware contributor handling and sponsor detection to server/api/contributors.get.ts: introduces a Role type, team-member fetching (with fallback), sponsorable lookup, role determination, and returns contributors ordered by role and contributions while exposing 🚥 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)
⚔️ Resolve merge conflicts (beta)
Comment |
|
Note: I thought I was looking at an issue 🤦🏼
|
|
It would be good to also show for governance folk their github, npm, etc too. Maybe at one point we find a way to do it for everyone once we have more people connecting their accounts together. 
One interesting discussion, we decided to remove the sponsors button for stewards in the main page of the repo. It didn't feel right that we were the only ones having the exposure there. But maybe we could add them back for everybody (or as many of us as possible, maybe first governance, later when people connect to github we could do better). I think we need to start trying to start working to help with OSS funding. I don't think people will give us a lot of money through this, I care more about the symbol that we should show that money is needed for npmx to work. It will also get better once we can start adding sponsors (soon!). |
|
Now that I know this is a PR, I checked how it looks :D So, we could merge this and iterated, but I would like we show the governance structure as a way to explain that npmx takes the foundation route seriously. So we should have Stewards first as a title, then Maintainers, then everyone else. Maybe with a short paragraph. And it could give us an option to do bigger cards for maintainers so we can show that they work in different companies and what other projects they participate in. Let's use this as a way to show that we are the result of uniting a lot of communities. |
|
It actually is a PR😄 Yes, this can be imrpoved iteratively. I also think about adding sections similar to https://github.com/npmx-dev/npmx.dev/blob/main/GOVERNANCE.md and we could add links to this docs later. |
|
Also, once npmx's profile page mature, we could link to the profile page |
|
I'm thinking that for the first iteration, until we have a core team, the best is if we only have two sections: TeamGovernanceDaniel Me [ ...maintainers ] <-- all with the same card size, just the role is diff ContributorsAll other contributors, maybe including the above too as a way to show we are all part of the team |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
server/api/contributors.get.ts (1)
1-8:⚠️ Potential issue | 🟡 MinorType mismatch between API response and interface
The
GitHubContributorinterface now requires arolefield, but the GitHub API response at line 49 does not include this property. Casting directly toGitHubContributor[]is type-unsafe since the objects won't haveroleuntil line 72.Consider defining a separate type for the raw API response:
🛡️ Proposed fix for type safety
+type GitHubAPIContributor = Omit<GitHubContributor, 'role'> + export interface GitHubContributor { login: string id: number avatar_url: string html_url: string contributions: number role: Role }Then at line 49:
- const contributors = (await response.json()) as GitHubContributor[] + const contributors = (await response.json()) as GitHubAPIContributor[]And update the
allContributorsarray type accordingly.Also applies to: 49-49
🧹 Nitpick comments (3)
app/pages/about.vue (3)
147-147: Consider extracting the governance filter to a computed propertyThe filter
c => c.role !== 'contributor'is used in multiple places (lines 147, 156–161). Extracting this to a computed property would improve readability and ensure consistency.♻️ Suggested refactor
In
<script setup>:const governanceMembers = computed(() => contributors.value?.filter(c => c.role !== 'contributor') ?? [] )Then in the template:
- <div v-if="contributors?.some(c => c.role !== 'contributor')" class="mb-12"> + <div v-if="governanceMembers.length" class="mb-12">- v-for="person in contributors.filter(c => c.role !== 'contributor')" + v-for="person in governanceMembers"
141-144: Consider using i18n keys for the "Team" headingThe hardcoded "Team" string at line 141 should use an i18n key for consistency with the rest of the page, which uses
$t()for all user-facing text.
148-148: Consider using i18n key for "Governance" headingSimilar to the "Team" heading, "Governance" should use an i18n key for internationalisation consistency.
|
I've created a simple mini cards (icon + name + role) for governance section as a starting point. Should I use this same card format for all contributors, or keep the current contributors cloud? I'll collect the maintainers GitHub handles from Discord and hardcode them (tomorrow). Also, could you provide the short paragraphs for each sections? I'm not sure what the best wordings. 
|
|
@shuuji3 I don't think we need the paragraph descriptions right now. Feel free to remove! |
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
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/api/contributors.get.ts (1)
24-54: Implement pagination for team members fetch to match contributors pattern.The
fetchTeamMembers()function fetches teams withper_page=100but without pagination. In contrast, the same file implements a pagination loop for the repository contributors fetch (lines 131–166). For consistency and to avoid silent data loss if either team grows beyond 100 members, apply the same pagination pattern here:♻️ Suggested implementation
for (const [role, slug] of Object.entries(teams) as [keyof TeamMembers, string][]) { + let page = 1 + const perPage = 100 + while (true) { - const response = await fetch( - `https://api.github.com/orgs/npmx-dev/teams/${slug}/members?per_page=100`, + const response = await fetch( + `https://api.github.com/orgs/npmx-dev/teams/${slug}/members?per_page=${perPage}&page=${page}`, { headers: { 'Accept': 'application/vnd.github.v3+json', 'Authorization': `Bearer ${token}`, 'User-Agent': 'npmx', }, }, ) - if (!response.ok) { + if (!response.ok) { console.warn(`Failed to fetch ${slug} team members: ${response.status}`) return null - } + } - const members = (await response.json()) as { login: string }[] - for (const member of members) { - result[role].add(member.login) - } + const members = (await response.json()) as { login: string }[] + for (const member of members) { + result[role].add(member.login) + } + if (members.length < perPage) break + page++ + } }
| <NuxtLink | ||
| :to="person.html_url" | ||
| target="_blank" | ||
| class="decoration-none after:content-[''] after:absolute after:inset-0" | ||
| :aria-label="$t('about.contributors.view_profile', { name: person.login })" | ||
| > | ||
| @{{ person.login }} |
There was a problem hiding this comment.
Add rel="noopener noreferrer" for external profile links opened in a new tab.
On Line 193, target="_blank" without a safe rel exposes window.opener and enables tabnabbing.
🔒 Suggested fix
<NuxtLink
:to="person.html_url"
target="_blank"
+ rel="noopener noreferrer"
class="decoration-none after:content-[''] after:absolute after:inset-0"
:aria-label="$t('about.contributors.view_profile', { name: person.login })"
>📝 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.
| <NuxtLink | |
| :to="person.html_url" | |
| target="_blank" | |
| class="decoration-none after:content-[''] after:absolute after:inset-0" | |
| :aria-label="$t('about.contributors.view_profile', { name: person.login })" | |
| > | |
| @{{ person.login }} | |
| <NuxtLink | |
| :to="person.html_url" | |
| target="_blank" | |
| rel="noopener noreferrer" | |
| class="decoration-none after:content-[''] after:absolute after:inset-0" | |
| :aria-label="$t('about.contributors.view_profile', { name: person.login })" | |
| > | |
| @{{ person.login }} |
|
Thanks for the update, Daniel🙂 It looks perfect to me. 
|
danielroe
left a comment
danielroe
left a comment
There was a problem hiding this comment.
great! we'll merge and we can make it better later 🙏
5 participants
An attempt to sort contributors list by the governance role (steward > core > maintainer > contributor).
I cannot find a public member list of the maintainers so kept it empty for now.
One option to avoid hardcoding is to create GitHub Team for each governance role, and create members list in GitHub Actions with command like
gh api /orgs/npmx-dev/teams/steward/members --jq 'map(.login)./cc @patak-dev @userquin
(ref. original chat: https://bsky.app/profile/patak.dev/post/3mek245m7as26)