Skip to content

Comments

Add update() method to DragGesture to handle prop changes#3443

Merged
mattgperry merged 1 commit intomainfrom
claude/fix-drag-controls-LixiH
Jan 5, 2026
Merged

Add update() method to DragGesture to handle prop changes#3443
mattgperry merged 1 commit intomainfrom
claude/fix-drag-controls-LixiH

Conversation

@mattgperry
Copy link
Collaborator

The DragGesture class was missing an update() method that other gesture features (like PanGesture) implement. This meant that when props changed during a drag operation, the gesture feature wasn't notified.

This fix adds the update() method which handles dragControls prop changes, ensuring that when the dragControls prop is updated, the old subscription is removed and a new one is created with the updated controls.

Fixes #1185

The DragGesture class was missing an update() method that other gesture
features (like PanGesture) implement. This meant that when props changed
during a drag operation, the gesture feature wasn't notified.

This fix adds the update() method which handles dragControls prop changes,
ensuring that when the dragControls prop is updated, the old subscription
is removed and a new one is created with the updated controls.

Fixes #1185
@claude
Copy link

claude bot commented Jan 5, 2026

Pull Request Review

Thank you for this PR! This is a well-targeted fix that addresses a real issue where DragGesture wasn't responding to prop changes. Here's my detailed review:


Strengths

  1. Correct Problem Identification: The DragGesture class was indeed missing the update() method that other gesture features (like PanGesture) implement, causing it to miss prop changes during runtime.

  2. Consistent with Existing Patterns: The implementation follows the same pattern as PanGesture's update() method (packages/framer-motion/src/gestures/pan/index.ts:58-60), maintaining architectural consistency.

  3. Proper Resource Management: The code correctly cleans up the old subscription via removeGroupControls() before creating a new one, preventing memory leaks.

  4. Good Test Coverage: The new test effectively validates the fix by:

    • Creating two separate drag controls
    • Switching between them during runtime
    • Verifying both work correctly after the switch

🔍 Code Quality Observations

Logic Issue: Missing noop Reset

In packages/framer-motion/src/gestures/drag/index.ts:34, when calling this.removeGroupControls(), the function is invoked but removeGroupControls isn't reset. This could cause a stale function reference to remain.

Current code:

if (dragControls !== prevDragControls) {
    this.removeGroupControls()
    if (dragControls) {
        this.removeGroupControls = dragControls.subscribe(this.controls)
    }
}

Potential issue: When dragControls becomes undefined or changes to null, removeGroupControls should be reset to noop to maintain consistency with the mount() method pattern.

Suggested fix:

if (dragControls !== prevDragControls) {
    this.removeGroupControls()
    if (dragControls) {
        this.removeGroupControls = dragControls.subscribe(this.controls)
    } else {
        this.removeGroupControls = noop
    }
}

This ensures that if dragControls is removed (set to undefined), we don't keep a stale subscription cleanup function around.


🧪 Test Coverage

Strengths:

  • Tests the core functionality of switching dragControls
  • Uses realistic component structure with state management
  • Properly awaits frames and verifies event handlers

Consider adding:

  1. Edge case: dragControls set to undefined - Test transitioning from a defined dragControls to undefined
  2. Multiple rapid updates - Verify no memory leaks occur when switching controls multiple times quickly
  3. During active drag - What happens if dragControls changes while a drag is in progress? (Though this may be expected to be a no-op)

🎯 Performance Considerations

The implementation is efficient:

  • Only updates when dragControls reference actually changes (identity check)
  • No unnecessary subscriptions or cleanup operations
  • Minimal overhead from the update check

🔒 Security Concerns

No security issues identified. The code:

  • Doesn't expose any sensitive data
  • Properly manages subscriptions
  • Doesn't introduce XSS or injection vulnerabilities

📋 Minor Suggestions

  1. Test naming: The test "dragControls can be updated" could be more specific, e.g., "dragControls subscription is updated when prop changes"

  2. Code comment: Consider adding a brief comment explaining why we check prevProps in the update method, similar to how PanGesture has contextual comments.


Recommendation

Approve with minor suggestion: The core fix is solid and solves the reported issue. The only substantive suggestion is to add the else clause to reset removeGroupControls to noop when dragControls becomes undefined, maintaining consistency with the initialization pattern in mount().

Great work identifying and fixing this gap in the Feature lifecycle! This will resolve issue #1185 where drag gestures weren't respecting updated props.


Files reviewed:

  • packages/framer-motion/src/gestures/drag/index.ts
  • packages/framer-motion/src/gestures/drag/tests/use-drag-controls.test.tsx

@mattgperry mattgperry merged commit d53ece5 into main Jan 5, 2026
4 checks passed
@mattgperry mattgperry deleted the claude/fix-drag-controls-LixiH branch January 5, 2026 13:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG] drag not respecting previously updated props

2 participants