From a70712fce44589b6bffeefaba75437e737895a89 Mon Sep 17 00:00:00 2001 From: Alonso Torres Date: Tue, 27 Oct 2026 15:44:30 +0100 Subject: [PATCH] :bug: Fix issue with compononent not sync on move (#11981) --- .../app/main/data/workspace/libraries.cljs | 30 ++++-- .../logic/comp_watcher_test.cljs | 100 ++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + .../src/composable-tests/cases.ts | 2 + .../cases/caseMainChildMovedOutLeavesCopy.ts | 59 +++++++++++ .../operations/OpCreateBoard.ts | 39 +++++++ .../operations/OpMoveShape.ts | 53 ++++++++++ 7 files changed, 275 insertions(+), 10 deletions(-) create mode 100644 frontend/test/frontend_tests/logic/comp_watcher_test.cljs create mode 100644 plugins/apps/composable-test-suite/src/composable-tests/cases/caseMainChildMovedOutLeavesCopy.ts create mode 100644 plugins/apps/composable-test-suite/src/composable-tests/operations/OpCreateBoard.ts create mode 100644 plugins/apps/composable-test-suite/src/composable-tests/operations/OpMoveShape.ts diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index b84faf4356..3044a05f6c 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/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)) diff --git a/frontend/test/frontend_tests/logic/comp_watcher_test.cljs b/frontend/test/frontend_tests/logic/comp_watcher_test.cljs new file mode 100644 index 0000000000..f8d3bc8783 --- /dev/null +++ b/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)))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index 1471547c4d..aefb617e23 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/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 diff --git a/plugins/apps/composable-test-suite/src/composable-tests/cases.ts b/plugins/apps/composable-test-suite/src/composable-tests/cases.ts index e9150ec16c..793903e15d 100644 --- a/plugins/apps/composable-test-suite/src/composable-tests/cases.ts +++ b/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(), ]; } diff --git a/plugins/apps/composable-test-suite/src/composable-tests/cases/caseMainChildMovedOutLeavesCopy.ts b/plugins/apps/composable-test-suite/src/composable-tests/cases/caseMainChildMovedOutLeavesCopy.ts new file mode 100644 index 0000000000..04df9632fd --- /dev/null +++ b/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(", ")}` + ); + }) + ) + ); +} diff --git a/plugins/apps/composable-test-suite/src/composable-tests/operations/OpCreateBoard.ts b/plugins/apps/composable-test-suite/src/composable-tests/operations/OpCreateBoard.ts new file mode 100644 index 0000000000..eeb7bb64ff --- /dev/null +++ b/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"); +} + +/** + * 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 { + 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}`; + } +} diff --git a/plugins/apps/composable-test-suite/src/composable-tests/operations/OpMoveShape.ts b/plugins/apps/composable-test-suite/src/composable-tests/operations/OpMoveShape.ts new file mode 100644 index 0000000000..6795dc121b --- /dev/null +++ b/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 { + 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}`; + } +}