fix(rtc): carefully compare t when add-ops to client-ops

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

@@ -529,7 +529,7 @@
             (if-let [not-found-target-ops (seq (:not-found-target-ops r))]
               (do (rollback repo block-ops-map-coll update-kv-value-ops-map-coll rename-db-ident-ops-map-coll)
                   ;; add more ents into ops for remote
-                  (let [ents (mapv
+                  (let [ents (keep
                               (fn [op]
                                 (let [block-uuid (:block-uuid (second op))]
                                   (assert block-uuid)

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

@@ -144,6 +144,40 @@
          {:block-uuid block-uuid
           :av-coll (concat av-coll1 av-coll2)}]))))
 
+(defn- merge-block-ops
+  "Carefully compare t among ops.
+  Return merged block-op-map."
+  [current-block-op-map op-to-add]
+  (let [[op-type op-t _value] op-to-add
+        {[_ remove-op-t _remove-op-value :as remove-op] :remove
+         [_ move-op-t _move-op-value :as move-op] :move
+         [_ update-op-t _update-op-value :as update-op] :update
+         [_ update-page-op-t _update-page-op-value :as update-page-op] :update-page
+         [_ remove-page-op-t _remove-page-op-value :as remove-page-op] :remove-page}
+        (into {} (filter (fn [[_op-type op]] (some-> op (not= :retract))) current-block-op-map))]
+    (case op-type
+      :move
+      (if (>= remove-op-t op-t) current-block-op-map
+          (cond-> (assoc current-block-op-map :remove :retract)
+            (or (nil? move-op) (> op-t move-op-t)) (assoc :move op-to-add)))
+      :update
+      (if (>= remove-op-t op-t) current-block-op-map
+          (assoc current-block-op-map
+                 :remove :retract
+                 :update (if update-op (merge-update-ops update-op op-to-add) op-to-add)))
+      :remove
+      (if (or (>= move-op-t op-t) (>= update-op-t op-t)) current-block-op-map
+          (cond-> (assoc current-block-op-map :move :retract :update :retract)
+            (or (nil? remove-op) (> op-t remove-op-t)) (assoc :remove op-to-add)))
+      :update-page
+      (if (>= remove-page-op-t op-t) current-block-op-map
+          (cond-> (assoc current-block-op-map :remove-page :retract)
+            (or (nil? update-page-op) (> op-t update-page-op-t)) (assoc :update-page op-to-add)))
+      :remove-page
+      (if (>= update-page-op-t op-t) current-block-op-map
+          (cond-> (assoc current-block-op-map :update-page :retract)
+            (or (nil? remove-page-op) (> op-t remove-page-op-t)) (assoc :remove-page op-to-add))))))
+
 (defn- generate-block-ops-tx-data
   [client-ops-db ops]
   (let [sorted-ops (sort-by second ops)
@@ -164,33 +198,10 @@
         block-uuid->op-type->op
         (reduce
          (fn [r op]
-           (let [[op-type _t value] op
-                 block-uuid (:block-uuid value)]
-             (case op-type
-               :move
-               (-> r
-                   (update block-uuid assoc :remove :retract)
-                   (assoc-in [block-uuid :move] op))
-               :update
-               (-> r
-                   (update block-uuid assoc :remove :retract)
-                   (update-in [block-uuid :update] (fn [old-op]
-                                                     (if (and old-op
-                                                              (not (keyword-identical? :retract old-op)))
-                                                       (merge-update-ops old-op op)
-                                                       op))))
-               :remove
-               (-> r
-                   (update block-uuid assoc :move :retract :update :retract)
-                   (assoc-in [block-uuid :remove] op))
-               :update-page
-               (-> r
-                   (update block-uuid assoc :remove-page :retract)
-                   (assoc-in [block-uuid :update-page] op))
-               :remove-page
-               (-> r
-                   (update block-uuid assoc :update-page :retract)
-                   (assoc-in [block-uuid :remove-page] op)))))
+           (let [[_ _ value] op
+                 block-uuid (:block-uuid value)
+                 current-block-op-map (get r block-uuid)]
+             (assoc r block-uuid (merge-block-ops current-block-op-map op))))
          init-block-uuid->op-type->op sorted-ops)]
     (mapcat
      (fn [[block-uuid op-type->op]]

src/test/frontend/worker/rtc/client_op_test.cljs

@@ -0,0 +1,188 @@
+(ns frontend.worker.rtc.client-op-test
+  (:require [cljs.test :as t :refer [deftest is testing]]
+            [datascript.core :as d]
+            [frontend.worker.rtc.client-op :as client-op]
+            [frontend.worker.state :as worker-state]))
+
+(deftest merge-update-ops-test
+  (testing "older op added after newer op should be merged in correct order"
+    (let [conn (d/create-conn client-op/schema-in-db)
+          block-uuid (random-uuid)
+          op1 [:update 1 {:block-uuid block-uuid
+                          :av-coll [[:block/title "A" 1 true]]}]
+          op2 [:update 2 {:block-uuid block-uuid
+                          :av-coll [[:block/title "B" 2 true]]}]]
+      (with-redefs [worker-state/get-client-ops-conn (constantly conn)]
+        ;; 1. Add newer op first
+        (client-op/add-ops! "repo" [op2])
+
+        ;; 2. Add older op later
+        (client-op/add-ops! "repo" [op1])
+
+        ;; 3. Verify merged result
+        (let [ops (client-op/get&remove-all-block-ops "repo")
+              merged-op (first ops)]
+          (is (= 1 (count ops)))
+          (is (= :update (first (:update merged-op))))
+          (is (= 2 (second (:update merged-op)))) ;; Should keep the max t
+
+          (let [av-coll (:av-coll (last (:update merged-op)))]
+            (is (= 2 (count av-coll)))
+            ;; The av-coll should contain both, but order matters for semantic correctness.
+            ;; Logic in merge-update-ops:
+            ;; (if (> t1 t2) (merge-update-ops op2 op1) ...)
+            ;; so it recursively ensures older is merged into newer (or concatenated properly).
+            ;; effectively: (concat av-coll-of-older av-coll-of-newer)
+
+            (is (= [[:block/title "A" 1 true]
+                    [:block/title "B" 2 true]]
+                   av-coll))))))))
+
+(deftest remove-op-wins-over-older-move-op-test
+  (testing "older move-op added after newer remove-op should NOT resurrect the block"
+    (let [conn (d/create-conn client-op/schema-in-db)
+          block-uuid (random-uuid)
+          move-op [:move 1 {:block-uuid block-uuid}]
+          remove-op [:remove 2 {:block-uuid block-uuid}]]
+      (with-redefs [worker-state/get-client-ops-conn (constantly conn)]
+        ;; 1. Add newer remove-op first
+        (client-op/add-ops! "repo" [remove-op])
+
+        ;; 2. Add older move-op later (simulating rollback)
+        (client-op/add-ops! "repo" [move-op])
+
+        ;; 3. Verify result
+        (let [ops (client-op/get&remove-all-block-ops "repo")
+              result-op (first ops)]
+          (is (= 1 (count ops)))
+          ;; Expectation: The remove-op (t=2) should win over move-op (t=1).
+          ;; The block should remain removed.
+          (is (some? (:remove result-op)))
+          (is (nil? (:move result-op)))
+          (is (= 2 (second (:remove result-op)))))))))
+
+(deftest move-op-and-remove-op-exclusivity-test
+  (testing "move-op and remove-op should never coexist"
+    (let [conn (d/create-conn client-op/schema-in-db)
+          block-uuid (random-uuid)]
+      (with-redefs [worker-state/get-client-ops-conn (constantly conn)]
+        (testing "Scenario 1: Newer Move after Older Remove -> Move wins"
+          (let [remove-op [:remove 1 {:block-uuid block-uuid}]
+                move-op [:move 2 {:block-uuid block-uuid}]]
+            (client-op/add-ops! "repo" [remove-op])
+            (client-op/add-ops! "repo" [move-op])
+
+            (let [ops (client-op/get-all-block-ops "repo")
+                  ;; ops = '([:move 2 {:block-uuid #uuid "461222f8-d43a-4726-9063-3a14b46d26b8"}])
+                  result-op (first ops)]
+              (is (= :move (first result-op)))
+              (is (= 2 (second result-op)))
+              ;; Clean up
+              (client-op/get&remove-all-block-ops "repo"))))
+
+        (testing "Scenario 2: Newer Remove after Older Move -> Remove wins"
+          (let [move-op [:move 1 {:block-uuid block-uuid}]
+                remove-op [:remove 2 {:block-uuid block-uuid}]]
+            (client-op/add-ops! "repo" [move-op])
+            (client-op/add-ops! "repo" [remove-op])
+
+            (let [ops (client-op/get-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= :remove (first result-op)))
+              (is (= 2 (second result-op)))
+              ;; Clean up
+              (client-op/get&remove-all-block-ops "repo"))))))))
+
+(deftest ops-combinations-test
+  (testing "Various combinations of ops on the same block"
+    (let [conn (d/create-conn client-op/schema-in-db)
+          block-uuid (random-uuid)]
+      (with-redefs [worker-state/get-client-ops-conn (constantly conn)]
+        (testing "Duplicate Ops: Add same op twice"
+          (let [move-op [:move 1 {:block-uuid block-uuid}]]
+            (client-op/add-ops! "repo" [move-op])
+            (client-op/add-ops! "repo" [move-op])
+            (let [ops (client-op/get&remove-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= 1 (count ops)))
+              (is (some? (:move result-op)))
+              (is (= 1 (second (:move result-op)))))))
+
+        (testing "Move then Update (older move, newer update) -> Both present"
+          (let [move-op [:move 1 {:block-uuid block-uuid}]
+                update-op [:update 2 {:block-uuid block-uuid :av-coll []}]]
+            (client-op/add-ops! "repo" [move-op])
+            (client-op/add-ops! "repo" [update-op])
+            (let [ops (client-op/get&remove-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= 1 (count ops)))
+              (is (some? (:move result-op)))
+              (is (= 1 (second (:move result-op))))
+              (is (some? (:update result-op)))
+              (is (= 2 (second (:update result-op)))))))
+
+        (testing "Update then Move (older update, newer move) -> Both present"
+          (let [update-op [:update 1 {:block-uuid block-uuid :av-coll []}]
+                move-op [:move 2 {:block-uuid block-uuid}]]
+            (client-op/add-ops! "repo" [update-op])
+            (client-op/add-ops! "repo" [move-op])
+            (let [ops (client-op/get&remove-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= 1 (count ops)))
+              (is (some? (:update result-op)))
+              (is (= 1 (second (:update result-op))))
+              (is (some? (:move result-op)))
+              (is (= 2 (second (:move result-op)))))))
+
+        (testing "Update then Remove (Newer Remove) -> Remove wins"
+          (let [update-op [:update 1 {:block-uuid block-uuid :av-coll []}]
+                remove-op [:remove 2 {:block-uuid block-uuid}]]
+            (client-op/add-ops! "repo" [update-op])
+            (client-op/add-ops! "repo" [remove-op])
+            (let [ops (client-op/get&remove-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= 1 (count ops)))
+              (is (some? (:remove result-op)))
+              (is (nil? (:update result-op))))))
+
+        (testing "Remove then Update (Newer Update) -> Update wins"
+          (let [remove-op [:remove 1 {:block-uuid block-uuid}]
+                update-op [:update 2 {:block-uuid block-uuid :av-coll []}]]
+            (client-op/add-ops! "repo" [remove-op])
+            (client-op/add-ops! "repo" [update-op])
+            (let [ops (client-op/get&remove-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= 1 (count ops)))
+              (is (some? (:update result-op)))
+              (is (nil? (:remove result-op))))))
+
+        (testing "Move(t1) -> Remove(t2) -> Update(t3) -> Update wins, Move stays retracted"
+          (let [move-op [:move 1 {:block-uuid block-uuid}]
+                remove-op [:remove 2 {:block-uuid block-uuid}]
+                update-op [:update 3 {:block-uuid block-uuid :av-coll []}]]
+            (client-op/add-ops! "repo" [move-op])
+            (client-op/add-ops! "repo" [remove-op])
+            (client-op/add-ops! "repo" [update-op])
+            (let [ops (client-op/get&remove-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= 1 (count ops)))
+              (is (some? (:update result-op)))
+              (is (= 3 (second (:update result-op))))
+              (is (nil? (:remove result-op)))
+              (is (nil? (:move result-op))))))
+
+        (testing "Remove(t1) -> Update(t2) -> Move(t3) -> Update+Move wins"
+          (let [remove-op [:remove 1 {:block-uuid block-uuid}]
+                update-op [:update 2 {:block-uuid block-uuid :av-coll []}]
+                move-op [:move 3 {:block-uuid block-uuid}]]
+            (client-op/add-ops! "repo" [remove-op])
+            (client-op/add-ops! "repo" [update-op])
+            (client-op/add-ops! "repo" [move-op])
+            (let [ops (client-op/get&remove-all-block-ops "repo")
+                  result-op (first ops)]
+              (is (= 1 (count ops)))
+              (is (some? (:update result-op)))
+              (is (= 2 (second (:update result-op))))
+              (is (some? (:move result-op)))
+              (is (= 3 (second (:move result-op))))
+              (is (nil? (:remove result-op))))))))))