From 09953708c5feafee5839ff7e81d75e7a2a3d0917 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20H=C3=BCning?= Date: Sat, 3 Oct 2026 19:19:30 +0200 Subject: [PATCH 1/3] feat(docs): show who created a page and who contributed to it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Byline under the title: "Created by · ", plus everyone who changed the page's content, each with when they last edited. Readable by every role that can open the page; names only. GET /api/docs/{id}/contributors. Contributors are recorded in a new doc_contributors table, upserted in the same statement that persists the edit (last_edited_at refreshed at most once a minute). To make that possible, live WS edits now carry their author, the writer stores each row's own author instead of the batch's first, and history restore and workspace import are attributed. Pages that predate this only list contributions from now on (documents.contributors_since), and say so. fix(collab): edits typed while disconnected never reached the server The client applied offline edits locally but only sent while its socket was open, and the server never sent SyncStep1, so a reconnecting client was never asked for what the server lacked. The edits were lost on reload, and later edits building on them were parked as pending, invisible to everyone else. The server now sends its state vector on join (docs and boards), which already-loaded clients answer without a reload. No-op replies (nothing new, just the delete set) are neither persisted nor fanned out, so opening a page writes no row and credits no one. Also: KnotLink skips autolinking text that arrives from a remote change, so an observer's editor no longer writes link edits under its user's name. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 23 + crates/knot-crdt/src/board_room.rs | 21 +- crates/knot-crdt/src/engine.rs | 137 +++ crates/knot-crdt/src/lib.rs | 2 +- crates/knot-crdt/src/room.rs | 390 ++++++- crates/knot-crdt/src/writer.rs | 10 +- crates/knot-server/src/board_room_shim.rs | 18 +- crates/knot-server/src/lib.rs | 7 +- crates/knot-server/src/protocol.rs | 18 + crates/knot-server/src/room.rs | 29 +- .../src/routes/api/contributors.rs | 64 ++ crates/knot-server/src/routes/api/docs.rs | 4 + .../src/routes/api/export_import.rs | 3 +- crates/knot-server/src/routes/api/history.rs | 8 +- crates/knot-server/src/routes/api/mod.rs | 1 + .../tests/contributors_integration.rs | 964 ++++++++++++++++++ crates/knot-server/tests/convergence.rs | 21 +- crates/knot-server/tests/offline_sync.rs | 719 +++++++++++++ crates/knot-storage/src/contributors.rs | 111 ++ crates/knot-storage/src/lib.rs | 4 + crates/knot-storage/src/updates_store.rs | 80 +- crates/knot-storage/tests/cancel_safety.rs | 7 +- crates/knot-storage/tests/contributors.rs | 172 ++++ crates/knot-storage/tests/migrations_apply.rs | 122 +++ crates/knot-storage/tests/updates.rs | 210 +++- e2e/flows/contributors.spec.ts | 127 +++ e2e/flows/ws-reconnect.spec.ts | 41 +- e2e/support/reset.ts | 1 + .../20261003120000_doc_contributors.sql | 43 + web/src/features/boards/BoardProvider.test.ts | 52 + web/src/features/comments/CommentThread.tsx | 9 +- web/src/features/docs/DocByline.test.tsx | 238 +++++ web/src/features/docs/DocByline.tsx | 156 +++ web/src/features/docs/DocPage.byline.test.tsx | 95 ++ web/src/features/docs/DocPage.tsx | 2 + web/src/features/docs/docs.api.test.ts | 59 ++ web/src/features/docs/docs.api.ts | 9 +- web/src/features/editor/KnotProvider.test.ts | 92 +- web/src/features/editor/autolink.test.ts | 78 ++ web/src/features/editor/extensions.ts | 44 + web/src/lib/relTime.test.ts | 38 + web/src/lib/relTime.ts | 15 + web/src/lib/validators.ts | 25 + web/src/test/fakeSocket.ts | 95 ++ 44 files changed, 4255 insertions(+), 109 deletions(-) create mode 100644 crates/knot-server/src/routes/api/contributors.rs create mode 100644 crates/knot-server/tests/contributors_integration.rs create mode 100644 crates/knot-server/tests/offline_sync.rs create mode 100644 crates/knot-storage/src/contributors.rs create mode 100644 crates/knot-storage/tests/contributors.rs create mode 100644 e2e/flows/contributors.spec.ts create mode 100644 migrations/20261003120000_doc_contributors.sql create mode 100644 web/src/features/boards/BoardProvider.test.ts create mode 100644 web/src/features/docs/DocByline.test.tsx create mode 100644 web/src/features/docs/DocByline.tsx create mode 100644 web/src/features/docs/DocPage.byline.test.tsx create mode 100644 web/src/features/docs/docs.api.test.ts create mode 100644 web/src/features/editor/autolink.test.ts create mode 100644 web/src/lib/relTime.test.ts create mode 100644 web/src/lib/relTime.ts create mode 100644 web/src/test/fakeSocket.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b91c05..aa32a39 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,29 @@ so this log can be regenerated from history (e.g. with `git-cliff`). ## [Unreleased] +### Added +- Page byline: "Created by … · date" under the title, plus the people who have + changed the page's content (typing, imports, restores, task toggles), each + with when they last edited. Visible to every role that can open the page; + names only. Pages created before this release list only contributions made + from now on, and say so. `GET /api/docs/{id}/contributors`. + +### Fixed +- **Edits typed while disconnected never reached the server.** The client kept + them locally but only sent changes while its socket was open, and the server + never asked a reconnecting client for what it was missing. They were lost on + reload, and later edits building on them were parked by the server as + pending, invisible to everyone else. The server now sends its state vector + on join (y-protocol SyncStep1), so the client uploads what the server lacks. + Already-loaded clients benefit without a reload. Boards had the same gap and + get the same fix. +- Live edits were stored without an author, and a writer batch was stamped + with its first row's author. Every `doc_updates` row now records who made it; + history restore and workspace import are attributed too. +- An open editor no longer autolinks text that arrives from someone else's + change, which wrote a link edit into the shared document under the + observer's name. + ## [0.7.0] - 2026-10-02 ### Added diff --git a/crates/knot-crdt/src/board_room.rs b/crates/knot-crdt/src/board_room.rs index 6c87b36..6156796 100644 --- a/crates/knot-crdt/src/board_room.rs +++ b/crates/knot-crdt/src/board_room.rs @@ -20,6 +20,7 @@ use crate::bus::Bus; use crate::engine::{DocHandle, Engine, EngineError}; use crate::protocol::wrap_sync_update; +pub use crate::room::JoinState; pub type ConnId = Uuid; @@ -37,7 +38,7 @@ pub enum Event { Join { conn_id: ConnId, handle: ConnHandle, - reply: oneshot::Sender, EngineError>>, + reply: oneshot::Sender>, }, Leave(ConnId), AwarenessIn { @@ -219,18 +220,24 @@ impl BoardRoom { &mut self, conn_id: ConnId, handle: ConnHandle, - reply: oneshot::Sender, EngineError>>, + reply: oneshot::Sender>, ) { self.conns.insert(conn_id, handle); - let r = self.engine.encode_state_as_update(&self.doc, None); - let _ = reply.send(r); + let _ = reply.send(JoinState::of(self.engine.as_ref(), &self.doc)); } #[tracing::instrument(skip(self, m), fields(board_id = %self.board_id, bytes = m.bytes.len()))] async fn on_inbound(&mut self, m: InMsg) { - if let Err(e) = self.engine.apply_update(&self.doc, &m.bytes) { - tracing::debug!(error=?e, "apply_update failed"); - return; + match self.engine.apply_update_changed(&self.doc, &m.bytes) { + Ok(true) => {} + // Nothing new — the usual answer to the server's SyncStep1 from a + // (re)connecting client. Don't append a `board_updates` row per + // open or echo the no-op to every peer (see `Room::on_inbound`). + Ok(false) => return, + Err(e) => { + tracing::debug!(error=?e, "apply_update failed"); + return; + } } // Persist inline and capture the seq so we can advance the watermark // before publishing — ensures the self-NOTIFY from our own publish diff --git a/crates/knot-crdt/src/engine.rs b/crates/knot-crdt/src/engine.rs index 15f535e..776b54e 100644 --- a/crates/knot-crdt/src/engine.rs +++ b/crates/knot-crdt/src/engine.rs @@ -30,6 +30,14 @@ impl DocHandle { pub trait Engine: Send + Sync + 'static { fn new_doc(&self) -> DocHandle; fn apply_update(&self, d: &DocHandle, update: &[u8]) -> Result<(), EngineError>; + /// Apply `update` and report whether the document changed: new content + /// integrated, something deleted, or content parked as pending because a + /// dependency is missing. `Ok(false)` means the update carried nothing the + /// doc did not already have — e.g. the SyncStep2 a reconnecting client + /// sends back, which always includes its full delete set. Callers use it + /// to skip persisting / fanning out such no-ops; anything that could not + /// be integrated yet reports `true`, so it is never dropped. + fn apply_update_changed(&self, d: &DocHandle, update: &[u8]) -> Result; fn encode_state_as_update( &self, d: &DocHandle, @@ -54,6 +62,24 @@ impl Engine for YrsEngine { Ok(()) } + fn apply_update_changed(&self, d: &DocHandle, update: &[u8]) -> Result { + let u = Update::decode_v1(update).map_err(|e| EngineError::Apply(e.to_string()))?; + let mut txn = d.0.transact_mut(); + txn.apply_update(u) + .map_err(|e| EngineError::Apply(e.to_string()))?; + // - state moved: new structs were integrated; + // - delete_set: holds only items THIS transaction newly deleted + // (re-deleting an already-deleted item adds nothing), so the full + // delete set every SyncStep2 carries does not count by itself; + // - missing updates: structs/deletes yrs had to park as pending. + // Doc-wide rather than per-update, so while anything is pending every + // update counts as a change — conservative on purpose: persisting a + // redundant row is harmless, dropping unintegrated content is not. + Ok(txn.before_state() != txn.after_state() + || !txn.delete_set().is_empty() + || txn.has_missing_updates()) + } + fn encode_state_as_update( &self, d: &DocHandle, @@ -86,3 +112,114 @@ pub struct TextMarkAttr { pub name: String, pub value: String, } + +#[cfg(test)] +mod tests { + use super::*; + use yrs::{GetString, Text}; + + /// Encode everything `doc` holds as a v1 update. + fn full_update(doc: &Doc) -> Vec { + doc.transact() + .encode_state_as_update_v1(&StateVector::default()) + } + + #[test] + fn apply_update_changed_reports_new_content() { + let src = Doc::new(); + src.get_or_insert_text("t") + .push(&mut src.transact_mut(), "hello"); + + let e = YrsEngine; + let d = e.new_doc(); + assert!(e.apply_update_changed(&d, &full_update(&src)).unwrap()); + } + + /// The reconnect handshake makes every writer reply with a SyncStep2 + /// that usually re-sends what the server already has (plus the whole + /// delete set). That must read as "unchanged". + #[test] + fn reapplying_known_content_including_deletes_is_unchanged() { + let src = Doc::new(); + let t = src.get_or_insert_text("t"); + t.push(&mut src.transact_mut(), "hellox"); + t.remove_range(&mut src.transact_mut(), 5, 1); + let update = full_update(&src); + + let e = YrsEngine; + let d = e.new_doc(); + assert!(e.apply_update_changed(&d, &update).unwrap()); + assert!( + !e.apply_update_changed(&d, &update).unwrap(), + "re-applying an identical update (with a delete set) must be a no-op" + ); + // And a diff against our own state vector — exactly what a client + // answers to the server's SyncStep1 — is a no-op too. + let sv = e.encode_state_vector(&d).unwrap(); + let reply = { + let sv = StateVector::decode_v1(&sv).unwrap(); + src.transact().encode_state_as_update_v1(&sv) + }; + assert!(!e.apply_update_changed(&d, &reply).unwrap()); + } + + /// A deletion adds no new structs (the state vector does not move) but + /// it does change the document. + #[test] + fn delete_only_update_is_changed() { + let src = Doc::new(); + let t = src.get_or_insert_text("t"); + t.push(&mut src.transact_mut(), "hello"); + + let e = YrsEngine; + let d = e.new_doc(); + e.apply_update(&d, &full_update(&src)).unwrap(); + + let before = src.transact().state_vector(); + t.remove_range(&mut src.transact_mut(), 0, 1); + let delete_only = src.transact().encode_state_as_update_v1(&before); + assert_eq!( + src.transact().state_vector(), + before, + "fixture: a delete must not advance the state vector" + ); + + assert!(e.apply_update_changed(&d, &delete_only).unwrap()); + let txt = d.0.get_or_insert_text("t"); + assert_eq!(txt.get_string(&d.0.transact()), "ello"); + } + + /// An update whose dependency the server lacks cannot be integrated yet; + /// yrs parks it as pending. It must still count as a change so the room + /// persists it — dropping it would lose the edit for good once the + /// missing piece arrives later. + #[test] + fn update_with_missing_dependency_is_changed() { + let src = Doc::new(); + let t = src.get_or_insert_text("t"); + t.push(&mut src.transact_mut(), "a"); + let sv_after_a = src.transact().state_vector(); + t.push(&mut src.transact_mut(), "b"); + let only_b = src.transact().encode_state_as_update_v1(&sv_after_a); + + let e = YrsEngine; + let d = e.new_doc(); + assert!( + e.apply_update_changed(&d, &only_b).unwrap(), + "an update parked as pending must be reported as changed" + ); + assert!(d.0.transact().has_missing_updates()); + assert_eq!( + e.encode_state_vector(&d).unwrap(), + StateVector::default().encode_v1(), + "fixture: the dependent update must not have integrated" + ); + } + + #[test] + fn malformed_update_is_an_error() { + let e = YrsEngine; + let d = e.new_doc(); + assert!(e.apply_update_changed(&d, &[0xff, 0xff, 0xff]).is_err()); + } +} diff --git a/crates/knot-crdt/src/lib.rs b/crates/knot-crdt/src/lib.rs index fc315ba..a3d45df 100644 --- a/crates/knot-crdt/src/lib.rs +++ b/crates/knot-crdt/src/lib.rs @@ -21,6 +21,6 @@ pub use engine::{DocHandle, Engine, EngineError, TextMark, TextMarkAttr, YrsEngi pub use gc::spawn as spawn_gc; pub use presence::PRESENCE_MAX_BYTES; pub use registry::{AcquireError, Rooms}; -pub use room::{ConnHandle, ConnId, Event, InMsg, Room, RoomHandle}; +pub use room::{ConnHandle, ConnId, Event, InMsg, JoinState, Room, RoomHandle}; pub use snapshot::{SnapshotPolicy, SnapshotState}; pub use writer::{Applied, PersistJob}; diff --git a/crates/knot-crdt/src/room.rs b/crates/knot-crdt/src/room.rs index cb80721..f441f03 100644 --- a/crates/knot-crdt/src/room.rs +++ b/crates/knot-crdt/src/room.rs @@ -25,6 +25,10 @@ pub type ConnId = Uuid; /// Bytes delivered from a local connection's WS read task. pub struct InMsg { pub from: ConnId, + /// The authenticated user behind the connection, persisted as the + /// update's author (`doc_updates.by_user_id`) and thereby credited as a + /// contributor. `None` only where no user is meaningful (tests). + pub by_user: Option, pub bytes: Vec, } @@ -34,13 +38,36 @@ pub struct ConnHandle { pub tx: mpsc::Sender>, } +/// What a joining connection needs for both halves of the y-sync handshake. +/// +/// `update` is the full document state, sent as SyncStep2. `state_vector` is +/// sent as the server's own SyncStep1 so the client replies with whatever the +/// server lacks — the only path by which edits a client made while its socket +/// was down ever reach the server (the web providers apply offline edits +/// locally but do not queue them for sending). Both are read in the same +/// actor turn, so they describe the same document state. +#[derive(Debug)] +pub struct JoinState { + pub update: Vec, + pub state_vector: Vec, +} + +impl JoinState { + pub(crate) fn of(engine: &dyn Engine, doc: &DocHandle) -> Result { + Ok(Self { + update: engine.encode_state_as_update(doc, None)?, + state_vector: engine.encode_state_vector(doc)?, + }) + } +} + /// All inputs the room actor multiplexes. pub enum Event { Inbound(InMsg), Join { conn_id: ConnId, handle: ConnHandle, - reply: oneshot::Sender, EngineError>>, + reply: oneshot::Sender>, }, Leave(ConnId), AwarenessIn { @@ -67,9 +94,9 @@ pub enum Event { ReplaceWithMarkdown { /// Full-state update bytes encoding the replacement content. update_bytes: Vec, - /// Attribution for the persisted update, mirroring `ApplyUpdate`. - /// `None` where there is no meaningful actor (workspace import), or - /// where the caller has not been threaded through yet. + /// Attribution for the persisted update, mirroring `ApplyUpdate`: + /// the importer / restorer, credited as a contributor. `None` only + /// where there is no meaningful actor (tests). by_user: Option, reply: oneshot::Sender>, }, @@ -553,18 +580,27 @@ impl Room { &mut self, conn_id: ConnId, handle: ConnHandle, - reply: oneshot::Sender, EngineError>>, + reply: oneshot::Sender>, ) { self.conns.insert(conn_id, handle); - let r = self.engine.encode_state_as_update(&self.doc, None); - let _ = reply.send(r); + let _ = reply.send(JoinState::of(self.engine.as_ref(), &self.doc)); } #[tracing::instrument(skip(self, m), fields(doc_id = %self.doc_id, bytes = m.bytes.len()))] async fn on_inbound(&mut self, m: InMsg) { - if let Err(e) = self.engine.apply_update(&self.doc, &m.bytes) { - tracing::debug!(error=?e, "apply_update failed"); - return; + match self.engine.apply_update_changed(&self.doc, &m.bytes) { + Ok(true) => {} + // Nothing new — typically the SyncStep2 a (re)connecting writer + // sends in answer to the server's SyncStep1, which re-sends the + // whole delete set even when the client has no offline edits. + // Persisting it would write a `doc_updates` row on every page + // open (and attribute an edit to someone who only looked); + // fanning it out would make every peer re-apply a no-op. + Ok(false) => return, + Err(e) => { + tracing::debug!(error=?e, "apply_update failed"); + return; + } } metrics::counter!("knot_room_updates_total", "source" => "local").increment(1); let framed = wrap_sync_update(&m.bytes); @@ -586,7 +622,7 @@ impl Room { .persist_tx .send(crate::writer::PersistJob { bytes: m.bytes, - by_user_id: None, + by_user_id: m.by_user, // Inbound WS updates are fire-and-forget — the client // already has the update locally, so we don't need to // hold up acknowledgement on the durable insert. @@ -658,8 +694,7 @@ mod tests { async fn insert_batch( &self, _: Uuid, - _: Option, - updates: &[Vec], + updates: &[(Option, Vec)], ) -> Result, knot_storage::UpdatesStoreError> { Ok((1..=updates.len() as i64).collect()) } @@ -682,7 +717,7 @@ mod tests { } } - /// Records the `by_user` argument each persist passes through, so + /// Records the author of every row the writer persists, in order, so /// tests can assert attribution reaches the store. struct RecordingUpdates { seen: std::sync::Arc>>>, @@ -692,11 +727,47 @@ mod tests { async fn insert_batch( &self, _: Uuid, - by_user: Option, - updates: &[Vec], + updates: &[(Option, Vec)], ) -> Result, knot_storage::UpdatesStoreError> { - self.seen.lock().unwrap().push(by_user); - Ok((1..=updates.len() as i64).collect()) + let mut seen = self.seen.lock().unwrap(); + let first = seen.len() as i64 + 1; + seen.extend(updates.iter().map(|(by, _)| *by)); + Ok((first..first + updates.len() as i64).collect()) + } + async fn since( + &self, + _: Uuid, + _: i64, + ) -> Result, knot_storage::UpdatesStoreError> { + Ok(vec![]) + } + async fn max_seq(&self, _: Uuid) -> Result { + Ok(0) + } + async fn delete_up_to( + &self, + _: Uuid, + _: i64, + ) -> Result { + Ok(0) + } + } + + /// Records every update the writer persists, in order. + struct CapturingUpdates { + persisted: std::sync::Arc>>>, + } + #[async_trait::async_trait] + impl knot_storage::UpdatesStore for CapturingUpdates { + async fn insert_batch( + &self, + _: Uuid, + updates: &[(Option, Vec)], + ) -> Result, knot_storage::UpdatesStoreError> { + let mut p = self.persisted.lock().unwrap(); + let first = p.len() as i64 + 1; + p.extend(updates.iter().map(|(_, b)| b.clone())); + Ok((first..first + updates.len() as i64).collect()) } async fn since( &self, @@ -969,7 +1040,7 @@ mod tests { }) .await .unwrap(); - let peer_state = join_rx.await.unwrap().unwrap(); + let peer_state = join_rx.await.unwrap().unwrap().update; let peer = engine.new_doc(); engine.apply_update(&peer, &peer_state).unwrap(); @@ -1115,11 +1186,19 @@ mod tests { .await .unwrap(); - // Produce an actual yrs update from a separate doc. - let other = engine.new_doc(); - // Force a tiny state change so encode_state_as_update returns non-empty bytes. - // (yrs always returns something even for an empty doc — this is enough.) - let real_update = engine.encode_state_as_update(&other, None).unwrap(); + // Produce an actual yrs update from a separate doc. It must change + // something: the room drops updates that leave its doc unchanged, so + // the bytes of an empty doc would never reach the writer. + let real_update = { + use yrs::{ReadTxn, Text, Transact}; + let other = yrs::Doc::new(); + other + .get_or_insert_text("t") + .push(&mut other.transact_mut(), "hello"); + other + .transact() + .encode_state_as_update_v1(&yrs::StateVector::default()) + }; // Join + send. let conn_id = Uuid::new_v4(); @@ -1135,6 +1214,7 @@ mod tests { let _ = reply_rx.await.unwrap().unwrap(); h.tx.send(Event::Inbound(InMsg { from: conn_id, + by_user: None, bytes: real_update.clone(), })) .await @@ -1187,7 +1267,7 @@ mod tests { let seed_bytes = engine.encode_state_as_update(&seed_doc, None).unwrap(); let updates = PgUpdatesStore::new(pool.clone()); updates - .insert_batch(d.id, Some(u.id), std::slice::from_ref(&seed_bytes)) + .insert_batch(d.id, &[(Some(u.id), seed_bytes)]) .await .unwrap(); @@ -1222,7 +1302,7 @@ mod tests { }) .await .unwrap(); - let state = reply_rx.await.unwrap().unwrap(); + let state = reply_rx.await.unwrap().unwrap().update; assert!( !state.is_empty(), "hydrated state should include the seed update" @@ -1277,4 +1357,262 @@ mod tests { "replace should persist with its author, got {recorded:?}" ); } + + /// Once the server asks every joining writer for its state (SyncStep1), + /// most replies carry nothing new — Yjs always includes the whole delete + /// set. Such a no-op must not be persisted (a `doc_updates` row per page + /// open, and the opener would count as a contributor), fanned out to + /// peers, or counted as an update. + #[tokio::test(flavor = "multi_thread")] + async fn noop_inbound_update_is_neither_fanned_out_nor_persisted() { + use yrs::{ReadTxn, Text, Transact}; + + let bus = Arc::new(MemBus::new()); + let doc_id = Uuid::new_v4(); + let sub = bus.subscribe(doc_id).await.unwrap(); + let persisted = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let updates: Arc = Arc::new(CapturingUpdates { + persisted: persisted.clone(), + }); + let snapshots: Arc = Arc::new(NoopSnapshots); + let policy = crate::snapshot::SnapshotPolicy { + every_n: 1000, + idle: std::time::Duration::from_secs(60), + }; + let h = Room::spawn( + doc_id, + Arc::new(YrsEngine), + bus, + sub, + updates, + snapshots, + policy, + None, + ) + .await + .unwrap(); + + // A client's history, including a deletion so the no-op carries a + // non-empty delete set like a real SyncStep2 reply does. + let client = yrs::Doc::new(); + let text = client.get_or_insert_text("t"); + text.push(&mut client.transact_mut(), "hellox"); + text.remove_range(&mut client.transact_mut(), 5, 1); + let real = client + .transact() + .encode_state_as_update_v1(&yrs::StateVector::default()); + let sv_before_sentinel = client.transact().state_vector(); + text.push(&mut client.transact_mut(), "!"); + let sentinel = client + .transact() + .encode_state_as_update_v1(&sv_before_sentinel); + + let sender = Uuid::new_v4(); + let (sender_tx, _sender_rx) = mpsc::channel(8); + let (peer_tx, mut peer_rx) = mpsc::channel(8); + for (conn_id, tx) in [(sender, sender_tx), (Uuid::new_v4(), peer_tx)] { + let (reply_tx, reply_rx) = oneshot::channel(); + h.tx.send(Event::Join { + conn_id, + handle: ConnHandle { tx }, + reply: reply_tx, + }) + .await + .unwrap(); + reply_rx.await.unwrap().unwrap(); + } + + // Real edit, then the identical bytes again (the no-op), then a + // second real edit. The actor handles them strictly in order. + for bytes in [real.clone(), real.clone(), sentinel.clone()] { + h.tx.send(Event::Inbound(InMsg { + from: sender, + by_user: None, + bytes, + })) + .await + .unwrap(); + } + + let mut fanned = Vec::new(); + for _ in 0..2 { + fanned.push( + tokio::time::timeout(std::time::Duration::from_secs(10), peer_rx.recv()) + .await + .expect("peer got no fan-out") + .expect("peer channel closed"), + ); + } + let (first, second) = (&fanned[0], &fanned[1]); + assert_eq!(*first, wrap_sync_update(&real)); + assert_eq!( + *second, + wrap_sync_update(&sentinel), + "the peer was sent the no-op instead of the next real edit" + ); + + // The writer persists in arrival order: once the sentinel is in, a + // persisted no-op would be too. + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while !persisted.lock().unwrap().contains(&sentinel) { + assert!( + std::time::Instant::now() < deadline, + "writer never persisted the sentinel" + ); + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + assert_eq!( + *persisted.lock().unwrap(), + vec![real, sentinel], + "the no-op update was persisted" + ); + + h.shutdown.cancel(); + } + + /// The flip side: content the room cannot integrate yet (a dependency is + /// missing) is parked as pending by yrs. It must still be persisted and + /// fanned out — dropping it would lose the edit once the gap is filled. + #[tokio::test(flavor = "multi_thread")] + async fn inbound_update_with_missing_dependency_is_still_persisted() { + use yrs::{ReadTxn, Text, Transact}; + + let bus = Arc::new(MemBus::new()); + let doc_id = Uuid::new_v4(); + let sub = bus.subscribe(doc_id).await.unwrap(); + let persisted = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let updates: Arc = Arc::new(CapturingUpdates { + persisted: persisted.clone(), + }); + let snapshots: Arc = Arc::new(NoopSnapshots); + let policy = crate::snapshot::SnapshotPolicy { + every_n: 1000, + idle: std::time::Duration::from_secs(60), + }; + let h = Room::spawn( + doc_id, + Arc::new(YrsEngine), + bus, + sub, + updates, + snapshots, + policy, + None, + ) + .await + .unwrap(); + + let client = yrs::Doc::new(); + let text = client.get_or_insert_text("t"); + text.push(&mut client.transact_mut(), "a"); + let sv = client.transact().state_vector(); + text.push(&mut client.transact_mut(), "b"); + let dependent = client.transact().encode_state_as_update_v1(&sv); + + let (peer_tx, mut peer_rx) = mpsc::channel(8); + let (reply_tx, reply_rx) = oneshot::channel(); + h.tx.send(Event::Join { + conn_id: Uuid::new_v4(), + handle: ConnHandle { tx: peer_tx }, + reply: reply_tx, + }) + .await + .unwrap(); + reply_rx.await.unwrap().unwrap(); + + h.tx.send(Event::Inbound(InMsg { + from: Uuid::new_v4(), + by_user: None, + bytes: dependent.clone(), + })) + .await + .unwrap(); + + let fanned = tokio::time::timeout(std::time::Duration::from_secs(10), peer_rx.recv()) + .await + .expect("pending update was not fanned out") + .unwrap(); + assert_eq!(fanned, wrap_sync_update(&dependent)); + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while persisted.lock().unwrap().is_empty() { + assert!( + std::time::Instant::now() < deadline, + "pending update was never persisted" + ); + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + assert_eq!(*persisted.lock().unwrap(), vec![dependent]); + + h.shutdown.cancel(); + } + + /// Live edits carry the authenticated user who typed them, row by row. + /// The writer batches whatever arrives within 250 ms, so two people + /// typing at once share a flush; each row must still be stored under + /// its own author — not the batch's first — or one of them would be + /// credited with the other's work (and the other with nothing). + #[tokio::test(flavor = "multi_thread")] + async fn inbound_updates_persist_under_each_senders_own_author() { + use yrs::{ReadTxn, Text, Transact}; + + let bus = Arc::new(MemBus::new()); + let doc_id = Uuid::new_v4(); + let sub = bus.subscribe(doc_id).await.unwrap(); + let seen = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let updates: Arc = + Arc::new(RecordingUpdates { seen: seen.clone() }); + let snapshots: Arc = Arc::new(NoopSnapshots); + let policy = crate::snapshot::SnapshotPolicy { + every_n: 1000, + idle: std::time::Duration::from_secs(60), + }; + let h = Room::spawn( + doc_id, + Arc::new(YrsEngine), + bus, + sub, + updates, + snapshots, + policy, + None, + ) + .await + .unwrap(); + + // Two independent clients, each with one real edit. + let edit = |s: &str| { + let d = yrs::Doc::new(); + d.get_or_insert_text("t").push(&mut d.transact_mut(), s); + d.transact() + .encode_state_as_update_v1(&yrs::StateVector::default()) + }; + let (alice, bob) = (Uuid::new_v4(), Uuid::new_v4()); + // Sent back-to-back: both reach the writer well inside one batch + // window, which is exactly the case a per-batch author gets wrong. + for (by, bytes) in [(alice, edit("a")), (bob, edit("b"))] { + h.tx.send(Event::Inbound(InMsg { + from: Uuid::new_v4(), + by_user: Some(by), + bytes, + })) + .await + .unwrap(); + } + + let deadline = std::time::Instant::now() + std::time::Duration::from_secs(10); + while seen.lock().unwrap().len() < 2 { + assert!( + std::time::Instant::now() < deadline, + "writer never persisted both edits" + ); + tokio::time::sleep(std::time::Duration::from_millis(20)).await; + } + assert_eq!( + *seen.lock().unwrap(), + vec![Some(alice), Some(bob)], + "each live edit must be persisted under its sender" + ); + + h.shutdown.cancel(); + } } diff --git a/crates/knot-crdt/src/writer.rs b/crates/knot-crdt/src/writer.rs index 2d5e9b2..7e46b3c 100644 --- a/crates/knot-crdt/src/writer.rs +++ b/crates/knot-crdt/src/writer.rs @@ -88,9 +88,13 @@ async fn flush( applied_tx: &mpsc::Sender, buf: &mut Vec, ) { - let by_user = buf.first().and_then(|j| j.by_user_id); - let updates: Vec> = buf.iter().map(|j| j.bytes.clone()).collect(); - match store.insert_batch(doc_id, by_user, &updates).await { + // Each row keeps its own author: a batch is just whatever arrived within + // BATCH_INTERVAL, so it routinely mixes several people's live edits. + let updates: Vec<(Option, Vec)> = buf + .iter() + .map(|j| (j.by_user_id, j.bytes.clone())) + .collect(); + match store.insert_batch(doc_id, &updates).await { Ok(seqs) => { for (seq, job) in seqs.into_iter().zip(buf.drain(..)) { if bus.publish(doc_id, seq).await.is_err() { diff --git a/crates/knot-server/src/board_room_shim.rs b/crates/knot-server/src/board_room_shim.rs index 5447929..2c5e8f2 100644 --- a/crates/knot-server/src/board_room_shim.rs +++ b/crates/knot-server/src/board_room_shim.rs @@ -9,7 +9,7 @@ use tokio::sync::{mpsc, oneshot}; use tokio_util::sync::CancellationToken; use uuid::Uuid; -use crate::protocol::{YSyncMessage, decode, encode_sync_step2}; +use crate::protocol::{YSyncMessage, decode, encode_sync_step1, encode_sync_step2}; pub async fn serve( rooms: Arc, @@ -40,11 +40,17 @@ pub async fn serve( { return; } - let initial = match reply_rx.await { - Ok(Ok(b)) => encode_sync_step2(&b), + let joined = match reply_rx.await { + Ok(Ok(j)) => j, _ => return, }; - let _ = out_tx.send(initial).await; + let _ = out_tx.send(encode_sync_step2(&joined.update)).await; + // Ask the client for what we lack, exactly as `room::serve` does: the + // board provider has the same offline gap (edits made while the socket is + // down are applied locally but never sent) and the same SyncStep1 answer. + // Sent as its own WS message — BoardProvider decodes one y-protocol + // message per frame. + let _ = out_tx.send(encode_sync_step1(&joined.state_vector)).await; let (mut sink, mut stream) = socket.split(); let writer_shutdown = shutdown.clone(); @@ -101,8 +107,8 @@ pub async fn serve( reply: rtx, }) .await; - if let Ok(Ok(state)) = rrx.await { - let _ = out_tx.send(encode_sync_step2(&state)).await; + if let Ok(Ok(joined)) = rrx.await { + let _ = out_tx.send(encode_sync_step2(&joined.update)).await; } } Ok(YSyncMessage::SyncStep2(inner)) | Ok(YSyncMessage::Update(inner)) => { diff --git a/crates/knot-server/src/lib.rs b/crates/knot-server/src/lib.rs index 1aa33d6..f7cd0f7 100644 --- a/crates/knot-server/src/lib.rs +++ b/crates/knot-server/src/lib.rs @@ -59,6 +59,7 @@ pub struct AppState { pub boards: Option>, pub board_rooms: Option>, pub tasks: Option>, + pub contributors: Option>, pub hasher: Arc, pub throttle: Arc, pub session_key: Vec, @@ -95,6 +96,7 @@ impl AppState { boards: None, board_rooms: None, tasks: None, + contributors: None, hasher: Arc::new(Hasher::new()), throttle: Arc::new(Throttle::new()), session_key: Vec::new(), @@ -136,6 +138,8 @@ impl AppState { Arc::new(knot_storage::PgBoardStore::new(pool.clone())); let tasks: Arc = Arc::new(knot_storage::PgTaskStore::new(pool.clone())); + let contributors: Arc = + Arc::new(knot_storage::PgContributorStore::new(pool.clone())); Self { pool: Some(pool), users: Some(users), @@ -156,6 +160,7 @@ impl AppState { boards: Some(boards), board_rooms: None, tasks: Some(tasks), + contributors: Some(contributors), hasher: Arc::new(Hasher::new()), throttle: Arc::new(Throttle::new()), session_key: Vec::new(), @@ -266,7 +271,7 @@ async fn collab_upgrade( .max_message_size(MAX_WS_MESSAGE_BYTES) .max_frame_size(MAX_WS_MESSAGE_BYTES); ws.on_upgrade(move |socket| async move { - crate::room::serve(rooms, doc_id, socket, can_write, shutdown).await; + crate::room::serve(rooms, doc_id, ctx.user_id, socket, can_write, shutdown).await; }) .into_response() } diff --git a/crates/knot-server/src/protocol.rs b/crates/knot-server/src/protocol.rs index f2cf967..65dc3f8 100644 --- a/crates/knot-server/src/protocol.rs +++ b/crates/knot-server/src/protocol.rs @@ -57,6 +57,13 @@ pub fn decode(buf: &[u8]) -> Result { } } +/// Server→client SyncStep1: "here is my state vector, send me what I lack". +/// Sent once on join so a reconnecting client uploads edits it made while +/// offline (see `room::serve`). +pub fn encode_sync_step1(state_vector: &[u8]) -> Vec { + encode_sync(SYNC_STEP_1, state_vector) +} + pub fn encode_sync_step2(payload: &[u8]) -> Vec { encode_sync(SYNC_STEP_2, payload) } @@ -150,4 +157,15 @@ mod tests { other => panic!("expected SyncStep2, got {other:?}"), } } + + #[test] + fn roundtrip_sync_step1() { + let sv = vec![0x01, 0x02, 0x03]; + let encoded = encode_sync_step1(&sv); + assert_eq!(&encoded[..3], &[MSG_SYNC, SYNC_STEP_1, 3]); + match decode(&encoded).expect("decode") { + YSyncMessage::SyncStep1(p) => assert_eq!(p, sv), + other => panic!("expected SyncStep1, got {other:?}"), + } + } } diff --git a/crates/knot-server/src/room.rs b/crates/knot-server/src/room.rs index 3f264f7..5bfa40c 100644 --- a/crates/knot-server/src/room.rs +++ b/crates/knot-server/src/room.rs @@ -5,6 +5,10 @@ //! `can_write` is the effective-role gate decided at upgrade: Viewers may //! connect and hydrate (read), but their inbound CRDT updates are dropped so //! a read-only grant cannot mutate the document over the socket. +//! +//! `user_id` is the authenticated user behind the socket. Every update the +//! connection forwards carries it, so the room persists live edits under +//! their author and credits that user as a contributor. use axum::extract::ws::{CloseFrame, Message, WebSocket}; use futures::{SinkExt, StreamExt}; @@ -14,11 +18,12 @@ use tokio::sync::{mpsc, oneshot}; use tokio_util::sync::CancellationToken; use uuid::Uuid; -use crate::protocol::{YSyncMessage, decode, encode_sync_step2}; +use crate::protocol::{YSyncMessage, decode, encode_sync_step1, encode_sync_step2}; pub async fn serve( rooms: Arc, doc_id: Uuid, + user_id: Uuid, socket: WebSocket, can_write: bool, shutdown: CancellationToken, @@ -48,11 +53,22 @@ pub async fn serve( { return; } - let initial = match reply_rx.await { - Ok(Ok(b)) => encode_sync_step2(&b), + let joined = match reply_rx.await { + Ok(Ok(j)) => j, _ => return, }; - let _ = out_tx.send(initial).await; + let _ = out_tx.send(encode_sync_step2(&joined.update)).await; + // Ask the client for what WE lack (standard y-protocol: both sides send + // SyncStep1). The web provider applies edits made while its socket was + // down to its local doc without queueing them, and its own SyncStep1 only + // fetches what the client lacks — so without this ask, offline edits never + // reach the server, and every later edit that builds on them is parked as + // pending by yrs and stays invisible to everyone else. The provider + // already answers a server SyncStep1 with + // `encodeStateAsUpdate(doc, serverSV)`, so already-deployed bundles are + // fixed by this alone. Viewers answer too; their reply is dropped below + // like any other inbound update from a read-only connection. + let _ = out_tx.send(encode_sync_step1(&joined.state_vector)).await; // Server-side share of "open a doc, wait for content": includes the // hydrate of a cold room (see knot_room_hydrate_seconds) and the wait for // the room actor's reply. @@ -119,8 +135,8 @@ pub async fn serve( reply: rtx, }) .await; - if let Ok(Ok(state)) = rrx.await { - let _ = out_tx.send(encode_sync_step2(&state)).await; + if let Ok(Ok(joined)) = rrx.await { + let _ = out_tx.send(encode_sync_step2(&joined.update)).await; } } Ok(YSyncMessage::SyncStep2(inner)) | Ok(YSyncMessage::Update(inner)) => { @@ -131,6 +147,7 @@ pub async fn serve( .tx .send(Event::Inbound(InMsg { from: conn_id, + by_user: Some(user_id), bytes: inner, })) .await; diff --git a/crates/knot-server/src/routes/api/contributors.rs b/crates/knot-server/src/routes/api/contributors.rs new file mode 100644 index 0000000..dd29e88 --- /dev/null +++ b/crates/knot-server/src/routes/api/contributors.rs @@ -0,0 +1,64 @@ +//! Doc byline: +//! GET /api/docs/{doc_id}/contributors → 200 {created_by, created_at, +//! contributors_since, contributors: [{user_id, display_name, +//! first_edited_at, last_edited_at}]} +//! +//! Readable by every role that can open the doc — the byline sits under the +//! title for viewers too. A contributor is anyone whose change to the page +//! CONTENT was persisted (see `knot_storage::contributors`); renames, moves, +//! permission changes and comments do not count. Names only, never emails. + +use axum::{ + Json, + extract::{Path, Request, State}, + http::StatusCode, + response::{IntoResponse, Response}, +}; +use uuid::Uuid; + +use crate::AppState; +use crate::auth::{AuthContext, EffectiveDocRole}; +use crate::http_error::json_err; + +/// Any effective role (Viewer and up). Expects `EffectiveDocRole` already +/// set by `require_doc_role_mw`; same responses as comments' `require_viewer`. +fn require_viewer(req: &Request) -> Option { + if req.extensions().get::().is_none() { + return Some(json_err( + StatusCode::UNAUTHORIZED, + "auth.session_required", + "", + )); + } + if req.extensions().get::().is_none() { + return Some(json_err(StatusCode::FORBIDDEN, "acl.no_grant", "")); + } + None +} + +pub async fn byline( + State(state): State, + Path(doc_id): Path, + req: Request, +) -> Response { + if let Some(r) = require_viewer(&req) { + return r; + } + let Some(contributors) = state.contributors.clone() else { + return internal(); + }; + match contributors.byline(doc_id).await { + Ok(Some(b)) => Json(b).into_response(), + // The ACL layer already resolved the doc, so this is a hard delete + // racing the request. + Ok(None) => json_err(StatusCode::NOT_FOUND, "doc.not_found", ""), + Err(e) => { + tracing::error!(error=?e, %doc_id, "contributors byline"); + internal() + } + } +} + +fn internal() -> Response { + json_err(StatusCode::INTERNAL_SERVER_ERROR, "internal", "") +} diff --git a/crates/knot-server/src/routes/api/docs.rs b/crates/knot-server/src/routes/api/docs.rs index 44a85c0..2cf9483 100644 --- a/crates/knot-server/src/routes/api/docs.rs +++ b/crates/knot-server/src/routes/api/docs.rs @@ -82,6 +82,10 @@ pub fn router(state: AppState) -> Router { post(crate::routes::api::history::restore), ) .merge(crate::routes::api::comments::routes()) + .route( + "/api/docs/{id}/contributors", + get(crate::routes::api::contributors::byline), + ) .route("/api/docs/{id}/template", post(set_template_inline)) .route( "/api/docs/from-template/{id}", diff --git a/crates/knot-server/src/routes/api/export_import.rs b/crates/knot-server/src/routes/api/export_import.rs index faf9257..46409b2 100644 --- a/crates/knot-server/src/routes/api/export_import.rs +++ b/crates/knot-server/src/routes/api/export_import.rs @@ -679,7 +679,8 @@ async fn import( .tx .send(knot_crdt::Event::ReplaceWithMarkdown { update_bytes, - by_user: None, + // The importer put this content into the new page. + by_user: Some(ctx.user_id), reply: tx, }) .await; diff --git a/crates/knot-server/src/routes/api/history.rs b/crates/knot-server/src/routes/api/history.rs index f9b31ee..3986256 100644 --- a/crates/knot-server/src/routes/api/history.rs +++ b/crates/knot-server/src/routes/api/history.rs @@ -112,6 +112,10 @@ pub async fn restore( if let Some(r) = require_editor(&req) { return r; } + // Present: require_editor rejected the request otherwise. + let Some(ctx) = req.extensions().get::().cloned() else { + return json_err(StatusCode::UNAUTHORIZED, "auth.session_required", ""); + }; let Some(snapshots) = state.snapshots.clone() else { return internal(); }; @@ -172,7 +176,9 @@ pub async fn restore( .tx .send(knot_crdt::Event::ReplaceWithMarkdown { update_bytes, - by_user: None, + // The restorer rewrote the page, so they are its latest + // contributor — not the authors of the snapshot's content. + by_user: Some(ctx.user_id), reply: tx, }) .await diff --git a/crates/knot-server/src/routes/api/mod.rs b/crates/knot-server/src/routes/api/mod.rs index f47f1dd..b8cb55c 100644 --- a/crates/knot-server/src/routes/api/mod.rs +++ b/crates/knot-server/src/routes/api/mod.rs @@ -8,6 +8,7 @@ use crate::auth::{csrf_mw, require_session_mw}; pub mod blobs; pub mod boards; pub mod comments; +pub mod contributors; pub mod docs; pub mod export_import; pub mod grants; diff --git a/crates/knot-server/tests/contributors_integration.rs b/crates/knot-server/tests/contributors_integration.rs new file mode 100644 index 0000000..3512441 --- /dev/null +++ b/crates/knot-server/tests/contributors_integration.rs @@ -0,0 +1,964 @@ +//! Doc contributors: who gets credited for changing a page's content, and +//! `GET /api/docs/{id}/contributors`. +//! +//! A contributor is anyone whose action changed the page CONTENT — live +//! typing over the collab socket, markdown import, history restore, +//! workspace import, … Every one of those is persisted by the room's writer, +//! which records the author per `doc_updates` row and upserts +//! `doc_contributors` in the same statement. +//! +//! The live-editing tests drive the real collab socket with a raw y-sync +//! client that behaves like the web provider (see `offline_sync.rs` for the +//! full version and the rationale behind each step). + +use std::io::Write; +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use axum::body::Body; +use axum::http::{Request, StatusCode}; +use chrono::{DateTime, TimeZone, Utc}; +use futures_util::{SinkExt, StreamExt}; +use http_body_util::BodyExt; +use knot_auth::{Hasher, Throttle}; +use knot_server::protocol::{YSyncMessage, decode}; +use knot_server::{AppState, router_with_state}; +use knot_storage::{ + PgSnapshotStore, PgUpdatesStore, SnapshotStore, UpdatesStore, WorkspaceRole, sort_key_between, +}; +use tokio::net::TcpListener; +use tokio::time::Instant; +use tokio_tungstenite::tungstenite::{self, client::IntoClientRequest}; +use tower::ServiceExt; +use uuid::Uuid; +use yrs::updates::decoder::Decode; +use yrs::updates::encoder::Encode; +use yrs::{ + Doc, GetString, ReadTxn, StateVector, Text, Transact, TransactionMut, Update, XmlElementPrelim, + XmlFragment, XmlFragmentRef, XmlTextPrelim, XmlTextRef, +}; + +type Ws = + tokio_tungstenite::WebSocketStream>; + +/// Generous: a loaded CI runner can take a while to hydrate a room and run +/// the writer's 250 ms batch. Every wait polls toward this deadline and +/// returns as soon as its condition holds — no fixed sleeps. +const DEADLINE: Duration = Duration::from_secs(10); +const PASSWORD: &str = "hunter22"; + +// --------------------------------------------------------------------------- +// Fixture +// --------------------------------------------------------------------------- + +struct Session { + /// `sid=…; csrf=…` + cookie: String, + csrf: String, +} + +struct Member { + id: Uuid, + session: Session, +} + +struct Fixture { + addr: std::net::SocketAddr, + app: axum::Router, + state: AppState, + pool: knot_storage::Pool, + ws_id: Uuid, + doc_id: Uuid, + doc_created_at: DateTime, + /// Owner; created the doc. + alice: Member, + /// Editor. + bob: Member, + /// Editor. + carol: Member, + /// Viewer. + vic: Member, +} + +/// Postgres-backed state with doc rooms on a MemBus, one doc created by +/// Alice, and four logged-in members, served on an ephemeral port. +async fn fixture() -> Fixture { + let db = knot_test_support::fresh_db().await; + let pool = db.pool.clone(); + + let mut s = AppState::with_pool(pool.clone()); + s.hasher = Arc::new(Hasher::fast_for_tests()); + s.throttle = Arc::new(Throttle::new()); + s.session_key = b"test-key-32-bytes-aaaaaaaaaaaaaa".to_vec(); + s.cookie_secure = false; + + let updates: Arc = Arc::new(PgUpdatesStore::new(pool.clone())); + let snapshots: Arc = Arc::new(PgSnapshotStore::new(pool.clone())); + s.rooms_v2 = Some(Arc::new(knot_crdt::Rooms::new( + Arc::new(knot_crdt::YrsEngine), + Arc::new(knot_crdt::MemBus::new()), + updates, + snapshots, + knot_crdt::SnapshotPolicy { + every_n: 1000, + idle: Duration::from_secs(600), + }, + Duration::from_secs(300), + ))); + + let ws = s + .workspaces + .as_ref() + .unwrap() + .create("default", "W") + .await + .unwrap(); + let mut ids = Vec::new(); + for (email, name, role) in [ + ("alice@contrib.test", "Alice", WorkspaceRole::Owner), + ("bob@contrib.test", "Bob", WorkspaceRole::Editor), + ("carol@contrib.test", "Carol", WorkspaceRole::Editor), + ("vic@contrib.test", "Vic", WorkspaceRole::Viewer), + ] { + let hash = s.hasher.hash(PASSWORD).unwrap(); + let u = s + .users + .as_ref() + .unwrap() + .create_local(email, name, &hash) + .await + .unwrap(); + s.workspaces + .as_ref() + .unwrap() + .add_member(ws.id, u.id, role) + .await + .unwrap(); + ids.push((u.id, email)); + } + let doc = s + .docs + .as_ref() + .unwrap() + .create( + ws.id, + None, + "Credits", + &sort_key_between(None, None), + ids[0].0, + ) + .await + .unwrap(); + + let app = router_with_state(s.clone()); + let mut members = Vec::new(); + for (id, email) in ids { + members.push(Member { + id, + session: login(&app, email).await, + }); + } + let [alice, bob, carol, vic]: [Member; 4] = members.try_into().ok().unwrap(); + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let served = app.clone(); + tokio::spawn(async move { + axum::serve(listener, served).await.unwrap(); + }); + + Fixture { + addr, + app, + state: s, + pool, + ws_id: ws.id, + doc_id: doc.id, + doc_created_at: doc.created_at, + alice, + bob, + carol, + vic, + } +} + +async fn login(app: &axum::Router, email: &str) -> Session { + let r = app + .clone() + .oneshot( + Request::builder() + .method("POST") + .uri("/auth/login") + .header("content-type", "application/json") + .body(Body::from( + serde_json::json!({"email": email, "password": PASSWORD}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(); + assert_eq!(r.status(), StatusCode::NO_CONTENT, "login {email} failed"); + let kvs: Vec = r + .headers() + .get_all("set-cookie") + .iter() + .map(|v| v.to_str().unwrap().split(';').next().unwrap().to_string()) + .filter(|kv| kv.starts_with("sid=") || kv.starts_with("csrf=")) + .collect(); + let csrf = kvs + .iter() + .find_map(|kv| kv.strip_prefix("csrf=")) + .expect("login set no csrf cookie") + .to_string(); + Session { + cookie: kvs.join("; "), + csrf, + } +} + +// --------------------------------------------------------------------------- +// HTTP helpers +// --------------------------------------------------------------------------- + +async fn get_contributors( + app: &axum::Router, + session: Option<&Session>, + doc_id: Uuid, +) -> (StatusCode, Option, String) { + let mut req = Request::builder() + .method("GET") + .uri(format!("/api/docs/{doc_id}/contributors")); + if let Some(s) = session { + req = req.header("cookie", &s.cookie); + } + let r = app + .clone() + .oneshot(req.body(Body::empty()).unwrap()) + .await + .unwrap(); + let status = r.status(); + let content_type = r + .headers() + .get("content-type") + .map(|v| v.to_str().unwrap().to_string()); + let bytes = r.into_body().collect().await.unwrap().to_bytes(); + ( + status, + content_type, + String::from_utf8(bytes.to_vec()).unwrap(), + ) +} + +async fn post( + app: &axum::Router, + session: &Session, + uri: &str, + content_type: &str, + body: Vec, +) -> (StatusCode, String) { + let r = app + .clone() + .oneshot( + Request::builder() + .method("POST") + .uri(uri) + .header("cookie", &session.cookie) + .header("x-csrf-token", &session.csrf) + .header("content-type", content_type) + .body(Body::from(body)) + .unwrap(), + ) + .await + .unwrap(); + let status = r.status(); + let bytes = r.into_body().collect().await.unwrap().to_bytes(); + (status, String::from_utf8_lossy(&bytes).into_owned()) +} + +/// The contributors' user ids, in the order the endpoint lists them. +fn listed(byline: &serde_json::Value) -> Vec { + byline["contributors"] + .as_array() + .expect("contributors array") + .iter() + .map(|c| c["user_id"].as_str().unwrap().parse().unwrap()) + .collect() +} + +/// Poll the endpoint (as Alice) until `done` holds for the byline, or panic. +async fn wait_for_byline( + fx: &Fixture, + doc_id: Uuid, + what: &str, + done: impl Fn(&[Uuid]) -> bool, +) -> serde_json::Value { + let deadline = Instant::now() + DEADLINE; + loop { + let (status, _, body) = get_contributors(&fx.app, Some(&fx.alice.session), doc_id).await; + assert_eq!(status, StatusCode::OK, "contributors: {body}"); + let byline: serde_json::Value = serde_json::from_str(&body).unwrap(); + if done(&listed(&byline)) { + return byline; + } + if Instant::now() >= deadline { + panic!("{what}: contributors never matched; last byline {byline}"); + } + // Storage is polled, not pushed: back off briefly between requests. + // Bounded by DEADLINE above. + tokio::time::sleep(Duration::from_millis(50)).await; + } +} + +// --------------------------------------------------------------------------- +// Storage helpers +// --------------------------------------------------------------------------- + +/// Every `doc_updates` row for the doc, in seq order. +async fn update_rows(pool: &knot_storage::Pool, doc_id: Uuid) -> Vec { + PgUpdatesStore::new(pool.clone()) + .since(doc_id, 0) + .await + .unwrap() +} + +fn doc_text(doc: &Doc) -> String { + let frag = doc.get_or_insert_xml_fragment("default"); + frag.get_string(&doc.transact()) +} + +/// Poll storage until a cold hydration of the doc contains `needle`. The +/// writer persists in arrival order, so once a sentinel edit is durable, +/// anything a connection sent before it has been persisted (or dropped). +async fn wait_for_durable_text(pool: &knot_storage::Pool, doc_id: Uuid, needle: &str) { + let deadline = Instant::now() + DEADLINE; + loop { + let doc = Doc::new(); + let mut after = 0; + if let Some(snap) = PgSnapshotStore::new(pool.clone()) + .latest(doc_id) + .await + .unwrap() + { + let u = Update::decode_v1(&snap.state_bytes).unwrap(); + doc.transact_mut().apply_update(u).unwrap(); + after = snap.snapshot_seq; + } + for r in PgUpdatesStore::new(pool.clone()) + .since(doc_id, after) + .await + .unwrap() + { + let u = Update::decode_v1(&r.update_bytes).unwrap(); + doc.transact_mut().apply_update(u).unwrap(); + } + let text = doc_text(&doc); + if text.contains(needle) { + return; + } + if Instant::now() >= deadline { + panic!("{needle:?} never became durable; storage holds {text:?}"); + } + tokio::time::sleep(Duration::from_millis(50)).await; + } +} + +// --------------------------------------------------------------------------- +// Raw y-sync client (slimmed-down `offline_sync::Peer`) +// --------------------------------------------------------------------------- + +fn sync_frame(subtype: u8, payload: &[u8]) -> Vec { + let mut out = vec![0u8, subtype]; + knot_server::protocol::append_var_uint(&mut out, payload.len() as u64); + out.extend_from_slice(payload); + out +} + +struct Peer { + doc: Doc, + ws: Option, + /// Server SyncStep1 frames answered with a SyncStep2. + step1_answered: usize, + /// Server SyncStep2 frames applied. + step2_received: usize, +} + +impl Peer { + fn new() -> Self { + Self { + doc: Doc::new(), + ws: None, + step1_answered: 0, + step2_received: 0, + } + } + + /// Dial as `who`, send SyncStep1, and complete the handshake: apply the + /// server's state and answer its SyncStep1 exactly like the web provider + /// (for a writer that answer reaches the room; it carries nothing new). + async fn join(fx: &Fixture, who: &Member) -> Self { + let mut p = Self::new(); + let mut req = format!("ws://{}/collab/doc/{}", fx.addr, fx.doc_id) + .into_client_request() + .unwrap(); + req.headers_mut().insert( + "cookie", + tungstenite::http::HeaderValue::from_str(&who.session.cookie).unwrap(), + ); + let (mut ws, _) = tokio_tungstenite::connect_async(req) + .await + .expect("ws connect"); + let sv = p.doc.transact().state_vector().encode_v1(); + ws.send(tungstenite::Message::Binary(sync_frame(0, &sv).into())) + .await + .unwrap(); + p.ws = Some(ws); + assert!( + p.pump_until(|p| p.step2_received >= 1 && p.step1_answered >= 1) + .await, + "handshake never completed" + ); + p + } + + /// Round-trip barrier: the server answers SyncStep1 from the same read + /// loop that forwards our updates to the room actor, and the actor + /// handles both in order, so once that SyncStep2 is back every update we + /// sent before it has been applied (or dropped, for a viewer). + async fn settle(&mut self) { + let sv = self.doc.transact().state_vector().encode_v1(); + self.ws + .as_mut() + .unwrap() + .send(tungstenite::Message::Binary(sync_frame(0, &sv).into())) + .await + .unwrap(); + let seen = self.step2_received; + assert!( + self.pump_until(|p| p.step2_received > seen).await, + "server never answered the settle SyncStep1" + ); + } + + /// Settle, then drop the connection. + async fn leave(mut self) { + self.settle().await; + self.ws.take(); + } + + /// A local edit, sent as SYNC_UPDATE frames like `handleDocUpdate`. + async fn edit(&mut self, f: impl FnOnce(&XmlFragmentRef, &mut TransactionMut)) { + // Root ref BEFORE the transaction: `get_or_insert_*` takes its own + // write lock and would deadlock against an open `TransactionMut`. + let frag = self.doc.get_or_insert_xml_fragment("default"); + let captured: Arc>>> = Arc::default(); + let sink = captured.clone(); + let sub = self + .doc + .observe_update_v1(move |_, e| sink.lock().unwrap().push(e.update.clone())); + { + let mut txn = self.doc.transact_mut(); + f(&frag, &mut txn); + } + drop(sub); + let updates = std::mem::take(&mut *captured.lock().unwrap()); + let ws = self.ws.as_mut().unwrap(); + for u in updates { + ws.send(tungstenite::Message::Binary(sync_frame(2, &u).into())) + .await + .unwrap(); + } + } + + /// Append a paragraph holding `s`. + async fn type_paragraph(&mut self, s: &str) { + let s = s.to_string(); + self.edit(move |frag, txn| { + paragraph_text(frag, txn, &s); + }) + .await; + } + + async fn handle(&mut self, bytes: &[u8]) { + match decode(bytes) { + Ok(YSyncMessage::SyncStep1(sv)) => { + let sv = StateVector::decode_v1(&sv).expect("server sent a valid state vector"); + let reply = self.doc.transact().encode_state_as_update_v1(&sv); + self.ws + .as_mut() + .unwrap() + .send(tungstenite::Message::Binary(sync_frame(1, &reply).into())) + .await + .unwrap(); + self.step1_answered += 1; + } + Ok(YSyncMessage::SyncStep2(u)) => { + self.apply(&u); + self.step2_received += 1; + } + Ok(YSyncMessage::Update(u)) => self.apply(&u), + _ => {} + } + } + + fn apply(&self, update: &[u8]) { + let u = Update::decode_v1(update).expect("server sent a valid update"); + self.doc.transact_mut().apply_update(u).unwrap(); + } + + /// Process inbound frames until `done` holds or DEADLINE elapses. + async fn pump_until(&mut self, done: impl Fn(&Peer) -> bool) -> bool { + let deadline = Instant::now() + DEADLINE; + loop { + if done(self) { + return true; + } + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return false; + } + let ws = self.ws.as_mut().expect("pump on a connected peer"); + match tokio::time::timeout(remaining, ws.next()).await { + Ok(Some(Ok(tungstenite::Message::Binary(b)))) => self.handle(&b).await, + Ok(Some(Ok(_))) => {} + Ok(Some(Err(e))) => panic!("ws error: {e}"), + Ok(None) => panic!("server closed the socket"), + Err(_) => return false, + } + } + } +} + +fn paragraph_text(frag: &XmlFragmentRef, txn: &mut TransactionMut, s: &str) -> XmlTextRef { + let p = frag.push_back(txn, XmlElementPrelim::empty("paragraph")); + p.push_back(txn, XmlTextPrelim::new(s)) +} + +// --------------------------------------------------------------------------- +// Who gets credited +// --------------------------------------------------------------------------- + +/// The everyday case: an editor types over the collab socket. +#[tokio::test(flavor = "multi_thread")] +async fn live_edit_credits_the_editor() { + let fx = fixture().await; + let mut bob = Peer::join(&fx, &fx.bob).await; + bob.type_paragraph("hello").await; + + let byline = wait_for_byline(&fx, fx.doc_id, "Bob's live edit", |l| { + l.contains(&fx.bob.id) + }) + .await; + assert_eq!( + listed(&byline), + vec![fx.bob.id], + "only the editor who typed is a contributor (not the creator, who did not edit)" + ); + let rows = update_rows(&fx.pool, fx.doc_id).await; + assert!( + !rows.is_empty() && rows.iter().all(|r| r.by_user_id == Some(fx.bob.id)), + "live edits must be stored under their author: {:?}", + rows.iter().map(|r| r.by_user_id).collect::>() + ); + bob.leave().await; +} + +/// A viewer's updates are dropped at the socket (`can_write`), so they can +/// neither change the page nor become a contributor by trying. +#[tokio::test(flavor = "multi_thread")] +async fn viewer_update_over_the_socket_credits_nobody() { + let fx = fixture().await; + let mut vic = Peer::join(&fx, &fx.vic).await; + vic.type_paragraph("vandal").await; + vic.settle().await; + + // Sentinel: once Bob's later edit is durable, anything of Vic's that + // reached the writer would be too. + let mut bob = Peer::join(&fx, &fx.bob).await; + bob.type_paragraph("sentinel").await; + wait_for_durable_text(&fx.pool, fx.doc_id, "sentinel").await; + + let byline = wait_for_byline(&fx, fx.doc_id, "sentinel", |l| l.contains(&fx.bob.id)).await; + assert_eq!(listed(&byline), vec![fx.bob.id], "the viewer was credited"); + let rows = update_rows(&fx.pool, fx.doc_id).await; + assert!( + rows.iter().all(|r| r.by_user_id != Some(fx.vic.id)), + "a viewer's update was persisted" + ); + vic.leave().await; + bob.leave().await; +} + +/// Two people typing at once share the writer's 250 ms batch. Both must be +/// credited, and every row must carry the author whose client produced it +/// — not whoever happened to be first in the batch. +#[tokio::test(flavor = "multi_thread")] +async fn concurrent_editors_are_each_credited_for_their_own_rows() { + let fx = fixture().await; + let mut bob = Peer::join(&fx, &fx.bob).await; + let mut carol = Peer::join(&fx, &fx.carol).await; + + // Interleaved, back to back: these land in the same writer batch. + for i in 0..3 { + bob.type_paragraph(&format!("bob{i}")).await; + carol.type_paragraph(&format!("carol{i}")).await; + } + bob.settle().await; + carol.settle().await; + wait_for_durable_text(&fx.pool, fx.doc_id, "bob2").await; + wait_for_durable_text(&fx.pool, fx.doc_id, "carol2").await; + + let byline = wait_for_byline(&fx, fx.doc_id, "both editors", |l| { + l.contains(&fx.bob.id) && l.contains(&fx.carol.id) + }) + .await; + assert_eq!(listed(&byline).len(), 2, "byline {byline}"); + + // Each row's yrs client id says whose editor produced it. + let author_of = |client: yrs::ClientID| { + if client == bob.doc.client_id() { + fx.bob.id + } else if client == carol.doc.client_id() { + fx.carol.id + } else { + panic!("row from unknown client {client}") + } + }; + let rows = update_rows(&fx.pool, fx.doc_id).await; + assert_eq!(rows.len(), 6, "one row per edit"); + for r in &rows { + let u = Update::decode_v1(&r.update_bytes).unwrap(); + // `state_vector_lower`: every client with blocks in the update + // (`state_vector` skips those whose blocks do not start at clock 0). + let clients: Vec<_> = u.state_vector_lower().iter().map(|(c, _)| *c).collect(); + assert_eq!(clients.len(), 1, "each live edit comes from one client"); + assert_eq!( + r.by_user_id, + Some(author_of(clients[0])), + "row {} stored under the wrong author", + r.seq + ); + } + bob.leave().await; + carol.leave().await; +} + +/// Opening a page as an editor is not editing it. On join the server asks +/// for the client's state (SyncStep1) and every writer answers; that answer +/// usually carries nothing new and must not count. +#[tokio::test(flavor = "multi_thread")] +async fn editor_who_only_opens_the_page_is_not_credited() { + let fx = fixture().await; + // Seed history with a deletion, so the no-op answer carries a non-empty + // delete set — the realistic case. + let mut bob = Peer::join(&fx, &fx.bob).await; + let text: Arc>> = Arc::default(); + let slot = text.clone(); + bob.edit(move |frag, txn| { + *slot.lock().unwrap() = Some(paragraph_text(frag, txn, "seedx")); + }) + .await; + let t = text.lock().unwrap().take().unwrap(); + bob.edit(move |_, txn| t.remove_range(txn, 4, 1)).await; + bob.settle().await; + + // Carol opens the page, syncs, answers the server's SyncStep1, leaves. + let mut carol = Peer::join(&fx, &fx.carol).await; + assert!( + carol + .pump_until(|p| doc_text(&p.doc).contains("seed<")) + .await, + "Carol never received the page" + ); + carol.leave().await; + + // Sentinel after Carol's answer: once durable, her answer would be too. + bob.type_paragraph("sentinel").await; + wait_for_durable_text(&fx.pool, fx.doc_id, "sentinel").await; + + let byline = wait_for_byline(&fx, fx.doc_id, "Bob", |l| l.contains(&fx.bob.id)).await; + assert_eq!( + listed(&byline), + vec![fx.bob.id], + "an editor who only opened the page was credited" + ); + let rows = update_rows(&fx.pool, fx.doc_id).await; + assert!( + rows.iter().all(|r| r.by_user_id == Some(fx.bob.id)), + "the opener's no-op answer was persisted: {:?}", + rows.iter().map(|r| r.by_user_id).collect::>() + ); + bob.leave().await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn history_restore_credits_the_restorer() { + let fx = fixture().await; + let (_, bytes) = knot_markdown::from_markdown::parse("# Snapshot\n\nOld text.").unwrap(); + let engine = knot_crdt::YrsEngine; + let doc = knot_crdt::Engine::new_doc(&engine); + knot_crdt::Engine::apply_update(&engine, &doc, &bytes).unwrap(); + let state = knot_crdt::Engine::encode_state_as_update(&engine, &doc, None).unwrap(); + let sv = knot_crdt::Engine::encode_state_vector(&engine, &doc).unwrap(); + PgSnapshotStore::new(fx.pool.clone()) + .insert(fx.doc_id, 1, &state, &sv) + .await + .unwrap(); + + let (status, body) = post( + &fx.app, + &fx.carol.session, + &format!("/api/docs/{}/history/1/restore", fx.doc_id), + "application/json", + Vec::new(), + ) + .await; + assert_eq!(status, StatusCode::NO_CONTENT, "restore: {body}"); + + let byline = wait_for_byline(&fx, fx.doc_id, "restore", |l| l.contains(&fx.carol.id)).await; + assert_eq!(listed(&byline), vec![fx.carol.id]); + let rows = update_rows(&fx.pool, fx.doc_id).await; + assert_eq!( + rows.last().map(|r| r.by_user_id), + Some(Some(fx.carol.id)), + "the restore must be stored under the restorer" + ); +} + +#[tokio::test(flavor = "multi_thread")] +async fn markdown_import_credits_the_importer() { + let fx = fixture().await; + let (status, body) = post( + &fx.app, + &fx.bob.session, + &format!("/api/docs/{}/markdown", fx.doc_id), + "text/markdown", + b"# Imported\n\nBody.".to_vec(), + ) + .await; + assert_eq!(status, StatusCode::NO_CONTENT, "import: {body}"); + + let byline = wait_for_byline(&fx, fx.doc_id, "import", |l| l.contains(&fx.bob.id)).await; + assert_eq!(listed(&byline), vec![fx.bob.id]); +} + +/// Importing a workspace export fills each new page with content; the +/// person who ran the import is the one who put it there. +#[tokio::test(flavor = "multi_thread")] +async fn workspace_import_credits_the_importer() { + let fx = fixture().await; + let old_id = Uuid::new_v4(); + let manifest = serde_json::json!({ + "knot_export_version": "2", + "docs": [{ + "id": old_id.to_string(), + "parent_id": null, + "title": "From the archive", + "sort_key": "n", + "path": "docs/From-the-archive.md", + }], + "attachments": [], + "boards": [], + }); + let mut zip = zip::ZipWriter::new(std::io::Cursor::new(Vec::new())); + let opts = zip::write::SimpleFileOptions::default(); + zip.start_file("index.json", opts).unwrap(); + zip.write_all(manifest.to_string().as_bytes()).unwrap(); + zip.start_file("docs/From-the-archive.md", opts).unwrap(); + zip.write_all(b"# From the archive\n\nImported body.") + .unwrap(); + let bytes = zip.finish().unwrap().into_inner(); + + let (status, body) = post( + &fx.app, + &fx.alice.session, + "/api/workspace/import", + "application/zip", + bytes, + ) + .await; + assert_eq!(status, StatusCode::OK, "workspace import: {body}"); + + let imported = fx + .state + .docs + .as_ref() + .unwrap() + .list_alive(fx.ws_id) + .await + .unwrap() + .into_iter() + .find(|d| d.title == "From the archive") + .expect("imported doc"); + let byline = wait_for_byline(&fx, imported.id, "workspace import", |l| { + l.contains(&fx.alice.id) + }) + .await; + assert_eq!(listed(&byline), vec![fx.alice.id]); +} + +// --------------------------------------------------------------------------- +// GET /api/docs/{id}/contributors +// --------------------------------------------------------------------------- + +fn at(h: u32) -> DateTime { + Utc.with_ymd_and_hms(2026, 3, 3, h, 0, 0).unwrap() +} + +/// Credit `user` with one content change, then pin its timestamps. +async fn contribute(fx: &Fixture, user: Uuid, first: DateTime, last: DateTime) { + PgUpdatesStore::new(fx.pool.clone()) + .insert_batch(fx.doc_id, &[(Some(user), vec![0u8])]) + .await + .unwrap(); + sqlx::query( + "UPDATE doc_contributors SET first_edited_at = $3, last_edited_at = $4 + WHERE doc_id = $1 AND user_id = $2", + ) + .bind(fx.doc_id) + .bind(user) + .bind(first) + .bind(last) + .execute(&fx.pool) + .await + .unwrap(); +} + +fn keys(v: &serde_json::Value) -> Vec<&str> { + let mut k: Vec<&str> = v.as_object().unwrap().keys().map(String::as_str).collect(); + k.sort(); + k +} + +fn ts(v: &serde_json::Value) -> DateTime { + let s = v.as_str().expect("timestamp string"); + assert!(s.ends_with('Z'), "timestamps are RFC 3339 in UTC: {s}"); + DateTime::parse_from_rfc3339(s).unwrap().with_timezone(&Utc) +} + +/// The full contract, read by the least-privileged role. +#[tokio::test(flavor = "multi_thread")] +async fn viewer_reads_the_byline() { + let fx = fixture().await; + contribute(&fx, fx.bob.id, at(9), at(11)).await; + contribute(&fx, fx.carol.id, at(10), at(12)).await; + + let (status, content_type, body) = + get_contributors(&fx.app, Some(&fx.vic.session), fx.doc_id).await; + assert_eq!(status, StatusCode::OK, "{body}"); + assert_eq!(content_type.as_deref(), Some("application/json")); + assert!( + !body.contains('@'), + "the byline must never carry emails: {body}" + ); + + let v: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!( + keys(&v), + [ + "contributors", + "contributors_since", + "created_at", + "created_by" + ] + ); + assert_eq!( + v["created_by"], + serde_json::json!({"id": fx.alice.id, "display_name": "Alice"}) + ); + assert_eq!(ts(&v["created_at"]), fx.doc_created_at); + assert_eq!( + ts(&v["contributors_since"]), + fx.doc_created_at, + "a doc created after tracking began is tracked from creation" + ); + + let contributors = v["contributors"].as_array().unwrap(); + assert_eq!(contributors.len(), 2, "{body}"); + for (c, (id, name, first, last)) in contributors.iter().zip([ + (fx.carol.id, "Carol", at(10), at(12)), + (fx.bob.id, "Bob", at(9), at(11)), + ]) { + assert_eq!( + keys(c), + [ + "display_name", + "first_edited_at", + "last_edited_at", + "user_id" + ] + ); + assert_eq!(c["user_id"], serde_json::json!(id)); + assert_eq!(c["display_name"], name); + assert_eq!(ts(&c["first_edited_at"]), first); + assert_eq!(ts(&c["last_edited_at"]), last); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn byline_requires_a_session() { + let fx = fixture().await; + let (status, _, body) = get_contributors(&fx.app, None, fx.doc_id).await; + assert_eq!(status, StatusCode::UNAUTHORIZED, "{body}"); + let v: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!(v["error"]["code"], "auth.session_required"); +} + +/// A doc the caller cannot open (here: another workspace's) is refused +/// exactly like every other doc sub-resource. +#[tokio::test(flavor = "multi_thread")] +async fn byline_is_denied_without_access_to_the_doc() { + let fx = fixture().await; + let ws = fx.state.workspaces.as_ref().unwrap(); + let other = ws.create("other", "Other").await.unwrap(); + let stranger = fx + .state + .users + .as_ref() + .unwrap() + .create_local("stranger@contrib.test", "Stranger", "$h$") + .await + .unwrap(); + ws.add_member(other.id, stranger.id, WorkspaceRole::Owner) + .await + .unwrap(); + let foreign = fx + .state + .docs + .as_ref() + .unwrap() + .create(other.id, None, "Foreign", "m", stranger.id) + .await + .unwrap(); + + let (status, _, body) = get_contributors(&fx.app, Some(&fx.alice.session), foreign.id).await; + assert_eq!(status, StatusCode::FORBIDDEN, "{body}"); + let v: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!(v["error"]["code"], "acl.no_grant"); +} + +/// Leaving the workspace does not erase what someone wrote. +#[tokio::test(flavor = "multi_thread")] +async fn byline_still_names_a_former_member() { + let fx = fixture().await; + contribute(&fx, fx.bob.id, at(9), at(11)).await; + fx.state + .workspaces + .as_ref() + .unwrap() + .remove_member(fx.ws_id, fx.bob.id) + .await + .unwrap(); + + let (status, _, body) = get_contributors(&fx.app, Some(&fx.alice.session), fx.doc_id).await; + assert_eq!(status, StatusCode::OK, "{body}"); + let v: serde_json::Value = serde_json::from_str(&body).unwrap(); + assert_eq!( + v["contributors"], + serde_json::json!([{ + "user_id": fx.bob.id, + "display_name": "Bob", + "first_edited_at": v["contributors"][0]["first_edited_at"], + "last_edited_at": v["contributors"][0]["last_edited_at"], + }]) + ); +} diff --git a/crates/knot-server/tests/convergence.rs b/crates/knot-server/tests/convergence.rs index 05d59d5..3685d29 100644 --- a/crates/knot-server/tests/convergence.rs +++ b/crates/knot-server/tests/convergence.rs @@ -179,15 +179,28 @@ fn append_var_uint(out: &mut Vec, mut v: u64) { out.push(v as u8); } -/// Drain all messages currently queued on `ws`, discarding them. -/// Used to consume the initial sync-step-2 frame the server sends on connect. +/// Consume the handshake the server sends on connect: its sync-step-2 (full +/// state) followed by its own sync-step-1 (asking the client for anything the +/// server lacks). This test client has nothing to offer, so it leaves the +/// step-1 unanswered. async fn drain_initial( ws: &mut tokio_tungstenite::WebSocketStream< tokio_tungstenite::MaybeTlsStream, >, ) { - // The server sends exactly one sync-step-2 on join. Read it. - let _ = tokio::time::timeout(Duration::from_secs(2), ws.next()).await; + for expected_subtype in [1u8, 0u8] { + let frame = tokio::time::timeout(Duration::from_secs(2), ws.next()) + .await + .expect("handshake frame") + .expect("stream open") + .expect("ws ok") + .into_data(); + assert_eq!( + &frame[..2], + &[0u8, expected_subtype], + "unexpected handshake frame" + ); + } } // --------------------------------------------------------------------------- diff --git a/crates/knot-server/tests/offline_sync.rs b/crates/knot-server/tests/offline_sync.rs new file mode 100644 index 0000000..a30dc42 --- /dev/null +++ b/crates/knot-server/tests/offline_sync.rs @@ -0,0 +1,719 @@ +//! Offline edits must reach the server on reconnect. +//! +//! The web providers (`web/src/features/editor/KnotProvider.ts`, +//! `web/src/features/boards/BoardProvider.ts`) apply edits made while the +//! socket is not OPEN to their local Y.Doc but do not send them. On +//! (re)connect they send SyncStep1, which only asks the server for what the +//! CLIENT lacks; they answer a server SyncStep1 with +//! `SyncStep2 = encodeStateAsUpdate(doc, serverSV)`. So offline edits reach +//! the server only if the server asks — standard y-protocol, where BOTH +//! sides send SyncStep1. These tests drive the real collab sockets with a raw +//! client that mirrors those providers exactly, so the fix has to work for +//! already-deployed SPA bundles without any client change. +//! +//! Once the server asks, every (re)connecting writer replies with a +//! SyncStep2 that usually carries nothing new (Yjs always includes its whole +//! delete set). The `*_noop_*` tests pin that such replies are neither +//! persisted nor fanned out. + +use std::sync::{Arc, Mutex}; +use std::time::Duration; + +use futures_util::{SinkExt, StreamExt}; +use knot_auth::{Hasher, Throttle}; +use knot_server::protocol::{YSyncMessage, decode}; +use knot_server::{AppState, router_with_state}; +use knot_storage::{ + BoardStore, PgSnapshotStore, PgUpdatesStore, SnapshotStore, UpdatesStore, WorkspaceRole, + sort_key_between, +}; +use tokio::net::TcpListener; +use tokio::time::Instant; +use tokio_tungstenite::tungstenite::{self, client::IntoClientRequest}; +use tower::ServiceExt; +use uuid::Uuid; +use yrs::updates::decoder::Decode; +use yrs::updates::encoder::Encode; +use yrs::{ + Doc, GetString, Map, MapRef, ReadTxn, StateVector, Text, Transact, TransactionMut, Update, + XmlElementPrelim, XmlFragment, XmlFragmentRef, XmlTextPrelim, XmlTextRef, +}; + +type Ws = + tokio_tungstenite::WebSocketStream>; + +/// Generous: a loaded CI runner can take a while to hydrate a room and run +/// the writer's 250 ms batch. Every wait polls toward this deadline and +/// returns as soon as its condition holds — no fixed sleeps. +const DEADLINE: Duration = Duration::from_secs(10); +/// How long the reconnecting client waits to be asked for its state. Pre-fix +/// the server never asks, so this is pure dead time — keep it short. +const HANDSHAKE_WAIT: Duration = Duration::from_secs(2); + +// --------------------------------------------------------------------------- +// Fixture +// --------------------------------------------------------------------------- + +struct Fixture { + addr: std::net::SocketAddr, + /// `sid=…; csrf=…` for the workspace owner. + cookie: String, + pool: knot_storage::Pool, + user_id: Uuid, + doc_id: Uuid, + boards: Arc, +} + +/// Postgres-backed state with doc rooms AND board rooms wired to a MemBus, +/// one owner, one doc, served on an ephemeral port. +async fn fixture() -> Fixture { + let db = knot_test_support::fresh_db().await; + let pool = db.pool.clone(); + + let mut s = AppState::with_pool(pool.clone()); + s.hasher = Arc::new(Hasher::fast_for_tests()); + s.throttle = Arc::new(Throttle::new()); + s.session_key = b"test-key-32-bytes-aaaaaaaaaaaaaa".to_vec(); + s.cookie_secure = false; + + let engine: Arc = Arc::new(knot_crdt::YrsEngine); + let bus: Arc = Arc::new(knot_crdt::MemBus::new()); + let updates: Arc = Arc::new(PgUpdatesStore::new(pool.clone())); + let snapshots: Arc = Arc::new(PgSnapshotStore::new(pool.clone())); + s.rooms_v2 = Some(Arc::new(knot_crdt::Rooms::new( + engine.clone(), + bus.clone(), + updates, + snapshots, + knot_crdt::SnapshotPolicy { + every_n: 1000, + idle: Duration::from_secs(600), + }, + Duration::from_secs(300), + ))); + let boards = s.boards.clone().expect("with_pool wires boards"); + s.board_rooms = Some(Arc::new(knot_crdt::BoardRooms::new( + engine, + boards.clone(), + bus, + ))); + + let email = "offline@sync.test"; + let password = "offlinepass"; + let hash = s.hasher.hash(password).unwrap(); + let ws = s + .workspaces + .as_ref() + .unwrap() + .create("default", "W") + .await + .unwrap(); + let u = s + .users + .as_ref() + .unwrap() + .create_local(email, "U", &hash) + .await + .unwrap(); + s.workspaces + .as_ref() + .unwrap() + .add_member(ws.id, u.id, WorkspaceRole::Owner) + .await + .unwrap(); + let doc = s + .docs + .as_ref() + .unwrap() + .create(ws.id, None, "Offline", &sort_key_between(None, None), u.id) + .await + .unwrap(); + + let r = router_with_state(s.clone()) + .oneshot( + axum::http::Request::builder() + .method("POST") + .uri("/auth/login") + .header("content-type", "application/json") + .body(axum::body::Body::from( + serde_json::json!({"email": email, "password": password}).to_string(), + )) + .unwrap(), + ) + .await + .unwrap(); + let cookie = r + .headers() + .get_all("set-cookie") + .iter() + .map(|v| v.to_str().unwrap().split(';').next().unwrap().to_string()) + .filter(|kv| kv.starts_with("sid=") || kv.starts_with("csrf=")) + .collect::>() + .join("; "); + assert!(cookie.contains("sid="), "login set no session cookie"); + + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + let app = router_with_state(s); + tokio::spawn(async move { + axum::serve(listener, app).await.unwrap(); + }); + + Fixture { + addr, + cookie, + pool, + user_id: u.id, + doc_id: doc.id, + boards, + } +} + +// --------------------------------------------------------------------------- +// Raw y-sync client mirroring KnotProvider / BoardProvider +// --------------------------------------------------------------------------- + +fn sync_frame(subtype: u8, payload: &[u8]) -> Vec { + let mut out = vec![0u8, subtype]; + knot_server::protocol::append_var_uint(&mut out, payload.len() as u64); + out.extend_from_slice(payload); + out +} + +/// Root types both providers use: Tiptap's `default` fragment and +/// Excalidraw's `elements` map. +struct Roots { + frag: XmlFragmentRef, + elements: MapRef, +} + +struct Peer { + doc: Doc, + ws: Option, + /// Server SyncStep1 frames this peer has answered with a SyncStep2. + step1_answered: usize, + /// Server SyncStep2 frames applied (the initial state on every join). + step2_received: usize, + /// SYNC_UPDATE frames applied — i.e. fan-out from other connections. + updates_received: usize, +} + +impl Peer { + fn new() -> Self { + Self { + doc: Doc::new(), + ws: None, + step1_answered: 0, + step2_received: 0, + updates_received: 0, + } + } + + /// `connect()` + `onopen`: dial, then send SyncStep1 with our state + /// vector. (The provider also sends its awareness state; awareness is + /// irrelevant to document sync and is left out.) + async fn connect(&mut self, fx: &Fixture, path: &str) { + let mut req = format!("ws://{}{path}", fx.addr) + .into_client_request() + .unwrap(); + req.headers_mut().insert( + "cookie", + tungstenite::http::HeaderValue::from_str(&fx.cookie).unwrap(), + ); + let (mut ws, _) = tokio_tungstenite::connect_async(req) + .await + .expect("ws connect"); + let sv = self.doc.transact().state_vector().encode_v1(); + ws.send(tungstenite::Message::Binary(sync_frame(0, &sv).into())) + .await + .unwrap(); + self.ws = Some(ws); + } + + /// The network goes away: drop the TCP connection without a close + /// handshake, as a laptop lid or a flaky train connection would. (A + /// graceful `close()` would wait for the server's Close reply, which the + /// server's read loop never sends once it has broken out.) + /// + /// Settles first: closing a socket with unread inbound data makes the OS + /// send RST, and the server may then discard frames it has not read yet. + /// That is a real-world loss mode too, but these tests pin a different + /// one, so make sure everything sent so far has been applied. + async fn disconnect(&mut self) { + if self.ws.is_some() { + self.settle().await; + } + self.ws.take(); + } + + /// Round-trip barrier: the server answers SyncStep1 from the same read + /// loop that forwards our updates to the room actor, and the actor handles + /// both in order, so once that SyncStep2 is back every update we sent + /// before it has been applied. + async fn settle(&mut self) { + let sv = self.doc.transact().state_vector().encode_v1(); + let ws = self.ws.as_mut().unwrap(); + ws.send(tungstenite::Message::Binary(sync_frame(0, &sv).into())) + .await + .unwrap(); + let seen = self.step2_received; + assert!( + self.pump_until(DEADLINE, |p| p.step2_received > seen).await, + "server never answered the settle SyncStep1" + ); + } + + /// A local edit, as Tiptap/Excalidraw would make it. Mirrors + /// `handleDocUpdate`: the resulting update is sent only while the socket + /// is open; offline it exists solely in the local doc. + async fn edit(&mut self, f: impl FnOnce(&Roots, &mut TransactionMut)) { + // Root refs must be taken BEFORE opening the transaction: + // `get_or_insert_*` acquires its own write lock and would deadlock + // against an open `TransactionMut`. + let roots = Roots { + frag: self.doc.get_or_insert_xml_fragment("default"), + elements: self.doc.get_or_insert_map("elements"), + }; + let captured: Arc>>> = Arc::default(); + let sink = captured.clone(); + let sub = self + .doc + .observe_update_v1(move |_, e| sink.lock().unwrap().push(e.update.clone())); + { + let mut txn = self.doc.transact_mut(); + f(&roots, &mut txn); + } + drop(sub); + let updates = std::mem::take(&mut *captured.lock().unwrap()); + if let Some(ws) = self.ws.as_mut() { + for u in updates { + ws.send(tungstenite::Message::Binary(sync_frame(2, &u).into())) + .await + .unwrap(); + } + } + } + + /// `handleMessage` for MSG_SYNC: answer SyncStep1 with + /// `encodeStateAsUpdate(doc, serverSV)`; apply SyncStep2 / Update. + async fn handle(&mut self, bytes: &[u8]) { + match decode(bytes) { + Ok(YSyncMessage::SyncStep1(sv)) => { + let sv = StateVector::decode_v1(&sv).expect("server sent a valid state vector"); + let reply = self.doc.transact().encode_state_as_update_v1(&sv); + self.ws + .as_mut() + .unwrap() + .send(tungstenite::Message::Binary(sync_frame(1, &reply).into())) + .await + .unwrap(); + self.step1_answered += 1; + } + Ok(YSyncMessage::SyncStep2(u)) => { + self.apply(&u); + self.step2_received += 1; + } + Ok(YSyncMessage::Update(u)) => { + self.apply(&u); + self.updates_received += 1; + } + _ => {} + } + } + + fn apply(&self, update: &[u8]) { + let u = Update::decode_v1(update).expect("server sent a valid update"); + self.doc.transact_mut().apply_update(u).unwrap(); + } + + /// Process inbound frames until `done` holds or `within` elapses. + /// Returns whether `done` held. + async fn pump_until(&mut self, within: Duration, done: impl Fn(&Peer) -> bool) -> bool { + let deadline = Instant::now() + within; + loop { + if done(self) { + return true; + } + let remaining = deadline.saturating_duration_since(Instant::now()); + if remaining.is_zero() { + return false; + } + let ws = self.ws.as_mut().expect("pump on a connected peer"); + match tokio::time::timeout(remaining, ws.next()).await { + Ok(Some(Ok(tungstenite::Message::Binary(b)))) => self.handle(&b).await, + Ok(Some(Ok(_))) => {} + Ok(Some(Err(e))) => panic!("ws error: {e}"), + Ok(None) => panic!("server closed the socket"), + Err(_) => return false, + } + } + } + + fn doc_text(&self) -> String { + doc_text(&self.doc) + } + + fn board_keys(&self) -> Vec { + board_keys(&self.doc) + } +} + +fn doc_text(doc: &Doc) -> String { + let frag = doc.get_or_insert_xml_fragment("default"); + frag.get_string(&doc.transact()) +} + +fn board_keys(doc: &Doc) -> Vec { + let elements = doc.get_or_insert_map("elements"); + let txn = doc.transact(); + let mut keys: Vec = elements.keys(&txn).map(str::to_string).collect(); + keys.sort(); + keys +} + +/// Rebuild the doc exactly as a cold `Room::spawn` would: latest snapshot, +/// then every persisted update after it. Returns the doc and the number of +/// `doc_updates` rows. +async fn hydrate_doc(pool: &knot_storage::Pool, doc_id: Uuid) -> (Doc, usize) { + let doc = Doc::new(); + let mut after = 0; + if let Some(snap) = PgSnapshotStore::new(pool.clone()) + .latest(doc_id) + .await + .unwrap() + { + let u = Update::decode_v1(&snap.state_bytes).unwrap(); + doc.transact_mut().apply_update(u).unwrap(); + after = snap.snapshot_seq; + } + let rows = PgUpdatesStore::new(pool.clone()) + .since(doc_id, after) + .await + .unwrap(); + for r in &rows { + let u = Update::decode_v1(&r.update_bytes).unwrap(); + doc.transact_mut().apply_update(u).unwrap(); + } + (doc, rows.len()) +} + +/// Board equivalent of [`hydrate_doc`] (`BoardRoom::spawn` replays the whole +/// `board_updates` log). Returns the doc and the row count. +async fn hydrate_board(boards: &Arc, board_id: Uuid) -> (Doc, usize) { + let doc = Doc::new(); + let rows = boards.load_updates(board_id).await.unwrap(); + for r in &rows { + let u = Update::decode_v1(r).unwrap(); + doc.transact_mut().apply_update(u).unwrap(); + } + (doc, rows.len()) +} + +/// Poll storage until `done` holds for the hydrated doc, or panic. +async fn wait_for_storage(what: &str, load: F, done: impl Fn(&Doc) -> bool) -> usize +where + F: Fn() -> Fut, + Fut: std::future::Future, +{ + let deadline = Instant::now() + DEADLINE; + loop { + let (doc, rows) = load().await; + if done(&doc) { + return rows; + } + if Instant::now() >= deadline { + panic!( + "{what} never became durable; storage holds text {:?}, board keys {:?}", + doc_text(&doc), + board_keys(&doc) + ); + } + // Storage is polled, not pushed: back off briefly between queries so + // the loop does not hammer Postgres. Bounded by DEADLINE above. + tokio::time::sleep(Duration::from_millis(50)).await; + } +} + +fn paragraph_text(roots: &Roots, txn: &mut TransactionMut, s: &str) -> XmlTextRef { + let p = roots + .frag + .push_back(txn, XmlElementPrelim::empty("paragraph")); + p.push_back(txn, XmlTextPrelim::new(s)) +} + +// --------------------------------------------------------------------------- +// Documents +// --------------------------------------------------------------------------- + +/// The reader report: edits made while offline never reached the server, and +/// later edits built on them were parked as pending by yrs and stayed +/// invisible to everyone else. +#[tokio::test(flavor = "multi_thread")] +async fn doc_edits_made_offline_reach_the_server_on_reconnect() { + let fx = fixture().await; + let path = format!("/collab/doc/{}", fx.doc_id); + + // A connects, syncs, types "online" while connected. + let mut a = Peer::new(); + a.connect(&fx, &path).await; + assert!( + a.pump_until(DEADLINE, |p| p.step2_received >= 1).await, + "A never received the initial state" + ); + let text: Arc>> = Arc::default(); + let slot = text.clone(); + a.edit(move |r, txn| { + *slot.lock().unwrap() = Some(paragraph_text(r, txn, "online")); + }) + .await; + let text = text.lock().unwrap().take().unwrap(); + + // Connection drops. A keeps typing; the update is NOT sent. + a.disconnect().await; + let t = text.clone(); + a.edit(move |_, txn| t.push(txn, " offline")).await; + + // Reconnect: onopen sends SyncStep1, and A answers any server SyncStep1. + let answered = a.step1_answered; + a.connect(&fx, &path).await; + let asked = a + .pump_until(HANDSHAKE_WAIT, |p| p.step1_answered > answered) + .await; + // A keeps typing live. This update depends on the offline one. + let t = text.clone(); + a.edit(move |_, txn| t.push(txn, "!")).await; + + // A fresh reader must see everything. + let mut b = Peer::new(); + b.connect(&fx, &path).await; + let converged = b + .pump_until(DEADLINE, |p| p.doc_text().contains("online offline!")) + .await; + assert!( + converged, + "fresh client never saw the offline edit: got {:?}; the server asked the \ + reconnecting client for its state (SyncStep1): {asked}", + b.doc_text() + ); + + // And it is durable: a cold hydration from doc_updates has it too. + let pool = fx.pool.clone(); + let doc_id = fx.doc_id; + wait_for_storage( + "offline doc edit", + || hydrate_doc(&pool, doc_id), + |d| doc_text(d).contains("online offline!"), + ) + .await; + + a.disconnect().await; + b.disconnect().await; +} + +/// Once the server asks every joining writer for its state, the typical +/// reply carries nothing new (just the full delete set). That must not be +/// persisted — otherwise every page open writes a `doc_updates` row and makes +/// the opener a "contributor" — nor fanned out to the other connections. +#[tokio::test(flavor = "multi_thread")] +async fn doc_noop_sync_reply_is_neither_persisted_nor_fanned_out() { + let fx = fixture().await; + let path = format!("/collab/doc/{}", fx.doc_id); + + // Seed real history including a deletion, so every later SyncStep2 reply + // carries a non-empty delete set — the realistic no-op. + let mut a = Peer::new(); + a.connect(&fx, &path).await; + assert!(a.pump_until(DEADLINE, |p| p.step2_received >= 1).await); + let text: Arc>> = Arc::default(); + let slot = text.clone(); + a.edit(move |r, txn| { + *slot.lock().unwrap() = Some(paragraph_text(r, txn, "seedx")); + }) + .await; + let text = text.lock().unwrap().take().unwrap(); + a.edit(move |_, txn| text.remove_range(txn, 4, 1)).await; + a.disconnect().await; + let real_updates = 2; + + // B observes. C joins and makes one real edit. + let mut b = Peer::new(); + b.connect(&fx, &path).await; + assert!( + b.pump_until(DEADLINE, |p| p.step1_answered >= 1 + && p.doc_text().contains("seed<")) + .await, + "B never completed the handshake: text {:?}, SyncStep1 answered {}", + b.doc_text(), + b.step1_answered + ); + let mut c = Peer::new(); + c.connect(&fx, &path).await; + assert!( + c.pump_until(DEADLINE, |p| p.step1_answered >= 1 + && p.doc_text().contains("seed<")) + .await, + "C never completed the handshake" + ); + c.edit(|r, txn| { + paragraph_text(r, txn, "sentinel"); + }) + .await; + let real_updates = real_updates + 1; + + // B's next fan-out must be the sentinel itself — not C's no-op reply. + assert!( + b.pump_until(DEADLINE, |p| p.doc_text().contains("sentinel")) + .await, + "B never received C's edit" + ); + assert_eq!( + b.updates_received, 1, + "B was sent {} SYNC_UPDATE frames for one real edit: C's no-op \ + SyncStep2 was fanned out", + b.updates_received + ); + + // The writer persists in arrival order, so once the sentinel is durable + // any no-op enqueued before it would be too. + let pool = fx.pool.clone(); + let doc_id = fx.doc_id; + let rows = wait_for_storage( + "sentinel", + || hydrate_doc(&pool, doc_id), + |d| doc_text(d).contains("sentinel"), + ) + .await; + assert_eq!( + rows, real_updates, + "doc_updates holds {rows} rows for {real_updates} real edits: no-op \ + SyncStep2 replies from (re)connecting writers were persisted" + ); + + b.disconnect().await; + c.disconnect().await; +} + +// --------------------------------------------------------------------------- +// Boards — BoardProvider has the identical offline gap +// --------------------------------------------------------------------------- + +#[tokio::test(flavor = "multi_thread")] +async fn board_edits_made_offline_reach_the_server_on_reconnect() { + let fx = fixture().await; + let board = fx.boards.create(fx.doc_id, fx.user_id, None).await.unwrap(); + let path = format!("/collab/board/{}", board.id); + + let mut a = Peer::new(); + a.connect(&fx, &path).await; + assert!(a.pump_until(DEADLINE, |p| p.step2_received >= 1).await); + a.edit(|r, txn| { + r.elements.insert(txn, "online", "rect"); + }) + .await; + + a.disconnect().await; + a.edit(|r, txn| { + r.elements.insert(txn, "offline", "ellipse"); + }) + .await; + + let answered = a.step1_answered; + a.connect(&fx, &path).await; + let asked = a + .pump_until(HANDSHAKE_WAIT, |p| p.step1_answered > answered) + .await; + + let mut b = Peer::new(); + b.connect(&fx, &path).await; + let converged = b + .pump_until(DEADLINE, |p| p.board_keys() == ["offline", "online"]) + .await; + assert!( + converged, + "fresh client never saw the offline board edit: got {:?}; the server \ + asked the reconnecting client for its state (SyncStep1): {asked}", + b.board_keys() + ); + + let boards = fx.boards.clone(); + wait_for_storage( + "offline board edit", + || hydrate_board(&boards, board.id), + |d| board_keys(d) == ["offline", "online"], + ) + .await; + + a.disconnect().await; + b.disconnect().await; +} + +#[tokio::test(flavor = "multi_thread")] +async fn board_noop_sync_reply_is_neither_persisted_nor_fanned_out() { + let fx = fixture().await; + let board = fx.boards.create(fx.doc_id, fx.user_id, None).await.unwrap(); + let path = format!("/collab/board/{}", board.id); + + // Seed history with a deletion (Excalidraw deletes map keys too). + let mut a = Peer::new(); + a.connect(&fx, &path).await; + assert!(a.pump_until(DEADLINE, |p| p.step2_received >= 1).await); + a.edit(|r, txn| { + r.elements.insert(txn, "keep", "rect"); + r.elements.insert(txn, "gone", "rect"); + }) + .await; + a.edit(|r, txn| { + r.elements.remove(txn, "gone"); + }) + .await; + a.disconnect().await; + let real_updates = 2; + + let mut b = Peer::new(); + b.connect(&fx, &path).await; + assert!( + b.pump_until(DEADLINE, |p| p.step1_answered >= 1 + && p.board_keys() == ["keep"]) + .await, + "B never completed the handshake: keys {:?}, SyncStep1 answered {}", + b.board_keys(), + b.step1_answered + ); + let mut c = Peer::new(); + c.connect(&fx, &path).await; + assert!( + c.pump_until(DEADLINE, |p| p.step1_answered >= 1 + && p.board_keys() == ["keep"]) + .await, + "C never completed the handshake" + ); + c.edit(|r, txn| { + r.elements.insert(txn, "sentinel", "rect"); + }) + .await; + let real_updates = real_updates + 1; + + assert!( + b.pump_until(DEADLINE, |p| p.board_keys().contains(&"sentinel".into())) + .await, + "B never received C's edit" + ); + assert_eq!( + b.updates_received, 1, + "B was sent {} SYNC_UPDATE frames for one real edit: C's no-op \ + SyncStep2 was fanned out", + b.updates_received + ); + + // BoardRoom appends inline before fanning out, so by the time B has the + // sentinel every earlier inbound update has already been written. + let (_, rows) = hydrate_board(&fx.boards, board.id).await; + assert_eq!( + rows, real_updates, + "board_updates holds {rows} rows for {real_updates} real edits: no-op \ + SyncStep2 replies from (re)connecting writers were persisted" + ); + + b.disconnect().await; + c.disconnect().await; +} diff --git a/crates/knot-storage/src/contributors.rs b/crates/knot-storage/src/contributors.rs new file mode 100644 index 0000000..093dd2d --- /dev/null +++ b/crates/knot-storage/src/contributors.rs @@ -0,0 +1,111 @@ +//! Doc byline: who created a page and who has changed its content since. +//! +//! `doc_contributors` is written by `PgUpdatesStore::insert_batch`, in the +//! same statement that persists each content update — this module only +//! reads it. Names are resolved by joining `users` rather than +//! `workspace_members`, so someone removed from the workspace keeps their +//! credit. Only ids and display names leave this module, never emails. + +use async_trait::async_trait; +use chrono::{DateTime, Utc}; +use serde::Serialize; +use sqlx::PgPool; +use thiserror::Error; +use uuid::Uuid; + +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct PersonRef { + pub id: Uuid, + pub display_name: String, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct Contributor { + pub user_id: Uuid, + pub display_name: String, + pub first_edited_at: DateTime, + pub last_edited_at: DateTime, +} + +#[derive(Debug, Clone, PartialEq, Eq, Serialize)] +pub struct DocByline { + pub created_by: PersonRef, + pub created_at: DateTime, + /// From when on `contributors` is complete. Docs that predate + /// contributor tracking carry the migration time here; their earlier + /// edits were never attributed. + pub contributors_since: DateTime, + /// Most recent editor first; ties broken by display name. + pub contributors: Vec, +} + +#[derive(Debug, Error)] +pub enum ContributorStoreError { + #[error("sqlx: {0}")] + Sqlx(#[from] sqlx::Error), +} + +#[async_trait] +pub trait ContributorStore: Send + Sync + 'static { + /// The byline for `doc_id`, or `None` if no such doc exists. + async fn byline(&self, doc_id: Uuid) -> Result, ContributorStoreError>; +} + +#[derive(Clone)] +pub struct PgContributorStore { + pool: PgPool, +} + +impl PgContributorStore { + pub fn new(pool: PgPool) -> Self { + Self { pool } + } +} + +#[async_trait] +impl ContributorStore for PgContributorStore { + async fn byline(&self, doc_id: Uuid) -> Result, ContributorStoreError> { + let Some((creator_id, creator_name, created_at, contributors_since)) = + sqlx::query_as::<_, (Uuid, String, DateTime, DateTime)>( + "SELECT d.created_by, u.display_name, d.created_at, d.contributors_since + FROM documents d + JOIN users u ON u.id = d.created_by + WHERE d.id = $1", + ) + .bind(doc_id) + .fetch_optional(&self.pool) + .await? + else { + return Ok(None); + }; + // user_id is a last tie-break only so equal (time, name) pairs come + // back in a stable order across requests. + let rows = sqlx::query_as::<_, (Uuid, String, DateTime, DateTime)>( + "SELECT c.user_id, u.display_name, c.first_edited_at, c.last_edited_at + FROM doc_contributors c + JOIN users u ON u.id = c.user_id + WHERE c.doc_id = $1 + ORDER BY c.last_edited_at DESC, u.display_name ASC, c.user_id", + ) + .bind(doc_id) + .fetch_all(&self.pool) + .await?; + Ok(Some(DocByline { + created_by: PersonRef { + id: creator_id, + display_name: creator_name, + }, + created_at, + contributors_since, + contributors: rows + .into_iter() + .map(|r| Contributor { + user_id: r.0, + display_name: r.1, + first_edited_at: r.2, + last_edited_at: r.3, + }) + .collect(), + })) + } +} diff --git a/crates/knot-storage/src/lib.rs b/crates/knot-storage/src/lib.rs index 76db9fa..bd1bc42 100644 --- a/crates/knot-storage/src/lib.rs +++ b/crates/knot-storage/src/lib.rs @@ -4,6 +4,7 @@ pub mod audit; pub mod blobs; pub mod boards; pub mod comments; +pub mod contributors; pub mod doc_store; pub mod grant_store; pub mod invalidations; @@ -22,6 +23,9 @@ pub mod workspace_store; pub use blobs::{BlobMeta, BlobMetadata, BlobStore, BlobStoreError, PgBytesStore, S3Store}; pub use boards::{Board, BoardStore, BoardStoreError, PgBoardStore}; pub use comments::{Comment, CommentStore, CommentStoreError, PgCommentStore, Reaction}; +pub use contributors::{ + Contributor, ContributorStore, ContributorStoreError, DocByline, PersonRef, PgContributorStore, +}; pub use doc_store::{DocStore, DocStoreError, Document, PgDocStore}; pub use grant_store::{Grant, GrantStore, GrantStoreError, PgGrantStore}; pub use lexorank::between as sort_key_between; diff --git a/crates/knot-storage/src/updates_store.rs b/crates/knot-storage/src/updates_store.rs index 39d6d04..7fb31a5 100644 --- a/crates/knot-storage/src/updates_store.rs +++ b/crates/knot-storage/src/updates_store.rs @@ -6,7 +6,7 @@ use async_trait::async_trait; use chrono::{DateTime, Utc}; -use sqlx::{AssertSqlSafe, PgPool}; +use sqlx::PgPool; use thiserror::Error; use uuid::Uuid; @@ -27,14 +27,14 @@ pub enum UpdatesStoreError { #[async_trait] pub trait UpdatesStore: Send + Sync + 'static { - /// Insert a batch of updates atomically. Returns the assigned seqs in - /// the same order as the input. The batch is one INSERT with a multi-row - /// VALUES list so all rows share one round-trip. + /// Insert a batch of `(by_user_id, update_bytes)` rows atomically, each + /// row stored with its OWN author: one batch can mix several people's + /// edits (the room writer flushes whatever arrived in its window). + /// Returns the assigned seqs in the same order as the input. async fn insert_batch( &self, doc_id: Uuid, - by_user_id: Option, - updates: &[Vec], + updates: &[(Option, Vec)], ) -> Result, UpdatesStoreError>; /// Fetch updates with `seq > after_seq` for a doc, in seq order. @@ -67,34 +67,56 @@ impl UpdatesStore for PgUpdatesStore { async fn insert_batch( &self, doc_id: Uuid, - by_user_id: Option, - updates: &[Vec], + updates: &[(Option, Vec)], ) -> Result, UpdatesStoreError> { if updates.is_empty() { return Ok(Vec::new()); } - // Build "($1, $2, $3), ($1, $2, $4), ..." with shared doc_id + - // by_user_id binds and one per-update bytea bind. + let (authors, bytes): (Vec>, Vec<&[u8]>) = + updates.iter().map(|(by, b)| (*by, b.as_slice())).unzip(); + // One statement whatever the batch size: the rows travel as two + // parallel arrays. `seq` comes from nextval() as rows are inserted, + // and UNNEST … ORDER BY ord feeds them in input order, so ascending + // seq IS input order — which the writer relies on to zip seqs back + // onto its jobs. // - // The generated string contains only a fixed prefix and `$N` placeholder - // tuples whose N is a loop counter, so the `AssertSqlSafe` below is - // sound: no caller data reaches the SQL text, only the binds. - let mut sql = - String::from("INSERT INTO doc_updates (doc_id, by_user_id, update_bytes) VALUES "); - for i in 0..updates.len() { - if i > 0 { - sql.push_str(", "); - } - sql.push_str(&format!("($1, $2, ${})", i + 3)); - } - sql.push_str(" RETURNING seq"); - let mut q = sqlx::query_scalar::<_, i64>(AssertSqlSafe(sql)) - .bind(doc_id) - .bind(by_user_id); - for u in updates { - q = q.bind(u); - } - let seqs = q.fetch_all(&self.pool).await?; + // The `contrib` CTE credits every distinct author in the SAME + // statement, so a content change and its byline entry commit or fail + // together. A data-modifying CTE always runs to completion even + // though the outer SELECT never reads it. Details: + // - DISTINCT: ON CONFLICT DO UPDATE may not touch a row twice in + // one statement, and a batch often holds many rows per author. + // - ORDER BY by_user_id: replicas flush the same doc concurrently; + // taking the row locks in one global order rules out deadlocks. + // - the WHERE throttles the refresh to once a minute — continuous + // typing flushes every 250 ms, and a minute-old "last edited" is + // as good as a fresh one for a byline. + let seqs = sqlx::query_scalar::<_, i64>( + "WITH ins AS ( + INSERT INTO doc_updates (doc_id, by_user_id, update_bytes) + SELECT $1, u.by_user_id, u.update_bytes + FROM UNNEST($2::uuid[], $3::bytea[]) + WITH ORDINALITY AS u(by_user_id, update_bytes, ord) + ORDER BY u.ord + RETURNING seq, by_user_id + ), + contrib AS ( + INSERT INTO doc_contributors (doc_id, user_id) + SELECT DISTINCT $1::uuid, by_user_id + FROM ins + WHERE by_user_id IS NOT NULL + ORDER BY by_user_id + ON CONFLICT (doc_id, user_id) DO UPDATE + SET last_edited_at = now() + WHERE doc_contributors.last_edited_at < now() - interval '1 minute' + ) + SELECT seq FROM ins ORDER BY seq", + ) + .bind(doc_id) + .bind(&authors) + .bind(&bytes) + .fetch_all(&self.pool) + .await?; Ok(seqs) } diff --git a/crates/knot-storage/tests/cancel_safety.rs b/crates/knot-storage/tests/cancel_safety.rs index 5ac75a6..e7496ba 100644 --- a/crates/knot-storage/tests/cancel_safety.rs +++ b/crates/knot-storage/tests/cancel_safety.rs @@ -71,11 +71,8 @@ async fn a_cancelled_fetch_does_not_strand_its_connection() { let store = PgUpdatesStore::new(pool.clone()); let blob = vec![7u8; 4096]; for _ in 0..2 { - let batch: Vec> = (0..125).map(|_| blob.clone()).collect(); - store - .insert_batch(doc.id, Some(u.id), &batch) - .await - .unwrap(); + let batch: Vec<_> = (0..125).map(|_| (Some(u.id), blob.clone())).collect(); + store.insert_batch(doc.id, &batch).await.unwrap(); } let obs = observer(&db.url).await; diff --git a/crates/knot-storage/tests/contributors.rs b/crates/knot-storage/tests/contributors.rs new file mode 100644 index 0000000..0ee0786 --- /dev/null +++ b/crates/knot-storage/tests/contributors.rs @@ -0,0 +1,172 @@ +//! Doc byline: creator + everyone who changed the page content. + +use chrono::{DateTime, TimeZone, Utc}; +use knot_storage::{ + Contributor, ContributorStore, DocStore, PgContributorStore, PgDocStore, PgUpdatesStore, + PgUserStore, PgWorkspaceStore, UpdatesStore, UserStore, WorkspaceRole, WorkspaceStore, +}; +use uuid::Uuid; + +struct Fx { + pool: sqlx::PgPool, + ws: Uuid, + doc: knot_storage::Document, + /// (id, display_name) of the doc's creator. + alice: (Uuid, String), +} + +async fn fixture() -> Fx { + let pool = knot_test_support::fresh_db().await.pool; + let ws = PgWorkspaceStore::new(pool.clone()) + .create("default", "W") + .await + .unwrap(); + let alice = add_user(&pool, ws.id, "alice@x.test", "Alice").await; + let doc = PgDocStore::new(pool.clone()) + .create(ws.id, None, "D", "m", alice) + .await + .unwrap(); + Fx { + pool, + ws: ws.id, + doc, + alice: (alice, "Alice".into()), + } +} + +async fn add_user(pool: &sqlx::PgPool, ws: Uuid, email: &str, name: &str) -> Uuid { + let u = PgUserStore::new(pool.clone()) + .create_local(email, name, "$h$") + .await + .unwrap(); + PgWorkspaceStore::new(pool.clone()) + .add_member(ws, u.id, WorkspaceRole::Editor) + .await + .unwrap(); + u.id +} + +/// Record a content change by `user` the way the room writer does. +async fn edit(pool: &sqlx::PgPool, doc: Uuid, user: Uuid) { + PgUpdatesStore::new(pool.clone()) + .insert_batch(doc, &[(Some(user), vec![0u8])]) + .await + .unwrap(); +} + +/// Pin a contributor's timestamps so ordering is deterministic. +async fn set_times( + pool: &sqlx::PgPool, + doc: Uuid, + user: Uuid, + first: DateTime, + last: DateTime, +) { + let n = sqlx::query( + "UPDATE doc_contributors SET first_edited_at = $3, last_edited_at = $4 + WHERE doc_id = $1 AND user_id = $2", + ) + .bind(doc) + .bind(user) + .bind(first) + .bind(last) + .execute(pool) + .await + .unwrap() + .rows_affected(); + assert_eq!(n, 1, "fixture: {user} is not a contributor"); +} + +fn at(h: u32) -> DateTime { + Utc.with_ymd_and_hms(2026, 3, 3, h, 0, 0).unwrap() +} + +#[tokio::test(flavor = "multi_thread")] +async fn byline_lists_creator_and_contributors_most_recent_first() { + let fx = fixture().await; + let bob = add_user(&fx.pool, fx.ws, "bob@x.test", "Bob").await; + let carol = add_user(&fx.pool, fx.ws, "carol@x.test", "Carol").await; + let dave = add_user(&fx.pool, fx.ws, "dave@x.test", "Dave").await; + for u in [dave, bob, carol] { + edit(&fx.pool, fx.doc.id, u).await; + } + // Carol edited last; Bob and Dave tie, so display_name breaks it. + set_times(&fx.pool, fx.doc.id, bob, at(9), at(11)).await; + set_times(&fx.pool, fx.doc.id, carol, at(10), at(12)).await; + set_times(&fx.pool, fx.doc.id, dave, at(8), at(11)).await; + + let b = PgContributorStore::new(fx.pool.clone()) + .byline(fx.doc.id) + .await + .unwrap() + .expect("doc exists"); + + assert_eq!((b.created_by.id, b.created_by.display_name), fx.alice); + assert_eq!(b.created_at, fx.doc.created_at); + assert_eq!( + b.contributors_since, fx.doc.created_at, + "a doc created after tracking began is tracked from its creation" + ); + let c = |user_id: Uuid, name: &str, first, last| Contributor { + user_id, + display_name: name.into(), + first_edited_at: first, + last_edited_at: last, + }; + assert_eq!( + b.contributors, + vec![ + c(carol, "Carol", at(10), at(12)), + c(bob, "Bob", at(9), at(11)), + c(dave, "Dave", at(8), at(11)), + ], + "ordered by last_edited_at DESC, then display_name ASC" + ); +} + +/// Credit outlives membership: someone removed from the workspace still +/// wrote what they wrote, and the byline still names them. +#[tokio::test(flavor = "multi_thread")] +async fn byline_resolves_a_removed_member_by_name() { + let fx = fixture().await; + let bob = add_user(&fx.pool, fx.ws, "bob@x.test", "Bob").await; + edit(&fx.pool, fx.doc.id, bob).await; + PgWorkspaceStore::new(fx.pool.clone()) + .remove_member(fx.ws, bob) + .await + .unwrap(); + + let b = PgContributorStore::new(fx.pool.clone()) + .byline(fx.doc.id) + .await + .unwrap() + .expect("doc exists"); + let names: Vec<_> = b + .contributors + .iter() + .map(|c| (c.user_id, c.display_name.as_str())) + .collect(); + assert_eq!(names, vec![(bob, "Bob")]); +} + +#[tokio::test(flavor = "multi_thread")] +async fn byline_of_a_doc_nobody_edited_has_no_contributors() { + let fx = fixture().await; + let b = PgContributorStore::new(fx.pool.clone()) + .byline(fx.doc.id) + .await + .unwrap() + .expect("doc exists"); + assert!(b.contributors.is_empty()); + assert_eq!(b.created_by.id, fx.alice.0); +} + +#[tokio::test(flavor = "multi_thread")] +async fn byline_of_unknown_doc_is_none() { + let fx = fixture().await; + let b = PgContributorStore::new(fx.pool.clone()) + .byline(Uuid::new_v4()) + .await + .unwrap(); + assert!(b.is_none()); +} diff --git a/crates/knot-storage/tests/migrations_apply.rs b/crates/knot-storage/tests/migrations_apply.rs index 68676d8..7c7f72a 100644 --- a/crates/knot-storage/tests/migrations_apply.rs +++ b/crates/knot-storage/tests/migrations_apply.rs @@ -32,6 +32,7 @@ async fn migrations_apply_cleanly() { "boards", "comment_reactions", "comments", + "doc_contributors", "doc_markdown_cache", "doc_snapshots", "doc_tasks", @@ -50,3 +51,124 @@ async fn migrations_apply_cleanly() { "v0.1 schema must define exactly these tables" ); } + +/// The doc_contributors migration backfills from the attribution already in +/// `doc_updates`: one row per (doc, author) spanning that author's first and +/// last surviving update. Anonymous rows (`by_user_id IS NULL`) credit nobody. +/// Existing docs get `contributors_since` = migration time, since history +/// before that is only partially attributed (live edits never were). +#[tokio::test(flavor = "multi_thread")] +async fn doc_contributors_migration_backfills_from_doc_updates() { + use chrono::{DateTime, TimeZone, Utc}; + use sqlx::migrate::Migrator; + use uuid::Uuid; + + const VERSION: i64 = 20261003120000; + + let url = knot_test_support::fresh_db_url().await; + let pool = sqlx::postgres::PgPoolOptions::new() + .max_connections(2) + .connect(&url) + .await + .expect("connect"); + let full = sqlx::migrate!("../../migrations"); + assert!( + full.migrations.iter().any(|m| m.version == VERSION), + "migration {VERSION} not embedded (cargo clean -p knot-storage?)" + ); + let before = Migrator { + migrations: std::borrow::Cow::Owned( + full.migrations + .iter() + .filter(|m| m.version < VERSION) + .cloned() + .collect(), + ), + ..Migrator::DEFAULT + }; + before + .run(&pool) + .await + .expect("migrate to previous version"); + + // Seed the pre-migration world with raw SQL (the stores already speak the + // new schema). + let ws: Uuid = sqlx::query_scalar( + "INSERT INTO workspaces (slug, name) VALUES ('default', 'W') RETURNING id", + ) + .fetch_one(&pool) + .await + .unwrap(); + let mut users = Vec::new(); + for (email, name) in [("a@x.test", "Alice"), ("b@x.test", "Bob")] { + let id: Uuid = sqlx::query_scalar( + "INSERT INTO users (email, display_name) VALUES ($1, $2) RETURNING id", + ) + .bind(email) + .bind(name) + .fetch_one(&pool) + .await + .unwrap(); + users.push(id); + } + let (alice, bob) = (users[0], users[1]); + let doc: Uuid = sqlx::query_scalar( + "INSERT INTO documents (workspace_id, title, sort_key, created_by, created_at) + VALUES ($1, 'D', 'm', $2, '2026-01-01T00:00:00Z') RETURNING id", + ) + .bind(ws) + .bind(alice) + .fetch_one(&pool) + .await + .unwrap(); + let t = |h: u32| -> DateTime { Utc.with_ymd_and_hms(2026, 3, 3, h, 0, 0).unwrap() }; + for (by, at) in [ + (Some(alice), t(10)), + (Some(bob), t(11)), + (None, t(12)), + (Some(alice), t(13)), + ] { + sqlx::query( + "INSERT INTO doc_updates (doc_id, update_bytes, by_user_id, created_at) + VALUES ($1, '\\x00', $2, $3)", + ) + .bind(doc) + .bind(by) + .bind(at) + .execute(&pool) + .await + .unwrap(); + } + + let migrated_from: DateTime = sqlx::query_scalar("SELECT now()") + .fetch_one(&pool) + .await + .unwrap(); + full.run(&pool).await.expect("apply doc_contributors"); + + let mut rows: Vec<(Uuid, DateTime, DateTime)> = sqlx::query_as( + "SELECT user_id, first_edited_at, last_edited_at + FROM doc_contributors WHERE doc_id = $1", + ) + .bind(doc) + .fetch_all(&pool) + .await + .unwrap(); + rows.sort_by_key(|r| r.1); + assert_eq!( + rows, + vec![(alice, t(10), t(13)), (bob, t(11), t(11))], + "backfill must yield one row per author spanning first..last update" + ); + + let since: DateTime = + sqlx::query_scalar("SELECT contributors_since FROM documents WHERE id = $1") + .bind(doc) + .fetch_one(&pool) + .await + .unwrap(); + assert!( + since >= migrated_from, + "existing docs must get the migration time as contributors_since, got {since}" + ); +} diff --git a/crates/knot-storage/tests/updates.rs b/crates/knot-storage/tests/updates.rs index 26739dc..ec2144e 100644 --- a/crates/knot-storage/tests/updates.rs +++ b/crates/knot-storage/tests/updates.rs @@ -27,8 +27,12 @@ async fn setup() -> (PgUpdatesStore, uuid::Uuid, uuid::Uuid) { #[tokio::test(flavor = "multi_thread")] async fn insert_batch_returns_monotone_seqs_in_input_order() { let (s, doc, user) = setup().await; - let batch = vec![vec![1u8, 2, 3], vec![4u8, 5], vec![6u8]]; - let seqs = s.insert_batch(doc, Some(user), &batch).await.unwrap(); + let batch = vec![ + (Some(user), vec![1u8, 2, 3]), + (Some(user), vec![4u8, 5]), + (Some(user), vec![6u8]), + ]; + let seqs = s.insert_batch(doc, &batch).await.unwrap(); assert_eq!(seqs.len(), 3); assert!(seqs[0] < seqs[1] && seqs[1] < seqs[2], "got {seqs:?}"); } @@ -37,7 +41,14 @@ async fn insert_batch_returns_monotone_seqs_in_input_order() { async fn since_returns_after_watermark_in_order() { let (s, doc, user) = setup().await; let seqs = s - .insert_batch(doc, Some(user), &[vec![1u8], vec![2u8], vec![3u8]]) + .insert_batch( + doc, + &[ + (Some(user), vec![1u8]), + (Some(user), vec![2u8]), + (Some(user), vec![3u8]), + ], + ) .await .unwrap(); let after = seqs[0]; @@ -53,7 +64,7 @@ async fn max_seq_zero_when_empty_then_grows() { let (s, doc, user) = setup().await; assert_eq!(s.max_seq(doc).await.unwrap(), 0); let seqs = s - .insert_batch(doc, Some(user), &[vec![1u8], vec![2u8]]) + .insert_batch(doc, &[(Some(user), vec![1u8]), (Some(user), vec![2u8])]) .await .unwrap(); assert_eq!(s.max_seq(doc).await.unwrap(), *seqs.last().unwrap()); @@ -63,7 +74,14 @@ async fn max_seq_zero_when_empty_then_grows() { async fn delete_up_to_removes_inclusive() { let (s, doc, user) = setup().await; let seqs = s - .insert_batch(doc, Some(user), &[vec![1u8], vec![2u8], vec![3u8]]) + .insert_batch( + doc, + &[ + (Some(user), vec![1u8]), + (Some(user), vec![2u8]), + (Some(user), vec![3u8]), + ], + ) .await .unwrap(); let n = s.delete_up_to(doc, seqs[1]).await.unwrap(); @@ -76,7 +94,187 @@ async fn delete_up_to_removes_inclusive() { #[tokio::test(flavor = "multi_thread")] async fn empty_batch_is_noop() { let (s, doc, _) = setup().await; - let seqs = s.insert_batch(doc, None, &[]).await.unwrap(); + let seqs = s.insert_batch(doc, &[]).await.unwrap(); assert!(seqs.is_empty()); assert_eq!(s.max_seq(doc).await.unwrap(), 0); } + +// --------------------------------------------------------------------------- +// Per-row authors + doc_contributors +// --------------------------------------------------------------------------- + +struct Authors { + s: PgUpdatesStore, + pool: sqlx::PgPool, + doc: uuid::Uuid, + alice: uuid::Uuid, + bob: uuid::Uuid, +} + +async fn two_authors() -> Authors { + let pool = knot_test_support::fresh_db().await.pool; + let ws = PgWorkspaceStore::new(pool.clone()) + .create("default", "W") + .await + .unwrap(); + let users = PgUserStore::new(pool.clone()); + let alice = users + .create_local("a@x.test", "Alice", "$h$") + .await + .unwrap(); + let bob = users.create_local("b@x.test", "Bob", "$h$").await.unwrap(); + for u in [&alice, &bob] { + PgWorkspaceStore::new(pool.clone()) + .add_member(ws.id, u.id, WorkspaceRole::Editor) + .await + .unwrap(); + } + let d = PgDocStore::new(pool.clone()) + .create(ws.id, None, "D", "m", alice.id) + .await + .unwrap(); + Authors { + s: PgUpdatesStore::new(pool.clone()), + pool, + doc: d.id, + alice: alice.id, + bob: bob.id, + } +} + +type ContributorRow = ( + uuid::Uuid, + chrono::DateTime, + chrono::DateTime, +); + +async fn contributors(pool: &sqlx::PgPool, doc: uuid::Uuid) -> Vec { + sqlx::query_as( + "SELECT user_id, first_edited_at, last_edited_at + FROM doc_contributors WHERE doc_id = $1 ORDER BY user_id", + ) + .bind(doc) + .fetch_all(pool) + .await + .unwrap() +} + +/// The writer batches whatever arrived in a 250 ms window. With live edits +/// attributed, two people typing at once land in one batch — each row must +/// keep its own author, and seqs must still line up with the input order. +#[tokio::test(flavor = "multi_thread")] +async fn insert_batch_keeps_each_rows_own_author() { + let a = two_authors().await; + let batch = vec![ + (Some(a.alice), vec![1u8]), + (Some(a.bob), vec![2u8]), + (None, vec![3u8]), + (Some(a.alice), vec![4u8]), + ]; + let seqs = a.s.insert_batch(a.doc, &batch).await.unwrap(); + let rows = a.s.since(a.doc, 0).await.unwrap(); + let got: Vec<_> = rows + .iter() + .map(|r| (r.seq, r.by_user_id, r.update_bytes.clone())) + .collect(); + let want: Vec<_> = seqs + .iter() + .zip(&batch) + .map(|(seq, (by, bytes))| (*seq, *by, bytes.clone())) + .collect(); + assert_eq!(got, want, "rows must keep their own author, in input order"); +} + +#[tokio::test(flavor = "multi_thread")] +async fn insert_batch_records_one_contributor_per_distinct_author() { + let a = two_authors().await; + a.s.insert_batch( + a.doc, + &[ + (Some(a.alice), vec![1u8]), + (Some(a.bob), vec![2u8]), + (None, vec![3u8]), + (Some(a.alice), vec![4u8]), + ], + ) + .await + .unwrap(); + let rows = contributors(&a.pool, a.doc).await; + let mut who: Vec<_> = rows.iter().map(|r| r.0).collect(); + let mut want = vec![a.alice, a.bob]; + who.sort(); + want.sort(); + assert_eq!(who, want, "one doc_contributors row per distinct author"); + for (user, first, last) in &rows { + assert_eq!( + first, last, + "a new contributor starts with first = last ({user})" + ); + } +} + +#[tokio::test(flavor = "multi_thread")] +async fn anonymous_batch_records_no_contributor() { + let a = two_authors().await; + a.s.insert_batch(a.doc, &[(None, vec![1u8]), (None, vec![2u8])]) + .await + .unwrap(); + assert!( + contributors(&a.pool, a.doc).await.is_empty(), + "unattributed updates must not create contributors" + ); +} + +/// Continuous typing flushes every 250 ms; refreshing last_edited_at on each +/// flush would rewrite the row four times a second for no visible gain. It +/// is refreshed only once the stored value is more than a minute old. +#[tokio::test(flavor = "multi_thread")] +async fn contributor_last_edited_at_refresh_is_throttled_to_once_a_minute() { + let a = two_authors().await; + let one_edit = [(Some(a.alice), vec![1u8])]; + let edit = || a.s.insert_batch(a.doc, &one_edit); + edit().await.unwrap(); + + // Backdate to just inside the window: the next edit must leave it alone. + let (recent, first): (chrono::DateTime, chrono::DateTime) = + sqlx::query_as( + "UPDATE doc_contributors + SET last_edited_at = now() - interval '30 seconds', + first_edited_at = now() - interval '1 day' + WHERE doc_id = $1 AND user_id = $2 + RETURNING last_edited_at, first_edited_at", + ) + .bind(a.doc) + .bind(a.alice) + .fetch_one(&a.pool) + .await + .unwrap(); + edit().await.unwrap(); + let rows = contributors(&a.pool, a.doc).await; + assert_eq!( + rows, + vec![(a.alice, first, recent)], + "an edit within a minute of last_edited_at must not rewrite the row" + ); + + // Backdate past the window: the next edit refreshes last_edited_at only. + let stale: chrono::DateTime = sqlx::query_scalar( + "UPDATE doc_contributors SET last_edited_at = now() - interval '61 seconds' + WHERE doc_id = $1 AND user_id = $2 + RETURNING last_edited_at", + ) + .bind(a.doc) + .bind(a.alice) + .fetch_one(&a.pool) + .await + .unwrap(); + edit().await.unwrap(); + let rows = contributors(&a.pool, a.doc).await; + assert_eq!(rows.len(), 1); + let (_, got_first, got_last) = rows[0]; + assert_eq!(got_first, first, "first_edited_at must never move"); + assert!( + got_last > stale + chrono::Duration::seconds(60), + "a stale last_edited_at must be refreshed to now(): {stale} -> {got_last}" + ); +} diff --git a/e2e/flows/contributors.spec.ts b/e2e/flows/contributors.spec.ts new file mode 100644 index 0000000..b4d5deb --- /dev/null +++ b/e2e/flows/contributors.spec.ts @@ -0,0 +1,127 @@ +import { expect, test, type Browser, type Page } from "@playwright/test"; + +import { docText } from "../support/docText"; +import { reset } from "../support/reset"; + +test.beforeAll(reset); + +const editorOf = (page: Page) => page.locator("[data-testid='editor-host'] .ProseMirror"); + +async function signIn(browser: Browser, email: string, password: string): Promise { + const ctx = await browser.newContext(); + const page = await ctx.newPage(); + await page.goto("/login"); + await page.getByTestId("login-email").fill(email); + await page.getByTestId("login-password").fill(password); + await page.getByTestId("login-submit").click(); + await page.waitForURL(/\/(?:doc\/.+)?$/, { timeout: 5_000 }); + return page; +} + +async function invite(page: Page, email: string, role: "editor" | "viewer", password: string) { + await page.goto("/members"); + await page.getByTestId("invite-email").fill(email); + await page.getByTestId("invite-role").selectOption(role); + await page.getByTestId("invite-password").fill(password); + await page.getByTestId("invite-submit").click(); + await expect(page.getByTestId("members-table").locator("tr", { hasText: email })).toHaveCount(1, { + timeout: 5_000, + }); +} + +/** Open the contributors popover (which refetches) and return its text. */ +async function contributorsText(page: Page): Promise { + const button = page.getByTestId("doc-contributors-button"); + const popover = page.getByTestId("doc-contributors-popover"); + if (await popover.isVisible()) await button.click(); + await button.click(); + await expect(popover).toBeVisible(); + // Opening refetches; give the fresh list a moment to replace the cached one. + await page.waitForLoadState("networkidle"); + return (await popover.innerText()).toLowerCase(); +} + +test("byline names the creator and lists only the people who edited the content", async ({ browser }) => { + // Alice sets up the workspace and writes the first lines of a page. + const aliceCtx = await browser.newContext(); + const alice = await aliceCtx.newPage(); + await alice.goto("/setup"); + await alice.getByTestId("setup-email").fill("alice@example.com"); + await alice.getByTestId("setup-display-name").fill("Alice"); + await alice.getByTestId("setup-password").fill("alice-hunter22"); + await alice.getByTestId("setup-submit").click(); + await alice.getByTestId("new-doc").click(); + await alice.waitForSelector("[data-testid='new-doc-modal']", { state: "visible", timeout: 5_000 }); + await alice.getByTestId("new-doc-blank").click(); + await alice.waitForURL(/\/doc\/.+/); + const docUrl = alice.url(); + await expect(alice.getByTestId("status-dot")).toHaveAttribute("data-status", "connected", { timeout: 10_000 }); + + await expect(alice.getByTestId("doc-byline-creator")).toContainText("Created by Alice"); + // Nobody has edited yet, so there is nothing to list. + await expect(alice.getByTestId("doc-contributors-button")).toHaveCount(0); + + await editorOf(alice).click(); + await alice.keyboard.type("Written by Alice. "); + + // Live edits are persisted in 250 ms batches; the byline is fetched on load. + await expect(async () => { + await alice.reload(); + await expect(alice.getByTestId("doc-contributors-button")).toContainText("1 contributor", { timeout: 2_000 }); + }).toPass({ timeout: 15_000 }); + expect(await contributorsText(alice)).toContain("alice"); + + await invite(alice, "bob@example.com", "editor", "bob-hunter22"); + await invite(alice, "vera@example.com", "viewer", "vera-hunter22"); + await alice.goto(docUrl); + + // Bob, an editor, opens the page and only watches. + const bob = await signIn(browser, "bob@example.com", "bob-hunter22"); + await bob.goto(docUrl); + await expect(bob.getByTestId("status-dot")).toHaveAttribute("data-status", "connected", { timeout: 10_000 }); + await expect.poll(() => editorOf(bob).evaluate(docText), { timeout: 8_000 }).toMatch(/Written by Alice\./); + + // While Bob watches, Alice types — including a URL, so autolink runs (a bare + // www. domain: typing "//" opens knot's date picker). Watching must never + // credit Bob: not the handshake his editor answered on connect, and not any + // edit his editor might send in reaction to hers. (Alice's own update + // already carries the link mark, so this path does not exercise the + // KnotLink remote-transaction guard; autolink.test.ts covers that.) + await expect(alice.getByTestId("status-dot")).toHaveAttribute("data-status", "connected", { timeout: 10_000 }); + await editorOf(alice).click(); + await alice.keyboard.press("End"); + await alice.keyboard.type("See www.example.com and more."); + await expect(editorOf(alice).locator("a[href*='example.com']")).toHaveCount(1, { timeout: 5_000 }); + await expect(editorOf(bob).locator("a[href*='example.com']")).toHaveCount(1, { timeout: 8_000 }); + // A phantom edit from Bob's editor would be sent immediately and persisted + // within one 250 ms writer batch. Wait well past that before asserting it + // never happened. + await bob.waitForTimeout(2_000); + + const afterWatching = await contributorsText(alice); + expect(afterWatching).toContain("alice"); + expect(afterWatching, "Bob only had the page open; he must not be listed").not.toContain("bob"); + await expect(alice.getByTestId("doc-contributors-button")).toContainText("1 contributor"); + + // Now Bob edits, and becomes a contributor. + await editorOf(bob).click(); + await bob.keyboard.press("End"); + await bob.keyboard.type(" Bob was here."); + await expect.poll(() => editorOf(alice).evaluate(docText), { timeout: 8_000 }).toMatch(/Bob was here\./); + await expect.poll(() => contributorsText(alice), { timeout: 10_000 }).toContain("bob"); + await expect(alice.getByTestId("doc-contributors-button")).toContainText("2 contributors"); + + // A viewer sees the byline too, and is not listed for having looked. + const vera = await signIn(browser, "vera@example.com", "vera-hunter22"); + await vera.goto(docUrl); + await expect(vera.getByTestId("doc-byline-creator")).toContainText("Created by Alice"); + await expect(vera.getByTestId("doc-contributors-button")).toContainText("2 contributors"); + const seenByViewer = await contributorsText(vera); + expect(seenByViewer).toContain("alice"); + expect(seenByViewer).toContain("bob"); + expect(seenByViewer).not.toContain("vera"); + + await aliceCtx.close(); + await bob.context().close(); + await vera.context().close(); +}); diff --git a/e2e/flows/ws-reconnect.spec.ts b/e2e/flows/ws-reconnect.spec.ts index 072e2e1..4ec543e 100644 --- a/e2e/flows/ws-reconnect.spec.ts +++ b/e2e/flows/ws-reconnect.spec.ts @@ -1,5 +1,6 @@ -import { expect, test } from "@playwright/test"; +import { expect, test, type Page } from "@playwright/test"; +import { docText } from "../support/docText"; import { reset } from "../support/reset"; const TOXIPROXY = "http://localhost:8474"; @@ -16,13 +17,15 @@ async function setProxyEnabled(enabled: boolean): Promise { if (!r.ok) throw new Error(`toxiproxy enabled=${enabled}: ${r.status} ${await r.text()}`); } -test.beforeAll(async () => { +// Per test: each one bootstraps through /setup, which only works on an empty +// database. +test.beforeEach(async () => { reset(); await setProxyEnabled(true); }); test.afterEach(async () => { await setProxyEnabled(true); }); -test("editor reconnects after a forced WS flap; content preserved", async ({ page }) => { +async function setupAndOpenBlankDoc(page: Page): Promise { await page.goto("/setup"); await page.getByTestId("setup-email").fill("o@e.com"); await page.getByTestId("setup-display-name").fill("O"); @@ -33,6 +36,10 @@ test("editor reconnects after a forced WS flap; content preserved", async ({ pag await page.getByTestId("new-doc-blank").click(); await page.waitForURL(/\/doc\/.+/); await expect(page.getByTestId("status-dot")).toHaveAttribute("data-status", "connected", { timeout: 10_000 }); +} + +test("editor reconnects after a forced WS flap; content preserved", async ({ page }) => { + await setupAndOpenBlankDoc(page); const editor = page.locator("[data-testid='editor-host'] .ProseMirror"); await editor.click(); @@ -51,3 +58,31 @@ test("editor reconnects after a forced WS flap; content preserved", async ({ pag // Y.Doc never lost state. await expect(editor).toContainText("Before the flap."); }); + +test("edits typed while disconnected reach the server after reconnect", async ({ page, context }) => { + await setupAndOpenBlankDoc(page); + const docUrl = page.url(); + + const editor = page.locator("[data-testid='editor-host'] .ProseMirror"); + await editor.click(); + await page.keyboard.type("Before the outage."); + await page.waitForTimeout(300); + + await setProxyEnabled(false); + await expect(page.getByTestId("status-dot")).toHaveAttribute("data-status", "offline", { timeout: 5_000 }); + + // The editor stays editable while offline; these keystrokes exist only in + // this tab's Y.Doc until the connection comes back. + await page.keyboard.type(" Typed offline."); + await expect(editor).toContainText("Typed offline."); + + await setProxyEnabled(true); + await expect(page.getByTestId("status-dot")).toHaveAttribute("data-status", "connected", { timeout: 10_000 }); + + // A second tab loads the doc from the server: it only sees the offline + // text if the reconnecting tab uploaded it. + const second = await context.newPage(); + await second.goto(docUrl); + const secondEditor = second.locator("[data-testid='editor-host'] .ProseMirror"); + await expect.poll(() => secondEditor.evaluate(docText), { timeout: 10_000 }).toMatch(/Before the outage\. Typed offline\./); +}); diff --git a/e2e/support/reset.ts b/e2e/support/reset.ts index 4740e07..a0d9021 100644 --- a/e2e/support/reset.ts +++ b/e2e/support/reset.ts @@ -42,6 +42,7 @@ const TABLES = [ "board_snapshots", "board_updates", "boards", + "doc_contributors", "doc_markdown_cache", "doc_snapshots", "doc_tasks", diff --git a/migrations/20261003120000_doc_contributors.sql b/migrations/20261003120000_doc_contributors.sql new file mode 100644 index 0000000..c9d4cdb --- /dev/null +++ b/migrations/20261003120000_doc_contributors.sql @@ -0,0 +1,43 @@ +-- doc_contributors +-- Created 2026-10-03 +-- +-- Who has changed a page's CONTENT, for the doc byline ("created by …, +-- edited by …"). Every content change — live typing over the collab socket, +-- markdown import, history restore, task checkbox toggles, create-from- +-- template, workspace import — is persisted as a `doc_updates` row by the +-- room's writer, and the writer upserts this table in the same statement. +-- Renames, moves, permission changes and comments never touch doc_updates +-- and so never make anyone a contributor. +-- +-- One row per (doc, user) instead of deriving the list from doc_updates: +-- snapshot GC deletes old doc_updates rows, so the log alone forgets early +-- contributors. last_edited_at is refreshed at most once per minute (see +-- PgUpdatesStore::insert_batch) so continuous typing does not rewrite the row +-- on every 250 ms writer flush. +-- +-- No ON DELETE CASCADE on user_id, matching doc_updates.by_user_id: a +-- removed workspace member keeps their user row and their credit. +CREATE TABLE doc_contributors ( + doc_id uuid NOT NULL REFERENCES documents(id) ON DELETE CASCADE, + user_id uuid NOT NULL REFERENCES users(id), + first_edited_at timestamptz NOT NULL DEFAULT now(), + last_edited_at timestamptz NOT NULL DEFAULT now(), + PRIMARY KEY (doc_id, user_id) +); + +-- The point from which the contributor list is complete. Before this +-- migration live WS edits were stored without an author, so for existing docs +-- the list only knows about attributed changes (imports, restores, task +-- toggles …) — the UI shows "since " for them. now() is +-- evaluated once when the column is added, so every existing row gets the +-- migration time; every doc created afterwards gets its own creation time. +ALTER TABLE documents + ADD COLUMN contributors_since timestamptz NOT NULL DEFAULT now(); + +-- Backfill from whatever attribution doc_updates still holds (rows that +-- survived snapshot GC and carry an author). +INSERT INTO doc_contributors (doc_id, user_id, first_edited_at, last_edited_at) +SELECT doc_id, by_user_id, min(created_at), max(created_at) +FROM doc_updates +WHERE by_user_id IS NOT NULL +GROUP BY doc_id, by_user_id; diff --git a/web/src/features/boards/BoardProvider.test.ts b/web/src/features/boards/BoardProvider.test.ts new file mode 100644 index 0000000..23f9d85 --- /dev/null +++ b/web/src/features/boards/BoardProvider.test.ts @@ -0,0 +1,52 @@ +/** + * Boards share the editor's offline path: BoardProvider forwards edits only + * while its socket is open, so a stroke drawn offline reaches the server only + * when, on reconnect, the server asks with SYNC_STEP_1 and the provider + * answers with SYNC_STEP_2 (crates/knot-server/tests/offline_sync.rs drives + * the server half). This pins the provider's answer. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import * as Y from "yjs"; + +import { FakeSocket } from "../../test/fakeSocket"; + +import { BoardProvider } from "./BoardProvider"; + +beforeEach(() => { + FakeSocket.all = []; + vi.useFakeTimers(); + vi.stubGlobal("WebSocket", FakeSocket); +}); +afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); +}); + +function handshake(server: Y.Doc): FakeSocket { + const ws = FakeSocket.all.at(-1)!; + ws.open(); + ws.serverState(server); + ws.serverAsks(server); + return ws; +} + +describe("BoardProvider offline edits", () => { + it("hands over an element added while offline when the server asks", () => { + const server = new Y.Doc(); + server.getMap("elements").set("a", { id: "a" }); + const doc = new Y.Doc(); + const p = new BoardProvider({ url: "ws://x/boards", doc }); + handshake(server); + expect(p.synced).toBe(true); + + FakeSocket.all.at(-1)!.drop(); + doc.getMap("elements").set("b", { id: "b" }); + vi.advanceTimersByTime(31_000); + const ws2 = handshake(server); + + for (const u of ws2.edits()) Y.applyUpdate(server, u); + expect([...server.getMap("elements").keys()].sort()).toEqual(["a", "b"]); + p.destroy(); + }); +}); diff --git a/web/src/features/comments/CommentThread.tsx b/web/src/features/comments/CommentThread.tsx index 14b57e5..e307a0b 100644 --- a/web/src/features/comments/CommentThread.tsx +++ b/web/src/features/comments/CommentThread.tsx @@ -6,18 +6,11 @@ import { useEffectiveRole } from "../../auth/useEffectiveRole"; import { useSession } from "../../auth/SessionContext"; import { Avatar } from "../../components/ui/Avatar"; import { commentsApi, ALLOWED_EMOJIS, type Comment } from "../../lib/comments.api"; +import { relTime } from "../../lib/relTime"; import { workspaceApi } from "../workspace/workspace.api"; import { useUi } from "../../stores/ui"; import { CommentComposer } from "./CommentComposer"; -function relTime(iso: string): string { - const diff = Date.now() - new Date(iso).getTime(); - if (diff < 60_000) return "just now"; - if (diff < 3_600_000) return `${Math.floor(diff / 60_000)}m ago`; - if (diff < 86_400_000) return `${Math.floor(diff / 3_600_000)}h ago`; - return `${Math.floor(diff / 86_400_000)}d ago`; -} - // --------------------------------------------------------------------------- // Reaction row // --------------------------------------------------------------------------- diff --git a/web/src/features/docs/DocByline.test.tsx b/web/src/features/docs/DocByline.test.tsx new file mode 100644 index 0000000..dd2be23 --- /dev/null +++ b/web/src/features/docs/DocByline.test.tsx @@ -0,0 +1,238 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { cleanup, fireEvent, render, screen, waitFor, within } from "@testing-library/react"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import type { DocContributors } from "../../lib/validators"; + +import { DocByline } from "./DocByline"; + +const contributors = vi.fn<(id: string) => Promise>(); + +vi.mock("./docs.api", () => ({ + docsApi: { contributors: (id: string) => contributors(id) }, +})); + +const CREATED = "2026-03-03T10:00:00Z"; + +function body(over: Partial = {}): DocContributors { + return { + created_by: { id: "u-alice", display_name: "Alice" }, + created_at: CREATED, + // Same instant as creation: tracking covered the doc's whole life. + contributors_since: CREATED, + contributors: [], + ...over, + }; +} + +function person(id: string, name: string, lastEdited = "2026-10-03T11:00:00Z") { + return { + user_id: id, + display_name: name, + first_edited_at: "2026-10-01T09:00:00Z", + last_edited_at: lastEdited, + }; +} + +const DATE_OPTS: Intl.DateTimeFormatOptions = { day: "numeric", month: "short", year: "numeric" }; + +function renderByline() { + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render( + + +

outside

+ , + ); +} + +beforeEach(() => { + contributors.mockReset(); +}); +afterEach(() => cleanup()); + +describe("DocByline", () => { + it("names the creator and the creation date", async () => { + contributors.mockResolvedValue({ ok: body() }); + renderByline(); + + const creator = await screen.findByTestId("doc-byline-creator"); + expect(creator).toHaveTextContent( + `Created by Alice · ${new Date(CREATED).toLocaleDateString(undefined, DATE_OPTS)}`, + ); + expect(contributors).toHaveBeenCalledWith("d1"); + }); + + it("shows no contributors button when nobody has edited the content", async () => { + contributors.mockResolvedValue({ ok: body() }); + renderByline(); + + await screen.findByTestId("doc-byline-creator"); + expect(screen.queryByTestId("doc-contributors-button")).toBeNull(); + expect(screen.getByTestId("doc-byline")).not.toHaveTextContent("contributor"); + }); + + it("says '1 contributor' for one", async () => { + contributors.mockResolvedValue({ ok: body({ contributors: [person("u-bob", "Bob")] }) }); + renderByline(); + + const button = await screen.findByTestId("doc-contributors-button"); + expect(button).toHaveTextContent(/1 contributor$/); + expect(button.querySelectorAll("span[title]")).toHaveLength(1); + }); + + it("caps the avatar stack at three but counts everyone", async () => { + contributors.mockResolvedValue({ + ok: body({ + contributors: [ + person("u-1", "Bob"), + person("u-2", "Carol"), + person("u-3", "Dan"), + person("u-4", "Erin"), + person("u-5", "Frank"), + ], + }), + }); + renderByline(); + + const button = await screen.findByTestId("doc-contributors-button"); + expect(button).toHaveTextContent(/5 contributors$/); + const avatars = [...button.querySelectorAll("span[title]")]; + expect(avatars).toHaveLength(3); + // The stack shows the most recent editors, which the server lists first. + expect(avatars.map((a) => a.getAttribute("title"))).toEqual(["Bob", "Carol", "Dan"]); + }); + + it("opens the list on click, in server order, and refetches it", async () => { + contributors.mockResolvedValue({ + ok: body({ + contributors: [ + person("u-zed", "Zed", "2026-10-03T11:58:00Z"), + person("u-amy", "Amy", "2026-10-03T09:00:00Z"), + ], + }), + }); + renderByline(); + + const button = await screen.findByTestId("doc-contributors-button"); + expect(button).toHaveAttribute("aria-expanded", "false"); + expect(screen.queryByTestId("doc-contributors-popover")).toBeNull(); + expect(contributors).toHaveBeenCalledTimes(1); + + fireEvent.click(button); + + const popover = screen.getByTestId("doc-contributors-popover"); + expect(button).toHaveAttribute("aria-expanded", "true"); + expect(button).toHaveAttribute("aria-controls", popover.id); + // Server order is kept even though it is not alphabetical. + const rows = within(popover).getAllByTestId(/^doc-contributor-/); + expect(rows.map((r) => r.dataset.testid)).toEqual([ + "doc-contributor-u-zed", + "doc-contributor-u-amy", + ]); + // Opening asks for a fresh list. + await waitFor(() => expect(contributors).toHaveBeenCalledTimes(2)); + }); + + it("shows when each contributor last edited, with the exact time on hover", async () => { + const last = new Date(Date.now() - 2 * 3_600_000).toISOString(); + contributors.mockResolvedValue({ ok: body({ contributors: [person("u-bob", "Bob", last)] }) }); + renderByline(); + + fireEvent.click(await screen.findByTestId("doc-contributors-button")); + + const row = screen.getByTestId("doc-contributor-u-bob"); + expect(row).toHaveTextContent("Bob"); + const when = within(row).getByText("edited 2h ago"); + expect(when).toHaveAttribute("title", new Date(last).toLocaleString()); + }); + + it("closes on Escape and hands focus back to the button", async () => { + contributors.mockResolvedValue({ ok: body({ contributors: [person("u-bob", "Bob")] }) }); + renderByline(); + + const button = await screen.findByTestId("doc-contributors-button"); + fireEvent.click(button); + expect(screen.getByTestId("doc-contributors-popover")).toBeInTheDocument(); + + fireEvent.keyDown(document, { key: "Escape" }); + + expect(screen.queryByTestId("doc-contributors-popover")).toBeNull(); + expect(button).toHaveAttribute("aria-expanded", "false"); + expect(button).toHaveFocus(); + }); + + it("closes on a click outside, but not on a click inside", async () => { + contributors.mockResolvedValue({ ok: body({ contributors: [person("u-bob", "Bob")] }) }); + renderByline(); + + fireEvent.click(await screen.findByTestId("doc-contributors-button")); + const popover = screen.getByTestId("doc-contributors-popover"); + + fireEvent.mouseDown(within(popover).getByTestId("doc-contributor-u-bob")); + expect(screen.getByTestId("doc-contributors-popover")).toBeInTheDocument(); + + fireEvent.mouseDown(screen.getByTestId("elsewhere")); + expect(screen.queryByTestId("doc-contributors-popover")).toBeNull(); + }); + + it("toggles closed when the button is clicked again", async () => { + contributors.mockResolvedValue({ ok: body({ contributors: [person("u-bob", "Bob")] }) }); + renderByline(); + + const button = await screen.findByTestId("doc-contributors-button"); + fireEvent.click(button); + // A real click is mousedown then click; the button must not count as + // "outside" or the two would cancel out and the popover would reopen. + fireEvent.mouseDown(button); + fireEvent.click(button); + expect(screen.queryByTestId("doc-contributors-popover")).toBeNull(); + }); + + it("footnotes the list when tracking started after the doc was created", async () => { + const since = "2026-10-03T12:00:00Z"; + contributors.mockResolvedValue({ + ok: body({ contributors_since: since, contributors: [person("u-bob", "Bob")] }), + }); + renderByline(); + + fireEvent.click(await screen.findByTestId("doc-contributors-button")); + + expect(screen.getByTestId("doc-contributors-popover")).toHaveTextContent( + `Edits before ${new Date(since).toLocaleDateString(undefined, DATE_OPTS)} aren't listed.`, + ); + }); + + it("has no footnote when tracking began with the doc (within a minute)", async () => { + contributors.mockResolvedValue({ + ok: body({ + // Row default vs. created_at can differ by a few seconds for a new doc. + contributors_since: "2026-03-03T10:00:30Z", + contributors: [person("u-bob", "Bob")], + }), + }); + renderByline(); + + fireEvent.click(await screen.findByTestId("doc-contributors-button")); + + expect(screen.getByTestId("doc-contributors-popover")).not.toHaveTextContent("Edits before"); + }); + + it("reserves its line while loading", () => { + contributors.mockReturnValue(new Promise(() => {})); + renderByline(); + + const line = screen.getByTestId("doc-byline"); + expect(line).toBeEmptyDOMElement(); + expect(line.className).toContain("min-h-5"); + }); + + it("renders nothing when the request fails", async () => { + contributors.mockResolvedValue({ + error: { code: "auth.forbidden", message: "no", details: {}, status: 403 }, + }); + renderByline(); + + await waitFor(() => expect(screen.queryByTestId("doc-byline")).toBeNull()); + }); +}); diff --git a/web/src/features/docs/DocByline.tsx b/web/src/features/docs/DocByline.tsx new file mode 100644 index 0000000..4beedbd --- /dev/null +++ b/web/src/features/docs/DocByline.tsx @@ -0,0 +1,156 @@ +import { useQuery } from "@tanstack/react-query"; +import { useEffect, useId, useRef, useState } from "react"; + +import { Avatar } from "../../components/ui/Avatar"; +import { relTime } from "../../lib/relTime"; +import type { DocContributor } from "../../lib/validators"; + +import { docsApi } from "./docs.api"; + +const DATE_OPTS: Intl.DateTimeFormatOptions = { day: "numeric", month: "short", year: "numeric" }; +const MAX_AVATARS = 3; +// `contributors_since` is the documents row's DEFAULT now() and `created_at` +// is stamped by the same insert, so for a doc created after tracking began the +// two agree to within a statement. Anything further apart means the doc +// predates tracking and its early edits were never attributed. +const TRACKING_GAP_MS = 60_000; + +function formatDate(iso: string): string { + return new Date(iso).toLocaleDateString(undefined, DATE_OPTS); +} + +/** + * "Created by Alice · 3 Mar 2026 · (avatars) 4 contributors", under the title. + * + * Shown to every role that can open the page. It is supporting information, + * so it never toasts: while loading it only holds its line (so the action row + * below does not jump when it arrives) and on error it renders nothing. + */ +export function DocByline({ docId }: { docId: string }) { + const [open, setOpen] = useState(false); + const buttonRef = useRef(null); + const popoverRef = useRef(null); + const popoverId = useId(); + + const q = useQuery({ + queryKey: ["contributors", docId], + queryFn: () => docsApi.contributors(docId), + }); + + useEffect(() => { + if (!open) return; + const onDown = (e: MouseEvent) => { + const t = e.target as Node; + // The button is excluded so its own click can toggle the popover shut; + // otherwise mousedown would close it and the click would reopen it. + if (popoverRef.current?.contains(t) || buttonRef.current?.contains(t)) return; + setOpen(false); + }; + const onKey = (e: KeyboardEvent) => { + if (e.key !== "Escape") return; + setOpen(false); + buttonRef.current?.focus(); + }; + document.addEventListener("mousedown", onDown); + document.addEventListener("keydown", onKey); + return () => { + document.removeEventListener("mousedown", onDown); + document.removeEventListener("keydown", onKey); + }; + }, [open]); + + // Same height as a line holding the sm avatars, so the loaded byline + // replaces the placeholder without shifting anything below it. + const lineCls = "relative mt-1 min-h-5 flex flex-wrap items-center gap-x-1.5 text-[13px] text-fg-muted"; + + if (q.isPending) return
; + if (!q.data || "error" in q.data) return null; + + const { created_by, created_at, contributors_since, contributors } = q.data.ok; + const n = contributors.length; + const showFootnote = + new Date(contributors_since).getTime() - new Date(created_at).getTime() > TRACKING_GAP_MS; + + function toggle() { + if (!open) void q.refetch(); + setOpen((v) => !v); + } + + return ( +
+ + Created by {created_by.display_name} · {formatDate(created_at)} + + {n > 0 && ( + <> + · + + + )} + {open && n > 0 && ( + + )} +
+ ); +} + +function ContributorRow({ c }: { c: DocContributor }) { + return ( +
  • + + {c.display_name} + + edited {relTime(c.last_edited_at)} + +
    • + ); +} diff --git a/web/src/features/docs/DocPage.byline.test.tsx b/web/src/features/docs/DocPage.byline.test.tsx new file mode 100644 index 0000000..e8b05ca --- /dev/null +++ b/web/src/features/docs/DocPage.byline.test.tsx @@ -0,0 +1,95 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { cleanup, render, screen } from "@testing-library/react"; +import { MemoryRouter, Route, Routes } from "react-router-dom"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { SessionProvider } from "../../auth/SessionContext"; + +// The editor is irrelevant here and needs a WebSocket; stub it out. +vi.mock("../editor/KnotEditor", () => ({ KnotEditor: () => null })); + +import DocPage from "./DocPage"; + +afterEach(() => { + cleanup(); + vi.unstubAllGlobals(); +}); + +function json(body: unknown, status = 200) { + return Promise.resolve( + new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }), + ); +} + +/** A server that knows one doc, d1, which the signed-in user can only view. */ +function viewerServer(url: string) { + if (url === "/auth/session") { + return json({ + user_id: "u-vic", + email: "vic@example.test", + display_name: "Vic", + workspace_id: "w1", + role: "viewer", + }); + } + if (url === "/api/docs/d1") { + return json({ + id: "d1", + workspace_id: "w1", + parent_id: null, + title: "Runbook", + sort_key: "a", + icon: null, + created_by: "u-alice", + archived: false, + is_template: false, + effective_role: "viewer", + }); + } + if (url === "/api/docs/d1/contributors") { + return json({ + created_by: { id: "u-alice", display_name: "Alice" }, + created_at: "2026-03-03T10:00:00Z", + contributors_since: "2026-03-03T10:00:00Z", + contributors: [ + { + user_id: "u-bob", + display_name: "Bob", + first_edited_at: "2026-10-01T09:00:00Z", + last_edited_at: "2026-10-03T11:00:00Z", + }, + ], + }); + } + return json({ error: { code: "not_found", message: url, details: {} } }, 404); +} + +describe("DocPage byline", () => { + it("shows the creator and contributors to a viewer, under the title", async () => { + vi.stubGlobal("fetch", vi.fn((url: string) => viewerServer(url))); + + render( + + + + + } /> + + + + , + ); + + const creator = await screen.findByTestId("doc-byline-creator"); + expect(creator).toHaveTextContent("Created by Alice"); + expect(await screen.findByTestId("doc-contributors-button")).toHaveTextContent("1 contributor"); + // Directly beneath the title, above the action row. + const title = screen.getByTestId("doc-title"); + const byline = screen.getByTestId("doc-byline"); + expect(title.nextElementSibling).toBe(byline); + expect(byline.nextElementSibling).toContainElement(screen.getByTestId("toggle-markdown")); + }); +}); diff --git a/web/src/features/docs/DocPage.tsx b/web/src/features/docs/DocPage.tsx index 8d788bb..d4724ac 100644 --- a/web/src/features/docs/DocPage.tsx +++ b/web/src/features/docs/DocPage.tsx @@ -11,6 +11,7 @@ import { useUi } from "../../stores/ui"; import { CommentSidebar } from "../comments/CommentSidebar"; import { MarkdownView } from "../editor/MarkdownView"; import { Breadcrumb } from "./Breadcrumb"; +import { DocByline } from "./DocByline"; import { docsApi } from "./docs.api"; import { editModeKey } from "./editMode"; import { HistoryDrawer } from "./HistoryDrawer"; @@ -189,6 +190,7 @@ export default function DocPage() {
    +
    {/* Keep the bare StatusDot mounted (invisible) so existing tests diff --git a/web/src/features/docs/docs.api.test.ts b/web/src/features/docs/docs.api.test.ts new file mode 100644 index 0000000..be26ea2 --- /dev/null +++ b/web/src/features/docs/docs.api.test.ts @@ -0,0 +1,59 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { docsApi } from "./docs.api"; + +afterEach(() => vi.restoreAllMocks()); + +function jsonResponse(body: unknown, status = 200) { + return new Response(JSON.stringify(body), { + status, + headers: { "Content-Type": "application/json" }, + }); +} + +const BODY = { + created_by: { id: "u-alice", display_name: "Alice" }, + created_at: "2026-03-03T10:00:00Z", + contributors_since: "2026-10-03T12:00:00Z", + contributors: [ + { + user_id: "u-bob", + display_name: "Bob", + first_edited_at: "2026-10-03T12:05:00Z", + last_edited_at: "2026-10-03T13:00:00Z", + }, + ], +}; + +describe("docsApi.contributors", () => { + it("GETs the doc's contributors sub-resource and returns the parsed body", async () => { + const spy = vi.spyOn(globalThis, "fetch").mockResolvedValue(jsonResponse(BODY)); + + const r = await docsApi.contributors("d/1"); + + expect(spy.mock.calls[0]?.[0]).toBe("/api/docs/d%2F1/contributors"); + expect(spy.mock.calls[0]?.[1]?.method).toBe("GET"); + if (!("ok" in r)) throw new Error("expected ok"); + expect(r.ok).toEqual(BODY); + }); + + it("passes the error envelope through on 403", async () => { + vi.spyOn(globalThis, "fetch").mockResolvedValue( + jsonResponse({ error: { code: "auth.forbidden", message: "no", details: {} } }, 403), + ); + + const r = await docsApi.contributors("d1"); + + if (!("error" in r)) throw new Error("expected error"); + expect(r.error.status).toBe(403); + }); + + it("rejects a body that does not match the contract", async () => { + // Same stance as docsApi.get: a malformed body is a bug, not data to render. + vi.spyOn(globalThis, "fetch").mockResolvedValue( + jsonResponse({ ...BODY, contributors: [{ user_id: "u-bob" }] }), + ); + + await expect(docsApi.contributors("d1")).rejects.toThrow(); + }); +}); diff --git a/web/src/features/docs/docs.api.ts b/web/src/features/docs/docs.api.ts index ffe0536..fe87a97 100644 --- a/web/src/features/docs/docs.api.ts +++ b/web/src/features/docs/docs.api.ts @@ -1,7 +1,7 @@ import * as v from "valibot"; import { apiFetch } from "../../lib/api"; -import { Doc, DocWithRole, parse } from "../../lib/validators"; +import { Doc, DocContributors, DocWithRole, parse } from "../../lib/validators"; export type DocCreate = { title?: string; parent_id?: string; after_id?: string }; export type DocPatch = { title?: string; icon?: string }; @@ -18,6 +18,13 @@ export const docsApi = { if ("error" in r) return r; return { ok: parse(DocWithRole, r.ok) }; }, + /** Creator plus everyone who changed the page content. Readable by any + * role that can open the doc. */ + async contributors(id: string) { + const r = await apiFetch(`/api/docs/${encodeURIComponent(id)}/contributors`); + if ("error" in r) return r; + return { ok: parse(DocContributors, r.ok) }; + }, create(body: DocCreate) { return apiFetch("/api/docs", { method: "POST", body }); }, diff --git a/web/src/features/editor/KnotProvider.test.ts b/web/src/features/editor/KnotProvider.test.ts index 8a5ea79..b7d71fd 100644 --- a/web/src/features/editor/KnotProvider.test.ts +++ b/web/src/features/editor/KnotProvider.test.ts @@ -1,6 +1,8 @@ -import { describe, expect, it } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import * as Y from "yjs"; +import { FakeSocket } from "../../test/fakeSocket"; + import { KnotProvider } from "./KnotProvider"; describe("KnotProvider", () => { @@ -95,3 +97,91 @@ function concat(a: Uint8Array, b: Uint8Array): Uint8Array { out.set(b, a.length); return out; } + +// --------------------------------------------------------------------------- +// Edits made while the socket is down +// --------------------------------------------------------------------------- + +/** + * The client half of offline sync. The provider forwards edits only while its + * socket is open, so an edit made offline lives solely in the local doc until + * the server asks for it: on (re)connect the server sends SYNC_STEP_1 with its + * state vector and the provider answers with a SYNC_STEP_2 carrying whatever + * the server lacks (crates/knot-server/tests/offline_sync.rs drives the server + * half). These pin that answer, offline deletions included. + */ +describe("KnotProvider offline edits", () => { + beforeEach(() => { + FakeSocket.all = []; + vi.useFakeTimers(); + vi.stubGlobal("WebSocket", FakeSocket); + }); + afterEach(() => { + vi.useRealTimers(); + vi.unstubAllGlobals(); + }); + + /** Open the newest socket and play the server's handshake: full state as + * SYNC_STEP_2, then SYNC_STEP_1 asking for what it lacks. */ + function handshake(p: KnotProvider, server: Y.Doc): FakeSocket { + const ws = FakeSocket.all.at(-1)!; + ws.open(); + ws.serverState(server); + ws.serverAsks(server); + expect(p.status).toBe("connected"); + return ws; + } + + function dropAndReconnect(p: KnotProvider, server: Y.Doc, offline: () => void): FakeSocket { + FakeSocket.all.at(-1)!.drop(); + expect(p.status).toBe("offline"); + offline(); + const before = FakeSocket.all.length; + vi.advanceTimersByTime(31_000); + expect(FakeSocket.all.length).toBe(before + 1); + return handshake(p, server); + } + + const text = (d: Y.Doc) => d.getText("t").toJSON(); + + it("hands over what was typed while offline when the server asks", () => { + const server = new Y.Doc(); + server.getText("t").insert(0, "hello"); + const doc = new Y.Doc(); + const p = new KnotProvider({ url: "ws://x/collab", doc }); + handshake(p, server); + + const ws2 = dropAndReconnect(p, server, () => doc.getText("t").insert(5, " world")); + + for (const u of ws2.edits()) Y.applyUpdate(server, u); + expect(text(server)).toBe("hello world"); + p.destroy(); + }); + + it("hands over an offline deletion, which moves no state-vector clock", () => { + const server = new Y.Doc(); + server.getText("t").insert(0, "hello world"); + const doc = new Y.Doc(); + const p = new KnotProvider({ url: "ws://x/collab", doc }); + handshake(p, server); + + const ws2 = dropAndReconnect(p, server, () => doc.getText("t").delete(5, 6)); + + for (const u of ws2.edits()) Y.applyUpdate(server, u); + expect(text(server)).toBe("hello"); + p.destroy(); + }); + + it("hands over edits made before the first connection opened", () => { + const server = new Y.Doc(); + const doc = new Y.Doc(); + const p = new KnotProvider({ url: "ws://x/collab", doc }); + doc.getText("t").insert(0, "early"); + + const ws = handshake(p, server); + + for (const u of ws.edits()) Y.applyUpdate(server, u); + expect(text(server)).toBe("early"); + p.destroy(); + }); +}); diff --git a/web/src/features/editor/autolink.test.ts b/web/src/features/editor/autolink.test.ts new file mode 100644 index 0000000..0ee1c05 --- /dev/null +++ b/web/src/features/editor/autolink.test.ts @@ -0,0 +1,78 @@ +/** + * Autolinking must only ever react to the local user's own typing. + * + * Tiptap's autolink plugin is an `appendTransaction` hook that runs on every + * document-changing transaction — including the ones y-tiptap dispatches to + * mirror a peer's edit into this editor. So when Bob types "https://… " while + * Alice merely has the page open in edit mode, Alice's editor appends a link + * mark of its own, writes it into the Y.Doc, and ships it to the server as + * Alice's edit. Every peer with the page open does the same. That spurious + * write is what would credit Alice as a contributor to a page she never + * touched (and it is duplicate traffic regardless: Bob's own editor already + * linked the URL in his local transaction). + */ + +import { afterEach, describe, expect, it } from "vitest"; +import * as Y from "yjs"; + +import { mountBoundEditor, type BoundEditor } from "../../test/boundEditor"; + +type Mark = { type: string; attrs?: Record }; +type Inline = { type: string; text?: string; marks?: Mark[] }; + +function inlines(bound: BoundEditor): Inline[] { + const json = bound.editor.getJSON() as { content?: { content?: Inline[] }[] }; + return (json.content ?? []).flatMap((block) => block.content ?? []); +} + +function hasLink(bound: BoundEditor): boolean { + return inlines(bound).some((n) => n.marks?.some((m) => m.type === "link")); +} + +describe("autolink", () => { + let bound: BoundEditor | null = null; + + afterEach(() => { + bound?.destroy(); + bound = null; + }); + + it("links a URL the local user types", () => { + bound = mountBoundEditor(); + bound.editor.chain().focus().insertContent("https://example.com ").run(); + + expect(hasLink(bound)).toBe(true); + }); + + it("leaves a URL that arrived from a peer alone, and writes nothing back", () => { + bound = mountBoundEditor(); + const local = bound; + + // A peer's edit, built directly against a replica of the Y.Doc so the + // peer side runs no autolink of its own — exactly the bytes a remote + // client without a link mark would send. + const peer = new Y.Doc(); + Y.applyUpdate(peer, Y.encodeStateAsUpdate(local.ydoc)); + const p = new Y.XmlElement("paragraph"); + p.insert(0, [new Y.XmlText("https://example.com ")]); + const frag = peer.getXmlFragment("default"); + frag.insert(frag.length, [p]); + + const writes: unknown[] = []; + local.ydoc.on("update", (_u: Uint8Array, origin: unknown) => { + if (origin !== "remote") writes.push(origin); + }); + Y.applyUpdate( + local.ydoc, + Y.encodeStateAsUpdate(peer, Y.encodeStateVector(local.ydoc)), + "remote", + ); + + // The peer's text did reach the editor… + expect(inlines(local).map((n) => n.text ?? "").join("")).toBe("https://example.com "); + // …without this editor decorating it or echoing anything back. + expect(hasLink(local)).toBe(false); + expect(writes).toEqual([]); + peer.destroy(); + }); +}); diff --git a/web/src/features/editor/extensions.ts b/web/src/features/editor/extensions.ts index f85f20c..3495f81 100644 --- a/web/src/features/editor/extensions.ts +++ b/web/src/features/editor/extensions.ts @@ -3,7 +3,9 @@ import CollaborationCaret from "@tiptap/extension-collaboration-caret"; import Image from "@tiptap/extension-image"; import Link from "@tiptap/extension-link"; import Underline from "@tiptap/extension-underline"; +import { Plugin, type Transaction } from "@tiptap/pm/state"; import StarterKit from "@tiptap/starter-kit"; +import { ySyncPluginKey } from "@tiptap/y-tiptap"; import type { Awareness } from "y-protocols/awareness"; import type * as Y from "yjs"; import type { NavigateFunction } from "react-router-dom"; @@ -52,8 +54,50 @@ const KnotLink = Link.extend({ }, }; }, + + /** + * Autolink only the local user's own typing. + * + * Upstream's autolink plugin is an `appendTransaction` hook that fires on + * any doc-changing transaction, and y-tiptap mirrors every peer edit into + * this editor as one. So a peer typing "https://… " made every other open + * editor append a link mark and write it into the shared doc as its own + * edit: duplicate traffic, and — since the server records who changed a + * page — a contributor credit for someone who only had the page open. The + * peer's own editor already linked the URL in its local transaction, so + * skipping the mirrored copy loses nothing. + * + * The batch is skipped only when every transaction in it came from y-sync; + * a local keystroke batched alongside a remote change still autolinks. + * Undo/redo also arrives as a y-sync change (Yjs UndoManager owns undo), + * which is fine: re-linking text the user just undid would be wrong anyway. + * + * The plugin is found by its key name because upstream does not export it; + * autolink.test.ts fails if that lookup ever stops matching. + */ + addProseMirrorPlugins() { + return (this.parent?.() ?? []).map((plugin) => { + const keyName = (plugin as unknown as { key?: string }).key ?? ""; + const append = plugin.spec.appendTransaction; + if (!keyName.startsWith("autolink$") || !append) return plugin; + return new Plugin({ + ...plugin.spec, + appendTransaction: (transactions, oldState, newState) => { + if (transactions.every(isRemoteSync)) return null; + return append.call(plugin, transactions, oldState, newState); + }, + }); + }); + }, }); +/** True for a transaction y-tiptap dispatched to mirror a Y.Doc change that + * did not originate in this editor (a peer's edit, or a Yjs undo/redo). */ +function isRemoteSync(tr: Transaction): boolean { + const meta = tr.getMeta(ySyncPluginKey) as { isChangeOrigin?: boolean } | undefined; + return meta?.isChangeOrigin === true; +} + /** * v3's Image adds `width` and `height`. Nothing in knot sets them — the * upload path calls setImage({ src }) and the paste sanitiser's attribute diff --git a/web/src/lib/relTime.test.ts b/web/src/lib/relTime.test.ts new file mode 100644 index 0000000..e0e28f3 --- /dev/null +++ b/web/src/lib/relTime.test.ts @@ -0,0 +1,38 @@ +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +import { relTime } from "./relTime"; + +const NOW = new Date("2026-10-03T12:00:00Z").getTime(); +const ago = (ms: number) => new Date(NOW - ms).toISOString(); + +describe("relTime", () => { + beforeEach(() => { + vi.useFakeTimers(); + vi.setSystemTime(NOW); + }); + afterEach(() => vi.useRealTimers()); + + it("says just now for anything under a minute", () => { + expect(relTime(ago(0))).toBe("just now"); + expect(relTime(ago(59_999))).toBe("just now"); + }); + + it("counts whole minutes under an hour", () => { + expect(relTime(ago(60_000))).toBe("1m ago"); + expect(relTime(ago(59 * 60_000 + 59_999))).toBe("59m ago"); + }); + + it("counts whole hours under a day", () => { + expect(relTime(ago(3_600_000))).toBe("1h ago"); + expect(relTime(ago(23 * 3_600_000 + 3_599_999))).toBe("23h ago"); + }); + + it("counts whole days beyond that", () => { + expect(relTime(ago(86_400_000))).toBe("1d ago"); + expect(relTime(ago(45 * 86_400_000))).toBe("45d ago"); + }); + + it("treats a timestamp slightly in the future (clock skew) as just now", () => { + expect(relTime(new Date(NOW + 5_000).toISOString())).toBe("just now"); + }); +}); diff --git a/web/src/lib/relTime.ts b/web/src/lib/relTime.ts new file mode 100644 index 0000000..85c45fa --- /dev/null +++ b/web/src/lib/relTime.ts @@ -0,0 +1,15 @@ +/** + * Compact "how long ago" label: "just now", "5m ago", "3h ago", "2d ago". + * + * Whole units, rounded down, no months or years — the callers (comment + * timestamps, the contributor list) show the absolute time on hover, so this + * only has to be glanceable. A timestamp slightly in the future (server and + * client clocks disagree) reads as "just now" rather than a negative age. + */ +export function relTime(iso: string): string { + const diff = Date.now() - new Date(iso).getTime(); + if (diff < 60_000) return "just now"; + if (diff < 3_600_000) return `${Math.floor(diff / 60_000)}m ago`; + if (diff < 86_400_000) return `${Math.floor(diff / 3_600_000)}h ago`; + return `${Math.floor(diff / 86_400_000)}d ago`; +} diff --git a/web/src/lib/validators.ts b/web/src/lib/validators.ts index e3e8c93..dc24f19 100644 --- a/web/src/lib/validators.ts +++ b/web/src/lib/validators.ts @@ -73,6 +73,31 @@ const grantSchema = v.object({ export type Grant = v.InferOutput; export const Grant = grantSchema; +/** + * GET /api/docs/{id}/contributors. Names only — the server joins `users` for + * display names and never sends emails, so neither does this schema. + * `contributors` arrives most-recent-first; the client keeps that order. + * `contributors_since` is when tracking began for this doc: edits made before + * it were never attributed, so the byline says so instead of implying a + * complete list. + */ +const docContributorSchema = v.object({ + user_id: v.string(), + display_name: v.string(), + first_edited_at: v.string(), + last_edited_at: v.string(), +}); +export type DocContributor = v.InferOutput; + +const docContributorsSchema = v.object({ + created_by: v.object({ id: v.string(), display_name: v.string() }), + created_at: v.string(), + contributors_since: v.string(), + contributors: v.array(docContributorSchema), +}); +export type DocContributors = v.InferOutput; +export const DocContributors = docContributorsSchema; + export function parse(schema: v.GenericSchema, data: unknown): T { return v.parse(schema, data); } diff --git a/web/src/test/fakeSocket.ts b/web/src/test/fakeSocket.ts new file mode 100644 index 0000000..d31c3ce --- /dev/null +++ b/web/src/test/fakeSocket.ts @@ -0,0 +1,95 @@ +/** + * A scriptable stand-in for the browser WebSocket, for the y-protocol + * providers (KnotProvider, BoardProvider). Records what the client sends and + * lets the test play the server's side of the conversation. + * + * Install with `vi.stubGlobal("WebSocket", FakeSocket)`; every socket the + * provider opens lands in `FakeSocket.all`. + */ + +import * as Y from "yjs"; + +export class FakeSocket { + static CONNECTING = 0; + static OPEN = 1; + static CLOSING = 2; + static CLOSED = 3; + static all: FakeSocket[] = []; + readyState = FakeSocket.CONNECTING; + binaryType = "blob"; + sent: Uint8Array[] = []; + onopen: (() => void) | null = null; + onmessage: ((e: { data: ArrayBuffer }) => void) | null = null; + onclose: ((e: { code: number }) => void) | null = null; + onerror: (() => void) | null = null; + constructor(readonly url: string) { + FakeSocket.all.push(this); + } + send(b: Uint8Array) { + this.sent.push(b); + } + close() { + this.readyState = FakeSocket.CLOSED; + } + // --- server side --- + open() { + this.readyState = FakeSocket.OPEN; + this.onopen?.(); + } + /** The server's opening move: its whole state as SYNC_STEP_2. */ + serverState(server: Y.Doc) { + this.deliver(syncMsg(1, Y.encodeStateAsUpdate(server))); + } + /** The server asking for what it lacks: SYNC_STEP_1 with its state + * vector. The provider answers with a SYNC_STEP_2. */ + serverAsks(server: Y.Doc) { + this.deliver(syncMsg(0, Y.encodeStateVector(server))); + } + /** A frame from the server, as the arraybuffer the provider asks for. */ + deliver(frame: Uint8Array) { + const data = new ArrayBuffer(frame.byteLength); + new Uint8Array(data).set(frame); + this.onmessage?.({ data }); + } + drop() { + this.readyState = FakeSocket.CLOSED; + this.onclose?.({ code: 1006 }); + } + /** Payloads of the document-carrying frames the client sent (SYNC_STEP_2 + * and SYNC_UPDATE — the server treats both as edits). */ + edits(): Uint8Array[] { + return this.sent.flatMap((f) => { + if (f[0] !== 0 || (f[1] !== 1 && f[1] !== 2)) return []; + return [readPayload(f, 2)]; + }); + } +} + +/** MSG_SYNC, `subtype`, varuint length, payload — mirrors the providers' + * own encoder. */ +export function syncMsg(subtype: number, payload: Uint8Array): Uint8Array { + const head: number[] = [0, subtype]; + let n = payload.length; + while (n >= 0x80) { + head.push((n & 0x7f) | 0x80); + n >>>= 7; + } + head.push(n); + const out = new Uint8Array(head.length + payload.length); + out.set(head, 0); + out.set(payload, head.length); + return out; +} + +export function readPayload(frame: Uint8Array, offset: number): Uint8Array { + let len = 0; + let shift = 0; + let i = offset; + for (;;) { + const b = frame[i++]!; + len |= (b & 0x7f) << shift; + if ((b & 0x80) === 0) break; + shift += 7; + } + return frame.subarray(i, i + len); +} From 11355b71cc7bfc79e0e4d77ef72356c7bd0448ce Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20H=C3=BCning?= Date: Sat, 3 Oct 2026 22:18:20 +0200 Subject: [PATCH 2/3] fix(docs): give the byline its own key so navigation drops the old title DocTitle and DocByline are siblings and both used `key={id}`. Duplicate sibling keys are unsupported in React: switching docs in a single render (target doc's metadata already cached) kept the previous doc's title input mounted next to the new one, which broke search.spec's strict locator. Co-Authored-By: Claude Opus 5.5 (1M context) --- web/src/features/docs/DocPage.byline.test.tsx | 71 ++++++++++++++++++- web/src/features/docs/DocPage.tsx | 4 +- 2 files changed, 72 insertions(+), 3 deletions(-) diff --git a/web/src/features/docs/DocPage.byline.test.tsx b/web/src/features/docs/DocPage.byline.test.tsx index e8b05ca..845af65 100644 --- a/web/src/features/docs/DocPage.byline.test.tsx +++ b/web/src/features/docs/DocPage.byline.test.tsx @@ -1,6 +1,6 @@ import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; -import { cleanup, render, screen } from "@testing-library/react"; -import { MemoryRouter, Route, Routes } from "react-router-dom"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { Link, MemoryRouter, Route, Routes } from "react-router-dom"; import { afterEach, describe, expect, it, vi } from "vitest"; import { SessionProvider } from "../../auth/SessionContext"; @@ -92,4 +92,71 @@ describe("DocPage byline", () => { expect(title.nextElementSibling).toBe(byline); expect(byline.nextElementSibling).toContainElement(screen.getByTestId("toggle-markdown")); }); + + // DocTitle and DocByline are siblings that both reset per doc. Giving them + // the same `key` made React keep the previous doc's title input when + // navigating between docs, so two titles showed at once (search.spec). + it("shows exactly one title after navigating to another doc", async () => { + const docs: Record = { d1: "Runbook", d2: "Second" }; + vi.stubGlobal( + "fetch", + vi.fn((url: string) => { + const m = /^\/api\/docs\/(d2)(\/contributors)?$/.exec(url); + if (!m) return viewerServer(url); + if (m[2]) { + return json({ + created_by: { id: "u-alice", display_name: "Alice" }, + created_at: "2026-03-03T10:00:00Z", + contributors_since: "2026-03-03T10:00:00Z", + contributors: [], + }); + } + return json({ + id: "d2", + workspace_id: "w1", + parent_id: null, + title: docs.d2, + sort_key: "b", + icon: null, + created_by: "u-alice", + archived: false, + is_template: false, + effective_role: "viewer", + }); + }), + ); + + render( + + + + go to first + go to second + + } /> + + + + , + ); + + // Visit d2, then d1, then d2 again: on the way back d2's metadata is + // cached, so the page switches docs in a single render — the case where + // colliding sibling keys left the old title behind. + await waitFor(() => expect(screen.getByTestId("doc-title")).toHaveValue(docs.d2)); + fireEvent.click(screen.getByText("go to first")); + await waitFor(() => expect(screen.getByTestId("doc-title")).toHaveValue(docs.d1)); + await screen.findByTestId("doc-byline-creator"); + + fireEvent.click(screen.getByText("go to second")); + + await waitFor(() => + expect(screen.getAllByTestId("doc-title").map((el) => (el as HTMLInputElement).value)).toContain( + docs.d2, + ), + ); + await screen.findByTestId("doc-byline-creator"); + expect(screen.getAllByTestId("doc-title")).toHaveLength(1); + expect(screen.getAllByTestId("doc-byline")).toHaveLength(1); + }); }); diff --git a/web/src/features/docs/DocPage.tsx b/web/src/features/docs/DocPage.tsx index d4724ac..f27da53 100644 --- a/web/src/features/docs/DocPage.tsx +++ b/web/src/features/docs/DocPage.tsx @@ -190,7 +190,9 @@ export default function DocPage() {
    - + {/* Its own key: sibling keys must be unique, and sharing DocTitle's + `id` left the previous doc's title input mounted on navigation. */} +
    {/* Keep the bare StatusDot mounted (invisible) so existing tests From 5a55f931a930d893321598981ae4ccc45c6b27ca Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Christian=20H=C3=BCning?= Date: Sun, 4 Oct 2026 10:00:28 +0200 Subject: [PATCH 3/3] fix: address the review of #35 (revoke, pending no-ops, byline freshness) - Revoking access now reaches open sockets (pre-existing). The 4403 close relied on the room dropping the connection's sender, but serve() holds its own for the whole session, so it never fired: a revoked user kept writing, and with this branch would have been credited. ConnHandle gains a `revoked` token the room cancels; the writer turns it into 4403. The room ignores updates from connections it no longer holds and refuses a re-join with a cancelled handle. - The same defect leaked a task and socket per disconnect: the writer waited on a channel that could not close. Both shims now share ws_writer::run, which stops when the read half ends (and vice versa). - No-op detection compares pending state before/after instead of the doc-wide flag. Docs left with pending content by the offline bug made every page open persist a row and credit the opener. It also checks insert_set rather than building two full state vectors per update. - SyncStep1 is sent only to connections that can write; a viewer's answer was always dropped. - The contributor upsert skips authors credited within the minute before ON CONFLICT, which locked their row on every 250 ms flush. - The byline refetches every minute and on focus, and restore/import refresh it immediately. - One shared require_viewer for comments and contributors. Co-Authored-By: Claude Opus 5.5 (1M context) --- CHANGELOG.md | 6 + crates/knot-crdt/src/engine.rs | 92 +++++++- crates/knot-crdt/src/room.rs | 212 +++++++++++++++++- crates/knot-server/src/auth/mod.rs | 2 +- .../knot-server/src/auth/require_doc_role.rs | 18 ++ crates/knot-server/src/board_room_shim.rs | 57 ++--- crates/knot-server/src/lib.rs | 1 + crates/knot-server/src/room.rs | 80 +++---- crates/knot-server/src/routes/api/comments.rs | 16 +- .../src/routes/api/contributors.rs | 18 +- crates/knot-server/src/ws_writer.rs | 171 ++++++++++++++ .../tests/contributors_integration.rs | 129 ++++++++++- crates/knot-server/tests/convergence.rs | 14 +- crates/knot-storage/src/updates_store.rs | 24 +- crates/knot-storage/tests/updates.rs | 34 +++ web/src/features/docs/DocByline.test.tsx | 22 ++ web/src/features/docs/DocByline.tsx | 5 + web/src/features/docs/HistoryDrawer.test.tsx | 43 ++++ web/src/features/docs/HistoryDrawer.tsx | 2 + .../docs/ImportMarkdownButton.test.tsx | 33 ++- .../features/docs/ImportMarkdownButton.tsx | 8 +- 21 files changed, 832 insertions(+), 155 deletions(-) create mode 100644 crates/knot-server/src/ws_writer.rs create mode 100644 web/src/features/docs/HistoryDrawer.test.tsx diff --git a/CHANGELOG.md b/CHANGELOG.md index aa32a39..c078023 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,12 @@ so this log can be regenerated from history (e.g. with `git-cliff`). - An open editor no longer autolinks text that arrives from someone else's change, which wrote a link edit into the shared document under the observer's name. +- **Revoking access did not reach open editors.** The 4403 close an ACL change + should send never fired, so a revoked user kept a working socket and their + edits were still applied and saved. Open connections are now closed with + 4403 and anything they send after the revoke is dropped. The same defect + leaked a task and a socket on the server for every disconnect, until + restart. ## [0.7.0] - 2026-10-02 diff --git a/crates/knot-crdt/src/engine.rs b/crates/knot-crdt/src/engine.rs index 776b54e..3694a47 100644 --- a/crates/knot-crdt/src/engine.rs +++ b/crates/knot-crdt/src/engine.rs @@ -65,19 +65,22 @@ impl Engine for YrsEngine { fn apply_update_changed(&self, d: &DocHandle, update: &[u8]) -> Result { let u = Update::decode_v1(update).map_err(|e| EngineError::Apply(e.to_string()))?; let mut txn = d.0.transact_mut(); + let pending_before = pending_fingerprint(&txn); txn.apply_update(u) .map_err(|e| EngineError::Apply(e.to_string()))?; - // - state moved: new structs were integrated; - // - delete_set: holds only items THIS transaction newly deleted - // (re-deleting an already-deleted item adds nothing), so the full + // - insert_set: structs THIS transaction integrated (yrs records every + // integrated item and GC range there), so already-known structs in + // a re-sent state do not count; + // - delete_set: likewise only items newly deleted here, so the full // delete set every SyncStep2 carries does not count by itself; - // - missing updates: structs/deletes yrs had to park as pending. - // Doc-wide rather than per-update, so while anything is pending every - // update counts as a change — conservative on purpose: persisting a - // redundant row is harmless, dropping unintegrated content is not. - Ok(txn.before_state() != txn.after_state() + // - pending: structs/deletes yrs had to park because a dependency is + // missing. Compared before/after, not as a doc-wide flag: a doc can + // hold pending content indefinitely (and snapshots and every + // client's state re-send it), and re-delivering it changes nothing. + // Content that is newly parked does change it, and is never dropped. + Ok(!txn.insert_set().is_empty() || !txn.delete_set().is_empty() - || txn.has_missing_updates()) + || pending_fingerprint(&txn) != pending_before) } fn encode_state_as_update( @@ -101,6 +104,19 @@ impl Engine for YrsEngine { } } +/// The doc's pending (not yet integrable) structs and deletes, encoded, so a +/// caller can tell whether a transaction parked anything new. `None` in the +/// common case of nothing pending, which costs nothing to compute. +fn pending_fingerprint(txn: &T) -> Option<(Vec, Vec)> { + let store = txn.store(); + let structs = store.pending_update().map(|p| p.update.encode_v1()); + let deletes = store.pending_ds().map(|ds| ds.encode_v1()); + if structs.is_none() && deletes.is_none() { + return None; + } + Some((structs.unwrap_or_default(), deletes.unwrap_or_default())) +} + #[derive(Debug, Clone)] pub struct TextMark { pub kind: String, @@ -216,6 +232,64 @@ mod tests { ); } + /// A doc can hold pending content for good: the offline-edit bug left + /// docs whose later edits depend on clocks that were never uploaded, and + /// snapshots keep the pending part (yrs and Yjs both re-encode it into + /// every state update). Then every client's SyncStep2 reply carries that + /// pending content again. Re-delivering what is already pending changes + /// nothing and must not read as a change — or every page open on such a + /// doc would be persisted and credited as an edit. + #[test] + fn redelivering_already_pending_content_is_unchanged() { + let src = Doc::new(); + let t = src.get_or_insert_text("t"); + t.push(&mut src.transact_mut(), "a"); + let sv_after_a = src.transact().state_vector(); + t.push(&mut src.transact_mut(), "b"); + let only_b = src.transact().encode_state_as_update_v1(&sv_after_a); + + let e = YrsEngine; + let d = e.new_doc(); + assert!(e.apply_update_changed(&d, &only_b).unwrap()); + assert!( + d.0.transact().has_missing_updates(), + "fixture: b must be pending" + ); + + assert!( + !e.apply_update_changed(&d, &only_b).unwrap(), + "re-delivering the pending update must be a no-op" + ); + // What a client that loaded this doc answers to SyncStep1: the doc's + // state relative to the server's (empty) state vector, which includes + // the pending content. + let reply = full_update(&d.0); + assert!( + !e.apply_update_changed(&d, &reply).unwrap(), + "a SyncStep2 reply that only re-sends pending content must be a no-op" + ); + } + + /// The counterpart: filling the gap integrates the pending content. + #[test] + fn filling_the_missing_dependency_is_changed() { + let src = Doc::new(); + let t = src.get_or_insert_text("t"); + t.push(&mut src.transact_mut(), "a"); + let only_a = full_update(&src); + let sv_after_a = src.transact().state_vector(); + t.push(&mut src.transact_mut(), "b"); + let only_b = src.transact().encode_state_as_update_v1(&sv_after_a); + + let e = YrsEngine; + let d = e.new_doc(); + e.apply_update(&d, &only_b).unwrap(); + assert!(e.apply_update_changed(&d, &only_a).unwrap()); + assert!(!d.0.transact().has_missing_updates()); + let txt = d.0.get_or_insert_text("t"); + assert_eq!(txt.get_string(&d.0.transact()), "ab"); + } + #[test] fn malformed_update_is_an_error() { let e = YrsEngine; diff --git a/crates/knot-crdt/src/room.rs b/crates/knot-crdt/src/room.rs index f441f03..33b7c7b 100644 --- a/crates/knot-crdt/src/room.rs +++ b/crates/knot-crdt/src/room.rs @@ -36,6 +36,21 @@ pub struct InMsg { /// to send framed messages back to the client. pub struct ConnHandle { pub tx: mpsc::Sender>, + /// Cancelled by the room when the connection's access is revoked; the WS + /// shim then closes the socket with 4403. A dedicated signal because the + /// channel cannot carry it by closing (the shim holds its own sender for + /// the whole session) and no frame value is free to mean it (an empty + /// frame is already the presence-clearing message). + pub revoked: CancellationToken, +} + +impl ConnHandle { + pub fn new(tx: mpsc::Sender>) -> Self { + Self { + tx, + revoked: CancellationToken::new(), + } + } } /// What a joining connection needs for both halves of the y-sync handshake. @@ -475,8 +490,12 @@ impl Room { let _ = reply.send(Ok(final_seq)); } Some(Event::Revoke) => { - // Drop all conns. WS shim's writer task sees the - // closed channel and closes the socket with 4403. + // Tell every connection to close (the WS shim sends + // 4403) and drop them: from here on `on_inbound` + // ignores anything they still send. + for conn in self.conns.values() { + conn.revoked.cancel(); + } self.conns.clear(); } Some(Event::Shutdown) | None => break, @@ -582,12 +601,26 @@ impl Room { handle: ConnHandle, reply: oneshot::Sender>, ) { + // A re-join (client SyncStep1) racing a revoke still carries the + // connection's cancelled handle. Refuse it by dropping `reply`: no + // state for someone whose access just ended, and no re-registration + // that would let their updates through again. + if handle.revoked.is_cancelled() { + return; + } self.conns.insert(conn_id, handle); let _ = reply.send(JoinState::of(self.engine.as_ref(), &self.doc)); } #[tracing::instrument(skip(self, m), fields(doc_id = %self.doc_id, bytes = m.bytes.len()))] async fn on_inbound(&mut self, m: InMsg) { + // Only connections the room still holds may write. A revoked one (or + // one dropped as a slow consumer) can keep sending until its socket + // is torn down; its access ended when it left `conns`. + if !self.conns.contains_key(&m.from) { + tracing::debug!(conn = %m.from, "dropping update from a connection the room no longer holds"); + return; + } match self.engine.apply_update_changed(&self.doc, &m.bytes) { Ok(true) => {} // Nothing new — typically the SyncStep2 a (re)connecting writer @@ -688,6 +721,25 @@ mod tests { use super::*; use crate::{MemBus, YrsEngine}; + /// Register a connection with the room, as the WS shim does before it + /// forwards anything: the room only accepts updates from connections it + /// holds. Keep the receiver alive — a closed one gets the conn dropped + /// on the next fan-out. + async fn join(h: &RoomHandle) -> (ConnId, mpsc::Receiver>) { + let conn_id = Uuid::new_v4(); + let (tx, rx) = mpsc::channel(64); + let (reply_tx, reply_rx) = oneshot::channel(); + h.tx.send(Event::Join { + conn_id, + handle: ConnHandle::new(tx), + reply: reply_tx, + }) + .await + .unwrap(); + reply_rx.await.unwrap().unwrap(); + (conn_id, rx) + } + struct NoopUpdates; #[async_trait::async_trait] impl knot_storage::UpdatesStore for NoopUpdates { @@ -1035,7 +1087,7 @@ mod tests { let (join_tx, join_rx) = tokio::sync::oneshot::channel(); h.tx.send(Event::Join { conn_id: Uuid::new_v4(), - handle: ConnHandle { tx: conn_tx }, + handle: ConnHandle::new(conn_tx), reply: join_tx, }) .await @@ -1206,7 +1258,7 @@ mod tests { let (reply_tx, reply_rx) = oneshot::channel(); h.tx.send(Event::Join { conn_id, - handle: ConnHandle { tx }, + handle: ConnHandle::new(tx), reply: reply_tx, }) .await @@ -1297,7 +1349,7 @@ mod tests { let (reply_tx, reply_rx) = oneshot::channel(); h.tx.send(Event::Join { conn_id: Uuid::new_v4(), - handle: ConnHandle { tx }, + handle: ConnHandle::new(tx), reply: reply_tx, }) .await @@ -1414,7 +1466,7 @@ mod tests { let (reply_tx, reply_rx) = oneshot::channel(); h.tx.send(Event::Join { conn_id, - handle: ConnHandle { tx }, + handle: ConnHandle::new(tx), reply: reply_tx, }) .await @@ -1513,15 +1565,16 @@ mod tests { let (reply_tx, reply_rx) = oneshot::channel(); h.tx.send(Event::Join { conn_id: Uuid::new_v4(), - handle: ConnHandle { tx: peer_tx }, + handle: ConnHandle::new(peer_tx), reply: reply_tx, }) .await .unwrap(); reply_rx.await.unwrap().unwrap(); + let (sender, _sender_rx) = join(&h).await; h.tx.send(Event::Inbound(InMsg { - from: Uuid::new_v4(), + from: sender, by_user: None, bytes: dependent.clone(), })) @@ -1587,11 +1640,13 @@ mod tests { .encode_state_as_update_v1(&yrs::StateVector::default()) }; let (alice, bob) = (Uuid::new_v4(), Uuid::new_v4()); + let (alice_conn, _alice_rx) = join(&h).await; + let (bob_conn, _bob_rx) = join(&h).await; // Sent back-to-back: both reach the writer well inside one batch // window, which is exactly the case a per-batch author gets wrong. - for (by, bytes) in [(alice, edit("a")), (bob, edit("b"))] { + for (conn, by, bytes) in [(alice_conn, alice, edit("a")), (bob_conn, bob, edit("b"))] { h.tx.send(Event::Inbound(InMsg { - from: Uuid::new_v4(), + from: conn, by_user: Some(by), bytes, })) @@ -1615,4 +1670,141 @@ mod tests { h.shutdown.cancel(); } + + /// An ACL change must take effect on sockets that are already open. The + /// room tells each connection to close (4403) through its `revoked` + /// token — the channel cannot carry that by closing, because the WS shim + /// keeps its own sender for the whole session — and ignores anything a + /// revoked connection still sends before its socket is gone. + #[tokio::test(flavor = "multi_thread")] + async fn revoke_signals_each_conn_and_ignores_its_updates() { + use yrs::{ReadTxn, Text, Transact}; + + let bus = Arc::new(MemBus::new()); + let doc_id = Uuid::new_v4(); + let sub = bus.subscribe(doc_id).await.unwrap(); + let persisted = std::sync::Arc::new(std::sync::Mutex::new(Vec::new())); + let updates: Arc = Arc::new(CapturingUpdates { + persisted: persisted.clone(), + }); + let snapshots: Arc = Arc::new(NoopSnapshots); + let policy = crate::snapshot::SnapshotPolicy { + every_n: 1000, + idle: std::time::Duration::from_secs(60), + }; + let h = Room::spawn( + doc_id, + Arc::new(YrsEngine), + bus, + sub, + updates, + snapshots, + policy, + None, + ) + .await + .unwrap(); + + let conn_id = Uuid::new_v4(); + let (tx, _rx) = mpsc::channel(8); + let handle = ConnHandle::new(tx); + let revoked = handle.revoked.clone(); + let (reply_tx, reply_rx) = oneshot::channel(); + h.tx.send(Event::Join { + conn_id, + handle, + reply: reply_tx, + }) + .await + .unwrap(); + reply_rx.await.unwrap().unwrap(); + + h.tx.send(Event::Revoke).await.unwrap(); + tokio::time::timeout(std::time::Duration::from_secs(10), revoked.cancelled()) + .await + .expect("the revoked connection was never told to close"); + + let edit = |text: &str| { + let d = yrs::Doc::new(); + d.get_or_insert_text("t").push(&mut d.transact_mut(), text); + d.transact() + .encode_state_as_update_v1(&yrs::StateVector::default()) + }; + h.tx.send(Event::Inbound(InMsg { + from: conn_id, + by_user: None, + bytes: edit("after the revoke"), + })) + .await + .unwrap(); + // Events are handled in order: once a later HTTP edit is durably + // persisted, the revoked connection's update would have been too. + let sentinel = edit("sentinel"); + let (reply_tx, reply_rx) = oneshot::channel(); + h.tx.send(Event::ApplyUpdate { + update_bytes: sentinel.clone(), + by_user: None, + reply: reply_tx, + }) + .await + .unwrap(); + reply_rx.await.unwrap().unwrap(); + assert_eq!( + *persisted.lock().unwrap(), + vec![sentinel], + "an update from a revoked connection was persisted" + ); + + h.shutdown.cancel(); + } + + /// A client SyncStep1 that races the revoke would re-join the room with + /// the connection's (already cancelled) handle. The room must refuse it: + /// no state for someone whose access just ended, and no re-registration + /// that would let their updates through again. + #[tokio::test(flavor = "multi_thread")] + async fn join_with_a_revoked_handle_is_refused() { + let bus = Arc::new(MemBus::new()); + let doc_id = Uuid::new_v4(); + let sub = bus.subscribe(doc_id).await.unwrap(); + let updates: Arc = Arc::new(NoopUpdates); + let snapshots: Arc = Arc::new(NoopSnapshots); + let policy = crate::snapshot::SnapshotPolicy { + every_n: 1000, + idle: std::time::Duration::from_secs(60), + }; + let h = Room::spawn( + doc_id, + Arc::new(YrsEngine), + bus, + sub, + updates, + snapshots, + policy, + None, + ) + .await + .unwrap(); + + let (tx, _rx) = mpsc::channel(8); + let handle = ConnHandle::new(tx); + handle.revoked.cancel(); + let (reply_tx, reply_rx) = oneshot::channel(); + h.tx.send(Event::Join { + conn_id: Uuid::new_v4(), + handle, + reply: reply_tx, + }) + .await + .unwrap(); + let reply = tokio::time::timeout(std::time::Duration::from_secs(10), reply_rx) + .await + .expect("the room never answered the join"); + assert!( + reply.is_err(), + "a revoked connection was sent the document state" + ); + + h.shutdown.cancel(); + } } diff --git a/crates/knot-server/src/auth/mod.rs b/crates/knot-server/src/auth/mod.rs index 66c203b..27edd7e 100644 --- a/crates/knot-server/src/auth/mod.rs +++ b/crates/knot-server/src/auth/mod.rs @@ -11,4 +11,4 @@ pub mod csrf; pub use csrf::{CSRF_COOKIE, CSRF_HEADER, csrf_mw}; pub mod require_doc_role; -pub use require_doc_role::{EffectiveDocRole, require_doc_role_mw}; +pub use require_doc_role::{EffectiveDocRole, require_doc_role_mw, require_viewer}; diff --git a/crates/knot-server/src/auth/require_doc_role.rs b/crates/knot-server/src/auth/require_doc_role.rs index 3f87465..377bf4d 100644 --- a/crates/knot-server/src/auth/require_doc_role.rs +++ b/crates/knot-server/src/auth/require_doc_role.rs @@ -59,3 +59,21 @@ pub async fn require_doc_role_mw( req.extensions_mut().insert(EffectiveDocRole(role)); next.run(req).await } + +/// Handler-side gate for doc sub-resources any role may read (Viewer and +/// up). Expects `EffectiveDocRole` already set by `require_doc_role_mw`: +/// 401 `auth.session_required` without a session, 403 `acl.no_grant` +/// without a role on the doc. +pub fn require_viewer(req: &Request) -> Option { + if req.extensions().get::().is_none() { + return Some(json_err( + StatusCode::UNAUTHORIZED, + "auth.session_required", + "", + )); + } + if req.extensions().get::().is_none() { + return Some(json_err(StatusCode::FORBIDDEN, "acl.no_grant", "")); + } + None +} diff --git a/crates/knot-server/src/board_room_shim.rs b/crates/knot-server/src/board_room_shim.rs index 2c5e8f2..ac0e868 100644 --- a/crates/knot-server/src/board_room_shim.rs +++ b/crates/knot-server/src/board_room_shim.rs @@ -1,8 +1,8 @@ //! WebSocket → BoardRoom shim. Mirrors `room::serve` but against the //! `BoardRooms` registry, since boards have their own y-protocol session. -use axum::extract::ws::{CloseFrame, Message, WebSocket}; -use futures::{SinkExt, StreamExt}; +use axum::extract::ws::{Message, WebSocket}; +use futures::StreamExt; use knot_crdt::board_room::{ConnHandle, ConnId, Event, InMsg}; use std::sync::Arc; use tokio::sync::{mpsc, oneshot}; @@ -25,7 +25,7 @@ pub async fn serve( } }; let conn_id: ConnId = Uuid::new_v4(); - let (out_tx, mut out_rx) = mpsc::channel::>(256); + let (out_tx, out_rx) = mpsc::channel::>(256); let (reply_tx, reply_rx) = oneshot::channel(); if handle @@ -52,44 +52,24 @@ pub async fn serve( // message per frame. let _ = out_tx.send(encode_sync_step1(&joined.state_vector)).await; - let (mut sink, mut stream) = socket.split(); - let writer_shutdown = shutdown.clone(); - let writer = tokio::spawn(async move { - loop { - tokio::select! { - biased; - _ = writer_shutdown.cancelled() => { - let _ = sink - .send(Message::Close(Some(CloseFrame { - code: 1001, - reason: "server.shutdown".into(), - }))) - .await; - return; - } - maybe = out_rx.recv() => match maybe { - Some(bytes) => { - if sink.send(Message::Binary(bytes.into())).await.is_err() { - return; - } - } - None => { - let _ = sink - .send(Message::Close(Some(CloseFrame { - code: 4403, - reason: "acl.revoked".into(), - }))) - .await; - return; - } - }, - } - } - }); + let (sink, mut stream) = socket.split(); + // As in `room::serve`: whichever half ends the session cancels `done` so + // the other stops. Boards have no ACL revocation of their own, so the + // writer's revoke signal is never raised here. + let done = CancellationToken::new(); + let writer = tokio::spawn(crate::ws_writer::run( + sink, + out_rx, + shutdown.clone(), + CancellationToken::new(), + done.clone(), + )); loop { let msg = tokio::select! { + biased; _ = shutdown.cancelled() => break, + _ = done.cancelled() => break, m = stream.next() => match m { Some(Ok(m)) => m, _ => break, @@ -135,6 +115,9 @@ pub async fn serve( _ => {} } } + // Stop the writer before waiting for it: this function still holds a + // sender, so the channel alone would never end it. + done.cancel(); let _ = handle.tx.send(Event::Leave(conn_id)).await; let _ = writer.await; } diff --git a/crates/knot-server/src/lib.rs b/crates/knot-server/src/lib.rs index f7cd0f7..cc0e6f6 100644 --- a/crates/knot-server/src/lib.rs +++ b/crates/knot-server/src/lib.rs @@ -35,6 +35,7 @@ pub mod room; pub mod routes; pub mod security_headers; pub mod static_files; +mod ws_writer; use auth::SessionDeps; diff --git a/crates/knot-server/src/room.rs b/crates/knot-server/src/room.rs index 5bfa40c..72e535a 100644 --- a/crates/knot-server/src/room.rs +++ b/crates/knot-server/src/room.rs @@ -10,8 +10,8 @@ //! connection forwards carries it, so the room persists live edits under //! their author and credits that user as a contributor. -use axum::extract::ws::{CloseFrame, Message, WebSocket}; -use futures::{SinkExt, StreamExt}; +use axum::extract::ws::{Message, WebSocket}; +use futures::StreamExt; use knot_crdt::{ConnHandle, ConnId, Event, InMsg}; use std::sync::Arc; use tokio::sync::{mpsc, oneshot}; @@ -37,7 +37,14 @@ pub async fn serve( } }; let conn_id: ConnId = Uuid::new_v4(); - let (out_tx, mut out_rx) = mpsc::channel::>(256); + let (out_tx, out_rx) = mpsc::channel::>(256); + // Cancelled by the room when an ACL change revokes this connection; the + // writer then closes the socket with 4403. + let revoked = CancellationToken::new(); + let conn_handle = || ConnHandle { + tx: out_tx.clone(), + revoked: revoked.clone(), + }; // Join — receive hydrated state as bytes; wrap in sync_step_2 frame. let (reply_tx, reply_rx) = oneshot::channel(); @@ -45,7 +52,7 @@ pub async fn serve( .tx .send(Event::Join { conn_id, - handle: ConnHandle { tx: out_tx.clone() }, + handle: conn_handle(), reply: reply_tx, }) .await @@ -66,56 +73,36 @@ pub async fn serve( // pending by yrs and stays invisible to everyone else. The provider // already answers a server SyncStep1 with // `encodeStateAsUpdate(doc, serverSV)`, so already-deployed bundles are - // fixed by this alone. Viewers answer too; their reply is dropped below - // like any other inbound update from a read-only connection. - let _ = out_tx.send(encode_sync_step1(&joined.state_vector)).await; + // fixed by this alone. Not sent to viewers: they cannot have edits to + // upload, and their reply (at least the whole delete set) would only be + // dropped below. + if can_write { + let _ = out_tx.send(encode_sync_step1(&joined.state_vector)).await; + } // Server-side share of "open a doc, wait for content": includes the // hydrate of a cold room (see knot_room_hydrate_seconds) and the wait for // the room actor's reply. metrics::histogram!("knot_collab_initial_sync_seconds") .record(join_started.elapsed().as_secs_f64()); - let (mut sink, mut stream) = socket.split(); - let writer_shutdown = shutdown.clone(); - let writer = tokio::spawn(async move { - loop { - tokio::select! { - biased; - // Server is draining (SIGTERM): tell the client we're going - // away so it reconnects elsewhere, rather than being severed. - _ = writer_shutdown.cancelled() => { - let _ = sink - .send(Message::Close(Some(CloseFrame { - code: 1001, - reason: "server.shutdown".into(), - }))) - .await; - return; - } - maybe = out_rx.recv() => match maybe { - Some(bytes) => { - if sink.send(Message::Binary(bytes.into())).await.is_err() { - return; - } - } - // Channel closed — likely an ACL revoke. Send 4403. - None => { - let _ = sink - .send(Message::Close(Some(CloseFrame { - code: 4403, - reason: "acl.revoked".into(), - }))) - .await; - return; - } - }, - } - } - }); + let (sink, mut stream) = socket.split(); + // Cancelled by whichever half ends the session first, so the other one + // stops too: the writer on a revoke, a server shutdown or a dead socket; + // this read loop when the client goes away. + let done = CancellationToken::new(); + let writer = tokio::spawn(crate::ws_writer::run( + sink, + out_rx, + shutdown.clone(), + revoked.clone(), + done.clone(), + )); loop { let msg = tokio::select! { + biased; _ = shutdown.cancelled() => break, + _ = done.cancelled() => break, m = stream.next() => match m { Some(Ok(m)) => m, _ => break, @@ -131,7 +118,7 @@ pub async fn serve( .tx .send(Event::Join { conn_id, - handle: ConnHandle { tx: out_tx.clone() }, + handle: conn_handle(), reply: rtx, }) .await; @@ -169,6 +156,9 @@ pub async fn serve( _ => {} } } + // Stop the writer before waiting for it: this function still holds a + // sender, so the channel alone would never end it. + done.cancel(); let _ = handle.tx.send(Event::Leave(conn_id)).await; let _ = writer.await; } diff --git a/crates/knot-server/src/routes/api/comments.rs b/crates/knot-server/src/routes/api/comments.rs index e66bea3..28fde35 100644 --- a/crates/knot-server/src/routes/api/comments.rs +++ b/crates/knot-server/src/routes/api/comments.rs @@ -31,7 +31,7 @@ static MENTION_RE: std::sync::LazyLock = std::sync::LazyLock::new(|| Regex::new(r"(?:^|\s)@(\w+)").expect("valid mention regex")); use crate::AppState; -use crate::auth::{AuthContext, EffectiveDocRole}; +use crate::auth::{AuthContext, EffectiveDocRole, require_viewer}; use crate::http_error::json_err; // --------------------------------------------------------------------------- @@ -107,20 +107,6 @@ fn require_editor(req: &Request) -> Option { } } -fn require_viewer(req: &Request) -> Option { - if req.extensions().get::().is_none() { - return Some(json_err( - StatusCode::UNAUTHORIZED, - "auth.session_required", - "", - )); - } - if req.extensions().get::().is_none() { - return Some(json_err(StatusCode::FORBIDDEN, "acl.no_grant", "")); - } - None -} - fn internal() -> Response { json_err(StatusCode::INTERNAL_SERVER_ERROR, "internal", "") } diff --git a/crates/knot-server/src/routes/api/contributors.rs b/crates/knot-server/src/routes/api/contributors.rs index dd29e88..855d792 100644 --- a/crates/knot-server/src/routes/api/contributors.rs +++ b/crates/knot-server/src/routes/api/contributors.rs @@ -17,25 +17,9 @@ use axum::{ use uuid::Uuid; use crate::AppState; -use crate::auth::{AuthContext, EffectiveDocRole}; +use crate::auth::require_viewer; use crate::http_error::json_err; -/// Any effective role (Viewer and up). Expects `EffectiveDocRole` already -/// set by `require_doc_role_mw`; same responses as comments' `require_viewer`. -fn require_viewer(req: &Request) -> Option { - if req.extensions().get::().is_none() { - return Some(json_err( - StatusCode::UNAUTHORIZED, - "auth.session_required", - "", - )); - } - if req.extensions().get::().is_none() { - return Some(json_err(StatusCode::FORBIDDEN, "acl.no_grant", "")); - } - None -} - pub async fn byline( State(state): State, Path(doc_id): Path, diff --git a/crates/knot-server/src/ws_writer.rs b/crates/knot-server/src/ws_writer.rs new file mode 100644 index 0000000..6bd6281 --- /dev/null +++ b/crates/knot-server/src/ws_writer.rs @@ -0,0 +1,171 @@ +//! Outbound half of a collab WebSocket, shared by the doc and board shims. +//! +//! The room actor hands frames to a connection through an mpsc channel; this +//! task forwards them to the socket until the session ends. It cannot learn +//! that from the channel closing: the read half holds a sender of its own for +//! the whole session (to re-join on a client SyncStep1), so the channel stays +//! open until both halves are gone. The end is signalled explicitly instead. + +use axum::extract::ws::{CloseFrame, Message}; +use futures::{Sink, SinkExt}; +use tokio::sync::mpsc; +use tokio_util::sync::CancellationToken; + +/// Forward room frames to `sink` until the session ends, then cancel `done` +/// so the read half stops too. +/// +/// - `shutdown` (server draining): close with 1001 so the client reconnects +/// elsewhere. +/// - `revoked` (the room revoked this connection's access): close with 4403. +/// - `done` (the read half ended — the client went away): stop. Without it +/// this task would wait on the still-open channel forever, leaking itself +/// and the socket for every disconnect. +pub(crate) async fn run( + mut sink: S, + mut out_rx: mpsc::Receiver>, + shutdown: CancellationToken, + revoked: CancellationToken, + done: CancellationToken, +) where + S: Sink + Unpin, +{ + loop { + tokio::select! { + biased; + _ = shutdown.cancelled() => { + close(&mut sink, 1001, "server.shutdown").await; + break; + } + _ = revoked.cancelled() => { + close(&mut sink, 4403, "acl.revoked").await; + break; + } + _ = done.cancelled() => break, + maybe = out_rx.recv() => match maybe { + Some(bytes) => { + if sink.send(Message::Binary(bytes.into())).await.is_err() { + break; + } + } + // Every sender is gone; nothing more can arrive. + None => break, + }, + } + } + done.cancel(); +} + +async fn close(sink: &mut S, code: u16, reason: &'static str) +where + S: Sink + Unpin, +{ + let _ = sink + .send(Message::Close(Some(CloseFrame { + code, + reason: reason.into(), + }))) + .await; +} + +#[cfg(test)] +mod tests { + use super::*; + use futures::StreamExt; + use std::time::Duration; + + struct Harness { + out_tx: mpsc::Sender>, + sent: futures::channel::mpsc::UnboundedReceiver, + shutdown: CancellationToken, + revoked: CancellationToken, + done: CancellationToken, + task: tokio::task::JoinHandle<()>, + } + + fn start() -> Harness { + let (sink, sent) = futures::channel::mpsc::unbounded::(); + let (out_tx, out_rx) = mpsc::channel(8); + let (shutdown, revoked, done) = ( + CancellationToken::new(), + CancellationToken::new(), + CancellationToken::new(), + ); + let task = tokio::spawn(run( + sink, + out_rx, + shutdown.clone(), + revoked.clone(), + done.clone(), + )); + Harness { + out_tx, + sent, + shutdown, + revoked, + done, + task, + } + } + + async fn finished(task: tokio::task::JoinHandle<()>) { + tokio::time::timeout(Duration::from_secs(5), task) + .await + .expect("the writer never stopped") + .unwrap(); + } + + fn close_code(m: &Message) -> Option { + match m { + Message::Close(Some(CloseFrame { code, .. })) => Some(*code), + _ => None, + } + } + + #[tokio::test] + async fn forwards_room_frames_as_binary_messages() { + let mut h = start(); + h.out_tx.send(vec![1, 2, 3]).await.unwrap(); + let m = tokio::time::timeout(Duration::from_secs(5), h.sent.next()) + .await + .expect("frame never forwarded") + .unwrap(); + assert!(matches!(m, Message::Binary(b) if b.as_ref() == [1, 2, 3])); + h.done.cancel(); + finished(h.task).await; + } + + /// The leak: the read half still holds a sender when the client goes + /// away, so the channel never closes. The writer must stop anyway — and + /// say nothing, the socket is gone. + #[tokio::test] + async fn stops_when_the_read_half_ends_although_the_channel_is_open() { + let mut h = start(); + h.done.cancel(); + finished(h.task).await; + drop(h.out_tx); + assert!( + h.sent.next().await.is_none(), + "nothing may be sent after the client left" + ); + } + + #[tokio::test] + async fn revoked_access_closes_with_4403_and_ends_the_session() { + let mut h = start(); + h.revoked.cancel(); + finished(h.task).await; + let m = h.sent.next().await.expect("no close frame sent"); + assert_eq!(close_code(&m), Some(4403)); + assert!(h.done.is_cancelled(), "the read half must be told to stop"); + } + + #[tokio::test] + async fn server_shutdown_closes_with_1001() { + let mut h = start(); + h.shutdown.cancel(); + finished(h.task).await; + let m = h.sent.next().await.expect("no close frame sent"); + assert_eq!(close_code(&m), Some(1001)); + assert!(h.done.is_cancelled()); + } +} diff --git a/crates/knot-server/tests/contributors_integration.rs b/crates/knot-server/tests/contributors_integration.rs index 3512441..4180d9c 100644 --- a/crates/knot-server/tests/contributors_integration.rs +++ b/crates/knot-server/tests/contributors_integration.rs @@ -395,6 +395,9 @@ impl Peer { /// Dial as `who`, send SyncStep1, and complete the handshake: apply the /// server's state and answer its SyncStep1 exactly like the web provider /// (for a writer that answer reaches the room; it carries nothing new). + /// Viewers are not sent a SyncStep1. Done once the server's reply to our + /// own SyncStep1 (its second SyncStep2) is in: the server queues its + /// SyncStep1, if any, ahead of that reply. async fn join(fx: &Fixture, who: &Member) -> Self { let mut p = Self::new(); let mut req = format!("ws://{}/collab/doc/{}", fx.addr, fx.doc_id) @@ -413,8 +416,7 @@ impl Peer { .unwrap(); p.ws = Some(ws); assert!( - p.pump_until(|p| p.step2_received >= 1 && p.step1_answered >= 1) - .await, + p.pump_until(|p| p.step2_received >= 2).await, "handshake never completed" ); p @@ -962,3 +964,126 @@ async fn byline_still_names_a_former_member() { }]) ); } + +// --------------------------------------------------------------------------- +// The sync handshake and access changes +// --------------------------------------------------------------------------- + +/// Only connections that can write are asked for their state (SyncStep1). +/// A viewer's answer would always be dropped, and costs it an upload of at +/// least its whole delete set on every connect. +#[tokio::test(flavor = "multi_thread")] +async fn only_writers_are_asked_for_their_state() { + let fx = fixture().await; + let bob = Peer::join(&fx, &fx.bob).await; + let vic = Peer::join(&fx, &fx.vic).await; + assert_eq!( + bob.step1_answered, 1, + "an editor must be asked for its offline edits" + ); + assert_eq!(vic.step1_answered, 0, "a viewer was asked for its state"); + bob.leave().await; + vic.leave().await; +} + +/// Wait for the server to close `peer`'s socket and return the close code. +async fn close_code(peer: &mut Peer) -> u16 { + let ws = peer.ws.as_mut().expect("connected peer"); + let deadline = Instant::now() + DEADLINE; + loop { + let remaining = deadline.saturating_duration_since(Instant::now()); + match tokio::time::timeout(remaining, ws.next()).await { + Ok(Some(Ok(tungstenite::Message::Close(Some(frame))))) => return frame.code.into(), + Ok(Some(Ok(tungstenite::Message::Close(None)))) | Ok(None) => { + panic!("socket closed without a close code") + } + Ok(Some(Ok(_))) => {} + Ok(Some(Err(e))) => panic!("ws error: {e}"), + Err(_) => panic!("the server never closed the socket"), + } + } +} + +/// An ACL change that revokes access must reach sockets that are already +/// open: the server closes them with 4403, and nothing the revoked user sends +/// on the way out is applied, persisted or credited. +#[tokio::test(flavor = "multi_thread")] +async fn revoked_editor_is_closed_with_4403_and_its_edits_are_dropped() { + let fx = fixture().await; + let mut bob = Peer::join(&fx, &fx.bob).await; + + fx.state + .rooms_v2 + .as_ref() + .expect("fixture wires rooms") + .revoke_all_for_doc(fx.doc_id) + .await; + // Bob's editor has not seen the close yet and keeps typing. + bob.type_paragraph("after the revoke").await; + assert_eq!(close_code(&mut bob).await, 4403); + + // Sentinel: once Carol's later edit is durable, an accepted edit of + // Bob's would be too. + let mut carol = Peer::join(&fx, &fx.carol).await; + carol.type_paragraph("sentinel").await; + wait_for_durable_text(&fx.pool, fx.doc_id, "sentinel").await; + let byline = wait_for_byline(&fx, fx.doc_id, "sentinel", |l| l.contains(&fx.carol.id)).await; + assert_eq!( + listed(&byline), + vec![fx.carol.id], + "a revoked editor was credited" + ); + let rows = update_rows(&fx.pool, fx.doc_id).await; + assert!( + rows.iter().all(|r| r.by_user_id != Some(fx.bob.id)), + "an edit sent after the revoke was persisted" + ); + carol.leave().await; +} + +/// Docs damaged by the old offline-edit bug hold pending content for good +/// (later edits depend on clocks that were never uploaded), and every client +/// that loads such a doc re-sends that content in its SyncStep2 answer. +/// Re-delivering it must not count as an edit: opening the page is not +/// contributing. +#[tokio::test(flavor = "multi_thread")] +async fn opening_a_doc_with_pending_content_credits_nobody() { + let fx = fixture().await; + // "b" depends on "a", which the server never received. + let dependent = { + let src = Doc::new(); + let t = src.get_or_insert_text("t"); + t.push(&mut src.transact_mut(), "a"); + let sv = src.transact().state_vector(); + t.push(&mut src.transact_mut(), "b"); + src.transact().encode_state_as_update_v1(&sv) + }; + knot_storage::PgUpdatesStore::new(fx.pool.clone()) + .insert_batch(fx.doc_id, &[(None, dependent)]) + .await + .unwrap(); + + let mut bob = Peer::join(&fx, &fx.bob).await; + assert!( + bob.doc.transact().has_missing_updates(), + "fixture: the doc's pending content must reach the client" + ); + bob.settle().await; + + let mut carol = Peer::join(&fx, &fx.carol).await; + carol.type_paragraph("sentinel").await; + wait_for_durable_text(&fx.pool, fx.doc_id, "sentinel").await; + let byline = wait_for_byline(&fx, fx.doc_id, "sentinel", |l| l.contains(&fx.carol.id)).await; + assert_eq!( + listed(&byline), + vec![fx.carol.id], + "opening a doc with pending content credited the opener" + ); + let rows = update_rows(&fx.pool, fx.doc_id).await; + assert!( + rows.iter().all(|r| r.by_user_id != Some(fx.bob.id)), + "the opener's no-op answer was persisted" + ); + bob.leave().await; + carol.leave().await; +} diff --git a/crates/knot-server/tests/convergence.rs b/crates/knot-server/tests/convergence.rs index 3685d29..b22abc6 100644 --- a/crates/knot-server/tests/convergence.rs +++ b/crates/knot-server/tests/convergence.rs @@ -180,15 +180,17 @@ fn append_var_uint(out: &mut Vec, mut v: u64) { } /// Consume the handshake the server sends on connect: its sync-step-2 (full -/// state) followed by its own sync-step-1 (asking the client for anything the -/// server lacks). This test client has nothing to offer, so it leaves the -/// step-1 unanswered. +/// state), followed — for connections that can write — by its own +/// sync-step-1 (asking the client for anything the server lacks). This test +/// client has nothing to offer, so it leaves the step-1 unanswered. async fn drain_initial( ws: &mut tokio_tungstenite::WebSocketStream< tokio_tungstenite::MaybeTlsStream, >, + can_write: bool, ) { - for expected_subtype in [1u8, 0u8] { + let expected: &[u8] = if can_write { &[1, 0] } else { &[1] }; + for &expected_subtype in expected { let frame = tokio::time::timeout(Duration::from_secs(2), ws.next()) .await .expect("handshake frame") @@ -330,8 +332,8 @@ async fn viewer_cannot_write_owner_can() { let mut viewer_ws = open_authed_ws(addr, doc_id, &viewer_sid).await; // Drain the initial sync-step-2 frame from each connection. - drain_initial(&mut owner_ws).await; - drain_initial(&mut viewer_ws).await; + drain_initial(&mut owner_ws, true).await; + drain_initial(&mut viewer_ws, false).await; // --- T1: Viewer sends an update; owner should NOT receive it ----------- // (The server drops inbound updates from can_write=false connections.) diff --git a/crates/knot-storage/src/updates_store.rs b/crates/knot-storage/src/updates_store.rs index 7fb31a5..5a0fc4b 100644 --- a/crates/knot-storage/src/updates_store.rs +++ b/crates/knot-storage/src/updates_store.rs @@ -88,9 +88,15 @@ impl UpdatesStore for PgUpdatesStore { // one statement, and a batch often holds many rows per author. // - ORDER BY by_user_id: replicas flush the same doc concurrently; // taking the row locks in one global order rules out deadlocks. - // - the WHERE throttles the refresh to once a minute — continuous - // typing flushes every 250 ms, and a minute-old "last edited" is - // as good as a fresh one for a byline. + // - the refresh is throttled to once a minute — continuous typing + // flushes every 250 ms, and a minute-old "last edited" is as good + // as a fresh one for a byline. The NOT EXISTS skips authors + // credited within the minute before the upsert, because ON + // CONFLICT … DO UPDATE locks the existing row even when its WHERE + // then skips the update; without it every flush would queue on + // the rows of concurrent flushes (other replicas) of the same doc + // while holding an allocated seq. The WHERE stays for the race + // where a row turns fresh after the NOT EXISTS ran. let seqs = sqlx::query_scalar::<_, i64>( "WITH ins AS ( INSERT INTO doc_updates (doc_id, by_user_id, update_bytes) @@ -102,10 +108,16 @@ impl UpdatesStore for PgUpdatesStore { ), contrib AS ( INSERT INTO doc_contributors (doc_id, user_id) - SELECT DISTINCT $1::uuid, by_user_id + SELECT DISTINCT $1::uuid, ins.by_user_id FROM ins - WHERE by_user_id IS NOT NULL - ORDER BY by_user_id + WHERE ins.by_user_id IS NOT NULL + AND NOT EXISTS ( + SELECT 1 FROM doc_contributors c + WHERE c.doc_id = $1 + AND c.user_id = ins.by_user_id + AND c.last_edited_at >= now() - interval '1 minute' + ) + ORDER BY ins.by_user_id ON CONFLICT (doc_id, user_id) DO UPDATE SET last_edited_at = now() WHERE doc_contributors.last_edited_at < now() - interval '1 minute' diff --git a/crates/knot-storage/tests/updates.rs b/crates/knot-storage/tests/updates.rs index ec2144e..33563c9 100644 --- a/crates/knot-storage/tests/updates.rs +++ b/crates/knot-storage/tests/updates.rs @@ -278,3 +278,37 @@ async fn contributor_last_edited_at_refresh_is_throttled_to_once_a_minute() { "a stale last_edited_at must be refreshed to now(): {stale} -> {got_last}" ); } + +/// Continuous typing flushes every 250 ms. A flush for an author credited +/// within the last minute must not even lock their contributor row: `ON +/// CONFLICT … DO UPDATE … WHERE` locks the row although the WHERE skips the +/// update, so concurrent flushes of the same doc (other replicas) would queue +/// on it — and a waiting insert holds an allocated seq while later seqs +/// commit, widening the window in which replay can see seqs out of order. +#[tokio::test(flavor = "multi_thread")] +async fn flush_for_a_freshly_credited_author_does_not_lock_their_row() { + let a = two_authors().await; + let one_edit = [(Some(a.alice), vec![1u8])]; + a.s.insert_batch(a.doc, &one_edit).await.unwrap(); + + // Another session holds Alice's contributor row. + let mut holder = a.pool.begin().await.unwrap(); + sqlx::query("SELECT 1 FROM doc_contributors WHERE doc_id = $1 AND user_id = $2 FOR UPDATE") + .bind(a.doc) + .bind(a.alice) + .execute(&mut *holder) + .await + .unwrap(); + + let flushed = tokio::time::timeout( + std::time::Duration::from_secs(3), + a.s.insert_batch(a.doc, &one_edit), + ) + .await; + holder.rollback().await.unwrap(); + assert!( + flushed.is_ok(), + "the flush waited on the contributor row's lock" + ); + flushed.unwrap().unwrap(); +} diff --git a/web/src/features/docs/DocByline.test.tsx b/web/src/features/docs/DocByline.test.tsx index dd2be23..82b9ba5 100644 --- a/web/src/features/docs/DocByline.test.tsx +++ b/web/src/features/docs/DocByline.test.tsx @@ -235,4 +235,26 @@ describe("DocByline", () => { await waitFor(() => expect(screen.queryByTestId("doc-byline")).toBeNull()); }); + + // Edits made while the page is open — your own first edit on a new page, + // or anyone else's — must show up without a reload. With zero contributors + // there is no button whose opening could refetch, and window-focus refetch + // is off app-wide, so the byline polls. + it("refreshes while the page stays open", async () => { + vi.useFakeTimers({ shouldAdvanceTime: true }); + try { + contributors + .mockResolvedValueOnce({ ok: body() }) + .mockResolvedValue({ ok: body({ contributors: [person("u-alice", "Alice")] }) }); + renderByline(); + await screen.findByTestId("doc-byline-creator"); + expect(screen.queryByTestId("doc-contributors-button")).toBeNull(); + + await vi.advanceTimersByTimeAsync(60_000); + expect(await screen.findByTestId("doc-contributors-button")).toHaveTextContent("1 contributor"); + } finally { + vi.useRealTimers(); + } + }); }); + diff --git a/web/src/features/docs/DocByline.tsx b/web/src/features/docs/DocByline.tsx index 4beedbd..cfe6f19 100644 --- a/web/src/features/docs/DocByline.tsx +++ b/web/src/features/docs/DocByline.tsx @@ -35,6 +35,11 @@ export function DocByline({ docId }: { docId: string }) { const q = useQuery({ queryKey: ["contributors", docId], queryFn: () => docsApi.contributors(docId), + // Edits made while the page is open (your own first edit, or anyone + // else's) must show without a reload. With no contributors there is no + // button whose opening could refetch, and focus refetch is off app-wide. + refetchInterval: 60_000, + refetchOnWindowFocus: true, }); useEffect(() => { diff --git a/web/src/features/docs/HistoryDrawer.test.tsx b/web/src/features/docs/HistoryDrawer.test.tsx new file mode 100644 index 0000000..6f6723f --- /dev/null +++ b/web/src/features/docs/HistoryDrawer.test.tsx @@ -0,0 +1,43 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +import { cleanup, fireEvent, render, screen, waitFor } from "@testing-library/react"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +import { HistoryDrawer } from "./HistoryDrawer"; + +vi.mock("../../lib/history.api", () => ({ + historyApi: { + list: () => + Promise.resolve({ + ok: [{ snapshot_seq: 7, byte_size: 2048, created_at: "2026-10-03T10:00:00Z" }], + }), + preview: () => Promise.resolve({ ok: "# Earlier\n" }), + restore: () => Promise.resolve({ ok: undefined }), + }, +})); + +afterEach(() => { + cleanup(); + vi.restoreAllMocks(); +}); + +describe("HistoryDrawer", () => { + // A restore changes the content under the restorer's name: the byline + // should list them right away, not on its next poll. + it("refreshes the byline after a restore", async () => { + vi.spyOn(window, "confirm").mockReturnValue(true); + const qc = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + const invalidate = vi.spyOn(qc, "invalidateQueries"); + render( + + {}} /> + , + ); + + fireEvent.click(await screen.findByTestId("history-snap-7")); + fireEvent.click(await screen.findByTestId("history-restore")); + + await waitFor(() => + expect(invalidate).toHaveBeenCalledWith({ queryKey: ["contributors", "d1"] }), + ); + }); +}); diff --git a/web/src/features/docs/HistoryDrawer.tsx b/web/src/features/docs/HistoryDrawer.tsx index d61eb86..532990f 100644 --- a/web/src/features/docs/HistoryDrawer.tsx +++ b/web/src/features/docs/HistoryDrawer.tsx @@ -29,6 +29,8 @@ export function HistoryDrawer({ docId, onClose }: { docId: string; onClose: () = if ("error" in r) { notify("error", "Restore failed"); return; } notify("info", "Restored — your editor will refresh shortly."); await qc.invalidateQueries({ queryKey: ["doc", docId] }); + // The restore is credited to the restorer; show them in the byline now. + await qc.invalidateQueries({ queryKey: ["contributors", docId] }); onClose(); }, }); diff --git a/web/src/features/docs/ImportMarkdownButton.test.tsx b/web/src/features/docs/ImportMarkdownButton.test.tsx index 993bbf3..6f10c00 100644 --- a/web/src/features/docs/ImportMarkdownButton.test.tsx +++ b/web/src/features/docs/ImportMarkdownButton.test.tsx @@ -1,3 +1,4 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; import { render, screen, cleanup, fireEvent, waitFor } from "@testing-library/react"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; @@ -13,6 +14,14 @@ vi.mock("../../lib/history.api", () => ({ historyApi: { exportMarkdown: (...a: unknown[]) => exportMarkdown(...a) }, })); +function renderButton(qc = new QueryClient()) { + return render( + + + , + ); +} + function pick(name = "notes.md", body = "# Imported\n") { const file = new File([body], name, { type: "text/markdown" }); fireEvent.change(screen.getByTestId("doc-import-md-input"), { target: { files: [file] } }); @@ -31,7 +40,7 @@ afterEach(() => { describe("ImportMarkdownButton", () => { it("imports without prompting when the page is empty", async () => { const confirm = vi.spyOn(window, "confirm").mockReturnValue(true); - render(); + renderButton(); pick(); await waitFor(() => expect(importMarkdown).toHaveBeenCalledWith("d1", "# Imported\n", "replace"), @@ -42,7 +51,7 @@ describe("ImportMarkdownButton", () => { it("prompts before replacing a page that has content", async () => { exportMarkdown.mockResolvedValue({ ok: "# Existing\n" }); const confirm = vi.spyOn(window, "confirm").mockReturnValue(true); - render(); + renderButton(); pick(); await waitFor(() => expect(importMarkdown).toHaveBeenCalled()); expect(confirm).toHaveBeenCalledOnce(); @@ -53,7 +62,7 @@ describe("ImportMarkdownButton", () => { it("imports nothing when the prompt is declined", async () => { exportMarkdown.mockResolvedValue({ ok: "# Existing\n" }); vi.spyOn(window, "confirm").mockReturnValue(false); - render(); + renderButton(); pick(); await waitFor(() => expect(exportMarkdown).toHaveBeenCalled()); expect(importMarkdown).not.toHaveBeenCalled(); @@ -64,18 +73,32 @@ describe("ImportMarkdownButton", () => { error: { code: "internal", message: "boom", details: {}, status: 500 }, }); const confirm = vi.spyOn(window, "confirm").mockReturnValue(false); - render(); + renderButton(); pick(); await waitFor(() => expect(confirm).toHaveBeenCalled()); expect(importMarkdown).not.toHaveBeenCalled(); }); it("rejects an oversized file before any network call", async () => { - render(); + renderButton(); const big = new File(["x".repeat(1024 * 1024 + 1)], "big.md", { type: "text/markdown" }); fireEvent.change(screen.getByTestId("doc-import-md-input"), { target: { files: [big] } }); await waitFor(() => expect(screen.getByTestId("doc-import-md")).toBeInTheDocument()); expect(exportMarkdown).not.toHaveBeenCalled(); expect(importMarkdown).not.toHaveBeenCalled(); }); + + // The importer just changed the content: the byline should list them now, + // not on its next poll. + it("refreshes the byline after a successful import", async () => { + vi.spyOn(window, "confirm").mockReturnValue(true); + const qc = new QueryClient(); + const invalidate = vi.spyOn(qc, "invalidateQueries"); + renderButton(qc); + pick(); + await waitFor(() => + expect(invalidate).toHaveBeenCalledWith({ queryKey: ["contributors", "d1"] }), + ); + }); }); + diff --git a/web/src/features/docs/ImportMarkdownButton.tsx b/web/src/features/docs/ImportMarkdownButton.tsx index 41fc570..4151e47 100644 --- a/web/src/features/docs/ImportMarkdownButton.tsx +++ b/web/src/features/docs/ImportMarkdownButton.tsx @@ -1,3 +1,4 @@ +import { useQueryClient } from "@tanstack/react-query"; import { FileUp } from "lucide-react"; import { useRef, useState } from "react"; @@ -43,6 +44,7 @@ export function ImportMarkdownButton({ docTitle: string; }) { const inputRef = useRef(null); + const qc = useQueryClient(); const notify = useUi((s) => s.notify); const [busy, setBusy] = useState(false); @@ -72,8 +74,10 @@ export function ImportMarkdownButton({ notify("error", messageFor(r.error.code)); return; } - // No refetch needed: the room actor fans the replace out over the same - // WebSocket the editor is already on, exactly as a history restore does. + // The content needs no refetch: the room actor fans the replace out over + // the same WebSocket the editor is already on, exactly as a history + // restore does. The byline does: the import is credited to the importer. + void qc.invalidateQueries({ queryKey: ["contributors", docId] }); notify("info", `Imported "${file.name}"`); }