From 583f3c712f638739c6ef5e4500d88edb3bb10e34 Mon Sep 17 00:00:00 2001 From: Emi Matchu Date: Tue, 20 Feb 2024 18:42:42 -0800 Subject: [PATCH] High-level caching for closet lists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Okay, so I still don't know why rendering is just so slow (though migrating away from item translations did help!), but I can at least cache entire closet lists as a basic measure. That way, the first user to see the latest version of a closet list will still need just as much time to load it… but *only* the ones that have changed since last time (rather than always the full page), and then subsequent users get to reuse it too! Should help a lot for high-traffic lists, which incidentally are likely to be the big ones belonging to highly active traders! One big change we needed to make was to extract the `user-owns` and `user-wants` classes (which we use for trade matches for *the user viewing the list right now*) out of the cached HTML, and apply them after with Javascript instead. I always dislike moving stuff to JS, but the wins here seem. truly very very good, all things considered! --- .../javascripts/closet_hangers/index.js | 36 +++++++++- app/controllers/closet_hangers_controller.rb | 8 +-- app/helpers/closet_hangers_helper.rb | 26 ++++--- app/models/closet_hanger.rb | 2 +- .../closet_hangers/_closet_hanger.html.haml | 3 +- app/views/closet_hangers/index.html.haml | 71 +++++++++++-------- app/views/closet_lists/_closet_list.html.haml | 18 ++--- app/views/items/_item_link.html.haml | 4 +- 8 files changed, 113 insertions(+), 55 deletions(-) diff --git a/app/assets/javascripts/closet_hangers/index.js b/app/assets/javascripts/closet_hangers/index.js index fd4d3675..58283fa2 100644 --- a/app/assets/javascripts/closet_hangers/index.js +++ b/app/assets/javascripts/closet_hangers/index.js @@ -7,7 +7,11 @@ function hangersInit() { for (var i = 0; i < hangersInitCallbacks.length; i++) { - hangersInitCallbacks[i](); + try { + hangersInitCallbacks[i](); + } catch (error) { + console.error(error); + } } } @@ -58,6 +62,36 @@ hangersEl.toggleClass("comparing"); }); + // Read the item IDs of trade matches from the meta tags. + const ownedIds = + document + .querySelector("meta[name=trade-matches-owns]") + ?.getAttribute("value") + ?.split(",") ?? []; + const wantedIds = + document + .querySelector("meta[name=trade-matches-wants]") + ?.getAttribute("value") + ?.split(",") ?? []; + + // Apply the `user-owns` and `user-wants` classes to the relevant entries. + // This both provides immediate visual feedback, and sets up "Compare with + // Your Items" to toggle to just them! + // + // NOTE: The motivation here is caching: this allows us to share a cache of + // the closet list contents across all users, without `user-owns` or + // `user-wants` classes for one specific user getting cached and reused. + const hangerEls = document.querySelectorAll("#closet-hangers .object"); + for (const hangerEl of hangerEls) { + const itemId = hangerEl.getAttribute("data-item-id"); + if (ownedIds.includes(itemId)) { + hangerEl.classList.add("user-owns"); + } + if (wantedIds.includes(itemId)) { + hangerEl.classList.add("user-wants"); + } + } + /* Hanger forms diff --git a/app/controllers/closet_hangers_controller.rb b/app/controllers/closet_hangers_controller.rb index 9e9fbe64..dbf09f53 100644 --- a/app/controllers/closet_hangers_controller.rb +++ b/app/controllers/closet_hangers_controller.rb @@ -45,24 +45,24 @@ class ClosetHangersController < ApplicationController visible_groups = @user.closet_hangers_groups_visible_to(@perspective_user) @unlisted_closet_hangers_by_owned = find_unlisted_closet_hangers_by_owned(visible_groups) - items = [] + @items = [] @closet_lists_by_owned.each do |owned, lists| lists.each do |list| list.hangers.each do |hanger| - items << hanger.item + @items << hanger.item end end end @unlisted_closet_hangers_by_owned.each do |owned, hangers| hangers.each do |hanger| - items << hanger.item + @items << hanger.item end end if @public_perspective && user_signed_in? - current_user.assign_closeted_to_items!(items) + current_user.assign_closeted_to_items!(@items) end @campaign = Fundraising::Campaign.current diff --git a/app/helpers/closet_hangers_helper.rb b/app/helpers/closet_hangers_helper.rb index 110dda93..05a3e304 100644 --- a/app/helpers/closet_hangers_helper.rb +++ b/app/helpers/closet_hangers_helper.rb @@ -1,17 +1,11 @@ require 'cgi' +require 'digest' module ClosetHangersHelper def closet_hangers_help_class(user) 'hidden' unless user.closet_hangers.empty? end - def closet_hanger_partial_class(hanger) - 'object'.tap do |class_name| - class_name << ' user-owns' if hanger.item.owned? - class_name << ' user-wants' if hanger.item.wanted? - end - end - def send_neomail_url(neopets_username) "https://www.neopets.com/neomessages.phtml?type=send&recipient=#{CGI.escape neopets_username}" end @@ -124,8 +118,8 @@ module ClosetHangersHelper def render_closet_lists(lists) if lists - render :partial => 'closet_lists/closet_list', :collection => lists, - :locals => {:show_controls => !public_perspective?} + render partial: 'closet_lists/closet_list', collection: lists, + locals: {show_controls: !public_perspective?} end end @@ -139,6 +133,20 @@ module ClosetHangersHelper hangers = @unlisted_closet_hangers_by_owned[owned] hangers ? hangers.size : 0 end + + def unlisted_hangers_cache_key(owned) + # It'd be nice if this was a closet list like any other, instead of being a + # weird separate thing! Then we could use the same closet list cache key, + # in which adding/removing stuff in the list touches the simple updated_at. + # Instead, we encode the hashed list of hanger IDs and last updated_at into + # the key! (The digest is to prevent the cache key from getting obscenely + # large for long lists!) + hangers = @unlisted_closet_hangers_by_owned[owned] || [] + ids = hangers.map(&:id).join(",") + last_updated_at = hangers.map(&:updated_at).max + + ["owned=#{owned}", Digest::MD5.hexdigest(ids), last_updated_at.to_i] + end def closet_lists_group_name(subject, owned) ownership_key = owned ? 'owned_by' : 'wanted_by' diff --git a/app/models/closet_hanger.rb b/app/models/closet_hanger.rb index 000a12eb..1247589c 100644 --- a/app/models/closet_hanger.rb +++ b/app/models/closet_hanger.rb @@ -1,6 +1,6 @@ class ClosetHanger < ApplicationRecord belongs_to :item - belongs_to :list, class_name: 'ClosetList', optional: true + belongs_to :list, class_name: 'ClosetList', optional: true, touch: true belongs_to :user delegate :name, to: :item, prefix: true diff --git a/app/views/closet_hangers/_closet_hanger.html.haml b/app/views/closet_hangers/_closet_hanger.html.haml index ced215d2..6577b6f0 100644 --- a/app/views/closet_hangers/_closet_hanger.html.haml +++ b/app/views/closet_hangers/_closet_hanger.html.haml @@ -1,4 +1,5 @@ -%div{'class' => closet_hanger_partial_class(closet_hanger), 'data-item-id' => closet_hanger.item_id, 'data-quantity' => closet_hanger.quantity, 'data-id' => closet_hanger.id} +-# NOTE: Changing this requires bumping the cache at `_closet_list.html.haml`! +.object{'data-item-id' => closet_hanger.item_id, 'data-quantity' => closet_hanger.quantity, 'data-id' => closet_hanger.id} = render_item_link(closet_hanger.item) .quantity{:class => "quantity-#{closet_hanger.quantity}"} %span= closet_hanger.quantity diff --git a/app/views/closet_hangers/index.html.haml b/app/views/closet_hangers/index.html.haml index bd40f54d..151cbc1b 100644 --- a/app/views/closet_hangers/index.html.haml +++ b/app/views/closet_hangers/index.html.haml @@ -108,44 +108,44 @@ %h4= t '.unlisted.header' - if !@public_perspective = render partial: 'closet_lists/trading_neomail_warning', locals: {list: @user.null_closet_list(owned), user: @user} - .closet-list-content - .closet-list-hangers - = render_unlisted_closet_hangers(owned) - %span.empty-list= t '.unlisted.empty' + - cache unlisted_hangers_cache_key(owned) do + .closet-list-content + .closet-list-hangers + = render_unlisted_closet_hangers(owned) + %span.empty-list= t '.unlisted.empty' - if user_signed_in? - - localized_cache action_suffix: 'tmpls' do - %script#autocomplete-item-tmpl{type: 'text/x-jquery-tmpl'} - %a - = t '.autocomplete.add_item_html', item_name: '${item_name}' + %script#autocomplete-item-tmpl{type: 'text/x-jquery-tmpl'} + %a + = t '.autocomplete.add_item_html', item_name: '${item_name}' - %script#autocomplete-add-to-list-tmpl{type: 'text/x-jquery-tmpl'} - %a - = t '.autocomplete.add_to_list_html', list_name: '${list_name}' + %script#autocomplete-add-to-list-tmpl{type: 'text/x-jquery-tmpl'} + %a + = t '.autocomplete.add_to_list_html', list_name: '${list_name}' - %script#autocomplete-add-to-group-tmpl{type: 'text/x-jquery-tmpl'} - %a - = t '.autocomplete.add_to_group_html', group_name: '${group_name}' + %script#autocomplete-add-to-group-tmpl{type: 'text/x-jquery-tmpl'} + %a + = t '.autocomplete.add_to_group_html', group_name: '${group_name}' - %script#autocomplete-already-in-collection-tmpl{type: 'text/x-jquery-tmpl'} - %span - = t '.autocomplete.already_in_collection_html', - collection_name: '${collection_name}' + %script#autocomplete-already-in-collection-tmpl{type: 'text/x-jquery-tmpl'} + %span + = t '.autocomplete.already_in_collection_html', + collection_name: '${collection_name}' - -# Gotta do this weird subbing in the path, since the braces will be - -# escaped if they themselves are inserted. Probably deserves a more legit - -# method, especially if we ever need it again. - - templated_hanger_path = user_closet_hanger_path(user_id: '$0', id: '$1').sub('$0', '${user_id}').sub('$1', '${closet_hanger_id}') - %script#closet-hanger-update-tmpl{type: 'text/x-jquery-tmpl'} - = form_tag templated_hanger_path, method: :put, authenticity_token: false, class: 'closet-hanger-update' do - = hidden_field_tag 'closet_hanger[list_id]', '${list_id}' - = hidden_field_tag 'closet_hanger[owned]', '${owned}' - = number_field_tag 'closet_hanger[quantity]', '${quantity}', - min: 0, required: true + -# Gotta do this weird subbing in the path, since the braces will be + -# escaped if they themselves are inserted. Probably deserves a more legit + -# method, especially if we ever need it again. + - templated_hanger_path = user_closet_hanger_path(user_id: '$0', id: '$1').sub('$0', '${user_id}').sub('$1', '${closet_hanger_id}') + %script#closet-hanger-update-tmpl{type: 'text/x-jquery-tmpl'} + = form_tag templated_hanger_path, method: :put, authenticity_token: false, class: 'closet-hanger-update' do + = hidden_field_tag 'closet_hanger[list_id]', '${list_id}' + = hidden_field_tag 'closet_hanger[owned]', '${owned}' + = number_field_tag 'closet_hanger[quantity]', '${quantity}', + min: 0, required: true - %script#closet-hanger-destroy-tmpl{type: 'text/x-jquery-tmpl'} - -# TODO: remove me? + %script#closet-hanger-destroy-tmpl{type: 'text/x-jquery-tmpl'} + -# TODO: remove me? - content_for :stylesheets do = stylesheet_link_tag 'https://ajax.googleapis.com/ajax/libs/jqueryui/1.9.0/themes/south-street/jquery-ui.css' @@ -154,3 +154,14 @@ = include_javascript_libraries :jquery, :jquery_tmpl = javascript_include_tag 'ajax_auth', 'lib/jquery.ui', 'lib/jquery.jgrowl', 'closet_hangers/index' + +- if public_perspective? && user_signed_in? + - content_for :meta do + %meta{ + name: "trade-matches-owns", + value: @items.select(&:owned?).map(&:id).join(",") + } + %meta{ + name: "trade-matches-wants", + value: @items.select(&:wanted?).map(&:id).join(",") + } \ No newline at end of file diff --git a/app/views/closet_lists/_closet_list.html.haml b/app/views/closet_lists/_closet_list.html.haml index e739c1a1..a15af51a 100644 --- a/app/views/closet_lists/_closet_list.html.haml +++ b/app/views/closet_lists/_closet_list.html.haml @@ -18,13 +18,15 @@ - if show_controls = render partial: 'closet_lists/trading_neomail_warning', locals: {list: closet_list, user: @user} - .closet-list-content - - if closet_list.description? - = closet_list_description_format closet_list + -# Cachebuster comment: Updated downstream content at Feb 20 2024, 6:15pm + - cache closet_list do + .closet-list-content + - if closet_list.description? + = closet_list_description_format closet_list - .closet-list-hangers - - unless closet_list.hangers.empty? - = render_sorted_hangers(closet_list) - - %span.empty-list= t('.empty') + .closet-list-hangers + - unless closet_list.hangers.empty? + = render_sorted_hangers(closet_list) + + %span.empty-list= t('.empty') diff --git a/app/views/items/_item_link.html.haml b/app/views/items/_item_link.html.haml index e50aafd3..14da7e4f 100644 --- a/app/views/items/_item_link.html.haml +++ b/app/views/items/_item_link.html.haml @@ -1,4 +1,6 @@ +-# NOTE: Changing this requires bumping the cache at `_closet_list.html.haml`! = link_to item_path(item) do - = image_tag item.thumbnail_url, :alt => item.description, :title => item.description + = image_tag item.thumbnail_url, alt: "Thumbnail for #{item.name}", + title: item.description, loading: "lazy" %span.name= item.name = nc_icon_for(item)