fix: optimize prefers-reduced-motion listener initialization

[mattgperry]

Summary

• Only initialize the matchMedia listener for prefers-reduced-motion when reducedMotionConfig is not explicitly set to "never" or "always"
• This avoids adding unnecessary event listeners when the motion preference is statically configured
• The render-step memory leak fix from PR fork: fix memory leak in prefers-reduced-motion listener and one othe… #3304 is already present in the codebase

Context

This addresses part of the memory leak issues discussed in:

• PR fork: fix memory leak in prefers-reduced-motion listener and one othe… #3304
• Issue When using DragControls with React Portal leads to memory leaks after unmounting. #2444

When reducedMotionConfig is set to "never" or "always", the dynamic prefersReducedMotion.current value is never read, so there's no need to initialize the matchMedia listener.

Closes #3304

Test plan

• Build passes (yarn build)
• All tests pass (yarn test)

🤖 Generated with Claude Code

[claude]

Code Review: PR #3453

Summary

This PR optimizes the initialization of the matchMedia listener for prefers-reduced-motion by only setting it up when the reduced motion config is dynamically determined (i.e., when it's set to "user" or undefined). When explicitly set to "never" or "always", the listener is skipped entirely.

✅ Positive Aspects

1. Performance & Memory Optimization

• The change effectively prevents unnecessary event listener registration when the motion preference is statically configured
• This directly addresses the memory leak concerns raised in issue When using DragControls with React Portal leads to memory leaks after unmounting. #2444 and PR fork: fix memory leak in prefers-reduced-motion listener and one othe… #3304
• Avoids polling matchMedia when the result will never be used

2. Code Quality

• The logic is clear and well-commented
• The early-return pattern for "never" and "always" cases makes the control flow explicit
• Maintains the same functional behavior while optimizing the common case

3. Correctness

• The implementation correctly handles all three cases of ReducedMotionConfig:
  • "never": Sets shouldReduceMotion = false without listener
  • "always": Sets shouldReduceMotion = true without listener
  • "user" (or undefined): Initializes listener and reads dynamic value

🔍 Observations & Suggestions

1. Test Coverage Gap
While the PR description mentions tests pass, I couldn't find unit tests specifically covering:

• The conditional initialization of initPrefersReducedMotion()
• Verification that the listener is NOT created when config is "never" or "always"
• Memory leak regression tests

Recommendation: Consider adding tests like:

describe("reduced motion listener initialization", () => {
  it("should not initialize listener when reducedMotionConfig is 'never'", () => {
    // Mock initPrefersReducedMotion and verify it's not called
  })
  
  it("should not initialize listener when reducedMotionConfig is 'always'", () => {
    // Mock initPrefersReducedMotion and verify it's not called
  })
  
  it("should initialize listener when reducedMotionConfig is 'user'", () => {
    // Verify listener is created
  })
})

2. Default Case Handling
The code assumes this.reducedMotionConfig has a value, but what if it's undefined? Looking at the type definition ("always" | "never" | "user"), it seems like undefined would fall through to the else block, which is probably correct behavior (defaulting to user preference). However, this could be more explicit:

// Current (implicit):
} else {
    // handles "user" and undefined

// Alternative (explicit):
} else if (this.reducedMotionConfig === "user" || !this.reducedMotionConfig) {

Though the current approach is fine if the default behavior is intentional.

3. Listener Cleanup Consideration
The initPrefersReducedMotion() function in packages/framer-motion/src/utils/reduced-motion/index.ts:14 adds an event listener but I don't see cleanup code:

motionMediaQuery.addEventListener("change", setReducedMotionPreferences)

While this PR doesn't introduce this issue (it was pre-existing), and the listener is only created once per application lifecycle via the hasReducedMotionListener.current guard, it's worth noting for future cleanup. The listener could theoretically persist even after all VisualElements are unmounted.

Potential future enhancement: Track the listener reference and provide a cleanup function, or use a reference counter to remove the listener when the last VisualElement unmounts.

4. Minor: Comment Clarity
The comment says "when config is neither 'never' nor 'always'", which is accurate. For even better clarity, you could add:

/**
 * Determine reduced motion preference. Only initialize the matchMedia
 * listener if we actually need the dynamic value (i.e., when config
 * is neither "never" nor "always", meaning it's "user" or undefined).
 */

🎯 Final Assessment

Approval: ✅ LGTM with minor suggestions

This is a solid optimization that addresses a real performance/memory concern with minimal risk. The logic is sound, the implementation is clean, and it maintains backward compatibility.

Key Strengths:

• Addresses documented memory leak issues
• Clear performance benefit with no functional changes
• Low risk, high value change

Minor Improvement Opportunities:

• Add specific unit tests for this optimization
• Consider documenting the listener lifecycle/cleanup strategy

Great work addressing this issue! 🚀

---

🤖 Review generated with Claude Code

[claude]

Code Review - PR #3453

Summary

This PR optimizes the prefers-reduced-motion listener initialization by only attaching the matchMedia listener when the reducedMotionConfig is set to "user". This is a solid performance optimization that prevents unnecessary event listeners when the motion preference is statically configured.

✅ Strengths

1. Performance Improvement: Correctly avoids creating unnecessary matchMedia listeners when reducedMotionConfig is set to "never" or "always". This is exactly the right approach.

2. Code Clarity: The refactored code in VisualElement.ts:410-427 is more explicit and easier to understand than the previous ternary chain. The comment clearly explains the optimization.

3. Test Coverage: Excellent test suite! The new tests comprehensively cover all three config states, default behavior, multiple components, mixed configurations, and sequential rendering scenarios.

4. Logic Correctness: The implementation correctly sets shouldReduceMotion based on the config.

5. Proper Gating: The hasReducedMotionListener.current check ensures the listener is only initialized once globally.

⚠️ Issues & Recommendations

1. Potential Memory Leak Still Present (Medium Priority)

The matchMedia event listener in packages/framer-motion/src/utils/reduced-motion/index.ts:14 is never removed. While this PR prevents unnecessary listener creation, it does not address cleanup when listeners ARE created.

Recommendation: Consider adding cleanup, accepting it as a singleton pattern, or documenting the intentional behavior.

2. Test Environment Consideration (Low Priority)

The tests do not verify that the actual matchMedia listener behavior is correct in a browser environment. Consider adding tests that mock window.matchMedia and verify the listener is attached and responds to changes.

3. Minor: Test Description Accuracy (Low Priority)

In index.test.tsx:51, consider renaming the test to better reflect it is testing the context default behavior.

🎯 Best Practices Alignment

• ✅ Uses interface for type definitions (following CLAUDE.md)
• ✅ Named exports (no default exports)
• ✅ Arrow callbacks used appropriately
• ✅ Clear, descriptive comments
• ✅ Proper test structure with descriptive test names

🔒 Security Considerations

No security concerns identified.

📊 Performance Impact

Positive: This PR reduces memory overhead and event listener count when reducedMotionConfig is statically set. Well done!

Verdict

Approve with minor suggestions. The core optimization is sound and well-tested. Great work on the comprehensive test coverage! 🎉