fix: improve file picker reliability

Related to #1239
bpmn-io · Sep 5, 2024 · c4bdaac · c4bdaac
1 parent 596aff5
commit c4bdaac
Show file tree

Hide file tree

Showing 9 changed files with 109 additions and 192 deletions.
diff --git a/packages/form-js-viewer/src/Form.js b/packages/form-js-viewer/src/Form.js
@@ -168,7 +168,7 @@ export class Form {
 
     const errors = this.validate();
 
-    const files = this.get('fileRegistry').getAllFiles();
+    const files = this.get('fileRegistry').getSubmitFiles();
 
     const result = {
       data,

diff --git a/packages/form-js-viewer/src/core/FormFieldInstanceRegistry.js b/packages/form-js-viewer/src/core/FormFieldInstanceRegistry.js
@@ -10,18 +10,15 @@ export class FormFieldInstanceRegistry {
   }
 
   syncInstance(instanceId, formFieldInfo) {
-    const { id, expressionContextInfo, valuePath, indexes, hidden } = formFieldInfo;
+    const { hidden, ...restInfo } = formFieldInfo;
 
     const instanceIsExpected = !hidden;
     const instanceExists = this._formFieldInstances[instanceId];
 
     if (instanceIsExpected && !instanceExists) {
       this._formFieldInstances[instanceId] = {
-        id,
         instanceId,
-        expressionContextInfo,
-        valuePath,
-        indexes,
+        ...restInfo,
       };
 
       this._eventBus.fire('formFieldInstance.added', { instanceId });
@@ -30,20 +27,14 @@ export class FormFieldInstanceRegistry {
 
       this._eventBus.fire('formFieldInstance.removed', { instanceId });
     } else if (instanceIsExpected && instanceExists) {
-      const instanceChanged =
-        this._formFieldInstances[instanceId].id !== id ||
-        // todo: not a new problem, but this almost always changes due to fact that we don't trim the expression context based on used variables
-        this._formFieldInstances[instanceId].expressionContextInfo !== expressionContextInfo ||
-        this._formFieldInstances[instanceId].valuePath !== valuePath ||
-        this._formFieldInstances[instanceId].indexes !== indexes;
+      const instanceChanged = Object.keys(restInfo).some((key) => {
+        return this._formFieldInstances[instanceId][key] !== restInfo[key];
+      });
 
       if (instanceChanged) {
         this._formFieldInstances[instanceId] = {
-          id,
           instanceId,
-          expressionContextInfo,
-          valuePath,
-          indexes,
+          ...restInfo,
         };
 
         this._eventBus.fire('formFieldInstance.changed', { instanceId });

diff --git a/packages/form-js-viewer/src/features/repeatRender/RepeatRenderManager.js b/packages/form-js-viewer/src/features/repeatRender/RepeatRenderManager.js
@@ -13,20 +13,18 @@ import DeleteSvg from '../../render/components/form-fields/icons/Delete.svg';
 import { buildExpressionContext } from '../../util';
 import { useScrollIntoView } from '../../render/hooks';
 import classNames from 'classnames';
-import { valuePathArrayToString, pathStringToValuePathArray } from '../../util/objectPath';
-import { FILE_PICKER_FILE_KEY_PREFIX } from '../../util/constants/FilePickerConstants';
 
 export class RepeatRenderManager {
-  constructor(form, formFields, formFieldRegistry, pathRegistry, fileRegistry) {
+  constructor(form, formFields, formFieldRegistry, pathRegistry, eventBus) {
     this._form = form;
     /** @type {import('../../render/FormFields').FormFields} */
     this._formFields = formFields;
     /** @type {import('../../core/FormFieldRegistry').FormFieldRegistry} */
     this._formFieldRegistry = formFieldRegistry;
     /** @type {import('../../core/PathRegistry').PathRegistry} */
     this._pathRegistry = pathRegistry;
-    /** @type {import('../../render/FileRegistry').FileRegistry} */
-    this._fileRegistry = fileRegistry;
+    /** @type {import('../../core/EventBus').EventBus} */
+    this._eventBus = eventBus;
     this.Repeater = this.Repeater.bind(this);
     this.RepeatFooter = this.RepeatFooter.bind(this);
   }
@@ -53,7 +51,6 @@ export class RepeatRenderManager {
     const [sharedRepeatState] = useSharedState;
 
     const { data } = this._form._getState();
-    const fileRegistry = this._fileRegistry;
 
     const repeaterField = props.field;
     const dataPath = this._pathRegistry.getValuePath(repeaterField, { indexes });
@@ -69,81 +66,18 @@ export class RepeatRenderManager {
     const displayValues = isCollapsed ? values.slice(0, nonCollapsedItems) : values;
     const hiddenValues = isCollapsed ? values.slice(nonCollapsedItems) : [];
 
-    /**
-     * @param {number} index
-     * @typedef Diff
-     * @property {[string, string][]} keysToUpdate
-     * @property {string[]} keysToDelete
-     *
-     * @returns {Diff}
-     */
-    const getFilesKeyDiff = (index) => {
-      const listPrefix = `${FILE_PICKER_FILE_KEY_PREFIX}${valuePathArrayToString(dataPath)}`;
-      const itemPrefix = `${FILE_PICKER_FILE_KEY_PREFIX}${valuePathArrayToString([...dataPath, index])}`;
-      const availableKeys = fileRegistry.getKeys();
-
-      const keysToDelete = availableKeys.filter((key) => key.startsWith(itemPrefix));
-
-      /** @type {[string, string][]} */
-      const keysToUpdate = availableKeys
-        .map((key) => {
-          if (key.startsWith(listPrefix)) {
-            return pathStringToValuePathArray(key.replace(listPrefix, ''));
-          }
-
-          return key;
-        })
-        .filter((path) => path.length > 0 && typeof path[0] === 'number' && path[0] > index)
-        .map((/** @type {number[]} */ path) => {
-          const oldKey = `${listPrefix}${valuePathArrayToString(path)}`;
-
-          path[0]--;
-
-          return [oldKey, `${listPrefix}${valuePathArrayToString(path)}`];
-        });
-
-      return { keysToDelete, keysToUpdate };
-    };
-
-    /**
-     * @param {Diff} diff
-     */
-    const updateFiles = (diff) => {
-      diff.keysToDelete.forEach((key) => {
-        fileRegistry.deleteFiles(key);
-      });
-      diff.keysToUpdate.forEach(([oldKey, newKey]) => {
-        const files = fileRegistry.getFiles(oldKey);
-        fileRegistry.deleteFiles(oldKey);
-        fileRegistry.setFiles(newKey, files);
-      });
-    };
-
-    /**
-     * @param {string} value
-     * @param {[string, string][]} diff
-     * @returns {string}
-     */
-    const updateFileKeyValues = (value, diff) => {
-      return diff.reduce((acc, [oldKey, newKey]) => {
-        return acc.replace(oldKey, newKey);
-      }, value);
-    };
-
     /**
      * @param {number} index
      */
     const onDeleteItem = (index) => {
       const updatedValues = values.slice();
-      updatedValues.splice(index, 1);
+      const removedItem = updatedValues.splice(index, 1)[0];
 
-      const diff = getFilesKeyDiff(index);
-
-      updateFiles(diff);
+      this._eventBus.fire('repeatRenderManager.remove', { dataPath, index, item: removedItem });
 
       props.onChange({
         field: repeaterField,
-        value: JSON.parse(updateFileKeyValues(JSON.stringify(updatedValues), diff.keysToUpdate)),
+        value: updatedValues,
         indexes,
       });
     };
@@ -222,6 +156,8 @@ export class RepeatRenderManager {
 
       shouldScroll.current = true;
 
+      this._eventBus.fire('repeatRenderManager.add', { dataPath, index: updatedValues.length - 1, item: newItem });
+
       props.onChange({
         value: updatedValues,
       });
@@ -353,4 +289,4 @@ const RepetitionScaffold = (props) => {
   );
 };
 
-RepeatRenderManager.$inject = ['form', 'formFields', 'formFieldRegistry', 'pathRegistry', 'fileRegistry'];
+RepeatRenderManager.$inject = ['form', 'formFields', 'formFieldRegistry', 'pathRegistry', 'eventBus'];
diff --git a/packages/form-js-viewer/src/features/repeatRender/index.js b/packages/form-js-viewer/src/features/repeatRender/index.js
@@ -1,10 +1,8 @@
 import { RepeatRenderManager } from './RepeatRenderManager';
-import { FileRegistry } from '../../render/FileRegistry';
 
 export const RepeatRenderModule = {
   __init__: ['repeatRenderManager'],
   repeatRenderManager: ['type', RepeatRenderManager],
-  fileRegistry: ['type', FileRegistry],
 };
 
 export { RepeatRenderManager };
diff --git a/packages/form-js-viewer/src/render/FileRegistry.js b/packages/form-js-viewer/src/render/FileRegistry.js
@@ -1,9 +1,26 @@
+import { getDataFileReferences } from '../util/files';
+import { FILE_PICKER_FILE_KEY_PREFIX } from '../util/constants/FilePickerConstants';
+
 const fileRegistry = Symbol('fileRegistry');
+const EMPTY_ARRAY = [];
 
 class FileRegistry {
-  constructor() {
+  constructor(eventBus, formFieldRegistry, formFieldInstanceRegistry) {
     /** @type {Map<string, File[]>} */
     this[fileRegistry] = new Map();
+    this._eventBus = eventBus;
+    this._formFieldRegistry = formFieldRegistry;
+    this._formFieldInstanceRegistry = formFieldInstanceRegistry;
+
+    eventBus.on('form.clear', () => this.clear());
+    eventBus.on('repeatRenderManager.remove', ({ item }) => {
+      const fileReferences = getDataFileReferences(item);
+
+      // Remove all file references from the registry
+      fileReferences.forEach((fileReference) => {
+        this.deleteFiles(fileReference);
+      });
+    });
   }
 
   /**
@@ -19,7 +36,7 @@ class FileRegistry {
    * @returns {File[]}
    */
   getFiles(id) {
-    return this[fileRegistry].get(id) || [];
+    return this[fileRegistry].get(id) || EMPTY_ARRAY;
   }
 
   /**
@@ -43,9 +60,32 @@ class FileRegistry {
     return new Map(this[fileRegistry]);
   }
 
-  reset() {
+  /**
+   * @returns {Map<string, File[]>}
+   */
+  getSubmitFiles() {
+    const instancedFileReferences = this._formFieldInstanceRegistry
+      .getAll()
+      .filter(
+        (instance) =>
+          this._formFieldRegistry.get(instance.id).type === 'filepicker' &&
+          typeof instance.value === 'string' &&
+          instance.value.startsWith(FILE_PICKER_FILE_KEY_PREFIX),
+      )
+      .map((formFieldInstance) => formFieldInstance.value);
+
+    return new Map(
+      Array.from(this[fileRegistry]).filter(([key]) => {
+        return instancedFileReferences.includes(key);
+      }),
+    );
+  }
+
+  clear() {
     this[fileRegistry].clear();
   }
 }
 
+FileRegistry.$inject = ['eventBus', 'formFieldRegistry', 'formFieldInstanceRegistry'];
+
 export { FileRegistry };
diff --git a/packages/form-js-viewer/src/render/components/FormField.js b/packages/form-js-viewer/src/render/components/FormField.js
@@ -67,10 +67,11 @@ export function FormField(props) {
       id: field.id,
       expressionContextInfo: localExpressionContext,
       valuePath,
+      value,
       indexes,
       hidden,
     });
-  }, [formFieldInstanceRegistry, field.id, localExpressionContext, valuePath, indexes, hidden]);
+  }, [formFieldInstanceRegistry, field.id, localExpressionContext, valuePath, value, indexes, hidden]);
 
   const fieldInstance = instanceId ? formFieldInstanceRegistry.get(instanceId) : null;