diff --git a/apps/client/src/features/page/tree/model/tree-model.test.ts b/apps/client/src/features/page/tree/model/tree-model.test.ts index a2dbd6b9..01682e2d 100644 --- a/apps/client/src/features/page/tree/model/tree-model.test.ts +++ b/apps/client/src/features/page/tree/model/tree-model.test.ts @@ -752,6 +752,27 @@ describe("treeModel.placeByPosition", () => { }); expect(t.map((n) => n.id)).toEqual(["r1", "child", "r2", "rp"]); }); + + it("returns same reference (no-op) when the destination parent is inside the source's own subtree (#206 ui-state-races-1)", () => { + // Moving `a` under its own descendant `b` is a cycle. Without the guard, + // remove(a) drops b too and insertByPosition can't re-place a -> the whole + // subtree silently vanishes. The guard refuses the move (same reference). + const cyclic: P[] = [ + { + id: "a", + name: "A", + position: "a0", + children: [{ id: "b", name: "B", position: "a1" }], + }, + ]; + const t = treeModel.placeByPosition(cyclic, "a", { + parentId: "b", + position: "a5", + }); + expect(t).toBe(cyclic); + expect(treeModel.find(t, "a")).not.toBeNull(); + expect(treeModel.find(t, "b")).not.toBeNull(); + }); }); describe("treeModel.move", () => { diff --git a/apps/client/src/features/page/tree/model/tree-model.ts b/apps/client/src/features/page/tree/model/tree-model.ts index aa13d8b4..bda4a74b 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -294,6 +294,20 @@ export const treeModel = { const source = treeModel.find(tree, sourceId); if (!source) return tree; if (to.parentId !== null && !treeModel.find(tree, to.parentId)) return tree; + // Cycle guard, mirroring `move`'s `isDescendant` check (#206 ui-state-races-1). + // If the destination parent is INSIDE the moved node's own subtree (reachable + // when server-authoritative move events arrive out of order — e.g. X moved + // under Y, then Y under X, but on this receiver Y is still inside X), then + // `remove(sourceId)` would drop the future parent along with the whole subtree + // and `insertByPosition` could not find it again — the node and ALL its + // descendants would silently vanish. Refuse the move and return the same + // reference so callers can detect the no-op and reconcile (refetch) instead. + if ( + to.parentId !== null && + treeModel.isDescendant(tree, sourceId, to.parentId) + ) { + return tree; + } const removed = treeModel.remove(tree, sourceId); // Reuse the same position-ordered insertion as `insertByPosition` by // stamping the authoritative position onto the moved node first. diff --git a/apps/client/src/features/websocket/tree-socket-reducers.test.ts b/apps/client/src/features/websocket/tree-socket-reducers.test.ts index f59f27cc..20abdf95 100644 --- a/apps/client/src/features/websocket/tree-socket-reducers.test.ts +++ b/apps/client/src/features/websocket/tree-socket-reducers.test.ts @@ -183,6 +183,34 @@ describe("applyMoveTreeNode", () => { expect(moved?.hasChildren).toBe(true); expect(moved?.position).toBe("a4"); }); + + it("does NOT drop a subtree on a cyclic/out-of-order move (parent inside source) (#206 ui-state-races-1)", () => { + // Locally `b` is still nested inside `a` (an earlier "a under b" echo hasn't + // applied yet). An out-of-order "move a under b" event now arrives — b is a + // descendant of a, so re-parenting would make placeByPosition remove a (and + // its whole subtree, incl. b) and fail to re-insert. Before the fix BOTH a + // and b silently vanished; now the reducer leaves the tree untouched. + const tree: SpaceTreeNode[] = [ + node("a", { + position: "a0", + hasChildren: true, + children: [node("b", { position: "a1", parentPageId: "a" })], + }), + ]; + const next = applyMoveTreeNode(tree, { + id: "a", + parentId: "b", + oldParentId: null, + index: 0, + position: "a4", + pageData: {}, + }); + // No silent data loss: both nodes survive. + expect(treeModel.find(next, "a")).not.toBeNull(); + expect(treeModel.find(next, "b")).not.toBeNull(); + // The cyclic move is refused as a no-op (same reference) pending reconcile. + expect(next).toBe(tree); + }); }); describe("applyDeleteTreeNode", () => { diff --git a/apps/client/src/features/websocket/tree-socket-reducers.ts b/apps/client/src/features/websocket/tree-socket-reducers.ts index 3f6226d9..fe3b1a43 100644 --- a/apps/client/src/features/websocket/tree-socket-reducers.ts +++ b/apps/client/src/features/websocket/tree-socket-reducers.ts @@ -76,6 +76,19 @@ export function applyMoveTreeNode( const oldParentId = (sourceBefore as SpaceTreeNode).parentPageId ?? null; const newParentId = payload.parentId as string | null; + // Cyclic / out-of-order move guard (#206 ui-state-races-1): if the + // authoritative new parent is currently INSIDE the moved node's own subtree on + // this client (e.g. server moved X under Y then Y under X and the events + // arrived such that Y is still nested in X here), re-parenting is impossible to + // represent locally. `placeByPosition` returns `prev` for this, but the + // `placed === prev` fallback below would then `remove` the source — dropping + // the node AND every descendant (incl. the would-be parent) silently. Leave the + // tree untouched instead; a later corrective event or a reconnect refetch + // reconciles it. Never delete a subtree we cannot safely re-place. + if (newParentId && treeModel.isDescendant(prev, payload.id, newParentId)) { + return prev; + } + // Place the node by its fractional `position` among the new siblings — NOT by // the sender's absolute `index` (the sender computed that against its own // loaded set, which differs from this receiver's). Using the position keeps