Merge branch '7737-ci-pipeline-view-slowed-down-massivly-if-security-tabs-has-many-entries' into 'master'

Improve performance of rendering large reports

See merge request gitlab-org/gitlab-ce!22835

Author: iamphill

app/assets/javascripts/reports/components/issues_list.vue

@@ -1,18 +1,31 @@
 <script>
-import IssuesBlock from '~/reports/components/report_issues.vue';
-import { STATUS_SUCCESS, STATUS_FAILED, STATUS_NEUTRAL } from '~/reports/constants';
+import ReportItem from '~/reports/components/report_item.vue';
+import { STATUS_FAILED, STATUS_NEUTRAL, STATUS_SUCCESS } from '~/reports/constants';
+import SmartVirtualList from '~/vue_shared/components/smart_virtual_list.vue';
+
+const wrapIssueWithState = (status, isNew = false) => issue => ({
+  status: issue.status || status,
+  isNew,
+  issue,
+});
 
 /**
  * Renders block of issues
  */
-
 export default {
   components: {
-    IssuesBlock,
+    SmartVirtualList,
+    ReportItem,
   },
-  success: STATUS_SUCCESS,
-  failed: STATUS_FAILED,
-  neutral: STATUS_NEUTRAL,
+  // Typical height of a report item in px
+  typicalReportItemHeight: 32,
+  /*
+   The maximum amount of shown issues. This is calculated by
+   ( max-height of report-block-list / typicalReportItemHeight ) + some safety margin
+   We will use VirtualList if we have more items than this number.
+   For entries lower than this number, the virtual scroll list calculates the total height of the element wrongly.
+   */
+  maxShownReportItems: 20,
   props: {
     newIssues: {
       type: Array,
@@ -40,42 +53,34 @@ export default {
       default: '',
     },
   },
+  computed: {
+    issuesWithState() {
+      return [
+        ...this.newIssues.map(wrapIssueWithState(STATUS_FAILED, true)),
+        ...this.unresolvedIssues.map(wrapIssueWithState(STATUS_FAILED)),
+        ...this.neutralIssues.map(wrapIssueWithState(STATUS_NEUTRAL)),
+        ...this.resolvedIssues.map(wrapIssueWithState(STATUS_SUCCESS)),
+      ];
+    },
+  },
 };
 </script>
 <template>
-  <div class="report-block-container">
-
-    <issues-block
-      v-if="newIssues.length"
-      :component="component"
-      :issues="newIssues"
-      class="js-mr-code-new-issues"
-      status="failed"
-      is-new
-    />
-
-    <issues-block
-      v-if="unresolvedIssues.length"
-      :component="component"
-      :issues="unresolvedIssues"
-      :status="$options.failed"
-      class="js-mr-code-new-issues"
-    />
-
-    <issues-block
-      v-if="neutralIssues.length"
-      :component="component"
-      :issues="neutralIssues"
-      :status="$options.neutral"
-      class="js-mr-code-non-issues"
-    />
-
-    <issues-block
-      v-if="resolvedIssues.length"
+  <smart-virtual-list
+    :length="issuesWithState.length"
+    :remain="$options.maxShownReportItems"
+    :size="$options.typicalReportItemHeight"
+    class="report-block-container"
+    wtag="ul"
+    wclass="report-block-list"
+  >
+    <report-item
+      v-for="(wrapped, index) in issuesWithState"
+      :key="index"
+      :issue="wrapped.issue"
+      :status="wrapped.status"
       :component="component"
-      :issues="resolvedIssues"
-      :status="$options.success"
-      class="js-mr-code-resolved-issues"
+      :is-new="wrapped.isNew"
     />
-  </div>
+  </smart-virtual-list>
 </template>

app/assets/javascripts/reports/components/report_issues.vue renamed to app/assets/javascripts/reports/components/report_item.vue

@@ -3,14 +3,14 @@ import IssueStatusIcon from '~/reports/components/issue_status_icon.vue';
 import { components, componentNames } from '~/reports/components/issue_body';
 
 export default {
-  name: 'ReportIssues',
+  name: 'ReportItem',
   components: {
     IssueStatusIcon,
     ...components,
   },
   props: {
-    issues: {
-      type: Array,
+    issue: {
+      type: Object,
       required: true,
     },
     component: {
@@ -33,27 +33,21 @@ export default {
 };
 </script>
 <template>
-  <div>
-    <ul class="report-block-list">
-      <li
-        v-for="(issue, index) in issues"
-        :key="index"
-        :class="{ 'is-dismissed': issue.isDismissed }"
-        class="report-block-list-issue"
-      >
-        <issue-status-icon
-          :status="issue.status || status"
-          class="append-right-5"
-        />
+  <li
+    :class="{ 'is-dismissed': issue.isDismissed }"
+    class="report-block-list-issue"
+  >
+    <issue-status-icon
+      :status="status"
+      class="append-right-5"
+    />
 
-        <component
-          :is="component"
-          v-if="component"
-          :issue="issue"
-          :status="issue.status || status"
-          :is-new="isNew"
-        />
-      </li>
-    </ul>
-  </div>
+    <component
+      :is="component"
+      v-if="component"
+      :issue="issue"
+      :status="status"
+      :is-new="isNew"
+    />
+  </li>
 </template>

app/assets/javascripts/vue_shared/components/smart_virtual_list.vue

@@ -0,0 +1,42 @@
+<script>
+import VirtualList from 'vue-virtual-scroll-list';
+
+export default {
+  name: 'SmartVirtualList',
+  components: { VirtualList },
+  props: {
+    size: { type: Number, required: true },
+    length: { type: Number, required: true },
+    remain: { type: Number, required: true },
+    rtag: { type: String, default: 'div' },
+    wtag: { type: String, default: 'div' },
+    wclass: { type: String, default: null },
+  },
+};
+</script>
+<template>
+  <virtual-list
+    v-if="length > remain"
+    v-bind="$attrs"
+    :size="remain"
+    :remain="remain"
+    :rtag="rtag"
+    :wtag="wtag"
+    :wclass="wclass"
+    class="js-virtual-list"
+  >
+    <slot></slot>
+  </virtual-list>
+  <component
+    :is="rtag"
+    v-else
+    class="js-plain-element"
+  >
+    <component
+      :is="wtag"
+      :class="wclass"
+    >
+      <slot></slot>
+    </component>
+  </component>
+</template>

changelogs/unreleased/7737-ci-pipeline-view-slowed-down-massivly-if-security-tabs-has-many-entries-ee.yml

@@ -0,0 +1,5 @@
+---
+title: Improve performance of rendering large reports
+merge_request: 22835
+author:
+type: performance

spec/javascripts/reports/components/grouped_test_reports_app_spec.js

@@ -151,11 +151,11 @@ describe('Grouped Test Reports App', () => {
 
     it('renders resolved failures', done => {
       setTimeout(() => {
-        expect(vm.$el.querySelector('.js-mr-code-resolved-issues').textContent).toContain(
+        expect(vm.$el.querySelector('.report-block-container').textContent).toContain(
           resolvedFailures.suites[0].resolved_failures[0].name,
         );
 
-        expect(vm.$el.querySelector('.js-mr-code-resolved-issues').textContent).toContain(
+        expect(vm.$el.querySelector('.report-block-container').textContent).toContain(
           resolvedFailures.suites[0].resolved_failures[1].name,
         );
         done();

spec/javascripts/reports/components/report_section_spec.js

@@ -120,7 +120,7 @@ describe('Report section', () => {
         'Code quality improved on 1 point and degraded on 1 point',
       );
 
-      expect(vm.$el.querySelectorAll('.js-mr-code-resolved-issues li').length).toEqual(
+      expect(vm.$el.querySelectorAll('.report-block-container li').length).toEqual(
         resolvedIssues.length,
       );
     });

spec/javascripts/vue_shared/components/smart_virtual_list_spec.js

@@ -0,0 +1,83 @@
+import Vue from 'vue';
+import SmartVirtualScrollList from '~/vue_shared/components/smart_virtual_list.vue';
+import mountComponent from 'spec/helpers/vue_mount_component_helper';
+
+describe('Toggle Button', () => {
+  let vm;
+
+  const createComponent = ({ length, remain }) => {
+    const smartListProperties = {
+      rtag: 'section',
+      wtag: 'ul',
+      wclass: 'test-class',
+      // Size in pixels does not matter for our tests here
+      size: 35,
+      length,
+      remain,
+    };
+
+    const Component = Vue.extend({
+      components: {
+        SmartVirtualScrollList,
+      },
+      smartListProperties,
+      items: Array(length).fill(1),
+      template: `
+      <smart-virtual-scroll-list v-bind="$options.smartListProperties">
+        <li v-for="(val, key) in $options.items" :key="key">{{ key + 1 }}</li>
+      </smart-virtual-scroll-list>`,
+    });
+
+    return mountComponent(Component);
+  };
+
+  afterEach(() => {
+    vm.$destroy();
+  });
+
+  describe('if the list is shorter than the maximum shown elements', () => {
+    const listLength = 10;
+
+    beforeEach(() => {
+      vm = createComponent({ length: listLength, remain: 20 });
+    });
+
+    it('renders without the vue-virtual-scroll-list component', () => {
+      expect(vm.$el.classList).not.toContain('js-virtual-list');
+      expect(vm.$el.classList).toContain('js-plain-element');
+    });
+
+    it('renders list with provided tags and classes for the wrapper elements', () => {
+      expect(vm.$el.tagName).toEqual('SECTION');
+      expect(vm.$el.firstChild.tagName).toEqual('UL');
+      expect(vm.$el.firstChild.classList).toContain('test-class');
+    });
+
+    it('renders all children list elements', () => {
+      expect(vm.$el.querySelectorAll('li').length).toEqual(listLength);
+    });
+  });
+
+  describe('if the list is longer than the maximum shown elements', () => {
+    const maxItemsShown = 20;
+
+    beforeEach(() => {
+      vm = createComponent({ length: 1000, remain: maxItemsShown });
+    });
+
+    it('uses the vue-virtual-scroll-list component', () => {
+      expect(vm.$el.classList).toContain('js-virtual-list');
+      expect(vm.$el.classList).not.toContain('js-plain-element');
+    });
+
+    it('renders list with provided tags and classes for the wrapper elements', () => {
+      expect(vm.$el.tagName).toEqual('SECTION');
+      expect(vm.$el.firstChild.tagName).toEqual('UL');
+      expect(vm.$el.firstChild.classList).toContain('test-class');
+    });
+
+    it('renders at max twice the maximum shown elements', () => {
+      expect(vm.$el.querySelectorAll('li').length).toBeLessThanOrEqual(2 * maxItemsShown);
+    });
+  });
+});