diff --git a/services/project-history/app/js/HistoryBlobTranslator.js b/services/project-history/app/js/HistoryBlobTranslator.js index 52092f06a4..4a6515a6ae 100644 --- a/services/project-history/app/js/HistoryBlobTranslator.js +++ b/services/project-history/app/js/HistoryBlobTranslator.js @@ -91,10 +91,6 @@ export function createRangeBlobDataFromUpdate(update) { /** @type {Map} */ const commentMap = new Map() for (const comment of comments) { - const range = new Range( - comment.op.hpos ?? comment.op.p, - comment.op.hlen ?? comment.op.c.length - ) const id = comment.op.t if (!commentMap.has(id)) { commentMap.set(id, { @@ -109,7 +105,13 @@ export function createRangeBlobDataFromUpdate(update) { if (entry.resolved !== (comment.op.resolved ?? false)) { throw new Error('Mismatching resolved status for comment') } - entry.ranges.push(range) + + const commentLength = comment.op.c.length + if (commentLength > 0) { + // Empty comments in operations are translated to detached comments + const range = new Range(comment.op.hpos ?? comment.op.p, commentLength) + entry.ranges.push(range) + } } const commentList = new CommentList( [...commentMap.entries()].map( diff --git a/services/project-history/app/js/SnapshotManager.js b/services/project-history/app/js/SnapshotManager.js index 6b9aea9cfa..f7d743864c 100644 --- a/services/project-history/app/js/SnapshotManager.js +++ b/services/project-history/app/js/SnapshotManager.js @@ -126,6 +126,18 @@ async function getRangesSnapshot(projectId, version, pathname) { for (const comment of comments) { trackedDeletionOffset = 0 let trackedDeletionIndex = 0 + if (comment.ranges.length === 0) { + // Translate detached comments into zero length comments at position 0 + docUpdaterCompatibleComments.push({ + op: { + p: 0, + c: '', + t: comment.id, + resolved: comment.resolved, + }, + }) + continue + } for (const commentRange of comment.ranges) { let commentRangeContent = '' let offsetFromOverlappingRangeAtStart = 0 diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index 8ddc796c03..f16474b802 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -906,13 +906,18 @@ class SyncUpdateExpander { * @param {Comment} expectedComment */ function commentRangesAreInSync(persistedComment, expectedComment) { + const expectedPos = expectedComment.op.hpos ?? expectedComment.op.p + const expectedLength = expectedComment.op.hlen ?? expectedComment.op.c.length + if (expectedLength === 0) { + // A zero length comment from RangesManager is a detached comment in history + return persistedComment.ranges.length === 0 + } + if (persistedComment.ranges.length !== 1) { // The editor only supports single range comments return false } const persistedRange = persistedComment.ranges[0] - const expectedPos = expectedComment.op.hpos ?? expectedComment.op.p - const expectedLength = expectedComment.op.hlen ?? expectedComment.op.c.length return ( persistedRange.pos === expectedPos && persistedRange.length === expectedLength diff --git a/services/project-history/test/unit/js/HistoryBlobTranslator/HistoryBlobTranslatorTests.js b/services/project-history/test/unit/js/HistoryBlobTranslator/HistoryBlobTranslatorTests.js index d3d691794f..e94f6bb02a 100644 --- a/services/project-history/test/unit/js/HistoryBlobTranslator/HistoryBlobTranslatorTests.js +++ b/services/project-history/test/unit/js/HistoryBlobTranslator/HistoryBlobTranslatorTests.js @@ -66,6 +66,25 @@ describe('HistoryBlobTranslator', function () { }) }) + describe('for update with zero length comments', function () { + beforeEach(function () { + this.result = createRangeBlobDataFromUpdate( + update('pathname', this.text, { + changes: [], + comments: [ + { op: { c: '', p: 4, t: 'comment-1', resolved: false } }, + ], + }) + ) + }) + it('should treat them as detached comments', function () { + expect(this.result).to.deep.equal({ + comments: [{ id: 'comment-1', ranges: [] }], + trackedChanges: [], + }) + }) + }) + describe('for update with ranges object with only comments', function () { it('should return unmoved ranges', function () { const result = createRangeBlobDataFromUpdate( diff --git a/services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js b/services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js index 8f3f5ccdf8..69891b604b 100644 --- a/services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js +++ b/services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js @@ -757,6 +757,7 @@ Four five six\ ], resolved: false, }, + { id: 'comment-2', ranges: [], resolved: true }, ], }) this.data = await this.SnapshotManager.promises.getRangesSnapshot( @@ -785,6 +786,19 @@ Four five six\ expect(this.data.comments[0].op.resolved).to.be.false expect(this.data.comments[1].op.resolved).to.be.false expect(this.data.comments[2].op.resolved).to.be.false + expect(this.data.comments[3].op.resolved).to.be.true + }) + + it('should include thread id', function () { + expect(this.data.comments[0].op.t).to.eq('comment-1') + expect(this.data.comments[1].op.t).to.eq('comment-1') + expect(this.data.comments[2].op.t).to.eq('comment-1') + expect(this.data.comments[3].op.t).to.eq('comment-2') + }) + + it('should translated detached comment to zero length op', function () { + expect(this.data.comments[3].op.p).to.eq(0) + expect(this.data.comments[3].op.c).to.eq('') }) }) diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index 4fafd1f2a8..215a406e74 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -1286,6 +1286,35 @@ describe('SyncManager', function () { }, ]) }) + + it('treats zero length comments as detached comments', async function () { + this.loadedSnapshotDoc.getComments.returns({ + toArray: sinon.stub().returns([new Comment('comment1', [])]), + }) + this.comments = [makeComment('comment1', 16, '')] + this.resolvedCommentIds = [] + + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + this.persistedDocContent, + { + comments: this.comments, + }, + this.resolvedCommentIds + ), + ] + + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + + expect(expandedUpdates).to.deep.equal([]) + }) }) describe('syncing tracked changes', function () {