diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index edca45f22f9..9ddca8548e0 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -41,6 +41,11 @@ export default { required: true, }, }, + data() { + return { + assignedDiscussions: false, + }; + }, computed: { ...mapState({ isLoading: state => state.diffs.isLoading, @@ -60,7 +65,7 @@ export default { }), ...mapState('diffs', ['showTreeList']), ...mapGetters('diffs', ['isParallelView']), - ...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']), + ...mapGetters(['isNotesFetched', 'getNoteableData']), targetBranch() { return { branchName: this.targetBranchName, @@ -147,11 +152,12 @@ export default { } }, setDiscussions() { - if (this.isNotesFetched) { + if (this.isNotesFetched && !this.assignedDiscussions) { requestIdleCallback( - () => { - this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode); - }, + () => + this.assignDiscussionsToDiff().then(() => { + this.assignedDiscussions = true; + }), { timeout: 1000 }, ); } diff --git a/app/assets/javascripts/diffs/components/diff_file.vue b/app/assets/javascripts/diffs/components/diff_file.vue index f72c7a84e5c..958e57c5652 100644 --- a/app/assets/javascripts/diffs/components/diff_file.vue +++ b/app/assets/javascripts/diffs/components/diff_file.vue @@ -29,7 +29,7 @@ export default { }, computed: { ...mapState('diffs', ['currentDiffFileId']), - ...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']), + ...mapGetters(['isNotesFetched']), isCollapsed() { return this.file.collapsed || false; }, @@ -79,7 +79,7 @@ export default { .then(() => { requestIdleCallback( () => { - this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode); + this.assignDiscussionsToDiff(); }, { timeout: 1000 }, ); diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 1e0b27b538d..e79351454ab 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -5,7 +5,6 @@ import createFlash from '~/flash'; import { s__ } from '~/locale'; import { handleLocationHash, historyPushState } from '~/lib/utils/common_utils'; import { mergeUrlParams, getLocationHash } from '~/lib/utils/url_utility'; -import { reduceDiscussionsToLineCodes } from '../../notes/stores/utils'; import { getDiffPositionByLineCode, getNoteFormData } from './utils'; import * as types from './mutation_types'; import { @@ -36,18 +35,33 @@ export const fetchDiffFiles = ({ state, commit }) => { // This is adding line discussions to the actual lines in the diff tree // once for parallel and once for inline mode -export const assignDiscussionsToDiff = ({ state, commit }, allLineDiscussions) => { +export const assignDiscussionsToDiff = ( + { commit, state, rootState }, + discussionsToAssign = rootState.notes.discussions, +) => { const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles); + const discussionsByDiffFile = discussionsToAssign + .filter(discussion => discussion.diff_discussion) + .reduce((acc, discussion) => { + const discussions = ( + acc[discussion.diff_file.file_hash] || { discussions: [] } + ).discussions.concat(discussion); - Object.values(allLineDiscussions).forEach(discussions => { - if (discussions.length > 0) { - const { fileHash } = discussions[0]; - commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, { - fileHash, - discussions, - diffPositionByLineCode, - }); - } + return { + ...acc, + [discussion.diff_file.file_hash]: { + diffFile: state.diffFiles.find(file => file.fileHash === discussion.diff_file.file_hash), + discussions, + }, + }; + }, {}); + + Object.values(discussionsByDiffFile).forEach(({ discussions, diffFile }) => { + commit(types.SET_LINE_DISCUSSIONS_FOR_FILE, { + diffFile, + discussions, + diffPositionByLineCode, + }); }); }; @@ -190,9 +204,7 @@ export const saveDiffDiscussion = ({ dispatch }, { note, formData }) => { return dispatch('saveNote', postData, { root: true }) .then(result => dispatch('updateDiscussion', result.discussion, { root: true })) - .then(discussion => - dispatch('assignDiscussionsToDiff', reduceDiscussionsToLineCodes([discussion])), - ) + .then(discussion => dispatch('assignDiscussionsToDiff', [discussion])) .catch(() => createFlash(s__('MergeRequests|Saving the comment failed'))); }; diff --git a/app/assets/javascripts/diffs/store/mutations.js b/app/assets/javascripts/diffs/store/mutations.js index 0b4485ecdb5..6a57a075579 100644 --- a/app/assets/javascripts/diffs/store/mutations.js +++ b/app/assets/javascripts/diffs/store/mutations.js @@ -90,53 +90,50 @@ export default { })); }, - [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, discussions, diffPositionByLineCode }) { - const selectedFile = state.diffFiles.find(f => f.fileHash === fileHash); - const firstDiscussion = discussions[0]; - const isDiffDiscussion = firstDiscussion.diff_discussion; - const hasLineCode = firstDiscussion.line_code; - const diffPosition = diffPositionByLineCode[firstDiscussion.line_code]; + [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { diffFile, discussions, diffPositionByLineCode }) { + const { latestDiff } = state; - if ( - selectedFile && - isDiffDiscussion && - hasLineCode && - diffPosition && - isDiscussionApplicableToLine({ - discussion: firstDiscussion, - diffPosition, - latestDiff: state.latestDiff, - }) - ) { - const targetLine = selectedFile.parallelDiffLines.find( - line => - (line.left && line.left.lineCode === firstDiscussion.line_code) || - (line.right && line.right.lineCode === firstDiscussion.line_code), - ); - if (targetLine) { - if (targetLine.left && targetLine.left.lineCode === firstDiscussion.line_code) { - Object.assign(targetLine.left, { - discussions, - }); - } else { - Object.assign(targetLine.right, { - discussions, + discussions.forEach(discussion => { + const discussionLineCode = discussion.line_code; + const lineCheck = ({ lineCode }) => + lineCode === discussionLineCode && + isDiscussionApplicableToLine({ + discussion, + diffPosition: diffPositionByLineCode[lineCode], + latestDiff, + }); + + if (diffFile.highlightedDiffLines) { + const line = diffFile.highlightedDiffLines.find(lineCheck); + + if (line) { + Object.assign(line, { + discussions: line.discussions.concat(discussion), }); } } - if (selectedFile.highlightedDiffLines) { - const targetInlineLine = selectedFile.highlightedDiffLines.find( - line => line.lineCode === firstDiscussion.line_code, + if (diffFile.parallelDiffLines) { + const { left, right } = diffFile.parallelDiffLines.find( + parallelLine => + (parallelLine.left && lineCheck(parallelLine.left)) || + (parallelLine.right && lineCheck(parallelLine.right)), ); + const line = left && left.lineCode === discussionLineCode ? left : right; - if (targetInlineLine) { - Object.assign(targetInlineLine, { - discussions, + if (line) { + Object.assign(line, { + discussions: line.discussions.concat(discussion), }); } } - } + + if (!diffFile.parallelDiffLines || !diffFile.highlightedDiffLines) { + Object.assign(diffFile, { + discussions: diffFile.discussions.concat(discussion), + }); + } + }); }, [types.REMOVE_LINE_DISCUSSIONS_FOR_FILE](state, { fileHash, lineCode }) { diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 21c334a9d33..e4f36154fcd 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -1,6 +1,5 @@ import _ from 'underscore'; import * as constants from '../constants'; -import { reduceDiscussionsToLineCodes } from './utils'; import { collapseSystemNotes } from './collapse_utils'; export const discussions = state => collapseSystemNotes(state.discussions); @@ -31,9 +30,6 @@ export const notesById = state => return acc; }, {}); -export const discussionsStructuredByLineCode = state => - reduceDiscussionsToLineCodes(state.discussions); - export const noteableType = state => { const { ISSUE_NOTEABLE_TYPE, MERGE_REQUEST_NOTEABLE_TYPE, EPIC_NOTEABLE_TYPE } = constants; diff --git a/app/assets/javascripts/notes/stores/utils.js b/app/assets/javascripts/notes/stores/utils.js index 0e41ff03d67..dd57539e4d8 100644 --- a/app/assets/javascripts/notes/stores/utils.js +++ b/app/assets/javascripts/notes/stores/utils.js @@ -25,18 +25,6 @@ export const getQuickActionText = note => { return text; }; -export const reduceDiscussionsToLineCodes = selectedDiscussions => - selectedDiscussions.reduce((acc, note) => { - if (note.diff_discussion && note.line_code) { - // For context about line notes: there might be multiple notes with the same line code - const items = acc[note.line_code] || []; - items.push(note); - - Object.assign(acc, { [note.line_code]: items }); - } - return acc; - }, {}); - export const hasQuickActions = note => REGEX_QUICK_ACTIONS.test(note); export const stripQuickActions = note => note.replace(REGEX_QUICK_ACTIONS, '').trim(); diff --git a/spec/javascripts/diffs/store/actions_spec.js b/spec/javascripts/diffs/store/actions_spec.js index 85c1926fcb1..12db3cc6a25 100644 --- a/spec/javascripts/diffs/store/actions_spec.js +++ b/spec/javascripts/diffs/store/actions_spec.js @@ -27,7 +27,6 @@ import actions, { toggleShowTreeList, } from '~/diffs/store/actions'; import * as types from '~/diffs/store/mutation_types'; -import { reduceDiscussionsToLineCodes } from '~/notes/stores/utils'; import axios from '~/lib/utils/axios_utils'; import testAction from '../../helpers/vuex_action_helper'; @@ -152,7 +151,7 @@ describe('DiffsStoreActions', () => { original_position: diffPosition, }; - const discussions = reduceDiscussionsToLineCodes([singleDiscussion]); + const discussions = [singleDiscussion]; testAction( assignDiscussionsToDiff, @@ -162,7 +161,7 @@ describe('DiffsStoreActions', () => { { type: types.SET_LINE_DISCUSSIONS_FOR_FILE, payload: { - fileHash: 'ABC', + diffFile: state.diffFiles[0], discussions: [singleDiscussion], diffPositionByLineCode: { ABC_1_1: { @@ -581,7 +580,6 @@ describe('DiffsStoreActions', () => { describe('saveDiffDiscussion', () => { beforeEach(() => { spyOnDependency(actions, 'getNoteFormData').and.returnValue('testData'); - spyOnDependency(actions, 'reduceDiscussionsToLineCodes').and.returnValue('discussions'); }); it('dispatches actions', done => { @@ -602,7 +600,7 @@ describe('DiffsStoreActions', () => { .then(() => { expect(dispatch.calls.argsFor(0)).toEqual(['saveNote', 'testData', { root: true }]); expect(dispatch.calls.argsFor(1)).toEqual(['updateDiscussion', 'test', { root: true }]); - expect(dispatch.calls.argsFor(2)).toEqual(['assignDiscussionsToDiff', 'discussions']); + expect(dispatch.calls.argsFor(2)).toEqual(['assignDiscussionsToDiff', ['discussion']]); }) .then(done) .catch(done.fail); diff --git a/spec/javascripts/diffs/store/mutations_spec.js b/spec/javascripts/diffs/store/mutations_spec.js index b7e28391419..3a9c8f3a825 100644 --- a/spec/javascripts/diffs/store/mutations_spec.js +++ b/spec/javascripts/diffs/store/mutations_spec.js @@ -222,7 +222,7 @@ describe('DiffsStoreMutations', () => { }; mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { - fileHash: 'ABC', + diffFile: state.diffFiles[0], discussions, diffPositionByLineCode, }); @@ -292,7 +292,7 @@ describe('DiffsStoreMutations', () => { }; mutations[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { - fileHash: 'ABC', + diffFile: state.diffFiles[0], discussions, diffPositionByLineCode, });