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 c13504b4b4..044157c769 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 @@ -251,8 +251,11 @@ const updateSender = EditorState.transactionExtender.of(tr => { const trackingUserId = tr.startState.field(trackChangesUserIdState) const trackedDeletes = trackedDeletesFromState(tr.startState) const startDoc = tr.startState.doc + // Seed the builder with the full snapshot length (visible length plus all + // tracked deletes). Mapping the CM length through toSnapshot under-counts + // when a tracked delete sits at the end of the document. const opBuilder = new OperationBuilder( - trackedDeletes.toSnapshot(startDoc.length) + startDoc.length + trackedDeletes.totalLength ) if (trackingUserId == null) { diff --git a/services/web/frontend/js/features/source-editor/utils/tracked-deletes.ts b/services/web/frontend/js/features/source-editor/utils/tracked-deletes.ts index 1a1ea96221..532faaa533 100644 --- a/services/web/frontend/js/features/source-editor/utils/tracked-deletes.ts +++ b/services/web/frontend/js/features/source-editor/utils/tracked-deletes.ts @@ -13,6 +13,12 @@ export class TrackedDeletes { toSnapshot: OffsetTable } + /** + * Total length of tracked deletes, i.e. the difference between the snapshot + * length and the visible CodeMirror length. + */ + readonly totalLength: number + constructor(trackedChanges: TrackedChangeList) { this.offsets = { toCM6: [{ pos: 0, map: pos => pos }], @@ -42,12 +48,20 @@ export class TrackedDeletes { offset = newOffset } } + this.totalLength = offset } toCodeMirror(snapshotPos: number) { return this.mapPos(snapshotPos, this.offsets.toCM6) } + /** + * Map a CodeMirror position to a snapshot position, for placing an edit. At a + * tracked-delete boundary this deliberately maps to *before* the delete (so + * typing lands before the struck-through text), which means it under-counts + * for a delete at the end of the document. Use `totalLength` for the + * whole-document length, not `toSnapshot(docLength)`. + */ toSnapshot(cm6Pos: number) { return this.mapPos(cm6Pos, this.offsets.toSnapshot) } 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 new file mode 100644 index 0000000000..c7fe386cfa --- /dev/null +++ b/services/web/test/frontend/features/source-editor/extensions/history-ot.test.ts @@ -0,0 +1,268 @@ +import { expect } from 'chai' +import { EditorState, Transaction, TransactionSpec } from '@codemirror/state' +import { + StringFileData, + TextOperation, + TrackingProps, +} from 'overleaf-editor-core' +import { + historyOT, + setTrackChangesUserId, +} from '@/features/source-editor/extensions/history-ot' +import { DocumentContainer } from '@/features/ide-react/editor/document-container' + +const TIMESTAMP = new Date('2025-01-01T00:00:00.000Z') +const USER_ID = 'user-1' + +// Build a StringFileData whose stored snapshot carries the given tracked-delete +// ranges (expressed in snapshot coordinates). A track-changes delete is encoded +// as a RetainOp with `tracking: delete`, so the deleted characters stay in the +// snapshot and are only hidden from CodeMirror by `filterTrackedDeletes` — the +// snapshot stays longer than the visible document. +function snapshotWithTrackedDeletes( + text: string, + trackedDeleteRanges: Array<{ pos: number; length: number }> = [] +) { + const snapshot = new StringFileData(text) + if (trackedDeleteRanges.length > 0) { + const op = new TextOperation() + let cursor = 0 + for (const { pos, length } of trackedDeleteRanges) { + if (pos > cursor) { + op.retain(pos - cursor) + } + op.retain(length, { + tracking: new TrackingProps('delete', USER_ID, TIMESTAMP), + }) + cursor = pos + length + } + if (cursor < text.length) { + op.retain(text.length - cursor) + } + snapshot.edit(op) + } + return snapshot +} + +// Mock a DocumentContainer around a snapshot that captures submitted ops. The +// same share doc backs both `historyOTShareDoc` and `doc._doc`, as in +// production. `submitOp` applies each op to the snapshot the way production +// does, so an op with the wrong base length throws here instead of silently +// passing — that is what makes the base-length assertions meaningful. +function setup( + snapshot: StringFileData, + { trackChangesEnabled = true }: { trackChangesEnabled?: boolean } = {} +) { + const submitted: TextOperation[] = [] + const shareDoc = { + snapshot, + submitOp: (ops: TextOperation[]) => { + for (const op of ops) { + snapshot.edit(op) + } + submitted.push(...ops) + }, + emit: () => {}, + } + const currentDoc = { + historyOTShareDoc: shareDoc, + doc: { _doc: shareDoc }, + } as unknown as DocumentContainer + + let state = EditorState.create({ + doc: snapshot.getContent({ filterTrackedDeletes: true }), + extensions: historyOT(currentDoc), + }) + if (trackChangesEnabled) { + state = state.update(setTrackChangesUserId(USER_ID)).state + } + return { + submitted, + edit(spec: TransactionSpec) { + state = state.update(spec).state + }, + } +} + +describe('historyOT updateSender', function () { + describe('when the transaction does not change the document', function () { + it('should not submit an operation', function () { + const { submitted, edit } = setup( + snapshotWithTrackedDeletes('Hello world') + ) + + // A selection-only transaction: no document change. + edit({ selection: { anchor: 2, head: 5 } }) + + expect(submitted).to.have.length(0) + }) + }) + + describe('when the transaction is a remote change', function () { + it('should not submit an operation', function () { + const { submitted, edit } = setup( + snapshotWithTrackedDeletes('Hello world') + ) + + // A change that originated remotely is annotated as such; the sender must + // not echo it back as an outgoing operation. + edit({ + changes: { from: 0, insert: 'remote text ' }, + annotations: Transaction.remote.of(true), + }) + + expect(submitted).to.have.length(0) + }) + }) + + describe('with no tracked changes in the document', function () { + describe('and tracked changes disabled', function () { + it('should submit a plain insertion', function () { + const { submitted, edit } = setup( + snapshotWithTrackedDeletes('Hello world'), + { trackChangesEnabled: false } + ) + + edit({ changes: { from: 11, insert: '!!!' } }) + + expect(submitted).to.have.length(1) + const op = submitted[0] + expect(op.baseLength).to.equal(11) + expect(op.targetLength).to.equal(14) + expect(op.toJSON().textOperation).to.deep.equal([11, '!!!']) + }) + + it('should submit a plain deletion', function () { + const { submitted, edit } = setup( + snapshotWithTrackedDeletes('Hello world'), + { trackChangesEnabled: false } + ) + + // Remove "Hello " from the start. + edit({ changes: { from: 0, to: 6 } }) + + expect(submitted).to.have.length(1) + const op = submitted[0] + expect(op.baseLength).to.equal(11) + expect(op.targetLength).to.equal(5) + expect(op.toJSON().textOperation).to.deep.equal([-6, 5]) + }) + }) + + describe('and tracked changes enabled', function () { + it('should submit the insertion as a tracked insert', function () { + const { submitted, edit } = setup( + snapshotWithTrackedDeletes('Hello world') + ) + + edit({ changes: { from: 11, insert: '!!!' } }) + + expect(submitted).to.have.length(1) + const op = submitted[0] + expect(op.baseLength).to.equal(11) + expect(op.targetLength).to.equal(14) + const [retain, insert] = op.toJSON().textOperation as any[] + expect(retain).to.equal(11) + expect(insert.i).to.equal('!!!') + expect(insert.tracking).to.include({ type: 'insert', userId: USER_ID }) + }) + + it('should submit the deletion as a tracked retain that keeps the snapshot length', function () { + const { submitted, edit } = setup( + snapshotWithTrackedDeletes('Hello world') + ) + + // Removing "Hello " while tracking must not shrink the snapshot: the + // characters are retained and marked as a tracked delete instead. + edit({ changes: { from: 0, to: 6 } }) + + expect(submitted).to.have.length(1) + const op = submitted[0] + expect(op.baseLength).to.equal(11) + expect(op.targetLength).to.equal(11) + const [tracked, retain] = op.toJSON().textOperation as any[] + expect(tracked.r).to.equal(6) + expect(tracked.tracking).to.include({ type: 'delete', userId: USER_ID }) + expect(retain).to.equal(5) + }) + }) + }) + + describe('with a tracked delete at the end of the document', function () { + // "Hello world" with the trailing "world" tracked-deleted: the visible + // CodeMirror document is "Hello " (6 chars) while the snapshot is still 11. + const trailingDelete = () => + snapshotWithTrackedDeletes('Hello world', [{ pos: 6, length: 5 }]) + + it('should base the operation on the full snapshot length, not the visible length', function () { + const { submitted, edit } = setup(trailingDelete()) + + // Type at the visible end of the document. + edit({ changes: { from: 6, insert: 'there' } }) + + expect(submitted).to.have.length(1) + const op = submitted[0] + // A base length of 6 (the visible length) would be rejected by + // TextOperation.apply against the 11-char snapshot. + expect(op.baseLength).to.equal(11) + expect(op.targetLength).to.equal(16) + }) + + it('should keep applying cleanly across consecutive edits', function () { + const { submitted, edit } = setup(trailingDelete()) + + // Three separate insertions at the growing visible end. submitOp applies + // each op to the snapshot, so an under-counted base length would throw + // ApplyError on the offending edit. + edit({ changes: { from: 6, insert: 'foo' } }) + edit({ changes: { from: 9, insert: 'bar' } }) + edit({ changes: { from: 12, insert: 'baz' } }) + + expect(submitted).to.have.length(3) + expect(submitted.map(op => op.baseLength)).to.deep.equal([11, 14, 17]) + expect(submitted.map(op => op.targetLength)).to.deep.equal([14, 17, 20]) + }) + + it('should base the operation correctly even when the current user is not tracking changes', function () { + // Tracked deletes can already be in the snapshot from an earlier session + // while the current user has tracking off; the base length must still + // account for them. + const { submitted, edit } = setup(trailingDelete(), { + trackChangesEnabled: false, + }) + + edit({ changes: { from: 6, insert: 'ABC' } }) + + expect(submitted).to.have.length(1) + const op = submitted[0] + expect(op.baseLength).to.equal(11) + expect(op.targetLength).to.equal(14) + // Plain insert (no tracking) because the current user is not tracking. + expect(op.toJSON().textOperation).to.deep.equal([6, 'ABC', 5]) + }) + }) + + describe('with tracked deletes spread across the document', function () { + it('should include every tracked delete in the operation base length', function () { + // "aaa bbb ccc" with the middle and last words tracked-deleted: visible + // "aaa " (5 chars) over an 11-char snapshot, 6 chars deleted in two runs. + const snapshot = snapshotWithTrackedDeletes('aaa bbb ccc', [ + { pos: 4, length: 3 }, + { pos: 8, length: 3 }, + ]) + expect(snapshot.getContent({ filterTrackedDeletes: true })).to.equal( + 'aaa ' + ) + + const { submitted, edit } = setup(snapshot) + + edit({ changes: { from: 5, insert: 'END' } }) + + expect(submitted).to.have.length(1) + const op = submitted[0] + // 5 visible + 6 deleted = 11, not the toSnapshot under-count. + expect(op.baseLength).to.equal(11) + expect(op.targetLength).to.equal(14) + }) + }) +})