Merge branch '48825-performance' into 'master'

Improves performance on MR refactor and adds specs

Closes #48825

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

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

@@ -24,19 +24,21 @@ export default {
     ...mapGetters(['commit']),
     parallelDiffLines() {
       return this.diffLines.map(line => {
+        const parallelLine = Object.assign({}, line);
+
         if (line.left) {
-          Object.assign(line, { left: trimFirstCharOfLineContent(line.left) });
+          parallelLine.left = trimFirstCharOfLineContent(line.left);
         } else {
-          Object.assign(line, { left: { type: EMPTY_CELL_TYPE } });
+          parallelLine.left = { type: EMPTY_CELL_TYPE };
         }
 
         if (line.right) {
-          Object.assign(line, { right: trimFirstCharOfLineContent(line.right) });
+          parallelLine.right = trimFirstCharOfLineContent(line.right);
         } else {
-          Object.assign(line, { right: { type: EMPTY_CELL_TYPE } });
+          parallelLine.right = { type: EMPTY_CELL_TYPE };
         }
 
-        return line;
+        return parallelLine;
       });
     },
     diffLinesLength() {

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

@@ -155,18 +155,21 @@ export function addContextLines(options) {
   }
 }
 
-export function trimFirstCharOfLineContent(line) {
-  if (!line.richText) {
-    return line;
-  }
-
-  const firstChar = line.richText.charAt(0);
-
-  if (firstChar === ' ' || firstChar === '+' || firstChar === '-') {
-    Object.assign(line, {
-      richText: line.richText.substring(1),
-    });
+/**
+ * Trims the first char of the `richText` property when it's either a space or a diff symbol.
+ * @param {Object} line
+ * @returns {Object}
+ */
+export function trimFirstCharOfLineContent(line = {}) {
+  const parsedLine = Object.assign({}, line);
+
+  if (line.richText) {
+    const firstChar = parsedLine.richText.charAt(0);
+
+    if (firstChar === ' ' || firstChar === '+' || firstChar === '-') {
+      parsedLine.richText = line.richText.substring(1);
+    }
   }
 
-  return line;
+  return parsedLine;
 }

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

 <script>
-import { mapState, mapActions } from 'vuex';
-import imageDiffHelper from '~/image_diff/helpers/index';
-import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
-import DiffFileHeader from '~/diffs/components/diff_file_header.vue';
-import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue';
-import { trimFirstCharOfLineContent } from '~/diffs/store/utils';
+  import { mapState, mapActions } from 'vuex';
+  import imageDiffHelper from '~/image_diff/helpers/index';
+  import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
+  import DiffFileHeader from '~/diffs/components/diff_file_header.vue';
+  import SkeletonLoadingContainer from '~/vue_shared/components/skeleton_loading_container.vue';
+  import { trimFirstCharOfLineContent } from '~/diffs/store/utils';
 
-export default {
-  components: {
-    DiffFileHeader,
-    SkeletonLoadingContainer,
-  },
-  props: {
-    discussion: {
-      type: Object,
-      required: true,
+  export default {
+    components: {
+      DiffFileHeader,
+      SkeletonLoadingContainer,
     },
-  },
-  data() {
-    return {
-      error: false,
-    };
-  },
-  computed: {
-    ...mapState({
-      noteableData: state => state.notes.noteableData,
-    }),
-    hasTruncatedDiffLines() {
-      return this.discussion.truncatedDiffLines && this.discussion.truncatedDiffLines.length !== 0;
+    props: {
+      discussion: {
+        type: Object,
+        required: true,
+      },
     },
-    isDiscussionsExpanded() {
-      return true; // TODO: @fatihacet - Fix this.
+    data() {
+      return {
+        error: false,
+      };
     },
-    isCollapsed() {
-      return this.diffFile.collapsed || false;
-    },
-    isImageDiff() {
-      return !this.diffFile.text;
-    },
-    diffFileClass() {
-      const { text } = this.diffFile;
-      return text ? 'text-file' : 'js-image-file';
-    },
-    diffFile() {
-      return convertObjectPropsToCamelCase(this.discussion.diffFile, { deep: true });
-    },
-    imageDiffHtml() {
-      return this.discussion.imageDiffHtml;
-    },
-    currentUser() {
-      return this.noteableData.current_user;
-    },
-    userColorScheme() {
-      return window.gon.user_color_scheme;
-    },
-    normalizedDiffLines() {
-      const lines = this.discussion.truncatedDiffLines || [];
+    computed: {
+      ...mapState({
+        noteableData: state => state.notes.noteableData,
+      }),
+      hasTruncatedDiffLines() {
+        return this.discussion.truncatedDiffLines &&
+          this.discussion.truncatedDiffLines.length !== 0;
+      },
+      isDiscussionsExpanded() {
+        return true; // TODO: @fatihacet - Fix this.
+      },
+      isCollapsed() {
+        return this.diffFile.collapsed || false;
+      },
+      isImageDiff() {
+        return !this.diffFile.text;
+      },
+      diffFileClass() {
+        const { text } = this.diffFile;
+        return text ? 'text-file' : 'js-image-file';
+      },
+      diffFile() {
+        return convertObjectPropsToCamelCase(this.discussion.diffFile, { deep: true });
+      },
+      imageDiffHtml() {
+        return this.discussion.imageDiffHtml;
+      },
+      currentUser() {
+        return this.noteableData.current_user;
+      },
+      userColorScheme() {
+        return window.gon.user_color_scheme;
+      },
+      normalizedDiffLines() {
+        if (this.discussion.truncatedDiffLines) {
+          return this.discussion.truncatedDiffLines.map(line =>
+            trimFirstCharOfLineContent(convertObjectPropsToCamelCase(line)),
+          );
+        }
 
-      return lines.map(line => trimFirstCharOfLineContent(convertObjectPropsToCamelCase(line)));
+        return [];
+      },
     },
-  },
-  mounted() {
-    if (this.isImageDiff) {
-      const canCreateNote = false;
-      const renderCommentBadge = true;
-      imageDiffHelper.initImageDiff(this.$refs.fileHolder, canCreateNote, renderCommentBadge);
-    } else if (!this.hasTruncatedDiffLines) {
-      this.fetchDiff();
-    }
-  },
-  methods: {
-    ...mapActions(['fetchDiscussionDiffLines']),
-    rowTag(html) {
-      return html.outerHTML ? 'tr' : 'template';
+    mounted() {
+      if (this.isImageDiff) {
+        const canCreateNote = false;
+        const renderCommentBadge = true;
+        imageDiffHelper.initImageDiff(this.$refs.fileHolder, canCreateNote, renderCommentBadge);
+      } else if (!this.hasTruncatedDiffLines) {
+        this.fetchDiff();
+      }
     },
-    fetchDiff() {
-      this.error = false;
-      this.fetchDiscussionDiffLines(this.discussion)
-        .then(this.highlight)
-        .catch(() => {
-          this.error = true;
-        });
+    methods: {
+      ...mapActions(['fetchDiscussionDiffLines']),
+      rowTag(html) {
+        return html.outerHTML ? 'tr' : 'template';
+      },
+      fetchDiff() {
+        this.error = false;
+        this.fetchDiscussionDiffLines(this.discussion)
+          .then(this.highlight)
+          .catch(() => {
+            this.error = true;
+          });
+      },
     },
-  },
-};
+  };
 </script>
 
 <template>

changelogs/unreleased/48825-performance.yml

+---
+title: Improves performance of mr code, by fixing the state being mutated outside
+  of the store in the util function trimFirstCharOfLineContent and in map operations.
+  Avoids map operation in an empty array. Adds specs to the trimFirstCharOfLineContent
+  function
+merge_request: 20380
+author: filipa
+type: performance

spec/javascripts/diffs/store/utils_spec.js

@@ -176,4 +176,35 @@ describe('DiffsStoreUtils', () => {
       expect(linesWithReferences[1].metaData.newPos).toEqual(3);
     });
   });
+
+  describe('trimFirstCharOfLineContent', () => {
+    it('trims the line when it starts with a space', () => {
+      expect(utils.trimFirstCharOfLineContent({ richText: ' diff' })).toEqual({ richText: 'diff' });
+    });
+
+    it('trims the line when it starts with a +', () => {
+      expect(utils.trimFirstCharOfLineContent({ richText: '+diff' })).toEqual({ richText: 'diff' });
+    });
+
+    it('trims the line when it starts with a -', () => {
+      expect(utils.trimFirstCharOfLineContent({ richText: '-diff' })).toEqual({ richText: 'diff' });
+    });
+
+    it('does not trims the line when it starts with a letter', () => {
+      expect(utils.trimFirstCharOfLineContent({ richText: 'diff' })).toEqual({ richText: 'diff' });
+    });
+
+    it('does not modify the provided object', () => {
+      const lineObj = {
+        richText: ' diff',
+      };
+
+      utils.trimFirstCharOfLineContent(lineObj);
+      expect(lineObj).toEqual({ richText: ' diff' });
+    });
+
+    it('handles a undefined or null parameter', () => {
+      expect(utils.trimFirstCharOfLineContent()).toEqual({});
+    });
+  });
 });