diff --git a/apps/client/src/features/page-embed/queries/page-embed-query.test.ts b/apps/client/src/features/page-embed/queries/page-embed-query.test.ts new file mode 100644 index 00000000..70336c5c --- /dev/null +++ b/apps/client/src/features/page-embed/queries/page-embed-query.test.ts @@ -0,0 +1,69 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { getDefaultStore } from "jotai"; + +// Mock the app entry so importing the query module doesn't boot the whole app +// (it only needs queryClient's cache methods, which we stub here). The spies are +// declared via vi.hoisted so they exist before the hoisted vi.mock factory runs. +const { setQueryData, getQueryData, invalidateQueries } = vi.hoisted(() => ({ + setQueryData: vi.fn(), + getQueryData: vi.fn(() => undefined as unknown), + invalidateQueries: vi.fn(), +})); +vi.mock("@/main.tsx", () => ({ + queryClient: { setQueryData, getQueryData, invalidateQueries }, +})); + +import { syncTemporaryExpiresInCache } from "./page-embed-query"; +import { treeDataAtom } from "@/features/page/tree/atoms/tree-data-atom.ts"; +import { SpaceTreeNode } from "@/features/page/tree/types.ts"; + +const mkNode = (id: string, slugId: string): SpaceTreeNode => + ({ + id, + slugId, + name: id, + position: "a0", + spaceId: "space-1", + parentPageId: null, + hasChildren: false, + children: [], + }) as unknown as SpaceTreeNode; + +describe("syncTemporaryExpiresInCache — treeDataAtom patch", () => { + beforeEach(() => { + vi.clearAllMocks(); + getQueryData.mockReturnValue(undefined); + }); + + it("patches the in-tree node's temporaryExpiresAt (sidebar marker updates without reload)", () => { + const store = getDefaultStore(); + const tree = [mkNode("p1", "slug-1"), mkNode("p2", "slug-2")]; + store.set(treeDataAtom, tree); + + const deadline = "2026-07-01T00:00:00.000Z"; + syncTemporaryExpiresInCache({ id: "p1", slugId: "slug-1" }, deadline); + + const next = store.get(treeDataAtom); + // A new atom value was written... + expect(next).not.toBe(tree); + // ...the matching node gained the deadline... + expect(next.find((n) => n.id === "p1")?.temporaryExpiresAt).toBe(deadline); + // ...and the untouched sibling is unchanged. + expect(next.find((n) => n.id === "p2")?.temporaryExpiresAt).toBeUndefined(); + }); + + it("leaves the atom value at the SAME reference when the id is absent from the tree (no write)", () => { + const store = getDefaultStore(); + const tree = [mkNode("p1", "slug-1")]; + store.set(treeDataAtom, tree); + + syncTemporaryExpiresInCache( + { id: "not-in-tree", slugId: "missing" }, + "2026-07-01T00:00:00.000Z", + ); + + // treeModel.update is a no-op (same reference) for an unknown id, so the + // guard skips the store write entirely — same reference back. + expect(store.get(treeDataAtom)).toBe(tree); + }); +}); diff --git a/apps/client/src/features/page/queries/page-query.ts b/apps/client/src/features/page/queries/page-query.ts index 1b1355ad..7837f8db 100644 --- a/apps/client/src/features/page/queries/page-query.ts +++ b/apps/client/src/features/page/queries/page-query.ts @@ -32,7 +32,7 @@ import { import { notifications } from "@mantine/notifications"; import { IPagination, QueryParams } from "@/lib/types.ts"; import { queryClient } from "@/main.tsx"; -import { buildTree } from "@/features/page/tree/utils"; +import { buildTree, pageToTreeNode } from "@/features/page/tree/utils"; import { useEffect } from "react"; import { validate as isValidUuid } from "uuid"; import { useTranslation } from "react-i18next"; @@ -210,18 +210,15 @@ export function useRestorePageMutation() { // Check if the page already exists in the tree (it shouldn't) if (!treeModel.find(currentTree, restoredPage.id)) { - // Create the tree node data with hasChildren from backend - const nodeData: SpaceTreeNode = { - id: restoredPage.id, - slugId: restoredPage.slugId, + // Create the tree node data with hasChildren from backend. Routed + // through the canonical mapper so the field copy stays in lockstep with + // buildTree. The server NULLS `temporaryExpiresAt` on restore (a restored + // page is made permanent), so the mapper carries that null through and + // the node correctly shows no clock marker. + const nodeData: SpaceTreeNode = pageToTreeNode(restoredPage, { name: restoredPage.title || "Untitled", - icon: restoredPage.icon, - position: restoredPage.position, - spaceId: restoredPage.spaceId, - parentPageId: restoredPage.parentPageId, hasChildren: restoredPage.hasChildren || false, - children: [], - }; + }); // Determine the parent and index const parentId = restoredPage.parentPageId || null; diff --git a/apps/client/src/features/page/tree/components/space-tree-node-menu.tsx b/apps/client/src/features/page/tree/components/space-tree-node-menu.tsx index cd868746..63ddd98e 100644 --- a/apps/client/src/features/page/tree/components/space-tree-node-menu.tsx +++ b/apps/client/src/features/page/tree/components/space-tree-node-menu.tsx @@ -37,6 +37,7 @@ import { } from "@/features/page-embed/queries/page-embed-query"; import { treeDataAtom } from "@/features/page/tree/atoms/tree-data-atom.ts"; import { treeModel } from "@/features/page/tree/model/tree-model"; +import { pageToTreeNode } from "@/features/page/tree/utils"; import { useTreeMutation } from "@/features/page/tree/hooks/use-tree-mutation.ts"; import type { SpaceTreeNode } from "@/features/page/tree/types.ts"; import classes from "@/features/page/tree/styles/tree.module.css"; @@ -130,18 +131,14 @@ export function NodeMenu({ node, canEdit }: NodeMenuProps) { const currentIndex = siblings?.index ?? 0; const newIndex = currentIndex + 1; - const treeNodeData: SpaceTreeNode = { - id: duplicatedPage.id, - slugId: duplicatedPage.slugId, - name: duplicatedPage.title, - position: duplicatedPage.position, - spaceId: duplicatedPage.spaceId, - parentPageId: duplicatedPage.parentPageId, - icon: duplicatedPage.icon, - hasChildren: duplicatedPage.hasChildren, + // Routed through the canonical mapper so the field copy stays in lockstep + // with buildTree. The server does NOT arm a death timer on duplicate (the + // copy's `temporaryExpiresAt` defaults to null = permanent), so the mapper + // carries that null through and the duplicated node correctly shows no + // clock marker — matching the server without a reload. + const treeNodeData: SpaceTreeNode = pageToTreeNode(duplicatedPage, { canEdit: true, - children: [], - }; + }); setData((prev) => treeModel.insert(prev, parentId, treeNodeData, newIndex), 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 21db8757..494f8b93 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 @@ -9,6 +9,7 @@ import { treeModel } from "@/features/page/tree/model/tree-model"; import type { DropOp } from "@/features/page/tree/model/tree-model.types"; import { dropOpToMovePayload } from "./drop-op-to-move-payload"; import { SpaceTreeNode } from "@/features/page/tree/types.ts"; +import { pageToTreeNode } from "@/features/page/tree/utils"; import { IPage } from "@/features/page/types/page.types.ts"; import { useCreatePageMutation, @@ -139,18 +140,15 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { throw new Error("Failed to create page"); } - const newNode: SpaceTreeNode = { - id: createdPage.id, - slugId: createdPage.slugId, + // Route through the canonical mapper so the field copy (esp. + // `temporaryExpiresAt`, which shows the temporary-note clock marker on + // optimistic insert) can't drift from buildTree. `name: ""` because a + // freshly created page is untitled; `hasChildren: false` because it has no + // children yet. + const newNode: SpaceTreeNode = pageToTreeNode(createdPage, { name: "", - position: createdPage.position, - spaceId: createdPage.spaceId, - parentPageId: createdPage.parentPageId, hasChildren: false, - // Show the temporary-note icon immediately on optimistic insert. - temporaryExpiresAt: createdPage.temporaryExpiresAt, - children: [], - }; + }); // Read latest tree at call time. Without this, callers that mutate the // tree (e.g. lazy-load children on expand) immediately before calling 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 01682e2d..3b365bf4 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 @@ -393,6 +393,101 @@ describe("handleCreate optimistic-insert idempotency (find-then-skip)", () => { }); }); +// handleCreate race-guard temporaryExpiresAt patch: when the server's +// addTreeNode broadcast wins the race and inserts the node BEFORE the optimistic +// updater runs, the updater must not re-insert. Two sub-branches: +// (a) the node the broadcast inserted carries NO deadline (an older broadcast +// omitted it) while the authoritative create response DOES → patch the +// deadline on so the clock marker shows now, without a reload. +// (b) the existing node ALREADY has a deadline → do NOT overwrite it; return +// `prev` by reference (a no-op write). +describe("handleCreate race-guard temporaryExpiresAt patch", () => { + type TN = TreeNode<{ name: string; temporaryExpiresAt?: string | null }>; + + // Mirrors the setData updater in use-tree-mutation handleCreate. + const applyOptimisticInsert = ( + tree: TN[], + parentId: string | null, + node: TN, + index: number, + ): TN[] => { + const existing = treeModel.find(tree, node.id) as TN | null; + if (existing) { + if (node.temporaryExpiresAt && !existing.temporaryExpiresAt) { + return treeModel.update(tree, node.id, { + temporaryExpiresAt: node.temporaryExpiresAt, + }); + } + return tree; + } + return treeModel.insert(tree, parentId, node, index); + }; + + const fixtureTN: TN[] = [ + { id: "a", name: "A" }, + { id: "b", name: "B" }, + ]; + + const deadline = "2026-07-01T00:00:00.000Z"; + + it("(a) patches temporaryExpiresAt when the existing node has none + the response carries a deadline", () => { + // Server broadcast won the race and inserted the node WITHOUT a deadline. + const afterServer = treeModel.insert(fixtureTN, null, { + id: "new", + name: "", + }); + expect((treeModel.find(afterServer, "new") as TN).temporaryExpiresAt).toBe( + undefined, + ); + + // The authoritative create response carries the deadline. + const created: TN = { id: "new", name: "", temporaryExpiresAt: deadline }; + const patched = applyOptimisticInsert( + afterServer, + null, + created, + afterServer.length, + ); + + // A new reference (the patch wrote) and the node now has the deadline... + expect(patched).not.toBe(afterServer); + expect((treeModel.find(patched, "new") as TN).temporaryExpiresAt).toBe( + deadline, + ); + // ...and still exactly one node (no duplicate re-insert). + expect(patched.filter((n) => n.id === "new")).toHaveLength(1); + }); + + it("(b) does NOT overwrite an existing deadline; returns prev by reference", () => { + const existingDeadline = deadline; + // The node already exists WITH a deadline (the broadcast carried it). + const afterServer = treeModel.insert(fixtureTN, null, { + id: "new", + name: "", + temporaryExpiresAt: existingDeadline, + }); + + // The create response carries a DIFFERENT deadline; the guard must ignore it. + const created: TN = { + id: "new", + name: "", + temporaryExpiresAt: "2099-01-01T00:00:00.000Z", + }; + const after = applyOptimisticInsert( + afterServer, + null, + created, + afterServer.length, + ); + + // prev returned by reference (no write) and the original deadline is kept. + expect(after).toBe(afterServer); + expect((treeModel.find(after, "new") as TN).temporaryExpiresAt).toBe( + existingDeadline, + ); + }); +}); + // 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 diff --git a/apps/client/src/features/page/tree/utils/utils.ts b/apps/client/src/features/page/tree/utils/utils.ts index 8abb16e1..11600162 100644 --- a/apps/client/src/features/page/tree/utils/utils.ts +++ b/apps/client/src/features/page/tree/utils/utils.ts @@ -9,26 +9,45 @@ export function sortPositionKeys(keys: any[]) { }); } +/** + * Single canonical `IPage -> SpaceTreeNode` field mapper. Every place that + * materialises a tree node from a page (buildTree, the optimistic insert in + * handleCreate, restore, duplicate) routes through here so the field copy — + * crucially `temporaryExpiresAt` — can never silently drift between sites. The + * `overrides` cover the small per-site differences (e.g. `name: ""` for an + * optimistic create, `name: title || "Untitled"` for restore, `canEdit: true` + * for duplicate). The default `temporaryExpiresAt` comes straight off the page, + * so restore (which the server nulls) stays permanent and a temporary create + * keeps its clock marker without a reload. + */ +export function pageToTreeNode( + page: IPage, + overrides?: Partial, +): SpaceTreeNode { + return { + id: page.id, + slugId: page.slugId, + name: page.title, + icon: page.icon, + position: page.position, + hasChildren: page.hasChildren, + spaceId: page.spaceId, + parentPageId: page.parentPageId, + canEdit: page.canEdit ?? page.permissions?.canEdit, + isTemplate: page.isTemplate, + temporaryExpiresAt: page.temporaryExpiresAt, + children: [], + ...overrides, + }; +} + export function buildTree(pages: IPage[]): SpaceTreeNode[] { const pageMap: Record = {}; const tree: SpaceTreeNode[] = []; pages.forEach((page) => { - pageMap[page.id] = { - id: page.id, - slugId: page.slugId, - name: page.title, - icon: page.icon, - position: page.position, - hasChildren: page.hasChildren, - spaceId: page.spaceId, - parentPageId: page.parentPageId, - canEdit: page.canEdit ?? page.permissions?.canEdit, - isTemplate: page.isTemplate, - temporaryExpiresAt: page.temporaryExpiresAt, - children: [], - }; + pageMap[page.id] = pageToTreeNode(page); }); // Defense-in-depth: a duplicate id in `pages` would push two references to the diff --git a/apps/server/src/database/listeners/page.listener.ts b/apps/server/src/database/listeners/page.listener.ts index 52b9f963..b12dc160 100644 --- a/apps/server/src/database/listeners/page.listener.ts +++ b/apps/server/src/database/listeners/page.listener.ts @@ -28,6 +28,36 @@ export interface TreeNodeSnapshot { temporaryExpiresAt?: Date | string | null; } +/** + * Single canonical builder for a `TreeNodeSnapshot` from a page-like row. Both + * the `PAGE_CREATED` event enrichment (`page.repo.insertPage`) and the + * `addTreeNode` broadcast (`WsTreeService.broadcastPageCreated`) build this same + * snapshot; routing both through here keeps the optional `temporaryExpiresAt` + * (and the `?? null` normalisation that pins a permanent note to an explicit + * null) from silently drifting between the two literals. + */ +export function toTreeNodeSnapshot(page: { + id: string; + slugId: string; + title: string | null; + icon: string | null; + position: string; + spaceId: string; + parentPageId: string | null; + temporaryExpiresAt?: Date | string | null; +}): TreeNodeSnapshot { + return { + id: page.id, + slugId: page.slugId, + title: page.title, + icon: page.icon, + position: page.position, + spaceId: page.spaceId, + parentPageId: page.parentPageId, + temporaryExpiresAt: page.temporaryExpiresAt ?? null, + }; +} + export class PageEvent { pageIds: string[]; workspaceId: string; diff --git a/apps/server/src/database/repos/page/page.repo.ts b/apps/server/src/database/repos/page/page.repo.ts index 1b711750..a7ac3a5e 100644 --- a/apps/server/src/database/repos/page/page.repo.ts +++ b/apps/server/src/database/repos/page/page.repo.ts @@ -16,7 +16,10 @@ import { jsonArrayFrom, jsonObjectFrom } from 'kysely/helpers/postgres'; import { SpaceMemberRepo } from '@docmost/db/repos/space/space-member.repo'; import { EventEmitter2 } from '@nestjs/event-emitter'; import { EventName } from '../../../common/events/event.contants'; -import { TreeUpdateSnapshot } from '../../listeners/page.listener'; +import { + TreeUpdateSnapshot, + toTreeNodeSnapshot, +} from '../../listeners/page.listener'; /** * Optional extras for the PAGE_UPDATED event emitted by updatePage(s). Lets the @@ -200,20 +203,10 @@ export class PageRepo { 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, - // Carry the death-timer deadline so a note created as temporary shows - // its sidebar clock marker on every client without a reload. - temporaryExpiresAt: result.temporaryExpiresAt, - }, - ], + // Built via the shared snapshot helper so the field copy (and the + // death-timer deadline that shows the sidebar clock marker without a + // reload) can't drift from the `addTreeNode` broadcast literal. + pages: [toTreeNodeSnapshot(result)], }); return result; diff --git a/apps/server/src/ws/ws-tree.service.spec.ts b/apps/server/src/ws/ws-tree.service.spec.ts index 1ee8d10b..8b5e3a2f 100644 --- a/apps/server/src/ws/ws-tree.service.spec.ts +++ b/apps/server/src/ws/ws-tree.service.spec.ts @@ -83,6 +83,27 @@ describe('WsTreeService', () => { ); }); + it('broadcastPageCreated carries temporaryExpiresAt when the page is a temporary note', async () => { + const expiresAt = new Date('2026-07-01T00:00:00.000Z'); + await service.broadcastPageCreated({ ...snapshot, temporaryExpiresAt: expiresAt }); + + const data = + wsService.emitTreeEvent.mock.calls[0][2].payload.data; + // The death-timer deadline reaches receivers so the clock marker renders + // immediately (incl. the author if this broadcast wins the optimistic race). + expect(data.temporaryExpiresAt).toBe(expiresAt); + }); + + it('broadcastPageCreated pins temporaryExpiresAt to null for a permanent page', async () => { + // Fixture omits temporaryExpiresAt; the `?? null` must send an explicit null + // (permanent) rather than undefined, so receivers clear any stale marker. + await service.broadcastPageCreated(snapshot); + + const data = + wsService.emitTreeEvent.mock.calls[0][2].payload.data; + expect(data.temporaryExpiresAt).toBeNull(); + }); + it('broadcastPageDeleted emits deleteTreeNode with the root node only', async () => { await service.broadcastPageDeleted({ ...snapshot, diff --git a/apps/server/src/ws/ws-tree.service.ts b/apps/server/src/ws/ws-tree.service.ts index 15b4dd87..65a6b4d9 100644 --- a/apps/server/src/ws/ws-tree.service.ts +++ b/apps/server/src/ws/ws-tree.service.ts @@ -5,6 +5,7 @@ import { PageMovedEvent, TreeNodeSnapshot, TreeUpdateSnapshot, + toTreeNodeSnapshot, } from '../database/listeners/page.listener'; @Injectable() @@ -28,20 +29,17 @@ export class WsTreeService { // Receivers place by `position` among already-loaded siblings, not by // this absolute index (sender's loaded set differs from receivers'). index: 0, + // Built via the shared snapshot helper (same one page.repo uses to fill + // the event), then extended with the tree-only fields the client + // receiver consumes. The helper carries the death-timer deadline + // (normalised to null => permanent) so receivers — and the author, if + // this broadcast wins the race against the optimistic insert — render + // the temporary-note clock marker immediately, without it drifting from + // the event literal. data: { - id: page.id, - slugId: page.slugId, + ...toTreeNodeSnapshot(page), name: page.title ?? '', - title: page.title, - icon: page.icon, - position: page.position, - spaceId: page.spaceId, - parentPageId: page.parentPageId, hasChildren: false, - // Carry the death-timer deadline so receivers (and the author, if this - // broadcast wins the race against the optimistic insert) render the - // temporary-note clock marker immediately. null => permanent. - temporaryExpiresAt: page.temporaryExpiresAt ?? null, children: [], }, },