🐛 Fix issue with compononent not sync on move (#11981)

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

@@ -1462,6 +1462,12 @@
                 (wrf/finish! task)
                 (swap! pending-sync-barriers* disj task)))
 
+            ;; Each batch entry is [[event task] old-data].
+            finish-batch-barriers!
+            (fn [batch]
+              (doseq [[[_ task] _] batch]
+                (finish-sync-barrier! task)))
+
             commits-s
             (->> stream
                  (rx/filter dch/commit?)
@@ -1474,6 +1480,9 @@
                  (rx/filter (complement :skip-component-sync?))
                  ;; Keep waits pending while component changes are checked.
                  (rx/map start-sync-barrier)
+                 ;; Paired as it arrives, so later commits in the same batch
+                 ;; cannot hide the shapes this one moved or deleted.
+                 (rx/with-latest-from workspace-buffer-s)
                  (rx/share))
 
             ;; Buffers commits until a timer turn passes with no new ones. The
@@ -1499,32 +1508,33 @@
                     (log/info :hint "detected component changes"
                               :ids (map str changed-components)
                               :undo-group undo-group)
-                    (map #(vector ::component-changed % undo-group)
+                    (map #(vector ::component-changed % (:id old-data) undo-group)
                          changed-components))
 
                   :else
                   ;; Undos only bump :modified-at.
                   (map #(vector ::touch-component %) changed-components))))
 
-            ;; One event per distinct component change in the batch
+            ;; One event per distinct component change in the batch, each
+            ;; commit checked against the file data right before it
             get-component-events
-            (fn [batch old-data]
+            (fn [batch]
               (->> batch
-                   (mapcat #(get-component-changes old-data (first %)))
+                   (mapcat (fn [[[event _] old-data]]
+                             (get-component-changes old-data event)))
                    (distinct)
-                   (map (fn [[type component-id undo-group]]
+                   (map (fn [[type component-id file-id undo-group]]
                           (if (= type ::component-changed)
-                            (component-changed component-id (:id old-data) undo-group)
+                            (component-changed component-id file-id undo-group)
                             (touch-component component-id))))
                    (rx/from)))
 
             component-events-s
             (->> commit-batches-s
-                 (rx/with-latest-from workspace-buffer-s)
                  (rx/mapcat
-                  (fn [[batch old-data]]
-                    (->> (get-component-events batch old-data)
-                         (rx/finalize #(run! (comp finish-sync-barrier! second) batch)))))
+                  (fn [batch]
+                    (->> (get-component-events batch)
+                         (rx/finalize #(finish-batch-barriers! batch)))))
                  ;; Close barriers left behind when the page shuts down.
                  (rx/finalize #(wrf/finish-tasks! @pending-sync-barriers*))
                  (rx/share))

frontend/test/frontend_tests/logic/comp_watcher_test.cljs

@@ -0,0 +1,100 @@
+;; 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.logic.comp-watcher-test
+  "Automatic propagation of main edits to copies, driven by the real
+  `dwl/watch-component-changes` on an isolated store."
+  (:require
+   [app.common.test-helpers.compositions :as ctho]
+   [app.common.test-helpers.files :as cthf]
+   [app.common.test-helpers.ids-map :as cthi]
+   [app.common.test-helpers.shapes :as cths]
+   [app.main.data.helpers :as dsh]
+   [app.main.data.workspace.libraries :as dwl]
+   [app.main.data.workspace.pages :as dwpg]
+   [app.main.data.workspace.shapes :as dwsh]
+   [app.main.data.workspace.thumbnails-wasm :as dwt.wasm]
+   [app.main.refs :as refs]
+   [app.main.store :as st]
+   [beicon.v2.core :as rx]
+   [cljs.test :as t :include-macros true]
+   [frontend-tests.helpers.async :as async]
+   [frontend-tests.helpers.mock :as mock]
+   [frontend-tests.helpers.pages :as thp]
+   [frontend-tests.helpers.state :as ths]
+   [frontend-tests.helpers.wasm :as thw]
+   [okulary.core :as l]
+   [potok.v2.core :as ptk]))
+
+(t/use-fixtures :each
+  {:before (fn []
+             (thp/reset-idmap!)
+             (thw/setup-wasm-mocks!))
+   :after  thw/teardown-wasm-mocks!})
+
+(defn- setup-file
+  []
+  ;; {:main-root} [:name Frame1]       # [Component :component1]
+  ;;     :main-child [:name Rect1]
+  ;;
+  ;; :copy-root [:name Frame1]         #--> [Component :component1] :main-root
+  ;;     :copy-child [:name Rect1]     ---> :main-child
+  ;;
+  ;; :outside [:name Frame1]
+  (-> (cthf/sample-file :file1)
+      (ctho/add-simple-component-with-copy :component1
+                                           :main-root
+                                           :main-child
+                                           :copy-root
+                                           :copy-root-params {:children-labels [:copy-child]})
+      (ctho/add-frame :outside)))
+
+(defn- noop-render-thumbnail
+  [& _]
+  (ptk/reify ::noop-render-thumbnail
+    ptk/WatchEvent
+    (watch [_ _ _] (rx/empty))))
+
+(defn- copy-children
+  [store]
+  (let [file (ths/get-file-from-state @store)]
+    (:shapes (cths/get-shape file :copy-root))))
+
+(t/deftest ^:async child-moved-out-of-main-leaves-copy-when-another-edit-follows
+  ;; The move and the fill change commit in the same tick, so the watcher
+  ;; inspects both in one batch. The move must be checked against the file
+  ;; data right before it, where the rect still sits in the main.
+  (let [file  (setup-file)
+        store (ths/setup-store file)]
+    (await
+     (mock/with-mocks*
+       {;; The watcher reads the file data through this lens.
+        refs/workspace-data      (l/derived dsh/lookup-file-data store)
+        ;; Thumbnail renders read the global store.
+        st/state                 store
+        dwt.wasm/render-thumbnail noop-render-thumbnail}
+
+       (ptk/emit! store (dwl/watch-component-changes))
+       (t/is (= 1 (count (copy-children store)))
+             "the copy starts with its rect")
+
+       (ptk/emit! store
+                  (dwsh/relocate-shapes #{(cthi/id :main-child)} (cthi/id :outside) 0)
+                  (dwsh/update-shapes [(cthi/id :outside)]
+                                      #(assoc % :fills [{:fill-color "#ff0000"
+                                                         :fill-opacity 1}])))
+
+       (await (async/wait-for #(empty? (copy-children store))
+                              "the copy to lose its rect"))
+
+       (let [file' (ths/get-file-from-state @store)]
+         (t/is (= (cthi/id :outside)
+                  (:parent-id (cths/get-shape file' :main-child)))
+               "the rect moved out of the main")
+         (t/is (empty? (copy-children store))
+               "the copy lost its rect"))
+
+       (ptk/emit! store (dwpg/finalize-page (:id file) (cthf/current-page-id file)))
+       (await (async/settle))))))

frontend/test/frontend_tests/runner.cljs

@@ -44,6 +44,7 @@
    [frontend-tests.fonts-test]
    [frontend-tests.helpers-shapes-test]
    [frontend-tests.logic.comp-remove-swap-slots-test]
+   [frontend-tests.logic.comp-watcher-test]
    [frontend-tests.logic.components-and-tokens]
    [frontend-tests.logic.copy-paste-typography-test]
    [frontend-tests.logic.copying-and-duplicating-test]
@@ -192,6 +193,7 @@
    'frontend-tests.fonts-test
    'frontend-tests.helpers-shapes-test
    'frontend-tests.logic.comp-remove-swap-slots-test
+   'frontend-tests.logic.comp-watcher-test
    'frontend-tests.logic.components-and-tokens
    'frontend-tests.logic.copy-paste-typography-test
    'frontend-tests.logic.copying-and-duplicating-test

plugins/apps/composable-test-suite/src/composable-tests/cases.ts

@@ -5,6 +5,7 @@ import { createTestCaseRemoteMainCopySyncNested } from "./cases/caseRemoteMainCo
 import { createTestCaseVariantSwitchPropagates } from "./cases/caseVariantSwitchPropagates.ts";
 import { createTestCaseCopySubheadDeletePreservesSlots } from "./cases/caseCopySubheadDeletePreservesSlots.ts";
 import { createTestCaseMainReorderKeepsCopySlots } from "./cases/caseMainReorderKeepsCopySlots.ts";
+import { createTestCaseMainChildMovedOutLeavesCopy } from "./cases/caseMainChildMovedOutLeavesCopy.ts";
 
 /**
  * All composable test cases currently defined. A factory (not a constant): each
@@ -19,5 +20,6 @@ export function allCases(): readonly TestCase[] {
         createTestCaseVariantSwitchPropagates(),
         createTestCaseCopySubheadDeletePreservesSlots(),
         createTestCaseMainReorderKeepsCopySlots(),
+        createTestCaseMainChildMovedOutLeavesCopy(),
     ];
 }

plugins/apps/composable-test-suite/src/composable-tests/cases/caseMainChildMovedOutLeavesCopy.ts

@@ -0,0 +1,59 @@
+import { Board } from "@penpot/plugin-types";
+import { TestCase } from "../test-suite/TestCase.ts";
+import { Color } from "../model/Color";
+import { ShapePropFillColor } from "../model/ShapeProp.ts";
+import { Assert } from "../util/Assert.ts";
+import { OpAssert } from "../operations/OpAssert";
+import { OpSequence } from "../operations/OpSequence.ts";
+import { OpOptional } from "../operations/OpOptional.ts";
+import { OpChangeProperty } from "../operations/OpChangeProperty";
+import { OpCreateBoard } from "../operations/OpCreateBoard";
+import { OpCreateSimpleComponentWithCopy } from "../operations/OpCreateSimpleComponentWithCopy";
+import { OpMoveShape } from "../operations/OpMoveShape";
+
+const BASELINE = new Color("#aaaaaa");
+const EDIT_COLOR = new Color("#ff0000");
+
+/**
+ * A shape moved out of a main leaves its copies, also when another edit lands
+ * right after the move, before propagation has run.
+ *
+ * The baseline assertion waits for the setup to settle; the setup's own commits
+ * would otherwise sync the copy in the same pass and hide the result. The move
+ * and the optional edit then run back to back, so their commits arrive together;
+ * without the edit, the move is the last commit.
+ */
+export function createTestCaseMainChildMovedOutLeavesCopy(): TestCase {
+    const foundation = new OpCreateSimpleComponentWithCopy(BASELINE);
+    const { mainChild, copyRoot } = foundation.roles;
+    const outside = new OpCreateBoard("Outside");
+    const outsideBoard = outside.roles.board;
+
+    const move = new OpMoveShape(mainChild, outsideBoard, "main rect", "outside board");
+
+    return new TestCase(
+        "MainChildMovedOutLeavesCopy",
+        "A component containing a single rectangle is created, plus a copy of it and a board " +
+            "outside the component. The main's rectangle is moved into that board, optionally " +
+            "followed at once by a fill change on the board. The copy must lose its rectangle " +
+            "either way.",
+        new OpSequence(
+            foundation,
+            outside,
+            new OpAssert("the copy starts with its rectangle", (s) => {
+                const copyChildren = (s.get(copyRoot) as Board).children;
+                Assert.that(copyChildren.length === 1, `the copy should have one child, found ${copyChildren.length}`);
+            }),
+            move,
+            new OpOptional(new OpChangeProperty(outsideBoard, new ShapePropFillColor(), EDIT_COLOR, "outside board")),
+            new OpAssert("the rectangle left the main and the copy", (s) => {
+                move.assertHasMoved(s, mainChild);
+                const copyChildren = (s.get(copyRoot) as Board).children;
+                Assert.that(
+                    copyChildren.length === 0,
+                    `the copy should have no children, found ${copyChildren.map((c) => c.name).join(", ")}`
+                );
+            })
+        )
+    );
+}

plugins/apps/composable-test-suite/src/composable-tests/operations/OpCreateBoard.ts

@@ -0,0 +1,39 @@
+import { Board } from "@penpot/plugin-types";
+import { Operation } from "../core/Operation";
+import { Role } from "../core/Role";
+import { RoleBundle } from "../core/RoleBundle";
+import { Situation } from "../core/Situation";
+
+/** The roles exposed by {@link OpCreateBoard}. */
+class RolesBoard extends RoleBundle {
+    /** The free board the operation created. */
+    readonly board = new Role<Board>("board");
+}
+
+/**
+ * Creates a free board on the page, outside any component, and binds it to its
+ * `board` role. Gives a case somewhere to move shapes to, or to edit, apart from
+ * the configuration under test.
+ */
+export class OpCreateBoard extends Operation {
+    readonly roles = new RolesBoard();
+
+    /**
+     * @param boardName - the name given to the new board
+     */
+    constructor(private readonly boardName: string) {
+        super();
+    }
+
+    async applyTo(situation: Situation): Promise<void> {
+        const board = penpot.createBoard();
+        board.name = this.boardName;
+        board.resize(100, 100);
+        situation.applyPosAdvanceX(board);
+        situation.bind(this.roles.board, board);
+    }
+
+    toString(): string {
+        return `create board ${this.boardName}`;
+    }
+}

plugins/apps/composable-test-suite/src/composable-tests/operations/OpMoveShape.ts

@@ -0,0 +1,53 @@
+import { Board } from "@penpot/plugin-types";
+import { Operation } from "../core/Operation";
+import { Situation } from "../core/Situation";
+import { ShapeTarget, resolveTarget } from "../core/ShapeTarget";
+import { Assert } from "../util/Assert";
+
+/**
+ * Moves the shape a target resolves to into another parent, via the Plugin API's
+ * `appendChild`. Used to move a shape out of the main of a component, which must
+ * remove the corresponding shape from its copies.
+ */
+export class OpMoveShape extends Operation {
+    /**
+     * @param target - resolves the shape to move (role or situation-derived)
+     * @param newParent - resolves the board that receives the shape
+     * @param targetLabel - a human-readable name for the moved shape, for the log
+     * @param parentLabel - a human-readable name for the new parent, for the log
+     */
+    constructor(
+        private readonly target: ShapeTarget,
+        private readonly newParent: ShapeTarget,
+        private readonly targetLabel: string = String(target),
+        private readonly parentLabel: string = String(newParent)
+    ) {
+        super();
+    }
+
+    async applyTo(situation: Situation): Promise<void> {
+        const shape = resolveTarget(this.target, situation);
+        const parent = resolveTarget(this.newParent, situation) as Board;
+        parent.appendChild(shape);
+    }
+
+    /**
+     * Asserts that the shape `target` resolves to now sits in this operation's
+     * new parent.
+     *
+     * @param situation - the situation to resolve the targets against
+     * @param target - resolves the shape expected to have moved
+     */
+    assertHasMoved(situation: Situation, target: ShapeTarget): void {
+        const shape = resolveTarget(target, situation);
+        const parent = resolveTarget(this.newParent, situation);
+        Assert.that(
+            shape.parent?.id === parent.id,
+            `expected "${shape.name}" inside "${parent.name}" but its parent is "${shape.parent?.name}"`
+        );
+    }
+
+    toString(): string {
+        return `move ${this.targetLabel} into ${this.parentLabel}`;
+    }
+}