DEV: Improve theme error handling UX

- Update UI to improve contrast
- Make it clear that the message is only shown to administrators
- Add theme name and id to the console output
- Parse the error backtrace to identify the theme-id for post-decoration errors
- Improve console output to include the theme name / URL
- Add `?safe_mode=no_custom` to the admin panel link, so that it will work even if the theme is causing the site to break

app/assets/javascripts/discourse/app/app.js

@@ -52,6 +52,11 @@ const Discourse = Application.extend({
   start() {
     document.querySelector("noscript")?.remove();
 
+    if (Error.stackTraceLimit) {
+      // We need Errors to have full stack traces for `lib/source-identifier`
+      Error.stackTraceLimit = Infinity;
+    }
+
     Object.keys(requirejs._eak_seen).forEach((key) => {
       if (/\/pre\-initializers\//.test(key)) {
         const initializer = this._prepareInitializer(key);

app/assets/javascripts/discourse/app/lib/source-identifier.js

@@ -0,0 +1,46 @@
+import getURL from "discourse-common/lib/get-url";
+import PreloadStore from "discourse/lib/preload-store";
+
+export default function identifySource(error) {
+  if (!error || !error.stack) {
+    try {
+      throw new Error("Source identification error");
+    } catch (e) {
+      error = e;
+    }
+  }
+
+  if (!error.stack) {
+    return;
+  }
+
+  const themeMatches =
+    error.stack.match(/\/theme-javascripts\/[\w-]+\.js/g) || [];
+
+  for (const match of themeMatches) {
+    const scriptElement = document.querySelector(`script[src*="${match}"`);
+    if (scriptElement?.dataset.themeId) {
+      return {
+        type: "theme",
+        ...getThemeInfo(scriptElement.dataset.themeId),
+      };
+    }
+  }
+}
+
+export function getThemeInfo(id) {
+  const name = PreloadStore.get("activatedThemes")[id] || `(theme-id: ${id})`;
+  return {
+    id,
+    name,
+    path: getURL(`/admin/customize/themes/${id}?safe_mode=no_custom`),
+  };
+}
+
+export function consolePrefix(error, source) {
+  source = source || identifySource(error);
+  if (source && source.type === "theme") {
+    return `[THEME ${source.id} '${source.name}']`;
+  }
+  return "";
+}

app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js

@@ -1,9 +1,13 @@
 import { isTesting } from "discourse-common/config/environment";
 import { getAndClearUnhandledThemeErrors } from "discourse/app";
-import PreloadStore from "discourse/lib/preload-store";
 import getURL from "discourse-common/lib/get-url";
 import I18n from "I18n";
 import { bind } from "discourse-common/utils/decorators";
+import { escape } from "pretty-text/sanitizer";
+import identifySource, {
+  consolePrefix,
+  getThemeInfo,
+} from "discourse/lib/source-identifier";
 
 const showingErrors = new Set();
 
@@ -56,39 +60,64 @@ function reportToLogster(name, error) {
 function reportThemeError(currentUser, e) {
   const { themeId, error } = e.detail;
 
-  const name =
+  const source = {
-    PreloadStore.get("activatedThemes")[themeId] || `(theme-id: ${themeId})`;
+    type: "theme",
-  /* eslint-disable-next-line no-console */
+    ...getThemeInfo(themeId),
-  console.error(`An error occurred in the "${name}" theme/component:`, error);
+  };
-  reportToLogster(name, error);
 
-  const path = getURL("/admin/customize/themes");
+  reportToConsole(error, source);
-  const message = I18n.t("themes.broken_theme_alert", {
+  reportToLogster(source.name, error);
-    theme: name,
+
-    path: `<a href="${path}">${path}</a>`,
+  const message = I18n.t("themes.broken_theme_alert");
-  });
+  displayErrorNotice(currentUser, message, source);
-  displayErrorNotice(currentUser, message);
 }
 
 function reportGenericError(currentUser, e) {
   const { messageKey, error } = e.detail;
 
-  /* eslint-disable-next-line no-console */
+  let message = I18n.t(messageKey);
-  console.error(error);
+
+  const source = identifySource(error);
+
+  reportToConsole(error, source);
 
   if (messageKey && !showingErrors.has(messageKey)) {
     showingErrors.add(messageKey);
-    displayErrorNotice(currentUser, I18n.t(messageKey));
+    displayErrorNotice(currentUser, message, source);
   }
 }
 
-function displayErrorNotice(currentUser, message) {
+function reportToConsole(error, source) {
+  const prefix = consolePrefix(error, source);
+  if (prefix) {
+    /* eslint-disable-next-line no-console */
+    console.error(prefix, error);
+  } else {
+    /* eslint-disable-next-line no-console */
+    console.error(error);
+  }
+}
+
+function displayErrorNotice(currentUser, message, source) {
   if (!currentUser?.admin) {
     return;
   }
 
+  let html = `⚠️ ${message}`;
+
+  if (source && source.type === "theme") {
+    html += `<br/>${I18n.t("themes.error_caused_by", {
+      name: escape(source.name),
+      path: source.path,
+    })}`;
+  }
+
+  html += `<br/><span class='theme-error-suffix'>${I18n.t(
+    "themes.only_admins"
+  )}</span>`;
+
   const alertDiv = document.createElement("div");
   alertDiv.classList.add("broken-theme-alert");
-  alertDiv.innerHTML = `⚠️ ${message}`;
+  alertDiv.innerHTML = html;
   document.body.prepend(alertDiv);
 }

app/assets/stylesheets/common/base/discourse.scss

@@ -830,10 +830,19 @@ table {
   font-size: $base-font-size;
   font-weight: bold;
   padding: 5px 0;
-  background: var(--danger-medium);
+  background: var(--danger);
   text-align: center;
   z-index: z("max");
   color: var(--secondary);
+
+  a {
+    color: var(--secondary);
+    text-decoration: underline;
+  }
+
+  .theme-error-suffix {
+    font-weight: normal;
+  }
 }
 
 .controls {

app/models/theme.rb

@@ -152,8 +152,7 @@ class Theme < ActiveRecord::Base
     SvgSprite.expire_cache
   end
 
-  BASE_COMPILER_VERSION = 53
+  BASE_COMPILER_VERSION = 54
-
   def self.compiler_version
     get_set_cache "compiler_version" do
       dependencies = [
@@ -390,7 +389,7 @@ class Theme < ActiveRecord::Base
       end
       caches = JavascriptCache.where(theme_id: theme_ids)
       caches = caches.sort_by { |cache| theme_ids.index(cache.theme_id) }
-      return caches.map { |c| "<script src='#{c.url}'></script>" }.join("\n")
+      return caches.map { |c| "<script src='#{c.url}' data-theme-id='#{c.theme_id}'></script>" }.join("\n")
     end
     list_baked_fields(theme_ids, target, name).map { |f| f.value_baked || f.value }.join("\n")
   end

app/models/theme_field.rb

@@ -142,7 +142,7 @@ class ThemeField < ActiveRecord::Base
     javascript_cache.content = js_compiler.content
     javascript_cache.save!
 
-    doc.add_child("<script src='#{javascript_cache.url}'></script>") if javascript_cache.content.present?
+    doc.add_child("<script src='#{javascript_cache.url}' data-theme-id='#{theme_id}'></script>") if javascript_cache.content.present?
     [doc.to_s, errors&.join("\n")]
   end
 
@@ -258,7 +258,7 @@ class ThemeField < ActiveRecord::Base
     javascript_cache.content = js_compiler.content
     javascript_cache.save!
     doc = ""
-    doc = "<script src='#{javascript_cache.url}'></script>" if javascript_cache.content.present?
+    doc = "<script src='#{javascript_cache.url}' data-theme-id='#{theme_id}'></script>" if javascript_cache.content.present?
     [doc, errors&.join("\n")]
   end
 

config/locales/client.en.yml

@@ -194,9 +194,11 @@ en:
 
     themes:
       default_description: "Default"
-      broken_theme_alert: "Your site may not work because theme / component %{theme} has errors. Disable it at %{path}."
+      broken_theme_alert: "Your site may not work because a theme / component has errors."
+      error_caused_by: "Caused by '%{name}'. <a target='blank' href='%{path}'>Click here</a> to reconfigure or disable."
+      only_admins: "(this message is only shown to site administrators)"
 
-    broken_decorator_alert: "Posts may not display correctly because one of the post content decorators on your site raised an error. Check the browser developer tools for more information."
+    broken_decorator_alert: "Posts may not display correctly because one of the post content decorators on your site raised an error."
 
     s3:
       regions:

spec/models/theme_field_spec.rb

@@ -77,7 +77,7 @@ describe ThemeField do
 
     theme_field = ThemeField.create!(theme_id: 1, target_id: 0, name: "header", value: html)
     theme_field.ensure_baked!
-    expect(theme_field.value_baked).to include("<script src=\"#{theme_field.javascript_cache.url}\"></script>")
+    expect(theme_field.value_baked).to include("<script src=\"#{theme_field.javascript_cache.url}\" data-theme-id=\"1\"></script>")
     expect(theme_field.value_baked).to include("external-script.js")
     expect(theme_field.value_baked).to include('<script type="text/template"')
     expect(theme_field.javascript_cache.content).to include('a = "inline discourse plugin"')
@@ -112,7 +112,7 @@ HTML
     field = ThemeField.create!(theme_id: 1, target_id: 0, name: "header", value: html)
     field.ensure_baked!
     expect(field.error).not_to eq(nil)
-    expect(field.value_baked).to include("<script src=\"#{field.javascript_cache.url}\"></script>")
+    expect(field.value_baked).to include("<script src=\"#{field.javascript_cache.url}\" data-theme-id=\"1\"></script>")
     expect(field.javascript_cache.content).to include("Theme Transpilation Error:")
 
     field.update!(value: '')
@@ -132,7 +132,7 @@ HTML
     theme_field.ensure_baked!
     javascript_cache = theme_field.javascript_cache
 
-    expect(theme_field.value_baked).to include("<script src=\"#{javascript_cache.url}\"></script>")
+    expect(theme_field.value_baked).to include("<script src=\"#{javascript_cache.url}\" data-theme-id=\"1\"></script>")
     expect(javascript_cache.content).to include("testing-div")
     expect(javascript_cache.content).to include("string_setting")
     expect(javascript_cache.content).to include("test text \\\" 123!")
@@ -380,7 +380,7 @@ HTML
     describe "javascript cache" do
       it "is generated correctly" do
         fr1.ensure_baked!
-        expect(fr1.value_baked).to include("<script src='#{fr1.javascript_cache.url}'></script>")
+        expect(fr1.value_baked).to include("<script src='#{fr1.javascript_cache.url}' data-theme-id='#{fr1.theme_id}'></script>")
         expect(fr1.javascript_cache.content).to include("bonjourworld")
         expect(fr1.javascript_cache.content).to include("helloworld")
         expect(fr1.javascript_cache.content).to include("enval1")