diff --git a/app/assets/javascripts/diffs/components/parallel_diff_view.vue b/app/assets/javascripts/diffs/components/parallel_diff_view.vue
index d7280338b6897ccc65d49d2dadff6604d8484b5e..52561e197e6ded119f0d0707544b34148394a5d8 100644
--- a/app/assets/javascripts/diffs/components/parallel_diff_view.vue
+++ b/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() {
diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js
index da7ae16aaf135b217efa8a4d2cfef4ba11a81377..d9589baa76ebae77328111a2e6fb5b62a82fd27a 100644
--- a/app/assets/javascripts/diffs/store/utils.js
+++ b/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;
}
diff --git a/app/assets/javascripts/notes/components/diff_with_note.vue b/app/assets/javascripts/notes/components/diff_with_note.vue
index d321f2ce15e12dab8928ddd5a979b6c572a9a5fc..9c2908c477ea2630457abf68c20f681ab4474ada 100644
--- a/app/assets/javascripts/notes/components/diff_with_note.vue
+++ b/app/assets/javascripts/notes/components/diff_with_note.vue
@@ -1,89 +1,94 @@
<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>
diff --git a/changelogs/unreleased/48825-performance.yml b/changelogs/unreleased/48825-performance.yml
new file mode 100644
index 0000000000000000000000000000000000000000..428852f6f8bc8d884d3b80c2a572bb0bd3461c75
--- /dev/null
+++ b/changelogs/unreleased/48825-performance.yml
@@ -0,0 +1,8 @@
+---
+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
diff --git a/spec/javascripts/diffs/store/utils_spec.js b/spec/javascripts/diffs/store/utils_spec.js
index 5a024a0f2adae86c1ea88aee98315207038e5495..32136d9ebffcaf133bf1559adec1f271d77997f2 100644
--- a/spec/javascripts/diffs/store/utils_spec.js
+++ b/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({});
+ });
+ });
});