From 2e2f9466aec89c2ab7fc03378e3cbf975ffd7f9d Mon Sep 17 00:00:00 2001 From: clover caruso Date: Fri, 11 Sep 2026 01:58:18 -0700 Subject: [PATCH] feat: merge concurrent page edits during reconciliation When the remote page changed since a save was made, reconcile by a three-way merge of page models: the remote page keeps everything it changed and the local changes are re-applied wherever the two sides touched different objects, fields or text ranges. Same-paragraph edits reuse the banded text rebase to place the local replacement inside the remote text; a paragraph removed on one side and edited on the other, overlapping text ranges, both sides moving an outline, or both sides reordering the same container remain ContentChanged conflicts for review_page. Validation: five sync tests (different paragraphs, same paragraph with and without overlap, local insertion beside a remote deletion, remote deletion of the edited paragraph, outline moves against remote text edits and moves) plus a text-merge unit test; Clippy and fmt. Assisted-by: claude-fable-5.1 --- crates/notebook/src/lib.rs | 1 + crates/notebook/src/merge.rs | 458 ++++++++++++++++++++++++++++ crates/notebook/src/sync.rs | 11 +- crates/notebook/tests/sync/model.rs | 258 ++++++++++++++++ 4 files changed, 724 insertions(+), 4 deletions(-) create mode 100644 crates/notebook/src/merge.rs diff --git a/crates/notebook/src/lib.rs b/crates/notebook/src/lib.rs index bf3166939e2ef79d4eb648d552609269fca4c066..f65c692d3ea74440414713b068cf661554a000dd 100644 --- a/crates/notebook/src/lib.rs +++ b/crates/notebook/src/lib.rs @@ -15,6 +15,7 @@ use std::{fs::OpenOptions, io, ops::Range, path::Path, sync::Mutex, time::Durati mod assets; mod formatting; +mod merge; mod outline; mod pages; mod paragraph; diff --git a/crates/notebook/src/merge.rs b/crates/notebook/src/merge.rs new file mode 100644 index 0000000000000000000000000000000000000000..65086ffaec50dabdb322ca032f482903ad9d8565 --- /dev/null +++ b/crates/notebook/src/merge.rs @@ -0,0 +1,458 @@ +//! Three-way merge of page models: the remote page keeps everything it changed, and the +//! local changes (`base` → `ours`) are re-applied wherever the two sides touched +//! different objects, fields or text ranges. Any overlap is a conflict for review. + +use onestore::{ + ExGuid, + page::{Outline, Page, PageObject, PageParagraph, ParagraphContent, text::Edit}, +}; +use std::collections::{BTreeMap, BTreeSet}; + +pub(crate) fn merge(base: &Page, ours: &Page, theirs: &Page) -> Option { + if ours == base { + return Some(theirs.clone()); + } + if theirs == base || ours == theirs { + return Some(ours.clone()); + } + if ours.created != base.created || ours.margin_origin != base.margin_origin { + return None; + } + fn by_id(page: &Page) -> BTreeMap { + page.objects + .iter() + .map(|object| (object.id(), object)) + .collect() + } + let (b, o, t) = (by_id(base), by_id(ours), by_id(theirs)); + let mut result: Vec = Vec::new(); + for object in &theirs.objects { + let id = object.id(); + match (b.get(&id), o.get(&id)) { + (Some(before), Some(after)) => match (before, after, object) { + (PageObject::Outline(x), PageObject::Outline(y), PageObject::Outline(z)) => { + result.push(PageObject::Outline(merge_outline(x, y, z)?)); + } + _ => { + if after != before { + return None; + } + result.push(object.clone()); + } + }, + (Some(before), None) => { + // Removed locally; keep only if the remote left it untouched. + if object != *before { + return None; + } + } + (None, _) => result.push(object.clone()), + } + } + for (index, object) in ours.objects.iter().enumerate() { + let id = object.id(); + if b.contains_key(&id) { + if !t.contains_key(&id) { + // Removed remotely; a local edit to it cannot be placed. + if b[&id] != object { + return None; + } + } + continue; + } + if t.contains_key(&id) { + return None; + } + let PageObject::Outline(_) = object else { + return None; + }; + let predecessor = ours.objects[..index] + .iter() + .rev() + .map(PageObject::id) + .find(|other| result.iter().any(|r| r.id() == *other)); + let at = match predecessor { + Some(other) => result.iter().position(|r| r.id() == other).unwrap() + 1, + None => 0, + }; + let at = at.min( + result + .iter() + .position(|r| matches!(r, PageObject::Title(_))) + .unwrap_or(result.len()), + ); + result.insert(at, object.clone()); + } + let order = |objects: &[PageObject], keep: &dyn Fn(ExGuid) -> bool| -> Vec { + objects + .iter() + .filter(|object| matches!(object, PageObject::Outline(_)) && keep(object.id())) + .map(PageObject::id) + .collect() + }; + let common = |id: ExGuid| b.contains_key(&id) && o.contains_key(&id) && t.contains_key(&id); + let (base_order, our_order, their_order) = ( + order(&base.objects, &common), + order(&ours.objects, &common), + order(&theirs.objects, &common), + ); + if our_order != base_order { + if their_order != base_order && their_order != our_order { + return None; + } + result = reorder(result, &our_order); + } + Some(Page { + title: theirs.title.clone(), + created: theirs.created, + margin_origin: theirs.margin_origin, + objects: result, + definitions: theirs.definitions.clone(), + }) +} + +/// Places the objects named in `order` into the slots those objects occupy in `objects`, +/// leaving every other object where it is. +fn reorder(objects: Vec, order: &[ExGuid]) -> Vec { + let mut slots: Vec> = objects.into_iter().map(Some).collect(); + let positions: Vec = slots + .iter() + .enumerate() + .filter(|(_, slot)| { + slot.as_ref() + .is_some_and(|object| order.contains(&object.id())) + }) + .map(|(i, _)| i) + .collect(); + let mut taken: BTreeMap = positions + .iter() + .map(|&i| { + let object = slots[i].take().unwrap(); + (object.id(), object) + }) + .collect(); + for (slot, id) in positions.into_iter().zip(order) { + slots[slot] = taken.remove(id); + } + slots.into_iter().flatten().collect() +} + +fn pick(base: &T, ours: &T, theirs: &T) -> Option { + if ours == base { + Some(theirs.clone()) + } else if theirs == base || theirs == ours { + Some(ours.clone()) + } else { + None + } +} + +fn merge_outline(base: &Outline, ours: &Outline, theirs: &Outline) -> Option { + if ours.title != base.title + || ours.min_width != base.min_width + || ours.indents != base.indents + || ours.unsupported != base.unsupported + { + return None; + } + let mut layout = theirs.layout.clone(); + let (x, y) = pick( + &(base.layout.x, base.layout.y), + &(ours.layout.x, ours.layout.y), + &(theirs.layout.x, theirs.layout.y), + )?; + let (max_width, user_set) = pick( + &(base.layout.max_width, base.layout.width_set_by_user), + &(ours.layout.max_width, ours.layout.width_set_by_user), + &(theirs.layout.max_width, theirs.layout.width_set_by_user), + )?; + layout.x = x; + layout.y = y; + layout.max_width = max_width; + layout.width_set_by_user = user_set; + Some(Outline { + id: theirs.id, + title: theirs.title, + min_width: theirs.min_width, + layout, + indents: theirs.indents.clone(), + paragraphs: merge_paragraphs(&base.paragraphs, &ours.paragraphs, &theirs.paragraphs)?, + unsupported: theirs.unsupported.clone(), + }) +} + +type Children = BTreeMap, Vec>; + +fn children(list: &[PageParagraph]) -> Children { + let mut children: Children = BTreeMap::new(); + for paragraph in list { + children + .entry(paragraph.parent) + .or_default() + .push(paragraph.id); + } + children +} + +fn merge_paragraphs( + base: &[PageParagraph], + ours: &[PageParagraph], + theirs: &[PageParagraph], +) -> Option> { + fn index(list: &[PageParagraph]) -> BTreeMap { + list.iter().map(|p| (p.id, p)).collect() + } + let (b, o, t) = (index(base), index(ours), index(theirs)); + let mut merged: BTreeMap = BTreeMap::new(); + let mut removed = BTreeSet::new(); + for id in b.keys().chain(o.keys()).chain(t.keys()) { + if merged.contains_key(id) || removed.contains(id) { + continue; + } + let paragraph = match (b.get(id), o.get(id), t.get(id)) { + (Some(x), Some(y), Some(z)) => merge_paragraph(x, y, z)?, + (Some(x), None, Some(z)) => { + if z != x { + return None; + } + removed.insert(*id); + continue; + } + (Some(x), Some(y), None) => { + if y != x { + return None; + } + removed.insert(*id); + continue; + } + (Some(_), None, None) => { + removed.insert(*id); + continue; + } + (None, Some(_), Some(_)) => return None, + (None, Some(y), None) => (*y).clone(), + (None, None, Some(z)) => (*z).clone(), + (None, None, None) => unreachable!(), + }; + merged.insert(*id, paragraph); + } + // Child order per container: the remote order unless only we reordered it. + let (bc, oc, tc) = (children(base), children(ours), children(theirs)); + let mut order: BTreeMap, Vec> = BTreeMap::new(); + let containers: BTreeSet> = bc + .keys() + .chain(oc.keys()) + .chain(tc.keys()) + .copied() + .collect(); + for container in containers { + let live = |ids: Option<&Vec>| -> Vec { + ids.map(|ids| { + ids.iter() + .copied() + .filter(|id| merged.contains_key(id)) + .collect() + }) + .unwrap_or_default() + }; + let (base_list, our_list, their_list) = ( + live(bc.get(&container)), + live(oc.get(&container)), + live(tc.get(&container)), + ); + let common = |list: &[ExGuid]| -> Vec { + list.iter() + .copied() + .filter(|id| { + base_list.contains(id) && our_list.contains(id) && their_list.contains(id) + }) + .collect() + }; + let (base_common, our_common, their_common) = + (common(&base_list), common(&our_list), common(&their_list)); + let mut list: Vec = their_list.clone(); + if our_common != base_common { + if their_common != base_common && their_common != our_common { + return None; + } + let slots: Vec = list + .iter() + .enumerate() + .filter(|(_, id)| our_common.contains(id)) + .map(|(i, _)| i) + .collect(); + for (slot, id) in slots.into_iter().zip(&our_common) { + list[slot] = *id; + } + } + // Paragraphs we added go after the predecessor they follow in our list. + for (at, id) in our_list.iter().enumerate() { + if list.contains(id) { + continue; + } + if !b.contains_key(id) && merged[id].parent == container { + let predecessor = our_list[..at] + .iter() + .rev() + .find(|other| list.contains(other)); + let position = + predecessor.map_or(0, |p| list.iter().position(|x| x == p).unwrap() + 1); + list.insert(position, *id); + } + } + for id in &list { + if merged[id].parent != container { + return None; + } + } + order.insert(container, list); + } + let mut out = Vec::new(); + let mut pending: Vec = order.get(&None).cloned().unwrap_or_default(); + pending.reverse(); + while let Some(id) = pending.pop() { + let paragraph = merged.remove(&id)?; + out.push(paragraph); + if let Some(children) = order.get(&Some(id)) { + pending.extend(children.iter().rev().copied()); + } + } + if !merged.is_empty() { + return None; + } + Some(out) +} + +fn merge_paragraph( + base: &PageParagraph, + ours: &PageParagraph, + theirs: &PageParagraph, +) -> Option { + if ours.lists != base.lists + || ours.tags != base.tags + || ours.style != base.style + || ours.format != base.format + { + return None; + } + let (parent, level) = pick( + &(base.parent, base.level), + &(ours.parent, ours.level), + &(theirs.parent, theirs.level), + )?; + let collapsed = pick(&base.collapsed, &ours.collapsed, &theirs.collapsed)?; + let content = match (&base.content, &ours.content, &theirs.content) { + (ParagraphContent::Text(x), ParagraphContent::Text(y), ParagraphContent::Text(z)) => { + if x.id != y.id || x.id != z.id || y.date_field != x.date_field || y.tags != x.tags { + return None; + } + let mut text = z.clone(); + text.text = merge_text(&x.text, &y.text, &z.text)?; + ParagraphContent::Text(text) + } + (x, y, z) => { + if y != x { + return None; + } + z.clone() + } + }; + Some(PageParagraph { + id: theirs.id, + parent, + level, + style: theirs.style, + format: theirs.format.clone(), + content, + lists: theirs.lists.clone(), + tags: theirs.tags.clone(), + collapsed, + }) +} + +/// Re-applies our replacement inside the remote text when its range maps unambiguously. +fn merge_text( + base: &onestore::page::Paragraph, + ours: &onestore::page::Paragraph, + theirs: &onestore::page::Paragraph, +) -> Option { + if let Some(picked) = pick(base, ours, theirs) { + return Some(picked); + } + if base.text() == ours.text() || base.text() == theirs.text() { + // Formatting-only changes on both sides cannot be attributed to ranges. + return None; + } + let (b, o) = (base.text(), ours.text()); + let prefix = b + .char_indices() + .zip(o.chars()) + .take_while(|((_, x), y)| x == y) + .map(|((i, x), _)| i + x.len_utf8()) + .last() + .unwrap_or(0); + let suffix = b[prefix..] + .chars() + .rev() + .zip(o[prefix..].chars().rev()) + .take_while(|(x, y)| x == y) + .map(|(x, _)| x.len_utf8()) + .sum::(); + let start = base.utf16_offset(prefix).ok()?; + let end = base.utf16_offset(b.len() - suffix).ok()?; + let inserted = ours.utf16_offset(o.len() - suffix).ok()?; + let replacement = ours.slice(start..inserted).ok()?; + let mapped = crate::rebase::rebase(b, theirs.text(), start..end)?; + let mut merged = theirs.clone(); + merged + .apply(Edit { + range: mapped, + replacement, + }) + .ok()?; + Some(merged) +} + +#[cfg(test)] +mod tests { + use super::*; + use onestore::page::Paragraph; + + fn text(s: &str) -> Paragraph { + Paragraph::new(s.into(), Default::default()) + } + + #[test] + fn same_paragraph_edits_merge_when_their_ranges_do_not_overlap() { + let base = text("one two three"); + let ours = text("one two three four"); + assert_eq!( + merge_text(&base, &ours, &text("zero one two three")) + .unwrap() + .text(), + "zero one two three four" + ); + assert_eq!( + merge_text(&base, &ours, &text("one 2 three")) + .unwrap() + .text(), + "one 2 three four" + ); + assert_eq!( + merge_text(&base, &text("one two THREE"), &text("one two 3")), + None + ); + assert_eq!( + merge_text(&base, &text("one two three"), &text("x")) + .unwrap() + .text(), + "x" + ); + assert_eq!( + merge_text(&base, &text("y"), &text("one two three")) + .unwrap() + .text(), + "y" + ); + } +} diff --git a/crates/notebook/src/sync.rs b/crates/notebook/src/sync.rs index 1a5513501b22157b9b6883862b956f23b4b72616..9e7f162bdfc10df25eb22a010b8ae0f87a93836e 100644 --- a/crates/notebook/src/sync.rs +++ b/crates/notebook/src/sync.rs @@ -142,10 +142,13 @@ impl Replica { Operation::Page(edit) => page_of(&snapshot, intent.space)? .ok_or(ConflictKind::TargetUnavailable) .and_then(|current| { - if current != edit.before { - return Err(ConflictKind::ContentChanged); - } - PreparedEdit::page(&snapshot, intent.space, &edit.after, &edit.author) + let target = if current == edit.before { + edit.after.clone() + } else { + crate::merge::merge(&edit.before, &edit.after, ¤t) + .ok_or(ConflictKind::ContentChanged)? + }; + PreparedEdit::page(&snapshot, intent.space, &target, &edit.author) .map_err(|_| ConflictKind::UnsupportedEdit) }), Operation::CreatePage(page) => PreparedEdit::create_page(&snapshot, page) diff --git a/crates/notebook/tests/sync/model.rs b/crates/notebook/tests/sync/model.rs index c75ce6aaa6e7a36201edd99d45e96953a52c7972..f0bd256aadff45d037bb55f00a4bb26eb30e36ff 100644 --- a/crates/notebook/tests/sync/model.rs +++ b/crates/notebook/tests/sync/model.rs @@ -266,3 +266,261 @@ fn pending_saves_survive_reopening_the_cache() { assert_eq!(reopened.status(id).unwrap(), Some(EditStatus::Pending)); assert_eq!(page_of(&reopened.snapshot().unwrap(), space), after); } + +/// Edits paragraph `index` of the first body outline: replaces its text range with `replacement`. +fn edit_paragraph(page: &mut Page, index: usize, range: std::ops::Range, replacement: &str) { + let outline = page + .objects + .iter_mut() + .find_map(|object| match object { + PageObject::Outline(outline) => Some(outline), + _ => None, + }) + .unwrap(); + let text = outline.paragraphs[index].text_mut().unwrap(); + let format = text.text.format_at(range.start).unwrap().clone(); + text.text + .apply(onestore::page::text::Edit { + range, + replacement: onestore::page::Paragraph::new(replacement.into(), format), + }) + .unwrap(); +} + +fn texts(page: &Page) -> Vec { + page.objects + .iter() + .find_map(|object| match object { + PageObject::Outline(outline) => Some( + outline + .paragraphs + .iter() + .filter_map(|p| p.text().map(|t| t.text.text().to_owned())) + .collect(), + ), + _ => None, + }) + .unwrap() +} + +fn remote_with(space: ExGuid, change: impl FnOnce(&mut Page)) -> Server { + let mut page = page_of(OUTLINES, space); + change(&mut page); + Server::new( + PreparedEdit::page(OUTLINES, space, &page, "Native author") + .unwrap() + .as_bytes(), + ) +} + +#[test] +fn concurrent_edits_to_different_paragraphs_merge() { + let (space, mut after) = page_titled(OUTLINES, "Move leaf down"); + let directory = tempfile::tempdir().unwrap(); + let cache = Replica::create(directory.path().join("cache.sqlite"), OUTLINES).unwrap(); + edit_paragraph(&mut after, 0, 0..0, "Local "); + let id = cache + .save(OUTLINES, space, &after, "Model author") + .unwrap() + .unwrap(); + let mut server = remote_with(space, |page| edit_paragraph(page, 2, 0..0, "Remote ")); + assert!( + matches!(cache.sync_once(&mut server).unwrap(), Some((n, EditStatus::Published { .. })) if n == id) + ); + let published = texts(&page_of(&server.durable, space)); + assert!(published[0].starts_with("Local Anchor"), "{published:?}"); + assert!(published[2].starts_with("Remote Trailing"), "{published:?}"); + assert_eq!( + cache + .status(id) + .unwrap() + .map(|s| matches!(s, EditStatus::Published { .. })), + Some(true) + ); +} + +#[test] +fn concurrent_edits_to_one_paragraph_merge_unless_their_ranges_overlap() { + let (space, mut after) = page_titled(OUTLINES, "Move leaf down"); + let directory = tempfile::tempdir().unwrap(); + let cache = Replica::create(directory.path().join("cache.sqlite"), OUTLINES).unwrap(); + append(&mut after, " local"); + let id = cache + .save(OUTLINES, space, &after, "Model author") + .unwrap() + .unwrap(); + let mut server = remote_with(space, |page| edit_paragraph(page, 0, 0..0, "Remote ")); + assert!( + matches!(cache.sync_once(&mut server).unwrap(), Some((n, EditStatus::Published { .. })) if n == id) + ); + assert_eq!( + texts(&page_of(&server.durable, space))[0], + "Remote Anchor local" + ); + + let cache = Replica::create(directory.path().join("overlap.sqlite"), OUTLINES).unwrap(); + let mut after = page_of(OUTLINES, space); + edit_paragraph(&mut after, 0, 0..6, "Local"); + let id = cache + .save(OUTLINES, space, &after, "Model author") + .unwrap() + .unwrap(); + let mut server = remote_with(space, |page| edit_paragraph(page, 0, 0..6, "Remote")); + assert_eq!( + cache.sync_once(&mut server).unwrap(), + Some((id, EditStatus::Conflict(ConflictKind::ContentChanged))) + ); + assert_eq!(server.publications, 0); +} + +#[test] +fn a_local_insertion_merges_with_a_remote_deletion_elsewhere() { + let (space, mut after) = page_titled(OUTLINES, "Move leaf down"); + let directory = tempfile::tempdir().unwrap(); + let cache = Replica::create(directory.path().join("cache.sqlite"), OUTLINES).unwrap(); + let template = texts(&after); + assert_eq!(template.len(), 3); + { + let outline = after + .objects + .iter_mut() + .find_map(|object| match object { + PageObject::Outline(outline) => Some(outline), + _ => None, + }) + .unwrap(); + let mut fresh = outline.paragraphs[0].clone(); + fresh.id = onestore::page::text::new_id().unwrap(); + fresh.style = None; + let text = fresh.text_mut().unwrap(); + text.id = onestore::page::text::new_id().unwrap(); + text.text = onestore::page::Paragraph::new( + "Inserted locally".into(), + text.text.format_at(0).unwrap().clone(), + ); + outline.paragraphs.insert(1, fresh); + } + let id = cache + .save(OUTLINES, space, &after, "Model author") + .unwrap() + .unwrap(); + let mut server = remote_with(space, |page| { + let outline = page + .objects + .iter_mut() + .find_map(|object| match object { + PageObject::Outline(outline) => Some(outline), + _ => None, + }) + .unwrap(); + outline.paragraphs.pop(); + }); + assert!( + matches!(cache.sync_once(&mut server).unwrap(), Some((n, EditStatus::Published { .. })) if n == id) + ); + assert_eq!( + texts(&page_of(&server.durable, space)), + vec![ + template[0].clone(), + "Inserted locally".to_owned(), + template[1].clone() + ] + ); +} + +#[test] +fn a_remote_deletion_of_the_edited_paragraph_conflicts() { + let (space, mut after) = page_titled(OUTLINES, "Move leaf down"); + let directory = tempfile::tempdir().unwrap(); + let cache = Replica::create(directory.path().join("cache.sqlite"), OUTLINES).unwrap(); + edit_paragraph(&mut after, 2, 0..0, "Local "); + let id = cache + .save(OUTLINES, space, &after, "Model author") + .unwrap() + .unwrap(); + let mut server = remote_with(space, |page| { + let outline = page + .objects + .iter_mut() + .find_map(|object| match object { + PageObject::Outline(outline) => Some(outline), + _ => None, + }) + .unwrap(); + outline.paragraphs.pop(); + }); + assert_eq!( + cache.sync_once(&mut server).unwrap(), + Some((id, EditStatus::Conflict(ConflictKind::ContentChanged))) + ); +} + +#[test] +fn outline_moves_merge_with_remote_text_edits_but_not_with_remote_moves() { + let (space, mut after) = page_titled(OUTLINES, "Move leaf down"); + let directory = tempfile::tempdir().unwrap(); + let cache = Replica::create(directory.path().join("cache.sqlite"), OUTLINES).unwrap(); + let outline_id = { + let PageObject::Outline(outline) = after + .objects + .iter_mut() + .find(|o| matches!(o, PageObject::Outline(_))) + .unwrap() + else { + unreachable!() + }; + outline.layout.x = Some(200.0); + outline.layout.y = Some(300.0); + outline.id + }; + let id = cache + .save(OUTLINES, space, &after, "Model author") + .unwrap() + .unwrap(); + let mut server = remote_with(space, |page| edit_paragraph(page, 0, 0..0, "Remote ")); + assert!( + matches!(cache.sync_once(&mut server).unwrap(), Some((n, EditStatus::Published { .. })) if n == id) + ); + let published = page_of(&server.durable, space); + let PageObject::Outline(outline) = published + .objects + .iter() + .find(|o| o.id() == outline_id) + .unwrap() + else { + unreachable!() + }; + assert_eq!( + (outline.layout.x, outline.layout.y), + (Some(200.0), Some(300.0)) + ); + assert!( + outline.paragraphs[0] + .text() + .unwrap() + .text + .text() + .starts_with("Remote ") + ); + + let cache = Replica::create(directory.path().join("moves.sqlite"), OUTLINES).unwrap(); + let id = cache + .save(OUTLINES, space, &after, "Model author") + .unwrap() + .unwrap(); + let mut server = remote_with(space, |page| { + let PageObject::Outline(outline) = page + .objects + .iter_mut() + .find(|o| matches!(o, PageObject::Outline(_))) + .unwrap() + else { + unreachable!() + }; + outline.layout.x = Some(50.0); + }); + assert_eq!( + cache.sync_once(&mut server).unwrap(), + Some((id, EditStatus::Conflict(ConflictKind::ContentChanged))) + ); +} -- 2.54.0