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()

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);
       }
     },
   },

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 }) => {

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 () => {};

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

changelogs/unreleased/48798-keybinding-mr-diff.yml

+---
+title: Next/previous navigation between files in MR review
+merge_request: 25355
+author:
+type: added
\ No newline at end of file

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
 

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,13 +46,18 @@ 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);
+  });
+
+  describe('row highlighting', () => {
+    beforeEach(() => {
+      window.location.hash = 'ABC_123';
     });
 
     it('sets highlighted row if hash exists in location object', done => {
@@ -68,6 +73,20 @@ describe('diffs/components/app', () => {
       });
     });
 
+    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();
+      });
+    });
+  });
+
   describe('resizable', () => {
     afterEach(() => {
       localStorage.removeItem('mr_tree_list_width');
@@ -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);
     });
   });
 });

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', () => {

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);
+    });
+  });
 });