From e195c5991dc255cbd80c376663fb59735889d3aa Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Sat, 1 Aug 2026 19:21:40 +0200 Subject: [PATCH 1/4] fix(Selection): stop Select's effect from depending on its own write Select's effect kept the context value in its own deps and returned a self-undoing cleanup, causing an infinite update loop. Now depends only on the stable setter, diffs inside the updater, and selects by isMesh/isLine/isPoints instead of an exact type-string match. --- src/Selection.tsx | 47 ++++--- src/tests/Selection.test.tsx | 232 +++++++++++++++++++++++++++++++++++ 2 files changed, 263 insertions(+), 16 deletions(-) create mode 100644 src/tests/Selection.test.tsx diff --git a/src/Selection.tsx b/src/Selection.tsx index 7151a9f4..cac94f7d 100644 --- a/src/Selection.tsx +++ b/src/Selection.tsx @@ -1,7 +1,7 @@ import { type ThreeElements } from '@react-three/fiber' import { createContext, - useContext, + use, useEffect, useMemo, useRef, @@ -10,7 +10,7 @@ import { type ReactNode, type SetStateAction, } from 'react' -import { type Group, type Object3D } from 'three' +import { type Group, type Line, type Mesh, type Object3D, type Points } from 'three' export type Api = { selected: Object3D[] @@ -29,25 +29,40 @@ export function Selection({ children, enabled = true }: { enabled?: boolean; chi return {children} } +// `.type` is a free-form string; these flags cover Mesh/Line/Points subclasses too. +function isSelectable(object: Object3D): boolean { + const o = object as Partial + return !!(o.isMesh || o.isLine || o.isPoints) +} + export function Select({ enabled = false, children, ...props }: SelectApi) { const group = useRef(null!) - const api = useContext(selectionContext) + // Stable setter, unlike the context value - avoids retriggering off our own write. + const select = use(selectionContext)?.select + useEffect(() => { - if (api && enabled) { - let changed = false - const current: Object3D[] = [] - group.current.traverse((o) => { - o.type === 'Mesh' && current.push(o) - if (api.selected.indexOf(o) === -1) changed = true + if (!select || !enabled) return + + const current: Object3D[] = [] + group.current.traverse((o) => { + if (isSelectable(o)) current.push(o) + }) + + // Diff against latest state inside the updater; bail with the same + // reference when unchanged so React can skip the re-render. + select((prev) => { + const additions = current.filter((o) => !prev.includes(o)) + return additions.length ? [...prev, ...additions] : prev + }) + + return () => { + select((prev) => { + const next = prev.filter((o) => !current.includes(o)) + return next.length !== prev.length ? next : prev }) - if (changed) { - api.select((state) => [...state, ...current]) - return () => { - api.select((state) => state.filter((selected) => !current.includes(selected))) - } - } } - }, [enabled, children, api]) + }, [enabled, children, select]) + return ( {children} diff --git a/src/tests/Selection.test.tsx b/src/tests/Selection.test.tsx new file mode 100644 index 00000000..7cda9a36 --- /dev/null +++ b/src/tests/Selection.test.tsx @@ -0,0 +1,232 @@ +import * as React from 'react' +import * as THREE from 'three' +import { afterEach, describe, expect, it } from 'vitest' +import { Select, Selection, selectionContext } from '../Selection' +import { flush, root } from './test-utils' + +afterEach(async () => { + await React.act(async () => { + root.render(null) + }) +}) + +function Capture({ onSnapshot }: { onSnapshot: (selected: THREE.Object3D[]) => void }) { + const api = React.useContext(selectionContext) + React.useEffect(() => { + onSnapshot(api?.selected ?? []) + }) + return null +} + +describe('Selection/Select', () => { + it('settles to a stable selected array without looping', async () => { + const snapshots: THREE.Object3D[][] = [] + const meshRef = React.createRef() + + await React.act(async () => + root.render( + + snapshots.push(s)} /> + + + ) + ) + await flush() + await flush() + + // Exactly one render for the initial (empty) commit, one for the + // single necessary addition - the original bug kept re-triggering + // itself, so this would grow unbounded (and eventually throw) instead + // of stopping at 2. + expect(snapshots).toEqual([[], [meshRef.current]]) + }) + + it('selects a Line and a Points object, not just Mesh', async () => { + const snapshots: THREE.Object3D[][] = [] + const lineRef = React.createRef() + const pointsRef = React.createRef() + + await React.act(async () => + root.render( + + snapshots.push(s)} /> + + + + ) + ) + await flush() + await flush() + + const last = snapshots[snapshots.length - 1] + expect(last).toContain(lineRef.current) + expect(last).toContain(pointsRef.current) + }) + + it('removes its objects once unmounted, leaving a sibling Select untouched', async () => { + const snapshots: THREE.Object3D[][] = [] + const meshARef = React.createRef() + const meshBRef = React.createRef() + + const render = (mountA: boolean) => + root.render( + + snapshots.push(s)} /> + {mountA && ( + + )} + + + ) + + await React.act(async () => render(true)) + await flush() + await flush() + expect(snapshots[snapshots.length - 1]).toEqual( + expect.arrayContaining([meshARef.current, meshBRef.current]) + ) + + await React.act(async () => render(false)) + await flush() + await flush() + + const last = snapshots[snapshots.length - 1] + expect(last).not.toContain(meshARef.current) + expect(last).toContain(meshBRef.current) + }) + + it('removes its objects when enabled toggles to false', async () => { + const snapshots: THREE.Object3D[][] = [] + const meshRef = React.createRef() + + const render = (enabled: boolean) => + root.render( + + snapshots.push(s)} /> + + + ) + + await React.act(async () => render(true)) + await flush() + await flush() + expect(snapshots[snapshots.length - 1]).toContain(meshRef.current) + + await React.act(async () => render(false)) + await flush() + await flush() + expect(snapshots[snapshots.length - 1]).not.toContain(meshRef.current) + }) + + it('selects a SkinnedMesh - isMesh is inherited, not just the exact type string', async () => { + const snapshots: THREE.Object3D[][] = [] + const meshRef = React.createRef() + + await React.act(async () => + root.render( + + snapshots.push(s)} /> + + + ) + ) + await flush() + await flush() + + expect(snapshots[snapshots.length - 1]).toContain(meshRef.current) + }) + + it('does not duplicate an object claimed by nested Selects', async () => { + const snapshots: THREE.Object3D[][] = [] + const meshRef = React.createRef() + + await React.act(async () => + root.render( + + snapshots.push(s)} /> + + + + + + + + + ) + ) + await flush() + await flush() + await flush() + + const last = snapshots[snapshots.length - 1] + expect(last.filter((o) => o === meshRef.current)).toHaveLength(1) + }) + + it('keeps an object selected via the outer Select after the inner one disables', async () => { + const snapshots: THREE.Object3D[][] = [] + const meshRef = React.createRef() + + const render = (innerEnabled: boolean) => + root.render( + + snapshots.push(s)} /> + + + + + + + + + ) + + await React.act(async () => render(true)) + await flush() + await flush() + await flush() + expect(snapshots[snapshots.length - 1]).toContain(meshRef.current) + + await React.act(async () => render(false)) + await flush() + await flush() + await flush() + + // The inner Select's own cleanup drops its claim, but the mesh never + // left the outer Select's subtree - its own traversal still finds it. + expect(snapshots[snapshots.length - 1]).toContain(meshRef.current) + }) +}) From 9dee43b4c7385ee39a796f75b6a896cea7b0ad8c Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Sat, 1 Aug 2026 20:55:05 +0200 Subject: [PATCH 2/4] fix(Selection): diff against live state instead of unconditional clear+readd Copilot review: Select's effect cleanup unconditionally cleared its claimed objects on every re-run, even when only `children`'s identity changed (not its content), churning the selected array's reference on every unrelated render. Now diffs against live state inside the updater and bails with the same reference when nothing actually changed, while still re-asserting a claim if another Select's update dropped it (nested Selects share objects). --- src/Selection.tsx | 42 ++++++++++++++++++++++----------- src/tests/Selection.test.tsx | 45 +++++++++++++++++++++++++++++++++--- 2 files changed, 71 insertions(+), 16 deletions(-) diff --git a/src/Selection.tsx b/src/Selection.tsx index cac94f7d..89ce02e2 100644 --- a/src/Selection.tsx +++ b/src/Selection.tsx @@ -39,29 +39,45 @@ export function Select({ enabled = false, children, ...props }: SelectApi) { const group = useRef(null!) // Stable setter, unlike the context value - avoids retriggering off our own write. const select = use(selectionContext)?.select + // What this Select currently has claimed in `selected`, so a re-run + // diffs against that instead of unconditionally clearing and re-adding through effect cleanup. + const claimed = useRef([]) useEffect(() => { - if (!select || !enabled) return + if (!select) return const current: Object3D[] = [] - group.current.traverse((o) => { - if (isSelectable(o)) current.push(o) - }) + if (enabled) { + group.current.traverse((o) => { + if (isSelectable(o)) current.push(o) + }) + } + + const previouslyClaimed = claimed.current + claimed.current = current - // Diff against latest state inside the updater; bail with the same - // reference when unchanged so React can skip the re-render. select((prev) => { - const additions = current.filter((o) => !prev.includes(o)) - return additions.length ? [...prev, ...additions] : prev + // Add anything missing from live state - covers both the normal + // case and re-asserting a claim some other Select's own update + // dropped in the meantime (nested Selects share objects). Only + // remove what this Select itself is no longer claiming. + const toAdd = current.filter((o) => !prev.includes(o)) + const toRemove = previouslyClaimed.filter((o) => !current.includes(o) && prev.includes(o)) + if (!toAdd.length && !toRemove.length) return prev + const kept = toRemove.length ? prev.filter((o) => !toRemove.includes(o)) : prev + return toAdd.length ? [...kept, ...toAdd] : kept }) + }, [enabled, children, select]) + // Only for unmount - a separate effect so it doesn't fire on every + // enabled/children change like the diffing effect above does. + useEffect(() => { return () => { - select((prev) => { - const next = prev.filter((o) => !current.includes(o)) - return next.length !== prev.length ? next : prev - }) + if (!select || !claimed.current.length) return + const stillClaimed = claimed.current + select((prev) => prev.filter((o) => !stillClaimed.includes(o))) } - }, [enabled, children, select]) + }, [select]) return ( diff --git a/src/tests/Selection.test.tsx b/src/tests/Selection.test.tsx index 7cda9a36..f16761e2 100644 --- a/src/tests/Selection.test.tsx +++ b/src/tests/Selection.test.tsx @@ -107,9 +107,7 @@ describe('Selection/Select', () => { await React.act(async () => render(true)) await flush() await flush() - expect(snapshots[snapshots.length - 1]).toEqual( - expect.arrayContaining([meshARef.current, meshBRef.current]) - ) + expect(snapshots[snapshots.length - 1]).toEqual(expect.arrayContaining([meshARef.current, meshBRef.current])) await React.act(async () => render(false)) await flush() @@ -229,4 +227,45 @@ describe('Selection/Select', () => { // left the outer Select's subtree - its own traversal still finds it. expect(snapshots[snapshots.length - 1]).toContain(meshRef.current) }) + + it('does not change the selected array reference on a re-render where children only changes identity', async () => { + const refs: THREE.Object3D[][] = [] + const meshRef = React.createRef() + + function CaptureRef() { + const api = React.useContext(selectionContext) + React.useEffect(() => { + if (api) refs.push(api.selected) + }) + return null + } + + // A fresh JSX tree every call, like a parent re-rendering for an + // unrelated reason - Select's own `children` prop is a new reference + // each time even though the underlying mesh never changes. + const render = () => + root.render( + + + + + ) + + await React.act(async () => render()) + await flush() + await flush() + + refs.length = 0 + for (let i = 0; i < 5; i++) { + await React.act(async () => render()) + await flush() + } + + expect(new Set(refs).size).toBe(1) + }) }) From 7cf17edfb4f5d9b7fd236be1b8c9abb17e70433a Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Sat, 1 Aug 2026 22:39:52 +0200 Subject: [PATCH 3/4] fix(Selection): trim comments --- src/Selection.tsx | 29 ++++++++++++++--------------- 1 file changed, 14 insertions(+), 15 deletions(-) diff --git a/src/Selection.tsx b/src/Selection.tsx index 89ce02e2..72668d18 100644 --- a/src/Selection.tsx +++ b/src/Selection.tsx @@ -29,7 +29,7 @@ export function Selection({ children, enabled = true }: { enabled?: boolean; chi return {children} } -// `.type` is a free-form string; these flags cover Mesh/Line/Points subclasses too. +// Covers Mesh/Line/Points subclasses too, unlike `.type`. function isSelectable(object: Object3D): boolean { const o = object as Partial return !!(o.isMesh || o.isLine || o.isPoints) @@ -37,10 +37,8 @@ function isSelectable(object: Object3D): boolean { export function Select({ enabled = false, children, ...props }: SelectApi) { const group = useRef(null!) - // Stable setter, unlike the context value - avoids retriggering off our own write. + // Stable, unlike the context value - avoids retriggering off our own write. const select = use(selectionContext)?.select - // What this Select currently has claimed in `selected`, so a re-run - // diffs against that instead of unconditionally clearing and re-adding through effect cleanup. const claimed = useRef([]) useEffect(() => { @@ -57,25 +55,26 @@ export function Select({ enabled = false, children, ...props }: SelectApi) { claimed.current = current select((prev) => { - // Add anything missing from live state - covers both the normal - // case and re-asserting a claim some other Select's own update - // dropped in the meantime (nested Selects share objects). Only - // remove what this Select itself is no longer claiming. - const toAdd = current.filter((o) => !prev.includes(o)) - const toRemove = previouslyClaimed.filter((o) => !current.includes(o) && prev.includes(o)) + const prevSet = new Set(prev) + const currentSet = new Set(current) + const toAdd = current.filter((o) => !prevSet.has(o)) + const toRemove = previouslyClaimed.filter((o) => !currentSet.has(o) && prevSet.has(o)) if (!toAdd.length && !toRemove.length) return prev - const kept = toRemove.length ? prev.filter((o) => !toRemove.includes(o)) : prev + const toRemoveSet = toRemove.length ? new Set(toRemove) : null + const kept = toRemoveSet ? prev.filter((o) => !toRemoveSet.has(o)) : prev return toAdd.length ? [...kept, ...toAdd] : kept }) }, [enabled, children, select]) - // Only for unmount - a separate effect so it doesn't fire on every - // enabled/children change like the diffing effect above does. + // Separate from the effect above so unmount cleanup doesn't fire on every enabled/children change. useEffect(() => { return () => { if (!select || !claimed.current.length) return - const stillClaimed = claimed.current - select((prev) => prev.filter((o) => !stillClaimed.includes(o))) + const stillClaimed = new Set(claimed.current) + select((prev) => { + const next = prev.filter((o) => !stillClaimed.has(o)) + return next.length !== prev.length ? next : prev + }) } }, [select]) From 2f987812b695fd2c0d11fd685062acad94a329d2 Mon Sep 17 00:00:00 2001 From: kvvasuu Date: Sat, 1 Aug 2026 22:40:09 +0200 Subject: [PATCH 4/4] test(Selection): cover unmount cleanup bail-out --- src/tests/Selection.test.tsx | 53 ++++++++++++++++++++++++++++++++++++ 1 file changed, 53 insertions(+) diff --git a/src/tests/Selection.test.tsx b/src/tests/Selection.test.tsx index f16761e2..cdc9dc49 100644 --- a/src/tests/Selection.test.tsx +++ b/src/tests/Selection.test.tsx @@ -268,4 +268,57 @@ describe('Selection/Select', () => { expect(new Set(refs).size).toBe(1) }) + + it('a cleanup updater returns the same reference when nothing is left for it to remove', async () => { + const meshRef = React.createRef() + const updaters: Array<(prev: THREE.Object3D[]) => THREE.Object3D[]> = [] + + // React batches both nested Selects' cleanup calls into a single + // re-render regardless of this bail-out (the net change from mount to + // unmount is real), so the optimization isn't observable through + // rendering alone. Capture the raw updater functions instead and + // replay them directly to verify the second one bails. + function CapturingSelection({ children }: { children: React.ReactNode }) { + const [selected, setSelected] = React.useState([]) + const select = React.useCallback((updater: React.SetStateAction) => { + if (typeof updater === 'function') updaters.push(updater as (prev: THREE.Object3D[]) => THREE.Object3D[]) + setSelected(updater) + }, []) + const value = React.useMemo(() => ({ selected, select, enabled: true }), [selected, select]) + return {children} + } + + const render = (mounted: boolean) => + root.render( + + {mounted && ( + + + + + + + + )} + + ) + + await React.act(async () => render(true)) + await flush() + await flush() + await flush() + + const mesh = meshRef.current! + updaters.length = 0 + await React.act(async () => render(false)) + await flush() + await flush() + await flush() + + expect(updaters).toHaveLength(2) + const afterFirst = updaters[0]([mesh]) + const afterSecond = updaters[1](afterFirst) + expect(afterSecond).toBe(afterFirst) + }) })