feat(payment): optional Stripe Checkout for USD top-up - #355
Conversation
Introduce Stripe Checkout sessions for freeform USD top-ups, webhook credit with idempotent payment_orders, config/admin options, and modern TopUp UI. Disabled when STRIPE_SECRET_KEY is unset.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Stripe configuration, checkout and webhook handlers, payment-order persistence, API routes, database migration, and a redesigned top-up page with Stripe payments and activity history. ChangesStripe top-up billing
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant TopUpPage
participant CreateStripeCheckout
participant Stripe
participant StripeWebhook
participant PaymentOrder
participant UserQuota
TopUpPage->>CreateStripeCheckout: Submit amount_usd and request_id
CreateStripeCheckout->>PaymentOrder: Store pending order
CreateStripeCheckout->>Stripe: Create Checkout Session
Stripe-->>CreateStripeCheckout: Return session URL and ID
CreateStripeCheckout-->>TopUpPage: Return checkout URL
Stripe->>StripeWebhook: Send signed payment event
StripeWebhook->>PaymentOrder: Claim and settle payment order
PaymentOrder->>UserQuota: Increment quota
StripeWebhook-->>Stripe: Return received acknowledgement
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements Stripe payment integration for quota top-ups, introducing new backend endpoints, database models to track payment orders, and a redesigned frontend billing page with payment history. The review feedback highlights critical issues that need to be addressed: a race condition in the webhook handler that could cause permanent quota loss (requiring the quota increase to be transactional with the order status update), concurrent data races on the global Stripe key (requiring a thread-safe Stripe client), and a potential DoS vulnerability on the webhook endpoint (requiring an io.LimitReader). Additionally, the feedback points out UI text contradictions regarding the minimum top-up amount and suggests allowing decimal inputs.
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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (3)
controller/payment_stripe.go (2)
152-154: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing parameter and return value descriptions to the function comment.
As per coding guidelines, every function must have a comment that starts with the function name and describes its purpose, parameters, and return values in complete sentences.
📝 Proposed comment
// StripeWebhook handles checkout.session.completed events. The route MUST receive // the raw request body — do not interpose middleware that consumes it. +// It takes a gin.Context to parse the Stripe webhook payload and writes a JSON status response. func StripeWebhook(c *gin.Context) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/payment_stripe.go` around lines 152 - 154, Update the StripeWebhook function comment to start with “StripeWebhook” and describe the handler’s purpose, its gin.Context parameter, and its return behavior in complete sentences, while preserving the raw-request-body requirement.Source: Coding guidelines
32-34: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing parameter and return value descriptions to the function comment.
As per coding guidelines, every function must have a comment that starts with the function name and describes its purpose, parameters, and return values in complete sentences.
📝 Proposed comment
// CreateStripeCheckout creates a Stripe Checkout Session for a freeform USD top-up. // The fee is absorbed by the platform: the user is charged exactly AmountUSD. +// It takes a gin.Context to read the request and writes a JSON response containing the checkout URL. func CreateStripeCheckout(c *gin.Context) {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/payment_stripe.go` around lines 32 - 34, Update the CreateStripeCheckout comment to retain its purpose description and add complete-sentence descriptions of the gin.Context parameter and the function’s return behavior, including that it writes the checkout response through the context rather than returning a value.Source: Coding guidelines
model/payment_order.go (1)
41-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd missing function comments.
As per coding guidelines, every function must have a comment that starts with the function name and describes its purpose, parameters, and return values in complete sentences.
CreatePaymentOrderandGetPaymentOrderBySessionare missing these comments.📝 Proposed comments
+// CreatePaymentOrder persists a new payment order to the database. +// It takes a context for cancellation and the order to create, returning an error if the operation fails. func CreatePaymentOrder(ctx context.Context, order *PaymentOrder) error { if err := DB.WithContext(ctx).Create(order).Error; err != nil { return errors.Wrap(err, "create payment order") } return nil } +// GetPaymentOrderBySession retrieves a payment order by its session ID. +// It takes a context and the sessionID string, returning the matched PaymentOrder or nil if not found, along with any error encountered. func GetPaymentOrderBySession(ctx context.Context, sessionID string) (*PaymentOrder, error) { var order PaymentOrder err := DB.WithContext(ctx).Where("session_id = ?", sessionID).First(&order).Error🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/payment_order.go` around lines 41 - 58, Add GoDoc comments immediately above CreatePaymentOrder and GetPaymentOrderBySession, starting each comment with its function name and describing the function’s purpose, parameters, and return values in complete sentences.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/payment_stripe.go`:
- Around line 201-216: Make the payment transition and quota credit atomic by
adding a model-layer operation that marks the order paid and increments the user
quota within one database transaction. Update the webhook flow around
MarkPaymentOrderPaid and IncreaseUserQuota to use this atomic operation,
preserving idempotent handling of already-processed payments and returning
errors only when the combined transaction fails.
In `@web/modern/src/pages/topup/TopUpPage.tsx`:
- Around line 30-40: Add comments beginning with each symbol’s name for
readQuotaPerUnit, formatUSD, TopUpPage, all loaders and handlers, and every
derived helper in the affected section. Each comment must describe the
function’s purpose, parameters, and return value in complete sentences,
following the project’s documentation guidelines.
- Around line 106-107: Update the catch blocks in TopUpPage’s user-data and
top-up request flows to avoid passing raw error objects to console.error. Log
only a sanitized string derived from the error name or message, with a safe
fallback for non-Error values, and apply this consistently to all referenced
catch blocks.
- Around line 509-545: Convert the history table in the TopUpPage component to
the repository’s responsive table pattern by using useResponsive,
ResponsivePageContainer, and AdaptiveGrid as appropriate. Add mobile data-label
attributes to each history cell using the existing translated Date, Source, and
Amount labels, while preserving the current history mapping, formatting, and
styling.
- Around line 113-122: Update loadSystemStatus and the checkout card rendering
in TopUpPage to consume an explicit Stripe capability from the parsed system
status, such as a stripe-enabled flag. Store that capability in component state
and render the Stripe checkout UI only when it is enabled; preserve the existing
top-up link behavior for enabled installations and hide the payment action when
Stripe is disabled or the capability is absent.
- Around line 26-28: Replace the hardcoded MIN_TOPUP_USD and related $20/$5
values in TopUpPage with the effective minimum from system status. Use that
configured minimum consistently for amount validation, preset/input constraints,
and all user-facing minimum descriptions so checkout and page behavior remain
aligned.
- Around line 35-37: Update formatUSD and the related date-formatting logic to
use the active i18n.language locale instead of hardcoded en-US values, while
retaining USD as the currency. Obtain the locale from the page’s existing i18n
context and pass it to both Intl.NumberFormat and Intl.DateTimeFormat so all
displayed currency and dates follow the selected application language.
- Around line 317-325: Update the affected icon-only controls in TopUpPage,
including both buttons using loadUserData and RefreshCw, to provide translated
aria-label text and a data-label for mobile layouts. Ensure each control has a
minimum 44px mobile tap target while preserving its existing sizing and behavior
on larger screens.
---
Nitpick comments:
In `@controller/payment_stripe.go`:
- Around line 152-154: Update the StripeWebhook function comment to start with
“StripeWebhook” and describe the handler’s purpose, its gin.Context parameter,
and its return behavior in complete sentences, while preserving the
raw-request-body requirement.
- Around line 32-34: Update the CreateStripeCheckout comment to retain its
purpose description and add complete-sentence descriptions of the gin.Context
parameter and the function’s return behavior, including that it writes the
checkout response through the context rather than returning a value.
In `@model/payment_order.go`:
- Around line 41-58: Add GoDoc comments immediately above CreatePaymentOrder and
GetPaymentOrderBySession, starting each comment with its function name and
describing the function’s purpose, parameters, and return values in complete
sentences.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3cbce84a-eb98-4adc-bdd8-c5548c1a65ae
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
common/config/config.gocontroller/payment_stripe.gogo.modmodel/main.gomodel/option.gomodel/payment_order.gomodel/payment_order_test.gorouter/api.goweb/modern/src/pages/topup/TopUpPage.tsx
Address PR review: credit quota in the same transaction as marking paid, use stripe client.New instead of package-level Key, expose stripe_enabled and min_topup_usd on /api/status, and harden Modern TopUpPage (dynamic min presets, hide Stripe when disabled, string-only logs, a11y/responsive).
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controller/misc.go`:
- Around line 55-56: Update the stripe_enabled value in the configuration map to
require both the trimmed StripeSecretKey and StripeWebhookSecret to be present,
keeping the top-up UI disabled unless checkout settlement is fully configured.
In `@model/payment_order.go`:
- Around line 123-128: Update the payment settlement update in the transaction
to condition the query on the order still having PaymentStatusPending, then
inspect RowsAffected and abort without crediting quota when no row was updated.
Preserve the existing error handling for database failures and ensure only the
successful conditional update proceeds to settlement.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5bff773-5fdb-45c6-a672-2b4b807af674
📒 Files selected for processing (4)
controller/misc.gocontroller/payment_stripe.gomodel/payment_order.goweb/modern/src/pages/topup/TopUpPage.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- controller/payment_stripe.go
- web/modern/src/pages/topup/TopUpPage.tsx
- Optimistic pending→paid claim (RowsAffected) to prevent double settle - Limit Stripe webhook body to 1 MiB - Gate stripe_enabled on both secret and webhook secret - Allow cent precision (step 0.01) on top-up amount input
|
Addressed review feedback in Already fixed earlier (
Follow-up (
Resolving the review threads accordingly. |
|
Thanks for this PR. The overall direction is good, and the current code already has several important protections: signed webhook verification, a request-body size limit, and an atomic However, I do not think this is ready to merge into production yet. Please address the following issues. 1. The Stripe secret key can be returned to the browser
Suggested fix:
Stripe secret keys must stay on the server: Stripe API keys. 2. Checkout can still be created when webhook settlement is not readyThe UI reports Stripe as enabled only when both secrets exist, but Suggested fix: Create one shared
Also add a dedicated 3. The success and cancel URLs currently go to the 404 pageThe backend redirects to Suggested fix: Use query parameters on the existing route, for example: Or register both child routes explicitly. The success page should not trust the query string alone. It should call an authenticated order-status endpoint and show 4. Settlement needs stronger database checksThe conditional
Suggested fix:
The webhook should also verify the stored order against Stripe data: session ID, amount, currency, mode, live/test mode, and a stable local order ID in metadata. 5. Checkout creation is not idempotentThe code creates the Stripe Session first and saves the local order afterwards. A timeout, retry, or local database failure can create duplicate or orphaned Stripe Sessions. Suggested fix:
See Stripe idempotent requests and Stripe webhooks. 6. The payment history can show incorrect amountsThe UI reads generic top-up logs and converts quota to USD using the current
Also, the top-up log is written after the settlement transaction and its error is ignored, so a successful payment may be missing from history. Suggested fix: Add a payment-order history endpoint that returns the original immutable fields:
Use structured 7. Important payment lifecycle events are missingThe code handles successful Checkout events only. It does not close or reconcile orders for:
Unknown sessions currently become an error inside Suggested fix:
8. Tests and CI need to cover the money pathThe new tests only check a constant and the table name. They do not test payment safety. CI is also currently failing, including the modern frontend test job and the integration test workflow. Please add at least these tests:
Recommended merge orderPlease fix items 1–6 and make CI green before merging. Before enabling live Stripe keys, I also recommend completing the lifecycle/reconciliation work in item 7 and adding the money-path tests above. I am happy with the general feature direction, but these changes are important to prevent secret leakage, duplicate charges/sessions, paid-without-credit cases, and incorrect payment history. |
Address Laisky PR feedback for production-safe top-up: - Hide Stripe secrets from OptionMap/GetOptions (Key/APIKey sensitive) - StripeReady gate: secret + webhook + trusted public base URL - STRIPE_PUBLIC_BASE_URL; no request Host for live return URLs - Local order + request_id before Checkout; Stripe Idempotency-Key - Success/cancel via /topup?stripe= query; order status poll API - Settlement: reload on empty claim, require user RowsAffected, amount/currency check, event id dedupe, unknown session 2xx - Payment order history with immutable amount_cents - Unit tests for settle idempotency, amount mismatch, missing user
|
Thanks for the detailed review. Implemented the merge-blocker items in 1. Secrets not returned to browser
2. Checkout blocked unless settlement-ready
3. Success / cancel URLs
4. Stronger settlement
5. Checkout creation idempotency
6. Payment history
Also
CI / modern TopUp tests updated with |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
web/modern/src/pages/topup/TopUpPage.tsx (1)
621-672: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUntranslated status text.
Unlike
sourceLabel,entry.status(e.g.pending,paid,manual_review,failed) is rendered directly in both the mobile card view and the desktop table cell without going throughtr(), bypassing i18n for this UI content.As per coding guidelines, "All UI content must support internationalization."
♻️ Proposed fix
+ const statusLabel = (s: string): string => { + switch (s) { + case 'paid': + return tr('history.status.paid', 'Paid'); + case 'pending': + return tr('history.status.pending', 'Pending'); + case 'manual_review': + return tr('history.status.manual_review', 'Under review'); + case 'failed': + return tr('history.status.failed', 'Failed'); + default: + return s; + } + };Then use
statusLabel(entry.status)in place of the rawentry.statusin both the mobile and desktop renderings.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/modern/src/pages/topup/TopUpPage.tsx` around lines 621 - 672, Update the history status rendering in the mobile cards and desktop table to use the existing statusLabel helper instead of rendering entry.status directly. Ensure both views pass each entry’s status through statusLabel while preserving the surrounding layout and data-label behavior.Source: Coding guidelines
controller/payment_stripe.go (2)
74-220: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIgnored error from
GetUserEmail.
userEmail, _ := model.GetUserEmail(userID)at line 164 drops the error entirely — it's neither logged nor returned, so a DB failure here is invisible. As per coding guidelines, "**/*.{go,ts,tsx,js,jsx,py,java}: Handle errors as close to their source as possible; never ignore errors" and "Always return or log errors; do not silently ignore failures."🩹 Proposed fix
- userEmail, _ := model.GetUserEmail(userID) + userEmail, emailErr := model.GetUserEmail(userID) + if emailErr != nil { + logger.Warn("lookup user email for stripe receipt", zap.Error(emailErr), zap.Int("user_id", userID)) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/payment_stripe.go` around lines 74 - 220, Handle the error returned by model.GetUserEmail in CreateStripeCheckout instead of discarding it: capture the error, log it with logger.Error, and return the existing failure response before continuing when retrieval fails. Preserve the current trimming and optional-email behavior for successful lookups.Source: Coding guidelines
257-304: 🗄️ Data Integrity & Integration | 🔴 Critical | 🏗️ Heavy liftEvent claimed before processing breaks Stripe retry semantics for transient failures.
model.ClaimStripeWebhookEventinserts the dedup row beforehandleCheckoutPaid/handleCheckoutTerminalrun. If the dispatched handler later fails (e.g. a transient DB error insideSettlePaidPaymentOrder, or an amount/currency mismatch), the handler returns a 5xx and Stripe retries with the same event id — but the retry hits!claimedand short-circuits to{"duplicate": true}with 200 OK, so settlement is never attempted again. The order is leftpendingforever even though Stripe has been paid, and Stripe stops retrying because it received a 200.
SettlePaidPaymentOrderalready provides its own idempotency via thepending → paidstatus guard, so the event-level claim is not required to prevent double-crediting on legitimate concurrent duplicate deliveries — it only needs to prevent redundant side effects (audit log, cache refresh) on successful runs. Recommend claiming (or finalizing) the event only after the handler completes successfully, so a failed attempt can be retried by Stripe.// Sketch: only persist the dedup row once processing has actually succeeded. switch event.Type { case "checkout.session.completed", "checkout.session.async_payment_succeeded": if !handleCheckoutPaid(c, event) { return // handler already wrote its own error response; event NOT claimed, safe to retry } case "checkout.session.expired", "checkout.session.async_payment_failed": if !handleCheckoutTerminal(c, event) { return } default: } if _, err := model.ClaimStripeWebhookEvent(ctx, event.ID, string(event.Type)); err != nil { logger.Warn("record processed stripe event", zap.Error(err), zap.String("event_id", event.ID)) } c.JSON(http.StatusOK, gin.H{"received": true})(Handlers would need to return a
bool/error instead of writing their own terminal JSON in all paths, or the dispatch loop needs restructuring to know success vs failure before claiming.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/payment_stripe.go` around lines 257 - 304, Move Stripe event deduplication in StripeWebhook until after the dispatched handler completes successfully, so failed handleCheckoutPaid and handleCheckoutTerminal attempts leave the event unclaimed for retry. Update those handlers or the dispatch flow to return/propagate success before recording the event with ClaimStripeWebhookEvent; preserve duplicate short-circuiting for already successfully processed events and keep existing error responses.model/payment_order.go (1)
37-50: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
RequestIDunique index is global, but lookups are scoped per-user.
RequestIDcarries a single-columnuniqueIndex, yet it is client-supplied (createStripeCheckoutRequest.RequestIDincontroller/payment_stripe.go) and every lookup (GetPaymentOrderByRequestID) filters byuser_id AND request_id. If two different users happen to submit the samerequest_idvalue,CreatePaymentOrderfails with a global unique-constraint violation for the second user, and the idempotent-retry fallback (GetPaymentOrderByRequestID(ctx, userID, requestID)) returnsnilbecause the existing row belongs to a different user — soCreateStripeCheckoutsurfaces a generic failure to a legitimate user instead of creating their order. The uniqueness should be scoped to(user_id, request_id)rather than global.🔧 Proposed fix: scope the unique index per user
type PaymentOrder struct { Id int `json:"id" gorm:"primaryKey"` - UserId int `json:"user_id" gorm:"index"` + UserId int `json:"user_id" gorm:"uniqueIndex:idx_payment_orders_user_request,priority:1"` Provider string `json:"provider" gorm:"type:varchar(32);index"` - RequestID string `json:"request_id" gorm:"type:varchar(64);uniqueIndex"` + RequestID string `json:"request_id" gorm:"type:varchar(64);uniqueIndex:idx_payment_orders_user_request,priority:2"` SessionID string `json:"session_id" gorm:"type:varchar(191);uniqueIndex"`🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/payment_order.go` around lines 37 - 50, Update the PaymentOrder model’s RequestID uniqueness to be scoped per user: remove the standalone uniqueIndex from RequestID and define a shared composite unique index on UserId and RequestID. Preserve the existing GetPaymentOrderByRequestID user-scoped lookup and ensure the ORM migration creates the composite constraint instead of the global one.
🧹 Nitpick comments (5)
web/modern/src/pages/topup/TopUpPage.tsx (1)
463-483: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd
aria-pressedto preset chips.Active state is only conveyed visually via className; screen-reader users get no indication which preset (if any) is currently selected.
♻️ Proposed fix
<button type="button" key={amount} onClick={() => setPreset(amount)} data-label={`$${amount}`} + aria-pressed={active} className={cn(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/modern/src/pages/topup/TopUpPage.tsx` around lines 463 - 483, Add an aria-pressed attribute to each preset button rendered in the presets.map callback, using the existing active value so screen readers receive the same selected-state information conveyed by the className.web/modern/src/pages/topup/__tests__/TopUpPage.amount.test.tsx (1)
40-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAmount-specific coverage was dropped from
TopUpPage.amount.test.tsx.The file retains its "amount" name, but the two remaining tests only check router-mount safety and the
stripe=cancelbanner. No test exercisesamount_usdmin/max validation, preset-chip selection, or the Checkout redirect fromonStripeSubmit— previously the presumed purpose of this suite. Given the PR's stated goal of improving payment-safety test coverage, consider restoring amount-boundary and submit-flow tests using the mocks already scaffolded here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/modern/src/pages/topup/__tests__/TopUpPage.amount.test.tsx` around lines 40 - 100, Restore amount-specific coverage in the TopUpPage test suite: add tests for amount_usd minimum and maximum validation, preset-chip selection, and the Checkout redirect triggered by onStripeSubmit. Reuse the existing beforeEach API/localStorage mocks and renderPage helper, while preserving the current router-safety and stripe=cancel tests.model/payment_order.go (2)
220-309: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAmount/currency mismatch leaves the order permanently
pendingwith no operator visibility.When
expectedAmountCents/expectedCurrencydon't match, the transaction returns a bare error and the order stayspendingforever (the caller's webhook event is already claimed by the time this runs — see the related webhook-ordering issue incontroller/payment_stripe.go). Consider transitioning toPaymentStatusManualReviewon validation mismatch, mirroring the existing missing-user handling a few lines below, so operators have a discoverable, terminal state to reconcile instead of a silently stuckpendingrow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/payment_order.go` around lines 220 - 309, The validation mismatch branches in SettlePaidPaymentOrder currently return errors without changing the order state; update the amount and currency checks to persist PaymentStatusManualReview with the settlement timestamp before returning the mismatch error. Mirror the existing missing-user manual-review update and keep the transaction/error context intact so the order becomes discoverable and terminal.
198-208: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse GORM’s duplicate-key sentinel here
gorm.ErrDuplicatedKeyis available in gorm.io/gorm v1.31.2, but it only applies whenTranslateError: trueis set on the DB openers; otherwise this helper still depends on driver-specific error strings. Replace the substring check witherrors.Is(err, gorm.ErrDuplicatedKey)and enable translation in the affected DB init paths.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/payment_order.go` around lines 198 - 208, Update isDuplicateKeyError to return errors.Is(err, gorm.ErrDuplicatedKey), retaining the nil guard and removing driver-specific string matching. Enable GORM TranslateError in all affected database opener/configuration paths so duplicate violations are converted to gorm.ErrDuplicatedKey.model/payment_order_test.go (1)
35-68: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd a true concurrent test for the optimistic settlement claim.
This test calls
SettlePaidPaymentOrdertwice sequentially, which validates idempotency but not the actual concurrent race theWhere("status = ?", PaymentStatusPending)guard is meant to protect against. A test that fires the same session through multiple goroutines and asserts exactly one credits quota would directly validate the fix for the earlier race-condition finding.func TestSettlePaidPaymentOrderConcurrent(t *testing.T) { setupPaymentTestDB(t) require.NoError(t, DB.Create(&User{Id: 3, Username: "u3", Password: "x", DisplayName: "p", Role: 1, Status: 1, Quota: 0}).Error) require.NoError(t, DB.Create(&PaymentOrder{ UserId: 3, Provider: PaymentProviderStripe, RequestID: "req-3", SessionID: "cs_3", AmountCents: 500, Currency: "usd", Quota: 50, Status: PaymentStatusPending, }).Error) var wg sync.WaitGroup transitions := make([]bool, 10) for i := 0; i < 10; i++ { wg.Add(1) go func(i int) { defer wg.Done() tr, _, err := SettlePaidPaymentOrder(context.Background(), "cs_3", time.Now().UnixMilli(), 500, "usd") require.NoError(t, err) transitions[i] = tr }(i) } wg.Wait() count := 0 for _, tr := range transitions { if tr { count++ } } require.Equal(t, 1, count) var u User require.NoError(t, DB.First(&u, 3).Error) require.Equal(t, int64(50), u.Quota) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@model/payment_order_test.go` around lines 35 - 68, Add a true concurrent settlement test alongside TestSettlePaidPaymentOrderIdempotent, using multiple goroutines and a shared WaitGroup to call SettlePaidPaymentOrder for the same session. Record each transition result, assert exactly one call transitions the order, and verify the user quota is credited only once; avoid calling testing assertions unsafely from worker goroutines by reporting errors back to the test goroutine.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@controller/payment_stripe.go`:
- Around line 74-220: Handle the error returned by model.GetUserEmail in
CreateStripeCheckout instead of discarding it: capture the error, log it with
logger.Error, and return the existing failure response before continuing when
retrieval fails. Preserve the current trimming and optional-email behavior for
successful lookups.
- Around line 257-304: Move Stripe event deduplication in StripeWebhook until
after the dispatched handler completes successfully, so failed
handleCheckoutPaid and handleCheckoutTerminal attempts leave the event unclaimed
for retry. Update those handlers or the dispatch flow to return/propagate
success before recording the event with ClaimStripeWebhookEvent; preserve
duplicate short-circuiting for already successfully processed events and keep
existing error responses.
In `@model/payment_order.go`:
- Around line 37-50: Update the PaymentOrder model’s RequestID uniqueness to be
scoped per user: remove the standalone uniqueIndex from RequestID and define a
shared composite unique index on UserId and RequestID. Preserve the existing
GetPaymentOrderByRequestID user-scoped lookup and ensure the ORM migration
creates the composite constraint instead of the global one.
In `@web/modern/src/pages/topup/TopUpPage.tsx`:
- Around line 621-672: Update the history status rendering in the mobile cards
and desktop table to use the existing statusLabel helper instead of rendering
entry.status directly. Ensure both views pass each entry’s status through
statusLabel while preserving the surrounding layout and data-label behavior.
---
Nitpick comments:
In `@model/payment_order_test.go`:
- Around line 35-68: Add a true concurrent settlement test alongside
TestSettlePaidPaymentOrderIdempotent, using multiple goroutines and a shared
WaitGroup to call SettlePaidPaymentOrder for the same session. Record each
transition result, assert exactly one call transitions the order, and verify the
user quota is credited only once; avoid calling testing assertions unsafely from
worker goroutines by reporting errors back to the test goroutine.
In `@model/payment_order.go`:
- Around line 220-309: The validation mismatch branches in
SettlePaidPaymentOrder currently return errors without changing the order state;
update the amount and currency checks to persist PaymentStatusManualReview with
the settlement timestamp before returning the mismatch error. Mirror the
existing missing-user manual-review update and keep the transaction/error
context intact so the order becomes discoverable and terminal.
- Around line 198-208: Update isDuplicateKeyError to return errors.Is(err,
gorm.ErrDuplicatedKey), retaining the nil guard and removing driver-specific
string matching. Enable GORM TranslateError in all affected database
opener/configuration paths so duplicate violations are converted to
gorm.ErrDuplicatedKey.
In `@web/modern/src/pages/topup/__tests__/TopUpPage.amount.test.tsx`:
- Around line 40-100: Restore amount-specific coverage in the TopUpPage test
suite: add tests for amount_usd minimum and maximum validation, preset-chip
selection, and the Checkout redirect triggered by onStripeSubmit. Reuse the
existing beforeEach API/localStorage mocks and renderPage helper, while
preserving the current router-safety and stripe=cancel tests.
In `@web/modern/src/pages/topup/TopUpPage.tsx`:
- Around line 463-483: Add an aria-pressed attribute to each preset button
rendered in the presets.map callback, using the existing active value so screen
readers receive the same selected-state information conveyed by the className.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a008faab-ac6a-4c4e-b6cd-27bd74a78e81
📒 Files selected for processing (12)
common/config/config.gocontroller/misc.gocontroller/option.gocontroller/payment_stripe.gocontroller/payment_stripe_ready_test.gomodel/main.gomodel/option.gomodel/payment_order.gomodel/payment_order_test.gorouter/api.goweb/modern/src/pages/topup/TopUpPage.tsxweb/modern/src/pages/topup/__tests__/TopUpPage.amount.test.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- controller/misc.go
- model/main.go
- common/config/config.go
- model/option.go
… user - Record webhook event ids only after handlers finish successfully so 5xx can retry - Short-circuit already-processed events via HasStripeWebhookEvent - Composite unique index on (user_id, request_id) - Amount/currency mismatch → manual_review without crediting - Log GetUserEmail failures; i18n status labels; aria-pressed presets - Concurrent settle test
Resolve conflicts keeping both upstream Resend email support and Stripe checkout hardening (sensitive options, StripeReady, payment orders).
|
Follow-up in Critical: webhook claim order
Major:
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
controller/option.go (1)
76-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the request-scoped logger.
logger.Loggerdrops request context and correlation fields. Initializelog := gmw.GetLogger(c)once inUpdateOption, then calllog.Debug(...)here.As per coding guidelines, “In request paths, use
gmw.GetLogger(c)instead of a global logger, call it once per function, and store the result locally.”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controller/option.go` around lines 76 - 77, Update UpdateOption to initialize a request-scoped logger once via gmw.GetLogger(c), store it as log, and replace logger.Logger.Debug in the sensitive-option branch with log.Debug while preserving the existing message and fields.Source: Coding guidelines
web/modern/src/pages/topup/TopUpPage.tsx (1)
304-316: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUsing
codeForm.setError('root', { type: 'success', ... })to render a success banner overloads the form-error channel for a non-error state, andreset()immediately preceding it can race with the error write. Consider a dedicatedredeemSuccessstate string instead; low priority.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@web/modern/src/pages/topup/TopUpPage.tsx` around lines 304 - 316, Replace the success-path use of codeForm.setError in the redemption flow with a dedicated redeemSuccess state string, setting it after codeForm.reset and rendering the success banner from that state. Keep the existing error-channel handling for failed redemptions and clear redeemSuccess when starting or resetting a new redemption.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@model/payment_order_test.go`:
- Around line 150-160: Update the concurrent settlement test loop around
SettlePaidPaymentOrder to record when all 20 attempts fail, rather than
returning silently. Track exhausted retry loops with a synchronized failure
counter or equivalent, then after wg.Wait() assert that the counter is zero
while preserving the existing transition counting.
In `@model/payment_order.go`:
- Around line 266-287: In the amount-mismatch and currency-mismatch branches,
guard each manual_review update in the payment-order transaction with
Where("status = ?", PaymentStatusPending), then inspect RowsAffected. If no
pending row was claimed, reload the order’s current terminal status and return
that result instead of overwriting it; preserve the existing error handling and
in-memory updates only for successfully claimed rows.
In `@web/modern/src/pages/topup/TopUpPage.tsx`:
- Line 50: Update the TopUpPage test renders to wrap the component with
MemoryRouter or the project’s shared test router helper, ensuring
useSearchParams() has the required Router context while preserving the existing
mount and redeem test behavior.
---
Nitpick comments:
In `@controller/option.go`:
- Around line 76-77: Update UpdateOption to initialize a request-scoped logger
once via gmw.GetLogger(c), store it as log, and replace logger.Logger.Debug in
the sensitive-option branch with log.Debug while preserving the existing message
and fields.
In `@web/modern/src/pages/topup/TopUpPage.tsx`:
- Around line 304-316: Replace the success-path use of codeForm.setError in the
redemption flow with a dedicated redeemSuccess state string, setting it after
codeForm.reset and rendering the success banner from that state. Keep the
existing error-channel handling for failed redemptions and clear redeemSuccess
when starting or resetting a new redemption.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 877e2607-a947-4f53-806f-102a933baa29
📒 Files selected for processing (8)
common/config/config.gocontroller/misc.gocontroller/option.gocontroller/payment_stripe.gomodel/option.gomodel/payment_order.gomodel/payment_order_test.goweb/modern/src/pages/topup/TopUpPage.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- model/option.go
- controller/misc.go
- controller/payment_stripe.go
- Wrap TopUpPage tests in MemoryRouter for useSearchParams - Guard amount/currency manual_review with pending status claim - Fail concurrent settle test when retries are exhausted
|
Follow-up
Also mocked Stripe order APIs in the older TopUp tests so mount/redeem stay stable. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@web/modern/src/pages/topup/__tests__/TopUpPage.amount.test.tsx`:
- Around line 90-107: Expand the TopUpPage amount tests beyond generic
balance/token text: restore cent-precision and minimum-amount boundary cases,
verify checkout initiation sends the expected request and redirect, and update
the successful-return test to wait for the mocked paid response before asserting
the specific refreshed balance or settlement status. Keep the Router and
cancellation coverage intact while ensuring assertions cannot pass from initial
page content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 282691b1-d2a0-41f7-982e-759183df60cb
📒 Files selected for processing (4)
model/payment_order.gomodel/payment_order_test.goweb/modern/src/pages/topup/TopUpPage.test.tsxweb/modern/src/pages/topup/__tests__/TopUpPage.amount.test.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- model/payment_order_test.go
- model/payment_order.go
| it('renders inside a Router without useSearchParams crashes', async () => { | ||
| renderPage(); | ||
|
|
||
| // Wait for the user payload to settle so userData is populated | ||
| await waitFor(() => expect(api.get).toHaveBeenCalled()); | ||
|
|
||
| // Find and update amount | ||
| const amountInput = await screen.findByLabelText(/^amount$/i); | ||
| fireEvent.change(amountInput, { target: { value: '5' } }); | ||
|
|
||
| const codeInput = screen.getByLabelText(/top-up code/i); | ||
| fireEvent.change(codeInput, { target: { value: 'PROMO5' } }); | ||
|
|
||
| const calcBtn = screen.getByRole('button', { name: /^calculate$/i }); | ||
| fireEvent.click(calcBtn); | ||
|
|
||
| await waitFor(() => { | ||
| expect(api.post).toHaveBeenCalledWith('/api/user/amount', { | ||
| amount: 5, | ||
| top_up_code: 'PROMO5', | ||
| }); | ||
| }); | ||
|
|
||
| // The result is shown via the data-testid | ||
| await screen.findByTestId('topup-amount-result'); | ||
|
|
||
| // Spy on window.location.href assignment | ||
| const originalLocation = window.location; | ||
| const hrefSpy = vi.fn(); | ||
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: new Proxy(originalLocation, { | ||
| set(_target, prop, value) { | ||
| if (prop === 'href') { | ||
| hrefSpy(value); | ||
| return true; | ||
| } | ||
| (originalLocation as any)[prop as any] = value; | ||
| return true; | ||
| }, | ||
| get(target, prop) { | ||
| return (target as any)[prop as any]; | ||
| }, | ||
| }) as any, | ||
| }); | ||
|
|
||
| const rechargeBtn = screen.getByRole('button', { name: /^recharge$/i }); | ||
| fireEvent.click(rechargeBtn); | ||
|
|
||
| await waitFor(() => expect(hrefSpy).toHaveBeenCalledTimes(1)); | ||
| const navigatedTo: string = hrefSpy.mock.calls[0][0]; | ||
| expect(navigatedTo).toContain('https://pay.example.com/checkout'); | ||
| expect(navigatedTo).toContain('username=amountuser'); | ||
| expect(navigatedTo).toContain('user_id=42'); | ||
| expect(navigatedTo).toContain('transaction_id='); | ||
|
|
||
| // Restore window.location | ||
| Object.defineProperty(window, 'location', { | ||
| configurable: true, | ||
| value: originalLocation, | ||
| }); | ||
| await waitFor(() => expect(api.get).toHaveBeenCalledWith('/api/user/self')); | ||
| expect(document.body.textContent || '').toMatch(/balance|token/i); | ||
| }); | ||
|
|
||
| it('shows error notification when backend returns failure with data field', async () => { | ||
| (api.post as any).mockResolvedValue({ data: { success: false, data: 'Invalid promo code' } }); | ||
|
|
||
| renderPage(); | ||
|
|
||
| await waitFor(() => expect(api.get).toHaveBeenCalled()); | ||
|
|
||
| const amountInput = await screen.findByLabelText(/^amount$/i); | ||
| fireEvent.change(amountInput, { target: { value: '10' } }); | ||
|
|
||
| const calcBtn = screen.getByRole('button', { name: /^calculate$/i }); | ||
| fireEvent.click(calcBtn); | ||
|
|
||
| await waitFor(() => expect(api.post).toHaveBeenCalled()); | ||
| it('shows cancel outcome from query string', async () => { | ||
| renderPage('/topup?stripe=cancel'); | ||
| await waitFor(() => expect(api.get).toHaveBeenCalledWith('/api/user/self')); | ||
| expect(document.body.textContent || '').toMatch(/cancel|not been charged/i); | ||
| }); | ||
|
|
||
| // Recharge should remain disabled because no amount was computed | ||
| const rechargeBtn = screen.getByRole('button', { name: /^recharge$/i }); | ||
| expect(rechargeBtn).toBeDisabled(); | ||
| it('polls order status after success return', async () => { | ||
| renderPage('/topup?stripe=success&session_id=cs_test'); | ||
| await waitFor(() => | ||
| expect(api.get).toHaveBeenCalledWith(expect.stringContaining('/topup/stripe/orders/cs_test')) | ||
| ); | ||
| expect(document.body.textContent || '').toMatch(/balance|token|credit/i); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restore payment-safety assertions.
The paid-order assertion can pass before the mocked paid response affects the UI, since the initial page already contains balance/token text. These replacements also remove amount/minimum-boundary and checkout initiation coverage. Re-add cent-precision and minimum-amount cases, assert the Checkout request/redirect, and verify a paid return produces the specific refreshed balance or settlement status.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@web/modern/src/pages/topup/__tests__/TopUpPage.amount.test.tsx` around lines
90 - 107, Expand the TopUpPage amount tests beyond generic balance/token text:
restore cent-precision and minimum-amount boundary cases, verify checkout
initiation sends the expected request and redirect, and update the
successful-return test to wait for the mocked paid response before asserting the
specific refreshed balance or settlement status. Keep the Router and
cancellation coverage intact while ensuring assertions cannot pass from initial
page content.
Laisky
left a comment
Laisky
left a comment
There was a problem hiding this comment.
Maintainer review — Request changes
Reviewed the final patch at 1e6521e8c938d2df371dbad7bff760fe548767e9, including the checkout controller, payment models and migrations, runtime options, routing/middleware, top-up UI, tests, existing review threads, and current CI results.
The direction is good: the patch uses a dedicated Stripe client, verifies webhook signatures, caps the webhook body, stores immutable cents/quota on the order, scopes order reads to the authenticated user, and credits quota in the same database transaction as pending -> paid. Those are important improvements.
However, this is a money-moving path and the final head still has several correctness issues that can cause a successful charge to be credited to the wrong user or not credited at all. I do not consider the patch production-safe yet.
Overall score: 4.8/10 — Request changes.
P0 — A successful Stripe charge can be acknowledged without any quota credit
CreateStripeCheckout creates the remote Checkout Session and then treats BindPaymentOrderSession as best effort. If the database bind fails, the endpoint still returns the Checkout URL. Later, handleCheckoutPaid looks up only by session_id; when that lookup fails it returns HTTP 200 with unknown_session=true, which tells Stripe that delivery succeeded and stops automatic retries.
This creates a concrete paid-but-uncredited failure mode:
- Local placeholder order is created.
- Stripe Session creation succeeds.
- The local
session_idbind fails or the process loses the response before the bind commits. - The customer completes payment.
- The webhook cannot find the session locally and is acknowledged with 200.
- The customer is charged, quota is not credited, and no automatic retry repairs it.
Required changes:
- Do not return a usable Checkout URL unless the local order-to-session association is durable.
- Make the webhook recover the association from trusted Session metadata (
order_idor, preferably, an externally safe order UUID), then verify provider, user, amount, currency, and request identity before settlement. - If recovery cannot be completed because of a transient storage failure, return 5xx so Stripe retries. Return 2xx for an unknown paid Session only after an orphan/manual-review record has been durably stored and alerted.
- Add an integration test covering: Stripe Session succeeds -> local bind fails -> paid webhook is replayed -> exactly one quota credit occurs.
P0 — The Stripe idempotency key is not tenant-scoped and is directly controlled by the caller
The local uniqueness rule is (user_id, request_id), but params.SetIdempotencyKey(requestID) sends the raw caller-provided value to Stripe, where the key is scoped to the Stripe account rather than this application's user row.
Two users can submit the same request_id and the same amount. Stripe can return the same Checkout Session to both requests. The second local bind then fails on the unique session_id, but the controller currently ignores that bind error and returns the shared Checkout URL anyway. If the second user pays, the webhook settles the first user's order.
There are related retry problems:
- Reusing an existing local order does not verify that amount, currency, quota, provider, and status match the original request.
- Stripe parameters are built from the new request rather than the persisted order.
- The modern UI does not send
request_id, so browser/network retries create new orders and Sessions instead of exercising the advertised idempotency path. - A bound order still calls
CheckoutSessions.Newagain and assumes Stripe will retain the old response indefinitely.
Required changes:
- Generate and persist a provider idempotency key server-side, namespaced by provider and the immutable local order identity. Never pass an arbitrary client value directly to Stripe.
- Scope database uniqueness by provider, for example
(provider, user_id, request_id)and(provider, session_id). - On retry, compare the complete immutable request tuple. Return a conflict for the same request ID with different amount/currency/quota.
- Build all Stripe parameters from the persisted order, not from the retry payload.
- Bind with compare-and-swap semantics against the expected placeholder, not only
id AND status=pending, so an already-bound Session cannot be overwritten. - Have the UI generate one request ID per payment intent and reuse it across transport retries.
- Add cross-user collision, same-key/different-amount, concurrent creation, and already-bound retry tests.
P0 — The payment state machine depends on webhook delivery order
checkout.session.expired and checkout.session.async_payment_failed move a pending order to canceled or failed. SettlePaidPaymentOrder only claims pending -> paid. If a terminal event is delivered first and a genuine paid event is delivered later, settlement returns payment order not claimable, the handler returns 500, and the paid event can retry forever without crediting the account.
Stripe explicitly does not guarantee webhook delivery order, so event order cannot define the authoritative payment state.
Required changes:
- Define an explicit transition table and make verified provider-paid state authoritative.
- For a paid event received after a local terminal state, retrieve/verify the current Checkout Session and either transition safely to paid or persist a manual-review case; do not enter a permanent 500 loop.
- Store reconciliation evidence: Stripe event ID, observed amount/currency/payment status, failure/review reason, and preferably PaymentIntent ID.
- Add tests for terminal-before-paid, paid-before-terminal, duplicate events with different event IDs, and concurrent mixed event types.
P1 — Configuration precedence and validation are unsafe
STRIPE_SECRET_KEY, STRIPE_WEBHOOK_SECRET, and MIN_TOPUP_USD are read from the environment, but Stripe keys are absent from envOverrideForOption. Persisted database options can therefore silently replace deployment-managed environment secrets during startup or periodic option synchronization. This is inconsistent with the existing Resend environment precedence and can switch accounts, disable settlement, or use a stale webhook secret.
MinTopUpUSD also ignores parse errors and clamps invalid persisted values to 1, which can silently lower the configured minimum.
Required changes:
- Define and test one precedence rule. For deployment secrets, non-empty environment values should remain authoritative during startup, admin updates, and periodic synchronization.
- Prefer a secret manager/environment-only model for Stripe live credentials. If database configuration remains supported, encrypt secrets at rest and expose only configured/not-configured status.
- Strictly parse and range-check
MinTopUpUSDbefore persistence; reject invalid values instead of coercing them. - Add startup, runtime update, clear, and periodic-sync tests for environment-vs-database precedence.
P1 — Money input should not use binary floating point
The public API accepts amount_usd as float64 and rounds with int64(amount * 100 + 0.5). It also accepts more than two decimal places even though the UI shows a cent step. This makes the charged amount depend on binary-float rounding and silently changes values such as fractional-cent input.
Required changes:
- Accept integer
amount_cents, or accept a decimal string and parse it with an exact money parser. - Reject more than two decimal places; validate min/max in cents.
- Use checked integer arithmetic for quota calculation and define whether
QuotaPerUnitmust be integral. Do not silently truncate a fractional quota rate. - Require
amount_totalandcurrencyin paid webhook processing; missing values must not disable validation. Prefer retrieving the Session as Stripe recommends for fulfillment.
P1 — The current test suite and CI do not establish payment safety
The model tests cover basic idempotent settlement, amount mismatch, missing user, event-ID dedupe, and concurrent paid settlement, which is useful. They do not cover the cross-system failure boundaries above. The frontend Stripe tests only verify mounting, cancel text, and that polling was invoked; they do not assert cent precision, minimum boundaries, the checkout request, stable idempotency identity, redirect URL, successful paid rendering, or refreshed balance/history.
At this head, the PR checks are also not fully green: the Run Tests job is failing, and the coverage-report job is failing. The existing unresolved review thread about weakened Stripe UI coverage remains valid.
Required acceptance tests:
- Session-create success plus local-bind failure and recovery.
- Unknown paid Session with recoverable metadata and with unrecoverable metadata.
- Cross-user identical request IDs.
- Same request ID with changed amount.
- Concurrent checkout creation and concurrent mixed webhook events.
- Terminal-before-paid event delivery.
- Missing/invalid amount and currency fields.
- Environment/DB option precedence and explicit secret clear behavior.
- Frontend exact-cent/minimum validation, POST payload, stable request ID, redirect, paid state, and balance/history refresh.
- At least one real Stripe test-mode or Stripe-mock end-to-end path, plus database-dialect coverage for supported production databases.
Additional maintainability and operational changes
- Move checkout orchestration and settlement out of the Gin controller into an injectable payment service/gateway. The current controller is difficult to fault-inject and
TopUpPage.tsxis too large; split data hooks and presentation components. - Return a dedicated payment-order DTO instead of the raw database model, and do not expose internal
user_id/database IDs unnecessarily. - Do not return raw Stripe or database error strings to clients; log structured provider details and return stable public errors.
- Parse the public base URL with
net/url; require a valid host, reject userinfo/query/fragment where inappropriate, and permit HTTP only for explicit localhost development. - Exempt the signed webhook from the generic per-IP API limiter or give it a dedicated limiter/queue. Add metrics and alerts for unknown Sessions, manual review, settlement failures, and webhook lag.
- Add a lifecycle/retention policy for abandoned placeholder orders and processed webhook-event rows.
- Decide and implement refund/dispute behavior before calling this production-ready; at minimum store PaymentIntent/Charge identifiers and create an operator reconciliation path.
- Run
gofmtonrouter/api.go; the new webhook route is visibly mis-indented.
Acceptance checklist
- No successful paid Session can be lost between Stripe and the local order store.
- Provider idempotency keys are server-generated, tenant-safe, persisted, and payload-consistent.
- Paid fulfillment is correct under duplicate, concurrent, delayed, and out-of-order events.
- Environment-managed secrets cannot be overwritten by stale database options.
- Money is represented and validated in exact integer cents.
- Unknown/manual-review payments are durable, observable, and reconcilable.
- Frontend and backend tests cover the failure matrix above.
- All required CI checks pass.
Once these items are addressed, the existing transactional quota update and signed-webhook foundation should be a solid base for the feature.
2 participants
Summary
Adds optional Stripe Checkout so operators can accept freeform USD top-ups and credit user quota after a signed webhook.
The feature is disabled when
STRIPE_SECRET_KEYis unset (returns a clear "Stripe is not configured" message).Backend
POST /api/user/topup/stripe(authenticated) — create a Checkout Session foramount_usdPOST /api/payment/stripe/webhook(public) — verify Stripe signature, mark order paid, credit quota (idempotent viapayment_orders)PaymentOrder+AutoMigrateSTRIPE_SECRET_KEYSTRIPE_WEBHOOK_SECRETMIN_TOPUP_USD(default5)Frontend (modern)
Top-up page updates:
Configuration
STRIPE_SECRET_KEY=sk_live_... # or sk_test_... STRIPE_WEBHOOK_SECRET=whsec_... MIN_TOPUP_USD=5Webhook endpoint to configure in the Stripe Dashboard:
Behavior notes
amount_usd(platform absorbs Stripe processing fees in this implementation).amount_usd * QuotaPerUnit.Test plan
go build/go teston touched packagesOut of scope
This PR intentionally excludes fork-only items (custom
opentheme, Resend, Railway/VPS deploy packs).Summary by CodeRabbit
min_topup_usd(default 5; enforced whole-USD minimum of 1); exposed Stripe readiness in status.