Resolve "Navigating unresolved discussions on Merge Request page"

9 files changed, 105 insertions, 19 deletions

diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue
index 3b2a0d156ca..bed29efb253 100644
--- a/app/assets/javascripts/diffs/components/diff_file.vue
+++ b/app/assets/javascripts/diffs/components/diff_file.vue
@@ -4,6 +4,7 @@ import _ from 'underscore';
 import { __, sprintf } from '~/locale';
 import createFlash from '~/flash';
 import { GlLoadingIcon } from '@gitlab/ui';
+import eventHub from '../../notes/event_hub';
 import DiffFileHeader from './diff_file_header.vue';
 import DiffContent from './diff_content.vue';
 
@@ -75,6 +76,9 @@ export default {
       }
     },
   },
+  created() {
+    eventHub.$on(`loadCollapsedDiff/${this.file.file_hash}`, this.handleLoadCollapsedDiff);
+  },
   methods: {
     ...mapActions('diffs', ['loadCollapsedDiff', 'assignDiscussionsToDiff']),
     handleToggle() {
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index 952963e0711..00a4bb6d3a3 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -3,8 +3,9 @@ import axios from '~/lib/utils/axios_utils';
 import Cookies from 'js-cookie';
 import createFlash from '~/flash';
 import { s__ } from '~/locale';
-import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
+import { handleLocationHash, historyPushState, scrollToElement } from '~/lib/utils/common_utils';
 import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
+import eventHub from '../../notes/event_hub';
 import { getDiffPositionByLineCode, getNoteFormData } from './utils';
 import * as types from './mutation_types';
 import {
@@ -53,6 +54,10 @@ export const assignDiscussionsToDiff = (
         diffPositionByLineCode,
       });
     });
+
+  Vue.nextTick(() => {
+    eventHub.$emit('scrollToDiscussion');
+  });
 };
 
 export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => {
@@ -60,6 +65,27 @@ export const removeDiscussionsFromDiff = ({ commit }, removeDiscussion) => {
   commit(types.REMOVE_LINE_DISCUSSIONS_FOR_FILE, { fileHash: file_hash, lineCode: line_code, id });
 };
 
+export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => {
+  const discussion = rootState.notes.discussions.find(d => d.id === discussionId);
+
+  if (discussion) {
+    const file = state.diffFiles.find(f => f.file_hash === discussion.diff_file.file_hash);
+
+    if (file) {
+      if (!file.renderIt) {
+        commit(types.RENDER_FILE, file);
+      }
+
+      if (file.collapsed) {
+        eventHub.$emit(`loadCollapsedDiff/${file.file_hash}`);
+        scrollToElement(document.getElementById(file.file_hash));
+      } else {
+        eventHub.$emit('scrollToDiscussion');
+      }
+    }
+  }
+};
+
 export const startRenderDiffsQueue = ({ state, commit }) => {
   const checkItem = () =>
     new Promise(resolve => {
diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js
index 331fb052371..61314db1dbd 100644
--- a/app/assets/javascripts/diffs/store/mutations.js
+++ b/app/assets/javascripts/diffs/store/mutations.js
@@ -170,7 +170,7 @@ export default {
         }
 
         if (!file.parallel_diff_lines || !file.highlighted_diff_lines) {
-          file.discussions = file.discussions.concat(discussion);
+          file.discussions = (file.discussions || []).concat(discussion);
         }
 
         return file;
diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index 040d0bc659e..9e22cdc04e9 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -192,8 +192,12 @@ export const contentTop = () => {
   const mrTabsHeight = $('.merge-request-tabs').height() || 0;
   const headerHeight = $('.navbar-gitlab').height() || 0;
   const diffFilesChanged = $('.js-diff-files-changed').height() || 0;
+  const diffFileLargeEnoughScreen =
+    'matchMedia' in window ? window.matchMedia('min-width: 768') : true;
+  const diffFileTitleBar =
+    (diffFileLargeEnoughScreen && $('.diff-file .file-title-flex-parent:visible').height()) || 0;
 
-  return perfBar + mrTabsHeight + headerHeight + diffFilesChanged;
+  return perfBar + mrTabsHeight + headerHeight + diffFilesChanged + diffFileTitleBar;
 };
 
 export const scrollToElement = element => {
diff --git a/app/assets/javascripts/notes/components/noteable_discussion.vue b/app/assets/javascripts/notes/components/noteable_discussion.vue
index f4991a41325..d4450c9f2c8 100644
--- a/app/assets/javascripts/notes/components/noteable_discussion.vue
+++ b/app/assets/javascripts/notes/components/noteable_discussion.vue
@@ -81,6 +81,7 @@ export default {
       'nextUnresolvedDiscussionId',
       'unresolvedDiscussionsCount',
       'hasUnresolvedDiscussions',
+      'showJumpToNextDiscussion',
     ]),
     author() {
       return this.initialDiscussion.author;
@@ -121,6 +122,12 @@ export default {
     resolvedText() {
       return this.discussion.resolved_by_push ? __('Automatically resolved') : __('Resolved');
     },
+    shouldShowJumpToNextDiscussion() {
+      return this.showJumpToNextDiscussion(
+        this.discussion.id,
+        this.discussionsByDiffOrder ? 'diff' : 'discussion',
+      );
+    },
     shouldRenderDiffs() {
       return this.discussion.diff_discussion && this.renderDiffFile;
     },
@@ -418,7 +425,7 @@ Please check your network connection and try again.`;
                           <icon name="issue-new" />
                         </a>
                       </div>
-                      <div v-if="hasUnresolvedDiscussions" class="btn-group" role="group">
+                      <div v-if="shouldShowJumpToNextDiscussion" class="btn-group" role="group">
                         <button
                           v-gl-tooltip
                           class="btn btn-default discussion-next-btn"
diff --git a/app/assets/javascripts/notes/mixins/discussion_navigation.js b/app/assets/javascripts/notes/mixins/discussion_navigation.js
index f7c4deee1f8..3d89d907777 100644
--- a/app/assets/javascripts/notes/mixins/discussion_navigation.js
+++ b/app/assets/javascripts/notes/mixins/discussion_navigation.js
@@ -1,29 +1,56 @@
 import { scrollToElement } from '~/lib/utils/common_utils';
+import eventHub from '../../notes/event_hub';
 
 export default {
   methods: {
-    jumpToDiscussion(id) {
-      if (id) {
-        const activeTab = window.mrTabs.currentAction;
-        const selector =
-          activeTab === 'diffs'
-            ? `ul.notes[data-discussion-id="${id}"]`
-            : `div.discussion[data-discussion-id="${id}"]`;
-        const el = document.querySelector(selector);
+    diffsJump(id) {
+      const selector = `ul.notes[data-discussion-id="${id}"]`;
 
-        if (activeTab === 'commits' || activeTab === 'pipelines') {
-          window.mrTabs.activateTab('show');
-        }
+      eventHub.$once('scrollToDiscussion', () => {
+        const el = document.querySelector(selector);
 
         if (el) {
-          this.expandDiscussion({ discussionId: id });
-
           scrollToElement(el);
+
           return true;
         }
+
+        return false;
+      });
+
+      this.expandDiscussion({ discussionId: id });
+    },
+    discussionJump(id) {
+      const selector = `div.discussion[data-discussion-id="${id}"]`;
+
+      const el = document.querySelector(selector);
+
+      this.expandDiscussion({ discussionId: id });
+
+      if (el) {
+        scrollToElement(el);
+
+        return true;
       }
 
       return false;
     },
+    jumpToDiscussion(id) {
+      if (id) {
+        const activeTab = window.mrTabs.currentAction;
+
+        if (activeTab === 'diffs') {
+          this.diffsJump(id);
+        } else if (activeTab === 'commits' || activeTab === 'pipelines') {
+          window.mrTabs.eventHub.$once('MergeRequestTabChange', () => {
+            setTimeout(() => this.discussionJump(id), 0);
+          });
+
+          window.mrTabs.tabShown('show');
+        } else {
+          this.discussionJump(id);
+        }
+      }
+    },
   },
 };
diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js
index b4befdd6e4a..4716ab52333 100644
--- a/app/assets/javascripts/notes/stores/actions.js
+++ b/app/assets/javascripts/notes/stores/actions.js
@@ -17,7 +17,13 @@ import { __ } from '~/locale';
 
 let eTagPoll;
 
-export const expandDiscussion = ({ commit }, data) => commit(types.EXPAND_DISCUSSION, data);
+export const expandDiscussion = ({ commit, dispatch }, data) => {
+  if (data.discussionId) {
+    dispatch('diffs/renderFileForDiscussionId', data.discussionId, { root: true });
+  }
+
+  commit(types.EXPAND_DISCUSSION, data);
+};
 
 export const collapseDiscussion = ({ commit }, data) => commit(types.COLLAPSE_DISCUSSION, data);
 
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index 2ed8aac059a..0ffc0cb2593 100644
--- a/app/assets/javascripts/notes/stores/getters.js
+++ b/app/assets/javascripts/notes/stores/getters.js
@@ -57,6 +57,17 @@ export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCo
 export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount;
 export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions;
 
+export const showJumpToNextDiscussion = (state, getters) => (discussionId, mode = 'discussion') => {
+  const orderedDiffs =
+    mode !== 'discussion'
+      ? getters.unresolvedDiscussionsIdsByDiff
+      : getters.unresolvedDiscussionsIdsByDate;
+
+  const indexOf = orderedDiffs.indexOf(discussionId);
+
+  return indexOf !== -1 && indexOf < orderedDiffs.length - 1;
+};
+
 export const isDiscussionResolved = (state, getters) => discussionId =>
   getters.resolvedDiscussionsById[discussionId] !== undefined;
 
@@ -104,7 +115,7 @@ export const unresolvedDiscussionsIdsByDate = (state, getters) =>
 // line numbers.
 export const unresolvedDiscussionsIdsByDiff = (state, getters) =>
   getters.allResolvableDiscussions
-    .filter(d => !d.resolved)
+    .filter(d => !d.resolved && d.active)
     .sort((a, b) => {
       if (!a.diff_file || !b.diff_file) {
         return 0;
diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js
index bea396e5bb6..a3228f2cfea 100644
--- a/app/assets/javascripts/notes/stores/mutations.js
+++ b/app/assets/javascripts/notes/stores/mutations.js
@@ -22,6 +22,7 @@ export default {
       if (isDiscussion && isInMRPage()) {
         noteData.resolvable = note.resolvable;
         noteData.resolved = false;
+        noteData.active = true;
         noteData.resolve_path = note.resolve_path;
         noteData.resolve_with_issue_path = note.resolve_with_issue_path;
         noteData.diff_discussion = false;