Merge branch 'tz-mr-diff-performance' into 'master'

Performance Improvements to Vue MR page

Closes #48492

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

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

@@ -145,7 +145,7 @@ export default {
 </script>
 
 <template>
-  <div v-if="shouldShow">
+  <div v-show="shouldShow">
     <div
       v-if="isLoading"
       class="loading"

app/assets/javascripts/merge_request_tabs.js

@@ -69,12 +69,23 @@ let { location } = window;
 
 export default class MergeRequestTabs {
   constructor({ action, setUrl, stubLocation } = {}) {
-    const mergeRequestTabs = document.querySelector('.js-tabs-affix');
+    this.mergeRequestTabs = document.querySelector('.merge-request-tabs-container');
+    this.mergeRequestTabsAll =
+      this.mergeRequestTabs && this.mergeRequestTabs.querySelectorAll
+        ? this.mergeRequestTabs.querySelectorAll('.merge-request-tabs li')
+        : null;
+    this.mergeRequestTabPanes = document.querySelector('#diff-notes-app');
+    this.mergeRequestTabPanesAll =
+      this.mergeRequestTabPanes && this.mergeRequestTabPanes.querySelectorAll
+        ? this.mergeRequestTabPanes.querySelectorAll('.tab-pane')
+        : null;
     const navbar = document.querySelector('.navbar-gitlab');
     const peek = document.getElementById('js-peek');
     const paddingTop = 16;
+
     this.commitsTab = document.querySelector('.tab-content .commits.tab-pane');
 
+    this.currentTab = null;
     this.diffsLoaded = false;
     this.pipelinesLoaded = false;
     this.commitsLoaded = false;
@@ -84,15 +95,15 @@ export default class MergeRequestTabs {
     this.setUrl = setUrl !== undefined ? setUrl : true;
     this.setCurrentAction = this.setCurrentAction.bind(this);
     this.tabShown = this.tabShown.bind(this);
-    this.showTab = this.showTab.bind(this);
+    this.clickTab = this.clickTab.bind(this);
     this.stickyTop = navbar ? navbar.offsetHeight - paddingTop : 0;
 
     if (peek) {
       this.stickyTop += peek.offsetHeight;
     }
 
-    if (mergeRequestTabs) {
-      this.stickyTop += mergeRequestTabs.offsetHeight;
+    if (this.mergeRequestTabs) {
+      this.stickyTop += this.mergeRequestTabs.offsetHeight;
     }
 
     if (stubLocation) {
@@ -100,25 +111,22 @@ export default class MergeRequestTabs {
     }
 
     this.bindEvents();
-    this.activateTab(action);
+    if (
+      this.mergeRequestTabs &&
+      this.mergeRequestTabs.querySelector(`a[data-action='${action}']`) &&
+      this.mergeRequestTabs.querySelector(`a[data-action='${action}']`).click
+    )
+      this.mergeRequestTabs.querySelector(`a[data-action='${action}']`).click();
     this.initAffix();
   }
 
   bindEvents() {
-    $(document)
-      .on('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown)
-      .on('click', '.js-show-tab', this.showTab);
-
-    $('.merge-request-tabs a[data-toggle="tab"]').on('click', this.clickTab);
+    $('.merge-request-tabs a[data-toggle="tabvue"]').on('click', this.clickTab);
   }
 
   // Used in tests
   unbindEvents() {
-    $(document)
-      .off('shown.bs.tab', '.merge-request-tabs a[data-toggle="tab"]', this.tabShown)
-      .off('click', '.js-show-tab', this.showTab);
-
-    $('.merge-request-tabs a[data-toggle="tab"]').off('click', this.clickTab);
+    $('.merge-request-tabs a[data-toggle="tabvue"]').off('click', this.clickTab);
   }
 
   destroyPipelinesView() {
@@ -130,58 +138,87 @@ export default class MergeRequestTabs {
     }
   }
 
-  showTab(e) {
-    e.preventDefault();
-    this.activateTab($(e.target).data('action'));
-  }
-
   clickTab(e) {
-    if (e.currentTarget && isMetaClick(e)) {
-      const targetLink = e.currentTarget.getAttribute('href');
+    if (e.currentTarget) {
       e.stopImmediatePropagation();
       e.preventDefault();
-      window.open(targetLink, '_blank');
+
+      const { action } = e.currentTarget.dataset;
+
+      if (action) {
+        const href = e.currentTarget.getAttribute('href');
+        this.tabShown(action, href);
+      } else if (isMetaClick(e)) {
+        const targetLink = e.currentTarget.getAttribute('href');
+        window.open(targetLink, '_blank');
+      }
     }
   }
 
-  tabShown(e) {
-    const $target = $(e.target);
-    const action = $target.data('action');
-
-    if (action === 'commits') {
-      this.loadCommits($target.attr('href'));
-      this.expandView();
-      this.resetViewContainer();
-      this.destroyPipelinesView();
-    } else if (this.isDiffAction(action)) {
-      if (!isInVueNoteablePage()) {
-        this.loadDiff($target.attr('href'));
-      }
-      if (bp.getBreakpointSize() !== 'lg') {
-        this.shrinkView();
+  tabShown(action, href) {
+    if (action !== this.currentTab && this.mergeRequestTabs) {
+      this.currentTab = action;
+
+      if (this.mergeRequestTabPanesAll) {
+        this.mergeRequestTabPanesAll.forEach(el => {
+          const tabPane = el;
+          tabPane.style.display = 'none';
+        });
       }
-      if (this.diffViewType() === 'parallel') {
-        this.expandViewContainer();
+
+      if (this.mergeRequestTabsAll) {
+        this.mergeRequestTabsAll.forEach(el => {
+          el.classList.remove('active');
+        });
       }
-      this.destroyPipelinesView();
-      this.commitsTab.classList.remove('active');
-    } else if (action === 'pipelines') {
-      this.resetViewContainer();
-      this.mountPipelinesView();
-    } else {
-      if (bp.getBreakpointSize() !== 'xs') {
+
+      const tabPane = this.mergeRequestTabPanes.querySelector(`#${action}`);
+      if (tabPane) tabPane.style.display = 'block';
+      const tab = this.mergeRequestTabs.querySelector(`.${action}-tab`);
+      if (tab) tab.classList.add('active');
+
+      if (action === 'commits') {
+        this.loadCommits(href);
+        this.expandView();
+        this.resetViewContainer();
+        this.destroyPipelinesView();
+      } else if (action === 'new') {
         this.expandView();
+        this.resetViewContainer();
+        this.destroyPipelinesView();
+      } else if (this.isDiffAction(action)) {
+        if (!isInVueNoteablePage()) {
+          this.loadDiff(href);
+        }
+        if (bp.getBreakpointSize() !== 'lg') {
+          this.shrinkView();
+        }
+        if (this.diffViewType() === 'parallel') {
+          this.expandViewContainer();
+        }
+        this.destroyPipelinesView();
+        this.commitsTab.classList.remove('active');
+      } else if (action === 'pipelines') {
+        this.resetViewContainer();
+        this.mountPipelinesView();
+      } else {
+        this.mergeRequestTabPanes.querySelector('#notes').style.display = 'block';
+        this.mergeRequestTabs.querySelector('.notes-tab').classList.add('active');
+
+        if (bp.getBreakpointSize() !== 'xs') {
+          this.expandView();
+        }
+        this.resetViewContainer();
+        this.destroyPipelinesView();
+
+        initDiscussionTab();
+      }
+      if (this.setUrl) {
+        this.setCurrentAction(action);
       }
-      this.resetViewContainer();
-      this.destroyPipelinesView();
 
-      initDiscussionTab();
-    }
-    if (this.setUrl) {
-      this.setCurrentAction(action);
+      this.eventHub.$emit('MergeRequestTabChange', this.getCurrentAction());
     }
-
-    this.eventHub.$emit('MergeRequestTabChange', this.getCurrentAction());
   }
 
   scrollToElement(container) {
@@ -194,12 +231,6 @@ export default class MergeRequestTabs {
     }
   }
 
-  // Activate a tab based on the current action
-  activateTab(action) {
-    // important note: the .tab('show') method triggers 'shown.bs.tab' event itself
-    $(`.merge-request-tabs a[data-action='${action}']`).tab('show');
-  }
-
   // Replaces the current Merge Request-specific action in the URL with a new one
   //
   // If the action is "notes", the URL is reset to the standard

app/assets/javascripts/notes/components/notes_app.vue

@@ -175,7 +175,7 @@ export default {
 
 <template>
   <div
-    v-if="shouldShow"
+    v-show="shouldShow"
     id="notes"
   >
     <ul

app/helpers/merge_requests_helper.rb

@@ -108,7 +108,7 @@ module MergeRequestsHelper
     data_attrs = {
       action: tab.to_s,
       target: "##{tab}",
-      toggle: options.fetch(:force_link, false) ? '' : 'tab'
+      toggle: options.fetch(:force_link, false) ? '' : 'tabvue'
     }
 
     url = case tab

app/views/projects/merge_requests/creations/_new_submit.html.haml

@@ -24,23 +24,28 @@
         There are no commits yet.
       = custom_icon ('illustration_no_commits')
   - else
-    %ul.merge-request-tabs.nav.nav-tabs.nav-links.no-top.no-bottom
-      %li.commits-tab
-        = link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tab'} do
-          Commits
-          %span.badge.badge-pill= @commits.size
-      - if @pipelines.any?
-        %li.builds-tab
-          = link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tab'} do
-            Pipelines
-            %span.badge.badge-pill= @pipelines.size
-      %li.diffs-tab
-        = link_to url_for(safe_params.merge(action: 'diffs')), data: {target: 'div#diffs', action: 'diffs', toggle: 'tab'} do
-          Changes
-          %span.badge.badge-pill= @merge_request.diff_size
+    .merge-request-tabs-holder{ class: ("js-tabs-affix" unless ENV['RAILS_ENV'] == 'test') }
+      .merge-request-tabs-container
+        .scrolling-tabs-container.inner-page-scroll-tabs.is-smaller
+          .fade-left= icon('angle-left')
+          .fade-right= icon('angle-right')
+          %ul.merge-request-tabs.nav.nav-tabs.nav-links.no-top.no-bottom.js-tabs-affix
+            %li.commits-tab.new-tab
+              = link_to url_for(safe_params), data: {target: 'div#commits', action: 'new', toggle: 'tabvue'} do
+                Commits
+                %span.badge.badge-pill= @commits.size
+            - if @pipelines.any?
+              %li.builds-tab
+                = link_to url_for(safe_params.merge(action: 'pipelines')), data: {target: 'div#pipelines', action: 'pipelines', toggle: 'tabvue'} do
+                  Pipelines
+                  %span.badge.badge-pill= @pipelines.size
+            %li.diffs-tab
+              = link_to url_for(safe_params.merge(action: 'diffs')), data: {target: 'div#diffs', action: 'diffs', toggle: 'tabvue'} do
+                Changes
+                %span.badge.badge-pill= @merge_request.diff_size
 
-    .tab-content
-      #commits.commits.tab-pane.active
+    #diff-notes-app.tab-content
+      #new.commits.tab-pane.active
         = render "projects/merge_requests/commits"
       #diffs.diffs.tab-pane
         -# This tab is always loaded via AJAX

spec/helpers/merge_requests_helper_spec.rb

@@ -54,7 +54,7 @@ describe MergeRequestsHelper do
       let(:options) { { force_link: true } }
 
       it 'removes the data-toggle attributes' do
-        is_expected.not_to match(/data-toggle="tab"/)
+        is_expected.not_to match(/data-toggle="tabvue"/)
       end
     end
   end

spec/javascripts/helpers/init_vue_mr_page_helper.js

@@ -5,12 +5,16 @@ import { userDataMock, notesDataMock, noteableDataMock } from '../notes/mock_dat
 import diffFileMockData from '../diffs/mock_data/diff_file';
 
 export default function initVueMRPage() {
+  const mrTestEl = document.createElement('div');
+  mrTestEl.className = 'js-merge-request-test';
+  document.body.appendChild(mrTestEl);
+
   const diffsAppEndpoint = '/diffs/app/endpoint';
   const diffsAppProjectPath = 'testproject';
   const mrEl = document.createElement('div');
   mrEl.className = 'merge-request fixture-mr';
   mrEl.setAttribute('data-mr-action', 'diffs');
-  document.body.appendChild(mrEl);
+  mrTestEl.appendChild(mrEl);
 
   const mrDiscussionsEl = document.createElement('div');
   mrDiscussionsEl.id = 'js-vue-mr-discussions';
@@ -18,18 +22,18 @@ export default function initVueMRPage() {
   mrDiscussionsEl.setAttribute('data-noteable-data', JSON.stringify(noteableDataMock));
   mrDiscussionsEl.setAttribute('data-notes-data', JSON.stringify(notesDataMock));
   mrDiscussionsEl.setAttribute('data-noteable-type', 'merge-request');
-  document.body.appendChild(mrDiscussionsEl);
+  mrTestEl.appendChild(mrDiscussionsEl);
 
   const discussionCounterEl = document.createElement('div');
   discussionCounterEl.id = 'js-vue-discussion-counter';
-  document.body.appendChild(discussionCounterEl);
+  mrTestEl.appendChild(discussionCounterEl);
 
   const diffsAppEl = document.createElement('div');
   diffsAppEl.id = 'js-diffs-app';
   diffsAppEl.setAttribute('data-endpoint', diffsAppEndpoint);
   diffsAppEl.setAttribute('data-project-path', diffsAppProjectPath);
   diffsAppEl.setAttribute('data-current-user-data', JSON.stringify(userDataMock));
-  document.body.appendChild(diffsAppEl);
+  mrTestEl.appendChild(diffsAppEl);
 
   const mock = new MockAdapter(axios);
   mock.onGet(diffsAppEndpoint).reply(200, {

spec/javascripts/merge_request_spec.js

@@ -19,9 +19,11 @@ import IssuablesHelper from '~/helpers/issuables_helper';
         spyOn(axios, 'patch').and.callThrough();
         mock = new MockAdapter(axios);
 
-        mock.onPatch(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`).reply(200, {});
+        mock
+          .onPatch(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`)
+          .reply(200, {});
 
-        return this.merge = new MergeRequest();
+        return (this.merge = new MergeRequest());
       });
 
       afterEach(() => {
@@ -32,17 +34,22 @@ import IssuablesHelper from '~/helpers/issuables_helper';
         spyOn($, 'ajax').and.stub();
         const changeEvent = document.createEvent('HTMLEvents');
         changeEvent.initEvent('change', true, true);
-        $('input[type=checkbox]').attr('checked', true)[0].dispatchEvent(changeEvent);
+        $('input[type=checkbox]')
+          .attr('checked', true)[0]
+          .dispatchEvent(changeEvent);
         return expect($('.js-task-list-field').val()).toBe('- [x] Task List Item');
       });
 
-      it('submits an ajax request on tasklist:changed', (done) => {
+      it('submits an ajax request on tasklist:changed', done => {
         $('.js-task-list-field').trigger('tasklist:changed');
 
         setTimeout(() => {
-          expect(axios.patch).toHaveBeenCalledWith(`${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`, {
-            merge_request: { description: '- [ ] Task List Item' },
-          });
+          expect(axios.patch).toHaveBeenCalledWith(
+            `${gl.TEST_HOST}/frontend-fixtures/merge-requests-project/merge_requests/1.json`,
+            {
+              merge_request: { description: '- [ ] Task List Item' },
+            },
+          );
           done();
         });
       });
@@ -119,4 +126,4 @@ import IssuablesHelper from '~/helpers/issuables_helper';
       });
     });
   });
-}).call(window);
+}.call(window));

spec/javascripts/merge_request_tabs_spec.js

@@ -40,6 +40,7 @@ describe('MergeRequestTabs', function() {
     this.class.unbindEvents();
     this.class.destroyPipelinesView();
     mrPageMock.restore();
+    $('.js-merge-request-test').remove();
   });
 
   describe('opensInNewTab', function() {