feat(ui): add description and identity claims to governance cards#2198
feat(ui): add description and identity claims to governance cards#2198howwohmm wants to merge 1 commit intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
1 Skipped Deployment
|
📝 WalkthroughWalkthroughThe PR adds social-account support and bios for governance contributors. Frontend changes (app/pages/about.vue) introduce provider→icon/name lookups and helpers to derive ordered Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
server/api/contributors.get.ts (1)
222-233: Return a fresh object instead of mutatingcand casting it.
Object.assign(c, ...)changes aGitHubAPIContributorinto a wider shape and then hides the mismatch with a cast. Returning a new object keeps this mapper genuinely type-safe and easier to extend.♻️ Proposed refactor
return filtered - .map(c => { + .map((c): GitHubContributor & { order: number } => { const { role, order } = getRoleInfo(c.login, teams) const profile = governanceProfiles.get(c.login) const sponsors_url = profile?.hasSponsorsListing ? `https://github.com/sponsors/${c.login}` : null const bio = profile?.bio ?? null const twitterUsername = profile?.twitterUsername ?? null const socialAccounts = profile?.socialAccounts ?? [] - Object.assign(c, { role, order, sponsors_url, bio, twitterUsername, socialAccounts }) - return c as GitHubContributor & { order: number } + return { + ...c, + role, + order, + sponsors_url, + bio, + twitterUsername, + socialAccounts, + } })As per coding guidelines, "Ensure you write strictly type-safe code".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1b543fcc-a4bf-42e0-a636-4f3779dfe564
📒 Files selected for processing (2)
app/pages/about.vueserver/api/contributors.get.ts
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
server/api/contributors.get.ts
Outdated
| twitterUsername: string | null | ||
| socialAccounts: SocialAccount[] |
There was a problem hiding this comment.
twitter/x should just be in this social accounts array too
|
addressed your feedback:
the sponsors_url fallback is correct as-is — when no token is available, profile is undefined so null is returned gracefully. |
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 (2)
app/pages/about.vue (1)
269-290: Avoid callinggetSocialLinks(person)twice per card.The function is invoked both for the
v-ifcheck (line 270) and thev-forloop (line 274), creating redundant iterations for each governance member. Consider extracting the result to avoid duplicate computation.♻️ Suggested refactor using a computed or inline variable
One option is to use a small helper component or restructure to compute once. Alternatively, if you want to keep it inline, you could use a
v-forwith a length check:- <div - v-if="getSocialLinks(person).length" - class="relative z-10 flex items-center gap-1" - > - <a - v-for="link in getSocialLinks(person)" + <template + v-for="(links, index) in [getSocialLinks(person)]" + :key="index" + > + <div + v-if="links.length" + class="relative z-10 flex items-center gap-1" + > + <a + v-for="link in links"Or extract to a computed property that maps all governance members to their social links.
server/api/contributors.get.ts (1)
225-244: Consider restructuring to avoid object spread inmap.The static analysis tool flags spreading to modify object properties in
mapcalls as inefficient. While the impact is minimal for this array size, you could restructure to directly construct the object.♻️ Suggested refactor
return filtered - .map((c): GitHubContributor & { order: number } => { + .map((c): GitHubContributor & { order: number } => ({ const { role, order } = getRoleInfo(c.login, teams) const profile = governanceProfiles.get(c.login) - // When profile data is unavailable (e.g., no token), sponsors_url is null. - const sponsors_url = profile + login: c.login, + id: c.id, + avatar_url: c.avatar_url, + html_url: c.html_url, + contributions: c.contributions, + role, + order, + sponsors_url: profile ? profile.hasSponsorsListing ? `https://github.com/sponsors/${c.login}` : null - : null - const bio = profile?.bio ?? null - const socialAccounts = profile?.socialAccounts ?? [] - return { - ...c, - role, - order, - sponsors_url, - bio, - socialAccounts, - } - }) + : null, + bio: profile?.bio ?? null, + socialAccounts: profile?.socialAccounts ?? [], + }))Alternatively, keep the current structure if readability is preferred over the minor performance gain.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c58344d3-fb93-47bc-97be-2ce178a6c9ef
📒 Files selected for processing (3)
app/pages/about.vuei18n/locales/en.jsonserver/api/contributors.get.ts
✅ Files skipped from review due to trivial changes (1)
- i18n/locales/en.json
| // Fall back to REST-derived sponsors URL so the CTA remains available | ||
| // even in tokenless (preview) environments where GraphQL is skipped. | ||
| const sponsors_url = profile | ||
| ? profile.hasSponsorsListing | ||
| ? `https://github.com/sponsors/${c.login}` | ||
| : null | ||
| : null |
There was a problem hiding this comment.
Misleading comment: no REST-derived fallback exists.
The comment states "Fall back to REST-derived sponsors URL" but when profile is undefined (tokenless environments), sponsors_url is set to null. There's no actual REST-derived fallback implemented. Either remove the misleading comment or implement a fallback (e.g., constructing the URL directly since the GitHub REST contributors endpoint doesn't include sponsor info).
🔧 Suggested fix: clarify the comment
- // Fall back to REST-derived sponsors URL so the CTA remains available
- // even in tokenless (preview) environments where GraphQL is skipped.
+ // When profile data is unavailable (e.g., no token), sponsors_url is null.
+ // The sponsor CTA will not be shown in tokenless environments.📝 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.
| // Fall back to REST-derived sponsors URL so the CTA remains available | |
| // even in tokenless (preview) environments where GraphQL is skipped. | |
| const sponsors_url = profile | |
| ? profile.hasSponsorsListing | |
| ? `https://github.com/sponsors/${c.login}` | |
| : null | |
| : null | |
| // When profile data is unavailable (e.g., no token), sponsors_url is null. | |
| // The sponsor CTA will not be shown in tokenless environments. | |
| const sponsors_url = profile | |
| ? profile.hasSponsorsListing | |
| ? `https://github.com/sponsors/${c.login}` | |
| : null | |
| : null |
Display contributor bio and social account links with provider icons on the about page governance cards. Twitter/X is part of socialAccounts rather than a separate field. Dynamic icon classes are safelisted in UnoCSS config. Fixes npmx-dev#2197
howwohmm
force-pushed
the
feat/governance-card-identity
branch
from
125f9e9 to
c83970c
Compare
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
i18n/locales/te-IN.json (1)
16-171:⚠️ Potential issue | 🟠 MajorPlease do not ship
EN TEXT TO REPLACEliterals in this locale.These keys will render verbatim for Telugu users instead of falling back, including the new governance/social copy added in this PR. If the translations are not ready yet, it is safer to omit the unfinished entries from this locale file for now.
Based on learnings, "new or changed entries in i18n translation files (locale JSON files) may be omitted from non-English languages."
Also applies to: 181-246, 260-388, 393-559, 582-676, 798-843, 879-941, 998-1010, 1032-1034, 1068-1069, 1090-1434
♻️ Duplicate comments (1)
server/api/contributors.get.ts (1)
220-234:⚠️ Potential issue | 🟠 MajorTokenless fallback still drops the sponsor action.
When
githubTokenis absent,governanceProfilesis always empty, soprofileisundefinedandsponsors_urlstaysnullfor every governance member. That means preview/tokenless deployments cannot show the sponsor link on governance cards, and the inline comment is misleading because there is no REST-derived fallback in this branch. If the UI still needs that CTA in tokenless environments, this branch needs a deterministic fallback instead ofnull.
🧹 Nitpick comments (1)
app/pages/about.vue (1)
250-254: Only set the biotitlewhen truncation actually happens.At the moment every bio gets a browser tooltip, even when the full text is already visible. If the intended behaviour is “tooltip on overflow”, bind
titleonly after an overflow check.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c0f936ac-22a3-4979-8191-8a34cd6f6fe1
📒 Files selected for processing (31)
app/pages/about.vuei18n/locales/ar.jsoni18n/locales/az-AZ.jsoni18n/locales/bg-BG.jsoni18n/locales/bn-IN.jsoni18n/locales/cs-CZ.jsoni18n/locales/de-DE.jsoni18n/locales/en.jsoni18n/locales/es.jsoni18n/locales/fr-FR.jsoni18n/locales/hi-IN.jsoni18n/locales/hu-HU.jsoni18n/locales/id-ID.jsoni18n/locales/it-IT.jsoni18n/locales/ja-JP.jsoni18n/locales/kn-IN.jsoni18n/locales/mr-IN.jsoni18n/locales/nb-NO.jsoni18n/locales/ne-NP.jsoni18n/locales/pl-PL.jsoni18n/locales/pt-BR.jsoni18n/locales/ru-RU.jsoni18n/locales/ta-IN.jsoni18n/locales/te-IN.jsoni18n/locales/tr-TR.jsoni18n/locales/uk-UA.jsoni18n/locales/zh-CN.jsoni18n/locales/zh-TW.jsoni18n/schema.jsonserver/api/contributors.get.tsuno.config.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- uno.config.ts
| "trademark_disclaimer": "npm علامة تجارية مسجلة لشركة npm, Inc. هذا الموقع غير مرتبط بشركة npm, Inc.", | ||
| "footer": { | ||
| "about": "حول", | ||
| "blog": "EN TEXT TO REPLACE: blog", |
There was a problem hiding this comment.
Prefer locale fallback over committed placeholder copy.
Several newly added keys still contain EN TEXT TO REPLACE: values. Because those keys now exist in this locale, users will see the sentinel verbatim instead of the normal fallback, including the new governance-card label under about.team.social_link_aria. Please either add real Arabic strings or drop these entries until the translations are ready.
Based on learnings, non-English locale files in this repo may omit new keys and rely on fallback translations, so in-band placeholder values are not required.
Also applies to: 91-114, 1010-1010, 1407-1434
| "open_main": "EN TEXT TO REPLACE: Open main information", | ||
| "open_diff": "EN TEXT TO REPLACE: Open version differences" |
There was a problem hiding this comment.
Please avoid committing EN TEXT TO REPLACE strings in this locale.
If the copy is intentionally deferred, omitting these keys is better than shipping placeholder text. As it stands, localised users — and screen readers reading about.team.social_link_aria — will see the placeholder verbatim.
Based on learnings, new or changed entries in i18n translation files may be omitted from non-English languages; translations are not completed in-band in the same PR and are tracked elsewhere.
Also applies to: 99-99, 142-168, 217-229, 309-336, 397-445, 457-457, 482-482, 514-514, 536-537, 559-559, 635-637, 842-843, 905-906, 941-941, 1010-1010, 1090-1109, 1221-1266, 1407-1433
| "trademark_disclaimer": "npm е регистрирана търговска марка на npm, Inc. Този сайт не е свързан с npm, Inc.", | ||
| "footer": { | ||
| "about": "за нас", | ||
| "blog": "EN TEXT TO REPLACE: blog", |
There was a problem hiding this comment.
Remove the remaining placeholder copy before shipping this locale.
This file still contains several EN TEXT TO REPLACE: values. That includes about.team.social_link_aria, which app/pages/about.vue:273-288 uses directly for the new governance social-link ARIA label, so assistive tech would announce the placeholder verbatim. Please translate these entries or drop them so the fallback text is used until the Bulgarian copy is ready.
Based on learnings, non-English locale files in this repo may omit new keys and rely on fallback translations, so in-band placeholder values are not required.
Also applies to: 91-114, 1010-1010, 1220-1266, 1407-1434
| "blog": "EN TEXT TO REPLACE: blog", | ||
| "docs": "ডকুমেন্টেশন", | ||
| "source": "সোর্স", | ||
| "social": "সোশ্যাল", | ||
| "chat": "চ্যাট" | ||
| "chat": "চ্যাট", | ||
| "builders_chat": "EN TEXT TO REPLACE: builders", | ||
| "keyboard_shortcuts": "EN TEXT TO REPLACE: keyboard shortcuts" |
There was a problem hiding this comment.
Please drop the placeholder translations from this file.
EN TEXT TO REPLACE: is still present in newly added Bengali entries, so those strings will be shown verbatim to users and screen readers. The new governance social-link ARIA text on Line 1009 is affected as well. Either add the real translations here or omit these keys until localisation is done.
Based on learnings: In the npmx.dev project, new or changed entries in i18n translation files may be omitted from non-English languages. Translations are not completed in-band in the same PR and are tracked elsewhere.
Also applies to: 1009-1009, 1406-1433
| "open_main": "EN TEXT TO REPLACE: Open main information", | ||
| "open_diff": "EN TEXT TO REPLACE: Open version differences" |
There was a problem hiding this comment.
Please avoid committing EN TEXT TO REPLACE strings in this locale.
If the copy is intentionally deferred, omitting these keys is better than shipping placeholder text. As it stands, localised users — and screen readers reading about.team.social_link_aria — will see the placeholder verbatim.
Based on learnings, new or changed entries in i18n translation files may be omitted from non-English languages; translations are not completed in-band in the same PR and are tracked elsewhere.
Also applies to: 99-99, 142-168, 217-229, 309-336, 397-445, 457-457, 482-482, 514-514, 536-537, 559-559, 635-637, 842-843, 905-906, 941-941, 1010-1010, 1090-1090, 1108-1109, 1221-1266, 1407-1433
| "sponsor_aria": "Patrocinador {name} no GitHub", | ||
| "social_link_aria": "EN TEXT TO REPLACE: {name} on {platform}" |
There was a problem hiding this comment.
Replace the placeholder locale text before merging.
about.team.social_link_aria is used by the new icon-only links on /about, so this locale will currently expose EN TEXT TO REPLACE to users and screen readers. The same placeholder marker is present in other changed keys in this file as well. If these translations are not ready yet, please drop the unfinished entries and let the existing English fallback handle them instead.
Based on learnings, "In the npmx.dev project, new or changed entries in i18n translation files (locale JSON files) may be omitted from non-English languages. Translations are not completed in-band in the same PR and are tracked elsewhere. It is acceptable for non-English locale files to be missing keys that exist in English locale files."
Also applies to: 1407-1426
| "open_main": "EN TEXT TO REPLACE: Open main information", | ||
| "open_diff": "EN TEXT TO REPLACE: Open version differences" |
There was a problem hiding this comment.
Remove placeholder locale strings before merge.
This file still checks in literal EN TEXT TO REPLACE: values. Those are treated as real translations, so Turkish users and screen readers will see the placeholder text verbatim — including the new governance social-link ARIA label on Line 1010. Please either provide real translations or omit these entries until localisation catches up.
Based on learnings: In the npmx.dev project, new or changed entries in i18n translation files may be omitted from non-English languages. Translations are not completed in-band in the same PR and are tracked elsewhere.
Also applies to: 1010-1010, 1407-1434
| "sponsor_aria": "Спонсорувати {name} на GitHub", | ||
| "social_link_aria": "EN TEXT TO REPLACE: {name} on {platform}" |
There was a problem hiding this comment.
Don't ship placeholder copy for the new social-link ARIA label.
about.team.social_link_aria will render the literal EN TEXT TO REPLACE... string for Ukrainian users, and because the key exists they will not fall back to the default locale. Please either provide the final translation here or omit the key until localisation lands.
Based on learnings, "It is acceptable for non-English locale files to be missing keys that exist in English locale files."
| }, | ||
| "draft_badge": "草稿", | ||
| "draft_banner": "这是一篇未发布的草稿。内容可能不完整或包含不准确的信息。", | ||
| "no_posts": "EN TEXT TO REPLACE: No posts found.", |
There was a problem hiding this comment.
Avoid shipping raw placeholder markers in zh-CN.
There are still multiple EN TEXT TO REPLACE literals here, including about.team.social_link_aria for the new governance social links. Since missing non-English keys are acceptable in this repo, dropping unfinished entries is better than surfacing placeholder markers to Simplified Chinese users.
Based on learnings, "new or changed entries in i18n translation files (locale JSON files) may be omitted from non-English languages."
Also applies to: 150-168, 228-229, 309-336, 421-425, 457-457, 559-559, 635-636, 906-906, 1010-1010, 1221-1227, 1256-1266, 1407-1427
| }, | ||
| "draft_badge": "草稿", | ||
| "draft_banner": "這是尚未發佈的草稿。內容可能不完整或包含不準確的資訊。", | ||
| "no_posts": "EN TEXT TO REPLACE: No posts found.", |
There was a problem hiding this comment.
Avoid shipping raw placeholder markers in zh-TW.
Several new keys are still EN TEXT TO REPLACE literals, including about.team.social_link_aria for the governance social icons. Because the key exists, users will see the marker text rather than a normal fallback. Please translate these values or drop the unfinished keys for now.
Based on learnings, "new or changed entries in i18n translation files (locale JSON files) may be omitted from non-English languages."
Also applies to: 150-168, 228-229, 309-336, 421-425, 457-457, 559-559, 635-636, 906-906, 1010-1010, 1090-1090, 1221-1227, 1256-1266, 1407-1427
|
Looks like your rebase/merge got a little messed up? @howwohmm |
3 participants
Summary
Closes #1564
fetchSponsorableGraphQL query with a broaderfetchGovernanceProfilesthat fetchesbio,twitterUsername, andsocialAccountsin a single request — no extra API callsWhat changed
server/api/contributors.get.tsSocialAccountinterface exported alongsideRoleandGitHubContributorGitHubContributornow includesbio,twitterUsername, andsocialAccountsfieldsfetchSponsorable→fetchGovernanceProfiles: same single GraphQL batch query, now also fetchesbio,twitterUsername, andsocialAccounts(first: 10)app/pages/about.vuesocialIcons) andgetSocialLinks()helper to deduplicate Twitter username vs social accountsi-simple-icons:*andi-lucide:*icon setsTest plan
🤖 Generated with Claude Code