fix: don't double-invoke effects for moved children in StrictMode

[everettbu]

Mirror of facebook/react#35916
Original author: emmaeng700

---

Summary

Fixes #32561 — in React 19, StrictMode was incorrectly re-running effects (even with [] dependencies) when a keyed child is reordered within an array. This does not happen in React 18 or in production builds.

Root Cause

placeChild() in ReactChildFiber.js sets PlacementDEV on both newly inserted fibers and moved fibers (those with an existing alternate whose oldIndex < lastPlacedIndex). The StrictMode double-invoke logic in doubleInvokeEffectsInDEVIfNecessary treats any fiber with PlacementDEV as a new mount and re-runs its effects.

A moved fiber is not a new mount — it already has state, refs, and a lifecycle. Re-running its effects in dev mode is incorrect and diverges from production behavior.

Fix

Only set PlacementDEV for actual insertions (current === null). Moved fibers still receive the Placement flag so the host node is repositioned in the DOM, but PlacementDEV is omitted so StrictMode does not treat them as new mounts.

// Before
if (oldIndex < lastPlacedIndex) {
  // This is a move.
  newFiber.flags |= Placement | PlacementDEV; // <- wrong
}

// After
if (oldIndex < lastPlacedIndex) {
  // This is a move — DOM node moves but component is not newly mounting.
  newFiber.flags |= Placement; // PlacementDEV intentionally omitted
}

Test

Added a test in StrictEffectsMode-test.js that renders [A, B, C] in StrictMode, reorders to [C, A, B], and asserts that no effect callbacks fire (no mount/unmount of useEffect or useLayoutEffect with [] deps).

Checklist

• Reproduces the bug with a new test
• Fix is minimal and targeted — one line change in placeChild()
• No behavior change in production (the PlacementDEV path is dev-only)
• No behavior change for actual new insertions

[greptile-apps]

Greptile Summary

This PR fixes a StrictMode bug where reordering keyed children in an array incorrectly double-invoked effects (even with [] dependencies). The root cause was placeChild() in ReactChildFiber.js setting PlacementDEV on moved fibers, which caused doubleInvokeEffectsInDEVIfNecessary to treat them as new mounts. The fix is minimal: only set PlacementDEV for actual insertions (current === null), not moves.

Additionally:

• Fixes a TypeError when hook dependency arrays contain Symbol values by using .map(String).join() instead of .join() in the dev warning message for both ReactFiberHooks.js and ReactFizzHooks.js
• Fixes a copy-paste ref bug (ref2 → ref3) in a ReactFabric timestamp test
• Adds regression tests for both the StrictMode fix and the Symbol handling fix

Confidence Score: 5/5

• This PR is safe to merge — it fixes a dev-only bug with a minimal, well-targeted change that has no production impact.
• The core fix is a single-line change that only affects dev-mode behavior (PlacementDEV is a dev-only flag). The Symbol handling fix is also dev-only (warning messages). Both changes are accompanied by regression tests. The logic is sound: moved fibers have current !== null and should not be treated as new mounts.
• No files require special attention. The pre-existing swapped labels in ReactFizzHooks.js was already flagged in a prior review thread.

Important Files Changed

Filename	Overview
packages/react-reconciler/src/ReactChildFiber.js	Core fix: removes PlacementDEV flag from moved fibers so StrictMode doesn't double-invoke effects for reordered children. Correct and minimal change.
packages/react-reconciler/src/ReactFiberHooks.js	Uses .map(String) instead of plain .join() to handle Symbol values in dependency array warning messages, preventing TypeError.
packages/react-server/src/ReactFizzHooks.js	Applies same .map(String) fix for Symbol handling. Pre-existing swapped label issue (nextDeps/prevDeps) was already flagged in a prior review.
packages/react-reconciler/src/tests/StrictEffectsMode-test.js	New test verifying that reordering keyed children in StrictMode does not double-invoke effects. Well-structured regression test.
packages/react-reconciler/src/tests/ReactHooks-test.internal.js	New test verifying that Symbol values in hook dependencies produce correct warning messages with .map(String).
packages/react-native-renderer/src/tests/ReactFabric-test.internal.js	Bug fix: corrects ref2 → ref3 for the third View element in the timeStamp propagation test.

_{Last reviewed commit: a2b2040}

[greptile-apps]

_{6 files reviewed, 1 comment}

_{Edit Code Review Agent Settings | Greptile}

[greptile-apps]

Pre-existing swapped labels for Previous/Incoming

The nextDeps and prevDeps arguments are passed in the wrong order relative to their labels. "Previous" is being populated with nextDeps and "Incoming" with prevDeps, which is backwards. Compare with ReactFiberHooks.js (lines 486-487), where the ordering is correct:

`[${prevDeps.map(String).join(', ')}]`,  // ← "Previous"
`[${nextDeps.map(String).join(', ')}]`,  // ← "Incoming"

This bug predates this PR (existed before the .map(String) fix was applied), but since you're already touching these lines it would be good to fix the ordering:

Suggested change

	`[${nextDeps.map(String).join(', ')}]`,
	`[${prevDeps.map(String).join(', ')}]`,
	`[${prevDeps.map(String).join(', ')}]`,
	`[${nextDeps.map(String).join(', ')}]`,

Prompt To Fix With AI

This is a comment left during a code review.
Path: packages/react-server/src/ReactFizzHooks.js
Line: 154-155

Comment:
**Pre-existing swapped labels for Previous/Incoming**

The `nextDeps` and `prevDeps` arguments are passed in the wrong order relative to their labels. "Previous" is being populated with `nextDeps` and "Incoming" with `prevDeps`, which is backwards. Compare with `ReactFiberHooks.js` (lines 486-487), where the ordering is correct:

```js
`[${prevDeps.map(String).join(', ')}]`,  // ← "Previous"
`[${nextDeps.map(String).join(', ')}]`,  // ← "Incoming"
```

This bug predates this PR (existed before the `.map(String)` fix was applied), but since you're already touching these lines it would be good to fix the ordering:

```suggestion
        `[${prevDeps.map(String).join(', ')}]`,
        `[${nextDeps.map(String).join(', ')}]`,
```

How can I resolve this? If you propose a fix, please make it concise.

[everettbu]

Upstream PR was closed or merged. Code is synced via branch mirror.