Merge pull request #18355 from overleaf/em-resync-tracked-changes

Handle tracked changes during resyncs

GitOrigin-RevId: 1d5b16a4cb17226da184a5430ebbcfc79ad9c7ce
This commit is contained in:
Eric Mc Sween 2024-05-22 08:15:07 -04:00 committed by Copybot
parent a0334898db
commit 6d216d4738
5 changed files with 545 additions and 83 deletions

View file

@ -19,12 +19,19 @@ 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'
/**
* @typedef {import('overleaf-editor-core').Comment} HistoryComment
* @typedef {import('overleaf-editor-core').TrackedChange} HistoryTrackedChange
* @typedef {import('./types').Comment} Comment
* @typedef {import('./types').Entity} Entity
* @typedef {import('./types').ResyncDocContentUpdate} ResyncDocContentUpdate
* @typedef {import('./types').RetainOp} RetainOp
* @typedef {import('./types').TrackedChange} TrackedChange
* @typedef {import('./types').TrackedChangeTransition} TrackedChangeTransition
* @typedef {import('./types').TrackingDirective} TrackingDirective
* @typedef {import('./types').TrackingType} TrackingType
* @typedef {import('./types').Update} Update
*/
const MAX_RESYNC_HISTORY_RECORDS = 100 // keep this many records of previous resyncs
@ -639,12 +646,18 @@ class SyncUpdateExpander {
}
const persistedComments = file.getComments().toArray()
await this.queueUpdateForOutOfSyncComments(
await this.queueUpdatesForOutOfSyncComments(
update,
pathname,
persistedContent,
persistedComments
)
const persistedChanges = file.getTrackedChanges().asSorted()
await this.queueUpdatesForOutOfSyncTrackedChanges(
update,
pathname,
persistedChanges
)
}
/**
@ -694,19 +707,14 @@ class SyncUpdateExpander {
}
/**
* Queue update for out of sync comments
* Queue updates for out of sync comments
*
* @param {ResyncDocContentUpdate} update
* @param {string} pathname
* @param {string} persistedContent
* @param {HistoryComment[]} persistedComments
*/
async queueUpdateForOutOfSyncComments(
update,
pathname,
persistedContent,
persistedComments
) {
async queueUpdatesForOutOfSyncComments(update, pathname, persistedComments) {
const expectedContent = update.resyncDocContent.content
const expectedComments = update.resyncDocContent.ranges?.comments ?? []
const resolvedComments = new Set(
update.resyncDocContent.resolvedComments ?? []
@ -760,12 +768,129 @@ class SyncUpdateExpander {
origin: this.origin,
ts: update.meta.ts,
pathname,
doc_length: persistedContent.length,
doc_length: expectedContent.length,
},
})
}
}
}
/**
* Queue updates for out of sync tracked changes
*
* @param {ResyncDocContentUpdate} update
* @param {string} pathname
* @param {readonly HistoryTrackedChange[]} persistedChanges
*/
async queueUpdatesForOutOfSyncTrackedChanges(
update,
pathname,
persistedChanges
) {
const expectedChanges = update.resyncDocContent.ranges?.changes ?? []
const expectedContent = update.resyncDocContent.content
/**
* A cursor on the expected content
*/
let cursor = 0
/**
* The persisted tracking at cursor
*
* @type {TrackingDirective}
*/
let persistedTracking = { type: 'none' }
/**
* The expected tracking at cursor
*
* @type {TrackingDirective}
*/
let expectedTracking = { type: 'none' }
/**
* The retain ops for the update
*
* @type {RetainOp[]}
*/
const ops = []
/**
* The retain op being built
*
* @type {RetainOp | null}
*/
let currentOp = null
for (const transition of getTrackedChangesTransitions(
persistedChanges,
expectedChanges,
expectedContent.length
)) {
if (transition.pos > cursor) {
// The next transition will move the cursor. Decide what to do with the interval.
if (trackingDirectivesEqual(expectedTracking, persistedTracking)) {
// Expected tracking and persisted tracking are in sync. Emit the
// current op and skip this interval.
if (currentOp != null) {
ops.push(currentOp)
currentOp = null
}
} else {
// Expected tracking and persisted tracking are different.
const retainedText = expectedContent.slice(cursor, transition.pos)
if (
currentOp?.tracking != null &&
trackingDirectivesEqual(expectedTracking, currentOp.tracking)
) {
// The current op has the right tracking. Extend it.
currentOp.r += retainedText
} else {
// The current op doesn't have the right tracking. Emit the current
// op and start a new one.
if (currentOp != null) {
ops.push(currentOp)
}
currentOp = {
r: retainedText,
p: cursor,
tracking: expectedTracking,
}
}
}
// Advance cursor
cursor = transition.pos
}
// Update the expected and persisted tracking
if (transition.stage === 'persisted') {
persistedTracking = transition.tracking
} else {
expectedTracking = transition.tracking
}
}
// Emit the last op
if (currentOp != null) {
ops.push(currentOp)
}
if (ops.length > 0) {
this.expandedUpdates.push({
doc: update.doc,
op: ops,
meta: {
resync: true,
origin: this.origin,
ts: update.meta.ts,
pathname,
doc_length: expectedContent.length,
},
})
}
}
}
/**
@ -788,6 +913,116 @@ function commentRangesAreInSync(persistedComment, expectedComment) {
)
}
/**
* Iterates through expected tracked changes and persisted tracked changes and
* returns all transitions, sorted by position.
*
* @param {readonly HistoryTrackedChange[]} persistedChanges
* @param {TrackedChange[]} expectedChanges
* @param {number} docLength
*/
function getTrackedChangesTransitions(
persistedChanges,
expectedChanges,
docLength
) {
/** @type {TrackedChangeTransition[]} */
const transitions = []
for (const change of persistedChanges) {
transitions.push({
stage: 'persisted',
pos: change.range.start,
tracking: {
type: change.tracking.type,
userId: change.tracking.userId,
ts: change.tracking.ts.toISOString(),
},
})
transitions.push({
stage: 'persisted',
pos: change.range.end,
tracking: { type: 'none' },
})
}
for (const change of expectedChanges) {
const op = change.op
const pos = op.hpos ?? op.p
if (isInsert(op)) {
transitions.push({
stage: 'expected',
pos,
tracking: {
type: 'insert',
userId: change.metadata.user_id,
ts: change.metadata.ts,
},
})
transitions.push({
stage: 'expected',
pos: pos + op.i.length,
tracking: { type: 'none' },
})
} else {
transitions.push({
stage: 'expected',
pos,
tracking: {
type: 'delete',
userId: change.metadata.user_id,
ts: change.metadata.ts,
},
})
transitions.push({
stage: 'expected',
pos: pos + op.d.length,
tracking: { type: 'none' },
})
}
}
transitions.push({
stage: 'expected',
pos: docLength,
tracking: { type: 'none' },
})
transitions.sort((a, b) => {
if (a.pos < b.pos) {
return -1
} else if (a.pos > b.pos) {
return 1
} else if (a.tracking.type === 'none' && b.tracking.type !== 'none') {
// none type comes before other types so that it can be overridden at the
// same position
return -1
} else if (a.tracking.type !== 'none' && b.tracking.type === 'none') {
// none type comes before other types so that it can be overridden at the
// same position
return 1
} else {
return 0
}
})
return transitions
}
/**
* Returns true if both tracking directives are equal
*
* @param {TrackingDirective} a
* @param {TrackingDirective} b
*/
function trackingDirectivesEqual(a, b) {
if (a.type === 'none') {
return b.type === 'none'
} else {
return a.type === b.type && a.userId === b.userId && a.ts === b.ts
}
}
// EXPORTS
const startResyncCb = callbackify(startResync)

View file

@ -4,19 +4,17 @@ import _ from 'lodash'
import Core from 'overleaf-editor-core'
import * as Errors from './Errors.js'
import * as OperationsCompressor from './OperationsCompressor.js'
import { isInsert, isRetain, isDelete, isComment } from './Utils.js'
/**
* @typedef {import('./types').AddDocUpdate} AddDocUpdate
* @typedef {import('./types').AddFileUpdate} AddFileUpdate
* @typedef {import('./types').CommentOp} CommentOp
* @typedef {import('./types').DeleteCommentUpdate} DeleteCommentUpdate
* @typedef {import('./types').DeleteOp} DeleteOp
* @typedef {import('./types').InsertOp} InsertOp
* @typedef {import('./types').RetainOp} RetainOp
* @typedef {import('./types').Op} Op
* @typedef {import('./types').RawScanOp} RawScanOp
* @typedef {import('./types').RenameUpdate} RenameUpdate
* @typedef {import('./types').TextUpdate} TextUpdate
* @typedef {import('./types').TrackingDirective} TrackingDirective
* @typedef {import('./types').TrackingProps} TrackingProps
* @typedef {import('./types').SetCommentStateUpdate} SetCommentStateUpdate
* @typedef {import('./types').Update} Update
@ -405,7 +403,7 @@ class OperationsBuilder {
/**
* @param {number} length
* @param {object} opts
* @param {TrackingProps} [opts.tracking]
* @param {TrackingDirective} [opts.tracking]
*/
retain(length, opts = {}) {
if (opts.tracking) {
@ -461,35 +459,3 @@ class OperationsBuilder {
return this.operations
}
}
/**
* @param {Op} op
* @returns {op is InsertOp}
*/
function isInsert(op) {
return 'i' in op && op.i != null
}
/**
* @param {Op} op
* @returns {op is RetainOp}
*/
function isRetain(op) {
return 'r' in op && op.r != null
}
/**
* @param {Op} op
* @returns {op is DeleteOp}
*/
function isDelete(op) {
return 'd' in op && op.d != null
}
/**
* @param {Op} op
* @returns {op is CommentOp}
*/
function isComment(op) {
return 'c' in op && op.c != null && 't' in op && op.t != null
}

View file

@ -0,0 +1,41 @@
// @ts-check
/**
* @typedef {import('./types').CommentOp} CommentOp
* @typedef {import('./types').DeleteOp} DeleteOp
* @typedef {import('./types').InsertOp} InsertOp
* @typedef {import('./types').Op} Op
* @typedef {import('./types').RetainOp} RetainOp
*/
/**
* @param {Op} op
* @returns {op is InsertOp}
*/
export function isInsert(op) {
return 'i' in op && op.i != null
}
/**
* @param {Op} op
* @returns {op is RetainOp}
*/
export function isRetain(op) {
return 'r' in op && op.r != null
}
/**
* @param {Op} op
* @returns {op is DeleteOp}
*/
export function isDelete(op) {
return 'd' in op && op.d != null
}
/**
* @param {Op} op
* @returns {op is CommentOp}
*/
export function isComment(op) {
return 'c' in op && op.c != null && 't' in op && op.t != null
}

View file

@ -97,7 +97,7 @@ export type RetainOp = {
r: string
p: number
hpos?: number
tracking?: TrackingProps
tracking?: TrackingDirective
}
export type InsertOp = {
@ -140,20 +140,22 @@ export type RawOrigin = {
kind: string
}
export type TrackingProps =
| { type: 'none' }
| {
type: 'insert' | 'delete'
userId: string
ts: string
}
export type TrackingProps = {
type: 'insert' | 'delete'
userId: string
ts: string
}
export type TrackingDirective = TrackingProps | { type: 'none' }
export type TrackingType = 'insert' | 'delete' | 'none'
export type RawScanOp =
| number
| string
| { r: number; tracking?: TrackingProps }
| { r: number; tracking?: TrackingDirective }
| { i: string; tracking?: TrackingProps; commentIds?: string[] }
| { d: number; tracking?: TrackingProps }
| { d: number }
export type TrackedChangeSnapshot = {
op: {
@ -207,9 +209,15 @@ export type Comment = {
export type TrackedChange = {
id: string
op: Op
op: InsertOp | DeleteOp
metadata: {
user_id: string
ts: string
}
}
export type TrackedChangeTransition = {
pos: number
tracking: TrackingDirective
stage: 'persisted' | 'expected'
}

View file

@ -2,20 +2,20 @@ import sinon from 'sinon'
import { expect } from 'chai'
import mongodb from 'mongodb-legacy'
import tk from 'timekeeper'
import { Comment, Range } from 'overleaf-editor-core'
import { Comment, TrackedChange, Range } from 'overleaf-editor-core'
import { strict as esmock } from 'esmock'
const { ObjectId } = mongodb
const MODULE_PATH = '../../../../app/js/SyncManager.js'
const timestamp = new Date()
const TIMESTAMP = new Date().toISOString()
const USER_ID = 'user-id'
function resyncProjectStructureUpdate(docs, files) {
return {
resyncProjectStructure: { docs, files },
meta: {
ts: timestamp,
ts: TIMESTAMP,
},
}
}
@ -37,7 +37,7 @@ function docContentSyncUpdate(
},
meta: {
ts: timestamp,
ts: TIMESTAMP,
},
}
}
@ -46,12 +46,21 @@ function makeComment(commentId, pos, text) {
return {
id: commentId,
op: { p: pos, c: text, t: commentId },
meta: {
ts: timestamp,
metadata: {
user_id: USER_ID,
ts: TIMESTAMP,
},
}
}
function makeTrackedChange(id, op) {
return {
id,
op,
metadata: { user_id: USER_ID, ts: TIMESTAMP },
}
}
describe('SyncManager', function () {
beforeEach(async function () {
this.now = new Date()
@ -476,7 +485,7 @@ describe('SyncManager', function () {
doc: 'doc-id',
path: 'main.tex',
}
this.persistedDocContent = 'the quick brown fox jumps over the lazy fox'
this.persistedDocContent = 'the quick brown fox jumps over the lazy dog'
this.persistedFile = {
file: 'file-id',
path: '1.png',
@ -488,6 +497,9 @@ describe('SyncManager', function () {
getComments: sinon
.stub()
.returns({ toArray: sinon.stub().returns([]) }),
getTrackedChanges: sinon
.stub()
.returns({ asSorted: sinon.stub().returns([]) }),
getHash: sinon.stub().returns(null),
}
this.fileMap = {
@ -566,7 +578,7 @@ describe('SyncManager', function () {
new_pathname: '',
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -590,7 +602,7 @@ describe('SyncManager', function () {
new_pathname: '',
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -625,7 +637,7 @@ describe('SyncManager', function () {
url: newFile.url,
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -659,7 +671,7 @@ describe('SyncManager', function () {
docLines: '',
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -694,7 +706,7 @@ describe('SyncManager', function () {
new_pathname: '',
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -704,7 +716,7 @@ describe('SyncManager', function () {
url: fileWichWasADoc.url,
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -791,7 +803,7 @@ describe('SyncManager', function () {
new_pathname: '',
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -801,7 +813,7 @@ describe('SyncManager', function () {
url: persistedFileWithNewContent.url,
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -916,7 +928,7 @@ describe('SyncManager', function () {
pathname: this.persistedDoc.path,
doc_length: this.persistedDocContent.length,
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -953,7 +965,7 @@ describe('SyncManager', function () {
docLines: '',
meta: {
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -964,7 +976,7 @@ describe('SyncManager', function () {
pathname: newDoc.path,
doc_length: 0,
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -1023,7 +1035,7 @@ describe('SyncManager', function () {
pathname: this.persistedDoc.path,
doc_length: this.persistedDocContent.length,
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -1060,7 +1072,7 @@ describe('SyncManager', function () {
pathname: this.persistedDoc.path,
doc_length: 'stored content'.length,
resync: true,
ts: timestamp,
ts: TIMESTAMP,
origin: { kind: 'history-resync' },
},
},
@ -1143,7 +1155,7 @@ describe('SyncManager', function () {
},
pathname: this.persistedDoc.path,
resync: true,
ts: timestamp,
ts: TIMESTAMP,
doc_length: this.persistedDocContent.length,
},
},
@ -1178,7 +1190,7 @@ describe('SyncManager', function () {
kind: 'history-resync',
},
resync: true,
ts: timestamp,
ts: TIMESTAMP,
},
},
])
@ -1218,7 +1230,7 @@ describe('SyncManager', function () {
kind: 'history-resync',
},
resync: true,
ts: timestamp,
ts: TIMESTAMP,
pathname: this.persistedDoc.path,
doc_length: this.persistedDocContent.length,
},
@ -1259,5 +1271,205 @@ describe('SyncManager', function () {
])
})
})
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.changes = [
makeTrackedChange('td1', { p: 4, d: 'quick ' }),
makeTrackedChange('ti1', { p: 4, hpos: 10, i: 'brown ' }),
makeTrackedChange('td2', { p: 14, hpos: 20, d: 'jumps ' }),
makeTrackedChange('ti2', { p: 28, hpos: 40, i: 'dog' }),
]
})
it('does nothing if tracked changes have not changed', async function () {
const updates = [
docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, {
changes: this.changes,
}),
]
const expandedUpdates =
await this.SyncManager.promises.expandSyncUpdates(
this.projectId,
this.historyId,
updates,
this.extendLock
)
expect(expandedUpdates).to.deep.equal([])
})
it('adds new tracked changes', async function () {
this.changes.splice(
3,
0,
makeTrackedChange('td3', { p: 29, hpos: 35, d: 'lazy ' })
)
const updates = [
docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, {
changes: this.changes,
}),
]
const expandedUpdates =
await this.SyncManager.promises.expandSyncUpdates(
this.projectId,
this.historyId,
updates,
this.extendLock
)
expect(expandedUpdates).to.deep.equal([
{
doc: this.persistedDoc.doc,
op: [
{
p: 35,
r: 'lazy ',
tracking: {
type: 'delete',
userId: USER_ID,
ts: TIMESTAMP,
},
},
],
meta: {
origin: {
kind: 'history-resync',
},
pathname: this.persistedDoc.path,
resync: true,
ts: TIMESTAMP,
doc_length: this.persistedDocContent.length,
},
},
])
})
it('removes extra tracked changes', async function () {
this.changes.splice(0, 1)
const updates = [
docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, {
changes: this.changes,
}),
]
const expandedUpdates =
await this.SyncManager.promises.expandSyncUpdates(
this.projectId,
this.historyId,
updates,
this.extendLock
)
expect(expandedUpdates).to.deep.equal([
{
doc: this.persistedDoc.doc,
op: [
{
p: 4,
r: 'quick ',
tracking: { type: 'none' },
},
],
meta: {
origin: {
kind: 'history-resync',
},
pathname: this.persistedDoc.path,
resync: true,
ts: TIMESTAMP,
doc_length: this.persistedDocContent.length,
},
},
])
})
it('handles overlapping ranges', async function () {
this.changes = [
makeTrackedChange('ti1', { p: 0, i: 'the quic' }),
makeTrackedChange('td1', { p: 8, d: 'k br' }),
makeTrackedChange('ti2', { p: 14, hpos: 23, i: 'ps over' }),
]
const updates = [
docContentSyncUpdate(this.persistedDoc, this.persistedDocContent, {
changes: this.changes,
}),
]
const expandedUpdates =
await this.SyncManager.promises.expandSyncUpdates(
this.projectId,
this.historyId,
updates,
this.extendLock
)
// Before: the [quick ][brown ] fox [jumps ]over the lazy dog
// After: [the quic][k br]own fox jum[ps over] the lazy dog
expect(expandedUpdates).to.deep.equal([
{
doc: this.persistedDoc.doc,
op: [
{
p: 0,
r: 'the quic',
tracking: { type: 'insert', userId: USER_ID, ts: TIMESTAMP },
},
{
p: 10,
r: 'br',
tracking: { type: 'delete', userId: USER_ID, ts: TIMESTAMP },
},
{
p: 12,
r: 'own ',
tracking: { type: 'none' },
},
{
p: 20,
r: 'jum',
tracking: { type: 'none' },
},
{
p: 23,
r: 'ps over',
tracking: { type: 'insert', userId: USER_ID, ts: TIMESTAMP },
},
{
p: 40,
r: 'dog',
tracking: { type: 'none' },
},
],
meta: {
origin: { kind: 'history-resync' },
pathname: this.persistedDoc.path,
resync: true,
ts: TIMESTAMP,
doc_length: this.persistedDocContent.length,
},
},
])
})
})
})
})