From 9b12aa9882b32730550c64546c10b2d558377767 Mon Sep 17 00:00:00 2001 From: Evan Simkowitz Date: Tue, 10 Sep 2024 17:23:28 -0700 Subject: [PATCH] Fix bug in TileLayout where computeMoveNode had stale node data (#361) This was hard to debug. It manifested in nodes that had been collapsed into their parent. When a node was dragged over the recently-collapsed node, the placeholder would show the action as if the flex direction was reversed. It turns out useDrag has trouble tracking changes to the LayoutNode objects. For now, I am just finding the nodes again in the computeMoveNode function. Later when I redo the drag system, I will fix this better. --- frontend/layout/lib/TileLayout.tsx | 10 +++--- frontend/layout/lib/layoutTree.ts | 42 ++++++++++++++++-------- frontend/layout/lib/types.ts | 4 +-- frontend/layout/tests/layoutTree.test.ts | 16 ++++----- 4 files changed, 43 insertions(+), 29 deletions(-) diff --git a/frontend/layout/lib/TileLayout.tsx b/frontend/layout/lib/TileLayout.tsx index 612539bdb..9f48585e1 100644 --- a/frontend/layout/lib/TileLayout.tsx +++ b/frontend/layout/lib/TileLayout.tsx @@ -303,8 +303,8 @@ const OverlayNode = memo(({ node, layoutModel }: OverlayNodeProps) => { () => ({ accept: dragItemType, canDrop: (_, monitor) => { - const dragItem = monitor.getItem(); - if (monitor.isOver({ shallow: true }) && dragItem?.id !== node.id) { + const dragItemId = monitor.getItem(); + if (monitor.isOver({ shallow: true }) && dragItemId !== node.id) { return true; } return false; @@ -325,8 +325,8 @@ const OverlayNode = memo(({ node, layoutModel }: OverlayNodeProps) => { offset.y -= containerRect.y; layoutModel.treeReducer({ type: LayoutTreeActionType.ComputeMove, - node: node, - nodeToMove: dragItem, + nodeId: node.id, + nodeToMoveId: dragItem.id, direction: determineDropDirection(additionalProps.rect, offset), } as LayoutTreeComputeMoveNodeAction); } else { @@ -337,7 +337,7 @@ const OverlayNode = memo(({ node, layoutModel }: OverlayNodeProps) => { } }), }), - [node, additionalProps?.rect, layoutModel.displayContainerRef, layoutModel.onDrop, layoutModel.treeReducer] + [node.id, additionalProps?.rect, layoutModel.displayContainerRef, layoutModel.onDrop, layoutModel.treeReducer] ); // Register the overlay node as a drop target diff --git a/frontend/layout/lib/layoutTree.ts b/frontend/layout/lib/layoutTree.ts index 11f61f0ec..f1a98d0b0 100644 --- a/frontend/layout/lib/layoutTree.ts +++ b/frontend/layout/lib/layoutTree.ts @@ -39,27 +39,40 @@ export const DEFAULT_MAX_CHILDREN = 5; */ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertAction: LayoutTreeComputeMoveNodeAction) { const rootNode = layoutState.rootNode; - const { node, nodeToMove, direction } = computeInsertAction; + const { nodeId, nodeToMoveId, direction } = computeInsertAction; + if (!nodeId || !nodeToMoveId) { + console.warn("either nodeId or nodeToMoveId not set", nodeId, nodeToMoveId); + return; + } if (direction === undefined) { console.warn("No direction provided for insertItemInDirection"); return; } - if (node.id === nodeToMove.id) { + if (nodeId === nodeToMoveId) { console.warn("Cannot compute move node action since both nodes are equal"); return; } let newMoveOperation: MoveOperation; - const parent = lazy(() => findParent(rootNode, node.id)); + const parent = lazy(() => findParent(rootNode, nodeId)); const grandparent = lazy(() => findParent(rootNode, parent().id)); - const indexInParent = lazy(() => parent()?.children.findIndex((child) => node.id === child.id)); + const indexInParent = lazy(() => parent()?.children.findIndex((child) => nodeId === child.id)); const indexInGrandparent = lazy(() => grandparent()?.children.findIndex((child) => parent().id === child.id)); - const nodeToMoveParent = lazy(() => findParent(rootNode, nodeToMove.id)); + const nodeToMoveParent = lazy(() => findParent(rootNode, nodeToMoveId)); const nodeToMoveIndexInParent = lazy(() => - nodeToMoveParent()?.children.findIndex((child) => nodeToMove.id === child.id) + nodeToMoveParent()?.children.findIndex((child) => nodeToMoveId === child.id) ); - const isRoot = rootNode.id === node.id; + const isRoot = rootNode.id === nodeId; + + // TODO: this should not be necessary. The drag layer is having trouble tracking changes to the LayoutNode fields, so I need to grab the node again here to get the latest data. + const node = findNode(rootNode, nodeId); + const nodeToMove = findNode(rootNode, nodeToMoveId); + + if (!node || !nodeToMove) { + console.warn("node or nodeToMove not set", nodeId, nodeToMoveId); + return; + } switch (direction) { case DropDirection.OuterTop: @@ -77,7 +90,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio } case DropDirection.Top: if (node.flexDirection === FlexDirection.Column) { - newMoveOperation = { parentId: node.id, index: 0, node: nodeToMove }; + newMoveOperation = { parentId: nodeId, index: 0, node: nodeToMove }; } else { if (isRoot) newMoveOperation = { @@ -110,7 +123,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio } case DropDirection.Bottom: if (node.flexDirection === FlexDirection.Column) { - newMoveOperation = { parentId: node.id, index: 1, node: nodeToMove }; + newMoveOperation = { parentId: nodeId, index: 1, node: nodeToMove }; } else { if (isRoot) newMoveOperation = { @@ -143,7 +156,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio } case DropDirection.Left: if (node.flexDirection === FlexDirection.Row) { - newMoveOperation = { parentId: node.id, index: 0, node: nodeToMove }; + newMoveOperation = { parentId: nodeId, index: 0, node: nodeToMove }; } else { const parentNode = parent(); if (parentNode) @@ -169,7 +182,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio } case DropDirection.Right: if (node.flexDirection === FlexDirection.Row) { - newMoveOperation = { parentId: node.id, index: 1, node: nodeToMove }; + newMoveOperation = { parentId: nodeId, index: 1, node: nodeToMove }; } else { const parentNode = parent(); if (parentNode) @@ -181,11 +194,11 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio } break; case DropDirection.Center: - if (node.id !== rootNode.id && nodeToMove.id !== rootNode.id) { + if (nodeId !== rootNode.id && nodeToMoveId !== rootNode.id) { const swapAction: LayoutTreeSwapNodeAction = { type: LayoutTreeActionType.Swap, - node1Id: node.id, - node2Id: nodeToMove.id, + node1Id: nodeId, + node2Id: nodeToMoveId, }; return swapAction; } else { @@ -208,6 +221,7 @@ export function computeMoveNode(layoutState: LayoutTreeState, computeInsertActio } export function moveNode(layoutState: LayoutTreeState, action: LayoutTreeMoveNodeAction) { + console.log("moveNode", layoutState, action); const rootNode = layoutState.rootNode; if (!action) { console.error("no move node action provided"); diff --git a/frontend/layout/lib/types.ts b/frontend/layout/lib/types.ts index 064a3bbe6..692966994 100644 --- a/frontend/layout/lib/types.ts +++ b/frontend/layout/lib/types.ts @@ -86,8 +86,8 @@ export interface LayoutTreeAction { */ export interface LayoutTreeComputeMoveNodeAction extends LayoutTreeAction { type: LayoutTreeActionType.ComputeMove; - node: LayoutNode; - nodeToMove: LayoutNode; + nodeId: string; + nodeToMoveId: string; direction: DropDirection; } diff --git a/frontend/layout/tests/layoutTree.test.ts b/frontend/layout/tests/layoutTree.test.ts index 03cb3209b..85f9b707e 100644 --- a/frontend/layout/tests/layoutTree.test.ts +++ b/frontend/layout/tests/layoutTree.test.ts @@ -18,8 +18,8 @@ test("layoutTreeStateReducer - compute move", () => { let node1 = newLayoutNode(undefined, undefined, undefined, { blockId: "node1" }); let pendingAction = computeMoveNode(treeState, { type: LayoutTreeActionType.ComputeMove, - node: treeState.rootNode, - nodeToMove: node1, + nodeId: treeState.rootNode.id, + nodeToMoveId: node1.id, direction: DropDirection.Bottom, }); const insertOperation = pendingAction as LayoutTreeMoveNodeAction; @@ -37,8 +37,8 @@ test("layoutTreeStateReducer - compute move", () => { let node2 = newLayoutNode(undefined, undefined, undefined, { blockId: "node2" }); pendingAction = computeMoveNode(treeState, { type: LayoutTreeActionType.ComputeMove, - node: node1, - nodeToMove: node2, + nodeId: node1.id, + nodeToMoveId: node2.id, direction: DropDirection.Bottom, }); const insertOperation2 = pendingAction as LayoutTreeMoveNodeAction; @@ -64,8 +64,8 @@ test("computeMove - noop action", () => { ); let moveAction: LayoutTreeComputeMoveNodeAction = { type: LayoutTreeActionType.ComputeMove, - node: treeState.rootNode, - nodeToMove, + nodeId: treeState.rootNode.id, + nodeToMoveId: nodeToMove.id, direction: DropDirection.Left, }; let pendingAction = computeMoveNode(treeState, moveAction); @@ -74,8 +74,8 @@ test("computeMove - noop action", () => { moveAction = { type: LayoutTreeActionType.ComputeMove, - node: treeState.rootNode, - nodeToMove, + nodeId: treeState.rootNode.id, + nodeToMoveId: nodeToMove.id, direction: DropDirection.Right, };