From f6288c2e09220b654f2db1d2e1dc41800b7cedb9 Mon Sep 17 00:00:00 2001 From: Dave Pisek Date: Tue, 2 Dec 2025 15:29:24 +0100 Subject: [PATCH 1/6] Vulnerability Report: Add batch loading The query that is used to load the main data for the vulnerability report is very high in complexity and at the limit of the threshold. This is mainly due to the fact that we support a maximum of 100 results per page, which is a big complexity multiplier. This change introduces a batch-loading, which kicks in when the selected page size is larger than 50. It will then fire off two queries, and then show the first result as soon as it is available. --- .eslint_todo/vue-require-name-property.mjs | 1 - .../vulnerability_list.vue | 15 +- .../vulnerability_list_graphql.vue | 164 ++++++++++------ .../vulnerability_list_graphql_spec.js | 179 ++++++++++++++---- .../vulnerability_list_spec.js | 30 +++ locale/gitlab.pot | 3 + 6 files changed, 301 insertions(+), 91 deletions(-) diff --git a/.eslint_todo/vue-require-name-property.mjs b/.eslint_todo/vue-require-name-property.mjs index ea735e87929d71..dc9a2f03a8d4b5 100644 --- a/.eslint_todo/vue-require-name-property.mjs +++ b/.eslint_todo/vue-require-name-property.mjs @@ -1944,7 +1944,6 @@ export default { 'ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_counts.vue', 'ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_filters.vue', 'ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue', - 'ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue', 'ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_status.vue', 'ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_report_header.vue', 'ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_report_tab.vue', diff --git a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue index e76b02bc7eebd8..34d759633ed6aa 100644 --- a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue +++ b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue @@ -128,6 +128,11 @@ export default { sortDesc: true, }), }, + loadingMoreCount: { + type: Number, + required: false, + default: 0, + }, }, data() { return { @@ -607,7 +612,13 @@ export default { - - +
+ +
+ + diff --git a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue index 8fe77102f4d614..2e79feb8bed526 100644 --- a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue +++ b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue @@ -1,6 +1,7 @@ @@ -250,12 +304,12 @@ export default { :page-size="pageSize" :should-show-project-namespace="showProjectNamespace" :portal-name="portalName" + :loading-more-count="secondBatchLoadingCount" @vulnerability-clicked="$emit('vulnerability-clicked', $event)" /> -
diff --git a/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql_spec.js b/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql_spec.js index 43716aca5bfa8e..d86a3cb0c8d5b5 100644 --- a/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql_spec.js +++ b/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql_spec.js @@ -5,9 +5,9 @@ import VueRouter from 'vue-router'; import { shallowMount } from '@vue/test-utils'; import VulnerabilityListGraphql, { PAGE_SIZE_STORAGE_KEY, + MAX_GRAPHQL_REQUEST_SIZE, } from 'ee/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue'; import VulnerabilityList from 'ee/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue'; -import { isPolicyViolationFilterEnabled } from 'ee/security_dashboard/utils'; import PageSizeSelector from '~/vue_shared/components/page_size_selector.vue'; import { DEFAULT_PAGE_SIZE } from '~/vue_shared/issuable/list/constants'; import groupVulnerabilitiesQuery from 'ee/security_dashboard/graphql/queries/group_vulnerabilities.query.graphql'; @@ -20,7 +20,6 @@ import LocalStorageSync from '~/vue_shared/components/local_storage_sync.vue'; import { vulnerabilities } from '../../mock_data'; jest.mock('~/alert'); -jest.mock('ee/security_dashboard/utils'); Vue.use(VueApollo); Vue.use(VueRouter); @@ -443,45 +442,161 @@ describe('Vulnerability list GraphQL component', () => { expect(findLocalStorageSync().props('value')).toBe(pageSize); }); + }); - it('is shown when loaded and there are no vulnerabilities', async () => { - createWrapper({ vulnerabilitiesHandler: createVulnerabilitiesRequestHandler({}, []) }); + describe('sequential batch loading', () => { + const generateVulnerabilities = (count) => + Array.from({ length: count }, (_, i) => ({ + ...vulnerabilities[0], + id: `id_${i}`, + project: { id: `project-${i}`, nameWithNamespace: 'Test / Project' }, + })); + + const createResponse = (nodes, pageInfo = {}) => ({ + data: { + group: { + id: 'group-1', + __typename: 'Group', + vulnerabilities: { + nodes, + pageInfo: { + __typename: 'PageInfo', + startCursor: 'start', + endCursor: 'end', + hasNextPage: false, + hasPreviousPage: false, + ...pageInfo, + }, + }, + }, + }, + }); + + it('when page size is less than or equal to MAX_GRAPHQL_REQUEST_SIZE, does not batch', async () => { + createWrapper(); await waitForPromises(); - expect(findPageSizeSelector().exists()).toBe(true); + expect(vulnerabilitiesRequestHandler).toHaveBeenCalledTimes(1); }); - }); - describe('limit large page size', () => { - const scenarios = [ - { - description: 'when aiExperimentSastFpDetection is enabled', - setup: () => ({ aiExperimentSastFpDetection: true }), - }, - { - description: 'when isPolicyViolationFilterEnabled() is true', - setup: () => { - isPolicyViolationFilterEnabled.mockReturnValue(true); - return {}; - }, - }, - { - description: 'when autoDismissVulnerabilityPolicies is enabled', - setup: () => ({ autoDismissVulnerabilityPolicies: true }), - }, - ]; + describe('when page size is greater than MAX_GRAPHQL_REQUEST_SIZE', () => { + const BATCH_PAGE_SIZE = 100; - describe.each(scenarios)('$description', ({ setup }) => { - it('sets excludeLargePageSize on page size selector', () => { - createWrapper(setup()); - expect(findPageSizeSelector().props('excludeLargePageSize')).toBe(true); + beforeEach(() => { + localStorage.setItem(PAGE_SIZE_STORAGE_KEY, BATCH_PAGE_SIZE); }); - it('changes page size from 100 to 50', () => { - localStorage.setItem(PAGE_SIZE_STORAGE_KEY, 100); - createWrapper(setup()); + it('uses MAX_GRAPHQL_REQUEST_SIZE for first request when page size exceeds it', async () => { + const handler = jest.fn().mockResolvedValue( + createResponse(generateVulnerabilities(MAX_GRAPHQL_REQUEST_SIZE), { + endCursor: 'end-1', + hasNextPage: true, + }), + ); + createWrapper({ vulnerabilitiesHandler: handler }); + await waitForPromises(); + + expect(handler).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ first: MAX_GRAPHQL_REQUEST_SIZE }), + ); + }); + + it('fetches second batch after first batch loads when batching is enabled', async () => { + const handler = jest + .fn() + .mockResolvedValueOnce( + createResponse(generateVulnerabilities(MAX_GRAPHQL_REQUEST_SIZE), { + endCursor: 'end-1', + hasNextPage: true, + }), + ) + .mockResolvedValueOnce( + createResponse(generateVulnerabilities(MAX_GRAPHQL_REQUEST_SIZE), { + hasNextPage: false, + }), + ); + createWrapper({ vulnerabilitiesHandler: handler }); + await waitForPromises(); + + expect(handler).toHaveBeenCalledTimes(2); + expect(handler).toHaveBeenNthCalledWith( + 2, + expect.objectContaining({ after: 'end-1', first: MAX_GRAPHQL_REQUEST_SIZE }), + ); + }); - expect(findPageSizeSelector().props('value')).toBe(50); + it('does not fetch second batch when first batch has fewer items than MAX_GRAPHQL_REQUEST_SIZE', async () => { + const handler = jest + .fn() + .mockResolvedValue(createResponse(generateVulnerabilities(30), { hasNextPage: false })); + createWrapper({ vulnerabilitiesHandler: handler }); + await waitForPromises(); + + expect(handler).toHaveBeenCalledTimes(1); + }); + + it('passes loadingMoreCount to VulnerabilityList while loading second batch', async () => { + let resolveSecondBatch; + const handler = jest + .fn() + .mockResolvedValueOnce( + createResponse(generateVulnerabilities(MAX_GRAPHQL_REQUEST_SIZE), { + endCursor: 'end-1', + hasNextPage: true, + }), + ) + .mockReturnValueOnce( + new Promise((resolve) => { + resolveSecondBatch = resolve; + }), + ); + createWrapper({ vulnerabilitiesHandler: handler }); + await waitForPromises(); + + expect(findVulnerabilityList().props('loadingMoreCount')).toBe( + BATCH_PAGE_SIZE - MAX_GRAPHQL_REQUEST_SIZE, + ); + + resolveSecondBatch( + createResponse(generateVulnerabilities(MAX_GRAPHQL_REQUEST_SIZE), { hasNextPage: false }), + ); + await waitForPromises(); + + expect(findVulnerabilityList().props('loadingMoreCount')).toBe(0); + }); + + it('merges both batches into vulnerabilities', async () => { + const handler = jest + .fn() + .mockResolvedValueOnce( + createResponse(generateVulnerabilities(MAX_GRAPHQL_REQUEST_SIZE), { + endCursor: 'end-1', + hasNextPage: true, + }), + ) + .mockResolvedValueOnce( + createResponse(generateVulnerabilities(MAX_GRAPHQL_REQUEST_SIZE), { + hasNextPage: false, + }), + ); + createWrapper({ vulnerabilitiesHandler: handler }); + await waitForPromises(); + + expect(findVulnerabilityList().props('vulnerabilities')).toHaveLength( + MAX_GRAPHQL_REQUEST_SIZE * 2, + ); + }); + + it('does not batch when using backward pagination (before cursor)', async () => { + await router.push({ query: { before: 'some-cursor' } }); + createWrapper(); + await waitForPromises(); + + expect(vulnerabilitiesRequestHandler).toHaveBeenCalledWith( + expect.objectContaining({ last: BATCH_PAGE_SIZE, first: null }), + ); + expect(vulnerabilitiesRequestHandler).toHaveBeenCalledTimes(1); }); }); }); diff --git a/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_spec.js b/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_spec.js index e100def6475cea..2d93046144fabc 100644 --- a/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_spec.js +++ b/ee/spec/frontend/security_dashboard/components/shared/vulnerability_report/vulnerability_list_spec.js @@ -637,6 +637,36 @@ describe('Vulnerability list component', () => { }); }); + describe('loadingMoreCount prop', () => { + const findLoadingMoreSkeleton = () => wrapper.findByTestId('loading-more-skeleton'); + + it('does not show loading more skeletons when loadingMoreCount is `0`', () => { + createWrapper({ props: { vulnerabilities, loadingMoreCount: 0 } }); + + expect(findLoadingMoreSkeleton().exists()).toBe(false); + }); + + it('shows skeleton loaders when loadingMoreCount is greater than `0`', () => { + const loadingMoreCount = 50; + createWrapper({ props: { vulnerabilities, loadingMoreCount } }); + + expect(findLoadingMoreSkeleton().exists()).toBe(true); + expect(findLoadingMoreSkeleton().findAllComponents(GlSkeletonLoader)).toHaveLength( + loadingMoreCount, + ); + }); + + it('shows vulnerabilities and loading skeletons together', () => { + const loadingMoreCount = 25; + createWrapper({ props: { vulnerabilities, loadingMoreCount } }); + + expect(findAllStatusCells()).toHaveLength(vulnerabilities.length); + expect(findLoadingMoreSkeleton().findAllComponents(GlSkeletonLoader)).toHaveLength( + loadingMoreCount, + ); + }); + }); + describe('operational vulnerabilities', () => { beforeEach(() => { createWrapper({ diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 9d1307399db059..ec9035189e621a 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -62592,6 +62592,9 @@ msgstr "" msgid "SecurityReports|Enter at least 3 characters to view available identifiers." msgstr "" +msgid "SecurityReports|Error fetching additional vulnerabilities." +msgstr "" + msgid "SecurityReports|Error fetching the vulnerability counts. Please check your network connection and try again." msgstr "" -- GitLab From 8173b57632a903b01b3686656f9f4b76ec1bb3d7 Mon Sep 17 00:00:00 2001 From: Dave Pisek Date: Mon, 15 Dec 2025 10:08:44 +0100 Subject: [PATCH 2/6] WIP: revert to get/set --- .../vulnerability_list_graphql.vue | 44 ++++++++++++------- 1 file changed, 28 insertions(+), 16 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue index 2e79feb8bed526..94293f5f5d32db 100644 --- a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue +++ b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue @@ -127,13 +127,31 @@ export default { }, }, computed: { - before() { - // The GraphQL query can only have a "before" or an "after" but not both, so if both are in - // the querystring, we'll only use "after" and pretend the "before" doesn't exist. - return this.after ? undefined : this.$route.query.before; + before: { + get() { + // The GraphQL query can only have a "before" or an "after" but not both, so if both are in + // the querystring, we'll only use "after" and pretend the "before" doesn't exist. + return this.after ? undefined : this.$route.query.before; + }, + set(before) { + // Check if before will change, otherwise Vue Router will throw a "you're navigating to the + // same route" error. + if (before !== this.before) { + this.pushQuerystring({ before, after: undefined }); + } + }, }, - after() { - return this.$route.query.after; + after: { + get() { + return this.$route.query.after; + }, + set(after) { + // Check if after will change, otherwise Vue Router will throw a "you're navigating to the + // same route" error. + if (after !== this.after) { + this.pushQuerystring({ before: undefined, after }); + } + }, }, sort: { get() { @@ -226,20 +244,14 @@ export default { }, methods: { goToNextPage() { - this.updateCursors({ after: this.pageInfo.endCursor || this.before }); + this.after = this.pageInfo.endCursor || this.before; }, goToPrevPage() { - this.updateCursors({ before: this.pageInfo.startCursor || this.after }); + this.before = this.pageInfo.startCursor || this.after; }, resetPaging() { - this.updateCursors({ before: undefined, after: undefined }); - }, - updateCursors({ before, after }) { - // Avoid Vue Router "navigating to the same route" error. - if (before === this.before && after === this.after) { - return; - } - this.pushQuerystring({ before, after }); + this.before = undefined; + this.after = undefined; }, pushQuerystring(data) { this.$router.push({ query: { ...this.$route.query, ...data } }); -- GitLab From b41150a44fb0b10336ff007c254f8bc081b260d2 Mon Sep 17 00:00:00 2001 From: Dave Pisek Date: Mon, 15 Dec 2025 10:32:13 +0100 Subject: [PATCH 3/6] WIP: add back navigation --- .../vulnerability_list_graphql.vue | 57 +++++++++++-------- 1 file changed, 33 insertions(+), 24 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue index 94293f5f5d32db..e72b8ace73e951 100644 --- a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue +++ b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list_graphql.vue @@ -104,11 +104,17 @@ export default { }, result({ data, loading }) { if (!loading && data && this.shouldBatch && !this.secondBatchFetched) { - const vulnerabilities = this.getVulnerabilitiesFromData(data); - const firstBatchSize = vulnerabilities.nodes.length; + const { nodes, pageInfo } = this.getVulnerabilitiesFromData(data); - if (firstBatchSize === MAX_GRAPHQL_REQUEST_SIZE && vulnerabilities.pageInfo.hasNextPage) { - this.fetchSecondBatch(vulnerabilities.pageInfo.endCursor); + if (nodes.length === MAX_GRAPHQL_REQUEST_SIZE) { + const cursor = this.isForwardPagination ? pageInfo.endCursor : pageInfo.startCursor; + const hasMoreItems = this.isForwardPagination + ? pageInfo.hasNextPage + : pageInfo.hasPreviousPage; + + if (hasMoreItems) { + this.fetchSecondBatch(cursor); + } } } }, @@ -169,7 +175,10 @@ export default { return this.$apollo.queries.vulnerabilities.loading && !this.isLoadingSecondBatch; }, shouldBatch() { - return this.pageSize > MAX_GRAPHQL_REQUEST_SIZE && !this.before; + return this.pageSize > MAX_GRAPHQL_REQUEST_SIZE; + }, + isForwardPagination() { + return !this.before; }, requestSize() { return this.shouldBatch ? MAX_GRAPHQL_REQUEST_SIZE : this.pageSize; @@ -187,7 +196,7 @@ export default { // use "last". See this comment for more info: // https://gitlab.com/gitlab-org/gitlab/-/merge_requests/79834#note_831878506 first: this.before ? null : this.requestSize, - last: this.before ? this.pageSize : null, + last: this.before ? this.requestSize : null, before: this.before, after: this.after, ...this.filters, @@ -259,7 +268,7 @@ export default { getVulnerabilitiesFromData(data) { return get(data, GRAPHQL_DATA_PATH[this.dashboardType]); }, - async fetchSecondBatch(afterCursor) { + async fetchSecondBatch(cursor) { this.isLoadingSecondBatch = true; this.secondBatchFetched = true; @@ -267,13 +276,13 @@ export default { await this.$apollo.queries.vulnerabilities.fetchMore({ variables: { ...this.queryVariables, - after: afterCursor, - first: MAX_GRAPHQL_REQUEST_SIZE, - before: null, - last: null, + after: this.isForwardPagination ? cursor : null, + before: this.isForwardPagination ? null : cursor, + first: this.isForwardPagination ? MAX_GRAPHQL_REQUEST_SIZE : null, + last: this.isForwardPagination ? null : MAX_GRAPHQL_REQUEST_SIZE, }, - updateQuery: (previousResult, { fetchMoreResult }) => { - return this.mergeResults(previousResult, fetchMoreResult); + updateQuery: (firstBatchData, { fetchMoreResult: secondBatchData }) => { + return this.mergeResults(firstBatchData, secondBatchData); }, }); } catch { @@ -284,20 +293,20 @@ export default { this.isLoadingSecondBatch = false; } }, - mergeResults(previousResult, fetchMoreResult) { + mergeResults(firstBatchData, secondBatchData) { const path = GRAPHQL_DATA_PATH[this.dashboardType]; - return produce(fetchMoreResult, (draftData) => { - const prevVulnerabilities = get(previousResult, path); - const currentVulnerabilities = get(draftData, path); - - // Prepend the previous nodes to the current nodes - currentVulnerabilities.nodes.unshift(...prevVulnerabilities.nodes); + return produce(secondBatchData, (draftData) => { + const firstBatchVulnerabilities = get(firstBatchData, path); + const secondBatchVulnerabilities = get(draftData, path); - // Keep the startCursor from the first batch for backward pagination - currentVulnerabilities.pageInfo.startCursor = prevVulnerabilities.pageInfo.startCursor; - currentVulnerabilities.pageInfo.hasPreviousPage = - prevVulnerabilities.pageInfo.hasPreviousPage; + return { + nodes: [...firstBatchVulnerabilities.nodes, ...secondBatchVulnerabilities.nodes], + pageInfo: { + ...firstBatchVulnerabilities.pageInfo, + ...secondBatchVulnerabilities.pageInfo, + }, + }; }); }, }, -- GitLab From 483c6e4a6fe485015c9dcb1826954b289cfe0701 Mon Sep 17 00:00:00 2001 From: Dave Pisek Date: Mon, 15 Dec 2025 10:48:57 +0100 Subject: [PATCH 4/6] WIP: fix pagination --- .../vulnerability_list.vue | 3 +- .../vulnerability_list_graphql.vue | 32 +++++++++++++------ 2 files changed, 24 insertions(+), 11 deletions(-) diff --git a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue index 34d759633ed6aa..a08a6b4fec9592 100644 --- a/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue +++ b/ee/app/assets/javascripts/security_dashboard/components/shared/vulnerability_report/vulnerability_list.vue @@ -456,11 +456,12 @@ export default {
-