From 863e33af78440e7d7bc31da965370b5dccf3358a Mon Sep 17 00:00:00 2001 From: Malik Glossop Date: Mon, 24 Aug 2026 12:53:08 +0200 Subject: [PATCH] Merge pull request #35982 from overleaf/mg-history-ot-comments HistoryOT - Fix comments being dropped and crashing on detached ranges GitOrigin-RevId: 0906a55a51a6fda9ac828efcbe72989573161261 --- .../operation/edit_operation_transformer.js | 19 + .../test/unit/edit_operation.test.js | 30 ++ .../app/js/HistoryOTUpdateManager.js | 20 +- .../HistoryOTUpdateManagerTests.js | 340 ++++++++++++++++++ .../review-panel/utils/snapshot-ranges.ts | 9 +- .../source-editor/extensions/history-ot.ts | 22 +- .../source-editor/extensions/realtime.ts | 119 ++++-- .../utils/snapshot-ranges.test.ts | 88 +++++ .../extensions/history-ot.test.ts | 22 ++ .../source-editor/extensions/realtime.test.ts | 183 ++++++++++ 10 files changed, 804 insertions(+), 48 deletions(-) create mode 100644 services/document-updater/test/unit/js/HistoryOTUpdateManager/HistoryOTUpdateManagerTests.js create mode 100644 services/web/test/frontend/features/review-panel/utils/snapshot-ranges.test.ts diff --git a/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js b/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js index c82393c6a2..2ac89ba75a 100644 --- a/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js +++ b/libraries/overleaf-editor-core/lib/operation/edit_operation_transformer.js @@ -126,6 +126,25 @@ class EditOperationTransformer { `Transform not implemented for ${a.constructor.name}￮${b.constructor.name}` ) } + + /** + * Transform each operation in `as` against each operation in `bs`, priming + * both lists in place. + * @param {EditOperation[]} as - modified in place + * @param {EditOperation[]} bs - modified in place + */ + static transformMultiple(as, bs) { + for (let i = 0; i < as.length; ++i) { + for (let j = 0; j < bs.length; ++j) { + const [aPrime, bPrime] = EditOperationTransformer.transform( + as[i], + bs[j] + ) + as[i] = aPrime + bs[j] = bPrime + } + } + } } /** diff --git a/libraries/overleaf-editor-core/test/unit/edit_operation.test.js b/libraries/overleaf-editor-core/test/unit/edit_operation.test.js index d24ab0b693..c840b2c8e4 100644 --- a/libraries/overleaf-editor-core/test/unit/edit_operation.test.js +++ b/libraries/overleaf-editor-core/test/unit/edit_operation.test.js @@ -10,6 +10,7 @@ const DeleteCommentOperation = require('../../lib/operation/delete_comment_opera const SetCommentStateOperation = require('../../lib/operation/set_comment_state_operation') const Range = require('../../lib/range') const EditNoOperation = require('../../lib/operation/edit_no_operation') +const StringFileData = require('../../lib/file_data/string_file_data') describe('EditOperation', function () { it('Cannot be instantiated', function () { @@ -246,6 +247,35 @@ describe('EditOperationTransformer', function () { expect(bPrime).to.be.an.instanceof(SetCommentStateOperation) expect(bPrime.toJSON()).to.deep.eql(b.toJSON()) }) + + it('transformMultiple rebases two concurrent op lists so they converge', function () { + const base = 'AB' + // two sequential client edits, inserting at the start + const as = [ + new TextOperation().insert('x').retain(2), + new TextOperation().insert('y').retain(3), + ] + // two sequential server edits, inserting at the end + const bs = [ + new TextOperation().retain(2).insert('1'), + new TextOperation().retain(3).insert('2'), + ] + const asOrig = as.map(op => EditOperationBuilder.fromJSON(op.toJSON())) + const bsOrig = bs.map(op => EditOperationBuilder.fromJSON(op.toJSON())) + + EditOperationTransformer.transformMultiple(as, bs) + + const applySeq = ops => { + const file = new StringFileData(base) + for (const op of ops) { + file.edit(op) + } + return file.getContent() + } + // client edits then rebased server edits, and the mirror order, both converge + expect(applySeq([...asOrig, ...bs])).to.equal('yxAB12') + expect(applySeq([...bsOrig, ...as])).to.equal('yxAB12') + }) }) describe('EditOperationBuilder', function () { diff --git a/services/document-updater/app/js/HistoryOTUpdateManager.js b/services/document-updater/app/js/HistoryOTUpdateManager.js index 76ccc2bf90..2324f440ac 100644 --- a/services/document-updater/app/js/HistoryOTUpdateManager.js +++ b/services/document-updater/app/js/HistoryOTUpdateManager.js @@ -30,7 +30,8 @@ function isHistoryOTEditOperationUpdate(update) { 'op' in update && 'v' in update && Array.isArray(update.op) && - EditOperationBuilder.isValid(update.op[0]) + update.op.length > 0 && + update.op.every(op => EditOperationBuilder.isValid(op)) ) } @@ -54,7 +55,10 @@ async function tryApplyUpdate(projectId, docId, update, profiler) { throw new Errors.OTTypeMismatchError(type, 'history-ot') } - let op = EditOperationBuilder.fromJSON(update.op[0]) + // An update carries a list of sequential ops (e.g. a text edit composed + // with a comment op). Keep the sequential apply in step with the frontend's + // historyOTType.apply and HistoryOTAdapter.onRemoteOp. + let ops = update.op.map(EditOperationBuilder.fromJSON) if (version !== update.v) { const transformUpdates = await RedisManager.promises.getPreviousDocOps( docId, @@ -73,15 +77,19 @@ async function tryApplyUpdate(projectId, docId, update, profiler) { update.dup = true break } - const other = EditOperationBuilder.fromJSON(transformUpdate.op[0]) - op = EditOperationTransformer.transform(op, other)[0] + + // Rebase our ops onto the concurrent update. + const transformOps = transformUpdate.op.map(EditOperationBuilder.fromJSON) + EditOperationTransformer.transformMultiple(ops, transformOps) } - update.op = [op.toJSON()] + update.op = ops.map(op => op.toJSON()) } if (!update.dup) { const file = StringFileData.fromRaw(lines) - file.edit(op) + for (const op of ops) { + file.edit(op) + } version += 1 update.meta.ts = Date.now() await RedisManager.promises.updateDocument( diff --git a/services/document-updater/test/unit/js/HistoryOTUpdateManager/HistoryOTUpdateManagerTests.js b/services/document-updater/test/unit/js/HistoryOTUpdateManager/HistoryOTUpdateManagerTests.js new file mode 100644 index 0000000000..0333c1f3f0 --- /dev/null +++ b/services/document-updater/test/unit/js/HistoryOTUpdateManager/HistoryOTUpdateManagerTests.js @@ -0,0 +1,340 @@ +const sinon = require('sinon') +const { expect } = require('chai') +const SandboxedModule = require('sandboxed-module') +const { + StringFileData, + AddCommentOperation, + Range, + TextOperation, +} = require('overleaf-editor-core') + +const MODULE_PATH = '../../../../app/js/HistoryOTUpdateManager.js' + +describe('HistoryOTUpdateManager', function () { + beforeEach(function () { + this.project_id = 'project-id-123' + this.doc_id = 'document-id-123' + this.pathname = '/a/b/c.tex' + + this.Metrics = { inc: sinon.stub() } + + this.Profiler = class Profiler {} + this.Profiler.prototype.log = sinon.stub().returns({ end: sinon.stub() }) + this.Profiler.prototype.end = sinon.stub() + + this.DocumentManager = { + promises: { getDoc: sinon.stub() }, + } + + this.RedisManager = { + promises: { + updateDocument: sinon.stub().resolves(), + getPreviousDocOps: sinon.stub().resolves([]), + recordProjectNotificationTimestamp: sinon.stub().resolves(), + }, + } + + this.ProjectHistoryRedisManager = { + promises: { queueOps: sinon.stub().resolves(1) }, + } + + this.HistoryManager = { recordAndFlushHistoryOps: sinon.stub() } + + this.RealTimeRedisManager = { sendData: sinon.stub() } + + this.HistoryOTUpdateManager = SandboxedModule.require(MODULE_PATH, { + requires: { + './Profiler': this.Profiler, + './DocumentManager': this.DocumentManager, + './RedisManager': this.RedisManager, + './Metrics': this.Metrics, + './ProjectHistoryRedisManager': this.ProjectHistoryRedisManager, + './HistoryManager': this.HistoryManager, + './RealTimeRedisManager': this.RealTimeRedisManager, + }, + }) + }) + + const savedFile = redisManager => { + // updateDocument(projectId, docId, file.toRaw(), version, ...) + const raw = redisManager.promises.updateDocument.firstCall.args[2] + return StringFileData.fromRaw(raw) + } + + describe('tryApplyUpdate with a composed multi-op update', function () { + it('applies every op in the array, not just the first', async function () { + // Doc after a cut: " world" with comment "c1" detached (empty ranges) + const doc = new StringFileData('hello world') + new AddCommentOperation('c1', [new Range(0, 5)]).apply(doc) + const cut = new TextOperation() + cut.remove(5) + cut.retain(6) + doc.edit(cut) + expect(doc.getComments().getComment('c1').isEmpty()).to.be.true + + this.DocumentManager.promises.getDoc.resolves({ + lines: doc.toRaw(), + version: 1, + pathname: this.pathname, + type: 'history-ot', + }) + + // Paste "hello" at the end, then re-home the comment onto it. + const paste = new TextOperation() + paste.retain(6) + paste.insert('hello') + const readd = new AddCommentOperation('c1', [new Range(6, 5)]) + const update = { + doc: this.doc_id, + op: [paste.toJSON(), readd.toJSON()], + v: 1, + meta: {}, + } + + await this.HistoryOTUpdateManager.applyUpdate( + this.project_id, + this.doc_id, + update + ) + + const file = savedFile(this.RedisManager) + expect(file.getContent()).to.equal(' worldhello') + const comment = file.getComments().getComment('c1') + expect(comment.ranges).to.have.length(1) + expect(comment.ranges[0].pos).to.equal(6) + expect(comment.ranges[0].length).to.equal(5) + }) + }) + + describe('tryApplyUpdate with a concurrent update to transform against', function () { + it('transforms and applies every op when behind the latest version', async function () { + // Server is at v2: a previous op inserted "ZZ" at the end. + const doc = new StringFileData('hello worldZZ') + this.DocumentManager.promises.getDoc.resolves({ + lines: doc.toRaw(), + version: 2, + pathname: this.pathname, + type: 'history-ot', + }) + const previous = new TextOperation() + previous.retain(11) + previous.insert('ZZ') + this.RedisManager.promises.getPreviousDocOps.resolves([ + { doc: this.doc_id, op: [previous.toJSON()], v: 1, meta: {} }, + ]) + + // Our update (based on v1) inserts "X" at the start and comments it. + const insert = new TextOperation() + insert.insert('X') + insert.retain(11) + const addComment = new AddCommentOperation('c1', [new Range(0, 1)]) + const update = { + doc: this.doc_id, + op: [insert.toJSON(), addComment.toJSON()], + v: 1, + meta: {}, + } + + await this.HistoryOTUpdateManager.applyUpdate( + this.project_id, + this.doc_id, + update + ) + + const file = savedFile(this.RedisManager) + expect(file.getContent()).to.equal('Xhello worldZZ') + const comment = file.getComments().getComment('c1') + expect(comment.ranges).to.have.length(1) + expect(comment.ranges[0].pos).to.equal(0) + expect(comment.ranges[0].length).to.equal(1) + }) + + it('rebases an op to a no-op when a concurrent update already applied it', async function () { + // Server at v2: a concurrent client already added the same comment. + const doc = new StringFileData('hello world') + new AddCommentOperation('c1', [new Range(0, 5)]).apply(doc) + this.DocumentManager.promises.getDoc.resolves({ + lines: doc.toRaw(), + version: 2, + pathname: this.pathname, + type: 'history-ot', + }) + const concurrent = new AddCommentOperation('c1', [new Range(0, 5)]) + this.RedisManager.promises.getPreviousDocOps.resolves([ + { doc: this.doc_id, op: [concurrent.toJSON()], v: 1, meta: {} }, + ]) + + // Our update (based on v1) adds the identical comment. + const addComment = new AddCommentOperation('c1', [new Range(0, 5)]) + const update = { + doc: this.doc_id, + op: [addComment.toJSON()], + v: 1, + meta: {}, + } + + await this.HistoryOTUpdateManager.applyUpdate( + this.project_id, + this.doc_id, + update + ) + + expect(update.op).to.deep.equal([{ noOp: true }]) + const file = savedFile(this.RedisManager) + expect(file.getContent()).to.equal('hello world') + const comment = file.getComments().getComment('c1') + expect(comment.ranges).to.have.length(1) + expect(comment.ranges[0].pos).to.equal(0) + expect(comment.ranges[0].length).to.equal(5) + }) + + it('threads the concurrent op through the client op-list', async function () { + // Server advanced to v2 by inserting "Z" at position 6 of "hello world". + const doc = new StringFileData('hello Zworld') + this.DocumentManager.promises.getDoc.resolves({ + lines: doc.toRaw(), + version: 2, + pathname: this.pathname, + type: 'history-ot', + }) + const previous = new TextOperation() + previous.retain(6) + previous.insert('Z') + previous.retain(5) + this.RedisManager.promises.getPreviousDocOps.resolves([ + { doc: this.doc_id, op: [previous.toJSON()], v: 1, meta: {} }, + ]) + + // Our update (v1): insert "AAA" at the start, then comment "hello" (now at + // [3, 8)). The "Z" insert must be threaded past our "AAA" (to pos 9, after + // the comment) so it leaves the comment alone; un-threaded (pos 6) it would + // wrongly extend the comment. + const insert = new TextOperation() + insert.insert('AAA') + insert.retain(11) + const addComment = new AddCommentOperation('c1', [new Range(3, 5)]) + const update = { + doc: this.doc_id, + op: [insert.toJSON(), addComment.toJSON()], + v: 1, + meta: {}, + } + + await this.HistoryOTUpdateManager.applyUpdate( + this.project_id, + this.doc_id, + update + ) + + const file = savedFile(this.RedisManager) + expect(file.getContent()).to.equal('AAAhello Zworld') + const comment = file.getComments().getComment('c1') + expect(comment.ranges).to.have.length(1) + expect(comment.ranges[0].pos).to.equal(3) + expect(comment.ranges[0].length).to.equal(5) + }) + + it('rebases a [text, comment, text] update against a [text, text, comment] concurrent update', async function () { + // Concurrent update (theirs), v1 -> v2 on "hello world": append "1", + // append "2", then comment "12". Edits stay at the end. + const doc = new StringFileData('hello world') + const t0 = new TextOperation() + t0.retain(11) + t0.insert('1') + doc.edit(t0) // "hello world1" + const t1 = new TextOperation() + t1.retain(12) + t1.insert('2') + doc.edit(t1) // "hello world12" + const theirComment = new AddCommentOperation('sc', [new Range(11, 2)]) + theirComment.apply(doc) // comments "12" + this.DocumentManager.promises.getDoc.resolves({ + lines: doc.toRaw(), + version: 2, + pathname: this.pathname, + type: 'history-ot', + }) + this.RedisManager.promises.getPreviousDocOps.resolves([ + { + doc: this.doc_id, + op: [t0.toJSON(), t1.toJSON(), theirComment.toJSON()], + v: 1, + meta: {}, + }, + ]) + + // Our update (ours), based on v1: prepend "A", comment "hello", prepend + // "B". Edits stay at the start, so nothing collides with theirs. + const o0 = new TextOperation() + o0.insert('A') + o0.retain(11) // "Ahello world" + const ourComment = new AddCommentOperation('c1', [new Range(1, 5)]) // "hello" + const o2 = new TextOperation() + o2.insert('B') + o2.retain(12) // "BAhello world" + const update = { + doc: this.doc_id, + op: [o0.toJSON(), ourComment.toJSON(), o2.toJSON()], + v: 1, + meta: {}, + } + + await this.HistoryOTUpdateManager.applyUpdate( + this.project_id, + this.doc_id, + update + ) + + const file = savedFile(this.RedisManager) + const content = file.getContent() + expect(content).to.equal('BAhello world12') + const comment = file.getComments().getComment('c1') + expect(comment.ranges).to.have.length(1) + expect(comment.ranges[0].pos).to.equal(2) + expect(comment.ranges[0].length).to.equal(5) + expect(content.slice(2, 7)).to.equal('hello') + }) + }) + + describe('tryApplyUpdate with a duplicate update', function () { + it('does not apply or persist when dupIfSource matches', async function () { + const doc = new StringFileData('hello world') + this.DocumentManager.promises.getDoc.resolves({ + lines: doc.toRaw(), + version: 2, + pathname: this.pathname, + type: 'history-ot', + }) + const previous = new TextOperation() + previous.retain(11) + previous.insert('!') + this.RedisManager.promises.getPreviousDocOps.resolves([ + { + doc: this.doc_id, + op: [previous.toJSON()], + v: 1, + meta: { source: 'source-1' }, + }, + ]) + + const addComment = new AddCommentOperation('c1', [new Range(0, 5)]) + const update = { + doc: this.doc_id, + op: [addComment.toJSON()], + v: 1, + meta: {}, + dupIfSource: ['source-1'], + } + + await this.HistoryOTUpdateManager.applyUpdate( + this.project_id, + this.doc_id, + update + ) + + expect(update.dup).to.be.true + expect(this.RedisManager.promises.updateDocument.called).to.be.false + expect(this.RealTimeRedisManager.sendData.called).to.be.true + }) + }) +}) diff --git a/services/web/frontend/js/features/review-panel/utils/snapshot-ranges.ts b/services/web/frontend/js/features/review-panel/utils/snapshot-ranges.ts index 95a36e82f4..a9a06a3104 100644 --- a/services/web/frontend/js/features/review-panel/utils/snapshot-ranges.ts +++ b/services/web/frontend/js/features/review-panel/utils/snapshot-ranges.ts @@ -23,6 +23,7 @@ export const buildRangesFromSnapshot = ( const comments = snapshot.getComments() const trackedChanges = snapshot.getTrackedChanges() const snapshotContent = snapshot.getContent() + const visibleContent = snapshot.getContent({ filterTrackedDeletes: true }) const trackedDeletes = new TrackedDeletes(trackedChanges) @@ -54,8 +55,14 @@ export const buildRangesFromSnapshot = ( seenComments.add(comment.id) const range = comment.ranges[0] // show the comment next to the first range + if (!range) { + // detached comment (commented text deleted) has no range to anchor to + continue + } + // slice from visible content, not the raw snapshot (still has tracked-deletes) const pos = trackedDeletes.toCodeMirror(range.pos) - const text = snapshotContent.substring(pos, range.end) + const end = trackedDeletes.toCodeMirror(range.end) + const text = visibleContent.substring(pos, end) ranges.comments.push({ id: comment.id, diff --git a/services/web/frontend/js/features/source-editor/extensions/history-ot.ts b/services/web/frontend/js/features/source-editor/extensions/history-ot.ts index 044157c769..b910f30aca 100644 --- a/services/web/frontend/js/features/source-editor/extensions/history-ot.ts +++ b/services/web/frontend/js/features/source-editor/extensions/history-ot.ts @@ -145,17 +145,19 @@ const buildRangesDecorations = ({ for (const comment of comments) { if (!comment.resolved) { for (const range of comment.ranges) { - decorations.push( - Decoration.mark({ - class: 'ol-cm-change ol-cm-change-c', - id: comment.id, - rangeType: 'comment', - comment, - }).range( - trackedDeletes.toCodeMirror(range.pos), - trackedDeletes.toCodeMirror(range.end) + const from = trackedDeletes.toCodeMirror(range.pos) + const to = trackedDeletes.toCodeMirror(range.end) + // a range hidden inside a tracked delete has no visible text to mark + if (from < to) { + decorations.push( + Decoration.mark({ + class: 'ol-cm-change ol-cm-change-c', + id: comment.id, + rangeType: 'comment', + comment, + }).range(from, to) ) - ) + } } } } diff --git a/services/web/frontend/js/features/source-editor/extensions/realtime.ts b/services/web/frontend/js/features/source-editor/extensions/realtime.ts index 4932c70aab..e033def07d 100644 --- a/services/web/frontend/js/features/source-editor/extensions/realtime.ts +++ b/services/web/frontend/js/features/source-editor/extensions/realtime.ts @@ -2,6 +2,7 @@ import { Prec, Transaction, Annotation, + ChangeSet, ChangeSpec, Text, StateEffect, @@ -22,9 +23,17 @@ import { InsertOp, RemoveOp, RetainOp, + AddCommentOperation, + DeleteCommentOperation, + SetCommentStateOperation, + StringFileData, + TrackedChangeList, } from 'overleaf-editor-core' import { rangesUpdatedEffect, setTrackChangesUserId } from './history-ot' -import { trackedDeletesFromState } from '@/features/source-editor/utils/tracked-deletes' +import { + TrackedDeletes, + trackedDeletesFromState, +} from '@/features/source-editor/utils/tracked-deletes' /* * Integrate CodeMirror 6 with the real-time system, via ShareJS. @@ -355,65 +364,113 @@ class HistoryOTAdapter { } } - onRemoteOp(operations: EditOperation[]) { - const trackedDeletes = trackedDeletesFromState(this.editor.view.state) - const changes: ChangeSpec[] = [] - let trackedChangesUpdated = false - for (const operation of operations) { + // One dispatch per batch: a per-op dispatch would rebuild rangesState from + // shareDoc.snapshot (already final) against an intermediate doc. + onRemoteOp(operations: EditOperation[], oldSnapshot?: StringFileData) { + const view = this.editor.view + let composed = ChangeSet.empty(view.state.doc.length) + let trackedDeletes = trackedDeletesFromState(view.state) + let trackedChanges: TrackedChangeList | null = null + let snapshotLength = 0 + let trackedChangesOrCommentsUpdated = false + + for (const [index, operation] of operations.entries()) { if (operation instanceof TextOperation) { + const specs: ChangeSpec[] = [] let cursor = 0 for (const op of operation.ops) { if (op instanceof InsertOp) { if (op.tracking?.type !== 'delete') { - changes.push({ + specs.push({ from: trackedDeletes.toCodeMirror(cursor), insert: op.insertion, }) } - trackedChangesUpdated = true + trackedChangesOrCommentsUpdated = true } else if (op instanceof RemoveOp) { - changes.push({ + specs.push({ from: trackedDeletes.toCodeMirror(cursor), to: trackedDeletes.toCodeMirror(cursor + op.length), }) cursor += op.length - trackedChangesUpdated = true + trackedChangesOrCommentsUpdated = true } else if (op instanceof RetainOp) { if (op.tracking != null) { if (op.tracking.type === 'delete') { - changes.push({ + specs.push({ from: trackedDeletes.toCodeMirror(cursor), to: trackedDeletes.toCodeMirror(cursor + op.length), }) } - trackedChangesUpdated = true + trackedChangesOrCommentsUpdated = true } cursor += op.length } } + composed = composed.compose(ChangeSet.of(specs, composed.newLength)) + } else if ( + operation instanceof AddCommentOperation || + operation instanceof DeleteCommentOperation || + operation instanceof SetCommentStateOperation + ) { + // no text change, but comment state changed — rebuild decorations + trackedChangesOrCommentsUpdated = true } - const view = this.editor.view - const effects: StateEffect[] = [] - const scrollEffect = view - .scrollSnapshot() - .map(view.state.changes(changes)) - if (scrollEffect != null) { - effects.push(scrollEffect) + // shareDoc.snapshot is already the post-batch state, so advance our own + // copy of oldSnapshot's tracked changes per op, keeping trackedDeletes + // correct for the next op. Only the tracked-change list feeds + // trackedDeletes, so there's no need to clone/edit the whole snapshot. + if (index < operations.length - 1 && oldSnapshot) { + try { + if (trackedChanges === null) { + // clone once so we never mutate oldSnapshot (owned by the sharejs event) + trackedChanges = TrackedChangeList.fromRaw( + oldSnapshot.getTrackedChanges().toRaw() + ) + snapshotLength = oldSnapshot.getStringLength() + } + if (operation instanceof TextOperation) { + if (operation.baseLength !== snapshotLength) { + // op doesn't fit our snapshot — we're out of sync + throw new Error('remote operation does not fit the snapshot') + } + // comment ops don't move tracked ranges + trackedChanges.applyTextOperation(operation) + snapshotLength = operation.targetLength + } + trackedDeletes = new TrackedDeletes(trackedChanges) + } catch (error) { + // out of sync — don't map remaining ops against the wrong state + this.shareDoc.emit('error', error) + debugConsole.error( + 'Could not advance snapshot for remote operations', + error + ) + return + } } - if (trackedChangesUpdated) { - effects.push(rangesUpdatedEffect.of(null)) - } - - view.dispatch({ - changes, - effects, - annotations: [ - Transaction.remote.of(true), - Transaction.addToHistory.of(false), - ], - }) } + + const effects: StateEffect[] = [] + const scrollEffect = view.scrollSnapshot().map(composed) + if (scrollEffect != null) { + effects.push(scrollEffect) + } + if (trackedChangesOrCommentsUpdated) { + effects.push(rangesUpdatedEffect.of(null)) + } + + view.dispatch({ + changes: composed, + effects, + annotations: [ + Transaction.remote.of(true), + Transaction.addToHistory.of(false), + ], + }) + + this.checkContent() } onCodeMirrorChange( diff --git a/services/web/test/frontend/features/review-panel/utils/snapshot-ranges.test.ts b/services/web/test/frontend/features/review-panel/utils/snapshot-ranges.test.ts new file mode 100644 index 0000000000..fd7a0bd523 --- /dev/null +++ b/services/web/test/frontend/features/review-panel/utils/snapshot-ranges.test.ts @@ -0,0 +1,88 @@ +import { expect } from 'chai' +import { + AddCommentOperation, + Range, + StringFileData, + TextOperation, + TrackingProps, +} from 'overleaf-editor-core' +import { buildRangesFromSnapshot } from '../../../../../frontend/js/features/review-panel/utils/snapshot-ranges' + +describe('buildRangesFromSnapshot', function () { + it('includes anchored comments', function () { + const snapshot = new StringFileData('hello world') + new AddCommentOperation('c1', [new Range(0, 5)]).apply(snapshot) + + const ranges = buildRangesFromSnapshot(snapshot, 'doc1') + + expect(ranges.comments).to.have.length(1) + expect(ranges.comments[0].id).to.equal('c1') + }) + + it('skips detached comments without throwing', function () { + const snapshot = new StringFileData('hello world') + new AddCommentOperation('c1', [new Range(0, 5)]).apply(snapshot) + + // delete the commented text; the comment detaches (its ranges empty out) + const op = new TextOperation() + op.remove(5) + op.retain(6) + snapshot.edit(op) + expect(snapshot.getComments().toArray()[0].isEmpty()).to.be.true + + const ranges = buildRangesFromSnapshot(snapshot, 'doc1') + + expect(ranges.comments).to.have.length(0) + }) + + it('maps comment text containing a tracked delete', function () { + const snapshot = new StringFileData('one two three') + + // track-delete "two " inside the comment: both comment ends need mapping + // and the snippet must come from the visible content + const op = new TextOperation() + op.retain(4) + op.retain(4, { + tracking: new TrackingProps( + 'delete', + 'user-1', + new Date('2025-01-01T00:00:00.000Z') + ), + }) + op.retain(5) + snapshot.edit(op) + + new AddCommentOperation('c1', [new Range(0, 13)]).apply(snapshot) + + const ranges = buildRangesFromSnapshot(snapshot, 'doc1') + + expect(ranges.comments).to.have.length(1) + expect(ranges.comments[0].op.p).to.equal(0) + expect(ranges.comments[0].op.c).to.equal('one three') + }) + + it('maps comment position and text past a tracked delete', function () { + const snapshot = new StringFileData('one two three') + + // track-delete "one ": the characters stay in the snapshot but are hidden + // from CodeMirror, so the two coordinate spaces diverge by 4 + const op = new TextOperation() + op.retain(4, { + tracking: new TrackingProps( + 'delete', + 'user-1', + new Date('2025-01-01T00:00:00.000Z') + ), + }) + op.retain(9) + snapshot.edit(op) + + new AddCommentOperation('c1', [new Range(8, 5)]).apply(snapshot) + + const ranges = buildRangesFromSnapshot(snapshot, 'doc1') + + expect(ranges.comments).to.have.length(1) + expect(ranges.comments[0].op.p).to.equal(4) + expect(ranges.comments[0].op.c).to.equal('three') + }) +}) diff --git a/services/web/test/frontend/features/source-editor/extensions/history-ot.test.ts b/services/web/test/frontend/features/source-editor/extensions/history-ot.test.ts index c7fe386cfa..72a7f1cc12 100644 --- a/services/web/test/frontend/features/source-editor/extensions/history-ot.test.ts +++ b/services/web/test/frontend/features/source-editor/extensions/history-ot.test.ts @@ -1,12 +1,15 @@ import { expect } from 'chai' import { EditorState, Transaction, TransactionSpec } from '@codemirror/state' import { + AddCommentOperation, + Range, StringFileData, TextOperation, TrackingProps, } from 'overleaf-editor-core' import { historyOT, + rangesState, setTrackChangesUserId, } from '@/features/source-editor/extensions/history-ot' import { DocumentContainer } from '@/features/ide-react/editor/document-container' @@ -81,9 +84,28 @@ function setup( edit(spec: TransactionSpec) { state = state.update(spec).state }, + getState() { + return state + }, } } +describe('historyOT rangesState', function () { + it('applies a tracked delete covering a commented range without crashing', function () { + const snapshot = snapshotWithTrackedDeletes('one two three') + new AddCommentOperation('c1', [new Range(4, 3)]).apply(snapshot) + const { edit, getState } = setup(snapshot) + + // tracked-delete "two ": the comment's range is fully hidden, so no mark + edit({ changes: { from: 4, to: 8 } }) + + expect(getState().doc.toString()).to.equal('one three') + const { decorations } = getState().field(rangesState) + // the tracked-delete widget only — no empty comment mark + expect(decorations.size).to.equal(1) + }) +}) + describe('historyOT updateSender', function () { describe('when the transaction does not change the document', function () { it('should not submit an operation', function () { diff --git a/services/web/test/frontend/features/source-editor/extensions/realtime.test.ts b/services/web/test/frontend/features/source-editor/extensions/realtime.test.ts index 7e301fd2b0..925d085b1b 100644 --- a/services/web/test/frontend/features/source-editor/extensions/realtime.test.ts +++ b/services/web/test/frontend/features/source-editor/extensions/realtime.test.ts @@ -1,8 +1,23 @@ import sinon from 'sinon' import { expect } from 'chai' +import { EventEmitter } from 'events' import { EditorFacade } from '../../../../../frontend/js/features/source-editor/extensions/realtime' +import { + historyOT, + rangesState, +} from '../../../../../frontend/js/features/source-editor/extensions/history-ot' import { EditorView } from '@codemirror/view' import { EditorState } from '@codemirror/state' +import { + AddCommentOperation, + EditOperation, + Range, + SetCommentStateOperation, + StringFileData, + TextOperation, + TrackingProps, +} from 'overleaf-editor-core' +import { historyOTType } from '../../../../../frontend/js/features/ide-react/editor/share-js-history-ot-type' describe('CodeMirror EditorFacade', function () { let state: EditorState, view: EditorView @@ -64,3 +79,171 @@ describe('CodeMirror EditorFacade', function () { expect(shareDoc.detach_cm6).to.be.a('function') }) }) + +describe('HistoryOTAdapter remote ops', function () { + const tick = () => new Promise(resolve => setTimeout(resolve, 0)) + + const snapshotWithTrackedDelete = ( + content: string, + { pos, length }: { pos: number; length: number } + ) => { + const snapshot = new StringFileData(content) + const op = new TextOperation() + if (pos > 0) { + op.retain(pos) + } + op.retain(length, { + tracking: new TrackingProps( + 'delete', + 'user-1', + new Date('2025-01-01T00:00:00.000Z') + ), + }) + if (pos + length < content.length) { + op.retain(content.length - pos - length) + } + snapshot.edit(op) + return snapshot + } + + const setup = (contentOrSnapshot: string | StringFileData) => { + const snapshot = + typeof contentOrSnapshot === 'string' + ? new StringFileData(contentOrSnapshot) + : contentOrSnapshot + + const shareDoc = Object.assign(new EventEmitter(), { + otType: 'history-ot', + snapshot, + getText: () => + shareDoc.snapshot.getContent({ filterTrackedDeletes: true }), + }) + + const errors: unknown[] = [] + shareDoc.on('error', (error: unknown) => errors.push(error)) + + // Emit a remote batch like sharejs `_otApply`: set the post-batch snapshot + // first, then pass the old snapshot as the 2nd arg (vendor/libs/sharejs.js). + const applyRemote = (operations: EditOperation[]) => { + const oldSnapshot = shareDoc.snapshot + shareDoc.snapshot = historyOTType.apply(oldSnapshot, operations) + shareDoc.emit('remoteop', operations, oldSnapshot) + } + + const currentDoc = { + historyOTShareDoc: shareDoc, + doc: { _doc: shareDoc }, + } + const state = EditorState.create({ + doc: snapshot.getContent({ filterTrackedDeletes: true }), + extensions: [historyOT(currentDoc as any)], + }) + const view = new EditorView({ state }) + const editor = new EditorFacade(view) + editor.attachShareJs(shareDoc as any) + + return { view, shareDoc, errors, applyRemote } + } + + it('refreshes comment decorations when a remote AddCommentOperation arrives', async function () { + const { view, errors, applyRemote } = setup('hello world') + expect(view.state.field(rangesState).decorations.size).to.equal(0) + + applyRemote([new AddCommentOperation('thread-1', [new Range(0, 5)])]) + + expect(view.state.field(rangesState).comments.length).to.equal(1) + expect(view.state.field(rangesState).decorations.size).to.equal(1) + await tick() + expect(errors).to.deep.equal([]) + }) + + it('drops the decoration when a remote SetCommentStateOperation resolves it', async function () { + const snapshot = new StringFileData('hello world') + new AddCommentOperation('thread-1', [new Range(0, 5)]).apply(snapshot) + const { view, errors, applyRemote } = setup(snapshot) + expect(view.state.field(rangesState).decorations.size).to.equal(1) + + applyRemote([new SetCommentStateOperation('thread-1', true)]) + + expect(view.state.field(rangesState).decorations.size).to.equal(0) + await tick() + expect(errors).to.deep.equal([]) + }) + + it('interprets each operation against the document produced by the previous one', async function () { + const { view, shareDoc, errors, applyRemote } = setup('abc') + + const opA = new TextOperation() + opA.remove(1) + opA.retain(2) + + const opB = new TextOperation() + opB.retain(2) + opB.insert('X') + + applyRemote([opA, opB]) + + expect(shareDoc.snapshot.getContent()).to.equal('bcX') + expect(view.state.doc.toString()).to.equal('bcX') + await tick() + expect(errors).to.deep.equal([]) + }) + + it('maps positions through tracked deletes advanced by earlier operations', async function () { + const { view, shareDoc, errors, applyRemote } = setup( + snapshotWithTrackedDelete('AAAAddBBBB', { pos: 4, length: 2 }) + ) + expect(view.state.doc.toString()).to.equal('AAAABBBB') + + const opA = new TextOperation() + opA.remove(4) + opA.retain(6) + + const opB = new TextOperation() + opB.retain(6) + opB.insert('X') + + applyRemote([opA, opB]) + + expect( + shareDoc.snapshot.getContent({ filterTrackedDeletes: true }) + ).to.equal('BBBBX') + expect(view.state.doc.toString()).to.equal('BBBBX') + await tick() + expect(errors).to.deep.equal([]) + }) + + it('stops applying a batch that does not fit the snapshot', async function () { + const { view, shareDoc, errors } = setup('abc') + // flush the attach-time content check while still in sync + await tick() + + const opA = new TextOperation() + opA.remove(1) + opA.retain(2) + + const opB = new TextOperation() + opB.retain(2) + opB.insert('X') + + // an oldSnapshot the ops cannot apply to: the local advance throws and the + // rest of the batch must not reach the editor + shareDoc.emit('remoteop', [opA, opB], new StringFileData('xy')) + + expect(errors).to.have.length(1) + expect(view.state.doc.toString()).to.equal('abc') + }) + + it('reports divergence between the editor and the snapshot after a remote op', async function () { + const { shareDoc, errors } = setup('abc') + // flush the attach-time content check while still in sync + await tick() + expect(errors).to.deep.equal([]) + + shareDoc.snapshot = new StringFileData('abcd') + shareDoc.emit('remoteop', []) + + await tick() + expect(errors).to.have.length(1) + }) +})