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 6852d651e79f..4709542ad98a 100644 --- a/apps/builder/app/builder/features/settings-panel/variable-popover.tsx +++ b/apps/builder/app/builder/features/settings-panel/variable-popover.tsx @@ -303,7 +303,16 @@ const ParameterForm = forwardRef< }); ParameterForm.displayName = "ParameterForm"; -const saveVariable = (variable: undefined | DataSource, formData: FormData) => { +type ValueVariableType = Extract< + VariableType, + "string" | "number" | "boolean" | "json" +>; + +const saveVariable = ( + variable: undefined | DataSource, + type: ValueVariableType, + formData: FormData +) => { const dataSourceId = variable?.id ?? nanoid(); // preserve existing instance scope when edit const scopeInstanceId = @@ -311,7 +320,6 @@ const saveVariable = (variable: undefined | DataSource, formData: FormData) => { if (scopeInstanceId === undefined) { return; } - const type = z.string().parse(formData.get("type")); const name = z.string().parse(formData.get("name")); const value = z.string().nullable().parse(formData.get("value")); let variableValue: Extract["value"]; @@ -349,13 +357,15 @@ const saveVariable = (variable: undefined | DataSource, formData: FormData) => { const useValuePanelRef = ({ ref, variable, + type, }: { ref: Ref; variable?: DataSource; + type: ValueVariableType; }) => { useImperativeHandle(ref, () => ({ save: (formData) => { - saveVariable(variable, formData); + saveVariable(variable, type, formData); }, })); }; @@ -369,7 +379,7 @@ const StringForm = forwardRef< } >(({ variable, value: unknownValue, onChange }, ref) => { const value = typeof unknownValue === "string" ? unknownValue : ""; - useValuePanelRef({ ref, variable }); + useValuePanelRef({ ref, variable, type: "string" }); const valueId = useId(); return ( @@ -429,7 +439,7 @@ const NumberForm = forwardRef< valueRef.current?.setCustomValidity(validateNumberValue(value)); setValueError(""); }, [value]); - useValuePanelRef({ ref, variable }); + useValuePanelRef({ ref, variable, type: "number" }); const valueId = useId(); return ( <> @@ -465,7 +475,7 @@ const BooleanForm = forwardRef< } >(({ variable, value: unknownValue, onChange }, ref) => { const value = typeof unknownValue === "boolean" ? unknownValue : false; - useValuePanelRef({ ref, variable }); + useValuePanelRef({ ref, variable, type: "boolean" }); const valueId = useId(); return ( <> @@ -515,7 +525,7 @@ const JsonForm = forwardRef< valueRef.current?.setCustomValidity(validateJsonValue(value)); setValueError(""); }, [value]); - useValuePanelRef({ ref, variable }); + useValuePanelRef({ ref, variable, type: "json" }); return ( <> + @@ -279,6 +281,7 @@ export const VariablesSection = () => { suffix={ } // open panel when add new varable onClick={() => { diff --git a/apps/builder/app/builder/shared/binding-popover.test.ts b/apps/builder/app/builder/shared/binding-popover.test.ts index e4c398491a12..6ea68750f005 100644 --- a/apps/builder/app/builder/shared/binding-popover.test.ts +++ b/apps/builder/app/builder/shared/binding-popover.test.ts @@ -13,3 +13,8 @@ test("evaluateExpressionWithinScope works", () => { }) ).toEqual(2); }); + +test("evaluateExpressionWithinScope treats empty expression as undefined", () => { + expect(evaluateExpressionWithinScope("", {})).toBeUndefined(); + expect(evaluateExpressionWithinScope(" ", {})).toBeUndefined(); +}); diff --git a/apps/builder/app/builder/shared/binding-popover.tsx b/apps/builder/app/builder/shared/binding-popover.tsx index c3cb1d3db42d..53f0e37f782a 100644 --- a/apps/builder/app/builder/shared/binding-popover.tsx +++ b/apps/builder/app/builder/shared/binding-popover.tsx @@ -46,6 +46,7 @@ import { formatValuePreview, type EditorApi, } from "./expression-editor"; +import { normalizeEditorValue } from "~/shared/code-editor-base"; /** * Check if a value is a primitive that can be safely stringified. @@ -78,6 +79,10 @@ export const evaluateExpressionWithinScope = ( expression: string, scope: Record ) => { + if (expression.trim() === "") { + return; + } + const variables = new Map(); for (const [name, value] of Object.entries(scope)) { const decodedName = decodeDataSourceVariable(name); @@ -100,15 +105,16 @@ const BindingPanel = ({ scope: Record; aliases: Map; valueError?: string; - value: string; + value?: string; onChange: () => void; onSave: (value: string, invalid: boolean) => void; }) => { const editorApiRef = useRef(undefined); - const [expression, setExpression] = useState(value); + const normalizedValue = normalizeEditorValue(value); + const [expression, setExpression] = useState(normalizedValue); const usedIdentifiers = useMemo( - () => getExpressionIdentifiers(value), - [value] + () => getExpressionIdentifiers(normalizedValue), + [normalizedValue] ); const [errorsCount, setErrorsCount] = useState(0); const [touched, setTouched] = useState(false); @@ -372,7 +378,7 @@ export const BindingPopover = ({ aliases: Map; variant: BindingVariant; validate?: (value: unknown) => undefined | string; - value: string; + value?: string; onChange: (newValue: string) => void; onRemove: (evaluatedValue: unknown) => void; }) => { @@ -385,7 +391,10 @@ export const BindingPopover = ({ return; } - const valueError = validate?.(evaluateExpressionWithinScope(value, scope)); + const normalizedValue = normalizeEditorValue(value); + const valueError = validate?.( + evaluateExpressionWithinScope(normalizedValue, scope) + ); return ( { hasUnsavedChange.current = true; }} @@ -467,7 +476,11 @@ export const BindingPopover = ({ /> } > - + ); }; diff --git a/apps/builder/app/builder/shared/expression-editor.test.ts b/apps/builder/app/builder/shared/expression-editor.test.ts index dbf8e733a166..5d843def7879 100644 --- a/apps/builder/app/builder/shared/expression-editor.test.ts +++ b/apps/builder/app/builder/shared/expression-editor.test.ts @@ -5,6 +5,7 @@ import { javascript } from "@codemirror/lang-javascript"; import { allowedArrayMethods, allowedStringMethods } from "@webstudio-is/sdk"; import { completionPath, + formatValue, generateCompletionOptions, getCompletionTarget, getCompletionReplacement, @@ -75,6 +76,19 @@ const getCompletionPath = (value: string) => { return completionPath(new CompletionContext(state, value.length, true)); }; +describe("formatValue", () => { + test("formats missing preview values as an empty editor string", () => { + expect(formatValue(undefined)).toEqual(""); + expect(typeof formatValue(undefined)).toEqual("string"); + }); + + test("always returns a string for editor values", () => { + expect(formatValue(() => {})).toEqual(""); + expect(formatValue("text")).toEqual('"text"'); + expect(formatValue(0)).toEqual("0"); + }); +}); + describe("generateCompletionOptions", () => { describe("object properties", () => { test("returns object properties with preview values", () => { diff --git a/apps/builder/app/builder/shared/expression-editor.tsx b/apps/builder/app/builder/shared/expression-editor.tsx index 013c4865c886..f6053633551c 100644 --- a/apps/builder/app/builder/shared/expression-editor.tsx +++ b/apps/builder/app/builder/shared/expression-editor.tsx @@ -40,6 +40,7 @@ import { EditorDialogButton, EditorDialogControl, type EditorApi, + normalizeEditorValue, } from "~/shared/code-editor-base"; import { decodeDataVariableName, @@ -50,8 +51,11 @@ import { export type { EditorApi }; -export const formatValue = (value: unknown) => { +export const formatValue = (value: unknown): string => { try { + if (value === undefined) { + return ""; + } if (Array.isArray(value)) { // format arrays as multiline return JSON.stringify(value, null, 2); @@ -61,7 +65,7 @@ export const formatValue = (value: unknown) => { // syntax highlighting as expression instead of block return `(${JSON.stringify(value, null, 2)})`; } - return JSON.stringify(value); + return JSON.stringify(value) ?? ""; } catch { // show nothing when value is invalid return ""; @@ -574,10 +578,11 @@ export const ExpressionEditor = ({ color?: "error"; autoFocus?: boolean; readOnly?: boolean; - value: string; + value?: string; onChange: (value: string) => void; onChangeComplete: (value: string) => void; }) => { + const normalizedValue = normalizeEditorValue(value); const { nameById, idByName } = useMemo(() => { const nameById = new Map(); const idByName = new Map(); @@ -592,10 +597,10 @@ export const ExpressionEditor = ({ }, [aliases]); const expressionWithUnsetVariables = useMemo(() => { return unsetExpressionVariables({ - expression: value, + expression: normalizedValue, unsetNameById: nameById, }); - }, [value, nameById]); + }, [normalizedValue, nameById]); const scopeWithUnsetVariables = useMemo(() => { const newScope: typeof scope = {}; for (const [identifier, value] of Object.entries(scope)) { diff --git a/apps/builder/app/shared/code-editor-base.test.ts b/apps/builder/app/shared/code-editor-base.test.ts index 3d1ca7e2be9c..321b81b6ddea 100644 --- a/apps/builder/app/shared/code-editor-base.test.ts +++ b/apps/builder/app/shared/code-editor-base.test.ts @@ -1,6 +1,6 @@ import { EditorSelection } from "@codemirror/state"; import { expect, test } from "vitest"; -import { clampEditorSelection } from "./code-editor-base"; +import { clampEditorSelection, normalizeEditorValue } from "./code-editor-base"; test("clampEditorSelection preserves selection within document length", () => { const selection = EditorSelection.create([ @@ -35,3 +35,8 @@ test("clampEditorSelection clamps selection to document length", () => { ]); expect(clampedSelection.mainIndex).toBe(selection.mainIndex); }); + +test("normalizeEditorValue defaults undefined to empty string", () => { + expect(normalizeEditorValue(undefined)).toBe(""); + expect(normalizeEditorValue("value")).toBe("value"); +}); diff --git a/apps/builder/app/shared/code-editor-base.tsx b/apps/builder/app/shared/code-editor-base.tsx index d435d09636ef..561a568a4f6e 100644 --- a/apps/builder/app/shared/code-editor-base.tsx +++ b/apps/builder/app/shared/code-editor-base.tsx @@ -71,6 +71,8 @@ export const clampEditorSelection = ( ); }; +export const normalizeEditorValue = (value: undefined | string) => value ?? ""; + export const getCodeEditorCssVars = ({ minHeight, maxHeight, @@ -239,7 +241,7 @@ type EditorContentProps = { autoFocus?: boolean; invalid?: boolean; showShortcuts?: boolean; - value: string; + value?: string; onChange: (value: string) => void; onChangeComplete: (value: string) => void; }; @@ -385,15 +387,16 @@ export const EditorContent = ({ if (view === undefined) { return; } + const nextValue = normalizeEditorValue(value); // prevent updating when editor has the same state // and can be the source of new value - if (value === view.state.doc.toString()) { + if (nextValue === view.state.doc.toString()) { return; } view.dispatch({ - changes: { from: 0, to: view.state.doc.length, insert: value }, - selection: clampEditorSelection(view.state.selection, value.length), + changes: { from: 0, to: view.state.doc.length, insert: nextValue }, + selection: clampEditorSelection(view.state.selection, nextValue.length), annotations: [ExternalChange.of(true)], }); }, [value]); diff --git a/apps/builder/app/shared/instance-utils.test.tsx b/apps/builder/app/shared/instance-utils.test.tsx index 2f2788a089b0..c54cd423fd04 100644 --- a/apps/builder/app/shared/instance-utils.test.tsx +++ b/apps/builder/app/shared/instance-utils.test.tsx @@ -982,6 +982,46 @@ describe("reparent instance", () => { ); }); + test("sort shared slot content through visible slot drop target", () => { + const data = renderData( + <$.Body ws:id="body"> + <$.Slot ws:id="slot1"> + <$.Fragment ws:id="fragment"> + <$.Box ws:id="box1"> + <$.Box ws:id="box2"> + <$.Box ws:id="box3"> + + + <$.Slot ws:id="slot2"> + {/* same ids */} + <$.Fragment ws:id="fragment"> + <$.Box ws:id="box1"> + <$.Box ws:id="box2"> + <$.Box ws:id="box3"> + + + + ); + $registeredComponentMetas.set(createFakeComponentMetas({})); + + reparentInstanceMutable(data, ["box1", "fragment", "slot1", "body"], { + parentSelector: ["slot2", "body"], + position: 3, + }); + + expect(data.instances.get("slot1")?.children).toEqual([ + { type: "id", value: "fragment" }, + ]); + expect(data.instances.get("slot2")?.children).toEqual([ + { type: "id", value: "fragment" }, + ]); + expect(data.instances.get("fragment")?.children).toEqual([ + { type: "id", value: "box2" }, + { type: "id", value: "box3" }, + { type: "id", value: "box1" }, + ]); + }); + test("reparent only selected slot occurrence", () => { const data = renderData( <$.Body ws:id="body"> @@ -1259,7 +1299,7 @@ describe("delete instance", () => { expect(data.instances.get("slotId")?.children.length).toEqual(0); }); - test("delete only selected slot occurrence", () => { + test("delete shared slot content from all slot occurrences", () => { const data = renderData( <$.Body ws:id="bodyId"> <$.Slot ws:id="slot1"> @@ -1281,13 +1321,14 @@ describe("delete instance", () => { getInstancePath(["div", "fragment", "slot1", "bodyId"], data.instances) ); - expect(data.instances.get("slot1")?.children).toEqual([]); - expect(data.instances.get("slot2")?.children).toEqual([ + expect(data.instances.get("slot1")?.children).toEqual([ { type: "id", value: "fragment" }, ]); - expect(data.instances.get("fragment")?.children).toEqual([ - { type: "id", value: "div" }, + expect(data.instances.get("slot2")?.children).toEqual([ + { type: "id", value: "fragment" }, ]); + expect(data.instances.get("fragment")?.children).toEqual([]); + expect(data.instances.has("div")).toEqual(false); }); }); diff --git a/apps/builder/app/shared/instance-utils.ts b/apps/builder/app/shared/instance-utils.ts index 2e658bf8948e..3fc55b6c7711 100644 --- a/apps/builder/app/shared/instance-utils.ts +++ b/apps/builder/app/shared/instance-utils.ts @@ -505,18 +505,14 @@ export const reparentInstanceMutable = ( ? initialSourceInstancePath[1].instance.id : undefined; const targetSlot = data.instances.get(dropTarget.parentSelector[0]); - if ( + const isSameSharedSlotFragment = sourceFragmentId !== undefined && targetSlot?.component === "Slot" && targetSlot.children[0]?.type === "id" && - targetSlot.children[0].value === sourceFragmentId - ) { - return sourceInstanceSelector; - } - const sourceInstancePath = detachSharedSlotContentMutable( - data, - initialSourceInstancePath ?? [] - ); + targetSlot.children[0].value === sourceFragmentId; + const sourceInstancePath = isSameSharedSlotFragment + ? (initialSourceInstancePath ?? []) + : detachSharedSlotContentMutable(data, initialSourceInstancePath ?? []); sourceInstanceSelector = sourceInstancePath[0]?.instanceSelector; if (sourceInstanceSelector === undefined) { return; @@ -621,6 +617,21 @@ export const reparentInstance = ( }); }; +const countInstanceChildReferences = ( + instances: Instances, + instanceId: Instance["id"] +) => { + let count = 0; + for (const instance of instances.values()) { + for (const child of instance.children) { + if (child.type === "id" && child.value === instanceId) { + count += 1; + } + } + } + return count; +}; + export const deleteInstanceMutable = ( data: Omit, instancePath: undefined | InstancePath @@ -628,7 +639,6 @@ export const deleteInstanceMutable = ( if (instancePath === undefined) { return false; } - instancePath = detachSharedSlotContentMutable(data, instancePath); const { instances, props, @@ -650,7 +660,8 @@ export const deleteInstanceMutable = ( if ( parentInstance?.component === "Fragment" && parentInstance.children.length === 1 && - grandparentInstance + grandparentInstance && + countInstanceChildReferences(instances, parentInstance.id) < 2 ) { targetInstance = parentInstance; parentInstance = grandparentInstance; @@ -707,21 +718,6 @@ export const deleteInstanceMutable = ( return true; }; -const countInstanceChildReferences = ( - instances: Instances, - instanceId: Instance["id"] -) => { - let count = 0; - for (const instance of instances.values()) { - for (const child of instance.children) { - if (child.type === "id" && child.value === instanceId) { - count += 1; - } - } - } - return count; -}; - const cloneSharedSlotFragmentMutable = ( data: Omit, slotId: Instance["id"], diff --git a/apps/builder/app/shared/page-utils.test.tsx b/apps/builder/app/shared/page-utils.test.tsx index bca34a5836af..452b94c87a78 100644 --- a/apps/builder/app/shared/page-utils.test.tsx +++ b/apps/builder/app/shared/page-utils.test.tsx @@ -217,6 +217,59 @@ describe("insert page copy", () => { }); }); + test("preserves slot content ids when duplicating page", () => { + const dataWithoutPages = renderData( + <$.Body ws:id="bodyId"> + <$.Slot ws:id="slotId"> + <$.Fragment ws:id="fragmentId"> + <$.Box ws:id="boxId"> + + + + ); + const data = getWebstudioDataStub({ + ...dataWithoutPages, + pages: migratePages({ + meta: {}, + homePage: { + id: "pageId", + name: "Name", + path: "", + title: `"Title"`, + meta: {}, + rootInstanceId: "bodyId", + }, + pages: [], + folders: [createRootFolder(["pageId"])], + }), + }); + + insertPageCopyMutable({ + source: { data, pageId: "pageId" }, + target: { data, folderId: ROOT_FOLDER_ID }, + }); + + const copiedPage = getCopiedPages(data)[0]; + expect(copiedPage.rootInstanceId).not.toBe("bodyId"); + const copiedBody = data.instances.get(copiedPage.rootInstanceId); + const copiedSlotId = copiedBody?.children[0]?.value; + if (copiedSlotId === undefined) { + throw Error("Expected copied slot id"); + } + expect(copiedSlotId).not.toBe("slotId"); + expect(data.instances.get(copiedSlotId)?.children).toEqual([ + { type: "id", value: "fragmentId" }, + ]); + expect( + Array.from(data.instances.values()).filter( + (instance) => instance.id === "fragmentId" + ) + ).toHaveLength(1); + expect(data.instances.get("fragmentId")?.children).toEqual([ + { type: "id", value: "boxId" }, + ]); + }); + test("deduplicate path for non-home page copy", () => { const data = getWebstudioDataStub({ instances: toMap([ diff --git a/packages/project/src/db/build-patch.test.ts b/packages/project/src/db/build-patch.test.ts index bc2906e0516e..a014e7b0774d 100644 --- a/packages/project/src/db/build-patch.test.ts +++ b/packages/project/src/db/build-patch.test.ts @@ -249,6 +249,55 @@ describe("patchBuild", () => { ).toBe("/company"); }); + test("applies data source variable additions", async () => { + const result = await createBuildPatchUpdate({ + build: buildRow, + clientVersion: 3, + transactions: [ + transaction({ + payload: [ + { + namespace: "dataSources", + patches: [ + { + op: "add", + path: ["variable-1"], + value: { + id: "variable-1", + scopeInstanceId: "body-1", + name: "title", + type: "variable", + value: { type: "string", value: "" }, + }, + }, + ], + }, + ], + }), + ], + }); + + expect(result).toMatchObject({ + status: "ok", + nextVersion: 4, + update: { + lastTransactionId: "tx-1", + version: 4, + }, + }); + if (result.status === "ok") { + expect(JSON.parse(result.update?.dataSources ?? "")).toEqual([ + { + id: "variable-1", + scopeInstanceId: "body-1", + name: "title", + type: "variable", + value: { type: "string", value: "" }, + }, + ]); + } + }); + test("returns ok when retrying a transaction already saved by the server", async () => { let didPatch = false; server.use(