Skip to content

fix(core): stop useTexture's array/record forms looping forever - #3858

Merged
DennisSmolek merged 2 commits into
v10from
fix/3849-usetexture-loop
Aug 11, 2026
Merged

fix(core): stop useTexture's array/record forms looping forever#3858
DennisSmolek merged 2 commits into
v10from
fix/3849-usetexture-loop

Conversation

@DennisSmolek

Copy link
Copy Markdown
Member

Fixes #3849.

useTexture with an array or record argument re-ran its registry effect on every render, and the store.setState that effect performs re-renders every store subscriber, so the two fed each other until React bailed out with Maximum update depth exceeded. The single-URL form was unaffected.

Two instabilities, both load-bearing

Fixing either one alone still loops — I verified this by reverting each half independently.

1. useLoader returned a new array every call. .map() builds a fresh array each render, so the result could never be used as an effect dependency — by anyone. This is the root cause and it isn't useTexture-specific; useTexture's registry effect was just the first place it bit.

The array is now shallow-compared and its identity preserved while the contents are unchanged. Shallow-compared rather than keyed on the cache keys — that alternative looks simpler but silently breaks invalidation, because useLoader.clear() can hand back new objects for the same keys and that change still has to propagate. Comparing resolved values means a genuine change always yields a new identity and an incidental re-render never does.

2. useTexture keyed its memos and effect on the raw input reference. Callers routinely pass an inline literal, so that reference is new every render too. Everything now keys off a value signature plus an identity-stable view of the input.

The signature includes the record's keys, not just its URLs — two records can share URLs while mapping them to different names, and those aren't interchangeable.

On the render-phase ref write in useLoader

useLoader previously used no React hooks. It now uses one useRef. The write during render is idempotent — given equal contents it always settles on the same array — so it's safe under StrictMode's double render. Flagging it since it's the least conventional part of this change.

Testing

Three regression tests, all confirmed failing on pre-fix source with the reported error:

× array form settles instead of looping forever
    Error: Maximum update depth exceeded.
× record form settles instead of looping forever
    Error: Maximum update depth exceeded.
× array form returns a reference-stable result across re-renders
    AssertionError: expected [ …(2) ] to be [ …(2) ]

Worth knowing if you edit these: the useThree() call inside each Consumer is load-bearing. The loop only closes when the component calling useTexture is itself a broad store subscriber — a sibling subscriber re-renders itself, not the useTexture caller. My first draft put it in a sibling and the tests passed on the broken build. The issue's own repro has it in the same component; that detail matters.

  • pnpm test ✅ 584 passed (was 581), 45 files
  • pnpm typecheck / pnpm eslint / pnpm format

🤖 Generated with Claude Code

useTexture with an array or record argument re-ran its registry effect on every
render, and the store.setState that effect performs re-renders every store
subscriber, so the two fed each other until React bailed out with "Maximum
update depth exceeded". The single-URL form was unaffected.

Two independent instabilities had to go, and fixing either alone still loops:

useLoader built its result array with .map() on every call, so the returned
array had a new identity every render. That is the root cause and it is not
useTexture-specific -- any consumer using the result as an effect dependency
hits it. The array is now shallow-compared and its identity preserved while the
contents are unchanged. Shallow-compared rather than keyed on the cache keys,
because useLoader.clear() can hand back new objects for the same keys and that
invalidation still has to propagate.

useTexture keyed its memos and its registry effect on the raw `input`
reference. Callers routinely pass an inline literal, so that reference is new
every render too. Everything now keys off a value signature (which includes the
record's keys -- two records can share URLs but map them to different names)
and an identity-stable view of the input.

Tests reproduce the reported failure exactly: the broad `useThree()`
subscription lives in the same component as useTexture, because a sibling
subscriber re-renders itself and not the useTexture caller, and without that the
tests pass even on the broken build.

Fixes #3849

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

@MILLERMARRU MILLERMARRU left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good diagnosis splitting this into two independent instabilities, and the note about verifying each in isolation (reverting one half still loops) is exactly the kind of thing that's easy to skip and then have someone ask "did you check the other half wasn't also necessary" in review anyway.

The useLoader shallow-compare-and-keep-identity approach over keying on cache keys makes sense given useLoader.clear() can hand back new objects under the same key, keying on keys alone would miss that invalidation.

One thing I wanted to check rather than assume: the render-phase stableRef.current = stable write. React's own docs describe the sanctioned ref-during-render pattern specifically as conditional initialization (if (ref.current === null) ref.current = value, the lazy-init idiom), not "write every render as long as the value is idempotent." This PR's version writes on every render, it just happens to write back the same reference when contents are unchanged. I don't think this is actually unsafe given results is only ever read afterward (never mutated) and the write has no observable effect when it's a no-op, but it's a slightly different shape from what the docs explicitly bless, so flagging it since the PR itself calls it out as the least conventional part. Might be worth a one-line comment noting it was checked against the "except for initialization" caveat specifically, for whoever reads this in six months without the PR description in front of them.

Test coverage is thorough, appreciate the note about useThree() being load-bearing in the regression tests, that's the kind of detail that gets silently lost when someone "simplifies" a test later.

Comment only, no behaviour change. Records why a useMemo cannot express this
(its dep array would be `results`, and React throws when a dep array changes
length), why writing a ref during render is sound here (content-keyed cache
rather than state -- the reused array is only returned when it is element-wise
equal to what was just computed, so an abandoned concurrent render cannot yield
a stale value), and that only the wrapper is stabilised while the elements stay
shared process-wide via the suspend cache.

Also notes that this is the first hook in useLoader, so calling it conditionally
now breaks -- something that was always a rules-of-hooks violation and merely
happened to work because suspend() is a cache-and-throw rather than a hook.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@DennisSmolek
DennisSmolek merged commit 20de6b2 into v10 Aug 11, 2026
3 checks passed
@DennisSmolek
DennisSmolek deleted the fix/3849-usetexture-loop branch August 11, 2026 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants