From a5a246db770c83cf3ff7c665268a08f44a2bd0ab Mon Sep 17 00:00:00 2001 From: Valdemar Lorenzen Date: Tue, 20 Oct 2026 12:16:22 +0200 Subject: [PATCH] feat: copy diff review comments to configured clipboard --- TODO.md | 4 +- docs/configuration.md | 2 +- docs/development.md | 2 + docs/editing.md | 3 ++ src/editor.rs | 9 ++++ src/editor_review.rs | 16 +++++- src/main.rs | 1 + src/review.rs | 80 ++++++++++++++++++++++++++++ src/review_cli.rs | 100 ++++++++--------------------------- tests/unit/editor.rs | 110 +++++++++++++++++++++++++++++++++++++++ tests/unit/review_cli.rs | 2 +- 11 files changed, 248 insertions(+), 81 deletions(-) diff --git a/TODO.md b/TODO.md index a4e0fd1..3f5f540 100644 --- a/TODO.md +++ b/TODO.md @@ -1,8 +1,10 @@ - [x] improve: checkmark persistance should be preserved in a diff where a certain file hasn't changed. - this is to avoid that changing a single file will cause all reviewed files to drop the reviewed state, making it harder to spot what actually requires a re-review and what can be ignored. - [x] bug: diff view - git blame doesn't show in diff buffers for some reason. -- [ ] feature: diff view - command to copy all review comments in the diff to the configured clipboard. +- [x] feature: diff view - command to copy all review comments in the diff to the configured clipboard. - format should be the same as when printed via the cli +- [ ] improve: diff view - instead of a tab name following the format `Diff · + - · `, let's make it `Diff · + -`. + - let's also insert another element in the top-right status bar that displays the total changes across the diff, so we don't have only per-file changes for the currently selected file - [ ] feature: diff view - GitHub code review integration after the local review UX is usable. - publish local reviews explicitly: general PR comments, inline line/range comments, review summaries, approval, and requests for changes. - preserve explicit PR identity and reviewed commit IDs; validate anchors against GitHub's PR diff and recheck the PR head before submission. diff --git a/docs/configuration.md b/docs/configuration.md index 3877a3c..e1b5443 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -240,7 +240,7 @@ Normal and Visual Ctrl+u/Ctrl+d invoke `config.core.scroll_step_up` / `config.co | Normal | `follow_link` | | Normal | `diff`, `diff.mode.toggle`, `diff.view.toggle`, `diff.orientation.toggle`, `diff.review.toggle` | | Normal, Visual | `diff.comment` | -| Normal | `diff.review.edit`, `diff.review.delete`, `diff.review.resolve`, `diff.review.unresolve`, `diff.review.next`, `diff.review.previous`, `diff.review.overview`, `diff.review.summary`, `diff.review.verdict`, `diff.review.save`, `diff.review.open`, `diff.review.new` | +| Normal | `diff.review.edit`, `diff.review.delete`, `diff.review.resolve`, `diff.review.unresolve`, `diff.review.next`, `diff.review.previous`, `diff.review.overview`, `diff.review.copy`, `diff.review.summary`, `diff.review.verdict`, `diff.review.save`, `diff.review.open`, `diff.review.new` | | Normal, Insert, Visual, Command, Search | `diff.close` | | Normal | `resolve`, `resolve.next`, `resolve.previous`, `resolve.use_source`, `resolve.source`, `resolve.base`, `resolve.next_file`, `resolve.previous_file`, `resolve.files`, `resolve.draft`, `resolve.history`, `resolve.view.toggle`, `resolve.close` | | Normal, Insert, Visual, Command, Search | `word_wrap.toggle` | diff --git a/docs/development.md b/docs/development.md index 5b529ef..cebfe6c 100644 --- a/docs/development.md +++ b/docs/development.md @@ -70,6 +70,8 @@ Diff checks cover staged category/filter navigation, stale request cancellation, Local review checks cover native comment/summary composition and undo, Visual ranges on either side, verdicts, JSON save/reopen, external-write refusal, hidden drafts and teardown guards, immutable Working identities in Git/jj, renamed paths, live EOF, and stale anchors. Render checks cover selected/range markers, split/unified context, wrapped continuations, and narrow gutters. Live PTY smoke creates and saves a comment/summary/verdict, closes and reopens the review, edits the reopened comment, changes the reviewed source, and observes stale status without altering the saved anchor. Fixtures are local repositories; no forge writes. +Review clipboard checks cover System/Separate routing with isolated providers, exact Unicode excerpts, resolved/stale comments across files, retained unsaved drafts, unchanged storage revisions, and empty/invalid/failed-copy preservation. Live PTY smoke imports a review, copies it, pastes into an empty buffer, and verifies the saved bytes exactly match CLI `show --all`; default CLI output still excludes resolved comments. + SQLite review checks cover exact anchors across restarts and revision changes, repository isolation, independent connections and optimistic conflicts, locked/unwritable stores, incompatible schema preservation, relational constraints, full unsigned comment IDs, stable comment order, bounded/corrupt rows, rollback after partial comment insertion, private file permissions, and XDG/config path resolution. Schema 1 document storage is rejected without modification. Editor checks require explicit writes for comment/summary text, discard unsaved text on dismissal without retaining discarded undo history, prevent metadata autosaves from committing hidden drafts, persist verdicts/deletions, restore a sole review, choose among multiple reviews, and keep exports independent from autosaves. Test fixtures use isolated review directories. Live PTY smoke verifies that Escape leaves no new stored comment, `:w` persists exact text, and pane close discards a later edit; reopening and writing again preserves the saved body. Manual-resolution checks cover immediate persistence, reopen/export/import, independent stale status, conflict-retained drafts, and incompatible version preservation. CLI checks cover filename-safe dispatch, Unicode excerpts, unresolved filtering/counts, absent storage, read-only access and permissions, symlink rejection, and incompatible schema preservation. diff --git a/docs/editing.md b/docs/editing.md index 9bded5d..8b0973f 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -478,6 +478,7 @@ Local review buffers (comments, summaries, overview, and stored-review selector) | `:diff.review.resolve [id]` / `:diff.review.unresolve [id]` | Mark a comment resolved/unresolved; uses the same default selection as edit/delete | | `:diff.review.next` / `:diff.review.previous` | Cycle comments, opening their text and revealing a matching source anchor | | `:diff.review.overview` | Refresh/open a read-only summary and comment list; Enter edits the entry under the caret | +| `:diff.review.copy` | Copy all saved comments in the selected review to the configured clipboard, using the CLI text format | | `:diff.review.summary` | Compose the review summary | | `:diff.review.verdict looks_good` | Record **Looks good** | | `:diff.review.verdict changes_requested` | Record **Changes requested**; `none` clears the verdict | @@ -487,6 +488,8 @@ Local review buffers (comments, summaries, overview, and stored-review selector) Comment and summary text is committed only by **`:w`** or **`:diff.review.save`**, which saves the entire review, including hidden drafts. Dismissing an editor discards its unsaved text; switching between review editors can retain hidden drafts but does not save them. File checkmarks, verdicts, deletions, and manual resolution changes commit immediately to local SQLite without committing draft text. `:diff.review.save path` or `:w path` additionally exports JSON; later metadata autosaves still go to SQLite and leave that export unchanged. Imports create a new stored review identity rather than overwriting another review. +`:diff.review.copy` copies the same text as `vamp --review show ID --all`: repository, summary, verdict, counts, and every saved comment with its anchor and exact excerpt. Resolved and stale comments are included, across all files in the selected review. `clipboard_mode` chooses the system clipboard or Separate-mode internal register. Copying does not save or include unsaved comment/summary drafts. No comments or a clipboard write failure reports an error without replacing the register. + From the shell, use `vamp --review delete REVIEW_ID COMMENT_ID [--repo PATH] [--json]` to delete a stored comment immediately without opening the editor. Find IDs with `vamp --review list` and `vamp --review show REVIEW_ID --all`. The repository defaults to the current checkout. Both resolved and unresolved comments can be deleted; the review and any JSON exports remain intact. An editor holding the old review revision must reload before saving. Storage defaults to `$XDG_DATA_HOME/vamp/reviews/reviews.sqlite3`, or `~/.local/share/vamp/reviews/reviews.sqlite3` when XDG_DATA_HOME is unset or relative. Configure `review_directory` to override its directory; relative configuration paths resolve against the config file. Repository identity is the canonical checkout root. Each review has a persistent database ID independent of comparison revisions, so reopening changed revisions retains stale comments. Opening a comparison automatically restores the repository's sole review. Multiple reviews open a floating selector: move to an entry and press Enter, or use `:diff.review.open id`. Use `:diff.review.new` for another independent review. diff --git a/src/editor.rs b/src/editor.rs index 3609320..3cdc077 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -1041,6 +1041,12 @@ core_actions![ &[Mode::Normal], true ), + ( + "core.diff.review.copy", + Action::ReviewCopy, + &[Mode::Normal], + true + ), ( "core.diff.review.summary", Action::ReviewSummary, @@ -5436,6 +5442,7 @@ impl Editor { | Action::ReviewNext | Action::ReviewPrevious | Action::ReviewOverview + | Action::ReviewCopy | Action::ReviewSummary | Action::ReviewVerdict | Action::ReviewSave @@ -8867,6 +8874,7 @@ impl Editor { | Action::ReviewNext | Action::ReviewPrevious | Action::ReviewOverview + | Action::ReviewCopy | Action::ReviewSummary | Action::ReviewVerdict | Action::ReviewSave @@ -9825,6 +9833,7 @@ impl Editor { | Action::ReviewNext | Action::ReviewPrevious | Action::ReviewOverview + | Action::ReviewCopy | Action::ReviewSummary | Action::ReviewVerdict | Action::ReviewSave diff --git a/src/editor_review.rs b/src/editor_review.rs index 5681794..39296ec 100644 --- a/src/editor_review.rs +++ b/src/editor_review.rs @@ -460,7 +460,7 @@ impl Editor { { self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone()); } - if !matches!(action, Action::ReviewSave) { + if !matches!(action, Action::ReviewSave | Action::ReviewCopy) { self.autosave_local_review()?; } if action == Action::ReviewOpen { @@ -539,6 +539,20 @@ impl Editor { self.show_review_comment(comments[index].id)?; } Action::ReviewOverview => self.open_review_overview()?, + Action::ReviewCopy => { + let review = &self.workspace_diff().unwrap().local_review.document; + let count = review.comments.len(); + if count == 0 { + return Err("No local review comments".into()); + } + let text = review.format_text(true); + self.store_clipboard( + vec![text], + self.clipboard_mode == ClipboardMode::System, + false, + )?; + self.set_message(format!("Copied {count} review comments")); + } Action::ReviewSummary => { let text = self .workspace_diff() diff --git a/src/main.rs b/src/main.rs index 10ea520..02da39e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -100,6 +100,7 @@ Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the Selected comments highlight their precise text :diff.review.edit/delete [ID], :diff.review.next/previous, :diff.review.overview :diff.review.summary; :diff.review.verdict looks_good|changes_requested|none + :diff.review.copy copies all saved comments in CLI format using the configured clipboard :w saves comment/summary text, including hidden drafts; verdicts/deletions commit immediately :diff.review.save PATH exports JSON; :diff.review.open [ID|PATH] selects/imports; :diff.review.new Normal Esc or :pane.close discards unsaved text and dismisses the floating editor diff --git a/src/review.rs b/src/review.rs index 5f41bc8..2663cdb 100644 --- a/src/review.rs +++ b/src/review.rs @@ -4,6 +4,7 @@ use std::{ cell::RefCell, collections::BTreeSet, ffi::OsStr, + fmt::Write as _, fs::{self, OpenOptions}, io::{Read, Write}, path::{Path, PathBuf}, @@ -59,6 +60,22 @@ impl Anchor { self.start.row..=end } + pub fn excerpt(&self) -> &str { + let offset = |position: crate::plugin::TextPosition| { + let line = self.content.split('\n').nth(position.row).unwrap_or(""); + self.content + .split('\n') + .take(position.row) + .map(|line| line.len() + 1) + .sum::() + + line + .grapheme_indices(true) + .nth(position.column) + .map_or(line.len(), |(index, _)| index) + }; + &self.content[offset(self.start)..offset(self.end)] + } + pub fn matches_lines( &self, revision: &str, @@ -148,6 +165,69 @@ impl Review { } } + pub fn summary_text(&self) -> String { + let resolved = self + .comments + .iter() + .filter(|comment| comment.resolved) + .count(); + format!( + "Review {} (revision {}): {}\n Verdict: {}; {} unresolved, {} resolved ({} total)\n", + self.storage_id + .map_or_else(|| "null".into(), |id| id.to_string()), + self.storage_revision, + self.summary, + match self.verdict { + Some(Verdict::LooksGood) => "looks_good", + Some(Verdict::ChangesRequested) => "changes_requested", + None => "none", + }, + self.comments.len() - resolved, + resolved, + self.comments.len(), + ) + } + + /// CLI show format, including the repository, summary, and selected comments. + pub fn format_text(&self, all: bool) -> String { + let mut text = format!( + "Repository: {}\n{}", + self.root.display(), + self.summary_text() + ); + for comment in self + .comments + .iter() + .filter(|comment| all || !comment.resolved) + { + let anchor = &comment.anchor; + writeln!( + text, + "\nComment {} [{}] {} ({}) {}:{}–{}:{} @ {}\n{}\n{}", + comment.id, + if comment.resolved { + "resolved" + } else { + "unresolved" + }, + anchor.path.display(), + match anchor.side { + Side::Before => "before", + Side::After => "after", + }, + anchor.start.row, + anchor.start.column, + anchor.end.row, + anchor.end.column, + anchor.revision, + anchor.excerpt(), + comment.body, + ) + .unwrap(); + } + text + } + fn validate(&self) -> Result<(), String> { if self.version != 4 { return Err(format!("Unsupported review version {}", self.version)); diff --git a/src/review_cli.rs b/src/review_cli.rs index d73951d..dd12cef 100644 --- a/src/review_cli.rs +++ b/src/review_cli.rs @@ -3,10 +3,9 @@ use std::{ ffi::{OsStr, OsString}, path::{Path, PathBuf}, }; -use unicode_segmentation::UnicodeSegmentation; use vamp::{ repository, - review::{Anchor, Review, Store}, + review::{Review, Store}, ruby, }; @@ -79,24 +78,6 @@ impl Request { } } -fn excerpt(anchor: &Anchor) -> String { - // Positions use extended graphemes, not bytes or Unicode scalar values. - let offset = |position: vamp::plugin::TextPosition| { - let line = anchor.content.split('\n').nth(position.row).unwrap_or(""); - anchor - .content - .split('\n') - .take(position.row) - .map(|line| line.len() + 1) - .sum::() - + line - .grapheme_indices(true) - .nth(position.column) - .map_or(line.len(), |(index, _)| index) - }; - anchor.content[offset(anchor.start)..offset(anchor.end)].to_owned() -} - fn envelope(review: &Review, all: bool, comments: bool) -> Value { let resolved = review .comments @@ -116,7 +97,7 @@ fn envelope(review: &Review, all: bool, comments: bool) -> Value { json!({"id": comment.id, "body": comment.body, "resolved": comment.resolved, "anchor": { "revision": comment.anchor.revision, "path": comment.anchor.path, "side": comment.anchor.side, "start": comment.anchor.start, "end": comment.anchor.end }, - "excerpt": excerpt(&comment.anchor)}) + "excerpt": comment.anchor.excerpt()}) }).collect(); } value @@ -131,7 +112,7 @@ pub fn run(request: &Request, config: Option<&Path>) -> Result<(), String> { let cwd = std::env::current_dir().map_err(|error| error.to_string())?; let root = repository::discover_root(request.repo.as_deref().unwrap_or(&cwd))?; let store = Store::open_read_only(&directory)?; - let output = if let Some(id) = request.id { + let reviews = if let Some(id) = request.id { let mut review = store .as_ref() .ok_or("Review does not exist in this repository")? @@ -145,76 +126,41 @@ pub fn run(request: &Request, config: Option<&Path>) -> Result<(), String> { review.comments.remove(index); Store::open(&directory)?.save(&mut review)?; } - envelope(&review, request.all, true) + vec![review] } else { - let reviews = match store { - Some(store) => store - .list(&root)? - .into_iter() - .map(|review| envelope(&review, true, false)) - .collect::>(), + match store { + Some(store) => store.list(&root)?, None => Vec::new(), - }; - json!({"repository": root, "reviews": reviews}) + } }; if request.json { + let output = if request.id.is_some() { + envelope(&reviews[0], request.all, true) + } else { + let reviews = reviews + .iter() + .map(|review| envelope(review, true, false)) + .collect::>(); + json!({"repository": root, "reviews": reviews}) + }; println!( "{}", serde_json::to_string_pretty(&output).map_err(|error| error.to_string())? ); + } else if request.id.is_some() { + print!("{}", reviews[0].format_text(request.all)); } else { println!("Repository: {}", root.display()); - if let Some(reviews) = output["reviews"].as_array() { - if reviews.is_empty() { - println!("No stored reviews."); - } - for review in reviews { - print_review(review); - } - } else { - print_review(&output); - for comment in output["comments"].as_array().unwrap() { - let anchor = &comment["anchor"]; - println!( - "\nComment {} [{}] {} ({}) {}:{}–{}:{} @ {}", - comment["id"], - if comment["resolved"] == true { - "resolved" - } else { - "unresolved" - }, - anchor["path"].as_str().unwrap(), - anchor["side"].as_str().unwrap(), - anchor["start"]["row"], - anchor["start"]["column"], - anchor["end"]["row"], - anchor["end"]["column"], - anchor["revision"].as_str().unwrap() - ); - println!( - "{}\n{}", - comment["excerpt"].as_str().unwrap(), - comment["body"].as_str().unwrap() - ); - } + if reviews.is_empty() { + println!("No stored reviews."); + } + for review in reviews { + print!("{}", review.summary_text()); } } Ok(()) } -fn print_review(review: &Value) { - println!( - "Review {} (revision {}): {}\n Verdict: {}; {} unresolved, {} resolved ({} total)", - review["id"], - review["revision"], - review["summary"].as_str().unwrap(), - review["verdict"].as_str().unwrap_or("none"), - review["counts"]["unresolved"], - review["counts"]["resolved"], - review["counts"]["total"] - ); -} - #[cfg(test)] #[path = "../tests/unit/review_cli.rs"] mod tests; diff --git a/tests/unit/editor.rs b/tests/unit/editor.rs index 97e3c6f..18450d0 100644 --- a/tests/unit/editor.rs +++ b/tests/unit/editor.rs @@ -7462,6 +7462,116 @@ mod system_clipboard { } } + #[test] + fn review_copy_uses_cli_text_and_configured_clipboard_without_saving_drafts() { + use crate::review::{Anchor, Comment, Review, Side, Store, Verdict}; + + for mode in ["system", "separate"] { + let fixture = Fixture::new(); + let root = fixture.0.0.canonicalize().unwrap(); + let storage = root.join("reviews"); + let mut review = Review::new(root.clone()); + review.summary = "Review summary".into(); + review.verdict = Some(Verdict::ChangesRequested); + review.comments = vec![ + Comment { + id: 1, + body: "First\nfeedback".into(), + resolved: false, + anchor: Anchor { + revision: "base".into(), + path: "file.txt".into(), + side: Side::Before, + start: TextPosition { row: 0, column: 1 }, + end: TextPosition { row: 1, column: 1 }, + content: "xe\u{301}👩‍💻\n界z".into(), + }, + }, + Comment { + id: 2, + body: "Resolved feedback".into(), + resolved: true, + anchor: Anchor { + revision: "stale".into(), + path: "other.rs".into(), + side: Side::After, + start: TextPosition { row: 0, column: 0 }, + end: TextPosition { row: 0, column: 3 }, + content: "old".into(), + }, + }, + ]; + let mut store = Store::open(&storage).unwrap(); + store.save(&mut review).unwrap(); + let id = review.storage_id.unwrap(); + let expected = format!( + "Repository: {}\nReview {id} (revision 1): Review summary\n \ + Verdict: changes_requested; 1 unresolved, 1 resolved (2 total)\n\ + \nComment 1 [unresolved] file.txt (before) 0:1–1:1 @ base\n\ + e\u{301}👩‍💻\n界\nFirst\nfeedback\n\ + \nComment 2 [resolved] other.rs (after) 0:0–0:3 @ stale\n\ + old\nResolved feedback\n", + root.display(), + ); + let mut editor = fixture.editor(); + editor.set_clipboard_mode(mode).unwrap(); + editor.clipboard = vec!["internal".into()]; + fixture.external("external"); + editor.execute("diff.review.copy"); + assert_eq!(editor.clipboard, ["internal"]); + assert_eq!(fixture.bytes("state"), b"external"); + + editor.set_review_directory(storage); + historical_diff( + &mut editor, + &root, + vec!["xe\u{301}👩‍💻".into(), "界z".into()], + vec!["new".into()], + ); + editor.execute("diff.review.edit 1"); + type_text(&mut editor, "A UNSAVED"); + editor.handle(Input::Escape); + let draft = editor.context().text; + editor.execute("diff.review.copy"); + assert_eq!(editor.context().text, draft); + assert!(editor.local_reviews_dirty()); + let stored = store.load(&root, id).unwrap(); + assert_eq!(stored.storage_revision, review.storage_revision); + assert_eq!(stored.comments, review.comments); + if mode == "system" { + assert_eq!(fixture.bytes("state"), expected.as_bytes()); + assert_eq!(editor.clipboard, ["internal"]); + let owned = editor.owned_system_clipboard.clone(); + fixture.fail("fail-write", true); + editor.execute("diff.review.copy"); + assert_eq!(fixture.bytes("state"), expected.as_bytes()); + assert_eq!(editor.owned_system_clipboard, owned); + } else { + assert_eq!(editor.clipboard, [expected.clone()]); + assert!(!editor.clipboard_linewise); + assert_eq!(fixture.bytes("state"), b"external"); + assert!(fixture.bytes("writes").is_empty()); + } + editor.execute("diff.review.copy unexpected"); + editor.with_pane(editor.diff_owner.unwrap(), |editor| { + editor + .diff + .as_mut() + .unwrap() + .local_review + .document + .comments + .clear(); + }); + editor.execute("diff.review.copy"); + if mode == "system" { + assert_eq!(fixture.bytes("state"), expected.as_bytes()); + } else { + assert_eq!(editor.clipboard, [expected]); + } + } + } + #[test] fn line_actions_clipboard_failure_preserves_carets_history_and_repeat() { let fixture = Fixture::new(); diff --git a/tests/unit/review_cli.rs b/tests/unit/review_cli.rs index e458b2a..5f0035e 100644 --- a/tests/unit/review_cli.rs +++ b/tests/unit/review_cli.rs @@ -1,7 +1,7 @@ use super::*; use vamp::{ plugin::TextPosition, - review::{Comment, Side}, + review::{Anchor, Comment, Side}, }; #[test]