#2900: Avoid duplicate reporting in menuItemExtension (#2901)

src/extensionPoints/menuItemExtension.ts

@@ -71,6 +71,7 @@ import sanitize from "@/utils/sanitize";
 import { EXTENSION_POINT_DATA_ATTR } from "@/common";
 import BackgroundLogger from "@/telemetry/BackgroundLogger";
 import reportError from "@/telemetry/reportError";
+import pluralize from "@/utils/pluralize";
 
 interface ShadowDOM {
   mode?: "open" | "closed";
@@ -726,18 +727,13 @@ export abstract class MenuItemExtensionPoint extends ExtensionPoint<MenuItemExte
     }
 
     if (errors.length > 0) {
-      // Report the error to the user. Don't report to to the telemetry service since we already reported
-      // above in the loop
-      console.warn(`An error occurred adding ${errors.length} menu item(s)`, {
-        errors,
+      const subject = pluralize(errors.length, "the button", "$$ buttons");
+      const message = `An error occurred adding ${subject}`;
+      console.warn(message, { errors });
+      this.notifyError({
+        message,
+        reportError: false, // We already reported it in the loop above
       });
-      if (errors.length === 1) {
-        this.notifyError("An error occurred adding the menu item/button");
-      } else {
-        this.notifyError(
-          `An error occurred adding ${errors.length} menu items`
-        );
-      }
     }
   }
 }

src/extensionPoints/triggerExtension.ts

@@ -48,10 +48,11 @@ import apiVersionOptions from "@/runtime/apiVersionOptions";
 import { blockList } from "@/blocks/util";
 import { makeServiceContext } from "@/services/serviceUtils";
 import { mergeReaders } from "@/blocks/readers/readerUtils";
-import { asyncForEach, PromiseCancelled, sleep } from "@/utils";
+import { PromiseCancelled, sleep } from "@/utils";
 import initialize from "@/vendors/initialize";
 import { $safeFind } from "@/helpers";
 import BackgroundLogger from "@/telemetry/BackgroundLogger";
+import pluralize from "@/utils/pluralize";
 
 export type TriggerConfig = {
   action: BlockPipeline | BlockConfig;
@@ -266,6 +267,7 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
       optionsArgs: extension.optionsArgs,
     };
 
+    // FIXME: https://github.com/pixiebrix/pixiebrix-extension/issues/2910
     try {
       await reducePipeline(actionConfig, initialValues, {
         logger: extensionLogger,
@@ -299,10 +301,14 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
       );
     }
 
-    const promises = await Promise.allSettled([this.runTrigger(element)]);
-    TriggerExtensionPoint.notifyErrors(compact(promises));
+    await this.runTriggersAndNotify(element);
   };
 
+  /**
+   * Run all extensions for a given root (i.e., handle the trigger firing)
+   * @return array of errors from the extensions
+   * @throws Error on non-extension error, e.g., reader error for the default reader
+   */
   private async runTrigger(root: ReaderRoot): Promise<unknown[]> {
     const reader = await this.defaultReader();
     const readerContext = await reader.read(root);
@@ -324,14 +330,34 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
     return compact(errors);
   }
 
-  static notifyErrors(results: Array<PromiseSettledResult<unknown[]>>): void {
-    const errors = compact(
-      results.flatMap((x) => (x.status === "fulfilled" ? x.value : [x.reason]))
+  private async runTriggersAndNotify(...roots: ReaderRoot[]): Promise<void> {
+    const promises = roots.map(async (root) => this.runTrigger(root));
+    const results = await Promise.allSettled(promises);
+    const errors = results.flatMap((x) =>
+      // `runTrigger` fulfills with list of extension error from extension, or rejects on other error, e.g., reader
+      // error from the extension point.
+      x.status === "fulfilled" ? x.value : x.reason
     );
-    if (errors.length > 0) {
-      console.debug("Trigger errors", errors);
-      notify.error(`An error occurred running ${errors.length} triggers(s)`);
+
+    TriggerExtensionPoint.notifyErrors(errors);
+  }
+
+  /**
+   * Show notification for errors to the user. Caller is responsible for sending error telemetry.
+   * @param errors
+   */
+  static notifyErrors(errors: unknown[]): void {
+    if (errors.length === 0) {
+      return;
     }
+
+    const subject = pluralize(errors.length, "a trigger", "$$ triggers");
+    const message = `An error occurred running ${subject}`;
+    console.debug(message, { errors });
+    notify.error({
+      message,
+      reportError: false,
+    });
   }
 
   private async getRoot(): Promise<JQuery<HTMLElement | Document>> {
@@ -372,9 +398,7 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
 
       const intervalEffect = async () => {
         const $root = await this.getRoot();
-        await Promise.allSettled(
-          $root.toArray().flatMap(async (root) => this.runTrigger(root))
-        );
+        await this.runTriggersAndNotify(...$root);
       };
 
       void interval({
@@ -401,23 +425,14 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
 
     // The caller will have already waited for the element. So $element will contain at least one element
     if (this.attachMode === "once") {
-      for (const element of $element.get()) {
-        void this.runTrigger(element);
-      }
-
+      void this.runTriggersAndNotify(...$element);
       return;
     }
 
     const observer = initialize(
       this.triggerSelector,
-      async (index, element) => {
-        const errors = await this.runTrigger(element as HTMLElement);
-        if (errors.length > 0) {
-          console.error("An error occurred while running a trigger", {
-            errors,
-          });
-          notify.error("An error occurred while running a trigger");
-        }
+      (index, element: HTMLElement) => {
+        void this.runTriggersAndNotify(element);
       },
       // `target` is a required option
       { target: document }
@@ -434,18 +449,10 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
     // https://developer.mozilla.org/en-US/docs/Web/API/Intersection_Observer_API
     const appearObserver = new IntersectionObserver(
       (entries) => {
-        void asyncForEach(entries, async (entry) => {
-          if (!entry.isIntersecting) {
-            return;
-          }
-
-          const errors = await this.runTrigger(entry.target as HTMLElement);
-          if (errors.length > 0) {
-            const message = "An error occurred while running a trigger";
-            console.error(message, { errors });
-            notify.error({ message, error: errors[0] });
-          }
-        });
+        const roots = entries
+          .filter((x) => x.isIntersecting)
+          .map((x) => x.target as HTMLElement);
+        void this.runTriggersAndNotify(...roots);
       },
       {
         root: null,
@@ -546,10 +553,7 @@ export abstract class TriggerExtensionPoint extends ExtensionPoint<TriggerConfig
 
     switch (this.trigger) {
       case "load": {
-        const promises = await Promise.allSettled(
-          $root.toArray().flatMap(async (root) => this.runTrigger(root))
-        );
-        TriggerExtensionPoint.notifyErrors(promises);
+        await this.runTriggersAndNotify(...$root);
         break;
       }
 

src/telemetry/BackgroundLogger.ts

@@ -17,7 +17,7 @@
 
 import { Logger, MessageContext } from "@/core";
 import { JsonObject } from "type-fest";
-import { getErrorMessage, isConnectionError } from "@/errors";
+import { isConnectionError, selectError } from "@/errors";
 import { getContextName, isContentScript } from "webext-detect-page";
 import { showConnectionLost } from "@/contentScript/connection";
 import { serializeError } from "serialize-error";
@@ -71,8 +71,8 @@ class BackgroundLogger implements Logger {
   }
 
   async error(error: unknown, data: JsonObject): Promise<void> {
-    console.error("An error occurred: %s", getErrorMessage(error), {
-      error,
+    // Use selectError which returns an error object ot avoid user-defined format strings
+    console.error(selectError(error), {
       context: this.context,
       data,
     });

src/utils/notify.tsx

@@ -185,7 +185,11 @@ export function notifyResult(
   extensionId: string,
   { message, config: { className } }: MessageConfig
 ): void {
-  showNotification({ message, type: className as NotificationType });
+  showNotification({
+    message,
+    type: className as NotificationType,
+    reportError: false,
+  });
 }
 
 // Private method to prevent adding logic to the `notify.*` helpers.

src/utils/pluralize.ts

@@ -0,0 +1,22 @@
+// From: https://github.com/refined-github/refined-github/blob/4232313404c9d4afaa711217ce539680b7e3799e/source/helpers/pluralize.ts
+
+function regular(single: string): string {
+  return single + "s";
+}
+
+export default function pluralize(
+  count: number,
+  single: string,
+  plural = regular(single),
+  zero?: string
+): string {
+  if (count === 0 && zero) {
+    return zero.replace("$$", "0");
+  }
+
+  if (count === 1) {
+    return single.replace("$$", "1");
+  }
+
+  return plural.replace("$$", String(count));
+}