From 961f0d6b97e7619673488f02ef2fb593008b2ec9 Mon Sep 17 00:00:00 2001 From: Cam Slade Date: Thu, 7 May 2026 09:50:24 -0500 Subject: [PATCH] fix(undo): ignore revived tree metadata in remote diffs During import, tree checkout can emit Delete+Create for the same live node when reconciling from the LCA to the import frontier. That marks the node's associated metadata container as new and emits full-state bring-back diffs for the metadata subtree. Those synthetic diffs look like real concurrent insertions to the undo transform, causing cursor positions to shift incorrectly and undo() to return false for nested text edits. Detect same-target Delete+Create tree diffs, collect their associated metadata containers, and exclude those containers and path descendants from remote_diff composition. Fixes #915. --- crates/loro-internal/src/undo.rs | 58 ++++++++- crates/loro/tests/issue.rs | 196 ++++++++++++++++++++++++++++++- 2 files changed, 251 insertions(+), 3 deletions(-) diff --git a/crates/loro-internal/src/undo.rs b/crates/loro-internal/src/undo.rs index 787e56633..4e652c0f1 100644 --- a/crates/loro-internal/src/undo.rs +++ b/crates/loro-internal/src/undo.rs @@ -402,6 +402,14 @@ impl Stack { } pub fn compose_remote_event(&mut self, diff: &[&ContainerDiff]) { + self.compose_remote_event_excluding(diff, &FxHashSet::default()); + } + + pub fn compose_remote_event_excluding( + &mut self, + diff: &[&ContainerDiff], + exclude: &FxHashSet, + ) { if self.is_empty() { return; } @@ -409,6 +417,17 @@ impl Stack { let remote_diff = &mut self.stack.back_mut().unwrap().1; let mut remote_diff = remote_diff.lock(); for e in diff { + if exclude.contains(&e.id) { + continue; + } + // Skip containers whose path passes through an excluded container: they + // are part of the same bring-back subtree emitted by the diff calculator. + if e.path + .iter() + .any(|(ancestor, _)| exclude.contains(ancestor)) + { + continue; + } if let Some(d) = remote_diff.cid_to_events.get_mut(&e.id) { d.compose_ref(&e.diff); } else { @@ -616,8 +635,30 @@ impl UndoManager { let lock = inner_clone.lock(); let mut inner = lock.borrow_mut(); + // When the diff calculator checks out from the LCA forward to the + // import frontier, a tree node that is alive on both sides can appear + // as Delete then Create in the same external TreeDiff (e.g. due to + // concurrent sibling insertions changing node ordering). In that case + // TreeDiffCalculator marks the node's associated_meta_container as a + // new container and emits full-state bring-back diffs for the entire + // metadata subtree. Those diffs look like concurrent insertions to the + // undo transform but carry no real new content. + // + // Note: same-target Delete+Create can also represent genuine + // delete-then-resurrect sequences. The exclusion below applies only to + // the metadata subtree's *bring-back* content; real edits to those + // containers within the same import batch are an edge case that could + // still be over-excluded. This heuristic is correct for the common case + // (issue #915) where no concurrent edits touch the subtree. + let mut revived_meta_containers: FxHashSet = FxHashSet::default(); for e in event.events { if let Diff::Tree(tree) = &e.diff { + let mut deleted: FxHashSet = FxHashSet::default(); + for item in &tree.diff { + if let TreeExternalDiff::Delete { .. } = &item.action { + deleted.insert(item.target); + } + } for item in &tree.diff { let target = item.target; if let TreeExternalDiff::Create { .. } = &item.action { @@ -626,6 +667,15 @@ impl UndoManager { remap_containers_clone .lock() .remove(&target.associated_meta_container()); + + if deleted.contains(&target) { + // Same-target Delete+Create in one diff: the diff + // calculator emitted full-state bring-back diffs for + // this node's metadata subtree. Exclude those from + // remote_diff so they don't corrupt undo transforms. + revived_meta_containers + .insert(target.associated_meta_container()); + } } } } @@ -633,8 +683,12 @@ impl UndoManager { let is_import_disjoint = inner.is_disjoint_with_group(event.events); - inner.undo_stack.compose_remote_event(event.events); - inner.redo_stack.compose_remote_event(event.events); + inner + .undo_stack + .compose_remote_event_excluding(event.events, &revived_meta_containers); + inner + .redo_stack + .compose_remote_event_excluding(event.events, &revived_meta_containers); // If the import is not disjoint, we end the active group // all subsequent changes will be new undo items diff --git a/crates/loro/tests/issue.rs b/crates/loro/tests/issue.rs index 87222ef1a..1a5e46df0 100644 --- a/crates/loro/tests/issue.rs +++ b/crates/loro/tests/issue.rs @@ -2,7 +2,7 @@ #![allow(unexpected_cfgs)] use loro::{ cursor::Cursor, ContainerID, ContainerTrait, EncodedBlobMode, ExportMode, LoroDoc, LoroError, - LoroList, LoroText, UndoManager, + LoroList, LoroMap, LoroText, UndoManager, }; use std::sync::{Arc, Mutex}; use tracing::{trace, trace_span}; @@ -468,3 +468,197 @@ fn get_unknown_cursor_position_but_its_in_pending() { assert!(!doc_1.has_container(&text.id())); assert_eq!(doc_1.get_path_to_container(&text.id()), None); } + +#[test] +fn test_undo_nested_text_in_tree_after_independent_import_issue_915() { + // undo() returned false for a text edit inside a tree node's meta map after + // importing from a completely independent document (issue #915). + let doc_a = LoroDoc::new(); + doc_a.set_peer_id(1).unwrap(); + + let tree = doc_a.get_tree("tree"); + let node = tree.create(None).unwrap(); + let meta = tree.get_meta(node).unwrap(); + let text = meta.insert_container("content", LoroText::new()).unwrap(); + text.insert(0, "Hello").unwrap(); + doc_a.commit(); + + // UndoManager created after initial content (matches the reported condition) + let mut undo = UndoManager::new(&doc_a); + undo.set_merge_interval(0); + + text.insert(5, " world").unwrap(); + doc_a.commit(); + assert_eq!(text.to_string(), "Hello world"); + assert!(undo.can_undo()); + + // Import from a completely independent doc (no shared history) + let doc_b = LoroDoc::new(); + doc_b.set_peer_id(2).unwrap(); + let tree_b = doc_b.get_tree("tree"); + let node_b = tree_b.create(None).unwrap(); + tree_b + .get_meta(node_b) + .unwrap() + .insert_container("content", LoroText::new()) + .unwrap() + .insert(0, "From B") + .unwrap(); + doc_b.commit(); + + doc_a + .import(&doc_b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + + assert!(undo.can_undo()); + assert!(undo.undo().unwrap()); + assert_eq!(text.to_string(), "Hello"); +} + +#[test] +fn test_undo_deeply_nested_text_in_tree_after_independent_import_issue_915() { + // Deeper nesting: tree node meta -> child map -> LoroText + let doc_a = LoroDoc::new(); + doc_a.set_peer_id(1).unwrap(); + + let tree = doc_a.get_tree("tree"); + let node = tree.create(None).unwrap(); + let child_map = tree + .get_meta(node) + .unwrap() + .insert_container("data", LoroMap::new()) + .unwrap(); + let text = child_map + .insert_container("content", LoroText::new()) + .unwrap(); + text.insert(0, "Hello").unwrap(); + doc_a.commit(); + + let mut undo = UndoManager::new(&doc_a); + undo.set_merge_interval(0); + + text.insert(5, " world").unwrap(); + doc_a.commit(); + + let doc_b = LoroDoc::new(); + doc_b.set_peer_id(2).unwrap(); + let node_b = doc_b.get_tree("tree").create(None).unwrap(); + doc_b + .get_tree("tree") + .get_meta(node_b) + .unwrap() + .insert("kind", "paragraph") + .unwrap(); + doc_b.commit(); + + doc_a + .import(&doc_b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + + assert!(undo.can_undo()); + assert!(undo.undo().unwrap()); + assert_eq!(text.to_string(), "Hello"); +} + +#[test] +fn test_undo_multi_node_reindex_after_independent_import_issue_915() { + // Multiple local tree nodes reindexed by a single independent import + let doc_a = LoroDoc::new(); + doc_a.set_peer_id(1).unwrap(); + + let tree = doc_a.get_tree("tree"); + let node1 = tree.create(None).unwrap(); + let node2 = tree.create(None).unwrap(); + let text1 = tree + .get_meta(node1) + .unwrap() + .insert_container("t", LoroText::new()) + .unwrap(); + let text2 = tree + .get_meta(node2) + .unwrap() + .insert_container("t", LoroText::new()) + .unwrap(); + text1.insert(0, "A").unwrap(); + text2.insert(0, "B").unwrap(); + doc_a.commit(); + + let mut undo = UndoManager::new(&doc_a); + undo.set_merge_interval(0); + + text1.insert(1, "1").unwrap(); + text2.insert(1, "2").unwrap(); + doc_a.commit(); + assert_eq!(text1.to_string(), "A1"); + assert_eq!(text2.to_string(), "B2"); + + // Independent doc adds two root nodes, forcing reindex of both peer-1 nodes + let doc_b = LoroDoc::new(); + doc_b.set_peer_id(2).unwrap(); + doc_b.get_tree("tree").create(None).unwrap(); + doc_b.get_tree("tree").create(None).unwrap(); + doc_b.commit(); + + doc_a + .import(&doc_b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + + assert!(undo.can_undo()); + assert!(undo.undo().unwrap()); + assert_eq!(text1.to_string(), "A"); + assert_eq!(text2.to_string(), "B"); +} + +#[test] +fn test_undo_preserves_real_remote_edit_on_revived_node_issue_915() { + // Risk case: same-event Delete+Create on a node whose text was also concurrently + // edited by the remote peer. The real remote edit must survive the undo transform. + // + // doc_a: node with "Hello", undo target = append " world" + // doc_b: (1) adds its own root node (triggers node reindex in merge diff) + // (2) concurrently prepends "Hi " to the same text + // After undo: text should be "Hi Hello" — remote prepend stays, local append removed. + let doc_a = LoroDoc::new(); + doc_a.set_peer_id(1).unwrap(); + + let tree = doc_a.get_tree("tree"); + let node = tree.create(None).unwrap(); + let text = tree + .get_meta(node) + .unwrap() + .insert_container("content", LoroText::new()) + .unwrap(); + text.insert(0, "Hello").unwrap(); + doc_a.commit(); + + let mut undo = UndoManager::new(&doc_a); + undo.set_merge_interval(0); + + text.insert(5, " world").unwrap(); + doc_a.commit(); + assert_eq!(text.to_string(), "Hello world"); + + // doc_b starts from doc_a's state, then diverges + let doc_b = LoroDoc::new(); + doc_b.set_peer_id(2).unwrap(); + doc_b + .import(&doc_a.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + + doc_b.get_tree("tree").create(None).unwrap(); + let text_b: LoroText = match doc_b.get_container(text.id().clone()).unwrap() { + loro::Container::Text(t) => t, + _ => panic!("expected text"), + }; + text_b.insert(0, "Hi ").unwrap(); + doc_b.commit(); + + doc_a + .import(&doc_b.export(ExportMode::all_updates()).unwrap()) + .unwrap(); + assert_eq!(text.to_string(), "Hi Hello world"); + + assert!(undo.can_undo()); + assert!(undo.undo().unwrap()); + assert_eq!(text.to_string(), "Hi Hello"); +}