Revert "Fix AnimatePresence exit animations with Radix UI asChild"

[mattgperry]

Reverts #3457

[claude]

Code Review Summary

This PR reverts #3457 which attempted to fix AnimatePresence exit animations with Radix UI's asChild pattern. I've analyzed the changes and have some concerns about this revert.

---

Understanding the Changes

What PR #3457 Did (Now Being Reverted)

• Problem: Radix UI's asChild creates new callback refs on each render, causing externalRef in the dependency array to trigger ref recreation
• Solution: Stored externalRef in a ref container using useInsertionEffect to avoid including it in useCallback dependencies
• Trade-off: Removed React 19 cleanup function support to fix the Radix UI issue

What This Revert Does (PR #3459)

• Restores: React 19 cleanup function pattern support
• Adds back: externalRef to the useCallback dependency array along with visualState and visualElement
• Removes: All Radix UI integration tests and the @radix-ui/react-dialog dependency
• Impact: Will likely re-break AnimatePresence exit animations with Radix UI (issue [BUG] - Possible AnimatePresence regression in 12.24.4 #3455)

---

Critical Concerns

1. Breaking Change Without Alternative Solution

This revert will re-introduce the regression reported in issue #3455. The PR removes:

• E2E tests specifically designed to catch the Radix UI bug
• Test files: animate-presence-radix-dialog.tsx, motion-ref-forwarding.tsx
• Cypress tests verifying the fix works

Question: What's the plan to address #3455 after this revert? Users relying on the fix in #3457 will experience broken exit animations again.

2. Problematic Dependency Array

The reverted code includes externalRef, visualState, and visualElement in the dependency array:

[visualElement, visualState, externalRef]

Issues:

• externalRef: Changes on every render with Radix UI's asChild pattern → causes ref callback recreation → breaks AnimatePresence
• visualState: This is a mutable object that doesn't change reference. Including it is misleading and violates React hooks rules (dependencies should be primitive values or stable references)
• Only visualElement makes sense as a dependency

3. React Hooks Best Practices Violation

Including visualState in the dependency array is problematic:

// From use-visual-state.ts - this is a mutable object
export class VisualState<Instance, RenderState> {
    // ... mutable properties
}

Mutable objects shouldn't be in dependency arrays because they don't trigger re-renders when their internal state changes. This creates false expectations about when the callback updates.

4. Incomplete React 19 Support

While the code handles React 19 cleanup functions, it doesn't fully address the compatibility concerns:

const cleanup = setRef(externalRef, instance)
if (typeof cleanup === "function") {
    externalRefCleanupRef.current = cleanup
}

This is forward-compatible, but the dependency array issue will cause the cleanup to be called prematurely when refs change (not just on unmount).

---

Test Coverage Concerns

The PR removes comprehensive test coverage:

• ❌ Removes E2E tests for Radix UI Dialog integration
• ❌ Removes ref forwarding behavior tests
• ❌ Removes tests verifying exit animations actually run (not just complete)

The remaining tests in use-motion-ref.test.tsx:

• ✅ Test React 19 cleanup function pattern
• ✅ Test RefObject handling
• ✅ Test callback ref mount/unmount
• ⚠️ But don't test the specific Radix UI scenario that Fix AnimatePresence exit animations with Radix UI asChild #3457 fixed

---

Recommendations

Option 1: Hold the Revert (Recommended)

Don't merge this revert until there's a solution that:

1. Fixes the Radix UI exit animation issue ([BUG] - Possible AnimatePresence regression in 12.24.4 #3455)
2. Supports React 19 cleanup functions
3. Maintains test coverage for both scenarios

Option 2: If Revert is Necessary

If React 19 support is more critical than Radix UI compatibility:

1. Fix the dependency array:

   // Only include dependencies that actually change
   [visualElement]

2. Document the breaking change:

   • Add a migration guide for Radix UI users
   • Explain the trade-off in the changelog
   • Provide a workaround for [BUG] - Possible AnimatePresence regression in 12.24.4 #3455

3. Keep test coverage:

   • Don't remove the Radix UI tests
   • Mark them as known failures
   • This prevents regression tracking from being lost

Option 3: Find a Hybrid Solution

Investigate if there's a way to support both use cases:

// Pseudocode idea
const stableExternalRef = useStableCallback(externalRef)
// Or use a ref but ensure cleanup functions still work

---

Code Quality Notes

Positive Aspects

• ✅ Well-documented with clear comments
• ✅ Proper TypeScript typing
• ✅ Follows the repository's code style (interfaces, named exports, arrow functions)
• ✅ Good test structure with clear test names

Minor Improvements

1. The setRef function uses (ref as any).current - could use proper typing:

   (ref as React.MutableRefObject<T>).current = value

2. Consider extracting magic into constants if this pattern repeats

---

Security & Performance

• ✅ No security concerns identified
• ✅ No performance regressions (actually might improve perf by avoiding ref recreation in some cases)
• ✅ No memory leaks (cleanup functions are properly stored and called)

---

Final Recommendation

I recommend NOT merging this revert as-is because:

1. It will re-break a critical user-facing bug ([BUG] - Possible AnimatePresence regression in 12.24.4 #3455) without providing an alternative solution
2. The dependency array includes values that shouldn't be there (visualState)
3. It removes valuable test coverage that prevents regressions

Instead, consider:

• Finding a solution that addresses both React 19 compatibility AND Radix UI compatibility
• If that's not possible, document the trade-off and keep the tests as known failures
• Engage with the community to understand which issue is more critical to address first

Would you like me to help investigate a hybrid solution that could support both use cases?