fix(footnotes): address PR #232 review — fragment-safe canonicalization, plugin placement parity, dead-code removal (#228)

Must-fix:
- Move canonicalizeFootnotes OUT of parseProsemirrorContent. It now runs only
  on FULL writes (createPage, updatePageContent operation==='replace'), never on
  an append/prepend fragment (a fragment would lose definition-only footnotes or
  synthesize a bogus empty list). Add a server binding spec.
- Match the live plugin's list PLACEMENT: a single already-canonical
  footnotesList is left exactly where it sits (the plugin never repositions a
  sole correct list), so the first write no longer reorders content that follows
  the list. Applied to BOTH the editor-ext copy and the MCP mirror; pinned by a
  shared golden corpus case with content after the list.
- Fix MCP tool count 38 -> 39 (README x3, AGENTS.md) and the transformJs param
  help (add canonicalizeFootnotes/insertInlineFootnote).

Simplifications:
- Remove the dead duplicate re-id mechanism (deriveFootnoteId/suffix/occurrence)
  from the PURE canonicalizer in both copies — references are never renamed, so
  the derived ids were never requested; first-wins-drop is the real behaviour.
  This also makes the editor-ext footnote-util note about "no cross-package copy"
  true again.
- Remove the sentinel round-trip in insertInlineFootnote: a generalized
  insertNodesAfterAnchor core inserts the footnoteReference node directly.
- Drop the redundant per-definition deep clone in step 4 (shallow id-normalizing
  copy; out is already deep-cloned).

Docs / architecture:
- Correct the editor-ext copy's "It exists because…" header to its real
  consumers (server import, page.service create/update, client paste).
- Note markdownToProseMirror reuse for create/update comment in collaboration.ts.
- A: shared golden JSON corpus exercised by BOTH the editor-ext copy and the MCP
  mirror (footnote-corpus.ts / .mjs) so "the two copies behave identically" is
  checkable.
- C: split the MCP canonicalizer into a pure mirror + footnote-authoring.ts.
- B: import services persist via a different path, so left one-line consolidation
  comments at the call sites rather than folding (does not fall out cleanly).

Tests: insertFootnote wrapper guards + docmost_transform dryRun auto-canonicalize
(MCP mock), page.service create/update + append/prepend binding (server jest),
shared corpus incl. nested-container reference.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

apps/server/src/core/page/services/page.service.footnote-canonicalize.spec.ts

@@ -0,0 +1,153 @@
+// Binding test for issue #228 must-fix #1 / test-coverage #12: footnote
+// canonicalization moved OUT of parseProsemirrorContent and is now applied only
+// on FULL-document writes (createPage, and updatePageContent with operation
+// 'replace'), NEVER on an append/prepend FRAGMENT.
+//
+// The Yjs encode / plain-text extract are stubbed (partial module mock keeps the
+// REAL canonicalizeFootnotes) and parseProsemirrorContent is spied to return the
+// raw fixture, so the test isolates the canonicalize BINDING from schema/Yjs.
+jest.mock('@docmost/editor-ext', () => {
+  const actual = jest.requireActual('@docmost/editor-ext');
+  return {
+    ...actual,
+    createYdocFromJson: jest.fn(() => Buffer.from([])),
+    jsonToText: jest.fn(() => ''),
+  };
+});
+
+import { PageService } from './page.service';
+
+const refNode = (id: string) => ({ type: 'footnoteReference', attrs: { id } });
+const defNode = (id: string, text: string) => ({
+  type: 'footnoteDefinition',
+  attrs: { id },
+  content: [{ type: 'paragraph', content: [{ type: 'text', text }] }],
+});
+const doc = (...content: any[]) => ({ type: 'doc', content });
+
+/** A full doc whose footnote definitions are OUT of reference order (b,a refs;
+ *  a,b defs) — canonicalization must reorder the definitions to [b, a]. */
+const outOfOrderFull = () =>
+  doc(
+    { type: 'paragraph', content: [{ type: 'text', text: 'x' }, refNode('b'), refNode('a')] },
+    { type: 'footnotesList', content: [defNode('a', 'A'), defNode('b', 'B')] },
+  );
+
+/** A definition-ONLY fragment (no references): canonicalizing it would drop the
+ *  whole footnotesList (referenceIds is empty) — i.e. LOSE the footnote. */
+const defOnlyFragment = () =>
+  doc({ type: 'footnotesList', content: [defNode('a', 'appended note')] });
+
+/** A reference-only fragment that REUSES an id defined elsewhere in the live
+ *  doc: canonicalizing it would synthesize a bogus empty footnotesList/def. */
+const refReuseFragment = () =>
+  doc({ type: 'paragraph', content: [{ type: 'text', text: 'more' }, refNode('a')] });
+
+function listDefIds(content: any): string[] {
+  const list = (content.content ?? []).find((n: any) => n.type === 'footnotesList');
+  return (list?.content ?? [])
+    .filter((n: any) => n.type === 'footnoteDefinition')
+    .map((n: any) => n.attrs?.id);
+}
+function hasFootnotesList(content: any): boolean {
+  return (content.content ?? []).some((n: any) => n.type === 'footnotesList');
+}
+
+describe('PageService footnote canonicalization binding (#228)', () => {
+  function makeService() {
+    let insertedContent: any = null;
+    let yjsPayload: any = null;
+
+    const pageRepo = {
+      insertPage: jest.fn(async (values: any) => {
+        insertedContent = values.content;
+        return { id: 'page-id', slugId: 'slug-id' };
+      }),
+    };
+    const generalQueue = { add: jest.fn().mockReturnValue({ catch: jest.fn() }) };
+    const collaborationGateway = {
+      handleYjsEvent: jest.fn(async (_evt: string, _name: string, payload: any) => {
+        yjsPayload = payload;
+      }),
+    };
+
+    const service = new PageService(
+      pageRepo as any,
+      {} as any, // pagePermissionRepo
+      {} as any, // attachmentRepo
+      {} as any, // db
+      {} as any, // storageService
+      {} as any, // attachmentQueue
+      {} as any, // aiQueue
+      generalQueue as any,
+      {} as any, // eventEmitter
+      collaborationGateway as any,
+      {} as any, // watcherService
+      {} as any, // transclusionService
+    );
+    // Isolate the canonicalize BINDING: return the raw fixture (a deep clone so
+    // canonicalize never mutates the caller's object) instead of running the
+    // real markdown/HTML/JSON parse + schema validation.
+    jest
+      .spyOn(service as any, 'parseProsemirrorContent')
+      .mockImplementation(async (content: any) => structuredClone(content));
+    jest.spyOn(service as any, 'nextPagePosition').mockResolvedValue('a0');
+
+    return { service, getInsertedContent: () => insertedContent, getYjsPayload: () => yjsPayload };
+  }
+
+  it('createPage (full write) canonicalizes footnotes into reference order', async () => {
+    const { service, getInsertedContent } = makeService();
+    await service.create('user-id', 'workspace-id', {
+      spaceId: 'space-id',
+      content: outOfOrderFull(),
+      format: 'json',
+    } as any);
+    // Definitions reordered to reference order [b, a].
+    expect(listDefIds(getInsertedContent())).toEqual(['b', 'a']);
+  });
+
+  it("updatePageContent operation 'replace' canonicalizes footnotes", async () => {
+    const { service, getYjsPayload } = makeService();
+    await service.updatePageContent(
+      'page-id',
+      outOfOrderFull(),
+      'replace' as any,
+      'json' as any,
+      { id: 'user-id' } as any,
+    );
+    expect(getYjsPayload().operation).toBe('replace');
+    expect(listDefIds(getYjsPayload().prosemirrorJson)).toEqual(['b', 'a']);
+  });
+
+  it("append of a definition-only fragment is NOT canonicalized (footnote preserved, not dropped)", async () => {
+    const { service, getYjsPayload } = makeService();
+    await service.updatePageContent(
+      'page-id',
+      defOnlyFragment(),
+      'append' as any,
+      'json' as any,
+      { id: 'user-id' } as any,
+    );
+    // Canonicalizing a reference-less fragment would DROP the whole list; the
+    // fragment must pass through untouched so the merge keeps the definition.
+    expect(getYjsPayload().operation).toBe('append');
+    expect(hasFootnotesList(getYjsPayload().prosemirrorJson)).toBe(true);
+    expect(listDefIds(getYjsPayload().prosemirrorJson)).toEqual(['a']);
+  });
+
+  it('prepend of a reference-reuse fragment is NOT canonicalized (no synthesized garbage list)', async () => {
+    const { service, getYjsPayload } = makeService();
+    await service.updatePageContent(
+      'page-id',
+      refReuseFragment(),
+      'prepend' as any,
+      'json' as any,
+      { id: 'user-id' } as any,
+    );
+    // Canonicalizing would synthesize a bogus empty footnotesList for the reused
+    // reference; the fragment must pass through with no list at all.
+    expect(getYjsPayload().operation).toBe('prepend');
+    expect(hasFootnotesList(getYjsPayload().prosemirrorJson)).toBe(false);
+  });
+});

apps/server/src/core/page/services/page.service.ts

@@ -160,9 +160,14 @@ export class PageService {
     let ydoc = undefined;
 
     if (createPageDto?.content && createPageDto?.format) {
-      const prosemirrorJson = await this.parseProsemirrorContent(
-        createPageDto.content,
-        createPageDto.format,
+      // createPage always writes a FULL document, so canonicalize footnotes to
+      // the editor's invariant before persisting (issue #228). Pure + idempotent
+      // + shape-safe: a doc with no footnotes is returned unchanged.
+      const prosemirrorJson = canonicalizeFootnotes(
+        await this.parseProsemirrorContent(
+          createPageDto.content,
+          createPageDto.format,
+        ),
       );
 
       content = prosemirrorJson;
@@ -343,7 +348,17 @@ export class PageService {
     format: ContentFormat,
     user: User,
   ): Promise<void> {
-    const prosemirrorJson = await this.parseProsemirrorContent(content, format);
+    let prosemirrorJson = await this.parseProsemirrorContent(content, format);
+
+    // Canonicalize footnotes ONLY for a full-document write ('replace'). For an
+    // append/prepend FRAGMENT, canonicalizing is semantically wrong (it would
+    // drop a definition-only fragment's list, or synthesize a duplicate empty
+    // definition for a fragment reusing an existing id) — the fragment merges
+    // into the live doc where the editor's footnoteSyncPlugin keeps the invariant
+    // (issue #228, must-fix #1).
+    if (operation === 'replace') {
+      prosemirrorJson = canonicalizeFootnotes(prosemirrorJson);
+    }
 
     const documentName = `page.${pageId}`;
     await this.collaborationGateway.handleYjsEvent(
@@ -1301,14 +1316,18 @@ export class PageService {
       }
     }
 
-    // markdown/html are converted via markdownToHtml -> htmlToJson and json may
-    // be written programmatically (API createPage/updatePageContent) — none of
-    // these run the editor's footnoteSyncPlugin, so footnotes keep the source's
-    // physical order, orphans survive, and reused references aren't collapsed.
-    // Canonicalize to the editor's invariant before persisting (issue #228).
-    // Pure + idempotent + shape-safe: a doc with no footnotes is unchanged.
-    prosemirrorJson = canonicalizeFootnotes(prosemirrorJson);
-
+    // NOTE: footnote canonicalization is intentionally NOT done here. This
+    // method serves BOTH full writes (createPage / updatePageContent with
+    // operation 'replace') AND fragment writes (append / prepend). Canonicalizing
+    // a FRAGMENT is semantically wrong — e.g. a definition-only fragment has no
+    // references, so the canonicalizer would drop its whole footnotesList (lost
+    // footnotes), and a fragment reusing an existing id would synthesize an empty
+    // duplicate definition. The canonicalizer therefore runs only at the
+    // FULL-DOCUMENT callers (createPage, and updatePageContent for 'replace'),
+    // never on a fragment (issue #228, must-fix #1).
+    // (Future consolidation, architecture B: the import services persist via a
+    // different path; folding all of these into one "prepare JSON for persist"
+    // helper would centralize the canonicalize call — left as follow-up.)
     try {
       jsonToNode(prosemirrorJson);
     } catch (err) {

apps/server/src/integrations/import/services/file-import-task.service.ts

@@ -504,6 +504,9 @@ export class FileImportTaskService {
             // zip-imported page's footnotes are reference-ordered, deduped, and
             // orphan-free like the editor's invariant (issue #228). Pure +
             // idempotent + shape-safe; a footnote-free doc is unchanged.
+            // (Future consolidation, architecture B: like import.service, this
+            // path persists directly rather than via PageService — a shared
+            // "prepare JSON for persist" helper would centralize this call.)
             const prosemirrorJson = canonicalizeFootnotes(extractedJson);
 
             const insertablePage: InsertablePage = {

apps/server/src/integrations/import/services/import.service.ts

@@ -91,6 +91,10 @@ export class ImportService {
     // retains orphan definitions, and is not deduped. Canonicalize before
     // persisting so the stored page matches the editor's invariant (issue #228).
     // Pure + idempotent + shape-safe: a doc with no footnotes is unchanged.
+    // (Future consolidation, architecture B: this import path persists directly
+    // via pageRepo.insertPage rather than through PageService.createPage, so the
+    // canonicalize call lives here; folding both into one "prepare JSON for
+    // persist" helper is a sensible follow-up.)
     const prosemirrorJson = canonicalizeFootnotes(extracted.prosemirrorJson);
 
     const pageTitle = title || fileName;