fix: Inserting Slots within Slots from the Marketplace causes infinit…

…e loop and crash (#4113)

## Description

closes #4111

- [x] - Fixes client side
- [x] - Fixes server side


https://github.com/user-attachments/assets/8bb6ffdc-2e78-45eb-b98b-e697daee822a

## Steps for reproduction

Now seems like impossible to save broken project. Tested locally on
broken project.


## Code Review

- [ ] hi @kof, I need you to do
  - conceptual review (architecture, feature-correctness)
  - detailed review (read every line)
  - test it on preview

## Before requesting a review

- [ ] made a self-review
- [ ] added inline comments where things may be not obvious (the "why",
not "what")

## Before merging

- [ ] tested locally and on preview environment (preview dev login:
5de6)
- [ ] updated [test
cases](https://github.com/webstudio-is/webstudio/blob/main/apps/builder/docs/test-cases.md)
document
- [ ] added tests
- [ ] if any new env variables are added, added them to `.env` file

apps/builder/app/routes/rest.patch.ts

@@ -18,7 +18,11 @@ import {
   Resources,
   Resource,
 } from "@webstudio-is/sdk";
-import { type Build, MarketplaceProduct } from "@webstudio-is/project-build";
+import {
+  type Build,
+  findCycles,
+  MarketplaceProduct,
+} from "@webstudio-is/project-build";
 import {
   parsePages,
   parseStyleSourceSelections,
@@ -31,6 +35,7 @@ import {
   parseConfig,
   serializeConfig,
   loadRawBuildById,
+  parseInstanceData,
 } from "@webstudio-is/project-build/index.server";
 import { patchAssets } from "@webstudio-is/asset-uploader/index.server";
 import type { Project } from "@webstudio-is/project";
@@ -183,8 +188,24 @@ export const action = async ({
 
         if (namespace === "instances") {
           const instances =
-            buildData.instances ?? parseData<Instance>(build.instances);
+            buildData.instances ?? parseInstanceData(build.instances);
+
           buildData.instances = applyPatches(instances, patches);
+
+          // Detect cycles
+          const cycles = findCycles(buildData.instances.values());
+          if (cycles.length > 0) {
+            console.error(
+              "Cycles detected in the instance tree after patching",
+              cycles
+            );
+
+            return {
+              status: "error",
+              errors: "Cycles detected in the instance tree",
+            };
+          }
+
           continue;
         }
 

apps/builder/app/shared/instance-utils.ts

@@ -62,6 +62,7 @@ import { isBaseBreakpoint } from "./breakpoints";
 import { humanizeString } from "./string-utils";
 import { serverSyncStore } from "./sync";
 import { setDifference, setUnion } from "./shim";
+import { breakCyclesMutable, findCycles } from "@webstudio-is/project-build";
 
 export const updateWebstudioData = (mutate: (data: WebstudioData) => void) => {
   serverSyncStore.createTransaction(
@@ -105,6 +106,18 @@ export const updateWebstudioData = (mutate: (data: WebstudioData) => void) => {
         styles,
         assets,
       });
+
+      const cycles = findCycles(instances.values());
+
+      // Detect and fix cycles in the instance tree, then report
+      if (cycles.length > 0) {
+        toast.info("Detected and fixed cycles in the instance tree.");
+
+        breakCyclesMutable(
+          instances.values(),
+          (node) => node.component === "Slot"
+        );
+      }
     }
   );
 };

apps/builder/app/shared/sync/sync-stores.ts

@@ -336,7 +336,6 @@ export const useBuilderStore = (publish: Publish) => {
       // @todo subscribe prematurely and compute initial changes
       // from current state whenever new app is connected
       unsubscribeStoresState = syncStoresState("builder", publish);
-      unsubscribeStoresChanges = syncStoresChanges("builder", publish);
       // immerhin data is sent only initially so not part of syncStoresState
       // expect data to be populated by the time effect is called
       const data = [];
@@ -361,6 +360,8 @@ export const useBuilderStore = (publish: Publish) => {
           data,
         },
       });
+
+      unsubscribeStoresChanges = syncStoresChanges("builder", publish);
     });
 
     const unsubscribeDisconnect = subscribe("disconnect", () => {

packages/project-build/src/db/build.ts

@@ -25,17 +25,36 @@ import { parseDeployment } from "./deployment";
 import { serializePages } from "./pages";
 import { createDefaultPages } from "../shared/pages-utils";
 import type { MarketplaceProduct } from "../shared//marketplace";
+import { breakCyclesMutable } from "../shared/graph-utils";
 
 const parseCompactData = <Item>(serialized: string) =>
   JSON.parse(serialized) as Item[];
 
+const parseCompactInstanceData = (serialized: string) => {
+  const instances = JSON.parse(serialized) as Instance[];
+
+  // @todo: Remove after measurements on real data
+  console.time("breakCyclesMutable");
+  breakCyclesMutable(instances, (node) => node.component === "Slot");
+  console.timeEnd("breakCyclesMutable");
+
+  return instances;
+};
+
 export const parseData = <Type extends { id: string }>(
   string: string
 ): Map<Type["id"], Type> => {
   const list = JSON.parse(string) as Type[];
   return new Map(list.map((item) => [item.id, item]));
 };
 
+export const parseInstanceData = (
+  string: string
+): Map<Instance["id"], Instance> => {
+  const list = parseCompactInstanceData(string);
+  return new Map(list.map((item) => [item.id, item]));
+};
+
 export const serializeData = <Type extends { id: string }>(
   data: Map<Type["id"], Type>
 ) => {
@@ -71,7 +90,7 @@ const parseCompactBuild = async (
       props: parseCompactData<Prop>(build.props),
       dataSources: parseCompactData<DataSource>(build.dataSources),
       resources: parseCompactData<Resource>(build.resources),
-      instances: parseCompactData<Instance>(build.instances),
+      instances: parseCompactInstanceData(build.instances),
       deployment: parseDeployment(build.deployment),
       marketplaceProduct: parseConfig<MarketplaceProduct>(
         build.marketplaceProduct

packages/project-build/src/index.ts

@@ -1,3 +1,4 @@
 export * from "./types";
 export * from "./shared/pages-utils";
 export * from "./shared/marketplace";
+export * from "./shared/graph-utils";

packages/project-build/src/shared/graph-utils.test.ts

@@ -0,0 +1,147 @@
+import { test, expect, describe } from "@jest/globals";
+import { findCycles, breakCyclesMutable } from "./graph-utils";
+import type { Instance } from "@webstudio-is/sdk";
+
+const typeId = "id" as const;
+
+describe("findCycles", () => {
+  test("should return an empty array for an empty graph", () => {
+    const graph: Instance[] = [];
+    const result = findCycles(graph);
+    expect(result).toEqual([]);
+  });
+
+  test("should return an empty array for a graph with no cycles", () => {
+    const graph = [
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      { id: "2", children: [{ type: typeId, value: "3" }] },
+      { id: "3", children: [] },
+    ];
+    const result = findCycles(graph);
+    expect(result).toEqual([]);
+  });
+
+  test("should return a single cycle for a graph with one cycle", () => {
+    const graph = [
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      { id: "2", children: [{ type: typeId, value: "3" }] },
+      { id: "3", children: [{ type: typeId, value: "1" }] },
+    ];
+    const result = findCycles(graph);
+    expect(result).toEqual([["1", "2", "3", "1"]]);
+  });
+
+  test("should return multiple cycles for a graph with multiple cycles", () => {
+    const graph = [
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      {
+        id: "2",
+        children: [
+          { type: typeId, value: "3" },
+          { type: typeId, value: "4" },
+        ],
+      },
+      { id: "3", children: [{ type: typeId, value: "1" }] },
+      { id: "4", children: [{ type: typeId, value: "2" }] },
+    ];
+    const result = findCycles(graph);
+    expect(result).toEqual([
+      ["1", "2", "3", "1"],
+      ["2", "4", "2"],
+    ]);
+  });
+
+  test("should return multiple cycles for a graph with multiple inline cycles", () => {
+    const graph = [
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      {
+        id: "2",
+        children: [{ type: typeId, value: "3" }],
+      },
+      {
+        id: "3",
+        children: [
+          { type: typeId, value: "4" },
+          { type: typeId, value: "2" },
+        ],
+      },
+      { id: "4", children: [{ type: typeId, value: "1" }] },
+    ];
+
+    const result = findCycles(graph);
+
+    expect(result).toEqual([
+      ["1", "2", "3", "4", "1"],
+      ["2", "3", "2"],
+    ]);
+  });
+});
+
+describe("breakCyclesMutable", () => {
+  test("should return the same instances for an empty graph", () => {
+    const result = breakCyclesMutable([], () => false);
+
+    expect(result).toEqual([]);
+  });
+
+  test("should return the same instances for a graph with no cycles", () => {
+    const instances = [
+      { id: "1", component: "Slot", children: [{ type: typeId, value: "2" }] },
+      { id: "2", children: [{ type: typeId, value: "3" }] },
+      { id: "3", children: [] },
+    ];
+    const result = breakCyclesMutable(
+      instances,
+      (node) => node?.component === "Slot"
+    );
+    expect(result).toEqual(instances);
+  });
+
+  test("should break a single cycle in the graph", () => {
+    const instances = [
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      { id: "2", children: [{ type: typeId, value: "3" }] },
+      { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] },
+    ];
+
+    const result = breakCyclesMutable(
+      instances,
+      (node) => node?.component === "Slot"
+    );
+
+    expect(result).toEqual([
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      { id: "2", children: [] },
+      { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] },
+    ]);
+  });
+
+  test("should break multiple cycles in the graph", () => {
+    const instances = [
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      {
+        id: "2",
+        children: [
+          { type: typeId, value: "3" },
+          { type: typeId, value: "4" },
+        ],
+      },
+      { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] },
+      { id: "4", component: "Slot", children: [{ type: typeId, value: "2" }] },
+    ];
+
+    const result = breakCyclesMutable(
+      instances,
+      (node) => node?.component === "Slot"
+    );
+    expect(result).toEqual([
+      { id: "1", children: [{ type: typeId, value: "2" }] },
+      {
+        id: "2",
+        children: [],
+      },
+      { id: "3", component: "Slot", children: [{ type: typeId, value: "1" }] },
+      { id: "4", component: "Slot", children: [{ type: typeId, value: "2" }] },
+    ]);
+  });
+});

packages/project-build/src/shared/graph-utils.ts

@@ -0,0 +1,93 @@
+import type { Instance } from "@webstudio-is/sdk";
+
+type InstanceId = Instance["id"];
+
+// Depth-First Search (DFS) algorithm to find cycles in a directed graph
+export const findCycles = (
+  graph: Iterable<Pick<Instance, "id" | "children">>
+): InstanceId[][] => {
+  const adjacencyList: Record<InstanceId, InstanceId[]> = {};
+
+  // Build adjacency list
+  for (const node of graph) {
+    adjacencyList[node.id] = node.children
+      .filter((child) => child.type === "id")
+      .map((child) => child.value);
+  }
+
+  const visited = new Set<string>();
+  const path: InstanceId[] = [];
+  const cycles: InstanceId[][] = [];
+
+  const dfs = (nodeId: string): void => {
+    if (path.includes(nodeId)) {
+      const cycleStart = path.indexOf(nodeId);
+      cycles.push(path.slice(cycleStart).concat(nodeId));
+      return;
+    }
+
+    if (visited.has(nodeId)) {
+      return;
+    }
+
+    visited.add(nodeId);
+    path.push(nodeId);
+
+    for (const childId of adjacencyList[nodeId] || []) {
+      dfs(childId);
+    }
+
+    path.pop();
+  };
+
+  // Start DFS from each node
+  for (const node of graph) {
+    if (!visited.has(node.id)) {
+      dfs(node.id);
+    }
+  }
+
+  return cycles;
+};
+
+export const breakCyclesMutable = <T extends Pick<Instance, "id" | "children">>(
+  instances: Iterable<T>,
+  breakOn: (node: T) => boolean
+) => {
+  const cycles = findCycles(instances);
+  if (cycles.length === 0) {
+    return instances;
+  }
+
+  const cycleInstances = new Map<T["id"], T>();
+  const cycleInstanceIdSet = new Set<T["id"]>(cycles.flat());
+
+  // Pick all instances that are part of the cycle
+  for (const instance of instances) {
+    if (cycleInstanceIdSet.has(instance.id)) {
+      cycleInstances.set(instance.id, instance);
+    }
+  }
+
+  for (const cycle of cycles) {
+    // Find slot or take last instance
+    const slotId =
+      cycle.find((id) => breakOn(cycleInstances.get(id)!)) ??
+      cycle[cycle.length - 1];
+
+    // Remove slot from children of all instances in the cycle
+    for (const id of cycle) {
+      const instance = cycleInstances.get(id);
+      if (instance === undefined) {
+        continue;
+      }
+
+      if (instance.children.find((child) => child.value === slotId)) {
+        instance.children = instance.children.filter(
+          (child) => child.value !== slotId
+        );
+      }
+    }
+  }
+  return instances;
+};