fix: transitionEnd stuck with stale values after rapid variant switching

[mattgperry]

Summary

• Fixes #1668: transitionEnd values override the new variant's values when rapidly switching variants with instant transitions (type: false or duration: 0)
• Root cause: when all animations are instant (shouldSkip), Promise.all([]) resolves as a microtask, deferring the transitionEnd frame.update callback until after the new variant's callbacks — so the stale transitionEnd fires last and wins
• Fix: when animations array is empty, queue transitionEnd synchronously via frame.update instead of through Promise.all, keeping it ordered correctly in the same frame batch

Test plan

• Added regression test: "transitionEnd from instant animation does not override subsequent variant"
• Full variant test suite passes (37/37)
• All motion tests pass (229/229)
• Build succeeds with no type errors

🤖 Generated with Claude Code

[claude]

Pull Request Review: Fix transitionEnd Stuck with Stale Values

Summary

This PR addresses a race condition where transitionEnd values from instant animations (using type: false or duration: 0) could override subsequent variant values when rapidly switching variants. The fix introduces an InstantAnimation class that provides proper animation lifecycle management for instant transitions.

Code Quality ✅

Strengths

1. Root Cause Analysis: The PR correctly identifies the core issue - the shouldSkip path in animateMotionValue was scheduling callbacks via frame.update() but returning undefined, leaving no animation object to cancel and causing Promise.all([]) to resolve immediately.

2. Clean Architecture: The InstantAnimation class properly implements the AnimationPlaybackControls interface, ensuring it's a drop-in replacement for regular animations.

3. Excellent Test Coverage: The added regression test at packages/framer-motion/src/motion/__tests__/variant.test.tsx:1383-1438 is well-documented and specifically targets the race condition with clear comments explaining the issue.

4. Minimal Changes: The fix is surgical - only 3 files changed with the smallest possible surface area for the fix.

Potential Issues 🔍

1. Missing Unit Tests for InstantAnimation Class

The InstantAnimation class itself lacks dedicated unit tests. While the integration test is excellent, unit tests would verify edge cases:

// Suggested test cases:
// - stop() prevents onUpdate/onComplete from firing
// - complete() immediately fires callbacks and resolves promise
// - cancel() prevents callbacks (verify difference from stop())
// - multiple stop() calls are safe
// - promise rejection handling (onReject parameter)

Recommendation: Add unit tests at packages/motion-dom/src/animation/__tests__/InstantAnimation.test.ts

2. Race Condition in Constructor

There's a subtle timing issue in the InstantAnimation constructor:

constructor(
    private onUpdate: VoidFunction,
    private onComplete: VoidFunction
) {
    super()
    
    frame.update(() => {
        if (this._isStopped) return  // ✅ Good check
        onUpdate()
        this.notifyFinished()
        onComplete()
    })
}

If stop() or cancel() is called synchronously after construction but before frame.update() fires, the callbacks won't execute - which is correct behavior. However, the promise state should also be managed. Currently:

• stop() → sets _isStopped = true → callbacks don't fire → promise never resolves
• complete() → fires callbacks immediately → promise resolves

Issue: If stop() or cancel() is called before the frame callback fires, this.finished never resolves.

Recommendation: Either:

• Resolve the promise in stop()/cancel() (treat as completed)
• Reject the promise in cancel() (treat as aborted)
• Document this behavior if intentional

3. Inconsistent cancel() vs stop() Behavior

Both methods do the same thing (this._isStopped = true), but based on AnimationPlaybackControls interface comments:

• stop(): "prevents it from resuming" → should resolve promise
• cancel(): "applies the initial state" → might want to reject or handle differently

The current implementation doesn't distinguish between these semantics.

4. complete() Method Doesn't Check _isStopped

complete() {
    this.onUpdate()
    this.notifyFinished()
    this.onComplete()
}

If the animation is already stopped, calling complete() will still fire callbacks. This might be intentional, but consider:

complete() {
    if (this._isStopped) return
    this._isStopped = true
    this.onUpdate()
    this.notifyFinished()
    this.onComplete()
}

5. Memory Leak Risk (Minor)

If a component unmounts before the frame.update() callback fires, the callbacks might retain references to unmounted components. This is mitigated by the _isStopped check, but ensuring stop() is called during cleanup is critical.

Recommendation: Verify that value.stop() is consistently called during component unmount in the animation lifecycle.

Performance Considerations ⚡

1. Minimal Overhead: The InstantAnimation class is lightweight - only adds a boolean flag and inherits promise management from WithPromise.

2. Frame Scheduling: Using frame.update() aligns with the existing animation system's timing, so no additional performance concerns.

3. Promise Creation: Each instant animation creates a promise, but this is unavoidable for proper lifecycle management and is already happening in the codebase.

Security Concerns 🔒

No security concerns identified. This is pure animation lifecycle management with no external inputs or DOM manipulation.

Test Coverage 📊

What's Tested ✅

• Integration test covering the exact race condition from issue [BUG] Custom variant is stuck with stale values if the variant changes faster than the transition duration #1668
• Full variant test suite (37/37) passes
• All motion tests (229/229) pass

What's Missing ❌

1. Unit tests for InstantAnimation class:

   • Edge cases (stop before frame callback, multiple calls)
   • Promise lifecycle (resolve/reject behavior)
   • complete() behavior when already stopped

2. Error handling tests:

   • What happens if onUpdate or onComplete throw?
   • Should the error propagate or be caught?

Style & Convention Compliance 📋

✅ Uses interface for type definitions (line 7: implements AnimationPlaybackControls)
✅ Named exports only
✅ Arrow callbacks used appropriately
✅ No var declarations
✅ Consistent with existing code patterns (extends WithPromise like other animation classes)

Recommendations Summary

High Priority

1. Add unit tests for InstantAnimation class
2. Fix promise resolution in stop()/cancel() methods
3. Clarify/document difference between cancel() and stop()

Medium Priority

4. Add guard to complete() method to prevent double execution
5. Consider error handling for callbacks

Low Priority

6. Add JSDoc comments to InstantAnimation class explaining its purpose

Overall Assessment ⭐

Verdict: ✅ Approve with minor improvements

This is a solid fix that correctly addresses the root cause. The code is clean, well-tested at the integration level, and follows repository conventions. The main gaps are around edge case testing and promise lifecycle management, but these don't block merging since the fix solves the reported issue and all existing tests pass.

The implementation demonstrates a good understanding of the animation system's architecture and the fix is appropriately scoped.

---

Nice work on the thorough investigation and minimal fix! The PR description clearly explains the problem and solution, making it easy to review.

[claude]

Code Review

Summary

This PR fixes a critical race condition in instant animations where transitionEnd values could override subsequent variant values when variants are switched rapidly. The fix ensures all animations, including instant ones, go through the proper animation lifecycle with cancellation semantics.

---

✅ Strengths

1. Root Cause Analysis
The fix correctly identifies and addresses the core issue: the shouldSkip early return path bypassed creating an animation object, which meant:

• value.stop() had nothing to cancel
• Promise.all([]) resolved immediately
• Stale transitionEnd callbacks fired after variant switches

2. Excellent Test Coverage
The regression test (packages/framer-motion/src/motion/__tests__/variant.test.tsx:1383-1438) is well-designed:

• Clear documentation linking to issue [BUG] Custom variant is stuck with stale values if the variant changes faster than the transition duration #1668
• Reproduces the exact race condition (rapid variant switching with instant transitions)
• Tests the specific failure mode (transitionEnd overriding subsequent variant values)
• Uses appropriate double-render pattern to ensure timing consistency

3. Clean Solution
Removing the early-return optimization in favor of uniform animation lifecycle is architecturally sound:

• makeAnimationInstant() already sets duration: 0, so instant animations still complete immediately
• Proper cancellation semantics are maintained through AsyncMotionValueAnimation/JSAnimation
• Eliminates special-case code paths that can introduce bugs

4. Backwards Compatibility
The change maintains the same observable behavior for correctly-functioning code:

• Instant animations still execute with zero duration
• The timing behavior is preserved (duration: 0 already existed)
• No breaking API changes

---

🔍 Potential Concerns & Questions

1. Performance Impact ⚠️
The removed code path was an optimization for instant animations. Every instant animation now creates a full AsyncMotionValueAnimation or JSAnimation object instead of bypassing animation creation entirely.

Impact analysis:

• Instant animations are common (search shows 25+ uses of type: false in tests/examples)
• Each animation object creation involves:
  • Keyframe resolution
  • Generator instantiation
  • Promise creation
  • Driver setup (even if duration is 0)

Question: Have you benchmarked the performance difference? For high-frequency instant animations (like display: none/flex toggles), this could add overhead.

Recommendation: Consider adding a micro-benchmark or stress test for instant animations to quantify any performance regression. If measurable, document it as an acceptable tradeoff for correctness.

2. Test Coverage Gap
The test validates the fix but doesn't cover all edge cases:

Missing scenarios:

• Instant animation → non-instant animation transition
• Multiple rapid switches (on → off → on → off)
• transitionEnd with delays
• Instant animations with skipAnimations global config

Recommendation: Consider adding a parametrized test or additional test cases for these scenarios.

3. Code Deletion Confidence
The removed code (shouldSkip logic) included this block:

if (shouldSkip && \!isHandoff && value.get() \!== undefined) {
    const finalKeyframe = getFinalKeyframe(...)
    if (finalKeyframe \!== undefined) {
        frame.update(() => {
            options.onUpdate\!(finalKeyframe)
            options.onComplete\!()
        })
        return
    }
}

Question: Was the isHandoff check important? The comment in AsyncMotionValueAnimation.ts:162-163 mentions handoff animations need JS to run "under" WAAPI optimized animations. Does removing this optimization affect handoff behavior?

Recommendation: Verify that handoff animations still work correctly, especially optimized-appear scenarios.

4. Alternative Approach
Instead of removing the optimization entirely, could we:

1. Keep the fast path for instant animations
2. Store a reference to the pending frame.update callback
3. Cancel it when value.stop() is called

This would preserve performance while fixing the race condition.

Example:

let pendingInstantUpdate: (() => void) | undefined

value.stop = () => {
    if (pendingInstantUpdate) {
        frame.cancel(pendingInstantUpdate)
        pendingInstantUpdate = undefined
    }
    // ... normal stop logic
}

Question: Was this approach considered? If so, why was it rejected?

---

🐛 Potential Issues

5. Edge Case: Zero-Duration vs Type False
The code treats these equivalently:

if ((options as any).type === false || 
    (options.duration === 0 && \!options.repeatDelay)) {
    makeAnimationInstant(options)
}

But type: false and duration: 0 may have different semantic meanings in user code. The old optimization only applied when delay === 0 as well:

if (options.delay === 0) {
    shouldSkip = true  // old code
}

Question: Is there a behavior difference for { type: false, delay: 100 } animations? The old code wouldn't skip them, but now they all go through the same path.

---

📚 Documentation & Style

6. Code Style ✅

• Follows repository conventions (named exports, interfaces)
• Test uses nextFrame() helper correctly per CLAUDE.md guidelines
• Clear inline documentation in test

7. Missing Context
Recommendation: Add a comment in motion-value.ts explaining why instant animations must create animation objects:

// Instant animations must still create animation objects to ensure
// proper cancellation semantics. Without this, rapid variant switches
// can cause stale transitionEnd values to override new variants.
// See: https://github.com/motiondivision/motion/issues/1668

---

🧪 Testing Recommendations

8. Additional Testing
Run these commands to ensure no regressions:

# Full test suite
yarn test

# Playwright E2E tests (especially instant transition scenarios)
make test-e2e

# Verify no build errors
yarn build

From the PR description, these have passed ✅, which is excellent.

9. Performance Testing
Consider adding to the test plan:

# Run stress test with instant animations
# (if such a test exists in dev/html/public/benchmarks/)

---

🎯 Final Verdict

Overall: Strong approval with minor reservations

This is a well-researched fix for a real user-facing bug. The solution is architecturally clean and the test coverage demonstrates understanding of the race condition.

Approval contingent on:

1. ✅ Confirming no performance regression in high-frequency instant animation scenarios
2. ✅ Verifying handoff animations still work correctly
3. ⚠️ Consider adding inline documentation explaining why the optimization was removed

Minor improvements:

• Add comment explaining the fix
• Consider additional edge case tests
• Document any performance tradeoffs

The core fix is correct and should be merged after addressing the performance verification question.

---

💡 Positive Notes

• Excellent bug report linkage and test documentation
• Follows CLAUDE.md test-first approach
• Clean git history with descriptive commit messages
• No over-engineering or scope creep

Great work on tracking down this subtle race condition! 🎉

---

Rating: 8.5/10 - Solid fix with room for minor improvements in documentation and performance validation.

[claude]

Code Review

Summary

This PR fixes a critical race condition (issue #1668) where transitionEnd values override new variant values when rapidly switching variants with instant transitions. The fix is well-implemented, minimal, and surgically addresses the root cause.

---

✅ Strengths

1. Excellent Root Cause Analysis
The PR description clearly identifies the microtask timing issue:

• When all animations are instant (shouldSkip), Promise.all([]) resolves as a microtask
• This defers the transitionEnd callback until after the new variant's callbacks are queued
• The stale transitionEnd fires last and wins

2. Minimal, Focused Fix
The change in visual-element-target.ts:134-145 is elegantly simple:

• Extracts applyTransitionEnd as a closure to avoid duplication
• Synchronously calls applyTransitionEnd() when animations.length === 0
• Maintains existing Promise.all() behavior when animations exist
• No changes to the public API or behavior contracts

3. Comprehensive Test Coverage
The test in variant.test.tsx:1383-1437 is exemplary:

• Reproduces the exact race condition scenario
• Uses type: false to trigger instant animations
• Tests rapid variant switching (on → off before frame fires)
• Verifies the final state is correct (display: none)
• Includes excellent inline documentation explaining the race condition

4. Follows Repository Conventions

• Uses frame.update() correctly per CLAUDE.md timing guidance
• Named exports only
• Proper TypeScript usage
• Test uses nextFrame() helper as recommended

---

🔍 Code Quality Analysis

visual-element-target.ts (lines 134-145)

if (transitionEnd) {
    const applyTransitionEnd = () =>
        frame.update(() => {
            transitionEnd && setTarget(visualElement, transitionEnd)
        })

    if (animations.length) {
        Promise.all(animations).then(applyTransitionEnd)
    } else {
        applyTransitionEnd()
    }
}

Observations:

• ✅ The closure pattern is clean and avoids duplication
• ✅ The conditional logic is clear and easy to understand
• ⚠️ Minor note: The inner transitionEnd && check is now redundant since it's already within the outer if (transitionEnd) block

Potential micro-optimization (optional, not blocking):

if (transitionEnd) {
    const applyTransitionEnd = () =>
        frame.update(() => setTarget(visualElement, transitionEnd))

    if (animations.length) {
        Promise.all(animations).then(applyTransitionEnd)
    } else {
        applyTransitionEnd()
    }
}

---

🎯 Test Quality

The new test is production-grade:

• Directly targets the race condition from issue [BUG] Custom variant is stuck with stale values if the variant changes faster than the transition duration #1668
• Clear documentation of what's being tested
• Uses multiple rerender() calls to ensure variant is applied
• Uses await nextFrame() to allow deferred callbacks to fire
• Verifies the final DOM state, not just internal state

Test structure follows best practices:

1. Setup component with variants
2. Render to initial state
3. Trigger rapid variant changes
4. Wait for frame updates
5. Assert final state

---

🔒 Security & Performance

No Security Concerns:

• No user input handling
• No external data
• Internal animation timing logic only

Performance Impact: Positive

• Removes unnecessary microtask scheduling for instant animations
• Reduces event loop overhead when animations.length === 0
• No additional allocations or closures compared to before

---

🧪 Test Coverage

Verified Coverage:

• ✅ New regression test for the exact issue
• ✅ PR description confirms all existing tests pass (37/37 variant tests, 229/229 total)
• ✅ Build succeeds with no type errors

Edge Cases Covered:

• Instant transitions (type: false)
• Rapid variant switching
• transitionEnd with display property changes
• Multiple rerender() calls (idempotency)

---

📝 Suggestions

1. Documentation (Optional)
Consider adding a code comment explaining the timing behavior:

if (transitionEnd) {
    const applyTransitionEnd = () =>
        frame.update(() => setTarget(visualElement, transitionEnd))

    if (animations.length) {
        // Defer transitionEnd until all animations complete
        Promise.all(animations).then(applyTransitionEnd)
    } else {
        // No animations means instant transition - apply synchronously
        // to avoid microtask timing issues with rapid variant changes
        applyTransitionEnd()
    }
}

2. Test Refinement (Optional)
The test could be slightly more concise by removing duplicate rerender() calls (lines 1424-1425, 1430-1431), though these may be intentional for stress-testing.

---

✅ Final Verdict

This PR is ready to merge. The fix is:

• ✅ Correct and minimal
• ✅ Well-tested with a comprehensive regression test
• ✅ Performance-neutral or slightly positive
• ✅ No breaking changes
• ✅ Follows repository conventions
• ✅ Excellent documentation in PR description and test

The only suggestions above are minor polish items that don't block merging.

---

Recommendation: APPROVE ✅

Great work identifying and fixing this subtle race condition. The microtask timing issue would have been very difficult to debug without the clear analysis provided.

---

Review generated by Claude Code using repository conventions from CLAUDE.md