Merge pull request #839 from vector-im/fix-833

Improve error handling in the UI and logs

src/platform/web/Platform.js

@@ -192,7 +192,7 @@ export class Platform {
                     await this._themeLoader?.init(manifests, log);
                     const { themeName, themeVariant } = await this._themeLoader.getActiveTheme();
                     log.log({ l: "Active theme", name: themeName, variant: themeVariant });
-                    this._themeLoader.setTheme(themeName, themeVariant, log);
+                    await this._themeLoader.setTheme(themeName, themeVariant, log);
                 }
             });
         } catch (err) {
@@ -332,7 +332,9 @@ export class Platform {
         return this._themeLoader;
     }
 
-    replaceStylesheet(newPath) {
+    async replaceStylesheet(newPath, log) {
+        const error = await this.logger.wrapOrRun(log, { l: "replaceStylesheet", location: newPath, }, async (l) => {
+            let error;
             const head = document.querySelector("head");
             // remove default theme 
             document.querySelectorAll(".theme").forEach(e => e.remove());
@@ -342,7 +344,23 @@ export class Platform {
             styleTag.rel = "stylesheet";
             styleTag.type = "text/css";
             styleTag.className = "theme";
+            const promise = new Promise(resolve => {
+                styleTag.onerror = () => {
+                    error = new Error(`Failed to load stylesheet from ${newPath}`);
+                    l.catch(error);
+                    resolve();
+                };
+                styleTag.onload = () => {
+                    resolve();
+                };
+            });
             head.appendChild(styleTag);
+            await promise;
+            return error;
+        });
+        if (error) {
+            throw error;
+        }
     }
 
     get description() {

src/platform/web/dom/request/fetch.js

@@ -120,6 +120,7 @@ export function createFetchRequest(createTimeout, serviceWorkerHandler) {
                 }
             } catch (err) {
                 // some error pages return html instead of json, ignore error
+                // detect these ignored errors from the response status 
                 if (!(err.name === "SyntaxError" && status >= 400)) {
                     throw err;
                 }

src/platform/web/theming/ThemeLoader.ts

@@ -14,12 +14,14 @@ See the License for the specific language governing permissions and
 limitations under the License.
 */
 
-import type {ILogItem} from "../../../logging/types";
-import type {Platform} from "../Platform.js";
 import {RuntimeThemeParser} from "./parsers/RuntimeThemeParser";
-import type {Variant, ThemeInformation} from "./parsers/types";
 import {ColorSchemePreference} from "./parsers/types";
 import {BuiltThemeParser} from "./parsers/BuiltThemeParser";
+import type {Variant, ThemeInformation} from "./parsers/types";
+import type {ThemeManifest} from "../../types/theme";
+import type {ILogItem} from "../../../logging/types";
+import type {Platform} from "../Platform.js";
+import {LogLevel} from "../../../logging/LogFilter";
 
 export class ThemeLoader {
     private _platform: Platform;
@@ -32,6 +34,9 @@ export class ThemeLoader {
 
     async init(manifestLocations: string[], log?: ILogItem): Promise<void> {
         await this._platform.logger.wrapOrRun(log, "ThemeLoader.init", async (log) => {
+            let noManifestsAvailable = true;
+            const failedManifestLoads: string[] = [];
+            const parseErrors: string[] = [];
             const results = await Promise.all(
                 manifestLocations.map(location => this._platform.request(location, { method: "GET", format: "json", cache: true, }).response())
             );
@@ -39,14 +44,22 @@ export class ThemeLoader {
             const builtThemeParser = new BuiltThemeParser(this.preferredColorScheme);
             const runtimeThemePromises: Promise<void>[] = [];
             for (let i = 0; i < results.length; ++i) {
-                const { body } = results[i];
+                const result = results[i];
+                const { status, body } = result;
+                if (!(status >= 200 && status <= 299)) {
+                    console.error(`Failed to load manifest at ${manifestLocations[i]}, status: ${status}`);
+                    log.log({ l: "Manifest fetch failed", location: manifestLocations[i], status }, LogLevel.Error);
+                    failedManifestLoads.push(manifestLocations[i])
+                    continue;
+                }
+                noManifestsAvailable = false;
                 try {
                     if (body.extends) {
-                        const indexOfBaseManifest = results.findIndex(manifest => manifest.body.id === body.extends);
+                        const indexOfBaseManifest = results.findIndex(result => "value" in result && result.value.body.id === body.extends);
                         if (indexOfBaseManifest === -1) {
                             throw new Error(`Base manifest for derived theme at ${manifestLocations[i]} not found!`);
                         }
-                        const {body: baseManifest} = results[indexOfBaseManifest];
+                        const { body: baseManifest } = (results[indexOfBaseManifest] as PromiseFulfilledResult<{ body: ThemeManifest }>).value;
                         const baseManifestLocation = manifestLocations[indexOfBaseManifest];
                         const promise = runtimeThemeParser.parse(body, baseManifest, baseManifestLocation, log);
                         runtimeThemePromises.push(promise);
@@ -57,19 +70,27 @@ export class ThemeLoader {
                 }
                 catch(e) {
                     console.error(e);
+                    parseErrors.push(e.message);
                 }
             }
             await Promise.all(runtimeThemePromises);
             this._themeMapping = { ...builtThemeParser.themeMapping, ...runtimeThemeParser.themeMapping };
-            Object.assign(this._themeMapping, builtThemeParser.themeMapping, runtimeThemeParser.themeMapping);
+            if (noManifestsAvailable) {
+                // We need at least one working theme manifest!
+                throw new Error(`All configured theme manifests failed to load, the following were tried: ${failedManifestLoads.join(", ")}`);
+            }
+            else if (Object.keys(this._themeMapping).length === 0 && parseErrors.length) {
+                // Something is wrong..., themeMapping is empty!
+                throw new Error(`Failed to parse theme manifests, the following errors were encountered: ${parseErrors.join(", ")}`);
+            }
             this._addDefaultThemeToMapping(log);
             log.log({ l: "Preferred colorscheme", scheme: this.preferredColorScheme === ColorSchemePreference.Dark ? "dark" : "light" });
             log.log({ l: "Result", themeMapping: this._themeMapping });
         });
     }
 
-    setTheme(themeName: string, themeVariant?: "light" | "dark" | "default", log?: ILogItem) {
+    async setTheme(themeName: string, themeVariant?: "light" | "dark" | "default", log?: ILogItem) {
-        this._platform.logger.wrapOrRun(log, { l: "change theme", name: themeName, variant: themeVariant }, () => {
+        await this._platform.logger.wrapOrRun(log, { l: "change theme", name: themeName, variant: themeVariant }, async (l) => {
             let cssLocation: string, variables: Record<string, string>;
             let themeDetails = this._themeMapping[themeName];
             if ("id" in themeDetails) {
@@ -83,7 +104,7 @@ export class ThemeLoader {
                 cssLocation = themeDetails[themeVariant].cssLocation;
                 variables = themeDetails[themeVariant].variables;
             }
-            this._platform.replaceStylesheet(cssLocation);
+            await this._platform.replaceStylesheet(cssLocation, l);
             if (variables) {
                 log?.log({l: "Derived Theme", variables});
                 this._injectCSSVariables(variables);