diff --git a/packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.spec.ts b/packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.spec.ts new file mode 100644 index 00000000000..cab9823cb47 --- /dev/null +++ b/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( + { + uuid: 'note-uuid', + content_type: ContentType.TYPES.Note, + ...PayloadTimestampDefaults(), + dirty: overrides.dirty, + content: FillItemContent({ + 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( + { + uuid: 'incoming-uuid', + content_type: ContentType.TYPES.Note, + ...PayloadTimestampDefaults(), + content: FillItemContent({ + 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) + }) +}) diff --git a/packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.ts b/packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.ts index 5869e7eda9d..3388bb23e10 100644 --- a/packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.ts +++ b/packages/models/src/Domain/Abstract/Item/Implementations/GenericItem.ts @@ -180,7 +180,7 @@ export abstract class GenericItem

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

* 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 { diff --git a/packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts b/packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts index dce3b5a1efe..19b213150df 100644 --- a/packages/models/src/Domain/Runtime/Deltas/Conflict.spec.ts +++ b/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({ + uuid: '123', + content_type: ContentType.TYPES.Note, + content: FillItemContent({ title: 'base', text: '' }), + ...PayloadTimestampDefaults(), + }) + + const nonMatchingConflict = new DecryptedPayload({ + uuid: 'conflict-1', + content_type: ContentType.TYPES.Note, + content: FillItemContent({ title: 'something else', text: '', conflict_of: '123' }), + ...PayloadTimestampDefaults(), + }) + + const matchingConflict = new DecryptedPayload({ + uuid: 'conflict-2', + content_type: ContentType.TYPES.Note, + content: FillItemContent({ title: 'incoming content', text: '', conflict_of: '123' }), + ...PayloadTimestampDefaults(), + }) + + const baseCollection = createBaseCollection(basePayload, nonMatchingConflict, matchingConflict) + + const applyPayload = new DecryptedPayload({ + uuid: '123', + content_type: ContentType.TYPES.Note, + content: FillItemContent({ 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) diff --git a/packages/models/src/Domain/Runtime/Deltas/Conflict.ts b/packages/models/src/Domain/Runtime/Deltas/Conflict.ts index a4c81fa4b8e..5c148ed7c5b 100644 --- a/packages/models/src/Domain/Runtime/Deltas/Conflict.ts +++ b/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 {