From f0ef9637ddb6735526edeac088e2a3ae7af26a19 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 03:57:44 -0800 Subject: [PATCH 01/49] Add block-level three-way merge with a merger registry merge_block(type, base, mine, theirs) merges two changed versions of one block through the merger its type registered. With no merger the result is a conflict, which keeps today's conflict copy. merge_page uses it only when a changed block opted in, every block changed on both sides merges, and the other page fields changed on one side at most. The merged revision comes from the two revisions alone, so every device reaches the same bytes in any order (ADR 0089). Co-Authored-By: Claude Opus 5.5 --- crates/core/src/lib.rs | 1 + crates/core/src/merge/mod.rs | 623 +++++++++++++++++++++++++++++++++++ 2 files changed, 624 insertions(+) create mode 100644 crates/core/src/merge/mod.rs diff --git a/crates/core/src/lib.rs b/crates/core/src/lib.rs index 0aac2c908..122ba6af7 100644 --- a/crates/core/src/lib.rs +++ b/crates/core/src/lib.rs @@ -11,6 +11,7 @@ pub mod error; pub mod format; pub mod id; pub mod limits; +pub mod merge; pub mod model; pub mod ops; pub mod order; diff --git a/crates/core/src/merge/mod.rs b/crates/core/src/merge/mod.rs new file mode 100644 index 000000000..888342ec6 --- /dev/null +++ b/crates/core/src/merge/mod.rs @@ -0,0 +1,623 @@ +//! Block-level three-way merge: the follow-up to ADR 0049 that ADR 0089 records. +//! +//! When two devices changed the same page, sync keeps one version as `page.json` and the other as a conflict copy +//! (spec 14.1). A block type can do better by registering a [`BlockMerger`]: given the version both devices started +//! from and the two changed versions of one block, it returns one block that keeps both changes, or `None`. +//! +//! [`merge_page`] returns `None` unless every rule here holds, and then the caller keeps today's conflict copy. +//! At least one block that changed has a type with a registered merger, so with none registered nothing changes. +//! Every block that changed on both sides, to different results, has a merger, and that merger succeeds. No block +//! was deleted on one side and changed on the other. Every other page field changed on at most one side. +//! +//! Every device must reach the same bytes from the same versions, in any order, so mergers must be commutative: +//! `merge(base, a, b) == merge(base, b, a)`. The merged revision is made from the two revisions alone, never from +//! a clock or a random source, for the same reason. + +use std::collections::{BTreeMap, BTreeSet}; +use std::sync::{Arc, OnceLock, PoisonError, RwLock}; + +use crate::id::{BlockId, Id, RevisionId}; +use crate::model::{Block, Page, Revision}; + +/// The most earlier revisions a revision lists (spec 5.3). +const MAX_ANCESTORS: usize = 32; + +/// Merges two changed versions of one block of one type. +pub trait BlockMerger: Send + Sync { + /// One block that keeps the changes from `base` to `mine` and from `base` to `theirs`, or `None` when they + /// can't both stay. All three have the same ID and type. The result must not depend on which side is `mine`. + fn merge(&self, base: &Block, mine: &Block, theirs: &Block) -> Option; +} + +/// The mergers, by block type. +#[derive(Clone, Default)] +pub struct Mergers { + by_type: BTreeMap>, +} + +impl std::fmt::Debug for Mergers { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_set().entries(self.by_type.keys()).finish() + } +} + +/// The result of merging one block. +#[derive(Clone, Debug, PartialEq)] +pub enum BlockMerge { + /// One block keeps both changes. + Merged(Box), + /// The changes can't both stay: the page keeps today's conflict copy. + Conflict, +} + +impl Mergers { + /// No mergers. + pub fn new() -> Mergers { + Mergers::default() + } + + /// Registers the merger for a block type, in place of any earlier one. + pub fn register(&mut self, type_name: impl Into, merger: Arc) { + self.by_type.insert(type_name.into(), merger); + } + + /// Whether a block type has a merger. + pub fn handles(&self, type_name: &str) -> bool { + self.by_type.contains_key(type_name) + } + + /// Whether no type has a merger. + pub fn is_empty(&self) -> bool { + self.by_type.is_empty() + } + + /// Merges two changed versions of a block of type `type_name`. Equal sides, or a side equal to `base`, need + /// no merger. With no merger for the type, the result is [`BlockMerge::Conflict`]. + pub fn merge_block(&self, type_name: &str, base: &Block, mine: &Block, theirs: &Block) -> BlockMerge { + if mine == theirs || theirs == base { + return BlockMerge::Merged(Box::new(mine.clone())); + } + if mine == base { + return BlockMerge::Merged(Box::new(theirs.clone())); + } + let same = |b: &Block| b.id == base.id && b.type_name() == type_name; + if !(same(base) && same(mine) && same(theirs)) { + return BlockMerge::Conflict; + } + match self.by_type.get(type_name).and_then(|m| m.merge(base, mine, theirs)) { + Some(merged) if same(&merged) => BlockMerge::Merged(Box::new(merged)), + _ => BlockMerge::Conflict, + } + } +} + +fn registry() -> &'static RwLock { + static GLOBAL: OnceLock> = OnceLock::new(); + GLOBAL.get_or_init(RwLock::default) +} + +/// Registers a merger for every merge in this process. Call it at startup, before the first sync. +pub fn register(type_name: impl Into, merger: Arc) { + registry() + .write() + .unwrap_or_else(PoisonError::into_inner) + .register(type_name, merger); +} + +/// The mergers registered for this process. +pub fn mergers() -> Mergers { + registry().read().unwrap_or_else(PoisonError::into_inner).clone() +} + +/// [`Mergers::merge_block`] with the mergers registered for this process. +pub fn merge_block(type_name: &str, base: &Block, mine: &Block, theirs: &Block) -> BlockMerge { + mergers().merge_block(type_name, base, mine, theirs) +} + +/// The three-way choice for one value: equal sides, or the side that changed. `None` if both changed it +/// differently. +fn pick<'a, T: PartialEq>(base: &T, mine: &'a T, theirs: &'a T) -> Option<&'a T> { + if mine == theirs || theirs == base { + Some(mine) + } else if mine == base { + Some(theirs) + } else { + None + } +} + +/// The three-way merge of a map, key by key. `None` if one key changed differently on both sides. +fn pick_map( + base: &BTreeMap, + mine: &BTreeMap, + theirs: &BTreeMap, +) -> Option> { + let keys: BTreeSet<&K> = base.keys().chain(mine.keys()).chain(theirs.keys()).collect(); + let mut out = BTreeMap::new(); + for key in keys { + if let Some(value) = pick(&base.get(key), &mine.get(key), &theirs.get(key))? { + out.insert(key.clone(), (*value).clone()); + } + } + Some(out) +} + +/// The blocks of two versions of a page that differ, by ID: present on one side only, or different on both. +/// For a side-by-side view of a conflict, which shows each block type's own compare view. +pub fn differing_blocks<'a>(mine: &'a Page, theirs: &'a Page) -> Vec<(BlockId, Option<&'a Block>, Option<&'a Block>)> { + let ids: BTreeSet = mine.blocks.iter().chain(theirs.blocks.iter()).map(|b| b.id).collect(); + ids.into_iter() + .filter_map(|id| { + let m = mine.blocks.get(id).map(Arc::as_ref); + let t = theirs.blocks.get(id).map(Arc::as_ref); + (m != t).then_some((id, m, t)) + }) + .collect() +} + +/// A block as `page.json` writes it. +pub fn block_json(block: &Block) -> serde_json::Value { + let device = crate::model::DeviceRef { + id: crate::id::DeviceId(Id::ZERO), + label: String::new(), + }; + let revision = Revision::new(RevisionId(Id::ZERO), block.created, device, ""); + let mut page = Page::new(crate::id::PageId(Id::ZERO), block.created, revision); + // A page with one block can't hold a duplicate ID. + let _ = page.blocks.insert(Arc::new(block.clone())); + serde_json::from_slice::(&crate::format::page_json::write_page(&page)) + .ok() + .and_then(|mut v| v.get_mut("blocks")?.as_array_mut()?.pop()) + .unwrap_or(serde_json::Value::Null) +} + +/// [`differing_blocks`] as JSON for the app's conflict view: `{ id, type, mine, theirs }`, where a side without +/// the block is `null` and the type is the one on this device, else the other. +pub fn differing_blocks_json(mine: &Page, theirs: &Page) -> Vec { + differing_blocks(mine, theirs) + .into_iter() + .map(|(id, m, t)| { + let type_name = m.or(t).map(Block::type_name).unwrap_or_default(); + serde_json::json!({ + "id": id.to_string(), + "type": type_name, + "mine": m.map(block_json), + "theirs": t.map(block_json), + }) + }) + .collect() +} + +/// One page from two versions that changed `base` concurrently, or `None` to keep today's conflict copy. See the +/// module documentation for when it merges. +pub fn merge_page(mergers: &Mergers, base: &Page, mine: &Page, theirs: &Page) -> Option { + if mergers.is_empty() || mine.id != base.id || theirs.id != base.id || mine.revision.id == theirs.revision.id { + return None; + } + let ids: BTreeSet = [base, mine, theirs] + .iter() + .flat_map(|p| p.blocks.iter().map(|b| b.id)) + .collect(); + let mut opted_in = false; + let mut kept: Vec> = Vec::new(); + for id in ids { + let (b, m, t) = (base.blocks.get(id), mine.blocks.get(id), theirs.blocks.get(id)); + let changed = m != b || t != b; + if changed { + opted_in |= [b, m, t].into_iter().flatten().any(|x| mergers.handles(x.type_name())); + } + let result = match (b, m, t) { + (Some(b), Some(m), Some(t)) => match mergers.merge_block(b.type_name(), b, m, t) { + BlockMerge::Merged(block) if *block == **m => Some(m.clone()), + BlockMerge::Merged(block) if *block == **t => Some(t.clone()), + BlockMerge::Merged(block) => Some(Arc::new(*block)), + BlockMerge::Conflict => return None, + }, + // Deleted on one side or both: it stays deleted unless the other side changed it. + (Some(b), m, t) => { + if m.is_some_and(|m| m != b) || t.is_some_and(|t| t != b) { + return None; + } + None + } + // Added on one side, or the same block added on both. + (None, m, t) => match (m, t) { + (Some(m), Some(t)) if m != t => return None, + (m, t) => m.or(t).cloned(), + }, + }; + kept.extend(result); + } + if !opted_in { + return None; + } + let mut merged = later(mine, theirs).clone(); + merged.blocks = crate::model::blocks::Blocks::new(); + for block in kept { + merged.blocks.insert(block).ok()?; + } + merged.title = pick(&base.title, &mine.title, &theirs.title)?.clone(); + merged.created = *pick(&base.created, &mine.created, &theirs.created)?; + merged.modified = mine.modified.max(theirs.modified); + merged.tags = pick(&base.tags, &mine.tags, &theirs.tags)?.clone(); + merged.view = pick(&base.view, &mine.view, &theirs.view)?.clone(); + merged.assets = pick_map(&base.assets, &mine.assets, &theirs.assets)?; + merged.ink = pick(&base.ink, &mine.ink, &theirs.ink)?.clone(); + merged.recordings = pick(&base.recordings, &mine.recordings, &theirs.recordings)?.clone(); + merged.encryption = pick(&base.encryption, &mine.encryption, &theirs.encryption)?.clone(); + merged.extra = pick(&base.extra, &mine.extra, &theirs.extra)?.clone(); + merged.revision = merged_revision(&mine.revision, &theirs.revision); + Some(merged) +} + +/// The side whose revision was saved later, then sorts later: the same on every device. +fn later<'a>(a: &'a Page, b: &'a Page) -> &'a Page { + if (b.revision.saved_at, b.revision.id) > (a.revision.saved_at, a.revision.id) { + b + } else { + a + } +} + +/// The revision of a merged page, made from the two revisions alone. Its ID's time is one millisecond after the +/// later of the two, so it sorts after both, and its random part joins theirs. Both are its parents and +/// ancestors, so sync sees the two as older saves of it. +pub fn merged_revision(a: &Revision, b: &Revision) -> Revision { + let low = |id: RevisionId| u128::from_be_bytes(*id.0.as_bytes()); + let time = a.id.0.time_ms().max(b.id.0.time_ms()).saturating_add(1); + let id = RevisionId(Id::from_parts(time, low(a.id) ^ low(b.id))); + let newer = if (b.saved_at, b.id) > (a.saved_at, a.id) { b } else { a }; + let mut parents = vec![a.id, b.id]; + parents.sort_unstable_by(|x, y| y.cmp(x)); + let mut ancestors: Vec = parents + .iter() + .chain(&a.ancestors) + .chain(&b.ancestors) + .copied() + .collect::>() + .into_iter() + .rev() + .collect(); + ancestors.truncate(MAX_ANCESTORS); + Revision { + id, + parents, + ancestors, + saved_at: newer.saved_at, + device: newer.device.clone(), + writer: newer.writer.clone(), + extra: newer.extra.clone(), + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::format::page_json::{read_page, write_page}; + use crate::id::{DeviceId, PageId}; + use crate::model::block::{BlockData, Fallback, OtherData, TextData}; + use crate::model::{DeviceRef, JsonMap}; + use crate::{Limits, OrderKey, Timestamp}; + use serde_json::{json, Value}; + + const TEST_TYPE: &str = "test.tags"; + + /// A fake merger for a test type whose data is a set of tags: a tag added on either side is added, and a tag + /// removed on either side is removed. Any other change on both sides can't merge. + struct Tags; + + fn tags(block: &Block) -> BTreeSet { + match &block.data { + BlockData::Other(other) => other + .data + .get("tags") + .and_then(Value::as_array) + .map(|a| a.iter().filter_map(|v| v.as_str().map(str::to_owned)).collect()) + .unwrap_or_default(), + _ => BTreeSet::new(), + } + } + + fn with_tags(block: &Block, set: &BTreeSet, modified: Timestamp) -> Block { + let mut out = block.clone(); + if let BlockData::Other(other) = &mut out.data { + other.data.insert("tags".into(), json!(set.iter().collect::>())); + } + out.modified = modified; + out + } + + impl BlockMerger for Tags { + fn merge(&self, base: &Block, mine: &Block, theirs: &Block) -> Option { + let strip = |b: &Block| with_tags(b, &BTreeSet::new(), base.modified); + if strip(mine) != strip(base) || strip(theirs) != strip(base) { + return None; + } + let (b, m, t) = (tags(base), tags(mine), tags(theirs)); + let set: BTreeSet = m + .union(&t) + .filter(|tag| !(b.contains(*tag) && (!m.contains(*tag) || !t.contains(*tag)))) + .cloned() + .collect(); + Some(with_tags(base, &set, mine.modified.max(theirs.modified))) + } + } + + fn fake() -> Mergers { + let mut m = Mergers::new(); + m.register(TEST_TYPE, Arc::new(Tags)); + m + } + + fn id(n: u128) -> Id { + Id::from_parts(1_760_000_000_000, n) + } + + fn at(ms: i64) -> Timestamp { + Timestamp::from_unix_ms(1_760_000_000_000 + ms) + } + + fn key(n: u128) -> OrderKey { + let mut k = OrderKey::between(None, None).expect("order"); + for _ in 0..(n % 64) { + k = OrderKey::between(Some(&k), None).expect("order"); + } + k + } + + fn block(n: u128, data: BlockData) -> Block { + let fallback = matches!(data, BlockData::Other(_)).then(|| Fallback { + markdown: "Tags".into(), + image: None, + extra: JsonMap::new(), + }); + Block { + id: BlockId(id(n)), + order: key(n), + frame: None, + lock: None, + created: at(0), + modified: at(0), + data, + fallback, + extra: JsonMap::new(), + } + } + + fn tag_block(n: u128, list: &[&str]) -> Block { + let mut data = JsonMap::new(); + data.insert("tags".into(), json!(list)); + block( + n, + BlockData::Other(OtherData { + type_name: TEST_TYPE.into(), + data, + unreadable: None, + }), + ) + } + + fn text_block(n: u128, text: &str) -> Block { + block( + n, + BlockData::Text(TextData { + markdown: text.into(), + ..TextData::default() + }), + ) + } + + fn revision(n: u128, ancestors: &[u128], saved: i64) -> Revision { + let mut r = Revision::new( + RevisionId(id(1000 + n)), + at(saved), + DeviceRef { + id: DeviceId(id(9)), + label: "Test device".into(), + }, + "OpenNote test", + ); + r.ancestors = ancestors.iter().map(|a| RevisionId(id(1000 + a))).collect(); + r.parents = r.ancestors.iter().take(1).copied().collect(); + r + } + + fn page(rev: Revision, blocks: &[Block]) -> Page { + let mut p = Page::new(PageId(id(1)), at(0), rev); + for b in blocks { + p.blocks.insert(Arc::new(b.clone())).expect("unique"); + } + p + } + + fn base() -> Page { + page( + revision(1, &[], 10), + &[tag_block(1, &["a"]), tag_block(2, &["b"]), text_block(3, "Hello")], + ) + } + + fn edit(rev: Revision, f: impl FnOnce(&mut Page)) -> Page { + let mut p = base(); + p.revision = rev; + f(&mut p); + p + } + + fn set(p: &mut Page, b: Block) { + p.blocks.replace(Arc::new(b)).expect("present"); + } + + #[test] + fn with_no_merger_a_block_changed_on_both_sides_is_a_conflict() { + let (b, m, t) = ( + tag_block(1, &["a"]), + tag_block(1, &["a", "m"]), + tag_block(1, &["a", "t"]), + ); + assert_eq!(Mergers::new().merge_block(TEST_TYPE, &b, &m, &t), BlockMerge::Conflict); + assert_eq!( + merge_block(TEST_TYPE, &b, &m, &t), + BlockMerge::Conflict, + "nothing registered here" + ); + // A side that kept the base, or equal sides, need no merger. + assert_eq!( + Mergers::new().merge_block(TEST_TYPE, &b, &m, &b), + BlockMerge::Merged(Box::new(m.clone())) + ); + assert_eq!( + Mergers::new().merge_block(TEST_TYPE, &b, &b, &t), + BlockMerge::Merged(Box::new(t.clone())) + ); + assert_eq!( + Mergers::new().merge_block(TEST_TYPE, &b, &m, &m), + BlockMerge::Merged(Box::new(m.clone())) + ); + } + + #[test] + fn a_registered_merger_merges_its_type_only() { + let (b, m, t) = (tag_block(1, &["a"]), tag_block(1, &["a", "m"]), tag_block(1, &["t"])); + let BlockMerge::Merged(merged) = fake().merge_block(TEST_TYPE, &b, &m, &t) else { + panic!("the fake merger merges tags"); + }; + assert_eq!(tags(&merged), ["m", "t"].iter().map(|s| (*s).to_owned()).collect()); + let (tb, tm, tt) = (text_block(3, "a"), text_block(3, "b"), text_block(3, "c")); + assert_eq!(fake().merge_block("text", &tb, &tm, &tt), BlockMerge::Conflict); + // A merger never sees blocks of another type. + assert_eq!(fake().merge_block(TEST_TYPE, &tb, &tm, &tt), BlockMerge::Conflict); + } + + #[test] + fn with_no_merger_registered_no_page_merges() { + let mine = edit(revision(2, &[1], 20), |p| set(p, tag_block(1, &["a", "m"]))); + let theirs = edit(revision(3, &[1], 30), |p| set(p, tag_block(2, &["b", "t"]))); + assert_eq!(merge_page(&Mergers::new(), &base(), &mine, &theirs), None); + } + + #[test] + fn two_devices_that_changed_different_blocks_merge() { + let mine = edit(revision(2, &[1], 20), |p| set(p, tag_block(1, &["a", "m"]))); + let theirs = edit(revision(3, &[1], 30), |p| { + set(p, tag_block(2, &["b", "t"])); + set(p, text_block(3, "Hello there")); + }); + let merged = merge_page(&fake(), &base(), &mine, &theirs).expect("merges"); + assert_eq!( + tags(merged.blocks.get(BlockId(id(1))).expect("1")), + ["a", "m"].map(String::from).into() + ); + assert_eq!( + tags(merged.blocks.get(BlockId(id(2))).expect("2")), + ["b", "t"].map(String::from).into() + ); + assert_eq!( + merged.plain_text(), + theirs.plain_text(), + "the text change on one side stays" + ); + let parents: Vec = vec![theirs.revision.id, mine.revision.id]; + assert_eq!(merged.revision.parents, parents); + assert!(merged.revision.id > theirs.revision.id); + assert_eq!(merged.revision.ancestors[..2], parents[..]); + // The merged page writes and reads back. + let read = read_page(&write_page(&merged), &Limits::default()).expect("reads").page; + assert_eq!(read.blocks, merged.blocks); + } + + #[test] + fn a_page_whose_changes_have_no_merger_keeps_the_conflict_copy() { + // Only text changed, and text has no merger. + let mine = edit(revision(2, &[1], 20), |p| set(p, text_block(3, "Mine"))); + let theirs = edit(revision(3, &[1], 30), |p| set(p, text_block(3, "Theirs"))); + assert_eq!(merge_page(&fake(), &base(), &mine, &theirs), None); + // A block changed on one side and deleted on the other. + let mine = edit(revision(2, &[1], 20), |p| set(p, tag_block(1, &["m"]))); + let theirs = edit(revision(3, &[1], 30), |p| { + p.blocks.remove(BlockId(id(1))); + }); + assert_eq!(merge_page(&fake(), &base(), &mine, &theirs), None); + // The title changed differently on both sides. + let mine = edit(revision(2, &[1], 20), |p| { + set(p, tag_block(1, &["m"])); + p.title = "Mine".into(); + }); + let theirs = edit(revision(3, &[1], 30), |p| p.title = "Theirs".into()); + assert_eq!(merge_page(&fake(), &base(), &mine, &theirs), None); + } + + #[test] + fn differing_blocks_name_each_changed_block_with_its_type() { + let mine = edit(revision(2, &[1], 20), |p| set(p, tag_block(1, &["m"]))); + let theirs = edit(revision(3, &[1], 30), |p| { + p.blocks.remove(BlockId(id(3))); + }); + let list = differing_blocks_json(&mine, &theirs); + assert_eq!(list.len(), 2, "{list:?}"); + assert_eq!(list[0]["id"], json!(BlockId(id(1)).to_string())); + assert_eq!(list[0]["type"], json!(TEST_TYPE)); + assert_eq!(list[0]["mine"]["data"]["tags"], json!(["m"])); + assert_eq!(list[0]["theirs"]["data"]["tags"], json!(["a"])); + assert_eq!(list[1]["type"], json!("text")); + assert_eq!(list[1]["theirs"], Value::Null); + } + + /// A small, seeded generator, so the generated cases are the same on every run. + struct Rng(u64); + + impl Rng { + fn next(&mut self, n: u64) -> u64 { + self.0 ^= self.0 << 13; + self.0 ^= self.0 >> 7; + self.0 ^= self.0 << 17; + self.0 % n + } + } + + fn random_edit(rng: &mut Rng, rev: Revision, side: u128) -> Page { + edit(rev, |p| { + for n in 1..=3u128 { + match rng.next(6) { + 0 if n < 3 => { + let mut t = tags(p.blocks.get(BlockId(id(n))).expect("block")); + t.insert(format!("s{side}-{}", rng.next(3))); + set(p, with_tags(&tag_block(n, &[]), &t, at(rng.next(50) as i64))); + } + 1 if n < 3 => set(p, tag_block(n, &[])), + 2 if n == 3 => set(p, text_block(3, &format!("Text {}", rng.next(2)))), + 3 => { + p.blocks.remove(BlockId(id(n))); + } + _ => {} + } + } + if rng.next(3) == 0 { + p.blocks + .insert(Arc::new(tag_block(100 + side * 10 + u128::from(rng.next(2)), &["new"]))) + .expect("unique"); + } + match rng.next(8) { + 0 => p.title = format!("Title {}", rng.next(2)), + 1 => p.tags = vec![format!("tag{}", rng.next(2))], + _ => {} + } + }) + } + + #[test] + fn the_merge_does_not_depend_on_the_order_of_the_sides() { + let mut rng = Rng(0x9e37_79b9_7f4a_7c15); + let mut merged = 0; + for case in 0..2000u128 { + let (sa, sb) = (20 + rng.next(30) as i64, 20 + rng.next(30) as i64); + let a = random_edit(&mut rng, revision(2 + case * 2, &[1], sa), 1); + let b = random_edit(&mut rng, revision(3 + case * 2, &[1], sb), 2); + let ab = merge_page(&fake(), &base(), &a, &b); + let ba = merge_page(&fake(), &base(), &b, &a); + assert_eq!(ab.as_ref().map(write_page), ba.as_ref().map(write_page), "case {case}"); + merged += usize::from(ab.is_some()); + } + assert!(merged > 200, "enough generated cases merge to mean something: {merged}"); + } +} From 9bfe161d2f94d86a550ce23222dfa23f4902be19 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 03:57:44 -0800 Subject: [PATCH 02/49] Merge concurrent page saves by block when a merger opts in Before the page rule, sync tries the block merge for two concurrent live versions of page.json, from the newest .history snapshot both list as an ancestor. The process-wide mergers take part; merge_with takes explicit ones for tests. With no merger registered the merged manifest and its conflict copies are byte-identical to before. Co-Authored-By: Claude Opus 5.5 --- crates/sync/src/merge.rs | 107 ++++++++-- crates/sync/tests/block_merge.rs | 335 +++++++++++++++++++++++++++++++ 2 files changed, 430 insertions(+), 12 deletions(-) create mode 100644 crates/sync/tests/block_merge.rs diff --git a/crates/sync/src/merge.rs b/crates/sync/src/merge.rs index ad1b58bc1..c699cfa79 100644 --- a/crates/sync/src/merge.rs +++ b/crates/sync/src/merge.rs @@ -4,15 +4,18 @@ //! A version that another has seen loses to it. Versions that saw nothing of each other are concurrent: equal //! contents just join, an edit beats a delete, and different contents follow the rule for the kind of file. No rule //! drops text: when two versions of a page can't both be `page.json`, the other one goes to `.conflicts/`, which -//! the app shows as a conflict to review (spec 14.1). +//! the app shows as a conflict to review (spec 14.1). Before that, block types that registered a merger may merge +//! the two versions of a page into one (ADR 0089). use std::collections::{BTreeMap, BTreeSet}; use std::io::Read; use opennote_core::format::history_json::{merge_versions, read_versions, write_versions}; -use opennote_core::format::page_json::read_page; +use opennote_core::format::page_json::{read_page, write_page}; use opennote_core::format::tree_json::{merge_notebooks, merge_sections, read_notebook, read_section}; use opennote_core::format::tree_json::{write_notebook, write_section}; +use opennote_core::merge::{merge_page, mergers, Mergers}; +use opennote_core::model::Page; use opennote_core::Limits; use crate::error::{Result, SyncError}; @@ -335,25 +338,26 @@ fn merge_tree(path: &str, live: &[Content], contents: &dyn Contents) -> Result BTreeSet { +/// A page file, read whether it is plain or gzip-compressed, as `.history` snapshots are. +fn read_any_page(bytes: &[u8]) -> Option { let mut plain = Vec::new(); let bytes = if bytes.starts_with(&[0x1f, 0x8b]) { - if flate2::read::GzDecoder::new(bytes) + flate2::read::GzDecoder::new(bytes) .take(64 * 1024 * 1024) .read_to_end(&mut plain) - .is_err() - { - return BTreeSet::new(); - } + .ok()?; &plain[..] } else { bytes }; - let Ok(read) = read_page(bytes, &Limits::default()) else { + read_page(bytes, &Limits::default()).ok().map(|read| read.page) +} + +/// The ink segments and assets a page file points at, as paths relative to its page folder. +fn page_refs(bytes: &[u8]) -> BTreeSet { + let Some(page) = read_any_page(bytes) else { return BTreeSet::new(); }; - let page = read.page; let mut refs: BTreeSet = page .ink .segments() @@ -364,15 +368,94 @@ fn page_refs(bytes: &[u8]) -> BTreeSet { refs } +/// Two devices changed one page, and neither saw the other's save: the one page that block mergers make of the +/// two versions and the newest `.history` snapshot both started from (ADR 0089), or `None` to keep today's +/// conflict copy. With no merger registered it is always `None`, so nothing changes. +fn merge_page_blocks( + path: &str, + candidates: &[Entry], + by_path: &BTreeMap<&str, Vec>, + contents: &dyn Contents, + mergers: &Mergers, +) -> Result> { + if mergers.is_empty() || kind(path) != Kind::Page { + return Ok(None); + } + let top = maximal(candidates.to_vec()); + let [a, b] = &top[..] else { return Ok(None) }; + if a.deleted || b.deleted || a.content == b.content { + return Ok(None); + } + let (Some(mine), Some(theirs)) = ( + read_any_page(&contents.bytes(&a.content)?), + read_any_page(&contents.bytes(&b.content)?), + ) else { + return Ok(None); + }; + let (ra, rb) = (&mine.revision, &theirs.revision); + if ra.ancestors.contains(&rb.id) || rb.ancestors.contains(&ra.id) { + return Ok(None); + } + let shared: BTreeSet<_> = ra.ancestors.iter().filter(|r| rb.ancestors.contains(r)).collect(); + let (dir, _) = split(path); + let prefix = if dir.is_empty() { + String::new() + } else { + format!("{dir}/") + }; + let mut base = None; + for rev in shared.into_iter().rev() { + let snapshot = format!("{prefix}.history/{rev}.json.gz"); + let Some(entry) = by_path.get(snapshot.as_str()).and_then(|e| e.iter().find(|e| e.live())) else { + continue; + }; + if let Some(page) = read_any_page(&contents.bytes(&entry.content)?) { + base = Some(page); + break; + } + } + let Some(merged) = base.and_then(|base| merge_page(mergers, &base, &mine, &theirs)) else { + return Ok(None); + }; + let bytes = write_page(&merged); + if read_page(&bytes, &Limits::default()).is_err() { + return Ok(None); + } + Ok(Some(Entry { + vv: join(&a.vv, &b.vv), + content: contents.make(bytes), + deleted: false, + })) +} + /// Merges manifests. `before` is this device's own manifest before the merge: folders where nothing changed are -/// not checked again. +/// not checked again. Block mergers registered for this process take part (ADR 0089). pub fn merge(inputs: &[&Manifest], before: &Manifest, contents: &dyn Contents) -> Result { + merge_with(inputs, before, contents, &mergers()) +} + +/// [`merge`] with these block mergers. +pub fn merge_with( + inputs: &[&Manifest], + before: &Manifest, + contents: &dyn Contents, + mergers: &Mergers, +) -> Result { let mut by_path: BTreeMap<&str, Vec> = BTreeMap::new(); for manifest in inputs { for (path, entry) in manifest.iter() { by_path.entry(path.as_str()).or_default().push(entry.clone()); } } + let mut block_merged = Vec::new(); + for (path, candidates) in &by_path { + if let Some(entry) = merge_page_blocks(path, candidates, &by_path, contents, mergers)? { + block_merged.push((*path, entry)); + } + } + for (path, entry) in block_merged { + by_path.insert(path, vec![entry]); + } let mut merged = Merged::default(); let mut extras: Vec<(String, String, Entry)> = Vec::new(); let mut concurrent: BTreeSet = BTreeSet::new(); diff --git a/crates/sync/tests/block_merge.rs b/crates/sync/tests/block_merge.rs new file mode 100644 index 000000000..6c539f423 --- /dev/null +++ b/crates/sync/tests/block_merge.rs @@ -0,0 +1,335 @@ +//! Block-level merge of a page that two devices changed (ADR 0089): with no merger registered the result is +//! today's conflict copy, byte for byte, and a registered merger merges blocks of its own type. + +use std::collections::{BTreeMap, BTreeSet}; +use std::io::Write; +use std::sync::{Arc, Mutex}; + +use opennote_core::format::page_json::{read_page, write_page}; +use opennote_core::id::{BlockId, DeviceId, Id, PageId, RevisionId, SectionId}; +use opennote_core::merge::{BlockMerger, Mergers}; +use opennote_core::model::block::{BlockData, Fallback, OtherData, TextData}; +use opennote_core::model::{Block, DeviceRef, JsonMap, Page, Revision}; +use opennote_core::{Limits, OrderKey, Timestamp}; +use opennote_sync::manifest::{Content, Entry, Manifest}; +use opennote_sync::merge::{merge, merge_with, Contents, Merged}; +use opennote_sync::Result; +use serde_json::{json, Value}; + +const TEST_TYPE: &str = "test.tags"; + +/// Contents kept in memory, named by a hash of their bytes. +#[derive(Default)] +struct Store(Mutex>>); + +fn content_of(bytes: &[u8]) -> Content { + // FNV-1a: enough to name test contents apart. + let hash = bytes.iter().fold(0xcbf2_9ce4_8422_2325u64, |h, b| { + (h ^ u64::from(*b)).wrapping_mul(0x100_0000_01b3) + }); + Content { + id: format!("{hash:016x}{:08x}", bytes.len()), + size: bytes.len() as u64, + chunks: vec![], + } +} + +impl Store { + fn put(&self, bytes: Vec) -> Content { + let content = content_of(&bytes); + self.0.lock().expect("lock").insert(content.id.clone(), bytes); + content + } +} + +impl Contents for Store { + fn bytes(&self, content: &Content) -> Result> { + Ok(self.0.lock().expect("lock")[&content.id].clone()) + } + fn make(&self, bytes: Vec) -> Content { + self.put(bytes) + } +} + +/// A fake merger for the test type: tags added on either side are added, and tags removed on either side go. +struct Tags; + +fn tags(block: &Block) -> BTreeSet { + match &block.data { + BlockData::Other(o) => o.data["tags"] + .as_array() + .map(|a| a.iter().filter_map(|v| v.as_str().map(str::to_owned)).collect()) + .unwrap_or_default(), + _ => BTreeSet::new(), + } +} + +impl BlockMerger for Tags { + fn merge(&self, base: &Block, mine: &Block, theirs: &Block) -> Option { + let (b, m, t) = (tags(base), tags(mine), tags(theirs)); + let keep: Vec = m + .union(&t) + .filter(|tag| !(b.contains(*tag) && (!m.contains(*tag) || !t.contains(*tag)))) + .cloned() + .collect(); + Some(tag_block_from(base, &keep)) + } +} + +fn fake() -> Mergers { + let mut m = Mergers::new(); + m.register(TEST_TYPE, Arc::new(Tags)); + m +} + +fn id(n: u128) -> Id { + Id::from_parts(1_760_000_000_000, n) +} + +fn block(n: u128, data: BlockData) -> Block { + let mut order = OrderKey::between(None, None).expect("order"); + for _ in 0..n { + order = OrderKey::between(Some(&order), None).expect("order"); + } + Block { + id: BlockId(id(n)), + order, + frame: None, + lock: None, + created: Timestamp::from_unix_ms(1_760_000_000_000), + modified: Timestamp::from_unix_ms(1_760_000_000_000), + fallback: matches!(data, BlockData::Other(_)).then(|| Fallback { + markdown: "Tags".into(), + image: None, + extra: JsonMap::new(), + }), + data, + extra: JsonMap::new(), + } +} + +fn tag_block(n: u128, list: &[&str]) -> Block { + let mut data = JsonMap::new(); + data.insert("tags".into(), json!(list)); + block( + n, + BlockData::Other(OtherData { + type_name: TEST_TYPE.into(), + data, + unreadable: None, + }), + ) +} + +fn tag_block_from(base: &Block, list: &[String]) -> Block { + let mut out = base.clone(); + if let BlockData::Other(o) = &mut out.data { + o.data.insert("tags".into(), Value::from(list.to_vec())); + } + out +} + +fn text_block(n: u128, text: &str) -> Block { + block( + n, + BlockData::Text(TextData { + markdown: text.into(), + ..TextData::default() + }), + ) +} + +fn page(revision: u128, ancestors: &[u128], saved: i64, blocks: &[Block]) -> Page { + let rev = |n: u128| RevisionId(id(1000 + n)); + let mut r = Revision::new( + rev(revision), + Timestamp::from_unix_ms(1_760_000_000_000 + saved), + DeviceRef { + id: DeviceId(id(9)), + label: "Windows device TEST".into(), + }, + "OpenNote test", + ); + r.ancestors = ancestors.iter().map(|a| rev(*a)).collect(); + r.parents = r.ancestors.iter().take(1).copied().collect(); + let mut p = Page::new(PageId(id(1)), Timestamp::from_unix_ms(1_760_000_000_000), r); + for b in blocks { + p.blocks.insert(Arc::new(b.clone())).expect("unique"); + } + p +} + +fn dir() -> String { + format!("Bio/{}/{}", SectionId(id(1)), PageId(id(1))) +} + +fn gzip(bytes: &[u8]) -> Vec { + let mut gz = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + gz.write_all(bytes).expect("gzip"); + gz.finish().expect("gzip") +} + +fn entry(device: &str, count: u64, content: Content) -> Entry { + Entry { + vv: [(device.to_owned(), count)].into(), + content, + deleted: false, + } +} + +/// Two devices that both had `base` (saved and kept in `.history`) and then saved `mine` and `theirs`. +fn devices(store: &Store, base: &Page, mine: &Page, theirs: &Page) -> (Manifest, Manifest) { + let (page, history) = ( + format!("{}/page.json", dir()), + format!("{}/.history/{}.json.gz", dir(), base.revision.id), + ); + let snapshot = entry("d0", 1, store.put(gzip(&write_page(base)))); + let mut a = Manifest::new(); + a.insert(history.clone(), snapshot.clone()); + let mut b = a.clone(); + let va = entry("d0", 2, store.put(write_page(mine))); + let mut vb = entry("d0", 1, store.put(write_page(theirs))); + vb.vv.insert("d1".into(), 1); + a.insert(page.clone(), va); + b.insert(page, vb); + (a, b) +} + +fn base_page() -> Page { + page( + 1, + &[], + 10, + &[tag_block(1, &["a"]), tag_block(2, &["b"]), text_block(3, "Hello")], + ) +} + +fn page_of(store: &Store, merged: &Merged) -> Page { + let e = &merged.entries[&format!("{}/page.json", dir())]; + read_page(&store.bytes(&e.content).expect("bytes"), &Limits::default()) + .expect("reads") + .page +} + +#[test] +fn with_no_merger_a_concurrent_page_edit_is_still_a_conflict_copy() { + let store = Store::default(); + let base = base_page(); + let blocks = |a: &[&str], b: &[&str]| [tag_block(1, a), tag_block(2, b), text_block(3, "Hello")]; + let mine = page(2, &[1], 20, &blocks(&["a", "m"], &["b"])); + let theirs = page(3, &[1], 30, &blocks(&["a"], &["b", "t"])); + let (a, b) = devices(&store, &base, &mine, &theirs); + let none = merge_with(&[&a, &b], &a, &store, &Mergers::new()).expect("merges"); + let global = merge(&[&a, &b], &a, &store).expect("merges"); + assert_eq!(none.entries, global.entries, "nothing is registered in this process"); + assert_eq!(none.conflicts, global.conflicts); + assert_eq!(none.conflicts.len(), 1, "{:?}", none.conflicts); + let copy = format!("{}/.conflicts/{}.json", dir(), mine.revision.id); + assert_eq!(none.conflicts[0].kept_as, copy); + assert_eq!( + store.bytes(&none.entries[©].content).expect("copy"), + write_page(&mine) + ); + let kept = &none.entries[&format!("{}/page.json", dir())]; + assert_eq!( + store.bytes(&kept.content).expect("page"), + write_page(&theirs), + "the later save stays" + ); +} + +#[test] +fn a_merger_for_another_type_leaves_text_conflicts_byte_identical() { + let store = Store::default(); + let base = base_page(); + let edit = |n: u128, saved: i64, text: &str| { + page( + n, + &[1], + saved, + &[tag_block(1, &["a"]), tag_block(2, &["b"]), text_block(3, text)], + ) + }; + let (a, b) = devices(&store, &base, &edit(2, 20, "Mine"), &edit(3, 30, "Theirs")); + let none = merge_with(&[&a, &b], &a, &store, &Mergers::new()).expect("merges"); + let with = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + assert_eq!(none.entries, with.entries); + assert_eq!(none.conflicts, with.conflicts); + assert_eq!(with.conflicts.len(), 1); +} + +#[test] +fn a_registered_merger_merges_two_devices_that_changed_different_blocks() { + let store = Store::default(); + let base = base_page(); + let mine = page( + 2, + &[1], + 20, + &[tag_block(1, &["a", "m"]), tag_block(2, &["b"]), text_block(3, "Hello")], + ); + let theirs = page( + 3, + &[1], + 30, + &[tag_block(1, &["a"]), tag_block(2, &["t"]), text_block(3, "Hello")], + ); + let (a, b) = devices(&store, &base, &mine, &theirs); + let ab = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + let ba = merge_with(&[&b, &a], &b, &store, &fake()).expect("merges"); + assert!(ab.conflicts.is_empty(), "no conflict copy: {:?}", ab.conflicts); + assert!(!ab.entries.keys().any(|k| k.contains(".conflicts"))); + assert_eq!(ab.entries, ba.entries, "both devices reach the same files"); + let merged = page_of(&store, &ab); + let at = |n: u128| tags(merged.blocks.get(BlockId(id(n))).expect("block")); + assert_eq!(at(1), ["a", "m"].map(String::from).into()); + assert_eq!(at(2), ["t"].map(String::from).into()); + assert!(merged.revision.ancestors.contains(&mine.revision.id)); + assert!(merged.revision.ancestors.contains(&theirs.revision.id)); +} + +#[test] +fn the_merged_page_does_not_depend_on_which_device_merges() { + let store = Store::default(); + let base = base_page(); + let lists: [&[&str]; 4] = [&["a"], &["a", "x"], &[], &["y"]]; + for (i, mine_tags) in lists.iter().enumerate() { + for (j, theirs_tags) in lists.iter().enumerate() { + let mine = page(2, &[1], 20 + i as i64, &[tag_block(1, mine_tags), tag_block(2, &["b"])]); + let theirs = page( + 3, + &[1], + 20 + j as i64, + &[tag_block(1, theirs_tags), tag_block(2, &["b", "z"])], + ); + let (a, b) = devices(&store, &base, &mine, &theirs); + let ab = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + let ba = merge_with(&[&b, &a], &b, &store, &fake()).expect("merges"); + assert_eq!(ab.entries, ba.entries, "case {i} {j}"); + assert_eq!(ab.conflicts, ba.conflicts, "case {i} {j}"); + } + } +} + +#[test] +fn with_no_shared_snapshot_the_page_keeps_the_conflict_copy() { + let store = Store::default(); + let base = base_page(); + let mine = page( + 2, + &[1], + 20, + &[tag_block(1, &["a", "m"]), tag_block(2, &["b"]), text_block(3, "Hello")], + ); + let theirs = page( + 3, + &[1], + 30, + &[tag_block(1, &["a"]), tag_block(2, &["t"]), text_block(3, "Hello")], + ); + let (mut a, mut b) = devices(&store, &base, &mine, &theirs); + a.retain(|k, _| !k.contains(".history")); + b.retain(|k, _| !k.contains(".history")); + let merged = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + assert_eq!(merged.conflicts.len(), 1, "no base to merge from"); +} From 3fadf3933464c472ba21ffe9b623e00739a1fdf9 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 03:57:44 -0800 Subject: [PATCH 03/49] Add a conflict view registry for block types conflict.text now also returns the blocks that differ between the two versions, with their types. A block type registers a compare and resolve view with registerConflictView, and the conflict dialog shows it for that type's blocks and the side-by-side text for the rest. Tables and code projects register theirs in later lanes. Co-Authored-By: Claude Opus 5.5 --- app/src-tauri/src/shellqol/conflicts.rs | 1 + app/src/features/qol/ConflictDialog.tsx | 15 +++- .../features/qol/conflict/registry.test.tsx | 83 +++++++++++++++++++ app/src/features/qol/conflict/registry.ts | 58 +++++++++++++ app/src/features/qol/index.ts | 2 + crates/core/src/session/page.rs | 7 ++ 6 files changed, 163 insertions(+), 3 deletions(-) create mode 100644 app/src/features/qol/conflict/registry.test.tsx create mode 100644 app/src/features/qol/conflict/registry.ts diff --git a/app/src-tauri/src/shellqol/conflicts.rs b/app/src-tauri/src/shellqol/conflicts.rs index 37eadb46a..a28968ff6 100644 --- a/app/src-tauri/src/shellqol/conflicts.rs +++ b/app/src-tauri/src/shellqol/conflicts.rs @@ -25,6 +25,7 @@ pub fn call(app: &AppHandle, name: &str, args: &Value) -> IpcResult { Ok(json!({ "mine": handle.text(), "theirs": handle.conflict_text(revision).map_err(core_error)?, + "blocks": handle.conflict_blocks(revision).map_err(core_error)?, })) } "conflict.resolve" => { diff --git a/app/src/features/qol/ConflictDialog.tsx b/app/src/features/qol/ConflictDialog.tsx index d3555a98b..915b48803 100644 --- a/app/src/features/qol/ConflictDialog.tsx +++ b/app/src/features/qol/ConflictDialog.tsx @@ -1,5 +1,6 @@ // The side-by-side view of a page and its conflict copy: this version on one side, the other on the other, and the // choice to keep this one, keep the other one, or keep both as separate pages. Nothing changes until a choice. +// A block type with a view in the conflict registry shows that view for its changed blocks (ADR 0089). import { useEffect, useState } from 'react'; import { shellCall } from '../../platform/shellqol'; @@ -8,13 +9,14 @@ import { t } from '../../strings/t'; import { Dialog, showToast } from '../../ui'; import { showDialog } from './dialogHost'; import { reloadPage } from './ExternalNotice'; +import { splitByView, type ConflictBlock, type ConflictChoice as Choice } from './conflict/registry'; import styles from './Conflict.module.css'; -type Choice = 'keepMine' | 'keepTheirs' | 'keepBoth'; - interface Texts { readonly mine: string; readonly theirs: string; + /** The blocks that differ, when the shell sends them. */ + readonly blocks?: readonly ConflictBlock[]; } function ConflictContent(props: { @@ -50,6 +52,9 @@ function ConflictContent(props: { showToast({ message: t(`qol.conflict.done.${choice}`) }); }; const other = props.device || t('qol.conflict.otherDevice'); + const { registered, others } = splitByView(texts?.blocks); + // The text view stays for every block without a view of its own, and when only page fields differ. + const showText = others.length > 0 || registered.length === 0; return ( {failed &&

{t('qol.conflict.unreadable')}

} {!failed && !texts &&

{t('qol.conflict.loading')}

} - {texts && ( + {texts && + registered.map(({ block, View }) => ( + void resolve(choice)} /> + ))} + {texts && showText && (

{t('qol.conflict.mine')}

diff --git a/app/src/features/qol/conflict/registry.test.tsx b/app/src/features/qol/conflict/registry.test.tsx new file mode 100644 index 000000000..73cd354b5 --- /dev/null +++ b/app/src/features/qol/conflict/registry.test.tsx @@ -0,0 +1,83 @@ +// The conflict view registry (ADR 0089): a block type's own view shows for its changed blocks, and every other +// block keeps the side-by-side text. +import { screen, within } from '@testing-library/react'; +import { afterEach, describe, expect, it } from 'vitest'; +import { userEvent } from 'vitest/browser'; +import { resetFakeShell, setFakeShell } from '../../../platform/shellqol'; +import { pressChord, renderApp } from '../../../test'; +import { openConflictDialog } from '../ConflictDialog'; +import { registerConflictView, type ConflictViewProps } from './registry'; + +const PAGE = '01m3sa12426sg32pmtyffjaqcf'; +const FIRST = { revision: '01m3sa12426sg32pmtyffjaqc9', device: { label: 'Laptop' }, savedAt: '2026-10-01T10:00:00Z' }; +const TEXTS = { mine: 'Mine says hello', theirs: 'Theirs says goodbye' }; +const block = (id: string, type: string) => ({ id, type, mine: { data: { n: 1 } }, theirs: { data: { n: 2 } } }); + +function TestView(props: ConflictViewProps) { + const n = (side: unknown) => (side as { data: { n: number } }).data.n; + return ( +
+

+ Here {n(props.block.mine)}, on {props.otherDevice} {n(props.block.theirs)} +

+ +
+ ); +} + +let unregister: (() => void) | undefined; + +afterEach(async () => { + unregister?.(); + unregister = undefined; + for (let i = 0; i < 3 && document.querySelector('[role="dialog"]'); i += 1) await pressChord('Escape'); + resetFakeShell(); +}); + +async function open(blocks: unknown) { + const calls: Array<[string, Record]> = []; + setFakeShell((name, args) => { + calls.push([name, args]); + if (name === 'conflict.text') return { ...TEXTS, blocks }; + if (name === 'conflict.resolve') return { ok: true }; + return null; + }); + await renderApp(); + const closed = openConflictDialog(PAGE, FIRST, 1); + const dialog = await screen.findByRole('dialog', { name: 'Two versions of this page' }); + return { calls, closed, dialog }; +} + +describe('the conflict view registry', () => { + it('shows the registered view for its block type and resolves through it', async () => { + unregister = registerConflictView('test.view', TestView); + const { calls, closed, dialog } = await open([block('b1', 'test.view')]); + const view = await within(dialog).findByRole('region', { name: 'Test compare b1' }); + expect(within(view).getByText('Here 1, on Laptop 2')).toBeTruthy(); + expect(within(dialog).queryByText(TEXTS.mine)).toBeNull(); + await userEvent.click(within(view).getByRole('button', { name: 'Take theirs' })); + await closed; + expect(calls.find(([name]) => name === 'conflict.resolve')?.[1]).toEqual({ + pageId: PAGE, + revision: FIRST.revision, + choice: 'keepTheirs', + }); + }); + + it('keeps the text view for block types without a view', async () => { + unregister = registerConflictView('test.view', TestView); + const { dialog } = await open([block('b1', 'test.view'), block('b2', 'text')]); + expect(await within(dialog).findByRole('region', { name: 'Test compare b1' })).toBeTruthy(); + expect(within(dialog).queryByRole('region', { name: 'Test compare b2' })).toBeNull(); + expect(within(dialog).getByText(TEXTS.mine)).toBeTruthy(); + expect(within(dialog).getByText(TEXTS.theirs)).toBeTruthy(); + }); + + it('shows only the text view when no type registered a view', async () => { + const { dialog } = await open([block('b1', 'test.view')]); + expect(await within(dialog).findByText(TEXTS.mine)).toBeTruthy(); + expect(within(dialog).queryByRole('region', { name: /^Test compare/ })).toBeNull(); + }); +}); diff --git a/app/src/features/qol/conflict/registry.ts b/app/src/features/qol/conflict/registry.ts new file mode 100644 index 000000000..69a272795 --- /dev/null +++ b/app/src/features/qol/conflict/registry.ts @@ -0,0 +1,58 @@ +// The conflict view registry (ADR 0089). A block type registers its own compare and resolve view, and the +// conflict dialog shows it for each changed block of that type. Every other block keeps the side-by-side text. +// Tables (table merge on sync) and code projects (the code project conflict view) register theirs. + +import type { ComponentType } from 'react'; + +/** How a conflict is resolved: this version, the other one, or both as separate pages. */ +export type ConflictChoice = 'keepMine' | 'keepTheirs' | 'keepBoth'; + +/** One block that differs between the two versions, as the core writes it in `page.json`. */ +export interface ConflictBlock { + readonly id: string; + readonly type: string; + /** The block on this device, or `null` if this version has no such block. */ + readonly mine: unknown; + /** The block in the other version, or `null` if that version has no such block. */ + readonly theirs: unknown; +} + +export interface ConflictViewProps { + readonly block: ConflictBlock; + /** The other device's label, or a generic name. */ + readonly otherDevice: string; + /** Resolves the whole conflict, as the dialog's own buttons do. */ + resolve(choice: ConflictChoice): void; +} + +export type ConflictView = ComponentType; + +const views = new Map(); + +/** Registers the compare and resolve view for a block type. Returns a function that removes it again. */ +export function registerConflictView(type: string, view: ConflictView): () => void { + views.set(type, view); + return () => { + if (views.get(type) === view) views.delete(type); + }; +} + +/** The view registered for a block type, if any. */ +export function conflictViewFor(type: string): ConflictView | undefined { + return views.get(type); +} + +/** Splits changed blocks into those with a registered view and the rest, which the text view shows. */ +export function splitByView(blocks: readonly ConflictBlock[] | undefined): { + readonly registered: ReadonlyArray<{ block: ConflictBlock; View: ConflictView }>; + readonly others: readonly ConflictBlock[]; +} { + const registered: Array<{ block: ConflictBlock; View: ConflictView }> = []; + const others: ConflictBlock[] = []; + for (const block of blocks ?? []) { + const View = views.get(block.type); + if (View) registered.push({ block, View }); + else others.push(block); + } + return { registered, others }; +} diff --git a/app/src/features/qol/index.ts b/app/src/features/qol/index.ts index c550b6b8f..6cba7d5ca 100644 --- a/app/src/features/qol/index.ts +++ b/app/src/features/qol/index.ts @@ -9,6 +9,8 @@ export { showDialog } from './dialogHost'; export { pagesClient } from './pagesApi'; export { readPrefs, writePrefs } from './prefs'; export { openInNewTab } from './tabs'; +export { registerConflictView } from './conflict/registry'; +export type { ConflictBlock, ConflictChoice, ConflictViewProps } from './conflict/registry'; /** The notice about a page edited in another app loads when first needed. */ export const loadExternalNotice = () => import('./ExternalNotice'); /** A page in a window of its own; it loads only in that window. */ diff --git a/crates/core/src/session/page.rs b/crates/core/src/session/page.rs index f7051373a..19799101b 100644 --- a/crates/core/src/session/page.rs +++ b/crates/core/src/session/page.rs @@ -337,6 +337,13 @@ impl PageHandle { self.session.conflict_page(rev).map(|(_, _, page)| page.plain_text()) } + /// The blocks that differ between the page now and the other version of a conflict, as JSON, so each block + /// type can show its own compare view (ADR 0089). + pub fn conflict_blocks(&self, rev: RevisionId) -> Result, CoreError> { + let (_, _, other) = self.session.conflict_page(rev)?; + Ok(crate::merge::differing_blocks_json(&self.session.state().page, &other)) + } + /// The other version of a conflict, read-only. pub fn open_conflict(&self, rev: RevisionId) -> Result { self.session.open_conflict(rev) From a47a52cabee6e3f0f8096515640e72a2c628d652 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 03:57:45 -0800 Subject: [PATCH 04/49] Record block-level merge on sync in ADR 0089 The follow-up that ADR 0049 named: one merger and one conflict view registry per block type, and when sync merges a page by block. Co-Authored-By: Claude Opus 5.5 --- docs/adr/0089-block-level-merge-on-sync.md | 38 ++++++++++++++++++++++ 1 file changed, 38 insertions(+) create mode 100644 docs/adr/0089-block-level-merge-on-sync.md diff --git a/docs/adr/0089-block-level-merge-on-sync.md b/docs/adr/0089-block-level-merge-on-sync.md new file mode 100644 index 000000000..82cb98298 --- /dev/null +++ b/docs/adr/0089-block-level-merge-on-sync.md @@ -0,0 +1,38 @@ +# ADR 0089: Block-level merge on sync, with a merger and a conflict view per block type + +- Status: Proposed +- Date: 2026-11-07 + +## Context + +When two devices change the same page and neither saw the other's save, sync keeps one version as `page.json` and the other in `.conflicts/` (ADR 0049, format spec 14.1). ADR 0049 lists block-level merge of `page.json` as a follow-up. Tables (ADR 0066) and code projects (ADR 0069) each want to merge their own blocks and to show their own compare view. Without one shared entry point, each would change the sync merge and the conflict dialog on its own. + +## Decision + +One entry point in `crates/core/src/merge` and one view registry in `app/src/features/qol/conflict`. + +- **Mergers.** A block type registers a `BlockMerger` with `opennote_core::merge::register(type, merger)` at startup. `merge_block(type, base, mine, theirs)` returns the merged block, or a conflict. Equal sides, or a side equal to the base, need no merger. With no merger for the type, the result is a conflict, which means today's conflict copy. +- **When a page merges.** Sync tries `merge_page` only when two maximal versions of one `page.json` are both live and neither revision lists the other among its ancestors. The base is the newest revision both list as an ancestor whose `.history/.json.gz` snapshot is in the merged manifests. With no such snapshot, nothing merges. `merge_page` returns a page only when all of these hold: + - At least one changed block has a type with a registered merger. So with no merger registered, every result is byte-identical to today's, and a page whose changes are all in text never merges here. + - Every block that changed on both sides, to different results, has a merger, and it succeeds. + - No block was deleted on one side and changed on the other. + - Every other page field (title, tags, view, ink, recordings, encryption, unknown keys) changed on at most one side. Assets merge per key. `modified` takes the later time. +- **Convergence.** Every device must reach the same bytes from the same versions, in any order. Mergers must be commutative. The merged revision is made from the two revisions alone. Its ID's time is one millisecond after the later one, and its random part is the exclusive or of theirs. Both revisions are its parents and ancestors. Its device, writer, and `saved_at` are the later side's. The merged file's version vector is the join of the two, as for the other merged files. +- **Conflict views.** `conflict.text` also returns `blocks`: each block that differs between the two versions, with its ID, type, and both sides as `page.json` writes them. A block type registers a compare and resolve view with `registerConflictView(type, view)`. The conflict dialog shows that view for each changed block of the type, and the side-by-side text for everything else. A view resolves the conflict with the same three choices as the dialog. + +## Options considered + +| Option | For | Against | +|---|---|---| +| One registry for mergers and one for views, opt in per block type (chosen) | Each program plugs in without touching sync or the dialog; nothing changes until a type registers | A page with a table and text merges text changes only when the table changed too | +| Merge every page whose blocks changed on different sides, even with no merger | More pages merge | Changes today's results for text pages, which the plan rules out until text merges per paragraph | +| Each program edits the sync merge and the dialog | No shared code | Three lanes editing the same two files; rules drift apart | + +## Consequences + +- Tables (ADR 0066) register their merger and view in B14; code projects (ADR 0069) register theirs in B13. +- Two devices on app versions with different mergers can disagree for one round: the newer one merges, and the older one keeps a conflict copy. The merged revision lists both versions as ancestors, so every device keeps the merged page, and the older device's conflict copy stays for the person to review. No text is lost. +- A merge needs a shared `.history` snapshot. History thinning (spec 13.3) can remove the newest one, and then an older shared snapshot is the base, or the page keeps its conflict copy. +- Tests in `crates/sync/tests/block_merge.rs`: with no merger, sync results and conflict copies are byte-identical. +- Tests in `crates/core/src/merge/mod.rs`: with a fake merger, two devices that changed different blocks merge with no conflict copy. `merge(base, a, b)` equals `merge(base, b, a)` over 2,000 generated cases. +- Tests in `app/src/features/qol/conflict/registry.test.tsx`: the conflict dialog shows a registered view, and the text view for the rest. From fddc3d4e498e7580c506c87127bc4777796e622c Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 05:13:12 -0800 Subject: [PATCH 05/49] Merge blocks only against the newest shared revision The block merge on sync used an older shared snapshot as the base when the newest one had none. A merged revision never had a snapshot, so the next concurrent round merged against a stale base and brought undone changes back. Now only the newest shared revision is the base, and the merge publishes a snapshot of the merged revision. merge_page also keeps the conflict copy when a side is read-only or the two sides were read at different version keys (R-VER-5). The conflict dialog keeps the whole-page text under a disclosure when every changed block has its own view, so page-field differences stay visible. Co-Authored-By: Claude Opus 5.5 --- app/src/features/qol/ConflictDialog.tsx | 30 +++++--- .../features/qol/conflict/registry.test.tsx | 8 ++- app/src/strings/en/qol.ts | 1 + crates/core/src/merge/mod.rs | 27 +++++++ crates/sync/src/merge.rs | 71 +++++++++++++------ crates/sync/tests/block_merge.rs | 64 +++++++++++++++++ docs/adr/0089-block-level-merge-on-sync.md | 10 +-- 7 files changed, 175 insertions(+), 36 deletions(-) diff --git a/app/src/features/qol/ConflictDialog.tsx b/app/src/features/qol/ConflictDialog.tsx index 915b48803..848fd1c40 100644 --- a/app/src/features/qol/ConflictDialog.tsx +++ b/app/src/features/qol/ConflictDialog.tsx @@ -55,6 +55,18 @@ function ConflictContent(props: { const { registered, others } = splitByView(texts?.blocks); // The text view stays for every block without a view of its own, and when only page fields differ. const showText = others.length > 0 || registered.length === 0; + const columns = texts && ( +
+
+

{t('qol.conflict.mine')}

+
{texts.mine}
+
+
+

{t('qol.conflict.theirs', { device: other, date: formatDate(props.savedAt) })}

+
{texts.theirs}
+
+
+ ); return ( ( void resolve(choice)} /> ))} - {texts && showText && ( -
-
-

{t('qol.conflict.mine')}

-
{texts.mine}
-
-
-

{t('qol.conflict.theirs', { device: other, date: formatDate(props.savedAt) })}

-
{texts.theirs}
-
-
+ {texts && showText && columns} + {texts && !showText && ( + // Every changed block has its own view, but a title, tag, or other page field may differ too. +
+ {t('qol.conflict.wholePage')} + {columns} +
)}
); diff --git a/app/src/features/qol/conflict/registry.test.tsx b/app/src/features/qol/conflict/registry.test.tsx index 73cd354b5..f1a26cd0f 100644 --- a/app/src/features/qol/conflict/registry.test.tsx +++ b/app/src/features/qol/conflict/registry.test.tsx @@ -56,7 +56,12 @@ describe('the conflict view registry', () => { const { calls, closed, dialog } = await open([block('b1', 'test.view')]); const view = await within(dialog).findByRole('region', { name: 'Test compare b1' }); expect(within(view).getByText('Here 1, on Laptop 2')).toBeTruthy(); - expect(within(dialog).queryByText(TEXTS.mine)).toBeNull(); + // The whole-page text stays one click away, so a title or tag difference is never hidden. + const details = within(dialog).getByText('Both whole pages as text').closest('details'); + expect(details?.open).toBe(false); + await userEvent.click(within(dialog).getByText('Both whole pages as text')); + expect(details?.open).toBe(true); + expect(within(dialog).getByText(TEXTS.mine)).toBeTruthy(); await userEvent.click(within(view).getByRole('button', { name: 'Take theirs' })); await closed; expect(calls.find(([name]) => name === 'conflict.resolve')?.[1]).toEqual({ @@ -73,6 +78,7 @@ describe('the conflict view registry', () => { expect(within(dialog).queryByRole('region', { name: 'Test compare b2' })).toBeNull(); expect(within(dialog).getByText(TEXTS.mine)).toBeTruthy(); expect(within(dialog).getByText(TEXTS.theirs)).toBeTruthy(); + expect(within(dialog).queryByText('Both whole pages as text')).toBeNull(); }); it('shows only the text view when no type registered a view', async () => { diff --git a/app/src/strings/en/qol.ts b/app/src/strings/en/qol.ts index 7af6ef5ac..5b2316e5e 100644 --- a/app/src/strings/en/qol.ts +++ b/app/src/strings/en/qol.ts @@ -134,6 +134,7 @@ export const qol = { mine: 'This version', theirs: 'From {device}, saved {date}', otherDevice: 'another device', + wholePage: 'Both whole pages as text', keepMine: 'Keep this version', keepTheirs: 'Keep the other version', keepBoth: 'Keep both', diff --git a/crates/core/src/merge/mod.rs b/crates/core/src/merge/mod.rs index 888342ec6..0a069356a 100644 --- a/crates/core/src/merge/mod.rs +++ b/crates/core/src/merge/mod.rs @@ -194,6 +194,12 @@ pub fn merge_page(mergers: &Mergers, base: &Page, mine: &Page, theirs: &Page) -> if mergers.is_empty() || mine.id != base.id || theirs.id != base.id || mine.revision.id == theirs.revision.id { return None; } + // A side this device may only read must not be rewritten by sync, and two sides read at different versions + // would leave the merged page stamped for one side while it holds the other's blocks (spec 15, `R-VER-5`). + let keys = |p: &Page| (p.format.version_read, p.format.min_reader, p.format.min_writer); + if mine.format.access.is_read_only() || theirs.format.access.is_read_only() || keys(mine) != keys(theirs) { + return None; + } let ids: BTreeSet = [base, mine, theirs] .iter() .flat_map(|p| p.blocks.iter().map(|b| b.id)) @@ -526,6 +532,27 @@ mod tests { assert_eq!(read.blocks, merged.blocks); } + #[test] + fn a_side_read_as_a_newer_version_or_read_only_keeps_the_conflict_copy() { + let mine = edit(revision(2, &[1], 20), |p| set(p, tag_block(1, &["a", "m"]))); + let theirs = edit(revision(3, &[1], 30), |p| set(p, tag_block(2, &["b", "t"]))); + assert!( + merge_page(&fake(), &base(), &mine, &theirs).is_some(), + "the same sides merge" + ); + // The other side was read as a newer version that this device may still edit (`R-VER-5`). + let mut newer = theirs.clone(); + newer.format.version_read = crate::FORMAT_VERSION + 1; + newer.format.min_writer = crate::FORMAT_VERSION; + assert_eq!(merge_page(&fake(), &base(), &mine, &newer), None); + assert_eq!(merge_page(&fake(), &base(), &newer, &mine), None); + // A side this device may only read. + let mut locked = theirs.clone(); + locked.format.access = crate::model::Access::ReadOnly(crate::model::ReadOnlyReason::NewerFormat); + assert_eq!(merge_page(&fake(), &base(), &mine, &locked), None); + assert_eq!(merge_page(&fake(), &base(), &locked, &mine), None); + } + #[test] fn a_page_whose_changes_have_no_merger_keeps_the_conflict_copy() { // Only text changed, and text has no merger. diff --git a/crates/sync/src/merge.rs b/crates/sync/src/merge.rs index c699cfa79..ddb4f0d34 100644 --- a/crates/sync/src/merge.rs +++ b/crates/sync/src/merge.rs @@ -369,15 +369,20 @@ fn page_refs(bytes: &[u8]) -> BTreeSet { } /// Two devices changed one page, and neither saw the other's save: the one page that block mergers make of the -/// two versions and the newest `.history` snapshot both started from (ADR 0089), or `None` to keep today's -/// conflict copy. With no merger registered it is always `None`, so nothing changes. +/// two versions and the newest revision both started from (ADR 0089), or `None` to keep today's conflict copy. +/// The base is only that newest shared revision: an older one would turn a change one side undid into no change +/// and bring the old value back. Without its `.history` snapshot the result is `None`. With no merger registered +/// it is always `None`, so nothing changes. +/// +/// The merged page comes with a `.history` snapshot of itself (path and entry), so the next merge of this page +/// has its base. fn merge_page_blocks( path: &str, candidates: &[Entry], by_path: &BTreeMap<&str, Vec>, contents: &dyn Contents, mergers: &Mergers, -) -> Result> { +) -> Result> { if mergers.is_empty() || kind(path) != Kind::Page { return Ok(None); } @@ -396,36 +401,53 @@ fn merge_page_blocks( if ra.ancestors.contains(&rb.id) || rb.ancestors.contains(&ra.id) { return Ok(None); } - let shared: BTreeSet<_> = ra.ancestors.iter().filter(|r| rb.ancestors.contains(r)).collect(); + let Some(newest) = ra.ancestors.iter().filter(|r| rb.ancestors.contains(r)).max() else { + return Ok(None); + }; let (dir, _) = split(path); let prefix = if dir.is_empty() { String::new() } else { format!("{dir}/") }; - let mut base = None; - for rev in shared.into_iter().rev() { - let snapshot = format!("{prefix}.history/{rev}.json.gz"); - let Some(entry) = by_path.get(snapshot.as_str()).and_then(|e| e.iter().find(|e| e.live())) else { - continue; - }; - if let Some(page) = read_any_page(&contents.bytes(&entry.content)?) { - base = Some(page); - break; - } - } - let Some(merged) = base.and_then(|base| merge_page(mergers, &base, &mine, &theirs)) else { + let snapshot = format!("{prefix}.history/{newest}.json.gz"); + let Some(entry) = by_path.get(snapshot.as_str()).and_then(|e| e.iter().find(|e| e.live())) else { + return Ok(None); + }; + let Some(base) = read_any_page(&contents.bytes(&entry.content)?) else { + return Ok(None); + }; + let Some(merged) = merge_page(mergers, &base, &mine, &theirs) else { return Ok(None); }; let bytes = write_page(&merged); if read_page(&bytes, &Limits::default()).is_err() { return Ok(None); } - Ok(Some(Entry { - vv: join(&a.vv, &b.vv), + let vv = join(&a.vv, &b.vv); + let snapshot = Entry { + vv: vv.clone(), + content: contents.make(gzip(&bytes)?), + deleted: false, + }; + let page = Entry { + vv, content: contents.make(bytes), deleted: false, - })) + }; + Ok(Some(( + page, + format!("{prefix}.history/{}.json.gz", merged.revision.id), + snapshot, + ))) +} + +/// Gzip of `bytes`, the same on every device. +fn gzip(bytes: &[u8]) -> Result> { + use std::io::Write; + let mut gz = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + gz.write_all(bytes).map_err(SyncError::local)?; + gz.finish().map_err(SyncError::local) } /// Merges manifests. `before` is this device's own manifest before the merge: folders where nothing changed are @@ -441,6 +463,7 @@ pub fn merge_with( contents: &dyn Contents, mergers: &Mergers, ) -> Result { + let mut snapshots: Vec<(String, Entry)> = Vec::new(); let mut by_path: BTreeMap<&str, Vec> = BTreeMap::new(); for manifest in inputs { for (path, entry) in manifest.iter() { @@ -449,13 +472,21 @@ pub fn merge_with( } let mut block_merged = Vec::new(); for (path, candidates) in &by_path { - if let Some(entry) = merge_page_blocks(path, candidates, &by_path, contents, mergers)? { + if let Some((entry, at, snapshot)) = merge_page_blocks(path, candidates, &by_path, contents, mergers)? { block_merged.push((*path, entry)); + if !by_path.contains_key(at.as_str()) { + snapshots.push((at, snapshot)); + } } } for (path, entry) in block_merged { by_path.insert(path, vec![entry]); } + // The merged revision's snapshot is the base of the next merge of its page. Another device that made the + // same merge already published the same snapshot. + for (at, snapshot) in &snapshots { + by_path.insert(at.as_str(), vec![snapshot.clone()]); + } let mut merged = Merged::default(); let mut extras: Vec<(String, String, Entry)> = Vec::new(); let mut concurrent: BTreeSet = BTreeSet::new(); diff --git a/crates/sync/tests/block_merge.rs b/crates/sync/tests/block_merge.rs index 6c539f423..838db46d4 100644 --- a/crates/sync/tests/block_merge.rs +++ b/crates/sync/tests/block_merge.rs @@ -333,3 +333,67 @@ fn with_no_shared_snapshot_the_page_keeps_the_conflict_copy() { let merged = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); assert_eq!(merged.conflicts.len(), 1, "no base to merge from"); } + +/// A later save of `of` with these blocks, as the device that saved it lists its revisions. +fn child(of: &Page, n: u128, saved: i64, blocks: &[Block]) -> Page { + let mut p = page(n, &[], saved, blocks); + p.revision.parents = vec![of.revision.id]; + p.revision.ancestors = std::iter::once(of.revision.id) + .chain(of.revision.ancestors.iter().copied()) + .collect(); + p +} + +/// The next concurrent round after a block merge merges against the merged revision, never an older snapshot: +/// a tag one side removed after the merge stays removed (hand-worked: base {a}, one side removes x from {a, x}). +#[test] +fn the_round_after_a_block_merge_merges_against_the_merged_revision() { + let store = Store::default(); + let base = page(1, &[], 10, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + let mine = page(2, &[1], 20, &[tag_block(1, &["a", "x"]), tag_block(2, &["b"])]); + let theirs = page(3, &[1], 30, &[tag_block(1, &["a"]), tag_block(2, &["b", "y"])]); + let (a, b) = devices(&store, &base, &mine, &theirs); + let first = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + assert!(first.conflicts.is_empty(), "{:?}", first.conflicts); + let r4 = page_of(&store, &first); + let snapshot = format!("{}/.history/{}.json.gz", dir(), r4.revision.id); + assert!( + first.entries.contains_key(&snapshot), + "the merged revision has a snapshot" + ); + + let page_path = format!("{}/page.json", dir()); + let merged_entry = first.entries[&page_path].clone(); + let save = |device: &str, p: &Page| { + let mut e = merged_entry.clone(); + *e.vv.entry(device.to_owned()).or_insert(0) += 1; + e.content = store.put(write_page(p)); + e + }; + let r5 = child(&r4, 5, 50, &[tag_block(1, &["a"]), tag_block(2, &["b", "y"])]); + let r6 = child(&r4, 6, 60, &[tag_block(1, &["a", "x"]), tag_block(2, &["b", "y", "z"])]); + let (mut a2, mut b2) = (first.entries.clone(), first.entries.clone()); + a2.insert(page_path.clone(), save("d0", &r5)); + b2.insert(page_path.clone(), save("d1", &r6)); + let second = merge_with(&[&a2, &b2], &a2, &store, &fake()).expect("merges"); + assert!(second.conflicts.is_empty(), "no conflict copy: {:?}", second.conflicts); + let merged = page_of(&store, &second); + let at = |n: u128| tags(merged.blocks.get(BlockId(id(n))).expect("block")); + assert_eq!(at(1), ["a"].map(String::from).into(), "the removed tag stays removed"); + assert_eq!(at(2), ["b", "y", "z"].map(String::from).into()); +} + +/// When the newest shared revision has no snapshot, an older one is never the base: the page keeps the conflict +/// copy. +#[test] +fn an_older_shared_snapshot_is_never_the_base() { + let store = Store::default(); + let base = page(1, &[], 10, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + let newest = child(&base, 4, 15, &[tag_block(1, &["a", "x"]), tag_block(2, &["b"])]); + let mine = child(&newest, 5, 20, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + let theirs = child(&newest, 6, 30, &[tag_block(1, &["a", "x"]), tag_block(2, &["b", "z"])]); + // Only the older revision R1 has a snapshot. + let (a, b) = devices(&store, &base, &mine, &theirs); + let merged = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + assert_eq!(merged.conflicts.len(), 1, "{:?}", merged.conflicts); +} diff --git a/docs/adr/0089-block-level-merge-on-sync.md b/docs/adr/0089-block-level-merge-on-sync.md index 82cb98298..cdabcc774 100644 --- a/docs/adr/0089-block-level-merge-on-sync.md +++ b/docs/adr/0089-block-level-merge-on-sync.md @@ -12,13 +12,15 @@ When two devices change the same page and neither saw the other's save, sync kee One entry point in `crates/core/src/merge` and one view registry in `app/src/features/qol/conflict`. - **Mergers.** A block type registers a `BlockMerger` with `opennote_core::merge::register(type, merger)` at startup. `merge_block(type, base, mine, theirs)` returns the merged block, or a conflict. Equal sides, or a side equal to the base, need no merger. With no merger for the type, the result is a conflict, which means today's conflict copy. -- **When a page merges.** Sync tries `merge_page` only when two maximal versions of one `page.json` are both live and neither revision lists the other among its ancestors. The base is the newest revision both list as an ancestor whose `.history/.json.gz` snapshot is in the merged manifests. With no such snapshot, nothing merges. `merge_page` returns a page only when all of these hold: +- **When a page merges.** Sync tries `merge_page` only when two maximal versions of one `page.json` are both live and neither revision lists the other among its ancestors. The base is the newest revision both list as an ancestor, and only that one. An older base would turn a change one side undid into no change, and bring the old value back. Its `.history/.json.gz` snapshot must be in the merged manifests; without it, nothing merges. The merge also publishes a snapshot of the merged revision, so the next concurrent round has its base. `merge_page` returns a page only when all of these hold: - At least one changed block has a type with a registered merger. So with no merger registered, every result is byte-identical to today's, and a page whose changes are all in text never merges here. - Every block that changed on both sides, to different results, has a merger, and it succeeds. - No block was deleted on one side and changed on the other. + - Both sides may be written, and both were read at the same version keys (`formatVersion`, `minReaderVersion`, `minWriterVersion`). A read-only page, or a page that keeps a newer version's stamp (`R-VER-5`), keeps its conflict copy. - Every other page field (title, tags, view, ink, recordings, encryption, unknown keys) changed on at most one side. Assets merge per key. `modified` takes the later time. - **Convergence.** Every device must reach the same bytes from the same versions, in any order. Mergers must be commutative. The merged revision is made from the two revisions alone. Its ID's time is one millisecond after the later one, and its random part is the exclusive or of theirs. Both revisions are its parents and ancestors. Its device, writer, and `saved_at` are the later side's. The merged file's version vector is the join of the two, as for the other merged files. -- **Conflict views.** `conflict.text` also returns `blocks`: each block that differs between the two versions, with its ID, type, and both sides as `page.json` writes them. A block type registers a compare and resolve view with `registerConflictView(type, view)`. The conflict dialog shows that view for each changed block of the type, and the side-by-side text for everything else. A view resolves the conflict with the same three choices as the dialog. +- **Conflict views.** `conflict.text` also returns `blocks`: each block that differs between the two versions, with its ID, type, and both sides as `page.json` writes them. A block type registers a compare and resolve view with `registerConflictView(type, view)`. The conflict dialog shows that view for each changed block of the type, and the side-by-side text for everything else. + - When every changed block has a view, the whole-page text stays under a disclosure. So a title or tag difference is still shown. A view resolves the conflict with the same three choices as the dialog. ## Options considered @@ -32,7 +34,7 @@ One entry point in `crates/core/src/merge` and one view registry in `app/src/fea - Tables (ADR 0066) register their merger and view in B14; code projects (ADR 0069) register theirs in B13. - Two devices on app versions with different mergers can disagree for one round: the newer one merges, and the older one keeps a conflict copy. The merged revision lists both versions as ancestors, so every device keeps the merged page, and the older device's conflict copy stays for the person to review. No text is lost. -- A merge needs a shared `.history` snapshot. History thinning (spec 13.3) can remove the newest one, and then an older shared snapshot is the base, or the page keeps its conflict copy. -- Tests in `crates/sync/tests/block_merge.rs`: with no merger, sync results and conflict copies are byte-identical. +- A merge needs a shared `.history` snapshot. History thinning (spec 13.3) can remove the newest one, and then the page keeps its conflict copy. A merged revision's snapshot comes from the merge itself, since no local save writes it. +- Tests in `crates/sync/tests/block_merge.rs`: with no merger, sync results and conflict copies are byte-identical. The round after a block merge merges against the merged revision, and an older shared snapshot is never the base. - Tests in `crates/core/src/merge/mod.rs`: with a fake merger, two devices that changed different blocks merge with no conflict copy. `merge(base, a, b)` equals `merge(base, b, a)` over 2,000 generated cases. - Tests in `app/src/features/qol/conflict/registry.test.tsx`: the conflict dialog shows a registered view, and the text view for the rest. From caef668658e8811ecaac2685f28c9ff3187f95db Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 06:33:25 -0800 Subject: [PATCH 06/49] Pick the block-merge base in save order and keep its snapshot reachable - The merge base is the first revision each side's ancestor list shares with the other, and both sides must agree. A device whose clock runs behind can no longer make an older shared save the base. - Sync uploads a .history snapshot of each page.json revision it sends that has none yet, so pages synced while open keep their base. - The conflict list hides a copy the current page already includes. - Split long merge functions into helpers and move the block merge into crates/sync/src/merge/blocks.rs. Co-Authored-By: Claude Opus 5.5 --- crates/core/src/merge/mod.rs | 54 ++++---- crates/core/src/session/page/tests.rs | 35 +++++ crates/core/src/session/page/versions.rs | 8 +- crates/sync/src/engine/plan.rs | 59 ++++++++- crates/sync/src/merge.rs | 141 +++++--------------- crates/sync/src/merge/blocks.rs | 145 +++++++++++++++++++++ crates/sync/tests/block_merge.rs | 46 +++++++ crates/sync/tests/block_merge_sync.rs | 143 ++++++++++++++++++++ crates/sync/tests/sync.rs | 4 +- docs/adr/0089-block-level-merge-on-sync.md | 13 +- 10 files changed, 506 insertions(+), 142 deletions(-) create mode 100644 crates/sync/src/merge/blocks.rs create mode 100644 crates/sync/tests/block_merge_sync.rs diff --git a/crates/core/src/merge/mod.rs b/crates/core/src/merge/mod.rs index 0a069356a..d3e342690 100644 --- a/crates/core/src/merge/mod.rs +++ b/crates/core/src/merge/mod.rs @@ -212,27 +212,7 @@ pub fn merge_page(mergers: &Mergers, base: &Page, mine: &Page, theirs: &Page) -> if changed { opted_in |= [b, m, t].into_iter().flatten().any(|x| mergers.handles(x.type_name())); } - let result = match (b, m, t) { - (Some(b), Some(m), Some(t)) => match mergers.merge_block(b.type_name(), b, m, t) { - BlockMerge::Merged(block) if *block == **m => Some(m.clone()), - BlockMerge::Merged(block) if *block == **t => Some(t.clone()), - BlockMerge::Merged(block) => Some(Arc::new(*block)), - BlockMerge::Conflict => return None, - }, - // Deleted on one side or both: it stays deleted unless the other side changed it. - (Some(b), m, t) => { - if m.is_some_and(|m| m != b) || t.is_some_and(|t| t != b) { - return None; - } - None - } - // Added on one side, or the same block added on both. - (None, m, t) => match (m, t) { - (Some(m), Some(t)) if m != t => return None, - (m, t) => m.or(t).cloned(), - }, - }; - kept.extend(result); + kept.extend(merge_one(mergers, b, m, t)?); } if !opted_in { return None; @@ -242,6 +222,35 @@ pub fn merge_page(mergers: &Mergers, base: &Page, mine: &Page, theirs: &Page) -> for block in kept { merged.blocks.insert(block).ok()?; } + merge_fields(&mut merged, base, mine, theirs)?; + merged.revision = merged_revision(&mine.revision, &theirs.revision); + Some(merged) +} + +/// One block from its base and two sides: `Some(None)` when it stays deleted, `None` for a conflict. +fn merge_one( + mergers: &Mergers, + b: Option<&Arc>, + m: Option<&Arc>, + t: Option<&Arc>, +) -> Option>> { + match (b, m, t) { + (Some(b), Some(m), Some(t)) => match mergers.merge_block(b.type_name(), b, m, t) { + BlockMerge::Merged(block) if *block == **m => Some(Some(m.clone())), + BlockMerge::Merged(block) if *block == **t => Some(Some(t.clone())), + BlockMerge::Merged(block) => Some(Some(Arc::new(*block))), + BlockMerge::Conflict => None, + }, + // Deleted on one side or both: it stays deleted unless the other side changed it. + (Some(b), m, t) => (!(m.is_some_and(|m| m != b) || t.is_some_and(|t| t != b))).then_some(None), + // Added on one side, or the same block added on both. + (None, Some(m), Some(t)) if m != t => None, + (None, m, t) => Some(m.or(t).cloned()), + } +} + +/// The page fields outside the blocks, each from the side that changed it. `None` when both changed one apart. +fn merge_fields(merged: &mut Page, base: &Page, mine: &Page, theirs: &Page) -> Option<()> { merged.title = pick(&base.title, &mine.title, &theirs.title)?.clone(); merged.created = *pick(&base.created, &mine.created, &theirs.created)?; merged.modified = mine.modified.max(theirs.modified); @@ -252,8 +261,7 @@ pub fn merge_page(mergers: &Mergers, base: &Page, mine: &Page, theirs: &Page) -> merged.recordings = pick(&base.recordings, &mine.recordings, &theirs.recordings)?.clone(); merged.encryption = pick(&base.encryption, &mine.encryption, &theirs.encryption)?.clone(); merged.extra = pick(&base.extra, &mine.extra, &theirs.extra)?.clone(); - merged.revision = merged_revision(&mine.revision, &theirs.revision); - Some(merged) + Some(()) } /// The side whose revision was saved later, then sorts later: the same on every device. diff --git a/crates/core/src/session/page/tests.rs b/crates/core/src/session/page/tests.rs index 24c624f0d..6e77d0e94 100644 --- a/crates/core/src/session/page/tests.rs +++ b/crates/core/src/session/page/tests.rs @@ -223,6 +223,41 @@ fn two_clients_share_a_session_and_hear_about_each_other() { assert_eq!(on_disk(&s).title, "Shared"); } +/// A conflict copy whose revision the page already lists among its ancestors was merged into it on another +/// device (ADR 0089): it is not offered as a conflict, while a copy the page never saw still is. +#[test] +fn a_conflict_copy_the_page_already_includes_is_not_shown() { + let s = setup(); + let c = client("main-1"); + let handle = s.notebook.open_page(s.page, c.clone()).unwrap(); + let dir = s.notebook.inner.tree().store.page_dir(s.page).unwrap(); + let older = on_disk(&s); + handle + .commit_for_tests(&retitle(s.kit.clock.as_ref(), &c, "Photosynthesis", "Mine")) + .unwrap(); + handle.save_now().unwrap(); + assert!(handle.page_for_tests().revision.ancestors.contains(&older.revision.id)); + s.kit.fs.inner.mkdir_all(&dir.join(crate::store::layout::CONFLICTS_DIR)); + let copy = NotebookLayout::conflict_path(&dir, older.revision.id); + s.kit + .fs + .replace_durable(©, &s.kit.codec.write_page(&older)) + .unwrap(); + assert!(handle.conflicts().unwrap().is_empty()); + + let mut unseen = older.clone(); + unseen.revision.id = RevisionId(crate::id::Id::from_parts(1_700_000_000_000, 77)); + unseen.title = "Theirs".into(); + let other = NotebookLayout::conflict_path(&dir, unseen.revision.id); + s.kit + .fs + .replace_durable(&other, &s.kit.codec.write_page(&unseen)) + .unwrap(); + let shown = handle.conflicts().unwrap(); + assert_eq!(shown.len(), 1); + assert_eq!(shown[0].revision, unseen.revision.id); +} + #[test] fn a_change_made_elsewhere_is_kept_as_a_conflict() { let s = setup(); diff --git a/crates/core/src/session/page/versions.rs b/crates/core/src/session/page/versions.rs index 8009ea216..e26de2390 100644 --- a/crates/core/src/session/page/versions.rs +++ b/crates/core/src/session/page/versions.rs @@ -170,7 +170,9 @@ impl PageSession { Ok(()) } - /// The page's open conflicts: other versions kept in `.conflicts/` (spec 14.1). + /// The page's open conflicts: other versions kept in `.conflicts/` (spec 14.1). A copy whose revision the page + /// already lists among its ancestors is already part of the page (an app without block merge kept it while + /// another device merged the two, ADR 0089), so it is not shown. pub(crate) fn conflicts(&self) -> Result, CoreError> { let dir = self.dir().join(CONFLICTS_DIR); let entries = match self.ctx.fs.read_dir(&dir) { @@ -178,11 +180,15 @@ impl PageSession { Err(e) if e.kind == FsErrorKind::NotFound => return Ok(Vec::new()), Err(e) => return Err(e.into()), }; + let included = self.state().page.revision.ancestors.clone(); let mut found = Vec::new(); for entry in entries.iter().filter(|e| !e.is_dir) { let Some(rev) = entry.name.strip_suffix(".json").and_then(|r| RevisionId::parse(r).ok()) else { continue; }; + if included.contains(&rev) { + continue; + } let bytes = self .ctx .fs diff --git a/crates/sync/src/engine/plan.rs b/crates/sync/src/engine/plan.rs index d4c9c4d42..2f236d497 100644 --- a/crates/sync/src/engine/plan.rs +++ b/crates/sync/src/engine/plan.rs @@ -4,11 +4,15 @@ use std::cell::RefCell; use std::collections::{BTreeMap, BTreeSet, HashMap}; +use opennote_core::format::gzip::gzip; +use opennote_core::format::page_json::read_page; +use opennote_core::Limits; + use super::{digest, Action, Engine, LocalState, Plan, Store, Summary}; use crate::error::{Result, SyncError}; use crate::local::{self, Fingerprint}; use crate::manifest::{self, Entry, Manifest, Vv}; -use crate::merge::{self, Merged}; +use crate::merge::{self, Contents, Merged}; use crate::vault; /// The new heads to merge, and what this device will have seen after merging them. @@ -47,7 +51,8 @@ impl Engine { local_paths, pending: RefCell::new(HashMap::new()), }; - let mut inputs: Vec<&Manifest> = vec![&local_manifest, &state.deferred]; + let snapshots = self.page_snapshots(&state, &incoming.heads, &local_manifest, &floor, &store)?; + let mut inputs: Vec<&Manifest> = vec![&local_manifest, &state.deferred, &snapshots]; inputs.extend(incoming.heads.iter()); let merged = merge::merge(&inputs, &local_manifest, &store)?; let pending = store.pending.into_inner(); @@ -154,6 +159,56 @@ impl Engine { Ok((manifest, prints)) } + /// A `.history` snapshot of each `page.json` revision this device is about to upload that has none yet, so the + /// other devices hold the base a later block merge of the page needs (ADR 0089). The app keeps a version only + /// now and then, while every autosave is a new revision, so without this the newest shared revision of a page + /// that synced while open would rarely have one. The snapshot is the gzip of the exact bytes, as the app writes + /// versions, and lands in this folder like any merged file. + fn page_snapshots( + &self, + state: &LocalState, + heads: &[Manifest], + local_manifest: &Manifest, + floor: &HashMap, + store: &Store, + ) -> Result { + let mut out = Manifest::new(); + let held = |path: &str| { + std::iter::once(local_manifest) + .chain(std::iter::once(&state.deferred)) + .chain(heads) + .any(|m| m.get(path).is_some_and(Entry::live)) + }; + for (path, entry) in local_manifest { + let known = state + .files + .get(path) + .is_some_and(|k| k.entry.content.id == entry.content.id); + if known || !entry.live() || merge::kind(path) != merge::Kind::Page { + continue; + } + let Ok(bytes) = std::fs::read(local::join(&self.root, path)) else { + continue; + }; + let Ok(read) = read_page(&bytes, &Limits::default()) else { + continue; + }; + let dir = path.strip_suffix("page.json").unwrap_or_default(); + let at = format!("{dir}.history/{}.json.gz", read.page.revision.id); + // The file changed since the scan: the next round snapshots what is there then. + if self.hash(&bytes).id != entry.content.id || held(&at) { + continue; + } + let entry = Entry { + vv: self.fresh(&Vv::new(), &at, floor), + content: store.make(gzip(&bytes)), + deleted: false, + }; + out.insert(at, entry); + } + Ok(out) + } + /// A head's entries without the tombstones this device already dropped (see `prune_tombstones`). fn unpruned(&self, state: &LocalState, mut entries: Manifest) -> Manifest { entries.retain(|path, entry| { diff --git a/crates/sync/src/merge.rs b/crates/sync/src/merge.rs index ddb4f0d34..6a85582e1 100644 --- a/crates/sync/src/merge.rs +++ b/crates/sync/src/merge.rs @@ -11,16 +11,19 @@ use std::collections::{BTreeMap, BTreeSet}; use std::io::Read; use opennote_core::format::history_json::{merge_versions, read_versions, write_versions}; -use opennote_core::format::page_json::{read_page, write_page}; +use opennote_core::format::page_json::read_page; use opennote_core::format::tree_json::{merge_notebooks, merge_sections, read_notebook, read_section}; use opennote_core::format::tree_json::{write_notebook, write_section}; -use opennote_core::merge::{merge_page, mergers, Mergers}; +use opennote_core::merge::{mergers, Mergers}; use opennote_core::model::Page; use opennote_core::Limits; use crate::error::{Result, SyncError}; use crate::manifest::{dominates, join, Content, Entry, Manifest, Vv}; +mod blocks; +use blocks::merge_blocks_first; + /// Reads small files for the merge rules, and turns bytes a rule wrote into content. pub trait Contents { /// The bytes of a content, from this device or from storage. @@ -368,88 +371,6 @@ fn page_refs(bytes: &[u8]) -> BTreeSet { refs } -/// Two devices changed one page, and neither saw the other's save: the one page that block mergers make of the -/// two versions and the newest revision both started from (ADR 0089), or `None` to keep today's conflict copy. -/// The base is only that newest shared revision: an older one would turn a change one side undid into no change -/// and bring the old value back. Without its `.history` snapshot the result is `None`. With no merger registered -/// it is always `None`, so nothing changes. -/// -/// The merged page comes with a `.history` snapshot of itself (path and entry), so the next merge of this page -/// has its base. -fn merge_page_blocks( - path: &str, - candidates: &[Entry], - by_path: &BTreeMap<&str, Vec>, - contents: &dyn Contents, - mergers: &Mergers, -) -> Result> { - if mergers.is_empty() || kind(path) != Kind::Page { - return Ok(None); - } - let top = maximal(candidates.to_vec()); - let [a, b] = &top[..] else { return Ok(None) }; - if a.deleted || b.deleted || a.content == b.content { - return Ok(None); - } - let (Some(mine), Some(theirs)) = ( - read_any_page(&contents.bytes(&a.content)?), - read_any_page(&contents.bytes(&b.content)?), - ) else { - return Ok(None); - }; - let (ra, rb) = (&mine.revision, &theirs.revision); - if ra.ancestors.contains(&rb.id) || rb.ancestors.contains(&ra.id) { - return Ok(None); - } - let Some(newest) = ra.ancestors.iter().filter(|r| rb.ancestors.contains(r)).max() else { - return Ok(None); - }; - let (dir, _) = split(path); - let prefix = if dir.is_empty() { - String::new() - } else { - format!("{dir}/") - }; - let snapshot = format!("{prefix}.history/{newest}.json.gz"); - let Some(entry) = by_path.get(snapshot.as_str()).and_then(|e| e.iter().find(|e| e.live())) else { - return Ok(None); - }; - let Some(base) = read_any_page(&contents.bytes(&entry.content)?) else { - return Ok(None); - }; - let Some(merged) = merge_page(mergers, &base, &mine, &theirs) else { - return Ok(None); - }; - let bytes = write_page(&merged); - if read_page(&bytes, &Limits::default()).is_err() { - return Ok(None); - } - let vv = join(&a.vv, &b.vv); - let snapshot = Entry { - vv: vv.clone(), - content: contents.make(gzip(&bytes)?), - deleted: false, - }; - let page = Entry { - vv, - content: contents.make(bytes), - deleted: false, - }; - Ok(Some(( - page, - format!("{prefix}.history/{}.json.gz", merged.revision.id), - snapshot, - ))) -} - -/// Gzip of `bytes`, the same on every device. -fn gzip(bytes: &[u8]) -> Result> { - use std::io::Write; - let mut gz = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); - gz.write_all(bytes).map_err(SyncError::local)?; - gz.finish().map_err(SyncError::local) -} - /// Merges manifests. `before` is this device's own manifest before the merge: folders where nothing changed are /// not checked again. Block mergers registered for this process take part (ADR 0089). pub fn merge(inputs: &[&Manifest], before: &Manifest, contents: &dyn Contents) -> Result { @@ -463,25 +384,13 @@ pub fn merge_with( contents: &dyn Contents, mergers: &Mergers, ) -> Result { - let mut snapshots: Vec<(String, Entry)> = Vec::new(); let mut by_path: BTreeMap<&str, Vec> = BTreeMap::new(); for manifest in inputs { for (path, entry) in manifest.iter() { by_path.entry(path.as_str()).or_default().push(entry.clone()); } } - let mut block_merged = Vec::new(); - for (path, candidates) in &by_path { - if let Some((entry, at, snapshot)) = merge_page_blocks(path, candidates, &by_path, contents, mergers)? { - block_merged.push((*path, entry)); - if !by_path.contains_key(at.as_str()) { - snapshots.push((at, snapshot)); - } - } - } - for (path, entry) in block_merged { - by_path.insert(path, vec![entry]); - } + let snapshots = merge_blocks_first(&mut by_path, contents, mergers)?; // The merged revision's snapshot is the base of the next merge of its page. Another device that made the // same merge already published the same snapshot. for (at, snapshot) in &snapshots { @@ -506,8 +415,29 @@ pub fn merge_with( merged.entries.insert(path.to_owned(), resolved.entry); extras.extend(resolved.others.into_iter().map(|(at, e)| (path.to_owned(), at, e))); } - // Other versions land at new paths. A path may already hold one (another device kept it there too), so they - // merge like any other version. Each round moves names further from the original, and three rounds is plenty. + place_others(&mut merged, extras, &mut sealed_sets, contents)?; + part_case_collisions(&mut merged, contents)?; + merged + .conflicts + .sort_by(|a, b| (&a.path, &a.kept_as).cmp(&(&b.path, &b.kept_as))); + merged.conflicts.dedup(); + let sealed = Sealed { + concurrent: &concurrent, + had_live: &had_live, + }; + revive_referenced(&mut merged.entries, before, contents, &sealed)?; + copy_sealed_sets(&mut merged.entries, &sealed_sets, contents)?; + Ok(merged) +} + +/// Other versions land at new paths. A path may already hold one (another device kept it there too), so they +/// merge like any other version. Each round moves names further from the original, and three rounds is plenty. +fn place_others( + merged: &mut Merged, + mut extras: Vec<(String, String, Entry)>, + sealed_sets: &mut Vec<(String, String)>, + contents: &dyn Contents, +) -> Result<()> { for _ in 0..4 { if extras.is_empty() { break; @@ -529,18 +459,7 @@ pub fn merge_with( } extras = next; } - part_case_collisions(&mut merged, contents)?; - merged - .conflicts - .sort_by(|a, b| (&a.path, &a.kept_as).cmp(&(&b.path, &b.kept_as))); - merged.conflicts.dedup(); - let sealed = Sealed { - concurrent: &concurrent, - had_live: &had_live, - }; - revive_referenced(&mut merged.entries, before, contents, &sealed)?; - copy_sealed_sets(&mut merged.entries, &sealed_sets, contents)?; - Ok(merged) + Ok(()) } /// The version-vector entry that counts moves made because two paths differ only in case. Every device makes the diff --git a/crates/sync/src/merge/blocks.rs b/crates/sync/src/merge/blocks.rs new file mode 100644 index 000000000..51e66521c --- /dev/null +++ b/crates/sync/src/merge/blocks.rs @@ -0,0 +1,145 @@ +//! Block-level merge of a page that two devices changed, before the usual rules run (ADR 0089). + +use std::collections::BTreeMap; + +use opennote_core::format::page_json::{read_page, write_page}; +use opennote_core::id::RevisionId; +use opennote_core::merge::{merge_page, Mergers}; +use opennote_core::model::{Page, Revision}; +use opennote_core::Limits; + +use super::{kind, maximal, read_any_page, split, Contents, Kind}; +use crate::error::{Result, SyncError}; +use crate::manifest::{join, Entry}; + +/// The newest revision both `a` and `b` started from, in the order the saves were made, not by the time in its ID. +/// +/// IDs carry each device's own clock, so a later save made on a device whose clock runs behind can have a smaller +/// ID than the save it came from: the shared revision with the highest ID is then not the newest one. Each save +/// lists its parent first and then the parent's own list, so the first shared entry in either list is the newest +/// shared revision as that side saw it. `None` when the two sides disagree, so the page keeps the conflict copy. +fn newest_shared(a: &Revision, b: &Revision) -> Option { + let first = |x: &Revision, y: &Revision| x.ancestors.iter().find(|r| y.ancestors.contains(r)).copied(); + let from_a = first(a, b)?; + (first(b, a)? == from_a).then_some(from_a) +} + +/// The page saved as `.history/.json.gz` beside `path`, if both devices could see it. +fn snapshot_page( + prefix: &str, + revision: RevisionId, + by_path: &BTreeMap<&str, Vec>, + contents: &dyn Contents, +) -> Result> { + let snapshot = format!("{prefix}.history/{revision}.json.gz"); + let Some(entry) = by_path.get(snapshot.as_str()).and_then(|e| e.iter().find(|e| e.live())) else { + return Ok(None); + }; + Ok(read_any_page(&contents.bytes(&entry.content)?)) +} + +/// The folder part of a page path with its slash, or nothing for a page at the top. +fn dir_prefix(path: &str) -> String { + let (dir, _) = split(path); + if dir.is_empty() { + String::new() + } else { + format!("{dir}/") + } +} + +/// Two devices changed one page, and neither saw the other's save: the one page that block mergers make of the +/// two versions and the newest revision both started from (ADR 0089), or `None` to keep today's conflict copy. +/// The base is only that newest shared revision: an older one would turn a change one side undid into no change +/// and bring the old value back. Without its `.history` snapshot the result is `None`. With no merger registered +/// it is always `None`, so nothing changes. +/// +/// The merged page comes with a `.history` snapshot of itself (path and entry), so the next merge of this page +/// has its base. +fn merge_page_blocks( + path: &str, + candidates: &[Entry], + by_path: &BTreeMap<&str, Vec>, + contents: &dyn Contents, + mergers: &Mergers, +) -> Result> { + if mergers.is_empty() || kind(path) != Kind::Page { + return Ok(None); + } + let top = maximal(candidates.to_vec()); + let [a, b] = &top[..] else { return Ok(None) }; + if a.deleted || b.deleted || a.content == b.content { + return Ok(None); + } + let (Some(mine), Some(theirs)) = ( + read_any_page(&contents.bytes(&a.content)?), + read_any_page(&contents.bytes(&b.content)?), + ) else { + return Ok(None); + }; + let (ra, rb) = (&mine.revision, &theirs.revision); + if ra.ancestors.contains(&rb.id) || rb.ancestors.contains(&ra.id) { + return Ok(None); + } + let Some(newest) = newest_shared(ra, rb) else { + return Ok(None); + }; + let prefix = dir_prefix(path); + let Some(base) = snapshot_page(&prefix, newest, by_path, contents)? else { + return Ok(None); + }; + let Some(merged) = merge_page(mergers, &base, &mine, &theirs) else { + return Ok(None); + }; + let bytes = write_page(&merged); + if read_page(&bytes, &Limits::default()).is_err() { + return Ok(None); + } + let vv = join(&a.vv, &b.vv); + let snapshot = Entry { + vv: vv.clone(), + content: contents.make(gzip(&bytes)?), + deleted: false, + }; + let page = Entry { + vv, + content: contents.make(bytes), + deleted: false, + }; + Ok(Some(( + page, + format!("{prefix}.history/{}.json.gz", merged.revision.id), + snapshot, + ))) +} + +/// Gzip of `bytes`, the same on every device. +fn gzip(bytes: &[u8]) -> Result> { + use std::io::Write; + let mut gz = flate2::write::GzEncoder::new(Vec::new(), flate2::Compression::default()); + gz.write_all(bytes).map_err(SyncError::local)?; + gz.finish().map_err(SyncError::local) +} + +/// Replaces each page that block mergers merge with the one merged version (ADR 0089). Returns the merged +/// revisions' `.history` snapshots that no input has yet. +pub(super) fn merge_blocks_first( + by_path: &mut BTreeMap<&str, Vec>, + contents: &dyn Contents, + mergers: &Mergers, +) -> Result> { + let mut snapshots = Vec::new(); + let mut block_merged = Vec::new(); + for (path, candidates) in by_path.iter() { + if let Some((entry, at, snapshot)) = merge_page_blocks(path, candidates, by_path, contents, mergers)? { + block_merged.push((*path, entry)); + if !by_path.contains_key(at.as_str()) { + snapshots.push((at, snapshot)); + } + } + } + for (path, entry) in block_merged { + by_path.insert(path, vec![entry]); + } + Ok(snapshots) +} diff --git a/crates/sync/tests/block_merge.rs b/crates/sync/tests/block_merge.rs index 838db46d4..aa59b941e 100644 --- a/crates/sync/tests/block_merge.rs +++ b/crates/sync/tests/block_merge.rs @@ -397,3 +397,49 @@ fn an_older_shared_snapshot_is_never_the_base() { let merged = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); assert_eq!(merged.conflicts.len(), 1, "{:?}", merged.conflicts); } + +/// A device whose clock runs behind saves a later revision with a smaller ID. The base is still the newest shared +/// save, not the shared one with the highest ID. Hand-worked: Y adds x to {a}, and one side removes it again. So x +/// stays removed. With the older base {a}, it would come back. +#[test] +fn the_base_is_the_newest_shared_save_even_when_its_id_is_older() { + let store = Store::default(); + let x = page(9, &[], 10, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + // Y comes after X, but its ID sorts before X's: its device's clock was behind. + let y = child(&x, 4, 15, &[tag_block(1, &["a", "x"]), tag_block(2, &["b"])]); + assert!(y.revision.id < x.revision.id); + let mine = child(&y, 5, 20, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + let theirs = child(&y, 6, 30, &[tag_block(1, &["a", "x"]), tag_block(2, &["b", "z"])]); + let (mut a, mut b) = devices(&store, &x, &mine, &theirs); + let snapshot = entry("d0", 1, store.put(gzip(&write_page(&y)))); + let at = format!("{}/.history/{}.json.gz", dir(), y.revision.id); + a.insert(at.clone(), snapshot.clone()); + b.insert(at, snapshot); + let ab = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + let ba = merge_with(&[&b, &a], &b, &store, &fake()).expect("merges"); + assert!(ab.conflicts.is_empty(), "no conflict copy: {:?}", ab.conflicts); + assert_eq!(ab.entries, ba.entries); + let merged = page_of(&store, &ab); + let tags_of = |n: u128| tags(merged.blocks.get(BlockId(id(n))).expect("block")); + assert_eq!( + tags_of(1), + ["a"].map(String::from).into(), + "the removed tag stays removed" + ); + assert_eq!(tags_of(2), ["b", "z"].map(String::from).into()); +} + +/// Two sides whose ancestor lists name different newest shared revisions (a damaged or hostile file) keep the +/// conflict copy. +#[test] +fn sides_that_disagree_on_the_newest_shared_save_keep_the_conflict_copy() { + let store = Store::default(); + let x = page(1, &[], 10, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + let y = child(&x, 4, 15, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + let mine = child(&y, 5, 20, &[tag_block(1, &["a", "m"]), tag_block(2, &["b"])]); + let mut theirs = child(&y, 6, 30, &[tag_block(1, &["a"]), tag_block(2, &["b", "t"])]); + theirs.revision.ancestors = vec![x.revision.id, y.revision.id]; + let (a, b) = devices(&store, &x, &mine, &theirs); + let merged = merge_with(&[&a, &b], &a, &store, &fake()).expect("merges"); + assert_eq!(merged.conflicts.len(), 1, "{:?}", merged.conflicts); +} diff --git a/crates/sync/tests/block_merge_sync.rs b/crates/sync/tests/block_merge_sync.rs new file mode 100644 index 000000000..9f05f2910 --- /dev/null +++ b/crates/sync/tests/block_merge_sync.rs @@ -0,0 +1,143 @@ +//! Block-level merge through the sync engine (ADR 0089). A page can sync while open, at an autosave the app kept +//! no version of. It still has the base a block merge needs. This file runs in its own process, because the +//! merger here is registered for every merge in it. + +mod common; + +use std::collections::BTreeSet; +use std::io::Read; +use std::sync::Arc; + +use common::{devices, page_dir, settle}; +use opennote_core::format::page_json::{read_page, write_page}; +use opennote_core::id::{BlockId, DeviceId, Id, PageId, RevisionId}; +use opennote_core::merge::{register, BlockMerger}; +use opennote_core::model::block::{BlockData, Fallback, OtherData}; +use opennote_core::model::{Block, DeviceRef, JsonMap, Page, Revision}; +use opennote_core::{Limits, OrderKey, Timestamp}; +use opennote_sync::MemoryBackend; +use serde_json::{json, Value}; + +const TEST_TYPE: &str = "test.sync-tags"; + +fn id(n: u128) -> Id { + Id::from_parts(1_760_000_000_000, n) +} + +fn tags(block: &Block) -> BTreeSet { + match &block.data { + BlockData::Other(o) => o.data["tags"] + .as_array() + .map(|a| a.iter().filter_map(|v| v.as_str().map(str::to_owned)).collect()) + .unwrap_or_default(), + _ => BTreeSet::new(), + } +} + +/// Tags added on either side are added, and tags removed on either side go. +struct Tags; + +impl BlockMerger for Tags { + fn merge(&self, base: &Block, mine: &Block, theirs: &Block) -> Option { + let (b, m, t) = (tags(base), tags(mine), tags(theirs)); + let keep: Vec = m + .union(&t) + .filter(|tag| !(b.contains(*tag) && (!m.contains(*tag) || !t.contains(*tag)))) + .cloned() + .collect(); + let mut out = base.clone(); + if let BlockData::Other(o) = &mut out.data { + o.data.insert("tags".into(), Value::from(keep)); + } + Some(out) + } +} + +fn tag_block(n: u128, list: &[&str]) -> Block { + let mut order = OrderKey::between(None, None).expect("order"); + for _ in 0..n { + order = OrderKey::between(Some(&order), None).expect("order"); + } + let mut data = JsonMap::new(); + data.insert("tags".into(), json!(list)); + Block { + id: BlockId(id(n)), + order, + frame: None, + lock: None, + created: Timestamp::from_unix_ms(1_760_000_000_000), + modified: Timestamp::from_unix_ms(1_760_000_000_000), + fallback: Some(Fallback { + markdown: "Tags".into(), + image: None, + extra: JsonMap::new(), + }), + data: BlockData::Other(OtherData { + type_name: TEST_TYPE.into(), + data, + unreadable: None, + }), + extra: JsonMap::new(), + } +} + +/// A page saved as revision `n`, made from the revisions in `ancestors` (newest first). +fn page(n: u128, ancestors: &[u128], saved: i64, blocks: &[Block]) -> Page { + let rev = |n: u128| RevisionId(id(1000 + n)); + let mut r = Revision::new( + rev(n), + Timestamp::from_unix_ms(1_760_000_000_000 + saved), + DeviceRef { + id: DeviceId(id(9)), + label: "Windows device TEST".into(), + }, + "OpenNote test", + ); + r.ancestors = ancestors.iter().map(|a| rev(*a)).collect(); + r.parents = r.ancestors.iter().take(1).copied().collect(); + let mut p = Page::new(PageId(id(1)), Timestamp::from_unix_ms(1_760_000_000_000), r); + for b in blocks { + p.blocks.insert(Arc::new(b.clone())).expect("unique"); + } + p +} + +fn gunzip(bytes: &[u8]) -> Vec { + let mut out = Vec::new(); + flate2::read::GzDecoder::new(bytes).read_to_end(&mut out).expect("gzip"); + out +} + +#[test] +fn a_page_synced_between_versions_still_merges_by_block() { + register(TEST_TYPE, Arc::new(Tags)); + let d = devices(Arc::new(MemoryBackend::new()), 2); + let p = page_dir(1); + let page_path = format!("{p}/page.json"); + // An autosave: the app wrote no `.history` version of it. + let base = page(1, &[], 10, &[tag_block(1, &["a"]), tag_block(2, &["b"])]); + d[0].write(&page_path, &write_page(&base)); + settle(&d, 3); + let snapshot = format!("{p}/.history/{}.json.gz", base.revision.id); + for device in &d { + let kept = device.read(&snapshot).expect("the uploaded revision has a snapshot"); + assert_eq!(gunzip(&kept), write_page(&base), "the snapshot holds the exact bytes"); + } + + let mine = page(2, &[1], 20, &[tag_block(1, &["a", "m"]), tag_block(2, &["b"])]); + let theirs = page(3, &[1], 30, &[tag_block(1, &["a"]), tag_block(2, &["b", "t"])]); + d[0].write(&page_path, &write_page(&mine)); + d[1].write(&page_path, &write_page(&theirs)); + settle(&d, 4); + assert_eq!(d[0].files(), d[1].files(), "both devices reach the same files"); + assert!( + !d[0].files().keys().any(|k| k.contains(".conflicts")), + "merged by block, no conflict copy" + ); + let merged = read_page(&d[0].read(&page_path).expect("page"), &Limits::default()) + .expect("reads") + .page; + let at = |n: u128| tags(merged.blocks.get(BlockId(id(n))).expect("block")); + assert_eq!(at(1), ["a", "m"].map(String::from).into()); + assert_eq!(at(2), ["b", "t"].map(String::from).into()); +} diff --git a/crates/sync/tests/sync.rs b/crates/sync/tests/sync.rs index 23fc30453..a544c44b5 100644 --- a/crates/sync/tests/sync.rs +++ b/crates/sync/tests/sync.rs @@ -26,7 +26,9 @@ fn files_reach_the_other_device_and_deletes_follow() { assert_eq!(report.summary.upload.len(), 3); assert_eq!(report.generation, Some(1)); let report = d[1].engine.sync().expect("syncs"); - assert_eq!(report.summary.download.len(), 3); + // The three files, and the `.history` snapshot of the page revision the first device uploaded (ADR 0089). + assert_eq!(report.summary.download.len(), 4); + assert!(d[0].read(&format!("{p}/.history/{}.json.gz", rev(1))).is_some()); assert_eq!(d[1].files(), d[0].files()); // Nothing changed: no head is written and nothing moves. diff --git a/docs/adr/0089-block-level-merge-on-sync.md b/docs/adr/0089-block-level-merge-on-sync.md index cdabcc774..58fa661d3 100644 --- a/docs/adr/0089-block-level-merge-on-sync.md +++ b/docs/adr/0089-block-level-merge-on-sync.md @@ -12,7 +12,10 @@ When two devices change the same page and neither saw the other's save, sync kee One entry point in `crates/core/src/merge` and one view registry in `app/src/features/qol/conflict`. - **Mergers.** A block type registers a `BlockMerger` with `opennote_core::merge::register(type, merger)` at startup. `merge_block(type, base, mine, theirs)` returns the merged block, or a conflict. Equal sides, or a side equal to the base, need no merger. With no merger for the type, the result is a conflict, which means today's conflict copy. -- **When a page merges.** Sync tries `merge_page` only when two maximal versions of one `page.json` are both live and neither revision lists the other among its ancestors. The base is the newest revision both list as an ancestor, and only that one. An older base would turn a change one side undid into no change, and bring the old value back. Its `.history/.json.gz` snapshot must be in the merged manifests; without it, nothing merges. The merge also publishes a snapshot of the merged revision, so the next concurrent round has its base. `merge_page` returns a page only when all of these hold: +- **When a page merges.** Sync tries `merge_page` only when two maximal versions of one `page.json` are both live and neither revision lists the other among its ancestors. The base is the newest revision both list as an ancestor, and only that one. An older base would turn a change one side undid into no change, and bring the old value back. +- **Which revision is newest.** Newest means in the order the saves were made, not by the time in the revision ID. IDs carry each device's own clock, which can run behind, so a later save can have a smaller ID. Each save lists its parent first, then the parent's own list. So the base is the first entry of each side's list that the other side also lists. When the two sides name different revisions, nothing merges. +- **The base snapshot.** The base's `.history/.json.gz` snapshot must be in the merged manifests; without it, nothing merges. The merge also publishes a snapshot of the merged revision, so the next concurrent round has its base. +- **What must hold.** `merge_page` returns a page only when all of these hold: - At least one changed block has a type with a registered merger. So with no merger registered, every result is byte-identical to today's, and a page whose changes are all in text never merges here. - Every block that changed on both sides, to different results, has a merger, and it succeeds. - No block was deleted on one side and changed on the other. @@ -33,8 +36,10 @@ One entry point in `crates/core/src/merge` and one view registry in `app/src/fea ## Consequences - Tables (ADR 0066) register their merger and view in B14; code projects (ADR 0069) register theirs in B13. -- Two devices on app versions with different mergers can disagree for one round: the newer one merges, and the older one keeps a conflict copy. The merged revision lists both versions as ancestors, so every device keeps the merged page, and the older device's conflict copy stays for the person to review. No text is lost. -- A merge needs a shared `.history` snapshot. History thinning (spec 13.3) can remove the newest one, and then the page keeps its conflict copy. A merged revision's snapshot comes from the merge itself, since no local save writes it. -- Tests in `crates/sync/tests/block_merge.rs`: with no merger, sync results and conflict copies are byte-identical. The round after a block merge merges against the merged revision, and an older shared snapshot is never the base. +- Two devices on app versions with different mergers can disagree for one round: the newer one merges, and the older one keeps a conflict copy. The merged revision lists both versions as ancestors, so every device keeps the merged page, and the older device's conflict copy stays on disk, but the app no longer lists it (see below). No text is lost. +- A merge needs a shared `.history` snapshot. The app keeps a version only at close, at exit, and every 10 minutes of editing, while every autosave is a new revision. So when sync uploads a `page.json` revision that has no snapshot yet, it also uploads one: the gzip of the exact bytes, as the app writes versions. The other devices then hold the base for a page that synced while it was open. The version list picks these snapshots up like any other. History thinning (spec 13.3) can still remove the newest one, and then the page keeps its conflict copy. A merged revision's snapshot comes from the merge itself, since no local save writes it. +- A conflict copy whose revision the current page lists among its ancestors is already part of the page. An older device without the merger can keep such a copy while another device merged the pair. The app does not list it as a conflict, so "Keep the other version" can't replace the merged page with the older side. +- Tests in `crates/sync/tests/block_merge.rs`: with no merger, sync results and conflict copies are byte-identical. The round after a block merge merges against the merged revision. An older shared snapshot is never the base. The base is the newest shared save, even when its ID sorts before an older one. +- Tests in `crates/sync/tests/block_merge_sync.rs`: a page synced at an autosave still merges by block on the next concurrent round. - Tests in `crates/core/src/merge/mod.rs`: with a fake merger, two devices that changed different blocks merge with no conflict copy. `merge(base, a, b)` equals `merge(base, b, a)` over 2,000 generated cases. - Tests in `app/src/features/qol/conflict/registry.test.tsx`: the conflict dialog shows a registered view, and the text view for the rest. From ea8a5ac953a1d6e4cc85fed7cd009941915a4337 Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 13:34:12 -0800 Subject: [PATCH 07/49] Add comments, suggesting mode, and accept or reject Comments and suggestions are marks in a text block's Markdown with their records in data.comments and data.suggestions (ADR 0086, spec 6.10). A review model rides on the text sync, so a mark and its record change in one undo step, and records of marks that left are dropped. Suggesting mode turns typing into insertion marks, deletions into deletion marks, and formatting commands into format changes that keep the old Markdown. Accept and reject settle one change, all changes, or one author's changes. The marks get styles for the All, Simple, and None markup views, and a screen reader hears "Insertion by Sam". Co-Authored-By: Claude Opus 5.5 --- app/src/features/review/changes.ts | 96 +++++++++ app/src/features/review/comments.test.ts | 122 ++++++++++++ app/src/features/review/comments.ts | 65 ++++++ app/src/features/review/extension.ts | 93 +++++++++ app/src/features/review/model.ts | 227 +++++++++++++++++++++ app/src/features/review/ranges.ts | 78 ++++++++ app/src/features/review/records.ts | 136 +++++++++++++ app/src/features/review/review.module.css | 159 +++++++++++++++ app/src/features/review/state.ts | 62 ++++++ app/src/features/review/suggest.test.ts | 194 ++++++++++++++++++ app/src/features/review/suggest.ts | 229 ++++++++++++++++++++++ app/src/strings/en/textReview.ts | 129 +++++++++++- 12 files changed, 1589 insertions(+), 1 deletion(-) create mode 100644 app/src/features/review/changes.ts create mode 100644 app/src/features/review/comments.test.ts create mode 100644 app/src/features/review/comments.ts create mode 100644 app/src/features/review/extension.ts create mode 100644 app/src/features/review/model.ts create mode 100644 app/src/features/review/ranges.ts create mode 100644 app/src/features/review/records.ts create mode 100644 app/src/features/review/review.module.css create mode 100644 app/src/features/review/state.ts create mode 100644 app/src/features/review/suggest.test.ts create mode 100644 app/src/features/review/suggest.ts diff --git a/app/src/features/review/changes.ts b/app/src/features/review/changes.ts new file mode 100644 index 000000000..b0ef4de33 --- /dev/null +++ b/app/src/features/review/changes.ts @@ -0,0 +1,96 @@ +// Accept and reject suggestions (ADR 0086). Accepting an insertion keeps its text and drops the mark; rejecting it +// removes the text. Accepting a deletion removes the text; rejecting it drops the mark. Accepting a format change +// keeps the new look; rejecting it puts back the text's Markdown from before the change. Each call is one +// transaction, so one undo step. +import type { Node as PMNode } from '@tiptap/pm/model'; +import { TextSelection } from '@tiptap/pm/state'; +import type { EditorState, Transaction } from '@tiptap/pm/state'; +import { META_COMMAND } from '../../editor/meta'; +import { parseTextBlock } from '../../editor/markdown'; +import { SUGGESTION_MARKS } from './records'; +import { changeRanges, touches } from './ranges'; +import type { Change } from './ranges'; +import { REVIEW_META } from './suggest'; + +export type Decision = 'accept' | 'reject'; + +/** Which suggestions a decision is about. */ +export interface Which { + /** Only these IDs; all when left out. */ + ids?: ReadonlySet; + /** Only suggestions that touch this span. */ + near?: { from: number; to: number }; + /** Only suggestions whose record names this author. */ + author?: string; + /** The author of a suggestion, from its record; null when it has none. */ + authorOf?: (id: string) => string | null; +} + +/** The suggestions a decision is about, in document order. */ +export function pick(doc: PMNode, which: Which = {}): Change[] { + return changeRanges(doc).filter((change) => { + if (which.ids && !which.ids.has(change.id)) return false; + if (which.near && !touches(change, which.near.from, which.near.to)) return false; + if (which.author !== undefined && (which.authorOf?.(change.id) ?? null) !== which.author) return false; + return true; + }); +} + +/** The inline content a format change's `before` holds, when its text is the text the change covers now. */ +function beforeContent(doc: PMNode, change: Change, before: string | undefined) { + if (before === undefined || change.spans.length !== 1) return null; + const paragraph = parseTextBlock(before).firstChild; + if (!paragraph?.isTextblock) return null; + const now = doc.textBetween(change.from, change.to, '', ''); + return paragraph.textBetween(0, paragraph.content.size, '', '') === now ? paragraph.content : null; +} + +/** + * The transaction that accepts or rejects `changes`, or null when there are none. `beforeOf` gives a format + * change's Markdown from before it. + */ +export function decide( + state: EditorState, + changes: readonly Change[], + decision: Decision, + beforeOf: (id: string) => string | undefined = () => undefined, +): Transaction | null { + if (changes.length === 0) return null; + const tr = state.tr; + const schema = state.schema; + // Last first, so a removal never moves a change still to come; the mapping covers overlaps. + for (const change of [...changes].sort((a, b) => b.from - a.from)) { + const type = schema.marks[SUGGESTION_MARKS[change.kind].mark]; + const mark = type.create({ [SUGGESTION_MARKS[change.kind].key]: change.id }); + const remove = (change.kind === 'ins') === (decision === 'reject') && change.kind !== 'fmt'; + const restore = + change.kind === 'fmt' && decision === 'reject' ? beforeContent(state.doc, change, beforeOf(change.id)) : null; + for (const span of [...change.spans].reverse()) { + const from = tr.mapping.map(span.from, 1); + const to = tr.mapping.map(span.to, -1); + if (from >= to) continue; + if (remove) tr.delete(from, to); + else if (restore) tr.replaceWith(from, to, restore); + else tr.removeMark(from, to, mark); + } + } + if (!tr.docChanged) return null; + return tr.setMeta(REVIEW_META, true).setMeta(META_COMMAND, true); +} + +/** The next (or previous) suggestion after (or before) the selection in this document, or null. */ +export function nextChange(state: EditorState, direction: 1 | -1): Change | null { + const all = changeRanges(state.doc); + if (all.length === 0) return null; + const { from, to } = state.selection; + const found = + direction === 1 + ? all.find((change) => change.from >= to && change.from !== from) + : [...all].reverse().find((change) => change.from < from); + return found ?? null; +} + +/** The transaction that selects a change's whole range. */ +export function selectRange(state: EditorState, from: number, to: number): Transaction { + return state.tr.setSelection(TextSelection.create(state.doc, from, to)).scrollIntoView(); +} diff --git a/app/src/features/review/comments.test.ts b/app/src/features/review/comments.test.ts new file mode 100644 index 000000000..6985198a7 --- /dev/null +++ b/app/src/features/review/comments.test.ts @@ -0,0 +1,122 @@ +// Comments and their records (ADR 0086; spec 6.10: R-CMT-1, R-CMT-2). A comment's range is its marks, so it moves +// with its text. The review model sends each new mark's record in the same batch as the Markdown. It drops the +// records of marks that left (R-CMT-2: writers drop comments that no mark names). +import { EditorState, TextSelection } from '@tiptap/pm/state'; +import { describe, expect, it } from 'vitest'; +import { marked, markdownOf } from '../../editor/commands/testing'; +import { addComment, commentTarget, commentsAt, removeComment } from './comments'; +import { ReviewModel, stageComment, stageSuggestion } from './model'; +import { commentRanges } from './ranges'; +import type { CommentJson } from './records'; +import { readRecords } from './records'; + +const C1 = '01m3sbcmt0000000000000c001'; +const C2 = '01m3sbcmt0000000000000c002'; +const S1 = '01m3sbsgg0000000000000s001'; +const AT = '2026-11-07T09:00:00.000Z'; +const comment = (markdown: string): CommentJson => ({ + author: 'Avery Quinn', + time: AT, + markdown, + replies: [], + resolved: false, +}); + +describe('comment ranges', () => { + it("a comment's range follows its text when text is inserted before it", () => { + const { state } = marked('Plants [grow] fast'); + let now = state.apply(addComment(state, C1, commentTarget(state)!)!); + const [before] = commentRanges(now.doc); + expect(before.text).toBe('grow'); + now = now.apply(now.tr.insertText('Green ', 1)); + const [after] = commentRanges(now.doc); + expect(after.text).toBe('grow'); + expect(after.from).toBe(before.from + 'Green '.length); + expect(markdownOf(now)).toBe(`Green Plants grow fast`); + }); + + it('comments the word at the caret when nothing is selected, and nothing between words', () => { + const { state } = marked('Cells di[]vide often'); + expect(state.doc.textBetween(commentTarget(state)!.from, commentTarget(state)!.to)).toBe('divide'); + const space = state.apply(state.tr.setSelection(TextSelection.create(state.doc, 'Cells '.length + 1))); + // The caret right after a space and before a word still finds that word. + expect(space.doc.textBetween(commentTarget(space)!.from, commentTarget(space)!.to)).toBe('divide'); + }); + + it('nests two comments on the same text, lowest ID outermost (R-MRK-26), and deletes one alone', () => { + const { state } = marked('A [shared] word'); + let now = state.apply(addComment(state, C2, commentTarget(state)!)!); + now = now.apply(addComment(now, C1, commentTarget(state)!)!); + expect(markdownOf(now)).toBe(`A shared word`); + expect(commentsAt(now.doc, 4)).toEqual([C2, C1]); + now = now.apply(removeComment(now, C2)!); + expect(markdownOf(now)).toBe(`A shared word`); + }); +}); + +describe('review model', () => { + it('sends a new comment record with the Markdown that names it, and drops it when the mark leaves', () => { + const model = new ReviewModel(); + stageComment(C1, comment('')); + const named = `See this`; + expect(model.rider('b1', 'See this', named)).toEqual([ + { edit: 'patchBlock', block: 'b1', data: { comments: { [C1]: comment('') } } }, + ]); + expect(model.of('b1').comments[C1]).toEqual(comment('')); + // A later batch with the same marks sends nothing more. + expect(model.rider('b1', named, `${named}!`)).toEqual([]); + expect(model.rider('b1', named, 'See this')).toEqual([ + { edit: 'patchBlock', block: 'b1', data: { comments: { [C1]: null } } }, + ]); + // Undo in the editor brings the mark back, and the record comes with it. + expect(model.rider('b1', 'See this', named)).toEqual([ + { edit: 'patchBlock', block: 'b1', data: { comments: { [C1]: comment('') } } }, + ]); + model.dispose(); + }); + + it('applies reply, resolve, and reopen patches, and reads records the core sends', () => { + const model = new ReviewModel(); + model.setBlock('b1', { + markdown: `x`, + comments: { [C1]: comment('Check this'), bad: comment('no') }, + suggestions: { [S1]: { kind: 'ins', author: 'Jordan Lee', time: AT } }, + }); + expect(Object.keys(model.of('b1').comments)).toEqual([C1]); + const reply = { author: 'Jordan Lee', time: AT, markdown: 'Done' }; + model.apply('b1', { comments: { [C1]: { replies: [reply], resolved: true } } }); + expect(model.of('b1').comments[C1]).toEqual({ ...comment('Check this'), replies: [reply], resolved: true }); + model.apply('b1', { comments: { [C1]: { resolved: false } } }); + expect(model.comment(C1)?.record.resolved).toBe(false); + expect(model.suggestion(S1)?.author).toBe('Jordan Lee'); + model.dispose(); + }); + + it('sends a suggestion record with its insertion mark', () => { + const model = new ReviewModel(); + const record = { kind: 'ins' as const, author: 'Avery Quinn', time: AT }; + stageSuggestion(S1, record); + expect(model.rider('b2', 'Go', `Go on`)).toEqual([ + { edit: 'patchBlock', block: 'b2', data: { suggestions: { [S1]: record } } }, + ]); + model.dispose(); + }); + + it('reads only valid records (R-CMT-1, R-SUG-1)', () => { + const records = readRecords({ + comments: { [C1]: { author: 'A', time: AT, markdown: 'm', resolved: 'yes' }, [C2]: { author: '', time: AT } }, + suggestions: { [S1]: { kind: 'fmt', author: 'A', time: AT, before: '**x**' }, x: { kind: 'ins' } }, + }); + expect(records.comments).toEqual({ [C1]: { author: 'A', time: AT, markdown: 'm', replies: [], resolved: false } }); + expect(records.suggestions).toEqual({ [S1]: { kind: 'fmt', author: 'A', time: AT, before: '**x**' } }); + }); +}); + +describe('a comment in an editor state', () => { + it('cannot go into a code block', () => { + const { state } = marked('```\ncode here\n```'); + const inCode = state.apply(state.tr.setSelection(TextSelection.create(state.doc, 2, 6))); + expect(addComment(inCode, C1, { from: 2, to: 6 })).toBeNull(); + expect(EditorState).toBeDefined(); + }); +}); diff --git a/app/src/features/review/comments.ts b/app/src/features/review/comments.ts new file mode 100644 index 000000000..e1a3536f0 --- /dev/null +++ b/app/src/features/review/comments.ts @@ -0,0 +1,65 @@ +// Comments in a text block's document (ADR 0086). A comment's range is its `comment` marks, so the range follows its +// text through every edit. Several comments may cover the same text (R-MRK-26). These are the editor-level steps; +// page.ts sends the records. +import type { Node as PMNode } from '@tiptap/pm/model'; +import type { EditorState, Transaction } from '@tiptap/pm/state'; +import { META_COMMAND } from '../../editor/meta'; +import { commentRanges } from './ranges'; +import type { Ranged } from './ranges'; +import { REVIEW_META } from './suggest'; + +const WORD = /[\p{L}\p{N}\p{M}_'’-]/u; + +/** The word around a caret, as Word comments the word at the caret when nothing is selected; null between words. */ +export function wordAround(state: EditorState): { from: number; to: number } | null { + const { $from } = state.selection; + const parent = $from.parent; + if (!parent.isTextblock) return null; + const text = parent.textBetween(0, parent.content.size, '', ''); + let start = $from.parentOffset; + let end = start; + while (start > 0 && WORD.test(text[start - 1])) start--; + while (end < text.length && WORD.test(text[end])) end++; + if (start === end) return null; + const base = $from.start(); + return { from: base + start, to: base + end }; +} + +/** The range a new comment covers: the selection, or the word at the caret. Null when there is no text to comment. */ +export function commentTarget(state: EditorState): { from: number; to: number } | null { + const { from, to, empty } = state.selection; + if (!empty) return state.doc.textBetween(from, to, '', '').trim().length > 0 ? { from, to } : null; + return wordAround(state); +} + +/** The transaction that puts comment `id` over `range`, or null where comments cannot go (a code block). */ +export function addComment(state: EditorState, id: string, range: { from: number; to: number }): Transaction | null { + const type = state.schema.marks.comment; + const tr = state.tr.addMark(range.from, range.to, type.create({ comment: id })); + if (!tr.docChanged) return null; + return tr.setMeta(REVIEW_META, true).setMeta(META_COMMAND, true); +} + +/** The transaction that takes comment `id`'s marks off its text, or null when the document has none. */ +export function removeComment(state: EditorState, id: string): Transaction | null { + const mark = state.schema.marks.comment.create({ comment: id }); + const range = commentRanges(state.doc).find((one) => one.id === id); + if (!range) return null; + const tr = state.tr; + for (const span of range.spans) tr.removeMark(span.from, span.to, mark); + return tr.setMeta(REVIEW_META, true).setMeta(META_COMMAND, true); +} + +/** The comment `id` in a document, or null. */ +export function findRange(doc: PMNode, id: string): Ranged | null { + return commentRanges(doc).find((range) => range.id === id) ?? null; +} + +/** The comments at a position, innermost (latest ID) first. */ +export function commentsAt(doc: PMNode, pos: number): string[] { + return commentRanges(doc) + .filter((range) => range.spans.some((span) => span.from <= pos && pos <= span.to)) + .map((range) => range.id) + .sort() + .reverse(); +} diff --git a/app/src/features/review/extension.ts b/app/src/features/review/extension.ts new file mode 100644 index 000000000..1e62b7edf --- /dev/null +++ b/app/src/features/review/extension.ts @@ -0,0 +1,93 @@ +// The editor side of review (ADR 0086), added to every text editor through the editor extension registry behind +// text.review: the suggesting-mode plugin, and "Insertion by Sam" spoken when the caret enters a suggestion (text +// DESIGN.md 12). This file also loads the marks' styles and keeps the markup view on the root element. +// registrations/review.ts loads it right after start-up. +import { Extension } from '@tiptap/core'; +import { Plugin, PluginKey } from '@tiptap/pm/state'; +import type { EditorState } from '@tiptap/pm/state'; +import { editorExtensions } from '../../editor/extensions/kit'; +import type { EditorHost } from '../../editor/host'; +import { t } from '../../strings/t'; +import { newId } from '../../editor/ids'; +import { findSuggestion, stageSuggestion } from './model'; +import { SUGGESTION_MARKS, kindOfMark, stamp } from './records'; +import { authorName, reviewPrefs, suggesting } from './state'; +import { trackChanges } from './suggest'; +import type { Tracker } from './suggest'; +import './review.module.css'; + +/** The tracker the editors use: the person's name, new IDs, and the page models' records. */ +export const pageTracker: Tracker = { + author: authorName, + newId: () => newId(), + now: () => stamp(), + authorOf: (id) => findSuggestion(id)?.author ?? null, + stage: stageSuggestion, +}; + +const key = new PluginKey('opennoteSuggesting'); + +/** The suggesting-mode plugin: while `on()` holds, each edit becomes a suggestion. */ +export function suggestingPlugin(on: () => boolean, tracker: Tracker): Plugin { + return new Plugin({ + key, + appendTransaction: (transactions, old, state) => (on() ? trackChanges(transactions, old, state, tracker) : null), + }); +} + +const SAY = { ins: 'textReview.insertionBy', del: 'textReview.deletionBy', fmt: 'textReview.formatBy' } as const; + +/** What to say about the suggestion at a caret, with its ID, or null when the caret is in none. */ +export function suggestionAtCaret(state: EditorState): { id: string; text: string } | null { + const { selection } = state; + if (!selection.empty) return null; + const node = selection.$from.nodeAfter ?? selection.$from.nodeBefore; + for (const mark of node?.marks ?? []) { + const kind = kindOfMark(mark.type.name); + if (!kind) continue; + const id = String(mark.attrs[SUGGESTION_MARKS[kind].key]); + const author = findSuggestion(id)?.author ?? t('textReview.unknownAuthor'); + return { id, text: t(SAY[kind], { author }) }; + } + return null; +} + +/** Speaks a suggestion's kind and author once when the caret moves into it. */ +function announcePlugin(host: EditorHost): Plugin { + let last: string | null = null; + return new Plugin({ + key: new PluginKey('opennoteSuggestionAnnounce'), + view: () => ({ + update(view, previous) { + if (previous.selection.eq(view.state.selection) || !view.hasFocus()) return; + const found = suggestionAtCaret(view.state); + if (found && found.id !== last) host.announce(found.text); + last = found?.id ?? null; + }, + }), + }); +} + +const suggestingExtension = (host: EditorHost) => + Extension.create({ + name: 'opennoteSuggesting', + addProseMirrorPlugins() { + return [suggestingPlugin(() => suggesting.get() && this.editor.isEditable, pageTracker), announcePlugin(host)]; + }, + }); + +if (!editorExtensions.get('text.review')) { + editorExtensions.register({ + id: 'text.review', + order: 420, + flag: 'text.review', + kinds: ['text'], + create: suggestingExtension, + }); +} + +function showMarkup(): void { + if (typeof document !== 'undefined') document.documentElement.dataset.reviewMarkup = reviewPrefs.get().markup; +} +showMarkup(); +reviewPrefs.subscribe(showMarkup); diff --git a/app/src/features/review/model.ts b/app/src/features/review/model.ts new file mode 100644 index 000000000..33a41fa98 --- /dev/null +++ b/app/src/features/review/model.ts @@ -0,0 +1,227 @@ +// The comment and suggestion records of a page's text blocks (ADR 0086). The text sync asks `rider` for edits to +// send in the same batch as a block's new Markdown, so a mark and its record always change in one undo step. A mark +// that comes into a block brings its record. It may be a new one that a command or suggesting mode staged. It may +// also be one that was cut, moved, or deleted and typed back. Writers drop records that no mark names (R-CMT-2, R-SUG-2), so the rider +// does that too. +import { markIdsInMarkdown, readRecords } from './records'; +import type { CommentJson, SuggestionJson } from './records'; + +/** A merge patch of one block's records: a record, part of one, or null to remove it. */ +export type CommentsPatch = Record | null>; +export type SuggestionsPatch = Record; + +/** The `patchBlock` edit that changes a block's records. It has the shape of the page service's `Edit`. */ +export interface ReviewEdit { + edit: 'patchBlock'; + block: string; + data: { comments?: CommentsPatch; suggestions?: SuggestionsPatch }; +} + +interface BlockRecords { + comments: Record; + suggestions: Record; +} + +/** How many records of marks that left their blocks are kept for a paste or an undo in the editor. */ +const KEEP_LEFT = 1000; +/** How many staged records are kept, in case a mark's batch is never built. */ +const KEEP_STAGED = 1000; + +type Staged = { kind: 'comment'; record: CommentJson } | { kind: 'suggestion'; record: SuggestionJson }; + +/** Records that a command or suggesting mode made for marks it is about to put in a block, in any page. */ +const staged = new Map(); +const models = new Set(); + +function trim(map: Map, most: number): void { + while (map.size > most) map.delete(map.keys().next().value!); +} + +/** Stages a new comment's record: the next batch of the block whose Markdown names it sends it along. */ +export function stageComment(id: string, record: CommentJson): void { + staged.set(id, { kind: 'comment', record }); + trim(staged, KEEP_STAGED); +} + +/** Stages a new suggestion's record. */ +export function stageSuggestion(id: string, record: SuggestionJson): void { + staged.set(id, { kind: 'suggestion', record }); + trim(staged, KEEP_STAGED); +} + +/** A suggestion's record wherever it is: staged, or in a block of an open page. */ +export function findSuggestion(id: string): SuggestionJson | null { + const one = staged.get(id); + if (one?.kind === 'suggestion') return one.record; + for (const model of models) { + const found = model.suggestion(id); + if (found) return found; + } + return null; +} + +/** A comment's record wherever it is: staged, or in a block of an open page. */ +export function findComment(id: string): CommentJson | null { + const one = staged.get(id); + if (one?.kind === 'comment') return one.record; + for (const model of models) { + const found = model.comment(id)?.record; + if (found) return found; + } + return null; +} + +const empty = (): BlockRecords => ({ comments: {}, suggestions: {} }); + +export class ReviewModel { + private readonly blocks = new Map(); + private readonly left = new Map(); + private readonly listeners = new Set<() => void>(); + + constructor() { + models.add(this); + } + + /** The model's page closed. */ + dispose(): void { + models.delete(this); + this.listeners.clear(); + } + + /** The records of a block, as the core has them (with this window's patches applied). */ + of(block: string): Readonly { + return this.blocks.get(block) ?? empty(); + } + + blockIds(): string[] { + return [...this.blocks.keys()]; + } + + comment(id: string): { block: string; record: CommentJson } | null { + for (const [block, records] of this.blocks) + if (records.comments[id]) return { block, record: records.comments[id] }; + return null; + } + + suggestion(id: string): SuggestionJson | null { + for (const records of this.blocks.values()) if (records.suggestions[id]) return records.suggestions[id]; + return null; + } + + /** The core sent a block's data (the page as opened, an undo, another window): it is the truth. */ + setBlock(block: string, data: Record): void { + const records = readRecords(data); + if (Object.keys(records.comments).length === 0 && Object.keys(records.suggestions).length === 0) { + this.blocks.delete(block); + } else this.blocks.set(block, records); + this.changed(); + } + + /** The block left the page. Its records stay known, so an undo in an editor or a paste can bring them back. */ + removeBlock(block: string): void { + const records = this.blocks.get(block); + if (!records) return; + for (const [id, record] of Object.entries(records.comments)) this.keepLeft(id, { kind: 'comment', record }); + for (const [id, record] of Object.entries(records.suggestions)) this.keepLeft(id, { kind: 'suggestion', record }); + this.blocks.delete(block); + this.changed(); + } + + /** Applies a patch this window sends, so the model need not wait for the core. */ + apply(block: string, data: ReviewEdit['data']): void { + const records = this.blocks.get(block) ?? empty(); + const comments = { ...records.comments }; + const suggestions = { ...records.suggestions }; + for (const [id, value] of Object.entries(data.comments ?? {})) { + staged.delete(id); + if (value === null) delete comments[id]; + else + comments[id] = { + ...(comments[id] ?? { author: '', time: '', markdown: '', replies: [], resolved: false }), + ...value, + }; + } + for (const [id, value] of Object.entries(data.suggestions ?? {})) { + staged.delete(id); + if (value === null) delete suggestions[id]; + else suggestions[id] = value; + } + if (Object.keys(comments).length === 0 && Object.keys(suggestions).length === 0) this.blocks.delete(block); + else this.blocks.set(block, { comments, suggestions }); + this.changed(); + } + + /** + * The record edits that go with a block's new Markdown, `before` to `after`, in the same batch. An empty list when + * the block's records already match its marks. + */ + rider(block: string, before: string, after: string): ReviewEdit[] { + const have = this.of(block); + const known = Object.keys(have.comments).length + Object.keys(have.suggestions).length; + const ids = markIdsInMarkdown(after); + if (known === 0 && ids.comments.length === 0 && ids.suggestions.size === 0) return []; + if (before === after && staged.size === 0) return []; + const comments: CommentsPatch = {}; + const suggestions: SuggestionsPatch = {}; + const named = new Set(ids.comments); + for (const [id, record] of Object.entries(have.comments)) { + if (named.has(id)) continue; + comments[id] = null; + this.keepLeft(id, { kind: 'comment', record }); + } + for (const [id, record] of Object.entries(have.suggestions)) { + if (ids.suggestions.has(id)) continue; + suggestions[id] = null; + this.keepLeft(id, { kind: 'suggestion', record }); + } + for (const id of ids.comments) { + if (have.comments[id]) continue; + const found = this.bring(block, id, 'comment'); + if (found?.kind === 'comment') comments[id] = found.record; + } + for (const id of ids.suggestions.keys()) { + if (have.suggestions[id]) continue; + const found = this.bring(block, id, 'suggestion'); + if (found?.kind === 'suggestion') suggestions[id] = found.record; + } + const data: ReviewEdit['data'] = {}; + if (Object.keys(comments).length > 0) data.comments = comments; + if (Object.keys(suggestions).length > 0) data.suggestions = suggestions; + if (!data.comments && !data.suggestions) return []; + this.apply(block, data); + return [{ edit: 'patchBlock', block, data }]; + } + + onChange(listener: () => void): () => void { + this.listeners.add(listener); + return () => void this.listeners.delete(listener); + } + + /** The record for a mark that came into `block`: staged, left, or a copy of one another block holds. */ + private bring(block: string, id: string, kind: Staged['kind']): Staged | null { + const found = staged.get(id) ?? this.left.get(id) ?? this.elsewhere(block, id, kind); + if (!found || found.kind !== kind) return null; + staged.delete(id); + this.left.delete(id); + return found; + } + + private elsewhere(block: string, id: string, kind: Staged['kind']): Staged | null { + for (const [other, records] of this.blocks) { + if (other === block) continue; + if (kind === 'comment' && records.comments[id]) return { kind, record: structuredClone(records.comments[id]) }; + if (kind === 'suggestion' && records.suggestions[id]) return { kind, record: { ...records.suggestions[id] } }; + } + return null; + } + + private keepLeft(id: string, value: Staged): void { + this.left.delete(id); + this.left.set(id, value); + trim(this.left, KEEP_LEFT); + } + + private changed(): void { + this.listeners.forEach((listener) => listener()); + } +} diff --git a/app/src/features/review/ranges.ts b/app/src/features/review/ranges.ts new file mode 100644 index 000000000..f1c20caeb --- /dev/null +++ b/app/src/features/review/ranges.ts @@ -0,0 +1,78 @@ +// Where comments and suggestions sit in a text block's document. A comment or suggestion is a mark, so its range moves +// with its text in every edit. The marks with one ID may sit in several runs and paragraphs (R-CMT-2); together they +// are its range. This file lists those ranges in document order. +import type { Node as PMNode } from '@tiptap/pm/model'; +import { SUGGESTION_MARKS, kindOfMark } from './records'; +import type { SuggestionKind } from './records'; + +export interface Span { + from: number; + to: number; +} + +/** A comment or a suggestion, with the parts of the document it covers, first part first. */ +export interface Ranged { + id: string; + spans: Span[]; + /** The start of the first part. */ + from: number; + /** The end of the last part. */ + to: number; + /** The text it covers, its parts joined by a space. */ + text: string; +} + +export interface Change extends Ranged { + kind: SuggestionKind; +} + +function collect(doc: PMNode, idOf: (mark: PMNode['marks'][number]) => string | null): Map { + const found = new Map(); + doc.descendants((node, pos) => { + if (!node.isInline) return true; + for (const mark of node.marks) { + const id = idOf(mark); + if (id === null) continue; + const spans = found.get(id) ?? []; + const last = spans[spans.length - 1]; + if (last && last.to === pos) last.to = pos + node.nodeSize; + else spans.push({ from: pos, to: pos + node.nodeSize }); + found.set(id, spans); + } + return false; + }); + return found; +} + +function ranged(doc: PMNode, id: string, spans: Span[]): Ranged { + const text = spans.map((span) => doc.textBetween(span.from, span.to, ' ', ' ')).join(' '); + return { id, spans, from: spans[0].from, to: spans[spans.length - 1].to, text }; +} + +/** The comments of a document, in the order their ranges start. */ +export function commentRanges(doc: PMNode): Ranged[] { + const found = collect(doc, (mark) => (mark.type.name === 'comment' ? String(mark.attrs.comment) : null)); + return [...found].map(([id, spans]) => ranged(doc, id, spans)).sort((a, b) => a.from - b.from); +} + +/** The suggestions of a document, in the order their ranges start. */ +export function changeRanges(doc: PMNode): Change[] { + const kinds = new Map(); + const found = collect(doc, (mark) => { + const kind = kindOfMark(mark.type.name); + if (!kind) return null; + const id = String(mark.attrs[SUGGESTION_MARKS[kind].key]); + kinds.set(id, kind); + return id; + }); + return [...found] + .map(([id, spans]) => ({ ...ranged(doc, id, spans), kind: kinds.get(id)! })) + .sort((a, b) => a.from - b.from); +} + +/** Whether a range touches the span `from` to `to`; an empty span touches a range it is inside or at the edge of. */ +export function touches(range: Ranged, from: number, to: number): boolean { + return range.spans.some((span) => + from === to ? span.from <= from && from <= span.to : span.from < to && from < span.to, + ); +} diff --git a/app/src/features/review/records.ts b/app/src/features/review/records.ts new file mode 100644 index 000000000..76f611ba3 --- /dev/null +++ b/app/src/features/review/records.ts @@ -0,0 +1,136 @@ +// Comment and suggestion records (spec 6.10: R-CMT-1, R-SUG-1; ADR 0086). A comment's range and a suggestion's +// text are marks in a text block's Markdown, and their records live in the same block's `data.comments` and +// `data.suggestions`, keyed by the marks' IDs. This file reads the records and finds the IDs the Markdown names. +import type { Node as PMNode } from '@tiptap/pm/model'; +import { parseTextBlock } from '../../editor/markdown'; +import { isElementId } from '../../editor/schema/constants'; +export { MAX_AUTHOR, cleanAuthor } from './state'; + +export interface ReplyJson { + author: string; + time: string; + markdown: string; +} + +/** A comment's record (R-CMT-1). */ +export interface CommentJson { + author: string; + time: string; + markdown: string; + replies: ReplyJson[]; + resolved: boolean; +} + +export type SuggestionKind = 'ins' | 'del' | 'fmt'; + +/** A suggestion's record (R-SUG-1). `before` is the canonical inline Markdown of a format change's range. */ +export interface SuggestionJson { + kind: SuggestionKind; + author: string; + time: string; + before?: string; +} + +/** The mark names of the suggestion kinds (R-MRK-20 to R-MRK-22), and their attribute keys. */ +export const SUGGESTION_MARKS: Readonly> = { + ins: { mark: 'insertion', key: 'ins' }, + del: { mark: 'deletion', key: 'del' }, + fmt: { mark: 'formatChange', key: 'fmt' }, +}; + +/** The suggestion kind of a mark name, or null for other marks. */ +export function kindOfMark(name: string): SuggestionKind | null { + if (name === 'insertion') return 'ins'; + if (name === 'deletion') return 'del'; + if (name === 'formatChange') return 'fmt'; + return null; +} + +/** The longest Markdown of a comment or reply (spec 16: 64 KiB). */ +export const MAX_RECORD_BYTES = 64 * 1024; +/** The most replies a comment holds (spec 16). */ +export const MAX_REPLIES = 256; + +const isRecord = (value: unknown): value is Record => + !!value && typeof value === 'object' && !Array.isArray(value); + +const text = (value: unknown) => (typeof value === 'string' ? value : null); + +/** A timestamp as records hold it: UTC with milliseconds (spec 2.5). */ +export const stamp = (now: number = Date.now()) => new Date(now).toISOString(); + +function readReply(value: unknown): ReplyJson | null { + if (!isRecord(value)) return null; + const author = text(value.author); + const time = text(value.time); + const markdown = text(value.markdown); + return author && time && markdown !== null ? { author, time, markdown } : null; +} + +/** A valid comment record, or null. Replies that are not valid are left out, as the core reads them. */ +export function readComment(value: unknown): CommentJson | null { + if (!isRecord(value)) return null; + const author = text(value.author); + const time = text(value.time); + const markdown = text(value.markdown); + if (!author || !time || markdown === null) return null; + const replies = Array.isArray(value.replies) + ? value.replies.map(readReply).filter((reply): reply is ReplyJson => reply !== null) + : []; + return { author, time, markdown, replies, resolved: value.resolved === true }; +} + +/** A valid suggestion record, or null. */ +export function readSuggestion(value: unknown): SuggestionJson | null { + if (!isRecord(value)) return null; + const kind = value.kind; + const author = text(value.author); + const time = text(value.time); + if ((kind !== 'ins' && kind !== 'del' && kind !== 'fmt') || !author || !time) return null; + const before = text(value.before); + return kind === 'fmt' && before !== null ? { kind, author, time, before } : { kind, author, time }; +} + +function readAll(value: unknown, read: (one: unknown) => T | null): Record { + const out: Record = {}; + if (!isRecord(value)) return out; + for (const [id, one] of Object.entries(value)) { + const record = isElementId(id) ? read(one) : null; + if (record) out[id] = record; + } + return out; +} + +/** The comment and suggestion records of a text block's data. */ +export function readRecords(data: Record): { + comments: Record; + suggestions: Record; +} { + return { comments: readAll(data.comments, readComment), suggestions: readAll(data.suggestions, readSuggestion) }; +} + +/** The comment and suggestion IDs a document's marks name, each once, in document order. */ +export function markIds(doc: PMNode): { comments: string[]; suggestions: Map } { + const comments = new Set(); + const suggestions = new Map(); + doc.descendants((node) => { + for (const mark of node.marks) { + if (mark.type.name === 'comment') comments.add(String(mark.attrs.comment)); + const kind = kindOfMark(mark.type.name); + if (kind) suggestions.set(String(mark.attrs[SUGGESTION_MARKS[kind].key]), kind); + } + return true; + }); + return { comments: [...comments], suggestions }; +} + +const NAMES = /data-(?:comment|ins|del|fmt)=/; + +/** + * The IDs a block's Markdown names. Most blocks have no review marks, which skips the parse. The attribute can + * still be literal text (escaped, or in code), so the parse decides. + */ +export function markIdsInMarkdown(markdown: string): ReturnType { + if (!NAMES.test(markdown)) return { comments: [], suggestions: new Map() }; + return markIds(parseTextBlock(markdown)); +} diff --git a/app/src/features/review/review.module.css b/app/src/features/review/review.module.css new file mode 100644 index 000000000..ce4fc75e2 --- /dev/null +++ b/app/src/features/review/review.module.css @@ -0,0 +1,159 @@ +/* + * Comment and suggestion marks (ADR 0086). The marks are spans with data-comment, data-ins, data-del, and data-fmt + * (spec 7.10), in the editor and in static text alike. The markup view is a data attribute on the root element: + * All shows every mark, Simple shows the text with the changes made, and None also hides comment highlights. Under + * forced colors the marks keep a dashed underline in Highlight, so they never depend on color alone. + */ +@layer components { + :global([data-comment]) { + background-color: var(--color-accent-candle-subtle); + border-block-end: 1px dashed var(--color-text-muted); + } + + :global([data-ins]) { + color: var(--color-status-success); + text-decoration-line: underline; + text-decoration-thickness: 1px; + text-underline-offset: 2px; + } + + :global([data-del]) { + color: var(--color-status-danger); + text-decoration-line: line-through; + } + + :global([data-fmt]) { + text-decoration-line: underline; + text-decoration-style: double; + text-decoration-color: var(--color-text-muted); + } + + :global([data-review-markup='simple'] [data-ins]), + :global([data-review-markup='none'] [data-ins]), + :global([data-review-markup='simple'] [data-fmt]), + :global([data-review-markup='none'] [data-fmt]) { + color: inherit; + text-decoration-line: none; + } + + :global([data-review-markup='simple'] [data-del]), + :global([data-review-markup='none'] [data-del]) { + display: none; + } + + /* Simple keeps a quiet bar at the start of each change, so changed lines can still be found. */ + :global([data-review-markup='simple'] [data-ins]) { + box-shadow: inset 2px 0 0 var(--color-text-muted); + } + + :global([data-review-markup='none'] [data-comment]) { + background-color: transparent; + border-block-end: 0; + } + + @media (forced-colors: active) { + :global([data-comment]), + :global([data-ins]), + :global([data-del]), + :global([data-fmt]) { + text-decoration-color: Highlight; + border-block-end: 1px dashed Highlight; + forced-color-adjust: none; + color: CanvasText; + } + } +} + +.pane { + position: absolute; + inset-block: 0; + inset-inline-end: 0; + z-index: var(--layer-page-chrome); + display: flex; + flex-direction: column; + inline-size: min(320px, 100%); + background: var(--color-surface-raised); + border-inline-start: 1px solid var(--color-border-subtle); + color: var(--color-text-primary); +} + +.header { + display: flex; + align-items: center; + gap: 8px; + padding: 8px 12px; + border-block-end: 1px solid var(--color-border-subtle); +} + +.title { + flex: 1; + margin: 0; + font-size: var(--type-body-size); + font-weight: 600; +} + +.list { + flex: 1; + overflow-y: auto; + margin: 0; + padding: 8px; + list-style: none; +} + +.comment { + margin-block-end: 8px; + padding: 8px; + border: 1px solid var(--color-border-subtle); + border-radius: 6px; + background: var(--color-surface-page); +} + +.comment[data-active] { + border-color: var(--color-focus-ring); +} + +.comment[data-resolved] { + color: var(--color-text-secondary); +} + +.quote { + margin: 0 0 4px; + color: var(--color-text-secondary); + font-style: italic; +} + +.meta { + margin: 0; + color: var(--color-text-muted); + font-size: var(--type-small-size); +} + +.body { + margin: 4px 0; + white-space: pre-wrap; +} + +.reply { + margin-inline-start: 12px; + padding-inline-start: 8px; + border-inline-start: 2px solid var(--color-border-subtle); +} + +.actions { + display: flex; + flex-wrap: wrap; + gap: 4px; + margin-block-start: 4px; +} + +.input { + inline-size: 100%; + min-block-size: 3em; + box-sizing: border-box; + font: inherit; +} + +.empty { + padding: 12px; + color: var(--color-text-secondary); +} diff --git a/app/src/features/review/state.ts b/app/src/features/review/state.ts new file mode 100644 index 000000000..23381c03a --- /dev/null +++ b/app/src/features/review/state.ts @@ -0,0 +1,62 @@ +// The Review tab's state (ADR 0086): whether suggesting mode is on, which markup view shows, and the name that new +// comments and suggestions carry. The name is a display name the person types; it is never an account ID and is +// never sent anywhere but into the page. It is kept on this device. This file loads at start-up, so it imports +// nothing from the editor. +import { createStore } from '../../state/store'; +import { t } from '../../strings/t'; + +/** Simple shows the text with changes made and a bar beside changed lines; All shows every mark; None hides them. */ +export type Markup = 'simple' | 'all' | 'none'; + +/** The longest author name a record holds (spec: 1 to 100 characters). */ +export const MAX_AUTHOR = 100; + +/** A name that fits a record: trimmed, 1 to 100 characters, or the fallback. */ +export function cleanAuthor(name: string, fallback: string): string { + const trimmed = [...name.trim()].slice(0, MAX_AUTHOR).join(''); + return trimmed.length > 0 ? trimmed : fallback; +} + +export const MARKUPS: readonly Markup[] = ['simple', 'all', 'none']; + +export interface ReviewPrefs { + author: string; + markup: Markup; +} + +const KEY = 'opennote.review'; + +function load(): ReviewPrefs { + const prefs: ReviewPrefs = { author: '', markup: 'all' }; + try { + const saved: unknown = JSON.parse(globalThis.localStorage?.getItem(KEY) ?? 'null'); + if (saved && typeof saved === 'object') { + const { author, markup } = saved as Record; + if (typeof author === 'string') prefs.author = [...author].slice(0, MAX_AUTHOR).join(''); + if (MARKUPS.includes(markup as Markup)) prefs.markup = markup as Markup; + } + } catch { + // Defaults hold. + } + return prefs; +} + +export const reviewPrefs = createStore(load(), 'review prefs'); + +/** Whether typing and deleting make suggestions. It starts off in every window, as in Word. */ +export const suggesting = createStore(false, 'suggesting mode'); + +export function setReviewPrefs(change: Partial): void { + const next = { ...reviewPrefs.get(), ...change }; + reviewPrefs.set(next); + try { + globalThis.localStorage?.setItem(KEY, JSON.stringify(next)); + } catch { + // The choice holds for this session. + } +} + +/** The name new comments and suggestions carry: the person's display name, or "Author" until they set one. */ +export function authorName(): string { + return cleanAuthor(reviewPrefs.get().author, t('textReview.defaultAuthor')); +} diff --git a/app/src/features/review/suggest.test.ts b/app/src/features/review/suggest.test.ts new file mode 100644 index 000000000..8817f4007 --- /dev/null +++ b/app/src/features/review/suggest.test.ts @@ -0,0 +1,194 @@ +// @vitest-environment jsdom +// Suggesting mode and accept or reject (ADR 0086). The expected Markdown follows spec 7.10 (R-MRK-20 to R-MRK-22: +// ``, ``, ``). It also follows the rules in the ADR. +// Typing makes one insertion with the person's name. A deletion stays as a deletion mark. Accept or reject settles +// them. +import { EditorState, TextSelection } from '@tiptap/pm/state'; +import type { Transaction } from '@tiptap/pm/state'; +import { afterEach, beforeEach, describe, expect, it } from 'vitest'; +import { marked, markdownOf, mountEditor, typeInto } from '../../editor/commands/testing'; +import type { TestEditor } from '../../editor/commands/testing'; +import { decide, nextChange, pick } from './changes'; +import { findSuggestion } from './model'; +import { changeRanges } from './ranges'; +import type { SuggestionJson } from './records'; +import { setReviewPrefs, suggesting } from './state'; +import { suggestingPlugin, suggestionAtCaret } from './extension'; +import type { Tracker } from './suggest'; + +const NAME = 'Avery Quinn'; +const OTHER = 'Jordan Lee'; + +/** A tracker with its own records, and IDs that count up. */ +function testTracker(author = NAME) { + const records = new Map(); + let next = 0; + const tracker: Tracker & { records: typeof records; who: string } = { + records, + who: author, + author: () => tracker.who, + newId: () => `01m3sbsgg0000000000000s${String(++next).padStart(3, '0')}`, + now: () => '2026-11-07T09:00:00.000Z', + authorOf: (id) => records.get(id)?.author ?? null, + stage: (id, record) => void records.set(id, record), + }; + return tracker; +} + +/** A state over marked Markdown with suggesting mode on. */ +function suggestingState(source: string, tracker: Tracker) { + const { state } = marked(source); + return EditorState.create({ + doc: state.doc, + selection: state.selection, + plugins: [suggestingPlugin(() => true, tracker)], + }); +} + +/** Types text one character per transaction, as keystrokes do. */ +function type(state: EditorState, text: string): EditorState { + let now = state; + for (const char of text) now = now.apply(now.tr.insertText(char)); + return now; +} + +/** Backspace: deletes the character before the caret. */ +function backspace(state: EditorState): EditorState { + const at = state.selection.from; + return state.apply(state.tr.delete(at - 1, at)); +} + +function run(state: EditorState, tr: Transaction | null): EditorState { + expect(tr).not.toBeNull(); + return state.apply(tr!); +} + +describe('suggesting mode', () => { + it("typing 'abc' makes one insertion with the person's name; Accept all leaves 'abc'; Reject all removes it", () => { + const tracker = testTracker(); + const state = type(suggestingState('Say []', tracker), 'abc'); + const changes = changeRanges(state.doc); + expect(changes).toHaveLength(1); + expect(changes[0]).toMatchObject({ kind: 'ins', text: 'abc' }); + expect(tracker.records.get(changes[0].id)).toEqual({ kind: 'ins', author: NAME, time: '2026-11-07T09:00:00.000Z' }); + expect(markdownOf(state)).toBe(`Say abc`); + expect(markdownOf(run(state, decide(state, pick(state.doc), 'accept')))).toBe('Say abc'); + expect(markdownOf(run(state, decide(state, pick(state.doc), 'reject')))).toBe('Say '); + }); + + it("deleting 'xyz' keeps it as a deletion mark; Accept removes it, and Reject keeps the text", () => { + const tracker = testTracker(); + let state = suggestingState('Keep xyz[] here', tracker); + for (let i = 0; i < 3; i++) state = backspace(state); + const changes = changeRanges(state.doc); + expect(changes).toHaveLength(1); + expect(changes[0]).toMatchObject({ kind: 'del', text: 'xyz' }); + expect(tracker.records.get(changes[0].id)?.kind).toBe('del'); + expect(markdownOf(state)).toBe(`Keep xyz here`); + // Backspace leaves the caret before the text it deleted. + expect(state.selection.from).toBe(changes[0].from); + expect(markdownOf(run(state, decide(state, pick(state.doc), 'accept')))).toBe('Keep here'); + expect(markdownOf(run(state, decide(state, pick(state.doc), 'reject')))).toBe('Keep xyz here'); + }); + + it('a typed replacement of a selection is a deletion and an insertion', () => { + const tracker = testTracker(); + const state = type(suggestingState('A [cat] sat', tracker), 'dog'); + expect(changeRanges(state.doc).map((change) => [change.kind, change.text])).toEqual([ + ['del', 'cat'], + ['ins', 'dog'], + ]); + expect(markdownOf(run(state, decide(state, pick(state.doc), 'accept')))).toBe('A dog sat'); + expect(markdownOf(run(state, decide(state, pick(state.doc), 'reject')))).toBe('A cat sat'); + }); + + it('deleting text one typed as a suggestion removes it without a deletion mark', () => { + const tracker = testTracker(); + let state = type(suggestingState('Hi []', tracker), 'ab'); + state = backspace(backspace(state)); + expect(markdownOf(state)).toBe('Hi '); + expect(changeRanges(state.doc)).toEqual([]); + }); + + it("another author's typing next to an insertion makes a second suggestion", () => { + const tracker = testTracker(); + let state = type(suggestingState('Go []', tracker), 'ab'); + tracker.who = OTHER; + state = type(state, 'cd'); + const changes = changeRanges(state.doc); + expect(changes.map((change) => tracker.records.get(change.id)?.author)).toEqual([NAME, OTHER]); + const byOther = pick(state.doc, { author: OTHER, authorOf: tracker.authorOf }); + expect(byOther.map((change) => change.text)).toEqual(['cd']); + expect(markdownOf(run(state, decide(state, byOther, 'reject')))).toBe( + `Go ab`, + ); + }); + + it('a formatting command makes a format change; Reject puts the old Markdown back', () => { + const tracker = testTracker(); + const state = suggestingState('Make [this] bold', tracker); + const bold = state.schema.marks.bold.create(); + const after = state.apply(state.tr.addMark(state.selection.from, state.selection.to, bold)); + const [change] = changeRanges(after.doc); + expect(change).toMatchObject({ kind: 'fmt', text: 'this' }); + expect(tracker.records.get(change.id)).toMatchObject({ kind: 'fmt', author: NAME, before: 'this' }); + const beforeOf = (id: string) => tracker.records.get(id)?.before; + expect(markdownOf(run(after, decide(after, [change], 'accept', beforeOf)))).toBe('Make **this** bold'); + expect(markdownOf(run(after, decide(after, [change], 'reject', beforeOf)))).toBe('Make this bold'); + }); + + it('edits with suggesting mode off stay plain', () => { + const { state } = marked('Plain []'); + const plain = EditorState.create({ + doc: state.doc, + selection: state.selection, + plugins: [suggestingPlugin(() => false, testTracker())], + }); + expect(markdownOf(type(plain, 'abc'))).toBe('Plain abc'); + }); + + it('Next change moves through the suggestions in order', () => { + const tracker = testTracker(); + let state = type(suggestingState('One [] two', tracker), 'x'); + state = state.apply(state.tr.setSelection(TextSelection.atEnd(state.doc))); + state = type(state, 'y'); + state = state.apply(state.tr.setSelection(TextSelection.atStart(state.doc))); + const first = nextChange(state, 1); + expect(first?.text).toBe('x'); + state = state.apply(state.tr.setSelection(TextSelection.create(state.doc, first!.from, first!.to))); + expect(nextChange(state, 1)?.text).toBe('y'); + expect(nextChange(state, -1)).toBeNull(); + }); +}); + +describe('the suggestion at the caret', () => { + it('names the kind and the author, for a screen reader', () => { + setReviewPrefs({ author: NAME }); + const state = type(suggestingState('Hi []', testTracker()), 'yo'); + const inside = state.apply(state.tr.setSelection(TextSelection.create(state.doc, 5))); + // The tracker's records are its own, so the page models do not know this author. + expect(suggestionAtCaret(inside)?.text).toBe('Insertion by an unknown author'); + expect(suggestionAtCaret(state.apply(state.tr.setSelection(TextSelection.create(state.doc, 1))))).toBeNull(); + }); +}); + +describe('suggesting mode in an editor', () => { + let test: TestEditor | null = null; + beforeEach(() => { + setReviewPrefs({ author: NAME }); + suggesting.set(true); + }); + afterEach(() => { + suggesting.set(false); + test?.destroy(); + test = null; + }); + + it("typing in a mounted editor stages one record with the person's name", () => { + test = mountEditor('Note []'); + typeInto(test.editor, 'abc'); + const [change] = changeRanges(test.editor.state.doc); + expect(change).toMatchObject({ kind: 'ins', text: 'abc' }); + expect(findSuggestion(change.id)).toMatchObject({ kind: 'ins', author: NAME }); + }); +}); diff --git a/app/src/features/review/suggest.ts b/app/src/features/review/suggest.ts new file mode 100644 index 000000000..ac68fe8c3 --- /dev/null +++ b/app/src/features/review/suggest.ts @@ -0,0 +1,229 @@ +// Suggesting mode (track changes; ADR 0086). While it is on, an edit stays visible as a suggestion: +// - typed text gets an insertion mark; +// - deleted text comes back with a deletion mark; +// - a formatting command puts a formatChange mark over the text it changed, with its Markdown from before. +// Each suggestion's record is staged. The page's review model sends it with the block's Markdown, so a mark and its +// record make one undo step. Deleting text one typed as a suggestion just removes it, as in Word and Docs. +import type { Mark, MarkType, Node as PMNode } from '@tiptap/pm/model'; +import { Fragment, Slice } from '@tiptap/pm/model'; +import { TextSelection } from '@tiptap/pm/state'; +import type { EditorState, Transaction } from '@tiptap/pm/state'; +import { AddMarkStep, RemoveMarkStep, ReplaceStep } from '@tiptap/pm/transform'; +import type { StepMap } from '@tiptap/pm/transform'; +import { createMarkdownCache, serializeTextBlock } from '../../editor/markdown'; +import { META_HIGHLIGHT, META_REMOTE } from '../../editor/meta'; +import type { SuggestionJson, SuggestionKind } from './records'; + +/** A transaction a review command made (accept, reject, a new comment, or this mode's own marks). It is not tracked. */ +export const REVIEW_META = 'opennote.review'; + +/** What suggesting mode needs from the page: who is typing, new IDs, and where records go. */ +export interface Tracker { + author(): string; + newId(): string; + now(): string; + /** The author of a suggestion, so typing at the end of one's own insertion joins it. */ + authorOf(id: string): string | null; + stage(id: string, record: SuggestionJson): void; +} + +const REVIEW_MARKS = new Set(['comment', 'insertion', 'deletion', 'formatChange']); +const cache = createMarkdownCache(); + +interface ReplaceEvent { + kind: 'replace'; + index: number; + from: number; + to: number; + inserted: number; + deleted: Slice; +} + +interface MarkEvent { + kind: 'mark'; + from: number; + to: number; +} + +const markOf = (node: PMNode | null | undefined, type: MarkType) => node?.marks.find((mark) => mark.type === type); + +function isTracked(tr: Transaction): boolean { + return tr.docChanged && !tr.getMeta(META_REMOTE) && !tr.getMeta(REVIEW_META) && !tr.getMeta(META_HIGHLIGHT); +} + +/** The edits of the transactions, and every step's map, so positions can be carried to the final document. */ +function eventsOf(transactions: readonly Transaction[]): { events: (ReplaceEvent | MarkEvent)[]; maps: StepMap[] } { + const events: (ReplaceEvent | MarkEvent)[] = []; + const maps: StepMap[] = []; + for (const tr of transactions) { + const tracked = isTracked(tr); + tr.steps.forEach((step, i) => { + if (tracked && step instanceof ReplaceStep) { + const deleted = tr.docs[i].slice(step.from, step.to); + events.push({ + kind: 'replace', + index: maps.length, + from: step.from, + to: step.to, + inserted: step.slice.size, + deleted, + }); + } else if (tracked && (step instanceof AddMarkStep || step instanceof RemoveMarkStep)) { + if (!REVIEW_MARKS.has(step.mark.type.name)) events.push({ kind: 'mark', from: step.from, to: step.to }); + } + maps.push(step.getMap()); + }); + } + return { events, maps }; +} + +class Recorder { + readonly types: Record; + + constructor( + readonly tr: Transaction, + readonly tracker: Tracker, + ) { + const marks = tr.doc.type.schema.marks; + this.types = { ins: marks.insertion, del: marks.deletion, fmt: marks.formatChange }; + } + + private own(mark: Mark | undefined): mark is Mark { + if (!mark) return false; + const id = String(Object.values(mark.attrs)[0]); + return this.tracker.authorOf(id) === this.tracker.author(); + } + + /** The ID for a new suggestion at `pos`: that of the author's own suggestion of this kind right beside it, or new. */ + private idAt(kind: SuggestionKind, from: number, to: number): string { + const type = this.types[kind]; + const doc = this.tr.doc; + for (const mark of [markOf(doc.resolve(from).nodeBefore, type), markOf(doc.resolve(to).nodeAfter, type)]) { + if (this.own(mark)) return String(Object.values(mark.attrs)[0]); + } + const id = this.tracker.newId(); + this.tracker.stage(id, { kind, author: this.tracker.author(), time: this.tracker.now() }); + return id; + } + + /** Marks typed text as the author's insertion. Text already inserted by the author stays as it is. */ + insertion(from: number, to: number): void { + const { ins, del } = this.types; + if (from >= to) return; + this.tr.removeMark(from, to, del); + let id: string | null = null; + this.tr.doc.nodesBetween(from, to, (node, pos, parent) => { + if (!node.isInline) return true; + if (this.own(markOf(node, ins)) || !parent?.type.allowsMarkType(ins)) return false; + const start = Math.max(from, pos); + const end = Math.min(to, pos + node.nodeSize); + id ??= this.idAt('ins', from, to); + this.tr.removeMark(start, end, ins); + this.tr.addMark(start, end, ins.create({ ins: id })); + return false; + }); + } + + /** The deleted content with deletion marks; the author's own insertions in it are dropped. */ + private deletedContent(fragment: Fragment, mark: () => Mark): Fragment { + const { ins, del } = this.types; + const out: PMNode[] = []; + fragment.forEach((node) => { + if (!node.isInline) { + out.push(node.copy(this.deletedContent(node.content, mark))); + } else if (!this.own(markOf(node, ins))) { + out.push(markOf(node, del) ? node : node.mark(mark().addToSet(node.marks))); + } + }); + return Fragment.from(out); + } + + /** Whether every text block in the content takes the deletion mark. */ + private allowsMarks(fragment: Fragment): boolean { + let ok = true; + fragment.descendants((node) => { + if (node.isTextblock && !node.type.allowsMarkType(this.types.del)) ok = false; + return ok && !node.isTextblock; + }); + return ok; + } + + /** Puts deleted content back at `at` as the author's deletion. Returns its size, or 0 when nothing came back. */ + deletion(at: number, deleted: Slice): number { + const { del } = this.types; + const $at = this.tr.doc.resolve(at); + const inline = deleted.openStart === 0 && deleted.openEnd === 0 && deleted.content.firstChild?.isInline; + if (inline ? !$at.parent.type.allowsMarkType(del) : !this.allowsMarks(deleted.content)) return 0; + let id: string | null = null; + const mark = () => del.create({ del: (id ??= this.idAt('del', at, at)) }); + const content = this.deletedContent(deleted.content, mark); + if (content.textBetween(0, content.size, '', '').length === 0) return 0; + const before = this.tr.doc.content.size; + this.tr.replace(at, at, new Slice(content, deleted.openStart, deleted.openEnd)); + return this.tr.doc.content.size - before; + } + + /** Marks a formatting change over text whose positions are the same before and after it. */ + formatChange(old: PMNode, from: number, to: number): void { + const { fmt } = this.types; + const doc = this.tr.doc; + doc.nodesBetween(from, to, (node, pos) => { + if (!node.isTextblock) return true; + const start = Math.max(from, pos + 1); + const end = Math.min(to, pos + node.nodeSize - 1); + if (start >= end || !node.type.allowsMarkType(fmt)) return false; + let covered = true; + doc.nodesBetween(start, end, (child) => { + if (child.isInline && !markOf(child, fmt)) covered = false; + return covered; + }); + if (covered) return false; + const paragraph = old.type.schema.nodes.paragraph.create(null, old.slice(start, end).content); + const before = serializeTextBlock(old.type.create(null, [paragraph]), cache).trim(); + const id = this.tracker.newId(); + this.tracker.stage(id, { kind: 'fmt', author: this.tracker.author(), time: this.tracker.now(), before }); + this.tr.removeMark(start, end, fmt); + this.tr.addMark(start, end, fmt.create({ fmt: id })); + return false; + }); + } +} + +/** + * The transaction that turns the edits of `transactions` into suggestions, or null when they need none. It is for + * a plugin's appendTransaction: `old` is the state before the transactions and `state` the state after them. + */ +export function trackChanges( + transactions: readonly Transaction[], + old: EditorState, + state: EditorState, + tracker: Tracker, +): Transaction | null { + if (!transactions.some(isTracked)) return null; + const { events, maps } = eventsOf(transactions); + if (events.length === 0) return null; + const toEnd = (index: number, pos: number, assoc: number) => + maps.slice(index + 1).reduce((at, map) => map.map(at, assoc), pos); + const tr = state.tr; + const recorder = new Recorder(tr, tracker); + const replaces = events.filter((event): event is ReplaceEvent => event.kind === 'replace'); + let caret: number | null = null; + for (const event of replaces) { + const start = tr.mapping.map(toEnd(event.index, event.from, 1), 1); + const end = tr.mapping.map(toEnd(event.index, event.from + event.inserted, -1), -1); + recorder.insertion(start, end); + if (event.deleted.size === 0) continue; + const at = tr.mapping.map(toEnd(event.index, event.from, -1), -1); + const back = recorder.deletion(at, event.deleted); + // Backspace leaves the caret before the text it deleted; Delete and a typed replacement leave it after. + const backspace = old.selection.empty && old.selection.head === event.to && event.inserted === 0; + if (back > 0 && replaces.length === 1 && backspace) caret = at; + } + // A formatting command changes no positions, so the text before it is at the same place in the old document. + if (replaces.length === 0) { + for (const event of events) recorder.formatChange(old.doc, event.from, event.to); + } + if (!tr.docChanged) return null; + if (caret !== null) tr.setSelection(TextSelection.create(tr.doc, caret)); + return tr.setMeta(REVIEW_META, true).setMeta('addToHistory', false); +} diff --git a/app/src/strings/en/textReview.ts b/app/src/strings/en/textReview.ts index eb5db3cf2..7923cfc40 100644 --- a/app/src/strings/en/textReview.ts +++ b/app/src/strings/en/textReview.ts @@ -1,4 +1,131 @@ // The Review tab: comments, changes, and proofing. // Owner: text:review (B11 and B12). text:scaffold registered it in the string index, and the owner fills it in. -export const textReview = {} as const; +export const textReview = { + defaultAuthor: 'Author', + // Commands. + newComment: 'New comment', + newCommentKeywords: 'comment note remark annotate review', + showComments: 'Show comments', + showCommentsKeywords: 'comments pane list review', + nextComment: 'Next comment', + previousComment: 'Previous comment', + commentNavKeywords: 'comment next previous go review', + resolveComment: 'Resolve comment', + reopenComment: 'Reopen comment', + deleteComment: 'Delete comment', + commentKeywords: 'comment resolve reopen delete review', + trackChanges: 'Track changes', + trackChangesKeywords: 'suggest suggesting track changes edit mode review', + accept: 'Accept change', + reject: 'Reject change', + acceptAll: 'Accept all changes', + rejectAll: 'Reject all changes', + acceptByAuthor: 'Accept all changes by…', + rejectByAuthor: 'Reject all changes by…', + decideKeywords: 'accept reject suggestion change track review', + nextChange: 'Next change', + previousChange: 'Previous change', + changeNavKeywords: 'change suggestion next previous review', + markup: 'Markup', + markupSimple: 'Simple markup', + markupAll: 'All markup', + markupNone: 'No markup', + markupKeywords: 'markup simple all none show changes review', + // Announcements. + suggestingOn: 'Suggesting. Your edits show as suggestions.', + suggestingOff: 'Editing. Your edits change the text.', + needsText: 'Put the caret in a text box first.', + needsSelection: 'Select some text or put the caret in a word to comment on it.', + cannotHere: 'A comment can’t go here. Put the caret in ordinary text.', + noComments: 'This page has no comments.', + noCommentHere: 'Put the caret in a comment first.', + noChanges: 'There are no changes.', + noChangeHere: 'Put the caret in a change first.', + accepted: '{count, plural, one {# change accepted} other {# changes accepted}}', + rejected: '{count, plural, one {# change rejected} other {# changes rejected}}', + insertionBy: 'Insertion by {author}', + deletionBy: 'Deletion by {author}', + formatBy: 'Format change by {author}', + unknownAuthor: 'an unknown author', + markupNow: '{markup} shows.', + // The comments pane. + pane: 'Comments', + close: 'Close comments', + yourName: 'Your name for comments and changes', + yourNameHelp: 'Only this name goes into the page. It stays on this device.', + empty: 'No comments yet. Select text and choose New comment.', + commentLabel: 'Comment by {author}', + on: 'On “{text}”', + commentText: 'Comment text', + replyText: 'Reply', + reply: 'Reply', + post: 'Post', + resolve: 'Resolve', + reopen: 'Reopen', + delete: 'Delete', + goToText: 'Go to text', + resolved: 'Resolved', + when: '{author}, {time}', + // The compare view. + formattingChanged: 'Formatting changed: {details}', + markAdded: '{mark} added to “{text}”', + markRemoved: '{mark} removed from “{text}”', + markChanged: '{mark} changed on “{text}”', + paraChanged: '{key}: {value}', + paraRemoved: '{key} removed', + marks: { + link: 'link', + comment: 'comment', + insertion: 'insertion', + deletion: 'deletion', + formatChange: 'format change', + dir: 'direction', + lang: 'language', + charStyle: 'character style', + bold: 'bold', + weight: 'font weight', + italic: 'italic', + strike: 'strikethrough', + underline: 'underline', + highlight: 'highlight', + shade: 'shading', + textColor: 'text color', + font: 'font', + textSize: 'font size', + caps: 'capitals', + tracking: 'character spacing', + scale: 'character scale', + shift: 'raised or lowered', + kern: 'kerning', + otf: 'font features', + effect: 'text effect', + box: 'border', + subscript: 'subscript', + superscript: 'superscript', + code: 'code', + other: 'formatting', + }, + paras: { + align: 'alignment', + indent: 'indentation', + before: 'space before', + after: 'space after', + contextual: 'no space between paragraphs of one style', + line: 'line spacing', + keepNext: 'keep with next', + keepLines: 'keep lines together', + breakBefore: 'page break before', + widows: 'widow and orphan control', + tabs: 'tab stops', + border: 'borders', + shade: 'shading', + dir: 'direction', + lang: 'language', + outline: 'outline level', + dropCap: 'drop cap', + list: 'list', + hyphenate: 'hyphenation', + other: 'paragraph formatting', + }, +} as const; From 8104f15452adb337c374d456090481a1f3bdeaab Mon Sep 17 00:00:00 2001 From: Eric Dean Date: Sat, 7 Nov 2026 13:34:12 -0800 Subject: [PATCH 08/49] Add the comments pane and the Review tab commands The comments pane lists a page's comments in reading order. It posts a comment's text, replies, resolves, reopens, deletes, and goes to the text. The Review tab gets New comment, Show comments, Previous and Next comment, Resolve, Delete, Track changes (Ctrl+Shift+E), the markup views, Accept and Reject (one, all, or by author), and Previous and Next change, all behind text.review. The person's display name for comments and changes is typed in the pane and kept on this device. Co-Authored-By: Claude Opus 5.5 --- app/src/features/page/registrations/review.ts | 229 ++++++++++++++++++ app/src/features/review/CommentsPane.tsx | 172 +++++++++++++ app/src/features/review/actions.ts | 62 +++++ app/src/features/review/commands.ts | 192 +++++++++++++++ app/src/features/review/index.ts | 8 + app/src/features/review/page.ts | 170 +++++++++++++ app/src/features/review/pane.test.tsx | 158 ++++++++++++ app/src/features/review/pane.ts | 56 +++++ 8 files changed, 1047 insertions(+) create mode 100644 app/src/features/page/registrations/review.ts create mode 100644 app/src/features/review/CommentsPane.tsx create mode 100644 app/src/features/review/actions.ts create mode 100644 app/src/features/review/commands.ts create mode 100644 app/src/features/review/index.ts create mode 100644 app/src/features/review/page.ts create mode 100644 app/src/features/review/pane.test.tsx create mode 100644 app/src/features/review/pane.ts diff --git a/app/src/features/page/registrations/review.ts b/app/src/features/page/registrations/review.ts new file mode 100644 index 000000000..5f125c9f6 --- /dev/null +++ b/app/src/features/page/registrations/review.ts @@ -0,0 +1,229 @@ +// Comments, suggestions (track changes), and the markup views (text program, Beta 12; ADR 0086), behind text.review. +// This file only registers: the page hook, the commands, and the Review tab's buttons. The review code loads when a +// page view first needs it or a command runs, so it stays out of the start-up bundle. +import { isEnabled } from '../../../app/flags'; +import { chord } from '../../../commands/registry'; +import type { CommandCategory, CommandId } from '../../../commands/types'; +import { commandBar, commands } from '../../../registries'; +import type { MessageKey } from '../../../strings/t'; +import { loadReviewCommands, loadReviewExtension, loadReviewPage, reviewPrefs, suggesting } from '../../review'; +import type { Markup } from '../../review'; +import { mountedPageHooks, shownMounted } from '../pagesApi'; + +const commandsModule = loadReviewCommands; +const FLAG = 'text.review' as const; + +// The editor extension (its registration names the flag) and the marks' styles load right after start-up, before +// most editors mount. +void loadReviewExtension().catch(() => undefined); + +mountedPageHooks.register({ + id: 'text.review', + attach(mounted) { + if (!isEnabled(FLAG)) return () => undefined; + let detach: (() => void) | null = null; + let gone = false; + // The text sync and the text block view load with the page view, so these imports cost nothing more. + void Promise.all([loadReviewPage(), import('../sync/textSync'), import('../blocks/textBlock')]).then( + ([m, { addTextRider }, { liveText }]) => { + if (gone) return; + detach = m.attachReview(mounted, { addTextRider, liveDoc: (view) => liveText(view)?.liveDoc() ?? null }).detach; + }, + ); + return () => { + gone = true; + detach?.(); + }; + }, +}); + +interface Spec { + id: CommandId; + title: MessageKey; + keywords: MessageKey; + icon: string; + category?: CommandCategory; + keys?: readonly string[]; + checked?: () => boolean; + /** The Review tab group and priority; none for palette-only commands. */ + bar?: { group: string; priority: number; presentation?: 'toggle' }; + run(m: Awaited>): void | Promise; +} + +function add(spec: Spec): void { + commands.register({ + id: spec.id, + title: spec.title, + keywords: spec.keywords, + category: spec.category ?? 'editing', + icon: spec.icon, + flag: FLAG, + ...(spec.keys ? { keys: spec.keys.map(chord), scope: 'editor' as const, allowInTextInput: true } : {}), + ...(spec.checked ? { checked: spec.checked } : {}), + when: () => shownMounted.get() !== null, + run: () => void commandsModule().then((m) => spec.run(m)), + }); + if (spec.bar) { + commandBar.register({ id: spec.id, tab: 'review', command: spec.id, flag: FLAG, ...spec.bar }); + } +} + +add({ + id: 'text.review.newComment', + title: 'textReview.newComment', + keywords: 'textReview.newCommentKeywords', + icon: 'ChatCircleText', + category: 'insert', + bar: { group: 'comments', priority: 90 }, + run: (m) => m.newComment(), +}); +// Insert, Comment (the Insert tab order is text:scaffold's: Footnote, Endnote, Comment). +commandBar.register({ + id: 'text.review.newComment.insert', + tab: 'insert', + group: 'comment', + command: 'text.review.newComment', + priority: 50, + flag: FLAG, +}); +add({ + id: 'text.review.showComments', + title: 'textReview.showComments', + keywords: 'textReview.showCommentsKeywords', + icon: 'ChatsCircle', + category: 'view', + bar: { group: 'comments', priority: 80 }, + run: (m) => m.toggleComments(), +}); +add({ + id: 'text.review.previousComment', + title: 'textReview.previousComment', + keywords: 'textReview.commentNavKeywords', + icon: 'CaretLeft', + category: 'navigation', + bar: { group: 'comments', priority: 60 }, + run: (m) => m.moveToComment(-1), +}); +add({ + id: 'text.review.nextComment', + title: 'textReview.nextComment', + keywords: 'textReview.commentNavKeywords', + icon: 'CaretRight', + category: 'navigation', + bar: { group: 'comments', priority: 60 }, + run: (m) => m.moveToComment(1), +}); +add({ + id: 'text.review.resolveComment', + title: 'textReview.resolveComment', + keywords: 'textReview.commentKeywords', + icon: 'CheckCircle', + bar: { group: 'comments', priority: 50 }, + run: (m) => m.resolveHere(true), +}); +add({ + id: 'text.review.reopenComment', + title: 'textReview.reopenComment', + keywords: 'textReview.commentKeywords', + icon: 'ArrowCounterClockwise', + run: (m) => m.resolveHere(false), +}); +add({ + id: 'text.review.deleteComment', + title: 'textReview.deleteComment', + keywords: 'textReview.commentKeywords', + icon: 'Trash', + bar: { group: 'comments', priority: 40 }, + run: (m) => m.deleteHere(), +}); +add({ + id: 'text.review.trackChanges', + title: 'textReview.trackChanges', + keywords: 'textReview.trackChangesKeywords', + icon: 'PencilSimpleLine', + // Word's chord for Track Changes. + keys: ['Ctrl+Shift+E'], + checked: () => suggesting.get(), + bar: { group: 'tracking', priority: 90, presentation: 'toggle' }, + run: (m) => m.toggleSuggesting(), +}); +for (const [markup, title, priority] of [ + ['simple', 'textReview.markupSimple', 50], + ['all', 'textReview.markupAll', 50], + ['none', 'textReview.markupNone', 40], +] as const satisfies readonly (readonly [Markup, MessageKey, number])[]) { + add({ + id: `text.review.markup.${markup}`, + title, + keywords: 'textReview.markupKeywords', + icon: markup === 'none' ? 'EyeSlash' : 'Eye', + category: 'view', + checked: () => reviewPrefs.get().markup === markup, + bar: { group: 'tracking', priority }, + run: (m) => m.setMarkup(markup), + }); +} +add({ + id: 'text.review.accept', + title: 'textReview.accept', + keywords: 'textReview.decideKeywords', + icon: 'Check', + bar: { group: 'changes', priority: 90 }, + run: (m) => m.decideHere('accept'), +}); +add({ + id: 'text.review.reject', + title: 'textReview.reject', + keywords: 'textReview.decideKeywords', + icon: 'X', + bar: { group: 'changes', priority: 90 }, + run: (m) => m.decideHere('reject'), +}); +add({ + id: 'text.review.acceptAll', + title: 'textReview.acceptAll', + keywords: 'textReview.decideKeywords', + icon: 'Checks', + bar: { group: 'changes', priority: 70 }, + run: (m) => m.decideAll('accept'), +}); +add({ + id: 'text.review.rejectAll', + title: 'textReview.rejectAll', + keywords: 'textReview.decideKeywords', + icon: 'XCircle', + bar: { group: 'changes', priority: 70 }, + run: (m) => m.decideAll('reject'), +}); +add({ + id: 'text.review.acceptByAuthor', + title: 'textReview.acceptByAuthor', + keywords: 'textReview.decideKeywords', + icon: 'UserCheck', + run: (m) => m.decideByAuthor('accept'), +}); +add({ + id: 'text.review.rejectByAuthor', + title: 'textReview.rejectByAuthor', + keywords: 'textReview.decideKeywords', + icon: 'UserMinus', + run: (m) => m.decideByAuthor('reject'), +}); +add({ + id: 'text.review.previousChange', + title: 'textReview.previousChange', + keywords: 'textReview.changeNavKeywords', + icon: 'ArrowUp', + category: 'navigation', + bar: { group: 'changes', priority: 60 }, + run: (m) => m.moveToChange(-1), +}); +add({ + id: 'text.review.nextChange', + title: 'textReview.nextChange', + keywords: 'textReview.changeNavKeywords', + icon: 'ArrowDown', + category: 'navigation', + bar: { group: 'changes', priority: 60 }, + run: (m) => m.moveToChange(1), +}); diff --git a/app/src/features/review/CommentsPane.tsx b/app/src/features/review/CommentsPane.tsx new file mode 100644 index 000000000..36bba2802 --- /dev/null +++ b/app/src/features/review/CommentsPane.tsx @@ -0,0 +1,172 @@ +// The comments pane (ADR 0086): the page's comments in reading order, each with the text it covers, its author and +// time, its text, and its replies. A comment can be answered, resolved, reopened, deleted, or followed to its text. +// The comment at the caret is marked. Comments are notes on their ranges for screen readers (text DESIGN.md 12). +import { useEffect, useReducer, useState } from 'react'; +import type { KeyboardEvent } from 'react'; +import { XIcon } from '@phosphor-icons/react/dist/csr/X'; +import { escapeParagraphText, parseTextBlock } from '../../editor/markdown'; +import { currentLocale } from '../../state/language'; +import { t } from '../../strings/t'; +import { Button, IconButton, TextField } from '../../ui'; +import { targetEditor } from '../page'; +import { deleteComment, goToComment, setResolved } from './actions'; +import { commentsAt } from './comments'; +import type { PageComment, PageReview } from './page'; +import { stamp } from './records'; +import type { CommentJson } from './records'; +import { authorName, reviewPrefs, setReviewPrefs } from './state'; +import styles from './review.module.css'; + +/** A record's inline Markdown as plain text, hard breaks as new lines. */ +function plain(markdown: string): string { + const doc = parseTextBlock(markdown); + return doc.textBetween(0, doc.content.size, '\n', (leaf) => (leaf.type.name === 'hardBreak' ? '\n' : '')); +} + +/** Plain text typed in the pane as inline Markdown. */ +const markdownOf = (text: string) => escapeParagraphText(text.trim()); + +function when(time: string): string { + const date = new Date(time); + return Number.isNaN(date.getTime()) + ? time + : date.toLocaleString(currentLocale(), { dateStyle: 'medium', timeStyle: 'short' }); +} + +function Editor(props: { label: string; action: string; autoFocus?: boolean; onSubmit(text: string): void }) { + const [text, setText] = useState(''); + const submit = () => { + if (!text.trim()) return; + props.onSubmit(text); + setText(''); + }; + const onKeyDown = (event: KeyboardEvent) => { + if (event.key === 'Enter' && (event.ctrlKey || event.metaKey)) { + event.preventDefault(); + submit(); + } + }; + return ( +
+