fix streams in sticky LV being reset when same ref (#3683)

* fix streams in sticky LV being reset when same ref

Fixes #3681.

Child LiveViews would use the same data-phx-stream-ref, so it could
happen that they were cleared unexpectedly because we did not always
check the owner of the stream element when pruning.

There was another issue at play though: because we used assign_new to
set the streams, nested LiveViews would copy a parent's streams, instead
of creating a fresh one. This would lead to mixed up streams in the dead
render.

* fix test on latest elixir

* add test

Author: SteffenDE

assets/js/phoenix_live_view/dom_patch.js

@@ -295,14 +295,8 @@ export default class DOMPatch {
       // clear stream items from the dead render if they are not inserted again
       if(isJoinPatch){
         DOM.all(this.container, `[${phxUpdate}=${PHX_STREAM}]`, el => {
-          // make sure to only remove elements owned by the current view
-          // see https://github.com/phoenixframework/phoenix_live_view/issues/3047
-          this.liveSocket.owner(el, (view) => {
-            if(view === this.view){
-              Array.from(el.children).forEach(child => {
-                this.removeStreamChildElement(child)
-              })
-            }
+          Array.from(el.children).forEach(child => {
+            this.removeStreamChildElement(child)
           })
         })
       }
@@ -359,6 +353,11 @@ export default class DOMPatch {
   }
 
   removeStreamChildElement(child){
+    // make sure to only remove elements owned by the current view
+    // see https://github.com/phoenixframework/phoenix_live_view/issues/3047
+    // and https://github.com/phoenixframework/phoenix_live_view/issues/3681
+    if(!this.view.ownsElement(child)){ return }
+
     // we need to store the node if it is actually re-added in the same patch
     // we do NOT want to execute phx-remove, we do NOT want to call onNodeDiscarded
     if(this.streamInserts[child.id]){

lib/phoenix_live_view.ex

@@ -1888,9 +1888,19 @@ defmodule Phoenix.LiveView do
   end
 
   defp ensure_streams(%Socket{} = socket) do
-    Phoenix.LiveView.Utils.assign_new(socket, :streams, fn ->
-      %{__ref__: 0, __changed__: MapSet.new(), __configured__: %{}}
-    end)
+    # don't use assign_new here because we DON'T want to copy parent streams
+    # during the dead render of nested or sticky LiveViews
+    case socket.assigns do
+      %{streams: _} ->
+        socket
+
+      _ ->
+        Phoenix.LiveView.Utils.assign(socket, :streams, %{
+          __ref__: 0,
+          __changed__: MapSet.new(),
+          __configured__: %{}
+        })
+    end
   end
 
   @doc """

test/e2e/support/issues/issue_3681.ex

@@ -0,0 +1,95 @@
+defmodule Phoenix.LiveViewTest.E2E.Issue3681Live do
+  # https://github.com/phoenixframework/phoenix_live_view/issues/3681
+
+  use Phoenix.LiveView, layout: {__MODULE__, :live}
+
+  alias Phoenix.LiveView.JS
+
+  def mount(_params, _session, socket) do
+    {:ok, socket}
+  end
+
+  def render("live.html", assigns) do
+    ~H"""
+    {apply(Phoenix.LiveViewTest.E2E.Layout, :render, [
+      "live.html",
+      Map.put(assigns, :inner_content, [])
+    ])}
+
+    {live_render(
+      @socket,
+      Phoenix.LiveViewTest.E2E.Issue3681.StickyLive,
+      id: "sticky",
+      sticky: true
+    )}
+
+    <hr />
+    {@inner_content}
+    <hr />
+    """
+  end
+
+  def render(assigns) do
+    ~H"""
+    <h3>A LiveView that does nothing but render it's layout.</h3>
+    <.link navigate="/issues/3681/away">Go to a different LV with a (funcky) stream</.link>
+    """
+  end
+end
+
+defmodule Phoenix.LiveViewTest.E2E.Issue3681.AwayLive do
+  use Phoenix.LiveView, layout: {Phoenix.LiveViewTest.E2E.Issue3681Live, :live}
+
+  alias Phoenix.LiveView.JS
+
+  def mount(_params, _session, socket) do
+    socket =
+      socket
+      |> stream(:messages, [])
+      # <--- This is the root cause
+      |> stream(:messages, [msg(4)], reset: true)
+
+    {:ok, socket}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <h3>A liveview with a stream configured twice</h3>
+    <h4>This causes the nested liveview in the layout above to be reset by the client.</h4>
+
+    <.link navigate="/issues/3681">Go back to (the now borked) LV without a stream</.link>
+    <h1>Normal Stream</h1>
+    <div id="msgs-normal" phx-update="stream">
+      <div :for={{dom_id, msg} <- @streams.messages} id={dom_id}>
+        <div>{msg.msg}</div>
+      </div>
+    </div>
+    """
+  end
+
+  defp msg(num) do
+    %{id: num, msg: num}
+  end
+end
+
+defmodule Phoenix.LiveViewTest.E2E.Issue3681.StickyLive do
+  use Phoenix.LiveView, layout: false
+
+  def mount(_params, _session, socket) do
+    {:ok, stream(socket, :messages, [msg(1), msg(2), msg(3)])}
+  end
+
+  def render(assigns) do
+    ~H"""
+    <div id="msgs-sticky" phx-update="stream">
+      <div :for={{dom_id, msg} <- @streams.messages} id={dom_id}>
+        <div>{msg.msg}</div>
+      </div>
+    </div>
+    """
+  end
+
+  defp msg(num) do
+    %{id: num, msg: num}
+  end
+end

test/e2e/test_helper.exs

@@ -191,6 +191,8 @@ defmodule Phoenix.LiveViewTest.E2E.Router do
       live "/3169", Issue3169Live
       live "/3530", Issue3530Live
       live "/3647", Issue3647Live
+      live "/3681", Issue3681Live
+      live "/3681/away", Issue3681.AwayLive
     end
   end
 

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

@@ -0,0 +1,22 @@
+const {test, expect} = require("../../test-fixtures")
+const {syncLV} = require("../../utils")
+
+// https://github.com/phoenixframework/phoenix_live_view/issues/3681
+test("streams in nested LiveViews are not reset when they share the same stream ref", async ({page, request}) => {
+// this was a separate bug where child LiveViews accidentally shared the parent streams
+  // check that the initial render does not contain the messages-4 element twice
+  expect((await (await request.get("/issues/3681/away")).text()).match(/messages-4/g).length).toBe(1)
+
+  await page.goto("/issues/3681")
+  await syncLV(page)
+
+  await expect(page.locator("#msgs-sticky > div")).toHaveCount(3)
+
+  await page.getByRole("link", {name: "Go to a different LV with a (funcky) stream"}).click()
+  await syncLV(page)
+  await expect(page.locator("#msgs-sticky > div")).toHaveCount(3)
+
+  await page.getByRole("link", {name: "Go back to (the now borked) LV without a stream"}).click()
+  await syncLV(page)
+  await expect(page.locator("#msgs-sticky > div")).toHaveCount(3)
+})