diff --git a/packages/react-components/docs/adr/0001-payment-source-effect-invariants.md b/packages/react-components/docs/adr/0001-payment-source-effect-invariants.md new file mode 100644 index 00000000..fbd5dcde --- /dev/null +++ b/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 + +`` 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. diff --git a/packages/react-components/specs/gift_cards/GiftCardOrCouponForm.integration.spec.tsx b/packages/react-components/specs/gift_cards/GiftCardOrCouponForm.integration.spec.tsx new file mode 100644 index 00000000..d851910e --- /dev/null +++ b/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 + + + + + + + ) + 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() + }) +}) diff --git a/packages/react-components/specs/gift_cards/GiftCardOrCouponForm.spec.tsx b/packages/react-components/specs/gift_cards/GiftCardOrCouponForm.spec.tsx index 2922225a..9a5240de 100644 --- a/packages/react-components/specs/gift_cards/GiftCardOrCouponForm.spec.tsx +++ b/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 = , }: { codeType?: "coupon_code" | "gift_card_code" onSubmit?: ReturnType orderContextOverrides?: Record + children?: ReactNode }) { const setGiftCardOrCouponCode = vi.fn() const setOrderErrors = vi.fn() @@ -45,7 +47,7 @@ function renderComponent({ // biome-ignore lint/suspicious/noExplicitAny: test provider cast - + {children} ) @@ -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: , }) 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: , }) setGiftCardOrCouponCode.mockResolvedValue({ success: false, order: undefined }) diff --git a/packages/react-components/specs/payment_gateways/PaymentGateway.spec.tsx b/packages/react-components/specs/payment_gateways/PaymentGateway.spec.tsx index ffb70220..003d55a5 100644 --- a/packages/react-components/specs/payment_gateways/PaymentGateway.spec.tsx +++ b/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 + 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(() => {})) + + render( + + ) + + // 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(() => {})) + + render( + + ) + + // 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(() => {})) + const getCustomerPaymentSources = vi.fn() + + render( + + ) + + // 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() + }) +}) diff --git a/packages/react-components/src/components/gift_cards/GiftCardOrCouponForm.tsx b/packages/react-components/src/components/gift_cards/GiftCardOrCouponForm.tsx index 054de766..a1f98030 100644 --- a/packages/react-components/src/components/gift_cards/GiftCardOrCouponForm.tsx +++ b/packages/react-components/src/components/gift_cards/GiftCardOrCouponForm.tsx @@ -22,10 +22,17 @@ export function GiftCardOrCouponForm(props: Props): JSX.Element | null { const { setGiftCardOrCouponCode, order, errors, setOrderErrors } = useContext(OrderContext) const [type, setType] = useState(codeType) - // Propagate or clear order errors when the field is empty + // When the active field is emptied, drop the *other* fields' errors from the order. useEffect(() => { if (type == null || values[type]?.value !== "") return - const fieldErrors = (errors ?? []).filter((e) => e.field === type) + const current = errors ?? [] + const fieldErrors = current.filter((e) => e.field === type) + // Bail when filtering removes nothing. `fieldErrors` is an order-preserving subset, + // so equal lengths mean identical contents — dispatching it would only mint a fresh + // `errors` array reference, which this effect depends on, re-firing it forever + // (React 19 hard-crashes with "Maximum update depth exceeded"). Same identity-churn + // failure class as docs/adr/0001-payment-source-effect-invariants.md. + if (fieldErrors.length === current.length) return setOrderErrors(fieldErrors) onSubmit?.({ value: "", success: false }) }, [values, errors, type, setOrderErrors, onSubmit]) @@ -48,9 +55,16 @@ export function GiftCardOrCouponForm(props: Props): JSX.Element | null { const handleSubmit = useCallback( async (e: React.SyntheticEvent): Promise => { e.preventDefault() - if (type == null || values[type] == null || setGiftCardOrCouponCode == null) return + if (type == null || setGiftCardOrCouponCode == null) return const form = e.currentTarget - const code = values[type].value + // Read the typed code straight from the form DOM rather than rapid-form's tracked + // `values`. rapid-form v4 only tracks a field that is `required` or has a validation + // config, so a `required={false}` field (e.g. mfe-checkout) may never be tracked and + // `values[type]` stays undefined — which silently made submit a no-op. The form + // element is authoritative and available here regardless of any wiring/timing. + const field = form.elements.namedItem(type) as HTMLInputElement | null + const code = field?.value?.trim() ?? "" + if (code === "") return const { success, order: updatedOrder } = await setGiftCardOrCouponCode({ code, // "gift_card_or_coupon_code" is accepted by the CL API at runtime @@ -59,7 +73,7 @@ export function GiftCardOrCouponForm(props: Props): JSX.Element | null { onSubmit?.({ success, value: code, order: updatedOrder }) if (success) form.reset() }, - [type, values, setGiftCardOrCouponCode, onSubmit] + [type, setGiftCardOrCouponCode, onSubmit] ) if (codeType != null && order?.[codeType] != null && order?.[codeType] !== "") { @@ -67,7 +81,30 @@ export function GiftCardOrCouponForm(props: Props): JSX.Element | null { } return (order?.gift_card_code && order?.coupon_code) || order == null ? null : ( -
+ { + refValidation( + node, + type != null + ? { resetOnSubmit: false, validations: { [type]: { validation: () => true } } } + : { resetOnSubmit: false } + ) + }} + autoComplete={autoComplete} + onSubmit={handleSubmit} + {...p} + > {children}
diff --git a/packages/react-components/src/components/payment_gateways/PaymentGateway.tsx b/packages/react-components/src/components/payment_gateways/PaymentGateway.tsx index 1876d570..3eb56117 100644 --- a/packages/react-components/src/components/payment_gateways/PaymentGateway.tsx +++ b/packages/react-components/src/components/payment_gateways/PaymentGateway.tsx @@ -1,4 +1,4 @@ -import { type JSX, useContext, useEffect, useRef, useState } from "react" +import { type JSX, useContext, useEffect, useEffectEvent, useRef, useState } from "react" import CustomerContext from "#context/CustomerContext" import OrderContext from "#context/OrderContext" import PaymentMethodChildrenContext from "#context/PaymentMethodChildrenContext" @@ -51,7 +51,8 @@ export function PaymentGateway({ const loaderComponent = getLoaderComponent(loader) const [loading, setLoading] = useState(true) // Guards against the effect re-entering and firing a second `setPaymentSource` - // before the first (fire-and-forget) request resolves and settles state. See ADR 0001. + // before the first (fire-and-forget) request resolves and settles state. + // See docs/adr/0001-payment-source-effect-invariants.md. const settingPaymentSourceRef = useRef(false) const { payment, expressPayments } = useContext(PaymentMethodChildrenContext) const { order } = useContext(OrderContext) @@ -69,7 +70,15 @@ export function PaymentGateway({ const paymentResource = readonly ? currentPaymentMethodType : (payment?.payment_source_type as PaymentResource) - useEffect(() => { + + // Non-reactive reconcile pass. It reads the *latest* `order`, `config`, `paymentSource`, + // etc. on every invocation, so those objects are deliberately absent from the driving + // effect's dependency array below — only stable id/scalar selectors are. This is what + // ends the mismatched-amounts loop: a customer-sources refetch that only mints new + // `order`/`paymentSource` object identities no longer re-fires the effect, while a real + // field change (a flipped `mismatched_amounts`, a new source id, a status transition) + // still does. See docs/adr/0001-payment-source-effect-invariants.md. + const onPaymentSync = useEffectEvent((): void => { if ( payment?.id === currentPaymentMethodId && paymentResource && @@ -97,6 +106,10 @@ export function PaymentGateway({ // effect run must not fire a second create/update before state settles. if (settingPaymentSourceRef.current) return settingPaymentSourceRef.current = true + // Only refetch customer sources when a source was actually (re)created; the + // unconditional refetch on a no-op pass was what re-armed the effect and drove + // the single-method mismatched-amounts loop. + let recreated = false try { if (order != null && paymentMethods && paymentMethods?.length > 1) { await setPaymentSource({ @@ -104,9 +117,13 @@ export function PaymentGateway({ order, attributes, }) + recreated = true } if ( - ((errors != null && errors?.length > 0) || order?.payment_source === null) && + ((errors != null && errors?.length > 0) || + order?.payment_source == null || + // @ts-expect-error no type + order?.payment_source?.mismatched_amounts) && paymentMethods && paymentMethods?.length === 1 ) { @@ -115,11 +132,12 @@ export function PaymentGateway({ order, attributes, }) + recreated = true } } finally { settingPaymentSourceRef.current = false } - if (getCustomerPaymentSources) getCustomerPaymentSources() + if (recreated && getCustomerPaymentSources) getCustomerPaymentSources() } if (!paymentSource && order?.payment_method.id && show && !expressPayments) { setPaymentSources() @@ -149,31 +167,35 @@ export function PaymentGateway({ ) { setLoading(false) } - // No cleanup: setLoading(true) in cleanup caused unnecessary extra re-renders. + }) + + // The array below is a deliberate trigger set, not the effect body's reads — + // `onPaymentSync` (a useEffectEvent) reads the latest values, so the linter sees these + // deps as "more than necessary". See docs/adr/0001-payment-source-effect-invariants.md. + // biome-ignore lint/correctness/useExhaustiveDependencies: deliberate trigger set, see ADR 0001 + useEffect(() => { + onPaymentSync() + // Reactive triggers only: stable ids/scalars, never whole objects. `order` and `config` + // are read latest inside `onPaymentSync`, so listing their identities here would re-fire + // the effect on meaningless refetch churn. See docs/adr/0001-payment-source-effect-invariants.md. }, [ order?.payment_method?.id, - show, - paymentSource?.id, + order?.payment_method?.payment_source_type, order?.status, + order?.payment_source?.id, // @ts-expect-error no type - paymentSource?.mismatched_amounts, + order?.payment_source?.mismatched_amounts, + paymentSource?.id, paymentSource?.type, - paymentSource, + // @ts-expect-error no type + paymentSource?.mismatched_amounts, paymentResource, payment?.id, - setPaymentSource, - order?.payment_source?.id, - order?.payment_source, - order?.payment_method?.payment_source_type, - order, - getCustomerPaymentSources, - paymentMethods?.length, - paymentMethods, currentPaymentMethodId, expressPayments, + show, errors?.length, - errors, - config, + paymentMethods?.length, ]) useEffect(() => { diff --git a/packages/react-components/src/reducers/PaymentMethodReducer.ts b/packages/react-components/src/reducers/PaymentMethodReducer.ts index 4cbad2e7..979cc7f6 100644 --- a/packages/react-components/src/reducers/PaymentMethodReducer.ts +++ b/packages/react-components/src/reducers/PaymentMethodReducer.ts @@ -291,7 +291,8 @@ export interface SetPaymentSourceParams extends Omit