Add auto-scroll support to Reorder.Group

[mattgperry]

Summary

• Adds scrollParent prop to Reorder.Group that enables automatic scrolling when dragging items near the edges of a scrollable container
• Adds scrollOptions prop for customizing threshold and max scroll speed
• Includes unit tests for the auto-scroll utility

Fixes #1339

Usage

function Component() {
  const scrollRef = useRef(null)
  const [items, setItems] = useState([0, 1, 2, 3, 4, 5])

  return (
    <div ref={scrollRef} style={{ overflow: "auto", height: 200 }}>
      <Reorder.Group 
        values={items} 
        onReorder={setItems} 
        scrollParent={scrollRef}
        scrollOptions={{ threshold: 50, maxSpeed: 25 }}
      >
        {items.map((item) => (
          <Reorder.Item key={item} value={item}>
            {item}
          </Reorder.Item>
        ))}
      </Reorder.Group>
    </div>
  )
}

Test plan

• Unit tests for auto-scroll utility pass
• Manual testing with scrollable container
• Verify existing Reorder functionality unchanged

🤖 Generated with Claude Code

[claude]

Code Review - PR #3446: Add auto-scroll support to Reorder.Group

Thanks for this PR! I've reviewed the changes and have some feedback on code quality, potential issues, and suggestions for improvement.

---

🎯 Overall Assessment

Strengths:

• Clean API design with sensible defaults
• Good separation of concerns (auto-scroll logic isolated in its own module)
• Comprehensive unit tests with good edge case coverage
• Well-documented with JSDoc comments and usage examples
• The CSS variable fix is a nice bonus for issue [BUG] translateY animation incorrectly preserves CSS variables from calc() expressions #3410

Areas for Improvement:

• Performance concerns with scroll implementation
• Missing cleanup/memory management
• Test coverage gaps for integration scenarios

[claude]

🐛 Critical Issues

1. Missing RequestAnimationFrame for Scroll Updates (Performance/UX)

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:62-68

The current implementation directly updates scroll position on every drag event, which can cause janky scrolling. This runs on every pointer move event (potentially 60+ times per second), directly mutating the DOM without batching or throttling.

Recommendation: Use requestAnimationFrame to ensure smooth scrolling. Store the RAF ID and cancel it in the stop() function.

---

2. Memory Leak: No Cleanup on Component Unmount

Location: packages/framer-motion/src/components/Reorder/Group.tsx:142-145

The autoScroll object is created via useMemo but never cleaned up. If scrollParent, axis, or scrollOptions change, a new autoScroll instance is created, but the old one isn't cleaned up.

Recommendation: Add cleanup with useEffect:

useEffect(() => {
    return () => {
        autoScroll?.stop()
    }
}, [autoScroll])

---

3. Stale Scroll Amount on Rapid Updates

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:54-69

If using RAF, the scroll amount calculation happens once but may be stale by the time the frame renders.

Recommendation: Store the latest pointer position and calculate scroll amount inside the RAF callback for accuracy.

[claude]

⚠️ Minor Issues & Test Coverage

4. Inconsistent Optional Chaining

Location: packages/framer-motion/src/components/Reorder/Group.tsx:173-181

If autoScroll exists but scrollParent?.current is null, the function still runs (just with null). Add an early return for clarity:

handleScroll: autoScroll
    ? (pointerPosition: number) => {
          const element = scrollParent?.current
          if (!element) return
          autoScroll.updateScroll(pointerPosition, element)
      }
    : undefined,

---

5. Missing Integration Tests

Location: Tests only cover the auto-scroll.ts utility in isolation

Missing scenarios:

1. Test that scrolling actually happens during drag (integration with Reorder.Item)
2. Test scroll boundaries (what happens when scrolled to top/bottom?)
3. Test with nested scroll containers
4. Test cleanup on unmount

Recommendation: Add Cypress/Playwright E2E tests in dev/react/src/tests/ similar to the unit conversion test you added.

---

6. Edge Case: Scroll Container Smaller Than Threshold

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:41-49

If a scroll container is smaller than 2 * threshold (default 100px), the threshold zones overlap.

Example: Container is 80px tall, threshold is 50px:

• Top threshold zone: 0-50px
• Bottom threshold zone: 30-80px
• These overlap!

Recommendation: Handle this edge case:

const containerSize = end - start
const effectiveThreshold = Math.min(threshold, containerSize / 2)

[claude]

🔒 Security & Code Style

Security: No concerns identified. The implementation doesn't execute arbitrary code, handle user input unsafely, expose sensitive data, or create XSS vectors.

Code Style (per CLAUDE.md):
✅ Uses interface for type definitions
✅ Uses named exports
✅ Uses arrow callbacks
✅ Strict equality
✅ No var declarations

---

🧪 CSS Variable Fix (Bonus)

The changes to DOMKeyframesResolver.ts and is-css-variable.ts look good for fixing issue #3410!

Minor observation: The containsCSSVariable check at line 41:

return value.split("/*")[0].includes("var(--")

This is a simple check that works for the use case. Consider using a regex if you need more robustness:

return /var\(--/.test(value.split("/*")[0])

---

✅ Recommendations Summary

Must Fix (before merge):

1. Add requestAnimationFrame to prevent janky scrolling
2. Add cleanup logic to prevent memory leaks
3. Handle the stale scroll amount issue

Should Fix:
4. Add E2E integration tests
5. Handle edge case where container is smaller than threshold
6. Add early return for null scrollParent for clarity

Nice to Have:
7. Add options validation (negative values)
8. Enhance documentation about scroll parent requirements

---

🎉 Conclusion

This is a well-designed feature with clean code and good test coverage for the utility layer. The main concerns are around performance (RAF) and memory management (cleanup). Once those are addressed, this will be a great addition to the Reorder component!

Great work on tackling issue #1339! 🚀

[claude]

Code Review - PR #3446: Add auto-scroll support to Reorder.Group

Thank you for this contribution! I've reviewed the implementation and have the following feedback:

---

✅ Strengths

1. Clean API Design: The scrollParent and scrollOptions props are intuitive and well-documented with JSDoc comments.

2. Good Test Coverage: The auto-scroll utility has comprehensive unit tests covering:

   • Vertical and horizontal scrolling
   • Edge detection logic
   • Custom options
   • Null element handling
   • Boundary conditions

3. Performance: Using useMemo (Group.tsx:142-145) to memoize the auto-scroll instance is good practice.

4. Non-Breaking: The feature is opt-in via the scrollParent prop, maintaining backward compatibility.

---

🔴 Critical Issues

1. Continuous Scroll Implementation Missing

The current implementation only scrolls once per onDrag event. For smooth auto-scrolling, you need a continuous scroll loop using requestAnimationFrame.

Current behavior in auto-scroll.ts:54-68:

• updateScroll is called during onDrag, which fires periodically but not continuously
• Scrolling stops between drag events, creating a jerky experience

Recommended fix:

export function createAutoScroll(axis: "x" | "y", options: AutoScrollOptions = {}) {
    const mergedOptions = { ...defaultOptions, ...options }
    let rafId: number | null = null
    let currentScrollElement: HTMLElement | null = null
    let currentPointerPosition: number | null = null

    function scroll() {
        if (\!currentScrollElement || currentPointerPosition === null) return
        
        const scrollAmount = getScrollAmount(currentPointerPosition, currentScrollElement)
        
        if (scrollAmount \!== 0) {
            if (axis === "x") {
                currentScrollElement.scrollLeft += scrollAmount
            } else {
                currentScrollElement.scrollTop += scrollAmount
            }
            rafId = requestAnimationFrame(scroll)
        } else {
            rafId = null
        }
    }

    function updateScroll(pointerPosition: number, scrollElement: HTMLElement | null) {
        currentPointerPosition = pointerPosition
        currentScrollElement = scrollElement
        
        if (\!rafId && scrollElement) {
            rafId = requestAnimationFrame(scroll)
        }
    }

    function stop() {
        if (rafId \!== null) {
            cancelAnimationFrame(rafId)
            rafId = null
        }
        currentPointerPosition = null
        currentScrollElement = null
    }

    return { updateScroll, stop }
}

---

2. Unrelated Changes Included

The PR includes test files for unit conversion (issue #3410) that are unrelated to the auto-scroll feature:

• dev/react/src/tests/unit-conversion-var-to-simple.tsx (32 lines)
• packages/framer-motion/cypress/integration/unit-conversion.ts (21 lines)

Action required: Remove these files or move them to a separate PR addressing issue #3410.

---

⚠️ Issues & Suggestions

3. Potential Edge Case: Non-Scrollable Elements

The code doesn't check if the element can actually scroll before attempting to scroll it.

Recommendation in auto-scroll.ts:54-68:

function updateScroll(pointerPosition: number, scrollElement: HTMLElement | null) {
    if (\!scrollElement) return
    
    // Check if element is scrollable
    const canScroll = axis === "x" 
        ? scrollElement.scrollWidth > scrollElement.clientWidth
        : scrollElement.scrollHeight > scrollElement.clientHeight
    
    if (\!canScroll) return
    
    // ... rest of implementation
}

---

4. Memory Leak Risk in useMemo Dependencies

Group.tsx:142-145:

const autoScroll = useMemo(
    () => (scrollParent ? createAutoScroll(axis, scrollOptions) : null),
    [axis, scrollParent, scrollOptions]
)

Issue: scrollOptions is an object reference that will change on every render unless memoized by the consumer.

Recommendation: Either document that users should memoize scrollOptions, or use a deep comparison:

const autoScroll = useMemo(
    () => (scrollParent ? createAutoScroll(axis, scrollOptions) : null),
    [axis, scrollParent, scrollOptions?.threshold, scrollOptions?.maxSpeed]
)

---

5. Missing Cleanup on Unmount

If the component unmounts while scrolling, the auto-scroll should be stopped.

Add to Group.tsx after line 186:

useEffect(() => {
    return () => {
        autoScroll?.stop()
    }
}, [autoScroll])

---

6. Type Safety Issue

Item.tsx:92-93 destructures handleScroll and stopScroll from context without checking if they exist first, though they're used conditionally later. This is fine, but TypeScript might complain.

Consider: Making these required in the type definition when scrollParent is provided, or adding explicit undefined checks.

---

7. Test Gap: Integration Tests Missing

While unit tests for auto-scroll are excellent, there are no integration tests for:

• The full Reorder.Group + Item interaction with scrolling
• Actual scroll behavior during drag operations
• Cleanup when drag ends

Recommendation: Add a Cypress or Playwright E2E test demonstrating the feature working end-to-end.

---

8. Documentation: Missing maxSpeed Units

auto-scroll.ts:10-12 says "pixels per frame" but should clarify this is per RAF callback (typically 60fps).

Suggested improvement:

/**
 * The maximum scroll speed in pixels per animation frame (60fps).
 * At 60fps, maxSpeed of 25 equals 1500px/second.
 * @default 25
 */
maxSpeed?: number

---

9. Code Style: Unnecessary Comment

auto-scroll.ts:71-73:

function stop() {
    // No-op, kept for API consistency
}

This comment is misleading since the function should actually cancel the RAF (see issue #1).

---

🎯 Performance Considerations

1. ✅ Good use of quadratic easing (Math.pow(intensity, 2)) for smoother acceleration
2. ✅ Conditional context properties avoid unnecessary overhead when not using auto-scroll
3. ⚠️ Consider throttling getBoundingClientRect() calls if performance becomes an issue (though likely not necessary)

---

🔒 Security

No security concerns identified. The implementation safely handles user input and DOM manipulation.

---

📋 Summary & Action Items

Must fix before merge:

1. ❌ Implement continuous scrolling with requestAnimationFrame
2. ❌ Remove unrelated unit-conversion test files

Should fix:
3. Add cleanup effect for auto-scroll on unmount
4. Handle scrollOptions reference stability issue
5. Add check for scrollable elements

Nice to have:
6. Add integration/E2E test
7. Improve documentation on maxSpeed units
8. Fix misleading comment in stop() function

---

Great work on the feature implementation! The core logic is solid, just needs the continuous scrolling mechanism to provide the smooth UX users expect. Looking forward to seeing this merged once the critical issues are addressed.

🤖 Review generated with Claude Code

[claude]

Code Review: Add auto-scroll support to Reorder.Group

Thanks for this contribution! This is a well-implemented feature that addresses a long-standing request (#1339). Here's my detailed review:

✅ Strengths

1. Clean API Design: The scrollParent and scrollOptions props are intuitive and follow React patterns well
2. Excellent Test Coverage: Comprehensive unit tests for the auto-scroll utility covering:
   • Both axes (x/y)
   • Edge detection logic
   • Custom options (threshold, maxSpeed)
   • Edge cases (null elements, boundary conditions)
3. Good Code Quality: Clean separation of concerns with the auto-scroll logic isolated in its own utility
4. Thoughtful UX: The quadratic easing (Math.pow(intensity, 2)) provides smooth acceleration as you approach edges
5. Documentation: JSDoc comments are clear and include usage examples

🔍 Potential Issues & Suggestions

1. Performance Concern: Continuous Scrolling (Medium Priority)

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:54-69

The current implementation scrolls on every onDrag event. This could cause performance issues with:

• High-frequency drag events
• Large scroll containers
• Complex DOM structures

Suggestion: Consider using requestAnimationFrame for smooth, throttled scrolling:

export function createAutoScroll(axis: "x" | "y", options: AutoScrollOptions = {}) {
    const mergedOptions = { ...defaultOptions, ...options }
    let rafId: number | null = null
    let targetScrollAmount = 0
    let currentElement: HTMLElement | null = null

    function scroll() {
        if (!currentElement || targetScrollAmount === 0) {
            rafId = null
            return
        }

        if (axis === "x") {
            currentElement.scrollLeft += targetScrollAmount
        } else {
            currentElement.scrollTop += targetScrollAmount
        }

        rafId = requestAnimationFrame(scroll)
    }

    function updateScroll(pointerPosition: number, scrollElement: HTMLElement | null) {
        if (!scrollElement) {
            targetScrollAmount = 0
            return
        }

        currentElement = scrollElement
        targetScrollAmount = getScrollAmount(pointerPosition, scrollElement)

        if (targetScrollAmount !== 0 && rafId === null) {
            rafId = requestAnimationFrame(scroll)
        } else if (targetScrollAmount === 0 && rafId !== null) {
            cancelAnimationFrame(rafId)
            rafId = null
        }
    }

    function stop() {
        targetScrollAmount = 0
        if (rafId !== null) {
            cancelAnimationFrame(rafId)
            rafId = null
        }
    }

    return { updateScroll, stop }
}

2. Missing Scroll Boundary Checks (Low Priority)

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:62-67

The code doesn't check if the element can actually scroll in the requested direction (e.g., already at top/bottom).

Suggestion: Add boundary checks:

if (scrollAmount !== 0) {
    if (axis === "x") {
        const newScrollLeft = scrollElement.scrollLeft + scrollAmount
        if (newScrollLeft >= 0 && newScrollLeft <= scrollElement.scrollWidth - scrollElement.clientWidth) {
            scrollElement.scrollLeft = newScrollLeft
        }
    } else {
        const newScrollTop = scrollElement.scrollTop + scrollAmount
        if (newScrollTop >= 0 && newScrollTop <= scrollElement.scrollHeight - scrollElement.clientHeight) {
            scrollElement.scrollTop = newScrollTop
        }
    }
}

3. useMemo Dependency Array (Low Priority)

Location: packages/framer-motion/src/components/Reorder/Group.tsx:142-145

The useMemo includes scrollOptions in the dependency array, which will create a new autoScroll instance every time scrollOptions changes (including when it's passed as an inline object).

Suggestion: Either:

• Document that scrollOptions should be memoized by consumers, or
• Deep compare the options, or
• Accept this as expected behavior (recreating is probably fine)

4. Missing Cleanup in Item.tsx (Low Priority)

Location: packages/framer-motion/src/components/Reorder/Item.tsx:111-114

The onDragEnd handler calls stopScroll(), but if the component unmounts during a drag, the scroll might not stop.

Suggestion: Add cleanup in a useEffect:

useEffect(() => {
    return () => stopScroll && stopScroll()
}, [stopScroll])

5. Unit Conversion Test (Note)

Location: packages/framer-motion/cypress/integration/unit-conversion.ts:24

There's an unclosed comment block starting at line 24. This appears to be a formatting issue:

     /*
     * When animating from a calc() with CSS variables...

Should be:

    /**
     * When animating from a calc() with CSS variables...
     */

🧪 Test Plan Completeness

The PR description mentions:

• Unit tests for auto-scroll utility pass
• Manual testing with scrollable container
• Verify existing Reorder functionality unchanged

Suggestion: Consider adding a Cypress/Playwright E2E test that:

• Creates a scrollable Reorder.Group
• Simulates dragging to the edge
• Verifies the container actually scrolls

📊 Overall Assessment

Quality: ⭐⭐⭐⭐ (4/5)
Security: ✅ No concerns
Performance: ⚠️ Could be optimized with RAF
Breaking Changes: ✅ None - fully backward compatible

This is a solid implementation that's ready to merge with minor refinements. The main suggestion is to use requestAnimationFrame for smoother scrolling, but the current implementation is functional and well-tested.

Great work! 🎉

[claude]

Code Review

I've reviewed PR #3446 which adds auto-scroll support to Reorder.Group. Overall, this is a well-implemented feature with good test coverage. Here's my detailed feedback:

✅ Strengths

1. Good test coverage: Comprehensive unit tests for the auto-scroll utility covering vertical/horizontal scrolling, custom options, and edge cases
2. Clean API design: The scrollParent and scrollOptions props are intuitive and well-documented
3. Non-breaking change: Existing functionality remains unchanged; feature is opt-in
4. Performance-aware: Uses useMemo to avoid recreating the auto-scroll instance unnecessarily
5. Code style: Follows repository conventions (interfaces, named exports, arrow functions)

🐛 Critical Issues

1. Memory Leak: Missing cleanup for scroll RAF loop

The updateScroll function is called repeatedly during drag operations, but there's no cleanup mechanism. If a user drags and releases quickly, or if the component unmounts during a drag, there's no useEffect cleanup to stop the scroll animation.

Impact: Potential memory leak, especially if users rapidly drag items or navigate away during drag operations.

Recommendation: Add cleanup logic in Group.tsx:

useEffect(() => {
    return () => {
        autoScroll?.stop()
    }
}, [autoScroll])

2. Dependency array issue in useMemo

In Group.tsx:142-145, the dependency array for useMemo is:

const autoScroll = useMemo(
    () => (scrollParent ? createAutoScroll(axis, scrollOptions) : null),
    [axis, scrollParent, scrollOptions]
)

Problem: scrollOptions is an object that will cause the memoization to break on every render if the parent doesn't memoize it.

Recommendation: Either:

• Destructure and depend on individual options: [axis, scrollParent, scrollOptions?.threshold, scrollOptions?.maxSpeed]
• Document that users should memoize scrollOptions
• Use useRef instead of useMemo if recreation isn't expensive

3. Missing null safety in test at exact boundary

In auto-scroll.test.ts:143, the test "should handle pointer exactly at threshold boundary" expects scrollTop to be 0, but the implementation uses < (not <=) for the threshold check:

if (distanceFromStart < threshold) {
    // Will NOT trigger when distanceFromStart === threshold
}

Issue: Test passes by accident. When pointerPosition = 50 and threshold = 50, distanceFromStart = 50, so 50 < 50 is false, resulting in 0 scroll. But the test comment says "Should not scroll when at exact boundary" - this is implementation-dependent behavior that should be explicitly documented.

Recommendation: Either:

• Add a comment explaining the boundary behavior (< vs <=)
• Update test description to be more precise about the behavior

⚠️ Moderate Issues

4. Type safety: scrollParent doesn't validate element is scrollable

The scrollParent ref could point to an element without overflow: auto/scroll. The code won't crash but also won't work as expected.

Recommendation: Add a dev-mode warning using the existing invariant utility:

if (process.env.NODE_ENV !== "production" && scrollParent?.current) {
    const computed = getComputedStyle(scrollParent.current)
    const overflow = axis === "y" ? computed.overflowY : computed.overflowX
    invariant(
        overflow === "auto" || overflow === "scroll",
        `scrollParent element must have overflow-${axis}: auto or scroll`
    )
}

5. Performance: Direct DOM manipulation during drag

auto-scroll.ts:63-66 directly manipulates scrollTop/scrollLeft on every drag event. This could cause layout thrashing if many items are being reordered.

Current impact: Likely minimal for typical use cases, but could be noticeable with very long lists.

Future consideration: Consider using requestAnimationFrame for smoother scrolling, or batch scroll updates.

6. Missing TypeScript strictness

In Group.tsx:175-177, the optional chaining on scrollParent?.current is good, but handleScroll callback is created even when autoScroll is null (if no scrollParent provided).

Minor optimization: The ternary could be simplified since autoScroll is already null when scrollParent is undefined.

💡 Suggestions

7. Scroll easing

The Math.pow(intensity, 2) in auto-scroll.ts:43,47 provides quadratic easing, which is good. However, this is hardcoded.

Enhancement idea: Consider making the easing function customizable in scrollOptions for advanced users.

8. Test gap: Missing integration tests

Unit tests are excellent, but there are no integration tests showing the feature working end-to-end with actual Reorder.Group components.

Recommendation: Add at least one test in the Reorder component test suite (if it exists) or add to manual test plan checklist.

9. Documentation: Missing TypeScript example

The JSDoc example in Group.tsx:72-88 is great, but it's vanilla JSX. Consider adding a note about TypeScript usage since the repo uses TS.

10. onDragEnd forwarding preserved

Good catch on Item.tsx:104-107 ensuring onDragEnd is properly forwarded while adding stopScroll. This maintains backward compatibility. ✅

🔒 Security

No security concerns identified. The feature:

• Doesn't introduce XSS risks
• Doesn't expose user data
• Uses refs safely
• Validates inputs appropriately

📊 Performance

• Scroll calculations are O(1) and efficient
• useMemo usage is appropriate (with the caveat in issue Fixing types and es references #2)
• No unnecessary re-renders introduced
• Direct DOM manipulation is minimal and scoped

✅ Test Plan Status

From the PR description:

• ✅ Unit tests pass
• ⏳ Manual testing needed
• ⏳ Verify existing Reorder functionality unchanged

Recommendation: Before merging, run the existing Reorder test suite to ensure no regressions.

📝 Summary

This is a solid implementation of a frequently requested feature. The main concern is the potential memory leak from missing cleanup (issue #1), which should be addressed before merging. The dependency array issue (#2) could cause performance problems and should also be fixed.

The other issues are lower priority but worth considering for code quality and maintainability.

Recommendation: Request changes for issues #1 and #2, then approve with suggestions for the rest.

---

Great work overall! The test coverage and API design are particularly well done. 🎯

[claude]

Code Review: Auto-scroll Support for Reorder.Group

Thank you for implementing this highly requested feature! The implementation is clean and well-structured. Here's my detailed feedback:

---

✅ Strengths

1. Clean Architecture

• The auto-scroll logic is properly isolated in a separate utility module (auto-scroll.ts)
• Good separation of concerns between scroll behavior and component logic
• Optional props design maintains backward compatibility

2. Excellent Test Coverage

• Comprehensive unit tests covering vertical/horizontal scrolling, custom options, and edge cases
• Tests verify both directional scrolling and speed intensity
• Edge case handling (null elements, boundary conditions) is properly tested

3. Good Developer Experience

• Clear JSDoc documentation with usage examples
• Intuitive API design with scrollParent ref pattern
• Sensible defaults (threshold: 50px, maxSpeed: 25px/frame)

---

🔍 Issues & Recommendations

Critical: Scroll Implementation Flaw

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:54-68

The current implementation directly mutates scroll position on every drag event, which has several problems:

1. Scroll happens on every onDrag call - This could fire many times per frame during a drag operation, causing jittery/inconsistent scrolling
2. No smooth animation - Direct scrollTop += creates abrupt jumps rather than smooth scrolling
3. Performance concerns - Repeatedly calling getBoundingClientRect() on every drag event can be expensive

Recommended approach:

• Use requestAnimationFrame (RAF) loop that continues while dragging near edges
• Only call getBoundingClientRect once when starting scroll (or use cached values)
• Implement smooth scrolling using scrollBy with behavior: 'smooth' or manual RAF-based animation

Example pattern:

let scrollRafId: number | null = null

function startScrolling(amount: number, element: HTMLElement) {
  if (scrollRafId \!== null) return // already scrolling
  
  function scroll() {
    if (axis === 'x') {
      element.scrollLeft += amount
    } else {
      element.scrollTop += amount
    }
    scrollRafId = requestAnimationFrame(scroll)
  }
  scrollRafId = requestAnimationFrame(scroll)
}

function stop() {
  if (scrollRafId \!== null) {
    cancelAnimationFrame(scrollRafId)
    scrollRafId = null
  }
}

---

High Priority: useMemo Dependencies

Location: packages/framer-motion/src/components/Reorder/Group.tsx:142-145

const autoScroll = useMemo(
    () => (scrollParent ? createAutoScroll(axis, scrollOptions) : null),
    [axis, scrollParent, scrollOptions]
)

Issue: scrollOptions is an object passed as a prop. If the parent component doesn't memoize it, this will recreate the autoScroll instance on every render even when the options haven't meaningfully changed.

Recommendations:

1. Document the requirement: Add JSDoc note that scrollOptions should be memoized or use stable references
2. Destructure dependencies: Use scrollOptions?.threshold and scrollOptions?.maxSpeed in the dependency array
3. Alternative: Spread scrollOptions in dependencies: [axis, scrollParent, scrollOptions?.threshold, scrollOptions?.maxSpeed]

---

Medium: Type Safety

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:27-30

The getScrollAmount function receives a scrollElement: HTMLElement parameter but updateScroll accepts HTMLElement | null. The null check happens in updateScroll, which is correct, but consider these improvements:

1. Export the interface for external use:

export interface AutoScrollController {
    updateScroll: (pointerPosition: number, scrollElement: HTMLElement | null) => void
    stop: () => void
}

2. Add return type annotation: Explicitly type the return of createAutoScroll for better IDE support

---

Low Priority: Minor Improvements

1. Redundant Stop Function

packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:71-73

The stop() function is currently a no-op. If it won't be needed for the RAF-based approach either, consider:

• Removing it entirely and making stopScroll optional in the context
• Or documenting why it exists (for future extensibility)

2. Magic Numbers

The quadratic easing (Math.pow(intensity, 2)) is hardcoded. Consider making the easing function configurable:

export interface AutoScrollOptions {
    threshold?: number
    maxSpeed?: number
    easingPower?: number // default: 2
}

3. Testing Gap

While unit tests are excellent, consider adding:

• Integration test with actual Reorder.Group/Item components
• Test that scrolling stops on drag end
• Test for continuous scrolling (if implementing RAF approach)

---

🔒 Security

No security concerns identified. The implementation:

• Safely handles null refs
• Doesn't execute user-provided code
• Has proper bounds checking

---

⚡ Performance

Current implementation has room for optimization:

• ✅ Good: useMemo prevents recreating autoScroll unnecessarily
• ⚠️ Concern: getBoundingClientRect() on every drag event (fixed by RAF approach)
• ⚠️ Concern: Potential layout thrashing from rapid scroll mutations
• ✅ Good: Conditional context methods avoid overhead when not using scroll

---

📝 Test Plan Completion

From PR description, remaining tasks:

• Manual testing with scrollable container
• Verify existing Reorder functionality unchanged

Suggestions:

1. Add a test/demo in dev/react/src/examples/ showcasing the feature
2. Test with various scroll container scenarios:
   • Nested scroll containers
   • Container with padding/borders
   • CSS transforms on scroll container
   • Very small containers (< threshold * 2)

---

🎯 Summary

This is a solid implementation of a requested feature with good code organization and test coverage. The main concern is the scroll implementation approach which could cause performance and UX issues. Implementing a RAF-based continuous scroll will provide a much smoother user experience.

Recommendation: Address the critical scroll implementation issue before merging, as it affects the core functionality's smoothness and performance.

Great work overall! 🚀

[claude]

Code Review

Thank you for implementing auto-scroll support for Reorder.Group! This is a valuable feature that addresses issue #1339. Overall, the implementation is solid and well-tested. Here's my detailed feedback:

---

✅ Strengths

1. Clean API Design: The scrollParent ref pattern is intuitive and consistent with React conventions
2. Comprehensive Unit Tests: Excellent test coverage for the auto-scroll utility (150 lines of tests)
3. Configurable Options: The scrollOptions prop provides good flexibility with sensible defaults
4. Proper Memoization: Using useMemo for the autoScroll instance prevents unnecessary re-creates
5. Code Style Compliance: Follows the repo's conventions (interfaces, named exports, arrow callbacks)

---

🐛 Critical Issues

1. Memory Leak: Missing Cleanup in stop()

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:71

The stop() function is currently a no-op, but the auto-scroll utility should clean up any ongoing scroll operations. If updateScroll is called repeatedly (on every drag event), there's no mechanism to stop the scrolling when dragging ends.

Issue: While the current implementation doesn't use requestAnimationFrame, if this were to be optimized in the future to use RAF for smoother scrolling, the lack of cleanup would cause a memory leak.

Recommendation: Consider whether continuous scrolling (via RAF) would provide a smoother experience:

let rafId: number | null = null

function updateScroll(pointerPosition: number, scrollElement: HTMLElement | null) {
    if (!scrollElement) return
    
    const scrollAmount = getScrollAmount(pointerPosition, scrollElement)
    
    if (scrollAmount !== 0) {
        // Cancel any existing animation
        if (rafId !== null) {
            cancelAnimationFrame(rafId)
        }
        
        const scroll = () => {
            if (axis === 'x') {
                scrollElement.scrollLeft += scrollAmount
            } else {
                scrollElement.scrollTop += scrollAmount
            }
            rafId = requestAnimationFrame(scroll)
        }
        scroll()
    } else {
        stop()
    }
}

function stop() {
    if (rafId !== null) {
        cancelAnimationFrame(rafId)
        rafId = null
    }
}

---

2. Missing E2E Tests

Location: Test plan in PR description

The PR has comprehensive unit tests but lacks E2E tests for the actual user interaction. According to CLAUDE.md:

"For UI interaction bugs (like gesture handling), prefer E2E tests using Playwright or Cypress."

Recommendation: Add a Playwright test that:

• Sets up a scrollable container with Reorder.Group
• Simulates dragging an item near the edge
• Verifies the container actually scrolls
• Tests both top/bottom edges (or left/right for horizontal)

This would catch integration issues that unit tests might miss, such as coordinate system mismatches.

---

⚠️ Potential Bugs

3. Coordinate System Mismatch Risk

Location: packages/framer-motion/src/components/Reorder/Item.tsx:107

The pointerPoint[axis] passed to handleScroll might be in a different coordinate space than what getBoundingClientRect() returns.

Analysis:

• gesturePoint.point is typically in page/viewport coordinates
• getBoundingClientRect() returns viewport-relative coordinates
• If there's page scrolling, these might not align

Recommendation: Verify the coordinate system consistency. Consider adding a comment explaining the coordinate space assumptions:

// pointerPoint is in viewport coordinates, matching getBoundingClientRect()
handleScroll && handleScroll(pointerPoint[axis])

---

4. Quadratic Easing May Be Too Aggressive

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:43,47

return -maxSpeed * Math.pow(intensity, 2)

Using Math.pow(intensity, 2) creates a quadratic ease that accelerates very quickly. This might feel too aggressive for users.

Recommendation: Consider testing with linear easing first (intensity * maxSpeed) or offering easing as a configuration option. The quadratic curve means:

• At 50% into threshold: only 25% speed
• At 10% into threshold: only 1% speed
• Near edge: 100% speed (very fast jump)

---

🔍 Code Quality Issues

5. TypeScript: Stricter Type for scrollParent

Location: packages/framer-motion/src/components/Reorder/Group.tsx:94

scrollParent?: RefObject<HTMLElement | null>

The | null is redundant since RefObject already allows .current to be null.

Recommendation:

scrollParent?: RefObject<HTMLElement>

This is cleaner and still type-safe since refs can always be null.

---

6. Boundary Condition in Tests

Location: packages/framer-motion/src/components/Reorder/utils/__tests__/auto-scroll.test.ts:142

// Pointer exactly at threshold (50px from top)
autoScroll.updateScroll(50, element)

// Should not scroll when at exact boundary
expect(element.scrollTop).toBe(0)

The test comment says "exactly at threshold (50px from top)" but the pointer position is absolute (50), not relative to the container edge.

Issue: If the container starts at top: 0, then position 50 is actually 50px from the top, which should trigger scrolling based on the implementation (distanceFromStart < threshold where threshold is 50).

Recommendation: Fix the test logic or clarify the expected behavior:

// Pointer exactly at threshold boundary (50px from top of container at y=0)
autoScroll.updateScroll(50, element)

// distanceFromStart = 50 - 0 = 50, which is NOT < 50, so no scroll
expect(element.scrollTop).toBe(0)

Or adjust to test just inside the threshold:

autoScroll.updateScroll(49, element) // 49 < 50, should scroll
expect(element.scrollTop).toBeLessThan(0)

---

7. Missing JSDoc for Public Interface

Location: packages/framer-motion/src/components/Reorder/utils/auto-scroll.ts:21-23

The createAutoScroll function is exported but lacks JSDoc comments explaining its purpose and return value.

Recommendation:

/**
 * Creates an auto-scroll controller for a draggable reorder list.
 * Scrolls the container when the pointer is near its edges during drag.
 * 
 * @param axis - The scroll axis ('x' for horizontal, 'y' for vertical)
 * @param options - Configuration for threshold distance and scroll speed
 * @returns Controller with updateScroll and stop methods
 */
export function createAutoScroll(
    axis: "x" | "y",
    options: AutoScrollOptions = {}
)

---

🚀 Performance Considerations

8. Every onDrag Event Triggers Scroll Calculation

Location: packages/framer-motion/src/components/Reorder/Item.tsx:102-109

The onDrag callback runs on every mouse/touch move event, calling handleScroll each time. While the calculations are simple, they could be optimized.

Impact: Low - the calculations are minimal (getBoundingClientRect + arithmetic)

Optional Optimization: Throttle scroll updates using RAF:

let rafScheduled = false

function updateScroll(pointerPosition: number, scrollElement: HTMLElement | null) {
    if (rafScheduled || !scrollElement) return
    
    rafScheduled = true
    requestAnimationFrame(() => {
        rafScheduled = false
        const scrollAmount = getScrollAmount(pointerPosition, scrollElement)
        // ... apply scroll
    })
}

However, this adds complexity for minimal gain. Only worth it if performance issues are observed.

---

🔒 Security

No security concerns identified. The implementation:

• ✅ Doesn't execute arbitrary code
• ✅ Doesn't inject HTML
• ✅ Uses safe DOM APIs (scrollTop, scrollLeft)
• ✅ Validates inputs appropriately

---

📝 Minor Suggestions

9. useMemo Dependency Array

Location: packages/framer-motion/src/components/Reorder/Group.tsx:142-145

const autoScroll = useMemo(
    () => (scrollParent ? createAutoScroll(axis, scrollOptions) : null),
    [axis, scrollParent, scrollOptions]
)

Issue: scrollOptions is an object, so this will recreate on every render unless memoized by the parent.

Recommendation: Either:

1. Document that scrollOptions should be memoized/stable
2. Use individual option values in the dependency array:

   [axis, scrollParent, scrollOptions?.threshold, scrollOptions?.maxSpeed]

3. Add a note in the JSDoc about memoization

---

10. Test: Mock RAF for Deterministic Tests

Location: packages/framer-motion/src/components/Reorder/utils/__tests__/auto-scroll.test.ts

The tests directly modify scrollTop/scrollLeft synchronously, which works for the current implementation but would break if RAF is added later.

Recommendation: Consider using jest.useFakeTimers() or mocking RAF upfront to make tests more resilient to implementation changes.

---

📋 Summary

Category	Count
Critical Issues	2
Potential Bugs	2
Code Quality	3
Performance	1
Minor Suggestions	2
Security	0

---

✅ Approval Recommendation

This PR is nearly ready to merge after addressing:

1. Must Fix:

   • Add E2E tests (per CLAUDE.md guidelines)
   • Fix boundary condition test or clarify expected behavior

2. Should Fix:

   • Consider RAF for smoother scrolling + proper cleanup in stop()
   • Verify coordinate system consistency
   • Test the quadratic easing feel (may want linear)

3. Nice to Have:

   • Add JSDoc to createAutoScroll
   • Document scrollOptions memoization requirement
   • Clean up TypeScript types

Great work overall! The feature implementation is clean and the unit tests are thorough. Once E2E tests are added and the boundary behavior is clarified, this will be a solid addition to the library. 🎉

[claude]

PR Review: Add auto-scroll support to Reorder.Group

Thank you for this feature implementation! This addresses a long-requested feature (#1339) and the implementation is generally solid. Here's my detailed review:

✅ Strengths

1. Clean API Design: The scrollParent ref pattern is idiomatic React and the optional scrollOptions provides good customization
2. Comprehensive Unit Tests: Excellent test coverage for the auto-scroll utility (150 lines of tests)
3. Good Documentation: JSDoc comments with usage examples are very helpful
4. Non-breaking: This is a purely additive change with no impact on existing functionality

🐛 Potential Bugs & Issues

Critical: Performance Issue

Location: auto-scroll.ts:54-68

The auto-scroll is triggered on every onDrag event, which fires continuously during dragging. This causes:

• getBoundingClientRect() to be called on every frame (expensive DOM operation)
• Direct DOM manipulation (scrollTop/scrollLeft) on every drag event

Impact: This could cause performance degradation, especially on lower-end devices or with complex layouts.

Recommendation: Use requestAnimationFrame throttling or the existing frameloop system. According to CLAUDE.md, the codebase uses time.now() from motion-dom/src/frameloop/sync-time.ts for frame-synced operations. Consider integrating with the existing frame loop:

import { frame } from "motion-dom"

// In createAutoScroll, track if scrolling is active
let rafId: number | null = null

function updateScroll(pointerPosition: number, scrollElement: HTMLElement | null) {
    if (!scrollElement) return
    
    // Cancel previous frame if exists
    if (rafId !== null) {
        cancelAnimationFrame(rafId)
    }
    
    rafId = requestAnimationFrame(() => {
        const scrollAmount = getScrollAmount(pointerPosition, scrollElement)
        if (scrollAmount !== 0) {
            if (axis === "x") {
                scrollElement.scrollLeft += scrollAmount
            } else {
                scrollElement.scrollTop += scrollAmount
            }
        }
        rafId = null
    })
}

function stop() {
    if (rafId !== null) {
        cancelAnimationFrame(rafId)
        rafId = null
    }
}

Medium: useMemo Dependency Issue

Location: Group.tsx:142-145

const autoScroll = useMemo(
    () => (scrollParent ? createAutoScroll(axis, scrollOptions) : null),
    [axis, scrollParent, scrollOptions]
)

Issue: scrollOptions is an object, so this will recreate the auto-scroll instance on every render if the object reference changes (even with the same values). This negates the benefits of useMemo.

Recommendation: Either:

1. Destructure the options in dependencies:

const { threshold, maxSpeed } = scrollOptions || {}
const autoScroll = useMemo(
    () => (scrollParent ? createAutoScroll(axis, { threshold, maxSpeed }) : null),
    [axis, scrollParent, threshold, maxSpeed]
)

2. Use a ref for the auto-scroll instance and only update when necessary

Minor: Missing Cleanup

Location: Group.tsx:142-145

The autoScroll instance is created in useMemo but there's no cleanup when the component unmounts or when the instance changes. While the current stop() implementation is a no-op, if you implement the RAF solution above, you'll need proper cleanup:

useEffect(() => {
    return () => {
        autoScroll?.stop()
    }
}, [autoScroll])

🎯 Code Quality & Best Practices

Good Practices

• ✅ Named exports (follows CLAUDE.md guidelines)
• ✅ Interface for type definitions (follows CLAUDE.md)
• ✅ Optional chaining for safe property access
• ✅ Exponential easing (Math.pow(intensity, 2)) for smooth acceleration

Suggestions

1. Test boundary at threshold: The test at line 142 checks updateScroll(50, element) and expects no scroll, but the condition at auto-scroll.ts:41 is distanceFromStart < threshold, which means 50 is correctly excluded. However, consider testing 49 to verify scrolling does occur just inside the threshold.

2. Consider scroll limits: The implementation doesn't check if the container has reached its scroll limits. This means it will keep trying to scroll even when at max/min scroll position. Consider adding:

function updateScroll(pointerPosition: number, scrollElement: HTMLElement | null) {
    if (!scrollElement) return
    
    const scrollAmount = getScrollAmount(pointerPosition, scrollElement)
    
    if (scrollAmount !== 0) {
        if (axis === "x") {
            const maxScroll = scrollElement.scrollWidth - scrollElement.clientWidth
            const newScroll = Math.max(0, Math.min(maxScroll, scrollElement.scrollLeft + scrollAmount))
            scrollElement.scrollLeft = newScroll
        } else {
            const maxScroll = scrollElement.scrollHeight - scrollElement.clientHeight
            const newScroll = Math.max(0, Math.min(maxScroll, scrollElement.scrollTop + scrollAmount))
            scrollElement.scrollTop = newScroll
        }
    }
}

3. Add E2E test: Per CLAUDE.md: "For UI interaction bugs (like gesture handling), prefer E2E tests using Playwright or Cypress." Consider adding a Playwright test in the test plan that actually drags an item near the edge of a scrollable container and verifies scrolling occurs. The current tests are good unit tests, but don't test the integration with the actual Reorder component.

🔒 Security Concerns

No security issues identified. The implementation:

• Doesn't execute arbitrary code
• Doesn't expose sensitive data
• Uses safe DOM APIs
• Properly validates input (null checks)

📊 Test Coverage

Unit Tests: ✅ Excellent coverage of the auto-scroll utility

Missing Tests:

• ❌ Integration test with actual Reorder.Group component
• ❌ E2E test with real drag interactions
• ❌ Test for scroll limit behavior
• ❌ Test for performance/RAF throttling (if implemented)

Recommendation: Add a test in packages/framer-motion/src/components/Reorder/__tests__/index.test.tsx that verifies the handleScroll and stopScroll callbacks are properly passed through the context and called during drag events.

📝 Summary

This is a well-implemented feature with good documentation and test coverage. The main concerns are:

1. 🔴 Critical: Performance optimization needed for drag events
2. 🟡 Medium: Fix useMemo dependencies to prevent unnecessary re-creation
3. 🟢 Nice-to-have: Add E2E test, scroll limits, and cleanup effect

Overall: Approve with suggestions - The feature works but would benefit from the performance optimization before merging.

Great work on this feature! 🎉

[claude]

PR Review: Add auto-scroll support to Reorder.Group

Summary

This PR adds auto-scroll functionality to Reorder.Group, allowing automatic scrolling when dragging items near container edges. Overall, this is a well-structured implementation with good attention to edge cases.

---

🟢 Strengths

1. Clean Architecture: The auto-scroll logic is properly isolated in auto-scroll.ts, keeping concerns separated
2. Edge Case Handling: Good attention to preventing infinite scroll via initialScrollLimits tracking
3. Velocity-Based Activation: Smart use of velocity to determine when to start scrolling (lines 115-117 in auto-scroll.ts)
4. Scroll Anchoring Fix: The overflowAnchor: "none" addition (Group.tsx:152-155) is a thoughtful detail that prevents browser interference
5. E2E Tests: Cypress tests provide good coverage of the core auto-scroll behavior

---

🔴 Critical Issues

1. API Mismatch: Missing Advertised Props

The PR description shows scrollParent and scrollOptions props in the usage example, but these props are not implemented in the code. The implementation uses automatic ancestor detection instead.

Current behavior: Automatically finds scrollable ancestors
Advertised behavior: Accepts explicit scrollParent ref and scrollOptions

Recommendation: Either remove the props from the PR description and update the usage example, OR implement the props as advertised (preferred for flexibility)

2. Hard-Coded Configuration Values

Lines 1-2 in auto-scroll.ts have hard-coded constants (threshold = 50, maxSpeed = 25) with no way to customize. This limits user control over scroll behavior.

Recommendation: Expose these as optional props per the PR description.

---

🟡 Bugs & Issues

3. Non-Null Assertion Without Guard

Line 135 in auto-scroll.ts uses initialScrollLimits.get(scrollableAncestor)! which assumes the value exists, but it's only set when currentActiveEdge !== edge. If the edge switches mid-drag, this could be undefined.

4. Module-Level State in React Components

The module-level state variables (currentGroupElement, initialScrollLimits, activeScrollEdge) could cause issues with SSR, HMR, and multiple concurrent drag operations.

Recommendation: Consider moving this state into React context or refs managed by the component lifecycle.

---

🟡 Code Quality & Test Coverage

5. Missing Unit Tests

The PR description mentions "Unit tests for auto-scroll utility pass" ✅, but the diff shows check-reorder.test.ts was deleted (not replaced). Add unit tests for auto-scroll utilities.

6. E2E Test Limitations

• Only tests vertical (y-axis) scrolling
• Doesn't test horizontal (x-axis) scrolling
• Missing edge cases: nested scrollables, direction switching, scroll limits

7. Test Plan Incomplete

The PR description shows unchecked items for manual testing and verifying existing functionality.

---

⚡ Performance Considerations

8. Scroll Mutations on Every Drag Event

autoScrollIfNeeded() is called on every onDrag event, which means DOM walking and getBoundingClientRect() calls on every pointer move.

Recommendation: Cache the scrollable ancestor during drag start and consider using requestAnimationFrame for scroll updates.

---

📝 Minor Issues

9. Duplicate border-radius in test styles (lines 75 & 80)
10. Unclear comment change in VisualElementDragControls.ts

---

✅ Recommendations Summary

Before merging:

1. Fix critical API mismatch: implement scrollParent and scrollOptions props or update docs
2. Fix non-null assertion bug (line 135)
3. Add unit tests for auto-scroll utilities
4. Restore deleted check-reorder.test.ts or explain removal
5. Complete manual testing checklist items
6. Add E2E tests for horizontal scrolling

Nice to have:
7. Performance optimizations (caching, RAF throttling)
8. Move module-level state into component-managed state

---

Overall Assessment

This is a solid implementation of a useful feature, but it has a critical mismatch between the advertised API and actual implementation. The code quality is good, but test coverage needs improvement.

Recommendation: Request changes to address the API mismatch and missing tests before merging.

Great work on the velocity-based activation logic and scroll anchoring fix! 🚀

[claude]

Pull Request Review: Auto-scroll support for Reorder.Group

Thank you for this feature addition! Overall, this is a well-implemented feature that addresses issue #1339. Below is my detailed review:

Strengths

1. Clean Implementation: The auto-scroll logic is well-encapsulated in a separate utility module with clear separation of concerns.

2. Smart Edge Detection: The velocity-based edge activation (lines 115-118 in auto-scroll.ts) is a nice touch - it prevents accidental scrolling when just hovering near an edge.

3. Infinite Scroll Prevention: The initialScrollLimits tracking prevents the container from growing infinitely during drag operations (lines 123-130, 133-141 in auto-scroll.ts).

4. Scroll Anchoring Fix: Adding overflowAnchor: none in Group.tsx:152-155 is excellent - this prevents browser scroll anchoring from interfering with drag calculations.

5. State Cleanup: Proper cleanup on drag end with resetAutoScrollState() in Item.tsx:122.

---

Issues and Concerns

Critical Issues

1. Missing API from PR Description

The PR description mentions scrollParent and scrollOptions props, but these are NOT implemented in the actual code. The implementation uses hardcoded values and auto-detects the scroll parent. This is a significant discrepancy between documentation and implementation.

Recommendation: Either implement the mentioned props OR update the PR description to accurately reflect the auto-detection behavior.

2. Non-Configurable Constants

The threshold and maxSpeed are hardcoded at auto-scroll.ts:1-2. These should either be made configurable via props (as the PR description suggests) OR documented as intentionally fixed values with rationale.

3. Module-Level State with Multiple Groups

The currentGroupElement at auto-scroll.ts:14 is a module-level singleton. This could cause issues when multiple Reorder.Group components exist on the same page or when rapidly switching between dragging items in different groups.

Recommendation: Move state into React context or pass group identity explicitly.

---

High Priority Issues

4. Missing Unit Tests

The PR description mentions Unit tests for auto-scroll utility pass with a checkmark, but there are no unit test files for the auto-scroll utility. Only E2E Cypress tests exist, which are slower to run, dont cover edge cases, and dont test the utility functions in isolation.

Recommendation: Add unit tests for findScrollableAncestor(), getScrollAmount(), and autoScrollIfNeeded().

5. Potential Null Pointer with Non-null Assertion

At auto-scroll.ts:135, the non-null assertion assumes the limit was set, but if the WeakMap was cleared or the flow changed, this could throw. A defensive check would be safer.

---

Medium Priority Issues

6. Incomplete Test Coverage

The E2E tests only test scrolling down near bottom and scrolling up near top. Missing test cases include horizontal axis, multiple nested scrollable containers, rapid velocity changes, edge transition, and overflow: scroll vs overflow: auto.

7. Hardcoded Intensity Calculation

The quadratic easing (intensity * intensity) is hardcoded. This might feel too slow at the edges for some use cases. Consider linear easing option, cubic for more aggressive acceleration, or making this configurable.

8. Accessibility Consideration

Auto-scrolling during drag can be disorienting for users with vestibular disorders. Consider respecting prefers-reduced-motion media query and adding scrollOptions.enabled flag for opt-out.

9. Performance: Scroll Called Every Frame

During drag, autoScrollIfNeeded is called on every frame. Each call runs getBoundingClientRect() and potentially modifies scrollTop/scrollLeft. For complex layouts, this could cause performance issues. Consider throttling, batching, or using requestAnimationFrame.

---

Low Priority Issues

10. TypeScript: Loose Type in ReorderGroupComponent

Group.tsx:84 uses React.ForwardedRef - could be more specific.

11. Duplicate border-radius in Test Styles

dev/react/src/tests/reorder-auto-scroll.tsx:74-80 has border-radius defined twice.

12. Inconsistent Reset Logic

resetAutoScrollState() tries to clean up both x and y axes, even though each drag operation only uses one axis. This works but is slightly inefficient.

---

Security

No security concerns identified.

---

Test Plan Status

From the PR description:

• Unit tests for auto-scroll utility pass - FALSE: No unit tests exist
• Manual testing with scrollable container - Not verified in review
• Verify existing Reorder functionality unchanged - Should add regression tests

---

Recommendations Summary

Must Fix Before Merge:

1. Fix PR description to match actual implementation (no scrollParent/scrollOptions props)
2. Add unit tests for auto-scroll utility functions
3. Fix module-level state issue with currentGroupElement
4. Address null safety at auto-scroll.ts:135

Should Fix:

5. Add horizontal axis test coverage
6. Make threshold/maxSpeed configurable or document why they are fixed
7. Add prefers-reduced-motion support
8. Add more comprehensive E2E tests

Nice to Have:

9. Consider performance optimizations (throttling)
10. Improve TypeScript types
11. Fix minor style issues in test app

---

Conclusion

This is a valuable feature with solid core implementation. The main concerns are:

1. Documentation/implementation mismatch
2. Missing unit tests
3. Potential multi-group issues

With these addressed, this would be a great addition to the library. Great work on the velocity-based activation and scroll anchor fix!

---

Review generated by Claude Code