diff options
author | Phil Hughes <me@iamphill.com> | 2018-11-09 09:44:07 +0000 |
---|---|---|
committer | Phil Hughes <me@iamphill.com> | 2018-11-27 11:40:39 +0000 |
commit | adf8ad9eee20a2b4ea08054e36fede62ba110e57 (patch) | |
tree | ffdab5c766778eaf28a9a7cc1ddaea6928287516 /app/assets/javascripts/notes/stores | |
parent | 921d6b1a13b5ec59217ab714b4daa6800500d95f (diff) | |
download | gitlab-ce-adf8ad9eee20a2b4ea08054e36fede62ba110e57.tar.gz |
Improve discussion rendering performance
Improve the renderign of new and existing discussions
by reducing the number of watchers on each object & array.
Previously every discussion change would trigger an update for every
discussion component.
Also tidied up some components to get them closer to our docs.
Closes https://gitlab.com/gitlab-org/gitlab-ce/issues/51506
Diffstat (limited to 'app/assets/javascripts/notes/stores')
-rw-r--r-- | app/assets/javascripts/notes/stores/actions.js | 20 | ||||
-rw-r--r-- | app/assets/javascripts/notes/stores/getters.js | 44 | ||||
-rw-r--r-- | app/assets/javascripts/notes/stores/modules/index.js | 3 | ||||
-rw-r--r-- | app/assets/javascripts/notes/stores/mutation_types.js | 1 | ||||
-rw-r--r-- | app/assets/javascripts/notes/stores/mutations.js | 38 |
5 files changed, 63 insertions, 43 deletions
diff --git a/app/assets/javascripts/notes/stores/actions.js b/app/assets/javascripts/notes/stores/actions.js index 5b2f0540020..b4befdd6e4a 100644 --- a/app/assets/javascripts/notes/stores/actions.js +++ b/app/assets/javascripts/notes/stores/actions.js @@ -11,7 +11,7 @@ import * as constants from '../constants'; import service from '../services/notes_service'; import loadAwardsHandler from '../../awards_handler'; import sidebarTimeTrackingEventHub from '../../sidebar/event_hub'; -import { isInViewport, scrollToElement } from '../../lib/utils/common_utils'; +import { isInViewport, scrollToElement, isInMRPage } from '../../lib/utils/common_utils'; import mrWidgetEventHub from '../../vue_merge_request_widget/event_hub'; import { __ } from '~/locale'; @@ -39,12 +39,13 @@ export const setNotesFetchedState = ({ commit }, state) => export const toggleDiscussion = ({ commit }, data) => commit(types.TOGGLE_DISCUSSION, data); -export const fetchDiscussions = ({ commit }, { path, filter }) => +export const fetchDiscussions = ({ commit, dispatch }, { path, filter }) => service .fetchDiscussions(path, filter) .then(res => res.json()) .then(discussions => { commit(types.SET_INITIAL_DISCUSSIONS, discussions); + dispatch('updateResolvableDiscussonsCounts'); }); export const updateDiscussion = ({ commit, state }, discussion) => { @@ -53,11 +54,18 @@ export const updateDiscussion = ({ commit, state }, discussion) => { return utils.findNoteObjectById(state.discussions, discussion.id); }; -export const deleteNote = ({ commit, dispatch }, note) => +export const deleteNote = ({ commit, dispatch, state }, note) => service.deleteNote(note.path).then(() => { + const discussion = state.discussions.find(({ id }) => id === note.discussion_id); + commit(types.DELETE_NOTE, note); dispatch('updateMergeRequestWidget'); + dispatch('updateResolvableDiscussonsCounts'); + + if (isInMRPage()) { + dispatch('diffs/removeDiscussionsFromDiff', discussion); + } }); export const updateNote = ({ commit, dispatch }, { endpoint, note }) => @@ -89,6 +97,7 @@ export const createNewNote = ({ commit, dispatch }, { endpoint, data }) => dispatch('updateMergeRequestWidget'); dispatch('startTaskList'); + dispatch('updateResolvableDiscussonsCounts'); } return res; }); @@ -104,6 +113,8 @@ export const toggleResolveNote = ({ commit, dispatch }, { endpoint, isResolved, commit(mutationType, res); + dispatch('updateResolvableDiscussonsCounts'); + dispatch('updateMergeRequestWidget'); }); @@ -385,5 +396,8 @@ export const startTaskList = ({ dispatch }) => }), ); +export const updateResolvableDiscussonsCounts = ({ commit }) => + commit(types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS); + // prevent babel-plugin-rewire from generating an invalid default during karma tests export default () => {}; diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 980d79605d7..2ed8aac059a 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -53,30 +53,15 @@ export const getCurrentUserLastNote = state => export const getDiscussionLastNote = state => discussion => reverseNotes(discussion.notes).find(el => isLastNote(el, state)); -export const discussionCount = state => { - const filteredDiscussions = state.discussions.filter(n => !n.individual_note && n.resolvable); - - return filteredDiscussions.length; -}; - -export const unresolvedDiscussions = (state, getters) => { - const resolvedMap = getters.resolvedDiscussionsById; - - return state.discussions.filter(n => !n.individual_note && !resolvedMap[n.id]); -}; - -export const allDiscussions = (state, getters) => { - const resolved = getters.resolvedDiscussionsById; - const unresolved = getters.unresolvedDiscussions; - - return Object.values(resolved).concat(unresolved); -}; +export const unresolvedDiscussionsCount = state => state.unresolvedDiscussionsCount; +export const resolvableDiscussionsCount = state => state.resolvableDiscussionsCount; +export const hasUnresolvedDiscussions = state => state.hasUnresolvedDiscussions; export const isDiscussionResolved = (state, getters) => discussionId => getters.resolvedDiscussionsById[discussionId] !== undefined; -export const allResolvableDiscussions = (state, getters) => - getters.allDiscussions.filter(d => !d.individual_note && d.resolvable); +export const allResolvableDiscussions = state => + state.discussions.filter(d => !d.individual_note && d.resolvable); export const resolvedDiscussionsById = state => { const map = {}; @@ -147,15 +132,12 @@ export const resolvedDiscussionCount = (state, getters) => { return Object.keys(resolvedMap).length; }; -export const discussionTabCounter = state => { - let all = []; - - state.discussions.forEach(discussion => { - all = all.concat(discussion.notes.filter(note => !note.system && !note.placeholder)); - }); - - return all.length; -}; +export const discussionTabCounter = state => + state.discussions.reduce( + (acc, discussion) => + acc + discussion.notes.filter(note => !note.system && !note.placeholder).length, + 0, + ); // Returns the list of discussion IDs ordered according to given parameter // @param {Boolean} diffOrder - is ordered by diff? @@ -182,8 +164,10 @@ export const isLastUnresolvedDiscussion = (state, getters) => (discussionId, dif export const nextUnresolvedDiscussionId = (state, getters) => (discussionId, diffOrder) => { const idsOrdered = getters.unresolvedDiscussionsIdsOrdered(diffOrder); const currentIndex = idsOrdered.indexOf(discussionId); + const slicedIds = idsOrdered.slice(currentIndex + 1, currentIndex + 2); - return idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0]; + // Get the first ID if there is none after the currentIndex + return slicedIds.length ? idsOrdered.slice(currentIndex + 1, currentIndex + 2)[0] : idsOrdered[0]; }; // @param {Boolean} diffOrder - is ordered by diff? diff --git a/app/assets/javascripts/notes/stores/modules/index.js b/app/assets/javascripts/notes/stores/modules/index.js index 8aea269ea7d..b5fe8bdb1d3 100644 --- a/app/assets/javascripts/notes/stores/modules/index.js +++ b/app/assets/javascripts/notes/stores/modules/index.js @@ -22,6 +22,9 @@ export default () => ({ current_user: {}, }, commentsDisabled: false, + resolvableDiscussionsCount: 0, + unresolvedDiscussionsCount: 0, + hasUnresolvedDiscussions: false, }, actions, getters, diff --git a/app/assets/javascripts/notes/stores/mutation_types.js b/app/assets/javascripts/notes/stores/mutation_types.js index dfbf3b7b34b..9c68ab67a8c 100644 --- a/app/assets/javascripts/notes/stores/mutation_types.js +++ b/app/assets/javascripts/notes/stores/mutation_types.js @@ -21,6 +21,7 @@ export const DISABLE_COMMENTS = 'DISABLE_COMMENTS'; export const COLLAPSE_DISCUSSION = 'COLLAPSE_DISCUSSION'; export const EXPAND_DISCUSSION = 'EXPAND_DISCUSSION'; export const TOGGLE_DISCUSSION = 'TOGGLE_DISCUSSION'; +export const UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS = 'UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS'; // Issue export const CLOSE_ISSUE = 'CLOSE_ISSUE'; diff --git a/app/assets/javascripts/notes/stores/mutations.js b/app/assets/javascripts/notes/stores/mutations.js index f6054e0be87..667c8a97cf3 100644 --- a/app/assets/javascripts/notes/stores/mutations.js +++ b/app/assets/javascripts/notes/stores/mutations.js @@ -24,6 +24,7 @@ export default { noteData.resolved = false; noteData.resolve_path = note.resolve_path; noteData.resolve_with_issue_path = note.resolve_with_issue_path; + noteData.diff_discussion = false; } state.discussions.push(noteData); @@ -97,33 +98,36 @@ export default { }, [types.SET_INITIAL_DISCUSSIONS](state, discussionsData) { - const discussions = []; + const discussions = discussionsData.reduce((acc, d) => { + const discussion = { ...d }; + const diffData = {}; - discussionsData.forEach(discussion => { if (discussion.diff_file) { - Object.assign(discussion, { - file_hash: discussion.diff_file.file_hash, - truncated_diff_lines: discussion.truncated_diff_lines || [], - }); + diffData.file_hash = discussion.diff_file.file_hash; + diffData.truncated_diff_lines = discussion.truncated_diff_lines || []; } // To support legacy notes, should be very rare case. if (discussion.individual_note && discussion.notes.length > 1) { discussion.notes.forEach(n => { - discussions.push({ + acc.push({ ...discussion, + ...diffData, notes: [n], // override notes array to only have one item to mimick individual_note }); }); } else { const oldNote = utils.findNoteObjectById(state.discussions, discussion.id); - discussions.push({ + acc.push({ ...discussion, + ...diffData, expanded: oldNote ? oldNote.expanded : discussion.expanded, }); } - }); + + return acc; + }, []); Object.assign(state, { discussions }); }, @@ -195,7 +199,9 @@ export default { const selectedDiscussion = state.discussions.find(disc => disc.id === note.id); note.expanded = true; // override expand flag to prevent collapse if (note.diff_file) { - Object.assign(note, { file_hash: note.diff_file.file_hash }); + Object.assign(note, { + file_hash: note.diff_file.file_hash, + }); } Object.assign(selectedDiscussion, { ...note }); }, @@ -229,4 +235,16 @@ export default { [types.DISABLE_COMMENTS](state, value) { state.commentsDisabled = value; }, + [types.UPDATE_RESOLVABLE_DISCUSSIONS_COUNTS](state) { + state.resolvableDiscussionsCount = state.discussions.filter( + discussion => !discussion.individual_note && discussion.resolvable, + ).length; + state.unresolvedDiscussionsCount = state.discussions.filter( + discussion => + !discussion.individual_note && + discussion.resolvable && + discussion.notes.some(note => !note.resolved), + ).length; + state.hasUnresolvedDiscussions = state.unresolvedDiscussionsCount > 1; + }, }; |