diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index 8434ef5cc7a1d942df0c58c55258f38e9b6954c2..6695d9fe96cea5a6161877ae46032f2571c625e6 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/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) {
diff --git a/app/assets/javascripts/mr_notes/init_notes.js b/app/assets/javascripts/mr_notes/init_notes.js
index 842a209a54521072d38544d5c7e18b1705ae0aac..8caac68e0d4883f01dde775f11f4a806ccd4d8ab 100644
--- a/app/assets/javascripts/mr_notes/init_notes.js
+++ b/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,
+ },
+ }),
+ ]);
},
});
};
diff --git a/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue b/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue
new file mode 100644
index 0000000000000000000000000000000000000000..5fc2b6ba04c0ebf37d895fa521900a4667a64766
--- /dev/null
+++ b/app/assets/javascripts/notes/components/discussion_keyboard_navigator.vue
@@ -0,0 +1,47 @@
+<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>
diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js
index 8aa8f5037b3fd1917a27e0751619294c2113ee79..52410f18d4aa278e46ef3dfc1827642a82b3d586 100644
--- a/app/assets/javascripts/notes/stores/getters.js
+++ b/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) {
diff --git a/app/views/help/_shortcuts.html.haml b/app/views/help/_shortcuts.html.haml
index 46d7c367aa7acb22f6e664f462965687ab42d97e..0624f66b3a52678a83d946ececcf43bb61ffd41d 100644
--- a/app/views/help/_shortcuts.html.haml
+++ b/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
diff --git a/changelogs/unreleased/59590-keyboard-shortcut-for-jump-to-next-unresolved-discussion.yml b/changelogs/unreleased/59590-keyboard-shortcut-for-jump-to-next-unresolved-discussion.yml
new file mode 100644
index 0000000000000000000000000000000000000000..02e81c7fc8785f33246ec27bb51a313f5d782e9a
--- /dev/null
+++ b/changelogs/unreleased/59590-keyboard-shortcut-for-jump-to-next-unresolved-discussion.yml
@@ -0,0 +1,5 @@
+---
+title: Resolve Keyboard shortcut for jump to NEXT unresolved discussion
+merge_request: 30144
+author:
+type: added
diff --git a/doc/user/discussions/index.md b/doc/user/discussions/index.md
index 3cb765c0463e87af8c94a72003e14030b41af345..f844f56557fee5e57d5d0b147def0ba68e68f1a7 100644
--- a/doc/user/discussions/index.md
+++ b/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.
+
### Marking a comment or thread as resolved
diff --git a/doc/workflow/shortcuts.md b/doc/workflow/shortcuts.md
index d61d7eafd182c834faf063ecbc2966f3ce5bf707..5d08bf5e77d1c84df26c63ed8874c37b46fb555f 100644
--- a/doc/workflow/shortcuts.md
+++ b/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)**
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 7dd6e0f2a937d555243973f964d2bc99f5dc8d0a..e8a6ab31814ec1315e6ab5784d47059995c2b6a3 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -6952,6 +6952,12 @@ msgstr ""
msgid "Move this issue to another project."
msgstr ""
+msgid "Move to next unresolved discussion"
+msgstr ""
+
+msgid "Move to previous unresolved discussion"
+msgstr ""
+
msgid "MoveIssue|Cannot move issue due to insufficient permissions!"
msgstr ""
diff --git a/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js b/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js
new file mode 100644
index 0000000000000000000000000000000000000000..6d50713999ddc05ada7ae57fe7d1adaee3b20e43
--- /dev/null
+++ b/spec/frontend/notes/components/discussion_keyboard_navigator_spec.js
@@ -0,0 +1,77 @@
+/* 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);
+ });
+ });
+});
diff --git a/spec/javascripts/notes/stores/getters_spec.js b/spec/javascripts/notes/stores/getters_spec.js
index c3ed079e33bca9374086f19eed3fc0f16eb628fe..71dcba114a9cf591533b4564a702e0a16063f97b 100644
--- a/spec/javascripts/notes/stores/getters_spec.js
+++ b/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'],