Merge branch 'himkp-webide-binary-regression' into 'master'

Fix regression when uploading / viewing binary files in the Web IDE

See merge request gitlab-org/gitlab!44699

app/assets/javascripts/ide/components/new_dropdown/upload.vue

@@ -35,7 +35,7 @@ export default {
           name: `${this.path ? `${this.path}/` : ''}${name}`,
           type: 'blob',
           content,
-          rawPath: !isText ? target.result : '',
+          rawPath: !isText ? URL.createObjectURL(file) : '',
         });
 
       if (isText) {
@@ -44,7 +44,7 @@ export default {
         reader.addEventListener('load', e => emitCreateEvent(e.target.result), { once: true });
         reader.readAsText(file);
       } else {
-        emitCreateEvent(encodedContent);
+        emitCreateEvent(rawContent);
       }
     },
     readFile(file) {

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

@@ -68,7 +68,7 @@ export default {
     ]),
     ...mapGetters('fileTemplates', ['showFileTemplatesBar']),
     shouldHideEditor() {
-      return this.file && !isTextFile(this.file);
+      return this.file && !this.file.loading && !isTextFile(this.file);
     },
     showContentViewer() {
       return (
@@ -235,6 +235,7 @@ export default {
       return this.getFileData({
         path: this.file.path,
         makeFileActive: false,
+        toggleLoading: false,
       }).then(() =>
         this.getRawFileData({
           path: this.file.path,

app/assets/javascripts/ide/lib/editor.js

@@ -157,9 +157,11 @@ export default class Editor {
   }
 
   updateDimensions() {
+    if (this.instance) {
       this.instance.layout();
       this.updateDiffView();
     }
+  }
 
   setPosition({ lineNumber, column }) {
     this.instance.revealPositionInCenter({

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

@@ -59,7 +59,7 @@ export const setFileActive = ({ commit, state, getters, dispatch }, path) => {
 
 export const getFileData = (
   { state, commit, dispatch, getters },
-  { path, makeFileActive = true, openFile = makeFileActive },
+  { path, makeFileActive = true, openFile = makeFileActive, toggleLoading = true },
 ) => {
   const file = state.entries[path];
   const fileDeletedAndReadded = getters.isFileDeletedAndReadded(path);
@@ -99,7 +99,7 @@ export const getFileData = (
       });
     })
     .finally(() => {
-      commit(types.TOGGLE_LOADING, { entry: file, forceValue: false });
+      if (toggleLoading) commit(types.TOGGLE_LOADING, { entry: file, forceValue: false });
     });
 };
 

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

@@ -19,19 +19,20 @@ export default {
     }
   },
   [types.TOGGLE_FILE_OPEN](state, path) {
-    Object.assign(state.entries[path], {
-      opened: !state.entries[path].opened,
-    });
+    const entry = state.entries[path];
 
-    if (state.entries[path].opened) {
+    entry.opened = !entry.opened;
+    if (entry.opened && !entry.tempFile) {
+      entry.loading = true;
+    }
+
+    if (entry.opened) {
       Object.assign(state, {
         openFiles: state.openFiles.filter(f => f.path !== path).concat(state.entries[path]),
       });
     } else {
-      const file = state.entries[path];
-
       Object.assign(state, {
-        openFiles: state.openFiles.filter(f => f.key !== file.key),
+        openFiles: state.openFiles.filter(f => f.key !== entry.key),
       });
     }
   },

app/assets/javascripts/ide/stores/utils.js

@@ -3,7 +3,7 @@ import {
   relativePathToAbsolute,
   isAbsolute,
   isRootRelative,
-  isBase64DataUrl,
+  isBlobUrl,
 } from '~/lib/utils/url_utility';
 
 export const dataStructure = () => ({
@@ -110,14 +110,19 @@ export const createCommitPayload = ({
 }) => ({
   branch,
   commit_message: state.commitMessage || getters.preBuiltCommitMessage,
-  actions: getCommitFiles(rootState.stagedFiles).map(f => ({
+  actions: getCommitFiles(rootState.stagedFiles).map(f => {
+    const isBlob = isBlobUrl(f.rawPath);
+    const content = isBlob ? btoa(f.content) : f.content;
+
+    return {
       action: commitActionForFile(f),
       file_path: f.path,
       previous_path: f.prevPath || undefined,
-    content: f.prevPath && !f.changed ? null : f.content || undefined,
-    encoding: isBase64DataUrl(f.rawPath) ? 'base64' : 'text',
+      content: f.prevPath && !f.changed ? null : content || undefined,
+      encoding: isBlob ? 'base64' : 'text',
       last_commit_id: newBranch || f.deleted || f.prevPath ? undefined : f.lastCommitSha,
-  })),
+    };
+  }),
   start_sha: newBranch ? rootGetters.lastCommit.id : undefined,
 });
 

app/assets/javascripts/ide/utils.js

@@ -43,16 +43,17 @@ const KNOWN_TYPES = [
   },
 ];
 
-export function isTextFile({ name, content, mimeType = '' }) {
+export function isTextFile({ name, raw, content, mimeType = '' }) {
   const knownType = KNOWN_TYPES.find(type => type.isMatch(mimeType, name));
-
   if (knownType) return knownType.isText;
 
   // does the string contain ascii characters only (ranges from space to tilde, tabs and new lines)
   const asciiRegex = /^[ -~\t\n\r]+$/;
 
+  const fileContents = raw || content;
+
   // for unknown types, determine the type by evaluating the file contents
-  return isString(content) && (content === '' || asciiRegex.test(content));
+  return isString(fileContents) && (fileContents === '' || asciiRegex.test(fileContents));
 }
 
 export const createPathWithExt = p => {

app/assets/javascripts/lib/utils/url_utility.js

@@ -291,6 +291,15 @@ export function isBase64DataUrl(url) {
   return /^data:[.\w+-]+\/[.\w+-]+;base64,/.test(url);
 }
 
+/**
+ * Returns true if url is a blob: type url
+ *
+ * @param {String} url
+ */
+export function isBlobUrl(url) {
+  return /^blob:/.test(url);
+}
+
 /**
  * Returns true if url is an absolute or root-relative URL
  *

app/assets/javascripts/vue_shared/components/content_viewer/content_viewer.vue

@@ -4,6 +4,11 @@ import ImageViewer from './viewers/image_viewer.vue';
 import DownloadViewer from './viewers/download_viewer.vue';
 
 export default {
+  components: {
+    MarkdownViewer,
+    ImageViewer,
+    DownloadViewer,
+  },
   props: {
     content: {
       type: String,
@@ -45,35 +50,25 @@ export default {
       default: () => ({}),
     },
   },
-  computed: {
-    viewer() {
-      if (!this.path) return null;
-      if (!this.type) return DownloadViewer;
-
-      switch (this.type) {
-        case 'markdown':
-          return MarkdownViewer;
-        case 'image':
-          return ImageViewer;
-        default:
-          return DownloadViewer;
-      }
-    },
-  },
 };
 </script>
 
 <template>
   <div class="preview-container">
-    <component
-      :is="viewer"
-      :path="path"
+    <image-viewer v-if="type === 'image'" :path="path" :file-size="fileSize" />
+    <markdown-viewer
+      v-if="type === 'markdown'"
+      :content="content"
+      :commit-sha="commitSha"
       :file-path="filePath"
-      :file-size="fileSize"
       :project-path="projectPath"
-      :content="content"
       :images="images"
-      :commit-sha="commitSha"
+    />
+    <download-viewer
+      v-if="!type && path"
+      :path="path"
+      :file-path="filePath"
+      :file-size="fileSize"
     />
   </div>
 </template>

app/assets/javascripts/vue_shared/components/content_viewer/viewers/download_viewer.vue

 <script>
-import { GlLink, GlIcon } from '@gitlab/ui';
+import { GlIcon } from '@gitlab/ui';
 import { numberToHumanSize } from '../../../../lib/utils/number_utils';
 
 export default {
   components: {
-    GlLink,
     GlIcon,
   },
   props: {
@@ -44,16 +43,10 @@ export default {
           ({{ fileSizeReadable }})
         </template>
       </p>
-      <gl-link
-        :href="path"
-        class="btn btn-default"
-        rel="nofollow"
-        :download="fileName"
-        target="_blank"
-      >
+      <a :href="path" class="btn btn-default" rel="nofollow" :download="fileName" target="_blank">
         <gl-icon :size="16" name="download" class="float-left gl-mr-3" />
         {{ __('Download') }}
-      </gl-link>
+      </a>
     </div>
   </div>
 </template>

changelogs/unreleased/himkp-webide-binary-regression.yml

+---
+title: Fix regression when uploading / viewing binary files in the Web IDE
+merge_request: 44699
+author:
+type: fixed

spec/frontend/blob/sketch/index_spec.js

@@ -8,13 +8,6 @@ describe('Sketch viewer', () => {
 
   beforeEach(() => {
     loadFixtures('static/sketch_viewer.html');
-    window.URL = {
-      createObjectURL: jest.fn(() => 'http://foo/bar'),
-    };
-  });
-
-  afterEach(() => {
-    window.URL = {};
   });
 
   describe('with error message', () => {

spec/frontend/helpers/dom_shims/create_object_url.js

+URL.createObjectURL = function createObjectURL() {
+  return 'blob:https://gitlab.com/048c7ac1-98de-4a37-ab1b-0206d0ea7e1b';
+};

spec/frontend/helpers/dom_shims/index.js

+import './create_object_url';
 import './element_scroll_into_view';
 import './element_scroll_by';
 import './element_scroll_to';

spec/frontend/ide/components/new_dropdown/upload_spec.js

@@ -59,14 +59,11 @@ describe('new dropdown upload', () => {
       result: 'base64,cGxhaW4gdGV4dA==',
     };
     const binaryTarget = {
-      result: 'base64,w4I=',
+      result: 'base64,8PDw8A==', // ðððð
     };
-    const textFile = new File(['plain text'], 'textFile');
 
-    const binaryFile = {
-      name: 'binaryFile',
-      type: 'image/png',
-    };
+    const textFile = new File(['plain text'], 'textFile');
+    const binaryFile = new File(['😺'], 'binaryFile');
 
     beforeEach(() => {
       jest.spyOn(FileReader.prototype, 'readAsText');
@@ -92,16 +89,16 @@ describe('new dropdown upload', () => {
         .catch(done.fail);
     });
 
-    it('splits content on base64 if binary', () => {
+    it('creates a blob URL for the content if binary', () => {
       vm.createFile(binaryTarget, binaryFile);
 
-      expect(FileReader.prototype.readAsText).not.toHaveBeenCalledWith(textFile);
+      expect(FileReader.prototype.readAsText).not.toHaveBeenCalled();
 
       expect(vm.$emit).toHaveBeenCalledWith('create', {
         name: binaryFile.name,
         type: 'blob',
-        content: binaryTarget.result.split('base64,')[1],
-        rawPath: binaryTarget.result,
+        content: 'ðððð',
+        rawPath: 'blob:https://gitlab.com/048c7ac1-98de-4a37-ab1b-0206d0ea7e1b',
       });
     });
   });

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

@@ -291,6 +291,20 @@ describe('IDE store file actions', () => {
           expect(store.state.openFiles[0].name).toBe(localFile.name);
         });
       });
+
+      it('does not toggle loading if toggleLoading=false', () => {
+        expect(localFile.loading).toBe(false);
+
+        return store
+          .dispatch('getFileData', {
+            path: localFile.path,
+            makeFileActive: false,
+            toggleLoading: false,
+          })
+          .then(() => {
+            expect(localFile.loading).toBe(true);
+          });
+      });
     });
 
     describe('Re-named success', () => {

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

@@ -39,23 +39,37 @@ describe('IDE store file mutations', () => {
   });
 
   describe('TOGGLE_FILE_OPEN', () => {
-    beforeEach(() => {
+    it('adds into opened files', () => {
       mutations.TOGGLE_FILE_OPEN(localState, localFile.path);
-    });
 
-    it('adds into opened files', () => {
       expect(localFile.opened).toBeTruthy();
       expect(localState.openFiles.length).toBe(1);
     });
 
+    describe('if already open', () => {
       it('removes from opened files', () => {
         mutations.TOGGLE_FILE_OPEN(localState, localFile.path);
+        mutations.TOGGLE_FILE_OPEN(localState, localFile.path);
 
         expect(localFile.opened).toBeFalsy();
         expect(localState.openFiles.length).toBe(0);
       });
     });
 
+    it.each`
+      entry                                | loading
+      ${{ opened: false }}                 | ${true}
+      ${{ opened: false, tempFile: true }} | ${false}
+      ${{ opened: true }}                  | ${false}
+    `('for state: $entry, sets loading=$loading', ({ entry, loading }) => {
+      Object.assign(localFile, entry);
+
+      mutations.TOGGLE_FILE_OPEN(localState, localFile.path);
+
+      expect(localFile.loading).toBe(loading);
+    });
+  });
+
   describe('SET_FILE_DATA', () => {
     it('sets extra file data', () => {
       mutations.SET_FILE_DATA(localState, {

spec/frontend/ide/stores/utils_spec.js

@@ -46,7 +46,7 @@ describe('Multi-file store utils', () => {
             path: 'added',
             tempFile: true,
             content: 'new file content',
-            rawPath: 'data:image/png;base64,abc',
+            rawPath: 'blob:https://gitlab.com/048c7ac1-98de-4a37-ab1b-0206d0ea7e1b',
             lastCommitSha: '123456789',
           },
           { ...file('deletedFile'), path: 'deletedFile', deleted: true },
@@ -77,7 +77,8 @@ describe('Multi-file store utils', () => {
           {
             action: commitActionTypes.create,
             file_path: 'added',
-            content: 'new file content',
+            // atob("new file content")
+            content: 'bmV3IGZpbGUgY29udGVudA==',
             encoding: 'base64',
             last_commit_id: '123456789',
             previous_path: undefined,
@@ -117,7 +118,7 @@ describe('Multi-file store utils', () => {
             path: 'added',
             tempFile: true,
             content: 'new file content',
-            rawPath: 'data:image/png;base64,abc',
+            rawPath: 'blob:https://gitlab.com/048c7ac1-98de-4a37-ab1b-0206d0ea7e1b',
             lastCommitSha: '123456789',
           },
         ],
@@ -148,7 +149,8 @@ describe('Multi-file store utils', () => {
           {
             action: commitActionTypes.create,
             file_path: 'added',
-            content: 'new file content',
+            // atob("new file content")
+            content: 'bmV3IGZpbGUgY29udGVudA==',
             encoding: 'base64',
             last_commit_id: '123456789',
             previous_path: undefined,

spec/frontend/lib/utils/url_utility_spec.js

@@ -509,6 +509,20 @@ describe('URL utility', () => {
     });
   });
 
+  describe('isBlobUrl', () => {
+    it.each`
+      url                               | valid
+      ${undefined}                      | ${false}
+      ${'blob:http://gitlab.com/abcd'}  | ${true}
+      ${'data:image/png;base64,abcdef'} | ${false}
+      ${'notaurl'}                      | ${false}
+      ${'../relative_url'}              | ${false}
+      ${'<a></a>'}                      | ${false}
+    `('returns $valid for $url', ({ url, valid }) => {
+      expect(urlUtils.isBlobUrl(url)).toBe(valid);
+    });
+  });
+
   describe('relativePathToAbsolute', () => {
     it.each`
       path                       | base                                  | result

spec/frontend/monitoring/components/dashboard_panel_spec.js

@@ -38,8 +38,6 @@ import MonitorStackedColumnChart from '~/monitoring/components/charts/stacked_co
 import { createStore, monitoringDashboard } from '~/monitoring/stores';
 import { createStore as createEmbedGroupStore } from '~/monitoring/stores/embed_group';
 
-global.URL.createObjectURL = jest.fn();
-
 const mocks = {
   $toast: {
     show: jest.fn(),
@@ -94,6 +92,8 @@ describe('Dashboard Panel', () => {
     state = store.state.monitoringDashboard;
 
     axiosMock = new AxiosMockAdapter(axios);
+
+    jest.spyOn(URL, 'createObjectURL');
   });
 
   afterEach(() => {

spec/frontend/performance_bar/index_spec.js

@@ -9,7 +9,6 @@ describe('performance bar wrapper', () => {
   let vm;
 
   beforeEach(() => {
-    URL.createObjectURL = jest.fn();
     performance.getEntriesByType = jest.fn().mockReturnValue([]);
 
     // clear html so that elements from previous tests don't mess with this test