Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughSummary by CodeRabbitRelease Notes
WalkthroughThis PR adds billing and plans management functionality to the SDK, introducing new view components ( Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Coverage Report for CI Build 24420743019Warning Build has drifted: This PR's base is out of sync with its target branch, so coverage data may include unrelated changes. Coverage remained the same at 41.606%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (15)
web/sdk/react/views-new/billing/components/billing-details-card.tsx (1)
36-58: Tooltip logic is correct but could be clearer.The
disabled={!isButtonDisabled}onTooltip.Triggermeans the tooltip only activates when the button is disabled. This is correct behavior, but the double-negative makes it harder to read. Consider extracting to a named variable:const showTooltip = isButtonDisabled; // ... <Tooltip.Trigger disabled={!showTooltip} ...>web/sdk/react/views-new/billing/components/upcoming-plan-change-banner.tsx (2)
12-13: Consolidate duplicate imports from the same module.The imports from
@connectrpc/connect-queryare split across two lines. Consolidate them for cleaner code.♻️ Suggested fix
-import { useMutation } from '@connectrpc/connect-query'; -import { createConnectQueryKey, useTransport } from '@connectrpc/connect-query'; +import { useMutation, createConnectQueryKey, useTransport } from '@connectrpc/connect-query';
96-102: Unsafe type assertion on metadata.Casting
metadataasRecord<string, number>assumes all values are numbers, but metadata is typically a flexible key-value store that may contain strings. Ifweightageis stored as a string,Number()will handle it, but consider using a safer approach or validating the type.web/sdk/react/views-new/plans/components/confirm-plan-change-dialog.tsx (2)
177-187: Unnecessary loading state for synchronous operation.The
setIsNewPlanLoading(true/false)wraps a synchronousArray.find()operation. This will cause an unnecessary render cycle and the loading state will never be visible to users.♻️ Suggested fix
useEffect(() => { if (planId) { - setIsNewPlanLoading(true); - try { - const plan = isNewPlanBasePlan ? basePlan : currentPlan; - if (plan) setNewPlan(plan); - } finally { - setIsNewPlanLoading(false); - } + const plan = isNewPlanBasePlan ? basePlan : currentPlan; + if (plan) setNewPlan(plan); } }, [planId, isNewPlanBasePlan, basePlan, currentPlan]);Then remove
isNewPlanLoadingfrom theisLoadingcomputation on line 189.
102-102: Fragile string comparison for determining upgrade status.Comparing
planAction.btnLabel === 'Upgrade'couples business logic to UI text. If the label is localized or changed, this breaks.Consider using a semantic property like
planAction.isUpgradeor comparing weightage directly.web/sdk/react/views-new/billing/components/confirm-cycle-switch-dialog.tsx (2)
176-209:onConfirmshould be memoized withuseCallback.This async function is defined inside the component body without memoization, causing it to be recreated on every render. While not critical since it's only passed to an
onClickhandler, it's inconsistent with patterns in other dialogs.♻️ Suggested fix
- async function onConfirm() { + const onConfirm = useCallback(async () => { const nextPlanId = nextPlan?.id; if (!nextPlanId) return; // ... rest of function - } + }, [nextPlan?.id, isPaymentMethodRequired, checkoutPlan, changePlan, verifyPlanChange, handle, nextPlanIntervalName, dateFormat, isUpgrade]);
246-248: Limited currency symbol formatting.Only USD gets the
$symbol. Other currencies will display as{amount}without any symbol. Consider usingIntl.NumberFormator theAmountcomponent used elsewhere for consistent currency formatting.web/sdk/react/views-new/billing/components/invoices.tsx (1)
29-95: Consider guarding against NaN when coercing amount.On line 64, if
getValue()returnsundefinedornull,Number()will produceNaN. TheAmountcomponent may not handle this gracefully.🛡️ Suggested defensive check
cell: ({ row, getValue }) => { - const value = Number(getValue()); + const rawValue = getValue(); + const value = rawValue != null ? Number(rawValue) : 0; return ( <Text size="regular" variant="secondary"> <Amount currency={row?.original?.currency} value={value} /> </Text> ); }web/sdk/react/views-new/billing/billing-view.tsx (1)
27-29: Module-scope dialog handles are shared across component instances.These handles are created at module scope, meaning all instances of
BillingViewshare the same dialog state. This is typically fine for singleton views but could cause unexpected behavior if multipleBillingViewinstances are rendered simultaneously.web/sdk/react/views-new/plans/plans-view.tsx (3)
52-60: Consider adding error handling for features query.Unlike the billing view which handles invoice fetch errors with a toast, this component silently ignores errors from
listFeatures. If the features query fails, the UI might show an incomplete feature table without user feedback.🛡️ Suggested error handling
- const { data: featuresData } = useQuery( + const { data: featuresData, error: featuresError } = useQuery( FrontierServiceQueries.listFeatures, create(ListFeaturesRequestSchema, {}) ); + + useEffect(() => { + if (featuresError) { + toastManager.add({ + title: 'Failed to load plan features', + description: featuresError?.message, + type: 'error' + }); + } + }, [featuresError]);
104-123: Potential stale closure:selectedIntervalsread inside effect but not in deps.The effect reads
selectedIntervals[plan.slug]on line 109 to skip already-set intervals, butselectedIntervalsis not in the dependency array. On subsequent runs, the effect sees a stale snapshot.This may be intentional (only initialize defaults once per plan), but it can cause subtle bugs if the logic changes. Consider using a functional update or ref if this is intentional.
♻️ Using functional update to avoid stale closure
useEffect(() => { if (groupedPlans.length === 0) return; - const defaults: Record<string, IntervalKeys> = {}; - groupedPlans.forEach(plan => { - if (selectedIntervals[plan.slug]) return; - const planIntervals = Object.values(plan.intervals) - .sort((a, b) => a.weightage - b.weightage) - .map(i => i.interval); - const activePlanInterval = Object.values(plan.intervals).find( - p => p.planId === activeSubscription?.planId - ); - defaults[plan.slug] = - activePlanInterval?.interval || planIntervals[0] || 'year'; - }); - - if (Object.keys(defaults).length > 0) { - setSelectedIntervals(prev => ({ ...prev, ...defaults })); - } + setSelectedIntervals(prev => { + const defaults: Record<string, IntervalKeys> = {}; + groupedPlans.forEach(plan => { + if (prev[plan.slug]) return; + const planIntervals = Object.values(plan.intervals) + .sort((a, b) => a.weightage - b.weightage) + .map(i => i.interval); + const activePlanInterval = Object.values(plan.intervals).find( + p => p.planId === activeSubscription?.planId + ); + defaults[plan.slug] = + activePlanInterval?.interval || planIntervals[0] || 'year'; + }); + return Object.keys(defaults).length > 0 ? { ...prev, ...defaults } : prev; + }); }, [groupedPlans, activeSubscription?.planId]);
125-132: Consider memoizingcurrentPlanPricing.This nested loop runs on every render. While the dataset is likely small, memoizing it would be more consistent with the other derived values in this component.
♻️ Memoized version
- let currentPlanPricing: IntervalPricingWithPlan | undefined; - groupedPlans.forEach(group => { - Object.values(group.intervals).forEach(plan => { - if (plan.planId === activeSubscription?.planId) { - currentPlanPricing = plan; - } - }); - }); + const currentPlanPricing = useMemo(() => { + for (const group of groupedPlans) { + for (const plan of Object.values(group.intervals)) { + if (plan.planId === activeSubscription?.planId) { + return plan; + } + } + } + return undefined; + }, [groupedPlans, activeSubscription?.planId]);web/sdk/react/views-new/billing/components/upcoming-billing-cycle.tsx (3)
70-100: Inconsistent org ID usage between queries.The invoice query uses
activeOrganization?.id(line 77), while the member count query usesbillingAccount?.orgId(line 94). These should typically be the same, but if they ever differ, it could cause subtle bugs.Consider using a consistent source for the organization ID.
134-142: Error toast may fire multiple times for different errors.The
errorvariable combines bothmemberCountErrorandinvoiceErrorusing||. If both errors occur, only the first truthy one is displayed. Additionally, if one error clears and another appears, the effect will fire again.Consider handling each error independently or using a more robust error aggregation strategy.
♻️ Handle errors independently
- const error = memberCountError || invoiceError; - useEffect(() => { - if (error) { - toastManager.add({ - title: 'Failed to get upcoming billing cycle details', - type: 'error' - }); - } - }, [error]); + useEffect(() => { + if (invoiceError) { + toastManager.add({ + title: 'Failed to load upcoming invoice', + type: 'error' + }); + } + }, [invoiceError]); + + useEffect(() => { + if (memberCountError) { + toastManager.add({ + title: 'Failed to load member count', + type: 'error' + }); + } + }, [memberCountError]);
154-154: Consider specifying skeleton dimensions for better loading UX.A bare
<Skeleton />may render with default dimensions. Specifyingwidthandheightwould provide a more predictable placeholder that matches the final rendered content.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 100c21b4-2bf0-4923-b853-0c10014c94d8
📒 Files selected for processing (28)
web/apps/admin/src/utils/helper.tsweb/apps/client-demo/src/Router.tsxweb/apps/client-demo/src/pages/Settings.tsxweb/apps/client-demo/src/pages/settings/Billing.tsxweb/apps/client-demo/src/pages/settings/Plans.tsxweb/sdk/admin/utils/helper.tsweb/sdk/admin/views/organizations/details/side-panel/billing-details-section.tsxweb/sdk/react/index.tsweb/sdk/react/utils/index.tsweb/sdk/react/views-new/billing/billing-view.module.cssweb/sdk/react/views-new/billing/billing-view.tsxweb/sdk/react/views-new/billing/components/billing-details-card.tsxweb/sdk/react/views-new/billing/components/billing-details-dialog.tsxweb/sdk/react/views-new/billing/components/confirm-cycle-switch-dialog.tsxweb/sdk/react/views-new/billing/components/invoices.tsxweb/sdk/react/views-new/billing/components/payment-issue.tsxweb/sdk/react/views-new/billing/components/payment-method-card.tsxweb/sdk/react/views-new/billing/components/upcoming-billing-cycle.tsxweb/sdk/react/views-new/billing/components/upcoming-plan-change-banner.tsxweb/sdk/react/views-new/billing/index.tsweb/sdk/react/views-new/plans/components/confirm-plan-change-dialog.tsxweb/sdk/react/views-new/plans/components/feature-table.module.cssweb/sdk/react/views-new/plans/components/feature-table.tsxweb/sdk/react/views-new/plans/components/plan-card.module.cssweb/sdk/react/views-new/plans/components/plan-card.tsxweb/sdk/react/views-new/plans/index.tsweb/sdk/react/views-new/plans/plans-view.module.cssweb/sdk/react/views-new/plans/plans-view.tsx
web/sdk/react/views-new/billing/components/confirm-cycle-switch-dialog.tsx
Show resolved
Hide resolved
web/sdk/react/views-new/billing/components/upcoming-plan-change-banner.tsx
Show resolved
Hide resolved
web/sdk/react/views-new/billing/components/payment-method-card.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
web/sdk/react/views-new/plans/helpers/index.ts (3)
11-24: Redundant metadata extraction.
metaData(line 11) andplanMetadata(line 24) perform identical operations. Consider reusing the first variable.♻️ Proposed fix
const metaData = (plan?.metadata as Record<string, string>) || {}; const slug = metaData?.plan_group_id || makePlanSlug(plan); plansMap[slug] = plansMap[slug] || { slug: slug, title: plan.title, description: plan?.description, weightage: 0, intervals: {}, features: {} }; const planInterval = (plan?.interval || '') as IntervalKeys; const productPrices = getPlanPrice(plan); - const planMetadata = (plan?.metadata as Record<string, string>) || {}; plansMap[slug].intervals[planInterval] = { planId: plan?.id || '', planName: plan?.name || '', interval: planInterval, - weightage: planMetadata?.weightage ? Number(planMetadata?.weightage) : 0, + weightage: metaData?.weightage ? Number(metaData?.weightage) : 0, productNames: [], trialDays: plan?.trialDays ? String(plan.trialDays) : '', features: {}, ...productPrices };
13-20: Remove unusedfeaturesproperty from group-level initialization.The
features: {}property (line 19) is initialized at the group level but never populated—features are only stored at the interval level (line 32). Additionally,PlanIntervalPricingtype doesn't include a root-levelfeaturesproperty, so this creates an extra untyped property.♻️ Proposed fix
plansMap[slug] = plansMap[slug] || { slug: slug, title: plan.title, description: plan?.description, weightage: 0, - intervals: {}, - features: {} + intervals: {} };
36-45: Consider usingpush()instead of spread for accumulation.The spread pattern
[...arr, newItem]creates a new array on each iteration. For better performance with many products, usepush()directly.♻️ Proposed fix
plan?.products?.forEach(product => { - plansMap[slug].intervals[planInterval].productNames = [ - ...plansMap[slug].intervals[planInterval].productNames, - product.name || '' - ]; + plansMap[slug].intervals[planInterval].productNames.push(product.name || ''); product.features?.forEach(feature => { plansMap[slug].intervals[planInterval].features[feature?.title || ''] = feature; }); });web/sdk/react/views-new/plans/hooks/use-plans.tsx (3)
71-74: MemoizeplanMapto avoid recalculation on every render.
planMapis rebuilt on each render. Since it only depends onallPlans, wrap it inuseMemo.♻️ Proposed fix
+import { useCallback, useState, useMemo } from 'react'; ... - const planMap = allPlans.reduce((acc, p) => { - if (p.id) acc[p.id] = p; - return acc; - }, {} as Record<string, Plan>); + const planMap = useMemo( + () => + allPlans.reduce((acc, p) => { + if (p.id) acc[p.id] = p; + return acc; + }, {} as Record<string, Plan>), + [allPlans] + );
76-78: Consider memoizingisCurrentlyTrialing.This value is recalculated on every render. If
subscriptionsis stable, consider wrapping inuseMemo.
245-249:getSubscribedPlansdepends on unmemoizedplanMap.Since
planMapis recreated each render, theuseCallbackforgetSubscribedPlansmay not prevent unnecessary recalculations. This is resolved by memoizingplanMapas suggested earlier.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b79db317-5725-459b-82db-0de691e3dde3
📒 Files selected for processing (9)
web/sdk/react/views-new/billing/components/payment-method-card.tsxweb/sdk/react/views-new/plans/components/confirm-plan-change-dialog.tsxweb/sdk/react/views-new/plans/components/plan-card.module.cssweb/sdk/react/views-new/plans/components/plan-card.tsxweb/sdk/react/views-new/plans/helpers/index.tsweb/sdk/react/views-new/plans/hooks/use-plans.tsxweb/sdk/react/views-new/plans/index.tsweb/sdk/react/views-new/plans/plans-view.tsxweb/sdk/react/views/billing/billing-page.tsx
✅ Files skipped from review due to trivial changes (2)
- web/sdk/react/views-new/plans/components/plan-card.module.css
- web/sdk/react/views-new/plans/components/confirm-plan-change-dialog.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- web/sdk/react/views-new/plans/index.ts
- web/sdk/react/views-new/billing/components/payment-method-card.tsx
- web/sdk/react/views-new/plans/components/plan-card.tsx
- web/sdk/react/views-new/plans/plans-view.tsx
3 participants
Summary
views/planstoviews-new/plansusing@raystack/apsara-v1components, CSS Modules, and design tokensusePlans,useFrontier,groupPlansPricingByInterval,listFeaturesquery) without modification