diff --git a/apps/client/src/features/editor/extensions/extensions.ts b/apps/client/src/features/editor/extensions/extensions.ts index 63855097..fa6ae536 100644 --- a/apps/client/src/features/editor/extensions/extensions.ts +++ b/apps/client/src/features/editor/extensions/extensions.ts @@ -123,6 +123,7 @@ import { countWords } from "alfaaz"; import AutoJoiner from "@/features/editor/extensions/autojoiner.ts"; import GlobalDragHandle from "@/features/editor/extensions/drag-handle.ts"; import { CleanStyles } from "@/features/editor/extensions/clean-styles.ts"; +import { IntentionalClear } from "@/features/editor/extensions/intentional-clear.ts"; const lowlight = createLowlight(common); lowlight.register("mermaid", plaintext); @@ -486,4 +487,10 @@ export const collabExtensions: CollabExtensions = (provider, user) => [ color: randomElement(userColors), }, }), + // #251 — emit an intentional-clear signal to the server when the user + // deliberately empties the page, so the #248 store-side empty-guard lets that + // one clear through while still blocking accidental empties. + IntentionalClear.configure({ + provider, + }), ]; diff --git a/apps/client/src/features/editor/extensions/intentional-clear.test.ts b/apps/client/src/features/editor/extensions/intentional-clear.test.ts new file mode 100644 index 00000000..ad467327 --- /dev/null +++ b/apps/client/src/features/editor/extensions/intentional-clear.test.ts @@ -0,0 +1,88 @@ +import { describe, it, expect, vi, beforeEach } from "vitest"; +import { Editor } from "@tiptap/core"; +import { Document } from "@tiptap/extension-document"; +import { Paragraph } from "@tiptap/extension-paragraph"; +import { Text } from "@tiptap/extension-text"; +import { + IntentionalClear, + INTENTIONAL_CLEAR_MESSAGE_TYPE, +} from "./intentional-clear"; + +/** + * #251 — the intentional-clear signal is driven through the REAL editor path: + * a fresh Editor with the IntentionalClear extension, a fake provider that + * records sendStateless, and the actual select-all + delete command the user's + * keystroke runs. No hand-poke of any flag. + */ +describe("IntentionalClear extension", () => { + let sendStateless: ReturnType; + + const makeEditor = (content: unknown) => + new Editor({ + extensions: [ + Document, + Paragraph, + Text, + IntentionalClear.configure({ + // Minimal provider stand-in: only sendStateless is exercised. + provider: { sendStateless } as any, + }), + ], + content: content as any, + }); + + beforeEach(() => { + sendStateless = vi.fn(); + }); + + it("emits the clear signal when a user empties a non-empty doc (select-all + delete)", () => { + const editor = makeEditor({ + type: "doc", + content: [ + { type: "paragraph", content: [{ type: "text", text: "hello world" }] }, + ], + }); + + // The exact command path a select-all + Delete keystroke dispatches. + editor.chain().selectAll().deleteSelection().run(); + + expect(sendStateless).toHaveBeenCalledTimes(1); + const payload = JSON.parse(sendStateless.mock.calls[0][0]); + expect(payload).toEqual({ type: INTENTIONAL_CLEAR_MESSAGE_TYPE }); + + editor.destroy(); + }); + + it("does NOT emit when typing into an empty doc (no non-empty → empty transition)", () => { + const editor = makeEditor({ type: "doc", content: [{ type: "paragraph" }] }); + + editor.chain().insertContent("typed text").run(); + + expect(sendStateless).not.toHaveBeenCalled(); + editor.destroy(); + }); + + it("does NOT emit on an edit that leaves the doc non-empty", () => { + const editor = makeEditor({ + type: "doc", + content: [ + { type: "paragraph", content: [{ type: "text", text: "keep me" }] }, + ], + }); + + editor.chain().insertContent(" more").run(); + + expect(sendStateless).not.toHaveBeenCalled(); + editor.destroy(); + }); + + it("does NOT emit when the doc was already empty", () => { + const editor = makeEditor({ type: "doc", content: [{ type: "paragraph" }] }); + + // Selecting all + delete on an already-empty doc is a no-op transition. + editor.chain().selectAll().deleteSelection().run(); + + expect(sendStateless).not.toHaveBeenCalled(); + editor.destroy(); + }); +}); diff --git a/apps/client/src/features/editor/extensions/intentional-clear.ts b/apps/client/src/features/editor/extensions/intentional-clear.ts new file mode 100644 index 00000000..7652068c --- /dev/null +++ b/apps/client/src/features/editor/extensions/intentional-clear.ts @@ -0,0 +1,94 @@ +import { Extension } from "@tiptap/core"; +import { isChangeOrigin } from "@tiptap/extension-collaboration"; +import type { Node as PMNode } from "@tiptap/pm/model"; +import type { HocuspocusProvider } from "@hocuspocus/provider"; + +/** + * Stateless message type sent to the server when a user deliberately clears a + * page to empty. Kept in one place so the client emitter and the server + * consumer (PersistenceExtension.onStateless) agree on the wire format. + */ +export const INTENTIONAL_CLEAR_MESSAGE_TYPE = "intentional-clear"; + +export interface IntentionalClearOptions { + /** The collab provider used to send the stateless clear signal. */ + provider: HocuspocusProvider | null; +} + +/** + * A "document is empty" check that mirrors the server's `isEmptyParagraphDoc` + * (collaboration.util.ts): exactly one top-level paragraph with no inline + * content. After a select-all + delete TipTap leaves precisely this shape, so + * matching it here keeps the client signal aligned with the server guard that + * consumes it. + */ +function isEmptyParagraphDoc(doc: PMNode): boolean { + if (doc.childCount !== 1) return false; + const child = doc.firstChild; + return ( + child !== null && + child !== undefined && + child.type.name === "paragraph" && + child.content.size === 0 + ); +} + +/** + * #251 — intentional-clear signal. + * + * The server's #248 store-side empty-guard unconditionally refuses to overwrite + * non-empty persisted content with an empty document, because a momentarily + * empty live Y.Doc (a glitch, a bad merge, an emptying transclusion) is + * indistinguishable from a real clear *at the store layer*. That protection is + * correct, but it also blocks a user who genuinely wants to empty the page. + * + * This extension supplies the missing distinction. It watches LOCAL, user-driven + * transactions and, the moment one reduces a non-empty document to the empty + * single-paragraph shape, it sends a hocuspocus stateless message to the server. + * The server records a short-lived, single-use "intentional clear pending" flag + * for this document that the next (debounced) onStoreDocument consumes to let + * that one empty write through the guard. + * + * What counts as an intentional clear (precise definition): + * - the transaction actually changed the document (`docChanged`), AND + * - it is a LOCAL user edit, not a remote collab application — remote y-sync + * transactions are tagged and filtered out via `isChangeOrigin`, so an + * emptiness that arrives from another client / a merge never emits a signal, + * AND + * - the document was non-empty before the transaction and is the empty + * single-paragraph doc after it. + * + * This is exactly the select-all + Delete / Backspace (or any local command that + * empties the doc, e.g. clearContent) keystroke path. A transient/programmatic + * empty serialization that the server might see on the wire does NOT come with + * this signal, so the guard still blocks it. + */ +export const IntentionalClear = Extension.create({ + name: "intentionalClear", + + addOptions() { + return { + provider: null, + }; + }, + + onTransaction({ transaction }) { + if (!transaction.docChanged) return; + // Only react to local user edits. Remote collaboration steps (and other + // y-sync-applied changes) carry the change origin and must never be treated + // as an intentional clear, otherwise a remote/merge-induced emptiness would + // punch through the server guard. + if (isChangeOrigin(transaction)) return; + + const becameEmpty = + !isEmptyParagraphDoc(transaction.before) && + isEmptyParagraphDoc(transaction.doc); + if (!becameEmpty) return; + + // The server reads the originating document from the connection, so the + // payload only needs to declare intent — it cannot target another document. + this.options.provider?.sendStateless( + JSON.stringify({ type: INTENTIONAL_CLEAR_MESSAGE_TYPE }), + ); + }, +}); diff --git a/apps/server/src/collaboration/extensions/persistence-store.spec.ts b/apps/server/src/collaboration/extensions/persistence-store.spec.ts index 8bc713bf..ce3fbf76 100644 --- a/apps/server/src/collaboration/extensions/persistence-store.spec.ts +++ b/apps/server/src/collaboration/extensions/persistence-store.spec.ts @@ -205,31 +205,158 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' expect(historyQueue.add).toHaveBeenCalledTimes(1); }); - // #206 persist-6 — RED (it.failing): a momentarily-empty live Y.Doc must not - // overwrite non-empty persisted content. `onStoreDocument` empty-guards the - // LOAD path but not the STORE path, so today an empty doc (a client/agent - // glitch, a bad merge, an emptying transclusion) is written straight over the - // page and the content is wiped silently. A store-side empty-guard is a real - // behaviour change (a deliberate "select-all + delete" is also empty), so it - // is left UNFIXED pending a product decision; this documents the data-loss - // path and flips to a normal passing test the moment the guard lands. - it.failing( - 'does NOT overwrite non-empty content with a momentarily-empty live doc (persist-6)', - async () => { - const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; - const document = ydocFor(emptyDoc); - pageRepo.findById.mockResolvedValue({ - ...persistedHumanPage('IGNORED'), - content: doc('IMPORTANT RICH CONTENT'), - }); + // #206 persist-6 / #248 — a momentarily-empty live Y.Doc must not overwrite + // non-empty persisted content. The store-side empty-guard blocks an empty doc + // (a client/agent glitch, a bad merge, an emptying transclusion) from wiping + // the page silently when NO intentional-clear signal is present. + it('does NOT overwrite non-empty content with a momentarily-empty live doc (persist-6)', async () => { + const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; + const document = ydocFor(emptyDoc); + pageRepo.findById.mockResolvedValue({ + ...persistedHumanPage('IGNORED'), + content: doc('IMPORTANT RICH CONTENT'), + }); - await ext.onStoreDocument(buildData(document, 'user') as any); + await ext.onStoreDocument(buildData(document, 'user') as any); - // Desired contract: the empty incoming doc is rejected and the rich page - // survives. Today updatePage is called with the empty content (data loss). - expect(pageRepo.updatePage).not.toHaveBeenCalled(); - }, - ); + // The empty incoming doc is rejected and the rich page survives. + expect(pageRepo.updatePage).not.toHaveBeenCalled(); + }); + + // #248 — an empty-over-empty store is allowed (nothing to lose); the guard + // only protects non-empty persisted content. + it('allows an empty store over already-empty content (#248)', async () => { + const liveEmptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; + const document = ydocFor(liveEmptyDoc); + // Stored content is empty per isEmptyParagraphDoc (paragraph with content:[]) + // but NOT deep-equal to the normalized live doc, so the unchanged + // short-circuit is skipped and the empty-guard is genuinely reached. + pageRepo.findById.mockResolvedValue({ + ...persistedHumanPage('IGNORED'), + content: { type: 'doc', content: [{ type: 'paragraph', content: [] }] }, + }); + + await ext.onStoreDocument(buildData(document, 'user') as any); + + expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); + }); + + // #251 — REAL-PATH regression test. The intentional-clear signal is set via + // the actual transport seam (ext.onStateless with the exact stateless payload + // the client's IntentionalClear extension sends), NOT a hand-injected + // context.intentionalClear poke. We then run the debounced store with an empty + // live doc over non-empty persisted content and assert the empty write goes + // through — i.e. the clear persists. + it('persists an intentional clear signalled via the real stateless transport (#251)', async () => { + const documentName = `page.${PAGE_ID}`; + const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; + const document = ydocFor(emptyDoc); + pageRepo.findById.mockResolvedValue({ + ...persistedHumanPage('IGNORED'), + content: doc('IMPORTANT RICH CONTENT'), + }); + + // The client signalled a deliberate clear over the live connection. + await ext.onStateless({ + connection: { readOnly: false } as any, + documentName, + document: document as any, + payload: JSON.stringify({ type: 'intentional-clear' }), + } as any); + + await ext.onStoreDocument(buildData(document, 'user') as any); + + // The empty doc was written (the clear persisted). The persisted content is + // the Y.Doc round-trip of the empty doc (attrs normalized), so compare + // against fromYdoc rather than the raw literal. + expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); + const expectedEmpty = TiptapTransformer.fromYdoc(document, 'default'); + expect(pageRepo.updatePage.mock.calls[0][0].content).toEqual(expectedEmpty); + }); + + // #251 — the signal is single-use: it is consumed by the first empty store, + // so a SECOND accidental empty (no fresh signal) is still blocked. + it('consumes the intentional-clear signal once; a later empty is blocked (#251)', async () => { + const documentName = `page.${PAGE_ID}`; + const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; + pageRepo.findById.mockResolvedValue({ + ...persistedHumanPage('IGNORED'), + content: doc('IMPORTANT RICH CONTENT'), + }); + + await ext.onStateless({ + connection: { readOnly: false } as any, + documentName, + document: ydocFor(emptyDoc) as any, + payload: JSON.stringify({ type: 'intentional-clear' }), + } as any); + + // First empty store consumes the signal and writes. + await ext.onStoreDocument(buildData(ydocFor(emptyDoc), 'user') as any); + expect(pageRepo.updatePage).toHaveBeenCalledTimes(1); + + // Re-arm findById to non-empty (as if content came back) and fire another + // empty store WITHOUT a new signal — the guard must block it. + pageRepo.updatePage.mockClear(); + pageRepo.findById.mockResolvedValue({ + ...persistedHumanPage('IGNORED'), + content: doc('IMPORTANT RICH CONTENT'), + }); + await ext.onStoreDocument(buildData(ydocFor(emptyDoc), 'user') as any); + expect(pageRepo.updatePage).not.toHaveBeenCalled(); + }); + + // #251 — a read-only connection cannot arm the clear, so its empty store is + // still blocked (defends the guard against a read-only spoof). + it('ignores an intentional-clear signal from a read-only connection (#251)', async () => { + const documentName = `page.${PAGE_ID}`; + const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; + const document = ydocFor(emptyDoc); + pageRepo.findById.mockResolvedValue({ + ...persistedHumanPage('IGNORED'), + content: doc('IMPORTANT RICH CONTENT'), + }); + + await ext.onStateless({ + connection: { readOnly: true } as any, + documentName, + document: document as any, + payload: JSON.stringify({ type: 'intentional-clear' }), + } as any); + + await ext.onStoreDocument(buildData(document, 'user') as any); + + expect(pageRepo.updatePage).not.toHaveBeenCalled(); + }); + + // #251 — a non-empty store between the signal and the empty store drops the + // pending flag ("cleared then retyped" can't leave a usable signal behind). + it('drops a pending clear when a non-empty store intervenes (#251)', async () => { + const documentName = `page.${PAGE_ID}`; + const emptyDoc = { type: 'doc', content: [{ type: 'paragraph' }] }; + + await ext.onStateless({ + connection: { readOnly: false } as any, + documentName, + document: ydocFor(emptyDoc) as any, + payload: JSON.stringify({ type: 'intentional-clear' }), + } as any); + + // A non-empty store lands first → consumes/drops the stale flag. + pageRepo.findById.mockResolvedValue(persistedHumanPage('NEW HUMAN TEXT')); + await ext.onStoreDocument( + buildData(ydocFor(doc('NEW HUMAN TEXT')), 'user') as any, + ); + pageRepo.updatePage.mockClear(); + + // Now an empty store with no fresh signal must be blocked. + pageRepo.findById.mockResolvedValue({ + ...persistedHumanPage('IGNORED'), + content: doc('IMPORTANT RICH CONTENT'), + }); + await ext.onStoreDocument(buildData(ydocFor(emptyDoc), 'user') as any); + expect(pageRepo.updatePage).not.toHaveBeenCalled(); + }); // persist-1 — when every attempt fails the hook must NOT report a phantom // success: no "page.updated" badge broadcast and no history snapshot for diff --git a/apps/server/src/collaboration/extensions/persistence.extension.ts b/apps/server/src/collaboration/extensions/persistence.extension.ts index f802f229..081589f1 100644 --- a/apps/server/src/collaboration/extensions/persistence.extension.ts +++ b/apps/server/src/collaboration/extensions/persistence.extension.ts @@ -3,6 +3,7 @@ import { Extension, onChangePayload, onLoadDocumentPayload, + onStatelessPayload, onStoreDocumentPayload, } from '@hocuspocus/server'; import * as Y from 'yjs'; @@ -41,6 +42,26 @@ import { } from '../constants'; import { TransclusionService } from '../../core/page/transclusion/transclusion.service'; +/** + * #251 — wire format of the client→server stateless message that signals a + * deliberate page clear. The client (IntentionalClear editor extension) sends + * `{ type: INTENTIONAL_CLEAR_MESSAGE_TYPE }`; the document is taken from the + * connection, not the payload, so the signal cannot be aimed at another page. + */ +export const INTENTIONAL_CLEAR_MESSAGE_TYPE = 'intentional-clear'; + +/** + * #251 — how long an intentional-clear signal stays "pending" before it is + * ignored. The signal is set on the clearing keystroke but consumed by the + * DEBOUNCED onStoreDocument, so the TTL must comfortably exceed the collab + * store debounce window (hocuspocus is configured with maxDebounce = 45s in + * collaboration.gateway.ts). 60s leaves a margin while keeping the window for a + * stale flag small; on top of the TTL, any non-empty store immediately drops a + * pending flag (see onStoreDocument), so a "cleared then retyped" sequence can + * never leave a usable flag behind. + */ +export const INTENTIONAL_CLEAR_TTL_MS = 60_000; + /** * Resolve the provenance source for a coalesced snapshot. * @@ -96,6 +117,13 @@ export class PersistenceExtension implements Extension { // coalescing window" per document and OR it across all edits in the window, // so the snapshot is marked 'agent' regardless of who wrote last. private agentTouched: Map = new Map(); + // #251 — per-document "intentional clear pending" flags. Keyed by + // documentName, value = expiry timestamp (ms). Set by onStateless when the + // client reports a deliberate clear; consumed once by the next + // onStoreDocument empty-guard branch. This is the per-EDIT channel the + // per-connection context cannot provide (a clear is an edit event, but the + // store is debounced and connection context is fixed at authentication). + private intentionalClear: Map = new Map(); constructor( private readonly pageRepo: PageRepo, @@ -210,6 +238,43 @@ export class PersistenceExtension implements Extension { return; } + // #206 persist-6 / #248 — store-side empty-guard. A momentarily-empty + // live Y.Doc (a client/agent glitch, a bad merge, a transclusion that + // emptied) must NOT overwrite non-empty persisted content. The LOAD + // path already guards emptiness (onLoadDocument only hydrates from db + // when the live doc isEmpty); the STORE path did not, so an empty + // serialization was written straight over the page, wiping it + // silently. + // + // #251 — the ONE legitimate empty-over-non-empty write is a user who + // deliberately clears the page. That intent arrives out-of-band as a + // stateless message (consumed below), NOT from the doc content, which + // is why it cannot be spoofed for non-clear writes: the flag is only + // ever read on this empty-incoming branch, so the worst a forged + // signal can do is clear a page the connection may already edit. Every + // non-empty write ignores the flag entirely and additionally drops any + // pending flag (a "cleared then retyped" sequence can't leave a usable + // one behind). + const incomingEmpty = isEmptyParagraphDoc(tiptapJson as any); + if (!incomingEmpty) { + this.intentionalClear.delete(documentName); + } else if (page.content && !isEmptyParagraphDoc(page.content as any)) { + if (this.consumeIntentionalClear(documentName)) { + this.logger.debug( + `Intentional clear for ${pageId}: persisting empty doc over ` + + `non-empty content (user-signalled)`, + ); + // fall through — the empty write is allowed exactly once. + } else { + this.logger.warn( + `Skipping store for ${pageId}: empty live doc would overwrite ` + + `non-empty persisted content`, + ); + page = null; + return; + } + } + let contributorIds = undefined; try { const existingContributors = page.contributorIds || []; @@ -345,6 +410,37 @@ export class PersistenceExtension implements Extension { } } + /** + * #251 — receive the client's deliberate-clear signal. Records a short-lived, + * single-use pending flag for the originating document so the next + * onStoreDocument may let one empty-over-non-empty write through the guard. + * + * Hardening: read-only connections cannot arm the flag, and the document is + * taken from the connection (`data.documentName`), never the payload, so a + * client cannot target a page it isn't editing. The flag only ever RELAXES + * the guard for an empty write (a clear); it can never force or alter a + * non-empty write, so it is not a guard bypass for normal content. + */ + async onStateless(data: onStatelessPayload) { + const { connection, documentName, payload } = data; + + if (connection?.readOnly) return; + + let message: { type?: string } | undefined; + try { + message = JSON.parse(payload); + } catch { + return; // unrelated / malformed stateless message + } + + if (message?.type !== INTENTIONAL_CLEAR_MESSAGE_TYPE) return; + + this.intentionalClear.set( + documentName, + Date.now() + INTENTIONAL_CLEAR_TTL_MS, + ); + } + async onChange(data: onChangePayload) { const documentName = data.documentName; const userId = data.context?.user?.id; @@ -368,6 +464,7 @@ export class PersistenceExtension implements Extension { const documentName = data.documentName; this.contributors.delete(documentName); this.agentTouched.delete(documentName); + this.intentionalClear.delete(documentName); } private consumeContributors(documentName: string): string[] { @@ -385,6 +482,18 @@ export class PersistenceExtension implements Extension { return touched; } + /** + * #251 — read and clear the intentional-clear flag for this document. Returns + * true only if a flag was pending AND still within its TTL. Always deletes the + * entry so the signal is strictly single-use (one clear → one allowed empty + * write); an expired flag is treated as absent (guard still blocks). + */ + private consumeIntentionalClear(documentName: string): boolean { + const expiry = this.intentionalClear.get(documentName); + this.intentionalClear.delete(documentName); + return expiry !== undefined && Date.now() < expiry; + } + private async enqueuePageHistory( page: Page, lastUpdatedSource: string,