Skip to content

Conversation

@MatteoGabriele
Copy link
Contributor

Adding avatars for each connected service has several problems:

  • Visually, when a user has the same avatar everywhere, it creates multiple copies of the same image to be rendered
  • The feature won't scale if we add more services

My proposal is to retain only the single cloud icon and add a green dot to indicate to the user that it is connected.

before
Screenshot 2026-02-08 at 17 00 12

after
Screenshot 2026-02-08 at 17 24 37

@vercel
Copy link

vercel bot commented Feb 8, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
npmx.dev Ready Ready Preview, Comment Feb 8, 2026 6:14pm
2 Skipped Deployments
Project Deployment Actions Updated (UTC)
docs.npmx.dev Ignored Ignored Preview Feb 8, 2026 6:14pm
npmx-lunaria Ignored Ignored Feb 8, 2026 6:14pm

Request Review

@codecov
Copy link

codecov bot commented Feb 8, 2026

Codecov Report

❌ Patch coverage is 20.00000% with 8 lines in your changes missing coverage. Please review.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
app/components/Header/AccountMenu.client.vue 14.28% 6 Missing ⚠️
app/components/Header/MobileMenu.client.vue 33.33% 2 Missing ⚠️

📢 Thoughts on this report? Let us know!

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Feb 8, 2026

Warning

Rate limit exceeded

@MatteoGabriele has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 4 minutes and 46 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📝 Walkthrough

Walkthrough

This change removes avatar image usage from header components (AccountMenu and MobileMenu) by no longer extracting the avatar (npmAvatar) from useConnector() and by replacing conditional avatar <img> elements with inline icon spans. All instances that previously rendered npm or atproto/Atmosphere avatars now render fixed inline icons; accessibility attributes and existing textual/structural elements are preserved.

Possibly related PRs

Suggested reviewers

  • danielroe
  • zeucapua
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The pull request description clearly explains the rationale (avoiding duplicate avatars, poor scalability) and proposes a concrete solution (single cloud icon with green dot indicator). The description is directly related to the changeset, which removes avatar data and replaces avatar images with icons.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
app/components/Header/AccountMenu.client.vue (1)

68-76: Consider adding accessible text for connection status.

The green dot visually indicates that the user is connected to services, but screen reader users have no equivalent indication. Consider adding visually hidden text to convey this status.

Proposed fix
       <div v-if="hasAnyConnection" class="flex items-center relative">
         <span
           class="w-6 h-6 rounded-full bg-bg-muted ring-2 ring-bg flex items-center justify-center"
           :class="hasBothConnections ? 'relative z-10' : ''"
         >
-          <span class="size-1.5 rounded-full bg-green-600 absolute top-0 -inset-ie-0"></span>
+          <span class="size-1.5 rounded-full bg-green-600 absolute top-0 -inset-ie-0" aria-hidden="true"></span>
           <span class="i-carbon-cloud w-3 h-3 text-fg-muted" aria-hidden="true" />
         </span>
+        <span class="sr-only">{{ $t('account_menu.connected') }}</span>
       </div>

This requires adding a connected translation key to your i18n files.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/components/Header/AccountMenu.client.vue (1)

60-79: ⚠️ Potential issue | 🟡 Minor

Connection status is not accessible to screen readers.

The green dot indicator conveys the connected state visually but provides no accessible alternative. Screen reader users cannot perceive this status change. Consider adding an aria-label to the button that reflects the current connection state.

Suggested fix
     <ButtonBase
       type="button"
       :aria-expanded="isOpen"
       aria-haspopup="true"
+      :aria-label="hasAnyConnection ? $t('account_menu.connected') : $t('account_menu.connect')"
       `@click`="isOpen = !isOpen"
       class="border-none"
     >

This assumes the appropriate translation keys exist. Adjust key names as needed.

🧹 Nitpick comments (1)
app/components/Header/AccountMenu.client.vue (1)

69-75: Consider removing vestigial z-10 conditional class.

The conditional hasBothConnections ? 'relative z-10' : '' appears to be leftover from the previous multi-avatar stacking design. Since the UI now renders a single cloud icon regardless of how many services are connected, the z-index logic no longer serves a purpose. The parent div at line 68 already provides the relative positioning context needed for the status dot.

Suggested simplification
         <span
           class="w-6 h-6 rounded-full bg-bg-muted ring-2 ring-bg flex items-center justify-center"
-          :class="hasBothConnections ? 'relative z-10' : ''"
         >

@github-actions
Copy link

github-actions bot commented Feb 8, 2026

Lunaria Status Overview

🌕 This pull request will trigger status changes.

Learn more

By default, every PR changing files present in the Lunaria configuration's files property will be considered and trigger status changes accordingly.

You can change this by adding one of the keywords present in the ignoreKeywords property in your Lunaria configuration file in the PR's title (ignoring all files) or by including a tracker directive in the merged commit's description.

Tracked Files

File Note
lunaria/files/en-GB.json Localization changed, will be marked as complete.
lunaria/files/en-US.json Source changed, localizations will be marked as outdated.
lunaria/files/it-IT.json Localization changed, will be marked as complete.
Warnings reference
Icon Description
🔄️ The source for this localization has been updated since the creation of this pull request, make sure all changes in the source have been applied.

@danielroe
Copy link
Member

what about instead displaying the logo of the connected service? (npm, bluesky or atproto) and we revisit when we get lots more... (could display a +4 or something)

@MatteoGabriele
Copy link
Contributor Author

That could indeed be a valid alternative. I will use the logos

@Tobbe
Copy link

Tobbe commented Feb 8, 2026

what about instead displaying the logo of the connected service? (npm, bluesky or atproto) and we revisit when we get lots more... (could display a +4 or something)

How do you pick what logo to display though, if the user is connected to more than one thing? (assuming they can be)

Could also have overlapping avatars. (But then again it gets tricky - which one do we show on top?)

@MatteoGabriele
Copy link
Contributor Author

There's also an "issue" with the ATProto icon: it kinda looks like I want to connect my email.

Screenshot 2026-02-08 at 18 31 04

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/components/Header/AccountMenu.client.vue (1)

60-88: ⚠️ Potential issue | 🟠 Major

Add an explicit accessible label for the icon-only button.

When connected, the button contains only aria-hidden icons, so it has no accessible name. Please add an aria-label (you already added account_menu.connected in i18n).

✅ Proposed fix
     <ButtonBase
       type="button"
       :aria-expanded="isOpen"
       aria-haspopup="true"
+      :aria-label="hasAnyConnection ? $t('account_menu.connected') : $t('account_menu.connect')"
       `@click`="isOpen = !isOpen"
       class="border-none"
     >

@MatteoGabriele
Copy link
Contributor Author

MatteoGabriele commented Feb 8, 2026

@danielroe I've updated the PR with icons.
The npm icon is fine, but the one I picked for the ATProto might be misunderstood. Let me know what you think. I could use the Bluesky icon, but that would technically not be correct.

@danielroe
Copy link
Member

we need to update the mobile menu as well:

SCR-20260208-pqwx

I think the icon looks good.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
app/components/Header/MobileMenu.client.vue (1)

125-135: ⚠️ Potential issue | 🟡 Minor

Missing green status indicator for visual consistency.

The npm CLI connection (line 121) displays a green status dot to indicate active connection, but the Atmosphere connection lacks this indicator. For consistent UX, consider adding the same visual cue here.

💡 Suggested fix to add status indicator
               <span class="flex-1 truncate">@{{ atprotoUser.handle }}</span>
+              <span class="w-2 h-2 rounded-full bg-green-500" aria-hidden="true" />
             </button>

isConnected: isNpmConnected,
isConnecting: isNpmConnecting,
npmUser,
avatar: npmAvatar,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still wondering if it makes sense to show avatars in the dropdown, even if not in the top level .... 🤔

wdyt?

Copy link
Contributor Author

@MatteoGabriele MatteoGabriele Feb 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it may look a bit confusing and inconsistent.
If I have to go with avatars, which I do like because they give the impression that I've actually connected, I prefer the initial proposal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants