Merge branch 'paginate-commit' into 'master'

Paginate the single commit view [RUN ALL RSPEC] [RUN AS-IF-FOSS]

See merge request gitlab-org/gitlab!52819

app/controllers/projects/commit_controller.rb

@@ -23,6 +23,7 @@ class Projects::CommitController < Projects::ApplicationController
   end
 
   BRANCH_SEARCH_LIMIT = 1000
+  COMMIT_DIFFS_PER_PAGE = 75
 
   feature_category :source_code_management
 

app/helpers/commits_helper.rb

@@ -126,6 +126,14 @@ module CommitsHelper
     %w(btn gpg-status-box) + Array(additional_classes)
   end
 
+  def conditionally_paginate_diff_files(diffs, paginate:, per: Projects::CommitController::COMMIT_DIFFS_PER_PAGE)
+    if paginate && Feature.enabled?(:paginate_commit_view, @project, type: :development)
+      Kaminari.paginate_array(diffs.diff_files.to_a).page(params[:page]).per(per)
+    else
+      diffs.diff_files
+    end
+  end
+
   protected
 
   # Private: Returns a link to a person. If the person has a matching user and

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

@@ -12,7 +12,7 @@
 .container-fluid{ class: [limited_container_width, container_class] }
   = render "commit_box"
   = render "ci_menu"
-  = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, diff_page_context: "is-commit"
+  = render "projects/diffs/diffs", diffs: @diffs, environment: @environment, diff_page_context: "is-commit", paginate_diffs: true
 
   .limited-width-notes
     = render "shared/notes/notes_with_form", :autocomplete => true

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

 - environment = local_assigns.fetch(:environment, nil)
 - show_whitespace_toggle = local_assigns.fetch(:show_whitespace_toggle, true)
 - 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"
+- paginate_diffs = local_assigns.fetch(:paginate_diffs, false) && !load_diff_files_async && Feature.enabled?(:paginate_commit_view, @project, type: :development)
+- diff_files = conditionally_paginate_diff_files(diffs, paginate: paginate_diffs)
 
 .content-block.oneline-block.files-changed.diff-files-changed.js-diff-files-changed
   .files-changed-inner
@@ -27,7 +28,6 @@
 - if render_overflow_warning?(diffs)
   = render 'projects/diffs/warning', diff_files: diffs
 
-
 .files{ data: { can_create_note: can_create_note } }
   - if load_diff_files_async
     - url = url_for(safe_params.merge(action: 'diff_files'))
@@ -36,3 +36,6 @@
         %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 }
+
+    - if paginate_diffs
+      = paginate(diff_files, theme: "gitlab")

changelogs/unreleased/paginate-commit.yml

+---
+title: Paginate single commit view
+merge_request: 52819
+author:
+type: performance

config/feature_flags/development/paginate_commit_view.yml

+---
+name: paginate_commit_view
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/52819
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/300540
+milestone: '13.9'
+type: development
+group: group::source code
+default_enabled: false

spec/features/commit_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Commit' do
+  let_it_be(:project) { create(:project, :repository) }
+  let_it_be(:user) { create(:user) }
+
+  describe "single commit view" do
+    let(:commit) do
+      project.repository.commits(nil, limit: 100).find do |commit|
+        commit.diffs.size > 1
+      end
+    end
+
+    let(:files) { commit.diffs.diff_files.to_a }
+
+    before do
+      stub_feature_flags(async_commit_diff_files: false)
+      project.add_maintainer(user)
+      sign_in(user)
+    end
+
+    describe "commit details" do
+      before do
+        visit project_commit_path(project, commit)
+      end
+
+      it "shows the short commit message" do
+        expect(page).to have_content(commit.title)
+      end
+
+      it "reports the correct number of total changes" do
+        expect(page).to have_content("Changes #{commit.diffs.size}")
+      end
+    end
+
+    context "pagination enabled" do
+      before do
+        stub_feature_flags(paginate_commit_view: true)
+        stub_const("Projects::CommitController::COMMIT_DIFFS_PER_PAGE", 1)
+
+        visit project_commit_path(project, commit)
+      end
+
+      it "shows an adjusted count for changed files on this page" do
+        expect(page).to have_content("Showing 1 changed file")
+      end
+
+      it "shows only the first diff on the first page" do
+        expect(page).to have_selector(".files ##{files[0].file_hash}")
+        expect(page).not_to have_selector(".files ##{files[1].file_hash}")
+      end
+
+      it "can navigate to the second page" do
+        within(".files .gl-pagination") do
+          click_on("2")
+        end
+
+        expect(page).not_to have_selector(".files ##{files[0].file_hash}")
+        expect(page).to have_selector(".files ##{files[1].file_hash}")
+      end
+    end
+
+    context "pagination disabled" do
+      before do
+        stub_feature_flags(paginate_commit_view: false)
+
+        visit project_commit_path(project, commit)
+      end
+
+      it "shows both diffs on the page" do
+        expect(page).to have_selector(".files ##{files[0].file_hash}")
+        expect(page).to have_selector(".files ##{files[1].file_hash}")
+      end
+    end
+  end
+end

spec/helpers/commits_helper_spec.rb

@@ -176,4 +176,77 @@ RSpec.describe CommitsHelper do
       expect(helper.commit_path(project, commit)).to eq(project_commit_path(project, commit))
     end
   end
+
+  describe "#conditionally_paginate_diff_files" do
+    let(:diffs_collection) { instance_double(Gitlab::Diff::FileCollection::Commit, diff_files: diff_files) }
+    let(:diff_files) { Gitlab::Git::DiffCollection.new(files) }
+    let(:page) { nil }
+
+    let(:files) do
+      Array.new(85).map do
+        { too_large: false, diff: "" }
+      end
+    end
+
+    let(:params) do
+      {
+        page: page
+      }
+    end
+
+    subject { helper.conditionally_paginate_diff_files(diffs_collection, paginate: paginate) }
+
+    before do
+      allow(helper).to receive(:params).and_return(params)
+    end
+
+    context "pagination is enabled" do
+      let(:paginate) { true }
+
+      it "has been paginated" do
+        expect(subject).to be_an(Array)
+      end
+
+      it "can change the number of items per page" do
+        commits = helper.conditionally_paginate_diff_files(diffs_collection, paginate: paginate, per: 10)
+
+        expect(commits).to be_an(Array)
+        expect(commits.size).to eq(10)
+      end
+
+      context "page 1" do
+        let(:page) { 1 }
+
+        it "has 20 diffs" do
+          expect(subject.size).to eq(75)
+        end
+      end
+
+      context "page 2" do
+        let(:page) { 2 }
+
+        it "has the remaining 10 diffs" do
+          expect(subject.size).to eq(10)
+        end
+      end
+    end
+
+    context "pagination is disabled" do
+      let(:paginate) { false }
+
+      it "returns a standard DiffCollection" do
+        expect(subject).to be_a(Gitlab::Git::DiffCollection)
+      end
+    end
+
+    context "feature flag is disabled" do
+      let(:paginate) { true }
+
+      it "returns a standard DiffCollection" do
+        stub_feature_flags(paginate_commit_view: false)
+
+        expect(subject).to be_a(Gitlab::Git::DiffCollection)
+      end
+    end
+  end
 end