From 5d06e5449f2801d4471aa6f360ca6e4f638229bb Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 16:49:51 -0700 Subject: [PATCH 1/9] Etag the bubbles#show --- app/controllers/bubbles_controller.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/controllers/bubbles_controller.rb b/app/controllers/bubbles_controller.rb index 379ba1e1d..7ef4d276d 100644 --- a/app/controllers/bubbles_controller.rb +++ b/app/controllers/bubbles_controller.rb @@ -19,6 +19,7 @@ class BubblesController < ApplicationController end def show + fresh_when etag: @bubble end def edit From cd14fda395b89fba68ba771ec0088114b2f8ce75 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 16:50:05 -0700 Subject: [PATCH 2/9] Cache bubble partial on itself --- app/views/bubbles/_bubble.html.erb | 48 ++++++++++++++++-------------- 1 file changed, 25 insertions(+), 23 deletions(-) diff --git a/app/views/bubbles/_bubble.html.erb b/app/views/bubbles/_bubble.html.erb index c757ae6a9..49061b584 100644 --- a/app/views/bubbles/_bubble.html.erb +++ b/app/views/bubbles/_bubble.html.erb @@ -1,26 +1,28 @@ -
+<% cache bubble do %> +
-

- <%= turbo_frame_tag bubble, :edit do %> - <%= link_to bubble.title, edit_bucket_bubble_path(bubble.bucket, bubble), class: "txt-undecorated" %> +

+ <%= turbo_frame_tag bubble, :edit do %> + <%= link_to bubble.title, edit_bucket_bubble_path(bubble.bucket, bubble), class: "txt-undecorated" %> + <% end %> +

+ + + + + + + <%= link_to bucket_bubble_path(bubble.bucket, bubble), class: "bubble__link" do %> + <%= bubble.title %> <% end %> - - - - - - - <%= link_to bucket_bubble_path(bubble.bucket, bubble), class: "bubble__link" do %> - <%= bubble.title %> - <% end %> - - <%= render "bubbles/assignments", bubble: bubble %> - <%= render "bubbles/boosts", bubble: bubble %> - <%= render "bubbles/color", bubble: bubble %> - <%= render "bubbles/date", bubble: bubble %> - <%= render "bubbles/image", bubble: bubble %> - <%= render "bubbles/tags", bubble: bubble %> -
+ <%= render "bubbles/assignments", bubble: bubble %> + <%= render "bubbles/boosts", bubble: bubble %> + <%= render "bubbles/color", bubble: bubble %> + <%= render "bubbles/date", bubble: bubble %> + <%= render "bubbles/image", bubble: bubble %> + <%= render "bubbles/tags", bubble: bubble %> +
+<% end %> From 32d1792cb20902d78fb9aa400d3f46ab2b542691 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 16:50:19 -0700 Subject: [PATCH 3/9] Fetch bubbles index with multiget cache --- app/views/bubbles/index.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/bubbles/index.html.erb b/app/views/bubbles/index.html.erb index f2e05afc8..c6ae7517e 100644 --- a/app/views/bubbles/index.html.erb +++ b/app/views/bubbles/index.html.erb @@ -20,7 +20,7 @@
<% if @bubbles.any? %> - <%= render partial: "bubbles/bubble", collection: @bubbles.limit(10) %> + <%= render partial: "bubbles/bubble", collection: @bubbles.limit(10), cached: true %> <% else %>

Nothing here.

<% end %> From 942eb9fd645700001474c042a5583e450fa2e990 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 17:03:37 -0700 Subject: [PATCH 4/9] Extract explaining method And keep parent method at a consistent level of abstraction --- app/models/bubble/assignable.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/models/bubble/assignable.rb b/app/models/bubble/assignable.rb index db0b405eb..e43ef1e47 100644 --- a/app/models/bubble/assignable.rb +++ b/app/models/bubble/assignable.rb @@ -13,8 +13,13 @@ module Bubble::Assignable def assign(users, assigner: Current.user) transaction do - Assignment.insert_all Array(users).collect { |user| { assignee_id: user.id, assigner_id: assigner.id, bubble_id: id } } + Assignment.insert_all assignee_rows_from(users) track_event :assigned, assignee_ids: Array(users).map(&:id) end end + + private + def assignee_rows_from(users) + Array(users).collect { |user| { assignee_id: user.id, assigner_id: assigner.id, bubble_id: id } } + end end From 0bffcb8b86b67476775f8dde379a49e8af7f09d0 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 17:08:05 -0700 Subject: [PATCH 5/9] Actually need to do all work outside of transaction loop We can be more efficient as just as descriptive with an explaining variable --- app/models/bubble/assignable.rb | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/app/models/bubble/assignable.rb b/app/models/bubble/assignable.rb index e43ef1e47..d83c027ff 100644 --- a/app/models/bubble/assignable.rb +++ b/app/models/bubble/assignable.rb @@ -12,14 +12,11 @@ module Bubble::Assignable end def assign(users, assigner: Current.user) + assignee_rows = Array(users).collect { |user| { assignee_id: user.id, assigner_id: assigner.id, bubble_id: id } } + transaction do - Assignment.insert_all assignee_rows_from(users) - track_event :assigned, assignee_ids: Array(users).map(&:id) + Assignment.insert_all assignee_rows + track_event :assigned, assignee_ids: assignee_rows.pluck(:assignee_id) end end - - private - def assignee_rows_from(users) - Array(users).collect { |user| { assignee_id: user.id, assigner_id: assigner.id, bubble_id: id } } - end end From 1d404830f474c2db48878c34b66c831f0311adb9 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 17:42:06 -0700 Subject: [PATCH 6/9] Must touch to bust bubble caches --- app/models/comment.rb | 2 +- app/models/event.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/models/comment.rb b/app/models/comment.rb index ae1d8ef6f..b4a342e6b 100644 --- a/app/models/comment.rb +++ b/app/models/comment.rb @@ -1,7 +1,7 @@ class Comment < ApplicationRecord include Searchable - belongs_to :bubble + belongs_to :bubble, touch: true belongs_to :creator, class_name: "User", default: -> { Current.user } searchable_by :body, using: :comments_search_index diff --git a/app/models/event.rb b/app/models/event.rb index 76e60ff30..119d871ac 100644 --- a/app/models/event.rb +++ b/app/models/event.rb @@ -4,7 +4,7 @@ class Event < ApplicationRecord include Assignments belongs_to :creator, class_name: "User" - belongs_to :bubble + belongs_to :bubble, touch: true has_one :account, through: :creator From 716e14998fa9fc8762c998c5f67d865e0fbf7a51 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 17:46:36 -0700 Subject: [PATCH 7/9] Note that we need to keep all partials cacheable, so no dynamic path munging --- app/views/bubbles/list/_bubble.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/bubbles/list/_bubble.html.erb b/app/views/bubbles/list/_bubble.html.erb index 39752aba0..b384160c8 100644 --- a/app/views/bubbles/list/_bubble.html.erb +++ b/app/views/bubbles/list/_bubble.html.erb @@ -24,6 +24,7 @@ <% bubble.tags.each do |tag| %> + <%# FIXME: This is not going to work, this partial must be cacheable, so it can't mutate a changeable view filter live. Need a JS solution. %> <%= link_to "##{tag.title}", bucket_bubbles_path(bubble.bucket, view_filter_params.merge(tag_ids: tag.id)), style: "color: #{bubble.color}" %> <% end %> From 53afd0d93cb315a83e19955d1baf2bfd13a80c48 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 17:51:09 -0700 Subject: [PATCH 8/9] Note that Current.user cannot be referenced within partials that must be cached --- app/views/comments/_comment.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/comments/_comment.html.erb b/app/views/comments/_comment.html.erb index 0cd298ab3..7b310226d 100644 --- a/app/views/comments/_comment.html.erb +++ b/app/views/comments/_comment.html.erb @@ -1,3 +1,4 @@ +<%# FIXME: Can't use a Current.user reference in a partial that must be cached. Needs to convert to use Stimulus controller pulling from a meta current-id %> <%= tag.div id: dom_id(comment), class: [ "comment flex align-start full-width", { "comment--mine": Current.user == comment.creator } ] do %>
<%= avatar_tag comment.creator, loading: :lazy %> From cdcf2500c17f9721a777b651b9984f67431fbb07 Mon Sep 17 00:00:00 2001 From: David Heinemeier Hansson Date: Sat, 19 Oct 2024 18:31:21 -0700 Subject: [PATCH 9/9] Another spot where we need to move a Current.user check to a stimulus controller to cache --- app/views/accounts/users/_user.html.erb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/views/accounts/users/_user.html.erb b/app/views/accounts/users/_user.html.erb index 2a9823444..cb7855dd1 100644 --- a/app/views/accounts/users/_user.html.erb +++ b/app/views/accounts/users/_user.html.erb @@ -7,6 +7,7 @@ + <%# FIXME: Move this Current.user check to a stimulus controller that just checks for admin? or the like we so we can cache user list %> <%= button_to account_user_path(user), method: :delete, class: "btn btn--small btn--negative flex-item-no-shrink", disabled: !Current.user.can_remove?(user), data: { turbo_confirm: "Are you sure you want to permanently remove this person from the account?" } do %>