PR Review: Fix reorder autoscroll on scrollable pages

Summary

This PR fixes a coordinate system mismatch bug in the Reorder component's auto-scroll feature. The issue occurred when a scrollable container was placed inside a scrollable page - the auto-scroll would fail because gesture coordinates (pageX/pageY) weren't being converted to viewport coordinates before comparison with getBoundingClientRect().

Code Quality ✅

Excellent fix! The solution is:

• Minimal and focused: Only 6 lines changed in the core implementation
• Well-documented: Clear inline comments explain the coordinate system issue (lines 97-99 in auto-scroll.ts)
• Architecturally sound: Handles the conversion at the right layer (before distance calculations)

The Fix (auto-scroll.ts:97-101)

// Convert pointer position from page coordinates to viewport coordinates.
// The gesture system uses pageX/pageY but getBoundingClientRect() returns
// viewport-relative coordinates, so we need to account for page scroll.
const viewportPointerPosition =
    pointerPosition - (axis === "x" ? window.scrollX : window.scrollY)

This is the correct approach - adjusting page coordinates to viewport coordinates before comparing against the getBoundingClientRect() result.

Test Coverage ✅

Strong test coverage with good practices:

Strengths:

1. Dedicated test file: New reorder-auto-scroll-page.tsx specifically tests the scrollable-page scenario
2. Clear test structure: Tests are well-organized into describe blocks separating different scenarios
3. Comprehensive coverage: Tests both the original scenario (container only) and the new scenario (page + container)
4. Real-world reproduction: The test setup accurately reproduces the bug conditions with proper page scroll

Test Structure:

• Existing tests preserved: Original tests moved into describe("with scrollable container")
• New test added: describe("with scrollable container inside scrollable page") validates the fix
• Test methodology: Uses actual window.scrollTo() and verifies container.scrollTop changes

Potential Issues & Considerations

1. Horizontal Scrolling Edge Case ⚠️

The fix only accounts for window.scrollX when axis === "x" and window.scrollY when axis === "y". However, what if the page is scrolled in BOTH directions and you're doing vertical auto-scroll?

Example scenario:

• Page is scrolled 200px horizontally (window.scrollX = 200)
• Page is scrolled 300px vertically (window.scrollY = 300)
• User drags vertically in a container
• The fix correctly subtracts window.scrollY (300) from the vertical pointer position
• ✅ This is correct!

After analysis, the current implementation is actually correct - it only subtracts the relevant axis's scroll offset.

2. Missing Test for X-Axis Auto-Scroll

The tests only cover vertical (y-axis) auto-scroll. Consider adding tests for horizontal (x-axis) auto-scroll to ensure the window.scrollX path is also tested.

3. Test File Location & Naming

Minor: The new test file is at dev/react/src/tests/reorder-auto-scroll-page.tsx while the original is reorder-auto-scroll.tsx. The naming is clear and consistent. ✅

4. Cross-Browser Compatibility

window.scrollX and window.scrollY are well-supported (all modern browsers), but note:

• IE11 doesn't support them (uses window.pageXOffset/window.pageYOffset)
• Since this is motion v12+, IE11 support is likely not a concern ✅

5. Nested Scrollable Containers

The fix handles one level of page scroll, but what about:

• Multiple nested scrollable containers?
• A scrollable container inside another scrollable container inside a scrollable page?

Currently, findScrollableAncestor() finds the nearest scrollable ancestor, and the coordinate conversion only accounts for the page/window scroll. This should be fine because:

• getBoundingClientRect() always returns viewport-relative coordinates
• The pointer position from gestures is always page-relative
• The conversion to viewport coordinates is correct for any nesting level ✅

Performance Considerations ✅

No performance concerns:

• window.scrollX and window.scrollY are simple property reads (not layout-triggering)
• The subtraction is O(1)
• No additional DOM queries or expensive operations added

Security Concerns ✅

No security issues identified. The change is purely mathematical coordinate conversion.

Recommendations

Must Fix: None

The PR is production-ready as-is.

Nice to Have:

1. Add x-axis test: Consider adding a test for horizontal auto-scroll with page scroll to ensure symmetry
2. Consider edge case test: Test with both horizontal AND vertical page scroll simultaneously (though current implementation should handle this correctly)

Example test addition:

it("Auto-scrolls horizontal container after page has been scrolled both ways", () => {
    // Test x-axis auto-scroll with window scrolled in both directions
})

Final Verdict: ✅ LGTM - Approve

This is a clean, well-tested bug fix that solves a real coordinate system issue. The implementation is correct, the tests are comprehensive, and the code quality is excellent. The minor suggestions above are optional improvements, not blockers.

Great work on the fix and the thorough test coverage! 🎯