Merge pull request #35982 from overleaf/mg-history-ot-comments

HistoryOT - Fix comments being dropped and crashing on detached ranges

GitOrigin-RevId: 0906a55a51a6fda9ac828efcbe72989573161261

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,

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)
           )
-        )
+        }
       }
     }
   }

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<any>[] = []
-      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<any>[] = []
+    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(

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')
+  })
+})

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 () {

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)
+  })
+})