From 3d47c306fab950d1e97516c3f5d09141c0077211 Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Fri, 26 Jun 2026 06:00:26 +0300 Subject: [PATCH] fix(tree): cycle-guard placeByPosition so out-of-order moves don't drop subtrees (#206) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ui-state-races-1: the server-authoritative move path (placeByPosition, via applyMoveTreeNode) lacked the isDescendant cycle guard that drag-drop `move` has. When move events arrive out of order so the destination parent is still nested inside the moved node's own subtree, remove(source) dropped the whole subtree (incl. the future parent) and insertByPosition could not re-place it — the node and all descendants silently vanished with no error/refetch. Add the isDescendant guard to placeByPosition (returns same ref, like its other no-op cases) and short-circuit applyMoveTreeNode on the same condition BEFORE the placed===prev remove-fallback (which would otherwise still drop the subtree). Leave the tree untouched so a later corrective event / reconnect reconcile fixes it. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../page/tree/model/tree-model.test.ts | 21 ++++++++++++++ .../features/page/tree/model/tree-model.ts | 14 ++++++++++ .../websocket/tree-socket-reducers.test.ts | 28 +++++++++++++++++++ .../websocket/tree-socket-reducers.ts | 13 +++++++++ 4 files changed, 76 insertions(+) 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