diff --git a/CHANGELOG.md b/CHANGELOG.md index 9b91c05..c078023 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,35 @@ 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. +- **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 ### 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..3694a47 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,27 @@ 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(); + let pending_before = pending_fingerprint(&txn); + txn.apply_update(u) + .map_err(|e| EngineError::Apply(e.to_string()))?; + // - 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; + // - 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() + || pending_fingerprint(&txn) != pending_before) + } + fn encode_state_as_update( &self, d: &DocHandle, @@ -75,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, @@ -86,3 +128,172 @@ 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" + ); + } + + /// 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; + 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..33b7c7b 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, } @@ -32,6 +36,44 @@ 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. +/// +/// `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. @@ -40,7 +82,7 @@ pub enum Event { Join { conn_id: ConnId, handle: ConnHandle, - reply: oneshot::Sender, EngineError>>, + reply: oneshot::Sender>, }, Leave(ConnId), AwarenessIn { @@ -67,9 +109,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>, }, @@ -448,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, @@ -553,19 +599,42 @@ impl Room { &mut self, conn_id: ConnId, handle: ConnHandle, - reply: oneshot::Sender, EngineError>>, + 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 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"); + // 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 + // 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); let mut to_close: Vec = Vec::new(); @@ -586,7 +655,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. @@ -652,14 +721,32 @@ 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 { 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 +769,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 +779,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, @@ -964,12 +1087,12 @@ 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 .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 +1238,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(); @@ -1127,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 @@ -1135,6 +1266,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 +1319,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(); @@ -1217,12 +1349,12 @@ 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 .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 +1409,402 @@ 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::new(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::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: sender, + 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()); + 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 (conn, by, bytes) in [(alice_conn, alice, edit("a")), (bob_conn, bob, edit("b"))] { + h.tx.send(Event::Inbound(InMsg { + from: conn, + 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(); + } + + /// 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-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/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 5447929..ac0e868 100644 --- a/crates/knot-server/src/board_room_shim.rs +++ b/crates/knot-server/src/board_room_shim.rs @@ -1,15 +1,15 @@ //! 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}; 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, @@ -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 @@ -40,50 +40,36 @@ 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(); - 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, @@ -101,8 +87,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)) => { @@ -129,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 1aa33d6..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; @@ -59,6 +60,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 +97,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 +139,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 +161,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 +272,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..72e535a 100644 --- a/crates/knot-server/src/room.rs +++ b/crates/knot-server/src/room.rs @@ -5,20 +5,25 @@ //! `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}; +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}; 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, @@ -32,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(); @@ -40,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 @@ -48,58 +60,49 @@ 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. 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, @@ -115,12 +118,12 @@ pub async fn serve( .tx .send(Event::Join { conn_id, - handle: ConnHandle { tx: out_tx.clone() }, + handle: conn_handle(), 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 +134,7 @@ pub async fn serve( .tx .send(Event::Inbound(InMsg { from: conn_id, + by_user: Some(user_id), bytes: inner, })) .await; @@ -152,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 new file mode 100644 index 0000000..855d792 --- /dev/null +++ b/crates/knot-server/src/routes/api/contributors.rs @@ -0,0 +1,48 @@ +//! 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::require_viewer; +use crate::http_error::json_err; + +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/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 new file mode 100644 index 0000000..4180d9c --- /dev/null +++ b/crates/knot-server/tests/contributors_integration.rs @@ -0,0 +1,1089 @@ +//! 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). + /// 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) + .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 >= 2).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"], + }]) + ); +} + +// --------------------------------------------------------------------------- +// 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 05d59d5..b22abc6 100644 --- a/crates/knot-server/tests/convergence.rs +++ b/crates/knot-server/tests/convergence.rs @@ -179,15 +179,30 @@ 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 — 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, ) { - // The server sends exactly one sync-step-2 on join. Read it. - let _ = tokio::time::timeout(Duration::from_secs(2), ws.next()).await; + 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") + .expect("stream open") + .expect("ws ok") + .into_data(); + assert_eq!( + &frame[..2], + &[0u8, expected_subtype], + "unexpected handshake frame" + ); + } } // --------------------------------------------------------------------------- @@ -317,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-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..5a0fc4b 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,68 @@ 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 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) + 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, ins.by_user_id + FROM ins + 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' + ) + 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..33563c9 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,221 @@ 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}" + ); +} + +/// 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/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..82b9ba5 --- /dev/null +++ b/web/src/features/docs/DocByline.test.tsx @@ -0,0 +1,260 @@ +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()); + }); + + // 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 new file mode 100644 index 0000000..cfe6f19 --- /dev/null +++ b/web/src/features/docs/DocByline.tsx @@ -0,0 +1,161 @@ +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), + // 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(() => { + 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..845af65 --- /dev/null +++ b/web/src/features/docs/DocPage.byline.test.tsx @@ -0,0 +1,162 @@ +import { QueryClient, QueryClientProvider } from "@tanstack/react-query"; +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"; + +// 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")); + }); + + // 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 8d788bb..f27da53 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,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 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}"`); } 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); +}