Merge pull request #34714 from overleaf/mj-tracked-change-merges

[overleaf-editor-core+ranges-tracker] Pick lower timestamp when merging tracking info

GitOrigin-RevId: 5b1c8d7c719003df4506014c381f2a525d0a7ff4

libraries/overleaf-editor-core/lib/file_data/tracked_change.js

@@ -56,8 +56,7 @@ class TrackedChange {
       return false
     }
     return (
-      this.tracking.type === other.tracking.type &&
-      this.tracking.userId === other.tracking.userId &&
+      this.tracking.canMergeWith(other.tracking) &&
       this.range.touches(other.range) &&
       this.range.canMerge(other.range)
     )
@@ -75,13 +74,7 @@ class TrackedChange {
     }
     return new TrackedChange(
       this.range.merge(other.range),
-      new TrackingProps(
-        this.tracking.type,
-        this.tracking.userId,
-        this.tracking.ts.getTime() > other.tracking.ts.getTime()
-          ? this.tracking.ts
-          : other.tracking.ts
-      )
+      this.tracking.mergeWith(other.tracking)
     )
   }
 

libraries/overleaf-editor-core/test/unit/text_operation.test.js

@@ -405,6 +405,49 @@ describe('TextOperation', function () {
       })
     )
 
+    it(
+      'compose associativity (randomised)',
+      random.test(numTrials, () => {
+        const str = random.string(20)
+        const comments = random.comments(6)
+
+        const a = randomOperation(str, comments.ids)
+        const afterA = new StringFileData(str, comments.comments)
+        a.apply(afterA)
+
+        const b = randomOperation(afterA.getContent(), comments.ids)
+        const afterB = new StringFileData(
+          afterA.getContent(),
+          comments.comments
+        )
+        b.apply(afterB)
+
+        const c = randomOperation(afterB.getContent(), comments.ids)
+
+        const ab = a.compose(b)
+        const ab_c = ab.compose(c)
+
+        const bc = b.compose(c)
+        const a_bc = a.compose(bc)
+
+        const ab_c_file = new StringFileData(str, comments.comments)
+        ab_c.apply(ab_c_file)
+
+        const a_bc_file = new StringFileData(str, comments.comments)
+        a_bc.apply(a_bc_file)
+
+        const fuzzingError = fuzzingErrorMessage({
+          str,
+          comments,
+          a: a.toJSON(),
+          b: b.toJSON(),
+          c: c.toJSON(),
+        })
+
+        expect(ab_c_file.toRaw()).to.deep.equal(a_bc_file.toRaw(), fuzzingError)
+      })
+    )
+
     it('composes two operations with comments', function () {
       expect(
         compose(
@@ -625,11 +668,54 @@ describe('TextOperation', function () {
           a: a.toJSON(),
           b: b.toJSON(),
         })
-        expect(abPrime.equals(baPrime)).to.be.equal(true, fuzzingError)
+        // The composition of ab' and ba' is not guaranteed to be equal, but
+        // should converge to the same file contents + ranges.
         expect(abFile.toRaw()).to.deep.equal(baFile.toRaw(), fuzzingError)
       })
     )
 
+    it('chooses lower tracked change timestamp', function () {
+      const ts1 = '2024-01-01T01:00:00.000Z'
+      const ts2 = '2024-01-01T02:00:00.000Z'
+      const str = 'abcde'
+      const comments = []
+
+      const a = new TextOperation()
+        .retain(2, {
+          tracking: TrackingProps.fromRaw({
+            ts: ts1,
+            type: 'insert',
+            userId: 'user1',
+          }),
+        })
+        .retain(1)
+        .retain(2, {
+          tracking: TrackingProps.fromRaw({
+            ts: ts2,
+            type: 'insert',
+            userId: 'user1',
+          }),
+        })
+
+      const b = new TextOperation().retain(1).remove(3).retain(1)
+
+      const [aPrime, bPrime] = TextOperation.transform(a, b)
+      const aComposeBPrime = a.compose(bPrime)
+      const bComposeAPrime = b.compose(aPrime)
+
+      const aBPFile = new StringFileData(str, comments)
+      aComposeBPrime.apply(aBPFile)
+
+      const bAPFile = new StringFileData(str, comments)
+      bComposeAPrime.apply(bAPFile)
+
+      expect(aBPFile.toRaw()).to.deep.equal(bAPFile.toRaw())
+      expect(aBPFile.trackedChanges.length).to.equal(1)
+      expect(
+        aBPFile.trackedChanges.asSorted()[0].tracking.ts.toISOString()
+      ).to.equal(ts1)
+    })
+
     it('adds a tracked change from operation 1', function () {
       expect(
         transform(

libraries/overleaf-editor-core/test/unit/tracked_change_list.test.js

@@ -8,7 +8,7 @@ const { expect } = require('chai')
 describe('TrackedChangeList', function () {
   describe('applyInsert', function () {
     describe('with same author', function () {
-      it('should merge consecutive tracked changes and use the latest timestamp', function () {
+      it('should merge consecutive tracked changes and use the earliest timestamp', function () {
         const trackedChanges = TrackedChangeList.fromRaw([
           {
             range: { pos: 0, length: 3 },
@@ -33,7 +33,7 @@ describe('TrackedChangeList', function () {
             tracking: {
               type: 'insert',
               userId: 'user1',
-              ts: '2024-01-01T00:00:00.000Z',
+              ts: '2023-01-01T00:00:00.000Z',
             },
           },
         ])
@@ -699,7 +699,7 @@ describe('TrackedChangeList', function () {
           tracking: {
             type: 'insert',
             userId: 'user1',
-            ts: '2024-01-01T00:00:00.000Z',
+            ts: '2023-01-01T00:00:00.000Z',
           },
         },
       ])

libraries/ranges-tracker/index.cjs

@@ -420,7 +420,7 @@ class RangesTracker {
           offset = opStart - changeStart
           change.op.i =
             change.op.i.slice(0, offset) + op.i + change.op.i.slice(offset)
-          change.metadata.ts = metadata.ts
+          change.metadata.ts = pickTimestamp(change.metadata, metadata)
           alreadyMerged = true
           movedChanges.push(change)
         } else if (opStart <= changeStart) {
@@ -728,6 +728,10 @@ class RangesTracker {
         ) {
           removeChanges.push(change)
           previousChange.op.i += change.op.i
+          previousChange.metadata.ts = pickTimestamp(
+            previousChange.metadata,
+            change.metadata
+          )
           movedChanges.push(previousChange)
         }
       } else if (
@@ -790,4 +794,15 @@ class RangesTracker {
   }
 }
 
+function pickTimestamp(oldMetadata, newMetadata) {
+  // Make sure null values don't get treated as 1970-01-01 dates in the
+  // comparison below.
+  if (oldMetadata.ts == null) return newMetadata.ts
+  if (newMetadata.ts == null) return oldMetadata.ts
+
+  return new Date(oldMetadata.ts) < new Date(newMetadata.ts)
+    ? oldMetadata.ts
+    : newMetadata.ts
+}
+
 module.exports = RangesTracker

libraries/ranges-tracker/test/unit/ranges-tracker-test.js

@@ -222,4 +222,114 @@ describe('RangesTracker', function () {
       )
     })
   })
+
+  describe('merging tracked inserts from the same user', function () {
+    beforeEach(function () {
+      this.comments = []
+    })
+
+    it('keeps the earlier timestamp when the existing insert is older', function () {
+      this.rangesTracker = new RangesTracker(
+        [
+          {
+            id: 'id1',
+            op: { p: 10, i: 'foo' },
+            metadata: { user_id: 'user-1', ts: '2024-01-01T00:00:00.000Z' },
+          },
+        ],
+        this.comments
+      )
+      this.rangesTracker.track_changes = true
+      this.rangesTracker.applyOp(
+        { p: 13, i: 'bar' },
+        { user_id: 'user-1', ts: new Date('2024-06-01T00:00:00.000Z') }
+      )
+      expect(this.rangesTracker.changes).to.have.length(1)
+      const [change] = this.rangesTracker.changes
+      expect(change.op).to.deep.equal({ p: 10, i: 'foobar' })
+      expect(change.metadata.user_id).to.equal('user-1')
+      expect(new Date(change.metadata.ts).toISOString()).to.equal(
+        '2024-01-01T00:00:00.000Z'
+      )
+    })
+
+    it('keeps the earlier timestamp when the incoming insert is older', function () {
+      this.rangesTracker = new RangesTracker(
+        [
+          {
+            id: 'id1',
+            op: { p: 10, i: 'foo' },
+            metadata: { user_id: 'user-1', ts: '2024-06-01T00:00:00.000Z' },
+          },
+        ],
+        this.comments
+      )
+      this.rangesTracker.track_changes = true
+      this.rangesTracker.applyOp(
+        { p: 13, i: 'bar' },
+        { user_id: 'user-1', ts: new Date('2024-01-01T00:00:00.000Z') }
+      )
+      expect(this.rangesTracker.changes).to.have.length(1)
+      const [change] = this.rangesTracker.changes
+      expect(change.op).to.deep.equal({ p: 10, i: 'foobar' })
+      expect(change.metadata.user_id).to.equal('user-1')
+      expect(new Date(change.metadata.ts).toISOString()).to.equal(
+        '2024-01-01T00:00:00.000Z'
+      )
+    })
+
+    it('uses the defined timestamp when the existing insert has a null timestamp', function () {
+      this.rangesTracker = new RangesTracker(
+        [
+          {
+            id: 'id1',
+            op: { p: 10, i: 'foo' },
+            metadata: { user_id: 'user-1', ts: null },
+          },
+        ],
+        this.comments
+      )
+      this.rangesTracker.track_changes = true
+      this.rangesTracker.applyOp(
+        { p: 13, i: 'bar' },
+        { user_id: 'user-1', ts: new Date('2024-01-01T00:00:00.000Z') }
+      )
+      expect(this.rangesTracker.changes).to.have.length(1)
+      const [change] = this.rangesTracker.changes
+      expect(new Date(change.metadata.ts).toISOString()).to.equal(
+        '2024-01-01T00:00:00.000Z'
+      )
+    })
+
+    it('keeps the earlier timestamp when re-merging inserts after a deletion', function () {
+      this.rangesTracker = new RangesTracker(
+        [
+          {
+            id: 'id1',
+            op: { p: 10, i: 'aaa' },
+            metadata: { user_id: 'user-1', ts: '2024-03-01T00:00:00.000Z' },
+          },
+          {
+            id: 'id2',
+            op: { p: 13, i: 'bb' },
+            metadata: { user_id: 'user-2', ts: '2024-02-01T00:00:00.000Z' },
+          },
+          {
+            id: 'id3',
+            op: { p: 15, i: 'ccc' },
+            metadata: { user_id: 'user-1', ts: '2024-01-01T00:00:00.000Z' },
+          },
+        ],
+        this.comments
+      )
+      this.rangesTracker.applyOp({ p: 13, d: 'bb' })
+      expect(this.rangesTracker.changes).to.have.length(1)
+      const [change] = this.rangesTracker.changes
+      expect(change.op).to.deep.equal({ p: 10, i: 'aaaccc' })
+      expect(change.metadata.user_id).to.equal('user-1')
+      expect(new Date(change.metadata.ts).toISOString()).to.equal(
+        '2024-01-01T00:00:00.000Z'
+      )
+    })
+  })
 })

services/project-history/test/unit/js/SyncManager/SyncManagerTests.js

@@ -2,7 +2,13 @@ import sinon from 'sinon'
 import { expect } from 'chai'
 import mongodb from 'mongodb-legacy'
 import tk from 'timekeeper'
-import { File, Comment, TrackedChange, Range } from 'overleaf-editor-core'
+import {
+  File,
+  Comment,
+  TrackedChange,
+  Range,
+  TrackingProps,
+} from 'overleaf-editor-core'
 import { UnprocessableError } from 'overleaf-editor-core/lib/errors.js'
 import { strict as esmock } from 'esmock'
 import { FileContentEmptyError } from '../../../../app/js/Errors.js'
@@ -2013,32 +2019,44 @@ describe('SyncManager', function () {
     describe('syncing tracked changes', function () {
       beforeEach(function () {
         this.loadedSnapshotDoc.getTrackedChanges().add(
-          new TrackedChange(new Range(4, 6), {
-            type: 'delete',
-            userId: USER_ID,
-            ts: new Date(TIMESTAMP),
-          })
+          new TrackedChange(
+            new Range(4, 6),
+            TrackingProps.fromRaw({
+              type: 'delete',
+              userId: USER_ID,
+              ts: new Date(TIMESTAMP).toISOString(),
+            })
+          )
         )
         this.loadedSnapshotDoc.getTrackedChanges().add(
-          new TrackedChange(new Range(10, 6), {
-            type: 'insert',
-            userId: USER_ID,
-            ts: new Date(TIMESTAMP),
-          })
+          new TrackedChange(
+            new Range(10, 6),
+            TrackingProps.fromRaw({
+              type: 'insert',
+              userId: USER_ID,
+              ts: new Date(TIMESTAMP).toISOString(),
+            })
+          )
         )
         this.loadedSnapshotDoc.getTrackedChanges().add(
-          new TrackedChange(new Range(20, 6), {
-            type: 'delete',
-            userId: USER_ID,
-            ts: new Date(TIMESTAMP),
-          })
+          new TrackedChange(
+            new Range(20, 6),
+            TrackingProps.fromRaw({
+              type: 'delete',
+              userId: USER_ID,
+              ts: new Date(TIMESTAMP).toISOString(),
+            })
+          )
         )
         this.loadedSnapshotDoc.getTrackedChanges().add(
-          new TrackedChange(new Range(40, 3), {
-            type: 'insert',
-            userId: USER_ID,
-            ts: new Date(TIMESTAMP),
-          })
+          new TrackedChange(
+            new Range(40, 3),
+            TrackingProps.fromRaw({
+              type: 'insert',
+              userId: USER_ID,
+              ts: new Date(TIMESTAMP).toISOString(),
+            })
+          )
         )
         this.changes = [
           makeTrackedChange('td1', { p: 4, d: 'quick ' }),