🐛 Fix theme renaming and small refactor tokens forms validation

2025-12-11 22:14:05 +01:00 · 2025-10-24 15:33:01 +02:00
parent 1b81ddebb4
commit 70143f8ae3
7 changed files with 136 additions and 95 deletions
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -71,6 +71,7 @@
 - Fix remove flex button doesn’t work within variant [Taiga #12314](https://tree.taiga.io/project/penpot/issue/12314)
 - Fix an error translation [Taiga #12402](https://tree.taiga.io/project/penpot/issue/12402)
 - Fix problem with certain text input in some editable labels (pages, components, tokens...) being in conflict with the drag/drop functionality [Taiga #12316](https://tree.taiga.io/project/penpot/issue/12316)
+- Fix not controlled theme renaming [Taiga #12411](https://tree.taiga.io/project/penpot/issue/12411)

 ## 2.10.1

--- a/frontend/src/app/main/ui/workspace/tokens/management/create/form.cljs
+++ b/frontend/src/app/main/ui/workspace/tokens/management/create/form.cljs
@@ -29,7 +29,6 @@
   [app.main.ui.ds.buttons.button :refer [button*]]
   [app.main.ui.ds.buttons.icon-button :refer [icon-button*]]
   [app.main.ui.ds.controls.input :refer [input*]]
-   [app.main.ui.ds.controls.utilities.hint-message :refer [hint-message*]]
   [app.main.ui.ds.foundations.assets.icon :as i]
   [app.main.ui.ds.foundations.typography.heading :refer [heading*]]
   [app.main.ui.ds.notifications.context-notification :refer [context-notification*]]
@@ -51,41 +50,6 @@
   [malli.error :as me]
   [rumext.v2 :as mf]))

-;; Schemas ---------------------------------------------------------------------
-
-(def valid-token-name-regexp
-  "Only allow letters and digits for token names.
-  Also allow one `.` for a namespace separator.
-
-  Caution: This will allow a trailing dot like `token-name.`,
-  But we will trim that in the `finalize-name`,
-  to not throw too many errors while the user is editing."
-  #"(?!\$)([a-zA-Z0-9-$_]+\.?)*")
-
-(def valid-token-name-schema
-  (m/-simple-schema
-   {:type :token/invalid-token-name
-    :pred #(re-matches valid-token-name-regexp %)
-    :type-properties {:error/fn #(str (:value %) (tr "workspace.tokens.token-name-validation-error"))}}))
-
-(defn token-name-schema
-  "Generate a dynamic schema validation to check if a token path derived from the name already exists at `tokens-tree`."
-  [{:keys [tokens-tree]}]
-  (let [path-exists-schema
-        (m/-simple-schema
-         {:type :token/name-exists
-          :pred #(not (cft/token-name-path-exists? % tokens-tree))
-          :type-properties {:error/fn #(tr "workspace.tokens.token-name-duplication-validation-error" (:value %))}})]
-    (m/schema
-     [:and
-      [:string {:min 1 :max 255 :error/fn #(str (:value %) (tr "workspace.tokens.token-name-length-validation-error"))}]
-      valid-token-name-schema
-      path-exists-schema])))
-
-(def token-description-schema
-  (m/schema
-   [:string {:max 2048 :error/fn #(tr "errors.field-max-length" 2048)}]))
-
 ;; Helpers ---------------------------------------------------------------------

 (defn finalize-name [name]
@@ -103,7 +67,53 @@
 (defn valid-value? [value]
  (seq (finalize-value value)))

-;; Validation ------------------------------------------------------------------
+;; Schemas ---------------------------------------------------------------------
+
+(def ^:private well-formed-token-name-regexp
+  "Only allow letters and digits for token names.
+  Also allow one `.` for a namespace separator.
+
+  Caution: This will allow a trailing dot like `token-name.`,
+  But we will trim that in the `finalize-name`,
+  to not throw too many errors while the user is editing."
+  #"(?!\$)([a-zA-Z0-9-$_]+\.?)*")
+
+(def ^:private well-formed-token-name-schema
+  (m/-simple-schema
+   {:type :token/invalid-token-name
+    :pred #(re-matches well-formed-token-name-regexp %)
+    :type-properties {:error/fn #(str (:value %) (tr "workspace.tokens.token-name-validation-error"))}}))
+
+(defn- token-name-schema
+  "Generate a dynamic schema validation to check if a token path derived from the name already exists at `tokens-tree`."
+  [{:keys [tokens-tree]}]
+  (let [path-exists-schema
+        (m/-simple-schema
+         {:type :token/name-exists
+          :pred #(not (cft/token-name-path-exists? % tokens-tree))
+          :type-properties {:error/fn #(tr "workspace.tokens.token-name-duplication-validation-error" (:value %))}})]
+    (m/schema
+     [:and
+      [:string {:min 1 :max 255 :error/fn #(str (:value %) (tr "workspace.tokens.token-name-length-validation-error"))}]
+      well-formed-token-name-schema
+      path-exists-schema])))
+
+(defn- validate-token-name
+  [tokens-tree name]
+  (let [schema     (token-name-schema {:tokens-tree tokens-tree})
+        validation (m/explain schema (finalize-name name))]
+    (me/humanize validation)))
+
+(def ^:private token-description-schema
+  (m/schema
+   [:string {:max 2048 :error/fn #(tr "errors.field-max-length" 2048)}]))
+
+(defn- validate-token-description
+  [description]
+  (let [validation (m/explain token-description-schema description)]
+    (me/humanize validation)))
+
+;; Value Validation -------------------------------------------------------------

 (defn check-empty-value [token-value]
  (when (empty? (str/trim token-value))
@@ -330,33 +340,26 @@
        warning-name-change? (deref warning-name-change*)
        token-name-ref (mf/use-var (:name token))
        name-ref (mf/use-ref nil)
-        name-errors (mf/use-state nil)
-
-        validate-name
-        (mf/use-fn
-         (mf/deps tokens-tree-in-selected-set)
-         (fn [value]
-           (let [schema (token-name-schema {:token token
-                                            :tokens-tree tokens-tree-in-selected-set})]
-             (m/explain schema (finalize-name value)))))
+        name-errors* (mf/use-state nil)
+        name-errors (deref name-errors*)

        on-blur-name
        (mf/use-fn
-         (mf/deps touched-name? warning-name-change?)
+         (mf/deps touched-name? warning-name-change? tokens-tree-in-selected-set)
         (fn [e]
           (let [value (dom/get-target-val e)
-                 errors (validate-name value)]
+                 errors (validate-token-name tokens-tree-in-selected-set value)]
             (when touched-name?
               (reset! warning-name-change* true))
-             (reset! name-errors errors))))
+             (reset! name-errors* errors))))

        on-update-name-debounced
        (mf/use-fn
-         (mf/deps touched-name? validate-name)
+         (mf/deps touched-name? tokens-tree-in-selected-set)
         (uf/debounce (fn [token-name]
-                        (let [errors (validate-name token-name)]
+                        (let [errors (validate-token-name tokens-tree-in-selected-set token-name)]
                          (when touched-name?
-                            (reset! name-errors errors))))
+                            (reset! name-errors* errors))))
                      300))

        on-update-name
@@ -370,7 +373,7 @@
             (on-update-name-debounced token-name))))

        valid-name-field? (and
-                           (not @name-errors)
+                           (not name-errors)
                           (valid-name? @token-name-ref))

        ;; Value
@@ -430,19 +433,20 @@
        description-errors* (mf/use-state nil)
        description-errors (deref description-errors*)

-        validate-descripion (mf/use-fn #(m/explain token-description-schema %))
-        on-update-description-debounced (mf/use-fn
-                                         (uf/debounce (fn [e]
-                                                        (let [value (dom/get-target-val e)
-                                                              errors (validate-descripion value)]
-                                                          (reset! description-errors* errors)))))
+        on-update-description-debounced
+        (mf/use-fn
+         (uf/debounce (fn [e]
+                        (let [value (dom/get-target-val e)
+                              errors (validate-token-description value)]
+                          (reset! description-errors* errors)))))
+
        on-update-description
        (mf/use-fn
         (mf/deps on-update-description-debounced)
         (fn [e]
           (reset! description-ref (dom/get-target-val e))
           (on-update-description-debounced e)))
-        valid-description-field? (not description-errors)
+        valid-description-field? (empty? description-errors)

        ;; Form
        disabled? (or (not valid-name-field?)
@@ -451,7 +455,7 @@

        on-submit
        (mf/use-fn
-         (mf/deps is-create validate-name validate-descripion token active-theme-tokens validate-token)
+         (mf/deps is-create tokens-tree-in-selected-set token active-theme-tokens validate-token)
         (fn [e]
           (dom/prevent-default e)
           ;; We have to re-validate the current form values before submitting
@@ -460,13 +464,13 @@
           ;; and press enter before the next validations could return.
           (let [final-name (finalize-name @token-name-ref)
                 valid-name? (try
-                               (not (:errors (validate-name final-name)))
+                               (empty? (:errors (validate-token-name tokens-tree-in-selected-set final-name)))
                               (catch js/Error _ nil))
                 value (mf/ref-val value-ref)
                 final-description @description-ref
                 valid-description? (if final-description
                                      (try
-                                        (not (:errors (validate-descripion final-description)))
+                                        (empty? (:errors (validate-token-description final-description)))
                                        (catch js/Error _ nil))
                                      true)]
             (when (and valid-name? valid-description?)
@@ -560,21 +564,12 @@
                     :variant "comfortable"
                     :auto-focus true
                     :default-value @token-name-ref
-                     :hint-type (when (seq (:errors @name-errors)) "error")
+                     :hint-type (when-not (empty? name-errors) "error")
+                     :hint-message (first name-errors)
                     :ref name-ref
                     :on-blur on-blur-name
                     :on-change on-update-name}])

-       (for [error (->> (:errors @name-errors)
-                        (map #(-> (assoc @name-errors :errors [%])
-                                  (me/humanize)))
-                        (map first))]
-
-         [:> hint-message* {:key error
-                            :message error
-                            :type "error"
-                            :id "token-name-hint"}])
-
       (when (and warning-name-change? (= action "edit"))
         [:div {:class (stl/css :warning-name-change-notification-wrapper)}
          [:> context-notification*
@@ -611,6 +606,8 @@
                   :max-length max-input-length
                   :variant "comfortable"
                   :default-value @description-ref
+                   :hint-type (when-not (empty? description-errors) "error")
+                   :hint-message (first description-errors)
                   :on-blur on-update-description
                   :on-change on-update-description}]]

@@ -1162,4 +1159,4 @@
      :text-case [:> text-case-form* props]
      :text-decoration [:> text-decoration-form* props]
      :font-weight [:> font-weight-form* props]
-      [:> form* props])))
+      [:> form* props])))
--- a/frontend/src/app/main/ui/workspace/tokens/themes/create_modal.cljs
+++ b/frontend/src/app/main/ui/workspace/tokens/themes/create_modal.cljs
@@ -31,9 +31,32 @@
   [app.util.i18n :refer [tr]]
   [app.util.keyboard :as k]
   [cuerdas.core :as str]
+   [malli.core :as m]
+   [malli.error :as me]
   [potok.v2.core :as ptk]
   [rumext.v2 :as mf]))

+;; Schemas ---------------------------------------------------------------------
+
+(defn- theme-name-schema
+  "Generate a dynamic schema validation to check if a theme path derived from the name already exists at `tokens-tree`."
+  [{:keys [group theme-id tokens-lib]}]
+  (m/-simple-schema
+   {:type :token/name-exists
+    :pred (fn [name]
+            (let [theme (ctob/get-theme-by-name tokens-lib group name)]
+              (or (nil? theme)
+                  (= (ctob/get-id theme) theme-id))))
+    :type-properties {:error/fn #(tr "workspace.tokens.theme-name-already-exists")}}))
+
+(defn- validate-theme-name
+  [tokens-lib group theme-id name]
+  (let [schema     (theme-name-schema {:tokens-lib tokens-lib :theme-id theme-id :group group})
+        validation (m/explain schema (str/trim name))]
+    (me/humanize validation)))
+
+;; Form Component --------------------------------------------------------------
+
 (mf/defc empty-themes
  [{:keys [change-view]}]
  (let [create-theme
@@ -166,25 +189,36 @@

 (mf/defc theme-inputs*
  [{:keys [theme on-change-field]}]
-  (let [theme-groups (mf/deref refs/workspace-token-theme-groups)
+  (let [tokens-lib     (mf/deref refs/tokens-lib)
+        theme-groups   (mf/deref refs/workspace-token-theme-groups)
        theme-name-ref (mf/use-ref (:name theme))
-        options (map (fn [group]
-                       {:label group
-                        :id group})
-                     theme-groups)
+        options        (map (fn [group]
+                              {:label group
+                               :id group})
+                            theme-groups)
+        current-group* (mf/use-state (:group theme))
+        current-group  (deref current-group*)
+        name-errors*   (mf/use-state nil)
+        name-errors    (deref name-errors*)

        on-update-group
        (mf/use-fn
         (mf/deps on-change-field)
-         #(on-change-field :group %))
+         (fn [value]
+           (reset! current-group* value)
+           (on-change-field :group value)))

        on-update-name
        (mf/use-fn
-         (mf/deps on-change-field)
+         (mf/deps on-change-field tokens-lib current-group)
         (fn [event]
-           (let [value (-> event dom/get-target dom/get-value)]
-             (on-change-field :name value)
-             (mf/set-ref-val! theme-name-ref value))))]
+           (let [value  (-> event dom/get-target dom/get-value)
+                 errors (validate-theme-name tokens-lib current-group (ctob/get-id theme) value)]
+             (reset! name-errors* errors)
+             (mf/set-ref-val! theme-name-ref value)
+             (if (empty? errors)
+               (on-change-field :name value)
+               (on-change-field :name "")))))]

    [:div {:class (stl/css :edit-theme-inputs-wrapper)}
     [:div {:class (stl/css :group-input-wrapper)}
@@ -202,6 +236,8 @@
                  :variant "comfortable"
                  :default-value (mf/ref-val theme-name-ref)
                  :auto-focus true
+                  :hint-type (when-not (empty? name-errors) "error")
+                  :hint-message (first name-errors)
                  :on-change on-update-name}]]]))

 (mf/defc theme-modal-buttons*
--- a/frontend/src/app/main/ui/workspace/tokens/themes/create_modal.scss
+++ b/frontend/src/app/main/ui/workspace/tokens/themes/create_modal.scss
@@ -34,7 +34,6 @@
  display: flex;
  flex-direction: column;
  gap: deprecated.$s-16;
-  max-height: deprecated.$s-688;
 }

 .edit-theme-form {
--- a/frontend/test/frontend_tests/tokens/token_form_test.cljs
+++ b/frontend/test/frontend_tests/tokens/token_form_test.cljs
@@ -12,15 +12,15 @@

 (t/deftest test-valid-token-name-schema
  ;; Allow regular namespace token names
-  (t/is (some? (m/validate wtf/valid-token-name-schema "Foo")))
-  (t/is (some? (m/validate wtf/valid-token-name-schema "foo")))
-  (t/is (some? (m/validate wtf/valid-token-name-schema "FOO")))
-  (t/is (some? (m/validate wtf/valid-token-name-schema "Foo.Bar.Baz")))
+  (t/is (some? (m/validate wtf/well-formed-token-name-schema "Foo")))
+  (t/is (some? (m/validate wtf/well-formed-token-name-schema "foo")))
+  (t/is (some? (m/validate wtf/well-formed-token-name-schema "FOO")))
+  (t/is (some? (m/validate wtf/well-formed-token-name-schema "Foo.Bar.Baz")))
  ;; Allow trailing tokens
-  (t/is (nil? (m/validate wtf/valid-token-name-schema "Foo.Bar.Baz....")))
+  (t/is (nil? (m/validate wtf/well-formed-token-name-schema "Foo.Bar.Baz....")))
  ;; Disallow multiple separator dots
-  (t/is (nil? (m/validate wtf/valid-token-name-schema "Foo..Bar.Baz")))
+  (t/is (nil? (m/validate wtf/well-formed-token-name-schema "Foo..Bar.Baz")))
  ;; Disallow any special characters
-  (t/is (nil? (m/validate wtf/valid-token-name-schema "Hey Foo.Bar")))
-  (t/is (nil? (m/validate wtf/valid-token-name-schema "Hey😈Foo.Bar")))
-  (t/is (nil? (m/validate wtf/valid-token-name-schema "Hey%Foo.Bar"))))
+  (t/is (nil? (m/validate wtf/well-formed-token-name-schema "Hey Foo.Bar")))
+  (t/is (nil? (m/validate wtf/well-formed-token-name-schema "Hey😈Foo.Bar")))
+  (t/is (nil? (m/validate wtf/well-formed-token-name-schema "Hey%Foo.Bar"))))
--- a/frontend/translations/en.po
+++ b/frontend/translations/en.po
@@ -7858,6 +7858,10 @@ msgstr "none | underline | strike-through or {alias}"
 msgid "workspace.tokens.theme-name"
 msgstr "Theme %s"

+#: src/app/main/ui/workspace/tokens/themes/create_modal.cljs:48
+msgid "workspace.tokens.theme-name-already-exists"
+msgstr "A theme with this name already exists"
+
 #: src/app/main/ui/workspace/tokens/themes/create_modal.cljs:96
 msgid "workspace.tokens.themes-description"
 msgstr ""
--- a/frontend/translations/es.po
+++ b/frontend/translations/es.po
@@ -7736,6 +7736,10 @@ msgstr "none | underline | strike-through o {alias}"
 msgid "workspace.tokens.theme-name"
 msgstr "Tema %s"

+#: src/app/main/ui/workspace/tokens/themes/create_modal.cljs:48
+msgid "workspace.tokens.theme-name-already-exists"
+msgstr "Ya existe un tema con este nombre"
+
 #: src/app/main/ui/workspace/tokens/themes/create_modal.cljs:96
 msgid "workspace.tokens.themes-description"
 msgstr ""