Fix idempotent patchRoutes for sibling pathless layout routes (#12013)

Author: brophdawg11

packages/router/__tests__/lazy-discovery-test.ts

@@ -1330,6 +1330,196 @@ describe("Lazy Route Discovery (Fog of War)", () => {
     `);
   });
 
+  it("distinguishes sibling pathless layout routes in idempotent patch check (via id)", async () => {
+    let count = 0;
+    router = createRouter({
+      history: createMemoryHistory(),
+      routes: [
+        {
+          id: "root",
+          path: "/",
+          children: [
+            {
+              id: "a-layout",
+              children: [
+                {
+                  id: "a",
+                  path: "a",
+                },
+              ],
+            },
+          ],
+        },
+      ],
+      async patchRoutesOnNavigation({ patch, path }) {
+        count++;
+        if (path === "/b") {
+          patch("root", [
+            {
+              id: "b-layout",
+              children: [
+                {
+                  id: "b",
+                  path: "b",
+                },
+              ],
+            },
+          ]);
+        }
+        await tick();
+      },
+    });
+
+    await router.navigate("/a");
+    expect(router.state.location.pathname).toBe("/a");
+    expect(router.state.matches.map((m) => m.route.id)).toEqual([
+      "root",
+      "a-layout",
+      "a",
+    ]);
+    expect(router.state.errors).toBeNull();
+    expect(count).toBe(0);
+
+    await router.navigate("/b");
+    expect(router.state.location.pathname).toBe("/b");
+    expect(router.state.matches.map((m) => m.route.id)).toEqual([
+      "root",
+      "b-layout",
+      "b",
+    ]);
+    expect(router.state.errors).toBeNull();
+    expect(count).toBe(1);
+
+    expect(router.routes).toMatchInlineSnapshot(`
+      [
+        {
+          "children": [
+            {
+              "children": [
+                {
+                  "children": undefined,
+                  "hasErrorBoundary": false,
+                  "id": "a",
+                  "path": "a",
+                },
+              ],
+              "hasErrorBoundary": false,
+              "id": "a-layout",
+            },
+            {
+              "children": [
+                {
+                  "children": undefined,
+                  "hasErrorBoundary": false,
+                  "id": "b",
+                  "path": "b",
+                },
+              ],
+              "hasErrorBoundary": false,
+              "id": "b-layout",
+            },
+          ],
+          "hasErrorBoundary": false,
+          "id": "root",
+          "path": "/",
+        },
+      ]
+    `);
+  });
+
+  it("distinguishes sibling pathless layout routes in idempotent patch check (via children)", async () => {
+    let count = 0;
+    router = createRouter({
+      history: createMemoryHistory(),
+      routes: [
+        {
+          id: "root",
+          path: "/",
+          children: [
+            {
+              children: [
+                {
+                  path: "a",
+                },
+              ],
+            },
+          ],
+        },
+      ],
+      async patchRoutesOnNavigation({ patch, path }) {
+        count++;
+        if (path === "/b") {
+          patch("root", [
+            {
+              children: [
+                {
+                  path: "b",
+                },
+              ],
+            },
+          ]);
+        }
+        await tick();
+      },
+    });
+
+    await router.navigate("/a");
+    expect(router.state.location.pathname).toBe("/a");
+    expect(router.state.matches.map((m) => m.route.id)).toEqual([
+      "root",
+      "0-0",
+      "0-0-0",
+    ]);
+    expect(router.state.errors).toBeNull();
+    expect(count).toBe(0);
+
+    await router.navigate("/b");
+    expect(router.state.location.pathname).toBe("/b");
+    expect(router.state.matches.map((m) => m.route.id)).toEqual([
+      "root",
+      "root-patch-1-0",
+      "root-patch-1-0-0",
+    ]);
+    expect(router.state.errors).toBeNull();
+    expect(count).toBe(1);
+
+    expect(router.routes).toMatchInlineSnapshot(`
+      [
+        {
+          "children": [
+            {
+              "children": [
+                {
+                  "children": undefined,
+                  "hasErrorBoundary": false,
+                  "id": "0-0-0",
+                  "path": "a",
+                },
+              ],
+              "hasErrorBoundary": false,
+              "id": "0-0",
+            },
+            {
+              "children": [
+                {
+                  "children": undefined,
+                  "hasErrorBoundary": false,
+                  "id": "root-patch-1-0-0",
+                  "path": "b",
+                },
+              ],
+              "hasErrorBoundary": false,
+              "id": "root-patch-1-0",
+            },
+          ],
+          "hasErrorBoundary": false,
+          "id": "root",
+          "path": "/",
+        },
+      ]
+    `);
+  });
+
   describe("errors", () => {
     it("lazy 404s (GET navigation)", async () => {
       let childrenDfd = createDeferred<AgnosticDataRouteObject[]>();

packages/router/router.ts

@@ -4677,12 +4677,9 @@ function patchRoutesImpl(
   // to simplify user-land code. This is useful because we re-call the
   // `patchRoutesOnNavigation` function for matched routes with params.
   let uniqueChildren = children.filter(
-    (a) =>
-      !childrenToPatch.some(
-        (b) =>
-          a.index === b.index &&
-          a.path === b.path &&
-          a.caseSensitive === b.caseSensitive
+    (newRoute) =>
+      !childrenToPatch.some((existingRoute) =>
+        isSameRoute(newRoute, existingRoute)
       )
   );
 
@@ -4696,6 +4693,46 @@ function patchRoutesImpl(
   childrenToPatch.push(...newRoutes);
 }
 
+function isSameRoute(
+  newRoute: AgnosticRouteObject,
+  existingRoute: AgnosticRouteObject
+): boolean {
+  // Most optimal check is by id
+  if (
+    "id" in newRoute &&
+    "id" in existingRoute &&
+    newRoute.id === existingRoute.id
+  ) {
+    return true;
+  }
+
+  // Second is by pathing differences
+  if (
+    !(
+      newRoute.index === existingRoute.index &&
+      newRoute.path === existingRoute.path &&
+      newRoute.caseSensitive === existingRoute.caseSensitive
+    )
+  ) {
+    return false;
+  }
+
+  // Pathless layout routes are trickier since we need to check children.
+  // If they have no children then they're the same as far as we can tell
+  if (
+    (!newRoute.children || newRoute.children.length === 0) &&
+    (!existingRoute.children || existingRoute.children.length === 0)
+  ) {
+    return true;
+  }
+
+  // Otherwise, we look to see if every child in the new route is already
+  // represented in the existing route's children
+  return newRoute.children!.every((aChild, i) =>
+    existingRoute.children?.some((bChild) => isSameRoute(aChild, bChild))
+  );
+}
+
 /**
  * Execute route.lazy() methods to lazily load route modules (loader, action,
  * shouldRevalidate) and update the routeManifest in place which shares objects