Skip to content

fix(EffectComposer): fix StrictMode composer leak, guard against r3f reorder bug - #353

Open
kvvasuu wants to merge 1 commit into
fix/02-wrapeffect-refactorfrom
fix/03-effectcomposer-leak-and-reorder
Open

fix(EffectComposer): fix StrictMode composer leak, guard against r3f reorder bug#353
kvvasuu wants to merge 1 commit into
fix/02-wrapeffect-refactorfrom
fix/03-effectcomposer-leak-and-reorder

Conversation

@kvvasuu

@kvvasuu kvvasuu commented Jul 29, 2026

Copy link
Copy Markdown
Collaborator

Stacked PR 3/6 — part of a split of #350 .
← based on #352 · → followed by #354

Note: dispose coverage for ColorAverage isn't added here on purpose —
ColorAverage doesn't dispose itself until #355.

Fixes #270

EffectComposerImpl was created inside useMemo, which doesn't guarantee its cleanup runs before React discards a memoized value - this leaked the composer (and its WebGL resources) under StrictMode. It's now created and disposed inside a useState/useEffect lifecycle instead.

Pass collection walks the real r3f instance tree (group.current.__r3f .children) and rebuilds the pass list from scratch whenever it or the composer changes, so the order always matches current JSX - including through wrapper components - after a reorder or a remount.

While testing the reorder case, found a genuine bug in r3f's host config: when React moves (not remounts) an existing child - e.g. a key-preserving reorder of a keyed effect list - insertBefore/ appendChild splice the moved instance into its new slot without detaching it from the old one first, leaving a stale duplicate in Instance.children. Worth reporting upstream. Added a cheap dedupe (keep each object's last occurrence, sorted by that position) that recovers the correct order either way and is a no-op when the bug isn't present.

Also widens children from JSX.Element | JSX.Element[] to ReactNode, which was rejecting the common condition && <Effect/> idiom (that expression is false | Element, and false isn't assignable to either half of the union). The tree-walk already tolerates arbitrary children gracefully - it just filters for instanceof Effect || instanceof Pass - so the stricter type wasn't buying any actual safety.

Replaces the old root-level EffectComposer.test.tsx (superseded, predates this suite and used a different mock-root setup) with a suite covering pass registration/composition order, StrictMode disposal, and the reorder/dedupe behavior above. (ColorAverage-specific dispose coverage lands with the primitive-effect dispose fix, a few commits later - ColorAverage doesn't dispose itself until then.)

…reorder bug

EffectComposerImpl was created inside useMemo, which doesn't guarantee
its cleanup runs before React discards a memoized value - this leaked
the composer (and its WebGL resources) under StrictMode. It's now
created and disposed inside a useState/useEffect lifecycle instead.

Pass collection walks the real r3f instance tree (group.current.__r3f
.children) and rebuilds the pass list from scratch whenever it or the
composer changes, so the order always matches current JSX - including
through wrapper components - after a reorder or a remount.

While testing the reorder case, found a genuine bug in r3f's host
config: when React moves (not remounts) an existing child - e.g. a
key-preserving reorder of a keyed effect list - insertBefore/
appendChild splice the moved instance into its new slot without
detaching it from the old one first, leaving a stale duplicate in
Instance.children. Worth reporting upstream. Added a cheap dedupe
(keep each object's last occurrence, sorted by that position) that
recovers the correct order either way and is a no-op when the bug
isn't present.

Also widens `children` from JSX.Element | JSX.Element[] to ReactNode,
which was rejecting the common `condition && <Effect/>` idiom (that
expression is `false | Element`, and `false` isn't assignable to
either half of the union). The tree-walk already tolerates arbitrary
children gracefully - it just filters for `instanceof Effect ||
instanceof Pass` - so the stricter type wasn't buying any actual
safety.

Replaces the old root-level EffectComposer.test.tsx (superseded,
predates this suite and used a different mock-root setup) with a
suite covering pass registration/composition order, StrictMode
disposal, and the reorder/dedupe behavior above. (ColorAverage-specific
dispose coverage lands with the primitive-effect dispose fix, a few
commits later - ColorAverage doesn't dispose itself until then.)
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.

1 participant