diff --git a/src/lib/server/notes-service.test.ts b/src/lib/server/notes-service.test.ts index 9877673..1bf2dce 100644 --- a/src/lib/server/notes-service.test.ts +++ b/src/lib/server/notes-service.test.ts @@ -263,6 +263,31 @@ describe('updateNote', () => { const updated = updateNote(db, OWNER_ID, 'n1', { title: 'Now with #newtag' }); expect(updated!.tags).toContain('newtag'); }); + + it('should merge concurrent content edits using the snapshot created by the first update', () => { + seedNote('n1', { + title: 'Shopping', + content: '- [ ] Milk\n- [ ] Bread', + checklistMode: true, + version: 1 + }); + shareNote('n1'); + + const ownerUpdate = updateNote(db, OWNER_ID, 'n1', { + content: '- [ ] Milk\n- [x] Bread', + baseVersion: 1 + }); + expect(ownerUpdate!.version).toBe(2); + + const collabUpdate = updateNote(db, COLLAB_ID, 'n1', { + content: '- [x] Milk\n- [ ] Bread', + baseVersion: 1 + }); + + expect(collabUpdate!.content).toBe('- [x] Milk\n- [x] Bread'); + const snapshots = db.select().from(noteVersions).where(eq(noteVersions.noteId, 'n1')).all(); + expect(snapshots.some((snapshot) => snapshot.version === 1)).toBe(true); + }); }); describe('deleteNote', () => { diff --git a/src/lib/server/notes-service.ts b/src/lib/server/notes-service.ts index 3fc8633..d20d23f 100644 --- a/src/lib/server/notes-service.ts +++ b/src/lib/server/notes-service.ts @@ -9,7 +9,7 @@ import { fetchCollaboratorsForNotes } from './collaborators.js'; import { fetchSharesForNotes } from './shares-service.js'; import { canAccessNote } from './api-utils.js'; import { createSnapshot } from './versions-service.js'; -import { mergeContent } from '$lib/utils/content-merge.js'; +import { mergeContentUpdate } from '$lib/utils/content-merge.js'; import type { NoteFilter } from '$lib/types/index.js'; interface NoteRow { @@ -215,6 +215,19 @@ 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; @@ -238,29 +251,18 @@ export function updateNote(db: Db, userId: number, id: string, input: UpdateNote if (input.title !== undefined) { sharedUpdates.title = input.title; hasSharedUpdates = true; } if (input.content !== undefined) { - // 3-way merge to preserve concurrent edits from other users. - // Only attempt merge when there's a concurrent edit: the client's - // baseVersion is behind the current version, meaning someone else - // saved between the client's load and this save. + // Preserve concurrent edits at line/block granularity when the client + // saved from an older note version. const hasConcurrentEdit = input.baseVersion !== undefined && input.baseVersion < existing.version; - if (hasConcurrentEdit && input.content !== existing.content) { - const baseSnapshot = db.select({ content: noteVersions.content }) - .from(noteVersions) - .where(and(eq(noteVersions.noteId, id), lte(noteVersions.version, input.baseVersion!))) - .orderBy(desc(noteVersions.version)) - .limit(1) - .get(); - - if (baseSnapshot && baseSnapshot.content !== existing.content) { - sharedUpdates.content = mergeContent(baseSnapshot.content, input.content, existing.content); - } else { - sharedUpdates.content = input.content; - } - } else { - sharedUpdates.content = input.content; - } + sharedUpdates.content = hasConcurrentEdit + ? mergeContentUpdate({ + baseContent: getBaseContent(db, id, input.baseVersion), + incomingContent: input.content, + currentContent: existing.content + }) + : input.content; hasSharedUpdates = true; } if (input.color !== undefined) { sharedUpdates.color = input.color; hasSharedUpdates = true; } @@ -278,6 +280,22 @@ export function updateNote(db: Db, userId: number, id: string, input: UpdateNote if (input.sortOrder !== undefined) { sharedUpdates.sortOrder = input.sortOrder; hasSharedUpdates = true; } } + const hasActualContentChange = + (input.title !== undefined && input.title !== existing.title) || + (sharedUpdates.content !== undefined && sharedUpdates.content !== existing.content) || + (input.color !== undefined && input.color !== existing.color) || + (input.checklistMode !== undefined && input.checklistMode !== existing.checklistMode); + + if (hasActualContentChange) { + createSnapshot(db, id, { + version: existing.version, + title: existing.title, + content: existing.content, + checklistMode: existing.checklistMode, + color: existing.color + }); + } + if (hasSharedUpdates || isOwner) { db.update(notes).set(sharedUpdates).where(eq(notes.id, id)).run(); } @@ -324,23 +342,6 @@ export function updateNote(db: Db, userId: number, id: string, input: UpdateNote } } - // Create a version snapshot only when at least one content field actually changed - const hasActualContentChange = - (input.title !== undefined && input.title !== existing.title) || - (input.content !== undefined && input.content !== existing.content) || - (input.color !== undefined && input.color !== existing.color) || - (input.checklistMode !== undefined && input.checklistMode !== existing.checklistMode); - - if (hasActualContentChange) { - createSnapshot(db, id, { - version: existing.version, - title: existing.title, - content: existing.content, - checklistMode: existing.checklistMode, - color: existing.color - }); - } - return getNote(db, userId, id); } diff --git a/src/lib/server/versions-service.ts b/src/lib/server/versions-service.ts index 717bdf2..73ddb8a 100644 --- a/src/lib/server/versions-service.ts +++ b/src/lib/server/versions-service.ts @@ -7,12 +7,15 @@ import type { NoteVersion, NoteVersionSummary, NoteColor } from '$lib/types/inde const MAX_VERSIONS_PER_NOTE = 50; const CONTENT_PREVIEW_LENGTH = 80; +type Transaction = Parameters[0]>[0]; +type SnapshotDb = Db | Transaction; + /** * Create a snapshot of a note's current state. * Only called when content actually changed. Prunes snapshots beyond MAX_VERSIONS_PER_NOTE. */ export function createSnapshot( -db: Db, +db: SnapshotDb, noteId: string, snapshot: { version: number; diff --git a/src/lib/sync/server.test.ts b/src/lib/sync/server.test.ts index 4c3d4f0..b2d5c4b 100644 --- a/src/lib/sync/server.test.ts +++ b/src/lib/sync/server.test.ts @@ -1,6 +1,6 @@ import { describe, it, expect, beforeEach } from 'vitest'; import { createTestDb } from '../server/db/test-helpers.js'; -import { notes, noteCollaborators, noteUserState, users, syncLog } from '../server/db/schema.js'; +import { notes, noteCollaborators, noteUserState, users, syncLog, noteVersions } from '../server/db/schema.js'; import { eq, and } from 'drizzle-orm'; import type { BetterSQLite3Database } from 'drizzle-orm/better-sqlite3'; import type * as schema from '../server/db/schema.js'; @@ -289,6 +289,64 @@ describe('processSyncPush — shared notes', () => { expect(note.content).toBe('- [x] Milk\n- [x] Bread'); }); + it('should create a base snapshot during sync so later queued edits can merge', async () => { + const t1 = new Date('2024-01-01T00:00:00Z'); + seedSharedNote('n1', { + updatedAt: t1, + createdAt: t1, + content: '- [ ] Milk\n- [ ] Bread', + checklistMode: true, + version: 1 + }); + + await processSyncPush(db, [{ + noteId: 'n1', + operation: 'update', + timestamp: t1.getTime() + 1000, + data: { content: '- [ ] Milk\n- [x] Bread' }, + baseVersion: 1 + }], OWNER_ID); + + await processSyncPush(db, [{ + noteId: 'n1', + operation: 'update', + timestamp: t1.getTime() + 2000, + data: { content: '- [x] Milk\n- [ ] Bread' }, + baseVersion: 1 + }], COLLAB_ID); + + const note = db.select().from(notes).where(eq(notes.id, 'n1')).get()!; + expect(note.content).toBe('- [x] Milk\n- [x] Bread'); + + const snapshots = db.select().from(noteVersions).where(eq(noteVersions.noteId, 'n1')).all(); + expect(snapshots.some((snapshot) => snapshot.version === 1)).toBe(true); + }); + + it('should retain at most 50 snapshots created by sync updates', async () => { + const createdAt = new Date('2024-01-01T00:00:00Z'); + seedSharedNote('n1', { + updatedAt: createdAt, + createdAt, + content: 'version 1', + version: 1 + }); + + for (let version = 2; version <= 52; version++) { + await processSyncPush(db, [{ + noteId: 'n1', + operation: 'update', + timestamp: createdAt.getTime() + version, + data: { content: `version ${version}` }, + baseVersion: version - 1 + }], OWNER_ID); + } + + const snapshots = db.select().from(noteVersions).where(eq(noteVersions.noteId, 'n1')).all(); + expect(snapshots).toHaveLength(50); + expect(snapshots.some((snapshot) => snapshot.version === 1)).toBe(false); + expect(snapshots.some((snapshot) => snapshot.version === 51)).toBe(true); + }); + it('should not allow non-collaborator to update the note', async () => { const t1 = new Date('2024-01-01T00:00:00Z'); // Create note without sharing with user 3 diff --git a/src/lib/sync/server.ts b/src/lib/sync/server.ts index f21c993..8703f39 100644 --- a/src/lib/sync/server.ts +++ b/src/lib/sync/server.ts @@ -1,10 +1,10 @@ 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, or, sql, desc, lte } from 'drizzle-orm'; +import { eq, gt, and, sql, desc, lte } from 'drizzle-orm'; import { extractTags } from '$lib/utils/tags.js'; import { syncNoteTags } from '$lib/server/tags.js'; -import { canAccessNote } from '$lib/server/api-utils.js'; -import { mergeContent } from '$lib/utils/content-merge.js'; +import { createSnapshot } from '$lib/server/versions-service.js'; +import { mergeContentUpdate } from '$lib/utils/content-merge.js'; import type { SyncQueueItem } from './idb.js'; /** @@ -69,16 +69,17 @@ export async function processSyncPush(db: Db, changes: SyncQueueItem[], userId: if (change.data.color !== undefined) sharedData.color = change.data.color; if (change.data.checklistMode !== undefined) sharedData.checklistMode = change.data.checklistMode; - // Content: 3-way merge to preserve concurrent edits from other users + // Content: merge only when the client saved from an older note version. if (change.data.content !== undefined && change.data.content !== note.content) { - const baseContent = getBaseContent(tx, change.noteId, change.baseVersion); - if (baseContent !== null && baseContent !== note.content) { - // 3-way merge: base, incoming (local), server current (remote) - sharedData.content = mergeContent(baseContent, change.data.content, note.content); - } else { - // No base or base matches server — take incoming content - sharedData.content = change.data.content; - } + const hasConcurrentEdit = change.baseVersion !== undefined + && change.baseVersion < note.version; + sharedData.content = hasConcurrentEdit + ? mergeContentUpdate({ + baseContent: getBaseContent(tx, change.noteId, change.baseVersion), + incomingContent: change.data.content, + currentContent: note.content + }) + : change.data.content; } if (isOwner) { @@ -92,6 +93,22 @@ export async function processSyncPush(db: Db, changes: SyncQueueItem[], userId: if (change.data.sortOrder !== undefined) sharedData.sortOrder = change.data.sortOrder; } + const hasActualContentChange = + (change.data.title !== undefined && change.data.title !== note.title) || + (sharedData.content !== undefined && sharedData.content !== note.content) || + (change.data.color !== undefined && change.data.color !== note.color) || + (change.data.checklistMode !== undefined && change.data.checklistMode !== note.checklistMode); + + if (hasActualContentChange) { + createSnapshot(tx, change.noteId, { + version: note.version, + title: note.title, + content: note.content, + checklistMode: note.checklistMode, + color: note.color + }); + } + tx.update(notes) .set(sharedData) .where(eq(notes.id, change.noteId)) diff --git a/src/lib/utils/content-merge.test.ts b/src/lib/utils/content-merge.test.ts index 52b4705..e532f06 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 } from './content-merge.js'; +import { mergeContent, mergeContentTwoWay, mergeContentUpdate } from './content-merge.js'; describe('mergeContent (3-way)', () => { it('should return remote when local is unchanged', () => { @@ -94,12 +94,37 @@ describe('mergeContentTwoWay', () => { expect(mergeContentTwoWay('hello', 'hello')).toBe('hello'); }); - it('should attempt merge when contents differ', () => { + it('should preserve the current server document when contents differ', () => { const local = '- [x] Milk\n- [ ] Bread'; const remote = '- [ ] Milk\n- [ ] Bread\n- [ ] Eggs'; - const result = mergeContentTwoWay(local, remote); - // Without a base, conflict resolution prefers remote - expect(result).toContain('- [ ] Bread'); + 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({ + baseContent: '- [ ] Milk\n- [ ] Bread', + incomingContent: '- [x] Milk\n- [ ] Bread', + currentContent: '- [ ] Milk\n- [x] Bread' + }); + + expect(result).toBe('- [x] Milk\n- [x] Bread'); + }); + + it('should preserve current content when the base is missing', () => { + const result = mergeContentUpdate({ + baseContent: null, + incomingContent: '- [x] Milk', + currentContent: '- [ ] Bread' + }); + + expect(result).toBe('- [ ] Bread'); }); }); diff --git a/src/lib/utils/content-merge.ts b/src/lib/utils/content-merge.ts index bedbbad..7c16150 100644 --- a/src/lib/utils/content-merge.ts +++ b/src/lib/utils/content-merge.ts @@ -79,10 +79,31 @@ export function mergeContent(base: string, local: string, remote: string): strin /** * 2-way merge fallback when no base is available. - * Uses an empty string as the base, so all lines from both sides appear - * as additions. For conflicts (same region), prefers remote. + * 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 { - if (local === remote) return remote; - return mergeContent('', local, remote); + 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. + */ +export function mergeContentUpdate(input: { + baseContent: string | null; + incomingContent: string; + currentContent: string; +}): string { + const { baseContent, incomingContent, currentContent } = input; + + if (incomingContent === currentContent) return currentContent; + if (baseContent === incomingContent) return currentContent; + if (baseContent === currentContent) return incomingContent; + + return baseContent === null + ? mergeContentTwoWay(incomingContent, currentContent) + : mergeContent(baseContent, incomingContent, currentContent); }