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"); +}