fix: rewrote onDrop handler to account for insertion order issues (#353)

Author: lukasbach

next-release-notes.md

@@ -1,9 +1,3 @@
-<!--
-### Breaking Changes
-
-### Features
-
 ### Bug Fixes and Improvements
-
-### Other Changes
--->
+- Fixed a bug where items where sometimes inserted with inverted order. (#353)
+  - The `onDrop` should be a bit faster in general, since some redundant calls to `dataProvider.onChangeItemChildren` have been removed. In the past, this was called twice per item, once for moving the item out of its old folder, and once for moving it into the target folder. Now, all insertion calls into the target folder are batched into one `dataProvider.onChangeItemChildren` call for all dragged items.

packages/core/src/uncontrolledEnvironment/UncontrolledTreeEnvironment.tsx

@@ -126,12 +126,11 @@ export const UncontrolledTreeEnvironment = React.forwardRef<
       }}
       onDrop={async (items, target) => {
         const promises: Promise<void>[] = [];
+        const itemsIndices = items.map(i => i.index);
+        let itemsPriorToInsertion = 0;
 
-        // when dropped between items, items are injected at top of insertion point each
-        const orderedItems =
-          target.targetType === 'between-items' ? [...items].reverse() : items;
-
-        for (const item of orderedItems) {
+        // move old items out
+        for (const item of items) {
           const parent = Object.values(currentItems).find(potentialParent =>
             potentialParent.children?.includes(item.index)
           );
@@ -146,67 +145,73 @@ export const UncontrolledTreeEnvironment = React.forwardRef<
             );
           }
 
-          if (target.targetType === 'item' || target.targetType === 'root') {
-            if (target.targetItem === parent.index) {
-              // NOOP
-            } else {
-              promises.push(
-                dataProvider.onChangeItemChildren(
-                  parent.index,
-                  parent.children.filter(child => child !== item.index)
-                )
-              );
-              promises.push(
-                dataProvider.onChangeItemChildren(target.targetItem, [
-                  ...(currentItems[target.targetItem].children ?? []),
-                  item.index,
-                ])
-              );
-            }
-          } else {
-            const newParent = currentItems[target.parentItem];
-            const newParentChildren = [...(newParent.children ?? [])].filter(
-              child => child !== item.index
-            );
+          if (
+            target.targetType === 'between-items' &&
+            target.parentItem === item.index
+          ) {
+            // Trying to drop inside itself
+            return;
+          }
 
-            if (target.parentItem === item.index) {
-              // Trying to drop inside itself
-              return;
-            }
+          if (
+            (target.targetType === 'item' || target.targetType === 'root') &&
+            target.targetItem !== parent.index
+          ) {
+            promises.push(
+              dataProvider.onChangeItemChildren(
+                parent.index,
+                parent.children.filter(child => child !== item.index)
+              )
+            );
+          }
 
+          if (target.targetType === 'between-items') {
             if (target.parentItem === parent.index) {
+              const newParent = currentItems[target.parentItem];
               const isOldItemPriorToNewItem =
                 ((newParent.children ?? []).findIndex(
                   child => child === item.index
                 ) ?? Infinity) < target.childIndex;
-              newParentChildren.splice(
-                target.childIndex - (isOldItemPriorToNewItem ? 1 : 0),
-                0,
-                item.index
-              );
-              promises.push(
-                dataProvider.onChangeItemChildren(
-                  target.parentItem,
-                  newParentChildren
-                )
-              );
+              itemsPriorToInsertion += isOldItemPriorToNewItem ? 1 : 0;
             } else {
-              newParentChildren.splice(target.childIndex, 0, item.index);
               promises.push(
                 dataProvider.onChangeItemChildren(
                   parent.index,
                   parent.children.filter(child => child !== item.index)
                 )
               );
-              promises.push(
-                dataProvider.onChangeItemChildren(
-                  target.parentItem,
-                  newParentChildren
-                )
-              );
             }
           }
         }
+
+        // insert new items
+        if (target.targetType === 'item' || target.targetType === 'root') {
+          promises.push(
+            dataProvider.onChangeItemChildren(target.targetItem, [
+              ...(currentItems[target.targetItem].children ?? []).filter(
+                i => !itemsIndices.includes(i)
+              ),
+              ...itemsIndices,
+            ])
+          );
+        } else {
+          const newParent = currentItems[target.parentItem];
+          const newParentChildren = [...(newParent.children ?? [])].filter(
+            c => !itemsIndices.includes(c)
+          );
+          newParentChildren.splice(
+            target.childIndex - itemsPriorToInsertion,
+            0,
+            ...itemsIndices
+          );
+          promises.push(
+            dataProvider.onChangeItemChildren(
+              target.parentItem,
+              newParentChildren
+            )
+          );
+        }
+
         await Promise.all(promises);
         props.onDrop?.(items, target);
       }}

packages/core/test/dnd-basics.spec.tsx

@@ -219,6 +219,24 @@ describe('dnd basics', () => {
       ]);
     });
 
+    it('drag items with scrambled order below in same folder', async () => {
+      const test = await new TestUtil().renderOpenTree();
+      await test.selectItems('aac', 'aaa', 'aab');
+      await test.startDrag('aab');
+      await test.dragOver('aad', 'bottom');
+      await test.drop();
+      await test.expectVisibleItemContents('aa', ['aad', 'aaa', 'aab', 'aac']);
+    });
+
+    it('drag items with scrambled order above in same folder', async () => {
+      const test = await new TestUtil().renderOpenTree();
+      await test.selectItems('aac', 'aad', 'aab');
+      await test.startDrag('aab');
+      await test.dragOver('aaa', 'top');
+      await test.drop();
+      await test.expectVisibleItemContents('aa', ['aab', 'aac', 'aad', 'aaa']);
+    });
+
     it('drag folders into another opened folder', async () => {
       const test = await new TestUtil().renderOpenTree();
       await test.selectItems('ab', 'ac', 'ad');