From 987893aa676f43f7b286dbd580056e90eb64adb5 Mon Sep 17 00:00:00 2001
From: Florian Scholdei <florian.scholdei@cloudogu.com>
Date: Tue, 4 Feb 2025 15:06:09 +0100
Subject: [PATCH] Fix loop in secondary navigation render cycle

---
 .../secondary_navigation_context.yaml         |  2 +
 scm-ui/ui-api/src/localStorage.ts             | 31 +++++++++++-----
 .../src/__snapshots__/storyshots.test.ts.snap | 15 ++++++--
 .../src/layout/SecondaryNavigationColumn.tsx  | 11 +++++-
 .../src/navigation/ExternalNavLink.tsx        |  6 +--
 .../ui-components/src/navigation/NavLink.tsx  | 16 +++-----
 .../SecondaryNavigation.stories.tsx           | 11 ++++--
 .../src/navigation/SecondaryNavigation.tsx    | 15 ++------
 .../navigation/SecondaryNavigationContext.ts  | 19 ----------
 .../navigation/SecondaryNavigationContext.tsx | 37 +++++++++++++++++++
 .../src/useSecondaryNavigation.tsx            | 28 ++++++--------
 .../base/forms/chip-input/ChipInputField.tsx  |  1 -
 scm-ui/ui-extensions/src/extensionPoints.tsx  |  2 +-
 .../roles/containers/SingleRepositoryRole.tsx |  2 +-
 14 files changed, 112 insertions(+), 84 deletions(-)
 create mode 100644 gradle/changelog/secondary_navigation_context.yaml
 delete mode 100644 scm-ui/ui-components/src/navigation/SecondaryNavigationContext.ts
 create mode 100644 scm-ui/ui-components/src/navigation/SecondaryNavigationContext.tsx

diff --git a/gradle/changelog/secondary_navigation_context.yaml b/gradle/changelog/secondary_navigation_context.yaml
new file mode 100644
index 0000000000..8816fb76c7
--- /dev/null
+++ b/gradle/changelog/secondary_navigation_context.yaml
@@ -0,0 +1,2 @@
+- type: fixed
+  description: Loop in secondary navigation render cycle
diff --git a/scm-ui/ui-api/src/localStorage.ts b/scm-ui/ui-api/src/localStorage.ts
index 42c48d7c5d..24b1a57066 100644
--- a/scm-ui/ui-api/src/localStorage.ts
+++ b/scm-ui/ui-api/src/localStorage.ts
@@ -37,7 +37,7 @@ const determineInitialValue = <T>(key: string, initialValue: T) => {
  */
 export function useLocalStorage<T>(key: string, initialValue: T): [value: T, setValue: LocalStorageSetter<T>] {
   const initialValueOrValueFromStorage = useMemo(() => determineInitialValue(key, initialValue), [key, initialValue]);
-  const [item, setItem] = useState(initialValueOrValueFromStorage);
+  const [item, setItem] = useState<T>(initialValueOrValueFromStorage);
 
   useEffect(() => {
     const listener = (event: StorageEvent) => {
@@ -50,14 +50,27 @@ export function useLocalStorage<T>(key: string, initialValue: T): [value: T, set
   }, [key, initialValue]);
 
   const setValue: LocalStorageSetter<T> = (newValue) => {
-    const computedNewValue = newValue instanceof Function ? newValue(item) : newValue;
-    setItem(computedNewValue);
-    const json = JSON.stringify(computedNewValue);
-    localStorage.setItem(key, json);
-    // storage event is no triggered in same tab
-    window.dispatchEvent(
-      new StorageEvent("storage", { key, oldValue: item, newValue: json, storageArea: localStorage })
-    );
+    // We've got to use setItem here to get the actual current value for item, not the one we got when this function was
+    // created, in other words: We want to get rid of the dependency to item to get a similar behaviour as the setter
+    // from useState.
+    // (We also could wrap this function in a useCallback, but then we'd had to put this function in a dependency array
+    // when we use this function so that we always refer to the current function. This is not the case for useState.)
+    setItem((oldValue) => {
+      const computedNewValue = newValue instanceof Function ? newValue(oldValue) : newValue;
+      setItem(computedNewValue);
+      const json = JSON.stringify(computedNewValue);
+      localStorage.setItem(key, json);
+      // storage event is not triggered in same tab
+      window.dispatchEvent(
+        new StorageEvent("storage", {
+          key,
+          oldValue: JSON.stringify(oldValue),
+          newValue: json,
+          storageArea: localStorage,
+        })
+      );
+      return computedNewValue;
+    });
   };
 
   return [item, setValue];
diff --git a/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap b/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap
index aed5f205e5..a74f7cb029 100644
--- a/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap
+++ b/scm-ui/ui-components/src/__snapshots__/storyshots.test.ts.snap
@@ -78158,11 +78158,18 @@ exports[`Storyshots Secondary Navigation Active when match 1`] = `
       <div>
         <button
           aria-label="secondaryNavigation.hideContent"
-          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label is-clickable"
+          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label"
           collapsed={false}
           onClick={[Function]}
           type="button"
         >
+          <i
+            className="SecondaryNavigation__Icon-sc-8p1rgi-1 gqxbcY is-medium"
+          >
+            <i
+              className="fas fa-caret-down"
+            />
+          </i>
           Hitchhiker
         </button>
         <ul
@@ -78215,7 +78222,7 @@ exports[`Storyshots Secondary Navigation Default 1`] = `
       <div>
         <button
           aria-label="secondaryNavigation.hideContent"
-          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label is-clickable"
+          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label"
           collapsed={false}
           onClick={[Function]}
           type="button"
@@ -78278,7 +78285,7 @@ exports[`Storyshots Secondary Navigation Extension Point 1`] = `
       <div>
         <button
           aria-label="secondaryNavigation.hideContent"
-          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label is-clickable"
+          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label"
           collapsed={false}
           onClick={[Function]}
           type="button"
@@ -78369,7 +78376,7 @@ exports[`Storyshots Secondary Navigation Sub Navigation 1`] = `
       <div>
         <button
           aria-label="secondaryNavigation.hideContent"
-          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label is-clickable"
+          className="button SecondaryNavigation__MenuButton-sc-8p1rgi-2 fkeiWf menu-label"
           collapsed={false}
           onClick={[Function]}
           type="button"
diff --git a/scm-ui/ui-components/src/layout/SecondaryNavigationColumn.tsx b/scm-ui/ui-components/src/layout/SecondaryNavigationColumn.tsx
index ea722b0cd2..74643ea892 100644
--- a/scm-ui/ui-components/src/layout/SecondaryNavigationColumn.tsx
+++ b/scm-ui/ui-components/src/layout/SecondaryNavigationColumn.tsx
@@ -17,6 +17,7 @@
 import React, { FC } from "react";
 import styled from "styled-components";
 import { useSecondaryNavigation } from "../useSecondaryNavigation";
+import { SecondaryNavigationProvider } from "../navigation/SecondaryNavigationContext";
 
 const SecondaryColumn = styled.div<{ collapsed: boolean }>`
   flex: 0 0 auto;
@@ -28,7 +29,7 @@ const SecondaryColumn = styled.div<{ collapsed: boolean }>`
   }
 `;
 
-const SecondaryNavigationColumn: FC = ({ children }) => {
+const SecondaryNavigationColumnIntern: FC = ({ children }) => {
   const { collapsed } = useSecondaryNavigation();
 
   return (
@@ -38,4 +39,12 @@ const SecondaryNavigationColumn: FC = ({ children }) => {
   );
 };
 
+const SecondaryNavigationColumn: FC = ({ children }) => {
+  return (
+    <SecondaryNavigationProvider>
+      <SecondaryNavigationColumnIntern>{children}</SecondaryNavigationColumnIntern>
+    </SecondaryNavigationProvider>
+  );
+};
+
 export default SecondaryNavigationColumn;
diff --git a/scm-ui/ui-components/src/navigation/ExternalNavLink.tsx b/scm-ui/ui-components/src/navigation/ExternalNavLink.tsx
index dd77c5012b..118d5178af 100644
--- a/scm-ui/ui-components/src/navigation/ExternalNavLink.tsx
+++ b/scm-ui/ui-components/src/navigation/ExternalNavLink.tsx
@@ -14,11 +14,10 @@
  * along with this program. If not, see https://www.gnu.org/licenses/.
  */
 
-import React, { FC, useContext } from "react";
+import React, { FC } from "react";
 import classNames from "classnames";
 import { useSecondaryNavigation } from "../useSecondaryNavigation";
 import ExternalLink from "./ExternalLink";
-import { SecondaryNavigationContext } from "./SecondaryNavigationContext";
 
 type Props = {
   to: string;
@@ -28,7 +27,6 @@ type Props = {
 
 const ExternalNavLink: FC<Props> = ({ to, icon, label }) => {
   const { collapsed } = useSecondaryNavigation();
-  const isSecondaryNavigation = useContext(SecondaryNavigationContext);
 
   let showIcon;
   if (icon) {
@@ -43,7 +41,7 @@ const ExternalNavLink: FC<Props> = ({ to, icon, label }) => {
     <li title={collapsed ? label : undefined}>
       <ExternalLink to={to} className={collapsed ? "has-text-centered" : ""} aria-label={collapsed ? label : undefined}>
         {showIcon}
-        {isSecondaryNavigation && collapsed ? null : label}
+        {collapsed ? null : label}
       </ExternalLink>
     </li>
   );
diff --git a/scm-ui/ui-components/src/navigation/NavLink.tsx b/scm-ui/ui-components/src/navigation/NavLink.tsx
index e2ece41298..1d47096548 100644
--- a/scm-ui/ui-components/src/navigation/NavLink.tsx
+++ b/scm-ui/ui-components/src/navigation/NavLink.tsx
@@ -17,12 +17,11 @@
 import React, { FC, useContext, useEffect } from "react";
 import classNames from "classnames";
 import { Link } from "react-router-dom";
+import { createAttributesForTesting } from "@scm-manager/ui-core";
 import { useSecondaryNavigation } from "../useSecondaryNavigation";
 import { RoutingProps } from "./RoutingProps";
-import useActiveMatch from "./useActiveMatch";
-import { createAttributesForTesting } from "@scm-manager/ui-core";
-import { SecondaryNavigationContext } from "./SecondaryNavigationContext";
 import { SubNavigationContext } from "./SubNavigationContext";
+import useActiveMatch from "./useActiveMatch";
 
 type Props = RoutingProps & {
   label: string;
@@ -51,14 +50,13 @@ const NavLinkContent: FC<NavLinkContentProp> = ({ label, icon, collapsed }) => (
 const NavLink: FC<Props> = ({ to, activeWhenMatch, activeOnlyWhenExact, title, testId, children, ...contentProps }) => {
   const active = useActiveMatch({ to, activeWhenMatch, activeOnlyWhenExact });
   const { collapsed, setCollapsible } = useSecondaryNavigation();
-  const isSecondaryNavigation = useContext(SecondaryNavigationContext);
   const isSubNavigation = useContext(SubNavigationContext);
 
   useEffect(() => {
-    if (isSecondaryNavigation && active) {
+    if (active) {
       setCollapsible(!isSubNavigation);
     }
-  }, [active, isSecondaryNavigation, isSubNavigation, setCollapsible]);
+  }, [active, isSubNavigation, setCollapsible]);
 
   return (
     <li title={collapsed ? title : undefined}>
@@ -69,11 +67,7 @@ const NavLink: FC<Props> = ({ to, activeWhenMatch, activeOnlyWhenExact, title, t
         aria-label={collapsed ? title : undefined}
         {...(active ? { "aria-current": "page" } : {})}
       >
-        {children ? (
-          children
-        ) : (
-          <NavLinkContent {...contentProps} collapsed={(isSecondaryNavigation && collapsed) ?? false} />
-        )}
+        {children || <NavLinkContent {...contentProps} collapsed={collapsed} />}
       </Link>
     </li>
   );
diff --git a/scm-ui/ui-components/src/navigation/SecondaryNavigation.stories.tsx b/scm-ui/ui-components/src/navigation/SecondaryNavigation.stories.tsx
index 14db4ecc55..5aea7ddc9e 100644
--- a/scm-ui/ui-components/src/navigation/SecondaryNavigation.stories.tsx
+++ b/scm-ui/ui-components/src/navigation/SecondaryNavigation.stories.tsx
@@ -16,12 +16,13 @@
 
 import { storiesOf } from "@storybook/react";
 import React, { ReactElement } from "react";
+import { MemoryRouter } from "react-router-dom";
+import styled from "styled-components";
+import { Binder, ExtensionPoint, BinderContext } from "@scm-manager/ui-extensions";
+import { SecondaryNavigationProvider } from "./SecondaryNavigationContext";
 import SecondaryNavigation from "./SecondaryNavigation";
 import SecondaryNavigationItem from "./SecondaryNavigationItem";
-import styled from "styled-components";
 import SubNavigation from "./SubNavigation";
-import { Binder, ExtensionPoint, BinderContext } from "@scm-manager/ui-extensions";
-import { MemoryRouter } from "react-router-dom";
 
 const Columns = styled.div`
   margin: 2rem;
@@ -46,7 +47,9 @@ const withRoute = (route: string) => {
 storiesOf("Secondary Navigation", module)
   .addDecorator((story) => (
     <Columns className="columns">
-      <div className="column is-3">{story()}</div>
+      <div className="column is-3">
+        <SecondaryNavigationProvider>{story()}</SecondaryNavigationProvider>
+      </div>
     </Columns>
   ))
   .add("Default", () =>
diff --git a/scm-ui/ui-components/src/navigation/SecondaryNavigation.tsx b/scm-ui/ui-components/src/navigation/SecondaryNavigation.tsx
index 62b2b44c66..08fd6fc383 100644
--- a/scm-ui/ui-components/src/navigation/SecondaryNavigation.tsx
+++ b/scm-ui/ui-components/src/navigation/SecondaryNavigation.tsx
@@ -16,10 +16,8 @@
 
 import React, { FC } from "react";
 import styled from "styled-components";
-import classNames from "classnames";
 import { useTranslation } from "react-i18next";
 import { useSecondaryNavigation } from "../useSecondaryNavigation";
-import { SecondaryNavigationContext } from "./SecondaryNavigationContext";
 import { Button } from "@scm-manager/ui-buttons";
 
 type Props = {
@@ -73,14 +71,9 @@ const SecondaryNavigation: FC<Props> = ({ label, children, collapsible = true })
   const menuAriaLabel = collapsed ? t("secondaryNavigation.showContent") : t("secondaryNavigation.hideContent");
 
   return (
-    <SectionContainer className="menu" collapsed={collapsed ?? false}>
+    <SectionContainer className="menu" collapsed={collapsed}>
       <div>
-        <MenuButton
-          className={classNames("menu-label", { "is-clickable": true })}
-          collapsed={collapsed}
-          onClick={toggleCollapse}
-          aria-label={menuAriaLabel}
-        >
+        <MenuButton className="menu-label" collapsed={collapsed} onClick={toggleCollapse} aria-label={menuAriaLabel}>
           {isCollapsible ? (
             <Icon className="is-medium" collapsed={collapsed}>
               {arrowIcon}
@@ -88,9 +81,7 @@ const SecondaryNavigation: FC<Props> = ({ label, children, collapsible = true })
           ) : null}
           {collapsed ? "" : label}
         </MenuButton>
-        <ul className="menu-list">
-          <SecondaryNavigationContext.Provider value={true}>{children}</SecondaryNavigationContext.Provider>
-        </ul>
+        <ul className="menu-list">{children}</ul>
       </div>
     </SectionContainer>
   );
diff --git a/scm-ui/ui-components/src/navigation/SecondaryNavigationContext.ts b/scm-ui/ui-components/src/navigation/SecondaryNavigationContext.ts
deleted file mode 100644
index 30a53807e2..0000000000
--- a/scm-ui/ui-components/src/navigation/SecondaryNavigationContext.ts
+++ /dev/null
@@ -1,19 +0,0 @@
-/*
- * Copyright (c) 2020 - present Cloudogu GmbH
- *
- * This program is free software: you can redistribute it and/or modify it under
- * the terms of the GNU Affero General Public License as published by the Free
- * Software Foundation, version 3.
- *
- * This program is distributed in the hope that it will be useful, but WITHOUT
- * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
- * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
- * details.
- *
- * You should have received a copy of the GNU Affero General Public License
- * along with this program. If not, see https://www.gnu.org/licenses/.
- */
-
-import React from "react";
-
-export const SecondaryNavigationContext = React.createContext(false);
diff --git a/scm-ui/ui-components/src/navigation/SecondaryNavigationContext.tsx b/scm-ui/ui-components/src/navigation/SecondaryNavigationContext.tsx
new file mode 100644
index 0000000000..dda0e191d1
--- /dev/null
+++ b/scm-ui/ui-components/src/navigation/SecondaryNavigationContext.tsx
@@ -0,0 +1,37 @@
+/*
+ * Copyright (c) 2020 - present Cloudogu GmbH
+ *
+ * This program is free software: you can redistribute it and/or modify it under
+ * the terms of the GNU Affero General Public License as published by the Free
+ * Software Foundation, version 3.
+ *
+ * This program is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS
+ * FOR A PARTICULAR PURPOSE. See the GNU Affero General Public License for more
+ * details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program. If not, see https://www.gnu.org/licenses/.
+ */
+
+import React, { ReactNode, useMemo, useState } from "react";
+
+type SecondaryNavigationContextState = {
+  collapsible: boolean;
+  setCollapsible: (collapsed: boolean) => void;
+};
+
+const dummy: SecondaryNavigationContextState = {
+  collapsible: false,
+  // eslint-disable-next-line @typescript-eslint/no-empty-function
+  setCollapsible: () => {},
+};
+
+export const SecondaryNavigationContext = React.createContext<SecondaryNavigationContextState>(dummy);
+
+export const SecondaryNavigationProvider = ({ children }: { children: ReactNode }) => {
+  const [collapsible, setCollapsible] = useState(true);
+  const contextValue = useMemo(() => ({ collapsible, setCollapsible }), [collapsible, setCollapsible]);
+
+  return <SecondaryNavigationContext.Provider value={contextValue}>{children}</SecondaryNavigationContext.Provider>;
+};
diff --git a/scm-ui/ui-components/src/useSecondaryNavigation.tsx b/scm-ui/ui-components/src/useSecondaryNavigation.tsx
index 632bc5aac2..9375e73836 100644
--- a/scm-ui/ui-components/src/useSecondaryNavigation.tsx
+++ b/scm-ui/ui-components/src/useSecondaryNavigation.tsx
@@ -15,17 +15,14 @@
  */
 
 import { useLocalStorage } from "@scm-manager/ui-api";
-import { useCallback, useMemo } from "react";
+import { useCallback, useContext } from "react";
+import { SecondaryNavigationContext } from "./navigation/SecondaryNavigationContext";
 
 export const useSecondaryNavigation = (isNavigationCollapsible = true) => {
-  const [isCollapsed, setCollapsed] = useLocalStorage<boolean>("secondaryNavigation.collapsed", false);
-  const [isRouteCollapsible, setRouteCollapsible] = useLocalStorage<boolean>("secondaryNavigation.collapsible", true);
+  const { collapsible, setCollapsible } = useContext(SecondaryNavigationContext);
+  const [isCollapsed, setCollapsed] = useLocalStorage("secondaryNavigation.collapsed", false);
 
-  const collapsible = useMemo(
-    () => isRouteCollapsible && isNavigationCollapsible,
-    [isNavigationCollapsible, isRouteCollapsible]
-  );
-  const collapsed = useMemo(() => collapsible && isCollapsed, [collapsible, isCollapsed]);
+  const collapsed = collapsible && isCollapsed;
 
   const toggleCollapse = useCallback(() => {
     if (collapsible) {
@@ -33,13 +30,10 @@ export const useSecondaryNavigation = (isNavigationCollapsible = true) => {
     }
   }, [collapsible, setCollapsed]);
 
-  return useMemo(
-    () => ({
-      collapsed,
-      collapsible,
-      setCollapsible: setRouteCollapsible,
-      toggleCollapse,
-    }),
-    [collapsed, collapsible, setRouteCollapsible, toggleCollapse]
-  );
+  return {
+    collapsed,
+    collapsible,
+    setCollapsible,
+    toggleCollapse,
+  };
 };
diff --git a/scm-ui/ui-core/src/base/forms/chip-input/ChipInputField.tsx b/scm-ui/ui-core/src/base/forms/chip-input/ChipInputField.tsx
index 7c5485bb5b..bb14ab0b49 100644
--- a/scm-ui/ui-core/src/base/forms/chip-input/ChipInputField.tsx
+++ b/scm-ui/ui-core/src/base/forms/chip-input/ChipInputField.tsx
@@ -28,7 +28,6 @@ import { VisuallyHidden } from "@radix-ui/react-visually-hidden";
 import { useTranslation } from "react-i18next";
 import { withForwardRef } from "../helpers";
 import { Option } from "@scm-manager/ui-types";
-import { waitForRestartAfter } from "@scm-manager/ui-api";
 
 const StyledChipInput: typeof ChipInput = styled(ChipInput)`
   min-height: 40px;
diff --git a/scm-ui/ui-extensions/src/extensionPoints.tsx b/scm-ui/ui-extensions/src/extensionPoints.tsx
index b6d4fe062f..f35f22e585 100644
--- a/scm-ui/ui-extensions/src/extensionPoints.tsx
+++ b/scm-ui/ui-extensions/src/extensionPoints.tsx
@@ -686,7 +686,7 @@ export type RepositoryInformationTableBottom = RenderableExtensionPointDefinitio
 
 export type RepositoryBanner = RenderableExtensionPointDefinition<
   "repository.banner",
-  { repository: Repository, url: string }
+  { repository: Repository; url: string }
 >;
 
 export type UserInformationTableBottom = RenderableExtensionPointDefinition<
diff --git a/scm-ui/ui-webapp/src/admin/roles/containers/SingleRepositoryRole.tsx b/scm-ui/ui-webapp/src/admin/roles/containers/SingleRepositoryRole.tsx
index 47898e936a..81bf561231 100644
--- a/scm-ui/ui-webapp/src/admin/roles/containers/SingleRepositoryRole.tsx
+++ b/scm-ui/ui-webapp/src/admin/roles/containers/SingleRepositoryRole.tsx
@@ -18,7 +18,7 @@ import React, { FC } from "react";
 import { Route, useParams, useRouteMatch } from "react-router-dom";
 import { useTranslation } from "react-i18next";
 import { ExtensionPoint, extensionPoints } from "@scm-manager/ui-extensions";
-import { ErrorPage, Loading, Title, urls } from "@scm-manager/ui-components";
+import { ErrorPage, Loading, urls } from "@scm-manager/ui-components";
 import PermissionRoleDetail from "../components/PermissionRoleDetails";
 import EditRepositoryRole from "./EditRepositoryRole";
 import { useRepositoryRole } from "@scm-manager/ui-api";