diff --git a/crates/plannotator-tui/src/app/feedback.rs b/crates/plannotator-tui/src/app/feedback.rs index f89212c..24a34e9 100644 --- a/crates/plannotator-tui/src/app/feedback.rs +++ b/crates/plannotator-tui/src/app/feedback.rs @@ -40,6 +40,8 @@ impl ReviewCounts { #[derive(Debug)] pub(super) struct FeedbackPart { pub(super) path: Option, + /// The reply these notes belong to, in a reply review; `None` for files. + pub(super) reply: Option, pub(super) store: Store, pub(super) ids: Vec, } @@ -54,7 +56,15 @@ pub(super) struct Feedback { } impl Feedback { - fn add(&mut self, path: Option, name: &str, doc: &Document, store: Store, scope: SendScope) { + pub(super) fn add( + &mut self, + path: Option, + reply: Option, + name: &str, + doc: &Document, + store: Store, + scope: SendScope, + ) { if let Some(path) = &path { self.counts.insert(path.clone(), ReviewCounts::for_store(&store)); } @@ -93,7 +103,7 @@ impl Feedback { original_text: (!a.anchor.original_text.is_empty()).then(|| a.anchor.original_text.clone()), } })); - self.parts.push(FeedbackPart { path, store, ids }); + self.parts.push(FeedbackPart { path, reply, store, ids }); } fn exported_text(self) -> String { @@ -234,12 +244,17 @@ impl App { _ => None, }; let mut feedback = Feedback::default(); - feedback.add(path, &self.open.source.name, &self.open.doc, self.open.store.clone(), scope); + feedback.add(path, None, &self.open.source.name, &self.open.doc, self.open.store.clone(), scope); feedback } pub(super) fn prepare_feedback(&self, scope: SendScope) -> Result { - let Some(tree) = &self.tree else { return Ok(self.file_feedback(scope)) }; + let Some(tree) = &self.tree else { + if !self.pick_cache.is_empty() { + return Ok(self.reply_feedback(scope)); + } + return Ok(self.file_feedback(scope)); + }; let mut feedback = Feedback::default(); for path in self.review_files() { if !path.is_file() { @@ -249,10 +264,10 @@ impl App { } let name = path.strip_prefix(tree.root()).unwrap_or(&path).display().to_string(); if self.is_open(&path) { - feedback.add(Some(path), &name, &self.open.doc, self.open.store.clone(), scope); + feedback.add(Some(path), None, &name, &self.open.doc, self.open.store.clone(), scope); } else { let (doc, store) = self.load_review_file(&path)?; - feedback.add(Some(path), &name, &doc, store, scope); + feedback.add(Some(path), None, &name, &doc, store, scope); } } // Folder feedback has always ended each file's block with one extra newline, so diff --git a/crates/plannotator-tui/src/app/mod.rs b/crates/plannotator-tui/src/app/mod.rs index 566617a..bcc0962 100644 --- a/crates/plannotator-tui/src/app/mod.rs +++ b/crates/plannotator-tui/src/app/mod.rs @@ -9,6 +9,7 @@ mod header; mod input; mod menu; mod pick; +mod replies; mod review; #[cfg(test)] mod review_test_support; diff --git a/crates/plannotator-tui/src/app/replies.rs b/crates/plannotator-tui/src/app/replies.rs new file mode 100644 index 0000000..b78340c --- /dev/null +++ b/crates/plannotator-tui/src/app/replies.rs @@ -0,0 +1,67 @@ +//! A reply review that has opened more than one reply is still one review. Each reply +//! keeps its notes in memory (the open one in `open`, the rest in `pick_cache`), and +//! sending, counting and the quit question cover all of them. + +use super::feedback::{Feedback, SendScope}; +use super::{App, Open}; +use crate::store::Store; + +impl App { + /// Every reply this review has opened, with its candidate index, in picker order. + /// A file, folder or single-document review has just the open document. + pub(super) fn replies(&self) -> Vec<(usize, &Open)> { + let mut replies: Vec<(usize, &Open)> = std::iter::once((self.pick_open, &self.open)) + .chain(self.pick_cache.iter().map(|(index, open)| (*index, open))) + .collect(); + replies.sort_by_key(|(index, _)| *index); + replies + } + + pub(super) fn reply_store_mut(&mut self, index: usize) -> Option<&mut Store> { + if index == self.pick_open { + Some(&mut self.open.store) + } else { + self.pick_cache.get_mut(&index).map(|open| &mut open.store) + } + } + + /// Notes from every reply that has any. One such reply is sent exactly as a review + /// that never used the picker; two or more are sent one block per reply, each + /// headed with which reply it is so the agent can tell them apart. A reply that is + /// not open and whose notes were all delivered already is left out, so sending from + /// another reply does not repeat it. + pub(super) fn reply_feedback(&self, scope: SendScope) -> Feedback { + let annotated: Vec<(usize, &Open)> = self + .replies() + .into_iter() + .filter(|(_, open)| !open.store.placed().is_empty()) + .filter(|(index, open)| *index == self.pick_open || !open.store.all_delivered()) + .collect(); + let mut feedback = Feedback::default(); + let named = annotated.len() > 1; + for (index, open) in annotated { + let name = if named { self.reply_name(index, open) } else { open.source.name.clone() }; + feedback.add(None, Some(index), &name, &open.doc, open.store.clone(), scope); + } + feedback + } + + /// `claude · message 2 of 3 ("first line")`, numbered as the picker lists them. + fn reply_name(&self, index: usize, open: &Open) -> String { + let host = &self.message_host; + let total = self.candidates.len(); + let first = open + .doc + .source + .lines() + .map(|line| line.trim().trim_start_matches('#').trim()) + .find(|line| !line.is_empty()) + .unwrap_or(""); + let first: String = if first.chars().count() > 60 { + first.chars().take(59).chain(std::iter::once('…')).collect() + } else { + first.to_owned() + }; + format!("{host} · message {} of {total} (\"{first}\")", index + 1) + } +} diff --git a/crates/plannotator-tui/src/app/send.rs b/crates/plannotator-tui/src/app/send.rs index b3aac31..9aff59e 100644 --- a/crates/plannotator-tui/src/app/send.rs +++ b/crates/plannotator-tui/src/app/send.rs @@ -53,8 +53,13 @@ impl App { let errors = self.remember_delivery(&mut feedback, &target); self.derive_send_state(); let verb = if self.delivery.is_agent() { "sent" } else { "copied" }; - let across = - if self.tree.is_some() { format!(" across {files} files") } else { String::new() }; + let across = if self.tree.is_some() { + format!(" across {files} files") + } else if files > 1 { + format!(" across {files} replies") + } else { + String::new() + }; let mut status = format!("{verb} {} annotation(s){across} → {target}", feedback.count); if !errors.is_empty() { let _ = write!( @@ -104,7 +109,11 @@ impl App { { self.folder_counts.insert(path.clone(), ReviewCounts::for_store(&part.store)); } - if part.path.as_deref().is_none_or(|p| self.is_open(p)) { + if let Some(index) = part.reply { + if let Some(store) = self.reply_store_mut(index) { + *store = part.store; + } + } else if part.path.as_deref().is_none_or(|p| self.is_open(p)) { self.open.store = part.store; } } @@ -170,7 +179,11 @@ impl App { } pub(super) fn send_count(&self) -> usize { - if self.is_file_review() { self.review_counts().pending } else { self.open.store.placed().len() } + if self.is_file_review() { + self.review_counts().pending + } else { + self.replies().iter().map(|(_, open)| open.store.placed().len()).sum() + } } pub(super) fn send_label(&self) -> String { @@ -227,7 +240,15 @@ impl App { let counts = self.review_counts(); counts.pending == 0 && counts.sent > 0 } else { - self.open.store.all_delivered() + // Every reply with notes has had them delivered; a reply without notes has + // nothing to send and does not hold the review back. + let annotated: Vec<&Store> = self + .replies() + .into_iter() + .map(|(_, open)| &open.store) + .filter(|store| store.len() > 0) + .collect(); + !annotated.is_empty() && annotated.iter().all(|store| store.all_delivered()) }; self.send_state = if delivered { SendState::Sent } else { SendState::Ready }; } diff --git a/crates/plannotator-tui/src/app/tests.rs b/crates/plannotator-tui/src/app/tests.rs index 9b59e5b..49449f9 100644 --- a/crates/plannotator-tui/src/app/tests.rs +++ b/crates/plannotator-tui/src/app/tests.rs @@ -629,3 +629,110 @@ fn paging_a_one_row_pane_keeps_the_selection() { app.handle_event(&key(KeyCode::Char('d'), KeyModifiers::CONTROL)).expect("ctrl+d"); assert_eq!(app.selected, selected, "a page of zero rows does not jump to the top"); } + +/// A reply review over `candidates()` that records what it sends, isolated like `app`. +fn recorded_message_app() -> (App, super::review_test_support::RecordingDelivery) { + let delivery = super::review_test_support::RecordingDelivery::default(); + let app = message_app(None, Box::new(delivery.clone())); + (app, delivery) +} + +fn keys(app: &mut App, codes: &[KeyCode]) { + for code in codes { + app.handle_event(&Event::Key(KeyEvent::from(*code))).expect("key"); + } +} + +/// Annotate the newest reply, then open the middle one from the picker. +fn note_newest_then_open_middle(app: &mut App) { + keys(app, &[KeyCode::Esc]); + app.add_block_annotation(0, Kind::Comment, "on the newest".to_owned()).expect("annotate"); + keys(app, &[KeyCode::Char('p'), KeyCode::Char('j'), KeyCode::Enter]); + assert_eq!(app.open.doc.source, "# Second\n\nmiddle message\n"); +} + +#[test] +fn notes_on_every_reply_are_sent_together_each_under_its_own_reply() { + let (mut app, delivery) = recorded_message_app(); + note_newest_then_open_middle(&mut app); + app.add_block_annotation(1, Kind::Comment, "on the middle".to_owned()).expect("annotate"); + assert_eq!(app.send_count(), 2, "both replies' notes are waiting"); + + keys(&mut app, &[KeyCode::Char('E')]); + let calls = delivery.calls.borrow(); + assert_eq!(calls.len(), 1); + assert_eq!( + calls[0], + "# Annotations on claude · message 1 of 3 (\"Third\")\n\n\ + ## Annotation 1 (line 1)\nComment on: \"# Third\"\n> on the newest\n\n\ + \n# Annotations on claude · message 2 of 3 (\"Second\")\n\n\ + ## Annotation 1 (line 3)\nComment on: \"middle message\"\n> on the middle\n\n" + ); + assert_eq!(app.send_state, SendState::Sent); + assert!(!app.has_unsent(), "every reply's notes were delivered"); + let status = app.status.clone().expect("status"); + assert!(status.starts_with("sent 2 annotation(s) across 2 replies"), "{status}"); + + let index = app.data_dir.join("feedback").join(&app.project).join("index.jsonl"); + let text = std::fs::read_to_string(&index).expect("archived"); + let record: serde_json::Value = serde_json::from_str(text.trim()).expect("one json record"); + assert_eq!(record["surface"], "annotate-last"); + assert_eq!(record["feedback"], calls[0].as_str()); + assert_eq!(record["annotations"].as_array().expect("annotations").len(), 2); + drop(calls); + + keys(&mut app, &[KeyCode::Char('p'), KeyCode::Char('k'), KeyCode::Enter]); + assert!(app.open.store.all_delivered(), "the newest reply's note was marked sent too"); + keys(&mut app, &[KeyCode::Char('S')]); + assert!(app.quit, "nothing left to send, so S closes"); + assert_eq!(delivery.calls.borrow().len(), 1, "and does not send again"); +} + +#[test] +fn quitting_with_notes_only_on_a_reply_that_is_not_open_asks_first() { + let (mut app, delivery) = recorded_message_app(); + note_newest_then_open_middle(&mut app); + assert!(app.has_unsent(), "the newest reply's note is unsent"); + keys(&mut app, &[KeyCode::Char('q')]); + assert_eq!(app.mode, Mode::ConfirmQuit); + assert!(!app.quit); + keys(&mut app, &[KeyCode::Char('y')]); + assert!(app.quit); + let calls = delivery.calls.borrow(); + assert_eq!(calls.len(), 1, "y sends the note on the reply that is not open"); + assert!(calls[0].contains("on the newest"), "{}", calls[0]); +} + +/// One reply with notes sends exactly what a review that never used the picker sends, +/// whichever reply happens to be open. +#[test] +fn a_single_annotated_reply_sends_the_same_body_as_before() { + let expected = "# Annotations on claude · last message\n\n\ + ## Annotation 1 (line 1)\nComment on: \"# Third\"\n> on the newest\n\n"; + let (mut app, delivery) = recorded_message_app(); + keys(&mut app, &[KeyCode::Esc]); + app.add_block_annotation(0, Kind::Comment, "on the newest".to_owned()).expect("annotate"); + keys(&mut app, &[KeyCode::Char('E')]); + assert_eq!(delivery.calls.borrow().as_slice(), [expected]); + + let (mut app, delivery) = recorded_message_app(); + note_newest_then_open_middle(&mut app); + keys(&mut app, &[KeyCode::Char('E')]); + assert_eq!(delivery.calls.borrow().as_slice(), [expected]); + assert_eq!(app.status.as_deref(), Some("sent 1 annotation(s) → test agent")); +} + +#[test] +fn a_reply_already_sent_is_not_sent_again_from_another_reply() { + let (mut app, delivery) = recorded_message_app(); + keys(&mut app, &[KeyCode::Esc]); + app.add_block_annotation(0, Kind::Comment, "on the newest".to_owned()).expect("annotate"); + keys(&mut app, &[KeyCode::Char('E')]); + keys(&mut app, &[KeyCode::Char('p'), KeyCode::Char('j'), KeyCode::Enter]); + app.add_block_annotation(1, Kind::Comment, "on the middle".to_owned()).expect("annotate"); + keys(&mut app, &[KeyCode::Char('E')]); + let calls = delivery.calls.borrow(); + assert_eq!(calls.len(), 2); + assert!(calls[1].contains("on the middle"), "{}", calls[1]); + assert!(!calls[1].contains("on the newest"), "the newest reply was sent already: {}", calls[1]); +}