fix: relax detaching constraint

Every time somebody is facing invalid constraint issue whole tree is blocked from deleting instances except the problematic one with actual issue. Here rewrote detachability check to validate only ancestors and avoid traversing potentially problematic siblings.
webstudio-is · Dec 29, 2024 · 7822f5c · 7822f5c
1 parent ef6e71a
commit 7822f5c
Show file tree

Hide file tree

Showing 6 changed files with 132 additions and 38 deletions.
diff --git a/apps/builder/app/builder/features/ai/apply-operations.ts b/apps/builder/app/builder/features/ai/apply-operations.ts
@@ -7,7 +7,6 @@ import { isBaseBreakpoint } from "~/shared/breakpoints";
 import {
   deleteInstanceMutable,
   insertWebstudioFragmentAt,
-  isInstanceDetachable,
   updateWebstudioData,
   type Insertable,
 } from "~/shared/instance-utils";
@@ -22,6 +21,7 @@ import {
 } from "~/shared/nano-states";
 import type { InstanceSelector } from "~/shared/tree-utils";
 import { $selectedInstance } from "~/shared/awareness";
+import { isInstanceDetachable } from "~/shared/matcher";
 
 export const applyOperations = (operations: operations.WsOperations) => {
   for (const operation of operations) {
@@ -96,8 +96,15 @@ const deleteInstanceByOp = (
     if (instanceSelector.length === 1) {
       return;
     }
+    const metas = $registeredComponentMetas.get();
     updateWebstudioData((data) => {
-      if (isInstanceDetachable(data.instances, instanceSelector) === false) {
+      if (
+        isInstanceDetachable({
+          metas,
+          instances: data.instances,
+          instanceSelector,
+        }) === false
+      ) {
         return;
       }
       deleteInstanceMutable(data, instanceSelector);

diff --git a/apps/builder/app/builder/shared/commands.ts b/apps/builder/app/builder/shared/commands.ts
@@ -24,7 +24,6 @@ import {
   extractWebstudioFragment,
   insertWebstudioFragmentCopy,
   updateWebstudioData,
-  isInstanceDetachable,
 } from "~/shared/instance-utils";
 import type { InstanceSelector } from "~/shared/tree-utils";
 import { serverSyncStore } from "~/shared/sync";
@@ -41,6 +40,7 @@ import { builderApi } from "~/shared/builder-api";
 
 import {
   findClosestNonTextualContainer,
+  isInstanceDetachable,
   isTreeMatching,
 } from "~/shared/matcher";
 
@@ -75,7 +75,14 @@ export const deleteSelectedInstance = () => {
   const [selectedItem, parentItem] = instancePath;
   const selectedInstanceSelector = selectedItem.instanceSelector;
   const instances = $instances.get();
-  if (isInstanceDetachable(instances, selectedInstanceSelector) === false) {
+  const metas = $registeredComponentMetas.get();
+  if (
+    isInstanceDetachable({
+      metas,
+      instances,
+      instanceSelector: selectedInstanceSelector,
+    }) === false
+  ) {
     toast.error(
       "This instance can not be moved outside of its parent component."
     );

diff --git a/apps/builder/app/shared/copy-paste/plugin-instance.ts b/apps/builder/app/shared/copy-paste/plugin-instance.ts
@@ -8,19 +8,23 @@ import {
   WebstudioFragment,
   findTreeInstanceIdsExcludingSlotDescendants,
 } from "@webstudio-is/sdk";
-import { $selectedInstanceSelector, $instances } from "../nano-states";
+import {
+  $selectedInstanceSelector,
+  $instances,
+  $registeredComponentMetas,
+} from "../nano-states";
 import type { InstanceSelector, DroppableTarget } from "../tree-utils";
 import {
   deleteInstanceMutable,
   findAvailableDataSources,
   extractWebstudioFragment,
   insertWebstudioFragmentCopy,
-  isInstanceDetachable,
   updateWebstudioData,
   getWebstudioData,
   insertInstanceChildrenMutable,
   findClosestInsertable,
 } from "../instance-utils";
+import { isInstanceDetachable } from "../matcher";
 
 const version = "@webstudio/instance/v0.1";
 
@@ -30,24 +34,25 @@ const InstanceData = WebstudioFragment.extend({
 
 type InstanceData = z.infer<typeof InstanceData>;
 
-const getTreeData = (targetInstanceSelector: InstanceSelector) => {
+const getTreeData = (instanceSelector: InstanceSelector) => {
   const instances = $instances.get();
-  if (isInstanceDetachable(instances, targetInstanceSelector) === false) {
+  const metas = $registeredComponentMetas.get();
+  if (isInstanceDetachable({ metas, instances, instanceSelector }) === false) {
     toast.error(
       "This instance can not be moved outside of its parent component."
     );
     return;
   }
 
   // @todo tell user they can't copy or cut root
-  if (targetInstanceSelector.length === 1) {
+  if (instanceSelector.length === 1) {
     return;
   }
 
-  const [targetInstanceId] = targetInstanceSelector;
+  const [targetInstanceId] = instanceSelector;
 
   return {
-    instanceSelector: targetInstanceSelector,
+    instanceSelector,
     ...extractWebstudioFragment(getWebstudioData(), targetInstanceId),
   };
 };

diff --git a/apps/builder/app/shared/instance-utils.ts b/apps/builder/app/shared/instance-utils.ts
@@ -65,7 +65,6 @@ import { $awareness, $selectedPage, selectInstance } from "./awareness";
 import {
   findClosestNonTextualContainer,
   findClosestInstanceMatchingFragment,
-  isTreeMatching,
 } from "./matcher";
 
 export const updateWebstudioData = (mutate: (data: WebstudioData) => void) => {
@@ -258,32 +257,6 @@ export const findClosestEditableInstanceSelector = (
   }
 };
 
-export const isInstanceDetachable = (
-  instances: Instances,
-  instanceSelector: InstanceSelector
-) => {
-  const metas = $registeredComponentMetas.get();
-  const [instanceId, parentId] = instanceSelector;
-  const newInstances = new Map(instances);
-  // replace parent with the one without selected instance
-  let parentInstance = newInstances.get(parentId);
-  if (parentInstance) {
-    parentInstance = {
-      ...parentInstance,
-      children: parentInstance.children.filter(
-        (child) => child.type === "id" && child.value !== instanceId
-      ),
-    };
-    newInstances.set(parentInstance.id, parentInstance);
-  }
-  // check parent can follow constraints without selected instance
-  return isTreeMatching({
-    instances: newInstances,
-    metas,
-    instanceSelector: instanceSelector.slice(1),
-  });
-};
-
 export const insertInstanceChildrenMutable = (
   data: WebstudioData,
   children: Instance["children"],

diff --git a/apps/builder/app/shared/matcher.test.tsx b/apps/builder/app/shared/matcher.test.tsx
@@ -7,6 +7,7 @@ import {
 } from "@webstudio-is/template";
 import { coreMetas } from "@webstudio-is/react-sdk";
 import * as baseMetas from "@webstudio-is/sdk-components-react/metas";
+import * as radixMetas from "@webstudio-is/sdk-components-react-radix/metas";
 import type { WsComponentMeta } from "@webstudio-is/react-sdk";
 import type { Matcher } from "@webstudio-is/sdk";
 import {
@@ -15,6 +16,7 @@ import {
   isInstanceMatching,
   isTreeMatching,
   findClosestContainer,
+  isInstanceDetachable,
 } from "./matcher";
 
 const metas = new Map(Object.entries({ ...coreMetas, ...baseMetas }));
@@ -757,6 +759,65 @@ describe("is tree matching", () => {
   });
 });
 
+describe("is instance detachable", () => {
+  const metas = new Map(Object.entries({ ...baseMetas, ...radixMetas }));
+
+  test("allow deleting one of matching instances", () => {
+    expect(
+      isInstanceDetachable({
+        ...renderJsx(
+          <$.Body ws:id="body">
+            <$.Tabs ws:id="tabs">
+              <$.TabsList ws:id="list">
+                <$.TabsTrigger ws:id="trigger1"></$.TabsTrigger>
+                <$.TabsTrigger ws:id="trigger2"></$.TabsTrigger>
+              </$.TabsList>
+              <$.TabsContent ws:id="content1"></$.TabsContent>
+              <$.TabsContent ws:id="content2"></$.TabsContent>
+            </$.Tabs>
+          </$.Body>
+        ),
+        metas,
+        instanceSelector: ["trigger1", "list", "tabs", "body"],
+      })
+    ).toBeTruthy();
+  });
+
+  test("prevent deleting last matching instance", () => {
+    expect(
+      isInstanceDetachable({
+        ...renderJsx(
+          <$.Body ws:id="body">
+            <$.Tabs ws:id="tabs">
+              <$.TabsList ws:id="list">
+                <$.TabsTrigger ws:id="trigger1"></$.TabsTrigger>
+              </$.TabsList>
+              <$.TabsContent ws:id="content1"></$.TabsContent>
+            </$.Tabs>
+          </$.Body>
+        ),
+        metas,
+        instanceSelector: ["trigger1", "list", "tabs", "body"],
+      })
+    ).toBeFalsy();
+  });
+
+  test("allow deleting when siblings not matching", () => {
+    expect(
+      isInstanceDetachable({
+        ...renderJsx(
+          <$.Body ws:id="body">
+            <$.Tabs ws:id="tabs"></$.Tabs>
+            <$.Box ws:id="box"></$.Box>
+          </$.Body>
+        ),
+        metas,
+        instanceSelector: ["box", "body"],
+      })
+    ).toBeTruthy();
+  });
+});
+
 describe("find closest instance matching fragment", () => {
   test("finds closest list with list item fragment", () => {
     const { instances } = renderJsx(

diff --git a/apps/builder/app/shared/matcher.ts b/apps/builder/app/shared/matcher.ts
@@ -234,6 +234,47 @@ export const isTreeMatching = ({
   return matches;
 };
 
+export const isInstanceDetachable = ({
+  instances,
+  metas,
+  instanceSelector,
+}: {
+  instances: Instances;
+  metas: Map<string, WsComponentMeta>;
+  instanceSelector: InstanceSelector;
+}) => {
+  const [instanceId, parentId] = instanceSelector;
+  const newInstances = new Map(instances);
+  // replace parent with the one without selected instance
+  let parentInstance = newInstances.get(parentId);
+  if (parentInstance) {
+    parentInstance = {
+      ...parentInstance,
+      children: parentInstance.children.filter(
+        (child) => child.type === "id" && child.value !== instanceId
+      ),
+    };
+    newInstances.set(parentInstance.id, parentInstance);
+  }
+  // skip self
+  for (let index = 1; index < instanceSelector.length; index += 1) {
+    const instance = newInstances.get(instanceSelector[index]);
+    if (instance === undefined) {
+      continue;
+    }
+    const meta = metas.get(instance.component);
+    const matches = isInstanceMatching({
+      instances: newInstances,
+      instanceSelector: instanceSelector.slice(index),
+      query: meta?.constraints,
+    });
+    if (matches === false) {
+      return false;
+    }
+  }
+  return true;
+};
+
 export const findClosestInstanceMatchingFragment = ({
   instances,
   metas,