refactor: RTC reuse :db/id for deleted blocks when undo/redo (#12101)

1. reuse old eids for block/uuid when apply-remote-ops

2. enhance: clear undo history when failed to validate db

Also, throw transact error from db worker
3. test(rtc): add timeout for wait-for cloud-idle
4. fix: rtc extra tests
5.  enhance(test): retry new-block once when assert timeout
---------

Co-authored-by: rcmerci <[email protected]>

clj-e2e/src/logseq/e2e/block.clj

@@ -9,15 +9,22 @@
 
 (defn open-last-block
   "Open the last existing block or pressing add button to create a new block"
-  []
+  [& {:keys [retry?]}]
   (util/double-esc)
   (assert/assert-in-normal-mode?)
+
   (let [blocks-count (util/page-blocks-count)
         last-block (-> (if (zero? blocks-count)
                          (w/query ".ls-page-blocks .block-add-button")
                          (w/query ".ls-page-blocks .page-blocks-inner .ls-block .block-content"))
                        (last))]
-    (w/click last-block)))
+    (w/click last-block)
+    (if retry?
+      (assert/assert-editor-mode)
+      (try
+        (assert/assert-editor-mode)
+        (catch Error _e
+          (open-last-block {:retry? true}))))))
 
 (defn save-block
   [text]
@@ -30,7 +37,6 @@
   [title]
   (let [editor (util/get-editor)]
     (when-not editor (open-last-block))
-    (assert/assert-editor-mode)
     (let [last-id (.getAttribute (w/-query ".editor-wrapper textarea") "id")]
       (is (some? last-id))
       (k/press "Control+e")
@@ -47,7 +53,6 @@
   [titles]
   (let [editor? (util/get-editor)]
     (when-not editor? (open-last-block))
-    (assert/assert-editor-mode)
     (let [value (util/get-edit-content)]
       (if (string/blank? value)         ; empty block
         (save-block (first titles))

clj-e2e/src/logseq/e2e/rtc.clj

@@ -21,7 +21,7 @@
      (loop [i# 5]
        (when (zero? i#) (throw (ex-info "wait-tx-updated failed" {:old m# :new (get-rtc-tx)})))
        (util/wait-timeout 500)
-       (w/wait-for "button.cloud.on.idle")
+       (w/wait-for "button.cloud.on.idle" {:timeout 35000})
        (util/wait-timeout 1000)
        (let [new-m# (get-rtc-tx)
              new-local-tx# (or (:local-tx new-m#) 0)

deps/db/src/logseq/db.cljs

@@ -102,12 +102,13 @@
        ;; (cljs.pprint/pprint tx-data)
        ;; (js/console.trace)
 
-       (let [f (or @*transact-fn d/transact!)]
+       (if-let [transact-fn @*transact-fn]
+         (transact-fn repo-or-conn tx-data tx-meta)
          (try
-           (f repo-or-conn tx-data tx-meta)
+           (d/transact! repo-or-conn tx-data tx-meta)
            (catch :default e
              (js/console.trace)
-             (prn :debug-tx-data tx-data)
+             (prn :debug :transact-failed :tx-meta tx-meta :tx-data tx-data)
              (throw e))))))))
 
 (def page? common-entity-util/page?)

deps/outliner/src/logseq/outliner/core.cljs

@@ -410,19 +410,22 @@
 
 (defn- assign-temp-id
   [blocks replace-empty-target? target-block]
-  (->> (map-indexed (fn [idx block]
-                      (let [replacing-block? (and replace-empty-target? (zero? idx))]
-                        (if replacing-block?
-                          (let [db-id (or (:db/id block) (dec (- idx)))]
-                            (if (seq (:block/_parent target-block)) ; target-block has children
+  (->> blocks
+       (map-indexed
+        (fn [idx block]
+          (let [replacing-block? (and replace-empty-target? (zero? idx))
+                db-id (or (when (:block.temp/use-old-db-id? block)
+                            (:db/id block))
+                          (dec (- idx)))]
+            (if replacing-block?
+              (if (seq (:block/_parent target-block)) ; target-block has children
                               ;; update block properties
-                              [(assoc block
-                                      :db/id (:db/id target-block)
-                                      :block/uuid (:block/uuid target-block))]
-                              [[:db/retractEntity (:db/id target-block)] ; retract target-block first
-                               (assoc block
-                                      :db/id db-id)]))
-                          [(assoc block :db/id (dec (- idx)))]))) blocks)
+                [(assoc block
+                        :db/id (:db/id target-block)
+                        :block/uuid (:block/uuid target-block))]
+                [[:db/retractEntity (:db/id target-block)] ; retract target-block first
+                 (assoc block :db/id db-id)])
+              [(assoc block :db/id db-id)]))))
        (apply concat)))
 
 (defn- get-id
@@ -604,8 +607,10 @@
                                                                                 (page-ref/->page-ref (uuids id))))
                                                               value
                                                               template-ref-block-ids)
-                                                             value)))
-                                    (dissoc :db/id))
+                                                             value))))
+                           result* (if (:block.temp/use-old-db-id? result*)
+                                     result*
+                                     (dissoc result* :db/id))
                            page? (or (ldb/page? block) (:block/name block))
                            result (cond-> result*
                                     (not page?)
@@ -754,17 +759,17 @@
                        (if-let [eid (or (:db/id b)
                                         (when-let [id (:block/uuid b)]
                                           [:block/uuid id]))]
-                         (let [b (if-let [e (if (de/entity? b) b (d/entity db eid))]
-                                   (merge
-                                    (into {} e)
-                                    {:db/id (:db/id e)
-                                     :block/title (or (:block/raw-title e) (:block/title e))}
+                         (let [b' (if-let [e (if (de/entity? b) b (d/entity db eid))]
+                                    (merge
+                                     (into {} e)
+                                     {:db/id (:db/id e)
+                                      :block/title (or (:block/raw-title e) (:block/title e))}
+                                     b)
                                     b)
-                                   b)
                                dissoc-keys (concat [:block/tx-id]
                                                    (when (contains? #{:insert-template-blocks :paste} outliner-op)
                                                      [:block/refs]))]
-                           (apply dissoc b dissoc-keys))
+                           (apply dissoc b' dissoc-keys))
                          b))
                      blocks)
         [target-block sibling?] (get-target-block db blocks target-block opts)
@@ -800,10 +805,14 @@
                            :tx (vec blocks-tx)
                            :blocks (vec blocks)
                            :target-block target-block}))
-          (let [uuids-tx (->> (map :block/uuid blocks-tx)
+          (let [tx (assign-temp-id blocks-tx replace-empty-target? target-block)
+                old-db-id-blocks (->> (filter :block.temp/use-old-db-id? tx)
+                                      (map :block/uuid)
+                                      (set))
+                uuids-tx (->> (map :block/uuid blocks-tx)
+                              (remove old-db-id-blocks)
                               (remove nil?)
                               (map (fn [uuid'] {:block/uuid uuid'})))
-                tx (assign-temp-id blocks-tx replace-empty-target? target-block)
                 from-property (:logseq.property/created-from-property target-block)
                 many? (= :db.cardinality/many (:db/cardinality from-property))
                 property-values-tx (when (and sibling? from-property many?)

src/main/frontend/components/block.cljs

@@ -4413,7 +4413,7 @@
   (let [mobile? (util/mobile?)
         [ready? set-ready?] (hooks/use-state
                              (not (and mobile? (not (:journals? config)))))
-        [virtualized? _] (hooks/use-state (not (or (string/includes? js/window.location.search "?rtc-test=true")
+        [virtualized? _] (hooks/use-state (not (or (util/rtc-test?)
                                                    (if (:journals? config)
                                                      (< (count blocks) 50)
                                                      (< (count blocks) 10))

src/main/frontend/handler/editor.cljs

@@ -842,12 +842,6 @@
                     (p/do!
                      (ui-outliner-tx/transact!
                       transact-opts
-                      (when (= (:db/id current-block) (:db/id (:block/parent next-block)))
-                        (property-handler/set-block-properties!
-                         repo
-                         (:block/uuid next-block)
-                         {:block/parent (:db/id (:block/parent current-block))
-                          :block/order (:block/order current-block)}))
 
                       (when (seq children)
                         (outliner-op/move-blocks!
@@ -855,6 +849,12 @@
                          next-block
                          {:sibling? false}))
 
+                      (when (= (:db/id current-block) (:db/id (:block/parent next-block)))
+                        (outliner-op/move-blocks!
+                         [next-block]
+                         current-block
+                         {:sibling? true}))
+
                       (delete-block-aux! current-block))
                      (edit-block! (db/entity (:db/id next-block)) 0)))
 

src/main/frontend/handler/worker.cljs

@@ -5,6 +5,7 @@
             [frontend.handler.file-based.file :as file-handler]
             [frontend.handler.notification :as notification]
             [frontend.state :as state]
+            [frontend.undo-redo :as undo-redo]
             [lambdaisland.glogi :as log]
             [logseq.db :as ldb]
             [promesa.core :as p]))
@@ -44,6 +45,9 @@
 (defmethod handle :sync-db-changes [_ _worker data]
   (state/pub-event! [:db/sync-changes data]))
 
+(defmethod handle :clear-undo-history [_ _worker [repo]]
+  (undo-redo/clear-history! repo))
+
 (defmethod handle :rtc-log [_ _worker log]
   (state/pub-event! [:rtc/log log]))
 

src/main/frontend/undo_redo.cljs

@@ -5,6 +5,7 @@
             [frontend.db :as db]
             [frontend.state :as state]
             [frontend.util :as util]
+            [lambdaisland.glogi :as log]
             [logseq.common.defkeywords :refer [defkeywords]]
             [logseq.db :as ldb]
             [malli.core :as m]
@@ -53,6 +54,12 @@
 (defonce *undo-ops (atom {}))
 (defonce *redo-ops (atom {}))
 
+(defn clear-history!
+  [repo]
+  (prn :debug :clear-undo-history repo)
+  (swap! *undo-ops assoc repo [])
+  (swap! *redo-ops assoc repo []))
+
 (defn- conj-op
   [col op]
   (let [result (conj (if (empty? col) [] col) op)]
@@ -204,20 +211,12 @@
           e->datoms (->> (if redo? tx-data (reverse tx-data))
                          (group-by :e))
           schema (:schema @conn)
-          added-and-retracted-ids (set/union added-ids retracted-ids)
           moved-blocks (get-moved-blocks e->datoms)]
       (->>
        (mapcat
         (fn [[e datoms]]
           (let [entity (d/entity @conn e)]
             (cond
-              ;; entity has been deleted
-              (and (nil? entity)
-                   (not (contains? added-and-retracted-ids e)))
-              (throw (ex-info "Entity has been deleted"
-                              (merge op {:error :entity-deleted
-                                         :undo? undo?})))
-
               ;; new children blocks have been added
               (and
                (not (:local-tx? tx-meta))
@@ -247,8 +246,7 @@
        (remove nil?)))
     (catch :default e
       (prn :debug :undo-redo :error (:error (ex-data e)))
-      (when-not (contains? #{:entity-deleted
-                             :block-moved-or-target-deleted
+      (when-not (contains? #{:block-moved-or-target-deleted
                              :block-children-exists}
                            (:error (ex-data e)))
         (throw e)))))
@@ -275,7 +273,8 @@
                                        :batch-tx/batch-tx-mode?)
                                (assoc
                                 :gen-undo-ops? false
-                                :undo? undo?))
+                                :undo? undo?
+                                :redo? (not undo?)))
                   handler (fn handler []
                             ((if undo? push-redo-op push-undo-op) repo op)
                             (let [editor-cursors (->> (filter #(= ::record-editor-info (first %)) op)
@@ -292,10 +291,14 @@
                   (do
                     (ldb/transact! conn reversed-tx-data tx-meta')
                     (handler))
-                  (p/do!
-                   ;; async write to the master worker
-                   (ldb/transact! repo reversed-tx-data tx-meta')
-                   (handler)))))))))
+                  (->
+                   (p/do!
+                    ;; async write to the master worker
+                    (ldb/transact! repo reversed-tx-data tx-meta')
+                    (handler))
+                   (p/catch (fn [e]
+                              (log/error ::undo-redo-failed e)
+                              (clear-history! repo)))))))))))
 
     (when ((if undo? empty-undo-stack? empty-redo-stack?) repo)
       (prn (str "No further " (if undo? "undo" "redo") " information"))

src/main/frontend/util.cljc

@@ -1487,3 +1487,8 @@ Arg *stop: atom, reset to true to stop the loop"
         (let [f #(when-let [node (gdom/getElement "app-keep-keyboard-open-input")]
                    (.focus node))]
           (if schedule? (schedule f) (f)))))))
+
+#?(:cljs
+   (defn rtc-test?
+     []
+     (string/includes? js/window.location.search "?rtc-test=true")))

src/main/frontend/worker/db_worker.cljs

@@ -409,6 +409,8 @@
 
 (def-thread-api :thread-api/create-or-open-db
   [repo opts]
+  (when-not (= repo (worker-state/get-current-repo)) ; graph switched
+    (reset! worker-state/*deleted-block-uuid->db-id {}))
   (start-db! repo opts))
 
 (def-thread-api :thread-api/q
@@ -535,9 +537,8 @@
                                (ldb/transact! conn tx-data' tx-meta')))
         nil)
       (catch :default e
-        (prn :debug :error)
-        (js/console.error e)
-        (prn :debug :tx-meta tx-meta :tx-data tx-data)
+        (prn :debug :worker-transact-failed :tx-meta tx-meta :tx-data tx-data)
+        (log/error ::worker-transact-failed e)
         (throw e)))))
 
 (def-thread-api :thread-api/get-initial-data

src/main/frontend/worker/handler/page.cljs

@@ -12,14 +12,16 @@
             [logseq.graph-parser.db :as gp-db]))
 
 (defn rtc-create-page!
-  [conn config title {:keys [uuid]}]
+  [conn config title {:keys [uuid old-db-id]}]
   (assert (uuid? uuid) (str "rtc-create-page! `uuid` is not a uuid " uuid))
   (let [date-formatter    (common-config/get-date-formatter config)
         title (db-worker-page/sanitize-title title)
         page-name (common-util/page-name-sanity-lc title)
-        page              (gp-block/page-name->map title @conn true date-formatter
-                                                   {:page-uuid uuid
-                                                    :skip-existing-page-check? true})
+        page              (cond-> (gp-block/page-name->map title @conn true date-formatter
+                                                           {:page-uuid uuid
+                                                            :skip-existing-page-check? true})
+                            old-db-id
+                            (assoc :db/id old-db-id))
         result            (ldb/transact! conn [page] {:persist-op? false
                                                       :outliner-op :create-page
                                                       :rtc-op? true})]

src/main/frontend/worker/pipeline.cljs

@@ -105,13 +105,15 @@
   "Validate db is slow, we probably don't want to enable it for production."
   [repo conn tx-report tx-meta context]
   (when (and (not (::skip-validate-db? tx-meta false))
-             (:dev? context)
+             (or (:dev? context) (:undo? tx-meta) (:redo? tx-meta))
              (not (:importing? context)) (sqlite-util/db-based-graph? repo))
     (let [valid? (if (get-in tx-report [:tx-meta :reset-conn!])
                    true
                    (db-validate/validate-tx-report! tx-report (:validate-db-options context)))]
       (when-not valid?
-        (when (or (get-in context [:validate-db-options :fail-invalid?]) worker-util/dev?)
+        (when (and (or (get-in context [:validate-db-options :fail-invalid?]) worker-util/dev?)
+                   ;; don't notify on production when undo/redo failed
+                   (not (and (not (:dev? context)) (or (:undo? tx-meta) (:redo? tx-meta)))))
           (shared-service/broadcast-to-clients! :notification
                                                 [["Invalid DB!"] :error]))
         (throw (ex-info "Invalid data" {:graph repo})))))
@@ -379,6 +381,14 @@
             fix-page-tags-tx-data
             fix-inline-page-tx-data)))
 
+(defn- reverse-tx!
+  [conn tx-data]
+  (let [reversed-tx-data (map (fn [[e a v _tx add?]]
+                                (let [op (if add? :db/retract :db/add)]
+                                  [op e a v])) tx-data)]
+    (d/transact! conn reversed-tx-data {:revert-tx-data? true
+                                        :gen-undo-ops? false})))
+
 (defn- undo-tx-data-if-disallowed!
   [conn {:keys [tx-data tx-meta]}]
   (when-not (:rtc-download-graph? tx-meta)
@@ -389,10 +399,7 @@
                                                     (not (ldb/page? (d/entity db (:v d)))))) tx-data)]
       ;; TODO: add other cases that need to be undo
       (when page-has-block-parent?
-        (let [reversed-tx-data (map (fn [[e a v _tx add?]]
-                                      (let [op (if add? :db/retract :db/add)]
-                                        [op e a v])) tx-data)]
-          (d/transact! conn reversed-tx-data {:op :undo-tx-data}))
+        (reverse-tx! conn tx-data)
         (throw (ex-info "Page can't have block as parent"
                         {:type :notification
                          :payload {:message "Page can't have block as parent"
@@ -423,6 +430,10 @@
           deleted-blocks (outliner-pipeline/filter-deleted-blocks (:tx-data tx-report*))
           deleted-block-ids (set (map :db/id deleted-blocks))
           deleted-block-uuids (set (map :block/uuid deleted-blocks))
+          _ (when (seq deleted-block-uuids)
+              (swap! worker-state/*deleted-block-uuid->db-id merge
+                     (zipmap (map :block/uuid deleted-blocks)
+                             (map :db/id deleted-blocks))))
           deleted-assets (keep (fn [id]
                                  (let [e (d/entity (:db-before tx-report*) id)]
                                    (when (ldb/asset? e)
@@ -448,7 +459,18 @@
           tx-report' (ldb/transact! conn replace-tx {:pipeline-replace? true
                                                      ;; Ensure db persisted
                                                      :db-persist? true})
-          _ (validate-db! repo conn tx-report* tx-meta context)
+          _ (when-not (:revert-tx-data? tx-meta)
+              (try
+                (validate-db! repo conn tx-report* tx-meta context)
+                (catch :default e
+                  (when-not (rtc-tx-or-download-graph? tx-meta)
+                    (prn :debug :revert-invalid-tx
+                         :tx-meta
+                         tx-meta
+                         :tx-data
+                         (:tx-data tx-report*))
+                    (reverse-tx! conn (:tx-data tx-report*)))
+                  (throw e))))
           full-tx-data (concat (:tx-data tx-report*)
                                (:tx-data refs-tx-report)
                                (:tx-data tx-report'))
@@ -472,7 +494,8 @@
 
 (defn invoke-hooks
   [repo conn {:keys [tx-meta] :as tx-report} context]
-  (when-not (:pipeline-replace? tx-meta)
+  (when-not (or (:pipeline-replace? tx-meta)
+                (:revert-tx-data? tx-meta))
     (let [{:keys [from-disk? new-graph?]} tx-meta]
       (cond
         (or from-disk? new-graph?)

src/main/frontend/worker/rtc/remote_update.cljs

@@ -31,6 +31,14 @@ so need to pull earlier remote-data from websocket."})
 
 (defmulti ^:private transact-db! (fn [action & _args] action))
 
+(defn- block-reuse-db-id
+  [block]
+  (if-let [old-eid (@worker-state/*deleted-block-uuid->db-id (:block/uuid block))]
+    (assoc block
+           :db/id old-eid
+           :block.temp/use-old-db-id? true)
+    block))
+
 (defmethod transact-db! :delete-blocks [_ & args]
   (outliner-tx/transact!
    {:persist-op? false
@@ -105,8 +113,9 @@ so need to pull earlier remote-data from websocket."})
     :outliner-op :insert-blocks
     :transact-opts {:repo repo
                     :conn conn}}
-   (let [opts' (assoc opts :keep-block-order? true)]
-     (outliner-core/insert-blocks! repo conn blocks target opts')))
+   (let [opts' (assoc opts :keep-block-order? true)
+         blocks' (map block-reuse-db-id blocks)]
+     (outliner-core/insert-blocks! repo conn blocks' target opts')))
   (doseq [block blocks]
     (assert (some? (d/entity @conn [:block/uuid (:block/uuid block)]))
             {:msg "insert-block failed"
@@ -200,7 +209,7 @@ so need to pull earlier remote-data from websocket."})
       (case [whiteboard-page-block? (some? local-parent) (some? remote-block-order)]
         [false true true]
         (do (if move?
-              (transact-db! :move-blocks repo conn [b] local-parent {:sibling? false})
+              (transact-db! :move-blocks repo conn [(block-reuse-db-id b)] local-parent {:sibling? false})
               (transact-db! :insert-blocks repo conn
                             [{:block/uuid block-uuid
                               :block/title ""}]
@@ -555,8 +564,11 @@ so need to pull earlier remote-data from websocket."})
     (doseq [{:keys [self _page-name]
              title :block/title
              :as op-value} update-page-ops]
-      (let [create-opts {:uuid self}
-            [_ page-name page-uuid] (worker-page/rtc-create-page! conn config (ldb/read-transit-str title) create-opts)]
+      (let [create-opts {:uuid self
+                         :old-db-id (@worker-state/*deleted-block-uuid->db-id self)}
+            [_ page-name page-uuid] (worker-page/rtc-create-page! conn config
+                                                                  (ldb/read-transit-str title)
+                                                                  create-opts)]
         ;; TODO: current page-create fn is buggy, even provide :uuid option, it will create-page with different uuid,
         ;; if there's already existing same name page
         (assert (= page-uuid self) {:page-name page-name :page-uuid page-uuid :should-be self})

src/main/frontend/worker/state.cljs

@@ -5,6 +5,7 @@
 
 (defonce *main-thread (atom nil))
 (defonce *infer-worker (atom nil))
+(defonce *deleted-block-uuid->db-id (atom {}))
 
 (defn- <invoke-main-thread*
   [qkw direct-pass? args-list]
@@ -129,7 +130,6 @@
   []
   (:auth/id-token @*state))
 
-
 ;;; ========================== mobile log ======================================
 (defonce *log (atom []))
 (defn log-append!