Uppy uploader breaking with multiple files drop onto composer

bug
1

Hello,
since the last update (we are on a managed environment) our upload functionality seems broken but we aren’t doing any magic.

We simply register a new api.addComposerUploadHandler() from within a theme component.
This used to work great with multiple files dragged to the composer. Now the there are errors thrown in the console which seem to be related to Uppy (why we don’t want).

Our code is really simple, but still, Uppy seems to interfere with it.

// Register custom upload handler for videos.
	api.addComposerUploadHandler(["mp4", "mov", "mkv", "avi", "m4v"], (file, editor) => {
		console.log("Handling upload for", file.name);
	})

Dropping 3 files (mkv, mov, mkv) on the composer shows the “your file is larger than 4 MB” error message which we wanted to bypass in the first place as we upload everything to Gdrive.

image
image802×205 7.15 KB

The chrome console throws those:

image
image1482×906 68 KB

Dropping a single file that is 50mb in size doesn’t cause that “file too large error” and our file is properly processed, as expected. So the error appears to be happening with multiple files and one file larger than a file limit of 4MB (I am not sure where this is set).

Thanks for any help on this. I think it is related to the last update of Discourse itself.

@martin

2

Hi @Sören_Geier. There have been some changes in this area recently, though I had tried to maintain parity with the existing upload handler in the composer. I just want to get a better idea of your use case. From what I can tell, even the non-uppy version of the composer uploads via api.addComposerUploadHandler only handles a single file at a time:

So if you drop multiple files, the normal flow would happen, and I guess in the case in the OP of this topic you are simply hitting the file size limits from the normal upload flow.

What used to happen, or what are you expecting to happen, when you drag and drop multiple files at once? It would be helpful to see your theme-component code, if you are not comfortable sharing it publicly you can PM me here with it on Meta.

So just to confirm you are hosted by us?

2 Likes
4

Thanks for the quick answer @martin .

Yes, we are hosting with you. So what usually happens when you drag a file into the composer is that it inserts some text into the composer ala "Processing "… Also, in the case of using API.addComposerUploadHandler([“mp4”, “mov”, “mkv”, “avi”, “m4v”]) this is done by Discourse before handing over the file to the custom handler here. That placeholder text inserting stopped working at some point, which was when I inserted the code myself in my handler:

composerController.model.appEvents.trigger("composer:insert-block", `[Processing: ${file.name}...]()`);

The next thing that broke was that our handler didn’t kick off because out of a sudden those video extensions disappeared from the “theme authorized extensions” setting - or I had to re-add them there for things to start working again.

Then I discovered the “multiple” file dropped problem as described earlier.

We had it working in a way that I was able to drop 2+ files without error messages coming up. And it also felt correct because we were bypassing all that Discourse validation logic.

Here are relevant code snippets:

Here I simply expect Discourse to hand me the dropped files. One after another.

// Register custom upload handler for videos.
	api.addComposerUploadHandler(["mp4", "mov", "mkv", "avi", "m4v"], (file, editor) => {
		console.log("Handling upload for", file.name);
		sendToGDrive(file, api);
	})

Because Discourse handed us the files all individually I created a middleman function that simply fills an array and after a timeout kicks of the actual uploader function. So I am collecting the passed file from Discourse into my own array.

// Collect all dropped files in sequence - as reported by Discourse handler.
function sendToGDrive(file, api) {
	clearTimeout(uploaderStartTimeout);
	filesHolder.push(file)
	const composerController = api.container.lookup("controller:composer");
	composerController.model.appEvents.trigger("composer:insert-block", `[Processing: ${file.name}...]()`);

	uploaderStartTimeout = setTimeout(function () {
		initFileSend(api);
	}, 300);
}

Then I upload every file individually to Gdrive.

// Handle every file individually.
async function initFileSend(api) {
	for (const file of filesHolder) {
		const content = await sendFileToGdrive(file, api, uploadFolderId);
	}
}

Problems observed:

  • “Multiple files” drops cause file size validation whereas single file drops don’t
1 Like
5

Thank you for this detailed report, and the associated code. I was already kind of on this line of thinking for the upload handlers; allowing each file matching the handler to go into a sort of queue or pool like you have done here and then uploading them all at once or passing them on to some other UI, because I did find that the 1 at a time limit odd. Though from what you are saying perhaps I misunderstood how the 1 at a time limit worked in the old composer upload handler.

What I am going to do is some local tests to see what the old non-uppy uploader did with multiple files via an upload handler with a simplified version of the function you provided in a theme component, then try to achieve parity between the new way and the old way, because it will be much more flexible than allowing only one file at a time anyway.

This may take a little time to fix, I will work on it today.

2 Likes
6

I wanted to just give a quick update; I confirmed that with the pre-uppy composer upload handlers, while the code checks if only one file is uploaded, this is not accurate because jQuery file uploader only sends one file through this code path at a time, even if you drop multiple files at once. This is in contrast to uppy, which processes the added files in groups. This single file at a time assumption is made in two other plugins that we have created which utilize api.addComposerUploadHandler so it appears to be a common issue.

As I said, I think the best way forward will be allowing this handler to process multiple files which can then be batched and sent somewhere else in a way that makes sense to the plugin / theme author. At the very least I can fix the assumption of the uppy upload handler that only one file can be sent at a time. Will post again here once I have another update.

2 Likes
7

A final update before (my) weekend. I’ve got this fix that should be merged early next week that will restore the “old” way of doing things from pre-uppy, but inside uppy. So your implementation will go back to working correctly after this:

However I will also be adding a subsequent PR that changes addComposerUploadHandler to send through multiple files to the handler callback in an array, which will remove the need for you to set up a queue and setTimeout callbacks to handle multiple files coming through. I think this is more correct anyway, and an overall improvement to the API.

So your handler will then turn into something like this:

// Register custom upload handler for videos.
api.addComposerUploadHandler(
  ["mp4", "mov", "mkv", "avi", "m4v"],
  (files, editor) => {
    console.log("Handling upload for", files.map((file) => file.name).join(", "));
    sendToGDrive(files, api);
  }
);
2 Likes
8

Excellent. Thank you for looking into it so quickly!
Have a great, deserved weekend :blush:

2 Likes
9

@Sören_Geier I just merged DEV: Send multiple files in batches to composer upload handlers when using uppy by martin-brennan · Pull Request #15124 · discourse/discourse · GitHub which changes uppy to send through multiple files at once to the upload handler; you will need to update your theme component now to handle this :slight_smile:

3 Likes
10

Thats great. Its not yet deployed is it?

11

Are you on our Standard hosting? If so, it should be deployed by now :slight_smile:

3 Likes
12

Okay, I got complaints coming in that stuff broke for ppl who wanted to upload.
I now looked into it and also had initial issues sending my file to GDRIVE as I just passed the file object. Turned out the file object was an uppy-wrapped representation of the binary file looking like this.

image
image868×513 103 KB

In order to actually access the native file object, I had to work with files[0].data. (maybe a breaking change?)

Before that change, the handler just passed the native file object. I could expect other ppl to experience breaking functionality with this change, I am not sure.

Got everything working now. Thanks so much for the quick turnaround and support!

3 Likes
13

Oh dear, you are right, I am not sure how I missed that when I did that recent refactor! :sweat: I will push a fix for it this morning, will not take too long.

Edit: Fix is here, should be deployed to our standard hosting in the next few hours.

3 Likes
14

Great, adjusted out code as well. I think the topic can be closed now. Thanks for the exceptional help @martin

1 Like
15

No problem! I made the mess it’s only right I should clean it up :sweat_smile:

1 Like