From b7a8e0ee2bee426b4d59bc1a6c7139d2f4115a93 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Fri, 28 Feb 2025 13:04:49 +0000 Subject: [PATCH 01/23] Added rapid diffs route to merge request creation form --- .../merge_requests/creations_controller.rb | 11 ++++++++++- .../merge_requests/creations/_page.html.haml | 17 +++++++++++++++++ .../merge_requests/creations/new.html.haml | 18 +----------------- .../creations/rapid_diffs.html.haml | 7 +++++++ config/routes/merge_requests.rb | 2 ++ 5 files changed, 37 insertions(+), 18 deletions(-) create mode 100644 app/views/projects/merge_requests/creations/_page.html.haml create mode 100644 app/views/projects/merge_requests/creations/rapid_diffs.html.haml diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index e8e409f8917eee..8ee9a7e6f03bca 100644 --- a/app/controllers/projects/merge_requests/creations_controller.rb +++ b/app/controllers/projects/merge_requests/creations_controller.rb @@ -9,7 +9,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap skip_before_action :merge_request before_action :authorize_create_merge_request_from! - before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path] + before_action :apply_diff_view_cookie!, only: [:diffs, :diff_for_path, :rapid_diffs] before_action :build_merge_request, except: [:create] urgency :low, [ @@ -17,6 +17,7 @@ class Projects::MergeRequests::CreationsController < Projects::MergeRequests::Ap :create, :pipelines, :diffs, + :rapid_diffs, :branch_from, :branch_to ] @@ -61,6 +62,14 @@ def pipelines } end + def rapid_diffs + return render_404 unless ::Feature.enabled?(:rapid_diffs, current_user, type: :wip) + + @show_whitespace_default = current_user.nil? || current_user.show_whitespace_in_diffs + + define_new_vars + end + def diffs @diffs = @merge_request.diffs(diff_options) if @merge_request.can_be_created diff --git a/app/views/projects/merge_requests/creations/_page.html.haml b/app/views/projects/merge_requests/creations/_page.html.haml new file mode 100644 index 00000000000000..facb6727946e08 --- /dev/null +++ b/app/views/projects/merge_requests/creations/_page.html.haml @@ -0,0 +1,17 @@ +- add_to_breadcrumbs _("Merge requests"), project_merge_requests_path(@project) +- breadcrumb_title _("New merge request") +- page_title _("New merge request") +- add_page_specific_style 'page_bundles/pipelines' +- add_page_specific_style 'page_bundles/ci_status' +- add_page_specific_style 'page_bundles/merge_request' + +- conflicting_mr = @merge_request.existing_mrs_targeting_same_branch.first + +- if @merge_request.can_be_created && !params[:change_branches] && !conflicting_mr + = render 'new_submit' +- else + - if conflicting_mr + - link_to_mr = link_to(conflicting_mr.to_reference, project_merge_request_path(conflicting_mr.target_project, conflicting_mr)) + - flash.now[:alert] = safe_format(s_("These branches already have an open merge request: %{link_to_mr}. Select a different source or target branch."), link_to_mr: link_to_mr) + + = render 'new_compare' diff --git a/app/views/projects/merge_requests/creations/new.html.haml b/app/views/projects/merge_requests/creations/new.html.haml index facb6727946e08..b810c8659bdbab 100644 --- a/app/views/projects/merge_requests/creations/new.html.haml +++ b/app/views/projects/merge_requests/creations/new.html.haml @@ -1,17 +1 @@ -- add_to_breadcrumbs _("Merge requests"), project_merge_requests_path(@project) -- breadcrumb_title _("New merge request") -- page_title _("New merge request") -- add_page_specific_style 'page_bundles/pipelines' -- add_page_specific_style 'page_bundles/ci_status' -- add_page_specific_style 'page_bundles/merge_request' - -- conflicting_mr = @merge_request.existing_mrs_targeting_same_branch.first - -- if @merge_request.can_be_created && !params[:change_branches] && !conflicting_mr - = render 'new_submit' -- else - - if conflicting_mr - - link_to_mr = link_to(conflicting_mr.to_reference, project_merge_request_path(conflicting_mr.target_project, conflicting_mr)) - - flash.now[:alert] = safe_format(s_("These branches already have an open merge request: %{link_to_mr}. Select a different source or target branch."), link_to_mr: link_to_mr) - - = render 'new_compare' += render "page" diff --git a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml new file mode 100644 index 00000000000000..8e114f6785c9c9 --- /dev/null +++ b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml @@ -0,0 +1,7 @@ +- add_page_specific_style 'page_bundles/merge_request_rapid_diffs' +- @content_class = 'rd-page-container' + += render "page" + +- args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil } += render ::RapidDiffs::AppComponent.new(**args) diff --git a/config/routes/merge_requests.rb b/config/routes/merge_requests.rb index ee4e2e5d529e27..976d10f9237b23 100644 --- a/config/routes/merge_requests.rb +++ b/config/routes/merge_requests.rb @@ -88,6 +88,8 @@ end scope action: :new do + get :diffs, to: 'merge_requests/creations#rapid_diffs', defaults: { tab: 'diffs' }, + constraints: ->(params) { params[:rapid_diffs] == 'true' } get :diffs, defaults: { tab: 'diffs' } get :pipelines, defaults: { tab: 'pipelines' } end -- GitLab From 9fb65d3012c1b5567af2ff93a8be945a292f761f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Mar 2025 17:31:31 -0600 Subject: [PATCH 02/23] Enable rendering diffs on the MR Create page with Rapid Diffs --- app/assets/javascripts/merge_request_tabs.js | 19 ++++++++++--- .../creations/rapid_diffs/index.js | 18 +++++++++++++ .../javascripts/rapid_diffs/app/index.js | 27 +++++++++++-------- .../merge_request_rapid_diffs.scss | 5 ++++ .../rapid_diffs/app_component.html.haml | 9 ++++--- app/components/rapid_diffs/app_component.rb | 2 ++ .../projects/commit/rapid_diffs.html.haml | 2 +- .../creations/rapid_diffs.html.haml | 2 +- .../merge_requests/rapid_diffs.html.haml | 2 +- 9 files changed, 65 insertions(+), 21 deletions(-) create mode 100644 app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 705beb92dad35a..0a5079d5b6dd77 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -5,9 +5,10 @@ import VueApollo from 'vue-apollo'; import createDefaultClient from '~/lib/graphql'; import { createAlert } from '~/alert'; import { getCookie, isMetaClick, parseBoolean, scrollToElement } from '~/lib/utils/common_utils'; -import { parseUrlPathname, visitUrl } from '~/lib/utils/url_utility'; +import { parseUrlPathname, visitUrl, getParameterByName } from '~/lib/utils/url_utility'; import createEventHub from '~/helpers/event_hub_factory'; import { renderGFM } from '~/behaviors/markdown/render_gfm'; +import { createRapidDiffsApp } from '~/rapid_diffs/app'; import BlobForkSuggestion from './blob/blob_fork_suggestion'; import Diff from './diff'; import { initDiffStatsDropdown } from './init_diff_stats_dropdown'; @@ -221,6 +222,7 @@ export default class MergeRequestTabs { this.diffsClass = null; this.commitsLoaded = false; this.isFixedLayoutPreferred = this.contentWrapper.classList.contains('container-limited'); + this.isRapidDiffs = getParameterByName('rapid_diffs') === 'true'; this.eventHub = createEventHub(); this.loadedPages = { [action]: true }; @@ -357,7 +359,7 @@ export default class MergeRequestTabs { in practice, this only occurs when comparing commits in the new merge request form page. */ - this.loadDiff({ endpoint: href, strip: true }); + this.startDiffs({ endpoint: href, strip: true }); } // this.hideSidebar(); this.expandViewContainer(); @@ -528,7 +530,18 @@ export default class MergeRequestTabs { this.mergeRequestPipelinesTable = mountPipelines(); } - // load the diff tab content from the backend + // Initialize the Changes tab + startDiffs(options = {}) { + if (this.isRapidDiffs) { + const rd = createRapidDiffsApp(); + + rd.streamRemainingDiffs(); + rd.init(); + } else { + this.loadDiff(options); + } + } + // load the legacy diff tab content from the backend loadDiff({ endpoint, strip = true }) { if (this.diffsLoaded) { document.dispatchEvent(new CustomEvent('scroll')); diff --git a/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js b/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js new file mode 100644 index 00000000000000..62eb4e92ef3c9f --- /dev/null +++ b/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js @@ -0,0 +1,18 @@ +import { initMarkdownEditor } from 'ee_else_ce/pages/projects/merge_requests/init_markdown_editor'; +import initPipelines from '~/commit/pipelines/pipelines_bundle'; +import MergeRequest from '~/merge_request'; + +const mrNewSubmitNode = document.querySelector('.js-merge-request-new-submit'); +const rapidDiffsApp = document.querySelector('.rd-app[data-rapid-diffs]'); +const diffsPane = mrNewSubmitNode?.querySelector('#diffs'); + +if (diffsPane && rapidDiffsApp) { + diffsPane.appendChild(rapidDiffsApp); +} + +// eslint-disable-next-line no-new +new MergeRequest({ + action: mrNewSubmitNode.dataset.mrSubmitAction, +}); +initPipelines(); +initMarkdownEditor(); diff --git a/app/assets/javascripts/rapid_diffs/app/index.js b/app/assets/javascripts/rapid_diffs/app/index.js index 5a896ef7bc6505..5bc0122104a0bd 100644 --- a/app/assets/javascripts/rapid_diffs/app/index.js +++ b/app/assets/javascripts/rapid_diffs/app/index.js @@ -13,8 +13,9 @@ import { __ } from '~/locale'; // This facade interface joins together all the bits and pieces of Rapid Diffs: DiffFile, Settings, File browser, etc. // It's a unified entrypoint for Rapid Diffs and all external communications should happen through this interface. class RapidDiffsFacade { - constructor({ DiffFileImplementation = DiffFile } = {}) { + constructor({ DiffFileImplementation = DiffFile, fileBrowser = true } = {}) { this.DiffFileImplementation = DiffFileImplementation; + this.flags = { fileBrowser }; } init() { @@ -22,17 +23,21 @@ class RapidDiffsFacade { const { reloadStreamUrl, metadataEndpoint } = document.querySelector('[data-rapid-diffs]').dataset; useDiffsView(pinia).metadataEndpoint = metadataEndpoint; - useDiffsView(pinia) - .loadMetadata() - .then(() => { - initHiddenFilesWarning(); - initFileBrowser(); - }) - .catch(() => { - createAlert({ - message: __('Failed to load additional diffs information. Try reloading the page.'), + + if (this.flags.fileBrowser) { + useDiffsView(pinia) + .loadMetadata() + .then(() => { + initHiddenFilesWarning(); + initFileBrowser(); + }) + .catch(() => { + createAlert({ + message: __('Failed to load additional diffs information. Try reloading the page.'), + }); }); - }); + } + initViewSettings({ pinia, streamUrl: reloadStreamUrl }); } diff --git a/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss b/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss index 65707eeb4a9b9c..db99cdf56fd602 100644 --- a/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss +++ b/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss @@ -4,3 +4,8 @@ .rd-app { --rd-diff-file-sticky-top-padding: calc(#{$calc-application-header-height} + #{$mr-sticky-header-height}); } + +.js-merge-request-new-submit #diffs{ + position: relative; + z-index: 251; // If the settings menu displays upward on the create new/submit page, it needs to be in front of the tab bar +} \ No newline at end of file diff --git a/app/components/rapid_diffs/app_component.html.haml b/app/components/rapid_diffs/app_component.html.haml index 5b687a7e7acd08..a29391454bbd95 100644 --- a/app/components/rapid_diffs/app_component.html.haml +++ b/app/components/rapid_diffs/app_component.html.haml @@ -15,10 +15,11 @@ .rd-app-settings %div{ data: { view_settings: true, show_whitespace: @show_whitespace.to_json, diff_view_type: @diff_view, update_user_endpoint: @update_user_endpoint } } .rd-app-body - .rd-app-sidebar{ data: { file_browser: true }, style: ("width: #{initial_sidebar_width}px" if initial_sidebar_width) } - .rd-app-sidebar-loading - = helpers.gl_loading_icon(size: 'sm') - .rd-app-content{ data: { sidebar_visible: true } } + - if @include_sidebar + .rd-app-sidebar{ data: { file_browser: true }, style: ("width: #{initial_sidebar_width}px" if initial_sidebar_width) } + .rd-app-sidebar-loading + = helpers.gl_loading_icon(size: 'sm') + .rd-app-content{ data: { sidebar_visible: @include_sidebar } } .rd-app-content-header{ data: { hidden_files_warning: true } } .code{ class: helpers.user_color_scheme } %div{ data: { diffs_list: true } } diff --git a/app/components/rapid_diffs/app_component.rb b/app/components/rapid_diffs/app_component.rb index 70ae3056aee632..e22f3e3e596066 100644 --- a/app/components/rapid_diffs/app_component.rb +++ b/app/components/rapid_diffs/app_component.rb @@ -13,6 +13,7 @@ def initialize( update_user_endpoint:, metadata_endpoint:, preload: true + include_sidebar: ) @diffs_slice = diffs_slice @reload_stream_url = reload_stream_url @@ -22,6 +23,7 @@ def initialize( @update_user_endpoint = update_user_endpoint @metadata_endpoint = metadata_endpoint @preload = preload + @include_sidebar = include_sidebar end def initial_sidebar_width diff --git a/app/views/projects/commit/rapid_diffs.html.haml b/app/views/projects/commit/rapid_diffs.html.haml index 4a9df13a4ad301..3959253f2ec0c2 100644 --- a/app/views/projects/commit/rapid_diffs.html.haml +++ b/app/views/projects/commit/rapid_diffs.html.haml @@ -11,5 +11,5 @@ .container-fluid{ class: [container_class] } = render "commit_box" = render "ci_menu" - - args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url } + - args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url, include_sidebar: true } = render ::RapidDiffs::AppComponent.new(**args) diff --git a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml index 8e114f6785c9c9..37f07f0a2ad28e 100644 --- a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml +++ b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml @@ -3,5 +3,5 @@ = render "page" -- args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil } +- args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil, include_sidebar: false } = render ::RapidDiffs::AppComponent.new(**args) diff --git a/app/views/projects/merge_requests/rapid_diffs.html.haml b/app/views/projects/merge_requests/rapid_diffs.html.haml index 8037561944d0d3..787af4987a28bd 100644 --- a/app/views/projects/merge_requests/rapid_diffs.html.haml +++ b/app/views/projects/merge_requests/rapid_diffs.html.haml @@ -3,7 +3,7 @@ - @content_class = 'rd-page-container diffs-container-limited' = render 'page' -- args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url } +- args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url, include_sidebar: true } = render ::RapidDiffs::AppComponent.new(**args) do |c| - c.with_diffs_list do = render RapidDiffs::MergeRequestDiffFileComponent.with_collection(@diffs_slice, merge_request: @merge_request, parallel_view: @diff_view == :parallel) -- GitLab From 618e7d65875db731d91c7a5d08e29708036354b4 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Mar 2025 17:46:31 -0600 Subject: [PATCH 03/23] Fix the interaction of z-index and the sticky tabs --- .../page_bundles/merge_request_rapid_diffs.scss | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss b/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss index db99cdf56fd602..f961b2ec4ed625 100644 --- a/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss +++ b/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss @@ -5,7 +5,14 @@ --rd-diff-file-sticky-top-padding: calc(#{$calc-application-header-height} + #{$mr-sticky-header-height}); } -.js-merge-request-new-submit #diffs{ - position: relative; - z-index: 251; // If the settings menu displays upward on the create new/submit page, it needs to be in front of the tab bar -} \ No newline at end of file +.js-merge-request-new-submit .merge-request-tabs-holder{ + /* + If the settings menu displays upward on the create new/submit page, + it needs to be in front of the tab bar. + It's not clear that the previously-set z-index (250) has any effect + as of today (2025-03-12), but for now it feels safest to isolate + this change to just Rapid Diffs. + */ + z-index: unset; +} + -- GitLab From 69722b554ac7a8c06189494e6836f6e272099018 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Mar 2025 21:55:21 -0600 Subject: [PATCH 04/23] Test modifications that allow turning the sidebar off --- .../rapid_diffs/app_component_spec.rb | 35 ++++++++++++++++++- 1 file changed, 34 insertions(+), 1 deletion(-) diff --git a/spec/components/rapid_diffs/app_component_spec.rb b/spec/components/rapid_diffs/app_component_spec.rb index 5c34f7f6b66be5..edc5cbb8ade78d 100644 --- a/spec/components/rapid_diffs/app_component_spec.rb +++ b/spec/components/rapid_diffs/app_component_spec.rb @@ -10,6 +10,7 @@ let(:diff_view) { 'inline' } let(:update_user_endpoint) { '/update_user' } let(:metadata_endpoint) { '/metadata' } + let(:include_sidebar) { true } it "renders diffs slice" do render_component @@ -39,6 +40,37 @@ expect(container).not_to be_nil end + it "does not render sidebar if the initialization option is set to false" do + render_inline(described_class.new( + diffs_slice:, + stream_url:, + reload_stream_url:, + show_whitespace:, + diff_view:, + update_user_endpoint:, + metadata_endpoint:, + include_sidebar: false + )) + + expect(page).not_to have_css("[data-file-browser]") + end + + it "does not indicate the sidebar is open for the main content when the initialization option is set to false" do + render_inline(described_class.new( + diffs_slice:, + stream_url:, + reload_stream_url:, + show_whitespace:, + diff_view:, + update_user_endpoint:, + metadata_endpoint:, + include_sidebar: false + )) + + content = page.find(".rd-app-content") + expect(content['data-sidebar-visible']).to be_nil + end + it "sets sidebar width" do allow(vc_test_controller).to receive(:cookies).and_return({ mr_tree_list_width: '250' }) render_component @@ -77,7 +109,8 @@ def create_instance show_whitespace:, diff_view:, update_user_endpoint:, - metadata_endpoint: + metadata_endpoint:, + include_sidebar: ) end -- GitLab From c0dbb5834e0562cbbca2041cee5ee308d1f37a7e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Mar 2025 22:31:44 -0600 Subject: [PATCH 05/23] Test disabling the sidebar initializer --- spec/frontend/rapid_diffs/app/app_spec.js | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/spec/frontend/rapid_diffs/app/app_spec.js b/spec/frontend/rapid_diffs/app/app_spec.js index 75b65fe2a68c55..c378a4b3957fd6 100644 --- a/spec/frontend/rapid_diffs/app/app_spec.js +++ b/spec/frontend/rapid_diffs/app/app_spec.js @@ -23,11 +23,14 @@ describe('Rapid Diffs App', () => { app = createRapidDiffsApp(options); }; + const findSidebar = () => document.querySelector('.rd-app-sidebar'); + beforeEach(() => { createTestingPinia(); setHTMLFixture( `
+
`, @@ -55,6 +58,14 @@ describe('Rapid Diffs App', () => { expect(initFileBrowser).toHaveBeenCalled(); }); + it('does not start the sidebar app if the rapid diffs app was started with the option to disable it', () => { + createApp({ fileBrowser: false }); + + const sidebar = findSidebar(); + + expect(sidebar.childNodes.length).toBe(0); + }); + it('streams remaining diffs', () => { createApp(); app.streamRemainingDiffs(); -- GitLab From 60c69cbd6e5e65a1230a79c39aa15d3e435a8530 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Mar 2025 23:27:47 -0600 Subject: [PATCH 06/23] Test RD flag being set on the MR Tabs class --- spec/frontend/merge_request_tabs_spec.js | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/spec/frontend/merge_request_tabs_spec.js b/spec/frontend/merge_request_tabs_spec.js index 97c51f7a21b396..43e324ed2ac280 100644 --- a/spec/frontend/merge_request_tabs_spec.js +++ b/spec/frontend/merge_request_tabs_spec.js @@ -4,6 +4,7 @@ import htmlMergeRequestsWithTaskList from 'test_fixtures/merge_requests/merge_re import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; import initMrPage from 'helpers/init_vue_mr_page_helper'; import { stubPerformanceWebAPI } from 'helpers/performance'; +import setWindowLocation from 'helpers/set_window_location_helper'; import axios from '~/lib/utils/axios_utils'; import MergeRequestTabs, { getActionFromHref } from '~/merge_request_tabs'; import Diff from '~/diff'; @@ -50,6 +51,15 @@ describe('MergeRequestTabs', () => { document.body.innerHTML = ''; }); + describe('constructor', () => { + it('infers "Rapid Diffs" mode from the URL parameter', () => { + setWindowLocation('https://example.com?rapid_diffs=true'); + const tabs = new MergeRequestTabs(); + + expect(tabs.isRapidDiffs).toBe(true); + }); + }); + describe('clickTab', () => { let params; -- GitLab From a303f308fffdeafcc6d69105bd4ddb8d1676c4a1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 12 Mar 2025 23:32:30 -0600 Subject: [PATCH 07/23] Fix rubocop complaints Do not mix explicit and implicit hash values I don't know if this will fix it --- .../rapid_diffs/app_component_spec.rb | 28 +++++++++---------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/spec/components/rapid_diffs/app_component_spec.rb b/spec/components/rapid_diffs/app_component_spec.rb index edc5cbb8ade78d..9d5824055b6ded 100644 --- a/spec/components/rapid_diffs/app_component_spec.rb +++ b/spec/components/rapid_diffs/app_component_spec.rb @@ -42,13 +42,13 @@ it "does not render sidebar if the initialization option is set to false" do render_inline(described_class.new( - diffs_slice:, - stream_url:, - reload_stream_url:, - show_whitespace:, - diff_view:, - update_user_endpoint:, - metadata_endpoint:, + diffs_slice: diffs_slice, + stream_url: stream_url, + reload_stream_url: reload_stream_url, + show_whitespace: show_whitespace, + diff_view: diff_view, + update_user_endpoint: update_user_endpoint, + metadata_endpoint: metadata_endpoint, include_sidebar: false )) @@ -57,13 +57,13 @@ it "does not indicate the sidebar is open for the main content when the initialization option is set to false" do render_inline(described_class.new( - diffs_slice:, - stream_url:, - reload_stream_url:, - show_whitespace:, - diff_view:, - update_user_endpoint:, - metadata_endpoint:, + diffs_slice: diffs_slice, + stream_url: stream_url, + reload_stream_url: reload_stream_url, + show_whitespace: show_whitespace, + diff_view: diff_view, + update_user_endpoint: update_user_endpoint, + metadata_endpoint: metadata_endpoint, include_sidebar: false )) -- GitLab From e83eff19b2638c05ec7baec6a144ff7ae8fcbb75 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 14 Mar 2025 14:51:12 -0600 Subject: [PATCH 08/23] Don't try to instantiate the Rapid Diffs app more than once It could happen on any tab switch --- app/assets/javascripts/merge_request_tabs.js | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 0a5079d5b6dd77..98d5ae87ebba7f 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -223,6 +223,7 @@ export default class MergeRequestTabs { this.commitsLoaded = false; this.isFixedLayoutPreferred = this.contentWrapper.classList.contains('container-limited'); this.isRapidDiffs = getParameterByName('rapid_diffs') === 'true'; + this.rapidDiffsApp = null; this.eventHub = createEventHub(); this.loadedPages = { [action]: true }; @@ -533,10 +534,12 @@ export default class MergeRequestTabs { // Initialize the Changes tab startDiffs(options = {}) { if (this.isRapidDiffs) { - const rd = createRapidDiffsApp(); + if (!this.rapidDiffsApp){ + this.rapidDiffsApp = createRapidDiffsApp(); - rd.streamRemainingDiffs(); - rd.init(); + this.rapidDiffsApp.streamRemainingDiffs(); + this.rapidDiffsApp.init(); + } } else { this.loadDiff(options); } -- GitLab From 5cd11d7371a5d78961ef0e00c719ac0b77fc54dd Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 14 Mar 2025 14:51:35 -0600 Subject: [PATCH 09/23] Disable the file browser when instantiating on the MR Create page --- app/assets/javascripts/merge_request_tabs.js | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 98d5ae87ebba7f..a3d99d5dc0ec7a 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -534,8 +534,8 @@ export default class MergeRequestTabs { // Initialize the Changes tab startDiffs(options = {}) { if (this.isRapidDiffs) { - if (!this.rapidDiffsApp){ - this.rapidDiffsApp = createRapidDiffsApp(); + if (!this.rapidDiffsApp) { + this.rapidDiffsApp = createRapidDiffsApp({ fileBrowser: false }); this.rapidDiffsApp.streamRemainingDiffs(); this.rapidDiffsApp.init(); -- GitLab From 174346549724c64375996f7ae0a7a7baaa897cb9 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Fri, 14 Mar 2025 15:34:03 -0600 Subject: [PATCH 10/23] Render RD on any action because it's just a tab click away --- config/routes/merge_requests.rb | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/config/routes/merge_requests.rb b/config/routes/merge_requests.rb index 976d10f9237b23..7b28a748f3598e 100644 --- a/config/routes/merge_requests.rb +++ b/config/routes/merge_requests.rb @@ -79,6 +79,8 @@ post '', action: :create, as: nil scope path: 'new', as: :new_merge_request do + get '', action: :new, to: 'merge_requests/creations#rapid_diffs', defaults: { tab: 'commits' }, + constraints: ->(params) { params[:rapid_diffs] == 'true' } get '', action: :new scope constraints: ->(req) { req.format == :json }, as: :json do @@ -91,6 +93,8 @@ get :diffs, to: 'merge_requests/creations#rapid_diffs', defaults: { tab: 'diffs' }, constraints: ->(params) { params[:rapid_diffs] == 'true' } get :diffs, defaults: { tab: 'diffs' } + get :pipelines, to: 'merge_requests/creations#rapid_diffs', defaults: { tab: 'pipelines' }, + constraints: ->(params) { params[:rapid_diffs] == 'true' } get :pipelines, defaults: { tab: 'pipelines' } end -- GitLab From 7311a99e01609adaba6ea4f150224ff664b053fc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 13:33:50 -0600 Subject: [PATCH 11/23] Remove option to disable the file browser sidebar in rapid diffs --- app/assets/javascripts/merge_request_tabs.js | 2 +- .../javascripts/rapid_diffs/app/index.js | 27 ++++++++----------- .../rapid_diffs/app_component.html.haml | 9 +++---- app/components/rapid_diffs/app_component.rb | 2 -- .../creations/rapid_diffs.html.haml | 2 +- spec/frontend/rapid_diffs/app/app_spec.js | 11 -------- 6 files changed, 17 insertions(+), 36 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index a3d99d5dc0ec7a..d1316e76b1a211 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -535,7 +535,7 @@ export default class MergeRequestTabs { startDiffs(options = {}) { if (this.isRapidDiffs) { if (!this.rapidDiffsApp) { - this.rapidDiffsApp = createRapidDiffsApp({ fileBrowser: false }); + this.rapidDiffsApp = createRapidDiffsApp(); this.rapidDiffsApp.streamRemainingDiffs(); this.rapidDiffsApp.init(); diff --git a/app/assets/javascripts/rapid_diffs/app/index.js b/app/assets/javascripts/rapid_diffs/app/index.js index 5bc0122104a0bd..5a896ef7bc6505 100644 --- a/app/assets/javascripts/rapid_diffs/app/index.js +++ b/app/assets/javascripts/rapid_diffs/app/index.js @@ -13,9 +13,8 @@ import { __ } from '~/locale'; // This facade interface joins together all the bits and pieces of Rapid Diffs: DiffFile, Settings, File browser, etc. // It's a unified entrypoint for Rapid Diffs and all external communications should happen through this interface. class RapidDiffsFacade { - constructor({ DiffFileImplementation = DiffFile, fileBrowser = true } = {}) { + constructor({ DiffFileImplementation = DiffFile } = {}) { this.DiffFileImplementation = DiffFileImplementation; - this.flags = { fileBrowser }; } init() { @@ -23,21 +22,17 @@ class RapidDiffsFacade { const { reloadStreamUrl, metadataEndpoint } = document.querySelector('[data-rapid-diffs]').dataset; useDiffsView(pinia).metadataEndpoint = metadataEndpoint; - - if (this.flags.fileBrowser) { - useDiffsView(pinia) - .loadMetadata() - .then(() => { - initHiddenFilesWarning(); - initFileBrowser(); - }) - .catch(() => { - createAlert({ - message: __('Failed to load additional diffs information. Try reloading the page.'), - }); + useDiffsView(pinia) + .loadMetadata() + .then(() => { + initHiddenFilesWarning(); + initFileBrowser(); + }) + .catch(() => { + createAlert({ + message: __('Failed to load additional diffs information. Try reloading the page.'), }); - } - + }); initViewSettings({ pinia, streamUrl: reloadStreamUrl }); } diff --git a/app/components/rapid_diffs/app_component.html.haml b/app/components/rapid_diffs/app_component.html.haml index a29391454bbd95..5b687a7e7acd08 100644 --- a/app/components/rapid_diffs/app_component.html.haml +++ b/app/components/rapid_diffs/app_component.html.haml @@ -15,11 +15,10 @@ .rd-app-settings %div{ data: { view_settings: true, show_whitespace: @show_whitespace.to_json, diff_view_type: @diff_view, update_user_endpoint: @update_user_endpoint } } .rd-app-body - - if @include_sidebar - .rd-app-sidebar{ data: { file_browser: true }, style: ("width: #{initial_sidebar_width}px" if initial_sidebar_width) } - .rd-app-sidebar-loading - = helpers.gl_loading_icon(size: 'sm') - .rd-app-content{ data: { sidebar_visible: @include_sidebar } } + .rd-app-sidebar{ data: { file_browser: true }, style: ("width: #{initial_sidebar_width}px" if initial_sidebar_width) } + .rd-app-sidebar-loading + = helpers.gl_loading_icon(size: 'sm') + .rd-app-content{ data: { sidebar_visible: true } } .rd-app-content-header{ data: { hidden_files_warning: true } } .code{ class: helpers.user_color_scheme } %div{ data: { diffs_list: true } } diff --git a/app/components/rapid_diffs/app_component.rb b/app/components/rapid_diffs/app_component.rb index e22f3e3e596066..70ae3056aee632 100644 --- a/app/components/rapid_diffs/app_component.rb +++ b/app/components/rapid_diffs/app_component.rb @@ -13,7 +13,6 @@ def initialize( update_user_endpoint:, metadata_endpoint:, preload: true - include_sidebar: ) @diffs_slice = diffs_slice @reload_stream_url = reload_stream_url @@ -23,7 +22,6 @@ def initialize( @update_user_endpoint = update_user_endpoint @metadata_endpoint = metadata_endpoint @preload = preload - @include_sidebar = include_sidebar end def initial_sidebar_width diff --git a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml index 37f07f0a2ad28e..8e114f6785c9c9 100644 --- a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml +++ b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml @@ -3,5 +3,5 @@ = render "page" -- args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil, include_sidebar: false } +- args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil } = render ::RapidDiffs::AppComponent.new(**args) diff --git a/spec/frontend/rapid_diffs/app/app_spec.js b/spec/frontend/rapid_diffs/app/app_spec.js index c378a4b3957fd6..75b65fe2a68c55 100644 --- a/spec/frontend/rapid_diffs/app/app_spec.js +++ b/spec/frontend/rapid_diffs/app/app_spec.js @@ -23,14 +23,11 @@ describe('Rapid Diffs App', () => { app = createRapidDiffsApp(options); }; - const findSidebar = () => document.querySelector('.rd-app-sidebar'); - beforeEach(() => { createTestingPinia(); setHTMLFixture( `
-
`, @@ -58,14 +55,6 @@ describe('Rapid Diffs App', () => { expect(initFileBrowser).toHaveBeenCalled(); }); - it('does not start the sidebar app if the rapid diffs app was started with the option to disable it', () => { - createApp({ fileBrowser: false }); - - const sidebar = findSidebar(); - - expect(sidebar.childNodes.length).toBe(0); - }); - it('streams remaining diffs', () => { createApp(); app.streamRemainingDiffs(); -- GitLab From 5f9221d7256233decc7c097c45b8e52e098befb0 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 14:15:47 -0600 Subject: [PATCH 12/23] Switch from stream to reload --- app/assets/javascripts/merge_request_tabs.js | 2 +- app/assets/javascripts/rapid_diffs/app/index.js | 7 +++++++ spec/frontend/rapid_diffs/app/app_spec.js | 6 ++++++ 3 files changed, 14 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index d1316e76b1a211..9c15f837c5ab3e 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -537,7 +537,7 @@ export default class MergeRequestTabs { if (!this.rapidDiffsApp) { this.rapidDiffsApp = createRapidDiffsApp(); - this.rapidDiffsApp.streamRemainingDiffs(); + this.rapidDiffsApp.reloadDiffs(); this.rapidDiffsApp.init(); } } else { diff --git a/app/assets/javascripts/rapid_diffs/app/index.js b/app/assets/javascripts/rapid_diffs/app/index.js index 5a896ef7bc6505..161fc98d5f68ec 100644 --- a/app/assets/javascripts/rapid_diffs/app/index.js +++ b/app/assets/javascripts/rapid_diffs/app/index.js @@ -44,6 +44,13 @@ class RapidDiffsFacade { return useDiffsList(pinia).streamRemainingDiffs(streamContainer.dataset.diffsStreamUrl); } + // eslint-disable-next-line class-methods-use-this + reloadDiffs() { + const { reloadStreamUrl } = document.querySelector('[data-rapid-diffs]').dataset; + + return useDiffsList(pinia).reloadDiffs(reloadStreamUrl); + } + #registerCustomElements() { customElements.define('diff-file', this.DiffFileImplementation); customElements.define('diff-file-mounted', DiffFileMounted); diff --git a/spec/frontend/rapid_diffs/app/app_spec.js b/spec/frontend/rapid_diffs/app/app_spec.js index 75b65fe2a68c55..5109e2ada8fe58 100644 --- a/spec/frontend/rapid_diffs/app/app_spec.js +++ b/spec/frontend/rapid_diffs/app/app_spec.js @@ -60,4 +60,10 @@ describe('Rapid Diffs App', () => { app.streamRemainingDiffs(); expect(useDiffsList().streamRemainingDiffs).toHaveBeenCalledWith('/stream'); }); + + it('reloads diff files', () => { + createApp(); + app.reloadDiffs(); + expect(useDiffsList().reloadDiffs).toHaveBeenCalledWith('/reload'); + }); }); -- GitLab From ed035039949256eccba722c298cb64da68a7b375 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 14:26:59 -0600 Subject: [PATCH 13/23] Remove class selector specificity --- .../projects/merge_requests/creations/rapid_diffs/index.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js b/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js index 62eb4e92ef3c9f..1f128580882b56 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js +++ b/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js @@ -3,7 +3,7 @@ import initPipelines from '~/commit/pipelines/pipelines_bundle'; import MergeRequest from '~/merge_request'; const mrNewSubmitNode = document.querySelector('.js-merge-request-new-submit'); -const rapidDiffsApp = document.querySelector('.rd-app[data-rapid-diffs]'); +const rapidDiffsApp = document.querySelector('[data-rapid-diffs]'); const diffsPane = mrNewSubmitNode?.querySelector('#diffs'); if (diffsPane && rapidDiffsApp) { -- GitLab From 081909b668b75067fa699ddb4c5d60202fbdc84f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 14:35:04 -0600 Subject: [PATCH 14/23] Use yield to place the RD app inside the MR creation Changes tab --- .../projects/merge_requests/creations/rapid_diffs/index.js | 6 ------ .../merge_requests/creations/_new_submit.html.haml | 1 + .../projects/merge_requests/creations/_page.html.haml | 3 ++- .../merge_requests/creations/rapid_diffs.html.haml | 7 +++---- 4 files changed, 6 insertions(+), 11 deletions(-) diff --git a/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js b/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js index 1f128580882b56..5b2f57449bb2dc 100644 --- a/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js +++ b/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js @@ -3,12 +3,6 @@ import initPipelines from '~/commit/pipelines/pipelines_bundle'; import MergeRequest from '~/merge_request'; const mrNewSubmitNode = document.querySelector('.js-merge-request-new-submit'); -const rapidDiffsApp = document.querySelector('[data-rapid-diffs]'); -const diffsPane = mrNewSubmitNode?.querySelector('#diffs'); - -if (diffsPane && rapidDiffsApp) { - diffsPane.appendChild(rapidDiffsApp); -} // eslint-disable-next-line no-new new MergeRequest({ diff --git a/app/views/projects/merge_requests/creations/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml index d0e44258987590..40338fec690179 100644 --- a/app/views/projects/merge_requests/creations/_new_submit.html.haml +++ b/app/views/projects/merge_requests/creations/_new_submit.html.haml @@ -57,6 +57,7 @@ = render "projects/merge_requests/commits" #diffs.diffs.tab-pane{ class: "!gl-m-0" } -# This tab is always loaded via AJAX + = yield - if @pipelines.any? #pipelines.pipelines.tab-pane = render 'projects/merge_requests/pipelines', endpoint: url_for(safe_params.merge(action: 'pipelines', format: :json)), disable_initialization: true diff --git a/app/views/projects/merge_requests/creations/_page.html.haml b/app/views/projects/merge_requests/creations/_page.html.haml index facb6727946e08..3754eecda67c17 100644 --- a/app/views/projects/merge_requests/creations/_page.html.haml +++ b/app/views/projects/merge_requests/creations/_page.html.haml @@ -8,7 +8,8 @@ - conflicting_mr = @merge_request.existing_mrs_targeting_same_branch.first - if @merge_request.can_be_created && !params[:change_branches] && !conflicting_mr - = render 'new_submit' + = render 'new_submit' do + = yield - else - if conflicting_mr - link_to_mr = link_to(conflicting_mr.to_reference, project_merge_request_path(conflicting_mr.target_project, conflicting_mr)) diff --git a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml index 8e114f6785c9c9..9bf1685fa17b94 100644 --- a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml +++ b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml @@ -1,7 +1,6 @@ - add_page_specific_style 'page_bundles/merge_request_rapid_diffs' - @content_class = 'rd-page-container' -= render "page" - -- args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil } -= render ::RapidDiffs::AppComponent.new(**args) += render "page" do + - args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil } + = render ::RapidDiffs::AppComponent.new(**args) -- GitLab From 3d05bb6adcc7fd9a4987dce21aed9d3201469e66 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 14:41:02 -0600 Subject: [PATCH 15/23] Remove rd-page-container content class --- .../projects/merge_requests/creations/rapid_diffs.html.haml | 1 - 1 file changed, 1 deletion(-) diff --git a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml index 9bf1685fa17b94..b5a1e94b549983 100644 --- a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml +++ b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml @@ -1,5 +1,4 @@ - add_page_specific_style 'page_bundles/merge_request_rapid_diffs' -- @content_class = 'rd-page-container' = render "page" do - args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil } -- GitLab From e5084761971a2202083215b06bab5895a9530973 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 14:51:45 -0600 Subject: [PATCH 16/23] Disable the preload functionality to avoid unnecessary bandwidth --- .../projects/merge_requests/creations/rapid_diffs.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml index b5a1e94b549983..21c3b1feb933d3 100644 --- a/app/views/projects/merge_requests/creations/rapid_diffs.html.haml +++ b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml @@ -1,5 +1,5 @@ - add_page_specific_style 'page_bundles/merge_request_rapid_diffs' = render "page" do - - args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil } + - args = { diffs_slice: nil, reload_stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), stream_url: project_new_merge_request_diffs_stream_path(@project, merge_request: { source_branch: @merge_request.source_branch, target_branch: @merge_request.target_branch }), show_whitespace: @show_whitespace_default, diff_view: diff_view, update_user_endpoint: expose_path(api_v4_user_preferences_path), metadata_endpoint: nil, preload: false } = render ::RapidDiffs::AppComponent.new(**args) -- GitLab From 9d5bfd59d86047a761c7b05355f55c577bdb949b Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 15:02:50 -0600 Subject: [PATCH 17/23] Don't load the Rapid Diffs app code until the tab is selected https://gitlab.com/gitlab-org/gitlab/-/merge_requests/180675#note_2400360527 --- app/assets/javascripts/merge_request_tabs.js | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 9c15f837c5ab3e..4312c8f8eb2ae7 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -8,7 +8,6 @@ import { getCookie, isMetaClick, parseBoolean, scrollToElement } from '~/lib/uti import { parseUrlPathname, visitUrl, getParameterByName } from '~/lib/utils/url_utility'; import createEventHub from '~/helpers/event_hub_factory'; import { renderGFM } from '~/behaviors/markdown/render_gfm'; -import { createRapidDiffsApp } from '~/rapid_diffs/app'; import BlobForkSuggestion from './blob/blob_fork_suggestion'; import Diff from './diff'; import { initDiffStatsDropdown } from './init_diff_stats_dropdown'; @@ -532,9 +531,11 @@ export default class MergeRequestTabs { } // Initialize the Changes tab - startDiffs(options = {}) { + async startDiffs(options = {}) { if (this.isRapidDiffs) { if (!this.rapidDiffsApp) { + const { createRapidDiffsApp } = await import('~/rapid_diffs/app'); + this.rapidDiffsApp = createRapidDiffsApp(); this.rapidDiffsApp.reloadDiffs(); -- GitLab From 9042806fdec7365f429d59ee2dda1f91bc77e0c1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 15:55:37 -0600 Subject: [PATCH 18/23] Remove `include_sidebar` from non-MR create rapid diffs instances --- app/views/projects/commit/rapid_diffs.html.haml | 2 +- app/views/projects/merge_requests/rapid_diffs.html.haml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/projects/commit/rapid_diffs.html.haml b/app/views/projects/commit/rapid_diffs.html.haml index 3959253f2ec0c2..4a9df13a4ad301 100644 --- a/app/views/projects/commit/rapid_diffs.html.haml +++ b/app/views/projects/commit/rapid_diffs.html.haml @@ -11,5 +11,5 @@ .container-fluid{ class: [container_class] } = render "commit_box" = render "ci_menu" - - args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url, include_sidebar: true } + - args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url } = render ::RapidDiffs::AppComponent.new(**args) diff --git a/app/views/projects/merge_requests/rapid_diffs.html.haml b/app/views/projects/merge_requests/rapid_diffs.html.haml index 787af4987a28bd..8037561944d0d3 100644 --- a/app/views/projects/merge_requests/rapid_diffs.html.haml +++ b/app/views/projects/merge_requests/rapid_diffs.html.haml @@ -3,7 +3,7 @@ - @content_class = 'rd-page-container diffs-container-limited' = render 'page' -- args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url, include_sidebar: true } +- args = { diffs_slice: @diffs_slice, reload_stream_url: @reload_stream_url, stream_url: @stream_url, show_whitespace: @show_whitespace_default, diff_view: @diff_view, update_user_endpoint: @update_current_user_path, metadata_endpoint: @endpoint_metadata_url } = render ::RapidDiffs::AppComponent.new(**args) do |c| - c.with_diffs_list do = render RapidDiffs::MergeRequestDiffFileComponent.with_collection(@diffs_slice, merge_request: @merge_request, parallel_view: @diff_view == :parallel) -- GitLab From a1d5a4ec31538134847d2a586c1989f42f6c2cd8 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 17 Mar 2025 15:55:58 -0600 Subject: [PATCH 19/23] Remove tests for disabling the sidebar --- .../rapid_diffs/app_component_spec.rb | 35 +------------------ 1 file changed, 1 insertion(+), 34 deletions(-) diff --git a/spec/components/rapid_diffs/app_component_spec.rb b/spec/components/rapid_diffs/app_component_spec.rb index 9d5824055b6ded..5c34f7f6b66be5 100644 --- a/spec/components/rapid_diffs/app_component_spec.rb +++ b/spec/components/rapid_diffs/app_component_spec.rb @@ -10,7 +10,6 @@ let(:diff_view) { 'inline' } let(:update_user_endpoint) { '/update_user' } let(:metadata_endpoint) { '/metadata' } - let(:include_sidebar) { true } it "renders diffs slice" do render_component @@ -40,37 +39,6 @@ expect(container).not_to be_nil end - it "does not render sidebar if the initialization option is set to false" do - render_inline(described_class.new( - diffs_slice: diffs_slice, - stream_url: stream_url, - reload_stream_url: reload_stream_url, - show_whitespace: show_whitespace, - diff_view: diff_view, - update_user_endpoint: update_user_endpoint, - metadata_endpoint: metadata_endpoint, - include_sidebar: false - )) - - expect(page).not_to have_css("[data-file-browser]") - end - - it "does not indicate the sidebar is open for the main content when the initialization option is set to false" do - render_inline(described_class.new( - diffs_slice: diffs_slice, - stream_url: stream_url, - reload_stream_url: reload_stream_url, - show_whitespace: show_whitespace, - diff_view: diff_view, - update_user_endpoint: update_user_endpoint, - metadata_endpoint: metadata_endpoint, - include_sidebar: false - )) - - content = page.find(".rd-app-content") - expect(content['data-sidebar-visible']).to be_nil - end - it "sets sidebar width" do allow(vc_test_controller).to receive(:cookies).and_return({ mr_tree_list_width: '250' }) render_component @@ -109,8 +77,7 @@ def create_instance show_whitespace:, diff_view:, update_user_endpoint:, - metadata_endpoint:, - include_sidebar: + metadata_endpoint: ) end -- GitLab From b45d62a020610dd6d11f61eac4700513252967ba Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Tue, 18 Mar 2025 14:55:40 +0000 Subject: [PATCH 20/23] Added rapid diffs action request spec --- .../creations_controller_spec.rb | 55 +++++++++++++++++++ 1 file changed, 55 insertions(+) create mode 100644 spec/requests/projects/merge_requests/creations_controller_spec.rb diff --git a/spec/requests/projects/merge_requests/creations_controller_spec.rb b/spec/requests/projects/merge_requests/creations_controller_spec.rb new file mode 100644 index 00000000000000..7cf92ec85dd53d --- /dev/null +++ b/spec/requests/projects/merge_requests/creations_controller_spec.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Merge Request Creation', feature_category: :code_review_workflow do + let_it_be(:project) { create(:project, :public, :repository) } + let_it_be(:user) { create(:user, maintainer_of: project) } + let_it_be(:source_branch) { 'fix' } + let_it_be(:target_branch) { 'master' } + + before do + sign_in(user) + end + + describe 'GET rapid_diffs' do + def get_diffs(**extra_params) + params = { + namespace_id: project.namespace, + project_id: project, + merge_request: { + source_branch: source_branch, + target_branch: target_branch + } + } + + get namespace_project_new_merge_request_diffs_path(params.merge(extra_params)) + end + + context 'when the feature flag rapid_diffs is disabled' do + before do + stub_feature_flags(rapid_diffs: false) + end + + it 'returns 404' do + get_diffs(rapid_diffs: 'true') + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + it 'does not use rapid diffs action when rapid_diffs flag is false' do + get_diffs(rapid_diffs: 'false') + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to include('data-page="projects:merge_requests:creations:new"') + end + + it 'uses rapid diffs action when rapid_diffs flag is true' do + get_diffs(rapid_diffs: 'true') + + expect(response).to have_gitlab_http_status(:ok) + expect(response.body).to include('data-page="projects:merge_requests:creations:rapid_diffs"') + end + end +end -- GitLab From 3910d13d33f5e3db4baa02e2f90b45cfe794dfa7 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 19 Mar 2025 18:22:37 -0600 Subject: [PATCH 21/23] Test that the RD app is created when switching to the diffs tab --- spec/frontend/merge_request_tabs_spec.js | 47 ++++++++++++++++++++++++ 1 file changed, 47 insertions(+) diff --git a/spec/frontend/merge_request_tabs_spec.js b/spec/frontend/merge_request_tabs_spec.js index 43e324ed2ac280..0f538a6e0f376e 100644 --- a/spec/frontend/merge_request_tabs_spec.js +++ b/spec/frontend/merge_request_tabs_spec.js @@ -5,6 +5,7 @@ import { setHTMLFixture, resetHTMLFixture } from 'helpers/fixtures'; import initMrPage from 'helpers/init_vue_mr_page_helper'; import { stubPerformanceWebAPI } from 'helpers/performance'; import setWindowLocation from 'helpers/set_window_location_helper'; +import waitForPromises from 'helpers/wait_for_promises'; import axios from '~/lib/utils/axios_utils'; import MergeRequestTabs, { getActionFromHref } from '~/merge_request_tabs'; import Diff from '~/diff'; @@ -15,6 +16,13 @@ jest.mock('~/lib/utils/webpack', () => ({ resetServiceWorkersPublicPath: jest.fn(), })); +const mockCreateRapidDiffsApp = jest + .fn() + .mockReturnValue({ reloadDiffs: jest.fn(), init: jest.fn() }); +jest.mock('~/rapid_diffs/app', () => ({ + createRapidDiffsApp: mockCreateRapidDiffsApp, +})); + jest.mock('~/lib/utils/url_utility', () => ({ ...jest.requireActual('~/lib/utils/url_utility'), visitUrl: jest.fn(), @@ -439,6 +447,45 @@ describe('MergeRequestTabs', () => { expect(window.scrollTo).not.toHaveBeenCalled(); }); }); + + describe('switching to the diffs tab', () => { + beforeEach(() => { + setWindowLocation('https://example.com'); + mockCreateRapidDiffsApp.mockClear(); + }); + + it.each` + rdEnabled | rdExists | isConstructed + ${true} | ${false} | ${true} + ${true} | ${true} | ${false} + ${false} | ${false} | ${false} + `( + 'creates the Rapid Diffs app only when on the diffs page that does not already have the app initialized ($rdExists), and where the feature is enabled in the search parameters ($rdEnabled)', + async ({ rdEnabled, rdExists, isConstructed }) => { + if (rdEnabled) { + setWindowLocation('https://example.com?rapid_diffs=true'); + } + + testContext.class = new MergeRequestTabs({ stubLocation }); + + if (rdExists) { + testContext.class.rapidDiffsApp = 'non-falsey'; + } + + testContext.class.tabShown('diffs', 'not-a-vue-page'); + + await waitForPromises(); + + if (isConstructed) { + expect(mockCreateRapidDiffsApp).toHaveBeenCalledTimes(1); + expect(testContext.class.rapidDiffsApp).toEqual(expect.any(Object)); + } else { + expect(mockCreateRapidDiffsApp).toHaveBeenCalledTimes(0); + expect(testContext.class.rapidDiffsApp).toBe(rdExists ? 'non-falsey' : null); + } + }, + ); + }); }); describe('tabs <-> diff interactions', () => { -- GitLab From fadb07bf9932dfb1fa2b8080e509d37399192e2e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 19 Mar 2025 23:39:19 -0600 Subject: [PATCH 22/23] Use .not.toHaveBeenCalled --- spec/frontend/merge_request_tabs_spec.js | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/frontend/merge_request_tabs_spec.js b/spec/frontend/merge_request_tabs_spec.js index 0f538a6e0f376e..e0ce216f0d8788 100644 --- a/spec/frontend/merge_request_tabs_spec.js +++ b/spec/frontend/merge_request_tabs_spec.js @@ -480,7 +480,7 @@ describe('MergeRequestTabs', () => { expect(mockCreateRapidDiffsApp).toHaveBeenCalledTimes(1); expect(testContext.class.rapidDiffsApp).toEqual(expect.any(Object)); } else { - expect(mockCreateRapidDiffsApp).toHaveBeenCalledTimes(0); + expect(mockCreateRapidDiffsApp).not.toHaveBeenCalled(); expect(testContext.class.rapidDiffsApp).toBe(rdExists ? 'non-falsey' : null); } }, -- GitLab From e6dad26989114ffb22c66c205fd0f9bdf4dae573 Mon Sep 17 00:00:00 2001 From: Phil Hughes Date: Thu, 20 Mar 2025 09:56:11 +0000 Subject: [PATCH 23/23] Added additional specs to creations_controller_spec --- .../merge_requests/creations_controller_spec.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/spec/requests/projects/merge_requests/creations_controller_spec.rb b/spec/requests/projects/merge_requests/creations_controller_spec.rb index 7cf92ec85dd53d..0aa9e745ccb282 100644 --- a/spec/requests/projects/merge_requests/creations_controller_spec.rb +++ b/spec/requests/projects/merge_requests/creations_controller_spec.rb @@ -51,5 +51,18 @@ def get_diffs(**extra_params) expect(response).to have_gitlab_http_status(:ok) expect(response.body).to include('data-page="projects:merge_requests:creations:rapid_diffs"') end + + it 'sets flash alert when there is an existing MR targeting same branch' do + create(:merge_request, source_project: project, source_branch: source_branch, target_branch: target_branch) + get_diffs(rapid_diffs: 'true') + + expect(flash[:alert]).to be_present + end + + it 'assigns show_whitespace_default' do + get_diffs(rapid_diffs: 'true') + + expect(assigns(:show_whitespace_default)).to be(true) + end end end -- GitLab