diff --git a/src/lib/server/notes-service.test.ts b/src/lib/server/notes-service.test.ts index 1bf2dce..d3bc084 100644 --- a/src/lib/server/notes-service.test.ts +++ b/src/lib/server/notes-service.test.ts @@ -288,6 +288,21 @@ describe('updateNote', () => { const snapshots = db.select().from(noteVersions).where(eq(noteVersions.noteId, 'n1')).all(); expect(snapshots.some((snapshot) => snapshot.version === 1)).toBe(true); }); + + it('should keep the incoming edit when a metadata-only bump left no base snapshot', () => { + seedNote('n1', { title: 'Note', content: 'hello', version: 1 }); + + // A metadata-only change bumps the version but creates no snapshot. + updateNote(db, OWNER_ID, 'n1', { pinned: true }); + const snapshots = db.select().from(noteVersions).where(eq(noteVersions.noteId, 'n1')).all(); + expect(snapshots).toHaveLength(0); + + // A later concurrent content edit based on the pre-bump version therefore has + // no base to merge against — the incoming edit must be preserved, not dropped. + const edited = updateNote(db, OWNER_ID, 'n1', { content: 'hello world', baseVersion: 1 }); + + expect(edited!.content).toBe('hello world'); + }); }); describe('deleteNote', () => { diff --git a/src/lib/server/notes-service.ts b/src/lib/server/notes-service.ts index d20d23f..0d6b6e9 100644 --- a/src/lib/server/notes-service.ts +++ b/src/lib/server/notes-service.ts @@ -1,6 +1,6 @@ import type { Db } from './db/index.js'; -import { notes, noteTags, tags, noteCollaborators, noteUserState, noteVersions } from './db/schema.js'; -import { eq, and, desc, like, or, inArray, sql, lte } from 'drizzle-orm'; +import { notes, noteTags, tags, noteCollaborators, noteUserState } from './db/schema.js'; +import { eq, and, like, or, inArray, sql } from 'drizzle-orm'; import { v4 as uuidv4 } from 'uuid'; import { extractTags } from '$lib/utils/tags.js'; import { fetchTagsForNotes, syncNoteTags } from './tags.js'; @@ -8,7 +8,7 @@ import { fetchAttachmentsForNotes } from './attachments.js'; import { fetchCollaboratorsForNotes } from './collaborators.js'; import { fetchSharesForNotes } from './shares-service.js'; import { canAccessNote } from './api-utils.js'; -import { createSnapshot } from './versions-service.js'; +import { createSnapshot, getBaseContent } from './versions-service.js'; import { mergeContentUpdate } from '$lib/utils/content-merge.js'; import type { NoteFilter } from '$lib/types/index.js'; @@ -215,19 +215,6 @@ export interface UpdateNoteInput { /** Per-user fields that go to noteUserState for collaborators */ const PER_USER_FIELDS = ['pinned', 'archived', 'sortOrder'] as const; -function getBaseContent(db: Db, noteId: string, baseVersion: number | undefined): string | null { - if (baseVersion === undefined) return null; - - const snapshot = db.select({ content: noteVersions.content }) - .from(noteVersions) - .where(and(eq(noteVersions.noteId, noteId), lte(noteVersions.version, baseVersion))) - .orderBy(desc(noteVersions.version)) - .limit(1) - .get(); - - return snapshot?.content ?? null; -} - export function updateNote(db: Db, userId: number, id: string, input: UpdateNoteInput) { const { canAccess, isOwner } = canAccessNote(db, id, userId); if (!canAccess) return null; diff --git a/src/lib/server/versions-service.test.ts b/src/lib/server/versions-service.test.ts index 681dc9f..0b69f16 100644 --- a/src/lib/server/versions-service.test.ts +++ b/src/lib/server/versions-service.test.ts @@ -3,7 +3,7 @@ import { createTestDb } from './db/test-helpers.js'; import { users, notes, noteVersions } from './db/schema.js'; import { eq } from 'drizzle-orm'; import type { Db } from './db/index.js'; -import { createSnapshot, listVersions, getVersion, snapshotCurrentNote } from './versions-service.js'; +import { createSnapshot, listVersions, getVersion, snapshotCurrentNote, getBaseContent } from './versions-service.js'; let db: Db; @@ -44,6 +44,32 @@ beforeEach(() => { seedUser(); }); +describe('getBaseContent', () => { + it('returns null when the base version is unknown', () => { + seedNote(); + expect(getBaseContent(db, NOTE_ID, undefined)).toBeNull(); + }); + + it('returns the snapshot content at or before the base version', () => { + seedNote(); + createSnapshot(db, NOTE_ID, { version: 1, title: 'T', content: 'v1', checklistMode: false, color: 'default' }); + createSnapshot(db, NOTE_ID, { version: 3, title: 'T', content: 'v3', checklistMode: false, color: 'default' }); + + expect(getBaseContent(db, NOTE_ID, 2)).toBe('v1'); + expect(getBaseContent(db, NOTE_ID, 3)).toBe('v3'); + }); + + it('returns null instead of a newer snapshot when none exists at or before the base version', () => { + seedNote(); + // Only a snapshot NEWER than the requested base exists (e.g. the real base was + // pruned). Using it as the merge base would treat unseen lines as deletions, + // so getBaseContent must return null rather than fall back to it. + createSnapshot(db, NOTE_ID, { version: 5, title: 'T', content: 'v5', checklistMode: false, color: 'default' }); + + expect(getBaseContent(db, NOTE_ID, 2)).toBeNull(); + }); +}); + describe('createSnapshot', () => { it('should store a snapshot for a note', () => { seedNote(); diff --git a/src/lib/server/versions-service.ts b/src/lib/server/versions-service.ts index 73ddb8a..e8fa1ae 100644 --- a/src/lib/server/versions-service.ts +++ b/src/lib/server/versions-service.ts @@ -1,14 +1,39 @@ import type { Db } from './db/index.js'; import { noteVersions, notes } from './db/schema.js'; -import { eq, and, desc } from 'drizzle-orm'; +import { eq, and, desc, lte } from 'drizzle-orm'; import { v4 as uuidv4 } from 'uuid'; import type { NoteVersion, NoteVersionSummary, NoteColor } from '$lib/types/index.js'; const MAX_VERSIONS_PER_NOTE = 50; const CONTENT_PREVIEW_LENGTH = 80; -type Transaction = Parameters[0]>[0]; -type SnapshotDb = Db | Transaction; +export type Transaction = Parameters[0]>[0]; +export type SnapshotDb = Db | Transaction; + +/** + * Resolve the 3-way merge base for a concurrent edit: the content of the snapshot + * at or before the client's base version. Returns null when the base version is + * unknown or no snapshot at/before it exists. Callers must NOT substitute a newer + * snapshot as the base — a base newer than what the client actually saw makes the + * merge treat other users' lines as deletions and silently drop them. + */ +export function getBaseContent( + db: SnapshotDb, + noteId: string, + baseVersion: number | undefined +): string | null { + if (baseVersion === undefined) return null; + + const snapshot = db + .select({ content: noteVersions.content }) + .from(noteVersions) + .where(and(eq(noteVersions.noteId, noteId), lte(noteVersions.version, baseVersion))) + .orderBy(desc(noteVersions.version)) + .limit(1) + .get(); + + return snapshot?.content ?? null; +} /** * Create a snapshot of a note's current state. diff --git a/src/lib/sync/server.ts b/src/lib/sync/server.ts index 8703f39..21aae19 100644 --- a/src/lib/sync/server.ts +++ b/src/lib/sync/server.ts @@ -1,37 +1,12 @@ import type { Db } from '$lib/server/db/index.js'; -import { notes, noteCollaborators, noteUserState, noteVersions, syncLog } from '$lib/server/db/schema.js'; -import { eq, gt, and, sql, desc, lte } from 'drizzle-orm'; +import { notes, noteCollaborators, noteUserState, syncLog } from '$lib/server/db/schema.js'; +import { eq, gt, and, sql } from 'drizzle-orm'; import { extractTags } from '$lib/utils/tags.js'; import { syncNoteTags } from '$lib/server/tags.js'; -import { createSnapshot } from '$lib/server/versions-service.js'; +import { createSnapshot, getBaseContent } from '$lib/server/versions-service.js'; import { mergeContentUpdate } from '$lib/utils/content-merge.js'; import type { SyncQueueItem } from './idb.js'; -/** - * Find the base content for 3-way merge from version snapshots. - * Returns the content from the snapshot at or before the given version, - * or the current note content if no snapshot is found. - */ -function getBaseContent(tx: Parameters[0]>[0], noteId: string, baseVersion?: number): string | null { - if (baseVersion !== undefined) { - const snapshot = tx.select({ content: noteVersions.content }) - .from(noteVersions) - .where(and(eq(noteVersions.noteId, noteId), lte(noteVersions.version, baseVersion))) - .orderBy(desc(noteVersions.version)) - .limit(1) - .get(); - if (snapshot) return snapshot.content; - } - // Fallback: get the most recent snapshot before the current version - const latest = tx.select({ content: noteVersions.content }) - .from(noteVersions) - .where(eq(noteVersions.noteId, noteId)) - .orderBy(desc(noteVersions.version)) - .limit(1) - .get(); - return latest?.content ?? null; -} - /** * Process incoming sync changes from client. */ diff --git a/src/lib/utils/content-merge.test.ts b/src/lib/utils/content-merge.test.ts index e532f06..343b681 100644 --- a/src/lib/utils/content-merge.test.ts +++ b/src/lib/utils/content-merge.test.ts @@ -1,5 +1,5 @@ import { describe, it, expect } from 'vitest'; -import { mergeContent, mergeContentTwoWay, mergeContentUpdate } from './content-merge.js'; +import { mergeContent, mergeContentUpdate } from './content-merge.js'; describe('mergeContent (3-way)', () => { it('should return remote when local is unchanged', () => { @@ -89,24 +89,6 @@ describe('mergeContent (3-way)', () => { }); }); -describe('mergeContentTwoWay', () => { - it('should return remote when both are identical', () => { - expect(mergeContentTwoWay('hello', 'hello')).toBe('hello'); - }); - - it('should preserve the current server document when contents differ', () => { - const local = '- [x] Milk\n- [ ] Bread'; - const remote = '- [ ] Milk\n- [ ] Bread\n- [ ] Eggs'; - - expect(mergeContentTwoWay(local, remote)).toBe(remote); - }); - - it('should not resurrect content deleted from the current document', () => { - expect(mergeContentTwoWay('deleted content', '')).toBe(''); - }); -}); - - describe('mergeContentUpdate', () => { it('should merge an incoming update against current content when a base is available', () => { const result = mergeContentUpdate({ @@ -118,13 +100,45 @@ describe('mergeContentUpdate', () => { expect(result).toBe('- [x] Milk\n- [x] Bread'); }); - it('should preserve current content when the base is missing', () => { + it('should keep the incoming edit when no base snapshot is available', () => { + // Without a common ancestor a 3-way merge is impossible; dropping the + // incoming edit would silently lose the user's changes. const result = mergeContentUpdate({ baseContent: null, incomingContent: '- [x] Milk', currentContent: '- [ ] Bread' }); - expect(result).toBe('- [ ] Bread'); + expect(result).toBe('- [x] Milk'); + }); + + it('should keep current content when the incoming edit equals it', () => { + const result = mergeContentUpdate({ + baseContent: null, + incomingContent: 'same', + currentContent: 'same' + }); + + expect(result).toBe('same'); + }); + + it('should keep current content when the incoming edit matches the base (no local change)', () => { + const result = mergeContentUpdate({ + baseContent: 'base', + incomingContent: 'base', + currentContent: 'server changed' + }); + + expect(result).toBe('server changed'); + }); + + it('should take the incoming edit when the server is unchanged from the base', () => { + const result = mergeContentUpdate({ + baseContent: 'base', + incomingContent: 'incoming change', + currentContent: 'base' + }); + + expect(result).toBe('incoming change'); }); }); diff --git a/src/lib/utils/content-merge.ts b/src/lib/utils/content-merge.ts index 7c16150..bdaa337 100644 --- a/src/lib/utils/content-merge.ts +++ b/src/lib/utils/content-merge.ts @@ -77,20 +77,13 @@ export function mergeContent(base: string, local: string, remote: string): strin return merged.join('\n'); } -/** - * 2-way merge fallback when no base is available. - * Without a common ancestor, edits and deletions cannot be distinguished from - * concurrent changes. Preserve the current server document rather than - * fabricating content by combining two complete snapshots. - */ -export function mergeContentTwoWay(local: string, remote: string): string { - return remote; -} - /** * Merge an incoming content snapshot into the current server content. - * Uses 3-way merge when a base is available; otherwise falls back to a - * conservative 2-way merge instead of replacing the whole document. + * + * Uses a 3-way merge when a base (common ancestor) is available. Without a base — + * e.g. the base snapshot was pruned, or the version was bumped by a metadata-only + * change that created no snapshot — a 3-way merge is impossible, so keep the + * incoming edit rather than silently discarding the user's changes. */ export function mergeContentUpdate(input: { baseContent: string | null; @@ -103,7 +96,10 @@ export function mergeContentUpdate(input: { if (baseContent === incomingContent) return currentContent; if (baseContent === currentContent) return incomingContent; - return baseContent === null - ? mergeContentTwoWay(incomingContent, currentContent) - : mergeContent(baseContent, incomingContent, currentContent); + if (baseContent === null) { + console.warn('[content-merge] no base snapshot for concurrent edit; keeping incoming content'); + return incomingContent; + } + + return mergeContent(baseContent, incomingContent, currentContent); }