Merge branch 'ph/342849/fixUnresolvedDiscussionsDiffsJumping' into 'master'

Fix jump to next discussion on diffs tab not working

See merge request gitlab-org/gitlab!72278

app/assets/javascripts/diffs/components/app.vue

@@ -570,7 +570,7 @@ export default {
     jumpToFile(step) {
       const targetIndex = this.currentDiffIndex + step;
       if (targetIndex >= 0 && targetIndex < this.diffFiles.length) {
-        this.scrollToFile(this.diffFiles[targetIndex].file_path);
+        this.scrollToFile({ path: this.diffFiles[targetIndex].file_path });
       }
     },
     setTreeDisplay() {

app/assets/javascripts/diffs/components/tree_list.vue

@@ -98,7 +98,7 @@ export default {
           :file-row-component="$options.DiffFileRow"
           :current-diff-file-id="currentDiffFileId"
           @toggleTreeOpen="toggleTreeOpen"
-          @clickFile="scrollToFile"
+          @clickFile="(path) => scrollToFile({ path })"
         />
       </template>
       <p v-else class="prepend-top-20 append-bottom-20 text-center">

app/assets/javascripts/diffs/index.js

@@ -138,7 +138,7 @@ export default function initDiffsApp(store) {
         ...mapActions('diffs', ['toggleFileFinder', 'scrollToFile']),
         openFile(file) {
           window.mrTabs.tabShown('diffs');
-          this.scrollToFile(file.path);
+          this.scrollToFile({ path: file.path });
         },
       },
       render(createElement) {

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

@@ -518,7 +518,7 @@ export const toggleActiveFileByHash = ({ commit }, hash) => {
   commit(types.VIEW_DIFF_FILE, hash);
 };
 
-export const scrollToFile = ({ state, commit, getters }, path) => {
+export const scrollToFile = ({ state, commit, getters }, { path, setHash = true }) => {
   if (!state.treeEntries[path]) return;
 
   const { fileHash } = state.treeEntries[path];
@@ -528,9 +528,11 @@ export const scrollToFile = ({ state, commit, getters }, path) => {
   if (getters.isVirtualScrollingEnabled) {
     eventHub.$emit('scrollToFileHash', fileHash);
 
-    setTimeout(() => {
-      window.history.replaceState(null, null, `#${fileHash}`);
-    });
+    if (setHash) {
+      setTimeout(() => {
+        window.history.replaceState(null, null, `#${fileHash}`);
+      });
+    }
   } else {
     document.location.hash = fileHash;
 

app/assets/javascripts/lib/utils/common_utils.js

@@ -220,16 +220,16 @@ export const scrollToElement = (element, options = {}) => {
     // In the previous implementation, jQuery naturally deferred this scrolling.
     // Unfortunately, we're quite coupled to this implementation detail now.
     defer(() => {
-      const { duration = 200, offset = 0 } = options;
+      const { duration = 200, offset = 0, behavior = duration ? 'smooth' : 'auto' } = options;
       const y = el.getBoundingClientRect().top + window.pageYOffset + offset - contentTop();
-      window.scrollTo({ top: y, behavior: duration ? 'smooth' : 'auto' });
+      window.scrollTo({ top: y, behavior });
     });
   }
 };
 
-export const scrollToElementWithContext = (element) => {
+export const scrollToElementWithContext = (element, options) => {
   const offsetMultiplier = -0.1;
-  return scrollToElement(element, { offset: window.innerHeight * offsetMultiplier });
+  return scrollToElement(element, { ...options, offset: window.innerHeight * offsetMultiplier });
 };
 
 /**

app/assets/javascripts/notes/mixins/discussion_navigation.js

@@ -2,6 +2,8 @@ import { mapGetters, mapActions, mapState } from 'vuex';
 import { scrollToElementWithContext, scrollToElement } from '~/lib/utils/common_utils';
 import eventHub from '../event_hub';
 
+const isDiffsVirtualScrollingEnabled = () => window.gon?.features?.diffsVirtualScrolling;
+
 /**
  * @param {string} selector
  * @returns {boolean}
@@ -11,7 +13,9 @@ function scrollTo(selector, { withoutContext = false } = {}) {
   const scrollFunction = withoutContext ? scrollToElement : scrollToElementWithContext;
 
   if (el) {
-    scrollFunction(el);
+    scrollFunction(el, {
+      behavior: isDiffsVirtualScrollingEnabled() ? 'auto' : 'smooth',
+    });
     return true;
   }
 
@@ -81,8 +85,15 @@ function handleDiscussionJump(self, fn, discussionId = self.currentDiscussionId)
   const discussion = self.getDiscussion(targetId);
   const discussionFilePath = discussion?.diff_file?.file_path;
 
+  if (isDiffsVirtualScrollingEnabled()) {
+    window.location.hash = '';
+  }
+
   if (discussionFilePath) {
-    self.scrollToFile(discussionFilePath);
+    self.scrollToFile({
+      path: discussionFilePath,
+      setHash: !isDiffsVirtualScrollingEnabled(),
+    });
   }
 
   self.$nextTick(() => {

spec/frontend/diffs/components/app_spec.js

@@ -388,15 +388,24 @@ describe('diffs/components/app', () => {
 
       wrapper.vm.jumpToFile(+1);
 
-      expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual(['diffs/scrollToFile', '222.js']);
+      expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual([
+        'diffs/scrollToFile',
+        { path: '222.js' },
+      ]);
       store.state.diffs.currentDiffFileId = '222';
       wrapper.vm.jumpToFile(+1);
 
-      expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual(['diffs/scrollToFile', '333.js']);
+      expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual([
+        'diffs/scrollToFile',
+        { path: '333.js' },
+      ]);
       store.state.diffs.currentDiffFileId = '333';
       wrapper.vm.jumpToFile(-1);
 
-      expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual(['diffs/scrollToFile', '222.js']);
+      expect(spy.mock.calls[spy.mock.calls.length - 1]).toEqual([
+        'diffs/scrollToFile',
+        { path: '222.js' },
+      ]);
     });
 
     it('does not jump to previous file from the first one', async () => {

spec/frontend/diffs/components/tree_list_spec.js

@@ -113,7 +113,9 @@ describe('Diffs tree list component', () => {
 
       wrapper.find('.file-row').trigger('click');
 
-      expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', 'app/index.js');
+      expect(wrapper.vm.$store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', {
+        path: 'app/index.js',
+      });
     });
 
     it('renders as file list when renderTreeList is false', () => {

spec/frontend/diffs/store/actions_spec.js

@@ -890,7 +890,7 @@ describe('DiffsStoreActions', () => {
         },
       };
 
-      scrollToFile({ state, commit, getters }, 'path');
+      scrollToFile({ state, commit, getters }, { path: 'path' });
 
       expect(document.location.hash).toBe('#test');
     });
@@ -904,7 +904,7 @@ describe('DiffsStoreActions', () => {
         },
       };
 
-      scrollToFile({ state, commit, getters }, 'path');
+      scrollToFile({ state, commit, getters }, { path: 'path' });
 
       expect(commit).toHaveBeenCalledWith(types.VIEW_DIFF_FILE, 'test');
     });

spec/frontend/lib/utils/common_utils_spec.js

@@ -279,6 +279,14 @@ describe('common_utils', () => {
           top: elementTopWithContext,
         });
       });
+
+      it('passes through behaviour', () => {
+        commonUtils.scrollToElementWithContext(`#${id}`, { behavior: 'smooth' });
+        expect(window.scrollTo).toHaveBeenCalledWith({
+          behavior: 'smooth',
+          top: elementTopWithContext,
+        });
+      });
     });
   });
 

spec/frontend/notes/mixins/discussion_navigation_spec.js

 import { shallowMount, createLocalVue } from '@vue/test-utils';
+import { nextTick } from 'vue';
 import Vuex from 'vuex';
 import { setHTMLFixture } from 'helpers/fixtures';
 import createEventHub from '~/helpers/event_hub_factory';
@@ -7,12 +8,15 @@ import eventHub from '~/notes/event_hub';
 import discussionNavigation from '~/notes/mixins/discussion_navigation';
 import notesModule from '~/notes/stores/modules';
 
+let scrollToFile;
 const discussion = (id, index) => ({
   id,
   resolvable: index % 2 === 0,
   active: true,
   notes: [{}],
   diff_discussion: true,
+  position: { new_line: 1, old_line: 1 },
+  diff_file: { file_path: 'test.js' },
 });
 const createDiscussions = () => [...'abcde'].map(discussion);
 const createComponent = () => ({
@@ -45,6 +49,7 @@ describe('Discussion navigation mixin', () => {
     jest.spyOn(utils, 'scrollToElement');
 
     expandDiscussion = jest.fn();
+    scrollToFile = jest.fn();
     const { actions, ...notesRest } = notesModule();
     store = new Vuex.Store({
       modules: {
@@ -52,6 +57,10 @@ describe('Discussion navigation mixin', () => {
           ...notesRest,
           actions: { ...actions, expandDiscussion },
         },
+        diffs: {
+          namespaced: true,
+          actions: { scrollToFile },
+        },
       },
     });
     store.state.notes.discussions = createDiscussions();
@@ -136,6 +145,7 @@ describe('Discussion navigation mixin', () => {
         it('scrolls to element', () => {
           expect(utils.scrollToElement).toHaveBeenCalledWith(
             findDiscussion('div.discussion', expected),
+            { behavior: 'smooth' },
           );
         });
       });
@@ -163,6 +173,7 @@ describe('Discussion navigation mixin', () => {
 
           expect(utils.scrollToElementWithContext).toHaveBeenCalledWith(
             findDiscussion('ul.notes', expected),
+            { behavior: 'smooth' },
           );
         });
       });
@@ -203,10 +214,45 @@ describe('Discussion navigation mixin', () => {
           it('scrolls to discussion', () => {
             expect(utils.scrollToElement).toHaveBeenCalledWith(
               findDiscussion('div.discussion', expected),
+              { behavior: 'smooth' },
             );
           });
         });
       });
     });
+
+    describe.each`
+      diffsVirtualScrolling
+      ${false}
+      ${true}
+    `('virtual scrolling feature is $diffsVirtualScrolling', ({ diffsVirtualScrolling }) => {
+      beforeEach(async () => {
+        window.gon = { features: { diffsVirtualScrolling } };
+
+        jest.spyOn(store, 'dispatch');
+
+        store.state.notes.currentDiscussionId = 'a';
+        window.location.hash = 'test';
+        wrapper.vm.jumpToNextDiscussion();
+
+        await nextTick();
+      });
+
+      afterEach(() => {
+        window.gon = {};
+        window.location.hash = '';
+      });
+
+      it('resets location hash if diffsVirtualScrolling flag is true', () => {
+        expect(window.location.hash).toBe(diffsVirtualScrolling ? '' : '#test');
+      });
+
+      it(`calls scrollToFile with setHash as ${diffsVirtualScrolling ? 'false' : 'true'}`, () => {
+        expect(store.dispatch).toHaveBeenCalledWith('diffs/scrollToFile', {
+          path: 'test.js',
+          setHash: !diffsVirtualScrolling,
+        });
+      });
+    });
   });
 });