Merge branch 'discussion-perf-fixes' into 'master'

Improve discussion rendering performance

Closes #51506

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

11 files changed, 170 insertions, 165 deletions

diff --git a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
index 87613a4fdce..328f96e6ed7 100644
--- a/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
+++ b/spec/features/merge_request/user_resolves_diff_notes_and_discussions_resolve_spec.rb
@@ -50,7 +50,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
           find('.line-resolve-btn').click
 
           expect(page).to have_selector('.line-resolve-btn.is-active')
-          expect(find('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}")
+          expect(find('.line-resolve-btn')['aria-label']).to eq("Resolved by #{user.name}")
         end
 
         page.within '.diff-content' do
@@ -243,7 +243,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
           resolve_button.click
           wait_for_requests
 
-          expect(resolve_button['data-original-title']).to eq("Resolved by #{user.name}")
+          expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}")
         end
       end
 
@@ -266,7 +266,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
 
           wait_for_requests
 
-          expect(first('.line-resolve-btn')['data-original-title']).to eq("Resolved by #{user.name}")
+          expect(first('.line-resolve-btn')['aria-label']).to eq("Resolved by #{user.name}")
         end
 
         expect(page).to have_content('Last updated')
@@ -285,7 +285,7 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
         wait_for_requests
 
         resolve_buttons.each do |button|
-          expect(button['data-original-title']).to eq("Resolved by #{user.name}")
+          expect(button['aria-label']).to eq("Resolved by #{user.name}")
         end
 
         page.within '.line-resolve-all-container' do
@@ -357,13 +357,12 @@ describe 'Merge request > User resolves diff notes and discussions', :js do
           resolve_button.click
           wait_for_requests
 
-          expect(resolve_button['data-original-title']).to eq("Resolved by #{user.name}")
+          expect(resolve_button['aria-label']).to eq("Resolved by #{user.name}")
         end
       end
 
-      it 'shows jump to next discussion button, apart from the last one' do
-        expect(page).to have_selector('.discussion-reply-holder', count: 2)
-        expect(page).to have_selector('.discussion-reply-holder .discussion-next-btn', count: 1)
+      it 'shows jump to next discussion button' do
+        expect(page.all('.discussion-reply-holder', count: 2)).to all(have_selector('.discussion-next-btn'))
       end
 
       it 'displays next discussion even if hidden' do
diff --git a/spec/javascripts/diffs/components/diff_file_header_spec.js b/spec/javascripts/diffs/components/diff_file_header_spec.js
index 9530b50c729..b77907ff26f 100644
--- a/spec/javascripts/diffs/components/diff_file_header_spec.js
+++ b/spec/javascripts/diffs/components/diff_file_header_spec.js
@@ -464,7 +464,11 @@ describe('diff_file_header', () => {
           propsCopy.addMergeRequestButtons = true;
           propsCopy.diffFile.deleted_file = true;
 
-          const discussionGetter = () => [diffDiscussionMock];
+          const discussionGetter = () => [
+            {
+              ...diffDiscussionMock,
+            },
+          ];
           const notesModuleMock = notesModule();
           notesModuleMock.getters.discussions = discussionGetter;
           vm = mountComponentWithStore(Component, {
diff --git a/spec/javascripts/diffs/components/diff_line_note_form_spec.js b/spec/javascripts/diffs/components/diff_line_note_form_spec.js
index 81b66cf7c9b..b983dc35a57 100644
--- a/spec/javascripts/diffs/components/diff_line_note_form_spec.js
+++ b/spec/javascripts/diffs/components/diff_line_note_form_spec.js
@@ -62,6 +62,7 @@ describe('DiffLineNoteForm', () => {
         component.$nextTick(() => {
           expect(component.cancelCommentForm).toHaveBeenCalledWith({
             lineCode: diffLines[0].line_code,
+            fileHash: component.diffFileHash,
           });
 
           expect(component.resetAutoSave).toHaveBeenCalled();
diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js
index acd95a3dd8b..43d8d950bed 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/spec/javascripts/diffs/store/actions_spec.js
@@ -310,13 +310,13 @@ describe('DiffsStoreActions', () => {
 
   describe('showCommentForm', () => {
     it('should call mutation to show comment form', done => {
-      const payload = { lineCode: 'lineCode' };
+      const payload = { lineCode: 'lineCode', fileHash: 'hash' };
 
       testAction(
         showCommentForm,
         payload,
         {},
-        [{ type: types.ADD_COMMENT_FORM_LINE, payload }],
+        [{ type: types.TOGGLE_LINE_HAS_FORM, payload: { ...payload, hasForm: true } }],
         [],
         done,
       );
@@ -325,13 +325,13 @@ describe('DiffsStoreActions', () => {
 
   describe('cancelCommentForm', () => {
     it('should call mutation to cancel comment form', done => {
-      const payload = { lineCode: 'lineCode' };
+      const payload = { lineCode: 'lineCode', fileHash: 'hash' };
 
       testAction(
         cancelCommentForm,
         payload,
         {},
-        [{ type: types.REMOVE_COMMENT_FORM_LINE, payload }],
+        [{ type: types.TOGGLE_LINE_HAS_FORM, payload: { ...payload, hasForm: false } }],
         [],
         done,
       );
diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js
index eef95c823fb..582535e0a53 100644
--- a/spec/javascripts/diffs/store/getters_spec.js
+++ b/spec/javascripts/diffs/store/getters_spec.js
@@ -186,77 +186,6 @@ describe('Diffs Module Getters', () => {
     });
   });
 
-  describe('shouldRenderParallelCommentRow', () => {
-    let line;
-
-    beforeEach(() => {
-      line = {};
-
-      discussionMock.expanded = true;
-
-      line.left = {
-        line_code: 'ABC',
-        discussions: [discussionMock],
-      };
-
-      line.right = {
-        line_code: 'DEF',
-        discussions: [discussionMock1],
-      };
-    });
-
-    it('returns true when discussion is expanded', () => {
-      expect(getters.shouldRenderParallelCommentRow(localState)(line)).toEqual(true);
-    });
-
-    it('returns false when no discussion was found', () => {
-      line.left.discussions = [];
-      line.right.discussions = [];
-
-      localState.diffLineCommentForms.ABC = false;
-      localState.diffLineCommentForms.DEF = false;
-
-      expect(getters.shouldRenderParallelCommentRow(localState)(line)).toEqual(false);
-    });
-
-    it('returns true when discussionForm was found', () => {
-      localState.diffLineCommentForms.ABC = {};
-
-      expect(getters.shouldRenderParallelCommentRow(localState)(line)).toEqual(true);
-    });
-  });
-
-  describe('shouldRenderInlineCommentRow', () => {
-    let line;
-
-    beforeEach(() => {
-      discussionMock.expanded = true;
-
-      line = {
-        lineCode: 'ABC',
-        discussions: [discussionMock],
-      };
-    });
-
-    it('returns true when diffLineCommentForms has form', () => {
-      localState.diffLineCommentForms.ABC = {};
-
-      expect(getters.shouldRenderInlineCommentRow(localState)(line)).toEqual(true);
-    });
-
-    it('returns false when no line discussions were found', () => {
-      line.discussions = [];
-
-      expect(getters.shouldRenderInlineCommentRow(localState)(line)).toEqual(false);
-    });
-
-    it('returns true if all found discussions are expanded', () => {
-      discussionMock.expanded = true;
-
-      expect(getters.shouldRenderInlineCommentRow(localState)(line)).toEqual(true);
-    });
-  });
-
   describe('getDiffFileDiscussions', () => {
     it('returns an array with discussions when fileHash matches and the discussion belongs to a diff', () => {
       discussionMock.diff_file.file_hash = diffFileMock.file_hash;
diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js
index 598d723c940..d1040ace5ca 100644
--- a/spec/javascripts/diffs/store/mutations_spec.js
+++ b/spec/javascripts/diffs/store/mutations_spec.js
@@ -55,32 +55,6 @@ describe('DiffsStoreMutations', () => {
     });
   });
 
-  describe('ADD_COMMENT_FORM_LINE', () => {
-    it('should set a truthy reference for the given line code in diffLineCommentForms', () => {
-      const state = { diffLineCommentForms: {} };
-      const lineCode = 'FDE';
-
-      mutations[types.ADD_COMMENT_FORM_LINE](state, { lineCode });
-
-      expect(state.diffLineCommentForms[lineCode]).toBeTruthy();
-    });
-  });
-
-  describe('REMOVE_COMMENT_FORM_LINE', () => {
-    it('should remove given reference from diffLineCommentForms', () => {
-      const state = { diffLineCommentForms: {} };
-      const lineCode = 'FDE';
-
-      mutations[types.ADD_COMMENT_FORM_LINE](state, { lineCode });
-
-      expect(state.diffLineCommentForms[lineCode]).toBeTruthy();
-
-      mutations[types.REMOVE_COMMENT_FORM_LINE](state, { lineCode });
-
-      expect(state.diffLineCommentForms[lineCode]).toBeUndefined();
-    });
-  });
-
   describe('EXPAND_ALL_FILES', () => {
     it('should change the collapsed prop from diffFiles', () => {
       const diffFile = {
@@ -98,7 +72,9 @@ describe('DiffsStoreMutations', () => {
     it('should call utils.addContextLines with proper params', () => {
       const options = {
         lineNumbers: { oldLineNumber: 1, newLineNumber: 2 },
-        contextLines: [{ old_line: 1, new_line: 1, line_code: 'ff9200_1_1', discussions: [] }],
+        contextLines: [
+          { old_line: 1, new_line: 1, line_code: 'ff9200_1_1', discussions: [], hasForm: false },
+        ],
         fileHash: 'ff9200',
         params: {
           bottom: true,
@@ -383,4 +359,35 @@ describe('DiffsStoreMutations', () => {
       expect(state.currentDiffFileId).toBe('somefileid');
     });
   });
+
+  describe('TOGGLE_LINE_HAS_FORM', () => {
+    it('sets hasForm on lines', () => {
+      const file = {
+        file_hash: 'hash',
+        parallel_diff_lines: [
+          { left: { line_code: '123', hasForm: false }, right: {} },
+          { left: {}, right: { line_code: '124', hasForm: false } },
+        ],
+        highlighted_diff_lines: [
+          { line_code: '123', hasForm: false },
+          { line_code: '124', hasForm: false },
+        ],
+      };
+      const state = {
+        diffFiles: [file],
+      };
+
+      mutations[types.TOGGLE_LINE_HAS_FORM](state, {
+        lineCode: '123',
+        hasForm: true,
+        fileHash: 'hash',
+      });
+
+      expect(file.highlighted_diff_lines[0].hasForm).toBe(true);
+      expect(file.highlighted_diff_lines[1].hasForm).toBe(false);
+
+      expect(file.parallel_diff_lines[0].left.hasForm).toBe(true);
+      expect(file.parallel_diff_lines[1].right.hasForm).toBe(false);
+    });
+  });
 });
diff --git a/spec/javascripts/notes/components/diff_with_note_spec.js b/spec/javascripts/notes/components/diff_with_note_spec.js
index 95461396f10..0752bd05904 100644
--- a/spec/javascripts/notes/components/diff_with_note_spec.js
+++ b/spec/javascripts/notes/components/diff_with_note_spec.js
@@ -17,7 +17,7 @@ describe('diff_with_note', () => {
   };
   const selectors = {
     get container() {
-      return vm.$refs.fileHolder;
+      return vm.$el;
     },
     get diffTable() {
       return this.container.querySelector('.diff-content table');
@@ -70,7 +70,6 @@ describe('diff_with_note', () => {
     it('shows image diff', () => {
       vm = mountComponentWithStore(Component, { props, store });
 
-      expect(selectors.container).toHaveClass('js-image-file');
       expect(selectors.diffTable).not.toExist();
     });
   });
diff --git a/spec/javascripts/notes/components/noteable_discussion_spec.js b/spec/javascripts/notes/components/noteable_discussion_spec.js
index 4b4403689d9..76e9cd03d2d 100644
--- a/spec/javascripts/notes/components/noteable_discussion_spec.js
+++ b/spec/javascripts/notes/components/noteable_discussion_spec.js
@@ -80,43 +80,6 @@ describe('noteable_discussion component', () => {
   });
 
   describe('computed', () => {
-    describe('hasMultipleUnresolvedDiscussions', () => {
-      it('is false if there are no unresolved discussions', done => {
-        spyOnProperty(vm, 'unresolvedDiscussions').and.returnValue([]);
-
-        Vue.nextTick()
-          .then(() => {
-            expect(vm.hasMultipleUnresolvedDiscussions).toBe(false);
-          })
-          .then(done)
-          .catch(done.fail);
-      });
-
-      it('is false if there is one unresolved discussion', done => {
-        spyOnProperty(vm, 'unresolvedDiscussions').and.returnValue([discussionMock]);
-
-        Vue.nextTick()
-          .then(() => {
-            expect(vm.hasMultipleUnresolvedDiscussions).toBe(false);
-          })
-          .then(done)
-          .catch(done.fail);
-      });
-
-      it('is true if there are two unresolved discussions', done => {
-        const discussion = getJSONFixture(discussionWithTwoUnresolvedNotes)[0];
-        discussion.notes[0].resolved = false;
-        vm.$store.dispatch('setInitialNotes', [discussion, discussion]);
-
-        Vue.nextTick()
-          .then(() => {
-            expect(vm.hasMultipleUnresolvedDiscussions).toBe(true);
-          })
-          .then(done)
-          .catch(done.fail);
-      });
-    });
-
     describe('isRepliesCollapsed', () => {
       it('should return false for diff discussions', done => {
         const diffDiscussion = getJSONFixture(diffDiscussionFixture)[0];
diff --git a/spec/javascripts/notes/stores/actions_spec.js b/spec/javascripts/notes/stores/actions_spec.js
index fcdd834e4a0..24c2b3e6570 100644
--- a/spec/javascripts/notes/stores/actions_spec.js
+++ b/spec/javascripts/notes/stores/actions_spec.js
@@ -1,4 +1,5 @@
 import Vue from 'vue';
+import $ from 'jquery';
 import _ from 'underscore';
 import { headersInterceptor } from 'spec/helpers/vue_resource_helper';
 import * as actions from '~/notes/stores/actions';
@@ -330,10 +331,14 @@ describe('Actions Notes Store', () => {
 
     beforeEach(() => {
       Vue.http.interceptors.push(interceptor);
+
+      $('body').attr('data-page', '');
     });
 
     afterEach(() => {
       Vue.http.interceptors = _.without(Vue.http.interceptors, interceptor);
+
+      $('body').attr('data-page', '');
     });
 
     it('commits DELETE_NOTE and dispatches updateMergeRequestWidget', done => {
@@ -353,6 +358,39 @@ describe('Actions Notes Store', () => {
           {
             type: 'updateMergeRequestWidget',
           },
+          {
+            type: 'updateResolvableDiscussonsCounts',
+          },
+        ],
+        done,
+      );
+    });
+
+    it('dispatches removeDiscussionsFromDiff on merge request page', done => {
+      const note = { path: `${gl.TEST_HOST}`, id: 1 };
+
+      $('body').attr('data-page', 'projects:merge_requests:show');
+
+      testAction(
+        actions.deleteNote,
+        note,
+        store.state,
+        [
+          {
+            type: 'DELETE_NOTE',
+            payload: note,
+          },
+        ],
+        [
+          {
+            type: 'updateMergeRequestWidget',
+          },
+          {
+            type: 'updateResolvableDiscussonsCounts',
+          },
+          {
+            type: 'diffs/removeDiscussionsFromDiff',
+          },
         ],
         done,
       );
@@ -399,6 +437,9 @@ describe('Actions Notes Store', () => {
             {
               type: 'startTaskList',
             },
+            {
+              type: 'updateResolvableDiscussonsCounts',
+            },
           ],
           done,
         );
@@ -472,6 +513,9 @@ describe('Actions Notes Store', () => {
           ],
           [
             {
+              type: 'updateResolvableDiscussonsCounts',
+            },
+            {
               type: 'updateMergeRequestWidget',
             },
           ],
@@ -494,6 +538,9 @@ describe('Actions Notes Store', () => {
           ],
           [
             {
+              type: 'updateResolvableDiscussonsCounts',
+            },
+            {
               type: 'updateMergeRequestWidget',
             },
           ],
@@ -525,4 +572,17 @@ describe('Actions Notes Store', () => {
       );
     });
   });
+
+  describe('updateResolvableDiscussonsCounts', () => {
+    it('commits UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS', done => {
+      testAction(
+        actions.updateResolvableDiscussonsCounts,
+        null,
+        {},
+        [{ type: 'UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS' }],
+        [],
+        done,
+      );
+    });
+  });
 });
diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js
index f853f9ff088..c066975a43b 100644
--- a/spec/javascripts/notes/stores/getters_spec.js
+++ b/spec/javascripts/notes/stores/getters_spec.js
@@ -117,17 +117,15 @@ describe('Getters Notes Store', () => {
 
   describe('allResolvableDiscussions', () => {
     it('should return only resolvable discussions in same order', () => {
-      const localGetters = {
-        allDiscussions: [
-          discussion3,
-          unresolvableDiscussion,
-          discussion1,
-          unresolvableDiscussion,
-          discussion2,
-        ],
-      };
+      state.discussions = [
+        discussion3,
+        unresolvableDiscussion,
+        discussion1,
+        unresolvableDiscussion,
+        discussion2,
+      ];
 
-      expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([
+      expect(getters.allResolvableDiscussions(state)).toEqual([
         discussion3,
         discussion1,
         discussion2,
@@ -135,11 +133,9 @@ describe('Getters Notes Store', () => {
     });
 
     it('should return empty array if there are no resolvable discussions', () => {
-      const localGetters = {
-        allDiscussions: [unresolvableDiscussion, unresolvableDiscussion],
-      };
+      state.discussions = [unresolvableDiscussion, unresolvableDiscussion];
 
-      expect(getters.allResolvableDiscussions(state, localGetters)).toEqual([]);
+      expect(getters.allResolvableDiscussions(state)).toEqual([]);
     });
   });
 
@@ -236,7 +232,7 @@ describe('Getters Notes Store', () => {
     it('should return the ID of the discussion after the ID provided', () => {
       expect(getters.nextUnresolvedDiscussionId(state, localGetters)('123')).toBe('456');
       expect(getters.nextUnresolvedDiscussionId(state, localGetters)('456')).toBe('789');
-      expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe(undefined);
+      expect(getters.nextUnresolvedDiscussionId(state, localGetters)('789')).toBe('123');
     });
   });
 
diff --git a/spec/javascripts/notes/stores/mutation_spec.js b/spec/javascripts/notes/stores/mutation_spec.js
index 461de5a3106..1c4449d1055 100644
--- a/spec/javascripts/notes/stores/mutation_spec.js
+++ b/spec/javascripts/notes/stores/mutation_spec.js
@@ -437,4 +437,51 @@ describe('Notes Store mutations', () => {
       expect(state.commentsDisabled).toEqual(true);
     });
   });
+
+  describe('UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS', () => {
+    it('updates resolvableDiscussionsCount', () => {
+      const state = {
+        discussions: [
+          { individual_note: false, resolvable: true, notes: [] },
+          { individual_note: true, resolvable: true, notes: [] },
+          { individual_note: false, resolvable: false, notes: [] },
+        ],
+        resolvableDiscussionsCount: 0,
+      };
+
+      mutations.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS(state);
+
+      expect(state.resolvableDiscussionsCount).toBe(1);
+    });
+
+    it('updates unresolvedDiscussionsCount', () => {
+      const state = {
+        discussions: [
+          { individual_note: false, resolvable: true, notes: [{ resolved: false }] },
+          { individual_note: true, resolvable: true, notes: [{ resolved: false }] },
+          { individual_note: false, resolvable: false, notes: [{ resolved: false }] },
+        ],
+        unresolvedDiscussionsCount: 0,
+      };
+
+      mutations.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS(state);
+
+      expect(state.unresolvedDiscussionsCount).toBe(1);
+    });
+
+    it('updates hasUnresolvedDiscussions', () => {
+      const state = {
+        discussions: [
+          { individual_note: false, resolvable: true, notes: [{ resolved: false }] },
+          { individual_note: false, resolvable: true, notes: [{ resolved: false }] },
+          { individual_note: false, resolvable: false, notes: [{ resolved: false }] },
+        ],
+        hasUnresolvedDiscussions: 0,
+      };
+
+      mutations.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS(state);
+
+      expect(state.hasUnresolvedDiscussions).toBe(true);
+    });
+  });
 });