feat: Refactor checklist item rendering and caching; improve helper methods for better performance

Author: rsmithlal

app/controllers/better_together/checklist_items_controller.rb

@@ -6,6 +6,7 @@ class ChecklistItemsController < FriendlyResourceController # rubocop:todo Style
     before_action :checklist_item, only: %i[show edit update destroy position]
 
     helper_method :new_checklist_item
+    helper_method :checklist_items_for
 
     def create # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
       @checklist_item = new_checklist_item
@@ -94,9 +95,8 @@ def position # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
         format.turbo_stream do
           render turbo_stream: turbo_stream.replace(
             helpers.dom_id(@checklist, :checklist_items),
-            partial: 'better_together/checklist_items/checklist_item',
-            collection: @checklist.checklist_items.with_translations,
-            as: :checklist_item
+            partial: 'better_together/checklist_items/list',
+            locals: { checklist: @checklist }
           )
         end
       end
@@ -125,9 +125,8 @@ def reorder # rubocop:todo Metrics/AbcSize, Metrics/MethodLength
         format.turbo_stream do
           render turbo_stream: turbo_stream.replace(
             helpers.dom_id(@checklist, :checklist_items),
-            partial: 'better_together/checklist_items/checklist_item',
-            collection: @checklist.checklist_items.with_translations,
-            as: :checklist_item
+            partial: 'better_together/checklist_items/list',
+            locals: { checklist: @checklist }
           )
         end
       end
@@ -182,5 +181,27 @@ def resource_class
     def resource_collection
       resource_class.where(checklist: @checklist)
     end
+
+    # Returns a memoized relation (or array) of checklist items for a checklist and optional parent_id.
+    # Views should call this helper instead of building policy_scope queries inline so ordering and
+    # policy scoping remain consistent and memoized for a single request.
+    def checklist_items_for(checklist, parent_id: nil) # rubocop:disable Metrics/MethodLength
+      @__checklist_items_cache ||= {}
+      key = [checklist.id, parent_id]
+      return @__checklist_items_cache[key] if @__checklist_items_cache.key?(key)
+
+      scope = policy_scope(::BetterTogether::ChecklistItem)
+      scope = scope.where(checklist: checklist)
+      scope = if parent_id.nil?
+                scope.where(parent_id: nil)
+              else
+                scope.where(parent_id: parent_id)
+              end
+
+      # Ensure we enforce ordering by position regardless of any order applied by policy_scope
+      scope = scope.with_translations.reorder(:position)
+
+      @__checklist_items_cache[key] = scope
+    end
   end
 end

app/helpers/better_together/checklist_items_helper.rb

@@ -3,8 +3,25 @@
 module BetterTogether
   # Helper methods for rendering and formatting checklist items in views.
   #
-  # Provides small view helper utilities used by checklist-related templates.
+  # Provides view helper utilities used by checklist-related templates.
   module ChecklistItemsHelper
+    # Return a relation of checklist items scoped for the provided checklist and optional parent_id.
+    # This helper is available in views and mirrors the controller helper implementation; it
+    # ensures ordering by position and memoizes the relation per-request.
+    def checklist_items_for(checklist, parent_id: nil)
+      @__checklist_items_cache ||= {}
+      key = [checklist.id, parent_id]
+      return @__checklist_items_cache[key] if @__checklist_items_cache.key?(key)
+
+      scope = policy_scope(::BetterTogether::ChecklistItem)
+      scope = scope.where(checklist: checklist)
+      scope = parent_id.nil? ? scope.where(parent_id: nil) : scope.where(parent_id: parent_id)
+
+      scope = scope.with_translations.reorder(:position)
+
+      @__checklist_items_cache[key] = scope
+    end
+
     # Build an option title for a checklist item including depth-based prefix and slug.
     # Example: "— — Subitem label (subitem-slug)"
     def checklist_item_option_title(item)

app/views/better_together/checklist_items/_checklist_item.html.erb

@@ -86,8 +86,9 @@
   <%# Render children container (always present so Turbo can target it); CSS hides it when empty %>
   <% has_children = checklist_item.children.exists? %>
   <ul id="<%= dom_id(checklist_item, :children) %>" class="list-group ms-4 children_checklist_item <%= dom_class(checklist_item, :children) %>" data-has-children="<%= has_children ? 'true' : 'false' %>">
-    <% child_items = policy_scope(::BetterTogether::ChecklistItem).where(checklist: checklist_item.checklist, parent_id: checklist_item.id).with_translations.order(:position) %>
-    <%= render partial: 'better_together/checklist_items/checklist_item', collection: child_items, as: :checklist_item %>
+    <%# Prefer a provided `items` local (passed down by parent), otherwise use controller helper which memoizes policy_scope + reorder. Fallback to explicit policy_scope with reorder if helper is unavailable in this context. %>
+  <% children_items ||= checklist_items_for(checklist_item.checklist, parent_id: checklist_item.id) %>
+    <%= render partial: 'better_together/checklist_items/checklist_item', collection: children_items, as: :checklist_item %>
   </ul>
 </li>
 

app/views/better_together/checklist_items/_list.html.erb

@@ -1,7 +1,8 @@
 <%# app/views/better_together/checklist_items/_list.html.erb %>
 <div id="<%= dom_id(checklist, :checklist_items) %>" class="bt-list-fade" data-better_together--checklist-items-target="list" data-bt-debug="true" data-can-update="<%= policy(checklist).update? ? 'true' : 'false' %>">
   <ul class="list-group bt-checklist" role="list" aria-labelledby="checklist-items-list">
-  <% items = policy_scope(::BetterTogether::ChecklistItem).where(checklist: checklist, parent_id: nil).with_translations.order(:position) %>
+  <%# Use provided items local when available (e.g., from a parent partial), otherwise fallback to controller helper %>
+  <% items ||= checklist_items_for(checklist, parent_id: nil) %>
   <%= render partial: 'better_together/checklist_items/checklist_item', collection: items, as: :checklist_item %>
   </ul>
 </div>

app/views/better_together/checklist_items/_list_contents.html.erb

@@ -15,13 +15,17 @@
     end
 
     # create a new item without position - Positioned#set_position should set it to 5
-    new_item = create(:better_together_checklist_item, checklist: checklist, privacy: 'public',
-                                                       label: 'Appended Model Item')
+    # create a new item without a preset position so Positioned#set_position runs
+    new_item = build(:better_together_checklist_item, checklist: checklist, privacy: 'public',
+                                                      label: 'Appended Model Item')
+    # ensure factory default position (0) is not applied by setting to nil before save
+    new_item.position = nil
+    new_item.save!
 
     expect(new_item.position).to eq(5)
 
-    # ordering check
-    ordered = checklist.checklist_items.order(:position).pluck(:label, :position)
+    # ordering check (use Ruby accessors because label is translated and not a DB column)
+    ordered = checklist.checklist_items.order(:position).to_a.map { |ci| [ci.label, ci.position] }
     expect(ordered.last.first).to eq('Appended Model Item')
   end
 end