From 7b8d1649a15cd792191ea2232e42ec0948c900ac Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Thu, 25 Jun 2026 11:00:39 +0300 Subject: [PATCH] fix(tree): stop silent page loss on move-to-unloaded-parent + reconnect ghost roots (#159) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two confirmed P1 data-loss findings in the sidebar tree sync. #1 — Move into an unloaded/collapsed parent silently dropped pages. When a moveTreeNode (or addTreeNode) broadcast targeted a parent whose children were NOT yet lazy-loaded, `insertByPosition` did `kids = parent.children ?? []` and inserted the moved node, MATERIALIZING a misleading partial child list (`[movedNode]`) out of an unloaded (`children === undefined`) parent. The lazy-load gate fetches only when children are absent/empty, so it then refused to fetch — leaving the parent showing ONLY the moved node and HIDING all its other real children (and, when the parent wasn't in the tree at all, the node was removed and never re-fetched). Fix: `insertByPosition` distinguishes `children === undefined` (not loaded) from `[]` (loaded-empty) and, for an unloaded parent, does NOT insert — it leaves children unloaded and just flags `hasChildren`, so expanding fetches the FULL set (including the moved/added node) via the existing lazy-load. #2 — After a socket reconnect, a deleted/moved-away root lingered as a 404 "ghost". `mergeRootTrees` was append-only: it kept every previously-loaded root and only added new ones, so a root removed during the missed-events gap was never dropped. It runs only once all root pages are fetched, so the incoming list is the authoritative complete root set — fix reconciles to it (drop roots absent from incoming) while PRESERVING each surviving root's lazy-loaded subtree and refreshing its own fields. Tests: insertByPosition unloaded-vs-loaded-empty parent; the move reducer keeps a collapsed destination lazy-loadable instead of partial; mergeRootTrees drops a ghost root, preserves a surviving subtree, adds new roots, refreshes fields. The existing "remove when parent not in tree" reducer test still holds. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../page/tree/model/tree-model.test.ts | 840 ++++++++++-------- .../features/page/tree/model/tree-model.ts | 38 +- .../features/page/tree/utils/utils.test.ts | 66 +- .../src/features/page/tree/utils/utils.ts | 29 +- .../websocket/tree-socket-reducers.test.ts | 36 +- 5 files changed, 612 insertions(+), 397 deletions(-) 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 25c0bd1e..b726155e 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 @@ -1,218 +1,264 @@ -import { describe, it, expect } from 'vitest'; -import { treeModel } from './tree-model'; -import type { TreeNode } from './tree-model.types'; +import { describe, it, expect } from "vitest"; +import { treeModel } from "./tree-model"; +import type { TreeNode } from "./tree-model.types"; type N = TreeNode<{ name: string }>; const fixture: N[] = [ { - id: 'a', - name: 'A', + id: "a", + name: "A", children: [ - { id: 'a1', name: 'A1', children: [{ id: 'a1a', name: 'A1a' }] }, - { id: 'a2', name: 'A2' }, + { id: "a1", name: "A1", children: [{ id: "a1a", name: "A1a" }] }, + { id: "a2", name: "A2" }, ], }, - { id: 'b', name: 'B' }, + { id: "b", name: "B" }, ]; -describe('treeModel.find', () => { - it('finds a root node', () => { - expect(treeModel.find(fixture, 'a')?.name).toBe('A'); +describe("treeModel.find", () => { + it("finds a root node", () => { + expect(treeModel.find(fixture, "a")?.name).toBe("A"); }); - it('finds a deeply nested node', () => { - expect(treeModel.find(fixture, 'a1a')?.name).toBe('A1a'); + it("finds a deeply nested node", () => { + expect(treeModel.find(fixture, "a1a")?.name).toBe("A1a"); }); - it('returns null for unknown id', () => { - expect(treeModel.find(fixture, 'zzz')).toBeNull(); + it("returns null for unknown id", () => { + expect(treeModel.find(fixture, "zzz")).toBeNull(); }); }); -describe('treeModel.path', () => { - it('returns root-to-leaf path for nested id', () => { - const p = treeModel.path(fixture, 'a1a'); - expect(p?.map((n) => n.id)).toEqual(['a', 'a1', 'a1a']); +describe("treeModel.path", () => { + it("returns root-to-leaf path for nested id", () => { + const p = treeModel.path(fixture, "a1a"); + expect(p?.map((n) => n.id)).toEqual(["a", "a1", "a1a"]); }); - it('returns [node] for root-level id', () => { - expect(treeModel.path(fixture, 'b')?.map((n) => n.id)).toEqual(['b']); + it("returns [node] for root-level id", () => { + expect(treeModel.path(fixture, "b")?.map((n) => n.id)).toEqual(["b"]); }); - it('returns null for unknown id', () => { - expect(treeModel.path(fixture, 'zzz')).toBeNull(); + it("returns null for unknown id", () => { + expect(treeModel.path(fixture, "zzz")).toBeNull(); }); }); -describe('treeModel.siblingsOf', () => { - it('returns siblings + parent + index for a child', () => { - const info = treeModel.siblingsOf(fixture, 'a2'); - expect(info?.parentId).toBe('a'); - expect(info?.siblings.map((n) => n.id)).toEqual(['a1', 'a2']); +describe("treeModel.siblingsOf", () => { + it("returns siblings + parent + index for a child", () => { + const info = treeModel.siblingsOf(fixture, "a2"); + expect(info?.parentId).toBe("a"); + expect(info?.siblings.map((n) => n.id)).toEqual(["a1", "a2"]); expect(info?.index).toBe(1); }); - it('returns parentId null + root siblings for a root id', () => { - const info = treeModel.siblingsOf(fixture, 'b'); + it("returns parentId null + root siblings for a root id", () => { + const info = treeModel.siblingsOf(fixture, "b"); expect(info?.parentId).toBeNull(); - expect(info?.siblings.map((n) => n.id)).toEqual(['a', 'b']); + expect(info?.siblings.map((n) => n.id)).toEqual(["a", "b"]); expect(info?.index).toBe(1); }); - it('returns null for unknown id', () => { - expect(treeModel.siblingsOf(fixture, 'zzz')).toBeNull(); + it("returns null for unknown id", () => { + expect(treeModel.siblingsOf(fixture, "zzz")).toBeNull(); }); }); -describe('treeModel.isDescendant', () => { - it('returns true when descendantId is nested under ancestorId', () => { - expect(treeModel.isDescendant(fixture, 'a', 'a1a')).toBe(true); +describe("treeModel.isDescendant", () => { + it("returns true when descendantId is nested under ancestorId", () => { + expect(treeModel.isDescendant(fixture, "a", "a1a")).toBe(true); }); - it('returns false when ids are siblings', () => { - expect(treeModel.isDescendant(fixture, 'a1', 'a2')).toBe(false); + it("returns false when ids are siblings", () => { + expect(treeModel.isDescendant(fixture, "a1", "a2")).toBe(false); }); - it('returns false when ancestorId is the same as descendantId', () => { - expect(treeModel.isDescendant(fixture, 'a', 'a')).toBe(false); + it("returns false when ancestorId is the same as descendantId", () => { + expect(treeModel.isDescendant(fixture, "a", "a")).toBe(false); }); - it('returns false for unknown ids', () => { - expect(treeModel.isDescendant(fixture, 'zzz', 'a')).toBe(false); + it("returns false for unknown ids", () => { + expect(treeModel.isDescendant(fixture, "zzz", "a")).toBe(false); }); }); -describe('treeModel.visible', () => { - it('returns only root nodes when no openIds', () => { +describe("treeModel.visible", () => { + it("returns only root nodes when no openIds", () => { const v = treeModel.visible(fixture, new Set()); - expect(v.map((n) => n.id)).toEqual(['a', 'b']); + expect(v.map((n) => n.id)).toEqual(["a", "b"]); }); - it('includes children of open ids in DFS order', () => { - const v = treeModel.visible(fixture, new Set(['a'])); - expect(v.map((n) => n.id)).toEqual(['a', 'a1', 'a2', 'b']); + it("includes children of open ids in DFS order", () => { + const v = treeModel.visible(fixture, new Set(["a"])); + expect(v.map((n) => n.id)).toEqual(["a", "a1", "a2", "b"]); }); - it('recursively descends through chains of open ids', () => { - const v = treeModel.visible(fixture, new Set(['a', 'a1'])); - expect(v.map((n) => n.id)).toEqual(['a', 'a1', 'a1a', 'a2', 'b']); + it("recursively descends through chains of open ids", () => { + const v = treeModel.visible(fixture, new Set(["a", "a1"])); + expect(v.map((n) => n.id)).toEqual(["a", "a1", "a1a", "a2", "b"]); }); - it('ignores openIds that are not in the tree', () => { - const v = treeModel.visible(fixture, new Set(['ghost'])); - expect(v.map((n) => n.id)).toEqual(['a', 'b']); + it("ignores openIds that are not in the tree", () => { + const v = treeModel.visible(fixture, new Set(["ghost"])); + expect(v.map((n) => n.id)).toEqual(["a", "b"]); }); }); -describe('treeModel.insert', () => { +describe("treeModel.insert", () => { const leaf = (id: string): N => ({ id, name: id.toUpperCase() }); - it('inserts at end when index is undefined', () => { - const t = treeModel.insert(fixture, 'a', leaf('a3')); - expect(treeModel.siblingsOf(t, 'a3')?.siblings.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'a3', + it("inserts at end when index is undefined", () => { + const t = treeModel.insert(fixture, "a", leaf("a3")); + expect(treeModel.siblingsOf(t, "a3")?.siblings.map((n) => n.id)).toEqual([ + "a1", + "a2", + "a3", ]); }); - it('inserts at index 0', () => { - const t = treeModel.insert(fixture, 'a', leaf('a0'), 0); - expect(treeModel.siblingsOf(t, 'a0')?.siblings.map((n) => n.id)).toEqual([ - 'a0', 'a1', 'a2', + it("inserts at index 0", () => { + const t = treeModel.insert(fixture, "a", leaf("a0"), 0); + expect(treeModel.siblingsOf(t, "a0")?.siblings.map((n) => n.id)).toEqual([ + "a0", + "a1", + "a2", ]); }); - it('inserts in the middle', () => { - const t = treeModel.insert(fixture, 'a', leaf('a1half'), 1); + it("inserts in the middle", () => { + const t = treeModel.insert(fixture, "a", leaf("a1half"), 1); expect( - treeModel.siblingsOf(t, 'a1half')?.siblings.map((n) => n.id), - ).toEqual(['a1', 'a1half', 'a2']); + treeModel.siblingsOf(t, "a1half")?.siblings.map((n) => n.id), + ).toEqual(["a1", "a1half", "a2"]); }); - it('inserts at root when parentId is null', () => { - const t = treeModel.insert(fixture, null, leaf('c')); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'c']); + it("inserts at root when parentId is null", () => { + const t = treeModel.insert(fixture, null, leaf("c")); + expect(t.map((n) => n.id)).toEqual(["a", "b", "c"]); }); - it('returns same array reference for unknown parentId', () => { - const t = treeModel.insert(fixture, 'ghost', leaf('zz')); + it("returns same array reference for unknown parentId", () => { + const t = treeModel.insert(fixture, "ghost", leaf("zz")); expect(t).toBe(fixture); }); - it('initializes children array when parent had no children', () => { - const t = treeModel.insert(fixture, 'b', leaf('b1')); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['b1']); + it("initializes children array when parent had no children", () => { + const t = treeModel.insert(fixture, "b", leaf("b1")); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["b1"]); }); }); -describe('treeModel.insertByPosition', () => { +describe("treeModel.insertByPosition", () => { // Server-authoritative broadcasts ship the node's fractional `position`; the // receiver inserts among already-loaded siblings ordered by `position`. type P = TreeNode<{ name: string; position?: string }>; const roots: P[] = [ - { id: 'a', name: 'A', position: 'a0' }, - { id: 'b', name: 'B', position: 'a2' }, - { id: 'c', name: 'C', position: 'a4' }, + { id: "a", name: "A", position: "a0" }, + { id: "b", name: "B", position: "a2" }, + { id: "c", name: "C", position: "a4" }, ]; - it('inserts a root node in position order (middle)', () => { - const node: P = { id: 'x', name: 'X', position: 'a3' }; + it("inserts a root node in position order (middle)", () => { + const node: P = { id: "x", name: "X", position: "a3" }; const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'x', 'c']); + expect(t.map((n) => n.id)).toEqual(["a", "b", "x", "c"]); }); - it('inserts a root node at the front when its position sorts first', () => { - const node: P = { id: 'x', name: 'X', position: 'a-' }; + it("inserts a root node at the front when its position sorts first", () => { + const node: P = { id: "x", name: "X", position: "a-" }; const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['x', 'a', 'b', 'c']); + expect(t.map((n) => n.id)).toEqual(["x", "a", "b", "c"]); }); - it('appends a root node when its position sorts last', () => { - const node: P = { id: 'x', name: 'X', position: 'a9' }; + it("appends a root node when its position sorts last", () => { + const node: P = { id: "x", name: "X", position: "a9" }; const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'c', 'x']); + expect(t.map((n) => n.id)).toEqual(["a", "b", "c", "x"]); }); - it('produces the same order regardless of which siblings are loaded', () => { + it("produces the same order regardless of which siblings are loaded", () => { // Client 1 loaded all siblings; client 2 only loaded a subset. The inserted // node lands in a consistent relative position for both. const full: P[] = roots; const partial: P[] = [roots[0], roots[2]]; // a, c (b not loaded) - const node: P = { id: 'x', name: 'X', position: 'a3' }; + const node: P = { id: "x", name: "X", position: "a3" }; expect( treeModel.insertByPosition(full, null, node).map((n) => n.id), - ).toEqual(['a', 'b', 'x', 'c']); + ).toEqual(["a", "b", "x", "c"]); expect( treeModel.insertByPosition(partial, null, node).map((n) => n.id), - ).toEqual(['a', 'x', 'c']); + ).toEqual(["a", "x", "c"]); }); - it('inserts a child in position order under the parent', () => { + it("inserts a child in position order under the parent", () => { const tree: P[] = [ { - id: 'p', - name: 'P', - position: 'a0', + id: "p", + name: "P", + position: "a0", children: [ - { id: 'p1', name: 'P1', position: 'a0' }, - { id: 'p2', name: 'P2', position: 'a2' }, + { id: "p1", name: "P1", position: "a0" }, + { id: "p2", name: "P2", position: "a2" }, ], }, ]; - const node: P = { id: 'p15', name: 'P1.5', position: 'a1' }; - const t = treeModel.insertByPosition(tree, 'p', node); - expect(treeModel.find(t, 'p')?.children?.map((n) => n.id)).toEqual([ - 'p1', 'p15', 'p2', + const node: P = { id: "p15", name: "P1.5", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual([ + "p1", + "p15", + "p2", ]); }); - it('appends when the new node has no position', () => { - const node: P = { id: 'x', name: 'X' }; - const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'c', 'x']); + // #159 #1: inserting/moving a node under a parent whose children are NOT + // loaded (`children === undefined`, e.g. a collapsed page) must NOT materialize + // a partial `[node]` list — that would defeat the lazy-load gate and hide the + // parent's other real children. The node is left to be lazy-loaded; only + // `hasChildren` is flagged so the chevron appears. + it("does NOT materialize a child under an UNLOADED parent (children undefined)", () => { + type PH = TreeNode<{ + name: string; + position?: string; + hasChildren?: boolean; + }>; + const tree: PH[] = [ + { id: "p", name: "P", position: "a0", hasChildren: false }, // children: undefined + ]; + const node: PH = { id: "x", name: "X", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + const parent = treeModel.find(t, "p"); + // The node was NOT inserted (children stay unloaded -> lazy-load fetches the + // full set, including this node, on expand). + expect(parent?.children).toBeUndefined(); + expect(treeModel.find(t, "x")).toBeNull(); + // ...but the chevron is enabled so the user can expand to load it. + expect((parent as PH).hasChildren).toBe(true); }); - it('tie-break: a node whose position EQUALS a sibling lands deterministically (strict >)', () => { + it("DOES insert under a LOADED-but-empty parent (children: [])", () => { + type PH = TreeNode<{ + name: string; + position?: string; + hasChildren?: boolean; + }>; + const tree: PH[] = [ + { id: "p", name: "P", position: "a0", hasChildren: false, children: [] }, + ]; + const node: PH = { id: "x", name: "X", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + // A loaded (empty) child list is complete, so the node IS inserted. + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]); + }); + + it("appends when the new node has no position", () => { + const node: P = { id: "x", name: "X" }; + const t = treeModel.insertByPosition(roots, null, node); + expect(t.map((n) => n.id)).toEqual(["a", "b", "c", "x"]); + }); + + it("tie-break: a node whose position EQUALS a sibling lands deterministically (strict >)", () => { // The insertion index is the first sibling whose position sorts STRICTLY // after the new node's. An equal sibling is not strictly after, so it is // skipped — the new node lands immediately AFTER every equal-position // sibling and before the first strictly-greater one. This is deterministic: // a tie always resolves the same way on every client. - const node: P = { id: 'x', name: 'X', position: 'a2' }; // equals b's position + const node: P = { id: "x", name: "X", position: "a2" }; // equals b's position const t = treeModel.insertByPosition(roots, null, node); - expect(t.map((n) => n.id)).toEqual(['a', 'b', 'x', 'c']); + expect(t.map((n) => n.id)).toEqual(["a", "b", "x", "c"]); }); }); // addTreeNode idempotency: the receiver early-returns when the node id already // exists, so re-delivery (or the author's optimistic node) is never duplicated. // This guards the find-then-skip contract insertByPosition relies on. -describe('addTreeNode idempotency (find-then-skip)', () => { +describe("addTreeNode idempotency (find-then-skip)", () => { type P = TreeNode<{ name: string; position?: string }>; const applyAddTreeNode = (tree: P[], node: P): P[] => { @@ -220,22 +266,22 @@ describe('addTreeNode idempotency (find-then-skip)', () => { return treeModel.insertByPosition(tree, null, node); }; - it('does not insert a duplicate when the id already exists', () => { - const tree: P[] = [{ id: 'a', name: 'A', position: 'a0' }]; - const node: P = { id: 'a', name: 'A again', position: 'a5' }; + it("does not insert a duplicate when the id already exists", () => { + const tree: P[] = [{ id: "a", name: "A", position: "a0" }]; + const node: P = { id: "a", name: "A again", position: "a5" }; const t1 = applyAddTreeNode(tree, node); expect(t1).toBe(tree); - expect(t1.map((n) => n.id)).toEqual(['a']); + expect(t1.map((n) => n.id)).toEqual(["a"]); }); - it('inserts once, then is a no-op on repeat delivery', () => { - let tree: P[] = [{ id: 'a', name: 'A', position: 'a0' }]; - const node: P = { id: 'x', name: 'X', position: 'a5' }; + it("inserts once, then is a no-op on repeat delivery", () => { + let tree: P[] = [{ id: "a", name: "A", position: "a0" }]; + const node: P = { id: "x", name: "X", position: "a5" }; tree = applyAddTreeNode(tree, node); - expect(tree.map((n) => n.id)).toEqual(['a', 'x']); + expect(tree.map((n) => n.id)).toEqual(["a", "x"]); const again = applyAddTreeNode(tree, node); expect(again).toBe(tree); - expect(again.filter((n) => n.id === 'x')).toHaveLength(1); + expect(again.filter((n) => n.id === "x")).toHaveLength(1); }); }); @@ -243,7 +289,7 @@ describe('addTreeNode idempotency (find-then-skip)', () => { // now guarded by `treeModel.find` (same contract as the addTreeNode socket // handler) because the server's broadcast can win the race and insert the node // first. Whichever runs first inserts; the second is a no-op. Exactly one row. -describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { +describe("handleCreate optimistic-insert idempotency (find-then-skip)", () => { // Mirrors the guarded optimistic insert in use-tree-mutation handleCreate. const applyOptimisticInsert = ( tree: N[], @@ -256,17 +302,21 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { }; // Mirrors the addTreeNode socket handler guard. - const applyAddTreeNode = (tree: N[], parentId: string | null, node: N): N[] => { + const applyAddTreeNode = ( + tree: N[], + parentId: string | null, + node: N, + ): N[] => { if (treeModel.find(tree, node.id)) return tree; return treeModel.insert(tree, parentId, node); }; - const created: N = { id: 'new', name: '' }; + const created: N = { id: "new", name: "" }; - it('optimistic insert is a no-op when server addTreeNode already inserted it', () => { + it("optimistic insert is a no-op when server addTreeNode already inserted it", () => { // Reverse-of-reverse race: server wins. const afterServer = applyAddTreeNode(fixture, null, created); - expect(afterServer.filter((n) => n.id === 'new')).toHaveLength(1); + expect(afterServer.filter((n) => n.id === "new")).toHaveLength(1); const afterOptimistic = applyOptimisticInsert( afterServer, null, @@ -274,20 +324,27 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { afterServer.length, ); expect(afterOptimistic).toBe(afterServer); // skipped - expect(afterOptimistic.filter((n) => n.id === 'new')).toHaveLength(1); + expect(afterOptimistic.filter((n) => n.id === "new")).toHaveLength(1); }); - it('server addTreeNode is a no-op when optimistic insert already ran (optimistic-first)', () => { - const afterOptimistic = applyOptimisticInsert(fixture, null, created, fixture.length); - expect(afterOptimistic.filter((n) => n.id === 'new')).toHaveLength(1); + it("server addTreeNode is a no-op when optimistic insert already ran (optimistic-first)", () => { + const afterOptimistic = applyOptimisticInsert( + fixture, + null, + created, + fixture.length, + ); + expect(afterOptimistic.filter((n) => n.id === "new")).toHaveLength(1); const afterServer = applyAddTreeNode(afterOptimistic, null, created); expect(afterServer).toBe(afterOptimistic); // skipped - expect(afterServer.filter((n) => n.id === 'new')).toHaveLength(1); + expect(afterServer.filter((n) => n.id === "new")).toHaveLength(1); }); - it('inserts exactly once when only the optimistic path runs', () => { - const t = applyOptimisticInsert(fixture, 'a', { id: 'a3', name: '' }, 2); - expect(treeModel.find(t, 'a')?.children?.filter((n) => n.id === 'a3')).toHaveLength(1); + it("inserts exactly once when only the optimistic path runs", () => { + const t = applyOptimisticInsert(fixture, "a", { id: "a3", name: "" }, 2); + expect( + treeModel.find(t, "a")?.children?.filter((n) => n.id === "a3"), + ).toHaveLength(1); }); }); @@ -295,7 +352,7 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => { // by `position` (NOT index 0) and apply the `pageData` the payload carries so a // moved node's title/icon/chevron stay correct. This mirrors the reducer in // use-tree-socket.ts so the contract is unit-tested without rendering the hook. -describe('moveTreeNode handler (place by position + apply pageData)', () => { +describe("moveTreeNode handler (place by position + apply pageData)", () => { type P = TreeNode<{ name: string; position?: string; @@ -310,7 +367,11 @@ describe('moveTreeNode handler (place by position + apply pageData)', () => { id: string; parentId: string | null; position: string; - pageData?: { title?: string | null; icon?: string | null; hasChildren?: boolean }; + pageData?: { + title?: string | null; + icon?: string | null; + hasChildren?: boolean; + }; }, ): P[] => { if (!treeModel.find(tree, payload.id)) return tree; @@ -325,8 +386,10 @@ describe('moveTreeNode handler (place by position + apply pageData)', () => { } as Partial

; const pd = payload.pageData; if (pd) { - if (pd.title !== undefined) (patch as { name?: string }).name = pd.title ?? ''; - if (pd.icon !== undefined) (patch as { icon?: string }).icon = pd.icon ?? undefined; + if (pd.title !== undefined) + (patch as { name?: string }).name = pd.title ?? ""; + if (pd.icon !== undefined) + (patch as { icon?: string }).icon = pd.icon ?? undefined; if (pd.hasChildren !== undefined) (patch as { hasChildren?: boolean }).hasChildren = pd.hasChildren; } @@ -335,118 +398,128 @@ describe('moveTreeNode handler (place by position + apply pageData)', () => { const tree: P[] = [ { - id: 'dst', - name: 'DST', - position: 'a0', + id: "dst", + name: "DST", + position: "a0", children: [ - { id: 'c1', name: 'C1', position: 'a1' }, - { id: 'c2', name: 'C2', position: 'a3' }, - { id: 'c3', name: 'C3', position: 'a5' }, + { id: "c1", name: "C1", position: "a1" }, + { id: "c2", name: "C2", position: "a3" }, + { id: "c3", name: "C3", position: "a5" }, ], }, - { id: 'src', name: 'SRC', position: 'a9' }, + { id: "src", name: "SRC", position: "a9" }, ]; - it('lands the moved node in the correct MIDDLE slot, not at index 0', () => { + it("lands the moved node in the correct MIDDLE slot, not at index 0", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'dst', - position: 'a4', + id: "src", + parentId: "dst", + position: "a4", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'src', 'c3', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "src", + "c3", ]); }); - it('lands the moved node at the END when position sorts last', () => { + it("lands the moved node at the END when position sorts last", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'dst', - position: 'a8', + id: "src", + parentId: "dst", + position: "a8", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'c3', 'src', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "c3", + "src", ]); }); - it('applies pageData (title/icon/hasChildren) to the moved node', () => { + it("applies pageData (title/icon/hasChildren) to the moved node", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'dst', - position: 'a4', - pageData: { title: 'Renamed', icon: '🔥', hasChildren: true }, + id: "src", + parentId: "dst", + position: "a4", + pageData: { title: "Renamed", icon: "🔥", hasChildren: true }, }); - const moved = treeModel.find(t, 'src'); - expect(moved?.name).toBe('Renamed'); - expect(moved?.icon).toBe('🔥'); + const moved = treeModel.find(t, "src"); + expect(moved?.name).toBe("Renamed"); + expect(moved?.icon).toBe("🔥"); expect(moved?.hasChildren).toBe(true); - expect(moved?.position).toBe('a4'); + expect(moved?.position).toBe("a4"); }); - it('falls back to removing the node when the destination parent is not loaded', () => { + it("falls back to removing the node when the destination parent is not loaded", () => { const t = applyMoveTreeNode(tree, { - id: 'src', - parentId: 'not-loaded', - position: 'a4', + id: "src", + parentId: "not-loaded", + position: "a4", }); - expect(treeModel.find(t, 'src')).toBeNull(); + expect(treeModel.find(t, "src")).toBeNull(); }); }); -describe('treeModel.remove', () => { - it('removes a leaf', () => { - const t = treeModel.remove(fixture, 'a2'); - expect(treeModel.find(t, 'a2')).toBeNull(); +describe("treeModel.remove", () => { + it("removes a leaf", () => { + const t = treeModel.remove(fixture, "a2"); + expect(treeModel.find(t, "a2")).toBeNull(); }); - it('removes a subtree', () => { - const t = treeModel.remove(fixture, 'a1'); - expect(treeModel.find(t, 'a1')).toBeNull(); - expect(treeModel.find(t, 'a1a')).toBeNull(); + it("removes a subtree", () => { + const t = treeModel.remove(fixture, "a1"); + expect(treeModel.find(t, "a1")).toBeNull(); + expect(treeModel.find(t, "a1a")).toBeNull(); }); - it('removes a root node', () => { - const t = treeModel.remove(fixture, 'b'); - expect(t.map((n) => n.id)).toEqual(['a']); + it("removes a root node", () => { + const t = treeModel.remove(fixture, "b"); + expect(t.map((n) => n.id)).toEqual(["a"]); }); - it('returns same array reference for unknown id', () => { - expect(treeModel.remove(fixture, 'ghost')).toBe(fixture); + it("returns same array reference for unknown id", () => { + expect(treeModel.remove(fixture, "ghost")).toBe(fixture); }); }); -describe('treeModel.update', () => { - it('shallow-merges a patch on the matching node', () => { - const t = treeModel.update(fixture, 'a1', { name: 'A1-renamed' }); - expect(treeModel.find(t, 'a1')?.name).toBe('A1-renamed'); +describe("treeModel.update", () => { + it("shallow-merges a patch on the matching node", () => { + const t = treeModel.update(fixture, "a1", { name: "A1-renamed" }); + expect(treeModel.find(t, "a1")?.name).toBe("A1-renamed"); }); - it('returns same array reference for unknown id', () => { - expect(treeModel.update(fixture, 'ghost', { name: 'x' })).toBe(fixture); + it("returns same array reference for unknown id", () => { + expect(treeModel.update(fixture, "ghost", { name: "x" })).toBe(fixture); }); it("preserves children when patching parent's own fields", () => { - const t = treeModel.update(fixture, 'a', { name: 'A-renamed' }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', + const t = treeModel.update(fixture, "a", { name: "A-renamed" }); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", ]); }); - it('preserves reference identity of unrelated subtrees', () => { - const t = treeModel.update(fixture, 'a1', { name: 'X' }); + it("preserves reference identity of unrelated subtrees", () => { + const t = treeModel.update(fixture, "a1", { name: "X" }); expect(t[1]).toBe(fixture[1]); }); }); -describe('treeModel.appendChildren', () => { +describe("treeModel.appendChildren", () => { const kid = (id: string): N => ({ id, name: id }); - it('appends to existing children', () => { - const t = treeModel.appendChildren(fixture, 'a', [kid('a3'), kid('a4')]); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'a3', 'a4', + it("appends to existing children", () => { + const t = treeModel.appendChildren(fixture, "a", [kid("a3"), kid("a4")]); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", + "a3", + "a4", ]); }); - it('initializes children when parent had none', () => { - const t = treeModel.appendChildren(fixture, 'b', [kid('b1')]); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['b1']); + it("initializes children when parent had none", () => { + const t = treeModel.appendChildren(fixture, "b", [kid("b1")]); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["b1"]); }); - it('returns same array reference for unknown parentId', () => { - expect(treeModel.appendChildren(fixture, 'ghost', [kid('zz')])).toBe( + it("returns same array reference for unknown parentId", () => { + expect(treeModel.appendChildren(fixture, "ghost", [kid("zz")])).toBe( fixture, ); }); @@ -454,58 +527,60 @@ describe('treeModel.appendChildren', () => { // Regression: lazy-load + auto-expand can race and call appendChildren with // children that overlap what's already there. React then crashes on duplicate // keys. Defensive dedup at the model level. - it('dedups against existing children by id', () => { - const t1 = treeModel.appendChildren(fixture, 'a', [ - kid('a3'), - kid('a4'), + it("dedups against existing children by id", () => { + const t1 = treeModel.appendChildren(fixture, "a", [kid("a3"), kid("a4")]); + const t2 = treeModel.appendChildren(t1, "a", [ + kid("a3"), + kid("a4"), + kid("a5"), ]); - const t2 = treeModel.appendChildren(t1, 'a', [ - kid('a3'), - kid('a4'), - kid('a5'), - ]); - expect(treeModel.find(t2, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'a3', 'a4', 'a5', + expect(treeModel.find(t2, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", + "a3", + "a4", + "a5", ]); }); - it('returns same array reference when every child is a duplicate', () => { - const t1 = treeModel.appendChildren(fixture, 'a', [kid('a3')]); - const t2 = treeModel.appendChildren(t1, 'a', [kid('a3')]); + it("returns same array reference when every child is a duplicate", () => { + const t1 = treeModel.appendChildren(fixture, "a", [kid("a3")]); + const t2 = treeModel.appendChildren(t1, "a", [kid("a3")]); expect(t2).toBe(t1); }); }); -describe('treeModel.place', () => { - it('moves a node to a new parent at a given index', () => { - const t = treeModel.place(fixture, 'a2', { parentId: 'b', index: 0 }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual(['a1']); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['a2']); +describe("treeModel.place", () => { + it("moves a node to a new parent at a given index", () => { + const t = treeModel.place(fixture, "a2", { parentId: "b", index: 0 }); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual(["a1"]); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["a2"]); }); - it('moves a node to root', () => { - const t = treeModel.place(fixture, 'a1', { parentId: null, index: 0 }); - expect(t.map((n) => n.id)).toEqual(['a1', 'a', 'b']); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual(['a2']); + it("moves a node to root", () => { + const t = treeModel.place(fixture, "a1", { parentId: null, index: 0 }); + expect(t.map((n) => n.id)).toEqual(["a1", "a", "b"]); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual(["a2"]); }); - it('reorders within the same parent', () => { - const t = treeModel.place(fixture, 'a2', { parentId: 'a', index: 0 }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a2', 'a1', + it("reorders within the same parent", () => { + const t = treeModel.place(fixture, "a2", { parentId: "a", index: 0 }); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a2", + "a1", ]); }); - it('returns same array reference for unknown source', () => { - expect( - treeModel.place(fixture, 'ghost', { parentId: 'a', index: 0 }), - ).toBe(fixture); + it("returns same array reference for unknown source", () => { + expect(treeModel.place(fixture, "ghost", { parentId: "a", index: 0 })).toBe( + fixture, + ); }); - it('returns same array reference for unknown destination parent', () => { + it("returns same array reference for unknown destination parent", () => { expect( - treeModel.place(fixture, 'a1', { parentId: 'ghost', index: 0 }), + treeModel.place(fixture, "a1", { parentId: "ghost", index: 0 }), ).toBe(fixture); }); }); -describe('treeModel.placeByPosition', () => { +describe("treeModel.placeByPosition", () => { // Server-authoritative `moveTreeNode` ships the moved node's fractional // `position`; the receiver must sort it into the correct slot among the new // siblings — NOT drop it at index 0. @@ -513,198 +588,221 @@ describe('treeModel.placeByPosition', () => { const tree: P[] = [ { - id: 'dst', - name: 'DST', - position: 'a0', + id: "dst", + name: "DST", + position: "a0", children: [ - { id: 'c1', name: 'C1', position: 'a1' }, - { id: 'c2', name: 'C2', position: 'a3' }, - { id: 'c3', name: 'C3', position: 'a5' }, + { id: "c1", name: "C1", position: "a1" }, + { id: "c2", name: "C2", position: "a3" }, + { id: "c3", name: "C3", position: "a5" }, ], }, - { id: 'src', name: 'SRC', position: 'a9' }, + { id: "src", name: "SRC", position: "a9" }, ]; - it('places the moved node in the MIDDLE of new siblings by position', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a4', + it("places the moved node in the MIDDLE of new siblings by position", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a4", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'src', 'c3', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "src", + "c3", ]); }); - it('places the moved node at the END when its position sorts last', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a8', + it("places the moved node at the END when its position sorts last", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a8", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'c1', 'c2', 'c3', 'src', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "c1", + "c2", + "c3", + "src", ]); }); - it('places the moved node at the FRONT only when its position sorts first', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a0', + it("places the moved node at the FRONT only when its position sorts first", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a0", }); - expect(treeModel.find(t, 'dst')?.children?.map((n) => n.id)).toEqual([ - 'src', 'c1', 'c2', 'c3', + expect(treeModel.find(t, "dst")?.children?.map((n) => n.id)).toEqual([ + "src", + "c1", + "c2", + "c3", ]); }); - it('stamps the authoritative position onto the moved node', () => { - const t = treeModel.placeByPosition(tree, 'src', { - parentId: 'dst', - position: 'a4', + it("stamps the authoritative position onto the moved node", () => { + const t = treeModel.placeByPosition(tree, "src", { + parentId: "dst", + position: "a4", }); - expect(treeModel.find(t, 'src')?.position).toBe('a4'); + expect(treeModel.find(t, "src")?.position).toBe("a4"); }); - it('reorders within the same parent by position (not to index 0)', () => { + it("reorders within the same parent by position (not to index 0)", () => { const same: P[] = [ { - id: 'p', - name: 'P', - position: 'a0', + id: "p", + name: "P", + position: "a0", children: [ - { id: 'x', name: 'X', position: 'a1' }, - { id: 'y', name: 'Y', position: 'a2' }, - { id: 'z', name: 'Z', position: 'a3' }, + { id: "x", name: "X", position: "a1" }, + { id: "y", name: "Y", position: "a2" }, + { id: "z", name: "Z", position: "a3" }, ], }, ]; // Move x to between y and z. - const t = treeModel.placeByPosition(same, 'x', { - parentId: 'p', - position: 'a25', + const t = treeModel.placeByPosition(same, "x", { + parentId: "p", + position: "a25", }); - expect(treeModel.find(t, 'p')?.children?.map((n) => n.id)).toEqual([ - 'y', 'x', 'z', + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual([ + "y", + "x", + "z", ]); }); - it('returns same array reference for unknown source', () => { + it("returns same array reference for unknown source", () => { expect( - treeModel.placeByPosition(tree, 'ghost', { parentId: 'dst', position: 'a4' }), + treeModel.placeByPosition(tree, "ghost", { + parentId: "dst", + position: "a4", + }), ).toBe(tree); }); - it('returns same array reference when destination parent is not loaded', () => { + it("returns same array reference when destination parent is not loaded", () => { expect( - treeModel.placeByPosition(tree, 'src', { parentId: 'ghost', position: 'a4' }), + treeModel.placeByPosition(tree, "src", { + parentId: "ghost", + position: "a4", + }), ).toBe(tree); }); - it('moves a node to root by position', () => { + it("moves a node to root by position", () => { const roots: P[] = [ - { id: 'r1', name: 'R1', position: 'a1' }, - { id: 'r2', name: 'R2', position: 'a5' }, + { id: "r1", name: "R1", position: "a1" }, + { id: "r2", name: "R2", position: "a5" }, { - id: 'rp', - name: 'RP', - position: 'a7', - children: [{ id: 'child', name: 'CHILD', position: 'a1' }], + id: "rp", + name: "RP", + position: "a7", + children: [{ id: "child", name: "CHILD", position: "a1" }], }, ]; - const t = treeModel.placeByPosition(roots, 'child', { + const t = treeModel.placeByPosition(roots, "child", { parentId: null, - position: 'a3', + position: "a3", }); - expect(t.map((n) => n.id)).toEqual(['r1', 'child', 'r2', 'rp']); + expect(t.map((n) => n.id)).toEqual(["r1", "child", "r2", "rp"]); }); }); -describe('treeModel.move', () => { - it('reorder-before within same parent: moves source to target index', () => { - const { tree: t, result } = treeModel.move(fixture, 'a2', { - kind: 'reorder-before', - targetId: 'a1', +describe("treeModel.move", () => { + it("reorder-before within same parent: moves source to target index", () => { + const { tree: t, result } = treeModel.move(fixture, "a2", { + kind: "reorder-before", + targetId: "a1", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a2', 'a1', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a2", + "a1", ]); - expect(result).toEqual({ parentId: 'a', index: 0 }); + expect(result).toEqual({ parentId: "a", index: 0 }); }); - it('reorder-after within same parent', () => { - const { tree: t, result } = treeModel.move(fixture, 'a1', { - kind: 'reorder-after', - targetId: 'a2', + it("reorder-after within same parent", () => { + const { tree: t, result } = treeModel.move(fixture, "a1", { + kind: "reorder-after", + targetId: "a2", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a2', 'a1', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a2", + "a1", ]); - expect(result).toEqual({ parentId: 'a', index: 1 }); + expect(result).toEqual({ parentId: "a", index: 1 }); }); - it('make-child appends at end of target children', () => { - const { tree: t, result } = treeModel.move(fixture, 'b', { - kind: 'make-child', - targetId: 'a', + it("make-child appends at end of target children", () => { + const { tree: t, result } = treeModel.move(fixture, "b", { + kind: "make-child", + targetId: "a", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'a1', 'a2', 'b', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "a1", + "a2", + "b", ]); - expect(result).toEqual({ parentId: 'a', index: 2 }); + expect(result).toEqual({ parentId: "a", index: 2 }); }); - it('make-child initializes children when target had none', () => { - const { tree: t, result } = treeModel.move(fixture, 'a2', { - kind: 'make-child', - targetId: 'b', + it("make-child initializes children when target had none", () => { + const { tree: t, result } = treeModel.move(fixture, "a2", { + kind: "make-child", + targetId: "b", }); - expect(treeModel.find(t, 'b')?.children?.map((n) => n.id)).toEqual(['a2']); - expect(result).toEqual({ parentId: 'b', index: 0 }); + expect(treeModel.find(t, "b")?.children?.map((n) => n.id)).toEqual(["a2"]); + expect(result).toEqual({ parentId: "b", index: 0 }); }); - it('reorder-before across parents', () => { - const { tree: t, result } = treeModel.move(fixture, 'b', { - kind: 'reorder-before', - targetId: 'a1', + it("reorder-before across parents", () => { + const { tree: t, result } = treeModel.move(fixture, "b", { + kind: "reorder-before", + targetId: "a1", }); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual([ - 'b', 'a1', 'a2', + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual([ + "b", + "a1", + "a2", ]); - expect(result).toEqual({ parentId: 'a', index: 0 }); + expect(result).toEqual({ parentId: "a", index: 0 }); }); - it('reorder-after to root', () => { - const { tree: t, result } = treeModel.move(fixture, 'a1', { - kind: 'reorder-after', - targetId: 'a', + it("reorder-after to root", () => { + const { tree: t, result } = treeModel.move(fixture, "a1", { + kind: "reorder-after", + targetId: "a", }); - expect(t.map((n) => n.id)).toEqual(['a', 'a1', 'b']); - expect(treeModel.find(t, 'a')?.children?.map((n) => n.id)).toEqual(['a2']); + expect(t.map((n) => n.id)).toEqual(["a", "a1", "b"]); + expect(treeModel.find(t, "a")?.children?.map((n) => n.id)).toEqual(["a2"]); expect(result).toEqual({ parentId: null, index: 1 }); }); - it('no-op when sourceId === targetId', () => { - const out = treeModel.move(fixture, 'a', { - kind: 'make-child', - targetId: 'a', + it("no-op when sourceId === targetId", () => { + const out = treeModel.move(fixture, "a", { + kind: "make-child", + targetId: "a", }); expect(out.tree).toBe(fixture); }); - it('no-op when target is descendant of source', () => { - const out = treeModel.move(fixture, 'a', { - kind: 'make-child', - targetId: 'a1a', + it("no-op when target is descendant of source", () => { + const out = treeModel.move(fixture, "a", { + kind: "make-child", + targetId: "a1a", }); expect(out.tree).toBe(fixture); }); - it('no-op when source is unknown', () => { - const out = treeModel.move(fixture, 'ghost', { - kind: 'reorder-before', - targetId: 'a', + it("no-op when source is unknown", () => { + const out = treeModel.move(fixture, "ghost", { + kind: "reorder-before", + targetId: "a", }); expect(out.tree).toBe(fixture); }); - it('no-op when target is unknown', () => { - const out = treeModel.move(fixture, 'a1', { - kind: 'reorder-before', - targetId: 'ghost', + it("no-op when target is unknown", () => { + const out = treeModel.move(fixture, "a1", { + kind: "reorder-before", + targetId: "ghost", }); expect(out.tree).toBe(fixture); }); - it('cross-parent move does NOT apply the same-parent adjust (no off-by-one)', () => { + it("cross-parent move does NOT apply the same-parent adjust (no off-by-one)", () => { // Source `x3` sits at index 2 in parent `x`; target `y1` sits at index 0 in // parent `y`. sourceInfo.index (2) > info.index (0) AND the parents differ, // so the `sameParent && source.index < info.index` adjust must be 0 — the @@ -712,36 +810,36 @@ describe('treeModel.move', () => { // drop it at a wrong slot / off-by-one). const crossFixture: N[] = [ { - id: 'x', - name: 'X', + id: "x", + name: "X", children: [ - { id: 'x1', name: 'X1' }, - { id: 'x2', name: 'X2' }, - { id: 'x3', name: 'X3' }, + { id: "x1", name: "X1" }, + { id: "x2", name: "X2" }, + { id: "x3", name: "X3" }, ], }, { - id: 'y', - name: 'Y', + id: "y", + name: "Y", children: [ - { id: 'y1', name: 'Y1' }, - { id: 'y2', name: 'Y2' }, + { id: "y1", name: "Y1" }, + { id: "y2", name: "Y2" }, ], }, ]; - const { tree: t, result } = treeModel.move(crossFixture, 'x3', { - kind: 'reorder-before', - targetId: 'y1', + const { tree: t, result } = treeModel.move(crossFixture, "x3", { + kind: "reorder-before", + targetId: "y1", }); - expect(result).toEqual({ parentId: 'y', index: 0 }); - expect(treeModel.find(t, 'y')?.children?.map((n) => n.id)).toEqual([ - 'x3', - 'y1', - 'y2', + expect(result).toEqual({ parentId: "y", index: 0 }); + expect(treeModel.find(t, "y")?.children?.map((n) => n.id)).toEqual([ + "x3", + "y1", + "y2", ]); - expect(treeModel.find(t, 'x')?.children?.map((n) => n.id)).toEqual([ - 'x1', - 'x2', + expect(treeModel.find(t, "x")?.children?.map((n) => n.id)).toEqual([ + "x1", + "x2", ]); }); }); 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 2dd100aa..922305e7 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -1,4 +1,4 @@ -import type { TreeNode, SiblingsInfo } from './tree-model.types'; +import type { TreeNode, SiblingsInfo } from "./tree-model.types"; function findInternal( nodes: TreeNode[], @@ -19,7 +19,10 @@ export const treeModel = { return findInternal(tree, id)?.node ?? null; }, - path(tree: TreeNode[], id: string): TreeNode[] | null { + path( + tree: TreeNode[], + id: string, + ): TreeNode[] | null { const found = findInternal(tree, id); if (!found) return null; return [...found.parents, found.node]; @@ -123,6 +126,23 @@ export const treeModel = { return treeModel.insert(tree, null, node, index(tree)); } const parent = treeModel.find(tree, parentId); + // The parent is in the tree but its children have NOT been lazy-loaded yet + // (`children === undefined`, distinct from a loaded-but-empty `[]`). Inserting + // here would MATERIALIZE a misleading partial child list (`[node]`) that + // defeats the lazy-load gate — which fetches only when children are + // absent/empty — so the parent's OTHER real children would never load and the + // moved/added node would be the only one shown (a silent data loss, #159 #1). + // Instead, leave the children unloaded and just flag `hasChildren` so the + // chevron appears; expanding fetches the FULL set (including this node). + if (parent && parent.children === undefined) { + return treeModel.update( + tree, + parentId, + // hasChildren is not part of the generic T constraint; tree nodes carry + // it. Cast narrowly so this stays a single, well-understood exception. + { hasChildren: true } as unknown as Omit, "id" | "children">, + ); + } const kids = (parent?.children as TreeNode[] | undefined) ?? []; return treeModel.insert(tree, parentId, node, index(kids)); }, @@ -242,9 +262,10 @@ export const treeModel = { move( tree: TreeNode[], sourceId: string, - op: import('./tree-model.types').DropOp, - ): { tree: TreeNode[]; result: import('./tree-model.types').DropResult } { - if (sourceId === op.targetId) return { tree, result: { parentId: null, index: 0 } }; + op: import("./tree-model.types").DropOp, + ): { tree: TreeNode[]; result: import("./tree-model.types").DropResult } { + if (sourceId === op.targetId) + return { tree, result: { parentId: null, index: 0 } }; if (!treeModel.find(tree, sourceId) || !treeModel.find(tree, op.targetId)) { return { tree, result: { parentId: null, index: 0 } }; } @@ -255,7 +276,7 @@ export const treeModel = { let parentId: string | null; let index: number; - if (op.kind === 'make-child') { + if (op.kind === "make-child") { parentId = op.targetId; const target = treeModel.find(tree, op.targetId)!; index = target.children?.length ?? 0; @@ -264,9 +285,8 @@ export const treeModel = { parentId = info.parentId; const sourceInfo = treeModel.siblingsOf(tree, sourceId)!; const sameParent = sourceInfo.parentId === parentId; - const adjust = - sameParent && sourceInfo.index < info.index ? -1 : 0; - index = info.index + adjust + (op.kind === 'reorder-after' ? 1 : 0); + const adjust = sameParent && sourceInfo.index < info.index ? -1 : 0; + index = info.index + adjust + (op.kind === "reorder-after" ? 1 : 0); } const next = treeModel.place(tree, sourceId, { parentId, index }); diff --git a/apps/client/src/features/page/tree/utils/utils.test.ts b/apps/client/src/features/page/tree/utils/utils.test.ts index 14366b73..f10ab55e 100644 --- a/apps/client/src/features/page/tree/utils/utils.test.ts +++ b/apps/client/src/features/page/tree/utils/utils.test.ts @@ -6,6 +6,7 @@ import { collectBranchIds, openBranches, closeIds, + mergeRootTrees, } from "./utils"; import type { IPage } from "@/features/page/types/page.types.ts"; import type { SpaceTreeNode } from "@/features/page/tree/types.ts"; @@ -44,10 +45,7 @@ function flatNode( } // Nested SpaceTreeNode factory for collectAllIds / collectBranchIds. -function treeNode( - id: string, - children: SpaceTreeNode[] = [], -): SpaceTreeNode { +function treeNode(id: string, children: SpaceTreeNode[] = []): SpaceTreeNode { return { id, slugId: `slug-${id}`, @@ -94,11 +92,7 @@ describe("collectBranchIds", () => { ]), treeNode("root2", [treeNode("leaf3")]), ]; - expect(collectBranchIds(tree).sort()).toEqual([ - "branch1", - "root", - "root2", - ]); + expect(collectBranchIds(tree).sort()).toEqual(["branch1", "root", "root2"]); }); it("returns [] for a leaf-only tree", () => { @@ -273,3 +267,57 @@ describe("closeIds", () => { expect(twice).toEqual({ keep: true, a: false, b: false }); }); }); + +describe("mergeRootTrees (#159 #2 reconnect reconcile)", () => { + // Root node with a position and optional already-loaded children. + function root( + id: string, + position: string, + children?: SpaceTreeNode[], + ): SpaceTreeNode { + return { + id, + slugId: `slug-${id}`, + name: id.toUpperCase(), + icon: undefined, + position, + spaceId: "space-1", + parentPageId: null as unknown as string, + hasChildren: !!children?.length, + children: children as SpaceTreeNode[], + }; + } + + it("DROPS a stale root that is absent from the incoming (authoritative) set", () => { + // 'ghost' was a root before the gap; the server's current roots no longer + // include it (deleted / moved under another page). It must not linger. + const prev = [root("a", "a0"), root("ghost", "a2"), root("b", "a4")]; + const incoming = [root("a", "a0"), root("b", "a4")]; + const merged = mergeRootTrees(prev, incoming); + expect(merged.map((n) => n.id)).toEqual(["a", "b"]); + expect(merged.find((n) => n.id === "ghost")).toBeUndefined(); + }); + + it("PRESERVES a surviving root's lazy-loaded children (subtree not lost on refetch)", () => { + const loadedChild = root("a1", "a0"); + const prev = [root("a", "a0", [loadedChild])]; + // The root query returns only top-level roots (no children). + const incoming = [root("a", "a0")]; + const merged = mergeRootTrees(prev, incoming); + expect(merged[0].children?.map((c) => c.id)).toEqual(["a1"]); + }); + + it("ADDS a new incoming root", () => { + const prev = [root("a", "a0")]; + const incoming = [root("a", "a0"), root("new", "a2")]; + const merged = mergeRootTrees(prev, incoming); + expect(merged.map((n) => n.id)).toEqual(["a", "new"]); + }); + + it("REFRESHES a surviving root's own fields from the incoming copy (e.g. rename)", () => { + const prev = [{ ...root("a", "a0"), name: "OLD" }]; + const incoming = [{ ...root("a", "a0"), name: "NEW" }]; + const merged = mergeRootTrees(prev, incoming); + expect(merged[0].name).toBe("NEW"); + }); +}); diff --git a/apps/client/src/features/page/tree/utils/utils.ts b/apps/client/src/features/page/tree/utils/utils.ts index 53d787c6..04cb51df 100644 --- a/apps/client/src/features/page/tree/utils/utils.ts +++ b/apps/client/src/features/page/tree/utils/utils.ts @@ -214,21 +214,36 @@ export function appendNodeChildren( } /** - * Merge root nodes; keep existing ones intact, append new ones, + * Reconcile the loaded root nodes to the authoritative INCOMING set (the + * server's complete current roots for the space), preserving any lazy-loaded + * children/subtree of a root that still exists. + * + * This runs only once all root pages are fetched, so `incomingRoots` is the full + * server root set and is authoritative for WHICH roots exist: + * - a root in BOTH: kept, with its own fields refreshed from `incoming` (so a + * rename/move during a gap shows) while PRESERVING its previously lazy-loaded + * `children` (expanded subtrees + open-state survive a refetch); + * - a root only in `incoming`: a new root, added as-is; + * - a root only in `prev`: it was DELETED or moved under another page while we + * were not receiving events (e.g. a socket reconnect after a sleep/wifi gap). + * It is DROPPED instead of lingering as a 404 "ghost" root (#159 #2). The old + * append-only merge kept it forever. */ export function mergeRootTrees( prevRoots: SpaceTreeNode[], incomingRoots: SpaceTreeNode[], ): SpaceTreeNode[] { - const seen = new Set(prevRoots.map((r) => r.id)); + const prevById = new Map(prevRoots.map((r) => [r.id, r])); - // add new roots that were not present before - const merged = [...prevRoots]; - incomingRoots.forEach((node) => { - if (!seen.has(node.id)) merged.push(node); + const reconciled = incomingRoots.map((incoming) => { + const prev = prevById.get(incoming.id); + // Preserve the previously loaded children/subtree (the root query returns + // only top-level roots, so `incoming` carries no children); refresh the + // node's own fields from the authoritative incoming copy. + return prev ? { ...incoming, children: prev.children } : incoming; }); - return sortPositionKeys(merged); + return sortPositionKeys(reconciled); } // Collect every node id in the tree (roots, branches, leaves). Used by 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 52228949..f59f27cc 100644 --- a/apps/client/src/features/websocket/tree-socket-reducers.test.ts +++ b/apps/client/src/features/websocket/tree-socket-reducers.test.ts @@ -81,6 +81,38 @@ describe("applyMoveTreeNode", () => { ]); }); + it("does NOT create a partial child list when the destination is loaded-but-collapsed (children unloaded) — keeps it lazy-loadable (#159)", () => { + // `dstCollapsed` is in the tree but its children were never lazy-loaded + // (children === undefined). The OLD behavior inserted `src` as the ONLY + // child ([src]), which defeated the lazy-load gate and HID the parent's + // other real children. Now the move leaves children unloaded (so expanding + // fetches the FULL set, including src) and just flags hasChildren. + const tree: SpaceTreeNode[] = [ + node("dstCollapsed", { + position: "a0", + hasChildren: false, + children: undefined as unknown as SpaceTreeNode[], + }), + node("src", { position: "a9" }), + ]; + const next = applyMoveTreeNode(tree, { + id: "src", + parentId: "dstCollapsed", + oldParentId: null, + index: 0, + position: "a4", + pageData: {}, + }); + const dst = treeModel.find(next, "dstCollapsed"); + // Children stay unloaded -> the lazy-load gate fetches the FULL set (incl. + // src) on expand, rather than showing a misleading partial [src] list. + expect(dst?.children).toBeUndefined(); + expect(dst?.hasChildren).toBe(true); + // src moved away from its old root slot (it lives under dstCollapsed + // server-side and reappears when the parent is expanded/loaded). + expect(next.map((n) => n.id)).not.toContain("src"); + }); + it("flips the OLD parent's hasChildren to false when it is left childless", () => { // src is the only child of `old`; moving it to `dst` empties `old`. const tree: SpaceTreeNode[] = [ @@ -164,7 +196,9 @@ describe("applyDeleteTreeNode", () => { position: "a1", parentPageId: "p", hasChildren: true, - children: [node("grandchild", { position: "a1", parentPageId: "child" })], + children: [ + node("grandchild", { position: "a1", parentPageId: "child" }), + ], }), ], }),