From 24f5f9950afb432c98c815387c9f41786f1ea667 Mon Sep 17 00:00:00 2001
From: "Hung-Han (Henry) Chen" <chenhungh@gmail.com>
Date: Tue, 5 Oct 2021 09:39:21 +0300
Subject: [PATCH] Catch the cases where onRunHook throws or reject

Signed-off-by: Hung-Han (Henry) Chen <chenhungh@gmail.com>
---
 .../+catalog/catalog-entity-item.tsx          | 24 +++--
 .../components/+catalog/catalog.test.tsx      | 88 +++++++++++++++++++
 2 files changed, 107 insertions(+), 5 deletions(-)

diff --git a/src/renderer/components/+catalog/catalog-entity-item.tsx b/src/renderer/components/+catalog/catalog-entity-item.tsx
index 773b42113f..6259020544 100644
--- a/src/renderer/components/+catalog/catalog-entity-item.tsx
+++ b/src/renderer/components/+catalog/catalog-entity-item.tsx
@@ -33,6 +33,8 @@ import { toJS } from "mobx";
 
 const css = makeCss(styles);
 
+const isPromise = (obj: any): obj is Promise<any> => (obj?.then && typeof obj?.then === "function") ? true: false;
+
 export class CatalogEntityItem<T extends CatalogEntity> implements ItemObject {
   constructor(public entity: T) {}
 
@@ -112,11 +114,23 @@ export class CatalogEntityItem<T extends CatalogEntity> implements ItemObject {
     }
 
     if (typeof onRunHook === "function") {
-      // if onRunHook() returns a Promise, we wait for it to resolve
-      // if not, just take whatever it returns
-      Promise.resolve(onRunHook(toJS(this.entity))).then((shouldRun) => {
-        if (shouldRun) this.entity.onRun(ctx);
-      });      
+      let shouldRun;
+
+      try {
+        shouldRun = onRunHook(toJS(this.entity));
+      } catch (error) {
+        if (process?.env?.NODE_ENV !== "test") console.warn(`[CATALOG-ENTITY-ITEM] onRunHook of entity.metadata.uid ${this.entity.metadata.uid} throw an exception, stop before onRun`, error);
+      }
+
+      if (isPromise(shouldRun)) {
+        Promise.resolve(shouldRun).then((shouldRun) => {
+          if (shouldRun) this.entity.onRun(ctx);
+        }).catch((error) => {
+          if (process?.env?.NODE_ENV !== "test") console.warn(`[CATALOG-ENTITY-ITEM] onRunHook of entity.metadata.uid ${this.entity.metadata.uid} rejects, stop before onRun`, error);
+        }); 
+      }  else if (shouldRun) {
+        this.entity.onRun(ctx);
+      }
     }
   }
 
diff --git a/src/renderer/components/+catalog/catalog.test.tsx b/src/renderer/components/+catalog/catalog.test.tsx
index 4d4a105e40..4f77fc16e9 100644
--- a/src/renderer/components/+catalog/catalog.test.tsx
+++ b/src/renderer/components/+catalog/catalog.test.tsx
@@ -229,6 +229,50 @@ describe("<Catalog />", () => {
     userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
   });
 
+  it("addOnRunHook throw an exception => onRun wont be triggered", (done) => {
+    const onRun = jest.fn();
+    const catalogEntityItem = new CatalogEntityItem({
+      ...catalogEntity,
+      ...catalogEntityItemMethods,
+      onRun,
+    });
+
+    const catalogCategoryRegistry = new CatalogCategoryRegistry();
+    const catalogEntityRegistry = new CatalogEntityRegistry(catalogCategoryRegistry);
+    const catalogEntityStore = new CatalogEntityStore(catalogEntityRegistry);
+
+    // mock as if there is a selected item > the detail panel opens
+    jest
+      .spyOn(catalogEntityStore, "selectedItem", "get")
+      .mockImplementation(() => {
+        return catalogEntityItem;
+      });
+
+    const onRunDisposer = catalogEntityRegistry.addOnRunHook(
+      catalogEntityUid,
+      () => {
+        onRunDisposer?.();
+        setTimeout(() => {
+          expect(onRun).not.toHaveBeenCalled();
+          done();
+        }, 500);
+
+        throw new Error("error!");
+      }
+    );
+
+    render(
+      <Catalog
+        history={history}
+        location={mockLocation}
+        match={mockMatch}
+        catalogEntityStore={catalogEntityStore}
+      />
+    );
+
+    userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
+  });
+
   it("addOnRunHook return a promise and resolve true => onRun()", (done) => {
     let onRunTriggered = false;
     const onRun = () => {
@@ -322,4 +366,48 @@ describe("<Catalog />", () => {
 
     userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
   });
+
+  it("addOnRunHook return a promise and reject => onRun wont be triggered", (done) => {
+    const onRun = jest.fn();
+    const catalogEntityItem = new CatalogEntityItem({
+      ...catalogEntity,
+      ...catalogEntityItemMethods,
+      onRun,
+    });
+
+    const catalogCategoryRegistry = new CatalogCategoryRegistry();
+    const catalogEntityRegistry = new CatalogEntityRegistry(catalogCategoryRegistry);
+    const catalogEntityStore = new CatalogEntityStore(catalogEntityRegistry);
+
+    // mock as if there is a selected item > the detail panel opens
+    jest
+      .spyOn(catalogEntityStore, "selectedItem", "get")
+      .mockImplementation(() => {
+        return catalogEntityItem;
+      });
+
+    const onRunDisposer = catalogEntityRegistry.addOnRunHook(
+      catalogEntityUid,
+      async () => {
+        onRunDisposer?.();
+        setTimeout(() => {
+          expect(onRun).not.toHaveBeenCalled();
+          done();
+        }, 500);
+
+        throw new Error("rejection!");
+      }
+    );
+
+    render(
+      <Catalog
+        history={history}
+        location={mockLocation}
+        match={mockMatch}
+        catalogEntityStore={catalogEntityStore}
+      />
+    );
+
+    userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
+  });
 });