github/penpot
1
0
Fork 0
mirror of https://github.com/penpot/penpot.git synced 2026-10-29 05:50:35 +00:00
Code Issues Packages Projects Releases Wiki Activity

🐛 Fix tab on variant name doesn't validate formula (#12655)

Browse Source 
* 🐛 Fix tab on variant name doesn't validate formula

* ✨ Move helpers to common

* ✨ Mr proposed changes
This commit is contained in:
Pablo Alba 2026-10-28 11:44:38 +01:00 committed by GitHub
parent 00dec9143d
commit 0532f15cee
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
13 changed files with 351 additions and 50 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
common/src/app/common/path_names.cljc
View File

@ -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]

18
common/src/app/common/types/component.cljc
View File

@ -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

2
common/src/app/common/types/shape_tree.cljc
View File

@ -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]

9
common/test/common_tests/path_names_test.cljc
View File

@ -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 / "))))

21
common/test/common_tests/types/components_test.cljc
View File

@ -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")))))

4
frontend/src/app/main/data/changes.cljs
View File

@ -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})

78
frontend/src/app/main/data/workspace.cljs
View File

@ -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

8
frontend/src/app/main/data/workspace/libraries.cljs
View File

@ -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)]

2
frontend/src/app/main/ui/workspace/sidebar/layer_name.cljs
View File

@ -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)))))))

2
frontend/src/app/main/ui/workspace/viewport/widgets.cljs
View File

@ -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

12
frontend/src/app/plugins/shape.cljs
View File

@ -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

232
frontend/test/frontend_tests/data/workspace_variant_rename_test.cljs Normal file
View File

@ -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))))))))))

2
frontend/test/frontend_tests/runner.cljs
View File

@ -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