Fix shared element animations when animate() called before animateLayout()

[mattgperry]

Summary

• Fixes a bug where calling animate(element, { borderRadius: 20 }, { type: false }) before animateLayout() breaks shared element animations
• When animate() is called first, it creates and mounts a VisualElement, but when animateLayout() later creates a projection, projection.mount() was never called because visualElement.current was already set
• This meant registerSharedNode() was never called, so the element wasn't in the shared node stack

Test plan

• New test file modal-open-after-animate.html passes (calls animate() before animateLayout())
• Original modal-open.html test still passes
• Playwright tests pass
• Unit tests pass

🤖 Generated with Claude Code

[claude]

Pull Request Review - PR #3511

Summary

This PR fixes a critical bug where calling animate() before animateLayout() breaks shared element animations. The root cause is that animate() creates and mounts a VisualElement, but when animateLayout() later creates a projection, projection.mount() was never called, preventing registerSharedNode() from executing.

Code Quality: ✅ Excellent

Strengths:

1. Minimal, surgical fix - The 4-line change in LayoutAnimationBuilder.ts:333-337 is perfectly targeted and doesn't introduce unnecessary complexity
2. Clear logic - The conditional check !visualElement.projection.instance correctly identifies when projection mounting was skipped
3. Consistent with existing patterns - The fix mirrors the same check in VisualElement.ts:422-424, showing good code consistency
4. Well-documented - Inline comment clearly explains when and why this edge case occurs

Code Style:

• Follows repository conventions (named exports, interfaces, arrow functions)
• Comment is clear and concise
• No linting issues apparent

Test Coverage: ✅ Strong

Strengths:

1. Comprehensive E2E test - modal-open-after-animate.html (243 lines) thoroughly tests the exact failure scenario
2. Regression test included - Second Playwright test ensures original modal-open.html still works
3. Test-driven approach - The HTML test file includes assertions using matchViewportBox() to verify correct layout positions
4. Integration into test suite - Added to animate-layout-tests.json fixture list for Cypress coverage

Minor Suggestion:

The Playwright tests use page.waitForTimeout(500) which is a time-based wait. Consider using more deterministic waits if possible, though for animation testing this may be acceptable. The HTML test file itself has proper frame-synced assertions, so this is a minor point.

Potential Bugs: ✅ None Identified

The fix is sound. I verified:

• The conditional check prevents double-mounting (only mounts if !projection.instance)
• This matches the existing pattern in VisualElement.mount() (VisualElement.ts:422-424)
• The projection.mount() call will trigger registerSharedNode() when a layoutId is present (create-projection-node.ts:484-486)

Performance Considerations: ✅ Good

• No performance regressions expected
• The additional conditional check is trivial (single property access)
• Only executes when visualElement.current exists but projection isn't mounted - a rare edge case
• No memory leaks introduced - projection lifecycle is properly managed

Security Concerns: ✅ None

This is animation infrastructure code with no user input handling or external data processing. No security concerns identified.

Architecture Considerations: 💡 Suggestion

The fix correctly handles the edge case, but it highlights an interesting architectural question: Should the creation and mounting of projections be more tightly coupled to prevent this class of bugs?

Currently there are two places where we check !projection.instance and call projection.mount():

1. VisualElement.mount() (VisualElement.ts:422-424)
2. getOrCreateRecord() (LayoutAnimationBuilder.ts:333-337) - this PR

This duplication suggests the logic could potentially be encapsulated, though given the different contexts (React mounting vs. layout animation), the current approach is reasonable.

Specific Code Review

LayoutAnimationBuilder.ts:333-337

} else if (!visualElement.projection.instance) {
    // Mount projection if VisualElement is already mounted but projection isn't
    // This happens when animate() was called before animateLayout()
    visualElement.projection.mount(element as HTMLElement)
}

✅ Correct logic flow
✅ Clear comment explaining the edge case
✅ Consistent with VisualElement.mount() implementation
⚠️ Minor: Could potentially check visualElement.projection exists before accessing .instance, but since line 319-324 ensures projection exists, this is safe

Test File: modal-open-after-animate.html

Line 156-157:

const cardContent = document.querySelector('.card-content')
animate(cardContent, { borderRadius: 16 }, { type: false })

✅ Excellent reproduction of the bug scenario - calling animate() before any layout animation

Lines 186-188:

const modalContent = expandedCardContainer.querySelector('.card-content')
animate(modalContent, { borderRadius: 16 }, { type: false })

✅ Tests the scenario on both source and target elements

Recommendations

1. ✅ Approve and merge - This is a solid fix with excellent test coverage
2. 💡 Consider adding a unit test that directly tests the getOrCreateRecord() function with a pre-mounted VisualElement (though the E2E test is sufficient)
3. 📝 Optional: Update the PR description test plan checklist - the items should be checked off before merge

Final Verdict: ✅ APPROVED

This is high-quality work that fixes a real bug with minimal code changes, excellent test coverage, and clear documentation. The fix is architecturally sound and follows the repository's patterns and conventions.

Great job identifying and fixing this edge case! 🎯