Skip to content

Commit d9ee1ee

Browse files
TrySoundTrySound
committed
fix: rebind variables after delete
Ref #4768 Forgot to handle the case with rebinding with parent variables after variable on child is deleted. Also fixed the case with rebinding variables outside of slot scope.
1 parent ae864ef commit d9ee1ee

File tree

4 files changed

+148
-71
lines changed

4 files changed

+148
-71
lines changed

‎apps/builder/app/builder/features/settings-panel/resource-panel.tsx

+16-16
Original file line numberDiff line numberDiff line change
@@ -54,8 +54,8 @@ import {
5454
} from "~/builder/shared/code-editor-base";
5555
import { parseCurl, type CurlRequest } from "./curl";
5656
import {
57+
$selectedInstance,
5758
$selectedInstanceKey,
58-
$selectedInstancePath,
5959
$selectedPage,
6060
} from "~/shared/awareness";
6161
import { updateWebstudioData } from "~/shared/instance-utils";
@@ -583,11 +583,10 @@ export const ResourceForm = forwardRef<
583583

584584
useImperativeHandle(ref, () => ({
585585
save: (formData) => {
586-
const instancePath = $selectedInstancePath.get();
587-
if (instancePath === undefined) {
586+
const selectedInstance = $selectedInstance.get();
587+
if (selectedInstance === undefined) {
588588
return;
589589
}
590-
const [{ instance }] = instancePath;
591590
const name = z.string().parse(formData.get("name"));
592591
const newResource: Resource = {
593592
id: resource?.id ?? nanoid(),
@@ -600,15 +599,16 @@ export const ResourceForm = forwardRef<
600599
const newVariable: DataSource = {
601600
id: variable?.id ?? nanoid(),
602601
// preserve existing instance scope when edit
603-
scopeInstanceId: variable?.scopeInstanceId ?? instance.id,
602+
scopeInstanceId: variable?.scopeInstanceId ?? selectedInstance.id,
604603
name,
605604
type: "resource",
606605
resourceId: newResource.id,
607606
};
608607
updateWebstudioData((data) => {
609608
data.dataSources.set(newVariable.id, newVariable);
610609
data.resources.set(newResource.id, newResource);
611-
rebindTreeVariablesMutable({ instancePath, ...data });
610+
const startingInstanceId = selectedInstance.id;
611+
rebindTreeVariablesMutable({ startingInstanceId, ...data });
612612
});
613613
},
614614
}));
@@ -715,11 +715,10 @@ export const SystemResourceForm = forwardRef<
715715

716716
useImperativeHandle(ref, () => ({
717717
save: (formData) => {
718-
const instancePath = $selectedInstancePath.get();
719-
if (instancePath === undefined) {
718+
const selectedInstance = $selectedInstance.get();
719+
if (selectedInstance === undefined) {
720720
return;
721721
}
722-
const [{ instance }] = instancePath;
723722
const name = z.string().parse(formData.get("name"));
724723
const newResource: Resource = {
725724
id: resource?.id ?? nanoid(),
@@ -732,15 +731,16 @@ export const SystemResourceForm = forwardRef<
732731
const newVariable: DataSource = {
733732
id: variable?.id ?? nanoid(),
734733
// preserve existing instance scope when edit
735-
scopeInstanceId: variable?.scopeInstanceId ?? instance.id,
734+
scopeInstanceId: variable?.scopeInstanceId ?? selectedInstance.id,
736735
name,
737736
type: "resource",
738737
resourceId: newResource.id,
739738
};
740739
updateWebstudioData((data) => {
741740
data.dataSources.set(newVariable.id, newVariable);
742741
data.resources.set(newResource.id, newResource);
743-
rebindTreeVariablesMutable({ instancePath, ...data });
742+
const startingInstanceId = selectedInstance.id;
743+
rebindTreeVariablesMutable({ startingInstanceId, ...data });
744744
});
745745
},
746746
}));
@@ -824,11 +824,10 @@ export const GraphqlResourceForm = forwardRef<
824824

825825
useImperativeHandle(ref, () => ({
826826
save: (formData) => {
827-
const instancePath = $selectedInstancePath.get();
828-
if (instancePath === undefined) {
827+
const selectedInstance = $selectedInstance.get();
828+
if (selectedInstance === undefined) {
829829
return;
830830
}
831-
const [{ instance }] = instancePath;
832831
const name = z.string().parse(formData.get("name"));
833832
const body = generateObjectExpression(
834833
new Map([
@@ -848,15 +847,16 @@ export const GraphqlResourceForm = forwardRef<
848847
const newVariable: DataSource = {
849848
id: variable?.id ?? nanoid(),
850849
// preserve existing instance scope when edit
851-
scopeInstanceId: variable?.scopeInstanceId ?? instance.id,
850+
scopeInstanceId: variable?.scopeInstanceId ?? selectedInstance.id,
852851
name,
853852
type: "resource",
854853
resourceId: newResource.id,
855854
};
856855
updateWebstudioData((data) => {
857856
data.dataSources.set(newVariable.id, newVariable);
858857
data.resources.set(newResource.id, newResource);
859-
rebindTreeVariablesMutable({ instancePath, ...data });
858+
const startingInstanceId = selectedInstance.id;
859+
rebindTreeVariablesMutable({ startingInstanceId, ...data });
860860
});
861861
},
862862
}));

‎apps/builder/app/builder/features/settings-panel/variable-popover.tsx

+13-12
Original file line numberDiff line numberDiff line change
@@ -59,7 +59,7 @@ import {
5959
$instances,
6060
$props,
6161
} from "~/shared/nano-states";
62-
import { $selectedInstance, $selectedInstancePath } from "~/shared/awareness";
62+
import { $selectedInstance } from "~/shared/awareness";
6363
import { BindingPopoverProvider } from "~/builder/shared/binding-popover";
6464
import {
6565
EditorDialog,
@@ -92,13 +92,13 @@ const $variablesByName = computed(
9292
);
9393

9494
const $unsetVariableNames = computed(
95-
[$selectedInstancePath, $instances, $props, $dataSources, $resources],
96-
(instancePath, instances, props, dataSources, resources) => {
97-
if (instancePath === undefined) {
95+
[$selectedInstance, $instances, $props, $dataSources, $resources],
96+
(selectedInstance, instances, props, dataSources, resources) => {
97+
if (selectedInstance === undefined) {
9898
return [];
9999
}
100100
return findUnsetVariableNames({
101-
instancePath,
101+
startingInstanceId: selectedInstance.id,
102102
instances,
103103
props,
104104
dataSources,
@@ -288,8 +288,8 @@ const ParameterForm = forwardRef<
288288
>(({ variable }, ref) => {
289289
useImperativeHandle(ref, () => ({
290290
save: (formData) => {
291-
const instancePath = $selectedInstancePath.get();
292-
if (instancePath === undefined) {
291+
const selectedInstance = $selectedInstance.get();
292+
if (selectedInstance === undefined) {
293293
return;
294294
}
295295
// only existing parameter variables can be renamed
@@ -299,7 +299,8 @@ const ParameterForm = forwardRef<
299299
const name = z.string().parse(formData.get("name"));
300300
updateWebstudioData((data) => {
301301
data.dataSources.set(variable.id, { ...variable, name });
302-
rebindTreeVariablesMutable({ instancePath, ...data });
302+
const startingInstanceId = selectedInstance.id;
303+
rebindTreeVariablesMutable({ startingInstanceId, ...data });
303304
});
304305
},
305306
}));
@@ -318,11 +319,10 @@ const useValuePanelRef = ({
318319
}) => {
319320
useImperativeHandle(ref, () => ({
320321
save: (formData) => {
321-
const instancePath = $selectedInstancePath.get();
322-
if (instancePath === undefined) {
322+
const selectedInstance = $selectedInstance.get();
323+
if (selectedInstance === undefined) {
323324
return;
324325
}
325-
const [{ instance: selectedInstance }] = instancePath;
326326
const dataSourceId = variable?.id ?? nanoid();
327327
// preserve existing instance scope when edit
328328
const scopeInstanceId = variable?.scopeInstanceId ?? selectedInstance.id;
@@ -339,7 +339,8 @@ const useValuePanelRef = ({
339339
type: "variable",
340340
value: variableValue,
341341
});
342-
rebindTreeVariablesMutable({ instancePath, ...data });
342+
const startingInstanceId = selectedInstance.id;
343+
rebindTreeVariablesMutable({ startingInstanceId, ...data });
343344
});
344345
},
345346
}));

‎apps/builder/app/shared/data-variables.test.tsx

+70-25
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,6 @@ import {
1818
unsetExpressionVariables,
1919
deleteVariableMutable,
2020
} from "./data-variables";
21-
import { getInstancePath } from "./awareness";
2221

2322
test("encode data variable name when necessary", () => {
2423
expect(encodeDataVariableName("formState")).toEqual("formState");
@@ -153,10 +152,7 @@ test("find unset variable names", () => {
153152
</$.Body>
154153
);
155154
expect(
156-
findUnsetVariableNames({
157-
instancePath: getInstancePath(["body"], data.instances),
158-
...data,
159-
})
155+
findUnsetVariableNames({ startingInstanceId: "body", ...data })
160156
).toEqual([
161157
"one",
162158
"two",
@@ -181,10 +177,7 @@ test("restore tree variables in children", () => {
181177
</$.Box>
182178
</$.Body>
183179
);
184-
rebindTreeVariablesMutable({
185-
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
186-
...data,
187-
});
180+
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
188181
expect(Array.from(data.dataSources.values())).toEqual([
189182
expect.objectContaining({ scopeInstanceId: "bodyId" }),
190183
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -212,10 +205,7 @@ test("restore tree variables in props", () => {
212205
</$.Box>
213206
</$.Body>
214207
);
215-
rebindTreeVariablesMutable({
216-
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
217-
...data,
218-
});
208+
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
219209
const [_bodyVariableId, boxOneVariableId, boxTwoVariableId] =
220210
data.dataSources.keys();
221211
const boxOneIdentifier = encodeDataVariableId(boxOneVariableId);
@@ -262,10 +252,7 @@ test("rebind tree variables in props and children", () => {
262252
</$.Box>
263253
</$.Body>
264254
);
265-
rebindTreeVariablesMutable({
266-
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
267-
...data,
268-
});
255+
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
269256
expect(Array.from(data.dataSources.values())).toEqual([
270257
expect.objectContaining({ scopeInstanceId: "bodyId" }),
271258
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -312,10 +299,7 @@ test("restore tree variables in resources", () => {
312299
</$.Box>
313300
</$.Body>
314301
);
315-
rebindTreeVariablesMutable({
316-
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
317-
...data,
318-
});
302+
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
319303
expect(Array.from(data.dataSources.values())).toEqual([
320304
expect.objectContaining({ scopeInstanceId: "bodyId" }),
321305
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -365,10 +349,7 @@ test("rebind tree variables in resources", () => {
365349
</$.Box>
366350
</$.Body>
367351
);
368-
rebindTreeVariablesMutable({
369-
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
370-
...data,
371-
});
352+
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
372353
expect(Array.from(data.dataSources.values())).toEqual([
373354
expect.objectContaining({ scopeInstanceId: "bodyId" }),
374355
expect.objectContaining({ scopeInstanceId: "boxId" }),
@@ -392,6 +373,23 @@ test("rebind tree variables in resources", () => {
392373
]);
393374
});
394375

376+
test("prevent rebinding tree variables from slots", () => {
377+
const bodyVariable = new Variable("myVariable", "one value of body");
378+
const data = renderData(
379+
<$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}>
380+
<$.Slot ws:id="slotId">
381+
<$.Fragment ws:id="fragmentId">
382+
<$.Box ws:id="boxId">{expression`myVariable`}</$.Box>
383+
</$.Fragment>
384+
</$.Slot>
385+
</$.Body>
386+
);
387+
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
388+
expect(data.instances.get("boxId")?.children).toEqual([
389+
{ type: "expression", value: "myVariable" },
390+
]);
391+
});
392+
395393
test("delete variable and unset it in expressions", () => {
396394
const bodyVariable = new Variable("bodyVariable", "one value of body");
397395
const data = renderData(
@@ -465,3 +463,50 @@ test("delete variable and unset it in resources", () => {
465463
}),
466464
]);
467465
});
466+
467+
test("rebind expressions with parent variable when delete variable on child", () => {
468+
const bodyVariable = new Variable("myVariable", "one value of body");
469+
const boxVariable = new Variable("myVariable", "one value of body");
470+
const data = renderData(
471+
<$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}>
472+
<$.Box ws:id="boxId" data-box-vars={expression`${boxVariable}`}>
473+
<$.Text ws:id="textId">{expression`${boxVariable}`}</$.Text>
474+
</$.Box>
475+
</$.Body>
476+
);
477+
expect(Array.from(data.dataSources.values())).toEqual([
478+
expect.objectContaining({ scopeInstanceId: "bodyId" }),
479+
expect.objectContaining({ scopeInstanceId: "boxId" }),
480+
]);
481+
const [bodyVariableId, boxVariableId] = data.dataSources.keys();
482+
deleteVariableMutable(data, boxVariableId);
483+
const bodyIdentifier = encodeDataVariableId(bodyVariableId);
484+
expect(data.instances.get("textId")?.children).toEqual([
485+
{ type: "expression", value: bodyIdentifier },
486+
]);
487+
});
488+
489+
test("prevent rebinding with variables outside of slot content scope", () => {
490+
const bodyVariable = new Variable("myVariable", "one value of body");
491+
const boxVariable = new Variable("myVariable", "one value of body");
492+
const data = renderData(
493+
<$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}>
494+
<$.Slot>
495+
<$.Fragment>
496+
<$.Box ws:id="boxId" data-box-vars={expression`${boxVariable}`}>
497+
<$.Text ws:id="textId">{expression`${boxVariable}`}</$.Text>
498+
</$.Box>
499+
</$.Fragment>
500+
</$.Slot>
501+
</$.Body>
502+
);
503+
expect(Array.from(data.dataSources.values())).toEqual([
504+
expect.objectContaining({ scopeInstanceId: "bodyId" }),
505+
expect.objectContaining({ scopeInstanceId: "boxId" }),
506+
]);
507+
const [_bodyVariableId, boxVariableId] = data.dataSources.keys();
508+
deleteVariableMutable(data, boxVariableId);
509+
expect(data.instances.get("textId")?.children).toEqual([
510+
{ type: "expression", value: "myVariable" },
511+
]);
512+
});

0 commit comments

Comments
 (0)