test(joint-react): add repro for useCombinedRef assigning the ref after layout effects - #3444
Open
kumilingus wants to merge 1 commit into
Open
Conversation
…t effects
`useCombinedRef` assigns the forwarded ref inside a passive effect:
useEffect(() => { setForwardRef(ref, innerRef.current); }, [ref]);
React attaches refs during commit, before layout effects run, and
consumers are entitled to rely on that. Because this defers the
assignment, a parent that renders `<SVGText ref={myRef} />` and reads
`myRef.current` in its own `useLayoutEffect` sees null — parent layout
effects run before any passive effect.
`useMeasureElement` is exactly that consumer: it reads `nodeRef.current`
in a layout effect, returns early when empty, and its dependency list
contains nothing that changes when the node is finally assigned, so the
node is never registered with the size observer and the element stays 0x0
for good.
Two things hide it. StrictMode remounts effects, so a second pass finds
the ref already set — and it is on for every story and, via
`configure({ reactStrictMode: true })`, for every test, so the suite is
blind to this by default and the repro has to opt out with
`reactStrictMode: false`. Separately, no example in the package passes a
ref to `SVGText`: the flowchart, svg-node and portal-selectors stories put
a plain DOM ref on a native `<text>`, and the `SVGText` story measures a
`<g>` wrapper.
Found in an app measuring `SVGText` directly, where it was correct in
development and every node lost its shape and layout in the production
build.
The first test is `it.failing`, so this documents the defect without
reddening CI and starts failing once the hook is fixed. Verified both
ways: as it stands the body throws, and with `useCombinedRef` switched to
assign during commit only that test flips, leaving the other two green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
Repro only — no production code touched. Adds
src/hooks/__tests__/use-combined-ref-timing.test.tsx.The defect
useCombinedRefassigns the forwarded ref inside a passive effect:React attaches refs during commit, before layout effects run, and consumers are entitled to rely on that. Deferring the assignment means a parent that renders
<SVGText ref={myRef} />and readsmyRef.currentin its ownuseLayoutEffectseesnull, because parent layout effects run before any passive effect.useMeasureElementis exactly that consumer:It returns before
setMeasuredNode, and its dependency list contains nothing that changes when the node is finally assigned — so the node is never registered with the size observer and the element stays 0×0 permanently. No size, and no layout for anything that consumes sizes.Why nothing caught it
.storybook/preview.ts) and for every test (configure({ reactStrictMode: true })in__mocks__/jest-setup.ts). The suite is blind to this class of bug by default, which is why the new test has to passreactStrictMode: falseexplicitly.SVGText. The flowchart, svg-node and portal-selectors stories put a plain DOM ref on a native<text>; theSVGTextstory measures a<g>wrapper. The broken path is never exercised.The existing
use-combined-ref.test.tsxcases assertref.currentafterrender()returns, by which point passive effects have flushed — so they pass regardless. The new file keeps one such assertion, labelled, to make that explicit.How it was found
An app that measured
SVGTextdirectly: correct undervite dev, and in the production build every node rendered as a bare label with no shape behind it and no layout. TracingResizeObserverin the page showed dev observing all tentextnodes and the production build observing none.The tests
assigns the forwarded ref before a consumer layout effect runs— markedit.failing, so it documents the defect without reddening CI and starts failing once the hook is fixed (drop the.failingthen).is masked by StrictMode, which is why nothing caught it— asserts only the final value, so it holds before and after a fix.has assigned the ref by the time render returns…— shows why the current tests pass.Verified in both directions: as it stands the first body throws (all three green); with
useCombinedRefswitched to assign during commit, only that test flips and the other two stay green. So it pins this defect specifically.Possible fix
Assign during commit rather than in an effect — a callback ref, or a proxy whose setter forwards:
Hardening
useMeasureElementso a late ref is still picked up would be worth doing too, but the contract violation is inuseCombinedRef. Happy to turn this into a fix PR if you'd like it in the same change.🤖 Generated with Claude Code