From 587a940959acc4d1c1b5f4cc67baa6eac711f0ff Mon Sep 17 00:00:00 2001 From: claude code agent 227 Date: Sat, 20 Jun 2026 15:44:08 +0300 Subject: [PATCH] perf+fix(footnotes): minimal-diff sync (no concurrent-edit loss); cache numbering Release-cycle review found two hardening gaps: - The sync plugin deleted+rebuilt the WHOLE footnotesList on any reorder/orphan, replacing every definition's Yjs subtree -> a collaborator typing in a definition could lose in-flight characters on merge. Rework to targeted, minimal mutations: attr-only setNodeMarkup for collision re-ids, delete only genuine orphans, insert only genuinely-missing definitions (at the list end, not shifting existing subtrees), and consolidate multiple lists only in the abnormal paste/merge case. An unchanged (correct id, referenced) definition is left completely untouched. Numbering is decoration-only, so physical list order may drift after a reorder (accepted) while displayed numbers stay correct. Invariants preserved (reviewed + tested): one SYNC_META transaction, null when canonical (terminates), deterministic deriveFootnoteId, remote-skip -> no re-introduced freeze or divergence. - computeFootnoteNumbers ran per-NodeView-render (O(n^2)/keystroke in big docs). The numbering plugin now caches the number map in its state (computed once per docChanged); NodeViews read it O(1) via getFootnoteNumber. Tests: no-rebuild-on-reorder asserts unchanged definition node subtrees are identity-preserved; isRemoteTransaction skip; enableSync:false read-only; cache correctness. Browser re-smoke: insert (no freeze), number, persist across reload, cascade delete all pass. Co-Authored-By: Claude Opus 4.8 --- .../footnote/footnote-definition-view.tsx | 7 +- .../footnote/footnote-reference-view.tsx | 9 +- .../src/lib/footnote/footnote-numbering.ts | 56 +++- .../src/lib/footnote/footnote-sync.ts | 284 +++++++++++++----- .../src/lib/footnote/footnote.test.ts | 258 ++++++++++++++++ 5 files changed, 524 insertions(+), 90 deletions(-) diff --git a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx index b5aa5486..2685fbc3 100644 --- a/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx +++ b/apps/client/src/features/editor/components/footnote/footnote-definition-view.tsx @@ -1,6 +1,6 @@ import { NodeViewContent, NodeViewProps, NodeViewWrapper } from "@tiptap/react"; import { useTranslation } from "react-i18next"; -import { computeFootnoteNumbers } from "@docmost/editor-ext"; +import { getFootnoteNumber } from "@docmost/editor-ext"; import classes from "./footnote.module.css"; /** @@ -13,8 +13,9 @@ export default function FootnoteDefinitionView(props: NodeViewProps) { const { t } = useTranslation(); const id = node.attrs.id as string; - const numbers = computeFootnoteNumbers(editor.state.doc); - const number = numbers.get(id) ?? "?"; + // Read the cached number from the numbering plugin (computed once per doc + // change) rather than recomputing the whole map on every render. + const number = getFootnoteNumber(editor.state, id) ?? "?"; const handleBack = (e: React.MouseEvent) => { e.preventDefault(); diff --git a/apps/client/src/features/editor/components/footnote/footnote-reference-view.tsx b/apps/client/src/features/editor/components/footnote/footnote-reference-view.tsx index c75766da..7ea9e87d 100644 --- a/apps/client/src/features/editor/components/footnote/footnote-reference-view.tsx +++ b/apps/client/src/features/editor/components/footnote/footnote-reference-view.tsx @@ -11,7 +11,7 @@ import { } from "@floating-ui/dom"; import { FOOTNOTE_DEFINITION_NAME, - computeFootnoteNumbers, + getFootnoteNumber, } from "@docmost/editor-ext"; import { ActionIcon } from "@mantine/core"; import { IconArrowDown } from "@tabler/icons-react"; @@ -45,9 +45,10 @@ export default function FootnoteReferenceView(props: NodeViewProps) { const popoverRef = useRef(null); const [open, setOpen] = useState(false); - // Number is derived (not stored) — recompute from the current doc. - const numbers = computeFootnoteNumbers(editor.state.doc); - const number = numbers.get(id) ?? "?"; + // Number is derived (not stored). Read it from the numbering plugin's cached + // map (computed once per doc change) instead of walking the whole document on + // every render — recomputing per NodeView per render was O(n^2) per keystroke. + const number = getFootnoteNumber(editor.state, id) ?? "?"; const defText = open ? getDefinitionText(editor, id) : ""; const position = useCallback(() => { diff --git a/packages/editor-ext/src/lib/footnote/footnote-numbering.ts b/packages/editor-ext/src/lib/footnote/footnote-numbering.ts index f93a3b08..8a487b1f 100644 --- a/packages/editor-ext/src/lib/footnote/footnote-numbering.ts +++ b/packages/editor-ext/src/lib/footnote/footnote-numbering.ts @@ -1,4 +1,4 @@ -import { Plugin, PluginKey } from "@tiptap/pm/state"; +import { EditorState, Plugin, PluginKey } from "@tiptap/pm/state"; import { Decoration, DecorationSet } from "@tiptap/pm/view"; import { Node as ProseMirrorNode } from "@tiptap/pm/model"; import { @@ -7,7 +7,23 @@ import { computeFootnoteNumbers, } from "./footnote-util"; -export const footnoteNumberingPluginKey = new PluginKey("footnoteNumbering"); +export const footnoteNumberingPluginKey = new PluginKey( + "footnoteNumbering", +); + +/** + * Cached state of the numbering plugin. Both the displayed-number map and the + * decoration set are computed ONCE per doc-changing transaction (in `apply`) and + * cached here, so NodeViews can read a footnote's number by id without walking + * the whole document on every React render (which was O(n^2) per keystroke in + * large docs). + */ +interface FootnoteNumberingState { + /** referenceId -> 1-based display number, for the current doc. */ + numbers: Map; + /** Decorations rendering those numbers (refs + definitions). */ + decorations: DecorationSet; +} /** * Build the decoration set for footnote numbers. Pure function of the document: @@ -18,6 +34,17 @@ export const footnoteNumberingPluginKey = new PluginKey("footnoteNumbering"); * with no document mutation. */ export function buildFootnoteDecorations(doc: ProseMirrorNode): DecorationSet { + return buildFootnoteNumberingState(doc).decorations; +} + +/** + * Compute both the number map AND the decorations for `doc` in a single walk. + * The plugin caches the result so NodeViews can read numbers without + * recomputing. + */ +function buildFootnoteNumberingState( + doc: ProseMirrorNode, +): FootnoteNumberingState { const numbers = computeFootnoteNumbers(doc); const decorations: Decoration[] = []; @@ -46,7 +73,21 @@ export function buildFootnoteDecorations(doc: ProseMirrorNode): DecorationSet { } }); - return DecorationSet.create(doc, decorations); + return { numbers, decorations: DecorationSet.create(doc, decorations) }; +} + +/** + * Read the cached footnote number for `id` from the numbering plugin's state. + * This is the source NodeViews should use instead of calling + * computeFootnoteNumbers() on every render (that walked the whole doc per + * NodeView per render = O(n^2) per keystroke). Returns undefined if the plugin + * is not installed or the id has no number yet. + */ +export function getFootnoteNumber( + state: EditorState, + id: string, +): number | undefined { + return footnoteNumberingPluginKey.getState(state)?.numbers.get(id); } /** @@ -59,16 +100,19 @@ export function footnoteNumberingPlugin(): Plugin { key: footnoteNumberingPluginKey, state: { init(_, { doc }) { - return buildFootnoteDecorations(doc); + return buildFootnoteNumberingState(doc); }, apply(tr, old) { + // Recompute (and re-cache) only when the document actually changed, so + // the number map NodeViews read stays current on every edit while + // non-doc transactions (selection, etc.) reuse the cache for free. if (!tr.docChanged) return old; - return buildFootnoteDecorations(tr.doc); + return buildFootnoteNumberingState(tr.doc); }, }, props: { decorations(state) { - return this.getState(state); + return footnoteNumberingPluginKey.getState(state)?.decorations; }, }, }); diff --git a/packages/editor-ext/src/lib/footnote/footnote-sync.ts b/packages/editor-ext/src/lib/footnote/footnote-sync.ts index 33258590..505a60d0 100644 --- a/packages/editor-ext/src/lib/footnote/footnote-sync.ts +++ b/packages/editor-ext/src/lib/footnote/footnote-sync.ts @@ -293,107 +293,237 @@ export function footnoteSyncPlugin( const plan = resolveCollisions(info); const referenceIds = plan.referenceIds; - // 1) Desired definitions: one per referenced id, in reference order, - // reusing existing definition nodes (preserving their content) and - // synthesizing empty ones for references that lack a definition. - // Definitions whose id has no matching reference (true orphans) are - // dropped per the existing orphan policy — but a collision is NEVER the - // cause of a drop, because collisions were re-id'd above. - const desiredDefs: ProseMirrorNode[] = referenceIds.map((id) => { - const existing = plan.definitions.get(id); - if (existing) { - // A definition paired to a re-id'd reference keeps its CONTENT but - // must carry the new id. Rewrite the id attr when it differs (cheap - // no-op when it already matches). - if (existing.attrs.id !== id) { - return defType.create({ id }, existing.content); - } - return existing; - } - return defType.create({ id }, paragraphType.create()); - }); + // The set of ids that must have a definition, in reference order (after + // collision re-id). De-duplicated already by resolveCollisions. + const referenceIdSet = new Set(referenceIds); - // 2) Determine whether the document already matches the desired end-state. - const hasRefs = desiredDefs.length > 0; + // 1) For each definition occurrence, compute the id it should END UP with + // (which differs from its current id only when collision resolution + // re-id'd it). plan.definitions maps a FINAL id -> the chosen node, so + // we invert it by node identity to recover each occurrence's target id. + const finalIdByNode = new Map(); + for (const [id, node] of plan.definitions) finalIdByNode.set(node, id); - // Is the existing single list already exactly the desired list, placed - // after all meaningful content (nothing but empty paragraphs after it)? const isEmptyParagraph = (node: ProseMirrorNode) => node.type === paragraphType && node.content.size === 0; - let alreadyCanonical = false; - if (plan.changed) { - // A collision was detected (duplicate ids among refs/defs). The doc must - // be rewritten (re-id'd references + rebuilt list); it is never already - // canonical in this case. - alreadyCanonical = false; - } else if (!hasRefs) { - // Canonical when there is no footnotesList at all. - alreadyCanonical = info.lists.length === 0; - } else if (info.lists.length === 1) { - const { pos, node } = info.lists[0]; - // Same definitions, same order, same identity (no rewrite needed)? - const sameDefs = - node.childCount === desiredDefs.length && - desiredDefs.every((d, i) => node.child(i) === d); + // 2) Classify every existing definition occurrence: + // - reId: keep the node in place, only change its id attr (collision). + // - orphan: delete it (its final id has no matching reference). + // A definition that already carries the right id and is referenced is + // left COMPLETELY untouched (its Yjs subtree is preserved). This is the + // core of the data-loss fix: a pure reference reorder produces NO + // mutation of any definition subtree. + interface DefReid { + pos: number; + node: ProseMirrorNode; + newId: string; + } + const defReids: DefReid[] = []; + const orphanDefs: DefOccurrence[] = []; + // Track which referenced ids already have a surviving (non-orphan) + // definition, so we can synthesize the genuinely missing ones. + const satisfiedIds = new Set(); + // Choose a "primary" list to receive inserts/migrated defs: the LAST list + // whose placement is canonical (only empty paragraphs follow it), else the + // last list, else none. New defs and consolidated defs land here. + for (const occ of info.defOccurrences) { + const finalId = finalIdByNode.get(occ.node) ?? occ.id; + if (!referenceIdSet.has(finalId)) { + orphanDefs.push(occ); + continue; + } + if (occ.id !== finalId) { + defReids.push({ pos: occ.pos, node: occ.node, newId: finalId }); + } + satisfiedIds.add(finalId); + } - // Placement: only empty paragraphs may follow the list. - const listEnd = pos + node.nodeSize; - let onlyEmptyParasAfter = true; + // 3) Referenced ids with no surviving definition need a fresh empty one. + const missingIds = referenceIds.filter((id) => !satisfiedIds.has(id)); + + // 4) Determine list topology. + const hasRefs = referenceIds.length > 0; + + // Pick the primary list: prefer the last canonically-placed list. + const listIsTrailing = (listPos: number, listNode: ProseMirrorNode) => { + const listEnd = listPos + listNode.nodeSize; + let ok = true; doc.nodesBetween(listEnd, doc.content.size, (child, childPos) => { - // Only inspect top-level children that start at/after the list end. - if (childPos >= listEnd && child !== node) { - if (!isEmptyParagraph(child)) onlyEmptyParasAfter = false; + if (childPos >= listEnd && child !== listNode) { + if (!isEmptyParagraph(child)) ok = false; } return false; // do not descend }); - - alreadyCanonical = sameDefs && onlyEmptyParasAfter; + return ok; + }; + let primaryList: { pos: number; node: ProseMirrorNode } | null = null; + for (let i = info.lists.length - 1; i >= 0; i--) { + if (listIsTrailing(info.lists[i].pos, info.lists[i].node)) { + primaryList = info.lists[i]; + break; + } } + if (!primaryList && info.lists.length > 0) { + primaryList = info.lists[info.lists.length - 1]; + } + // Extra lists (everything except the primary) must be consolidated away. + const extraLists = info.lists.filter((l) => l !== primaryList); + const inExtraList = (pos: number) => + extraLists.some((l) => pos > l.pos && pos < l.pos + l.node.nodeSize); - if (alreadyCanonical) return null; + // Definitions inside an extra list are migrated (recreated with the right + // id) into the primary list, so drop their in-place re-id markups — the + // whole extra list is deleted below and the markup would be wasted. + const defReidsToApply = defReids.filter((r) => !inExtraList(r.pos)); - // 3) Rebuild: produce exactly ONE transaction that reaches the end-state. + // 5) Decide whether anything must change. The document is canonical when: + // - no collisions were resolved (refs or defs), AND + // - no orphan definitions, AND + // - no missing definitions, AND + // - exactly the right number of lists (0 when no refs, else 1) AND the + // single list is canonically placed (trailing). + const noChangeNeeded = + !plan.changed && + defReids.length === 0 && + orphanDefs.length === 0 && + missingIds.length === 0 && + extraLists.length === 0 && + (hasRefs + ? info.lists.length === 1 && primaryList !== null + : info.lists.length === 0); + + if (noChangeNeeded) return null; + + // 6) Apply the targeted, minimal mutations in ONE transaction. We never + // delete-and-recreate an unchanged definition subtree; we only: + // (a) re-id specific colliding references and definitions (attr-only), + // (b) delete genuine orphan definitions and extra/empty lists, + // (c) insert genuinely-missing empty definitions and migrate defs out + // of extra lists into the primary list, + // (d) create the primary list if references exist but none does yet. const tr = newState.tr; - // 3a) Re-id colliding body references FIRST. A footnoteReference is an - // inline atom, so setNodeMarkup changes only its attrs (not its size), - // leaving every other position valid for the list deletions/insert - // that follow. + // 6a) Re-id colliding references (inline atoms: attr-only, size-stable). for (const reid of plan.refReids) { - tr.setNodeMarkup(reid.pos, undefined, { + tr.setNodeMarkup(tr.mapping.map(reid.pos), undefined, { + ...reid.node.attrs, + id: reid.newId, + }); + } + // 6b) Re-id colliding definitions IN PLACE (attr-only). This preserves the + // definition's content subtree — never delete+recreate it. + for (const reid of defReidsToApply) { + tr.setNodeMarkup(tr.mapping.map(reid.pos), undefined, { ...reid.node.attrs, id: reid.newId, }); } - // Delete every existing footnotesList (from the end so earlier positions - // stay valid while we mutate). - [...info.lists] - .sort((a, b) => b.pos - a.pos) - .forEach(({ pos, node }) => { - tr.delete(pos, pos + node.nodeSize); + // 6c) Migrate non-orphan definitions out of every extra list into the + // primary list (or, if there is no primary list, into a new one we + // build), then delete the extra (now drained) lists. This is the only + // path that moves a definition subtree, and it runs ONLY in the + // abnormal multi-list case (paste/collab merge) — never on a plain + // reorder, which keeps a single list untouched. + const migrated: ProseMirrorNode[] = []; + for (const extra of extraLists) { + extra.node.forEach((defChild) => { + if (defChild.type !== defType) return; + const finalId = finalIdByNode.get(defChild) ?? defChild.attrs.id; + if (!referenceIdSet.has(finalId)) return; // orphan: drop it + migrated.push( + defChild.attrs.id === finalId + ? defChild + : defType.create({ id: finalId }, defChild.content), + ); + }); + } + + // 6c-bis) The definitions to INSERT into the primary list: migrated defs + // from extra lists + freshly synthesized empty defs for references + // that have no definition at all. Computed before deletions so we can + // decide whether the primary list would be left empty. + const toInsert: ProseMirrorNode[] = [ + ...migrated, + ...missingIds.map((id) => + defType.create({ id }, paragraphType.create()), + ), + ]; + + // Does the primary list keep at least one definition after we strip its + // orphans AND counting the defs we are about to insert? If it ends up + // empty (an empty footnotesList is invalid schema), delete the WHOLE list + // instead of leaving a hollow shell. Only the primary list can receive + // inserts; extra lists are always deleted wholesale. + let primarySurvivors = 0; + if (primaryList) { + primaryList.node.forEach((defChild) => { + if (defChild.type !== defType) return; + const finalId = finalIdByNode.get(defChild) ?? defChild.attrs.id; + if (referenceIdSet.has(finalId)) primarySurvivors += 1; + }); + } + const primaryWillBeEmpty = + !!primaryList && primarySurvivors === 0 && toInsert.length === 0; + + // 6d) Delete orphan definitions, extra lists, and any list that would be + // left empty. Sort deletions from the end so earlier positions stay + // valid; map through tr.mapping to account for the (size-stable) re-id + // markups and earlier deletions. + const deletions: Array<{ from: number; to: number }> = []; + const wholeListDeletes = new Set(extraLists); + if (primaryWillBeEmpty && primaryList) wholeListDeletes.add(primaryList); + + for (const occ of orphanDefs) { + // Skip orphans inside a list that is being deleted wholesale. + const inWholeDeleted = [...wholeListDeletes].some( + (l) => occ.pos > l.pos && occ.pos < l.pos + l.node.nodeSize, + ); + if (inWholeDeleted) continue; + deletions.push({ from: occ.pos, to: occ.pos + occ.node.nodeSize }); + } + for (const l of wholeListDeletes) { + deletions.push({ from: l.pos, to: l.pos + l.node.nodeSize }); + } + deletions + .sort((a, b) => b.from - a.from) + .forEach(({ from, to }) => { + tr.delete(tr.mapping.map(from), tr.mapping.map(to)); }); - if (hasRefs) { - // Insert a single canonical list holding the desired definitions. Place - // it after the last meaningful (non-empty-paragraph) top-level block, so - // it lands before any trailing empty paragraph the trailing-node plugin - // maintains. This keeps both plugins idempotent. - const mappedDoc = tr.doc; - let insertPos = mappedDoc.content.size; - for (let i = mappedDoc.childCount - 1; i >= 0; i--) { - const child = mappedDoc.child(i); - if (isEmptyParagraph(child)) { - // skip trailing empty paragraphs; insert before them - insertPos -= child.nodeSize; - } else { - break; - } - } + // If we deleted the primary list wholesale, it can no longer receive the + // inserts below — null it out so a fresh list is created when needed. + if (primaryWillBeEmpty) primaryList = null; - const merged = listType.create(null, Fragment.fromArray(desiredDefs)); - tr.insert(insertPos, merged); + // 6e) Insert the migrated + synthesized definitions. + if (hasRefs) { + if (primaryList) { + if (toInsert.length > 0) { + // Append at the end of the (mapped) primary list, just before its + // closing token, so its existing definition subtrees are untouched. + // We only changed attrs (size-stable) and deleted OTHER nodes, so + // mapping the original list-end position forward lands at the same + // boundary; -1 puts us just inside the list's closing token. + const insertAt = + tr.mapping.map(primaryList.pos + primaryList.node.nodeSize) - 1; + tr.insert(insertAt, Fragment.fromArray(toInsert)); + } + } else { + // No usable list exists yet but references do — create one holding the + // migrated + synthesized definitions, placed after the last meaningful + // (non-empty-paragraph) top-level block so it sits before any trailing + // empty paragraph the trailing-node plugin maintains. + const mappedDoc = tr.doc; + let insertPos = mappedDoc.content.size; + for (let i = mappedDoc.childCount - 1; i >= 0; i--) { + const child = mappedDoc.child(i); + if (isEmptyParagraph(child)) insertPos -= child.nodeSize; + else break; + } + const list = listType.create(null, Fragment.fromArray(toInsert)); + tr.insert(insertPos, list); + } } if (!tr.docChanged) return null; diff --git a/packages/editor-ext/src/lib/footnote/footnote.test.ts b/packages/editor-ext/src/lib/footnote/footnote.test.ts index 5dfc666c..9ecf9a55 100644 --- a/packages/editor-ext/src/lib/footnote/footnote.test.ts +++ b/packages/editor-ext/src/lib/footnote/footnote.test.ts @@ -6,10 +6,13 @@ import { Text } from "@tiptap/extension-text"; import { Superscript } from "@tiptap/extension-superscript"; import { Plugin, PluginKey } from "@tiptap/pm/state"; import { Node as PMNode } from "@tiptap/pm/model"; +import { EditorState } from "@tiptap/pm/state"; import { FootnoteReference } from "./footnote-reference"; import { FootnotesList } from "./footnotes-list"; import { FootnoteDefinition } from "./footnote-definition"; import { TrailingNode } from "../trailing-node"; +import { footnoteSyncPlugin } from "./footnote-sync"; +import { getFootnoteNumber } from "./footnote-numbering"; import { computeFootnoteNumbers, collectReferenceIds, @@ -688,3 +691,258 @@ describe("footnote sync plugin (no infinite loop — live editor)", () => { editor.destroy(); }); }); + +/** + * Data-loss-window regression guard (Fix 1). A pure reference REORDER must not + * cause the sync plugin to delete-and-recreate any definition subtree — doing so + * (the previous behaviour) would, through Yjs, replace the CRDT subtree of every + * definition and could lose a collaborator's in-flight characters on merge. + * + * Numbering is decoration-only (footnote-numbering.ts derives numbers from + * reference order), so the bottom list's PHYSICAL order need not match reference + * order for the displayed numbers to be correct. We therefore assert: the + * existing definition NODE INSTANCES are preserved (identity-equal) after the + * sync pass, AND the derived numbers follow the new reference order. + */ +describe("footnote sync plugin (no rebuild on reorder — data-loss guard)", () => { + function reorderedDoc() { + // The "out of order" end-state of a reorder: references occur as [b, a] but + // the bottom list still physically holds definitions in [a, b] order. This + // is exactly the situation a reference reorder produces (decoration-only + // numbering keeps the displayed numbers correct without physically moving + // the definition subtrees). The sync plugin must leave the definitions + // ALONE here — no delete/recreate of any definition subtree. + return { + type: "doc", + content: [ + { + type: "paragraph", + content: [ + { type: "text", text: "p" }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "b" } }, + { type: "text", text: "q" }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "a" } }, + ], + }, + { + type: FOOTNOTES_LIST_NAME, + content: [ + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: "a" }, + content: [ + { type: "paragraph", content: [{ type: "text", text: "A" }] }, + ], + }, + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: "b" }, + content: [ + { type: "paragraph", content: [{ type: "text", text: "B" }] }, + ], + }, + ], + }, + ], + }; + } + + function getDefNodesById(doc: PMNode): Map { + const m = new Map(); + doc.descendants((node) => { + if (node.type.name === FOOTNOTE_DEFINITION_NAME) m.set(node.attrs.id, node); + }); + return m; + } + + it("does NOT delete/recreate existing definition subtrees for an out-of-order list (numbers still correct)", () => { + const editor = makeEditor(reorderedDoc()); + + // Capture the exact definition NODE INSTANCES before any sync pass. + const before = getDefNodesById(editor.state.doc); + // Sanity: both carry their content right now. + expect(before.get("a")!.textContent).toBe("A"); + expect(before.get("b")!.textContent).toBe("B"); + + // Trigger a local edit elsewhere in the body so the sync plugin runs. + editor.commands.insertContentAt(1, "z"); + + const doc = editor.state.doc; + + // Reference order is [b, a]; the displayed numbers follow reference order + // (decoration-only numbering): b -> 1, a -> 2 — regardless of physical list + // order. + expect(collectReferenceIds(doc)).toEqual(["b", "a"]); + const numbers = computeFootnoteNumbers(doc); + expect(numbers.get("b")).toBe(1); + expect(numbers.get("a")).toBe(2); + + // CRITICAL regression guard: both definitions still exist and are the SAME + // node instances as before the edit — the plugin did NOT delete/recreate the + // list (which would replace every definition's CRDT subtree and open the + // concurrent-edit data-loss window). Identity equality proves the subtree + // was preserved verbatim. + const after = getDefNodesById(doc); + expect(after.get("a")).toBe(before.get("a")); + expect(after.get("b")).toBe(before.get("b")); + // Content intact, exactly one list, both definitions present. + expect(after.get("a")!.textContent).toBe("A"); + expect(after.get("b")!.textContent).toBe("B"); + expect(countType(doc, FOOTNOTES_LIST_NAME)).toBe(1); + expect(countType(doc, FOOTNOTE_DEFINITION_NAME)).toBe(2); + + editor.destroy(); + }); +}); + +/** + * Sync-plugin guard paths that are awkward to exercise through a live editor: + * the remote-transaction skip and the enableSync:false (read-only) mode. + */ +describe("footnote sync plugin (guards)", () => { + // Build a non-canonical document (an orphan reference with no definition) so a + // sync pass would normally append a transaction. + function nonCanonicalState() { + const schema = getSchema(extensions); + const doc = PMNode.fromJSON(schema, { + type: "doc", + content: [ + { + type: "paragraph", + content: [ + { type: "text", text: "x" }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "orphan" } }, + ], + }, + ], + }); + return EditorState.create({ schema, doc }); + } + + it("isRemoteTransaction => true: appendTransaction returns null (no rebuild on remote txns)", () => { + // The sync plugin must SKIP remote/collab transactions so orphan cleanup and + // structural rewrites only ever run on local edits. + const plugin = footnoteSyncPlugin(() => true); + const state = nonCanonicalState(); + + // Produce a doc-changing transaction (insert a space) and feed it to the + // plugin's appendTransaction exactly as ProseMirror would. + const tr = state.tr.insertText(" ", 1); + const newState = state.apply(tr); + const result = plugin.spec.appendTransaction!( + [tr], + state, + newState, + ); + expect(result).toBeNull(); + }); + + it("isRemoteTransaction => false: appendTransaction DOES rebuild (sanity)", () => { + // Control: with a local (non-remote) transaction the same non-canonical doc + // triggers a sync transaction, proving the null above is the remote guard + // and not a no-op everywhere. + const plugin = footnoteSyncPlugin(() => false); + const state = nonCanonicalState(); + const tr = state.tr.insertText(" ", 1); + const newState = state.apply(tr); + const result = plugin.spec.appendTransaction!([tr], state, newState); + expect(result).not.toBeNull(); + expect(result!.docChanged).toBe(true); + }); + + it("enableSync:false: the plugin never mutates the doc (read-only viewer)", () => { + // Build an editor with sync disabled. An orphan reference (no definition) + // must NOT trigger a definition insertion — the document is left untouched. + const editor = new Editor({ + extensions: [ + Document, + Paragraph, + Text, + FootnoteReference.configure({ enableSync: false }), + FootnotesList, + FootnoteDefinition, + ], + content: { + type: "doc", + content: [ + { + type: "paragraph", + content: [ + { type: "text", text: "x" }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "orphan" } }, + ], + }, + ], + }, + }); + // A local edit that would normally trigger orphan-definition synthesis. + editor.commands.insertContentAt(1, "y"); + + const doc = editor.state.doc; + // No definition (and no list) was ever created — sync is disabled. + expect(countType(doc, FOOTNOTE_DEFINITION_NAME)).toBe(0); + expect(countType(doc, FOOTNOTES_LIST_NAME)).toBe(0); + // Numbering decorations still work: the reference is numbered 1. + expect(getFootnoteNumber(editor.state, "orphan")).toBe(1); + editor.destroy(); + }); +}); + +/** + * Numbering cache (Fix 2). NodeViews must read footnote numbers from the + * numbering plugin's cached map (updated once per doc change) rather than + * recomputing the whole map per render. We assert the cache exists, is correct, + * and stays current across edits. + */ +describe("footnote numbering cache", () => { + it("exposes correct numbers via getFootnoteNumber and updates on edits", () => { + const editor = makeEditor({ + type: "doc", + content: [ + { + type: "paragraph", + content: [ + { type: "text", text: "a" }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "x" } }, + { type: "text", text: "b" }, + { type: FOOTNOTE_REFERENCE_NAME, attrs: { id: "y" } }, + ], + }, + { + type: FOOTNOTES_LIST_NAME, + content: [ + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: "x" }, + content: [{ type: "paragraph" }], + }, + { + type: FOOTNOTE_DEFINITION_NAME, + attrs: { id: "y" }, + content: [{ type: "paragraph" }], + }, + ], + }, + ], + }); + + // The cache mirrors computeFootnoteNumbers — but is read in O(1) per id. + expect(getFootnoteNumber(editor.state, "x")).toBe(1); + expect(getFootnoteNumber(editor.state, "y")).toBe(2); + // The cached map is the SAME values a fresh full computation would yield. + const fresh = computeFootnoteNumbers(editor.state.doc); + expect(getFootnoteNumber(editor.state, "x")).toBe(fresh.get("x")); + expect(getFootnoteNumber(editor.state, "y")).toBe(fresh.get("y")); + + // After inserting a new earlier reference, the cache updates so the numbers + // shift (decoration-only numbering follows reference order). + editor.commands.insertContentAt(1, { + type: FOOTNOTE_REFERENCE_NAME, + attrs: { id: "z" }, + }); + expect(getFootnoteNumber(editor.state, "z")).toBe(1); + expect(getFootnoteNumber(editor.state, "x")).toBe(2); + expect(getFootnoteNumber(editor.state, "y")).toBe(3); + editor.destroy(); + }); +});