From 6237d8328ba09d83162c26c1c76acd4388165439 Mon Sep 17 00:00:00 2001 From: jdickson Date: Fri, 27 Jun 2025 11:41:39 +0700 Subject: [PATCH] Refactor comment management in VCS adapters - Update ESLint setting to use explicit fixing. - Modify VCSAdapter interface to require current comments for removal. - Enhance GitHubAdapter and GitLabAdapter to manage existing comments and discussions, preventing duplicates during creation. - Implement selective deletion of outdated comments based on current issues in both adapters. - Add new methods in GitLabMRService for handling discussions. This improves comment handling and ensures that only relevant comments are maintained. --- .vscode/settings.json | 2 +- src/Provider/@interfaces/VCSAdapter.ts | 3 +- src/Provider/CommonVCS/VCSEngine.ts | 2 +- src/Provider/GitHub/GitHub.spec.ts | 10 ++- src/Provider/GitHub/GitHubAdapter.ts | 103 ++++++++++++++++++++---- src/Provider/GitLab/GitLab.spec.ts | 15 ++++ src/Provider/GitLab/GitLabAdapter.ts | 75 ++++++++++++++--- src/Provider/GitLab/GitLabMRService.ts | 20 +++++ src/Provider/GitLab/IGitLabMRService.ts | 2 + 9 files changed, 200 insertions(+), 32 deletions(-) diff --git a/.vscode/settings.json b/.vscode/settings.json index aab8069..a03ac94 100644 --- a/.vscode/settings.json +++ b/.vscode/settings.json @@ -1,6 +1,6 @@ { "editor.codeActionsOnSave": { - "source.fixAll.eslint": true + "source.fixAll.eslint": "explicit" }, "typescript.preferences.importModuleSpecifier": "relative" } diff --git a/src/Provider/@interfaces/VCSAdapter.ts b/src/Provider/@interfaces/VCSAdapter.ts index b9735b9..02493bf 100644 --- a/src/Provider/@interfaces/VCSAdapter.ts +++ b/src/Provider/@interfaces/VCSAdapter.ts @@ -1,5 +1,6 @@ import { Diff } from '../../Git/@types/PatchTypes'; import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot'; +import { Comment } from '../../AnalyzerBot/@types/CommentTypes'; export interface VCSAdapter { init(): Promise; @@ -14,5 +15,5 @@ export interface VCSAdapter { line: number, nLines?: number, ): Promise; - removeExistingComments(): Promise; + removeExistingComments(currentComments: Comment[]): Promise; } diff --git a/src/Provider/CommonVCS/VCSEngine.ts b/src/Provider/CommonVCS/VCSEngine.ts index ecb6d70..d91754d 100644 --- a/src/Provider/CommonVCS/VCSEngine.ts +++ b/src/Provider/CommonVCS/VCSEngine.ts @@ -19,7 +19,7 @@ export class VCSEngine implements VCS { await this.setup(items); if (this.config.removeOldComment) { - await this.adapter.removeExistingComments(); + await this.adapter.removeExistingComments(this.analyzerBot.comments); } if (!this.config.silent) { diff --git a/src/Provider/GitHub/GitHub.spec.ts b/src/Provider/GitHub/GitHub.spec.ts index 7c452ba..d17602d 100644 --- a/src/Provider/GitHub/GitHub.spec.ts +++ b/src/Provider/GitHub/GitHub.spec.ts @@ -22,11 +22,17 @@ const mockedUserId = 1234; const mockedSha = '123456'; const mockedReviews = [ - { id: mockedReviewId, user: { id: mockedUserId } }, + { + id: mockedReviewId, + user: { id: mockedUserId }, + body: 'test review comment', + path: 'test.js', + line: 1, + }, ] as PullsListReviewCommentsResponseData; const mockedComments = [ - { id: mockedCommentId, user: { id: mockedUserId } }, + { id: mockedCommentId, user: { id: mockedUserId }, body: 'test comment' }, ] as IssuesListCommentsResponseData; class PrServiceMock implements IGitHubPRService { diff --git a/src/Provider/GitHub/GitHubAdapter.ts b/src/Provider/GitHub/GitHubAdapter.ts index 9e790ed..f1e41fd 100644 --- a/src/Provider/GitHub/GitHubAdapter.ts +++ b/src/Provider/GitHub/GitHubAdapter.ts @@ -4,13 +4,52 @@ import { Diff } from '../../Git/@types/PatchTypes'; import { CommitStatus } from './CommitStatus'; import { Log } from '../../Logger'; import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot'; +import { Comment } from '../../AnalyzerBot/@types/CommentTypes'; export class GitHubAdapter implements VCSAdapter { private commitId: string; + private existingComments: Set = new Set(); + private existingCommentIds: Map = new Map(); + private existingReviewIds: Map = new Map(); + constructor(private readonly prService: IGitHubPRService) {} async init(): Promise { this.commitId = await this.prService.getLatestCommitSha(); + + // Store existing comments for duplicate detection + const [userId, comments, reviews] = await Promise.all([ + this.prService.getCurrentUserId(), + this.prService.listAllComments(), + this.prService.listAllReviewComments(), + ]); + + // Store regular comments + comments + .filter((comment) => comment.user?.id === userId) + .forEach((comment) => { + if (comment.body) { + this.existingComments.add(comment.body); + this.existingCommentIds.set(comment.body, comment.id); + } + }); + + // Store review comments + reviews + .filter((review) => review.user?.id === userId) + .forEach((review) => { + const key = this.generateCommentKey( + review.path || '', + review.line || 0, + review.body, + ); + this.existingComments.add(key); + this.existingReviewIds.set(key, review.id); + }); + } + + private generateCommentKey(file: string, line: number, text: string): string { + return `${file}:${line}:${text}`; } async wrapUp(analyzer: IAnalyzerBot): Promise { @@ -29,8 +68,14 @@ export class GitHubAdapter implements VCSAdapter { diff(): Promise { return this.prService.diff(); } + createComment(comment: string): Promise { - return this.prService.createComment(comment); + if (!this.existingComments.has(comment)) { + return this.prService.createComment(comment); + } else { + Log.debug('Skipped creating duplicate comment'); + return Promise.resolve(); + } } createReviewComment( @@ -39,27 +84,51 @@ export class GitHubAdapter implements VCSAdapter { line: number, nLines: number, ): Promise { - return this.prService.createReviewComment(this.commitId, text, file, line, nLines); + const commentKey = this.generateCommentKey(file, line, text); + + if (!this.existingComments.has(commentKey)) { + return this.prService.createReviewComment(this.commitId, text, file, line, nLines); + } else { + Log.debug('Skipped creating duplicate review comment'); + return Promise.resolve(); + } } - async removeExistingComments(): Promise { - const [userId, comments, reviews] = await Promise.all([ - this.prService.getCurrentUserId(), - this.prService.listAllComments(), - this.prService.listAllReviewComments(), - ]); - Log.debug('Get existing CodeCoach comments completed'); + async removeExistingComments(currentComments: Comment[]): Promise { + // Create a set of current issue keys + const currentIssueKeys = new Set(); + currentComments.forEach((comment) => { + const key = this.generateCommentKey(comment.file, comment.line, comment.text); + currentIssueKeys.add(key); + }); - const deleteComments = comments - .filter((comment) => comment.user?.id === userId) - .map((comment) => this.prService.deleteComment(comment.id)); + // Delete comments that are no longer relevant + const commentsToDelete: Promise[] = []; - const deleteReviews = reviews - .filter((review) => review.user?.id === userId) - .map((review) => this.prService.deleteReviewComment(review.id)); + // Check regular comments + this.existingCommentIds.forEach((commentId, commentText) => { + if (!currentIssueKeys.has(commentText)) { + commentsToDelete.push(this.prService.deleteComment(commentId)); + this.existingComments.delete(commentText); + this.existingCommentIds.delete(commentText); + } + }); + + // Check review comments + this.existingReviewIds.forEach((reviewId, commentKey) => { + if (!currentIssueKeys.has(commentKey)) { + commentsToDelete.push(this.prService.deleteReviewComment(reviewId)); + this.existingComments.delete(commentKey); + this.existingReviewIds.delete(commentKey); + } + }); - await Promise.all([...deleteComments, ...deleteReviews]); - Log.debug('Delete CodeCoach comments completed'); + if (commentsToDelete.length > 0) { + await Promise.all(commentsToDelete); + Log.debug(`Deleted ${commentsToDelete.length} outdated comments`); + } else { + Log.debug('No outdated comments to delete'); + } } private async setCommitStatus(analyzer: IAnalyzerBot) { diff --git a/src/Provider/GitLab/GitLab.spec.ts b/src/Provider/GitLab/GitLab.spec.ts index 72e524d..5311f4a 100644 --- a/src/Provider/GitLab/GitLab.spec.ts +++ b/src/Provider/GitLab/GitLab.spec.ts @@ -42,6 +42,8 @@ class MrServiceMock implements IGitLabMRService { createMRDiscussion = jest.fn().mockResolvedValue(undefined); createNote = jest.fn().mockResolvedValue(undefined); deleteNote = jest.fn().mockResolvedValue(undefined); + listAllDiscussions = jest.fn().mockResolvedValue([]); + deleteDiscussion = jest.fn().mockResolvedValue(undefined); diff = jest.fn().mockResolvedValue([mockTouchDiff]); getCurrentUserId = jest.fn().mockResolvedValue(mockCurrentUserId); getLatestVersion = jest.fn().mockResolvedValue(mockVersion); @@ -148,6 +150,19 @@ describe('VCS: GitLab', () => { expect(service.createNote).not.toHaveBeenCalled(); }); + it('should remove existing comments when removeOldComment is enabled', async () => { + const service = new MrServiceMock(); + const configsWithRemoveOldComment = { ...configs, removeOldComment: true }; + const gitLab = createGitLab(service, configsWithRemoveOldComment); + + await gitLab.report([touchFileWarning]); + + // With the new approach, we use selective deletion based on current issues + expect(service.listAllNotes).toHaveBeenCalled(); + expect(service.listAllDiscussions).toHaveBeenCalled(); + // deleteNote and deleteDiscussion will be called for outdated comments + }); + describe('when failOnWarnings is true', () => { const warningConfigs = { ...configs, failOnWarnings: true }; diff --git a/src/Provider/GitLab/GitLabAdapter.ts b/src/Provider/GitLab/GitLabAdapter.ts index 1aed012..c5cc836 100644 --- a/src/Provider/GitLab/GitLabAdapter.ts +++ b/src/Provider/GitLab/GitLabAdapter.ts @@ -4,38 +4,60 @@ import { Log } from '../../Logger'; import { IGitLabMRService } from './IGitLabMRService'; import { MergeRequestDiffVersionsSchema, MergeRequestNoteSchema } from '@gitbeaker/core'; import { IAnalyzerBot } from '../../AnalyzerBot/@interfaces/IAnalyzerBot'; +import { Comment } from '../../AnalyzerBot/@types/CommentTypes'; export class GitLabAdapter implements VCSAdapter { private latestMrVersion: MergeRequestDiffVersionsSchema; private existingComments: Set = new Set(); + private existingCommentIds: Map = new Map(); + private existingDiscussionIds: Map = new Map(); constructor(private readonly mrService: IGitLabMRService) {} async init(): Promise { - const [latestVersion, userId, notes] = await Promise.all([ + const [latestVersion, userId, notes, discussions] = await Promise.all([ this.mrService.getLatestVersion(), this.mrService.getCurrentUserId(), this.mrService.listAllNotes(), + this.mrService.listAllDiscussions(), ]); this.latestMrVersion = latestVersion; - // Store existing bot comments + // Store existing bot comments and their IDs notes .filter( (note: { author: { id: any }; system: any }) => note.author.id === userId && !note.system, ) - .forEach((note: { body: string }) => this.existingComments.add(note.body)); + .forEach((note: { id: number; body: string }) => { + this.existingComments.add(note.body); + this.existingCommentIds.set(note.body, note.id); + }); + + // Store existing discussions and their IDs + discussions + .filter( + (discussion: { notes: any[] }) => + discussion.notes && + discussion.notes.some( + (note: { author: { id: any } }) => note.author.id === userId, + ), + ) + .forEach((discussion: { id: string; notes: any[] }) => { + discussion.notes + .filter((note: { author: { id: any } }) => note.author.id === userId) + .forEach((note: { body: string }) => { + const commentKey = this.generateCommentKey('', 0, note.body); + this.existingComments.add(commentKey); + this.existingDiscussionIds.set(commentKey, discussion.id); + }); + }); Log.debug(`Found ${this.existingComments.size} existing CodeCoach comments`); } - private generateCommentKey( - file: string, - line: number | undefined, - text: string, - ): string { + private generateCommentKey(file: string, line: number, text: string): string { return `${file}:${line}:${text}`; } @@ -82,7 +104,40 @@ export class GitLabAdapter implements VCSAdapter { return this.mrService.diff(); } - async removeExistingComments(): Promise { - Log.debug('Skipping comment removal as we now handle duplicates on creation'); + async removeExistingComments(currentComments: Comment[]): Promise { + // Create a set of current issue keys + const currentIssueKeys = new Set(); + currentComments.forEach((comment) => { + const key = this.generateCommentKey(comment.file, comment.line, comment.text); + currentIssueKeys.add(key); + }); + + // Delete comments that are no longer relevant + const commentsToDelete: Promise[] = []; + + // Check regular comments + this.existingCommentIds.forEach((commentId, commentText) => { + if (!currentIssueKeys.has(commentText)) { + commentsToDelete.push(this.mrService.deleteNote(commentId)); + this.existingComments.delete(commentText); + this.existingCommentIds.delete(commentText); + } + }); + + // Check discussion comments + this.existingDiscussionIds.forEach((discussionId, commentKey) => { + if (!currentIssueKeys.has(commentKey)) { + commentsToDelete.push(this.mrService.deleteDiscussion(discussionId)); + this.existingComments.delete(commentKey); + this.existingDiscussionIds.delete(commentKey); + } + }); + + if (commentsToDelete.length > 0) { + await Promise.all(commentsToDelete); + Log.debug(`Deleted ${commentsToDelete.length} outdated comments`); + } else { + Log.debug('No outdated comments to delete'); + } } } diff --git a/src/Provider/GitLab/GitLabMRService.ts b/src/Provider/GitLab/GitLabMRService.ts index e8e8c49..82ec48e 100644 --- a/src/Provider/GitLab/GitLabMRService.ts +++ b/src/Provider/GitLab/GitLabMRService.ts @@ -62,6 +62,26 @@ export class GitLabMRService implements IGitLabMRService { await this.api.MergeRequestNotes.remove(this.projectId, this.mrIid, noteId); } + async listAllDiscussions(): Promise { + return await this.api.MergeRequestDiscussions.all(this.projectId, this.mrIid); + } + + async deleteDiscussion(discussionId: string): Promise { + // GitLab discussions are deleted by removing all notes in the discussion + // We need to get the discussion and remove its notes + const discussion = await this.api.MergeRequestDiscussions.show( + this.projectId, + this.mrIid, + discussionId, + ); + if (discussion.notes && discussion.notes.length > 0) { + const deletePromises = discussion.notes.map((note: any) => + this.api.MergeRequestNotes.remove(this.projectId, this.mrIid, note.id), + ); + await Promise.all(deletePromises); + } + } + // github can do someone fancy shit here we cant async createNote(note: string): Promise { await this.api.MergeRequestNotes.create(this.projectId, this.mrIid, note); diff --git a/src/Provider/GitLab/IGitLabMRService.ts b/src/Provider/GitLab/IGitLabMRService.ts index 5824f58..ee4d1f7 100644 --- a/src/Provider/GitLab/IGitLabMRService.ts +++ b/src/Provider/GitLab/IGitLabMRService.ts @@ -5,6 +5,8 @@ export interface IGitLabMRService { createNote(note: string): Promise; listAllNotes(): Promise; deleteNote(noteId: number): Promise; + listAllDiscussions(): Promise; + deleteDiscussion(discussionId: string): Promise; getCurrentUserId(): Promise; diff(): Promise; getLatestVersion(): Promise;