refactor(ui): declare the whole Mosaic UserButton surface in one table - #9411
Conversation
Every mode now describes its own surface top to bottom in a single table, so the header gear and the row menus read where they land instead of hardcoding it.
🦋 Changeset detectedLatest commit: 889cd73 The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@clerk/expo-google-signin
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/hono
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/react
@clerk/react-router
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/ui
@clerk/upgrade
@clerk/vue
commit: |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe UserButton layout resolver now uses mode-defined action slots for headers, headings, and footers. It separately controls organization and session visibility and filters unavailable actions. The view renders organization and session sections from the resolved layout and uses session-oriented component names. Tests cover placement, visibility, availability, and lead-surface priority. An empty Changeset file was added. Estimated code review effort: 4 (Complex) | ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
API Changes Report
Summary
🔴 Breaking changes index (8)Every breaking change, up front. Full diffs are in the package sections below.
@clerk/sharedCurrent version: 4.28.1 Subpath
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/ui/src/mosaic/user-button/user-button.layout.ts (1)
102-104: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider
readonlyarrays in the resolved contract.
actionsexposes mutableUserButtonAction[]values, so any consumer can push into the resolved layout. The resolver already builds fresh arrays, so a readonly type costs nothing and keeps the contract immutable.♻️ Proposed change
/** What each slot carries, in the order it renders. */ - actions: Record<UserButtonSlot, UserButtonAction[]>; + readonly actions: Readonly<Record<UserButtonSlot, readonly UserButtonAction[]>>; }The internal
actionsvariable inresolveUserButtonLayoutcan stayRecord<UserButtonSlot, UserButtonAction[]>; the return type widens to the readonly shape.As per coding guidelines: "Verify consistent use of
readonlyfor immutable data in code review" and "Declare readonly arrays and objects for immutable data structures".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/ui/src/mosaic/user-button/user-button.layout.ts` around lines 102 - 104, Update the resolved layout contract’s actions property to use readonly UserButtonAction arrays, while keeping the internal actions variable in resolveUserButtonLayout mutable as currently constructed. Ensure the returned resolved layout exposes immutable arrays without changing resolver behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/ui/src/mosaic/user-button/user-button.layout.ts`:
- Around line 102-104: Update the resolved layout contract’s actions property to
use readonly UserButtonAction arrays, while keeping the internal actions
variable in resolveUserButtonLayout mutable as currently constructed. Ensure the
returned resolved layout exposes immutable arrays without changing resolver
behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: cc93d315-d84c-4387-951a-7363599a41ce
📒 Files selected for processing (4)
.changeset/clear-mosaic-user-button-layout.mdpackages/ui/src/mosaic/user-button/__tests__/user-button.layout.test.tspackages/ui/src/mosaic/user-button/user-button.layout.tspackages/ui/src/mosaic/user-button/user-button.view.tsx
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
clerk/clerk_go(manual)clerk/dashboard(manual)clerk/accounts(manual)clerk/backoffice(manual)clerk/clerk(manual)clerk/clerk-docs(manual)clerk/cloudflare-workers(manual)clerk/clerk-ios(auto-detected)clerk/cli(auto-detected)clerk/clerk-android(auto-detected)
Ephem
left a comment
Ephem
left a comment
There was a problem hiding this comment.
This is so much clearer, I ❤️ it! 🙏
| footer: readonly UserButtonAction[]; | ||
| } | ||
|
|
||
| const modes = { |
There was a problem hiding this comment.
This could be renamed something like modeDefaults right? Get's merged with data and produces a UserButtonLayout which is the "public" shape outside of this file?
Renaming is very much a NIT, mostly checking my understanding.
There was a problem hiding this comment.
Your understanding is exactly right: modes is the static per-mode declaration, resolveUserButtonLayout() runs it against UserButtonData, and UserButtonLayout is the only shape that leaves the file (ListLayout, ModeLayout and modes are all private).
I skipped modeDefaults though, because "defaults" implies something overrides them and nothing does. The data can only drop an action (offered() removes inviteMembers when no org is active) or relocate one (the accounts heading's actions fall to the footer when there is no other account). Nothing ever supplies a competing value that wins, so the name would advertise an override mechanism that isn't there.
But you were pointing at something real. The collision wasn't the table, it was that ModeLayout (static input) and UserButtonLayout (resolved output) both say "layout" — and inside the resolver there was a local literally named layout that was not what the function returns:
const layout: ModeLayout = modes[mode]; // not the returned layoutRenamed that to declared, so UserButtonLayout is now the only "layout" in the file. Thanks for the poke.
| {/* Memberships, invitations and suggestions are three separate requests landing at three | ||
| different moments. Rendering each as it arrives walks the list in in stages, so the | ||
| placeholder stands in for all of them until the last one is in. */} |
There was a problem hiding this comment.
Tangent: Oh how I want suspense. 🙃
Co-authored-by: Fredrik Höglund <fredrik@clerk.dev>
2 participants
Description
feedback follow up from #9184
The Mosaic
UserButtonpopup decided its shape in two places.user-button.layout.tsheld two tables of opposite orientation (one keyed by mode, one keyed by affordance) plus a conditional type cross-checking them, and the view still hardcoded the header gear and the active-account row menu on its own.Each mode now declares its whole surface in one table, top to bottom:
A section the mode does not carry is
false, so it has no array to hold actions and placing one there is impossible to express. That removed the'none'slot and theModeSlot<M>conditional type outright.resolveUserButtonLayout()runs the table against the data once and hands back which sections render and what each slot carries, in order. Every consumer loops its own list rather than asking where a given action landed:Behaviour is unchanged in every reachable state. The ASCII diagram at the top of the file shows the three modes side by side.
Renames follow the data contract, so
workspacenow means only the personal-or-organization union:ActiveAccountRow->OrganizationsHeading,AccountsSection->SessionSection,WorkspaceRow->SwitcherRow, and so on.This is internal to
@clerk/ui; the MosaicUserButtonis not exported yet, hence the empty changeset.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change