Skip to content

fix: rebind variables after delete #4865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Feb 13, 2025
Merged

fix: rebind variables after delete #4865

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d9ee1ee
fix: rebind variables after delete
TrySound Feb 13, 2025
d88870d
Bump codemirror
TrySound Feb 13, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 16 additions & 16 deletions apps/builder/app/builder/features/settings-panel/resource-panel.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,8 +54,8 @@ import {
} from "~/builder/shared/code-editor-base";
import { parseCurl, type CurlRequest } from "./curl";
import {
$selectedInstance,
$selectedInstanceKey,
$selectedInstancePath,
$selectedPage,
} from "~/shared/awareness";
import { updateWebstudioData } from "~/shared/instance-utils";
Expand Down Expand Up @@ -583,11 +583,10 @@ export const ResourceForm = forwardRef<

useImperativeHandle(ref, () => ({
save: (formData) => {
const instancePath = $selectedInstancePath.get();
if (instancePath === undefined) {
const selectedInstance = $selectedInstance.get();
if (selectedInstance === undefined) {
return;
}
const [{ instance }] = instancePath;
const name = z.string().parse(formData.get("name"));
const newResource: Resource = {
id: resource?.id ?? nanoid(),
Expand All @@ -600,15 +599,16 @@ export const ResourceForm = forwardRef<
const newVariable: DataSource = {
id: variable?.id ?? nanoid(),
// preserve existing instance scope when edit
scopeInstanceId: variable?.scopeInstanceId ?? instance.id,
scopeInstanceId: variable?.scopeInstanceId ?? selectedInstance.id,
name,
type: "resource",
resourceId: newResource.id,
};
updateWebstudioData((data) => {
data.dataSources.set(newVariable.id, newVariable);
data.resources.set(newResource.id, newResource);
rebindTreeVariablesMutable({ instancePath, ...data });
const startingInstanceId = selectedInstance.id;
rebindTreeVariablesMutable({ startingInstanceId, ...data });
});
},
}));
Expand Down Expand Up @@ -715,11 +715,10 @@ export const SystemResourceForm = forwardRef<

useImperativeHandle(ref, () => ({
save: (formData) => {
const instancePath = $selectedInstancePath.get();
if (instancePath === undefined) {
const selectedInstance = $selectedInstance.get();
if (selectedInstance === undefined) {
return;
}
const [{ instance }] = instancePath;
const name = z.string().parse(formData.get("name"));
const newResource: Resource = {
id: resource?.id ?? nanoid(),
Expand All @@ -732,15 +731,16 @@ export const SystemResourceForm = forwardRef<
const newVariable: DataSource = {
id: variable?.id ?? nanoid(),
// preserve existing instance scope when edit
scopeInstanceId: variable?.scopeInstanceId ?? instance.id,
scopeInstanceId: variable?.scopeInstanceId ?? selectedInstance.id,
name,
type: "resource",
resourceId: newResource.id,
};
updateWebstudioData((data) => {
data.dataSources.set(newVariable.id, newVariable);
data.resources.set(newResource.id, newResource);
rebindTreeVariablesMutable({ instancePath, ...data });
const startingInstanceId = selectedInstance.id;
rebindTreeVariablesMutable({ startingInstanceId, ...data });
});
},
}));
Expand Down Expand Up @@ -824,11 +824,10 @@ export const GraphqlResourceForm = forwardRef<

useImperativeHandle(ref, () => ({
save: (formData) => {
const instancePath = $selectedInstancePath.get();
if (instancePath === undefined) {
const selectedInstance = $selectedInstance.get();
if (selectedInstance === undefined) {
return;
}
const [{ instance }] = instancePath;
const name = z.string().parse(formData.get("name"));
const body = generateObjectExpression(
new Map([
Expand All @@ -848,15 +847,16 @@ export const GraphqlResourceForm = forwardRef<
const newVariable: DataSource = {
id: variable?.id ?? nanoid(),
// preserve existing instance scope when edit
scopeInstanceId: variable?.scopeInstanceId ?? instance.id,
scopeInstanceId: variable?.scopeInstanceId ?? selectedInstance.id,
name,
type: "resource",
resourceId: newResource.id,
};
updateWebstudioData((data) => {
data.dataSources.set(newVariable.id, newVariable);
data.resources.set(newResource.id, newResource);
rebindTreeVariablesMutable({ instancePath, ...data });
const startingInstanceId = selectedInstance.id;
rebindTreeVariablesMutable({ startingInstanceId, ...data });
});
},
}));
Expand Down
25 changes: 13 additions & 12 deletions apps/builder/app/builder/features/settings-panel/variable-popover.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ import {
$instances,
$props,
} from "~/shared/nano-states";
import { $selectedInstance, $selectedInstancePath } from "~/shared/awareness";
import { $selectedInstance } from "~/shared/awareness";
import { BindingPopoverProvider } from "~/builder/shared/binding-popover";
import {
EditorDialog,
Expand Down Expand Up @@ -92,13 +92,13 @@ const $variablesByName = computed(
);

const $unsetVariableNames = computed(
[$selectedInstancePath, $instances, $props, $dataSources, $resources],
(instancePath, instances, props, dataSources, resources) => {
if (instancePath === undefined) {
[$selectedInstance, $instances, $props, $dataSources, $resources],
(selectedInstance, instances, props, dataSources, resources) => {
if (selectedInstance === undefined) {
return [];
}
return findUnsetVariableNames({
instancePath,
startingInstanceId: selectedInstance.id,
instances,
props,
dataSources,
Expand Down Expand Up @@ -288,8 +288,8 @@ const ParameterForm = forwardRef<
>(({ variable }, ref) => {
useImperativeHandle(ref, () => ({
save: (formData) => {
const instancePath = $selectedInstancePath.get();
if (instancePath === undefined) {
const selectedInstance = $selectedInstance.get();
if (selectedInstance === undefined) {
return;
}
// only existing parameter variables can be renamed
Expand All @@ -299,7 +299,8 @@ const ParameterForm = forwardRef<
const name = z.string().parse(formData.get("name"));
updateWebstudioData((data) => {
data.dataSources.set(variable.id, { ...variable, name });
rebindTreeVariablesMutable({ instancePath, ...data });
const startingInstanceId = selectedInstance.id;
rebindTreeVariablesMutable({ startingInstanceId, ...data });
});
},
}));
Expand All @@ -318,11 +319,10 @@ const useValuePanelRef = ({
}) => {
useImperativeHandle(ref, () => ({
save: (formData) => {
const instancePath = $selectedInstancePath.get();
if (instancePath === undefined) {
const selectedInstance = $selectedInstance.get();
if (selectedInstance === undefined) {
return;
}
const [{ instance: selectedInstance }] = instancePath;
const dataSourceId = variable?.id ?? nanoid();
// preserve existing instance scope when edit
const scopeInstanceId = variable?.scopeInstanceId ?? selectedInstance.id;
Expand All @@ -339,7 +339,8 @@ const useValuePanelRef = ({
type: "variable",
value: variableValue,
});
rebindTreeVariablesMutable({ instancePath, ...data });
const startingInstanceId = selectedInstance.id;
rebindTreeVariablesMutable({ startingInstanceId, ...data });
});
},
}));
Expand Down
95 changes: 70 additions & 25 deletions apps/builder/app/shared/data-variables.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@ import {
unsetExpressionVariables,
deleteVariableMutable,
} from "./data-variables";
import { getInstancePath } from "./awareness";

test("encode data variable name when necessary", () => {
expect(encodeDataVariableName("formState")).toEqual("formState");
Expand Down Expand Up @@ -153,10 +152,7 @@ test("find unset variable names", () => {
</$.Body>
);
expect(
findUnsetVariableNames({
instancePath: getInstancePath(["body"], data.instances),
...data,
})
findUnsetVariableNames({ startingInstanceId: "body", ...data })
).toEqual([
"one",
"two",
Expand All @@ -181,10 +177,7 @@ test("restore tree variables in children", () => {
</$.Box>
</$.Body>
);
rebindTreeVariablesMutable({
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
...data,
});
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
expect(Array.from(data.dataSources.values())).toEqual([
expect.objectContaining({ scopeInstanceId: "bodyId" }),
expect.objectContaining({ scopeInstanceId: "boxId" }),
Expand Down Expand Up @@ -212,10 +205,7 @@ test("restore tree variables in props", () => {
</$.Box>
</$.Body>
);
rebindTreeVariablesMutable({
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
...data,
});
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
const [_bodyVariableId, boxOneVariableId, boxTwoVariableId] =
data.dataSources.keys();
const boxOneIdentifier = encodeDataVariableId(boxOneVariableId);
Expand Down Expand Up @@ -262,10 +252,7 @@ test("rebind tree variables in props and children", () => {
</$.Box>
</$.Body>
);
rebindTreeVariablesMutable({
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
...data,
});
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
expect(Array.from(data.dataSources.values())).toEqual([
expect.objectContaining({ scopeInstanceId: "bodyId" }),
expect.objectContaining({ scopeInstanceId: "boxId" }),
Expand Down Expand Up @@ -312,10 +299,7 @@ test("restore tree variables in resources", () => {
</$.Box>
</$.Body>
);
rebindTreeVariablesMutable({
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
...data,
});
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
expect(Array.from(data.dataSources.values())).toEqual([
expect.objectContaining({ scopeInstanceId: "bodyId" }),
expect.objectContaining({ scopeInstanceId: "boxId" }),
Expand Down Expand Up @@ -365,10 +349,7 @@ test("rebind tree variables in resources", () => {
</$.Box>
</$.Body>
);
rebindTreeVariablesMutable({
instancePath: getInstancePath(["boxId", "bodyId"], data.instances),
...data,
});
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
expect(Array.from(data.dataSources.values())).toEqual([
expect.objectContaining({ scopeInstanceId: "bodyId" }),
expect.objectContaining({ scopeInstanceId: "boxId" }),
Expand All @@ -392,6 +373,23 @@ test("rebind tree variables in resources", () => {
]);
});

test("prevent rebinding tree variables from slots", () => {
const bodyVariable = new Variable("myVariable", "one value of body");
const data = renderData(
<$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}>
<$.Slot ws:id="slotId">
<$.Fragment ws:id="fragmentId">
<$.Box ws:id="boxId">{expression`myVariable`}</$.Box>
</$.Fragment>
</$.Slot>
</$.Body>
);
rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data });
expect(data.instances.get("boxId")?.children).toEqual([
{ type: "expression", value: "myVariable" },
]);
});

test("delete variable and unset it in expressions", () => {
const bodyVariable = new Variable("bodyVariable", "one value of body");
const data = renderData(
Expand Down Expand Up @@ -465,3 +463,50 @@ test("delete variable and unset it in resources", () => {
}),
]);
});

test("rebind expressions with parent variable when delete variable on child", () => {
const bodyVariable = new Variable("myVariable", "one value of body");
const boxVariable = new Variable("myVariable", "one value of body");
const data = renderData(
<$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}>
<$.Box ws:id="boxId" data-box-vars={expression`${boxVariable}`}>
<$.Text ws:id="textId">{expression`${boxVariable}`}</$.Text>
</$.Box>
</$.Body>
);
expect(Array.from(data.dataSources.values())).toEqual([
expect.objectContaining({ scopeInstanceId: "bodyId" }),
expect.objectContaining({ scopeInstanceId: "boxId" }),
]);
const [bodyVariableId, boxVariableId] = data.dataSources.keys();
deleteVariableMutable(data, boxVariableId);
const bodyIdentifier = encodeDataVariableId(bodyVariableId);
expect(data.instances.get("textId")?.children).toEqual([
{ type: "expression", value: bodyIdentifier },
]);
});

test("prevent rebinding with variables outside of slot content scope", () => {
const bodyVariable = new Variable("myVariable", "one value of body");
const boxVariable = new Variable("myVariable", "one value of body");
const data = renderData(
<$.Body ws:id="bodyId" data-body-vars={expression`${bodyVariable}`}>
<$.Slot>
<$.Fragment>
<$.Box ws:id="boxId" data-box-vars={expression`${boxVariable}`}>
<$.Text ws:id="textId">{expression`${boxVariable}`}</$.Text>
</$.Box>
</$.Fragment>
</$.Slot>
</$.Body>
);
expect(Array.from(data.dataSources.values())).toEqual([
expect.objectContaining({ scopeInstanceId: "bodyId" }),
expect.objectContaining({ scopeInstanceId: "boxId" }),
]);
const [_bodyVariableId, boxVariableId] = data.dataSources.keys();
deleteVariableMutable(data, boxVariableId);
expect(data.instances.get("textId")?.children).toEqual([
{ type: "expression", value: "myVariable" },
]);
});
Loading
Loading