fix(react): handle ref changes across rerenders before hydration#2500
fix(react): handle ref changes across rerenders before hydration#2500
Conversation
🦋 Changeset detectedLatest commit: 3975ffb The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughRef extraction and queuing in background pre-hydration renders was refactored: a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
upupming
force-pushed
the
fix/ref-apply-dedup
branch
from
297dea3 to
ce0fb92
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/react/runtime/src/snapshot/backgroundSnapshot.ts (1)
329-339:⚠️ Potential issue | 🟡 MinorOptional: Minor readability improvement — asymmetric optional chaining.
The
v !== oldVguard is correct and prevents redundant ref queue calls. Practically speaking, theoldVread here is alwaysundefinedbecause:
- Call path analysis: All production paths calling
setAttribute('values', …)either explicitly null__valuesfirst (lines 723–724 inreconstructInstanceTree, line 140 insnapshot.ts) or occur during initial setup before__valuesis populated.- Branch coverage: This
elsebranch runs only when__globalSnapshotPatchis null/undefined; patch-apply paths (where patches might contain pre-populated refs) initialize__globalSnapshotPatchfirst, so they take the first branch.- No ref swap risk: The concern about passing
nullinstead ofoldV's ref is not a practical issue here becauseoldVis always undefined.That said, the read-side optional chaining
(this.__values as unknown[])?.[index]is asymmetric with the unconditional writethis.__values = value as unknown[]on line 341, which is a minor readability smell. Consider dropping the optional chaining for consistency:const oldV = (this.__values as unknown[])[index];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/backgroundSnapshot.ts` around lines 329 - 339, In the forEach over this.__snapshot_def.refAndSpreadIndexes, remove the asymmetric optional chaining when reading oldV so it matches the unconditional write to this.__values; specifically change the read of this.__values to use (this.__values as unknown[])[index] (referenced in the loop where oldV is assigned) so the access style is consistent with the later unconditional assignment to this.__values and improves readability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/react/runtime/src/snapshot/backgroundSnapshot.ts`:
- Around line 329-339: In the forEach over
this.__snapshot_def.refAndSpreadIndexes, remove the asymmetric optional chaining
when reading oldV so it matches the unconditional write to this.__values;
specifically change the read of this.__values to use (this.__values as
unknown[])[index] (referenced in the loop where oldV is assigned) so the access
style is consistent with the later unconditional assignment to this.__values and
improves readability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 12841f3c-280c-4b85-80de-b7f537b2671e
📒 Files selected for processing (3)
.changeset/fix-react-ref-apply-dedup.mdpackages/react/runtime/src/snapshot/backgroundSnapshot.tspackages/react/testing-library/src/__tests__/ref-dedup.test.jsx
Merging this PR will degrade performance by 24.98%
Performance Changes
Comparing 
Footnotes
|
React External#694 Bundle Size — 680.2KiB (+0.04%).3975ffb(current) vs 8352530 main#686(baseline) Bundle metrics
|
| Current #694 |
Baseline #686 |
|
|---|---|---|
 Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
 Cache Invalidation |
39.67% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
17 |
17 |
| Duplicate Modules |
5 |
5 |
| Duplicate Code |
8.59% |
8.59% |
| Packages |
0 |
0 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 
1 regression
| Current #694 |
Baseline #686 |
|
|---|---|---|
| Other |
680.2KiB (+0.04%) |
679.93KiB |
Bundle analysis report Branch fix/ref-apply-dedup Project dashboard
Generated by RelativeCI Documentation Report issue
React MTF Example#708 Bundle Size — 196.47KiB (+0.04%).3975ffb(current) vs 8352530 main#700(baseline) Bundle metrics
|
| Current #708 |
Baseline #700 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
43.36% |
0% |
| Chunks |
0 |
0 |
| Assets |
3 |
3 |
| Modules |
173 |
173 |
| Duplicate Modules |
66 |
66 |
| Duplicate Code |
44.07% |
44.07% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #708 |
Baseline #700 |
|
|---|---|---|
| IMG |
111.23KiB |
111.23KiB |
| Other |
85.24KiB (+0.1%) |
85.15KiB |
Bundle analysis report Branch fix/ref-apply-dedup Project dashboard
Generated by RelativeCI Documentation Report issue
React Example#7576 Bundle Size — 225.31KiB (+0.04%).3975ffb(current) vs 8352530 main#7568(baseline) Bundle metrics
|
| Current #7576 |
Baseline #7568 |
|
|---|---|---|
| Initial JS |
0B |
0B |
| Initial CSS |
0B |
0B |
| Cache Invalidation |
35.28% |
0% |
| Chunks |
0 |
0 |
| Assets |
4 |
4 |
| Modules |
179 |
179 |
| Duplicate Modules |
69 |
69 |
| Duplicate Code |
44.57% |
44.57% |
| Packages |
2 |
2 |
| Duplicate Packages |
0 |
0 |
Bundle size by type 1 change 1 regression
| Current #7576 |
Baseline #7568 |
|
|---|---|---|
| IMG |
145.76KiB |
145.76KiB |
| Other |
79.55KiB (+0.1%) |
79.47KiB |
Bundle analysis report Branch fix/ref-apply-dedup Project dashboard
Generated by RelativeCI Documentation Report issue
Web Explorer#9148 Bundle Size — 900.04KiB (0%).3975ffb(current) vs 8352530 main#9140(baseline) Bundle metrics
|
| Current #9148 |
Baseline #9140 |
|
|---|---|---|
| Initial JS |
44.46KiB |
44.46KiB |
| Initial CSS |
2.22KiB |
2.22KiB |
| Cache Invalidation |
0% |
8.1% |
| Chunks |
9 |
9 |
| Assets |
11 |
11 |
| Modules |
229(+0.44%) |
228 |
| Duplicate Modules |
11 |
11 |
| Duplicate Code |
27.28% |
27.28% |
| Packages |
10 |
10 |
| Duplicate Packages |
0 |
0 |
Bundle size by type no changes
| Current #9148 |
Baseline #9140 |
|
|---|---|---|
| JS |
495.9KiB |
495.9KiB |
| Other |
401.92KiB |
401.92KiB |
| CSS |
2.22KiB |
2.22KiB |
Bundle analysis report Branch fix/ref-apply-dedup Project dashboard
Generated by RelativeCI Documentation Report issue
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react/runtime/__test__/snapshot/ref-dedup.test.jsx`:
- Around line 186-224: The test needs an intervening different render to force a
re-diff so the sibling-ref `v !== oldV` guard is exercised; modify the test
around the second re-render (where deinitGlobalSnapshotPatch(),
ref1.mockClear(), ref2.mockClear(), render(<Comp />, __root) currently occur) to
insert a short different render (for example call render(<view />, __root) or
render(<div /> , __root)) before re-rendering <Comp />. This change touches the
test's use of render, __root, Comp, deinitGlobalSnapshotPatch and the ref1/ref2
expectations and will ensure the runtime performs a values attribute update
rather than skipping it.
- Around line 146-184: The test "re-applies when the callback ref identity
actually changes" currently only asserts refB attached but doesn't verify the
old callback ref was released; update the assertions after swapping currentRef
to refB and rendering tick=1 to also assert that refA was invoked with null
(e.g., that refA received a null argument when detached) to ensure the old ref
is cleared when identity changes; reference the mock functions refA and refB,
the currentRef variable, and the render(<Comp tick={1} />, __root) call when
adding this assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0905a691-17aa-4808-ad34-30f89821358c
📒 Files selected for processing (1)
packages/react/runtime/__test__/snapshot/ref-dedup.test.jsx
upupming
force-pushed
the
fix/ref-apply-dedup
branch
from
3822e86 to
1750a68
Compare
upupming
changed the title
upupming
force-pushed
the
fix/ref-apply-dedup
branch
from
1750a68 to
79716c0
Compare
|
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/react/runtime/__test__/snapshot/ref.test.jsx (1)
1982-2153: LGTM — thorough unit coverage at the snapshot layer.The suite correctly exercises identity short-circuiting, null transitions in both directions, spread forms, object refs via
createRef, and the three-rerender intermediate cleanup case. Minor nit: the newit(...)blocks useasync function()but none of themawait— converting to syncfunction()(matching the plain tests) would be slightly clearer, but it's cosmetic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/__test__/snapshot/ref.test.jsx` around lines 1982 - 2153, Tests use async function() in multiple it(...) blocks though no awaits are used; change those to plain synchronous function() to match other tests. Edit the it(...) declarations (e.g., the cases titled 'ref is changed across rerenders before hydration', 'ref becomes null on rerender before hydration', 'ref is added on rerender before hydration', 'spread ref is removed on rerender before hydration', 'spread ref is added on rerender before hydration', 'object ref (createRef) is changed...', 'object ref (createRef) becomes null...', 'object ref (createRef) is added...', 'same ref callback in spread form should not be re-invoked', 'three consecutive rerenders before hydration clean up intermediate refs') and replace "async function()" with "function()" so the test signatures are synchronous.packages/react/testing-library/src/__tests__/ref.test.jsx (1)
487-572: LGTM — good matrix coverage for the pre-hydration ref fix.The normal×spread matrix plus the same-callback short-circuit and
useRef+useEffectportal-host pattern closely mirror the real-world regressions the fix targets. Consider also asserting the cleanup return-value path (a ref callback that returns a function) across a rerender before hydration to pin down_unmountsemantics fromapplyRef— that's the one branch inapplyRefnot directly exercised here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/ref.test.jsx` around lines 487 - 572, Add a test exercising the ref-callback cleanup path that `applyRef` handles: create a component like the existing App patterns that triggers a pre-hydration rerender (useState + useEffect bump) and use a ref callback that returns a cleanup function (e.g., const cb = vi.fn(() => cleanupFn)). Assert that the cleanup function is invoked when the ref is removed/changed (old callback called with null and its returned cleanup called once) and that the new ref receives the host node; reference the existing test helpers App, render, and the ref callbacks (oldCb/newCb) to mirror the normal×spread matrix setup so the `_unmount`/cleanup branch in applyRef is covered.packages/react/runtime/src/snapshot/snapshot/ref.ts (1)
83-94: LGTM — helper correctly covers both ref encodings.
getRefFromValuesymmetrically handles the__spread+refand__refforms and returnsnullfor anything else, which is exactly whatqueueRefAttrUpdateneeds to short-circuit identity on same-ref rerenders. One optional follow-up: the post-hydrationsetAttributeImplinbackgroundSnapshot.ts(lines 403, 412–423, 429–431, 464–466) still inlines the same pattern-matching. Routing those throughgetRefFromValuetoo would eliminate the pre/post divergence that caused this bug in the first place and keep the two paths from drifting again.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/snapshot/ref.ts` around lines 83 - 94, The post-hydration setAttributeImpl implementation in backgroundSnapshot.ts duplicates the ref-pattern matching logic; replace those inline checks with a call to getRefFromValue so both pre-hydration and post-hydration paths use the same ref extraction logic. Locate setAttributeImpl (the post-hydration variant that currently inspects __spread/ref and __ref inline) and swap the manual pattern matching with getRefFromValue(value) and use its null/ref result for the identity short-circuiting and subsequent handling, ensuring behavior is consistent with queueRefAttrUpdate and preventing drift between the two paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react/runtime/__test__/snapshot/ref.test.jsx`:
- Around line 1982-2153: Tests use async function() in multiple it(...) blocks
though no awaits are used; change those to plain synchronous function() to match
other tests. Edit the it(...) declarations (e.g., the cases titled 'ref is
changed across rerenders before hydration', 'ref becomes null on rerender before
hydration', 'ref is added on rerender before hydration', 'spread ref is removed
on rerender before hydration', 'spread ref is added on rerender before
hydration', 'object ref (createRef) is changed...', 'object ref (createRef)
becomes null...', 'object ref (createRef) is added...', 'same ref callback in
spread form should not be re-invoked', 'three consecutive rerenders before
hydration clean up intermediate refs') and replace "async function()" with
"function()" so the test signatures are synchronous.
In `@packages/react/runtime/src/snapshot/snapshot/ref.ts`:
- Around line 83-94: The post-hydration setAttributeImpl implementation in
backgroundSnapshot.ts duplicates the ref-pattern matching logic; replace those
inline checks with a call to getRefFromValue so both pre-hydration and
post-hydration paths use the same ref extraction logic. Locate setAttributeImpl
(the post-hydration variant that currently inspects __spread/ref and __ref
inline) and swap the manual pattern matching with getRefFromValue(value) and use
its null/ref result for the identity short-circuiting and subsequent handling,
ensuring behavior is consistent with queueRefAttrUpdate and preventing drift
between the two paths.
In `@packages/react/testing-library/src/__tests__/ref.test.jsx`:
- Around line 487-572: Add a test exercising the ref-callback cleanup path that
`applyRef` handles: create a component like the existing App patterns that
triggers a pre-hydration rerender (useState + useEffect bump) and use a ref
callback that returns a cleanup function (e.g., const cb = vi.fn(() =>
cleanupFn)). Assert that the cleanup function is invoked when the ref is
removed/changed (old callback called with null and its returned cleanup called
once) and that the new ref receives the host node; reference the existing test
helpers App, render, and the ref callbacks (oldCb/newCb) to mirror the
normal×spread matrix setup so the `_unmount`/cleanup branch in applyRef is
covered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 89d937d3-6b53-448b-b9ec-c760cae78d25
📒 Files selected for processing (6)
.changeset/fix-react-ref-before-hydration.mdpackages/react/runtime/__test__/snapshot/ref.test.jsxpackages/react/runtime/src/snapshot/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/snapshot/ref.tspackages/react/testing-library/src/__tests__/ref.test.jsxpackages/react/testing-library/src/pure.jsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-react-ref-before-hydration.md
When a rerender happens before hydration (e.g. a `useEffect` that
triggers `setState` during the initial background render), the
no-snapshot-patch branch of `BackgroundSnapshotInstance.setAttribute`
used to always pass `null` as the old ref to `queueRefAttrUpdate`.
As a result, the old ref was never invoked with `null` when it should
have been detached, and identical ref identities were re-applied each
render instead of being short-circuited.
Extract old and new refs symmetrically via a shared `getRefFromValue`
helper for both normal (`<view ref={…} />`) and spread
(`<view {...{ ref }} />`) slot forms, and let `queueRefAttrUpdate`
handle identity/clear/apply decisions.
upupming
force-pushed
the
fix/ref-apply-dedup
branch
from
79716c0 to
3975ffb
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
packages/react/testing-library/src/__tests__/ref.test.jsx (1)
487-572: Good coverage of the regression.The matrix over
normal/spreadforms plus theuseRef+useEffecthost-capture test nicely exercises the real-world useEffect→setState→rerender-before-hydration path that the fix targets. The assertions on call counts and thenullcleanup call are precise.Minor nit (optional): in the
seenHoststest (Line 557–572), consider also assertingseenHosts.mock.calls[0][0]is the expectedRefProxyto catch accidentalnullcaptures; as written,seenHostsonly gates on truthyhost, so a regression that always capturesnullwould silently pass the call-count check. Not a blocker.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/testing-library/src/__tests__/ref.test.jsx` around lines 487 - 572, The test "useRef + useEffect + setState host capture is stable (portal-host pattern)" only asserts seenHosts call count which can miss a regression that captures null; update the App test to also assert the captured host is the expected RefProxy shape by checking seenHosts.mock.calls[0][0] (referencing the seenHosts mock and the hostRef/useRef in App) equals or matches an object with ref-like structure (e.g., not null and has the refAttr array used elsewhere in this file), so the test fails if host is null or not the expected RefProxy.packages/react/runtime/src/snapshot/snapshot/backgroundSnapshot.ts (1)
363-371: Fix correctly symmetrizes old/new ref extraction in the pre-hydration branch.Passing
getRefFromValue(oldValue)instead ofnullis exactly the missing piece —queueRefAttrUpdatenow has the information needed to detach the previous ref and short-circuit when identity is unchanged, matching the behavior ofsetAttributeImplon the post-hydration path. Thethis.__values?.[index]guard also correctly handles the first-render case where__valuesis still undefined.One small thought (not blocking):
setAttributeImplstill performs its own ad-hoc extraction at lines 403–404, 418–423, 430, 464–466. SincegetRefFromValuenow exists, a follow-up could consolidate those call sites to use the same helper for consistency, reducing the chance of the two paths drifting again. Leaving as-is for this PR is fine given the stated scope of not touching the hot path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react/runtime/src/snapshot/snapshot/backgroundSnapshot.ts` around lines 363 - 371, In the pre-hydration branch that iterates this.__snapshot_def.refAndSpreadIndexes, ensure old ref extraction is symmetrized by using getRefFromValue(this.__values?.[index]) (i.e., pass getRefFromValue(oldValue) rather than null) so queueRefAttrUpdate receives the previous ref for proper detach/identity checks; keep the this.__values?.[index] guard to handle first-render undefined __values and call queueRefAttrUpdate(getRefFromValue(oldValue), getRefFromValue(v), this.__id, index).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react/runtime/src/snapshot/snapshot/backgroundSnapshot.ts`:
- Around line 363-371: In the pre-hydration branch that iterates
this.__snapshot_def.refAndSpreadIndexes, ensure old ref extraction is
symmetrized by using getRefFromValue(this.__values?.[index]) (i.e., pass
getRefFromValue(oldValue) rather than null) so queueRefAttrUpdate receives the
previous ref for proper detach/identity checks; keep the this.__values?.[index]
guard to handle first-render undefined __values and call
queueRefAttrUpdate(getRefFromValue(oldValue), getRefFromValue(v), this.__id,
index).
In `@packages/react/testing-library/src/__tests__/ref.test.jsx`:
- Around line 487-572: The test "useRef + useEffect + setState host capture is
stable (portal-host pattern)" only asserts seenHosts call count which can miss a
regression that captures null; update the App test to also assert the captured
host is the expected RefProxy shape by checking seenHosts.mock.calls[0][0]
(referencing the seenHosts mock and the hostRef/useRef in App) equals or matches
an object with ref-like structure (e.g., not null and has the refAttr array used
elsewhere in this file), so the test fails if host is null or not the expected
RefProxy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 060aa897-b12d-43c4-a0fc-b68f57a63e30
📒 Files selected for processing (6)
.changeset/fix-react-ref-before-hydration.mdpackages/react/runtime/__test__/snapshot/ref.test.jsxpackages/react/runtime/src/snapshot/snapshot/backgroundSnapshot.tspackages/react/runtime/src/snapshot/snapshot/ref.tspackages/react/testing-library/src/__tests__/ref.test.jsxpackages/react/testing-library/src/pure.jsx
✅ Files skipped from review due to trivial changes (1)
- .changeset/fix-react-ref-before-hydration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react/testing-library/src/pure.jsx
2 participants
Bug
In
BackgroundSnapshotInstance.setAttribute, the pre-hydration branch (__globalSnapshotPatchfalsy) always passednullas the old ref toqueueRefAttrUpdate. So when a rerender happened before hydration (e.g.useEffect→setState), the old ref was never detached and identical refs were re-applied instead of short-circuiting. Post-hydrationsetAttributeImplalready did this correctly — the two paths diverged.Fix
Extract old/new refs symmetrically via a shared
getRefFromValue(val)helper (handles both__refand__spread-with-ref) and letqueueRefAttrUpdatedecide identity/clear/apply. Hot-pathsetAttributeImplis intentionally untouched.testing-library/src/pure.jsxalso movesflushDelayedLifecycleEvents()out ofact()souseEffectfires beforerLynxFirstScreenhydration — matching the real lifecycle.Tests
createRef) → different object refTest plan
pnpm -F @lynx-js/react-runtime test— 560 pass, 100% coverage onbackgroundSnapshot.tspnpm -F @lynx-js/react-testing-library test— 104 passSummary by CodeRabbit
Bug Fixes
Tests
Chores