Refactor payment methods: standalone components without deprecated containers#796
Open
acasazza wants to merge 36 commits into
Open
Refactor payment methods: standalone components without deprecated containers#796acasazza wants to merge 36 commits into
acasazza wants to merge 36 commits into
Conversation
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Add overrides for fast-uri, @babel/plugin-transform-modules-systemjs, js-cookie, postcss, qs, uuid, axios, vitest, handlebars, lodash - Fix msgpackr-extract allowBuilds placeholder - Result: 0 high/critical vulnerabilities (5 moderate devDeps only)
- Extract getFormElement + shared types to addressFormUtils.ts - Extract standalone address reducer/context to useStandaloneAddress hook - Extract form field effects (validation, reset, checkbox, includes) to useAddressFormFields hook - Slim BillingAddressForm: 416 → 113 lines (73% reduction) - Slim ShippingAddressForm: 423 → 115 lines (73% reduction) - Add unit tests: saveOrderAddresses (15), useAddressForm (12), standalone form modes (+19)
…te loop BillingAddressForm and ShippingAddressForm were wrapping the memoized setValue from useAddressFormFields in a new arrow function on every render. This created a new function reference each cycle, which AddressInput detected as a dependency change in its useEffect, triggering setState, which caused the 'Maximum update depth exceeded' loop. Pass the memoized setValue directly to the context provider instead. Add stability regression test to BillingAddressForm spec.
…r edit When editing an existing address, AddressInput calls setValue for each field individually. Previously, each call dispatched setAddress with only that single field, replacing the entire billing_address/shipping_address in the reducer state (each dispatch lost all previously set fields). Two fixes in useAddressFormFields: - setValue now reads ALL form inputs before dispatching so each call accumulates the full address instead of overwriting it. - The main validation effect now supplements rapid-form tracked values (required fields only) with non-required fields read from the DOM, so optional fields like phone/line_2 are preserved when the user edits any required field.
…ptures programmatic updates
rapid-form attaches 'input' event listeners (not 'change') to required form
elements. setValue and resetField were dispatching 'change', which meant
rapid-form never updated its internal formValues when a value was set
programmatically (e.g., pre-filling from an existing address).
This caused AddressStateSelector to read an empty country code from
billingAddress.values and fall back to a plain text input instead of
showing the state/province select.
Fix: dispatch 'input' in both setValue and resetField.
Also adds an integration test proving that ctx.values is updated after
setValue('billing_address_country_code', 'US').
…ropdown The BaseSelect (rendered when a country with states is selected) had no onChange handler, so picking a state from the dropdown never updated val or called billingAddress.setValue / shippingAddress.setValue. The state_code was therefore never written into the address context. Added onChange handler mirroring the existing BaseInput handler. Added regression test covering this path.
…dressStateSelector When editing an existing address, the country code is pre-filled via setValue, which triggers changeBillingCountry=true (countryCode was '' → 'US'). The state written into the form context. Fix: distinguish initial country detection (countryCode was empty) from a user-initiated country change. On initial detection, pre-fill the state from the value prop. On user-initiated change, reset the state only if the current value is invalid for the new country (existing behavior, now guarded by Added regression test.
…d in AddressStateSelector
BaseSelect uses defaultValue (uncontrolled), so val must be correct at the
exact moment the select first mounts. If no value prop is passed to
AddressStateSelector but the text input was pre-filled externally via a
billingAddress.setValue call (e.g. from AddressInput), val remained '' and
the select showed empty.
Fix: add a ref to the BaseInput so its current DOM value can be read when
the country is first detected (isFirstCountryDetection). The state value
is resolved in priority order: value prop > DOM text input value > ''.
This handles two pre-fill scenarios:
1. Explicit value prop (e.g. value={existingAddress.state_code})
2. External setValue call setting the DOM value before country arrives
Added regression tests for both scenarios.
…ingAddressFormContext so AddressStateSelector can detect country regardless of required attribute AddressStateSelector watches billingAddress.values[country_key] to detect the country and load state options. billingAddress.values was only rapid-form's formValues, which only tracks required fields. If the country input was not required (or used without rapid-form capturing the input event), formValues never got billing_address_country_code, countryCode stayed '' and the Italian (or other) state options never loaded. Fix: BillingAddressForm and ShippingAddressForm now merge the address reducer values (which are always updated by setValue via setAddress, regardless of the required attribute) into the form context values alongside rapid-form's tracked values. Rapid-form values (Value objects) take precedence. AddressStateSelector already handles both formats: typeof v === 'string' ? v : v?.value
…cer values access
- Add errorMode='inline'|'submit' prop to both address form components - 'inline' (default): errors shown as user types (existing behavior) - 'submit': errors suppressed until SaveAddressesButton is clicked; after first validation, errors update live as user corrects fields - Add validate() function exposed via form context for manual triggering - SaveAddressesButton calls validate() before saving when errorMode='submit' - Export ErrorMode type from BillingAddressFormContext - Add 10 tests covering new behavior
… paths, and branch gaps
- Add test for line 117 false branch: billing context with no setValue, value prop changes while internal val differs (typed input) - Refactor lines 126 and 160 to split the unreachable ?? "" fallback onto a separate line with v8 ignore, making branch coverage trackable - Both rawStateValue expressions (billing and shipping) are covered by existing pre-fill and external-setValue tests
When useShipments returns a new shipments array reference on every render (e.g. due to unstable hook args), the useEffect would re-run on every render and call setErrors() which triggered another re-render, causing 'Maximum update depth exceeded'. Two root causes fixed: 1. Cleanup function setErrors([]) ran before every effect re-execution, scheduling an extra state update that compounded the loop. 2. setErrors(nextErrors) always scheduled a state update even when the computed errors were identical to the previous value. Fix: remove the cleanup (errors are always recomputed from current deps), and use a functional setErrors updater that returns prev unchanged when error codes have not changed, breaking the re-render cycle.
Shipment.tsx listed setShippingMethod in its useEffect deps, but setShippingMethod was an inline async function recreated on every render of Shipments.tsx. Each re-render of Shipments provided a new function reference via ShipmentContext, causing Shipment's useEffect to re-run, which triggered the cleanup setLoading(true) + setLoading(false), re-rendering Shipment and consuming the changed context again — infinite 'Maximum update depth exceeded' loop. Two root causes fixed: Shipments.tsx: - Wrap setShippingMethod with useCallback so its reference is stable across re-renders (only changes when order/orderId/getOrder change) - Wrap setShipmentErrors with useCallback for the same reason - Memoize contextValue with useMemo to prevent all ShipmentContext consumers from re-rendering on every Shipments render Shipment.tsx: - Remove cleanup setLoading(true): same spurious-state-update pattern as the previous Shipments.tsx fix; the new effect always sets the correct loading state without needing a reset on cleanup - Remove redundant shipments?.length dep (shipments already covers it)
After calling setShippingMethod, the order updates in OrderContext which recreates the setShippingMethod callback (it depends on order). The new reference triggered the autoSelect useEffect to re-run before SWR refetches shipments, so shipping_method still appeared null and setShippingMethod was called again — infinite loop. - Shipment: access setShippingMethod via a ref so it is not a dep of the autoSelect effect; effect now re-runs only when shipments or autoSelectSingleShippingMethod changes - Shipment: extract ShipmentItem component that memoises the ShipmentChildrenContext value, stabilising deliveryLeadTimes for ShippingMethod and preventing unnecessary consumer re-renders - ShippingMethod: remove setItems([]) cleanup; with stable context deps the cleanup is never needed and caused extra renders
The second useEffect had 'loading' in its deps and called setLoading inside the body, while the cleanup always reset setLoading(true). This created an infinite toggle loop: setLoading(false) → loading dep changes → cleanup setLoading(true) → loading dep changes → setLoading(false) → repeat. - Remove 'loading' from second effect deps (React bails out on same primitive value, so the && loading guards were unnecessary) - Remove 'order' from second effect deps in favour of order?.status - Remove both cleanup functions (same pattern fixed in Shipments, Shipment and ShippingMethod)
getPayMethods was defined as an inline async function inside the component body, making it a new reference on every render. Because it was listed as a useEffect dependency, the effect re-ran on every render. While state.paymentMethods was still unset (between async dispatches true and getPaymentMethods was called again — infinite loop. - Remove the getPayMethods wrapper function entirely - Call getPaymentMethods directly inside the effect - Remove getPayMethods from the dependency array
fetchOrder(state.order) was called inside useMemo, which runs during the render phase. When fetchOrder updated external state (e.g. AppProvider), React logged: 'Cannot update a component while rendering a different component'. - Move the fetchOrder call into a useEffect with [fetchOrder, state.order] deps - Remove fetchOrder from useMemo deps (it is no longer referenced inside it)
Introduces usePaymentMethod hook that encapsulates the reducer, includes setup, and payment method fetching currently owned by PaymentMethodsContainer. PaymentMethod now works in two modes: - Standalone (no container parent): calls usePaymentMethod internally and wraps its children with PaymentMethodContext.Provider — no container needed. Pass gateway config via the new config prop on PaymentMethod. - Container mode (existing): detects the parent PaymentMethodsContainer via the new _isProvided marker on PaymentMethodContext and uses it as before. PaymentMethodsContainer is kept for backwards compatibility with a @deprecated JSDoc notice; it will be removed in the next major version. Closes #795 (partial — payment_gateways and payment_source still pending)
PaymentMethodsContainer (backward compat): - renders children, provides _isProvided in context - populates paymentMethods from order, calls addResourceToInclude PaymentMethod standalone mode: - detects standalone via _isProvided absence, provides context to children - populates paymentMethods, calls addResourceToInclude on mount - skips addResourceToInclude when includes already loaded - renders one div per payment method after effects settle PaymentMethod container mode: - reads paymentMethods from parent container context - standalone hook does not call addResourceToInclude when isStandalone=false - _isProvided preserved through the tree Regression: does not trigger 'Maximum update depth exceeded'
…ner and usePaymentMethod - Refactor PaymentMethod.tsx: replace module-level loadingResource variable with useRef to properly scope per component instance - Add comprehensive tests covering all uncovered branches and statements: expressPayments flow, autoSelect paths, clickableContainer, hide/sortBy props, showLoader effects, config prop, getOrder fallback call - Mock @commercelayer/core with importOriginal to preserve existing exports - Add MockPaymentMethodProvider helper for isolated effect testing - PaymentMethod.tsx: 100% statements, 99.21% branches - PaymentMethodsContainer.tsx: 100% statements, 100% branches - usePaymentMethod.ts: 100% statements, 100% branches
- Remove react-doctor.yml CI workflow - Fix pgk-pr-new.yaml: replace 'pnpm build' (no root script) with explicit filter for the three published packages
commit: |
added 10 commits
June 10, 2026 10:19
…rms not accepted The second useEffect in PlaceOrderButton called setNotPermitted(false) unconditionally when paymentMethodErrors/errors cleared, ignoring isPermitted. This meant filling in payment data (which clears prior errors) would enable the button even if the PrivacyAndTermsCheckbox was not checked. Fix: guard the re-enable path with isPermitted so clearing errors only enables the button when all other conditions (privacy/terms accepted, billing/shipping address, etc.) are also satisfied.
…rs clear The previous fix introduced a regression: the error-clearing useEffect called setNotPermitted(false) whenever isPermitted=true, but isPermitted only checks if payment_method.id is set — it does not check whether the payment source (card details) is actually filled. This caused the button to enable as soon as errors cleared, even without card details entered. Fix: introduce a hasBlockingErrors state. The error effect only manages this flag (and loading/forceDisable side-effects). The first payment effect now reacts to hasBlockingErrors changes — so when errors clear, it re-runs its full check including the card.brand and onsubmit conditions before enabling the button.
…tainer to prevent infinite re-render loop
Inline functions inside useMemo get a new reference on every recompute.
Payment forms (StripePaymentForm, BraintreePayment, KlarnaPayment, etc.)
include setPaymentRef in their useEffect deps — when this changes on every
render the effect re-runs, calling setPaymentRef again, which mutates state,
which triggers useMemo to recompute, causing an infinite loop.
Fix: extract setLoading, setPaymentRef and setPaymentMethodErrors as stable
useCallback(() => {}, []) callbacks. dispatch from useReducer is guaranteed
stable so empty deps are correct. This matches the pattern already used in
usePaymentMethod (standalone mode), where setPaymentRefCallback is useCallback.
ExternalPayment.tsx had an explicit biome-ignore to work around this bug;
StripePaymentForm, BraintreePayment, KlarnaPayment and CheckoutComPayment
did not and were all susceptible to the infinite loop in container mode.
The status useEffect had a default case calling setNotPermitted(false) on every mount, running after the payment check effect and unconditionally enabling the button regardless of payment state. Root cause: dep was [status != null] — always true, so the effect only ran once on mount. With status='standby' (initial state) it hit the default case, overrode the payment check, and left the button enabled. For orders without a payment method, isPermitted stayed false and the payment check effect never re-ran to correct this. Fix: remove the default case — the payment check effect is the sole authority for enabling the button. Change dep to [status] so the effect re-runs on actual status changes (e.g. 'standby' -> 'disabled'). Also update the handleClick tests: they previously relied on the buggy status effect to enable the button for clicking. Now they correctly mock getCardDetails to return a brand (matching real user state), add paymentType to contexts that were missing it, and align currentPaymentMethodType in Providers with the paymentType under test.
BaseSelect used an uncontrolled <select> (defaultValue) which only applied on the initial mount. When the value prop changed externally (e.g. from a country to empty), React had no mechanism to update the DOM element and the old selection persisted. Switch BaseSelect to a controlled component: maintain localValue state synced via useEffect([value]), and expose onChange to callers. This ensures the select always reflects the current value prop. Also remove the truthy guard in AddressCountrySelector's useEffect so setValue is called even when value is falsy, resetting the form element to the placeholder in the browser/form scenario.
Calling setValue(name, '') unconditionally on mount would cause rapid-form to process the empty required field immediately, potentially triggering premature validation errors. The controlled BaseSelect already handles the visual reset via localValue state — no manual setValue needed for the placeholder case.
When order.billing_address?.country_code is null (SDK returns null for
unset fields), the destructuring default '' does not apply (it only
applies for undefined). This caused localValue=null which renders
<select value={null}>, potentially showing the first option instead of
the placeholder.
Apply nullish coalescing (value ?? '') on both useState initializer and
useEffect sync so null is treated identically to undefined: both result
in the placeholder being selected.
…ween null/undefined The useEffect approach had two problems: 1. When value prop changed from null to undefined (or vice versa), the effect fired and reset the user's selection to '' — both are the same semantic 'no value' but have different JS identities. 2. The effect ran after the commit phase, causing a one-frame flicker where the wrong option was briefly visible. Switch to the React derived-state pattern: update localValue synchronously during render when safeValue (value ?? '') differs from the last synced value. This ensures: - null/undefined oscillation does NOT reset the user's own selection - value=null -> value='IT' (order pre-fill) correctly shows Italy - value='IT' -> value='' (reset) correctly shows placeholder - User selecting a country while value stays null is preserved
Switch from controlled select (value + useState derived-state) back to an uncontrolled approach (defaultValue) combined with useLayoutEffect to imperatively update the DOM when the external value prop genuinely changes. The controlled approach caused the selected label to not update when the user changed the option while the parent held a stable pre-filled value prop — React would revert the DOM back to localValue on every render cycle before the user's pick could be reflected, or browser-native autofill/form behaviour conflicted with the React-managed value. With the uncontrolled approach: - defaultValue sets the correct initial option (placeholder or pre-fill). - useLayoutEffect fires before paint and sets select.value imperatively only when the external safeValue actually changes, so user-driven changes are never silently reset. - null/undefined are normalised to "" so the placeholder option is always visible when no value is provided by the SDK.
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.
Closes #795
Changes
Phase 1 — PaymentMethod standalone component
PaymentMethodnow works withoutPaymentMethodsContainerby using the newusePaymentMethodhook internally_isProvidedsentinel onPaymentMethodContextloadingResourcevariable withuseRefto properly scope state per instancePaymentMethodsContainer deprecated
_isProvided: trueon context to signal container mode toPaymentMethodNew hook: usePaymentMethod
Tests (Phase 1)
PaymentMethod.tsx: 100% statements, 99.21% branches (1 unreachable null branch)PaymentMethodsContainer.tsx: 100% statements, 100% branchesPhase 2 — PlaceOrderButton, PlaceOrderContainer, PrivacyAndTermsCheckbox standalone
PlaceOrderButton standalone
PlaceOrderContainerwrapper — detects standalone mode via_isProvidedsentineloptionsprop directly (previously owned byPlaceOrderContainer)usePlaceOrderhook to manage state in standalone modePrivacyAndTermsCheckbox standalone
PlaceOrderContainerwrappercl:placeorder:recheckDOM event so siblingPlaceOrderButtoncan re-run permission checks without a common parentPlaceOrderContainer deprecated
@deprecatedJSDoc, kept for backward compatibility_isProvided: trueon context to signal container modeNew hook: usePlaceOrder
includesfor shipments, billing and shipping addressescl:placeorder:recheckevents fromPrivacyAndTermsCheckboxPLACE_ORDER_RECHECK_EVENTconstant for sibling communicationTests (Phase 2 — 100% coverage)
PlaceOrderContainer.tsx: 100% statements, 100% branchesPrivacyAndTermsCheckbox.tsx: 100% statements, 100% branchesusePlaceOrder.ts: 100% statements, 100% branchesPlaceOrderButton.tsx: ~60% (payment-gateway redirect flows require full integration mocks — covered in a future pass)specs/orders/place-order.spec.tsxCI fixes
pgk-pr-new.yaml: fixedpnpm build→ per-package filter commandreact-doctor.ymlworkflow