From bdbd8eb664709ae905cd122c430d4f16b84a60c7 Mon Sep 17 00:00:00 2001 From: Artur Fedorov Date: Fri, 28 Jul 2023 16:22:37 +0200 Subject: [PATCH 1/2] This MR optimise request load for branch selector Branches selector now loads only chunks of branches 10 at a time and append additional items on scroll instead of increasing number of branches loaded at once Changelog: changed EE: true --- .../project_branch_selector.vue | 35 ++++++++++++++----- .../project_branch_selector_spec.js | 6 ++-- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue b/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue index beee6781807f5f..9f307c8e73f8ad 100644 --- a/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue +++ b/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue @@ -7,6 +7,8 @@ import { __ } from '~/locale'; import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; import { BRANCHES_PER_PAGE } from './constants'; +const branchListboxMapper = ({ name }) => ({ value: name, text: name }); + export default { i18n: { defaultText: __('Select branches'), @@ -56,7 +58,9 @@ export default { branches: [], loading: false, openedOnce: false, - limit: BRANCHES_PER_PAGE, + searchTerm: '', + searching: false, + page: 1, totalBranches: BRANCHES_PER_PAGE, }; }, @@ -67,6 +71,9 @@ export default { category() { return this.hasError ? 'secondary' : 'primary'; }, + itemsLoadedLength() { + return this.page * BRANCHES_PER_PAGE; + }, variant() { return this.hasError ? 'danger' : 'default'; }, @@ -88,19 +95,29 @@ export default { this.handleSearch = debounce(this.fetchBranches, DEFAULT_DEBOUNCE_AND_THROTTLE_MS); }, methods: { + search(searchTerm) { + this.searchTerm = searchTerm; + this.searching = true; + this.page = 1; + this.branches = []; + + this.handleSearch(searchTerm); + }, handleSelect(value) { this.$emit('select', value); }, - async fetchBranches(searchTerm) { + async fetchBranches() { this.loading = true; try { - const payload = await Api.branches(this.projectFullPath, searchTerm, { - per_page: this.limit, + const payload = await Api.branches(this.projectFullPath, this.searchTerm, { + per_page: BRANCHES_PER_PAGE, + page: this.page, }); const totalBranches = payload.headers['x-total']; - this.branches = payload.data.map(({ name }) => ({ value: name, text: name })); + const items = payload.data.map(branchListboxMapper); + this.branches = [...this.branches, ...items]; this.totalBranches = Number.parseInt(totalBranches, 10); this.$emit('error', { hasErrored: false }); } catch (error) { @@ -110,14 +127,15 @@ export default { } finally { this.loading = false; this.openedOnce = true; + this.searching = false; } }, onBottomReached() { - if (this.limit >= this.totalBranches) { + if (this.itemsLoadedLength >= this.totalBranches) { return; } - this.limit += BRANCHES_PER_PAGE; + this.page += 1; this.fetchBranches(); }, }, @@ -138,10 +156,11 @@ export default { :items="branches" :reset-button-label="$options.i18n.resetLabel" :toggle-text="toggleText" + :searching="searching" :selected="selected" :show-select-all-button-label="$options.i18n.selectAllLabel" @bottom-reached="onBottomReached" - @search="handleSearch" + @search="search" @select="handleSelect" @select-all="handleSelect(branchNames)" @shown.once="fetchBranches" diff --git a/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js b/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js index 94d3391d3e7c9a..acb2a753c4697b 100644 --- a/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js +++ b/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js @@ -16,7 +16,7 @@ const MOCKED_LISTBOX_ITEMS = TEST_BRANCHES.map(({ name }) => ({ value: name, })); -const TOTAL_BRANCHES = 30; +const TOTAL_BRANCHES = 15; describe('ProjectBranchSelector', () => { const PROJECT_ID = '1'; @@ -95,7 +95,7 @@ describe('ProjectBranchSelector', () => { describe('loading successfully', () => { beforeEach(() => { - mockAxios.onGet(MOCKED_BRANCHES_URL).reply(HTTP_STATUS_OK, TEST_BRANCHES, { + mockAxios.onGet(MOCKED_BRANCHES_URL).replyOnce(HTTP_STATUS_OK, TEST_BRANCHES, { 'x-total': TOTAL_BRANCHES, }); }); @@ -146,7 +146,7 @@ describe('ProjectBranchSelector', () => { findListBox().vm.$emit('bottom-reached'); await waitForPromises(); - expect(findListBox().props('items')).toHaveLength(TEST_BRANCHES.length); + expect(findListBox().props('items')).toHaveLength(0); }); it.each` -- GitLab From b3f69af7158d19dd7ca78dadef83114da0ab5463 Mon Sep 17 00:00:00 2001 From: Artur Fedorov Date: Tue, 1 Aug 2023 01:08:29 +0200 Subject: [PATCH 2/2] Applied review changes --- .../project_branch_selector.vue | 13 ++-- .../project_branch_selector_spec.js | 66 ++++++++++++++----- 2 files changed, 56 insertions(+), 23 deletions(-) diff --git a/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue b/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue index 9f307c8e73f8ad..4f5f878b41d55c 100644 --- a/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue +++ b/ee/app/assets/javascripts/vue_shared/components/branches_selector/project_branch_selector.vue @@ -92,16 +92,16 @@ export default { }, }, created() { - this.handleSearch = debounce(this.fetchBranches, DEFAULT_DEBOUNCE_AND_THROTTLE_MS); + this.search = debounce(this.fetchBranches, DEFAULT_DEBOUNCE_AND_THROTTLE_MS); }, methods: { - search(searchTerm) { + handleSearch(searchTerm) { this.searchTerm = searchTerm; this.searching = true; this.page = 1; this.branches = []; - this.handleSearch(searchTerm); + this.search(searchTerm); }, handleSelect(value) { this.$emit('select', value); @@ -117,11 +117,12 @@ export default { const totalBranches = payload.headers['x-total']; const items = payload.data.map(branchListboxMapper); + this.branches = [...this.branches, ...items]; this.totalBranches = Number.parseInt(totalBranches, 10); - this.$emit('error', { hasErrored: false }); + this.$emit('success'); } catch (error) { - this.$emit('error', { hasErrored: true, error: this.customErrorMessage }); + this.$emit('error', { error: this.customErrorMessage }); this.branches = []; Sentry.captureException(error); } finally { @@ -160,7 +161,7 @@ export default { :selected="selected" :show-select-all-button-label="$options.i18n.selectAllLabel" @bottom-reached="onBottomReached" - @search="search" + @search="handleSearch" @select="handleSelect" @select-all="handleSelect(branchNames)" @shown.once="fetchBranches" diff --git a/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js b/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js index acb2a753c4697b..410e965aec4d45 100644 --- a/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js +++ b/ee/spec/frontend/vue_shared/components/branches_selector/project_branch_selector_spec.js @@ -8,15 +8,18 @@ import ProjectBranchSelector from 'ee/vue_shared/components/branches_selector/pr import axios from '~/lib/utils/axios_utils'; import { HTTP_STATUS_BAD_REQUEST, HTTP_STATUS_OK } from '~/lib/utils/http_status'; -const branches = Array.from({ length: 15 }, (_, index) => ({ id: index, name: `test-${index}` })); +const branches = Array.from({ length: 30 }, (_, index) => ({ id: index, name: `test-${index}` })); const TEST_BRANCHES = [{ id: 16, name: 'main' }, ...branches]; +const TEST_BRANCHES_PAGE_ONE = TEST_BRANCHES.slice(0, 15); +const TEST_BRANCHES_PAGE_TWO = TEST_BRANCHES.slice(15); + const MOCKED_LISTBOX_ITEMS = TEST_BRANCHES.map(({ name }) => ({ text: name, value: name, })); -const TOTAL_BRANCHES = 15; +const TOTAL_BRANCHES = 31; describe('ProjectBranchSelector', () => { const PROJECT_ID = '1'; @@ -95,7 +98,7 @@ describe('ProjectBranchSelector', () => { describe('loading successfully', () => { beforeEach(() => { - mockAxios.onGet(MOCKED_BRANCHES_URL).replyOnce(HTTP_STATUS_OK, TEST_BRANCHES, { + mockAxios.onGet(MOCKED_BRANCHES_URL).reply(HTTP_STATUS_OK, TEST_BRANCHES, { 'x-total': TOTAL_BRANCHES, }); }); @@ -108,7 +111,7 @@ describe('ProjectBranchSelector', () => { expect(findListBox().props('items')).toEqual(MOCKED_LISTBOX_ITEMS); expect(findAllListboxItems()).toHaveLength(MOCKED_LISTBOX_ITEMS.length); - expect(wrapper.emitted('error')).toEqual([[{ hasErrored: false }]]); + expect(wrapper.emitted('success')).toHaveLength(1); expect(findListBox().props('variant')).toEqual('default'); expect(findListBox().props('category')).toEqual('primary'); }); @@ -137,18 +140,6 @@ describe('ProjectBranchSelector', () => { expect(wrapper.emitted('select')).toEqual([[[]]]); }); - it('should stop fetching branches when limit is reached', async () => { - createComponent(); - await openDropdown(); - - expect(findListBox().props('items')).toHaveLength(TEST_BRANCHES.length); - - findListBox().vm.$emit('bottom-reached'); - await waitForPromises(); - - expect(findListBox().props('items')).toHaveLength(0); - }); - it.each` selected | expectedSelected | expectedResult ${['main', 'development']} | ${['main', 'development']} | ${['main', 'development'].join(', ')} @@ -170,6 +161,48 @@ describe('ProjectBranchSelector', () => { ); }); + describe('infinite scroll', () => { + it('should stop fetching branches when limit is reached', async () => { + mockAxios.onGet(MOCKED_BRANCHES_URL).replyOnce(HTTP_STATUS_OK, TEST_BRANCHES_PAGE_ONE, { + 'x-total': TOTAL_BRANCHES, + }); + + createComponent(); + await openDropdown(); + + expect(findListBox().props('items')).toHaveLength(TEST_BRANCHES_PAGE_ONE.length); + + mockAxios.onGet(MOCKED_BRANCHES_URL).replyOnce(HTTP_STATUS_OK, TEST_BRANCHES_PAGE_TWO, { + 'x-total': TOTAL_BRANCHES, + }); + + findListBox().vm.$emit('bottom-reached'); + await waitForPromises(); + + expect(findListBox().props('items')).toHaveLength(TEST_BRANCHES.length); + }); + + it('should reset branches when searched', async () => { + mockAxios.onGet(MOCKED_BRANCHES_URL).replyOnce(HTTP_STATUS_OK, TEST_BRANCHES_PAGE_ONE, { + 'x-total': TOTAL_BRANCHES, + }); + + createComponent(); + await openDropdown(); + + expect(findListBox().props('items')).toHaveLength(TEST_BRANCHES_PAGE_ONE.length); + + mockAxios.onGet(MOCKED_BRANCHES_URL).replyOnce(HTTP_STATUS_OK, [TEST_BRANCHES_PAGE_ONE[0]], { + 'x-total': TOTAL_BRANCHES, + }); + + findListBox().vm.$emit('search', 'main'); + await waitForPromises(); + + expect(findListBox().props('items')).toHaveLength(1); + }); + }); + describe('has no protected branches', () => { beforeEach(() => { mockAxios.onGet(MOCKED_BRANCHES_URL).replyOnce(HTTP_STATUS_OK, []); @@ -223,7 +256,6 @@ describe('ProjectBranchSelector', () => { [ { error: expectedError, - hasErrored: true, }, ], ]); -- GitLab