From 4a9a2897b71759e97d7435f208985fc16d9140ad Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Fri, 9 Oct 2026 10:12:26 +0200 Subject: [PATCH] :bug: Hold the sync wait for every local commit The component watcher opened its :sync-file wait only for commits saved to the undo stack. Undo and redo commit without it, so a wait such as the plugin waitForLayoutUpdate returned before the watcher had handled them. Translation and derived commits were filtered out before the wait opened. Open the wait for every local commit of a file and close it once the watcher has decided what to do: sync, only touch the component, or drop the commit. Dropped commits close it right away. Add wrf/pending, a snapshot of the pending work, for tests and diagnostics. --- .../app/main/data/workspace/libraries.cljs | 32 ++++-- .../src/app/main/data/workspace/reflow.cljs | 5 + .../composable_tests/interpreter.cljs | 28 ++--- .../data/workspace_sync_barrier_test.cljs | 101 ++++++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 5 files changed, 147 insertions(+), 21 deletions(-) create mode 100644 frontend/test/frontend_tests/data/workspace_sync_barrier_test.cljs diff --git a/frontend/src/app/main/data/workspace/libraries.cljs b/frontend/src/app/main/data/workspace/libraries.cljs index b9554c4aa4..7e2422844f 100644 --- a/frontend/src/app/main/data/workspace/libraries.cljs +++ b/frontend/src/app/main/data/workspace/libraries.cljs @@ -1401,11 +1401,13 @@ (rx/map first)) ;; Barriers open before async inspection and close after detection. + ;; Every local commit opens one, so a wait covers the watcher's + ;; decision for any commit: sync, only touch, or drop. pending-sync-barriers* (atom #{}) start-sync-barrier - (fn [{:keys [file-id save-undo?] :as event}] - (let [task (when (and save-undo? (uuid? file-id)) + (fn [{:keys [file-id] :as event}] + (let [task (when (uuid? file-id) (wrf/start! :sync-file [file-id]))] (when task (swap! pending-sync-barriers* conj task)) @@ -1423,18 +1425,31 @@ (doseq [[[_ task] _] batch] (finish-sync-barrier! task))) - commits-s + local-commits-s (->> stream (rx/filter dch/commit?) (rx/map deref) (rx/filter #(= :local (:source %))) - ;; Translation commits never propagate component changes. - (rx/filter (complement :translation?)) - ;; Derived / corrective commits (font-load selrect fix, - ;; position-data regen) are not user component edits. - (rx/filter (complement :skip-component-sync?)) ;; Keep waits pending while component changes are checked. (rx/map start-sync-barrier) + (rx/share)) + + ;; Translation commits never propagate component changes. Derived / + ;; corrective commits (font-load selrect fix, position-data regen) + ;; are not user component edits. + dropped-commit? + (fn [[{:keys [translation? skip-component-sync?]} _]] + (or translation? skip-component-sync?)) + + dropped-commits-s + (->> local-commits-s + (rx/filter dropped-commit?) + (rx/tap (comp finish-sync-barrier! second)) + (rx/ignore)) + + commits-s + (->> local-commits-s + (rx/filter (complement dropped-commit?)) ;; 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) @@ -1502,6 +1517,7 @@ (when (or (contains? cf/flags :component-thumbnails) (features/active-feature? state "render-wasm/v1")) (->> (rx/merge + dropped-commits-s component-events-s ;; WASM only: render the thumbnail on every component diff --git a/frontend/src/app/main/data/workspace/reflow.cljs b/frontend/src/app/main/data/workspace/reflow.cljs index b6326d0e14..18f1deffd2 100644 --- a/frontend/src/app/main/data/workspace/reflow.cljs +++ b/frontend/src/app/main/data/workspace/reflow.cljs @@ -229,6 +229,11 @@ (finish-tasks! (map tasks-by-id @remaining)) (reset! remaining #{}))))))))) +(defn pending + "The work pending right now, as `{id -> {kind -> #{task-id}}}`." + [] + (.getValue ^js pending-shapes)) + (defn settled "Observable that emits once every id in `ids` has drained from the pending map, then completes. A nil `ids` waits for every pending id; an empty one has diff --git a/frontend/test/frontend_tests/composable_tests/interpreter.cljs b/frontend/test/frontend_tests/composable_tests/interpreter.cljs index 4c1ea73af8..92f92f4edd 100644 --- a/frontend/test/frontend_tests/composable_tests/interpreter.cljs +++ b/frontend/test/frontend_tests/composable_tests/interpreter.cljs @@ -245,7 +245,7 @@ (map (fn [[lib-id lib]] [lib-id (assoc lib :library-of (:id file))])) aux)))))) -(defn- current-file +(defn current-file "Read the live workspace file out of the global store." [] (let [st @st/state] @@ -380,23 +380,25 @@ (set! st/state (:state original-store)) (set! st/stream (:stream original-store))) +(defn install! + "Install `situation` in the global store and start the watchers the workspace + starts, replacing the previous variant's. Restores the global store first (a + preceding namespace may have swapped it). Returns the situation." + [situation] + (restore-global-store!) + (st/emit! (install-situation-event situation) + (dw/finalize-edit-watchers) + (dw/initialize-edit-watchers) + (dwl/watch-library-changes)) + situation) + (defn- run-variant "Set up one variant on the global store and run its ops. `setup` returns a - situation (file + roles). Restores the global store (a preceding namespace may - have swapped it), installs the file + starts the watcher, then folds the ops. - Calls `k` with the final situation." + situation (file + roles). Calls `k` with the final situation." [setup ops k] ;; fresh label space per variant (mirrors the pure `tm/run-all`) (cthi/reset-idmap!) - (restore-global-store!) - (let [situation (setup)] - (st/emit! (install-situation-event situation)) - ;; The same watchers the workspace starts; re-emitting them replaces the - ;; previous variant's. - (st/emit! (dw/finalize-edit-watchers) - (dw/initialize-edit-watchers) - (dwl/watch-library-changes)) - (run-ops situation ops k))) + (run-ops (install! (setup)) ops k)) ;; -------------------------------------------------------------------------- ;; (3) Test-facing check diff --git a/frontend/test/frontend_tests/data/workspace_sync_barrier_test.cljs b/frontend/test/frontend_tests/data/workspace_sync_barrier_test.cljs new file mode 100644 index 0000000000..a610a5b32d --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_sync_barrier_test.cljs @@ -0,0 +1,101 @@ +;; 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.data.workspace-sync-barrier-test + "Tests the `:sync-file` wait the component watcher holds for each local + commit until it has decided what to do with it: sync, only touch the + component, or drop the commit. Plugin `waitForLayoutUpdate` and the + composable interpreter rely on it." + (:require + [app.common.files.changes-builder :as pcb] + [app.common.logic.shapes :as cls] + [app.common.test-helpers.composable.comp.setups :as setup] + [app.common.test-helpers.files :as thf] + [app.common.test-helpers.ids-map :as thi] + [app.common.test-helpers.shapes :as ths] + [app.main.data.changes :as dch] + [app.main.data.workspace.reflow :as wrf] + [app.main.store :as st] + [app.plugins.reflow :as pwrf] + [cljs.test :as t :include-macros true] + [frontend-tests.composable-tests.interpreter :as ftm])) + +;; The watcher schedules thumbnail renders that reach `window`, absent +;; headless (see `ftm/install-thumbnail-noop!`). +(t/use-fixtures :each + {:before (fn [] + (thi/reset-idmap!) + (ftm/install-thumbnail-noop!)) + :after ftm/restore-thumbnail!}) + +(def ^:private red "#ff0000") + +(defn- main-child-fill-commit + "A commit that sets the main child's fill to red, with `opts` merged into the + commit params." + [opts] + (let [file (ftm/current-file) + page (thf/current-page file) + changes (cls/generate-update-shapes + (pcb/empty-changes nil (:id page)) + #{(thi/id :main-child)} + #(assoc % :fills (ths/sample-fills-color :fill-color red)) + (:objects page) + {})] + (dch/commit-changes (merge {:redo-changes (:redo-changes changes) + :undo-changes (:undo-changes changes)} + opts)))) + +(defn- sync-pending? + [file-id] + (contains? (get (wrf/pending) file-id) :sync-file)) + +(defn- fill-of + [label] + (-> (ths/get-shape (ftm/current-file) label) :fills first :fill-color)) + +(defn- run-commit + "Install a component with a copy and emit the main child fill commit with + `commit-opts`. Once the wait settles, call `check` with whether the sync + wait was open right after the commit. Fails the test when the wait does not + settle." + [done commit-opts check] + (ftm/install! (setup/simple-component-with-labeled-copy)) + (let [file-id (:id (ftm/current-file))] + (st/emit! (main-child-fill-commit commit-opts)) + (let [open? (sync-pending? file-id)] + (-> (pwrf/wait-for-layout-update nil 2000) + (.then #(check open?) + #(t/is false "the sync wait did not settle")) + (.catch #(t/is false (str "the check threw: " %))) + (.finally done))))) + +(t/deftest user-edit-holds-the-wait-until-copies-sync + (t/async done + (run-commit done {} + (fn [open?] + (t/is open?) + (t/is (= red (fill-of :copy-child))))))) + +(t/deftest undo-commit-holds-the-wait-until-the-watcher-handles-it + ;; Undo and redo commit with `save-undo? false`. The watcher only touches + ;; the component for them, after a timer turn; the wait must cover that. + (t/async done + (run-commit done {:save-undo? false} + (fn [open?] + (t/is open?))))) + +(t/deftest translation-commit-leaves-nothing-pending + (t/async done + (run-commit done {:translation? true} + (fn [open?] + (t/is (not open?)))))) + +(t/deftest derived-commit-leaves-nothing-pending + (t/async done + (run-commit done {:skip-component-sync? true} + (fn [open?] + (t/is (not open?)))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index e23f915520..80b98597a3 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -37,6 +37,7 @@ [frontend-tests.data.workspace-path-edition-test] [frontend-tests.data.workspace-reflow-test] [frontend-tests.data.workspace-shortcuts-test] + [frontend-tests.data.workspace-sync-barrier-test] [frontend-tests.data.workspace-texts-test] [frontend-tests.data.workspace-thumbnails-test] [frontend-tests.data.workspace-variant-rename-test] @@ -204,6 +205,7 @@ 'frontend-tests.data.workspace-pages-test 'frontend-tests.data.workspace-path-edition-test 'frontend-tests.data.workspace-reflow-test + 'frontend-tests.data.workspace-sync-barrier-test 'frontend-tests.data.workspace-shortcuts-test 'frontend-tests.data.workspace-texts-test 'frontend-tests.data.workspace-thumbnails-test