From 21f3f72647a1959429df701cb130b48363e5acee Mon Sep 17 00:00:00 2001 From: Jeremy Daer Date: Thu, 4 Dec 2025 23:17:21 -0800 Subject: [PATCH] Immediate avatar and embed variants Process variants synchronously on attachment to close the window between image upload and variant availability, guaranteeing that we won't have lazy variant processing attempts in GET requests. Tradeoff is that we do variant processing in upload requests, which is actually desirable. We're working with images that should take milliseconds to resize given that we'll already have the file on hand. References https://github.com/rails/rails/pull/51951 --- Gemfile.lock | 2 +- Gemfile.saas.lock | 6 ++--- app/models/concerns/attachments.rb | 9 +++---- app/models/user/avatar.rb | 2 +- config/initializers/action_text.rb | 2 +- .../users/avatars_controller_test.rb | 8 +++++-- .../notification/bundle_mailer_test.rb | 3 ++- test/models/comment_test.rb | 20 ++++++++++++++++ test/models/user/avatar_test.rb | 24 +++++++++++++++++-- 9 files changed, 59 insertions(+), 17 deletions(-) diff --git a/Gemfile.lock b/Gemfile.lock index b9c3489b5..59e79ec08 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -6,7 +6,7 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 3c3f6c8a253c8a0f346695374f927c9ab32fbefb + revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 branch: main specs: actioncable (8.2.0.alpha) diff --git a/Gemfile.saas.lock b/Gemfile.saas.lock index 5cd308155..328158c0d 100644 --- a/Gemfile.saas.lock +++ b/Gemfile.saas.lock @@ -75,7 +75,7 @@ GIT GIT remote: https://github.com/rails/rails.git - revision: 690ec8898318b8f50714e86676353ebe1551261e + revision: 3d105fc346fbb3121bbcf6340f2b19104bf326f0 branch: main specs: actioncable (8.2.0.alpha) @@ -292,7 +292,7 @@ GEM actionview (>= 7.0.0) activesupport (>= 7.0.0) jmespath (1.6.2) - json (2.16.0) + json (2.17.1) jwt (3.1.2) base64 kamal (2.9.0) @@ -552,7 +552,7 @@ GEM ostruct stimulus-rails (1.3.4) railties (>= 6.0.0) - stringio (3.1.8) + stringio (3.1.9) thor (1.4.0) thruster (0.1.16) thruster (0.1.16-aarch64-linux) diff --git a/app/models/concerns/attachments.rb b/app/models/concerns/attachments.rb index 1d1c2c4d6..bcc7303ae 100644 --- a/app/models/concerns/attachments.rb +++ b/app/models/concerns/attachments.rb @@ -1,13 +1,10 @@ module Attachments extend ActiveSupport::Concern - # The variants we use must all declared here so that we can preprocess them. + # Variants used by ActionText embeds. Processed immediately on attachment to avoid + # read replica issues (lazy variants would attempt writes on read replicas). # - # If they are not preprocessed, then Rails will attempt to transform the image on-the-fly when - # they are first viewed, which may be on the read replica where writing to the database is not - # allowed. Chaos will ensue if that happens. - # - # These variants are patched into ActionText::RichText in config/initializers/action_text.rb + # Patched into ActionText::RichText in config/initializers/action_text.rb VARIANTS = { # vipsthumbnail used to create sized image variants has a intent setting to manage colors during # resize. By setting an invalid intent value the gif-incompatible intent filtering is skipped and diff --git a/app/models/user/avatar.rb b/app/models/user/avatar.rb index a4b2ae631..c635edddb 100644 --- a/app/models/user/avatar.rb +++ b/app/models/user/avatar.rb @@ -5,7 +5,7 @@ module User::Avatar included do has_one_attached :avatar do |attachable| - attachable.variant :thumb, resize_to_fill: [ 256, 256 ] + attachable.variant :thumb, resize_to_fill: [ 256, 256 ], process: :immediately end validate :avatar_content_type_allowed diff --git a/config/initializers/action_text.rb b/config/initializers/action_text.rb index b2724b1c7..eee87de0d 100644 --- a/config/initializers/action_text.rb +++ b/config/initializers/action_text.rb @@ -7,7 +7,7 @@ module ActionText # This overrides the default :embeds association! has_many_attached :embeds do |attachable| ::Attachments::VARIANTS.each do |variant_name, variant_options| - attachable.variant variant_name, variant_options.merge(preprocessed: true) + attachable.variant variant_name, **variant_options, process: :immediately end end end diff --git a/test/controllers/users/avatars_controller_test.rb b/test/controllers/users/avatars_controller_test.rb index 3ded884b8..be4532448 100644 --- a/test/controllers/users/avatars_controller_test.rb +++ b/test/controllers/users/avatars_controller_test.rb @@ -27,7 +27,9 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show own image redirects to the blob url" do - users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + # Create blob separately to ensure file is uploaded before variant processing + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + users(:david).avatar.attach(blob) assert users(:david).avatar.attached? get user_avatar_path(users(:david)) @@ -36,7 +38,9 @@ class Users::AvatarsControllerTest < ActionDispatch::IntegrationTest end test "show other image redirects to the blob url" do - users(:kevin).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + # Create blob separately to ensure file is uploaded before variant processing + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + users(:kevin).avatar.attach(blob) assert users(:kevin).avatar.attached? get user_avatar_path(users(:kevin)) diff --git a/test/mailers/notification/bundle_mailer_test.rb b/test/mailers/notification/bundle_mailer_test.rb index fbc4274c9..63647e68a 100644 --- a/test/mailers/notification/bundle_mailer_test.rb +++ b/test/mailers/notification/bundle_mailer_test.rb @@ -22,11 +22,12 @@ class Notification::BundleMailerTest < ActionMailer::TestCase end test "renders avatar with external image URL when avatar is attached" do - @user.avatar.attach( + blob = ActiveStorage::Blob.create_and_upload!( io: File.open(Rails.root.join("test", "fixtures", "files", "avatar.png")), filename: "avatar.png", content_type: "image/png" ) + @user.avatar.attach(blob) create_notification(@user) diff --git a/test/models/comment_test.rb b/test/models/comment_test.rb index c519f8fbd..13a5e36c3 100644 --- a/test/models/comment_test.rb +++ b/test/models/comment_test.rb @@ -4,4 +4,24 @@ class CommentTest < ActiveSupport::TestCase setup do Current.session = sessions(:david) end + + test "rich text embed variants are processed immediately on attachment" do + comment = cards(:logo).comments.create!(body: "Check this out") + comment.body.body.attachables # force load + + blob = ActiveStorage::Blob.create_and_upload! \ + io: File.open(file_fixture("moon.jpg")), + filename: "moon.jpg", + content_type: "image/jpeg" + + comment.body.body = ActionText::Content.new(comment.body.body.to_html).append_attachables(blob) + comment.save! + + embed = comment.body.embeds.sole + + Attachments::VARIANTS.each_key do |variant_name| + variant = embed.variant(variant_name) + assert variant.processed?, "Expected #{variant_name} variant to be processed immediately" + end + end end diff --git a/test/models/user/avatar_test.rb b/test/models/user/avatar_test.rb index 96ea5bac7..58b78969c 100644 --- a/test/models/user/avatar_test.rb +++ b/test/models/user/avatar_test.rb @@ -2,7 +2,8 @@ require "test_helper" class User::AvatarTest < ActiveSupport::TestCase test "avatar_thumbnail returns variant for variable images" do - users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "moon.jpg", content_type: "image/jpeg") + users(:david).avatar.attach(blob) assert users(:david).avatar.variable? assert_equal users(:david).avatar.variant(:thumb).blob, users(:david).avatar_thumbnail.blob @@ -16,7 +17,8 @@ class User::AvatarTest < ActiveSupport::TestCase end test "allows valid image content types" do - users(:david).avatar.attach(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg") + blob = ActiveStorage::Blob.create_and_upload!(io: File.open(file_fixture("moon.jpg")), filename: "test.jpg", content_type: "image/jpeg") + users(:david).avatar.attach(blob) assert users(:david).valid? end @@ -27,4 +29,22 @@ class User::AvatarTest < ActiveSupport::TestCase assert_not users(:david).valid? assert_includes users(:david).errors[:avatar], "must be a JPEG, PNG, GIF, or WebP image" end + + test "thumb variant is processed immediately on attachment" do + # Create blob separately to ensure file is uploaded before variant processing. + # + # Root cause: When ActiveStorage::Record uses `connects_to` for read replica support + # (as in SAAS mode), it creates a separate connection pool from application models. + # Since after_commit callbacks are tracked per connection pool, the callback order + # between User's pool (upload) and Attachment's pool (create_variants) isn't guaranteed. + # In MySQL/SAAS mode, the Attachment callback fires before the file is uploaded. + blob = ActiveStorage::Blob.create_and_upload!( + io: File.open(file_fixture("avatar.png")), + filename: "avatar.png", + content_type: "image/png" + ) + users(:david).avatar.attach(blob) + + assert users(:david).avatar.variant(:thumb).processed? + end end