From d08a28ae1a218dd312d7a788b5dee92bd7eb3b13 Mon Sep 17 00:00:00 2001 From: Tim Zallmann Date: Mon, 13 Aug 2018 10:47:54 +0200 Subject: Loading Discussions later on Diffs --- app/assets/javascripts/diffs/components/app.vue | 25 +++++++- .../diffs/components/parallel_diff_comment_row.vue | 46 +++++++-------- .../diffs/components/parallel_diff_view.vue | 13 ++--- app/assets/javascripts/diffs/store/actions.js | 67 +++++++++++++++++----- app/assets/javascripts/diffs/store/getters.js | 67 +++++++++++++++++----- app/assets/javascripts/diffs/store/utils.js | 1 + app/assets/javascripts/notes/stores/getters.js | 7 ++- 7 files changed, 160 insertions(+), 66 deletions(-) diff --git a/app/assets/javascripts/diffs/components/app.vue b/app/assets/javascripts/diffs/components/app.vue index b5b05df4d34..b801dd56392 100644 --- a/app/assets/javascripts/diffs/components/app.vue +++ b/app/assets/javascripts/diffs/components/app.vue @@ -59,7 +59,7 @@ export default { emailPatchPath: state => state.diffs.emailPatchPath, }), ...mapGetters('diffs', ['isParallelView']), - ...mapGetters(['isNotesFetched']), + ...mapGetters(['isNotesFetched', 'discussionsStructuredByLineCode']), targetBranch() { return { branchName: this.targetBranchName, @@ -112,13 +112,32 @@ export default { }, created() { this.adjustView(); + eventHub.$once('renderedFiles', this.assignDiscussionsToDiff); }, methods: { - ...mapActions('diffs', ['setBaseConfig', 'fetchDiffFiles', 'startRenderDiffsQueue']), + ...mapActions('diffs', [ + 'setBaseConfig', + 'fetchDiffFiles', + 'startRenderDiffsQueue', + 'assignDiscussionsToDiff', + ]), + fetchData() { this.fetchDiffFiles() .then(() => { - requestIdleCallback(this.startRenderDiffsQueue, { timeout: 1000 }); + requestIdleCallback( + () => { + this.startRenderDiffsQueue() + .then(() => { + console.log('Done rendering Que'); + this.assignDiscussionsToDiff(this.discussionsStructuredByLineCode); + }) + .catch(() => { + createFlash(__('Something went wrong on our end. Please try again!')); + }); + }, + { timeout: 1000 }, + ); }) .catch(() => { createFlash(__('Something went wrong on our end. Please try again!')); diff --git a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue index 48b8feeb0b4..90f7fd22b5c 100644 --- a/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue +++ b/app/assets/javascripts/diffs/components/parallel_diff_comment_row.vue @@ -21,51 +21,47 @@ export default { type: Number, required: true, }, - leftDiscussions: { - type: Array, - required: false, - default: () => [], - }, - rightDiscussions: { - type: Array, - required: false, - default: () => [], - }, }, computed: { ...mapState({ diffLineCommentForms: state => state.diffs.diffLineCommentForms, }), leftLineCode() { - return this.line.left.lineCode; + return this.line.left && this.line.left.lineCode; }, rightLineCode() { - return this.line.right.lineCode; + return this.line.right && this.line.right.lineCode; }, hasExpandedDiscussionOnLeft() { - const discussions = this.leftDiscussions; - - return discussions ? discussions.every(discussion => discussion.expanded) : false; + return this.line.left && this.line.left.discussions + ? this.line.left.discussions.every(discussion => discussion.expanded) + : false; }, hasExpandedDiscussionOnRight() { - const discussions = this.rightDiscussions; - - return discussions ? discussions.every(discussion => discussion.expanded) : false; + return this.line.right && this.line.right.discussions + ? this.line.right.discussions.every(discussion => discussion.expanded) + : false; }, hasAnyExpandedDiscussion() { return this.hasExpandedDiscussionOnLeft || this.hasExpandedDiscussionOnRight; }, shouldRenderDiscussionsOnLeft() { - return this.leftDiscussions && this.hasExpandedDiscussionOnLeft; + return this.line.left && this.line.left.discussions && this.hasExpandedDiscussionOnLeft; }, shouldRenderDiscussionsOnRight() { - return this.rightDiscussions && this.hasExpandedDiscussionOnRight && this.line.right.type; + return ( + this.line.right && + this.line.right.discussions && + this.hasExpandedDiscussionOnRight && + this.line.right.type + ); }, showRightSideCommentForm() { return this.line.right.type && this.diffLineCommentForms[this.rightLineCode]; }, className() { - return this.leftDiscussions.length > 0 || this.rightDiscussions.length > 0 + return (this.left && this.line.left.discussions.length > 0) || + (this.right && this.line.right.discussions.length > 0) ? '' : 'js-temp-notes-holder'; }, @@ -85,8 +81,8 @@ export default { class="content" > - + /> diff --git a/app/assets/javascripts/diffs/store/actions.js b/app/assets/javascripts/diffs/store/actions.js index 4ab6ceb249a..6571484fa46 100644 --- a/app/assets/javascripts/diffs/store/actions.js +++ b/app/assets/javascripts/diffs/store/actions.js @@ -29,25 +29,64 @@ export const fetchDiffFiles = ({ state, commit }) => { .then(handleLocationHash); }; +export const assignDiscussionsToDiff = ({ state, commit }, allLineDiscussions) => { + console.log('DIFF : ', state.diffFiles); + console.log('STATE : ', allLineDiscussions); + + Object.values(allLineDiscussions).forEach(discussions => { + if (discussions.length > 0) { + console.log('KE : ', discussions); + const fileHash = discussions[0].fileHash; + const selectedFile = state.diffFiles.find(file => file.fileHash === fileHash); + console.log('FILE : ', selectedFile); + if (selectedFile) { + const targetLine = selectedFile.parallelDiffLines.find(line => { + return ( + (line.left && line.left.lineCode === discussions[0].line_code) || + (line.right && line.right.lineCode === discussions[0].line_code) + ); + }); + + if (targetLine) { + console.log('TARGET LINE : ', targetLine); + Object.assign(targetLine.right, { discussions }); + } + } + } + }); +}; + export const startRenderDiffsQueue = ({ state, commit }) => { const checkItem = () => { - const nextFile = state.diffFiles.find( - file => !file.renderIt && (!file.collapsed || !file.text), - ); - if (nextFile) { - requestAnimationFrame(() => { - commit(types.RENDER_FILE, nextFile); - }); - requestIdleCallback( - () => { - checkItem(); - }, - { timeout: 1000 }, + return new Promise(resolve => { + const nextFile = state.diffFiles.find( + file => !file.renderIt && (!file.collapsed || !file.text), ); - } + + if (nextFile) { + requestAnimationFrame(() => { + commit(types.RENDER_FILE, nextFile); + }); + requestIdleCallback( + () => { + checkItem() + .then(resolve) + .catch(() => {}); + }, + { timeout: 1000 }, + ); + } else { + console.log('No more items found -> Done'); + resolve(); + } + }); }; - checkItem(); + return new Promise(resolve => { + checkItem() + .then(resolve) + .catch(() => {}); + }); }; export const setInlineDiffViewType = ({ commit }) => { diff --git a/app/assets/javascripts/diffs/store/getters.js b/app/assets/javascripts/diffs/store/getters.js index 8dfa7c5d9da..f56b0ac695e 100644 --- a/app/assets/javascripts/diffs/store/getters.js +++ b/app/assets/javascripts/diffs/store/getters.js @@ -75,26 +75,63 @@ export const singleDiscussionByLineCodeOld = ( return discussions[lineCode] || []; }; -export const shouldRenderParallelCommentRowOld = (state, getters) => line => { - const leftLineCode = line.left.lineCode; - const rightLineCode = line.right.lineCode; - const leftDiscussions = getters.singleDiscussionByLineCode(leftLineCode); - const rightDiscussions = getters.singleDiscussionByLineCode(rightLineCode); - const hasDiscussion = leftDiscussions.length || rightDiscussions.length; - - const hasExpandedDiscussionOnLeft = leftDiscussions.length - ? leftDiscussions.every(discussion => discussion.expanded) - : false; - const hasExpandedDiscussionOnRight = rightDiscussions.length - ? rightDiscussions.every(discussion => discussion.expanded) - : false; +/** + * Returns an Object with discussions by their diff line code + * To avoid rendering outdated discussions on the Changes tab we should do a bunch of SHA + * comparisions. `note.position.formatter` have the current version diff refs but + * `note.original_position.formatter` will have the first version's diff refs. + * If line diff refs matches with one of them, we should render it as a discussion on Changes tab. + * + * @param {Object} diff + * @returns {Array} + */ +export const discussionsByLineCode = (state, getters, rootState, rootGetters) => { + const diffRefsByLineCode = getDiffRefsByLineCode(state.diffFiles); + + return rootGetters.discussions.reduce((acc, note) => { + const isDiffDiscussion = note.diff_discussion; + const hasLineCode = note.line_code; + const isResolvable = note.resolvable; + const diffRefs = diffRefsByLineCode[note.line_code]; + + if (isDiffDiscussion && hasLineCode && isResolvable && diffRefs) { + const refs = convertObjectPropsToCamelCase(note.position.formatter); + const originalRefs = convertObjectPropsToCamelCase(note.original_position.formatter); + + if (_.isEqual(refs, diffRefs) || _.isEqual(originalRefs, diffRefs)) { + const lineCode = note.line_code; + + if (acc[lineCode]) { + acc[lineCode].push(note); + } else { + acc[lineCode] = [note]; + } + } + } + + return acc; + }, {}); +}; + +export const shouldRenderParallelCommentRow = (state, getters) => line => { + const hasDiscussion = + (line.left && line.left.discussions.length) || (line.right && line.right.discussions.length); + + const hasExpandedDiscussionOnLeft = + line.left && line.left.discussions.length + ? line.left.discussions.every(discussion => discussion.expanded) + : false; + const hasExpandedDiscussionOnRight = + line.right && line.right.discussions.length + ? line.right.discussions.every(discussion => discussion.expanded) + : false; if (hasDiscussion && (hasExpandedDiscussionOnLeft || hasExpandedDiscussionOnRight)) { return true; } - const hasCommentFormOnLeft = state.diffLineCommentForms[leftLineCode]; - const hasCommentFormOnRight = state.diffLineCommentForms[rightLineCode]; + const hasCommentFormOnLeft = line.left && state.diffLineCommentForms[line.left.lineCode]; + const hasCommentFormOnRight = line.right && state.diffLineCommentForms[line.right.lineCode]; return hasCommentFormOnLeft || hasCommentFormOnRight; }; diff --git a/app/assets/javascripts/diffs/store/utils.js b/app/assets/javascripts/diffs/store/utils.js index 2d474217d83..31a5c8bbbec 100644 --- a/app/assets/javascripts/diffs/store/utils.js +++ b/app/assets/javascripts/diffs/store/utils.js @@ -162,6 +162,7 @@ export function addContextLines(options) { */ export function trimFirstCharOfLineContent(line = {}) { delete line.text; + line.discussions = []; const parsedLine = Object.assign({}, line); diff --git a/app/assets/javascripts/notes/stores/getters.js b/app/assets/javascripts/notes/stores/getters.js index 5b3b9f8776f..6fb4fdf74da 100644 --- a/app/assets/javascripts/notes/stores/getters.js +++ b/app/assets/javascripts/notes/stores/getters.js @@ -28,11 +28,16 @@ export const notesById = state => return acc; }, {}); -export const discussionsByLineCode = state => +export const discussionsStructuredByLineCode = state => state.discussions.reduce((acc, note) => { if (note.diff_discussion && note.line_code && note.resolvable) { // For context about line notes: there might be multiple notes with the same line code const items = acc[note.line_code] || []; + if (note.diff_file) { + Object.assign(note, { fileHash: note.diff_file.file_hash }); + // delete note.diff_file; + } + items.push(note); Object.assign(acc, { [note.line_code]: items }); -- cgit v1.2.1