search

phoenixframework / phoenix_live_view

Rich, real-time user experiences with server-rendered HTML
https://hex.pm/packages/phoenix_live_view
MIT License
6.21k stars 930 forks source link

uploads with max_entries: 1 does not seem to auto upload every twice #2271

Closed ykato123 closed 1 year ago

ykato123 commented 2 years ago

Environment

Actual behavior

I tried allow_upload/3 with max_entries: 1, auto_upload: true but auto upload seems to be not working every two times(2,4,6,...).

With my heex code below, progress bar will move and complete soon in the first try. When I upload the second file, which should replace the former file, the progress bar stays 0. If I try uploading again, progress bar will move soon, but if I try another time, it doesn't move again. The progress bar moves when I submit the form, so I'm guessing it is something about auto uploading.

<form id="upload-form" phx-submit="save" phx-change="validate">
  <%= for entry <- @uploads.file.entries do %>
    <.live_img_preview entry={entry} />
    <progress value={entry.progress} max="100">
      <%= entry.progress %>%
    </progress>
  <% end %>
  <.live_file_input upload={@uploads.file} />
  <button type="submit">Upload</button>
</form>

Expected behavior

The progress bar should move and soon complete as the file is uploaded for the second time.

MullPointer commented 1 year ago

I also reproduced this issue:

Starting from what @Gazler found, I fell into a debugging rabbit hole. While #2443 does appear to fix the issue for me also, I do not think it fixes to the root of the problem (more details in a comment on that pull request).

Phoenix.LiveView.UploadConfig.put_entries/2 updates the upload config when new uploads arrive. When allow_upload has max_entries: 1 and the validation message is sent for a second upload, the existing upload is cancelled by calling Phoenix.LiveView.UploadConfig.maybe_replace_sole_entry/2, which calls cancel_entry in those circumstances. However, because the upload has started and has an upload channel associated, the update is not immediately removed from the UploadConfig, instead it is just marked with cancelled? field set to true. The actual removal of upload entries is triggered when the upload's Phoenix.LiveView.Channel stops, which triggers drop_entry for the upload in UploadConfig (see Phoenix.LiveView.Channel). This removal does not happen until after the processing of the validation message for the second upload is completed, so the response to that validation is as if the first upload is not cancelled, and an update to the page is sent with a too_many_files error, which prevents the upload from continuing to the next stage (as the error means the data-phx-auto-upload attribute is removed). However, the user never actually sees this error, as the dropping of the upload by the channel closing triggers another update to the page with the first upload removed and just the second upload active. But the upload has now missed the opportunity to be automatically started. On the third upload, the second upload gets immediately cancelled on validation because it never started and does not have an associated channel, so all works as intended.

The cleanest fix for this issue seems to me to change the behaviour of cancel_upload to fully remove the upload when it is called. I cannot see the need for the two stage remove process in live view's own code. The only advantage I can see is that it allows users to do some processing on the upload being replaced in their validate event handler, but this is not something I would expect and does not seem all that useful. I am submitting a pull request that makes this change, but I am not sure if it is the right approach. I think I may be missing some reason why this two stage approach is beneficial and was taken originally.

Alternatively, the behaviour of Phoenix.LiveView.UploadConfig.too_many_files?/1and maybe also Phoenix.Component.live_file_input/1 could be changed to ignore any uploads marked as cancelled and it be documented that the UploadConfig seen in the validate handler will include the automatically cancelled previous upload.

chrismccord commented 1 year ago

This should be fixed on main. Thanks!