Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughSummary by CodeRabbitNew Features
WalkthroughThis pull request introduces a comprehensive billing management feature by adding a new billing page to the client-demo application and exposing a complete BillingView component from the React SDK. The changes include routing and navigation updates for the settings section, a billing page wrapper, and eight new billing-related components (billing view, details card/dialog, payment method card, cycle switching, invoices display, payment issue handling, upcoming billing cycle, and plan change banner). Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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 |
rohanchkrabrty
changed the title
Coverage Report for CI Build 24117711088Coverage remained the same at 41.146%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: 4
🧹 Nitpick comments (10)
web/sdk/react/views-new/billing/billing-view.module.css (2)
4-4: Consider using1pxinstead of0.5pxfor border width.Sub-pixel border values like
0.5pxmay render inconsistently across browsers and display densities—some browsers round to 0px or 1px depending on the device pixel ratio.
48-51: Consider using a CSS custom property or viewport-relative unit formax-height.The hardcoded
424pxvalue may not adapt well to different viewport sizes or when the dialog content varies. Consider using a design token or a viewport-relative unit likemax-height: min(424px, 70vh).web/sdk/react/views-new/billing/components/payment-issue.tsx (1)
36-36: Skeleton lacks dimensions.The
<Skeleton />element renders without explicit width/height, which may cause layout shifts or render as a minimal placeholder. Consider adding dimensions to match the expected banner size.♻️ Proposed fix
- if (isLoading) return <Skeleton />; + if (isLoading) return <Skeleton width="100%" height={48} />;web/sdk/react/views-new/billing/components/invoices.tsx (1)
107-109: Consider memoizing columns to avoid recreation on every render.
getColumnsis called on every render. While the performance impact is minimal, memoizing withuseMemowould be more idiomatic when the columns depend on config values.♻️ Proposed fix
+import { useMemo } from 'react'; export function Invoices({ isLoading, invoices }: InvoicesProps) { const { config } = useFrontier(); - const columns = getColumns({ - dateFormat: config?.dateFormat || DEFAULT_DATE_FORMAT - }); + const columns = useMemo( + () => getColumns({ dateFormat: config?.dateFormat || DEFAULT_DATE_FORMAT }), + [config?.dateFormat] + );web/sdk/react/views-new/billing/components/payment-method-card.tsx (2)
100-113: Tooltip wrapper is missing content.The
Tooltipcomponent wraps the button but never rendersTooltip.Content, so it serves no purpose. Either remove theTooltipwrapper or add appropriate content (e.g., a disabled state message similar toBillingDetailsCard).Proposed fix: Remove unnecessary Tooltip wrapper
{isAllowed ? ( - <Tooltip> - <Tooltip.Trigger render={<span />}> - <Button - variant="outline" - color="neutral" - size="small" - onClick={updatePaymentMethod} - disabled={isBtnDisabled} - data-test-id="frontier-sdk-update-payment-method-btn" - > - {isPaymentMethodAvailable ? 'Update' : 'Add method'} - </Button> - </Tooltip.Trigger> - </Tooltip> + <Button + variant="outline" + color="neutral" + size="small" + onClick={updatePaymentMethod} + disabled={isBtnDisabled} + data-test-id="frontier-sdk-update-payment-method-btn" + > + {isPaymentMethodAvailable ? 'Update' : 'Add method'} + </Button> ) : null}
85-88: No feedback when checkout URL is missing.If the mutation succeeds but
checkoutUrlis empty or undefined, the user receives no feedback. Consider adding a fallback toast notification.Proposed fix
const checkoutUrl = resp?.checkoutSession?.checkoutUrl; if (checkoutUrl) { window.location.href = checkoutUrl; + } else { + toastManager.add({ + title: 'Unable to start checkout', + description: 'No checkout URL was returned. Please try again.', + type: 'error' + }); }web/sdk/react/views-new/billing/billing-view.tsx (1)
27-29: Module-scoped dialog handles create singleton state.These handles are created at module scope, meaning all instances of
BillingViewwould share the same dialog state. If this component is ever rendered multiple times (e.g., in different routes that remain mounted), opening a dialog from one instance would affect all instances. Consider moving the handle creation inside the component or usinguseMemo.Proposed fix: Move handles inside component
-const cycleSwitchDialogHandle = - Dialog.createHandle<ConfirmCycleSwitchPayload>(); -const billingDetailsDialogHandle = Dialog.createHandle(); - export interface BillingViewProps { onNavigateToPlans?: () => void; } export function BillingView({ onNavigateToPlans }: BillingViewProps) { + const cycleSwitchDialogHandle = useMemo( + () => Dialog.createHandle<ConfirmCycleSwitchPayload>(), + [] + ); + const billingDetailsDialogHandle = useMemo( + () => Dialog.createHandle(), + [] + ); + const {web/sdk/react/views-new/billing/components/upcoming-billing-cycle.tsx (1)
134-142: Error toast loses specificity.Both
memberCountErrorandinvoiceErrortrigger the same generic toast message. Consider differentiating or including the error message for debugging.Proposed fix
const error = memberCountError || invoiceError; useEffect(() => { if (error) { toastManager.add({ title: 'Failed to get upcoming billing cycle details', + description: error instanceof Error ? error.message : undefined, type: 'error' }); } }, [error]);web/sdk/react/views-new/billing/components/confirm-cycle-switch-dialog.tsx (2)
17-17: Import only needed lodash function.Importing the entire lodash library (
import * as _) for a singleisEmptycall increases bundle size. Import only what's needed.Proposed fix
-import * as _ from 'lodash'; +import isEmpty from 'lodash/isEmpty';Then update line 121:
- _.isEmpty(paymentMethod) && nextPlanPrice.amount > 0; + isEmpty(paymentMethod) && nextPlanPrice.amount > 0;
243-250: Currency symbol handling is limited.Only USD gets a symbol (
$). Other currencies will display just the amount without a symbol. Consider using a more robust currency formatter or theAmountcomponent used elsewhere in the billing views.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a8871bb8-8cdd-414c-b14a-40acbff38821
📒 Files selected for processing (15)
web/apps/client-demo/src/Router.tsxweb/apps/client-demo/src/pages/Settings.tsxweb/apps/client-demo/src/pages/settings/Billing.tsxweb/sdk/react/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.ts
| onSuccess: data => { | ||
| window.location.href = data?.checkoutUrl as string; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Unsafe cast of potentially undefined checkoutUrl.
If data?.checkoutUrl is undefined, casting it as string will pass the type check but window.location.href will receive undefined, causing navigation to a literal "undefined" URL. Add a guard.
Proposed fix
checkoutPlan({
planId: nextPlanId,
isTrial: false,
onSuccess: data => {
- window.location.href = data?.checkoutUrl as string;
+ if (data?.checkoutUrl) {
+ window.location.href = data.checkoutUrl;
+ }
}
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onSuccess: data => { | |
| window.location.href = data?.checkoutUrl as string; | |
| } | |
| }); | |
| onSuccess: data => { | |
| if (data?.checkoutUrl) { | |
| window.location.href = data.checkoutUrl; | |
| } | |
| } | |
| }); |
| onSuccess: async () => { | ||
| const planPhase = await verifyPlanChange({ | ||
| planId: nextPlanId | ||
| }); | ||
| if (planPhase) { | ||
| handle.close(); | ||
| const changeDate = timestampToDayjs( | ||
| planPhase?.effectiveAt | ||
| )?.format(dateFormat); | ||
| toastManager.add({ | ||
| title: 'Plan cycle switch successful', | ||
| description: `Your plan cycle will be switched to ${nextPlanIntervalName} on ${changeDate}`, | ||
| type: 'success' | ||
| }); | ||
| } | ||
| }, | ||
| immediate: isUpgrade | ||
| }); | ||
| } |
There was a problem hiding this comment.
No user feedback if verifyPlanChange returns undefined.
Per the codebase, verifyPlanChange performs a single fetch (not polling). If the subscription phase isn't immediately available after changePlan completes, planPhase will be undefined, and the user receives no feedback—the dialog remains open with no indication of success or failure.
Consider adding an else branch or implementing retry logic.
Proposed fix: Add fallback feedback
const planPhase = await verifyPlanChange({
planId: nextPlanId
});
if (planPhase) {
handle.close();
const changeDate = timestampToDayjs(
planPhase?.effectiveAt
)?.format(dateFormat);
toastManager.add({
title: 'Plan cycle switch successful',
description: `Your plan cycle will be switched to ${nextPlanIntervalName} on ${changeDate}`,
type: 'success'
});
+ } else {
+ handle.close();
+ toastManager.add({
+ title: 'Plan cycle switch initiated',
+ description: 'Your plan change is being processed.',
+ type: 'success'
+ });
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| onSuccess: async () => { | |
| const planPhase = await verifyPlanChange({ | |
| planId: nextPlanId | |
| }); | |
| if (planPhase) { | |
| handle.close(); | |
| const changeDate = timestampToDayjs( | |
| planPhase?.effectiveAt | |
| )?.format(dateFormat); | |
| toastManager.add({ | |
| title: 'Plan cycle switch successful', | |
| description: `Your plan cycle will be switched to ${nextPlanIntervalName} on ${changeDate}`, | |
| type: 'success' | |
| }); | |
| } | |
| }, | |
| immediate: isUpgrade | |
| }); | |
| } | |
| onSuccess: async () => { | |
| const planPhase = await verifyPlanChange({ | |
| planId: nextPlanId | |
| }); | |
| if (planPhase) { | |
| handle.close(); | |
| const changeDate = timestampToDayjs( | |
| planPhase?.effectiveAt | |
| )?.format(dateFormat); | |
| toastManager.add({ | |
| title: 'Plan cycle switch successful', | |
| description: `Your plan cycle will be switched to ${nextPlanIntervalName} on ${changeDate}`, | |
| type: 'success' | |
| }); | |
| } else { | |
| handle.close(); | |
| toastManager.add({ | |
| title: 'Plan cycle switch initiated', | |
| description: 'Your plan change is being processed.', | |
| type: 'success' | |
| }); | |
| } | |
| }, | |
| immediate: isUpgrade | |
| }); | |
| } |
| const onRetryPayment = useCallback(() => { | ||
| window.location.href = openInvoices[0]?.hostedUrl || ''; | ||
| }, [openInvoices]); |
There was a problem hiding this comment.
Guard against empty or missing hostedUrl before navigation.
If openInvoices is empty or hostedUrl is undefined, setting window.location.href = '' can cause unexpected navigation (typically reloads the current page or navigates to the root). Consider disabling the retry button or showing a fallback message when no valid invoice URL is available.
🛡️ Proposed fix
+ const retryUrl = openInvoices[0]?.hostedUrl;
+
const onRetryPayment = useCallback(() => {
- window.location.href = openInvoices[0]?.hostedUrl || '';
- }, [openInvoices]);
+ if (retryUrl) {
+ window.location.href = retryUrl;
+ }
+ }, [retryUrl]);
if (isLoading) return <Skeleton />;
if (!isPastDue) return null;
+ if (!retryUrl) return null; // Or show a message to contact support
return (📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onRetryPayment = useCallback(() => { | |
| window.location.href = openInvoices[0]?.hostedUrl || ''; | |
| }, [openInvoices]); | |
| const retryUrl = openInvoices[0]?.hostedUrl; | |
| const onRetryPayment = useCallback(() => { | |
| if (retryUrl) { | |
| window.location.href = retryUrl; | |
| } | |
| }, [retryUrl]); |
There was a problem hiding this comment.
@rohilsurana @rsbh What should be the fallback here? All invoices page? Empty string as fallback can cause issues.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| <Text size="small"> | ||
| Your {currentPlanName} will be{' '} | ||
| {planAction?.btnDoneLabel.toLowerCase()} to {upcomingPlanName} from{' '} | ||
| {expiryDate}. | ||
| </Text> |
There was a problem hiding this comment.
Potential undefined access on planAction.
If getPlanChangeAction returns undefined (e.g., when weightages are equal), planAction?.btnDoneLabel will be undefined, and calling .toLowerCase() on it will throw. The optional chaining only prevents accessing btnDoneLabel on undefined, but .toLowerCase() is still called on the result.
Proposed fix
<Text size="small">
Your {currentPlanName} will be{' '}
- {planAction?.btnDoneLabel.toLowerCase()} to {upcomingPlanName} from{' '}
+ {planAction?.btnDoneLabel?.toLowerCase() ?? 'changed'} to {upcomingPlanName} from{' '}
{expiryDate}.
</Text>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| <Text size="small"> | |
| Your {currentPlanName} will be{' '} | |
| {planAction?.btnDoneLabel.toLowerCase()} to {upcomingPlanName} from{' '} | |
| {expiryDate}. | |
| </Text> | |
| <Text size="small"> | |
| Your {currentPlanName} will be{' '} | |
| {planAction?.btnDoneLabel?.toLowerCase() ?? 'changed'} to {upcomingPlanName} from{' '} | |
| {expiryDate}. | |
| </Text> |
| } from '@raystack/proton/frontier'; | ||
| import { useMutation } from '@connectrpc/connect-query'; | ||
| import { create } from '@bufbuild/protobuf'; | ||
| import { AuthTooltipMessage } from '../../../utils'; |
There was a problem hiding this comment.
Please check if this import is unused.
| import { DEFAULT_DATE_FORMAT } from '../../../utils/constants'; | ||
| import { timestampToDayjs } from '../../../../utils/timestamp'; | ||
| import { usePlans } from '../../../views/plans/hooks/usePlans'; | ||
| import * as _ from 'lodash'; |
There was a problem hiding this comment.
Lets avoid * import. Please check if this can be a named import. If it's a big effort lets pick this later.
I think we are only using isEmpty from lodash.
| </Text> | ||
| {isAllowed ? ( | ||
| <Tooltip> | ||
| <Tooltip.Trigger render={<span />}> |
There was a problem hiding this comment.
Tooltip doesn't have Content. Please add.
|
|
||
| import { Button, Skeleton, Text, Flex, Tooltip } from '@raystack/apsara-v1'; | ||
| import type { BillingAccount } from '@raystack/proton/frontier'; | ||
| import { converBillingAddressToString } from '../../../utils'; |
There was a problem hiding this comment.
converBillingAddressToString lets fix the typo and all reference. conver -> convert
3 participants
Summary
views/billingtoviews-new/billingusing apsara-v1 components, CSS Modules with design tokens, and theViewContainer/ViewHeaderpatternBillingDetailsDialogwith inline form (Name, Email, Address, Pincode, City, State, Country, Tax ID) that callsupdateBillingAccountAPI directly, replacing the previous Stripe customer portal redirectConfirmCycleSwitchDialogto match Figma design with inline bold text layout and a new savings line ("You can save $X with Y cycle")Dialog.createHandlepattern with handles scoped in the parent view and passed as props to avoid non-portable DTS build errors