Merge branch 'ph/321691/improveDiffsRenderingQueue' into 'master'

Impove the diffs app render queue

See merge request gitlab-org/gitlab!57369

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

@@ -349,14 +349,6 @@ export default {
     refetchDiffData() {
       this.fetchData(false);
     },
-    startDiffRendering() {
-      requestIdleCallback(
-        () => {
-          this.startRenderDiffsQueue();
-        },
-        { timeout: 1000 },
-      );
-    },
     needsReload() {
       return this.diffFiles.length && isSingleViewStyle(this.diffFiles[0]);
     },
@@ -368,8 +360,6 @@ export default {
         .then(({ real_size }) => {
           this.diffFilesLength = parseInt(real_size, 10);
           if (toggleTree) this.setTreeDisplay();
-
-          this.startDiffRendering();
         })
         .catch(() => {
           createFlash(__('Something went wrong on our end. Please try again!'));
@@ -384,7 +374,6 @@ export default {
           // change when loading the other half of the diff files.
           this.setDiscussions();
         })
-        .then(() => this.startDiffRendering())
         .catch(() => {
           createFlash(__('Something went wrong on our end. Please try again!'));
         });

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

@@ -49,7 +49,6 @@ import {
   convertExpandLines,
   idleCallback,
   allDiscussionWrappersExpanded,
-  prepareDiffData,
   prepareLineForRenamedFile,
 } from './utils';
 
@@ -163,7 +162,15 @@ export const fetchDiffFilesBatch = ({ commit, state, dispatch }) => {
 
         return pagination.next_page;
       })
-      .then((nextPage) => nextPage && getBatch(nextPage))
+      .then((nextPage) => {
+        dispatch('startRenderDiffsQueue');
+
+        if (nextPage) {
+          return getBatch(nextPage);
+        }
+
+        return null;
+      })
       .catch(() => commit(types.SET_RETRIEVING_BATCHES, false));
 
   return getBatch()
@@ -197,13 +204,7 @@ export const fetchDiffFilesMeta = ({ commit, state }) => {
       commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []);
       commit(types.SET_DIFF_METADATA, strippedData);
 
-      worker.postMessage(
-        prepareDiffData({
-          diff: data,
-          priorFiles: state.diffFiles,
-          meta: true,
-        }),
-      );
+      worker.postMessage(data.diff_files);
 
       return data;
     })
@@ -304,33 +305,38 @@ export const renderFileForDiscussionId = ({ commit, rootState, state }, discussi
 };
 
 export const startRenderDiffsQueue = ({ state, commit }) => {
-  const checkItem = () =>
-    new Promise((resolve) => {
-      const nextFile = state.diffFiles.find(
-        (file) =>
-          !file.renderIt &&
-          file.viewer &&
-          (!isCollapsed(file) || file.viewer.name !== diffViewerModes.text),
-      );
-
-      if (nextFile) {
-        requestAnimationFrame(() => {
-          commit(types.RENDER_FILE, nextFile);
+  const diffFilesToRender = state.diffFiles.filter(
+    (file) =>
+      !file.renderIt &&
+      file.viewer &&
+      (!isCollapsed(file) || file.viewer.name !== diffViewerModes.text),
+  );
+  let currentDiffFileIndex = 0;
+
+  const checkItem = () => {
+    const nextFile = diffFilesToRender[currentDiffFileIndex];
+
+    if (nextFile) {
+      currentDiffFileIndex += 1;
+      commit(types.RENDER_FILE, nextFile);
+
+      const requestIdle = () =>
+        requestIdleCallback((idleDeadline) => {
+          // Wait for at least 5ms before trying to render
+          if (idleDeadline.timeRemaining() >= 6) {
+            checkItem();
+          } else {
+            requestIdle();
+          }
         });
-        requestIdleCallback(
-          () => {
-            checkItem()
-              .then(resolve)
-              .catch(() => {});
-          },
-          { timeout: 1000 },
-        );
-      } else {
-        resolve();
-      }
-    });
 
-  return checkItem();
+      requestIdle();
+    }
+  };
+
+  if (diffFilesToRender.length) {
+    checkItem();
+  }
 };
 
 export const setRenderIt = ({ commit }, file) => commit(types.RENDER_FILE, file);

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

@@ -381,22 +381,13 @@ function prepareDiffFileLines(file) {
 
   inlineLines.forEach((line) => prepareLine(line, file)); // WARNING: In-Place Mutations!
 
-  Object.assign(file, {
-    inlineLinesCount: inlineLines.length,
-  });
-
   return file;
 }
 
-function getVisibleDiffLines(file) {
-  return file.inlineLinesCount;
-}
-
-function finalizeDiffFile(file) {
-  const lines = getVisibleDiffLines(file);
-
+function finalizeDiffFile(file, index) {
   Object.assign(file, {
-    renderIt: lines < LINES_TO_BE_RENDERED_DIRECTLY,
+    renderIt:
+      index < 3 ? file[INLINE_DIFF_LINES_KEY].length < LINES_TO_BE_RENDERED_DIRECTLY : false,
     isShowingFullFile: false,
     isLoadingFullFile: false,
     discussions: [],
@@ -424,7 +415,7 @@ export function prepareDiffData({ diff, priorFiles = [], meta = false }) {
     .map((file, index, allFiles) => prepareRawDiffFile({ file, allFiles, meta }))
     .map(ensureBasicDiffFileLines)
     .map(prepareDiffFileLines)
-    .map(finalizeDiffFile);
+    .map((file, index) => finalizeDiffFile(file, priorFiles.length + index));
 
   return deduplicateFilesList([...priorFiles, ...cleanedFiles]);
 }

spec/frontend/diffs/components/app_spec.js

@@ -105,7 +105,6 @@ describe('diffs/components/app', () => {
       jest.spyOn(wrapper.vm, 'fetchDiffFilesBatch').mockImplementation(fetchResolver);
       jest.spyOn(wrapper.vm, 'fetchCoverageFiles').mockImplementation(fetchResolver);
       jest.spyOn(wrapper.vm, 'setDiscussions').mockImplementation(() => {});
-      jest.spyOn(wrapper.vm, 'startRenderDiffsQueue').mockImplementation(() => {});
       jest.spyOn(wrapper.vm, 'unwatchDiscussions').mockImplementation(() => {});
       jest.spyOn(wrapper.vm, 'unwatchRetrievingBatches').mockImplementation(() => {});
       store.state.diffs.retrievingBatches = true;
@@ -119,7 +118,6 @@ describe('diffs/components/app', () => {
 
       await nextTick();
 
-      expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
       expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();
       expect(wrapper.vm.fetchDiffFilesBatch).toHaveBeenCalled();
       expect(wrapper.vm.fetchCoverageFiles).toHaveBeenCalled();
@@ -134,7 +132,6 @@ describe('diffs/components/app', () => {
 
       await nextTick();
 
-      expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
       expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();
       expect(wrapper.vm.fetchDiffFilesBatch).toHaveBeenCalled();
       expect(wrapper.vm.fetchCoverageFiles).toHaveBeenCalled();

spec/frontend/diffs/store/actions_spec.js

@@ -80,7 +80,7 @@ describe('DiffsStoreActions', () => {
     jest.spyOn(utils, 'idleCallback').mockImplementation(() => null);
     ['requestAnimationFrame', 'requestIdleCallback'].forEach((method) => {
       global[method] = (cb) => {
-        cb();
+        cb({ timeRemaining: () => 10 });
       };
     });
   });
@@ -198,7 +198,7 @@ describe('DiffsStoreActions', () => {
           { type: types.VIEW_DIFF_FILE, payload: 'test2' },
           { type: types.SET_RETRIEVING_BATCHES, payload: false },
         ],
-        [],
+        [{ type: 'startRenderDiffsQueue' }, { type: 'startRenderDiffsQueue' }],
         done,
       );
     });