feat(ui): add UserButton controller - #9185
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 23d2212 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 |
|
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:
📝 WalkthroughWalkthroughAdded Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to The PR connects UserButton actions to live account and organization data; the only remaining concern is limited interaction-test coverage for pending actions and closing after selection. No actionable merge-blocking risk remains after normal checks and review. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 18.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 4 files. (1 skipped: 1 unsupported.) Comment |
8786841 to
ba22f92
Compare
ba22f92 to
c9bdf23
Compare
alexcarpenter
changed the title
c9bdf23 to
93aa6f2
Compare
93aa6f2 to
4643795
Compare
4643795 to
4467f3d
Compare
4467f3d to
fbd5c2f
Compare
The sizes track the Icon scale (sm 14px, md 16px) so a spinner can stand in for the icon it replaces, and the UserButton's trailing column is now one slot the width of the menu button, so the spinner, the active check, and the menu all sit on the same centre line.
The trigger carried the avatar alone. It now names what is active beside it — the organization and its plan wherever one heads the trigger, the account otherwise — behind `showLabel`, which defaults on. Badge's `neutral` color was unreadable in both schemes: its fill is a 900 and its text token is a text color, not an on-fill one. It now rides the same black/white scrim the button's neutral fill does.
…ccount The trigger and the popup's header now always name the same workspace. `combined` carries both switchers, so `modePriority` picks which one it leads with: the active organization by default, the account with `modePriority="user"`. Both are still listed either way.
…ser fixture The controller now reads hasOrganizations off the user resource, so the mocked user needs the field the real one has.
`setActive` swaps the active organization while its promise is still in flight, so the popup rearranged mid-action: the header renamed itself, the check jumped rows, and Invite came and went as the permission was re-read. The connected component now snapshots the controller when an action starts and renders that until it settles, so the result lands in one step. Two smaller faults fell out of the same interaction: - The spinner waited out a delay window before appearing, and the check raced ahead of it. Every action here is a network round trip, so there is nothing to debounce: `useSpinDelay` takes `delay: 0` and shows the value in the same pass, with `minDuration` still steadying it. - A row going busy swapped its host element from `<button>` to `<div>`, remounting the subtree and dropping the avatar back to its initials for the length of the action. A row that stands down now stays the button it was, disabled, and `Avatar.Image` resolves a browser-cached `src` in a layout effect so neither a remount nor a swap flashes the fallback.
The popover stayed up behind the surface it opened. Managing, inviting, creating an organization, and adding an account now close it on the way out, whether they open a modal or navigate.
The connected test drives the real controller against a mocked Clerk, which is what makes it worth having and also what makes it slow. Cases that only ever asserted what the popover renders now sit in the view test, leaving the connected one to prove the layers compose. Also covers `hidePersonal` reaching the popover through the container.
UserButtonProps picked only modePriority off the root, so the connected component was hard-wired to the combined surface and the orgs/user modes were reachable only by composing UserButtonView directly.
Presses a custom row on the connected UserButton and checks the app's callback runs and the popover closes behind it.
An instance with organizations turned off has none to lead with or list, so the button is the account's whatever `mode` asked for — `orgs` would otherwise render an empty shell of a switcher. clerk-js withholds its own OrganizationSwitcher at the mount boundary, which an app importing this one never crosses, so the gate lives in the component.
The popover's open state and the one action in flight are the same flow, so they now live in one machine instead of two useStates. Re-entry, clearing busy, and closing on success stop being hand-written in the container: RUN is simply unhandled while busy, and busy is only reachable from open. Dismissing the popover mid-action now abandons the result rather than letting it land in a surface that is already gone.
Follows the view's rename of `'orgs'` to `'organization'`. Also corrects the integration suite's opening comment, which attributed close-on-success to the container and had the navigation case backwards.
Carries the app's own pages and links into the profile the UserButton opens, through useCustomPages and the built-in page list it orders them against.
Moves useCustomPages and useUserProfilePages out of the shared mosaic hooks folder into user-button.pages, and routes the custom page order through the same applyOrder rule the menu uses, which also stops a custom page named after a built-in from being sent twice.
Ephem
left a comment
Ephem
left a comment
There was a problem hiding this comment.
I did some AI-assisted reviewing and found a few things to dig into. I'll follow this up with stepping through everything myself as well.
Happy to tackle a few of these if you'd like after I've gotten through the full review.
| if (model.status !== 'ready') { | ||
| return { status: model.status }; | ||
| } |
There was a problem hiding this comment.
Since this happens before respecting the frozen state I think this is going to revert back to 'loading' during the transitive state when a navigation is involved after setActive.
That means we'll pop the fallback/null during that window.
Reverting to 'loading' also isn't covered by tests currently.
| onSignOutSession: runAction(userButtonBusyKeys.signOutSession, onSignOutSession), | ||
| onSignOutAll: runAction(userButtonBusyKeys.signOutAll, onSignOutAll), |
There was a problem hiding this comment.
I think onSignOutAll should have closeOnSuccess: true, and onSignOutSession should have data.additionalSessions.length === 0.
Otherwise, the state will still be 'open', but the component unmounts. If the user signs in again while UserButton is still mounted, the popup will open again after the sign in which feels off.
| const selected = membershipData.find(m => m.organization.id === organizationId)?.organization; | ||
| return selected ? resolveAfterSelectUrl(options?.afterSelectOrganizationUrl, selected) : undefined; |
There was a problem hiding this comment.
This is called via onSelectOrganization and one of the callsites for that is when switching to an recently accepted invitation. When it's an invitation, there's no guarantee the organization is in membershipData by the time this gets called, resulting in an undefined url and no navigation.
This can happen on fast clicks on slow networks because we don't await the revalidates further down, see separate comment.
| void userInvitations.revalidate?.(); | ||
| void userMemberships.revalidate?.(); |
There was a problem hiding this comment.
These should not be voided, instead return a Promise.all(...) from the finally so we wait for the revalidate. This fixes a bug mentioned above, but it also fixes a case where the UI goes back to showing "Join" again right after already having joined, and the possibility of trying to accept invitations twice.
Or put differently, by not waiting for the revalidation, we are not getting the benefits of frozen.
Not sure if we also want to add a separate catch with a noop for the revalidations, otherwise the action will be treated as failed just because revalidations failed. 🤔 Both versions has tradeoffs.
| // unhandled here is what stops a second action starting while one is in flight. Dismissing the | ||
| // popup abandons the action: the request finishes, but nothing is left for its result to land in. | ||
| busy: { | ||
| on: { CLOSE: { target: 'closed', actions: assign(() => settled) } }, |
There was a problem hiding this comment.
What if you reopen the popup while the action you were taking is still ongoing? We'd want to still show pending then right?
I think we might need to split it into two separate states?
popup: open | closed
action: idle | busy
| return; | ||
| } | ||
| // `redirectUrl` decorated for us; taking the callback takes the Safari ITP refresh with it. | ||
| await router.navigate(decorateUrl(displayConfig.afterSwitchSessionUrl)); |
There was a problem hiding this comment.
This tries to navigate even if afterSwitchSessionUrl is empty, which might navigate to the same page as we are on, old behavior in useMultisessionActions was to call it conditionally.
Also, are we missing a afterSwitchSessionUrl prop on the component? Is that intentional?
| export type UserButtonModelOptions = UserProfileMode & | ||
| OrganizationProfileMode & | ||
| CreateOrganizationMode & { | ||
| afterSelectOrganizationUrl?: AfterSelectUrl<OrganizationResource>; | ||
| /** Where selecting the personal workspace lands. Resolved against the user, not an organization. */ | ||
| afterSelectPersonalUrl?: AfterSelectUrl<UserResource>; | ||
| /** | ||
| * Leaves the personal workspace out. An instance that forces organization selection withholds it | ||
| * either way, so this cannot opt back in. | ||
| */ | ||
| hidePersonal?: boolean; | ||
| }; |
There was a problem hiding this comment.
I had AI go through and map props to the old button, these are the discrepancies it found. Some of this might be intentional, or saved for follow-ups but it's worth being explicit about so I thought I'd post the list, maybe you can shed some light on which are missing intentionally and not?
Likely worth adding
defaultOpen- Supported by
UserButtonRoot, but not exposed by the connectedUserButtonor controller.
- Supported by
signInUrl- Legacy uses the component override for “Add account” and session-task URLs.
- Mosaic always uses
clerk.buildSignInUrl().
- Additional
userProfileProps- Mosaic only accepts
customPagesandpageOrder. - Legacy also forwards:
additionalOAuthScopesapiKeysPropsappearancefor the opened UserProfile modal
- Mosaic only accepts
- Custom menu
open/ profile start path- Legacy custom items can open a specific UserProfile page.
- Mosaic items only support
hreforonClick.
Because Mosaic UserButton also replaces OrganizationSwitcher functionality, these are missing too:
afterCreateOrganizationUrl- Controls where Clerk navigates after creating an organization.
- This differs from
createOrganizationUrl, which controls whether clicking Create opens Clerk’s modal or navigates to an app page.
skipInvitationScreen- Configures the Clerk create-organization modal.
afterLeaveOrganizationUrl- Used after leaving an organization through OrganizationProfile.
organizationProfileProps- Legacy forwards custom pages and appearance into the opened OrganizationProfile.
| activeOrganization: organization ? toMembership(organization) : null, | ||
| // The user resource settles this before the paginated list answers; the count covers a stale resource. | ||
| hasOrganizations: user.organizationMemberships.length > 0 || (userMemberships.count ?? 0) > 0, | ||
| hidePersonal: forceOrganizationSelection || (options?.hidePersonal ?? false), |
There was a problem hiding this comment.
There's a state I don't think is modelled here which is "No organization selected". I think this is when forceOrganizationSelection is true, but organization is null. Not sure when this happens (deleted orgs?), but I know the old UserButton handles it.
Requires changes in the view too, but this is within the PR diff and I don't think we are providing everything we need from here (forceOrganizationSelection).
2 participants
Description
Stacked on #9184. Connects the
UserButtonview to live Clerk data.useUserButtonController()returns a'loading' | 'hidden' | 'ready'union. When it isreadyit carries the view's data contract and the callback behind every row.user-button.tsxis the connected container that owns the popover.Where the data comes from:
activeSessionfromuseUser()anduseSession(). The name follows Clerk's own order: first and last name, then username, then the identifier.activeOrganizationfromuseOrganization(), wherenullis the personal workspace.memberships,suggestions, andinvitationsfromuseOrganizationList(), paged as the list scrolls.hasOrganizationsfrom the user resource, so it can answer before those lists load.additionalSessionsfrom the client, without the active one.org:sys_memberships:manage.What the rows do:
setActive. Signing out callssignOut. Invitations and suggestions accept in place and revalidate the list.userProfileUrl,organizationProfileUrl,createOrganizationUrl.afterSelectOrganizationUrlandafterSelectPersonalUrlsay where picking a workspace lands.hidePersonalwithholds it by request.customMenuItemsreach the menu through the container, and a custom action closes the popover behind whatever it opens.The button renders nothing until Clerk answers. While it loads, a signed-out visitor and a session still resolving are indistinguishable, so anything rendered then is a button promised to people who are never going to get one.
<ClerkLoading>is where an app that knows its own nav puts a placeholder.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change