fix(tree): cycle-guard placeByPosition so out-of-order moves don't drop subtrees (#206)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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", () => {
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user