Add sequence callback support for animation timelines

[mattgperry]

Summary

This PR adds support for callbacks in animation sequences, allowing developers to execute code at specific points during a sequence timeline. Callbacks can be positioned at absolute times, relative offsets, or referenced by labels, and support both forward playback (onEnter) and backward scrubbing (onLeave).

Key Changes

• New SequenceCallbackAnimation class (motion-dom): A lightweight animation implementation that fires callbacks when the timeline crosses specific time points. It handles both forward playback and backward scrubbing scenarios.

• Callback segment support: Extended AnimationSequence type to support callback segments as [{ onEnter?, onLeave? }, { at? }] tuples, allowing callbacks to be interspersed with animations.

• Enhanced createAnimationsFromSequence:

  • Now returns a CreateAnimationsResult object containing animationDefinitions, callbacks, and totalDuration
  • Extracts and resolves callback timing using the same at positioning logic as animations (absolute, relative +/-, < for previous, or label references)
  • Sorts callbacks by time for efficient lookup during playback

• Integration with animation sequences: Updated both animateSequence functions (in framer-motion and waapi) to instantiate and include SequenceCallbackAnimation in the animation group when callbacks are present.

• Comprehensive test coverage: Added 10+ tests covering callback extraction, timing resolution, multiple callbacks, label-based positioning, and interaction with animation timing.

Implementation Details

• Callbacks are extracted during sequence parsing and don't affect animation timing or duration calculations
• The SequenceCallbackAnimation uses simple time comparison logic with minimal overhead
• Callbacks fire only when time crosses their threshold (prevents duplicate fires on repeated time sets)
• Backward scrubbing triggers onLeave callbacks, enabling interactive timeline scrubbing scenarios
• All existing tests updated to use a helper function that maintains backward compatibility with the new return structure

https://claude.ai/code/session_01Es5grCjnfwALQxsWvrrpzF

[claude]

Code Review

I've reviewed PR #3526 which adds sequence callback support for animation timelines. Overall, this is a well-implemented feature with strong test coverage. Here's my detailed feedback:

---

✅ Strengths

1. Excellent Architecture

• Clean separation of concerns with SequenceCallbackAnimation as a standalone class
• Minimal overhead implementation - no DOM operations, just simple time comparisons
• Proper integration with existing AnimationPlaybackControls interface
• The callback extraction logic in createAnimationsFromSequence is well-structured

2. Comprehensive Test Coverage

• 10+ test cases covering edge cases (time 0, end of animation, empty arrays, etc.)
• Tests for both forward playback and backward scrubbing
• Good integration tests in sequence/__tests__/index.test.ts
• The helper function getAnimationDefinitions for backward compatibility is a nice touch

3. Good Documentation

• Clear comments explaining callback behavior
• Type definitions include JSDoc explanations
• PR description is thorough with implementation details

---

⚠️ Issues & Concerns

1. Type Safety Issue (High Priority)

Location: packages/framer-motion/src/animation/animate/sequence.ts:40 and packages/framer-motion/src/animation/animators/waapi/animate-sequence.ts:81

animations.push(
    callbackAnimation as unknown as AnimationPlaybackControlsWithThen
)

Problem: The double type assertion (as unknown as) is a code smell that bypasses TypeScript's type checking. This suggests the types don't properly align.

Root Cause: SequenceCallbackAnimation implements AnimationPlaybackControls but is being cast to AnimationPlaybackControlsWithThen, which likely includes a .then() method for promise chaining that SequenceCallbackAnimation doesn't have.

Impact: Runtime errors could occur if code tries to call .then() on the callback animation.

Recommendation:

• Either add .then() support to SequenceCallbackAnimation (preferred)
• Or create a proper type union that allows both types
• Check how other animation types handle the then method - likely need to return this to support chaining

2. Edge Case: Large Time Jumps (Medium Priority)

Location: packages/motion-dom/src/animation/SequenceCallbackAnimation.ts:73-93

Problem: When time is set with large jumps (e.g., from 0 to 1 second), the code fires ALL callbacks in between sequentially in a single frame. For sequences with many callbacks (say, 100+), this could cause:

• Frame drops
• Long synchronous execution blocking the main thread
• Potential memory issues if callbacks create objects

Test Gap: No tests verify behavior with many callbacks (e.g., 50-100 callbacks)

Recommendation:

• Add a test with a large number of callbacks to verify performance
• Consider documenting this behavior or adding a warning if callback count is high
• For production use with many callbacks, consider batching or async execution

3. Memory Leak Potential (Medium Priority)

Location: packages/motion-dom/src/animation/SequenceCallbackAnimation.ts:37-40

this.finished = new Promise((resolve, reject) => {
    this.resolveFinished = resolve
    this.rejectFinished = reject
})

Problem: If cancel() is called, the promise is rejected (line 109), but if the animation is never completed or cancelled, the promise hangs indefinitely with callbacks stored in memory.

Test Gap: No test verifies memory cleanup or that finished promise is properly handled

Recommendation:

• Add a test that verifies calling cancel() properly rejects the promise
• Consider adding cleanup logic in stop() as well (currently it just sets state to idle)
• Document the lifecycle expectations

4. Callback Detection Logic (Low-Medium Priority)

Location: packages/framer-motion/src/animation/sequence/create.ts:692-704

function isCallbackSegment(segment: unknown): segment is [SequenceCallback, At] {
    if (\!Array.isArray(segment) || segment.length \!== 2) return false
    const [callback, options] = segment
    if (typeof callback \!== 'object' || callback === null) return false
    return (
        ('onEnter' in callback || 'onLeave' in callback) &&
        \!('duration' in options) &&
        \!('ease' in options)
    )
}

Problem: This detection heuristic could have false positives/negatives:

• What if someone accidentally passes { onEnter: true } (not a function)?
• What if future animation options are added that aren't checked here?
• The check \!('duration' in options) might not be sufficient to distinguish callbacks from animations

Recommendation:

• Add runtime type validation: check if onEnter/onLeave are actually functions
• Consider a more explicit marker (e.g., { type: 'callback', onEnter: ... })
• Add tests for malformed callback segments

5. Incomplete finished Promise Handling (Low Priority)

Location: packages/motion-dom/src/animation/SequenceCallbackAnimation.ts:30

Problem: The finished promise type in the class doesn't match the interface. AnimationPlaybackControls likely expects Promise<unknown> or similar, but this returns Promise<void>.

Also: The finished promise is only resolved in complete(), but GroupAnimation expects all animations' finished promises to resolve. If someone calls stop() or pause(), the promise never resolves.

Recommendation:

• Check what other animation implementations do (likely resolve on stop as well)
• Ensure promise resolves in all terminal states

---

🔍 Minor Issues

6. Code Style Consistency

• Line 879 (SequenceCallbackAnimation.ts:125): The type cast could be cleaner

  return timeline.observe(this as unknown as Parameters<typeof timeline.observe>[0])

  This is overly complex - consider defining a proper type

7. Missing Null Check

• Line 114 (SequenceCallbackAnimation.ts): complete() calls fireCallbacks() but doesn't verify callbacks exist (though fireCallbacks does check)

8. Test Quality

• Test at line 69 (SequenceCallbackAnimation.test.ts): Uses calls.length = 0 to clear array. While valid, calls.splice(0) is more idiomatic

---

🔒 Security Considerations

No security issues identified. User-provided callbacks are executed as expected, which is the intended behavior. The implementation doesn't introduce XSS, injection, or other vulnerabilities.

---

🚀 Performance Considerations

Positive:

• Minimal overhead (just time comparisons)
• Callbacks sorted once at construction
• No DOM operations or heavy computations

Potential Issue:

• See "Large Time Jumps" concern above about many callbacks firing synchronously

---

📋 Recommendations Priority

1. HIGH: Fix the type casting issues (as unknown as)
2. MEDIUM: Add tests for edge cases (many callbacks, memory cleanup)
3. MEDIUM: Clarify finished promise behavior across all terminal states
4. LOW: Improve callback detection robustness
5. LOW: Clean up minor style issues

---

✨ Overall Assessment

This is a solid implementation with good separation of concerns and strong test coverage. The main issues are around type safety and some edge case handling. With the recommended fixes, this would be production-ready.

The feature design is sound - the bidirectional callback system (onEnter/onLeave) is well thought out for scrubbing scenarios.

Recommendation: Request changes for the type safety issues, then approve after fixes.

[claude]

PR Review: Add sequence callback support for animation timelines

I've thoroughly reviewed this PR. Overall, this is a well-implemented feature with solid test coverage and clear design decisions. Here's my detailed feedback:

---

✅ Strengths

1. Clean API Design

• The callback API is intuitive: [{ forward?, backward? }, { at? }]
• Supports all existing timing mechanisms (at, "+/-", "<", labels)
• Forward/backward distinction enables interactive scrubbing scenarios

2. Excellent Test Coverage

• 10+ comprehensive tests covering all timing scenarios
• Tests verify callbacks don't affect animation timing/duration
• Good use of helper function for backward compatibility

3. Minimal Performance Overhead

• Callbacks are sorted once during parsing for O(n) lookup
• Simple time comparison logic with no complex state management
• Only creates tracking animation when callbacks are present

4. Architecture

• Correctly returns structured CreateAnimationsResult object
• Both animateSequence implementations properly updated
• Type definitions are clear and well-documented

---

🔍 Issues Found

Critical: Incorrect Callback Timing Logic

Location: packages/framer-motion/src/animation/animate/sequence.ts:29-38 and packages/framer-motion/src/animation/animators/waapi/animate-sequence.ts:25-34

The current implementation recalculates prevTime inside the callback loop, which is incorrect:

for (const callback of callbacks) {
    const prevTime = prevProgress * totalDuration  // ❌ WRONG: recalculated each iteration
    
    if (prevTime < callback.time && currentTime >= callback.time) {
        callback.forward?.()
    } else if (prevTime >= callback.time && currentTime < callback.time) {
        callback.backward?.()
    }
}

Problem: When multiple callbacks exist at different times, prevTime should be constant across all callbacks in a single onUpdate call. The current code recalculates it for each callback, which is wasteful and could lead to confusion.

Fix: Move prevTime calculation outside the loop:

return (progress: number) => {
    const currentTime = progress * totalDuration
    const prevTime = prevProgress * totalDuration  // ✅ Calculate once
    
    for (const callback of callbacks) {
        if (prevTime < callback.time && currentTime >= callback.time) {
            callback.forward?.()
        } else if (prevTime >= callback.time && currentTime < callback.time) {
            callback.backward?.()
        }
    }
    
    prevProgress = progress
}

This affects both files (lines 29-38 in sequence.ts and lines 25-34 in animate-sequence.ts).

---

⚠️ Potential Issues

1. Edge Case: Callbacks at Exact Boundaries

The current logic uses < and >=, which means:

• A callback at time 0 fires when currentTime >= 0 (will fire on first frame if starting from 0)
• A callback at totalDuration fires when crossing into that time

Question: Is this the intended behavior? Consider:

• Should callbacks at time 0 fire immediately on animation start?
• What happens if the animation starts at a non-zero time (e.g., after seeking)?

Recommendation: Add tests for these edge cases or document the expected behavior.

2. Missing Integration Tests

While unit tests are excellent, I don't see E2E or integration tests that verify:

• Callbacks actually fire during real animations
• Backward scrubbing works with playback controls (.seek(), .reverse())
• Multiple callbacks fire in correct order during a single frame jump
• Callbacks work with repeat, yoyo, or other playback options

Recommendation: Add at least one integration test demonstrating the feature working end-to-end.

3. Type Safety in isCallbackSegment

Location: packages/framer-motion/src/animation/sequence/create.ts:465-477

The type guard checks:

return (
    ("forward" in callback || "backward" in callback) &&
    !("duration" in options) &&
    !("ease" in options)
)

Issue: This could false-positive on objects that accidentally have forward/backward properties. Also, why check for duration and ease in options instead of callback?

Suggestion: Add a runtime check that forward and backward are functions:

return (
    (("forward" in callback && typeof callback.forward === "function") ||
     ("backward" in callback && typeof callback.backward === "function")) &&
    !("duration" in options) &&
    !("ease" in options)
)

4. Duplicate Code

The createCallbackUpdater function is identical in both files:

• packages/framer-motion/src/animation/animate/sequence.ts:19-42
• packages/framer-motion/src/animation/animators/waapi/animate-sequence.ts:17-38

Recommendation: Extract to a shared utility function to follow DRY principle.

5. Performance: Unnecessary Iteration

The callback loop runs through all callbacks on every frame, even those that won't fire. For sequences with many callbacks, consider optimizing:

// Track the index of the next callback to check
let nextCallbackIndex = 0

return (progress: number) => {
    const currentTime = progress * totalDuration
    const prevTime = prevProgress * totalDuration
    
    if (currentTime > prevTime) {
        // Forward playback
        while (nextCallbackIndex < callbacks.length && 
               callbacks[nextCallbackIndex].time <= currentTime &&
               callbacks[nextCallbackIndex].time > prevTime) {
            callbacks[nextCallbackIndex].forward?.()
            nextCallbackIndex++
        }
    } else if (currentTime < prevTime) {
        // Backward playback
        while (nextCallbackIndex > 0 && 
               callbacks[nextCallbackIndex - 1].time < prevTime &&
               callbacks[nextCallbackIndex - 1].time >= currentTime) {
            nextCallbackIndex--
            callbacks[nextCallbackIndex].backward?.()
        }
    }
    
    prevProgress = progress
}

However, this may be premature optimization unless you expect sequences with 100+ callbacks.

---

🎨 Style & Documentation

1. Naming: forward vs onEnter

The PR description mentions onEnter and onLeave, but the implementation uses forward and backward. Consider if the original names were more intuitive:

• onEnter / onLeave - suggests entering/leaving a time region (clearer for users)
• forward / backward - suggests playback direction (more technical)

Not a bug, but worth considering for API clarity.

2. Documentation

Add JSDoc to the public-facing SequenceCallback type with examples:

/**
 * Callback to be invoked at a specific point in the sequence.
 * 
 * @example
 * ```ts
 * animate([
 *   [element, { opacity: 1 }, { duration: 1 }],
 *   [{ forward: () => console.log('halfway!') }, { at: 0.5 }],
 *   [element, { x: 100 }, { duration: 1 }]
 * ])
 * ```
 * 
 * @property forward - Called when timeline crosses this point moving forward
 * @property backward - Called when timeline crosses this point moving backward (e.g., during scrubbing)
 */
export interface SequenceCallback {
    forward?: VoidFunction
    backward?: VoidFunction
}

---

🧪 Testing Recommendations

1. Add integration test:

   test("Callbacks fire during actual animation playback", async () => {
       const calls: string[] = []
       const element = document.createElement("div")
       
       const controls = animateSequence([
           [element, { x: 100 }, { duration: 1 }],
           [{ forward: () => calls.push('mid') }, { at: 0.5 }],
           [element, { y: 100 }, { duration: 1 }]
       ])
       
       await controls.then(() => {
           expect(calls).toEqual(['mid'])
       })
   })

2. Add backward playback test:
   Test with .seek() or .reverse() to verify backward callbacks work.

3. Add boundary test:
   Verify behavior when callbacks are at time 0 or totalDuration.

---

🎯 Summary

Overall Assessment: This is a solid implementation with good design decisions. The critical issue with timing calculation should be fixed before merging, and I recommend adding integration tests. The other issues are minor improvements.

Recommendation: ✅ Approve after addressing the critical timing bug

Required Changes:

• ✅ Fix prevTime calculation (move outside loop in both files)

Strongly Recommended:

• ⚠️ Add integration/E2E tests
• ⚠️ Add edge case tests (callbacks at 0, totalDuration)

Optional Improvements:

• 💡 Extract createCallbackUpdater to shared utility
• 💡 Improve type guard in isCallbackSegment
• 💡 Add JSDoc with examples
• 💡 Consider performance optimization for many callbacks

Great work on this feature! The API design is clean and the test coverage is thorough. 🚀

[claude]

PR Review: Add sequence callback support for animation timelines

Summary

This PR adds callback support to animation sequences, allowing developers to execute code at specific timeline points. The implementation is clean and well-tested with comprehensive coverage.

✅ Code Quality & Best Practices

Strengths:

• Follows repository conventions (interfaces over types, named exports, arrow callbacks)
• Clean separation of concerns with the CreateAnimationsResult interface
• Excellent test coverage with 10+ new tests covering edge cases
• Good TypeScript typing with clear interfaces (SequenceCallback, ResolvedSequenceCallback)
• Helpful inline documentation explaining the callback timing logic

Minor suggestions:

1. Type guard robustness (create.ts:465-477): The isCallbackSegment function could be more defensive:

   // Current check might miss some edge cases
   return (
       ("forward" in callback || "backward" in callback) &&
       \!("duration" in options) &&
       \!("ease" in options)
   )

   Consider also checking that options is an object to prevent runtime errors if someone passes non-object values.

2. Callback timing precision (sequence.ts:31-37): The comparison uses >= and < which means callbacks fire when time equals or exceeds the threshold. Document whether callbacks at exactly time=0 fire on initialization, as this could be unexpected behavior.

🐛 Potential Issues

1. Edge case: Multiple callbacks at the same time
The current implementation fires all callbacks at the same time point in the order they appear in the sorted array. This is likely fine, but worth documenting the execution order guarantee.

2. Callback execution during seeks (sequence.ts:25-40)
When jumping from time 0 to time 5 (e.g., .currentTime = 5), all callbacks between 0 and 5 will fire. This is probably desired, but could cause performance issues with many callbacks. Consider documenting this behavior.

3. Memory leak potential
The createCallbackUpdater creates a closure that retains callbacks and totalDuration. If animations are created/destroyed frequently, ensure the callback animation is properly cleaned up. The existing animation cleanup mechanisms should handle this, but verify in integration tests.

4. WAAPI implementation (waapi/animate-sequence.ts:12-15)
The WAAPI version correctly ignores callbacks (as noted in commit message), but this creates an inconsistency where the same API behaves differently depending on the animation backend. Consider:

• Adding a console warning when callbacks are used with WAAPI backend
• Or documenting this limitation clearly in TypeScript types/JSDoc

⚡ Performance Considerations

Excellent:

• Callbacks are sorted once during setup, not on every update
• Uses simple threshold crossing logic with minimal overhead
• The 0→1 animation approach is clever and reuses existing infrastructure

Suggestion:
The createCallbackUpdater iterates over ALL callbacks on every frame (sequence.ts:28). With many callbacks, consider optimizing:

// Early exit when we've passed all callbacks in forward direction
if (currentTime >= callbacks[callbacks.length - 1].time && 
    prevTime >= callbacks[callbacks.length - 1].time) {
    prevProgress = progress
    return
}

🔒 Security Concerns

No security issues identified. Callbacks are user-provided functions executed in the user's context, which is expected behavior.

🧪 Test Coverage

Excellent coverage with tests for:

• Default timing, explicit at timing, relative timing (+/-), < syntax
• Multiple callbacks, label-based positioning
• Forward/backward callback firing
• Callbacks don't affect animation timing

Missing test cases:

1. Callbacks at exactly time 0 - does forward fire immediately?
2. Rapid direction changes (forward → backward → forward)
3. Multiple callbacks at the exact same time
4. What happens with callbacks when animation is cancelled/stopped mid-sequence?
5. Callback exceptions - if a callback throws, does it stop the animation?

📝 Documentation

The PR description is excellent. Consider adding:

1. JSDoc examples in types.ts for SequenceCallback
2. Note about WAAPI limitation
3. Behavior documentation for edge cases mentioned above

Code-Specific Comments

sequence.ts:59-66 - Consider adding a comment explaining why linear ease is used:

// Use linear ease to ensure onUpdate receives uniform progress values
// for accurate time threshold detection

create.ts:420 - Nice that callbacks are sorted! Consider adding a comment explaining why:

// Sort callbacks by time for deterministic execution order
// and potential future optimization (early exit)

Conclusion

This is a solid implementation that follows the codebase's high standards. The feature is well-designed, properly tested, and adds valuable functionality. The main suggestions are around edge case documentation and minor optimizations.

Recommendation: Approve with minor suggestions ✅

The identified issues are low-severity and mostly around documentation/edge cases rather than fundamental problems. Nice work!

[claude]

Code Review: Sequence Callback Support

Thank you for this PR! The implementation of callback support for animation sequences is well-designed and thoughtfully executed.

---

Strengths

1. Clean Architecture

• The CreateAnimationsResult interface provides a clear contract for the enhanced return type
• Callback extraction is cleanly separated from animation processing in create.ts
• The isCallbackSegment type guard properly distinguishes callbacks from animations

2. Comprehensive Test Coverage

• 10+ new test cases covering timing resolution, multiple callbacks, label positioning
• Tests verify that callbacks do not affect animation timing (critical!)
• Good coverage of edge cases like previous and relative timing

3. Smart Timing Logic

• The createCallbackUpdater correctly tracks direction for forward/backward scrubbing
• Callbacks are sorted by time for efficient lookup
• Uses the same calcNextTime logic as animations for consistent timing resolution

---

Potential Issues

1. Critical: Callback Firing on Repeated Time Sets (HIGH PRIORITY)

Location: packages/framer-motion/src/animation/animate/sequence.ts:31-37

Problem: The implementation does not explicitly track which callbacks have already fired. If progress is set externally via scrubbing, the same callback could fire multiple times during forward playback if the progress crosses the threshold multiple times.

Recommendation: Consider tracking which callbacks have fired in each direction using a Set.

2. WAAPI Implementation Missing Callback Support (HIGH PRIORITY)

Location: packages/framer-motion/src/animation/animators/waapi/animate-sequence.ts

The WAAPI version only extracts animationDefinitions but completely ignores callbacks. This creates an inconsistent API where callbacks work in some contexts but silently fail in others.

Recommendation: Either add callback support to WAAPI, log a warning when callbacks are present but ignored, or document this limitation clearly.

3. Type Safety Issue in isCallbackSegment

Location: packages/framer-motion/src/animation/sequence/create.ts:465-477

The heuristic could incorrectly identify animation segments as callbacks if someone passes forward: 100 trying to animate a forward property. The check for duration/ease helps but is not foolproof.

Recommendation: Make callbacks more explicit with a unique property, class constructor, or Symbol key for type discrimination.

4. Performance: Linear Search on Every Update

Location: packages/framer-motion/src/animation/animate/sequence.ts:28

On every animation frame, this loops through ALL callbacks even if none are near the current time. For sequences with many callbacks this could impact performance.

Recommendation: Implement binary search or maintain a pointer to the next callback to only check callbacks near the current position.

5. Edge Case: Callbacks at Time 0

If a callback is positioned at time 0, it might fire immediately when the animation starts. This might be unexpected behavior.

Recommendation: Add a test case and clarify documentation on whether time-0 callbacks fire on animation start.

---

Code Style and Best Practices

Minor Issues:

1. Line 92-104 in create.ts: The callback extraction logic could be extracted into a helper function
2. Naming: forward and backward could be more descriptive. Consider onEnter and onLeave as mentioned in PR description
3. Documentation: Add JSDoc comments to createCallbackUpdater explaining the algorithm and edge cases

---

Test Coverage Assessment

Excellent coverage of timing resolution, but missing:

• Edge case: callback at time 0
• Edge case: callback at end of sequence
• Integration test: verify callbacks fire during actual animation playback not just extraction
• Edge case: what happens if forward throws an error
• Backward scrubbing behavior only extraction is tested not actual firing

---

Recommendations Summary

Must Fix before merge:

1. Fix or document WAAPI callback support
2. Improve type discrimination for callbacks

Should Fix:
3. Consider callback tracking to prevent duplicate fires
4. Add missing edge case tests

Nice to Have:
5. Optimize callback lookup for large sequences
6. Extract callback logic into helper function
7. Improve naming

---

Overall Assessment

This is a high-quality PR with solid implementation and thorough testing. The main concern is the missing WAAPI support and the type discrimination heuristic. With those addressed, this feature will be a valuable addition to the animation API.

Score: 8/10 - Would be 9.5/10 after addressing the WAAPI issue and type safety concern.

Let me know if you would like me to provide code snippets for any of the suggested fixes!

[claude]

Code Review Summary

Thank you for this well-structured PR! The implementation of sequence callbacks is clean and follows good practices. Below is my detailed feedback:

---

✅ Strengths

Code Quality

• Clean architecture: The separation between callback extraction (create.ts) and execution (sequence.ts) is well-designed
• Type safety: Proper TypeScript interfaces with clear documentation (SequenceCallback, ResolvedSequenceCallback)
• Consistent patterns: The isCallbackSegment type guard follows the same pattern as other segment detection in the codebase
• Good documentation: Comments clearly explain the forward/backward callback behavior

Implementation Details

• Minimal overhead: The createCallbackUpdater uses simple time comparison logic that's efficient
• Proper state tracking: Using prevProgress to detect direction is a clean approach
• Correct integration: Callbacks are sorted by time and don't interfere with animation timing calculations

---

🔍 Issues & Concerns

1. CRITICAL: WAAPI Implementation Missing Callback Support

The WAAPI version of animateSequence (in packages/framer-motion/src/animation/animators/waapi/animate-sequence.ts) was not updated to support callbacks. This creates an inconsistency:

// waapi/animate-sequence.ts - Line 12
createAnimationsFromSequence(definition, options).forEach(
    ({ keyframes, transition }, element: Element) => {
        animations.push(...animateElements(element, keyframes, transition))
    }
)
// ❌ Missing: No callbacks array, no callback animation added

Impact: Callbacks will silently not work when using WAAPI animations, leading to confusing behavior differences between animation backends.

Recommendation: Update the WAAPI implementation to match the main implementation, or document why callbacks are not supported in WAAPI mode.

---

2. Edge Case: Empty Sequence with Only Callbacks

// packages/framer-motion/src/animation/animate/sequence.ts:67-71
const firstTransition = animationDefinitions.values().next().value?.transition
const totalDuration = firstTransition
    ? (Object.values(firstTransition)[0] as any)?.duration ?? 0
    : 0

If a sequence contains only callbacks and no animations, animationDefinitions will be empty, resulting in totalDuration = 0. This means:

• All callbacks will fire immediately at time 0
• The callback animation will have 0 duration

Test case that would expose this:

const sequence = [
    [{ forward: () => console.log('1') }, { at: 0 }],
    [{ forward: () => console.log('2') }, { at: 1 }],
]
// Expected: callbacks fire at 0s and 1s
// Actual: both fire immediately at 0s

Recommendation: Either:

• Calculate duration from callbacks if no animations exist: Math.max(...callbacks.map(c => c.time))
• Throw an error or warning for callback-only sequences
• Document this as unsupported behavior

---

3. Type Safety: Unsafe any Cast

// Line 70
(Object.values(firstTransition)[0] as any)?.duration ?? 0

The as any cast bypasses type checking. While the code likely works, this could mask type errors.

Recommendation: Use proper type assertion:

const firstTransitionValue = Object.values(firstTransition)[0] as Transition | undefined
const totalDuration = firstTransitionValue?.duration ?? 0

---

4. Potential Bug: Callback Firing Logic

// packages/framer-motion/src/animation/animate/sequence.ts:31-34
if (prevTime < callback.time && currentTime >= callback.time) {
    callback.forward?.()
} else if (prevTime >= callback.time && currentTime < callback.time) {
    callback.backward?.()
}

Issue with boundary conditions:

• If currentTime === callback.time and prevTime === callback.time, neither condition fires
• On first update when prevProgress = 0, if a callback is at time 0, it won't fire (since prevTime = 0, currentTime >= 0, but condition requires prevTime < 0)

Recommendation: Consider using <= for the first condition to handle exact matches:

if (prevTime <= callback.time && currentTime > callback.time) {
    // ... or use strict inequality with initialization
}

Or initialize prevProgress = -Infinity to ensure callbacks at time 0 fire on first frame.

---

5. Test Coverage Gaps

The tests added are good but missing critical scenarios:

Missing test cases:

• ✅ Testing that callbacks don't affect timing (present)
• ✅ Testing callback segments are skipped (present)
• ❌ Testing that callbacks actually fire with correct timing
• ❌ Testing backward/scrubbing behavior (onLeave callbacks)
• ❌ Testing callbacks at time 0
• ❌ Testing callbacks with label-based positioning
• ❌ Testing multiple callbacks at the same time
• ❌ Testing integration with animateSequence (end-to-end)

Per CLAUDE.md: "IMPORTANT: Always write a failing test FIRST before implementing any bug fix or feature."

Recommendation: Add integration tests that verify callbacks fire at the correct times during playback. Example:

test('Callbacks fire at correct times during playback', async () => {
    const calls: number[] = []
    const controls = animateSequence([
        [a, { x: 100 }, { duration: 1 }],
        [{ forward: () => calls.push(1) }, { at: 0.5 }],
        [{ forward: () => calls.push(2) }, { at: 1.0 }],
    ])
    
    await controls[0].then(() => {
        expect(calls).toEqual([1, 2])
    })
})

---

6. isCallbackSegment Type Guard Logic Issue

// packages/framer-motion/src/animation/sequence/create.ts:467-479
function isCallbackSegment(segment: unknown): segment is [SequenceCallback, At] {
    if (!Array.isArray(segment) || segment.length !== 2) return false
    const [callback, options] = segment
    if (typeof callback !== 'object' || callback === null) return false
    return (
        ('forward' in callback || 'backward' in callback) &&
        !('duration' in options) &&
        !('ease' in options)
    )
}

Issues:

1. False positives: An object like [{ forward: fn, x: 100 }, { at: 0 }] would pass this check but is not a valid callback (has animation property x)
2. Fragile detection: Relies on absence of duration/ease rather than presence of expected properties
3. Interface At: Only checks for duration and ease, but At interface only has at? property. An object with duration is actually a full transition object

Better approach:

function isCallbackSegment(segment: unknown): segment is [SequenceCallback, At] {
    if (!Array.isArray(segment) || segment.length !== 2) return false
    const [callback, options] = segment
    if (typeof callback !== 'object' || callback === null) return false
    
    // Must have at least forward or backward
    const hasCallbackProps = 'forward' in callback || 'backward' in callback
    if (!hasCallbackProps) return false
    
    // Should not have animation keyframe properties (x, y, opacity, etc.)
    const callbackKeys = Object.keys(callback)
    const hasOnlyCallbackKeys = callbackKeys.every(k => k === 'forward' || k === 'backward')
    
    return hasOnlyCallbackKeys
}

---

7. Performance: Iterating All Callbacks on Every Frame

// sequence.ts:28-36
for (const callback of callbacks) {
    const prevTime = prevProgress * totalDuration
    
    if (prevTime < callback.time && currentTime >= callback.time) {
        callback.forward?.()
    } else if (prevTime >= callback.time && currentTime < callback.time) {
        callback.backward?.()
    }
}

Issue: On every frame update, this loops through all callbacks (even those already fired or not yet reached).

Impact: For sequences with many callbacks (e.g., 100+), this could impact performance, especially on mobile devices.

Optimization suggestion: Track the current callback index and only check nearby callbacks:

let currentIndex = 0

return (progress: number) => {
    const currentTime = progress * totalDuration
    const prevTime = prevProgress * totalDuration
    const movingForward = currentTime > prevTime
    
    if (movingForward) {
        // Fire all callbacks between prevTime and currentTime
        while (currentIndex < callbacks.length && callbacks[currentIndex].time <= currentTime) {
            if (callbacks[currentIndex].time > prevTime) {
                callbacks[currentIndex].forward?.()
            }
            currentIndex++
        }
    } else {
        // Handle backward scrubbing
        while (currentIndex > 0 && callbacks[currentIndex - 1].time >= currentTime) {
            currentIndex--
            if (callbacks[currentIndex].time < prevTime) {
                callbacks[currentIndex].backward?.()
            }
        }
    }
    
    prevProgress = progress
}

This reduces complexity from O(n) per frame to O(1) in most cases.

---

🔒 Security

No security concerns identified. The callbacks are user-provided functions with no special privileges.

---

📊 Performance

• Minor concern: See Adding useViewportScroll #7 above regarding callback iteration
• Memory: Minimal overhead - just storing callback references and times
• Animation performance: No impact on animation calculations since callbacks are handled separately

---

📋 Recommendations Summary

Must fix before merge:

1. Update WAAPI implementation or document limitation
2. Handle empty sequence edge case
3. Add integration tests for callback firing

Should fix:
4. Improve isCallbackSegment type guard logic
5. Remove any cast for type safety
6. Fix callback firing boundary conditions

Nice to have:
7. Optimize callback iteration for sequences with many callbacks
8. Add tests for backward scrubbing behavior

---

📝 Nitpicks

• Line 50: Consider extracting magic value 0.3 (defaultDuration) to a named constant
• Documentation: Consider adding JSDoc to createCallbackUpdater explaining the progress → time conversion

---

Overall, this is solid work with a clean design. The main blockers are the WAAPI inconsistency and missing integration tests. Once those are addressed, this will be a great addition to the library! 🚀