Refactor comment management in VCS adapters (#166)

- 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.

Co-authored-by: jdickson <joel.dickson@agoda.com>

Author: joeldickson

.vscode/settings.json

@@ -1,6 +1,6 @@
 {
   "editor.codeActionsOnSave": {
-    "source.fixAll.eslint": true
+    "source.fixAll.eslint": "explicit"
   },
   "typescript.preferences.importModuleSpecifier": "relative"
 }

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<void>;
@@ -14,5 +15,5 @@ export interface VCSAdapter {
     line: number,
     nLines?: number,
   ): Promise<void>;
-  removeExistingComments(): Promise<void>;
+  removeExistingComments(currentComments: Comment[]): Promise<void>;
 }

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) {

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 {

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<string> = new Set();
+  private existingCommentIds: Map<string, number> = new Map();
+  private existingReviewIds: Map<string, number> = new Map();
+
   constructor(private readonly prService: IGitHubPRService) {}
 
   async init(): Promise<void> {
     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<boolean> {
@@ -29,8 +68,14 @@ export class GitHubAdapter implements VCSAdapter {
   diff(): Promise<Diff[]> {
     return this.prService.diff();
   }
+
   createComment(comment: string): Promise<void> {
-    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<void> {
-    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<void> {
-    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<void> {
+    // Create a set of current issue keys
+    const currentIssueKeys = new Set<string>();
+    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<void>[] = [];
 
-    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) {

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 };
 

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<string> = new Set();
+  private existingCommentIds: Map<string, number> = new Map();
+  private existingDiscussionIds: Map<string, string> = new Map();
 
   constructor(private readonly mrService: IGitLabMRService) {}
 
   async init(): Promise<void> {
-    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<void> {
-    Log.debug('Skipping comment removal as we now handle duplicates on creation');
+  async removeExistingComments(currentComments: Comment[]): Promise<void> {
+    // Create a set of current issue keys
+    const currentIssueKeys = new Set<string>();
+    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<void>[] = [];
+
+    // 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');
+    }
   }
 }

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<any[]> {
+    return await this.api.MergeRequestDiscussions.all(this.projectId, this.mrIid);
+  }
+
+  async deleteDiscussion(discussionId: string): Promise<void> {
+    // 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<void> {
     await this.api.MergeRequestNotes.create(this.projectId, this.mrIid, note);

src/Provider/GitLab/IGitLabMRService.ts

@@ -5,6 +5,8 @@ export interface IGitLabMRService {
   createNote(note: string): Promise<void>;
   listAllNotes(): Promise<MergeRequestNoteSchema[]>;
   deleteNote(noteId: number): Promise<void>;
+  listAllDiscussions(): Promise<any[]>;
+  deleteDiscussion(discussionId: string): Promise<void>;
   getCurrentUserId(): Promise<number>;
   diff(): Promise<Diff[]>;
   getLatestVersion(): Promise<MergeRequestDiffVersionsSchema>;