vvzvlad/gitmost
1
0
Fork 0
Code Issues 14 Pull Requests 12 Packages Projects Releases Activity

fix(tree): place remote moves by position; remove stale node on move-into-restricted

Browse Source 
Release-cycle review found two move-path issues:
- Remote moves were placed at index:0 (broadcastPageMoved hardcodes index:0),
  so every observer rendered the moved node at the TOP of its new siblings
  until refetch. Client moveTreeNode now places by fractional position
  (treeModel.placeByPosition, mirroring addTreeNode/insertByPosition) and
  applies the payload's pageData (title->name, icon, hasChildren) so receivers
  keep the node correct.
- Moving a page under a restricted ancestor left a stale named node (title/
  slugId/icon) in the trees of users who lost visibility. broadcastPageMoved
  now derives one FRESH hasRestrictedAncestor decision and drives both paths
  from it: when restricted, the move goes to authorized users only
  (emitToAuthorizedUsers, not the space-cache-gated emitTreeEvent) and a
  compensating deleteTreeNode goes to the unauthorized complement (same fresh
  getUserIdsWithPageAccess set) — disjoint, no stale-cache window. Non-restricted
  moves are unchanged (one moveTreeNode to the room).

Follow-up (noted): invalidateSpaceRestrictionCache is still unwired at
permission-mutation sites; the open-space fast path can lag up to the 30s TTL,
but the move/delete consistency above no longer depends on it.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
claude code agent 227
2026-06-20 14:01:37 +03:00
parent 31d6498b24
commit 5d5f61fc6e
6 changed files with 502 additions and 13 deletions
Download Patch File Download Diff File Expand all files Collapse all files

214
apps/client/src/features/page/tree/model/tree-model.test.ts
View File

@@ -280,6 +280,108 @@ describe('handleCreate optimistic-insert idempotency (find-then-skip)', () => {
});
});
// 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<P> = {
position: payload.position,
parentPageId: payload.parentId,
} as Partial<P>;
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');
@@ -392,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', {

24
apps/client/src/features/page/tree/model/tree-model.ts
View File

@@ -215,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<T extends { position?: string }>(
tree: TreeNode<T>[],
sourceId: string,
to: { parentId: string | null; position?: string },
): TreeNode<T>[] {
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<T>;
return treeModel.insertByPosition(removed, to.parentId, positioned);
},
move<T extends object>(
tree: TreeNode<T>[],
sourceId: string,

44
apps/client/src/features/websocket/use-tree-socket.ts
View File

@@ -84,22 +84,48 @@ 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<SpaceTreeNode> = {
position: event.payload.position,
parentPageId: newParentId,
} as Partial<SpaceTreeNode>);
parentPageId: newParentId as string,
};
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.

117
apps/server/src/ws/ws-tree.service.spec.ts
View File

@@ -21,16 +21,33 @@ const snapshot: TreeNodeSnapshot = {
describe('WsTreeService', () => {
let service: WsTreeService;
let wsService: { emitTreeEvent: jest.Mock; emitToSpaceRoom: jest.Mock };
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 }],
providers: [
WsTreeService,
{ provide: WsService, useValue: wsService },
{ provide: PagePermissionRepo, useValue: pagePermissionRepo },
],
}).compile();
service = module.get<WsTreeService>(WsTreeService);
@@ -118,6 +135,76 @@ describe('WsTreeService', () => {
);
});
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');
@@ -203,4 +290,30 @@ describe('WsService.emitTreeEvent', () => {
expect(okEmit).toHaveBeenCalledWith('message', data);
expect(noEmit).not.toHaveBeenCalled();
});
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);
});
});

60
apps/server/src/ws/ws-tree.service.ts
View File

@@ -1,5 +1,6 @@
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,
@@ -8,7 +9,10 @@ import {
@Injectable()
export class WsTreeService {
constructor(private readonly wsService: WsService) {}
constructor(
private readonly wsService: WsService,
private readonly pagePermissionRepo: PagePermissionRepo,
) {}
// 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
@@ -56,7 +60,8 @@ export class WsTreeService {
async broadcastPageMoved(event: PageMovedEvent): Promise<void> {
const { node } = event;
await this.wsService.emitTreeEvent(node.spaceId, node.id, {
const movePayload = {
operation: 'moveTreeNode',
spaceId: node.spaceId,
payload: {
@@ -77,6 +82,57 @@ export class WsTreeService {
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,
},
},
});
}

56
apps/server/src/ws/ws.service.ts
View File

@@ -127,6 +127,62 @@ export class WsService {
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,
pageId: string,
data: any,
): Promise<void> {
const room = getSpaceRoomName(spaceId);
const sockets = await this.server.in(room).fetchSockets();
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;
// 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);
}
}
}
// 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<void> {
const room = getSpaceRoomName(spaceId);
await this.broadcastToAuthorizedUsers(room, null, pageId, data);
}
async emitToUsers(userIds: string[], data: any): Promise<void> {
if (userIds.length === 0) return;
const rooms = userIds.map((id) => getUserRoomName(id));