[project-history] fix convertToDiffUpdates when track delete is moved by previous op (#17681)

* [project-history] fix convertToDiffUpdates

* fix test

* preserve source ranges for applyRetain

* use sourceRanges for applyDelete

* use cursor difference for offset

* move statement to closer to original

* handle deletion before tracked delete

* Revert "handle deletion before tracked delete"

This reverts commit 6f2570f22473bd64516c166b29a34639ec701230.

* using resultCursor and sourceCursor

* refactor, use scanCursor

* skip track delete properly

* prettier

* remove .sort() in applyRetain

* small test fixes

GitOrigin-RevId: 94755c219c90d6fedcdc64284d71137cf56d2442

services/project-history/app/js/ChunkTranslator.js

@@ -447,32 +447,52 @@ class TextUpdateBuilder {
    * @param {RetainOp} retain
    */
   applyRetain(retain) {
-    const rangeOfRetention = new Range(this.sourceCursor, retain.length)
-    let cursor = this.sourceCursor
+    const resultRetentionRange = new Range(this.result.length, retain.length)
+    const sourceRetentionRange = new Range(this.sourceCursor, retain.length)
+
+    let scanCursor = this.result.length
     if (retain.tracking) {
       // We are modifying existing tracked deletes. We need to treat removal
       // (type insert/none) of a tracked delete as an insertion. Similarly, any
       // range we introduce as a tracked deletion must be reported as a deletion.
       const trackedDeletes = this.trackedChanges.trackedChanges.filter(
         tc =>
-          tc.tracking.type === 'delete' && tc.range.overlaps(rangeOfRetention)
+          tc.tracking.type === 'delete' &&
+          tc.range.overlaps(resultRetentionRange)
       )
+
       for (const trackedDelete of trackedDeletes) {
-        if (cursor < trackedDelete.range.start) {
+        const resultTrackedDelete = trackedDelete.range
+        const sourceTrackedDelete = trackedDelete.range.moveBy(
+          this.sourceCursor - this.result.length
+        )
+
+        if (scanCursor < resultTrackedDelete.start) {
           if (retain.tracking.type === 'delete') {
             this.changes.push({
-              d: this.source.slice(cursor, trackedDelete.range.start),
+              d: this.source.slice(
+                this.sourceCursor,
+                sourceTrackedDelete.start
+              ),
               p: this.result.length,
             })
           }
-          this.result += this.source.slice(cursor, trackedDelete.range.start)
-          cursor = trackedDelete.range.start
+          this.result += this.source.slice(
+            this.sourceCursor,
+            sourceTrackedDelete.start
+          )
+          scanCursor = resultTrackedDelete.start
+          this.sourceCursor = sourceTrackedDelete.start
         }
-        const endOfInsertion = Math.min(
-          trackedDelete.range.end,
-          rangeOfRetention.end
+        const endOfInsertionResult = Math.min(
+          resultTrackedDelete.end,
+          resultRetentionRange.end
+        )
+        const endOfInsertionSource = Math.min(
+          sourceTrackedDelete.end,
+          sourceRetentionRange.end
         )
-        const text = this.source.slice(cursor, endOfInsertion)
+        const text = this.source.slice(this.sourceCursor, endOfInsertionSource)
         if (
           retain.tracking.type === 'none' ||
           retain.tracking.type === 'insert'
@@ -483,16 +503,22 @@ class TextUpdateBuilder {
           })
         }
         this.result += text
-        cursor = endOfInsertion
-        if (cursor >= rangeOfRetention.end) {
+        // skip the tracked delete itself
+        scanCursor = endOfInsertionResult
+        this.sourceCursor = endOfInsertionSource
+
+        if (scanCursor >= resultRetentionRange.end) {
           break
         }
       }
     }
-    if (cursor < rangeOfRetention.end) {
+    if (scanCursor < resultRetentionRange.end) {
       // The last region is not a tracked delete. But we should still handle
       // a new tracked delete as a deletion.
-      const text = this.source.slice(cursor, rangeOfRetention.end)
+      const text = this.source.slice(
+        this.sourceCursor,
+        sourceRetentionRange.end
+      )
       if (retain.tracking?.type === 'delete') {
         this.changes.push({
           d: text,
@@ -501,7 +527,7 @@ class TextUpdateBuilder {
       }
       this.result += text
     }
-    this.sourceCursor += retain.length
+    this.sourceCursor = sourceRetentionRange.end
   }
 
   /**
@@ -525,34 +551,49 @@ class TextUpdateBuilder {
    * @param {RemoveOp} deletion
    */
   applyDelete(deletion) {
-    const rangeOfDeletion = new Range(this.sourceCursor, deletion.length)
+    const sourceDeletionRange = new Range(this.sourceCursor, deletion.length)
+    const resultDeletionRange = new Range(this.result.length, deletion.length)
+
     const trackedDeletes = this.trackedChanges.trackedChanges
       .filter(
         tc =>
-          tc.tracking.type === 'delete' && tc.range.overlaps(rangeOfDeletion)
+          tc.tracking.type === 'delete' &&
+          tc.range.overlaps(resultDeletionRange)
       )
       .sort((a, b) => a.range.start - b.range.start)
-    let cursor = this.sourceCursor
+
+    let scanCursor = this.result.length
+
     for (const trackedDelete of trackedDeletes) {
-      if (cursor < trackedDelete.range.start) {
+      const resultTrackDeleteRange = trackedDelete.range
+      const sourceTrackDeleteRange = trackedDelete.range.moveBy(
+        this.sourceCursor - this.result.length
+      )
+
+      if (scanCursor < resultTrackDeleteRange.start) {
         this.changes.push({
-          d: this.source.slice(cursor, trackedDelete.range.start),
+          d: this.source.slice(this.sourceCursor, sourceTrackDeleteRange.start),
           p: this.result.length,
         })
       }
       // skip the tracked delete itself
-      cursor = Math.min(trackedDelete.range.end, rangeOfDeletion.end)
-      if (cursor >= rangeOfDeletion.end) {
+      scanCursor = Math.min(resultTrackDeleteRange.end, resultDeletionRange.end)
+      this.sourceCursor = Math.min(
+        sourceTrackDeleteRange.end,
+        sourceDeletionRange.end
+      )
+
+      if (scanCursor >= resultDeletionRange.end) {
         break
       }
     }
-    if (cursor < rangeOfDeletion.end) {
+    if (scanCursor < resultDeletionRange.end) {
       this.changes.push({
-        d: this.source.slice(cursor, rangeOfDeletion.end),
+        d: this.source.slice(this.sourceCursor, sourceDeletionRange.end),
         p: this.result.length,
       })
     }
-    this.sourceCursor = rangeOfDeletion.end
+    this.sourceCursor = sourceDeletionRange.end
   }
 
   finish() {

services/project-history/test/unit/js/ChunkTranslator/ChunkTranslatorTests.js

@@ -2277,6 +2277,479 @@ describe('ChunkTranslator', function () {
           }
         )
       })
+
+      it('should properly create changes when deleting after moved track deletes', function (done) {
+        this.chunk = {
+          chunk: {
+            startVersion: 0,
+            history: {
+              snapshot: {
+                files: {
+                  'main.tex': {
+                    hash: this.fileHash,
+                    stringLength: 34,
+                  },
+                },
+              },
+              changes: [
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // // He[ll]o planet world, this is a test
+                        2,
+                        {
+                          r: 2,
+                          tracking: {
+                            type: 'delete',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                        30,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // {...} is a tracked insert
+                        // He[ll]o {TEST }planet world, this is a test
+                        6,
+                        {
+                          i: 'TEST ',
+                          tracking: {
+                            type: 'insert',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                        28,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // {...} is a tracked insert
+                        // He[ll]o {TEST }planet world, [this] is a test
+                        25,
+                        {
+                          r: 4,
+                          tracking: {
+                            type: 'delete',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                        10,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // {...} is a tracked insert
+                        2, // He|[ll]o {TEST }planet world, [this] is a test
+                        -2, // He|o {TEST }planet world, [this] is a test
+                        2, // Heo |{TEST }planet world, [this] is a test
+                        {
+                          r: 5,
+                          tracking: {
+                            type: 'none',
+                            userId: '65f1baeb3cee7c3563fbd1d1',
+                            ts: '2024-03-27T13:20:23.253Z',
+                          },
+                        }, // Heo TEST| planet world, [this] is a test
+                        14, // Heo TEST planet world, |[this] is a test
+                        -4, // Heo TEST planet world, | is a test
+                        10, // Heo TEST planet world,  is a test|
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+              ],
+            },
+          },
+          authors: [this.author1.id],
+        }
+
+        this.ChunkTranslator.convertToDiffUpdates(
+          this.projectId,
+          this.chunk,
+          'main.tex',
+          0,
+          4,
+          (error, diff) => {
+            expect(error).to.be.null
+            expect(diff.updates).to.deep.equal([
+              {
+                op: [
+                  {
+                    d: 'll',
+                    p: 2,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 0,
+              },
+              {
+                op: [
+                  {
+                    i: 'TEST ',
+                    p: 4,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 1,
+              },
+              {
+                op: [
+                  {
+                    d: 'this',
+                    p: 23,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 2,
+              },
+              {
+                op: [],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 3,
+              },
+            ])
+            done()
+          }
+        )
+      })
+
+      it('should properly create changes when retaining after moved track deletes', function (done) {
+        this.chunk = {
+          chunk: {
+            startVersion: 0,
+            history: {
+              snapshot: {
+                files: {
+                  'main.tex': {
+                    hash: this.fileHash,
+                    stringLength: 34,
+                  },
+                },
+              },
+              changes: [
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // // He[ll]o planet world, this is a test
+                        2,
+                        {
+                          r: 2,
+                          tracking: {
+                            type: 'delete',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                        30,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // He[ll]o planet world, [this] is a test
+                        20,
+                        {
+                          r: 4,
+                          tracking: {
+                            type: 'delete',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                        10,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // {...} is a tracked insert
+                        // He[ll]o planet world, [this] {TEST }is a test
+                        25,
+                        {
+                          i: 'TEST ',
+                          tracking: {
+                            type: 'insert',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                        9,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // {...} is a tracked insert
+                        2, // He|[ll]o planet world, [this] {TEST }is a test
+                        -2, // He|o planet world, [this] {TEST }is a test
+                        {
+                          r: 39,
+                          tracking: {
+                            type: 'none',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                      ], // He|o planet world, this TEST is a test
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+              ],
+            },
+          },
+          authors: [this.author1.id],
+        }
+
+        this.ChunkTranslator.convertToDiffUpdates(
+          this.projectId,
+          this.chunk,
+          'main.tex',
+          0,
+          4,
+          (error, diff) => {
+            expect(error).to.be.null
+            expect(diff.updates).to.deep.equal([
+              {
+                op: [
+                  {
+                    d: 'll',
+                    p: 2,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 0,
+              },
+              {
+                op: [
+                  {
+                    d: 'this',
+                    p: 18,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 1,
+              },
+              {
+                op: [
+                  {
+                    i: 'TEST ',
+                    p: 19,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 2,
+              },
+              {
+                op: [
+                  {
+                    i: 'this',
+                    p: 18,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 3,
+              },
+            ])
+            done()
+          }
+        )
+      })
+
+      it('should handle deletion that starts before tracked delete', function (done) {
+        this.chunk = {
+          chunk: {
+            startVersion: 0,
+            history: {
+              snapshot: {
+                files: {
+                  'main.tex': {
+                    hash: this.fileHash,
+                    stringLength: 34,
+                  },
+                },
+              },
+              changes: [
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        // Hello planet world, [this] is a test
+                        20,
+                        {
+                          r: 4,
+                          tracking: {
+                            type: 'delete',
+                            userId: this.author1.id,
+                            ts: this.date.toISOString(),
+                          },
+                        },
+                        10,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+                {
+                  operations: [
+                    {
+                      pathname: 'main.tex',
+                      textOperation: [
+                        // [...] is a tracked delete
+                        5, // Hello| planet world, [this] is a test
+                        -25, // Hellotest
+                        4,
+                      ],
+                    },
+                  ],
+                  timestamp: this.date.toISOString(),
+                  authors: [this.author1.id],
+                },
+              ],
+            },
+          },
+          authors: [this.author1.id],
+        }
+
+        this.ChunkTranslator.convertToDiffUpdates(
+          this.projectId,
+          this.chunk,
+          'main.tex',
+          0,
+          2,
+          (error, diff) => {
+            expect(error).to.be.null
+            expect(diff.updates).to.deep.equal([
+              {
+                op: [
+                  {
+                    d: 'this',
+                    p: 20,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 0,
+              },
+              {
+                op: [
+                  {
+                    d: ' planet world, ',
+                    p: 5,
+                  },
+                  {
+                    d: ' is a ',
+                    p: 5,
+                  },
+                ],
+                meta: {
+                  users: [this.author1.id],
+                  start_ts: this.date.getTime(),
+                  end_ts: this.date.getTime(),
+                },
+                v: 1,
+              },
+            ])
+            done()
+          }
+        )
+      })
     })
   })
 })