feat(tags): wire re-anchoring into the page mutation transaction

apps/web/src/services/api/__tests__/page-mutation-reanchor.test.ts

@@ -0,0 +1,179 @@
+/**
+ * The content-tag re-anchoring hook inside `applyPageMutation`.
+ *
+ * WHY THIS TEST EXISTS. Forward-porting is the PRIMARY anchoring mechanism, and
+ * `applyPageMutation` is its only possible call site: `previousContent` and
+ * `nextContent` exist together in exactly one transaction in the codebase. Every
+ * way this wiring can break is silent —
+ *
+ *   - the call removed        -> every edit degrades to quote repair, the
+ *                                accuracy floor, and nothing fails
+ *   - the wrong executor      -> anchors commit independently of the content
+ *                                they describe (both reviewers of #2494 raised
+ *                                this one)
+ *   - the wrong content modes -> convert-content-mode projects the old revision
+ *                                with the new mode and orphans every anchor
+ *
+ * None of those makes a test go red on their own, so they are asserted here
+ * directly. The porting BEHAVIOUR is covered against real Postgres by
+ * packages/lib/src/tags/__tests__/tag-service.integration.test.ts; this file
+ * only pins the seam.
+ */
+import { describe, it, expect, vi, beforeEach } from 'vitest';
+
+const mockReanchor = vi.fn(async () => ({ ok: true as const, data: { considered: 0, updated: 0, orphaned: 0, skippedStaleHash: 0, skippedFormatFlip: 0 } }));
+const mockSyncMentions = vi.fn(async () => undefined);
+const mockLogError = vi.fn();
+
+/**
+ * The row `applyPageMutation` reads before it writes.
+ *
+ * HTML on purpose. The conversion case below converts to markdown, so the old
+ * and new modes DIFFER — without that the two are identical and swapping one
+ * for the other is invisible, which is exactly how the first version of this
+ * test passed while the mutation that swaps them survived.
+ */
+const currentPage = {
+  id: 'page-1',
+  driveId: 'drive-1',
+  revision: 1,
+  type: 'DOCUMENT',
+  content: 'the original content',
+  contentMode: 'html',
+  title: 'Doc',
+};
+
+function queryBuilder(rows: unknown[]) {
+  const chain: Record<string, unknown> = {};
+  for (const method of ['select', 'from', 'where', 'limit', 'update', 'set', 'returning', 'insert', 'values']) {
+    chain[method] = vi.fn(() => chain);
+  }
+  chain.limit = vi.fn(async () => rows);
+  chain.returning = vi.fn(async () => rows);
+  return chain;
+}
+
+/** The nested executor the sweep receives — a SAVEPOINT, not the outer tx. */
+const savepoint = { __savepoint: true } as unknown as Record<string, unknown>;
+
+/**
+ * The transaction handed to the mutation body.
+ *
+ * A full query builder, because the mutation writes through it — but ONE stable
+ * object, so the assertion that the sweep received this exact executor is an
+ * identity check rather than a shape check. A shape check would pass for the db
+ * singleton too, which is the bug being guarded against.
+ */
+const transaction = queryBuilder([currentPage]) as Record<string, unknown> & {
+  transaction: ReturnType<typeof vi.fn>;
+};
+/** Records whether the savepoint body threw, i.e. whether it would have rolled back. */
+const savepointRolledBack = { value: false };
+transaction.transaction = vi.fn(async (fn: (sp: unknown) => Promise<unknown>) => {
+  savepointRolledBack.value = false;
+  try {
+    return await fn(savepoint);
+  } catch (error) {
+    // A real SAVEPOINT unwinds here, clearing the aborted transaction state.
+    savepointRolledBack.value = true;
+    throw error;
+  }
+});
+
+vi.mock('@pagespace/db/db', () => ({
+  db: {
+    ...queryBuilder([currentPage]),
+    transaction: vi.fn(async (fn: (tx: unknown) => Promise<unknown>) => fn(transaction)),
+  },
+}));
+vi.mock('@pagespace/db/operators', () => ({ eq: vi.fn(), and: vi.fn() }));
+vi.mock('@pagespace/db/schema/core', () => ({ pages: { id: 'id', revision: 'revision' } }));
+vi.mock('@pagespace/lib/tags/tag-service', () => ({ reanchorPageTags: (...a: unknown[]) => mockReanchor(...(a as [])) }));
+vi.mock('@/services/api/page-mention-service', () => ({ syncMentions: (...a: unknown[]) => mockSyncMentions(...(a as [])) }));
+vi.mock('@pagespace/lib/logging/logger-config', () => ({ loggers: { api: { error: (...a: unknown[]) => mockLogError(...(a as [])), warn: vi.fn(), info: vi.fn() } } }));
+vi.mock('@pagespace/lib/monitoring/activity-logger', () => ({ logActivityWithTx: vi.fn(async () => undefined) }));
+vi.mock('@pagespace/lib/monitoring/change-group', () => ({ inferChangeGroupType: vi.fn(() => 'edit'), createChangeGroupId: vi.fn(() => 'cg-1') }));
+vi.mock('@pagespace/lib/services/page-version-service', () => ({ computePageStateHash: vi.fn(() => 'hash'), createPageVersion: vi.fn(async () => undefined) }));
+vi.mock('@pagespace/lib/services/page-content-store', () => ({ writePageContent: vi.fn(async () => ({ ref: 'stored-ref' })) }));
+vi.mock('@pagespace/lib/content/page-content-format', () => ({ detectPageContentFormat: vi.fn(() => 'markdown') }));
+vi.mock('@pagespace/lib/utils/hash-utils', () => ({ hashWithPrefix: vi.fn(() => 'ref') }));
+vi.mock('@pagespace/lib/sheets/sheet', () => ({ isSheetType: vi.fn(() => false) }));
+vi.mock('@pagespace/lib/sheets/store', () => ({ replaceFromDocument: vi.fn(), readSheetDocument: vi.fn(async () => null) }));
+vi.mock('@pagespace/lib/utils/enums', () => ({ PageType: { DOCUMENT: 'DOCUMENT' } }));
+vi.mock('@pagespace/lib/notifications/notifications', () => ({ createMentionNotification: vi.fn(async () => undefined) }));
+
+const { applyPageMutation } = await import('../page-mutation-service');
+
+const baseInput = {
+  pageId: 'page-1',
+  operation: 'update' as const,
+  updatedFields: ['content'],
+  expectedRevision: 1,
+  context: { userId: 'user-1' },
+  source: 'user' as const,
+};
+
+describe('applyPageMutation re-anchors content tags', () => {
+  beforeEach(() => {
+    mockReanchor.mockClear();
+    mockLogError.mockClear();
+    // `transaction.transaction` is a module-level spy, so its call count
+    // ACCUMULATES across cases. Without this clear, the savepoint assertion
+    // below ("called exactly once") holds only while its test happens to run
+    // first — a case added above it, or randomised order, would fail it for a
+    // reason that has nothing to do with the savepoint.
+    transaction.transaction.mockClear();
+    savepointRolledBack.value = false;
+  });
+
+  it('sweeps with BOTH revisions and the caller transaction', async () => {
+    await applyPageMutation({ ...baseInput, updates: { content: 'the edited content' } } as never);
+
+    expect(mockReanchor).toHaveBeenCalledTimes(1);
+    const [pageId, oldContent, newContent, options] = mockReanchor.mock.calls[0] as unknown as [string, string, string, Record<string, unknown>];
+    expect(pageId).toBe('page-1');
+    expect(oldContent).toBe('the original content');
+    expect(newContent).toBe('the edited content');
+    // The sweep runs on the SAVEPOINT, not the outer transaction and not the db
+    // singleton. A savepoint is still inside the caller's transaction — so the
+    // anchors commit with the content — but a failure inside it can be rolled
+    // back without poisoning the outer one.
+    expect(options.executor).toBe(savepoint);
+    expect(transaction.transaction).toHaveBeenCalledTimes(1);
+  });
+
+  it('passes the PRE-update mode as the old one across a conversion', async () => {
+    // The pages row is already updated when the hook runs, so a service reading
+    // the stored mode would see the new one for both revisions — which projects
+    // the old HTML as raw text and orphans every anchor.
+    await applyPageMutation({
+      ...baseInput,
+      updatedFields: ['content', 'contentMode'],
+      updates: { content: '# converted', contentMode: 'markdown' },
+    } as never);
+
+    const [, , , options] = mockReanchor.mock.calls[0] as unknown as [string, string, string, Record<string, unknown>];
+    expect(options.oldContentMode, 'the mode the old revision was written in').toBe('html');
+    expect(options.newContentMode, 'the mode the page now has').toBe('markdown');
+  });
+
+  it('does not sweep when the content did not change', async () => {
+    await applyPageMutation({ ...baseInput, updatedFields: ['title'], updates: { title: 'Renamed' } } as never);
+    expect(mockReanchor).not.toHaveBeenCalled();
+  });
+
+  it('rolls the savepoint back on a failed sweep and still completes the save', async () => {
+    // Catching the exception is NOT enough on its own: Postgres marks the whole
+    // transaction aborted on any statement error, so a failed UPDATE inside the
+    // sweep would make every LATER statement fail and roll back the page edit.
+    // The savepoint has to unwind for the outer transaction to stay usable.
+    mockReanchor.mockResolvedValueOnce({ ok: false as const, error: 'internal_error' } as never);
+
+    await expect(
+      applyPageMutation({ ...baseInput, updates: { content: 'still saves' } } as never),
+    ).resolves.toBeDefined();
+
+    expect(savepointRolledBack.value, 'the savepoint must unwind so the outer tx survives').toBe(true);
+    expect(mockLogError).toHaveBeenCalled();
+  });
+});

apps/web/src/services/api/page-mutation-service.ts

@@ -6,6 +6,7 @@ import { inferChangeGroupType, createChangeGroupId } from '@pagespace/lib/monito
 import { computePageStateHash, createPageVersion, type PageVersionSource } from '@pagespace/lib/services/page-version-service'
 import { loggers } from '@pagespace/lib/logging/logger-config';
 import { writePageContent } from '@pagespace/lib/services/page-content-store';
+import { reanchorPageTags } from '@pagespace/lib/tags/tag-service';
 import { detectPageContentFormat, type PageContentFormat } from '@pagespace/lib/content/page-content-format';
 import { hashWithPrefix } from '@pagespace/lib/utils/hash-utils';
 import { isSheetType } from '@pagespace/lib/sheets/sheet';
@@ -297,6 +298,70 @@ export async function applyPageMutation({
         mentionedByUserId: context.userId,
         driveId: currentPage.driveId,
       });
+
+      // FORWARD-PORT CONTENT TAG ANCHORS. This is the primary anchoring
+      // mechanism and this is its only call site: `previousContent` and
+      // `nextContent` exist together in exactly one place in the codebase, and
+      // that place is this transaction. Without the hook every edit falls
+      // through to quote repair, which is the accuracy floor rather than the
+      // target — tags on actively-edited pages would decay while tags on stale
+      // pages kept working, which is backwards for retrieval.
+      //
+      // IN the transaction, on `transaction`: the sweep must commit with the
+      // content it describes. Ported outside it, anchors can point at a
+      // revision that then rolls back, or the page can commit with only some
+      // of its anchors moved.
+      //
+      // BOTH MODES ARE PASSED EXPLICITLY, always. The `pages` row has already
+      // been updated by this point, so letting the service read the stored mode
+      // would give it the NEW mode for BOTH revisions — which is precisely the
+      // broken case for `convert-content-mode`: the old HTML then projects as
+      // raw text and every correctly built anchor fails its hash check.
+      //
+      // `currentPage` is a full row read before the update, so it still holds
+      // the old mode. For an ordinary edit the two are equal and the service
+      // treats it as a normal transition; when they differ it knows the change
+      // is a DECLARED conversion rather than the accidental format flip its
+      // guard exists to catch, and routes the anchors to quote repair.
+      const previousContentMode = currentPage.contentMode;
+      const nextContentMode =
+        typeof updates.contentMode === 'string' ? updates.contentMode : previousContentMode;
+
+      // A SWEEP FAILURE MUST NOT FAIL THE SAVE — and catching the exception is
+      // NOT enough to achieve that. Postgres marks the ENTIRE transaction
+      // aborted on any statement error, so a failed UPDATE inside the sweep
+      // poisons this transaction even though `reanchorPageTags` swallows the
+      // error and returns a result. Every statement after it — createPageVersion,
+      // the activity log — would then fail with "current transaction is aborted"
+      // and roll back the page edit, which is the opposite of best-effort.
+      //
+      // The sweep therefore runs in a SAVEPOINT (drizzle's nested transaction).
+      // On success it is released and the ported anchors commit with the
+      // content. On failure it is rolled back, which clears the aborted state
+      // and leaves the outer transaction healthy to finish the save. Anchors are
+      // recoverable by a later repair pass; a refused page save is not.
+      //
+      // The throw is what triggers the rollback: `reanchorPageTags` returns a
+      // result rather than throwing, so a non-ok result has to be converted into
+      // one for the savepoint to unwind.
+      try {
+        await transaction.transaction(async (savepoint) => {
+          const reanchored = await reanchorPageTags(pageId, previousContent, nextContent, {
+            executor: savepoint,
+            oldContentMode: previousContentMode,
+            newContentMode: nextContentMode,
+          });
+          if (!reanchored.ok) {
+            throw new Error(`reanchorPageTags returned ${reanchored.error}`);
+          }
+        });
+      } catch (error) {
+        loggers.api.error(
+          'Failed to re-anchor content tags after a page mutation; the save continues',
+          error as Error,
+          { pageId },
+        );
+      }
     }
 
     // Create page version BEFORE acquiring the activity chain lock,

packages/lib/src/content/anchoring/anchor.ts

@@ -19,9 +19,24 @@ import type { TextAnchor } from './types';
 /** How much surrounding text a TextAnchor carries on each side. ~64 bytes total. */
 export const ANCHOR_CONTEXT_LENGTH = 32;
 
-const FNV_OFFSET_BASIS = 0xcbf29ce484222325n;
-const FNV_PRIME = 0x100000001b3n;
-const FNV_MASK = 0xffffffffffffffffn;
+/*
+ * BigInt CALLS, not BigInt literals.
+ *
+ * `0xcbf29ce484222325n` requires an ES2020 target. This module is pure and was
+ * only ever imported by other lib code until `tag-service` reached it, and
+ * `apps/web` compiles at ES2018 — so the moment anything in the web app pulled
+ * this file in transitively, `web#build` failed with "BigInt literals are not
+ * available when targeting lower than ES2020". The literal was latent, not
+ * safe.
+ *
+ * `BigInt('0x…')` is a runtime call rather than literal syntax, compiles at
+ * ES2018, and produces the identical value — so `hashText` output is unchanged
+ * and every anchor already stored still verifies. Raising the web app's target
+ * would also work, but that is a build-wide change to fix one constant here.
+ */
+const FNV_OFFSET_BASIS = BigInt('0xcbf29ce484222325');
+const FNV_PRIME = BigInt('0x100000001b3');
+const FNV_MASK = BigInt('0xffffffffffffffff');
 
 /**
  * FNV-1a over the UTF-16 code units of `text`, as 16 lowercase hex chars.