diff --git a/services/document-updater/app/js/DocumentManager.js b/services/document-updater/app/js/DocumentManager.js index a9a720ed13..39832c8e7a 100644 --- a/services/document-updater/app/js/DocumentManager.js +++ b/services/document-updater/app/js/DocumentManager.js @@ -17,6 +17,7 @@ const DocumentManager = { lines, version, ranges, + resolvedCommentIds, pathname, projectHistoryId, unflushedTime, @@ -31,6 +32,7 @@ const DocumentManager = { lines, version, ranges, + resolvedCommentIds, pathname, projectHistoryId, historyRangesSupport, @@ -40,6 +42,8 @@ const DocumentManager = { projectId, docId, lines, + ranges, + resolvedCommentIds, version, pathname, projectHistoryId, @@ -53,6 +57,7 @@ const DocumentManager = { lines, version, ranges, + resolvedCommentIds, pathname, projectHistoryId, historyRangesSupport @@ -61,6 +66,7 @@ const DocumentManager = { lines, version, ranges: ranges || {}, + resolvedCommentIds, pathname, projectHistoryId, unflushedTime: null, @@ -74,6 +80,7 @@ const DocumentManager = { ranges, pathname, projectHistoryId, + resolvedCommentIds, unflushedTime, alreadyLoaded: true, historyRangesSupport, @@ -291,6 +298,8 @@ const DocumentManager = { } if (historyRangesSupport) { + await RedisManager.promises.updateCommentState(docId, commentId, resolved) + await ProjectHistoryRedisManager.promises.queueOps( projectId, JSON.stringify({ @@ -326,6 +335,7 @@ const DocumentManager = { ) if (historyRangesSupport) { + await RedisManager.promises.updateCommentState(docId, commentId, false) await ProjectHistoryRedisManager.promises.queueOps( projectId, JSON.stringify({ @@ -368,8 +378,14 @@ const DocumentManager = { async resyncDocContents(projectId, docId, path) { logger.debug({ projectId, docId, path }, 'start resyncing doc contents') - let { lines, ranges, version, projectHistoryId, historyRangesSupport } = - await RedisManager.promises.getDoc(projectId, docId) + let { + lines, + ranges, + resolvedCommentIds, + version, + projectHistoryId, + historyRangesSupport, + } = await RedisManager.promises.getDoc(projectId, docId) // To avoid issues where the same docId appears with different paths, // we use the path from the resyncProjectStructure update. If we used @@ -381,10 +397,16 @@ const DocumentManager = { { projectId, docId }, 'resyncing doc contents - not found in redis - retrieving from web' ) - ;({ lines, ranges, version, projectHistoryId, historyRangesSupport } = - await PersistenceManager.promises.getDoc(projectId, docId, { - peek: true, - })) + ;({ + lines, + ranges, + resolvedCommentIds, + version, + projectHistoryId, + historyRangesSupport, + } = await PersistenceManager.promises.getDoc(projectId, docId, { + peek: true, + })) } else { logger.debug( { projectId, docId }, @@ -398,6 +420,7 @@ const DocumentManager = { docId, lines, ranges, + resolvedCommentIds, version, // use the path from the resyncProjectStructure update path, diff --git a/services/document-updater/app/js/PersistenceManager.js b/services/document-updater/app/js/PersistenceManager.js index 74a97baede..484b9bab2a 100644 --- a/services/document-updater/app/js/PersistenceManager.js +++ b/services/document-updater/app/js/PersistenceManager.js @@ -102,7 +102,8 @@ function getDoc(projectId, docId, options = {}, _callback) { body.ranges, body.pathname, body.projectHistoryId?.toString(), - body.historyRangesSupport || false + body.historyRangesSupport || false, + body.resolvedCommentIds || [] ) } else if (res.statusCode === 404) { callback(new Errors.NotFoundError(`doc not not found: ${urlPath}`)) @@ -188,6 +189,7 @@ module.exports = { 'pathname', 'projectHistoryId', 'historyRangesSupport', + 'resolvedCommentIds', ]), setDoc: promisify(setDoc), }, diff --git a/services/document-updater/app/js/ProjectHistoryRedisManager.js b/services/document-updater/app/js/ProjectHistoryRedisManager.js index 6a2eb5e9f1..d65b98177e 100644 --- a/services/document-updater/app/js/ProjectHistoryRedisManager.js +++ b/services/document-updater/app/js/ProjectHistoryRedisManager.js @@ -135,6 +135,7 @@ const ProjectHistoryRedisManager = { * @param {string} docId * @param {string[]} lines * @param {Ranges} ranges + * @param {string[]} resolvedCommentIds * @param {number} version * @param {string} pathname * @param {boolean} historyRangesSupport @@ -146,6 +147,7 @@ const ProjectHistoryRedisManager = { docId, lines, ranges, + resolvedCommentIds, version, pathname, historyRangesSupport @@ -173,6 +175,7 @@ const ProjectHistoryRedisManager = { if (historyRangesSupport) { projectUpdate.resyncDocContent.ranges = HistoryConversions.toHistoryRanges(ranges) + projectUpdate.resyncDocContent.resolvedCommentIds = resolvedCommentIds } const jsonUpdate = JSON.stringify(projectUpdate) diff --git a/services/document-updater/app/js/RedisManager.js b/services/document-updater/app/js/RedisManager.js index 7cd038794d..ec6e2990b2 100644 --- a/services/document-updater/app/js/RedisManager.js +++ b/services/document-updater/app/js/RedisManager.js @@ -35,6 +35,7 @@ const RedisManager = { docLines, version, ranges, + resolvedCommentIds, pathname, projectHistoryId, historyRangesSupport, @@ -101,6 +102,13 @@ const RedisManager = { }) if (historyRangesSupport) { multi.sadd(keys.historyRangesSupport(), docId) + multi.del(keys.resolvedCommentIds({ doc_id: docId })) + if (resolvedCommentIds.length > 0) { + multi.sadd( + keys.resolvedCommentIds({ doc_id: docId }), + ...resolvedCommentIds + ) + } } multi.exec(callback) } @@ -132,7 +140,8 @@ const RedisManager = { keys.projectHistoryId({ doc_id: docId }), keys.unflushedTime({ doc_id: docId }), keys.lastUpdatedAt({ doc_id: docId }), - keys.lastUpdatedBy({ doc_id: docId }) + keys.lastUpdatedBy({ doc_id: docId }), + keys.resolvedCommentIds({ doc_id: docId }) ) multi.exec((error, response) => { if (error) { @@ -209,70 +218,85 @@ const RedisManager = { if (error) { return callback(error) } - const historyRangesSupport = result === 1 + rclient.smembers( + keys.resolvedCommentIds({ doc_id: docId }), + (error, resolvedCommentIds) => { + if (error) { + return callback(error) + } - const timeSpan = timer.done() - // check if request took too long and bail out. only do this for - // get, because it is the first call in each update, so if this - // passes we'll assume others have a reasonable chance to succeed. - if (timeSpan > MAX_REDIS_REQUEST_LENGTH) { - error = new Error('redis getDoc exceeded timeout') - return callback(error) - } - // record bytes loaded from redis - if (docLines != null) { - metrics.summary('redis.docLines', docLines.length, { status: 'get' }) - } - // check sha1 hash value if present - if (docLines != null && storedHash != null) { - const computedHash = RedisManager._computeHash(docLines) - if (logHashReadErrors && computedHash !== storedHash) { - logger.error( - { - projectId, - docId, - docProjectId, - computedHash, - storedHash, - docLines, - }, - 'hash mismatch on retrieved document' + const historyRangesSupport = result === 1 + + const timeSpan = timer.done() + // check if request took too long and bail out. only do this for + // get, because it is the first call in each update, so if this + // passes we'll assume others have a reasonable chance to succeed. + if (timeSpan > MAX_REDIS_REQUEST_LENGTH) { + error = new Error('redis getDoc exceeded timeout') + return callback(error) + } + // record bytes loaded from redis + if (docLines != null) { + metrics.summary('redis.docLines', docLines.length, { + status: 'get', + }) + } + // check sha1 hash value if present + if (docLines != null && storedHash != null) { + const computedHash = RedisManager._computeHash(docLines) + if (logHashReadErrors && computedHash !== storedHash) { + logger.error( + { + projectId, + docId, + docProjectId, + computedHash, + storedHash, + docLines, + }, + 'hash mismatch on retrieved document' + ) + } + } + + try { + docLines = JSON.parse(docLines) + ranges = RedisManager._deserializeRanges(ranges) + } catch (e) { + return callback(e) + } + + version = parseInt(version || 0, 10) + // check doc is in requested project + if (docProjectId != null && docProjectId !== projectId) { + logger.error( + { projectId, docId, docProjectId }, + 'doc not in project' + ) + return callback(new Errors.NotFoundError('document not found')) + } + + if (docLines && version && !pathname) { + metrics.inc('pathname', 1, { + path: 'RedisManager.getDoc', + status: pathname === '' ? 'zero-length' : 'undefined', + }) + } + + callback( + null, + docLines, + version, + ranges, + pathname, + projectHistoryId, + unflushedTime, + lastUpdatedAt, + lastUpdatedBy, + historyRangesSupport, + resolvedCommentIds ) } - } - - try { - docLines = JSON.parse(docLines) - ranges = RedisManager._deserializeRanges(ranges) - } catch (e) { - return callback(e) - } - - version = parseInt(version || 0, 10) - // check doc is in requested project - if (docProjectId != null && docProjectId !== projectId) { - logger.error({ projectId, docId, docProjectId }, 'doc not in project') - return callback(new Errors.NotFoundError('document not found')) - } - - if (docLines && version && !pathname) { - metrics.inc('pathname', 1, { - path: 'RedisManager.getDoc', - status: pathname === '' ? 'zero-length' : 'undefined', - }) - } - - callback( - null, - docLines, - version, - ranges, - pathname, - projectHistoryId, - unflushedTime, - lastUpdatedAt, - lastUpdatedBy, - historyRangesSupport ) }) }) @@ -513,6 +537,22 @@ const RedisManager = { rclient.del(keys.unflushedTime({ doc_id: docId }), callback) }, + updateCommentState(docId, commentId, resolved, callback) { + if (resolved) { + rclient.sadd( + keys.resolvedCommentIds({ doc_id: docId }), + commentId, + callback + ) + } else { + rclient.srem( + keys.resolvedCommentIds({ doc_id: docId }), + commentId, + callback + ) + } + }, + getDocIdsInProject(projectId, callback) { rclient.smembers(keys.docsInProject({ project_id: projectId }), callback) }, @@ -629,6 +669,7 @@ module.exports.promises = promisifyAll(RedisManager, { 'lastUpdatedAt', 'lastUpdatedBy', 'historyRangesSupport', + 'resolvedCommentIds', ], getNextProjectToFlushAndDelete: [ 'projectId', diff --git a/services/document-updater/config/settings.defaults.js b/services/document-updater/config/settings.defaults.js index f8a3f73f51..c3e7526a98 100755 --- a/services/document-updater/config/settings.defaults.js +++ b/services/document-updater/config/settings.defaults.js @@ -137,6 +137,9 @@ module.exports = { lastUpdatedAt({ doc_id: docId }) { return `lastUpdatedAt:{${docId}}` }, + resolvedCommentIds({ doc_id: docId }) { + return `ResolvedCommentIds:{${docId}}` + }, flushAndDeleteQueue() { return 'DocUpdaterFlushAndDeleteQueue' }, diff --git a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js index 2d3f8cd4ba..58e4aca9e7 100644 --- a/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js +++ b/services/document-updater/test/unit/js/DocumentManager/DocumentManagerTests.js @@ -22,6 +22,7 @@ describe('DocumentManager', function () { putDocInMemory: sinon.stub().resolves(), removeDocFromMemory: sinon.stub().resolves(), renameDoc: sinon.stub().resolves(), + updateCommentState: sinon.stub().resolves(), updateDocument: sinon.stub().resolves(), }, } @@ -73,6 +74,7 @@ describe('DocumentManager', function () { this.lines = ['one', 'two', 'three'] this.version = 42 this.ranges = { comments: 'mock', entries: 'mock' } + this.resolvedCommentIds = ['comment-1'] this.pathname = '/a/b/c.tex' this.unflushedTime = Date.now() this.lastUpdatedAt = Date.now() @@ -170,17 +172,15 @@ describe('DocumentManager', function () { }) it('should write the doc lines to the persistence layer', function () { - this.PersistenceManager.promises.setDoc - .calledWith( - this.project_id, - this.doc_id, - this.lines, - this.version, - this.ranges, - this.lastUpdatedAt, - this.lastUpdatedBy - ) - .should.equal(true) + this.PersistenceManager.promises.setDoc.should.have.been.calledWith( + this.project_id, + this.doc_id, + this.lines, + this.version, + this.ranges, + this.lastUpdatedAt, + this.lastUpdatedBy + ) }) }) @@ -298,6 +298,7 @@ describe('DocumentManager', function () { lines: this.lines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, unflushedTime: this.unflushedTime, @@ -322,6 +323,7 @@ describe('DocumentManager', function () { lines: this.lines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, unflushedTime: this.unflushedTime, @@ -344,6 +346,7 @@ describe('DocumentManager', function () { lines: this.lines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, historyRangesSupport: this.historyRangesSupport, @@ -374,6 +377,7 @@ describe('DocumentManager', function () { this.lines, this.version, this.ranges, + this.resolvedCommentIds, this.pathname, this.projectHistoryId, this.historyRangesSupport @@ -386,6 +390,7 @@ describe('DocumentManager', function () { lines: this.lines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, unflushedTime: null, @@ -409,6 +414,7 @@ describe('DocumentManager', function () { lines: this.beforeLines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, unflushedTime: this.unflushedTime, @@ -428,6 +434,7 @@ describe('DocumentManager', function () { lines: this.beforeLines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, unflushedTime: this.unflushedTime, @@ -746,12 +753,14 @@ describe('DocumentManager', function () { this.version = 34 this.lines = ['original', 'lines'] this.ranges = { comments: ['one', 'two', 'three'] } + this.resolvedCommentIds = ['comment1'] this.updated_ranges = { comments: ['one', 'three'] } this.historyRangesSupport = true this.DocumentManager.promises.getDoc = sinon.stub().resolves({ lines: this.lines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, unflushedTime: Date.now() - 1e9, @@ -797,6 +806,14 @@ describe('DocumentManager', function () { .should.equal(true) }) + it('should unset the comment resolved state', function () { + this.RedisManager.promises.updateCommentState.should.have.been.calledWith( + this.doc_id, + this.comment_id, + false + ) + }) + it('should queue the delete comment operation', function () { this.ProjectHistoryRedisManager.promises.queueOps.should.have.been.calledWith( this.project_id, @@ -985,6 +1002,7 @@ describe('DocumentManager', function () { lines: this.lines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, historyRangesSupport: this.historyRangesSupport, @@ -1010,6 +1028,7 @@ describe('DocumentManager', function () { this.doc_id, this.lines, this.ranges, + this.resolvedCommentIds, this.version, this.pathnameFromProjectStructureUpdate, this.historyRangesSupport @@ -1026,6 +1045,7 @@ describe('DocumentManager', function () { lines: this.lines, version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, pathname: this.pathname, projectHistoryId: this.projectHistoryId, historyRangesSupport: this.historyRangesSupport, @@ -1057,6 +1077,7 @@ describe('DocumentManager', function () { this.doc_id, this.lines, this.ranges, + this.resolvedCommentIds, this.version, this.pathnameFromProjectStructureUpdate, this.historyRangesSupport diff --git a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js index 2718488cad..f176401982 100644 --- a/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js +++ b/services/document-updater/test/unit/js/ProjectHistoryRedisManager/ProjectHistoryRedisManagerTests.js @@ -180,6 +180,7 @@ describe('ProjectHistoryRedisManager', function () { this.ranges = { changes: [{ op: { i: 'ne', p: 1 } }, { op: { d: 'deleted', p: 3 } }], } + this.resolvedCommentIds = ['comment-1'] this.version = 2 this.pathname = '/path' @@ -207,6 +208,7 @@ describe('ProjectHistoryRedisManager', function () { this.doc_id, this.lines, this.ranges, + this.resolvedCommentIds, this.version, this.pathname, false @@ -240,6 +242,7 @@ describe('ProjectHistoryRedisManager', function () { this.doc_id, this.lines, this.ranges, + this.resolvedCommentIds, this.version, this.pathname, false @@ -261,6 +264,7 @@ describe('ProjectHistoryRedisManager', function () { content: 'onedeleted\ntwo', version: this.version, ranges: this.ranges, + resolvedCommentIds: this.resolvedCommentIds, }, projectHistoryId: this.projectHistoryId, path: this.pathname, @@ -274,6 +278,7 @@ describe('ProjectHistoryRedisManager', function () { this.doc_id, this.lines, this.ranges, + this.resolvedCommentIds, this.version, this.pathname, true diff --git a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js index d5d041bb53..173e9aeeeb 100644 --- a/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js +++ b/services/document-updater/test/unit/js/RedisManager/RedisManagerTests.js @@ -66,6 +66,9 @@ describe('RedisManager', function () { historyRangesSupport() { return 'HistoryRangesSupport' }, + resolvedCommentIds({ doc_id: docId }) { + return `ResolvedCommentIds:${docId}` + }, }, }, }, @@ -112,6 +115,7 @@ describe('RedisManager', function () { .update(this.jsonlines, 'utf8') .digest('hex') this.ranges = { comments: 'mock', entries: 'mock' } + this.resolvedCommentIds = ['comment-1'] this.json_ranges = JSON.stringify(this.ranges) this.unflushed_time = 12345 this.pathname = '/a/b/c.tex' @@ -131,6 +135,10 @@ describe('RedisManager', function () { this.rclient.sismember .withArgs('HistoryRangesSupport', this.docId) .yields(null, 0) + this.rclient.smembers = sinon.stub() + this.rclient.smembers + .withArgs(`ResolvedCommentIds:${this.docId}`) + .yields(null, this.resolvedCommentIds) }) describe('successfully', function () { @@ -166,7 +174,8 @@ describe('RedisManager', function () { this.unflushed_time, this.lastUpdatedAt, this.lastUpdatedBy, - this.historyRangesSupport + this.historyRangesSupport, + this.resolvedCommentIds ) }) @@ -274,7 +283,8 @@ describe('RedisManager', function () { this.unflushed_time, this.lastUpdatedAt, this.lastUpdatedBy, - true + true, + this.resolvedCommentIds ) }) }) @@ -771,6 +781,7 @@ describe('RedisManager', function () { beforeEach(function () { this.multi.mset = sinon.stub() this.multi.sadd = sinon.stub() + this.multi.del = sinon.stub() this.rclient.sadd = sinon.stub().yields() this.lines = ['one', 'two', 'three', 'これは'] this.version = 42 @@ -779,6 +790,7 @@ describe('RedisManager', function () { .update(JSON.stringify(this.lines), 'utf8') .digest('hex') this.ranges = { comments: 'mock', entries: 'mock' } + this.resolvedCommentIds = ['comment-1'] this.pathname = '/a/b/c.tex' }) @@ -790,6 +802,7 @@ describe('RedisManager', function () { this.lines, this.version, this.ranges, + this.resolvedCommentIds, this.pathname, this.projectHistoryId, this.historyRangesSupport, @@ -824,6 +837,12 @@ describe('RedisManager', function () { it('should not add the document to the HistoryRangesSupport set in Redis', function () { this.multi.sadd.should.not.have.been.calledWith('HistoryRangesSupport') }) + + it('should not store the resolved comments in Redis', function () { + this.multi.sadd.should.not.have.been.calledWith( + `ResolvedCommentIds:${this.docId}` + ) + }) }) describe('with empty ranges', function () { @@ -834,6 +853,7 @@ describe('RedisManager', function () { this.lines, this.version, {}, + [], this.pathname, this.projectHistoryId, this.historyRangesSupport, @@ -865,6 +885,7 @@ describe('RedisManager', function () { this.lines, this.version, this.ranges, + this.resolvedCommentIds, this.pathname, this.projectHistoryId, this.historyRangesSupport, @@ -898,6 +919,7 @@ describe('RedisManager', function () { this.lines, this.version, this.ranges, + this.resolvedCommentIds, this.pathname, this.projectHistoryId, this.historyRangesSupport, @@ -925,6 +947,7 @@ describe('RedisManager', function () { this.lines, this.version, this.ranges, + this.resolvedCommentIds, this.pathname, this.projectHistoryId, this.historyRangesSupport, @@ -938,6 +961,16 @@ describe('RedisManager', function () { this.docId ) }) + + it('should store the resolved comments in Redis', function () { + this.multi.del.should.have.been.calledWith( + `ResolvedCommentIds:${this.docId}` + ) + this.multi.sadd.should.have.been.calledWith( + `ResolvedCommentIds:${this.docId}`, + ...this.resolvedCommentIds + ) + }) }) }) @@ -966,7 +999,8 @@ describe('RedisManager', function () { `ProjectHistoryId:${this.docId}`, `UnflushedTime:${this.docId}`, `lastUpdatedAt:${this.docId}`, - `lastUpdatedBy:${this.docId}` + `lastUpdatedBy:${this.docId}`, + `ResolvedCommentIds:${this.docId}` ) .should.equal(true) }) diff --git a/services/project-history/app/js/SyncManager.js b/services/project-history/app/js/SyncManager.js index 144f62bc24..8ddc796c03 100644 --- a/services/project-history/app/js/SyncManager.js +++ b/services/project-history/app/js/SyncManager.js @@ -716,8 +716,8 @@ class SyncUpdateExpander { async queueUpdatesForOutOfSyncComments(update, pathname, persistedComments) { const expectedContent = update.resyncDocContent.content const expectedComments = update.resyncDocContent.ranges?.comments ?? [] - const resolvedComments = new Set( - update.resyncDocContent.resolvedComments ?? [] + const resolvedCommentIds = new Set( + update.resyncDocContent.resolvedCommentIds ?? [] ) const expectedCommentsById = new Map( expectedComments.map(comment => [comment.id, comment]) @@ -743,11 +743,11 @@ class SyncUpdateExpander { for (const expectedComment of expectedComments) { const persistedComment = persistedCommentsById.get(expectedComment.id) + const expectedCommentResolved = resolvedCommentIds.has(expectedComment.id) if ( persistedComment != null && commentRangesAreInSync(persistedComment, expectedComment) ) { - const expectedCommentResolved = resolvedComments.has(expectedComment.id) if (expectedCommentResolved === persistedComment.resolved) { // Both comments are identical; do nothing } else { @@ -756,13 +756,19 @@ class SyncUpdateExpander { pathname, commentId: expectedComment.id, resolved: expectedCommentResolved, + meta: { + resync: true, + origin: this.origin, + ts: update.meta.ts, + }, }) } } else { + const op = { ...expectedComment.op, resolved: expectedCommentResolved } // New comment or ranges differ this.expandedUpdates.push({ doc: update.doc, - op: [expectedComment.op], + op: [op], meta: { resync: true, origin: this.origin, diff --git a/services/project-history/app/js/UpdateTranslator.js b/services/project-history/app/js/UpdateTranslator.js index ad5a5ce4ce..2d5b330bb6 100644 --- a/services/project-history/app/js/UpdateTranslator.js +++ b/services/project-history/app/js/UpdateTranslator.js @@ -278,7 +278,7 @@ class OperationsBuilder { this.pushTextOperation() // Add a comment operation - this.operations.push({ + const commentOp = { pathname: this.pathname, commentId: op.t, ranges: [ @@ -287,7 +287,11 @@ class OperationsBuilder { length: op.hlen ?? op.c.length, }, ], - }) + } + if ('resolved' in op) { + commentOp.resolved = op.resolved + } + this.operations.push(commentOp) return } diff --git a/services/project-history/app/js/types.ts b/services/project-history/app/js/types.ts index 4e6c6a88a4..6f787850d2 100644 --- a/services/project-history/app/js/types.ts +++ b/services/project-history/app/js/types.ts @@ -81,7 +81,7 @@ export type ResyncDocContentUpdate = { content: string version: number ranges?: Ranges - resolvedComments?: string[] + resolvedCommentIds?: string[] } projectHistoryId: string path: string @@ -129,6 +129,7 @@ export type CommentOp = { t: string hpos?: number hlen?: number + resolved?: boolean } export type UpdateWithBlob = { diff --git a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js index 0347df979e..4fafd1f2a8 100644 --- a/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js +++ b/services/project-history/test/unit/js/SyncManager/SyncManagerTests.js @@ -24,7 +24,7 @@ function docContentSyncUpdate( doc, content, ranges = {}, - resolvedComments = [] + resolvedCommentIds = [] ) { return { path: doc.path, @@ -33,7 +33,7 @@ function docContentSyncUpdate( resyncDocContent: { content, ranges, - resolvedComments, + resolvedCommentIds, }, meta: { @@ -1096,7 +1096,7 @@ describe('SyncManager', function () { makeComment('comment1', 4, 'quick'), makeComment('comment2', 10, 'brown'), ] - this.resolvedComments = ['comment2'] + this.resolvedCommentIds = ['comment2'] }) it('does nothing if comments have not changed', async function () { @@ -1107,7 +1107,7 @@ describe('SyncManager', function () { { comments: this.comments, }, - this.resolvedComments + this.resolvedCommentIds ), ] const expandedUpdates = @@ -1129,7 +1129,7 @@ describe('SyncManager', function () { { comments: this.comments, }, - this.resolvedComments + this.resolvedCommentIds ), ] const expandedUpdates = @@ -1147,6 +1147,7 @@ describe('SyncManager', function () { c: 'jumps', p: 20, t: 'comment3', + resolved: false, }, ], meta: { @@ -1171,7 +1172,7 @@ describe('SyncManager', function () { { comments: this.comments, }, - this.resolvedComments + this.resolvedCommentIds ), ] const expandedUpdates = @@ -1205,7 +1206,7 @@ describe('SyncManager', function () { { comments: this.comments, }, - this.resolvedComments + this.resolvedCommentIds ), ] const expandedUpdates = @@ -1223,6 +1224,7 @@ describe('SyncManager', function () { c: 'fox', p: 16, t: 'comment2', + resolved: true, }, ], meta: { @@ -1239,7 +1241,7 @@ describe('SyncManager', function () { }) it('sets the resolved state when it differs', async function () { - this.resolvedComments = ['comment1'] + this.resolvedCommentIds = ['comment1'] const updates = [ docContentSyncUpdate( this.persistedDoc, @@ -1247,7 +1249,7 @@ describe('SyncManager', function () { { comments: this.comments, }, - this.resolvedComments + this.resolvedCommentIds ), ] const expandedUpdates = @@ -1262,11 +1264,25 @@ describe('SyncManager', function () { pathname: this.persistedDoc.path, commentId: 'comment1', resolved: true, + meta: { + origin: { + kind: 'history-resync', + }, + resync: true, + ts: TIMESTAMP, + }, }, { pathname: this.persistedDoc.path, commentId: 'comment2', resolved: false, + meta: { + origin: { + kind: 'history-resync', + }, + resync: true, + ts: TIMESTAMP, + }, }, ]) })