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

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

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