Merge branch...

Merge branch '232509-improve-performance-of-show-action-for-projects-commitcontroller-under-load-into-s2-tier' into 'master'

Load commit diff_files asynchronously

See merge request gitlab-org/gitlab!38450

app/assets/javascripts/pages/projects/commit/show/index.js

@@ -7,23 +7,47 @@ import ShortcutsNavigation from '~/behaviors/shortcuts/shortcuts_navigation';
 import MiniPipelineGraph from '~/mini_pipeline_graph_dropdown';
 import initNotes from '~/init_notes';
 import initChangesDropdown from '~/init_changes_dropdown';
-import initDiffNotes from '~/diff_notes/diff_notes_bundle';
 import { fetchCommitMergeRequests } from '~/commit_merge_requests';
 import '~/sourcegraph/load';
+import { handleLocationHash } from '~/lib/utils/common_utils';
+import axios from '~/lib/utils/axios_utils';
+import syntaxHighlight from '~/syntax_highlight';
+import flash from '~/flash';
+import { __ } from '~/locale';
 
 document.addEventListener('DOMContentLoaded', () => {
   const hasPerfBar = document.querySelector('.with-performance-bar');
   const performanceHeight = hasPerfBar ? 35 : 0;
-  new Diff();
-  new ZenMode();
-  new ShortcutsNavigation();
-  new MiniPipelineGraph({
-    container: '.js-commit-pipeline-graph',
-  }).bindEvents();
-  initNotes();
-  initChangesDropdown(document.querySelector('.navbar-gitlab').offsetHeight + performanceHeight);
-  // eslint-disable-next-line no-jquery/no-load
-  $('.commit-info.branches').load(document.querySelector('.js-commit-box').dataset.commitPath);
-  fetchCommitMergeRequests();
-  initDiffNotes();
+  const filesContainer = $('.js-diffs-batch');
+  const initAfterPageLoad = () => {
+    new Diff();
+    new ZenMode();
+    new ShortcutsNavigation();
+    new MiniPipelineGraph({
+      container: '.js-commit-pipeline-graph',
+    }).bindEvents();
+    initNotes();
+    initChangesDropdown(document.querySelector('.navbar-gitlab').offsetHeight + performanceHeight);
+    // eslint-disable-next-line no-jquery/no-load
+    $('.commit-info.branches').load(document.querySelector('.js-commit-box').dataset.commitPath);
+    fetchCommitMergeRequests();
+  };
+
+  if (filesContainer.length) {
+    const batchPath = filesContainer.data('diffFilesPath');
+
+    axios
+      .get(batchPath)
+      .then(({ data }) => {
+        filesContainer.html($(data.html));
+        syntaxHighlight(filesContainer);
+        handleLocationHash();
+        initAfterPageLoad();
+      })
+      .catch(() => {
+        flash(__('An error occurred while retrieving diff files'));
+      });
+  } else {
+    initAfterPageLoad();
+  }
 });

app/controllers/projects/commit_controller.rb

@@ -15,8 +15,8 @@ class Projects::CommitController < Projects::ApplicationController
   before_action :authorize_download_code!
   before_action :authorize_read_pipeline!, only: [:pipelines]
   before_action :commit
-  before_action :define_commit_vars, only: [:show, :diff_for_path, :pipelines, :merge_requests]
-  before_action :define_note_vars, only: [:show, :diff_for_path]
+  before_action :define_commit_vars, only: [:show, :diff_for_path, :diff_files, :pipelines, :merge_requests]
+  before_action :define_note_vars, only: [:show, :diff_for_path, :diff_files]
   before_action :authorize_edit_tree!, only: [:revert, :cherry_pick]
 
   BRANCH_SEARCH_LIMIT = 1000
@@ -41,6 +41,10 @@ class Projects::CommitController < Projects::ApplicationController
     render_diff_for_path(@commit.diffs(diff_options))
   end
 
+  def diff_files
+    render json: { html: view_to_html_string('projects/commit/diff_files', diffs: @diffs, environment: @environment) }
+  end
+
   # rubocop: disable CodeReuse/ActiveRecord
   def pipelines
     @pipelines = @commit.pipelines.order(id: :desc)

app/views/projects/commit/diff_files.html.haml

+- diff_files = diffs.diff_files
+
+= render partial: 'projects/diffs/file', collection: diff_files, as: :diff_file, locals: { project: diffs.project, environment: environment, diff_page_context: 'is-commit' }

app/views/projects/diffs/_diffs.html.haml

@@ -3,6 +3,7 @@
 - can_create_note = !@diff_notes_disabled && can?(current_user, :create_note, diffs.project)
 - diff_files = diffs.diff_files
 - diff_page_context = local_assigns.fetch(:diff_page_context, nil)
+- load_diff_files_async = Feature.enabled?(:async_commit_diff_files, @project) && diff_page_context == "is-commit"
 
 .content-block.oneline-block.files-changed.diff-files-changed.js-diff-files-changed
   .files-changed-inner
@@ -26,5 +27,12 @@
 - if render_overflow_warning?(diffs)
   = render 'projects/diffs/warning', diff_files: diffs
 
+
 .files{ data: { can_create_note: can_create_note } }
-  = render partial: 'projects/diffs/file', collection: diff_files, as: :diff_file, locals: { project: diffs.project, environment: environment, diff_page_context: diff_page_context }
+  - if load_diff_files_async
+    - url = url_for(safe_params.merge(action: 'diff_files'))
+    .js-diffs-batch{ data: { diff_files_path: url } }
+      .text-center
+        %span.spinner.spinner-md
+  - else
+    = render partial: 'projects/diffs/file', collection: diff_files, as: :diff_file, locals: { project: diffs.project, environment: environment, diff_page_context: diff_page_context }

config/routes/repository.rb

@@ -17,6 +17,7 @@ resources :commit, only: [:show], constraints: { id: /\h{7,40}/ } do
     post :revert
     post :cherry_pick
     get :diff_for_path
+    get :diff_files
     get :merge_requests
   end
 end

locale/gitlab.pot

@@ -2786,6 +2786,9 @@ msgstr ""
 msgid "An error occurred while retrieving diff"
 msgstr ""
 
+msgid "An error occurred while retrieving diff files"
+msgstr ""
+
 msgid "An error occurred while saving LDAP override status. Please try again."
 msgstr ""
 

spec/features/markdown/copy_as_gfm_spec.rb

@@ -737,6 +737,7 @@ RSpec.describe 'Copy as GFM', :js do
       context 'inline diff' do
         before do
           visit project_commit_path(project, sample_commit.id, view: 'inline')
+          wait_for_requests
         end
 
         it_behaves_like 'copying code from a diff'
@@ -745,6 +746,7 @@ RSpec.describe 'Copy as GFM', :js do
       context 'parallel diff' do
         before do
           visit project_commit_path(project, sample_commit.id, view: 'parallel')
+          wait_for_requests
         end
 
         it_behaves_like 'copying code from a diff'

spec/features/projects/commit/diff_notes_spec.rb

@@ -8,30 +8,35 @@ RSpec.describe 'Commit diff', :js do
   let(:user)          { create(:user) }
   let(:project)       { create(:project, :public, :repository) }
 
-  before do
-    project.add_maintainer(user)
-    sign_in user
+  using RSpec::Parameterized::TableSyntax
+
+  where(:view, :async_diff_file_loading) do
+    'inline' | true
+    'inline' | false
+    'parallel' | true
+    'parallel' | false
   end
 
-  %w(inline parallel).each do |view|
-    context "#{view} view" do
-      before do
-        visit project_commit_path(project, sample_commit.id, view: view)
-      end
+  with_them do
+    before do
+      stub_feature_flags(async_commit_diff_files: async_diff_file_loading)
+      project.add_maintainer(user)
+      sign_in user
+      visit project_commit_path(project, sample_commit.id, view: view)
+    end
 
-      it "adds comment to diff" do
-        diff_line_num = first('.diff-line-num.new')
+    it "adds comment to diff" do
+      diff_line_num = first('.diff-line-num.new')
 
-        diff_line_num.hover
-        diff_line_num.find('.js-add-diff-note-button').click
+      diff_line_num.hover
+      diff_line_num.find('.js-add-diff-note-button').click
 
-        page.within(first('.diff-viewer')) do
-          find('.js-note-text').set 'test comment'
+      page.within(first('.diff-viewer')) do
+        find('.js-note-text').set 'test comment'
 
-          click_button 'Comment'
+        click_button 'Comment'
 
-          expect(page).to have_content('test comment')
-        end
+        expect(page).to have_content('test comment')
       end
     end
   end

spec/features/projects/commit/user_views_user_status_on_commit_spec.rb

@@ -30,7 +30,7 @@ RSpec.describe 'Project > Commit > View user status' do
     end
   end
 
-  describe 'status for a diff note on the commit' do
+  describe 'status for a diff note on the commit', :js do
     let(:note) { create(:diff_note_on_commit, project: project) }
 
     it_behaves_like 'showing user status' do

spec/features/projects/commits/user_browses_commits_spec.rb

@@ -41,7 +41,7 @@ RSpec.describe 'User browses commits' do
       .and have_selector('ul.breadcrumb a', count: 4)
   end
 
-  it 'renders diff links to both the previous and current image' do
+  it 'renders diff links to both the previous and current image', :js do
     visit project_commit_path(project, sample_image_commit.id)
 
     links = page.all('.file-actions a')

spec/views/projects/commit/show.html.haml_spec.rb

@@ -14,6 +14,12 @@ RSpec.describe 'projects/commit/show.html.haml' do
     assign(:notes, [])
     assign(:diffs, commit.diffs)
 
+    controller.params[:controller] = 'projects/commit'
+    controller.params[:action] = 'show'
+    controller.params[:namespace_id] = project.namespace.to_param
+    controller.params[:project_id] = project.to_param
+    controller.params[:id] = commit.id
+
     allow(view).to receive(:current_user).and_return(nil)
     allow(view).to receive(:can?).and_return(false)
     allow(view).to receive(:can_collaborate_with_project?).and_return(false)