Close payment-gateway render loop and repair gift-card/coupon submit

packages/react-components/docs/adr/0001-payment-source-effect-invariants.md

@@ -0,0 +1,68 @@
+# ADR 0001 — Payment-source effect invariants
+
+- Status: Accepted
+- Component: `src/components/payment_gateways/PaymentGateway.tsx`
+- Reducer: `src/reducers/PaymentMethodReducer.ts` (`setPaymentSource`)
+- Related issue: [#803](https://github.com/commercelayer/commercelayer-react-components/issues/803)
+
+## Context
+
+`<PaymentGateway>` runs an effect that reconciles the order's payment source: it
+creates or recreates a source when the current one is missing, mismatched, or wrong
+for the selected method, and toggles the loader. This effect has repeatedly
+regressed into infinite render loops — React 19's stricter update-depth accounting
+turns what used to be an intermittent extra-render nuisance into a hard
+`Maximum update depth exceeded` crash.
+
+Two independent forces cause the loops:
+
+1. **Object-identity dependencies.** The effect used to depend on whole `order`,
+   `paymentSource`, `errors`, and `config` objects. A `getCustomerPaymentSources()`
+   refetch mints new object identities for the same data, which re-fired the effect
+   even though nothing meaningful changed.
+2. **No-op passes that still re-armed the effect.** The single-payment-method branch
+   only recreated a source when it was `null` or when there were errors — never when an
+   existing source had `mismatched_amounts: true`. So a mismatched single-method source
+   ran the reconcile helper, recreated nothing, yet still called
+   `getCustomerPaymentSources()`, whose refetch churned identities and re-fired the
+   effect indefinitely (issue #803).
+
+## Decision
+
+The effect obeys three invariants. Changing any of them has historically reintroduced
+a loop — treat them as load-bearing.
+
+1. **Reactive triggers are stable ids/scalars only — never whole objects.**
+   The driving `useEffect` depends on selectors such as `order?.payment_source?.id`,
+   `order?.payment_source?.mismatched_amounts`, `paymentSource?.id`,
+   `errors?.length`, `paymentMethods?.length`, `order?.status`. The reconcile logic
+   itself lives in a `useEffectEvent` (`onPaymentSync`) that reads the *latest*
+   `order`/`config`/`paymentSource` on each call, so those objects never need to be
+   dependencies and their identity churn cannot re-fire the effect. Because the
+   trigger set is now hand-curated (the linter cannot infer it from a `useEffectEvent`
+   body), **any new field the reconcile logic branches on must be added here as a
+   selector**, or the effect will silently stop reacting to it.
+
+2. **Recreate on a mismatched single-method source, and only refetch after a real
+   (re)create.** The single-method branch recreates when the source is absent
+   (`== null`), there are errors, **or** the existing source is mismatched.
+   `getCustomerPaymentSources()` fires only when a source was actually (re)created
+   (the `recreated` flag), so a no-op pass can never re-arm the effect.
+
+3. **Two layers of concurrency protection, kept in sync.**
+   - `settingPaymentSourceRef` in the component skips a whole reconcile pass while a
+     previous fire-and-forget request is still in flight.
+   - `inFlightPaymentSourceRequests` in `setPaymentSource` coalesces genuinely
+     concurrent duplicate calls, keyed per order/resource/operation, and removes the
+     entry once settled so a later legitimate re-create still runs.
+
+## Consequences
+
+- The single-method mismatched-amounts loop (#803) is closed: the mismatch is now a
+  reactive trigger, the source is genuinely recreated, `mismatched_amounts` flips to
+  `false`, and the effect settles.
+- The `useEffect` dependency array is no longer a mechanical "everything the body
+  reads" list — it is a deliberate trigger set. Reviewers must reason about triggers,
+  not rely solely on `useExhaustiveDependencies`.
+- Regression coverage lives in `specs/payment_gateways/PaymentGateway.spec.tsx`; the
+  deterministic loop reproduction lives in the checkout e2e suite.

packages/react-components/specs/gift_cards/GiftCardOrCouponForm.integration.spec.tsx

@@ -0,0 +1,66 @@
+// Integration coverage that exercises the REAL rapid-form wiring (unlike
+// GiftCardOrCouponForm.spec.tsx, which mocks `rapid-form` and injects `values`
+// directly). rapid-form v4 auto-discovers a form field only when it is `required`
+// OR has a validation config; consumers such as mfe-checkout render the input with
+// `required={false}`, which used to leave the field untracked so `values[type]`
+// stayed undefined and submit never called the API. The form now registers a
+// pass-through validation for its active field to force tracking.
+import { fireEvent, render, screen } from "@testing-library/react"
+import { GiftCardOrCouponForm } from "#components/gift_cards/GiftCardOrCouponForm"
+import { GiftCardOrCouponInput } from "#components/gift_cards/GiftCardOrCouponInput"
+import { GiftCardOrCouponSubmit } from "#components/gift_cards/GiftCardOrCouponSubmit"
+import OrderContext, { defaultOrderContext } from "#context/OrderContext"
+
+function renderForm(required: boolean) {
+  const setGiftCardOrCouponCode = vi.fn(async () => ({ success: true, order: undefined }))
+  render(
+    // biome-ignore lint/suspicious/noExplicitAny: test provider cast
+    <OrderContext.Provider
+      value={
+        {
+          ...defaultOrderContext,
+          order: { id: "order-1", gift_card_code: "", coupon_code: "" },
+          errors: [],
+          setGiftCardOrCouponCode,
+          setOrderErrors: vi.fn(),
+          // biome-ignore lint/suspicious/noExplicitAny: test provider cast
+        } as any
+      }
+    >
+      <GiftCardOrCouponForm>
+        <GiftCardOrCouponInput data-testid="input" required={required} />
+        <GiftCardOrCouponSubmit data-testid="submit" />
+      </GiftCardOrCouponForm>
+    </OrderContext.Provider>
+  )
+  return setGiftCardOrCouponCode
+}
+
+describe("GiftCardOrCouponForm real rapid-form wiring", () => {
+  it("submits a non-required field (mfe-checkout renders required={false})", async () => {
+    const setGiftCardOrCouponCode = renderForm(false)
+    fireEvent.input(screen.getByTestId("input"), { target: { value: "SUMMER10" } })
+    fireEvent.click(screen.getByTestId("submit"))
+    await Promise.resolve()
+    expect(setGiftCardOrCouponCode).toHaveBeenCalledWith(
+      expect.objectContaining({ code: "SUMMER10", codeType: "gift_card_or_coupon_code" })
+    )
+  })
+
+  it("submits a required field", async () => {
+    const setGiftCardOrCouponCode = renderForm(true)
+    fireEvent.input(screen.getByTestId("input"), { target: { value: "SUMMER10" } })
+    fireEvent.click(screen.getByTestId("submit"))
+    await Promise.resolve()
+    expect(setGiftCardOrCouponCode).toHaveBeenCalledWith(
+      expect.objectContaining({ code: "SUMMER10" })
+    )
+  })
+
+  it("does not call the API when submitting without typing", async () => {
+    const setGiftCardOrCouponCode = renderForm(false)
+    fireEvent.click(screen.getByTestId("submit"))
+    await Promise.resolve()
+    expect(setGiftCardOrCouponCode).not.toHaveBeenCalled()
+  })
+})

packages/react-components/specs/gift_cards/GiftCardOrCouponForm.spec.tsx

@@ -1,5 +1,5 @@
 import { fireEvent, render, screen, waitFor } from "@testing-library/react"
-import { useContext } from "react"
+import { type ReactNode, useContext } from "react"
 import { GiftCardOrCouponForm } from "#components/gift_cards/GiftCardOrCouponForm"
 import CouponAndGiftCardFormContext from "#context/CouponAndGiftCardFormContext"
 import OrderContext, { defaultOrderContext } from "#context/OrderContext"
@@ -21,10 +21,12 @@ function renderComponent({
   codeType,
   onSubmit = vi.fn(),
   orderContextOverrides = {},
+  children = <CodeTypeProbe />,
 }: {
   codeType?: "coupon_code" | "gift_card_code"
   onSubmit?: ReturnType<typeof vi.fn>
   orderContextOverrides?: Record<string, unknown>
+  children?: ReactNode
 }) {
   const setGiftCardOrCouponCode = vi.fn()
   const setOrderErrors = vi.fn()
@@ -45,7 +47,7 @@ function renderComponent({
     // biome-ignore lint/suspicious/noExplicitAny: test provider cast
     <OrderContext.Provider value={orderContext as any}>
       <GiftCardOrCouponForm codeType={codeType} onSubmit={onSubmit} data-testid="gift-card-form">
-        <CodeTypeProbe />
+        {children}
       </GiftCardOrCouponForm>
     </OrderContext.Provider>
   )
@@ -179,7 +181,7 @@ describe("GiftCardOrCouponForm", () => {
     })
   })
 
-  it("clears order errors to an empty list when none are present", async () => {
+  it("does not dispatch when the active field is empty and there is nothing to clear", async () => {
     const setOrderErrors = vi.fn()
     rapidForm.useRapidForm.mockReturnValue({
       refValidation: vi.fn(),
@@ -196,7 +198,33 @@ describe("GiftCardOrCouponForm", () => {
       },
     })
 
-    await waitFor(() => expect(setOrderErrors).toHaveBeenCalledWith([]))
+    // No errors to strip → dispatching a fresh [] would only churn `errors`' identity
+    // and re-fire the effect forever (React 19 "Maximum update depth exceeded").
+    await new Promise((resolve) => setTimeout(resolve, 0))
+    expect(setOrderErrors).not.toHaveBeenCalled()
+  })
+
+  it("does not dispatch when every existing error already belongs to the active field", async () => {
+    const setOrderErrors = vi.fn()
+    rapidForm.useRapidForm.mockReturnValue({
+      refValidation: vi.fn(),
+      values: {
+        coupon_code: { value: "" },
+      },
+    })
+
+    renderComponent({
+      codeType: "coupon_code",
+      orderContextOverrides: {
+        // Filtering keeps all of these → same contents → no-op. This is the settled
+        // state the loop must converge to; a fresh-array dispatch here would spin.
+        errors: [{ code: "VALIDATION_ERROR", message: "coupon", field: "coupon_code" }],
+        setOrderErrors,
+      },
+    })
+
+    await new Promise((resolve) => setTimeout(resolve, 0))
+    expect(setOrderErrors).not.toHaveBeenCalled()
   })
 
   it("returns early on submit when the current field value is missing", async () => {
@@ -250,6 +278,8 @@ describe("GiftCardOrCouponForm", () => {
     const { container, setGiftCardOrCouponCode } = renderComponent({
       codeType: "coupon_code",
       onSubmit,
+      // handleSubmit reads the code from the form DOM, not rapid-form's values.
+      children: <input name="coupon_code" defaultValue="SAVE10" />,
     })
     setGiftCardOrCouponCode.mockResolvedValue({ success: true, order: updatedOrder })
 
@@ -288,6 +318,8 @@ describe("GiftCardOrCouponForm", () => {
       orderContextOverrides: {
         order: { id: "order-1", gift_card_code: "", coupon_code: "" },
       },
+      // handleSubmit reads the code from the form DOM, not rapid-form's values.
+      children: <input name="gift_card_code" defaultValue="GC-123" />,
     })
     setGiftCardOrCouponCode.mockResolvedValue({ success: false, order: undefined })
 

packages/react-components/specs/payment_gateways/PaymentGateway.spec.tsx

@@ -19,14 +19,44 @@ function makeOrder() {
   }
 }
 
-const getCustomerPaymentSources = vi.fn()
+// Single-payment-method order whose existing source no longer matches the total
+// (e.g. a coupon lowered it): `payment_source.mismatched_amounts === true`. This is
+// the #803 shape that used to loop.
+function makeMismatchedOrder() {
+  return {
+    id: "order-1",
+    status: "pending",
+    payment_method: { id: "pm-1", payment_source_type: "stripe_payments" },
+    payment_source: { id: "ps-1", mismatched_amounts: true },
+  }
+}
+
+// Same order once the source has been recreated for the current total.
+function makeSettledOrder() {
+  return {
+    id: "order-1",
+    status: "pending",
+    payment_method: { id: "pm-1", payment_source_type: "stripe_payments" },
+    payment_source: { id: "ps-2", mismatched_amounts: false },
+  }
+}
+
+const noopGetCustomerPaymentSources = vi.fn()
 
 function Tree({
   order,
   setPaymentSource,
+  paymentSource = null,
+  paymentMethods = [{ id: "pm-1" }],
+  errors = [],
+  getCustomerPaymentSources = noopGetCustomerPaymentSources,
 }: {
   order: any
   setPaymentSource: (...args: any[]) => Promise<unknown>
+  paymentSource?: any
+  paymentMethods?: any
+  errors?: any
+  getCustomerPaymentSources?: (...args: any[]) => unknown
 }): ReactElement {
   return (
     // biome-ignore lint/suspicious/noExplicitAny: test cast
@@ -51,9 +81,9 @@ function Tree({
                   currentPaymentMethodType: "stripe_payments",
                   config: null,
                   setPaymentSource,
-                  paymentSource: null,
-                  paymentMethods: [{ id: "pm-1" }],
-                  errors: [],
+                  paymentSource,
+                  paymentMethods,
+                  errors,
                   // biome-ignore lint/suspicious/noExplicitAny: test cast
                 } as any
               }
@@ -86,3 +116,61 @@ describe("PaymentGateway in-flight guard", () => {
     expect(setPaymentSource).toHaveBeenCalledTimes(1)
   })
 })
+
+// Regression coverage for #803: a single-payment-method order whose existing source is
+// mismatched used to spin forever (recreate nothing → refetch → new identities → repeat).
+describe("PaymentGateway single-method mismatched source (#803)", () => {
+  it("recreates the source when a single-method order's source is mismatched", () => {
+    const setPaymentSource = vi.fn(() => new Promise<unknown>(() => {}))
+
+    render(
+      <Tree
+        order={makeMismatchedOrder()}
+        setPaymentSource={setPaymentSource}
+        // The context source is mismatched too — this is what invokes the reconcile pass.
+        paymentSource={{ id: "ps-1", type: "stripe_payments", mismatched_amounts: true }}
+      />
+    )
+
+    // Before the fix the single-method branch matched neither condition and never
+    // recreated; now the mismatched source is genuinely recreated.
+    expect(setPaymentSource).toHaveBeenCalledTimes(1)
+  })
+
+  it("does not recreate a settled single-method source", () => {
+    const setPaymentSource = vi.fn(() => new Promise<unknown>(() => {}))
+
+    render(
+      <Tree
+        order={makeSettledOrder()}
+        setPaymentSource={setPaymentSource}
+        paymentSource={{ id: "ps-2", type: "stripe_payments", mismatched_amounts: false }}
+      />
+    )
+
+    // Source is present, matches, and is not mismatched → nothing to recreate. This is
+    // the settled state the loop must converge to.
+    expect(setPaymentSource).not.toHaveBeenCalled()
+  })
+
+  it("does not refetch customer sources on a no-op reconcile pass", () => {
+    const setPaymentSource = vi.fn(() => new Promise<unknown>(() => {}))
+    const getCustomerPaymentSources = vi.fn()
+
+    render(
+      <Tree
+        order={makeSettledOrder()}
+        setPaymentSource={setPaymentSource}
+        // Context source flags mismatched (invoking the reconcile pass) while the order's
+        // source is already settled — the exact no-op pass that used to re-arm the effect.
+        paymentSource={{ id: "ps-2", type: "stripe_payments", mismatched_amounts: true }}
+        getCustomerPaymentSources={getCustomerPaymentSources}
+      />
+    )
+
+    // Nothing was recreated, so the customer-sources refetch — the loop's engine — must
+    // not fire (Hardening #1).
+    expect(setPaymentSource).not.toHaveBeenCalled()
+    expect(getCustomerPaymentSources).not.toHaveBeenCalled()
+  })
+})