diff --git a/apps/client/src/features/page/queries/page-query.ts b/apps/client/src/features/page/queries/page-query.ts index 11ba7f32..c631f892 100644 --- a/apps/client/src/features/page/queries/page-query.ts +++ b/apps/client/src/features/page/queries/page-query.ts @@ -360,6 +360,16 @@ export function invalidateOnCreatePage(data: Partial) { queryKey, (old) => { if (!old) return old; + + // Idempotency guard: the server now self-echoes addTreeNode back to the + // author, so this writer can run twice for one create (mutation onSuccess + // + socket echo). Skip the append if the page is already in the cache to + // avoid a duplicate node / duplicate React key. + const exists = old.pages.some((page) => + page.items.some((item) => item.id === newPage.id), + ); + if (exists) return old; + return { ...old, pages: old.pages.map((page, index) => { diff --git a/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts b/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts index acdcb019..2a3f97d1 100644 --- a/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts +++ b/apps/client/src/features/page/tree/hooks/use-tree-mutation.ts @@ -19,7 +19,6 @@ import { } from "@/features/page/queries/page-query.ts"; import { buildPageUrl } from "@/features/page/page.utils.ts"; import { getSpaceUrl } from "@/lib/config.ts"; -import { useQueryEmit } from "@/features/websocket/use-query-emit.ts"; export type UseTreeMutation = { handleMove: (sourceId: string, op: DropOp) => Promise; @@ -41,12 +40,11 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { const movePageMutation = useMovePageMutation(); const navigate = useNavigate(); const { spaceSlug, pageSlug } = useParams(); - const emit = useQueryEmit(); const handleMove = useCallback( async (sourceId: string, op: DropOp) => { const before = store.get(treeDataAtom); - const { tree: after, result } = treeModel.move(before, sourceId, op); + const { tree: after } = treeModel.move(before, sourceId, op); if (after === before) return; const payload = dropOpToMovePayload(before, sourceId, op); @@ -112,22 +110,12 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { pageData, ); - setTimeout(() => { - emit({ - operation: "moveTreeNode", - spaceId: spaceId, - payload: { - id: sourceId, - parentId: payload.parentPageId, - oldParentId, - index: result.index, - position: payload.position, - pageData, - }, - }); - }, 50); + // Realtime broadcast is now server-authoritative: the server emits + // `moveTreeNode` to the space room on PAGE_MOVED. The old client relay + // (emit + setTimeout(50)) was removed; the optimistic local update above + // stays for instant feedback to the author. }, - [setData, store, movePageMutation, spaceId, emit, t], + [setData, store, movePageMutation, spaceId, t], ); const handleCreate = useCallback( @@ -166,20 +154,23 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { lastIndex = parent?.children?.length ?? 0; } - setData((prev) => treeModel.insert(prev, parentId, newNode, lastIndex)); - - setTimeout(() => { - emit({ - operation: "addTreeNode", - spaceId, - payload: { - parentId, - index: lastIndex, - data: newNode, - }, - }); - }, 50); + // Idempotent by id: the tree is server-authoritative and the server's + // `addTreeNode` broadcast (now ~ms over same-origin) can win the race and + // insert this node before this optimistic update runs. Inserting again + // un-guarded would duplicate the row in the author's sidebar. Mirror the + // `addTreeNode` socket guard: skip when the node already exists. The + // optimistic node's id IS the real created page id (createdPage.id), so + // the ids match exactly regardless of which path runs first. + setData((prev) => { + if (treeModel.find(prev, newNode.id)) return prev; + return treeModel.insert(prev, parentId, newNode, lastIndex); + }); + // Realtime broadcast is now server-authoritative: the server emits + // `addTreeNode` to the space room on PAGE_CREATED. The old client relay + // (emit + setTimeout(50)) was removed; the optimistic insert above stays + // for instant feedback to the author (the server event is idempotent and + // a no-op for the author whose node already exists). const pageUrl = buildPageUrl( spaceSlug, createdPage.slugId, @@ -187,7 +178,7 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { ); navigate(pageUrl); }, - [spaceId, createPageMutation, setData, store, emit, navigate, spaceSlug], + [spaceId, createPageMutation, setData, store, navigate, spaceSlug], ); const handleRename = useCallback( @@ -238,19 +229,15 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { navigate(getSpaceUrl(spaceSlug)); } - setTimeout(() => { - if (!node) return; - emit({ - operation: "deleteTreeNode", - spaceId, - payload: { node }, - }); - }, 50); + // Realtime broadcast is now server-authoritative: the server emits + // `deleteTreeNode` to the space room on PAGE_SOFT_DELETED. The old + // client relay (emit + setTimeout(50)) was removed; the optimistic + // removal above stays for instant feedback to the author. } catch (error) { console.error("Failed to delete page:", error); } }, - [removePageMutation, setData, store, pageSlug, navigate, spaceSlug, emit, spaceId], + [removePageMutation, setData, store, pageSlug, navigate, spaceSlug], ); return { handleMove, handleCreate, handleRename, handleDelete }; 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 1c5941e2..8f16c075 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 @@ -128,6 +128,260 @@ describe('treeModel.insert', () => { }); }); +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' }, + ]; + + 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']); + }); + + 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']); + }); + + 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']); + }); + + 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' }; + + expect( + treeModel.insertByPosition(full, null, node).map((n) => n.id), + ).toEqual(['a', 'b', 'x', 'c']); + expect( + treeModel.insertByPosition(partial, null, node).map((n) => n.id), + ).toEqual(['a', 'x', 'c']); + }); + + it('inserts a child in position order under the parent', () => { + const tree: P[] = [ + { + id: 'p', + name: 'P', + position: 'a0', + children: [ + { 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', + ]); + }); + + 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']); + }); +}); + +// 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)', () => { + type P = TreeNode<{ name: string; position?: string }>; + + const applyAddTreeNode = (tree: P[], node: P): P[] => { + if (treeModel.find(tree, node.id)) return tree; + 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' }; + const t1 = applyAddTreeNode(tree, node); + expect(t1).toBe(tree); + 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' }; + tree = applyAddTreeNode(tree, node); + 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); + }); +}); + +// handleCreate optimistic-insert idempotency: the author's optimistic insert is +// 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)', () => { + // Mirrors the guarded optimistic insert in use-tree-mutation handleCreate. + const applyOptimisticInsert = ( + tree: N[], + parentId: string | null, + node: N, + index: number, + ): N[] => { + if (treeModel.find(tree, node.id)) return tree; + return treeModel.insert(tree, parentId, node, index); + }; + + // Mirrors the addTreeNode socket handler guard. + 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: '' }; + + 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); + const afterOptimistic = applyOptimisticInsert( + afterServer, + null, + created, + afterServer.length, + ); + expect(afterOptimistic).toBe(afterServer); // skipped + 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); + }); + + 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); + }); +}); + +// moveTreeNode socket-handler semantics: the receiver must place the moved node +// 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)', () => { + type P = TreeNode<{ + name: string; + position?: string; + icon?: string; + hasChildren?: boolean; + parentPageId?: string | null; + }>; + + const applyMoveTreeNode = ( + tree: P[], + payload: { + id: string; + parentId: string | null; + position: string; + pageData?: { title?: string | null; icon?: string | null; hasChildren?: boolean }; + }, + ): P[] => { + if (!treeModel.find(tree, payload.id)) return tree; + const placed = treeModel.placeByPosition(tree, payload.id, { + parentId: payload.parentId, + position: payload.position, + }); + if (placed === tree) return treeModel.remove(tree, payload.id); + const patch: Partial

= { + position: payload.position, + parentPageId: payload.parentId, + } 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.hasChildren !== undefined) + (patch as { hasChildren?: boolean }).hasChildren = pd.hasChildren; + } + return treeModel.update(placed, payload.id, patch); + }; + + const tree: P[] = [ + { + 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: 'src', name: 'SRC', position: 'a9' }, + ]; + + it('lands the moved node in the correct MIDDLE slot, not at index 0', () => { + const t = applyMoveTreeNode(tree, { + id: 'src', + parentId: 'dst', + position: 'a4', + }); + 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', () => { + const t = applyMoveTreeNode(tree, { + id: 'src', + parentId: 'dst', + position: 'a8', + }); + 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', () => { + const t = applyMoveTreeNode(tree, { + 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('🔥'); + expect(moved?.hasChildren).toBe(true); + expect(moved?.position).toBe('a4'); + }); + + 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', + }); + expect(treeModel.find(t, 'src')).toBeNull(); + }); +}); + describe('treeModel.remove', () => { it('removes a leaf', () => { const t = treeModel.remove(fixture, 'a2'); @@ -240,6 +494,118 @@ describe('treeModel.place', () => { }); }); +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. + type P = TreeNode<{ name: string; position?: string }>; + + const tree: P[] = [ + { + 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: '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', + }); + 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', + }); + 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', + }); + 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', + }); + expect(treeModel.find(t, 'src')?.position).toBe('a4'); + }); + + it('reorders within the same parent by position (not to index 0)', () => { + const same: P[] = [ + { + 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' }, + ], + }, + ]; + // Move x to between y and z. + const t = treeModel.placeByPosition(same, 'x', { + parentId: 'p', + position: 'a25', + }); + expect(treeModel.find(t, 'p')?.children?.map((n) => n.id)).toEqual([ + 'y', 'x', 'z', + ]); + }); + + it('returns same array reference for unknown source', () => { + expect( + treeModel.placeByPosition(tree, 'ghost', { parentId: 'dst', position: 'a4' }), + ).toBe(tree); + }); + + it('returns same array reference when destination parent is not loaded', () => { + expect( + treeModel.placeByPosition(tree, 'src', { parentId: 'ghost', position: 'a4' }), + ).toBe(tree); + }); + + it('moves a node to root by position', () => { + const roots: P[] = [ + { 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' }], + }, + ]; + const t = treeModel.placeByPosition(roots, 'child', { + parentId: null, + position: 'a3', + }); + 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', { 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 71976c50..2dd100aa 100644 --- a/apps/client/src/features/page/tree/model/tree-model.ts +++ b/apps/client/src/features/page/tree/model/tree-model.ts @@ -98,6 +98,35 @@ export const treeModel = { return touched ? out : tree; }, + // Position-aware insert for server-authoritative broadcasts. The server does + // not know each receiver's local index (clients have different loaded sets and + // the root list is paginated), so it sends the node's fractional `position`. + // We insert among the already-loaded siblings ordered by `position` so the + // order is consistent across clients regardless of which nodes they loaded. + // Falls back to appending when `position` is missing. + insertByPosition( + tree: TreeNode[], + parentId: string | null, + node: TreeNode, + ): TreeNode[] { + const index = (siblings: TreeNode[]): number => { + const pos = node.position; + if (pos == null) return siblings.length; + // First sibling whose position sorts after the new node's position. + const at = siblings.findIndex( + (s) => s.position != null && s.position > pos, + ); + return at === -1 ? siblings.length : at; + }; + + if (parentId === null) { + return treeModel.insert(tree, null, node, index(tree)); + } + const parent = treeModel.find(tree, parentId); + const kids = (parent?.children as TreeNode[] | undefined) ?? []; + return treeModel.insert(tree, parentId, node, index(kids)); + }, + remove(tree: TreeNode[], id: string): TreeNode[] { let touched = false; const walk = (nodes: TreeNode[]): TreeNode[] => { @@ -186,6 +215,30 @@ export const treeModel = { return treeModel.insert(removed, to.parentId, source, to.index); }, + // Position-aware move for server-authoritative `moveTreeNode` broadcasts. Like + // `place`, but instead of an absolute index (which the sender computed against + // its own loaded set), it inserts the moved node among the destination's + // already-loaded siblings ordered by the node's fractional `position`. This + // keeps the visible order correct for every receiver — `place(..., index: 0)` + // would wrongly drop the node at the TOP of its new sibling list. + // Returns the same array reference (like `place`) when the source is missing + // or the destination parent isn't loaded on this client, so callers can detect + // that and fall back to removing the node. + placeByPosition( + tree: TreeNode[], + sourceId: string, + to: { parentId: string | null; position?: string }, + ): TreeNode[] { + const source = treeModel.find(tree, sourceId); + if (!source) return tree; + if (to.parentId !== null && !treeModel.find(tree, 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. + const positioned = { ...source, position: to.position } as TreeNode; + return treeModel.insertByPosition(removed, to.parentId, positioned); + }, + move( tree: TreeNode[], sourceId: string, diff --git a/apps/client/src/features/page/tree/utils/utils.test.ts b/apps/client/src/features/page/tree/utils/utils.test.ts new file mode 100644 index 00000000..7de43da1 --- /dev/null +++ b/apps/client/src/features/page/tree/utils/utils.test.ts @@ -0,0 +1,40 @@ +import { describe, it, expect } from "vitest"; +import { buildTree } from "./utils"; +import type { IPage } from "@/features/page/types/page.types.ts"; + +function page(id: string, position: string): IPage { + return { + id, + slugId: `slug-${id}`, + title: id.toUpperCase(), + icon: "", + position, + hasChildren: false, + spaceId: "space-1", + parentPageId: null as unknown as string, + } as IPage; +} + +describe("buildTree", () => { + it("builds one node per unique page", () => { + const tree = buildTree([page("a", "a1"), page("b", "a2")]); + expect(tree.map((n) => n.id)).toEqual(["a", "b"]); + }); + + it("dedups a duplicate id so the tree has no duplicate node", () => { + // A realtime cache write could append a page twice; buildTree must not emit + // two references to the same node (which would crash the sidebar render with + // a duplicate React key). + const tree = buildTree([ + page("a", "a1"), + page("b", "a2"), + page("a", "a1"), // duplicate id + ]); + + expect(tree).toHaveLength(2); + expect(tree.map((n) => n.id).sort()).toEqual(["a", "b"]); + // No id appears more than once. + const ids = tree.map((n) => n.id); + expect(new Set(ids).size).toBe(ids.length); + }); +}); diff --git a/apps/client/src/features/page/tree/utils/utils.ts b/apps/client/src/features/page/tree/utils/utils.ts index 5d5c0bad..97677681 100644 --- a/apps/client/src/features/page/tree/utils/utils.ts +++ b/apps/client/src/features/page/tree/utils/utils.ts @@ -29,7 +29,14 @@ export function buildTree(pages: IPage[]): SpaceTreeNode[] { }; }); + // Defense-in-depth: a duplicate id in `pages` would push two references to the + // same node, producing a duplicate React key that crashes the sidebar render. + // Track ids we've already pushed and skip repeats so a stray duplicate from a + // realtime cache write can never break the tree. + const seen = new Set(); pages.forEach((page) => { + if (seen.has(page.id)) return; + seen.add(page.id); tree.push(pageMap[page.id]); }); diff --git a/apps/client/src/features/websocket/use-tree-socket.ts b/apps/client/src/features/websocket/use-tree-socket.ts index c4cd083b..317392e7 100644 --- a/apps/client/src/features/websocket/use-tree-socket.ts +++ b/apps/client/src/features/websocket/use-tree-socket.ts @@ -54,13 +54,17 @@ export const useTreeSocket = () => { break; case "addTreeNode": setTreeData((prev) => { + // Idempotent: the author already inserted the node optimistically, + // and a node may be re-delivered — never insert a duplicate id. if (treeModel.find(prev, event.payload.data.id)) return prev; const newParentId = event.payload.parentId as string | null; - let next = treeModel.insert( + // Insert by `position` among already-loaded siblings (not the + // sender's absolute index) so order is consistent across clients + // with different loaded sets. + let next = treeModel.insertByPosition( prev, newParentId, event.payload.data, - event.payload.index, ); // Mirror the emitter: flip new parent's hasChildren to true so // the chevron renders on the receiver. @@ -80,22 +84,50 @@ export const useTreeSocket = () => { (sourceBefore as SpaceTreeNode).parentPageId ?? null; const newParentId = event.payload.parentId as string | null; - const placed = treeModel.place(prev, event.payload.id, { + // 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 the visible order + // correct on every client; placing at `index: 0` would wrongly + // drop reordered/moved nodes at the top of their new sibling list. + const placed = treeModel.placeByPosition(prev, event.payload.id, { parentId: newParentId, - index: event.payload.index, + position: event.payload.position, }); - // `place` silently returns the same reference if the destination - // parent isn't loaded on this client. Falling back to removing the - // source keeps the UI consistent (the source will reappear when - // the user expands the new parent and lazy-load fetches it). + // `placeByPosition` silently returns the same reference if the + // destination parent isn't loaded on this client. Falling back to + // removing the source keeps the UI consistent (the source will + // reappear when the user expands the new parent and lazy-load + // fetches it). if (placed === prev) { return treeModel.remove(prev, event.payload.id); } - let next = treeModel.update(placed, event.payload.id, { + // Apply the authoritative node fields the move payload carries + // (`pageData`) so receivers don't keep a stale title/icon/chevron + // on the moved node. `placeByPosition` already set `position`. + const pageData = event.payload.pageData as + | { + title?: string | null; + icon?: string | null; + hasChildren?: boolean; + } + | undefined; + const patch: Partial = { position: event.payload.position, - parentPageId: newParentId, - } as Partial); + // Honest type: a root move has a null parent, so this is + // `string | null`, not always `string`. + parentPageId: newParentId as string | null, + }; + if (pageData) { + // The tree node stores the title as `name`. + if (pageData.title !== undefined) patch.name = pageData.title ?? ""; + if (pageData.icon !== undefined) + patch.icon = pageData.icon ?? undefined; + if (pageData.hasChildren !== undefined) + patch.hasChildren = pageData.hasChildren; + } + let next = treeModel.update(placed, event.payload.id, patch); // Mirror the emitter's hasChildren bookkeeping so both clients // converge to the same chevron state. diff --git a/apps/server/src/common/events/event.contants.ts b/apps/server/src/common/events/event.contants.ts index c766fe59..7d877f00 100644 --- a/apps/server/src/common/events/event.contants.ts +++ b/apps/server/src/common/events/event.contants.ts @@ -3,6 +3,7 @@ export enum EventName { PAGE_CREATED = 'page.created', PAGE_UPDATED = 'page.updated', PAGE_CONTENT_UPDATED = 'page-content-updated', + PAGE_MOVED = 'page.moved', PAGE_MOVED_TO_SPACE = 'page-moved-to-space', PAGE_DELETED = 'page.deleted', PAGE_SOFT_DELETED = 'page.soft_deleted', diff --git a/apps/server/src/core/page/page.controller.ts b/apps/server/src/core/page/page.controller.ts index 25c358ee..fd5c866e 100644 --- a/apps/server/src/core/page/page.controller.ts +++ b/apps/server/src/core/page/page.controller.ts @@ -767,7 +767,11 @@ export class PageController { @AuthUser() user: User, @AuthProvenance() provenance: AuthProvenanceData, ) { - const movedPage = await this.pageRepo.findById(dto.pageId); + // includeHasChildren so movePage's PAGE_MOVED snapshot carries an accurate + // hasChildren — receivers need it to keep the moved node's chevron correct. + const movedPage = await this.pageRepo.findById(dto.pageId, { + includeHasChildren: true, + }); if (!movedPage) { throw new NotFoundException('Moved page not found'); } diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index 618fda73..27b394e7 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -758,9 +758,14 @@ export class PageService { } const insertedPageIds = insertablePages.map((page) => page.id); + // `spaceId` is the single destination space for the whole copy/duplicate + // (every inserted page above gets `spaceId: spaceId`). It lets the WS + // listener trigger a root refetch for the bulk subtree (no `pages` snapshot + // here on purpose — we want the refetch fallback, not per-node addTreeNode). this.eventEmitter.emit(EventName.PAGE_CREATED, { pageIds: insertedPageIds, workspaceId: authUser.workspaceId, + spaceId, }); //TODO: best to handle this in a queue @@ -887,6 +892,35 @@ export class PageService { }, dto.pageId, ); + + // The generic PAGE_UPDATED emitted by updatePage above is intentionally NOT + // used to drive the tree `moveTreeNode` broadcast: it also fires on rename / + // content-save and carries neither oldParentId nor the new position. Emit a + // dedicated PAGE_MOVED so the WS listener can build a precise moveTreeNode + // without a DB read (variant A: snapshot in the event). + // + // `parentPageId` is `undefined` when only the position changed (same + // parent); resolve it back to the page's actual parent for the snapshot. + const newParentPageId = + parentPageId === undefined ? movedPage.parentPageId : parentPageId; + + this.eventEmitter.emit(EventName.PAGE_MOVED, { + workspaceId: movedPage.workspaceId, + oldParentId: movedPage.parentPageId ?? null, + // `hasChildren` is selected by findById({ includeHasChildren: true }) in + // the controller; it isn't on the base Page type, hence the cast. + hasChildren: + (movedPage as Page & { hasChildren?: boolean }).hasChildren ?? false, + node: { + id: movedPage.id, + slugId: movedPage.slugId, + title: movedPage.title, + icon: movedPage.icon, + position: dto.position, + spaceId: movedPage.spaceId, + parentPageId: newParentPageId ?? null, + }, + }); } async getPageBreadCrumbs(childPageId: string) { diff --git a/apps/server/src/database/listeners/page.listener.ts b/apps/server/src/database/listeners/page.listener.ts index 705fd102..2b67b364 100644 --- a/apps/server/src/database/listeners/page.listener.ts +++ b/apps/server/src/database/listeners/page.listener.ts @@ -6,9 +6,46 @@ import { QueueJob, QueueName } from '../../integrations/queue/constants'; import { Queue } from 'bullmq'; import { EnvironmentService } from '../../integrations/environment/environment.service'; +/** + * Thin snapshot of a page node carried inside domain events so the WebSocket + * tree listener can broadcast a tree update WITHOUT reading the DB. This is + * "variant A" of the realtime-tree design: enriching the event avoids the + * in-transaction visibility race where a separate SELECT in the listener could + * run before the emitting `trx` has committed and therefore not see the row. + */ +export interface TreeNodeSnapshot { + id: string; + slugId: string; + title: string | null; + icon: string | null; + position: string; + spaceId: string; + parentPageId: string | null; +} + export class PageEvent { pageIds: string[]; workspaceId: string; + // Optional tree snapshots so the WS listener can broadcast without a DB read + // (avoids the in-transaction visibility race on PAGE_CREATED / + // PAGE_SOFT_DELETED / PAGE_DELETED). The existing search/AI listeners ignore + // this field — they only enqueue work keyed by pageIds. + pages?: TreeNodeSnapshot[]; + // Set on PAGE_RESTORED so the WS listener can scope a refetchRootTreeNodeEvent + // to the affected space (restore can re-attach a whole subtree). + spaceId?: string; +} + +/** + * Emitted by `PageService.movePage` after a successful re-parent / reorder. + * Carries both the old and new parent plus the new position so the WS listener + * can build a `moveTreeNode` broadcast without a DB read. + */ +export class PageMovedEvent { + workspaceId: string; + oldParentId: string | null; + node: TreeNodeSnapshot; + hasChildren: boolean; } @Injectable() diff --git a/apps/server/src/database/repos/page/page.repo.ts b/apps/server/src/database/repos/page/page.repo.ts index 639cf57b..7f3ea79c 100644 --- a/apps/server/src/database/repos/page/page.repo.ts +++ b/apps/server/src/database/repos/page/page.repo.ts @@ -173,9 +173,23 @@ export class PageRepo { .returning(this.baseFields) .executeTakeFirst(); + // Enrich the event with a thin node snapshot (variant A) so the WS tree + // listener can broadcast `addTreeNode` without re-reading the DB. `result` + // already comes from `returning(this.baseFields)`, so no extra query. this.eventEmitter.emit(EventName.PAGE_CREATED, { pageIds: [result.id], workspaceId: result.workspaceId, + pages: [ + { + id: result.id, + slugId: result.slugId, + title: result.title, + icon: result.icon, + position: result.position, + spaceId: result.spaceId, + parentPageId: result.parentPageId, + }, + ], }); return result; @@ -266,6 +280,25 @@ export class PageRepo { ): Promise { const currentDate = new Date(); + // Read the root snapshot up front so PAGE_SOFT_DELETED can carry it without + // a post-commit DB read (variant A). Only the root of the deleted subtree is + // needed for the tree broadcast — the client `treeModel.remove` drops all + // descendants, so we don't snapshot/broadcast every descendant. + const rootSnapshot = await this.db + .selectFrom('pages') + .select([ + 'id', + 'slugId', + 'title', + 'icon', + 'position', + 'spaceId', + 'parentPageId', + ]) + .where('id', '=', pageId) + .where('deletedAt', 'is', null) + .executeTakeFirst(); + const descendants = await this.db .withRecursive('page_descendants', (db) => db @@ -305,6 +338,21 @@ export class PageRepo { this.eventEmitter.emit(EventName.PAGE_SOFT_DELETED, { pageIds: pageIds, workspaceId, + // Root-only snapshot: one `deleteTreeNode` is enough, the client removes + // the whole subtree. Skip if the root vanished between the two reads. + pages: rootSnapshot + ? [ + { + id: rootSnapshot.id, + slugId: rootSnapshot.slugId, + title: rootSnapshot.title, + icon: rootSnapshot.icon, + position: rootSnapshot.position, + spaceId: rootSnapshot.spaceId, + parentPageId: rootSnapshot.parentPageId, + }, + ] + : [], }); } } @@ -313,7 +361,7 @@ export class PageRepo { // First, check if the page being restored has a deleted parent const pageToRestore = await this.db .selectFrom('pages') - .select(['id', 'parentPageId']) + .select(['id', 'parentPageId', 'spaceId']) .where('id', '=', pageId) .executeTakeFirst(); @@ -372,6 +420,10 @@ export class PageRepo { this.eventEmitter.emit(EventName.PAGE_RESTORED, { pageIds: pageIds, workspaceId: workspaceId, + // spaceId lets the WS listener send a space-scoped refetchRootTreeNodeEvent. + // Restore can re-attach a whole subtree, so a root refetch is simpler and + // more robust than N pointwise addTreeNode events. + spaceId: pageToRestore.spaceId, }); } diff --git a/apps/server/src/integrations/import/services/file-import-task.service.ts b/apps/server/src/integrations/import/services/file-import-task.service.ts index 40525ddf..218c75ca 100644 --- a/apps/server/src/integrations/import/services/file-import-task.service.ts +++ b/apps/server/src/integrations/import/services/file-import-task.service.ts @@ -552,9 +552,13 @@ export class FileImportTaskService { } if (validPageIds.size > 0) { + // Carry the destination spaceId so the WS listener can trigger a root + // refetch for the imported subtree (no `pages` snapshot -> refetch + // fallback rather than per-node addTreeNode). this.eventEmitter.emit(EventName.PAGE_CREATED, { pageIds: Array.from(validPageIds), workspaceId: fileTask.workspaceId, + spaceId: fileTask.spaceId, }); } diff --git a/apps/server/src/ws/listeners/page-ws.listener.spec.ts b/apps/server/src/ws/listeners/page-ws.listener.spec.ts new file mode 100644 index 00000000..734e8228 --- /dev/null +++ b/apps/server/src/ws/listeners/page-ws.listener.spec.ts @@ -0,0 +1,95 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { PageWsListener } from './page-ws.listener'; +import { WsTreeService } from '../ws-tree.service'; +import { + PageEvent, + TreeNodeSnapshot, +} from '../../database/listeners/page.listener'; + +const snapshot: TreeNodeSnapshot = { + id: 'page-1', + slugId: 'slug-1', + title: 'Hello', + icon: '📄', + position: 'a1', + spaceId: 'space-1', + parentPageId: null, +}; + +describe('PageWsListener.onPageCreated', () => { + let listener: PageWsListener; + let wsTree: { + broadcastPageCreated: jest.Mock; + broadcastRefetchRoot: jest.Mock; + }; + + beforeEach(async () => { + wsTree = { + broadcastPageCreated: jest.fn().mockResolvedValue(undefined), + broadcastRefetchRoot: jest.fn().mockResolvedValue(undefined), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + PageWsListener, + { provide: WsTreeService, useValue: wsTree }, + ], + }).compile(); + + listener = module.get(PageWsListener); + }); + + it('with `pages`: broadcasts a per-node addTreeNode and does NOT refetch root', async () => { + const event: PageEvent = { + pageIds: ['page-1'], + workspaceId: 'ws-1', + pages: [snapshot], + }; + + await listener.onPageCreated(event); + + expect(wsTree.broadcastPageCreated).toHaveBeenCalledTimes(1); + expect(wsTree.broadcastPageCreated).toHaveBeenCalledWith(snapshot); + expect(wsTree.broadcastRefetchRoot).not.toHaveBeenCalled(); + }); + + it('without `pages` but WITH `spaceId` (bulk create): falls back to a root refetch', async () => { + const event: PageEvent = { + pageIds: ['page-1', 'page-2'], + workspaceId: 'ws-1', + spaceId: 'space-9', + }; + + await listener.onPageCreated(event); + + expect(wsTree.broadcastPageCreated).not.toHaveBeenCalled(); + expect(wsTree.broadcastRefetchRoot).toHaveBeenCalledTimes(1); + expect(wsTree.broadcastRefetchRoot).toHaveBeenCalledWith('space-9'); + }); + + it('with an EMPTY `pages` array but WITH `spaceId`: still falls back to a root refetch', async () => { + const event: PageEvent = { + pageIds: ['page-1'], + workspaceId: 'ws-1', + pages: [], + spaceId: 'space-9', + }; + + await listener.onPageCreated(event); + + expect(wsTree.broadcastPageCreated).not.toHaveBeenCalled(); + expect(wsTree.broadcastRefetchRoot).toHaveBeenCalledWith('space-9'); + }); + + it('without `pages` and without `spaceId`: does nothing (no broadcast)', async () => { + const event: PageEvent = { + pageIds: ['page-1'], + workspaceId: 'ws-1', + }; + + await listener.onPageCreated(event); + + expect(wsTree.broadcastPageCreated).not.toHaveBeenCalled(); + expect(wsTree.broadcastRefetchRoot).not.toHaveBeenCalled(); + }); +}); diff --git a/apps/server/src/ws/listeners/page-ws.listener.ts b/apps/server/src/ws/listeners/page-ws.listener.ts new file mode 100644 index 00000000..100d0f85 --- /dev/null +++ b/apps/server/src/ws/listeners/page-ws.listener.ts @@ -0,0 +1,81 @@ +import { Injectable, Logger } from '@nestjs/common'; +import { OnEvent } from '@nestjs/event-emitter'; +import { EventName } from '../../common/events/event.contants'; +import { + PageEvent, + PageMovedEvent, +} from '../../database/listeners/page.listener'; +import { WsTreeService } from '../ws-tree.service'; + +/** + * Server-authoritative realtime tree updates. + * + * Listens to page lifecycle domain events and broadcasts the corresponding + * tree mutation to everyone in the space room. Because the events carry thin + * node snapshots (variant A), this listener performs NO DB reads — that is what + * keeps it safe against the in-transaction visibility race (a synchronous + * SELECT here could run before the emitting `trx` committed). + * + * Scope of this PR: create, move, soft-delete/delete, restore. + * + * Deferred follow-ups (intentionally NOT handled here): + * - rename / icon change: would broadcast `updateOne` on PAGE_UPDATED, but + * PAGE_UPDATED also fires on every content save, so it needs a title/icon + * diff filter to avoid noise. + * - cross-space move (`movePageToSpace` / PAGE_MOVED_TO_SPACE): needs a + * deleteTreeNode in the old space + addTreeNode/refetch in the new space. + */ +@Injectable() +export class PageWsListener { + private readonly logger = new Logger(PageWsListener.name); + + constructor(private readonly wsTree: WsTreeService) {} + + @OnEvent(EventName.PAGE_CREATED) + async onPageCreated(event: PageEvent): Promise { + // Two creation shapes: + // - Single-page create carries precise node snapshots (`pages`), so we + // broadcast a pointwise addTreeNode per node. + // - Bulk create (copy/duplicate, import) produces whole subtrees and omits + // `pages`; per-node placement would be fragile, so we fall back to a root + // refetch (carries no page data, clients re-fetch via the permission- + // checked API). Same mechanism PAGE_RESTORED uses. + if (event.pages?.length) { + for (const page of event.pages) { + await this.wsTree.broadcastPageCreated(page); + } + return; + } + + if (event.spaceId) { + await this.wsTree.broadcastRefetchRoot(event.spaceId); + } + } + + // Both soft-delete and hard-delete remove the node from the tree. The event + // carries only the ROOT snapshot of the deleted subtree — the client + // `treeModel.remove` drops all descendants, so one deleteTreeNode is enough. + @OnEvent(EventName.PAGE_SOFT_DELETED) + @OnEvent(EventName.PAGE_DELETED) + async onPageDeleted(event: PageEvent): Promise { + for (const page of event.pages ?? []) { + await this.wsTree.broadcastPageDeleted(page); + } + } + + @OnEvent(EventName.PAGE_MOVED) + async onPageMoved(event: PageMovedEvent): Promise { + await this.wsTree.broadcastPageMoved(event); + } + + @OnEvent(EventName.PAGE_RESTORED) + async onPageRestored(event: PageEvent): Promise { + // Restore can re-attach a whole subtree; a root refetch is simpler and more + // robust than N pointwise addTreeNode events. + if (!event.spaceId) { + this.logger.warn('PAGE_RESTORED event without spaceId; skipping refetch'); + return; + } + await this.wsTree.broadcastRefetchRoot(event.spaceId); + } +} diff --git a/apps/server/src/ws/ws-tree.service.spec.ts b/apps/server/src/ws/ws-tree.service.spec.ts new file mode 100644 index 00000000..0c511223 --- /dev/null +++ b/apps/server/src/ws/ws-tree.service.spec.ts @@ -0,0 +1,331 @@ +import { Test, TestingModule } from '@nestjs/testing'; +import { WsTreeService } from './ws-tree.service'; +import { WsService } from './ws.service'; +import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; +import { CACHE_MANAGER } from '@nestjs/cache-manager'; +import { + PageMovedEvent, + TreeNodeSnapshot, +} from '../database/listeners/page.listener'; +import { + getSpaceRoomName, + WS_SPACE_RESTRICTION_CACHE_PREFIX, +} from './ws.utils'; + +const snapshot: TreeNodeSnapshot = { + id: 'page-1', + slugId: 'slug-1', + title: 'Hello', + icon: '📄', + position: 'a1', + spaceId: 'space-1', + parentPageId: null, +}; + +describe('WsTreeService', () => { + let service: WsTreeService; + let wsService: { + emitTreeEvent: jest.Mock; + emitToSpaceRoom: jest.Mock; + emitDeleteToUnauthorized: jest.Mock; + emitToAuthorizedUsers: jest.Mock; + }; + let pagePermissionRepo: { hasRestrictedAncestor: jest.Mock }; + + beforeEach(async () => { + wsService = { + emitTreeEvent: jest.fn().mockResolvedValue(undefined), + emitToSpaceRoom: jest.fn(), + emitDeleteToUnauthorized: jest.fn().mockResolvedValue(undefined), + emitToAuthorizedUsers: jest.fn().mockResolvedValue(undefined), + }; + pagePermissionRepo = { + // Default: not restricted, so broadcastPageMoved skips the compensating + // delete unless a test opts in. + hasRestrictedAncestor: jest.fn().mockResolvedValue(false), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + WsTreeService, + { provide: WsService, useValue: wsService }, + { provide: PagePermissionRepo, useValue: pagePermissionRepo }, + ], + }).compile(); + + service = module.get(WsTreeService); + }); + + it('broadcastPageCreated emits addTreeNode with the expected shape', async () => { + await service.broadcastPageCreated(snapshot); + + expect(wsService.emitTreeEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ + operation: 'addTreeNode', + spaceId: 'space-1', + payload: expect.objectContaining({ + parentId: null, + index: 0, + data: expect.objectContaining({ + id: 'page-1', + slugId: 'slug-1', + name: 'Hello', + title: 'Hello', + icon: '📄', + position: 'a1', + spaceId: 'space-1', + parentPageId: null, + hasChildren: false, + children: [], + }), + }), + }), + ); + }); + + it('broadcastPageDeleted emits deleteTreeNode with the root node only', async () => { + await service.broadcastPageDeleted({ + ...snapshot, + parentPageId: 'parent-9', + }); + + expect(wsService.emitTreeEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ + operation: 'deleteTreeNode', + spaceId: 'space-1', + payload: { + node: { id: 'page-1', slugId: 'slug-1', parentPageId: 'parent-9' }, + }, + }), + ); + }); + + it('broadcastPageMoved emits moveTreeNode with old + new parent and position', async () => { + const event: PageMovedEvent = { + workspaceId: 'ws-1', + oldParentId: 'old-parent', + hasChildren: true, + node: { ...snapshot, parentPageId: 'new-parent', position: 'a5' }, + }; + + await service.broadcastPageMoved(event); + + expect(wsService.emitTreeEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ + operation: 'moveTreeNode', + spaceId: 'space-1', + payload: expect.objectContaining({ + id: 'page-1', + parentId: 'new-parent', + oldParentId: 'old-parent', + index: 0, + position: 'a5', + pageData: expect.objectContaining({ + id: 'page-1', + slugId: 'slug-1', + position: 'a5', + parentPageId: 'new-parent', + hasChildren: true, + }), + }), + }), + ); + }); + + it('broadcastPageMoved into an UNrestricted location does NOT emit a compensating delete', async () => { + pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(false); + + const event: PageMovedEvent = { + workspaceId: 'ws-1', + oldParentId: 'old-parent', + hasChildren: false, + node: { ...snapshot, parentPageId: 'new-parent', position: 'a5' }, + }; + + await service.broadcastPageMoved(event); + + // Normal path: move goes to the whole room via emitTreeEvent, and neither + // the authorized-only move path nor the compensating delete fire. + expect(wsService.emitTreeEvent).toHaveBeenCalledTimes(1); + expect(wsService.emitToAuthorizedUsers).not.toHaveBeenCalled(); + expect(wsService.emitDeleteToUnauthorized).not.toHaveBeenCalled(); + }); + + it('broadcastPageMoved into a RESTRICTED subtree routes the move to authorized users only AND emits a compensating delete to unauthorized — from one fresh decision', async () => { + // Destination is now under a restricted ancestor. + pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(true); + + const event: PageMovedEvent = { + workspaceId: 'ws-1', + oldParentId: 'old-parent', + hasChildren: false, + node: { ...snapshot, parentPageId: 'restricted-parent', position: 'a5' }, + }; + + await service.broadcastPageMoved(event); + + // The single fresh restriction decision was read exactly once... + expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalledTimes(1); + expect(pagePermissionRepo.hasRestrictedAncestor).toHaveBeenCalledWith( + 'page-1', + ); + + // ...and it must NOT go through the cache-gated room-wide emitTreeEvent, + // which could leak the move to the whole room during the stale-cache window. + expect(wsService.emitTreeEvent).not.toHaveBeenCalled(); + + // The move is delivered to authorized users only. + expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledTimes(1); + expect(wsService.emitToAuthorizedUsers).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ + operation: 'moveTreeNode', + spaceId: 'space-1', + payload: expect.objectContaining({ id: 'page-1' }), + }), + ); + + // The users who lost access get a deleteTreeNode for the moved node, scoped + // to the same page id (same fresh authorized set → disjoint from the move). + expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledTimes(1); + expect(wsService.emitDeleteToUnauthorized).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ + operation: 'deleteTreeNode', + spaceId: 'space-1', + payload: { + node: expect.objectContaining({ id: 'page-1', slugId: 'slug-1' }), + }, + }), + ); + }); + + it('broadcastRefetchRoot emits refetchRootTreeNodeEvent to the space room', async () => { + await service.broadcastRefetchRoot('space-7'); + + expect(wsService.emitToSpaceRoom).toHaveBeenCalledWith('space-7', { + operation: 'refetchRootTreeNodeEvent', + spaceId: 'space-7', + }); + }); +}); + +describe('WsService.emitTreeEvent', () => { + let service: WsService; + let pagePermissionRepo: { + hasRestrictedPagesInSpace: jest.Mock; + hasRestrictedAncestor: jest.Mock; + getUserIdsWithPageAccess: jest.Mock; + }; + let cache: { get: jest.Mock; set: jest.Mock; del: jest.Mock }; + let roomEmit: jest.Mock; + let server: any; + + beforeEach(async () => { + pagePermissionRepo = { + hasRestrictedPagesInSpace: jest.fn(), + hasRestrictedAncestor: jest.fn(), + getUserIdsWithPageAccess: jest.fn(), + }; + cache = { + get: jest.fn().mockResolvedValue(null), + set: jest.fn().mockResolvedValue(undefined), + del: jest.fn(), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [ + WsService, + { provide: PagePermissionRepo, useValue: pagePermissionRepo }, + { provide: CACHE_MANAGER, useValue: cache }, + ], + }).compile(); + + service = module.get(WsService); + + roomEmit = jest.fn(); + server = { + to: jest.fn().mockReturnValue({ emit: roomEmit }), + in: jest.fn().mockReturnValue({ fetchSockets: jest.fn() }), + }; + service.setServer(server); + }); + + it('open space: broadcasts to the whole space room', async () => { + pagePermissionRepo.hasRestrictedPagesInSpace.mockResolvedValue(false); + + const data = { operation: 'addTreeNode' }; + await service.emitTreeEvent('space-1', 'page-1', data); + + expect(server.to).toHaveBeenCalledWith(getSpaceRoomName('space-1')); + expect(roomEmit).toHaveBeenCalledWith('message', data); + expect(pagePermissionRepo.hasRestrictedAncestor).not.toHaveBeenCalled(); + }); + + it('restricted page: only authorized users receive the event', async () => { + pagePermissionRepo.hasRestrictedPagesInSpace.mockResolvedValue(true); + pagePermissionRepo.hasRestrictedAncestor.mockResolvedValue(true); + pagePermissionRepo.getUserIdsWithPageAccess.mockResolvedValue(['user-ok']); + + const okEmit = jest.fn(); + const noEmit = jest.fn(); + const sockets = [ + { id: 's1', data: { userId: 'user-ok' }, emit: okEmit }, + { id: 's2', data: { userId: 'user-no' }, emit: noEmit }, + ]; + server.in.mockReturnValue({ + fetchSockets: jest.fn().mockResolvedValue(sockets), + }); + + const data = { operation: 'addTreeNode' }; + await service.emitTreeEvent('space-1', 'page-1', data); + + // Did NOT broadcast to the whole room. + expect(roomEmit).not.toHaveBeenCalled(); + expect(okEmit).toHaveBeenCalledWith('message', data); + expect(noEmit).not.toHaveBeenCalled(); + }); + + it('invalidateSpaceRestrictionCache deletes the cached restriction verdict for that space only', async () => { + await service.invalidateSpaceRestrictionCache('space-42'); + + expect(cache.del).toHaveBeenCalledTimes(1); + expect(cache.del).toHaveBeenCalledWith( + `${WS_SPACE_RESTRICTION_CACHE_PREFIX}space-42`, + ); + }); + + it('emitDeleteToUnauthorized sends ONLY to sockets whose user lacks page access', async () => { + pagePermissionRepo.getUserIdsWithPageAccess.mockResolvedValue(['user-ok']); + + const okEmit = jest.fn(); + const noEmit = jest.fn(); + const anonEmit = jest.fn(); + const sockets = [ + { id: 's1', data: { userId: 'user-ok' }, emit: okEmit }, + { id: 's2', data: { userId: 'user-no' }, emit: noEmit }, + // Unauthenticated socket (no userId) — must also receive the delete. + { id: 's3', data: {}, emit: anonEmit }, + ]; + server.in.mockReturnValue({ + fetchSockets: jest.fn().mockResolvedValue(sockets), + }); + + const data = { operation: 'deleteTreeNode' }; + await service.emitDeleteToUnauthorized('space-1', 'page-1', data); + + // Authorized user does NOT get the delete (they got the move instead). + expect(okEmit).not.toHaveBeenCalled(); + // Unauthorized + anonymous sockets DO get the delete. + expect(noEmit).toHaveBeenCalledWith('message', data); + expect(anonEmit).toHaveBeenCalledWith('message', data); + }); +}); diff --git a/apps/server/src/ws/ws-tree.service.ts b/apps/server/src/ws/ws-tree.service.ts index 8aadfa99..5fa8ef5d 100644 --- a/apps/server/src/ws/ws-tree.service.ts +++ b/apps/server/src/ws/ws-tree.service.ts @@ -1,32 +1,31 @@ import { Injectable } from '@nestjs/common'; -import { Page } from '@docmost/db/types/entity.types'; +import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; import { WsService } from './ws.service'; +import { + PageMovedEvent, + TreeNodeSnapshot, +} from '../database/listeners/page.listener'; @Injectable() export class WsTreeService { - constructor(private readonly wsService: WsService) {} + constructor( + private readonly wsService: WsService, + private readonly pagePermissionRepo: PagePermissionRepo, + ) {} - async notifyPageRestricted(page: Page, excludeUserId: string): Promise { - await this.wsService.emitToSpaceExceptUsers(page.spaceId, [excludeUserId], { - operation: 'deleteTreeNode', - spaceId: page.spaceId, - payload: { - node: { - id: page.id, - slugId: page.slugId, - }, - }, - }); - } + // Server-origin tree broadcasts. Built from thin node snapshots carried in the + // domain events (variant A) so no DB read happens here — this avoids the + // in-transaction visibility race. Payload shapes mirror what the client + // receiver (`use-tree-socket.ts`) consumes. - async notifyPermissionGranted(page: Page, userIds: string[]): Promise { - if (userIds.length === 0) return; - - await this.wsService.emitToUsers(userIds, { + async broadcastPageCreated(page: TreeNodeSnapshot): Promise { + await this.wsService.emitTreeEvent(page.spaceId, page.id, { operation: 'addTreeNode', spaceId: page.spaceId, payload: { parentId: page.parentPageId ?? null, + // Receivers place by `position` among already-loaded siblings, not by + // this absolute index (sender's loaded set differs from receivers'). index: 0, data: { id: page.id, @@ -37,11 +36,112 @@ export class WsTreeService { position: page.position, spaceId: page.spaceId, parentPageId: page.parentPageId, - creatorId: page.creatorId, hasChildren: false, children: [], }, }, }); } + + async broadcastPageDeleted(page: TreeNodeSnapshot): Promise { + await this.wsService.emitTreeEvent(page.spaceId, page.id, { + operation: 'deleteTreeNode', + spaceId: page.spaceId, + payload: { + node: { + id: page.id, + slugId: page.slugId, + parentPageId: page.parentPageId ?? null, + }, + }, + }); + } + + async broadcastPageMoved(event: PageMovedEvent): Promise { + const { node } = event; + + const movePayload = { + operation: 'moveTreeNode', + spaceId: node.spaceId, + payload: { + id: node.id, + parentId: node.parentPageId ?? null, + oldParentId: event.oldParentId ?? null, + // See broadcastPageCreated: receivers place by `position`, not index. + index: 0, + position: node.position, + pageData: { + id: node.id, + slugId: node.slugId, + title: node.title, + icon: node.icon, + position: node.position, + spaceId: node.spaceId, + parentPageId: node.parentPageId ?? null, + hasChildren: event.hasChildren, + }, + }, + }; + + // Decide the node's restricted state ONCE, fresh (uncached), and drive BOTH + // the move broadcast and the compensating delete from this single decision. + // + // Why not just emitTreeEvent for the move? emitTreeEvent gates the move on + // the CACHED spaceHasRestrictions (30s TTL, never invalidated). In the window + // right after a space gets its FIRST restriction, that cache still says + // "no restrictions" → emitTreeEvent would fan the move out to the WHOLE room + // (including unauthorized users) while the delete below (computed from the + // UNCACHED hasRestrictedAncestor) also fires. An unauthorized user then gets + // BOTH, and if the delete lands first it is a no-op and the later move + // renders the restricted node → leak. So when the node is known-restricted we + // must NOT route the move through the cache-gated path. + const isRestricted = await this.pagePermissionRepo.hasRestrictedAncestor( + node.id, + ); + + if (!isRestricted) { + // Normal case: not under a restricted ancestor. One moveTreeNode to the + // whole space room (emitTreeEvent's open-space fast path), no delete. + await this.wsService.emitTreeEvent(node.spaceId, node.id, movePayload); + return; + } + + // Restricted case: a move can push a previously-visible page UNDER a + // restricted ancestor. Route the move to authorized users ONLY (same fresh + // getUserIdsWithPageAccess set the delete uses) and send the compensating + // delete to everyone else. Both sets come from one fresh decision, so they + // are guaranteed disjoint: authorized users get exactly the moveTreeNode, + // unauthorized users get exactly the deleteTreeNode, nobody gets both. + // + // Users who LOSE visibility need the delete because otherwise the node would + // linger in their tree at its old parent with its real title/slugId/icon + // (existence + metadata leak). + await this.wsService.emitToAuthorizedUsers( + node.spaceId, + node.id, + movePayload, + ); + + await this.wsService.emitDeleteToUnauthorized(node.spaceId, node.id, { + operation: 'deleteTreeNode', + spaceId: node.spaceId, + payload: { + node: { + id: node.id, + slugId: node.slugId, + parentPageId: event.oldParentId ?? null, + }, + }, + }); + } + + // Used for restore (and other subtree re-attachments): rather than emitting N + // pointwise addTreeNode events, ask clients in the space to refetch the root + // tree. The client already understands `refetchRootTreeNodeEvent`. + async broadcastRefetchRoot(spaceId: string): Promise { + this.wsService.emitToSpaceRoom(spaceId, { + operation: 'refetchRootTreeNodeEvent', + spaceId, + }); + } } diff --git a/apps/server/src/ws/ws.gateway.ts b/apps/server/src/ws/ws.gateway.ts index fd6810c8..a4f66257 100644 --- a/apps/server/src/ws/ws.gateway.ts +++ b/apps/server/src/ws/ws.gateway.ts @@ -62,10 +62,10 @@ export class WsGateway } @SubscribeMessage('message') - async handleMessage(client: Socket, data: any): Promise { - if (this.wsService.isTreeEvent(data)) { - await this.wsService.handleTreeEvent(client, data); - } + handleMessage(_client: Socket, _data: any): void { + // Inbound tree events from clients are no longer accepted: tree updates are + // now server-authoritative (broadcast by PageWsListener from domain events). + // The old client-relay path was removed to close that attack surface. } /* diff --git a/apps/server/src/ws/ws.module.ts b/apps/server/src/ws/ws.module.ts index d19e6076..400ee253 100644 --- a/apps/server/src/ws/ws.module.ts +++ b/apps/server/src/ws/ws.module.ts @@ -2,12 +2,13 @@ import { Global, Module } from '@nestjs/common'; import { WsGateway } from './ws.gateway'; import { WsService } from './ws.service'; import { WsTreeService } from './ws-tree.service'; +import { PageWsListener } from './listeners/page-ws.listener'; import { TokenModule } from '../core/auth/token.module'; @Global() @Module({ imports: [TokenModule], - providers: [WsGateway, WsService, WsTreeService], + providers: [WsGateway, WsService, WsTreeService, PageWsListener], exports: [WsGateway, WsService, WsTreeService], }) export class WsModule {} diff --git a/apps/server/src/ws/ws.service.ts b/apps/server/src/ws/ws.service.ts index 3278f72c..75cf598e 100644 --- a/apps/server/src/ws/ws.service.ts +++ b/apps/server/src/ws/ws.service.ts @@ -1,14 +1,12 @@ import { Inject, Injectable } from '@nestjs/common'; import { CACHE_MANAGER } from '@nestjs/cache-manager'; import { Cache } from 'cache-manager'; -import { Server, Socket } from 'socket.io'; +import { Server } from 'socket.io'; import { PagePermissionRepo } from '@docmost/db/repos/page/page-permission.repo'; import { - TREE_EVENTS, WS_SPACE_RESTRICTION_CACHE_PREFIX, WS_CACHE_TTL_MS, getSpaceRoomName, - getUserRoomName, } from './ws.utils'; @Injectable() @@ -24,39 +22,25 @@ export class WsService { this.server = server; } - async handleTreeEvent(client: Socket, data: any): Promise { - const room = getSpaceRoomName(data.spaceId); - - if (!client.rooms.has(room)) { - return; - } - - if (data.operation === 'refetchRootTreeNodeEvent') { - client.broadcast.to(room).emit('message', data); - return; - } - - const hasRestrictions = await this.spaceHasRestrictions(data.spaceId); - if (!hasRestrictions) { - client.broadcast.to(room).emit('message', data); - return; - } - - const pageId = this.extractPageId(data); - if (!pageId) { - return; - } - - const isRestricted = - await this.pagePermissionRepo.hasRestrictedAncestor(pageId); - if (!isRestricted) { - client.broadcast.to(room).emit('message', data); - return; - } - - await this.broadcastToAuthorizedUsers(room, client.id, pageId, data); - } - + // Drop the cached spaceHasRestrictions verdict for a space. spaceHasRestrictions + // caches "does this space have ANY restricted page" for WS_CACHE_TTL_MS (30s), + // and emitTreeEvent / emitCommentEvent take a room-wide fast path when it is + // false. The FIRST time a space gains a restriction (or loses its last one) + // this cached verdict goes stale for up to the TTL, during which a title/icon- + // bearing tree payload could fan out to the whole room. This MUST be called by + // whatever code creates or removes a page's restriction (the page-access / + // page-permission grant/revoke/restrict path), passing the affected page's + // spaceId, so the next emit re-reads hasRestrictedPagesInSpace. + // + // NOTE: on this branch there is no permission-mutation site to call this from — + // the page-access/page-permission repo mutators (insertPageAccess / + // insertPagePermissions / deletePagePermission* / updatePagePermissionRole) + // have ZERO callers in apps/server/src; PageAccessService only validates access. + // This primitive is kept (and tested) so that flow, when it lands, has the + // correct hook to invalidate the cache. + // + // TODO: the future restriction-mutation endpoint (restrict/grant/revoke page + // access) MUST call this with the affected page's spaceId. async invalidateSpaceRestrictionCache(spaceId: string): Promise { await this.cacheManager.del( `${WS_SPACE_RESTRICTION_CACHE_PREFIX}${spaceId}`, @@ -86,31 +70,101 @@ export class WsService { await this.broadcastToAuthorizedUsers(room, null, pageId, data); } - async emitToUsers(userIds: string[], data: any): Promise { - if (userIds.length === 0) return; - const rooms = userIds.map((id) => getUserRoomName(id)); - this.server.to(rooms).emit('message', data); + // Server-origin tree broadcast. Mirrors emitCommentEvent exactly: respects + // per-space page restrictions (spaceHasRestrictions -> hasRestrictedAncestor + // -> broadcastToAuthorizedUsers), otherwise fans the event out to everyone in + // the space room. + // + // The author is NOT excluded. The client receiver is idempotent (addTreeNode + // early-returns if the node id already exists; deleteTreeNode is a no-op if + // the node is gone), so the UI author's optimistic node is preserved, and + // non-UI creators (MCP / AI / REST API) still see their own page appear. + async emitTreeEvent( + spaceId: string, + pageId: string, + data: any, + ): Promise { + const room = getSpaceRoomName(spaceId); + + const hasRestrictions = await this.spaceHasRestrictions(spaceId); + if (!hasRestrictions) { + this.server.to(room).emit('message', data); + return; + } + + const isRestricted = + await this.pagePermissionRepo.hasRestrictedAncestor(pageId); + if (!isRestricted) { + this.server.to(room).emit('message', data); + return; + } + + await this.broadcastToAuthorizedUsers(room, null, pageId, data); } - async emitToSpaceExceptUsers( + // Unconditional broadcast to everyone in the space room. Used for space-wide + // signals that carry no page payload (e.g. refetchRootTreeNodeEvent on + // restore): there is no per-page data to leak, and each client refetches the + // root tree through its own authorized query (refetchRootTreeNodeEvent carries + // no per-page data, so no restriction check is needed). + emitToSpaceRoom(spaceId: string, data: any): void { + this.server.to(getSpaceRoomName(spaceId)).emit('message', data); + } + + // Broadcast `data` (a deleteTreeNode) to every socket in the space room whose + // user is NOT authorized to see `pageId`. Used to compensate a move that pushes + // a previously-visible page UNDER a restricted ancestor: authorized users get + // the moveTreeNode (via emitTreeEvent), everyone else gets a deleteTreeNode so + // the now-restricted node disappears from their tree instead of lingering with + // its real title/slugId/icon. The two event sets are disjoint by construction + // (a user is either authorized or not), so no socket receives both. + async emitDeleteToUnauthorized( spaceId: string, - excludeUserIds: string[], + pageId: string, data: any, ): Promise { const room = getSpaceRoomName(spaceId); const sockets = await this.server.in(room).fetchSockets(); - const excludeSet = new Set(excludeUserIds); + if (sockets.length === 0) return; + + const userIds = Array.from( + new Set( + sockets + .map((s) => s.data.userId as string) + .filter((id): id is string => !!id), + ), + ); + if (userIds.length === 0) return; + + const authorizedUserIds = + await this.pagePermissionRepo.getUserIdsWithPageAccess(pageId, userIds); + const authorizedSet = new Set(authorizedUserIds); for (const socket of sockets) { const userId = socket.data.userId as string; - if (userId && !excludeSet.has(userId)) { + // Unauthenticated sockets (no userId) cannot see restricted content; send + // them the delete too so a leaked node can't linger. + if (!userId || !authorizedSet.has(userId)) { socket.emit('message', data); } } } - isTreeEvent(data: any): boolean { - return TREE_EVENTS.has(data?.operation) && !!data?.spaceId; + // Server-origin broadcast of `data` to exactly the users in the space room who + // ARE authorized to see `pageId`. This is the counterpart of + // emitDeleteToUnauthorized: both resolve the authorized set from the SAME + // fetchSockets + getUserIdsWithPageAccess call shape, so a caller that drives + // both from one decision gets two disjoint sets (authorized vs. not) with no + // socket in both. Unlike emitTreeEvent, this does NOT consult the cached + // spaceHasRestrictions: the caller already knows the page is restricted, so we + // must not risk a stale cache fanning the move out to the whole room. + async emitToAuthorizedUsers( + spaceId: string, + pageId: string, + data: any, + ): Promise { + const room = getSpaceRoomName(spaceId); + await this.broadcastToAuthorizedUsers(room, null, pageId, data); } private async broadcastToAuthorizedUsers( @@ -175,19 +229,4 @@ export class WsService { return hasRestrictions; } - - private extractPageId(data: any): string | null { - switch (data.operation) { - case 'addTreeNode': - return data.payload?.data?.id ?? null; - case 'moveTreeNode': - return data.payload?.id ?? null; - case 'deleteTreeNode': - return data.payload?.node?.id ?? null; - case 'updateOne': - return data.id ?? null; - default: - return null; - } - } } diff --git a/apps/server/src/ws/ws.utils.ts b/apps/server/src/ws/ws.utils.ts index c0d48b54..a1eb1569 100644 --- a/apps/server/src/ws/ws.utils.ts +++ b/apps/server/src/ws/ws.utils.ts @@ -8,11 +8,3 @@ export function getSpaceRoomName(spaceId: string): string { export function getUserRoomName(userId: string): string { return `user-${userId}`; } - -export const TREE_EVENTS = new Set([ - 'updateOne', - 'addTreeNode', - 'moveTreeNode', - 'deleteTreeNode', - 'refetchRootTreeNodeEvent', -]); diff --git a/docs/backlog/realtime-tree-server-authoritative.md b/docs/backlog/realtime-tree-server-authoritative.md deleted file mode 100644 index e60914a3..00000000 --- a/docs/backlog/realtime-tree-server-authoritative.md +++ /dev/null @@ -1,387 +0,0 @@ -# Realtime-дерево: сделать обновления сервер-авторитетными (как контент) - -## Контекст (проблема) - -Контент страницы синхронизируется между пользователями в реальном времени всегда, -а **дерево страниц в сайдбаре не обновляется**, когда кто-то создаёт / перемещает / -удаляет страницу — у других участников спейса (а часто и у самого автора в соседней -вкладке) дерево «застывает» до ручного refetch (перезагрузка страницы или -переключение спейса). - -Причина — в том, что это два разных realtime-канала с разной «авторитетностью»: - -- **Контент — сервер-авторитетный (Yjs / Hocuspocus).** Любое изменение текста - проходит через collab-сервер (`apps/server/src/collaboration/`) и раздаётся всем - подписчикам документа независимо от того, кто и каким способом редактировал. -- **Дерево — ретрансляция, инициируемая клиентом.** Броадкаст изменения дерева - делает **браузер автора**, а не сервер. Сервер только пересылает уже готовое - сообщение другим клиентам и **сам по событиям жизненного цикла страницы ничего - не вещает**. - -Поэтому дерево обновляется у других **только если** страница создана через UI-дерево, -в открытой вкладке, при живом сокете, и вкладка не закрылась/не сменила URL в течение -~50 мс после действия. **Любой другой путь создания/изменения страницы броадкаста не -даёт вообще:** AI-агент (`core/ai-chat/tools/`), встроенный MCP `/mcp` и standalone -`@docmost/mcp`, REST API напрямую, импорт markdown/zip, копирование/дублирование -страницы, фоновые серверные операции. - -Цель фичи: **перенести источник истины tree-событий на сервер** — чтобы дерево -обновлялось у всех в спейсе при любом способе изменения, надёжно, по аналогии с -контентом. - -## Как сейчас устроено (цепочка) - -### Клиентский relay (единственный текущий источник tree-событий) - -- `apps/client/src/features/page/tree/hooks/use-tree-mutation.ts` - - `handleCreate` (строки ~133-191): после успешного `createPageMutation` делает - оптимистичную вставку в `treeDataAtom`, затем через `setTimeout(50)` — - `emit({ operation: "addTreeNode", spaceId, payload: { parentId, index, data } })`. - - `handleMove` (~46-131): оптимистично двигает узел, затем `emit("moveTreeNode", …)`. - - `handleDelete` (~207-254): удаляет узел, затем `emit("deleteTreeNode", …)`. - - `handleRename` (~193-205): оптимистично меняет имя, **emit НЕ делает**. -- `apps/client/src/features/websocket/use-query-emit.ts`: `emit` — это просто - `socket?.emit("message", input)`. - -### Сервер — только пересылка - -- `apps/server/src/ws/ws.gateway.ts` (`@SubscribeMessage('message')`, ~64-69): - если `wsService.isTreeEvent(data)` — отдаёт в `wsService.handleTreeEvent`. -- `apps/server/src/ws/ws.service.ts` `handleTreeEvent` (~27-58): - `client.broadcast.to(getSpaceRoomName(spaceId)).emit('message', data)` — пересылка - пришедшего от клиента события в комнату спейса (с учётом ограничений доступа). -- `apps/server/src/database/listeners/page.listener.ts`: слушает `PAGE_CREATED` / - `PAGE_UPDATED` / `PAGE_DELETED` / `PAGE_SOFT_DELETED` / `PAGE_RESTORED`, но **только - ставит задачи в очереди (search / AI)** — WebSocket не трогает. - -### Что уже есть для серверного броадкаста (но не используется) - -- `apps/server/src/ws/ws-tree.service.ts` — `WsTreeService` с методами - `notifyPermissionGranted` (строит готовый payload `addTreeNode`) и - `notifyPageRestricted` (payload `deleteTreeNode`). **Нигде не вызывается** (мёртвый - код) — но это точный шаблон формата событий и доказательство, что инфраструктура - серверного броадкаста работоспособна. -- `WsService.emitCommentEvent(spaceId, pageId, data)` (~66-87) — образец - **серверного** броадкаста в комнату спейса с проверкой ограничений доступа - (`spaceHasRestrictions` → `hasRestrictedAncestor` → `broadcastToAuthorizedUsers`). -- `WsModule` — `@Global`, экспортирует `WsService` и `WsTreeService`. - -### Приёмник на клиенте (переиспользуем как есть) - -- `apps/client/src/features/websocket/use-tree-socket.ts` (`socket.on("message")`): - - `addTreeNode` (~55-74): вставляет узел; **идемпотентен** — - `if (treeModel.find(prev, event.payload.data.id)) return prev;` (повторная - доставка того же id безопасна). - - `moveTreeNode` (~75-117), `deleteTreeNode` (~119-138), `updateOne` (~36-54). -- `apps/client/src/features/websocket/use-query-subscription.ts`: на те же события - синхронизирует кэш TanStack Query сайдбара (`invalidateOnCreatePage`, - `updateCacheOnMovePage`, `invalidateOnDeletePage`). - -## Целевое поведение - -При **любом** способе изменения структуры (UI, AI-агент, MCP, REST API, импорт, -копирование, фоновые операции) сервер сам рассылает соответствующее tree-событие всем -клиентам в комнате спейса (с учётом ограничений доступа), и у всех участников дерево -обновляется без ручного refetch: - -- создание страницы → `addTreeNode`; -- перемещение/переупорядочивание → `moveTreeNode`; -- мягкое/жёсткое удаление → `deleteTreeNode`; -- восстановление из корзины → `addTreeNode` (или `refetchRootTreeNodeEvent`); -- (расширение) переименование / смена иконки → `updateOne`; -- (расширение) перенос между спейсами → `deleteTreeNode` в старом спейсе + - `addTreeNode` в новом. - -## Решение (архитектура) - -Перенести генерацию tree-событий на сервер и сделать его единственным источником -истины. Состоит из трёх частей: (1) серверный эмиттер, (2) обогащённые доменные -события, (3) удаление клиентского relay. - -### 1. Серверный метод броадкаста tree-события - -В `WsService` добавить метод по образцу `emitCommentEvent` — рассылка в комнату спейса -с учётом ограничений доступа. Не исключаем автора: повторная доставка безопасна -благодаря идемпотентности приёмника (см. edge cases). - -```ts -// apps/server/src/ws/ws.service.ts -// Server-origin tree broadcast. Mirrors emitCommentEvent: respects per-space page -// restrictions, then fans the event out to everyone in the space room. The author -// is NOT excluded — the client receiver is idempotent (addTreeNode early-returns if -// the node id already exists), so the author's optimistic node is preserved and -// non-UI creators (MCP / AI / API) still see their own page appear. -async emitTreeEvent(spaceId: string, pageId: string, data: any): Promise { - const room = getSpaceRoomName(spaceId); - const hasRestrictions = await this.spaceHasRestrictions(spaceId); - if (!hasRestrictions) { - this.server.to(room).emit('message', data); - return; - } - const isRestricted = await this.pagePermissionRepo.hasRestrictedAncestor(pageId); - if (!isRestricted) { - this.server.to(room).emit('message', data); - return; - } - await this.broadcastToAuthorizedUsers(room, null, pageId, data); -} -``` - -`WsTreeService` расширить методами, которые строят payload и вызывают `emitTreeEvent` -(переиспользуя формат из существующих `notifyPermissionGranted`/`notifyPageRestricted`): - -```ts -// apps/server/src/ws/ws-tree.service.ts -async broadcastPageCreated(page: TreeNodeData): Promise { - await this.wsService.emitTreeEvent(page.spaceId, page.id, { - operation: 'addTreeNode', - spaceId: page.spaceId, - payload: { - parentId: page.parentPageId ?? null, - // Receivers should place by `position`, not this index — see edge cases. - index: 0, - data: { - id: page.id, slugId: page.slugId, - name: page.title ?? '', title: page.title, icon: page.icon, - position: page.position, spaceId: page.spaceId, - parentPageId: page.parentPageId, hasChildren: false, children: [], - }, - }, - }); -} - -async broadcastPageDeleted(page: TreeNodeData): Promise { - await this.wsService.emitTreeEvent(page.spaceId, page.id, { - operation: 'deleteTreeNode', - spaceId: page.spaceId, - payload: { node: { id: page.id, slugId: page.slugId, parentPageId: page.parentPageId } }, - }); -} - -async broadcastPageMoved(p: MovedTreeNodeData): Promise { - await this.wsService.emitTreeEvent(p.spaceId, p.id, { - operation: 'moveTreeNode', - spaceId: p.spaceId, - payload: { - id: p.id, parentId: p.parentPageId ?? null, oldParentId: p.oldParentId ?? null, - index: 0, position: p.position, - pageData: { id: p.id, slugId: p.slugId, title: p.title, icon: p.icon, - position: p.position, spaceId: p.spaceId, parentPageId: p.parentPageId, - hasChildren: p.hasChildren }, - }, - }); -} -``` - -### 2. Источник событий: обогатить payload и/или эмитить из сервиса post-commit - -Главная сложность — листенеру нужны поля, которых нет в `PageEvent` -(`{ pageIds, workspaceId }`), а дочитывание из БД по `pageId` гонится с транзакцией -(`insertPage`/`removePage` эмитят событие, иногда находясь внутри ещё не -закоммиченного `trx` — отдельный SELECT может не увидеть строку). Два варианта (см. -«Открытые вопросы», по умолчанию — **A**): - -**Вариант A (рекомендуется): обогатить доменные события снимком узла.** Добавить в -payload событий тонкие поля дерева, чтобы листенер не читал БД: - -```ts -// apps/server/src/database/listeners/page.listener.ts (PageEvent) -export class PageEvent { - pageIds: string[]; - workspaceId: string; - // Optional tree snapshots so the WS listener can broadcast without a DB read - // (avoids the in-transaction visibility race on PAGE_CREATED / PAGE_SOFT_DELETED). - pages?: TreeNodeSnapshot[]; // { id, slugId, title, icon, position, spaceId, parentPageId } -} -``` - -`insertPage` уже делает `returning(this.baseFields)` — снимок собирается из `result` -без доплат. `removePage` знает удаляемые `pageIds`; для `deleteTreeNode` достаточно -`{ id, slugId, parentPageId, spaceId }`, которые можно вернуть из того же `withRecursive`. - -**Вариант B: эмитить tree-broadcast из сервиса после завершения операции (post-commit).** -Внедрить `WsTreeService` в `PageService` и вызывать `broadcastPage*` после успешного -`insertPage`/`removePage`/`movePage` (когда транзакция уже закоммичена и данные на -руках). Минус — размазывает realtime-логику по доменному сервису вместо одного -листенера. - -### 3. Отдельное событие для перемещения - -`movePage` сейчас эмитит общий `PAGE_UPDATED` — он непригоден: (а) не несёт -`oldParentId`/`position`, (б) срабатывает также на rename и сохранение контента (шум, -ложные `moveTreeNode`). Ввести выделенное событие: - -```ts -// apps/server/src/common/events/event.contants.ts -PAGE_MOVED = 'page.moved', -``` - -`pageService.movePage()` знает старого родителя (читает страницу до апдейта), новый -`parentPageId` и новый `position` — эмитить `PAGE_MOVED` с полным снимком (вариант A) -после апдейта. Листенер вешает `@OnEvent(EventName.PAGE_MOVED)` → -`wsTreeService.broadcastPageMoved(...)`. - -### 4. Новый листенер в модуле ws - -```ts -// apps/server/src/ws/listeners/page-ws.listener.ts -@Injectable() -export class PageWsListener { - constructor(private readonly wsTree: WsTreeService) {} - - @OnEvent(EventName.PAGE_CREATED) - async onCreated(e: PageEvent) { - for (const p of e.pages ?? []) await this.wsTree.broadcastPageCreated(p); - } - - @OnEvent(EventName.PAGE_SOFT_DELETED) - @OnEvent(EventName.PAGE_DELETED) - async onDeleted(e: PageEvent) { - for (const p of e.pages ?? []) await this.wsTree.broadcastPageDeleted(p); - } - - @OnEvent(EventName.PAGE_MOVED) - async onMoved(e: PageMovedEvent) { await this.wsTree.broadcastPageMoved(e); } - - @OnEvent(EventName.PAGE_RESTORED) - async onRestored(e: PageEvent) { - // Restore can re-attach a subtree; simplest correct option is a root refetch - // hint (see edge cases) instead of N addTreeNode events. - // await this.wsTree.broadcastRefetchRoot(spaceId); - } -} -``` - -Зарегистрировать `PageWsListener` в `WsModule.providers`. `WsTreeService` уже там; -`PageRepo` доступен из глобального `DatabaseModule` (если выберем вариант B/дочитывание). - -### 5. Убрать клиентский relay (источник истины — только сервер) - -После включения серверного броадкаста убрать `emit(...)` из -`use-tree-mutation.ts` (`handleCreate`/`handleMove`/`handleDelete`) и связанный -`setTimeout(50)`. Оптимистичные локальные обновления **оставить** (мгновенный отклик у -автора). Тогда на каждую операцию будет ровно один броадкаст (серверный), исчезает -гонка 50 мс и зависимость от того, успел ли браузер автора отправить событие. - -> Безопасный порядок выката: серверный броадкаст можно включить, **не** удаляя relay -> сразу — приёмник идемпотентен, дубль `addTreeNode`/`deleteTreeNode` безвреден (второй -> — no-op). Это позволяет проверить серверный путь в изоляции, затем удалить relay -> отдельным коммитом. `moveTreeNode` при двойной доставке тоже идемпотентен по позиции. - -## Тонкие моменты / edge cases - -- **Гонка видимости транзакции.** Главная причина выбрать вариант A (снимок в - событии): `insertPage`/`removePage` эмитят событие, находясь иногда внутри - незакоммиченного `trx`; отдельный SELECT в листенере может не увидеть строку. - Существующие листенеры (search/AI) не страдают, т.к. лишь ставят отложенную задачу, - выполняемую после коммита. Синхронный re-fetch для броадкаста — нет. -- **Двойная вставка у автора.** Не исключаем автора из рассылки: приёмник `addTreeNode` - делает `if (treeModel.find(prev, id)) return prev` — у UI-автора оптимистичный узел - уже есть, серверное событие игнорируется (и не затирает редактируемое имя). У - non-UI автора (MCP/AI/API) узла нет — он его получит. Это и есть аргумент против - `emitToSpaceExceptUsers([creatorId])`: исключение автора сломало бы non-UI случай. -- **Порядок/позиция.** Сервер не знает локальный `index` каждого получателя (корневой - список пагинируется, у клиентов разный набор загруженных узлов). Поэтому в payload - кладём `position` (фракционный индекс — реальный порядок), а приёмник `addTreeNode` - стоит доработать так, чтобы вставлять **по `position`** среди уже загруженных - сиблингов, а не по абсолютному `index` отправителя. Сейчас `treeModel.insert` - принимает `index`; нужна вставка с сортировкой по `position` (или отдельный - `insertByPosition`). Без этого порядок у получателей может разойтись. -- **Пагинация корня → дубликаты.** Если новая корневая страница по `position` попадает - за пределы уже загруженного «окна» корневого инфинит-списка, прямая вставка в атом - может позже задвоиться при подгрузке следующей страницы. `use-query-subscription.ts` - уже инвалидирует кэш сайдбара на `addTreeNode` (`invalidateOnCreatePage`) — следить, - чтобы оба приёмника (`useTreeSocket` мутирует атом, `useQuerySubscription` - инвалидирует query) сходились к одному состоянию и не дублировали узлы. -- **Перенос между спейсами (`movePageToSpace`).** Сейчас эмитит `PAGE_MOVED_TO_SPACE` - **без листенера**. Корректный realtime: в **старом** спейсе — `deleteTreeNode`, в - **новом** — `addTreeNode` (для всего перенесённого поддерева — вероятно проще - `refetchRootTreeNodeEvent` на оба спейса). Вынести в отдельный пункт объёма. -- **Восстановление из корзины (`PAGE_RESTORED`).** Может вернуть целое поддерево и - переприкрепить его к родителю. N точечных `addTreeNode` хрупки по порядку — проще - отправить `refetchRootTreeNodeEvent` (он уже поддержан и сервером-пересыльщиком, и - `use-query-subscription`), пусть клиенты перезапросят корень спейса. -- **Rename / иконка.** `handleRename` сейчас emit не делает, а `updateOne` хоть и - обрабатывается приёмником, серверно не рассылается → переименования тоже не - пропагируются. Естественное расширение этой же фичи: на `PAGE_UPDATED`, когда - изменились `title`/`icon`, слать `updateOne` (но фильтровать, чтобы не слать на - каждое сохранение контента). Вынесено в расширения, чтобы не раздувать базовый объём. -- **Каскадное мягкое удаление.** `removePage` удаляет всё поддерево и эмитит **все** - `pageIds` потомков. Для дерева достаточно одного `deleteTreeNode` по корню удаляемого - поддерева (клиент `treeModel.remove` убирает узел с детьми). Слать событие только по - корню удаления, а не по каждому потомку, иначе лишний трафик. -- **Ограничения доступа** наследуются бесплатно из `emitCommentEvent`-паттерна - (`spaceHasRestrictions` → `hasRestrictedAncestor` → `broadcastToAuthorizedUsers`): - закрытые страницы не утекут неавторизованным. -- **Мёртвый `WsTreeService`.** Его текущие `notifyPermissionGranted` / - `notifyPageRestricted` нигде не вызываются — заодно проверить, не должны ли они - вызываться при смене прав доступа на страницу (отдельный, но смежный баг realtime). -- **Идемпотентность move/delete.** `moveTreeNode` (place по позиции) и `deleteTreeNode` - (`if (!find) return prev`) тоже безопасны к повторной доставке — это позволяет - поэтапный выкат (п. 5). -- **Комментарии в коде — на английском** (правило проекта). - -## Объём работ (файлы) - -Сервер: -- [ ] `apps/server/src/common/events/event.contants.ts` — добавить `PAGE_MOVED` - (и при необходимости тип `PageMovedEvent`). -- [ ] `apps/server/src/database/listeners/page.listener.ts` — обогатить `PageEvent` - снимками узлов (вариант A); экспортировать общий тип снимка. -- [ ] `apps/server/src/database/repos/page/page.repo.ts` — класть снимок в payload - `PAGE_CREATED` (`insertPage`) и `PAGE_SOFT_DELETED` (`removePage`, только корень - удаления). -- [ ] `apps/server/src/core/page/services/page.service.ts` — `movePage` эмитит - `PAGE_MOVED` со старым/новым родителем и `position` (и `movePageToSpace` — для - расширения). -- [ ] `apps/server/src/ws/ws.service.ts` — `emitTreeEvent(spaceId, pageId, data)`. -- [ ] `apps/server/src/ws/ws-tree.service.ts` — `broadcastPageCreated/Deleted/Moved` - (+ опц. `broadcastRefetchRoot`). -- [ ] `apps/server/src/ws/listeners/page-ws.listener.ts` — новый листенер. -- [ ] `apps/server/src/ws/ws.module.ts` — зарегистрировать `PageWsListener`. - -Клиент: -- [ ] `apps/client/src/features/page/tree/hooks/use-tree-mutation.ts` — убрать - `emit(...)` и `setTimeout(50)` из create/move/delete (оптимистику оставить). -- [ ] `apps/client/src/features/page/tree/model/tree-model.ts` — - вставка `addTreeNode` по `position` среди сиблингов (а не по абсолютному index). -- [ ] Проверить согласованность `use-tree-socket.ts` и `use-query-subscription.ts` - (мутация атома vs инвалидация кэша) — без дубликатов узлов. - -## Тесты - -- Сервер (Jest): юнит на `WsTreeService.broadcastPage*` — корректный формат payload - (`operation`, `spaceId`, `payload.data/node/pageData`) для create/delete/move. - `emitTreeEvent` — рассылка в комнату спейса и ветка ограничений (restricted → - только авторизованные). Запуск: `pnpm --filter server test`. -- Клиент (Vitest): приёмник `addTreeNode` идемпотентен (повтор того же id — no-op); - вставка по `position` даёт верный порядок при разном наборе загруженных сиблингов. -- Линт: `pnpm --filter server lint`, `pnpm --filter client lint`. -- Ручная проверка матрицы способов создания: UI-дерево, AI-агент, MCP `/mcp`, REST - `POST /pages/create`, импорт markdown — во всех случаях дерево обновляется у второго - пользователя без перезагрузки. - -## Альтернативы - -- **Только клиентский патч (быстро, не рекомендуется).** Убрать `setTimeout(50)` и/или - слать `refetchRootTreeNodeEvent` после create. Лечит лишь UI-сценарий между людьми, - не покрывает AI/MCP/API и остаётся клиент-зависимым — против цели фичи. -- **Сервер всегда шлёт `refetchRootTreeNodeEvent` вместо точечных событий.** Проще - (не нужен снимок узла, нет проблемы порядка), но грубее: каждый клиент перезапрашивает - корневое дерево спейса на любое изменение — больше нагрузки и моргание UI. Возможен - как временный/откатной режим для сложных случаев (restore, move-to-space). -- **Вариант B (эмит из сервиса post-commit)** вместо обогащения событий — см. п. 2. - Надёжно по транзакциям, но размазывает realtime-логику по доменному сервису. - -## Открытые вопросы (согласовать перед реализацией) - -- [ ] Источник данных для броадкаста: обогатить доменные события снимком узла - (**вариант A, рекомендуется**) или эмитить из сервиса post-commit (вариант B)? -- [ ] Удалять клиентский relay сразу в той же задаче или вторым коммитом после - проверки серверного пути (приёмник идемпотентен — оба варианта безопасны)? -- [ ] `restore` и `move-to-space`: точечные `addTreeNode`/`deleteTreeNode` или более - простой и устойчивый `refetchRootTreeNodeEvent` на затронутые спейсы? -- [ ] Включать ли в базовый объём rename/иконку (`updateOne` от сервера на - `PAGE_UPDATED`) или вынести в отдельную задачу? -- [ ] Чинить ли заодно мёртвый `WsTreeService` (broadcast при смене прав доступа) — - в рамках этой задачи или отдельной?