Fix AnimatePresence exit animations with Radix UI asChild

[mattgperry]

Summary

• Fixes AnimatePresence exit animations not running when using Radix UI's asChild prop with motion components
• Stores externalRef in a ref instead of including it in useCallback dependency array
• Preserves React 19 cleanup function support added in previous PRs

Problem

When using Radix UI Dialog (or similar libraries) with asChild and motion components inside AnimatePresence, exit animations were not running. The elements were immediately removed from the DOM instead of animating out.

<AnimatePresence>
  {isOpen ? (
    <DialogPrimitive.Portal forceMount>
      <DialogPrimitive.Overlay asChild>
        <motion.div exit={{ opacity: 0 }} />  {/* Exit animation not working */}
      </DialogPrimitive.Overlay>
    </DialogPrimitive.Portal>
  ) : null}
</AnimatePresence>

Root Cause

In use-motion-ref.ts, externalRef was in the useCallback dependency array. Radix UI's asChild creates a new composed callback ref on each render. This caused:

1. useCallback to return a new function each render
2. React to call the ref with null (unmount) then the new instance (remount)
3. AnimatePresence exit animations to be bypassed

Solution

Store externalRef in a ref (externalRefRef) and access the current value inside the callback, rather than including it in dependencies. This maintains:

• React 19 cleanup function support
• Proper ref forwarding
• AnimatePresence exit animations working correctly

Test plan

• Added E2E test with @radix-ui/react-dialog reproducing the exact issue from [BUG] - Possible AnimatePresence regression in 12.24.4 #3455
• All 702 unit tests pass
• All 18 AnimatePresence E2E tests pass (including new Radix Dialog tests and existing pop-ref tests from Fix React 19 ref compatibility in AnimatePresence popLayout mode #3452)

Fixes #3455

🤖 Generated with Claude Code

[claude]

PR Review

Summary

This PR successfully fixes a critical regression where AnimatePresence exit animations were not running when using Radix UI's asChild prop with motion components. The root cause was identified correctly, and the solution is elegant and maintains backward compatibility with React 19's cleanup function support.

✅ Code Quality & Best Practices

Strengths:

• Clean solution: The fix correctly identifies that externalRef shouldn't be in the dependency array of useCallback. By storing it in a ref (externalRefRef), the callback remains stable while still accessing the latest ref value.
• Excellent documentation: The comments in use-motion-ref.ts (lines 34-42 and 78-84) clearly explain the rationale, making it easy for future maintainers to understand why this pattern is necessary.
• Follows existing patterns: Using a ref to store a value that needs to be current but shouldn't trigger re-renders is a well-established React pattern.
• Maintains compatibility: The fix preserves React 19 cleanup function support added in previous PRs while fixing the Radix UI issue.

Style compliance:

• ✅ Uses interface for type definitions (where applicable)
• ✅ Named exports used throughout
• ✅ Arrow functions used appropriately in test code
• ✅ Consistent with repository conventions

✅ Implementation Correctness

The core change is sound:

1. Before: externalRef in dependency array → new callback on every render when Radix changes the ref → triggers remount → breaks exit animations
2. After: externalRef stored in ref → stable callback → no remount → exit animations work

Key correctness points:

• ✅ externalRefRef.current is updated on every render (line 42), ensuring the latest ref is always used
• ✅ Cleanup function handling is preserved (lines 68-74)
• ✅ Both mount and unmount paths correctly use currentExternalRef (lines 58-76)
• ✅ The dependency array now only contains visualElement, which is the only value that genuinely needs to trigger callback recreation

✅ Test Coverage

Excellent test coverage:

• ✅ E2E test in animate-presence-radix-dialog.tsx reproduces the exact issue from [BUG] - Possible AnimatePresence regression in 12.24.4 #3455
• ✅ Two Cypress tests verify:
  1. Exit animations complete properly (onExitComplete fires, elements removed)
  2. Elements stay in DOM during animation (not immediately removed)
• ✅ Tests use actual Radix UI Dialog with asChild, ensuring real-world compatibility
• ✅ PR description states all 702 unit tests + 18 AnimatePresence E2E tests pass

Test quality:

• Clear test names and descriptions
• Proper use of waits to account for animation timing (300ms duration)
• Verification of both the animation lifecycle and final DOM state
• Uses data-value attributes for reliable assertions

🔍 Potential Considerations

Minor observations (not blocking):

1. visualState dependency: The comment states "visualState: stable across renders" - while this is likely true, it might be worth confirming that visualState truly never changes identity during a component's lifetime. If it could change, excluding it might cause stale closures.

2. Ref timing edge case: There's a theoretical edge case where externalRef could change between when the callback is called and when externalRefRef.current is read. However, this is extremely unlikely in practice and would only matter if:

   • The component rerenders mid-callback execution
   • The new ref is different from the old one
   • The timing window is infinitesimally small

   This is not a realistic concern and doesn't warrant changes.

3. Test timing hardcoding: The Cypress tests use hardcoded waits (.wait(100), .wait(500)) which match the 300ms animation duration. While functional, these could potentially be flaky on slower CI systems. Consider using more robust waiting strategies if flakiness occurs in the future.

✅ Performance

No performance concerns:

• Adding one additional ref (externalRefRef) has negligible memory overhead
• Actually improves performance by preventing unnecessary callback recreations
• No new re-renders introduced

✅ Security

No security concerns identified. The changes are internal to the ref management system and don't introduce any attack vectors.

🎯 Final Assessment

Recommendation: APPROVED ✅

This is a well-crafted fix that:

• Correctly identifies and solves the root cause
• Includes comprehensive tests reproducing the exact issue
• Maintains backward compatibility
• Follows repository conventions
• Is well-documented

The implementation is clean, the testing is thorough, and the PR description clearly explains the problem, root cause, and solution. This is exactly how bug fixes should be done.

Excellent work! 🚀

[claude]

Code Review

I've reviewed this PR and the implementation looks solid overall. Here's my detailed feedback:

✅ Strengths

1. Problem Analysis: The root cause analysis is excellent - clearly identifying that Radix UI's asChild creates new callback refs on each render, which was triggering ref remounts and breaking exit animations.

2. Elegant Solution: Using useInsertionEffect to update a ref container is a clean pattern that avoids the dependency array issue while maintaining correctness in concurrent mode.

3. Excellent Test Coverage:

   • Comprehensive E2E tests with real Radix UI Dialog integration
   • Tests verify both that exit animations complete AND that they actually run (not immediately removed)
   • Tests cover the exact bug scenario from issue [BUG] - Possible AnimatePresence regression in 12.24.4 #3455

4. Documentation: The inline comments are thorough and explain the "why" behind the approach, which will help future maintainers.

5. Backward Compatibility: The fix preserves React 19 cleanup function support from PR Fix React 19 ref compatibility in AnimatePresence popLayout mode #3452.

🔍 Observations & Potential Concerns

1. useInsertionEffect Dependency Array

useInsertionEffect(() => {
    externalRefContainer.current = externalRef
})

Issue: This useInsertionEffect is missing a dependency array. According to React docs, effects without dependencies run after every render.

Analysis: While this works correctly (the ref is updated before layout effects), it's unconventional. Looking at other useInsertionEffect usage in the codebase:

• use-motion-value-event.ts:20: includes [value, event, callback]
• use-spring.ts:64: includes [value, JSON.stringify(options)]

Recommendation: Consider adding [externalRef] as the dependency array for clarity and consistency:

useInsertionEffect(() => {
    externalRefContainer.current = externalRef
}, [externalRef])

This would:

• Make the intent explicit (update when externalRef changes)
• Follow the same pattern as other effects in the codebase
• Satisfy linting rules (if exhaustive-deps is enabled)
• Still solve the original problem (the ref container stays stable, only its .current updates)

2. Edge Case: Initial Render

The externalRefContainer is initialized with useRef(externalRef), which captures the initial value. The useInsertionEffect then runs to update it. This means:

• On first render: container holds initial externalRef
• After insertion effect: container updated to current externalRef

This is correct, but in the unlikely event that externalRef changes between initial render and insertion effect in concurrent mode, there's a tiny window where the old ref is in the container. This is probably not a real-world issue, but worth being aware of.

3. Type Safety

The code correctly handles all ref types (callback refs, ref objects, and React 19 cleanup functions). No issues here.

4. Performance

The fix actually improves performance by reducing unnecessary ref callback invocations. Previously, Radix's changing callback refs would trigger the whole ref callback to be recreated and called with null then the new instance. Now it's stable. ✅

🔒 Security Considerations

No security concerns. The changes are limited to ref handling and don't introduce any injection vulnerabilities, XSS risks, or unsafe operations.

🐛 Potential Bugs

None identified. The logic correctly:

• Handles both mount and unmount paths
• Preserves React 19 cleanup function behavior
• Falls back to React <19 behavior when no cleanup is returned
• Maintains visualElement lifecycle

📊 Test Coverage Assessment

Excellent coverage:

• ✅ Real-world integration test with Radix UI
• ✅ Verifies onExitComplete callback fires
• ✅ Verifies elements are actually animating (not immediately removed)
• ✅ Verifies DOM cleanup after animation completes
• ✅ Tests the specific bug scenario from issue [BUG] - Possible AnimatePresence regression in 12.24.4 #3455

Optional enhancement: Consider adding a unit test that directly tests useMotionRef with changing callback refs to document the fix at a lower level. Not critical since E2E tests cover it.

📝 Code Quality

• ✅ Follows repository conventions (no default exports, named exports)
• ✅ Clear, descriptive variable names
• ✅ Excellent documentation
• ✅ TypeScript types are correct
• ⚠️ Minor: Missing dependency array on useInsertionEffect (see concern Initial API (WIP) #1)

🎯 Verdict

Approve with minor suggestion: The PR successfully fixes the AnimatePresence bug with Radix UI's asChild. The approach is sound and well-tested.

Suggested change (non-blocking): Add [externalRef] to the useInsertionEffect dependency array for consistency and clarity.

---

Great work tracking down this subtle bug! The fix is elegant and the test coverage ensures it won't regress. 🎉

[claude]

Code Review: Fix AnimatePresence exit animations with Radix UI asChild

Summary

This PR successfully addresses a critical bug where AnimatePresence exit animations were not running when using Radix UI's asChild prop. The solution is elegant and well-tested.

---

✅ Strengths

1. Root Cause Analysis

The PR correctly identifies the issue: Radix UI creates new callback refs on each render with asChild, and including externalRef in the useCallback dependency array caused unnecessary recreations of the ref callback, triggering remounts that bypassed exit animations.

2. Clean Solution (use-motion-ref.ts:24-27)

Using useInsertionEffect to keep externalRefContainer.current synchronized is the right approach:

• Avoids dependency array issues
• useInsertionEffect fires before layout effects, ensuring refs are always current
• Maintains React 19 forward compatibility

3. Comprehensive Test Coverage

• E2E tests with actual Radix UI Dialog reproduce the exact user-reported issue
• Ref forwarding tests verify both RefObject and callback ref behavior
• Tests cover both React 18 and React 19 scenarios
• Tests verify exit animations actually run (not just complete)

4. Code Style Compliance

• Uses optional chaining (visualState.onMount?.())
• Clean ternary operators
• Good inline documentation

---

💡 Observations & Suggestions

1. React 19 Cleanup Function Support

The new implementation at line 40-44 correctly passes through callback ref return values (React 19 cleanup functions), but this behavior is no longer explicitly tested since the tests were simplified.

Consideration: The E2E test at motion-ref-forwarding.tsx:34-40 returns a cleanup function, but the Cypress test (motion-ref-forwarding.ts:21-33) doesn't verify that cleanupCalled becomes true. This might be intentional if React 19 cleanup functions aren't fully supported yet, but it's worth clarifying.

2. Potential Edge Case: Ref Switching

The test at line 50-64 in use-motion-ref.test.tsx verifies switching between callback and RefObject refs works, which is good. However, with the new implementation where externalRef is stored in a ref, there's a subtle timing consideration:

const externalRefContainer = useRef(externalRef)
useInsertionEffect(() => {
    externalRefContainer.current = externalRef
})

On the first render, externalRefContainer.current initially holds the initial externalRef value, and only updates in useInsertionEffect. The callback ref (line 29-45) might fire before useInsertionEffect runs. This is likely fine in practice since:

• The callback is created in the same render
• useInsertionEffect runs before layout effects and browser paint
• But it's worth documenting this timing assumption

3. Type Assertion (line 43)

;(ref as React.MutableRefObject<Instance>).current = instance

This type assertion is safe because we check typeof ref === "function" first, but the semicolon prefix is unusual. Consider:

} else if (ref) {
    (ref as React.MutableRefObject<Instance>).current = instance
}

The leading semicolon is defensive programming for ASI but unnecessary here since the previous line doesn't create ambiguity.

4. Dependency Array (line 46)

[visualElement]

Good! Removing externalRef from dependencies is the core fix. However, visualState is also not in the array. This is correct since:

• visualState should be stable (created once per component instance)
• But worth confirming this assumption is documented in useVisualState

---

🔒 Security Considerations

No security concerns identified. The changes:

• Don't introduce XSS, injection, or other OWASP risks
• Don't expose internal state
• Handle refs safely with proper null checks

---

⚡ Performance Considerations

Positive impact:

• Reduces unnecessary ref callback recreations (the bug being fixed)
• useInsertionEffect is faster than useLayoutEffect or useEffect
• Dependency array is smaller (better for React's reconciliation)

No performance regressions expected.

---

🧪 Test Coverage

Excellent coverage:

• ✅ Unit tests for basic ref forwarding
• ✅ E2E tests with real Radix UI integration
• ✅ Tests verify animations actually run (not just complete)
• ✅ Tests cover both React 18 and React 19

Minor gap: React 19 cleanup function behavior tested in E2E but not verified in the Cypress test.

---

📝 Code Quality

Overall: Excellent

• Clear, focused changes
• Well-documented problem and solution
• Follows repository conventions (interface usage, named exports, etc.)
• Good commit message practices

---

🎯 Recommendation

APPROVE ✅

This is a high-quality fix that:

1. Correctly identifies and solves the root cause
2. Has comprehensive test coverage
3. Maintains backward compatibility
4. Follows best practices

Optional Follow-ups (non-blocking):

1. Consider adding a comment explaining why visualState is not in the dependency array
2. Verify React 19 cleanup function E2E test coverage (may be intentionally deferred)
3. Consider documenting the timing assumption about useInsertionEffect and ref callback execution order

Great work solving this tricky interaction between Framer Motion, React refs, and Radix UI! 🚀