From 38d01bb3728b1928b2a971610a9a3d0d5510c20f Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 13 Feb 2023 19:51:00 -0700 Subject: [PATCH 01/11] Don't add a batch files startup request if single file view is active --- app/views/projects/merge_requests/_page.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/projects/merge_requests/_page.html.haml b/app/views/projects/merge_requests/_page.html.haml index f042fd56132062..1070b756d4c6be 100644 --- a/app/views/projects/merge_requests/_page.html.haml +++ b/app/views/projects/merge_requests/_page.html.haml @@ -18,7 +18,7 @@ - add_page_specific_style 'page_bundles/ci_status' - add_page_startup_api_call @endpoint_metadata_url -- if mr_action == 'diffs' +- if mr_action == 'diffs' && !@file_by_file_default - add_page_startup_api_call @endpoint_diff_batch_url .merge-request{ data: { mr_action: mr_action, url: merge_request_path(@merge_request, format: :json), project_path: project_path(@merge_request.project), lock_version: @merge_request.lock_version, diffs_batch_cache_key: @diffs_batch_cache_key } } -- GitLab From e67c8310288f0cad96610f71465449a7a8a1fa74 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 13 Feb 2023 23:05:01 -0700 Subject: [PATCH 02/11] Add basic implementation of loading only a single diff file In `viewDiffsFileByFile` mode, the app will wait until the diffs metadata is loaded - this basic amount of information is absolutely crucial to being able to load one file - and then trigger the `fetchFileByFile` action. This is INSTEAD of the unscoped triggering of the `fetchDiffFilesBatch` action, which doesn't need the metadata in order to begin. `fetchDiffFilesBatch` is entirely skipped in file-by-file mode. When using the file-by-file sibling navigation buttons, we also need to trigger the `fetchFileByFile` action, since simply updating the current diff file hash in the store is not enough to show that file (since it's not yet loaded, we have to fetch it). Changelog: changed --- .../javascripts/diffs/components/app.vue | 33 +++++++++++-------- .../diffs/components/diff_file.vue | 2 -- app/assets/javascripts/diffs/store/actions.js | 6 +++- spec/frontend/diffs/components/app_spec.js | 4 +-- spec/frontend/diffs/store/actions_spec.js | 10 ++++++ 5 files changed, 37 insertions(+), 18 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 99bc3780b557e9..7e9574364b92f8 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -429,6 +429,7 @@ export default { 'setCodequalityEndpoint', 'fetchDiffFilesMeta', 'fetchDiffFilesBatch', + 'fetchFileByFile', 'fetchCoverageFiles', 'fetchCodequality', 'startRenderDiffsQueue', @@ -472,6 +473,10 @@ export default { if (data) { realSize = data.real_size; + + if (this.viewDiffsFileByFile) { + this.fetchFileByFile(); + } } this.diffFilesLength = parseInt(realSize, 10) || 0; @@ -489,20 +494,22 @@ export default { }); }); - this.fetchDiffFilesBatch() - .then(() => { - if (toggleTree) this.setTreeDisplay(); - // Guarantee the discussions are assigned after the batch finishes. - // Just watching the length of the discussions or the diff files - // isn't enough, because with split diff loading, neither will - // change when loading the other half of the diff files. - this.setDiscussions(); - }) - .catch(() => { - createAlert({ - message: __('Something went wrong on our end. Please try again!'), + if (!this.viewDiffsFileByFile) { + this.fetchDiffFilesBatch() + .then(() => { + if (toggleTree) this.setTreeDisplay(); + // Guarantee the discussions are assigned after the batch finishes. + // Just watching the length of the discussions or the diff files + // isn't enough, because with split diff loading, neither will + // change when loading the other half of the diff files. + this.setDiscussions(); + }) + .catch(() => { + createAlert({ + message: __('Something went wrong on our end. Please try again!'), + }); }); - }); + } if (this.endpointCoverage) { this.fetchCoverageFiles(); diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index c19174dda8a20f..07c635d05c0e30 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -209,8 +209,6 @@ export default { if (this.hasDiff) { this.postRender(); - } else if (this.viewDiffsFileByFile && !this.isCollapsed) { - this.requestDiff(); } this.manageViewedEffects(); diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index f6552d391931bc..d53a87d0e7b260 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -926,11 +926,15 @@ export const setCurrentDiffFileIdFromNote = ({ commit, getters, rootGetters }, n } }; -export const navigateToDiffFileIndex = ({ commit, getters }, index) => { +export const navigateToDiffFileIndex = ({ state, getters, commit, dispatch }, index) => { const { fileHash } = getters.flatBlobsList[index]; document.location.hash = fileHash; commit(types.SET_CURRENT_DIFF_FILE, fileHash); + + if (state.viewDiffsFileByFile) { + dispatch('fetchFileByFile'); + } }; export const setFileByFile = ({ state, commit }, { fileByFile }) => { diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 545ba4a0e04667..e98c1728173727 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -723,8 +723,8 @@ describe('diffs/components/app', () => { async ({ currentDiffFileId, targetFile }) => { createComponent({ fileByFileUserPreference: true }, ({ state }) => { state.diffs.treeEntries = { - 123: { type: 'blob', fileHash: '123' }, - 312: { type: 'blob', fileHash: '312' }, + 123: { type: 'blob', fileHash: '123', filePaths: { old: '1234', new: '123' } }, + 312: { type: 'blob', fileHash: '312', filePaths: { old: '3124', new: '312' } }, }; state.diffs.currentDiffFileId = currentDiffFileId; }); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index b988472f947a7a..4662565fd7f0ba 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -1696,6 +1696,16 @@ describe('DiffsStoreActions', () => { [], ); }); + + it('dispatches the fetchFileByFile action when the state value viewDiffsFileByFile is true', () => { + return testAction( + diffActions.navigateToDiffFileIndex, + 0, + { viewDiffsFileByFile: true, flatBlobsList: [{ fileHash: '123' }] }, + [{ type: types.SET_CURRENT_DIFF_FILE, payload: '123' }], + [{ type: 'fetchFileByFile' }], + ); + }); }); describe('setFileByFile', () => { -- GitLab From 9d1b6fe3298a7c14e9c50b35e98219aa689991e1 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 13 Feb 2023 23:15:58 -0700 Subject: [PATCH 03/11] Parse the URL hash for a note ID when notes finish loading MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It's possible this may have been a race condition before, anyway. Now that the UI has totally asynchronous load of ALL of the UI data, we need to trigger parsing that used to only happen once. That is: - Discussions are loaded async - Files are loaded async - Any number of files may be present, from 0—total. Reading the note hash could extract a note ID that doesn't yet exist in the `notes` store. Or, the existing note may point to a file that hasn't yet loaded. For the former, we simply trigger this new action (which extracts the URL hash) when the notes indicate they have updated. For the latter, we trigger `fetchFileByFile` to ensure that the file that this note points to is actually loaded. `fetchFileByFile` will quickly short-circuit if it has already been loaded, so there's no harm in triggering it. --- app/assets/javascripts/diffs/components/app.vue | 3 +++ 1 file changed, 3 insertions(+) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 7e9574364b92f8..20059025066a24 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -432,6 +432,7 @@ export default { 'fetchFileByFile', 'fetchCoverageFiles', 'fetchCodequality', + 'rereadNoteHash', 'startRenderDiffsQueue', 'assignDiscussionsToDiff', 'setHighlightedRow', @@ -449,8 +450,10 @@ export default { subscribeToEvents() { notesEventHub.$once('fetchDiffData', this.fetchData); notesEventHub.$on('refetchDiffData', this.refetchDiffData); + notesEventHub.$on('fetchedNotesData', this.rereadNoteHash); }, unsubscribeFromEvents() { + notesEventHub.$off('fetchedNotesData', this.rereadNoteHash); notesEventHub.$off('refetchDiffData', this.refetchDiffData); notesEventHub.$off('fetchDiffData', this.fetchData); }, -- GitLab From 3bcb45c0a3471aa6fa4bf0bf71612897b7fb2079 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 13 Feb 2023 23:18:36 -0700 Subject: [PATCH 04/11] Assign discussions any time the files list changes In the new `fetchFileByFile` action, the end of a successful load fires the `diffFilesModified` event. Rather than try to work with the length of the diffFiles array (as previous implementations have done) this event is a clear signal that the files list has changed. The app's response to the files list changing is always to assign discussions to the available files, as the files and discussions are separate, so they need to be kept in sync when either changes. --- app/assets/javascripts/diffs/components/app.vue | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 20059025066a24..1c7eb5bb232975 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -448,6 +448,7 @@ export default { this.setDrawer({}); }, subscribeToEvents() { + diffsEventHub.$on('diffFilesModified', this.setDiscussions); notesEventHub.$once('fetchDiffData', this.fetchData); notesEventHub.$on('refetchDiffData', this.refetchDiffData); notesEventHub.$on('fetchedNotesData', this.rereadNoteHash); @@ -456,6 +457,7 @@ export default { notesEventHub.$off('fetchedNotesData', this.rereadNoteHash); notesEventHub.$off('refetchDiffData', this.refetchDiffData); notesEventHub.$off('fetchDiffData', this.fetchData); + diffsEventHub.$off('diffFilesModified', this.setDiscussions); }, navigateToDiffFileNumber(number) { this.navigateToDiffFileIndex(number - 1); -- GitLab From b67d8d342d4cb36815f4cd32784f95e8b9f3a956 Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 13 Feb 2023 23:22:54 -0700 Subject: [PATCH 05/11] Use superlative action `goToFile` to wrap `scrollToFile` When the UI is in file-by-file mode and a user clicks in the file tree, simply SCROLLING to the file won't work. The drop-in replacement for the `scrollToFile` action does a quick check (using the getter `isTreePathLoaded`) to see if we should FIRST set the current file hash, then trigger a fetch, and then FINALLY scroll to the newly loaded file. In reality, "scrolling" to the only file loaded is likely not useful but as a first iteration, the continuity of behavior and - crucially - fewer code branches is preferable. --- app/assets/javascripts/diffs/components/app.vue | 4 ++-- app/assets/javascripts/diffs/components/tree_list.vue | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index 1c7eb5bb232975..c9203cd427d4df 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -437,7 +437,7 @@ export default { 'assignDiscussionsToDiff', 'setHighlightedRow', 'cacheTreeListWidth', - 'scrollToFile', + 'goToFile', 'setShowTreeList', 'navigateToDiffFileIndex', 'setFileByFile', @@ -590,7 +590,7 @@ export default { jumpToFile(step) { const targetIndex = this.currentDiffIndex + step; if (targetIndex >= 0 && targetIndex < this.flatBlobsList.length) { - this.scrollToFile({ path: this.flatBlobsList[targetIndex].path }); + this.goToFile({ path: this.flatBlobsList[targetIndex].path }); } }, setTreeDisplay() { diff --git a/app/assets/javascripts/diffs/components/tree_list.vue b/app/assets/javascripts/diffs/components/tree_list.vue index 2675099a2f59e4..544bbbfe9d8a50 100644 --- a/app/assets/javascripts/diffs/components/tree_list.vue +++ b/app/assets/javascripts/diffs/components/tree_list.vue @@ -105,7 +105,7 @@ export default { this.resizeObserver.disconnect(); }, methods: { - ...mapActions('diffs', ['toggleTreeOpen', 'scrollToFile']), + ...mapActions('diffs', ['toggleTreeOpen', 'goToFile']), clearSearch() { this.search = ''; }, @@ -175,7 +175,7 @@ export default { :class="{ 'tree-list-parent': item.level > 0 }" class="gl-relative" @toggleTreeOpen="toggleTreeOpen" - @clickFile="(path) => scrollToFile({ path })" + @clickFile="(path) => goToFile({ path })" />