diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index 4e1ccfc530e95d652f526fef51961f1cbd7634e0..d044a878b140336359c6e76d4eb303ef1334cfa9 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -226,6 +226,12 @@ export default { } this.manageViewedEffects(); + + if (this.viewDiffsFileByFile) { + requestIdleCallback(() => { + this.prefetchFileNeighbors(); + }); + } }, beforeDestroy() { eventHub.$off(EVT_EXPAND_ALL_FILES, this.expandAllListener); @@ -234,6 +240,7 @@ export default { ...mapActions('diffs', [ 'loadCollapsedDiff', 'assignDiscussionsToDiff', + 'prefetchFileNeighbors', 'setFileCollapsedByUser', 'saveDiffDiscussion', 'toggleFileCommentForm', diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 2a5570179539903861a39ec8b3320e6c89133301..c3dc4a198e79313b656583719533c90d52a2bb39 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -113,6 +113,46 @@ export const setBaseConfig = ({ commit }, options) => { }); }; +export const prefetchSingleFile = async ({ state, getters, commit }, treeEntry) => { + const versionPath = state.mergeRequestDiff?.version_path; + + if ( + treeEntry && + !treeEntry.diffLoaded && + !treeEntry.diffLoading && + !getters.getDiffFileByHash(treeEntry.fileHash) + ) { + const urlParams = { + old_path: treeEntry.filePaths.old, + new_path: treeEntry.filePaths.new, + w: state.showWhitespace ? '0' : '1', + view: 'inline', + commit_id: getters.commitId, + }; + + if (versionPath) { + const { diffId, startSha } = getDerivedMergeRequestInformation({ endpoint: versionPath }); + + urlParams.diff_id = diffId; + urlParams.start_sha = startSha; + } + + commit(types.TREE_ENTRY_DIFF_LOADING, { path: treeEntry.filePaths.new }); + + try { + const { data: diffData } = await axios.get( + mergeUrlParams({ ...urlParams }, state.endpointDiffForPath), + ); + + commit(types.SET_DIFF_DATA_BATCH, { diff_files: diffData.diff_files }); + + eventHub.$emit('diffFilesModified'); + } catch (e) { + commit(types.TREE_ENTRY_DIFF_LOADING, { path: treeEntry.filePaths.new, loading: false }); + } + } +}; + export const fetchFileByFile = async ({ state, getters, commit }) => { const isNoteLink = isUrlHashNoteLink(window?.location?.hash); const id = parseUrlHashAsFileHash(window?.location?.hash, state.currentDiffFileId); @@ -304,6 +344,17 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { } }); }; + +export function prefetchFileNeighbors({ getters, dispatch }) { + const { flatBlobsList: allBlobs, currentDiffIndex: currentIndex } = getters; + + const previous = Math.max(currentIndex - 1, 0); + const next = Math.min(allBlobs.length - 1, currentIndex + 1); + + dispatch('prefetchSingleFile', allBlobs[next]); + dispatch('prefetchSingleFile', allBlobs[previous]); +} + export const fetchCoverageFiles = ({ commit, state }) => { const coveragePoll = new Poll({ resource: { diff --git a/app/assets/javascripts/diffs/store/mutation_types.js b/app/assets/javascripts/diffs/store/mutation_types.js index c32d82faad0dab4d079348ae19c197e21a9eee7f..3df491503a43525a25fec7f362f82da633fcd7f7 100644 --- a/app/assets/javascripts/diffs/store/mutation_types.js +++ b/app/assets/javascripts/diffs/store/mutation_types.js @@ -19,6 +19,7 @@ export const RENDER_FILE = 'RENDER_FILE'; export const SET_LINE_DISCUSSIONS_FOR_FILE = 'SET_LINE_DISCUSSIONS_FOR_FILE'; export const REMOVE_LINE_DISCUSSIONS_FOR_FILE = 'REMOVE_LINE_DISCUSSIONS_FOR_FILE'; export const TOGGLE_FOLDER_OPEN = 'TOGGLE_FOLDER_OPEN'; +export const TREE_ENTRY_DIFF_LOADING = 'TREE_ENTRY_DIFF_LOADING'; export const SET_SHOW_TREE_LIST = 'SET_SHOW_TREE_LIST'; export const SET_CURRENT_DIFF_FILE = 'SET_CURRENT_DIFF_FILE'; export const SET_DIFF_FILE_VIEWED = 'SET_DIFF_FILE_VIEWED'; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 4855ca87e9117305bc3249f2b987ab1b6e5b5166..402b1a82ca40ffa076322c0e2a3a12f676465b6a 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -266,6 +266,9 @@ export default { [types.TOGGLE_FOLDER_OPEN](state, path) { state.treeEntries[path].opened = !state.treeEntries[path].opened; }, + [types.TREE_ENTRY_DIFF_LOADING](state, { path, loading = true }) { + state.treeEntries[path].diffLoading = loading; + }, [types.SET_SHOW_TREE_LIST](state, showTreeList) { state.showTreeList = showTreeList; }, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 97dfd351e67a93fae64d2b5a7cb95bb8d260538c..307c41a98f83a62910e6137cbff438ba3c9e0450 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -593,6 +593,7 @@ export function markTreeEntriesLoaded({ priorEntries, loadedFiles }) { if (entry) { entry.diffLoaded = true; + entry.diffLoading = false; } }); diff --git a/app/assets/javascripts/diffs/utils/tree_worker_utils.js b/app/assets/javascripts/diffs/utils/tree_worker_utils.js index 8689809cfa9d75f284acdab15c32c009c3794fd6..01a475f48d16d7685ba0b0f1d392274527b84734 100644 --- a/app/assets/javascripts/diffs/utils/tree_worker_utils.js +++ b/app/assets/javascripts/diffs/utils/tree_worker_utils.js @@ -86,6 +86,7 @@ export const generateTreeList = (files) => { Object.assign(entry, { changed: true, diffLoaded: false, + diffLoading: false, filePaths: { old: file.old_path, new: file.new_path, diff --git a/spec/frontend/diffs/components/diff_file_spec.js b/spec/frontend/diffs/components/diff_file_spec.js index db6cde883f30bc21d01c35fb272f658e5ae02d02..d0b490198bc85cb452bb5ecd3cd351887363e4c0 100644 --- a/spec/frontend/diffs/components/diff_file_spec.js +++ b/spec/frontend/diffs/components/diff_file_spec.js @@ -42,6 +42,7 @@ jest.mock('~/notes/mixins/diff_line_note_form', () => ({ Vue.use(Vuex); const saveDiffDiscussionMock = jest.fn(); +const prefetchFileNeighborsMock = jest.fn(); function changeViewer(store, index, { automaticallyCollapsed, manuallyCollapsed, name }) { const file = store.state.diffs.diffFiles[index]; @@ -91,6 +92,7 @@ function createComponent({ file, first = false, last = false, options = {}, prop const diffs = diffsModule(); diffs.actions = { ...diffs.actions, + prefetchFileNeighbors: prefetchFileNeighborsMock, saveDiffDiscussion: saveDiffDiscussionMock, }; @@ -155,19 +157,44 @@ const triggerSaveDraftNote = (wrapper, note, parent, error) => findNoteForm(wrapper).vm.$emit('handleFormUpdateAddToReview', note, false, parent, error); describe('DiffFile', () => { + let readableFile; let wrapper; let store; let axiosMock; beforeEach(() => { + readableFile = getReadableFile(); axiosMock = new MockAdapter(axios); - ({ wrapper, store } = createComponent({ file: getReadableFile() })); + ({ wrapper, store } = createComponent({ file: readableFile })); }); afterEach(() => { axiosMock.restore(); }); + describe('mounted', () => { + beforeEach(() => { + jest.spyOn(window, 'requestIdleCallback').mockImplementation((fn) => fn()); + }); + + it.each` + description | fileByFile + ${'does not prefetch if not in file-by-file mode'} | ${false} + ${'prefetches when in file-by-file mode'} | ${true} + `('$description', ({ fileByFile }) => { + createComponent({ + props: { viewDiffsFileByFile: fileByFile }, + file: readableFile, + }); + + if (fileByFile) { + expect(prefetchFileNeighborsMock).toHaveBeenCalled(); + } else { + expect(prefetchFileNeighborsMock).not.toHaveBeenCalled(); + } + }); + }); + describe('bus events', () => { beforeEach(() => { jest.spyOn(eventHub, '$emit').mockImplementation(() => {}); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index bbe748b8e1fc8666cf8babe5efb5da0e02b5f5b0..387407a7e4d6cb08c6775f76968d9c336e26cdb5 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -147,6 +147,175 @@ describe('DiffsStoreActions', () => { }); }); + describe('prefetchSingleFile', () => { + beforeEach(() => { + window.location.hash = 'e334a2a10f036c00151a04cea7938a5d4213a818'; + }); + + it('should do nothing if the tree entry is already loading', () => { + return testAction(diffActions.prefetchSingleFile, { diffLoading: true }, {}, [], []); + }); + + it('should do nothing if the tree entry has already been marked as loaded', () => { + return testAction( + diffActions.prefetchSingleFile, + { diffLoaded: true }, + { + flatBlobsList: [ + { fileHash: 'e334a2a10f036c00151a04cea7938a5d4213a818', diffLoaded: true }, + ], + }, + [], + [], + ); + }); + + describe('when a tree entry exists for the file, but it has not been marked as loaded', () => { + let state; + let getters; + let commit; + let hubSpy; + const defaultParams = { + old_path: 'old/123', + new_path: 'new/123', + w: '1', + view: 'inline', + }; + const endpointDiffForPath = '/diffs/set/endpoint/path'; + const diffForPath = mergeUrlParams(defaultParams, endpointDiffForPath); + const treeEntry = { + fileHash: 'e334a2a10f036c00151a04cea7938a5d4213a818', + filePaths: { old: 'old/123', new: 'new/123' }, + }; + const fileResult = { + diff_files: [{ file_hash: 'e334a2a10f036c00151a04cea7938a5d4213a818' }], + }; + + beforeEach(() => { + commit = jest.fn(); + state = { + endpointDiffForPath, + diffFiles: [], + }; + getters = { + flatBlobsList: [treeEntry], + getDiffFileByHash(hash) { + return state.diffFiles?.find((entry) => entry.file_hash === hash); + }, + }; + hubSpy = jest.spyOn(diffsEventHub, '$emit'); + }); + + it('does nothing if the file already exists in the loaded diff files', () => { + state.diffFiles = fileResult.diff_files; + + return testAction(diffActions.prefetchSingleFile, treeEntry, getters, [], []); + }); + + it('does some standard work every time', async () => { + mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + expect(commit).toHaveBeenCalledWith(types.TREE_ENTRY_DIFF_LOADING, { + path: treeEntry.filePaths.new, + }); + + // wait for the mocked network request to return + await waitForPromises(); + + expect(commit).toHaveBeenCalledWith(types.SET_DIFF_DATA_BATCH, fileResult); + + expect(hubSpy).toHaveBeenCalledWith('diffFilesModified'); + }); + + it('should fetch data without commit ID', async () => { + getters.commitId = null; + mock.onGet(diffForPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + // This tests that commit_id is NOT added, if there isn't one in the store + expect(mock.history.get[0].url).toEqual(diffForPath); + }); + + it('should fetch data with commit ID', async () => { + const finalPath = mergeUrlParams( + { ...defaultParams, commit_id: '123' }, + endpointDiffForPath, + ); + + getters.commitId = '123'; + mock.onGet(finalPath).reply(HTTP_STATUS_OK, fileResult); + + await diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(mock.history.get[0].url).toEqual(finalPath); + }); + + describe('version parameters', () => { + const diffId = '4'; + const startSha = 'abc'; + const pathRoot = 'a/a/-/merge_requests/1'; + + it('fetches the data when there is no mergeRequestDiff', async () => { + diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return and start processing the .then + await waitForPromises(); + + expect(mock.history.get[0].url).toEqual(diffForPath); + }); + + it.each` + desc | versionPath | start_sha | diff_id + ${'no additional version information'} | ${`${pathRoot}?search=terms`} | ${undefined} | ${undefined} + ${'the diff_id'} | ${`${pathRoot}?diff_id=${diffId}`} | ${undefined} | ${diffId} + ${'the start_sha'} | ${`${pathRoot}?start_sha=${startSha}`} | ${startSha} | ${undefined} + ${'all available version information'} | ${`${pathRoot}?diff_id=${diffId}&start_sha=${startSha}`} | ${startSha} | ${diffId} + `('fetches the data and includes $desc', async ({ versionPath, start_sha, diff_id }) => { + const finalPath = mergeUrlParams( + { ...defaultParams, diff_id, start_sha }, + endpointDiffForPath, + ); + state.mergeRequestDiff = { version_path: versionPath }; + mock.onGet(finalPath).reply(HTTP_STATUS_OK, fileResult); + + diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return + await waitForPromises(); + + expect(mock.history.get[0].url).toEqual(finalPath); + }); + }); + + describe('when the prefetch fails', () => { + beforeEach(() => { + mock.onGet(diffForPath).reply(HTTP_STATUS_INTERNAL_SERVER_ERROR); + }); + + it('should commit a mutation to set the tree entry diff loading to false', async () => { + diffActions.prefetchSingleFile({ state, getters, commit }, treeEntry); + + // wait for the mocked network request to return + await waitForPromises(); + + expect(commit).toHaveBeenCalledWith(types.TREE_ENTRY_DIFF_LOADING, { + path: treeEntry.filePaths.new, + loading: false, + }); + }); + }); + }); + }); + describe('fetchFileByFile', () => { beforeEach(() => { window.location.hash = 'e334a2a10f036c00151a04cea7938a5d4213a818'; @@ -460,6 +629,37 @@ describe('DiffsStoreActions', () => { }); }); + describe('prefetchFileNeighbors', () => { + it('dispatches two requests to prefetch the next/previous files', () => { + testAction( + diffActions.prefetchFileNeighbors, + {}, + { + currentDiffIndex: 0, + flatBlobsList: [ + { + type: 'blob', + fileHash: 'abc', + }, + { + type: 'blob', + fileHash: 'def', + }, + { + type: 'blob', + fileHash: 'ghi', + }, + ], + }, + [], + [ + { type: 'prefetchSingleFile', payload: { type: 'blob', fileHash: 'def' } }, + { type: 'prefetchSingleFile', payload: { type: 'blob', fileHash: 'abc' } }, + ], + ); + }); + }); + describe('fetchCoverageFiles', () => { const endpointCoverage = '/fetch'; diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js index 274cb40dac8e7379e886cba6bc49995835114097..e87c5d0a9b1e230c456894c6ce37ccf5d19e2c49 100644 --- a/spec/frontend/diffs/store/mutations_spec.js +++ b/spec/frontend/diffs/store/mutations_spec.js @@ -684,6 +684,36 @@ describe('DiffsStoreMutations', () => { }); }); + describe('TREE_ENTRY_DIFF_LOADING', () => { + it('sets the entry loading state to true by default', () => { + const state = { + treeEntries: { + path: { + diffLoading: false, + }, + }, + }; + + mutations[types.TREE_ENTRY_DIFF_LOADING](state, { path: 'path' }); + + expect(state.treeEntries.path.diffLoading).toBe(true); + }); + + it('sets the entry loading state to the provided value', () => { + const state = { + treeEntries: { + path: { + diffLoading: true, + }, + }, + }; + + mutations[types.TREE_ENTRY_DIFF_LOADING](state, { path: 'path', loading: false }); + + expect(state.treeEntries.path.diffLoading).toBe(false); + }); + }); + describe('SET_SHOW_TREE_LIST', () => { it('sets showTreeList', () => { const state = createState(); diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index 117ed56e3479e0bdbd869b3cf3831cce7d3b55d9..24cb8158739c8e41719c8798f159aefe01d4eaa0 100644 --- a/spec/frontend/diffs/store/utils_spec.js +++ b/spec/frontend/diffs/store/utils_spec.js @@ -948,9 +948,9 @@ describe('DiffsStoreUtils', () => { describe('markTreeEntriesLoaded', () => { it.each` desc | entries | loaded | outcome - ${'marks an existing entry as loaded'} | ${{ abc: {} }} | ${[{ new_path: 'abc' }]} | ${{ abc: { diffLoaded: true } }} + ${'marks an existing entry as loaded'} | ${{ abc: {} }} | ${[{ new_path: 'abc' }]} | ${{ abc: { diffLoaded: true, diffLoading: false } }} ${'does nothing if the new file is not found in the tree entries'} | ${{ abc: {} }} | ${[{ new_path: 'def' }]} | ${{ abc: {} }} - ${'leaves entries unmodified if they are not in the loaded files'} | ${{ abc: {}, def: { diffLoaded: true }, ghi: {} }} | ${[{ new_path: 'ghi' }]} | ${{ abc: {}, def: { diffLoaded: true }, ghi: { diffLoaded: true } }} + ${'leaves entries unmodified if they are not in the loaded files'} | ${{ abc: {}, def: { diffLoaded: true }, ghi: {} }} | ${[{ new_path: 'ghi' }]} | ${{ abc: {}, def: { diffLoaded: true }, ghi: { diffLoaded: true, diffLoading: false } }} `('$desc', ({ entries, loaded, outcome }) => { expect(utils.markTreeEntriesLoaded({ priorEntries: entries, loadedFiles: loaded })).toEqual( outcome, diff --git a/spec/frontend/diffs/utils/tree_worker_utils_spec.js b/spec/frontend/diffs/utils/tree_worker_utils_spec.js index b8bd4fcd0814a63f70ffabe1d57ad1b60328b67c..5fba00f9258acc16f120fee614756dcd3be2ad56 100644 --- a/spec/frontend/diffs/utils/tree_worker_utils_spec.js +++ b/spec/frontend/diffs/utils/tree_worker_utils_spec.js @@ -76,6 +76,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 0, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -103,6 +104,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 0, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -123,6 +125,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 0, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -154,6 +157,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { addedLines: 42, changed: true, diffLoaded: false, + diffLoading: false, deleted: false, fileHash: 'test', filePaths: { @@ -181,6 +185,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { type: 'blob', changed: true, diffLoaded: false, + diffLoading: false, tempFile: true, submodule: true, deleted: false, @@ -201,6 +206,7 @@ describe('~/diffs/utils/tree_worker_utils', () => { type: 'blob', changed: true, diffLoaded: false, + diffLoading: false, tempFile: false, submodule: undefined, deleted: true,