From 0532f15cee94653b7acf863772b7e39e0f515186 Mon Sep 17 00:00:00 2001 From: Pablo Alba Date: Wed, 28 Oct 2026 11:44:38 +0100 Subject: [PATCH] :bug: Fix tab on variant name doesn't validate formula (#12655) * :bug: Fix tab on variant name doesn't validate formula * :sparkles: Move helpers to common * :sparkles: Mr proposed changes --- common/src/app/common/path_names.cljc | 11 + common/src/app/common/types/component.cljc | 18 ++ common/src/app/common/types/shape_tree.cljc | 2 +- common/test/common_tests/path_names_test.cljc | 9 + .../common_tests/types/components_test.cljc | 21 ++ frontend/src/app/main/data/changes.cljs | 4 +- frontend/src/app/main/data/workspace.cljs | 78 +++--- .../app/main/data/workspace/libraries.cljs | 8 +- .../main/ui/workspace/sidebar/layer_name.cljs | 2 +- .../main/ui/workspace/viewport/widgets.cljs | 2 +- frontend/src/app/plugins/shape.cljs | 12 +- .../data/workspace_variant_rename_test.cljs | 232 ++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 13 files changed, 351 insertions(+), 50 deletions(-) create mode 100644 frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs diff --git a/common/src/app/common/path_names.cljc b/common/src/app/common/path_names.cljc index cc1526eae0..3f5fd28603 100644 --- a/common/src/app/common/path_names.cljc +++ b/common/src/app/common/path_names.cljc @@ -55,6 +55,17 @@ Some naming conventions: (->> (split-path path) (join-path))) +(defn normalize-path-name + "Trim and clean a path-style name. Returns nil when the name is not usable." + [name] + (let [name (some-> name str/trim) + clean-name (when (string? name) (clean-path name))] + (when (and (string? name) + (not (str/ends-with? name "/")) + (string? clean-name) + (not (str/blank? clean-name))) + clean-name))) + (defn merge-path-item "Put the item at the end of the path." [path name] diff --git a/common/src/app/common/types/component.cljc b/common/src/app/common/types/component.cljc index deb5379e72..c45b59ada0 100644 --- a/common/src/app/common/types/component.cljc +++ b/common/src/app/common/types/component.cljc @@ -251,6 +251,24 @@ [shape] (boolean (:is-variant-container shape))) +(defn is-variant-main? + "True when this shape is a variant component main and not the variant + container." + [shape] + (and (is-variant? shape) + (not (is-variant-container? shape)))) + +(defn variant-formula-rename? + "True when renaming this shape with `name` should update the variant + formula instead of the layer or component name. + + `name` must already be trimmed by the caller." + [shape name] + (and (is-variant-main? shape) + (some? (:variant-name shape)) + (string? name) + (not (str/blank? name)))) + (defn set-touched-group [touched group] (when group diff --git a/common/src/app/common/types/shape_tree.cljc b/common/src/app/common/types/shape_tree.cljc index 90109d5b46..0080aae00d 100644 --- a/common/src/app/common/types/shape_tree.cljc +++ b/common/src/app/common/types/shape_tree.cljc @@ -376,7 +376,7 @@ (defn update-object-indices [file page-id] - (update-in file [:pages-index page-id :objects] update-page-index)) + (d/update-in-when file [:pages-index page-id :objects] update-page-index)) (defn rotated-frame? [frame] diff --git a/common/test/common_tests/path_names_test.cljc b/common/test/common_tests/path_names_test.cljc index 9e0d3fb873..04cf0650b8 100644 --- a/common/test/common_tests/path_names_test.cljc +++ b/common/test/common_tests/path_names_test.cljc @@ -31,3 +31,12 @@ (t/is (= (first path) "group")) (t/is (= (second path) "subgroup")) (t/is (= (nth path 2) "name")))) + +(t/deftest normalize-path-name + (t/is (= "Button" (cpn/normalize-path-name " Button "))) + (t/is (= "Group / Button" (cpn/normalize-path-name "Group / Button"))) + (t/is (nil? (cpn/normalize-path-name nil))) + (t/is (nil? (cpn/normalize-path-name ""))) + (t/is (nil? (cpn/normalize-path-name " "))) + (t/is (nil? (cpn/normalize-path-name "Button/"))) + (t/is (nil? (cpn/normalize-path-name "Group / ")))) diff --git a/common/test/common_tests/types/components_test.cljc b/common/test/common_tests/types/components_test.cljc index 4d42df92f8..6098db1e80 100644 --- a/common/test/common_tests/types/components_test.cljc +++ b/common/test/common_tests/types/components_test.cljc @@ -397,3 +397,24 @@ (t/is (= (:id near-copy2-nested-head) (thi/id :nested2-head))) (t/is (= (:id near-copy2-nested4-head) (thi/id :nested4-head))) (t/is (= (:id near-copy2-nested4-child) (thi/id :nested4-child)))))) + +(t/deftest variant-main-and-formula-rename + (let [container {:id (thi/new-id! :container) + :is-variant-container true} + main {:id (thi/new-id! :main) + :variant-id (thi/id :container) + :variant-name "Value1"} + main-bare {:id (thi/new-id! :main-bare) + :variant-id (thi/id :container)} + plain {:id (thi/new-id! :plain)}] + (t/is (false? (ctk/is-variant-main? container))) + (t/is (true? (ctk/is-variant-main? main))) + (t/is (true? (ctk/is-variant-main? main-bare))) + (t/is (false? (ctk/is-variant-main? plain))) + + (t/is (true? (ctk/variant-formula-rename? main "Property 1=another 21"))) + (t/is (false? (ctk/variant-formula-rename? main ""))) + (t/is (false? (ctk/variant-formula-rename? main nil))) + (t/is (false? (ctk/variant-formula-rename? main-bare "Property 1=another 21"))) + (t/is (false? (ctk/variant-formula-rename? container "Property 1=another 21"))) + (t/is (false? (ctk/variant-formula-rename? plain "Button"))))) diff --git a/frontend/src/app/main/data/changes.cljs b/frontend/src/app/main/data/changes.cljs index 865779e5dc..0baea3a2ab 100644 --- a/frontend/src/app/main/data/changes.cljs +++ b/frontend/src/app/main/data/changes.cljs @@ -75,7 +75,9 @@ (not-empty))) (def ^:private xf:map-page-id - (map :page-id)) + ;; Component-only changes have no :page-id; keep drops those nils so + ;; update-object-indices does not create a nil key in :pages-index. + (keep :page-id)) (def ^:private wasm-structural-change-types #{:add-obj :mov-objects}) diff --git a/frontend/src/app/main/data/workspace.cljs b/frontend/src/app/main/data/workspace.cljs index 8390a3ca37..74fe1252be 100644 --- a/frontend/src/app/main/data/workspace.cljs +++ b/frontend/src/app/main/data/workspace.cljs @@ -746,6 +746,37 @@ (update [_ state] (assoc-in state [:workspace-local :shape-for-rename] id)))) +(defn- rename-variant-main + "Apply a layers-panel formula to one variant main. + + A valid formula updates the property names and values. Any other + text keeps the current malformed path: values are cleared and the + text is stored as the variant error." + [file-id shape name] + (ptk/reify ::rename-variant-main + ptk/WatchEvent + (watch [_ state _] + (let [file-data (dsh/lookup-file-data state file-id) + component-id (:component-id shape) + component (ctkl/get-component file-data component-id) + variant-id (:variant-id shape) + variant-properties (:variant-properties component)] + (if (ctv/valid-properties-formula? name) + (rx/of (dwva/update-properties-names-and-values + component-id + variant-id + variant-properties + (ctv/properties-formula->map name)) + (dwva/remove-empty-properties variant-id) + (dwva/update-error component-id)) + (rx/of (dwva/update-properties-names-and-values + component-id + variant-id + variant-properties + {}) + (dwva/remove-empty-properties variant-id) + (dwva/update-error component-id name))))))) + (defn end-rename-shape "End the ongoing shape rename process" ([] (end-rename-shape nil nil)) @@ -760,14 +791,17 @@ (when-let [shape-id (d/nilv shape-id (dm/get-in state [:workspace-local :shape-for-rename]))] (let [shape (dsh/lookup-shape state shape-id) name (str/trim name) - clean-name (cpn/clean-path name) - valid? (and (not (str/ends-with? name "/")) - (string? clean-name) - (not (str/blank? clean-name))) + clean-name (cpn/normalize-path-name name) component-id (:component-id shape) - undo-id (js/Symbol)] + undo-id (js/Symbol)] - (when valid? + (cond + ;; Variant formula edits go through rename-shape-or-variant. + ;; A variant main must not fall through to a plain rename. + (ctc/is-variant-main? shape) + (rx/empty) + + (some? clean-name) (if (ctc/is-variant-container? shape) ;; Rename the full variant when it is a variant container (rx/of (dwva/rename-variant shape-id clean-name)) @@ -791,32 +825,12 @@ (watch [_ state _] (let [file-id (d/nilv file-id (:current-file-id state)) page-id (d/nilv page-id (:current-page-id state)) - - file-data (dsh/lookup-file-data state file-id) - shape - (-> (dsh/lookup-page-objects state file-id page-id) - (get id)) - - is-variant? (ctc/is-variant? shape) - variant-id (when is-variant? (:variant-id shape)) - variant-name (when is-variant? (:variant-name shape)) - component-id (:component-id shape) - component (ctkl/get-component file-data (:component-id shape)) - variant-properties (:variant-properties component)] - (cond - (and variant-name (ctv/valid-properties-formula? name)) - (rx/of (dwva/update-properties-names-and-values - component-id variant-id variant-properties (ctv/properties-formula->map name)) - (dwva/remove-empty-properties variant-id) - (dwva/update-error component-id)) - - variant-name - (rx/of (dwva/update-properties-names-and-values - component-id variant-id variant-properties {}) - (dwva/remove-empty-properties variant-id) - (dwva/update-error component-id name)) - - :else + shape (-> (dsh/lookup-page-objects state file-id page-id) + (get id)) + name (str/trim name)] + (if (ctc/variant-formula-rename? shape name) + ;; update-properties-names-and-values clears :shape-for-rename. + (rx/of (rename-variant-main file-id shape name)) (rx/of (end-rename-shape id name)))))))) ;; --- Update Selected Shapes attrs diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index 3044a05f6c..bc52e63736 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -498,15 +498,11 @@ (ptk/reify ::rename-component-and-main-instance ptk/WatchEvent (watch [_ state _] - (let [name (str/trim name) - clean-name (cpn/clean-path name) - valid? (and (not (str/ends-with? name "/")) - (string? clean-name) - (not (str/blank? clean-name))) + (let [clean-name (cpn/normalize-path-name name) data (dsh/lookup-file-data state) component (dm/get-in data [:components component-id])] - (when (and valid? component) + (when (and (some? clean-name) component) (let [shape-id (:main-instance-id component) page-id (:main-instance-page component)] diff --git a/frontend/src/app/main/ui/workspace/sidebar/layer_name.cljs b/frontend/src/app/main/ui/workspace/sidebar/layer_name.cljs index db497cf242..95afbd2922 100644 --- a/frontend/src/app/main/ui/workspace/sidebar/layer_name.cljs +++ b/frontend/src/app/main/ui/workspace/sidebar/layer_name.cljs @@ -93,7 +93,7 @@ name (str/trim (dom/get-value name-input))] (on-stop-edit) (reset! edition* false) - (st/emit! (dw/end-rename-shape shape-id name)) + (st/emit! (dw/rename-shape-or-variant shape-id name)) (when (fn? on-tab-press) (on-tab-press event))))))) diff --git a/frontend/src/app/main/ui/workspace/viewport/widgets.cljs b/frontend/src/app/main/ui/workspace/viewport/widgets.cljs index aa83faf963..b9806fddda 100644 --- a/frontend/src/app/main/ui/workspace/viewport/widgets.cljs +++ b/frontend/src/app/main/ui/workspace/viewport/widgets.cljs @@ -181,7 +181,7 @@ (let [name-input (mf/ref-val ref) name (str/trim (dom/get-value name-input))] (reset! edition* false) - (st/emit! (dw/end-rename-shape frame-id name)) + (st/emit! (dw/rename-shape-or-variant frame-id name)) (on-frame-leave frame-id)))) cancel-edit diff --git a/frontend/src/app/plugins/shape.cljs b/frontend/src/app/plugins/shape.cljs index 817492536c..aea764dc28 100644 --- a/frontend/src/app/plugins/shape.cljs +++ b/frontend/src/app/plugins/shape.cljs @@ -70,8 +70,7 @@ [app.plugins.utils :as u] [app.util.http :as http] [app.util.object :as obj] - [beicon.v2.core :as rx] - [cuerdas.core :as str])) + [beicon.v2.core :as rx])) (declare shape-proxy) (declare shape-proxy?) @@ -385,11 +384,8 @@ :get #(-> % u/proxy->shape :name) :set (fn [self value] - (let [id (obj/get self "$id") - value (when (string? value) (-> value str/trim cpn/clean-path)) - valid? (and (some? value) - (not (str/ends-with? value "/")) - (not (str/blank? value)))] + (let [id (obj/get self "$id") + value (cpn/normalize-path-name value)] (cond (not (r/check-permission plugin-id "content:write")) (u/not-valid plugin-id :name "Plugin doesn't have 'content:write' permission") @@ -397,7 +393,7 @@ (not (u/page-active? page-id)) (u/not-valid plugin-id :name "Cannot modify a page that is not currently active") - (not valid?) + (nil? value) (u/not-valid plugin-id :name value) :else diff --git a/frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs b/frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs new file mode 100644 index 0000000000..5e177b1b21 --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs @@ -0,0 +1,232 @@ +;; This Source Code Form is subject to the terms of the Mozilla Public +;; License, v. 2.0. If a copy of the MPL was not distributed with this +;; file, You can obtain one at http://mozilla.org/MPL/2.0/. +;; +;; Copyright (c) KALEIDOS SUBSIDIARY SL + +(ns frontend-tests.data.workspace-variant-rename-test + (:require + [app.common.test-helpers.components :as cthc] + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] + [app.common.test-helpers.shapes :as cths] + [app.common.test-helpers.variants :as thv] + [app.common.types.file :as ctf] + [app.common.types.pages-list :as ctpl] + [app.main.data.workspace :as dw] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.state :as ths])) + +(t/use-fixtures :each + {:before cthi/reset-idmap!}) + +(defn- variant-file + [] + (-> (cthf/sample-file :file1) + (thv/add-variant :variant :comp1 :root1 :comp2 :root2 + :variant1-params {:name "Button"} + :variant2-params {:name "Button"}) + (cths/update-shape :variant :name "Button"))) + +(defn- without-variant-name + [file label] + (let [page-id (cthf/current-page-id file) + shape-id (cthi/id label)] + (ctf/update-file-data + file + (fn [data] + (ctpl/update-page data + page-id + #(update-in % [:objects shape-id] dissoc :variant-name)))))) + +(defn- prop-value + [component] + (-> component :variant-properties first :value)) + +(defn- run + "Run workspace events, then validate the file before assertions. + + Pass `:validate? false` only when the fixture is intentionally incomplete + (e.g. a variant main without `:variant-name`)." + [file events done assert-fn & {:keys [validate?] :or {validate? true}}] + (ths/run-store + (ths/setup-store file) + done + events + (fn [state] + (when validate? + (cthf/validate-file! (ths/get-file-from-state state))) + (assert-fn state)))) + +(t/deftest end-rename-shape-does-not-apply-variant-formula + (t/async + done + (let [file (variant-file) + component (cthc/get-component file :comp1) + original-name (:name component) + original-path (:path component) + root2-id (cthi/id :root2)] + (run file + [(dw/start-rename-shape (cthi/id :root1)) + (dw/end-rename-shape (cthi/id :root1) "Property 1=another 21") + (dw/start-rename-shape root2-id)] + done + (fn [state] + (let [file (ths/get-file-from-state state) + shape (cths/get-shape file :root1) + component (cthc/get-component file :comp1)] + ;; Formula updates belong to rename-shape-or-variant. + (t/is (= "Value1" (prop-value component))) + (t/is (= "Value1" (:variant-name shape))) + (t/is (= "Button" (:name shape))) + (t/is (= "Button" (:name (cths/get-shape file :variant)))) + (t/is (= original-name (:name component))) + (t/is (= original-path (:path component))) + (t/is (= "Value2" (prop-value (cthc/get-component file :comp2)))) + (t/is (= root2-id (:shape-for-rename (:workspace-local state)))))))))) + +(t/deftest rename-shape-or-variant-updates-variant-value-and-keeps-layer-name + (t/async + done + (let [file (variant-file) + component (cthc/get-component file :comp1) + original-name (:name component) + original-path (:path component)] + (run file + [(dw/start-rename-shape (cthi/id :root1)) + (dw/rename-shape-or-variant (cthi/id :root1) "Property 1=another 21")] + done + (fn [state] + (let [file (ths/get-file-from-state state) + shape (cths/get-shape file :root1) + component (cthc/get-component file :comp1)] + (t/is (= "another 21" (prop-value component))) + (t/is (= "another 21" (:variant-name shape))) + (t/is (= "Button" (:name shape))) + (t/is (= original-name (:name component))) + (t/is (= original-path (:path component))) + (t/is (nil? (:shape-for-rename (:workspace-local state)))))))))) + +(t/deftest rename-shape-or-variant-clears-shape-for-rename-on-malformed-formula + (t/async + done + (run (variant-file) + [(dw/start-rename-shape (cthi/id :root1)) + (dw/rename-shape-or-variant (cthi/id :root1) "not-a-formula")] + done + (fn [state] + (let [file (ths/get-file-from-state state) + shape (cths/get-shape file :root1)] + (t/is (= "Button" (:name shape))) + (t/is (= "not-a-formula" (:variant-error shape))) + (t/is (= "" (prop-value (cthc/get-component file :comp1)))) + (t/is (nil? (:shape-for-rename (:workspace-local state))))))))) + +(t/deftest rename-shape-or-variant-blank-name-does-not-clear-variant-values + (t/async + done + (run (variant-file) + [(dw/start-rename-shape (cthi/id :root1)) + (dw/rename-shape-or-variant (cthi/id :root1) "")] + done + (fn [state] + (let [file (ths/get-file-from-state state) + shape (cths/get-shape file :root1)] + (t/is (= "Button" (:name shape))) + (t/is (= "Value1" (:variant-name shape))) + (t/is (= "Value1" (prop-value (cthc/get-component file :comp1)))) + (t/is (nil? (:variant-error shape))) + (t/is (nil? (:shape-for-rename (:workspace-local state))))))))) + +(t/deftest end-rename-shape-nil-leaves-variant-main-unchanged + (t/async + done + (run (variant-file) + [(dw/start-rename-shape (cthi/id :root1)) + (dw/end-rename-shape (cthi/id :root1) nil)] + done + (fn [state] + (let [file (ths/get-file-from-state state) + shape (cths/get-shape file :root1)] + (t/is (= "Button" (:name shape))) + (t/is (= "Value1" (:variant-name shape))) + (t/is (= "Value1" (prop-value (cthc/get-component file :comp1)))) + (t/is (nil? (:shape-for-rename (:workspace-local state))))))))) + +(t/deftest end-rename-shape-ignores-variant-main-without-variant-name + (t/async + done + (let [file (-> (variant-file) + (without-variant-name :root1)) + component (cthc/get-component file :comp1) + original-name (:name component) + original-path (:path component) + root2-id (cthi/id :root2)] + (run file + [(dw/start-rename-shape (cthi/id :root1)) + (dw/end-rename-shape (cthi/id :root1) "Property 1=another 21") + (dw/start-rename-shape root2-id)] + done + (fn [state] + (let [file (ths/get-file-from-state state) + shape (cths/get-shape file :root1) + component (cthc/get-component file :comp1)] + (t/is (= "Button" (:name shape))) + (t/is (nil? (:variant-name shape))) + (t/is (= "Value1" (prop-value component))) + (t/is (= original-name (:name component))) + (t/is (= original-path (:path component))) + (t/is (= root2-id (:shape-for-rename (:workspace-local state)))))) + ;; Fixture starts without :variant-name, so the file is incomplete. + :validate? false)))) + +(t/deftest end-rename-shape-keeps-formula-text-off-the-layer-name + (t/async + done + (run (variant-file) + [(dw/start-rename-shape (cthi/id :root1)) + (dw/end-rename-shape (cthi/id :root1) "not-a-formula")] + done + (fn [state] + (let [file (ths/get-file-from-state state) + shape (cths/get-shape file :root1)] + (t/is (= "Button" (:name shape))) + (t/is (= "Value1" (:variant-name shape))) + (t/is (= "Value1" (prop-value (cthc/get-component file :comp1)))) + (t/is (nil? (:variant-error shape))) + (t/is (nil? (:shape-for-rename (:workspace-local state))))))))) + +(t/deftest end-rename-shape-renames-variant-container + (t/async + done + (run (variant-file) + [(dw/end-rename-shape (cthi/id :variant) "Renamed set")] + done + (fn [state] + (let [file (ths/get-file-from-state state)] + (t/is (= "Renamed set" (:name (cths/get-shape file :variant)))) + (t/is (= "Renamed set" (:name (cths/get-shape file :root1)))) + (t/is (= "Renamed set" (:name (cths/get-shape file :root2)))) + (t/is (= "Renamed set" (:name (cthc/get-component file :comp1)))) + (t/is (= "Renamed set" (:name (cthc/get-component file :comp2))))))))) + +(t/deftest end-rename-shape-renames-plain-shape-and-component-main + (t/async + done + (let [file (-> (cthf/sample-file :file1) + (cths/add-sample-shape :frame :type :frame :name "Frame") + (cthc/make-component :comp :frame) + (cths/add-sample-shape :rect :name "Rect")) + rect-id (cthi/id :rect)] + (run file + [(dw/start-rename-shape (cthi/id :frame)) + (dw/end-rename-shape (cthi/id :frame) "Renamed") + (dw/start-rename-shape rect-id) + (dw/end-rename-shape rect-id "Box")] + done + (fn [state] + (let [file (ths/get-file-from-state state)] + (t/is (= "Renamed" (:name (cths/get-shape file :frame)))) + (t/is (= "Renamed" (:name (cthc/get-component file :comp)))) + (t/is (= "Box" (:name (cths/get-shape file :rect)))) + (t/is (nil? (:shape-for-rename (:workspace-local state)))))))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index aefb617e23..c7e2d547fa 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -37,6 +37,7 @@ [frontend-tests.data.workspace-shortcuts-test] [frontend-tests.data.workspace-texts-test] [frontend-tests.data.workspace-thumbnails-test] + [frontend-tests.data.workspace-variant-rename-test] [frontend-tests.data.workspace-versions-test] [frontend-tests.debug-logging-test] [frontend-tests.errors-governor-test] @@ -186,6 +187,7 @@ 'frontend-tests.data.workspace-shortcuts-test 'frontend-tests.data.workspace-texts-test 'frontend-tests.data.workspace-thumbnails-test + 'frontend-tests.data.workspace-variant-rename-test 'frontend-tests.data.workspace-versions-test 'frontend-tests.debug-logging-test 'frontend-tests.errors-governor-test