From 0d5ec109a25719d8e752a0d656d27038d7a88fe7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 00:07:28 -0600 Subject: [PATCH 01/57] Get the Merge Request into the Diff File Header component --- app/components/rapid_diffs/diff_file_component.html.haml | 2 +- app/components/rapid_diffs/diff_file_component.rb | 7 +++++++ app/components/rapid_diffs/diff_file_header_component.rb | 3 ++- .../merge_request_diff_file_component.html.haml | 5 ++++- 4 files changed, 14 insertions(+), 3 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_component.html.haml b/app/components/rapid_diffs/diff_file_component.html.haml index 65f6a9948e6549..d770d97314b525 100644 --- a/app/components/rapid_diffs/diff_file_component.html.haml +++ b/app/components/rapid_diffs/diff_file_component.html.haml @@ -2,7 +2,7 @@ %diff-file.rd-diff-file-component{ id: id, data: { testid: 'rd-diff-file', **server_data } } .rd-diff-file - = render RapidDiffs::DiffFileHeaderComponent.new(diff_file: @diff_file) + = header || default_header -# extra wrapper needed so content-visibility: hidden doesn't require removing border or other styles %div{ data: { file_body: '' } } .rd-diff-file-body diff --git a/app/components/rapid_diffs/diff_file_component.rb b/app/components/rapid_diffs/diff_file_component.rb index a8a5bf7ca1de69..7a04f5ccc97756 100644 --- a/app/components/rapid_diffs/diff_file_component.rb +++ b/app/components/rapid_diffs/diff_file_component.rb @@ -4,6 +4,8 @@ module RapidDiffs class DiffFileComponent < ViewComponent::Base include TreeHelper + renders_one :header + def initialize(diff_file:, parallel_view: false) @diff_file = diff_file @parallel_view = parallel_view @@ -33,5 +35,10 @@ def viewer_component Viewers::NoPreviewComponent end + + def default_header + render RapidDiffs::DiffFileHeaderComponent.new(diff_file: @diff_file) + end + end end diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 5b0a88002c2d88..b7e7adaa6791cc 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -4,8 +4,9 @@ module RapidDiffs class DiffFileHeaderComponent < ViewComponent::Base include ButtonHelper - def initialize(diff_file:) + def initialize(diff_file:, merge_request: nil) @diff_file = diff_file + @merge_request = merge_request end def copy_path_button diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml index fbb39bb1afe3bd..1907bde18da2aa 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml +++ b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml @@ -1 +1,4 @@ -= render RapidDiffs::DiffFileComponent.new(diff_file: @diff_file, parallel_view: @parallel_view) += render RapidDiffs::DiffFileComponent.new(diff_file: @diff_file, parallel_view: @parallel_view) do |c| + - c.with_header do + = render RapidDiffs::DiffFileHeaderComponent.new(diff_file: @diff_file, merge_request: @merge_request) + end -- GitLab From 048735c28c5d1439a7da1136d7648b12948c3f3d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 00:32:43 -0600 Subject: [PATCH 02/57] Add the actual single file editor href --- .../rapid_diffs/diff_file_header_component.html.haml | 4 +++- .../rapid_diffs/diff_file_header_component.rb | 10 ++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.html.haml b/app/components/rapid_diffs/diff_file_header_component.html.haml index bfe99e0d7d6170..03b85f5ab2a4b5 100644 --- a/app/components/rapid_diffs/diff_file_header_component.html.haml +++ b/app/components/rapid_diffs/diff_file_header_component.html.haml @@ -13,7 +13,9 @@ - view_title = _('View file @ %{commitSha}') % { commitSha: Commit.truncate_sha(@diff_file.content_sha) } - view_href = project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) -- options_menu_items = [{ "text": "#{view_title}", "href": "#{view_href}" }].to_json +- edit_title = _('Edit in single-file editor') +- edit_href = single_file_editor_path +- options_menu_items = [{ "text": "#{view_title}", "href": "#{view_href}" },{ "text": "#{edit_title}", "href": "#{edit_href}" }].to_json .rd-diff-file-header{ data: { testid: 'rd-diff-file-header' } } .rd-diff-file-toggle< diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index b7e7adaa6791cc..995994bb441576 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -19,5 +19,15 @@ def copy_path_button testid: "rd-diff-file-copy-clipboard" ) end + + def single_file_editor_path + url_for( + helpers.edit_blob_path( + @diff_file.repository.project, + @merge_request.source_branch, + @diff_file.new_path + ) + "?from_merge_request_iid=#{@merge_request.iid}" + ) + end end end -- GitLab From 716ef6123b0c26cb094bc55ba1cb0ec0d95ff9b4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 19:57:04 -0600 Subject: [PATCH 03/57] Remove blank line --- app/components/rapid_diffs/diff_file_component.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/components/rapid_diffs/diff_file_component.rb b/app/components/rapid_diffs/diff_file_component.rb index 7a04f5ccc97756..f109b5f5c7cdb0 100644 --- a/app/components/rapid_diffs/diff_file_component.rb +++ b/app/components/rapid_diffs/diff_file_component.rb @@ -39,6 +39,5 @@ def viewer_component def default_header render RapidDiffs::DiffFileHeaderComponent.new(diff_file: @diff_file) end - end end -- GitLab From 652e019efce965bd07e6405b66d5155fa1d9d2a3 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 22:34:37 -0600 Subject: [PATCH 04/57] Protect against potential nil merge_request in non-MR uses --- app/components/rapid_diffs/diff_file_header_component.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 995994bb441576..9085c4d09a8ec1 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -21,6 +21,8 @@ def copy_path_button end def single_file_editor_path + return unless @merge_request && merge_request.source_branch + url_for( helpers.edit_blob_path( @diff_file.repository.project, -- GitLab From c83fc6d9974cbbe3709673c3cc682780925edbde Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 22:51:09 -0600 Subject: [PATCH 05/57] Move the menu options generation into the component class --- .../diff_file_header_component.html.haml | 8 +------- .../rapid_diffs/diff_file_header_component.rb | 18 ++++++++++++++++++ 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.html.haml b/app/components/rapid_diffs/diff_file_header_component.html.haml index 03b85f5ab2a4b5..6174ec73dd3960 100644 --- a/app/components/rapid_diffs/diff_file_header_component.html.haml +++ b/app/components/rapid_diffs/diff_file_header_component.html.haml @@ -11,12 +11,6 @@ -# * toggle file comments -# * submodule compare -- view_title = _('View file @ %{commitSha}') % { commitSha: Commit.truncate_sha(@diff_file.content_sha) } -- view_href = project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) -- edit_title = _('Edit in single-file editor') -- edit_href = single_file_editor_path -- options_menu_items = [{ "text": "#{view_title}", "href": "#{view_href}" },{ "text": "#{edit_title}", "href": "#{edit_href}" }].to_json - .rd-diff-file-header{ data: { testid: 'rd-diff-file-header' } } .rd-diff-file-toggle< = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, icon: 'chevron-down', button_options: { class: 'rd-diff-file-toggle-button', data: { opened: '', click: 'toggleFile' }, aria: { label: _('Hide file contents') } }) @@ -61,6 +55,6 @@ -# https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182850#note_2387011092 -# haml-lint:disable InlineJavaScript %script{ type: "application/json" } - = options_menu_items.html_safe + = menu_items.to_json.html_safe - button_params = { icon: 'ellipsis_v', button_options: { data: { click: 'toggleOptionsMenu' }, aria: { label: _('Options') } } } = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, **button_params) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 9085c4d09a8ec1..1b2219238c77d8 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -31,5 +31,23 @@ def single_file_editor_path ) + "?from_merge_request_iid=#{@merge_request.iid}" ) end + + def menu_items + items = [ + { + "text": _('View file @ %{commitSha}') % { commitSha: Commit.truncate_sha(@diff_file.content_sha) }, + "href": helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) + } + ] + + if single_file_editor_path() do + items.push( { + "text": _('Edit in single-file editor'), + "href": single_file_editor_path() + } ) + end + + items + end end end -- GitLab From 675b43f22ce48d90f7bff97c19fc2f3f4a7fb34c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 22:51:37 -0600 Subject: [PATCH 06/57] Test the new changes to the diff file component (etc.) --- .../rapid_diffs/diff_file_component_spec.rb | 34 ++++++++ .../diff_file_header_component_spec.rb | 79 +++++++++++++++++++ .../merge_request_diff_file_component_spec.rb | 25 ++++++ 3 files changed, 138 insertions(+) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index 2afaff289976b9..c593618604fcb3 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -10,4 +10,38 @@ def render_component(**args) render_inline(described_class.new(diff_file: diff_file, **args)) end + + describe 'header slot' do + let(:diff_file) { create(:diff_file) } + + it 'renders the default header when no custom header is provided' do + component = described_class.new(diff_file: diff_file) + + expect(component).to receive(:default_header) + + render_inline(component) + end + + it 'renders a custom header when provided' do + custom_header = '
Custom Header
' + + result = render_inline(described_class.new(diff_file: diff_file)) do |c| + c.with_header { custom_header } + end + + expect(result.css('.custom-header').text).to eq('Custom Header') + end + end + + describe '#default_header' do + let(:diff_file) { create(:diff_file) } + subject { described_class.new(diff_file: diff_file) } + + it 'renders the DiffFileHeaderComponent with the diff file' do + expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new).with(diff_file: diff_file).and_call_original + + subject.default_header + end + end + end diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 42c8ca34ec714a..b5e498d2d0dc50 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -71,4 +71,83 @@ def render_component render_inline(described_class.new(diff_file: diff_file)) end + + describe '#single_file_editor_path' do + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:diff_file) { create(:diff_file, repository: project.repository) } + + context 'when merge_request is provided' do + subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + + it 'returns the path to edit the file in the single file editor' do + expected_path = edit_blob_path(project, merge_request.source_branch, diff_file.new_path) + + "?from_merge_request_iid=#{merge_request.iid}" + + expect(subject.single_file_editor_path).to eq(expected_path) + end + end + + context 'when merge_request is nil' do + subject { described_class.new(diff_file: diff_file) } + + it 'returns nil' do + expect(subject.single_file_editor_path).to be_nil + end + end + + context 'when merge_request has no source_branch' do + subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + + before do + allow(merge_request).to receive(:source_branch).and_return(nil) + end + + it 'returns nil' do + expect(subject.single_file_editor_path).to be_nil + end + end + end + + describe 'options menu items' do + let(:project) { create(:project, :repository) } + let(:diff_file) { create(:diff_file, repository: project.repository) } + let(:content_sha) { 'abc123' } + + before do + allow(diff_file).to receive(:content_sha).and_return(content_sha) + allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') + end + + context 'when merge_request is provided' do + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + + it 'includes both view and edit options' do + render_inline(subject) + + options_menu_items = JSON.parse(page.find('script', visible: false).text) + + expect(options_menu_items.length).to eq(2) + expect(options_menu_items[0]['text']).to include('View file @') + expect(options_menu_items[1]['text']).to eq('Edit in single-file editor') + expect(options_menu_items[1]['href']).to eq(subject.single_file_editor_path) + end + end + + context 'when merge_request is nil' do + subject { described_class.new(diff_file: diff_file) } + + it 'includes only the view option' do + render_inline(subject) + + options_menu_items = JSON.parse(page.find('script', visible: false).text) + + expect(options_menu_items.length).to eq(2) + expect(options_menu_items[0]['text']).to include('View file @') + expect(options_menu_items[1]['text']).to eq('Edit in single-file editor') + expect(options_menu_items[1]['href']).to be_nil + end + end + end end diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 4a0236725f6b7b..5b29cb439987af 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -11,4 +11,29 @@ def render_component(**args) render_inline(described_class.new(diff_file:, merge_request:, **args)) end + + describe RapidDiffs::MergeRequestDiffFileComponent, type: :component do + let(:project) { create(:project, :repository) } + let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let(:diff_file) { create(:diff_file, repository: project.repository) } + + subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + + it 'renders the DiffFileComponent with a custom header' do + expect(RapidDiffs::DiffFileComponent).to receive(:new).with(diff_file: diff_file, parallel_view: false).and_call_original + expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new).with(diff_file: diff_file, merge_request: merge_request).and_call_original + + render_inline(subject) + end + + it 'passes the merge_request to the header component' do + allow(RapidDiffs::DiffFileHeaderComponent).to receive(:new).and_call_original + + render_inline(subject) + + expect(RapidDiffs::DiffFileHeaderComponent).to have_received(:new).with( + hash_including(merge_request: merge_request) + ) + end + end end -- GitLab From 39152d6897830c06bad79da2b3c94e7455bacce2 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 22:59:14 -0600 Subject: [PATCH 07/57] Fix issues with the Rails tests --- .../rapid_diffs/diff_file_header_component.rb | 10 +++++----- .../rapid_diffs/diff_file_header_component_spec.rb | 8 +++----- 2 files changed, 8 insertions(+), 10 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 1b2219238c77d8..b93f787eccd211 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -21,10 +21,10 @@ def copy_path_button end def single_file_editor_path - return unless @merge_request && merge_request.source_branch + return unless @merge_request && @merge_request.source_branch url_for( - helpers.edit_blob_path( + url_helpers.edit_blob_path( @diff_file.repository.project, @merge_request.source_branch, @diff_file.new_path @@ -36,14 +36,14 @@ def menu_items items = [ { "text": _('View file @ %{commitSha}') % { commitSha: Commit.truncate_sha(@diff_file.content_sha) }, - "href": helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) + "href": url_helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) } ] - if single_file_editor_path() do + if single_file_editor_path items.push( { "text": _('Edit in single-file editor'), - "href": single_file_editor_path() + "href": single_file_editor_path } ) end diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index b5e498d2d0dc50..15b204c51e1bf1 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -73,7 +73,7 @@ def render_component end describe '#single_file_editor_path' do - let(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository) } let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } let(:diff_file) { create(:diff_file, repository: project.repository) } @@ -110,7 +110,7 @@ def render_component end describe 'options menu items' do - let(:project) { create(:project, :repository) } + let_it_be(:project) { create(:project, :repository) } let(:diff_file) { create(:diff_file, repository: project.repository) } let(:content_sha) { 'abc123' } @@ -143,10 +143,8 @@ def render_component options_menu_items = JSON.parse(page.find('script', visible: false).text) - expect(options_menu_items.length).to eq(2) + expect(options_menu_items.length).to eq(1) expect(options_menu_items[0]['text']).to include('View file @') - expect(options_menu_items[1]['text']).to eq('Edit in single-file editor') - expect(options_menu_items[1]['href']).to be_nil end end end -- GitLab From 03b58cd14738cdafa029d8b2fc79d76bd4b838c9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 23:06:06 -0600 Subject: [PATCH 08/57] Fix more Rails spec issues --- .../rapid_diffs/diff_file_header_component.rb | 10 +++++----- .../rapid_diffs/diff_file_header_component_spec.rb | 6 +++--- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index b93f787eccd211..5b2e5b91a3ad54 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -21,10 +21,10 @@ def copy_path_button end def single_file_editor_path - return unless @merge_request && @merge_request.source_branch + return unless @merge_request&.source_branch url_for( - url_helpers.edit_blob_path( + helpers.edit_blob_path( @diff_file.repository.project, @merge_request.source_branch, @diff_file.new_path @@ -36,15 +36,15 @@ def menu_items items = [ { "text": _('View file @ %{commitSha}') % { commitSha: Commit.truncate_sha(@diff_file.content_sha) }, - "href": url_helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) + "href": helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) } ] if single_file_editor_path - items.push( { + items.push({ "text": _('Edit in single-file editor'), "href": single_file_editor_path - } ) + }) end items diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 15b204c51e1bf1..40ed9ee3c82d99 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -74,8 +74,8 @@ def render_component describe '#single_file_editor_path' do let_it_be(:project) { create(:project, :repository) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - let(:diff_file) { create(:diff_file, repository: project.repository) } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } + let_it_be(:diff_file) { create(:diff_file, repository: project.repository) } context 'when merge_request is provided' do subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } @@ -111,7 +111,7 @@ def render_component describe 'options menu items' do let_it_be(:project) { create(:project, :repository) } - let(:diff_file) { create(:diff_file, repository: project.repository) } + let_it_be(:diff_file) { create(:diff_file, repository: project.repository) } let(:content_sha) { 'abc123' } before do -- GitLab From 5b7521e4e2cbb1fd4c4483c56e93a17bc9166bc4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 23:17:30 -0600 Subject: [PATCH 09/57] Remove string concatenation by using project_ in front of edit path --- .../rapid_diffs/diff_file_header_component.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 5b2e5b91a3ad54..95fed52b808d68 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -23,12 +23,10 @@ def copy_path_button def single_file_editor_path return unless @merge_request&.source_branch - url_for( - helpers.edit_blob_path( - @diff_file.repository.project, - @merge_request.source_branch, - @diff_file.new_path - ) + "?from_merge_request_iid=#{@merge_request.iid}" + helpers.project_edit_blob_path( + @diff_file.repository.project, + helpers.tree_join(@merge_request.source_branch, @diff_file.new_path), + from_merge_request_iid: @merge_request.iid ) end -- GitLab From cd2ad9b4ebd1eacb4236cf33e5c0f99d2a009f98 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 23:30:09 -0600 Subject: [PATCH 10/57] Fix some rubocop issues --- .../rapid_diffs/diff_file_header_component.rb | 8 ++++---- .../rapid_diffs/diff_file_header_component_spec.rb | 10 +++++----- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 95fed52b808d68..46b06278d9f5f0 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -33,15 +33,15 @@ def single_file_editor_path def menu_items items = [ { - "text": _('View file @ %{commitSha}') % { commitSha: Commit.truncate_sha(@diff_file.content_sha) }, - "href": helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) + text: format(_('View file @ %{commitSha}'), commitSha: Commit.truncate_sha(@diff_file.content_sha)), + href: helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) } ] if single_file_editor_path items.push({ - "text": _('Edit in single-file editor'), - "href": single_file_editor_path + text: _('Edit in single-file editor'), + href: single_file_editor_path }) end diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 40ed9ee3c82d99..69410f5f2dc8de 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -73,9 +73,9 @@ def render_component end describe '#single_file_editor_path' do - let_it_be(:project) { create(:project, :repository) } - let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - let_it_be(:diff_file) { create(:diff_file, repository: project.repository) } + let_it_be(:project) { build(:project, :repository) } + let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } context 'when merge_request is provided' do subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } @@ -110,8 +110,8 @@ def render_component end describe 'options menu items' do - let_it_be(:project) { create(:project, :repository) } - let_it_be(:diff_file) { create(:diff_file, repository: project.repository) } + let_it_be(:project) { build(:project, :repository) } + let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } let(:content_sha) { 'abc123' } before do -- GitLab From aedc7867804a0ab9ffae64ef486570495bb06512 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 9 Apr 2025 23:52:49 -0600 Subject: [PATCH 11/57] Fix even more rubocop errors --- .../rapid_diffs/diff_file_header_component.rb | 5 +++- .../rapid_diffs/diff_file_component_spec.rb | 8 +++---- .../diff_file_header_component_spec.rb | 24 +++++++++---------- .../merge_request_diff_file_component_spec.rb | 6 ++--- 4 files changed, 23 insertions(+), 20 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 46b06278d9f5f0..80a67b190a04bf 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -34,7 +34,10 @@ def menu_items items = [ { text: format(_('View file @ %{commitSha}'), commitSha: Commit.truncate_sha(@diff_file.content_sha)), - href: helpers.project_blob_path(@diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path)) + href: helpers.project_blob_path( + @diff_file.repository.project, + helpers.tree_join(@diff_file.content_sha, @diff_file.new_path) + ) } ] diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index c593618604fcb3..5d31f0d66f9e4f 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -12,7 +12,7 @@ def render_component(**args) end describe 'header slot' do - let(:diff_file) { create(:diff_file) } + let(:diff_file) { build_stubbed(:diff_file) } it 'renders the default header when no custom header is provided' do component = described_class.new(diff_file: diff_file) @@ -34,13 +34,13 @@ def render_component(**args) end describe '#default_header' do - let(:diff_file) { create(:diff_file) } - subject { described_class.new(diff_file: diff_file) } + let(:diff_file) { build_stubbed(:diff_file) } + subject(:component) { described_class.new(diff_file: diff_file) } it 'renders the DiffFileHeaderComponent with the diff file' do expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new).with(diff_file: diff_file).and_call_original - subject.default_header + component.default_header end end diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 69410f5f2dc8de..d0bc8a281d434a 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -73,9 +73,9 @@ def render_component end describe '#single_file_editor_path' do - let_it_be(:project) { build(:project, :repository) } - let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } - let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } + let_it_be(:project) { build_stubbed(:project, :repository) } + let_it_be(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } + let_it_be(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } context 'when merge_request is provided' do subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } @@ -110,8 +110,8 @@ def render_component end describe 'options menu items' do - let_it_be(:project) { build(:project, :repository) } - let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } + let_it_be(:project) { build_stubbed(:project, :repository) } + let_it_be(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } let(:content_sha) { 'abc123' } before do @@ -120,13 +120,13 @@ def render_component end context 'when merge_request is provided' do - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + let(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } + subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } it 'includes both view and edit options' do - render_inline(subject) + render_inline(component) - options_menu_items = JSON.parse(page.find('script', visible: false).text) + options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) expect(options_menu_items.length).to eq(2) expect(options_menu_items[0]['text']).to include('View file @') @@ -136,12 +136,12 @@ def render_component end context 'when merge_request is nil' do - subject { described_class.new(diff_file: diff_file) } + subject(:component) { described_class.new(diff_file: diff_file) } it 'includes only the view option' do - render_inline(subject) + render_inline(component) - options_menu_items = JSON.parse(page.find('script', visible: false).text) + options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) expect(options_menu_items.length).to eq(1) expect(options_menu_items[0]['text']).to include('View file @') diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 5b29cb439987af..da47eab9259ef7 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -13,9 +13,9 @@ def render_component(**args) end describe RapidDiffs::MergeRequestDiffFileComponent, type: :component do - let(:project) { create(:project, :repository) } - let(:merge_request) { create(:merge_request, source_project: project, target_project: project) } - let(:diff_file) { create(:diff_file, repository: project.repository) } + let(:project) { build_stubbed(:project, :repository) } + let(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } + let(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } -- GitLab From 18d4ce425119928939b2da2cfdaf23dd5be87e03 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 00:03:49 -0600 Subject: [PATCH 12/57] Fix even more (more) rubocop issues --- .../rapid_diffs/diff_file_component_spec.rb | 6 ++++-- .../diff_file_header_component_spec.rb | 15 ++++++++------- .../merge_request_diff_file_component_spec.rb | 8 ++++++-- 3 files changed, 18 insertions(+), 11 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index 5d31f0d66f9e4f..bfaddeb98e23fe 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -35,13 +35,15 @@ def render_component(**args) describe '#default_header' do let(:diff_file) { build_stubbed(:diff_file) } + subject(:component) { described_class.new(diff_file: diff_file) } it 'renders the DiffFileHeaderComponent with the diff file' do - expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new).with(diff_file: diff_file).and_call_original + expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new) + .with(diff_file: diff_file) + .and_call_original component.default_header end end - end diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index d0bc8a281d434a..453db238ee5f37 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -78,33 +78,33 @@ def render_component let_it_be(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } context 'when merge_request is provided' do - subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } it 'returns the path to edit the file in the single file editor' do expected_path = edit_blob_path(project, merge_request.source_branch, diff_file.new_path) + - "?from_merge_request_iid=#{merge_request.iid}" + "?from_merge_request_iid=#{merge_request.iid}" expect(subject.single_file_editor_path).to eq(expected_path) end end context 'when merge_request is nil' do - subject { described_class.new(diff_file: diff_file) } + subject(:component) { described_class.new(diff_file: diff_file) } it 'returns nil' do - expect(subject.single_file_editor_path).to be_nil + expect(component.single_file_editor_path).to be_nil end end context 'when merge_request has no source_branch' do - subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } before do allow(merge_request).to receive(:source_branch).and_return(nil) end it 'returns nil' do - expect(subject.single_file_editor_path).to be_nil + expect(component.single_file_editor_path).to be_nil end end end @@ -121,6 +121,7 @@ def render_component context 'when merge_request is provided' do let(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } + subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } it 'includes both view and edit options' do @@ -131,7 +132,7 @@ def render_component expect(options_menu_items.length).to eq(2) expect(options_menu_items[0]['text']).to include('View file @') expect(options_menu_items[1]['text']).to eq('Edit in single-file editor') - expect(options_menu_items[1]['href']).to eq(subject.single_file_editor_path) + expect(options_menu_items[1]['href']).to eq(component.single_file_editor_path) end end diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index da47eab9259ef7..be07ea278974b7 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -20,8 +20,12 @@ def render_component(**args) subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } it 'renders the DiffFileComponent with a custom header' do - expect(RapidDiffs::DiffFileComponent).to receive(:new).with(diff_file: diff_file, parallel_view: false).and_call_original - expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new).with(diff_file: diff_file, merge_request: merge_request).and_call_original + expect(RapidDiffs::DiffFileComponent).to receive(:new) + .with(diff_file: diff_file, parallel_view: false) + .and_call_original + expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new) + .with(diff_file: diff_file, merge_request: merge_request) + .and_call_original render_inline(subject) end -- GitLab From ce78e2e9009a74e714fe38fdb5f22585b259fca4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 00:10:56 -0600 Subject: [PATCH 13/57] Fix more (yes even more) rubocop issues --- .../rapid_diffs/diff_file_header_component_spec.rb | 2 +- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 453db238ee5f37..d6b671d3527589 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -84,7 +84,7 @@ def render_component expected_path = edit_blob_path(project, merge_request.source_branch, diff_file.new_path) + "?from_merge_request_iid=#{merge_request.iid}" - expect(subject.single_file_editor_path).to eq(expected_path) + expect(component.single_file_editor_path).to eq(expected_path) end end diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index be07ea278974b7..d65e8c96bdec68 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -17,7 +17,7 @@ def render_component(**args) let(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } let(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } - subject { described_class.new(diff_file: diff_file, merge_request: merge_request) } + subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } it 'renders the DiffFileComponent with a custom header' do expect(RapidDiffs::DiffFileComponent).to receive(:new) @@ -27,13 +27,13 @@ def render_component(**args) .with(diff_file: diff_file, merge_request: merge_request) .and_call_original - render_inline(subject) + render_inline(component) end it 'passes the merge_request to the header component' do allow(RapidDiffs::DiffFileHeaderComponent).to receive(:new).and_call_original - render_inline(subject) + render_inline(component) expect(RapidDiffs::DiffFileHeaderComponent).to have_received(:new).with( hash_including(merge_request: merge_request) -- GitLab From 2b3dc2666354d6773a8e6eca9e3d81f46dc2360c Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 00:35:18 -0600 Subject: [PATCH 14/57] Switch to safe_format --- app/components/rapid_diffs/diff_file_header_component.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 80a67b190a04bf..c819f5e22479c3 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -33,7 +33,7 @@ def single_file_editor_path def menu_items items = [ { - text: format(_('View file @ %{commitSha}'), commitSha: Commit.truncate_sha(@diff_file.content_sha)), + text: helpers.safe_format(_('View file @ %{commitSha}', commitSha: Commit.truncate_sha(@diff_file.content_sha))), href: helpers.project_blob_path( @diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path) -- GitLab From 8833d83306dbaf1282f961469a396047d265614e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 01:02:17 -0600 Subject: [PATCH 15/57] Don't stub the mocked objects we need --- .../rapid_diffs/diff_file_component_spec.rb | 4 ++-- .../rapid_diffs/diff_file_header_component_spec.rb | 12 ++++++------ .../merge_request_diff_file_component_spec.rb | 6 +++--- 3 files changed, 11 insertions(+), 11 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index bfaddeb98e23fe..896c44c57635d9 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -12,7 +12,7 @@ def render_component(**args) end describe 'header slot' do - let(:diff_file) { build_stubbed(:diff_file) } + let(:diff_file) { build(:diff_file) } it 'renders the default header when no custom header is provided' do component = described_class.new(diff_file: diff_file) @@ -34,7 +34,7 @@ def render_component(**args) end describe '#default_header' do - let(:diff_file) { build_stubbed(:diff_file) } + let(:diff_file) { build(:diff_file) } subject(:component) { described_class.new(diff_file: diff_file) } diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index d6b671d3527589..4804346caf159e 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -73,9 +73,9 @@ def render_component end describe '#single_file_editor_path' do - let_it_be(:project) { build_stubbed(:project, :repository) } - let_it_be(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } - let_it_be(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } + let_it_be(:project) { build(:project, :repository) } + let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } context 'when merge_request is provided' do subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } @@ -110,8 +110,8 @@ def render_component end describe 'options menu items' do - let_it_be(:project) { build_stubbed(:project, :repository) } - let_it_be(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } + let_it_be(:project) { build(:project, :repository) } + let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } let(:content_sha) { 'abc123' } before do @@ -120,7 +120,7 @@ def render_component end context 'when merge_request is provided' do - let(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } + let(:merge_request) { build(:merge_request, source_project: project, target_project: project) } subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index d65e8c96bdec68..a5aa390c0058dd 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -13,9 +13,9 @@ def render_component(**args) end describe RapidDiffs::MergeRequestDiffFileComponent, type: :component do - let(:project) { build_stubbed(:project, :repository) } - let(:merge_request) { build_stubbed(:merge_request, source_project: project, target_project: project) } - let(:diff_file) { build_stubbed(:diff_file, repository: project.repository) } + let(:project) { build(:project, :repository) } + let(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + let(:diff_file) { build(:diff_file, repository: project.repository) } subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } -- GitLab From 961d49bfc86d56bb9932f877e7e670b7402008fc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 01:20:26 -0600 Subject: [PATCH 16/57] Fix line length --- app/components/rapid_diffs/diff_file_header_component.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index c819f5e22479c3..fc277c06c64299 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -33,7 +33,9 @@ def single_file_editor_path def menu_items items = [ { - text: helpers.safe_format(_('View file @ %{commitSha}', commitSha: Commit.truncate_sha(@diff_file.content_sha))), + text: helpers.safe_format( + _('View file @ %{commitSha}', commitSha: Commit.truncate_sha(@diff_file.content_sha)) + ), href: helpers.project_blob_path( @diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path) -- GitLab From c1887e298687b9d5117028d8e43c8d1faddd7f72 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 01:36:26 -0600 Subject: [PATCH 17/57] Fix typo with safe_format --- app/components/rapid_diffs/diff_file_header_component.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index fc277c06c64299..6539930a9526f3 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -34,7 +34,8 @@ def menu_items items = [ { text: helpers.safe_format( - _('View file @ %{commitSha}', commitSha: Commit.truncate_sha(@diff_file.content_sha)) + _('View file @ %{commitSha}'), + commitSha: Commit.truncate_sha(@diff_file.content_sha) ), href: helpers.project_blob_path( @diff_file.repository.project, -- GitLab From 881973ca45cb7ecd9df5dbd998254c8a5f0a2aa4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 01:59:42 -0600 Subject: [PATCH 18/57] Switch a bunch of lets to let_it_bes --- spec/components/rapid_diffs/diff_file_component_spec.rb | 4 ++-- .../rapid_diffs/diff_file_header_component_spec.rb | 2 +- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index 896c44c57635d9..7cbeeca789a9b4 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -12,7 +12,7 @@ def render_component(**args) end describe 'header slot' do - let(:diff_file) { build(:diff_file) } + let_it_be(:diff_file) { build(:diff_file) } it 'renders the default header when no custom header is provided' do component = described_class.new(diff_file: diff_file) @@ -34,7 +34,7 @@ def render_component(**args) end describe '#default_header' do - let(:diff_file) { build(:diff_file) } + let_it_be(:diff_file) { build(:diff_file) } subject(:component) { described_class.new(diff_file: diff_file) } diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 4804346caf159e..20692530cac23f 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -120,7 +120,7 @@ def render_component end context 'when merge_request is provided' do - let(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index a5aa390c0058dd..14690507a0e1ca 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -13,9 +13,9 @@ def render_component(**args) end describe RapidDiffs::MergeRequestDiffFileComponent, type: :component do - let(:project) { build(:project, :repository) } - let(:merge_request) { build(:merge_request, source_project: project, target_project: project) } - let(:diff_file) { build(:diff_file, repository: project.repository) } + let_it_be(:project) { build(:project, :repository) } + let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } -- GitLab From 720ba96d03165455d38966bb4e998ead87cec25e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 10:56:45 -0600 Subject: [PATCH 19/57] Move the knowledge of the merge_request up into the MR-specific file --- .../rapid_diffs/diff_file_header_component.rb | 24 +++++++++---------- ...erge_request_diff_file_component.html.haml | 5 +++- .../merge_request_diff_file_component.rb | 19 +++++++++++++++ 3 files changed, 35 insertions(+), 13 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 6539930a9526f3..5b4b3348039c3d 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -4,9 +4,9 @@ module RapidDiffs class DiffFileHeaderComponent < ViewComponent::Base include ButtonHelper - def initialize(diff_file:, merge_request: nil) + def initialize(diff_file:, additional_menu_items: []) @diff_file = diff_file - @merge_request = merge_request + @additional_menu_items = additional_menu_items end def copy_path_button @@ -31,7 +31,7 @@ def single_file_editor_path end def menu_items - items = [ + base_items = [ { text: helpers.safe_format( _('View file @ %{commitSha}'), @@ -40,18 +40,18 @@ def menu_items href: helpers.project_blob_path( @diff_file.repository.project, helpers.tree_join(@diff_file.content_sha, @diff_file.new_path) - ) + ), + position: 0 } ] - if single_file_editor_path - items.push({ - text: _('Edit in single-file editor'), - href: single_file_editor_path - }) - end - - items + [*base_items, *@additional_menu_items] + .sort_by { + |item| item[:position] || Float::INFINITY + } + .map { + |item| item.except(:position) + } end end end diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml index 1907bde18da2aa..86482046a0273d 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml +++ b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml @@ -1,4 +1,7 @@ = render RapidDiffs::DiffFileComponent.new(diff_file: @diff_file, parallel_view: @parallel_view) do |c| - c.with_header do - = render RapidDiffs::DiffFileHeaderComponent.new(diff_file: @diff_file, merge_request: @merge_request) + = render RapidDiffs::DiffFileHeaderComponent.new( + diff_file: @diff_file, + additional_menu_items: editor_menu_items + ) end diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.rb b/app/components/rapid_diffs/merge_request_diff_file_component.rb index 8c6781d99b2677..2835cc4ea0ff07 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.rb +++ b/app/components/rapid_diffs/merge_request_diff_file_component.rb @@ -9,5 +9,24 @@ def initialize(diff_file:, merge_request:, parallel_view: false) @merge_request = merge_request @parallel_view = parallel_view end + + def editor_menu_items + return [] unless @merge_request&.source_branch + + editor_path = helpers.project_edit_blob_path( + @diff_file.repository.project, + helpers.tree_join(@merge_request.source_branch, @diff_file.new_path), + from_merge_request_iid: @merge_request.iid + ) + + [ + { + text: _('Edit in single-file editor'), + href: editor_path, + position: 1 + } + ] + end + end end -- GitLab From 292ccf3ccc4f1ba80666d2d85baed7cf3777b39f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 11:13:42 -0600 Subject: [PATCH 20/57] Update tests for additional_menu_items --- .../diff_file_header_component_spec.rb | 108 +++++++++--------- .../merge_request_diff_file_component_spec.rb | 63 +++++++--- 2 files changed, 104 insertions(+), 67 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 20692530cac23f..b7219fa296ba5a 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -72,80 +72,84 @@ def render_component render_inline(described_class.new(diff_file: diff_file)) end - describe '#single_file_editor_path' do + describe '#menu_items' do let_it_be(:project) { build(:project, :repository) } - let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } + let(:content_sha) { 'abc123' } - context 'when merge_request is provided' do - subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } - - it 'returns the path to edit the file in the single file editor' do - expected_path = edit_blob_path(project, merge_request.source_branch, diff_file.new_path) + - "?from_merge_request_iid=#{merge_request.iid}" - - expect(component.single_file_editor_path).to eq(expected_path) - end + before do + allow(diff_file).to receive(:content_sha).and_return(content_sha) + allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') end - context 'when merge_request is nil' do + context 'with no additional menu items' do subject(:component) { described_class.new(diff_file: diff_file) } - it 'returns nil' do - expect(component.single_file_editor_path).to be_nil - end - end - - context 'when merge_request has no source_branch' do - subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } + it 'includes only the base view option' do + items = component.menu_items - before do - allow(merge_request).to receive(:source_branch).and_return(nil) - end - - it 'returns nil' do - expect(component.single_file_editor_path).to be_nil + expect(items.length).to eq(1) + expect(items[0]['text']).to include('View file @') + expect(items[0]).not_to have_key(:position) end end - end - describe 'options menu items' do - let_it_be(:project) { build(:project, :repository) } - let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } - let(:content_sha) { 'abc123' } - - before do - allow(diff_file).to receive(:content_sha).and_return(content_sha) - allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') - end + context 'with additional menu items' do + let(:additional_items) do + [ + { + text: 'Edit in single-file editor', + href: '/path/to/edit', + position: 1 + } + ] + end - context 'when merge_request is provided' do - let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + subject(:component) { described_class.new(diff_file: diff_file, additional_menu_items: additional_items) } - subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } + it 'includes both base and additional items' do + items = component.menu_items - it 'includes both view and edit options' do - render_inline(component) + expect(items.length).to eq(2) + expect(items[0]['text']).to include('View file @') + expect(items[1]['text']).to eq('Edit in single-file editor') + expect(items[1]['href']).to eq('/path/to/edit') + end - options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) + it 'removes position attributes from final items' do + items = component.menu_items - expect(options_menu_items.length).to eq(2) - expect(options_menu_items[0]['text']).to include('View file @') - expect(options_menu_items[1]['text']).to eq('Edit in single-file editor') - expect(options_menu_items[1]['href']).to eq(component.single_file_editor_path) + items.each do |item| + expect(item).not_to have_key(:position) + end end end - context 'when merge_request is nil' do - subject(:component) { described_class.new(diff_file: diff_file) } + context 'with items that need to be sorted by position' do + let(:additional_items) do + [ + { + text: 'First item', + href: '/first', + position: -1 + }, + { + text: 'Last item', + href: '/last', + position: 2 + } + ] + end - it 'includes only the view option' do - render_inline(component) + subject(:component) { described_class.new(diff_file: diff_file, additional_menu_items: additional_items) } - options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) + it 'sorts items by position' do + items = component.menu_items - expect(options_menu_items.length).to eq(1) - expect(options_menu_items[0]['text']).to include('View file @') + expect(items.length).to eq(3) + expect(items[0]['text']).to eq('First item') + expect(items[1]['text']).to include('View file @') + expect(items[2]['text']).to eq('Last item') end end end diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 14690507a0e1ca..8eaeaa4faafb8e 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -12,32 +12,65 @@ def render_component(**args) render_inline(described_class.new(diff_file:, merge_request:, **args)) end - describe RapidDiffs::MergeRequestDiffFileComponent, type: :component do + describe '#editor_menu_items' do let_it_be(:project) { build(:project, :repository) } let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } - it 'renders the DiffFileComponent with a custom header' do - expect(RapidDiffs::DiffFileComponent).to receive(:new) - .with(diff_file: diff_file, parallel_view: false) - .and_call_original - expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new) - .with(diff_file: diff_file, merge_request: merge_request) - .and_call_original + context 'when merge_request has a source branch' do + before do + allow(merge_request).to receive(:source_branch).and_return('feature-branch') + allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') + end - render_inline(component) + it 'returns the edit menu item with correct path' do + items = component.editor_menu_items + + expect(items.length).to eq(1) + expect(items[0][:text]).to eq('Edit in single-file editor') + expect(items[0][:position]).to eq(1) + expect(items[0][:href]).to include('path/to/file.rb') + expect(items[0][:href]).to include("from_merge_request_iid=#{merge_request.iid}") + end end - it 'passes the merge_request to the header component' do - allow(RapidDiffs::DiffFileHeaderComponent).to receive(:new).and_call_original + context 'when merge_request has no source branch' do + before do + allow(merge_request).to receive(:source_branch).and_return(nil) + end - render_inline(component) + it 'returns an empty array' do + expect(component.editor_menu_items).to eq([]) + end + end + + context 'when merge_request is nil' do + subject(:component) { described_class.new(diff_file: diff_file, merge_request: nil) } + + it 'returns an empty array' do + expect(component.editor_menu_items).to eq([]) + end + end + end - expect(RapidDiffs::DiffFileHeaderComponent).to have_received(:new).with( - hash_including(merge_request: merge_request) - ) + describe 'rendering' do + let_it_be(:project) { build(:project, :repository) } + let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } + + subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } + + it 'passes additional menu items to the header component' do + editor_items = [{ text: 'Test item', href: '/test', position: 1 }] + allow(component).to receive(:editor_menu_items).and_return(editor_items) + + expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new) + .with(diff_file: diff_file, additional_menu_items: editor_items) + .and_call_original + + render_inline(component) end end end -- GitLab From b70402909056983f55fc0706134db2a7b5b4dfa7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 11:33:55 -0600 Subject: [PATCH 21/57] Remove direct use of render_inline for render_component --- spec/components/rapid_diffs/diff_file_component_spec.rb | 8 ++++---- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index 7cbeeca789a9b4..7626e3b23b183b 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -7,8 +7,8 @@ RSpec.describe RapidDiffs::DiffFileComponent, type: :component, feature_category: :code_review_workflow do include_context "with diff file component tests" - def render_component(**args) - render_inline(described_class.new(diff_file: diff_file, **args)) + def render_component(**args, &block) + render_inline(described_class.new(diff_file: diff_file, **args), &block) end describe 'header slot' do @@ -19,13 +19,13 @@ def render_component(**args) expect(component).to receive(:default_header) - render_inline(component) + render_component end it 'renders a custom header when provided' do custom_header = '
Custom Header
' - result = render_inline(described_class.new(diff_file: diff_file)) do |c| + result = render_component do |c| c.with_header { custom_header } end diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 8eaeaa4faafb8e..398a9f485861a2 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -70,7 +70,7 @@ def render_component(**args) .with(diff_file: diff_file, additional_menu_items: editor_items) .and_call_original - render_inline(component) + render_component end end end -- GitLab From fcd4e5241f7746f66673af6b79789e46432ce1f4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 13:37:12 -0600 Subject: [PATCH 22/57] Fix block style from linter --- app/components/rapid_diffs/diff_file_header_component.rb | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 5b4b3348039c3d..60ab44b032ed31 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -46,12 +46,8 @@ def menu_items ] [*base_items, *@additional_menu_items] - .sort_by { - |item| item[:position] || Float::INFINITY - } - .map { - |item| item.except(:position) - } + .sort_by { |item| item[:position] || Float::INFINITY } + .map { |item| item.except(:position) } end end end -- GitLab From b3f722b15b6fd0f24d5188663009b2da41c53b1d Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 13:37:29 -0600 Subject: [PATCH 23/57] Expect the exact string for the View @ SHA entry --- spec/components/rapid_diffs/diff_file_header_component_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index b7219fa296ba5a..dba8376c6c8078 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -89,7 +89,7 @@ def render_component items = component.menu_items expect(items.length).to eq(1) - expect(items[0]['text']).to include('View file @') + expect(items[0]['text']).to eq("View file @ #{content_sha}") expect(items[0]).not_to have_key(:position) end end -- GitLab From 0c14637793e5779799e90baf328ae91614cc2e50 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 13:41:51 -0600 Subject: [PATCH 24/57] Remove method for MR-specific link --- .../rapid_diffs/diff_file_header_component.rb | 10 ---------- 1 file changed, 10 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 60ab44b032ed31..9b458c6ac6d116 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -20,16 +20,6 @@ def copy_path_button ) end - def single_file_editor_path - return unless @merge_request&.source_branch - - helpers.project_edit_blob_path( - @diff_file.repository.project, - helpers.tree_join(@merge_request.source_branch, @diff_file.new_path), - from_merge_request_iid: @merge_request.iid - ) - end - def menu_items base_items = [ { -- GitLab From feba32a590b2a67a8e3f8f5b464a767572ab7630 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 13:42:46 -0600 Subject: [PATCH 25/57] Allow the position to be returned from the menu_items method --- app/components/rapid_diffs/diff_file_header_component.rb | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.rb b/app/components/rapid_diffs/diff_file_header_component.rb index 9b458c6ac6d116..94b97f06d28d6d 100644 --- a/app/components/rapid_diffs/diff_file_header_component.rb +++ b/app/components/rapid_diffs/diff_file_header_component.rb @@ -35,9 +35,7 @@ def menu_items } ] - [*base_items, *@additional_menu_items] - .sort_by { |item| item[:position] || Float::INFINITY } - .map { |item| item.except(:position) } + [*base_items, *@additional_menu_items].sort_by { |item| item[:position] || Float::INFINITY } end end end -- GitLab From ee9aa3be60389668434c91f118d24dfb5a0d92c9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 13:44:37 -0600 Subject: [PATCH 26/57] Filter out unnecessary item properties in the HAML Some properties (like position) would be useful for other purposes, but aren't necessary when rendering the final version to the DOM. This saves a few bytes and can be inferred on the front end if necessary. --- app/components/rapid_diffs/diff_file_header_component.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/components/rapid_diffs/diff_file_header_component.html.haml b/app/components/rapid_diffs/diff_file_header_component.html.haml index 6174ec73dd3960..4b922320815f78 100644 --- a/app/components/rapid_diffs/diff_file_header_component.html.haml +++ b/app/components/rapid_diffs/diff_file_header_component.html.haml @@ -55,6 +55,6 @@ -# https://gitlab.com/gitlab-org/gitlab/-/merge_requests/182850#note_2387011092 -# haml-lint:disable InlineJavaScript %script{ type: "application/json" } - = menu_items.to_json.html_safe + = menu_items.map { |item| item.except(:position) }.to_json.html_safe - button_params = { icon: 'ellipsis_v', button_options: { data: { click: 'toggleOptionsMenu' }, aria: { label: _('Options') } } } = render Pajamas::ButtonComponent.new(category: :tertiary, size: :small, **button_params) -- GitLab From fe8065177dc5438450c7fdbd10faa7b41827bfe1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 13:46:45 -0600 Subject: [PATCH 27/57] Remove padding line --- app/components/rapid_diffs/merge_request_diff_file_component.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.rb b/app/components/rapid_diffs/merge_request_diff_file_component.rb index 2835cc4ea0ff07..f5328b577fba0e 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.rb +++ b/app/components/rapid_diffs/merge_request_diff_file_component.rb @@ -27,6 +27,5 @@ def editor_menu_items } ] end - end end -- GitLab From 3a98604cdd91af2000b9add2220075ec10719f35 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 14:08:11 -0600 Subject: [PATCH 28/57] Update test for header being rendered MR review note --- .../rapid_diffs/diff_file_component_spec.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index 7626e3b23b183b..b66466eb75f5a9 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -34,16 +34,17 @@ def render_component(**args, &block) end describe '#default_header' do - let_it_be(:diff_file) { build(:diff_file) } - - subject(:component) { described_class.new(diff_file: diff_file) } - it 'renders the DiffFileHeaderComponent with the diff file' do - expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new) - .with(diff_file: diff_file) - .and_call_original + allow_next_instance_of( + RapidDiffs::DiffFileHeaderComponent, + diff_file: diff_file + ) do |instance| + allow(instance).to receive(:render_in).and_return('diff-file-header') + end + + result = render_component - component.default_header + expect(result.to_html).to include('diff-file-header') end end end -- GitLab From 87e0e890bee7bef96cd6f28938861d9da724c713 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 14:08:29 -0600 Subject: [PATCH 29/57] Update tests to allow for position to be in the returned items --- .../rapid_diffs/diff_file_header_component_spec.rb | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index dba8376c6c8078..7035d5f6ae06e6 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -90,7 +90,6 @@ def render_component expect(items.length).to eq(1) expect(items[0]['text']).to eq("View file @ #{content_sha}") - expect(items[0]).not_to have_key(:position) end end @@ -111,18 +110,10 @@ def render_component items = component.menu_items expect(items.length).to eq(2) - expect(items[0]['text']).to include('View file @') + expect(items[0]['text']).to eq("View file @ #{content_sha}") expect(items[1]['text']).to eq('Edit in single-file editor') expect(items[1]['href']).to eq('/path/to/edit') end - - it 'removes position attributes from final items' do - items = component.menu_items - - items.each do |item| - expect(item).not_to have_key(:position) - end - end end context 'with items that need to be sorted by position' do -- GitLab From a5c425b32bb13f22e906dd00d29bfb941467130e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 14:09:49 -0600 Subject: [PATCH 30/57] Test that position is removed when sending the data via DOM --- .../diff_file_header_component_spec.rb | 26 +++++++++++++++++++ 1 file changed, 26 insertions(+) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 7035d5f6ae06e6..a5a1f5004d0f70 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -144,4 +144,30 @@ def render_component end end end + + describe 'template rendering' do + let(:additional_items) do + [ + { + text: 'Test item', + href: '/test', + position: 1 + } + ] + end + + subject(:component) { described_class.new(diff_file: diff_file, additional_menu_items: additional_items) } + + it 'removes position attributes when rendering to JSON' do + render_inline(component) + + json_content = page.find('script', visible: false).text + parsed_items = Gitlab::Json.parse(json_content) + + expect(parsed_items.length).to eq(2) + parsed_items.each do |item| + expect(item).not_to have_key('position') + end + end + end end -- GitLab From 575d32f69ba5cf149f0d216eed19faef86673de8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 14:19:26 -0600 Subject: [PATCH 31/57] Test that the MR-specific diff file is rendering the correct items --- .../merge_request_diff_file_component_spec.rb | 34 +++++++++---------- 1 file changed, 16 insertions(+), 18 deletions(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 398a9f485861a2..f7256056288006 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -6,10 +6,11 @@ RSpec.describe RapidDiffs::MergeRequestDiffFileComponent, type: :component, feature_category: :code_review_workflow do include_context "with diff file component tests" - let_it_be(:merge_request) { build(:merge_request) } + let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + let(:content_sha) { 'abc123' } - def render_component(**args) - render_inline(described_class.new(diff_file:, merge_request:, **args)) + before do + allow(diff_file).to receive(:content_sha).and_return(content_sha) end describe '#editor_menu_items' do @@ -31,8 +32,7 @@ def render_component(**args) expect(items.length).to eq(1) expect(items[0][:text]).to eq('Edit in single-file editor') expect(items[0][:position]).to eq(1) - expect(items[0][:href]).to include('path/to/file.rb') - expect(items[0][:href]).to include("from_merge_request_iid=#{merge_request.iid}") + expect(items[0][:href]).to include("/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=#{merge_request.iid}") end end @@ -56,21 +56,19 @@ def render_component(**args) end describe 'rendering' do - let_it_be(:project) { build(:project, :repository) } - let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } - let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } - - subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } - - it 'passes additional menu items to the header component' do - editor_items = [{ text: 'Test item', href: '/test', position: 1 }] - allow(component).to receive(:editor_menu_items).and_return(editor_items) + it 'renders additional options in the header menu' do + render_component - expect(RapidDiffs::DiffFileHeaderComponent).to receive(:new) - .with(diff_file: diff_file, additional_menu_items: editor_items) - .and_call_original + options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) - render_component + expect(options_menu_items.length).to eq(2) + expect(options_menu_items[0]['text']).to eq('View file @ abc123') + expect(options_menu_items[1]['text']).to eq('Edit in single-file editor') + expect(options_menu_items[1]['href']).to include("/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=#{merge_request.iid}") end end end + +def render_component(**args) + render_inline(described_class.new(diff_file:, merge_request:, **args)) +end \ No newline at end of file -- GitLab From 64f1a2ec93cae2496e255713cfdb71d532456615 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 20:42:12 -0600 Subject: [PATCH 32/57] Fix rubocop errors --- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index f7256056288006..f33f1a19291301 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -8,6 +8,7 @@ include_context "with diff file component tests" let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } let(:content_sha) { 'abc123' } + let(:edit_path_base) { '/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=' } before do allow(diff_file).to receive(:content_sha).and_return(content_sha) @@ -32,7 +33,7 @@ expect(items.length).to eq(1) expect(items[0][:text]).to eq('Edit in single-file editor') expect(items[0][:position]).to eq(1) - expect(items[0][:href]).to include("/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=#{merge_request.iid}") + expect(items[0][:href]).to include("#{edit_path_base}#{merge_request.iid}") end end @@ -64,11 +65,11 @@ expect(options_menu_items.length).to eq(2) expect(options_menu_items[0]['text']).to eq('View file @ abc123') expect(options_menu_items[1]['text']).to eq('Edit in single-file editor') - expect(options_menu_items[1]['href']).to include("/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=#{merge_request.iid}") + expect(options_menu_items[1]['href']).to include("#{edit_path_base}#{merge_request.iid}") end end end def render_component(**args) render_inline(described_class.new(diff_file:, merge_request:, **args)) -end \ No newline at end of file +end -- GitLab From 688f9853c352c1a61f9417509f45b70453a5f751 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 20:47:12 -0600 Subject: [PATCH 33/57] Fix test issues with diff file component --- .../rapid_diffs/diff_file_component_spec.rb | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index b66466eb75f5a9..50a0a7411d0415 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -15,15 +15,20 @@ def render_component(**args, &block) let_it_be(:diff_file) { build(:diff_file) } it 'renders the default header when no custom header is provided' do - component = described_class.new(diff_file: diff_file) + allow_next_instance_of( + RapidDiffs::DiffFileHeaderComponent, + diff_file: diff_file + ) do |instance| + allow(instance).to receive(:render_in).and_return('diff-file-header') + end - expect(component).to receive(:default_header) + result = render_component - render_component + expect(result.to_html).to include('diff-file-header') end it 'renders a custom header when provided' do - custom_header = '
Custom Header
' + custom_header = '
Custom Header
'.html_safe result = render_component do |c| c.with_header { custom_header } -- GitLab From 86d97621d3b36a93c7a5b7fda2f0b993cd3eaa4a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 20:55:37 -0600 Subject: [PATCH 34/57] Fix let issue --- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index f33f1a19291301..afc6f55a364790 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -6,7 +6,8 @@ RSpec.describe RapidDiffs::MergeRequestDiffFileComponent, type: :component, feature_category: :code_review_workflow do include_context "with diff file component tests" - let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } + + let(:merge_request) { build(:merge_request, source_project: project, target_project: project) } let(:content_sha) { 'abc123' } let(:edit_path_base) { '/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=' } -- GitLab From 4da7a22eaa2a435d5e377f6362e7d6565c8748ca Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 23:11:42 -0600 Subject: [PATCH 35/57] Fix scope of render_component --- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index afc6f55a364790..ae05cc963f2134 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -11,6 +11,10 @@ let(:content_sha) { 'abc123' } let(:edit_path_base) { '/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=' } + def render_component(**args) + render_inline(described_class.new(diff_file:, merge_request:, **args)) + end + before do allow(diff_file).to receive(:content_sha).and_return(content_sha) end @@ -70,7 +74,3 @@ end end end - -def render_component(**args) - render_inline(described_class.new(diff_file:, merge_request:, **args)) -end -- GitLab From f60bba57014ff70ed74b23c4e783b21b821e06b2 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 23:34:49 -0600 Subject: [PATCH 36/57] Fix indentation in MR component --- .../rapid_diffs/merge_request_diff_file_component.html.haml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml index 86482046a0273d..dc3de947859961 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml +++ b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml @@ -1,7 +1,7 @@ = render RapidDiffs::DiffFileComponent.new(diff_file: @diff_file, parallel_view: @parallel_view) do |c| - c.with_header do = render RapidDiffs::DiffFileHeaderComponent.new( - diff_file: @diff_file, - additional_menu_items: editor_menu_items - ) + diff_file: @diff_file, + additional_menu_items: editor_menu_items + ) end -- GitLab From d2705da8ad9ef1b556d33eca008644e15e66bf8a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 10 Apr 2025 23:55:24 -0600 Subject: [PATCH 37/57] Prevent diff file syntax error with multi-line sigil --- .../rapid_diffs/merge_request_diff_file_component.html.haml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml index dc3de947859961..472ba575e653a2 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml +++ b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml @@ -1,7 +1,7 @@ = render RapidDiffs::DiffFileComponent.new(diff_file: @diff_file, parallel_view: @parallel_view) do |c| - c.with_header do - = render RapidDiffs::DiffFileHeaderComponent.new( - diff_file: @diff_file, - additional_menu_items: editor_menu_items + = render RapidDiffs::DiffFileHeaderComponent.new( | + diff_file: @diff_file, | + additional_menu_items: editor_menu_items | ) end -- GitLab From 130a224b88cbe0ea79422d6f45fbcad27f547137 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Apr 2025 00:04:45 -0600 Subject: [PATCH 38/57] Bring the header component constructor to a single line --- .../merge_request_diff_file_component.html.haml | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml index 472ba575e653a2..4b7416f7fd40a4 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml +++ b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml @@ -1,7 +1,8 @@ +- # Can't have multi-line constructor, can't extend beyond a column count. This is to shorten. +- d = @diff_file +- i = editor_menu_items + = render RapidDiffs::DiffFileComponent.new(diff_file: @diff_file, parallel_view: @parallel_view) do |c| - c.with_header do - = render RapidDiffs::DiffFileHeaderComponent.new( | - diff_file: @diff_file, | - additional_menu_items: editor_menu_items | - ) + = render RapidDiffs::DiffFileHeaderComponent.new(diff_file: d, additional_menu_items: i) end -- GitLab From b7ce5d77c4e61892006e4854495d41097dfa72ae Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Apr 2025 00:38:15 -0600 Subject: [PATCH 39/57] Fix string matching and increase specificity --- .../rapid_diffs/diff_file_header_component_spec.rb | 2 +- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 8 +++----- 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index a5a1f5004d0f70..93b6d389641065 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -139,7 +139,7 @@ def render_component expect(items.length).to eq(3) expect(items[0]['text']).to eq('First item') - expect(items[1]['text']).to include('View file @') + expect(items[1]['text']).to eq("View file @ #{content_sha}") expect(items[2]['text']).to eq('Last item') end end diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index ae05cc963f2134..dcfa9acf4f3020 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -16,6 +16,9 @@ def render_component(**args) end before do + allow(merge_request).to receive(:source_branch).and_return('feature-branch') + allow(merge_request).to receive(:iid).and_return(123) + allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') allow(diff_file).to receive(:content_sha).and_return(content_sha) end @@ -27,11 +30,6 @@ def render_component(**args) subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } context 'when merge_request has a source branch' do - before do - allow(merge_request).to receive(:source_branch).and_return('feature-branch') - allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') - end - it 'returns the edit menu item with correct path' do items = component.editor_menu_items -- GitLab From a56a58c254a1eb57eba87b8f87e142a979f6cd39 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Apr 2025 00:46:32 -0600 Subject: [PATCH 40/57] Mock the commit that the repository should have --- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index dcfa9acf4f3020..d8fbe909ffe8d4 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -16,6 +16,7 @@ def render_component(**args) end before do + allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(RepoHelpers.sample_commit) allow(merge_request).to receive(:source_branch).and_return('feature-branch') allow(merge_request).to receive(:iid).and_return(123) allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') -- GitLab From 8378d788c5a0b0f3d006081b683a09aa47b11f40 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Apr 2025 00:57:19 -0600 Subject: [PATCH 41/57] Consolidate mocks with receive_messages --- .../merge_request_diff_file_component_spec.rb | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index d8fbe909ffe8d4..d01971ac4b5086 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -17,10 +17,14 @@ def render_component(**args) before do allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(RepoHelpers.sample_commit) - allow(merge_request).to receive(:source_branch).and_return('feature-branch') - allow(merge_request).to receive(:iid).and_return(123) - allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') - allow(diff_file).to receive(:content_sha).and_return(content_sha) + allow(merge_request).to receive_messages( + source_branch: 'feature-branch', + iid: 123 + ) + allow(diff_file).to receive_messages( + new_path: 'path/to/file.rb', + content_sha: content_sha + ) end describe '#editor_menu_items' do -- GitLab From 62be86623009d3f1335cf3ea0acd0f4e1bed24e8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Apr 2025 01:45:07 -0600 Subject: [PATCH 42/57] Make sure the repository is properly mocked --- .../rapid_diffs/diff_file_header_component_spec.rb | 10 ++++++++-- .../merge_request_diff_file_component_spec.rb | 2 ++ 2 files changed, 10 insertions(+), 2 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 93b6d389641065..fe0fe5b94ef37a 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -74,12 +74,18 @@ def render_component describe '#menu_items' do let_it_be(:project) { build(:project, :repository) } + let_it_be(:repository) { project.repository } let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } let(:content_sha) { 'abc123' } before do - allow(diff_file).to receive(:content_sha).and_return(content_sha) - allow(diff_file).to receive(:new_path).and_return('path/to/file.rb') + allow(diff_file).to receive_messages( + content_sha: content_sha, + new_path: 'path/to/file.rb', + repository: repository + ) + allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(RepoHelpers.sample_commit) + allow(RepoHelpers.sample_commit).to receive(:raw_diffs).and_return([diff_file]) end context 'with no additional menu items' do diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index d01971ac4b5086..264cf21d6dc682 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -16,7 +16,9 @@ def render_component(**args) end before do + allow(diff_file).to receive(:repository).and_return(repository) allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(RepoHelpers.sample_commit) + allow(RepoHelpers.sample_commit).to receive(:raw_diffs).and_return([diff_file]) allow(merge_request).to receive_messages( source_branch: 'feature-branch', iid: 123 -- GitLab From 668b71e1fe89a6fc8832772b685fad19dee83ace Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 11 Apr 2025 02:10:51 -0600 Subject: [PATCH 43/57] Mock more repository/commits --- .../rapid_diffs/diff_file_header_component_spec.rb | 4 ++-- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 6 ++++-- spec/components/rapid_diffs/shared.rb | 6 ++++++ 3 files changed, 12 insertions(+), 4 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index fe0fe5b94ef37a..8aa38e92a7f341 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -76,6 +76,7 @@ def render_component let_it_be(:project) { build(:project, :repository) } let_it_be(:repository) { project.repository } let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } + let_it_be(:sample_commit) { instance_double(Commit, raw_diffs: [diff_file]) } let(:content_sha) { 'abc123' } before do @@ -84,8 +85,7 @@ def render_component new_path: 'path/to/file.rb', repository: repository ) - allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(RepoHelpers.sample_commit) - allow(RepoHelpers.sample_commit).to receive(:raw_diffs).and_return([diff_file]) + allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(sample_commit) end context 'with no additional menu items' do diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 264cf21d6dc682..2644013ce8c777 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -7,6 +7,8 @@ RSpec.describe RapidDiffs::MergeRequestDiffFileComponent, type: :component, feature_category: :code_review_workflow do include_context "with diff file component tests" + let_it_be(:repository) { project.repository } + let_it_be(:sample_commit) { instance_double(Commit, raw_diffs: [diff_file]) } let(:merge_request) { build(:merge_request, source_project: project, target_project: project) } let(:content_sha) { 'abc123' } let(:edit_path_base) { '/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=' } @@ -17,8 +19,7 @@ def render_component(**args) before do allow(diff_file).to receive(:repository).and_return(repository) - allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(RepoHelpers.sample_commit) - allow(RepoHelpers.sample_commit).to receive(:raw_diffs).and_return([diff_file]) + allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(sample_commit) allow(merge_request).to receive_messages( source_branch: 'feature-branch', iid: 123 @@ -26,6 +27,7 @@ def render_component(**args) allow(diff_file).to receive_messages( new_path: 'path/to/file.rb', content_sha: content_sha + repository: repository ) end diff --git a/spec/components/rapid_diffs/shared.rb b/spec/components/rapid_diffs/shared.rb index 99dfe3be9c6e41..287c6e199812b8 100644 --- a/spec/components/rapid_diffs/shared.rb +++ b/spec/components/rapid_diffs/shared.rb @@ -9,6 +9,12 @@ let(:repository) { diff_file.repository } let(:project) { repository.container } let(:namespace) { project.namespace } + let(:sample_commit) { instance_double(Commit, raw_diffs: [diff_file]) } + + before do + allow(diff_file).to receive(:repository).and_return(repository) + allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(sample_commit) + end # This should be overridden in the including spec def render_component -- GitLab From 3dc420531e846ce2030e5e3a230abcd087c8d712 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 10:58:01 -0600 Subject: [PATCH 44/57] Use keyword arguments via MR review --- .../merge_request_diff_file_component.html.haml | 8 ++------ .../rapid_diffs/merge_request_diff_file_component.rb | 4 +++- 2 files changed, 5 insertions(+), 7 deletions(-) diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml index 4b7416f7fd40a4..8de679514080ab 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.html.haml +++ b/app/components/rapid_diffs/merge_request_diff_file_component.html.haml @@ -1,8 +1,4 @@ -- # Can't have multi-line constructor, can't extend beyond a column count. This is to shorten. -- d = @diff_file -- i = editor_menu_items - -= render RapidDiffs::DiffFileComponent.new(diff_file: @diff_file, parallel_view: @parallel_view) do |c| += render RapidDiffs::DiffFileComponent.new(diff_file:, parallel_view: @parallel_view) do |c| - c.with_header do - = render RapidDiffs::DiffFileHeaderComponent.new(diff_file: d, additional_menu_items: i) + = render RapidDiffs::DiffFileHeaderComponent.new(diff_file:, additional_menu_items:) end diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.rb b/app/components/rapid_diffs/merge_request_diff_file_component.rb index f5328b577fba0e..33c2168e78aa9a 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.rb +++ b/app/components/rapid_diffs/merge_request_diff_file_component.rb @@ -4,13 +4,15 @@ module RapidDiffs class MergeRequestDiffFileComponent < ViewComponent::Base with_collection_parameter :diff_file + attr_reader :diff_file + def initialize(diff_file:, merge_request:, parallel_view: false) @diff_file = diff_file @merge_request = merge_request @parallel_view = parallel_view end - def editor_menu_items + def additional_menu_items return [] unless @merge_request&.source_branch editor_path = helpers.project_edit_blob_path( -- GitLab From dd8826d374001e544f6c6c90deec86bc8242a639 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 11:04:12 -0600 Subject: [PATCH 45/57] Remove safe drilling check This is a defensive coding technique that we're removing. In theory it produces no extra cycles, while protecting against "unthinkable" edge cases. t:pd#3,ty#be --- app/components/rapid_diffs/merge_request_diff_file_component.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/components/rapid_diffs/merge_request_diff_file_component.rb b/app/components/rapid_diffs/merge_request_diff_file_component.rb index 33c2168e78aa9a..ea997634a7497a 100644 --- a/app/components/rapid_diffs/merge_request_diff_file_component.rb +++ b/app/components/rapid_diffs/merge_request_diff_file_component.rb @@ -13,8 +13,6 @@ def initialize(diff_file:, merge_request:, parallel_view: false) end def additional_menu_items - return [] unless @merge_request&.source_branch - editor_path = helpers.project_edit_blob_path( @diff_file.repository.project, helpers.tree_join(@merge_request.source_branch, @diff_file.new_path), -- GitLab From fd3ac402a07098ccfdc2f797a30eb474fe49573f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 11:07:38 -0600 Subject: [PATCH 46/57] Move render_component to after the specs --- spec/components/rapid_diffs/diff_file_component_spec.rb | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index 50a0a7411d0415..430f4f2369153d 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -7,10 +7,6 @@ RSpec.describe RapidDiffs::DiffFileComponent, type: :component, feature_category: :code_review_workflow do include_context "with diff file component tests" - def render_component(**args, &block) - render_inline(described_class.new(diff_file: diff_file, **args), &block) - end - describe 'header slot' do let_it_be(:diff_file) { build(:diff_file) } @@ -52,4 +48,8 @@ def render_component(**args, &block) expect(result.to_html).to include('diff-file-header') end end + + def render_component(**args, &block) + render_inline(described_class.new(diff_file: diff_file, **args), &block) + end end -- GitLab From 48c69bc8ab1a38a6ef7a79387f390e051a1e2fb8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 11:11:09 -0600 Subject: [PATCH 47/57] Remove test of `default_header` method While redundant, this leave the specifics of this exact method untested, relying exclusively on other checks that depend on this method internally. There may be a time when this method and the abstracted tests deviate, and that could expose some untested edge cases. t:pd#2,ty#be --- .../rapid_diffs/diff_file_component_spec.rb | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_component_spec.rb b/spec/components/rapid_diffs/diff_file_component_spec.rb index 430f4f2369153d..e693c821bfe346 100644 --- a/spec/components/rapid_diffs/diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_component_spec.rb @@ -34,21 +34,6 @@ end end - describe '#default_header' do - it 'renders the DiffFileHeaderComponent with the diff file' do - allow_next_instance_of( - RapidDiffs::DiffFileHeaderComponent, - diff_file: diff_file - ) do |instance| - allow(instance).to receive(:render_in).and_return('diff-file-header') - end - - result = render_component - - expect(result.to_html).to include('diff-file-header') - end - end - def render_component(**args, &block) render_inline(described_class.new(diff_file: diff_file, **args), &block) end -- GitLab From 3137caebcd97983de54945955822b2a922d172fa Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 11:46:03 -0600 Subject: [PATCH 48/57] Remove redundant mocks --- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 2644013ce8c777..fc132a5309988c 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -7,8 +7,6 @@ RSpec.describe RapidDiffs::MergeRequestDiffFileComponent, type: :component, feature_category: :code_review_workflow do include_context "with diff file component tests" - let_it_be(:repository) { project.repository } - let_it_be(:sample_commit) { instance_double(Commit, raw_diffs: [diff_file]) } let(:merge_request) { build(:merge_request, source_project: project, target_project: project) } let(:content_sha) { 'abc123' } let(:edit_path_base) { '/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=' } -- GitLab From 4297cbc692ba26e1d3d23abfe1c8a231be68a3ab Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 16:10:03 -0600 Subject: [PATCH 49/57] Remove tests for "non-happy" path behavior via MR review The escalated risk here is that while we "expect" everything to be populated properly, not testing that the code works in "unexpected" scenarios leaves us open to uncaught bugs, the entire purpose of tests. There was a recent issue where a thing that could never be empty - oops - turned out to be. The entire professional intent of testing code is to make sure that - in the event of something that "isn't possible," the system as a whole operates reasonably. Removing these tests reduces our resiliency to unexpected issues. t:pd#-1,ty#be --- .../merge_request_diff_file_component_spec.rb | 18 ------------------ 1 file changed, 18 deletions(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index fc132a5309988c..86c6a210e3109a 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -46,24 +46,6 @@ def render_component(**args) expect(items[0][:href]).to include("#{edit_path_base}#{merge_request.iid}") end end - - context 'when merge_request has no source branch' do - before do - allow(merge_request).to receive(:source_branch).and_return(nil) - end - - it 'returns an empty array' do - expect(component.editor_menu_items).to eq([]) - end - end - - context 'when merge_request is nil' do - subject(:component) { described_class.new(diff_file: diff_file, merge_request: nil) } - - it 'returns an empty array' do - expect(component.editor_menu_items).to eq([]) - end - end end describe 'rendering' do -- GitLab From 06b60b6fa733697649d60b97afa36b7369869e32 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 16:20:25 -0600 Subject: [PATCH 50/57] Move render_component to bottom --- .../rapid_diffs/diff_file_header_component_spec.rb | 7 ++++--- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 8 ++++---- 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 8aa38e92a7f341..dc99761b2f0b62 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -68,9 +68,6 @@ expect(page.find('[data-testid="js-file-deletion-line"]')).to have_text(diff_file.removed_lines) end - def render_component - render_inline(described_class.new(diff_file: diff_file)) - end describe '#menu_items' do let_it_be(:project) { build(:project, :repository) } @@ -176,4 +173,8 @@ def render_component end end end + + def render_component(**args) + render_inline(described_class.new(diff_file:, **args)) + end end diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 86c6a210e3109a..4d6a965a930c82 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -11,10 +11,6 @@ let(:content_sha) { 'abc123' } let(:edit_path_base) { '/-/edit/feature-branch/path/to/file.rb?from_merge_request_iid=' } - def render_component(**args) - render_inline(described_class.new(diff_file:, merge_request:, **args)) - end - before do allow(diff_file).to receive(:repository).and_return(repository) allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(sample_commit) @@ -60,4 +56,8 @@ def render_component(**args) expect(options_menu_items[1]['href']).to include("#{edit_path_base}#{merge_request.iid}") end end + + def render_component(**args) + render_inline(described_class.new(diff_file:, merge_request:, **args)) + end end -- GitLab From 0ffe32ccc21e707be2b8d27008eb2b413d6e57ea Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 16:20:45 -0600 Subject: [PATCH 51/57] Test only the rendered output --- .../diff_file_header_component_spec.rb | 114 ++++-------------- 1 file changed, 24 insertions(+), 90 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index dc99761b2f0b62..6d6f89faa3d38b 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -68,107 +68,41 @@ expect(page.find('[data-testid="js-file-deletion-line"]')).to have_text(diff_file.removed_lines) end + describe 'menu items' do + it 'renders default menu items' do + render_component - describe '#menu_items' do - let_it_be(:project) { build(:project, :repository) } - let_it_be(:repository) { project.repository } - let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } - let_it_be(:sample_commit) { instance_double(Commit, raw_diffs: [diff_file]) } - let(:content_sha) { 'abc123' } - - before do - allow(diff_file).to receive_messages( - content_sha: content_sha, - new_path: 'path/to/file.rb', - repository: repository - ) - allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(sample_commit) - end - - context 'with no additional menu items' do - subject(:component) { described_class.new(diff_file: diff_file) } - - it 'includes only the base view option' do - items = component.menu_items - - expect(items.length).to eq(1) - expect(items[0]['text']).to eq("View file @ #{content_sha}") - end - end + options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) - context 'with additional menu items' do - let(:additional_items) do - [ - { - text: 'Edit in single-file editor', - href: '/path/to/edit', - position: 1 - } - ] - end - - subject(:component) { described_class.new(diff_file: diff_file, additional_menu_items: additional_items) } - - it 'includes both base and additional items' do - items = component.menu_items - - expect(items.length).to eq(2) - expect(items[0]['text']).to eq("View file @ #{content_sha}") - expect(items[1]['text']).to eq('Edit in single-file editor') - expect(items[1]['href']).to eq('/path/to/edit') - end + expect(options_menu_items.length).to eq(1) + expect(options_menu_items[0]['text']).to eq("View file @ #{content_sha}") + expect(options_menu_items[0]).not_to have_key('position') end - context 'with items that need to be sorted by position' do - let(:additional_items) do - [ - { - text: 'First item', - href: '/first', - position: -1 - }, - { - text: 'Last item', - href: '/last', - position: 2 - } - ] - end - - subject(:component) { described_class.new(diff_file: diff_file, additional_menu_items: additional_items) } - - it 'sorts items by position' do - items = component.menu_items - - expect(items.length).to eq(3) - expect(items[0]['text']).to eq('First item') - expect(items[1]['text']).to eq("View file @ #{content_sha}") - expect(items[2]['text']).to eq('Last item') - end - end - end - - describe 'template rendering' do - let(:additional_items) do - [ + it 'renders additional menu items with respective order' do + additional_menu_items = [ { - text: 'Test item', - href: '/test', - position: 1 + text: 'First item', + href: '/first', + position: -1 + }, + { + text: 'Last item', + href: '/last', + position: 2 } ] - end - subject(:component) { described_class.new(diff_file: diff_file, additional_menu_items: additional_items) } + render_inline(additional_menu_items:) - it 'removes position attributes when rendering to JSON' do - render_inline(component) + options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) - json_content = page.find('script', visible: false).text - parsed_items = Gitlab::Json.parse(json_content) + expect(options_menu_items.length).to eq(3) + expect(options_menu_items[0]['text']).to eq('First item') + expect(options_menu_items[1]['text']).to eq("View file @ #{content_sha}") + expect(options_menu_items[2]['text']).to eq('Last item') - expect(parsed_items.length).to eq(2) - parsed_items.each do |item| + options_menu_items.each do |item| expect(item).not_to have_key('position') end end -- GitLab From c7770bfaae2f03b3e1a6045f4696e56abd6e1e2a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 16:25:27 -0600 Subject: [PATCH 52/57] Fix a missing comma How did that happen.... --- .../rapid_diffs/merge_request_diff_file_component_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index 4d6a965a930c82..c0e9ec0fb3738e 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -20,7 +20,7 @@ ) allow(diff_file).to receive_messages( new_path: 'path/to/file.rb', - content_sha: content_sha + content_sha: content_sha, repository: repository ) end -- GitLab From 758362aa479bf8c7e5d14109d9fd7f8a93395bf0 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 16:51:16 -0600 Subject: [PATCH 53/57] Ensure content_sha is available and mocked --- .../rapid_diffs/diff_file_header_component_spec.rb | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 6d6f89faa3d38b..2b0150194a53db 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -69,6 +69,12 @@ end describe 'menu items' do + let(:content_sha) { 'abc123' } + + before do + allow(diff_file).to receive(:content_sha).and_return(content_sha) + end + it 'renders default menu items' do render_component -- GitLab From a57b877967fc8267e0f03b0c5f4b41c65e9f4d33 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 16:52:03 -0600 Subject: [PATCH 54/57] Pass correct additional menu items in test --- .../components/rapid_diffs/diff_file_header_component_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index 2b0150194a53db..ea6c57931d2e0c 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -86,7 +86,7 @@ end it 'renders additional menu items with respective order' do - additional_menu_items = [ + menu_items = [ { text: 'First item', href: '/first', @@ -99,7 +99,7 @@ } ] - render_inline(additional_menu_items:) + render_inline(additional_menu_items: menu_items) options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) -- GitLab From 00d2b3ef9fbfb6e69dc46c857c950b48ae267e3a Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 14 Apr 2025 16:54:01 -0600 Subject: [PATCH 55/57] Remove test for method and rely exclusively on the rendering test --- .../merge_request_diff_file_component_spec.rb | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb index c0e9ec0fb3738e..a53219d3a2c0ec 100644 --- a/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb +++ b/spec/components/rapid_diffs/merge_request_diff_file_component_spec.rb @@ -25,25 +25,6 @@ ) end - describe '#editor_menu_items' do - let_it_be(:project) { build(:project, :repository) } - let_it_be(:merge_request) { build(:merge_request, source_project: project, target_project: project) } - let_it_be(:diff_file) { build(:diff_file, repository: project.repository) } - - subject(:component) { described_class.new(diff_file: diff_file, merge_request: merge_request) } - - context 'when merge_request has a source branch' do - it 'returns the edit menu item with correct path' do - items = component.editor_menu_items - - expect(items.length).to eq(1) - expect(items[0][:text]).to eq('Edit in single-file editor') - expect(items[0][:position]).to eq(1) - expect(items[0][:href]).to include("#{edit_path_base}#{merge_request.iid}") - end - end - end - describe 'rendering' do it 'renders additional options in the header menu' do render_component -- GitLab From ec20b2f0f4fe416b271670bb8c53028a94ffd8bf Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Tue, 15 Apr 2025 10:35:39 -0600 Subject: [PATCH 56/57] Use correct rendering abstraction --- spec/components/rapid_diffs/diff_file_header_component_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/components/rapid_diffs/diff_file_header_component_spec.rb b/spec/components/rapid_diffs/diff_file_header_component_spec.rb index ea6c57931d2e0c..146cb71c0c50b2 100644 --- a/spec/components/rapid_diffs/diff_file_header_component_spec.rb +++ b/spec/components/rapid_diffs/diff_file_header_component_spec.rb @@ -99,7 +99,7 @@ } ] - render_inline(additional_menu_items: menu_items) + render_component(additional_menu_items: menu_items) options_menu_items = Gitlab::Json.parse(page.find('script', visible: false).text) -- GitLab From f1201cc3628eda7f30c5ee7dbb39df848f85e257 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Thu, 17 Apr 2025 15:20:27 -0600 Subject: [PATCH 57/57] Remove redundant repository mock --- spec/components/rapid_diffs/shared.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/components/rapid_diffs/shared.rb b/spec/components/rapid_diffs/shared.rb index 287c6e199812b8..6eab9a352a261e 100644 --- a/spec/components/rapid_diffs/shared.rb +++ b/spec/components/rapid_diffs/shared.rb @@ -12,7 +12,6 @@ let(:sample_commit) { instance_double(Commit, raw_diffs: [diff_file]) } before do - allow(diff_file).to receive(:repository).and_return(repository) allow(repository).to receive(:commit).with(RepoHelpers.sample_commit.id).and_return(sample_commit) end -- GitLab