Avoid creating reviews on the same diff hunk

ee/lib/gitlab/llm/vertex_ai/completions/review_merge_request.rb

@@ -79,24 +79,37 @@ def build_draft_note_params(response_modifier, diff_file, hunk, diff_refs)
             # create the note on the old line.
             old_line = hunk[:removed].last&.old_pos if hunk[:added].empty?
 
+            position = {
+              base_sha: diff_refs.base_sha,
+              start_sha: diff_refs.start_sha,
+              head_sha: diff_refs.head_sha,
+              old_path: diff_file.old_path,
+              new_path: diff_file.new_path,
+              position_type: 'text',
+              old_line: old_line,
+              new_line: hunk[:added].last&.new_pos,
+              ignore_whitespace_change: false
+            }
+
+            return if review_note_already_exists?(position)
+
             @draft_notes_params << {
               merge_request: merge_request,
               author: review_bot,
               note: response_modifier.response_body,
-              position: {
-                base_sha: diff_refs.base_sha,
-                start_sha: diff_refs.start_sha,
-                head_sha: diff_refs.head_sha,
-                old_path: diff_file.old_path,
-                new_path: diff_file.new_path,
-                position_type: 'text',
-                old_line: old_line,
-                new_line: hunk[:added].last&.new_pos,
-                ignore_whitespace_change: false
-              }
+              position: position
             }
           end
 
+          def review_note_already_exists?(position)
+            merge_request
+              .notes
+              .diff_notes
+              .authored_by(review_bot)
+              .positions
+              .any? { |pos| pos.to_h >= position }
+          end
+
           def create_progress_note
             @progress_note = Notes::CreateService.new(
               merge_request.project,
@@ -108,26 +121,27 @@ def create_progress_note
           end
 
           def update_progress_note_with_review_summary(draft_notes)
-            summary_note = if draft_notes.blank?
-                             s_("DuoCodeReview|I finished my review and found nothing to comment on. Nice work! :tada:")
-                           else
-                             response = summary_response_for(user, draft_notes)
-
-                             if response.errors.any? || response.response_body.blank?
-                               s_("DuoCodeReview|I have encountered some issues while I was reviewing. " \
-                                 "Please try again later.")
-                             else
-                               response.response_body
-                             end
-                           end
-
             Notes::UpdateService.new(
               merge_request.project,
               review_bot,
-              note: summary_note
+              note: summary_note(draft_notes)
             ).execute(@progress_note)
           end
 
+          def summary_note(draft_notes)
+            if draft_notes.blank?
+              s_("DuoCodeReview|I finished my review and found nothing to comment on. Nice work! :tada:")
+            else
+              response = summary_response_for(user, draft_notes)
+
+              if response.errors.any? || response.response_body.blank?
+                s_("DuoCodeReview|I have encountered some issues while I was reviewing. Please try again later.")
+              else
+                response.response_body
+              end
+            end
+          end
+
           def publish_draft_notes
             if @draft_notes_params.empty?
               update_progress_note_with_review_summary([])

ee/spec/lib/gitlab/llm/vertex_ai/completions/review_merge_request_spec.rb

@@ -9,59 +9,59 @@
   let(:options) { {} }
   let(:response_modifier) { double }
   let(:create_note_allowed?) { true }
+
+  let(:review_start_note) do
+    s_("DuoCodeReview|Hey :wave: I'm starting to review your merge request and I will let you know when I'm finished.")
+  end
+
+  let(:review_no_comment_note) do
+    s_("DuoCodeReview|I finished my review and found nothing to comment on. Nice work! :tada:")
+  end
+
+  let(:review_error_note) do
+    s_("DuoCodeReview|I have encountered some issues while I was reviewing. Please try again later.")
+  end
+
   let_it_be(:duo_code_review_bot) { create(:user, :duo_code_review_bot) }
-  let_it_be(:merge_request) { create(:merge_request) }
-  let_it_be(:user) { create(:user, developer_of: merge_request.project) }
-  let_it_be(:diff_refs) { merge_request.diff_refs }
+  let_it_be(:project) do
+    create(:project, :custom_repo, files: { 'UPDATED.md' => "existing line 1\nexisting line 2\n" })
+  end
 
-  let(:ai_reviewable_diff_files) do
-    [
-      instance_double(
-        Gitlab::Diff::File,
-        old_path: 'NEW.md',
-        new_path: 'NEW.md',
-        diff_lines_by_hunk: [
-          {
-            added: [
-              instance_double(Gitlab::Diff::Line, old_pos: 5, new_pos: 3),
-              instance_double(Gitlab::Diff::Line, old_pos: 5, new_pos: 4)
-            ],
-            removed: [
-              instance_double(Gitlab::Diff::Line, old_pos: 3, new_pos: 3),
-              instance_double(Gitlab::Diff::Line, old_pos: 4, new_pos: 3)
-            ]
-          }
-        ]
-      ),
-      instance_double(
-        Gitlab::Diff::File,
-        old_path: 'UPDATED.md',
-        new_path: 'UPDATED.md',
-        diff_lines_by_hunk: [
-          {
-            added: [],
-            removed: [
-              instance_double(Gitlab::Diff::Line, old_pos: 3, new_pos: 3),
-              instance_double(Gitlab::Diff::Line, old_pos: 4, new_pos: 3)
-            ]
-          }
-        ]
-      )
-    ]
+  let_it_be(:user) { create(:user, developer_of: project) }
+  let_it_be(:source_branch) { 'review-merge-request-test' }
+  let_it_be(:merge_request) do
+    project.repository.create_branch(source_branch, project.default_branch)
+    project.repository.update_file(
+      user,
+      'UPDATED.md',
+      "existing line 1\nnew line\n",
+      message: 'Update file',
+      branch_name: source_branch)
+
+    project.repository.create_file(
+      user,
+      'NEW.md',
+      "new line1\nnew line 2\n",
+      message: 'Create file',
+      branch_name: source_branch)
+
+    create(
+      :merge_request,
+      target_project: project,
+      source_project: project,
+      source_branch: source_branch,
+      target_branch: project.default_branch
+    )
   end
 
+  let_it_be(:diff_refs) { merge_request.diff_refs }
+
   let(:review_prompt_message) do
     build(:ai_message, :review_merge_request, user: user, resource: merge_request, request_id: 'uuid')
   end
 
   subject(:completion) { described_class.new(review_prompt_message, review_prompt_class, options) }
 
-  before do
-    allow(merge_request)
-      .to receive(:ai_reviewable_diff_files)
-      .and_return(ai_reviewable_diff_files)
-  end
-
   describe '#execute' do
     let(:review_prompt) { 'This is a review prompt' }
     let(:summary_prompt) { 'This is a summary prompt' }
@@ -147,56 +147,56 @@
         }
       end
 
-      it 'builds draft note and publish to create diff note on new and updated files' do
-        new_file_draft_note_params = {
-          merge_request: merge_request,
-          author: duo_code_review_bot,
-          note: review_answer,
-          position: {
-            base_sha: diff_refs.base_sha,
-            start_sha: diff_refs.start_sha,
-            head_sha: diff_refs.head_sha,
-            old_path: 'NEW.md',
-            new_path: 'NEW.md',
-            position_type: 'text',
-            old_line: nil,
-            new_line: 4,
-            ignore_whitespace_change: false
-          }
-        }
+      it 'creates diff notes on new and updated files' do
+        completion.execute
 
-        updated_file_draft_note_params = {
-          merge_request: merge_request,
-          author: duo_code_review_bot,
-          note: review_answer,
-          position: {
-            base_sha: diff_refs.base_sha,
-            start_sha: diff_refs.start_sha,
-            head_sha: diff_refs.head_sha,
-            old_path: 'UPDATED.md',
-            new_path: 'UPDATED.md',
-            position_type: 'text',
-            old_line: 4,
-            new_line: nil,
-            ignore_whitespace_change: false
-          }
-        }
+        diff_notes = merge_request.notes.diff_notes.authored_by(duo_code_review_bot)
+        expect(diff_notes.count).to eq 2
+
+        new_file_note = diff_notes[0]
+        expect(new_file_note.note).to eq review_answer
+        expect(new_file_note.position.to_h).to eq({
+          base_sha: diff_refs.base_sha,
+          start_sha: diff_refs.start_sha,
+          head_sha: diff_refs.head_sha,
+          old_path: 'NEW.md',
+          new_path: 'NEW.md',
+          position_type: 'text',
+          old_line: nil,
+          new_line: 2,
+          line_range: nil,
+          ignore_whitespace_change: false
+        })
+
+        updated_file_note = diff_notes[1]
+        expect(updated_file_note.note).to eq review_answer
+        expect(updated_file_note.position.to_h).to eq({
+          base_sha: diff_refs.base_sha,
+          start_sha: diff_refs.start_sha,
+          head_sha: diff_refs.head_sha,
+          old_path: 'UPDATED.md',
+          new_path: 'UPDATED.md',
+          position_type: 'text',
+          old_line: nil,
+          new_line: 2,
+          line_range: nil,
+          ignore_whitespace_change: false
+        })
+      end
 
-        draft_note_1 = instance_double(DraftNote)
-        draft_note_2 = instance_double(DraftNote)
-
-        expect(DraftNote).to receive(:new).with(new_file_draft_note_params).and_return(draft_note_1)
-        expect(DraftNote).to receive(:new).with(updated_file_draft_note_params).and_return(draft_note_2)
-        expect(DraftNote).to receive(:bulk_insert!).with([draft_note_1, draft_note_2], batch_size: 20)
-        expect_next_instance_of(
-          DraftNotes::PublishService,
-          merge_request,
-          duo_code_review_bot
-        ) do |svc|
-          expect(svc).to receive(:execute).with(executing_user: user)
-        end
+      context 'when review note alredy exist on the same position' do
+        it 'does not add more notes to the same position' do
+          expect do
+            described_class.new(review_prompt_message, review_prompt_class, options).execute
+          end.to change { merge_request.notes.diff_notes.count }.by(2)
+            .and change { merge_request.notes.non_diff_notes.count }.by(1) # review summary
 
-        completion.execute
+          expect { completion.execute }
+            .to not_change { merge_request.notes.diff_notes.count }
+            .and change { merge_request.notes.non_diff_notes.count }.by(1) # review summary
+
+          expect(merge_request.notes.last.note).to eq(review_no_comment_note)
+        end
       end
 
       context 'when user is not allowed to create notes' do
@@ -216,9 +216,8 @@
         it 'updates progress note with a success message' do
           completion.execute
 
-          expect(Note.count).to eq 1
-          expect(Note.last.note).to eq s_("DuoCodeReview|I finished my review and found nothing to comment on. " \
-            "Nice work! :tada:")
+          expect(merge_request.notes.count).to eq 1
+          expect(merge_request.notes.last.note).to eq(review_no_comment_note)
         end
       end
 
@@ -231,14 +230,13 @@
               merge_request.project,
               duo_code_review_bot,
               noteable: merge_request,
-              note: s_("MergeRequests|Hey :wave: I'm starting to review your merge request and " \
-                "I will let you know when I'm finished.")
+              note: review_start_note
             ).and_call_original
             allow(Notes::CreateService).to receive(:new).and_call_original
 
             completion.execute
 
-            expect(Note.last.note).to eq s_("Helpful review summary")
+            expect(merge_request.notes.non_diff_notes.last.note).to eq s_("Helpful review summary")
           end
         end
 
@@ -255,9 +253,9 @@
           it 'updates progress note with an error message' do
             completion.execute
 
-            expect(Note.count).to eq 1
-            expect(Note.last.note).to eq s_("DuoCodeReview|I have encountered some issues while I was reviewing. " \
-              "Please try again later.")
+            expect(merge_request.notes.count).to eq 3
+            expect(merge_request.notes.diff_notes.count).to eq 2
+            expect(merge_request.notes.non_diff_notes.last.note).to eq(review_error_note)
           end
         end
 
@@ -267,9 +265,9 @@
           it 'updates progress note with an error message' do
             completion.execute
 
-            expect(Note.count).to eq 1
-            expect(Note.last.note).to eq s_("DuoCodeReview|I have encountered some issues while I was reviewing. " \
-              "Please try again later.")
+            expect(merge_request.notes.count).to eq 3
+            expect(merge_request.notes.diff_notes.count).to eq 2
+            expect(merge_request.notes.non_diff_notes.last.note).to eq(review_error_note)
           end
         end
       end
@@ -279,37 +277,23 @@
           stub_const("#{described_class}::DRAFT_NOTES_COUNT_LIMIT", 1)
         end
 
-        it 'builds draft note and publish to create diff note on new and updated files' do
-          new_file_draft_note_params = {
-            merge_request: merge_request,
-            author: duo_code_review_bot,
-            note: review_answer,
-            position: {
-              base_sha: diff_refs.base_sha,
-              start_sha: diff_refs.start_sha,
-              head_sha: diff_refs.head_sha,
-              old_path: 'NEW.md',
-              new_path: 'NEW.md',
-              position_type: 'text',
-              old_line: nil,
-              new_line: 4,
-              ignore_whitespace_change: false
-            }
-          }
-
-          draft_note_1 = instance_double(DraftNote)
-
-          expect(DraftNote).to receive(:new).with(new_file_draft_note_params).and_return(draft_note_1)
-          expect(DraftNote).to receive(:bulk_insert!).with([draft_note_1], batch_size: 20)
-          expect_next_instance_of(
-            DraftNotes::PublishService,
-            merge_request,
-            duo_code_review_bot
-          ) do |svc|
-            expect(svc).to receive(:execute).with(executing_user: user)
-          end
-
+        it 'creates diff note on the first file only' do
           completion.execute
+          diff_notes = merge_request.notes.diff_notes
+          expect(diff_notes.count).to eq 1
+
+          expect(diff_notes[0].position.to_h).to eq({
+            base_sha: diff_refs.base_sha,
+            start_sha: diff_refs.start_sha,
+            head_sha: diff_refs.head_sha,
+            old_path: 'NEW.md',
+            new_path: 'NEW.md',
+            position_type: 'text',
+            old_line: nil,
+            new_line: 2,
+            line_range: nil,
+            ignore_whitespace_change: false
+          })
         end
       end
     end