🐛 Preserve component order when combining components as variants (#10562)

frontend/src/app/main/data/workspace/variants.cljs

@@ -612,7 +612,37 @@
        (map first)
        vec))
 
+(defn- order-variant-children
+  "Reorders the children of a variant container so that the variant components
+   (as returned by cfv/find-variant-components, which reverses the children
+   vector) appear in the given order of ids. Ids that are not children of the
+   container are ignored; children not covered by ids keep their position at
+   the end."
+  [variant-id ordered-ids]
+  (ptk/reify ::order-variant-children
+    ptk/WatchEvent
+    (watch [it state _]
+      (let [page-id   (:current-page-id state)
+            objects   (dsh/lookup-page-objects state page-id)
+            children  (set (dm/get-in objects [variant-id :shapes]))
+            ;; The desired :shapes vector is the reverse of ordered-ids;
+            ;; change-parent at index 0 inverts the order of the given shapes,
+            ;; so they are passed in ordered-ids order
+            shapes    (->> ordered-ids
+                           (filter children)
+                           (mapv (d/getf objects)))]
+        (when (> (count shapes) 1)
+          (let [changes (-> (pcb/empty-changes it page-id)
+                            (pcb/with-objects objects)
+                            (pcb/change-parent variant-id shapes 0))]
+            (rx/of (dch/commit-changes changes)
+                   (ptk/data-event :layout/update {:ids [variant-id]}))))))))
+
 (defn combine-as-variants
+  "Combines the components identified by the main-instance ids `ids` into a
+   new variant container. If `ids` is a sequential collection, its order
+   determines the order of the resulting variant components; unordered
+   collections are normalized to the layer-tree order."
   [ids {:keys [page-id trigger variant-id]}]
   (ptk/reify ::combine-as-variants
     ptk/WatchEvent
@@ -622,12 +652,15 @@
             combine
             (fn [current-page]
               (let [objects       (dsh/lookup-page-objects state current-page)
-                    ids           (->> ids
+                    ids           (->> (if (sequential? ids)
+                                         ids
+                                         (cfh/order-by-indexed-shapes objects ids))
                                        (cfh/clean-loops objects)
                                        (remove (fn [id]
                                                  (let [shape (get objects id)]
                                                    (or (not (ctc/main-instance? shape))
-                                                       (ctc/is-variant? shape))))))]
+                                                       (ctc/is-variant? shape)))))
+                                       (vec))]
                 (when (> (count ids) 1)
                   (let [shapes        (mapv #(get objects %) ids)
                         rect          (bounding-rect shapes)
@@ -657,7 +690,10 @@
 
                      (rx/of (dwu/start-undo-transaction undo-id)
                             (transform-in-variant (first ids) variant-id delta prefix add-wrapper? false false)
-                            (dwsh/relocate-shapes (into #{} (-> ids rest reverse)) variant-id 0)
+                            (dwsh/relocate-shapes (into #{} (rest ids)) variant-id 0)
+                            ;; relocate-shapes inserts the shapes in layer-tree
+                            ;; order, so restore the intended order afterwards
+                            (order-variant-children variant-id ids)
                             (dwsh/update-shapes ids #(-> %
                                                          (assoc :constraints-h :left)
                                                          (assoc :constraints-v :top)

frontend/src/app/plugins/api.cljs

@@ -687,9 +687,12 @@
         :else
         (let [file-id (obj/get (first shapes) "$file")
               page-id (obj/get (first shapes) "$page")
+              ;; Keep the input order: it determines the order of the
+              ;; resulting variant components (see combine-as-variants)
               ids (->> shapes
                        (map #(obj/get % "$id"))
-                       (into #{}))
+                       (distinct)
+                       (vec))
 
               ;; Check that every component is:
               ;; - in the same page

frontend/src/app/plugins/shape.cljs

@@ -1644,8 +1644,15 @@
                (u/not-valid plugin-id :ids ids)
 
                :else
-               (let [ids
+               (let [;; Keep the input order (head shape first): it determines
-                     (into #{id} (keep uuid/parse*) ids)
+                     ;; the order of the resulting variant components (see
+                     ;; combine-as-variants)
+                     ids
+                     (into [id]
+                           (comp (keep uuid/parse*)
+                                 (remove #{id})
+                                 (distinct))
+                           ids)
 
                      valid?
                      (every?

frontend/test/frontend_tests/plugins/shape_bugfixes_test.cljs

@@ -161,12 +161,13 @@
   ;; `combine-as-variants` needs real main components and the variant pipeline,
   ;; so this stays at the proxy boundary and verifies the component ids that
   ;; the head proxy collects from its argument before delegating.
-  (let [file-id  (uuid/next)
+  (let [file-id   (uuid/next)
-        page-id  (uuid/next)
+        page-id   (uuid/next)
-        head-id  (uuid/next)
+        head-id   (uuid/next)
-        other-id (uuid/next)
+        other-id  (uuid/next)
-        proxy    (shape/shape-proxy plugin-id file-id page-id head-id)
+        third-id  (uuid/next)
-        captured (atom nil)]
+        proxy     (shape/shape-proxy plugin-id file-id page-id head-id)
+        captured  (atom nil)]
     (with-redefs [u/locate-shape (fn [_file _page id] {:id id :component-id id})
                   u/locate-library-component (constantly {:id (uuid/next)})
                   ctk/is-variant? (constantly false)
@@ -178,8 +179,11 @@
                     {:event :combine-as-variants})
                   st/emit! mock/noop
                   shape/shape-proxy (mock/stub (fn [& _] #js {}))]
-      (.combineAsVariants proxy #js [(str other-id)])
+      ;; the collected ids must be ordered (head shape first, then the passed
-      (t/is (= #{head-id other-id} (:ids @captured))))))
+      ;; ids in their given order, deduplicated), since the order determines
+      ;; the order of the resulting variant components
+      (.combineAsVariants proxy #js [(str other-id) (str head-id) (str third-id) (str other-id)])
+      (t/is (= [head-id other-id third-id] (:ids @captured))))))
 
 (t/deftest remove-ruler-guide-deletes-the-guide-from-the-page
   ;; Adds a real ruler guide through the API and asserts it is gone from the