Skip to content

feat(billing): prepaid AI token bank replaces the spending cap - #406

Open
pavlo-flamingo wants to merge 4 commits into
mainfrom
feat/ai-token-bank
Open

pavlo-flamingo wants to merge 4 commits into
mainfrom
feat/ai-token-bank

Conversation

@pavlo-flamingo

Copy link
Copy Markdown
Contributor

Problem

AI was sold as a spending cap over pay-as-you-go tokens (updateAiSpendCap, aiSpendUsd, aiTokensOverage). Product moved to a prepaid token bank — purchaseTokens, purchasedTokensRemaining, CheckoutInput.tokenAmountUsd — and the billing screens still drew the old model. On a trial, Activate Subscription opened the device-only Upgrade Plan modal, whose checkout sends no top-up and is refused once one is required.

Changes

  • Token bank. Manage AI Balance modal ($20 / $50 / $100 / custom → purchaseTokens invoice), Paid AI Tokens counter with the model-rates popover, low / empty / trial-exhausted alerts, an app-wide AI balance bar that is not dismissible and deep-links to the modal (?action=manageAiBalance). Spend-cap UI and hooks removed; payAsYouGoEnabled no longer forced for the AI product.
  • Paywall. AI Token Balance card; the first top-up rides on the checkout as tokenAmountUsd with a $10 floor (MIN_TOP_UP_USD). The button is never locked over a bad amount: the press shows the problem under the field and in a toast.
  • Trial. Activate Subscription opens the paywall form in a modal (ActivateSubscriptionModal, same createCheckoutSession); Manage AI Balance and Change Plan are dropped on a trial. The form is shared with the lock screen through usePlanCheckout / PlanCheckoutCards; UpgradePlanModal is update-only.
  • Auto top-up. Drawn per the mockups — checkbox, refresh mark on the counter, popover line — and locked behind AUTO_TOP_UP: the backend has no field or mutation for it yet.
  • From Fix Proceed to Payment dead ends on the subscription lock screen #274. Proceed to Payment is disabled while there is nothing to buy; every billing mutation toasts through getRelayErrorMessage, so the server's message shows instead of Relay's wrapper. The Relay-layer throw from Fix Proceed to Payment dead ends on the subscription lock screen #274 is not taken — it strips error.source, which that helper and 16 other call sites read.

Caveats

  • The $10 floor and the 1M-token "low" threshold are frontend constants; the schema names a configured minimum but does not expose it.
  • The backend still carries the cap (aiSpendCapUsd, updateAiSpendCap, aiSpendUsd, aiTokensOverage). Nothing here reads it, but a stored cap can still pause AI on a non-zero balance.
  • tsc, lint:ci, format and build are green. Checked against the code and the Figma frames, not on dev4.

Related: #274 (superseded by this PR)

The product drops the per-period AI spend cap and pay-as-you-go token
billing: AI runs on the period's free grant, then on a balance the tenant
tops up. The backend exposes it as purchaseTokens(amountUsd) -> invoice,
usage.purchasedTokensRemaining(+Usd) and CheckoutInput.tokenAmountUsd.

- Billing & Usage: "Manage AI Balance" header action and modal with
  $20 / $50 / $100 / Custom one-time top-ups. The Paid AI Tokens card shows
  the balance (tokens + USD) with the per-model rates popover. Low / empty /
  trial-exhausted alerts under the cards. No AI rate row, no "AI Usage
  Beyond Free Tokens" block, no plan block on a trial.
- Paywall: "AI Token Balance" card picks the first top-up ($50 by default),
  sent as tokenAmountUsd on checkout and added to the total due today.
- App-wide bar: AiBalanceBar (low / empty, not dismissible) replaces
  AiSpendLimitBar; its CTA deep-links to the modal (?action=manageAiBalance).
- Checkout inputs no longer force payAsYouGoEnabled on committed packages
  or on the AI product: the backend decides per product now.
- Removed: use-ai-spend-limit, ai-spend-limit-fields, ai-tokens-limit-modal,
  use-update-ai-spend-cap, ai-tokens-usage-card, lib/ai-spend-tone.

Not built, pending backend support: auto top-up (no API for it), tiered
token pricing in the tile estimates (entry rate only), the purchase
minimum. schema.graphql refreshed from bnk-tkn.dev4.
…ists

The mockups show auto top-up in three places: an "Enable Auto Top-up" checkbox in
Manage AI Balance, a refresh mark beside the Paid AI Tokens figure, and an
enabled/disabled line at the top of the model rates popover. The backend has no
field to store the choice and no mutation to set it, so none of it was drawn.

Draw all three, driven by one AUTO_TOP_UP status ({ available: false,
enabled: false }) in lib/auto-top-up.ts. The checkbox renders unchecked and
disabled with a "Coming soon" tag; the card mark renders only when enabled; the
popover line reads "Auto Top Up Disabled" on the billing page and is left out
on the paywall, where there is no balance yet. When the subscription grows the
field, the constant becomes a read of it and the surfaces stay as they are.
…op-up at $10

On a trial the billing page offered Manage AI Balance beside Activate
Subscription, and Activate opened the Upgrade Plan modal — a device-only
checkout that sends no tokenAmountUsd, which a checkout requiring a first
top-up refuses. A trial has no balance to manage either.

Activate Subscription now opens the paywall's own form in a modal
(ActivateSubscriptionModal): device plan, AI Token Balance card with the first
top-up, one Proceed to Payment on createCheckoutSession — the same purchase the
lock screen makes, made early. The form (query, choices, what the button
sends) moves into usePlanCheckout and PlanCheckoutCards, shared by the lock
screen; UpgradePlanModal becomes update-only and Manage AI Balance and Change
Plan are dropped on a trial.

The top-up gets a $10 floor (MIN_TOP_UP_USD — the schema names a configured
minimum but does not expose it). Neither surface locks its button over a bad
amount any more: the press reveals the problem under the fields and in a toast,
and the button stays pressable. The mockup's auto top-up checkbox joins the
paywall card, locked like everywhere else (AutoTopUpCheckbox).
…verywhere

Ports #274 as reviewed. Proceed to Payment looked live before the plan picker
reported anything and did nothing on click; it is disabled until there is
something to buy. The billing mutations (update, cancel, resume, customer
portal, test clock, seed) showed Relay's raw wrapper on failure; they now go
through getRelayErrorMessage like checkout and purchaseTokens already do, and
the weaker local extractGraphqlErrorMessage goes.

The Relay network-layer throw from #274 is deliberately not taken: it drops
error.source, which the shared helper and 16 other call sites read.
@github-actions

github-actions Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

🦩 Flamingo Code Review

8 finding(s) — 1 action required · 5 recommended · 2 informational

Mode: advisory · Rules cited: MULTIPLA-002-2 · 7 defect(s) outside any rule

Inline comments: 7 new

Findings without an inline anchor in this diff

  • 🔵 [info/informational] MULTIPLA-002-2 src/app/(app)/settings/billing-usage/hooks/extract-graphql-error-message.ts:1 — extract-graphql-error-message.ts removed in favor of shared handle-api-error helper — verify no other callers remain
    The local extract-graphql-error-message.ts file was deleted and all its consumers switched to getRelayErrorMessage from '@/lib/handle-api-error'. This is a good consolidation aligned with avoiding local re-implementations, but it's worth confirming (outside this diff) that no other file in the repo still imports the deleted './extract-graphql-error-message' module, which would now fail to build.
    export function extractGraphqlErrorMessage(err: unknown, fallback: string): string {
      if (!(err instanceof Error)) return fallback;
      const match = /got error\(s\):\s*([\s\S]+)/.exec(err.message);
      return (match?.[1] ?? err.message).trim() || fallback;
    }
    

Need another pass? Commits pushed after this review are not reviewed automatically.

  • Review the new commits — the commits added since this review
  • Review the whole diff again — ignoring what was already reviewed

Prefer typing? Comment @flamingo-review, or @flamingo-review full. To review every push on this pull request, add the flamingo-review-always label.

React 👍/👎 on inline comments to teach the reviewer.

Started 2026-09-15 11:23 UTC · updated 2026-09-15 11:24 UTC · workflow run

Comment on lines 37 to +39
const mutate = useCallback(
(input: CheckoutInput, options?: CreateCheckoutSessionOptions) => {
const tab = options?.target ?? openDeferredTab();
(input: CheckoutInput) => {
const tab = openDeferredTab();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🔴 [error/action_required] Removed target option breaks stripe tab reuse from AI paywall spend-cap callback flow

The CreateCheckoutSessionOptions.target option was removed along with its doc comment explaining that some callers (the AI spending-cap paywall flow) must pre-open a tab before calling mutate because the mutation is invoked from a callback where the user gesture is already gone. Now mutate always calls openDeferredTab() internally, which will attempt to open a new tab at mutate-call time. If any remaining caller in the codebase relied on passing a pre-opened target tab (the very reason this option existed, per the deleted doc comment), that caller's tab will silently be discarded/never navigated, and the internally-opened tab will be blocked by the popup blocker since it's not called directly from a user click. Verify no callers still rely on this now-removed capability before merging; if the AI top-up flow described in the deleted comment no longer needs it, that's fine — but this needs to be confirmed, not just implied by the hook's own doc comment change.

Evidence
  const mutate = useCallback(
    (input: CheckoutInput) => {
      const tab = openDeferredTab();

🤖 Prompt for AI agents
In src/app/(app)/settings/billing-usage/subscription/hooks/use-create-checkout-session.ts around lines 37-39, address this code-review finding: Removed target option breaks stripe tab reuse from AI paywall spend-cap callback flow.
The `CreateCheckoutSessionOptions.target` option was removed along with its doc comment explaining that some callers (the AI spending-cap paywall flow) must pre-open a tab before calling `mutate` because the mutation is invoked from a callback where the user gesture is already gone. Now `mutate` always calls `openDeferredTab()` internally, which will attempt to open a *new* tab at mutate-call time. If any remaining caller in the codebase relied on passing a pre-opened `target` tab (the very reason this option existed, per the deleted doc comment), that caller's tab will silently be discarded/never navigated, and the internally-opened tab will be blocked by the popup blocker since it's not called directly from a user click. Verify no callers still rely on this now-removed capability before merging; if the AI top-up flow described in the deleted comment no longer needs it, that's fine — but this needs to be confirmed, not just implied by the hook's own doc comment change.
The flagged code:
```
  const mutate = useCallback(
    (input: CheckoutInput) => {
      const tab = openDeferredTab();
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 45 — react 👍/👎 to teach the reviewer

Comment on lines +93 to +107
const handleCheckout = () => {
// Checkout has no diff to gate on, but an out-of-range quantity is still
// one: it would be sent as a plan nobody can be billed for. The same
// goes for a top-up with no figure behind it.
if (hasInvalidCustom) {
rejectInvalidAmount();
return;
}
const topUpProblem = validateTopUp?.() ?? null;
if (topUpProblem != null) {
rejectInvalidTopUp(topUpProblem);
return;
}
createCheckout.mutate({ products: checkoutProducts, tokenAmountUsd: tokenAmountUsd ?? undefined });
};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🟠 [warn/recommended] createCheckout.mutate no longer receives a { target: tab } opener, removing the deferred-tab open-on-click UX

The previous implementation opened a browser tab synchronously on click (openDeferredTab()) and passed it as { target: tab } to createCheckout.mutate so the Stripe redirect could reuse the user-gesture-opened tab and avoid popup blockers, cancelling it on failure. The new handleCheckout calls createCheckout.mutate({ products: checkoutProducts, tokenAmountUsd: tokenAmountUsd ?? undefined }) with no second argument and no tab handling at all. If useCreateCheckoutSession's onSuccess does something like window.location.href = url this is fine, but if it still expects a target tab (mirroring the old contract) or if it now opens a new tab itself post-fetch, that call will very likely be blocked by the browser's popup blocker because it fires asynchronously outside the original click's user-gesture context. This is a behavior regression that should be verified against use-create-checkout-session's current implementation.

Evidence
    const handleCheckout = () => {
      // Checkout has no diff to gate on, but an out-of-range quantity is still
      // one: it would be sent as a plan nobody can be billed for. The same
      // goes for a top-up with no figure behind it.
      if (hasInvalidCustom) {
        rejectInvalidAmount();
        return;
      }
      const topUpProblem = validateTopUp?.() ?? null;
      if (topUpProblem != null) {
        rejectInvalidTopUp(topUpProblem);
        return;
🤖 Prompt for AI agents
In src/app/(app)/settings/billing-usage/subscription/components/subscription-submit-button.tsx around lines 93-107, address this code-review finding: createCheckout.mutate no longer receives a { target: tab } opener, removing the deferred-tab open-on-click UX.
The previous implementation opened a browser tab synchronously on click (openDeferredTab()) and passed it as `{ target: tab }` to createCheckout.mutate so the Stripe redirect could reuse the user-gesture-opened tab and avoid popup blockers, cancelling it on failure. The new handleCheckout calls `createCheckout.mutate({ products: checkoutProducts, tokenAmountUsd: tokenAmountUsd ?? undefined })` with no second argument and no tab handling at all. If useCreateCheckoutSession's onSuccess does something like `window.location.href = url` this is fine, but if it still expects a `target` tab (mirroring the old contract) or if it now opens a new tab itself post-fetch, that call will very likely be blocked by the browser's popup blocker because it fires asynchronously outside the original click's user-gesture context. This is a behavior regression that should be verified against use-create-checkout-session's current implementation.
The flagged code:
```
    const handleCheckout = () => {
      // Checkout has no diff to gate on, but an out-of-range quantity is still
      // one: it would be sent as a plan nobody can be billed for. The same
      // goes for a top-up with no figure behind it.
      if (hasInvalidCustom) {
        rejectInvalidAmount();
        return;
      }
      const topUpProblem = validateTopUp?.() ?? null;
      if (topUpProblem != null) {
        rejectInvalidTopUp(topUpProblem);
        return;
      }
      createCheckout.mutate({ products: checkoutProducts, tokenAmountUsd: tokenAmountUsd ?? undefined });
    };
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 45 — react 👍/👎 to teach the reviewer

Comment on lines +199 to +206
const tone: AiBalanceTone =
subscription?.status === SubscriptionStatus.TRIAL
? 'default'
: aiBalanceTone({
freeTokens: Number(usage?.aiTokensFree ?? 0),
freeUsed: Number(usage?.aiTokensFreeUsed ?? 0),
purchasedRemaining: Number(usage?.purchasedTokensRemaining ?? 0),
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🟠 [warn/recommended] GraphQL Long fields cast with bare Number() without NaN guard

Number(usage?.aiTokensFree ?? 0), Number(usage?.aiTokensFreeUsed ?? 0), and Number(usage?.purchasedTokensRemaining ?? 0) convert GraphQL Long values (which the comment itself notes can arrive as a string or number depending on size) directly with Number(). If the backend ever returns a non-numeric string, an empty string, or a value that doesn't parse cleanly, Number() can produce NaN, which will propagate into aiBalanceTone's comparisons (freeUsed >= freeTokens, purchasedRemaining <= 0) and silently evaluate to false for every branch, causing incorrect (silently wrong, not crashing) tone determination. Given this directly drives whether the AI-paused banner is shown to users, a NaN guard or explicit parsing with fallback would be safer.

Evidence
  const tone: AiBalanceTone =
    subscription?.status === SubscriptionStatus.TRIAL
      ? 'default'
      : aiBalanceTone({
          freeTokens: Number(usage?.aiTokensFree ?? 0),
          freeUsed: Number(usage?.aiTokensFreeUsed ?? 0),
          purchasedRemaining: Number(usage?.purchasedTokensRemaining ?? 0),
        });
🤖 Prompt for AI agents
In src/app/components/billing-bars.tsx around lines 199-206, address this code-review finding: GraphQL Long fields cast with bare Number() without NaN guard.
`Number(usage?.aiTokensFree ?? 0)`, `Number(usage?.aiTokensFreeUsed ?? 0)`, and `Number(usage?.purchasedTokensRemaining ?? 0)` convert GraphQL `Long` values (which the comment itself notes can arrive as a string or number depending on size) directly with `Number()`. If the backend ever returns a non-numeric string, an empty string, or a value that doesn't parse cleanly, `Number()` can produce `NaN`, which will propagate into `aiBalanceTone`'s comparisons (`freeUsed >= freeTokens`, `purchasedRemaining <= 0`) and silently evaluate to `false` for every branch, causing incorrect (silently wrong, not crashing) tone determination. Given this directly drives whether the AI-paused banner is shown to users, a NaN guard or explicit parsing with fallback would be safer.
The flagged code:
```
  const tone: AiBalanceTone =
    subscription?.status === SubscriptionStatus.TRIAL
      ? 'default'
      : aiBalanceTone({
          freeTokens: Number(usage?.aiTokensFree ?? 0),
          freeUsed: Number(usage?.aiTokensFreeUsed ?? 0),
          purchasedRemaining: Number(usage?.purchasedTokensRemaining ?? 0),
        });
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 35 — react 👍/👎 to teach the reviewer

Comment on lines 77 to 84
<SubscriptionSubmitButton
needsCheckout={needsCheckout}
packageUpdates={selection?.updates.packageUpdates ?? []}
checkoutProducts={checkoutProducts}
hasInvalidCustom={selection != null && !selection.updates.valid}
needsCheckout={false}
packageUpdates={updates?.packageUpdates ?? []}
checkoutProducts={[]}
hasInvalidCustom={updates != null && !updates.valid}
onUpdated={onUpdated}
className="flex-1"
/>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🟠 [warn/recommended] checkoutProducts hardcoded to empty array removes previously-required AI pay-as-you-go activation payload

The removed PlanSelection.otherProducts logic ensured that every non-device product (notably AI_ASSISTANCE) was included as payAsYouGoEnabled: true in the checkout products list whenever a checkout-based plan change occurred, with an explicit comment warning that omitting them "would activate a subscription with the AI assistants switched off." The new code hardcodes needsCheckout={false} and checkoutProducts={[]} unconditionally. While the accompanying doc comment states this component is now update-only (never checkout), the SubscriptionSubmitButton still receives a needsCheckout prop, implying the button itself still branches on this flag elsewhere. If needsCheckout is ever true here in some future state (e.g., a bug in caller logic re-adds it, or the SubscriptionSubmitButton reads other state to decide), the checkoutProducts is now always empty, silently losing the AI activation-safety behavior that was explicitly called out as critical in the deleted comment. This is risky given the same file explicitly documented this exact production bug scenario previously.

Evidence
          <SubscriptionSubmitButton
            needsCheckout={false}
            packageUpdates={updates?.packageUpdates ?? []}
            checkoutProducts={[]}
            hasInvalidCustom={updates != null && !updates.valid}
            onUpdated={onUpdated}
            className="flex-1"
          />
🤖 Prompt for AI agents
In src/app/(app)/settings/billing-usage/components/upgrade-plan-modal.tsx around lines 77-84, address this code-review finding: checkoutProducts hardcoded to empty array removes previously-required AI pay-as-you-go activation payload.
The removed `PlanSelection.otherProducts` logic ensured that every non-device product (notably AI_ASSISTANCE) was included as `payAsYouGoEnabled: true` in the checkout products list whenever a checkout-based plan change occurred, with an explicit comment warning that omitting them "would activate a subscription with the AI assistants switched off." The new code hardcodes `needsCheckout={false}` and `checkoutProducts={[]}` unconditionally. While the accompanying doc comment states this component is now update-only (never checkout), the `SubscriptionSubmitButton` still receives a `needsCheckout` prop, implying the button itself still branches on this flag elsewhere. If `needsCheckout` is ever true here in some future state (e.g., a bug in caller logic re-adds it, or the SubscriptionSubmitButton reads other state to decide), the checkoutProducts is now always empty, silently losing the AI activation-safety behavior that was explicitly called out as critical in the deleted comment. This is risky given the same file explicitly documented this exact production bug scenario previously.
The flagged code:
```
          <SubscriptionSubmitButton
            needsCheckout={false}
            packageUpdates={updates?.packageUpdates ?? []}
            checkoutProducts={[]}
            hasInvalidCustom={updates != null && !updates.valid}
            onUpdated={onUpdated}
            className="flex-1"
          />
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 30 — react 👍/👎 to teach the reviewer

Comment on lines +41 to +45
{total && total.prepaid && (
<p>
Total due today: <span className="text-ods-text-primary text-h3">{formatCurrency(total.amount + topUp)}</span>
</p>
)}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🟠 [warn/recommended] PlanTotalSummary total-due-today calculation folds in topUp for prepaid plans even when topUpUsd represents an amount already excluded from the base total

For prepaid plans the new code computes formatCurrency(total.amount + topUp) as "Total due today". This assumes total.amount (from SelectionTotal, computed by the device pricing logic) never already includes any AI/top-up component. That assumption is plausible given the comment structure, but there is no shared type or contract enforcing that total.amount is device-only. If a future change to the pricing hook ever includes AI in the prepaid total (e.g., because the backend price preview starts summing all line items), this line will silently double count the top-up. Given this is new code introduced in this diff, it deserves an explicit safeguard or a stronger type marking total.amount as device-only.

Evidence
      {total && total.prepaid && (
        <p>
          Total due today: <span className="text-ods-text-primary text-h3">{formatCurrency(total.amount + topUp)}</span>
        </p>
      )}
🤖 Prompt for AI agents
In src/app/(app)/settings/billing-usage/subscription/components/plan-total-summary.tsx around lines 41-45, address this code-review finding: PlanTotalSummary total-due-today calculation folds in topUp for prepaid plans even when topUpUsd represents an amount already excluded from the base total.
For prepaid plans the new code computes `formatCurrency(total.amount + topUp)` as "Total due today". This assumes `total.amount` (from SelectionTotal, computed by the device pricing logic) never already includes any AI/top-up component. That assumption is plausible given the comment structure, but there is no shared type or contract enforcing that `total.amount` is device-only. If a future change to the pricing hook ever includes AI in the prepaid total (e.g., because the backend price preview starts summing all line items), this line will silently double count the top-up. Given this is new code introduced in this diff, it deserves an explicit safeguard or a stronger type marking `total.amount` as device-only.
The flagged code:
```
      {total && total.prepaid && (
        <p>
          Total due today: <span className="text-ods-text-primary text-h3">{formatCurrency(total.amount + topUp)}</span>
        </p>
      )}
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 25 — react 👍/👎 to teach the reviewer

Comment on lines +59 to +60
export function UpgradePlanModal({ isOpen, onClose, onUpdated }: UpgradePlanModalProps) {
const [updates, setUpdates] = useState<ProductUpdates | null>(null);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🟠 [warn/recommended] UpgradePlanModal now silently drops the previous 'needsCheckout' contract without adjusting call sites shown in this diff

The needsCheckout prop was removed from UpgradePlanModalProps and the component now hardcodes needsCheckout={false} to SubscriptionSubmitButton. The doc comment clarifies this is now an "update flow only" component and that trial/no-subscription flows use ActivateSubscriptionModal instead, which is a reasonable rename of responsibility. However, this diff does not include any change to the caller(s) that previously passed needsCheckout into UpgradePlanModal — if any caller elsewhere in the code still passes needsCheckout as a prop, TypeScript will simply drop the extra prop silently (JSX allows extra props to be a compile error only if using strict prop typing, but since it's now removed from the interface it will actually be a type error). This should be verified against all call sites of UpgradePlanModal to ensure they were updated in a paired commit.

Evidence
export function UpgradePlanModal({ isOpen, onClose, onUpdated }: UpgradePlanModalProps) {
  const [updates, setUpdates] = useState<ProductUpdates | null>(null);
🤖 Prompt for AI agents
In src/app/(app)/settings/billing-usage/components/upgrade-plan-modal.tsx around lines 59-60, address this code-review finding: UpgradePlanModal now silently drops the previous 'needsCheckout' contract without adjusting call sites shown in this diff.
The `needsCheckout` prop was removed from `UpgradePlanModalProps` and the component now hardcodes `needsCheckout={false}` to `SubscriptionSubmitButton`. The doc comment clarifies this is now an "update flow only" component and that trial/no-subscription flows use `ActivateSubscriptionModal` instead, which is a reasonable rename of responsibility. However, this diff does not include any change to the caller(s) that previously passed `needsCheckout` into `UpgradePlanModal` — if any caller elsewhere in the code still passes `needsCheckout` as a prop, TypeScript will simply drop the extra prop silently (JSX allows extra props to be a compile error only if using strict prop typing, but since it's now removed from the interface it will actually be a type error). This should be verified against all call sites of `UpgradePlanModal` to ensure they were updated in a paired commit.
The flagged code:
```
export function UpgradePlanModal({ isOpen, onClose, onUpdated }: UpgradePlanModalProps) {
  const [updates, setUpdates] = useState<ProductUpdates | null>(null);
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 20 — react 👍/👎 to teach the reviewer

// new status into the Relay store, which is what unlocks the app — and Billing
// & Usage is where the plan just bought is worth looking at.
const handleUpdated = useCallback(() => router.push(routes.settings.billingUsage), [router]);
const handleUpdated = useCallback(() => router.push(routes.settings.billingUsage()), [router]);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🦩 🔵 [info/informational] routes.settings.billingUsage changed from a static path to a function call without corresponding diff context

handleUpdated now calls router.push(routes.settings.billingUsage()) (with parentheses) whereas the removed line used routes.settings.billingUsage as a value. This implies a breaking change to the shared routes registry's shape (from a string constant to a function). Since this diff doesn't include the routes.ts change, verify that routes.settings.billingUsage was indeed converted to a function everywhere else it's referenced in the app, otherwise other call sites still treating it as a string will break at runtime (calling a string) or produce [object Object] push targets.

Evidence
  const handleUpdated = useCallback(() => router.push(routes.settings.billingUsage()), [router]);
🤖 Prompt for AI agents
In src/app/(app)/settings/billing-usage/subscription/components/subscription-settings-view.tsx around line 87, address this code-review finding: routes.settings.billingUsage changed from a static path to a function call without corresponding diff context.
handleUpdated now calls `router.push(routes.settings.billingUsage())` (with parentheses) whereas the removed line used `routes.settings.billingUsage` as a value. This implies a breaking change to the shared routes registry's shape (from a string constant to a function). Since this diff doesn't include the routes.ts change, verify that `routes.settings.billingUsage` was indeed converted to a function everywhere else it's referenced in the app, otherwise other call sites still treating it as a string will break at runtime (calling a string) or produce `[object Object]` push targets.
The flagged code:
```
  const handleUpdated = useCallback(() => router.push(routes.settings.billingUsage()), [router]);
```
Make the minimal change that resolves the finding; do not refactor unrelated code.

confidence: 30 — react 👍/👎 to teach the reviewer

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.

1 participant