Skip to content

Stop the OrdersList refetch loop and the disabledClassName DOM leak - #845

Merged
acasazza merged 5 commits into
mainfrom
fix/orderlist-refetch-loop
Sep 16, 2026
Merged

acasazza merged 5 commits into
mainfrom
fix/orderlist-refetch-loop

Conversation

@acasazza

Copy link
Copy Markdown
Member

What was happening

On a page rendering <OrderList>, the browser fired a request to orders (or order_subscriptions) roughly once a second, indefinitely. There is no timer involved — the rate simply equals the API round-trip time, because the effect that fetches retriggers itself.

Two independent routes closed the cycle, and both had to be cut.

1. The context rebuilt its callbacks on every fetch

useCustomerProviderValue memoized the whole context value with state among its dependencies, and spread ...state into it. So:

  1. OrderList calls getCustomerOrdersGET /orders
  2. the response does setState({...prev, orders}) → new state object
  3. the memo re-runs → getCustomerOrders gets a new identity
  4. OrderList has that callback in its effect deps → the effect runs again → back to 1

Fixed by splitting the value in two: a stable actions memo keyed only on what the callbacks actually close over (accessToken, customerId, interceptors, order, pageSize, withoutIncludes), and a thin outer memo that joins state and actions. The single callback that read state.addresses directly (createCustomerAddress) now reads it through a ref.

The callbacks still change identity when accessToken or customerId change, so a fetch still reruns when the token arrives. That property is why the callbacks were left in OrderList's dependency array rather than removed from it.

2. defaultSdkSorting was a fresh object on every render

const defaultSdkSorting = sortBy.length && sortBy[0] != null ? { ... } : undefined

A bare object literal, so a new identity on every render — enough to refire the effect even if the callbacks were stable. Now memoized on the primitives it derives from.

Where it came from

36a721d9 (v5.0.0-beta.0~11) widened the dependency array:

-  }, [pageIndex, currentPageSize, sorting, id != null])
+  }, [pageIndex, currentPageSize, sorting, getCustomerOrders, type, id, getCustomerSubscriptions, defaultSdkSorting])

4.29.7 is not affected.

Also in here: disabledClassName reaching the DOM

React does not recognize the `disabledClassName` prop on a DOM element.

Address declared disabledClassName in Props but did not destructure it, so it survived into the rest spread that lands on a <div>. It came loose in 1c85518b, the addresses-domain refactor, which also dropped the disabled branch — so in v5 the prop had no effect at all beyond polluting the DOM. It is now pulled out of the spread; the children-as-function form still receives it through parentProps, and the JSDoc says so.

4.29.7 is not affected here either.

Tests

specs/orders/order-list.refetch-loop.spec.tsx mounts <OrderList> inside the real Customer provider, with columns and sortBy passed as inline literals the way a consumer actually passes them.

Both new tests were checked against the unfixed source, so they are not vacuous:

without the fix with the fix
getCustomerOrders calls 32 1
disabledClassName on the DOM attribute present absent

Full suite: 1087 tests across 93 files, green.

🤖 Generated with Claude Code

OrderList fired a fetch roughly once per API round-trip: the effect that
calls getCustomerOrders retriggered itself through two independent routes.

1. useCustomerProviderValue memoized the whole context value with `state`
   among its deps and spread `...state` into it, so every landed fetch
   produced a new getCustomerOrders identity — and OrderList has that
   callback in its effect deps. Split into a stable `actions` memo plus a
   thin outer memo that joins state and actions; the one callback that
   read `state.addresses` directly now reads it through a ref.

   The callbacks still change identity when accessToken or customerId
   change, so a fetch still reruns when the token arrives.

2. defaultSdkSorting was a bare object literal, i.e. a new identity on
   every render, which refired the effect even with stable callbacks.
   Memoized on the primitives it derives from.

Also stop Address from leaking disabledClassName onto the DOM: it was
declared in Props but not destructured, so it survived into the rest
spread that lands on a <div>. The children-as-function form still
receives it through parentProps.

Both regressions arrived in v5 and are absent from 4.29.7.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@acasazza acasazza added the bug Something isn't working label Sep 15, 2026
@acasazza acasazza self-assigned this Sep 15, 2026
@netlify

netlify Bot commented Sep 15, 2026

Copy link
Copy Markdown

Deploy Preview for commercelayer-react-components ready!

Name Link
🔨 Latest commit c25c8ee
🔍 Latest deploy log https://app.netlify.com/projects/commercelayer-react-components/deploys/6aaa6a8208dcbc0008f1aa8a
😎 Deploy Preview https://deploy-preview-845--commercelayer-react-components.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

Alessandro Casazza and others added 2 commits September 16, 2026 11:51
BillingAddressForm declares fieldEvent in Props but never destructures it,
so it survives into the rest spread that lands on the <form>:

  React does not recognize the `fieldEvent` prop on a DOM element.

Its twin already does this correctly — ShippingAddressForm.tsx destructures
`fieldEvent: _fieldEvent = "change"` — which is what makes this an omission
rather than a decision. Neither component reads the prop: it appears nowhere
else in src/, so it is accepted, typed, and inert.

Found by scanning for components whose Props extend JSX.IntrinsicElements,
declare non-DOM props, and spread the rest onto a DOM element. Only two
candidates survived filtering for real HTML attributes, and the other one
(AvailabilityTemplate) is a false positive — its extra names live inside a
`labels` object that is destructured.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Nothing in CI ran the tests. The check that reads "test / test" comes from
merge-test-env-check.yml, whose `test` job only lists licenses — it takes
three seconds and passes regardless of the code. The 1376 tests across the
three packages ran only in the local pre-commit hook, so a PR that broke
them showed all green.

This workflow is named unit-tests with a vitest job, rendering as
"unit-tests / vitest", so it cannot be mistaken for the licenses check
sitting next to it.

`pnpm test` is not reused: the react-components script starts with
`pnpm audit --prod --audit-level high`, which fails on any newly published
advisory and would turn the suite red for reasons unrelated to the change.
Lint and vitest run directly instead. No build step is needed — the vitest
configs alias the sibling workspace packages to their source.

Verified both ways locally: the commands exit 0 on this branch, and exit 1
with one of the fixes in this PR reverted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@pkg-pr-new

pkg-pr-new Bot commented Sep 16, 2026

Copy link
Copy Markdown
npm i https://pkg.pr.new/@commercelayer/core-components@845
npm i https://pkg.pr.new/@commercelayer/react-components@845
npm i https://pkg.pr.new/@commercelayer/react-hooks-components@845

commit: c25c8ee

The first run of this workflow failed, which is the workflow doing its job.
react-hooks-components' lint script ends in `tsc`, and that resolves
@commercelayer/core-components through its dist/ — absent on a clean
checkout, so the package produced 30-odd TS2307 errors and the job stopped
before reaching vitest.

It passed locally only because a previous build had left dist/ in place.
netlify.toml warns about exactly this for the storybook build; the comment
claiming no build was needed was right about vitest and wrong about lint.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@acasazza
acasazza requested a review from pfferrari September 16, 2026 10:04
The provider wrapped its value in an object literal, so every render of
CommerceLayer gave every consumer a fresh identity. This provider sits at
the root of the tree, and consumers put values from it into effect
dependency arrays — it was the amplifier behind the OrderList refetch loop
rather than its cause, and it stays a trap for anything else downstream.

The test needs a fresh child element per render: reusing one element makes
React skip rendering the child entirely, and the assertion then measures
nothing. It fails with the memo removed.

The five other providers built from object literals are left alone: three
sit inside .map() callbacks where a per-item memo is awkward and the value
is a single loop variable, and the remaining two are narrow leaf contexts.
None of them wraps the app.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@acasazza
acasazza merged commit 9209ed1 into main Sep 16, 2026
8 checks passed
@acasazza acasazza mentioned this pull request Sep 16, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants