mirror of
https://github.com/penpot/penpot.git
synced 2026-11-06 01:50:39 +00:00
🐛 Drop the unsaved edits made on a version a peer restored
When a peer restores a version, the file reloads to it. Edits made on the replaced version stayed queued: their save failed for good and locked the workspace, and a rollback or a rebase replayed them on the restored data. Loading another version forgets the queued, buffered and saved commits made on the old one. A save of such a commit, in flight or answered, is dropped instead of failing or being rolled back, and one save carries the edits of one version only.
This commit is contained in:
alonso.torres
parent
95584ea916
commit
c36216f9ec
@ -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.
|
||||
- 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.
|
||||
- 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.
|
||||
|
||||
@ -323,6 +323,16 @@
|
||||
(update-in [:remote-changes file-id] (fnil conj [])
|
||||
{:seq number :changes changes}))))))))
|
||||
|
||||
(defn- stale-commit?
|
||||
"True when `commit` was made on another version of its file than the
|
||||
loaded one, which a peer's restore replaced: its changes are not in the
|
||||
local data and the server refuses them. A file loaded with no version
|
||||
known is never stale."
|
||||
[state {:keys [file-id file-vern]}]
|
||||
(let [vern (:vern (dsh/lookup-file state file-id))]
|
||||
(and (some? vern)
|
||||
(not= vern file-vern))))
|
||||
|
||||
(defn- discard-commit
|
||||
[commit-id]
|
||||
(ptk/reify ::discard-commit
|
||||
@ -687,28 +697,35 @@
|
||||
(ptk/reify ::rollback-rejected
|
||||
ptk/UpdateEvent
|
||||
(update [_ state]
|
||||
(let [plan' (pcm/rollback-plan (:persistence state) entry rejected)]
|
||||
(vreset! plan plan')
|
||||
(update state :persistence pcm/without-rolled-back entry plan')))
|
||||
;; A save made on a version a restore replaced: the reloaded data
|
||||
;; holds none of it, so there is nothing to revert
|
||||
(if (stale-commit? state entry)
|
||||
state
|
||||
(let [plan' (pcm/rollback-plan (:persistence state) entry rejected)]
|
||||
(vreset! plan plan')
|
||||
(update state :persistence pcm/without-rolled-back entry plan'))))
|
||||
|
||||
ptk/WatchEvent
|
||||
(watch [it state stream]
|
||||
(apply-rollback-plan it state stream @plan))
|
||||
(if-let [plan @plan]
|
||||
(apply-rollback-plan it state stream plan)
|
||||
(rx/empty)))
|
||||
|
||||
ptk/EffectEvent
|
||||
(effect [_ _ _]
|
||||
(let [{:keys [error commit-ids]} rejected]
|
||||
(errors/flash :cause (ex-info "The server rejected some changes"
|
||||
(merge error
|
||||
{:file-id file-id
|
||||
:commit-ids commit-ids}
|
||||
(pcm/rollback-summary @plan)))
|
||||
:type :unhandled
|
||||
:level :warning
|
||||
:timeout rejected-timeout-ms
|
||||
:tag rejected-tag
|
||||
:report-link? true
|
||||
:hint (tr "errors.save-rejected-changes-undone")))))))
|
||||
(when-let [plan @plan]
|
||||
(let [{:keys [error commit-ids]} rejected]
|
||||
(errors/flash :cause (ex-info "The server rejected some changes"
|
||||
(merge error
|
||||
{:file-id file-id
|
||||
:commit-ids commit-ids}
|
||||
(pcm/rollback-summary plan)))
|
||||
:type :unhandled
|
||||
:level :warning
|
||||
:timeout rejected-timeout-ms
|
||||
:tag rejected-tag
|
||||
:report-link? true
|
||||
:hint (tr "errors.save-rejected-changes-undone"))))))))
|
||||
|
||||
(defn- update-file-request
|
||||
"Issues the `update-file` request, tracked as active for its lifetime."
|
||||
@ -736,6 +753,7 @@
|
||||
(nil? commit) :missing-commit
|
||||
(::acknowledged commit) :acknowledged
|
||||
(contains? @active-requests (::request-id commit)) :in-flight
|
||||
(stale-commit? state commit) :stale
|
||||
(not (dm/get-in state [:permissions :can-edit])) :permission-denied
|
||||
:else :ready)))
|
||||
|
||||
@ -750,6 +768,25 @@
|
||||
(assoc :hint-origin origin)))
|
||||
commits)))
|
||||
|
||||
(defn- save-attempt-failed
|
||||
"Routes a failed attempt to send `commit-id`, which took `elapsed-ms`. A
|
||||
commit made on a version a restore replaced is dropped instead: the
|
||||
reloaded file does not hold it and no resend can save it."
|
||||
[commit-id cause elapsed-ms]
|
||||
(ptk/reify ::save-attempt-failed
|
||||
ptk/WatchEvent
|
||||
(watch [_ state _]
|
||||
(let [commit (dm/get-in state [:persistence :index commit-id])]
|
||||
(rx/of (cond
|
||||
(and (some? commit) (stale-commit? state commit))
|
||||
(commit-persisted commit)
|
||||
|
||||
(transient-error? (ex-data cause))
|
||||
(persistence-transient-failure commit-id cause elapsed-ms)
|
||||
|
||||
:else
|
||||
(persistence-failed commit-id cause)))))))
|
||||
|
||||
(defn- send-queued-commit
|
||||
"Sends one queued commit and maps its outcome to persistence events."
|
||||
[request-id session-id {:keys [id file-id file-revn file-vern features] :as commit}]
|
||||
@ -790,10 +827,7 @@
|
||||
:code :invalid-save-response
|
||||
:file-id file-id})))))
|
||||
(rx/catch (fn [cause]
|
||||
(rx/of (if (transient-error? (ex-data cause))
|
||||
(persistence-transient-failure
|
||||
id cause (- (inst-ms (ct/now)) sent-at))
|
||||
(persistence-failed id cause))))))))
|
||||
(rx/of (save-attempt-failed id cause (- (inst-ms (ct/now)) sent-at))))))))
|
||||
|
||||
(defn- persist-commit
|
||||
"Sends the queued commit, stamping the attempt first. A commit whose
|
||||
@ -836,6 +870,8 @@
|
||||
:acknowledged (rx/of (commit-persisted commit))
|
||||
;; The replacement runner listens for the original request's result.
|
||||
:in-flight (rx/empty)
|
||||
;; Made on a version a restore replaced: never sent
|
||||
:stale (rx/of (commit-persisted commit))
|
||||
:permission-denied (fail :save-permission-denied "Edit permission was lost before changes could be saved")
|
||||
:ready (send-queued-commit request-id (:session-id state) commit))))))))
|
||||
|
||||
@ -980,6 +1016,19 @@
|
||||
(not= :error status)
|
||||
(not= :retrying status)))
|
||||
|
||||
(defn forget-other-versions
|
||||
"Forgets the commits of the freshly loaded `file` made on another version
|
||||
(`pcm/without-other-versions`), so no rollback or rebase replays them on
|
||||
the loaded data. A pending status with nothing left to save turns saved."
|
||||
[state {:keys [id vern]}]
|
||||
(update state :persistence
|
||||
(fn [pstate]
|
||||
(let [pstate (pcm/without-other-versions pstate id vern)]
|
||||
(cond-> pstate
|
||||
(and (= :pending (:status pstate))
|
||||
(nothing-to-save? pstate))
|
||||
(assoc :status :saved))))))
|
||||
|
||||
(defn pending-file-changes
|
||||
"Queued and buffered edits to restore into a freshly loaded file, oldest
|
||||
first. Only edits of the loaded version belong in its data. A
|
||||
@ -1108,8 +1157,9 @@
|
||||
(rx/empty)))))))
|
||||
|
||||
(defn- merge-commit
|
||||
"One queue entry per file and version: a save carries one version."
|
||||
[buffer]
|
||||
(->> (rx/from (group-by :file-id buffer))
|
||||
(->> (rx/from (group-by (juxt :file-id :file-vern) buffer))
|
||||
(rx/map (comp pcm/queue-entry val))))
|
||||
|
||||
(defn- flush-buffer
|
||||
@ -1238,7 +1288,8 @@
|
||||
(ptk/type? ::append-commit %)
|
||||
(ptk/type? ::discard-commit %)
|
||||
(ptk/type? ::rollback-rejected %)
|
||||
(ptk/type? ::release-failed-file %)))))
|
||||
(ptk/type? ::release-failed-file %)
|
||||
(ptk/type? ::dw/bundle-fetched %)))))
|
||||
(rx/map (fn [_] (sync-unload-guard)))
|
||||
(rx/take-until stoper-s))
|
||||
|
||||
|
||||
@ -55,6 +55,30 @@
|
||||
(fn [entries] (into [] (filter #(> (:seq %) since)) entries)))
|
||||
(update pstate :remote-changes dissoc file-id)))
|
||||
|
||||
(defn without-other-versions
|
||||
"`pstate` without the commits of `file-id` made on a version other than
|
||||
`vern`, the one just loaded: a restore replaced it, so they are not in
|
||||
the data and the server refuses them. Their queue entries stay, emptied,
|
||||
for the runner to settle the one in flight and skip the rest."
|
||||
[pstate file-id vern]
|
||||
(let [other? #(and (= file-id (:file-id %))
|
||||
(not= vern (:file-vern %)))
|
||||
emptied (into {}
|
||||
(keep (fn [id]
|
||||
(let [entry (get-in pstate [:index id])]
|
||||
(when (and (other? entry) (seq (:commits entry)))
|
||||
[id (assoc entry :commits [])]))))
|
||||
(:queue pstate))]
|
||||
(if (or (seq emptied)
|
||||
(some other? (:buffer pstate))
|
||||
(some other? (:saved pstate)))
|
||||
(-> pstate
|
||||
(update :index merge emptied)
|
||||
(update :buffer #(into [] (remove other?) %))
|
||||
(update :saved #(into [] (remove other?) %))
|
||||
(prune-remote-changes file-id))
|
||||
pstate)))
|
||||
|
||||
(defn- saved-commits
|
||||
"The `commits` of `entry` as saved commits, which carry their file."
|
||||
[entry commits]
|
||||
|
||||
@ -343,7 +343,8 @@
|
||||
(let [file (restore-pending-changes state file)]
|
||||
(-> state
|
||||
(assoc :thumbnails (d/update-vals thumbnails (fn [uri] {:uri uri :rendered-at nil})))
|
||||
(update :files assoc file-id file))))))
|
||||
(update :files assoc file-id file)
|
||||
(dps/forget-other-versions file))))))
|
||||
|
||||
(defn zoom-to-frame
|
||||
[]
|
||||
|
||||
@ -568,6 +568,16 @@
|
||||
(await (async/wait-for #(= :saved (get-in @store [:persistence :status]))
|
||||
"the rollback settles")))))))
|
||||
|
||||
;; Version restores.
|
||||
|
||||
(defn- restore-version
|
||||
"The load of `file-id` that a peer's restore causes: another version
|
||||
with `data`."
|
||||
[file-id data]
|
||||
(#'dw/bundle-fetched {:file {:id file-id :revn 0 :vern 1 :data data}
|
||||
:file-id file-id
|
||||
:thumbnails {}}))
|
||||
|
||||
;; Scenario: the undo of a rejected rename cannot be applied, so the
|
||||
;; rollback reloads the file. Proves: `::rolled-back` waits for the
|
||||
;; reloaded data, so a listener that reads the listed shapes sees the
|
||||
@ -613,3 +623,69 @@
|
||||
(finally
|
||||
(-add-method ptk/resolve ::dw/reload-current-file original)))))
|
||||
|
||||
;; Scenario: a save of two renames is in flight and a third rename waits in
|
||||
;; the queue when a peer restores an earlier version; the file reloads to
|
||||
;; it and a peer's change to it arrives. The answer then comes: the server
|
||||
;; kept the first rename and rejected the second, before the restore.
|
||||
;; Proves: neither the peer's change, the answer nor its rollback replays
|
||||
;; the renames on the reloaded data, the user gets no rejection warning,
|
||||
;; and the queued rename, made on the replaced version, is dropped unsent.
|
||||
(t/deftest ^:async an-answer-from-before-a-restore-leaves-the-reloaded-file-alone
|
||||
(await
|
||||
(thp/with-persistence
|
||||
(^:async fn [{:keys [file-id response requests store failures]}]
|
||||
(let [page-id (uuid/next)
|
||||
shape-id (uuid/next)
|
||||
commit1 (thp/rename-commit file-id page-id shape-id "A" "B")
|
||||
commit2 (thp/rename-commit file-id page-id shape-id "B" "C")
|
||||
commit3 (thp/rename-commit file-id page-id shape-id "C" "D")]
|
||||
(ptk/emit! store
|
||||
(thp/open-file file-id page-id (thp/file-data page-id shape-id "D"))
|
||||
#(assoc-in % [:files file-id :vern] 0))
|
||||
(ptk/emit! store commit1 commit2 ::dps/force-persist)
|
||||
(await (async/wait-for #(= 1 (count @requests)) "the first two renames are sent"))
|
||||
(ptk/emit! store commit3 ::dps/force-persist)
|
||||
(await (async/wait-for #(= 2 (count (get-in @store [:persistence :queue])))
|
||||
"the third rename is queued"))
|
||||
|
||||
(ptk/emit! store (restore-version file-id (thp/file-data page-id shape-id "Restored")))
|
||||
(ptk/emit! store (thp/remote-commit file-id 1 [(thp/set-attr page-id shape-id :opacity 0.5)]))
|
||||
(await (async/settle))
|
||||
(t/is (= "Restored" (thp/shape-name store file-id page-id shape-id))
|
||||
"a peer's change does not replay the renames")
|
||||
(rx/push! response (thp/rejection (:id @commit2)))
|
||||
(await (async/wait-for #(and (= :saved (get-in @store [:persistence :status]))
|
||||
(empty? (get-in @store [:persistence :queue])))
|
||||
"the queue drains"))
|
||||
(t/is (= "Restored" (thp/shape-name store file-id page-id shape-id))
|
||||
"the file keeps the restored version")
|
||||
(t/is (= 0.5 (thp/shape-attr store file-id page-id shape-id :opacity)))
|
||||
(t/is (empty? @failures) "no rejection warning for a replaced version")
|
||||
(t/is (= 1 (count @requests)) "the rename made on the replaced version is not sent"))))))
|
||||
|
||||
;; Scenario: a save is in flight when a peer restores an earlier version and
|
||||
;; the file reloads; the server handled the save after the restore, so it
|
||||
;; refuses it for its old version. Proves: the failure of a save made on a
|
||||
;; replaced version drops it instead of halting the queue and locking the
|
||||
;; workspace.
|
||||
(t/deftest ^:async a-save-refused-after-a-restore-is-dropped
|
||||
(await
|
||||
(thp/with-persistence
|
||||
(^:async fn [{:keys [file-id response requests store failures]}]
|
||||
(let [page-id (uuid/next)
|
||||
shape-id (uuid/next)]
|
||||
(ptk/emit! store
|
||||
(thp/open-file file-id page-id (thp/file-data page-id shape-id "B"))
|
||||
#(assoc-in % [:files file-id :vern] 0))
|
||||
(ptk/emit! store (thp/rename-commit file-id page-id shape-id "A" "B") ::dps/force-persist)
|
||||
(await (async/wait-for #(= 1 (count @requests)) "the rename is sent"))
|
||||
(ptk/emit! store (restore-version file-id (thp/file-data page-id shape-id "Restored")))
|
||||
(rx/error! response (ex-info "A different version has been restored for the file."
|
||||
{:type :validation :code :vern-conflict}))
|
||||
(await (async/wait-for #(and (= :saved (get-in @store [:persistence :status]))
|
||||
(empty? (get-in @store [:persistence :queue])))
|
||||
"the refused save is dropped"))
|
||||
(t/is (empty? @failures))
|
||||
(t/is (nil? (get-in @store [:workspace-global :persistence-lock])))
|
||||
(t/is (= "Restored" (thp/shape-name store file-id page-id shape-id))))))))
|
||||
|
||||
|
||||
@ -81,8 +81,12 @@
|
||||
(when restored?
|
||||
(t/is (= "Retained rectangle" (:name restored))
|
||||
"queued edits apply in order and exclude other files"))
|
||||
(t/is (identical? pstate (:persistence loaded))
|
||||
"restoring the editor must preserve the save queue")))))
|
||||
(if (zero? vern)
|
||||
(t/is (identical? pstate (:persistence loaded))
|
||||
"restoring the editor must preserve the save queue")
|
||||
(t/is (= [[] [] (get-in pstate [:index other-id :commits])]
|
||||
(mapv #(get-in loaded [:persistence :index % :commits]) [first-id next-id other-id]))
|
||||
"the commits made on the replaced version are forgotten"))))))
|
||||
|
||||
;; Scenario: a file loads again while an edit of it waits in the buffer.
|
||||
;; Proves: buffered edits apply too, after the queued ones, and those of
|
||||
|
||||
Loading…
x
Reference in New Issue
Block a user