From 6ae8bf06a8b3aaeb9a46b359977de8fc5b3ab31a Mon Sep 17 00:00:00 2001 From: Valdemar Lorenzen Date: Fri, 16 Oct 2026 15:42:33 +0200 Subject: [PATCH] feat: resolve local review comments manually --- docs/configuration.md | 6 +- docs/editing.md | 7 ++- src/editor.rs | 126 ++++++++++++++++++++++++++++++++++++++++++ src/editor_review.rs | 52 ++++++++++++++++- src/review.rs | 89 ++++++++++++++++++++++++++--- src/tui.rs | 2 + 6 files changed, 267 insertions(+), 15 deletions(-) diff --git a/docs/configuration.md b/docs/configuration.md index dea8b29..85146f4 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -230,7 +230,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.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.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` | @@ -316,7 +316,7 @@ Ordinary `config.core.file_picker` and `config.core.file_explorer` bindings work `config.core.diff.review.toggle` is a non-mutating Normal-mode action, also available as `:diff.review.toggle` and in the palette. It marks/unmarks the highlighted changed-file leaf when the comparison's explorer is focused, or the current comparison file from a text pane. Read-only snapshots permit review marks. The default `c` shortcut applies only while browsing the diff's changed-file tree; explicit bindings and disabled keys override it. For example, `config.bind "ctrl+g", config.core.diff.review.toggle`; use the palette or a control-key binding while filtering, including List view, so printable input stays in the query. -Local comments use `config.core.diff.comment` in Normal or Visual mode; the remaining `config.core.diff.review` actions in the table above use Normal mode. They have no default bindings and are available through the palette and canonical Ex commands. `edit`, `delete`, `verdict`, `save`, and `open` open argument prompts when selected as actions. For example: +Local comments use `config.core.diff.comment` in Normal or Visual mode; the remaining `config.core.diff.review` actions in the table above use Normal mode. They have no default bindings and are available through the palette and canonical Ex commands. `edit`, `delete`, `resolve`, `unresolve`, `verdict`, `save`, and `open` open argument prompts when selected as actions. Comment IDs are optional and default to the overview entry under the caret or selected comment. For example: ```ruby config.bind "leader r c", config.core.diff.comment @@ -325,7 +325,7 @@ config.bind "leader r o", config.core.diff.review.overview config.bind "leader r n", config.core.diff.review.next ``` -Review editing uses a floating window without adding workspace splits. Dismissing or switching editors commits text to local SQLite; verdicts and deletions commit immediately. `:w` commits explicitly; `:diff.review.save path` exports JSON without redirecting autosaves. `config.review_directory` defaults to `nil`, selecting `$XDG_DATA_HOME/vamp/reviews/` (absolute XDG path only), otherwise `~/.local/share/vamp/reviews/`. String overrides are resolved relative to the configuration file; empty/NUL-containing strings and non-string/non-nil values are rejected. These actions do not contact a forge. See [local review behavior](editing.md#repository-diff). +Review editing uses a floating window without adding workspace splits. Dismissing or switching editors commits text to local SQLite; verdicts, deletions, and resolution changes commit immediately. `:w` commits explicitly; `:diff.review.save path` exports JSON without redirecting autosaves. `config.review_directory` defaults to `nil`, selecting `$XDG_DATA_HOME/vamp/reviews/` (absolute XDG path only), otherwise `~/.local/share/vamp/reviews/`. String overrides are resolved relative to the configuration file; empty/NUL-containing strings and non-string/non-nil values are rejected. These actions do not contact a forge. See [local review behavior](editing.md#repository-diff). `config.core.pane.close` and `config.core.pane.force_close` close a focused **docked** file list instead of its editor pane, without discarding buffers or exiting the comparison. A floating picker does not intercept these actions: normal close still protects underlying dirty buffers, while force-close retains its existing discard semantics. For example, bind `config.bind "ctrl+x", config.core.pane.close`; focused tree/selection overrides and printable filter input retain priority. Palette-originated close actions retain the originating dock target through force confirmation. diff --git a/docs/editing.md b/docs/editing.md index b422b39..84b9eae 100644 --- a/docs/editing.md +++ b/docs/editing.md @@ -461,6 +461,7 @@ Comments, summaries, and the review overview open in a focused floating editor o | `:diff.comment` | Add a comment on the current line or exact selected text range, on the focused side | | `:diff.review.edit [id]` | Edit an existing comment; defaults to the overview entry under the caret or selected comment | | `:diff.review.delete [id]` | Delete that draft comment | +| `: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.summary` | Compose the review summary | @@ -470,16 +471,18 @@ Comments, summaries, and the review overview open in a focused floating editor o | `:diff.review.open [id or path]` | Show stored reviews, open a stored review ID, or import a JSON review as a new review | | `:diff.review.new` | Create a separate stored review for the current repository | -Reviews save automatically to a local SQLite database when you dismiss or switch comment/summary editors. Verdicts and deletions commit immediately; typing alone does not write per keystroke. **`:w`** explicitly commits the entire review, including hidden drafts. `:diff.review.save path` or `:w path` additionally exports JSON; later autosaves still go to SQLite and leave that export unchanged. Imports create a new stored review identity rather than overwriting another review. +Reviews save automatically to a local SQLite database when you dismiss or switch comment/summary editors. Verdicts, deletions, and manual resolution changes commit immediately; typing alone does not write per keystroke. **`:w`** explicitly commits the entire review, including hidden drafts. `:diff.review.save path` or `:w path` additionally exports JSON; later autosaves still go to SQLite and leave that export unchanged. Imports create a new stored review identity rather than overwriting another review. 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. Normal destructive closes remain guarded until persistence succeeds. `: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. -Version 2 JSON is the agent-readable sharing format: repository root, summary, optional verdict, and comments with stable IDs, bodies, and anchors. Database review IDs and optimistic storage revisions stay internal. Each anchor retains its immutable comparison revision, side, relative path (old path for renamed Before files), and exact whole-file text including the final newline. `start` and `end` contain zero-based source `row` and extended-grapheme `column` coordinates; `end` is exclusive. Columns count complete Unicode graphemes, not bytes, code points, UTF-16 units, or screen cells. Overview/window labels display one-based rows and columns. Only the current format is supported. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Each stored or exported review is limited to 64 MiB. +Version 3 JSON is the agent-readable sharing format: repository root, summary, optional verdict, and comments with stable IDs, bodies, required boolean `resolved` states, and anchors. New comments are unresolved. Database review IDs and optimistic storage revisions stay internal. Each anchor retains its immutable comparison revision, side, relative path (old path for renamed Before files), and exact whole-file text including the final newline. `start` and `end` contain zero-based source `row` and extended-grapheme `column` coordinates; `end` is exclusive. Columns count complete Unicode graphemes, not bytes, code points, UTF-16 units, or screen cells. Overview/window labels display one-based rows and columns. Older formats, including version 2, are rejected and preserved without migration. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Each stored or exported review is limited to 64 MiB. Reopening requires the same repository root. Comments whose revision, path, or exact file text no longer matches are **stale**: their original anchors remain unchanged and their gutter markers disappear. Unloaded files are shown as **not checked** until visited; refresh the overview to update its statuses. There is no automatic reanchoring, server, authentication, or review-publishing operation. +Resolution is a manual human decision, independent of anchor freshness: a comment can be both resolved and stale. Overview entries and comment titles label resolved/unresolved status. Resolved comments remain in the overview and next/previous navigation, and can still be edited or reopened. Resolving never changes an anchor or deletes feedback. + Text panes show change excerpts with **10 unchanged lines above and below** each change, preserving original file line numbers. Overlapping or touching excerpts merge; omitted stretches show an unchanged-line count in both panes. Files with only metadata changes show their notice or “No text changes” rather than unrelated source. diff --git a/src/editor.rs b/src/editor.rs index 3804c2d..4c6256d 100644 --- a/src/editor.rs +++ b/src/editor.rs @@ -945,6 +945,20 @@ core_actions![ true, true ), + ( + "core.diff.review.resolve", + Action::ReviewResolve, + &[Mode::Normal], + true, + true + ), + ( + "core.diff.review.unresolve", + Action::ReviewUnresolve, + &[Mode::Normal], + true, + true + ), ( "core.diff.review.next", Action::ReviewNext, @@ -5254,6 +5268,8 @@ impl Editor { | Action::DiffComment | Action::ReviewEdit | Action::ReviewDelete + | Action::ReviewResolve + | Action::ReviewUnresolve | Action::ReviewNext | Action::ReviewPrevious | Action::ReviewOverview @@ -8453,6 +8469,8 @@ impl Editor { | Action::DiffComment | Action::ReviewEdit | Action::ReviewDelete + | Action::ReviewResolve + | Action::ReviewUnresolve | Action::ReviewNext | Action::ReviewPrevious | Action::ReviewOverview @@ -9367,6 +9385,8 @@ impl Editor { Action::DiffComment | Action::ReviewEdit | Action::ReviewDelete + | Action::ReviewResolve + | Action::ReviewUnresolve | Action::ReviewNext | Action::ReviewPrevious | Action::ReviewOverview @@ -16090,6 +16110,112 @@ mod tests { ); } + #[test] + fn review_resolution_persists_without_hiding_comments_or_changing_stale_anchors() { + use crate::review::{Review, Store}; + let directory = Directory::new(); + let root = directory.0.canonicalize().unwrap(); + let storage = root.join("data"); + let mut first = editor(); + first.set_review_directory(storage.clone()); + historical_diff(&mut first, &root, vec!["old".into()], vec!["new".into()]); + first.execute("diff.comment"); + type_text(&mut first, "iFeedback"); + first.handle(Input::Escape); + assert!( + first + .review_panel_title() + .unwrap() + .contains(" · unresolved · ") + ); + first.execute("diff.review.resolve"); + assert!( + first + .context() + .text + .contains("Comment #1 · resolved · current") + ); + let store = Store::open(&storage).unwrap(); + let document = &first.workspace_diff().unwrap().local_review.document; + let id = document.storage_id.unwrap(); + let anchor = document.comments[0].anchor.clone(); + assert!(store.load(&root, id).unwrap().comments[0].resolved); + first.execute("diff.review.next"); + assert_eq!(first.context().text, "Feedback"); + assert!( + first + .review_panel_title() + .unwrap() + .contains(" · resolved · ") + ); + let export = root.join("resolved.json"); + first.execute(&format!("diff.review.save {}", export.display())); + assert!(Review::load(&export, &root).unwrap().comments[0].resolved); + let mut stale = editor(); + stale.set_review_directory(storage.clone()); + historical_diff( + &mut stale, + &root, + vec!["changed".into()], + vec!["new".into()], + ); + stale.execute("diff.review.overview"); + assert!( + stale + .context() + .text + .contains("Comment #1 · resolved · stale") + ); + first.execute("diff.review.unresolve 1"); + assert!(!store.load(&root, id).unwrap().comments[0].resolved); + stale.execute("diff.review.unresolve 1"); + assert!(stale.current_review_dirty()); + assert!( + !stale + .workspace_diff() + .unwrap() + .local_review + .document + .comments[0] + .resolved + ); + assert_eq!( + stale + .workspace_diff() + .unwrap() + .local_review + .document + .comments[0] + .anchor, + anchor + ); + assert!(stale.message().contains("changed in another session")); + first.execute("diff.review.resolve"); + assert!( + first + .context() + .text + .contains("Comment #1 · resolved · current") + ); + first.execute("diff.review.unresolve"); + assert!( + first + .context() + .text + .contains("Comment #1 · unresolved · current") + ); + first.execute(&format!("diff.review.open {}", export.display())); + let imported = &first.workspace_diff().unwrap().local_review.document; + assert_ne!(imported.storage_id, Some(id)); + assert!( + store + .load(&root, imported.storage_id.unwrap()) + .unwrap() + .comments[0] + .resolved + ); + } + #[test] fn sqlite_review_conflicts_and_write_failures_keep_dirty_drafts() { use crate::review::Store; diff --git a/src/editor_review.rs b/src/editor_review.rs index da744ab..d60e166 100644 --- a/src/editor_review.rs +++ b/src/editor_review.rs @@ -306,6 +306,8 @@ impl Editor { action, Action::ReviewEdit | Action::ReviewDelete + | Action::ReviewResolve + | Action::ReviewUnresolve | Action::ReviewVerdict | Action::ReviewSave | Action::ReviewOpen @@ -371,6 +373,40 @@ impl Editor { let id = self.review_comment_id(args)?; self.show_review_comment(id)?; } + Action::ReviewResolve | Action::ReviewUnresolve => { + let id = self.review_comment_id(args)?; + let resolved = action == Action::ReviewResolve; + let state = &mut self.review_diff_mut().unwrap().local_review; + let comment = state + .document + .comments + .iter_mut() + .find(|comment| comment.id == id) + .unwrap(); + if comment.resolved != resolved { + comment.resolved = resolved; + state.dirty = true; + } + let status = if resolved { "resolved" } else { "unresolved" }; + for buffer in std::iter::once(&mut self.active).chain(self.parked.values_mut()) { + if let Some(binding) = buffer.review_buffer.as_mut() + && binding.owner == self.tab_id + && binding.kind == ReviewText::Comment(id) + { + binding.title = binding.title.replacen( + if resolved { + " · unresolved · " + } else { + " · resolved · " + }, + &format!(" · {status} · "), + 1, + ); + } + } + self.autosave_local_review()?; + self.open_review_overview()?; + } Action::ReviewDelete => { let id = self.review_comment_id(args)?; let state = &mut self.review_diff_mut().unwrap().local_review; @@ -562,6 +598,7 @@ impl Editor { id, anchor, body: String::new(), + resolved: false, }); state.dirty = true; self.show_review_comment(id) @@ -578,8 +615,13 @@ impl Editor { .ok_or("Comment no longer exists")?; let text = comment.body.clone(); let anchor = &comment.anchor; + let status = if comment.resolved { + "resolved" + } else { + "unresolved" + }; let title = format!( - "Comment #{id} · {}:{}:{}–{}:{} · {:?} · :w saves review", + "Comment #{id} · {status} · {}:{}:{}–{}:{} · {:?} · :w saves review", anchor.path.display(), anchor.start.row + 1, anchor.start.column + 1, @@ -714,10 +756,15 @@ impl Editor { } else { "stale" }; + let resolution = if comment.resolved { + "resolved" + } else { + "unresolved" + }; use std::fmt::Write as _; write!( text, - "\n## Comment #{} · {status}\n{}:{}:{}–{}:{} · {:?}\nRevision: {}\n{}\n", + "\n## Comment #{} · {resolution} · {status}\n{}:{}:{}–{}:{} · {:?}\nRevision: {}\n{}\n", comment.id, anchor.path.display(), anchor.start.row + 1, @@ -774,6 +821,7 @@ impl Editor { self.select_pane(panel); if let Some(id) = existing { self.switch_buffer(id); + self.active.review_buffer.as_mut().unwrap().title = title; if matches!(kind, ReviewText::Overview | ReviewText::Reviews) { self.active.lines = split_lines(text); self.active.saved_lines.clone_from(&self.active.lines); diff --git a/src/review.rs b/src/review.rs index b4df995..67c7b5e 100644 --- a/src/review.rs +++ b/src/review.rs @@ -87,6 +87,7 @@ pub struct Comment { pub id: u64, pub anchor: Anchor, pub body: String, + pub resolved: bool, } #[derive(Clone, Debug, Serialize, Deserialize)] @@ -130,7 +131,7 @@ impl Review { fs::canonicalize(&root).unwrap_or(root) }; Self { - version: 2, + version: 3, root, summary: String::new(), verdict: None, @@ -142,7 +143,7 @@ impl Review { } fn validate(&self) -> Result<(), String> { - if self.version != 2 { + if self.version != 3 { return Err(format!("Unsupported review version {}", self.version)); } if !self.root.is_absolute() || !self.root.is_dir() { @@ -188,10 +189,22 @@ impl Review { Ok(()) } + fn from_json(bytes: &[u8]) -> Result { + #[derive(Deserialize)] + struct Header { + version: u32, + } + let header: Header = serde_json::from_slice(bytes) + .map_err(|error| format!("Invalid review JSON: {error}"))?; + if header.version != 3 { + return Err(format!("Unsupported review version {}", header.version)); + } + serde_json::from_slice(bytes).map_err(|error| format!("Invalid review JSON: {error}")) + } + pub fn load(path: &Path, root: &Path) -> Result { let bytes = read_file(path)?.ok_or("Review file does not exist")?; - let mut review: Self = serde_json::from_slice(&bytes) - .map_err(|error| format!("Invalid review JSON: {error}"))?; + let mut review = Self::from_json(&bytes)?; review.validate()?; let expected = fs::canonicalize(root).map_err(|error| error.to_string())?; let actual = fs::canonicalize(&review.root).map_err(|error| error.to_string())?; @@ -483,8 +496,7 @@ fn stored_review(row: &rusqlite::Row<'_>, root: &Path) -> Result if bytes.len() > LIMIT { return Err("Review exceeds the 64 MiB limit".into()); } - let mut review: Review = serde_json::from_slice(bytes) - .map_err(|error| format!("Invalid stored review JSON: {error}"))?; + let mut review = Review::from_json(bytes)?; review.validate()?; if review.root != root { return Err("Stored review belongs to a different repository".into()); @@ -578,6 +590,7 @@ mod tests { content: "a👩‍🔬e\u{301}\r\n界x\n".into(), }, body: "Please preserve 👩‍🔬".into(), + resolved: true, }); { let mut store = Store::open(&storage).unwrap(); @@ -606,7 +619,8 @@ mod tests { loaded.save(&path).unwrap(); let exported = fs::read(&path).unwrap(); let json: serde_json::Value = serde_json::from_slice(&exported).unwrap(); - assert_eq!(json["version"], 2); + assert_eq!(json["comments"][0]["resolved"], true); + assert_eq!(json["version"], 3); assert!(json.get("storage_id").is_none()); assert!(json.get("storage_revision").is_none()); loaded.summary = "Database only".into(); @@ -857,6 +871,7 @@ mod tests { content: "first\nlast\n".into(), }, body: "Please adjust this".into(), + resolved: false, }); review.save(&path).unwrap(); let mut loaded = Review::load(&path, &root).unwrap(); @@ -895,7 +910,7 @@ mod tests { assert!(Review::new(root.clone()).save(&path).is_err()); let valid = fs::read(&path).unwrap(); for (field, value) in [ - ("version", serde_json::json!(3)), + ("version", serde_json::json!(2)), ("root", serde_json::json!(root.join("other"))), ( "comments", @@ -948,6 +963,46 @@ mod tests { } } + #[test] + fn stored_version_two_is_rejected_without_rewriting_it() { + let directory = directory(); + let root = directory.0.canonicalize().unwrap(); + let mut store = Store::open(&root.join("storage")).unwrap(); + let mut review = Review::new(root.clone()); + store.save(&mut review).unwrap(); + let mut old = serde_json::to_value(&review).unwrap(); + old["version"] = 2.into(); + let document = serde_json::to_string(&old).unwrap(); + store + .connection + .execute( + "UPDATE reviews SET document = ?1 WHERE id = ?2", + params![document, review.storage_id.unwrap()], + ) + .unwrap(); + assert!( + store + .load(&root, review.storage_id.unwrap()) + .unwrap_err() + .contains("Unsupported review version 2") + ); + assert!( + store + .list(&root) + .unwrap_err() + .contains("Unsupported review version 2") + ); + let preserved: String = store + .connection + .query_row( + "SELECT document FROM reviews WHERE id = ?1", + [review.storage_id.unwrap()], + |row| row.get(0), + ) + .unwrap(); + assert_eq!(preserved, document); + } + #[test] fn exact_grapheme_ranges_roundtrip_and_reject_invalid_coordinates() { use crate::plugin::TextPosition; @@ -966,12 +1021,30 @@ mod tests { content: "a👩‍🔬e\u{301}\n界x\n".into(), }, body: "Exact text".into(), + resolved: false, }); review.save(&path).unwrap(); let mut loaded = Review::load(&path, &root).unwrap(); assert_eq!(loaded.comments, review.comments); assert_eq!(loaded.comments[0].anchor.rows(), 0..=1); let valid = serde_json::to_value(&review).unwrap(); + let mut missing_resolution = valid.clone(); + missing_resolution["comments"][0] + .as_object_mut() + .unwrap() + .remove("resolved"); + for version in [2, 3] { + missing_resolution["version"] = version.into(); + let bytes = serde_json::to_vec(&missing_resolution).unwrap(); + fs::write(&path, &bytes).unwrap(); + let error = Review::load(&path, &root).unwrap_err(); + assert!(error.contains(if version == 2 { + "Unsupported review version 2" + } else { + "missing field `resolved`" + })); + assert_eq!(fs::read(&path).unwrap(), bytes); + } for (field, value) in [ ("start", serde_json::json!({"row": 0, "column": 4})), ("end", serde_json::json!({"row": 1, "column": 3})), diff --git a/src/tui.rs b/src/tui.rs index c3c6b57..556dd14 100644 --- a/src/tui.rs +++ b/src/tui.rs @@ -9409,6 +9409,7 @@ mod tests { content: before, }, body: "Before range".into(), + resolved: false, }, Comment { id: 2, @@ -9421,6 +9422,7 @@ mod tests { content: after, }, body: "After line".into(), + resolved: false, }, ]; diff.local_review.selected = Some(1);