From af24c1031426d58b4fdc8ff0eaf311c47d59e799 Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 11 Feb 2022 09:16:18 +0000 Subject: [PATCH] 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 | 5 ++ .../discourse/app/lib/source-identifier.js | 46 ++++++++++++++ .../pre-initializers/theme-errors-handler.js | 63 ++++++++++++++----- .../stylesheets/common/base/discourse.scss | 11 +++- app/models/theme.rb | 5 +- app/models/theme_field.rb | 4 +- config/locales/client.en.yml | 6 +- spec/models/theme_field_spec.rb | 8 +-- 8 files changed, 119 insertions(+), 29 deletions(-) create mode 100644 app/assets/javascripts/discourse/app/lib/source-identifier.js diff --git a/app/assets/javascripts/discourse/app/app.js b/app/assets/javascripts/discourse/app/app.js index bce247db3c5..19da3d28362 100644 --- a/app/assets/javascripts/discourse/app/app.js +++ b/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); diff --git a/app/assets/javascripts/discourse/app/lib/source-identifier.js b/app/assets/javascripts/discourse/app/lib/source-identifier.js new file mode 100644 index 00000000000..0ba32e30a4b --- /dev/null +++ b/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 ""; +} diff --git a/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js b/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js index 98a2f914396..af3f4cfdcc9 100644 --- a/app/assets/javascripts/discourse/app/pre-initializers/theme-errors-handler.js +++ b/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 = - PreloadStore.get("activatedThemes")[themeId] || `(theme-id: ${themeId})`; - /* eslint-disable-next-line no-console */ - console.error(`An error occurred in the "${name}" theme/component:`, error); - reportToLogster(name, error); + const source = { + type: "theme", + ...getThemeInfo(themeId), + }; - const path = getURL("/admin/customize/themes"); - const message = I18n.t("themes.broken_theme_alert", { - theme: name, - path: `${path}`, - }); - displayErrorNotice(currentUser, message); + reportToConsole(error, source); + reportToLogster(source.name, error); + + const message = I18n.t("themes.broken_theme_alert"); + displayErrorNotice(currentUser, message, source); } function reportGenericError(currentUser, e) { const { messageKey, error } = e.detail; - /* eslint-disable-next-line no-console */ - console.error(error); + let message = I18n.t(messageKey); + + 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 += `
${I18n.t("themes.error_caused_by", { + name: escape(source.name), + path: source.path, + })}`; + } + + html += `
${I18n.t( + "themes.only_admins" + )}`; + const alertDiv = document.createElement("div"); alertDiv.classList.add("broken-theme-alert"); - alertDiv.innerHTML = `⚠️ ${message}`; + alertDiv.innerHTML = html; document.body.prepend(alertDiv); } diff --git a/app/assets/stylesheets/common/base/discourse.scss b/app/assets/stylesheets/common/base/discourse.scss index 9b253d36b8e..0126ae6cc55 100644 --- a/app/assets/stylesheets/common/base/discourse.scss +++ b/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 { diff --git a/app/models/theme.rb b/app/models/theme.rb index 5fcaf5f436a..34795dfe101 100644 --- a/app/models/theme.rb +++ b/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| "" }.join("\n") + return caches.map { |c| "" }.join("\n") end list_baked_fields(theme_ids, target, name).map { |f| f.value_baked || f.value }.join("\n") end diff --git a/app/models/theme_field.rb b/app/models/theme_field.rb index fac8bf1b781..162d80b1e48 100644 --- a/app/models/theme_field.rb +++ b/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("") if javascript_cache.content.present? + doc.add_child("") 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 = "" if javascript_cache.content.present? + doc = "" if javascript_cache.content.present? [doc, errors&.join("\n")] end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 80b7b8fda1d..a778b2e6763 100644 --- a/config/locales/client.en.yml +++ b/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}'. Click here 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: diff --git a/spec/models/theme_field_spec.rb b/spec/models/theme_field_spec.rb index 63b2cc2b67a..d09c7625b61 100644 --- a/spec/models/theme_field_spec.rb +++ b/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("") + expect(theme_field.value_baked).to include("") expect(theme_field.value_baked).to include("external-script.js") expect(theme_field.value_baked).to include('") + expect(field.value_baked).to include("") 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("") + expect(theme_field.value_baked).to include("") 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("") + expect(fr1.value_baked).to include("") expect(fr1.javascript_cache.content).to include("bonjourworld") expect(fr1.javascript_cache.content).to include("helloworld") expect(fr1.javascript_cache.content).to include("enval1")