Merge branch '50347-fix-scrolling-to-diff-note-after-incremental-rendering' into 'master'

Resolve "Link to file in Changes tab of MR no longer works for all files after incremental rendering improvement"

Closes #50347

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

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

 <script>
-import { mapGetters } from 'vuex';
+import { mapGetters, mapActions } from 'vuex';
 import DiffTableCell from './diff_table_cell.vue';
 import {
   NEW_LINE_TYPE,
@@ -63,7 +63,11 @@ export default {
     this.linePositionLeft = LINE_POSITION_LEFT;
     this.linePositionRight = LINE_POSITION_RIGHT;
   },
+  mounted() {
+    this.scrollToLineIfNeededInline(this.line);
+  },
   methods: {
+    ...mapActions('diffs', ['scrollToLineIfNeededInline']),
     handleMouseMove(e) {
       // To show the comment icon on the gutter we need to know if we hover the line.
       // Current table structure doesn't allow us to do this with CSS in both of the diff view types

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

 <script>
+import { mapActions } from 'vuex';
 import $ from 'jquery';
 import DiffTableCell from './diff_table_cell.vue';
 import {
@@ -64,7 +65,11 @@ export default {
     this.oldLineType = OLD_LINE_TYPE;
     this.parallelDiffViewType = PARALLEL_DIFF_VIEW_TYPE;
   },
+  mounted() {
+    this.scrollToLineIfNeededParallel(this.line);
+  },
   methods: {
+    ...mapActions('diffs', ['scrollToLineIfNeededParallel']),
     handleMouseMove(e) {
       const isHover = e.type === 'mouseover';
       const hoveringCell = e.target.closest('td');

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

@@ -2,7 +2,7 @@ import Vue from 'vue';
 import axios from '~/lib/utils/axios_utils';
 import Cookies from 'js-cookie';
 import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils';
-import { mergeUrlParams } from '~/lib/utils/url_utility';
+import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility';
 import { getDiffPositionByLineCode } from './utils';
 import * as types from './mutation_types';
 import {
@@ -120,6 +120,25 @@ export const loadMoreLines = ({ commit }, options) => {
   });
 };
 
+export const scrollToLineIfNeededInline = (_, line) => {
+  const hash = getLocationHash();
+
+  if (hash && line.lineCode === hash) {
+    handleLocationHash();
+  }
+};
+
+export const scrollToLineIfNeededParallel = (_, line) => {
+  const hash = getLocationHash();
+
+  if (
+    hash &&
+    ((line.left && line.left.lineCode === hash) || (line.right && line.right.lineCode === hash))
+  ) {
+    handleLocationHash();
+  }
+};
+
 export const loadCollapsedDiff = ({ commit }, file) =>
   axios.get(file.loadCollapsedDiffUrl).then(res => {
     commit(types.ADD_COLLAPSED_DIFFS, {

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

@@ -56,7 +56,8 @@ export const rstrip = val => {
   return val;
 };
 
-export const updateTooltipTitle = ($tooltipEl, newTitle) => $tooltipEl.attr('title', newTitle).tooltip('_fixTitle');
+export const updateTooltipTitle = ($tooltipEl, newTitle) =>
+  $tooltipEl.attr('title', newTitle).tooltip('_fixTitle');
 
 export const disableButtonIfEmptyField = (fieldSelector, buttonSelector, eventName = 'input') => {
   const field = $(fieldSelector);
@@ -86,6 +87,7 @@ export const handleLocationHash = () => {
   const fixedTabs = document.querySelector('.js-tabs-affix');
   const fixedDiffStats = document.querySelector('.js-diff-files-changed');
   const fixedNav = document.querySelector('.navbar-gitlab');
+  const performanceBar = document.querySelector('#js-peek');
 
   let adjustment = 0;
   if (fixedNav) adjustment -= fixedNav.offsetHeight;
@@ -102,6 +104,10 @@ export const handleLocationHash = () => {
     adjustment -= fixedDiffStats.offsetHeight;
   }
 
+  if (performanceBar) {
+    adjustment -= performanceBar.offsetHeight;
+  }
+
   window.scrollBy(0, adjustment);
 };
 
@@ -131,21 +137,20 @@ export const parseUrlPathname = url => {
   return parsedUrl.pathname.charAt(0) === '/' ? parsedUrl.pathname : `/${parsedUrl.pathname}`;
 };
 
-const splitPath = (path = '') => path
-  .replace(/^\?/, '')
-  .split('&');
+const splitPath = (path = '') => path.replace(/^\?/, '').split('&');
 
-export const urlParamsToArray = (path = '') => splitPath(path)
-  .filter(param => param.length > 0)
-  .map(param => {
-    const split = param.split('=');
-    return [decodeURI(split[0]), split[1]].join('=');
-  });
+export const urlParamsToArray = (path = '') =>
+  splitPath(path)
+    .filter(param => param.length > 0)
+    .map(param => {
+      const split = param.split('=');
+      return [decodeURI(split[0]), split[1]].join('=');
+    });
 
 export const getUrlParamsArray = () => urlParamsToArray(window.location.search);
 
-export const urlParamsToObject = (path = '') => splitPath(path)
-  .reduce((dataParam, filterParam) => {
+export const urlParamsToObject = (path = '') =>
+  splitPath(path).reduce((dataParam, filterParam) => {
     if (filterParam === '') {
       return dataParam;
     }
@@ -216,7 +221,7 @@ export const getParameterByName = (name, urlToParse) => {
   return decodeURIComponent(results[2].replace(/\+/g, ' '));
 };
 
-const handleSelectedRange = (range) => {
+const handleSelectedRange = range => {
   const container = range.commonAncestorContainer;
   // add context to fragment if needed
   if (container.tagName === 'OL') {
@@ -453,7 +458,7 @@ export const backOff = (fn, timeout = 60000) => {
 export const createOverlayIcon = (iconPath, overlayPath) => {
   const faviconImage = document.createElement('img');
 
-  return new Promise((resolve) => {
+  return new Promise(resolve => {
     faviconImage.onload = () => {
       const size = 32;
 
@@ -464,13 +469,29 @@ export const createOverlayIcon = (iconPath, overlayPath) => {
       const context = canvas.getContext('2d');
       context.clearRect(0, 0, size, size);
       context.drawImage(
-        faviconImage, 0, 0, faviconImage.width, faviconImage.height, 0, 0, size, size,
+        faviconImage,
+        0,
+        0,
+        faviconImage.width,
+        faviconImage.height,
+        0,
+        0,
+        size,
+        size,
       );
 
       const overlayImage = document.createElement('img');
       overlayImage.onload = () => {
         context.drawImage(
-          overlayImage, 0, 0, overlayImage.width, overlayImage.height, 0, 0, size, size,
+          overlayImage,
+          0,
+          0,
+          overlayImage.width,
+          overlayImage.height,
+          0,
+          0,
+          size,
+          size,
         );
 
         const faviconWithOverlayUrl = canvas.toDataURL();
@@ -483,17 +504,21 @@ export const createOverlayIcon = (iconPath, overlayPath) => {
   });
 };
 
-export const setFaviconOverlay = (overlayPath) => {
+export const setFaviconOverlay = overlayPath => {
   const faviconEl = document.getElementById('favicon');
 
-  if (!faviconEl) { return null; }
+  if (!faviconEl) {
+    return null;
+  }
 
   const iconPath = faviconEl.getAttribute('data-original-href');
 
-  return createOverlayIcon(iconPath, overlayPath).then(faviconWithOverlayUrl => faviconEl.setAttribute('href', faviconWithOverlayUrl));
+  return createOverlayIcon(iconPath, overlayPath).then(faviconWithOverlayUrl =>
+    faviconEl.setAttribute('href', faviconWithOverlayUrl),
+  );
 };
 
-export const setFavicon = (faviconPath) => {
+export const setFavicon = faviconPath => {
   const faviconEl = document.getElementById('favicon');
   if (faviconEl && faviconPath) {
     faviconEl.setAttribute('href', faviconPath);
@@ -518,7 +543,7 @@ export const setCiStatusFavicon = pageUrl =>
       }
       return resetFavicon();
     })
-    .catch((error) => {
+    .catch(error => {
       resetFavicon();
       throw error;
     });

spec/javascripts/diffs/store/actions_spec.js

@@ -5,7 +5,23 @@ import {
   INLINE_DIFF_VIEW_TYPE,
   PARALLEL_DIFF_VIEW_TYPE,
 } from '~/diffs/constants';
-import * as actions from '~/diffs/store/actions';
+import actions, {
+  setBaseConfig,
+  fetchDiffFiles,
+  assignDiscussionsToDiff,
+  removeDiscussionsFromDiff,
+  startRenderDiffsQueue,
+  setInlineDiffViewType,
+  setParallelDiffViewType,
+  showCommentForm,
+  cancelCommentForm,
+  loadMoreLines,
+  scrollToLineIfNeededInline,
+  scrollToLineIfNeededParallel,
+  loadCollapsedDiff,
+  expandAllFiles,
+  toggleFileDiscussions,
+} from '~/diffs/store/actions';
 import * as types from '~/diffs/store/mutation_types';
 import { reduceDiscussionsToLineCodes } from '~/notes/stores/utils';
 import axios from '~/lib/utils/axios_utils';
@@ -37,7 +53,7 @@ describe('DiffsStoreActions', () => {
       const projectPath = '/root/project';
 
       testAction(
-        actions.setBaseConfig,
+        setBaseConfig,
         { endpoint, projectPath },
         { endpoint: '', projectPath: '' },
         [{ type: types.SET_BASE_CONFIG, payload: { endpoint, projectPath } }],
@@ -55,7 +71,7 @@ describe('DiffsStoreActions', () => {
       mock.onGet(endpoint).reply(200, res);
 
       testAction(
-        actions.fetchDiffFiles,
+        fetchDiffFiles,
         {},
         { endpoint },
         [
@@ -139,7 +155,7 @@ describe('DiffsStoreActions', () => {
       const discussions = reduceDiscussionsToLineCodes([singleDiscussion]);
 
       testAction(
-        actions.assignDiscussionsToDiff,
+        assignDiscussionsToDiff,
         discussions,
         state,
         [
@@ -208,7 +224,7 @@ describe('DiffsStoreActions', () => {
       };
 
       testAction(
-        actions.removeDiscussionsFromDiff,
+        removeDiscussionsFromDiff,
         singleDiscussion,
         state,
         [
@@ -252,8 +268,7 @@ describe('DiffsStoreActions', () => {
         });
       };
 
-      actions
-        .startRenderDiffsQueue({ state, commit: pseudoCommit })
+      startRenderDiffsQueue({ state, commit: pseudoCommit })
         .then(() => {
           expect(state.diffFiles[0].renderIt).toBeTruthy();
           expect(state.diffFiles[1].renderIt).toBeTruthy();
@@ -269,7 +284,7 @@ describe('DiffsStoreActions', () => {
   describe('setInlineDiffViewType', () => {
     it('should set diff view type to inline and also set the cookie properly', done => {
       testAction(
-        actions.setInlineDiffViewType,
+        setInlineDiffViewType,
         null,
         {},
         [{ type: types.SET_DIFF_VIEW_TYPE, payload: INLINE_DIFF_VIEW_TYPE }],
@@ -287,7 +302,7 @@ describe('DiffsStoreActions', () => {
   describe('setParallelDiffViewType', () => {
     it('should set diff view type to parallel and also set the cookie properly', done => {
       testAction(
-        actions.setParallelDiffViewType,
+        setParallelDiffViewType,
         null,
         {},
         [{ type: types.SET_DIFF_VIEW_TYPE, payload: PARALLEL_DIFF_VIEW_TYPE }],
@@ -307,7 +322,7 @@ describe('DiffsStoreActions', () => {
       const payload = { lineCode: 'lineCode' };
 
       testAction(
-        actions.showCommentForm,
+        showCommentForm,
         payload,
         {},
         [{ type: types.ADD_COMMENT_FORM_LINE, payload }],
@@ -322,7 +337,7 @@ describe('DiffsStoreActions', () => {
       const payload = { lineCode: 'lineCode' };
 
       testAction(
-        actions.cancelCommentForm,
+        cancelCommentForm,
         payload,
         {},
         [{ type: types.REMOVE_COMMENT_FORM_LINE, payload }],
@@ -344,7 +359,7 @@ describe('DiffsStoreActions', () => {
       mock.onGet(endpoint).reply(200, contextLines);
 
       testAction(
-        actions.loadMoreLines,
+        loadMoreLines,
         options,
         {},
         [
@@ -370,7 +385,7 @@ describe('DiffsStoreActions', () => {
       mock.onGet(file.loadCollapsedDiffUrl).reply(200, data);
 
       testAction(
-        actions.loadCollapsedDiff,
+        loadCollapsedDiff,
         file,
         {},
         [
@@ -391,7 +406,7 @@ describe('DiffsStoreActions', () => {
   describe('expandAllFiles', () => {
     it('should change the collapsed prop from the diffFiles', done => {
       testAction(
-        actions.expandAllFiles,
+        expandAllFiles,
         null,
         {},
         [
@@ -415,7 +430,7 @@ describe('DiffsStoreActions', () => {
 
       const dispatch = jasmine.createSpy('dispatch');
 
-      actions.toggleFileDiscussions({ getters, dispatch });
+      toggleFileDiscussions({ getters, dispatch });
 
       expect(dispatch).toHaveBeenCalledWith(
         'collapseDiscussion',
@@ -433,7 +448,7 @@ describe('DiffsStoreActions', () => {
 
       const dispatch = jasmine.createSpy();
 
-      actions.toggleFileDiscussions({ getters, dispatch });
+      toggleFileDiscussions({ getters, dispatch });
 
       expect(dispatch).toHaveBeenCalledWith(
         'expandDiscussion',
@@ -451,7 +466,7 @@ describe('DiffsStoreActions', () => {
 
       const dispatch = jasmine.createSpy();
 
-      actions.toggleFileDiscussions({ getters, dispatch });
+      toggleFileDiscussions({ getters, dispatch });
 
       expect(dispatch).toHaveBeenCalledWith(
         'expandDiscussion',
@@ -460,4 +475,111 @@ describe('DiffsStoreActions', () => {
       );
     });
   });
+
+  describe('scrollToLineIfNeededInline', () => {
+    const lineMock = {
+      lineCode: 'ABC_123',
+    };
+
+    it('should not call handleLocationHash when there is not hash', () => {
+      window.location.hash = '';
+
+      const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub();
+
+      scrollToLineIfNeededInline({}, lineMock);
+
+      expect(handleLocationHashSpy).not.toHaveBeenCalled();
+    });
+
+    it('should not call handleLocationHash when the hash does not match any line', () => {
+      window.location.hash = 'XYZ_456';
+
+      const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub();
+
+      scrollToLineIfNeededInline({}, lineMock);
+
+      expect(handleLocationHashSpy).not.toHaveBeenCalled();
+    });
+
+    it('should call handleLocationHash only when the hash matches a line', () => {
+      window.location.hash = 'ABC_123';
+
+      const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub();
+
+      scrollToLineIfNeededInline(
+        {},
+        {
+          lineCode: 'ABC_456',
+        },
+      );
+      scrollToLineIfNeededInline({}, lineMock);
+      scrollToLineIfNeededInline(
+        {},
+        {
+          lineCode: 'XYZ_456',
+        },
+      );
+
+      expect(handleLocationHashSpy).toHaveBeenCalled();
+      expect(handleLocationHashSpy).toHaveBeenCalledTimes(1);
+    });
+  });
+
+  describe('scrollToLineIfNeededParallel', () => {
+    const lineMock = {
+      left: null,
+      right: {
+        lineCode: 'ABC_123',
+      },
+    };
+
+    it('should not call handleLocationHash when there is not hash', () => {
+      window.location.hash = '';
+
+      const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub();
+
+      scrollToLineIfNeededParallel({}, lineMock);
+
+      expect(handleLocationHashSpy).not.toHaveBeenCalled();
+    });
+
+    it('should not call handleLocationHash when the hash does not match any line', () => {
+      window.location.hash = 'XYZ_456';
+
+      const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub();
+
+      scrollToLineIfNeededParallel({}, lineMock);
+
+      expect(handleLocationHashSpy).not.toHaveBeenCalled();
+    });
+
+    it('should call handleLocationHash only when the hash matches a line', () => {
+      window.location.hash = 'ABC_123';
+
+      const handleLocationHashSpy = spyOnDependency(actions, 'handleLocationHash').and.stub();
+
+      scrollToLineIfNeededParallel(
+        {},
+        {
+          left: null,
+          right: {
+            lineCode: 'ABC_456',
+          },
+        },
+      );
+      scrollToLineIfNeededParallel({}, lineMock);
+      scrollToLineIfNeededParallel(
+        {},
+        {
+          left: null,
+          right: {
+            lineCode: 'XYZ_456',
+          },
+        },
+      );
+
+      expect(handleLocationHashSpy).toHaveBeenCalled();
+      expect(handleLocationHashSpy).toHaveBeenCalledTimes(1);
+    });
+  });
 });