From 0868d31475dde44ae07fffb43e0ec57bc9981c4a Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 24 Jun 2026 00:43:15 +0000 Subject: [PATCH 1/3] fix(core): drag-and-drop memory leak and reordering with variable item heights Two issues in the dynamic-height drag-and-drop path: - Memory leak: computeItemHeightArray() logged its result object (and a "Document not found" string) on every dragover event, which fires continuously during a drag. Objects passed to console.log are retained by the browser console heap and can never be GC'd, so memory grew unbounded for the duration of each drag. Removed the hot-path logging. - Reordering didn't track when items have different sizes: the cumulative- height loop only assigned linearIndex/hoveringPosition when the pointer fell within an item, so dragging below the last item (into the tree's empty space) left both at 0 and snapped the drop indicator to the top. Rewrote it to compute a fractional linear index from each item's measured height, clamp linearIndex against the real linear-item list, and treat "below the last item" as past-the-end so the bottom-drop offset resolves correctly. Removed dead commented-out code left from the WIP. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01GtzjXm3E3o416rYjqWzEqv --- .../src/controlledEnvironment/layoutUtils.ts | 10 ++-- packages/core/src/drag/useDraggingPosition.ts | 57 ++++++++++++------- 2 files changed, 39 insertions(+), 28 deletions(-) diff --git a/packages/core/src/controlledEnvironment/layoutUtils.ts b/packages/core/src/controlledEnvironment/layoutUtils.ts index a8ce214d..3fff859e 100644 --- a/packages/core/src/controlledEnvironment/layoutUtils.ts +++ b/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(`[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( + `[data-rct-tree="${treeId}"] [data-rct-item-container="true"]` + ); + return Array.from(items, item => item.offsetHeight); }; export const isOutsideOfContainer = (e: DragEvent, treeBb: DOMRect) => diff --git a/packages/core/src/drag/useDraggingPosition.ts b/packages/core/src/drag/useDraggingPosition.ts index a267421b..19579c61 100644 --- a/packages/core/src/drag/useDraggingPosition.ts +++ b/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,41 @@ 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++) { + const height = itemHeights[i] || 0; + if (height > 0 && clientYRelativeToTreeTop < cumulativeHeight + height) { + 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 +183,6 @@ export const useDraggingPosition = () => { dragCode.current = 'initial'; itemHeight.current = computeItemHeight(treeId); itemsHeightArray.current = computeItemHeightArray(treeId); - } ); @@ -177,7 +191,6 @@ export const useDraggingPosition = () => { dragCode.current = 'initial'; itemHeight.current = 0; itemsHeightArray.current = [0]; - }); return { From 053e327df55972420917eda93083b3bf7d894621 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 24 Jun 2026 00:47:02 +0000 Subject: [PATCH 2/3] fix(core): satisfy lint (no-plusplus, unused itemHeight, prettier) - Use `i += 1` instead of `i++` in the hovering-position loop (airbnb no-plusplus) and hoist the loop condition into a named variable to keep lines within the 80-col prettier limit. - Drop the now-unused `itemHeight` from DragBetweenLine's destructure (it was left over when the line offset switched to itemsHeightArray) and extract the summed-height offset into a `lineOffset` variable. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01GtzjXm3E3o416rYjqWzEqv --- packages/core/src/drag/useDraggingPosition.ts | 6 ++++-- packages/core/src/tree/DragBetweenLine.tsx | 10 ++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/packages/core/src/drag/useDraggingPosition.ts b/packages/core/src/drag/useDraggingPosition.ts index 19579c61..deb55636 100644 --- a/packages/core/src/drag/useDraggingPosition.ts +++ b/packages/core/src/drag/useDraggingPosition.ts @@ -87,9 +87,11 @@ export const useDraggingPosition = () => { let cumulativeHeight = 0; let matchedItem = false; - for (let i = 0; i < itemHeights.length; i++) { + for (let i = 0; i < itemHeights.length; i += 1) { const height = itemHeights[i] || 0; - if (height > 0 && clientYRelativeToTreeTop < cumulativeHeight + height) { + const withinItem = + height > 0 && clientYRelativeToTreeTop < cumulativeHeight + height; + if (withinItem) { hoveringPosition = i + (clientYRelativeToTreeTop - cumulativeHeight) / height; matchedItem = true; diff --git a/packages/core/src/tree/DragBetweenLine.tsx b/packages/core/src/tree/DragBetweenLine.tsx index afc8fd1c..4b94c08a 100644 --- a/packages/core/src/tree/DragBetweenLine.tsx +++ b/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 (
acc + height, 0)}px`, + top: `${lineOffset}px`, }} > {renderers.renderDragBetweenLine({ From 3c26b762a54bd93cd236620fc86774c85538595d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 24 Jun 2026 00:54:25 +0000 Subject: [PATCH 3/3] test(core): mock computeItemHeightArray so drag tests use 10px item heights The dynamic-height refactor switched the drag math to computeItemHeightArray, but the test harness only mocked computeItemHeight. Under the auto-mocked layoutUtils module, computeItemHeightArray returned undefined, so reading its .length threw and every drag-and-drop test failed. Mock it to return uniform 10px heights (matching dragOver's clientY math of itemIndex * 10 + offset). With uniform heights the cumulative-height calculation is mathematically identical to the previous clientY / itemHeight formula, so behavior is unchanged. Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01GtzjXm3E3o416rYjqWzEqv --- packages/core/test/helpers/TestUtil.tsx | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/packages/core/test/helpers/TestUtil.tsx b/packages/core/test/helpers/TestUtil.tsx index 4c510584..0338c3ae 100644 --- a/packages/core/test/helpers/TestUtil.tsx +++ b/packages/core/test/helpers/TestUtil.tsx @@ -15,6 +15,7 @@ import { import { buildTestTree } from './testTree'; import { computeItemHeight, + computeItemHeightArray, isOutsideOfContainer, } from '../../src/controlledEnvironment/layoutUtils'; import '@testing-library/jest-dom'; @@ -22,6 +23,12 @@ 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 {