create new modeler instance always, but only call getModeler once in …

…useBpmnEditor hook

frontend/packages/process-editor/src/hooks/useBpmnEditor.test.tsx

@@ -8,14 +8,26 @@ import { getBpmnEditorDetailsFromBusinessObject } from '../utils/hookUtils';
 import type { BpmnDetails } from '../types/BpmnDetails';
 import { BpmnTypeEnum } from '../enum/BpmnTypeEnum';
 import type { BpmnTaskType } from '../types/BpmnTaskType';
+import type { LayoutSets } from 'app-shared/types/api/LayoutSetsResponse';
 
+const taskId = 'testId';
+const layoutSetId = 'someLayoutSetId';
 const bpmnDetailsMock: BpmnDetails = {
-  id: 'testId',
+  id: taskId,
   name: 'mockName',
   type: BpmnTypeEnum.Task,
   taskType: 'data',
 };
 
+const layoutSetsMock: LayoutSets = {
+  sets: [
+    {
+      id: layoutSetId,
+      tasks: [taskId],
+    },
+  ],
+};
+
 class BpmnModelerMockImpl {
   public readonly _currentEventName: string;
   private readonly eventBus: any;
@@ -40,12 +52,7 @@ class BpmnModelerMockImpl {
 }
 
 jest.mock('../utils/hookUtils', () => ({
-  getBpmnEditorDetailsFromBusinessObject: jest.fn().mockReturnValue({
-    id: 'testId',
-    name: 'mockName',
-    type: 'bpmn:Task',
-    taskType: 'data',
-  }),
+  getBpmnEditorDetailsFromBusinessObject: jest.fn().mockReturnValue({}),
 }));
 
 jest.mock('../contexts/BpmnConfigPanelContext', () => ({
@@ -74,9 +81,7 @@ const overrideUseBpmnContext = () => {
 };
 
 jest.mock('./useBpmnModeler', () => ({
-  useBpmnModeler: jest.fn().mockReturnValue({
-    getModeler: jest.fn(),
-  }),
+  useBpmnModeler: jest.fn().mockReturnValue({}),
 }));
 
 const overrideUseBpmnModeler = (currentEventName: string) => {
@@ -85,12 +90,17 @@ const overrideUseBpmnModeler = (currentEventName: string) => {
   });
 };
 
+const overrideGetBpmnEditorDetailsFromBusinessObject = (bpmnDetails: BpmnDetails) => {
+  (getBpmnEditorDetailsFromBusinessObject as jest.Mock).mockReturnValue(bpmnDetails);
+};
+
 const wrapper = ({ children }) => (
   <BpmnContextProvider appLibVersion={'8.0.0'}>
     <BpmnApiContextProvider
       addLayoutSet={addLayoutSetMock}
       deleteLayoutSet={deleteLayoutSetMock}
       saveBpmn={saveBpmnMock}
+      layoutSets={layoutSetsMock}
     >
       {children}
     </BpmnApiContextProvider>
@@ -129,35 +139,56 @@ describe('useBpmnEditor', () => {
   it.each(['confirmation', 'signing', 'feedback', 'endEvent'])(
     'should not call saveBpmn when "shape.add" event is triggered on modelerInstance when taskType is not data',
     (taskType: BpmnTaskType) => {
-      const mockBpmnDetailsConfirm: BpmnDetails = {
+      const mockBpmnDetailsNotData: BpmnDetails = {
         id: 'otherTestId',
         name: 'mockName',
         type: BpmnTypeEnum.Task,
         taskType: taskType,
       };
-      (getBpmnEditorDetailsFromBusinessObject as jest.Mock).mockReturnValue(mockBpmnDetailsConfirm);
       const currentEventName = 'shape.add';
-      renderUseBpmnEditor(true, currentEventName);
+      renderUseBpmnEditor(true, currentEventName, mockBpmnDetailsNotData);
 
       expect(addLayoutSetMock).not.toHaveBeenCalled();
       expect(setBpmnDetailsMock).toHaveBeenCalledTimes(1);
-      expect(setBpmnDetailsMock).toHaveBeenCalledWith(mockBpmnDetailsConfirm);
+      expect(setBpmnDetailsMock).toHaveBeenCalledWith(mockBpmnDetailsNotData);
+    },
+  );
+
+  it.each(['data', 'confirmation', 'signing', 'feedback', 'endEvent'])(
+    'should call deleteLayoutSet when "shape.remove" event is triggered on modelerInstance and the task has a connected layout set',
+    (taskType: BpmnTaskType) => {
+      const mockBpmnDetailsWithConnectedLayoutSet: BpmnDetails = {
+        id: taskId,
+        name: 'mockName',
+        type: BpmnTypeEnum.Task,
+        taskType: taskType,
+      };
+      const currentEventName = 'shape.remove';
+      renderUseBpmnEditor(true, currentEventName, mockBpmnDetailsWithConnectedLayoutSet);
+
+      expect(deleteLayoutSetMock).toHaveBeenCalledTimes(1);
+      expect(deleteLayoutSetMock).toHaveBeenCalledWith({ layoutSetIdToUpdate: layoutSetId });
+      expect(setBpmnDetailsMock).toHaveBeenCalled();
+      expect(setBpmnDetailsMock).toHaveBeenCalledWith(null);
     },
   );
 
-  it('should call deleteLayoutSet when "shape.remove" event is triggered on modelerInstance', () => {
-    (getBpmnEditorDetailsFromBusinessObject as jest.Mock).mockReturnValue(bpmnDetailsMock);
+  it('should not call deleteLayoutSet when "shape.remove" event is triggered on modelerInstance and deleted task has no layoutSet', () => {
+    const mockBpmnDetailsWithoutConnectedLayoutSet: BpmnDetails = {
+      id: 'TaskIdNotPresetInLayoutSets',
+      name: 'mockName',
+      type: BpmnTypeEnum.Task,
+      taskType: 'data',
+    };
     const currentEventName = 'shape.remove';
-    renderUseBpmnEditor(true, currentEventName);
+    renderUseBpmnEditor(true, currentEventName, mockBpmnDetailsWithoutConnectedLayoutSet);
 
-    expect(deleteLayoutSetMock).toHaveBeenCalledTimes(1);
-    expect(deleteLayoutSetMock).toHaveBeenCalledWith({ layoutSetIdToUpdate: bpmnDetailsMock.id });
+    expect(deleteLayoutSetMock).not.toHaveBeenCalled();
     expect(setBpmnDetailsMock).toHaveBeenCalled();
     expect(setBpmnDetailsMock).toHaveBeenCalledWith(null);
   });
 
   it('should call setBpmnDetails when "element.click" event is triggered on eventBus', () => {
-    (getBpmnEditorDetailsFromBusinessObject as jest.Mock).mockReturnValue(bpmnDetailsMock);
     const currentEventName = 'element.click';
     renderUseBpmnEditor(true, currentEventName);
 
@@ -169,8 +200,13 @@ describe('useBpmnEditor', () => {
   });
 });
 
-const renderUseBpmnEditor = (overrideBpmnContext: boolean, currentEventName: string) => {
+const renderUseBpmnEditor = (
+  overrideBpmnContext: boolean,
+  currentEventName: string,
+  bpmnDetails = bpmnDetailsMock,
+) => {
   overrideBpmnContext && overrideUseBpmnContext();
+  overrideGetBpmnEditorDetailsFromBusinessObject(bpmnDetails);
   overrideUseBpmnModeler(currentEventName);
   renderHook(() => useBpmnEditor(), { wrapper });
 };

frontend/packages/process-editor/src/hooks/useBpmnEditor.ts

@@ -1,12 +1,13 @@
 import type { MutableRefObject } from 'react';
-import { useEffect, useRef } from 'react';
+import { useCallback, useEffect, useRef } from 'react';
 import type BpmnModeler from 'bpmn-js/lib/Modeler';
 import { useBpmnContext } from '../contexts/BpmnContext';
 import { useBpmnModeler } from './useBpmnModeler';
 import { getBpmnEditorDetailsFromBusinessObject } from '../utils/hookUtils';
 import { useBpmnConfigPanelFormContext } from '../contexts/BpmnConfigPanelContext';
 import { useBpmnApiContext } from '../contexts/BpmnApiContext';
 import { BpmnTypeEnum } from '../enum/BpmnTypeEnum';
+import { getLayoutSetIdFromTaskId } from '../utils/hookUtils/hookUtils';
 
 // Wrapper around bpmn-js to Reactify it
 
@@ -20,23 +21,14 @@ export const useBpmnEditor = (): UseBpmnViewerResult => {
   const canvasRef = useRef<HTMLDivElement | null>(null);
   const { metaDataFormRef, resetForm } = useBpmnConfigPanelFormContext();
   const { getModeler } = useBpmnModeler();
-  const { addLayoutSet, deleteLayoutSet, saveBpmn } = useBpmnApiContext();
+  const { addLayoutSet, deleteLayoutSet, saveBpmn, layoutSets } = useBpmnApiContext();
+  //let modelerRef: BpmnModeler | null = null;
 
   const handleCommandStackChanged = async () => {
     saveBpmn(await getUpdatedXml(), metaDataFormRef.current || null);
     resetForm();
   };
 
-  const handleShapeRemove = (e) => {
-    const bpmnDetails = getBpmnEditorDetailsFromBusinessObject(e?.element?.businessObject);
-    if (bpmnDetails.type === BpmnTypeEnum.Task) {
-      deleteLayoutSet({
-        layoutSetIdToUpdate: bpmnDetails.id,
-      });
-    }
-    setBpmnDetails(null);
-  };
-
   const handleShapeAdd = (e) => {
     const bpmnDetails = getBpmnEditorDetailsFromBusinessObject(e?.element?.businessObject);
     setBpmnDetails(bpmnDetails);
@@ -48,62 +40,68 @@ export const useBpmnEditor = (): UseBpmnViewerResult => {
     }
   };
 
-  const handleSetBpmnDetails = (e) => {
-    const bpmnDetails = {
-      ...getBpmnEditorDetailsFromBusinessObject(e.element?.businessObject),
-      element: e.element,
-    };
-    setBpmnDetails(bpmnDetails);
+  const handleShapeRemove = (e) => {
+    const bpmnDetails = getBpmnEditorDetailsFromBusinessObject(e?.element?.businessObject);
+    if (bpmnDetails.type === BpmnTypeEnum.Task) {
+      const layoutSetId = getLayoutSetIdFromTaskId(bpmnDetails, layoutSets);
+      if (layoutSetId) {
+        deleteLayoutSet({
+          layoutSetIdToUpdate: layoutSetId,
+        });
+      }
+    }
+    setBpmnDetails(null);
+  };
+
+  const handleSetBpmnDetails = useCallback(
+    (e) => {
+      const bpmnDetails = {
+        ...getBpmnEditorDetailsFromBusinessObject(e.element?.businessObject),
+        element: e.element,
+      };
+      setBpmnDetails(bpmnDetails);
+    },
+    [setBpmnDetails],
+  );
+
+  const initializeEditor = async () => {
+    try {
+      await modelerRef.current.importXML(bpmnXml);
+      const canvas: any = modelerRef.current.get('canvas');
+      canvas.zoom('fit-viewport');
+    } catch (exception) {
+      console.log('An error occurred while rendering the viewer:', exception);
+    }
+  };
+
+  const initializeBpmnChanges = () => {
+    modelerRef.current.on('commandStack.changed', async () => {
+      await handleCommandStackChanged();
+    });
+    modelerRef.current.on('shape.add', (e) => {
+      handleShapeAdd(e);
+    });
+    modelerRef.current.on('shape.remove', (e) => {
+      handleShapeRemove(e);
+    });
   };
 
   useEffect(() => {
     if (!canvasRef.current) {
       console.log('Canvas reference is not yet available in the DOM.');
-      return;
     }
-    const modelerInstance: BpmnModeler = getModeler(canvasRef.current);
-
-    // set modelerRef.current to the Context so that it can be used in other components
-    modelerRef.current = modelerInstance;
-
-    const initializeEditor = async () => {
-      try {
-        await modelerInstance.importXML(bpmnXml);
-        const canvas: any = modelerInstance.get('canvas');
-        canvas.zoom('fit-viewport');
-      } catch (exception) {
-        console.log('An error occurred while rendering the viewer:', exception);
-      }
-    };
-
+    // GetModeler can only be fetched from this hook once since the modeler creates a
+    // new instance and will attach the same canvasRef container to all instances it fetches
+    modelerRef.current = getModeler(canvasRef.current);
     initializeEditor();
-  }, [modelerRef]); // Missing dependencies are not added due to resulting wierd behaviour in the process editor
-
-  useEffect(() => {
-    const initializeBpmnChanges = () => {
-      const modelerInstance = getModeler(canvasRef.current);
-
-      modelerInstance.on('commandStack.changed', async () => {
-        await handleCommandStackChanged();
-      });
-
-      modelerInstance.on('shape.add', (e) => {
-        handleShapeAdd(e);
-      });
-
-      modelerInstance.on('shape.remove', (e) => {
-        handleShapeRemove(e);
-      });
-    };
-
     initializeBpmnChanges();
-  }, []); // Missing dependencies are not added due to resulting wierd behaviour in the process editor
+    // set modelerRef.current to the Context so that it can be used in other components
+  }, []); // Missing dependencies are not added to avoid getModeler to be called multiple times
 
   useEffect(() => {
-    const modelerInstance: BpmnModeler = getModeler(canvasRef.current);
-    const eventBus: BpmnModeler = modelerInstance.get('eventBus');
+    const eventBus: BpmnModeler = modelerRef.current.get('eventBus');
     eventBus.on('element.click', handleSetBpmnDetails);
-  }, [getModeler, handleSetBpmnDetails]);
+  }, [modelerRef, handleSetBpmnDetails]);
 
   return { canvasRef, modelerRef };
 };

frontend/packages/process-editor/src/hooks/useBpmnModeler/useBpmnModeler.test.ts

@@ -17,7 +17,6 @@ jest.mock(
 );
 
 const mockedCanvasHTMLDivElement = `<div>MockedHtml</div>` as unknown as HTMLDivElement;
-const mockedCanvasHTMLDivElement2 = `<div>MockedHtml2</div>` as unknown as HTMLDivElement;
 
 describe('useBpmnModeler', () => {
   it('should create instance of the BpmnModeler when calling getModeler', () => {
@@ -27,16 +26,4 @@ describe('useBpmnModeler', () => {
     const modelerInstance = getModeler(mockedCanvasHTMLDivElement) as ModelerMock;
     expect(modelerInstance.container.container).toBe(mockedCanvasHTMLDivElement);
   });
-
-  it('should avoid creating a new instance of the class if it already has an instance', () => {
-    const { result } = renderHook(() => useBpmnModeler());
-    const { getModeler } = result.current;
-
-    const modelerInstance = getModeler(mockedCanvasHTMLDivElement) as ModelerMock;
-    expect(modelerInstance.container.container).toBe(mockedCanvasHTMLDivElement);
-
-    const modelerInstance2 = getModeler(mockedCanvasHTMLDivElement2) as ModelerMock;
-    expect(modelerInstance2.container.container).not.toBe(mockedCanvasHTMLDivElement2);
-    expect(modelerInstance2.container.container).toBe(mockedCanvasHTMLDivElement);
-  });
 });

frontend/packages/process-editor/src/utils/StudioBpmnModeler.ts

@@ -6,20 +6,19 @@ import { altinnCustomTasks } from '../extensions/altinnCustomTasks';
 export class StudioBpmnModeler {
   private static instance: BpmnModeler | null = null;
 
-  // Singleton pattern to ensure only one instance of the StudioBpmnModeler is created
+  // Need to create a new modelerInstance in order to get the updated
+  // canvasContainer when switching between tabs in Studio
   public static getInstance(canvasContainer: HTMLDivElement): BpmnModeler {
-    if (!StudioBpmnModeler.instance) {
-      StudioBpmnModeler.instance = new BpmnModeler({
-        container: canvasContainer,
-        keyboard: {
-          bindTo: document,
-        },
-        additionalModules: [SupportedPaletteProvider, SupportedContextPadProvider],
-        moddleExtensions: {
-          altinn: altinnCustomTasks,
-        },
-      });
-    }
+    StudioBpmnModeler.instance = new BpmnModeler({
+      container: canvasContainer,
+      keyboard: {
+        bindTo: document,
+      },
+      additionalModules: [SupportedPaletteProvider, SupportedContextPadProvider],
+      moddleExtensions: {
+        altinn: altinnCustomTasks,
+      },
+    });
     return StudioBpmnModeler.instance;
   }
 }