Upgrade diff preparation

This change modifies the actions and mutations
to handle only a single diff style (either inline or
parallel) and properly merge that with existing
files already in state (if any).

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

@@ -6,6 +6,7 @@ import { __ } from '~/locale';
 import createFlash from '~/flash';
 import PanelResizer from '~/vue_shared/components/panel_resizer.vue';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
+import { isSingleViewStyle } from '~/helpers/diffs_helper';
 import eventHub from '../../notes/event_hub';
 import CompareVersions from './compare_versions.vue';
 import DiffFile from './diff_file.vue';
@@ -145,6 +146,9 @@ export default {
   },
   watch: {
     diffViewType() {
+      if (this.needsReload() || this.needsFirstLoad()) {
+        this.refetchDiffData();
+      }
       this.adjustView();
     },
     shouldShow() {
@@ -224,6 +228,16 @@ export default {
         { timeout: 1000 },
       );
     },
+    needsReload() {
+      return (
+        this.glFeatures.singleMrDiffView &&
+        this.diffFiles.length &&
+        isSingleViewStyle(this.diffFiles[0])
+      );
+    },
+    needsFirstLoad() {
+      return this.glFeatures.singleMrDiffView && !this.diffFiles.length;
+    },
     fetchData(toggleTree = true) {
       if (this.glFeatures.diffsBatchLoad) {
         this.fetchDiffFilesMeta()
@@ -237,6 +251,13 @@ export default {
           });
 
         this.fetchDiffFilesBatch()
+          .then(() => {
+            // Guarantee the discussions are assigned after the batch finishes.
+            // Just watching the length of the discussions or the diff files
+            // isn't enough, because with split diff loading, neither will
+            // 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!'));
@@ -250,6 +271,7 @@ export default {
 
             requestIdleCallback(
               () => {
+                this.setDiscussions();
                 this.startRenderDiffsQueue();
               },
               { timeout: 1000 },

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

@@ -4,6 +4,7 @@ import _ from 'underscore';
 import { GlLoadingIcon } from '@gitlab/ui';
 import { __, sprintf } from '~/locale';
 import createFlash from '~/flash';
+import { hasDiff } from '~/helpers/diffs_helper';
 import eventHub from '../../notes/event_hub';
 import DiffFileHeader from './diff_file_header.vue';
 import DiffContent from './diff_content.vue';
@@ -55,12 +56,7 @@ export default {
       return this.isLoadingCollapsedDiff || (!this.file.renderIt && !this.isCollapsed);
     },
     hasDiff() {
-      return (
-        (this.file.highlighted_diff_lines &&
-          this.file.parallel_diff_lines &&
-          this.file.parallel_diff_lines.length > 0) ||
-        !this.file.blob.readable_text
-      );
+      return hasDiff(this.file);
     },
     isFileTooLarge() {
       return this.file.viewer.error === diffViewerErrors.too_large;

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

@@ -65,6 +65,10 @@ export const fetchDiffFiles = ({ state, commit }) => {
     w: state.showWhitespace ? '0' : '1',
   };
 
+  if (state.useSingleDiffStyle) {
+    urlParams.view = state.diffViewType;
+  }
+
   commit(types.SET_LOADING, true);
 
   worker.addEventListener('message', ({ data }) => {
@@ -90,13 +94,22 @@ export const fetchDiffFiles = ({ state, commit }) => {
 };
 
 export const fetchDiffFilesBatch = ({ commit, state }) => {
+  const urlParams = {
+    per_page: DIFFS_PER_PAGE,
+    w: state.showWhitespace ? '0' : '1',
+  };
+
+  if (state.useSingleDiffStyle) {
+    urlParams.view = state.diffViewType;
+  }
+
   commit(types.SET_BATCH_LOADING, true);
   commit(types.SET_RETRIEVING_BATCHES, true);
 
   const getBatch = page =>
     axios
       .get(state.endpointBatch, {
-        params: { page, per_page: DIFFS_PER_PAGE, w: state.showWhitespace ? '0' : '1' },
+        params: { ...urlParams, page },
       })
       .then(({ data: { pagination, diff_files } }) => {
         commit(types.SET_DIFF_DATA_BATCH, { diff_files });
@@ -150,7 +163,10 @@ export const assignDiscussionsToDiff = (
   { commit, state, rootState },
   discussions = rootState.notes.discussions,
 ) => {
-  const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles);
+  const diffPositionByLineCode = getDiffPositionByLineCode(
+    state.diffFiles,
+    state.useSingleDiffStyle,
+  );
   const hash = getLocationHash();
 
   discussions
@@ -339,24 +355,23 @@ export const toggleFileDiscussions = ({ getters, dispatch }, diff) => {
 
 export const toggleFileDiscussionWrappers = ({ commit }, diff) => {
   const discussionWrappersExpanded = allDiscussionWrappersExpanded(diff);
-  let linesWithDiscussions;
-  if (diff.highlighted_diff_lines) {
-    linesWithDiscussions = diff.highlighted_diff_lines.filter(line => line.discussions.length);
-  }
-  if (diff.parallel_diff_lines) {
-    linesWithDiscussions = diff.parallel_diff_lines.filter(
-      line =>
-        (line.left && line.left.discussions.length) ||
-        (line.right && line.right.discussions.length),
-    );
-  }
-
-  if (linesWithDiscussions.length) {
-    linesWithDiscussions.forEach(line => {
+  const lineCodesWithDiscussions = new Set();
+  const { parallel_diff_lines: parallelLines, highlighted_diff_lines: inlineLines } = diff;
+  const allLines = inlineLines.concat(
+    parallelLines.map(line => line.left),
+    parallelLines.map(line => line.right),
+  );
+  const lineHasDiscussion = line => Boolean(line?.discussions.length);
+  const registerDiscussionLine = line => lineCodesWithDiscussions.add(line.line_code);
+
+  allLines.filter(lineHasDiscussion).forEach(registerDiscussionLine);
+
+  if (lineCodesWithDiscussions.size) {
+    Array.from(lineCodesWithDiscussions).forEach(lineCode => {
       commit(types.TOGGLE_LINE_DISCUSSIONS, {
         fileHash: diff.file_hash,
-        lineCode: line.line_code,
         expanded: !discussionWrappersExpanded,
+        lineCode,
       });
     });
   }

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

@@ -45,26 +45,28 @@ export default {
   },
 
   [types.SET_DIFF_DATA](state, data) {
+    let files = state.diffFiles;
+
     if (
-      !(
-        gon &&
-        gon.features &&
-        gon.features.diffsBatchLoad &&
-        window.location.search.indexOf('diff_id') === -1
-      )
+      !(gon?.features?.diffsBatchLoad && window.location.search.indexOf('diff_id') === -1) &&
+      data.diff_files
     ) {
-      prepareDiffData(data);
+      files = prepareDiffData(data, files);
     }
 
     Object.assign(state, {
       ...convertObjectPropsToCamelCase(data),
+      diffFiles: files,
     });
   },
 
   [types.SET_DIFF_DATA_BATCH](state, data) {
-    prepareDiffData(data);
+    const files = prepareDiffData(data, state.diffFiles);
 
-    state.diffFiles.push(...data.diff_files);
+    Object.assign(state, {
+      ...convertObjectPropsToCamelCase(data),
+      diffFiles: files,
+    });
   },
 
   [types.RENDER_FILE](state, file) {
@@ -88,11 +90,11 @@ export default {
 
     if (!diffFile) return;
 
-    if (diffFile.highlighted_diff_lines) {
+    if (diffFile.highlighted_diff_lines.length) {
       diffFile.highlighted_diff_lines.find(l => l.line_code === lineCode).hasForm = hasForm;
     }
 
-    if (diffFile.parallel_diff_lines) {
+    if (diffFile.parallel_diff_lines.length) {
       const line = diffFile.parallel_diff_lines.find(l => {
         const { left, right } = l;
 
@@ -153,13 +155,13 @@ export default {
   },
 
   [types.EXPAND_ALL_FILES](state) {
-    state.diffFiles = state.diffFiles.map(file => ({
-      ...file,
-      viewer: {
-        ...file.viewer,
-        collapsed: false,
-      },
-    }));
+    state.diffFiles.forEach(file => {
+      Object.assign(file, {
+        viewer: Object.assign(file.viewer, {
+          collapsed: false,
+        }),
+      });
+    });
   },
 
   [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) {
@@ -197,29 +199,29 @@ export default {
       };
     };
 
-    state.diffFiles = state.diffFiles.map(diffFile => {
-      if (diffFile.file_hash === fileHash) {
-        const file = { ...diffFile };
-
-        if (file.highlighted_diff_lines) {
-          file.highlighted_diff_lines = file.highlighted_diff_lines.map(line =>
-            setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line),
-          );
+    state.diffFiles.forEach(file => {
+      if (file.file_hash === fileHash) {
+        if (file.highlighted_diff_lines.length) {
+          file.highlighted_diff_lines.forEach(line => {
+            Object.assign(
+              line,
+              setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line),
+            );
+          });
         }
 
-        if (file.parallel_diff_lines) {
-          file.parallel_diff_lines = file.parallel_diff_lines.map(line => {
+        if (file.parallel_diff_lines.length) {
+          file.parallel_diff_lines.forEach(line => {
             const left = line.left && lineCheck(line.left);
             const right = line.right && lineCheck(line.right);
 
             if (left || right) {
-              return {
-                ...line,
+              Object.assign(line, {
                 left: line.left ? setDiscussionsExpanded(mapDiscussions(line.left)) : null,
                 right: line.right
                   ? setDiscussionsExpanded(mapDiscussions(line.right, () => !left))
                   : null,
-              };
+              });
             }
 
             return line;
@@ -227,15 +229,15 @@ export default {
         }
 
         if (!file.parallel_diff_lines || !file.highlighted_diff_lines) {
-          file.discussions = (file.discussions || [])
+          const newDiscussions = (file.discussions || [])
             .filter(d => d.id !== discussion.id)
             .concat(discussion);
-        }
 
-        return file;
+          Object.assign(file, {
+            discussions: newDiscussions,
+          });
+        }
       }
-
-      return diffFile;
     });
   },
 
@@ -259,9 +261,9 @@ export default {
   [types.TOGGLE_LINE_DISCUSSIONS](state, { fileHash, lineCode, expanded }) {
     const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash);
 
-    updateLineInFile(selectedFile, lineCode, line =>
-      Object.assign(line, { discussionsExpanded: expanded }),
-    );
+    updateLineInFile(selectedFile, lineCode, line => {
+      Object.assign(line, { discussionsExpanded: expanded });
+    });
   },
 
   [types.TOGGLE_FOLDER_OPEN](state, path) {

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

@@ -185,6 +185,7 @@ export function addContextLines(options) {
  * Trims the first char of the `richText` property when it's either a space or a diff symbol.
  * @param {Object} line
  * @returns {Object}
+ * @deprecated
  */
 export function trimFirstCharOfLineContent(line = {}) {
   // eslint-disable-next-line no-param-reassign
@@ -212,79 +213,171 @@ function getLineCode({ left, right }, index) {
   return index;
 }
 
-// This prepares and optimizes the incoming diff data from the server
-// by setting up incremental rendering and removing unneeded data
-export function prepareDiffData(diffData) {
-  const filesLength = diffData.diff_files.length;
-  let showingLines = 0;
-  for (let i = 0; i < filesLength; i += 1) {
-    const file = diffData.diff_files[i];
-
-    if (file.parallel_diff_lines) {
-      const linesLength = file.parallel_diff_lines.length;
-      for (let u = 0; u < linesLength; u += 1) {
-        const line = file.parallel_diff_lines[u];
-
-        line.line_code = getLineCode(line, u);
-        if (line.left) {
-          line.left = trimFirstCharOfLineContent(line.left);
-          line.left.discussions = [];
-          line.left.hasForm = false;
-        }
-        if (line.right) {
-          line.right = trimFirstCharOfLineContent(line.right);
-          line.right.discussions = [];
-          line.right.hasForm = false;
-        }
+function diffFileUniqueId(file) {
+  return `${file.content_sha}-${file.file_hash}`;
+}
+
+function combineDiffFilesWithPriorFiles(files, prior = []) {
+  files.forEach(file => {
+    const id = diffFileUniqueId(file);
+    const oldMatch = prior.find(oldFile => diffFileUniqueId(oldFile) === id);
+
+    if (oldMatch) {
+      const missingInline = !file.highlighted_diff_lines;
+      const missingParallel = !file.parallel_diff_lines;
+
+      if (missingInline) {
+        Object.assign(file, {
+          highlighted_diff_lines: oldMatch.highlighted_diff_lines,
+        });
       }
-    }
 
-    if (file.highlighted_diff_lines) {
-      const linesLength = file.highlighted_diff_lines.length;
-      for (let u = 0; u < linesLength; u += 1) {
-        const line = file.highlighted_diff_lines[u];
-        Object.assign(line, {
-          ...trimFirstCharOfLineContent(line),
-          discussions: [],
-          hasForm: false,
+      if (missingParallel) {
+        Object.assign(file, {
+          parallel_diff_lines: oldMatch.parallel_diff_lines,
         });
       }
-      showingLines += file.parallel_diff_lines.length;
+    }
+  });
+
+  return files;
+}
+
+function ensureBasicDiffFileLines(file) {
+  const missingInline = !file.highlighted_diff_lines;
+  const missingParallel = !file.parallel_diff_lines;
+
+  Object.assign(file, {
+    highlighted_diff_lines: missingInline ? [] : file.highlighted_diff_lines,
+    parallel_diff_lines: missingParallel ? [] : file.parallel_diff_lines,
+  });
+
+  return file;
+}
+
+function cleanRichText(text) {
+  return text ? text.replace(/^[+ -]/, '') : undefined;
+}
+
+function prepareLine(line) {
+  return Object.assign(line, {
+    rich_text: cleanRichText(line.rich_text),
+    discussionsExpanded: true,
+    discussions: [],
+    hasForm: false,
+    text: undefined,
+  });
+}
+
+function prepareDiffFileLines(file) {
+  const inlineLines = file.highlighted_diff_lines;
+  const parallelLines = file.parallel_diff_lines;
+  let parallelLinesCount = 0;
+
+  inlineLines.forEach(prepareLine);
+
+  parallelLines.forEach((line, index) => {
+    Object.assign(line, { line_code: getLineCode(line, index) });
+
+    if (line.left) {
+      parallelLinesCount += 1;
+      prepareLine(line.left);
     }
 
-    const name = (file.viewer && file.viewer.name) || diffViewerModes.text;
+    if (line.right) {
+      parallelLinesCount += 1;
+      prepareLine(line.right);
+    }
 
     Object.assign(file, {
-      renderIt: showingLines < LINES_TO_BE_RENDERED_DIRECTLY,
-      collapsed: name === diffViewerModes.text && showingLines > MAX_LINES_TO_BE_RENDERED,
-      isShowingFullFile: false,
-      isLoadingFullFile: false,
-      discussions: [],
-      renderingLines: false,
+      inlineLinesCount: inlineLines.length,
+      parallelLinesCount,
     });
-  }
+  });
+
+  return file;
 }
 
-export function getDiffPositionByLineCode(diffFiles) {
-  return diffFiles.reduce((acc, diffFile) => {
-    // We can only use highlightedDiffLines to create the map of diff lines because
-    // highlightedDiffLines will also include every parallel diff line in it.
-    if (diffFile.highlighted_diff_lines) {
+function getVisibleDiffLines(file) {
+  return Math.max(file.inlineLinesCount, file.parallelLinesCount);
+}
+
+function finalizeDiffFile(file) {
+  const name = (file.viewer && file.viewer.name) || diffViewerModes.text;
+  const lines = getVisibleDiffLines(file);
+
+  Object.assign(file, {
+    renderIt: lines < LINES_TO_BE_RENDERED_DIRECTLY,
+    collapsed: name === diffViewerModes.text && lines > MAX_LINES_TO_BE_RENDERED,
+    isShowingFullFile: false,
+    isLoadingFullFile: false,
+    discussions: [],
+    renderingLines: false,
+  });
+
+  return file;
+}
+
+export function prepareDiffData(diffData, priorFiles) {
+  return combineDiffFilesWithPriorFiles(diffData.diff_files, priorFiles)
+    .map(ensureBasicDiffFileLines)
+    .map(prepareDiffFileLines)
+    .map(finalizeDiffFile);
+}
+
+export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) {
+  let lines = [];
+  const hasInlineDiffs = diffFiles.some(file => file.highlighted_diff_lines.length > 0);
+
+  if (!useSingleDiffStyle || hasInlineDiffs) {
+    // In either of these cases, we can use `highlighted_diff_lines` because
+    // that will include all of the parallel diff lines, too
+
+    lines = diffFiles.reduce((acc, diffFile) => {
       diffFile.highlighted_diff_lines.forEach(line => {
-        if (line.line_code) {
-          acc[line.line_code] = {
-            base_sha: diffFile.diff_refs.base_sha,
-            head_sha: diffFile.diff_refs.head_sha,
-            start_sha: diffFile.diff_refs.start_sha,
-            new_path: diffFile.new_path,
-            old_path: diffFile.old_path,
-            old_line: line.old_line,
-            new_line: line.new_line,
-            line_code: line.line_code,
-            position_type: 'text',
-          };
+        acc.push({ file: diffFile, line });
+      });
+
+      return acc;
+    }, []);
+  } else {
+    // If we're in single diff view mode and the inline lines haven't been
+    // loaded yet, we need to parse the parallel lines
+
+    lines = diffFiles.reduce((acc, diffFile) => {
+      diffFile.parallel_diff_lines.forEach(pair => {
+        // It's possible for a parallel line to have an opposite line that doesn't exist
+        // For example: *deleted* lines will have `null` right lines, while
+        // *added* lines will have `null` left lines.
+        // So we have to check each line before we push it onto the array so we're not
+        // pushing null line diffs
+
+        if (pair.left) {
+          acc.push({ file: diffFile, line: pair.left });
+        }
+
+        if (pair.right) {
+          acc.push({ file: diffFile, line: pair.right });
         }
       });
+
+      return acc;
+    }, []);
+  }
+
+  return lines.reduce((acc, { file, line }) => {
+    if (line.line_code) {
+      acc[line.line_code] = {
+        base_sha: file.diff_refs.base_sha,
+        head_sha: file.diff_refs.head_sha,
+        start_sha: file.diff_refs.start_sha,
+        new_path: file.new_path,
+        old_path: file.old_path,
+        old_line: line.old_line,
+        new_line: line.new_line,
+        line_code: line.line_code,
+        position_type: 'text',
+      };
     }
 
     return acc;
@@ -462,47 +555,47 @@ export const convertExpandLines = ({
 
 export const idleCallback = cb => requestIdleCallback(cb);
 
-export const updateLineInFile = (selectedFile, lineCode, updateFn) => {
-  if (selectedFile.parallel_diff_lines) {
-    const targetLine = selectedFile.parallel_diff_lines.find(
-      line =>
-        (line.left && line.left.line_code === lineCode) ||
-        (line.right && line.right.line_code === lineCode),
-    );
-    if (targetLine) {
-      const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right';
+function getLinesFromFileByLineCode(file, lineCode) {
+  const parallelLines = file.parallel_diff_lines;
+  const inlineLines = file.highlighted_diff_lines;
+  const matchesCode = line => line.line_code === lineCode;
 
-      updateFn(targetLine[side]);
-    }
-  }
-  if (selectedFile.highlighted_diff_lines) {
-    const targetInlineLine = selectedFile.highlighted_diff_lines.find(
-      line => line.line_code === lineCode,
-    );
+  return [
+    ...parallelLines.reduce((acc, line) => {
+      if (line.left) {
+        acc.push(line.left);
+      }
 
-    if (targetInlineLine) {
-      updateFn(targetInlineLine);
-    }
-  }
+      if (line.right) {
+        acc.push(line.right);
+      }
+
+      return acc;
+    }, []),
+    ...inlineLines,
+  ].filter(matchesCode);
+}
+
+export const updateLineInFile = (selectedFile, lineCode, updateFn) => {
+  getLinesFromFileByLineCode(selectedFile, lineCode).forEach(updateFn);
 };
 
 export const allDiscussionWrappersExpanded = diff => {
-  const discussionsExpandedArray = [];
-  if (diff.parallel_diff_lines) {
-    diff.parallel_diff_lines.forEach(line => {
-      if (line.left && line.left.discussions.length) {
-        discussionsExpandedArray.push(line.left.discussionsExpanded);
-      }
-      if (line.right && line.right.discussions.length) {
-        discussionsExpandedArray.push(line.right.discussionsExpanded);
-      }
-    });
-  } else if (diff.highlighted_diff_lines) {
-    diff.highlighted_diff_lines.forEach(line => {
-      if (line.discussions.length) {
-        discussionsExpandedArray.push(line.discussionsExpanded);
-      }
-    });
-  }
-  return discussionsExpandedArray.every(el => el);
+  let discussionsExpanded = true;
+  const changeExpandedResult = line => {
+    if (line && line.discussions.length) {
+      discussionsExpanded = discussionsExpanded && line.discussionsExpanded;
+    }
+  };
+
+  diff.parallel_diff_lines.forEach(line => {
+    changeExpandedResult(line.left);
+    changeExpandedResult(line.right);
+  });
+
+  diff.highlighted_diff_lines.forEach(line => {
+    changeExpandedResult(line);
+  });
+
+  return discussionsExpanded;
 };

app/assets/javascripts/helpers/diffs_helper.js

+export function hasInlineLines(diffFile) {
+  return diffFile?.highlighted_diff_lines?.length > 0; /* eslint-disable-line camelcase */
+}
+
+export function hasParallelLines(diffFile) {
+  return diffFile?.parallel_diff_lines?.length > 0; /* eslint-disable-line camelcase */
+}
+
+export function isSingleViewStyle(diffFile) {
+  return !hasParallelLines(diffFile) || !hasInlineLines(diffFile);
+}
+
+export function hasDiff(diffFile) {
+  return (
+    hasInlineLines(diffFile) ||
+    hasParallelLines(diffFile) ||
+    !diffFile?.blob?.readable_text /* eslint-disable-line camelcase */
+  );
+}