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 494f8b93..b1e1758a 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 @@ -15,7 +15,6 @@ import { useCreatePageMutation, useRemovePageMutation, useMovePageMutation, - useUpdatePageMutation, updateCacheOnMovePage, } from "@/features/page/queries/page-query.ts"; import { buildPageUrl } from "@/features/page/page.utils.ts"; @@ -27,7 +26,6 @@ export type UseTreeMutation = { parentId: string | null, opts?: { temporary?: boolean }, ) => Promise; - handleRename: (id: string, name: string) => Promise; handleDelete: (id: string) => Promise; }; @@ -39,7 +37,6 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { // children) and then immediately invokes a handler. const store = useStore(); const createPageMutation = useCreatePageMutation(); - const updatePageMutation = useUpdatePageMutation(); const removePageMutation = useRemovePageMutation(); const movePageMutation = useMovePageMutation(); const navigate = useNavigate(); @@ -205,20 +202,6 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { [spaceId, createPageMutation, setData, store, navigate, spaceSlug], ); - const handleRename = useCallback( - async (id: string, name: string) => { - setData((prev) => - treeModel.update(prev, id, { name } as Partial), - ); - try { - await updatePageMutation.mutateAsync({ pageId: id, title: name }); - } catch (error) { - console.error("Error updating page title:", error); - } - }, - [updatePageMutation, setData], - ); - const handleDelete = useCallback( async (id: string) => { const node = treeModel.find( @@ -264,7 +247,7 @@ export function useTreeMutation(spaceId: string): UseTreeMutation { [removePageMutation, setData, store, pageSlug, navigate, spaceSlug], ); - return { handleMove, handleCreate, handleRename, handleDelete }; + return { handleMove, handleCreate, handleDelete }; } function isPageInNode(node: SpaceTreeNode, pageSlug: string): boolean { diff --git a/apps/server/src/collaboration/collaboration.gateway.ts b/apps/server/src/collaboration/collaboration.gateway.ts index b46c13c8..f681686d 100644 --- a/apps/server/src/collaboration/collaboration.gateway.ts +++ b/apps/server/src/collaboration/collaboration.gateway.ts @@ -24,7 +24,9 @@ import { CollabWsAdapter } from './adapter/collab-ws.adapter'; import { CollaborationHandler, CollabEventHandlers, + writeTitleFragment, } from './collaboration.handler'; +import { User } from '@docmost/db/types/entity.types'; @Injectable() export class CollaborationGateway { @@ -149,6 +151,45 @@ export class CollaborationGateway { return this.hocuspocus.openDirectConnection(documentName, context); } + /** + * Write a new page title INTO the page's Yjs 'title' fragment, Redis-INDEPENDENT. + * + * Unlike the Redis-routed `handleYjsEvent` path — which routes through + * `redisSync?.handleEvent` and SILENTLY no-ops when Redis is disabled + * (COLLAB_DISABLE_REDIS=true → redisSync === null) — this goes straight + * through the local Hocuspocus `openDirectConnection`. The title sync + * therefore works in BOTH single-process (no Redis) and Redis-clustered + * deployments. + * + * openDirectConnection loads the doc from persistence when no editor is + * connected, so this works whether or not an editor is currently open: the + * clear+reseed lands on the loaded doc and is persisted by onStoreDocument. + * + * Provenance: when the caller is the agent, the actor/aiChatId are threaded + * into the connection `context` so onStoreDocument sees `context.actor === + * 'agent'` for the resulting title store (mirrors the body/REST path). The + * resulting title store is usually a no-op anyway — PageService already wrote + * the same title to the page.title column, so onStoreDocument's + * `titleText !== page.title` guard skips the column write — but we wire the + * context for correctness regardless. + */ + async writePageTitle( + pageId: string, + title: string, + context?: { user?: User; actor?: string; aiChatId?: string }, + ): Promise { + const documentName = `page.${pageId}`; + const connection = await this.hocuspocus.openDirectConnection( + documentName, + context ?? {}, + ); + try { + await connection.transact((doc) => writeTitleFragment(doc, title)); + } finally { + await connection.disconnect(); + } + } + /* *Can be used before calling openDirectConnection directly */ diff --git a/apps/server/src/collaboration/collaboration.handler.spec.ts b/apps/server/src/collaboration/collaboration.handler.spec.ts new file mode 100644 index 00000000..8d1fff15 --- /dev/null +++ b/apps/server/src/collaboration/collaboration.handler.spec.ts @@ -0,0 +1,139 @@ +import * as Y from 'yjs'; +import { TiptapTransformer } from '@hocuspocus/transformer'; +import { writeTitleFragment } from './collaboration.handler'; +import { CollaborationGateway } from './collaboration.gateway'; +import { + buildTitleSeedYdoc, + jsonToText, + tiptapExtensions, +} from './collaboration.util'; + +// Read the plain text held in the doc's 'title' XmlFragment, the same way +// PersistenceExtension.onStoreDocument extracts it before writing page.title. +const readTitleText = (doc: Y.Doc): string => { + const titleJson = TiptapTransformer.fromYdoc(doc, 'title'); + return titleJson ? jsonToText(titleJson).trim() : ''; +}; + +describe('writeTitleFragment — the clear+seed title write (Bug 1)', () => { + it('replaces an OLD title fragment with EXACTLY the new title (no duplication)', () => { + // Seed the doc's 'title' fragment with an OLD title, like a real page. + const doc = new Y.Doc(); + Y.applyUpdate(doc, Y.encodeStateAsUpdate(buildTitleSeedYdoc('Old Title'))); + expect(readTitleText(doc)).toBe('Old Title'); + + writeTitleFragment(doc, 'New Title'); + + // The fragment must contain EXACTLY the new title — not "Old TitleNew Title" + // (append) or "New TitleNew Title" (duplication). A single heading node. + expect(readTitleText(doc)).toBe('New Title'); + + const titleJson = TiptapTransformer.fromYdoc(doc, 'title') as any; + expect(titleJson.content).toHaveLength(1); + expect(titleJson.content[0].type).toBe('heading'); + }); + + it('seeds the title fragment when it started empty', () => { + const doc = new Y.Doc(); + // Force the 'title' fragment to exist but be empty. + doc.getXmlFragment('title'); + expect(readTitleText(doc)).toBe(''); + + writeTitleFragment(doc, 'First Title'); + + expect(readTitleText(doc)).toBe('First Title'); + }); + + it('does not corrupt the body when rewriting the title', () => { + // A doc with both a body and an old title; the body must survive untouched. + const doc = new Y.Doc(); + const bodyDoc = TiptapTransformer.toYdoc( + { + type: 'doc', + content: [ + { type: 'paragraph', content: [{ type: 'text', text: 'body text' }] }, + ], + }, + 'default', + tiptapExtensions, + ); + Y.applyUpdate(doc, Y.encodeStateAsUpdate(bodyDoc)); + Y.applyUpdate(doc, Y.encodeStateAsUpdate(buildTitleSeedYdoc('Old'))); + + writeTitleFragment(doc, 'New'); + + expect(readTitleText(doc)).toBe('New'); + const bodyJson = TiptapTransformer.fromYdoc(doc, 'default'); + expect(jsonToText(bodyJson)).toContain('body text'); + }); +}); + +describe('CollaborationGateway.writePageTitle — Redis-independent path', () => { + // Build a gateway with only its hocuspocus.openDirectConnection stubbed; the + // method must drive the clear+seed through that direct connection (NOT through + // redisSync), so the title write survives COLLAB_DISABLE_REDIS. + const makeGateway = (doc: Y.Doc) => { + const disconnect = jest.fn().mockResolvedValue(undefined); + const transact = jest.fn(async (fn: (d: Y.Doc) => void) => { + fn(doc); + }); + const openDirectConnection = jest + .fn() + .mockResolvedValue({ transact, disconnect }); + + const gateway = Object.create(CollaborationGateway.prototype); + // redisSync is intentionally null — this is the no-Redis scenario. + gateway.redisSync = null; + gateway.hocuspocus = { openDirectConnection } as any; + + return { gateway, openDirectConnection, transact, disconnect }; + }; + + it('writes the new title via openDirectConnection and disconnects', async () => { + const doc = new Y.Doc(); + Y.applyUpdate(doc, Y.encodeStateAsUpdate(buildTitleSeedYdoc('Old Title'))); + + const { gateway, openDirectConnection, disconnect } = makeGateway(doc); + + await gateway.writePageTitle('page-1', 'New Title', { user: { id: 'u1' } }); + + expect(openDirectConnection).toHaveBeenCalledWith( + 'page.page-1', + expect.objectContaining({ user: { id: 'u1' } }), + ); + expect(readTitleText(doc)).toBe('New Title'); + expect(disconnect).toHaveBeenCalledTimes(1); + }); + + it('threads agent provenance into the connection context', async () => { + const doc = new Y.Doc(); + const { gateway, openDirectConnection } = makeGateway(doc); + + await gateway.writePageTitle('page-1', 'Agent Title', { + user: { id: 'u1' }, + actor: 'agent', + aiChatId: 'chat-1', + }); + + expect(openDirectConnection).toHaveBeenCalledWith( + 'page.page-1', + expect.objectContaining({ actor: 'agent', aiChatId: 'chat-1' }), + ); + }); + + it('disconnects even when the transaction throws', async () => { + const disconnect = jest.fn().mockResolvedValue(undefined); + const openDirectConnection = jest.fn().mockResolvedValue({ + transact: jest.fn().mockRejectedValue(new Error('boom')), + disconnect, + }); + const gateway = Object.create(CollaborationGateway.prototype); + gateway.redisSync = null; + gateway.hocuspocus = { openDirectConnection } as any; + + await expect( + gateway.writePageTitle('page-1', 'X', {}), + ).rejects.toThrow('boom'); + expect(disconnect).toHaveBeenCalledTimes(1); + }); +}); diff --git a/apps/server/src/collaboration/collaboration.handler.ts b/apps/server/src/collaboration/collaboration.handler.ts index 2d4ae58f..3f7f672c 100644 --- a/apps/server/src/collaboration/collaboration.handler.ts +++ b/apps/server/src/collaboration/collaboration.handler.ts @@ -2,6 +2,7 @@ import { Injectable, Logger } from '@nestjs/common'; import { Hocuspocus, Document } from '@hocuspocus/server'; import { TiptapTransformer } from '@hocuspocus/transformer'; import { + buildTitleSeedYdoc, prosemirrorNodeToYElement, tiptapExtensions, } from './collaboration.util'; @@ -13,6 +14,35 @@ export type CollabEventHandlers = ReturnType< CollaborationHandler['getHandlers'] >; +/** + * Clear+reseed the 'title' XmlFragment of `doc` so it holds EXACTLY `title`. + * + * Used by the gateway's direct `writePageTitle` method to write a new page + * title INTO the page's Yjs 'title' fragment. The title lives in the same + * Y.Doc as the body; onStoreDocument extracts it on every save, so a REST/MCP + * rename that only updated the page.title DB column would be reverted on the + * next collaborative save unless the Yjs 'title' fragment is kept in sync. + * The whole fragment is replaced (no merge/append), + * mirroring the 'replace' body path: the new title fully supersedes the old. + * + * DELIBERATE TRADE-OFF: because this does a FULL clear+replace of the 'title' + * fragment, a REST/MCP rename arriving while a user is actively editing the + * title in an open editor WILL overwrite that in-progress edit. This is + * acceptable — the title is a short, rarely-concurrently-edited field — and is + * preferable to leaving a stale Yjs title that onStoreDocument would revert the + * DB column to on the next save. + */ +export function writeTitleFragment(doc: Y.Doc, title: string): void { + const titleFragment = doc.getXmlFragment('title'); + + if (titleFragment.length > 0) { + titleFragment.delete(0, titleFragment.length); + } + + const newTitleDoc = buildTitleSeedYdoc(title); + Y.applyUpdate(doc, Y.encodeStateAsUpdate(newTitleDoc)); +} + @Injectable() export class CollaborationHandler { private readonly logger = new Logger(CollaborationHandler.name); diff --git a/apps/server/src/collaboration/extensions/persistence.extension.spec.ts b/apps/server/src/collaboration/extensions/persistence.extension.spec.ts index 832367ff..92756aa8 100644 --- a/apps/server/src/collaboration/extensions/persistence.extension.spec.ts +++ b/apps/server/src/collaboration/extensions/persistence.extension.spec.ts @@ -182,6 +182,55 @@ describe('PersistenceExtension', () => { expect(historyQueue.add).not.toHaveBeenCalled(); }); + it('an EMPTY title fragment does NOT overwrite a non-empty page.title (anti-corruption guard, Bug 2)', async () => { + // The client can momentarily seed the 'title' fragment as an EMPTY heading + // (hasTitleFragment true, extracted text '') before the real title syncs. + // Body is unchanged here, so the only candidate write is the title -> the + // guard must turn this into a full no-op (no updatePage, no broadcast). + const document = buildDoc(); + addTitleFragment(document, ''); // empty heading: length > 0 but text '' + const page = basePage({ + content: cloneOut(document), + title: 'Real Title', + }); + pageRepo.findById.mockResolvedValue(page); + + await ext.onStoreDocument({ + documentName: 'page.PAGE_ID', + document, + context, + } as any); + + // No write at all: the empty title is not authoritative and the body is + // unchanged, so onStoreDocument must take the no-op fast path. + expect(pageRepo.updatePage).not.toHaveBeenCalled(); + expect(document.broadcastStateless).not.toHaveBeenCalled(); + }); + + it('an EMPTY title fragment alongside a body change persists the body but NOT an empty title (anti-corruption guard, Bug 2)', async () => { + const document = buildDoc(); + addTitleFragment(document, ''); // empty title fragment + const page = basePage({ + content: { type: 'doc', content: [] }, // different body -> bodyChanged + title: 'Real Title', + }); + pageRepo.findById.mockResolvedValue(page); + + await ext.onStoreDocument({ + documentName: 'page.PAGE_ID', + document, + context, + } as any); + + expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); + const call = pageRepo.updatePage.mock.calls[0]; + // Body is persisted, but the title is NOT included (empty == not + // authoritative) and no tree update is broadcast for the title. + expect(call[0].content).toBeTruthy(); + expect('title' in call[0]).toBe(false); + expect(call[3]).toBeUndefined(); + }); + it('body + title change persists both with full body side-effects', async () => { const document = buildDoc(); addTitleFragment(document, 'New Title'); diff --git a/apps/server/src/collaboration/extensions/persistence.extension.ts b/apps/server/src/collaboration/extensions/persistence.extension.ts index 06458654..f44df212 100644 --- a/apps/server/src/collaboration/extensions/persistence.extension.ts +++ b/apps/server/src/collaboration/extensions/persistence.extension.ts @@ -319,8 +319,26 @@ export class PersistenceExtension implements Extension { bodyChanged = !isDeepStrictEqual(tiptapJson, page.content); // Only a populated 'title' fragment may update page.title; compare // against the current column value (treat null as ''). + // + // ANTI-CORRUPTION GUARD (Bug 2): the client's collaborative title-editor + // can momentarily initialize the 'title' fragment as an EMPTY heading + // (so `hasTitleFragment` is true, but the extracted `titleText` is '') + // BEFORE the server's real-title seed has synced. Writing that '' would + // silently wipe a non-empty page.title to "untitled". A wiki page is + // never legitimately retitled to empty via this path, so we treat an + // empty extracted title as "not authoritative" and never persist it. + // The `titleText.length > 0` clause makes this guard apply to BOTH the + // title-only branch and the body+title branch below. + // + // DELIBERATE: this intentionally makes it impossible to retitle a page + // to EMPTY via the collab path — a wiki page is never legitimately + // empty-titled. If a non-empty-title rule ever needs relaxing or + // enforcing differently, the REST UpdatePageDto is the place to validate + // the title, not this collab guard. const titleChanged = - hasTitleFragment && titleText !== (page.title ?? ''); + hasTitleFragment && + titleText.length > 0 && + titleText !== (page.title ?? ''); // No-op fast path: neither body nor title changed. if (!bodyChanged && !titleChanged) { diff --git a/apps/server/src/core/page/services/page.service.spec.ts b/apps/server/src/core/page/services/page.service.spec.ts index a6ba89c6..925023cb 100644 --- a/apps/server/src/core/page/services/page.service.spec.ts +++ b/apps/server/src/core/page/services/page.service.spec.ts @@ -31,6 +31,102 @@ describe('PageService', () => { expect(service).toBeDefined(); }); + describe('update — title sync into collab doc (Bug 1)', () => { + const makeUpdateService = () => { + const pageRepo = { + updatePage: jest.fn().mockResolvedValue(undefined), + findById: jest.fn().mockResolvedValue({ id: 'page-1' }), + }; + const generalQueue = { add: jest.fn().mockResolvedValue(undefined) }; + const collaborationGateway = { + writePageTitle: jest.fn().mockResolvedValue(undefined), + }; + + const svc = new PageService( + pageRepo as any, // pageRepo + {} as any, // pagePermissionRepo + {} as any, // attachmentRepo + {} as any, // db + {} as any, // storageService + {} as any, // attachmentQueue + {} as any, // aiQueue + generalQueue as any, // generalQueue + {} as any, // eventEmitter + collaborationGateway as any, // collaborationGateway + {} as any, // watcherService + {} as any, // transclusionService + ); + + return { svc, pageRepo, collaborationGateway }; + }; + + const basePage = (): Page => + ({ + id: 'page-1', + slugId: 'slug-1', + spaceId: 'space-1', + workspaceId: 'ws-1', + parentPageId: null, + title: 'Old Title', + icon: null, + contributorIds: [], + }) as any; + + const user = { id: 'u1' } as any; + + it('writes the new title into the collab doc when the title actually changed', async () => { + const { svc, collaborationGateway } = makeUpdateService(); + + await svc.update(basePage(), { title: 'New Title' } as any, user); + + // Must use the Redis-independent writePageTitle (direct + // openDirectConnection), NOT handleYjsEvent which no-ops without Redis. + expect(collaborationGateway.writePageTitle).toHaveBeenCalledTimes(1); + expect(collaborationGateway.writePageTitle).toHaveBeenCalledWith( + 'page-1', + 'New Title', + expect.objectContaining({ user }), + ); + }); + + it('threads agent provenance into the collab title write', async () => { + const { svc, collaborationGateway } = makeUpdateService(); + + await svc.update(basePage(), { title: 'New Title' } as any, user, { + actor: 'agent', + aiChatId: 'chat-1', + } as any); + + expect(collaborationGateway.writePageTitle).toHaveBeenCalledWith( + 'page-1', + 'New Title', + expect.objectContaining({ actor: 'agent', aiChatId: 'chat-1' }), + ); + }); + + it('does NOT write into the collab doc when the title is unchanged', async () => { + const { svc, collaborationGateway } = makeUpdateService(); + + // Same title -> titleChanged is false; an icon-only change must not fire + // the title sync. + await svc.update( + basePage(), + { title: 'Old Title', icon: '📄' } as any, + user, + ); + + expect(collaborationGateway.writePageTitle).not.toHaveBeenCalled(); + }); + + it('does NOT write into the collab doc when the DTO omits the title', async () => { + const { svc, collaborationGateway } = makeUpdateService(); + + await svc.update(basePage(), { icon: '📄' } as any, user); + + expect(collaborationGateway.writePageTitle).not.toHaveBeenCalled(); + }); + }); + describe('movePage cycle guard (#67)', () => { // A valid fractional-indexing key — movePage validates `position` by feeding // it to generateJitteredKeyBetween(position, null) before anything else. diff --git a/apps/server/src/core/page/services/page.service.ts b/apps/server/src/core/page/services/page.service.ts index c6ee150d..161feae3 100644 --- a/apps/server/src/core/page/services/page.service.ts +++ b/apps/server/src/core/page/services/page.service.ts @@ -265,6 +265,8 @@ export class PageService { contributors.add(user.id); const contributorIds = Array.from(contributors); + const isAgent = provenance?.actor === 'agent'; + // Detect a real title/icon change so the WS tree listener can broadcast an // `updateOne` to the space (rename / icon swap) WITHOUT re-broadcasting on a // content-only save. Only treat a field as changed when the DTO actually @@ -307,6 +309,43 @@ export class PageService { : undefined, ); + // Bug 1: a REST/MCP rename wrote the new title ONLY to the page.title DB + // column above. The title's source of truth is the Yjs 'title' fragment in + // the page's collab doc, which onStoreDocument re-extracts on every save — + // so leaving the fragment stale would REVERT this rename on the page's next + // collaborative save (and re-broadcast the old title). Push the new title + // into the Yjs 'title' fragment so Yjs stays in sync and never reverts. + // + // Use the gateway's writePageTitle (direct openDirectConnection) rather than + // a Redis-routed handleYjsEvent path: handleYjsEvent routes through + // redisSync and SILENTLY no-ops when Redis is disabled + // (COLLAB_DISABLE_REDIS=true), which would let the rename revert in a + // single-process deployment. writePageTitle is Redis-independent and + // openDirectConnection loads the doc from persistence when no editor is + // connected, so this also works for an offline page. Thread agent provenance + // into the context so onStoreDocument tags the title store 'agent' too. + if (titleChanged) { + try { + await this.collaborationGateway.writePageTitle( + page.id, + updatePageDto.title, + { + user, + ...(isAgent + ? { actor: 'agent', aiChatId: provenance.aiChatId } + : {}), + }, + ); + } catch (err) { + // The DB column write already succeeded (fast-read source stays + // correct); a failure to sync Yjs here must not fail the rename. Log so + // a persistent desync is visible. + this.logger.warn( + `Failed to sync renamed title into collab doc for page ${page.id}: ${err?.['message']}`, + ); + } + } + this.generalQueue .add(QueueJob.ADD_PAGE_WATCHERS, { userIds: [user.id],