chore(clerk-js): Use error metadata for invalid change plan screen on <Checkout />
#6102
Conversation
🦋 Changeset detectedLatest commit: 49eaec2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 22 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 |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
📝 WalkthroughWalkthroughThe changes introduce a new test for subscription plan restrictions, refactor the pricing table page object for better modularity, and enhance error handling by including plan details in error metadata. Several UI components are updated to extract and display plan information from error objects, and related type definitions are extended to support the new error metadata structure. Additionally, plan-related state and logic are removed from the checkout page component, and the invalid plan error component is renamed and refactored to use error metadata. Changes
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (5)
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
@clerk/agent-toolkit
@clerk/astro
@clerk/backend
@clerk/chrome-extension
@clerk/clerk-js
@clerk/dev-cli
@clerk/elements
@clerk/clerk-expo
@clerk/expo-passkeys
@clerk/express
@clerk/fastify
@clerk/localizations
@clerk/nextjs
@clerk/nuxt
@clerk/clerk-react
@clerk/react-router
@clerk/remix
@clerk/shared
@clerk/tanstack-react-start
@clerk/testing
@clerk/themes
@clerk/types
@clerk/upgrade
@clerk/vue
commit: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🔭 Outside diff range comments (1)
packages/clerk-js/src/ui/components/Checkout/CheckoutPage.tsx (1)
92-105: 🛠️ Refactor suggestion
useMemodependency list & error scan need tightening
checkout?.statusis redundant in the deps array becausecheckoutalready covers identity changes; conversely, if the same object is reused but the status mutates internally, React cannot detect it. Prefer derivingstatusoutside the object or always replacing the object.Only
errors?.[0]is inspected forinvalid_plan_change, risking mis-classification (cf. comment inparts.tsx). Useerrors?.some(e => e.code === invalidChangeCode)instead.
useEffect(() => void startCheckout(), [])never re-fires whenplanId/planPeriodchange. If the user toggles plans in the UI, the checkout flow will remain stale. Add them to the dependency list or document immutability.
♻️ Duplicate comments (1)
packages/shared/src/error.ts (1)
103-116: ```diff
plan: error?.meta?.plan,
...(error?.meta?.plan ? { plan: error.meta.plan } : {}),Apply to keep JSON minimal. </blockquote></details> </blockquote></details> <details> <summary>🧹 Nitpick comments (3)</summary><blockquote> <details> <summary>packages/shared/src/error.ts (2)</summary><blockquote> `87-100`: **Only include `plan` in `meta` when present.** Serialising `plan: undefined` pollutes the payload and may break strict back-end schemas. Add a conditional spread instead: ```diff - plan: error?.meta?.plan, + ...(error?.meta?.plan ? { plan: error.meta.plan } : {}),Replicate the same pattern in
errorToJSON.
273-275: Delete or fill the empty JSDoc block.An empty comment triggers lint/docs tooling noise. Either document
buildErrorThroweror remove these lines.-/** - * - */ +integration/tests/pricing-table.test.ts (1)
320-351: Make the assertion resilient to localization changes.The test matches an exact English sentence; if copy or locales change the suite will become flaky. Consider asserting on the presence of the info
Alertelement or the error code instead:await expect( page.locator('.cl-checkout-root [data-test-id="invalid-plan-change-alert"]'), ).toBeVisible();(where the component exposes
data-test-id).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
integration/tests/pricing-table.test.ts(1 hunks)packages/clerk-js/src/ui/components/Checkout/CheckoutPage.tsx(3 hunks)packages/clerk-js/src/ui/components/Checkout/index.tsx(2 hunks)packages/clerk-js/src/ui/components/Checkout/parts.tsx(3 hunks)packages/shared/src/error.ts(3 hunks)packages/testing/src/playwright/unstable/page-objects/pricingTable.ts(2 hunks)packages/types/src/api.ts(1 hunks)packages/types/src/json.ts(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
packages/clerk-js/src/ui/components/Checkout/index.tsx (1)
packages/clerk-js/src/ui/components/Checkout/parts.tsx (1)
InvalidPlanScreen(39-95)
integration/tests/pricing-table.test.ts (1)
integration/testUtils/index.ts (1)
createTestUtils(23-85)
packages/testing/src/playwright/unstable/page-objects/pricingTable.ts (1)
packages/clerk-js/src/ui/styledSystem/common.ts (1)
common(204-216)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: semgrep/ci
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
packages/clerk-js/src/ui/components/Checkout/index.tsx (2)
10-10: Remove leftoverInvalidPlanErrorexports/imports.
InvalidPlanScreenfully replacesInvalidPlanError; make sure no code path still exports or imports the old component to avoid unused-export/tree-shaking warnings.
42-44: LGTM – new screen wired correctly.The stage now renders
InvalidPlanScreen; matches the new metadata-driven flow and keeps JSX tidy.packages/types/src/json.ts (1)
352-358: Verify naming consistency against API contract.All other
metafields in JSON are snake_case; the new nested fields (annual_monthly_amount_formatted, etc.) keep that style—good. Confirm the back-end really returns strings for formatted amounts (not numbers) to avoid runtime coercion.packages/types/src/api.ts (1)
32-38: LGTM – runtime type extended.
planis optional and mirrors the JSON interface; keeps parsing type-safe.packages/testing/src/playwright/unstable/page-objects/pricingTable.ts (1)
20-24: Toggle-state heuristic may be inverted
data-checked === 'true'is mapped to monthly. If the design system flips the semantic (checked ⇢ annual), the helper silently sets the wrong period.
Consider reading an explicitdata-periodattribute or the visible price label instead of boolean state.
| title={planFromError.name} | ||
| description={planPeriod === 'annual' ? localizationKeys('commerce.billedAnnually') : undefined} | ||
| /> | ||
| <LineItems.Description | ||
| prefix={planPeriod === 'annual' ? 'x12' : undefined} | ||
| text={`${plan.currencySymbol}${planPeriod === 'month' ? plan.amountFormatted : plan.annualMonthlyAmountFormatted}`} | ||
| text={`${planFromError.currency_symbol}${planPeriod === 'month' ? planFromError.amount_formatted : planFromError.annual_monthly_amount_formatted}`} | ||
| suffix={localizationKeys('commerce.checkout.perMonth')} | ||
| /> |
There was a problem hiding this comment.
Missing null-guards for optional plan fields
planFromError.currency_symbol, amount_formatted, annual_monthly_amount_formatted are all optional according to the API typings.
If the backend omits any of them the component will crash during render.
Guard or provide fallbacks, e.g.
- text={`${planFromError.currency_symbol}${planPeriod === 'month' ? planFromError.amount_formatted : planFromError.annual_monthly_amount_formatted}`}
+ text={`${planFromError?.currency_symbol ?? ''}${planPeriod === 'month'
+ ? planFromError?.amount_formatted ?? '--'
+ : planFromError?.annual_monthly_amount_formatted ?? '--'}`}📝 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.
| title={planFromError.name} | |
| description={planPeriod === 'annual' ? localizationKeys('commerce.billedAnnually') : undefined} | |
| /> | |
| <LineItems.Description | |
| prefix={planPeriod === 'annual' ? 'x12' : undefined} | |
| text={`${plan.currencySymbol}${planPeriod === 'month' ? plan.amountFormatted : plan.annualMonthlyAmountFormatted}`} | |
| text={`${planFromError.currency_symbol}${planPeriod === 'month' ? planFromError.amount_formatted : planFromError.annual_monthly_amount_formatted}`} | |
| suffix={localizationKeys('commerce.checkout.perMonth')} | |
| /> | |
| title={planFromError.name} | |
| description={planPeriod === 'annual' ? localizationKeys('commerce.billedAnnually') : undefined} | |
| /> | |
| <LineItems.Description | |
| prefix={planPeriod === 'annual' ? 'x12' : undefined} | |
| text={`${planFromError?.currency_symbol ?? ''}${planPeriod === 'month' | |
| ? planFromError?.amount_formatted ?? '--' | |
| : planFromError?.annual_monthly_amount_formatted ?? '--'}`} | |
| suffix={localizationKeys('commerce.checkout.perMonth')} | |
| /> |
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/components/Checkout/parts.tsx around lines 74 to 81,
the properties planFromError.currency_symbol, amount_formatted, and
annual_monthly_amount_formatted are optional and may be undefined, which can
cause the component to crash during render. Add null-guards or provide default
fallback values (such as empty strings) when accessing these fields to ensure
the component handles missing data gracefully without crashing.
panteliselef
deleted the
elef/invalid-plan-change-screen-to-use-data-from-error
branch
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit