Merge branch '214824-fix-web-ide-open-file-race-condition' into 'master'

Resolve "WebIDE displays blank file incorrectly"

Closes #214824

See merge request gitlab-org/gitlab!33391

app/assets/javascripts/ide/components/repo_editor.vue

@@ -185,7 +185,6 @@ export default {
       'setFileLanguage',
       'setEditorPosition',
       'setFileViewMode',
-      'updateViewer',
       'removePendingTab',
       'triggerFilesChange',
       'addTempImage',
@@ -241,7 +240,7 @@ export default {
       });
     },
     setupEditor() {
-      if (!this.file || !this.editor.instance) return;
+      if (!this.file || !this.editor.instance || this.file.loading) return;
 
       const head = this.getStagedFile(this.file.path);
 

app/assets/javascripts/ide/stores/actions/file.js

@@ -65,7 +65,7 @@ export const getFileData = (
   if (file.raw || (file.tempFile && !file.prevPath && !fileDeletedAndReadded))
     return Promise.resolve();
 
-  commit(types.TOGGLE_LOADING, { entry: file });
+  commit(types.TOGGLE_LOADING, { entry: file, forceValue: true });
 
   const url = joinPaths(
     gon.relative_url_root || '/',
@@ -84,10 +84,8 @@ export const getFileData = (
       if (data) commit(types.SET_FILE_DATA, { data, file });
       if (openFile) commit(types.TOGGLE_FILE_OPEN, path);
       if (makeFileActive) dispatch('setFileActive', path);
-      commit(types.TOGGLE_LOADING, { entry: file });
     })
     .catch(() => {
-      commit(types.TOGGLE_LOADING, { entry: file });
       dispatch('setErrorMessage', {
         text: __('An error occurred while loading the file.'),
         action: payload =>
@@ -95,6 +93,9 @@ export const getFileData = (
         actionText: __('Please try again'),
         actionPayload: { path, makeFileActive },
       });
+    })
+    .finally(() => {
+      commit(types.TOGGLE_LOADING, { entry: file, forceValue: false });
     });
 };
 
@@ -106,45 +107,41 @@ export const getRawFileData = ({ state, commit, dispatch, getters }, { path }) =
   const file = state.entries[path];
   const stagedFile = state.stagedFiles.find(f => f.path === path);
 
-  return new Promise((resolve, reject) => {
-    const fileDeletedAndReadded = getters.isFileDeletedAndReadded(path);
-    service
-      .getRawFileData(fileDeletedAndReadded ? stagedFile : file)
-      .then(raw => {
-        if (!(file.tempFile && !file.prevPath && !fileDeletedAndReadded))
-          commit(types.SET_FILE_RAW_DATA, { file, raw, fileDeletedAndReadded });
-
-        if (file.mrChange && file.mrChange.new_file === false) {
-          const baseSha =
-            (getters.currentMergeRequest && getters.currentMergeRequest.baseCommitSha) || '';
-
-          service
-            .getBaseRawFileData(file, baseSha)
-            .then(baseRaw => {
-              commit(types.SET_FILE_BASE_RAW_DATA, {
-                file,
-                baseRaw,
-              });
-              resolve(raw);
-            })
-            .catch(e => {
-              reject(e);
-            });
-        } else {
-          resolve(raw);
-        }
-      })
-      .catch(() => {
-        dispatch('setErrorMessage', {
-          text: __('An error occurred while loading the file content.'),
-          action: payload =>
-            dispatch('getRawFileData', payload).then(() => dispatch('setErrorMessage', null)),
-          actionText: __('Please try again'),
-          actionPayload: { path },
+  const fileDeletedAndReadded = getters.isFileDeletedAndReadded(path);
+  commit(types.TOGGLE_LOADING, { entry: file, forceValue: true });
+  return service
+    .getRawFileData(fileDeletedAndReadded ? stagedFile : file)
+    .then(raw => {
+      if (!(file.tempFile && !file.prevPath && !fileDeletedAndReadded))
+        commit(types.SET_FILE_RAW_DATA, { file, raw, fileDeletedAndReadded });
+
+      if (file.mrChange && file.mrChange.new_file === false) {
+        const baseSha =
+          (getters.currentMergeRequest && getters.currentMergeRequest.baseCommitSha) || '';
+
+        return service.getBaseRawFileData(file, baseSha).then(baseRaw => {
+          commit(types.SET_FILE_BASE_RAW_DATA, {
+            file,
+            baseRaw,
+          });
+          return raw;
         });
-        reject();
+      }
+      return raw;
+    })
+    .catch(e => {
+      dispatch('setErrorMessage', {
+        text: __('An error occurred while loading the file content.'),
+        action: payload =>
+          dispatch('getRawFileData', payload).then(() => dispatch('setErrorMessage', null)),
+        actionText: __('Please try again'),
+        actionPayload: { path },
       });
-  });
+      throw e;
+    })
+    .finally(() => {
+      commit(types.TOGGLE_LOADING, { entry: file, forceValue: false });
+    });
 };
 
 export const changeFileContent = ({ commit, state, getters }, { path, content }) => {

changelogs/unreleased/214824-fix-web-ide-open-file-race-condition.yml

+---
+title: Resolve "WebIDE displays blank file incorrectly"
+merge_request: 33391
+type: fixed

spec/frontend/helpers/wait_using_real_timer.js

+/* useful for timing promises when jest fakeTimers are not reliable enough */
+export default timeout =>
+  new Promise(resolve => {
+    jest.useRealTimers();
+    setTimeout(resolve, timeout);
+    jest.useFakeTimers();
+  });

spec/frontend/ide/components/repo_editor_spec.js

@@ -4,19 +4,25 @@ import MockAdapter from 'axios-mock-adapter';
 import '~/behaviors/markdown/render_gfm';
 import { Range } from 'monaco-editor';
 import axios from '~/lib/utils/axios_utils';
+import service from '~/ide/services';
 import { createStoreOptions } from '~/ide/stores';
 import RepoEditor from '~/ide/components/repo_editor.vue';
 import Editor from '~/ide/lib/editor';
-import { leftSidebarViews, FILE_VIEW_MODE_EDITOR, FILE_VIEW_MODE_PREVIEW } from '~/ide/constants';
+import {
+  leftSidebarViews,
+  FILE_VIEW_MODE_EDITOR,
+  FILE_VIEW_MODE_PREVIEW,
+  viewerTypes,
+} from '~/ide/constants';
 import { createComponentWithStore } from '../../helpers/vue_mount_component_helper';
 import waitForPromises from 'helpers/wait_for_promises';
 import { file } from '../helpers';
 import { exampleConfigs, exampleFiles } from '../lib/editorconfig/mock_data';
+import waitUsingRealTimer from 'helpers/wait_using_real_timer';
 
 describe('RepoEditor', () => {
   let vm;
   let store;
-  let mockActions;
 
   const waitForEditorSetup = () =>
     new Promise(resolve => {
@@ -30,6 +36,10 @@ describe('RepoEditor', () => {
     vm = createComponentWithStore(Vue.extend(RepoEditor), store, {
       file: store.state.openFiles[0],
     });
+
+    jest.spyOn(vm, 'getFileData').mockResolvedValue();
+    jest.spyOn(vm, 'getRawFileData').mockResolvedValue();
+
     vm.$mount();
   };
 
@@ -43,21 +53,12 @@ describe('RepoEditor', () => {
   };
 
   beforeEach(() => {
-    mockActions = {
-      getFileData: jest.fn().mockResolvedValue(),
-      getRawFileData: jest.fn().mockResolvedValue(),
-    };
-
     const f = {
       ...file(),
       viewMode: FILE_VIEW_MODE_EDITOR,
     };
 
     const storeOptions = createStoreOptions();
-    storeOptions.actions = {
-      ...storeOptions.actions,
-      ...mockActions,
-    };
     store = new Vuex.Store(storeOptions);
 
     f.active = true;
@@ -438,7 +439,7 @@ describe('RepoEditor', () => {
         vm.initEditor();
         vm.$nextTick()
           .then(() => {
-            expect(mockActions.getFileData).not.toHaveBeenCalled();
+            expect(vm.getFileData).not.toHaveBeenCalled();
           })
           .then(done)
           .catch(done.fail);
@@ -449,10 +450,11 @@ describe('RepoEditor', () => {
         vm.file.raw = '';
 
         vm.initEditor();
+
         vm.$nextTick()
           .then(() => {
-            expect(mockActions.getFileData).toHaveBeenCalled();
-            expect(mockActions.getRawFileData).toHaveBeenCalled();
+            expect(vm.getFileData).toHaveBeenCalled();
+            expect(vm.getRawFileData).toHaveBeenCalled();
           })
           .then(done)
           .catch(done.fail);
@@ -464,8 +466,8 @@ describe('RepoEditor', () => {
         vm.initEditor();
         vm.$nextTick()
           .then(() => {
-            expect(mockActions.getFileData).not.toHaveBeenCalled();
-            expect(mockActions.getRawFileData).not.toHaveBeenCalled();
+            expect(vm.getFileData).not.toHaveBeenCalled();
+            expect(vm.getRawFileData).not.toHaveBeenCalled();
             expect(vm.editor.createInstance).not.toHaveBeenCalled();
           })
           .then(done)
@@ -526,6 +528,65 @@ describe('RepoEditor', () => {
       });
     });
 
+    describe('populates editor with the fetched content', () => {
+      beforeEach(() => {
+        vm.getRawFileData.mockRestore();
+      });
+
+      const createRemoteFile = name => ({
+        ...file(name),
+        tmpFile: false,
+      });
+
+      it('after switching viewer from edit to diff', async () => {
+        jest.spyOn(service, 'getRawFileData').mockImplementation(async () => {
+          expect(vm.file.loading).toBe(true);
+
+          // switching from edit to diff mode usually triggers editor initialization
+          store.state.viewer = viewerTypes.diff;
+
+          // we delay returning the file to make sure editor doesn't initialize before we fetch file content
+          await waitUsingRealTimer(10);
+          return 'rawFileData123\n';
+        });
+
+        const f = createRemoteFile('newFile');
+        Vue.set(store.state.entries, f.path, f);
+
+        vm.file = f;
+
+        // use the real timer to accurately simulate the race condition
+        await waitUsingRealTimer(20);
+        expect(vm.model.getModel().getValue()).toBe('rawFileData123\n');
+      });
+
+      it('after opening multiple files at the same time', async () => {
+        const fileA = createRemoteFile('fileA');
+        const fileB = createRemoteFile('fileB');
+        Vue.set(store.state.entries, fileA.path, fileA);
+        Vue.set(store.state.entries, fileB.path, fileB);
+
+        jest
+          .spyOn(service, 'getRawFileData')
+          .mockImplementationOnce(async () => {
+            // opening fileB while the content of fileA is still being fetched
+            vm.file = fileB;
+            return 'fileA-rawContent\n';
+          })
+          .mockImplementationOnce(async () => {
+            // we delay returning fileB content to make sure the editor doesn't initialize prematurely
+            await waitUsingRealTimer(10);
+            return 'fileB-rawContent\n';
+          });
+
+        vm.file = fileA;
+
+        // use the real timer to accurately simulate the race condition
+        await waitUsingRealTimer(20);
+        expect(vm.model.getModel().getValue()).toBe('fileB-rawContent\n');
+      });
+    });
+
     describe('onPaste', () => {
       const setFileName = name => {
         Vue.set(vm, 'file', {
@@ -632,8 +693,8 @@ describe('RepoEditor', () => {
         return waitForEditorSetup().then(() => {
           expect(vm.rules).toEqual(monacoRules);
           expect(vm.model.options).toMatchObject(monacoRules);
-          expect(mockActions.getFileData).not.toHaveBeenCalled();
-          expect(mockActions.getRawFileData).not.toHaveBeenCalled();
+          expect(vm.getFileData).not.toHaveBeenCalled();
+          expect(vm.getRawFileData).not.toHaveBeenCalled();
         });
       },
     );
@@ -649,13 +710,13 @@ describe('RepoEditor', () => {
       createComponent();
 
       return waitForEditorSetup().then(() => {
-        expect(mockActions.getFileData.mock.calls.map(([, args]) => args)).toEqual([
+        expect(vm.getFileData.mock.calls.map(([args]) => args)).toEqual([
           { makeFileActive: false, path: 'foo/bar/baz/.editorconfig' },
           { makeFileActive: false, path: 'foo/bar/.editorconfig' },
           { makeFileActive: false, path: 'foo/.editorconfig' },
           { makeFileActive: false, path: '.editorconfig' },
         ]);
-        expect(mockActions.getRawFileData.mock.calls.map(([, args]) => args)).toEqual([
+        expect(vm.getRawFileData.mock.calls.map(([args]) => args)).toEqual([
           { path: 'foo/bar/baz/.editorconfig' },
           { path: 'foo/bar/.editorconfig' },
           { path: 'foo/.editorconfig' },

spec/frontend/ide/stores/actions/file_spec.js

@@ -446,6 +446,54 @@ describe('IDE store file actions', () => {
           })
           .catch(done.fail);
       });
+
+      describe('sets file loading to true', () => {
+        let loadingWhenGettingRawData;
+        let loadingWhenGettingBaseRawData;
+
+        beforeEach(() => {
+          loadingWhenGettingRawData = undefined;
+          loadingWhenGettingBaseRawData = undefined;
+
+          jest.spyOn(service, 'getRawFileData').mockImplementation(f => {
+            loadingWhenGettingRawData = f.loading;
+            return Promise.resolve('raw');
+          });
+          jest.spyOn(service, 'getBaseRawFileData').mockImplementation(f => {
+            loadingWhenGettingBaseRawData = f.loading;
+            return Promise.resolve('rawBase');
+          });
+        });
+
+        it('when getting raw file data', async () => {
+          expect(tmpFile.loading).toBe(false);
+
+          await store.dispatch('getRawFileData', { path: tmpFile.path });
+
+          expect(loadingWhenGettingRawData).toBe(true);
+          expect(tmpFile.loading).toBe(false);
+        });
+
+        it('when getting base raw file data', async () => {
+          tmpFile.mrChange = { new_file: false };
+
+          expect(tmpFile.loading).toBe(false);
+
+          await store.dispatch('getRawFileData', { path: tmpFile.path });
+
+          expect(loadingWhenGettingBaseRawData).toBe(true);
+          expect(tmpFile.loading).toBe(false);
+        });
+
+        it('when file was already loading', async () => {
+          tmpFile.loading = true;
+
+          await store.dispatch('getRawFileData', { path: tmpFile.path });
+
+          expect(loadingWhenGettingRawData).toBe(true);
+          expect(tmpFile.loading).toBe(false);
+        });
+      });
     });
 
     describe('return JSON', () => {
@@ -489,6 +537,12 @@ describe('IDE store file actions', () => {
             });
           });
       });
+
+      it('toggles loading off after error', async () => {
+        await expect(store.dispatch('getRawFileData', { path: tmpFile.path })).rejects.toThrow();
+
+        expect(tmpFile.loading).toBe(false);
+      });
     });
   });