From 52dfaf1b628b59fa054da33323b9f9c0908a75ca Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 8 Mar 2023 01:23:47 -0700 Subject: [PATCH 1/2] Add an action to only scroll after loading the right file --- app/assets/javascripts/diffs/store/actions.js | 27 ++++++++ locale/gitlab.pot | 3 + spec/frontend/diffs/store/actions_spec.js | 65 +++++++++++++++++++ 3 files changed, 95 insertions(+) diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 29b77431f7db68..55e850957ce03f 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -590,6 +590,33 @@ export const setCurrentFileHash = ({ commit }, hash) => { commit(types.SET_CURRENT_DIFF_FILE, hash); }; +export const goToFile = ({ state, commit, dispatch, getters }, { path }) => { + if (!state.viewDiffsFileByFile) { + dispatch('scrollToFile', { path }); + } else { + if (!state.treeEntries[path]) return; + + const { fileHash } = state.treeEntries[path]; + + commit(types.SET_CURRENT_DIFF_FILE, fileHash); + + if (!getters.isTreePathLoaded(path)) { + document.location.hash = fileHash; + dispatch('fetchFileByFile') + .then(() => { + dispatch('scrollToFile', { path }); + }) + .catch(() => { + createAlert({ + message: s__( + 'MergeRequest|Encountered an issue while trying to fetch the single file diff.', + ), + }); + }); + } + } +}; + export const scrollToFile = ({ state, commit, getters }, { path }) => { if (!state.treeEntries[path]) return; diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 75ce826e1be683..f86c294a03ab93 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -27265,6 +27265,9 @@ msgstr "" msgid "MergeRequest|Compare %{target} and %{source}" msgstr "" +msgid "MergeRequest|Encountered an issue while trying to fetch the single file diff." +msgstr "" + msgid "MergeRequest|Error dismissing suggestion popover. Please try again." msgstr "" diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 8ef41cc83f2e2c..420418707bbc51 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -994,6 +994,71 @@ describe('DiffsStoreActions', () => { }); }); + describe('goToFile', () => { + const getters = {}; + let state; + let dispatch; + let commit; + + beforeEach(() => { + getters.isTreePathLoaded = () => false; + state = { + viewDiffsFileByFile: true, + treeEntries: { + path: { + fileHash: 'test', + }, + }, + }; + commit = jest.fn(); + dispatch = jest.fn().mockResolvedValue(); + }); + + it('immediately defers to scrollToFile if the app is not in file-by-file mode', () => { + state.viewDiffsFileByFile = false; + + diffActions.goToFile({ state, dispatch }, { path: 'path' }); + + expect(dispatch).toHaveBeenCalledWith('scrollToFile', { path: 'path' }); + }); + + describe('when the app is in fileByFile mode', () => { + it('commits SET_CURRENT_DIFF_FILE', () => { + diffActions.goToFile({ state, commit, dispatch, getters }, { path: 'path' }); + + expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, 'test'); + }); + + it('does nothing more if the path has already been loaded', () => { + getters.isTreePathLoaded = () => true; + + diffActions.goToFile({ state, dispatch, getters, commit }, { path: 'path' }); + + expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, 'test'); + expect(dispatch).toHaveBeenCalledTimes(0); + }); + + describe('when the tree entry has not been loaded', () => { + it('updates location hash', () => { + diffActions.goToFile({ state, commit, getters, dispatch }, { path: 'path' }); + + expect(document.location.hash).toBe('#test'); + }); + + it('loads the file and then scrolls to it', async () => { + diffActions.goToFile({ state, commit, getters, dispatch }, { path: 'path' }); + + // Wait for the fetchFileByFile dispatch to return, to trigger scrollToFile + await waitForPromises(); + + expect(dispatch).toHaveBeenCalledWith('fetchFileByFile'); + expect(dispatch).toHaveBeenCalledWith('scrollToFile', { path: 'path' }); + expect(dispatch).toHaveBeenCalledTimes(2); + }); + }); + }); + }); + describe('scrollToFile', () => { let commit; const getters = { isVirtualScrollingEnabled: false }; -- GitLab From ee7d86b6cdbaa276c24f5b0d24f5b39e7744be8e Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Wed, 29 Mar 2023 11:21:03 -0600 Subject: [PATCH 2/2] Respond to review comments: - "I'd probably use variables here for the sake of not repeating strings multiple times." - "How about a test checking if the alert is displayed upon errors?" --- app/assets/javascripts/diffs/i18n.js | 3 ++ app/assets/javascripts/diffs/store/actions.js | 5 +-- spec/frontend/diffs/store/actions_spec.js | 37 ++++++++++++++----- 3 files changed, 32 insertions(+), 13 deletions(-) diff --git a/app/assets/javascripts/diffs/i18n.js b/app/assets/javascripts/diffs/i18n.js index 0f44eb06cb3d7b..d8812de12d48a3 100644 --- a/app/assets/javascripts/diffs/i18n.js +++ b/app/assets/javascripts/diffs/i18n.js @@ -1,6 +1,9 @@ import { __, s__ } from '~/locale'; export const GENERIC_ERROR = __('Something went wrong on our end. Please try again!'); +export const LOAD_SINGLE_DIFF_FAILED = s__( + 'MergeRequest|Encountered an issue while trying to fetch the single file diff.', +); export const DIFF_FILE_HEADER = { optionsDropdownTitle: __('Options'), diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 55e850957ce03f..6c461021f86587 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -50,6 +50,7 @@ import { TRACKING_SINGLE_FILE_MODE, TRACKING_MULTIPLE_FILES_MODE, } from '../constants'; +import { LOAD_SINGLE_DIFF_FAILED } from '../i18n'; import eventHub from '../event_hub'; import { isCollapsed } from '../utils/diff_file'; import { markFileReview, setReviewsForMergeRequest } from '../utils/file_reviews'; @@ -608,9 +609,7 @@ export const goToFile = ({ state, commit, dispatch, getters }, { path }) => { }) .catch(() => { createAlert({ - message: s__( - 'MergeRequest|Encountered an issue while trying to fetch the single file diff.', - ), + message: LOAD_SINGLE_DIFF_FAILED, }); }); } diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 420418707bbc51..351e62fb6e009a 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -10,6 +10,7 @@ import { INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE, } from '~/diffs/constants'; +import { LOAD_SINGLE_DIFF_FAILED } from '~/diffs/i18n'; import * as diffActions from '~/diffs/store/actions'; import * as types from '~/diffs/store/mutation_types'; import * as utils from '~/diffs/store/utils'; @@ -996,6 +997,8 @@ describe('DiffsStoreActions', () => { describe('goToFile', () => { const getters = {}; + const file = { path: 'path' }; + const fileHash = 'test'; let state; let dispatch; let commit; @@ -1006,7 +1009,7 @@ describe('DiffsStoreActions', () => { viewDiffsFileByFile: true, treeEntries: { path: { - fileHash: 'test', + fileHash, }, }, }; @@ -1017,44 +1020,58 @@ describe('DiffsStoreActions', () => { it('immediately defers to scrollToFile if the app is not in file-by-file mode', () => { state.viewDiffsFileByFile = false; - diffActions.goToFile({ state, dispatch }, { path: 'path' }); + diffActions.goToFile({ state, dispatch }, file); - expect(dispatch).toHaveBeenCalledWith('scrollToFile', { path: 'path' }); + expect(dispatch).toHaveBeenCalledWith('scrollToFile', file); }); describe('when the app is in fileByFile mode', () => { it('commits SET_CURRENT_DIFF_FILE', () => { - diffActions.goToFile({ state, commit, dispatch, getters }, { path: 'path' }); + diffActions.goToFile({ state, commit, dispatch, getters }, file); - expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, 'test'); + expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, fileHash); }); it('does nothing more if the path has already been loaded', () => { getters.isTreePathLoaded = () => true; - diffActions.goToFile({ state, dispatch, getters, commit }, { path: 'path' }); + diffActions.goToFile({ state, dispatch, getters, commit }, file); - expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, 'test'); + expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, fileHash); expect(dispatch).toHaveBeenCalledTimes(0); }); describe('when the tree entry has not been loaded', () => { it('updates location hash', () => { - diffActions.goToFile({ state, commit, getters, dispatch }, { path: 'path' }); + diffActions.goToFile({ state, commit, getters, dispatch }, file); expect(document.location.hash).toBe('#test'); }); it('loads the file and then scrolls to it', async () => { - diffActions.goToFile({ state, commit, getters, dispatch }, { path: 'path' }); + diffActions.goToFile({ state, commit, getters, dispatch }, file); // Wait for the fetchFileByFile dispatch to return, to trigger scrollToFile await waitForPromises(); expect(dispatch).toHaveBeenCalledWith('fetchFileByFile'); - expect(dispatch).toHaveBeenCalledWith('scrollToFile', { path: 'path' }); + expect(dispatch).toHaveBeenCalledWith('scrollToFile', file); expect(dispatch).toHaveBeenCalledTimes(2); }); + + it('shows an alert when there was an error fetching the file', async () => { + dispatch = jest.fn().mockRejectedValue(); + + diffActions.goToFile({ state, commit, getters, dispatch }, file); + + // Wait for the fetchFileByFile dispatch to return, to trigger the catch + await waitForPromises(); + + expect(createAlert).toHaveBeenCalledTimes(1); + expect(createAlert).toHaveBeenCalledWith({ + message: expect.stringMatching(LOAD_SINGLE_DIFF_FAILED), + }); + }); }); }); }); -- GitLab