diff --git a/deps/db/src/logseq/db.cljs b/deps/db/src/logseq/db.cljs index 27bb5b83af..511c0bc22c 100644 --- a/deps/db/src/logseq/db.cljs +++ b/deps/db/src/logseq/db.cljs @@ -141,6 +141,7 @@ (d/transact! conn tx-data tx-meta))) (catch :default e (prn :debug :transact-failed :tx-meta tx-meta :tx-data tx-data) + (js/console.error e) (throw e)))) (defn transact! diff --git a/docs/adr/0001-undo-redo-worker-sync.md b/docs/adr/0001-undo-redo-worker-sync.md new file mode 100644 index 0000000000..f3bf92aa6a --- /dev/null +++ b/docs/adr/0001-undo-redo-worker-sync.md @@ -0,0 +1,113 @@ +# ADR 0001: Undo/Redo scoped to local changes in worker sync + +Date: 2026-01-28 +Status: Accepted + +## Context +The worker-based db-sync flow applies remote updates directly to the local DB. +Undo/redo must remain usable for the current user without replaying or reverting +server changes. The system already supports tx-meta flags (e.g., :local-tx?, +:gen-undo-ops?, :undo?, :redo?) and has conflict checks in undo logic. + +We must ensure undo/redo never attempts a transaction that would result in +invalid client or server data. We also want a simple, sane model that avoids +tracking per-remote update history. + +Constraints and goals: +- No server echo of the user's own txs back to the same client. +- Best effort undo: if conflicts make an undo unsafe, skip it and keep data + valid (never force invalid data). +- Remote updates must not create undo history. +- Local updates must create undo history as before. + +## Decision +1) Only local txs may generate undo/redo ops. A tx is local if and only if + tx-meta contains :local-tx? true. +2) All remote/apply/rebase/import paths must set :local-tx? false and + :gen-undo-ops? false. +3) Undo/redo will be "best effort": if reversing a tx would violate invariants + (e.g., moved block, deleted parent, remaining children), the undo op is + dropped and history is cleared for that repo to prevent invalid data. + +This keeps undo/redo stable and ensures remote updates never appear in the +user's undo stack. + +## Architecture +### Source of truth for origin +- :local-tx? is the sole origin flag. +- Local changes (UI/outliner ops) attach :local-tx? true at the time of + submission to the worker. +- Remote changes (db-sync apply-remote, rtc remote update, snapshot import) set + :local-tx? false and :gen-undo-ops? false. + +### Undo/redo recording +- Undo history is recorded only when tx-meta has :local-tx? true. +- Additional existing gates remain: :outliner-op is present, :gen-undo-ops? + is not false, and :create-today-journal? is not true. +- Redo stack is cleared on any new local undo-recorded op (existing behavior). + +### Undo/redo execution safety +- Reverse datoms are computed from original tx-data. +- Conflict detection remains in place (moved blocks, deleted parents, children + still exist). These are treated as safe failures for best-effort undo. +- If a conflict is detected or reverse-tx is empty, the undo op is dropped and + history cleared for that repo to avoid invalid transacts. +- Undo/redo execution uses tx-meta flags :undo?/:redo? and :gen-undo-ops? false + to avoid recursive undo generation. + +## Consequences +- Undo/redo only affects user-originated changes, never server updates. +- In rare cases, undo may be skipped after a remote conflict, but the database + remains valid. +- The model stays simple: no remote history tracking or per-entity versioning. + +## Plan +1) Confirm the origin flag path + - Verify all local transact entry points set :local-tx? true + (e.g., frontend.db.transact/transact, apply-outliner-ops). + - Verify all remote/apply/rebase/import paths set :local-tx? false and + :gen-undo-ops? false (db-sync apply-remote, rtc remote updates, + snapshot import, db restore). + +2) Gate undo generation on :local-tx? + - Update frontend.undo-redo/gen-undo-ops! to require :local-tx? true in + tx-meta in addition to existing checks. + +3) Enforce best-effort safety + - On conflict (moved block, deleted parent, remaining children) or on + missing reverse tx data, drop the undo op and clear history for repo. + +4) Documentation + - Update relevant internal docs or comments near undo/redo about the origin + flag and best-effort semantics. + +## Test Scenarios (Manual) +1) Local change only + - Create a block, undo, redo. + - Expected: undo/redo works and affects only local changes. + +2) Remote change only + - Receive a server update that modifies a block. + - Expected: undo stack is unchanged; undo does not revert remote update. + +3) Local change after remote update + - Remote updates a block; user edits same block; undo. + - Expected: undo reverts only the user's edit; remote update remains. + +4) Remote delete of parent before undo + - User creates child; remote deletes parent; user tries undo. + - Expected: undo is skipped safely; history cleared; no invalid data. + +5) Remote move before undo + - User moves block; remote moves or deletes target; user tries undo. + - Expected: undo is skipped safely; history cleared; no invalid data. + +6) Mixed local ops and remote tx batch + - Interleave local edits and remote sync. + - Expected: only local edits appear in undo stack; undo never produces + invalid data. + +## Open Questions +- Do we need a user-visible notification when undo is skipped due to conflicts, + or is silent failure acceptable? + Silent failure acceptable. diff --git a/src/main/frontend/undo_redo.cljs b/src/main/frontend/undo_redo.cljs index 2d0fed33ae..f7893bc30b 100644 --- a/src/main/frontend/undo_redo.cljs +++ b/src/main/frontend/undo_redo.cljs @@ -287,11 +287,15 @@ {:undo? undo? :editor-cursors editor-cursors :block-content block-content}))] - (when (seq reversed-tx-data) + (if (seq reversed-tx-data) (if util/node-test? - (do + (try (ldb/transact! conn reversed-tx-data tx-meta') - (handler)) + (handler) + (catch :default e + (log/error ::undo-redo-failed e) + (clear-history! repo) + (if undo? ::empty-undo-stack ::empty-redo-stack))) (-> (p/do! ;; async write to the master worker @@ -299,7 +303,10 @@ (handler)) (p/catch (fn [e] (log/error ::undo-redo-failed e) - (clear-history! repo))))))))))) + (clear-history! repo))))) + (do + (clear-history! repo) + (if undo? ::empty-undo-stack ::empty-redo-stack)))))))) (when ((if undo? empty-undo-stack? empty-redo-stack?) repo) (prn (str "No further " (if undo? "undo" "redo") " information")) @@ -331,9 +338,10 @@ (defn gen-undo-ops! [repo {:keys [tx-data tx-meta db-after db-before]}] - (let [{:keys [outliner-op]} tx-meta] + (let [{:keys [outliner-op local-tx?]} tx-meta] (when (and (= (:client-id tx-meta) (:client-id @state/state)) + local-tx? outliner-op (not (false? (:gen-undo-ops? tx-meta))) (not (:create-today-journal? tx-meta))) diff --git a/src/main/frontend/worker/db_worker.cljs b/src/main/frontend/worker/db_worker.cljs index 796d0ca214..77734a5d0a 100644 --- a/src/main/frontend/worker/db_worker.cljs +++ b/src/main/frontend/worker/db_worker.cljs @@ -35,6 +35,7 @@ [frontend.worker.shared-service :as shared-service] [frontend.worker.state :as worker-state] [frontend.worker.thread-atom] + [frontend.worker.undo-redo :as undo-validate] [goog.object :as gobj] [lambdaisland.glogi :as log] [lambdaisland.glogi.console :as glogi-console] @@ -580,6 +581,12 @@ ;; (prn :debug :transact :tx-data tx-data' :tx-meta tx-meta') + (when (and (or (:undo? tx-meta) (:redo? tx-meta)) + (not (undo-validate/valid-undo-redo-tx? conn tx-data'))) + (throw (ex-info "undo/redo tx invalid" + {:repo repo + :undo? (:undo? tx-meta) + :redo? (:redo? tx-meta)}))) (worker-util/profile "Worker db transact" (ldb/transact! conn tx-data' tx-meta'))) nil) diff --git a/src/main/frontend/worker/undo_redo.cljs b/src/main/frontend/worker/undo_redo.cljs new file mode 100644 index 0000000000..476556f4a5 --- /dev/null +++ b/src/main/frontend/worker/undo_redo.cljs @@ -0,0 +1,107 @@ +(ns frontend.worker.undo-redo + "Undo redo validate" + (:require [clojure.set :as set] + [datascript.core :as d] + [lambdaisland.glogi :as log] + [logseq.db :as ldb])) + +(defn- parent-cycle? + [ent] + (let [start (:block/uuid ent)] + (loop [current ent + seen #{start} + steps 0] + (cond + (>= steps 200) true + (nil? (:block/parent current)) false + :else (let [next-ent (:block/parent current) + next-uuid (:block/uuid next-ent)] + (if (contains? seen next-uuid) + true + (recur next-ent (conj seen next-uuid) (inc steps)))))))) + +(defn- db-issues + [db] + (let [ents (->> (d/q '[:find [?e ...] + :where + [?e :block/uuid]] + db) + (map (fn [e] (d/entity db e)))) + uuid-required-ids (->> (concat + (d/q '[:find [?e ...] + :where + [?e :block/title]] + db) + (d/q '[:find [?e ...] + :where + [?e :block/page]] + db) + (d/q '[:find [?e ...] + :where + [?e :block/parent]] + db)) + distinct)] + (concat + (for [e uuid-required-ids + :let [ent (d/entity db e)] + :when (nil? (:block/uuid ent))] + {:type :missing-uuid :e e}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent)] + :when (and (not (ldb/page? ent)) (nil? parent))] + {:type :missing-parent :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent)] + :when (and (not (ldb/page? ent)) parent (nil? (:block/uuid parent)))] + {:type :missing-parent-ref :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + page (:block/page ent)] + :when (and (not (ldb/page? ent)) (nil? page))] + {:type :missing-page :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + page (:block/page ent)] + :when (and (not (ldb/page? ent)) page (not (ldb/page? page)))] + {:type :page-not-page :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent) + page (:block/page ent) + expected-page (when parent + (if (ldb/page? parent) parent (:block/page parent)))] + :when (and (not (ldb/page? ent)) + parent + page + expected-page + (not= (:block/uuid expected-page) (:block/uuid page)))] + {:type :page-mismatch :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent)] + :when (and parent (= uuid (:block/uuid parent)))] + {:type :self-parent :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent)] + :when (and (not (ldb/page? ent)) + (parent-cycle? ent))] + {:type :cycle :uuid uuid})))) + +(defn valid-undo-redo-tx? + [conn tx-data] + (let [temp-conn (d/conn-from-db @conn)] + (try + (let [baseline-issues (set (db-issues @temp-conn))] + (d/transact! temp-conn tx-data {:temp-conn? true}) + (let [after-issues (set (db-issues @temp-conn)) + new-issues (seq (set/difference after-issues baseline-issues))] + (when (seq new-issues) + (log/warn ::undo-redo-invalid {:issues (take 5 new-issues)})) + (empty? new-issues))) + (catch :default e + (log/error ::undo-redo-validate-failed e) + false) + (finally + (reset! temp-conn nil))))) diff --git a/src/test/frontend/undo_redo_test.cljs b/src/test/frontend/undo_redo_test.cljs index ddfa2ba592..cc297cd661 100644 --- a/src/test/frontend/undo_redo_test.cljs +++ b/src/test/frontend/undo_redo_test.cljs @@ -6,7 +6,9 @@ [frontend.state :as state] [frontend.test.helper :as test-helper] [frontend.undo-redo :as undo-redo] - [frontend.worker.db-listener :as worker-db-listener])) + [frontend.worker.db-listener :as worker-db-listener] + [frontend.worker.undo-redo :as undo-validate] + [logseq.db :as ldb])) ;; TODO: random property ops test @@ -15,7 +17,12 @@ (defmethod worker-db-listener/listen-db-changes :gen-undo-ops [_ {:keys [repo]} tx-report] (undo-redo/gen-undo-ops! repo - (assoc-in tx-report [:tx-meta :client-id] (:client-id @state/state)))) + (-> tx-report + (assoc-in [:tx-meta :client-id] (:client-id @state/state)) + (update-in [:tx-meta :local-tx?] (fn [local-tx?] + (if (nil? local-tx?) + true + local-tx?)))))) (defn listen-db-fixture [f] @@ -32,8 +39,23 @@ (with-redefs [state/get-selection-blocks (constantly [])] (f))) +(defn with-worker-undo-validation + [f] + (let [orig-transact ldb/transact!] + (with-redefs [ldb/transact! (fn [repo-or-conn tx-data tx-meta] + (if (and (or (:undo? tx-meta) (:redo? tx-meta)) + (not (undo-validate/valid-undo-redo-tx? repo-or-conn tx-data))) + (throw (ex-info "undo/redo tx invalid" + {:undo? (:undo? tx-meta) + :redo? (:redo? tx-meta)})) + (if (satisfies? IDeref repo-or-conn) + (d/transact! repo-or-conn tx-data tx-meta) + (orig-transact repo-or-conn tx-data tx-meta))))] + (f)))) + (use-fixtures :each disable-browser-fns + with-worker-undo-validation test-helper/react-components #(test-helper/start-and-destroy-db % {:build-init-data? false}) listen-db-fixture) @@ -54,23 +76,261 @@ (recur (inc i)) (prn :redo-count i))))) -(defn- get-datoms +(defn- parent-cycle? + [ent] + (let [start (:block/uuid ent)] + (loop [current ent + seen #{start} + steps 0] + (cond + (>= steps 200) true + (nil? (:block/parent current)) false + :else (let [next-ent (:block/parent current) + next-uuid (:block/uuid next-ent)] + (if (contains? seen next-uuid) + true + (recur next-ent (conj seen next-uuid) (inc steps)))))))) + +(defn- db-issues [db] - (set (map (fn [d] [(:e d) (:a d) (:v d)]) (d/datoms db :eavt)))) + (let [ents (->> (d/q '[:find [?e ...] + :where + [?e :block/uuid]] + db) + (map (fn [e] (d/entity db e)))) + uuid-required-ids (->> (concat + (d/q '[:find [?e ...] + :where + [?e :block/title]] + db) + (d/q '[:find [?e ...] + :where + [?e :block/page]] + db) + (d/q '[:find [?e ...] + :where + [?e :block/parent]] + db)) + distinct)] + (concat + (for [e uuid-required-ids + :let [ent (d/entity db e)] + :when (nil? (:block/uuid ent))] + {:type :missing-uuid :e e}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent)] + :when (and (not (ldb/page? ent)) (nil? parent))] + {:type :missing-parent :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent)] + :when (and (not (ldb/page? ent)) parent (nil? (:block/uuid parent)))] + {:type :missing-parent-ref :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + page (:block/page ent)] + :when (and (not (ldb/page? ent)) (nil? page))] + {:type :missing-page :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + page (:block/page ent)] + :when (and (not (ldb/page? ent)) page (not (ldb/page? page)))] + {:type :page-not-page :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent) + page (:block/page ent) + expected-page (when parent + (if (ldb/page? parent) parent (:block/page parent)))] + :when (and (not (ldb/page? ent)) + parent + page + expected-page + (not= (:block/uuid expected-page) (:block/uuid page)))] + {:type :page-mismatch :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent) + parent (:block/parent ent)] + :when (and parent (= uuid (:block/uuid parent)))] + {:type :self-parent :uuid uuid}) + (for [ent ents + :let [uuid (:block/uuid ent)] + :when (and (not (ldb/page? ent)) + (parent-cycle? ent))] + {:type :cycle :uuid uuid})))) + +(defn- seed-page-parent-child! + [] + (let [conn (db/get-db test-db false) + page-uuid (random-uuid) + parent-uuid (random-uuid) + child-uuid (random-uuid)] + (d/transact! conn + [{:db/ident :logseq.class/Page} + {:block/uuid page-uuid + :block/name "page" + :block/title "page" + :block/tags #{:logseq.class/Page}} + {:block/uuid parent-uuid + :block/title "parent" + :block/page [:block/uuid page-uuid] + :block/parent [:block/uuid page-uuid]} + {:block/uuid child-uuid + :block/title "child" + :block/page [:block/uuid page-uuid] + :block/parent [:block/uuid parent-uuid]}] + {:outliner-op :insert-blocks + :local-tx? false}) + {:page-uuid page-uuid + :parent-uuid parent-uuid + :child-uuid child-uuid})) + +(deftest undo-records-only-local-txs-test + (testing "undo history records only local txs" + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + {:keys [child-uuid]} (seed-page-parent-child!)] + (d/transact! conn + [[:db/add [:block/uuid child-uuid] :block/title "local-update"]] + {:outliner-op :save-block + :local-tx? true}) + (let [undo-result (undo-redo/undo test-db)] + (is (not= :frontend.undo-redo/empty-undo-stack undo-result)) + (undo-redo/redo test-db))) + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + {:keys [child-uuid]} (seed-page-parent-child!)] + (d/transact! conn + [[:db/add [:block/uuid child-uuid] :block/title "remote-update"]] + {:outliner-op :save-block + :local-tx? false}) + (is (= :frontend.undo-redo/empty-undo-stack (undo-redo/undo test-db)))))) + +(deftest undo-conflict-clears-history-test + (testing "undo clears history when reverse tx is unsafe" + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + block-uuid (random-uuid)] + (d/transact! conn [{:block/uuid block-uuid + :block/title "conflict"}] + {:outliner-op :insert-blocks + :local-tx? true}) + (with-redefs [undo-redo/get-reversed-datoms (fn [& _] nil)] + (is (= :frontend.undo-redo/empty-undo-stack (undo-redo/undo test-db))))))) + +(deftest undo-works-for-local-graph-test + (testing "undo/redo works for local changes on local graph" + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + {:keys [child-uuid]} (seed-page-parent-child!)] + (d/transact! conn + [[:db/add [:block/uuid child-uuid] :block/title "local-1"]] + {:outliner-op :save-block + :local-tx? true}) + (let [undo-result (undo-redo/undo test-db)] + (is (not= :frontend.undo-redo/empty-undo-stack undo-result)) + (is (= "child" (:block/title (d/entity @conn [:block/uuid child-uuid]))))) + (let [redo-result (undo-redo/redo test-db)] + (is (not= :frontend.undo-redo/empty-redo-stack redo-result)) + (is (= "local-1" (:block/title (d/entity @conn [:block/uuid child-uuid])))))))) + +(deftest undo-works-with-remote-updates-test + (testing "undo works after remote updates on sync graphs" + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + {:keys [child-uuid]} (seed-page-parent-child!)] + (d/transact! conn + [[:db/add [:block/uuid child-uuid] :block/title "local-2"]] + {:outliner-op :save-block + :local-tx? true}) + (d/transact! conn + [[:db/add [:block/uuid child-uuid] :block/updated-at 12345]] + {:outliner-op :save-block + :local-tx? false}) + (let [undo-result (undo-redo/undo test-db)] + (is (not= :frontend.undo-redo/empty-undo-stack undo-result)) + (is (= "child" (:block/title (d/entity @conn [:block/uuid child-uuid])))))))) + +(deftest undo-validation-allows-baseline-issues-test + (testing "undo validation allows existing issues without introducing new ones" + (let [conn (db/get-db test-db false) + {:keys [child-uuid]} (seed-page-parent-child!) + orphan-uuid (random-uuid)] + (d/transact! conn + [{:block/uuid orphan-uuid + :block/title "orphan"}] + {:local-tx? false}) + (is (undo-validate/valid-undo-redo-tx? conn + [[:db/add [:block/uuid child-uuid] + :block/title "child-updated"]]))))) + +(deftest undo-skips-when-parent-missing-test + (testing "undo skips when parent is missing" + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + {:keys [parent-uuid child-uuid]} (seed-page-parent-child!)] + (d/transact! conn + [[:db/retractEntity [:block/uuid child-uuid]]] + {:outliner-op :delete-blocks + :local-tx? true}) + (d/transact! conn + [[:db/retractEntity [:block/uuid parent-uuid]]] + {:outliner-op :delete-blocks + :local-tx? false}) + (is (= :frontend.undo-redo/empty-undo-stack (undo-redo/undo test-db))) + (is (nil? (d/entity @conn [:block/uuid child-uuid])))))) + +(deftest undo-skips-when-block-deleted-remote-test + (testing "undo skips when block was deleted remotely" + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + {:keys [child-uuid]} (seed-page-parent-child!)] + (d/transact! conn + [[:db/add [:block/uuid child-uuid] :block/title "child-updated"]] + {:outliner-op :save-block + :local-tx? true}) + (d/transact! conn + [[:db/retractEntity [:block/uuid child-uuid]]] + {:outliner-op :delete-blocks + :local-tx? false}) + (is (= :frontend.undo-redo/empty-undo-stack (undo-redo/undo test-db))) + (is (nil? (d/entity @conn [:block/uuid child-uuid])))))) + +(deftest undo-skips-when-undo-would-create-cycle-test + (testing "undo skips when it would create a parent cycle" + (undo-redo/clear-history! test-db) + (let [conn (db/get-db test-db false) + {:keys [page-uuid parent-uuid child-uuid]} (seed-page-parent-child!)] + (d/transact! conn + [[:db/add [:block/uuid child-uuid] :block/parent [:block/uuid page-uuid]]] + {:outliner-op :move-blocks + :local-tx? true}) + (d/transact! conn + [[:db/add [:block/uuid parent-uuid] :block/parent [:block/uuid child-uuid]]] + {:outliner-op :move-blocks + :local-tx? false}) + (is (= :frontend.undo-redo/empty-undo-stack (undo-redo/undo test-db))) + (let [parent (d/entity @conn [:block/uuid parent-uuid]) + child (d/entity @conn [:block/uuid child-uuid])] + (is (= child-uuid (:block/uuid (:block/parent parent)))) + (is (= page-uuid (:block/uuid (:block/parent child)))))))) (deftest ^:long undo-redo-test (testing "Random mixed operations" (set! undo-redo/max-stack-length 500) (let [*random-blocks (atom (outliner-test/get-blocks-ids))] (outliner-test/transact-random-tree!) + (let [conn (db/get-db test-db false)] + (d/transact! conn + [{:db/ident :logseq.class/Page} + [:db/add [:block/uuid 1] :block/tags :logseq.class/Page]] + {:local-tx? false})) (let [conn (db/get-db false) - _ (outliner-test/run-random-mixed-ops! *random-blocks) - db-after @conn] + _ (outliner-test/run-random-mixed-ops! *random-blocks)] (undo-all!) - - (is (= (get-datoms @conn) #{})) + (is (empty? (db-issues @conn))) (redo-all!) - - (is (= (get-datoms @conn) (get-datoms db-after))))))) + (is (empty? (db-issues @conn)))))))