🐛 Fix repair bad variants (#11983)

* 🐛 Ensure normalized names in components and variants

We have a rule of that shape names that include / separator should have
it normalized, with one space before and one after. But this rule is
being enforced only in the UI and the plugins API. There may be other
ways of creating shapes that does not follow it.

For normal shapes this is not really a problem. But for main instances
it may cause that the name of the shape does not match the joined
path+name of the component, and this may generate some problems.

We have decided to add a migration to fix this in all main instances and
variant containers and enforce it in the future with integrity
validations (normal shapes may be left as is).

* 🐛 Fix bad variant-id in the component and the main instance

* 🔧 Fix some issues detected in PR review

* 🐛 Add nil guard

common/src/app/common/files/builder.cljc

@@ -16,6 +16,7 @@
    [app.common.files.helpers :as cph]
    [app.common.files.migrations :as fmig]
    [app.common.geom.shapes :as gsh]
+   [app.common.path-names :as cpn]
    [app.common.schema :as sm]
    [app.common.svg :as csvg]
    [app.common.time :as dt]
@@ -102,7 +103,8 @@
                      (str/capital (d/name type)))))
 
     :always
-    (update :name unique-name state)))
+    (-> (update :name cpn/clean-path)
+        (update :name unique-name state))))
 
 ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
 ;; SCHEMAS

common/src/app/common/files/migrations.cljc

@@ -21,21 +21,24 @@
    [app.common.geom.shapes.text :as gsht]
    [app.common.logging :as l]
    [app.common.math :as mth]
+   [app.common.path-names :as cpn]
    [app.common.schema :as sm]
    [app.common.svg :as csvg]
    [app.common.types.color :as types.color]
    [app.common.types.component :as ctk]
+   [app.common.types.components-list :as ctkl]
    [app.common.types.container :as ctn]
    [app.common.types.file :as ctf]
    [app.common.types.fills :as types.fills]
    [app.common.types.path :as path]
    [app.common.types.path.segment :as path.segment]
    [app.common.types.shape :as cts]
+   [app.common.types.shape-tree :as ctt]
    [app.common.types.shape.interactions :as ctsi]
    [app.common.types.shape.shadow :as ctss]
    [app.common.types.shape.text :as ctst]
    [app.common.types.text :as types.text]
-   [app.common.types.token :as ctt]
+   [app.common.types.token :as cto]
    [app.common.types.tokens-lib :as ctob]
    [app.common.types.variant :as ctv]
    [app.common.uuid :as uuid]
@@ -2169,7 +2172,7 @@
               (if (some? token)
                 (update shape :applied-tokens
                         (fn [applied]
-                          (-> (zipmap ctt/per-side-stroke-width-keys (repeat token))
+                          (-> (zipmap cto/per-side-stroke-width-keys (repeat token))
                               (merge applied)
                               (dissoc :stroke-width))))
                 shape)))
@@ -2219,6 +2222,66 @@
         (update :pages-index d/update-vals update-container)
         (d/update-when :components d/update-vals update-container))))
 
+(defmethod migrate-data "0033-normalize-main-instance-names"
+  [data _]
+  ;; In penpot shape names always have "/" characters normalized with one space between and after,
+  ;; to form paths. But this is not enforced and may be places that still admit names like
+  ;; "Group/Name" or "Group   /   Name". With normal shapes this is not a problem. But for main
+  ;; instances, the name of the root shape must match exactly the path+name of the component.
+  ;; Here we normalize the names of all main instances, and a new file integrity validation has
+  ;; been added to enforce this from now on.
+  ;; Main instances in variants are a special case because they need to match the variant name.
+  (letfn [(update-shape [container shape-id]
+            (let [shape (ctn/get-shape container shape-id)
+
+                  shape (cond
+                          ;; Variant container must have a normalized name
+                          (ctk/is-variant-container? shape)
+                          (let [name       (:name shape)
+                                clean-name (cpn/clean-path name)
+                                clean-name (if (str/blank? clean-name)
+                                             "(empty)" ;; this may occur if the name contains only separators
+                                             clean-name)]
+                            (if (= name clean-name)
+                              shape
+                              (assoc shape :name clean-name)))
+
+                          ;; Main instance inside a variant must have the same name as the container
+                          (ctk/main-instance? shape)
+                          (if (ctk/is-variant? shape)
+                            (let [variant-container (ctn/get-shape container (:parent-id shape))]
+                              (if (or (= (:name shape) (:name variant-container))
+                                      (nil? variant-container))
+                                shape
+                                (assoc shape :name (:name variant-container))))
+
+                            ;; Other main instances must have the same name as the path+name of the component
+                            (let [component     (ctkl/get-component data (:component-id shape) true)
+                                  expected-name (when component
+                                                  (cpn/merge-path-item (:path component) (:name component)))]
+                              (if (or (= expected-name (:name shape))
+                                      (nil? expected-name))
+                                shape
+                                (assoc shape :name expected-name))))
+
+                          ;; Any other shape does not need processing
+                          :else shape)]
+
+              (reduce update-shape
+                      (ctt/set-shape container shape)
+                      (:shapes shape))))
+
+          (update-container
+            [container]
+            (let [root (ctn/get-container-root container)]
+              (if root
+                (update-shape container (:id root))
+                container)))]
+
+    (-> data
+        (update :pages-index d/update-vals update-container)
+        (d/update-when :components d/update-vals update-container))))
+
 (def available-migrations
   (into (d/ordered-set)
         ["legacy-2"
@@ -2308,4 +2371,5 @@
          "0029-move-background-blur-out-of-blur"
          "0030-remove-stroke-per-side-attr"
          "0031-migrate-stroke-width-token-attr"
-         "0032-fix-children-not-in-parent"]))
+         "0032-fix-children-not-in-parent"
+         "0033-normalize-main-instance-names"]))

common/src/app/common/files/repair.cljc

@@ -7,6 +7,7 @@
 (ns app.common.files.repair
   (:require
    [app.common.data :as d]
+   [app.common.data.macros :as dm]
    [app.common.files.changes-builder :as pcb]
    [app.common.files.helpers :as cfh]
    [app.common.files.tokens :as cfo]
@@ -215,6 +216,20 @@
         (pcb/with-library-data file-data)
         (pcb/update-component (:component-id shape) repair-component))))
 
+(defmethod repair-error :invalid-main-instance-name
+  [_ {:keys [shape page-id args] :as error} file-data _]
+  (let [repair-shape
+        (fn [shape]
+          ;; Change the name to match the component
+          (let [component-name (:component-name args)]
+            (log/debug :hint (str "  -> rename shape " (:id shape) " to " component-name))
+            (assoc shape :name component-name)))]
+
+    (log/debug :hint "repairing shape :invalid-main-instance-name" :id (:id shape) :name (:name shape) :page-id page-id)
+    (-> (pcb/empty-changes nil page-id)
+        (pcb/with-file-data file-data)
+        (pcb/update-shapes [(:id shape)] repair-shape))))
+
 (defmethod repair-error :invalid-main-instance
   [_ {:keys [shape page-id] :as error} file-data _]
   (let [repair-shape
@@ -729,6 +744,42 @@
         (pcb/with-file-data file-data)
         (pcb/update-shapes [parent-id] repair-fn))))
 
+(defmethod repair-error :variant-container-bad-name
+  [_ {:keys [shape page-id args]} file-data _]
+  (let [clean-name (:clean-name args)
+
+        repair-shape
+        (fn [shape]
+          (log/debug :hint "  -> set :name of shape" :id (:id shape) :name clean-name)
+          (assoc shape :name clean-name))
+
+        repair-component
+        (fn [component]
+          (let [[path name] (cpn/split-group-name clean-name)]
+            (log/debug :hint "  -> set :path and :name of component" :id (:id component) :path path :name name)
+            (assoc component :path path :name name)))
+
+        all-shape-ids
+        (cons (:id shape) (:shapes shape))
+
+        objects
+        (dm/get-in file-data [:pages-index page-id :objects])
+
+        all-main-instances
+        (map (d/getf objects) (:shapes shape))
+
+        all-component-ids
+        (map :component-id all-main-instances)]
+
+    (log/debug :hint "repairing shape :variant-container-bad-name" :id (:id shape) :name (:name shape) :page-id page-id)
+    (as-> (pcb/empty-changes nil page-id) $
+      (pcb/with-file-data $ file-data)
+      (pcb/with-library-data $ file-data)
+      (pcb/update-shapes $ all-shape-ids repair-shape)
+      (reduce #(pcb/update-component %1 %2 repair-component)
+              $
+              all-component-ids))))
+
 (defmethod repair-error :variant-main-bad-name
   [_ {:keys [shape page-id args]} file-data _]
   (let [repair-fn
@@ -771,14 +822,22 @@
         (fn [shape]
           (let [variant-id (:variant-id args)]
             ;; Set the desired variant-id
-            (log/debug :hint (str "  -> set variant-id to " variant-id))
-            (assoc shape
-                   :variant-id variant-id)))]
+            (log/debug :hint (str "  -> set variant-id in shape to " variant-id))
+            (assoc shape :variant-id variant-id)))
+
+        repair-component
+        (fn [component]
+          (let [variant-id (:variant-id args)]
+            ;; Set the desired variant-id
+            (log/debug :hint (str "  -> set variant-id in component to " variant-id))
+            (assoc component :variant-id variant-id)))]
 
     (log/debug :hint "repairing shape :variant-component-bad-id" :id (:id shape) :name (:name shape) :page-id page-id)
     (-> (pcb/empty-changes nil page-id)
         (pcb/with-file-data file-data)
-        (pcb/update-shapes [(:id shape)] repair-shape))))
+        (pcb/with-library-data file-data)
+        (pcb/update-shapes [(:id shape)] repair-shape)
+        (pcb/update-component (:component-id shape) repair-component))))
 
 (defmethod repair-error :missing-tokens-status
   [_ _ file-data _]

common/src/app/common/files/validate.cljc

@@ -41,6 +41,7 @@
     :duplicate-slot
     :invalid-main-instance-id
     :invalid-main-instance-page
+    :invalid-main-instance-name
     :invalid-main-instance
     :invalid-parent
     :component-main
@@ -70,6 +71,7 @@
     :invalid-variant-properties
     :variant-not-main
     :parent-not-variant
+    :variant-container-bad-name
     :variant-main-bad-name
     :variant-main-bad-variant-name
     :variant-component-bad-name
@@ -302,7 +304,14 @@
                             shape file page)
               (report-error :invalid-main-instance-page
                             (str/ffmt "Main instance page of component % is not valid" (:component-id shape))
-                            shape file page))))))))
+                            shape file page))))
+        (when-not (ctk/is-variant? component)  ;; Variants are checked elsewhere
+          (let [component-name (cpn/merge-path-item (:path component) (:name component))]
+            (when-not (= (:name shape) component-name)
+              (report-error :invalid-main-instance-name
+                            (str/ffmt "Main instance % has an invalid name" (:id shape))
+                            shape file page
+                            :component-name component-name))))))))
 
 (defn- check-component-not-main-head
   "Validate shape is a not-main instance head, component
@@ -570,14 +579,25 @@
 (defn- check-variant-container
   "Shape is a variant container, so:
      -all its children should be variants with variant-id equals to the shape-id
-     -all the components should have the same properties"
+     -all the components should have the same properties
+     -the name of the container should be normalized for paths (separator / with one space at each side)."
   [shape file page]
-  (let [shape-id    (:id shape)
-        shapes      (:shapes shape)
-        objects     (:objects page)
-        file-data   (:data file)
-        first-child (get objects (first shapes))
-        prop-names  (cfv/extract-properties-names first-child file-data)]
+  (let [shape-id       (:id shape)
+        container-name (:name shape)
+        shapes         (:shapes shape)
+        objects        (:objects page)
+        file-data      (:data file)
+        first-child    (get objects (first shapes))
+        prop-names     (cfv/extract-properties-names first-child file-data)
+        clean-path     (cpn/clean-path container-name)
+        clean-path     (if (str/blank? clean-path)
+                         "(empty)" ;; this may occur if the name contains only separators
+                         clean-path)]
+    (when-not (= container-name clean-path)
+      (report-error :variant-container-bad-name
+                    (str/ffmt "Variant container % has a bad name %" shape-id container-name)
+                    shape file page
+                    :clean-name clean-path))
     (run! (fn [child-id]
             (when-let [child (get objects child-id)]
               (if (not (ctk/is-variant? child))
@@ -598,6 +618,11 @@
                                   :prop-names prop-names))))))
           shapes)))
 
+(defn- renamed-container?
+  [container]
+  (and (some? container)
+       (not= (:name container) (cpn/clean-path (:name container)))))
+
 (defn- check-variant
   "Shape is a variant, so
      -it should be a main component
@@ -622,21 +647,23 @@
                     (str/ffmt "Variant % has an invalid variant-name" (:id shape))
                     shape file page
                     :variant-name variant-name))
-    (when-not (= (:name parent) (:name shape))
-      (report-error :variant-main-bad-name
-                    (str/ffmt "Main instance inside variant % has an invalid name" (:id shape))
-                    shape file page
-                    :variant-name (:name parent)))
-    (when-not (= (:name parent) (cpn/merge-path-item (:path component) (:name component)))
-      (report-error :variant-component-bad-name
-                    (str/ffmt "Component % has an invalid name" (:id shape))
-                    shape file page
-                    :variant-container-name (:name parent)))
+    ;; If the container is going to be renamed all main and components will be renamed, too
+    (when-not (renamed-container? parent)
+      (when-not (= (:name parent) (:name shape))
+        (report-error :variant-main-bad-name
+                      (str/ffmt "Main instance inside variant % has an invalid name" (:id shape))
+                      shape file page
+                      :variant-name (:name parent)))
+      (when-not (= (:name parent) (cpn/merge-path-item (:path component) (:name component)))
+        (report-error :variant-component-bad-name
+                      (str/ffmt "Component % has an invalid name" (:id shape))
+                      shape file page
+                      :variant-container-name (:name parent))))
     (when-not (= (:variant-id component) (:variant-id shape))
       (report-error :variant-component-bad-id
-                    (str/ffmt "Variant % has adifferent variant-id than its component" (:id shape))
+                    (str/ffmt "Variant % has a :variant-id that does not point to the container" (:id shape))
                     shape file page
-                    :variant-id (:variant-id component)))))
+                    :variant-id (:parent-id shape)))))
 
 (defn- check-shape
   "Validate referential integrity and semantic coherence of

common/src/app/common/test_helpers/variants.cljc

@@ -62,7 +62,6 @@
         (thc/make-component component2-label root2-label)
         (thc/update-component component2-label {:variant-id variant-id :variant-properties [{:name "Property 1" :value "Value2"}]}))))
 
-
 (defn add-variant-with-copy
   [file variant-label component1-label root1-label component2-label root2-label child1-label child2-label component-copy-label]
   (let [file (ths/add-sample-shape file variant-label :type :frame :is-variant-container true)
@@ -77,8 +76,6 @@
         (thc/make-component component2-label root2-label)
         (thc/update-component component2-label {:variant-id variant-id :variant-properties [{:name "Property 1" :value "Value2"}]}))))
 
-
-
 (defn add-variant-with-text
   [file variant-label component1-label root1-label component2-label root2-label child1-label child2-label text1 text2
    & {:keys [text1-params text2-params]}]

common/test/common_tests/files/repair_test.cljc

@@ -15,6 +15,7 @@
    [app.common.files.repair :as cfr]
    [app.common.files.validate :as cfv]
    [app.common.test-helpers.components :as thc]
+   [app.common.test-helpers.compositions :as tho]
    [app.common.test-helpers.files :as thf]
    [app.common.test-helpers.ids-map :as thi]
    [app.common.test-helpers.shapes :as ths]
@@ -24,6 +25,48 @@
 
 (t/use-fixtures :each thi/test-fixture)
 
+(t/deftest repair-invalid-main-instance-name
+  (t/testing "detect and repair a main instance whose name doesn't match the component's name"
+    (let [file    (-> (thf/sample-file :file1 :page-label :page1)
+                      (tho/add-simple-component-with-copy :component1 :root1 :child1 :copy1)
+                      (tho/add-simple-component-with-copy :component2 :root2 :child2 :copy2)
+
+                      ;; Update component paths and names
+                      (thc/update-component :component1 {:path "Group / Subgroup" :name "Component1"})
+                      (thc/update-component :component2 {:path "Group / Subgroup" :name "Component2"})
+
+                      ;; Update component1 shapes name to have path structure
+                      (ths/update-shape :root1 :name "Group / Subgroup / Component1")
+                      (ths/update-shape :copy1 :name "Group / Subgroup / Component1")
+
+                      ;; Update component2's name to have unnormalized path
+                      ;; Note that the copy name is allowed to be different
+                      (ths/update-shape :root2 :name "Group/Subgroup  /  Component2")
+                      (ths/update-shape :copy2 :name "Group/Subgroup  /  Component2"))
+
+          errors  (cfv/validate-file file {})
+          changes (cfr/repair-file file {} errors)
+          file'   (thf/apply-changes file {:redo-changes changes} :validate? false)
+          errors' (cfv/validate-file file' {})
+          root1'  (ths/get-shape file' :root1)
+          child1' (ths/get-shape file' :child1)
+          copy1'  (ths/get-shape file' :copy1)
+          root2'  (ths/get-shape file' :root2)
+          child2' (ths/get-shape file' :child2)
+          copy2'  (ths/get-shape file' :copy2)]
+
+      (t/is (= 1 (count errors)))
+      (t/is (= :invalid-main-instance-name (:code (first errors))))
+      (t/is (nil? errors'))
+
+      (t/is (= "Group / Subgroup / Component1" (:name root1')))
+      (t/is (= "Rect1" (:name child1')))
+      (t/is (= "Group / Subgroup / Component1" (:name copy1')))
+
+      (t/is (= "Group / Subgroup / Component2" (:name root2')))
+      (t/is (= "Rect1" (:name child2')))
+      (t/is (= "Group/Subgroup  /  Component2" (:name copy2'))))))
+
 (t/deftest repair-main-instance-not-a-variant
   (t/testing "detect and repair a variant component whose root shape is not a variant"
     (let [file    (-> (thf/sample-file :file1 :page-label :page1)
@@ -44,7 +87,7 @@
       (t/is (= (thi/id :variant1) (:variant-id root1'))))))
 
 (t/deftest repair-invalid-variant-id-variant-component-bad-id
-  (t/testing "detect and repair a variant component whose variant id does not match the container's id"
+  (t/testing "detect and repair a variant main whose variant id does not match the container's id"
     (let [file    (-> (thf/sample-file :file1 :page-label :page1)
                       (thv/add-variant :variant1 :component1 :root1 :component2 :root2)
                       (ths/update-shape :root1 :variant-id (uuid/next)))
@@ -61,7 +104,25 @@
       (t/is (= :variant-component-bad-id (:code (second errors))))
 
       (t/is (nil? errors'))
-      (t/is (= (thi/id :variant1) (:variant-id root1'))))))
+      (t/is (= (thi/id :variant1) (:variant-id root1')))))
+
+  (t/testing "detect and repair a variant component whose variant id does not match the container's id"
+    (let [file         (-> (thf/sample-file :file1 :page-label :page1)
+                           (thv/add-variant :variant1 :component1 :root1 :component2 :root2)
+                           (thc/update-component :component1 {:variant-id (uuid/next)}))
+
+          errors       (cfv/validate-file file {})
+          changes      (cfr/repair-file file {} errors)
+          file'        (thf/apply-changes file {:redo-changes changes} :validate? false)
+          errors'      (cfv/validate-file file' {})
+
+          component1'  (thc/get-component file' :component1)]
+
+      (t/is (= 1 (count errors)))
+      (t/is (= :variant-component-bad-id (:code (first errors))))
+
+      (t/is (nil? errors'))
+      (t/is (= (thi/id :variant1) (:variant-id component1'))))))
 
 (t/deftest repair-invalid-variant-properties
   (t/testing "detect and repair a second variant component whose properties do not match the first variant component's properties"
@@ -161,23 +222,60 @@
 
       (t/is (true? (:is-variant-container container'))))))
 
+(t/deftest repair-variant-container-bad-name
+  (t/testing "detect and repair a variant container who has an unnormalized name"
+    (let [file    (-> (thf/sample-file :file1 :page-label :page1)
+                      (thv/add-variant :variant1 :component1 :root1 :component2 :root2)
+                      ;; Variant container has a name that is not normalized and thus it cannot
+                      ;; match the component names, that are separated in path and group.
+                      (ths/update-shape :variant1 :name "Group  /  Subgroup/Component")
+                      (ths/update-shape :root1 :name "Group  /  Subgroup/Component")
+                      (ths/update-shape :root2 :name "Group  /  Subgroup/Component")
+                      (thc/update-component :component1 {:path "Group / Subgroup" :name "Component"})
+                      (thc/update-component :component2 {:path "Group/Subgroup" :name "Component"}))
+
+          errors  (cfv/validate-file file {})
+          changes (cfr/repair-file file {} errors)
+          file'   (thf/apply-changes file {:redo-changes changes} :validate? false)
+          errors' (cfv/validate-file file' {})
+          var1'   (ths/get-shape file' :variant1)
+          root1'  (ths/get-shape file' :root1)
+          comp1'  (thc/get-component file' :component1)
+          root2'  (ths/get-shape file' :root2)
+          comp2'  (thc/get-component file' :component2)]
+
+      (t/is (= 1 (count errors)))
+      (t/is (= :variant-container-bad-name (:code (first errors))))
+      (t/is (nil? errors'))
+      (t/is (= "Group / Subgroup / Component" (:name var1')))
+      (t/is (= "Group / Subgroup / Component" (:name root1')))
+      (t/is (= "Group / Subgroup" (:path comp1')))
+      (t/is (= "Component" (:name comp1')))
+      (t/is (= "Group / Subgroup / Component" (:name root2')))
+      (t/is (= "Group / Subgroup" (:path comp2')))
+      (t/is (= "Component" (:name comp2'))))))
+
 (t/deftest repair-variant-main-bad-name
   (t/testing "detect and repair a main instance whose name doesn't match the variant container's name"
     (let [file    (-> (thf/sample-file :file1 :page-label :page1)
                       (thv/add-variant :variant1 :component1 :root1 :component2 :root2)
-                      ;; Change root1's name so it doesn't match the container
+                      ;; Change component's name so it doesn't match the container
+                      (thc/update-component :component1 :name "WrongName")
                       (ths/update-shape :root1 :name "WrongName"))
 
           errors  (cfv/validate-file file {})
           changes (cfr/repair-file file {} errors)
           file'   (thf/apply-changes file {:redo-changes changes} :validate? false)
           errors' (cfv/validate-file file' {})
-          root1'  (ths/get-shape file' :root1)]
+          root1'  (ths/get-shape file' :root1)
+          comp1'  (thc/get-component file' :component1)]
 
-      (t/is (= 1 (count errors)))
+      (t/is (= 2 (count errors)))
       (t/is (= :variant-main-bad-name (:code (first errors))))
+      (t/is (= :variant-component-bad-name (:code (second errors))))
       (t/is (nil? errors'))
-      (t/is (= "Board" (:name root1'))))))
+      (t/is (= "Board" (:name root1')))
+      (t/is (= "Board" (:name comp1'))))))
 
 (t/deftest repair-variant-main-bad-variant-name
   (t/testing "detect and repair a variant shape whose :variant-name doesn't match the component's properties"
@@ -211,23 +309,28 @@
                       (ths/update-shape :variant1 :name "Group / Subgroup / Component")
                       (ths/update-shape :root1 :name "Group / Subgroup / Component")
                       (ths/update-shape :root2 :name "Group / Subgroup / Component")
-                      ;; Update component paths and names
                       (thc/update-component :component1 {:path "Group / Subgroup" :name "Component"})
-                      (thc/update-component :component2 {:path "Group / Subgroup" :name "Component"})
-                      ;; Break component1's name
-                      (thc/update-component :component1 {:name "WrongName"}))
+                      ;; Break component2's name
+                      (thc/update-component :component2 {:name "WrongName"}))
 
           errors  (cfv/validate-file file {})
           changes (cfr/repair-file file {} errors)
           file'   (thf/apply-changes file {:redo-changes changes} :validate? false)
           errors' (cfv/validate-file file' {})
-          comp1'  (thc/get-component file' :component1)]
+          root1'  (ths/get-shape file' :root1)
+          comp1'  (thc/get-component file' :component1)
+          root2'  (ths/get-shape file' :root2)
+          comp2'  (thc/get-component file' :component2)]
 
       (t/is (= 1 (count errors)))
       (t/is (= :variant-component-bad-name (:code (first errors))))
       (t/is (nil? errors'))
+      (t/is (= "Group / Subgroup / Component" (:name root1')))
       (t/is (= "Group / Subgroup" (:path comp1')))
-      (t/is (= "Component" (:name comp1'))))))
+      (t/is (= "Component" (:name comp1')))
+      (t/is (= "Group / Subgroup / Component" (:name root2')))
+      (t/is (= "Group / Subgroup" (:path comp2')))
+      (t/is (= "Component" (:name comp2'))))))
 
 (t/deftest repair-child-not-in-parent
   (t/testing "detect and repair shapes whose parent exists but does not list them"

common/test/common_tests/files_builder_test.cljc

@@ -70,3 +70,20 @@
     (t/is (= "Image" (:name media)))
     (t/is (= 128 (:width media)))
     (t/is (= 64 (:height media)))))
+
+(t/deftest add-shape-normalizes-path-name
+  (let [file-id (uuid/next)
+        page-id (uuid/next)
+        shape-id (uuid/next)
+        state (-> (fb/create-state)
+                  (fb/add-file {:id file-id :name "Test file"})
+                  (fb/add-page {:id page-id :name "Page 1"})
+                  (fb/add-shape {:id shape-id
+                                 :type :frame
+                                 :name "Group/Subgroup   /   Name"
+                                 :x 0
+                                 :y 0
+                                 :width 10
+                                 :height 10}))
+        shape (fb/get-shape state shape-id)]
+    (t/is (= "Group / Subgroup / Name" (:name shape)))))

common/test/common_tests/files_migrations_test.cljc

@@ -9,12 +9,20 @@
    [app.common.data :as d]
    [app.common.files.migrations :as cfm]
    [app.common.files.tokens :as cfo]
+   [app.common.test-helpers.components :as thk]
+   [app.common.test-helpers.compositions :as tho]
+   [app.common.test-helpers.files :as thf]
+   [app.common.test-helpers.ids-map :as thi]
+   [app.common.test-helpers.shapes :as ths]
+   [app.common.test-helpers.variants :as thv]
    [app.common.types.file :as ctf]
    [app.common.types.shape :as cts]
    [app.common.types.token :as ctt]
    [app.common.uuid :as uuid]
    [clojure.test :as t]))
 
+(t/use-fixtures :each thi/test-fixture)
+
 (defmethod cfm/migrate-data "test/1" [data _] (update data :sum inc))
 (defmethod cfm/migrate-data "test/2" [data _] (update data :sum inc))
 (defmethod cfm/migrate-data "test/3" [data _] (update data :sum inc))
@@ -647,3 +655,98 @@
     (t/is (contains? (:migrations file') migration-id) "migration id persisted")
     (t/is (= 4 (count (get-in file' [:data :pages-index page-id :objects shape-id :points])))
           "path points repaired before schema validation")))
+
+(t/deftest migration-0033-normalize-main-instance-diff-from-component
+  (let [migration-id "0033-normalize-main-instance-names"
+        file         (-> (thf/sample-file :file1)
+                         (tho/add-simple-component :component1 :root1 :child1
+                                                   :component-params {:path "Group / Subgroup"
+                                                                      :name "Name"}
+                                                   :root-params {:name "Group/Subgroup   /   Name"})
+                         (tho/add-simple-component :component2 :root2 :child2
+                                                   :component-params {:path "Group / Subgroup"
+                                                                      :name "Other Name"}
+                                                   :root-params {:name "Group / Subgroup / Other Name"})
+                         (ths/add-sample-shape :shape1 :name "Other Group/Other Subgroup  /  Other Name"))
+
+        data         (ctf/file-data file)
+        data'        (cfm/migrate-data data migration-id)
+        file'        (assoc file :data data')
+
+        root1'       (ths/get-shape file' :root1)
+        component1'  (thk/get-component file' :component1)
+        root2'       (ths/get-shape file' :root2)
+        component2'  (thk/get-component file' :component2)
+        shape1'      (ths/get-shape file' :shape1)]
+
+    (t/is (= "Group / Subgroup" (:path component1')))
+    (t/is (= "Name" (:name component1')))
+    (t/is (= "Group / Subgroup / Name" (:name root1')))
+    (t/is (= "Group / Subgroup" (:path component2')))
+    (t/is (= "Other Name" (:name component2')))
+    (t/is (= "Group / Subgroup / Other Name" (:name root2')))
+    (t/is (= "Other Group/Other Subgroup  /  Other Name" (:name shape1')))))
+
+(t/deftest migration-0033-normalize-variant-instance-diff-from-container
+  (let [migration-id "0033-normalize-main-instance-names"
+        file         (-> (thf/sample-file :file1)
+                         (thv/add-variant :variant1 :component1 :root1 :component2 :root2)
+                         (ths/update-shape :variant1 :name "Group / Subgroup / Component")
+                         (ths/update-shape :root1 :name "Group / Subgroup / Component")
+                         (ths/update-shape :root2 :name "Group  /  Subgroup/Component")  ;; Bad path
+                         (thk/update-component :component1 {:path "Group / Subgroup" :name "Component"})
+                         (thk/update-component :component2 {:path "Group / Subgroup" :name "Component"})
+                         (thk/instantiate-component :component2 :copy2)
+                         (ths/update-shape :copy2 :name "Group  /  Subgroup/Component"))  ;; Allowed name in copies
+
+        data         (ctf/file-data file)
+        data'        (cfm/migrate-data data migration-id)
+        file'        (assoc file :data data')
+
+        variant1'    (ths/get-shape file' :variant1)
+        root1'       (ths/get-shape file' :root1)
+        component1'  (thk/get-component file' :component1)
+        root2'       (ths/get-shape file' :root2)
+        component2'  (thk/get-component file' :component2)
+        copy2'       (ths/get-shape file' :copy2)]
+
+    (t/is (= "Group / Subgroup / Component" (:name variant1')))
+    (t/is (= "Group / Subgroup / Component" (:name root1')))
+    (t/is (= "Group / Subgroup / Component" (:name root2')))
+    (t/is (= "Group / Subgroup" (:path component1')))
+    (t/is (= "Group / Subgroup" (:path component2')))
+    (t/is (= "Component" (:name component1')))
+    (t/is (= "Component" (:name component2')))
+    (t/is (= "Group  /  Subgroup/Component" (:name copy2')))))
+
+(t/deftest migration-0033-normalize-variant-name-not-normalized
+  (let [migration-id "0033-normalize-main-instance-names"
+        file         (-> (thf/sample-file :file1)
+                         (thv/add-variant :variant1 :component1 :root1 :component2 :root2)
+                         (ths/update-shape :variant1 :name "Group/Subgroup  /  Component")
+                         (ths/update-shape :root1 :name "Group/Subgroup  /  Component")
+                         (ths/update-shape :root2 :name "Group/Subgroup  /  Component")  ;; Bad path
+                         (thk/update-component :component1 {:path "Group / Subgroup" :name "Component"})
+                         (thk/update-component :component2 {:path "Group / Subgroup" :name "Component"})
+                         (thk/instantiate-component :component2 :copy2)
+                         (ths/update-shape :copy2 :name "Group  /  Subgroup/Component"))  ;; Allowed name in copies
+
+        data         (ctf/file-data file)
+        data'        (cfm/migrate-data data migration-id)
+        file'        (assoc file :data data')
+
+        variant1'    (ths/get-shape file' :variant1)
+        root1'       (ths/get-shape file' :root1)
+        component1'  (thk/get-component file' :component1)
+        root2'       (ths/get-shape file' :root2)
+        component2'  (thk/get-component file' :component2)
+        copy2'       (ths/get-shape file' :copy2)]
+
+    (t/is (= "Group / Subgroup / Component" (:name variant1')))
+    (t/is (= "Group / Subgroup / Component" (:name root1')))
+    (t/is (= "Group / Subgroup / Component" (:name root2')))
+    (t/is (= "Group / Subgroup" (:path component1')))
+    (t/is (= "Group / Subgroup" (:path component2')))
+    (t/is (= "Component" (:name component1')))
+    (t/is (= "Component" (:name component2')))
+    (t/is (= "Group  /  Subgroup/Component" (:name copy2')))))

common/test/common_tests/logic/comp_creation_test.cljc

@@ -19,6 +19,7 @@
    [app.common.test-helpers.shapes :as ths]
    [app.common.types.component :as ctk]
    [app.common.types.components-list :as ctkl]
+   [app.common.types.file :as ctf]
    [app.common.types.shape-tree :as ctst]
    [app.common.uuid :as uuid]
    [clojure.test :as t]))
@@ -250,18 +251,25 @@
         component (thc/get-component file :component1)
 
         ;; ==== Action
-        changes   (cll/generate-rename-component (pcb/empty-changes)
-                                                 (:id component)
-                                                 "Test component after"
-                                                 (:data file))
+        changes   (-> (pcb/empty-changes nil (thf/current-page-id file))
+                      (pcb/with-file-data (ctf/file-data file))
+                      (cll/generate-rename-component
+                       (:id component)
+                       "Test component after"
+                       (:data file))
+                      (pcb/update-shapes
+                       [(thi/id :main1-root)]
+                       #(assoc % :name "Test component after")))
 
         file' (thf/apply-changes file changes)
 
         ;; ==== Get
-        component' (thc/get-component file' :component1)]
+        component'  (thc/get-component file' :component1)
+        main1-root' (ths/get-shape file' :main1-root)]
 
     ;; ==== Check
-    (t/is (= (:name component') "Test component after"))))
+    (t/is (= (:name component') "Test component after"))
+    (t/is (= (:name main1-root') "Test component after"))))
 
 (t/deftest test-duplicate-component
   (let [;; ==== Setup