Automatic merge of gitlab-org/gitlab-ce master

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

@@ -109,7 +109,7 @@ export const toggleLineDiscussions = ({ commit }, options) => {
 export const renderFileForDiscussionId = ({ commit, rootState, state }, discussionId) => {
   const discussion = rootState.notes.discussions.find(d => d.id === discussionId);
 
-  if (discussion) {
+  if (discussion && discussion.diff_file) {
     const file = state.diffFiles.find(f => f.file_hash === discussion.diff_file.file_hash);
 
     if (file) {

app/assets/javascripts/mr_notes/init_notes.js

@@ -3,6 +3,7 @@ import Vue from 'vue';
 import { mapActions, mapState, mapGetters } from 'vuex';
 import store from 'ee_else_ce/mr_notes/stores';
 import notesApp from '../notes/components/notes_app.vue';
+import discussionKeyboardNavigator from '../notes/components/discussion_keyboard_navigator.vue';
 
 export default () => {
   // eslint-disable-next-line no-new
@@ -56,15 +57,19 @@ export default () => {
       },
     },
     render(createElement) {
-      return createElement('notes-app', {
-        props: {
-          noteableData: this.noteableData,
-          notesData: this.notesData,
-          userData: this.currentUserData,
-          shouldShow: this.activeTab === 'show',
-          helpPagePath: this.helpPagePath,
-        },
-      });
+      const isDiffView = this.activeTab === 'diffs';
+
+      return createElement(discussionKeyboardNavigator, { props: { isDiffView } }, [
+        createElement('notes-app', {
+          props: {
+            noteableData: this.noteableData,
+            notesData: this.notesData,
+            userData: this.currentUserData,
+            shouldShow: this.activeTab === 'show',
+            helpPagePath: this.helpPagePath,
+          },
+        }),
+      ]);
     },
   });
 };

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

+<script>
+/* global Mousetrap */
+import 'mousetrap';
+import { mapGetters, mapActions } from 'vuex';
+import discussionNavigation from '~/notes/mixins/discussion_navigation';
+
+export default {
+  mixins: [discussionNavigation],
+  props: {
+    isDiffView: {
+      type: Boolean,
+      required: false,
+      default: false,
+    },
+  },
+  data() {
+    return {
+      currentDiscussionId: null,
+    };
+  },
+  computed: {
+    ...mapGetters(['nextUnresolvedDiscussionId', 'previousUnresolvedDiscussionId']),
+  },
+  mounted() {
+    Mousetrap.bind('n', () => this.jumpToNextDiscussion());
+    Mousetrap.bind('p', () => this.jumpToPreviousDiscussion());
+  },
+  methods: {
+    ...mapActions(['expandDiscussion']),
+    jumpToNextDiscussion() {
+      const nextId = this.nextUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
+
+      this.jumpToDiscussion(nextId);
+      this.currentDiscussionId = nextId;
+    },
+    jumpToPreviousDiscussion() {
+      const prevId = this.previousUnresolvedDiscussionId(this.currentDiscussionId, this.isDiffView);
+
+      this.jumpToDiscussion(prevId);
+      this.currentDiscussionId = prevId;
+    },
+  },
+  render() {
+    return this.$slots.default;
+  },
+};
+</script>

app/assets/javascripts/notes/stores/getters.js

@@ -183,6 +183,15 @@ export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, dif
   return slicedIds.length ? idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0] : idsOrdered[0];
 };
 
+export const previousUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => {
+  const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder);
+  const currentIndex = idsOrdered.indexOf(discussionId);
+  const slicedIds = idsOrdered.slice(currentIndex - 1, currentIndex);
+
+  // Get the last ID if there is none after the currentIndex
+  return slicedIds.length ? slicedIds[0] : idsOrdered[idsOrdered.length - 1];
+};
+
 // @param {Boolean} diffOrder - is ordered by diff?
 export const firstUnresolvedDiscussionId = (state, getters) => diffOrder => {
   if (diffOrder) {

app/views/help/_shortcuts.html.haml

@@ -332,7 +332,7 @@
                   %td.shortcut
                     %kbd l
                   %td Change Label
-              %tbody.hidden-shortcut{ style: 'display:none' }
+              %tbody.hidden-shortcut.merge_requests{ style: 'display:none' }
                 %tr
                   %th
                   %th Merge Requests
@@ -368,6 +368,14 @@
                     \/
                     %kbd k
                   %td Move to previous file
+                %tr
+                  %td.shortcut
+                    %kbd n
+                  %td= _("Move to next unresolved discussion")
+                %tr
+                  %td.shortcut
+                    %kbd p
+                  %td= _("Move to previous unresolved discussion")
               %tbody.hidden-shortcut{ style: 'display:none' }
                 %tr
                   %th

changelogs/unreleased/59590-keyboard-shortcut-for-jump-to-next-unresolved-discussion.yml

+---
+title: Resolve Keyboard shortcut for jump to NEXT unresolved discussion
+merge_request: 30144
+author:
+type: added

doc/user/discussions/index.md

@@ -88,6 +88,11 @@ Jump button next to the Reply field on a thread.
 You can also jump to the first unresolved thread from the button next to the
 resolved threads tracker.
 
+You can also use keyboard shortcuts to navigate among threads:
+
+- Use <kbd>n</kbd> to jump to the next unresolved thread.
+- Use <kbd>p</kbd> to jump to the previous unresolved thread.
+
 !["8/9 threads resolved"](img/threads_resolved.png)
 
 ### Marking a comment or thread as resolved

doc/workflow/shortcuts.md

@@ -84,6 +84,8 @@ You can see GitLab's keyboard shortcuts by using <kbd>shift</kbd> + <kbd>?</kbd>
 | <kbd>l</kbd> | Change label |
 | <kbd>]</kbd> or <kbd>j</kbd> | Move to next file |
 | <kbd>[</kbd> or <kbd>k</kbd> | Move to previous file |
+| <kbd>n</kbd> | Move to next unresolved discussion     |
+| <kbd>p</kbd> | Move to previous unresolved discussion |
 
 ## Epics **(ULTIMATE)**
 

spec/frontend/notes/components/discussion_keyboard_navigator_spec.js

+/* global Mousetrap */
+import 'mousetrap';
+import { shallowMount, createLocalVue } from '@vue/test-utils';
+import Vuex from 'vuex';
+import DiscussionKeyboardNavigator from '~/notes/components/discussion_keyboard_navigator.vue';
+import notesModule from '~/notes/stores/modules';
+
+const localVue = createLocalVue();
+localVue.use(Vuex);
+
+const NEXT_ID = 'abc123';
+const PREV_ID = 'def456';
+const NEXT_DIFF_ID = 'abc123_diff';
+const PREV_DIFF_ID = 'def456_diff';
+
+describe('notes/components/discussion_keyboard_navigator', () => {
+  let storeOptions;
+  let wrapper;
+  let store;
+
+  const createComponent = (options = {}) => {
+    store = new Vuex.Store(storeOptions);
+
+    wrapper = shallowMount(DiscussionKeyboardNavigator, {
+      localVue,
+      store,
+      ...options,
+    });
+
+    wrapper.vm.jumpToDiscussion = jest.fn();
+  };
+
+  beforeEach(() => {
+    const notes = notesModule();
+
+    notes.getters.nextUnresolvedDiscussionId = () => (currId, isDiff) =>
+      isDiff ? NEXT_DIFF_ID : NEXT_ID;
+    notes.getters.previousUnresolvedDiscussionId = () => (currId, isDiff) =>
+      isDiff ? PREV_DIFF_ID : PREV_ID;
+
+    storeOptions = {
+      modules: {
+        notes,
+      },
+    };
+  });
+
+  afterEach(() => {
+    wrapper.destroy();
+    storeOptions = null;
+    store = null;
+  });
+
+  describe.each`
+    isDiffView | expectedNextId  | expectedPrevId
+    ${true}    | ${NEXT_DIFF_ID} | ${PREV_DIFF_ID}
+    ${false}   | ${NEXT_ID}      | ${PREV_ID}
+  `('when isDiffView is $isDiffView', ({ isDiffView, expectedNextId, expectedPrevId }) => {
+    beforeEach(() => {
+      createComponent({ propsData: { isDiffView } });
+    });
+
+    it('calls jumpToNextDiscussion when pressing `n`', () => {
+      Mousetrap.trigger('n');
+
+      expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(expectedNextId);
+      expect(wrapper.vm.currentDiscussionId).toEqual(expectedNextId);
+    });
+
+    it('calls jumpToPreviousDiscussion when pressing `p`', () => {
+      Mousetrap.trigger('p');
+
+      expect(wrapper.vm.jumpToDiscussion).toHaveBeenCalledWith(expectedPrevId);
+      expect(wrapper.vm.currentDiscussionId).toEqual(expectedPrevId);
+    });
+  });
+});

spec/javascripts/notes/stores/getters_spec.js

@@ -256,6 +256,54 @@ describe('Getters Notes Store', () => {
     });
   });
 
+  describe('previousUnresolvedDiscussionId', () => {
+    describe('with unresolved discussions', () => {
+      const localGetters = {
+        unresolvedDiscussionsIdsOrdered: () => ['123', '456', '789'],
+      };
+
+      it('with bogus returns falsey', () => {
+        expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('456');
+      });
+
+      [
+        { id: '123', expected: '789' },
+        { id: '456', expected: '123' },
+        { id: '789', expected: '456' },
+      ].forEach(({ id, expected }) => {
+        it(`with ${id}, returns previous value`, () => {
+          expect(getters.previousUnresolvedDiscussionId(state, localGetters)(id)).toBe(expected);
+        });
+      });
+    });
+
+    describe('with 1 unresolved discussion', () => {
+      const localGetters = {
+        unresolvedDiscussionsIdsOrdered: () => ['123'],
+      };
+
+      it('with bogus returns id', () => {
+        expect(getters.previousUnresolvedDiscussionId(state, localGetters)('bogus')).toBe('123');
+      });
+
+      it('with match, returns value', () => {
+        expect(getters.previousUnresolvedDiscussionId(state, localGetters)('123')).toEqual('123');
+      });
+    });
+
+    describe('with 0 unresolved discussions', () => {
+      const localGetters = {
+        unresolvedDiscussionsIdsOrdered: () => [],
+      };
+
+      it('returns undefined', () => {
+        expect(
+          getters.previousUnresolvedDiscussionId(state, localGetters)('bogus'),
+        ).toBeUndefined();
+      });
+    });
+  });
+
   describe('firstUnresolvedDiscussionId', () => {
     const localGetters = {
       unresolvedDiscussionsIdsByDate: ['123', '456'],