Merge branch 'ph/37740/diffBatchRaceCondition' into 'master'

Fix race condition with batch diff loading

Closes #37740

See merge request gitlab-org/gitlab!21124

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

@@ -144,6 +144,9 @@ export default {
     isLimitedContainer() {
       return !this.showTreeList && !this.isParallelView && !this.isFluidLayout;
     },
+    shouldSetDiscussions() {
+      return this.isNotesFetched && !this.assignedDiscussions && !this.isLoading;
+    },
   },
   watch: {
     diffViewType() {
@@ -160,6 +163,11 @@ export default {
     },
     isLoading: 'adjustView',
     showTreeList: 'adjustView',
+    shouldSetDiscussions(newVal) {
+      if (newVal) {
+        this.setDiscussions();
+      }
+    },
   },
   mounted() {
     this.setBaseConfig({
@@ -214,26 +222,28 @@ export default {
     isLatestVersion() {
       return window.location.search.indexOf('diff_id') === -1;
     },
+    startDiffRendering() {
+      requestIdleCallback(
+        () => {
+          this.startRenderDiffsQueue();
+        },
+        { timeout: 1000 },
+      );
+    },
     fetchData(toggleTree = true) {
       if (this.isLatestVersion() && this.glFeatures.diffsBatchLoad) {
         this.fetchDiffFilesMeta()
           .then(() => {
             if (toggleTree) this.hideTreeListIfJustOneFile();
+
+            this.startDiffRendering();
           })
           .catch(() => {
             createFlash(__('Something went wrong on our end. Please try again!'));
           });
 
         this.fetchDiffFilesBatch()
-          .then(() => {
-            requestIdleCallback(
-              () => {
-                this.setDiscussions();
-                this.startRenderDiffsQueue();
-              },
-              { timeout: 1000 },
-            );
-          })
+          .then(() => this.startDiffRendering())
           .catch(() => {
             createFlash(__('Something went wrong on our end. Please try again!'));
           });
@@ -246,7 +256,6 @@ export default {
 
             requestIdleCallback(
               () => {
-                this.setDiscussions();
                 this.startRenderDiffsQueue();
               },
               { timeout: 1000 },
@@ -262,7 +271,7 @@ export default {
       }
     },
     setDiscussions() {
-      if (this.isNotesFetched && !this.assignedDiscussions && !this.isLoading) {
+      if (this.shouldSetDiscussions) {
         this.assignedDiscussions = true;
 
         requestIdleCallback(

app/assets/javascripts/diffs/store/actions.js

@@ -119,7 +119,7 @@ export const fetchDiffFilesMeta = ({ commit, state }) => {
     .get(state.endpointMetadata)
     .then(({ data }) => {
       const strippedData = { ...data };
-      strippedData.diff_files = [];
+      delete strippedData.diff_files;
       commit(types.SET_LOADING, false);
       commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []);
       commit(types.SET_DIFF_DATA, strippedData);

app/assets/javascripts/diffs/store/mutations.js

@@ -39,7 +39,16 @@ export default {
   },
 
   [types.SET_DIFF_DATA](state, data) {
+    if (
+      !(
+        gon &&
+        gon.features &&
+        gon.features.diffsBatchLoad &&
+        window.location.search.indexOf('diff_id') === -1
+      )
+    ) {
       prepareDiffData(data);
+    }
 
     Object.assign(state, {
       ...convertObjectPropsToCamelCase(data),

spec/javascripts/diffs/components/app_spec.js

@@ -77,16 +77,17 @@ describe('diffs/components/app', () => {
       spyOn(wrapper.vm, 'startRenderDiffsQueue');
     });
 
-    it('calls fetchDiffFiles if diffsBatchLoad is not enabled', () => {
+    it('calls fetchDiffFiles if diffsBatchLoad is not enabled', done => {
       wrapper.vm.glFeatures.diffsBatchLoad = false;
       wrapper.vm.fetchData(false);
 
       expect(wrapper.vm.fetchDiffFiles).toHaveBeenCalled();
-      wrapper.vm.$nextTick(() => {
-        expect(wrapper.vm.setDiscussions).toHaveBeenCalled();
+      setTimeout(() => {
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
+
+        done();
       });
     });
 
@@ -97,7 +98,6 @@ describe('diffs/components/app', () => {
 
       expect(wrapper.vm.fetchDiffFiles).toHaveBeenCalled();
       wrapper.vm.$nextTick(() => {
-        expect(wrapper.vm.setDiscussions).toHaveBeenCalled();
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
@@ -110,7 +110,6 @@ describe('diffs/components/app', () => {
 
       expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
       wrapper.vm.$nextTick(() => {
-        expect(wrapper.vm.setDiscussions).toHaveBeenCalled();
         expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();
         expect(wrapper.vm.fetchDiffFilesBatch).toHaveBeenCalled();

spec/javascripts/diffs/store/actions_spec.js

@@ -186,7 +186,7 @@ describe('DiffsStoreActions', () => {
           { type: types.SET_LOADING, payload: true },
           { type: types.SET_LOADING, payload: false },
           { type: types.SET_MERGE_REQUEST_DIFFS, payload: [] },
-          { type: types.SET_DIFF_DATA, payload: { data, diff_files: [] } },
+          { type: types.SET_DIFF_DATA, payload: { data } },
         ],
         [],
         () => {