Implement Jump behavior for Changes tab

app/assets/javascripts/diff_notes/components/jump_to_discussion.js.es6

@@ -43,39 +43,139 @@
     },
     methods: {
       jumpToNextUnresolvedDiscussion: function () {
-        let unresolvedIds = CommentsStore.unresolvedDiscussionIds(),
-            nextUnresolvedDiscussionId;
-        const activePage = $('.merge-request-tabs .active a').attr('data-action'),
-              $diffDiscussions = $('.discussion').filter(function () {
-                return unresolvedIds.indexOf($(this).attr('data-discussion-id')) !== -1;
-              });
+        let discussionsSelector,
+            discussionIdsInScope,
+            firstUnresolvedDiscussionId,
+            nextUnresolvedDiscussionId,
+            activeTab = window.mrTabs.currentAction,
+            hasDiscussionsToJumpTo = true,
+            jumpToFirstDiscussion = !this.discussionId;
 
-        unresolvedIds = unresolvedIds.sort(function (a, b) {
-          return $diffDiscussions.index(`[data-discussion-id="${b}"]`) > $diffDiscussions.index(`[data-discussion-id="${a}"]`);
-        });
+        const discussionIdsForElements = function(elements) {
+          return elements.map(function() {
+            return $(this).attr('data-discussion-id');
+          }).toArray();
+        };
+
+        const discussions = this.discussions;
+
+        if (activeTab === 'diffs') {
+          discussionsSelector = '.diffs .notes[data-discussion-id]';
+          discussionIdsInScope = discussionIdsForElements($(discussionsSelector));
+
+          let unresolvedDiscussionCount = 0;
+          for (const discussionId of discussionIdsInScope) {
+            const discussion = discussions[discussionId];
+            if (discussion && !discussion.isResolved()) {
+              unresolvedDiscussionCount++;
+            }
+          }
+
+          if (this.discussionId && !this.discussion.isResolved()) {
+            // If this is the last unresolved discussion on the diffs tab,
+            // there are no discussions to jump to.
+            if (unresolvedDiscussionCount === 1) {
+              hasDiscussionsToJumpTo = false;
+            }
+          } else {
+            // If there are no unresolved discussions on the diffs tab at all,
+            // there are no discussions to jump to.
+            if (unresolvedDiscussionCount === 0) {
+              hasDiscussionsToJumpTo = false;
+            }
+          }
+        } else if (activeTab !== 'notes') {
+          // If we are on the commits or builds tabs,
+          // there are no discussions to jump to.
+          hasDiscussionsToJumpTo = false;
+        }
+
+        if (!hasDiscussionsToJumpTo) {
+          // If there are no discussions to jump to on the current page,
+          // switch to the notes tab and jump to the first disucssion there.
+          window.mrTabs.activateTab('notes');
+          activeTab = 'notes';
+          jumpToFirstDiscussion = true;
+        }
+
+        if (activeTab === 'notes') {
+          discussionsSelector = '.discussion[data-discussion-id]';
+          discussionIdsInScope = discussionIdsForElements($(discussionsSelector));
+        }
+
+        let currentDiscussionFound = false;
+        for (const discussionId of discussionIdsInScope) {
+          const discussion = discussions[discussionId];
+
+          if (!discussion) {
+            // Discussions for comments on commits in this MR don't have a resolved status.
+            continue;
+          }
 
-        unresolvedIds.forEach(function (discussionId, i) {
-          if (this.discussionId && discussionId === this.discussionId) {
-            nextUnresolvedDiscussionId = unresolvedIds[i + 1];
+          if (!firstUnresolvedDiscussionId && !discussion.isResolved()) {
+            firstUnresolvedDiscussionId = discussionId;
+
+            if (jumpToFirstDiscussion) {
+              break;
+            }
+          }
+
+          if (!jumpToFirstDiscussion) {
+            if (currentDiscussionFound) {
+              if (!discussion.isResolved()) {
+                nextUnresolvedDiscussionId = discussionId;
+                break;
+              }
+              else {
+                continue;
+              }
+            }
+
+            if (discussionId === this.discussionId) {
+              currentDiscussionFound = true;
+            }
+          }
+        }
+
+        nextUnresolvedDiscussionId = nextUnresolvedDiscussionId || firstUnresolvedDiscussionId;
+
+        if (!nextUnresolvedDiscussionId) {
           return;
         }
-        }.bind(this));
 
-        nextUnresolvedDiscussionId = nextUnresolvedDiscussionId || unresolvedIds[0];
+        let $target = $(`${discussionsSelector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`);
+
+        if (activeTab === 'notes') {
+          $target = $target.closest('.note-discussion');
+        } else if (activeTab === 'diffs') {
+          // Resolved discussions are hidden in the diffs tab by default.
+          // If they are marked unresolved on the notes tab, they will still be hidden on the diffs tab.
+          // When jumping between unresolved discussions on the diffs tab, we show them.
+          $target.closest(".content").show();
 
-        if (nextUnresolvedDiscussionId) {
-          let selector = '.discussion';
+          $target = $target.closest("tr.notes_holder");
+          $target.show();
 
-          if (activePage === 'diffs' && $(`${selector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`).length) {
-            selector = '.diffs .notes';
+          // If we are on the diffs tab, we don't scroll to the discussion itself, but to
+          // 4 diff lines above it: the line the discussion was in response to + 3 context
+          let prevEl;
+          for (let i = 0; i < 4; i++) {
+            prevEl = $target.prev();
+
+            // If the discussion doesn't have 4 lines above it, we'll have to do with fewer.
+            if (!prevEl.hasClass("line_holder")) {
+              break;
             }
 
-          $.scrollTo(`${selector}[data-discussion-id="${nextUnresolvedDiscussionId}"]`, {
+            $target = prevEl;
+          }
+        }
+
+        $.scrollTo($target, {
           offset: -($('.navbar-gitlab').outerHeight() + $('.layout-nav').outerHeight())
         });
       }
     }
-    }
   });
 
   Vue.component('jump-to-discussion', JumpToDiscussion);

app/assets/javascripts/diff_notes/components/resolve_count.js.es6

@@ -11,7 +11,7 @@
     },
     computed: {
       allResolved: function () {
-        return this.resolved === this.discussionCount;
+        return this.resolvedDiscussionCount === this.discussionCount;
       }
     }
   });

app/assets/javascripts/diff_notes/mixins/discussion.js.es6

@@ -4,7 +4,7 @@
       discussionCount: function () {
         return Object.keys(this.discussions).length;
       },
-      resolved: function () {
+      resolvedDiscussionCount: function () {
         let resolvedCount = 0;
 
         for (const discussionId in this.discussions) {
@@ -19,6 +19,7 @@
       },
       unresolvedDiscussionCount: function () {
         let unresolvedCount = 0;
+
         for (const discussionId in this.discussions) {
           const discussion = this.discussions[discussionId];
 

app/assets/javascripts/diff_notes/services/resolve.js.es6

@@ -11,10 +11,8 @@
 
     prepareRequest(namespace) {
       this.setCSRF();
-      if (Vue.http.options.root !== `/${namespace}`) {
       Vue.http.options.root = `/${namespace}`;
     }
-    }
 
     resolve(namespace, noteId) {
       this.prepareRequest(namespace);

app/assets/javascripts/merge_request_tabs.js

@@ -81,6 +81,7 @@
       if (action === 'show') {
         action = 'notes';
       }
+      this.currentAction = action;
       new_state = this._location.pathname.replace(/\/(commits|diffs|builds)(\.html)?\/?$/, '');
       if (action !== 'notes') {
         new_state += "/" + action;

app/views/projects/merge_requests/_show.html.haml

@@ -66,12 +66,12 @@
         %li#resolve-count-app.line-resolve-all-container.pull-right.prepend-top-10.hidden-xs{ "v-cloak" => true }
           %resolve-count{ "inline-template" => true, ":logged-out" => "#{current_user.nil?}"  }
             .line-resolve-all{ "v-show" => "discussionCount > 0",
-              ":class" => "{ 'has-next-btn': !loggedOut && resolved !== discussionCount }" }
+              ":class" => "{ 'has-next-btn': !loggedOut && resolvedDiscussionCount !== discussionCount }" }
               %span.line-resolve-btn.is-disabled{ type: "button",
-                  ":class" => "{ 'is-active': resolved === discussionCount }" }
+                  ":class" => "{ 'is-active': resolvedDiscussionCount === discussionCount }" }
                 = icon("check")
               %span.line-resolve-text
-                {{ resolved }}/{{ discussionCount }} {{ discussionCount | pluralize 'discussion' }} resolved
+                {{ resolvedDiscussionCount }}/{{ discussionCount }} {{ discussionCount | pluralize 'discussion' }} resolved
             = render "discussions/jump_to_next"
 
       .tab-content#diff-notes-app