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

Improve discussion rendering performance

Closes #51506

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

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

@@ -99,7 +99,7 @@ export default {
   methods: {
     ...mapActions('diffs', ['loadMoreLines', 'showCommentForm']),
     handleCommentButton() {
-      this.showCommentForm({ lineCode: this.line.line_code });
+      this.showCommentForm({ lineCode: this.line.line_code, fileHash: this.fileHash });
     },
     handleLoadMoreLines() {
       if (this.isRequesting) {
@@ -160,7 +160,7 @@ export default {
     >
     <template v-else>
       <button
-        v-if="shouldShowCommentButton"
+        v-show="shouldShowCommentButton"
         type="button"
         class="add-diff-note js-add-diff-note-button qa-diff-comment"
         title="Add a comment to this line"

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

@@ -73,6 +73,7 @@ export default {
 
       this.cancelCommentForm({
         lineCode: this.line.line_code,
+        fileHash: this.diffFileHash,
       });
       this.$nextTick(() => {
         this.resetAutoSave();

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

 <script>
-import { mapState } from 'vuex';
 import diffDiscussions from './diff_discussions.vue';
 import diffLineNoteForm from './diff_line_note_form.vue';
 
@@ -17,29 +16,31 @@ export default {
       type: String,
       required: true,
     },
-    lineIndex: {
-      type: Number,
-      required: true,
-    },
   },
   computed: {
-    ...mapState({
-      diffLineCommentForms: state => state.diffs.diffLineCommentForms,
-    }),
     className() {
       return this.line.discussions.length ? '' : 'js-temp-notes-holder';
     },
+    shouldRender() {
+      if (this.line.hasForm) return true;
+
+      if (!this.line.discussions || !this.line.discussions.length) {
+        return false;
+      }
+
+      return this.line.discussions.every(discussion => discussion.expanded);
+    },
   },
 };
 </script>
 
 <template>
-  <tr :class="className" class="notes_holder">
+  <tr v-if="shouldRender" :class="className" class="notes_holder">
     <td class="notes_content" colspan="3">
       <div class="content">
         <diff-discussions v-if="line.discussions.length" :discussions="line.discussions" />
         <diff-line-note-form
-          v-if="diffLineCommentForms[line.line_code]"
+          v-if="line.hasForm"
           :diff-file-hash="diffFileHash"
           :line="line"
           :note-target-line="line"

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

 <script>
-import { mapGetters, mapState } from 'vuex';
+import { mapGetters } from 'vuex';
 import inlineDiffTableRow from './inline_diff_table_row.vue';
 import inlineDiffCommentRow from './inline_diff_comment_row.vue';
 
@@ -19,23 +19,18 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId', 'shouldRenderInlineCommentRow']),
-    ...mapState({
-      diffLineCommentForms: state => state.diffs.diffLineCommentForms,
-    }),
+    ...mapGetters('diffs', ['commitId']),
     diffLinesLength() {
       return this.diffLines.length;
     },
-    userColorScheme() {
-      return window.gon.user_color_scheme;
-    },
   },
+  userColorScheme: window.gon.user_color_scheme,
 };
 </script>
 
 <template>
   <table
-    :class="userColorScheme"
+    :class="$options.userColorScheme"
     :data-commit-id="commitId"
     class="code diff-wrap-lines js-syntax-highlight text-file js-diff-inline-view"
   >
@@ -49,11 +44,9 @@ export default {
           :is-bottom="index + 1 === diffLinesLength"
         />
         <inline-diff-comment-row
-          v-if="shouldRenderInlineCommentRow(line)"
-          :key="index"
+          :key="`icr-${index}`"
           :diff-file-hash="diffFile.file_hash"
           :line="line"
-          :line-index="index"
         />
       </template>
     </tbody>

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

 <script>
-import { mapState } from 'vuex';
 import diffDiscussions from './diff_discussions.vue';
 import diffLineNoteForm from './diff_line_note_form.vue';
 
@@ -23,22 +22,13 @@ export default {
     },
   },
   computed: {
-    ...mapState({
-      diffLineCommentForms: state => state.diffs.diffLineCommentForms,
-    }),
-    leftLineCode() {
-      return this.line.left && this.line.left.line_code;
-    },
-    rightLineCode() {
-      return this.line.right && this.line.right.line_code;
-    },
     hasExpandedDiscussionOnLeft() {
-      return this.line.left && this.line.left.discussions
+      return this.line.left && this.line.left.discussions.length
         ? this.line.left.discussions.every(discussion => discussion.expanded)
         : false;
     },
     hasExpandedDiscussionOnRight() {
-      return this.line.right && this.line.right.discussions
+      return this.line.right && this.line.right.discussions.length
         ? this.line.right.discussions.every(discussion => discussion.expanded)
         : false;
     },
@@ -57,9 +47,10 @@ export default {
       );
     },
     showRightSideCommentForm() {
-      return (
-        this.line.right && this.line.right.type && this.diffLineCommentForms[this.rightLineCode]
-      );
+      return this.line.right && this.line.right.type && this.line.right.hasForm;
+    },
+    showLeftSideCommentForm() {
+      return this.line.left && this.line.left.hasForm;
     },
     className() {
       return (this.left && this.line.left.discussions.length > 0) ||
@@ -67,12 +58,30 @@ export default {
         ? ''
         : 'js-temp-notes-holder';
     },
+    shouldRender() {
+      const { line } = this;
+      const hasDiscussion =
+        (line.left && line.left.discussions && line.left.discussions.length) ||
+        (line.right && line.right.discussions && line.right.discussions.length);
+
+      if (
+        hasDiscussion &&
+        (this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight)
+      ) {
+        return true;
+      }
+
+      const hasCommentFormOnLeft = line.left && line.left.hasForm;
+      const hasCommentFormOnRight = line.right && line.right.hasForm;
+
+      return hasCommentFormOnLeft || hasCommentFormOnRight;
+    },
   },
 };
 </script>
 
 <template>
-  <tr :class="className" class="notes_holder">
+  <tr v-if="shouldRender" :class="className" class="notes_holder">
     <td class="notes_content parallel old" colspan="2">
       <div v-if="shouldRenderDiscussionsOnLeft" class="content">
         <diff-discussions
@@ -81,7 +90,7 @@ export default {
         />
       </div>
       <diff-line-note-form
-        v-if="diffLineCommentForms[leftLineCode]"
+        v-if="showLeftSideCommentForm"
         :diff-file-hash="diffFileHash"
         :line="line.left"
         :note-target-line="line.left"

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

 <script>
-import { mapState, mapGetters } from 'vuex';
+import { mapGetters } from 'vuex';
 import parallelDiffTableRow from './parallel_diff_table_row.vue';
 import parallelDiffCommentRow from './parallel_diff_comment_row.vue';
 
@@ -19,23 +19,18 @@ export default {
     },
   },
   computed: {
-    ...mapGetters('diffs', ['commitId', 'shouldRenderParallelCommentRow']),
-    ...mapState({
-      diffLineCommentForms: state => state.diffs.diffLineCommentForms,
-    }),
+    ...mapGetters('diffs', ['commitId']),
     diffLinesLength() {
       return this.diffLines.length;
     },
-    userColorScheme() {
-      return window.gon.user_color_scheme;
-    },
   },
+  userColorScheme: window.gon.user_color_scheme,
 };
 </script>
 
 <template>
   <div
-    :class="userColorScheme"
+    :class="$options.userColorScheme"
     :data-commit-id="commitId"
     class="code diff-wrap-lines js-syntax-highlight text-file"
   >
@@ -50,7 +45,6 @@ export default {
             :is-bottom="index + 1 === diffLinesLength"
           />
           <parallel-diff-comment-row
-            v-if="shouldRenderParallelCommentRow(line)"
             :key="`dcr-${index}`"
             :line="line"
             :diff-file-hash="diffFile.file_hash"

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

@@ -99,12 +99,12 @@ export const setParallelDiffViewType = ({ commit }) => {
   historyPushState(url);
 };
 
-export const showCommentForm = ({ commit }, params) => {
-  commit(types.ADD_COMMENT_FORM_LINE, params);
+export const showCommentForm = ({ commit }, { lineCode, fileHash }) => {
+  commit(types.TOGGLE_LINE_HAS_FORM, { lineCode, fileHash, hasForm: true });
 };
 
-export const cancelCommentForm = ({ commit }, params) => {
-  commit(types.REMOVE_COMMENT_FORM_LINE, params);
+export const cancelCommentForm = ({ commit }, { lineCode, fileHash }) => {
+  commit(types.TOGGLE_LINE_HAS_FORM, { lineCode, fileHash, hasForm: false });
 };
 
 export const loadMoreLines = ({ commit }, options) => {
@@ -191,8 +191,8 @@ export const saveDiffDiscussion = ({ dispatch }, { note, formData }) => {
   return dispatch('saveNote', postData, { root: true })
     .then(result => dispatch('updateDiscussion', result.discussion, { root: true }))
     .then(discussion => dispatch('assignDiscussionsToDiff', [discussion]))
+    .then(() => dispatch('updateResolvableDiscussonsCounts', null, { root: true }))
     .then(() => dispatch('closeDiffFileCommentForm', formData.diffFile.file_hash))
-    .then(() => dispatch('startTaskList', null, { root: true }))
     .catch(() => createFlash(s__('MergeRequests|Saving the comment failed')));
 };
 

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

@@ -70,40 +70,6 @@ export const getDiffFileDiscussions = (state, getters, rootState, rootGetters) =
     discussion => discussion.diff_discussion && discussion.diff_file.file_hash === diff.file_hash,
   ) || [];
 
-export const shouldRenderParallelCommentRow = state => line => {
-  const hasDiscussion =
-    (line.left && line.left.discussions && line.left.discussions.length) ||
-    (line.right && line.right.discussions && line.right.discussions.length);
-
-  const hasExpandedDiscussionOnLeft =
-    line.left && line.left.discussions && line.left.discussions.length
-      ? line.left.discussions.every(discussion => discussion.expanded)
-      : false;
-  const hasExpandedDiscussionOnRight =
-    line.right && line.right.discussions && line.right.discussions.length
-      ? line.right.discussions.every(discussion => discussion.expanded)
-      : false;
-
-  if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) {
-    return true;
-  }
-
-  const hasCommentFormOnLeft = line.left && state.diffLineCommentForms[line.left.line_code];
-  const hasCommentFormOnRight = line.right && state.diffLineCommentForms[line.right.line_code];
-
-  return hasCommentFormOnLeft || hasCommentFormOnRight;
-};
-
-export const shouldRenderInlineCommentRow = state => line => {
-  if (state.diffLineCommentForms[line.line_code]) return true;
-
-  if (!line.discussions || line.discussions.length === 0) {
-    return false;
-  }
-
-  return line.discussions.every(discussion => discussion.expanded);
-};
-
 // prevent babel-plugin-rewire from generating an invalid default during karma∂ tests
 export const getDiffFileByHash = state => fileHash =>
   state.diffFiles.find(file => file.file_hash === fileHash);

app/assets/javascripts/diffs/store/modules/diff_state.js

@@ -18,7 +18,6 @@ export default () => ({
   diffFiles: [],
   mergeRequestDiffs: [],
   mergeRequestDiff: null,
-  diffLineCommentForms: {},
   diffViewType: viewTypeFromQueryString || viewTypeFromCookie || defaultViewType,
   tree: [],
   treeEntries: {},

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

@@ -3,8 +3,7 @@ export const SET_LOADING = 'SET_LOADING';
 export const SET_DIFF_DATA = 'SET_DIFF_DATA';
 export const SET_DIFF_VIEW_TYPE = 'SET_DIFF_VIEW_TYPE';
 export const SET_MERGE_REQUEST_DIFFS = 'SET_MERGE_REQUEST_DIFFS';
-export const ADD_COMMENT_FORM_LINE = 'ADD_COMMENT_FORM_LINE';
-export const REMOVE_COMMENT_FORM_LINE = 'REMOVE_COMMENT_FORM_LINE';
+export const TOGGLE_LINE_HAS_FORM = 'TOGGLE_LINE_HAS_FORM';
 export const ADD_CONTEXT_LINES = 'ADD_CONTEXT_LINES';
 export const ADD_COLLAPSED_DIFFS = 'ADD_COLLAPSED_DIFFS';
 export const EXPAND_ALL_FILES = 'EXPAND_ALL_FILES';

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

-import Vue from 'vue';
 import { convertObjectPropsToCamelCase } from '~/lib/utils/common_utils';
 import { sortTree } from '~/ide/stores/utils';
 import {
@@ -49,12 +48,30 @@ export default {
     Object.assign(state, { diffViewType });
   },
 
-  [types.ADD_COMMENT_FORM_LINE](state, { lineCode }) {
-    Vue.set(state.diffLineCommentForms, lineCode, true);
-  },
+  [types.TOGGLE_LINE_HAS_FORM](state, { lineCode, fileHash, hasForm }) {
+    const diffFile = state.diffFiles.find(f => f.file_hash === fileHash);
+
+    if (!diffFile) return;
+
+    if (diffFile.highlighted_diff_lines) {
+      diffFile.highlighted_diff_lines.find(l => l.line_code === lineCode).hasForm = hasForm;
+    }
+
+    if (diffFile.parallel_diff_lines) {
+      const line = diffFile.parallel_diff_lines.find(l => {
+        const { left, right } = l;
 
-  [types.REMOVE_COMMENT_FORM_LINE](state, { lineCode }) {
-    Vue.delete(state.diffLineCommentForms, lineCode);
+        return (left && left.line_code === lineCode) || (right && right.line_code === lineCode);
+      });
+
+      if (line.left && line.left.line_code === lineCode) {
+        line.left.hasForm = hasForm;
+      }
+
+      if (line.right && line.right.line_code === lineCode) {
+        line.right.hasForm = hasForm;
+      }
+    }
   },
 
   [types.ADD_CONTEXT_LINES](state, options) {
@@ -68,6 +85,7 @@ export default {
       ...line,
       line_code: line.line_code || `${fileHash}_${line.old_line}_${line.new_line}`,
       discussions: line.discussions || [],
+      hasForm: false,
     }));
 
     addContextLines({

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

@@ -209,9 +209,11 @@ export function prepareDiffData(diffData) {
         const line = file.parallel_diff_lines[u];
         if (line.left) {
           line.left = trimFirstCharOfLineContent(line.left);
+          line.left.hasForm = false;
         }
         if (line.right) {
           line.right = trimFirstCharOfLineContent(line.right);
+          line.right.hasForm = false;
         }
       }
     }
@@ -220,7 +222,7 @@ export function prepareDiffData(diffData) {
       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) });
+        Object.assign(line, { ...trimFirstCharOfLineContent(line), hasForm: false });
       }
       showingLines += file.parallel_diff_lines.length;
     }

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

@@ -4,7 +4,9 @@ import DiffFileHeader from '~/diffs/components/diff_file_header.vue';
 import DiffViewer from '~/vue_shared/components/diff_viewer/diff_viewer.vue';
 import ImageDiffOverlay from '~/diffs/components/image_diff_overlay.vue';
 import { GlSkeletonLoading } from '@gitlab/ui';
-import { trimFirstCharOfLineContent, getDiffMode } from '~/diffs/store/utils';
+import { getDiffMode } from '~/diffs/store/utils';
+
+const FIRST_CHAR_REGEX = /^(\+|-| )/;
 
 export default {
   components: {
@@ -26,46 +28,16 @@ export default {
   },
   computed: {
     ...mapState({
-      noteableData: state => state.notes.noteableData,
       projectPath: state => state.diffs.projectPath,
     }),
     diffMode() {
-      return getDiffMode(this.diffFile);
+      return getDiffMode(this.discussion.diff_file);
     },
     hasTruncatedDiffLines() {
       return (
         this.discussion.truncated_diff_lines && this.discussion.truncated_diff_lines.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 this.discussion.diff_file;
-    },
-    imageDiffHtml() {
-      return this.discussion.image_diff_html;
-    },
-    userColorScheme() {
-      return window.gon.user_color_scheme;
-    },
-    normalizedDiffLines() {
-      if (this.discussion.truncated_diff_lines) {
-        return this.discussion.truncated_diff_lines.map(line => trimFirstCharOfLineContent(line));
-      }
-
-      return [];
-    },
   },
   mounted() {
     if (!this.hasTruncatedDiffLines) {
@@ -74,9 +46,6 @@ export default {
   },
   methods: {
     ...mapActions(['fetchDiscussionDiffLines']),
-    rowTag(html) {
-      return html.outerHTML ? 'tr' : 'template';
-    },
     fetchDiff() {
       this.error = false;
       this.fetchDiscussionDiffLines(this.discussion)
@@ -85,31 +54,45 @@ export default {
           this.error = true;
         });
     },
+    trimChar(line) {
+      return line.replace(FIRST_CHAR_REGEX, '');
+    },
   },
+  userColorSchemeClass: window.gon.user_color_scheme,
 };
 </script>
 
 <template>
-  <div ref="fileHolder" :class="diffFileClass" class="diff-file file-holder">
+  <div :class="{ 'text-file': discussion.diff_file.text }" class="diff-file file-holder">
     <diff-file-header
       :discussion-path="discussion.discussion_path"
-      :diff-file="diffFile"
+      :diff-file="discussion.diff_file"
       :can-current-user-fork="false"
-      :discussions-expanded="isDiscussionsExpanded"
-      :expanded="!isCollapsed"
+      :expanded="!discussion.diff_file.collapsed"
     />
-    <div v-if="diffFile.text" :class="userColorScheme" class="diff-content code">
+    <div
+      v-if="discussion.diff_file.text"
+      :class="$options.userColorSchemeClass"
+      class="diff-content code"
+    >
       <table>
-        <tr v-for="line in normalizedDiffLines" :key="line.line_code" class="line_holder">
-          <td class="diff-line-num old_line">{{ line.old_line }}</td>
-          <td class="diff-line-num new_line">{{ line.new_line }}</td>
-          <td :class="line.type" class="line_content" v-html="line.rich_text"></td>
-        </tr>
+        <template v-if="hasTruncatedDiffLines">
+          <tr
+            v-for="line in discussion.truncated_diff_lines"
+            v-once
+            :key="line.line_code"
+            class="line_holder"
+          >
+            <td class="diff-line-num old_line">{{ line.old_line }}</td>
+            <td class="diff-line-num new_line">{{ line.new_line }}</td>
+            <td :class="line.type" class="line_content" v-html="trimChar(line.rich_text)"></td>
+          </tr>
+        </template>
         <tr v-if="!hasTruncatedDiffLines" class="line_holder line-holder-placeholder">
           <td class="old_line diff-line-num"></td>
           <td class="new_line diff-line-num"></td>
           <td v-if="error" class="js-error-lazy-load-diff diff-loading-error-block">
-            Unable to load the diff
+            {{ error }} Unable to load the diff
             <button
               class="btn-link btn-link-retry btn-no-padding js-toggle-lazy-diff-retry-button"
               @click="fetchDiff"
@@ -131,17 +114,17 @@ export default {
     <div v-else>
       <diff-viewer
         :diff-mode="diffMode"
-        :new-path="diffFile.new_path"
-        :new-sha="diffFile.diff_refs.head_sha"
-        :old-path="diffFile.old_path"
-        :old-sha="diffFile.diff_refs.base_sha"
-        :file-hash="diffFile.file_hash"
+        :new-path="discussion.diff_file.new_path"
+        :new-sha="discussion.diff_file.diff_refs.head_sha"
+        :old-path="discussion.diff_file.old_path"
+        :old-sha="discussion.diff_file.diff_refs.base_sha"
+        :file-hash="discussion.diff_file.file_hash"
         :project-path="projectPath"
       >
         <image-diff-overlay
           slot="image-overlay"
           :discussions="discussion"
-          :file-hash="diffFile.file_hash"
+          :file-hash="discussion.diff_file.file_hash"
           :show-comment-icon="true"
           :should-toggle-discussion="false"
           badge-class="image-comment-badge"

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

 <script>
 import { mapActions, mapGetters } from 'vuex';
+import { GlTooltipDirective } from '@gitlab/ui';
 import Icon from '~/vue_shared/components/icon.vue';
-import { pluralize } from '../../lib/utils/text_utility';
 import discussionNavigation from '../mixins/discussion_navigation';
-import tooltip from '../../vue_shared/directives/tooltip';
 
 export default {
   directives: {
-    tooltip,
+    GlTooltip: GlTooltipDirective,
   },
   components: {
     Icon,
@@ -17,9 +16,9 @@ export default {
     ...mapGetters([
       'getUserData',
       'getNoteableData',
-      'discussionCount',
+      'resolvableDiscussionsCount',
       'firstUnresolvedDiscussionId',
-      'resolvedDiscussionCount',
+      'unresolvedDiscussionsCount',
     ]),
     isLoggedIn() {
       return this.getUserData.id;
@@ -27,15 +26,15 @@ export default {
     hasNextButton() {
       return this.isLoggedIn && !this.allResolved;
     },
-    countText() {
-      return pluralize('discussion', this.discussionCount);
-    },
     allResolved() {
-      return this.resolvedDiscussionCount === this.discussionCount;
+      return this.unresolvedDiscussionsCount === 0;
     },
     resolveAllDiscussionsIssuePath() {
       return this.getNoteableData.create_issue_to_resolve_discussions_path;
     },
+    resolvedDiscussionsCount() {
+      return this.resolvableDiscussionsCount - this.unresolvedDiscussionsCount;
+    },
   },
   methods: {
     ...mapActions(['expandDiscussion']),
@@ -50,7 +49,7 @@ export default {
 </script>
 
 <template>
-  <div v-if="discussionCount > 0" class="line-resolve-all-container prepend-top-8">
+  <div v-if="resolvableDiscussionsCount > 0" class="line-resolve-all-container prepend-top-8">
     <div>
       <div :class="{ 'has-next-btn': hasNextButton }" class="line-resolve-all">
         <span
@@ -61,15 +60,15 @@ export default {
           <icon name="check-circle" />
         </span>
         <span class="line-resolve-text">
-          {{ resolvedDiscussionCount }}/{{ discussionCount }} {{ countText }} resolved
+          {{ resolvedDiscussionsCount }}/{{ resolvableDiscussionsCount }}
+          {{ n__('discussion resolved', 'discussions resolved', resolvableDiscussionsCount) }}
         </span>
       </div>
       <div v-if="resolveAllDiscussionsIssuePath && !allResolved" class="btn-group" role="group">
         <a
-          v-tooltip
+          v-gl-tooltip
           :href="resolveAllDiscussionsIssuePath"
           :title="s__('Resolve all discussions in new issue')"
-          data-container="body"
           class="new-issue-for-discussion btn btn-default discussion-create-issue-btn"
         >
           <icon name="issue-new" />
@@ -77,9 +76,8 @@ export default {
       </div>
       <div v-if="isLoggedIn && !allResolved" class="btn-group" role="group">
         <button
-          v-tooltip
+          v-gl-tooltip
           title="Jump to first unresolved discussion"
-          data-container="body"
           class="btn btn-default discussion-next-btn"
           @click="jumpToFirstUnresolvedDiscussion"
         >

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

 <script>
 import { mapGetters } from 'vuex';
 import Icon from '~/vue_shared/components/icon.vue';
-import tooltip from '~/vue_shared/directives/tooltip';
-import { GlLoadingIcon } from '@gitlab/ui';
+import { GlLoadingIcon, GlTooltipDirective } from '@gitlab/ui';
 
 export default {
   name: 'NoteActions',
@@ -11,7 +10,7 @@ export default {
     GlLoadingIcon,
   },
   directives: {
-    tooltip,
+    GlTooltip: GlTooltipDirective,
   },
   props: {
     authorId: {
@@ -119,10 +118,10 @@ export default {
 
 <template>
   <div class="note-actions">
-    <span v-if="accessLevel" class="note-role user-access-role"> {{ accessLevel }} </span>
+    <span v-if="accessLevel" class="note-role user-access-role">{{ accessLevel }}</span>
     <div v-if="canResolve" class="note-actions-item">
       <button
-        v-tooltip
+        v-gl-tooltip
         :class="{ 'is-disabled': !resolvable, 'is-active': isResolved }"
         :title="resolveButtonTitle"
         :aria-label="resolveButtonTitle"
@@ -138,12 +137,10 @@ export default {
     </div>
     <div v-if="canAwardEmoji" class="note-actions-item">
       <a
-        v-tooltip
+        v-gl-tooltip.bottom
         :class="{ 'js-user-authored': isAuthoredByCurrentUser }"
         class="note-action-button note-emoji-button js-add-award js-note-emoji"
         data-position="right"
-        data-placement="bottom"
-        data-container="body"
         href="#"
         title="Add reaction"
       >
@@ -158,12 +155,10 @@ export default {
     </div>
     <div v-if="canEdit" class="note-actions-item">
       <button
-        v-tooltip
+        v-gl-tooltip.bottom
         type="button"
         title="Edit comment"
         class="note-action-button js-note-edit btn btn-transparent"
-        data-container="body"
-        data-placement="bottom"
         @click="onEdit"
       >
         <icon name="pencil" css-classes="link-highlight" />
@@ -171,12 +166,10 @@ export default {
     </div>
     <div v-if="showDeleteAction" class="note-actions-item">
       <button
-        v-tooltip
+        v-gl-tooltip.bottom
         type="button"
         title="Delete comment"
         class="note-action-button js-note-delete btn btn-transparent"
-        data-container="body"
-        data-placement="bottom"
         @click="onDelete"
       >
         <icon name="remove" class="link-highlight" />
@@ -184,19 +177,17 @@ export default {
     </div>
     <div v-else-if="shouldShowActionsDropdown" class="dropdown more-actions note-actions-item">
       <button
-        v-tooltip
+        v-gl-tooltip.bottom
         type="button"
         title="More actions"
         class="note-action-button more-actions-toggle btn btn-transparent"
         data-toggle="dropdown"
-        data-container="body"
-        data-placement="bottom"
       >
         <icon css-classes="icon" name="ellipsis_v" />
       </button>
       <ul class="dropdown-menu more-actions-dropdown dropdown-open-left">
         <li v-if="canReportAsAbuse">
-          <a :href="reportAbusePath"> {{ __('Report abuse to GitLab') }} </a>
+          <a :href="reportAbusePath">{{ __('Report abuse to GitLab') }}</a>
         </li>
         <li v-if="noteUrl">
           <button
@@ -213,7 +204,7 @@ export default {
             type="button"
             @click.prevent="onDelete"
           >
-            <span class="text-danger"> {{ __('Delete comment') }} </span>
+            <span class="text-danger">{{ __('Delete comment') }}</span>
           </button>
         </li>
       </ul>

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

 <script>
 import { mapActions, mapGetters } from 'vuex';
+import { GlTooltipDirective } from '@gitlab/ui';
 import Icon from '~/vue_shared/components/icon.vue';
 import Flash from '../../flash';
 import { glEmojiTag } from '../../emoji';
-import tooltip from '../../vue_shared/directives/tooltip';
 
 export default {
   components: {
     Icon,
   },
   directives: {
-    tooltip,
+    GlTooltip: GlTooltipDirective,
   },
   props: {
     awards: {
@@ -167,21 +167,19 @@ export default {
       <button
         v-for="(awardList, awardName, index) in groupedAwards"
         :key="index"
-        v-tooltip
+        v-gl-tooltip.bottom="{ boundary: 'viewport' }"
         :class="getAwardClassBindings(awardList)"
         :title="awardTitle(awardList)"
         class="btn award-control"
-        data-boundary="viewport"
-        data-placement="bottom"
         type="button"
         @click="handleAward(awardName);"
       >
         <span v-html="getAwardHTML(awardName)"></span>
-        <span class="award-control-text js-counter"> {{ awardList.length }} </span>
+        <span class="award-control-text js-counter">{{ awardList.length }}</span>
       </button>
       <div v-if="canAwardEmoji" class="award-menu-holder">
         <button
-          v-tooltip
+          v-gl-tooltip
           :class="{ 'js-user-authored': isAuthoredByMe }"
           class="award-control btn js-add-award"
           title="Add reaction"

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

@@ -73,7 +73,7 @@ export default {
         {{ __('Toggle discussion') }}
       </button>
     </div>
-    <a v-if="hasAuthor" :href="author.path">
+    <a v-if="hasAuthor" v-once :href="author.path">
       <span class="note-header-author-name">{{ author.name }}</span>
       <span v-if="author.status_tooltip_html" v-html="author.status_tooltip_html"></span>
       <span class="note-headline-light"> @{{ author.username }} </span>

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

@@ -177,7 +177,7 @@ export default {
     class="note timeline-entry note-wrapper"
   >
     <div class="timeline-entry-inner">
-      <div class="timeline-icon">
+      <div v-once class="timeline-icon">
         <user-avatar-link
           :link-href="author.path"
           :img-src="author.avatar_url"
@@ -190,6 +190,7 @@ export default {
       <div class="timeline-content">
         <div class="note-header">
           <note-header
+            v-once
             :author="author"
             :created-at="note.created_at"
             :note-id="note.id"

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

@@ -22,6 +22,7 @@ export default {
     commentForm,
     placeholderNote,
     placeholderSystemNote,
+    skeletonLoadingContainer,
   },
   props: {
     noteableData: {
@@ -59,7 +60,6 @@ export default {
       'isNotesFetched',
       'discussions',
       'getNotesDataByProp',
-      'discussionCount',
       'isLoading',
       'commentsDisabled',
     ]),
@@ -109,39 +109,22 @@ export default {
     this.$nextTick(() => highlightCurrentUser(this.$el.querySelectorAll('.gfm-project_member')));
   },
   methods: {
-    ...mapActions({
-      setLoadingState: 'setLoadingState',
-      fetchDiscussions: 'fetchDiscussions',
-      poll: 'poll',
-      actionToggleAward: 'toggleAward',
-      scrollToNoteIfNeeded: 'scrollToNoteIfNeeded',
-      setNotesData: 'setNotesData',
-      setNoteableData: 'setNoteableData',
-      setUserData: 'setUserData',
-      setLastFetchedAt: 'setLastFetchedAt',
-      setTargetNoteHash: 'setTargetNoteHash',
-      toggleDiscussion: 'toggleDiscussion',
-      setNotesFetchedState: 'setNotesFetchedState',
-      startTaskList: 'startTaskList',
-    }),
-    getComponentName(discussion) {
-      if (discussion.isSkeletonNote) {
-        return skeletonLoadingContainer;
-      }
-      if (discussion.isPlaceholderNote) {
-        if (discussion.placeholderType === constants.SYSTEM_NOTE) {
-          return placeholderSystemNote;
-        }
-        return placeholderNote;
-      } else if (discussion.individual_note) {
-        return discussion.notes[0].system ? systemNote : noteableNote;
-      }
-
-      return noteableDiscussion;
-    },
-    getComponentData(discussion) {
-      return discussion.individual_note ? { note: discussion.notes[0] } : { discussion };
-    },
+    ...mapActions([
+      'setLoadingState',
+      'fetchDiscussions',
+      'poll',
+      'toggleAward',
+      'scrollToNoteIfNeeded',
+      'setNotesData',
+      'setNoteableData',
+      'setUserData',
+      'setLastFetchedAt',
+      'setTargetNoteHash',
+      'toggleDiscussion',
+      'setNotesFetchedState',
+      'expandDiscussion',
+      'startTaskList',
+    ]),
     fetchNotes() {
       if (this.isFetching) return null;
 
@@ -181,31 +164,46 @@ export default {
       const noteId = hash && hash.replace(/^note_/, '');
 
       if (noteId) {
-        this.discussions.forEach(discussion => {
-          if (discussion.notes) {
-            discussion.notes.forEach(note => {
-              if (`${note.id}` === `${noteId}`) {
-                // FIXME: this modifies the store state without using a mutation/action
-                Object.assign(discussion, { expanded: true });
-              }
-            });
-          }
-        });
+        const discussion = this.discussions.find(d => d.notes.some(({ id }) => id === noteId));
+
+        if (discussion) {
+          this.expandDiscussion({ discussionId: discussion.id });
+        }
       }
     },
   },
+  systemNote: constants.SYSTEM_NOTE,
 };
 </script>
 
 <template>
   <div v-show="shouldShow" id="notes">
     <ul id="notes-list" class="notes main-notes-list timeline">
-      <component
-        :is="getComponentName(discussion)"
-        v-for="discussion in allDiscussions"
-        :key="discussion.id"
-        v-bind="getComponentData(discussion)"
-      />
+      <template v-for="discussion in allDiscussions">
+        <skeleton-loading-container v-if="discussion.isSkeletonNote" :key="discussion.id" />
+        <template v-else-if="discussion.isPlaceholderNote">
+          <placeholder-system-note
+            v-if="discussion.placeholderType === $options.systemNote"
+            :key="discussion.id"
+            :note="discussion.notes[0]"
+          />
+          <placeholder-note v-else :key="discussion.id" :note="discussion.notes[0]" />
+        </template>
+        <template v-else-if="discussion.individual_note">
+          <system-note
+            v-if="discussion.notes[0].system"
+            :key="discussion.id"
+            :note="discussion.notes[0]"
+          />
+          <noteable-note v-else :key="discussion.id" :note="discussion.notes[0]" />
+        </template>
+        <noteable-discussion
+          v-else
+          :key="discussion.id"
+          :discussion="discussion"
+          :render-diff-file="true"
+        />
+      </template>
     </ul>
 
     <comment-form

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 () => {};

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?

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,

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';

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;
+  },
 };

locale/gitlab.pot

@@ -804,6 +804,9 @@ msgstr ""
 msgid "Automatically marked as default internal user"
 msgstr ""
 
+msgid "Automatically resolved"
+msgstr ""
+
 msgid "Available"
 msgstr ""
 
@@ -5465,6 +5468,9 @@ msgstr ""
 msgid "Resolve discussion"
 msgstr ""
 
+msgid "Resolved"
+msgstr ""
+
 msgid "Response metrics (AWS ELB)"
 msgstr ""
 
@@ -7571,6 +7577,11 @@ msgstr ""
 msgid "disabled"
 msgstr ""
 
+msgid "discussion resolved"
+msgid_plural "discussions resolved"
+msgstr[0] ""
+msgstr[1] ""
+
 msgid "done"
 msgstr ""
 

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

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, {

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();

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,
       );

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;

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);
+    });
+  });
 });

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();
     });
   });

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];

spec/javascripts/notes/stores/actions_spec.js

 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,
         );
@@ -471,6 +512,9 @@ describe('Actions Notes Store', () => {
             },
           ],
           [
+            {
+              type: 'updateResolvableDiscussonsCounts',
+            },
             {
               type: 'updateMergeRequestWidget',
             },
@@ -493,6 +537,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,
+      );
+    });
+  });
 });

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');
     });
   });
 

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);
+    });
+  });
 });