fix: reduce potential conflict duplicates

packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.spec.ts

@@ -0,0 +1,130 @@
+import { ContentType } from '@standardnotes/domain-core'
+import { FillItemContent } from '../../Content/ItemContent'
+import { DecryptedPayload, PayloadTimestampDefaults } from '../../Payload'
+import { PayloadSource } from '../../Payload/Types/PayloadSource'
+import { ConflictStrategy } from '../Types/ConflictStrategy'
+import { AppDataField } from '../Types/AppDataField'
+import { DefaultAppDomain } from '../Types/DefaultAppDomain'
+import { SNNote } from '../../../Syncable/Note/Note'
+import { NoteContent } from '../../../Syncable/Note/NoteContent'
+
+describe('GenericItem strategyWhenConflictingWithItem', () => {
+  const createNote = (
+    overrides: {
+      title?: string
+      text?: string
+      dirty?: boolean
+      userModifiedDate?: Date
+      source?: PayloadSource
+    } = {},
+  ): SNNote => {
+    const now = overrides.userModifiedDate ?? new Date()
+    return new SNNote(
+      new DecryptedPayload<NoteContent>(
+        {
+          uuid: 'note-uuid',
+          content_type: ContentType.TYPES.Note,
+          ...PayloadTimestampDefaults(),
+          dirty: overrides.dirty,
+          content: FillItemContent<NoteContent>({
+            title: overrides.title ?? 'Test Note',
+            text: overrides.text ?? 'some text',
+            appData: {
+              [DefaultAppDomain]: {
+                [AppDataField.UserModifiedDate]: now.toISOString(),
+              },
+            },
+          }),
+        },
+        overrides.source,
+      ),
+    )
+  }
+
+  const createIncomingNote = (
+    overrides: { title?: string; text?: string; source?: PayloadSource } = {},
+  ): SNNote => {
+    return new SNNote(
+      new DecryptedPayload<NoteContent>(
+        {
+          uuid: 'incoming-uuid',
+          content_type: ContentType.TYPES.Note,
+          ...PayloadTimestampDefaults(),
+          content: FillItemContent<NoteContent>({
+            title: overrides.title ?? 'Incoming Note',
+            text: overrides.text ?? 'different text',
+          }),
+        },
+        overrides.source,
+      ),
+    )
+  }
+
+  it('should keep apply when local item is not dirty and content differs', () => {
+    const localNote = createNote({
+      title: 'Local Title',
+      dirty: false,
+      userModifiedDate: new Date(Date.now() - 120_000),
+    })
+    const incomingNote = createIncomingNote({ title: 'Server Title' })
+
+    const strategy = localNote.strategyWhenConflictingWithItem(incomingNote)
+
+    expect(strategy).toBe(ConflictStrategy.KeepApply)
+  })
+
+  it('should duplicate base when local item is dirty and content differs', () => {
+    const localNote = createNote({
+      title: 'Local Title',
+      dirty: true,
+      userModifiedDate: new Date(Date.now() - 120_000),
+    })
+    const incomingNote = createIncomingNote({ title: 'Server Title' })
+
+    const strategy = localNote.strategyWhenConflictingWithItem(incomingNote)
+
+    expect(strategy).toBe(ConflictStrategy.DuplicateBaseKeepApply)
+  })
+
+  it('should keep base and duplicate apply when edited within 60 seconds', () => {
+    const localNote = createNote({
+      title: 'Local Title',
+      dirty: true,
+      userModifiedDate: new Date(Date.now() - 30_000),
+    })
+    const incomingNote = createIncomingNote({ title: 'Server Title' })
+
+    const strategy = localNote.strategyWhenConflictingWithItem(incomingNote)
+
+    expect(strategy).toBe(ConflictStrategy.KeepBaseDuplicateApply)
+  })
+
+  it('should not treat 60+ seconds ago as actively editing', () => {
+    const localNote = createNote({
+      title: 'Local Title',
+      dirty: true,
+      userModifiedDate: new Date(Date.now() - 61_000),
+    })
+    const incomingNote = createIncomingNote({ title: 'Server Title' })
+
+    const strategy = localNote.strategyWhenConflictingWithItem(incomingNote)
+
+    expect(strategy).toBe(ConflictStrategy.DuplicateBaseKeepApply)
+  })
+
+  it('should keep base duplicate apply for file imports regardless of dirty state', () => {
+    const localNote = createNote({
+      title: 'Local Title',
+      dirty: false,
+      userModifiedDate: new Date(Date.now() - 120_000),
+    })
+    const incomingNote = createIncomingNote({
+      title: 'Imported Title',
+      source: PayloadSource.FileImport,
+    })
+
+    const strategy = localNote.strategyWhenConflictingWithItem(incomingNote)
+
+    expect(strategy).toBe(ConflictStrategy.KeepBaseDuplicateApply)
+  })
+})

packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.ts

@@ -180,7 +180,7 @@ export abstract class GenericItem<P extends PayloadInterface = PayloadInterface>
           return ConflictStrategy.KeepBase
         }
       }
-      const twentySeconds = 20_000
+      const sixtySeconds = 60_000
       if (
         /**
          * If the incoming item comes from an import, treat it as
@@ -191,10 +191,14 @@ export abstract class GenericItem<P extends PayloadInterface = PayloadInterface>
          * If the user is actively editing our item, duplicate the incoming item
          * to avoid creating surprises in the client's UI.
          */
-        Date.now() - this.userModifiedDate.getTime() < twentySeconds
+        Date.now() - this.userModifiedDate.getTime() < sixtySeconds
       ) {
         return ConflictStrategy.KeepBaseDuplicateApply
       } else {
+        if (this.dirty === false) {
+          // local item was already synced, just a stale server copy
+          return ConflictStrategy.KeepApply
+        }
         return ConflictStrategy.DuplicateBaseKeepApply
       }
     } else {

packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts

@@ -8,6 +8,7 @@ import {
   PayloadTimestampDefaults,
 } from '../../Abstract/Payload'
 import { ItemsKeyContent } from '../../Syncable/ItemsKey/ItemsKeyInterface'
+import { NoteContent } from '../../Syncable/Note/NoteContent'
 import { ImmutablePayloadCollection } from '../Collection/Payload/ImmutablePayloadCollection'
 import { PayloadCollection } from '../Collection/Payload/PayloadCollection'
 import { HistoryMap } from '../History'
@@ -16,9 +17,11 @@ import { ConflictDelta } from './Conflict'
 describe('conflict delta', () => {
   const historyMap = {} as HistoryMap
 
-  const createBaseCollection = (payload: FullyFormedPayloadInterface) => {
+  const createBaseCollection = (...payloads: FullyFormedPayloadInterface[]) => {
     const baseCollection = new PayloadCollection()
-    baseCollection.set(payload)
+    for (const payload of payloads) {
+      baseCollection.set(payload)
+    }
     return ImmutablePayloadCollection.FromCollection(baseCollection)
   }
 
@@ -100,6 +103,43 @@ describe('conflict delta', () => {
     expect(delta.getConflictStrategy()).toBe(ConflictStrategy.KeepApply)
   })
 
+  it('should detect matching conflict even if it is not the first in the list', () => {
+    const basePayload = new DecryptedPayload<NoteContent>({
+      uuid: '123',
+      content_type: ContentType.TYPES.Note,
+      content: FillItemContent<NoteContent>({ title: 'base', text: '' }),
+      ...PayloadTimestampDefaults(),
+    })
+
+    const nonMatchingConflict = new DecryptedPayload<NoteContent>({
+      uuid: 'conflict-1',
+      content_type: ContentType.TYPES.Note,
+      content: FillItemContent<NoteContent>({ title: 'something else', text: '', conflict_of: '123' }),
+      ...PayloadTimestampDefaults(),
+    })
+
+    const matchingConflict = new DecryptedPayload<NoteContent>({
+      uuid: 'conflict-2',
+      content_type: ContentType.TYPES.Note,
+      content: FillItemContent<NoteContent>({ title: 'incoming content', text: '', conflict_of: '123' }),
+      ...PayloadTimestampDefaults(),
+    })
+
+    const baseCollection = createBaseCollection(basePayload, nonMatchingConflict, matchingConflict)
+
+    const applyPayload = new DecryptedPayload<NoteContent>({
+      uuid: '123',
+      content_type: ContentType.TYPES.Note,
+      content: FillItemContent<NoteContent>({ title: 'incoming content', text: '' }),
+      ...PayloadTimestampDefaults(),
+      updated_at_timestamp: 999,
+    })
+
+    const delta = new ConflictDelta(baseCollection, basePayload, applyPayload, historyMap)
+
+    expect(delta.getConflictStrategy()).toBe(ConflictStrategy.KeepBase)
+  })
+
   it('if keep base strategy, always use the apply payloads updated_at_timestamp', () => {
     const basePayload = createDecryptedItemsKey('123', 'secret', 2)
 

packages/models/src/Domain/Runtime/Deltas/Conflict.ts

@@ -59,13 +59,14 @@ export class ConflictDelta {
        * uploading the changes until after the multi-page request completes, we may have
        * already conflicted this item.
        */
-      const existingConflict = this.baseCollection.conflictsOf(this.applyPayload.uuid)[0]
-      if (
-        existingConflict &&
-        isDecryptedPayload(existingConflict) &&
-        isDecryptedPayload(this.applyPayload) &&
-        PayloadContentsEqual(existingConflict, this.applyPayload)
-      ) {
+      const existingConflicts = this.baseCollection.conflictsOf(this.applyPayload.uuid)
+      const hasMatchingConflict = existingConflicts.some(
+        (conflict) =>
+          isDecryptedPayload(conflict) &&
+          isDecryptedPayload(this.applyPayload) &&
+          PayloadContentsEqual(conflict, this.applyPayload),
+      )
+      if (hasMatchingConflict) {
         /** Conflict exists and its contents are the same as incoming value, do not make duplicate */
         return ConflictStrategy.KeepBase
       } else {