From ef3f146ca0ef19e5feab3df59e35e51624212e2c Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Tue, 20 Oct 2026 16:44:33 +0200 Subject: [PATCH] :bug: Revert whole user actions in a rollback A rollback reverts whole undo groups, across files and saves, so that no user action is left half done. Some actions still split: - An action that commits several times inside one undo transaction (e.g. applying a token to several shapes) gave each commit its own group, so a rejection could revert part of it: shapes ended up linked to a token whose value was never applied. An undo transaction now opens with a commit group that every commit made inside it takes. - The commit that undoes saved commits of a rejected action joined an open transaction's group, so a later rejection there reverted the undo too. It gets a group of its own. - Emptying the open transaction during a rollback dropped its group, so its later commits were no longer one unit. It keeps its group. - Waiting for a save ended as soon as the queue emptied, while later commits could still wait in the buffer. Waiters now also wait for an empty buffer. A test covers a token action of three commits in one save whose third commit is rejected. --- .../workspace-state-persistence-subtleties.md | 2 +- frontend/src/app/main/data/changes.cljs | 11 +- frontend/src/app/main/data/persistence.cljs | 13 +- .../src/app/main/data/workspace/undo.cljs | 14 +- .../frontend_tests/data/persistence_test.cljs | 144 +++++++++++++++++- .../data/workspace_undo_test.cljs | 113 ++++++++++++++ frontend/test/frontend_tests/runner.cljs | 2 + 7 files changed, 285 insertions(+), 14 deletions(-) create mode 100644 frontend/test/frontend_tests/data/workspace_undo_test.cljs diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index 40ba0f333c..b16875237f 100644 --- a/.serena/memories/frontend/workspace-state-persistence-subtleties.md +++ b/.serena/memories/frontend/workspace-state-persistence-subtleties.md @@ -28,7 +28,7 @@ - A terminal failure halts the queue on purpose: the edits stay queued so `wait-persisted-or-error` keeps rejecting, and `workspace/versions.cljs` relies on that to avoid snapshotting a file whose edits were never saved. `recover-persistence` lifts the halt when a file is opened again or the role changes. A transport failure still inside its retry window (`slow-retry-pending?`) is resent once (`resume-persistence true`), never released; any other halt emits `release-failed-file` (drops every queued commit of the file that failed) then `resume-persistence`. Opening a file also replays the queued, then buffered, edits of its version onto the loaded data (`pending-file-changes` → `restore-pending-changes` in `bundle-fetched`); a replay that throws keeps the server data and the queue. That is what unblocks the editor without a reload; because the queue is shared, dropping the failed file is also what lets other files' pending edits drain. `release-failed-file` runs at recovery time, never at failure time — clearing the error eagerly would make waiters resolve. - The split between `initialize-persistence-state` and `recover-persistence` is deliberate: initialization deliberately does NOT lift a halt, so undeliverable edits are released when the user acts, not when the view mounts. - **Every non-transport terminal save failure is permanent** (a backend without commit-id groups still answers an invalid batch with a terminal `:validation`). There is no "stale but valid" category: permission lost, file gone, and server-rejected changes all fail again on a resend. Losing edit permission is not temporary — the backend raises `:not-found` for it (`check-edition-permissions!` → `perms/make-check-fn`), and a save's `:not-found` is a terminal save error that keeps the workspace open (`errors/delegated-persistence-types` is only `#{:authentication}`; a missing data fragment answers it too). So `:save-permission-denied` (a client-side `can-edit` check) must never be resumed: release it. Its toast `errors.save-permission-lost` is plural over `:unsaved-commits` (`count-unsaved-commits`); plural keys need `(tr k (i18n/c n))`, a bare number renders the fallback text. -- Save failures split transient vs terminal (`transient-error?`: the repo retryable types `:network`/`:offline`/`:bad-gateway`/`:service-unavailable`/`:gateway-error`, plus `:invalid-save-response`, plus save-only `busy-server-error?`: `:rate-limit` (429 from rlimit/climit — a save parked on another save's file lock holds the profile's single `:update-file/by-profile` permit, so the user's saves to other files 429 after 60 s) and backend-written 504 `:server-error` with `:statement-timeout`/`:idle-in-transaction-timeout` (Postgres cancels the lock wait at 300 s, before nginx; rolled back, so resend is safe). Kept out of `rp/retryable-types` on purpose so GETs do not retry them; everything else is terminal). Terminal keeps the `:error` halt + `flash-persistence` path; transient enters a `:retrying` episode: the head commit stays queued and resends with backoff 2s/8s/20s (3 retries, then the `:error` path). A transient failure that outlasts the backoff arms `slow-retry-cycle`: one silent resend (`:recovering`) every `slow-retry-delay-ms` (30s) until a save lands, `retry-give-up-ms` (24h since `:failing-since`) passes, or the head commit collects `max-slow-failures` (3) attempts that failed after `slow-failure-ms` (90s) or more (`:slow-failures`, counted in `persistence-transient-failure` from the attempt's elapsed time): a gateway timeout may leave the backend applying the save, so a save that keeps failing slowly must not be replayed for a day, while fast failures (offline) never count; the cycle stops on `::resume-persistence`, `::initialize-persistence` and `finalize-workspace`. Leaving the workspace also emits `suspend-retries` (`:suspended` in `:persistence`, drops `:retry-token` so a pending backoff timer goes stale): no backoff, slow cycle or `online` resend runs outside the workspace; only `workspace-initialized` (not a role change) lifts it via `recover-persistence` (`opened-file?`), which also resends a `:retrying` episode whose backoff was cut short. Its toast is `errors.save-still-retrying` without a reload link; a landing save hides the failure toast (`errors/persistence-failed-tag`). Replaying a save is safe because changes are idempotent, so there is no unknown-outcome guard: a commit whose earlier request left `active-requests` is simply sent again under the same `:commit-id`; only a request still in flight blocks a resend (`:in-flight` stays silent). Tests hold the slow cycle by stubbing `rx/timer` to never fire for `slow-retry-delay-ms`, or instant timers loop forever. Retry timers carry the episode token; a superseded token stays silent. Status stays `:retrying` through re-entries (`next-status` refuses `:pending`/`:saving` from it); waiters (`wait-persisted-or-error`) wait through it and reject only on `:error`. +- Save failures split transient vs terminal (`transient-error?`: the repo retryable types `:network`/`:offline`/`:bad-gateway`/`:service-unavailable`/`:gateway-error`, plus `:invalid-save-response`, plus save-only `busy-server-error?`: `:rate-limit` (429 from rlimit/climit — a save parked on another save's file lock holds the profile's single `:update-file/by-profile` permit, so the user's saves to other files 429 after 60 s) and backend-written 504 `:server-error` with `:statement-timeout`/`:idle-in-transaction-timeout` (Postgres cancels the lock wait at 300 s, before nginx; rolled back, so resend is safe). Kept out of `rp/retryable-types` on purpose so GETs do not retry them; everything else is terminal). Terminal keeps the `:error` halt + `flash-persistence` path; transient enters a `:retrying` episode: the head commit stays queued and resends with backoff 2s/8s/20s (3 retries, then the `:error` path). A transient failure that outlasts the backoff arms `slow-retry-cycle`: one silent resend (`:recovering`) every `slow-retry-delay-ms` (30s) until a save lands, `retry-give-up-ms` (24h since `:failing-since`) passes, or the head commit collects `max-slow-failures` (3) attempts that failed after `slow-failure-ms` (90s) or more (`:slow-failures`, counted in `persistence-transient-failure` from the attempt's elapsed time): a gateway timeout may leave the backend applying the save, so a save that keeps failing slowly must not be replayed for a day, while fast failures (offline) never count; the cycle stops on `::resume-persistence`, `::initialize-persistence` and `finalize-workspace`. Leaving the workspace also emits `suspend-retries` (`:suspended` in `:persistence`, drops `:retry-token` so a pending backoff timer goes stale): no backoff, slow cycle or `online` resend runs outside the workspace; only `workspace-initialized` (not a role change) lifts it via `recover-persistence` (`opened-file?`), which also resends a `:retrying` episode whose backoff was cut short. Its toast is `errors.save-still-retrying` without a reload link; a landing save hides the failure toast (`errors/persistence-failed-tag`). Replaying a save is safe because changes are idempotent, so there is no unknown-outcome guard: a commit whose earlier request left `active-requests` is simply sent again under the same `:commit-id`; only a request still in flight blocks a resend (`:in-flight` stays silent). Tests hold the slow cycle by stubbing `rx/timer` to never fire for `slow-retry-delay-ms`, or instant timers loop forever. Retry timers carry the episode token; a superseded token stays silent. Status stays `:retrying` through re-entries (`next-status` refuses `:pending`/`:saving` from it); waiters (`wait-persisted-or-error`) wait through it and reject only on `:error`. `terminal-status?` releases waiters only with an empty queue and an empty buffer: `run-persistence-task` sets `:saved` as soon as the queue empties, even with commits still buffered (a slow save landing while later edits wait for the 3 s flush). - One reconnect notice per episode: sticky toast tagged `:persistence-reconnecting` (single-toast store, re-show replaces), hidden by tag on drain (`:saved`) and on terminal failure; recovery is silent. Header indicator has a `:retrying` state (`workspace.header.retrying`). - Resume triggers: backoff timer and the browser `online` event (guarded by `exists? js/window`; re-enters the runner only for a live `:retrying` episode). New local edits during `:retrying` only join the queue: the episode keeps its `:run-id`, so `append-commit` does not re-enter, and the live runner (still waiting on the head's `commit-persisted`) sends them after the head. Re-entering on edits would bypass the backoff and spend an attempt per batched commit. - Tests instant-trigger retries by stubbing `rx/timer` (recording delays to assert the schedule); dynamic bindings do not survive `await` continuations, so no dynamic var for delays. diff --git a/frontend/src/app/main/data/changes.cljs b/frontend/src/app/main/data/changes.cljs index cdae674035..9573a34cdf 100644 --- a/frontend/src/app/main/data/changes.cljs +++ b/frontend/src/app/main/data/changes.cljs @@ -261,13 +261,14 @@ Options: - save-undo?: if set to false, do not add undo changes. - undo-group: if some consecutive changes (or even transactions) share the same - undo-group, they will be undone or redone in a single step + undo-group, they will be undone or redone in a single step. Without + it, a commit made while an undo transaction is open joins the + transaction's group, and any other gets a new one. " [{:keys [redo-changes undo-changes save-undo? undo-group tags stack-undo? file-id translation? skip-component-sync?] :or {save-undo? true stack-undo? false - undo-group (uuid/next) tags #{}} :as params}] (ptk/reify ::commit-changes @@ -289,7 +290,11 @@ (not (dm/get-in state [:workspace-global :preview-id]))) (do (log/trace :hint "commit-changes" :redo-changes redo-changes) - (let [selected (dm/get-in state [:workspace-local :selected])] + (let [selected (dm/get-in state [:workspace-local :selected]) + tx (dm/get-in state [:workspace-undo :transaction]) + ;; Inside an undo transaction, the group of its history entry + ;; (taken from its first commit), or the one it was opened with + undo-group (or undo-group (:undo-group tx) (:commit-group tx) (uuid/next))] (rx/of (-> params (assoc :undo-group undo-group) (assoc :features features) diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index 3007d1fc0a..3086280976 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -70,12 +70,15 @@ (defn terminal-status? "True when a persistence snapshot releases waiters: a failed save, or - a settled (`nil` / `:saved`) empty queue. Anything else — `:pending`, - `:saving` and the `:retrying` episode — keeps waiting." - [{:keys [status queue]}] + a settled (`nil` / `:saved`) empty queue and buffer. Anything else — + `:pending`, `:saving`, the `:retrying` episode, or commits still in the + buffer (the status turns `:saved` as soon as the queue empties) — keeps + waiting." + [{:keys [status queue buffer]}] (boolean (or (= status :error) (and (empty? queue) + (empty? buffer) (or (nil? status) (= status :saved)))))) (defn wait-persisted-or-error @@ -548,7 +551,9 @@ (dch/commit-changes (assoc changes :origin origin :file-id file-id - :save-undo? false)) + :save-undo? false + ;; Its own group, never that of an open undo transaction + :undo-group (uuid/next))) (let [{:keys [file-revn file-vern features]} (peek commits)] (append-commit (pcm/queue-entry diff --git a/frontend/src/app/main/data/workspace/undo.cljs b/frontend/src/app/main/data/workspace/undo.cljs index 6fa7fceb3c..ee24789e08 100644 --- a/frontend/src/app/main/data/workspace/undo.cljs +++ b/frontend/src/app/main/data/workspace/undo.cljs @@ -42,6 +42,7 @@ [app.common.schema :as sm] [app.common.time :as ct] [app.common.types.shape.layout :as ctl] + [app.common.uuid :as uuid] [app.main.data.changes :as dch] [app.main.data.common :as dcm] [app.main.data.helpers :as dsh] @@ -205,7 +206,9 @@ (update state :workspace-undo (fn [undo-state] (-> undo-state - (update :transaction #(d/nilv % empty-tx)) + ;; Commits made while it is open without an undo group of + ;; their own get this one + (update :transaction #(d/nilv % (assoc empty-tx :commit-group (uuid/next)))) (update :transactions-pending assoc id (ct/now)))))) ptk/WatchEvent @@ -237,7 +240,7 @@ (if (and (some? current-tx) (empty? pending-tx)) (-> state - (add-undo-entry current-tx) + (add-undo-entry (dissoc current-tx :commit-group)) (update :workspace-undo dissoc :transaction)) state))) @@ -253,7 +256,8 @@ (defn forget-undo-groups "Drops the undo history from the first entry of `undo-groups` on, or all - of it if none matches, and empties the open transaction." + of it if none matches, and empties the open transaction. Later commits + of that transaction keep the undo group of its earlier ones." [undo-groups] (ptk/reify ::forget-undo-groups ptk/UpdateEvent @@ -270,7 +274,9 @@ (dissoc :items :index) (some? transaction) - (assoc :transaction empty-tx)))))))) + (assoc :transaction + (assoc empty-tx :commit-group (or (:undo-group transaction) + (:commit-group transaction))))))))))) (def reinitialize-undo "Clears the undo stack, removing all entries and transactions." diff --git a/frontend/test/frontend_tests/data/persistence_test.cljs b/frontend/test/frontend_tests/data/persistence_test.cljs index a30cc53851..dc72ae5a7a 100644 --- a/frontend/test/frontend_tests/data/persistence_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_test.cljs @@ -770,7 +770,10 @@ (t/is (not (dps/terminal-status? {:status :retrying :queue [1]}))) (t/is (not (dps/terminal-status? {:status :saving :queue [1]}))) (t/is (not (dps/terminal-status? {:status :pending :queue [1]}))) - (t/is (not (dps/terminal-status? {:status :saved :queue [1]})))) + (t/is (not (dps/terminal-status? {:status :saved :queue [1]}))) + (t/is (not (dps/terminal-status? {:status :saved :queue [] :buffer [{:id 1}]})) + "commits still buffered keep the waiters waiting") + (t/is (dps/terminal-status? {:status :error :queue [] :buffer [{:id 1}]}))) ;; Scenario: the queue references a commit id with no matching commit ;; (plus a dangling run id) when persistence initializes. It must error @@ -1280,6 +1283,32 @@ (await (async/wait-for #(= :saved (get-in @store [:persistence :status])) "the save lands"))))))) +;; Scenario: a save is in flight while a later edit waits in the buffer, and +;; something waits for the edits to be saved (as a library sync does before +;; marking the library synced). The save lands and the queue empties, so the +;; status turns :saved. Proves: the waiter is released only once the +;; buffered edit is saved too. +(t/deftest ^:async waiting-for-the-save-covers-the-buffered-edits + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (let [released? #(dps/terminal-status? (:persistence @store))] + (ptk/emit! store (local-commit file-id) ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the first edit is sent")) + (ptk/emit! store (local-commit file-id)) + (await (async/wait-for #(seq (get-in @store [:persistence :buffer])) + "the second edit waits in the buffer")) + (rx/push! response {:revn 1}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the first save lands")) + (t/is (seq (get-in @store [:persistence :buffer]))) + (t/is (not (released?)) "the buffered edit is not saved yet") + (ptk/emit! store ::dps/force-persist) + (await (async/wait-for #(= 2 (count @requests)) "the buffered edit is sent")) + (rx/push! response {:revn 2}) + (await (async/wait-for released? "the waiters are released"))))))) + ;; Rollback of rejected commits. ;; Scenario: the first save carries two commits; while it is in flight a @@ -1601,6 +1630,41 @@ (await (async/wait-for #(= :saved (get-in @store [:persistence :status])) "the queue drains"))))))))) +;; Scenario: as above, while the user has an undo transaction open. Proves: +;; the undo commit gets its own group, not the transaction's, so a later +;; rejection in the transaction does not revert it. +(t/deftest ^:async the-undo-of-a-saved-commit-stays-out-of-an-open-transaction + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (await + (mock/with-mocks* + {mw/ask! (mock/stub (fn [_] (rx/empty)))} + (let [lib-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + lshape-id (uuid/next) + group (uuid/next) + linked (grouped (rename-commit lib-id page-id lshape-id "X" "Y") group) + commit (grouped (rename-commit file-id page-id shape-id "A" "B") group)] + (ptk/emit! store (with-library file-id lib-id page-id shape-id "B" lshape-id "Y")) + (ptk/emit! store linked commit ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the library's save is sent first")) + (rx/push! response {:revn 1}) + (await (async/wait-for #(= 2 (count @requests)) "this file's save is sent")) + (ptk/emit! store (dwu/start-undo-transaction (js/Symbol) :timeout 0)) + (let [tx-group (get-in @store [:workspace-undo :transaction :commit-group])] + (rx/push! response (rejected-second commit)) + (await (async/wait-for #(seq (get-in @store [:persistence :buffer])) + "a new commit undoes the library's")) + (t/is (some? tx-group)) + (t/is (not= tx-group (get-in @store [:persistence :buffer 0 :undo-group]))) + (ptk/emit! store ::dps/force-persist) + (await (async/wait-for #(= 3 (count @requests)) "the new commit is sent")) + (rx/push! response {:revn 2}) + (await (async/wait-for #(= :saved (get-in @store [:persistence :status])) + "the queue drains")))))))))) + ;; Scenario: as above, but the library is no longer loaded when this file's ;; save comes back. Proves: the undo of the library's saved commit is still ;; sent, without touching local data the library no longer has. @@ -1692,6 +1756,80 @@ (await (async/wait-for #(= :saved (get-in @store [:persistence :status])) "the queue drains"))))))))) +;; Scenario: applying a token to five shapes makes three commits inside one +;; undo transaction, all sent in one save; the server keeps the first two +;; and rejects the third. Proves: the whole action is reverted, the kept +;; commits by a new commit, so opacity and applied tokens are back to their +;; starting values on every shape. +(t/deftest ^:async a-rejection-reverts-every-commit-of-an-undo-transaction + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (await + (mock/with-mocks* + {mw/ask! (mock/stub (fn [_] (rx/empty)))} + (let [page-id (uuid/next) + shape-ids (into [] (repeatedly 5 uuid/next)) + shapes (map #(cts/setup-shape {:id % :type :rect :name "R" :opacity 1 + :parent-id uuid/zero :frame-id uuid/zero}) + shape-ids) + apply-to (fn [ids] + (dch/commit-changes + {:redo-changes (into [] + (mapcat (fn [id] + [(set-attr page-id id :opacity 0.5) + (set-attr page-id id :applied-tokens {:opacity "half"})])) + ids) + :undo-changes (into [] + (mapcat (fn [id] + [(set-attr page-id id :opacity 1) + (set-attr page-id id :applied-tokens nil)])) + ids)})) + tx-id (js/Symbol)] + (ptk/emit! store #(-> % + (assoc :current-page-id page-id) + (assoc-in [:files file-id :data] + {:pages [page-id] + :pages-index {page-id {:id page-id + :name "Page" + :objects (into {uuid/zero (cts/setup-shape {:id uuid/zero :type :frame + :shapes shape-ids})} + (map (juxt :id identity)) + shapes)}}}))) + (ptk/emit! store + (dwu/start-undo-transaction tx-id :timeout 0) + (apply-to (subvec shape-ids 0 2)) + (apply-to (subvec shape-ids 2 4)) + (apply-to (subvec shape-ids 4 5)) + (dwu/commit-undo-transaction tx-id) + ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the action is sent")) + (let [groups (get-in (first @requests) [1 :changes-with-metadata]) + third (:commit-id (nth groups 2))] + (t/is (= 3 (count groups)) "the save carries the three commits") + (rx/push! response {:revn 0 + :rejected {:commit-ids [third] + :failed-commit-id third + :error {:type :validation :code :referential-integrity}}})) + (await (async/wait-for #(seq (get-in @store [:persistence :buffer])) + "a new commit undoes the kept ones")) + (ptk/emit! store ::dps/force-persist) + (await (async/wait-for #(= 2 (count @requests)) "the new commit is sent")) + (doseq [id shape-ids] + (t/is (= 1 (shape-attr store file-id page-id id :opacity))) + (t/is (nil? (shape-attr store file-id page-id id :applied-tokens)))) + (t/is (= #{1} + (into #{} + (comp (mapcat :changes) + (mapcat :operations) + (filter #(= :opacity (:attr %))) + (map :val)) + (get-in (second @requests) [1 :changes-with-metadata]))) + "the new commit puts the kept opacity back on the server") + (rx/push! response {:revn 1}) + (await (async/wait-for #(= :saved (get-in @store [:persistence :status])) + "the queue drains"))))))))) + ;; Scenario: the rejected commit added the text being edited. Proves: the ;; rollback leaves edition mode, so the editor does not keep pointing at a ;; shape that no longer exists. @@ -1739,7 +1877,9 @@ undo (:workspace-undo (ptk/update (dwu/forget-undo-groups #{g2 g3}) state))] (t/is (= [g1] (mapv :undo-group (:items undo)))) (t/is (= 0 (:index undo))) - (t/is (empty? (get-in undo [:transaction :undo-changes])) "the open transaction is emptied"))) + (t/is (empty? (get-in undo [:transaction :undo-changes])) "the open transaction is emptied") + (t/is (= g3 (get-in undo [:transaction :commit-group])) + "its later commits keep the group of its earlier ones"))) (t/deftest forgetting-undo-groups-clears-the-history-without-a-match (let [state {:workspace-undo {:items [{:undo-group (uuid/next)}] :index 0}} diff --git a/frontend/test/frontend_tests/data/workspace_undo_test.cljs b/frontend/test/frontend_tests/data/workspace_undo_test.cljs new file mode 100644 index 0000000000..0c54aeb5c1 --- /dev/null +++ b/frontend/test/frontend_tests/data/workspace_undo_test.cljs @@ -0,0 +1,113 @@ +;; 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-undo-test + "Tests for the undo groups of commits made inside an undo transaction." + (:require + [app.common.test-helpers.files :as cthf] + [app.common.test-helpers.ids-map :as cthi] + [app.common.uuid :as uuid] + [app.main.data.changes :as dch] + [app.main.data.workspace.undo :as dwu] + [beicon.v2.core :as rx] + [cljs.test :as t :include-macros true] + [frontend-tests.helpers.state :as ths] + [potok.v2.core :as ptk])) + +(t/use-fixtures :each {:before cthi/reset-idmap!}) + +(defn- rename-page + "Commits a page rename, with `params` for `commit-changes`." + [page-id name & {:as params}] + (dch/commit-changes (merge {:redo-changes [{:type :mod-page :id page-id :name name}] + :undo-changes [{:type :mod-page :id page-id :name "Page 1"}]} + params))) + +(defn- run-commits + "Runs `events` on a store of a sample file and calls `check` with the + undo groups of the commits made and the final state." + [done events check] + (let [file (cthf/sample-file :file1 :page-label :page1) + store (ths/setup-store file) + groups (atom []) + sub (->> (ptk/input-stream store) + (rx/filter dch/commit?) + (rx/subs! #(swap! groups conj (:undo-group (deref %)))))] + (ths/run-store store done (events (cthf/current-page-id file)) + (fn [state] + (rx/dispose! sub) + (check @groups state))))) + +;; Scenario: an action commits twice inside one undo transaction (as applying +;; a token to several shapes does). Proves: both commits carry the undo group +;; of the transaction, which its history entry takes too, so a rejected save +;; reverts the whole Ctrl+Z step, not half of it. +(t/deftest commits-inside-a-transaction-share-its-undo-group + (t/async + done + (let [tx (js/Symbol) + tx-group (atom nil) + spy (ptk/reify ::spy-transaction + ptk/EffectEvent + (effect [_ state _] + (reset! tx-group (get-in state [:workspace-undo :transaction :commit-group]))))] + (run-commits done + (fn [page-id] + [(dwu/start-undo-transaction tx) + spy + (rename-page page-id "A") + (rename-page page-id "B") + (dwu/commit-undo-transaction tx)]) + (fn [groups _] + (t/is (some? @tx-group) "the transaction has its own group") + (t/is (= [@tx-group @tx-group] groups))))))) + +;; Scenario: commits made with no transaction open. Proves: each one keeps +;; its own undo group. +(t/deftest commits-outside-a-transaction-get-their-own-undo-group + (t/async + done + (run-commits done + (fn [page-id] + [(rename-page page-id "A") + (rename-page page-id "B")]) + (fn [groups _] + (t/is (= 2 (count (distinct groups)))))))) + +;; Scenario: inside a transaction, a commit names its undo group (e.g. a +;; component update that also writes the library). Proves: the explicit group +;; is kept. +(t/deftest an-explicit-undo-group-is-kept-inside-a-transaction + (t/async + done + (let [tx (js/Symbol) + group (uuid/next)] + (run-commits done + (fn [page-id] + [(dwu/start-undo-transaction tx) + (rename-page page-id "A" :undo-group group) + (dwu/commit-undo-transaction tx)]) + (fn [groups _] + (t/is (= [group] groups))))))) + +;; Scenario: a rollback forgets undo groups while a transaction is open, then +;; the transaction commits again. Proves: the later commit keeps the group of +;; the earlier one, so the transaction stays one rollback unit. +(t/deftest a-transaction-keeps-its-undo-group-after-forget-undo-groups + (t/async + done + (let [tx (js/Symbol)] + (run-commits done + (fn [page-id] + [(dwu/start-undo-transaction tx) + (rename-page page-id "A") + (dwu/forget-undo-groups #{(uuid/next)}) + (rename-page page-id "B") + (dwu/commit-undo-transaction tx)]) + (fn [[first-group second-group :as groups] _] + (t/is (= 2 (count groups))) + (t/is (some? first-group)) + (t/is (= first-group second-group))))))) diff --git a/frontend/test/frontend_tests/runner.cljs b/frontend/test/frontend_tests/runner.cljs index ca11c834a5..96ca35d60b 100644 --- a/frontend/test/frontend_tests/runner.cljs +++ b/frontend/test/frontend_tests/runner.cljs @@ -38,6 +38,7 @@ [frontend-tests.data.workspace-shortcuts-test] [frontend-tests.data.workspace-texts-test] [frontend-tests.data.workspace-thumbnails-test] + [frontend-tests.data.workspace-undo-test] [frontend-tests.data.workspace-variant-rename-test] [frontend-tests.data.workspace-versions-test] [frontend-tests.debug-logging-test] @@ -190,6 +191,7 @@ 'frontend-tests.data.workspace-shortcuts-test 'frontend-tests.data.workspace-texts-test 'frontend-tests.data.workspace-thumbnails-test + 'frontend-tests.data.workspace-undo-test 'frontend-tests.data.workspace-variant-rename-test 'frontend-tests.data.workspace-versions-test 'frontend-tests.debug-logging-test