hl-valdemar/vamp
1
0
Fork 0
Code Issues Pull Requests Actions Packages Releases Wiki Activity

feat: scope local reviews to comparisons

Browse Source
This commit is contained in:
Valdemar Høgsvig Lorenzen
2026-10-25 01:00:58 +02:00
parent 815fcccbe8
commit 448c3bf477
15 changed files with 760 additions and 72 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/diff.rs
+45 -5
View File
@@ -773,6 +773,9 @@ pub struct DiffState {
pub error: Option<String>,
pub comparison: Option<Comparison>,
pub comparison_id: Option<u64>,
/// Frozen initial endpoint identities; checkout refreshes do not select a new review.
pub(crate) review_comparison: Option<[String; 2]>,
review_root: Option<PathBuf>,
pub reviewed_files: BTreeSet<usize>,
pub(crate) snapshot_roots: [Option<PathBuf>; 2],
pub files: Option<FilePicker>,
@@ -840,6 +843,8 @@ impl DiffState {
error: None,
comparison: None,
comparison_id: None,
review_comparison: None,
review_root: None,
reviewed_files: BTreeSet::new(),
files: None,
snapshot_roots: [None, None],
@@ -1221,6 +1226,13 @@ impl DiffState {
}
Ok(Outcome::CheckoutRefresh(Some(mut comparison)))
| Ok(Outcome::BookmarkRefresh(mut comparison)) => {
if bookmark_refresh {
self.review_root = Some(
std::fs::canonicalize(&comparison.root)
.unwrap_or_else(|_| comparison.root.clone()),
);
self.review_comparison = Some(comparison.revisions.clone());
}
let pinned = !bookmark_refresh
&& self
.after
@@ -1344,6 +1356,10 @@ impl DiffState {
self.error = None;
}
Ok(Outcome::Comparison(comparison)) if expected == Some(RequestKind::Comparison) => {
self.review_root = Some(
std::fs::canonicalize(&comparison.root)
.unwrap_or_else(|_| comparison.root.clone()),
);
self.live_files.clear();
self.modified_after.clear();
self.frozen_after.clear();
@@ -1364,6 +1380,7 @@ impl DiffState {
files.reveal_path(&file.path);
}
self.files = Some(files);
self.review_comparison = Some(comparison.revisions.clone());
self.comparison = Some(comparison);
self.comparison_id = Some(id);
self.snapshot_roots = [None, None];
@@ -1895,6 +1912,7 @@ impl DiffState {
self.phase = Phase::After;
self.comparison = None;
self.comparison_id = None;
self.review_comparison = None;
self.reviewed_files.clear();
self.queue(operation);
}
@@ -2208,6 +2226,27 @@ impl DiffState {
.files
.get(index)
.ok_or("No changed file is selected")?;
if comparison
.revisions
.iter()
.all(|revision| !revision.is_empty())
{
if !review.initialized && review.document.root.as_os_str().is_empty() {
review.document = crate::review::Review::new(
self.review_root
.as_ref()
.unwrap_or(&comparison.root)
.clone(),
);
review.document.comparison = self.review_comparison.clone();
review.initialized = true;
}
if &review.document.root != self.review_root.as_ref().unwrap_or(&comparison.root)
|| review.document.comparison != self.review_comparison
{
return Err("Select the review for this comparison first");
}
}
let reviewed = !self.reviewed_files.remove(&index);
if reviewed {
self.reviewed_files.insert(index);
@@ -2218,9 +2257,6 @@ impl DiffState {
.iter()
.all(|revision| !revision.is_empty())
{
if review.document.root.as_os_str().is_empty() {
review.document = crate::review::Review::new(comparison.root.clone());
}
let records = &mut review.document.reviewed_files;
records.retain(|record| {
record.revisions
@@ -2255,7 +2291,9 @@ impl DiffState {
let Some(comparison) = self.comparison.as_ref() else {
return;
};
if review.document.root != comparison.root {
if &review.document.root != self.review_root.as_ref().unwrap_or(&comparison.root)
|| review.document.comparison != self.review_comparison
{
return;
}
let records: BTreeMap<_, _> = review
@@ -2703,7 +2741,9 @@ impl DiffState {
self.modified_after.insert(self.file);
}
let comparison = self.comparison.as_ref().unwrap();
let record = (review.document.root == comparison.root)
let record = (&review.document.root
== self.review_root.as_ref().unwrap_or(&comparison.root)
&& review.document.comparison == self.review_comparison)
.then(|| {
review.document.reviewed_files.iter().position(|record| {
record.revisions
src/editor.rs
+30 -2
View File
@@ -4593,6 +4593,16 @@ impl Editor {
.as_mut()
.is_some_and(|diff| diff.apply_result(id, result));
}
let result = if let Ok(Outcome::BookmarkRefresh(comparison)) = &result
&& self.current_review_dirty()
&& self
.workspace_diff()
.is_some_and(|diff| diff.review_comparison.as_ref() != Some(&comparison.revisions))
{
Err("Save the local review before changing comparison endpoints".into())
} else {
result
};
self.outstanding_diff_request = None;
if self.diff.as_ref().is_some_and(DiffState::refresh_pending)
&& !matches!(&result, Ok(Outcome::CheckoutRefresh(None)))
@@ -4653,8 +4663,7 @@ impl Editor {
view.clamp(&self.active.lines);
self.active.view = view;
}
if self.review_directory.is_some()
&& self.resolver().is_none()
if self.resolver().is_none()
&& self
.workspace_diff()
.is_some_and(|diff| diff.phase == Phase::View)
@@ -10132,6 +10141,9 @@ impl Editor {
self.active.view.mode = Mode::Normal;
}
if entry.action == Action::DiffModeToggle {
if self.local_review_close_blocked() {
return None;
}
let Some(diff) = self.diff.as_mut() else {
self.set_message("No comparison is open");
return None;
@@ -10145,6 +10157,19 @@ impl Editor {
return None;
}
if entry.action == Action::DiffReviewToggle {
if self
.picker_diff()
.is_none_or(|diff| diff.phase != Phase::View)
{
self.set_message("No comparison is open");
return None;
}
if self.resolver().is_none()
&& let Err(error) = self.initialize_local_review()
{
self.set_message(format!("Local review: {error}"));
return None;
}
let mut review = std::mem::take(&mut self.local_review);
let result = self
.picker_diff_mut()
@@ -10254,6 +10279,9 @@ impl Editor {
self.active.view.command_line.clear();
self.active.view.mode = Mode::Normal;
}
if let Err(error) = self.initialize_local_review() {
self.set_message(format!("Local review: {error}"));
}
} else {
self.set_message("The diff.close command takes no arguments");
}
src/editor_review.rs
+70 -20
View File
@@ -34,12 +34,15 @@ impl Editor {
pub(super) fn initialize_local_review(&mut self) -> Result<(), String> {
let root = self.review_root()?;
if self.local_review.document.root == root {
let comparison = self.review_comparison();
if self.local_review.document.root == root
&& self.local_review.document.comparison == comparison
{
self.local_review.initialized = true;
return Ok(());
}
if self.current_review_dirty() {
return Err("Save the current review before changing repositories".into());
return Err("Save the current review before changing review scopes".into());
}
let reviews = match self.review_directory.as_deref() {
Some(directory) => crate::review::Store::open_read_only(directory)?
@@ -49,12 +52,17 @@ impl Editor {
None => Vec::new(),
};
self.detach_review_buffers();
let mut document = Review::new(root);
document.comparison = comparison.clone();
self.local_review = crate::review::ReviewState {
document: Review::new(root),
document,
initialized: true,
..Default::default()
};
let mut reviews = reviews;
let mut reviews: Vec<_> = reviews
.into_iter()
.filter(|review| review.comparison == comparison)
.collect();
if reviews.len() == 1 {
self.local_review.document = reviews.pop().unwrap();
} else if reviews.len() > 1 {
@@ -63,8 +71,30 @@ impl Editor {
Ok(())
}
fn review_comparison(&self) -> Option<[String; 2]> {
if self.active.review_buffer.is_some() {
return self.local_review.document.comparison.clone();
}
self.diff
.as_ref()
.and_then(|diff| diff.review_comparison.clone())
}
fn review_root(&self) -> Result<PathBuf, String> {
if let Some(diff) = self.workspace_diff() {
if self
.active
.review_buffer
.as_ref()
.is_some_and(|binding| binding.owner != self.tab_id)
{
return Err("Focus a source or comparison pane in this tab to select a review".into());
}
if self.active.review_buffer.is_some()
|| (self.active.path.is_none() && self.local_review.initialized && self.diff.is_none())
{
return Ok(self.local_review.document.root.clone());
}
if let Some(diff) = self.diff.as_ref() {
return diff
.comparison
.as_ref()
@@ -73,18 +103,16 @@ impl Editor {
})
.ok_or_else(|| "Comparison is still loading".into());
}
if self.active.review_buffer.is_some()
|| (self.active.path.is_none() && self.local_review.initialized)
{
return Ok(self.local_review.document.root.clone());
}
let path = self
.active
.identity
.as_deref()
.or(self.active.path.as_deref())
.ok_or("Save the source file before commenting")?;
crate::repository::discover_root(path.parent().ok_or("Source has no parent directory")?)
let root = crate::repository::discover_root(
path.parent().ok_or("Source has no parent directory")?,
)?;
Ok(fs::canonicalize(&root).unwrap_or(root))
}
fn open_review_selector(&mut self) -> Result<(), String> {
@@ -93,7 +121,12 @@ impl Editor {
.review_directory
.as_deref()
.ok_or("Review storage is not configured")?;
let reviews = crate::review::Store::open(directory)?.list(&root)?;
let comparison = self.review_comparison();
let reviews = crate::review::Store::open(directory)?
.list(&root)?
.into_iter()
.filter(|review| review.comparison == comparison)
.collect();
self.show_review_selector(reviews)
}
@@ -241,6 +274,10 @@ impl Editor {
root: PathBuf,
source: ReviewSource<'_>,
) -> Result<(), String> {
if self.current_review_dirty() {
return Err("Save the current local review before opening another".into());
}
let comparison = self.review_comparison();
let (document, path, dirty) = match source {
ReviewSource::Stored(id) => {
let directory = self
@@ -258,8 +295,15 @@ impl Editor {
Some(path.to_path_buf()),
self.review_directory.is_some(),
),
ReviewSource::New => (Review::new(root), None, true),
ReviewSource::New => {
let mut document = Review::new(root);
document.comparison = comparison.clone();
(document, None, true)
}
};
if document.comparison != comparison {
return Err("Review belongs to a different comparison or source scope".into());
}
// Retire old drafts before comment IDs can be reused.
self.detach_review_buffers();
let state = &mut self.local_review;
@@ -431,9 +475,6 @@ impl Editor {
{
return Err("This review command takes no arguments".into());
}
if self.local_review.document.root.as_os_str().is_empty() {
self.local_review.document = Review::new(root.clone());
}
if !matches!(action, Action::ReviewSave | Action::ReviewCopy) {
self.autosave_local_review()?;
}
@@ -602,7 +643,10 @@ impl Editor {
&self,
anchor: &Anchor,
) -> Option<std::ops::Range<TextPosition>> {
if self.active.review_buffer.is_some() || !anchor.revision.starts_with("working:") {
if self.local_review.document.comparison.is_some()
|| self.active.review_buffer.is_some()
|| !anchor.revision.starts_with("working:")
{
return None;
}
let path = self
@@ -621,7 +665,9 @@ impl Editor {
}
fn review_anchor_range(&self, anchor: &Anchor) -> Option<std::ops::Range<TextPosition>> {
if let Some(diff) = self.workspace_diff() {
if self.local_review.document.comparison.is_some()
&& let Some(diff) = self.workspace_diff()
{
return diff.review_anchor_range(anchor, &self.local_review);
}
let path = self.local_review.document.root.join(&anchor.path);
@@ -642,7 +688,9 @@ impl Editor {
)
}
fn add_review_comment(&mut self) -> Result<(), String> {
if let Some(owner) = self.diff_owner {
if self.local_review.document.comparison.is_some()
&& let Some(owner) = self.diff_owner
{
if self.pane_id != owner {
self.select_pane(owner);
}
@@ -801,7 +849,9 @@ impl Editor {
let anchor = anchor.clone();
let mut stale = !current;
self.local_review.selected = Some(id);
if let Some(owner) = self.diff_owner {
if self.local_review.document.comparison.is_some()
&& let Some(owner) = self.diff_owner
{
let target = self
.workspace_diff()
.and_then(|diff| diff.comparison.as_ref())
src/main.rs
+2
View File
@@ -99,6 +99,8 @@ Diff: :diff (Normal); choose Before then After; Working checkout After edits liv
Changed files: normal picker/explorer bindings, Ctrl+w close; A/M/D/R status and rename paths
Local reviews: :comment anchors exact Visual text, Visual Line rows, or the current line
Works in ordinary repository source buffers and diff text panes
Different resolved Before/After pairs select separate reviews; reopening a pair resumes its review
Working refreshes retain the active review; ordinary source reviews stay separate
Selected comments highlight their precise text
:review.edit/delete [ID], :review.next/previous, :review.overview
:review.summary; :review.verdict looks_good|changes_requested|none
src/review.rs
+67 -17
View File
@@ -195,6 +195,8 @@ pub struct ReviewedFile {
pub struct Review {
pub version: u32,
pub root: PathBuf,
/// Ordered frozen comparison endpoints; `None` is a source review.
pub comparison: Option<[String; 2]>,
pub summary: String,
pub verdict: Option<Verdict>,
pub comments: Vec<Comment>,
@@ -231,8 +233,9 @@ impl Review {
fs::canonicalize(&root).unwrap_or(root)
};
Self {
version: 4,
version: 5,
root,
comparison: None,
summary: String::new(),
verdict: None,
comments: Vec::new(),
@@ -249,12 +252,20 @@ impl Review {
.iter()
.filter(|comment| comment.resolved)
.count();
let (scope, before, arrow, after) = match &self.comparison {
Some([before, after]) => ("comparison ", before.as_str(), " → ", after.as_str()),
None => ("source", "", "", ""),
};
format!(
"Review ID: {}{}{}\n Verdict: {}; {} unresolved, {} resolved ({} total)\n",
"Review ID: {}{}{}\n Scope: {}{}{}{}\n Verdict: {}; {} unresolved, {} resolved ({} total)\n",
self.storage_id
.map_or_else(|| "null".into(), |id| id.to_string()),
if self.summary.is_empty() { "" } else { ": " },
self.summary,
scope,
before,
arrow,
after,
match self.verdict {
Some(Verdict::LooksGood) => "looks_good",
Some(Verdict::ChangesRequested) => "changes_requested",
@@ -325,12 +336,19 @@ impl Review {
}
fn validate(&self) -> Result<(), String> {
if self.version != 4 {
if self.version != 5 {
return Err(format!("Unsupported review version {}", self.version));
}
if !self.root.is_absolute() || !self.root.is_dir() {
return Err("Review repository root must be an existing absolute directory".into());
}
if let Some(revisions) = &self.comparison
&& revisions
.iter()
.any(|revision| revision.is_empty() || revision.chars().any(char::is_control))
{
return Err("Review comparison endpoints require immutable revision identities".into());
}
let mut ids = BTreeSet::new();
for comment in &self.comments {
if comment.id == 0 || !ids.insert(comment.id) {
@@ -392,7 +410,7 @@ impl Review {
}
let header: Header = serde_json::from_slice(bytes)
.map_err(|error| format!("Invalid review JSON: {error}"))?;
if header.version != 4 {
if header.version != 5 {
return Err(format!("Unsupported review version {}", header.version));
}
serde_json::from_slice(bytes).map_err(|error| format!("Invalid review JSON: {error}"))
@@ -495,7 +513,14 @@ const REVIEWS_SCHEMA: &str = "CREATE TABLE reviews (
root TEXT NOT NULL,
revision INTEGER NOT NULL CHECK (revision > 0),
summary TEXT NOT NULL,
verdict TEXT CHECK (verdict IN ('looks_good', 'changes_requested'))
verdict TEXT CHECK (verdict IN ('looks_good', 'changes_requested')),
before_revision TEXT,
after_revision TEXT,
CHECK (
(before_revision IS NULL AND after_revision IS NULL)
OR (before_revision IS NOT NULL AND after_revision IS NOT NULL
AND length(before_revision) > 0 AND length(after_revision) > 0)
)
) STRICT";
// SQLite integers are signed; decimal text preserves the model's full u64 comment IDs.
@@ -578,7 +603,7 @@ impl Store {
.map_err(database_error)?
.collect::<Result<_, _>>()
.map_err(database_error)?;
if version != 3 || schema != [COMMENTS_SCHEMA, REVIEWED_FILES_SCHEMA, REVIEWS_SCHEMA] {
if version != 4 || schema != [COMMENTS_SCHEMA, REVIEWED_FILES_SCHEMA, REVIEWS_SCHEMA] {
return Err(format!(
"Unsupported review database schema (version {version}); existing data was not changed"
));
@@ -646,7 +671,7 @@ impl Store {
.execute_batch(REVIEWED_FILES_SCHEMA)
.map_err(database_error)?;
transaction
.pragma_update(None, "user_version", 3)
.pragma_update(None, "user_version", 4)
.map_err(database_error)?;
transaction.commit().map_err(database_error)?;
} else {
@@ -673,7 +698,7 @@ impl Store {
let reviews = {
let mut statement = transaction
.prepare(
"SELECT id, root, revision, summary, verdict FROM reviews WHERE root = ?1 ORDER BY id DESC",
"SELECT id, root, revision, summary, verdict, before_revision, after_revision FROM reviews WHERE root = ?1 ORDER BY id DESC",
)
.map_err(database_error)?;
let mut rows = statement.query([root_text]).map_err(database_error)?;
@@ -698,7 +723,7 @@ impl Store {
.map_err(database_error)?;
let review = {
let mut statement = transaction
.prepare("SELECT id, root, revision, summary, verdict FROM reviews WHERE id = ?1 AND root = ?2")
.prepare("SELECT id, root, revision, summary, verdict, before_revision, after_revision FROM reviews WHERE id = ?1 AND root = ?2")
.map_err(database_error)?;
let mut rows = statement
.query(params![id, root_text])
@@ -734,6 +759,9 @@ impl Store {
_ => return Err("Invalid review storage identity".into()),
};
let mut size = review.root.as_os_str().len() + review.summary.len();
if let Some(revisions) = &review.comparison {
size += revisions[0].len() + revisions[1].len();
}
for comment in &review.comments {
size = size
.checked_add(
@@ -768,26 +796,34 @@ impl Store {
Some(Verdict::ChangesRequested) => Some("changes_requested"),
None => None,
};
let before = review
.comparison
.as_ref()
.map(|revisions| revisions[0].as_str());
let after = review
.comparison
.as_ref()
.map(|revisions| revisions[1].as_str());
let transaction = self
.connection
.transaction_with_behavior(TransactionBehavior::Immediate)
.map_err(database_error)?;
let id = if let Some(id) = review.storage_id {
let changed = transaction.execute(
"UPDATE reviews SET revision = ?1, summary = ?2, verdict = ?3 WHERE id = ?4 AND root = ?5 AND revision = ?6",
params![revision, review.summary, verdict, id, root, review.storage_revision],
"UPDATE reviews SET revision = ?1, summary = ?2, verdict = ?3 WHERE id = ?4 AND root = ?5 AND revision = ?6 AND before_revision IS ?7 AND after_revision IS ?8",
params![revision, review.summary, verdict, id, root, review.storage_revision, before, after],
).map_err(database_error)?;
if changed != 1 {
return Err(
"Review changed in another session; draft kept, reload before saving".into(),
"Review changed in another session or comparison scope changed; draft kept, reload before saving".into(),
);
}
id
} else {
transaction
.execute(
"INSERT INTO reviews (root, revision, summary, verdict) VALUES (?1, ?2, ?3, ?4)",
params![root, revision, review.summary, verdict],
"INSERT INTO reviews (root, revision, summary, verdict, before_revision, after_revision) VALUES (?1, ?2, ?3, ?4, ?5, ?6)",
params![root, revision, review.summary, verdict, before, after],
)
.map_err(database_error)?;
transaction.last_insert_rowid()
@@ -906,9 +942,15 @@ fn stored_review(
|row| row.get(0),
)
.map_err(database_error)?;
if stored_bytes < 0
|| stored_bytes as u64 + stored_root.len() as u64 + summary.len() as u64 > LIMIT as u64
{
let mut header_bytes = stored_root.len() + summary.len();
for index in [5, 6] {
if let rusqlite::types::ValueRef::Text(bytes) =
row.get_ref(index).map_err(database_error)?
{
header_bytes += bytes.len();
}
}
if stored_bytes < 0 || stored_bytes as u64 + header_bytes as u64 > LIMIT as u64 {
return Err("Review exceeds the 64 MiB limit".into());
}
let mut review = Review {
@@ -916,6 +958,14 @@ fn stored_review(
..Review::default()
};
review.summary = summary.to_owned();
review.comparison = match (
row.get::<_, Option<String>>(5).map_err(database_error)?,
row.get::<_, Option<String>>(6).map_err(database_error)?,
) {
(None, None) => None,
(Some(before), Some(after)) => Some([before, after]),
_ => return Err("Invalid stored review comparison endpoints".into()),
};
review.verdict = match row
.get::<_, Option<String>>(4)
.map_err(database_error)?
src/review_cli.rs
+1
View File
@@ -109,6 +109,7 @@ fn envelope(review: &Review, all: bool, comments: bool) -> Value {
"repository": review.root,
"id": review.storage_id,
"revision": review.storage_revision,
"comparison": review.comparison,
"summary": review.summary,
"verdict": review.verdict,
"counts": { "total": review.comments.len(), "resolved": resolved, "unresolved": review.comments.len() - resolved },