Skip to content

Comments

Fix snapToCursor not working with initial coordinates#3445

Merged
mattgperry merged 1 commit intomainfrom
claude/fix-usedragcontrols-VumSW
Jan 6, 2026
Merged

Fix snapToCursor not working with initial coordinates#3445
mattgperry merged 1 commit intomainfrom
claude/fix-usedragcontrols-VumSW

Conversation

@mattgperry
Copy link
Collaborator

When using useDragControls with snapToCursor: true on a component with initial coordinates, the component would snap to the wrong position on the first drag. This was because the layout measurement includes the current transform value, but the snap calculation didn't account for it.

The fix adds the current motion value to the snap calculation to ensure the element snaps to the correct cursor position regardless of its initial coordinates.

Fixes #1607

When using useDragControls with snapToCursor: true on a component with
initial coordinates, the component would snap to the wrong position on
the first drag. This was because the layout measurement includes the
current transform value, but the snap calculation didn't account for it.

The fix adds the current motion value to the snap calculation to ensure
the element snaps to the correct cursor position regardless of its
initial coordinates.

Fixes #1607
@claude
Copy link

claude bot commented Jan 5, 2026

Code Review

Summary

This PR fixes a bug where snapToCursor: true on useDragControls would incorrectly position elements with initial coordinates on the first drag. The fix is well-implemented and addresses the root cause.

Code Quality ✅

Strengths:

  • Clean, minimal fix: The solution adds only 2 lines of actual logic, demonstrating good understanding of the problem
  • Excellent documentation: The multi-line comment clearly explains why the fix is needed
  • Follows repo conventions: Uses interface (not shown but implied), proper formatting, and naming conventions per CLAUDE.md
  • Good variable naming: current is clear and concise

Minor suggestions:

  • Consider renaming current to currentValue or currentAxisValue for slightly better clarity, though current is acceptable given the context

Bug Fix Analysis ✅

The fix is mathematically correct:

  • Problem: projection.layout.layoutBox includes the current transform, so mixNumber(min, max, 0.5) calculates the center in transformed space
  • Solution: Adding current compensates for the transform, ensuring the cursor-to-element-center calculation works regardless of initial position
  • Formula: point[axis] - center + current correctly positions the element so its center aligns with the cursor

This explains why the second drag worked but the first didn't—after the first drag, the element was already offset incorrectly, but subsequent drags from that position appeared correct relative to where it landed.

Test Coverage ✅

Excellent test design:

  • Tests the exact bug scenario: element with initial={{ x: 100, y: 100 }}
  • Smart verification approach: Compares first drag to second drag, ensuring consistent behavior
  • Uses toBeCloseTo() appropriately for float comparisons
  • Properly uses nextFrame() for async frame handling per repo guidelines
  • Good use of comments explaining expected behavior

Potential improvements:

  1. Could add assertions for the actual expected values (e.g., verify element centers at cursor position), though the current approach of comparing consistency is also valid
  2. Consider adding a test case with negative initial values to ensure the fix works in all quadrants
  3. Edge case: What happens when projection or projection.layout is undefined? The existing if (projection && projection.layout) guard handles this, but a test confirming graceful degradation would be comprehensive

Performance Considerations ✅

  • Negligible impact: Adds one .get() call per axis during drag initialization only
  • No performance concerns—this is a one-time calculation at drag start, not per-frame

Security Concerns ✅

No security issues identified. This is a pure calculation fix with no user input handling changes.

Additional Observations

  1. Type safety: The || 0 fallback for axisValue.get() is good defensive programming
  2. Fixes a 3+ year old issue: Great to see [BUG] snapToCursor with initial property not working #1607 finally resolved!
  3. Maintains backward compatibility: No breaking changes, purely a bug fix

Recommendations

Required before merge:

  • None—this is ready to merge as-is

Optional enhancements:

  • Add test case with negative initial coordinates
  • Add test case with non-draggable axis to verify early return path

Conclusion

LGTM ✅ This is a well-crafted fix with excellent test coverage. The code is clean, well-documented, and solves the reported issue correctly. Great work!


Review by Claude Code

@mattgperry mattgperry merged commit cd91119 into main Jan 6, 2026
4 checks passed
@mattgperry mattgperry deleted the claude/fix-usedragcontrols-VumSW branch January 6, 2026 10:01
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] snapToCursor with initial property not working

2 participants