Skip to content

fix(pricing): make price and currency formatting more robust#374

Open
Atharvsinh-codez wants to merge 1 commit into
dodopayments:mainfrom
Atharvsinh-codez:fix/robust-price-handling-and-currency-formatting
Open

fix(pricing): make price and currency formatting more robust#374
Atharvsinh-codez wants to merge 1 commit into
dodopayments:mainfrom
Atharvsinh-codez:fix/robust-price-handling-and-currency-formatting

Conversation

@Atharvsinh-codez

@Atharvsinh-codez Atharvsinh-codez commented Jun 2, 2026

Copy link
Copy Markdown

Summary

  • Centralized price parsing and display in src/utils/price-utils.ts, then registered that helper in the affected registry entries so installed components get the same code.
  • Fixed pricing-table-one rendering the currency twice after switching to shared formatting.
  • Updated proration-preview to format full amounts directly instead of extracting a currency symbol from a dummy formatted value.
  • Made null, empty, and contact-sales style prices behave like custom prices instead of falling through to $0.00.
  • Reused the same display helper in the update-plan card/dialog and let usage-table format costs with either a symbol or an ISO currency code.

Review updates

  • Removed the unused getParsedPrice and shouldShowCurrencyPrefix exports.
  • Dropped the currency-codes import from the shared price helper; ISO currency codes now use Intl.NumberFormat directly.
  • Documented the parser assumption that decimal input is in an en-US style format.
  • Regenerated the affected public/r/*.json files and registry manifests.

Test plan

  • npm run typecheck
  • npm run lint (passes with the existing .source/dynamic.ts unused variable warning)
  • npm run build:shadcn

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude Code Review

This pull request is from a fork — automated review is disabled. A repository maintainer can comment @claude review to run a one-time review.

@vercel

vercel Bot commented Jun 2, 2026

Copy link
Copy Markdown

@Atharvsinh-codez is attempting to deploy a commit to the Dodo Payments Team on Vercel.

A member of the Team first needs to authorize it.

@Atharvsinh-codez

Copy link
Copy Markdown
Author

@tsahil01 Can you please merge it, After reviewing,

thanks.

@thepushkaraj

Copy link
Copy Markdown
Collaborator

@dodo-squirrels review

@dodo-squirrels

Copy link
Copy Markdown

Code Review

The overall direction of this PR is good — centralizing price parsing and formatting logic into a shared module eliminates copy-paste bugs and makes "Custom" price handling consistent. Here are the issues I found that need to be addressed before merging:


Bug: Double currency symbol in pricing-table-one.tsx

This is the most impactful issue. The yearly and monthly price display blocks now render the currency symbol twice:

{shouldShowCurrencyPrefix(plan.yearlyPrice) && (
  <>{plan.currency}</>   {/* renders '$' */}
)}
{isCustomPrice(plan.yearlyPrice) ? plan.yearlyPrice : formatPrice(parsePrice(plan.yearlyPrice) ?? 0, plan.currency)}
{/* formatPrice also prepends '$', producing '729199.00' */}

Since formatPrice already includes the currency prefix, the explicit {plan.currency} render and shouldShowCurrencyPrefix guard are now redundant. Remove both, and just use formatPrice/isCustomPrice directly:

{isCustomPrice(plan.yearlyPrice) ? plan.yearlyPrice : formatPrice(parsePrice(plan.yearlyPrice) ?? 0, plan.currency)}

The same double-prefix pattern appears for plan.monthlyPrice.


Bug: proration-preview.tsx currency extraction is fragile

The new approach to extract the currency symbol is:

const chargeCurrency = formatPrice(0, newPlan.currency ?? ...).replace(/[\d.,\s]/g, "") || "$";

This formats a dummy value and then strips digits/punctuation to get back just the symbol. For "$" this works ("/usr/bin/bash.00" → "$"), but for ISO codes like "EUR" it returns "€" which is correct, though for "JPY" (no decimal) it would return "¥" correctly too. However, this is a roundabout approach — you're formatting a number and then un-formatting it just to get a symbol. A cleaner approach is to add a dedicated getCurrencySymbol(currency: string): string helper to price-utils.ts using Intl.NumberFormat directly, or to pass the currency through and let formatPrice handle the full amount display (avoiding the split between symbol and number entirely).

More importantly, the existing display strings in proration-preview.tsx still interpolate currency manually:

`${creditCurrency}${currentPrice}/${currentPlan.type}`
// renders: "0/monthly" (raw number, no .toFixed(2))

These should use formatPrice(currentPrice, creditCurrency) for consistent formatting.


isCustomPrice behavior inconsistency with null

isCustomPrice(null) returns false (due to the early value == null → return false), but parsePrice(null) returns undefined. This means a null value silently passes the isCustomPrice check, falls through to parsePrice, and becomes undefined — which then gets displayed as formatPrice(0, ...) via the ?? 0 fallback. A null price is not the same as a zero price. Consider returning true from isCustomPrice for null/undefined values, or handle null explicitly.


Brittle regex in parsePrice

const cleaned = String(value).replace(/[^\d.\-]/g, "");

This strips all non-numeric chars, meaning "1,234.56" (locale-formatted input) becomes "1234.56" correctly, but "1.234,56" (European format) becomes "1.23456" — silently wrong. This is a pre-existing issue but worth noting since the PR adds new call sites. At minimum, add a comment documenting the assumption that input is in en-US decimal format.


getParsedPrice is exported but unused

getParsedPrice is defined and exported in price-utils.ts but not called by any component in this PR. Consider removing it or marking it for future use to avoid dead code.


New currency-codes dependency

The formatPrice function imports currency-codes for ISO code lookup. Please verify:

  1. This package is already listed in package.json (or add it explicitly)
  2. The bundle size impact is acceptable for a component library

Minor: currency prop comment in usage-table.tsx

currency?: string; // e.g. "$" or "USD" - now properly supported

This comment says both symbol ("$") and ISO code ("USD") are accepted. That's good, but worth also documenting that ISO codes trigger Intl.NumberFormat formatting (which changes the output format significantly — "USD"→"0.00" vs. "$"→"$20.00") so consumers know what to expect.


Summary

The core concept (shared utility module, consistent custom-price detection) is solid. The main blocker is the double currency symbol bug in pricing-table-one.tsx. The currency extraction in proration-preview.tsx also needs a cleaner approach. The other points are lower priority but worth addressing for correctness and maintainability.

@Atharvsinh-codez Atharvsinh-codez force-pushed the fix/robust-price-handling-and-currency-formatting branch 2 times, most recently from 016870f to 12f50d5 Compare June 19, 2026 17:52
A bunch of components were doing their own ad-hoc parsing for prices (which are strings to allow 'Custom' etc). Some had basic guards, proration had nicer regex stripping after a previous review, but it wasn't shared.

This adds a small shared utils/price-utils.ts (modeled after the existing card-validation one) with:
- safe parsePrice that handles junk input
- isCustomPrice
- formatPrice that prefers proper Intl when you give it a real currency code like 'USD'
- calculateDiscount that reuses the above
- a couple helpers for the 'show prefix or raw' cases

Then wired it into the main pricing tables, the update plan stuff, proration preview, and usage table (which was always hardcoding $ before).

Also registered the util so it comes along when people install the components via the CLI or shadcn.

Should make weird plan data from APIs or configs less likely to blow up the UI, and currency display more correct for non-USD stuff.

Tested typecheck and lint clean (build has a pre-existing tailwind issue in global.css unrelated to this).
@Atharvsinh-codez Atharvsinh-codez force-pushed the fix/robust-price-handling-and-currency-formatting branch from 12f50d5 to 101bc00 Compare June 19, 2026 17:55
@Atharvsinh-codez Atharvsinh-codez changed the title Make price and currency formatting more robust and consistent fix(pricing): make price and currency formatting more robust Jun 19, 2026
@Atharvsinh-codez

Copy link
Copy Markdown
Author

Thanks for the review. I pushed an update that addresses the points you called out:

  • removed the double currency prefix in pricing-table-one
  • changed proration display to format full amounts directly with formatPrice
  • made null/empty prices resolve as custom instead of falling back to zero
  • removed the unused helper exports and the currency-codes import from price-utils
  • documented the parser assumption and regenerated the affected registry JSON

Local checks: npm run typecheck, npm run lint, and npm run build:shadcn pass. Lint still has the pre-existing .source/dynamic.ts unused variable warning.

@Atharvsinh-codez

Copy link
Copy Markdown
Author

@dodo-squirrels

Copy link
Copy Markdown

Code Review

The refactor to centralize price formatting in price-utils.ts is a meaningful improvement. Centralizing isCustomPrice, parsePrice, formatPrice, formatBillingPrice, and calculateDiscount eliminates copy-paste bugs and makes "Custom" handling consistent across all billing components.

The previous review's blockers have been addressed:

  • No more double currency prefix in pricing-table-one.tsx
  • isCustomPrice(null) now correctly returns true (the value == null early-return guard)
  • Proration display now uses formatPrice for full amounts
  • Dead-code exports removed

One typographic regression to fix before merging:

Em dash replaced with double hyphen in proration-preview.tsx

The original code used "—" (proper em dash) for the "not computed" fallback in the credit, charge, and net amount rows. The new code replaced these with "--" (double hyphen). The em dash is the semantically correct character for a missing/unknown value in a financial UI. "--" will look out of place relative to the rest of the UI.

Additionally, formatPrice itself now also returns "--" for non-finite amounts. Both should be updated back to "—".

Fix:

// In proration-preview.tsx (three occurrences):
: --}    : }

// In price-utils.ts formatPrice:
return --;    return ;

Minor: formatBillingPrice returns lowercase for "contact us" / "contact sales"

When a price value is "contact us" or "contact sales", isCustomPrice returns true and formatBillingPrice returns the raw lowercase string as-is. This could display "contact us" in a price slot in the UI. Consider normalizing to title case or mapping to a canonical display string (e.g., "Contact Sales") for these cases.


Informational: CRLF line endings in JSON content strings

The demo files inside the registry JSON blobs (proration-preview-demo.tsx, usage-table-demo.tsx, etc.) were changed from \n to \r\n line endings. This adds noise to the diff but has no runtime impact since the strings are decoded when consumed. Worth normalizing if the project has a consistent line-ending convention (.gitattributes or editorconfig).


Overall the PR is close to mergeable. The em dash fix is the main thing worth addressing before merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants