From 3c270ed7b599aea424f50a717fe3cc621d3d1106 Mon Sep 17 00:00:00 2001 From: hl-valdemar Date: Sun, 18 Oct 2026 10:14:26 +0200 Subject: [PATCH] feat: delete local review comments from CLI --- README.md | 5 +- docs/architecture.md | 1 + docs/development.md | 1 + docs/editing.md | 2 + src/main.rs | 4 +- src/review_cli.rs | 40 ++++++++++--- tests/review_cli.rs | 130 +++++++++++++++++++++++++++++++++++++++++ tests/unit/cli_args.rs | 26 +++++++++ 8 files changed, 200 insertions(+), 9 deletions(-) create mode 100644 tests/review_cli.rs diff --git a/README.md b/README.md index 1bbbcda..aaefdf0 100644 --- a/README.md +++ b/README.md @@ -13,19 +13,22 @@ cargo run -- --config examples/config.rb notes.txt Without a file, vamp opens an unnamed buffer. A nonexistent file opens an empty buffer. `cargo run -- --help` lists the command-line options. -### Read local reviews without opening the editor +### Manage local reviews without opening the editor ```sh vamp --review list --repo . vamp --review list --repo . --json vamp --review show 1 --repo . --json vamp --review show 1 --repo . --all +vamp --review delete 1 3 --repo . # Delete comment 3 from review 1 ``` The review CLI discovers the Git/Jujutsu checkout from `--repo` (default: current directory). It uses the same default configuration, `--config PATH`, `--no-config`, and `review_directory` as the editor, but starts no editor workers or terminal UI. Missing storage lists no reviews without creating a database; incompatible storage is rejected and preserved. `show` includes unresolved comments by default; `--all` includes resolved comments too. Human-readable output is the default. JSON includes the canonical repository root, storage review ID/revision, summary/verdict, total/resolved/unresolved counts, and comments with exact zero-based, end-exclusive grapheme anchors and selected source excerpts—not whole source files. `list --json` returns the repository and review metadata/counts without comments. Explicit editor JSON exports still contain complete anchor source text. +`list` and `show` are read-only. `delete REVIEW_ID COMMENT_ID` immediately commits deletion of one resolved or unresolved comment, then prints the updated review with its remaining unresolved comments (`--json` is supported). Both IDs are required; missing reviews/comments and repository mismatches fail without changing stored reviews or creating missing storage. Deleting the last comment keeps the review, summary, and verdict. Concurrent saves use the same revision-conflict protection as the editor; an already-open editor must reload before saving after a CLI deletion. Existing JSON exports are not modified. + `vamp review` still opens a file named `review`; `vamp -- --review` opens a file named `--review`. ## Getting started diff --git a/docs/architecture.md b/docs/architecture.md index 9cf9283..9aa2e5e 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -19,6 +19,7 @@ One editor crate; the workspace also includes its patched Crossterm dependency. - `repository`: discovers jj/Git repositories and local/advertised remote revisions, builds disk snapshots and changed-file hunks, and owns isolated temporary comparison repositories with bounded, cancellable subprocesses. It never saves editor buffers or modifies active refs/checkouts. - `review`: owns the validated local review model, exact immutable anchors, SQLite storage through bundled rusqlite, and atomic JSON import/export with external-change protection. `Store` uses schema-versioned transactions and compare-and-swap review revisions; repository roots and persistent review IDs are independent of comparison revisions. One bounded JSON document is stored per row, retaining exact grapheme endpoints and stale snapshots. Incompatible schemas and concurrent edits fail without replacing data or advancing in-memory saved state. Store listing returns validated review documents in descending ID order. CLI metadata, sole-review restoration, and selector labels reuse those documents without reloading them; selector choices live only in the displayed buffer. + The review CLI keeps `list`/`show` read-only. Comment deletion validates the explicit review/comment IDs through an existing read-only store before opening it for a compare-and-swap save; absent storage and invalid targets do not create reviews or a database. `editor_review` commits native comment/summary buffers on dismissal or switching, and verdicts/deletions immediately. It restores a sole stored review or opens the floating selector for multiple reviews. Exports retain their own file baseline and do not become automatic persistence targets; imports get fresh database identities. Review editors remain tab-owned native panes outside the workspace `PaneTree`. Storage opens lazily when a comparison is ready; the session supplies the configured or XDG default directory. Direct `Editor` callers inject a directory with `set_review_directory` to enable persistence. Successful SQLite autosaves and JSON-only saves share one acknowledgement operation: clear review dirtiness and advance the owning tab's native draft saved baselines only after persistence succeeds. Failed writes retain unsaved state; JSON exports do not acknowledge a pending SQLite save. Review replacement and comment deletion are private editor-owned operations. Replacement loads and validates its Stored/JSON/New source before retiring old bindings, installing the document, and resetting selection/navigation. Deletion retires and acknowledges only the matching comment drafts, preventing stale text from overwriting reused IDs. Both persist before opening the overview; a post-mutation save failure retains the applied, unsaved identity change. Native text/undo and general transition guards remain unchanged. diff --git a/docs/development.md b/docs/development.md index 0abc4c7..6d4eada 100644 --- a/docs/development.md +++ b/docs/development.md @@ -74,6 +74,7 @@ SQLite review checks cover exact anchors across restarts and revision changes, r 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. Live PTY/CLI smoke composes and explicitly saves a comment, resolves it in the editor, observes an empty default CLI comment list with explicit counts, retrieves the resolved comment and exact excerpt with `--all`, and unresolves it. Each transition advances the same stored review revision; listing absent storage creates no data directory. +CLI deletion checks exercise explicit review/comment IDs, resolved-comment deletion, repository isolation, preserved summaries/verdicts/other comments, last-comment deletion, missing targets/storage, and stale-save conflicts through the executable. CLI smoke verifies persisted deletion and rejects a repeated deletion without advancing the review revision. Conflicted-revision regressions preserve unchanged symlinks, defer oversized unselected blobs until file selection, compare selected conflict text with jj's projection, and exclude unchanged conflicts when comparing later revisions. Live terminal smoke covers local bookmark selection and opening an actual conflicted file without changing the checkout. diff --git a/docs/editing.md b/docs/editing.md index 5ab86e2..c719d3a 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -487,6 +487,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. 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. +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. Write errors, locked databases, incompatible schemas, and conflicting edits from another editor instance retain dirty drafts and report the failure. Failed text saves leave the committed review unchanged; unrelated metadata autosaves cannot commit those drafts. Normal destructive closes remain guarded until drafts are explicitly saved or discarded. `:w!` does not bypass database revision conflicts; export your draft before reopening the current stored review. JSON exports retain external-file-change protection. Only the current database schema is accepted; existing incompatible databases are preserved rather than reset or migrated. diff --git a/src/main.rs b/src/main.rs index 41978e3..10ea520 100644 --- a/src/main.rs +++ b/src/main.rs @@ -12,8 +12,10 @@ const HELP: &str = "vamp — modal editor with isolated Ruby plugins Usage: vamp [--config PATH | --no-config] [FILE] vamp [--config PATH | --no-config] --review list [--repo PATH] [--json] vamp [--config PATH | --no-config] --review show ID [--repo PATH] [--json] [--all] + vamp [--config PATH | --no-config] --review delete REVIEW_ID COMMENT_ID [--repo PATH] [--json] -Review CLI: read-only local storage; show defaults to unresolved comments; --all includes resolved. +Review CLI: list/show are read-only; delete commits one comment deletion immediately. +Show defaults to unresolved comments; --all includes resolved. Use vamp -- --review to edit a file named --review. Default keys (configurable with config.bind): diff --git a/src/review_cli.rs b/src/review_cli.rs index 44e6a27..d73951d 100644 --- a/src/review_cli.rs +++ b/src/review_cli.rs @@ -12,6 +12,7 @@ use vamp::{ pub struct Request { id: Option, + delete_comment: Option, repo: Option, json: bool, all: bool, @@ -19,22 +20,37 @@ pub struct Request { impl Request { pub fn parse(args: &mut impl Iterator) -> Result { - let command = args.next().ok_or("--review requires list or show ID")?; + let usage = "--review requires list, show ID, or delete REVIEW_ID COMMENT_ID"; + let command = args.next().ok_or(usage)?; let id = if command == "list" { None - } else if command == "show" { - let id = args.next().ok_or("show requires a positive review ID")?; + } else if command == "show" || command == "delete" { + let error = "show/delete requires a positive review ID"; + let id = args.next().ok_or(error)?; Some( id.to_str() .and_then(|id| id.parse::().ok()) .filter(|id| *id > 0) - .ok_or("show requires a positive review ID")?, + .ok_or(error)?, ) } else { - return Err("--review requires list or show ID".into()); + return Err(usage.into()); + }; + let delete_comment = if command == "delete" { + let error = "delete requires a positive comment ID"; + let id = args.next().ok_or(error)?; + Some( + id.to_str() + .and_then(|id| id.parse::().ok()) + .filter(|id| *id > 0) + .ok_or(error)?, + ) + } else { + None }; Ok(Self { id, + delete_comment, repo: None, json: false, all: false, @@ -50,7 +66,8 @@ impl Request { self.repo = Some(PathBuf::from(args.next().ok_or("--repo requires a path")?)); } else if arg == "--json" && !self.json { self.json = true; - } else if arg == "--all" && self.id.is_some() && !self.all { + } else if arg == "--all" && self.id.is_some() && self.delete_comment.is_none() && !self.all + { self.all = true; } else { return Err(format!( @@ -115,10 +132,19 @@ pub fn run(request: &Request, config: Option<&Path>) -> Result<(), 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 review = store + let mut review = store .as_ref() .ok_or("Review does not exist in this repository")? .load(&root, id)?; + if let Some(comment_id) = request.delete_comment { + let index = review + .comments + .iter() + .position(|comment| comment.id == comment_id) + .ok_or_else(|| format!("Comment {comment_id} does not exist in review {id}"))?; + review.comments.remove(index); + Store::open(&directory)?.save(&mut review)?; + } envelope(&review, request.all, true) } else { let reviews = match store { diff --git a/tests/review_cli.rs b/tests/review_cli.rs new file mode 100644 index 0000000..1d94cbe --- /dev/null +++ b/tests/review_cli.rs @@ -0,0 +1,130 @@ +use std::{fs, path::PathBuf, process::Command}; +use vamp::{ + plugin::TextPosition, + review::{Anchor, Comment, Review, Side, Store, Verdict}, +}; + +struct Directory(PathBuf); +impl Drop for Directory { + fn drop(&mut self) { + let _ = fs::remove_dir_all(&self.0); + } +} + +#[test] +fn cli_deletes_only_the_requested_comment_and_preserves_failed_targets() { + let directory = Directory( + std::env::temp_dir().join(format!("vamp-review-cli-delete-{}", std::process::id())), + ); + fs::create_dir(&directory.0).unwrap(); + let root = directory.0.canonicalize().unwrap(); + for name in ["repo", "other"] { + let output = Command::new("jj") + .args(["git", "init"]) + .arg(root.join(name)) + .env("JJ_CONFIG", "/dev/null") + .output() + .unwrap(); + assert!(output.status.success(), "{:?}", output); + } + let config = root.join("config.rb"); + fs::write( + &config, + "Vamp.configure { |config| config.review_directory = 'reviews' }", + ) + .unwrap(); + let storage = root.join("reviews"); + let mut store = Store::open(&storage).unwrap(); + let mut review = Review::new(root.join("repo")); + review.summary = "Keep summary".into(); + review.verdict = Some(Verdict::ChangesRequested); + review.comments = [1, u64::MAX] + .into_iter() + .map(|id| Comment { + id, + body: format!("Feedback {id}"), + resolved: id == u64::MAX, + anchor: Anchor { + revision: "snapshot".into(), + path: "file.rs".into(), + side: Side::After, + start: TextPosition { row: 0, column: 0 }, + end: TextPosition { row: 0, column: 1 }, + content: "界\n".into(), + }, + }) + .collect(); + store.save(&mut review).unwrap(); + let mut other = Review::new(root.join("other")); + other.comments = review.comments.clone(); + store.save(&mut other).unwrap(); + let id = review.storage_id.unwrap(); + let cli = |repo: &str, args: &[&str]| { + Command::new(env!("CARGO_BIN_EXE_vamp")) + .arg("--config") + .arg(&config) + .arg("--review") + .args(args) + .current_dir(root.join(repo)) + .output() + .unwrap() + }; + let id_text = id.to_string(); + for (repo, args) in [ + ("repo", vec!["delete", "999", "1"]), + ("repo", vec!["delete", &id_text, "2"]), + ("other", vec!["delete", &id_text, "1"]), + ] { + let output = cli(repo, &args); + assert!(!output.status.success(), "{:?}", output); + let loaded = store.load(&review.root, id).unwrap(); + assert_eq!(loaded.storage_revision, review.storage_revision); + assert_eq!(loaded.comments, review.comments); + } + let output = cli( + "repo", + &["delete", &id_text, "18446744073709551615", "--json"], + ); + assert!(output.status.success(), "{:?}", output); + let json: serde_json::Value = serde_json::from_slice(&output.stdout).unwrap(); + assert_eq!(json["id"], id); + assert_eq!( + json["counts"], + serde_json::json!({"total": 1, "resolved": 0, "unresolved": 1}) + ); + let loaded = store.load(&review.root, id).unwrap(); + assert_eq!(loaded.storage_revision, review.storage_revision + 1); + assert_eq!(loaded.comments, review.comments[..1]); + assert_eq!(loaded.summary, review.summary); + assert_eq!(loaded.verdict, review.verdict); + assert!(store.save(&mut review).is_err()); // Stale editors cannot resurrect the deletion. + let output = cli("repo", &["delete", &id_text, "18446744073709551615"]); + assert!(!output.status.success()); + assert_eq!( + store.load(&review.root, id).unwrap().storage_revision, + loaded.storage_revision + ); + let output = cli("repo", &["delete", &id_text, "1"]); + assert!(output.status.success(), "{:?}", output); + assert!( + String::from_utf8(output.stdout) + .unwrap() + .contains("0 total") + ); + let empty = store.load(&review.root, id).unwrap(); + assert!(empty.comments.is_empty()); + assert_eq!(empty.storage_revision, loaded.storage_revision + 1); + assert_eq!(empty.summary, review.summary); + assert_eq!( + store + .load(&other.root, other.storage_id.unwrap()) + .unwrap() + .comments, + other.comments + ); + drop(store); + fs::remove_dir_all(&storage).unwrap(); + let output = cli("repo", &["delete", &id_text, "1"]); + assert!(!output.status.success()); + assert!(!storage.exists()); +} diff --git a/tests/unit/cli_args.rs b/tests/unit/cli_args.rs index 0e0374c..72326b2 100644 --- a/tests/unit/cli_args.rs +++ b/tests/unit/cli_args.rs @@ -28,12 +28,38 @@ fn review_commands_preserve_filenames_and_validate_arguments() { .review .is_some() ); + assert!( + parse(&[ + "--review", + "delete", + "1", + "2", + "--repo", + ".", + "--json", + "--no-config" + ]) + .unwrap() + .unwrap() + .review + .is_some() + ); for args in [ vec!["--review"], vec!["--review", "show"], vec!["--review", "show", "0"], vec!["--review", "show", "-1"], vec!["--review", "show", "abc"], + vec!["--review", "delete"], + vec!["--review", "delete", "1"], + vec!["--review", "delete", "0", "1"], + vec!["--review", "delete", "9223372036854775808", "1"], + vec!["--review", "delete", "1", "0"], + vec!["--review", "delete", "1", "-1"], + vec!["--review", "delete", "1", "abc"], + vec!["--review", "delete", "1", "18446744073709551616"], + vec!["--review", "delete", "1", "2", "--all"], + vec!["--review", "delete", "1", "2", "3"], vec!["--review", "list", "--all"], vec!["--review", "list", "--repo"], vec!["--review", "list", "--repo", ".", "--repo", "."],