Change onBeforeRun API (#3981)

Co-authored-by: Sebastian Malton <sebastian@malton.name>

src/renderer/api/catalog-entity-registry.ts

@@ -19,7 +19,7 @@
  * CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE SOFTWARE.
  */
 
-import { computed, observable, makeObservable, action, ObservableSet } from "mobx";
+import { computed, observable, makeObservable, action } from "mobx";
 import { ipcRendererOn } from "../../common/ipc";
 import { CatalogCategory, CatalogEntity, CatalogEntityData, catalogCategoryRegistry, CatalogCategoryRegistry, CatalogEntityKindData } from "../../common/catalog";
 import "../../common/catalog-entities";
@@ -29,11 +29,10 @@ import { Disposer, iter } from "../utils";
 import { once } from "lodash";
 import logger from "../../common/logger";
 import { catalogEntityRunContext } from "./catalog-entity";
+import { CatalogRunEvent } from "../../common/catalog/catalog-run-event";
 
 export type EntityFilter = (entity: CatalogEntity) => any;
-export type CatalogEntityOnBeforeRun = (entity: CatalogEntity) => boolean | Promise<boolean>;
-
-type CatalogEntityUid = CatalogEntity["metadata"]["uid"];
+export type CatalogEntityOnBeforeRun = (event: CatalogRunEvent) => void | Promise<void>;
 
 export class CatalogEntityRegistry {
   @observable protected activeEntityId: string | undefined = undefined;
@@ -41,7 +40,7 @@ export class CatalogEntityRegistry {
   protected filters = observable.set<EntityFilter>([], {
     deep: false,
   });
-  protected onBeforeRunHooks = observable.map<CatalogEntityUid, ObservableSet<CatalogEntityOnBeforeRun>>({}, {
+  protected onBeforeRunHooks = observable.set<CatalogEntityOnBeforeRun>([], {
     deep: false,
   });
 
@@ -179,49 +178,36 @@ export class CatalogEntityRegistry {
   }
 
   /**
-   * Add a onBeforeRun hook to a catalog entity. If `onBeforeRun` was previously added then it will not be added again
-   * @param catalogEntityUid The uid of the catalog entity
+   * Add a onBeforeRun hook. If `onBeforeRun` was previously added then it will not be added again
    * @param onBeforeRun The function that should return a boolean if the onRun of catalog entity should be triggered.
    * @returns A function to remove that hook
    */
-  addOnBeforeRun(entityOrId: CatalogEntity | CatalogEntityUid, onBeforeRun: CatalogEntityOnBeforeRun): Disposer {
-    logger.debug(`[CATALOG-ENTITY-REGISTRY]: adding onBeforeRun to ${entityOrId}`);
+  addOnBeforeRun(onBeforeRun: CatalogEntityOnBeforeRun): Disposer {
+    logger.debug(`[CATALOG-ENTITY-REGISTRY]: adding onBeforeRun hook`);
 
-    const id = typeof entityOrId === "string"
-      ? entityOrId
-      : entityOrId.getId();
-    const hooks = this.onBeforeRunHooks.get(id) ??
-      this.onBeforeRunHooks.set(id, observable.set([], { deep: false })).get(id);
-      
-    hooks.add(onBeforeRun);
-  
-    return once(() => void hooks.delete(onBeforeRun));
+    this.onBeforeRunHooks.add(onBeforeRun);
+
+    return once(() => void this.onBeforeRunHooks.delete(onBeforeRun));
   }
 
   /**
-   * Runs all the registered `onBeforeRun` hooks, short circuiting on the first falsy returned/resolved valued
+   * Runs all the registered `onBeforeRun` hooks, short circuiting on the first event that's preventDefaulted
    * @param entity The entity to run the hooks on
    * @returns Whether the entities `onRun` method should be executed
    */
   async onBeforeRun(entity: CatalogEntity): Promise<boolean> {
     logger.debug(`[CATALOG-ENTITY-REGISTRY]: run onBeforeRun on ${entity.getId()}`);
-    
-    const hooks = this.onBeforeRunHooks.get(entity.getId());
 
-    if (!hooks) {
-      return true;
-    }
+    const runEvent = new CatalogRunEvent({ target: entity });
 
-    for (const onBeforeRun of hooks) {
-      try {
-        if (!await onBeforeRun(entity)) {
-          return false;
-        }
+    for (const onBeforeRun of this.onBeforeRunHooks) {
+      try { 
+        await onBeforeRun(runEvent);
       } catch (error) {
         logger.warn(`[CATALOG-ENTITY-REGISTRY]: entity ${entity.getId()} onBeforeRun threw an error`, error);
+      }
 
-        // If a handler throws treat it as if it has returned `false`
-        // Namely: assume that its internal logic has failed and didn't complete as expected
+      if (runEvent.defaultPrevented) {
         return false;
       }
     }

src/renderer/components/+catalog/catalog.test.tsx

@@ -129,9 +129,8 @@ describe("<Catalog />", () => {
       .mockImplementation(() => catalogEntityItem);
 
     catalogEntityRegistry.addOnBeforeRun(
-      catalogEntityUid,
-      (entity) => {
-        expect(entity).toMatchInlineSnapshot(`
+      (event) => {
+        expect(event.target).toMatchInlineSnapshot(`
           Object {
             "apiVersion": "api",
             "enabled": true,
@@ -159,8 +158,6 @@ describe("<Catalog />", () => {
           expect(onRun).toHaveBeenCalled();
           done();
         }, 500);
-
-        return true;
       }
     );
 
@@ -176,7 +173,7 @@ describe("<Catalog />", () => {
     userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
   });
 
-  it("onBeforeRun return false => onRun wont be triggered", (done) => {
+  it("onBeforeRun prevents event => onRun wont be triggered", (done) => {
     const catalogCategoryRegistry = new CatalogCategoryRegistry();
     const catalogEntityRegistry = new CatalogEntityRegistry(catalogCategoryRegistry);
     const catalogEntityStore = new CatalogEntityStore(catalogEntityRegistry);
@@ -193,14 +190,12 @@ describe("<Catalog />", () => {
       .mockImplementation(() => catalogEntityItem);
 
     catalogEntityRegistry.addOnBeforeRun(
-      catalogEntityUid,
-      () => {
+      (e) => {
         setTimeout(() => {
           expect(onRun).not.toHaveBeenCalled();
           done();
         }, 500);
-
-        return false;
+        e.preventDefault();
       }
     );
 
@@ -216,7 +211,7 @@ describe("<Catalog />", () => {
     userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
   });
 
-  it("addOnBeforeRun throw an exception => onRun wont be triggered", (done) => {
+  it("addOnBeforeRun throw an exception => onRun will be triggered", (done) => {
     const catalogCategoryRegistry = new CatalogCategoryRegistry();
     const catalogEntityRegistry = new CatalogEntityRegistry(catalogCategoryRegistry);
     const catalogEntityStore = new CatalogEntityStore(catalogEntityRegistry);
@@ -233,10 +228,9 @@ describe("<Catalog />", () => {
       .mockImplementation(() => catalogEntityItem);
 
     catalogEntityRegistry.addOnBeforeRun(
-      catalogEntityUid,
       () => {
         setTimeout(() => {
-          expect(onRun).not.toHaveBeenCalled();
+          expect(onRun).toHaveBeenCalled();
           done();
         }, 500);
 
@@ -256,7 +250,7 @@ describe("<Catalog />", () => {
     userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
   });
 
-  it("addOnRunHook return a promise and resolve true => onRun()", (done) => {
+  it("addOnRunHook return a promise and does not prevent run event => onRun()", (done) => {
     const catalogCategoryRegistry = new CatalogCategoryRegistry();
     const catalogEntityRegistry = new CatalogEntityRegistry(catalogCategoryRegistry);
     const catalogEntityStore = new CatalogEntityStore(catalogEntityRegistry);
@@ -273,9 +267,8 @@ describe("<Catalog />", () => {
       .mockImplementation(() => catalogEntityItem);
 
     catalogEntityRegistry.addOnBeforeRun(
-      catalogEntityUid,
       async () => {
-        return true;
+        // no op
       }
     );
 
@@ -291,7 +284,7 @@ describe("<Catalog />", () => {
     userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
   });
 
-  it("addOnRunHook return a promise and resolve false => onRun() wont be triggered", (done) => {
+  it("addOnRunHook return a promise and prevents event wont be triggered", (done) => {
     const catalogCategoryRegistry = new CatalogCategoryRegistry();
     const catalogEntityRegistry = new CatalogEntityRegistry(catalogCategoryRegistry);
     const catalogEntityStore = new CatalogEntityStore(catalogEntityRegistry);
@@ -308,8 +301,7 @@ describe("<Catalog />", () => {
       .mockImplementation(() => catalogEntityItem);
 
     catalogEntityRegistry.addOnBeforeRun(
-      catalogEntityUid,
-      async () => {
+      async (e) => {
         expect(onRun).not.toBeCalled();
 
         setTimeout(() => {
@@ -317,7 +309,7 @@ describe("<Catalog />", () => {
           done();
         }, 500);
 
-        return false;
+        e.preventDefault();
       }
     );
 
@@ -333,7 +325,7 @@ describe("<Catalog />", () => {
     userEvent.click(screen.getByTestId("detail-panel-hot-bar-icon"));
   });
 
-  it("addOnRunHook return a promise and reject => onRun wont be triggered", (done) => {
+  it("addOnRunHook return a promise and reject => onRun will be triggered", (done) => {
     const catalogCategoryRegistry = new CatalogCategoryRegistry();
     const catalogEntityRegistry = new CatalogEntityRegistry(catalogCategoryRegistry);
     const catalogEntityStore = new CatalogEntityStore(catalogEntityRegistry);
@@ -350,10 +342,9 @@ describe("<Catalog />", () => {
       .mockImplementation(() => catalogEntityItem);
 
     catalogEntityRegistry.addOnBeforeRun(
-      catalogEntityUid,
       async () => {
         setTimeout(() => {
-          expect(onRun).not.toHaveBeenCalled();
+          expect(onRun).toHaveBeenCalled();
           done();
         }, 500);