Skip to content

Commit e2963e2

Browse files
authored
fix: preserve undo and redo history boundaries on no-ops (#1202)
* fix: stop undo and redo at no-op history items Keep each existing API call within one recorded item, preserving unrelated edits after a transformed no-op. See context/undo-history-boundaries.md for the contract and regression coverage. * fix(loro.js): preserve and report no-op undo items Record empty local state diffs for UndoManager without notifying public state subscribers, and return whether an opposite history item was generated. See context/undo-history-boundaries.md for the seed 2 regression and contract.
1 parent d6f9e6e commit e2963e2

14 files changed

Lines changed: 504 additions & 40 deletions

File tree

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
---
2+
"loro-crdt": patch
3+
"loro.js": patch
4+
---
5+
6+
Fix `UndoManager.undo()` and `redo()` consuming additional history items when the top item becomes a no-op after remote edits. Each call now consumes at most one recorded item, preserving unrelated edits and parent containers for subsequent calls. This also applies to Rust `UndoManager::undo()` and `redo()`.
7+
8+
A consumed no-op returns `false` and creates no opposite-stack item. Check `canUndo()` / `canRedo()` (Rust: `can_undo()` / `can_redo()`) for remaining history instead of treating `false` as an empty stack. Grouping and the merge interval still define one history item.
9+
10+
Align loro.js `UndoManager.undo()` and `redo()` with this result: consuming a no-op returns `false` rather than reporting a change. The item is still consumed and other history remains available.
11+
12+
Record local commits whose operations cancel in the state diff as undo items in loro.js, so undo consumes that no-op instead of reverting an earlier item. Public state subscriptions remain silent for empty diffs.

‎AGENTS.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ Loro is a Rust CRDT workspace with JS/WASM packaging and a MoonBit codec.
1818

1919
## Context Index
2020

21+
- Undo/redo item boundaries when remote edits make an inverse a no-op:
22+
[context/undo-history-boundaries.md](context/undo-history-boundaries.md).
2123
- Encoding/import/export modes, current vs outdated formats, shallow snapshots:
2224
[context/internal-encoding.md](context/internal-encoding.md).
2325
- Mergeable container model, marker/cid rules, tests, and common pitfalls:

‎context/undo-history-boundaries.md‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
# Undo/redo history boundaries
2+
3+
Verified against code 2026-10-17.
4+
5+
`crates/loro-internal/src/undo.rs`: `UndoManager::perform` processes at most
6+
one recorded `StackItem` per call to `undo` or `redo`. Grouping and the merge
7+
interval can put several edits, including edits to different containers, into
8+
that one item.
9+
10+
Remote edits can make the item's transformed inverse a no-op. For example,
11+
inserting a MovableList element locally and then moving it remotely can cancel
12+
the positional inverse delete. That item's lack of generated operations must
13+
not cause `perform` to pop another item: the next item may create an unrelated
14+
parent container, which would then disappear in the same undo call (#1183).
15+
16+
A consumed no-op still runs the existing `on_pop` and cursor bookkeeping. It
17+
returns `false`, generates no opposite-stack entry, and leaves other items for
18+
later calls. Callers must check `can_undo` / `can_redo` to decide whether history
19+
remains. The public Rust and WASM wrappers share this implementation.
20+
21+
`loro-js/src/runtime/undo.ts`: `UndoManager.undo` and `redo` also consume one
22+
item. Their result is whether `#invert` produced an opposite-stack item, not
23+
whether an item was popped. An empty inverse returns `false` while preserving
24+
pop callbacks, tracked-range cleanup and the remaining history.
25+
26+
`LoroDoc._subscribeUndo` in `loro-js/src/runtime/document.ts` also delivers
27+
local commits with empty state diffs to the manager, while public state
28+
subscriptions remain silent. Without this notification, a local MovableList
29+
insertion and deletion grouped into one item is never recorded, and undo
30+
instead consumes an earlier item. This is the `insert/delete undo (one peer)`
31+
differential profile, seed 2, step 33; changing the boolean return alone does
32+
not fix it. The subscription cleanup removes both listeners.
33+
34+
This boundary rule does not change MovableList identity/anchor transformation,
35+
grouping, or error handling. In particular, it does not force an inverse that
36+
has become empty to delete a remotely moved element.
37+
38+
Regressions:
39+
40+
- `crates/loro/tests/integration_test/undo_test.rs`: `noop_history_boundaries`
41+
covers undo before a scalar edit or parent creation, redo before a parent
42+
creation, unchanged versions for no-ops, subsequent history and convergence.
43+
- `crates/loro-internal/tests/undo.rs`:
44+
`consecutive_noops_consume_one_item_and_notify_once` covers multiple no-ops
45+
and one pop callback per item.
46+
- `crates/loro-wasm/tests/undo.test.ts` covers the exposed JS methods.
47+
- `loro-js/tests/undo-noop.test.ts` covers no-op undo and redo return values,
48+
unchanged versions/frontiers, pop callbacks, subsequent parent history and
49+
notification cleanup when the manager is destroyed.

‎crates/loro-internal/AGENTS.md‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,8 @@ over graceful degradation.
77

88
## Internal Map
99

10+
- `src/undo.rs`: one recorded item per undo/redo call, including no-ops:
11+
[../../context/undo-history-boundaries.md](../../context/undo-history-boundaries.md).
1012
- `src/loro.rs`: document-level orchestration for commit, import/export,
1113
checkout, barriers, state/oplog coordination, and event emission.
1214
`apply_diff`/`revert_to` roll a failed batch back:

‎crates/loro-internal/src/undo.rs‎

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -826,6 +826,9 @@ impl UndoManager {
826826
Ok(())
827827
}
828828

829+
/// Consume at most one undo item, even if its transformed inverse is a no-op.
830+
/// Returns whether local operations were generated; use `can_undo` to check
831+
/// whether history remains after a `false` result.
829832
#[instrument(skip_all)]
830833
pub fn undo(&self) -> LoroResult<bool> {
831834
self.perform(
@@ -835,6 +838,8 @@ impl UndoManager {
835838
)
836839
}
837840

841+
/// Consume at most one redo item, including a no-op. A `false` result means
842+
/// no local operations were generated, not necessarily an empty redo stack.
838843
#[instrument(skip_all)]
839844
pub fn redo(&self) -> LoroResult<bool> {
840845
self.perform(
@@ -909,7 +914,7 @@ impl UndoManager {
909914
// rather than using the current selection directly.
910915
self.record_new_checkpoint()?;
911916
let end_counter = get_counter_end(doc, self.peer());
912-
let mut top = {
917+
let top = {
913918
let lock = self.inner.lock();
914919
let mut inner = lock.borrow_mut();
915920
inner.processing_undo = true;
@@ -920,7 +925,9 @@ impl UndoManager {
920925
};
921926

922927
let mut executed = false;
923-
while let Some((mut span, remote_diff)) = top {
928+
// A no-op still consumes this item. Never cross its history boundary
929+
// to undo/redo an unrelated edit in the same call.
930+
if let Some((mut span, remote_diff)) = top {
924931
let mut next_push_selection = None;
925932
{
926933
let inner = self.inner.clone();
@@ -1018,11 +1025,6 @@ impl UndoManager {
10181025
.push(CounterSpan::new(end_counter, new_counter), meta);
10191026
inner.borrow_mut().next_counter = Some(new_counter);
10201027
executed = true;
1021-
break;
1022-
} else {
1023-
// continue to pop the undo item as this undo is a no-op
1024-
top = get_stack(&mut self.inner.lock().borrow_mut()).pop();
1025-
continue;
10261028
}
10271029
}
10281030

‎crates/loro-internal/tests/undo.rs‎

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,56 @@ use loro_internal::{
44
cursor::PosType, handler::UpdateOptions, loro::ExportMode, LoroDoc, UndoManager,
55
};
66

7+
#[test]
8+
fn consecutive_noops_consume_one_item_and_notify_once() {
9+
use std::sync::{
10+
atomic::{AtomicUsize, Ordering},
11+
Arc,
12+
};
13+
14+
let doc = LoroDoc::new_auto_commit();
15+
doc.set_peer_id(1).unwrap();
16+
let undo = UndoManager::new(&doc);
17+
undo.set_merge_interval(0);
18+
let pops = Arc::new(AtomicUsize::new(0));
19+
let popped = pops.clone();
20+
undo.set_on_pop(Some(Box::new(move |_, _, _| {
21+
popped.fetch_add(1, Ordering::Relaxed);
22+
})));
23+
doc.get_map("map").insert("prior", 42).unwrap();
24+
doc.commit_then_renew();
25+
let text = doc.get_text("text");
26+
text.insert(0, "a", PosType::Unicode).unwrap();
27+
doc.commit_then_renew();
28+
text.insert(1, "b", PosType::Unicode).unwrap();
29+
doc.commit_then_renew();
30+
31+
let other = LoroDoc::from_snapshot(&doc.export(ExportMode::Snapshot).unwrap()).unwrap();
32+
other.set_peer_id(2).unwrap();
33+
other
34+
.get_text("text")
35+
.delete(0, 2, PosType::Unicode)
36+
.unwrap();
37+
doc.import(&other.export(ExportMode::all_updates()).unwrap())
38+
.unwrap();
39+
let before = doc.get_deep_value();
40+
assert_eq!(undo.undo_count(), 3);
41+
for remaining in [2, 1] {
42+
assert!(!undo.undo().unwrap());
43+
assert_eq!(undo.undo_count(), remaining);
44+
assert_eq!(undo.redo_count(), 0);
45+
assert_eq!(pops.load(Ordering::Relaxed), 3 - remaining);
46+
assert_eq!(doc.get_deep_value(), before);
47+
}
48+
assert!(undo.undo().unwrap());
49+
assert_eq!(pops.load(Ordering::Relaxed), 3);
50+
assert!(!undo.can_undo());
51+
assert!(!undo.undo().unwrap());
52+
assert_eq!(pops.load(Ordering::Relaxed), 3);
53+
assert!(undo.redo().unwrap());
54+
assert_eq!(doc.get_deep_value(), before);
55+
}
56+
757
#[test]
858
fn test_basic_undo_group_checkpoint() {
959
let doc = LoroDoc::new();

‎crates/loro-wasm/src/lib.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5721,6 +5721,11 @@ impl UndoManager {
57215721

57225722
/// Undo the last operation.
57235723
///
5724+
/// Consumes at most one history item, as defined by grouping and the merge
5725+
/// interval. Returns whether local operations were generated. A no-op item
5726+
/// is consumed and returns false without undoing earlier items. Use
5727+
/// `canUndo()` to check whether history remains.
5728+
///
57245729
/// Throws without changing the doc if the step would have to recreate a
57255730
/// container of a type unknown to this version (created by a newer
57265731
/// loro-crdt). That step, with every edit merged into it, is dropped and its
@@ -5733,6 +5738,10 @@ impl UndoManager {
57335738

57345739
/// Redo the last undone operation.
57355740
///
5741+
/// Consumes at most one history item, including a no-op. Returns whether
5742+
/// local operations were generated. A false result does not imply that the
5743+
/// redo stack is empty; use `canRedo()` to check for remaining history.
5744+
///
57365745
/// Throws like `undo` on steps that would recreate a container of an
57375746
/// unknown type.
57385747
pub fn redo(&mut self) -> JsResult<bool> {

‎crates/loro-wasm/tests/undo.test.ts‎

Lines changed: 73 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,8 +1,80 @@
1-
import { Cursor, LoroDoc, UndoManager } from "../bundler/index";
1+
import { Cursor, LoroDoc, LoroMap, UndoManager } from "../bundler/index";
22
import { describe, expect, test } from "vitest";
33
import { expectDefined } from "./helpers";
44

55
describe("undo", () => {
6+
test.each([false, true])(
7+
"no-op undo preserves an earlier item (parent=%s)",
8+
(parent) => {
9+
const doc = new LoroDoc();
10+
doc.setPeerId(1);
11+
const list = doc.getMovableList("list");
12+
list.insert(0, 1);
13+
list.insert(1, 2);
14+
doc.commit();
15+
const remote = new LoroDoc();
16+
remote.setPeerId(2);
17+
remote.import(doc.export({ mode: "snapshot" }));
18+
const undo = new UndoManager(doc, { mergeInterval: 0 });
19+
const root = doc.getMap("root");
20+
if (parent) {
21+
root.setContainer("prior", new LoroMap()).set("value", 42);
22+
} else {
23+
root.set("prior", 42);
24+
}
25+
doc.commit();
26+
list.insert(0, 0);
27+
doc.commit();
28+
remote.import(doc.export({ mode: "update" }));
29+
remote.getMovableList("list").move(0, 2);
30+
remote.commit();
31+
doc.import(remote.export({ mode: "update" }));
32+
33+
const before = doc.toJSON();
34+
expect(undo.undo()).toBe(false);
35+
expect(doc.toJSON()).toEqual(before);
36+
expect(undo.canUndo()).toBe(true);
37+
expect(undo.canRedo()).toBe(false);
38+
expect(undo.undo()).toBe(true);
39+
expect(root.get("prior")).toBeUndefined();
40+
expect(undo.redo()).toBe(true);
41+
expect(doc.toJSON()).toEqual(before);
42+
},
43+
);
44+
45+
test("no-op redo preserves a later parent creation", () => {
46+
const doc = new LoroDoc();
47+
doc.setPeerId(1);
48+
const list = doc.getMovableList("list");
49+
list.insert(0, 1);
50+
list.insert(1, 2);
51+
doc.commit();
52+
const undo = new UndoManager(doc, { mergeInterval: 0 });
53+
list.delete(0, 1);
54+
doc.commit();
55+
const root = doc.getMap("root");
56+
root.setContainer("parent", new LoroMap());
57+
doc.commit();
58+
expect(undo.undo()).toBe(true);
59+
expect(undo.undo()).toBe(true);
60+
const remote = new LoroDoc();
61+
remote.setPeerId(2);
62+
remote.import(doc.export({ mode: "snapshot" }));
63+
remote.getMovableList("list").move(0, 1);
64+
remote.commit();
65+
doc.import(remote.export({ mode: "update" }));
66+
67+
const before = doc.toJSON();
68+
expect(undo.redo()).toBe(false);
69+
expect(doc.toJSON()).toEqual(before);
70+
expect(undo.canRedo()).toBe(true);
71+
expect(undo.canUndo()).toBe(false);
72+
expect(undo.redo()).toBe(true);
73+
expect(root.get("parent")).toBeDefined();
74+
expect(undo.undo()).toBe(true);
75+
expect(doc.toJSON()).toEqual(before);
76+
});
77+
678
test("basic text undo", () => {
779
const doc = new LoroDoc();
880
doc.setPeerId(1);

‎crates/loro/src/lib.rs‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4045,6 +4045,11 @@ impl UndoManager {
40454045

40464046
/// Undo the last change made by the peer.
40474047
///
4048+
/// Consumes at most one history item, as defined by grouping and the merge
4049+
/// interval. Returns whether local operations were generated. An item that
4050+
/// has become a no-op is consumed and returns `Ok(false)` without undoing
4051+
/// earlier items. Use [`Self::can_undo`] to check whether history remains.
4052+
///
40484053
/// Returns `Err(LoroError::ArgErr)` without changing the doc if the step
40494054
/// would have to recreate a container of a type unknown to this version
40504055
/// (written by a newer Loro). That step is dropped and its edits stay; the
@@ -4056,6 +4061,10 @@ impl UndoManager {
40564061

40574062
/// Redo the last change made by the peer.
40584063
///
4064+
/// Consumes at most one history item, including a no-op. Returns whether
4065+
/// local operations were generated; `Ok(false)` does not imply the redo
4066+
/// stack is empty. See [`Self::can_redo`].
4067+
///
40594068
/// Fails like [`UndoManager::undo`] on steps that would recreate a
40604069
/// container of an unknown type.
40614070
pub fn redo(&mut self) -> LoroResult<bool> {

0 commit comments

Comments
 (0)