FIX: Video thumbnail uploads interfering with subsequent uploads (#23216)

Short answer -- the problem is the video thumbnail generator & uploader
code added a couple of months back in f144c64e13.
It was implemented as another Mixin which overrides `this._uppyInstance`
when uploading the video thumbnail after the initial upload is complete,
which means the composer's `this._uppyInstance` value is overridden,
and it loses all of its preprocessors & upload code.

This is generally a problem with the Mixin based architecture that I
used for the Uppy code, which we need to remove at some point and
refacotr.

The most ideal thing to do here would be to convert this video thumbnail
code into an Uppy
[postprocessor](https://uppy.io/docs/uppy/#addpostprocessorfn) plugin,
which runs on each upload after they are complete. I started looking
into this, and the main hurdle here is adding support to tracking the
progress of postprocessors to
[ExtendableUploader](cf42466dea/app/assets/javascripts/discourse/app/mixins/extendable-uploader.js)
so that is out of scope at this time.

The fix here makes it so the ComposerVideoThumbnailUppy code is no
longer a Mixin, but acts more like a normal class, a pattern which
we have used in chat. I also clean up a lot of the thumbnail uploader
code and remove some unnecessary things.

Attempted to add a system spec, but video streaming does not work
in Chrome for Testing at this time, and it is needed for the
onloadedmetadata event.

app/assets/javascripts/discourse/app/mixins/composer-upload-uppy.js

@@ -1,4 +1,5 @@
 import Mixin from "@ember/object/mixin";
+import { getOwner } from "@ember/application";
 import ExtendableUploader from "discourse/mixins/extendable-uploader";
 import EmberObject from "@ember/object";
 import UppyS3Multipart from "discourse/mixins/uppy-s3-multipart";
@@ -11,6 +12,7 @@ import { warn } from "@ember/debug";
 import I18n from "I18n";
 import getURL from "discourse-common/lib/get-url";
 import { clipboardHelpers } from "discourse/lib/utilities";
+import ComposerVideoThumbnailUppy from "discourse/mixins/composer-video-thumbnail-uppy";
 import { bind, observes, on } from "discourse-common/utils/decorators";
 import {
   bindFileInputChangeListener,
@@ -325,28 +327,32 @@ export default Mixin.create(ExtendableUploader, UppyS3Multipart, {
 
         cacheShortUploadUrl(upload.short_url, upload);
 
-        this._generateVideoThumbnail(file, upload.url, () => {
-          if (this.useUploadPlaceholders) {
+        new ComposerVideoThumbnailUppy(getOwner(this)).generateVideoThumbnail(
+          file,
+          upload.url,
+          () => {
+            if (this.useUploadPlaceholders) {
+              this.appEvents.trigger(
+                `${this.composerEventPrefix}:replace-text`,
+                this.placeholders[file.id].uploadPlaceholder.trim(),
+                markdown
+              );
+            }
+            this._resetUpload(file, { removePlaceholder: false });
             this.appEvents.trigger(
-              `${this.composerEventPrefix}:replace-text`,
-              this.placeholders[file.id].uploadPlaceholder.trim(),
-              markdown
+              `${this.composerEventPrefix}:upload-success`,
+              file.name,
+              upload
             );
-          }
-          this._resetUpload(file, { removePlaceholder: false });
-          this.appEvents.trigger(
-            `${this.composerEventPrefix}:upload-success`,
-            file.name,
-            upload
-          );
 
-          if (this.inProgressUploads.length === 0) {
-            this.appEvents.trigger(
-              `${this.composerEventPrefix}:all-uploads-complete`
-            );
-            this._reset();
+            if (this.inProgressUploads.length === 0) {
+              this.appEvents.trigger(
+                `${this.composerEventPrefix}:all-uploads-complete`
+              );
+              this._reset();
+            }
           }
-        });
+        );
       });
     });
 

app/assets/javascripts/discourse/app/mixins/composer-video-thumbnail-uppy.js

@@ -1,30 +1,52 @@
-import Mixin from "@ember/object/mixin";
-import ExtendableUploader from "discourse/mixins/extendable-uploader";
-import UppyS3Multipart from "discourse/mixins/uppy-s3-multipart";
+import { setOwner } from "@ember/application";
+import { isVideo } from "discourse/lib/uploads";
+import { tracked } from "@glimmer/tracking";
+import UppyUploadMixin from "discourse/mixins/uppy-upload";
+import EmberObject from "@ember/object";
 import Uppy from "@uppy/core";
-import DropTarget from "@uppy/drop-target";
-import XHRUpload from "@uppy/xhr-upload";
 import { warn } from "@ember/debug";
 import I18n from "I18n";
-import getURL from "discourse-common/lib/get-url";
-import { bind } from "discourse-common/utils/decorators";
 import { inject as service } from "@ember/service";
 
-export default Mixin.create(ExtendableUploader, UppyS3Multipart, {
-  dialog: service(),
-  uploadRootPath: "/uploads",
-  uploadTargetBound: false,
-  useUploadPlaceholders: true,
+// It is not ideal that this is a class extending a mixin, but in the case
+// where this is needed (a second background uppy uploader on a class that
+// already has an uppyInstance) then it is acceptable for now.
+//
+// Ideally, this would be refactored into an uppy postprocessor and support
+// for that would be added to the ExtendableUploader. Generally, we want to
+// move away from these Mixins in future.
+//
+// Video thumbnail is attached to the post/topic here:
+//
+// https://github.com/discourse/discourse/blob/110a3025dbf5c7205cec498c7d83dc258d994cfe/app/models/post.rb#L1013-L1035
+export default class ComposerVideoThumbnailUppy extends EmberObject.extend(
+  UppyUploadMixin
+) {
+  @service dialog;
+  @service siteSettings;
+  @service session;
 
-  @bind
-  _generateVideoThumbnail(videoFile, uploadUrl, callback) {
+  @tracked uploading;
+
+  uploadRootPath = "/uploads";
+  uploadTargetBound = false;
+  useUploadPlaceholders = true;
+
+  constructor(owner) {
+    super(...arguments);
+    setOwner(this, owner);
+  }
+
+  generateVideoThumbnail(videoFile, uploadUrl, callback) {
     if (!this.siteSettings.video_thumbnails_enabled) {
       return callback();
     }
-    if (videoFile.type.split("/")[0] !== "video") {
+
+    if (!isVideo(videoFile.name)) {
       return callback();
     }
-    let video = document.createElement("video");
+
+    const video = document.createElement("video");
     video.src = URL.createObjectURL(videoFile.data);
 
     // These attributes are needed for thumbnail generation on mobile.
@@ -40,8 +62,8 @@ export default Mixin.create(ExtendableUploader, UppyS3Multipart, {
     // Wait for the video element to load, otherwise the canvas will be empty.
     // iOS Safari prefers onloadedmetadata over oncanplay.
     video.onloadedmetadata = () => {
-      let canvas = document.createElement("canvas");
-      let ctx = canvas.getContext("2d");
+      const canvas = document.createElement("canvas");
+      const ctx = canvas.getContext("2d");
       canvas.width = video.videoWidth;
       canvas.height = video.videoHeight;
 
@@ -54,8 +76,8 @@ export default Mixin.create(ExtendableUploader, UppyS3Multipart, {
           this._uppyInstance = new Uppy({
             id: "video-thumbnail",
             meta: {
-              upload_type: `thumbnail`,
               videoSha1,
+              upload_type: "thumbnail",
             },
             autoProceed: true,
           });
@@ -69,14 +91,13 @@ export default Mixin.create(ExtendableUploader, UppyS3Multipart, {
           } else {
             this._useXHRUploads();
           }
-          this._uppyInstance.use(DropTarget, { target: this.element });
 
           this._uppyInstance.on("upload", () => {
-            this.set("uploading", true);
+            this.uploading = true;
           });
 
           this._uppyInstance.on("upload-success", () => {
-            this.set("uploading", false);
+            this.uploading = false;
             callback();
           });
 
@@ -88,13 +109,13 @@ export default Mixin.create(ExtendableUploader, UppyS3Multipart, {
 
             // eslint-disable-next-line no-console
             console.error(message);
-            this.set("uploading", false);
+            this.uploading = false;
             callback();
           });
 
           try {
             this._uppyInstance.addFile({
-              source: `${this.id} thumbnail`,
+              source: `${this.id}-video-thumbnail`,
               name: `${videoSha1}`,
               type: blob.type,
               data: blob,
@@ -107,19 +128,5 @@ export default Mixin.create(ExtendableUploader, UppyS3Multipart, {
         });
       }, 100);
     };
-  },
-
-  // This should be overridden in a child component if you need to
-  // hook into uppy events and be sure that everything is already
-  // set up for _uppyInstance.
-  _uppyReady() {},
-
-  _useXHRUploads() {
-    this._uppyInstance.use(XHRUpload, {
-      endpoint: getURL(`/uploads.json?client_id=${this.messageBus.clientId}`),
-      headers: {
-        "X-CSRF-Token": this.session.csrfToken,
-      },
-    });
-  },
-});
+  }
+}

spec/system/composer_uploads_spec.rb

@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+describe "Uploading files in the composer", type: :system, js: true do
+  fab!(:current_user) { Fabricate(:user) }
+
+  let(:modal) { PageObjects::Modals::Base.new }
+  let(:composer) { PageObjects::Components::Composer.new }
+  let(:topic) { PageObjects::Pages::Topic.new }
+
+  before { sign_in(current_user) }
+
+  it "uploads multiple files at once" do
+    sign_in(current_user)
+
+    visit "/new-topic"
+    expect(composer).to be_opened
+
+    file_path_1 = file_from_fixtures("logo.png", "images").path
+    file_path_2 = file_from_fixtures("logo.jpg", "images").path
+    attach_file([file_path_1, file_path_2]) { composer.click_toolbar_button("upload") }
+
+    expect(composer).to have_no_in_progress_uploads
+    expect(composer.preview).to have_css(".image-wrapper", count: 2)
+  end
+
+  it "allows cancelling uploads" do
+    sign_in(current_user)
+
+    visit "/new-topic"
+    expect(composer).to be_opened
+
+    page.driver.browser.network_conditions = { latency: 20_000 }
+
+    file_path_1 = file_from_fixtures("logo.png", "images").path
+    attach_file(file_path_1) { composer.click_toolbar_button("upload") }
+    expect(composer).to have_in_progress_uploads
+    find("#cancel-file-upload").click
+
+    expect(composer).to have_no_in_progress_uploads
+    expect(composer.preview).to have_no_css(".image-wrapper")
+  ensure
+    page.driver.browser.network_conditions = { latency: 0 }
+  end
+
+  context "when video thumbnails are enabled" do
+    before do
+      SiteSetting.video_thumbnails_enabled = true
+      SiteSetting.authorized_extensions += "|mp4"
+    end
+
+    # TODO (martin): Video streaming is not yet available in Chrome for Testing,
+    # we need to come back to this in a few months and try again.
+    #
+    # c.f. https://groups.google.com/g/chromedriver-users/c/1SMbByMfO2U
+    xit "generates a thumbnail for the video" do
+      sign_in(current_user)
+
+      visit "/new-topic"
+      expect(composer).to be_opened
+      topic.fill_in_composer_title("Video upload test")
+
+      file_path_1 = file_from_fixtures("small.mp4", "media").path
+      attach_file(file_path_1) { composer.click_toolbar_button("upload") }
+
+      expect(composer).to have_no_in_progress_uploads
+      expect(composer.preview).to have_css(".video-container")
+
+      composer.submit
+
+      expect(find("#topic-title")).to have_content("Video upload test")
+      expect(topic.image_upload_id).to eq(Upload.last.id)
+    end
+  end
+end

spec/system/page_objects/components/composer.rb

@@ -200,10 +200,22 @@ module PageObjects
         JS
       end
 
+      def submit
+        find("#{COMPOSER_ID} .save-or-cancel .create").click
+      end
+
       def close
         find("#{COMPOSER_ID} .save-or-cancel .cancel").click
       end
 
+      def has_no_in_progress_uploads?
+        find("#{COMPOSER_ID}").has_no_css?("#file-uploading")
+      end
+
+      def has_in_progress_uploads?
+        find("#{COMPOSER_ID}").has_css?("#file-uploading")
+      end
+
       private
 
       def emoji_preview_selector(emoji)

spec/system/s3_uploads_spec.rb

@@ -1,6 +1,6 @@
 # frozen_string_literal: true
 
-describe "Uploading files to S3", type: :system do
+describe "Uploading files in the composer to S3", type: :system do
   fab!(:current_user) { Fabricate(:admin) }
 
   let(:modal) { PageObjects::Modals::Base.new }