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..066464d7 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,158 @@ 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); + }); +}); + describe('treeModel.remove', () => { it('removes a leaf', () => { const t = treeModel.remove(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..0c1b26a4 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[] => { diff --git a/apps/client/src/features/websocket/use-tree-socket.ts b/apps/client/src/features/websocket/use-tree-socket.ts index c4cd083b..8b17caee 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. 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 1f5163bd..fc2618ad 100644 --- a/apps/server/src/core/page/page.controller.ts +++ b/apps/server/src/core/page/page.controller.ts @@ -724,7 +724,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 cc1dfb24..672ff3fb 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -887,6 +887,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 b2884603..066add81 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/ws/listeners/page-ws.listener.ts b/apps/server/src/ws/listeners/page-ws.listener.ts new file mode 100644 index 00000000..c2afc2c6 --- /dev/null +++ b/apps/server/src/ws/listeners/page-ws.listener.ts @@ -0,0 +1,67 @@ +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 { + for (const page of event.pages ?? []) { + await this.wsTree.broadcastPageCreated(page); + } + } + + // 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..00877d6d --- /dev/null +++ b/apps/server/src/ws/ws-tree.service.spec.ts @@ -0,0 +1,206 @@ +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 } 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 }; + + beforeEach(async () => { + wsService = { + emitTreeEvent: jest.fn().mockResolvedValue(undefined), + emitToSpaceRoom: jest.fn(), + }; + + const module: TestingModule = await Test.createTestingModule({ + providers: [WsTreeService, { provide: WsService, useValue: wsService }], + }).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('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(); + }); +}); diff --git a/apps/server/src/ws/ws-tree.service.ts b/apps/server/src/ws/ws-tree.service.ts index 8aadfa99..dd5bdb71 100644 --- a/apps/server/src/ws/ws-tree.service.ts +++ b/apps/server/src/ws/ws-tree.service.ts @@ -1,11 +1,95 @@ import { Injectable } from '@nestjs/common'; import { Page } from '@docmost/db/types/entity.types'; import { WsService } from './ws.service'; +import { + PageMovedEvent, + TreeNodeSnapshot, +} from '../database/listeners/page.listener'; @Injectable() export class WsTreeService { constructor(private readonly wsService: WsService) {} + // 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 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, + 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: 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; + await this.wsService.emitTreeEvent(node.spaceId, node.id, { + 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, + }, + }, + }); + } + + // 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, + }); + } + async notifyPageRestricted(page: Page, excludeUserId: string): Promise { await this.wsService.emitToSpaceExceptUsers(page.spaceId, [excludeUserId], { operation: 'deleteTreeNode', 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..bfa421c6 100644 --- a/apps/server/src/ws/ws.service.ts +++ b/apps/server/src/ws/ws.service.ts @@ -86,6 +86,47 @@ export class WsService { await this.broadcastToAuthorizedUsers(room, null, pageId, 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); + } + + // 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. Mirrors handleTreeEvent's + // special-casing of refetchRootTreeNodeEvent (no restriction check). + emitToSpaceRoom(spaceId: string, data: any): void { + this.server.to(getSpaceRoomName(spaceId)).emit('message', data); + } + async emitToUsers(userIds: string[], data: any): Promise { if (userIds.length === 0) return; const rooms = userIds.map((id) => getUserRoomName(id));