PERF: Don't serialize value for theme_fields unnecessarily (#21201)

The value field of ThemeField is only used when viewing a diff in the staff action logs and local theme editing. value is being serialized into the theme index as well, which is not used. It's a huge amount of JSON that we can cut by removing it.

This also breaks up the various theme serializers into separate classes so they autoload properly (or at least restart the server on edit)

app/serializers/basic_theme_serializer.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class BasicThemeSerializer < ApplicationSerializer
+  attributes :id, :name, :created_at, :updated_at, :default, :component
+
+  def include_default?
+    object.id == SiteSetting.default_theme_id
+  end
+
+  def default
+    true
+  end
+end

app/serializers/remote_theme_serializer.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+class RemoteThemeSerializer < ApplicationSerializer
+  attributes :id,
+             :remote_url,
+             :remote_version,
+             :local_version,
+             :commits_behind,
+             :branch,
+             :remote_updated_at,
+             :updated_at,
+             :github_diff_link,
+             :last_error_text,
+             :is_git?,
+             :license_url,
+             :about_url,
+             :authors,
+             :theme_version,
+             :minimum_discourse_version,
+             :maximum_discourse_version
+
+  # wow, AMS has some pretty nutty logic where it tries to find the path here
+  # from action dispatch, tell it not to
+  def about_url
+    object.about_url
+  end
+
+  def include_github_diff_link?
+    github_diff_link.present?
+  end
+end

app/serializers/theme_field_serializer.rb

@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+class ThemeFieldSerializer < ApplicationSerializer
+  attributes :name, :target, :value, :error, :type_id, :upload_id, :url, :filename
+
+  def include_url?
+    object.upload
+  end
+
+  def include_upload_id?
+    object.upload
+  end
+
+  def include_filename?
+    object.upload
+  end
+
+  def url
+    object.upload&.url
+  end
+
+  def filename
+    object.upload&.original_filename
+  end
+
+  def include_value?
+    @options[:include_value] || false
+  end
+
+  def target
+    Theme.lookup_target(object.target_id)&.to_s
+  end
+
+  def include_error?
+    object.error.present?
+  end
+end

app/serializers/theme_serializer.rb

@@ -2,80 +2,6 @@
 
 require "base64"
 
-class ThemeFieldSerializer < ApplicationSerializer
-  attributes :name, :target, :value, :error, :type_id, :upload_id, :url, :filename
-
-  def include_url?
-    object.upload
-  end
-
-  def include_upload_id?
-    object.upload
-  end
-
-  def include_filename?
-    object.upload
-  end
-
-  def url
-    object.upload&.url
-  end
-
-  def filename
-    object.upload&.original_filename
-  end
-
-  def target
-    Theme.lookup_target(object.target_id)&.to_s
-  end
-
-  def include_error?
-    object.error.present?
-  end
-end
-
-class BasicThemeSerializer < ApplicationSerializer
-  attributes :id, :name, :created_at, :updated_at, :default, :component
-
-  def include_default?
-    object.id == SiteSetting.default_theme_id
-  end
-
-  def default
-    true
-  end
-end
-
-class RemoteThemeSerializer < ApplicationSerializer
-  attributes :id,
-             :remote_url,
-             :remote_version,
-             :local_version,
-             :commits_behind,
-             :branch,
-             :remote_updated_at,
-             :updated_at,
-             :github_diff_link,
-             :last_error_text,
-             :is_git?,
-             :license_url,
-             :about_url,
-             :authors,
-             :theme_version,
-             :minimum_discourse_version,
-             :maximum_discourse_version
-
-  # wow, AMS has some pretty nutty logic where it tries to find the path here
-  # from action dispatch, tell it not to
-  def about_url
-    object.about_url
-  end
-
-  def include_github_diff_link?
-    github_diff_link.present?
-  end
-end
-
 class ThemeSerializer < BasicThemeSerializer
   attributes :color_scheme,
              :color_scheme_id,
@@ -87,12 +13,12 @@ class ThemeSerializer < BasicThemeSerializer
              :supported?,
              :description,
              :enabled?,
-             :disabled_at
+             :disabled_at,
+             :theme_fields
 
   has_one :user, serializer: UserNameSerializer, embed: :object
   has_one :disabled_by, serializer: UserNameSerializer, embed: :object
 
-  has_many :theme_fields, serializer: ThemeFieldSerializer, embed: :objects
   has_many :child_themes, serializer: BasicThemeSerializer, embed: :objects
   has_many :parent_themes, serializer: BasicThemeSerializer, embed: :objects
   has_one :remote_theme, serializer: RemoteThemeSerializer, embed: :objects
@@ -100,11 +26,27 @@ class ThemeSerializer < BasicThemeSerializer
 
   def initialize(theme, options = {})
     super
+    @include_theme_field_values = options[:include_theme_field_values] || false
     @errors = []
 
     object.theme_fields.each { |o| @errors << o.error if o.error }
   end
 
+  def theme_fields
+    ActiveModel::ArraySerializer.new(
+      object.theme_fields,
+      each_serializer: ThemeFieldSerializer,
+      include_value: include_theme_field_values?,
+    ).as_json
+  end
+
+  def include_theme_field_values?
+    # This is passed into each `ThemeFieldSerializer` to determine if `value` will be serialized.
+    # We only want to serialize if we are viewing staff_action_logs (for diffing changes), or if
+    # the theme is a local theme, so the saved values appear in the theme field editor.
+    @include_theme_field_values || object.remote_theme_id.nil?
+  end
+
   def child_themes
     object.child_themes
   end

app/services/staff_action_logger.rb

@@ -219,7 +219,7 @@ class StaffActionLogger
   end
 
   def theme_json(theme)
-    ThemeSerializer.new(theme, root: false).to_json
+    ThemeSerializer.new(theme, root: false, include_theme_field_values: true).to_json
   end
 
   def strip_duplicates(old, cur)

spec/requests/admin/staff_action_logs_controller_spec.rb

@@ -105,7 +105,8 @@ RSpec.describe Admin::StaffActionLogsController do
         theme.set_field(target: :mobile, name: :scss, value: "body {.up}")
         theme.set_field(target: :common, name: :scss, value: "omit-dupe")
 
-        original_json = ThemeSerializer.new(theme, root: false).to_json
+        original_json =
+          ThemeSerializer.new(theme, root: false, include_theme_field_values: true).to_json
 
         theme.set_field(target: :mobile, name: :scss, value: "body {.down}")
 

spec/requests/admin/themes_controller_spec.rb

@@ -605,12 +605,7 @@ RSpec.describe Admin::ThemesController do
 
         json = response.parsed_body
 
-        fields = json["theme"]["theme_fields"].sort { |a, b| a["value"] <=> b["value"] }
+        expect(json["theme"]["theme_fields"].length).to eq(2)
-
-        expect(fields[0]["value"]).to eq("")
-        expect(fields[0]["upload_id"]).to eq(upload.id)
-        expect(fields[1]["value"]).to eq("body{color: blue;}")
-        expect(fields.length).to eq(2)
         expect(json["theme"]["child_themes"].length).to eq(1)
         expect(UserHistory.where(action: UserHistory.actions[:change_theme]).count).to eq(1)
       end

spec/system/admin_customize_themes_spec.rb

@@ -27,4 +27,16 @@ describe "Admin Customize Themes", type: :system, js: true do
       expect(theme.reload.color_scheme_id).to eq(color_scheme.id)
     end
   end
+
+  describe "when editing a local theme" do
+    it "The saved value is present in the editor" do
+      theme.set_field(target: "common", name: "head_tag", value: "console.log('test')", type_id: 0)
+      theme.save!
+
+      visit("/admin/customize/themes/#{theme.id}/common/head_tag/edit")
+
+      ace_content = find(".ace_content")
+      expect(ace_content.text).to eq("console.log('test')")
+    end
+  end
 end