Properly track preflighted uploads on the server (#3133)

* Properly track preflighted uploads on the server

Fixes #3115.
Relates to #3004.
Partially fixes #2835.

* add test for #2965, #3115

assets/js/phoenix_live_view/live_uploader.js

@@ -70,7 +70,7 @@ export default class LiveUploader {
   static trackFiles(inputEl, files, dataTransfer){
     if(inputEl.getAttribute("multiple") !== null){
       let newFiles = files.filter(file => !this.activeFiles(inputEl).find(f => Object.is(f, file)))
-      DOM.putPrivate(inputEl, "files", this.activeFiles(inputEl).concat(newFiles))
+      DOM.updatePrivate(inputEl, "files", [], (existing) => existing.concat(newFiles))
       inputEl.value = null
     } else {
       // Reset inputEl files to align output with programmatic changes (i.e. drag and drop)

assets/js/phoenix_live_view/upload_entry.js

@@ -15,10 +15,9 @@ import DOM from "./dom"
 export default class UploadEntry {
   static isActive(fileEl, file){
     let isNew = file._phxRef === undefined
-    let isPreflightInProgress = UploadEntry.isPreflightInProgress(file)
     let activeRefs = fileEl.getAttribute(PHX_ACTIVE_ENTRY_REFS).split(",")
     let isActive = activeRefs.indexOf(LiveUploader.genFileRef(file)) >= 0
-    return file.size > 0 && (isNew || isActive || !isPreflightInProgress)
+    return file.size > 0 && (isNew || isActive)
   }
 
   static isPreflighted(fileEl, file){

lib/phoenix_live_view/channel.ex

@@ -219,7 +219,8 @@ defmodule Phoenix.LiveView.Channel do
 
         {ok_or_error, reply, %Socket{} = new_socket} =
           with {:ok, new_socket} <- Upload.put_entries(socket, conf, entries, cid) do
-            Upload.generate_preflight_response(new_socket, conf.name, cid)
+            refs = Enum.map(entries, fn %{"ref" => ref} -> ref end)
+            Upload.generate_preflight_response(new_socket, conf.name, cid, refs)
           end
 
         new_upload_names =

lib/phoenix_live_view/upload.ex

@@ -337,16 +337,22 @@ defmodule Phoenix.LiveView.Upload do
   @doc """
   Generates a preflight response by calling the `:external` function.
   """
-  def generate_preflight_response(%Socket{} = socket, name, cid) do
+  def generate_preflight_response(%Socket{} = socket, name, cid, refs) do
     %UploadConfig{} = conf = Map.fetch!(socket.assigns.uploads, name)
 
+    # don't send more than max_entries preflight responses
+    refs = for {entry, i} <- Enum.with_index(conf.entries),
+      entry.ref in refs,
+      i < conf.max_entries && not entry.preflighted?,
+      do: entry.ref
+
     client_meta = %{
       max_file_size: conf.max_file_size,
       max_entries: conf.max_entries,
       chunk_size: conf.chunk_size
     }
 
-    {new_socket, new_conf, new_entries} = mark_preflighted(socket, conf)
+    {new_socket, new_conf, new_entries} = mark_preflighted(socket, conf, refs)
 
     case new_conf.external do
       false ->
@@ -357,8 +363,8 @@ defmodule Phoenix.LiveView.Upload do
     end
   end
 
-  defp mark_preflighted(socket, conf) do
-    {new_conf, new_entries} = UploadConfig.mark_preflighted(conf)
+  defp mark_preflighted(socket, conf, refs) do
+    {new_conf, new_entries} = UploadConfig.mark_preflighted(conf, refs)
     new_socket = update_uploads(new_conf, socket)
     {new_socket, new_conf, new_entries}
   end

lib/phoenix_live_view/upload_config.ex

@@ -357,23 +357,15 @@ defmodule Phoenix.LiveView.UploadConfig do
   end
 
   @doc false
-  def mark_preflighted(%UploadConfig{} = conf) do
-    refs_awaiting = refs_awaiting_preflight(conf)
-
+  def mark_preflighted(%UploadConfig{} = conf, refs) do
     new_entries =
       for entry <- conf.entries do
-        %UploadEntry{entry | preflighted?: entry.preflighted? || entry.ref in refs_awaiting}
+        %UploadEntry{entry | preflighted?: entry.preflighted? || entry.ref in refs}
       end
 
     new_conf = %UploadConfig{conf | entries: new_entries}
 
-    {new_conf, for(ref <- refs_awaiting, do: get_entry_by_ref(new_conf, ref))}
-  end
-
-  defp refs_awaiting_preflight(%UploadConfig{} = conf) do
-    for {entry, i} <- Enum.with_index(conf.entries),
-        i < conf.max_entries && not entry.preflighted?,
-        do: entry.ref
+    {new_conf, for(ref <- refs, do: get_entry_by_ref(new_conf, ref))}
   end
 
   @doc false

test/e2e/test_helper.exs

@@ -92,11 +92,12 @@ defmodule Phoenix.LiveViewTest.E2E.Router do
   end
 
   # these routes use a custom layout and therefore cannot be in the live_session
-  scope "/issues" do
+  scope "/issues", Phoenix.LiveViewTest.E2E do
     pipe_through(:browser)
 
-    live "/3047/a", Phoenix.LiveViewTest.E2E.Issue3047ALive
-    live "/3047/b", Phoenix.LiveViewTest.E2E.Issue3047BLive
+    live "/2965", Issue2965Live
+    live "/3047/a", Issue3047ALive
+    live "/3047/b", Issue3047BLive
   end
 end
 

test/e2e/tests/issues/2965.spec.js

@@ -0,0 +1,30 @@
+const { test, expect } = require("@playwright/test");
+const { syncLV } = require("../../utils");
+const { randomBytes } = require("crypto");
+
+test("can upload files with custom chunk hook", async ({ page }) => {
+  await page.goto("/issues/2965");
+  await syncLV(page);
+
+  const files = [];
+  for (let i = 1; i <= 20; i++) {
+    files.push({
+      name: `file${i}.txt`,
+      mimeType: "text/plain",
+      // random 100 kb
+      buffer: randomBytes(100 * 1024),
+    });
+  }
+
+  await page.locator("#fileinput").setInputFiles(files);
+  await syncLV(page);
+
+  // wait for uploads to finish
+  for (let i = 0; i < 20; i++) {
+    const row = page.locator(`tbody tr`).nth(i);
+    await expect(row).toContainText(`file${i + 1}.txt`);
+    await expect(row.locator("progress")).toHaveAttribute("value", "100");
+  }
+
+  // all uploads are finished!
+});

test/e2e/tests/uploads.spec.js

@@ -179,3 +179,33 @@ test("auto upload", async ({ page }) => {
 
   await expect(page.locator("ul li")).toBeVisible();
 });
+
+test("issue 3115 - cancelled upload is not re-added", async ({ page }) => {
+  await page.goto("/upload");
+  await syncLV(page);
+
+  await page.locator("#upload-form input").setInputFiles([
+    {
+      name: "file.txt",
+      mimeType: "text/plain",
+      buffer: Buffer.from("this is a test")
+    }
+  ]);
+  await syncLV(page);
+  // cancel the file
+  await page.getByLabel("cancel").click();
+
+  // add other file
+  await page.locator("#upload-form input").setInputFiles([
+    {
+      name: "file.md",
+      mimeType: "text/markdown",
+      buffer: Buffer.from("## this is a markdown file")
+    }
+  ]);
+  await syncLV(page);
+  await page.getByRole("button", { name: "Upload" }).click();
+
+  // we should see one uploaded file in the list
+  await expect(page.locator("ul li")).toHaveCount(1);
+});