Repository navigation
feat(desktop): add plans comparison page with feature matrix - #867
Merged
Merged
Conversation
- Add billing settings section with plans overview - Create PlansComparison component with Linear-inspired design - Add feature comparison grid showing Usage, Features, Support, Security - Support monthly/yearly billing toggle for Pro plan pricing - Include Free ($0), Pro ($20/mo or $180/yr), and Enterprise tiers - Add proper plan feature breakdown (workspaces, team members, etc.)
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a comprehensive billing settings feature to the desktop application. It adds a billing overview dashboard displaying the current plan and invoices, a plans comparison view showing feature matrices across free/pro/enterprise tiers, new route pages, and integrates billing-related items into the settings sidebar and search functionality. The changes define billing plan structures, mock data, and UI components with associated type definitions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/components/FeatureList/FeatureList.tsx`:
- Around line 8-23: The FeatureList component currently renders every item in
the features prop with a check icon regardless of the PlanFeature.included flag;
update FeatureList to only render features where included is true (e.g., filter
the features prop before mapping) so excluded features are not shown with a
green check, and keep the rest of the rendering logic (including HiCheck,
feature.name and feature.limit) unchanged to preserve layout and behavior.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsx`:
- Around line 176-288: The code checks current plan by comparing
currentPlanLabel === plan.name which is brittle; change the comparison to use
the canonical plan identifier (e.g., billingInfo.currentPlan) against the plan's
tier/id field (e.g., plan.tier or plan.id) inside the PLAN_CARDS mapping so
isCurrent is computed as billingInfo.currentPlan === plan.tier (or plan.id); if
PLAN_CARDS entries don't include a tier/id, add that property to each plan
object and update any references that assumed plan.name for logic (leave UI
labels using plan.name or currentPlanLabel untouched).
- Around line 321-326: The Switch rendering under plan.showBillingToggle uses a
static aria-label "Billed yearly" which misrepresents the current state; update
the Switch (the element with props checked={isYearly} and
onCheckedChange={setIsYearly}) to supply a dynamic aria-label that reflects
isYearly (e.g., conditional label like "Billed yearly" when isYearly is true and
"Billed monthly" when false) so screen readers announce the correct billing
period.
🧹 Nitpick comments (5)
apps/desktop/src/renderer/routes/_authenticated/settings/billing/constants.ts (1)
34-133: Extract pricing/limit literals into named constants (clarify units).There are many numeric literals (e.g., 2000/18000, 1/5, mock usage). Naming them (e.g.,
PRO_MONTHLY_PRICE_CENTS,FREE_MAX_USERS) improves clarity and reuse.♻️ Example refactor pattern
+const FREE_MAX_USERS = 1; +const FREE_MAX_WORKSPACES = 5; +const PRO_MONTHLY_PRICE_CENTS = 2000; +const PRO_YEARLY_PRICE_CENTS = 18000; + export const PLANS: Record<PlanTier, Plan> = { free: { ... limits: { - maxUsers: 1, - maxWorkspaces: 5, + maxUsers: FREE_MAX_USERS, + maxWorkspaces: FREE_MAX_WORKSPACES, }, }, pro: { ... - price: { monthly: 2000, yearly: 18000 }, + price: { monthly: PRO_MONTHLY_PRICE_CENTS, yearly: PRO_YEARLY_PRICE_CENTS }, },Based on learnings, extract magic numbers to named constants.
apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/BillingOverview/components/UpgradeCard/UpgradeCard.tsx (1)
24-24: Consider extracting the cents-to-dollars divisor to a named constant.Per coding guidelines, magic numbers should be extracted to named constants. The
100divisor for converting cents to dollars could be defined at module level.💡 Suggested improvement
+const CENTS_PER_DOLLAR = 100; + export function UpgradeCard() { const plan = PLANS.pro;Then use:
- ${plan.price?.monthly ? plan.price.monthly / 100 : 0} per user/mo + ${plan.price?.monthly ? plan.price.monthly / CENTS_PER_DOLLAR : 0} per user/moapps/desktop/src/renderer/routes/_authenticated/settings/billing/page.tsx (1)
14-18: Prefer params object forgetMatchingItemsForSection.
Repo guidance requires object params for 2+ arguments; consider updating the helper signature and this call for consistency.♻️ Proposed adjustment (with helper signature update)
- return getMatchingItemsForSection(searchQuery, "billing").map( + return getMatchingItemsForSection({ query: searchQuery, section: "billing" }).map( (item) => item.id, );apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/components/PlanCard/PlanCard.tsx (2)
25-35: Refactor CTA branching into an action map.
Guidelines prefer lookup objects over repeated conditionals; this also makes new actions easier to add.♻️ Example refactor
- const handleCTA = () => { - if (plan.cta.action === "current") { - return; - } - - if (plan.cta.action === "contact") { - window.open("mailto:founders@superset.sh", "_blank"); - } else if (plan.cta.action === "upgrade") { - toast.info("Stripe integration coming soon"); - } - }; + const ctaHandlers: Record<Plan["cta"]["action"], () => void> = { + current: () => {}, + contact: () => window.open("mailto:founders@superset.sh", "_blank"), + upgrade: () => toast.info("Stripe integration coming soon"), + }; + + const handleCTA = () => { + ctaHandlers[plan.cta.action](); + };
64-73: Extract pricing math constants.
The100and12literals are core pricing semantics; promoting them to named constants improves readability and reduces mistakes.♻️ Proposed constants
+const CENTS_PER_DOLLAR = 100; +const MONTHS_PER_YEAR = 12; + export function PlanCard({ plan, currentPlan }: PlanCardProps) { const isCurrent = plan.id === currentPlan; @@ <span className="text-3xl font-bold"> - ${plan.price.monthly / 100} + ${plan.price.monthly / CENTS_PER_DOLLAR} </span> @@ - or ${plan.price.yearly / 100}/year (~$ - {Math.round(plan.price.yearly / 12 / 100)} + or ${plan.price.yearly / CENTS_PER_DOLLAR}/year (~$ + {Math.round( + plan.price.yearly / MONTHS_PER_YEAR / CENTS_PER_DOLLAR, + )} /mo)
Comment on lines
+8
to
+23
| export function FeatureList({ features }: FeatureListProps) { | ||
| return ( | ||
| <ul className="space-y-3"> | ||
| {features.map((feature) => ( | ||
| <li key={feature.id} className="flex items-start gap-2"> | ||
| <HiCheck className="h-5 w-5 text-green-600 flex-shrink-0 mt-0.5" /> | ||
| <div className="flex-1"> | ||
| <span className="text-sm">{feature.name}</span> | ||
| {feature.limit && ( | ||
| <span className="text-xs text-muted-foreground ml-1"> | ||
| ({feature.limit}) | ||
| </span> | ||
| )} | ||
| </div> | ||
| </li> | ||
| ))} |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the FeatureList component and its type definitions
find apps/desktop/src/renderer -name "FeatureList*" -type fRepository: superset-sh/superset
Length of output: 194
🏁 Script executed:
# Check the FeatureList.tsx file for its prop types
cat -n apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/components/FeatureList/FeatureList.tsxRepository: superset-sh/superset
Length of output: 936
🏁 Script executed:
# Find where PlanFeature is defined
rg "type PlanFeature|interface PlanFeature" --type ts --type tsxRepository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Find where FeatureList is imported and used
rg "FeatureList" --type ts --type tsx -B 2 -A 5Repository: superset-sh/superset
Length of output: 91
🏁 Script executed:
# Find the constants file that defines PlanFeature
find apps/desktop/src/renderer/routes/_authenticated/settings/billing -name "constants*" -type fRepository: superset-sh/superset
Length of output: 141
🏁 Script executed:
# Search for PlanFeature definition more broadly
rg "PlanFeature" apps/desktop/src/renderer/routes/_authenticated/settings/billing --max-count 20Repository: superset-sh/superset
Length of output: 622
🏁 Script executed:
# Look at the directory structure to find the constants file
ls -la apps/desktop/src/renderer/routes/_authenticated/settings/billing/Repository: superset-sh/superset
Length of output: 415
🏁 Script executed:
# Read the constants.ts file to see PlanFeature definition
cat -n apps/desktop/src/renderer/routes/_authenticated/settings/billing/constants.tsRepository: superset-sh/superset
Length of output: 4031
🏁 Script executed:
# Find where FeatureList is used - check PlansComparison parent component
find apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison -name "*.tsx" -type fRepository: superset-sh/superset
Length of output: 431
🏁 Script executed:
# Read PlansComparison component
cat -n apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsxRepository: superset-sh/superset
Length of output: 12975
🏁 Script executed:
# Read PlanCard component to see how FeatureList is used
cat -n apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/components/PlanCard/PlanCard.tsxRepository: superset-sh/superset
Length of output: 3176
Filter features to respect the included flag or document that upstream filtering is guaranteed.
The PlanFeature type includes an included boolean field, but FeatureList renders all items with a green checkmark regardless of that flag. While all current plan features have included: true, this component will misrepresent excluded features if the field is ever used. Either filter to included features here or ensure the caller guarantees pre-filtered data.
⚠️ Possible fix (filter to included only)
export function FeatureList({ features }: FeatureListProps) {
+ const includedFeatures = features.filter((feature) => feature.included);
return (
<ul className="space-y-3">
- {features.map((feature) => (
+ {includedFeatures.map((feature) => (
<li key={feature.id} className="flex items-start gap-2">
<HiCheck className="h-5 w-5 text-green-600 flex-shrink-0 mt-0.5" />
<div className="flex-1">
<span className="text-sm">{feature.name}</span>📝 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.
Suggested change
| export function FeatureList({ features }: FeatureListProps) { | |
| return ( | |
| <ul className="space-y-3"> | |
| {features.map((feature) => ( | |
| <li key={feature.id} className="flex items-start gap-2"> | |
| <HiCheck className="h-5 w-5 text-green-600 flex-shrink-0 mt-0.5" /> | |
| <div className="flex-1"> | |
| <span className="text-sm">{feature.name}</span> | |
| {feature.limit && ( | |
| <span className="text-xs text-muted-foreground ml-1"> | |
| ({feature.limit}) | |
| </span> | |
| )} | |
| </div> | |
| </li> | |
| ))} | |
| export function FeatureList({ features }: FeatureListProps) { | |
| const includedFeatures = features.filter((feature) => feature.included); | |
| return ( | |
| <ul className="space-y-3"> | |
| {includedFeatures.map((feature) => ( | |
| <li key={feature.id} className="flex items-start gap-2"> | |
| <HiCheck className="h-5 w-5 text-green-600 flex-shrink-0 mt-0.5" /> | |
| <div className="flex-1"> | |
| <span className="text-sm">{feature.name}</span> | |
| {feature.limit && ( | |
| <span className="text-xs text-muted-foreground ml-1"> | |
| ({feature.limit}) | |
| </span> | |
| )} | |
| </div> | |
| </li> | |
| ))} | |
| </ul> | |
| ); | |
| } |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/components/FeatureList/FeatureList.tsx`
around lines 8 - 23, The FeatureList component currently renders every item in
the features prop with a check icon regardless of the PlanFeature.included flag;
update FeatureList to only render features where included is true (e.g., filter
the features prop before mapping) so excluded features are not shown with a
green check, and keep the rest of the rendering logic (including HiCheck,
feature.name and feature.limit) unchanged to preserve layout and behavior.
Comment on lines
+176
to
+288
| const billingInfo = MOCK_BILLING_INFO; | ||
| const currentPlanLabelByTier: Record<PlanTier, string> = { | ||
| free: "Free", | ||
| pro: "Pro", | ||
| enterprise: "Enterprise", | ||
| }; | ||
| const currentPlanLabel = currentPlanLabelByTier[billingInfo.currentPlan]; | ||
|
|
||
| const getValue = <T,>(value: T | { monthly: T; yearly: T }): T => { | ||
| if (typeof value === "object" && value !== null && "monthly" in value) { | ||
| return isYearly ? value.yearly : value.monthly; | ||
| } | ||
| return value as T; | ||
| }; | ||
|
|
||
| const handlePlanAction = (action: PlanCardAction) => { | ||
| if (action === "current") { | ||
| return; | ||
| } | ||
|
|
||
| if (action === "contact") { | ||
| window.open("mailto:founders@superset.sh", "_blank"); | ||
| return; | ||
| } | ||
|
|
||
| toast.info("Stripe integration coming soon"); | ||
| }; | ||
|
|
||
| const renderComparisonValue = (value: ComparisonValue) => { | ||
| if (value === null || value === false) { | ||
| return <span className="sr-only">Not included</span>; | ||
| } | ||
|
|
||
| if (value === true) { | ||
| return <HiCheck className="h-3.5 w-3.5 text-muted-foreground" />; | ||
| } | ||
|
|
||
| return ( | ||
| <> | ||
| <HiCheck className="h-3.5 w-3.5 text-muted-foreground" /> | ||
| <span className="text-sm">{value}</span> | ||
| </> | ||
| ); | ||
| }; | ||
|
|
||
| const highlightColumnIndex = 1; | ||
| const highlightColumnStart = highlightColumnIndex + 2; | ||
| const gridColumnsClass = "grid grid-cols-[180px_repeat(3,_1fr)]"; | ||
|
|
||
| return ( | ||
| <div className="p-6 max-w-7xl w-full"> | ||
| <div className="mb-6 space-y-4"> | ||
| <Button variant="ghost" size="sm" asChild> | ||
| <Link to="/settings/billing"> | ||
| <HiArrowLeft className="h-4 w-4" /> | ||
| Billing | ||
| </Link> | ||
| </Button> | ||
| <div> | ||
| <h2 className="text-xl font-semibold">Plans</h2> | ||
| <p className="text-sm text-muted-foreground mt-1"> | ||
| You are on the{" "} | ||
| <span className="text-foreground font-medium"> | ||
| {currentPlanLabel} plan | ||
| </span> | ||
| . If you have any questions or would like further support with your | ||
| plan,{" "} | ||
| <a | ||
| href="mailto:founders@superset.sh" | ||
| className="inline-flex items-center gap-1 text-primary hover:underline" | ||
| > | ||
| contact us | ||
| <HiArrowUpRight className="h-3 w-3" /> | ||
| </a> | ||
| . | ||
| </p> | ||
| </div> | ||
| </div> | ||
|
|
||
| <div className="overflow-x-auto"> | ||
| <div className="relative min-w-[720px]"> | ||
| <div | ||
| className={cn( | ||
| gridColumnsClass, | ||
| "pointer-events-none absolute inset-0", | ||
| )} | ||
| > | ||
| <div | ||
| className="bg-accent/30 border border-border/60 rounded-lg" | ||
| style={{ | ||
| gridColumn: `${highlightColumnStart} / ${highlightColumnStart + 1}`, | ||
| gridRow: "span 3", | ||
| }} | ||
| /> | ||
| </div> | ||
| <div className={cn(gridColumnsClass, "relative z-10 items-start")}> | ||
| {(["plan", "billing", "cta"] as const).map((rowKey, rowIndex) => ( | ||
| <Fragment key={rowKey}> | ||
| <div | ||
| className={cn("px-2", rowKey === "cta" ? "py-3" : "py-2.5")} | ||
| /> | ||
| {PLAN_CARDS.map((plan) => { | ||
| const isCurrent = currentPlanLabel === plan.name; | ||
| const planActions = isCurrent | ||
| ? [ | ||
| { | ||
| label: "Current plan", | ||
| action: "current" as const, | ||
| variant: "secondary" as const, | ||
| }, | ||
| ] | ||
| : plan.actions; | ||
|
|
Contributor
There was a problem hiding this comment.
Avoid comparing current plan by display label.
Using currentPlanLabel === plan.name is brittle if labels change or are localized; compare tiers/ids instead to keep the “current plan” state stable.
🛠️ Suggested fix
- const currentPlanLabelByTier: Record<PlanTier, string> = {
+ const currentPlanLabelByTier: Record<PlanTier, string> = {
free: "Free",
pro: "Pro",
enterprise: "Enterprise",
};
- const currentPlanLabel = currentPlanLabelByTier[billingInfo.currentPlan];
+ const currentPlanTier = billingInfo.currentPlan;
+ const currentPlanLabel = currentPlanLabelByTier[currentPlanTier];
@@
- const isCurrent = currentPlanLabel === plan.name;
+ const isCurrent = currentPlanTier === plan.id;🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsx`
around lines 176 - 288, The code checks current plan by comparing
currentPlanLabel === plan.name which is brittle; change the comparison to use
the canonical plan identifier (e.g., billingInfo.currentPlan) against the plan's
tier/id field (e.g., plan.tier or plan.id) inside the PLAN_CARDS mapping so
isCurrent is computed as billingInfo.currentPlan === plan.tier (or plan.id); if
PLAN_CARDS entries don't include a tier/id, add that property to each plan
object and update any references that assumed plan.name for logic (leave UI
labels using plan.name or currentPlanLabel untouched).
Comment on lines
+321
to
+326
| {plan.showBillingToggle && ( | ||
| <Switch | ||
| checked={isYearly} | ||
| onCheckedChange={setIsYearly} | ||
| aria-label="Billed yearly" | ||
| /> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n "apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsx" | sed -n '310,335p'Repository: superset-sh/superset
Length of output: 935
🏁 Script executed:
wc -l "apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsx"Repository: superset-sh/superset
Length of output: 179
🏁 Script executed:
cat -n "apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsx" | sed -n '1,100p'Repository: superset-sh/superset
Length of output: 3051
🏁 Script executed:
rg "isYearly" "apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsx" -B 5 -A 2Repository: superset-sh/superset
Length of output: 876
Make the billing toggle's aria-label reflect the selected state.
The aria-label currently announces "Billed yearly" regardless of the selected billing period. When isYearly is false, screen readers will provide incorrect information. Use a dynamic label:
- <Switch
- checked={isYearly}
- onCheckedChange={setIsYearly}
- aria-label="Billed yearly"
- />
+ <Switch
+ checked={isYearly}
+ onCheckedChange={setIsYearly}
+ aria-label={isYearly ? "Billed yearly" : "Billed monthly"}
+ />📝 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.
Suggested change
| {plan.showBillingToggle && ( | |
| <Switch | |
| checked={isYearly} | |
| onCheckedChange={setIsYearly} | |
| aria-label="Billed yearly" | |
| /> | |
| {plan.showBillingToggle && ( | |
| <Switch | |
| checked={isYearly} | |
| onCheckedChange={setIsYearly} | |
| aria-label={isYearly ? "Billed yearly" : "Billed monthly"} | |
| /> |
🤖 Prompt for AI Agents
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/billing/components/PlansComparison/PlansComparison.tsx`
around lines 321 - 326, The Switch rendering under plan.showBillingToggle uses a
static aria-label "Billed yearly" which misrepresents the current state; update
the Switch (the element with props checked={isYearly} and
onCheckedChange={setIsYearly}) to supply a dynamic aria-label that reflects
isYearly (e.g., conditional label like "Billed yearly" when isYearly is true and
"Billed monthly" when false) so screen readers announce the correct billing
period.
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
This was referenced Jan 22, 2026
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.