Merge pull request 'feat(footnotes): author-inline footnotes + deterministic server canonicalization (#228)' (#232) from feat/228-inline-footnotes into develop

Reviewed-on: #232

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

@@ -0,0 +1,150 @@
+// Importing FileImportTaskService transitively loads import-formatter.ts, which
+// imports the ESM-only @sindresorhus/slugify package (not in jest's transform
+// allowlist). slugify is irrelevant to the path under test, so it is mocked out
+// to keep the module graph loadable under ts-jest (mirrors the import.service spec).
+jest.mock('@sindresorhus/slugify', () => ({
+  __esModule: true,
+  default: (input: string) => String(input),
+}));
+// import-attachment.service.ts (loaded transitively for DI typing) imports the
+// ESM-only `p-limit` / `image-dimensions`; neither is exercised on the path under
+// test, so stub them so the module graph loads under ts-jest.
+jest.mock('p-limit', () => ({
+  __esModule: true,
+  default: () => (fn: any) => fn(),
+}));
+jest.mock('image-dimensions', () => ({
+  __esModule: true,
+  imageDimensionsFromData: () => undefined,
+}));
+
+import { promises as fs } from 'fs';
+import * as os from 'os';
+import * as path from 'path';
+import { FileImportTaskService } from './file-import-task.service';
+import { ImportService } from './import.service';
+
+/**
+ * Binding test for issue #228 / review #5: FileImportTaskService.processGenericImport
+ * is a NON-editor write path (markdownToHtml -> processHTML -> JSON, never runs
+ * footnoteSyncPlugin), so it canonicalizes footnotes before persisting. This pins
+ * that binding — the same one import.service has a spec for — which previously had
+ * NO spec at all.
+ *
+ * The markdown -> HTML -> ProseMirror conversion is REAL (a real ImportService,
+ * its createYdoc stubbed); the filesystem is a real temp dir with one .md file;
+ * the DB transaction is stubbed to capture the persisted page content.
+ */
+
+// Out-of-order references (c, a, b), a REUSED reference ([^a] twice), and an
+// ORPHAN definition ([^z], never referenced).
+const MARKDOWN = [
+  '# Title',
+  '',
+  'Body refs [^c] and [^a] and [^b] and again [^a].',
+  '',
+  '[^a]: note A',
+  '[^b]: note B',
+  '[^c]: note C',
+  '[^z]: orphan note',
+].join('\n');
+
+function footnoteListIds(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);
+}
+
+// A permissive chainable stub for the spaces lookup (selectFrom(...).select(...)
+// .where(...).executeTakeFirst()).
+function chainable(result: any): any {
+  const proxy: any = new Proxy(function () {}, {
+    get: (_t, prop) => {
+      if (prop === 'executeTakeFirst') return async () => result;
+      if (prop === 'execute') return async () => [];
+      return () => proxy;
+    },
+  });
+  return proxy;
+}
+
+describe('FileImportTaskService.processGenericImport — footnote canonicalization (#228)', () => {
+  it('orders footnotes by first reference, dedupes reuse, and drops orphans on zip import', async () => {
+    const extractDir = await fs.mkdtemp(path.join(os.tmpdir(), 'fit-canon-'));
+    await fs.writeFile(path.join(extractDir, 'note.md'), MARKDOWN, 'utf-8');
+
+    // Real ImportService for the html -> JSON conversion; stub the yjs encode.
+    const importService = new ImportService(
+      {} as any,
+      {} as any,
+      {} as any,
+      {} as any,
+    );
+    jest
+      .spyOn(importService as any, 'createYdoc')
+      .mockResolvedValue(Buffer.from([]) as any);
+
+    let captured: any = null;
+    const trx = {
+      insertInto: (table: string) => ({
+        values: (v: any) => {
+          if (table === 'pages') captured = v;
+          return { execute: async () => {} };
+        },
+      }),
+    };
+    const db: any = {
+      selectFrom: () => chainable({ slug: 'space-slug' }),
+      transaction: () => ({ execute: (fn: any) => fn(trx) }),
+    };
+
+    const importAttachmentService = {
+      processAttachments: async ({ html }: any) => html,
+    };
+    const backlinkRepo = { insertBacklink: jest.fn() };
+    const eventEmitter = { emit: jest.fn() };
+    const auditService = { logBatchWithContext: jest.fn() };
+
+    const pageService = { nextPagePosition: async () => 'a0' };
+
+    const service = new FileImportTaskService(
+      {} as any, // storageService
+      importService as any,
+      pageService as any,
+      backlinkRepo as any,
+      db,
+      importAttachmentService as any,
+      eventEmitter as any,
+      auditService as any,
+    );
+
+    const fileTask: any = {
+      id: 'task-1',
+      source: 'generic',
+      spaceId: 'space-1',
+      workspaceId: 'ws-1',
+      creatorId: 'user-1',
+    };
+
+    try {
+      await service.processGenericImport({ extractDir, fileTask });
+
+      expect(captured).toBeTruthy();
+      const content = captured.content;
+      // Reference order is c, a, b (NOT the markdown definition order a, b, c).
+      expect(footnoteListIds(content)).toEqual(['c', 'a', 'b']);
+      // Orphan [^z] dropped; reused [^a] collapses to one definition; one list.
+      expect(footnoteListIds(content)).not.toContain('z');
+      const lists = (content.content ?? []).filter(
+        (n: any) => n.type === 'footnotesList',
+      );
+      expect(lists).toHaveLength(1);
+      expect(footnoteListIds(content).filter((id) => id === 'a')).toHaveLength(1);
+    } finally {
+      await fs.rm(extractDir, { recursive: true, force: true });
+    }
+  });
+});

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

@@ -18,7 +18,7 @@ import { generateSlugId } from '../../../common/helpers';
 import { v7 } from 'uuid';
 import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
 import { FileTask, InsertablePage } from '@docmost/db/types/entity.types';
-import { markdownToHtml } from '@docmost/editor-ext';
+import { markdownToHtml, canonicalizeFootnotes } from '@docmost/editor-ext';
 import { getProsemirrorContent } from '../../../common/helpers/prosemirror/utils';
 import { formatImportHtml } from '../utils/import-formatter';
 import {
@@ -496,9 +496,19 @@ export class FileImportTaskService {
               await this.importService.processHTML(html),
             );
 
-            const { title, prosemirrorJson } =
+            const { title, prosemirrorJson: extractedJson } =
               this.importService.extractTitleAndRemoveHeading(pmState);
 
+            // Canonicalize footnote topology on this non-editor write path
+            // (markdownToHtml/processHTML never runs footnoteSyncPlugin), so a
+            // 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 = {
               id: page.id,
               slugId: page.slugId,

apps/server/src/integrations/import/services/import.service.footnote-canonicalize.spec.ts

@@ -0,0 +1,139 @@
+// Importing ImportService transitively loads import-formatter.ts, which imports
+// the ESM-only @sindresorhus/slugify package (not in jest's transform
+// allowlist). slugify is irrelevant to the path under test, so it is mocked out
+// to keep the module graph loadable under ts-jest.
+jest.mock('@sindresorhus/slugify', () => ({
+  __esModule: true,
+  default: (input: string) => String(input),
+}));
+
+import { ImportService } from './import.service';
+import { canonicalizeFootnotes } from '@docmost/editor-ext';
+
+/**
+ * Integration-ish test for the USER-FACING markdown import path
+ * (`ImportService.importPage`). It exercises the REAL markdown -> HTML -> JSON
+ * conversion and asserts that the stored page content has its footnotes
+ * canonicalized — the gap that issue #228 fixes: the import path builds
+ * ProseMirror JSON directly (never running the editor's footnoteSyncPlugin), so
+ * before this wiring the stored footnotes kept the markdown's physical
+ * definition order (out of order vs. references), retained orphan definitions,
+ * and did not collapse reused references.
+ *
+ * The DB/ydoc side-effects are stubbed: `getNewPagePosition` (DB query) and
+ * `createYdoc` (Yjs encode) are spied, and `pageRepo.insertPage` captures the
+ * persisted `content`. Everything between markdown and persistence is REAL.
+ */
+
+// Out-of-order references (c, a, b), a REUSED reference ([^a] twice -> one
+// footnote), and an ORPHAN definition ([^z], never referenced).
+const MARKDOWN = [
+  '# Title',
+  '',
+  'Body refs [^c] and [^a] and [^b] and again [^a].',
+  '',
+  '[^a]: note A',
+  '[^b]: note B',
+  '[^c]: note C',
+  '[^z]: orphan note',
+].join('\n');
+
+function makeFile(filename: string, contents: string) {
+  return {
+    filename,
+    toBuffer: async () => Buffer.from(contents),
+  } as any;
+}
+
+function makeService() {
+  let captured: any = null;
+  const pageRepo = {
+    insertPage: jest.fn(async (values: any) => {
+      captured = values;
+      return { id: 'page-id', slugId: 'slug-id' };
+    }),
+  };
+  const service = new ImportService(
+    pageRepo as any,
+    {} as any,
+    {} as any,
+    {} as any,
+  );
+  jest.spyOn(service as any, 'getNewPagePosition').mockResolvedValue('a0');
+  jest
+    .spyOn(service as any, 'createYdoc')
+    .mockResolvedValue(Buffer.from([]) as any);
+  return { service, pageRepo, getCaptured: () => captured };
+}
+
+/** List the footnote-definition ids of the (single) footnotesList, in order. */
+function footnoteListIds(content: any): string[] {
+  const list = (content.content ?? []).find(
+    (n: any) => n.type === 'footnotesList',
+  );
+  if (!list) return [];
+  return (list.content ?? [])
+    .filter((n: any) => n.type === 'footnoteDefinition')
+    .map((n: any) => n.attrs?.id);
+}
+
+function definitionText(content: any, id: string): string | undefined {
+  const list = (content.content ?? []).find(
+    (n: any) => n.type === 'footnotesList',
+  );
+  const def = (list?.content ?? []).find(
+    (n: any) => n.type === 'footnoteDefinition' && n.attrs?.id === id,
+  );
+  return def?.content?.[0]?.content?.[0]?.text;
+}
+
+describe('ImportService.importPage — footnote canonicalization (#228)', () => {
+  it('orders footnotes by first reference, dedupes reuse, and drops orphans', async () => {
+    const { service, getCaptured } = makeService();
+
+    await service.importPage(
+      Promise.resolve(makeFile('note.md', MARKDOWN)),
+      'user-id',
+      'space-id',
+      'workspace-id',
+    );
+
+    const content = getCaptured().content;
+    expect(content).toBeTruthy();
+
+    // Reference order is c, a, b (NOT the markdown definition order a, b, c).
+    expect(footnoteListIds(content)).toEqual(['c', 'a', 'b']);
+
+    // Definitions preserved and attached to the right ids.
+    expect(definitionText(content, 'c')).toBe('note C');
+    expect(definitionText(content, 'a')).toBe('note A');
+    expect(definitionText(content, 'b')).toBe('note B');
+
+    // Orphan definition [^z] is dropped.
+    expect(footnoteListIds(content)).not.toContain('z');
+
+    // Reused [^a] yields exactly ONE definition, and exactly one list.
+    const lists = (content.content ?? []).filter(
+      (n: any) => n.type === 'footnotesList',
+    );
+    expect(lists).toHaveLength(1);
+    expect(footnoteListIds(content).filter((id) => id === 'a')).toHaveLength(1);
+  });
+
+  it('is idempotent: canonicalizing the stored output again is a no-op', async () => {
+    const { service, getCaptured } = makeService();
+    await service.importPage(
+      Promise.resolve(makeFile('note.md', MARKDOWN)),
+      'user-id',
+      'space-id',
+      'workspace-id',
+    );
+    const stored = getCaptured().content;
+
+    // The stored content is already canonical; running the canonicalizer a second
+    // time must not change it (safe to wire into every write path).
+    const second = canonicalizeFootnotes(stored);
+    expect(second).toEqual(stored);
+    expect(footnoteListIds(second)).toEqual(['c', 'a', 'b']);
+  });
+});

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

@@ -17,7 +17,7 @@ import {
 import { generateJitteredKeyBetween } from 'fractional-indexing-jittered';
 import { TiptapTransformer } from '@hocuspocus/transformer';
 import * as Y from 'yjs';
-import { markdownToHtml } from '@docmost/editor-ext';
+import { markdownToHtml, canonicalizeFootnotes } from '@docmost/editor-ext';
 import {
   FileTaskStatus,
   FileTaskType,
@@ -85,7 +85,17 @@ export class ImportService {
 
     const extracted = this.extractTitleAndRemoveHeading(prosemirrorState);
     const title = extracted.title;
-    const prosemirrorJson = extracted.prosemirrorJson;
+    // Imported markdown/HTML is built via markdownToHtml -> htmlToJson, which
+    // never runs the editor's footnoteSyncPlugin, so the footnote topology keeps
+    // the source's PHYSICAL definition order (out of order vs. references),
+    // 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;