feat: forbid data variables with the same name on instance (#4771)

Ref #4768 We don't want to allow data variables on one instance with the same name. Though we will allow child variables to mask parent ones in another PR. To test: - create variable var1 on box - try to create another variable with var1 - create variable var2 - try to rename it to var1 - create variable on body with var1
webstudio-is · Jan 22, 2025 · 7376743 · 7376743
1 parent aa8a720
commit 7376743
Show file tree

Hide file tree

Showing 4 changed files with 112 additions and 58 deletions.
diff --git a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx
@@ -1,5 +1,6 @@
 import { z } from "zod";
 import { nanoid } from "nanoid";
+import { computed } from "nanostores";
 import { useStore } from "@nanostores/react";
 import {
   type ReactNode,
@@ -12,6 +13,7 @@ import {
   useRef,
   createContext,
   useEffect,
+  useCallback,
 } from "react";
 import { CopyIcon, RefreshIcon, UpgradeIcon } from "@webstudio-is/icons";
 import {
@@ -55,7 +57,7 @@ import {
   $userPlanFeatures,
 } from "~/shared/nano-states";
 import { serverSyncStore } from "~/shared/sync";
-
+import { $selectedInstance } from "~/shared/awareness";
 import { BindingPopoverProvider } from "~/builder/shared/binding-popover";
 import {
   EditorDialog,
@@ -68,18 +70,46 @@ import {
   SystemResourceForm,
 } from "./resource-panel";
 import { generateCurl } from "./curl";
-import { $selectedInstance } from "~/shared/awareness";
 
-const validateName = (value: string) =>
-  value.trim().length === 0 ? "Name is required" : "";
+const $variablesByName = computed(
+  [$selectedInstance, $dataSources],
+  (instance, dataSources) => {
+    const variablesByName = new Map<DataSource["name"], DataSource["id"]>();
+    for (const dataSource of dataSources.values()) {
+      if (dataSource.scopeInstanceId === instance?.id) {
+        variablesByName.set(dataSource.name, dataSource.id);
+      }
+    }
+    return variablesByName;
+  }
+);
 
-const NameField = ({ defaultValue }: { defaultValue: string }) => {
+const NameField = ({
+  variableId,
+  defaultValue,
+}: {
+  variableId: undefined | DataSource["id"];
+  defaultValue: string;
+}) => {
   const ref = useRef<HTMLInputElement>(null);
   const [error, setError] = useState("");
   const nameId = useId();
+  const variablesByName = useStore($variablesByName);
+  const validateName = useCallback(
+    (value: string) => {
+      if (variablesByName.get(value) !== variableId) {
+        return "Name is already used by another variable on this instance";
+      }
+      if (value.trim().length === 0) {
+        return "Name is required";
+      }
+      return "";
+    },
+    [variablesByName, variableId]
+  );
   useEffect(() => {
     ref.current?.setCustomValidity(validateName(defaultValue));
-  }, [defaultValue]);
+  }, [defaultValue, validateName]);
   return (
     <Grid gap={1}>
       <Label htmlFor={nameId}>Name</Label>
@@ -510,15 +540,21 @@ const VariablePanel = forwardRef<
   if (variableType === "parameter") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <ParameterForm ref={ref} variable={variable} />
       </>
     );
   }
   if (variableType === "string") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <TypeField value={variableType} onChange={setVariableType} />
         <StringForm ref={ref} variable={variable} />
       </>
@@ -527,7 +563,10 @@ const VariablePanel = forwardRef<
   if (variableType === "number") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <TypeField value={variableType} onChange={setVariableType} />
         <NumberForm ref={ref} variable={variable} />
       </>
@@ -536,7 +575,10 @@ const VariablePanel = forwardRef<
   if (variableType === "boolean") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <TypeField value={variableType} onChange={setVariableType} />
         <BooleanForm ref={ref} variable={variable} />
       </>
@@ -545,7 +587,10 @@ const VariablePanel = forwardRef<
   if (variableType === "json") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <TypeField value={variableType} onChange={setVariableType} />
         <JsonForm ref={ref} variable={variable} />
       </>
@@ -555,7 +600,10 @@ const VariablePanel = forwardRef<
   if (variableType === "resource") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <TypeField value={variableType} onChange={setVariableType} />
         <ResourceForm ref={ref} variable={variable} />
       </>
@@ -565,7 +613,10 @@ const VariablePanel = forwardRef<
   if (variableType === "graphql-resource") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <TypeField value={variableType} onChange={setVariableType} />
         <GraphqlResourceForm ref={ref} variable={variable} />
       </>
@@ -575,7 +626,10 @@ const VariablePanel = forwardRef<
   if (variableType === "system-resource") {
     return (
       <>
-        <NameField defaultValue={variable?.name ?? ""} />
+        <NameField
+          variableId={variable?.id}
+          defaultValue={variable?.name ?? ""}
+        />
         <TypeField value={variableType} onChange={setVariableType} />
         <SystemResourceForm ref={ref} variable={variable} />
       </>

diff --git a/apps/builder/app/builder/shared/binding-popover.test.ts b/apps/builder/app/builder/shared/binding-popover.test.ts
@@ -1,6 +1,6 @@
 import { expect, test } from "vitest";
-import { evaluateExpressionWithinScope } from "./binding-popover";
 import { encodeDataSourceVariable } from "@webstudio-is/sdk";
+import { evaluateExpressionWithinScope } from "./binding-popover";
 
 test("evaluateExpressionWithinScope works", () => {
   const variableName = "jsonVariable";

diff --git a/apps/builder/package.json b/apps/builder/package.json
@@ -19,15 +19,15 @@
   "dependencies": {
     "@atlaskit/pragmatic-drag-and-drop": "^1.4.0",
     "@codemirror/autocomplete": "^6.18.4",
-    "@codemirror/commands": "^6.7.1",
+    "@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-markdown": "^6.3.1",
+    "@codemirror/lang-markdown": "^6.3.2",
     "@codemirror/language": "^6.10.8",
     "@codemirror/lint": "^6.8.4",
-    "@codemirror/state": "^6.5.0",
-    "@codemirror/view": "^6.36.1",
+    "@codemirror/state": "^6.5.1",
+    "@codemirror/view": "^6.36.2",
     "@floating-ui/dom": "^1.6.12",
     "@fontsource-variable/inter": "^5.0.20",
     "@fontsource-variable/manrope": "^5.0.20",

diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml