fix(alerts): ensure hiding works correctly and alerts are not re-adde…

…d [DHIS2-15438] (#859)

* chore: add bug reproduction test case

* fix: ensure hiding works correctly and alerts are not re-added

* chore: fix typos

* chore: import useAlerts from app-runtime

---------

Co-authored-by: Kai Vandivier <49666798+KaiVandivier@users.noreply.github.com>

adapter/src/components/Alerts.js

@@ -1,53 +1,89 @@
 import { useAlerts } from '@dhis2/app-runtime'
-import { AlertBar, AlertStack } from '@dhis2/ui'
-import React, { useState, useEffect } from 'react'
+import { AlertStack, AlertBar } from '@dhis2/ui'
+import React, { useCallback, useState } from 'react'
 
-/*
- * The alert-manager which populates the `useAlerts` hook from `@dhis2/app-service-alerts`
- * hook with alerts only supports simply adding and removing alerts. However, the
- * `AlertBar` from `@dhis2/ui` should leave the screen with a hide-animation, so this
- * requires an additional state. The `alertStackAlerts` state in the Alerts component
- * provides this addional state:
- *  - It contains all alerts from the alert-manager, with `options.hidden` set to `false`
- *  - And also alerts which have been removed from the alert-manager, but still have their
- *    leave animation in progress, whtih `options.hidden` set to `true`)
- * Alerts are removed from the `alertStackAlerts` state once the `onHidden` callback fires
- */
+/* The alerts-manager which populates the `useAlerts` hook from
+ * `@dhis2/app-service-alerts` hook with alerts only supports
+ * simply adding and removing alerts. However, the `AlertBar`
+ * from `@dhis2/ui` should leave the screen with a hide-animation.
+ * This works well, for alerts that hide "naturally" (after the
+ * timeout expires or when the close icon is clicked). In these
+ * cases the component will request to be removed from the alerts-
+ * manager after the animation completes. However, when
+ * programatically hiding an alert this is the other way around:
+ * the alert is removed from the alerts-manager straight away and
+ * if we were to render the alerts from the `useAlerts` hook, these
+ * alerts would be removed from the DOM abruptly without an animation.
+ * To prevent this from happening, we have implemented the
+ * `useAlertsWithHideCache` hook:
+ *  - It contains all alerts from the alert-manager, with
+ *    `options.hidden` set to `false`
+ *  - And also alerts which have been removed from the alert-manager,
+ *    but still have their leave animation in progress, with
+ *    `options.hidden` set to `true`
+ *  - Alerts are removed once the `onHidden` callback fires */
 
-const Alerts = () => {
+const useAlertsWithHideCache = () => {
+    const [alertsMap] = useState(new Map())
+    /* We don't use this state value, it is used to trigger
+     * a rerender to remove the hidden alert from the DOM */
+    const [, setLastRemovedId] = useState(null)
     const alertManagerAlerts = useAlerts()
-    const [alertStackAlerts, setAlertStackAlerts] = useState(alertManagerAlerts)
-    const removeAlertStackAlert = (id) =>
-        setAlertStackAlerts(
-            alertStackAlerts.filter(
-                (alertStackAlert) => alertStackAlert.id !== id
-            )
-        )
+    const updateAlertsFromManager = useCallback(
+        (newAlerts = []) => {
+            const newAlertsIdLookup = new Set()
+            newAlerts.forEach((alert) => {
+                newAlertsIdLookup.add(alert.id)
+                if (!alertsMap.has(alert.id)) {
+                    // new alerts, these are not hiding
+                    alertsMap.set(alert.id, {
+                        ...alert,
+                        options: {
+                            ...alert.options,
+                            hidden: alert.options.hidden || false,
+                        },
+                    })
+                }
+            })
+            // alerts in alertsMap but not in newAlerts are hiding
+            alertsMap.forEach((alert) => {
+                if (!newAlertsIdLookup.has(alert.id)) {
+                    alert.options.hidden = true
+                }
+            })
+        },
+        [alertsMap]
+    )
+    const removeAlert = useCallback(
+        (id) => {
+            alertsMap.delete(id)
+            setLastRemovedId(id)
+        },
+        [alertsMap]
+    )
 
-    useEffect(() => {
-        if (alertManagerAlerts.length > 0) {
-            setAlertStackAlerts((currentAlertStackAlerts) =>
-                mergeAlertStackAlerts(
-                    currentAlertStackAlerts,
-                    alertManagerAlerts
-                )
-            )
-        }
-    }, [alertManagerAlerts])
+    updateAlertsFromManager(alertManagerAlerts)
+
+    return {
+        alerts: Array.from(alertsMap.values()).sort((a, b) => a.id - b.id),
+        removeAlert,
+    }
+}
+
+const Alerts = () => {
+    const { alerts, removeAlert } = useAlertsWithHideCache()
 
     return (
         <AlertStack>
-            {alertStackAlerts.map(
+            {alerts.map(
                 ({ message, remove, id, options: { onHidden, ...props } }) => (
                     <AlertBar
                         {...props}
                         key={id}
                         onHidden={() => {
                             onHidden && onHidden()
-                            removeAlertStackAlert(id)
-                            if (alertManagerAlerts.some((a) => a.id === id)) {
-                                remove()
-                            }
+                            removeAlert(id)
+                            remove()
                         }}
                     >
                         {message}
@@ -58,34 +94,4 @@ const Alerts = () => {
     )
 }
 
-function mergeAlertStackAlerts(alertStackAlerts, alertManagerAlerts) {
-    return Object.values({
-        /*
-         * Assume that all alerts in the alertStackAlerts array are hiding.
-         * After the object merge only the alerts not in the alertManagerAlerts
-         * array will have `options.hidden === true`.
-         */
-        ...toIdBasedObjectWithHiddenOption(alertStackAlerts, true),
-        /*
-         * All alertManagerAlerts should be showing. This object merge will
-         * overwrite any alertStackAlert by the alertManagerAlert with
-         * the same `id`, thus ensuring the alert is visible.
-         */
-        ...toIdBasedObjectWithHiddenOption(alertManagerAlerts, false),
-    })
-}
-
-function toIdBasedObjectWithHiddenOption(arr, hidden) {
-    return arr.reduce((obj, item) => {
-        obj[item.id] = {
-            ...item,
-            options: {
-                ...item.options,
-                hidden,
-            },
-        }
-        return obj
-    }, {})
-}
-
-export { Alerts, mergeAlertStackAlerts }
+export { Alerts }