Refactor LayoutAnimationBuilder

[mattgperry]

No description provided.

[claude]

See review comment in next message

[claude]

Code Review: Refactor LayoutAnimationBuilder

Overview

This PR refactors the LayoutAnimationBuilder class by consolidating logic and removing several supporting files. The refactor moves from a more distributed architecture to a self-contained builder pattern.

✅ Positive Changes

1. Improved Encapsulation

• All layout animation logic is now centralized in LayoutAnimationBuilder.ts (188→382 lines)
• Eliminates 312 lines from deleted files, net addition of ~194 lines for better clarity
• Clear separation of concerns with well-named private methods

2. Better Snapshot Management
The new snapshotFromTarget function (lines 44-60) is more explicit and robust, creating proper snapshots with animation ID tracking.

3. Improved Element Lifecycle Handling
The handleExitingElements method (lines 193-229) now properly handles edge cases:

• Clears resumeFrom on existing nodes pointing to unmounted projections
• Prevents incorrect crossfade animations when source elements are removed
• Preserves resumeFrom for new nodes and newly promoted lead nodes

4. Enhanced Test Coverage
Added 6 new HTML test files for modal interactions covering basic open/close, interruption scenarios, multiple cycles, and opacity transitions.

[claude]

⚠️ Concerns & Recommendations

1. Test Timeout Doubled (Potential Flakiness)

Location: packages/framer-motion/cypress/integration-html/animate-layout.ts:11

The timeout was doubled from 250ms to 500ms. This suggests the refactored code may be slower or introduces timing issues that could indicate:

• Performance regression
• Race conditions requiring longer settling time
• Asynchronous coordination issues

Recommendation: Profile the animation timing to understand why longer waits are needed. Consider using proper Cypress waiting strategies (cy.should() with retry logic) instead of fixed timeouts.

2. Missing Error Handling in Async Operations

The start() method has multiple await points but no try-catch. The updateDom() call could throw, and promise rejections could cause silent failures.

Recommendation: Add error handling to prevent promise rejections from causing silent failures.

3. Potential Race Condition in Constructor

Lines 85-87: If consumers call .then() synchronously after construction, they might miss errors if start() throws synchronously before the promise handlers are attached.

Recommendation: Consider making the promise setup synchronous in the constructor.

[claude]

4. Transform Cleanup Logic May Be Too Aggressive

Lines 142-149: The code clears transforms when they are identical between current and resumeFrom, but does not check if the transform is intentional vs. a projection artifact. Could this remove user-applied transforms?

Recommendation: Add a comment explaining the reasoning, or add a guard to only clear transforms applied by the animation system.

5. Type Safety Concerns

Line 184: Type assertion transition as Transition bypasses type checking. If AnimationOptions and Transition have type mismatches, this could cause runtime issues.

Recommendation: Fix the type definitions or add runtime validation.

6. WeakMap/Map Memory Management

The code creates multiple Maps (recordMap, sharedTransitions) but some could potentially leak memory if elements are long-lived.

Recommendation: Verify cleanup happens properly, especially for sharedTransitions which persists across calls.

[claude]

⚡ Performance Considerations

Positive:

• Reduced function call overhead by consolidating logic
• Better object reuse with getOrCreateRecord

Concerns:

• collectLayoutElements calls querySelectorAll twice per animation (lines 103, 128)
• Multiple forEach loops over records could be combined
• The doubled test timeout hints at potential slowdown

📋 Code Style & Best Practices

Excellent:

• ✅ Uses interfaces for type definitions (per CLAUDE.md)
• ✅ Named exports only
• ✅ Clear method naming
• ✅ Good separation of concerns

Minor:

• Missing JSDoc comments on public methods
• Magic values (e.g., layoutSelector string) could be constants with documentation

[claude]

🧪 Testing Recommendations

1. Add unit tests for the new private methods:

   • snapshotFromTarget
   • handleExitingElements
   • buildRecords

2. Add specific tests for the edge cases mentioned in comments:

   • Crossfade prevention when source element is removed (line 214)
   • ResumFrom preservation for lead nodes (line 217)

3. Performance regression test: Measure animation timing before/after to validate the timeout increase.

4. Write a failing test first (per CLAUDE.md) for any specific bug this refactor fixes.

🔒 Security

No security concerns identified. The code operates on DOM elements provided by the caller.

Summary

This is a solid refactoring that improves code organization and handles several edge cases better. The main concerns are:

1. The doubled test timeout suggesting potential performance issues
2. Missing error handling in async code
3. Type safety around the AnimationOptions → Transition conversion

Recommendation: Address the timeout increase and async error handling before merging. Consider adding unit tests for the critical new private methods.

[claude]

Code Review: Refactor LayoutAnimationBuilder

Strengths

1. Excellent Test Coverage: Added 6 comprehensive modal test cases covering various scenarios. Tests are included in Cypress suite.

2. Code Consolidation: Consolidates layout animation logic into a single LayoutAnimationBuilder class, removing projection-tree.ts (285 lines) and get-layout-elements.ts (23 lines).

3. Clean Architecture: Builder pattern well-implemented with clear separation of concerns, fluent API, and promise-based async handling.

Areas for Improvement

1. Missing Unit Tests (Critical)
Per CLAUDE.md: "Always write a failing test FIRST before implementing any bug fix or feature."

No unit tests for LayoutAnimationBuilder exist. While E2E tests are valuable, unit tests would verify edge cases in snapshot management, test handleExitingElements logic independently, and validate complex resumeFrom/crossfade logic.

Recommendation: Add Jest tests at packages/motion-dom/src/layout/tests/LayoutAnimationBuilder.test.ts

2. Type Safety Concerns
Line 184: Unsafe type assertion bypasses type checking. Consider using proper type guards.

3. Potential Memory Leak
Lines 66, 171: sharedTransitions Map persists but is never explicitly cleared. Add cleanup or destroy() method.

4. Complex Conditional Logic
Lines 218-228: handleExitingElements logic has nested conditions that could benefit from extraction to a helper function with clear documentation.

5. Missing Error Handling
Lines 85-87: Promise chain could fail silently if updateDom() throws. Consider try-catch.

6. Documentation Gaps

• Line 44: snapshotFromTarget lacks JSDoc
• Lines 193-229: handleExitingElements needs high-level documentation

Security

No security concerns identified.

Performance

1. Lines 159-161: Consider using nextFrame() pattern from CLAUDE.md
2. Line 262: querySelectorAll could be expensive on large DOMs

Summary

Solid refactoring that improves organization. Main concerns:

1. Add unit tests
2. Fix type assertions
3. Improve error handling
4. Add documentation

E2E test coverage is excellent. Once unit tests are added and minor issues addressed, ready to merge.

Recommendation: Request changes for unit tests and type safety.