Add transition.inherit to allow transition composition

[mattgperry]

Summary

• Adds inherit: true to Transition

[claude]

PR Review: Add inheritTransition prop to MotionConfig

Summary

This PR adds an opt-in inheritTransition prop to MotionConfig that enables shallow-merging of transitions with parent configs instead of full replacement. The implementation is clean and well-tested.

✅ Strengths

1. Well-designed API: The opt-in approach preserves backward compatibility perfectly
2. Comprehensive test coverage: Four test cases cover the key scenarios including edge cases
3. Clear documentation: JSDoc comments explain the feature well
4. Follows repository conventions: Uses named exports, interfaces, and matches existing code style

🔍 Code Quality Review

Implementation (packages/framer-motion/src/components/MotionConfig/index.tsx:47-49)

The merge logic is clean and straightforward:

if (config.inheritTransition) {
    config.transition = { ...parentConfig.transition, ...config.transition }
}

Minor concern: The inheritTransition flag itself is being passed down through the context and included in the useMemo dependency array. This means inheritTransition becomes part of the config context, which seems intentional but worth noting.

Context Definition (packages/framer-motion/src/context/MotionConfigContext.tsx:56-62)

Good JSDoc documentation added. Interface follows existing patterns.

⚠️ Potential Issues

1. useMemo dependency array may cause unnecessary re-renders

In index.tsx:61-70, the dependency array includes:

[
    JSON.stringify(config.transition),
    config.transformPagePoint,
    config.reducedMotion,
    config.skipAnimations,
    config.inheritTransition,  // Added in this PR
]

Issue: When inheritTransition changes, it triggers a re-render even though inheritTransition itself isn't used by child components—only its effect on transition matters. The JSON.stringify(config.transition) should capture the merged result already.

Impact: Low—in practice, inheritTransition is unlikely to change dynamically, but this creates an unnecessary dependency.

Suggestion: Consider whether inheritTransition needs to be in the dependency array, or if it should be excluded since its effect is already captured by the stringified transition.

2. Shallow merge limitations not documented

The implementation does shallow merge only:

config.transition = { ...parentConfig.transition, ...config.transition }

Issue: If transitions contain nested objects (e.g., transition: { type: "spring", spring: { stiffness: 100 } }), the nested objects won't merge—they'll replace.

Impact: Medium—this could surprise users who expect deep merging behavior.

Suggestion: Add a note to the JSDoc that this is a shallow merge, and nested transition objects will replace rather than merge.

3. Edge case: inheritTransition without transition prop

Consider this scenario:

<MotionConfig transition={{ type: "spring" }}>
  <MotionConfig inheritTransition>
    {/* What transition does this have? */}
  </MotionConfig>
</MotionConfig>

When config.transition is undefined but inheritTransition is true, the spread will work correctly:

{ ...parentConfig.transition, ...undefined } // Results in parentConfig.transition

This is actually correct behavior! But it's a subtle edge case worth having a test for.

Suggestion: Add a test case for inheritTransition without a transition prop to document this behavior.

🚀 Performance Considerations

• The shallow merge operation is O(n) where n = number of transition keys, which is negligible
• No new re-render concerns beyond the existing JSON.stringify dependency
• Overall performance impact: Minimal

🔒 Security Concerns

• No security issues identified
• The feature doesn't introduce any user-controlled code execution or data leakage vectors

✅ Test Coverage

Excellent test coverage with four scenarios:

1. ✅ Existing behavior preserved (no inheritTransition)
2. ✅ Basic shallow merge functionality
3. ✅ Inner keys override parent keys
4. ✅ Cascading through deeply nested configs

Missing test cases:

• inheritTransition set but no transition prop provided (edge case mentioned above)
• Behavior with complex nested transition objects (to document shallow merge behavior)

📋 Recommendations

Required Changes

None—the PR is functionally correct and ready to merge.

Suggested Improvements (Optional)

1. Clarify shallow merge in documentation:

/**
 * If true, this MotionConfig's transition will be shallow-merged with the
 * parent MotionConfig's transition instead of replacing it.
 *
 * Note: This performs a shallow merge only. Nested transition objects will
 * be replaced rather than merged.
 *
 * @public
 */
inheritTransition?: boolean

2. Consider removing inheritTransition from useMemo deps if the merged transition already captures its effect.

3. Add edge case test:

it("inheritTransition without transition prop inherits parent transition", () => {
    const { getByTestId } = render(
        <MotionConfig transition={{ type: "spring", duration: 1 }}>
            <MotionConfig inheritTransition>
                <TransitionConsumer />
            </MotionConfig>
        </MotionConfig>
    )

    const transition: Transition = JSON.parse(
        getByTestId(consumerId).textContent!
    )
    expect(transition.type).toBe("spring")
    expect(transition.duration).toBe(1)
})

🎯 Final Verdict

APPROVE ✅

This is a well-implemented feature that:

• Solves the reported issue ([BUG] MotionConfig is overridden completely once transition is used for an animation #2271)
• Maintains backward compatibility
• Includes comprehensive tests
• Follows repository conventions
• Has minimal performance impact

The suggested improvements are minor and optional. The code is production-ready as-is.

Great work! 🚀

---

Review performed by Claude Code

[claude]

Code Review

I've reviewed PR #3538 which adds transition inheritance functionality to MotionConfig. Overall, this is a well-implemented feature that solves a real user pain point (#2271). Here's my detailed feedback:

---

✅ Strengths

1. Clean API Design

The evolution from a separate inheritTransition prop to transition.inherit is excellent. This enables merging at both MotionConfig nesting and value-specific levels (e.g., opacity: { inherit: true, ... }), which is more flexible and intuitive.

2. Comprehensive Test Coverage

• 87 test suites with 717 tests passing is excellent
• New tests cover all critical scenarios:
  • Default replacement behavior (backwards compatibility)
  • Inheritance merging
  • Key override precedence
  • Deep nesting cascade
  • Value-specific inheritance in getValueTransition
• Tests properly verify the inherit key is stripped from results

3. Backwards Compatibility

The opt-in nature via inherit: true means existing code continues to work unchanged. This is the right approach.

4. Implementation Quality

• Clean separation of concerns between MotionConfig and getValueTransition
• Proper use of object destructuring to strip the inherit key
• Consistent shallow merge strategy

---

🔍 Potential Issues & Suggestions

1. Type Safety Concern (Minor)

Location: packages/motion-dom/src/animation/utils/get-value-transition.ts:1

The transition parameter is typed as any, which bypasses type checking:

export function getValueTransition(transition: any, key: string) {

Recommendation: Consider typing this as Transition | undefined for better type safety, or at minimum add a comment explaining why any is necessary here.

2. Edge Case: Circular Reference Risk (Low Priority)

Location: packages/framer-motion/src/components/MotionConfig/index.tsx:47-50

While unlikely in practice, if a user inadvertently creates circular references in nested MotionConfigs with inherit, the shallow merge won't protect against this. The current implementation handles this fine for normal use cases, but consider if documentation should warn about this.

3. Documentation Gap

The inherit property in ValueTransition interface has good JSDoc (packages/motion-dom/src/animation/types.ts:472-478), but users might benefit from examples showing:

• How inherit works at the MotionConfig level
• How inherit works at the value-specific level (e.g., opacity: { inherit: true, delay: 0.5 })
• The precedence rules (inner keys win)

Consider adding examples to the MotionConfig component JSDoc.

4. useMemo Dependency (Correctness Check)

Location: packages/framer-motion/src/components/MotionConfig/index.tsx:62-70

The useMemo dependencies include JSON.stringify(config.transition) to detect changes. This works but:

• After the inherit merge at line 49, the resulting config.transition object is new on every render where inherit is used
• This could cause unnecessary re-renders of all motion components

Test this scenario:

<MotionConfig transition={{ duration: 1 }}>
  <MotionConfig transition={{ inherit: true, delay: 0.5 }}>
    <motion.div />
  </MotionConfig>
</MotionConfig>

Does the inner MotionConfig create a new context on every render? If yes, consider memoizing the merged transition separately.

5. Whitespace Issue (Nitpick)

Location: packages/framer-motion/src/context/MotionConfigContext.tsx:54

There's an extraneous blank line added after skipAnimations?: boolean. This appears to be unintentional.

---

🔒 Security

No security concerns identified. The feature doesn't introduce any injection risks or unsafe operations.

---

⚡ Performance

Performance impact should be minimal:

• Shallow object spreading is fast
• The inherit check is a simple boolean comparison
• Most of the work happens during render, not during animation

The only concern is the useMemo dependency mentioned above (item #4).

---

📝 Code Style

Follows the repository conventions:

• Uses interface for type definitions ✓
• Named exports ✓
• Arrow functions where appropriate ✓
• Strict equality ✓

---

🎯 Recommendations Priority

1. High: Verify the useMemo behavior doesn't cause excessive re-renders (item adding circleci config #4)
2. Medium: Improve type safety in getValueTransition (item Initial API (WIP) #1)
3. Low: Add documentation examples (item Add motionValue prop #3)
4. Low: Remove extraneous whitespace (item Initial event setup #5)

---

✨ Verdict

This is a solid implementation of a highly requested feature. The test coverage is excellent, the API design is clean, and backwards compatibility is preserved. The concerns raised are minor and don't block merging, but addressing the useMemo dependency (item #4) would be good to verify before release.

Great work! 🚀

[claude]

Code Review

Overview

This PR adds transition inheritance capability to MotionConfig via an inherit flag. The implementation is clean, well-tested, and follows the repository's conventions. Overall, this is a solid implementation with good test coverage.

---

✅ Strengths

1. Excellent test coverage - Comprehensive tests covering:

   • Backward compatibility (non-inherit behavior preserved)
   • Basic inheritance with shallow merge
   • Override behavior (inner keys win)
   • Deep nesting cascade
   • Proper stripping of inherit key from results

2. Non-breaking change - Opt-in behavior preserves existing functionality perfectly

3. Consistent architecture - The resolveTransition utility is reused in multiple places (MotionConfig, animateTarget, getValueTransition), following DRY principles

4. Clear documentation - JSDoc comments explain the behavior concisely

5. Size-conscious implementation - Following the CLAUDE.md guidance on prioritizing small file size, the implementation is minimal and uses optional chaining effectively

---

🔍 Potential Issues & Suggestions

1. Performance: Repeated object spreading on hot path

In packages/motion-dom/src/animation/interfaces/visual-element-target.ts:44-46:

transition = transition
    ? resolveTransition(transition, defaultTransition)
    : defaultTransition

This calls resolveTransition on every animation, and when inherit: true, it creates a new object via { ...parentTransition, ...rest }. For high-frequency animations (e.g., scroll-linked, gesture-driven), this could cause unnecessary allocations.

Impact: Low-to-medium depending on animation frequency
Suggestion: Consider memoization or early-exit optimization if !transition?.inherit

2. Edge case: inherit key pollution

In get-value-transition.ts:9-11:

if (valueTransition !== transition) {
    return resolveTransition(valueTransition, transition)
}

If the base transition object itself has inherit: true (not just value-specific transitions), this would pass it through to resolveTransition with parentTransition = transition, causing it to try merging with itself. While the test on line 58-66 of the test file confirms this returns the transition as-is, the logic could be clearer.

Actual behavior: Works correctly (test confirms)
Suggestion: Add a comment explaining this edge case handling, or add an early guard

3. Type safety: any usage

resolveTransition and getValueTransition use any types for flexibility:

export function resolveTransition(
    transition: any,
    parentTransition?: any
)

Impact: Low (existing pattern in codebase)
Note: While this reduces bundle size (no complex generic constraints), it sacrifices type safety. Acceptable trade-off given repository priorities, but worth documenting.

4. Missing export validation

The PR exports resolveTransition from motion-dom/src/index.ts, but this is only used internally by framer-motion.

Question: Should this be a public API? If not, consider keeping it internal to reduce API surface.

5. Usability: Title vs. implementation mismatch

PR title says "Add transition.inherit" but the actual API is inherit: true as a key alongside other transition properties, not a nested transition.inherit object. This is clear in the description but the title could be adjusted to "Add inherit flag to transitions" for clarity.

---

🧪 Test Coverage Assessment

Excellent - All critical paths covered:

• ✅ Backward compatibility
• ✅ Basic inheritance
• ✅ Override behavior
• ✅ Deep nesting
• ✅ Key stripping
• ✅ Value-specific transitions with inherit

Potential addition: Integration test showing actual animation behavior with inherited transitions (current tests verify context propagation but not animation execution).

---

🔒 Security Considerations

No security concerns. The implementation:

• Doesn't execute user-provided code
• Only performs shallow object merging
• Properly validates existence with optional chaining

---

📊 Code Quality

Adherence to CLAUDE.md guidelines:

• ✅ Uses optional chaining (transition?.inherit)
• ✅ Uses arrow callbacks
• ✅ Uses strict equality
• ✅ No var declarations
• ✅ Named exports only
• ✅ Concise implementation prioritizing file size
• ✅ Comprehensive tests written first (TDD approach)

Minor style notes:

• Line 11 uses _ for unused variable (good practice)
• Consistent formatting throughout

---

🎯 Final Recommendation

Approve with minor suggestions ✅

This is a well-implemented feature that solves a real user need (issue #2271). The code is clean, well-tested, and follows repository conventions. The concerns raised are minor optimizations and documentation improvements, not blocking issues.

Suggested follow-ups (optional, can be separate PRs):

1. Add performance benchmark for high-frequency animations with inherit: true
2. Clarify whether resolveTransition should be public API
3. Consider adding integration test showing actual animation behavior

Great work! 🚀

[lochie]

yessss lets goooooo 🎉