Set up basic keyboard next/previous navigation in diff list

Mousetrap is used as the help-tool to listen to keystrokes

Added currentDiffIndex getter to store that holds
the index of currently active diff file in the list

Instead of computing it on the component, we will take advantage of it
being available for all components in DiffsApp

Testing keyboard navigation and jumpToFile()

9 files changed, 287 insertions, 39 deletions

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index 8f47931d14a..1fc2b7fe859 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -5,6 +5,7 @@ import { __ } from '~/locale';
 import createFlash from '~/flash';
 import { GlLoadingIcon } from '@gitlab/ui';
 import PanelResizer from '~/vue_shared/components/panel_resizer.vue';
+import Mousetrap from 'mousetrap';
 import eventHub from '../../notes/event_hub';
 import CompareVersions from './compare_versions.vue';
 import DiffFile from './diff_file.vue';
@@ -87,7 +88,7 @@ export default {
       emailPatchPath: state => state.diffs.emailPatchPath,
     }),
     ...mapState('diffs', ['showTreeList', 'isLoading', 'startVersion']),
-    ...mapGetters('diffs', ['isParallelView']),
+    ...mapGetters('diffs', ['isParallelView', 'currentDiffIndex']),
     ...mapGetters(['isNotesFetched', 'getNoteableData']),
     targetBranch() {
       return {
@@ -149,6 +150,7 @@ export default {
   },
   beforeDestroy() {
     eventHub.$off('fetchDiffData', this.fetchData);
+    this.removeEventListeners();
   },
   methods: {
     ...mapActions(['startTaskList']),
@@ -159,6 +161,7 @@ export default {
       'assignDiscussionsToDiff',
       'setHighlightedRow',
       'cacheTreeListWidth',
+      'scrollToFile',
     ]),
     fetchData() {
       this.fetchDiffFiles()
@@ -197,7 +200,35 @@ export default {
         this.$nextTick(() => {
           window.mrTabs.resetViewContainer();
           window.mrTabs.expandViewContainer(this.showTreeList);
+          this.setEventListeners();
         });
+      } else {
+        this.removeEventListeners();
+      }
+    },
+    setEventListeners() {
+      Mousetrap.bind(['[', 'k', ']', 'j'], (e, combo) => {
+        switch (combo) {
+          case '[':
+          case 'k':
+            this.jumpToFile(-1);
+            break;
+          case ']':
+          case 'j':
+            this.jumpToFile(+1);
+            break;
+          default:
+            break;
+        }
+      });
+    },
+    removeEventListeners() {
+      Mousetrap.unbind(['[', 'k', ']', 'j']);
+    },
+    jumpToFile(step) {
+      const targetIndex = this.currentDiffIndex + step;
+      if (targetIndex >= 0 && targetIndex < this.diffFiles.length) {
+        this.scrollToFile(this.diffFiles[targetIndex].file_path);
       }
     },
   },
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index 82ff2e3be76..c40775c3259 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -52,7 +52,9 @@ export const fetchDiffFiles = ({ state, commit }) => {
 };
 
 export const setHighlightedRow = ({ commit }, lineCode) => {
+  const fileHash = lineCode.split('_')[0];
   commit(types.SET_HIGHLIGHTED_ROW, lineCode);
+  commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash);
 };
 
 // This is adding line discussions to the actual lines in the diff tree
@@ -262,8 +264,6 @@ export const scrollToFile = ({ state, commit }, path) => {
   document.location.hash = fileHash;
 
   commit(types.UPDATE_CURRENT_DIFF_FILE_ID, fileHash);
-
-  setTimeout(() => commit(types.UPDATE_CURRENT_DIFF_FILE_ID, ''), 1000);
 };
 
 export const toggleShowTreeList = ({ commit, state }) => {
diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js
index 4e7e5306995..bc27e263bff 100644
--- a/app/assets/javascripts/diffs/store/getters.js
+++ b/app/assets/javascripts/diffs/store/getters.js
@@ -100,5 +100,12 @@ export const diffFilesLength = state => state.diffFiles.length;
 export const getCommentFormForDiffFile = state => fileHash =>
   state.commentForms.find(form => form.fileHash === fileHash);
 
+/**
+ * Returns index of a currently selected diff in diffFiles
+ * @returns {number}
+ */
+export const currentDiffIndex = state =>
+  Math.max(0, state.diffFiles.findIndex(diff => diff.file_hash === state.currentDiffFileId));
+
 // prevent babel-plugin-rewire from generating an invalid default during karma tests
 export default () => {};
diff --git a/app/views/help/_shortcuts.html.haml b/app/views/help/_shortcuts.html.haml
index 28ffb2dd63c..efb3815b257 100644
--- a/app/views/help/_shortcuts.html.haml
+++ b/app/views/help/_shortcuts.html.haml
@@ -356,6 +356,18 @@
                   %td.shortcut
                     %kbd l
                   %td Change Label
+                %tr
+                  %td.shortcut
+                    %kbd ]
+                    \/
+                    %kbd j
+                  %td Move to next file
+                %tr
+                  %td.shortcut
+                    %kbd [
+                    \/
+                    %kbd k
+                  %td Move to previous file
               %tbody.hidden-shortcut{ style: 'display:none' }
                 %tr
                   %th
diff --git a/changelogs/unreleased/48798-keybinding-mr-diff.yml b/changelogs/unreleased/48798-keybinding-mr-diff.yml
new file mode 100644
index 00000000000..3ef3f07f27c
--- /dev/null
+++ b/changelogs/unreleased/48798-keybinding-mr-diff.yml
@@ -0,0 +1,5 @@
+---
+title: Next/previous navigation between files in MR review
+merge_request: 25355
+author:
+type: added
\ No newline at end of file
diff --git a/doc/workflow/shortcuts.md b/doc/workflow/shortcuts.md
index 7359e1c6119..6ed6b0bda66 100644
--- a/doc/workflow/shortcuts.md
+++ b/doc/workflow/shortcuts.md
@@ -82,6 +82,8 @@ You can see GitLab's keyboard shortcuts by using 'shift + ?'
 | <kbd>r</kbd> | Reply (quoting selected text) |
 | <kbd>e</kbd> | Edit issue/merge request |
 | <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 |
 
 ## Wiki pages
 
diff --git a/spec/javascripts/diffs/components/app_spec.js b/spec/javascripts/diffs/components/app_spec.js
index 5abdfe695d0..bce6113f75a 100644
--- a/spec/javascripts/diffs/components/app_spec.js
+++ b/spec/javascripts/diffs/components/app_spec.js
@@ -4,12 +4,13 @@ import { TEST_HOST } from 'spec/test_constants';
 import App from '~/diffs/components/app.vue';
 import NoChanges from '~/diffs/components/no_changes.vue';
 import DiffFile from '~/diffs/components/diff_file.vue';
+import Mousetrap from 'mousetrap';
 import createDiffsStore from '../create_diffs_store';
 
 describe('diffs/components/app', () => {
   const oldMrTabs = window.mrTabs;
   let store;
-  let vm;
+  let wrapper;
 
   function createComponent(props = {}, extendStore = () => {}) {
     const localVue = createLocalVue();
@@ -21,7 +22,7 @@ describe('diffs/components/app', () => {
 
     extendStore(store);
 
-    vm = shallowMount(localVue.extend(App), {
+    wrapper = shallowMount(localVue.extend(App), {
       localVue,
       propsData: {
         endpoint: `${TEST_HOST}/diff/endpoint`,
@@ -38,7 +39,6 @@ describe('diffs/components/app', () => {
     // setup globals (needed for component to mount :/)
     window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']);
     window.mrTabs.expandViewContainer = jasmine.createSpy();
-    window.location.hash = 'ABC_123';
   });
 
   afterEach(() => {
@@ -46,25 +46,44 @@ describe('diffs/components/app', () => {
     window.mrTabs = oldMrTabs;
 
     // reset component
-    vm.destroy();
+    wrapper.destroy();
   });
 
   it('does not show commit info', () => {
     createComponent();
 
-    expect(vm.contains('.blob-commit-info')).toBe(false);
+    expect(wrapper.contains('.blob-commit-info')).toBe(false);
   });
 
-  it('sets highlighted row if hash exists in location object', done => {
-    createComponent({
-      shouldShow: true,
+  describe('row highlighting', () => {
+    beforeEach(() => {
+      window.location.hash = 'ABC_123';
     });
 
-    // Component uses $nextTick so we wait until that has finished
-    setTimeout(() => {
-      expect(store.state.diffs.highlightedRow).toBe('ABC_123');
+    it('sets highlighted row if hash exists in location object', done => {
+      createComponent({
+        shouldShow: true,
+      });
 
-      done();
+      // Component uses $nextTick so we wait until that has finished
+      setTimeout(() => {
+        expect(store.state.diffs.highlightedRow).toBe('ABC_123');
+
+        done();
+      });
+    });
+
+    it('marks current diff file based on currently highlighted row', done => {
+      createComponent({
+        shouldShow: true,
+      });
+
+      // Component uses $nextTick so we wait until that has finished
+      setTimeout(() => {
+        expect(store.state.diffs.currentDiffFileId).toBe('ABC');
+
+        done();
+      });
     });
   });
 
@@ -76,7 +95,7 @@ describe('diffs/components/app', () => {
     it('sets initial width when no localStorage has been set', () => {
       createComponent();
 
-      expect(vm.vm.treeWidth).toEqual(320);
+      expect(wrapper.vm.treeWidth).toEqual(320);
     });
 
     it('sets initial width to localStorage size', () => {
@@ -84,13 +103,26 @@ describe('diffs/components/app', () => {
 
       createComponent();
 
-      expect(vm.vm.treeWidth).toEqual(200);
+      expect(wrapper.vm.treeWidth).toEqual(200);
     });
 
     it('sets width of tree list', () => {
       createComponent();
 
-      expect(vm.find('.js-diff-tree-list').element.style.width).toEqual('320px');
+      expect(wrapper.find('.js-diff-tree-list').element.style.width).toEqual('320px');
+    });
+  });
+
+  it('marks current diff file based on currently highlighted row', done => {
+    createComponent({
+      shouldShow: true,
+    });
+
+    // Component uses $nextTick so we wait until that has finished
+    setTimeout(() => {
+      expect(store.state.diffs.currentDiffFileId).toBe('ABC');
+
+      done();
     });
   });
 
@@ -98,7 +130,7 @@ describe('diffs/components/app', () => {
     it('renders empty state when no diff files exist', () => {
       createComponent();
 
-      expect(vm.contains(NoChanges)).toBe(true);
+      expect(wrapper.contains(NoChanges)).toBe(true);
     });
 
     it('does not render empty state when diff files exist', () => {
@@ -108,8 +140,8 @@ describe('diffs/components/app', () => {
         });
       });
 
-      expect(vm.contains(NoChanges)).toBe(false);
-      expect(vm.findAll(DiffFile).length).toBe(1);
+      expect(wrapper.contains(NoChanges)).toBe(false);
+      expect(wrapper.findAll(DiffFile).length).toBe(1);
     });
 
     it('does not render empty state when versions match', () => {
@@ -118,7 +150,161 @@ describe('diffs/components/app', () => {
         store.state.diffs.mergeRequestDiff = { version_index: 1 };
       });
 
-      expect(vm.contains(NoChanges)).toBe(false);
+      expect(wrapper.contains(NoChanges)).toBe(false);
+    });
+  });
+
+  describe('keyboard shortcut navigation', () => {
+    const mappings = {
+      '[': -1,
+      k: -1,
+      ']': +1,
+      j: +1,
+    };
+    let spy;
+
+    describe('visible app', () => {
+      beforeEach(() => {
+        spy = jasmine.createSpy('spy');
+
+        createComponent({
+          shouldShow: true,
+        });
+        wrapper.setMethods({
+          jumpToFile: spy,
+        });
+      });
+
+      it('calls `jumpToFile()` with correct parameter whenever pre-defined key is pressed', done => {
+        wrapper.vm
+          .$nextTick()
+          .then(() => {
+            Object.keys(mappings).forEach(function(key) {
+              Mousetrap.trigger(key);
+
+              expect(spy.calls.mostRecent().args).toEqual([mappings[key]]);
+            });
+
+            expect(spy.calls.count()).toEqual(Object.keys(mappings).length);
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+
+      it('does not call `jumpToFile()` when unknown key is pressed', done => {
+        wrapper.vm
+          .$nextTick()
+          .then(() => {
+            Mousetrap.trigger('d');
+
+            expect(spy).not.toHaveBeenCalled();
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+
+    describe('hideen app', () => {
+      beforeEach(() => {
+        spy = jasmine.createSpy('spy');
+
+        createComponent({
+          shouldShow: false,
+        });
+        wrapper.setMethods({
+          jumpToFile: spy,
+        });
+      });
+
+      it('stops calling `jumpToFile()` when application is hidden', done => {
+        wrapper.vm
+          .$nextTick()
+          .then(() => {
+            Object.keys(mappings).forEach(function(key) {
+              Mousetrap.trigger(key);
+
+              expect(spy).not.toHaveBeenCalled();
+            });
+          })
+          .then(done)
+          .catch(done.fail);
+      });
+    });
+  });
+
+  describe('jumpToFile', () => {
+    let spy;
+
+    beforeEach(() => {
+      spy = jasmine.createSpy();
+
+      createComponent({}, () => {
+        store.state.diffs.diffFiles = [
+          { file_hash: '111', file_path: '111.js' },
+          { file_hash: '222', file_path: '222.js' },
+          { file_hash: '333', file_path: '333.js' },
+        ];
+      });
+
+      wrapper.setMethods({
+        scrollToFile: spy,
+      });
+    });
+
+    afterEach(() => {
+      wrapper.destroy();
+    });
+
+    it('jumps to next and previous files in the list', done => {
+      wrapper.vm
+        .$nextTick()
+        .then(() => {
+          wrapper.vm.jumpToFile(+1);
+
+          expect(spy.calls.mostRecent().args).toEqual(['222.js']);
+          store.state.diffs.currentDiffFileId = '222';
+          wrapper.vm.jumpToFile(+1);
+
+          expect(spy.calls.mostRecent().args).toEqual(['333.js']);
+          store.state.diffs.currentDiffFileId = '333';
+          wrapper.vm.jumpToFile(-1);
+
+          expect(spy.calls.mostRecent().args).toEqual(['222.js']);
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('does not jump to previous file from the first one', done => {
+      wrapper.vm
+        .$nextTick()
+        .then(() => {
+          store.state.diffs.currentDiffFileId = '333';
+
+          expect(wrapper.vm.currentDiffIndex).toEqual(2);
+
+          wrapper.vm.jumpToFile(+1);
+
+          expect(wrapper.vm.currentDiffIndex).toEqual(2);
+          expect(spy).not.toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
+    });
+
+    it('does not jump to next file from the last one', done => {
+      wrapper.vm
+        .$nextTick()
+        .then(() => {
+          expect(wrapper.vm.currentDiffIndex).toEqual(0);
+
+          wrapper.vm.jumpToFile(-1);
+
+          expect(wrapper.vm.currentDiffIndex).toEqual(0);
+          expect(spy).not.toHaveBeenCalled();
+        })
+        .then(done)
+        .catch(done.fail);
     });
   });
 });
diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js
index acff80bca62..e47c7906fcb 100644
--- a/spec/javascripts/diffs/store/actions_spec.js
+++ b/spec/javascripts/diffs/store/actions_spec.js
@@ -100,9 +100,10 @@ describe('DiffsStoreActions', () => {
   });
 
   describe('setHighlightedRow', () => {
-    it('should set lineHash and fileHash of highlightedRow', () => {
+    it('should mark currently selected diff and set lineHash and fileHash of highlightedRow', () => {
       testAction(setHighlightedRow, 'ABC_123', {}, [
         { type: types.SET_HIGHLIGHTED_ROW, payload: 'ABC_123' },
+        { type: types.UPDATE_CURRENT_DIFF_FILE_ID, payload: 'ABC' },
       ]);
     });
   });
@@ -713,22 +714,6 @@ describe('DiffsStoreActions', () => {
 
       expect(commit).toHaveBeenCalledWith(types.UPDATE_CURRENT_DIFF_FILE_ID, 'test');
     });
-
-    it('resets currentDiffId after timeout', () => {
-      const state = {
-        treeEntries: {
-          path: {
-            fileHash: 'test',
-          },
-        },
-      };
-
-      scrollToFile({ state, commit }, 'path');
-
-      jasmine.clock().tick(1000);
-
-      expect(commit.calls.argsFor(1)).toEqual([types.UPDATE_CURRENT_DIFF_FILE_ID, '']);
-    });
   });
 
   describe('toggleShowTreeList', () => {
diff --git a/spec/javascripts/diffs/store/getters_spec.js b/spec/javascripts/diffs/store/getters_spec.js
index 0ab88e6b2aa..eab5703dfb2 100644
--- a/spec/javascripts/diffs/store/getters_spec.js
+++ b/spec/javascripts/diffs/store/getters_spec.js
@@ -270,4 +270,24 @@ describe('Diffs Module Getters', () => {
       expect(getters.diffFilesLength(localState)).toBe(2);
     });
   });
+
+  describe('currentDiffIndex', () => {
+    it('returns index of currently selected diff in diffList', () => {
+      localState.diffFiles = [{ file_hash: '111' }, { file_hash: '222' }, { file_hash: '333' }];
+      localState.currentDiffFileId = '222';
+
+      expect(getters.currentDiffIndex(localState)).toEqual(1);
+
+      localState.currentDiffFileId = '333';
+
+      expect(getters.currentDiffIndex(localState)).toEqual(2);
+    });
+
+    it('returns 0 if no diff is selected yet or diff is not found', () => {
+      localState.diffFiles = [{ file_hash: '111' }, { file_hash: '222' }, { file_hash: '333' }];
+      localState.currentDiffFileId = '';
+
+      expect(getters.currentDiffIndex(localState)).toEqual(0);
+    });
+  });
 });