From 7be49c1280402314e62c063ca677fe497ea86674 Mon Sep 17 00:00:00 2001 From: agent_coder Date: Sat, 11 Jul 2026 17:53:41 +0300 Subject: [PATCH] =?UTF-8?q?fix(comments):=20=D0=B4=D0=B5=D0=B4=D1=83=D0=BF?= =?UTF-8?q?=20=D0=B4=D0=B2=D0=BE=D0=B9=D0=BD=D0=BE=D0=B3=D0=BE=20WS-broadc?= =?UTF-8?q?ast=20=D0=BF=D1=80=D0=B8=20apply=20=D1=82=D1=80=D0=B5=D0=B4?= =?UTF-8?q?=D0=B0=20=D1=81=20=D0=BE=D1=82=D0=B2=D0=B5=D1=82=D0=B0=D0=BC?= =?UTF-8?q?=D0=B8=20(#496)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit finalizeAppliedSuggestion на ветке «есть ответы» вызывал resolveComment (бродкаст commentResolved с обогащённой строкой), а затем сам слал ещё и commentUpdated — клиент получал два события на один apply. Теперь commentUpdated шлётся только когда resolveComment НЕ вызывался (редкий повторный вход по уже разрешённому треду), иначе один бродкаст. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../comment.service.apply-suggestion.spec.ts | 42 ++++++++++++++++++- .../src/core/comment/comment.service.ts | 21 +++++++--- 2 files changed, 56 insertions(+), 7 deletions(-) diff --git a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts index bcfc2f2c..100dae67 100644 --- a/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts +++ b/apps/server/src/core/comment/comment.service.apply-suggestion.spec.ts @@ -197,17 +197,25 @@ describe('CommentService — applySuggestion', () => { expect(resolvePatch.resolvedAt).toBeInstanceOf(Date); expect(resolvePatch.resolvedById).toBe('user-1'); - // NOT deleted; broadcast an update, not a deletion. + // NOT deleted. expect(commentRepo.deleteComment).not.toHaveBeenCalled(); expect(collaborationGateway.handleYjsEvent).not.toHaveBeenCalledWith( 'deleteCommentMark', expect.anything(), expect.anything(), ); + // #496 dedup: resolveComment broadcasts `commentResolved` with the enriched + // row; finalize must NOT ALSO emit a redundant `commentUpdated`. So the + // thread receives exactly ONE resolve broadcast and no update broadcast. expect(wsService.emitCommentEvent).toHaveBeenCalledWith( 'space-1', 'page-1', - expect.objectContaining({ operation: 'commentUpdated', comment: UPDATED }), + expect.objectContaining({ operation: 'commentResolved', comment: UPDATED }), + ); + expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentUpdated' }), ); expect(auditService.log).toHaveBeenCalledWith( @@ -219,6 +227,36 @@ describe('CommentService — applySuggestion', () => { expect(result.outcome).toBe('resolved'); }); + it('re-entry: already applied+resolved WITH replies → emits commentUpdated (dedup does not over-suppress)', async () => { + // suggestionAppliedAt set → idempotent finalize; resolvedAt set → resolveComment + // is skipped, so there is NO commentResolved broadcast. The applied-stamp state + // must still reach clients via a single commentUpdated. + const { service, wsService } = makeService( + { applied: false, currentText: 'new text' }, + true, + ); + + await service.applySuggestion( + suggestionComment({ + suggestionAppliedAt: new Date(), + resolvedAt: new Date(), + }), + user(), + ); + + expect(wsService.emitCommentEvent).toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentUpdated', comment: UPDATED }), + ); + // Nothing resolved this time (already resolved) → no resolve broadcast. + expect(wsService.emitCommentEvent).not.toHaveBeenCalledWith( + 'space-1', + 'page-1', + expect.objectContaining({ operation: 'commentResolved' }), + ); + }); + // --- error / rejection branches ----------------------------------------- it('applied=false and currentText differs → ConflictException with currentText in payload', async () => { diff --git a/apps/server/src/core/comment/comment.service.ts b/apps/server/src/core/comment/comment.service.ts index c053ccf8..88122e96 100644 --- a/apps/server/src/core/comment/comment.service.ts +++ b/apps/server/src/core/comment/comment.service.ts @@ -577,8 +577,10 @@ export class CommentService { // Auto-resolve the thread. resolveComment handles the resolve mark, its ws // broadcast and the resolve notification. Stay defensive on re-entry. + let didResolveBroadcast = false; if (!comment.resolvedAt) { await this.resolveComment(comment, true, user, provenance); + didResolveBroadcast = true; } const updatedComment = await this.commentRepo.findById(comment.id, { @@ -586,11 +588,20 @@ export class CommentService { includeResolvedBy: true, }); - this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { - operation: 'commentUpdated', - pageId: comment.pageId, - comment: updatedComment, - }); + // #496 dedup: resolveComment already broadcast `commentResolved` carrying + // the fully-enriched row (the applied stamps were persisted above, before + // that call, so its re-read reflects them). Emitting `commentUpdated` here + // too made the client receive TWO events for one apply. Broadcast the + // update ONLY when we did NOT resolve — i.e. the rare re-entry on an + // already-resolved thread, where the applied-stamp change still needs a + // broadcast and resolveComment did not run. + if (!didResolveBroadcast) { + this.wsService.emitCommentEvent(comment.spaceId, comment.pageId, { + operation: 'commentUpdated', + pageId: comment.pageId, + comment: updatedComment, + }); + } this.auditService.log({ event: AuditEvent.COMMENT_SUGGESTION_APPLIED,