diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index c8ba4a1676da24933b740885f56ad41aadab248a..2b5670bd9c7a021546f5b0573ff71c4037699955 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -226,6 +226,7 @@ export default { 'isVirtualScrollingEnabled', 'isBatchLoading', 'isBatchLoadingError', + 'flatBlobsList', ]), ...mapGetters(['isNotesFetched', 'getNoteableData']), diffs() { @@ -241,7 +242,7 @@ export default { return this.currentUser.can_fork === true && this.currentUser.can_create_merge_request; }, renderDiffFiles() { - return this.diffFiles.length > 0; + return this.flatBlobsList.length > 0; }, renderFileTree() { return this.renderDiffFiles && this.showTreeList; @@ -253,7 +254,7 @@ export default { return this.startVersion === null && this.latestDiff; }, showFileByFileNavigation() { - return this.diffFiles.length > 1 && this.viewDiffsFileByFile; + return this.flatBlobsList.length > 1 && this.viewDiffsFileByFile; }, currentFileNumber() { return this.currentDiffIndex + 1; @@ -264,9 +265,9 @@ export default { return currentDiffIndex >= 1 ? currentDiffIndex : null; }, nextFileNumber() { - const { currentFileNumber, diffFiles } = this; + const { currentFileNumber, flatBlobsList } = this; - return currentFileNumber < diffFiles.length ? currentFileNumber + 1 : null; + return currentFileNumber < flatBlobsList.length ? currentFileNumber + 1 : null; }, visibleWarning() { let visible = false; @@ -385,7 +386,7 @@ export default { this.subscribeToEvents(); this.unwatchDiscussions = this.$watch( - () => `${this.diffFiles.length}:${this.$store.state.notes.discussions.length}`, + () => `${this.flatBlobsList.length}:${this.$store.state.notes.discussions.length}`, () => { this.setDiscussions(); @@ -572,8 +573,8 @@ export default { }, jumpToFile(step) { const targetIndex = this.currentDiffIndex + step; - if (targetIndex >= 0 && targetIndex < this.diffFiles.length) { - this.scrollToFile({ path: this.diffFiles[targetIndex].file_path }); + if (targetIndex >= 0 && targetIndex < this.flatBlobsList.length) { + this.scrollToFile({ path: this.flatBlobsList[targetIndex].path }); } }, setTreeDisplay() { @@ -582,7 +583,7 @@ export default { if (storedTreeShow !== null) { showTreeList = parseBoolean(storedTreeShow); - } else if (!bp.isDesktop() || (!this.isBatchLoading && this.diffFiles.length <= 1)) { + } else if (!bp.isDesktop() || (!this.isBatchLoading && this.flatBlobsList.length <= 1)) { showTreeList = false; } @@ -753,7 +754,7 @@ export default { /> - + diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 9d7a540570e0c170c49fa2a01d627542e33f41e4..e06c15bc1b4b609efcc60e7cfcb27c689bc730d5 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -819,20 +819,20 @@ export function moveToNeighboringCommit({ dispatch, state }, { direction }) { } } -export const setCurrentDiffFileIdFromNote = ({ commit, state, rootGetters }, noteId) => { +export const setCurrentDiffFileIdFromNote = ({ commit, getters, rootGetters }, noteId) => { const note = rootGetters.notesById[noteId]; if (!note) return; const fileHash = rootGetters.getDiscussion(note.discussion_id).diff_file?.file_hash; - if (fileHash && state.diffFiles.some((f) => f.file_hash === fileHash)) { + if (fileHash && getters.flatBlobsList.some((f) => f.fileHash === fileHash)) { commit(types.SET_CURRENT_DIFF_FILE, fileHash); } }; -export const navigateToDiffFileIndex = ({ commit, state }, index) => { - const fileHash = state.diffFiles[index].file_hash; +export const navigateToDiffFileIndex = ({ commit, getters }, index) => { + const { fileHash } = getters.flatBlobsList[index]; document.location.hash = fileHash; commit(types.SET_CURRENT_DIFF_FILE, fileHash); diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 3a85c1a9fe140538eaa1402f74e011a8dfc34209..f530c05ae1e1a3d2f4a9c262379154cccdde25b0 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -148,7 +148,7 @@ export const fileLineCodequality = () => () => { export const currentDiffIndex = (state) => Math.max( 0, - state.diffFiles.findIndex((diff) => diff.file_hash === state.currentDiffFileId), + flatBlobsList(state).findIndex((diff) => diff.fileHash === state.currentDiffFileId), ); export const diffLines = (state) => (file) => { diff --git a/spec/frontend/diffs/components/app_spec.js b/spec/frontend/diffs/components/app_spec.js index 3048a40809ead1cc4c59f79a3b675dc33e882b66..3d8cbaa6b895d156bccb91e40387e114e50ddc68 100644 --- a/spec/frontend/diffs/components/app_spec.js +++ b/spec/frontend/diffs/components/app_spec.js @@ -259,7 +259,7 @@ describe('diffs/components/app', () => { it('sets width of tree list', () => { createComponent({}, ({ state }) => { - state.diffs.diffFiles = [{ file_hash: '111', file_path: '111.js' }]; + state.diffs.treeEntries = { 111: { type: 'blob', fileHash: '111', path: '111.js' } }; }); expect(wrapper.find('.js-diff-tree-list').element.style.width).toEqual('320px'); @@ -288,7 +288,8 @@ describe('diffs/components/app', () => { it('does not render empty state when diff files exist', () => { createComponent({}, ({ state }) => { - state.diffs.diffFiles.push({ id: 1 }); + state.diffs.diffFiles = ['anything']; + state.diffs.treeEntries['1'] = { type: 'blob', id: 1 }; }); expect(wrapper.findComponent(NoChanges).exists()).toBe(false); @@ -382,10 +383,10 @@ describe('diffs/components/app', () => { beforeEach(() => { createComponent({}, () => { - store.state.diffs.diffFiles = [ - { file_hash: '111', file_path: '111.js' }, - { file_hash: '222', file_path: '222.js' }, - { file_hash: '333', file_path: '333.js' }, + store.state.diffs.treeEntries = [ + { type: 'blob', fileHash: '111', path: '111.js' }, + { type: 'blob', fileHash: '222', path: '222.js' }, + { type: 'blob', fileHash: '333', path: '333.js' }, ]; }); spy = jest.spyOn(store, 'dispatch'); @@ -501,7 +502,6 @@ describe('diffs/components/app', () => { describe('diffs', () => { it('should render compare versions component', () => { createComponent({}, ({ state }) => { - state.diffs.diffFiles = [{ file_hash: '111', file_path: '111.js' }]; state.diffs.mergeRequestDiffs = diffsMockData; state.diffs.targetBranchName = 'target-branch'; state.diffs.mergeRequestDiff = mergeRequestDiff; @@ -572,7 +572,12 @@ describe('diffs/components/app', () => { it('should display diff file if there are diff files', () => { createComponent({}, ({ state }) => { - state.diffs.diffFiles.push({ sha: '123' }); + state.diffs.diffFiles = [{ file_hash: '111', file_path: '111.js' }]; + state.diffs.treeEntries = { + 111: { type: 'blob', fileHash: '111', path: '111.js' }, + 123: { type: 'blob', fileHash: '123', path: '123.js' }, + 312: { type: 'blob', fileHash: '312', path: '312.js' }, + }; }); expect(wrapper.findComponent({ name: 'DynamicScroller' }).exists()).toBe(true); @@ -589,7 +594,7 @@ describe('diffs/components/app', () => { it('should render tree list', () => { createComponent({}, ({ state }) => { - state.diffs.diffFiles = [{ file_hash: '111', file_path: '111.js' }]; + state.diffs.treeEntries = { 111: { type: 'blob', fileHash: '111', path: '111.js' } }; }); expect(wrapper.findComponent(TreeList).exists()).toBe(true); @@ -603,7 +608,7 @@ describe('diffs/components/app', () => { it('calls setShowTreeList when only 1 file', () => { createComponent({}, ({ state }) => { - state.diffs.diffFiles.push({ sha: '123' }); + state.diffs.treeEntries = { 123: { type: 'blob', fileHash: '123' } }; }); jest.spyOn(store, 'dispatch'); wrapper.vm.setTreeDisplay(); @@ -614,10 +619,12 @@ describe('diffs/components/app', () => { }); }); - it('calls setShowTreeList with true when more than 1 file is in diffs array', () => { + it('calls setShowTreeList with true when more than 1 file is in tree entries map', () => { createComponent({}, ({ state }) => { - state.diffs.diffFiles.push({ sha: '123' }); - state.diffs.diffFiles.push({ sha: '124' }); + state.diffs.treeEntries = { + 111: { type: 'blob', fileHash: '111', path: '111.js' }, + 123: { type: 'blob', fileHash: '123', path: '123.js' }, + }; }); jest.spyOn(store, 'dispatch'); @@ -637,7 +644,7 @@ describe('diffs/components/app', () => { localStorage.setItem('mr_tree_show', showTreeList); createComponent({}, ({ state }) => { - state.diffs.diffFiles.push({ sha: '123' }); + state.diffs.treeEntries['123'] = { sha: '123' }; }); jest.spyOn(store, 'dispatch'); @@ -653,7 +660,10 @@ describe('diffs/components/app', () => { describe('file-by-file', () => { it('renders a single diff', async () => { createComponent({ fileByFileUserPreference: true }, ({ state }) => { - state.diffs.diffFiles.push({ file_hash: '123' }); + state.diffs.treeEntries = { + 123: { type: 'blob', fileHash: '123' }, + 312: { type: 'blob', fileHash: '312' }, + }; state.diffs.diffFiles.push({ file_hash: '312' }); }); @@ -668,7 +678,10 @@ describe('diffs/components/app', () => { it('sets previous button as disabled', async () => { createComponent({ fileByFileUserPreference: true }, ({ state }) => { - state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' }); + state.diffs.treeEntries = { + 123: { type: 'blob', fileHash: '123' }, + 312: { type: 'blob', fileHash: '312' }, + }; }); await nextTick(); @@ -679,7 +692,10 @@ describe('diffs/components/app', () => { it('sets next button as disabled', async () => { createComponent({ fileByFileUserPreference: true }, ({ state }) => { - state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' }); + state.diffs.treeEntries = { + 123: { type: 'blob', fileHash: '123' }, + 312: { type: 'blob', fileHash: '312' }, + }; state.diffs.currentDiffFileId = '312'; }); @@ -691,7 +707,7 @@ describe('diffs/components/app', () => { it("doesn't display when there's fewer than 2 files", async () => { createComponent({ fileByFileUserPreference: true }, ({ state }) => { - state.diffs.diffFiles.push({ file_hash: '123' }); + state.diffs.treeEntries = { 123: { type: 'blob', fileHash: '123' } }; state.diffs.currentDiffFileId = '123'; }); @@ -708,7 +724,10 @@ describe('diffs/components/app', () => { 'calls navigateToDiffFileIndex with $index when $link is clicked', async ({ currentDiffFileId, targetFile }) => { createComponent({ fileByFileUserPreference: true }, ({ state }) => { - state.diffs.diffFiles.push({ file_hash: '123' }, { file_hash: '312' }); + state.diffs.treeEntries = { + 123: { type: 'blob', fileHash: '123' }, + 312: { type: 'blob', fileHash: '312' }, + }; state.diffs.currentDiffFileId = currentDiffFileId; }); diff --git a/spec/frontend/diffs/store/actions_spec.js b/spec/frontend/diffs/store/actions_spec.js index 614742d026f349d8babaa3eb694e03c11add5c97..b5e773ecdabaf6849f64061121788b128c0ef4f6 100644 --- a/spec/frontend/diffs/store/actions_spec.js +++ b/spec/frontend/diffs/store/actions_spec.js @@ -393,7 +393,7 @@ describe('DiffsStoreActions', () => { return testAction( diffActions.assignDiscussionsToDiff, [], - { diffFiles: [] }, + { diffFiles: [], flatBlobsList: [] }, [], [{ type: 'setCurrentDiffFileIdFromNote', payload: '123' }], ); @@ -1397,39 +1397,38 @@ describe('DiffsStoreActions', () => { describe('setCurrentDiffFileIdFromNote', () => { it('commits SET_CURRENT_DIFF_FILE', () => { const commit = jest.fn(); - const state = { diffFiles: [{ file_hash: '123' }] }; + const getters = { flatBlobsList: [{ fileHash: '123' }] }; const rootGetters = { getDiscussion: () => ({ diff_file: { file_hash: '123' } }), notesById: { 1: { discussion_id: '2' } }, }; - diffActions.setCurrentDiffFileIdFromNote({ commit, state, rootGetters }, '1'); + diffActions.setCurrentDiffFileIdFromNote({ commit, getters, rootGetters }, '1'); expect(commit).toHaveBeenCalledWith(types.SET_CURRENT_DIFF_FILE, '123'); }); it('does not commit SET_CURRENT_DIFF_FILE when discussion has no diff_file', () => { const commit = jest.fn(); - const state = { diffFiles: [{ file_hash: '123' }] }; const rootGetters = { getDiscussion: () => ({ id: '1' }), notesById: { 1: { discussion_id: '2' } }, }; - diffActions.setCurrentDiffFileIdFromNote({ commit, state, rootGetters }, '1'); + diffActions.setCurrentDiffFileIdFromNote({ commit, rootGetters }, '1'); expect(commit).not.toHaveBeenCalled(); }); it('does not commit SET_CURRENT_DIFF_FILE when diff file does not exist', () => { const commit = jest.fn(); - const state = { diffFiles: [{ file_hash: '123' }] }; + const getters = { flatBlobsList: [{ fileHash: '123' }] }; const rootGetters = { getDiscussion: () => ({ diff_file: { file_hash: '124' } }), notesById: { 1: { discussion_id: '2' } }, }; - diffActions.setCurrentDiffFileIdFromNote({ commit, state, rootGetters }, '1'); + diffActions.setCurrentDiffFileIdFromNote({ commit, getters, rootGetters }, '1'); expect(commit).not.toHaveBeenCalled(); }); @@ -1440,7 +1439,7 @@ describe('DiffsStoreActions', () => { return testAction( diffActions.navigateToDiffFileIndex, 0, - { diffFiles: [{ file_hash: '123' }] }, + { flatBlobsList: [{ fileHash: '123' }] }, [{ type: types.SET_CURRENT_DIFF_FILE, payload: '123' }], [], ); diff --git a/spec/frontend/diffs/store/getters_spec.js b/spec/frontend/diffs/store/getters_spec.js index 2e3a66d5b012bf86e190dbdc4b22845de177527f..70e0689786a9cd661f4c6cd576868e09573aa849 100644 --- a/spec/frontend/diffs/store/getters_spec.js +++ b/spec/frontend/diffs/store/getters_spec.js @@ -328,7 +328,11 @@ describe('Diffs Module Getters', () => { describe('currentDiffIndex', () => { it('returns index of currently selected diff in diffList', () => { - localState.diffFiles = [{ file_hash: '111' }, { file_hash: '222' }, { file_hash: '333' }]; + localState.treeEntries = [ + { type: 'blob', fileHash: '111' }, + { type: 'blob', fileHash: '222' }, + { type: 'blob', fileHash: '333' }, + ]; localState.currentDiffFileId = '222'; expect(getters.currentDiffIndex(localState)).toEqual(1); @@ -339,7 +343,11 @@ describe('Diffs Module Getters', () => { }); it('returns 0 if no diff is selected yet or diff is not found', () => { - localState.diffFiles = [{ file_hash: '111' }, { file_hash: '222' }, { file_hash: '333' }]; + localState.treeEntries = [ + { type: 'blob', fileHash: '111' }, + { type: 'blob', fileHash: '222' }, + { type: 'blob', fileHash: '333' }, + ]; localState.currentDiffFileId = ''; expect(getters.currentDiffIndex(localState)).toEqual(0);