fix(drag): Fix slow flick velocity and catch-and-release momentum

[mattgperry]

Summary

• Fix slow flick velocity ([BUG] Drag inertia decelerates abruptly after a fast flick on a touch device. #1747): Skip stale pointer-down origin point in PanSession.getVelocity() when calculating velocity for hold-then-flick gestures. The pointer-down timestamp was diluting velocity over a long time gap instead of using recent movement points.
• Fix momentum carry-over after catch-and-release: Stop animations on pointer down instead of pausing them. On release without drag, start a fresh constraint/snap-to-origin animation instead of resuming the old paused momentum animation.
• Remove unused pauseAnimation and getAnimationState private methods from VisualElementDragControls.

Test plan

• New Cypress test: fast flick after hold produces momentum (>300px travel)
• New Cypress test: catch-and-release stops momentum (element stays near caught position)
• All 24 existing drag Cypress tests pass
• All 87 unit test suites (723 tests) pass
• Build passes

🤖 Generated with Claude Code

[claude]

Code Review

I've reviewed this PR and it looks like a well-thought-out fix for drag momentum issues. Here's my detailed feedback:

✅ Strengths

1. Excellent Problem Analysis: The fix correctly identifies two distinct issues:

   • Velocity dilution from stale pointer-down timestamps in hold-then-flick gestures
   • Momentum carry-over after catch-and-release due to paused animations

2. Targeted Solution: The velocity calculation fix in PanSession.ts:392-398 is elegant and surgical. The heuristic (>2x timeDelta) is reasonable for detecting hold-then-flick patterns.

3. Comprehensive Test Coverage: Two new Cypress E2E tests directly verify the bug fixes with realistic gesture sequences. This aligns perfectly with CLAUDE.md's requirement to write tests for UI interaction bugs.

4. Code Cleanup: Removing unused pauseAnimation() and getAnimationState() methods reduces complexity.

5. Simplification: The drag control logic is now cleaner - always stop on pointer down, start fresh on release without drag.

🔍 Potential Issues & Questions

1. Edge Case in Velocity Calculation (Medium Priority)

// PanSession.ts:392-398
if (
    timestampedPoint === history[0] &&
    history.length > 2 &&
    lastPoint.timestamp - timestampedPoint.timestamp >
        secondsToMilliseconds(timeDelta) * 2
) {
    timestampedPoint = history[1]
}

Concern: What if history[1] is also very close to history[0] in time (e.g., a small jitter right after pointer-down)? This could still result in inflated velocity from a near-zero distance over near-zero time.

Suggestion: Consider checking that the distance between history[1] and lastPoint is meaningful, or validate that history[1].timestamp - history[0].timestamp is sufficiently small to ensure it's still representative of the flick motion.

2. Behavior Change in resumeAnimation (Low-Medium Priority)

// Before: Resume paused constraint/snap animation
// After: Always start fresh animation with {x: 0, y: 0}
const resumeAnimation = () => {
    const { dragSnapToOrigin: snap } = this.getProps()
    if (snap || this.constraints) {
        this.startAnimation({ x: 0, y: 0 })
    }
}

Question: Is {x: 0, y: 0} always the correct target for constraint animations? This seems to assume the constraint origin is always (0,0), but what about:

• Elements with dragConstraints that don't center on origin?
• Elements with dragElastic that should snap back to their constraint boundary, not (0,0)?

Suggestion: Verify this works correctly with various constraint configurations, or consider calculating the appropriate snap-back target based on current constraints.

3. Test Flakiness Risk (Low Priority)

The tests use hard-coded timing (wait(300), wait(50), wait(16)) which could be flaky on slower CI systems.

Suggestion: The tests look well-structured for Cypress patterns, but consider if any waits could be replaced with more deterministic conditions. The current approach is standard for Cypress though.

4. Missing Test Case (Low Priority)

The tests cover:

• Fast flick after hold ✓
• Catch-and-release stops momentum ✓

Suggestion: Consider adding a test for drag with constraints to verify resumeAnimation works correctly when dragging beyond constraint boundaries.

🎨 Code Quality

• ✅ Follows CLAUDE.md style guide (interfaces, named exports, arrow callbacks)
• ✅ Good separation of concerns
• ✅ Clear, self-documenting code with appropriate comments
• ✅ No security concerns (no user input handling beyond pointer events)

🚀 Performance

• ✅ The velocity calculation change adds minimal overhead (one extra check)
• ✅ Removing pause/resume complexity should slightly improve performance
• ✅ No unnecessary loops or expensive operations

📋 Test Coverage

• ✅ Two targeted E2E tests for the specific bugs
• ✅ PR description confirms all existing tests pass
• ⚠️ Consider adding a unit test for the getVelocity edge case logic (testing with synthetic history arrays)

🎯 Overall Recommendation

Approve with minor suggestions. This is a solid fix that addresses real user-facing issues. The main concern is ensuring resumeAnimation's behavior is correct for all constraint scenarios. I'd recommend:

1. Manual testing with various dragConstraints configurations
2. Consider the history[1] edge case mentioned above
3. Optionally add a constraint-based test case

Great work on the thorough testing and clean implementation! 🎉

[claude]

Pull Request Review

Great work on addressing issue #1747! This PR fixes two related drag momentum bugs with a clean, well-tested implementation. Here's my detailed review:

---

✅ Code Quality & Implementation

PanSession.ts velocity calculation fix (lines 385-399)

• Excellent solution: The logic to skip stale pointer-down points when calculating velocity is well-reasoned. The 2x timeDelta threshold (200ms when timeDelta=0.1s) is a good heuristic to detect hold-then-flick gestures.
• Clear comments: The inline explanation effectively describes the problem and solution.
• Edge case handling: Properly checks history.length > 2 before accessing history[1].

VisualElementDragControls.ts animation lifecycle fix (lines 110-114, 237-242)

• Simplified logic: Removing the pauseAnimation complexity in favor of always stopping animations on pointer-down is cleaner and more predictable.
• Correct resumeAnimation behavior: Starting a fresh constraint/snap animation instead of resuming a paused momentum animation makes sense for catch-and-release scenarios.
• Good cleanup: Removing unused pauseAnimation and getAnimationState methods reduces code complexity.

---

🧪 Test Coverage

Cypress E2E tests are excellent:

• Tests both bug fixes with realistic user interactions
• Clear assertions with meaningful thresholds (>300px travel, <50px tolerance)
• Good comments explaining expected behavior and what the fix prevents
• Follows repository testing patterns

Minor suggestions:

1. Consider adding edge cases to the test suite:

   • Very short holds (e.g., 50ms) to ensure normal drags aren't affected
   • Multiple catch-and-release cycles in succession
   • Testing with dragMomentum={false} to ensure no regression

2. The wait times in tests (300ms, 50ms, 500ms) work but could be fragile on slow CI. Consider using Cypress's built-in retry-ability or cy.tick() if using fake timers.

---

⚠️ Potential Issues & Considerations

1. Magic number in velocity calculation:

secondsToMilliseconds(timeDelta) * 2  // Why 2x?

• While 2x seems reasonable, consider extracting this as a named constant:

const STALE_POINT_THRESHOLD_MULTIPLIER = 2

This makes the heuristic more discoverable and easier to tune if needed.

2. Behavior change for non-flick gestures:
The resumeAnimation change (VisualElementDragControls.ts:237-242) now always starts a fresh animation when:

• dragSnapToOrigin is true, OR
• constraints exist

Question: What happens if a user:

1. Drags an element near the constraint boundary
2. Releases to trigger momentum animation toward boundary
3. Immediately taps the element (pointer down + up without drag)

Before: The momentum animation would resume
After: A fresh snap-to-origin animation starts

Is this the desired behavior? If the user just tapped without dragging, should we distinguish between "interrupted during momentum" vs "fresh start"? The current logic seems correct for the catch-and-release case, but worth verifying for tap-during-momentum scenarios.

3. Snap-to-cursor interaction:
In onSessionStart, snapToCursor now calls stopAnimation() after snapping:

if (snapToCursor) {
    this.snapToCursor(extractEventInfo(event).point)
}
this.stopAnimation()  // Always called now

This is fine, but the previous logic suggested snapToCursor needed special handling. Verify that removing this distinction doesn't cause issues.

4. Type safety in test:

const draggable = $draggable[0] as HTMLDivElement

Consider adding runtime type checking in tests to fail gracefully if the element is unexpectedly null.

---

🔒 Security Considerations

No security concerns identified. The changes are purely behavioral and don't introduce:

• User input handling vulnerabilities
• XSS risks
• Prototype pollution
• Resource exhaustion issues

---

⚡ Performance Considerations

Positive impacts:

• Removing pauseAnimation and getAnimationState reduces code paths and potential memory leaks from paused animations
• More aggressive stopAnimation() calls prevent accumulation of stale animation state

Neutral:

• The additional check in getVelocity (lines 392-398) adds minimal overhead and only executes when conditions are met
• Fresh animation starts vs. resumed animations should have comparable performance

---

📝 Suggestions & Best Practices

1. Consider adding unit tests:
While the Cypress tests are excellent for integration testing, consider adding unit tests for:

• getVelocity() function with various history patterns (hold-then-flick, continuous drag, etc.)
• Edge cases like single-point history, identical timestamps

2. Documentation:
Consider adding JSDoc to the velocity calculation logic explaining the 2x threshold choice:

/**
 * Skip stale pointer-down origin if:
 * - It's the first point (history[0])
 * - Better points exist (history.length > 2)  
 * - Time gap is suspiciously large (>2x timeDelta = >200ms)
 * 
 * This prevents hold-then-flick gestures from having velocity
 * diluted by the stale pointer-down timestamp.
 */

3. CLAUDE.md compliance:
✅ Uses strict equality (===)
✅ Uses const/let (no var)
✅ Named exports maintained
✅ Follows existing code patterns

---

🎯 Summary

Strengths:

• Cleanly fixes two related bugs with minimal code changes
• Well-tested with realistic E2E scenarios
• Reduces code complexity by removing unused methods
• Clear commit message and PR description

Recommendations:

1. Extract magic number (2) as named constant
2. Verify tap-during-momentum behavior is intentional
3. Consider adding unit tests for getVelocity()
4. Add edge case tests (short holds, multiple catches)

Overall Assessment: This is a high-quality PR that effectively addresses the reported issues. The implementation is sound, well-tested, and improves code maintainability. With minor refinements around edge cases and constants, this is ready to merge.

Great work! 🚀

[claude]

Pull Request Review: Fix drag momentum velocity and catch-and-release behavior

Summary

This PR addresses two related issues in drag gesture handling:

1. Slow flick velocity after hold: Fixes velocity calculation being diluted by stale pointer-down timestamps
2. Momentum carry-over after catch-and-release: Ensures animations stop properly when catching a moving element

Code Quality & Best Practices ✅

Strengths

• Clear intent: The changes are focused and address specific bugs without introducing unnecessary complexity
• Code style compliance: Follows the repository's style guide (optional chaining, arrow functions, concise patterns)
• File size consciousness: The fix in PanSession.ts is minimal and efficient, aligning with the library's goal to minimize bundle size
• Proper cleanup: Removal of unused pauseAnimation and getAnimationState methods reduces dead code

Implementation Analysis

PanSession.ts (lines 385-399)

The velocity calculation fix is elegant and well-reasoned:

if (
    timestampedPoint === history[0] &&
    history.length > 2 &&
    lastPoint.timestamp - timestampedPoint.timestamp >
        secondsToMilliseconds(timeDelta) * 2
) {
    timestampedPoint = history[1]
}

Rationale: When a user holds down the pointer for >200ms (2x the 100ms timeDelta) before flicking, the algorithm was calculating velocity from the initial pointer-down point, resulting in artificially low velocity. Skipping to history[1] when this condition is met ensures velocity reflects the recent flick motion.

Trade-off consideration: The 2x timeDelta threshold is reasonable, but this is a magic number. Consider if this should be configurable or documented more explicitly. However, given the library's focus on file size, the current approach is pragmatic.

VisualElementDragControls.ts (lines 110-115, 237-242)

The animation lifecycle changes are correct:

• Before: pauseAnimation() on pointer down, stopAnimation() on drag start
• After: stopAnimation() on pointer down unconditionally
• Resume logic: Instead of resuming paused animations, starts fresh constraint/snap-to-origin animations

This prevents the problematic scenario where:

1. User drags and releases → momentum animation starts
2. User catches element → old animation pauses
3. User releases without dragging → old momentum animation resumes (❌ bug)

New behavior: After catch-and-release, if constraints or dragSnapToOrigin are set, a fresh animation to origin is started instead.

Test Coverage ✅

Cypress E2E Tests

The two new tests in drag-momentum.ts directly verify the fixed behaviors:

1. "Fast flick after hold produces momentum" (lines 2-24):

   • Simulates 300ms hold, then quick flick
   • Verifies element travels >300px (top < -200)
   • Strong test: Directly validates the velocity calculation fix

2. "Catch-and-release stops momentum" (lines 26-62):

   • Throws element, waits 100ms, catches and immediately releases
   • Verifies element stays within 50px of caught position
   • Strong test: Validates the animation stop/resume fix

Test quality: Both tests use realistic gesture sequences and verify observable behavior (element position) rather than internal state. The timing values (waits) are appropriate for simulating human gestures.

Potential Test Improvements (minor)

• Consider testing edge cases:
  • What happens with very short holds (<50ms)?
  • What about catch-and-release with no constraints and no dragSnapToOrigin?
  • Multiple rapid catches?
• The 50px threshold in test 2 (line 60) seems reasonable but could be tighter with a comment explaining why 50px is acceptable

Potential Issues & Concerns

1. History Array Edge Cases ⚠️

In PanSession.ts:392-398, the code checks history.length > 2 before accessing history[1]. However:

• What if history.length === 2? The original point would still be used even if the time gap is large
• Recommendation: Consider history.length >= 2 or add a comment explaining why length must be > 2

2. Type Safety (minor)

PanSession.ts:393: The identity check timestampedPoint === history[0] is correct, but TypeScript's strict null checks should ensure timestampedPoint is not null here (due to line 381 guard). Good defensive programming.

3. Backward Compatibility ✅

The behavior changes are bug fixes that improve user experience. No breaking API changes. Users may notice improved drag feel, which is positive.

4. Performance ✅

• Minimal overhead: One additional condition check in the velocity calculation
• No new allocations or loops
• Stopping animations immediately (vs pausing) is potentially more efficient

Security Concerns ✅

No security issues identified:

• No user input parsing or validation changes
• No XSS vectors introduced
• No prototype pollution risks
• Event handling remains properly bounded

Additional Observations

1. The resumeAnimation Function (VisualElementDragControls.ts:237-242)

const resumeAnimation = () => {
    const { dragSnapToOrigin: snap } = this.getProps()
    if (snap || this.constraints) {
        this.startAnimation({ x: 0, y: 0 })
    }
}

Question: Should this pass velocity { x: 0, y: 0 } to startAnimation? Looking at line 276, startAnimation expects velocity to calculate momentum. Passing zero velocity means no momentum, which is correct for snap-to-origin and constraint animations after catch-and-release. ✅

2. Removed Methods

The deletion of pauseAnimation() and getAnimationState() is safe since:

• Git history shows they were only used in the now-removed pause/resume logic
• No external API surface change (they were private)

3. Documentation

The PR description is excellent and clearly explains:

• What was broken
• Why it was broken
• How the fix works
• Test coverage

Recommendations

High Priority

None - the code is ready to merge

Low Priority (future considerations)

1. Document the magic number: Consider adding a constant for the 2x timeDelta threshold or a comment explaining the heuristic
2. Additional test case: Test the history.length === 2 edge case explicitly
3. Memory profile: With drag being a hot path, ensure the resumeAnimation closure doesn't cause memory issues (likely fine, but worth a profile pass)

Final Verdict

✅ APPROVE - Ready to Merge

This is a well-crafted bug fix that:

• Solves real UX issues (references [BUG] Drag inertia decelerates abruptly after a fast flick on a touch device. #1747)
• Includes comprehensive tests
• Maintains code quality and library size constraints
• Has no breaking changes or security concerns
• Follows repository conventions

The implementation is thoughtful, the tests verify the fixes, and the code is production-ready. Excellent work!

---

Review generated via Claude Code - Analysis based on CLAUDE.md conventions and best practices