Merge pull request #942 from mittwald/fix/MultipleOverlayControllerUsage

fix(OverlayController): fix overlayController handler management

packages/components/src/lib/controller/overlay/OverlayController.ts

@@ -2,16 +2,19 @@ import { action, makeObservable, observable } from "mobx";
 import useSelector from "@/lib/mobx/useSelector";
 import { useRef } from "react";
 
+type OpenStateHandler = () => void;
+type DisposerFn = () => void;
+
 export interface OverlayControllerOptions {
   isDefaultOpen?: boolean;
-  onOpen?: () => void;
-  onClose?: () => void;
+  onOpen?: OpenStateHandler;
+  onClose?: OpenStateHandler;
 }
 
 export class OverlayController {
   public isOpen: boolean;
-  public onOpen?: () => void;
-  public onClose?: () => void;
+  private onOpenHandlers = new Set<OpenStateHandler>();
+  private onCloseHandlers = new Set<OpenStateHandler>();
 
   public constructor(options?: OverlayControllerOptions) {
     makeObservable(this, {
@@ -22,41 +25,68 @@ export class OverlayController {
       setOpen: action.bound,
     });
     this.isOpen = options?.isDefaultOpen ?? false;
-    this.onOpen = options?.onOpen;
-    this.onClose = options?.onClose;
+
+    if (options?.onOpen) {
+      this.onOpenHandlers.add(options.onOpen);
+    }
+    if (options?.onClose) {
+      this.onCloseHandlers.add(options.onClose);
+    }
   }
 
   public static useNew(options?: OverlayControllerOptions): OverlayController {
     return useRef(new OverlayController(options)).current;
   }
 
+  public addOnOpen(handler: OpenStateHandler): DisposerFn {
+    this.onOpenHandlers.add(handler);
+    return () => {
+      this.onOpenHandlers.delete(handler);
+    };
+  }
+
+  public addOnClose(handler: OpenStateHandler): DisposerFn {
+    this.onCloseHandlers.add(handler);
+    return () => {
+      this.onCloseHandlers.delete(handler);
+    };
+  }
+
+  private executeOnClose(): void {
+    this.onCloseHandlers.forEach((handler) => handler());
+  }
+
+  private executeOnOpen(): void {
+    this.onOpenHandlers.forEach((handler) => handler());
+  }
+
   public open(): void {
     this.isOpen = true;
-    this.onOpen?.();
+    this.executeOnOpen();
   }
 
   public close(): void {
     this.isOpen = false;
-    this.onClose?.();
+    this.executeOnClose();
   }
 
   public toggle(): void {
     this.isOpen = !this.isOpen;
 
     if (this.isOpen) {
-      this.onOpen?.();
+      this.executeOnOpen();
     } else {
-      this.onClose?.();
+      this.executeOnClose();
     }
   }
 
   public setOpen(to: boolean): void {
     this.isOpen = to;
 
     if (to) {
-      this.onOpen?.();
+      this.executeOnOpen();
     } else {
-      this.onClose?.();
+      this.executeOnClose();
     }
   }
 

packages/components/src/lib/controller/overlay/useOverlayController.test.ts

@@ -0,0 +1,143 @@
+import { renderHook, act } from "@testing-library/react";
+import { useOverlayController } from "@/lib/controller";
+import { useOverlayContext } from "@/lib/controller/overlay/context";
+import { OverlayController } from "@/lib/controller";
+import type { Mock } from "vitest";
+import { vitest, describe, test, expect, beforeEach } from "vitest";
+
+vitest.mock("@/lib/controller/overlay/context", () => ({
+  useOverlayContext: vitest.fn(),
+}));
+
+describe("useOverlayController", () => {
+  let mockOnOpen: Mock;
+  let mockOnClose: Mock;
+  let contextController: OverlayController;
+
+  beforeEach(() => {
+    mockOnOpen = vitest.fn();
+    mockOnClose = vitest.fn();
+    contextController = new OverlayController();
+
+    (useOverlayContext as Mock).mockReturnValue({
+      Modal: contextController,
+    });
+  });
+
+  test("should use controller from context when reuseControllerFromContext is true", () => {
+    const { result } = renderHook(() =>
+      useOverlayController("Modal", {
+        reuseControllerFromContext: true,
+        onOpen: mockOnOpen,
+        onClose: mockOnClose,
+      }),
+    );
+
+    expect(result.current).toBe(contextController);
+  });
+
+  test("should create new controller when reuseControllerFromContext is false", () => {
+    const { result } = renderHook(() =>
+      useOverlayController("Modal", {
+        reuseControllerFromContext: false,
+        onOpen: mockOnOpen,
+        onClose: mockOnClose,
+      }),
+    );
+
+    expect(result.current).not.toBe(contextController);
+  });
+
+  test("should add onOpen handler when controller is closed", () => {
+    const { result } = renderHook(() =>
+      useOverlayController("Modal", {
+        onOpen: mockOnOpen,
+      }),
+    );
+
+    act(() => {
+      result.current.open();
+    });
+
+    expect(mockOnOpen).toHaveBeenCalledTimes(1);
+  });
+
+  test("should add onClose handler when controller is open", () => {
+    contextController.open();
+
+    const { result } = renderHook(() =>
+      useOverlayController("Modal", {
+        onClose: mockOnClose,
+      }),
+    );
+
+    act(() => {
+      result.current.close();
+    });
+
+    expect(mockOnClose).toHaveBeenCalledTimes(1);
+  });
+
+  test("should cleanup handlers on unmount", () => {
+    const { unmount } = renderHook(() =>
+      useOverlayController("Modal", {
+        onOpen: mockOnOpen,
+        onClose: mockOnClose,
+      }),
+    );
+
+    unmount();
+
+    act(() => {
+      contextController.open();
+      contextController.close();
+    });
+
+    expect(mockOnOpen).not.toHaveBeenCalled();
+    expect(mockOnClose).not.toHaveBeenCalled();
+  });
+
+  test("should update handlers when options change", () => {
+    const newMockOnOpen = vitest.fn();
+    const { rerender } = renderHook(
+      (props) => useOverlayController("Modal", props),
+      {
+        initialProps: { onOpen: mockOnOpen },
+      },
+    );
+
+    rerender({ onOpen: newMockOnOpen });
+
+    act(() => {
+      contextController.open();
+    });
+
+    expect(mockOnOpen).not.toHaveBeenCalled();
+    expect(newMockOnOpen).toHaveBeenCalledTimes(1);
+  });
+
+  test("should handle multiple handlers correctly", () => {
+    const secondOnOpen = vitest.fn();
+
+    renderHook(() => useOverlayController("Modal", { onOpen: mockOnOpen }));
+    renderHook(() => useOverlayController("Modal", { onOpen: secondOnOpen }));
+
+    act(() => {
+      contextController.open();
+    });
+
+    expect(mockOnOpen).toHaveBeenCalledTimes(1);
+    expect(secondOnOpen).toHaveBeenCalledTimes(1);
+  });
+
+  test("should handle undefined handlers gracefully", () => {
+    const { result } = renderHook(() => useOverlayController("Modal", {}));
+
+    expect(() => {
+      act(() => {
+        result.current.open();
+        result.current.close();
+      });
+    }).not.toThrow();
+  });
+});

packages/components/src/lib/controller/overlay/useOverlayController.ts

@@ -2,6 +2,7 @@ import { useOverlayContext } from "@/lib/controller/overlay/context";
 import type { OverlayControllerOptions } from "@/lib/controller/overlay/OverlayController";
 import { OverlayController } from "@/lib/controller/overlay/OverlayController";
 import type { FlowComponentName } from "@/components/propTypes";
+import { useEffect } from "react";
 
 interface Options extends OverlayControllerOptions {
   reuseControllerFromContext?: boolean;
@@ -25,7 +26,26 @@ export const useOverlayController = (
   });
   const controllerFromContext = useOverlayContext()[overlayType];
 
-  return reuseControllerFromContext && controllerFromContext
-    ? controllerFromContext
-    : newController;
+  const controller =
+    reuseControllerFromContext && controllerFromContext
+      ? controllerFromContext
+      : newController;
+
+  useEffect(() => {
+    const disposers: (() => void)[] = [];
+
+    if (onOpen) {
+      disposers.push(controller.addOnOpen(onOpen));
+    }
+
+    if (onClose) {
+      disposers.push(controller.addOnClose(onClose));
+    }
+
+    return () => {
+      disposers.forEach((dispose) => dispose());
+    };
+  }, [onOpen, onClose]);
+
+  return controller;
 };