fix(AnimatePresence): Support dynamic mode changes#3531
Conversation
PR ReviewThank you for this well-thought-out fix for dynamic ✅ Strengths1. Clever Architecture DecisionThe core insight—always rendering // Old approach: conditionally wrap
if (mode === "popLayout") {
children = <PopChild>{children}</PopChild>
}
// New approach: always wrap, control behavior
children = <PopChild pop={mode === "popLayout"}>{children}</PopChild>This is a minimal change with maximum impact. 2. Comprehensive Guard ConditionsThe implementation correctly guards all
This ensures 3. Excellent Test CoverageThe two new tests cover both transition directions:
These directly validate the fix for the reported bug. 4. Backward CompatibilityNo breaking changes—existing code continues to work identically since the 🔍 Code Quality ObservationsMinor: Potential Type Safety EnhancementThe
This is correct and intentional, but consider adding a JSDoc comment to interface Props {
// ... other props
/**
* Whether to enable popLayout behavior (measurement and positioning).
* When false, PopChild acts as a transparent passthrough.
* @default true
*/
pop?: boolean
}This would make the default behavior explicit in the type definition. Minor: Test Assertion Could Be More SpecificIn the new tests (lines 744 and 770), the assertions check expect(container.childElementCount).toBe(2) // or .toBe(1)While this validates the fix, consider also asserting that:
However, the current assertions are sufficient for regression prevention. 🎯 Performance ConsiderationsPositive: The change adds negligible overhead:
The performance impact is essentially zero. 🔒 Security ConsiderationsNo security concerns. The change:
🐛 Potential Edge CasesI considered several edge cases that appear to be handled correctly:
📝 Suggestions (Optional)1. Consider an E2E TestWhile the unit tests validate the fix, consider adding a Playwright test that:
This would catch any subtle layout shift or animation timing issues. 2. Documentation UpdateIf there's user-facing documentation about ✅ Final VerdictApproved. This is a high-quality fix that:
The implementation demonstrates a deep understanding of React's reconciliation and the existing 🏷️ Minor NitpickLine 1162 in the test file adds an extra Recommendation: Merge after addressing the optional JSDoc suggestion if desired. The code is production-ready as-is. |
…nimations Always render PopChild wrapper in PresenceChild to keep the React tree stable when AnimatePresence's mode prop changes dynamically (e.g., "wait" → "popLayout"). Add a `pop` prop to PopChild that controls whether measurement and CSS injection are active, preventing tree structure changes that cause React to unmount/remount inner components. Fixes #1717 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
b506645 to
e3bf501
Compare
Code ReviewSummaryThis PR fixes a React component tree stability issue when dynamically switching AnimatePresence modes. The solution is elegant and well-tested. Overall, this is high-quality work that follows the repository's conventions. ✅ Strengths
🔍 Observations & Minor Considerations1. Ref composition skip logic (PopChild.tsx:137-139){pop === false
? children
: React.cloneElement(children as any, { ref: composedRef })}Good: Skipping Question: Is there any edge case where a parent component might expect the ref to still be composed even when pop is disabled? This seems unlikely but worth considering. 2. Guard conditions use strict equality (PopChild.tsx:40, 103)this.props.pop \!== false // Line 40
pop === false // Line 103Observation: Line 40 uses Suggestion: Consider adding a brief comment explaining the default behavior, e.g.: // When pop is undefined, default behavior is to measure/inject styles
if (element && prevProps.isPresent && \!this.props.isPresent && this.props.pop \!== false) {3. Test file change (AnimatePresence.test.tsx:1162)async function complete() {
await nextFrame()
+ await nextFrame() // Why was this added?Question: This appears to be an unrelated fix. Was this test flaky before? If so, it might warrant a brief comment or a separate commit to explain why the extra frame wait is needed. 🎯 Performance Considerations
🔒 Security Concerns
🧪 Test CoverageStrong coverage, but consider:
📋 DocumentationMinor suggestion: Consider adding a JSDoc comment to the interface Props {
children: React.ReactElement
isPresent: boolean
anchorX?: "left" | "right"
anchorY?: "top" | "bottom"
root?: HTMLElement | ShadowRoot
/**
* Controls whether PopChild actively measures and applies positioning.
* When false, PopChild acts as a transparent wrapper.
* @internal
*/
pop?: boolean
}✨ RecommendationApprove with minor suggestions. This PR:
The minor suggestions above are non-blocking and can be addressed in a follow-up if desired. Great work! 🚀 |
Summary
PopChildin thePresenceChildtree to keep the React component structure stable whenAnimatePresence'smodeprop changes dynamically (e.g., "wait" → "popLayout")popprop toPopChildthat controls whether measurement and CSS injection are active — whenpop={false},PopChildacts as a transparent passthroughcloneElementref composition whenpop={false}to avoid unnecessary ref interferenceFixes #1717
Test plan
popLayouttests pass (anchorY bottom positioning)🤖 Generated with Claude Code