From cf9a17f3952daa42c9a183b1bc470dbce344cf99 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Tue, 3 Oct 2023 10:18:26 -0600
Subject: [PATCH 1/4] Fix scrolling to a note hash in single file mode

Changelog: fixed
---
 app/assets/javascripts/diffs/components/app.vue  | 13 ++++++++++++-
 app/assets/javascripts/diffs/constants.js        |  1 +
 app/assets/javascripts/diffs/store/actions.js    |  3 ++-
 spec/frontend/diffs/components/diff_file_spec.js |  4 +++-
 4 files changed, 18 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index a389ab8e35586c..1c3f56fe7edc7c 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -1,6 +1,7 @@
 <script>
 import { GlLoadingIcon, GlPagination, GlSprintf, GlAlert } from '@gitlab/ui';
 import { GlBreakpointInstance as bp } from '@gitlab/ui/dist/utils';
+import { debounce } from 'lodash';
 // eslint-disable-next-line no-restricted-imports
 import { mapState, mapGetters, mapActions } from 'vuex';
 import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
@@ -16,7 +17,8 @@ import {
 import { createAlert } from '~/alert';
 import { isSingleViewStyle } from '~/helpers/diffs_helper';
 import { helpPagePath } from '~/helpers/help_page_helper';
-import { parseBoolean } from '~/lib/utils/common_utils';
+import { parseBoolean, handleLocationHash } from '~/lib/utils/common_utils';
+import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants';
 import { Mousetrap } from '~/lib/mousetrap';
 import { updateHistory } from '~/lib/utils/url_utility';
 import { __ } from '~/locale';
@@ -39,6 +41,7 @@ import {
   TRACKING_SINGLE_FILE_MODE,
   TRACKING_MULTIPLE_FILES_MODE,
   EVT_MR_PREPARED,
+  EVT_DISCUSSIONS_ASSIGNED,
 } from '../constants';
 
 import { isCollapsed } from '../utils/diff_file';
@@ -358,6 +361,7 @@ export default {
     this.adjustView();
     this.subscribeToEvents();
 
+    this.slowHashHandler = debounce(handleLocationHash, DEFAULT_DEBOUNCE_AND_THROTTLE_MS);
     this.unwatchDiscussions = this.$watch(
       () => `${this.flatBlobsList.length}:${this.$store.state.notes.discussions.length}`,
       () => {
@@ -420,8 +424,10 @@ export default {
       diffsEventHub.$on('diffFilesModified', this.setDiscussions);
       diffsEventHub.$on('doneLoadingBatches', this.autoScroll);
       diffsEventHub.$on(EVT_MR_PREPARED, this.fetchData);
+      diffsEventHub.$on(EVT_DISCUSSIONS_ASSIGNED, this.handleHash);
     },
     unsubscribeFromEvents() {
+      diffsEventHub.$off(EVT_DISCUSSIONS_ASSIGNED, this.handleHash);
       diffsEventHub.$off(EVT_MR_PREPARED, this.fetchData);
       diffsEventHub.$off('doneLoadingBatches', this.autoScroll);
       diffsEventHub.$off('diffFilesModified', this.setDiscussions);
@@ -449,6 +455,11 @@ export default {
           .catch(() => {});
       }
     },
+    handleHash() {
+      if (this.viewDiffsFileByFile) {
+        this.slowHashHandler();
+      }
+    },
     navigateToDiffFileNumber(number) {
       this.navigateToDiffFileIndex(number - 1);
     },
diff --git a/app/assets/javascripts/diffs/constants.js b/app/assets/javascripts/diffs/constants.js
index 575cd05ceb898f..e48eb10753c308 100644
--- a/app/assets/javascripts/diffs/constants.js
+++ b/app/assets/javascripts/diffs/constants.js
@@ -82,6 +82,7 @@ export const RENAMED_DIFF_TRANSITIONS = {
 // MR Diffs known events
 export const EVT_MR_PREPARED = 'mr:asyncPreparationFinished';
 export const EVT_EXPAND_ALL_FILES = 'mr:diffs:expandAllFiles';
+export const EVT_DISCUSSIONS_ASSIGNED = 'mr:diffs:discussionsAssigned';
 export const EVT_PERF_MARK_FILE_TREE_START = 'mr:diffs:perf:fileTreeStart';
 export const EVT_PERF_MARK_FILE_TREE_END = 'mr:diffs:perf:fileTreeEnd';
 export const EVT_PERF_MARK_DIFF_FILES_START = 'mr:diffs:perf:filesStart';
diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js
index ed8ae795bdafcc..d86a88f97b8314 100644
--- a/app/assets/javascripts/diffs/store/actions.js
+++ b/app/assets/javascripts/diffs/store/actions.js
@@ -49,6 +49,7 @@ import {
   TRACKING_MULTIPLE_FILES_MODE,
   EVT_MR_PREPARED,
   FILE_DIFF_POSITION_TYPE,
+  EVT_DISCUSSIONS_ASSIGNED,
 } from '../constants';
 import {
   DISCUSSION_SINGLE_DIFF_FAILED,
@@ -413,7 +414,7 @@ export const assignDiscussionsToDiff = (
   }
 
   Vue.nextTick(() => {
-    notesEventHub.$emit('scrollToDiscussion');
+    eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
   });
 };
 
diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js
index 13efd3584b4158..34af3d72b04e1a 100644
--- a/spec/frontend/diffs/components/diff_file_spec.js
+++ b/spec/frontend/diffs/components/diff_file_spec.js
@@ -13,6 +13,7 @@ import DiffFileComponent from '~/diffs/components/diff_file.vue';
 import DiffFileHeaderComponent from '~/diffs/components/diff_file_header.vue';
 
 import {
+  EVT_DISCUSSIONS_ASSIGNED,
   EVT_EXPAND_ALL_FILES,
   EVT_PERF_MARK_DIFF_FILES_END,
   EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN,
@@ -271,9 +272,10 @@ describe('DiffFile', () => {
         await nextTick(); // Wait for the idleCallback
         await nextTick(); // Wait for nextTick inside postRender
 
-        expect(eventHub.$emit).toHaveBeenCalledTimes(2);
+        expect(eventHub.$emit).toHaveBeenCalledTimes(3);
         expect(eventHub.$emit).toHaveBeenCalledWith(EVT_PERF_MARK_FIRST_DIFF_FILE_SHOWN);
         expect(eventHub.$emit).toHaveBeenCalledWith(EVT_PERF_MARK_DIFF_FILES_END);
+        expect(eventHub.$emit).toHaveBeenCalledWith(EVT_DISCUSSIONS_ASSIGNED);
       });
     });
   });
-- 
GitLab


From eb2422d60e28c498853d69a4114593bc93064e2f Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Wed, 18 Oct 2023 09:15:06 -0600
Subject: [PATCH 2/4] Test that Vue calls through JS functions

---
 spec/frontend/diffs/components/app_spec.js | 24 ++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js
index 47fd96ff625860..e12b08d1106617 100644
--- a/spec/frontend/diffs/components/app_spec.js
+++ b/spec/frontend/diffs/components/app_spec.js
@@ -22,11 +22,13 @@ import CollapsedFilesWarning from '~/diffs/components/collapsed_files_warning.vu
 import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue';
 
 import eventHub from '~/diffs/event_hub';
+import { EVT_DISCUSSIONS_ASSIGNED } from '~/diffs/constants';
 
 import axios from '~/lib/utils/axios_utils';
 import { HTTP_STATUS_OK } from '~/lib/utils/http_status';
 import { Mousetrap } from '~/lib/mousetrap';
 import * as urlUtils from '~/lib/utils/url_utility';
+import * as commonUtils from '~/lib/utils/common_utils';
 import { stubPerformanceWebAPI } from 'helpers/performance';
 import createDiffsStore from '../create_diffs_store';
 import diffsMockData from '../mock_data/merge_request_diffs';
@@ -662,6 +664,12 @@ describe('diffs/components/app', () => {
   });
 
   describe('file-by-file', () => {
+    let hashSpy;
+
+    beforeEach(() => {
+      hashSpy = jest.spyOn(commonUtils, 'handleLocationHash');
+    });
+
     it('renders a single diff', async () => {
       createComponent(
         undefined,
@@ -681,6 +689,22 @@ describe('diffs/components/app', () => {
       expect(wrapper.findAllComponents(DiffFile).length).toBe(1);
     });
 
+    it('re-checks the URL hash for items to scroll to', () => {
+      createComponent(undefined, () => {}, undefined, { viewDiffsFileByFile: true });
+
+      eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+
+      expect(hashSpy).toHaveBeenCalledTimes(1);
+    });
+
+    it('does not re-check the URL hash to scroll when not in single-file mode', () => {
+      createComponent();
+
+      eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+
+      expect(hashSpy).not.toHaveBeenCalled();
+    });
+
     describe('pagination', () => {
       const fileByFileNav = () => wrapper.find('[data-testid="file-by-file-navigation"]');
       const paginator = () => fileByFileNav().findComponent(GlPagination);
-- 
GitLab


From 39798ff07e70d82942ce2d3c5c4c71616482b775 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Mon, 23 Oct 2023 11:56:18 -0600
Subject: [PATCH 3/4] Wait until the file has finished displaying before trying
 to scroll

---
 app/assets/javascripts/diffs/components/app.vue | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue
index 1c3f56fe7edc7c..7c3d6dc8c42dd0 100644
--- a/app/assets/javascripts/diffs/components/app.vue
+++ b/app/assets/javascripts/diffs/components/app.vue
@@ -139,6 +139,7 @@ export default {
       diffFilesLength: 0,
       virtualScrollCurrentIndex: -1,
       subscribedToVirtualScrollingEvents: false,
+      autoScrolled: false,
     };
   },
   apollo: {
@@ -361,7 +362,10 @@ export default {
     this.adjustView();
     this.subscribeToEvents();
 
-    this.slowHashHandler = debounce(handleLocationHash, DEFAULT_DEBOUNCE_AND_THROTTLE_MS);
+    this.slowHashHandler = debounce(() => {
+      handleLocationHash();
+      this.autoScrolled = true;
+    }, DEFAULT_DEBOUNCE_AND_THROTTLE_MS);
     this.unwatchDiscussions = this.$watch(
       () => `${this.flatBlobsList.length}:${this.$store.state.notes.discussions.length}`,
       () => {
@@ -456,8 +460,12 @@ export default {
       }
     },
     handleHash() {
-      if (this.viewDiffsFileByFile) {
-        this.slowHashHandler();
+      if (this.viewDiffsFileByFile && !this.autoScrolled) {
+        const file = this.diffs[0];
+
+        if (file && !file.isLoadingFullFile) {
+          requestIdleCallback(() => this.slowHashHandler());
+        }
       }
     },
     navigateToDiffFileNumber(number) {
-- 
GitLab


From 4d53f6be8a39fcb5832461ce512633ff9ca7e509 Mon Sep 17 00:00:00 2001
From: Thomas Randolph <trandolph@gitlab.com>
Date: Mon, 23 Oct 2023 21:33:52 -0600
Subject: [PATCH 4/4] Test the wait before scrolling

---
 spec/frontend/diffs/components/app_spec.js | 45 +++++++++++++++++-----
 1 file changed, 36 insertions(+), 9 deletions(-)

diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js
index e12b08d1106617..5299361a4930a8 100644
--- a/spec/frontend/diffs/components/app_spec.js
+++ b/spec/frontend/diffs/components/app_spec.js
@@ -29,6 +29,7 @@ import { HTTP_STATUS_OK } from '~/lib/utils/http_status';
 import { Mousetrap } from '~/lib/mousetrap';
 import * as urlUtils from '~/lib/utils/url_utility';
 import * as commonUtils from '~/lib/utils/common_utils';
+import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants';
 import { stubPerformanceWebAPI } from 'helpers/performance';
 import createDiffsStore from '../create_diffs_store';
 import diffsMockData from '../mock_data/merge_request_diffs';
@@ -689,20 +690,46 @@ describe('diffs/components/app', () => {
       expect(wrapper.findAllComponents(DiffFile).length).toBe(1);
     });
 
-    it('re-checks the URL hash for items to scroll to', () => {
-      createComponent(undefined, () => {}, undefined, { viewDiffsFileByFile: true });
+    describe('rechecking the url hash for scrolling', () => {
+      const advanceAndCheckCalls = (count = 0) => {
+        jest.advanceTimersByTime(DEFAULT_DEBOUNCE_AND_THROTTLE_MS);
+        expect(hashSpy).toHaveBeenCalledTimes(count);
+      };
 
-      eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+      it('re-checks one time after the file finishes loading', () => {
+        createComponent(
+          undefined,
+          ({ state }) => {
+            state.diffs.diffFiles = [{ isLoadingFullFile: true }];
+          },
+          undefined,
+          { viewDiffsFileByFile: true },
+        );
 
-      expect(hashSpy).toHaveBeenCalledTimes(1);
-    });
+        // The hash check is not called if the file is still marked as loading
+        expect(hashSpy).toHaveBeenCalledTimes(0);
+        eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+        advanceAndCheckCalls();
+        eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+        advanceAndCheckCalls();
+        // Once the file has finished loading, it calls through to check the hash
+        store.state.diffs.diffFiles[0].isLoadingFullFile = false;
+        eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+        advanceAndCheckCalls(1);
+        // No further scrolls happen after one hash check / scroll
+        eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+        advanceAndCheckCalls(1);
+        eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+        advanceAndCheckCalls(1);
+      });
 
-    it('does not re-check the URL hash to scroll when not in single-file mode', () => {
-      createComponent();
+      it('does not re-check when not in single-file mode', () => {
+        createComponent();
 
-      eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
+        eventHub.$emit(EVT_DISCUSSIONS_ASSIGNED);
 
-      expect(hashSpy).not.toHaveBeenCalled();
+        expect(hashSpy).not.toHaveBeenCalled();
+      });
     });
 
     describe('pagination', () => {
-- 
GitLab