Skip to content

feat: card on file credit bonus change - #449

Open
gianpaj wants to merge 12 commits into
mainfrom
feat/card-on-file-credit-bonus-change
Open

feat: card on file credit bonus change#449
gianpaj wants to merge 12 commits into
mainfrom
feat/card-on-file-credit-bonus-change

Conversation

@gianpaj

@gianpaj gianpaj commented Jul 2, 2026

Copy link
Copy Markdown
Owner

No description provided.

@bolt-new-by-stackblitz

Copy link
Copy Markdown

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

@vercel

vercel Bot commented Jul 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
sexyvoice Ready Ready Preview, Comment Jul 10, 2026 6:01am
sexyvoice-docs Ready Ready Preview, Comment Jul 10, 2026 6:01am

Request Review

@claude

claude Bot commented Jul 2, 2026

Copy link
Copy Markdown
Contributor

Claude finished @gianpaj's task in 4m 10s —— View job


PR Review: Card-on-file credit bonus

Reviewed against docs/plans/2026-07-02-card-on-file-credit-bonus-plan.md. This is a well-executed change — the plan is faithfully implemented, the three-layer idempotency model is sound, i18n is complete across all six locales (including the important marketing/FAQ/SEO copy accuracy fixes), and the test coverage is strong. A few correctness notes below, one of which I'd resolve before merge.

  • Read plan & migrations
  • Review Stripe webhook & actions
  • Review queries & UI
  • Review i18n & docs
  • Post review feedback

🟠 Should fix: partial-failure permanently burns the card and blocks recovery

insertCardBonusCreditTransaction (apps/web/lib/supabase/queries.ts:589) writes the card_bonus_claims row first, then grants credits:

  1. insert card_bonus_claims → on 23505, return without granting
  2. per-user check
  3. insert credit_transactions
  4. increment_user_credits RPC

If step 3 or 4 fails with a transient error after the claim row is committed, the state is unrecoverable:

  • The claim row now exists, so any retry — including the deliberately-added setup_intent.succeeded backup handler (route.ts:79) — hits 23505 at step 1 and returns before granting. The backup path can no longer do the one job it was added for.
  • Worse, the webhook POST handler swallows all processing errors and returns 200 received: true (route.ts:39-54), so Stripe never auto-retries either. The user permanently loses 9,000 credits and that physical card is globally burned.

This gap is specific to this PR: the topup/subscription paths dedupe purely on reference_id, so a redelivery re-attempts cleanly. Here the card_bonus_claims "claim-first" write defeats redelivery.

Suggested fix (most robust): make the claim insert + credit insert + balance increment atomic in a single SECURITY DEFINER Postgres function so they commit or roll back together. Lighter alternative: on the 23505 claim conflict, look up whether the card_bonus credit_transactions row for this user/setup_intent_id actually exists — if it doesn't, continue to grant instead of returning.

Fix this →

🟡 Minor: claim recorded even when the per-user check skips the grant

Because the claim insert precedes the per-user check (step 1 before step 2), a user who already claimed with card A but completes a second setup with card B gets card B's fingerprint written to card_bonus_claims while no bonus is granted (step 2 short-circuits). Card B is then globally burned without ever yielding a bonus — it could block a legitimate different user later. The UI/action hasClaimedCardBonus guards make this hard to reach in normal flow, but the backup webhook path doesn't share that guard. Moving the per-user check ahead of the claim insert (or the atomic-function approach above) closes this too.

🟡 Minor: dead try/catch around .single()

queries.ts:622-641supabase-js .single() returns { data: null, error } for zero rows rather than throwing, so the catch block never executes. Harmless, but the "Transaction doesn't exist, continue" comment is misleading. .maybeSingle() (or just reading data) expresses the intent more honestly.

🟢 Nits

  • 20260702120100_add_card_bonus_transaction_type.sql uses a bare ALTER TYPE credit_transaction_type … rather than the schema-qualified/DO-guarded pattern of the earlier ..._add_topup_transaction_type.sql. ADD VALUE IF NOT EXISTS is functionally fine and idempotent, so this is acceptable — just noting the inconsistency with CLAUDE.md's "fully qualified names" guidance.
  • Nice touch switching daily-stats/queries.ts to Database['public']['Enums']['credit_transaction_type'] instead of a hand-maintained union — it now auto-tracks new enum values.

What looks good

  • i18n: CTA/status/banner keys added in all six locales, and the previously-false "10,000 free credits" hero/FAQ/descriptionSignup/signup-title copy is correctly rewritten to the 1,000 + 9,000 split — this was flagged in the plan as a legal/accuracy requirement and it's handled thoroughly.
  • Idempotency design: fingerprint PK + per-user partial unique index + pre-checks is a solid layered model (modulo the recovery gap above); concurrent checkout.session.completed / setup_intent.succeeded double-fires are correctly deduped.
  • Metadata on both session and SetupIntent, matching the webhook's session-level and authoritative SetupIntent-level checks.
  • Eligibility correctly requires freemium amount === 1000, excludes payers (subscription credits use type: 'purchase', so they're covered) and prior claimants; shared computeCardBonusEligibility avoids an extra query in the dashboard layout.
  • Tests (card-bonus-credit-transaction.test.ts, webhook, stripe-actions) cover the fresh/duplicate-card/duplicate-user/index-conflict/rethrow paths and eligibility matrix.

Note: I reviewed statically and did not run pnpm type-check / pnpm test / pnpm fixall in this pass — worth confirming they're green before merge.
· feat/card-on-file-credit-bonus-change

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces a card-on-file credit bonus feature, lowering the initial signup grant to 1,000 credits and offering a one-time 9,000 credit bonus when a user adds a payment method. Key feedback includes resolving a hardcoded English locale in the Stripe setup session by dynamically passing the language parameter from the client, correcting an ineffective try/catch block in Supabase queries by replacing .single() with .maybeSingle(), and adjusting Tailwind CSS syntax for backward compatibility.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread apps/web/app/[lang]/actions/stripe.ts Outdated
Comment thread apps/web/app/[lang]/(dashboard)/dashboard/credits/card-bonus-cta.tsx Outdated
Comment thread apps/web/lib/supabase/queries.ts Outdated
@argos-ci

argos-ci Bot commented Jul 2, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Argos notifications ↗︎

Build Status Details Updated (UTC)
default (Inspect) ⚠️ Changes detected (Review) 3 changed Jul 10, 2026, 6:03 AM

@chatgpt-codex-connector chatgpt-codex-connector 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.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 3ad080f035

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread apps/web/app/[lang]/actions/stripe.ts Outdated
Comment on lines +597 to +603
const { error: claimError } = await supabase
.from('card_bonus_claims')
.insert({
fingerprint,
user_id: userId,
setup_intent_id: setupIntentId,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Check the user bonus before reserving a card fingerprint

When a user opens two setup sessions before either webhook completes and uses different cards, the second webhook inserts a card_bonus_claims row for the second fingerprint, then the later per-user card_bonus check returns without granting credits. That burns the second physical card globally even though it never unlocked a bonus, blocking another account from legitimately using it; check the existing user bonus before inserting the fingerprint claim, or make the claim/credit insert atomic so skipped grants roll back the claim.

Useful? React with 👍 / 👎.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Claude Code - Opus 4.8

Leaving this open for a human decision. It overlaps with the non-atomic claim/grant thread below, and the two suggest different orderings (check-user-first here vs. claim-first + atomicity there). A correct fix likely means making the fingerprint reservation, ledger insert, and balance increment atomic (a single SECURITY DEFINER Postgres function) rather than reordering — that is a money-path migration I did not want to land autonomously in this pass.

@pullfrog pullfrog 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.

Important

The card-bonus grant records the global per-card claim before granting credits, and the webhook always returns 200 (no Stripe retry). A transient failure between those two steps can leave a user who added a card without their 9,000 credits, unrecoverably. Details inline.

Reviewed changes — a freemium-model change: new signups get 1,000 credits (down from 10,000) and unlock the remaining 9,000 by adding a card on file (Stripe setup mode, no charge), granted once per user and once per physical card globally.

  • Lower signup grant + drop redundant triggerhandle_new_user() grants 1,000 instead of 10,000, and add_credits_trigger / add_credits_on_event() are dropped so the ledger stops double-counting the freemium row.
  • New card_bonus type + guards — adds the card_bonus enum value (own migration), a per-user partial unique index, and a card_bonus_claims table (fingerprint PK, RLS on, service-role only) for global per-card dedupe.
  • Grant logicinsertCardBonusCreditTransaction reserves the card claim, pre-checks the per-user row, inserts the ledger entry, then increments the balance; any 23505 is treated as already-claimed.
  • Webhookcheckout.session.completed (mode setup) and a backup setup_intent.succeeded both route to grantCardBonusForSetupIntent, which re-retrieves the SetupIntent with payment_method expanded, resolves the user, sets the default payment method, and grants.
  • Eligibility + UIcomputeCardBonusEligibility (not claimed, not paid, freemium amount === 1000) drives the credits-page CTA and a new non-dismissible cardBonusBanner (priority 1000); credit history renders card_bonus as a positive/green entry with localized labels.
  • Copy, terms, i18n — all six locales updated (marketing 10,000→1,000, FAQ, banner copy); terms.mdx / terms.es.mdx add an "Eligibility and Age Requirement" section framing the card as an adult/abuse control.
  • Tests — unit coverage for insertCardBonusCreditTransaction dedupe branches, computeCardBonusEligibility, the webhook setup-intent paths, and createCardBonusSetupSession.

ℹ️ Nitpicks

  • messages/*.json hero noCreditCard now ends in "No credit card required*⃰" — the trailing asterisk has no footnote/disclaimer rendered on the landing page (app/[lang]/page.tsx:182 just prints the string), and the *⃰ sequence includes a stray combining-asterisk glyph. Either add the referenced disclaimer or drop the marks (applies to all six locales).

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

fingerprint,
user_id: userId,
setup_intent_id: setupIntentId,
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

⚠️ The per-card claim is inserted before the credit grant, and this isn't atomic. If the credit_transactions insert or the increment_user_credits RPC below throws (e.g. a transient DB/RPC error), the claim row is already committed — and because the webhook POST handler swallows the error and always returns 200 (api/stripe/webhook/route.ts:39-54), Stripe never retries. On any later attempt (including the setup_intent.succeeded backup path), this card_bonus_claims insert now returns 23505 and short-circuits, so the user who added a card never receives their 9,000 credits, with no recovery and only a Sentry log.

The claim-first ordering is correct for abuse prevention (you must reserve the card globally before granting), so the fix isn't to reorder — it's to make reservation + grant recoverable.

Technical details
# Non-atomic per-card claim + no webhook retry can strand the bonus

## Affected sites
- `apps/web/lib/supabase/queries.ts``insertCardBonusCreditTransaction`: `card_bonus_claims` insert precedes the `credit_transactions` insert and `updateUserCredits` RPC; a throw after the claim insert leaves an orphaned claim.
- `apps/web/app/api/stripe/webhook/route.ts:39-54` — the top-level catch logs to Sentry and still returns `{ received: true }` (200), so Stripe does not retry the event.
- Backup path `grantCardBonusForSetupIntent` (`setup_intent.succeeded`) calls the same function, so it also hits the orphaned-claim `23505` and skips granting.

## Required outcome
- A transient failure during the grant must not permanently prevent the eligible user from receiving the 9,000-credit bonus.

## Suggested approach
- Perform the claim reservation, ledger insert, and balance increment in a single Postgres function (SECURITY DEFINER) so they commit or roll back together; or
- On the grant-failure path (non-23505 error from the `credit_transactions` insert or the RPC), delete the just-inserted `card_bonus_claims` row before rethrowing so a retry / the backup event can succeed; or
- Have the webhook return a non-2xx for these failures so Stripe retries — but that alone still needs the claim row cleaned up to be effective.

## Open questions for the human
- Is silent, unrecoverable loss of the bonus in this narrow window acceptable, or should this ship with atomicity before launch? The window is small (same DB, microseconds apart) but the failure is money-adjacent and self-heals nowhere.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Claude Code - Opus 4.8

Agreed this is the highest-value concern here, and leaving it open for a human call on your explicit open question (ship atomicity before launch vs. accept the narrow window). It needs a design decision + a Postgres function migration (reserve + ledger insert + increment committing/rolling back together), which is beyond a one-comment mechanical fix. Note: the .maybeSingle() change in 193c788 deliberately logs-and-continues on a read error instead of throwing, specifically to avoid widening this orphaned-claim window in the meantime.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

Heads-up for the still-open "ship atomicity before launch?" decision: commit 248107a (auto-grant the bonus on a new-regime user's first direct top-up/subscription) adds new callers of this same insertCardBonusCreditTransaction, so the non-atomic claim/grant window now also fires on the payment paths — not just the deliberate "add a card" setup flow. That widens the blast radius of this concern (more grants flowing through the same reserve-then-insert-then-increment sequence), but it doesn't change the nature of the fix: a single SECURITY DEFINER Postgres function committing the fingerprint reservation, ledger insert, and balance increment together would cover all callers at once. Flagging so the launch decision accounts for the expanded surface; deliberately not implementing the migration autonomously here.


Generated by Claude Code

gianpaj and others added 3 commits July 2, 2026 23:06
Retrieve lang via useParams in CardBonusCta and forward it to createCardBonusSetupSession so users return to their locale after Stripe setup instead of always /en.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion

Hiding the CTA is not a security boundary: any authenticated user could call createCardBonusSetupSession directly and mint a card_bonus setup session, letting paid or legacy 10,000-credit users claim the 9,000 bonus. Swap the weaker hasClaimedCardBonus check for isEligibleForCardBonus, which also enforces the non-paid and new-regime constraints server-side.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The Supabase client returns { data, error } instead of throwing, so the try/catch never caught anything and .single() logged a PGRST116 error on every first-time claim. Switch to .maybeSingle(), which returns data: null with no error when no row matches, and log-and-continue on a genuine read error so a transient failure doesn't strand the grant (the partial unique index on the insert is the authoritative dedup guard).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@pullfrog pullfrog 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.

ℹ️ No new critical issues — the three fix commits are sound. One minor dead-code note inline.

Reviewed changes — the delta since the prior pullfrog review (3ad080f): three follow-up fixes to the card-on-file bonus setup path.

  • Dynamic locale in setup sessioncreateCardBonusSetupSession now accepts { lang } from the client (useParams().lang) instead of hardcoding 'en', so the Stripe success/cancel redirects land on the user's active locale.
  • Server-side eligibility enforcement — the action's guard was upgraded from hasClaimedCardBonus to isEligibleForCardBonus, re-checking not-claimed AND not-paid AND new-regime (1000 freemium) before minting a setup session. This closes the gap where any authenticated user could invoke the action directly.
  • Robust dedup lookupinsertCardBonusCreditTransaction swapped .single() + try/catch for .maybeSingle(), logging-and-continuing on a genuine read error rather than throwing, so a transient read failure doesn't strand the grant (the partial unique index remains the authoritative guard). Also resolves the earlier ineffective-try/catch note.

The headline atomicity concern from the prior review (non-atomic claim-before-grant + webhook always returning 200) remains open by design — the author is leaving it for a human call on whether to ship an atomic RPC before launch. The .maybeSingle() change deliberately avoids widening that window in the meantime.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix all ➔Fix 👍s ➔View workflow run | Using Claude Opus𝕏

Comment thread apps/web/lib/supabase/queries.ts Outdated
The server action now guards with isEligibleForCardBonus (which already covers the not-claimed check), leaving hasClaimedCardBonus with no callers. Remove it and update the implementation plan to reference isEligibleForCardBonus as the CTA/banner and server-action guard.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
New-regime users who pay directly (a top-up or an initial subscription)
now receive the same one-time 9,000-credit card-on-file bonus as users
who take the "add a card" CTA, so both paths reach 10,000 credits.

Previously the bonus was only granted via the mode:'setup' Checkout flow;
worse, once a user paid, computeCardBonusEligibility flipped hasPaid=true
and hid the CTA, permanently disqualifying them from the bonus.

- Add maybeGrantCardBonusFromPayment() in the Stripe webhook, called after
  crediting a topup and an initial subscription. It reuses the payment's
  card fingerprint so the existing global one-card-one-bonus dedupe
  (card_bonus_claims) and per-user uniqueness guard apply. Best-effort:
  it never throws, so a bonus hiccup can't fail the purchase crediting or
  trigger Stripe retries. Non-card methods without a fingerprint are
  skipped rather than granted unguarded.
- Add isEligibleForCardBonusOnPayment()/computeCardBonusOnPaymentEligibility():
  requires a new-regime user who hasn't already claimed, but — unlike the
  CTA — does NOT treat paying as a disqualifier. Runs from the webhook via
  the admin client (no user session).
- Extract shared hasCardBonusTransaction()/isNewRegimeUser() predicates.
- Document the behavior in docs/devops.md and add unit + webhook tests.

Tests: pnpm type-check (green); card-bonus-credit-transaction.test.ts
(18/18). The stripe-webhook integration test requires redis-memory-server,
whose binary download is blocked in this sandbox, so it was not run here.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CeGPfVo7CoSfaMhUqroiwP
@cursor

cursor Bot commented Jul 4, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@pullfrog pullfrog 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.

ℹ️ No new issues. The payment auto-grant reuses the already-guarded grant flow; the one carried-forward risk (claim/grant atomicity) is already tracked on the open thread and acknowledged there.

Reviewed changes — the delta since the prior pullfrog review (1cf7a82): commit 248107a auto-grants the 9,000 card-on-file bonus on a new-regime user's first direct payment.

  • Auto-grant on first direct payment — a new maybeGrantCardBonusFromPayment fires after a top-up and after an initial subscription grant, so paying customers reach the same 10,000-credit total as users who take the "add a card" CTA. It reuses the payment's card fingerprint through insertCardBonusCreditTransaction, so the same global one-card-one-bonus dedupe and per-user uniqueness guards apply.
  • Best-effort, non-card safe — the grant is wrapped so it never throws (a failure logs to Sentry but can't fail or retrigger the purchase crediting), and a payment method with no card fingerprint (e.g. some wallets) is skipped rather than granted unguarded.
  • On-payment eligibility — new isEligibleForCardBonusOnPayment (admin client, since the webhook has no session) plus a pure computeCardBonusOnPaymentEligibility require a new-regime user (1,000 freemium row) who hasn't already been granted the bonus; unlike the CTA path, a prior payment does not disqualify because paying is the trigger.
  • Webhook refactorhandleCheckoutSessionCompleted was split into handleTopupCheckoutSession, handleCardBonusSetupCheckoutSession, and grantCardBonusForSetupIntent; the mode branching is behavior-preserving. computeCardBonusEligibility was refactored to reuse extracted hasCardBonusTransaction / isNewRegimeUser helpers with identical logic.
  • Tests + docs — webhook tests cover eligible-grant, ineligible-skip, and no-fingerprint-skip for both top-up and subscription; computeCardBonusOnPaymentEligibility gets its own unit suite; docs/devops.md documents the payment path.

The reference_id reuse on the payment path is safe: the card_bonus row reuses the same paymentIntentId already consumed by the top-up/subscription transaction, but unique_reference_id_idx is partial to type IN ('purchase','topup'), so no 23505 collision — only the per-user credit_transactions_one_card_bonus_per_user index can fire, matching the inline comment.

The pre-existing non-atomic claim-before-grant concern now also flows through the payment paths, but that is already captured on the open thread and acknowledged, so it is not re-raised here.

Pullfrog  | ⚠️ this action is pinned to a commit SHA, which freezes the cleanup step — switch to @v0 or keep the SHA fresh with Dependabot | Fix it ➔View workflow run | Using Claude Opus𝕏

Resolve types.d.ts conflict: keep main's new call_session_analysis and
call_session_analytics table types alongside this branch's
card_bonus_claims type (adjacent additions, no semantic overlap).
docs/devops.md auto-merged, preserving both the new call-analysis env
vars and this branch's card-on-file payment-path documentation.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01CeGPfVo7CoSfaMhUqroiwP
@cursor

cursor Bot commented Jul 4, 2026

Copy link
Copy Markdown

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.

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