diff --git a/services/project-history/app/js/ChunkTranslator.js b/services/project-history/app/js/ChunkTranslator.js index 544db19ef8..c98496fc7e 100644 --- a/services/project-history/app/js/ChunkTranslator.js +++ b/services/project-history/app/js/ChunkTranslator.js @@ -470,8 +470,20 @@ class TextUpdateBuilder { const sourceOffset = this.sourceCursor - this.result.length for (const trackedDelete of trackedDeletes) { - const resultTrackedDelete = trackedDelete.range - const sourceTrackedDelete = trackedDelete.range.moveBy(sourceOffset) + // Clamp the tracked delete to the retention range start. A prior + // insert (tracked as delete) can merge with an existing tracked + // delete, extending it before the retention range. That part has no + // source equivalent, so mapping it with sourceOffset would produce a + // negative position. + const clampedStart = Math.max( + trackedDelete.range.start, + resultRetentionRange.start + ) + const resultTrackedDelete = new Range( + clampedStart, + trackedDelete.range.end - clampedStart + ) + const sourceTrackedDelete = resultTrackedDelete.moveBy(sourceOffset) if (scanCursor < resultTrackedDelete.start) { if (retain.tracking.type === 'delete') { @@ -573,8 +585,17 @@ class TextUpdateBuilder { const sourceOffset = this.sourceCursor - this.result.length for (const trackedDelete of trackedDeletes) { - const resultTrackDeleteRange = trackedDelete.range - const sourceTrackDeleteRange = trackedDelete.range.moveBy(sourceOffset) + // Clamp the tracked delete to the deletion range start (see applyRetain + // for the full explanation). + const clampedStart = Math.max( + trackedDelete.range.start, + resultDeletionRange.start + ) + const resultTrackDeleteRange = new Range( + clampedStart, + trackedDelete.range.end - clampedStart + ) + const sourceTrackDeleteRange = resultTrackDeleteRange.moveBy(sourceOffset) if (scanCursor < resultTrackDeleteRange.start) { this.changes.push({ diff --git a/services/project-history/test/unit/js/ChunkTranslator/ChunkTranslatorTests.js b/services/project-history/test/unit/js/ChunkTranslator/ChunkTranslatorTests.js index e4b32c4c02..0782406ca5 100644 --- a/services/project-history/test/unit/js/ChunkTranslator/ChunkTranslatorTests.js +++ b/services/project-history/test/unit/js/ChunkTranslator/ChunkTranslatorTests.js @@ -3045,6 +3045,98 @@ describe('ChunkTranslator', function () { ) }) }) + + it('should handle tracked delete insert that merges with existing tracked delete before retain with none tracking', function (done) { + // Bug reproduction: when an insert with tracking type 'delete' is + // placed at the start of an existing tracked delete (same userId), + // the two tracked deletes merge. A subsequent retain with 'none' + // tracking then encounters the merged range. The sourceOffset + // (sourceCursor - result.length) is negative due to the insert + // advancing result.length without advancing sourceCursor. When + // moveBy(sourceOffset) is called on the merged range, the position + // becomes negative, triggering "OError: Invalid range". + this.fileContents = 'Hello world' + this.ranges = JSON.stringify({ + trackedChanges: [ + { + range: { pos: 0, length: 11 }, + tracking: { + type: 'delete', + userId: this.author1.id, + ts: '2024-01-01T00:00:00.000Z', + }, + }, + ], + }) + this.HistoryStoreManager.getProjectBlob + .withArgs(this.historyId, this.rangesHash) + .yields(null, this.ranges) + this.HistoryStoreManager.getProjectBlob + .withArgs(this.historyId, this.fileHash) + .yields(null, this.fileContents) + + this.chunk = { + chunk: { + startVersion: 0, + history: { + snapshot: { + files: { + 'main.tex': { + hash: this.fileHash, + rangesHash: this.rangesHash, + stringLength: this.fileContents.length, + }, + }, + }, + changes: [ + { + operations: [ + { + pathname: 'main.tex', + textOperation: [ + // Initial state: [Hello world] (all tracked-deleted) + // Insert "X" tracked as delete at position 0, + // then retain all 11 chars clearing tracking + { + i: 'X', + tracking: { + type: 'delete', + userId: this.author1.id, + ts: '2024-01-01T00:00:00.000Z', + }, + }, + { + r: 11, + tracking: { type: 'none' }, + }, + ], + }, + ], + timestamp: this.date.toISOString(), + authors: [this.author1.id], + }, + ], + }, + }, + authors: [this.author1.id], + } + + this.ChunkTranslator.convertToDiffUpdates( + this.projectId, + this.chunk, + 'main.tex', + 0, + 1, + (error, diff) => { + expect(error).to.be.null + expect(diff.updates.length).to.equal(1) + expect(diff.updates[0].op).to.deep.equal([ + { i: 'Hello world', p: 0 }, + ]) + done() + } + ) + }) }) }) })