Fix race condition with plugin bundles

There may be a race condition when loading plugin bundles with lazy dependencies:

The OpenAPI plugin depends on "redux" and "react-redux", which are bundled in the lazy "ui-legacy" module, as the usage of redux is deprecated in the scmm. The "ui-legacy" module also binds a global wrapper extension point around the whole app. Due to a bug in the plugin loader, plugin bundles were marked as successfully loaded even if a lazy dependency hadn't successfully loaded yet. This caused the extension point from the "ui-legacy" bundle to be bound after the initial render. As the process of extension point binding  doesn't trigger a re-render, the redux provider was not wrapped around the app on initial load. When the user now moved focus out of and back into the window, react-query hooks automatically refetched e.g. the index links, which caused a re-render. Now with the bound extension point applied. This caused the whole app to be unmounted and re-mounted, which in turn reset all form fields anywhere below in the tree.

Also fixes a bug where the global notifications component was executing a state update while already unmounted.

Also fixes a bug in the user creation form where an object literal was passed to the form's default values which caused a form reset whenever the component re-rendered.

Committed-by: Rene Pfeuffer <rene.pfeuffer@cloudogu.com>

gradle/changelog/implicit_form_reset.yaml

@@ -0,0 +1,6 @@
+- type: fixed
+  description: Forms randomly resetting when OpenAPI plugin is installed
+- type: fixed
+  description: React error in global notifications
+- type: fixed
+  description: User creation form resetting on re-render

scm-ui/ui-api/src/notifications.ts

@@ -28,6 +28,7 @@ import { Link, Notification, NotificationCollection } from "@scm-manager/ui-type
 import { apiClient } from "./apiclient";
 import { useCallback, useEffect, useState } from "react";
 import { requiredLink } from "./links";
+import { useCancellablePromise } from "./utils";
 
 export const useNotifications = () => {
   const { data: me } = useMe();
@@ -95,11 +96,13 @@ export const useNotificationSubscription = (
   const [notifications, setNotifications] = useState<Notification[]>([]);
   const [disconnectedAt, setDisconnectedAt] = useState<Date>();
   const link = (notificationCollection?._links.subscribe as Link)?.href;
+  const cancelOnUnmount = useCancellablePromise();
 
   const onVisible = useCallback(() => {
     // we don't need to catch the error,
     // because if the refetch throws an error the parent useNotifications should catch it
-    refetch().then((collection) => {
+    cancelOnUnmount(refetch()).then(
+      (collection) => {
         if (collection) {
           const newNotifications = collection._embedded?.notifications.filter((n) => {
             return disconnectedAt && disconnectedAt < new Date(n.createdAt);
@@ -109,8 +112,14 @@ export const useNotificationSubscription = (
           }
           setDisconnectedAt(undefined);
         }
-    });
-  }, [disconnectedAt, refetch]);
+      },
+      (reason) => {
+        if (!reason.isCanceled) {
+          throw reason;
+        }
+      }
+    );
+  }, [cancelOnUnmount, disconnectedAt, refetch]);
 
   const onHide = useCallback(() => {
     setDisconnectedAt(new Date());

scm-ui/ui-api/src/utils.ts

@@ -22,8 +22,44 @@
  * SOFTWARE.
  */
 
+import { useCallback, useEffect, useRef } from "react";
+
 export const createQueryString = (params: Record<string, string>) => {
   return Object.keys(params)
     .map((k) => encodeURIComponent(k) + "=" + encodeURIComponent(params[k]))
     .join("&");
 };
+
+export type CancelablePromise<T> = Promise<T> & { cancel: () => void };
+
+export function makeCancelable<T>(promise: Promise<T>): CancelablePromise<T> {
+  let isCanceled = false;
+  const wrappedPromise = new Promise<T>((resolve, reject) => {
+    promise
+      .then((val) => (isCanceled ? reject({ isCanceled }) : resolve(val)))
+      .catch((error) => (isCanceled ? reject({ isCanceled }) : reject(error)));
+  });
+  return Object.assign(wrappedPromise, {
+    cancel() {
+      isCanceled = true;
+    },
+  });
+}
+
+export function useCancellablePromise() {
+  const promises = useRef<Array<CancelablePromise<unknown>>>();
+
+  useEffect(() => {
+    promises.current = promises.current || [];
+    return function cancel() {
+      promises.current?.forEach((p) => p.cancel());
+      promises.current = [];
+    };
+  }, []);
+
+  return useCallback(<T>(p: Promise<T>) => {
+    const cPromise = makeCancelable(p);
+    promises.current?.push(cPromise);
+    return cPromise;
+  }, []);
+}

scm-ui/ui-modules/src/index.ts

@@ -36,6 +36,9 @@ class ModuleResolutionError extends Error {
 const modules: { [name: string]: unknown } = {};
 const lazyModules: { [name: string]: () => Promise<unknown> } = {};
 const queue: { [name: string]: Module } = {};
+const bundleLoaderPromises: {
+  [name: string]: { resolve: (module: unknown) => void; reject: (reason?: unknown) => void };
+} = {};
 
 export const defineLazy = (name: string, cmp: () => Promise<unknown>) => {
   lazyModules[name] = cmp;
@@ -45,56 +48,108 @@ export const defineStatic = (name: string, cmp: unknown) => {
   modules[name] = cmp;
 };
 
-const resolveModule = (name: string) => {
+/**
+ * Attempt to retrieve a module from the registry.
+ *
+ * If a lazy module is requested, it will be loaded and then returned after adding it to the registry.
+ *
+ * @see defineLazy
+ * @see defineStatic
+ * @see defineModule
+ * @throws {ModuleResolutionError} If the requested module cannot be found/loaded
+ */
+const resolveModule = async (name: string) => {
   const module = modules[name];
   if (module) {
-    return Promise.resolve(module);
+    return module;
   }
 
-  const lazyModule = lazyModules[name];
-  if (lazyModule) {
-    return lazyModule().then((mod: unknown) => {
-      modules[name] = mod;
-      return mod;
-    });
+  const lazyModuleLoader = lazyModules[name];
+  if (lazyModuleLoader) {
+    const lazyModule = await lazyModuleLoader();
+    modules[name] = lazyModule;
+    return lazyModule;
   }
 
-  return Promise.reject(new ModuleResolutionError(name));
+  throw new ModuleResolutionError(name);
 };
 
-const defineModule = (name: string, module: Module) => {
-  Promise.all(module.dependencies.map(resolveModule))
-    .then((resolvedDependencies) => {
-      modules["@scm-manager/" + name] = module.fn(...resolvedDependencies);
+/**
+ * Executes a module and attempts to resolve all of its dependencies.
+ *
+ * If a dependency is not (yet) present, the module loading is deferred.
+ *
+ * Once a module on which the given module depends loaded successfully, it will
+ * kickstart another attempt at loading the given module.
+ */
+const defineModule = async (name: string, module: Module) => {
+  try {
+    const resolvedDependencies = await Promise.all(module.dependencies.map(resolveModule));
+    const loadedModuleName = "@scm-manager/" + name;
 
-      Object.keys(queue).forEach((queuedModuleName) => {
-        const queueModule = queue[queuedModuleName];
+    // Store module to be used as dependency for other modules
+    modules[loadedModuleName] = module.fn(...resolvedDependencies);
+
+    // Resolve bundle in which module was defined
+    if (name in bundleLoaderPromises) {
+      bundleLoaderPromises[name].resolve(modules[loadedModuleName]);
+      delete bundleLoaderPromises[name];
+    }
+
+    // Executed queued modules that depend on the loaded module
+    for (const [queuedModuleName, queuedModule] of Object.entries(queue)) {
+      if (queuedModule.dependencies.includes(loadedModuleName)) {
         delete queue[queuedModuleName];
-        defineModule(queuedModuleName, queueModule);
-      });
-    })
-    .catch((e) => {
-      if (e instanceof ModuleResolutionError) {
-        queue[name] = module;
-      } else {
-        throw e;
+        defineModule(queuedModuleName, queuedModule);
+      }
+    }
+  } catch (reason) {
+    if (reason instanceof ModuleResolutionError) {
+      // Wait for module dependency to load
+      queue[name] = module;
+    } else if (name in bundleLoaderPromises) {
+      // Forward error to bundle loader in which module was defined
+      bundleLoaderPromises[name].reject(reason);
+      delete bundleLoaderPromises[name];
+    } else {
+      // Throw unhandled exception
+      throw reason;
+    }
   }
-    });
 };
 
+/**
+ * This is attached to the global window scope and is automatically executed when a plugin module bundle is loaded.
+ *
+ * @see https://github.com/amdjs/amdjs-api/blob/master/AMD.md
+ */
 export const define = (name: string, dependencies: string[], fn: (...args: unknown[]) => Module) => {
   defineModule(name, { dependencies, fn });
 };
 
-export const load = (resource: string) => {
-  return new Promise((resolve, reject) => {
+/**
+ * Asynchronously loads and executes a given resource bundle.
+ *
+ * If a module name is supplied, the bundle is expected to contain a single (AMD)[https://github.com/amdjs/amdjs-api/blob/master/AMD.md]
+ * module matching the provided module name.
+ *
+ * The promise will only resolve once the bundle loaded and, if it is a module,
+ * all dependencies are resolved and the module executed.
+ */
+export const load = (resource: string, moduleName?: string) =>
+  new Promise((resolve, reject) => {
     const script = document.createElement("script");
     script.src = resource;
+
+    if (moduleName) {
+      bundleLoaderPromises[moduleName] = { resolve, reject };
+    } else {
       script.onload = resolve;
+    }
+
     script.onerror = reject;
 
     const body = document.querySelector("body");
     body?.appendChild(script);
     body?.removeChild(script);
   });
-};

scm-ui/ui-webapp/src/containers/PluginLoader.tsx

@@ -28,6 +28,8 @@ import { apiClient, ErrorBoundary, ErrorNotification, Icon, Loading } from "@scm
 import loadBundle from "./loadBundle";
 import { ExtensionPoint } from "@scm-manager/ui-extensions";
 
+const isMainModuleBundle = (bundlePath: string, pluginName: string) => bundlePath.endsWith(`${pluginName}.bundle.js`);
+
 type Props = {
   loaded: boolean;
   children: ReactNode;
@@ -54,7 +56,7 @@ class PluginLoader extends React.Component<Props, State> {
   constructor(props: Props) {
     super(props);
     this.state = {
-      message: "booting"
+      message: "booting",
     };
   }
 
@@ -62,7 +64,7 @@ class PluginLoader extends React.Component<Props, State> {
     const { loaded } = this.props;
     if (!loaded) {
       this.setState({
-        message: "loading plugin information"
+        message: "loading plugin information",
       });
 
       this.getPlugins(this.props.link);
@@ -72,16 +74,16 @@ class PluginLoader extends React.Component<Props, State> {
   getPlugins = (link: string) => {
     apiClient
       .get(link)
-      .then(response => response.text())
+      .then((response) => response.text())
       .then(JSON.parse)
-      .then(pluginCollection => pluginCollection._embedded.plugins)
+      .then((pluginCollection) => pluginCollection._embedded.plugins)
       .then(this.loadPlugins)
       .then(this.props.callback);
   };
 
   loadPlugins = (plugins: Plugin[]) => {
     this.setState({
-      message: "loading plugins"
+      message: "loading plugins",
     });
 
     const promises = [];
@@ -94,16 +96,19 @@ class PluginLoader extends React.Component<Props, State> {
 
   loadPlugin = (plugin: Plugin) => {
     const promises = [];
-    for (const bundle of plugin.bundles) {
+    for (const bundlePath of plugin.bundles) {
       promises.push(
-        loadBundle(bundle).catch(error => this.setState({ error, errorMessage: `loading ${plugin.name} failed` }))
+        (isMainModuleBundle(bundlePath, plugin.name)
+          ? loadBundle(bundlePath, plugin.name)
+          : loadBundle(bundlePath)
+        ).catch((error) => this.setState({ error, errorMessage: `loading ${plugin.name} failed` }))
       );
     }
     return Promise.all(promises);
   };
 
   render() {
-    const { loaded } = this.props;
+    const { loaded, children } = this.props;
     const { message, error, errorMessage } = this.state;
 
     if (error) {
@@ -130,11 +135,7 @@ class PluginLoader extends React.Component<Props, State> {
     }
 
     if (loaded) {
-      return (
-        <ExtensionPoint name="main.wrapper" wrapper={true}>
-          {this.props.children}
-        </ExtensionPoint>
-      );
+      return <ExtensionPoint name="main.wrapper">{children}</ExtensionPoint>;
     }
     return <Loading message={message} />;
   }

scm-ui/ui-webapp/src/users/containers/CreateUser.tsx

@@ -21,7 +21,7 @@
  * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE
  * SOFTWARE.
  */
-import React, { FC } from "react";
+import React, { FC, useRef } from "react";
 import { Redirect } from "react-router-dom";
 import { useTranslation } from "react-i18next";
 import { useRequiredIndexLink } from "@scm-manager/ui-api";
@@ -45,17 +45,7 @@ const CreateUser: FC = () => {
       contentType: "application/vnd.scmm-user+json;v=2",
     }
   );
-
-  if (!!createdUser) {
-    return <Redirect to={`/user/${createdUser.name}`} />;
-  }
-
-  return (
-    <Page title={t("createUser.title")} subtitle={t("createUser.subtitle")} showContentOnError={true}>
-      <Form
-        onSubmit={submit}
-        translationPath={["users", "createUser.form"]}
-        defaultValues={{
+  const defaultValuesRef = useRef({
     name: "",
     password: "",
     passwordConfirmation: "",
@@ -63,8 +53,15 @@ const CreateUser: FC = () => {
     external: false,
     displayName: "",
     mail: "",
-        }}
-      >
+  });
+
+  if (!!createdUser) {
+    return <Redirect to={`/user/${createdUser.name}`} />;
+  }
+
+  return (
+    <Page title={t("createUser.title")} subtitle={t("createUser.subtitle")} showContentOnError={true}>
+      <Form onSubmit={submit} translationPath={["users", "createUser.form"]} defaultValues={defaultValuesRef.current}>
         {({ watch }) => (
           <>
             <Form.Row>