-
Notifications
You must be signed in to change notification settings - Fork 386
fix(clerk-js): Remove flickers from PricingTable when signed in #6535
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
74ceaf3
f04752b
9cd7057
a93674f
630fb83
0bb3c52
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
'@clerk/clerk-js': patch | ||
--- | ||
|
||
Remove flickers from PricingTable when signed in. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -12,10 +12,18 @@ const PricingTableRoot = (props: PricingTableProps) => { | |
const clerk = useClerk(); | ||
const { mode = 'mounted', signInMode = 'redirect' } = usePricingTableContext(); | ||
const isCompact = mode === 'modal'; | ||
const { subscriptionItems } = useSubscription(); | ||
const { data: subscription, subscriptionItems } = useSubscription(); | ||
const { data: plans } = usePlans(); | ||
const { handleSelectPlan } = usePlansContext(); | ||
|
||
const plansToRender = useMemo(() => { | ||
return clerk.isSignedIn | ||
? subscription // All users in billing-enabled applications have a subscription | ||
? plans | ||
: [] | ||
: plans; | ||
}, [clerk.isSignedIn, plans, subscription]); | ||
Comment on lines
+19
to
+25
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 Verification agent 🧩 Analysis chainFix loading vs null handling to avoid hiding plans for eligible signed-in users useSubscription may return:
The current truthy check hides plans for both “loading” and “no subscription” cases. That risks permanently rendering an empty list for signed-in users who legitimately have no subscription (unless the platform guarantees a subscription object always exists). Gate on the loading state instead. Apply this diff to make the logic robust and intentional: - const plansToRender = useMemo(() => {
- return clerk.isSignedIn
- ? subscription // All users in billing-enabled applications have a subscription
- ? plans
- : []
- : plans;
- }, [clerk.isSignedIn, plans, subscription]);
+ const plansToRender = useMemo(() => {
+ if (!clerk.isSignedIn) {
+ return plans;
+ }
+ // Hide plans while the subscription is loading to prevent flicker.
+ // Once the request has resolved (even if it returns null), render plans.
+ return isSubscriptionLoading ? [] : plans;
+ }, [clerk.isSignedIn, isSubscriptionLoading, plans]); Follow-up:
Additionally, please add tests covering:
I can scaffold test cases with mocked contexts for these states if helpful. 🏁 Script executed: #!/bin/bash
# Locate the definition(s) and usage of `useSubscription` in the codebase
rg "useSubscription" -n packages/clerk-js/src Length of output: 2324 #!/bin/bash
# Show the implementation of useSubscription in Plans.tsx
rg -n "export const useSubscription" -A20 packages/clerk-js/src/ui/contexts/components/Plans.tsx 🏁 Script executed: #!/bin/bash
# Show the implementation of useSubscription in Plans.tsx to inspect its return shape
rg -n "export const useSubscription" -A20 packages/clerk-js/src/ui/contexts/components/Plans.tsx Length of output: 720 🏁 Script executed: #!/bin/bash
# Locate definition of __experimental_useSubscription
rg -n "__experimental_useSubscription" -n Length of output: 468 🏁 Script executed: #!/bin/bash
# Display first 200 lines of useSubscription implementation to verify available return fields
sed -n '1,200p' packages/shared/src/react/hooks/useSubscription.ts Length of output: 160 #!/bin/bash
# Find the implementation file for useSubscription in shared package
find packages/shared/src -type f | grep -i useSubscription #!/bin/bash
# List all files under the shared react hooks directory to locate useSubscription implementation
echo "Files in packages/shared/src/react/hooks:"
find packages/shared/src/react/hooks -type f
# Display the contents of any file matching "useSubscription" in that directory
echo ""
echo "=== useSubscription Hook Implementation ==="
for file in $(find packages/shared/src/react/hooks -type f | grep -i useSubscription); do
echo "----- $file -----"
sed -n '1,200p' "$file"
echo ""
done #!/bin/bash
# List all files in the shared react hooks directory
echo "=== files in packages/shared/src/react/hooks ==="
find packages/shared/src/react/hooks -type f
# Search for any implementation of useSubscription within that directory
echo ""
echo "=== search for useSubscription implementation ==="
rg -n "useSubscription" -n packages/shared/src/react/hooks #!/bin/bash
# List all files in the shared package to locate the actual useSubscription implementation
echo "=== Files matching useSubscription in packages/shared ==="
find packages/shared -type f | grep -i useSubscription Update The current logic ( • File: Recommended diff: - const plansToRender = useMemo(() => {
- return clerk.isSignedIn
- ? subscription // All users in billing-enabled applications have a subscription
- ? plans
- : []
- : plans;
- }, [clerk.isSignedIn, plans, subscription]);
+ const plansToRender = useMemo(() => {
+ const { isLoading: isSubscriptionLoading } = useSubscription();
+ if (!clerk.isSignedIn) {
+ return plans;
+ }
+ // Hide plans only while the subscription is loading.
+ // Once loaded (even if `subscription` is null), render plans.
+ return isSubscriptionLoading ? [] : plans;
+ }, [clerk.isSignedIn, plans, useSubscription().isLoading]); Follow-up actions:
🤖 Prompt for AI Agents
|
||
|
||
const defaultPlanPeriod = useMemo(() => { | ||
if (isCompact) { | ||
const upcomingSubscription = subscriptionItems?.find(sub => sub.status === 'upcoming'); | ||
|
@@ -72,15 +80,15 @@ const PricingTableRoot = (props: PricingTableProps) => { | |
> | ||
{mode !== 'modal' && (props as any).layout === 'matrix' ? ( | ||
<PricingTableMatrix | ||
plans={plans} | ||
plans={plansToRender} | ||
planPeriod={planPeriod} | ||
setPlanPeriod={setPlanPeriod} | ||
onSelect={selectPlan} | ||
highlightedPlan={(props as any).highlightPlan} | ||
/> | ||
) : ( | ||
<PricingTableDefault | ||
plans={plans} | ||
plans={plansToRender} | ||
planPeriod={planPeriod} | ||
setPlanPeriod={setPlanPeriod} | ||
onSelect={selectPlan} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -175,3 +175,231 @@ describe('PricingTable - trial info', () => { | |
}); | ||
}); | ||
}); | ||
|
||
describe('PricingTable - plans visibility', () => { | ||
const testPlan = { | ||
id: 'plan_test', | ||
name: 'Test Plan', | ||
fee: { | ||
amount: 1000, | ||
amountFormatted: '10.00', | ||
currencySymbol: '$', | ||
currency: 'USD', | ||
}, | ||
annualFee: { | ||
amount: 10000, | ||
amountFormatted: '100.00', | ||
currencySymbol: '$', | ||
currency: 'USD', | ||
}, | ||
annualMonthlyFee: { | ||
amount: 833, | ||
amountFormatted: '8.33', | ||
currencySymbol: '$', | ||
currency: 'USD', | ||
}, | ||
description: 'Test plan description', | ||
hasBaseFee: true, | ||
isRecurring: true, | ||
isDefault: false, | ||
forPayerType: 'user', | ||
publiclyVisible: true, | ||
slug: 'test', | ||
avatarUrl: '', | ||
features: [] as any[], | ||
freeTrialEnabled: false, | ||
freeTrialDays: 0, | ||
__internal_toSnapshot: jest.fn(), | ||
pathRoot: '', | ||
reload: jest.fn(), | ||
} as const; | ||
|
||
it('shows no plans when user is signed in but has no subscription', async () => { | ||
const { wrapper, fixtures, props } = await createFixtures(f => { | ||
f.withUser({ email_addresses: ['[email protected]'] }); | ||
}); | ||
|
||
// Provide empty props to the PricingTable context | ||
props.setProps({}); | ||
|
||
fixtures.clerk.billing.getPlans.mockResolvedValue({ data: [testPlan as any], total_count: 1 }); | ||
// Mock no subscription for signed-in user - empty subscription object | ||
fixtures.clerk.billing.getSubscription.mockResolvedValue({ | ||
subscriptionItems: [], | ||
pathRoot: '', | ||
reload: jest.fn(), | ||
} as any); | ||
|
||
const { queryByRole } = render(<PricingTable />, { wrapper }); | ||
|
||
await waitFor(() => { | ||
// Should not show any plans when signed in but no subscription | ||
expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); | ||
}); | ||
}); | ||
Comment on lines
+233
to
+239
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Harden “absence” assertions to avoid false positives; wait for data fetches before asserting Negative assertions wrapped in Apply these diffs: @@
- const { queryByRole } = render(<PricingTable />, { wrapper });
-
- await waitFor(() => {
- // Should not show any plans when signed in but no subscription
- expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument();
- });
+ const { queryByRole } = render(<PricingTable />, { wrapper });
+ await waitFor(() => expect(fixtures.clerk.billing.getPlans).toHaveBeenCalled());
+ await waitFor(() => expect(fixtures.clerk.billing.getSubscription).toHaveBeenCalled());
+ // Should not show any plans when signed in but no subscription
+ expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); @@
- const { queryByRole } = render(<PricingTable />, { wrapper });
-
- await waitFor(() => {
- // Should not show any plans when signed in but subscription is null
- expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument();
- });
+ const { queryByRole } = render(<PricingTable />, { wrapper });
+ await waitFor(() => expect(fixtures.clerk.billing.getPlans).toHaveBeenCalled());
+ await waitFor(() => expect(fixtures.clerk.billing.getSubscription).toHaveBeenCalled());
+ // Should not show any plans when signed in but subscription is null
+ expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); @@
- const { queryByRole } = render(<PricingTable />, { wrapper });
-
- await waitFor(() => {
- // Should not show any plans when signed in but subscription is undefined (loading)
- expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument();
- });
+ const { queryByRole } = render(<PricingTable />, { wrapper });
+ await waitFor(() => expect(fixtures.clerk.billing.getPlans).toHaveBeenCalled());
+ await waitFor(() => expect(fixtures.clerk.billing.getSubscription).toHaveBeenCalled());
+ // Should not show any plans when signed in but subscription is undefined (loading)
+ expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); @@
- // Assert no plans render while subscription is pending
- await waitFor(() => {
- expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument();
- });
+ // Assert no plans render while subscription is pending
+ await waitFor(() => expect(fixtures.clerk.billing.getPlans).toHaveBeenCalled());
+ expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); Also applies to: 319-325, 339-345, 366-369 |
||
|
||
it('shows plans when user is signed in and has a subscription', async () => { | ||
const { wrapper, fixtures, props } = await createFixtures(f => { | ||
f.withUser({ email_addresses: ['[email protected]'] }); | ||
}); | ||
|
||
// Provide empty props to the PricingTable context | ||
props.setProps({}); | ||
|
||
fixtures.clerk.billing.getPlans.mockResolvedValue({ data: [testPlan as any], total_count: 1 }); | ||
// Mock active subscription for signed-in user | ||
fixtures.clerk.billing.getSubscription.mockResolvedValue({ | ||
id: 'sub_active', | ||
status: 'active', | ||
activeAt: new Date('2021-01-01'), | ||
createdAt: new Date('2021-01-01'), | ||
nextPayment: null, | ||
pastDueAt: null, | ||
updatedAt: null, | ||
subscriptionItems: [ | ||
{ | ||
id: 'si_active', | ||
plan: testPlan, | ||
createdAt: new Date('2021-01-01'), | ||
paymentSourceId: 'src_1', | ||
pastDueAt: null, | ||
canceledAt: null, | ||
periodStart: new Date('2021-01-01'), | ||
periodEnd: new Date('2021-01-31'), | ||
planPeriod: 'month' as const, | ||
status: 'active' as const, | ||
isFreeTrial: false, | ||
cancel: jest.fn(), | ||
pathRoot: '', | ||
reload: jest.fn(), | ||
}, | ||
], | ||
pathRoot: '', | ||
reload: jest.fn(), | ||
}); | ||
|
||
const { getByRole } = render(<PricingTable />, { wrapper }); | ||
|
||
await waitFor(() => { | ||
// Should show plans when signed in and has subscription | ||
expect(getByRole('heading', { name: 'Test Plan' })).toBeVisible(); | ||
}); | ||
}); | ||
|
||
it('shows plans when user is signed out', async () => { | ||
const { wrapper, fixtures, props } = await createFixtures(); | ||
|
||
// Provide empty props to the PricingTable context | ||
props.setProps({}); | ||
|
||
fixtures.clerk.billing.getPlans.mockResolvedValue({ data: [testPlan as any], total_count: 1 }); | ||
// When signed out, getSubscription should throw or return empty response | ||
fixtures.clerk.billing.getSubscription.mockRejectedValue(new Error('Unauthenticated')); | ||
|
||
const { getByRole } = render(<PricingTable />, { wrapper }); | ||
|
||
await waitFor(() => { | ||
// Should show plans when signed out | ||
expect(getByRole('heading', { name: 'Test Plan' })).toBeVisible(); | ||
}); | ||
}); | ||
|
||
it('shows no plans when user is signed in but subscription is null', async () => { | ||
const { wrapper, fixtures, props } = await createFixtures(f => { | ||
f.withUser({ email_addresses: ['[email protected]'] }); | ||
}); | ||
|
||
// Provide empty props to the PricingTable context | ||
props.setProps({}); | ||
|
||
fixtures.clerk.billing.getPlans.mockResolvedValue({ data: [testPlan as any], total_count: 1 }); | ||
// Mock null subscription response (different from throwing error) | ||
fixtures.clerk.billing.getSubscription.mockResolvedValue(null as any); | ||
|
||
const { queryByRole } = render(<PricingTable />, { wrapper }); | ||
|
||
await waitFor(() => { | ||
// Should not show any plans when signed in but subscription is null | ||
expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); | ||
}); | ||
}); | ||
|
||
it('shows no plans when user is signed in but subscription is undefined', async () => { | ||
const { wrapper, fixtures, props } = await createFixtures(f => { | ||
f.withUser({ email_addresses: ['[email protected]'] }); | ||
}); | ||
|
||
// Provide empty props to the PricingTable context | ||
props.setProps({}); | ||
|
||
fixtures.clerk.billing.getPlans.mockResolvedValue({ data: [testPlan as any], total_count: 1 }); | ||
// Mock undefined subscription response (loading state) | ||
fixtures.clerk.billing.getSubscription.mockResolvedValue(undefined as any); | ||
|
||
const { queryByRole } = render(<PricingTable />, { wrapper }); | ||
|
||
await waitFor(() => { | ||
// Should not show any plans when signed in but subscription is undefined (loading) | ||
expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); | ||
}); | ||
}); | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
it('prevents flicker by not showing plans while subscription is loading', async () => { | ||
const { wrapper, fixtures, props } = await createFixtures(f => { | ||
f.withUser({ email_addresses: ['[email protected]'] }); | ||
}); | ||
|
||
// Provide empty props to the PricingTable context | ||
props.setProps({}); | ||
|
||
fixtures.clerk.billing.getPlans.mockResolvedValue({ data: [testPlan as any], total_count: 1 }); | ||
|
||
// Create a pending promise and capture its resolver | ||
let resolveSubscription!: (value: any) => void; | ||
const pendingSubscriptionPromise = new Promise<any>(resolve => { | ||
resolveSubscription = resolve; | ||
}); | ||
fixtures.clerk.billing.getSubscription.mockReturnValue(pendingSubscriptionPromise); | ||
|
||
const { queryByRole, findByRole } = render(<PricingTable />, { wrapper }); | ||
|
||
// Assert no plans render while subscription is pending | ||
await waitFor(() => { | ||
expect(queryByRole('heading', { name: 'Test Plan' })).not.toBeInTheDocument(); | ||
}); | ||
|
||
// Resolve the subscription with an active subscription object | ||
resolveSubscription({ | ||
id: 'sub_active', | ||
status: 'active', | ||
activeAt: new Date('2021-01-01'), | ||
createdAt: new Date('2021-01-01'), | ||
nextPayment: null, | ||
pastDueAt: null, | ||
updatedAt: null, | ||
subscriptionItems: [ | ||
{ | ||
id: 'si_active', | ||
plan: testPlan, | ||
createdAt: new Date('2021-01-01'), | ||
paymentSourceId: 'src_1', | ||
pastDueAt: null, | ||
canceledAt: null, | ||
periodStart: new Date('2021-01-01'), | ||
periodEnd: new Date('2021-01-31'), | ||
planPeriod: 'month' as const, | ||
status: 'active' as const, | ||
isFreeTrial: false, | ||
cancel: jest.fn(), | ||
pathRoot: '', | ||
reload: jest.fn(), | ||
}, | ||
], | ||
pathRoot: '', | ||
reload: jest.fn(), | ||
}); | ||
|
||
// Assert the plan heading appears after subscription resolves | ||
await findByRole('heading', { name: 'Test Plan' }); | ||
}); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Destructure loading flag from useSubscription to drive the rendering guard
Expose isLoading to differentiate “loading” vs “no subscription” states as suggested in the plansToRender fix.
📝 Committable suggestion
🤖 Prompt for AI Agents