fix(sync): prevent silent content loss in concurrent-edit merges

Address review feedback on the concurrent-edit merge change:

- When no base snapshot is available (e.g. a metadata-only change bumped
  the version without creating a snapshot, or the base was pruned),
  mergeContentUpdate now keeps the incoming edit instead of discarding it
  in favour of the server copy, and logs a warning. Dropping it silently
  lost the user's offline edits.
- Unify getBaseContent into one shared helper in versions-service and drop
  sync's "fall back to the latest snapshot" branch: a base newer than what
  the client actually saw made the 3-way merge treat other users' lines as
  deletions and silently remove them.
- Remove the now-dead mergeContentTwoWay helper.
- Add coverage: null-base keeps incoming (unit), a metadata-bump concurrent
  edit is preserved (integration), and getBaseContent returns null rather
  than a newer snapshot. Verified the null-base cases fail against the old
  drop-incoming behavior.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

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', () => {

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;

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();

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<Parameters<Db['transaction']>[0]>[0];
-type SnapshotDb = Db | Transaction;
+export type Transaction = Parameters<Parameters<Db['transaction']>[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.

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<Parameters<Db['transaction']>[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.
  */

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');
 	});
 });

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);
 }