Skip to content

Commit fb2c6b5

Browse files
authored
refactor: improve constraints errors (#4535)
Now errors are toasted when insert or paste anything. Legacy constraints are removed from components.
1 parent 0dbdbe6 commit fb2c6b5

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

41 files changed

+220
-110
lines changed

.github/workflows/main.yml

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -83,8 +83,8 @@ jobs:
8383
}
8484
}
8585
const results = [
86-
await assertSize('./fixtures/ssg/dist/client', 288),
87-
await assertSize('./fixtures/webstudio-remix-netlify-functions/build/client', 376),
86+
await assertSize('./fixtures/ssg/dist/client', 352),
87+
await assertSize('./fixtures/webstudio-remix-netlify-functions/build/client', 440),
8888
await assertSize('./fixtures/webstudio-remix-vercel/build/client', 908),
8989
]
9090
for (const result of results) {

apps/builder/app/builder/features/command-panel/command-panel.tsx

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,14 @@ import {
3535
$selectedBreakpoint,
3636
$selectedBreakpointId,
3737
} from "~/shared/nano-states";
38-
import { getInstanceLabel } from "~/shared/instance-utils";
38+
import {
39+
findClosestInsertable,
40+
getComponentTemplateData,
41+
getInstanceLabel,
42+
insertTemplateData,
43+
} from "~/shared/instance-utils";
3944
import { humanizeString } from "~/shared/string-utils";
4045
import { setCanvasWidth } from "~/builder/features/breakpoints";
41-
import { insert as insertComponent } from "~/builder/features/components/insert";
4246
import { $selectedPage, selectPage } from "~/shared/awareness";
4347
import { mapGroupBy } from "~/shared/shim";
4448
import { setActiveSidebarPanel } from "~/builder/shared/nano-states";
@@ -146,7 +150,13 @@ const ComponentOptionsGroup = ({ options }: { options: ComponentOption[] }) => {
146150
value={component}
147151
onSelect={() => {
148152
closeCommandPanel();
149-
insertComponent(component);
153+
const fragment = getComponentTemplateData(component);
154+
if (fragment) {
155+
const insertable = findClosestInsertable(fragment);
156+
if (insertable) {
157+
insertTemplateData(fragment, insertable);
158+
}
159+
}
150160
}}
151161
>
152162
<Flex gap={2}>

apps/builder/app/builder/features/components/components.tsx

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,9 +34,13 @@ import {
3434
type MetaByCategory,
3535
type ComponentNamesByMeta,
3636
} from "./get-meta-maps";
37-
import { getInstanceLabel } from "~/shared/instance-utils";
37+
import {
38+
findClosestInsertable,
39+
getComponentTemplateData,
40+
getInstanceLabel,
41+
insertTemplateData,
42+
} from "~/shared/instance-utils";
3843
import { isFeatureEnabled } from "@webstudio-is/feature-flags";
39-
import { insert } from "./insert";
4044
import { matchSorter } from "match-sorter";
4145
import { parseComponentName } from "@webstudio-is/sdk";
4246
import type { Publish } from "~/shared/pubsub";
@@ -183,7 +187,13 @@ export const ComponentsPanel = ({
183187

184188
const handleInsert = (component: string) => {
185189
onClose();
186-
insert(component);
190+
const fragment = getComponentTemplateData(component);
191+
if (fragment) {
192+
const insertable = findClosestInsertable(fragment);
193+
if (insertable) {
194+
insertTemplateData(fragment, insertable);
195+
}
196+
}
187197
};
188198

189199
const resetSelectedComponent = () => {

apps/builder/app/builder/features/components/insert.ts

Lines changed: 0 additions & 46 deletions
This file was deleted.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1459,6 +1459,7 @@ export const findClosestInsertable = (
14591459
instances,
14601460
instanceSelector: instanceSelector.slice(closestContainerIndex),
14611461
fragment,
1462+
onError: (message) => toast.error(message),
14621463
});
14631464
if (insertableIndex === -1) {
14641465
return;

apps/builder/app/shared/matcher.test.tsx

Lines changed: 86 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
import { describe, expect, test } from "vitest";
1+
import { describe, expect, test, vi } from "vitest";
22
import { renderJsx, $, ExpressionValue } from "@webstudio-is/sdk/testing";
33
import { coreMetas } from "@webstudio-is/react-sdk";
44
import * as baseMetas from "@webstudio-is/sdk-components-react/metas";
@@ -563,6 +563,72 @@ describe("is instance matching", () => {
563563
})
564564
).toBeFalsy();
565565
});
566+
567+
test("provide error message when negated matcher is failed", () => {
568+
const onError = vi.fn();
569+
isInstanceMatching({
570+
...renderJsx(
571+
<$.Body ws:id="body">
572+
<$.Box ws:id="box"></$.Box>
573+
</$.Body>
574+
),
575+
instanceSelector: ["box", "body"],
576+
query: {
577+
relation: "self",
578+
component: { $nin: ["Box", "Text"] },
579+
},
580+
onError,
581+
});
582+
expect(onError).toHaveBeenLastCalledWith("Box or Text is not acceptable");
583+
isInstanceMatching({
584+
...renderJsx(
585+
<$.Body ws:id="body">
586+
<$.Box ws:id="box">
587+
<$.ListItem ws:id="listitem"></$.ListItem>
588+
</$.Box>
589+
</$.Body>
590+
),
591+
instanceSelector: ["listitem", "box", "body"],
592+
query: {
593+
relation: "ancestor",
594+
component: { $nin: ["Box", "Text"] },
595+
},
596+
onError,
597+
});
598+
expect(onError).toHaveBeenLastCalledWith("Box or Text is not acceptable");
599+
});
600+
601+
test("provide error message when positive matcher is failed", () => {
602+
const onError = vi.fn();
603+
isInstanceMatching({
604+
...renderJsx(
605+
<$.Body ws:id="body">
606+
<$.ListItem ws:id="listitem"></$.ListItem>
607+
</$.Body>
608+
),
609+
instanceSelector: ["box", "body"],
610+
query: {
611+
relation: "self",
612+
component: { $in: ["Box", "Text"] },
613+
},
614+
onError,
615+
});
616+
expect(onError).toHaveBeenLastCalledWith("Box or Text is missing");
617+
isInstanceMatching({
618+
...renderJsx(
619+
<$.Body ws:id="body">
620+
<$.ListItem ws:id="listitem"></$.ListItem>
621+
</$.Body>
622+
),
623+
instanceSelector: ["box", "body"],
624+
query: {
625+
relation: "ancestor",
626+
component: { $in: ["Box", "Text"] },
627+
},
628+
onError,
629+
});
630+
expect(onError).toHaveBeenLastCalledWith("Box or Text is missing");
631+
});
566632
});
567633

568634
describe("is tree matching", () => {
@@ -781,6 +847,25 @@ describe("find closest instance matching fragment", () => {
781847
})
782848
).toEqual(1);
783849
});
850+
851+
test("report first error", () => {
852+
const onError = vi.fn();
853+
const { instances } = renderJsx(<$.Body ws:id="body"></$.Body>);
854+
const fragment = createFragment(
855+
// only children are tested
856+
<>
857+
<$.ListItem ws:id="listitem"></$.ListItem>
858+
</>
859+
);
860+
findClosestInstanceMatchingFragment({
861+
metas,
862+
instances,
863+
instanceSelector: ["body"],
864+
fragment,
865+
onError,
866+
});
867+
expect(onError).toHaveBeenLastCalledWith("List is missing");
868+
});
784869
});
785870

786871
describe("find closest container", () => {

apps/builder/app/shared/matcher.ts

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,26 @@ const isLevelMatchingRelation = (level: number, relation: MatcherRelation) => {
4848
);
4949
};
5050

51+
const formatList = (operation: MatcherOperation) => {
52+
const listFormat = new Intl.ListFormat("en", {
53+
type: "disjunction",
54+
});
55+
const list: string[] = [];
56+
if (operation.$eq) {
57+
list.push(operation.$eq);
58+
}
59+
if (operation.$in) {
60+
list.push(...operation.$in);
61+
}
62+
if (operation.$neq) {
63+
list.push(operation.$neq);
64+
}
65+
if (operation.$nin) {
66+
list.push(...operation.$nin);
67+
}
68+
return listFormat.format(list);
69+
};
70+
5171
/**
5272
* @todo following features are missing
5373
* - tag matcher
@@ -56,10 +76,12 @@ export const isInstanceMatching = ({
5676
instances,
5777
instanceSelector,
5878
query,
79+
onError,
5980
}: {
6081
instances: Instances;
6182
instanceSelector: InstanceSelector;
6283
query: undefined | Matcher | Matcher[];
84+
onError?: (message: string) => void;
6385
}): boolean => {
6486
// fast path to the lack of constraints
6587
if (query === undefined) {
@@ -73,6 +95,9 @@ export const isInstanceMatching = ({
7395
if (isNegated(matcher.component)) {
7496
if (matches) {
7597
aborted = true;
98+
if (matcher.component) {
99+
onError?.(`${formatList(matcher.component)} is not acceptable`);
100+
}
76101
return;
77102
}
78103
// inverse negated match
@@ -124,18 +149,29 @@ export const isInstanceMatching = ({
124149
if (aborted) {
125150
return false;
126151
}
127-
return queries.every((matcher) => matchesByMatcher.get(matcher));
152+
for (const matcher of queries) {
153+
const matches = matchesByMatcher.get(matcher) ?? false;
154+
if (matches === false) {
155+
if (matcher.component) {
156+
onError?.(`${formatList(matcher.component)} is missing`);
157+
}
158+
return false;
159+
}
160+
}
161+
return true;
128162
};
129163

130164
export const isTreeMatching = ({
131165
instances,
132166
metas,
133167
instanceSelector,
168+
onError,
134169
level = 0,
135170
}: {
136171
instances: Instances;
137172
metas: Map<string, WsComponentMeta>;
138173
instanceSelector: InstanceSelector;
174+
onError?: (message: string) => void;
139175
level?: number;
140176
}): boolean => {
141177
const [instanceId] = instanceSelector;
@@ -150,6 +186,7 @@ export const isTreeMatching = ({
150186
instances,
151187
instanceSelector,
152188
query: meta?.constraints,
189+
onError,
153190
});
154191
// check ancestors only on the first run
155192
if (level === 0) {
@@ -164,6 +201,7 @@ export const isTreeMatching = ({
164201
instances,
165202
instanceSelector: instanceSelector.slice(index),
166203
query: meta?.constraints,
204+
onError,
167205
});
168206
if (matches === false) {
169207
return false;
@@ -180,6 +218,7 @@ export const isTreeMatching = ({
180218
metas,
181219
instanceSelector: [child.value, ...instanceSelector],
182220
level: level + 1,
221+
onError,
183222
});
184223
if (matches === false) {
185224
return false;
@@ -194,16 +233,19 @@ export const findClosestInstanceMatchingFragment = ({
194233
metas,
195234
instanceSelector,
196235
fragment,
236+
onError,
197237
}: {
198238
instances: Instances;
199239
metas: Map<string, WsComponentMeta>;
200240
instanceSelector: InstanceSelector;
201241
fragment: WebstudioFragment;
242+
onError?: (message: string) => void;
202243
}) => {
203244
const mergedInstances = new Map(instances);
204245
for (const instance of fragment.instances) {
205246
mergedInstances.set(instance.id, instance);
206247
}
248+
let firstError = "";
207249
for (let index = 0; index < instanceSelector.length; index += 1) {
208250
const instanceId = instanceSelector[index];
209251
const instance = instances.get(instanceId);
@@ -222,13 +264,19 @@ export const findClosestInstanceMatchingFragment = ({
222264
instances: mergedInstances,
223265
metas,
224266
instanceSelector: [child.value, ...instanceSelector.slice(index)],
267+
onError: (message) => {
268+
if (firstError === "") {
269+
firstError = message;
270+
}
271+
},
225272
});
226273
}
227274
}
228275
if (matches) {
229276
return index;
230277
}
231278
}
279+
onError?.(firstError);
232280
return -1;
233281
};
234282

fixtures/ssg/.webstudio/data.json

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -473,6 +473,20 @@
473473
"width": 14,
474474
"height": 16
475475
}
476+
},
477+
{
478+
"id": "d0974db9300c1a3b0fb8b291dd9fabd45ad136478908394280af2f7087e3aecd",
479+
"name": "147-1478573_cat-icon-png-black-cat-png-icon.png_ZJ6-qJjk1RlFzuYwyCXdp.jpeg",
480+
"description": null,
481+
"projectId": "d845c167-ea07-4875-b08d-83e97c09dcce",
482+
"size": 64701,
483+
"type": "image",
484+
"format": "jpg",
485+
"createdAt": "2024-12-06T14:36:07.046+00:00",
486+
"meta": {
487+
"width": 820,
488+
"height": 985
489+
}
476490
}
477491
],
478492
"user": {
63.2 KB
Loading

0 commit comments

Comments
 (0)