feat(checkout): three-step checkout (PR #194 variant B) + payment-flow battle-test harness#196
Conversation
…t harness Implements the checkout design picked in PR #194 (variant B) and builds the launch test harness around it. Checkout v2 (Account → Billing → Payment, collapsing steps + sticky summary): - Inline account step: signed-out buyers register (or sign in on 409 ACCOUNT_EXISTS) without leaving checkout; middleware no longer gates /pro/checkout (cart APIs still enforce auth server-side); OAuth stays available via /login with redirect-back. - Billing address step persisted to the Medusa cart (billing_address passes through the existing PATCH route). - Payment step: Stripe Express Checkout wallets on top; radio accordion with Card default/expanded and PayPal + Bitcoin as first-class rows. All provider components (CheckoutForm/PayPalButton/BTCPayButton) and the entire checkout-safety machinery (order_pending protection, payment_uncertain persistence, owner alerting) are reused unchanged; wallets confirm through the same safety path as the card form. - No blocking init spinner: the form renders instantly and the cart initializes in the background (billing submit awaits it if needed). Battle-test harness (mocked, runs in CI): - Mock backend now covers the full money path: emailpass register/login, customer creation, dynamic personas, cart completion that mints orders with license metadata, per-provider session data, a BTCPay invoice simulator (redirect → pay → webhook-settle → success), and failure injections (fail-session+, order-pending+, fail-complete+ emails). - e2e/pro-checkout-journeys.spec.ts: new-customer registers inline and buys with Bitcoin end-to-end, license appears in the new account; existing-email sign-in flip; wrong-password stays put; signed-in BTC purchase; order-pending blocks checkout across reloads; session failure surfaces; API-level provider matrix (stripe/paypal/btcpay complete → order + license metadata; order_pending 409 contract; 5xx completion never reports success); fresh account reads its own licenses via the API. - pro-checkout.spec.ts reworked for the stepper; auth/middleware specs updated to pin the new no-login-bounce contract; @integration Stripe spec updated to drive the new UI (billing step, card row). Validation: 915 unit tests, 71/71 mocked e2e (chromium), lint clean, production build via the e2e webServer.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
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:
📝 WalkthroughWalkthroughCheckout now uses inline account, billing, and payment steps, with cart ownership checks, same-origin auth guards, BTCPay support, and updated mock backend and E2E coverage. ChangesInline checkout redesign
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
🚀 Preview: https://wcpos-548g9gs2f-wcpos.vercel.app |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a04b40c6e0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
Review-fix triage before code changes:
Skipped threads: none at triage time; both unresolved threads are actionable and planned for fixes. |
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Review-thread fixes pushed in f4168af.
Excluded threads:
Validation:
|
|
🚀 Preview: https://wcpos-41t76iguh-wcpos.vercel.app |
|
Follow-up PR-fix status after fresh review-thread inventory (2026-07-02T00:10:57Z): unresolved review threads remaining: 0.
Excluded threads:
Current validation state:
|
|
Review-fix triage after fixes:
Skipped threads: none. Fresh unresolved inline review-thread inventory returned 0. |
|
Follow-up PR-fix status after a fresh unresolved-thread inventory: unresolved inline review threads remaining: 0. No additional code changes were needed in this round. The PR head is still f4168af, which already contains the single batched fix commit for the two actionable review threads.
Excluded threads:
Current blocker: GitHub Actions and CodeQL are passing, but the CodeRabbit status context is still pending, so GitHub reports |
|
Follow-up PR-fix status after fresh unresolved-thread inventory: unresolved inline review threads remaining: 0. No additional code changes were needed in this round. The PR head remains f4168af, which already contains the single batched fix commit for the two actionable review threads.
Excluded threads:
Validation in this round:
Current blocker: GitHub Actions and CodeQL are passing, and review threads are resolved, but the external |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/pro/checkout-client.tsx (1)
109-131: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
resolvePaymentSessionskips the legacy singular fallback wheneverpayment_sessionsis non-empty, even if no match is found.The first branch (
payment_collection.payment_sessions) correctly only returns when a match is actuallyfound(Line 116-118,if (collectionSession) { return collectionSession }). The second branch instead gates on array length, not on whether.find()actually matched:if (cart.payment_sessions?.length) { return cart.payment_sessions.find( (session) => session.provider_id === providerId ) }If
cart.payment_sessionsis non-empty but contains no session forproviderId, this returnsundefinedimmediately — the subsequentcart.payment_session(singular) fallback check on Line 126 is never reached, even though it might hold the matching session for that provider. This directly affects PayPal/BTCPay checkout-link resolution (paypalSession/btcpaySessionat Lines 474-479), and none of the current unit tests combine a populatedpayment_sessionsarray with apayment_sessionsingular field, so this gap isn't caught by the suite.🐛 Suggested fix
if (cart.payment_sessions?.length) { - return cart.payment_sessions.find( + const legacySession = cart.payment_sessions.find( (session) => session.provider_id === providerId ) + if (legacySession) { + return legacySession + } } if (cart.payment_session?.provider_id === providerId) { return cart.payment_session } return undefined🤖 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 `@src/components/pro/checkout-client.tsx` around lines 109 - 131, `resolvePaymentSession` returns too early from the `payment_sessions` array branch and skips the legacy `payment_session` fallback when no provider match is found. Update `resolvePaymentSession` in `checkout-client.tsx` so it only returns from `cart.payment_sessions` when `.find()` actually matches, then falls through to the singular `cart.payment_session` check. Make sure the `paypalSession` and `btcpaySession` lookup paths still resolve correctly when only the legacy singular session matches.
🧹 Nitpick comments (8)
src/components/pro/checkout-client.tsx (1)
90-94: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low value
DEFAULT_PAYMENT_METHODsilently falls back to'btcpay'even when no provider is enabled.If
isStripeEnabled,isPayPalEnabled, andisBTCPayEnabledare allfalse(misconfiguration), this still resolves to'btcpay'andinitializeCheckoutwill attemptcreatePaymentSessionfor a provider that isn't actually configured, producing a confusing "Failed to initialize payment" error instead of surfacing the real "no payment methods configured" state thatPaymentStepalready handles gracefully (Lines 114-120 ofpayment-step.tsx).🤖 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 `@src/components/pro/checkout-client.tsx` around lines 90 - 94, The DEFAULT_PAYMENT_METHOD fallback in checkout-client.tsx is masking the misconfiguration case by resolving to 'btcpay' even when no payment provider is enabled. Update the PaymentMethod selection logic near DEFAULT_PAYMENT_METHOD so it only chooses a provider that is actually enabled, and make the no-provider case explicit instead of defaulting to BTCPay. Then ensure initializeCheckout uses that signal to avoid calling createPaymentSession for an unavailable provider and let the existing PaymentStep “no payment methods configured” handling remain the user-facing path.src/app/[locale]/(main)/pro/checkout/page.tsx (1)
138-155: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSuspense fallback skeleton no longer matches the new checkout layout.
The fallback still renders a 2-panel
md:grid-cols-2skeleton left over from the old two-column design, while the actual loaded content is now a 3-step list plus a sticky summary in a[1.6fr_1fr]grid (percheckout-client.tsxLine 519). This is a brief, self-correcting visual mismatch during the loading flash, but worth aligning for polish.🤖 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 `@src/app/`[locale]/(main)/pro/checkout/page.tsx around lines 138 - 155, The Suspense fallback in CheckoutContent no longer matches the current checkout layout. Update the fallback skeleton inside the checkout page’s Suspense block to mirror the new structure from CheckoutContent/checkout-client.tsx: a 3-step list area plus a sticky summary column in the same [1.6fr_1fr] grid. Remove the old md:grid-cols-2 two-panel skeleton and align the Skeleton placeholders with the new checkout sections so the loading state matches the rendered UI.src/components/pro/checkout/step-shell.tsx (1)
63-71: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win"Edit" buttons lack contextual accessible names.
editLabelis rendered as the sole visible/accessible text for the button (Line 69). SinceCheckoutClientpasseseditLabel="Edit"for both the Account and Billing steps, a screen reader user navigating by buttons will hear two indistinguishable "Edit" controls with no indication of what each edits.♻️ Suggested fix
<button type="button" onClick={onEdit} + aria-label={`Edit ${title}`} className="text-sm text-muted-foreground underline underline-offset-4" > {editLabel} </button>🤖 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 `@src/components/pro/checkout/step-shell.tsx` around lines 63 - 71, The StepShell edit button uses only editLabel, so both Account and Billing render as indistinguishable “Edit” controls. Update the button in StepShell to include a contextual accessible name derived from the step or section title while keeping the visible label unchanged, using the existing onEdit/editLabel props and the step context passed into this component.src/components/pro/checkout/express-checkout.tsx (1)
44-117: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winKeep a local in-flight guard around
handleConfirm. TheExpressCheckoutElementconfirm flow is intended as a single-trigger event, but Stripe does not promise your handler won’t be re-entered by the UI or app state, and the docs still recommend preventing duplicate submissions yourself. This checkout path should mirror the existingisProcessingprotection used in the card flow.🤖 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 `@src/components/pro/checkout/express-checkout.tsx` around lines 44 - 117, The `handleConfirm` flow in `ExpressCheckoutElement` can be re-entered, so add a local in-flight guard like the `isProcessing` protection used in the card checkout path. Track a boolean state or ref in `ExpressCheckout` and short-circuit `handleConfirm` when a confirm is already running, then set/reset it around the `stripe.confirmPayment` and `completeProviderConfirmedCheckout` work so duplicate submissions are prevented.e2e/mocks/server.mjs (1)
319-338: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winCustomer creation mutates the shared fixture object for fixture personas.
personaForRequestreturns a direct reference intofixtures.personas[base]for non-dynamic tokens. If this route is ever hit by a fixture persona whose.customeris stillnull(currently unlikely since named fixtures already carry a customer),auth.persona.customer = {...}permanently mutates the shared, process-wide fixture object rather than a per-test copy — leaking state into every later test that resolves the same fixture persona.Cheap to future-proof: only mutate a stored copy for dynamic personas, or deep-clone fixture personas before returning them from
personaForRequest.🛡️ Suggested defensive fix
function personaForRequest(req) { const auth = req.headers.authorization || '' const token = auth.startsWith('Bearer ') ? auth.slice('Bearer '.length) : '' if (!token) return null const dynamic = dynamicPersonas.get(token) if (dynamic) return { persona: dynamic, suffix: null } const { base, suffix } = splitSuffix(token) - const persona = fixtures.personas[base] + const persona = fixtures.personas[base] if (!persona) return null - return { persona, suffix } + // Never let request handlers mutate the shared fixture singleton. + return { persona: structuredClone(persona), suffix } }🤖 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 `@e2e/mocks/server.mjs` around lines 319 - 338, The customer creation handler in the server mock mutates the shared fixture persona returned by personaForRequest, which can leak state across tests. Update personaForRequest and/or the /store/customers POST handling so only dynamic personas are mutated, or return a cloned persona for fixture-backed identities before assigning auth.persona.customer. Keep the fix localized around personaForRequest, auth.persona.customer, and the customer creation route.e2e/pro-checkout-integration.spec.ts (1)
52-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the new
completeBillingStephelper instead of re-duplicating the fill sequence.Lines 64-74 duplicate
completeBillingStepfrome2e/helpers/checkout.tsfield-for-field (same locators, same "Continue to payment" click, same step-3 wait). None of this file's assertions depend on the specific name/address values entered, so the helper can be reused directly.♻️ Proposed refactor
-import { test, expect } from '`@playwright/test`' +import { test, expect } from '`@playwright/test`' +import { completeBillingStep } from './helpers/checkout'await expect(page.getByTestId('checkout-steps')).toBeVisible({ timeout: 30000, }) - // Billing address step - await expect(page.getByTestId('billing-step-form')).toBeVisible({ - timeout: 15000, - }) - await page.getByLabel('First name').fill('E2E') - await page.getByLabel('Last name').fill('Tester') - await page.getByLabel('Address').fill('1 Integration Way') - await page.getByLabel('City').fill('Sydney') - await page.getByLabel('Postal code').fill('2000') - await page.getByLabel('Country').selectOption('au') - await page.getByRole('button', { name: /continue to payment/i }).click() + // Billing address step + await completeBillingStep(page)🤖 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 `@e2e/pro-checkout-integration.spec.ts` around lines 52 - 81, The billing-step interaction in this checkout test duplicates the existing completeBillingStep helper logic instead of reusing it. Replace the repeated field fills and continue-click flow in the e2e/pro-checkout-integration.spec.ts scenario with a call to completeBillingStep from e2e/helpers/checkout.ts, and keep only the assertions that are specific to this test, such as the payment-method verification.e2e/pro-checkout-journeys.spec.ts (2)
129-186: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert registration succeeded before exercising the sign-in-fallback flow.
Lines 135-137 and 166-168 seed the "existing account" state via
page.request.post('/api/auth/register', ...)without checking the response, unlikeregisterViaApi(lines 74-83) which does. If seeding silently fails, the lateraccount-exists-noticeassertion fails with a misleading root cause.♻️ Proposed fix
- await page.request.post('/api/auth/register', { - data: { email, password: PASSWORD }, - }) + const seed = await page.request.post('/api/auth/register', { + data: { email, password: PASSWORD }, + }) + expect(seed.status()).toBe(200)(apply the same change at the other seeding call site)
🤖 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 `@e2e/pro-checkout-journeys.spec.ts` around lines 129 - 186, The two account-seeding calls in the checkout journey tests do not verify that registration succeeded before testing the sign-in fallback flow. Update the setup in the affected specs to assert the `/api/auth/register` response is successful, following the same pattern used by `registerViaApi`, so failures are caught at the seeding step rather than later in the `account-exists-notice` or sign-in assertions.
248-260: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCentralize the checkout trigger literals
fail-session+,order-pending+,fail-complete+,PURC-HASE-FLOW-7777, andlic-e2e-purchaseare duplicated acrosse2e/pro-checkout-journeys.spec.ts,e2e/mocks/server.mjs, ande2e/mocks/fixtures.json. Move them to shared e2e constants so the mock/test contract can’t drift.🤖 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 `@e2e/pro-checkout-journeys.spec.ts` around lines 248 - 260, Centralize the checkout trigger literals used by this test flow so the mock and spec stay in sync; the duplicated values like fail-session+, order-pending+, fail-complete+, PURC-HASE-FLOW-7777, and lic-e2e-purchase should be moved into shared e2e constants. Update the references in pro-checkout-journeys.spec.ts, mocks/server.mjs, and mocks/fixtures.json to consume the shared constants, and keep the test behavior in the payment-session failure case wired through those symbols rather than inline strings.
🤖 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 `@e2e/mocks/server.mjs`:
- Around line 535-568: The cart completion mock in the
/store/carts/{id}/complete handler is not idempotent because it always calls
completeCartIntoOrder(cart), which can mint duplicate orders on retries. Update
this path to short-circuit when the cart already has an order_id, matching the
retry guard used in the BTCPay pay flow, and return the existing completed
result instead of creating a new order.
In `@src/components/pro/checkout-client.tsx`:
- Around line 378-414: handleBillingSubmit currently treats billing save and
payment-session refresh as one failure path, so a createPaymentSession error is
surfaced as a billing-save error. Split the two phases in handleBillingSubmit so
the PATCH to /api/store/cart and the createPaymentSession call are handled
separately, and wrap the payment refresh failure with a distinct message before
it bubbles to BillingStep. Keep the success path setting cart,
paymentCollectionId, clientSecret, and step only after both phases succeed.
---
Outside diff comments:
In `@src/components/pro/checkout-client.tsx`:
- Around line 109-131: `resolvePaymentSession` returns too early from the
`payment_sessions` array branch and skips the legacy `payment_session` fallback
when no provider match is found. Update `resolvePaymentSession` in
`checkout-client.tsx` so it only returns from `cart.payment_sessions` when
`.find()` actually matches, then falls through to the singular
`cart.payment_session` check. Make sure the `paypalSession` and `btcpaySession`
lookup paths still resolve correctly when only the legacy singular session
matches.
---
Nitpick comments:
In `@e2e/mocks/server.mjs`:
- Around line 319-338: The customer creation handler in the server mock mutates
the shared fixture persona returned by personaForRequest, which can leak state
across tests. Update personaForRequest and/or the /store/customers POST handling
so only dynamic personas are mutated, or return a cloned persona for
fixture-backed identities before assigning auth.persona.customer. Keep the fix
localized around personaForRequest, auth.persona.customer, and the customer
creation route.
In `@e2e/pro-checkout-integration.spec.ts`:
- Around line 52-81: The billing-step interaction in this checkout test
duplicates the existing completeBillingStep helper logic instead of reusing it.
Replace the repeated field fills and continue-click flow in the
e2e/pro-checkout-integration.spec.ts scenario with a call to completeBillingStep
from e2e/helpers/checkout.ts, and keep only the assertions that are specific to
this test, such as the payment-method verification.
In `@e2e/pro-checkout-journeys.spec.ts`:
- Around line 129-186: The two account-seeding calls in the checkout journey
tests do not verify that registration succeeded before testing the sign-in
fallback flow. Update the setup in the affected specs to assert the
`/api/auth/register` response is successful, following the same pattern used by
`registerViaApi`, so failures are caught at the seeding step rather than later
in the `account-exists-notice` or sign-in assertions.
- Around line 248-260: Centralize the checkout trigger literals used by this
test flow so the mock and spec stay in sync; the duplicated values like
fail-session+, order-pending+, fail-complete+, PURC-HASE-FLOW-7777, and
lic-e2e-purchase should be moved into shared e2e constants. Update the
references in pro-checkout-journeys.spec.ts, mocks/server.mjs, and
mocks/fixtures.json to consume the shared constants, and keep the test behavior
in the payment-session failure case wired through those symbols rather than
inline strings.
In `@src/app/`[locale]/(main)/pro/checkout/page.tsx:
- Around line 138-155: The Suspense fallback in CheckoutContent no longer
matches the current checkout layout. Update the fallback skeleton inside the
checkout page’s Suspense block to mirror the new structure from
CheckoutContent/checkout-client.tsx: a 3-step list area plus a sticky summary
column in the same [1.6fr_1fr] grid. Remove the old md:grid-cols-2 two-panel
skeleton and align the Skeleton placeholders with the new checkout sections so
the loading state matches the rendered UI.
In `@src/components/pro/checkout-client.tsx`:
- Around line 90-94: The DEFAULT_PAYMENT_METHOD fallback in checkout-client.tsx
is masking the misconfiguration case by resolving to 'btcpay' even when no
payment provider is enabled. Update the PaymentMethod selection logic near
DEFAULT_PAYMENT_METHOD so it only chooses a provider that is actually enabled,
and make the no-provider case explicit instead of defaulting to BTCPay. Then
ensure initializeCheckout uses that signal to avoid calling createPaymentSession
for an unavailable provider and let the existing PaymentStep “no payment methods
configured” handling remain the user-facing path.
In `@src/components/pro/checkout/express-checkout.tsx`:
- Around line 44-117: The `handleConfirm` flow in `ExpressCheckoutElement` can
be re-entered, so add a local in-flight guard like the `isProcessing` protection
used in the card checkout path. Track a boolean state or ref in
`ExpressCheckout` and short-circuit `handleConfirm` when a confirm is already
running, then set/reset it around the `stripe.confirmPayment` and
`completeProviderConfirmedCheckout` work so duplicate submissions are prevented.
In `@src/components/pro/checkout/step-shell.tsx`:
- Around line 63-71: The StepShell edit button uses only editLabel, so both
Account and Billing render as indistinguishable “Edit” controls. Update the
button in StepShell to include a contextual accessible name derived from the
step or section title while keeping the visible label unchanged, using the
existing onEdit/editLabel props and the step context passed into this component.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 149d547f-13c4-4976-a5fb-39efbb8192f4
📒 Files selected for processing (17)
e2e/auth.spec.tse2e/helpers/checkout.tse2e/mocks/server.mjse2e/pro-checkout-integration.spec.tse2e/pro-checkout-journeys.spec.tse2e/pro-checkout.spec.tsplaywright.config.tssrc/app/[locale]/(main)/pro/checkout/page.tsxsrc/components/pro/checkout-client.test.tsxsrc/components/pro/checkout-client.tsxsrc/components/pro/checkout/account-step.tsxsrc/components/pro/checkout/billing-step.tsxsrc/components/pro/checkout/express-checkout.tsxsrc/components/pro/checkout/payment-step.tsxsrc/components/pro/checkout/step-shell.tsxsrc/middleware.test.tssrc/middleware.ts
|
Round 3 review-fix triage before code changes:
Review-body outside-diff finding:
Skipped threads: none among the fresh unresolved inline thread inventory. |
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Round 3 review-fix status after fresh unresolved-thread inventory: unresolved inline review threads remaining: 0.
Excluded threads:
Validation:
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
e2e/pro-checkout.spec.ts (1)
142-188: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueIdempotency test looks solid.
Retry-completion coverage against the mock backend correctly asserts stable
id/display_idacross duplicatecompletecalls. One optional addition: also assertretriedCompletedoesn't create a second cart/order side effect (e.g., checking cart status or a distinct completion count) if the mock server exposes that, to catch regressions where IDs match but duplicate state is created underneath.🤖 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 `@e2e/pro-checkout.spec.ts` around lines 142 - 188, The retry-completion test in e2e/pro-checkout.spec.ts already verifies the same order id and display_id, but it should also cover the absence of duplicate side effects. Update the test around `retriedComplete` in `returns the same order when cart completion is retried` to assert an additional observable state from the mock backend, such as unchanged cart status or a completion counter, if available, so the `complete` endpoint’s idempotency is validated beyond matching order fields.
🤖 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.
Nitpick comments:
In `@e2e/pro-checkout.spec.ts`:
- Around line 142-188: The retry-completion test in e2e/pro-checkout.spec.ts
already verifies the same order id and display_id, but it should also cover the
absence of duplicate side effects. Update the test around `retriedComplete` in
`returns the same order when cart completion is retried` to assert an additional
observable state from the mock backend, such as unchanged cart status or a
completion counter, if available, so the `complete` endpoint’s idempotency is
validated beyond matching order fields.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: df98e44d-16f5-4e14-91b0-4ab793ec1929
📒 Files selected for processing (5)
e2e/mocks/server.mjse2e/pro-checkout.spec.tssrc/components/pro/checkout-client.test.tsxsrc/components/pro/checkout-client.tsxsrc/components/pro/checkout/billing-step.tsx
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/pro/checkout/billing-step.tsx
- e2e/mocks/server.mjs
- src/components/pro/checkout-client.test.tsx
- src/components/pro/checkout-client.tsx
|
🚀 Preview: https://wcpos-m6kf0npm9-wcpos.vercel.app |
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Review-body fix pushed in 5242aa6. Fresh post-push unresolved inline review-thread inventory: 0.
Excluded threads:
Validation:
Final PR state after polling: mergeable, |
|
🚀 Preview: https://wcpos-l1mh585oi-wcpos.vercel.app |
|
Post-creation review (partial): the full 3-agent adversarial pass hit the session token limit mid-run (resumes after 04:10); I completed the highest-risk checks inline and fixed two findings in the latest commit:
Also verified inline (no change needed): safety-restore ordering gates cart init correctly (a restored After validation: 915 unit / 71/71 e2e still green. A full multi-angle review pass will run once the token window resets. |
…e-submit guard Inline review findings: a restored payment_uncertain warning rendered only inside the payment step, so a customer on billing wouldn't see it until step 3 — the notice now renders above the steps from first paint (unit test pins visible-before-billing). The Express Checkout wallet confirm path gains the same in-flight guard the card form has.
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Overnight status (final for this window): branch now carries the full v2 + harness + both review rounds (mine inline + the agents bot's session-refresh/idempotency work, rebased together). Combined validation: 918 unit tests / 73 e2e passing, tsc + eslint clean. Remaining before launch (needs credentials or a human):
Optional hardening flagged during review: rate limiting on /api/auth/register and /api/auth/login (pre-existing gap, more exposed now that checkout links to them). |
|
🚀 Preview: https://wcpos-499ioe6mf-wcpos.vercel.app |
…aces, cart binding Full multi-angle review of the checkout diff surfaced and fixes: LAUNCH-BLOCKING: inline registration persisted Medusa's registration token, whose JWT has an empty actor_id until re-auth — on real Medusa every brand-new customer's session was dead at cart creation while the mocked suite passed (the mock accepted registration tokens as sessions). register() now exchanges for a session token via login (the same refresh-after-linking invariant the OAuth flow enforces), and the mock now mirrors real token semantics (registration tokens can only create the customer; persona reads require a login-minted session token) so this class of bug can never pass CI green again. Races (two finders independently): handleBillingSubmit read the payment collection id from React state that hasn't rendered when billing beats the background init — minting a SECOND payment collection (customer pays intent A, completion authorizes collection B → paid-but-no-order). The init promise now carries the full init result. Session mutations are serialized (billing refresh + method switch share the isProcessing lock), provider content shows a preparing state during mutations (no paying a superseded BTCPay invoice), billing Edit and method rows lock while a provider confirmation may be charging (onProcessingChange from CheckoutForm + ExpressCheckoutRow), and Stripe Elements is keyed by client secret so a refreshed session can't leave the form confirming a stale intent. Hardening: cart routes now bind carts to the caller (cart.email must match the session customer on GET/PATCH/line-items/payment-sessions/ complete); PATCH whitelists billing_address instead of spreading arbitrary body fields into Medusa; login/register reject cross-origin browser POSTs (login-CSRF); the account step's sign-in failure copy points OAuth-only customers at the OAuth path instead of dead-ending them at a password they don't have. Tests updated to pin the new contracts: register token exchange, cart-ownership fixtures, retry-idempotency spec now performs the login exchange. 918 unit / 73 e2e green.
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
|
Full adversarial review complete (3 finder angles + verification, applied in The big one (would have broken launch)Inline registration produced a dead session on real Medusa. Money-path races (confirmed by two independent finders)
Hardening
Verified-no-change-needed / follow-ups (non-blocking)
With this, the mocked matrix + safety machinery are as battle-tested as they can be without real keys. The credential-dependent items from the earlier comment remain the pre-launch checklist. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/lib/medusa-auth.test.ts (1)
90-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for a Step 3 (login-exchange) failure.
The happy path is now well covered, but there's no test for
login()failing after the customer is already created in Step 2 — the new edge case this three-step flow introduces. Worth a test assertingregister()still throws/handles this distinctly (see companion comment onmedusa-auth.ts).🤖 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 `@src/lib/medusa-auth.test.ts` around lines 90 - 141, Add a test for the new Step 3 login-exchange failure path in the register() flow. In src/lib/medusa-auth.test.ts, extend the register() coverage to mock a successful auth identity creation and customer creation, then make the third fetch call for the login() exchange fail. Assert that register() throws or surfaces the login failure distinctly, and verify the flow stops after the failed login attempt rather than returning a session token.
🤖 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 `@src/app/api/store/cart/payment-sessions/route.ts`:
- Around line 66-70: The payment session route is trusting the caller’s
paymentCollectionId instead of the cart’s own payment collection. In the
payment-sessions route, update the logic around createPaymentSession to compare
the provided paymentCollectionId against currentCart.payment_collection?.id, or
derive the ID directly from currentCart before using it. Keep the existing
cart/email validation intact, and reject mismatched or stale collection IDs
before creating the session.
In `@src/app/api/store/cart/route.ts`:
- Around line 118-119: The PATCH handler in cart route should validate the
parsed request body before destructuring, since request.json() can fail or
return null and non-string cartId values can reach getCart/updateCart. Update
the PATCH flow in the route handler to mirror the safer object/type guard used
by the line-items route, then only read cartId after confirming the body is a
non-null object with a string cartId and return a client error otherwise; apply
the same validation pattern to the related cart update path referenced by the
comment.
In `@src/lib/medusa-auth.ts`:
- Around line 209-217: The registration flow in medusa-auth’s customer creation
path returns a plain failure when the Step 3 login exchange throws, leaving an
already-created customer behind. Wrap the `login(email, password)` call in the
registration flow so failures are converted into a distinct typed error or
actionable message that callers can use to route to the sign-in fallback, while
keeping the existing `AccountExistsError` handling unchanged.
---
Nitpick comments:
In `@src/lib/medusa-auth.test.ts`:
- Around line 90-141: Add a test for the new Step 3 login-exchange failure path
in the register() flow. In src/lib/medusa-auth.test.ts, extend the register()
coverage to mock a successful auth identity creation and customer creation, then
make the third fetch call for the login() exchange fail. Assert that register()
throws or surfaces the login failure distinctly, and verify the flow stops after
the failed login attempt rather than returning a session token.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 2c43aa13-ac80-4a28-b411-9fc83c9a26a2
📒 Files selected for processing (19)
e2e/mocks/server.mjse2e/pro-checkout.spec.tssrc/app/api/auth/login/route.tssrc/app/api/auth/register/route.tssrc/app/api/store/cart/complete/route.test.tssrc/app/api/store/cart/complete/route.tssrc/app/api/store/cart/line-items/route.test.tssrc/app/api/store/cart/line-items/route.tssrc/app/api/store/cart/payment-sessions/route.tssrc/app/api/store/cart/route.tssrc/components/pro/checkout-client.tsxsrc/components/pro/checkout-form.tsxsrc/components/pro/checkout/account-step.tsxsrc/components/pro/checkout/express-checkout.tsxsrc/components/pro/checkout/payment-step.tsxsrc/components/pro/stripe-provider.tsxsrc/lib/api/same-origin.tssrc/lib/medusa-auth.test.tssrc/lib/medusa-auth.ts
✅ Files skipped from review due to trivial changes (1)
- src/lib/api/same-origin.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/components/pro/checkout/express-checkout.tsx
- e2e/pro-checkout.spec.ts
- e2e/mocks/server.mjs
- src/components/pro/checkout/payment-step.tsx
|
🚀 Preview: https://wcpos-l7lmnyw5v-wcpos.vercel.app |
/fix triage — round 1CI is green; 4 earlier threads already fixed+resolved by the repo bot. Remaining findings:
Greptile: no findings (trial credit limit). Codex: both inline findings previously fixed in f4168af. |
Review: a caller-supplied paymentCollectionId was passed straight to createPaymentSession — a stale or mismatched id could attach sessions to the wrong collection. The route now prefers the cart's own collection and rejects mismatched ids (MedusaCart type gains the v2 payment_collection field).
Review: request.json() failures / null bodies surfaced as 500s and non-string cartId could reach getCart. Mirrors the line-items guard.
… fails Review: if step 3 (login) fails after the customer exists, the generic 'registration failed' error invited a confusing retry into ACCOUNT_EXISTS. The customer is now told the account was created and to sign in.
Review nit: with no NEXT_PUBLIC payment env at all, init attempted a session for an unconfigured provider and failed with a misleading error; PaymentStep's explicit no-methods state now surfaces instead (both at init and after a billing save).
…c hygiene - Checkout Suspense fallback mirrors the 3-step + summary layout - Step Edit buttons get contextual aria-labels (two identical 'Edit' controls were indistinguishable to screen readers) - Integration spec reuses the shared completeBillingStep helper - Journeys spec asserts its account-seeding register responses - Failure-injection prefixes + purchase-license literals move to a shared e2e/mocks/constants.mjs consumed by both the mock server and the specs (the strings are the mock<->spec contract)
There was a problem hiding this comment.
kilbot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
/fix round 1 — completeAll 11 triage items closed (commits
All three open CodeRabbit threads replied + resolved. Validation after fixes: 918 unit / 73 e2e passing, tsc + eslint clean. Waiting on CI for the new commits; after that the PR needs only human approval (plus the credential-dependent pre-launch checklist from earlier comments). |
|
🚀 Preview: https://wcpos-1fusynrwl-wcpos.vercel.app |
1 participant
Summary
Two things in one coherent change, built for the launch in two days:
Checkout v2
billing_addressthrough the existing PATCH route), collapsible/editable.The battle-test harness
Mock backend additions (
e2e/mocks/server.mjs): emailpass register/login + customer creation (dynamic personas), cart completion that mints orders with license metadata, provider-shaped session data, a BTCPay invoice simulator (redirect → simulate payment → webhook-settles the order → returns to success), and deterministic failure injections keyed by email prefix (fail-session+,order-pending+,fail-complete+).Coverage now proven in CI (71/71 mocked e2e, chromium):
pp_stripe_stripe,pp_paypal_paypal,pp_btcpay_btcpay→ real order + license metadataorder_pendingwire contract; Medusa 5xx never reports successWhat still needs real credentials (can't be mocked honestly)
e2e/pro-checkout-integration.spec.ts(@integration) updated to drive the new UI — needsNEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY+E2E_TEST_EMAIL/PASSWORD+MEDUSA_API_KEYagainst staging. Wallets additionally need a real device (manual: open checkout on a phone with Apple/Google Pay in test mode).NEXT_PUBLIC_STRIPE_PUBLISHABLE_KEY,NEXT_PUBLIC_PAYPAL_CLIENT_ID,NEXT_PUBLIC_BTCPAY_ENABLEDare set for production so all three rows render.Validation
Notes
main; feat(pro): buy-box pricing section — feature list once, term choice as radio rows #195 (buy-box pricing) doesn't touch checkout, so these merge independently.Summary by CodeRabbit