From d9ee1ee9dc580e39c526c4e3bfafd24e4188c969 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Thu, 13 Feb 2025 12:06:18 +0800 Subject: [PATCH 1/2] fix: rebind variables after delete Ref https://github.com/webstudio-is/webstudio/issues/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. --- .../settings-panel/resource-panel.tsx | 32 +++---- .../settings-panel/variable-popover.tsx | 25 ++--- .../app/shared/data-variables.test.tsx | 95 ++++++++++++++----- apps/builder/app/shared/data-variables.ts | 67 +++++++++---- 4 files changed, 148 insertions(+), 71 deletions(-) diff --git a/apps/builder/app/builder/features/settings-panel/resource-panel.tsx b/apps/builder/app/builder/features/settings-panel/resource-panel.tsx index 5f1a5de8679d..6ac01b769c1b 100644 --- a/apps/builder/app/builder/features/settings-panel/resource-panel.tsx +++ b/apps/builder/app/builder/features/settings-panel/resource-panel.tsx @@ -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"; @@ -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(), @@ -600,7 +599,7 @@ 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, @@ -608,7 +607,8 @@ export const ResourceForm = forwardRef< updateWebstudioData((data) => { data.dataSources.set(newVariable.id, newVariable); data.resources.set(newResource.id, newResource); - rebindTreeVariablesMutable({ instancePath, ...data }); + const startingInstanceId = selectedInstance.id; + rebindTreeVariablesMutable({ startingInstanceId, ...data }); }); }, })); @@ -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(), @@ -732,7 +731,7 @@ 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, @@ -740,7 +739,8 @@ export const SystemResourceForm = forwardRef< updateWebstudioData((data) => { data.dataSources.set(newVariable.id, newVariable); data.resources.set(newResource.id, newResource); - rebindTreeVariablesMutable({ instancePath, ...data }); + const startingInstanceId = selectedInstance.id; + rebindTreeVariablesMutable({ startingInstanceId, ...data }); }); }, })); @@ -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([ @@ -848,7 +847,7 @@ 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, @@ -856,7 +855,8 @@ export const GraphqlResourceForm = forwardRef< updateWebstudioData((data) => { data.dataSources.set(newVariable.id, newVariable); data.resources.set(newResource.id, newResource); - rebindTreeVariablesMutable({ instancePath, ...data }); + const startingInstanceId = selectedInstance.id; + rebindTreeVariablesMutable({ startingInstanceId, ...data }); }); }, })); diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx index 15bce59ddf25..f23f26cfc25b 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -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, @@ -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, @@ -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 @@ -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 }); }); }, })); @@ -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; @@ -339,7 +339,8 @@ const useValuePanelRef = ({ type: "variable", value: variableValue, }); - rebindTreeVariablesMutable({ instancePath, ...data }); + const startingInstanceId = selectedInstance.id; + rebindTreeVariablesMutable({ startingInstanceId, ...data }); }); }, })); diff --git a/apps/builder/app/shared/data-variables.test.tsx b/apps/builder/app/shared/data-variables.test.tsx index b314641d38a7..672527af1318 100644 --- a/apps/builder/app/shared/data-variables.test.tsx +++ b/apps/builder/app/shared/data-variables.test.tsx @@ -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"); @@ -153,10 +152,7 @@ test("find unset variable names", () => { ); expect( - findUnsetVariableNames({ - instancePath: getInstancePath(["body"], data.instances), - ...data, - }) + findUnsetVariableNames({ startingInstanceId: "body", ...data }) ).toEqual([ "one", "two", @@ -181,10 +177,7 @@ test("restore tree variables in children", () => { ); - 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" }), @@ -212,10 +205,7 @@ test("restore tree variables in props", () => { ); - rebindTreeVariablesMutable({ - instancePath: getInstancePath(["boxId", "bodyId"], data.instances), - ...data, - }); + rebindTreeVariablesMutable({ startingInstanceId: "boxId", ...data }); const [_bodyVariableId, boxOneVariableId, boxTwoVariableId] = data.dataSources.keys(); const boxOneIdentifier = encodeDataVariableId(boxOneVariableId); @@ -262,10 +252,7 @@ test("rebind tree variables in props and children", () => { ); - 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" }), @@ -312,10 +299,7 @@ test("restore tree variables in resources", () => { ); - 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" }), @@ -365,10 +349,7 @@ test("rebind tree variables in resources", () => { ); - 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" }), @@ -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`} + + + + ); + 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( @@ -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}`} + + + ); + 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}`} + + + + + ); + 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" }, + ]); +}); diff --git a/apps/builder/app/shared/data-variables.ts b/apps/builder/app/shared/data-variables.ts index 7d3f275a35f9..0d2c9d0052fa 100644 --- a/apps/builder/app/shared/data-variables.ts +++ b/apps/builder/app/shared/data-variables.ts @@ -1,10 +1,10 @@ import { type DataSource, type DataSources, - Instance, + type Instance, type Instances, type Props, - Resource, + type Resource, type Resources, type WebstudioData, decodeDataVariableId, @@ -16,7 +16,6 @@ import { createJsonStringifyProxy, isPlainObject, } from "@webstudio-is/sdk/runtime"; -import type { InstancePath } from "./awareness"; const allowedJsChars = /[A-Za-z_]/; @@ -183,19 +182,39 @@ export const computeExpression = ( } }; -export const findMaskedVariables = ({ - instancePath, +const findMaskedVariablesByInstanceId = ({ + startingInstanceId, + instances, dataSources, }: { - instancePath: InstancePath; + startingInstanceId: Instance["id"]; + instances: Instances; dataSources: DataSources; }) => { + const parentInstanceById = new Map(); + for (const instance of instances.values()) { + // interrupt lookup because slot variables cannot be passed to slot content + if (instance.component === "Slot") { + continue; + } + for (const child of instance.children) { + if (child.type === "id") { + parentInstanceById.set(child.value, instance.id); + } + } + } + let currentId: undefined | string = startingInstanceId; + const instanceIdsPath: Instance["id"][] = []; + while (currentId) { + instanceIdsPath.push(currentId); + currentId = parentInstanceById.get(currentId); + } const maskedVariables = new Map(); // start from the root to descendant // so child variables override parent variables - for (const { instance } of instancePath.slice().reverse()) { + for (const instanceId of instanceIdsPath.reverse()) { for (const dataSource of dataSources.values()) { - if (dataSource.scopeInstanceId === instance.id) { + if (dataSource.scopeInstanceId === instanceId) { maskedVariables.set(dataSource.name, dataSource.id); } } @@ -297,22 +316,21 @@ const traverseExpressions = ({ }; export const findUnsetVariableNames = ({ - instancePath, + startingInstanceId, instances, props, dataSources, resources, }: { - instancePath: InstancePath; + startingInstanceId: Instance["id"]; instances: Instances; props: Props; dataSources: DataSources; resources: Resources; }) => { const unsetVariables = new Set(); - const [{ instance: startingInstance }] = instancePath; traverseExpressions({ - startingInstanceId: startingInstance.id, + startingInstanceId: startingInstanceId, instances, props, dataSources, @@ -333,26 +351,29 @@ export const findUnsetVariableNames = ({ }; export const rebindTreeVariablesMutable = ({ - instancePath, + startingInstanceId, instances, props, dataSources, resources, }: { - instancePath: InstancePath; + startingInstanceId: Instance["id"]; instances: Instances; props: Props; dataSources: DataSources; resources: Resources; }) => { - const maskedVariables = findMaskedVariables({ instancePath, dataSources }); + const maskedVariables = findMaskedVariablesByInstanceId({ + startingInstanceId, + dataSources, + instances, + }); const unsetNameById = new Map(); for (const { id, name } of dataSources.values()) { unsetNameById.set(id, name); } - const [{ instance: startingInstance }] = instancePath; traverseExpressions({ - startingInstanceId: startingInstance.id, + startingInstanceId, instances, props, dataSources, @@ -392,15 +413,25 @@ export const deleteVariableMutable = ( } const unsetNameById = new Map(); unsetNameById.set(dataSource.id, dataSource.name); + const maskedIdByName = findMaskedVariablesByInstanceId({ + startingInstanceId: dataSource.scopeInstanceId, + instances: data.instances, + dataSources: data.dataSources, + }); // unset deleted variable in expressions traverseExpressions({ ...data, startingInstanceId: dataSource.scopeInstanceId, update: (expression) => { - return unsetExpressionVariables({ + expression = unsetExpressionVariables({ expression, unsetNameById, }); + expression = restoreExpressionVariables({ + expression, + maskedIdByName, + }); + return expression; }, }); }; From d88870dce83af4683e6f8a2e9adc3b4ea3c819a0 Mon Sep 17 00:00:00 2001 From: Bogdan Chadkin Date: Thu, 13 Feb 2025 17:04:25 +0800 Subject: [PATCH 2/2] Bump codemirror --- apps/builder/package.json | 4 ++-- pnpm-lock.yaml | 30 +++++++++++++++--------------- 2 files changed, 17 insertions(+), 17 deletions(-) diff --git a/apps/builder/package.json b/apps/builder/package.json index b6639edcd376..03bb155c1c24 100644 --- a/apps/builder/package.json +++ b/apps/builder/package.json @@ -19,11 +19,11 @@ "dependencies": { "@atlaskit/pragmatic-drag-and-drop": "^1.5.0", "@bomb.sh/args": "^0.3.0", - "@codemirror/autocomplete": "^6.18.4", + "@codemirror/autocomplete": "^6.18.6", "@codemirror/commands": "^6.8.0", "@codemirror/lang-css": "^6.3.1", "@codemirror/lang-html": "^6.4.9", - "@codemirror/lang-javascript": "^6.2.2", + "@codemirror/lang-javascript": "^6.2.3", "@codemirror/lang-markdown": "^6.3.2", "@codemirror/language": "^6.10.8", "@codemirror/lint": "^6.8.4", diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7555ebd3ee43..a036a96c3c6a 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -128,8 +128,8 @@ importers: specifier: ^0.3.0 version: 0.3.0 '@codemirror/autocomplete': - specifier: ^6.18.4 - version: 6.18.4 + specifier: ^6.18.6 + version: 6.18.6 '@codemirror/commands': specifier: ^6.8.0 version: 6.8.0 @@ -140,8 +140,8 @@ importers: specifier: ^6.4.9 version: 6.4.9 '@codemirror/lang-javascript': - specifier: ^6.2.2 - version: 6.2.2 + specifier: ^6.2.3 + version: 6.2.3 '@codemirror/lang-markdown': specifier: ^6.3.2 version: 6.3.2 @@ -2397,8 +2397,8 @@ packages: '@cloudflare/workers-types@4.20240701.0': resolution: {integrity: sha512-6Cu6NIAicmb8H6CkzFdQG5Ib+TDs9HU8AKQEGlxnbvEBDmhUNpmL30ETXpLS0asdeyc+rTb2xag0hSv0Z1BflQ==} - '@codemirror/autocomplete@6.18.4': - resolution: {integrity: sha512-sFAphGQIqyQZfP2ZBsSHV7xQvo9Py0rV0dW7W3IMRdS+zDuNb2l3no78CvUaWKGfzFjI4FTrLdUSj86IGb2hRA==} + '@codemirror/autocomplete@6.18.6': + resolution: {integrity: sha512-PHHBXFomUs5DF+9tCOM/UoW6XQ4R44lLNNhRaW9PKPTU0D7lIjRg3ElxaJnTwsl/oHiR93WSXDBrekhoUGCPtg==} '@codemirror/commands@6.8.0': resolution: {integrity: sha512-q8VPEFaEP4ikSlt6ZxjB3zW72+7osfAYW9i8Zu943uqbKuz6utc1+F170hyLUCUltXORjQXRyYQNfkckzA/bPQ==} @@ -2409,8 +2409,8 @@ packages: '@codemirror/lang-html@6.4.9': resolution: {integrity: sha512-aQv37pIMSlueybId/2PVSP6NPnmurFDVmZwzc7jszd2KAF8qd4VBbvNYPXWQq90WIARjsdVkPbw29pszmHws3Q==} - '@codemirror/lang-javascript@6.2.2': - resolution: {integrity: sha512-VGQfY+FCc285AhWuwjYxQyUQcYurWlxdKYT4bqwr3Twnd5wP5WSeu52t4tvvuWmljT4EmgEgZCqSieokhtY8hg==} + '@codemirror/lang-javascript@6.2.3': + resolution: {integrity: sha512-8PR3vIWg7pSu7ur8A07pGiYHgy3hHj+mRYRCSG8q+mPIrl0F02rgpGv+DsQTHRTc30rydOsf5PZ7yjKFg2Ackw==} '@codemirror/lang-markdown@6.3.2': resolution: {integrity: sha512-c/5MYinGbFxYl4itE9q/rgN/sMTjOr8XL5OWnC+EaRMLfCbVUmmubTJfdgpfcSS2SCaT7b+Q+xi3l6CgoE+BsA==} @@ -9801,7 +9801,7 @@ snapshots: '@cloudflare/workers-types@4.20240701.0': {} - '@codemirror/autocomplete@6.18.4': + '@codemirror/autocomplete@6.18.6': dependencies: '@codemirror/language': 6.10.8 '@codemirror/state': 6.5.2 @@ -9817,7 +9817,7 @@ snapshots: '@codemirror/lang-css@6.3.1': dependencies: - '@codemirror/autocomplete': 6.18.4 + '@codemirror/autocomplete': 6.18.6 '@codemirror/language': 6.10.8 '@codemirror/state': 6.5.2 '@lezer/common': 1.2.3 @@ -9825,9 +9825,9 @@ snapshots: '@codemirror/lang-html@6.4.9': dependencies: - '@codemirror/autocomplete': 6.18.4 + '@codemirror/autocomplete': 6.18.6 '@codemirror/lang-css': 6.3.1 - '@codemirror/lang-javascript': 6.2.2 + '@codemirror/lang-javascript': 6.2.3 '@codemirror/language': 6.10.8 '@codemirror/state': 6.5.2 '@codemirror/view': 6.36.2 @@ -9835,9 +9835,9 @@ snapshots: '@lezer/css': 1.1.10 '@lezer/html': 1.3.10 - '@codemirror/lang-javascript@6.2.2': + '@codemirror/lang-javascript@6.2.3': dependencies: - '@codemirror/autocomplete': 6.18.4 + '@codemirror/autocomplete': 6.18.6 '@codemirror/language': 6.10.8 '@codemirror/lint': 6.8.4 '@codemirror/state': 6.5.2 @@ -9847,7 +9847,7 @@ snapshots: '@codemirror/lang-markdown@6.3.2': dependencies: - '@codemirror/autocomplete': 6.18.4 + '@codemirror/autocomplete': 6.18.6 '@codemirror/lang-html': 6.4.9 '@codemirror/language': 6.10.8 '@codemirror/state': 6.5.2