Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
2 Skipped Deployments
|
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
|
📝 WalkthroughWalkthroughAdds a 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)
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: 6
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)
244-254:⚠️ Potential issue | 🟡 MinorType contract broken on the no-token path.
When
githubTokenis absent,userDatais empty anduserData.get(c.login) || {}yields{}. AfterObject.assigntheGitHubUserDatafields remainundefinedon the resulting object, yet the cast on Line 251 tells TypeScript they arestring | null. The template'sv-ifguards survive this at runtime, but it silently violates the interface contract and can confuse future callers.🛠️ Suggested fix — provide explicit null fallback
- const userInfo = userData.get(c.login) || {} + const userInfo: GitHubUserData = userData.get(c.login) ?? { + name: null, + bio: null, + company: null, + companyHTML: null, + location: null, + websiteUrl: null, + twitterUsername: null, + }
🧹 Nitpick comments (1)
app/pages/about.vue (1)
8-11:isMountedis never consumed; timer refs typed asany.
isMountedis set inonMountedbut is not referenced anywhere in the template or script — it is dead code. Additionally,shallowRef<any>for timer handles violates the project's strict type-safety guideline; these should be typed asReturnType<typeof setTimeout> | null.✏️ Suggested fix
-// SSR & Validation Fix -const isMounted = shallowRef(false) const activeContributor = shallowRef<GitHubContributor | null>(null) -const openTimer = shallowRef<any>(null) -const closeTimer = shallowRef<any>(null) +const openTimer = shallowRef<ReturnType<typeof setTimeout> | null>(null) +const closeTimer = shallowRef<ReturnType<typeof setTimeout> | null>(null) const isFlipped = shallowRef(false) - -onMounted(() => { - isMounted.value = true -})
# Conflicts: # app/components/Link/Base.vue
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
app/pages/about.vue (5)
10-11: Prefer a narrower type for timer refs instead ofany.Using
shallowRef<any>loses type safety.ReturnType<typeof setTimeout>is the idiomatic way to type timer handles in TypeScript.♻️ Suggested fix
-const openTimer = shallowRef<any>(null) -const closeTimer = shallowRef<any>(null) +const openTimer = shallowRef<ReturnType<typeof setTimeout> | null>(null) +const closeTimer = shallowRef<ReturnType<typeof setTimeout> | null>(null)As per coding guidelines: "Ensure you write strictly type-safe code" (
**/*.{ts,tsx,vue}).
84-88: Silent emptycatchblocks hide potential issues.Lines 86–87 and similarly lines 153–155 swallow all exceptions without logging. While this is likely intentional for browsers lacking Popover API support, a brief comment explaining the rationale would aid future maintainers.
♻️ Suggested improvement
try { ;(popover as any).showPopover() - } catch (e) {} + } catch { + // Popover API not supported — silently degrade + }
170-174: Inlinefocus-visibleutility on a<button>— should rely on the global rule.The button at Line 173 applies
focus-visible:outline-accent/70as an inline utility class. The project's globalmain.cssalready providesbutton:focus-visiblestyling, so this inline class either conflicts with or duplicates it.♻️ Suggested fix
- class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" + class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-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 in Vue templates."
305-307: Loading indicator and list can render simultaneously.The
<ul>usesv-if="contributors.length"(Line 306) independently of the pending/error guards above (Lines 292, 299 which usev-if/v-else-if). WhenuseLazyFetchrefreshes cached data, both the "Loading…" text and the stale list will appear at the same time. If this is intentional (showing stale-while-revalidate), consider adding a visual indicator (e.g. reduced opacity) on the list during refresh. If not, change Line 306 tov-else-if.
7-16: Remove the unusedisMountedref andonMountedhook.The
isMountedref is declared and set totrueinonMounted, but is never referenced anywhere else in the component. This is dead code and should be removed.Proposed removal
-// SSR & Validation Fix -const isMounted = shallowRef(false) const activeContributor = shallowRef<GitHubContributor | null>(null) const openTimer = shallowRef<any>(null) const closeTimer = shallowRef<any>(null) const isFlipped = shallowRef(false) -onMounted(() => { - isMounted.value = true -})server/api/contributors.get.ts (2)
121-124: GraphQL query interpolatesloginwithout escaping.Contributor logins are embedded directly into the GraphQL query string. While GitHub usernames are restricted to alphanumeric characters and hyphens (making exploitation near-impossible), escaping the
"character would add defence-in-depth against any unexpected input.🛡️ Suggested hardening
const fragments = logins.map( (login, i) => - `user${i}: user(login: "${login}") { hasSponsorsListing login name bio company companyHTML location websiteUrl twitterUsername }`, + `user${i}: user(login: "${login.replace(/"/g, '')}") { hasSponsorsListing login name bio company companyHTML location websiteUrl twitterUsername }`, )
81-94:sanitizeGitHubHTMLmay duplicatetarget/relon<a>tags that already have them.Line 89 unconditionally injects
target="_blank" rel="noopener noreferrer"into every<aopening tag. If GitHub's rendered HTML already includes these attributes, they'll appear twice. While browsers tolerate duplicates (using the first occurrence), it's slightly untidy.This is low-priority since the HTML source is GitHub's API, which currently doesn't include these attributes on company links.
…eam.sponsor_aria` to sponsor anchor
…opup-card' into userquin/feat-add-contributors-popup-card
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
app/pages/about.vue (1)
306-328: Expose the popover on keyboard focus as well.Hover-only access hides the expanded card from keyboard users; mirroring hover with focus/blur keeps it accessible without changing the visual behaviour.
♿ Suggested tweak
`@mouseenter`="onMouseEnter(contributor)" `@mouseleave`="onMouseLeave" + `@focus`="onMouseEnter(contributor)" + `@blur`="onMouseLeave"
| v-if="canGoBack" | ||
| type="button" | ||
| class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" | ||
| @click="router.back()" |
There was a problem hiding this comment.
Remove the per-button focus-visible utility class.
Buttons already inherit a global focus-visible style, so this extra utility is redundant and diverges from the shared rule.
🎯 Suggested fix
- class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0"
+ class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-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 with the rule: button:focus-visible, select:focus-visible { outline: 2px solid var(--accent); outline-offset: 2px; border-radius: 4px; }. Do not apply per-element inline utility classes like focus-visible:outline-accent/70 on these elements in Vue templates or components.
📝 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.
| v-if="canGoBack" | |
| type="button" | |
| class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded focus-visible:outline-accent/70 shrink-0" | |
| @click="router.back()" | |
| v-if="canGoBack" | |
| type="button" | |
| class="cursor-pointer inline-flex items-center gap-2 font-mono text-sm text-fg-muted hover:text-fg transition-colors duration-200 rounded shrink-0" | |
| `@click`="router.back()" |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
server/api/contributors.get.ts (1)
256-267:Object.assignmutates the original REST response object — prefer spreading into a new object.
Object.assign(c, ...)modifiescin-place, which is a surprising side-effect even thoughallContributorsis not referenced afterwards. Spreading into a fresh object is cleaner and removes the need for the broad cast (noting thatGitHubContributoralready declaressponsors_urlandrole, so the intersection type is redundant for those fields):♻️ Proposed refactor
- Object.assign(c, { role, order, sponsors_url, ...userInfo }) - return c as GitHubContributor & { order: number; sponsors_url: string | null; role: Role } + return { ...c, role, order, sponsors_url, ...userInfo } satisfies GitHubContributor & { order: number }
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
app/pages/about.vue (4)
22-30:isMountedis dead code — remove it.
isMountedis set inonMountedbut never read anywhere;<ClientOnly>already gates the popover to the client. The ref and its lifecycle hook can be dropped.♻️ Proposed removal
-const isMounted = shallowRef(false) const activeContributor = shallowRef<GitHubContributor | null>(null) const openTimer = shallowRef<ReturnType<typeof setTimeout> | undefined>() const closeTimer = shallowRef<ReturnType<typeof setTimeout> | undefined>() const isFlipped = shallowRef(false) - -onMounted(() => { - isMounted.value = true -}) - onBeforeUnmount(() => {
439-441: Mixed border styling: hardcoded colours alongside a design token.
border-t-gray-400/65 dark:border-t-gray-300andborder-border-subtleare applied to the same element. The hardcoded values are redundant; the design token alone is sufficient and keeps the component consistent with the rest of the codebase.♻️ Proposed fix
- class="mt-3 flex items-center justify-between border-t border-t-gray-400/65 dark:border-t-gray-300 border-border-subtle pt-3" + class="mt-3 flex items-center justify-between border-t border-border-subtle pt-3"
463-470:will-change-transformon the arrow is declared twice.The UnoCSS class
will-change-transformon the element (line 465) already generateswill-change: transform. The identical rule in the scoped styles (.popover-arrow { will-change: transform }, lines 495–497) is therefore redundant — remove one of them.
481-487: Scoped[popover]rule duplicates template utility classes.
@apply top-0 inset-is-0and thetransitiondeclaration in the scoped rule are already present as UnoCSS utilities in the template (top-0 inset-is-0,transition-[opacity,visibility] duration-150 ease-out). The scoped rule is purely redundant and can be removed, leaving only thewill-changeand[popover]:not(:popover-open)display-reset rules.♻️ Proposed fix
[popover]:not(:popover-open) { display: none; } -[popover] { - `@apply` top-0 inset-is-0; - will-change: transform, opacity; - transition: - opacity 0.15s ease-out, - visibility 0.15s ease-out; -} +/* will-change hint for compositor promotion */ +[popover] { + will-change: transform, opacity; +}
|
You seem to have cited the wrong issue link; it seems more relevant to #1562. |
…eset after clearing
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/pages/about.vue (1)
327-343:⚠️ Potential issue | 🟡 Minor
v-ifon the<ul>is not chained with the pending/error guards — list may render alongside the loading indicator.The
<ul>at line 342 uses a standalonev-ifinstead ofv-else-if. BecauseuseLazyFetchpreserves the previousdataacross refetches,contributors.lengthcan be truthy whilecontributorsStatusis'pending', causing both the "loading" message and the stale contributor list to render simultaneously.🐛 Proposed fix
<ul - v-if="contributors.length" + v-else-if="contributors.length" class="grid grid-cols-[repeat(auto-fill,48px)] justify-center gap-2 list-none p-0 overflow-visible" >
♻️ Duplicate comments (2)
app/pages/about.vue (2)
88-97:as anycasts onshowPopover/hidePopoverremain.TypeScript ≥ 5.2 ships Popover API types in
lib.dom.d.ts, andpackage.jsondeclares TS 5.9.3. The(popover as any)casts on lines 90 and 165 should no longer be necessary — removing them would restore type-safety and let the compiler catch misuse.Also applies to: 164-171
207-210: Per-elementfocus-visible:outline-accent/70duplicates the global rule.This was flagged previously. The global
button:focus-visiblestyle inmain.cssalready covers this element; the inline utility is redundant and diverges from the shared rule. Based on learnings: focus-visible styling for button and select elements is implemented globally inapp/assets/main.css.
🧹 Nitpick comments (4)
server/api/contributors.get.ts (1)
135-197: Consider extracting query building or user normalisation to keep this function concise.It now exceeds the 50‑line guideline; splitting into small helpers would keep it focused and easier to test. As per coding guidelines: “Keep functions focused and manageable (generally under 50 lines)”.
app/pages/about.vue (3)
177-195: Use camelCase for local variables (works_at→worksAt).
works_at(line 183) and the corresponding destructured usage break the standard TypeScript camelCase convention used elsewhere in the file. This also applies to thelocationvariable that shadows the globallocation— consider renaming to e.g.locationStrto avoid the shadow.♻️ Suggested rename
- const works_at = c.company - ? $t('about.contributors.works_at', { separator, company: c.company }) + const worksAt = c.company + ? $t('about.contributors.works_at', { separator, company: c.company }) : '' - const location = c.location - ? $t('about.contributors.location', { separator, location: c.location }) + const locationLabel = c.location + ? $t('about.contributors.location', { separator, location: c.location }) : '' return $t('about.contributors.view_profile_detailed', { name: c.name || c.login, role, - works_at, - location, + works_at: worksAt, + location: locationLabel, })As per coding guidelines:
**/*.{ts,tsx,vue}: Follow standard TypeScript conventions and best practices.
415-442: Plain-text company branch is missinggap-1and carries an unused[&_a]selector.The
companyHTMLblock (line 417) hasgap-1between the icon and text, but the plain-textcompanyfallback (line 431) omits it, producing tighter spacing. Additionally, the[&_a]:(…)UnoCSS selector on the fallback<div>(line 438) targets<a>children, but the content is text interpolation — there will never be an<a>to style.♻️ Suggested fix
<div v-else-if="activeContributor.company" - class="flex items-center font-sans text-2xs text-fg-muted text-start min-w-0" + class="flex items-center gap-1 font-sans text-2xs text-fg-muted text-start min-w-0" > <div class="i-lucide:building-2 size-3 shrink-0 mt-0.5 text-accent/80" aria-hidden="true" /> <div - class="company-content leading-relaxed break-words min-w-0 [&_a]:(text-accent no-underline hover:underline transition-all)" + class="leading-relaxed break-words min-w-0" >
74-131:positionPopoveris slightly above the 50-line guideline but acceptable.The function is ~54 lines and handles a cohesive task (measure, flip, position, arrow). Splitting it further would likely hurt readability. Just noting for awareness — no action needed.
ℹ️ Review info
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
app/pages/about.vuei18n/locales/en.jsoni18n/locales/es.jsoni18n/schema.jsonlunaria/files/en-GB.jsonlunaria/files/en-US.jsonlunaria/files/es-419.jsonlunaria/files/es-ES.jsonserver/api/contributors.get.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- lunaria/files/es-ES.json
- lunaria/files/en-GB.json
- lunaria/files/es-419.json
| } | ||
| } | ||
|
|
||
| function onMouseEnter(contributor: GitHubContributor) { |
There was a problem hiding this comment.
note - we should also do this when focusing these cards, otherwise the content is inaccessible to non-mouse-users.
same for tap devices - maybe if on a device with touch support then tapping should open the popup rather than go directly to the link
There was a problem hiding this comment.
please check my discussion with coderabbitai here #1596 (comment) it is a complex problem we handle as a progressive enhacement.
We will need another approach, like an activator but ouside the link/anchor. Check some code I did on first approach in the previous coderabbitai comment.
There was a problem hiding this comment.
As alternative we can switch the BaseLink to a BaseButton or just a custom button and use ENTER to go to GH profile or SPACE to show the popup or just prevent this logic and use the lnk in the popup to go to the GH profile if the user needs more info or just want.
There was a problem hiding this comment.
We can't use an anchor as both a link and a button; this is semantically incorrect and a screen reader will go crazy: tapping the link will open a pop-up window, so what? It's an anchor/link.
There was a problem hiding this comment.
Another problem, without JavaScript we'll need to use an anchor via noscript inside a list: maybe I can explore css alternatives (CSS grid)
There was a problem hiding this comment.
how about we just never have the cards work as a link? they always popup a dialog...
There was a problem hiding this comment.
Yeah, but patak wants same layout for any contributor but we cannot add such ammount of cards, we'll need to add an upper limit (the card size may vary, some empty some full, some with long bio or location)...
There was a problem hiding this comment.
Since the “hovercard” includes contents that would be available if a user were to following the link and its not essential for the contents to be displayed on our page, I think it could be worth thinking of this feature as an enhancement. So, I don’t think it’s worth making it work for mobile—they can just tap through and find the same information.
I think we could just start off by copying GitHub here:
- Use a link as the base, but don’t alter its normal activation behaviour.
- Show the card on hover (for mouse users).
- We can set a shortcut to show the card for keyboard/screen reader users. GitHub uses
Alt+ArrowUpwhich it sets inaria-keyshortcuts. We can include whatever we decide on in our keyboard shortcuts legend (I don’t think it’s worth showing the keys like we do for other shortcuts).- When the shortcut is used, the popover opens with the first focusable item focused.
- GitHub uses the
regionlandmark here, but I might prefer thearticlerole for the hovercard instead.
I’ll take a look around for more and do some more testing (I only tested VoiceOver on macOS), but I think this is an acceptable start.
There was a problem hiding this comment.
Ok, I’m looking at the updated design and since it does include the name and role, that would be considered essential content, so it might be better for these to be buttons that open an anchored popover. I wouldn’t make the buttons themselves links. Links would need to be within the popover.
A rough example: https://knowler.dev/demos/r3QNDGk?codepen (Firefox and Safari don’t quite work as expected)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
app/pages/about.vue (1)
207-209:focus-visible:outline-accent/70still present on back button.This inline utility duplicates the global
button:focus-visiblerule inmain.cssand was previously flagged as redundant.
🧹 Nitpick comments (4)
app/pages/about.vue (4)
22-30: Remove unusedisMountedref — dead code.
isMountedis set inonMountedbut is never referenced anywhere in the current template. The keyboard-interactive prototype (where it guardedaria-expanded/aria-controls) was intentionally dropped;ClientOnlynow handles SSR concerns. The ref can be removed alongside itsonMountedcallback.🧹 Proposed cleanup
-const isMounted = shallowRef(false) const activeContributor = shallowRef<GitHubContributor | null>(null) const openTimer = shallowRef<ReturnType<typeof setTimeout> | undefined>() const closeTimer = shallowRef<ReturnType<typeof setTimeout> | undefined>() const isFlipped = shallowRef(false) -onMounted(() => { - isMounted.value = true -}) - onBeforeUnmount(() => {
147-147: Use() => void trigger()for consistency with line 145.The prior review fix explicitly recommended this form;
setTimeout(trigger, 80)is functionally equivalent but inconsistently signals intent compared tovoid trigger()on line 145.- openTimer.value = setTimeout(trigger, 80) + openTimer.value = setTimeout(() => void trigger(), 80)
158-175:closeTimer.valuenot cleared after its callback fires.
openTimer.valueis always reset toundefinedafter use, butcloseTimer.valueretains the spent ID after the 120 ms callback runs. While harmless (subsequentcancelClose()callsclearTimeouton a stale ID safely), it is inconsistent with the established reset pattern.🔧 Proposed fix
closeTimer.value = setTimeout(() => { const popover = document.getElementById('shared-contributor-popover') if (popover && !popover.matches(':hover')) { try { ;(popover as any).hidePopover() } catch (e) { if (import.meta.dev) { // oxlint-disable-next-line no-console console.warn('[onMouseLeave] hidePopover failed:', e) } } activeContributor.value = null } + closeTimer.value = undefined }, 120)
177-195:works_atis snake_case;roleLabels.value[c.role]accessed twice without caching.Two issues:
works_atshould beworksAt— TypeScript/JavaScript convention is camelCase for local variables.roleLabels.value[c.role]is accessed twice: once as a truthiness guard and again as theroleparameter. TypeScript does not narrow the second access, so it remainsstring | undefined. Store it in an intermediate variable.✏️ Proposed fix
function getAriaLabel(c: GitHubContributor): string { const separator = $t('about.contributors.separator') - const role = roleLabels.value[c.role] - ? $t('about.contributors.role', { separator, role: roleLabels.value[c.role] }) - : '' - const works_at = c.company + const roleLabel = roleLabels.value[c.role] + const role = roleLabel + ? $t('about.contributors.role', { separator, role: roleLabel }) + : '' + const worksAt = c.company ? $t('about.contributors.works_at', { separator, company: c.company }) : '' const location = c.location ? $t('about.contributors.location', { separator, location: c.location }) : '' return $t('about.contributors.view_profile_detailed', { name: c.name || c.login, role, - works_at, + works_at: worksAt, location, }) }As per coding guidelines: "Follow standard TypeScript conventions and best practices" and "Ensure you write strictly type-safe code, for example by ensuring you always check when accessing an array value by index."
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
app/pages/about.vue (1)
207-210:focus-visible:outline-accent/70on the back button is still present.This was flagged previously — the per-element override conflicts with the global
button:focus-visiblerule inmain.cssand should be removed.
🧹 Nitpick comments (3)
app/pages/about.vue (3)
78-131:positionPopoverslightly exceeds the 50-line guideline — consider extracting sub-helpers.At ~54 lines, the function mildly violates the project's "generally under 50 lines" rule. The position-maths and arrow-update logic are natural extraction points.
♻️ Suggested split
+function calculatePosition( + rect: DOMRect, + popoverWidth: number, + popoverHeight: number, + padding: number, +) { + const showBelow = rect.top < popoverHeight + 20 + const idealLeft = rect.left + rect.width / 2 + const minLeft = popoverWidth / 2 + padding + const maxLeft = window.innerWidth - popoverWidth / 2 - padding + const finalLeft = Math.max(minLeft, Math.min(idealLeft, maxLeft)) + const yBase = showBelow ? rect.bottom + 12 : rect.top - 12 + const yPercent = showBelow ? '0' : '-100%' + return { showBelow, idealLeft, finalLeft, yBase, yPercent } +} + +function updateArrow(popover: HTMLElement, idealLeft: number, finalLeft: number) { + const arrow = popover.querySelector('.popover-arrow') as HTMLElement | null + if (arrow) { + arrow.style.setProperty('--arrow-delta', `${idealLeft - finalLeft}px`) + } +} async function positionPopover(anchorId: string) { const popover = document.getElementById('shared-contributor-popover') const anchor = document.getElementById(anchorId) if (!popover || !anchor) return await nextTick() if (!popover.matches(':popover-open')) { try { ;(popover as any).showPopover() } catch (e) { if (import.meta.dev) { // oxlint-disable-next-line no-console console.warn('[positionPopover] showPopover failed:', e) } } } await nextTick() const rect = anchor.getBoundingClientRect() const padding = 16 const popoverWidth = popover.offsetWidth || 256 const popoverHeight = popover.offsetHeight || 280 - const showBelow = rect.top < popoverHeight + 20 - isFlipped.value = showBelow - - const idealLeft = rect.left + rect.width / 2 - const minLeft = popoverWidth / 2 + padding - const maxLeft = window.innerWidth - popoverWidth / 2 - padding - const finalLeft = Math.max(minLeft, Math.min(idealLeft, maxLeft)) - - const yBase = showBelow ? rect.bottom + 12 : rect.top - 12 - const yPercent = showBelow ? '0' : '-100%' - + const { showBelow, idealLeft, finalLeft, yBase, yPercent } = calculatePosition(rect, popoverWidth, popoverHeight, padding) + isFlipped.value = showBelow popover.style.transform = `translate3d(${finalLeft}px, ${yBase}px, 0) translate(-50%, ${yPercent})` - - const arrow = popover.querySelector('.popover-arrow') as HTMLElement - if (arrow) { - const delta = idealLeft - finalLeft - arrow.style.setProperty('--arrow-delta', `${delta}px`) - } + updateArrow(popover, idealLeft, finalLeft) }As per coding guidelines: "Keep functions focused and manageable (generally under 50 lines)".
183-188: Use camelCase for local variables; avoid shadowingwindow.location.Two naming issues in
getAriaLabel:
works_at(line 183) is snake_case — TypeScript convention requires camelCaseworksAt.location(line 186) shadows the globalwindow.location, which can cause confusion and may trigger lint warnings.♻️ Suggested fix
- const works_at = c.company - ? $t('about.contributors.works_at', { separator, company: c.company }) + const worksAt = c.company + ? $t('about.contributors.works_at', { separator, company: c.company }) : '' - const location = c.location - ? $t('about.contributors.location', { separator, location: c.location }) + const locationPart = c.location + ? $t('about.contributors.location', { separator, location: c.location }) : '' return $t('about.contributors.view_profile_detailed', { name: c.name || c.login, role, - works_at, - location, + works_at: worksAt, + location: locationPart, })As per coding guidelines: "Follow standard TypeScript conventions and best practices".
408-413: Hardcoded quote characters around bio are not i18n-aware.Wrapping the bio in literal
"…"works for English but is incorrect for many locales (French uses«…», German uses„…"). Consider moving the surrounding quotes into the i18n string or removing them and relying on theitalicstyling alone to convey the quote context.
| <div | ||
| v-else-if="activeContributor.company" | ||
| class="flex items-center font-sans text-2xs text-fg-muted text-start min-w-0" | ||
| > | ||
| <div | ||
| class="i-lucide:building-2 size-3 shrink-0 mt-0.5 text-accent/80" | ||
| aria-hidden="true" | ||
| /> | ||
| <div | ||
| class="company-content leading-relaxed break-words min-w-0 [&_a]:(text-accent no-underline hover:underline transition-all)" | ||
| > | ||
| {{ activeContributor.company }} | ||
| </div> | ||
| </div> |
There was a problem hiding this comment.
Missing gap-1 on the plain-text company row.
The companyHTML branch (line 417) includes gap-1 on the flex container, but the v-else-if plain-text branch (line 431) omits it, causing the building icon and the company name to render flush against each other.
🐛 Proposed fix
- class="flex items-center font-sans text-2xs text-fg-muted text-start min-w-0"
+ class="flex items-center gap-1 font-sans text-2xs text-fg-muted text-start min-w-0"There was a problem hiding this comment.
remove mt-0.5 from icon adding gap-1 to the container
There was a problem hiding this comment.
Task: Adjust company row styling
- Add
gap-1to the plain-text company container (line 431) - Remove
mt-0.5from both company icons (lines 420 and 436)
@userquin, got it! I've updated the fix to add gap-1 to the plain-text container and remove mt-0.5 from both icons for consistent spacing.
🧠 Learnings used
Learnt from: userquin
Repo: npmx-dev/npmx.dev PR: 1596
File: app/pages/about.vue:318-341
Timestamp: 2026-02-23T05:31:51.043Z
Learning: In the npmx.dev project, the contributor popover on the about page is intentionally a mouse-only progressive enhancement. Keyboard support (focus/blur with full popover interaction) was tested but rejected due to Firefox performance issues (3-4 second lag with per-contributor popovers in the Top Layer) and implementation complexity (aria-controls, aria-expanded, focus trapping, Escape handling). All contributor information remains accessible via the clickable link to the GitHub profile.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
💡 For code changes across multiple files, use @coderabbitai plan in an issue comment
4 participants
🔗 Linked issue
resolves #1564
🧭 Context
This PR includes a single popup for all contributors avatar including sponsor link.
NOTE: this PR dont't use
dir="auto"norbditags for RTL locales in the popup content, the browser interprets that as LTR and, by default, the text will align to the physical left of the popup. In a purely RTL (Real-Time) UI, this creates a "zigzag" effect:I cannot test this on Safari, so any help on this is welcome 🙏 .
NOTE: we need
NUXT_GITHUB_ORG_TOKENat Vercel (for PR preview environment), without the token only popup shown forstewardcontributors (default ones)some screenshots
📚 Description
This PR uses GH graphql to get the following info to be displayed in the popup (sponsor was already there):
The card uses purple colors for the sponsor anchor colors and includes also an anchor for the GH account. The card won't work without JavaScript, too complex to handle focus state, the user can just press enter on the avatar and will be sent to the GH account page.
This PR also includes a change to
Link/Base.vuesince there is no way to remove the external link icon (added backnoExternalIconboolean prop)To test this PR in your local you MUST be a maintainer here, and create a PAT with
read:orgatadmin:org:Generate new tokenand selectGenerate new token (classic)read:orgatadmin:orgGenerate tokenbutton: don't close the page, copy the token before leaving the pageread:orgOnce created, you MUST copy and paste it at
.envfile at root (included at.gitignoreand generated by one module) using:NUXT_GITHUB_ORG_TOKEN=<your_token>:Stop dev server, delete
.nuxt/cachefolder and start dev server.Used Gemini PRO to change the popup position and the arrow at the bottom/top, I did some manual changes.