chore(clerk-js, shared): Remove SWR from clerk-js and create variants for existing internal hooks #7270
Conversation
🦋 Changeset detectedLatest commit: f820c96 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 |
WalkthroughReplaces many SWR-based hooks and mutation flows with new internal query implementations (RQ & SWR variants), centralizes cache-key and keep-previous-data utilities, adds billing/Stripe hook-driven initialization, updates UI consumers/tests to use new hooks, and adds virtual module path mappings and internal stable keys. Changes
Sequence Diagram(s)%%{init: { "themeVariables": { "sequenceActorBackground": "#F5F7FA", "sequenceNoteBackground": "#FFF9E6" } }}%%
sequenceDiagram
participant UI as Consumer Component
participant Hook as __internal_usePaymentAttemptQuery
participant Keys as usePaymentAttemptQueryCacheKeys
participant Query as useClerkQuery / SWR
participant API as Clerk.billing.getPaymentAttempt
UI->>Hook: call(paymentAttemptId, enabled?, for?)
Hook->>Keys: build queryKey(paymentAttemptId, userId, orgId, for)
Keys-->>Hook: return queryKey
Hook->>Query: start query(queryKey, enabled)
alt enabled & ids present
Query->>API: getPaymentAttempt(args)
API-->>Query: data
Query-->>Hook: data cached
else
Query-->>Hook: undefined / placeholder
end
Hook-->>UI: { data, error, isLoading, isFetching }
%%{init: { "themeVariables": { "sequenceActorBackground": "#F5F7FA", "sequenceNoteBackground": "#EFFAF0" } }}%%
sequenceDiagram
participant Provider as PaymentElementProvider
participant Init as __internal_useInitializePaymentMethod
participant Libs as __internal_useStripeClerkLibs
participant Loader as __internal_useStripeLoader
participant ClerkQ as useClerkQuery / SWR
participant Stripe as Stripe SDK
Provider->>Init: request initializePaymentMethod(for)
Init->>ClerkQ: call resource.initializePaymentMethod({gateway:'stripe'})
ClerkQ->>Stripe: returns initializedPaymentMethod
Init-->>Provider: initializedPaymentMethod + init fn
Provider->>Libs: request stripe libs
Libs->>ClerkQ: call __internal_loadStripeJs()
ClerkQ->>Stripe: returns { loadStripe }
Libs-->>Provider: loadStripe
Provider->>Loader: request Stripe instance(publishableKey, account)
Loader->>ClerkQ: call loadStripe(publishableKey, { stripeAccount })
ClerkQ->>Stripe: returns Stripe instance
Loader-->>Provider: Stripe instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (6)
💤 Files with no reviewable changes (3)
🧰 Additional context used🧬 Code graph analysis (3)packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx (1)
packages/shared/src/react/hooks/useAPIKeys.swr.tsx (8)
packages/shared/src/react/hooks/useAPIKeys.rq.tsx (7)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
Comment |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Add todos
panteliselef
force-pushed
the
elef/remove-swr-from-main
branch
from
d74b809 to
2f87ceb
Compare
@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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/clerk-js/src/ui/components/APIKeys/APIKeys.tsx (1)
97-102: APIKeys.tsx inconsistency: other billing components refactored to internal hooks, but this remains on SWR.The premise is correct—PaymentAttemptPage, PlanDetails, and StatementPage have already been refactored to use internal query hooks (
__internal_usePaymentAttemptQuery,__internal_usePlanDetailsQuery,__internal_useStatementQuery) from@clerk/shared/react. However, APIKeys.tsx still uses rawuseSWRMutation(lines 5, 98–102) anduseSWRConfig(in utils.ts), creating an inconsistency.Since this is WIP and the TODO is a placeholder, decide whether to:
- Complete the refactor in this PR to align with the pattern used by other billing components (likely creating a similar
__internal_useCreateAPIKeyhook), or- Document the specific deferral plan and timeline if this is intentionally staged.
🧹 Nitpick comments (11)
packages/shared/src/react/commerce.tsx (1)
35-35: Add more detail to TODO comments for better tracking.The three TODO comments marking SWR/SWRMutation migration lack actionable context. Per the AI summary, the broader PR introduces internal react-query hooks (
__internal_useStatementQuery, etc.) elsewhere. Consider:
- Specifying the intended react-query hook equivalent (or cross-referencing where new internal hooks are defined)
- Linking to an issue tracking this migration work
- Clarifying priority or timeline
This will help future developers understand the exact migration path rather than having to infer it from the broader PR context.
Example improvements:
- // TODO: Replace useSWR with the react-query equivalent. + // TODO: Replace useSWR with __internal_useStripeLibsQuery or equivalent + // Tracking: See PR #7270 (remove SWR usage behind FF)Or if linking to an issue:
- // TODO: Replace useSWRMutation with the react-query equivalent. + // TODO(elef): Replace useSWRMutation with react-query mutation hook + // See: https://github.com/clerk/javascript/issues/XXXXXAlso applies to: 89-89, 118-118
packages/shared/src/react/providers/SWRConfigCompat.swr.tsx (1)
8-8: Make the TODO more specific and actionableThe TODO is directionally useful but a bit vague (“react-query equivalent”). Consider naming the intended target (e.g., a specific react-query provider/compat wrapper) or referencing the migration ticket/feature flag so future work here is clearly scoped and discoverable.
packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
8-33: Minor redundancy in nullish coalescing.The cache key generation logic is correct. However, on line 27,
paymentAttemptId ?? undefinedis redundant sincepaymentAttemptIdis a requiredstringparameter and can never be nullish.Consider simplifying line 27:
args: { - id: paymentAttemptId ?? undefined, + id: paymentAttemptId, orgId: orgId ?? undefined, },Note: Line 28's
orgId ?? undefinedis correct sinceorgIdcan benull.packages/shared/src/react/hooks/usePagesOrInfinite.swr.tsx (1)
93-93: Consider usingtoSWRQueryfor consistency.Line 93 still manually accesses
keys.queryKey[0]andkeys.queryKey[2]. For consistency with the refactoring at lines 53 and 138, consider extracting this to usetoSWRQueryor a similar helper.packages/shared/src/types/virtual-data-hooks.d.ts (1)
6-8: LGTM! Type declarations enable virtual module resolution.The
anytyping is pragmatic for virtual modules, though consider importing the actual hook signatures for stronger type safety as an optional improvement.packages/shared/src/react/hooks/useSubscription.swr.tsx (1)
49-56: Consider the fragility of array index access.The fetcher accesses
queryKey[3].argsandqueryKey[2].userIdusing magic indices, which can be brittle if the cache key structure changes. While this pattern is consistent with other internal hooks in the PR, consider whether destructuring or named accessors would improve maintainability.Example with destructuring (if this pattern is used widely, a helper could centralize this):
({ queryKey }) => { const [, , tracked, untracked] = queryKey; const args = untracked.args; if (tracked.userId) { return clerk.billing.getSubscription(args); } return null; }packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
51-56: Consider whether the defensive error throw is necessary.The
statementIdcheck at line 52 is technically unreachable becausequeryEnabledat line 47 already ensuresBoolean(statementId)is true before the query runs. While this defensive programming provides a safeguard if the enabled logic changes, you might consider whether it's necessary given that TypeScript should catch such issues.This pattern is consistent with the SWR implementation (useStatementQuery.swr.tsx), so keeping it maintains consistency across implementations.
packages/clerk-js/src/ui/components/Statements/StatementPage.tsx (1)
93-97: Consider removing redundant type annotations.The explicit type annotations
(group: StatementGroup)and(item: StatementItem)are redundant since TypeScript infers these types fromstatement.groupsandgroup.itemsrespectively.- {statement.groups.map((group: StatementGroup) => ( + {statement.groups.map(group => ( <Statement.Section key={group.timestamp.toISOString()}> <Statement.SectionHeader text={formatDate(group.timestamp, 'long')} /> <Statement.SectionContent> - {group.items.map((item: StatementItem) => ( + {group.items.map(item => (packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (1)
1-60: Consider aligning SWR key pattern with other hooks.The implementation is functionally correct. However, there's an inconsistency: this hook wraps the key in an object (
{ queryKey }) and destructures it in the fetcher, whileuseStatementQuery.swr.tsxandusePlanDetailsQuery.swr.tsxpassqueryKeydirectly and use closure in the fetcher.For consistency, consider refactoring to match the pattern used in other SWR hooks:
const swr = useSWR( - queryEnabled ? { queryKey } : null, - ({ queryKey }) => { - const args = queryKey[3].args; - return clerk.billing.getPaymentAttempt(args); + queryEnabled ? queryKey : null, + () => { + return clerk.billing.getPaymentAttempt({ + id: paymentAttemptId, + orgId: organizationId ?? undefined + }); },Alternatively, update the other hooks to use this more explicit pattern if dynamic arg extraction is preferred.
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
12-14: Note: KeepPreviousDataFn is duplicated across files.This helper function is identical in
usePlanDetailsQuery.rq.tsxandusePaymentAttemptQuery.rq.tsx. Consider extracting it to a shared utility file to follow DRY principles.Example:
// In packages/shared/src/react/clerk-rq/utils.ts export function keepPreviousData<T>(previousData: T): T { return previousData; }Then import and use across both files.
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (1)
17-19: Duplicate helper function.Same as noted in
usePlanDetailsQuery.rq.tsx- thisKeepPreviousDataFnhelper is duplicated. Consider extracting to a shared utility.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (35)
packages/clerk-js/src/ui/components/APIKeys/APIKeys.tsx(1 hunks)packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx(2 hunks)packages/clerk-js/src/ui/components/Plans/PlanDetails.tsx(4 hunks)packages/clerk-js/src/ui/components/Statements/StatementPage.tsx(3 hunks)packages/shared/src/react/clerk-swr.ts(1 hunks)packages/shared/src/react/commerce.tsx(3 hunks)packages/shared/src/react/hooks/__tests__/wrapper.tsx(1 hunks)packages/shared/src/react/hooks/createCacheKeys.ts(3 hunks)packages/shared/src/react/hooks/index.ts(1 hunks)packages/shared/src/react/hooks/usePageOrInfinite.types.ts(2 hunks)packages/shared/src/react/hooks/usePagesOrInfinite.swr.tsx(3 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.tsx(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts(1 hunks)packages/shared/src/react/hooks/useStatementQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.shared.ts(1 hunks)packages/shared/src/react/hooks/useStatementQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.types.ts(1 hunks)packages/shared/src/react/hooks/useSubscription.rq.tsx(3 hunks)packages/shared/src/react/hooks/useSubscription.shared.ts(1 hunks)packages/shared/src/react/hooks/useSubscription.swr.tsx(2 hunks)packages/shared/src/react/index.ts(1 hunks)packages/shared/src/react/providers/SWRConfigCompat.rq.tsx(1 hunks)packages/shared/src/react/providers/SWRConfigCompat.swr.tsx(1 hunks)packages/shared/src/react/stable-keys.ts(1 hunks)packages/shared/src/types/virtual-data-hooks.d.ts(1 hunks)packages/shared/tsconfig.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (21)
packages/shared/src/react/hooks/useStatementQuery.shared.ts (3)
packages/shared/src/types/billing.ts (1)
ForPayerType(69-69)packages/shared/src/react/hooks/createCacheKeys.ts (1)
createCacheKeys(7-23)packages/shared/src/react/stable-keys.ts (1)
INTERNAL_STABLE_KEYS(63-67)
packages/shared/src/react/hooks/useStatementQuery.types.ts (1)
packages/shared/src/types/billing.ts (2)
ForPayerType(69-69)BillingStatementResource(445-466)
packages/shared/src/types/virtual-data-hooks.d.ts (7)
packages/shared/src/react/hooks/index.ts (3)
__internal_useStatementQuery(18-18)__internal_usePlanDetailsQuery(19-19)__internal_usePaymentAttemptQuery(20-20)packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
__internal_useStatementQuery(27-70)packages/shared/src/react/hooks/useStatementQuery.swr.tsx (1)
__internal_useStatementQuery(20-62)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
__internal_usePlanDetailsQuery(22-60)packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (1)
__internal_usePlanDetailsQuery(15-50)packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (1)
__internal_usePaymentAttemptQuery(27-68)packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (1)
__internal_usePaymentAttemptQuery(20-60)
packages/shared/src/react/hooks/useStatementQuery.swr.tsx (4)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
__internal_useStatementQuery(27-70)packages/shared/src/react/hooks/useStatementQuery.types.ts (2)
UseStatementQueryParams(6-29)StatementQueryResult(34-51)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
useStatementQueryCacheKeys(8-33)
packages/shared/src/react/hooks/createCacheKeys.ts (2)
packages/shared/src/react/stable-keys.ts (2)
ResourceCacheStableKey(53-53)__internal_ResourceCacheStableKey(69-69)packages/shared/src/react/hooks/usePageOrInfinite.types.ts (1)
QueryKeyWithArgs(13-19)
packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (1)
packages/shared/src/types/billing.ts (1)
BillingPlanResource(120-187)
packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (4)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (1)
__internal_usePaymentAttemptQuery(27-68)packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (2)
UsePaymentAttemptQueryParams(7-30)PaymentAttemptQueryResult(35-52)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
usePaymentAttemptQueryCacheKeys(8-33)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (5)
packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (1)
__internal_usePaymentAttemptQuery(20-60)packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (2)
UsePaymentAttemptQueryParams(7-30)PaymentAttemptQueryResult(35-52)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
usePaymentAttemptQueryCacheKeys(8-33)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)
packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (1)
packages/shared/src/types/billing.ts (2)
ForPayerType(69-69)BillingPaymentResource(383-420)
packages/shared/src/react/hooks/useSubscription.rq.tsx (2)
packages/shared/src/react/hooks/useSubscription.shared.ts (1)
useSubscriptionCacheKeys(8-30)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (5)
packages/shared/src/react/hooks/useStatementQuery.swr.tsx (1)
__internal_useStatementQuery(20-62)packages/shared/src/react/hooks/useStatementQuery.types.ts (2)
UseStatementQueryParams(6-29)StatementQueryResult(34-51)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
useStatementQueryCacheKeys(8-33)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)
packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (3)
packages/shared/src/types/billing.ts (1)
ForPayerType(69-69)packages/shared/src/react/hooks/createCacheKeys.ts (1)
createCacheKeys(7-23)packages/shared/src/react/stable-keys.ts (1)
INTERNAL_STABLE_KEYS(63-67)
packages/clerk-js/src/ui/components/Plans/PlanDetails.tsx (4)
packages/shared/src/types/billing.ts (1)
BillingPlanResource(120-187)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
__internal_usePlanDetailsQuery(22-60)packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (1)
__internal_usePlanDetailsQuery(15-50)packages/shared/src/types/virtual-data-hooks.d.ts (1)
__internal_usePlanDetailsQuery(7-7)
packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx (2)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (1)
__internal_usePaymentAttemptQuery(27-68)packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (1)
__internal_usePaymentAttemptQuery(20-60)
packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (4)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
__internal_usePlanDetailsQuery(22-60)packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (2)
UsePlanDetailsQueryParams(7-28)PlanDetailsQueryResult(33-50)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
usePlanDetailsQueryCacheKeys(6-22)
packages/shared/src/react/hooks/useSubscription.swr.tsx (1)
packages/shared/src/react/hooks/useSubscription.shared.ts (1)
useSubscriptionCacheKeys(8-30)
packages/shared/src/react/hooks/useSubscription.shared.ts (3)
packages/shared/src/types/billing.ts (1)
ForPayerType(69-69)packages/shared/src/react/hooks/createCacheKeys.ts (1)
createCacheKeys(7-23)packages/shared/src/react/stable-keys.ts (1)
STABLE_KEYS(30-51)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (5)
packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (1)
__internal_usePlanDetailsQuery(15-50)packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (2)
UsePlanDetailsQueryParams(7-28)PlanDetailsQueryResult(33-50)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
usePlanDetailsQueryCacheKeys(6-22)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)
packages/clerk-js/src/ui/components/Statements/StatementPage.tsx (4)
packages/shared/src/types/billing.ts (1)
BillingStatementResource(445-466)packages/clerk-js/src/ui/contexts/components/SubscriberType.ts (2)
useSubscriberTypeContext(7-7)useSubscriberTypeLocalizationRoot(9-12)packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
__internal_useStatementQuery(27-70)packages/shared/src/react/hooks/useStatementQuery.swr.tsx (1)
__internal_useStatementQuery(20-62)
packages/shared/src/react/hooks/usePagesOrInfinite.swr.tsx (1)
packages/shared/src/react/hooks/createCacheKeys.ts (1)
toSWRQuery(25-32)
packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (2)
packages/shared/src/react/hooks/createCacheKeys.ts (1)
createCacheKeys(7-23)packages/shared/src/react/stable-keys.ts (1)
INTERNAL_STABLE_KEYS(63-67)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (35)
packages/shared/src/react/providers/SWRConfigCompat.rq.tsx (1)
9-9: TODO comment appropriately documents planned react-query migration.The TODO comment clearly signals the intended replacement of
SWRConfigwith a react-query equivalent. The component maintains backward compatibility during the transition without behavioral changes.Consider: If not already tracked, open a GitHub issue to formally document this migration task and link it to this PR for visibility and progress tracking during the SWR removal effort.
packages/shared/src/react/clerk-swr.ts (1)
3-3: TODO comment noted.This comment clearly documents the migration intention. No functional changes introduced.
packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (1)
1-50: Well-documented type definitions.The types are comprehensive and follow standard query hook patterns. JSDoc comments clearly describe each field and their defaults.
packages/shared/src/react/hooks/useSubscription.shared.ts (1)
8-30: LGTM! Proper cache key generation for subscriptions.The hook correctly handles organization vs user scope and uses proper memoization with the right dependencies.
packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (1)
1-52: Well-structured type definitions.The types are properly documented with clear JSDoc comments describing parameters and defaults. Follows consistent patterns with other query hooks.
packages/shared/src/react/stable-keys.ts (1)
54-69: LGTM! Clear separation of internal and public stable keys.The new internal keys are well-documented and properly scoped with the
__internal_prefix. The comment clearly explains their purpose.packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
6-22: LGTM! Proper cache key generation for plan details.The hook correctly handles nullable planId and generates appropriate cache keys with proper memoization.
packages/shared/src/react/hooks/__tests__/wrapper.tsx (1)
5-5: TODO comment noted for test configuration.The comment clearly marks the test wrapper for future migration to React Query. No functional changes.
packages/shared/src/react/hooks/usePaymentAttemptQuery.tsx (1)
1-1: LGTM! Clean re-export pattern.The virtual module export provides flexibility for switching between SWR and React Query implementations at build time.
packages/shared/src/react/hooks/useStatementQuery.tsx (1)
1-1: LGTM! Consistent with the re-export pattern.packages/shared/src/react/hooks/usePlanDetailsQuery.tsx (1)
1-1: LGTM! Maintains consistency with other virtual hook re-exports.packages/shared/src/react/index.ts (1)
3-4: LGTM! Type export extends the public API appropriately.Adding the
UseSubscriptionParamstype export is a non-breaking change that makes this type available for external consumers.packages/shared/src/react/hooks/usePagesOrInfinite.swr.tsx (3)
7-7: LGTM! Import addition for cache key refactoring.
52-56: LGTM! Cleaner cache key construction.Using
toSWRQuery(keys)centralizes the cache key generation logic and reduces duplication.
137-141: LGTM! Consistent refactoring with pagesCacheKey.packages/shared/tsconfig.json (1)
28-31: LGTM! Path mappings enable virtual module resolution.The new mappings for statement, payment attempt, and plan details queries are correctly configured and follow the existing pattern.
packages/clerk-js/src/ui/components/Plans/PlanDetails.tsx (4)
1-1: LGTM! Import updated to use internal hook.
31-32: LGTM! Type alias improves clarity.The
PlanFeaturetype alias enhances readability and type safety for the features array.
89-93: LGTM! Clean refactoring to internal hook.The component now uses the abstracted query hook, which supports both SWR and React Query implementations. The enabled flag logic correctly guards against fetching when there's no plan identifier.
158-158: LGTM! Type annotation improves type safety.packages/shared/src/react/hooks/index.ts (1)
18-20: LGTM! Consistent internal API exports.The new exports follow the established
__internal_naming convention, clearly signaling these hooks are not part of the public API surface.packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
8-33: LGTM! Clean cache key generation with proper memoization.The hook correctly memoizes cache key generation and handles the null-to-undefined conversion for API arguments.
packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx (1)
1-1: LGTM! Clean migration to internal query hook.The component successfully migrates from direct API calls to the new
__internal_usePaymentAttemptQueryhook while maintaining the same external interface and behavior.Also applies to: 33-42
packages/shared/src/react/hooks/useSubscription.rq.tsx (1)
51-64: LGTM! Consistent migration to shared cache key generation.The hook now uses
useSubscriptionCacheKeysfor cache key generation, and correctly extracts args fromqueryKey[3]to match the new structure fromcreateCacheKeys.packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
64-66: Good backwards compatibility consideration.The inline comment clearly explains the error normalization from
nulltoundefined, ensuring compatibility with existing SWR-based types.packages/clerk-js/src/ui/components/Statements/StatementPage.tsx (3)
1-2: LGTM - Clean migration to internal hook.The import changes correctly reference the new internal hook and shared type definitions.
23-24: LGTM - Good use of type extraction.The local type aliases for
StatementGroupandStatementItemimprove readability and type safety in the map callbacks below.
31-40: LGTM - Correct hook integration.The
requesterTypederivation and hook parameters are properly configured. The hook correctly handles theforparameter to fetch statement data in the appropriate context.packages/shared/src/react/hooks/useStatementQuery.swr.tsx (1)
1-62: LGTM - Clean SWR implementation.The SWR-based implementation correctly:
- Validates Clerk context and records telemetry
- Derives organizationId based on the
forparameter- Builds consistent cache keys
- Implements proper query enablement logic (requires statementId and org context when scope is 'organization')
- Maps SWR response fields to the expected result shape
packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (1)
1-50: LGTM - Correct SWR implementation with fallback data.The implementation properly:
- Derives
targetPlanIdfrom eitherplanIdorinitialPlan.id- Uses
fallbackDatato provide initial plan data before fetching- Applies
keepPreviousDatato maintain UI stability during refetches- Returns the standard query result shape
packages/shared/src/react/hooks/createCacheKeys.ts (3)
1-2: LGTM - Necessary imports for internal key support.The new imports support the widened type signature and enable conversion between query key formats.
12-12: LGTM - Type widening supports internal hooks.Widening the
stablePrefixparameter to accept__internal_ResourceCacheStableKeyenables the new internal billing hooks to use the shared cache key infrastructure.
25-32: LGTM - Clean query key transformation utility.The
toSWRQueryhelper correctly flattens the structured query key into a plain object by spreading the tracked parameters (queryKey[2]) and args (queryKey[3].args). This maintains consistency between SWR and React Query key formats.packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
22-60: LGTM - Well-structured React Query implementation.The hook correctly:
- Uses both
initialData(for cache seeding) andplaceholderData(for showing stale data during refetch)- Disables
refetchOnWindowFocusandretrywhich are appropriate for relatively static plan details- Normalizes React Query's null error to undefined for SWR compatibility
- Applies a 60-second stale time matching the SWR implementation
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (1)
27-68: LGTM - Solid React Query implementation.The hook properly:
- Extracts args from the query key for flexibility
- Applies
placeholderDatawhenkeepPreviousDatais enabled- Normalizes the error response for backward compatibility
- Uses appropriate staleTime (60s) matching the SWR version
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/shared/src/react/hooks/useSubscription.shared.ts (1)
7-28: Consider includingforTypein tracked cache-key fields for consistencyRight now the tracked keys are just
{ userId, orgId: safeOrgId }. This is functionally fine, but bothuseStatementQueryCacheKeysandusePaymentAttemptQueryCacheKeysalso trackforType. AddingforTypehere would make the key shape more self-descriptive and consistent across billing helpers, and avoid any future surprises if logic ever depends on distinguishing user vs organization scenarios purely from the key.packages/shared/src/react/hooks/useStatementQuery.swr.tsx (1)
20-62: LGTM! SWR implementation is correct and consistent.The hook correctly validates context, derives IDs based on scope, generates cache keys, and conditionally enables the query. The return shape matches the expected
StatementQueryResulttype.The
statementIdvalidation at lines 45-47 is redundant sincequeryEnabled(line 40) already ensuresstatementIdis truthy before SWR calls the fetcher. You may simplify:const swr = useSWR( queryEnabled ? queryKey : null, () => { - if (!statementId) { - throw new Error('statementId is required to fetch a statement'); - } - return clerk.billing.getStatement({ id: statementId, orgId: organizationId ?? undefined }); + return clerk.billing.getStatement({ id: statementId!, orgId: organizationId ?? undefined }); }, { dedupingInterval: 1_000 * 60, keepPreviousData, }, );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (9)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.shared.ts(1 hunks)packages/shared/src/react/hooks/useStatementQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/useSubscription.shared.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/shared/src/react/hooks/useStatementQuery.shared.ts (3)
packages/shared/src/types/billing.ts (1)
ForPayerType(69-69)packages/shared/src/react/hooks/createCacheKeys.ts (1)
createCacheKeys(7-23)packages/shared/src/react/stable-keys.ts (1)
INTERNAL_STABLE_KEYS(63-67)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (5)
packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (1)
__internal_usePaymentAttemptQuery(20-60)packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (2)
UsePaymentAttemptQueryParams(7-30)PaymentAttemptQueryResult(35-52)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
usePaymentAttemptQueryCacheKeys(7-32)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)
packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (4)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (1)
__internal_usePaymentAttemptQuery(27-66)packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (2)
UsePaymentAttemptQueryParams(7-30)PaymentAttemptQueryResult(35-52)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
usePaymentAttemptQueryCacheKeys(7-32)
packages/shared/src/react/hooks/useSubscription.shared.ts (3)
packages/shared/src/types/billing.ts (1)
ForPayerType(69-69)packages/shared/src/react/hooks/createCacheKeys.ts (1)
createCacheKeys(7-23)packages/shared/src/react/stable-keys.ts (1)
STABLE_KEYS(30-51)
packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (3)
packages/shared/src/types/billing.ts (1)
ForPayerType(69-69)packages/shared/src/react/hooks/createCacheKeys.ts (1)
createCacheKeys(7-23)packages/shared/src/react/stable-keys.ts (1)
INTERNAL_STABLE_KEYS(63-67)
packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (5)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
__internal_usePlanDetailsQuery(22-58)packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (2)
UsePlanDetailsQueryParams(7-28)PlanDetailsQueryResult(33-50)packages/shared/src/react/index.ts (2)
useAssertWrappedByClerkProvider(11-11)useClerkInstanceContext(12-12)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
usePlanDetailsQueryCacheKeys(6-22)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (5)
packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (1)
__internal_usePlanDetailsQuery(15-50)packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (2)
UsePlanDetailsQueryParams(7-28)PlanDetailsQueryResult(33-50)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
usePlanDetailsQueryCacheKeys(6-22)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (5)
packages/shared/src/react/hooks/useStatementQuery.swr.tsx (1)
__internal_useStatementQuery(20-62)packages/shared/src/react/hooks/useStatementQuery.types.ts (2)
UseStatementQueryParams(6-29)StatementQueryResult(34-51)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
useStatementQueryCacheKeys(7-32)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)
packages/shared/src/react/hooks/useStatementQuery.swr.tsx (4)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
__internal_useStatementQuery(27-68)packages/shared/src/react/hooks/useStatementQuery.types.ts (2)
UseStatementQueryParams(6-29)StatementQueryResult(34-51)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
useStatementQueryCacheKeys(7-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (1)
20-59: SWR hook logic is consistent with the RQ variant and looks correctThe context checks, telemetry, cache-key construction, enabled gating, and result mapping align with the React Query implementation and other billing hooks; no issues stand out here.
packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
7-31: Statement cache-key helper is well-structured and consistentTracked vs untracked fields, inclusion of
forType, anduseMemodependencies all look correct and consistent with the payment-attempt cache-key helper; no changes needed.packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
27-67: React Query statement hook cleanly mirrors the existing SWR behaviorParameter defaults, context handling, cache-key usage, enabled logic, and data/error/loading mapping all look consistent with the SWR variant and the shared cache-key helper; this implementation is solid.
packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
7-31: Payment-attempt cache-key helper matches the shared patternsThe tracked/untracked structure, use of
INTERNAL_STABLE_KEYS.PAYMENT_ATTEMPT_KEY, and memoization dependencies are all consistent with the statement helper and the consuming hooks; no changes required.packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (3)
17-19: Verify thatKeepPreviousDataFnhandles undefined correctly.React Query may pass
undefinedaspreviousDataon the initial render or when no previous data exists. The current signature<Data>(previousData: Data): Datadoesn't handle this case, which could lead to type mismatches or unexpected behavior.Consider updating the signature to handle undefined:
-function KeepPreviousDataFn<Data>(previousData: Data): Data { - return previousData; +function KeepPreviousDataFn<Data>(previousData: Data | undefined): Data | undefined { + return previousData; }Alternatively, verify whether React Query only invokes this function when
previousDatais defined, or check if the type system is loose enough to accept the current signature without issues.
27-66: React Query implementation looks correct overall.The hook properly validates context, derives scope-based IDs, constructs the cache key, conditionally enables the query, and returns the expected
PaymentAttemptQueryResultshape. The logic mirrors the SWR version appropriately.Ensure the verification items flagged in previous comments are addressed:
- Confirm
KeepPreviousDataFnhandles undefined correctly- Validate the
queryKey[3].argsindexing matches the cache key structure
51-53: Cache key structure indexing is correct and verified.The code at lines 51-53 correctly accesses
queryKey[3].args. ThecreateCacheKeysfunction structures the query key as[stablePrefix, authenticated, tracked, untracked], placing theuntrackedobject at index 3. TheusePaymentAttemptQueryCacheKeysimplementation confirms thatuntrackedcontains anargsproperty with the expected structure, matching the established codebase pattern.packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (1)
15-49: SWR hook implementation looks correct and aligns with the intended behaviorThe SWR version of
__internal_usePlanDetailsQuerymirrors the expected behavior: provider assertion, telemetry, cache‑key usage,targetPlanIdderivation, guarded fetch, and the{ data, error, isLoading, isFetching }mapping all look consistent. The options (dedupingInterval,keepPreviousData,fallbackData) match the documented intent ofUsePlanDetailsQueryParamsand provide a good baseline for the React Query port to match.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
19-44: React Query config for__internal_usePlanDetailsQuerystill diverges from SWRinitialPlan/keepPreviousDatasemanticsYou’re still using:
initialData: initialPlan ?? undefinedtogether withstaleTime: 1_000 * 60In React Query this means
initialPlanis written to the cache and treated as fresh for 60 seconds, so the query will not refetch on mount during that window even whenenabledistrue. In the SWR version,initialPlanwas described and used as data “to use before fetching”, with a real network call still happening in the background. That’s a behavioral regression: callers may never see a fresh plan for up to a minute, andisFetchingwill stayfalse.Given the types/JSDoc for
initialPlan, a closer mapping to SWR and to the “before fetching” wording would be something like:const query = useClerkQuery({ queryKey, queryFn: () => { if (!targetPlanId) { throw new Error('planId is required to fetch plan details'); } return clerk.billing.getPlan({ id: targetPlanId }); }, - enabled: queryEnabled, - initialData: initialPlan ?? undefined, - placeholderData: defineKeepPreviousDataFn(keepPreviousData), + enabled: queryEnabled, + // Show initialPlan as a non-cached placeholder while still fetching. + placeholderData: initialPlan ?? undefined, + // Let React Query handle previous-data semantics across key changes. + keepPreviousData, refetchOnWindowFocus: false, retry: false, - staleTime: 1_000 * 60, + // Consider relying on the default staleTime, or document any custom value. });This preserves the “initial before fetch” behavior and makes
keepPreviousDatasemantics more in line with React Query’s native behavior and the prior SWR implementation.
🧹 Nitpick comments (3)
packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
1-11: Helper implementation is solid; optional explicit return type for clarityThe logic is straightforward and type-safe. If you want to make the intent even clearer to future maintainers, you could add an explicit return type:
export function defineKeepPreviousDataFn( enabled: boolean, ): (<Data>(previousData: Data) => Data) | undefined { ... }Purely optional, since TS already infers this correctly.
packages/shared/src/react/hooks/useSubscription.rq.tsx (1)
37-62: Verify org-scoped gating andkeepPreviousDatabehavior vs the SWR hookThe RQ wiring looks consistent (cache keys, contexts, telemetry, enabled flag), and the new
defineKeepPreviousDataFnintegration is clean. Two things worth double‑checking against the SWR implementation and intended semantics:
Org-scoped subscriptions without an organization
queriesEnabledonly checksuser?.idandbillingEnabled. Whenparams?.for === 'organization'but there is noorganization?.id,getSubscriptionwill still be called with{ orgId: undefined }. If the SWR version previously short-circuited in that case, this may change behavior.
keepPreviousDataonly when the query is enabled
PassingdefineKeepPreviousDataFn(keepPreviousData && queriesEnabled)means previous data will not be preserved once the query becomes disabled. If callers relied on seeing the last subscription data while togglingenabled, you might wantkeepPreviousDatato stand on its own.Consider aligning these two points with however the SWR version behaved, or documenting the deliberate differences.
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
24-61: CheckkeepPreviousDatasemantics against React Query’s native option and the SWR versionThe hook’s context checks, cache-key usage, and
enabledgating (including org-only whenorganizationIdexists) all look good. One subtle point:
keepPreviousDatais implemented viaplaceholderData: defineKeepPreviousDataFn(keepPreviousData), which reuses previous data for the same query key. React Query’s built‑inkeepPreviousDataalso preserves data across key changes (e.g., ifstatementIdchanges), which is closer to typical “keep previous data while fetching the new one” semantics.If callers expect SWR‑style behavior when
statementIdchanges, consider switching to React Query’s nativekeepPreviousDataoption (and usingplaceholderDataonly for true placeholders), or confirm that the current behavior is intentional.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx(2 hunks)packages/clerk-js/src/ui/components/Plans/PlanDetails.tsx(2 hunks)packages/clerk-js/src/ui/components/Statements/StatementPage.tsx(2 hunks)packages/shared/src/react/clerk-rq/keep-previous-data.ts(1 hunks)packages/shared/src/react/hooks/usePageOrInfinite.types.ts(2 hunks)packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx(2 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/useSubscription.rq.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/clerk-js/src/ui/components/Statements/StatementPage.tsx
- packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx
- packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx
- packages/clerk-js/src/ui/components/Plans/PlanDetails.tsx
- packages/clerk-js/src/ui/components/PaymentAttempts/PaymentAttemptPage.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx (1)
packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (5)
packages/shared/src/react/hooks/useStatementQuery.types.ts (2)
UseStatementQueryParams(6-29)StatementQueryResult(34-51)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
useStatementQueryCacheKeys(7-32)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (6)
packages/shared/src/types/virtual-data-hooks.d.ts (1)
__internal_usePlanDetailsQuery(7-7)packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (2)
UsePlanDetailsQueryParams(7-28)PlanDetailsQueryResult(33-50)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
usePlanDetailsQueryCacheKeys(6-22)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/useSubscription.rq.tsx (2)
packages/shared/src/react/hooks/useSubscription.shared.ts (1)
useSubscriptionCacheKeys(7-29)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (3)
packages/shared/src/react/hooks/usePageOrInfinite.types.ts (2)
13-19: LGTM! Good practice to export shared type definitions.Exporting
QueryKeyWithArgsenables type reuse across modules and improves type safety throughout the codebase.
30-30: All call sites properly provide theauthenticatedproperty.Verification confirms that all implementations of
usePagesOrInfiniteare already passing the requiredauthenticatedproperty viacreateCacheKeys. The four main hook implementations—useAPIKeys,createBillingPaginatedHook,useOrganization, anduseOrganizationList—all provide this property correctly, and thecreateCacheKeysfunction properly constructs it into the returned keys object. No breaking changes are present.packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx (1)
6-6: Centralized keep-previous-data helper integration looks correctSwitching to
defineKeepPreviousDataFn(keepPreviousData)keeps the existing behavior while deduplicating the inline helper; key and query logic remain unchanged.Also applies to: 74-75
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/clerk-js/src/ui/components/APIKeys/APIKeys.tsx (2)
105-125: Consider clearing stale API key state on error.When API key creation fails, the previous successful
apiKeystate remains populated. If the user then opens the copy modal (via some other path or if the modal opening logic has a race condition), they might see stale data.Apply this diff to clear the state on error:
} catch (err: any) { + setAPIKey(null); if (isClerkAPIResponseError(err)) { if (err.status === 409) { card.setError('API Key name already exists'); } }
194-195: Consider defensive null check for modal data.While the current flow prevents opening the modal when
apiKeyis null, the fallback to empty strings could display a confusing empty modal if the state becomes desynchronized due to future changes.Consider this alternative that only opens the modal when data is available:
-<CopyAPIKeyModal - isOpen={isCopyModalOpen} +<CopyAPIKeyModal + isOpen={isCopyModalOpen && apiKey !== null} onOpen={() => setIsCopyModalOpen(true)} onClose={() => setIsCopyModalOpen(false)} apiKeyName={apiKey?.name || ''} apiKeySecret={apiKey?.secret || ''} modalRoot={revokeModalRoot} />
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
packages/clerk-js/src/ui/components/APIKeys/APIKeys.tsx(5 hunks)packages/clerk-js/src/ui/components/APIKeys/CreateAPIKeyForm.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
packages/clerk-js/src/ui/components/APIKeys/CreateAPIKeyForm.tsx (1)
packages/clerk-js/src/ui/elements/contexts/index.tsx (1)
useCardState(42-70)
packages/clerk-js/src/ui/components/APIKeys/APIKeys.tsx (2)
packages/shared/src/types/apiKeys.ts (1)
APIKeyResource(5-22)packages/clerk-js/src/ui/components/APIKeys/CreateAPIKeyForm.tsx (1)
CreateAPIKeyForm(120-261)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (7)
packages/clerk-js/src/ui/components/APIKeys/APIKeys.tsx (2)
3-3: LGTM!The
APIKeyResourcetype import is correctly added to support the new local state typing.
185-185: LGTM!The removal of the
isSubmittingprop aligns with the refactor whereCreateAPIKeyFormnow manages loading state internally viauseCardState().packages/clerk-js/src/ui/components/APIKeys/CreateAPIKeyForm.tsx (5)
6-6: LGTM!The
useCardStateimport is correctly added to support internal loading state management.
120-120: LGTM!The component signature correctly reflects the updated
CreateAPIKeyFormPropsinterface.
124-124: LGTM!The
useCardState()hook correctly accesses the shared card state from the context provider, allowing the form to reflect loading states managed by the parent component.
255-255: LGTM!The switch from
isSubmittingprop tocard.isLoadingcorrectly reflects the new loading state management approach, ensuring the button is disabled during API key creation.
30-32: No issues found - all usages updated correctly.The comprehensive search confirms that
CreateAPIKeyFormis used in only one location (APIKeys.tsx:185), and that usage correctly passes only theonCreateprop with noisSubmittingparameter. The breaking change has been properly handled throughout the codebase.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
packages/shared/vitest.config.mts (1)
24-36: Billing path inclusion in candidate resolution looks correct; consider stricter failure behaviorThe new
billingcandidate path is wired consistently withhooksandproviders, and the ordering (hooks→billing→providersfor each base dir) keeps resolution deterministic. This should integrate cleanly with the new billing hooks.One optional improvement: if none of the candidates exist, returning
candidates[0]will surface as a “module not found” on a non-existent path, which can be harder to debug. You could instead throw or log an explicit error when!candidates.some(fs.existsSync), making misconfigured aliases easier to diagnose.packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx (1)
44-52: Consider logging errors from auto-initialization.The auto-initialization effect silently catches and ignores all errors. While this maintains backwards compatibility, it could hide legitimate issues like network failures, authentication errors, or validation problems during development. Consider at minimum logging these errors to the console in development mode.
Apply this diff to add development-time error logging:
useEffect(() => { if (!resource?.id) { return; } trigger().catch(() => { - // ignore errors + if (process.env.NODE_ENV === 'development') { + console.warn('[Clerk] Payment method auto-initialization failed:', error); + } }); }, [resource?.id, trigger]);packages/shared/src/react/__tests__/payment-element.test.tsx (1)
5-5: Mocks are aligned with the new billing hooks and Stripe loaderThe updated import and mocks for
__experimental_PaymentElement,__internal_useInitializePaymentMethod,__internal_useStripeClerkLibs, and__internal_useStripeLoadermatch the new hook contracts and keep the test focused on locale behavior without introducing asynchronous flakiness.To keep fixtures DRY, consider extracting the common
initializedPaymentMethodpayload into a shared constant used by both theuseUseranduseInitializePaymentMethodmocks:+const initializedPaymentMethodFixture = { + externalGatewayId: 'acct_123', + externalClientSecret: 'seti_123', + paymentMethodOrder: ['card'], +}; + vi.mock('../hooks/useUser', () => ({ useUser: () => ({ user: { id: 'user_123', - initializePaymentMethod: vi.fn().mockResolvedValue({ - externalGatewayId: 'acct_123', - externalClientSecret: 'seti_123', - paymentMethodOrder: ['card'], - }), + initializePaymentMethod: vi.fn().mockResolvedValue(initializedPaymentMethodFixture), }, }), })); -const mockInitializePaymentMethod = vi.fn().mockResolvedValue({ - externalGatewayId: 'acct_123', - externalClientSecret: 'seti_123', - paymentMethodOrder: ['card'], -}); +const mockInitializePaymentMethod = vi.fn().mockResolvedValue(initializedPaymentMethodFixture); vi.mock('../hooks/useInitializePaymentMethod', () => ({ __internal_useInitializePaymentMethod: vi.fn(() => ({ - initializedPaymentMethod: { - externalGatewayId: 'acct_123', - externalClientSecret: 'seti_123', - paymentMethodOrder: ['card'], - }, + initializedPaymentMethod: initializedPaymentMethodFixture, initializePaymentMethod: mockInitializePaymentMethod, })), }));Also applies to: 47-52, 62-77, 79-91
packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx (1)
1-76: Hook behavior is correct; consider centralizing the initialization logicThe hook correctly:
- Chooses user vs organization from context based on
for.- Gates the query on
resource?.id.- Builds a stable query key and keeps previous data.
- Exposes an imperative initializer that also updates the cache via the same key.
To avoid drift between the
queryFnand the imperativeinitializePaymentMethod, you can centralize the shared initialization logic:function useInitializePaymentMethod(options?: InitializePaymentMethodOptions): UseInitializePaymentMethodResult { const { for: forType = 'user' } = options ?? {}; const { organization } = useOrganizationContext(); const user = useUserContext(); const resource = forType === 'organization' ? organization : user; const queryKey = useMemo(() => { return ['billing-payment-method-initialize', { resourceId: resource?.id, for: forType }] as const; }, [resource?.id, forType]); const isEnabled = Boolean(resource?.id); - const query = useClerkQuery({ - queryKey, - queryFn: async () => { - if (!resource) { - return undefined; - } - - return resource.initializePaymentMethod({ - gateway: 'stripe', - }); - }, - enabled: isEnabled, - staleTime: 1_000 * 60, - refetchOnWindowFocus: false, - placeholderData: defineKeepPreviousDataFn(true), - }); - const [queryClient] = useClerkQueryClient(); - const initializePaymentMethod = useCallback(async () => { - if (!resource) { - return undefined; - } - - const result = await resource.initializePaymentMethod({ - gateway: 'stripe', - }); - - queryClient.setQueryData(queryKey, result); - - return result; - }, [queryClient, queryKey, resource]); + const runInitialize = useCallback(async () => { + if (!resource) { + return undefined; + } + + return resource.initializePaymentMethod({ + gateway: 'stripe', + }); + }, [resource]); + + const query = useClerkQuery({ + queryKey, + queryFn: runInitialize, + enabled: isEnabled, + staleTime: 1_000 * 60, + refetchOnWindowFocus: false, + placeholderData: defineKeepPreviousDataFn(true), + }); + + const initializePaymentMethod = useCallback(async () => { + const result = await runInitialize(); + + if (result) { + queryClient.setQueryData(queryKey, result); + } + + return result; + }, [queryClient, queryKey, runInitialize]); return { initializedPaymentMethod: query.data ?? undefined, initializePaymentMethod, }; }packages/shared/src/react/billing/payment-element.tsx (3)
51-64: Consider more defensive optional chaining when reading commerce settings
environment?.commerceSettings.billing.stripePublishableKeyonly guardsenvironment. IfcommerceSettingsorbillingcan ever beundefinedonEnvironmentResource, this will throw at runtime when those fields are missing. Consider guarding each hop:- const stripePublishableKey = environment?.commerceSettings.billing.stripePublishableKey ?? undefined; + const stripePublishableKey = + environment?.commerceSettings?.billing?.stripePublishableKey ?? undefined;This keeps the loader input safe if billing settings are not present.
128-142: Avoidas anyinDummyStripeUtilsfor clearer types
DummyStripeUtilscurrently providesvalue={{ value: {} as any }}, which hides type mismatches for consumers ofuseStripeUtilsContext. You can preserve type safety and avoidanyby returning an explicit, typed empty state:-const DummyStripeUtils = ({ children }: PropsWithChildren) => { - return <StripeUtilsContext.Provider value={{ value: {} as any }}>{children}</StripeUtilsContext.Provider>; -}; +const DummyStripeUtils = ({ children }: PropsWithChildren) => { + return ( + <StripeUtilsContext.Provider value={{ value: { stripe: null, elements: null } }}> + {children} + </StripeUtilsContext.Provider> + ); +};This keeps the context shape consistent without sacrificing typing.
319-383: Avoid duplicateusePaymentElementContextcalls and tighten readiness handling
usePaymentElementcallsusePaymentElementContext()twice (Lines 320 and 322) to pull out different fields. You can simplify and avoid redundant work:-const usePaymentElement = (): UsePaymentElementReturn => { - const { isPaymentElementReady, initializePaymentMethod } = usePaymentElementContext(); - const { stripe, elements } = useStripeUtilsContext(); - const { externalClientSecret } = usePaymentElementContext(); +const usePaymentElement = (): UsePaymentElementReturn => { + const { isPaymentElementReady, initializePaymentMethod, externalClientSecret } = + usePaymentElementContext(); + const { stripe, elements } = useStripeUtilsContext();Behavior stays the same but the hook is a bit clearer and avoids accidental divergence between multiple context reads.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
packages/shared/src/react/__tests__/payment-element.test.tsx(3 hunks)packages/shared/src/react/billing/payment-element.tsx(3 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx(1 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx(1 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.swr.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.tsx(1 hunks)packages/shared/src/react/hooks/index.ts(1 hunks)packages/shared/src/react/hooks/useCheckout.ts(2 hunks)packages/shared/src/react/index.ts(2 hunks)packages/shared/tsconfig.json(1 hunks)packages/shared/tsdown.config.mts(1 hunks)packages/shared/vitest.config.mts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (8)
packages/shared/src/react/hooks/useCheckout.ts (2)
packages/shared/src/react/index.ts (1)
useOrganizationContext(15-15)packages/shared/src/react/contexts.tsx (1)
useOrganizationContext(131-131)
packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx (2)
packages/shared/src/types/billing.ts (2)
ForPayerType(69-69)BillingInitializedPaymentMethodResource(349-362)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx (2)
UseInitializePaymentMethodResult(13-16)useInitializePaymentMethod(76-76)
packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx (5)
packages/shared/src/types/billing.ts (2)
ForPayerType(69-69)BillingInitializedPaymentMethodResource(349-362)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)packages/shared/src/react/clerk-rq/use-clerk-query-client.ts (1)
useClerkQueryClient(84-84)packages/clerk-js/src/core/modules/billing/payment-source-methods.ts (1)
initializePaymentMethod(20-30)
packages/shared/src/react/billing/useStripeLoader.swr.tsx (2)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (1)
UseStripeClerkLibsResult(13-13)packages/shared/src/react/billing/useStripeLoader.rq.tsx (2)
UseStripeLoaderResult(14-14)useStripeLoader(51-51)
packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (2)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (2)
UseStripeClerkLibsResult(13-13)useStripeClerkLibs(38-38)packages/shared/src/react/hooks/index.ts (1)
useClerk(9-9)
packages/shared/src/react/billing/useStripeLoader.rq.tsx (4)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (1)
UseStripeClerkLibsResult(13-13)packages/shared/src/react/billing/useStripeLoader.swr.tsx (2)
UseStripeLoaderResult(12-12)useStripeLoader(46-46)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/billing/payment-element.tsx (6)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (1)
useStripeClerkLibs(38-38)packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (1)
useStripeClerkLibs(39-39)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx (2)
UseInitializePaymentMethodResult(13-16)useInitializePaymentMethod(76-76)packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx (2)
UseInitializePaymentMethodResult(11-14)useInitializePaymentMethod(60-60)packages/shared/src/react/billing/useStripeLoader.rq.tsx (2)
UseStripeLoaderResult(14-14)useStripeLoader(51-51)packages/shared/src/react/billing/useStripeLoader.swr.tsx (2)
UseStripeLoaderResult(12-12)useStripeLoader(46-46)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (3)
packages/shared/src/react/hooks/index.ts (1)
useClerk(9-9)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (17)
packages/shared/src/react/hooks/useCheckout.ts (1)
11-11: Confirm behavioral parity when switching fromuseOrganizationtouseOrganizationContextinuseCheckout.This looks like a mechanical change and should be equivalent if
useOrganizationwas just a thin wrapper over the same context. The only thing to double‑check is that everyuseCheckoutusage sits under whatever provider backsuseOrganizationContext; otherwise this refactor could introduce a new "missing provider" failure path even for non‑organization flows. Please confirm that this assumption holds (or thatuseOrganizationContext’s contract matches the previous hook) across existing call sites.Also applies to: 125-125
packages/shared/src/react/hooks/index.ts (1)
18-27: Internal query hook exports and docblock look consistent with intended usage.The new
__internal_*exports and the accompanying comment clearly mark these hooks as internal while keeping the existing public exports unchanged. This seems like a straightforward, low‑risk way to centralize the RQ/SWR implementations.packages/shared/tsconfig.json (1)
29-34: All referenced hook implementation files exist and are properly configured.Verification confirms that all six
.swr.tsxfiles referenced in the tsconfig.json path mappings exist, along with their corresponding.rq.tsxvariants. The setup correctly supports the feature flag-based dynamic resolution between SWR and React Query implementations.packages/shared/tsdown.config.mts (1)
67-71: LGTM! Cleaner approach to candidate path resolution.Verification confirms no duplicate hook implementations exist across the three candidate directories—each implementation is isolated to a single location, so the search order is deterministic and won't cause unexpected resolution conflicts.
packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx (3)
1-14: LGTM! Type definitions align with the React Query version.The imports and type definitions are clean and consistent with the React Query implementation, ensuring API compatibility across both implementations.
37-40: Verify the hardcoded 'stripe' gateway is the only supported provider.The
gatewayparameter is hardcoded to'stripe'. Ensure this aligns with the product requirements and that no other payment gateways need to be supported in this backwards-compatible implementation.
54-60: LGTM! Export pattern aligns with internal hook conventions.The return values correctly match the type signature, and the
__internal_prefix export follows the established pattern for internal hooks exposed behind feature flags.packages/shared/src/react/billing/useInitializePaymentMethod.tsx (1)
1-2: Re-exports correctly mirror the virtual data-hook surfaceThe type and internal hook alias re-exports look consistent with the underlying implementation and keep the indirection through
virtual:data-hookswithout adding runtime logic.packages/shared/src/react/billing/useStripeLoader.tsx (1)
1-2: Stripe loader virtual re-exports look consistentThe type and internal hook alias are correctly re-exported from the virtual
useStripeLoadermodule, matching the pattern used for other internal data hooks.packages/shared/src/react/index.ts (1)
3-4: Confirm this wildcard export replacement does not break external consumersThe change is confirmed:
export * from './commerce'was replaced withexport * from './billing/payment-element'. Since the old./commercefile no longer exists and cannot be inspected, you must verify that:
- The old
./commerceexport is no longer part of your public API contract, or- Any consumers previously relying on
./commerceexports now use./billing/payment-elementinsteadNo internal codebase imports from
./commerceremain, but external packages or applications may depend on it. Ensure this breaking change is intentional and documented if it affects published versions.packages/shared/src/react/billing/useStripeClerkLibs.tsx (1)
1-2: Re-export wiring for Stripe Clerk libs looks correctThese re-exports cleanly route both the result type and internal hook alias through the virtual data-hooks layer without adding runtime behavior. No issues from this file alone.
packages/shared/src/react/billing/payment-element.tsx (2)
163-196: Provider/root composition for Stripe<Elements>looks coherent
PaymentElementProvider→PropsProvider→PaymentElementInternalRootcleanly separate concerns: loading/init, context wiring, and conditional mounting of<Elements>with the correctclientSecret,appearance, andlocale. Thekey={externalClientSecret}strategy for resetting the intent is consistent with Stripe Elements usage.
208-250: PaymentElement fallback and Apple Pay options wiring look soundThe component correctly short-circuits to the
fallbackuntil bothstripeandexternalClientSecretare available, and the Apple PayrecurringPaymentRequestis derived fromcheckout,planPeriod, andenvironmentin a memoized way. Dependency arrays on bothapplePayandoptionscover all referenced values.packages/shared/src/react/billing/useStripeLoader.swr.tsx (1)
6-44: SWR-based Stripe loader matches the new options contractThe SWR implementation correctly:
- Accepts the same
StripeLoaderOptionsshape as the RQ variant.- Only issues a request when
stripeClerkLibs,externalGatewayId, andstripePublishableKeyare all present (key is otherwisenull).- Delegates to
stripeClerkLibs?.loadStripewith the expected{ stripeAccount: externalGatewayId }options.This should behave compatibly with the new React Query loader for existing SWR consumers.
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (1)
7-36: React Query loader for Stripe libs is consistent with the SWR versionThe RQ-based
useStripeClerkLibs:
- Normalizes the result into
{ loadStripe } | null, matchingUseStripeClerkLibsResult.- Uses a stable query key
'clerk-stripe-sdk'withstaleTime: InfinityandrefetchOnWindowFocus: false, which is appropriate for a long-lived SDK loader.- Applies
placeholderData: defineKeepPreviousDataFn(true)to preserve the previous loader object across key/option changes.This should provide equivalent behavior to the SWR implementation while fitting into the shared
useClerkQueryinfrastructure.packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (1)
6-37: SWR implementation of Stripe Clerk libs loader is straightforward and consistentThe SWR hook cleanly wraps
__internal_loadStripeJsinto a{ loadStripe } | nullresult, with:
- A single static key
'clerk-stripe-sdk'for the SDK resource.keepPreviousData: trueanddedupingInterval: Infinityto avoid unnecessary refetching.- Normalization to
UseStripeClerkLibsResultviaswr.data ?? null.This pairs well with the new RQ-based implementation and keeps the public surface uniform.
packages/shared/src/react/billing/useStripeLoader.rq.tsx (1)
8-49: React Query Stripe loader mirrors the SWR semantics with proper guardsThe RQ
useStripeLoader:
- Shares the same options shape and
UseStripeLoaderResulttype as the SWR variant.- Uses a memoized key
['stripe-sdk', { externalGatewayId, stripePublishableKey }]and anenabledflag so the query only runs when all required inputs andstripeClerkLibsare present.- Re-checks inputs in
queryFnand callsstripeClerkLibs.loadStripewith{ stripeAccount: externalGatewayId }.- Keeps the previous Stripe instance via
placeholderData: defineKeepPreviousDataFn(true)and a 60sstaleTime.This should behave as an RQ drop-in for the existing SWR loader while preserving caching and deduping expectations.
panteliselef
force-pushed
the
elef/remove-swr-from-main
branch
from
2826b6f to
a9359ad
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (2)
20-37: Consider exposing loading and error states.The hook returns only
swr.data ?? null, which means consumers cannot distinguish between loading, error, and successful-but-null states. While the SWR configuration (dedupingInterval: Infinity, keepPreviousData: true) suggests this is a one-time load operation, consumers might benefit from knowing when the libraries are still loading versus when loading has failed.Consider returning additional state:
function useStripeClerkLibs(): UseStripeClerkLibsResult { const clerk = useClerk(); const swr = useSWR( 'clerk-stripe-sdk', async () => { const loadStripe = (await clerk.__internal_loadStripeJs()) as LoadStripeFn; return { loadStripe }; }, { keepPreviousData: true, revalidateOnFocus: false, dedupingInterval: Infinity, }, ); - return swr.data ?? null; + return swr.data ?? null; + // Alternatively, expose loading/error states: + // return { data: swr.data ?? null, isLoading: swr.isLoading, error: swr.error }; }Note: This would require updating the return type
UseStripeClerkLibsResultand all consumers.
14-19: Clarify the backwards compatibility comment for an internal hook.The comment states this is kept "for backwards compatibility until our next major version," but the hook is marked
@internal. Internal APIs typically aren't subject to backwards compatibility guarantees. Consider clarifying whether:
- This is the legacy SWR implementation behind a feature flag (alongside a newer RQ implementation)
- The internal hook will be removed/changed in the next major version
- This impacts any internal consumers that should migrate
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
packages/shared/src/react/__tests__/payment-element.test.tsx(7 hunks)packages/shared/src/react/billing/payment-element.tsx(3 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx(1 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx(1 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.swr.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.tsx(1 hunks)packages/shared/src/react/hooks/index.ts(1 hunks)packages/shared/src/react/hooks/useCheckout.ts(2 hunks)packages/shared/src/react/index.ts(2 hunks)packages/shared/tsconfig.json(1 hunks)packages/shared/tsdown.config.mts(1 hunks)packages/shared/vitest.config.mts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (10)
- packages/shared/src/react/index.ts
- packages/shared/src/react/hooks/useCheckout.ts
- packages/shared/src/react/billing/useStripeClerkLibs.tsx
- packages/shared/vitest.config.mts
- packages/shared/tsconfig.json
- packages/shared/src/react/billing/useStripeLoader.tsx
- packages/shared/src/react/billing/useStripeLoader.swr.tsx
- packages/shared/src/react/billing/useStripeLoader.rq.tsx
- packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx
- packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (2)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (2)
UseStripeClerkLibsResult(13-13)useStripeClerkLibs(38-38)packages/shared/src/react/hooks/index.ts (1)
useClerk(9-9)
packages/shared/src/react/__tests__/payment-element.test.tsx (1)
packages/shared/src/react/index.ts (4)
ClerkInstanceContext(6-6)UserContext(16-16)OrganizationProvider(9-9)OptionsContext(8-8)
packages/shared/src/react/billing/payment-element.tsx (7)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (1)
useStripeClerkLibs(38-38)packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (1)
useStripeClerkLibs(39-39)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx (2)
UseInitializePaymentMethodResult(13-16)useInitializePaymentMethod(76-76)packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx (2)
UseInitializePaymentMethodResult(11-14)useInitializePaymentMethod(60-60)packages/shared/src/react/billing/useStripeLoader.rq.tsx (2)
UseStripeLoaderResult(14-14)useStripeLoader(51-51)packages/shared/src/react/billing/useStripeLoader.swr.tsx (2)
UseStripeLoaderResult(12-12)useStripeLoader(46-46)packages/shared/src/react/billing/useStripeLoader.tsx (1)
UseStripeLoaderResult(1-1)
packages/shared/src/react/billing/useInitializePaymentMethod.swr.tsx (2)
packages/shared/src/types/billing.ts (2)
ForPayerType(69-69)BillingInitializedPaymentMethodResource(349-362)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx (2)
UseInitializePaymentMethodResult(13-16)useInitializePaymentMethod(76-76)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Packages
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
packages/shared/tsdown.config.mts (1)
67-71: Original review comment concern is not applicable—no hooks exist in multiple directories.Verification shows that each hook file is unique across all candidate directories:
hooks/: 5 unique hooksbilling/: 3 unique hooksproviders/: 1 unique hookSince no hook exists in multiple directories, the fixed candidate order cannot create resolution conflicts. The simplified logic is safe.
packages/shared/src/react/hooks/index.ts (1)
18-27: LGTM - Clear internal API surface.The documentation clearly marks these hooks as internal to
@clerk/clerk-jswith explicit warnings that query keys can change without notice. The exports follow a consistent naming pattern with the__internal_prefix.packages/shared/src/react/billing/useInitializePaymentMethod.tsx (1)
1-2: LGTM - Standard virtual module re-export.The file correctly re-exports the type and internal hook from the virtual data-hooks module, following the established pattern for this architecture.
packages/shared/src/react/billing/payment-element.tsx (3)
163-169: LGTM - Simplified provider structure.Removing the
StripeLibsProviderwrapper aligns with the shift to per-component, hook-based initialization. This is a cleaner architecture that avoids global state management for Stripe libraries.
9-14: LGTM - Internal hook imports align with new architecture.The imports correctly use the
__internal_prefixed hooks and their corresponding types, consistent with the internal API surface established in this PR.
51-75: No issues found.The
useStripeLoaderhook properly handles undefined inputs in both implementations:
- React Query version: Uses
isEnabledflag that checks all required parameters exist before fetching, and thequeryFnincludes a defensive check returningnullif any parameter is missing.- SWR version: Sets the SWR key to
nullwhen any required parameter is missing, effectively disabling the fetch.Both safely return
undefinedwhen parameters are unavailable, so callinguseStripeLoaderwith potentially undefinedexternalGatewayIdandstripePublishableKey(lines 62-63 of payment-element.tsx) is safe and handled correctly.packages/shared/src/react/__tests__/payment-element.test.tsx (2)
62-91: LGTM - Comprehensive mock setup for new hooks.The mock implementations properly cover all three new internal hooks:
__internal_useInitializePaymentMethodwith both the data and trigger function__internal_useStripeClerkLibswith the loadStripe mock__internal_useStripeLoaderwith the Stripe instance mockThis structure should effectively test the new hook-based initialization flow.
206-217: LGTM - Proper context provisioning for updated hooks.The test render wrapper correctly provides all necessary contexts (ClerkInstanceContext, UserContext, OrganizationProvider, OptionsContext) required by the new hook-based architecture. The context nesting order is appropriate and the mock data is comprehensive.
panteliselef
force-pushed
the
elef/remove-swr-from-main
branch
from
6f79bea to
42fda7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/shared/src/react/hooks/__tests__/wrapper.tsx (1)
2-3: Test wrapper’sswrConfigis now ignored in the RQ build; consider clarifying intentHere the test wrapper still passes:
<SWRConfigCompat swrConfig={{ provider: () => new Map(), }} > {children} </SWRConfigCompat>Given the new RQ implementation of
SWRConfigCompatis just a fragment, thisswrConfigonly has an effect in the SWR-backed build; in the RQ build it’s a no-op.That’s fine for a compat layer, but it’s a bit opaque for future readers and for tests that might subtly rely on SWR’s
providerbehavior.Two small suggestions:
- Add a brief comment here noting that
swrConfigis only honored in the SWR implementation and ignored in the RQ implementation; and/or- If you expect RQ tests to need their own isolated cache/query client, plan a follow-up RQ-specific test wrapper so this one remains clearly “SWR-only” in terms of semantics.
This isn’t a blocker, but clarifying the mode-dependent behavior will make the transition away from SWR easier to reason about.
Also applies to: 6-12
integration/tests/machine-auth/component.test.ts (1)
542-603: Consider adding cleanup.The test logic is solid and properly verifies invalidation works with search filters. However, unlike some other tests in this suite, this test doesn't revoke the created API key at the end, which could accumulate test data.
Consider adding cleanup similar to test 1:
await expect(table.locator('.cl-tableRow', { hasText: newApiKeyName })).toBeVisible(); + + // Clean up + const menuButton = table.locator('.cl-tableRow', { hasText: newApiKeyName }).locator('.cl-menuButton'); + await menuButton.click(); + const revokeButton = u.page.getByRole('menuitem', { name: 'Revoke key' }); + await revokeButton.click(); + await u.po.apiKeys.waitForRevokeModalOpened(); + await u.po.apiKeys.typeRevokeConfirmation('Revoke'); + await u.po.apiKeys.clickConfirmRevokeButton(); + await u.po.apiKeys.waitForRevokeModalClosed(); });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (3)
integration/tests/machine-auth/component.test.ts(1 hunks)packages/shared/src/react/hooks/__tests__/wrapper.tsx(1 hunks)packages/shared/src/react/providers/SWRConfigCompat.rq.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
packages/shared/src/react/hooks/__tests__/wrapper.tsx (3)
packages/shared/src/react/providers/SWRConfigCompat.rq.tsx (1)
SWRConfigCompat(6-8)packages/shared/src/types/virtual-data-hooks.d.ts (1)
SWRConfigCompat(3-3)packages/shared/src/react/providers/SWRConfigCompat.swr.tsx (1)
SWRConfigCompat(7-10)
integration/tests/machine-auth/component.test.ts (1)
integration/testUtils/index.ts (1)
createTestUtils(24-86)
packages/shared/src/react/providers/SWRConfigCompat.rq.tsx (2)
packages/shared/src/types/virtual-data-hooks.d.ts (1)
SWRConfigCompat(3-3)packages/shared/src/react/providers/SWRConfigCompat.swr.tsx (1)
SWRConfigCompat(7-10)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (32)
- GitHub Check: Integration Tests (machine, chrome, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 16)
- GitHub Check: Integration Tests (nextjs, chrome, 15, RQ)
- GitHub Check: Integration Tests (quickstart, chrome, 16)
- GitHub Check: Integration Tests (quickstart, chrome, 15)
- GitHub Check: Integration Tests (custom, chrome)
- GitHub Check: Integration Tests (billing, chrome, RQ)
- GitHub Check: Integration Tests (nextjs, chrome, 15)
- GitHub Check: Integration Tests (billing, chrome)
- GitHub Check: Integration Tests (machine, chrome)
- GitHub Check: Integration Tests (nuxt, chrome)
- GitHub Check: Integration Tests (nextjs, chrome, 14)
- GitHub Check: Integration Tests (react-router, chrome)
- GitHub Check: Integration Tests (vue, chrome)
- GitHub Check: Integration Tests (sessions, chrome)
- GitHub Check: Integration Tests (expo-web, chrome)
- GitHub Check: Integration Tests (handshake:staging, chrome)
- GitHub Check: Integration Tests (localhost, chrome)
- GitHub Check: Integration Tests (handshake, chrome)
- GitHub Check: Integration Tests (tanstack-react-start, chrome)
- GitHub Check: Integration Tests (express, chrome)
- GitHub Check: Integration Tests (sessions:staging, chrome)
- GitHub Check: Integration Tests (astro, chrome)
- GitHub Check: Integration Tests (generic, chrome)
- GitHub Check: Integration Tests (elements, chrome)
- GitHub Check: Integration Tests (ap-flows, chrome)
- GitHub Check: Static analysis
- GitHub Check: Publish with pkg-pr-new
- GitHub Check: Unit Tests (22, **)
- GitHub Check: Unit Tests (22, shared, clerk-js, RQ)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (4)
packages/shared/src/react/providers/SWRConfigCompat.rq.tsx (1)
6-7: RQ compat wrapper is now a no-op; verify provider/config isn’t needed hereThis RQ variant of
SWRConfigCompatnow just renders{children}and silently ignores the optionalswrConfigprop, while the SWR variant still pipesswrConfigthrough to<SWRConfig>. That’s a reasonable way to keep a shared call surface, but it does mean:
- Any
swrConfigpassed by callers (e.g., customprovider, fetcher, etc.) has no effect in the RQ build.- If React Query needs a per-test or per-tree provider, it must be wired somewhere else in the React tree.
Please double‑check that:
- No remaining callers rely on
swrConfigsemantics in the RQ path, and- The React Query provider/QueryClient is indeed configured in a different wrapper.
If that’s intentional, this implementation looks fine as a thin compat shim.
integration/tests/machine-auth/component.test.ts (3)
425-469: LGTM!The test correctly verifies that a newly created API key appears immediately in the list and that the count increments as expected. The cleanup pattern is appropriate.
471-540: LGTM!Excellent boundary testing that verifies pagination appears exactly when crossing the 10-item threshold. Using a dedicated user ensures test isolation, and the cleanup is thorough.
664-717: LGTM!This test effectively validates that cache invalidation handles multiple rapid, consecutive creations correctly. The cleanup loop properly revokes all created keys.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
packages/shared/src/react/hooks/useSubscription.rq.tsx (1)
32-57: Subscription RQ hook wiring looks consistent; minor redundancy in enabled logic onlyUsing
useBillingHookEnabled(params)plusqueriesEnabled = Boolean(user?.id && billingEnabled)correctly gates the query on environment billing flags, auth, and user presence, anddefineKeepPreviousDataFn(keepPreviousData && queriesEnabled)aligns with the intended keep-previous-data semantics. Note thatuseBillingHookEnabledalready enforcesclerk.loadedand (by default) user/org presence, so the extrauser?.idcheck inqueriesEnabledis redundant; you could simplify toconst queriesEnabled = billingEnabledwithout changing behavior.packages/shared/src/react/hooks/createBillingPaginatedHook.tsx (1)
109-150: Enablement logic works but can be simplified for clarity
billingEnabled = useBillingHookEnabled({ for: safeFor, enabled: externalEnabled, authenticated: !options?.unauthenticated })already factors inclerk.loadedand auth requirements, andhookParamsis always non‑undefined given the current destructuring. As a result:const isEnabled = !!hookParams && clerk.loaded && !!billingEnabled;effectively collapses to
clerk.loaded && !!billingEnabled, duplicating theclerk.loadedcheck and the (always‑true)!!hookParams. You could simplify to something like:const isEnabled = !!billingEnabled;or, if you want an explicit unauthenticated branch, to the minimal expression you actually need, without changing behavior.
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (1)
14-51: RQ payment-attempt hook wiring looks correct; only minor API-shape nitThe hook correctly:
- Derives
organizationId/userIdand feeds them intousePaymentAttemptQueryCacheKeys.- Gates the query with
queryEnabled = Boolean(paymentAttemptId) && billingEnabled, wherebillingEnabled = useBillingHookEnabled(params)folds inparams.enabled, env flags, and auth.- Uses
queryKey[3].argsconsistently with the cache-key helper and normalizeserroranddatato the publicPaymentAttemptQueryResultshape.To reduce coupling between this hook and
useBillingHookEnabled’s param shape, you could optionally pass only the relevant fields instead of the wholeparamsobject:const billingEnabled = useBillingHookEnabled({ for: forType, enabled: params.enabled, // authenticated left to default (true) unless you later need to override });This keeps behavior the same while making the dependency more explicit.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.rq.tsx(1 hunks)packages/shared/src/react/hooks/createBillingPaginatedHook.tsx(4 hunks)packages/shared/src/react/hooks/createCacheKeys.ts(3 hunks)packages/shared/src/react/hooks/useBillingHookEnabled.ts(1 hunks)packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx(4 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/useSubscription.rq.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx
- packages/shared/src/react/billing/useStripeLoader.rq.tsx
- packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx
- packages/shared/src/react/hooks/useStatementQuery.rq.tsx
- packages/shared/src/react/hooks/createCacheKeys.ts
🧰 Additional context used
🧬 Code graph analysis (6)
packages/shared/src/react/hooks/useSubscription.rq.tsx (3)
packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/hooks/useSubscription.shared.ts (1)
useSubscriptionCacheKeys(7-29)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/createBillingPaginatedHook.tsx (1)
packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)
packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx (1)
packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (5)
packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (1)
useStripeClerkLibs(39-39)packages/shared/src/react/hooks/index.ts (1)
useClerk(9-9)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (5)
packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (2)
UsePaymentAttemptQueryParams(7-30)PaymentAttemptQueryResult(35-52)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
usePaymentAttemptQueryCacheKeys(7-32)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/useBillingHookEnabled.ts (2)
packages/shared/src/types/billing.ts (1)
ForPayerType(69-69)packages/shared/src/types/environment.ts (1)
EnvironmentResource(11-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (5)
packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx (2)
6-6: LGTM: Centralized utility import improves maintainability.The refactor to use
defineKeepPreviousDataFnfrom a shared module reduces code duplication and aligns with the PR's goal of consolidating utilities. The conditional return of a keep-previous-data function orundefinedcorrectly matches React Query'splaceholderDataAPI.Also applies to: 74-74
130-173: Verify the page fallback behavior in infinite mode (line 147).The consolidation of data/count/page computation into
computedValuesimproves maintainability. However, line 147's fallback logic warrants review:page: validPages.length > 0 ? validPages.length : initialPageRef.current,When no valid pages are loaded, this returns
initialPageRef.current(e.g., ifinitialPageis 3,pagewould be 3 despite having zero data pages). This could be confusing to consumers who might expectpageto reflect actual loaded pages rather than the initial configuration.Is this fallback intentional for maintaining page state during loading, or should it default to a more intuitive value like
0or1?Otherwise, the refactoring is solid:
- Proper null/undefined filtering and array validation (line 138)
- Correct data flattening for infinite accumulation (lines 141-145)
- Appropriate cache-only reads when queries are disabled (lines 153-155)
- Comprehensive dependency array with clear eslint-disable rationale
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (3)
1-6: LGTM: Imports are appropriate.All imported utilities are used and correctly structured for the React Query-based implementation.
14-19: LGTM: Documentation is clear and accurate.The JSDoc correctly describes the hook's purpose, internal status, and conditional export behavior.
40-40: LGTM: Export follows the established pattern.The
__internal_prefix correctly signals this is for internal consumption and matches the pattern used in the SWR version and other hooks in this PR.
panteliselef
force-pushed
the
elef/remove-swr-from-main
branch
from
cbe42db to
10c1b5f
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/shared/src/react/hooks/__tests__/useAPIKeys.spec.tsx (1)
101-137: Update Test 2 to match the actual cascade behavior in SWR mode.The code correctly cascades revalidation for all queries matching the subject, regardless of pagination type (paginated vs infinite). Test 2 at line 97 expects only 1 call in SWR mode, but should expect 2 calls to match Test 3 and Test 4 behavior and align with the
invalidateAlllogic in useAPIKeys.swr.tsx.The changes at lines 135 and 173 are correct (expecting 2 calls). Line 97 needs the same fix:
// Line 97: Change from expect(getAllSpy).toHaveBeenCalledTimes(1); // To: expect(getAllSpy).toHaveBeenCalledTimes(2);The
invalidateAllfunction matches all cache entries withtype === 'apiKeys'andsubject === safeValues.subject, regardless of whether the query is paginated or infinite. Both the paginated and infinite hooks in Test 2 have the same subject and will both revalidate, triggering 2 API calls—not 1.
♻️ Duplicate comments (2)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (1)
37-40: Verify thatinitialDataUpdatedAt: 0fully addresses the behavioral divergence concern.The addition of
initialDataUpdatedAt: 0(Line 39) marksinitialPlanas maximally stale, which should trigger an immediate refetch on mount—partially addressing the past review comment's concern aboutinitialDatasuppressing the initial fetch.However, there remains a semantic difference from the SWR implementation:
- SWR:
fallbackDatais non-cached and only displayed until real data arrives- React Query (current):
initialDatais persisted to cache (albeit marked stale), triggering a fetch but maintaining cache semanticsAdditionally, using both
initialDataandplaceholderDatasimultaneously is unusual:
initialData: initialPlanseeds the cacheplaceholderData: defineKeepPreviousDataFn(keepPreviousData)keeps previous data during subsequent refetches- This combination differs from the past review comment's recommendation to use
placeholderDataforinitialPlanand nativekeepPreviousDataWhile the current implementation may behave correctly (fetch on mount + show initialPlan while fetching), it diverges from both the SWR pattern and React Query's recommended approach for placeholder data.
Verify via test that:
- When
initialPlanis provided, a network request is still triggered on mount (not suppressed by cache freshness)isFetchingcorrectly reflects the loading state- The behavior matches the SWR implementation's immediate-fetch semantics
If tests confirm correct behavior, consider documenting why both
initialDataandplaceholderDataare used together, as this pattern is non-standard.packages/shared/src/react/hooks/__tests__/useAPIKeys.spec.tsx (1)
139-175: Same inconsistency as in the pageSize test above.Line 173 exhibits the same inconsistency: it expects 2 calls in SWR mode after revalidation, while test 3 at line 97 expects only 1 call. This mirrors the concern raised for line 135.
If this is intentional behavior where queries with different query filters trigger an additional call during SWR revalidation (but different pagination types don't), that distinction should be documented.
🧹 Nitpick comments (6)
.changeset/early-clowns-dance.md (1)
1-5: Enhance the changeset description for consumer clarity.The changeset message is minimal and doesn't convey the full context of this change. Since SWR is being replaced with internal React Query-based hooks (per the PR context), consumers would benefit from understanding:
- Whether this is a breaking change to their integrations
- If any migration steps are required for applications using SWR-dependent APIs
- The feature flag strategy for gradual rollout
Consider expanding the description to include migration guidance or a reference to documentation, especially given the WIP status and the broader scope indicated in the AI summary.
Consider revising the changeset description to something like:
--- '@clerk/clerk-js': minor --- -Removes SWR as direct dependency +Removes SWR as a direct dependency. Data-fetching is now handled internally via React Query-based hooks, reducing bundle size and simplifying dependency management. See [migration guide] for details if you have direct SWR dependencies.Alternatively, if the SWR removal is being rolled out behind a feature flag and is not yet enforced for end users, clarify this in the changeset.
.changeset/fuzzy-donuts-jam.md (1)
1-5: Changeset structure is correct; consider expanding the description for clarity.The changeset metadata is well-formed and the
minorversion bump is appropriate for introducing new internal hooks and public API surfaces. However, the message "Creates compatibility layer for SWR hooks that were previously inside@clerk/clerk-js" is somewhat vague given the scope of changes described in the PR.The AI summary indicates this PR introduces internal query implementations (RQ-based with SWR variants), creates internal/public hook surfaces, and exposes stable keys—all of which are more nuanced than the current message suggests. Consider expanding the description to highlight these key additions:
- Creates compatibility layer for SWR hooks that were previously inside `@clerk/clerk-js` + Introduces internal query hooks and exposes SWR compatibility layer for hooks previously located in `@clerk/clerk-js`, including utilities for cache keys and keep-previous-data behaviorpackages/shared/src/react/hooks/createBillingPaginatedHook.tsx (1)
124-124: Remove redundantclerk.loadedcheck.Since
useBillingHookEnabledalready includesclerk.loadedin its return value, the check here is redundant.Apply this diff:
- const isEnabled = !!hookParams && clerk.loaded && !!billingEnabled; + const isEnabled = !!hookParams && !!billingEnabled;packages/shared/src/react/hooks/createCacheKeys.ts (1)
28-35: Document or prevent potential key collisions in object spreading.The function spreads both
queryKey[2](tracked params) andqueryKey[3].args(untracked args) into a single object. If both contain overlapping property names (e.g., both have anorgIdproperty), the args value will silently override the tracked value, which could lead to unexpected behavior.While this may be intentional based on the semantic distinction between tracked and untracked params, it's worth either:
- Documenting this override behavior in a comment
- Using namespace prefixes (e.g.,
tracked_*andargs_*) to prevent collisions- Validating at runtime that no key collisions occur (in development mode)
Example documentation:
/** * @internal + * + * Note: If tracked params and args contain overlapping keys, args will override tracked values. */ export function toSWRQuery<T extends { queryKey: QueryKeyWithArgs<unknown> }>(keys: T) {packages/shared/src/react/billing/useStripeLoader.rq.tsx (2)
9-15: Consider documenting the credential requirements.The
externalGatewayIdandstripePublishableKeyare typed as optional, but both are required for the Stripe SDK to load (enforced by theisEnabledcheck on line 32). While this flexible design allows the hook to be called before credentials are available, consider adding a brief comment explaining that the hook returnsundefineduntil both credentials are provided.
17-22: Consider enhancing the JSDoc with usage guidance.The JSDoc correctly marks this as
@internal, but could benefit from additional details such as:
- Brief description of what the hook does
- When it returns
undefinedvsnullvs aStripeinstance- Example usage showing how to handle the loading state
This would help internal consumers understand the hook's behavior, especially since it only returns
datawithout exposing error or loading states.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (17)
.changeset/early-clowns-dance.md(1 hunks).changeset/fuzzy-donuts-jam.md(1 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.rq.tsx(1 hunks)packages/shared/src/react/hooks/__tests__/useAPIKeys.spec.tsx(2 hunks)packages/shared/src/react/hooks/createBillingPaginatedHook.tsx(4 hunks)packages/shared/src/react/hooks/createCacheKeys.ts(3 hunks)packages/shared/src/react/hooks/useBillingHookEnabled.ts(1 hunks)packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx(4 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.swr.tsx(1 hunks)packages/shared/src/react/hooks/useSubscription.rq.tsx(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx
- packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx
- packages/shared/src/react/hooks/useBillingHookEnabled.ts
- packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx
- packages/shared/src/react/hooks/useStatementQuery.swr.tsx
🧰 Additional context used
🧬 Code graph analysis (9)
packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx (1)
packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/useSubscription.rq.tsx (4)
packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/telemetry/events/method-called.ts (1)
eventMethodCalled(13-25)packages/shared/src/react/hooks/useSubscription.shared.ts (1)
useSubscriptionCacheKeys(7-29)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/createBillingPaginatedHook.tsx (1)
packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)
packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx (6)
packages/shared/src/types/virtual-data-hooks.d.ts (1)
__internal_usePlanDetailsQuery(8-8)packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (2)
UsePlanDetailsQueryParams(7-28)PlanDetailsQueryResult(33-50)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
usePlanDetailsQueryCacheKeys(6-22)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (2)
packages/shared/src/react/hooks/usePlanDetailsQuery.types.ts (2)
UsePlanDetailsQueryParams(7-28)PlanDetailsQueryResult(33-50)packages/shared/src/react/hooks/usePlanDetailsQuery.shared.ts (1)
usePlanDetailsQueryCacheKeys(6-22)
packages/shared/src/react/hooks/createCacheKeys.ts (2)
packages/shared/src/react/stable-keys.ts (2)
ResourceCacheStableKey(53-53)__internal_ResourceCacheStableKey(69-69)packages/shared/src/react/hooks/usePageOrInfinite.types.ts (1)
QueryKeyWithArgs(13-19)
packages/shared/src/react/billing/useStripeLoader.rq.tsx (4)
packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (1)
UseStripeClerkLibsResult(12-12)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (4)
packages/shared/src/types/virtual-data-hooks.d.ts (1)
__internal_usePaymentAttemptQuery(9-9)packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (2)
UsePaymentAttemptQueryParams(7-30)PaymentAttemptQueryResult(35-52)packages/shared/src/react/index.ts (3)
useClerkInstanceContext(12-12)useUserContext(18-18)useOrganizationContext(15-15)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
usePaymentAttemptQueryCacheKeys(7-32)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (5)
packages/shared/src/react/hooks/useStatementQuery.types.ts (2)
UseStatementQueryParams(6-29)StatementQueryResult(34-51)packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
useStatementQueryCacheKeys(7-32)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (25)
packages/shared/src/react/hooks/useSubscription.rq.tsx (7)
1-1: LGTM: Import changes align with the refactoring.The new imports (defineKeepPreviousDataFn, useBillingHookEnabled, useSubscriptionCacheKeys) support the migration from SWR to React Query by centralizing billing enablement checks, cache key generation, and keep-previous-data logic.
Also applies to: 4-4, 13-14
32-32: LGTM: Billing enablement check centralized.The useBillingHookEnabled hook consolidates the billing enablement logic, including the params.enabled check, which is cleaner than inline conditionals.
40-44: LGTM: Cache key generation centralized.The useSubscriptionCacheKeys hook consolidates cache key generation, making it easier to maintain consistency across the codebase.
46-46: LGTM: Simplified enabled check.The queriesEnabled logic is now cleaner since useBillingHookEnabled already incorporates the params?.enabled check and organization requirements based on the 'for' parameter.
56-56: LGTM: Keep-previous-data logic simplified.The defineKeepPreviousDataFn utility cleanly encapsulates the placeholder data logic, returning a keep-previous-data function when enabled or undefined otherwise, which is the expected React Query behavior.
51-51: No issues found—queryKey structure is correctly implemented.The change from
queryKey[1]toqueryKey[3]is correct. The queryKey array structure fromcreateCacheKeysis[stablePrefix, authenticated, tracked, untracked], anduseSubscriptionCacheKeyspassesuntracked: { args: { orgId: safeOrgId } }. AccessingqueryKey[3].argsat line 51 correctly retrieves the args object needed for the API call.
19-24: The review comment is based on inaccurate information.Based on codebase verification:
- No
subscriptionQueryexport exists inpackages/shared/src/react/hooks/useSubscription.rq.tsxor any index file- No usages of
subscriptionQueryexist anywhere in the codebase (rg search returned zero results)- The file only exports
useSubscription, which is properly exported as__experimental_useSubscriptionin the public APIThe AI summary indicating
subscriptionQuerywas removed appears to be incorrect. There is no removal of this symbol to verify, and no breaking changes related to it.Likely an incorrect or invalid review comment.
packages/shared/src/react/hooks/createBillingPaginatedHook.tsx (3)
2-2: LGTM: Imports support the refactoring.The new imports are necessary for centralizing billing enablement logic through the
useBillingHookEnabledhook.Also applies to: 12-12
126-150: LGTM: Direct return simplifies the code.The inline return of
usePagesOrInfiniteis cleaner, and all parameters are correctly configured.
109-113: Refactoring verified and working as intended.The
useBillingHookEnabledhook correctly centralizes billing enablement logic with proper user/organization presence checks. Test suite validates that authenticated hooks require user context (test: "authenticated hook: does not fetch when user is null"), while unauthenticated hooks (options: { unauthenticated: true }) proceed without it. The behavior is intentional, tested, and doesn't break existing flows.packages/shared/src/react/hooks/usePlanDetailsQuery.swr.tsx (1)
1-45: LGTM! SWR implementation follows best practices.The implementation correctly:
- Uses
fallbackData(non-cached placeholder) rather thaninitialData, ensuring an immediate fetch on mount- Derives
targetPlanIdfromplanIdorinitialPlan?.id- Conditionally enables the query when
targetPlanIdis present andenabledis true- Maps SWR's
isValidatingto the expectedisFetchingproperty- Configures
dedupingIntervalto prevent duplicate requests within a 60-second windowpackages/shared/src/react/hooks/createCacheKeys.ts (1)
1-23: LGTM! Type widening supports internal stable keys.The changes appropriately widen the
stablePrefixparameter type to accept both public and internal stable keys, enabling the cache key utilities to be used by internal hooks without exposing internal keys publicly.packages/shared/src/react/hooks/usePagesOrInfinite.rq.tsx (3)
6-6: LGTM! Using shared utility for consistency.Replacing the local
KeepPreviousDataFnimplementation with the importeddefineKeepPreviousDataFnimproves consistency across the codebase.
74-74: LGTM! Correct placeholder data configuration.Using
defineKeepPreviousDataFn(keepPreviousData)forplaceholderDatacorrectly implements the "keep previous data" behavior in React Query.
130-173: LGTM! Consolidated data derivation improves maintainability.The unified
computedValuesapproach:
- Reduces code duplication by consolidating data/count/page derivation into a single memo
- Adds defensive normalization (Array.isArray checks, filtering null/undefined)
- Correctly distinguishes between enabled vs cache-only reads
- Properly handles both infinite and paginated modes
The eslint-disable comment is justified—
forceUpdateCounteris intentionally used to trigger re-renders for cache updates.packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
1-55: LGTM! Clean React Query implementation following best practices.This implementation is well-structured:
- Correctly derives
organizationIdanduserIdfrom context based onforType- Uses
useBillingHookEnabledappropriately (authenticated defaults totrue, which is correct for statements)- Only uses
placeholderDataforkeepPreviousDatabehavior (noinitialDataseeding)- Properly conditionally enables the query based on
statementIdpresence and billing enablement- Maps query response to the expected
StatementQueryResultshapeThe absence of
initialData(unlikeusePlanDetailsQuery.rq.tsx) makes this implementation cleaner and avoids potential cache seeding issues.packages/shared/src/react/hooks/usePaymentAttemptQuery.swr.tsx (7)
1-4: LGTM! Clean imports.All imports are used and properly organized.
6-12: LGTM! Clear documentation.The JSDoc comment effectively communicates that this is a backwards-compatibility implementation marked for deprecation in the next major version.
13-16: LGTM! Sensible defaults and context usage.The default values align with the type definitions and the context hooks follow standard patterns.
18-19: LGTM! Correct ID derivation logic.The conditional logic properly derives organizationId only for organization scope and handles null cases appropriately.
21-26: LGTM! Proper cache key generation.The hook correctly delegates to the shared cache key function with all required parameters.
28-28: LGTM! Robust enabled logic.The query is correctly gated on the presence of required IDs and respects the enabled flag, preventing requests when organizational context is missing for organization-scoped queries.
30-40: The array indexing is intentionally stable and type-safe; the fragility concern is not valid.The
queryKey[3].argsaccess you flagged is not fragile. The query key structure is explicitly defined by theQueryKeyWithArgs<Params>type, which guarantees index [3] is{ args: Params }. This structure is enforced at compile time by TypeScript, not dependent on implementation details. If the structure were to change, it would be a type error before reaching runtime.The pattern
queryKey[3].argsis used consistently across the codebase (usePaymentAttemptQuery.swr.tsx, useSubscription.swr.tsx, usePagesOrInfinite.rq.tsx, and others). While a helper liketoSWRQuery(which exists in createCacheKeys.ts) is used in some places for consistency, it is optional.The code as written is correct and will function reliably.
Likely an incorrect or invalid review comment.
packages/shared/src/react/billing/useStripeLoader.rq.tsx (2)
26-32: LGTM! Well-structured query key and enabled logic.The
queryKeyis properly memoized to ensure React Query cache stability. TheisEnabledlogic correctly guards the query execution by requiring all credentials (stripeClerkLibs,externalGatewayId,stripePublishableKey) and billing context, which aligns with the flexible design allowing the hook to be called before credentials are available.
54-54: LGTM! Export follows internal API conventions.The
__internal_prefix clearly indicates this hook is for internal use, consistent with the broader API design for this billing functionality.
panteliselef
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (2)
8-12: MissingUseStripeClerkLibsResulttype alias.The past review comment already identified that
UseStripeClerkLibsResultshould be defined here (as it is in the SWR variant atuseStripeClerkLibs.swr.tsx:11). The inline return type on line 17 works but lacks the semantic clarity of a named type.
22-32: Query configuration concerns previously identified.Past review comments already noted:
- The magic string
['clerk-stripe-sdk']could be extracted to a constant for consistency- The type assertion
as LoadStripeFnon line 25 relies on loosePromise<any>typing fromclerk.__internal_loadStripeJs()- Consumers cannot access error or loading states since only
query.datais returnedpackages/shared/src/react/billing/useStripeLoader.rq.tsx (1)
31-46: Critical behavioral difference from SWR implementation previously identified.The past review comment already raised a critical concern: React Query's
staleTime(line 43) does not replicate SWR'sdedupingIntervalbehavior. Requests after 60 seconds will trigger new fetches with React Query, whereas the original SWR implementation would prevent them. This is a behavioral regression for SDK loading where you want to minimize refetches.
🧹 Nitpick comments (5)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
27-29: Consider destructuring params for clarity.Passing the entire
paramsobject touseBillingHookEnabledworks due to structural typing, but onlyforandenabledare actually used. For better readability, consider:-const billingEnabled = useBillingHookEnabled(params); +const billingEnabled = useBillingHookEnabled({ for: forType, enabled: params.enabled });This makes it explicit which properties are consumed by the billing check.
packages/shared/src/react/billing/useStripeLoader.rq.tsx (1)
48-48: Consider consistent return pattern withuseStripeClerkLibs.This hook returns
query.datadirectly (allowingundefined), while the similaruseStripeClerkLibshook returnsquery.data ?? null(ensuringnullinstead ofundefined). Both are technically correct per their return types, but the inconsistency may cause confusion.For consistency, consider:
- return query.data; + return query.data ?? null;And update the return type:
-export type UseStripeLoaderResult = Stripe | null | undefined; +export type UseStripeLoaderResult = Stripe | null;packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (3)
8-11: Consider adding comprehensive JSDoc documentation.The current JSDoc only includes the
@internalmarker. Adding a description of the hook's purpose, parameters, and return value would improve maintainability and developer experience.Example:
/** + * Fetches a payment attempt by ID for a user or organization. + * + * @param params - Configuration for the payment attempt query + * @returns Payment attempt data, error state, and loading states * @internal */ function usePaymentAttemptQuery(params: UsePaymentAttemptQueryParams): PaymentAttemptQueryResult {
31-36: Consider refactoring to avoid hardcoded array index.Line 34 accesses
queryKey[3].argswith a hardcoded index, which is brittle and tightly couples this code to the internal structure of the cache key generated bycreateCacheKeys. If the cache key structure changes, this will break silently at runtime.Consider using closure to pass args directly:
const query = useClerkQuery({ queryKey, - queryFn: ({ queryKey }) => { - const args = queryKey[3].args; - return clerk.billing.getPaymentAttempt(args); + queryFn: () => { + return clerk.billing.getPaymentAttempt({ + id: paymentAttemptId ?? undefined, + orgId: organizationId ?? undefined, + }); }, enabled: queryEnabled,
37-40: Consider making staleTime configurable.The hardcoded staleTime of 60 seconds might be too short for payment attempt data, which typically doesn't change frequently. Consider making this configurable via params or using a longer default value (e.g., 5 minutes).
Example:
+ const staleTime = params.staleTime ?? 1_000 * 60 * 5; // 5 minutes default const query = useClerkQuery({ queryKey, queryFn: ({ queryKey }) => { const args = queryKey[3].args; return clerk.billing.getPaymentAttempt(args); }, enabled: queryEnabled, placeholderData: defineKeepPreviousDataFn(keepPreviousData), - staleTime: 1_000 * 60, + staleTime, });
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (7)
packages/clerk-js/package.json(1 hunks)packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx(1 hunks)packages/shared/src/react/billing/useStripeLoader.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx(1 hunks)packages/shared/src/react/hooks/useStatementQuery.rq.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/clerk-js/package.json
- packages/shared/src/react/billing/useInitializePaymentMethod.rq.tsx
- packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx
🧰 Additional context used
🧬 Code graph analysis (4)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (5)
packages/shared/src/react/hooks/useStatementQuery.types.ts (2)
UseStatementQueryParams(6-29)StatementQueryResult(34-51)packages/shared/src/react/hooks/useStatementQuery.shared.ts (1)
useStatementQueryCacheKeys(7-32)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (4)
packages/shared/src/react/hooks/index.ts (1)
useClerk(9-9)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (5)
packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts (2)
UsePaymentAttemptQueryParams(7-30)PaymentAttemptQueryResult(35-52)packages/shared/src/react/hooks/usePaymentAttemptQuery.shared.ts (1)
usePaymentAttemptQueryCacheKeys(7-32)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
packages/shared/src/react/billing/useStripeLoader.rq.tsx (4)
packages/shared/src/react/billing/useStripeClerkLibs.swr.tsx (1)
UseStripeClerkLibsResult(12-12)packages/shared/src/react/hooks/useBillingHookEnabled.ts (1)
useBillingHookEnabled(7-27)packages/shared/src/react/clerk-rq/useQuery.ts (1)
useClerkQuery(40-42)packages/shared/src/react/clerk-rq/keep-previous-data.ts (1)
defineKeepPreviousDataFn(4-11)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Build Packages
- GitHub Check: Formatting | Dedupe | Changeset
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: semgrep-cloud-platform/scan
🔇 Additional comments (8)
packages/shared/src/react/hooks/useStatementQuery.rq.tsx (1)
1-52: LGTM! Clean React Query implementation.The hook is well-structured and follows React Query best practices:
- Proper context usage and derived values (organizationId, userId)
- Memoized cache key generation
- Correct enabled logic combining statementId presence and billing enablement
- Defensive error handling in queryFn (lines 34-36)
- Appropriate staleTime (60s) for billing data
- Clean return value mapping to StatementQueryResult
packages/shared/src/react/billing/useStripeClerkLibs.rq.tsx (1)
34-37: LGTM!The return pattern correctly ensures
nullinstead ofundefinedwhen no data is available, and the internal export alias follows the established pattern.packages/shared/src/react/billing/useStripeLoader.rq.tsx (2)
1-16: LGTM!Type definitions and imports are well-structured. The explicit
undefinedinUseStripeLoaderResultcorrectly reflects the query return behavior.
20-30: LGTM!The memoized query key and computed enablement logic correctly prevent unnecessary fetches when required parameters are missing or billing is disabled.
packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx (4)
1-6: LGTM!The imports are well-organized and necessary for the hook implementation.
12-18: LGTM!The parameter destructuring with sensible defaults and context derivation logic are correctly implemented. The conditional organizationId based on forType ensures the correct payer context is used.
20-29: LGTM!The cache key generation and query enablement logic correctly coordinate to ensure the query only executes when appropriate conditions are met (valid paymentAttemptId and billing enabled).
50-50: LGTM!The export alias follows the established pattern for internal hooks in this PR, enabling internal usage while maintaining the
@internaldesignation.
| return { | ||
| data: query.data, | ||
| error: (query.error ?? null) as PaymentAttemptQueryResult['error'], | ||
| isLoading: query.isLoading, | ||
| isFetching: query.isFetching, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
Verify error type compatibility.
Line 44 uses a type assertion to coerce query.error to PaymentAttemptQueryResult['error'] (which is ClerkAPIResponseError | null). This bypasses TypeScript's type checking and could hide type mismatches if the underlying query error type doesn't match ClerkAPIResponseError.
Please verify that errors thrown by clerk.billing.getPaymentAttempt are actually of type ClerkAPIResponseError and that the error handling in the query library preserves this type. Consider removing the type assertion if types are compatible, or add runtime validation if they might differ.
🏁 Script executed:
#!/bin/bash
# Description: Search for the getPaymentAttempt implementation and error handling to verify error types
# Search for getPaymentAttempt method definition
ast-grep --pattern 'getPaymentAttempt($$$) {
$$$
}'
# Search for ClerkAPIResponseError usage in billing-related code
rg -n --type=ts -C3 'ClerkAPIResponseError' -g '*billing*'
# Search for error handling in clerk-rq useQuery implementation
rg -n --type=ts -C5 'export function useClerkQuery' -A 20Length of output: 2693
🏁 Script executed:
#!/bin/bash
# Search for the full usePaymentAttemptQuery implementation
rg -n --type=ts -B5 -A30 'function usePaymentAttemptQuery' packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx
# Search for PaymentAttemptQueryResult type definition
rg -n --type=ts 'type PaymentAttemptQueryResult\|interface PaymentAttemptQueryResult'
# Search for clerk.billing.getPaymentAttempt usage and type hints
rg -n --type=ts 'clerk\.billing\.getPaymentAttempt\|getPaymentAttempt' -g '*billing*'Length of output: 1248
🏁 Script executed:
#!/bin/bash
# View the full file to see the complete return statement
cat packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx
# Search for .types file with PaymentAttemptQueryResult
fd 'usePaymentAttemptQuery.types' packages/Length of output: 1831
🏁 Script executed:
#!/bin/bash
# View the types file
cat packages/shared/src/react/hooks/usePaymentAttemptQuery.types.ts
# Search for DefaultError definition
rg -n --type=ts 'type DefaultError\|export.*DefaultError' packages/shared/src/react/Length of output: 1329
🏁 Script executed:
#!/bin/bash
# Search for DefaultError from React Query
rg -n 'DefaultError' packages/shared/src/react/clerk-rq/ -A2 -B2
# Search for other uses of useClerkQuery with explicit TError
rg -n 'useClerkQuery' packages/shared/src/react/hooks/ -A5 | head -100
# Check if clerk.billing methods are documented to throw ClerkAPIResponseError
rg -n 'clerk\.billing\.' packages/shared/src/react/hooks/ -C2Length of output: 24853
🏁 Script executed:
#!/bin/bash
# Check return statements in similar query hooks for type assertions
rg -n 'error:' packages/shared/src/react/hooks/ --type=ts -A2 -B2 | grep -A5 -B5 'as'
# Search for all type assertions on error in billing hooks
rg -n 'as.*\[.*error' packages/shared/src/react/hooks/ --type=ts
# View useSubscription return to compare patterns
cat packages/shared/src/react/hooks/useSubscription.rq.tsx | tail -20
# View usePlanDetailsQuery return to compare patterns
cat packages/shared/src/react/hooks/usePlanDetailsQuery.rq.tsx | tail -20Length of output: 6438
Add explicit TError parameter to useClerkQuery or validate error types at runtime.
The type assertion on line 44 masks a real type mismatch. useClerkQuery defaults to TError = DefaultError (from @tanstack/query-core), but the function returns error: ClerkAPIResponseError | null. This same pattern affects useStatementQuery and usePlanDetailsQuery.
Fix by either:
- Passing explicit
TErrorparameter:useClerkQuery<..., ClerkAPIResponseError>({...}) - Or adding runtime validation to ensure
clerk.billing.getPaymentAttempterrors areClerkAPIResponseError
Similar hooks like useSubscription.rq.tsx handle this differently (converting to undefined without type assertion), suggesting inconsistent error handling across the codebase.
🤖 Prompt for AI Agents
In packages/shared/src/react/hooks/usePaymentAttemptQuery.rq.tsx around lines 42
to 47, the return casts query.error to PaymentAttemptQueryResult['error']
masking a type mismatch; change the useClerkQuery invocation to supply the
explicit TError generic (e.g., ClerkAPIResponseError) so the query.error type
matches the hook's declared error type, remove the runtime type assertion on
return, and apply the same explicit-generic fix to useStatementQuery and
usePlanDetailsQuery (alternatively add runtime validation that converts
non-ClerkAPIResponseError values to null/undefined before returning).
wobsoriano
left a comment
wobsoriano
left a comment
There was a problem hiding this comment.
sweet. thanks for improving the api key tests!
Description
This PR removes completely the
swrfrom builds when theCLERK_USE_RQ=trueenv var is used.It removes
swrfrom a direct dependecy and moves everything to@clerk/sharedso that the compatibility layer is only exists inside the shared package until the new major releases are out.FWIW we could have
useClerkQuerybeing imported directly intoclerk-jsbut I decided against it. Instead hooks were transfered inside@clerk/shared/reactand were exported with the__internal_prefix.Now that migration is complete (behind the FF), we can successfully measure the impact of
RQinside clerk-js. the overall bundlesize was increased by 5.72KB which is well within the limits the original projection.Checklist
pnpm testruns as expected.pnpm buildruns as expected.Type of change
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.