FIX: Ensure live-reloading of theme CSS works first time (#8052)

The client-side theme-selector would always apply the first in a series of file change notifications. This has been fixed, so it now applies the most recent notification.

Duplicate notifications were being sent because
- The remote_theme autosave was causing every change notification to be doubled
- Color scheme change notifications were being sent every time a theme was uploaded, even if the colors were unchanged

These duplicate notifications have been fixed, and a spec added to ensure it does not regress in future

app/assets/javascripts/discourse/lib/theme-selector.js.es6

@@ -43,16 +43,12 @@ export function setLocalTheme(ids, themeSeq) {
   }
 }
 
-export function refreshCSS(node, hash, newHref, options) {
+export function refreshCSS(node, hash, newHref) {
   let $orig = $(node);
 
   if ($orig.data("reloading")) {
-    if (options && options.force) {
-      clearTimeout($orig.data("timeout"));
-      $orig.data("copy").remove();
-    } else {
-      return;
-    }
+    clearTimeout($orig.data("timeout"));
+    $orig.data("copy").remove();
   }
 
   if (!$orig.data("orig")) {
@@ -99,7 +95,7 @@ export function previewTheme(ids = []) {
             `link[rel=stylesheet][data-target=${theme.target}]`
           )[0];
           if (node) {
-            refreshCSS(node, null, theme.new_href, { force: true });
+            refreshCSS(node, null, theme.new_href);
           }
         });
       }

app/models/remote_theme.rb

@@ -21,7 +21,7 @@ class RemoteTheme < ActiveRecord::Base
   GITHUB_REGEXP = /^https?:\/\/github\.com\//
   GITHUB_SSH_REGEXP = /^git@github\.com:/
 
-  has_one :theme
+  has_one :theme, autosave: false
   scope :joined_remotes, -> {
     joins("JOIN themes ON themes.remote_theme_id = remote_themes.id").where.not(remote_url: "")
   }
@@ -211,7 +211,7 @@ class RemoteTheme < ActiveRecord::Base
         color_scheme_color = scheme.color_scheme_colors.to_a.find { |c| c.name == color[:name] } ||
                   scheme.color_scheme_colors.build(name: color[:name])
         color_scheme_color.hex = override || color[:hex]
-        theme.notify_color_change(color_scheme_color)
+        theme.notify_color_change(color_scheme_color) if color_scheme_color.hex_changed?
       end
 
       # Update advanced colors
@@ -221,7 +221,7 @@ class RemoteTheme < ActiveRecord::Base
         if override
           color_scheme_color ||= scheme.color_scheme_colors.build(name: variable_name)
           color_scheme_color.hex = override
-          theme.notify_color_change(color_scheme_color)
+          theme.notify_color_change(color_scheme_color) if color_scheme_color.hex_changed?
         elsif color_scheme_color # No longer specified in about.json, delete record
           scheme.color_scheme_colors.delete(color_scheme_color)
           theme.notify_color_change(nil, scheme: scheme)

app/models/theme.rb

@@ -22,7 +22,7 @@ class Theme < ActiveRecord::Base
   has_many :child_themes, -> { order(:name) }, through: :child_theme_relation, source: :child_theme
   has_many :parent_themes, -> { order(:name) }, through: :parent_theme_relation, source: :parent_theme
   has_many :color_schemes
-  belongs_to :remote_theme, autosave: true, dependent: :destroy
+  belongs_to :remote_theme, dependent: :destroy
 
   has_one :settings_field, -> { where(target_id: Theme.targets[:settings], name: "yaml") }, class_name: 'ThemeField'
   has_one :javascript_cache, dependent: :destroy

spec/models/theme_spec.rb

@@ -62,6 +62,7 @@ describe Theme do
   it "can automatically disable for mismatching version" do
     expect(theme.supported?).to eq(true)
     theme.create_remote_theme!(remote_url: "", minimum_discourse_version: "99.99.99")
+    theme.save!
     expect(theme.supported?).to eq(false)
 
     expect(Theme.transform_ids([theme.id])).to be_empty

spec/requests/admin/themes_controller_spec.rb

@@ -136,12 +136,20 @@ describe Admin::ThemesController do
       existing_theme = Fabricate(:theme, name: "Header Icons")
       other_existing_theme = Fabricate(:theme, name: "Some other name")
 
-      expect do
-        post "/admin/themes/import.json", params: { bundle: theme_archive, theme_id: other_existing_theme.id }
-      end.to change { Theme.count }.by (0)
+      messages = MessageBus.track_publish do
+        expect do
+          post "/admin/themes/import.json", params: { bundle: theme_archive, theme_id: other_existing_theme.id }
+        end.to change { Theme.count }.by (0)
+      end
       expect(response.status).to eq(201)
       json = ::JSON.parse(response.body)
 
+      # Ensure only one refresh message is sent.
+      # More than 1 is wasteful, and can trigger unusual race conditions in the client
+      # If this test fails, it probably means `theme.save` is being called twice - check any 'autosave' relations
+      file_change_messages = messages.filter { |m| m[:channel] == "/file-change" }
+      expect(file_change_messages.count).to eq(1)
+
       expect(json["theme"]["name"]).to eq("Some other name")
       expect(json["theme"]["id"]).to eq(other_existing_theme.id)
       expect(json["theme"]["theme_fields"].length).to eq(5)
@@ -383,6 +391,7 @@ describe Admin::ThemesController do
 
     it 'handles import errors on update' do
       theme.create_remote_theme!(remote_url: "https://example.com/repository")
+      theme.save!
 
       # RemoteTheme is extensively tested, and setting up the test scaffold is a large overhead
       # So use a stub here to test the controller