Fix pan event firing order: ensure onPanStart fires before onPan

[mattgperry]

Summary

This PR fixes the event firing order for pan gestures to ensure onPanStart fires before onPan, and onPan fires before onPanEnd. Previously, onPan could fire in the same frame as onPanStart, violating the expected event sequence.

Key Changes

• Pan gesture handler: Wrapped the onPan callback with asyncHandler to defer its execution to the next frame, ensuring it fires after onPanStart
• Drag controls: Deferred onDrag callback execution using frame.postRender() to maintain consistent timing with pan events
• Test coverage: Added comprehensive test case "onPanStart fires before onPan" to verify the correct event firing order during drag operations

Implementation Details

The fix leverages the existing asyncHandler utility and frame.postRender() mechanism to defer callback execution to the appropriate frame phase. This ensures the event sequence is:

1. onPanStart - fires immediately when pan gesture begins
2. onPan - fires after the render phase (via asyncHandler)
3. onPanEnd - fires when the gesture ends

This maintains backward compatibility while fixing the event ordering issue that could cause state management problems in applications relying on the expected event sequence.

https://claude.ai/code/session_01NUF3Mj5SSALV5gSW6AVGn6

[claude]

PR Review: Fix pan event firing order

Summary

This PR addresses a timing issue where onPan could fire in the same frame as onPanStart, violating the expected event sequence. The fix ensures proper event ordering: onPanStart → onPan → onPanEnd.

---

✅ Code Quality & Best Practices

Strengths:

• Clean, minimal changes that follow the existing codebase patterns
• Reuses the existing asyncHandler utility and frame.postRender() mechanism
• Consistent approach across both pan gestures and drag controls
• Well-structured test case with clear assertions
• Follows the repository's code style guidelines

Observations:

• The asyncHandler wrapper (packages/framer-motion/src/gestures/pan/index.ts:8-13) is elegant and maintains consistency
• The drag controls change (packages/framer-motion/src/gestures/drag/VisualElementDragControls.ts:230-232) properly wraps the callback in a conditional block

---

🐛 Potential Issues

1. Event Object Validity Concern
The onDrag callback now defers execution via frame.postRender():

if (onDrag) {
    frame.postRender(() => onDrag(event, info))
}

Issue: The event and info objects are passed into a deferred callback. If these objects are pooled or mutated after the current frame, consumers might receive stale data.

Recommendation: Verify that PointerEvent and PanInfo objects remain valid across frame boundaries. If event pooling is used, consider creating frozen snapshots of the necessary data before deferring.

2. Test Robustness
The new test (packages/framer-motion/src/gestures/tests/pan.test.tsx:47-79) validates event ordering but could be more thorough:

expect(startIndex).toBeLessThan(firstPanIndex)

Potential Gap: The test doesn't verify that:

• onPan fires in a different frame than onPanStart
• Multiple onPan events maintain proper ordering
• The fix works correctly with rapid pointer movements

Recommendation: Consider adding assertions that check frame boundaries more explicitly, perhaps by capturing timestamps or frame counts.

---

⚡ Performance Considerations

Frame Scheduling Overhead:
Each onPan callback now schedules a postRender task. For high-frequency pan gestures (e.g., 60+ events per second), this creates many deferred callbacks.

Analysis:

• Looking at packages/motion-dom/src/frameloop/batcher.ts:42-73, the postRender step is the last phase in the render loop
• The frame loop is optimized and well-tested, so the overhead should be minimal
• The fix is consistent with how onDragStart (line 178) and onDragEnd (line 288) already work

Verdict: Performance impact should be negligible. The consistency with existing patterns is more valuable than micro-optimizing this path.

---

🔒 Security Concerns

No security issues identified. The changes don't introduce:

• User input validation concerns
• DOM manipulation vulnerabilities
• State management race conditions that could be exploited

---

🧪 Test Coverage

Current Coverage:
✅ Event ordering verification
✅ Handler update behavior (existing test)
✅ Threshold behavior (existing test)

Gaps:
❌ No test for the drag controls onDrag callback timing
❌ No test verifying frame boundary separation
❌ No test for rapid successive pan events

Recommendation:

test("onDrag fires after render in drag controls", async () => {
    const renderOrder: string[] = []
    const Component = () => {
        const ref = useRef<HTMLDivElement>(null)
        return (
            <motion.div
                ref={ref}
                drag
                onDrag={() => renderOrder.push("drag")}
                onUpdate={() => renderOrder.push("render")}
            />
        )
    }
    // ... verify render happens before drag callback
})

---

📊 Implementation Review

PanSession Integration (packages/framer-motion/src/gestures/pan/PanSession.ts:280-285):
The updatePoint method calls handlers synchronously:

if (!isPanStarted) {
    onStart && onStart(this.lastMoveEvent, info)
    this.startEvent = this.lastMoveEvent
}
onMove && onMove(this.lastMoveEvent, info)

With the new asyncHandler, onStart defers to postRender, while onMove (also wrapped) does the same. This ensures they both execute in the postRender phase but in the correct sequence since onStart is called first in this synchronous block.

However: Since both are deferred to the same postRender phase, they might still execute in the same frame—just in the correct order. This is actually the desired behavior for maintaining timing consistency.

---

✨ Recommendations

1. Documentation: Add a comment explaining why asyncHandler is necessary for event ordering
2. Test Enhancement: Add a test that validates frame separation if that's a critical requirement
3. Event Object Safety: Verify/document that event objects remain valid across frames
4. Drag Controls Test: Add test coverage for the onDrag timing fix in VisualElementDragControls

---

🎯 Final Verdict

APPROVED with minor suggestions

This is a well-implemented fix that:

• Solves the reported issue
• Maintains backward compatibility
• Follows existing patterns
• Includes test coverage

The implementation is sound, though additional test coverage and documentation would strengthen it further. The approach of using frame.postRender() is consistent with how other lifecycle events are handled in the codebase.

Merge confidence: High ✅

[claude]

Pull Request Review

This PR addresses issue #2056 by fixing the event firing order for pan and drag gestures. The fix ensures onPanStart fires before onPan, and onDragStart fires before onDrag.

---

✅ Positive Findings

1. Correct Root Cause Analysis

The fix correctly identifies the timing issue: both onPanStart and onPan are called within the same frame.update() cycle in PanSession.updatePoint() (PanSession.ts:280-285), but previously onPanStart was deferred to frame.postRender() while onPan executed synchronously, causing the reversed order.

2. Appropriate Solution

Changing from frame.postRender() to frame.update() is the right approach:

• Both handlers now execute in the same frame phase (update)
• The sequential call order in PanSession.updatePoint() naturally ensures onStart fires before onMove
• Maintains responsiveness by keeping events in the update phase rather than deferring to postRender

3. Good Test Coverage

The new test case at pan.test.tsx:47-79 properly validates the fix:

• Tests the actual bug condition (event firing order)
• Uses appropriate assertions to verify startIndex < firstPanIndex
• Follows existing test patterns in the file

4. Consistent Application

The fix is consistently applied to both:

• Pan gestures (pan/index.ts:11)
• Drag gestures (VisualElementDragControls.ts:178)

---

⚠️ Concerns & Questions

1. Critical Issue: onPan Handler Inconsistency

Looking at pan/index.ts:38, I notice that onMove: onPan is NOT wrapped with asyncHandler, while onStart: asyncHandler(onPanStart) is wrapped.

This creates a timing discrepancy:

• onPanStart is wrapped with asyncHandler → deferred to frame.update()
• onPan is called directly → executes synchronously in PanSession.updatePoint()

Question: Why isn't onPan also wrapped with asyncHandler? This asymmetry seems intentional but creates potential for the same timing issue this PR is trying to fix. When PanSession.updatePoint() calls both handlers (lines 281, 285), onPan executes immediately while onPanStart is deferred.

Expected behavior: Either both should be wrapped with asyncHandler, or the commit message should explain why only start events need deferral.

2. Incomplete Fix for onDrag?

Similar to the pan handler, I need to verify whether onDrag has the same asymmetry. Looking at VisualElementDragControls.ts:

Line 178: onDragStart is now deferred with frame.update() ✅

But where is onDrag called? It should also be examined for consistency. The drag system uses the pan gesture under the hood, so onDrag maps to onPan, but we should verify the timing is correct throughout the chain.

3. Test Coverage Gap

The test only verifies the pan gesture event order. Consider adding:

• A similar test for drag events (onDragStart before onDrag)
• Edge case: what happens when drag/pan is very quick (single frame)?
• Verify that onPanEnd still fires after onPan (which uses frame.postRender() on line 42)

4. Performance Consideration

Using frame.update() instead of frame.postRender() means callbacks execute earlier in the frame lifecycle. This is generally good for responsiveness, but:

Question: Are there any visual effects or animations that depend on these events firing in the postRender phase? The commit message mentions "without introducing lag" but doesn't provide before/after performance analysis.

5. Backward Compatibility

The commit message claims this "maintains backward compatibility," but this is a behavior change:

• Applications that worked around the bug (e.g., using setTimeout to ensure correct order) might now have double delays
• Code that relied on onPanStart firing in postRender phase might break

Suggestion: Consider documenting this as a breaking change in a changelog/migration guide, even though it fixes incorrect behavior.

---

🔍 Code Quality

Positive:

• ✅ Follows existing code patterns
• ✅ Uses existing utilities (asyncHandler, frame.update)
• ✅ Minimal changes (2 lines modified)
• ✅ Clear commit message with context

Suggestions:

• Add JSDoc comments explaining why frame.update() is used for event timing
• Consider adding a comment in asyncHandler explaining its purpose for event ordering

---

🔒 Security

No security concerns identified. This is an internal timing fix with no user input handling changes.

---

📋 Recommendations

Before Merging:

1. [CRITICAL] Clarify why onMove: onPan (line 38 in pan/index.ts) is not wrapped with asyncHandler - this appears to contradict the fix
2. Add a test for drag event ordering (onDragStart before onDrag)
3. Run the full test suite to ensure no regressions (especially layout animations that might depend on frame timing)
4. Document this behavior change in CHANGELOG.md

Nice to Have:

• Add inline comment explaining the frame.update() choice
• Verify no performance regression with rapid gesture interactions
• Consider updating CLAUDE.md with guidance on frame timing for future contributors

---

Summary

This is a well-targeted fix for a real bug (issue #2056), but there's a critical inconsistency where onPan is not wrapped with asyncHandler while onPanStart is. This needs clarification before merging, as it may mean the fix is incomplete or there's a subtle timing requirement I'm missing.

Recommendation: Request changes to address the onMove handler inconsistency.