Skip to content

Conversation

@mattgperry
Copy link
Collaborator

@mattgperry mattgperry commented Jan 6, 2026

Summary

  • Fixes React 19 deprecation warning "Accessing element.ref was removed in React 19"
  • Updates PopChild to check props.ref first (React 19) and fall back to element.ref (React 18)
  • Adds E2E tests for both React 18 and React 19 to verify the fix

Fixes #3428
Closes #3429

Test plan

  • E2E tests pass on React 18 (make test-react)
  • E2E tests pass on React 19 (make test-react-19)
  • Verify AnimatePresence with mode="popLayout" works with forwardRef components

🤖 Generated with Claude Code

In React 19, element.ref was removed and refs are now passed as regular
props. This change updates PopChild to check props.ref first (React 19)
and fall back to element.ref (React 18) for backwards compatibility.

Fixes #3428

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
@claude
Copy link

claude bot commented Jan 6, 2026

Code Review

Thank you for this fix! This PR addresses the React 19 deprecation warning in a clean and backward-compatible way. Here's my detailed review:

Strengths

  1. Excellent backward compatibility: The nullish coalescing approach gracefully handles both React 18 (element.ref) and React 19 (props.ref) without breaking existing functionality.

  2. Comprehensive test coverage: The PR includes well-designed E2E tests covering two important scenarios:

    • Components using forwardRef + useImperativeHandle (most complex case)
    • Components with direct ref forwarding
  3. Clear documentation: The inline comment (lines 75-77 in PopChild.tsx) explains the React version differences concisely.

  4. Minimal changes: The fix is surgical—only modifying what's necessary without refactoring surrounding code.

  5. Proper test structure: Tests verify both the absence of warnings AND functional behavior, which is the right approach.


🔍 Observations & Suggestions

1. Type Safety Enhancement (Minor)

The unknown type assertion on line 81 is necessary but could benefit from a brief comment:

const childRef =
    (children.props as { ref?: React.Ref<HTMLElement> })?.ref ??
    // React 18 stores ref on the element itself
    (children as unknown as { ref?: React.Ref<HTMLElement> })?.ref

2. Test Robustness (Minor)

The Cypress tests capture console errors/warnings, but they could be more specific:

// Current approach is good, but consider also checking for:
const hasUnexpectedErrors = consoleErrors.filter(
    msg => !msg.includes('expected warning patterns')
).length > 0

This would catch any OTHER unexpected errors during the test.

3. Edge Case Question

Have you tested with:

  • Nested forwardRef components?
  • Components that don't use refs at all (should still work, but good to verify)?
  • Refs that are callback refs instead of useRef objects?

The current implementation should handle these, but explicit tests would provide confidence.

4. Test Component Cleanup (Very Minor)

In animate-presence-pop-ref.tsx:

  • Lines 80-81 declare refs that are never used (boxRef, directRef)
  • The flash() method in BoxHandle is implemented but never called

These don't affect functionality but could be cleaned up for clarity.


🔒 Security

✅ No security concerns identified.


Performance

✅ No performance concerns. The nullish coalescing operator is highly optimized and adds negligible overhead.


🐛 Potential Issues

None identified. The implementation is sound.


📋 Code Style Compliance

✅ Follows repository conventions:

  • Named exports used
  • Interface for type definitions (BoxHandle)
  • Proper arrow callbacks
  • No ESLint violations expected

🎯 Overall Assessment

This is a high-quality fix that I recommend merging. It solves the reported issue (#3428) elegantly while maintaining backward compatibility. The test coverage is thorough and the code changes are minimal and well-documented.

Minor Suggestions Summary:

  1. Consider adding a comment for the unknown type assertion
  2. Optional: Add edge case tests for nested refs and callback refs
  3. Optional: Clean up unused variables in test file

These are all non-blocking suggestions. The PR is ready to merge as-is.

Great work! 🎉

@mattgperry mattgperry merged commit 264b1a3 into main Jan 6, 2026
4 checks passed
@mattgperry mattgperry deleted the popchild-refs branch January 6, 2026 11:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] React 19 refs

2 participants