From 4051957b156c15affdfa1bef225c6620e50f006b Mon Sep 17 00:00:00 2001 From: "alonso.torres" Date: Wed, 4 Nov 2026 09:59:46 +0100 Subject: [PATCH] :bug: Apply the changes stored before a save before acknowledging it The answer to a save lists the changes other sessions stored before it, such as the backend's repair of a damaged file. When the answer came before the message with such a change, the saved commit was acknowledged first, and the late change was then applied on top of the local edits instead of under them, out of the server's order. A saved commit of the open file now stays pending until this tab has received every change the answer lists from other sessions, so a late change is applied under the local edits. If one does not arrive within 10 seconds, the file reloads. --- .../workspace-state-persistence-subtleties.md | 1 + frontend/src/app/main/data/persistence.cljs | 87 +++++++++- .../frontend_tests/data/persistence_test.cljs | 149 ++++++++++++++++++ 3 files changed, 230 insertions(+), 7 deletions(-) diff --git a/.serena/memories/frontend/workspace-state-persistence-subtleties.md b/.serena/memories/frontend/workspace-state-persistence-subtleties.md index a315b5cfd2..90237fb83c 100644 --- a/.serena/memories/frontend/workspace-state-persistence-subtleties.md +++ b/.serena/memories/frontend/workspace-state-persistence-subtleties.md @@ -24,6 +24,7 @@ - Persistence buffers local commits in the store (`buffer-commit` → `[:persistence :buffer]`, stamped with `:remote-seq`): status becomes pending after about 200ms; the 3s debounce or `::force-persist` emits `flush-buffer`, which merges the buffer per file into `append-commit`s, and each append removes its own commits from the buffer (a commit arriving mid-flush stays). `initialize-persistence-state` flushes a leftover buffer. Queue entries (`pcm/queue-entry`) hold the entry keys of the first commit (id, created-at, origin, features, file-id/revn/vern) and `:commits` (id, origin, undo-group, redo/undo changes, remote-seq of each merged commit), no merged change lists; every reader goes through `:commits`. The save sends `:changes-with-metadata`, one group per commit with `:commit-id`, never flat `:changes`. - A save answered with `:rejected` (backend kept the commits before the first failing one, see `mem:backend/subtleties`) is not a failure: `rollback-rejected` runs before `commit-persisted`. The unit is the undo group (one user action, which can commit to several files, e.g. `update-component` writes the copy's file and the library, and `component-changed` syncs inherit the edit's group; multi-file actions must share a non-nil group): the pure bookkeeping lives in `app.main.data.persistence.commits` (`pcm`): `pcm/rollback-plan` reverts, in the saved file, the pending commits from the first rejected one on, and in any file the pending commits from the first one of a reverted group on, iterating until no new group appears; other files' unrelated commits stay. `pcm/without-rolled-back` drops pending reverted commits from queue entries (partial entries trimmed, empty ones leave the queue) and the buffer; saved commits of a reverted group (the head's kept ones, and `:saved`, which `keep-saved-commits` fills on `discard-commit` with saved commits whose group a pending commit still shares) are undone by a new `save-undo? false` commit (`compensate-commits`, flushed like any edit); for a file whose data is not loaded the undo goes straight to the queue as an entry, nothing applied locally. Per file, `dch/rebase-commits` undoes every pending commit newest first, re-applies the remote changes kept since the oldest pending commit, and redoes the pending commits that stay. Undo changes hold absolute values from commit time, so undoing only the rejected commits would overwrite a peer's later change to the same attr. Remote changes are numbered (`record-remote-changes`, `[:persistence :remote-seq]`) and kept per file under `:remote-changes` only while the file has pending commits; `prune-remote-changes` (discard, release) forgets those at or below the oldest pending stamp, since re-applying them would undo saved edits. Then `dwu/forget-undo-groups groups undo-index` (cut history before the first entry whose `:undo-groups` — every group the entry holds, filled by `append-undo`, transactions and stacking — meets a reverted group; no match keeps the history; the open transaction is emptied only when cut or holding one, keeping its group for later commits; a reverted Ctrl+Z/Ctrl+Shift+Z commit, which carries real `:undo-changes` and `:undo-index` (the index it moved from, kept by `pcm` commit-keys), puts the index back to the plan's `:undo-index`, the oldest one's; background commits with `:undo-changes []` (position-data, component `modified-at`, deleted-font fixes) stay on screen after a rollback by design), selection pruned and edition mode left when the edited shape is gone, or when it is a path being edited that the rollback changed: `discard-path-edit` drops `[:workspace-local :edit-path id]` first, so `finalize-path-content` sees no `:old-content` and the editor's stale copy is never saved (`prune-removed-shapes` takes the edited object from before the rebase to compare). Text editions stay open: the v3 WASM editor already shows the reverted text, and the v2 editor resets its DOM root from the shape on `::rolled-back` when the text differs, since its unmount `on-blur` would otherwise save the stale DOM content (`prune-removed-shapes`; `v2-update-text-shape-content` also skips a missing shape, so the editor's exit commits nothing), `errors/flash` `:unhandled` toast tagged `:persistence-rejected`, hidden after `rejected-timeout-ms` (10 s). It ends with `(ptk/data-event ::rolled-back {:file-ids})`: `dwl/sync-file` waits for it or `wait-persisted` and marks the library synced only on a landed save (on rollback it restores `:synced-at` and re-checks updates after the rollback's own save). `update-status` ignores `:pending` when buffer and queue are empty (`nothing-to-save?`): a dropped commit's 200 ms pending debounce would otherwise leave the status stuck and `wait-persisted-or-error` waiting forever. `prune-removed-shapes` also closes a `:colorpicker` modal. `rebase-commits` is not a commit (not saved/broadcast/undoable, commit watchers such as thumbnails do not see it) and falls back to `::dw/reload-current-file` if applying throws. `debug.commitInvalidChange({kind, before, after})` forces such a rejection live. - Persistence sends revn as the max of the commit revn and locally tracked latest revn; remote commits update that revn tracker. +- Save answer `{:revn (pre-update) :lagged [rows revn > sent revn, created-at order, own row last] :rejected?}`. `settle-saved-commit` replaces a direct `commit-persisted`: for the current file, when the last `:lagged` row is this session's (`:session-id state`) and a row of another session (a peer, or the backend repair session, whose `:file-change` `:repaired` message also reaches the saver) has revn > `:remote-revn`, the head entry gets `::awaiting` (attempt-state `:in-flight`, no resend) and stays pending until a remote commit with that revn arrives, so the normal remote rebase puts that row under the saved commits, as the server ordered them; then `commit-persisted`. Bounded: `unseen-changes-timeout-ms` (10 s, `rx/timer`) acknowledges and emits `::dw/reload-current-file`; `finalize-workspace` acknowledges at once. Repair changes are never applied from the HTTP answer; ws messages per file arrive in publish order, so max received revn is the seen test. - A reload to another version (peer restore; `pending-file-changes` already skips other-vern edits): `bundle-fetched` calls `dps/forget-other-versions` → `pcm/without-other-versions`: other-vern buffered and `:saved` commits dropped, other-vern queue entries kept but emptied (`:commits []`), remote changes pruned, a `:pending` status with nothing left turns `:saved`. So remote rebases and rollbacks never replay them on the reloaded data. `stale-commit?` (entry/commit `:file-vern` ≠ loaded file `:vern`, never when the file has no `:vern`): attempt-state `:stale` → acknowledged unsent; `rollback-rejected` of a stale entry does nothing (no rebase, toast or `::rolled-back`); `save-attempt-failed` drops a stale commit's failure (e.g. `:vern-conflict`) instead of halting. `merge-commit` groups the buffer by file and vern (one save carries one vern). - Persistence is skipped in version preview/read-only mode or without edit permission. - Failing saves lock the workspace (`app.main.data.persistence.lock`, alias `plk`): `plk/failing?` (status `:error`, or `:recovering` while a slow-cycle attempt is in flight as `:saving`) drives `sync-failure-lock` (also on `change-team-role`), which locks only while `[:permissions :can-edit]`: a viewer, already read-only, gets the `:save-permission-denied` toast and never the save-lock banner; `lock-on-failure` stores the previous read-only value in `[:workspace-global :persistence-lock]` and sets `:read-only?`; `unlock-after-failure` restores it (into `[:render-state :pre-context-lost-read-only?]` instead while the WebGL context is lost, which keeps its own save/restore). `set-workspace-read-only` writes into the lock while it is held, so an Inspect/Design switch is not lost. `view-only-bar*` (`ui/workspace/viewport/top_bar.cljs`, shared by both renderers) shows the save error while the lock exists (text per `dps/save-retrying?`), hides its Done/exit button and takes priority over the context-loss message. No save-failure popup: page navigation, pan, zoom and inspection stay available. Once unlocked, the usual Inspect/context-loss message and action return. Plugins still write while read-only (never as a viewer: `commit-changes` drops commits without `:can-edit`). A peer's `:file-restored` (backend adds `:profile-id`, `:created-at` and, for a version a user named, `:label`) reloads the file and opens the `:file-restored` modal: who restored (profile, else the session's presence, else "A teammate"), the version by label, else date (`PPp`), and the "latest changes couldn't be saved" line when `dps/pending-file-changes` held edits of the replaced version, which the reload drops. diff --git a/frontend/src/app/main/data/persistence.cljs b/frontend/src/app/main/data/persistence.cljs index af640920e7..c6a316d98e 100644 --- a/frontend/src/app/main/data/persistence.cljs +++ b/frontend/src/app/main/data/persistence.cljs @@ -37,6 +37,7 @@ (declare ^:private resume-persistence) (declare ^:private slow-retry-cycle) (declare ^:private flush-buffer) +(declare ^:private commit-persisted) (log/set-level! :warn) @@ -65,6 +66,9 @@ sending it. Fast failures, such as being offline, never count." 3) +;; Wait for the changes stored before a saved commit, then reload the file. +(def ^:private unseen-changes-timeout-ms 10000) + (def ^:private saving-stall-timeout-ms (* 5 60 1000)) (def ^:private saving-check-interval-ms 30000) (def ^:private save-wait-timeout-ms (* 2 60 1000)) @@ -309,16 +313,22 @@ (swap! revn-data update file-id (fnil max 0) revn)))) (defn- record-remote-changes - "Numbers a remote commit and, while `file-id` has pending commits, keeps - its changes for a rollback to apply again." - [file-id changes] + "Numbers a remote commit, keeps the highest revision received per file + (`:remote-revn`) and, while `file-id` has pending commits, keeps its + changes for a rollback to apply again." + [file-id revn changes] (ptk/reify ::record-remote-changes + IDeref + (-deref [_] {:file-id file-id :revn revn}) + ptk/UpdateEvent (update [_ state] (update state :persistence (fn [pstate] (let [number (inc (:remote-seq pstate 0))] - (cond-> (assoc pstate :remote-seq number) + (cond-> (-> pstate + (assoc :remote-seq number) + (update-in [:remote-revn file-id] (fnil max revn) revn)) (seq (pcm/pending-commits pstate file-id)) (update-in [:remote-changes file-id] (fnil conj []) {:seq number :changes changes})))))))) @@ -333,6 +343,67 @@ (and (some? vern) (not= vern file-vern)))) +(defn- last-unseen-revn + "The highest revision above `seen` (the highest one received) of the rows + other sessions stored in `lagged`, the rows a save answer lists after + the revision it was sent with. Nil when there is none, or when the save + stored nothing (the last row is not this session's)." + [lagged session-id seen] + (let [own? #(= session-id (:session-id %))] + (when (own? (last lagged)) + (transduce (comp (remove own?) + (map :revn) + (filter #(> % seen))) + (completing (fnil max 0)) + nil + lagged)))) + +(defn- settle-saved-commit + "Acknowledges the saved `commit` once this tab received every change the + server stored before it (from `lagged`, the save answer's rows), such as + the repair of a damaged file. Until then its commits stay pending, so a + late change is applied under them, in the server's order. A change that + does not arrive in time reloads the file." + [commit lagged] + (let [needed (volatile! nil)] + (ptk/reify ::settle-saved-commit + ptk/UpdateEvent + (update [_ state] + (let [file-id (:file-id commit) + seen (dm/get-in state [:persistence :remote-revn file-id] -1) + revn (when (and (= file-id (:current-file-id state)) + (not (stale-commit? state commit))) + (last-unseen-revn lagged (:session-id state) seen))] + (vreset! needed revn) + (cond-> state + (some? revn) + (d/update-in-when [:persistence :index (:id commit)] assoc ::awaiting revn)))) + + ptk/WatchEvent + (watch [_ _ stream] + (if-let [revn @needed] + (let [file-id (:file-id commit) + arrived-s (->> stream + (rx/filter (ptk/type? ::record-remote-changes)) + (rx/map deref) + (rx/filter #(and (= file-id (:file-id %)) + (>= (:revn %) revn))) + (rx/map (constantly ::arrived))) + left-s (->> stream + (rx/filter (ptk/type? ::dw/finalize-workspace)) + (rx/map (constantly ::left))) + timeout-s (->> (rx/timer unseen-changes-timeout-ms) + (rx/map (constantly ::timeout)))] + (->> (rx/merge arrived-s left-s timeout-s) + (rx/take 1) + (rx/mapcat (fn [outcome] + (rx/concat + (rx/of (commit-persisted commit)) + (if (= ::timeout outcome) + (rx/of (ptk/event ::dw/reload-current-file)) + (rx/empty))))))) + (rx/of (commit-persisted commit))))))) + (defn- discard-commit [commit-id] (ptk/reify ::discard-commit @@ -753,6 +824,8 @@ (nil? commit) :missing-commit (::acknowledged commit) :acknowledged (contains? @active-requests (::request-id commit)) :in-flight + ;; Saved, waiting for the changes stored before it + (::awaiting commit) :in-flight (stale-commit? state commit) :stale (not (dm/get-in state [:permissions :can-edit])) :permission-denied :else :ready))) @@ -813,7 +886,7 @@ ;; A response that carries no revision, including one that never ;; arrived, is treated as a failed save rather than a saved file. (rx/if-empty nil) - (rx/mapcat (fn [{:keys [revn rejected]}] + (rx/mapcat (fn [{:keys [revn rejected lagged]}] (if (and (int? revn) (<= 0 revn)) (rx/concat (rx/of (update-file-revn file-id revn)) @@ -821,7 +894,7 @@ (if (some? rejected) (rx/of (rollback-rejected commit rejected)) (rx/empty)) - (rx/of (commit-persisted commit))) + (rx/of (settle-saved-commit commit lagged))) (rx/throw (ex-info "The save response has no valid revision" {:type :persistence :code :invalid-save-response @@ -1327,6 +1400,6 @@ (rx/map deref) (rx/filter #(= :remote (:source %))) (rx/mapcat (fn [{:keys [file-id file-revn redo-changes]}] - (rx/of (record-remote-changes file-id redo-changes) + (rx/of (record-remote-changes file-id file-revn redo-changes) (update-file-revn file-id file-revn)))) (rx/take-until stoper-s))))))) diff --git a/frontend/test/frontend_tests/data/persistence_test.cljs b/frontend/test/frontend_tests/data/persistence_test.cljs index 7e5a1486b7..544a41ee03 100644 --- a/frontend/test/frontend_tests/data/persistence_test.cljs +++ b/frontend/test/frontend_tests/data/persistence_test.cljs @@ -1307,6 +1307,155 @@ (t/is (= "A" (thp/shape-name store file-id page-id shape-id)) "the library edit is not replayed on this file")))))) +;; Changes the server stored before a save. + +(def ^:private repair-session-id + #uuid "a11c0de5-0000-4000-8000-000000000001") + +(defn- add-child-commit + "A local commit of `file-id` that adds `shape` as the last child of the + root of `page-id`." + [file-id page-id {:keys [id] :as shape}] + (ptk/data-event ::dch/commit + {:id (uuid/next) + :file-id file-id + :file-revn 0 + :file-vern 0 + :source :local + :features #{} + :undo-group (uuid/next) + :redo-changes [{:type :add-obj :id id :page-id page-id + :parent-id uuid/zero :frame-id uuid/zero :obj shape}] + :undo-changes [{:type :del-obj :id id :page-id page-id}]})) + +(defn- set-root-children + "A change that sets the children of the root of `page-id`." + [page-id ids] + {:type :mod-obj :id uuid/zero :page-id page-id + :operations [{:type :set :attr :shapes :val ids}]}) + +(defn- lagged-row + "A row of a save answer's `:lagged`: a change stored at `revn` by + `session-id`." + [file-id revn session-id changes] + {:id (uuid/next) :file-id file-id :revn revn :session-id session-id :changes changes}) + +(defn- root-children + [store file-id page-id] + (thp/shape-attr store file-id page-id uuid/zero :shapes)) + +(defn- open-with-new-child + "Opens a page whose root holds `shape-id` and the new child `child`, as + the local data does once the user adds it." + [store file-id session-id page-id shape-id child] + (ptk/emit! store + (thp/open-file file-id page-id + (thp/add-shape (thp/file-data page-id shape-id "S") page-id child)) + #(assoc % :session-id session-id))) + +;; Scenario: the user adds a child to a page with old damage. The server +;; repairs the page first, storing a repair that sets the root's children +;; without the new child, then stores the user's change, which adds it. The +;; save answer reaches the tab before the repair's message. Proves: the +;; saved commit stays pending until the repair arrives, so the repair goes +;; under it and the tab ends as the server does. +(t/deftest ^:async a-repair-answered-before-its-message-goes-under-the-saved-commit + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (let [session-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + child (cts/setup-shape {:id (uuid/next) :type :rect :name "Rectangle" + :parent-id uuid/zero :frame-id uuid/zero}) + commit (add-child-commit file-id page-id child) + repair [(set-root-children page-id [shape-id])]] + (open-with-new-child store file-id session-id page-id shape-id child) + (ptk/emit! store commit ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the new child is sent")) + (rx/push! response {:revn 0 + :lagged [(lagged-row file-id 1 repair-session-id repair) + (lagged-row file-id 2 session-id (:redo-changes @commit))]}) + (await (async/wait-for #(get-in @store [:persistence :index (:id @commit) ::dps/awaiting]) + "the saved commit waits for the repair")) + (t/is (= [(:id @commit)] (vec (get-in @store [:persistence :queue])))) + + (ptk/emit! store (thp/remote-commit file-id 1 repair)) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the saved commit is acknowledged once the repair arrives")) + (t/is (= [shape-id (:id child)] (root-children store file-id page-id)) + "the repair goes under the new child, as on the server")))))) + +;; Scenario: the repair's message reaches the tab before the save answer. +;; Proves: the answer does not wait for a change the tab already applied. +(t/deftest ^:async a-save-answer-does-not-wait-for-changes-already-received + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (let [session-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + child (cts/setup-shape {:id (uuid/next) :type :rect :name "Rectangle" + :parent-id uuid/zero :frame-id uuid/zero}) + commit (add-child-commit file-id page-id child) + repair [(set-root-children page-id [shape-id])]] + (open-with-new-child store file-id session-id page-id shape-id child) + (ptk/emit! store commit ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the new child is sent")) + (ptk/emit! store (thp/remote-commit file-id 1 repair)) + (rx/push! response {:revn 0 + :lagged [(lagged-row file-id 1 repair-session-id repair) + (lagged-row file-id 2 session-id (:redo-changes @commit))]}) + (await (async/wait-for #(and (= :saved (get-in @store [:persistence :status])) + (empty? (get-in @store [:persistence :queue]))) + "the save lands at once")) + (t/is (= [shape-id (:id child)] (root-children store file-id page-id)))))))) + +;; Scenario: the save answer lists a repair whose message never reaches the +;; tab. Proves: the wait is bounded; the saved commit is acknowledged and +;; the file reloads to get the server's data. +(t/deftest ^:async a-change-that-never-arrives-reloads-the-file + (let [reloads (atom 0) + timeout-s (rx/subject) + original (get-method ptk/resolve ::dw/reload-current-file)] + (-add-method ptk/resolve ::dw/reload-current-file + (fn [_ _] + (swap! reloads inc) + (ptk/data-event ::reloaded {}))) + (try + (await + (with-persistence + (^:async fn [{:keys [file-id response requests store]}] + (await + (mock/with-mocks* + {rx/timer (mock/stub (fn [ms] + (if (= ms @#'dps/unseen-changes-timeout-ms) + timeout-s + (rx/subject))))} + (let [session-id (uuid/next) + page-id (uuid/next) + shape-id (uuid/next) + child (cts/setup-shape {:id (uuid/next) :type :rect :name "Rectangle" + :parent-id uuid/zero :frame-id uuid/zero}) + commit (add-child-commit file-id page-id child)] + (open-with-new-child store file-id session-id page-id shape-id child) + (ptk/emit! store commit ::dps/force-persist) + (await (async/wait-for #(= 1 (count @requests)) "the new child is sent")) + (rx/push! response {:revn 0 + :lagged [(lagged-row file-id 1 repair-session-id + [(set-root-children page-id [shape-id])]) + (lagged-row file-id 2 session-id (:redo-changes @commit))]}) + (await (async/wait-for #(get-in @store [:persistence :index (:id @commit) ::dps/awaiting]) + "the saved commit waits for the repair")) + (t/is (zero? @reloads)) + (rx/push! timeout-s :tick) + (await (async/wait-for #(= 1 @reloads) "the file reloads")) + (t/is (empty? (get-in @store [:persistence :queue])) + "the saved commit is acknowledged"))))))) + (finally + (-add-method ptk/resolve ::dw/reload-current-file original))))) + ;; Commit boundaries. ;; Scenario: two edits are buffered and saved together. The request carries