Prevent fetching diffs and discussions data unnecessarily on MR page.acet-fix-fetching-diffs-data

5 files changed, 42 insertions, 33 deletions

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index deddb61ca31..df201ce0c0a 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -3,6 +3,7 @@ import { mapState, mapGetters, mapActions } from 'vuex';
 import Icon from '~/vue_shared/components/icon.vue';
 import { __ } from '~/locale';
 import createFlash from '~/flash';
+import eventHub from '../../notes/event_hub';
 import LoadingIcon from '../../vue_shared/components/loading_icon.vue';
 import CompareVersions from './compare_versions.vue';
 import ChangedFiles from './changed_files.vue';
@@ -62,7 +63,7 @@ export default {
       plainDiffPath: state => state.diffs.plainDiffPath,
       emailPatchPath: state => state.diffs.emailPatchPath,
     }),
-    ...mapGetters(['isParallelView']),
+    ...mapGetters(['isParallelView', 'discussions']),
     targetBranch() {
       return {
         branchName: this.targetBranchName,
@@ -94,20 +95,36 @@ export default {
       this.adjustView();
     },
     shouldShow() {
+      // When the shouldShow property changed to true, the route is rendered for the first time
+      // and if we have the isLoading as true this means we didn't fetch the discussions data
+      if (this.isLoading) {
+        this.fetchData();
+
+        if (!this.discussions.length) {
+          eventHub.$emit('fetchNotesData');
+        }
+      }
+
       this.adjustView();
     },
   },
   mounted() {
     this.setBaseConfig({ endpoint: this.endpoint, projectPath: this.projectPath });
-    this.fetchDiffFiles().catch(() => {
-      createFlash(__('Fetching diff files failed. Please reload the page to try again!'));
-    });
+
+    if (this.shouldShow) {
+      this.fetchData();
+    }
   },
   created() {
     this.adjustView();
   },
   methods: {
     ...mapActions(['setBaseConfig', 'fetchDiffFiles']),
+    fetchData() {
+      this.fetchDiffFiles().catch(() => {
+        createFlash(__('Something went wrong on our end. Please try again!'));
+      });
+    },
     setActive(filePath) {
       this.activeFile = filePath;
     },
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index bf188a44022..5e0fd5109bb 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -15,10 +15,6 @@ export const setBaseConfig = ({ commit }, options) => {
   commit(types.SET_BASE_CONFIG, { endpoint, projectPath });
 };
 
-export const setLoadingState = ({ commit }, state) => {
-  commit(types.SET_LOADING, state);
-};
-
 export const fetchDiffFiles = ({ state, commit }) => {
   commit(types.SET_LOADING, true);
 
@@ -88,7 +84,6 @@ export const expandAllFiles = ({ commit }) => {
 
 export default {
   setBaseConfig,
-  setLoadingState,
   fetchDiffFiles,
   setInlineDiffViewType,
   setParallelDiffViewType,
diff --git a/app/assets/javascripts/mr_notes/index.js b/app/assets/javascripts/mr_notes/index.js
index 3c0c9995cc2..7d0368c826f 100644
--- a/app/assets/javascripts/mr_notes/index.js
+++ b/app/assets/javascripts/mr_notes/index.js
@@ -45,13 +45,15 @@ export default function initMrNotes() {
         this.updateDiscussionTabCounter();
       },
     },
-    mounted() {
-      this.notesCountBadge = $('.issuable-details').find('.notes-tab .badge');
+    created() {
       this.setActiveTab(window.mrTabs.getCurrentAction());
 
       window.mrTabs.eventHub.$on('MergeRequestTabChange', tab => {
         this.setActiveTab(tab);
       });
+    },
+    mounted() {
+      this.notesCountBadge = $('.issuable-details').find('.notes-tab .badge');
       $(document).on('visibilitychange', this.updateDiscussionTabCounter);
     },
     beforeDestroy() {
diff --git a/app/assets/javascripts/notes/components/notes_app.vue b/app/assets/javascripts/notes/components/notes_app.vue
index 98f8b9af168..b6415bbba15 100644
--- a/app/assets/javascripts/notes/components/notes_app.vue
+++ b/app/assets/javascripts/notes/components/notes_app.vue
@@ -3,6 +3,7 @@ import { mapGetters, mapActions } from 'vuex';
 import { getLocationHash } from '../../lib/utils/url_utility';
 import Flash from '../../flash';
 import * as constants from '../constants';
+import eventHub from '../event_hub';
 import noteableNote from './noteable_note.vue';
 import noteableDiscussion from './noteable_discussion.vue';
 import systemNote from '../../vue_shared/components/notes/system_note.vue';
@@ -64,16 +65,26 @@ export default {
       return this.discussions;
     },
   },
+  watch: {
+    shouldShow() {
+      if (!this.discussions.length) {
+        this.fetchNotes();
+      }
+    },
+  },
   created() {
     this.setNotesData(this.notesData);
     this.setNoteableData(this.noteableData);
     this.setUserData(this.userData);
     this.setTargetNoteHash(getLocationHash());
+    eventHub.$once('fetchNotesData', this.fetchNotes);
   },
   mounted() {
-    this.fetchNotes();
-    const { parentElement } = this.$el;
+    if (this.shouldShow) {
+      this.fetchNotes();
+    }
 
+    const { parentElement } = this.$el;
     if (parentElement && parentElement.classList.contains('js-vue-notes-event')) {
       parentElement.addEventListener('toggleAward', event => {
         const { awardName, noteId } = event.detail;
@@ -161,11 +172,12 @@ export default {
 <template>
   <div
     v-if="shouldShow"
-    id="notes">
+    id="notes"
+  >
     <ul
       id="notes-list"
-      class="notes main-notes-list timeline">
-
+      class="notes main-notes-list timeline"
+    >
       <component
         v-for="discussion in allDiscussions"
         :is="getComponentName(discussion)"
diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js
index f0bd098f698..6829c1e956a 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/spec/javascripts/diffs/store/actions_spec.js
@@ -5,7 +5,6 @@ import {
   INLINE_DIFF_VIEW_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
 } from '~/diffs/constants';
-import store from '~/diffs/store';
 import * as actions from '~/diffs/store/actions';
 import * as types from '~/diffs/store/mutation_types';
 import axios from '~/lib/utils/axios_utils';
@@ -28,22 +27,6 @@ describe('DiffsStoreActions', () => {
     });
   });
 
-  describe('setLoadingState', () => {
-    it('should set loading state', done => {
-      expect(store.state.diffs.isLoading).toEqual(true);
-      const loadingState = false;
-
-      testAction(
-        actions.setLoadingState,
-        loadingState,
-        {},
-        [{ type: types.SET_LOADING, payload: loadingState }],
-        [],
-        done,
-      );
-    });
-  });
-
   describe('fetchDiffFiles', () => {
     it('should fetch diff files', done => {
       const endpoint = '/fetch/diff/files';