FEATURE: detect theme errors and catch them (PR #7589)

GitHub

You’ve signed the CLA, OsamaSayegh. Thank you! This pull request is ready for review.

The title of this pull request changed from “Theme errors shield” to "FEATURE: detect theme errors and catch them

This could be replaced with vanilla js:

const alertDiv = document.createElement("div");
alertDiv.classList.add("broken-theme-alert");
alertDiv.innerText = message;
document.body.prepend(alertDiv);

Note I removed the <b> because it doesn’t have much interest to me. You should probably set font-weight on .broken-theme-alert

  if (!currentUser || !currentUser.admin) {

Why not using our ajax lib?

Also why not "/logs/report_js_error" instead of "/logs" + "/report_js_error"

is this necessary ?

I would add a line break before and after this user check block. Doesn’t have any direct link with console logging and the alert warning.

@jjaffeux thanks for the review! I’ve made the changes you suggested :+1:

It seems MiniRacer doesn’t like importing ajax from the utilities file:

MiniRacer::RuntimeError: Error: Could not find module `discourse/lib/ajax` imported from `discourse/lib/utilities`

https://travis-ci.org/discourse/discourse/jobs/536272772#L7870

Works perfectly fine locally though.

After this PR gets merged, please watch the Timeline at Transifex carefully for “Autofetch failed” errors. A year ago an Emoji stopped Transifex from doing auto updates. It will be interesting to see if they fixed the bug in the meantime. I highly doubt that it will work, so be prepared to show the warning sign in another way.

1 Like

Thanks @gschlager for the heads up. I did notice there is another string with an emoji here:

https://github.com/discourse/discourse/blob/1fbe078ae074f29395d79db0856d1991f1675ab5/config/locales/client.en.yml#L182

This line was last changed 2 years ago, so maybe the problem is specific to a set of emojis? This makes me wonder if emojis have to “translatable”?

Yes, I think it depends on the Emoji. Transifex expects some of them to be represented as Unicode Surrogates which isn’t supported by Ruby. You might not even need to include this Emoji in the translation as long as it still works with RTL. There’s definitely no need to translate a warning sign.

Ok I’ve moved the emoji from client.en.yml to the JS file and tested with an RTL locale and it looked alright :+1:

Will merge this PR once the build passes.

This pull request has been mentioned on Discourse Meta. There might be relevant details there: