Commit 57db55af authored by Phil Hughes's avatar Phil Hughes

Fix race condition with batch diff loading

Fixes a race condition with batch diff loading that
would cause the diff files to get overwritten with
an empty array.

Closes https://gitlab.com/gitlab-org/gitlab/issues/37740
parent c87734e1
...@@ -144,6 +144,9 @@ export default { ...@@ -144,6 +144,9 @@ export default {
isLimitedContainer() { isLimitedContainer() {
return !this.showTreeList && !this.isParallelView && !this.isFluidLayout; return !this.showTreeList && !this.isParallelView && !this.isFluidLayout;
}, },
shouldSetDiscussions() {
return this.isNotesFetched && !this.assignedDiscussions && !this.isLoading;
},
}, },
watch: { watch: {
diffViewType() { diffViewType() {
...@@ -160,6 +163,11 @@ export default { ...@@ -160,6 +163,11 @@ export default {
}, },
isLoading: 'adjustView', isLoading: 'adjustView',
showTreeList: 'adjustView', showTreeList: 'adjustView',
shouldSetDiscussions(newVal) {
if (newVal) {
this.setDiscussions();
}
},
}, },
mounted() { mounted() {
this.setBaseConfig({ this.setBaseConfig({
...@@ -214,26 +222,28 @@ export default { ...@@ -214,26 +222,28 @@ export default {
isLatestVersion() { isLatestVersion() {
return window.location.search.indexOf('diff_id') === -1; return window.location.search.indexOf('diff_id') === -1;
}, },
startDiffRendering() {
requestIdleCallback(
() => {
this.startRenderDiffsQueue();
},
{ timeout: 1000 },
);
},
fetchData(toggleTree = true) { fetchData(toggleTree = true) {
if (this.isLatestVersion() && this.glFeatures.diffsBatchLoad) { if (this.isLatestVersion() && this.glFeatures.diffsBatchLoad) {
this.fetchDiffFilesMeta() this.fetchDiffFilesMeta()
.then(() => { .then(() => {
if (toggleTree) this.hideTreeListIfJustOneFile(); if (toggleTree) this.hideTreeListIfJustOneFile();
this.startDiffRendering();
}) })
.catch(() => { .catch(() => {
createFlash(__('Something went wrong on our end. Please try again!')); createFlash(__('Something went wrong on our end. Please try again!'));
}); });
this.fetchDiffFilesBatch() this.fetchDiffFilesBatch()
.then(() => { .then(() => this.startDiffRendering())
requestIdleCallback(
() => {
this.setDiscussions();
this.startRenderDiffsQueue();
},
{ timeout: 1000 },
);
})
.catch(() => { .catch(() => {
createFlash(__('Something went wrong on our end. Please try again!')); createFlash(__('Something went wrong on our end. Please try again!'));
}); });
...@@ -246,7 +256,6 @@ export default { ...@@ -246,7 +256,6 @@ export default {
requestIdleCallback( requestIdleCallback(
() => { () => {
this.setDiscussions();
this.startRenderDiffsQueue(); this.startRenderDiffsQueue();
}, },
{ timeout: 1000 }, { timeout: 1000 },
...@@ -262,7 +271,7 @@ export default { ...@@ -262,7 +271,7 @@ export default {
} }
}, },
setDiscussions() { setDiscussions() {
if (this.isNotesFetched && !this.assignedDiscussions && !this.isLoading) { if (this.shouldSetDiscussions) {
this.assignedDiscussions = true; this.assignedDiscussions = true;
requestIdleCallback( requestIdleCallback(
......
...@@ -119,7 +119,7 @@ export const fetchDiffFilesMeta = ({ commit, state }) => { ...@@ -119,7 +119,7 @@ export const fetchDiffFilesMeta = ({ commit, state }) => {
.get(state.endpointMetadata) .get(state.endpointMetadata)
.then(({ data }) => { .then(({ data }) => {
const strippedData = { ...data }; const strippedData = { ...data };
strippedData.diff_files = []; delete strippedData.diff_files;
commit(types.SET_LOADING, false); commit(types.SET_LOADING, false);
commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []); commit(types.SET_MERGE_REQUEST_DIFFS, data.merge_request_diffs || []);
commit(types.SET_DIFF_DATA, strippedData); commit(types.SET_DIFF_DATA, strippedData);
......
...@@ -39,7 +39,16 @@ export default { ...@@ -39,7 +39,16 @@ export default {
}, },
[types.SET_DIFF_DATA](state, data) { [types.SET_DIFF_DATA](state, data) {
prepareDiffData(data); if (
!(
gon &&
gon.features &&
gon.features.diffsBatchLoad &&
window.location.search.indexOf('diff_id') === -1
)
) {
prepareDiffData(data);
}
Object.assign(state, { Object.assign(state, {
...convertObjectPropsToCamelCase(data), ...convertObjectPropsToCamelCase(data),
......
...@@ -77,16 +77,17 @@ describe('diffs/components/app', () => { ...@@ -77,16 +77,17 @@ describe('diffs/components/app', () => {
spyOn(wrapper.vm, 'startRenderDiffsQueue'); spyOn(wrapper.vm, 'startRenderDiffsQueue');
}); });
it('calls fetchDiffFiles if diffsBatchLoad is not enabled', () => { 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);
expect(wrapper.vm.fetchDiffFiles).toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFiles).toHaveBeenCalled();
wrapper.vm.$nextTick(() => { setTimeout(() => {
expect(wrapper.vm.setDiscussions).toHaveBeenCalled();
expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled(); expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
done();
}); });
}); });
...@@ -97,7 +98,6 @@ describe('diffs/components/app', () => { ...@@ -97,7 +98,6 @@ describe('diffs/components/app', () => {
expect(wrapper.vm.fetchDiffFiles).toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFiles).toHaveBeenCalled();
wrapper.vm.$nextTick(() => { wrapper.vm.$nextTick(() => {
expect(wrapper.vm.setDiscussions).toHaveBeenCalled();
expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled(); expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFilesMeta).not.toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFilesBatch).not.toHaveBeenCalled();
...@@ -110,7 +110,6 @@ describe('diffs/components/app', () => { ...@@ -110,7 +110,6 @@ describe('diffs/components/app', () => {
expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFiles).not.toHaveBeenCalled();
wrapper.vm.$nextTick(() => { wrapper.vm.$nextTick(() => {
expect(wrapper.vm.setDiscussions).toHaveBeenCalled();
expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled(); expect(wrapper.vm.startRenderDiffsQueue).toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFilesMeta).toHaveBeenCalled();
expect(wrapper.vm.fetchDiffFilesBatch).toHaveBeenCalled(); expect(wrapper.vm.fetchDiffFilesBatch).toHaveBeenCalled();
......
...@@ -186,7 +186,7 @@ describe('DiffsStoreActions', () => { ...@@ -186,7 +186,7 @@ describe('DiffsStoreActions', () => {
{ type: types.SET_LOADING, payload: true }, { type: types.SET_LOADING, payload: true },
{ type: types.SET_LOADING, payload: false }, { type: types.SET_LOADING, payload: false },
{ type: types.SET_MERGE_REQUEST_DIFFS, payload: [] }, { type: types.SET_MERGE_REQUEST_DIFFS, payload: [] },
{ type: types.SET_DIFF_DATA, payload: { data, diff_files: [] } }, { type: types.SET_DIFF_DATA, payload: { data } },
], ],
[], [],
() => { () => {
......
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