do not reorder stream items if they already exist (#3030)

* do not reorder stream items if they already exist

* add tests

* update documentation and changelog

CHANGELOG.md

@@ -9,7 +9,7 @@
   * Send `{:shutdown, :cancel}` to `handle_async/3` on `cancel_async`
   * Prevent events from child LiveViews from bubbling up to root LiveView when child is not mounted yet
   * Fix `phx-mounted` being called twice for stream items
-  * Do not move existing stream items when prepending
+  * Never move existing stream items if they already exist (use stream_delete and then stream_insert instead)
   * Fix live component rendering breaking when the server adds a component back that was pruned by the client (#3026)
   * Allow redirect from upload progress callback
 

assets/js/phoenix_live_view/dom_patch.js

@@ -149,7 +149,7 @@ export default class DOMPatch {
           let {ref, streamAt} = this.getStreamInsert(child)
           if(ref === undefined){ return parent.appendChild(child) }
 
-          DOM.putSticky(child, PHX_STREAM_REF, el => el.setAttribute(PHX_STREAM_REF, ref))
+          this.setStreamRef(child, ref)
 
           // we may need to restore skipped components, see removeStreamChildElement
           child.querySelectorAll(`[${PHX_MAGIC_ID}][${PHX_SKIP}]`).forEach(el => {
@@ -344,12 +344,21 @@ export default class DOMPatch {
     return insert || {}
   }
 
-  maybeReOrderStream(el, isNew){
-    let {ref, streamAt} = this.getStreamInsert(el)
-    if(streamAt === undefined || (streamAt === 0 && !isNew)){ return }
-
-    // we need to the PHX_STREAM_REF here as well as addChild is invoked only for parents
+  setStreamRef(el, ref){
     DOM.putSticky(el, PHX_STREAM_REF, el => el.setAttribute(PHX_STREAM_REF, ref))
+  }
+
+  maybeReOrderStream(el, isNew){
+    let {ref, streamAt, reset} = this.getStreamInsert(el)
+    if(streamAt === undefined){ return }
+
+    // we need to set the PHX_STREAM_REF here as well as addChild is invoked only for parents
+    this.setStreamRef(el, ref)
+
+    if(!reset && !isNew){
+      // we only reorder if the element is new or it's a stream reset
+      return
+    }
 
     if(streamAt === 0){
       el.parentElement.insertBefore(el, el.parentElement.firstElementChild)

lib/phoenix_live_view.ex

@@ -1728,12 +1728,11 @@ defmodule Phoenix.LiveView do
 
   ## Updating Items
 
-  As shown, an existing item on the client can be updated by issuing a `stream_insert` for
-  the existing item. When the client updates an existing item with an "append" operation
-  (passing the `at: -1` option), the item will remain in the same location as it was
-  previously, and will not be moved to the end of the parent children. To both update an
-  existing item and move it to the end of a collection, issue a `stream_delete`, followed
-  by a `stream_insert`. For example:
+  As shown, an existing item on the client can be updated by issuing a `stream_insert`
+  for the existing item. When the client updates an existing item, the item will remain
+  in the same location as it was previously, and will not be moved to the end of the
+  parent children. To both update an existing item and move it to another position,
+  issue a `stream_delete`, followed by a `stream_insert`. For example:
 
       song = get_song!(id)
 

lib/phoenix_live_view/test/dom.ex

@@ -474,7 +474,7 @@ defmodule Phoenix.LiveViewTest.DOM do
                   acc
 
                 # do not append existing child if already present, only update in place
-                current_index && insert_at == -1 && (existing? or appended?) ->
+                current_index && insert_at && existing? ->
                   if deleted? do
                     acc |> List.delete_at(current_index) |> List.insert_at(insert_at, child)
                   else

test/e2e/tests/streams.spec.js

@@ -191,9 +191,10 @@ test.describe("Issue #2656", () => {
   });
 });
 
-test.describe("Issue #2994", () => {
-  const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id));
+// helper function used below
+const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id));
 
+test.describe("Issue #2994", () => {
   test("can filter and reset a stream", async ({ page }) => {
     await page.goto("/stream/reset");
     await syncLV(page);
@@ -234,7 +235,7 @@ test.describe("Issue #2994", () => {
 
     await expect(await listItems(page)).toEqual(["items-b", "items-c", "items-d"]);
 
-    await page.getByRole("button", { name: "Prepend" }).click();
+    await page.getByRole("button", { name: "Prepend", exact: true }).click();
     await syncLV(page);
 
     await expect(await listItems(page)).toEqual([
@@ -249,7 +250,7 @@ test.describe("Issue #2994", () => {
 
     await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]);
 
-    await page.getByRole("button", { name: "Append" }).click();
+    await page.getByRole("button", { name: "Append", exact: true }).click();
     await syncLV(page);
 
     await expect(await listItems(page)).toEqual([
@@ -263,8 +264,6 @@ test.describe("Issue #2994", () => {
 });
 
 test.describe("Issue #2982", () => {
-  const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id));
-
   test("can reorder a stream with LiveComponents as direct stream children", async ({ page }) => {
     await page.goto("/stream/reset-lc");
     await syncLV(page);
@@ -279,8 +278,6 @@ test.describe("Issue #2982", () => {
 });
 
 test.describe("Issue #3023", () => {
-  const listItems = async (page) => page.locator("ul > li").evaluateAll(list => list.map(el => el.id));
-
   test("can bulk insert items at a specific index", async ({ page }) => {
     await page.goto("/stream/reset");
     await syncLV(page);
@@ -533,3 +530,40 @@ test.describe("stream limit - issue #2686", () => {
     ]);
   });
 });
+
+test("any stream insert for elements already in the DOM does not reorder", async ({ page }) => {
+  await page.goto("/stream/reset");
+  await syncLV(page);
+
+  await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]);
+
+  await page.getByRole("button", { name: "Prepend C" }).click();
+  await syncLV(page);
+  await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]);
+
+  await page.getByRole("button", { name: "Append C" }).click();
+  await syncLV(page);
+  await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]);
+
+  await page.getByRole("button", { name: "Insert C at 1" }).click();
+  await syncLV(page);
+  await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]);
+
+  await page.getByRole("button", { name: "Insert at 1", exact: true }).click();
+  await syncLV(page);
+  await expect(await listItems(page)).toEqual([
+    "items-a",
+    expect.stringMatching(/items-a-.*/),
+    "items-b",
+    "items-c",
+    "items-d"
+  ]);
+
+  await page.getByRole("button", { name: "Reset" }).click();
+  await syncLV(page);
+  await expect(await listItems(page)).toEqual(["items-a", "items-b", "items-c", "items-d"]);
+
+  await page.getByRole("button", { name: "Delete C and insert at 1" }).click();
+  await syncLV(page);
+  await expect(await listItems(page)).toEqual(["items-a", "items-c", "items-b", "items-d"]);
+});

test/phoenix_live_view/integrations/stream_test.exs

@@ -210,13 +210,13 @@ defmodule Phoenix.LiveView.StreamTest do
       html = assert lv |> element("button", "Filter") |> render_click()
       assert ids_in_ul_list(html) == ["items-b", "items-c", "items-d"]
 
-      html = assert lv |> element("button", "Prepend") |> render_click()
+      html = assert lv |> element(~s(button[phx-click="prepend"]), "Prepend") |> render_click()
       assert [<<"items-a-", _::binary>>, "items-b", "items-c", "items-d"] = ids_in_ul_list(html)
 
       html = assert lv |> element("button", "Reset") |> render_click()
       assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"]
 
-      html = assert lv |> element("button", "Append") |> render_click()
+      html = assert lv |> element(~s(button[phx-click="append"]), "Append") |> render_click()
 
       assert ["items-a", "items-b", "items-c", "items-d", <<"items-a-", _::binary>>] =
                ids_in_ul_list(html)
@@ -328,6 +328,30 @@ defmodule Phoenix.LiveView.StreamTest do
     assert ids_in_ul_list(html) == ["items-a", "items-e", "items-f", "items-g", "items-b", "items-c", "items-d"]
   end
 
+  test "any stream insert for elements already in the DOM does not reorder", %{conn: conn} do
+    {:ok, lv, html} = live(conn, "/stream/reset")
+
+    assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"]
+
+    html = assert lv |> element("button", "Prepend C") |> render_click()
+    assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"]
+
+    html = assert lv |> element("button", "Append C") |> render_click()
+    assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"]
+
+    html = assert lv |> element("button", "Insert C at 1") |> render_click()
+    assert ids_in_ul_list(html) == ["items-a", "items-b", "items-c", "items-d"]
+
+    html = assert lv |> element("button", "Insert at 1") |> render_click()
+    assert ["items-a", _, "items-b", "items-c", "items-d"] = ids_in_ul_list(html)
+
+    html = assert lv |> element("button", "Reset") |> render_click()
+    assert ["items-a", "items-b", "items-c", "items-d"] = ids_in_ul_list(html)
+
+    html = assert lv |> element("button", "Delete C and insert at 1") |> render_click()
+    assert ["items-a", "items-c", "items-b", "items-d"] = ids_in_ul_list(html)
+  end
+
   test "stream raises when attempting to consume ahead of for", %{conn: conn} do
     {:ok, lv, _html} = live(conn, "/stream")
 

test/support/live_views/streams.ex

@@ -117,7 +117,10 @@ defmodule Phoenix.LiveViewTest.StreamLive do
   end
 
   def handle_event("admin-move-to-first", %{"id" => "admins-" <> id}, socket) do
-    {:noreply, stream_insert(socket, :admins, user(id, "updated"), at: 0)}
+    {:noreply,
+     socket
+     |> stream_delete_by_dom_id(:admins, "admins-" <> id)
+     |> stream_insert(:admins, user(id, "updated"), at: 0)}
   end
 
   def handle_event("admin-move-to-last", %{"id" => "admins-" <> id = dom_id}, socket) do
@@ -294,6 +297,11 @@ defmodule Phoenix.LiveViewTest.StreamResetLive do
     <button phx-click="prepend">Prepend</button>
     <button phx-click="append">Append</button>
     <button phx-click="bulk-insert">Bulk insert</button>
+    <button phx-click="insert-at-one">Insert at 1</button>
+    <button phx-click="insert-existing-at-one">Insert C at 1</button>
+    <button phx-click="delete-insert-existing-at-one">Delete C and insert at 1</button>
+    <button phx-click="prepend-existing">Prepend C</button>
+    <button phx-click="append-existing">Append C</button>
     """
   end
 
@@ -374,6 +382,57 @@ defmodule Phoenix.LiveViewTest.StreamResetLive do
        at: 1
      )}
   end
+
+  def handle_event("insert-at-one", _, socket) do
+    {:noreply,
+     stream_insert(
+       socket,
+       :items,
+       %{id: "a" <> "#{System.unique_integer()}", name: "#{System.unique_integer()}"},
+       at: 1
+     )}
+  end
+
+  def handle_event("insert-existing-at-one", _, socket) do
+    {:noreply,
+     stream_insert(
+       socket,
+       :items,
+       %{id: "c", name: "C"},
+       at: 1
+     )}
+  end
+
+  def handle_event("delete-insert-existing-at-one", _, socket) do
+    {:noreply,
+     socket
+     |> stream_delete_by_dom_id(:items, "items-c")
+     |> stream_insert(
+       :items,
+       %{id: "c", name: "C"},
+       at: 1
+     )}
+  end
+
+  def handle_event("prepend-existing", _, socket) do
+    {:noreply,
+     stream_insert(
+       socket,
+       :items,
+       %{id: "c", name: "C"},
+       at: 0
+     )}
+  end
+
+  def handle_event("append-existing", _, socket) do
+    {:noreply,
+     stream_insert(
+       socket,
+       :items,
+       %{id: "c", name: "C"},
+       at: -1
+     )}
+  end
 end
 
 defmodule Phoenix.LiveViewTest.StreamResetLCLive do