Merge branch 'ph/224161/graduallyLoadDiffs' into 'master'

Gradually load more diffs as time goes on

See merge request gitlab-org/gitlab!48253

app/assets/javascripts/diffs/components/app.vue

@@ -539,6 +539,7 @@ export default {
                 <template #total>{{ diffFiles.length }}</template>
               </gl-sprintf>
             </div>
+            <gl-loading-icon v-else-if="retrievingBatches" size="lg" />
           </template>
           <no-changes v-else :changes-empty-state-illustration="changesEmptyStateIllustration" />
         </div>

app/assets/javascripts/diffs/store/actions.js

@@ -75,22 +75,33 @@ export const setBaseConfig = ({ commit }, options) => {
 };
 
 export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
+  const diffsGradualLoad = window.gon?.features?.diffsGradualLoad;
+  let perPage = DIFFS_PER_PAGE;
+  let increaseAmount = 1.4;
+
+  if (diffsGradualLoad) {
+    perPage = state.viewDiffsFileByFile ? 1 : 5;
+  }
+
+  const startPage = diffsGradualLoad ? 0 : 1;
   const id = window?.location?.hash;
   const isNoteLink = id.indexOf('#note') === 0;
   const urlParams = {
-    per_page: DIFFS_PER_PAGE,
     w: state.showWhitespace ? '0' : '1',
     view: 'inline',
   };
+  let totalLoaded = 0;
 
   commit(types.SET_BATCH_LOADING, true);
   commit(types.SET_RETRIEVING_BATCHES, true);
   eventHub.$emit(EVT_PERF_MARK_DIFF_FILES_START);
 
-  const getBatch = (page = 1) =>
+  const getBatch = (page = startPage) =>
     axios
-      .get(mergeUrlParams({ ...urlParams, page }, state.endpointBatch))
+      .get(mergeUrlParams({ ...urlParams, page, per_page: perPage }, state.endpointBatch))
       .then(({ data: { pagination, diff_files } }) => {
+        totalLoaded += diff_files.length;
+
         commit(types.SET_DIFF_DATA_BATCH, { diff_files });
         commit(types.SET_BATCH_LOADING, false);
 
@@ -102,7 +113,10 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
           dispatch('setCurrentDiffFileIdFromNote', id.split('_').pop());
         }
 
-        if (!pagination.next_page) {
+        if (
+          (diffsGradualLoad && totalLoaded === pagination.total_pages) ||
+          (!diffsGradualLoad && !pagination.next_page)
+        ) {
           commit(types.SET_RETRIEVING_BATCHES, false);
 
           // We need to check that the currentDiffFileId points to a file that exists
@@ -128,6 +142,16 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
               }),
             );
           }
+
+          return null;
+        }
+
+        if (diffsGradualLoad) {
+          const nextPage = page + perPage;
+          perPage = Math.min(Math.ceil(perPage * increaseAmount), 30);
+          increaseAmount = Math.min(increaseAmount + 0.2, 2);
+
+          return nextPage;
         }
 
         return pagination.next_page;

app/controllers/projects/merge_requests_controller.rb

@@ -42,6 +42,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
     push_frontend_feature_flag(:core_security_mr_widget_counts, @project)
     push_frontend_feature_flag(:remove_resolve_note, @project, default_enabled: true)
     push_frontend_feature_flag(:test_failure_history, @project)
+    push_frontend_feature_flag(:diffs_gradual_load, @project)
 
     record_experiment_user(:invite_members_version_a)
     record_experiment_user(:invite_members_version_b)

config/feature_flags/development/diffs_gradual_load.yml

+---
+name: diffs_gradual_load
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/48253/
+rollout_issue_url:
+milestone: '13.7'
+type: development
+group: group::code review
+default_enabled: false

lib/gitlab/diff/file_collection/merge_request_diff_batch.rb

@@ -11,7 +11,7 @@ module Gitlab
       #
       class MergeRequestDiffBatch < MergeRequestDiffBase
         DEFAULT_BATCH_PAGE = 1
-        DEFAULT_BATCH_SIZE = 20
+        DEFAULT_BATCH_SIZE = 30
 
         attr_reader :pagination_data
 
@@ -21,9 +21,9 @@ module Gitlab
           @paginated_collection = load_paginated_collection(batch_page, batch_size, diff_options)
 
           @pagination_data = {
-            current_page: @paginated_collection.current_page,
-            next_page: @paginated_collection.next_page,
-            total_pages: @paginated_collection.total_pages
+            current_page: batch_gradual_load? ? nil : @paginated_collection.current_page,
+            next_page: batch_gradual_load? ? nil : @paginated_collection.next_page,
+            total_pages: batch_gradual_load? ? relation.size : @paginated_collection.total_pages
           }
         end
 
@@ -62,17 +62,28 @@ module Gitlab
           @merge_request_diff.merge_request_diff_files
         end
 
+        # rubocop: disable CodeReuse/ActiveRecord
         def load_paginated_collection(batch_page, batch_size, diff_options)
           batch_page ||= DEFAULT_BATCH_PAGE
           batch_size ||= DEFAULT_BATCH_SIZE
 
           paths = diff_options&.fetch(:paths, nil)
 
-          paginated_collection = relation.page(batch_page).per(batch_size)
+          paginated_collection = if batch_gradual_load?
+                                   relation.offset(batch_page).limit([batch_size.to_i, DEFAULT_BATCH_SIZE].min)
+                                 else
+                                   relation.page(batch_page).per(batch_size)
+                                 end
+
           paginated_collection = paginated_collection.by_paths(paths) if paths
 
           paginated_collection
         end
+        # rubocop: enable CodeReuse/ActiveRecord
+
+        def batch_gradual_load?
+          Feature.enabled?(:diffs_gradual_load, @merge_request_diff.project)
+        end
       end
     end
   end

spec/controllers/projects/merge_requests/diffs_controller_spec.rb

@@ -74,6 +74,8 @@ RSpec.describe Projects::MergeRequests::DiffsController do
   let(:merge_request) { create(:merge_request_with_diffs, target_project: project, source_project: project) }
 
   before do
+    stub_feature_flags(diffs_gradual_load: false)
+
     project.add_maintainer(user)
     sign_in(user)
   end

spec/features/merge_request/user_sees_versions_spec.rb

@@ -17,6 +17,8 @@ RSpec.describe 'Merge request > User sees versions', :js do
   let!(:params) { {} }
 
   before do
+    stub_feature_flags(diffs_gradual_load: false)
+
     project.add_maintainer(user)
     sign_in(user)
     visit diffs_project_merge_request_path(project, merge_request, params)

spec/frontend/diffs/store/actions_spec.js

@@ -160,10 +160,10 @@ describe('DiffsStoreActions', () => {
         .onGet(
           mergeUrlParams(
             {
-              per_page: DIFFS_PER_PAGE,
               w: '1',
               view: 'inline',
               page: 1,
+              per_page: DIFFS_PER_PAGE,
             },
             endpointBatch,
           ),
@@ -172,10 +172,10 @@ describe('DiffsStoreActions', () => {
         .onGet(
           mergeUrlParams(
             {
-              per_page: DIFFS_PER_PAGE,
               w: '1',
               view: 'inline',
               page: 2,
+              per_page: DIFFS_PER_PAGE,
             },
             endpointBatch,
           ),

spec/lib/gitlab/diff/file_collection/merge_request_diff_batch_spec.rb

@@ -18,6 +18,10 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
 
   let(:diff_files) { subject.diff_files }
 
+  before do
+    stub_feature_flags(diffs_gradual_load: false)
+  end
+
   describe 'initialize' do
     it 'memoizes pagination_data' do
       expect(subject.pagination_data).to eq(current_page: 1, next_page: 2, total_pages: 2)
@@ -97,6 +101,18 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
         expect(collection.diff_files.map(&:new_path)).to eq(expected_batch_files)
       end
     end
+
+    context 'with diffs gradual load feature flag enabled' do
+      let(:batch_page) { 0 }
+
+      before do
+        stub_feature_flags(diffs_gradual_load: true)
+      end
+
+      it 'returns correct diff files' do
+        expect(subject.diffs.map(&:new_path)).to eq(diff_files_relation.page(1).per(batch_size).map(&:new_path))
+      end
+    end
   end
 
   it_behaves_like 'unfoldable diff' do

spec/models/merge_request_diff_spec.rb

@@ -9,6 +9,10 @@ RSpec.describe MergeRequestDiff do
 
   let(:diff_with_commits) { create(:merge_request).merge_request_diff }
 
+  before do
+    stub_feature_flags(diffs_gradual_load: false)
+  end
+
   describe 'validations' do
     subject { diff_with_commits }
 

spec/serializers/paginated_diff_entity_spec.rb

@@ -19,6 +19,10 @@ RSpec.describe PaginatedDiffEntity do
 
   subject { entity.as_json }
 
+  before do
+    stub_feature_flags(diffs_gradual_load: false)
+  end
+
   it 'exposes diff_files' do
     expect(subject[:diff_files]).to be_present
   end