From d20aed01c47e3c80c5bf8af6cb4a5ed7828e7fdc Mon Sep 17 00:00:00 2001 From: Thomas Randolph Date: Mon, 13 Feb 2023 23:07:51 -0700 Subject: [PATCH] Mark diff tree entries as loaded when we place them into the store SET_DIFF_DATA_BATCH already assumes we may not have the full list of files. So whether we're loading in a batch of files, or one single file, this mutation can handle the update. This new code updates each tree entry that matches one of the inbound diff files to read as `diffLoaded = true`. By default this property is `false`. --- .../javascripts/diffs/store/mutations.js | 13 ++++++-- app/assets/javascripts/diffs/store/utils.js | 14 +++++++++ .../diffs/utils/tree_worker_utils.js | 5 ++++ spec/frontend/diffs/store/mutations_spec.js | 9 ++++-- spec/frontend/diffs/store/utils_spec.js | 13 ++++++++ .../diffs/utils/tree_worker_utils_spec.js | 30 +++++++++++++++++++ 6 files changed, 79 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 04f08e869552af..5e7fe8b5cd83cf 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -15,6 +15,7 @@ import { prepareDiffData, isDiscussionApplicableToLine, updateLineInFile, + markTreeEntriesLoaded, } from './utils'; function updateDiffFilesInState(state, files) { @@ -82,9 +83,15 @@ export default { }, [types.SET_DIFF_DATA_BATCH](state, data) { - state.diffFiles = prepareDiffData({ - diff: data, - priorFiles: state.diffFiles, + Object.assign(state, { + diffFiles: prepareDiffData({ + diff: data, + priorFiles: state.diffFiles, + }), + treeEntries: markTreeEntriesLoaded({ + priorEntries: state.treeEntries, + loadedFiles: data.diff_files, + }), }); }, diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 84a2af6c5936d2..3739ef0cd55c8a 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -585,3 +585,17 @@ export function parseUrlHashAsFileHash(urlHash, currentDiffFileId = '') { return id; } + +export function markTreeEntriesLoaded({ priorEntries, loadedFiles }) { + const newEntries = { ...priorEntries }; + + loadedFiles.forEach((newFile) => { + const entry = newEntries[newFile.new_path]; + + if (entry) { + entry.diffLoaded = true; + } + }); + + return newEntries; +} diff --git a/app/assets/javascripts/diffs/utils/tree_worker_utils.js b/app/assets/javascripts/diffs/utils/tree_worker_utils.js index a90c1a5c64e3e7..8689809cfa9d75 100644 --- a/app/assets/javascripts/diffs/utils/tree_worker_utils.js +++ b/app/assets/javascripts/diffs/utils/tree_worker_utils.js @@ -85,6 +85,11 @@ export const generateTreeList = (files) => { if (type === 'blob') { Object.assign(entry, { changed: true, + diffLoaded: false, + filePaths: { + old: file.old_path, + new: file.new_path, + }, tempFile: file.new_file, deleted: file.deleted_file, fileHash: file.file_hash, diff --git a/spec/frontend/diffs/store/mutations_spec.js b/spec/frontend/diffs/store/mutations_spec.js index 031e4fe2be24b6..ed8d7397bbc5f5 100644 --- a/spec/frontend/diffs/store/mutations_spec.js +++ b/spec/frontend/diffs/store/mutations_spec.js @@ -93,15 +93,20 @@ describe('DiffsStoreMutations', () => { describe('SET_DIFF_DATA_BATCH_DATA', () => { it('should set diff data batch type properly', () => { - const state = { diffFiles: [] }; + const mockFile = getDiffFileMock(); + const state = { + diffFiles: [], + treeEntries: { [mockFile.file_path]: { fileHash: mockFile.file_hash } }, + }; const diffMock = { - diff_files: [getDiffFileMock()], + diff_files: [mockFile], }; mutations[types.SET_DIFF_DATA_BATCH](state, diffMock); expect(state.diffFiles[0].renderIt).toEqual(true); expect(state.diffFiles[0].collapsed).toEqual(false); + expect(state.treeEntries[mockFile.file_path].diffLoaded).toBe(true); }); }); diff --git a/spec/frontend/diffs/store/utils_spec.js b/spec/frontend/diffs/store/utils_spec.js index 68bbbdbe17637a..4760a8b7166eaf 100644 --- a/spec/frontend/diffs/store/utils_spec.js +++ b/spec/frontend/diffs/store/utils_spec.js @@ -936,4 +936,17 @@ describe('DiffsStoreUtils', () => { expect(utils.parseUrlHashAsFileHash(input, currentDiffId)).toBe(resultId); }); }); + + describe('markTreeEntriesLoaded', () => { + it.each` + desc | entries | loaded | outcome + ${'marks an existing entry as loaded'} | ${{ abc: {} }} | ${[{ new_path: 'abc' }]} | ${{ abc: { diffLoaded: true } }} + ${'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 } }} + `('$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 4df5fe75004f7e..b8bd4fcd0814a6 100644 --- a/spec/frontend/diffs/utils/tree_worker_utils_spec.js +++ b/spec/frontend/diffs/utils/tree_worker_utils_spec.js @@ -75,8 +75,13 @@ describe('~/diffs/utils/tree_worker_utils', () => { { addedLines: 0, changed: true, + diffLoaded: false, deleted: false, fileHash: 'test', + filePaths: { + new: 'app/index.js', + old: undefined, + }, key: 'app/index.js', name: 'index.js', parentPath: 'app/', @@ -97,8 +102,13 @@ describe('~/diffs/utils/tree_worker_utils', () => { { addedLines: 0, changed: true, + diffLoaded: false, deleted: false, fileHash: 'test', + filePaths: { + new: 'app/test/index.js', + old: undefined, + }, key: 'app/test/index.js', name: 'index.js', parentPath: 'app/test/', @@ -112,8 +122,13 @@ describe('~/diffs/utils/tree_worker_utils', () => { { addedLines: 0, changed: true, + diffLoaded: false, deleted: false, fileHash: 'test', + filePaths: { + new: 'app/test/filepathneedstruncating.js', + old: undefined, + }, key: 'app/test/filepathneedstruncating.js', name: 'filepathneedstruncating.js', parentPath: 'app/test/', @@ -138,8 +153,13 @@ describe('~/diffs/utils/tree_worker_utils', () => { { addedLines: 42, changed: true, + diffLoaded: false, deleted: false, fileHash: 'test', + filePaths: { + new: 'constructor/test/aFile.js', + old: undefined, + }, key: 'constructor/test/aFile.js', name: 'aFile.js', parentPath: 'constructor/test/', @@ -160,10 +180,15 @@ describe('~/diffs/utils/tree_worker_utils', () => { name: 'submodule @ abcdef123', type: 'blob', changed: true, + diffLoaded: false, tempFile: true, submodule: true, deleted: false, fileHash: 'test', + filePaths: { + new: 'submodule @ abcdef123', + old: undefined, + }, addedLines: 1, removedLines: 0, tree: [], @@ -175,10 +200,15 @@ describe('~/diffs/utils/tree_worker_utils', () => { name: 'package.json', type: 'blob', changed: true, + diffLoaded: false, tempFile: false, submodule: undefined, deleted: true, fileHash: 'test', + filePaths: { + new: 'package.json', + old: undefined, + }, addedLines: 0, removedLines: 0, tree: [], -- GitLab