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 3 commits intonpmx-dev:mainfrom
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
|
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:
📝 WalkthroughWalkthroughThe pull request adds support for displaying governance member profiles with biographical information and social media links on the about page. It introduces a new 🚥 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)
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✅ All modified and coverable lines are covered by tests. 📢 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
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
i18n/locales/de-DE.json
Outdated
| "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
i18n/locales/uk-UA.json
Outdated
| "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."
|
Looks like your rebase/merge got a little messed up? @howwohmm |
Expand governance member cards on the about page to show: - Bio/description line from GitHub profile - Social identity links (X/Twitter, Bluesky, Mastodon, LinkedIn, etc.) The existing `fetchSponsorable` GraphQL query is replaced with a broader `fetchGovernanceProfiles` that fetches bio, twitterUsername, and socialAccounts in the same request. Social icons use the existing UnoCSS icon sets (simple-icons + lucide). Data gracefully degrades when the GitHub token is unavailable. Closes npmx-dev#1564 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
howwohmm
force-pushed
the
feat/governance-card-identity
branch
from
c83970c to
bf0f030
Compare
|
rebased onto latest main — should be clean now. let me know if you spot anything else. |
There was a problem hiding this comment.
♻️ Duplicate comments (3)
app/pages/about.vue (1)
276-285:⚠️ Potential issue | 🟡 MinorLocalise the social-link labels.
These
aria-labels are still hard-coded English and derive the provider name straight from the enum value, so screen readers will announce odd strings such as “hometown”. Please route this through$t(...)and a provider display map instead oftoLowerCase().server/api/contributors.get.ts (2)
118-147:⚠️ Potential issue | 🟠 MajorHandle GitHub GraphQL
errorsexplicitly.GitHub can return HTTP 200 with an
errorsarray and partial or missingdata. This parser ignores that field entirely, so broken profile fetches get cached as if they succeeded and the cards quietly lose their extra metadata for an hour. Please parse and logjson.errorsbefore accepting the payload.GitHub GraphQL API response format: can a successful HTTP 200 response still include an `errors` array alongside `data`, and how should clients handle partial `data` plus `errors`?As per coding guidelines, "Use error handling patterns consistently".
218-228:⚠️ Potential issue | 🟠 MajorTokenless mode still removes the sponsor CTA.
When
githubTokenis absent,governanceProfilesis always empty, sosponsors_urlbecomesnullfor every governance member. That is the root cause of the sponsor link disappearing in preview/tokenless environments, which does not match the PR’s graceful-degradation goal. Please preserve a non-GraphQL fallback here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8ca3314b-3e38-424f-bb9a-00d5045db992
📒 Files selected for processing (2)
app/pages/about.vueserver/api/contributors.get.ts
- Merge twitterUsername into socialAccounts array (per ghostdevv feedback) - Simplify getSocialLinks — single unified array, no special-casing Closes npmx-dev#1564
howwohmm
force-pushed
the
feat/governance-card-identity
branch
from
7e191a0 to
d79af98
Compare
|
good call — moved |
3 participants
Closes #1564
Adds bio and social links to governance member cards on the about page.
The
fetchSponsorableGraphQL query is replaced withfetchGovernanceProfileswhich fetchesbio,twitterUsername, andsocialAccountsin one request. Twitter is normalised into thesocialAccountsarray server-side so the client just maps over a single unified array.Falls back gracefully when no GitHub token is available — cards still show login, role, and sponsor link.