feat(upgrade): looser paywall density + short CTA labels + USD→BRL checkout fix - #342
Conversation
… geo BFF Paywall polish (web + mobile): - Shorten the CTA labels so each fits one line at the column width, in both locales (en + pt-BR): convert.stayFree, convert.freeCta, convert.trialCta, plans.monthly.cta. - Loosen the web 3-column card layout (card padding 18->22, grid gap 12->16, price/sub/heroLine/features rhythm, sub/feature font 12.5->13, CTA top padding 16->20, header/notes spacing) so it reads premium. - Mirror the breathing room on the mobile plan cards (padding 18->20/20/22, name + feature gaps, +18 top margin above the card group). - Keep the navy-violet anchor, violet hero, ECONOMIZE badge, the paywall-checkout testid, and all pricing/plan logic unchanged. Billing fix (web-only): - Stripe checkout was charging USD even when the paywall showed BRL: the createCheckoutSession Server Action called orbit-api directly, bypassing the geo-forwarding BFF route, so no country header reached the backend. - Route handleCheckout through the existing /api/subscriptions/checkout BFF route (forwards x-vercel-ip-country / X-Orbit-Country-Code + handles auth and 401 refresh), matching how /plans already resolves currency. - Remove the now-unused createCheckoutSession action; keep openCustomerPortal. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review: PR #342
Scope: PR #342 in thomasluizon/orbit-ui-mobile — feat(upgrade): looser paywall density + short CTA labels + USD→BRL checkout fix
Recommendation: APPROVE
Summary
PR #342 delivers three things: shorter CTA labels on the paywall (both locales), looser spacing across the web and mobile upgrade components, and a root-cause fix for Brazilian users being charged USD instead of BRL. The currency fix removes the createCheckoutSession Server Action and replaces it with a direct client-side fetch to the existing dedicated BFF route (/api/subscriptions/checkout), which correctly forwards X-Forwarded-For and geo headers that the old Server Action bypassed. All i18n changes are in sync across both locales. No Critical or High findings survived the adversarial pass.
Findings
Critical
None
High
None
Medium
[MEDIUM] handleCheckout BFF-fetch exception not documented in apps/web/CLAUDE.md
· dimension: SOLID / clean architecture (#3) + Comment policy (#4)
· location: apps/web/CLAUDE.md (convention doc) / apps/web/app/(app)/upgrade/page.tsx:60
· issue: apps/web/CLAUDE.md lists exactly two sanctioned exceptions to the "all mutations through Server Actions" rule: cookie-setting auth flows and the SSE chat-stream. The checkout fetch() is a third exception: the dedicated BFF route handler is required because it receives a real NextRequest from which it can extract X-Forwarded-For, x-vercel-ip-country, and the timeZone query param for X-Orbit-Country-Code — none of which a Server Action can reliably supply. The exception is architecturally correct but undocumented.
· risk: A future developer sees "Never call the API from a client component" and refactors the fetch back to a Server Action, silently reintroducing the USD bug.
· fix: Add to the sanctioned-exceptions list in apps/web/CLAUDE.md: (3) POST /api/subscriptions/checkout — dedicated BFF route because it must forward X-Forwarded-For and x-vercel-ip-country from the real NextRequest for geo-based pricing; a Server Action cannot access these reliably.
· reference: CLAUDE.md rule 1 (root-cause, not workaround) + apps/web CLAUDE.md "Sanctioned exceptions" convention
Low / Info
None (signal gate applied).
Subagents
| Agent | Verdict |
|---|---|
| parity-checker | PAIRED — web spacing/padding tweaks mirrored in mobile; checkout flow change is web-only (mobile uses native Play Billing SDK, not Stripe checkout); no mobile mirror needed for the removed Server Action |
| i18n-syncer | IN SYNC — all 4 changed keys (upgrade.convert.trialCta, freeCta, stayFree, upgrade.plans.monthly.cta) present in both en.json and pt-BR.json; 610 keys each, zero orphans |
| contract-aligner | N/A — no packages/shared/src/types/*.ts schema fields added, renamed, or removed; no orbit-api changes in this PR |
| security-reviewer | N/A — no orbit-api code changed |
Validation
| Check | Result |
|---|---|
| Lint | N/A — toolchain not installed in review environment; PR author reports 0 errors; CI is the authoritative gate |
| Type check | N/A — same; PR author reports 0 errors |
| Tests | N/A — same; PR author reports web 1752 passing |
| Build (api) | N/A — orbit-api not touched |
Backward-compat guard
No fields were added, removed, or renamed in packages/shared/src/types/*.ts or any orbit-api DTO. The only packages/shared changes are string value updates in i18n/en.json and i18n/pt-BR.json — purely forward-compatible. No breaking risk to old mobile clients.
Deferred — N/A dimensions & files not verdicted
- Correctness (#1): Verified end-to-end — client passes
?timeZone=→ BFF reads it viarequest.nextUrl.searchParams.get('timeZone')→countryCodeFromTimeZonederivesX-Orbit-Country-Code: BR→ backend geo-routes to BRL. No gap. - Dead/stale code (#2):
createCheckoutSessionand itsCheckoutResponsetype fully removed; zero remaining references confirmed. - Comment policy (#4): The
/** BFF: POST ... */JSDoc inroute.tsis on an exported handler — permitted. No violations in changed files. - No-workaround (#5): This change IS the root-cause fix. The old workaround (Server Action bypassing geo) is deleted.
- Type safety (#6):
(await response.json()) as { url?: string }—response.json()returnsanyin the Fetch API types; the cast is idiomatic narrowing. Noas anyor escape hatches. - No console.log (#7): None in changed files.
- DESIGN.md / AI-slop (#8): Spacing adjustments use numeric literals only. No new colors, no raw
--slate-*, no hardcoded violet rgba, notransition-all, noh-screen. Font size bumps (12.5→13) stay within spec. - Contract drift + backward-compat (#11): N/A — no type schema or DTO changes.
- Security (#12):
fetchgoes to same-origin BFF, not orbit-api directly. Auth handled server-side byresolveServerSession()with 401-refresh retry. No credentials exposed client-side. - Backend hard rules (#13): N/A — orbit-api not touched.
What's good
- Root-cause fix, not a patch. The old Server Action bypassed the geo-BFF; instead of adding a header workaround, the PR deletes the Server Action entirely and routes through the purpose-built BFF route that already handles all geo forwarding.
- The dedicated checkout route is well-engineered.
resolveForwardedClientHeadershandles Vercel, Cloudflare, CloudFront, and Accept-Language as geo fallbacks — including timezone-to-country derivation for Brazilian timezones as a last resort. - The new test is behavioral, not implementation-bound. It asserts
fetchwas called with the BFF URL and POST method — exactly the contract that matters — without coupling to internal state. vi.unstubAllGlobals()inafterEach. Correct cleanup preventinglocationstub from leaking across tests.- i18n label changes are purposeful and atomic. Both locales updated in the same commit; shorter labels solve a real overflow problem.
Recommendation
Approve. The one Medium finding (undocumented sanctioned exception in apps/web/CLAUDE.md) is low-risk and can be addressed as a follow-up or squashed before merge. No blocking issues.
There was a problem hiding this comment.
PR Review - PR 342
Recommendation: APPROVE
| Severity | Count |
|---|---|
| Critical (incl. old-client breaks) | 0 |
| High | 0 |
| Medium | 1 |
| Low / Info | 0 |
Everything is clean. Parity is correct (mobile uses Play Billing natively - no mirror needed for the Server Action removal), i18n is in sync (610 keys in both locales, all 4 changed keys present and matching), no contract/DTO changes, and the backward-compat guard found nothing (only string values changed in i18n files).
Medium finding (non-blocking): The client-side fetch() to /api/subscriptions/checkout in apps/web/app/(app)/upgrade/page.tsx is architecturally correct - the dedicated BFF route handler is required because it needs the real NextRequest to extract X-Forwarded-For and x-vercel-ip-country for geo-based pricing (BRL vs. USD). A Server Action cannot reliably supply those headers. However, this exception is not documented in apps/web/CLAUDE.md sanctioned-exceptions list, which risks a future developer cleaning it up and reintroducing the USD bug. Suggested fix: add a third bullet noting that POST /api/subscriptions/checkout is a dedicated BFF route because it must forward geo headers from the real NextRequest for geo-based pricing. Non-blocking - the implementation is correct.
1 participant
Paywall polish + BRL checkout fix
Density + labels (both platforms)
paywall-checkouttestid all kept.Currency fix (web — the USD-at-Stripe bug)
Brazilian users were charged USD despite the paywall showing BRL. Root cause:
createCheckoutSession(Server Action) called orbit-api directly, bypassing the geo-forwarding BFF route, so no country header reached the backend → server geo → USD.handleCheckoutnow fetches the BFF route/api/subscriptions/checkout(forwardsx-vercel-ip-country/X-Orbit-Country-Code+ handles auth + 401 refresh), matching how/plansalready resolves BRL.createCheckoutSessionServer Action (keptopenCustomerPortal). New test asserts checkout POSTs to the geo-forwarding route.Validation
type-check + lint 6/6, 0 errors; tests shared 1241, mobile 725, web 1752.
🤖 Generated with Claude Code