diff --git a/CHANGELOG.md b/CHANGELOG.md index e16d47cb..e77b27d5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,14 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Added +- **Save intentional page versions.** Press `Cmd/Ctrl+S` (or use the page menu) + to save a named version of a page. The history panel now distinguishes + intentional versions (a "Saved" / "Agent version" badge) from automatic + snapshots, dims autosaves, and offers an "Only versions" filter. Automatic + snapshots switched from a fixed interval to a trailing idle-flush with a + max-wait ceiling, and a boundary snapshot is pinned whenever the editing source + changes (e.g. a person's edits followed by the AI agent). (#370) + - **Place several images side by side in a row.** A new "Inline (side by side)" alignment mode in the image bubble menu renders consecutive inline images as a row that wraps onto the next line on narrow screens. The row is diff --git a/apps/client/src/features/page-history/components/history-item.tsx b/apps/client/src/features/page-history/components/history-item.tsx index 6953a161..aa76b4f2 100644 --- a/apps/client/src/features/page-history/components/history-item.tsx +++ b/apps/client/src/features/page-history/components/history-item.tsx @@ -40,16 +40,17 @@ export function historyKindMeta(kind?: string | null): HistoryKindMeta { interface HistoryItemProps { historyItem: IPageHistory; - index: number; - onSelect: (id: string, index: number) => void; - onHover?: (id: string, index: number) => void; + // The previous snapshot for diff/restore is resolved by id from the FULL list + // in the parent (resolvePrevSnapshotId), so the item only needs to report its + // own id — never a list index (which would be the filtered-view index). + onSelect: (id: string) => void; + onHover?: (id: string) => void; onHoverEnd?: () => void; isActive: boolean; } const HistoryItem = memo(function HistoryItem({ historyItem, - index, onSelect, onHover, onHoverEnd, @@ -60,12 +61,12 @@ const HistoryItem = memo(function HistoryItem({ const kindMeta = historyKindMeta(historyItem.kind); const handleClick = useCallback(() => { - onSelect(historyItem.id, index); - }, [onSelect, historyItem.id, index]); + onSelect(historyItem.id); + }, [onSelect, historyItem.id]); const handleMouseEnter = useCallback(() => { - onHover?.(historyItem.id, index); - }, [onHover, historyItem.id, index]); + onHover?.(historyItem.id); + }, [onHover, historyItem.id]); const contributors = historyItem.contributors; const hasContributors = contributors && contributors.length > 0; diff --git a/apps/client/src/features/page-history/components/history-list.tsx b/apps/client/src/features/page-history/components/history-list.tsx index 78598fb1..b3400d5f 100644 --- a/apps/client/src/features/page-history/components/history-list.tsx +++ b/apps/client/src/features/page-history/components/history-list.tsx @@ -22,6 +22,7 @@ import { } from "@mantine/core"; import { useTranslation } from "react-i18next"; import { useHistoryRestore } from "@/features/page-history/hooks"; +import { resolvePrevSnapshotId } from "@/features/page-history/utils/resolve-prev-snapshot"; const PREFETCH_DELAY_MS = 150; @@ -58,11 +59,6 @@ function HistoryList({ pageId }: Props) { (kind?: string | null) => kind === "manual" || kind === "agent", [], ); - const originalIndexById = useMemo(() => { - const map = new Map(); - historyItems.forEach((item, index) => map.set(item.id, index)); - return map; - }, [historyItems]); const visibleItems = useMemo( () => onlyVersions @@ -84,11 +80,13 @@ function HistoryList({ pageId }: Props) { }, []); const handleHover = useCallback( - (historyId: string, index: number) => { + (historyId: string) => { clearPrefetchTimeout(); prefetchTimeoutRef.current = setTimeout(() => { prefetchPageHistory(historyId); - const prevId = historyItems[index + 1]?.id; + // The true previous snapshot in the FULL list (not the previous visible + // one under the "only versions" filter). + const prevId = resolvePrevSnapshotId(historyItems, historyId); if (prevId) { prefetchPageHistory(prevId); } @@ -102,9 +100,11 @@ function HistoryList({ pageId }: Props) { }, [clearPrefetchTimeout]); const handleSelect = useCallback( - (id: string, index: number) => { + (id: string) => { setActiveHistoryId(id); - setActiveHistoryPrevId(historyItems[index + 1]?.id ?? ""); + // Baseline = true previous snapshot in the FULL list, so the "only + // versions" filter never diffs/restores against the wrong item. + setActiveHistoryPrevId(resolvePrevSnapshotId(historyItems, id)); }, [historyItems, setActiveHistoryId, setActiveHistoryPrevId], ); @@ -173,9 +173,6 @@ function HistoryList({ pageId }: Props) { { + // Newest-first, as the history list stores it: a version, then two autosaves, + // then an older version. + const full = [ + { id: "v2", kind: "manual" }, + { id: "a2", kind: "idle" }, + { id: "a1", kind: "boundary" }, + { id: "v1", kind: "manual" }, + { id: "a0", kind: null }, + ]; + + it("returns the immediate FULL-list successor, not the previous visible version", () => { + // Selecting v2 while filtered to versions-only must baseline against a2 (the + // real chronological predecessor), NOT v1 (the previous visible version). + expect(resolvePrevSnapshotId(full, "v2")).toBe("a2"); + }); + + it("resolves an autosnapshot's predecessor by full-list order", () => { + expect(resolvePrevSnapshotId(full, "a1")).toBe("v1"); + }); + + it("returns '' for the oldest item (no predecessor)", () => { + expect(resolvePrevSnapshotId(full, "a0")).toBe(""); + }); + + it("returns '' for an id not in the list", () => { + expect(resolvePrevSnapshotId(full, "missing")).toBe(""); + }); + + it("does not depend on a filtered subset — same result whatever is visible", () => { + // The helper only ever sees the full list; a filtered view cannot change the + // baseline it computes. + expect(resolvePrevSnapshotId(full, "v1")).toBe("a0"); + }); +}); diff --git a/apps/client/src/features/page-history/utils/resolve-prev-snapshot.ts b/apps/client/src/features/page-history/utils/resolve-prev-snapshot.ts new file mode 100644 index 00000000..73320cd2 --- /dev/null +++ b/apps/client/src/features/page-history/utils/resolve-prev-snapshot.ts @@ -0,0 +1,22 @@ +/** + * #370 — resolve the TRUE previous snapshot for a history item. + * + * The history panel can be filtered to "only versions" (manual/agent), but diff + * and restore must always compare against the immediately-preceding snapshot in + * the FULL, unfiltered list — NOT the previous VISIBLE item. Comparing against + * the previous visible version would silently skip the autosnapshots between two + * versions and diff/restore the wrong baseline. + * + * Given the full (newest-first) list and an item id, this returns the id of the + * item right after it in the full list (its chronological predecessor), or "" if + * it is the oldest / not found. Pure and list-order-preserving so it can be unit + * tested without mounting the component. + */ +export function resolvePrevSnapshotId( + fullItems: ReadonlyArray<{ id: string }>, + id: string, +): string { + const index = fullItems.findIndex((item) => item.id === id); + if (index === -1) return ""; + return fullItems[index + 1]?.id ?? ""; +} diff --git a/apps/server/src/collaboration/extensions/compute-history-job.spec.ts b/apps/server/src/collaboration/extensions/compute-history-job.spec.ts index cd1d3711..be42040f 100644 --- a/apps/server/src/collaboration/extensions/compute-history-job.spec.ts +++ b/apps/server/src/collaboration/extensions/compute-history-job.spec.ts @@ -43,10 +43,10 @@ describe('computeHistoryJob (#370 — shared trailing idle pipeline)', () => { describe('max-wait ceiling', () => { const T0 = 1_000_000; // arbitrary fixed epoch for deterministic tests - it('early in the burst, the full trailing interval is used', () => { - // 1 minute into the burst: remaining budget (10m - 1m = 9m) still exceeds - // nothing that clamps it below the interval only if interval > remaining. - // For USER, interval 60m > remaining 9m, so delay clamps to the remaining. + it('once a burst is armed, delay clamps to the remaining max-wait budget', () => { + // 1 minute into the burst the USER interval (60m) far exceeds the remaining + // max-wait budget (10m - 1m = 9m), so the delay is clamped DOWN to that + // remaining budget — the full interval is NOT used once a ceiling applies. const { delay } = computeHistoryJob(page, 'user', T0, T0 + 60_000); expect(delay).toBe(IDLE_MAX_WAIT_USER - 60_000); }); diff --git a/apps/server/src/collaboration/extensions/persistence-store.spec.ts b/apps/server/src/collaboration/extensions/persistence-store.spec.ts index 73dbbe3c..c387b0c4 100644 --- a/apps/server/src/collaboration/extensions/persistence-store.spec.ts +++ b/apps/server/src/collaboration/extensions/persistence-store.spec.ts @@ -560,7 +560,7 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' // The pending idle autosnapshot is cancelled by the explicit version. expect(historyQueue.remove).toHaveBeenCalledWith(PAGE_ID); const msg = JSON.parse( - (document as any).broadcastStateless.mock.calls.at(-1)[0], + (document as any).broadcastStateless.mock.calls[(document as any).broadcastStateless.mock.calls.length - 1][0], ); expect(msg).toMatchObject({ type: 'version.saved', @@ -576,7 +576,7 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' await emitSave(document, 'agent'); - expect(pageHistoryRepo.saveHistory.mock.calls.at(-1)[1]).toEqual( + expect(pageHistoryRepo.saveHistory.mock.calls[pageHistoryRepo.saveHistory.mock.calls.length - 1][1]).toEqual( expect.objectContaining({ kind: 'agent' }), ); }); @@ -601,7 +601,7 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' ); expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); const msg = JSON.parse( - (document as any).broadcastStateless.mock.calls.at(-1)[0], + (document as any).broadcastStateless.mock.calls[(document as any).broadcastStateless.mock.calls.length - 1][0], ); expect(msg).toMatchObject({ historyId: 'auto-1', alreadySaved: false }); }); @@ -621,7 +621,7 @@ describe('PersistenceExtension.onStoreDocument — Approach-A boundary snapshot' expect(pageHistoryRepo.updateHistoryKind).not.toHaveBeenCalled(); expect(pageHistoryRepo.saveHistory).not.toHaveBeenCalled(); const msg = JSON.parse( - (document as any).broadcastStateless.mock.calls.at(-1)[0], + (document as any).broadcastStateless.mock.calls[(document as any).broadcastStateless.mock.calls.length - 1][0], ); expect(msg).toMatchObject({ alreadySaved: true, kind: 'manual' }); }); diff --git a/apps/server/src/collaboration/extensions/persistence.extension.ts b/apps/server/src/collaboration/extensions/persistence.extension.ts index 3c4de6cb..fc848a1e 100644 --- a/apps/server/src/collaboration/extensions/persistence.extension.ts +++ b/apps/server/src/collaboration/extensions/persistence.extension.ts @@ -693,12 +693,18 @@ export class PersistenceExtension implements Extension { // The FIRST arm of a burst records `burstStart`; computeHistoryJob shrinks // the delay to the remaining max-wait budget from that point, so a continuous // session cannot re-arm the trailing timer forever and starve the snapshot. - // A burst marker older than the (larger, USER) max-wait means the previous - // idle job has already fired — start a fresh window instead of firing - // immediately on the next edit. + // A burst marker older than THIS TIER's max-wait means the previous idle job + // has already fired — start a fresh window instead of firing immediately on + // the next edit. Must use the SAME source-specific max-wait computeHistoryJob + // uses (agent 5m / user 10m): a hardcoded USER ceiling would leave an agent + // burst's marker stale for 5..10m, forcing delay=0 on every store in that + // window and writing one idle row per store — exactly the per-store bloat the + // debounce exists to prevent, on the continuous-agent path. + const maxWait = + lastUpdatedSource === 'agent' ? IDLE_MAX_WAIT_AGENT : IDLE_MAX_WAIT_USER; const now = Date.now(); let burstStart = this.idleBurstStart.get(page.id); - if (burstStart === undefined || now - burstStart >= IDLE_MAX_WAIT_USER) { + if (burstStart === undefined || now - burstStart >= maxWait) { burstStart = now; this.idleBurstStart.set(page.id, burstStart); } diff --git a/apps/server/src/collaboration/processors/history.processor.spec.ts b/apps/server/src/collaboration/processors/history.processor.spec.ts index ecd44248..79d403f4 100644 --- a/apps/server/src/collaboration/processors/history.processor.spec.ts +++ b/apps/server/src/collaboration/processors/history.processor.spec.ts @@ -66,6 +66,15 @@ describe('HistoryProcessor.process', () => { notificationQueue = { add: jest.fn().mockResolvedValue(undefined) }; generalQueue = { add: jest.fn().mockResolvedValue(undefined) }; + // #370 F3 — the processor now serializes its find+save under a page-row lock + // via executeTx. A db whose transaction().execute(fn) runs fn with a trx stub + // drives the real executeTx() helper without a database. + const db = { + transaction: () => ({ + execute: (fn: (trx: any) => Promise) => fn({ __trx: true }), + }), + }; + // WorkerHost's constructor reads `this.worker`; passing repos positionally // matches the constructor and avoids the Nest DI container. proc = new HistoryProcessor( @@ -73,6 +82,7 @@ describe('HistoryProcessor.process', () => { pageRepo as any, collabHistory as any, watcherService as any, + db as any, notificationQueue as any, generalQueue as any, ); @@ -134,7 +144,8 @@ describe('HistoryProcessor.process', () => { ); expect(pageHistoryRepo.saveHistory).toHaveBeenCalledWith( expect.objectContaining({ id: PAGE_ID }), - { contributorIds: ['u1', 'u2'], kind: 'idle' }, + // #370 F3 — saveHistory now runs inside the locked tx, so it carries trx. + expect.objectContaining({ contributorIds: ['u1', 'u2'], kind: 'idle' }), ); expect(generalQueue.add).toHaveBeenCalledWith( QueueJob.PAGE_BACKLINKS, diff --git a/apps/server/src/collaboration/processors/history.processor.ts b/apps/server/src/collaboration/processors/history.processor.ts index 3fde25e4..5d98dec8 100644 --- a/apps/server/src/collaboration/processors/history.processor.ts +++ b/apps/server/src/collaboration/processors/history.processor.ts @@ -19,6 +19,9 @@ import { isDeepStrictEqual } from 'node:util'; import { CollabHistoryService } from '../services/collab-history.service'; import { WatcherService } from '../../core/watcher/watcher.service'; import { isEmptyParagraphDoc } from '../collaboration.util'; +import { InjectKysely } from 'nestjs-kysely'; +import { KyselyDB } from '@docmost/db/types/kysely.types'; +import { executeTx } from '@docmost/db/utils'; @Processor(QueueName.HISTORY_QUEUE) export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { @@ -29,6 +32,7 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { private readonly pageRepo: PageRepo, private readonly collabHistory: CollabHistoryService, private readonly watcherService: WatcherService, + @InjectKysely() private readonly db: KyselyDB, @InjectQueue(QueueName.NOTIFICATION_QUEUE) private notificationQueue: Queue, @InjectQueue(QueueName.GENERAL_QUEUE) private generalQueue: Queue, ) { @@ -41,6 +45,9 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { try { const { pageId } = job.data; + // Read the page WITHOUT a lock first, only to bail early on the two cheap + // no-write cases (page gone / empty first snapshot) without opening a + // transaction. The authoritative check-then-write happens locked below. const page = await this.pageRepo.findById(pageId, { includeContent: true, }); @@ -51,44 +58,81 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { return; } - const lastHistory = await this.pageHistoryRepo.findPageLastHistory( - pageId, - { includeContent: true }, - ); + // #370 F3 — the snapshot decision (findPageLastHistory → saveHistory) must + // be serialized against manual-save/boundary writers, which run under a + // page-row lock in onStoreDocument. Without it, this processor and a + // concurrent manual-save each read the same lastHistory (MVCC), both see + // content != lastHistory, and both insert — producing two page_history rows + // with IDENTICAL content (one 'idle', one 'manual'), defeating + // promote-not-dup and the version-vs-autosave split. Taking the same + // page-row lock makes the second writer observe the first's committed row so + // the isDeepStrictEqual gate collapses the duplicate. Only the read+write + // is transacted; the post-snapshot queue work stays outside. + let contributorIds: string[] = []; + let snapshotWritten = false; + let lastHistoryContent: unknown; - if (!lastHistory && isEmptyParagraphDoc(page.content as any)) { - this.logger.debug( - `Skipping first history for page ${pageId}: empty content`, + await executeTx(this.db, async (trx) => { + const lockedPage = await this.pageRepo.findById(pageId, { + includeContent: true, + withLock: true, + trx, + }); + if (!lockedPage) return; + + const lastHistory = await this.pageHistoryRepo.findPageLastHistory( + pageId, + { includeContent: true, trx }, ); - await this.collabHistory.clearContributors(pageId); - return; - } + lastHistoryContent = lastHistory?.content; - if ( - !lastHistory || - !isDeepStrictEqual(lastHistory.content, page.content) - ) { - const contributorIds = await this.collabHistory.popContributors(pageId); + if (!lastHistory && isEmptyParagraphDoc(lockedPage.content as any)) { + this.logger.debug( + `Skipping first history for page ${pageId}: empty content`, + ); + return; + } + if ( + lastHistory && + isDeepStrictEqual(lastHistory.content, lockedPage.content) + ) { + return; // already snapshotted at this content — nothing to write + } + + contributorIds = await this.collabHistory.popContributors(pageId); try { await this.watcherService.addPageWatchers( contributorIds, pageId, - page.spaceId, - page.workspaceId, + lockedPage.spaceId, + lockedPage.workspaceId, ); // #370 — every job on this queue is a trailing idle-flush autosnapshot. - await this.pageHistoryRepo.saveHistory(page, { + await this.pageHistoryRepo.saveHistory(lockedPage, { contributorIds, kind: job.data.kind ?? 'idle', + trx, }); + snapshotWritten = true; this.logger.debug(`History created for page: ${pageId}`); } catch (err) { await this.collabHistory.addContributors(pageId, contributorIds); throw err; } + }); + // No snapshot written (page vanished / empty-first / unchanged content) → + // clear the contributor set for the skip cases and stop. + if (!snapshotWritten) { + if (!lastHistoryContent && isEmptyParagraphDoc(page.content as any)) { + await this.collabHistory.clearContributors(pageId); + } + return; + } + + { const mentions = extractMentions(page.content); const pageMentions = extractPageMentions(mentions); const internalLinkSlugIds = extractInternalLinkSlugIds(page.content); @@ -106,7 +150,7 @@ export class HistoryProcessor extends WorkerHost implements OnModuleDestroy { ); }); - if (contributorIds.length > 0 && lastHistory?.content) { + if (contributorIds.length > 0 && lastHistoryContent) { await this.notificationQueue .add(QueueJob.PAGE_UPDATED, { pageId,