Entries compounding/not clearing out when using LiveUpload from a hook

[tmjoen]

Environment

• Elixir version (elixir -v): 1.18.1
• Phoenix version (mix deps): 1.17.18
• Phoenix LiveView version (mix deps): 1.0.1
• Operating system: OSX
• Browsers you attempted to reproduce this bug on (the more the merrier): Chrome, Firefox
• Does the problem persist after removing "assets/node_modules" and trying again? Yes/no: Yes

Actual behavior

I'm building a queued uploader, where I'll only upload one file at a time. I stumbled into some weirdness, though. If I queue up 20 files, the first file gets consumed and removed from entries, but is readded again on the next upload. I first checked dispatchUploads on the LV JS side, and that seems correct — it only sends one file at a time. I then checked maybe_update_uploads in Phoenix.LiveView.Channel and here it seems to compound.

The first upload:

[debug] => maybe_update_uploads -- %{
  "phx-GBY54CJK-JzSgAFo" => [
    %{
      "last_modified" => 1704894902000,
      "name" => "1",
      "path" => "transformer",
      "ref" => "0",
      "relative_path" => "",
      "size" => 153600,
      "type" => ""
    }
  ]
}

The second upload:

[debug] => maybe_update_uploads -- %{
  "phx-GBY54CJK-JzSgAFo" => [
    %{
      "last_modified" => 1704894902000,
      "name" => "1",
      "path" => "transformer",
      "ref" => "0",
      "relative_path" => "",
      "size" => 153600,
      "type" => ""
    },
    %{
      "last_modified" => 1704894902000,
      "name" => "2",
      "path" => "transformer",
      "ref" => "1",
      "relative_path" => "",
      "size" => 153600,
      "type" => ""
    }
  ]
}

etc.

This means that the entries remaining in the uploaders config. Maybe they're not cleared out somewhere when using this.upload from a hook? I couldn't find out more, unfortunately..

Single file app here:

Application.put_env(:sample, Example.Endpoint,
  http: [ip: {127, 0, 0, 1}, port: 5001],
  server: true,
  live_view: [signing_salt: "aaaaaaaa"],
  secret_key_base: String.duplicate("a", 64),
  pubsub_server: Example.PubSub
)

require Logger

Mix.install([
  {:plug_cowboy, "~> 2.5"},
  {:jason, "~> 1.2"},
  {:phoenix, "~> 1.7.18"},
  {:phoenix_live_view, "~> 1.0.1"}
])

defmodule Example.ErrorView do
  def render(template, _), do: Phoenix.Controller.status_message_from_template(template)
end

defmodule Example.CoreComponents do
  use Phoenix.Component
  attr(:for, :any, required: true, doc: "the datastructure for the form")
  attr(:as, :any, default: nil, doc: "the server side parameter to collect all input under")

  attr(:rest, :global,
    include: ~w(autocomplete name rel action enctype method novalidate target multipart),
    doc: "the arbitrary HTML attributes to apply to the form tag"
  )

  slot(:inner_block, required: true)
  slot(:actions, doc: "the slot for form actions, such as a submit button")

  def simple_form(assigns) do
    ~H"""
    <.form :let={f} for={@for} as={@as} {@rest}>
      <div>
        <%= render_slot(@inner_block, f) %>
        <div :for={action <- @actions}>
          <%= render_slot(action, f) %>
        </div>
      </div>
    </.form>
    """
  end
end

defmodule Example.UploadLive do
  use Phoenix.LiveView, layout: {__MODULE__, :live}

  import Example.CoreComponents

  def mount(_params, _session, socket) do
    socket =
      socket
      |> allow_upload(:files,
        accept: :any,
        max_entries: 1500,
        max_file_size: 10_000_000_000,
        auto_upload: true,
        progress: &handle_progress/3
      )
      |> assign(:form, to_form(%{}))

    {:ok, socket}
  end

  defp phx_vsn, do: Application.spec(:phoenix, :vsn)
  defp lv_vsn, do: Application.spec(:phoenix_live_view, :vsn)

  def render("live.html", assigns) do
    ~H"""
    <script src={"https://cdn.jsdelivr.net/npm/phoenix@#{phx_vsn()}/priv/static/phoenix.min.js"}></script>
    <script src={"https://cdn.jsdelivr.net/npm/phoenix_live_view@#{lv_vsn()}/priv/static/phoenix_live_view.min.js"}></script>
    <script>
      const QueuedUploaderHook = {
        async mounted() {
          const maxConcurrency = 1
          let filesRemaining = []

          this.el.addEventListener('input', async (event) => {
            event.preventDefault()

            // try to wait a tick — without this delay, nothing happens,
            // it readies the first 3 files but doesn't upload them

            setTimeout(() => {
              if (event.target instanceof HTMLInputElement) {
                const files_html = event.target.files
                if (files_html) {
                  filesRemaining = Array.from(files_html)
                  const firstFiles = filesRemaining.slice(0, maxConcurrency)
                  console.log('firstFiles', firstFiles)
                  this.upload('files', firstFiles)

                  console.log('filesRemaining', filesRemaining)
                  filesRemaining.splice(0, maxConcurrency)
                  console.log('filesRemaining after splice', filesRemaining)
                }
              }
            }, 0)
          })

          this.handleEvent('upload_send_next_file', () => {
            console.log('Uploading more files! Remainder:', filesRemaining)

            if (filesRemaining.length > 0) {
              console.log('got MORE!')
              const nextFile = filesRemaining.shift()
              if (nextFile != undefined) {
                console.log('Uploading file: ', nextFile.name)
                this.upload('files', [nextFile])
              }
            } else {
              console.log('Done uploading, noop!')
            }
          })
        }
      };
      let liveSocket = new window.LiveView.LiveSocket("/live", window.Phoenix.Socket, {hooks: {QueuedUploaderHook}});
      liveSocket.connect();
    </script>

    <%= @inner_content %>
    """
  end

  def render(assigns) do
    ~H"""
    <main>
      <h1>Uploader reproduction</h1>
      <!-- moved outside of form like in the upload by hook example from mcrumm,
           prevents extra validate event -->
      <input
        id="fileinput"
        type="file"
        multiple
        phx-hook="QueuedUploaderHook"
        disabled={file_picker_disabled?(@uploads)}
      />
      <.simple_form for={@form} phx-submit="save" phx-change="validate">
        <section>
          <.live_file_input upload={@uploads.files} style="display: none;" />
          <h2 :if={length(@uploads.files.entries) > 0}>Currently uploading files</h2>
          <div>
            <table>
              <!-- head -->
              <thead>
                <tr>
                  <th>File Name</th>
                  <th>Progress</th>
                  <th>Cancel</th>
                  <th>Errors</th>
                </tr>
              </thead>
              <tbody>
                <%= for entry <- uploads_in_progress(@uploads) do %>
                  <tr>
                    <td><%= entry.client_name %></td>
                    <td>
                      <progress value={entry.progress} max="100">
                        <%= entry.progress %>%
                      </progress>
                    </td>

                    <td>
                      <button
                        type="button"
                        phx-click="cancel-upload"
                        phx-value-ref={entry.ref}
                        aria-label="cancel"
                      >
                        <span>&times;</span>
                      </button>
                    </td>
                    <td>
                      <%= for err <- upload_errors(@uploads.files, entry) do %>
                        <p style="color: red;"><%= error_to_string(err) %></p>
                      <% end %>
                    </td>
                  </tr>
                <% end %>
              </tbody>
            </table>
          </div>
          <%!-- Phoenix.Component.upload_errors/1 returns a list of error atoms --%>
          <%= for err <- upload_errors(@uploads.files) do %>
            <p style="text-red"><%= error_to_string(err) %></p>
          <% end %>
        </section>
      </.simple_form>
    </main>
    """
  end

  def handle_progress(:files, entry, socket) do
    if entry.done? do
      case consume_uploaded_entry(socket, entry, fn _meta -> {:ok, "ok"} end) do
        "ok" ->
          Logger.debug("=> consumed")
          {:noreply, push_event(socket, "upload_send_next_file", %{})}
      end
    else
      {:noreply, socket}
    end
  end

  def handle_event("validate", _params, socket) do
    {:noreply, socket}
  end

  def handle_event("cancel-upload", %{"ref" => ref}, socket) do
    {:noreply, cancel_upload(socket, :files, ref)}
  end

  def handle_event("save", _params, socket) do
    {:noreply, socket}
  end

  def error_to_string(:too_large), do: "Too large"
  def error_to_string(:not_accepted), do: "You have selected an unacceptable file type"
  def error_to_string(:s3_error), do: "Error on writing to cloudflare"

  def error_to_string(unknown) do
    IO.inspect(unknown, label: "Unknown error")
    "unknown error"
  end

  ## Helpers

  defp file_picker_disabled?(uploads) do
    Enum.any?(uploads.files.entries, fn e -> !e.done? end)
  end

  defp uploads_in_progress(uploads) do
    uploads.files.entries
  end
end

defmodule Example.Router do
  use Phoenix.Router
  import Phoenix.LiveView.Router

  pipeline :browser do
    plug(:accepts, ["html"])
  end

  scope "/", Example do
    pipe_through(:browser)

    live("/", UploadLive, :index)
  end
end

defmodule Example.Endpoint do
  use Phoenix.Endpoint, otp_app: :sample
  socket("/live", Phoenix.LiveView.Socket)
  plug(Example.Router)
end

{:ok, _} =
  Supervisor.start_link(
    [Example.Endpoint, {Phoenix.PubSub, [name: Example.PubSub, adapter: Phoenix.PubSub.PG2]}],
    strategy: :one_for_one
  )

Process.sleep(:infinity)

Expected behavior

Entries are cleared out when consumed and there isn't any compounding of upload entries when uploading through a hook.

[tmjoen]

I've checked most of the JS code to locate where the extra entries are sent without much luck. However I can inspect the WS connection and it looks to be in the validate events.

The first validate event looks OK:

[
  "4",
  "14",
  "lv:phx-GBZDkJIZVqWmUwCG",
  "event",
  {
    "type": "form",
    "event": "validate",
    "value": "_target=files",
    "uploads": {
      "phx-GBZDkKKTiWKJPwDG": [
        {
          "path": "files",
          "ref": "0",
          "last_modified": 1704894902000,
          "name": "2",
          "relative_path": "",
          "type": "",
          "size": 153600
        }
      ]
    }
  }
]

then the next validate event:

[
  "4",
  "25",
  "lv:phx-GBZDkJIZVqWmUwCG",
  "event",
  {
    "type": "form",
    "event": "validate",
    "value": "_target=files",
    "uploads": {
      "phx-GBZDkKKTiWKJPwDG": [
        {
          "path": "files",
          "ref": "0",
          "last_modified": 1704894902000,
          "name": "2",
          "relative_path": "",
          "type": "",
          "size": 153600
        },
        {
          "path": "files",
          "ref": "1",
          "last_modified": 1704894902000,
          "name": "3",
          "relative_path": "",
          "type": "",
          "size": 153600
        }
      ]
    }
  }
]

still carries the previous file

The third event shifts out the first file, but now carries the second and third file:

[
  "4",
  "36",
  "lv:phx-GBZDkJIZVqWmUwCG",
  "event",
  {
    "type": "form",
    "event": "validate_catalog",
    "value": "_target=transformer",
    "uploads": {
      "phx-GBZDkKKTiWKJPwDG": [
        {
          "path": "transformer",
          "ref": "1",
          "last_modified": 1704894902000,
          "name": "3",
          "relative_path": "",
          "type": "",
          "size": 153600
        },
        {
          "path": "transformer",
          "ref": "2",
          "last_modified": 1704894902000,
          "name": "4",
          "relative_path": "",
          "type": "",
          "size": 153600
        }
      ]
    }
  }
]

etc

[tmjoen]

So:

If I add a setTimeout to the upload_send_next_file event it works. The entries are cleared out.

this.handleEvent('upload_send_next_file', () => {
  console.log('Uploading more files! Remainder:', filesRemaining)

  // waiting a tick here to allow LV to reset the files(?)
  setTimeout(() => {
    if (filesRemaining.length > 0) {
      console.log('got MORE!')
      const nextFile = filesRemaining.shift()
      if (nextFile != undefined) {
        console.log('Uploading file: ', nextFile.name)
        this.upload('files', [nextFile])
      }
    } else {
      console.log('Done uploading, noop!')
    }
  }, 0)
})

Maybe this is to be expected?

[tmjoen]

Sorry for the noise / 🦆 —— last note: It also works with this.upload wrapped in a window.requestAnimationFrame, which I see is used in other places in the codebase. I could PR this change in the this.upload function, if it seems right? I'm a little unsure of where this function resides though... 😅

[SteffenDE]

@tmjoen I didn't look into this in detail yet, but it looks very similar to https://github.com/phoenixframework/phoenix_live_view/blob/main/test/e2e/support/issues/issue_2965.ex; do you know what's the difference?

[tmjoen]

@SteffenDE Thanks for looking! In the issue_2965 file it initially waits for the "upload_scrub_list" which will delay the time between the input event and this.upload in the same way as requestAnimationFrame I guess? Also maybe it's different with the noop writer?

[SteffenDE]

@tmjoen this is because currently events are dispatched before the push is acknowledged to the caller. We can fix this in LV by dispatching events in the next microtick.

diff --git a/assets/js/phoenix_live_view/view.js b/assets/js/phoenix_live_view/view.js
index 6b9fe4ce..c01986aa 100644
--- a/assets/js/phoenix_live_view/view.js
+++ b/assets/js/phoenix_live_view/view.js
@@ -669,7 +669,9 @@ export default class View {
       })
     }
 
-    this.liveSocket.dispatchEvents(events)
+    // dispatch events in the next microtick to allow file progress to be tracked
+    // before hook event handlers are invoked (#3610)
+    window.requestAnimationFrame(() => { this.liveSocket.dispatchEvents(events) })
     if(phxChildrenAdded){ this.joinNewChildren() }
   }

@chrismccord the pushFileProgress callback is only called when the pushWithReply promise resolves, but the (hook) events are dispatched earlier:

phoenix_live_view/assets/js/phoenix_live_view/upload_entry.js

Lines 61 to 64 in 38be0a1

	this.view.pushFileProgress(this.fileEl, this.ref, 100, () => {
	LiveUploader.untrackFile(this.fileEl, this.file)
	this._onDone()
	})

Let's see what Chris thinks :)