refactor: improve constraints errors (#4535)

Now errors are toasted when insert or paste anything. Legacy constraints are removed from components.
webstudio-is · Dec 8, 2024 · fb2c6b5 · fb2c6b5
1 parent 0dbdbe6
commit fb2c6b5
Show file tree

Hide file tree

Showing 41 changed files with 220 additions and 110 deletions.
diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml
@@ -83,8 +83,8 @@ jobs:
               }
             }
             const results = [
-              await assertSize('./fixtures/ssg/dist/client', 288),
-              await assertSize('./fixtures/webstudio-remix-netlify-functions/build/client', 376),
+              await assertSize('./fixtures/ssg/dist/client', 352),
+              await assertSize('./fixtures/webstudio-remix-netlify-functions/build/client', 440),
               await assertSize('./fixtures/webstudio-remix-vercel/build/client', 908),
             ]
             for (const result of results) {

diff --git a/apps/builder/app/builder/features/command-panel/command-panel.tsx b/apps/builder/app/builder/features/command-panel/command-panel.tsx
@@ -35,10 +35,14 @@ import {
   $selectedBreakpoint,
   $selectedBreakpointId,
 } from "~/shared/nano-states";
-import { getInstanceLabel } from "~/shared/instance-utils";
+import {
+  findClosestInsertable,
+  getComponentTemplateData,
+  getInstanceLabel,
+  insertTemplateData,
+} from "~/shared/instance-utils";
 import { humanizeString } from "~/shared/string-utils";
 import { setCanvasWidth } from "~/builder/features/breakpoints";
-import { insert as insertComponent } from "~/builder/features/components/insert";
 import { $selectedPage, selectPage } from "~/shared/awareness";
 import { mapGroupBy } from "~/shared/shim";
 import { setActiveSidebarPanel } from "~/builder/shared/nano-states";
@@ -146,7 +150,13 @@ const ComponentOptionsGroup = ({ options }: { options: ComponentOption[] }) => {
             value={component}
             onSelect={() => {
               closeCommandPanel();
-              insertComponent(component);
+              const fragment = getComponentTemplateData(component);
+              if (fragment) {
+                const insertable = findClosestInsertable(fragment);
+                if (insertable) {
+                  insertTemplateData(fragment, insertable);
+                }
+              }
             }}
           >
             <Flex gap={2}>

diff --git a/apps/builder/app/builder/features/components/components.tsx b/apps/builder/app/builder/features/components/components.tsx
@@ -34,9 +34,13 @@ import {
   type MetaByCategory,
   type ComponentNamesByMeta,
 } from "./get-meta-maps";
-import { getInstanceLabel } from "~/shared/instance-utils";
+import {
+  findClosestInsertable,
+  getComponentTemplateData,
+  getInstanceLabel,
+  insertTemplateData,
+} from "~/shared/instance-utils";
 import { isFeatureEnabled } from "@webstudio-is/feature-flags";
-import { insert } from "./insert";
 import { matchSorter } from "match-sorter";
 import { parseComponentName } from "@webstudio-is/sdk";
 import type { Publish } from "~/shared/pubsub";
@@ -183,7 +187,13 @@ export const ComponentsPanel = ({
 
   const handleInsert = (component: string) => {
     onClose();
-    insert(component);
+    const fragment = getComponentTemplateData(component);
+    if (fragment) {
+      const insertable = findClosestInsertable(fragment);
+      if (insertable) {
+        insertTemplateData(fragment, insertable);
+      }
+    }
   };
 
   const resetSelectedComponent = () => {

diff --git a/apps/builder/app/builder/features/components/insert.ts b/apps/builder/app/builder/features/components/insert.ts
diff --git a/apps/builder/app/shared/instance-utils.ts b/apps/builder/app/shared/instance-utils.ts
@@ -1459,6 +1459,7 @@ export const findClosestInsertable = (
     instances,
     instanceSelector: instanceSelector.slice(closestContainerIndex),
     fragment,
+    onError: (message) => toast.error(message),
   });
   if (insertableIndex === -1) {
     return;

diff --git a/apps/builder/app/shared/matcher.test.tsx b/apps/builder/app/shared/matcher.test.tsx
@@ -1,4 +1,4 @@
-import { describe, expect, test } from "vitest";
+import { describe, expect, test, vi } from "vitest";
 import { renderJsx, $, ExpressionValue } from "@webstudio-is/sdk/testing";
 import { coreMetas } from "@webstudio-is/react-sdk";
 import * as baseMetas from "@webstudio-is/sdk-components-react/metas";
@@ -563,6 +563,72 @@ describe("is instance matching", () => {
       })
     ).toBeFalsy();
   });
+
+  test("provide error message when negated matcher is failed", () => {
+    const onError = vi.fn();
+    isInstanceMatching({
+      ...renderJsx(
+        <$.Body ws:id="body">
+          <$.Box ws:id="box"></$.Box>
+        </$.Body>
+      ),
+      instanceSelector: ["box", "body"],
+      query: {
+        relation: "self",
+        component: { $nin: ["Box", "Text"] },
+      },
+      onError,
+    });
+    expect(onError).toHaveBeenLastCalledWith("Box or Text is not acceptable");
+    isInstanceMatching({
+      ...renderJsx(
+        <$.Body ws:id="body">
+          <$.Box ws:id="box">
+            <$.ListItem ws:id="listitem"></$.ListItem>
+          </$.Box>
+        </$.Body>
+      ),
+      instanceSelector: ["listitem", "box", "body"],
+      query: {
+        relation: "ancestor",
+        component: { $nin: ["Box", "Text"] },
+      },
+      onError,
+    });
+    expect(onError).toHaveBeenLastCalledWith("Box or Text is not acceptable");
+  });
+
+  test("provide error message when positive matcher is failed", () => {
+    const onError = vi.fn();
+    isInstanceMatching({
+      ...renderJsx(
+        <$.Body ws:id="body">
+          <$.ListItem ws:id="listitem"></$.ListItem>
+        </$.Body>
+      ),
+      instanceSelector: ["box", "body"],
+      query: {
+        relation: "self",
+        component: { $in: ["Box", "Text"] },
+      },
+      onError,
+    });
+    expect(onError).toHaveBeenLastCalledWith("Box or Text is missing");
+    isInstanceMatching({
+      ...renderJsx(
+        <$.Body ws:id="body">
+          <$.ListItem ws:id="listitem"></$.ListItem>
+        </$.Body>
+      ),
+      instanceSelector: ["box", "body"],
+      query: {
+        relation: "ancestor",
+        component: { $in: ["Box", "Text"] },
+      },
+      onError,
+    });
+    expect(onError).toHaveBeenLastCalledWith("Box or Text is missing");
+  });
 });
 
 describe("is tree matching", () => {
@@ -781,6 +847,25 @@ describe("find closest instance matching fragment", () => {
       })
     ).toEqual(1);
   });
+
+  test("report first error", () => {
+    const onError = vi.fn();
+    const { instances } = renderJsx(<$.Body ws:id="body"></$.Body>);
+    const fragment = createFragment(
+      // only children are tested
+      <>
+        <$.ListItem ws:id="listitem"></$.ListItem>
+      </>
+    );
+    findClosestInstanceMatchingFragment({
+      metas,
+      instances,
+      instanceSelector: ["body"],
+      fragment,
+      onError,
+    });
+    expect(onError).toHaveBeenLastCalledWith("List is missing");
+  });
 });
 
 describe("find closest container", () => {

diff --git a/apps/builder/app/shared/matcher.ts b/apps/builder/app/shared/matcher.ts
@@ -48,6 +48,26 @@ const isLevelMatchingRelation = (level: number, relation: MatcherRelation) => {
   );
 };
 
+const formatList = (operation: MatcherOperation) => {
+  const listFormat = new Intl.ListFormat("en", {
+    type: "disjunction",
+  });
+  const list: string[] = [];
+  if (operation.$eq) {
+    list.push(operation.$eq);
+  }
+  if (operation.$in) {
+    list.push(...operation.$in);
+  }
+  if (operation.$neq) {
+    list.push(operation.$neq);
+  }
+  if (operation.$nin) {
+    list.push(...operation.$nin);
+  }
+  return listFormat.format(list);
+};
+
 /**
  * @todo following features are missing
  * - tag matcher
@@ -56,10 +76,12 @@ export const isInstanceMatching = ({
   instances,
   instanceSelector,
   query,
+  onError,
 }: {
   instances: Instances;
   instanceSelector: InstanceSelector;
   query: undefined | Matcher | Matcher[];
+  onError?: (message: string) => void;
 }): boolean => {
   // fast path to the lack of constraints
   if (query === undefined) {
@@ -73,6 +95,9 @@ export const isInstanceMatching = ({
     if (isNegated(matcher.component)) {
       if (matches) {
         aborted = true;
+        if (matcher.component) {
+          onError?.(`${formatList(matcher.component)} is not acceptable`);
+        }
         return;
       }
       // inverse negated match
@@ -124,18 +149,29 @@ export const isInstanceMatching = ({
   if (aborted) {
     return false;
   }
-  return queries.every((matcher) => matchesByMatcher.get(matcher));
+  for (const matcher of queries) {
+    const matches = matchesByMatcher.get(matcher) ?? false;
+    if (matches === false) {
+      if (matcher.component) {
+        onError?.(`${formatList(matcher.component)} is missing`);
+      }
+      return false;
+    }
+  }
+  return true;
 };
 
 export const isTreeMatching = ({
   instances,
   metas,
   instanceSelector,
+  onError,
   level = 0,
 }: {
   instances: Instances;
   metas: Map<string, WsComponentMeta>;
   instanceSelector: InstanceSelector;
+  onError?: (message: string) => void;
   level?: number;
 }): boolean => {
   const [instanceId] = instanceSelector;
@@ -150,6 +186,7 @@ export const isTreeMatching = ({
     instances,
     instanceSelector,
     query: meta?.constraints,
+    onError,
   });
   // check ancestors only on the first run
   if (level === 0) {
@@ -164,6 +201,7 @@ export const isTreeMatching = ({
         instances,
         instanceSelector: instanceSelector.slice(index),
         query: meta?.constraints,
+        onError,
       });
       if (matches === false) {
         return false;
@@ -180,6 +218,7 @@ export const isTreeMatching = ({
         metas,
         instanceSelector: [child.value, ...instanceSelector],
         level: level + 1,
+        onError,
       });
       if (matches === false) {
         return false;
@@ -194,16 +233,19 @@ export const findClosestInstanceMatchingFragment = ({
   metas,
   instanceSelector,
   fragment,
+  onError,
 }: {
   instances: Instances;
   metas: Map<string, WsComponentMeta>;
   instanceSelector: InstanceSelector;
   fragment: WebstudioFragment;
+  onError?: (message: string) => void;
 }) => {
   const mergedInstances = new Map(instances);
   for (const instance of fragment.instances) {
     mergedInstances.set(instance.id, instance);
   }
+  let firstError = "";
   for (let index = 0; index < instanceSelector.length; index += 1) {
     const instanceId = instanceSelector[index];
     const instance = instances.get(instanceId);
@@ -222,13 +264,19 @@ export const findClosestInstanceMatchingFragment = ({
           instances: mergedInstances,
           metas,
           instanceSelector: [child.value, ...instanceSelector.slice(index)],
+          onError: (message) => {
+            if (firstError === "") {
+              firstError = message;
+            }
+          },
         });
       }
     }
     if (matches) {
       return index;
     }
   }
+  onError?.(firstError);
   return -1;
 };
 

diff --git a/fixtures/ssg/.webstudio/data.json b/fixtures/ssg/.webstudio/data.json
@@ -473,6 +473,20 @@
         "width": 14,
         "height": 16
       }
+    },
+    {
+      "id": "d0974db9300c1a3b0fb8b291dd9fabd45ad136478908394280af2f7087e3aecd",
+      "name": "147-1478573_cat-icon-png-black-cat-png-icon.png_ZJ6-qJjk1RlFzuYwyCXdp.jpeg",
+      "description": null,
+      "projectId": "d845c167-ea07-4875-b08d-83e97c09dcce",
+      "size": 64701,
+      "type": "image",
+      "format": "jpg",
+      "createdAt": "2024-12-06T14:36:07.046+00:00",
+      "meta": {
+        "width": 820,
+        "height": 985
+      }
     }
   ],
   "user": {

diff --git a/...sets/147-1478573_cat-icon-png-black-cat-png-icon.png_ZJ6-qJjk1RlFzuYwyCXdp.jpeg b/...sets/147-1478573_cat-icon-png-black-cat-png-icon.png_ZJ6-qJjk1RlFzuYwyCXdp.jpeg