Fix drag-and-drop memory leak and reordering with variable item heights

packages/core/src/controlledEnvironment/layoutUtils.ts

@@ -10,14 +10,12 @@ export const computeItemHeight = (treeId: string) => {
 export const computeItemHeightArray = (treeId: string): number[] => {
   const document = getDocument();
   if (!document) {
-    console.log("Document not found");
     return [];
   }
-  const items = document.querySelectorAll<HTMLElement>(`[data-rct-tree="${treeId}"] [data-rct-item-container="true"]`);
-  const itemHeights = Array.from(items).map(item => item.offsetHeight);
-
-  console.log({ itemHeights });
-  return itemHeights;
+  const items = document.querySelectorAll<HTMLElement>(
+    `[data-rct-tree="${treeId}"] [data-rct-item-container="true"]`
+  );
+  return Array.from(items, item => item.offsetHeight);
 };
 
 export const isOutsideOfContainer = (e: DragEvent, treeBb: DOMRect) =>

packages/core/src/drag/useDraggingPosition.ts

@@ -65,27 +65,7 @@ export const useDraggingPosition = () => {
         return undefined;
       }
 
-      const clientYRelativeToTreeTop = e.clientY - treeBb.top;
-      let cumulativeHeight = 0;
-      let linearIndex = 0;
-      let hoveringPosition = 0;
-
-      for (let i = 0; i < itemsHeightArray.current.length; i++) {
-        cumulativeHeight += itemsHeightArray.current[i];
-        if (clientYRelativeToTreeTop <= cumulativeHeight) {
-          linearIndex = i;
-          // Calculate hovering position as a fraction within the current item
-          const previousItemsHeight = cumulativeHeight - itemsHeightArray.current[i];
-          hoveringPosition = linearIndex + (clientYRelativeToTreeTop - previousItemsHeight) / itemsHeightArray.current[i];
-          break;
-        }
-      }
-
       const treeLinearItems = env.linearItems[treeId];
-      // const linearIndexx = Math.min(
-      //   Math.max(0, Math.floor(hoveringPosition)),
-      //   treeLinearItems.length - 1
-      // );
 
       if (treeLinearItems.length === 0) {
         return {
@@ -95,6 +75,43 @@ export const useDraggingPosition = () => {
         };
       }
 
+      // Map the pointer's vertical position onto a fractional "linear index"
+      // using each item's *measured* height, so dragging tracks correctly even
+      // when items have different sizes. The integer part is the item index and
+      // the fractional part is how far the pointer is into that item (0 = top,
+      // approaching 1 = bottom).
+      const itemHeights = itemsHeightArray.current;
+      const clientYRelativeToTreeTop = e.clientY - treeBb.top;
+
+      let hoveringPosition = 0;
+      let cumulativeHeight = 0;
+      let matchedItem = false;
+
+      for (let i = 0; i < itemHeights.length; i += 1) {
+        const height = itemHeights[i] || 0;
+        const withinItem =
+          height > 0 && clientYRelativeToTreeTop < cumulativeHeight + height;
+        if (withinItem) {
+          hoveringPosition =
+            i + (clientYRelativeToTreeTop - cumulativeHeight) / height;
+          matchedItem = true;
+          break;
+        }
+        cumulativeHeight += height;
+      }
+
+      if (!matchedItem) {
+        // Pointer is below the last item (e.g. dragging into empty space at the
+        // bottom of the tree). Position it past the end so the "drop at the very
+        // bottom" detection below resolves to a bottom offset.
+        hoveringPosition = treeLinearItems.length;
+      }
+
+      const linearIndex = Math.min(
+        Math.max(0, Math.floor(hoveringPosition)),
+        treeLinearItems.length - 1
+      );
+
       const targetLinearItem = treeLinearItems[linearIndex];
       const targetItem = env.items[targetLinearItem.item];
 
@@ -168,7 +185,6 @@ export const useDraggingPosition = () => {
       dragCode.current = 'initial';
       itemHeight.current = computeItemHeight(treeId);
       itemsHeightArray.current = computeItemHeightArray(treeId);
-
     }
   );
 
@@ -177,7 +193,6 @@ export const useDraggingPosition = () => {
     dragCode.current = 'initial';
     itemHeight.current = 0;
     itemsHeightArray.current = [0];
-
   });
 
   return {

packages/core/src/tree/DragBetweenLine.tsx

@@ -6,7 +6,7 @@ import { useDragAndDrop } from '../drag/DragAndDropProvider';
 export const DragBetweenLine: React.FC<{
   treeId: string;
 }> = ({ treeId }) => {
-  const { draggingPosition, itemHeight, itemsHeightArray } = useDragAndDrop();
+  const { draggingPosition, itemsHeightArray } = useDragAndDrop();
   const { renderers } = useTree();
 
   const shouldDisplay =
@@ -22,13 +22,19 @@ export const DragBetweenLine: React.FC<{
     onDragOver: e => e.preventDefault(), // Allow dropping
   };
 
+  // Offset the line by the summed heights of the items above it, so it lands at
+  // the correct spot even when items have different heights.
+  const lineOffset = itemsHeightArray
+    .slice(0, draggingPosition.linearIndex)
+    .reduce((acc, height) => acc + height, 0);
+
   return (
     <div
       style={{
         position: 'absolute',
         left: '0',
         right: '0',
-        top: `${itemsHeightArray.slice(0, draggingPosition?.linearIndex ?? 0).reduce((acc, height) => acc + height, 0)}px`,
+        top: `${lineOffset}px`,
       }}
     >
       {renderers.renderDragBetweenLine({

packages/core/test/helpers/TestUtil.tsx

@@ -15,13 +15,20 @@ import {
 import { buildTestTree } from './testTree';
 import {
   computeItemHeight,
+  computeItemHeightArray,
   isOutsideOfContainer,
 } from '../../src/controlledEnvironment/layoutUtils';
 import '@testing-library/jest-dom';
 
 jest.mock('../../src/controlledEnvironment/layoutUtils');
 
 (computeItemHeight as jest.Mock).mockReturnValue(10);
+// jsdom reports every offsetHeight as 0, so mock uniform 10px item heights to
+// match the clientY math in dragOver (itemIndex * 10 + offset). The array is
+// intentionally longer than any test tree so every drag target is covered.
+(computeItemHeightArray as jest.Mock).mockReturnValue(
+  Array.from({ length: 1000 }, () => 10)
+);
 (isOutsideOfContainer as jest.Mock).mockReturnValue(false);
 
 export class TestUtil {