From 9f0f42a0125d26fb8c8a60bb399b4d3f7c741b34 Mon Sep 17 00:00:00 2001 From: Eric Mc Sween <5454374+emcsween@users.noreply.github.com> Date: Tue, 18 Jun 2024 09:59:44 -0400 Subject: [PATCH] Merge pull request #18930 from overleaf/em-resync-ranges Fix resyncs when diffs move ranges GitOrigin-RevId: 121c3a16cf19649538445e6ed8bc0a1129735eb9 --- .../project-history/app/js/SyncManager.js | 21 +- services/project-history/app/js/types.ts | 14 +- .../unit/js/SyncManager/SyncManagerTests.js | 209 +++++++++++++----- 3 files changed, 180 insertions(+), 64 deletions(-) diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index f16474b802..6772096611 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -7,7 +7,7 @@ import Settings from '@overleaf/settings' import logger from '@overleaf/logger' import Metrics from '@overleaf/metrics' import OError from '@overleaf/o-error' -import { File } from 'overleaf-editor-core' +import { File, Range } from 'overleaf-editor-core' import { SyncError } from './Errors.js' import { db, ObjectId } from './mongodb.js' import * as SnapshotManager from './SnapshotManager.js' @@ -19,7 +19,7 @@ import * as ErrorRecorder from './ErrorRecorder.js' import * as RedisManager from './RedisManager.js' import * as HistoryStoreManager from './HistoryStoreManager.js' import * as HashManager from './HashManager.js' -import { isInsert } from './Utils.js' +import { isInsert, isDelete } from './Utils.js' /** * @typedef {import('overleaf-editor-core').Comment} HistoryComment @@ -637,12 +637,24 @@ class SyncUpdateExpander { } if (!hashesMatch) { - await this.queueUpdateForOutOfSyncContent( + const expandedUpdate = await this.queueUpdateForOutOfSyncContent( update, pathname, persistedContent, expectedContent ) + if (expandedUpdate != null) { + // Adjust the ranges for the changes that have been made to the content + for (const op of expandedUpdate.op) { + if (isInsert(op)) { + file.getComments().applyInsert(new Range(op.p, op.i.length)) + file.getTrackedChanges().applyInsert(op.p, op.i) + } else if (isDelete(op)) { + file.getComments().applyDelete(new Range(op.p, op.d.length)) + file.getTrackedChanges().applyDelete(op.p, op.d.length) + } + } + } } const persistedComments = file.getComments().toArray() @@ -683,7 +695,7 @@ class SyncUpdateExpander { expectedContent ) if (op.length === 0) { - return + return null } const expandedUpdate = { doc: update.doc, @@ -704,6 +716,7 @@ class SyncUpdateExpander { Metrics.inc('project_history_resync_operation', 1, { status: 'update text file contents', }) + return expandedUpdate } /** diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index 363864c6ba..1cc44e3363 100644 --- a/services/project-history/app/js/types.ts +++ b/services/project-history/app/js/types.ts @@ -135,12 +135,14 @@ export type CommentOp = { resolved?: boolean } -export type UpdateWithBlob = { - update: Update - blobHashes: { - file: string - ranges?: string - } +export type UpdateWithBlob = { + update: T + blobHashes: T extends AddDocUpdate | AddFileUpdate + ? { + file: string + ranges?: string + } + : never } export type RawOrigin = { diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index 215a406e74..8a5edf809b 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -2,7 +2,7 @@ import sinon from 'sinon' import { expect } from 'chai' import mongodb from 'mongodb-legacy' import tk from 'timekeeper' -import { Comment, TrackedChange, Range } from 'overleaf-editor-core' +import { File, Comment, TrackedChange, Range } from 'overleaf-editor-core' import { strict as esmock } from 'esmock' const { ObjectId } = mongodb @@ -491,17 +491,7 @@ describe('SyncManager', function () { path: '1.png', _hash: 'abcde', } - this.loadedSnapshotDoc = { - isEditable: sinon.stub().returns(true), - getContent: sinon.stub().returns(this.persistedDocContent), - getComments: sinon - .stub() - .returns({ toArray: sinon.stub().returns([]) }), - getTrackedChanges: sinon - .stub() - .returns({ asSorted: sinon.stub().returns([]) }), - getHash: sinon.stub().returns(null), - } + this.loadedSnapshotDoc = File.fromString(this.persistedDocContent) this.fileMap = { 'main.tex': { isEditable: sinon.stub().returns(true), @@ -1050,10 +1040,11 @@ describe('SyncManager', function () { [this.persistedDoc], [this.persistedFile] ), - docContentSyncUpdate(this.persistedDoc, 'a'), + docContentSyncUpdate(this.persistedDoc, 'the quick brown fox'), ] - this.loadedSnapshotDoc.getContent.returns('stored content') - this.UpdateCompressor.diffAsShareJsOps.returns([{ d: 'sdf', p: 1 }]) + this.UpdateCompressor.diffAsShareJsOps.returns([ + { d: ' jumps over the lazy dog', p: 19 }, + ]) this.expandedUpdates = await this.SyncManager.promises.expandSyncUpdates( this.projectId, @@ -1067,10 +1058,10 @@ describe('SyncManager', function () { expect(this.expandedUpdates).to.deep.equal([ { doc: this.persistedDoc.doc, - op: [{ d: 'sdf', p: 1 }], + op: [{ d: ' jumps over the lazy dog', p: 19 }], meta: { pathname: this.persistedDoc.path, - doc_length: 'stored content'.length, + doc_length: this.persistedDocContent.length, resync: true, ts: TIMESTAMP, origin: { kind: 'history-resync' }, @@ -1084,14 +1075,12 @@ describe('SyncManager', function () { describe('syncing comments', function () { beforeEach(function () { - this.loadedSnapshotDoc.getComments.returns({ - toArray: sinon - .stub() - .returns([ - new Comment('comment1', [new Range(4, 5)]), - new Comment('comment2', [new Range(10, 5)], true), - ]), - }) + this.loadedSnapshotDoc + .getComments() + .add(new Comment('comment1', [new Range(4, 5)])) + this.loadedSnapshotDoc + .getComments() + .add(new Comment('comment2', [new Range(10, 5)], true)) this.comments = [ makeComment('comment1', 4, 'quick'), makeComment('comment2', 10, 'brown'), @@ -1288,11 +1277,12 @@ 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 = [] + this.loadedSnapshotDoc.getComments().add(new Comment('comment1', [])) + this.comments = [ + makeComment('comment1', 16, ''), + makeComment('comment2', 10, 'brown'), + ] + this.resolvedCommentIds = ['comment2'] const updates = [ docContentSyncUpdate( @@ -1315,34 +1305,101 @@ describe('SyncManager', function () { expect(expandedUpdates).to.deep.equal([]) }) + + it('adjusts comment positions when the underlying text has changed', async function () { + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + 'quick brown fox', + { + comments: [ + makeComment('comment1', 0, 'quick'), + makeComment('comment2', 12, 'fox'), + ], + }, + this.resolvedCommentIds + ), + ] + this.UpdateCompressor.diffAsShareJsOps.returns([ + { d: 'the ', p: 0 }, + { d: ' jumps over the lazy dog', p: 15 }, + ]) + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + doc: this.persistedDoc.doc, + op: [ + { d: 'the ', p: 0 }, + { d: ' jumps over the lazy dog', p: 15 }, + ], + meta: { + pathname: this.persistedDoc.path, + doc_length: this.persistedDocContent.length, + resync: true, + ts: TIMESTAMP, + origin: { kind: 'history-resync' }, + }, + }, + { + doc: this.persistedDoc.doc, + op: [ + { + c: 'fox', + p: 12, + t: 'comment2', + resolved: true, + }, + ], + meta: { + origin: { + kind: 'history-resync', + }, + pathname: this.persistedDoc.path, + resync: true, + ts: TIMESTAMP, + doc_length: 'quick brown fox'.length, + }, + }, + ]) + }) }) describe('syncing tracked changes', function () { beforeEach(function () { - this.loadedSnapshotDoc.getTrackedChanges.returns({ - asSorted: sinon.stub().returns([ - new TrackedChange(new Range(4, 6), { - type: 'delete', - userId: USER_ID, - ts: new Date(TIMESTAMP), - }), - new TrackedChange(new Range(10, 6), { - type: 'insert', - userId: USER_ID, - ts: new Date(TIMESTAMP), - }), - new TrackedChange(new Range(20, 6), { - type: 'delete', - userId: USER_ID, - ts: new Date(TIMESTAMP), - }), - new TrackedChange(new Range(40, 3), { - type: 'insert', - userId: USER_ID, - ts: new Date(TIMESTAMP), - }), - ]), - }) + this.loadedSnapshotDoc.getTrackedChanges().add( + new TrackedChange(new Range(4, 6), { + type: 'delete', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }) + ) + this.loadedSnapshotDoc.getTrackedChanges().add( + new TrackedChange(new Range(10, 6), { + type: 'insert', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }) + ) + this.loadedSnapshotDoc.getTrackedChanges().add( + new TrackedChange(new Range(20, 6), { + type: 'delete', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }) + ) + this.loadedSnapshotDoc.getTrackedChanges().add( + new TrackedChange(new Range(40, 3), { + type: 'insert', + userId: USER_ID, + ts: new Date(TIMESTAMP), + }) + ) this.changes = [ makeTrackedChange('td1', { p: 4, d: 'quick ' }), makeTrackedChange('ti1', { p: 4, hpos: 10, i: 'brown ' }), @@ -1515,6 +1572,50 @@ describe('SyncManager', function () { }, ]) }) + + it('adjusts tracked change positions when the underlying text has changed', async function () { + const updates = [ + docContentSyncUpdate( + this.persistedDoc, + 'every fox jumps over the lazy dog', + { + changes: [ + makeTrackedChange('ti1', { p: 5, i: ' ' }), // the space after every is still a tracked insert + makeTrackedChange('td2', { p: 10, d: 'jumps ' }), + makeTrackedChange('ti2', { p: 24, hpos: 30, i: 'dog' }), + ], + }, + this.resolvedCommentIds + ), + ] + this.UpdateCompressor.diffAsShareJsOps.returns([ + { d: 'the quick brown', p: 0 }, + { i: 'every', p: 0 }, + ]) + const expandedUpdates = + await this.SyncManager.promises.expandSyncUpdates( + this.projectId, + this.historyId, + updates, + this.extendLock + ) + expect(expandedUpdates).to.deep.equal([ + { + doc: this.persistedDoc.doc, + op: [ + { d: 'the quick brown', p: 0 }, + { i: 'every', p: 0 }, + ], + meta: { + pathname: this.persistedDoc.path, + doc_length: this.persistedDocContent.length, + resync: true, + ts: TIMESTAMP, + origin: { kind: 'history-resync' }, + }, + }, + ]) + }) }) }) })