Open
Conversation
…t is a mirror of existing checkout functionality
|
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:
📝 WalkthroughWalkthroughMigrates dues payment from Stripe Checkout sessions to Stripe PaymentIntents + Stripe Elements: payment button now routes to a local Changes
Sequence DiagramsequenceDiagram
participant User
participant UI as Payment Button / Checkout Page
participant Client as CheckoutForm (Browser)
participant TRPC as Backend (TRPC)
participant Stripe as Stripe API
participant Webhook as Webhook Handler
participant DB as Database
User->>UI: Click "Pay"
UI->>UI: router.push("/member/checkout")
UI->>TRPC: validatePaidDues()
TRPC-->>UI: not paid
UI->>Client: render CheckoutForm
Client->>TRPC: createPaymentIntent()
TRPC->>Stripe: Create PaymentIntent (metadata: member_id)
Stripe-->>TRPC: client_secret
TRPC-->>Client: clientSecret
Client->>Stripe: confirmPayment(return_url, receipt_email)
Stripe-->>User: redirect to success page with payment_intent
User->>UI: open success page (payment_intent)
UI->>TRPC: orderSuccess(paymentIntentId)
TRPC->>Stripe: retrieve PaymentIntent
Stripe-->>TRPC: payment intent data (status, metadata)
TRPC->>DB: insert DuesPayment (idempotent / onConflictDoNothing)
Webhook->>TRPC: payment_intent.succeeded -> fulfillPaymentIntent(paymentIntentId)
TRPC->>DB: idempotent insert (guarded by unique constraint)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (1)
apps/blade/src/app/_components/dashboard/member/checkout-form.tsx (1)
177-200: Drive the displayed price from the same source as Stripe.
$25.00is duplicated three times here while the server chargesCLUB.MEMBERSHIP_PRICE. The next price change can put the UI and Stripe total out of sync.Keep the summary bound to the shared price constant
Add
import { CLUB } from "@forge/consts";near the other imports, then derive one formatted value:+ const membershipPrice = `$${(CLUB.MEMBERSHIP_PRICE / 100).toFixed(2)}`; + return ( <div className="mx-auto max-w-4xl overflow-hidden rounded-lg border bg-card shadow-sm"> <div className="grid md:grid-cols-2"> {/* Left — order summary */} <div className="flex flex-col justify-between bg-muted p-8"> @@ - <h2 className="mt-2 text-4xl font-bold">$25.00</h2> + <h2 className="mt-2 text-4xl font-bold">{membershipPrice}</h2> @@ - <span className="font-medium">$25.00</span> + <span className="font-medium">{membershipPrice}</span> @@ - <span>$25.00</span> + <span>{membershipPrice}</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/dashboard/member/checkout-form.tsx` around lines 177 - 200, The UI is hardcoding "$25.00" in checkout-form.tsx in three places causing drift from the server value; import the CLUB constant (import { CLUB } from "@forge/consts") at the top of the file, compute a single formattedPrice from CLUB.MEMBERSHIP_PRICE (use Intl.NumberFormat or your project's currency formatter) inside the CheckoutForm component, and replace the three literal "$25.00" occurrences (the h2 amount, the line-item amount, and the "Total due today" amount) with that formattedPrice so the display always matches CLUB.MEMBERSHIP_PRICE.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/src/app/_components/dashboard/member/checkout-form.tsx`:
- Around line 98-103: The current conditional in checkout-form.tsx treats
paymentIntent.status === "processing" the same as "succeeded" and routes to
router.push(`/member/success?payment_intent=${paymentIntent.id}`), but
downstream logic (membership-success-page.tsx and api.duesPayment.orderSuccess)
only considers "succeeded" final; change the handling so only "succeeded" routes
to the success page and handle "processing" as a pending state instead (e.g.,
render or route to a pending UI or keep the user on the checkout/pending state
until a webhook or poll confirms success), updating the paymentIntent.status
check in the function that performs the router.push to exclude "processing" and
add the pending flow for "processing" intents.
In `@apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx`:
- Around line 25-27: Remove the client-side debug log that prints sensitive
payment data: locate the console.log(data) in the membership-success-page React
component (membership-success-page.tsx) and delete that line (or replace it with
a safe, non-sensitive debug statement), ensuring no payer email or payment
intent id is logged to the browser console.
In `@apps/blade/src/app/api/membership/route.ts`:
- Around line 78-86: The current select-then-insert race on DuesPayment (the
existing query and db.insert of validated.data) allows duplicates; add a
schema-backed unique key (e.g., payment_intent_id or a unique composite like
(member_id, period) depending on your domain) to the DuesPayment table, include
that unique field in validated.data (and in the payment_intent.succeeded flow),
and replace the preflight select with an atomic upsert/insert-with-conflict
behavior (use your ORM's insert...onConflict/doNothing or upsert API against
DuesPayment) so the database enforces idempotency instead of the non-atomic
existing check.
- Around line 130-137: The webhook handler currently returns non-2xx responses
for business outcomes; change the response codes for the cases handling
payment_intent.payment_failed, payment_intent.canceled,
checkout.session.async_payment_failed, checkout.session.expired (and any
checkout.session.canceled branch) to return a 2xx acknowledgement (e.g.,
Response("OK", { status: 200 })) so Stripe will not retry delivery, and instead
use the existing logger to record the business outcome/state (e.g., log event
type and reason) inside the same branches; ensure only genuine processing errors
(signature/db failures) still return non-2xx.
In `@packages/api/src/routers/dues-payment.ts`:
- Around line 128-142: discord.log(...) is called unconditionally causing
duplicate audit posts; move the discord.log call inside the idempotent branch
guarded by existing.length === 0 so it only runs when you actually insert a new
DuesPayment. Specifically, after the db.insert(DuesPayment).values({ memberId,
amount: paymentIntent.amount, paymentDate: new Date(paymentIntent.created *
1000), year: new Date().getFullYear() }) completes, call discord.log with the
same payload (using paymentIntent.amount and ctx.session.user.discordUserId) and
remove the unconditional call outside that block.
- Around line 64-90: Before creating a new Stripe PaymentIntent in
createPaymentIntent, query the DuesPayment table for the found member (use
member[0].id) to detect an existing successful/paid dues record and reject the
request; if a paid dues row exists throw a TRPCError (e.g., code "CONFLICT" or
"BAD_REQUEST") with a clear message instead of creating a new payment intent.
Ensure the check runs after locating the Member and before
stripe.paymentIntents.create, referencing DuesPayment and member[0].id to locate
any completed payments and short-circuit duplicate checkout attempts.
- Around line 110-149: The handler currently trusts the PaymentIntent id and
returns sensitive info without verifying ownership; retrieve
paymentIntent.metadata?.member_id (the memberId) and compare it to the
authenticated user identifier in ctx.session.user (e.g. ctx.session.user.id or
ctx.session.user.memberId — use whichever field represents the member record in
your app); if the metadata is missing or does not strictly equal the session
user’s id, throw a TRPCError with code "FORBIDDEN" and stop processing; perform
this check immediately after retrieving the PaymentIntent (before inserting into
DuesPayment, logging with discord.log, or returning receipt_email) so only the
owner can view/acknowledge their payment.
---
Nitpick comments:
In `@apps/blade/src/app/_components/dashboard/member/checkout-form.tsx`:
- Around line 177-200: The UI is hardcoding "$25.00" in checkout-form.tsx in
three places causing drift from the server value; import the CLUB constant
(import { CLUB } from "@forge/consts") at the top of the file, compute a single
formattedPrice from CLUB.MEMBERSHIP_PRICE (use Intl.NumberFormat or your
project's currency formatter) inside the CheckoutForm component, and replace the
three literal "$25.00" occurrences (the h2 amount, the line-item amount, and the
"Total due today" amount) with that formattedPrice so the display always matches
CLUB.MEMBERSHIP_PRICE.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e41d5ed3-cb6e-47fd-976e-680f04dfc421
📒 Files selected for processing (6)
apps/blade/src/app/_components/dashboard/member-dashboard/payment/payment-button.tsxapps/blade/src/app/_components/dashboard/member/checkout-form.tsxapps/blade/src/app/_components/dashboard/member/membership-success-page.tsxapps/blade/src/app/api/membership/route.tsapps/blade/src/app/member/checkout/page.tsxpackages/api/src/routers/dues-payment.ts
apps/blade/src/app/_components/dashboard/member/checkout-form.tsx
Outdated
Show resolved
Hide resolved
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx
Outdated
Show resolved
Hide resolved
Comment on lines
+130
to
+137
| case "payment_intent.payment_failed": | ||
| return new Response("Payment failed", { | ||
| status: 401, | ||
| }); | ||
| case "payment_intent.canceled": | ||
| return new Response("Payment canceled", { | ||
| status: 410, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
For Stripe webhooks, does returning a non-2xx status for a handled event cause Stripe to retry delivery, and should expected payment_intent.payment_failed/payment_intent.canceled events still return 2xx?
💡 Result:
Yes. For Stripe webhooks, any delivery that doesn’t get a successful 2xx response (including 3xx redirects, 4xx, 5xx, or timeouts) is treated as a failed delivery, and Stripe will automatically retry (in live mode, for up to ~3 days with exponential backoff; shorter in sandbox/test). Stripe’s dashboard will show the event as Pending/Failed and when the next retry will occur. [1]
And yes: if you successfully received and handled an event—even when the payment outcome is failure/canceled—you should still return 2xx for expected events like payment_intent.payment_failed and payment_intent.canceled. A non-2xx should be reserved for cases where your webhook processing failed (for example, you couldn’t verify/parse the event, your DB/queue is down, etc.) and you want Stripe to retry. [1][2]
🏁 Script executed:
# Read the membership webhook route to understand the full context
head -200 apps/blade/src/app/api/membership/route.ts | cat -nRepository: KnightHacks/forge
Length of output: 5564
Return 2xx for all handled Stripe events; reserve error codes for webhook processing failures.
The webhook handler returns 401/410 for payment_intent.payment_failed and payment_intent.canceled, but these are expected business outcomes, not delivery failures. Stripe treats any non-2xx response as a failed delivery and retries the event (up to ~3 days in live mode)—causing unnecessary retries of already-handled events.
Additionally, three other events have the same issue: checkout.session.async_payment_failed (line 113), checkout.session.expired (line 123), and checkout.session.canceled if added. Reserve non-2xx status codes only for actual webhook processing failures (e.g., signature verification failed, database unavailable).
The logger is already imported; use it to record the event state:
Fix for all affected events
case "checkout.session.async_payment_failed":
+ logger.warn("Async payment failed", { sessionId: event.data.object.id });
- return new Response("Payment failed", {
- status: 401,
+ return new Response("Event received", {
+ status: 200,
});
case "checkout.session.expired":
+ logger.warn("Checkout session expired", { sessionId: event.data.object.id });
- return new Response("Checkout session expired", {
- status: 408,
+ return new Response("Event received", {
+ status: 200,
});
case "payment_intent.payment_failed":
+ logger.warn("Payment failed", { paymentIntentId: event.data.object.id });
- return new Response("Payment failed", {
- status: 401,
+ return new Response("Event received", {
+ status: 200,
});
case "payment_intent.canceled":
+ logger.warn("Payment canceled", { paymentIntentId: event.data.object.id });
- return new Response("Payment canceled", {
- status: 410,
+ return new Response("Event received", {
+ status: 200,
});Per Stripe webhook documentation: acknowledge receipt with 2xx after processing, letting your logging track the business outcome.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| case "payment_intent.payment_failed": | |
| return new Response("Payment failed", { | |
| status: 401, | |
| }); | |
| case "payment_intent.canceled": | |
| return new Response("Payment canceled", { | |
| status: 410, | |
| }); | |
| case "payment_intent.payment_failed": | |
| logger.warn("Payment failed", { paymentIntentId: event.data.object.id }); | |
| return new Response("Event received", { | |
| status: 200, | |
| }); | |
| case "payment_intent.canceled": | |
| logger.warn("Payment canceled", { paymentIntentId: event.data.object.id }); | |
| return new Response("Event received", { | |
| status: 200, | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/blade/src/app/api/membership/route.ts` around lines 130 - 137, The
webhook handler currently returns non-2xx responses for business outcomes;
change the response codes for the cases handling payment_intent.payment_failed,
payment_intent.canceled, checkout.session.async_payment_failed,
checkout.session.expired (and any checkout.session.canceled branch) to return a
2xx acknowledgement (e.g., Response("OK", { status: 200 })) so Stripe will not
retry delivery, and instead use the existing logger to record the business
outcome/state (e.g., log event type and reason) inside the same branches; ensure
only genuine processing errors (signature/db failures) still return non-2xx.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/api/src/routers/dues-payment.ts`:
- Around line 78-83: The current dues lookup only filters by memberId
(existingDues) and must also be scoped to the billing year; compute a single
billingYear (e.g., from the incoming request or a helper like
computeBillingYear()) once and use it in the query predicates: add
eq(DuesPayment.billingYear, billingYear) to the .where(...) that builds
existingDues and to the later read/lookup used when deciding whether to record a
new payment (the same memberId-only predicate on Lines 143-147). Ensure both the
pre-check (existingDues) and the payment-recording/read paths use the same
billingYear variable so uniqueness by (memberId, year) is honored.
- Around line 142-155: Replace the race-prone check-then-insert with a single
conflict-aware insert into DuesPayment that uses the (memberId, year) unique key
as the ON CONFLICT target (e.g. via
db.insert(DuesPayment).values(...).onConflict(columns: [DuesPayment.memberId,
DuesPayment.year]).doNothing() or the equivalent in your ORM), capture whether
the insert actually affected a row (via returned row count/returning), and only
emit the Discord/logging action when the insert succeeded (i.e., this process
"won" the write); remove the separate select() existence check and ensure the
inserted payload still uses memberId, paymentIntent.amount,
paymentIntent.created -> paymentDate, and new Date().getFullYear() for year.
- Around line 119-124: The current check throws for any PaymentIntent.status !==
"succeeded" — update the handler around the paymentIntent variable so it only
throws a TRPCError for terminal failure statuses (e.g., "requires_action",
"requires_payment_method", "canceled") and returns non-terminal states (like
"processing") to the client so they can show a pending state; keep the existing
fulfillment path for paymentIntent.status === "succeeded" and ensure final
settlement is handled by your payment_intent.succeeded webhook and the frontend
success page logic (membership-success-page.tsx) is updated to display
"processing" as pending rather than an error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 322d9b00-7c6f-4d5f-8aef-47d569deb34a
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml,!pnpm-lock.yaml
📒 Files selected for processing (6)
MIGRATION_STATUS.mdapps/blade/src/app/_components/dashboard/member/checkout-form.tsxapps/blade/src/app/_components/dashboard/member/membership-success-page.tsxapps/blade/src/app/api/membership/route.tspackages/api/src/routers/dues-payment.tspackages/db/src/schemas/knight-hacks.ts
💤 Files with no reviewable changes (1)
- MIGRATION_STATUS.md
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/blade/src/app/_components/dashboard/member/checkout-form.tsx
- apps/blade/src/app/api/membership/route.ts
Contributor
Author
|
Most of the code rabbit changes were pretty intuitive to apply and easy to understand the rationale for. The race condition fix was a little difficult to wrap my head around though. But it prevents duplication through a constraint 2nd statement in image We have to update our schema with db:push for this. Let me know what you think |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
packages/api/src/routers/dues-payment.ts (4)
127-143:⚠️ Potential issue | 🔴 CriticalVerify the PaymentIntent belongs to
ctx.session.userbefore fulfilling it.This handler trusts the caller-provided PaymentIntent id and uses
metadata.member_iddirectly, but it never proves that member maps back to the authenticated user. Any signed-in user who gets another valid intent id can read its receipt email and attempt fulfillment for someone else. Reject missing or foreignmember_idbefore any insert or logging. As per coding guidelinespackages/api/**: "Authorization checks in every procedure".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 127 - 143, The code currently trusts paymentIntent.metadata.member_id and inserts a DuesPayment without confirming the intent actually belongs to the authenticated user; before doing any insert or logging, validate that paymentIntent.metadata.member_id is present and maps to ctx.session.user (or the expected member record) and reject if it is missing or does not match the authenticated user; locate the logic around paymentIntent, memberId, db and DuesPayment and perform an ownership/authorization check (e.g., compare memberId to ctx.session.user.id or query the Members table for a record owned by ctx.session.user) and return an error/abort if the check fails, only then proceed with inserting into DuesPayment.
79-83:⚠️ Potential issue | 🟠 MajorScope the dues guard to the billing year.
This lookup only filters by
memberId, so any historical dues row will block checkout forever once the(memberId, year)uniqueness rule is in play. Compute onebillingYearand include it here, then reuse that same year in the fulfillment path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 79 - 83, The lookup for existing dues only filters by memberId; compute the billing year once (e.g., billingYear) and add eq(DuesPayment.billingYear, billingYear) to the where clause that builds existingDues, and then reuse that same billingYear variable in the fulfillment path so both the guard and the row creation/fulfillment use the identical (memberId, billingYear) pair; update references to existingDues, DuesPayment, and any fulfillment code to use the billingYear variable.
129-150:⚠️ Potential issue | 🔴 CriticalMake fulfillment a single idempotent write and only log when it wins.
The
select-then-inserthere still races the webhook path, so the loser can hit the(memberId, year)unique constraint after a successful payment.discord.log()also runs even when this request did not create a new dues row. Collapse this to one conflict-aware insert and emit the audit log only when that insert actually succeeds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 129 - 150, Replace the select-then-insert race with a single conflict-aware insert on the DuesPayment table (include memberId and year in the unique constraint) using your DB client's ON CONFLICT DO NOTHING / upsert API (call db.insert(DuesPayment).values(...).onConflictDoNothing() or equivalent). After the insert, check the insert result/affected rows to determine whether the insert actually succeeded and only call discord.log(...) when the insert won (i.e., affectedRows > 0); do not run the prior select or log unconditionally. Ensure you still set amount and paymentDate from paymentIntent and compute year as before.
120-125:⚠️ Potential issue | 🟠 MajorTreat
processingas pending, not failed.
createPaymentIntent()enablesus_bank_account, and those PaymentIntents can legitimately come back asprocessingwhile the bank transfer settles. Throwing on every non-succeededstatus will turn valid ACH payments into false failures; only reject terminal failure states here and letpayment_intent.succeededfinish fulfillment.For Stripe PaymentIntents created with payment_method_types ["card", "us_bank_account"], can status "processing" be returned after confirmation, and should fulfillment wait for the payment_intent.succeeded webhook instead of treating "processing" as a failure?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 120 - 125, The current check in the dues-payment handler that throws whenever paymentIntent.status !== "succeeded" is treating transient states like "processing" (used by us_bank_account ACH flows) as failures; update the logic around the paymentIntent.status check (the block referencing paymentIntent.status and the createPaymentIntent() flow) to only throw for terminal failure states (e.g., "canceled" or "requires_payment_method") while treating "processing" (and other non-terminal states such as "requires_capture") as pending: return a pending response or defer fulfillment and do not throw, and only fulfill when paymentIntent.status === "succeeded".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx`:
- Around line 20-23: The code currently sets paymentIntentId =
searchParams.get("payment_intent") ?? "" and immediately calls
api.duesPayment.orderSuccess.useQuery(paymentIntentId); instead, detect a
missing/empty payment_intent first and avoid calling the backend: if
searchParams.get("payment_intent") is null/empty, render the invalid
confirmation state (or redirect) and do not invoke
api.duesPayment.orderSuccess.useQuery; otherwise pass the real id to the hook
and/or use the query's enabled option (e.g., enabled: Boolean(paymentIntentId))
so the query only runs when paymentIntentId is present. Ensure references:
paymentIntentId, searchParams, and api.duesPayment.orderSuccess.useQuery are
updated accordingly.
---
Duplicate comments:
In `@packages/api/src/routers/dues-payment.ts`:
- Around line 127-143: The code currently trusts
paymentIntent.metadata.member_id and inserts a DuesPayment without confirming
the intent actually belongs to the authenticated user; before doing any insert
or logging, validate that paymentIntent.metadata.member_id is present and maps
to ctx.session.user (or the expected member record) and reject if it is missing
or does not match the authenticated user; locate the logic around paymentIntent,
memberId, db and DuesPayment and perform an ownership/authorization check (e.g.,
compare memberId to ctx.session.user.id or query the Members table for a record
owned by ctx.session.user) and return an error/abort if the check fails, only
then proceed with inserting into DuesPayment.
- Around line 79-83: The lookup for existing dues only filters by memberId;
compute the billing year once (e.g., billingYear) and add
eq(DuesPayment.billingYear, billingYear) to the where clause that builds
existingDues, and then reuse that same billingYear variable in the fulfillment
path so both the guard and the row creation/fulfillment use the identical
(memberId, billingYear) pair; update references to existingDues, DuesPayment,
and any fulfillment code to use the billingYear variable.
- Around line 129-150: Replace the select-then-insert race with a single
conflict-aware insert on the DuesPayment table (include memberId and year in the
unique constraint) using your DB client's ON CONFLICT DO NOTHING / upsert API
(call db.insert(DuesPayment).values(...).onConflictDoNothing() or equivalent).
After the insert, check the insert result/affected rows to determine whether the
insert actually succeeded and only call discord.log(...) when the insert won
(i.e., affectedRows > 0); do not run the prior select or log unconditionally.
Ensure you still set amount and paymentDate from paymentIntent and compute year
as before.
- Around line 120-125: The current check in the dues-payment handler that throws
whenever paymentIntent.status !== "succeeded" is treating transient states like
"processing" (used by us_bank_account ACH flows) as failures; update the logic
around the paymentIntent.status check (the block referencing
paymentIntent.status and the createPaymentIntent() flow) to only throw for
terminal failure states (e.g., "canceled" or "requires_payment_method") while
treating "processing" (and other non-terminal states such as "requires_capture")
as pending: return a pending response or defer fulfillment and do not throw, and
only fulfill when paymentIntent.status === "succeeded".
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8b14a623-6d6a-413f-b78b-6199bca61ec7
📒 Files selected for processing (3)
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsxapps/blade/src/app/api/membership/route.tspackages/api/src/routers/dues-payment.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/blade/src/app/api/membership/route.ts
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx
Outdated
Show resolved
Hide resolved
… but also missing query fix
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
packages/api/src/routers/dues-payment.ts (1)
143-144:⚠️ Potential issue | 🔴 CriticalAuthorization gap: verify the PaymentIntent belongs to the current user.
The procedure retrieves a PaymentIntent by ID from user input but never confirms that
paymentIntent.metadata.member_idbelongs toctx.session.user. This allows any authenticated user who obtains a valid PaymentIntent ID to:
- View another user's
receipt_email- Potentially trigger fulfillment for another member
Per coding guidelines: "Authorization checks in every procedure."
🛡️ Ownership check
const memberId = paymentIntent.metadata.member_id ?? ""; + // Verify ownership: the PaymentIntent must belong to the current user + const owner = await db + .select({ userId: Member.userId }) + .from(Member) + .where(eq(Member.id, memberId)) + .limit(1); + + if (owner.length === 0 || owner[0].userId !== ctx.session.user.id) { + throw new TRPCError({ + code: "FORBIDDEN", + message: "Payment does not belong to the current user.", + }); + } + const billingYear = new Date().getFullYear();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 143 - 144, Retrieve the current user id from ctx.session.user (e.g., ctx.session.user.id) and enforce ownership by comparing it to paymentIntent.metadata.member_id (the memberId variable) after fetching the PaymentIntent in the procedure; if they differ, throw an authorization error (e.g., TRPCError with code "FORBIDDEN") and stop processing so the receipt_email and any fulfillment actions are only accessible for the owning user.
🧹 Nitpick comments (1)
packages/api/src/routers/dues-payment.ts (1)
91-98: Use the already-extractedmemberIdvariable for consistency.Line 96 re-accesses
member[0]?.idwhen you've already safely extractedmemberIdon line 78. Using the existing variable improves readability and avoids redundant optional chaining.🔧 Suggested fix
const paymentIntent = await stripe.paymentIntents.create({ amount: price, currency: "usd", payment_method_types: ["card", "us_bank_account"], metadata: { - member_id: member[0]?.id ?? "", + member_id: memberId, }, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 91 - 98, Replace the redundant member[0]?.id usage with the already-extracted memberId when creating the Stripe PaymentIntent; in the payment creation call (stripe.paymentIntents.create / paymentIntent) use metadata: { member_id: memberId ?? "" } so the code consistently references the memberId variable and avoids repeated optional chaining.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx`:
- Around line 43-64: The terminalFailureStatuses array in dues-payment.ts
currently omits "requires_capture", allowing Stripe payment intents that require
capture to be treated as successful; update the terminalFailureStatuses constant
(used in the payment result validation logic in dues-payment.ts) to include
"requires_capture" so that these intents are treated as failures and trigger the
existing error path (preventing membership creation and allowing the frontend
isError handling to redirect).
- Around line 27-31: The current early-return branch that calls toast.error and
router.push during render should be moved into a useEffect to avoid side effects
during render; update the logic around paymentIntentId in
membership-success-page.tsx so that when paymentIntentId is falsy you call
toast.error(...) and router.replace(SIGN_IN_PATH) from inside a useEffect
(dependent on paymentIntentId and router) and still return null from the
component render until navigation occurs; replace router.push with
router.replace to avoid leaving the invalid URL in history.
In `@packages/api/src/routers/dues-payment.ts`:
- Around line 134-141: The early return for PaymentIntent with status
"processing" exposes receipt_email before verifying ownership; after retrieving
paymentIntent in the handler (the paymentIntent variable returned from Stripe),
immediately perform the authorization check by comparing
paymentIntent.metadata.member_id to ctx.session.user (or the appropriate user id
field) and throw/return an unauthorized error if they don't match, before any
returns that expose email or other sensitive fields—apply this change to the
branch that currently returns the object for status === "processing" so the
ownership check runs first.
---
Duplicate comments:
In `@packages/api/src/routers/dues-payment.ts`:
- Around line 143-144: Retrieve the current user id from ctx.session.user (e.g.,
ctx.session.user.id) and enforce ownership by comparing it to
paymentIntent.metadata.member_id (the memberId variable) after fetching the
PaymentIntent in the procedure; if they differ, throw an authorization error
(e.g., TRPCError with code "FORBIDDEN") and stop processing so the receipt_email
and any fulfillment actions are only accessible for the owning user.
---
Nitpick comments:
In `@packages/api/src/routers/dues-payment.ts`:
- Around line 91-98: Replace the redundant member[0]?.id usage with the
already-extracted memberId when creating the Stripe PaymentIntent; in the
payment creation call (stripe.paymentIntents.create / paymentIntent) use
metadata: { member_id: memberId ?? "" } so the code consistently references the
memberId variable and avoids repeated optional chaining.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 704e0632-e3d5-4bf5-801d-41a4eb65121c
📒 Files selected for processing (2)
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsxpackages/api/src/routers/dues-payment.ts
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx
Show resolved
Hide resolved
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx
Show resolved
Hide resolved
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 (1)
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx (1)
39-43:⚠️ Potential issue | 🟡 MinorSide effects during render: move
isErrorhandling intouseEffect.The
toast.error()androuter.push()calls here execute during render, which can fire multiple times as React reconciles. The!paymentIntentIdcase was correctly moved touseEffect, but thisisErrorcase was missed.This causes duplicate error toasts and potential race conditions with navigation.
Move into the existing useEffect
useEffect(() => { if (!paymentIntentId) { toast.error("Invalid confirmation link."); router.replace(SIGN_IN_PATH); + return; + } + + if (isError) { + toast.error("Something went wrong, please contact support."); + router.replace(SIGN_IN_PATH); } - }, [paymentIntentId, router]); + }, [paymentIntentId, isError, router]); if (!paymentIntentId) { return null; } - if (isError) { - toast.error("Something went wrong, please contact support."); - router.push(SIGN_IN_PATH); + if (isError) { return null; }Also note: use
router.replace()instead ofrouter.push()to prevent the broken URL from staying in browser history (consistent with thepaymentIntentIdcase).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx` around lines 39 - 43, The isError branch currently calls toast.error(...) and router.push(...) during render; move this logic into the existing useEffect that handles !paymentIntentId so side effects run only after mount, and replace router.push with router.replace to avoid leaving the broken URL in history; locate the isError check and relocate its toast.error and navigation calls into the same useEffect (keeping the toast message and using router.replace), ensuring the component returns null or fallback UI during render while effects handle the notifications/navigation.
🧹 Nitpick comments (2)
packages/api/src/routers/dues-payment.ts (2)
64-100: Well-structured PaymentIntent creation with proper guards.This implementation correctly:
- Validates membership before creating the intent
- Scopes the existing dues check to the current billing year (line 83)
- Stores
member_idin metadata for later verification- Returns the client secret for frontend use
One small defensive improvement:
paymentIntent.client_secretcan benullin Stripe's TypeScript definitions (though practically it's always present on creation). Consider a guard or assertion.Optional defensive check
const paymentIntent = await stripe.paymentIntents.create({ amount: price, currency: "usd", payment_method_types: ["card", "us_bank_account"], metadata: { member_id: memberId, }, }); + + if (!paymentIntent.client_secret) { + throw new TRPCError({ + code: "INTERNAL_SERVER_ERROR", + message: "Failed to create payment intent.", + }); + } + return { clientSecret: paymentIntent.client_secret };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 64 - 100, In createPaymentIntent (protectedProcedure.mutation) add a defensive check after stripe.paymentIntents.create: verify paymentIntent.client_secret is non-null and if it is null throw a TRPCError (e.g., INTERNAL) with a clear message (or otherwise handle/assert) before returning { clientSecret: paymentIntent.client_secret }; this ensures the createPaymentIntent flow fails loudly if Stripe returns a null client_secret and avoids returning an unexpected null to the frontend.
174-176: Consider formatting the amount in dollars for readability.The Discord log currently shows
paymentIntent.amountin cents (e.g.,2500). For staff reviewing logs, dollars would be clearer.Suggested format
if (inserted.length > 0) { await discord.log({ - message: `A member has successfully paid their dues. ${paymentIntent.amount}`, + message: `A member has successfully paid their dues. $${(paymentIntent.amount / 100).toFixed(2)}`, title: "Dues Paid", color: "success_green", userId: ctx.session.user.discordUserId, }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/api/src/routers/dues-payment.ts` around lines 174 - 176, The Discord log is showing paymentIntent.amount in cents; update the discord.log call (where paymentIntent.amount is used) to convert cents to dollars and format for readability (e.g., divide by 100 and format to two decimal places or use Intl.NumberFormat for USD) so the message shows a dollar amount like "$25.00" instead of "2500"; keep the log title "Dues Paid" and include the formatted amount in the message string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@apps/blade/src/app/_components/dashboard/member/membership-success-page.tsx`:
- Around line 39-43: The isError branch currently calls toast.error(...) and
router.push(...) during render; move this logic into the existing useEffect that
handles !paymentIntentId so side effects run only after mount, and replace
router.push with router.replace to avoid leaving the broken URL in history;
locate the isError check and relocate its toast.error and navigation calls into
the same useEffect (keeping the toast message and using router.replace),
ensuring the component returns null or fallback UI during render while effects
handle the notifications/navigation.
---
Nitpick comments:
In `@packages/api/src/routers/dues-payment.ts`:
- Around line 64-100: In createPaymentIntent (protectedProcedure.mutation) add a
defensive check after stripe.paymentIntents.create: verify
paymentIntent.client_secret is non-null and if it is null throw a TRPCError
(e.g., INTERNAL) with a clear message (or otherwise handle/assert) before
returning { clientSecret: paymentIntent.client_secret }; this ensures the
createPaymentIntent flow fails loudly if Stripe returns a null client_secret and
avoids returning an unexpected null to the frontend.
- Around line 174-176: The Discord log is showing paymentIntent.amount in cents;
update the discord.log call (where paymentIntent.amount is used) to convert
cents to dollars and format for readability (e.g., divide by 100 and format to
two decimal places or use Intl.NumberFormat for USD) so the message shows a
dollar amount like "$25.00" instead of "2500"; keep the log title "Dues Paid"
and include the formatted amount in the message string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 2a473477-5bfe-44ae-8a5e-9eec9ecf92cf
📒 Files selected for processing (2)
apps/blade/src/app/_components/dashboard/member/membership-success-page.tsxpackages/api/src/routers/dues-payment.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Why
To update theming and appearance of stripe dashboard so that it integrates with blade's aesthetic. It also transitions us from a checkout session based payment portal (legacy) to embedded (stripe elements)
What
Closes: #321
Additional Notes
Test Plan
Checklist
db:pushbefore mergingSummary by CodeRabbit
New Features
Improvements
Bug Fixes