From e970cfcba8d623a1608a3fdd35f92fa5022acb84 Mon Sep 17 00:00:00 2001
From: Marc Shaw <mshaw@gitlab.com>
Date: Mon, 4 Aug 2025 19:50:20 +0200
Subject: [PATCH] Fix nil position error in UpdateDiffPositionService

Add nil check for position parameter before calling SystemNoteService.diff_discussion_outdated
to prevent NoMethodError when position is nil.

The error occurred when the position tracer returned a nil position, which was then passed
to diff_discussion_outdated causing "undefined method `diff_refs' for nil:NilClass" error.

This fix ensures the system note is only created when position is present.
---
 .../system_notes/merge_requests_service.rb    |  7 +++---
 .../update_diff_position_service_spec.rb      | 24 +++++++++++++++++++
 .../merge_requests_service_spec.rb            | 13 ++++++++++
 3 files changed, 41 insertions(+), 3 deletions(-)

diff --git a/app/services/system_notes/merge_requests_service.rb b/app/services/system_notes/merge_requests_service.rb
index 47c35bf01b7b85..743845cf2d1d1e 100644
--- a/app/services/system_notes/merge_requests_service.rb
+++ b/app/services/system_notes/merge_requests_service.rb
@@ -57,12 +57,13 @@ def discussion_continued_in_issue(discussion, issue)
 
     def diff_discussion_outdated(discussion, change_position)
       merge_request = discussion.noteable
-      diff_refs = change_position.diff_refs
+      diff_refs = change_position&.diff_refs
       version_index = merge_request.merge_request_diffs.viewable.count
-      position_on_text = change_position.on_text?
+
+      position_on_text = change_position&.on_text?
       text_parts = ["changed this #{position_on_text ? 'line' : 'file'} in"]
 
-      if version_params = merge_request.version_params_for(diff_refs)
+      if diff_refs && version_params = merge_request.version_params_for(diff_refs)
         repository = project.repository
         anchor = position_on_text ? change_position.line_code(repository) : change_position.file_hash
         url = url_helpers.diffs_project_merge_request_path(project, merge_request, version_params.merge(anchor: anchor))
diff --git a/spec/services/discussions/update_diff_position_service_spec.rb b/spec/services/discussions/update_diff_position_service_spec.rb
index 274430fdccb031..0ed51538a603c4 100644
--- a/spec/services/discussions/update_diff_position_service_spec.rb
+++ b/spec/services/discussions/update_diff_position_service_spec.rb
@@ -223,5 +223,29 @@
         include_examples 'outdated diff note'
       end
     end
+
+    context 'when position tracer returns nil for outdated discussion' do
+      let(:line) { 9 }
+      let(:tracer_double) { instance_double(Gitlab::Diff::PositionTracer) }
+
+      before do
+        allow(Gitlab::Diff::PositionTracer).to receive(:new).and_return(tracer_double)
+        allow(tracer_double).to receive(:trace).and_return({ position: nil, outdated: true })
+      end
+
+      it "doesn't update the position" do
+        subject.execute(discussion)
+
+        expect(discussion.original_position).to eq(old_position)
+        expect(discussion.position).to eq(old_position)
+      end
+
+      it 'creates a system discussion' do
+        expect(SystemNoteService).to receive(:diff_discussion_outdated).with(
+          discussion, project, current_user, nil)
+
+        subject.execute(discussion)
+      end
+    end
   end
 end
diff --git a/spec/services/system_notes/merge_requests_service_spec.rb b/spec/services/system_notes/merge_requests_service_spec.rb
index b0dceacf81f43e..21fe625f8538a8 100644
--- a/spec/services/system_notes/merge_requests_service_spec.rb
+++ b/spec/services/system_notes/merge_requests_service_spec.rb
@@ -177,6 +177,19 @@ def reloaded_merge_request
         expect(subject.note).to eq('changed this line in version 1 of the diff')
       end
     end
+
+    context 'when change position is nil' do
+      let(:change_position) { nil }
+
+      it 'creates a new note in the discussion' do
+        # we need to completely rebuild the merge request object, or the `@discussions` on the merge request are not reloaded.
+        expect { subject }.to change { reloaded_merge_request.discussions.first.notes.size }.by(1)
+      end
+
+      it 'does not create a link' do
+        expect(subject.note).to eq('changed this file in version 1 of the diff')
+      end
+    end
   end
 
   describe '.change_branch' do
-- 
GitLab