Improve discussion rendering performance

Improve the renderign of new and existing discussions
by reducing the number of watchers on each object & array.
Previously every discussion change would trigger an update for every
discussion component.

Also tidied up some components to get them closer to our docs.

Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/51506

5 files changed, 63 insertions, 43 deletions

diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js
index 5b2f0540020..b4befdd6e4a 100644
--- a/app/assets/javascripts/notes/stores/actions.js
+++ b/app/assets/javascripts/notes/stores/actions.js
@@ -11,7 +11,7 @@ import * as constants from '../constants';
 import service from '../services/notes_service';
 import loadAwardsHandler from '../../awards_handler';
 import sidebarTimeTrackingEventHub from '../../sidebar/event_hub';
-import { isInViewport, scrollToElement } from '../../lib/utils/common_utils';
+import { isInViewport, scrollToElement, isInMRPage } from '../../lib/utils/common_utils';
 import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub';
 import { __ } from '~/locale';
 
@@ -39,12 +39,13 @@ export const setNotesFetchedState = ({ commit }, state) =>
 
 export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data);
 
-export const fetchDiscussions = ({ commit }, { path, filter }) =>
+export const fetchDiscussions = ({ commit, dispatch }, { path, filter }) =>
   service
     .fetchDiscussions(path, filter)
     .then(res => res.json())
     .then(discussions => {
       commit(types.SET_INITIAL_DISCUSSIONS, discussions);
+      dispatch('updateResolvableDiscussonsCounts');
     });
 
 export const updateDiscussion = ({ commit, state }, discussion) => {
@@ -53,11 +54,18 @@ export const updateDiscussion = ({ commit, state }, discussion) => {
   return utils.findNoteObjectById(state.discussions, discussion.id);
 };
 
-export const deleteNote = ({ commit, dispatch }, note) =>
+export const deleteNote = ({ commit, dispatch, state }, note) =>
   service.deleteNote(note.path).then(() => {
+    const discussion = state.discussions.find(({ id }) => id === note.discussion_id);
+
     commit(types.DELETE_NOTE, note);
 
     dispatch('updateMergeRequestWidget');
+    dispatch('updateResolvableDiscussonsCounts');
+
+    if (isInMRPage()) {
+      dispatch('diffs/removeDiscussionsFromDiff', discussion);
+    }
   });
 
 export const updateNote = ({ commit, dispatch }, { endpoint, note }) =>
@@ -89,6 +97,7 @@ export const createNewNote = ({ commit, dispatch }, { endpoint, data }) =>
 
         dispatch('updateMergeRequestWidget');
         dispatch('startTaskList');
+        dispatch('updateResolvableDiscussonsCounts');
       }
       return res;
     });
@@ -104,6 +113,8 @@ export const toggleResolveNote = ({ commit, dispatch }, { endpoint, isResolved,
 
       commit(mutationType, res);
 
+      dispatch('updateResolvableDiscussonsCounts');
+
       dispatch('updateMergeRequestWidget');
     });
 
@@ -385,5 +396,8 @@ export const startTaskList = ({ dispatch }) =>
       }),
   );
 
+export const updateResolvableDiscussonsCounts = ({ commit }) =>
+  commit(types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS);
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index 980d79605d7..2ed8aac059a 100644
--- a/app/assets/javascripts/notes/stores/getters.js
+++ b/app/assets/javascripts/notes/stores/getters.js
@@ -53,30 +53,15 @@ export const getCurrentUserLastNote = state =>
 export const getDiscussionLastNote = state => discussion =>
   reverseNotes(discussion.notes).find(el => isLastNote(el, state));
 
-export const discussionCount = state => {
-  const filteredDiscussions = state.discussions.filter(n => !n.individual_note && n.resolvable);
-
-  return filteredDiscussions.length;
-};
-
-export const unresolvedDiscussions = (state, getters) => {
-  const resolvedMap = getters.resolvedDiscussionsById;
-
-  return state.discussions.filter(n => !n.individual_note && !resolvedMap[n.id]);
-};
-
-export const allDiscussions = (state, getters) => {
-  const resolved = getters.resolvedDiscussionsById;
-  const unresolved = getters.unresolvedDiscussions;
-
-  return Object.values(resolved).concat(unresolved);
-};
+export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCount;
+export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount;
+export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions;
 
 export const isDiscussionResolved = (state, getters) => discussionId =>
   getters.resolvedDiscussionsById[discussionId] !== undefined;
 
-export const allResolvableDiscussions = (state, getters) =>
-  getters.allDiscussions.filter(d => !d.individual_note && d.resolvable);
+export const allResolvableDiscussions = state =>
+  state.discussions.filter(d => !d.individual_note && d.resolvable);
 
 export const resolvedDiscussionsById = state => {
   const map = {};
@@ -147,15 +132,12 @@ export const resolvedDiscussionCount = (state, getters) => {
   return Object.keys(resolvedMap).length;
 };
 
-export const discussionTabCounter = state => {
-  let all = [];
-
-  state.discussions.forEach(discussion => {
-    all = all.concat(discussion.notes.filter(note => !note.system && !note.placeholder));
-  });
-
-  return all.length;
-};
+export const discussionTabCounter = state =>
+  state.discussions.reduce(
+    (acc, discussion) =>
+      acc + discussion.notes.filter(note => !note.system && !note.placeholder).length,
+    0,
+  );
 
 // Returns the list of discussion IDs ordered according to given parameter
 // @param {Boolean} diffOrder - is ordered by diff?
@@ -182,8 +164,10 @@ export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, dif
 export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => {
   const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder);
   const currentIndex = idsOrdered.indexOf(discussionId);
+  const slicedIds = idsOrdered.slice(currentIndex + 1, currentIndex + 2);
 
-  return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0];
+  // Get the first ID if there is none after the currentIndex
+  return slicedIds.length ? idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0] : idsOrdered[0];
 };
 
 // @param {Boolean} diffOrder - is ordered by diff?
diff --git a/app/assets/javascripts/notes/stores/modules/index.js b/app/assets/javascripts/notes/stores/modules/index.js
index 8aea269ea7d..b5fe8bdb1d3 100644
--- a/app/assets/javascripts/notes/stores/modules/index.js
+++ b/app/assets/javascripts/notes/stores/modules/index.js
@@ -22,6 +22,9 @@ export default () => ({
       current_user: {},
     },
     commentsDisabled: false,
+    resolvableDiscussionsCount: 0,
+    unresolvedDiscussionsCount: 0,
+    hasUnresolvedDiscussions: false,
   },
   actions,
   getters,
diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js
index dfbf3b7b34b..9c68ab67a8c 100644
--- a/app/assets/javascripts/notes/stores/mutation_types.js
+++ b/app/assets/javascripts/notes/stores/mutation_types.js
@@ -21,6 +21,7 @@ export const DISABLE_COMMENTS = 'DISABLE_COMMENTS';
 export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION';
 export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION';
 export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION';
+export const UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS = 'UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS';
 
 // Issue
 export const CLOSE_ISSUE = 'CLOSE_ISSUE';
diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js
index f6054e0be87..667c8a97cf3 100644
--- a/app/assets/javascripts/notes/stores/mutations.js
+++ b/app/assets/javascripts/notes/stores/mutations.js
@@ -24,6 +24,7 @@ export default {
         noteData.resolved = false;
         noteData.resolve_path = note.resolve_path;
         noteData.resolve_with_issue_path = note.resolve_with_issue_path;
+        noteData.diff_discussion = false;
       }
 
       state.discussions.push(noteData);
@@ -97,33 +98,36 @@ export default {
   },
 
   [types.SET_INITIAL_DISCUSSIONS](state, discussionsData) {
-    const discussions = [];
+    const discussions = discussionsData.reduce((acc, d) => {
+      const discussion = { ...d };
+      const diffData = {};
 
-    discussionsData.forEach(discussion => {
       if (discussion.diff_file) {
-        Object.assign(discussion, {
-          file_hash: discussion.diff_file.file_hash,
-          truncated_diff_lines: discussion.truncated_diff_lines || [],
-        });
+        diffData.file_hash = discussion.diff_file.file_hash;
+        diffData.truncated_diff_lines = discussion.truncated_diff_lines || [];
       }
 
       // To support legacy notes, should be very rare case.
       if (discussion.individual_note && discussion.notes.length > 1) {
         discussion.notes.forEach(n => {
-          discussions.push({
+          acc.push({
             ...discussion,
+            ...diffData,
             notes: [n], // override notes array to only have one item to mimick individual_note
           });
         });
       } else {
         const oldNote = utils.findNoteObjectById(state.discussions, discussion.id);
 
-        discussions.push({
+        acc.push({
           ...discussion,
+          ...diffData,
           expanded: oldNote ? oldNote.expanded : discussion.expanded,
         });
       }
-    });
+
+      return acc;
+    }, []);
 
     Object.assign(state, { discussions });
   },
@@ -195,7 +199,9 @@ export default {
     const selectedDiscussion = state.discussions.find(disc => disc.id === note.id);
     note.expanded = true; // override expand flag to prevent collapse
     if (note.diff_file) {
-      Object.assign(note, { file_hash: note.diff_file.file_hash });
+      Object.assign(note, {
+        file_hash: note.diff_file.file_hash,
+      });
     }
     Object.assign(selectedDiscussion, { ...note });
   },
@@ -229,4 +235,16 @@ export default {
   [types.DISABLE_COMMENTS](state, value) {
     state.commentsDisabled = value;
   },
+  [types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS](state) {
+    state.resolvableDiscussionsCount = state.discussions.filter(
+      discussion => !discussion.individual_note && discussion.resolvable,
+    ).length;
+    state.unresolvedDiscussionsCount = state.discussions.filter(
+      discussion =>
+        !discussion.individual_note &&
+        discussion.resolvable &&
+        discussion.notes.some(note => !note.resolved),
+    ).length;
+    state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1;
+  },
 };