If you upload a CSV file with tags, the console will display an error after a successful upload.
Here is some digging.
The underlying issue is essentially that the modal closes too fast with the current logic.
The error happens in uppy-upload.js.uppy-upload) was already destroyed.
How is it possible? this._uppyInstance?.cancelAll();
For reference, _reset is called from _allUploadsComplete.
_reset() {
this._uppyInstance?.cancelAll();
this.setProperties({
uploading: false,
processing: false,
cancellable: false,
uploadProgress: 0,
filesAwaitingUpload: false,
});
On successful upload, this is the order of functions:uploadDone → _allUploadsComplete → [ _reset]
this._uppyInstance.on("upload-success", (file, response) => {
if (this.usingS3Uploads) {
this.setProperties({ uploading: false, processing: true });
this._completeExternalUpload(file)
.then((completeResponse) => {
this._removeInProgressUpload(file.id);
this.appEvents.trigger(
`upload-mixin:${this.id}:upload-success`,
file.name,
completeResponse
);
this.uploadDone(
deepMerge(completeResponse, { file_name: file.name })
);
this._triggerInProgressUploadsEvent();
if (this.inProgressUploads.length === 0) {
this._allUploadsComplete();
When uploadDone is called, the modal is closed right away.uppy-upload element will be destroyed at the end of the frame.https://github.com/discourse/discourse/blob/main/app/assets/javascripts/admin/addon/components/tags-uploader.js#L22-L26
Back to this._uppyInstance?.cancelAll();. This will trigger the event below.run(), the properties will be set after the element is destroyed.
this._uppyInstance.on("file-removed", (file, reason) => {
run(() => {
// we handle the cancel-all event specifically, so no need
// to do anything here. this event is also fired when some files
// are handled by an upload handler
if (reason === "cancel-all") {
return;
}
this.appEvents.trigger(
`upload-mixin:${this.id}:upload-cancelled`,
file.id
);
});
});
This is a minor regression. Introduced here:
uppy-upload.js
main ← dev/uppy-upload-mixin-improvements
merged 02:59AM - 07 Apr 22 UTC
This PR brings the `UppyUploadMixin` more into line with the `ComposerUppyUpload… ` mixin, by extending the `ExtendableUploader` . This also adds better tracking of and events for in progress uploads in the `UppyUploadMixin` for better UI interactions, and also opens up the use of `_useUploadPlugin` for the mixin, so anything implementing `UppyUploadMixin` can add extra uppy preprocessor plugins as needed.
This has been done as part of work on extracting uploads out of the chat composer. In future, we might be able to do the same for `ComposerUppyUpload`, getting rid of that mixin to standardise on `UppyUploadMixin` and have a separate `composer-uploads` component that lives alongside `composer-editor` like what we are doing in https://github.com/discourse/discourse-chat/pull/764
main ← issue/improve-starting-new-uploads-progress
merged 01:01AM - 30 Sep 22 UTC
This commit addresses issues around starting new uploads in a composer etc. when… one or more uploads are already processing or uploading. There were a couple of issues:
1. When all preprocessors were complete, we were not resetting `completeProcessing` to 0, which meant that `needProcessing` would never match `completeProcessing` if a new upload was started.
2. We were relying on the uppy "complete" event which is supposed to fire when all uploads are complete, but this doesn't seem to take into account new uploads that are added. Instead now we can rely on our own `inProgressUploads` tracker, and consider all uploads complete when there are no `inProgressUploads` in flight
This is difficult to test for in JS since it involves the upload lifecycle and AJAX calls, so tests are omitted.
tags-upload.js
main ← a11y-refactor-bootbox-alerts
merged 06:47PM - 27 Sep 22 UTC
Possible working solutions:
Closing the modal a little later
uploadDone() {
this.refresh();
this.dialog
.alert(I18n.t("tagging.upload_successful"))
.finally(() => this.closeModal());
}
Moving the check out of the run() loop.
this._uppyInstance.on("file-removed", (file, reason) => {
// we handle the cancel-all event specifically, so no need
// to do anything here. this event is also fired when some files
// are handled by an upload handler
if (reason === "cancel-all") {
return;
}
run(() => {
I’m not sure if there is a better solution. So, I’m posting here.
