brendon / acts_as_list

An ActiveRecord plugin for managing lists.
http://brendon.github.io/acts_as_list/
MIT License
2.05k stars 356 forks source link

Shifted records are not broadcast by Turbo Streams #405

Closed garethbradley closed 1 year ago

garethbradley commented 1 year ago

Hey,

I'm seeing an issue whilst using acts_as_list with Turbo Streams (Rails 7). The inserted record is broadcast as expected, but the "shifted" records (whilst updated) are not broadcast.

To help add context, I'm writing an app that has a Scenario, and each Scenario has many TimelineItems. The timeline items can be moved around to reorder them.

I wonder if it's the "Update All" that's not triggering individual updates?

3.1.2 :008 > ti = Scenario.first.timeline_items.create({name: "1a", position: 2})
  Scenario Load (0.2ms)  SELECT "scenarios".* FROM "scenarios" ORDER BY "scenarios"."id" ASC LIMIT ?  [["LIMIT", 1]]
  TRANSACTION (0.1ms)  begin transaction
  TimelineItem Pluck (0.2ms)  SELECT "timeline_items"."id" FROM "timeline_items" WHERE "timeline_items"."scenario_id" = ? AND ("timeline_items"."position" >= 2) ORDER BY "timeline_items"."position" DESC  [["scenario_id", 1]]
  TimelineItem Update All (1.1ms)  UPDATE "timeline_items" SET "position" = ("timeline_items"."position" + 1), "updated_at" = '2022-11-15 16:29:19.960960' WHERE "timeline_items"."id" IN (SELECT "timeline_items"."id" FROM "timeline_items" WHERE "timeline_items"."scenario_id" = ? AND ("timeline_items"."position" >= 2) AND "timeline_items"."id" = ? ORDER BY "timeline_items"."position" DESC)  [["scenario_id", 1], ["id", 2]]
  TimelineItem Create (0.4ms)  INSERT INTO "timeline_items" ("scenario_id", "name", "description", "card_type", "position", "trigger", "triggerAt", "content", "created_at", "updated_at") VALUES (?, ?, ?, ?, ?, ?, ?, ?, ?, ?)  [["scenario_id", 1], ["name", "1a"], ["description", nil], ["card_type", "message"], ["position", 2], ["trigger", "click"], ["triggerAt", nil], ["content", "--- {}\n"], ["created_at", "2022-11-15 16:29:19.958904"], ["updated_at", "2022-11-15 16:29:19.958904"]]
  TRANSACTION (12.7ms)  commit transaction
Enqueued Turbo::Streams::ActionBroadcastJob (Job ID: b60fdad7-1f0f-4f55-b884-a7a16f3be2ca) to Async(default) with arguments: "Z2lkOi8vbHVybml0L1NjZW5hcmlvLzE", {:action=>:append, :target=>"timeline_items", :targets=>nil, :locals=>{:timeline_item=>#<GlobalID:0x0000000105d643a8 @uri=#<URI::GID gid://lurnit/TimelineItem/3>>}, :partial=>"timeline_items/timeline_item"}
 =>
#<TimelineItem:0x0000000105b9c688
...
3.1.2 :009 >   TimelineItem Load (0.1ms)  SELECT "timeline_items".* FROM "timeline_items" WHERE "timeline_items"."id" = ? LIMIT ?  [["id", 3], ["LIMIT", 1]]
Performing Turbo::Streams::ActionBroadcastJob (Job ID: b60fdad7-1f0f-4f55-b884-a7a16f3be2ca) from Async(default) enqueued at 2022-11-15T16:29:19Z with arguments: "Z2lkOi8vbHVybml0L1NjZW5hcmlvLzE", {:action=>:append, :target=>"timeline_items", :targets=>nil, :locals=>{:timeline_item=>#<GlobalID:0x0000000105e9fc90 @uri=#<URI::GID gid://lurnit/TimelineItem/3>>}, :partial=>"timeline_items/timeline_item"}
  Rendered timeline_items/_triggerAtLabel.html.erb (Duration: 0.9ms | Allocations: 256)
  Rendered timeline_items/_message.html.erb (Duration: 2.3ms | Allocations: 573)
  Rendered scenarios/_add_timeline_item_divider.html.erb (Duration: 1.0ms | Allocations: 159)
  Rendered timeline_items/_timeline_item.html.erb (Duration: 5.1ms | Allocations: 1238)
[ActionCable] Broadcasting to Z2lkOi8vbHVybml0L1NjZW5hcmlvLzE: "<turbo-stream action=\"append\" target=\"timeline_items\"><template><div id=\"timeline_item_3\" style=\"order: 2\">\n    \n    <div class=\"timeline_item\">\n        <div class=\"md:grid md:grid-cols-3 md:gap-6\">\n    <div class=\"md:col-span-1\">\n        <div class=\"px-4 sm:px-0\">\n        ...
Performed Turbo::Streams::ActionBroadcastJob (Job ID: b60fdad7-1f0f-4f55-b884-a7a16f3be2ca) from Async(default) in 8.53ms
3.1.2 :009 >
3.1.2 :010 > exit
brendon commented 1 year ago

Hi @garethbradley, I've done a bit of work with Turbo Streams broadcasting and there really isn't a method for communicating the reordering of existing items. You can emulate it by removing the item from the list then inserting it where it needs to be. Or you can just re-render the entire list after a reorder.

You're right in that the update_all method just executes SQL so Rails doesn't know anything happened. You can actually broadcast from the controller instead of the model, and that's what I've settled on as I found the concept of broadcasting from the model too restrictive. It assumes a very simple use of your models, whereas you might have these things rendered differently all over the place. Your choice of course :D

Here's a sample of my update method for a collection of images:

def update
    respond_with image do |format|
      if image.update(image_params)
        format.turbo_stream
        format.json

        if image.saved_change_to_position?
          Turbo::StreamsChannel.broadcast_update_to online_newsletter, target: helpers.dom_id(article, :images),
            html: render_to_string(partial: 'online_newsletters/articles/image', collection: images, formats: [:html])
        else
          Turbo::StreamsChannel.broadcast_replace_to online_newsletter, target: image,
            html: render_to_string(partial: 'online_newsletters/articles/image')
        end
      else
        format.html { render 'edit', status: :unprocessable_entity }
        format.json { head :unprocessable_entity }
      end
    end
  end

I'm using the responders gem plus decent_exposure so ignore those parts but the key is that I'm detecting if I've moved the current item and if so I'm just re-rendering the entire list to everyone (in the updated order). I'm also not using acts_as_list for this reordering so you may have to detect the change in position slightly differently in your code.

Hope that helps! :)

garethbradley commented 1 year ago

You legend! Thanks so much for the nudge (actually, fairly large budge) in the right direction!

def create
      requires_full_refresh = (timeline_item_params[:position].to_i <= @scenario.timeline_items.count)
      @timeline_item = @scenario.timeline_items.create!(timeline_item_params)

      respond_to do |format|
        format.turbo_stream
        format.html { redirect_to @scenario, notice: "Timeline item was successfully created." }
        format.json do
          if (requires_full_refresh)
            @timeline_items = @scenario.timeline_items.order(position: :asc)

            puts "FULL REFRESH"

            Turbo::StreamsChannel.broadcast_update_to @scenario, target:  'timeline_items',
              html: render_to_string(partial: 'timeline_items/timeline_item', collection: @timeline_items, formats: [:html])
          else
            puts "NO REFRESH"

          end
          render json: @timeline_item
        end
      end
    end

Excuse the little debugging puts in there, but it's working a treat.

Many thanks

brendon commented 1 year ago

Nice @garethbradley, always happy to help. I moved away from acts_as_list for this mini-project using turbo streams and instead rolled my own ordering system: https://gist.github.com/brendon/d6cfd60cb5e70dc77a15a2476f04d279

I should turn that into a gem one day but for now it's there if you want to try it out. It tries to be way more strict on reordering within a transaction to avoid concurrency issues. Not sure if it's perfect though :)

garethbradley commented 1 year ago

I'll be sure to give it a test once I get a little traction on this project. But yes - release it! :D

Thanks again