From 51aa2a5e0e23c688cc1dc13c8adbc10331b6aae2 Mon Sep 17 00:00:00 2001
From: Sean Doyle <sean.p.doyle24@gmail.com>
Date: Wed, 18 Sep 2024 16:37:13 -0400
Subject: [PATCH] Ensure `turbo-stream[action="remove"]` does not render

Closes [#679][]

Prior to this change, calls to
`Turbo::Streams::Broadcasts#broadcast_remove_to` would render a
view partial that corresponds to the model *regardless* of whether or
not the contents of that render would be included in the broadcast
`<turbo-stream>` element.

Not only is it wasteful to render HTML and then do nothing with it, it
also poses the risk of failing due to errors raised during the rendering
process.

This commit restructures the contents of the
`Turbo::Streams::Broadcasts#broadcast_action_to` method to skip
extraneous rendering if `render: false` is passed. In addition, it
ensures that calls to `broadcast_remove_to` pass `render: false` by
default.

[#679]: https://github.com/hotwired/turbo-rails/issues/679
---
 app/channels/turbo/streams/broadcasts.rb      | 18 ++++++++++-----
 .../app/views/messages/_raises_error.html.erb |  1 +
 test/streams/broadcastable_test.rb            | 22 +++++++++++++++++++
 3 files changed, 36 insertions(+), 5 deletions(-)
 create mode 100644 test/dummy/app/views/messages/_raises_error.html.erb

diff --git a/app/channels/turbo/streams/broadcasts.rb b/app/channels/turbo/streams/broadcasts.rb
index ab7a9f04..a50c2070 100644
--- a/app/channels/turbo/streams/broadcasts.rb
+++ b/app/channels/turbo/streams/broadcasts.rb
@@ -6,7 +6,7 @@ module Turbo::Streams::Broadcasts
   include Turbo::Streams::ActionHelper
 
   def broadcast_remove_to(*streamables, **opts)
-    broadcast_action_to(*streamables, action: :remove, **opts)
+    broadcast_action_to(*streamables, action: :remove, render: false, **opts)
   end
 
   def broadcast_replace_to(*streamables, **opts)
@@ -38,10 +38,18 @@ def broadcast_refresh_to(*streamables, **opts)
   end
 
   def broadcast_action_to(*streamables, action:, target: nil, targets: nil, attributes: {}, **rendering)
-    broadcast_stream_to(*streamables, content: turbo_stream_action_tag(action, target: target, targets: targets, template:
-      rendering.delete(:content) || rendering.delete(:html) || (rendering[:render] != false && rendering.any? ? render_format(:html, **rendering) : nil),
-      **attributes
-    ))
+    content = rendering.delete(:content)
+    html = rendering.delete(:html)
+    render = rendering.delete(:render)
+
+    template =
+      if render == false
+        nil
+      else
+        content || html || (render_format(:html, **rendering) if rendering.present?)
+      end
+
+    broadcast_stream_to(*streamables, content: turbo_stream_action_tag(action, target: target, targets: targets, template: template, **attributes))
   end
 
   def broadcast_replace_later_to(*streamables, **opts)
diff --git a/test/dummy/app/views/messages/_raises_error.html.erb b/test/dummy/app/views/messages/_raises_error.html.erb
new file mode 100644
index 00000000..2aeb4950
--- /dev/null
+++ b/test/dummy/app/views/messages/_raises_error.html.erb
@@ -0,0 +1 @@
+<%= raise "Failed to render" %>
diff --git a/test/streams/broadcastable_test.rb b/test/streams/broadcastable_test.rb
index dbbb1d78..989d6585 100644
--- a/test/streams/broadcastable_test.rb
+++ b/test/streams/broadcastable_test.rb
@@ -5,6 +5,12 @@
 class Turbo::BroadcastableTest < ActionCable::Channel::TestCase
   include ActiveJob::TestHelper, Turbo::Streams::ActionHelper
 
+  class MessageThatRendersError < Message
+    def to_partial_path
+      "messages/raises_error"
+    end
+  end
+
   setup { @message = Message.new(id: 1, content: "Hello!") }
 
   test "broadcasting ignores blank streamables" do
@@ -37,6 +43,14 @@ class Turbo::BroadcastableTest < ActionCable::Channel::TestCase
     end
   end
 
+  test "broadcasting remove does not render contents" do
+    message = MessageThatRendersError.new(id: 1)
+
+    assert_broadcast_on message.to_gid_param, turbo_stream_action_tag("remove", target: dom_id(message)) do
+      message.broadcast_remove
+    end
+  end
+
   test "broadcasting replace to stream now" do
     assert_broadcast_on "stream", turbo_stream_action_tag("replace", target: "message_1", template: render(@message)) do
       @message.broadcast_replace_to "stream"
@@ -127,6 +141,14 @@ class Turbo::BroadcastableTest < ActionCable::Channel::TestCase
     end
   end
 
+  test "broadcasting refresh does not render contents" do
+    message = MessageThatRendersError.new(id: 1)
+
+    assert_broadcast_on message.to_gid_param, turbo_stream_action_tag("refresh") do
+      message.broadcast_refresh
+    end
+  end
+
   test "broadcasting refresh later is debounced" do
     assert_broadcast_on @message.to_gid_param, turbo_stream_refresh_tag do
       assert_broadcasts(@message.to_gid_param, 1) do