Commit ed88f425 authored by Martin Wortschack's avatar Martin Wortschack

Merge branch...

Merge branch '33183-request-inline-or-parallel-diffs-instead-fetching-all-data-from-backend-in-one-request' into 'master'

MR Diffs | Load Diff Types Lazily

Closes #39494

See merge request gitlab-org/gitlab!19930
parents 13595eb0 760c0ec0
...@@ -6,6 +6,7 @@ import { __ } from '~/locale'; ...@@ -6,6 +6,7 @@ import { __ } from '~/locale';
import createFlash from '~/flash'; import createFlash from '~/flash';
import PanelResizer from '~/vue_shared/components/panel_resizer.vue'; import PanelResizer from '~/vue_shared/components/panel_resizer.vue';
import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin';
import { isSingleViewStyle } from '~/helpers/diffs_helper';
import eventHub from '../../notes/event_hub'; import eventHub from '../../notes/event_hub';
import CompareVersions from './compare_versions.vue'; import CompareVersions from './compare_versions.vue';
import DiffFile from './diff_file.vue'; import DiffFile from './diff_file.vue';
...@@ -145,6 +146,9 @@ export default { ...@@ -145,6 +146,9 @@ export default {
}, },
watch: { watch: {
diffViewType() { diffViewType() {
if (this.needsReload() || this.needsFirstLoad()) {
this.refetchDiffData();
}
this.adjustView(); this.adjustView();
}, },
shouldShow() { shouldShow() {
...@@ -224,6 +228,16 @@ export default { ...@@ -224,6 +228,16 @@ export default {
{ timeout: 1000 }, { timeout: 1000 },
); );
}, },
needsReload() {
return (
this.glFeatures.singleMrDiffView &&
this.diffFiles.length &&
isSingleViewStyle(this.diffFiles[0])
);
},
needsFirstLoad() {
return this.glFeatures.singleMrDiffView && !this.diffFiles.length;
},
fetchData(toggleTree = true) { fetchData(toggleTree = true) {
if (this.glFeatures.diffsBatchLoad) { if (this.glFeatures.diffsBatchLoad) {
this.fetchDiffFilesMeta() this.fetchDiffFilesMeta()
...@@ -237,6 +251,13 @@ export default { ...@@ -237,6 +251,13 @@ export default {
}); });
this.fetchDiffFilesBatch() this.fetchDiffFilesBatch()
.then(() => {
// Guarantee the discussions are assigned after the batch finishes.
// Just watching the length of the discussions or the diff files
// isn't enough, because with split diff loading, neither will
// change when loading the other half of the diff files.
this.setDiscussions();
})
.then(() => this.startDiffRendering()) .then(() => this.startDiffRendering())
.catch(() => { .catch(() => {
createFlash(__('Something went wrong on our end. Please try again!')); createFlash(__('Something went wrong on our end. Please try again!'));
...@@ -250,6 +271,7 @@ export default { ...@@ -250,6 +271,7 @@ export default {
requestIdleCallback( requestIdleCallback(
() => { () => {
this.setDiscussions();
this.startRenderDiffsQueue(); this.startRenderDiffsQueue();
}, },
{ timeout: 1000 }, { timeout: 1000 },
......
...@@ -4,6 +4,7 @@ import _ from 'underscore'; ...@@ -4,6 +4,7 @@ import _ from 'underscore';
import { GlLoadingIcon } from '@gitlab/ui'; import { GlLoadingIcon } from '@gitlab/ui';
import { __, sprintf } from '~/locale'; import { __, sprintf } from '~/locale';
import createFlash from '~/flash'; import createFlash from '~/flash';
import { hasDiff } from '~/helpers/diffs_helper';
import eventHub from '../../notes/event_hub'; import eventHub from '../../notes/event_hub';
import DiffFileHeader from './diff_file_header.vue'; import DiffFileHeader from './diff_file_header.vue';
import DiffContent from './diff_content.vue'; import DiffContent from './diff_content.vue';
...@@ -55,12 +56,7 @@ export default { ...@@ -55,12 +56,7 @@ export default {
return this.isLoadingCollapsedDiff || (!this.file.renderIt && !this.isCollapsed); return this.isLoadingCollapsedDiff || (!this.file.renderIt && !this.isCollapsed);
}, },
hasDiff() { hasDiff() {
return ( return hasDiff(this.file);
(this.file.highlighted_diff_lines &&
this.file.parallel_diff_lines &&
this.file.parallel_diff_lines.length > 0) ||
!this.file.blob.readable_text
);
}, },
isFileTooLarge() { isFileTooLarge() {
return this.file.viewer.error === diffViewerErrors.too_large; return this.file.viewer.error === diffViewerErrors.too_large;
......
...@@ -65,6 +65,10 @@ export const fetchDiffFiles = ({ state, commit }) => { ...@@ -65,6 +65,10 @@ export const fetchDiffFiles = ({ state, commit }) => {
w: state.showWhitespace ? '0' : '1', w: state.showWhitespace ? '0' : '1',
}; };
if (state.useSingleDiffStyle) {
urlParams.view = state.diffViewType;
}
commit(types.SET_LOADING, true); commit(types.SET_LOADING, true);
worker.addEventListener('message', ({ data }) => { worker.addEventListener('message', ({ data }) => {
...@@ -90,13 +94,22 @@ export const fetchDiffFiles = ({ state, commit }) => { ...@@ -90,13 +94,22 @@ export const fetchDiffFiles = ({ state, commit }) => {
}; };
export const fetchDiffFilesBatch = ({ commit, state }) => { export const fetchDiffFilesBatch = ({ commit, state }) => {
const urlParams = {
per_page: DIFFS_PER_PAGE,
w: state.showWhitespace ? '0' : '1',
};
if (state.useSingleDiffStyle) {
urlParams.view = state.diffViewType;
}
commit(types.SET_BATCH_LOADING, true); commit(types.SET_BATCH_LOADING, true);
commit(types.SET_RETRIEVING_BATCHES, true); commit(types.SET_RETRIEVING_BATCHES, true);
const getBatch = page => const getBatch = page =>
axios axios
.get(state.endpointBatch, { .get(state.endpointBatch, {
params: { page, per_page: DIFFS_PER_PAGE, w: state.showWhitespace ? '0' : '1' }, params: { ...urlParams, page },
}) })
.then(({ data: { pagination, diff_files } }) => { .then(({ data: { pagination, diff_files } }) => {
commit(types.SET_DIFF_DATA_BATCH, { diff_files }); commit(types.SET_DIFF_DATA_BATCH, { diff_files });
...@@ -150,7 +163,10 @@ export const assignDiscussionsToDiff = ( ...@@ -150,7 +163,10 @@ export const assignDiscussionsToDiff = (
{ commit, state, rootState }, { commit, state, rootState },
discussions = rootState.notes.discussions, discussions = rootState.notes.discussions,
) => { ) => {
const diffPositionByLineCode = getDiffPositionByLineCode(state.diffFiles); const diffPositionByLineCode = getDiffPositionByLineCode(
state.diffFiles,
state.useSingleDiffStyle,
);
const hash = getLocationHash(); const hash = getLocationHash();
discussions discussions
...@@ -339,24 +355,23 @@ export const toggleFileDiscussions = ({ getters, dispatch }, diff) => { ...@@ -339,24 +355,23 @@ export const toggleFileDiscussions = ({ getters, dispatch }, diff) => {
export const toggleFileDiscussionWrappers = ({ commit }, diff) => { export const toggleFileDiscussionWrappers = ({ commit }, diff) => {
const discussionWrappersExpanded = allDiscussionWrappersExpanded(diff); const discussionWrappersExpanded = allDiscussionWrappersExpanded(diff);
let linesWithDiscussions; const lineCodesWithDiscussions = new Set();
if (diff.highlighted_diff_lines) { const { parallel_diff_lines: parallelLines, highlighted_diff_lines: inlineLines } = diff;
linesWithDiscussions = diff.highlighted_diff_lines.filter(line => line.discussions.length); const allLines = inlineLines.concat(
} parallelLines.map(line => line.left),
if (diff.parallel_diff_lines) { parallelLines.map(line => line.right),
linesWithDiscussions = diff.parallel_diff_lines.filter(
line =>
(line.left && line.left.discussions.length) ||
(line.right && line.right.discussions.length),
); );
} const lineHasDiscussion = line => Boolean(line?.discussions.length);
const registerDiscussionLine = line => lineCodesWithDiscussions.add(line.line_code);
allLines.filter(lineHasDiscussion).forEach(registerDiscussionLine);
if (linesWithDiscussions.length) { if (lineCodesWithDiscussions.size) {
linesWithDiscussions.forEach(line => { Array.from(lineCodesWithDiscussions).forEach(lineCode => {
commit(types.TOGGLE_LINE_DISCUSSIONS, { commit(types.TOGGLE_LINE_DISCUSSIONS, {
fileHash: diff.file_hash, fileHash: diff.file_hash,
lineCode: line.line_code,
expanded: !discussionWrappersExpanded, expanded: !discussionWrappersExpanded,
lineCode,
}); });
}); });
} }
......
...@@ -45,26 +45,28 @@ export default { ...@@ -45,26 +45,28 @@ export default {
}, },
[types.SET_DIFF_DATA](state, data) { [types.SET_DIFF_DATA](state, data) {
let files = state.diffFiles;
if ( if (
!( !(gon?.features?.diffsBatchLoad && window.location.search.indexOf('diff_id') === -1) &&
gon && data.diff_files
gon.features &&
gon.features.diffsBatchLoad &&
window.location.search.indexOf('diff_id') === -1
)
) { ) {
prepareDiffData(data); files = prepareDiffData(data, files);
} }
Object.assign(state, { Object.assign(state, {
...convertObjectPropsToCamelCase(data), ...convertObjectPropsToCamelCase(data),
diffFiles: files,
}); });
}, },
[types.SET_DIFF_DATA_BATCH](state, data) { [types.SET_DIFF_DATA_BATCH](state, data) {
prepareDiffData(data); const files = prepareDiffData(data, state.diffFiles);
state.diffFiles.push(...data.diff_files); Object.assign(state, {
...convertObjectPropsToCamelCase(data),
diffFiles: files,
});
}, },
[types.RENDER_FILE](state, file) { [types.RENDER_FILE](state, file) {
...@@ -88,11 +90,11 @@ export default { ...@@ -88,11 +90,11 @@ export default {
if (!diffFile) return; if (!diffFile) return;
if (diffFile.highlighted_diff_lines) { if (diffFile.highlighted_diff_lines.length) {
diffFile.highlighted_diff_lines.find(l => l.line_code === lineCode).hasForm = hasForm; diffFile.highlighted_diff_lines.find(l => l.line_code === lineCode).hasForm = hasForm;
} }
if (diffFile.parallel_diff_lines) { if (diffFile.parallel_diff_lines.length) {
const line = diffFile.parallel_diff_lines.find(l => { const line = diffFile.parallel_diff_lines.find(l => {
const { left, right } = l; const { left, right } = l;
...@@ -153,13 +155,13 @@ export default { ...@@ -153,13 +155,13 @@ export default {
}, },
[types.EXPAND_ALL_FILES](state) { [types.EXPAND_ALL_FILES](state) {
state.diffFiles = state.diffFiles.map(file => ({ state.diffFiles.forEach(file => {
...file, Object.assign(file, {
viewer: { viewer: Object.assign(file.viewer, {
...file.viewer,
collapsed: false, collapsed: false,
}, }),
})); });
});
}, },
[types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) { [types.SET_LINE_DISCUSSIONS_FOR_FILE](state, { discussion, diffPositionByLineCode, hash }) {
...@@ -197,29 +199,29 @@ export default { ...@@ -197,29 +199,29 @@ export default {
}; };
}; };
state.diffFiles = state.diffFiles.map(diffFile => { state.diffFiles.forEach(file => {
if (diffFile.file_hash === fileHash) { if (file.file_hash === fileHash) {
const file = { ...diffFile }; if (file.highlighted_diff_lines.length) {
file.highlighted_diff_lines.forEach(line => {
if (file.highlighted_diff_lines) { Object.assign(
file.highlighted_diff_lines = file.highlighted_diff_lines.map(line => line,
setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line), setDiscussionsExpanded(lineCheck(line) ? mapDiscussions(line) : line),
); );
});
} }
if (file.parallel_diff_lines) { if (file.parallel_diff_lines.length) {
file.parallel_diff_lines = file.parallel_diff_lines.map(line => { file.parallel_diff_lines.forEach(line => {
const left = line.left && lineCheck(line.left); const left = line.left && lineCheck(line.left);
const right = line.right && lineCheck(line.right); const right = line.right && lineCheck(line.right);
if (left || right) { if (left || right) {
return { Object.assign(line, {
...line,
left: line.left ? setDiscussionsExpanded(mapDiscussions(line.left)) : null, left: line.left ? setDiscussionsExpanded(mapDiscussions(line.left)) : null,
right: line.right right: line.right
? setDiscussionsExpanded(mapDiscussions(line.right, () => !left)) ? setDiscussionsExpanded(mapDiscussions(line.right, () => !left))
: null, : null,
}; });
} }
return line; return line;
...@@ -227,15 +229,15 @@ export default { ...@@ -227,15 +229,15 @@ export default {
} }
if (!file.parallel_diff_lines || !file.highlighted_diff_lines) { if (!file.parallel_diff_lines || !file.highlighted_diff_lines) {
file.discussions = (file.discussions || []) const newDiscussions = (file.discussions || [])
.filter(d => d.id !== discussion.id) .filter(d => d.id !== discussion.id)
.concat(discussion); .concat(discussion);
}
return file; Object.assign(file, {
discussions: newDiscussions,
});
}
} }
return diffFile;
}); });
}, },
...@@ -259,9 +261,9 @@ export default { ...@@ -259,9 +261,9 @@ export default {
[types.TOGGLE_LINE_DISCUSSIONS](state, { fileHash, lineCode, expanded }) { [types.TOGGLE_LINE_DISCUSSIONS](state, { fileHash, lineCode, expanded }) {
const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash); const selectedFile = state.diffFiles.find(f => f.file_hash === fileHash);
updateLineInFile(selectedFile, lineCode, line => updateLineInFile(selectedFile, lineCode, line => {
Object.assign(line, { discussionsExpanded: expanded }), Object.assign(line, { discussionsExpanded: expanded });
); });
}, },
[types.TOGGLE_FOLDER_OPEN](state, path) { [types.TOGGLE_FOLDER_OPEN](state, path) {
......
...@@ -185,6 +185,7 @@ export function addContextLines(options) { ...@@ -185,6 +185,7 @@ export function addContextLines(options) {
* Trims the first char of the `richText` property when it's either a space or a diff symbol. * Trims the first char of the `richText` property when it's either a space or a diff symbol.
* @param {Object} line * @param {Object} line
* @returns {Object} * @returns {Object}
* @deprecated
*/ */
export function trimFirstCharOfLineContent(line = {}) { export function trimFirstCharOfLineContent(line = {}) {
// eslint-disable-next-line no-param-reassign // eslint-disable-next-line no-param-reassign
...@@ -212,80 +213,172 @@ function getLineCode({ left, right }, index) { ...@@ -212,80 +213,172 @@ function getLineCode({ left, right }, index) {
return index; return index;
} }
// This prepares and optimizes the incoming diff data from the server function diffFileUniqueId(file) {
// by setting up incremental rendering and removing unneeded data return `${file.content_sha}-${file.file_hash}`;
export function prepareDiffData(diffData) { }
const filesLength = diffData.diff_files.length;
let showingLines = 0;
for (let i = 0; i < filesLength; i += 1) {
const file = diffData.diff_files[i];
if (file.parallel_diff_lines) { function combineDiffFilesWithPriorFiles(files, prior = []) {
const linesLength = file.parallel_diff_lines.length; files.forEach(file => {
for (let u = 0; u < linesLength; u += 1) { const id = diffFileUniqueId(file);
const line = file.parallel_diff_lines[u]; const oldMatch = prior.find(oldFile => diffFileUniqueId(oldFile) === id);
line.line_code = getLineCode(line, u); if (oldMatch) {
if (line.left) { const missingInline = !file.highlighted_diff_lines;
line.left = trimFirstCharOfLineContent(line.left); const missingParallel = !file.parallel_diff_lines;
line.left.discussions = [];
line.left.hasForm = false; if (missingInline) {
} Object.assign(file, {
if (line.right) { highlighted_diff_lines: oldMatch.highlighted_diff_lines,
line.right = trimFirstCharOfLineContent(line.right); });
line.right.discussions = [];
line.right.hasForm = false;
} }
if (missingParallel) {
Object.assign(file, {
parallel_diff_lines: oldMatch.parallel_diff_lines,
});
} }
} }
});
if (file.highlighted_diff_lines) { return files;
const linesLength = file.highlighted_diff_lines.length; }
for (let u = 0; u < linesLength; u += 1) {
const line = file.highlighted_diff_lines[u]; function ensureBasicDiffFileLines(file) {
Object.assign(line, { const missingInline = !file.highlighted_diff_lines;
...trimFirstCharOfLineContent(line), const missingParallel = !file.parallel_diff_lines;
Object.assign(file, {
highlighted_diff_lines: missingInline ? [] : file.highlighted_diff_lines,
parallel_diff_lines: missingParallel ? [] : file.parallel_diff_lines,
});
return file;
}
function cleanRichText(text) {
return text ? text.replace(/^[+ -]/, '') : undefined;
}
function prepareLine(line) {
return Object.assign(line, {
rich_text: cleanRichText(line.rich_text),
discussionsExpanded: true,
discussions: [], discussions: [],
hasForm: false, hasForm: false,
text: undefined,
}); });
}
function prepareDiffFileLines(file) {
const inlineLines = file.highlighted_diff_lines;
const parallelLines = file.parallel_diff_lines;
let parallelLinesCount = 0;
inlineLines.forEach(prepareLine);
parallelLines.forEach((line, index) => {
Object.assign(line, { line_code: getLineCode(line, index) });
if (line.left) {
parallelLinesCount += 1;
prepareLine(line.left);
} }
showingLines += file.parallel_diff_lines.length;
if (line.right) {
parallelLinesCount += 1;
prepareLine(line.right);
} }
Object.assign(file, {
inlineLinesCount: inlineLines.length,
parallelLinesCount,
});
});
return file;
}
function getVisibleDiffLines(file) {
return Math.max(file.inlineLinesCount, file.parallelLinesCount);
}
function finalizeDiffFile(file) {
const name = (file.viewer && file.viewer.name) || diffViewerModes.text; const name = (file.viewer && file.viewer.name) || diffViewerModes.text;
const lines = getVisibleDiffLines(file);
Object.assign(file, { Object.assign(file, {
renderIt: showingLines < LINES_TO_BE_RENDERED_DIRECTLY, renderIt: lines < LINES_TO_BE_RENDERED_DIRECTLY,
collapsed: name === diffViewerModes.text && showingLines > MAX_LINES_TO_BE_RENDERED, collapsed: name === diffViewerModes.text && lines > MAX_LINES_TO_BE_RENDERED,
isShowingFullFile: false, isShowingFullFile: false,
isLoadingFullFile: false, isLoadingFullFile: false,
discussions: [], discussions: [],
renderingLines: false, renderingLines: false,
}); });
}
return file;
} }
export function getDiffPositionByLineCode(diffFiles) { export function prepareDiffData(diffData, priorFiles) {
return diffFiles.reduce((acc, diffFile) => { return combineDiffFilesWithPriorFiles(diffData.diff_files, priorFiles)
// We can only use highlightedDiffLines to create the map of diff lines because .map(ensureBasicDiffFileLines)
// highlightedDiffLines will also include every parallel diff line in it. .map(prepareDiffFileLines)
if (diffFile.highlighted_diff_lines) { .map(finalizeDiffFile);
}
export function getDiffPositionByLineCode(diffFiles, useSingleDiffStyle) {
let lines = [];
const hasInlineDiffs = diffFiles.some(file => file.highlighted_diff_lines.length > 0);
if (!useSingleDiffStyle || hasInlineDiffs) {
// In either of these cases, we can use `highlighted_diff_lines` because
// that will include all of the parallel diff lines, too
lines = diffFiles.reduce((acc, diffFile) => {
diffFile.highlighted_diff_lines.forEach(line => { diffFile.highlighted_diff_lines.forEach(line => {
acc.push({ file: diffFile, line });
});
return acc;
}, []);
} else {
// If we're in single diff view mode and the inline lines haven't been
// loaded yet, we need to parse the parallel lines
lines = diffFiles.reduce((acc, diffFile) => {
diffFile.parallel_diff_lines.forEach(pair => {
// It's possible for a parallel line to have an opposite line that doesn't exist
// For example: *deleted* lines will have `null` right lines, while
// *added* lines will have `null` left lines.
// So we have to check each line before we push it onto the array so we're not
// pushing null line diffs
if (pair.left) {
acc.push({ file: diffFile, line: pair.left });
}
if (pair.right) {
acc.push({ file: diffFile, line: pair.right });
}
});
return acc;
}, []);
}
return lines.reduce((acc, { file, line }) => {
if (line.line_code) { if (line.line_code) {
acc[line.line_code] = { acc[line.line_code] = {
base_sha: diffFile.diff_refs.base_sha, base_sha: file.diff_refs.base_sha,
head_sha: diffFile.diff_refs.head_sha, head_sha: file.diff_refs.head_sha,
start_sha: diffFile.diff_refs.start_sha, start_sha: file.diff_refs.start_sha,
new_path: diffFile.new_path, new_path: file.new_path,
old_path: diffFile.old_path, old_path: file.old_path,
old_line: line.old_line, old_line: line.old_line,
new_line: line.new_line, new_line: line.new_line,
line_code: line.line_code, line_code: line.line_code,
position_type: 'text', position_type: 'text',
}; };
} }
});
}
return acc; return acc;
}, {}); }, {});
...@@ -462,47 +555,47 @@ export const convertExpandLines = ({ ...@@ -462,47 +555,47 @@ export const convertExpandLines = ({
export const idleCallback = cb => requestIdleCallback(cb); export const idleCallback = cb => requestIdleCallback(cb);
export const updateLineInFile = (selectedFile, lineCode, updateFn) => { function getLinesFromFileByLineCode(file, lineCode) {
if (selectedFile.parallel_diff_lines) { const parallelLines = file.parallel_diff_lines;
const targetLine = selectedFile.parallel_diff_lines.find( const inlineLines = file.highlighted_diff_lines;
line => const matchesCode = line => line.line_code === lineCode;
(line.left && line.left.line_code === lineCode) ||
(line.right && line.right.line_code === lineCode),
);
if (targetLine) {
const side = targetLine.left && targetLine.left.line_code === lineCode ? 'left' : 'right';
updateFn(targetLine[side]); return [
} ...parallelLines.reduce((acc, line) => {
if (line.left) {
acc.push(line.left);
} }
if (selectedFile.highlighted_diff_lines) {
const targetInlineLine = selectedFile.highlighted_diff_lines.find(
line => line.line_code === lineCode,
);
if (targetInlineLine) { if (line.right) {
updateFn(targetInlineLine); acc.push(line.right);
}
} }
return acc;
}, []),
...inlineLines,
].filter(matchesCode);
}
export const updateLineInFile = (selectedFile, lineCode, updateFn) => {
getLinesFromFileByLineCode(selectedFile, lineCode).forEach(updateFn);
}; };
export const allDiscussionWrappersExpanded = diff => { export const allDiscussionWrappersExpanded = diff => {
const discussionsExpandedArray = []; let discussionsExpanded = true;
if (diff.parallel_diff_lines) { const changeExpandedResult = line => {
diff.parallel_diff_lines.forEach(line => { if (line && line.discussions.length) {
if (line.left && line.left.discussions.length) { discussionsExpanded = discussionsExpanded && line.discussionsExpanded;
discussionsExpandedArray.push(line.left.discussionsExpanded);
}
if (line.right && line.right.discussions.length) {
discussionsExpandedArray.push(line.right.discussionsExpanded);
} }
};
diff.parallel_diff_lines.forEach(line => {
changeExpandedResult(line.left);
changeExpandedResult(line.right);
}); });
} else if (diff.highlighted_diff_lines) {
diff.highlighted_diff_lines.forEach(line => { diff.highlighted_diff_lines.forEach(line => {
if (line.discussions.length) { changeExpandedResult(line);
discussionsExpandedArray.push(line.discussionsExpanded);
}
}); });
}
return discussionsExpandedArray.every(el => el); return discussionsExpanded;
}; };
export function hasInlineLines(diffFile) {
return diffFile?.highlighted_diff_lines?.length > 0; /* eslint-disable-line camelcase */
}
export function hasParallelLines(diffFile) {
return diffFile?.parallel_diff_lines?.length > 0; /* eslint-disable-line camelcase */
}
export function isSingleViewStyle(diffFile) {
return !hasParallelLines(diffFile) || !hasInlineLines(diffFile);
}
export function hasDiff(diffFile) {
return (
hasInlineLines(diffFile) ||
hasParallelLines(diffFile) ||
!diffFile?.blob?.readable_text /* eslint-disable-line camelcase */
);
}
---
title: Load MR diff types lazily to reduce initial diff payload size
merge_request: 19930
author:
type: added
...@@ -31,11 +31,8 @@ describe 'Merge request > Batch comments', :js do ...@@ -31,11 +31,8 @@ describe 'Merge request > Batch comments', :js do
end end
end end
it_behaves_like 'rendering a single diff version'
context 'Feature is enabled' do context 'Feature is enabled' do
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
stub_licensed_features(batch_comments: true) stub_licensed_features(batch_comments: true)
......
...@@ -11,7 +11,6 @@ describe 'Merge request > image review', :js do ...@@ -11,7 +11,6 @@ describe 'Merge request > image review', :js do
let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) } let(:merge_request) { create(:merge_request_with_diffs, :with_image_diffs, source_project: project, author: user) }
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_licensed_features(batch_comments: true) stub_licensed_features(batch_comments: true)
sign_in(user) sign_in(user)
...@@ -24,8 +23,6 @@ describe 'Merge request > image review', :js do ...@@ -24,8 +23,6 @@ describe 'Merge request > image review', :js do
wait_for_requests wait_for_requests
end end
it_behaves_like 'rendering a single diff version'
it 'leaves review' do it 'leaves review' do
find('.js-add-image-diff-note-button', match: :first).click find('.js-add-image-diff-note-button', match: :first).click
......
...@@ -32,8 +32,6 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js ...@@ -32,8 +32,6 @@ describe 'a maintainer edits files on a source-branch of an MR from a fork', :js
wait_for_requests wait_for_requests
end end
it_behaves_like 'rendering a single diff version'
it 'mentions commits will go to the source branch' do it 'mentions commits will go to the source branch' do
expect(page).to have_content('Your changes can be committed to fix because a merge request is open.') expect(page).to have_content('Your changes can be committed to fix because a merge request is open.')
end end
......
...@@ -13,15 +13,12 @@ describe 'User comments on a diff', :js do ...@@ -13,15 +13,12 @@ describe 'User comments on a diff', :js do
let(:user) { create(:user) } let(:user) { create(:user) }
before do before do
stub_feature_flags(single_mr_diff_view: false)
project.add_maintainer(user) project.add_maintainer(user)
sign_in(user) sign_in(user)
visit(diffs_project_merge_request_path(project, merge_request)) visit(diffs_project_merge_request_path(project, merge_request))
end end
it_behaves_like 'rendering a single diff version'
context 'when viewing comments' do context 'when viewing comments' do
context 'when toggling inline comments' do context 'when toggling inline comments' do
context 'in a single file' do context 'in a single file' do
......
...@@ -9,7 +9,6 @@ describe 'Merge request > User creates image diff notes', :js do ...@@ -9,7 +9,6 @@ describe 'Merge request > User creates image diff notes', :js do
let(:user) { project.creator } let(:user) { project.creator }
before do before do
stub_feature_flags(single_mr_diff_view: false)
sign_in(user) sign_in(user)
# Stub helper to return any blob file as image from public app folder. # Stub helper to return any blob file as image from public app folder.
...@@ -18,8 +17,6 @@ describe 'Merge request > User creates image diff notes', :js do ...@@ -18,8 +17,6 @@ describe 'Merge request > User creates image diff notes', :js do
allow_any_instance_of(DiffHelper).to receive(:diff_file_old_blob_raw_url).and_return('/favicon.png') allow_any_instance_of(DiffHelper).to receive(:diff_file_old_blob_raw_url).and_return('/favicon.png')
end end
it_behaves_like 'rendering a single diff version'
context 'create commit diff notes' do context 'create commit diff notes' do
commit_id = '2f63565e7aac07bcdadb654e253078b727143ec4' commit_id = '2f63565e7aac07bcdadb654e253078b727143ec4'
......
...@@ -7,7 +7,6 @@ describe 'User expands diff', :js do ...@@ -7,7 +7,6 @@ describe 'User expands diff', :js do
let(:merge_request) { create(:merge_request, source_branch: 'expand-collapse-files', source_project: project, target_project: project) } let(:merge_request) { create(:merge_request, source_branch: 'expand-collapse-files', source_project: project, target_project: project) }
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(100.kilobytes) allow(Gitlab::Git::Diff).to receive(:size_limit).and_return(100.kilobytes)
...@@ -18,8 +17,6 @@ describe 'User expands diff', :js do ...@@ -18,8 +17,6 @@ describe 'User expands diff', :js do
wait_for_requests wait_for_requests
end end
it_behaves_like 'rendering a single diff version'
it 'allows user to expand diff' do it 'allows user to expand diff' do
page.within find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9"]') do page.within find('[id="6eb14e00385d2fb284765eb1cd8d420d33d63fc9"]') do
click_link 'Click to expand it.' click_link 'Click to expand it.'
......
...@@ -14,15 +14,12 @@ describe 'Merge request > User posts diff notes', :js do ...@@ -14,15 +14,12 @@ describe 'Merge request > User posts diff notes', :js do
let(:test_note_comment) { 'this is a test note!' } let(:test_note_comment) { 'this is a test note!' }
before do before do
stub_feature_flags(single_mr_diff_view: false)
set_cookie('sidebar_collapsed', 'true') set_cookie('sidebar_collapsed', 'true')
project.add_developer(user) project.add_developer(user)
sign_in(user) sign_in(user)
end end
it_behaves_like 'rendering a single diff version'
context 'when hovering over a parallel view diff file' do context 'when hovering over a parallel view diff file' do
before do before do
visit diffs_project_merge_request_path(project, merge_request, view: 'parallel') visit diffs_project_merge_request_path(project, merge_request, view: 'parallel')
......
...@@ -9,7 +9,6 @@ describe 'Merge request > User resolves conflicts', :js do ...@@ -9,7 +9,6 @@ describe 'Merge request > User resolves conflicts', :js do
before do before do
# In order to have the diffs collapsed, we need to disable the increase feature # In order to have the diffs collapsed, we need to disable the increase feature
stub_feature_flags(gitlab_git_diff_size_limit_increase: false) stub_feature_flags(gitlab_git_diff_size_limit_increase: false)
stub_feature_flags(single_mr_diff_view: false)
end end
def create_merge_request(source_branch) def create_merge_request(source_branch)
...@@ -18,8 +17,6 @@ describe 'Merge request > User resolves conflicts', :js do ...@@ -18,8 +17,6 @@ describe 'Merge request > User resolves conflicts', :js do
end end
end end
it_behaves_like 'rendering a single diff version'
shared_examples 'conflicts are resolved in Interactive mode' do shared_examples 'conflicts are resolved in Interactive mode' do
it 'conflicts are resolved in Interactive mode' do it 'conflicts are resolved in Interactive mode' do
within find('.files-wrapper .diff-file', text: 'files/ruby/popen.rb') do within find('.files-wrapper .diff-file', text: 'files/ruby/popen.rb') do
......
...@@ -20,12 +20,9 @@ describe 'Merge request > User resolves diff notes and threads', :js do ...@@ -20,12 +20,9 @@ describe 'Merge request > User resolves diff notes and threads', :js do
end end
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
end end
it_behaves_like 'rendering a single diff version'
context 'no threads' do context 'no threads' do
before do before do
project.add_maintainer(user) project.add_maintainer(user)
......
...@@ -21,7 +21,6 @@ describe 'Merge request > User sees avatars on diff notes', :js do ...@@ -21,7 +21,6 @@ describe 'Merge request > User sees avatars on diff notes', :js do
let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) } let!(:note) { create(:diff_note_on_merge_request, project: project, noteable: merge_request, position: position) }
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
project.add_maintainer(user) project.add_maintainer(user)
sign_in user sign_in user
...@@ -29,8 +28,6 @@ describe 'Merge request > User sees avatars on diff notes', :js do ...@@ -29,8 +28,6 @@ describe 'Merge request > User sees avatars on diff notes', :js do
set_cookie('sidebar_collapsed', 'true') set_cookie('sidebar_collapsed', 'true')
end end
it_behaves_like 'rendering a single diff version'
context 'discussion tab' do context 'discussion tab' do
before do before do
visit project_merge_request_path(project, merge_request) visit project_merge_request_path(project, merge_request)
......
...@@ -10,12 +10,9 @@ describe 'Merge request > User sees diff', :js do ...@@ -10,12 +10,9 @@ describe 'Merge request > User sees diff', :js do
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
end end
it_behaves_like 'rendering a single diff version'
context 'when linking to note' do context 'when linking to note' do
describe 'with unresolved note' do describe 'with unresolved note' do
let(:note) { create :diff_note_on_merge_request, project: project, noteable: merge_request } let(:note) { create :diff_note_on_merge_request, project: project, noteable: merge_request }
......
...@@ -11,14 +11,11 @@ describe 'Merge request > User sees MR with deleted source branch', :js do ...@@ -11,14 +11,11 @@ describe 'Merge request > User sees MR with deleted source branch', :js do
let(:user) { project.creator } let(:user) { project.creator }
before do before do
stub_feature_flags(single_mr_diff_view: false)
merge_request.update!(source_branch: 'this-branch-does-not-exist') merge_request.update!(source_branch: 'this-branch-does-not-exist')
sign_in(user) sign_in(user)
visit project_merge_request_path(project, merge_request) visit project_merge_request_path(project, merge_request)
end end
it_behaves_like 'rendering a single diff version'
it 'shows a message about missing source branch' do it 'shows a message about missing source branch' do
expect(page).to have_content('Source branch does not exist.') expect(page).to have_content('Source branch does not exist.')
end end
......
...@@ -16,7 +16,6 @@ describe 'Merge request > User sees versions', :js do ...@@ -16,7 +16,6 @@ describe 'Merge request > User sees versions', :js do
let!(:params) { {} } let!(:params) { {} }
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
project.add_maintainer(user) project.add_maintainer(user)
...@@ -24,8 +23,6 @@ describe 'Merge request > User sees versions', :js do ...@@ -24,8 +23,6 @@ describe 'Merge request > User sees versions', :js do
visit diffs_project_merge_request_path(project, merge_request, params) visit diffs_project_merge_request_path(project, merge_request, params)
end end
it_behaves_like 'rendering a single diff version'
shared_examples 'allows commenting' do |file_id:, line_code:, comment:| shared_examples 'allows commenting' do |file_id:, line_code:, comment:|
it do it do
diff_file_selector = ".diff-file[id='#{file_id}']" diff_file_selector = ".diff-file[id='#{file_id}']"
......
...@@ -25,15 +25,12 @@ describe 'User comments on a diff', :js do ...@@ -25,15 +25,12 @@ describe 'User comments on a diff', :js do
let(:user) { create(:user) } let(:user) { create(:user) }
before do before do
stub_feature_flags(single_mr_diff_view: false)
project.add_maintainer(user) project.add_maintainer(user)
sign_in(user) sign_in(user)
visit(diffs_project_merge_request_path(project, merge_request)) visit(diffs_project_merge_request_path(project, merge_request))
end end
it_behaves_like 'rendering a single diff version'
context 'single suggestion note' do context 'single suggestion note' do
it 'hides suggestion popover' do it 'hides suggestion popover' do
click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']")) click_diff_line(find("[id='#{sample_compare.changes[1][:line_code]}']"))
......
...@@ -8,7 +8,6 @@ describe 'Merge request > User toggles whitespace changes', :js do ...@@ -8,7 +8,6 @@ describe 'Merge request > User toggles whitespace changes', :js do
let(:user) { project.creator } let(:user) { project.creator }
before do before do
stub_feature_flags(single_mr_diff_view: false)
project.add_maintainer(user) project.add_maintainer(user)
sign_in(user) sign_in(user)
visit diffs_project_merge_request_path(project, merge_request) visit diffs_project_merge_request_path(project, merge_request)
...@@ -16,8 +15,6 @@ describe 'Merge request > User toggles whitespace changes', :js do ...@@ -16,8 +15,6 @@ describe 'Merge request > User toggles whitespace changes', :js do
find('.js-show-diff-settings').click find('.js-show-diff-settings').click
end end
it_behaves_like 'rendering a single diff version'
it 'has a button to toggle whitespace changes' do it 'has a button to toggle whitespace changes' do
expect(page).to have_content 'Show whitespace changes' expect(page).to have_content 'Show whitespace changes'
end end
......
...@@ -9,7 +9,6 @@ describe 'User views diffs', :js do ...@@ -9,7 +9,6 @@ describe 'User views diffs', :js do
let(:project) { create(:project, :public, :repository) } let(:project) { create(:project, :public, :repository) }
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
visit(diffs_project_merge_request_path(project, merge_request)) visit(diffs_project_merge_request_path(project, merge_request))
...@@ -18,8 +17,6 @@ describe 'User views diffs', :js do ...@@ -18,8 +17,6 @@ describe 'User views diffs', :js do
find('.js-toggle-tree-list').click find('.js-toggle-tree-list').click
end end
it_behaves_like 'rendering a single diff version'
shared_examples 'unfold diffs' do shared_examples 'unfold diffs' do
it 'unfolds diffs upwards' do it 'unfolds diffs upwards' do
first('.js-unfold').click first('.js-unfold').click
......
...@@ -12,11 +12,9 @@ describe 'Editing file blob', :js do ...@@ -12,11 +12,9 @@ describe 'Editing file blob', :js do
let(:readme_file_path) { 'README.md' } let(:readme_file_path) { 'README.md' }
before do before do
stub_feature_flags(web_ide_default: false, single_mr_diff_view: false) stub_feature_flags(web_ide_default: false)
end end
it_behaves_like 'rendering a single diff version'
context 'as a developer' do context 'as a developer' do
let(:user) { create(:user) } let(:user) { create(:user) }
let(:role) { :developer } let(:role) { :developer }
......
...@@ -9,14 +9,11 @@ describe 'View on environment', :js do ...@@ -9,14 +9,11 @@ describe 'View on environment', :js do
let(:user) { project.creator } let(:user) { project.creator }
before do before do
stub_feature_flags(single_mr_diff_view: false)
stub_feature_flags(diffs_batch_load: false) stub_feature_flags(diffs_batch_load: false)
project.add_maintainer(user) project.add_maintainer(user)
end end
it_behaves_like 'rendering a single diff version'
context 'when the branch has a route map' do context 'when the branch has a route map' do
let(:route_map) do let(:route_map) do
<<-MAP.strip_heredoc <<-MAP.strip_heredoc
......
import * as diffsHelper from '~/helpers/diffs_helper';
describe('diffs helper', () => {
function getDiffFile(withOverrides = {}) {
return {
parallel_diff_lines: ['line'],
highlighted_diff_lines: ['line'],
blob: {
readable_text: 'text',
},
...withOverrides,
};
}
describe('hasInlineLines', () => {
it('is false when the file does not exist', () => {
expect(diffsHelper.hasInlineLines()).toBeFalsy();
});
it('is false when the file does not have the highlighted_diff_lines property', () => {
const missingInline = getDiffFile({ highlighted_diff_lines: undefined });
expect(diffsHelper.hasInlineLines(missingInline)).toBeFalsy();
});
it('is false when the file has zero highlighted_diff_lines', () => {
const emptyInline = getDiffFile({ highlighted_diff_lines: [] });
expect(diffsHelper.hasInlineLines(emptyInline)).toBeFalsy();
});
it('is true when the file has at least 1 highlighted_diff_lines', () => {
expect(diffsHelper.hasInlineLines(getDiffFile())).toBeTruthy();
});
});
describe('hasParallelLines', () => {
it('is false when the file does not exist', () => {
expect(diffsHelper.hasParallelLines()).toBeFalsy();
});
it('is false when the file does not have the parallel_diff_lines property', () => {
const missingInline = getDiffFile({ parallel_diff_lines: undefined });
expect(diffsHelper.hasParallelLines(missingInline)).toBeFalsy();
});
it('is false when the file has zero parallel_diff_lines', () => {
const emptyInline = getDiffFile({ parallel_diff_lines: [] });
expect(diffsHelper.hasParallelLines(emptyInline)).toBeFalsy();
});
it('is true when the file has at least 1 parallel_diff_lines', () => {
expect(diffsHelper.hasParallelLines(getDiffFile())).toBeTruthy();
});
});
describe('isSingleViewStyle', () => {
it('is true when the file has at least 1 inline line but no parallel lines for any reason', () => {
const noParallelLines = getDiffFile({ parallel_diff_lines: undefined });
const emptyParallelLines = getDiffFile({ parallel_diff_lines: [] });
expect(diffsHelper.isSingleViewStyle(noParallelLines)).toBeTruthy();
expect(diffsHelper.isSingleViewStyle(emptyParallelLines)).toBeTruthy();
});
it('is true when the file has at least 1 parallel line but no inline lines for any reason', () => {
const noInlineLines = getDiffFile({ highlighted_diff_lines: undefined });
const emptyInlineLines = getDiffFile({ highlighted_diff_lines: [] });
expect(diffsHelper.isSingleViewStyle(noInlineLines)).toBeTruthy();
expect(diffsHelper.isSingleViewStyle(emptyInlineLines)).toBeTruthy();
});
it('is true when the file does not have any inline lines or parallel lines for any reason', () => {
const noLines = getDiffFile({
highlighted_diff_lines: undefined,
parallel_diff_lines: undefined,
});
const emptyLines = getDiffFile({
highlighted_diff_lines: [],
parallel_diff_lines: [],
});
expect(diffsHelper.isSingleViewStyle(noLines)).toBeTruthy();
expect(diffsHelper.isSingleViewStyle(emptyLines)).toBeTruthy();
expect(diffsHelper.isSingleViewStyle()).toBeTruthy();
});
it('is false when the file has both inline and parallel lines', () => {
expect(diffsHelper.isSingleViewStyle(getDiffFile())).toBeFalsy();
});
});
describe.each`
context | inline | parallel | blob | expected
${'only has inline lines'} | ${['line']} | ${undefined} | ${undefined} | ${true}
${'only has parallel lines'} | ${undefined} | ${['line']} | ${undefined} | ${true}
${"doesn't have inline, parallel, or blob"} | ${undefined} | ${undefined} | ${undefined} | ${true}
${'has blob readable text'} | ${undefined} | ${undefined} | ${{ readable_text: 'text' }} | ${false}
`('when hasDiff', ({ context, inline, parallel, blob, expected }) => {
it(`${context}`, () => {
const diffFile = getDiffFile({
highlighted_diff_lines: inline,
parallel_diff_lines: parallel,
blob,
});
expect(diffsHelper.hasDiff(diffFile)).toEqual(expected);
});
});
});
...@@ -10,6 +10,7 @@ import CompareVersions from '~/diffs/components/compare_versions.vue'; ...@@ -10,6 +10,7 @@ import CompareVersions from '~/diffs/components/compare_versions.vue';
import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue'; import HiddenFilesWarning from '~/diffs/components/hidden_files_warning.vue';
import CommitWidget from '~/diffs/components/commit_widget.vue'; import CommitWidget from '~/diffs/components/commit_widget.vue';
import TreeList from '~/diffs/components/tree_list.vue'; import TreeList from '~/diffs/components/tree_list.vue';
import { INLINE_DIFF_VIEW_TYPE, PARALLEL_DIFF_VIEW_TYPE } from '~/diffs/constants';
import createDiffsStore from '../create_diffs_store'; import createDiffsStore from '../create_diffs_store';
import diffsMockData from '../mock_data/merge_request_diffs'; import diffsMockData from '../mock_data/merge_request_diffs';
...@@ -41,7 +42,6 @@ describe('diffs/components/app', () => { ...@@ -41,7 +42,6 @@ describe('diffs/components/app', () => {
changesEmptyStateIllustration: '', changesEmptyStateIllustration: '',
dismissEndpoint: '', dismissEndpoint: '',
showSuggestPopover: true, showSuggestPopover: true,
useSingleDiffStyle: false,
...props, ...props,
}, },
store, store,
...@@ -53,6 +53,12 @@ describe('diffs/components/app', () => { ...@@ -53,6 +53,12 @@ describe('diffs/components/app', () => {
}); });
} }
function getOppositeViewType(currentViewType) {
return currentViewType === INLINE_DIFF_VIEW_TYPE
? PARALLEL_DIFF_VIEW_TYPE
: INLINE_DIFF_VIEW_TYPE;
}
beforeEach(() => { beforeEach(() => {
// setup globals (needed for component to mount :/) // setup globals (needed for component to mount :/)
window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']); window.mrTabs = jasmine.createSpyObj('mrTabs', ['resetViewContainer']);
...@@ -82,9 +88,146 @@ describe('diffs/components/app', () => { ...@@ -82,9 +88,146 @@ describe('diffs/components/app', () => {
spyOn(wrapper.vm, 'startRenderDiffsQueue'); spyOn(wrapper.vm, 'startRenderDiffsQueue');
spyOn(wrapper.vm, 'unwatchDiscussions'); spyOn(wrapper.vm, 'unwatchDiscussions');
store.state.diffs.retrievingBatches = true; store.state.diffs.retrievingBatches = true;
store.state.diffs.diffFiles = [];
wrapper.vm.$nextTick(done); wrapper.vm.$nextTick(done);
}); });
describe('when the diff view type changes and it should load a single diff view style', () => {
const noLinesDiff = {
highlighted_diff_lines: [],
parallel_diff_lines: [],
};
const parallelLinesDiff = {
highlighted_diff_lines: [],
parallel_diff_lines: ['line'],
};
const inlineLinesDiff = {
highlighted_diff_lines: ['line'],
parallel_diff_lines: [],
};
const fullDiff = {
highlighted_diff_lines: ['line'],
parallel_diff_lines: ['line'],
};
function expectFetchToOccur({
vueInstance,
done = () => {},
batch = false,
existingFiles = 1,
} = {}) {
vueInstance.$nextTick(() => {
expect(vueInstance.diffFiles.length).toEqual(existingFiles);
if (!batch) {
expect(vueInstance.fetchDiffFiles).toHaveBeenCalled();
expect(vueInstance.fetchDiffFilesBatch).not.toHaveBeenCalled();
} else {
expect(vueInstance.fetchDiffFiles).not.toHaveBeenCalled();
expect(vueInstance.fetchDiffFilesBatch).toHaveBeenCalled();
}
done();
});
}
beforeEach(() => {
wrapper.vm.glFeatures.singleMrDiffView = true;
});
it('fetches diffs if it has none', done => {
wrapper.vm.isLatestVersion = () => false;
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, batch: false, existingFiles: 0, done });
});
it('fetches diffs if it has both view styles, but no lines in either', done => {
wrapper.vm.isLatestVersion = () => false;
store.state.diffs.diffFiles.push(noLinesDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, done });
});
it('fetches diffs if it only has inline view style', done => {
wrapper.vm.isLatestVersion = () => false;
store.state.diffs.diffFiles.push(inlineLinesDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, done });
});
it('fetches diffs if it only has parallel view style', done => {
wrapper.vm.isLatestVersion = () => false;
store.state.diffs.diffFiles.push(parallelLinesDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, done });
});
it('fetches batch diffs if it has none', done => {
wrapper.vm.glFeatures.diffsBatchLoad = true;
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, existingFiles: 0, done });
});
it('fetches batch diffs if it has both view styles, but no lines in either', done => {
wrapper.vm.glFeatures.diffsBatchLoad = true;
store.state.diffs.diffFiles.push(noLinesDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done });
});
it('fetches batch diffs if it only has inline view style', done => {
wrapper.vm.glFeatures.diffsBatchLoad = true;
store.state.diffs.diffFiles.push(inlineLinesDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done });
});
it('fetches batch diffs if it only has parallel view style', done => {
wrapper.vm.glFeatures.diffsBatchLoad = true;
store.state.diffs.diffFiles.push(parallelLinesDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expectFetchToOccur({ vueInstance: wrapper.vm, batch: true, done });
});
it('does not fetch diffs if it has already fetched both styles of diff', () => {
wrapper.vm.glFeatures.diffsBatchLoad = false;
store.state.diffs.diffFiles.push(fullDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expect(wrapper.vm.diffFiles.length).toEqual(1);
expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
});
it('does not fetch batch diffs if it has already fetched both styles of diff', () => {
wrapper.vm.glFeatures.diffsBatchLoad = true;
store.state.diffs.diffFiles.push(fullDiff);
store.state.diffs.diffViewType = getOppositeViewType(wrapper.vm.diffViewType);
expect(wrapper.vm.diffFiles.length).toEqual(1);
expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
});
});
it('calls fetchDiffFiles if diffsBatchLoad is not enabled', done => { it('calls fetchDiffFiles if diffsBatchLoad is not enabled', done => {
wrapper.vm.glFeatures.diffsBatchLoad = false; wrapper.vm.glFeatures.diffsBatchLoad = false;
wrapper.vm.fetchData(false); wrapper.vm.fetchData(false);
......
...@@ -120,7 +120,7 @@ describe('DiffsStoreActions', () => { ...@@ -120,7 +120,7 @@ describe('DiffsStoreActions', () => {
describe('fetchDiffFiles', () => { describe('fetchDiffFiles', () => {
it('should fetch diff files', done => { it('should fetch diff files', done => {
const endpoint = '/fetch/diff/files?w=1'; const endpoint = '/fetch/diff/files?view=inline&w=1';
const mock = new MockAdapter(axios); const mock = new MockAdapter(axios);
const res = { diff_files: 1, merge_request_diffs: [] }; const res = { diff_files: 1, merge_request_diffs: [] };
mock.onGet(endpoint).reply(200, res); mock.onGet(endpoint).reply(200, res);
...@@ -128,7 +128,7 @@ describe('DiffsStoreActions', () => { ...@@ -128,7 +128,7 @@ describe('DiffsStoreActions', () => {
testAction( testAction(
fetchDiffFiles, fetchDiffFiles,
{}, {},
{ endpoint }, { endpoint, diffFiles: [], showWhitespace: false, diffViewType: 'inline' },
[ [
{ type: types.SET_LOADING, payload: true }, { type: types.SET_LOADING, payload: true },
{ type: types.SET_LOADING, payload: false }, { type: types.SET_LOADING, payload: false },
......
...@@ -52,7 +52,14 @@ describe('DiffsStoreMutations', () => { ...@@ -52,7 +52,14 @@ describe('DiffsStoreMutations', () => {
describe('SET_DIFF_DATA', () => { describe('SET_DIFF_DATA', () => {
it('should set diff data type properly', () => { it('should set diff data type properly', () => {
const state = {}; const state = {
diffFiles: [
{
content_sha: diffFileMockData.content_sha,
file_hash: diffFileMockData.file_hash,
},
],
};
const diffMock = { const diffMock = {
diff_files: [diffFileMockData], diff_files: [diffFileMockData],
}; };
...@@ -62,9 +69,41 @@ describe('DiffsStoreMutations', () => { ...@@ -62,9 +69,41 @@ describe('DiffsStoreMutations', () => {
const firstLine = state.diffFiles[0].parallel_diff_lines[0]; const firstLine = state.diffFiles[0].parallel_diff_lines[0];
expect(firstLine.right.text).toBeUndefined(); expect(firstLine.right.text).toBeUndefined();
expect(state.diffFiles.length).toEqual(1);
expect(state.diffFiles[0].renderIt).toEqual(true); expect(state.diffFiles[0].renderIt).toEqual(true);
expect(state.diffFiles[0].collapsed).toEqual(false); expect(state.diffFiles[0].collapsed).toEqual(false);
}); });
describe('given diffsBatchLoad feature flag is enabled', () => {
beforeEach(() => {
gon.features = { diffsBatchLoad: true };
});
afterEach(() => {
delete gon.features;
});
it('should not modify the existing state', () => {
const state = {
diffFiles: [
{
content_sha: diffFileMockData.content_sha,
file_hash: diffFileMockData.file_hash,
highlighted_diff_lines: [],
},
],
};
const diffMock = {
diff_files: [diffFileMockData],
};
mutations[types.SET_DIFF_DATA](state, diffMock);
// If the batch load is enabled, there shouldn't be any processing
// done on the existing state object, so we shouldn't have this.
expect(state.diffFiles[0].parallel_diff_lines).toBeUndefined();
});
});
}); });
describe('SET_DIFFSET_DIFF_DATA_BATCH_DATA', () => { describe('SET_DIFFSET_DIFF_DATA_BATCH_DATA', () => {
...@@ -168,11 +207,17 @@ describe('DiffsStoreMutations', () => { ...@@ -168,11 +207,17 @@ describe('DiffsStoreMutations', () => {
it('should update the state with the given data for the given file hash', () => { it('should update the state with the given data for the given file hash', () => {
const fileHash = 123; const fileHash = 123;
const state = { const state = {
diffFiles: [{}, { file_hash: fileHash, existing_field: 0 }], diffFiles: [{}, { content_sha: 'abc', file_hash: fileHash, existing_field: 0 }],
}; };
const data = { const data = {
diff_files: [ diff_files: [
{ file_hash: fileHash, extra_field: 1, existing_field: 1, viewer: { name: 'text' } }, {
content_sha: 'abc',
file_hash: fileHash,
extra_field: 1,
existing_field: 1,
viewer: { name: 'text' },
},
], ],
}; };
...@@ -208,7 +253,7 @@ describe('DiffsStoreMutations', () => { ...@@ -208,7 +253,7 @@ describe('DiffsStoreMutations', () => {
discussions: [], discussions: [],
}, },
right: { right: {
line_code: 'ABC_1', line_code: 'ABC_2',
discussions: [], discussions: [],
}, },
}, },
...@@ -274,7 +319,7 @@ describe('DiffsStoreMutations', () => { ...@@ -274,7 +319,7 @@ describe('DiffsStoreMutations', () => {
discussions: [], discussions: [],
}, },
right: { right: {
line_code: 'ABC_1', line_code: 'ABC_2',
discussions: [], discussions: [],
}, },
}, },
...@@ -352,7 +397,7 @@ describe('DiffsStoreMutations', () => { ...@@ -352,7 +397,7 @@ describe('DiffsStoreMutations', () => {
discussions: [], discussions: [],
}, },
right: { right: {
line_code: 'ABC_1', line_code: 'ABC_2',
discussions: [], discussions: [],
}, },
}, },
...@@ -448,6 +493,7 @@ describe('DiffsStoreMutations', () => { ...@@ -448,6 +493,7 @@ describe('DiffsStoreMutations', () => {
discussions: [], discussions: [],
}, },
], ],
parallel_diff_lines: [],
}, },
], ],
}; };
......
...@@ -314,11 +314,29 @@ describe('DiffsStoreUtils', () => { ...@@ -314,11 +314,29 @@ describe('DiffsStoreUtils', () => {
}); });
describe('prepareDiffData', () => { describe('prepareDiffData', () => {
let mock;
let preparedDiff; let preparedDiff;
let splitInlineDiff;
let splitParallelDiff;
let completedDiff;
beforeEach(() => { beforeEach(() => {
preparedDiff = { diff_files: [getDiffFileMock()] }; mock = getDiffFileMock();
preparedDiff = { diff_files: [mock] };
splitInlineDiff = {
diff_files: [Object.assign({}, mock, { parallel_diff_lines: undefined })],
};
splitParallelDiff = {
diff_files: [Object.assign({}, mock, { highlighted_diff_lines: undefined })],
};
completedDiff = {
diff_files: [Object.assign({}, mock, { highlighted_diff_lines: undefined })],
};
utils.prepareDiffData(preparedDiff); utils.prepareDiffData(preparedDiff);
utils.prepareDiffData(splitInlineDiff);
utils.prepareDiffData(splitParallelDiff);
utils.prepareDiffData(completedDiff, [mock]);
}); });
it('sets the renderIt and collapsed attribute on files', () => { it('sets the renderIt and collapsed attribute on files', () => {
...@@ -359,6 +377,19 @@ describe('DiffsStoreUtils', () => { ...@@ -359,6 +377,19 @@ describe('DiffsStoreUtils', () => {
expect(firstLine.line_code).toEqual(firstLine.right.line_code); expect(firstLine.line_code).toEqual(firstLine.right.line_code);
}); });
it('guarantees an empty array for both diff styles', () => {
expect(splitInlineDiff.diff_files[0].parallel_diff_lines.length).toEqual(0);
expect(splitInlineDiff.diff_files[0].highlighted_diff_lines.length).toBeGreaterThan(0);
expect(splitParallelDiff.diff_files[0].parallel_diff_lines.length).toBeGreaterThan(0);
expect(splitParallelDiff.diff_files[0].highlighted_diff_lines.length).toEqual(0);
});
it('merges existing diff files with newly loaded diff files to ensure split diffs are eventually completed', () => {
expect(completedDiff.diff_files.length).toEqual(1);
expect(completedDiff.diff_files[0].parallel_diff_lines.length).toBeGreaterThan(0);
expect(completedDiff.diff_files[0].highlighted_diff_lines.length).toBeGreaterThan(0);
});
}); });
describe('isDiscussionApplicableToLine', () => { describe('isDiscussionApplicableToLine', () => {
......
# frozen_string_literal: true
# This pending test can be removed when `single_mr_diff_view` is enabled by default
# disabling the feature flag above is then not needed anymore.
RSpec.shared_examples 'rendering a single diff version' do |attribute|
before do
stub_feature_flags(diffs_batch_load: false)
end
pending 'allows editing diff settings single_mr_diff_view is enabled' do
project = create(:project, :repository)
user = project.creator
merge_request = create(:merge_request, source_project: project)
stub_feature_flags(single_mr_diff_view: true)
sign_in(user)
visit(diffs_project_merge_request_path(project, merge_request))
expect(page).to have_selector('.js-show-diff-settings')
end
end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment