diff --git a/apps/client/src/features/page/tree/components/space-tree.tsx b/apps/client/src/features/page/tree/components/space-tree.tsx index 98317797..64586f9c 100644 --- a/apps/client/src/features/page/tree/components/space-tree.tsx +++ b/apps/client/src/features/page/tree/components/space-tree.tsx @@ -281,10 +281,12 @@ const SpaceTree = forwardRef(function SpaceTree( setOpenTreeNodes((prev) => ({ ...prev, [id]: isOpen })); if (isOpen) { const node = treeModel.find(data, id) as SpaceTreeNode | null; - if ( - node?.hasChildren && - (!node.children || node.children.length === 0) - ) { + // Same "unloaded branch" predicate the realtime insert paths use + // (`isUnloadedBranch`) so the lazy-load gate and the realtime inserts + // (`insertByPosition` / `placeByPosition`) can never disagree about what + // counts as unloaded (#525). Note: local raw `insert` (DnD/create-page) + // does not yet route through it — see #525 follow-up. + if (treeModel.isUnloadedBranch(node)) { const fetched = await fetchAllAncestorChildren({ pageId: id, spaceId: node.spaceId, 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 3b365bf4..4ea060e9 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 @@ -74,6 +74,48 @@ describe("treeModel.isDescendant", () => { }); }); +// #525: the single "is this branch unloaded?" predicate shared by the lazy-load +// gate and the insert paths. Unloaded == server says hasChildren but none are +// present locally (canonical form `children: []`, also `undefined`). A parent +// without hasChildren is genuinely empty, not unloaded. +describe("treeModel.isUnloadedBranch", () => { + type PH = TreeNode<{ name: string; hasChildren?: boolean }>; + it("true for hasChildren + empty array (canonical unloaded form)", () => { + const n: PH = { id: "p", name: "P", hasChildren: true, children: [] }; + expect(treeModel.isUnloadedBranch(n)).toBe(true); + }); + it("true for hasChildren + undefined children", () => { + const n: PH = { id: "p", name: "P", hasChildren: true }; + expect(treeModel.isUnloadedBranch(n)).toBe(true); + }); + it("false for hasChildren + already-loaded children", () => { + const n: PH = { + id: "p", + name: "P", + hasChildren: true, + children: [{ id: "c", name: "C" }], + }; + expect(treeModel.isUnloadedBranch(n)).toBe(false); + }); + it("false for a genuinely-empty parent (no hasChildren)", () => { + expect( + treeModel.isUnloadedBranch({ + id: "p", + name: "P", + hasChildren: false, + children: [], + } as PH), + ).toBe(false); + expect( + treeModel.isUnloadedBranch({ id: "p", name: "P" } as PH), + ).toBe(false); + }); + it("false for null/undefined", () => { + expect(treeModel.isUnloadedBranch(null)).toBe(false); + expect(treeModel.isUnloadedBranch(undefined)).toBe(false); + }); +}); + describe("treeModel.visible", () => { it("returns only root nodes when no openIds", () => { const v = treeModel.visible(fixture, new Set()); @@ -197,43 +239,64 @@ describe("treeModel.insertByPosition", () => { ]); }); - // #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; - }>; + type PH = TreeNode<{ + name: string; + position?: string; + hasChildren?: boolean; + }>; + + // #159 #1 / #525: inserting/moving a node under an UNLOADED parent must NOT + // materialize a partial `[node]` list — that would defeat the lazy-load gate and + // hide the parent's other real children. The canonical unloaded form here is + // `children: []` + `hasChildren: true` (from `pageToTreeNode` / + // `pruneCollapsedChildren`), which the pre-#525 `=== undefined` guard MISSED. + // The node is left to be lazy-loaded; the chevron stays enabled. + it("does NOT materialize a child under an UNLOADED parent (children: [], hasChildren: true)", () => { const tree: PH[] = [ - { id: "p", name: "P", position: "a0", hasChildren: false }, // children: undefined + { id: "p", name: "P", position: "a0", hasChildren: true, children: [] }, ]; 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(); + // full set, including this node, on expand). MUTATION: the pre-#525 predicate + // `children === undefined` does not fire for `[]`, so it would insert `[x]` + // here and reredden this expectation. + expect(parent?.children).toEqual([]); expect(treeModel.find(t, "x")).toBeNull(); - // ...but the chevron is enabled so the user can expand to load it. + // ...and the chevron stays enabled so the user can expand to load it. expect((parent as PH).hasChildren).toBe(true); }); - it("DOES insert under a LOADED-but-empty parent (children: [])", () => { - type PH = TreeNode<{ - name: string; - position?: string; - hasChildren?: boolean; - }>; + it("does NOT materialize a child under an UNLOADED parent (children undefined, hasChildren: true)", () => { + const tree: PH[] = [ + { id: "p", name: "P", position: "a0", hasChildren: true }, // children: undefined + ]; + const node: PH = { id: "x", name: "X", position: "a1" }; + const t = treeModel.insertByPosition(tree, "p", node); + const parent = treeModel.find(t, "p"); + expect(parent?.children).toBeUndefined(); + expect(treeModel.find(t, "x")).toBeNull(); + expect((parent as PH).hasChildren).toBe(true); + }); + + it("DOES insert under a genuinely-empty parent (children: [], hasChildren: false)", () => { 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. + // No server children (`hasChildren: false`), so materializing the first child + // is correct — nothing is hidden. + expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]); + }); + + it("DOES insert under a genuinely-empty parent (children undefined, hasChildren: false)", () => { + 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); expect(treeModel.find(t, "p")?.children?.map((n) => n.id)).toEqual(["x"]); }); 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 bda4a74b..7cdfc797 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -43,6 +43,26 @@ export const treeModel = { }; }, + // A branch is "unloaded" when the server says it HAS children (`hasChildren`) + // but none are present locally. The canonical unloaded form in this codebase + // is `children: []` (produced by `pageToTreeNode` and by `pruneCollapsedChildren` + // resetting collapsed branches), NOT `children: undefined` — so a predicate that + // only checks `=== undefined` misses the real case and materializes a misleading + // partial list (#525). This is the SINGLE source of truth for "should a + // fetch/materialize be deferred?", shared by the lazy-load gate (`handleToggle`) + // and the realtime insert paths (`insertByPosition` / `placeByPosition`), so they + // can never drift apart again. (The local raw `insert` primitive and its DnD/ + // create-page callers do NOT yet route through this predicate — see #525 + // follow-up.) A parent WITHOUT `hasChildren` is genuinely empty + // (no server children) — inserting its first child is correct, not deferred. + isUnloadedBranch( + node: TreeNode | null | undefined, + ): boolean { + if (!node) return false; + const hasChildren = (node as { hasChildren?: boolean }).hasChildren === true; + return hasChildren && (node.children == null || node.children.length === 0); + }, + isDescendant( tree: TreeNode[], ancestorId: string, @@ -127,14 +147,15 @@ export const treeModel = { } 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 + // (`hasChildren` set + children absent/empty — see `isUnloadedBranch`; the + // canonical unloaded form is `children: []`, NOT just `undefined`). 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) { + if (parent && treeModel.isUnloadedBranch(parent)) { return treeModel.update( tree, parentId, 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 ae93a714..43aa642f 100644 --- a/apps/client/src/features/websocket/tree-socket-reducers.test.ts +++ b/apps/client/src/features/websocket/tree-socket-reducers.test.ts @@ -82,17 +82,19 @@ 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. + it("does NOT create a partial child list when the destination is loaded-but-collapsed (children unloaded) — keeps it lazy-loadable (#159 #525)", () => { + // `dstCollapsed` is in the tree but its children were never lazy-loaded. The + // CANONICAL unloaded form here is `hasChildren: true` + `children: []` (from + // `pageToTreeNode` / `pruneCollapsedChildren`), NOT `children: undefined`. + // The pre-#525 predicate (`children === undefined`) missed this form and + // inserted `src` as the ONLY child ([src]), defeating the lazy-load gate and + // HIDING the parent's other real children. Now the move leaves children + // unloaded (so expanding fetches the FULL set, including src). const tree: SpaceTreeNode[] = [ node("dstCollapsed", { position: "a0", - hasChildren: false, - children: undefined as unknown as SpaceTreeNode[], + hasChildren: true, + children: [], }), node("src", { position: "a9" }), ]; @@ -105,9 +107,10 @@ describe("applyMoveTreeNode", () => { 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(); + // Children stay unloaded ([] not materialized to [src]) -> the lazy-load gate + // fetches the FULL set (incl. src) on expand. MUTATION: the pre-#525 + // `=== undefined` predicate would insert [src] here and redden this. + expect(dst?.children).toEqual([]); 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).