Merge branch '48798-keybinding-mr-diff' into 'master'

Resolve "Add keybinding for next/previous file in merge request diff"

See merge request gitlab-org/gitlab-ce!25355

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

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