feat(shared,ui): Render plan badge in OrganizationSwitcher#9149
feat(shared,ui): Render plan badge in OrganizationSwitcher#9149dstaley wants to merge 5 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
🦋 Changeset detectedLatest commit: 6d49e41 The changes in this PR will be included in the next version bump. This PR includes changesets to release 23 packages
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 |
| )} | ||
| </AcceptedInvitationsProvider> | ||
| </Flow.Root> | ||
| <SubscriberTypeContext.Provider value='organization'> |
There was a problem hiding this comment.
the only thing changed here is wrapping the existing Flow.Root in the SubscriberTypeContext.Provider
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Repository UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesOrganization plan badge
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OrganizationSwitcherTrigger
participant useSubscription
participant Billing
participant OrganizationPreview
participant Badge
OrganizationSwitcherTrigger->>useSubscription: Read active organization subscription
useSubscription->>Billing: Fetch subscription
Billing-->>useSubscription: Return subscription items
useSubscription-->>OrganizationSwitcherTrigger: Loading or plan item state
OrganizationSwitcherTrigger->>OrganizationPreview: Pass plan badge
OrganizationPreview->>Badge: Render loading or plan badge
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
Comment |
|
|
||
| if (isLoading) { | ||
| // 5ch is specifically chosen to balance the size of "Free" versus paid plans in Inter | ||
| return <LoadingBadge sx={{ width: '5ch' }} />; |
There was a problem hiding this comment.
the screenshots in the PR make this look larger than it actually is. This is what it looks like when not zoomed in.
| return ( | ||
| <Badge | ||
| elementDescriptor={descriptors.organizationSwitcherTriggerBadge} | ||
| elementId={descriptors.organizationSwitcherTriggerBadge.setId(slug)} |
There was a problem hiding this comment.
added this so you can specifically target certain plans to customize the badge
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/electron
@clerk/electron-passkeys
@clerk/eslint-plugin
@clerk/expo
@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: |
API Changes Report
Summary
@clerk/sharedCurrent version: 4.25.2 Subpath
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/ui/src/components/OrganizationSwitcher/__tests__/OrganizationSwitcher.test.tsx (1)
90-166: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for the free-plan (
primary) and loading-state paths.Both new tests only cover a paid plan (
isDefault: false→secondarycolorScheme). Per the PR objective, free plans should render with theprimarycolor scheme, and there's also an animated loading state (LoadingBadge) rendered while the subscription is being resolved — neither is exercised here. As per path instructions,**/*.{test,spec}.{ts,tsx,js,jsx}requires that "Unit tests are required for all new functionality" and to "Verify proper error handling and edge cases."Consider adding:
- A test with
isDefault: trueasserting theprimarycolorScheme is applied.- A test asserting the pulsing
LoadingBadgerenders whilegetSubscriptionis pending.Do you want me to draft these additional test cases?
🤖 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/components/OrganizationSwitcher/__tests__/OrganizationSwitcher.test.tsx` around lines 90 - 166, Add coverage in OrganizationSwitcher tests for both missing paths: configure the mocked subscription plan with isDefault: true and assert the rendered plan badge uses the primary color scheme, then keep getSubscription pending and assert the animated LoadingBadge appears during resolution. Reuse the existing fixtures and showPlanName setup while preserving the paid-plan and billing-disabled tests.Source: Path instructions
🤖 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.
Inline comments:
In
`@packages/ui/src/components/OrganizationSwitcher/OrganizationSwitcherTrigger.tsx`:
- Line 66: Update OrganizationSwitcherTrigger’s badge rendering and the
corresponding PlanBadge usage around the noted lines so PlanBadge is mounted
only when showPlanName is enabled. Ensure useSubscription() is not invoked on
the default showPlanName={false} path, while preserving the existing badge
behavior when showPlanName is true.
- Around line 106-108: Update the LoadingBadge render in
OrganizationSwitcherTrigger to apply the same organizationSwitcherTriggerBadge
descriptor and corresponding elementId used by the normal plan badge, while
preserving its loading width and existing behavior.
---
Nitpick comments:
In
`@packages/ui/src/components/OrganizationSwitcher/__tests__/OrganizationSwitcher.test.tsx`:
- Around line 90-166: Add coverage in OrganizationSwitcher tests for both
missing paths: configure the mocked subscription plan with isDefault: true and
assert the rendered plan badge uses the primary color scheme, then keep
getSubscription pending and assert the animated LoadingBadge appears during
resolution. Reuse the existing fixtures and showPlanName setup while preserving
the paid-plan and billing-disabled tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Repository UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: beb9aa54-c754-49fd-a67d-36e1d6dce633
📒 Files selected for processing (9)
.changeset/hungry-lands-fold.mdpackages/shared/src/types/clerk.tspackages/ui/src/components/OrganizationSwitcher/OrganizationSwitcherTrigger.tsxpackages/ui/src/components/OrganizationSwitcher/__tests__/OrganizationSwitcher.test.tsxpackages/ui/src/components/OrganizationSwitcher/index.tsxpackages/ui/src/customizables/elementDescriptors.tspackages/ui/src/elements/Badge.tsxpackages/ui/src/internal/appearance.tspackages/ui/src/styledSystem/animations.ts
| gap={3} | ||
| size='xs' | ||
| organization={organization} | ||
| badge={<PlanBadge />} |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid fetching subscriptions when showPlanName is disabled.
OrganizationSwitcher always mounts <PlanBadge />, and PlanBadge calls useSubscription() before checking showPlanName. With the organization subscriber context and billing permissions, this makes the default showPlanName={false} path eligible to fetch billing data unnecessarily. Gate mounting at the caller or split the hook into a child rendered only when the prop is enabled.
Suggested gating
- const { hidePersonal } = useOrganizationSwitcherContext();
+ const { hidePersonal, showPlanName } = useOrganizationSwitcherContext();
- badge={<PlanBadge />}
+ badge={showPlanName ? <PlanBadge /> : undefined}Also applies to: 98-104
🤖 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/components/OrganizationSwitcher/OrganizationSwitcherTrigger.tsx`
at line 66, Update OrganizationSwitcherTrigger’s badge rendering and the
corresponding PlanBadge usage around the noted lines so PlanBadge is mounted
only when showPlanName is enabled. Ensure useSubscription() is not invoked on
the default showPlanName={false} path, while preserving the existing badge
behavior when showPlanName is true.
| if (isLoading) { | ||
| // 5ch is specifically chosen to balance the size of "Free" versus paid plans in Inter | ||
| return <LoadingBadge sx={{ width: '5ch' }} />; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Apply the OrganizationSwitcher badge descriptor to the loading state.
The normal plan badge uses organizationSwitcherTriggerBadge, but LoadingBadge is rendered without an elementDescriptor or elementId. Appearance overrides therefore cannot target the loading placeholder. As per path instructions, themable elements under packages/ui/src must apply element descriptors.
Suggested fix
- return <LoadingBadge sx={{ width: '5ch' }} />;
+ return (
+ <LoadingBadge
+ elementDescriptor={descriptors.organizationSwitcherTriggerBadge}
+ elementId={descriptors.organizationSwitcherTriggerBadge.setId('loading')}
+ sx={{ width: '5ch' }}
+ />
+ );📝 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.
| if (isLoading) { | |
| // 5ch is specifically chosen to balance the size of "Free" versus paid plans in Inter | |
| return <LoadingBadge sx={{ width: '5ch' }} />; | |
| if (isLoading) { | |
| // 5ch is specifically chosen to balance the size of "Free" versus paid plans in Inter | |
| return ( | |
| <LoadingBadge | |
| elementDescriptor={descriptors.organizationSwitcherTriggerBadge} | |
| elementId={descriptors.organizationSwitcherTriggerBadge.setId('loading')} | |
| sx={{ width: '5ch' }} | |
| /> | |
| ); |
🤖 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/components/OrganizationSwitcher/OrganizationSwitcherTrigger.tsx`
around lines 106 - 108, Update the LoadingBadge render in
OrganizationSwitcherTrigger to apply the same organizationSwitcherTriggerBadge
descriptor and corresponding elementId used by the normal plan badge, while
preserving its loading width and existing behavior.
Source: Path instructions
alexcarpenter
left a comment
There was a problem hiding this comment.
can you add an example showing how this wraps?
| colorScheme={isDefault ? 'primary' : 'secondary'} | ||
| sx={{ whiteSpace: 'nowrap' }} | ||
| > | ||
| <Text |
There was a problem hiding this comment.
do we need the extra text usage in the badge vs supplying the sizing to the badge component?
There was a problem hiding this comment.
nah, we use <Text> in Badge in a few places, but here we don't really need it. Removed!
|
@alexcarpenter played around a bit and this is where I landed with a long organization name: with a long plan name: with long organization and long plan name: |
Description
This PR adds a new prop to the
OrganizationSwitchercomponent,showPlanName, that renders a badge when the current active organization is subscribed to a Clerk Billing Plan. The badge has an animated loading state (since it's resolved separately from the Organization information). Free plans use theprimarycolor scheme, whereas paid plans use thesecondary. This gives paid plans more visual weight.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
showPlanNameoption to display the active organization’s Billing plan name in the Organization Switcher.