diff --git a/apps/web/src/services/api/__tests__/page-mutation-reanchor.test.ts b/apps/web/src/services/api/__tests__/page-mutation-reanchor.test.ts new file mode 100644 index 0000000000..4e6fe7a853 --- /dev/null +++ b/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 = {}; + 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; + +/** + * 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 & { + transaction: ReturnType; +}; +/** 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) => { + 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) => 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]; + 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]; + 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(); + }); +}); diff --git a/apps/web/src/services/api/page-mutation-service.ts b/apps/web/src/services/api/page-mutation-service.ts index 7a417ac31a..fa9e5548ff 100644 --- a/apps/web/src/services/api/page-mutation-service.ts +++ b/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, diff --git a/packages/lib/src/content/anchoring/anchor.ts b/packages/lib/src/content/anchoring/anchor.ts index af579e71c7..d24006b174 100644 --- a/packages/lib/src/content/anchoring/anchor.ts +++ b/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.