Skip to content

Commit b8d9844

Browse files
TrySoundTrySound
committed
feat: forbid data variables with the same name on instance
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.
1 parent aa8a720 commit b8d9844

File tree

4 files changed

+111
-57
lines changed

4 files changed

+111
-57
lines changed

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

+67-13
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,6 @@
11
import { z } from "zod";
22
import { nanoid } from "nanoid";
3+
import { computed } from "nanostores";
34
import { useStore } from "@nanostores/react";
45
import {
56
type ReactNode,
@@ -12,6 +13,7 @@ import {
1213
useRef,
1314
createContext,
1415
useEffect,
16+
useCallback,
1517
} from "react";
1618
import { CopyIcon, RefreshIcon, UpgradeIcon } from "@webstudio-is/icons";
1719
import {
@@ -55,7 +57,7 @@ import {
5557
$userPlanFeatures,
5658
} from "~/shared/nano-states";
5759
import { serverSyncStore } from "~/shared/sync";
58-
60+
import { $selectedInstance } from "~/shared/awareness";
5961
import { BindingPopoverProvider } from "~/builder/shared/binding-popover";
6062
import {
6163
EditorDialog,
@@ -68,15 +70,43 @@ import {
6870
SystemResourceForm,
6971
} from "./resource-panel";
7072
import { generateCurl } from "./curl";
71-
import { $selectedInstance } from "~/shared/awareness";
7273

73-
const validateName = (value: string) =>
74-
value.trim().length === 0 ? "Name is required" : "";
74+
const $variablesByName = computed(
75+
[$selectedInstance, $dataSources],
76+
(instance, dataSources) => {
77+
const variablesByName = new Map<DataSource["name"], DataSource["id"]>();
78+
for (const dataSource of dataSources.values()) {
79+
if (dataSource.scopeInstanceId === instance?.id) {
80+
variablesByName.set(dataSource.name, dataSource.id);
81+
}
82+
}
83+
return variablesByName;
84+
}
85+
);
7586

76-
const NameField = ({ defaultValue }: { defaultValue: string }) => {
87+
const NameField = ({
88+
variableId,
89+
defaultValue,
90+
}: {
91+
variableId: undefined | DataSource["id"];
92+
defaultValue: string;
93+
}) => {
7794
const ref = useRef<HTMLInputElement>(null);
7895
const [error, setError] = useState("");
7996
const nameId = useId();
97+
const variablesByName = useStore($variablesByName);
98+
const validateName = useCallback(
99+
(value: string) => {
100+
if (variablesByName.get(value) !== variableId) {
101+
return "Name is already used by another variable on this instance";
102+
}
103+
if (value.trim().length === 0) {
104+
return "Name is required";
105+
}
106+
return "";
107+
},
108+
[variablesByName, variableId]
109+
);
80110
useEffect(() => {
81111
ref.current?.setCustomValidity(validateName(defaultValue));
82112
}, [defaultValue]);
@@ -510,15 +540,21 @@ const VariablePanel = forwardRef<
510540
if (variableType === "parameter") {
511541
return (
512542
<>
513-
<NameField defaultValue={variable?.name ?? ""} />
543+
<NameField
544+
variableId={variable?.id}
545+
defaultValue={variable?.name ?? ""}
546+
/>
514547
<ParameterForm ref={ref} variable={variable} />
515548
</>
516549
);
517550
}
518551
if (variableType === "string") {
519552
return (
520553
<>
521-
<NameField defaultValue={variable?.name ?? ""} />
554+
<NameField
555+
variableId={variable?.id}
556+
defaultValue={variable?.name ?? ""}
557+
/>
522558
<TypeField value={variableType} onChange={setVariableType} />
523559
<StringForm ref={ref} variable={variable} />
524560
</>
@@ -527,7 +563,10 @@ const VariablePanel = forwardRef<
527563
if (variableType === "number") {
528564
return (
529565
<>
530-
<NameField defaultValue={variable?.name ?? ""} />
566+
<NameField
567+
variableId={variable?.id}
568+
defaultValue={variable?.name ?? ""}
569+
/>
531570
<TypeField value={variableType} onChange={setVariableType} />
532571
<NumberForm ref={ref} variable={variable} />
533572
</>
@@ -536,7 +575,10 @@ const VariablePanel = forwardRef<
536575
if (variableType === "boolean") {
537576
return (
538577
<>
539-
<NameField defaultValue={variable?.name ?? ""} />
578+
<NameField
579+
variableId={variable?.id}
580+
defaultValue={variable?.name ?? ""}
581+
/>
540582
<TypeField value={variableType} onChange={setVariableType} />
541583
<BooleanForm ref={ref} variable={variable} />
542584
</>
@@ -545,7 +587,10 @@ const VariablePanel = forwardRef<
545587
if (variableType === "json") {
546588
return (
547589
<>
548-
<NameField defaultValue={variable?.name ?? ""} />
590+
<NameField
591+
variableId={variable?.id}
592+
defaultValue={variable?.name ?? ""}
593+
/>
549594
<TypeField value={variableType} onChange={setVariableType} />
550595
<JsonForm ref={ref} variable={variable} />
551596
</>
@@ -555,7 +600,10 @@ const VariablePanel = forwardRef<
555600
if (variableType === "resource") {
556601
return (
557602
<>
558-
<NameField defaultValue={variable?.name ?? ""} />
603+
<NameField
604+
variableId={variable?.id}
605+
defaultValue={variable?.name ?? ""}
606+
/>
559607
<TypeField value={variableType} onChange={setVariableType} />
560608
<ResourceForm ref={ref} variable={variable} />
561609
</>
@@ -565,7 +613,10 @@ const VariablePanel = forwardRef<
565613
if (variableType === "graphql-resource") {
566614
return (
567615
<>
568-
<NameField defaultValue={variable?.name ?? ""} />
616+
<NameField
617+
variableId={variable?.id}
618+
defaultValue={variable?.name ?? ""}
619+
/>
569620
<TypeField value={variableType} onChange={setVariableType} />
570621
<GraphqlResourceForm ref={ref} variable={variable} />
571622
</>
@@ -575,7 +626,10 @@ const VariablePanel = forwardRef<
575626
if (variableType === "system-resource") {
576627
return (
577628
<>
578-
<NameField defaultValue={variable?.name ?? ""} />
629+
<NameField
630+
variableId={variable?.id}
631+
defaultValue={variable?.name ?? ""}
632+
/>
579633
<TypeField value={variableType} onChange={setVariableType} />
580634
<SystemResourceForm ref={ref} variable={variable} />
581635
</>

‎apps/builder/app/builder/shared/binding-popover.test.ts

+1-1
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
import { expect, test } from "vitest";
2-
import { evaluateExpressionWithinScope } from "./binding-popover";
32
import { encodeDataSourceVariable } from "@webstudio-is/sdk";
3+
import { evaluateExpressionWithinScope } from "./binding-popover";
44

55
test("evaluateExpressionWithinScope works", () => {
66
const variableName = "jsonVariable";

‎apps/builder/package.json

+4-4
Original file line numberDiff line numberDiff line change
@@ -19,15 +19,15 @@
1919
"dependencies": {
2020
"@atlaskit/pragmatic-drag-and-drop": "^1.4.0",
2121
"@codemirror/autocomplete": "^6.18.4",
22-
"@codemirror/commands": "^6.7.1",
22+
"@codemirror/commands": "^6.8.0",
2323
"@codemirror/lang-css": "^6.3.1",
2424
"@codemirror/lang-html": "^6.4.9",
2525
"@codemirror/lang-javascript": "^6.2.2",
26-
"@codemirror/lang-markdown": "^6.3.1",
26+
"@codemirror/lang-markdown": "^6.3.2",
2727
"@codemirror/language": "^6.10.8",
2828
"@codemirror/lint": "^6.8.4",
29-
"@codemirror/state": "^6.5.0",
30-
"@codemirror/view": "^6.36.1",
29+
"@codemirror/state": "^6.5.1",
30+
"@codemirror/view": "^6.36.2",
3131
"@floating-ui/dom": "^1.6.12",
3232
"@fontsource-variable/inter": "^5.0.20",
3333
"@fontsource-variable/manrope": "^5.0.20",

‎pnpm-lock.yaml

+39-39
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)