Skip to content

fix(billing): wire billing interval through to purchasePlan on upgrade #5962

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
M3gA-Mind merged 6 commits into from
Sep 3, 2026
+231 −35
Merged

fix(billing): wire billing interval through to purchasePlan on upgrade #5962

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
b62e623
fix(billing): wire billing interval through to purchasePlan on upgrade
YellowSnnowmann Sep 2, 2026
d49c8c5
surface getCurrentPlan and upgrade errors in BillingPanel
YellowSnnowmann Sep 2, 2026
058b525
fix crypto payment UX and disable upgrades during plan load
YellowSnnowmann Sep 2, 2026
093a93b
fix: surface error when checkoutUrl is null; keep upgrades disabled o…
YellowSnnowmann Sep 2, 2026
11e2052
test(billing): pin the interval and crypto paths the panel exists to fix
M3gA-Mind Sep 2, 2026
838c3aa
Merge branch 'main' into fix/billing-interval-persist-5865
M3gA-Mind Sep 3, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
177 changes: 155 additions & 22 deletions app/src/components/settings/panels/BillingPanel.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import { fireEvent, render, screen, waitFor } from '@testing-library/react';
import { beforeEach, describe, expect, it, vi } from 'vitest';

import BillingPanel from './BillingPanel';

const navigateBack = vi.fn();

vi.mock('../hooks/useSettingsNavigation', () => ({
Expand All @@ -15,51 +17,182 @@ vi.mock('../hooks/useSettingsNavigation', () => ({
const openUrlMock = vi.fn();
vi.mock('../../../utils/openUrl', () => ({ openUrl: (url: string) => openUrlMock(url) }));

async function importPanel() {
vi.resetModules();
const mod = await import('./BillingPanel');
return mod.default;
}
const getCurrentPlanMock = vi.fn();
const purchasePlanMock = vi.fn();
const createCoinbaseChargeMock = vi.fn();

vi.mock('../../../services/api/billingApi', () => ({
billingApi: {
getCurrentPlan: (...args: unknown[]) => getCurrentPlanMock(...args),
purchasePlan: (...args: unknown[]) => purchasePlanMock(...args),
createCoinbaseCharge: (...args: unknown[]) => createCoinbaseChargeMock(...args),
},
}));

describe('<BillingPanel />', () => {
beforeEach(() => {
vi.clearAllMocks();
openUrlMock.mockResolvedValue(undefined);
getCurrentPlanMock.mockResolvedValue({
plan: 'FREE',
hasActiveSubscription: false,
planExpiry: null,
subscription: null,
monthlyBudgetUsd: 0,
weeklyBudgetUsd: 0,
});
purchasePlanMock.mockResolvedValue({
checkoutUrl: 'https://checkout.stripe.com/test',
sessionId: 'test-session',
});
createCoinbaseChargeMock.mockResolvedValue({
gatewayTransactionId: 'test-gw',
hostedUrl: 'https://commerce.coinbase.com/test',
status: 'NEW',
expiresAt: '2026-01-01T00:00:00Z',
});
});

it('renders the "billing moved to web" view without auto-opening the browser', async () => {
const Panel = await importPanel();
render(<Panel />);

// Billing no longer auto-opens the dashboard on mount (auto-open removed):
// the panel just explains billing moved to the web.
expect(
screen.getByText(
/Subscription changes, payment methods, credits, and invoices are now managed/
)
).toBeInTheDocument();
it('renders the plan selector and the dashboard button without auto-opening the browser', async () => {
render(<BillingPanel />);

// SubscriptionPlans renders its own title; billing frequency selection is
// back in-app so users can change their plan without leaving the desktop app.
expect(screen.getByText('Choose a Plan')).toBeInTheDocument();
expect(screen.getByRole('button', { name: 'Open billing dashboard' })).toBeInTheDocument();
// No openUrl call happens on mount.

// getCurrentPlan is called on mount but must not trigger a browser open.
await waitFor(() => expect(getCurrentPlanMock).toHaveBeenCalledTimes(1));
expect(openUrlMock).not.toHaveBeenCalled();
});

it('opens the dashboard when the user clicks the primary button', async () => {
const Panel = await importPanel();
render(<Panel />);
it('loads the current plan tier on mount and passes it to SubscriptionPlans', async () => {
getCurrentPlanMock.mockResolvedValue({
plan: 'BASIC',
hasActiveSubscription: true,
planExpiry: null,
subscription: null,
monthlyBudgetUsd: 20,
weeklyBudgetUsd: 10,
});

render(<BillingPanel />);

await waitFor(() => expect(getCurrentPlanMock).toHaveBeenCalledTimes(1));
// With BASIC as current tier the BASIC card shows the "Current plan" badge.
expect(await screen.findByText('Current plan')).toBeInTheDocument();
});

it('upgrade with card payment calls purchasePlan and opens the checkout URL', async () => {
render(<BillingPanel />);

await waitFor(() => expect(getCurrentPlanMock).toHaveBeenCalledTimes(1));

// Both BASIC and PRO show upgrade buttons when current tier is FREE.
const upgradeButtons = screen.getAllByRole('button', { name: 'Upgrade' });
fireEvent.click(upgradeButtons[0]);

await waitFor(() => expect(purchasePlanMock).toHaveBeenCalledTimes(1));
Comment thread
M3gA-Mind marked this conversation as resolved.
expect(purchasePlanMock).toHaveBeenCalledWith('BASIC_MONTHLY');
Comment thread
coderabbitai[bot] marked this conversation as resolved.
await waitFor(() =>
expect(openUrlMock).toHaveBeenCalledWith('https://checkout.stripe.com/test')
);
});

// The reason this PR exists: the interval toggle must reach `purchasePlan`.
// The monthly case above passes on the DEFAULT interval, so it stays green
// even if `buildPlanId(tier, billingInterval)` is hardcoded back to
// 'monthly' — i.e. even with the bug in #5865 fully restored. This is the
// case that fails when that happens.
it('upgrade after selecting Annual sends the yearly plan id', async () => {
render(<BillingPanel />);
await waitFor(() => expect(getCurrentPlanMock).toHaveBeenCalledTimes(1));

fireEvent.click(screen.getByRole('button', { name: 'Annual' }));

const upgradeButtons = await screen.findAllByRole('button', { name: 'Upgrade' });
fireEvent.click(upgradeButtons[0]);

await waitFor(() => expect(purchasePlanMock).toHaveBeenCalledTimes(1));
expect(purchasePlanMock).toHaveBeenCalledWith('BASIC_YEARLY');
});

// The crypto branch of `handleUpgrade` had no test at all: the mock was
// declared and stubbed but never asserted on, so the whole branch was
// unexecuted. Also pins the interval coupling from the Codex P1 — selecting
// crypto forces `annual`, so the price on screen matches the charge.
it('upgrade with crypto creates a Coinbase charge and opens the hosted URL', async () => {
render(<BillingPanel />);
await waitFor(() => expect(getCurrentPlanMock).toHaveBeenCalledTimes(1));

fireEvent.click(screen.getByRole('switch'));

const upgradeButtons = await screen.findAllByRole('button', { name: 'Upgrade' });
fireEvent.click(upgradeButtons[0]);

await waitFor(() => expect(createCoinbaseChargeMock).toHaveBeenCalledTimes(1));
expect(createCoinbaseChargeMock).toHaveBeenCalledWith('BASIC');
// Crypto must never go through the Stripe path.
expect(purchasePlanMock).not.toHaveBeenCalled();
await waitFor(() =>
expect(openUrlMock).toHaveBeenCalledWith('https://commerce.coinbase.com/test')
);
// Selecting crypto switches the interval to annual, so the monthly
// button is disabled and the displayed price cannot disagree with the
// charge that was created.
expect(screen.getByRole('button', { name: 'Monthly' })).toBeDisabled();
});

it('opens the billing dashboard when the user clicks the secondary button', async () => {
render(<BillingPanel />);

fireEvent.click(screen.getByRole('button', { name: 'Open billing dashboard' }));
await waitFor(() => expect(openUrlMock).toHaveBeenCalledTimes(1));
expect(openUrlMock).toHaveBeenLastCalledWith('https://tinyhumans.ai/dashboard');
});

it('invokes the navigation back handler from both the header and the inline button', async () => {
const Panel = await importPanel();
render(<Panel />);
render(<BillingPanel />);

// The SettingsHeader back button (aria-label "Back") and the inline
// "Back to settings" button both route through navigateBack.
fireEvent.click(screen.getByRole('button', { name: 'Back' }));
fireEvent.click(screen.getByRole('button', { name: 'Back to settings' }));
expect(navigateBack).toHaveBeenCalledTimes(2);
});

it('shows an error message when getCurrentPlan rejects', async () => {
getCurrentPlanMock.mockRejectedValue(new Error('Network error'));

render(<BillingPanel />);

await waitFor(() => expect(screen.getByText('Network error')).toBeInTheDocument());

@tinysweeper tinysweeper Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

priority medium critique uncertain

Assert against a translation key or role, not raw error strings

The test at line 124 asserts that the rendered output contains the exact string 'Network error'. The component BillingPanel almost certainly wraps the error in an i18n t(...) call – a raw backend error message would be surprising to an end user and is unlikely to be the string the component actually renders. The same problem exists for 'Payment failed' at line 136. The test will fail once the component uses a translated message, or will pass spuriously if the error display appends extra text and this substring still matches. Instead, look for a consistent UI element (e.g. a role alert or an accessible label) that the component uses for error states.

[RULE] unreliable-text-matching ·

@YellowSnnowmann YellowSnnowmann Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

The strings asserted ('Network error', 'Payment failed') are JavaScript Error.message values from new Error('Network error') in the mock — not translated UI copy. The component passes the raw API error message through to SettingsStatusLine unchanged (line 64, error={error}). The tests are verifying that this propagation chain works end-to-end, which requires asserting the exact message. Switching to a role query without a message assertion would weaken the test: we'd know an error element appeared, but not whether the right content reached the user.

@M3gA-Mind M3gA-Mind Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Disagreeing on this one, with the code — leaving it open for a human.

The finding rests on a premise that does not hold here:

The component BillingPanel almost certainly wraps the error in an i18n t(...) call

It does not. The panel stores the raw message and passes it straight through:

// BillingPanel.tsx:35 and :61
.catch(err => setError(err instanceof Error ? err.message : String(err)))
...
<SettingsStatusLine saving={false} error={error} savingLabel="" />

and SettingsStatusLine is a re-export of ui/StatusLine, which renders that string verbatim:

// ui/StatusLine.tsx:21-22
if (error) {
  content = <span className="text-coral-600 dark:text-coral-300">{error}</span>;
}

So 'Network error' and 'Payment failed' are exactly what reaches the DOM, and the assertions are correct rather than fragile.

The suggested alternative is also not available: that container has aria-live="polite" / aria-atomic="true" but no role="alert" and no accessible name, so there is no role or label to query by. Querying the generic status line instead would be strictly weaker — it would assert that an error rendered, not that this backend error reached the user, which is the behaviour these three tests were added to pin (getCurrentPlan rejects, purchasePlan rejects, purchasePlan returns no checkoutUrl — each surfacing its own distinct message rather than leaving currentTier silently at FREE).

If the panel later starts translating these messages, the right response is to assert the translation key at that point, not to loosen the assertion pre-emptively now.

Leaving this open rather than resolving it, since I argued against the suggestion rather than acting on it.

});

it('shows an error message when purchasePlan rejects', async () => {
purchasePlanMock.mockRejectedValue(new Error('Payment failed'));

render(<BillingPanel />);
await waitFor(() => expect(getCurrentPlanMock).toHaveBeenCalledTimes(1));

const upgradeButtons = screen.getAllByRole('button', { name: 'Upgrade' });
fireEvent.click(upgradeButtons[0]);

await waitFor(() => expect(screen.getByText('Payment failed')).toBeInTheDocument());
});

it('shows an error when purchasePlan returns no checkout URL', async () => {
purchasePlanMock.mockResolvedValue({ checkoutUrl: null, sessionId: 'test-session' });

render(<BillingPanel />);
await waitFor(() => expect(getCurrentPlanMock).toHaveBeenCalledTimes(1));

const upgradeButtons = screen.getAllByRole('button', { name: 'Upgrade' });
fireEvent.click(upgradeButtons[0]);

await waitFor(() =>
expect(screen.getByText('Checkout session did not return a redirect URL')).toBeInTheDocument()
);
expect(openUrlMock).not.toHaveBeenCalled();
});
});
85 changes: 73 additions & 12 deletions app/src/components/settings/panels/BillingPanel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,34 +1,95 @@
import { useEffect, useState } from 'react';

import { useT } from '../../../lib/i18n/I18nContext';
import { billingApi } from '../../../services/api/billingApi';
import type { PlanTier } from '../../../types/api';
import { BILLING_DASHBOARD_URL } from '../../../utils/links';
import { openUrl } from '../../../utils/openUrl';
import Button from '../../ui/Button';
import { SettingsStatusLine } from '../controls';
import { useSettingsNavigation } from '../hooks/useSettingsNavigation';
import SettingsPanel from '../layout/SettingsPanel';
import SubscriptionPlans from './billing/SubscriptionPlans';
import { buildPlanId } from './billingHelpers';

const BillingPanel = () => {
const { t } = useT();
const { navigateBack } = useSettingsNavigation();
const [currentTier, setCurrentTier] = useState<PlanTier>('FREE');
Comment thread
M3gA-Mind marked this conversation as resolved.
const [billingInterval, setBillingInterval] = useState<'monthly' | 'annual'>('monthly');
Comment thread
coderabbitai[bot] marked this conversation as resolved.
const [paymentMethod, setPaymentMethod] = useState<'card' | 'crypto'>('card');
const [isPurchasing, setIsPurchasing] = useState(false);
const [purchasingTier, setPurchasingTier] = useState<PlanTier | null>(null);
const [error, setError] = useState<string | null>(null);
const [planLoading, setPlanLoading] = useState(true);
const [planKnown, setPlanKnown] = useState(false);
const paymentConfirmed = false;

useEffect(() => {
billingApi
Comment thread
M3gA-Mind marked this conversation as resolved.
.getCurrentPlan()
.then(data => {
setCurrentTier(data.plan);
setPlanKnown(true);
})
.catch(err => setError(err instanceof Error ? err.message : String(err)))
.finally(() => setPlanLoading(false));
}, []);

const handleSetPaymentMethod = (method: 'card' | 'crypto') => {
setPaymentMethod(method);
if (method === 'crypto') setBillingInterval('annual');
};

const handleUpgrade = async (tier: PlanTier): Promise<void> => {
setError(null);
setIsPurchasing(true);
setPurchasingTier(tier);
try {
if (paymentMethod === 'crypto') {
Comment thread
M3gA-Mind marked this conversation as resolved.
const charge = await billingApi.createCoinbaseCharge(tier);
Comment thread
M3gA-Mind marked this conversation as resolved.
await openUrl(charge.hostedUrl);
} else {
const session = await billingApi.purchasePlan(buildPlanId(tier, billingInterval));
Comment thread
M3gA-Mind marked this conversation as resolved.
if (session.checkoutUrl) {
await openUrl(session.checkoutUrl);
} else {
throw new Error('Checkout session did not return a redirect URL');
}
}
} catch (err) {
setError(err instanceof Error ? err.message : String(err));
} finally {
setIsPurchasing(false);
setPurchasingTier(null);
}
};

return (
// The description rides the scaffold's own slot: SettingsPanel already
// renders the page h1 (from the settings route registry), so a second
// `text-2xl` heading here stacked two page titles on one page.
<SettingsPanel description={t('settings.billing.movedToWebDesc')}>
<p className="text-xs font-semibold uppercase tracking-wide text-content-muted">
{t('settings.billing.movedToWeb')}
</p>
<SettingsPanel>
<SettingsStatusLine saving={false} error={error} savingLabel="" />
Comment thread
M3gA-Mind marked this conversation as resolved.
<SubscriptionPlans

@chatgpt-codex-connector chatgpt-codex-connector Bot Sep 2, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Keep payment management in the hosted dashboard

Mounting SubscriptionPlans restores embedded desktop payment flows, but gitbooks/features/billing-and-usage.md:51-53 explicitly defines the hosted dashboard as the single place to manage plans and says the desktop panel intentionally contains no payment UI; the parent implementation also enforced that contract. This should remain dashboard-only unless that product decision is deliberately reversed and the authoritative documentation is updated with the behavior change.

AGENTS.md reference: AGENTS.md:L1258-L1260

Useful? React with 👍 / 👎.

@YellowSnnowmann YellowSnnowmann Sep 2, 2026

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Valid flag — the gitbooks doc at lines 51-53 does say the panel "intentionally has no embedded payment UI." The original base code at d67674bff was the simple web-redirect panel.

Issue #5865 describes a user selecting annual billing in the in-app plan selection UI, which implies the in-app flow existed at the time the bug was filed (and SubscriptionPlans.tsx has been in the codebase since 29ce30b73). The PR restored that component into BillingPanel under the assumption that the product is bringing in-app billing back.

If the product direction is still web-only, this PR needs to be scoped down to a simpler fix (e.g. persisting the interval preference so it survives the redirect, or handling it in the web dashboard). Flagging for maintainer clarification — I will not resolve this thread until the product decision is confirmed.

@M3gA-Mind M3gA-Mind Sep 2, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Not resolving this — it needs a maintainer's product decision, not a reviewer's. Adding evidence rather than an opinion.

I checked the claim independently and it holds, in both halves:

1. The documentation says it explicitly. gitbooks/features/billing-and-usage.md on main:

"The desktop Settings → Billing panel intentionally has no embedded payment UI. It links out to the hosted web billing dashboard, which is the single place to manage plans, cards and invoices."

2. It is live, not aspirational. On main, SubscriptionPlans, buildPlanId, purchasePlan and createCoinbaseCharge have no production caller — BillingPanel is the moved-to-web text plus two buttons. This PR revives all four.

One correction to the PR's framing that I think changes the conclusion, and is the reason I am not treating this as a nitpick: the body describes the interval toggle as "orphaned" local state, implying a regression to restore. But on main the toggle is not rendered at all. So #5865's in-app symptom cannot have come from the shipped desktop panel — the reporter was on the hosted dashboard or a pre-move build. That makes this a reversal of a product decision rather than a regression fix, and if the reporter was on the web dashboard it does not fix their bug either.

Either way the decision is above a reviewer's pay grade, and it is binary:

  • Reverse the decision → this PR is the right shape, but billing-and-usage.md must move with it in the same change, or the docs immediately contradict the build.
  • Keep the decision → the panel stays dashboard-only and the interval fix belongs in the hosted dashboard, not here.

@YellowSnnowmann has been holding this thread pending exactly that call, which I think was the right instinct. I have not approved this PR, and this thread is why — the rest of it is in good shape (the interval and crypto coverage gap is now genuinely closed; I verified it, see my summary comment).

currentTier={currentTier}
billingInterval={billingInterval}
setBillingInterval={setBillingInterval}
paymentMethod={paymentMethod}
setPaymentMethod={handleSetPaymentMethod}
isPurchasing={isPurchasing}
purchasingTier={purchasingTier}
paymentConfirmed={paymentConfirmed}
upgradesDisabled={planLoading || !planKnown}
onUpgrade={handleUpgrade}
/>

<div className="flex flex-wrap gap-3">
<Button
type="button"
variant="primary"
variant="secondary"
size="md"
onClick={() => {
void openUrl(BILLING_DASHBOARD_URL);
}}>
onClick={() => void openUrl(BILLING_DASHBOARD_URL)}>
{t('settings.billing.openDashboard')}
</Button>
<Button type="button" variant="secondary" size="md" onClick={navigateBack}>
<Button type="button" variant="tertiary" size="md" onClick={navigateBack}>
{t('settings.billing.backToSettings')}
</Button>
</div>
Expand Down
4 changes: 3 additions & 1 deletion app/src/components/settings/panels/billing/SubscriptionPlans.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ interface SubscriptionPlansProps {
isPurchasing: boolean;
purchasingTier: PlanTier | null;
paymentConfirmed: boolean;
upgradesDisabled?: boolean;
onUpgrade: (tier: PlanTier) => void;
}

Expand All @@ -29,6 +30,7 @@ const SubscriptionPlans = ({
isPurchasing,
purchasingTier,
paymentConfirmed,
upgradesDisabled = false,
onUpgrade,
}: SubscriptionPlansProps) => {
const { t } = useT();
Expand Down Expand Up @@ -228,7 +230,7 @@ const SubscriptionPlans = ({
size="sm"
className="rounded-full"
onClick={() => onUpgrade(plan.tier)}
disabled={isPurchasing}>
disabled={isPurchasing || upgradesDisabled}>
{isThisPurchasing
? t('settings.billing.subscription.waiting')
: t('settings.billing.subscription.upgrade')}
Expand Down
Loading