diff --git a/app/assets/javascripts/merge_request_tabs.js b/app/assets/javascripts/merge_request_tabs.js index 705beb92dad35acc6995a3fe5f76c3d2f2533f28..4312c8f8eb2ae7cc3dfad99a9ceb397abdfe8811 100644 --- a/app/assets/javascripts/merge_request_tabs.js +++ b/app/assets/javascripts/merge_request_tabs.js @@ -5,7 +5,7 @@ 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 BlobForkSuggestion from './blob/blob_fork_suggestion'; @@ -221,6 +221,8 @@ 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.rapidDiffsApp = null; 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,22 @@ export default class MergeRequestTabs { this.mergeRequestPipelinesTable = mountPipelines(); } - // load the diff tab content from the backend + // Initialize the Changes tab + async startDiffs(options = {}) { + if (this.isRapidDiffs) { + if (!this.rapidDiffsApp) { + const { createRapidDiffsApp } = await import('~/rapid_diffs/app'); + + this.rapidDiffsApp = createRapidDiffsApp(); + + this.rapidDiffsApp.reloadDiffs(); + this.rapidDiffsApp.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 0000000000000000000000000000000000000000..5b2f57449bb2dc2327c927b42f6377e89db4f6d2 --- /dev/null +++ b/app/assets/javascripts/pages/projects/merge_requests/creations/rapid_diffs/index.js @@ -0,0 +1,12 @@ +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'); + +// 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 5a896ef7bc650530fb0561664be9864a780e8095..161fc98d5f68ec0fdcfee54654580de874dedb2e 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/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss b/app/assets/stylesheets/page_bundles/merge_request_rapid_diffs.scss index 65707eeb4a9b9cf0010ea1553deb672b6e760e3a..f961b2ec4ed625047418a89c75be1e94b7edbcf7 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,15 @@ .rd-app { --rd-diff-file-sticky-top-padding: calc(#{$calc-application-header-height} + #{$mr-sticky-header-height}); } + +.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; +} + diff --git a/app/controllers/projects/merge_requests/creations_controller.rb b/app/controllers/projects/merge_requests/creations_controller.rb index e8e409f8917eee1081fc72b3ad6f444c5f2caa8e..8ee9a7e6f03bcaa8b5ae605a9c366bec0f1cf3e7 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/_new_submit.html.haml b/app/views/projects/merge_requests/creations/_new_submit.html.haml index d0e44258987590d0c8962058b9f9bdc3c32512fa..40338fec6901796f4c7a6fb9bd41c297744c40da 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 new file mode 100644 index 0000000000000000000000000000000000000000..3754eecda67c1752217ec7c5a9a405410c29d0fe --- /dev/null +++ b/app/views/projects/merge_requests/creations/_page.html.haml @@ -0,0 +1,18 @@ +- 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' 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)) + - 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 facb6727946e08403a3ebb7349cb08f48724499c..b810c8659bdbab683239a70dbba513d11005774a 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 0000000000000000000000000000000000000000..21c3b1feb933d3c5b94c0b23e013d2272984a87c --- /dev/null +++ b/app/views/projects/merge_requests/creations/rapid_diffs.html.haml @@ -0,0 +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, preload: false } + = render ::RapidDiffs::AppComponent.new(**args) diff --git a/config/routes/merge_requests.rb b/config/routes/merge_requests.rb index ee4e2e5d529e274b5ad34a3353c2b2cbbed1fa13..7b28a748f3598e6ef9006c8f50d8eabf4d314d99 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 @@ -88,7 +90,11 @@ 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, to: 'merge_requests/creations#rapid_diffs', defaults: { tab: 'pipelines' }, + constraints: ->(params) { params[:rapid_diffs] == 'true' } get :pipelines, defaults: { tab: 'pipelines' } end diff --git a/spec/frontend/merge_request_tabs_spec.js b/spec/frontend/merge_request_tabs_spec.js index 97c51f7a21b396feb7d80ee2ee69a7be1a1e3331..e0ce216f0d87881963bb838101e7ac7048952545 100644 --- a/spec/frontend/merge_request_tabs_spec.js +++ b/spec/frontend/merge_request_tabs_spec.js @@ -4,6 +4,8 @@ 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 waitForPromises from 'helpers/wait_for_promises'; import axios from '~/lib/utils/axios_utils'; import MergeRequestTabs, { getActionFromHref } from '~/merge_request_tabs'; import Diff from '~/diff'; @@ -14,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(), @@ -50,6 +59,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; @@ -429,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).not.toHaveBeenCalled(); + expect(testContext.class.rapidDiffsApp).toBe(rdExists ? 'non-falsey' : null); + } + }, + ); + }); }); describe('tabs <-> diff interactions', () => { diff --git a/spec/frontend/rapid_diffs/app/app_spec.js b/spec/frontend/rapid_diffs/app/app_spec.js index 75b65fe2a68c556d9dabb395ce947023ebde6aed..5109e2ada8fe581120eacb05339924dbabb26bc1 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'); + }); }); 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 0000000000000000000000000000000000000000..0aa9e745ccb282b800fa806df1d0d69ff40318df --- /dev/null +++ b/spec/requests/projects/merge_requests/creations_controller_spec.rb @@ -0,0 +1,68 @@ +# 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 + + 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