From 6e369a7260f77e3b0d1cb94a02cbc6c3dd1d6e7f Mon Sep 17 00:00:00 2001
From: Nils Andresen <nils@nils-andresen.de>
Date: Thu, 22 Dec 2022 16:44:21 +0100
Subject: [PATCH] (#3299) fixed nesting of Headers

The NavLinkBuilder was modifying arrays in place,
as well as returning the modifications. While this was probably not
the cause of the error, I fixed this by not returning the modifications
anymore.

I changed the algorithm from counting the depth up, to counting the
order down to 0. Thus, I could remove the depth entirely.
---
 .../src/Service/NavLinkBuilder.test.ts        | 286 +++++++++---------
 .../src/Service/NavLinkBuilder.ts             |  50 ++-
 .../src/Service/SPService.ts                  |   2 +-
 .../__snapshots__/NavLinkBuilder.test.ts.snap |   4 -
 4 files changed, 170 insertions(+), 172 deletions(-)

diff --git a/samples/react-page-navigator/src/Service/NavLinkBuilder.test.ts b/samples/react-page-navigator/src/Service/NavLinkBuilder.test.ts
index 7ce4c773b..158a9c1dc 100644
--- a/samples/react-page-navigator/src/Service/NavLinkBuilder.test.ts
+++ b/samples/react-page-navigator/src/Service/NavLinkBuilder.test.ts
@@ -20,7 +20,8 @@ describe("The NavLinkBuilder without a preceding collapsable header", () => {
         name: "xyz",
     };
 
-    const actual = navLinkBuilder.build([], lnk, h1, depth);
+    const actual = [];
+    navLinkBuilder.build(actual, lnk, h1);
 
     expect(actual).toMatchSnapshot();
   });
@@ -33,8 +34,9 @@ describe("The NavLinkBuilder without a preceding collapsable header", () => {
       name: "abc",
     };
 
-    let actual = navLinkBuilder.build([], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h1, depth);
+    const actual = [];
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h1);
 
     expect(actual).toMatchSnapshot();
   })
@@ -47,8 +49,9 @@ describe("The NavLinkBuilder without a preceding collapsable header", () => {
       name: "abc",
     };
 
-    let actual = navLinkBuilder.build([], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h2, depth);
+    const actual = [];
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h2);
 
     expect(actual).toMatchSnapshot();
   });
@@ -67,10 +70,11 @@ describe("The NavLinkBuilder without a preceding collapsable header", () => {
       name: "abc.1",
     };
 
-    let actual = navLinkBuilder.build([], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk11, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk21, h2, depth);
+    const actual = [];
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk11, h2);
+    navLinkBuilder.build(actual, lnk2, h1);
+    navLinkBuilder.build(actual, lnk21, h2);
 
     expect(actual).toMatchSnapshot();
   });
@@ -89,127 +93,15 @@ describe("The NavLinkBuilder without a preceding collapsable header", () => {
       name: "geh",
     };
 
-    let actual = navLinkBuilder.build([], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk3, h3, depth);
-    actual = navLinkBuilder.build(actual, lnk4, h4, depth);
+    const actual = [];
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h2);
+    navLinkBuilder.build(actual, lnk3, h3);
+    navLinkBuilder.build(actual, lnk4, h4);
 
     expect(actual).toMatchSnapshot();
   });
 
-  it.skip("should not nest two h2", () => {
-    const lnk1: IMockLink = {
-      name: "h1",
-    };
-    const lnk2: IMockLink = {
-      name: "h2",
-    };
-    const lnk3: IMockLink = {
-      name: "h2",
-    };
-    const lnk4: IMockLink = {
-      name: "h3",
-    };
-
-    let actual = navLinkBuilder.build([], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk3, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk4, h3, depth);
-
-    expect(actual).toMatchSnapshot();
-  });
-
-  it.skip("should not nest two h3", () => {
-    const lnk0: IMockLink = {
-      name: "h1",
-    };
-    const lnk1: IMockLink = {
-      name: "h1",
-    };
-    const lnk2: IMockLink = {
-      name: "h2",
-    };
-    const lnk3: IMockLink = {
-      name: "h2",
-    };
-    const lnk4: IMockLink = {
-      name: "h3",
-    };
-    const lnk5: IMockLink = {
-      name: "h3",
-    };
-    const lnk6: IMockLink = {
-      name: "h4",
-    };
-
-    let actual = navLinkBuilder.build([], lnk0, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk3, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk4, h3, depth);
-    actual = navLinkBuilder.build(actual, lnk5, h3, depth);
-    actual = navLinkBuilder.build(actual, lnk6, h4, depth);
-
-    expect(actual).toMatchSnapshot();
-  });
-});
-
-describe("The NavLinkBuilder with a collapsable header", () => {
-  let head: IMockLink;
-  const depth = DEPTH_COLLAPSABLE_HEADER;
-  const h1 = depth;
-  const h2 = h1+1;
-  const h3 = h2+1;
-  const h4 = h3+1;
-
-  beforeEach(() => {
-    head = {
-      name: "head",
-    };
-  });
-
-  it("should add a single item to the heading", () => {
-    const lnk: IMockLink = {
-      name: "xyz",
-    };
-
-    const actual = navLinkBuilder.build([ head ], lnk, h1, depth);
-
-    expect(actual).toMatchSnapshot();
-  });
-
-  it("should add a two items on the same level", () => {
-    const lnk1: IMockLink = {
-      name: "xyz",
-    };
-    const lnk2: IMockLink = {
-      name: "abc",
-    };
-
-    let actual = navLinkBuilder.build([ head ], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h1, depth);
-
-    expect(actual).toMatchSnapshot();
-  })
-
-  it("should add a one item in a collapsable section two inside that one", () => {
-    const lnk1: IMockLink = {
-      name: "xyz",
-    };
-    const lnk2: IMockLink = {
-      name: "abc",
-    };
-    const lnk3: IMockLink = {
-      name: "def",
-    };
-
-    let actual = navLinkBuilder.build([ head ], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk3, h2, depth);
-
-    expect(actual).toMatchSnapshot();
-  })
-
   it("should not nest two h2", () => {
     const lnk1: IMockLink = {
       name: "h1",
@@ -224,10 +116,11 @@ describe("The NavLinkBuilder with a collapsable header", () => {
       name: "h3",
     };
 
-    let actual = navLinkBuilder.build([ head ], lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk3, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk4, h3, depth);
+    const actual = []
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h2);
+    navLinkBuilder.build(actual, lnk3, h2);
+    navLinkBuilder.build(actual, lnk4, h3);
 
     expect(actual).toMatchSnapshot();
   });
@@ -255,13 +148,132 @@ describe("The NavLinkBuilder with a collapsable header", () => {
       name: "h4",
     };
 
-    let actual = navLinkBuilder.build([ head ], lnk0, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk1, h1, depth);
-    actual = navLinkBuilder.build(actual, lnk2, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk3, h2, depth);
-    actual = navLinkBuilder.build(actual, lnk4, h3, depth);
-    actual = navLinkBuilder.build(actual, lnk5, h3, depth);
-    actual = navLinkBuilder.build(actual, lnk6, h4, depth);
+    const actual = [];
+    navLinkBuilder.build(actual, lnk0, h1);
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h2);
+    navLinkBuilder.build(actual, lnk3, h2);
+    navLinkBuilder.build(actual, lnk4, h3);
+    navLinkBuilder.build(actual, lnk5, h3);
+    navLinkBuilder.build(actual, lnk6, h4);
+
+    expect(actual).toMatchSnapshot();
+  });
+});
+
+describe("The NavLinkBuilder with a collapsable header", () => {
+  let head: IMockLink;
+  const depth = DEPTH_COLLAPSABLE_HEADER;
+  const h1 = depth;
+  const h2 = h1+1;
+  const h3 = h2+1;
+  const h4 = h3+1;
+
+  beforeEach(() => {
+    head = {
+      name: "head",
+    };
+  });
+
+  it("should add a single item to the heading", () => {
+    const lnk: IMockLink = {
+      name: "xyz",
+    };
+
+    const actual = [ head ];
+    navLinkBuilder.build(actual, lnk, h1);
+
+    expect(actual).toMatchSnapshot();
+  });
+
+  it("should add a two items on the same level", () => {
+    const lnk1: IMockLink = {
+      name: "xyz",
+    };
+    const lnk2: IMockLink = {
+      name: "abc",
+    };
+
+    const actual = [ head ];
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h1);
+
+    expect(actual).toMatchSnapshot();
+  })
+
+  it("should add a one item in a collapsable section two inside that one", () => {
+    const lnk1: IMockLink = {
+      name: "xyz",
+    };
+    const lnk2: IMockLink = {
+      name: "abc",
+    };
+    const lnk3: IMockLink = {
+      name: "def",
+    };
+
+    const actual = [ head ];
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h2);
+    navLinkBuilder.build(actual, lnk3, h2);
+
+    expect(actual).toMatchSnapshot();
+  })
+
+  it("should not nest two h2", () => {
+    const lnk1: IMockLink = {
+      name: "h1",
+    };
+    const lnk2: IMockLink = {
+      name: "h2",
+    };
+    const lnk3: IMockLink = {
+      name: "h2",
+    };
+    const lnk4: IMockLink = {
+      name: "h3",
+    };
+
+    const actual = [ head ];
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h2);
+    navLinkBuilder.build(actual, lnk3, h2);
+    navLinkBuilder.build(actual, lnk4, h3);
+
+    expect(actual).toMatchSnapshot();
+  });
+
+  it("should not nest two h3", () => {
+    const lnk0: IMockLink = {
+      name: "h1",
+    };
+    const lnk1: IMockLink = {
+      name: "h1",
+    };
+    const lnk2: IMockLink = {
+      name: "h2",
+    };
+    const lnk3: IMockLink = {
+      name: "h2",
+    };
+    const lnk4: IMockLink = {
+      name: "h3",
+    };
+    const lnk5: IMockLink = {
+      name: "h3",
+    };
+    const lnk6: IMockLink = {
+      name: "h4",
+    };
+
+    const actual = [ head ];
+    navLinkBuilder.build(actual, lnk0, h1);
+    navLinkBuilder.build(actual, lnk1, h1);
+    navLinkBuilder.build(actual, lnk2, h2);
+    navLinkBuilder.build(actual, lnk3, h2);
+    navLinkBuilder.build(actual, lnk4, h3);
+    navLinkBuilder.build(actual, lnk5, h3);
+    navLinkBuilder.build(actual, lnk6, h4);
 
     expect(actual).toMatchSnapshot();
   });
diff --git a/samples/react-page-navigator/src/Service/NavLinkBuilder.ts b/samples/react-page-navigator/src/Service/NavLinkBuilder.ts
index 1d9aaa273..f67552972 100644
--- a/samples/react-page-navigator/src/Service/NavLinkBuilder.ts
+++ b/samples/react-page-navigator/src/Service/NavLinkBuilder.ts
@@ -1,37 +1,27 @@
 export interface IHierarchyEntry<T extends IHierarchyEntry<T>> {
-    links?: IHierarchyEntry<T>[];
+  links?: IHierarchyEntry<T>[];
 }
 
 export class navLinkBuilder {
-  /**
-   * Nests a new nav link within the nav links tree
-   * @param currentLinks current nav links
-   * @param newLink the new nav link to be added to the structure
-   * @param order place order of the new link
-   * @param depth sequence depth
-   * @returns navLinks
-   */
-   public static build<T extends IHierarchyEntry<T>>(currentLinks: T[], newLink: T, order: number, depth: number): T[] {
-    const lastIndex = currentLinks.length - 1;
+/**
+ * Nests a new nav link within the nav links tree. Modifies the current tree IN PLACE.
+ * @param currentLinks current nav links
+ * @param newLink the new nav link to be added to the structure
+ * @param order place order of the new link
+ * @returns navLinks
+ */
+ public static build<T extends IHierarchyEntry<T>>(currentLinks: T[], newLink: T, order: number) {
+  const lastIndex = currentLinks.length - 1;
 
-    if (lastIndex === -1) {
-      return [newLink];
-    }
-
-    const lastTopLevelLink = currentLinks[lastIndex];
-    lastTopLevelLink.links = lastTopLevelLink.links || [];
-
-    if (lastTopLevelLink.links.length === 0 || order === depth) {
-      if (order !== depth || depth !== 0) {
-        lastTopLevelLink.links.push(newLink);
-      } else {
-        currentLinks.push(newLink);
-      }
-    } else {
-      depth++;
-      currentLinks[lastIndex].links.concat(this.build(currentLinks[lastIndex].links, newLink, order, depth));
-    }
-
-    return currentLinks;
+  if (lastIndex < 0 || order <= 0) {
+    currentLinks.push(newLink);
+    return;
   }
+
+  const lastTopLevelLink = currentLinks[lastIndex];
+  lastTopLevelLink.links = lastTopLevelLink.links || [];
+
+  order--;
+  this.build(lastTopLevelLink.links, newLink, order);
+}
 }
\ No newline at end of file
diff --git a/samples/react-page-navigator/src/Service/SPService.ts b/samples/react-page-navigator/src/Service/SPService.ts
index e12a1059a..abe5bfcd0 100644
--- a/samples/react-page-navigator/src/Service/SPService.ts
+++ b/samples/react-page-navigator/src/Service/SPService.ts
@@ -100,7 +100,7 @@ export class SPService {
 
             // Add link to nav element
             const newNavLink: INavLink = { name: headingValue, key: anchorUrl, url: anchorUrl, links: [], isExpanded: true };
-            anchorLinks = navLinkBuilder.build<INavLink>(anchorLinks, newNavLink, headingOrder, hasCollapsableHeader ? 1 : 0);
+            navLinkBuilder.build<INavLink>(anchorLinks, newNavLink, headingOrder);
           });
         }
       });
diff --git a/samples/react-page-navigator/src/Service/__snapshots__/NavLinkBuilder.test.ts.snap b/samples/react-page-navigator/src/Service/__snapshots__/NavLinkBuilder.test.ts.snap
index 9707cafe0..88269091c 100644
--- a/samples/react-page-navigator/src/Service/__snapshots__/NavLinkBuilder.test.ts.snap
+++ b/samples/react-page-navigator/src/Service/__snapshots__/NavLinkBuilder.test.ts.snap
@@ -160,7 +160,6 @@ Array [
 exports[`The NavLinkBuilder without a preceding collapsable header should add a two items on the same level 1`] = `
 Array [
   Object {
-    "links": Array [],
     "name": "xyz",
   },
   Object {
@@ -195,7 +194,6 @@ Array [
   Object {
     "links": Array [
       Object {
-        "links": Array [],
         "name": "h2",
       },
       Object {
@@ -215,13 +213,11 @@ Array [
 exports[`The NavLinkBuilder without a preceding collapsable header should not nest two h3 1`] = `
 Array [
   Object {
-    "links": Array [],
     "name": "h1",
   },
   Object {
     "links": Array [
       Object {
-        "links": Array[],
         "name": "h2",
       },
       Object {