chore(clerk-js,localizations): Tidy up some commerce localizations #6101
Conversation
🦋 Changeset detectedLatest commit: 7cb2b0f 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 ↗︎
|
📝 Walkthrough## Walkthrough
This change introduces context-aware localization for billing-related UI components, dynamically adjusting localization keys based on whether the subscriber is a user or an organization. New localization keys and type definitions are added to support billing statements, payment history, and subscription lists. Minor navigation and UI adjustments are also included.
## Changes
| Files/Paths | Change Summary |
|-------------------------------------------------------------------------------------------------|---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| .../OrganizationProfile/OrganizationPlansPage.tsx<br>.../UserProfile/PlansPage.tsx | Updated the back link in header components to navigate to the parent route with the query parameter `tab=subscriptions` instead of `tab=plans`. No other logic or UI changes. |
| .../PaymentAttempts/PaymentAttemptPage.tsx<br>.../PaymentAttempts/PaymentAttemptsList.tsx | Incorporated subscriber type context to dynamically select localization keys for messages, headers, and labels. Replaced hardcoded localization key prefixes with context-sensitive ones, enabling dynamic localization for both users and organizations in payment attempt components. |
| .../PaymentSources/AddPaymentSource.tsx<br>.../PaymentSources/PaymentSources.tsx | Introduced context-aware localization for button labels, titles, messages, and action labels based on subscriber type. Removed unnecessary console logs from error handling. All localization key usages now use a dynamic prefix according to subscriber context. |
| .../Statements/Statement.tsx | Applied a style override to render description text in lowercase within the `SectionContentDetailsHeader` component. No logic changes. |
| .../Statements/StatementPage.tsx<br>.../Statements/StatementsList.tsx | Enhanced localization by dynamically selecting keys based on subscriber type for all UI strings, captions, table headers, and empty state messages in statements components. All static text replaced with localized equivalents. |
| .../Subscriptions/SubscriptionsList.tsx | Replaced static table header labels with dynamic localization keys based on subscriber type context for the subscriptions list. No other logic changes. |
| .../localizations/src/en-US.ts | Added new localization keys for billing, payment history, statements, and subscription list sections under both user and organization profiles. Modified some existing keys for clarity and singular/plural consistency. |
| .../types/src/localization.ts | Extended the `_LocalizationResource` type with new fields and nested objects for billing-related localization, including statements, payment history, and subscription list sections for both user and organization profiles, and added a `totalDue` field under `commerce`. |
| .../contexts/components/SubscriberType.ts | Added new hook `useSubscriberTypeLocalizationRoot` that returns the localization root key string based on subscriber type context ('userProfile' or 'organizationProfile'). |
| .../core/resources/CommerceSettings.ts | Removed optional chaining from billing property accesses in `CommerceSettings.fromJSON`, assuming `data.billing` is always defined. |
| .../core/resources/CommercePayment.ts | Changed timestamp properties `failedAt`, `paidAt`, and `updatedAt` from `number` to `Date` and updated deserialization logic accordingly. |
| .../core/resources/CommercePaymentSource.ts | Added fallback default array `['card']` for `paymentMethodOrder` property if missing during deserialization in `CommerceInitializedPaymentSource.fromJSON`. |
| .../core/resources/CommerceStatement.ts | Changed `timestamp` property type from `number` to `Date` in `CommerceStatement` and `CommerceStatementGroup` classes, updating deserialization accordingly. |
| .../ui/elements/DataTable.tsx | Added new reusable `DataTable` React component with pagination, loading, and empty state support, along with `DataTableEmptyRow` and `DataTableRow` components for consistent table rendering and styling. |
| .../ui/utils/formatDate.ts | Extended `formatDate` utility to support a new `'monthyear'` format that omits the day component, including month and year. |
| .../ui/utils/index.ts | Exported new utility function `truncateTextWithEndVisible`. |
| .../clerk-js/bundlewatch.config.json | Increased maximum allowed bundle size limits slightly for `ui-common*.js` and `paymentSources*.js` files. |
## Suggested reviewers
- tmilewski📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ 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: 0
🔭 Outside diff range comments (3)
packages/clerk-js/src/ui/components/Statements/StatementsList.tsx (1)
191-205: 🛠️ Refactor suggestion
colSpanhard-coded to 4 – now mismatches 2-column table
DataTableEmptyRowstill spans 4 columns, but headers/rows only render 2 cells after this change. Browsers tolerate the mismatch, yet it’s semantically wrong and may break future accessibility tooling.-<Td colSpan={4}> +<Td colSpan={headers.length}>Update the prop (and pass
headersdown or compute locally) so the empty row always spans the correct number of columns.packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx (1)
121-124:⚠️ Potential issueAvoid in-place sort & fix comparator contract
Array.prototype.sortmutates the original array returned from SWR.
- This can break memoization elsewhere because SWR keeps a reference to the same array.
- A comparator must return
0when two elements are equal; returning1unconditionally violates the spec and leads to unstable sorting.- const sortedPaymentSources = useMemo( - () => paymentSources.sort((a, b) => (a.isDefault && !b.isDefault ? -1 : 1)), - [paymentSources], - ); + const sortedPaymentSources = useMemo( + () => + [...paymentSources] // copy to keep SWR cache immutable + .sort((a, b) => { + if (a.isDefault === b.isDefault) { + return 0; // stable when equal + } + return a.isDefault ? -1 : 1; // default card first + }), + [paymentSources], + );packages/clerk-js/src/ui/components/Statements/StatementPage.tsx (1)
118-121:⚠️ Potential issue“Total paid” is still hard-coded
After the localization overhaul nearly every label is keyed except the footer:
<Statement.Footer label='Total paid' // <-- hard-codedAdd a key, e.g.:
- label='Total paid' + label={localizationKeys('commerce.totalPaid')}(And of course supply translations under both user/organization roots if required.)
♻️ Duplicate comments (3)
packages/clerk-js/src/ui/components/UserProfile/PlansPage.tsx (1)
22-24: See previous comment – duplicated literalSame comment as in
OrganizationPlansPage.tsx. Extract the"subscriptions"tab literal to a shared constant.packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx (1)
33-35: Same helper / duplication comment appliesThe ternary for
localizationRootre-appears here. Centralising avoids risking typos (e.g.'org'vs'organization') and makes tests simpler.packages/clerk-js/src/ui/components/Statements/StatementPage.tsx (1)
13-16: Consistency: reuse helper forlocalizationRootSame duplication note as above.
🧹 Nitpick comments (12)
packages/clerk-js/src/ui/components/Statements/Statement.tsx (1)
215-217: Avoid forcing lowercase on localized strings
textTransform: 'lowercase'will indiscriminately lowercase the description for every locale, which can mangle proper nouns, brand names, and non-Latin scripts (e.g. German “ß”, Turkish “İ”, etc.). Let translators decide the exact casing or provide an already-formatted key instead.- sx={() => ({ - textTransform: 'lowercase', - })} + // Deliberately leave casing untouched; translators/localization keys control presentation.Do you foresee any actual UI where forcing lowercase is required? If not, consider dropping the rule entirely.
packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationPlansPage.tsx (1)
25-27: Hard-coded query param duplicated in several files
new URLSearchParams('tab=subscriptions')appears here (and inUserProfile/PlansPage.tsx). Duplicating the string literal increases drift-risk if the tab name ever changes.-onClick={() => void navigate('../', { searchParams: new URLSearchParams('tab=subscriptions') })} +onClick={() => void navigate('../', { searchParams: new URLSearchParams(`tab=${SUBSCRIPTIONS_TAB}`) })} ... +const SUBSCRIPTIONS_TAB = 'subscriptions';Expose
SUBSCRIPTIONS_TABfrom a shared config to keep both pages in sync.packages/clerk-js/src/ui/components/Subscriptions/SubscriptionsList.tsx (1)
38-97: RepeatedlocalizationRootlogic could live in a hook/utilitySeveral components now compute the root with
subscriberType === 'user' ? 'userProfile' : 'organizationProfile'.
Consider a helper such asuseLocalizationRoot()to avoid repetition and typos:const localizationRoot = useLocalizationRoot(); // returns 'userProfile' | 'organizationProfile'This keeps the component lean and centralises future changes (e.g. a third
subscriber type).packages/clerk-js/src/ui/components/Statements/StatementsList.tsx (2)
6-20: Import path consistencyMost sibling components import contexts via
'../../contexts'; this file uses'../../../ui/contexts'. Both resolve, but mixing styles hampers grepping and may break if folder depth changes. Prefer the two-level path for consistency:-import { useStatements, useSubscriberTypeContext } from '../../../ui/contexts'; +import { useStatements, useSubscriberTypeContext } from '../../contexts';
32-47: Helper forlocalizationRootSame duplication observation as in
SubscriptionsList.tsx. A shared helper keeps logic DRY and consistent.packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx (1)
26-27: Factor-out repeatedlocalizationRootcalculationThe ternary
subscriberType === 'user' ? 'userProfile' : 'organizationProfile'is duplicated four times in this file (and many more across the PR).
Placing this logic inuseSubscriberTypeContext(e.g.useLocalizationRoot()) or a tiny helper eliminates repetition and the risk of drifting logic if new subscriber types appear ('org'vs'organization', etc.).Also applies to: 68-69, 114-115, 203-204
packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx (1)
314-316: Type-safety lost with dynamic template literal
localizationKeys(...)is designed to narrow to specific union literals for type-safety. Passing a runtime–built string (${localizationRoot}...) degrades that guarantee tostring.
If you adopt a helper as proposed (getBillingKey('paymentSourcesSection.formButtonPrimary__add')) you can preserve compile-time checking while still being dynamic.packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx (1)
44-48: MissinglocalizationKeysfor “copy” aria-labelMost visible strings are now localized, but the
aria-labelinCopyButton(“Copy” / “Copied”) remains hard-coded a few lines below.
Leaving mixed locales is jarring for screen-reader users. Please create keys undercommerce.copy/commerce.copied(or similar) and reference them.packages/types/src/localization.ts (3)
765-775: Consider extracting a sharedBillingStatementsSectiontype to avoid duplication.
statementsSectionis declared twice (user & org profiles) with identical shape. Typing it once and re-using would:
- Remove boilerplate from this already-huge declaration.
- Guarantee both sections stay in sync when new fields are introduced.
Something along the lines of:
type _StatementsSection = { empty: LocalizationValue; itemCaption__paidForPlan: LocalizationValue; itemCaption__proratedCredit: LocalizationValue; itemCaption__subscribedAndPaidForPlan: LocalizationValue; notFound: LocalizationValue; tableHeaders__date: LocalizationValue; tableHeaders__amount: LocalizationValue; title: LocalizationValue; totalPaid: LocalizationValue; }; ... billingPage: { ... statementsSection: _StatementsSection; ... }
780-786: Minor ordering nit – keep table header keys grouped.For readability, list all
tableHeaders__*keys together (plan, startDate, edit) and then the non-header keys.
Not a blocker, but helps when scanning the giant type file.
788-793:paymentHistorySectionmirrorsstatementsSection– same refactor applies.A shared
_PaymentHistorySection(empty, notFound, tableHeaders__…) reused for user & org keeps the type lean.packages/localizations/src/en-US.ts (1)
193-200: Keep key order consistent with type definition.The
notFoundkey is declared after the table headers in the EN resource but before them in the type file.
Order does not affect runtime, yet aligning helps future diffing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationPlansPage.tsx(1 hunks)packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx(5 hunks)packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx(3 hunks)packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx(2 hunks)packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx(9 hunks)packages/clerk-js/src/ui/components/Statements/Statement.tsx(1 hunks)packages/clerk-js/src/ui/components/Statements/StatementPage.tsx(6 hunks)packages/clerk-js/src/ui/components/Statements/StatementsList.tsx(3 hunks)packages/clerk-js/src/ui/components/Subscriptions/SubscriptionsList.tsx(2 hunks)packages/clerk-js/src/ui/components/UserProfile/PlansPage.tsx(1 hunks)packages/localizations/src/en-US.ts(5 hunks)packages/types/src/localization.ts(3 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
packages/clerk-js/src/ui/components/UserProfile/PlansPage.tsx (1)
packages/clerk-js/src/ui/elements/Header.tsx (1)
Header(103-108)
packages/clerk-js/src/ui/components/OrganizationProfile/OrganizationPlansPage.tsx (1)
packages/clerk-js/src/ui/elements/Header.tsx (1)
Header(103-108)
packages/clerk-js/src/ui/components/Subscriptions/SubscriptionsList.tsx (1)
packages/clerk-js/src/ui/customizables/index.ts (1)
Th(61-61)
packages/clerk-js/src/ui/components/Statements/StatementsList.tsx (2)
packages/clerk-js/src/ui/contexts/components/SubscriberType.ts (1)
useSubscriberTypeContext(6-6)packages/clerk-js/src/ui/customizables/index.ts (1)
localizationKeys(11-11)
packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx (2)
packages/clerk-js/src/ui/contexts/components/SubscriberType.ts (1)
useSubscriberTypeContext(6-6)packages/clerk-js/src/ui/customizables/index.ts (1)
localizationKeys(11-11)
packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx (2)
packages/clerk-js/src/ui/contexts/components/SubscriberType.ts (1)
useSubscriberTypeContext(6-6)packages/clerk-js/src/ui/customizables/index.ts (1)
localizationKeys(11-11)
⏰ 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 (8)
packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx (1)
239-241:subscriberTypevalue mismatch can cause wrong locale branchAll other branches treat the organization case as
'org', but the ternary here only checks for'user'.
If a future (or existing) value like'organization'is introduced, it will silently fall into the organization path without an explicit check. Consider:-const localizationRoot = subscriberType === 'user' ? 'userProfile' : 'organizationProfile'; +const localizationRoot = + subscriberType === 'user' + ? 'userProfile' + : subscriberType === 'org' + ? 'organizationProfile' + : 'userProfile'; // sensible fallback + exhaustivenessOr, even better, centralise this mapping as suggested in the previous comment.
packages/types/src/localization.ts (2)
134-135:totalDueaddition looks good – verify downstream usage.The new
totalDuekey completes the billing vocabulary and is typed correctly.
Just ensure every component that renders totals (e.g.CheckoutSummary,InvoicesList) is now readingcommerce.totalDueinstead of building the string ad-hoc.
991-1019: Keep user/org shapes perfectly in sync.The organization profile copies the exact keys introduced for the user profile – good.
After factoring the common sub-types (see above), only one place will need updates, avoiding future divergence.packages/localizations/src/en-US.ts (5)
95-97: 👍 English copy fortotalDuereads naturally.No wording issues spotted.
219-223: Double-check singular “Subscription” wording matches UI context.The label switched from “Subscriptions” → “Subscription” while the key remains
headerTitle__subscriptions.
If the pane can still show multiple subscriptions for legacy customers, the singular text might confuse.
223-234: Great to see concise captions; verify interpolation placeholders.➜
{{plan}}and{{period}}placeholders initemCaption__…must be provided by the caller – confirm all usages supply both to avoid showing raw braces.
235-242:Editheader added – ensure role-based visibility.If editing subscriptions is gated behind admin / owner roles, confirm components hide the column when the action is disallowed; otherwise the column looks empty.
842-890: User & org sections kept in lock-step – nice.String set mirrors organization section perfectly; no gaps detected.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/clerk-js/src/ui/contexts/components/SubscriberType.ts (2)
3-4: Rename constant toDEFAULT& useas constfor literal-type safetyThe current identifier
DEFAUlT(lower-case “l” instead of upper-case “L”) looks like a typo and makes the code harder to scan.
While only local, adopting the conventionalDEFAULT(all caps) clarifies intent and eliminates the visual confusion.-const DEFAUlT = 'user'; -export const SubscriberTypeContext = createContext<'user' | 'org'>(DEFAUlT); +const DEFAULT = 'user' as const; +export const SubscriberTypeContext = createContext<'user' | 'org'>(DEFAULT);
6-6: Drop the redundant fallback|| DEFAUlT
useContextalready returns the context’s default value when no provider is mounted, so the logical-OR is never reached. Removing it tightens the hook and avoids the extra branch.-export const useSubscriberTypeContext = () => useContext(SubscriberTypeContext) || DEFAUlT; +export const useSubscriberTypeContext = () => useContext(SubscriberTypeContext);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.changeset/soft-toys-flow.md(1 hunks)packages/clerk-js/bundlewatch.config.json(2 hunks)packages/clerk-js/src/core/resources/CommerceSettings.ts(1 hunks)packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx(5 hunks)packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx(3 hunks)packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx(3 hunks)packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx(10 hunks)packages/clerk-js/src/ui/components/Statements/StatementPage.tsx(7 hunks)packages/clerk-js/src/ui/components/Statements/StatementsList.tsx(3 hunks)packages/clerk-js/src/ui/components/Subscriptions/SubscriptionsList.tsx(3 hunks)packages/clerk-js/src/ui/contexts/components/SubscriberType.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- packages/clerk-js/bundlewatch.config.json
- packages/clerk-js/src/core/resources/CommerceSettings.ts
- .changeset/soft-toys-flow.md
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx
- packages/clerk-js/src/ui/components/Subscriptions/SubscriptionsList.tsx
- packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx
- packages/clerk-js/src/ui/components/PaymentSources/PaymentSources.tsx
- packages/clerk-js/src/ui/components/Statements/StatementsList.tsx
- packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx
- packages/clerk-js/src/ui/components/Statements/StatementPage.tsx
⏰ 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 (1)
packages/clerk-js/src/ui/contexts/components/SubscriberType.ts (1)
8-11: Hook logic looks good – maps subscriber type to localization root correctly
No further issues found in this block.
packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/Statements/StatementPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Remaining issues from the PR that got merged last week
- Hard coded "USD"
- Replace in place date formatting with existing utils/patterns. Either use formatDate that you introduced or preferably or introduce use a localizationModifier
- and here
<DataTable/>should not be duplicated per AIO component.- 🧹 Drop the leftover console.logs
- In a follow-up PR let's fix the useSWR + React.Context usage in PaymentAttempts.tsx as it adds unnecessary mental overhead. The fetched data are already accessible from any other component due to the shared cache.
- Looping through the cached data to find by id will not work the moment that resource you are looking for is anywhere but the first paginated page. What happens if the resource does exist in the DB but it is never fetched as part of the paginated hooks ? Consider fetching the resources by id.
packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/Statements/StatementPage.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/Statements/StatementPage.tsx
Outdated
Show resolved
Hide resolved
packages/clerk-js/src/ui/components/Statements/StatementPage.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/types/src/commerce.ts (1)
117-122: Breaking API change – timestamps switched fromnumbertoDateAll consuming apps compiled against the public
@clerk/typespackage will receive a compile-time break and may need migration logic.
Add this to the CHANGELOG and bump the major version, or provide overloads accepting both types.Consider shipping a helper:
export const toDate = (d: number | string | Date | undefined) => d instanceof Date ? d : d ? new Date(d) : undefined;…so external code mirrors the internal deserialization strategy.
Also applies to: 135-145
packages/clerk-js/src/core/resources/CommercePayment.ts (1)
35-39: Inconsistent nullability handling for date fields
paid_atandfailed_atare optional, butupdated_atis mandatory.
If back-end ever omitsupdated_atthe current code will construct an invalidDate(NaN).-this.updatedAt = new Date(data.updated_at); +this.updatedAt = data.updated_at ? new Date(data.updated_at) : new Date();Also consider normalising through a shared utility (
toDateas suggested above) to avoid triplication.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
packages/clerk-js/bundlewatch.config.json(2 hunks)packages/clerk-js/src/core/resources/CommercePayment.ts(2 hunks)packages/clerk-js/src/core/resources/CommercePaymentSource.ts(1 hunks)packages/clerk-js/src/core/resources/CommerceStatement.ts(4 hunks)packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx(3 hunks)packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx(3 hunks)packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx(4 hunks)packages/clerk-js/src/ui/components/Statements/Statement.tsx(2 hunks)packages/clerk-js/src/ui/components/Statements/StatementPage.tsx(3 hunks)packages/clerk-js/src/ui/components/Statements/StatementsList.tsx(4 hunks)packages/clerk-js/src/ui/components/Subscriptions/SubscriptionsList.tsx(3 hunks)packages/clerk-js/src/ui/elements/DataTable.tsx(1 hunks)packages/clerk-js/src/ui/utils/formatDate.ts(1 hunks)packages/clerk-js/src/ui/utils/index.ts(1 hunks)packages/localizations/src/en-US.ts(5 hunks)packages/types/src/commerce.ts(2 hunks)packages/types/src/localization.ts(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- packages/clerk-js/src/ui/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/clerk-js/bundlewatch.config.json
- packages/clerk-js/src/ui/components/Subscriptions/SubscriptionsList.tsx
- packages/clerk-js/src/ui/components/PaymentSources/AddPaymentSource.tsx
- packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptsList.tsx
- packages/clerk-js/src/ui/components/Statements/StatementsList.tsx
- packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx
- packages/clerk-js/src/ui/components/Statements/StatementPage.tsx
- packages/types/src/localization.ts
- packages/clerk-js/src/ui/components/Statements/Statement.tsx
- packages/localizations/src/en-US.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
packages/types/src/commerce.ts (1)
packages/clerk-js/src/core/resources/CommerceStatement.ts (1)
CommerceStatementGroup(38-57)
packages/clerk-js/src/ui/elements/DataTable.tsx (2)
packages/clerk-js/src/ui/customizables/index.ts (10)
LocalizationKey(12-12)Col(17-17)Flex(16-16)Thead(58-58)Tr(60-60)Th(61-61)Tbody(59-59)Td(62-62)Spinner(48-48)Text(29-29)packages/clerk-js/src/ui/styledSystem/types.ts (1)
PropsOfComponent(58-58)
⏰ 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 (3)
packages/clerk-js/src/core/resources/CommercePaymentSource.ts (1)
89-92: Validate the'card'hard-coded fallback against backend contractDefaulting to
['card']guarantees the property is neverundefined, which is good, but it also silently overrides any future change where the backend might intentionally omit the field to let the client decide the order (or use a different default).
Please confirm that this assumption is stable and documented in the API contract; otherwise consider falling back to an empty array and letting the rendering layer decide, e.g.:-this.paymentMethodOrder = data.payment_method_order ?? ['card']; +this.paymentMethodOrder = data.payment_method_order ?? [];or make the intent explicit with a comment.
packages/clerk-js/src/ui/elements/DataTable.tsx (1)
37-38: Guard against out-of-range page numbersIf
pageis 0 or exceedspageCount,startingRow/endingRowbecome inconsistent.
Consider clampingpagebefore these calculations or early-returning an empty state.packages/clerk-js/src/core/resources/CommerceStatement.ts (1)
31-32: Interface mismatch:CommerceStatementJSON.timestampstill typed asnumber
new Date(data.timestamp)presumestimestampis present. If the JSON interface wasn’t updated (or the back-end starts returning ISO strings) this will throw.
Please confirm that:
CommerceStatementJSON.timestampandCommerceStatementGroupJSON.timestampwere updated tostring | number.- A falsy guard is added:
-this.timestamp = new Date(data.timestamp); +this.timestamp = data.timestamp ? new Date(data.timestamp) : new Date();Also applies to: 52-54
| {isLoading ? ( | ||
| <Tr> | ||
| <Td colSpan={4}> | ||
| <Spinner | ||
| colorScheme='primary' | ||
| sx={{ margin: 'auto', display: 'block' }} | ||
| elementDescriptor={descriptors.spinner} | ||
| /> | ||
| </Td> | ||
| </Tr> | ||
| ) : !rows.length ? ( |
There was a problem hiding this comment.
Hard-coded colSpan={4} breaks when headers.length !== 4
Both the spinner row and the empty-state row assume exactly four columns.
If a caller renders 3 or 6 headers the table markup becomes invalid and causes layout glitches.
-<Td colSpan={4}>
+<Td colSpan={headers.length}>Apply the same change in DataTableEmptyRow.
Also applies to: 100-111
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/elements/DataTable.tsx around lines 60 to 70 and 100
to 111, the colSpan attribute is hard-coded to 4 for the spinner and empty-state
rows, which breaks the table layout if the number of headers is not 4. Replace
the hard-coded colSpan={4} with colSpan={headers.length} to dynamically match
the number of columns. Apply this change both in the spinner row and in the
DataTableEmptyRow component to ensure consistent and valid table markup
regardless of the headers count.
| export const DataTableRow = (props: PropsOfComponent<typeof Tr>) => { | ||
| return ( | ||
| <Tr | ||
| {...props} | ||
| sx={t => ({ ':hover': { backgroundColor: t.colors.$neutralAlpha50 } })} | ||
| /> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
DataTableRow overrides consumer-provided sx prop
Spreading props first and then specifying sx discards any custom styles passed in by the caller.
-export const DataTableRow = (props: PropsOfComponent<typeof Tr>) => {
- return (
- <Tr
- {...props}
- sx={t => ({ ':hover': { backgroundColor: t.colors.$neutralAlpha50 } })}
- />
- );
-};
+export const DataTableRow = React.forwardRef<HTMLTableRowElement, PropsOfComponent<typeof Tr>>(
+ ({ sx, ...rest }, ref) => (
+ <Tr
+ ref={ref}
+ {...rest}
+ sx={[
+ t => ({ ':hover': { backgroundColor: t.colors.$neutralAlpha50 } }),
+ sx,
+ ]}
+ />
+ ),
+);📝 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.
| export const DataTableRow = (props: PropsOfComponent<typeof Tr>) => { | |
| return ( | |
| <Tr | |
| {...props} | |
| sx={t => ({ ':hover': { backgroundColor: t.colors.$neutralAlpha50 } })} | |
| /> | |
| export const DataTableRow = React.forwardRef< | |
| HTMLTableRowElement, | |
| PropsOfComponent<typeof Tr> | |
| >(({ sx, ...rest }, ref) => ( | |
| <Tr | |
| ref={ref} | |
| {...rest} | |
| sx={[ | |
| t => ({ ':hover': { backgroundColor: t.colors.$neutralAlpha50 } }), | |
| sx, | |
| ]} | |
| /> | |
| )); |
🤖 Prompt for AI Agents
In packages/clerk-js/src/ui/elements/DataTable.tsx around lines 115 to 120, the
DataTableRow component spreads props before specifying the sx prop, which causes
any sx styles passed by the consumer to be overridden. To fix this, spread props
after defining the sx prop and merge the consumer's sx styles with the hover
style, ensuring custom styles are preserved and combined correctly.
There was a problem hiding this comment.
Added a few final comments. I recommend adding e2e tests for at least some of the functionality or things you consider easy to break that shouldn't.
For the changeset you can probably use an empty changeset since we haven't release the last one which is regarding the same feature.
Description
Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Enhancements
Localization