Commit c80bd1f0 authored by Phil Hughes's avatar Phil Hughes

Gradually load more diffs as time goes on

To increase loading performance of diffs, we will now load
a small amount at first and then as time goes on load more per
request.

If the user has file-by-file enabled, we will load
1 diff first and then gradually load more as time goes on.

Closes https://gitlab.com/gitlab-org/gitlab/-/issues/224161
parent 1e11c7fc
...@@ -518,6 +518,7 @@ export default { ...@@ -518,6 +518,7 @@ export default {
<template #total>{{ diffFiles.length }}</template> <template #total>{{ diffFiles.length }}</template>
</gl-sprintf> </gl-sprintf>
</div> </div>
<gl-loading-icon v-else-if="retrievingBatches" size="lg" />
</template> </template>
<no-changes v-else :changes-empty-state-illustration="changesEmptyStateIllustration" /> <no-changes v-else :changes-empty-state-illustration="changesEmptyStateIllustration" />
</div> </div>
......
...@@ -57,6 +57,7 @@ export const setBaseConfig = ({ commit }, options) => { ...@@ -57,6 +57,7 @@ export const setBaseConfig = ({ commit }, options) => {
projectPath, projectPath,
dismissEndpoint, dismissEndpoint,
showSuggestPopover, showSuggestPopover,
viewDiffsFileByFile,
} = options; } = options;
commit(types.SET_BASE_CONFIG, { commit(types.SET_BASE_CONFIG, {
endpoint, endpoint,
...@@ -66,26 +67,38 @@ export const setBaseConfig = ({ commit }, options) => { ...@@ -66,26 +67,38 @@ export const setBaseConfig = ({ commit }, options) => {
projectPath, projectPath,
dismissEndpoint, dismissEndpoint,
showSuggestPopover, showSuggestPopover,
viewDiffsFileByFile,
}); });
}; };
export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => { 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 id = window?.location?.hash;
const isNoteLink = id.indexOf('#note') === 0; const isNoteLink = id.indexOf('#note') === 0;
const urlParams = { const urlParams = {
per_page: DIFFS_PER_PAGE,
w: state.showWhitespace ? '0' : '1', w: state.showWhitespace ? '0' : '1',
view: 'inline', view: 'inline',
}; };
let totalLoaded = 0;
commit(types.SET_BATCH_LOADING, true); commit(types.SET_BATCH_LOADING, true);
commit(types.SET_RETRIEVING_BATCHES, true); commit(types.SET_RETRIEVING_BATCHES, true);
eventHub.$emit(EVT_PERF_MARK_DIFF_FILES_START); eventHub.$emit(EVT_PERF_MARK_DIFF_FILES_START);
const getBatch = (page = 1) => const getBatch = (page = startPage) =>
axios axios
.get(mergeUrlParams({ ...urlParams, page }, state.endpointBatch)) .get(mergeUrlParams({ ...urlParams, page, per_page: perPage }, state.endpointBatch))
.then(({ data: { pagination, diff_files } }) => { .then(({ data: { pagination, diff_files } }) => {
totalLoaded += diff_files.length;
commit(types.SET_DIFF_DATA_BATCH, { diff_files }); commit(types.SET_DIFF_DATA_BATCH, { diff_files });
commit(types.SET_BATCH_LOADING, false); commit(types.SET_BATCH_LOADING, false);
...@@ -97,7 +110,10 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => { ...@@ -97,7 +110,10 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
dispatch('setCurrentDiffFileIdFromNote', id.split('_').pop()); 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); commit(types.SET_RETRIEVING_BATCHES, false);
// We need to check that the currentDiffFileId points to a file that exists // We need to check that the currentDiffFileId points to a file that exists
...@@ -123,6 +139,16 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => { ...@@ -123,6 +139,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; return pagination.next_page;
......
...@@ -36,6 +36,7 @@ export default { ...@@ -36,6 +36,7 @@ export default {
projectPath, projectPath,
dismissEndpoint, dismissEndpoint,
showSuggestPopover, showSuggestPopover,
viewDiffsFileByFile,
} = options; } = options;
Object.assign(state, { Object.assign(state, {
endpoint, endpoint,
...@@ -45,6 +46,7 @@ export default { ...@@ -45,6 +46,7 @@ export default {
projectPath, projectPath,
dismissEndpoint, dismissEndpoint,
showSuggestPopover, showSuggestPopover,
viewDiffsFileByFile,
}); });
}, },
......
...@@ -41,6 +41,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo ...@@ -41,6 +41,7 @@ class Projects::MergeRequestsController < Projects::MergeRequests::ApplicationCo
push_frontend_feature_flag(:core_security_mr_widget, @project, default_enabled: true) push_frontend_feature_flag(:core_security_mr_widget, @project, default_enabled: true)
push_frontend_feature_flag(:remove_resolve_note, @project, default_enabled: true) push_frontend_feature_flag(:remove_resolve_note, @project, default_enabled: true)
push_frontend_feature_flag(:test_failure_history, @project) 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_a)
record_experiment_user(:invite_members_version_b) record_experiment_user(:invite_members_version_b)
......
---
name: diffs_gradual_load
introduced_by_url:
rollout_issue_url:
milestone: '13.7'
type: development
group: group::code review
default_enabled: false
...@@ -21,9 +21,9 @@ module Gitlab ...@@ -21,9 +21,9 @@ module Gitlab
@paginated_collection = load_paginated_collection(batch_page, batch_size, diff_options) @paginated_collection = load_paginated_collection(batch_page, batch_size, diff_options)
@pagination_data = { @pagination_data = {
current_page: @paginated_collection.current_page, current_page: batch_gradual_load? ? nil : @paginated_collection.current_page,
next_page: @paginated_collection.next_page, next_page: batch_gradual_load? ? nil : @paginated_collection.next_page,
total_pages: @paginated_collection.total_pages total_pages: batch_gradual_load? ? relation.size : @paginated_collection.total_pages
} }
end end
...@@ -68,11 +68,19 @@ module Gitlab ...@@ -68,11 +68,19 @@ module Gitlab
paths = diff_options&.fetch(:paths, nil) 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)
else
relation.page(batch_page).per(batch_size)
end
paginated_collection = paginated_collection.by_paths(paths) if paths paginated_collection = paginated_collection.by_paths(paths) if paths
paginated_collection paginated_collection
end end
def batch_gradual_load?
Feature.enabled?(:diffs_gradual_load, @merge_request_diff.project)
end
end end
end end
end end
......
...@@ -18,6 +18,10 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do ...@@ -18,6 +18,10 @@ RSpec.describe Gitlab::Diff::FileCollection::MergeRequestDiffBatch do
let(:diff_files) { subject.diff_files } let(:diff_files) { subject.diff_files }
before do
stub_feature_flags(diffs_gradual_load: false)
end
describe 'initialize' do describe 'initialize' do
it 'memoizes pagination_data' do it 'memoizes pagination_data' do
expect(subject.pagination_data).to eq(current_page: 1, next_page: 2, total_pages: 2) expect(subject.pagination_data).to eq(current_page: 1, next_page: 2, total_pages: 2)
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment