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

feat: add local diff review workflow

Browse Source
This commit is contained in:
Valdemar Lorenzen
2026-10-16 13:10:07 +02:00
parent 930735c537
commit 0446ec86c7
15 changed files with 1871 additions and 7 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/diff.rs
+79
View File
@@ -139,6 +139,8 @@ pub struct ChangedFile {
}
#[derive(Debug)]
pub struct Comparison {
/// Immutable comparison-context IDs; captured review text also freezes live edits.
pub revisions: [String; 2],
pub before: String,
pub after: String,
pub files: Vec<ChangedFile>,
@@ -273,6 +275,7 @@ pub struct DiffState {
pub loading: bool,
pub error: Option<String>,
pub comparison: Option<Comparison>,
pub local_review: crate::review::ReviewState,
pub comparison_id: Option<u64>,
pub reviewed_files: BTreeSet<usize>,
pub(crate) snapshot_roots: [Option<PathBuf>; 2],
@@ -330,6 +333,7 @@ impl DiffState {
loading: true,
error: None,
comparison: None,
local_review: Default::default(),
comparison_id: None,
reviewed_files: BTreeSet::new(),
files: None,
@@ -366,6 +370,7 @@ impl DiffState {
}
pub(crate) fn set_resolver_paths(&mut self, root: PathBuf, paths: Vec<PathBuf>, file: usize) {
let comparison = self.comparison.get_or_insert_with(|| Comparison {
revisions: Default::default(),
before: "Resolution".into(),
after: "Other version".into(),
files: Vec::new(),
@@ -1710,6 +1715,40 @@ impl DiffState {
lines
}
pub fn review_anchor_current(&self, anchor: &crate::review::Anchor) -> bool {
use crate::review::Side;
let Some(comparison) = &self.comparison else {
return false;
};
if self.local_review.document.root != comparison.root {
return false;
}
comparison.files.iter().enumerate().any(|(index, file)| {
if !file.loaded || file.notice.is_some() {
return false;
}
let (path, lines, eof) = match anchor.side {
Side::Before => (
file.old_path.as_deref().unwrap_or(&file.path),
&file.before,
file.before_eof,
),
Side::After => (
file.path.as_path(),
&file.after,
file.after_eof && !self.live_files.contains(&index),
),
};
anchor.matches_lines(
&comparison.revisions[anchor.side.index()],
path,
anchor.side,
lines,
eof,
)
})
}
pub(crate) fn corresponding_row(&self, from: Focus, to: Focus, row: usize) -> Option<usize> {
let aligned = self.rows.iter().position(|aligned| match from {
Focus::Before => aligned.before == Some(row),
@@ -3172,6 +3211,7 @@ mod tests {
fn comparison() -> Comparison {
Comparison {
revisions: ["abcdef123".into(), "working:snapshot".into()],
before: "abcdef123".into(),
after: "Working checkout".into(),
root: PathBuf::new(),
@@ -4159,6 +4199,7 @@ mod tests {
assert!(editor.apply_diff_result(
id,
Ok(Outcome::Comparison(Comparison {
revisions: ["working:snapshot".into(), "working:snapshot".into()],
before: "Working".into(),
after: "Working".into(),
files: vec![],
@@ -4190,6 +4231,7 @@ mod tests {
Category::Commits,
));
state.comparison = Some(Comparison {
revisions: ["base".into(), "working:snapshot".into()],
root: PathBuf::new(),
before: "base".into(),
after: "Working".into(),
@@ -4982,4 +5024,41 @@ mod tests {
));
assert!(editor.diff().unwrap().error.is_none());
}
#[test]
fn review_anchors_match_frozen_context_and_exact_live_eof_without_allocating() {
use crate::review::{Anchor, Side};
let mut state = live_state(vec!["old".into()], vec!["new".into()], true);
let anchor = Anchor {
revision: "working:snapshot".into(),
path: "file".into(),
side: Side::After,
start_line: 1,
end_line: 1,
content: "new\n".into(),
};
assert!(state.review_anchor_current(&anchor));
state.live_files.insert(0);
assert!(!state.review_anchor_current(&anchor));
state.comparison.as_mut().unwrap().files[0]
.after
.push(String::new());
assert!(state.review_anchor_current(&anchor));
state.comparison.as_mut().unwrap().files[0].after[0] = "changed".into();
assert!(!state.review_anchor_current(&anchor));
state.comparison.as_mut().unwrap().files[0].old_path = Some("original".into());
let before = Anchor {
revision: "base".into(),
path: "original".into(),
side: Side::Before,
content: "old\n".into(),
..anchor
};
assert!(state.review_anchor_current(&before));
state.comparison.as_mut().unwrap().files[0].loaded = false;
assert!(!state.review_anchor_current(&before));
state.comparison.as_mut().unwrap().files[0].loaded = true;
state.comparison.as_mut().unwrap().revisions[0] = "other".into();
assert!(!state.review_anchor_current(&before));
}
}
src/editor.rs
+361 -6
View File
@@ -1,3 +1,7 @@
#[path = "editor_review.rs"]
mod local_review;
use local_review::ReviewBuffer;
use crate::chrome::{ChromeLine, PopupConfig, validate_name};
use crate::clipboard::{ClipboardMode, SystemClipboard};
use crate::diff::{DiffState, Focus, Operation, Outcome, Phase, RevisionTarget, ViewMode};
@@ -917,6 +921,71 @@ core_actions![
&[Mode::Normal],
true,
),
(
"core.diff.comment",
Action::DiffComment,
&[Mode::Normal, Mode::Visual],
true
),
(
"core.diff.review.edit",
Action::ReviewEdit,
&[Mode::Normal],
true,
true
),
(
"core.diff.review.delete",
Action::ReviewDelete,
&[Mode::Normal],
true,
true
),
(
"core.diff.review.next",
Action::ReviewNext,
&[Mode::Normal],
true
),
(
"core.diff.review.previous",
Action::ReviewPrevious,
&[Mode::Normal],
true
),
(
"core.diff.review.overview",
Action::ReviewOverview,
&[Mode::Normal],
true
),
(
"core.diff.review.summary",
Action::ReviewSummary,
&[Mode::Normal],
true
),
(
"core.diff.review.verdict",
Action::ReviewVerdict,
&[Mode::Normal],
true,
true
),
(
"core.diff.review.save",
Action::ReviewSave,
&[Mode::Normal],
true,
true
),
(
"core.diff.review.open",
Action::ReviewOpen,
&[Mode::Normal],
true,
true
),
(
"core.word_wrap.toggle",
Action::WordWrapToggle,
@@ -1758,6 +1827,7 @@ struct GutterChanges {
struct Buffer {
id: usize,
diff_snapshot: Option<(u64, usize, Focus)>,
review_buffer: Option<ReviewBuffer>,
resolver_readonly: bool,
snapshot_document_path: Option<PathBuf>,
snapshot_root: Option<PathBuf>,
@@ -1816,6 +1886,7 @@ impl Buffer {
Ok(Self {
id,
diff_snapshot: None,
review_buffer: None,
resolver_readonly: false,
snapshot_document_path: None,
snapshot_root: None,
@@ -3844,6 +3915,9 @@ impl Editor {
}
fn close_tab(&mut self, force: bool) {
if !force && self.local_review_close_blocked() {
return;
}
if self.resolver_close_blocked() {
self.set_message(
"Resolution drafts are unsaved; write them before closing the workspace",
@@ -3887,6 +3961,7 @@ impl Editor {
self.finish_pane_interaction();
if let Some(owner) = self.diff_owner {
self.with_pane(owner, |editor| {
editor.detach_review_buffers();
editor.release_diff_buffers();
editor.diff = None;
});
@@ -3954,7 +4029,11 @@ impl Editor {
return;
}
self.finish_pane_interaction();
if self.diff_owner == Some(id) && !force && self.local_review_close_blocked() {
return;
}
if self.diff_owner == Some(id) {
self.detach_review_buffers();
self.release_diff_buffers();
self.diff = None;
self.parked_diff = None;
@@ -4402,10 +4481,15 @@ impl Editor {
self.set_message(format!("LSP: definition failed: {error}"));
}
}
self.finish_review_navigation();
true
}
fn open_diff(&mut self, mut diff: DiffState) {
if self.local_review_close_blocked() {
return;
}
self.detach_review_buffers();
if let Some(owner) = self.diff_owner.filter(|owner| *owner != self.pane_id) {
self.with_pane(owner, |editor| {
editor.release_diff_buffers();
@@ -4588,6 +4672,11 @@ impl Editor {
.diff_bound
.is_some_and(|bound| bound.3 != self.active.id)
{
if self.current_review_dirty() {
self.bind_diff_buffer();
self.set_message("Save the local review before replacing its comparison");
return;
}
if let Some(role) = self.active.diff_snapshot {
if self
.diff
@@ -5131,6 +5220,16 @@ impl Editor {
| Action::DiffViewToggle
| Action::DiffOrientationToggle
| Action::DiffReviewToggle
| Action::DiffComment
| Action::ReviewEdit
| Action::ReviewDelete
| Action::ReviewNext
| Action::ReviewPrevious
| Action::ReviewOverview
| Action::ReviewSummary
| Action::ReviewVerdict
| Action::ReviewSave
| Action::ReviewOpen
| Action::WordWrapToggle
| Action::GitBlameToggle
| Action::Quit
@@ -7701,6 +7800,9 @@ impl Editor {
Input::Ctrl(ch) => control_key(ch),
input => input,
};
if self.handle_review_overview(&input) {
return None;
}
if self.handle_resolver_history_focus(&input) {
return None;
}
@@ -8178,12 +8280,13 @@ impl Editor {
&& !(matches!(
action,
Action::Save | Action::ForceSave | Action::SaveAndQuit | Action::ForceSaveAndQuit
) && self.resolvers.values().any(|resolver| {
resolver
.files
.iter()
.any(|file| file.draft_buffer == self.active.id)
}))
) && (self.active.review_buffer.is_some()
|| self.resolvers.values().any(|resolver| {
resolver
.files
.iter()
.any(|file| file.draft_buffer == self.active.id)
})))
{
self.set_message("Input pane is read-only");
self.count = None;
@@ -8303,6 +8406,16 @@ impl Editor {
| Action::DiffViewToggle
| Action::DiffOrientationToggle
| Action::DiffReviewToggle
| Action::DiffComment
| Action::ReviewEdit
| Action::ReviewDelete
| Action::ReviewNext
| Action::ReviewPrevious
| Action::ReviewOverview
| Action::ReviewSummary
| Action::ReviewVerdict
| Action::ReviewSave
| Action::ReviewOpen
| Action::WordWrapToggle
| Action::GitBlameToggle
| Action::LspDefinition
@@ -8973,8 +9086,17 @@ impl Editor {
.filter_map(|tab| tab.diff_owner),
)
.collect();
if !force
&& owners
.iter()
.any(|owner| self.with_pane(*owner, |editor| editor.current_review_dirty()))
{
self.set_message("Save the local review before deleting its comparison buffer");
return;
}
for owner in owners {
self.with_pane(owner, |editor| {
editor.detach_review_buffers();
editor.release_diff_buffers();
editor.diff = None;
editor.parked_diff = None;
@@ -9078,6 +9200,7 @@ impl Editor {
fn any_dirty(&self) -> bool {
self.active.dirty()
|| self.parked.values().any(Buffer::dirty)
|| self.local_reviews_dirty()
|| self.resolvers.values().any(|resolver| {
resolver.pending
|| resolver
@@ -9088,6 +9211,10 @@ impl Editor {
}
fn restart(&mut self, force: bool) {
if !force && self.local_reviews_dirty() {
self.set_message("Unsaved local review; use :diff.review.save <path>");
return;
}
if force || !self.any_dirty() {
self.restart = true;
return;
@@ -9184,6 +9311,25 @@ impl Editor {
}
fn execute_action(&mut self, entry: &CoreAction, args: &str) -> Option<Invocation> {
if matches!(
entry.action,
Action::DiffComment
| Action::ReviewEdit
| Action::ReviewDelete
| Action::ReviewNext
| Action::ReviewPrevious
| Action::ReviewOverview
| Action::ReviewSummary
| Action::ReviewVerdict
| Action::ReviewSave
| Action::ReviewOpen
) {
self.finish_diff_prompt();
if let Err(error) = self.local_review_command(entry.action, args) {
self.set_message(error);
}
return None;
}
if matches!(
entry.action,
Action::Resolve
@@ -9210,6 +9356,14 @@ impl Editor {
self.prepare_command();
self.finish_command_mode();
let quit = matches!(entry.action, Action::SaveAndQuit | Action::ForceSaveAndQuit);
if self.active.review_buffer.is_some() {
match self.save_local_review((!args.is_empty()).then(|| Path::new(args))) {
Ok(()) if quit && !self.any_dirty() => self.quit = true,
Ok(()) => {}
Err(error) => self.set_message(error),
}
return None;
}
if self.queue_resolver_save((!args.is_empty()).then(|| Path::new(args)), quit) {
return None;
}
@@ -9418,6 +9572,10 @@ impl Editor {
return None;
}
self.diff.as_ref()?;
if self.local_review_close_blocked() {
return None;
}
self.detach_review_buffers();
self.release_diff_buffers();
self.diff = None;
self.diff_owner = None;
@@ -9695,6 +9853,15 @@ impl Editor {
}
fn save(&mut self, path: Option<PathBuf>, force: bool) -> bool {
if self.active.review_buffer.is_some() {
return match self.save_local_review(path.as_deref()) {
Ok(()) => true,
Err(error) => {
self.set_message(error);
false
}
};
}
if self.queue_resolver_save(path.as_deref(), false) {
return false;
}
@@ -14435,6 +14602,7 @@ mod tests {
comparison_request,
Ok(Outcome::Comparison(Comparison {
root: editor.project_root().to_owned(),
revisions: ["base".into(), "working-snapshot".into()],
before: "base".into(),
after: "working".into(),
editable_paths: Default::default(),
@@ -15623,6 +15791,7 @@ mod tests {
id,
Ok(Outcome::Comparison(Comparison {
root: root.canonicalize().unwrap(),
revisions: ["base".into(), "working-snapshot".into()],
before: "base".into(),
after: "Working".into(),
editable_paths: paths.iter().map(|path| PathBuf::from(*path)).collect(),
@@ -15661,6 +15830,7 @@ mod tests {
id,
Ok(Outcome::Comparison(Comparison {
root: root.to_owned(),
revisions: ["base".into(), "head".into()],
before: "base".into(),
after: "head".into(),
editable_paths: Default::default(),
@@ -15683,6 +15853,189 @@ mod tests {
assert!(editor.diff_interacting());
}
#[test]
fn local_review_native_composition_roundtrip_and_draft_lifecycle() {
use crate::review::{Review, Side, Verdict};
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let path = root.join("feedback.json");
let mut editor = editor();
historical_diff(
&mut editor,
&root,
vec!["before".into(), "removed".into()],
vec!["after".into(), "added".into()],
);
let owner = editor.pane_id();
let source = editor.context().text;
editor.execute("diff.comment");
assert_ne!(editor.pane_id(), owner);
assert!(
editor
.review_panel_title()
.unwrap()
.starts_with("Comment #1")
);
type_text(&mut editor, "iExplain the removed branch");
editor.handle(Input::Escape);
editor.execute("diff.review.summary");
type_text(&mut editor, "iPlease simplify the branch");
editor.handle(Input::Escape);
editor.execute("diff.review.verdict changes_requested");
assert!(editor.context().text.contains("Changes requested"));
editor.execute(&format!("diff.review.save {}", path.display()));
let review = Review::load(&path, &root).unwrap();
assert_eq!(review.comments[0].body, "Explain the removed branch");
assert_eq!(review.comments[0].anchor.side, Side::Before);
assert_eq!(review.comments[0].anchor.content, source);
assert_eq!(review.summary, "Please simplify the branch");
assert_eq!(review.verdict, Some(Verdict::ChangesRequested));
assert!(!editor.local_reviews_dirty());
editor.execute("diff.review.edit 1");
// Native undo applies to comment text, never to the reviewed source.
type_text(&mut editor, "A!");
editor.handle(Input::Escape);
editor.handle(Input::Char('u'));
assert_eq!(editor.context().text, "Explain the removed branch");
editor.execute("w");
editor.execute("diff.close");
assert!(editor.workspace_diff().is_none());
historical_diff(
&mut editor,
&root,
vec!["before".into(), "removed".into()],
vec!["after".into(), "added".into()],
);
editor.execute(&format!("diff.review.open {}", path.display()));
assert!(editor.context().text.contains("Explain the removed branch"));
editor.execute("diff.review.next");
assert_eq!(editor.context().text, "Explain the removed branch");
editor.execute("diff.review.delete");
assert!(editor.context().text.starts_with("# Local review"));
editor.execute("w");
assert!(Review::load(&path, &root).unwrap().comments.is_empty());
assert_eq!(
editor
.workspace_diff()
.unwrap()
.comparison
.as_ref()
.unwrap()
.files[0]
.before,
["before", "removed"]
);
}
#[test]
fn local_review_visual_palette_anchors_both_sides_and_preserves_stale_ranges() {
use crate::review::Side;
for focus in [Focus::Before, Focus::After] {
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let mut editor = editor();
historical_diff(
&mut editor,
&root,
vec!["old first".into(), "old middle".into(), "old last".into()],
vec!["new first".into(), "new middle".into(), "new last".into()],
);
editor.diff.as_mut().unwrap().focus = focus;
editor.bind_diff_buffer();
type_text(&mut editor, "Vj");
editor.handle(Input::Ctrl('p'));
type_text(&mut editor, "diff.comment");
editor.handle(Input::Enter);
let diff = editor.workspace_diff().unwrap();
let anchor = diff.local_review.document.comments[0].anchor.clone();
assert_eq!((anchor.start_line, anchor.end_line), (1, 2));
assert_eq!(
anchor.side,
if focus == Focus::Before {
Side::Before
} else {
Side::After
}
);
assert!(diff.review_anchor_current(&anchor));
let source_pane = editor.diff_owner.unwrap();
editor.select_pane(source_pane);
editor
.diff
.as_mut()
.unwrap()
.comparison
.as_mut()
.unwrap()
.revisions[anchor.side.index()] = "changed-snapshot".into();
editor.execute("diff.review.overview");
assert!(editor.context().text.contains("stale"));
editor.execute("diff.review.edit 1");
assert_eq!(
editor
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.anchor,
anchor
);
}
}
#[test]
fn local_review_guards_metadata_and_hidden_native_drafts_against_loss() {
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let mut editor = editor();
historical_diff(&mut editor, &root, vec!["old".into()], vec!["new".into()]);
editor.execute("diff.comment");
type_text(&mut editor, "iUnsaved feedback");
editor.handle(Input::Escape);
editor.execute("diff.review.overview");
let panel = editor.pane_id();
let source_buffer = editor.diff_return_buffer.unwrap();
editor.delete_buffer(source_buffer, false);
assert!(editor.has_buffer(source_buffer));
assert!(editor.workspace_diff().is_some());
editor.select_pane(editor.diff_owner.unwrap());
fs::write(root.join("other.txt"), "other document").unwrap();
editor.execute(&format!("edit {}", root.join("other.txt").display()));
assert!(editor.workspace_diff().is_some());
assert_eq!(editor.context().text, "old");
editor.select_pane(panel);
editor.execute("diff.close");
assert!(editor.workspace_diff().is_some());
editor.execute("tab.close");
assert!(!editor.should_quit());
editor.execute("restart");
assert!(!editor.should_restart());
editor.execute("q");
assert!(!editor.should_quit());
let invalid = root.join("missing/review.json");
editor.execute(&format!("diff.review.save {}", invalid.display()));
assert!(editor.local_reviews_dirty());
assert_eq!(
editor
.workspace_diff()
.unwrap()
.local_review
.document
.comments[0]
.body,
"Unsaved feedback"
);
let path = root.join("review.json");
editor.execute(&format!("diff.review.save {}", path.display()));
assert!(!editor.local_reviews_dirty());
fs::write(&path, "external change").unwrap();
editor.execute("diff.review.verdict looks_good");
editor.execute("w");
assert_eq!(fs::read_to_string(&path).unwrap(), "external change");
assert!(editor.local_reviews_dirty());
}
#[test]
fn working_refresh_routes_hidden_owners_and_keeps_obsolete_shared_snapshots_readonly() {
let directory = Directory::new();
@@ -15807,6 +16160,7 @@ mod tests {
refresh,
Ok(Outcome::BookmarkRefresh(Comparison {
root: directory.0.clone(),
revisions: ["base".into(), "topic-snapshot".into()],
before: "base".into(),
after: "topic".into(),
editable_paths: Default::default(),
@@ -15927,6 +16281,7 @@ mod tests {
target.loaded = false;
let replacement = crate::diff::Comparison {
root: directory.0.clone(),
revisions: ["base".into(), "ahead-snapshot".into()],
before: "base".into(),
after: "ahead".into(),
editable_paths: Default::default(),
src/editor_review.rs
+606
View File
@@ -0,0 +1,606 @@
use super::*;
use crate::review::{Anchor, Comment, Review, Side, Verdict};
#[derive(Clone, Copy, Eq, PartialEq)]
enum ReviewText {
Comment(u64),
Summary,
Overview,
}
pub(super) struct ReviewBuffer {
owner: usize,
kind: ReviewText,
title: String,
}
impl Editor {
pub fn review_panel_title(&self) -> Option<&str> {
self.active
.review_buffer
.as_ref()
.map(|buffer| buffer.title.as_str())
}
fn review_diff_mut(&mut self) -> Option<&mut DiffState> {
self.diff.as_mut().or(self.parked_diff.as_mut())
}
pub(super) fn local_reviews_dirty(&self) -> bool {
self.workspace_diff()
.is_some_and(|diff| diff.local_review.dirty)
|| self
.tabs
.values()
.any(|tab| tab.diff().is_some_and(|diff| diff.local_review.dirty))
|| std::iter::once(&self.active)
.chain(self.parked.values())
.any(|buffer| buffer.review_buffer.is_some() && buffer.dirty())
}
pub(super) fn current_review_dirty(&self) -> bool {
self.workspace_diff()
.is_some_and(|diff| diff.local_review.dirty)
|| std::iter::once(&self.active)
.chain(self.parked.values())
.any(|buffer| {
buffer
.review_buffer
.as_ref()
.is_some_and(|binding| binding.owner == self.tab_id)
&& buffer.dirty()
})
}
pub(super) fn local_review_close_blocked(&mut self) -> bool {
if self.current_review_dirty() {
self.set_message("Unsaved local review; use :diff.review.save <path> before closing");
true
} else {
false
}
}
pub(super) fn detach_review_buffers(&mut self) {
for buffer in std::iter::once(&mut self.active).chain(self.parked.values_mut()) {
if buffer
.review_buffer
.as_ref()
.is_some_and(|binding| binding.owner == self.tab_id)
{
buffer.review_buffer = None;
}
}
}
fn sync_review_text(&mut self) {
let owner = self.tab_id;
let updates: Vec<_> = std::iter::once(&self.active)
.chain(self.parked.values())
.filter_map(|buffer| {
let binding = buffer.review_buffer.as_ref()?;
(binding.owner == owner && binding.kind != ReviewText::Overview)
.then(|| (binding.kind, buffer.lines.join("\n")))
})
.collect();
let Some(diff) = self.review_diff_mut() else {
return;
};
for (kind, text) in updates {
let destination = match kind {
ReviewText::Comment(id) => diff
.local_review
.document
.comments
.iter_mut()
.find(|comment| comment.id == id)
.map(|comment| &mut comment.body),
ReviewText::Summary => Some(&mut diff.local_review.document.summary),
ReviewText::Overview => None,
};
if let Some(destination) = destination
&& *destination != text
{
*destination = text;
diff.local_review.dirty = true;
}
}
}
pub(super) fn save_local_review(&mut self, path: Option<&Path>) -> Result<(), String> {
self.sync_review_text();
let state = &mut self
.review_diff_mut()
.ok_or("No comparison is open")?
.local_review;
let path = path
.map(Path::to_path_buf)
.or_else(|| state.path.clone())
.ok_or("Use :diff.review.save <path> to choose a local JSON review file")?;
state.document.save(&path)?;
state.path = Some(path);
state.dirty = false;
for buffer in std::iter::once(&mut self.active).chain(self.parked.values_mut()) {
if buffer
.review_buffer
.as_ref()
.is_some_and(|binding| binding.owner == self.tab_id)
{
buffer.saved_lines.clone_from(&buffer.lines);
}
}
self.set_message("Local review saved");
Ok(())
}
pub(super) fn local_review_command(
&mut self,
action: Action,
args: &str,
) -> Result<(), String> {
if self.resolver().is_some() {
return Err("Local reviews require an ordinary diff comparison".into());
}
let diff = self
.workspace_diff()
.filter(|diff| diff.phase == Phase::View)
.ok_or("No comparison is open")?;
let root = diff
.comparison
.as_ref()
.ok_or("Comparison is still loading")?
.root
.clone();
if !matches!(
action,
Action::ReviewEdit
| Action::ReviewDelete
| Action::ReviewVerdict
| Action::ReviewSave
| Action::ReviewOpen
) && !args.is_empty()
{
return Err("This review command takes no arguments".into());
}
if self
.workspace_diff()
.unwrap()
.local_review
.document
.root
.as_os_str()
.is_empty()
{
self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone());
}
if action == Action::ReviewOpen {
if args.is_empty() {
return Err("Usage: diff.review.open <path>".into());
}
if self.current_review_dirty() {
return Err("Save the current local review before opening another".into());
}
let document = Review::load(Path::new(args), &root)?;
self.detach_review_buffers();
let state = &mut self.review_diff_mut().unwrap().local_review;
state.document = document;
state.path = Some(PathBuf::from(args));
state.selected = None;
state.pending_navigation = None;
self.open_review_overview()?;
return Ok(());
}
self.sync_review_text();
match action {
Action::DiffComment => self.add_review_comment()?,
Action::ReviewEdit => {
let id = self.review_comment_id(args)?;
self.show_review_comment(id)?;
}
Action::ReviewDelete => {
let id = self.review_comment_id(args)?;
let state = &mut self.review_diff_mut().unwrap().local_review;
state.document.comments.retain(|comment| comment.id != id);
state.selected = None;
state.pending_navigation = None;
state.dirty = true;
// The deleted draft cannot later overwrite another comment with the same ID.
for buffer in std::iter::once(&mut self.active).chain(self.parked.values_mut()) {
if buffer.review_buffer.as_ref().is_some_and(|binding| {
binding.owner == self.tab_id && binding.kind == ReviewText::Comment(id)
}) {
buffer.review_buffer = None;
buffer.saved_lines.clone_from(&buffer.lines);
}
}
self.open_review_overview()?;
}
Action::ReviewNext | Action::ReviewPrevious => {
let state = &self.workspace_diff().unwrap().local_review;
let comments = &state.document.comments;
if comments.is_empty() {
return Err("No local review comments".into());
}
let current = state
.selected
.and_then(|id| comments.iter().position(|comment| comment.id == id));
let index = match (current, action == Action::ReviewNext) {
(Some(index), true) => (index + 1) % comments.len(),
(Some(index), false) => (index + comments.len() - 1) % comments.len(),
(None, true) => 0,
(None, false) => comments.len() - 1,
};
self.show_review_comment(comments[index].id)?;
}
Action::ReviewOverview => self.open_review_overview()?,
Action::ReviewSummary => {
let text = self
.workspace_diff()
.unwrap()
.local_review
.document
.summary
.clone();
self.open_review_buffer(
ReviewText::Summary,
&text,
"Review summary · :w saves review".into(),
)?;
}
Action::ReviewVerdict => {
let verdict = match args {
"looks_good" => Some(Verdict::LooksGood),
"changes_requested" => Some(Verdict::ChangesRequested),
"none" => None,
_ => {
return Err(
"Usage: diff.review.verdict <looks_good|changes_requested|none>".into(),
);
}
};
let state = &mut self.review_diff_mut().unwrap().local_review;
if state.document.verdict != verdict {
state.document.verdict = verdict;
state.dirty = true;
}
self.open_review_overview()?;
}
Action::ReviewSave => {
self.save_local_review((!args.is_empty()).then(|| Path::new(args)))?
}
_ => unreachable!("local review action"),
}
Ok(())
}
fn review_comment_id(&self, args: &str) -> Result<u64, String> {
let state = &self
.workspace_diff()
.ok_or("No comparison is open")?
.local_review;
let under_cursor = self
.active
.review_buffer
.as_ref()
.filter(|binding| binding.kind == ReviewText::Overview)
.and_then(|_| {
self.active.lines[..=self.cursor().row]
.iter()
.rev()
.find_map(|line| {
line.strip_prefix("## Comment #")?
.split_whitespace()
.next()?
.parse()
.ok()
})
});
let id = if args.is_empty() {
under_cursor
.or(state.selected)
.ok_or("Select a comment or supply its ID")?
} else {
args.parse()
.map_err(|_| "Comment ID must be a positive integer")?
};
state
.document
.comments
.iter()
.any(|comment| comment.id == id)
.then_some(id)
.ok_or_else(|| format!("No local comment #{id}"))
}
fn add_review_comment(&mut self) -> Result<(), String> {
let owner = self.diff_owner.ok_or("No comparison is open")?;
if self.pane_id != owner {
self.select_pane(owner);
}
self.bind_diff_buffer();
let focus = self
.diff_buffer_focus()
.filter(|_| self.diff_interacting())
.ok_or("Focus a diff text pane to add a comment")?;
if self.selection_count() != 1 {
return Err("Use one caret or one line range for a review comment".into());
}
let selection = self.selections().into_iter().next();
let (first, last) = selection.map_or((self.cursor().row, self.cursor().row), |selection| {
let last = selection.end.row.saturating_sub(usize::from(
selection.end.column == 0 && selection.end.row > selection.start.row,
));
(selection.start.row, last)
});
let diff = self.diff.as_ref().unwrap();
let comparison = diff.comparison.as_ref().unwrap();
let file = &comparison.files[diff.file];
let side = if focus == Focus::Before {
Side::Before
} else {
Side::After
};
let anchor = Anchor {
revision: comparison.revisions[side.index()].clone(),
path: if side == Side::Before {
file.old_path.as_ref().unwrap_or(&file.path)
} else {
&file.path
}
.clone(),
side,
start_line: first + 1,
end_line: last + 1,
content: self.active.lines.join("\n"),
};
let id = diff
.local_review
.document
.comments
.iter()
.map(|comment| comment.id)
.max()
.unwrap_or(0)
.checked_add(1)
.ok_or("Review comment IDs exhausted")?;
self.prepare_command();
self.finish_command_mode();
let state = &mut self.diff.as_mut().unwrap().local_review;
state.document.comments.push(Comment {
id,
anchor,
body: String::new(),
});
state.dirty = true;
self.show_review_comment(id)
}
fn show_review_comment(&mut self, id: u64) -> Result<(), String> {
let diff = self.workspace_diff().unwrap();
let comment = diff
.local_review
.document
.comments
.iter()
.find(|comment| comment.id == id)
.ok_or("Comment no longer exists")?;
let text = comment.body.clone();
let anchor = &comment.anchor;
let title = format!(
"Comment #{id} · {}:{}–{} · {:?} · :w saves review",
anchor.path.display(),
anchor.start_line,
anchor.end_line,
anchor.side
);
let target = diff.comparison.as_ref().and_then(|comparison| {
(comparison.revisions[anchor.side.index()] == anchor.revision)
.then(|| {
comparison.files.iter().position(|file| {
let path = if anchor.side == Side::Before {
file.old_path.as_ref().unwrap_or(&file.path)
} else {
&file.path
};
path == &anchor.path
})
})
.flatten()
});
let owner = self.diff_owner.unwrap();
self.select_pane(owner);
let diff = self.diff.as_mut().unwrap();
diff.local_review.selected = Some(id);
if let Some(index) = target {
diff.select_file_index(index);
diff.local_review.pending_navigation = Some(id);
}
self.finish_review_navigation();
self.open_review_buffer(ReviewText::Comment(id), &text, title)?;
if target.is_none() {
self.set_message("Comment is stale; original snapshot anchor preserved");
}
Ok(())
}
pub(super) fn finish_review_navigation(&mut self) {
let Some(diff) = self.diff.as_mut() else {
return;
};
let Some(id) = diff.local_review.pending_navigation else {
return;
};
if !diff.selected_file_loaded() {
return;
}
diff.local_review.pending_navigation = None;
let Some(comment) = diff
.local_review
.document
.comments
.iter()
.find(|comment| comment.id == id)
else {
return;
};
let selected_path = diff
.comparison
.as_ref()
.and_then(|comparison| comparison.files.get(diff.file))
.map(|file| {
if comment.anchor.side == Side::Before {
file.old_path.as_ref().unwrap_or(&file.path)
} else {
&file.path
}
});
if selected_path != Some(&comment.anchor.path) {
return;
}
if !diff.review_anchor_current(&comment.anchor) {
self.set_message("Comment is stale; original snapshot anchor preserved");
return;
}
let row = comment.anchor.start_line - 1;
let focus = if comment.anchor.side == Side::Before {
Focus::Before
} else {
Focus::After
};
diff.focus = focus;
diff.reveal_source_row(focus, row);
self.bind_diff_buffer();
self.place_navigation(TextPosition { row, column: 0 });
}
fn open_review_overview(&mut self) -> Result<(), String> {
let diff = self.workspace_diff().unwrap();
let state = &diff.local_review;
let verdict = match state.document.verdict {
Some(Verdict::LooksGood) => "Looks good",
Some(Verdict::ChangesRequested) => "Changes requested",
None => "Not set",
};
let mut text = format!(
"# Local review\n\nVerdict: {verdict}\nFile: {}\n\n## Summary\n{}\n\nEnter edits the comment under the caret.\n:diff.review.summary · :diff.review.verdict · :diff.review.save\n",
state
.path
.as_ref()
.map_or_else(|| "not saved".into(), |path| path.display().to_string()),
state.document.summary
);
for comment in &state.document.comments {
let anchor = &comment.anchor;
let status = if diff.review_anchor_current(anchor) {
"current"
} else if diff.comparison.as_ref().is_some_and(|comparison| {
comparison.revisions[anchor.side.index()] == anchor.revision
&& comparison.files.iter().any(|file| {
!file.loaded
&& if anchor.side == Side::Before {
file.old_path.as_ref().unwrap_or(&file.path) == &anchor.path
} else {
file.path == anchor.path
}
})
}) {
"not checked (file not loaded)"
} else {
"stale"
};
use std::fmt::Write as _;
write!(
text,
"\n## Comment #{} · {status}\n{}:{}–{} · {:?}\nRevision: {}\n{}\n",
comment.id,
anchor.path.display(),
anchor.start_line,
anchor.end_line,
anchor.side,
anchor.revision,
comment.body
)
.unwrap();
}
self.open_review_buffer(
ReviewText::Overview,
&text,
"Local review overview · Enter edits · :diff.review.next".into(),
)
}
fn open_review_buffer(
&mut self,
kind: ReviewText,
text: &str,
title: String,
) -> Result<(), String> {
self.prepare_command();
self.finish_command_mode();
let existing = std::iter::once(&self.active)
.chain(self.parked.values())
.find(|buffer| {
buffer
.review_buffer
.as_ref()
.is_some_and(|binding| binding.owner == self.tab_id && binding.kind == kind)
})
.map(|buffer| buffer.id);
let panel = self.panes.iter().find_map(|(id, pane)| {
(self.pane_layout.contains(*id)
&& self.buffer(pane.buffer_id).is_some_and(|buffer| {
buffer
.review_buffer
.as_ref()
.is_some_and(|binding| binding.owner == self.tab_id)
}))
.then_some(*id)
});
if let Some(panel) = panel {
self.select_pane(panel);
} else {
self.select_pane(self.diff_owner.unwrap());
self.split_pane(SplitAxis::Vertical);
}
if let Some(id) = existing {
self.switch_buffer(id);
if kind == ReviewText::Overview {
self.active.lines = split_lines(text);
self.active.saved_lines.clone_from(&self.active.lines);
self.active.text_revision = self.active.text_revision.wrapping_add(1);
self.normalize_targets();
}
} else {
let mut buffer = Buffer::from_text(self.next_buffer_id, None, None, text, None)
.map_err(|error| error.to_string())?;
buffer.review_buffer = Some(ReviewBuffer {
owner: self.tab_id,
kind,
title,
});
buffer.resolver_readonly = kind == ReviewText::Overview;
self.add_buffer(buffer);
}
self.set_message("Local review · :w saves JSON; first choose :diff.review.save <path>");
Ok(())
}
pub(super) fn handle_review_overview(&mut self, input: &Input) -> bool {
if *input == Input::Enter
&& self.active.view.mode == Mode::Normal
&& self.palette.is_none()
&& self
.active
.review_buffer
.as_ref()
.is_some_and(|binding| binding.kind == ReviewText::Overview)
{
if let Err(error) = self.local_review_command(Action::ReviewEdit, "") {
self.set_message(error);
}
true
} else {
false
}
}
}
src/lib.rs
+1
View File
@@ -14,6 +14,7 @@ pub mod panes;
pub mod plugin;
pub mod repository;
pub mod resolve;
pub mod review;
pub mod ruby;
pub mod session;
mod snippet;
src/main.rs
+5
View File
@@ -82,6 +82,11 @@ Diff: :diff (Normal); choose Before then After; Working checkout After edits liv
Esc closes Before/After selectors; :diff.close exits an opened comparison explicitly
Remote refs/history load into temporary comparison repos without changing active refs/checkout
Changed files: normal picker/explorer bindings, Ctrl+w close; A/M/D/R status and rename paths
Local reviews: :diff.comment at caret; use palette or Visual binding for a selected line range
: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.save PATH / :diff.review.open PATH; :w in review panes saves local JSON
Snapshot anchors stay immutable; stale comments lose gutter markers; no publishing
Pull requests: :pull_requests [REMOTE]; GitHub/GitLab/Gitea Open/Closed/All tabs, type to search
Enter opens the immutable PR head against its merge base; API auth uses provider tokens or gh/glab
Resolve jj conflicts: :resolve opens the current working-copy conflicts in a dedicated workspace
src/repository.rs
+52
View File
@@ -1058,6 +1058,7 @@ struct PreparedComparison {
id: u64,
selected: [Revision; 2],
selected_ids: [Option<String>; 2],
revisions: [String; 2],
mode: ComparisonMode,
left: Snapshot,
right: Snapshot,
@@ -2284,6 +2285,34 @@ impl Repository {
}
Ok(snapshot)
}
fn snapshot_revision(
&mut self,
target: &RevisionTarget,
snapshot: &Snapshot,
) -> Result<String, String> {
if let RevisionTarget::Commit(id) = target {
return Ok(id.clone());
}
self.ensure_private()?;
let store = &self.private.as_ref().unwrap().1;
let mut input = b"vamp-working-snapshot-v1\0".to_vec();
for (path, file) in snapshot {
self.check_cancelled()?;
for field in [
path.as_os_str().as_encoded_bytes(),
file.mode.as_bytes(),
file.bytes.as_slice(),
file.notice.as_deref().unwrap_or("").as_bytes(),
] {
input.extend_from_slice(&(field.len() as u64).to_be_bytes());
input.extend_from_slice(field);
}
}
let mut command = git_command(store);
command.args(["hash-object", "--no-filters", "--stdin"]);
let id = text(checked(run(command, input, &self.cancelled)?)?)?;
Ok(format!("working:{}", id.trim()))
}
fn snapshot(&mut self, target: &RevisionTarget) -> Result<Snapshot, String> {
if *target == RevisionTarget::Working {
self.working_snapshot(&[])
@@ -2454,10 +2483,17 @@ impl Repository {
let (store, id) = pinned.as_ref().unwrap();
self.commit_snapshot(store, id)?
};
let before_revision = self.snapshot_revision(before_target, &left)?;
let after_revision = if same {
before_revision.clone()
} else {
self.snapshot_revision(after_target, &right)?
};
let mut prepared = PreparedComparison {
id: comparison_id,
selected: [before.clone(), after.clone()],
selected_ids,
revisions: [before_revision, after_revision],
mode,
left,
right,
@@ -2574,9 +2610,11 @@ impl Repository {
return Ok(None);
}
let right = self.working_snapshot(buffers)?;
let revision = self.snapshot_revision(&RevisionTarget::Working, &right)?;
let mut prepared = self.comparison.take().unwrap();
let previous_right = std::mem::replace(&mut prepared.right, right);
let previous_files = std::mem::take(&mut prepared.files);
let previous_revision = std::mem::replace(&mut prepared.revisions[1], revision);
let result = self
.build_comparison(&mut prepared, false)
.and_then(|comparison| {
@@ -2598,6 +2636,7 @@ impl Repository {
} else {
prepared.right = previous_right;
prepared.files = previous_files;
prepared.revisions[1] = previous_revision;
}
self.comparison = Some(prepared);
result.map(|comparison| Some(CheckoutRefresh::Checkout(comparison)))
@@ -2628,11 +2667,14 @@ impl Repository {
let advance_working = working_after
&& follow_working
&& self.working_advanced(prepared.working_anchor.as_deref(), anchor.as_deref())?;
let mut revisions = prepared.revisions.clone();
let (left, before_label) = if branch {
let base = self.merge_base(ids[0].as_deref().unwrap(), ids[1].as_deref().unwrap())?;
let label = format!("merge base of {label} and {after_label} · {base}");
revisions[0] = base.clone();
(Some(self.snapshot(&RevisionTarget::Commit(base))?), label)
} else if changed_before {
revisions[0] = ids[0].clone().unwrap();
(
Some(self.snapshot(&RevisionTarget::Commit(ids[0].clone().unwrap()))?),
label,
@@ -2647,6 +2689,13 @@ impl Repository {
} else {
None
};
if let Some(right) = &right {
revisions[1] = if advance_working {
self.snapshot_revision(&RevisionTarget::Working, right)?
} else {
ids[1].clone().unwrap()
};
}
let pinned_checkout = ids[1]
.as_deref()
.map(|id| self.pinned_checkout(id))
@@ -2658,6 +2707,7 @@ impl Repository {
let previous_files = std::mem::take(&mut prepared.files);
let previous_before = std::mem::replace(&mut prepared.before, before_label);
let previous_editable = prepared.editable_after;
let previous_revisions = std::mem::replace(&mut prepared.revisions, revisions);
prepared.editable_after =
working_after || (pinned_checkout.is_some() && pinned_checkout == anchor);
let result = self
@@ -2693,6 +2743,7 @@ impl Repository {
prepared.files = previous_files;
prepared.before = previous_before;
prepared.editable_after = previous_editable;
prepared.revisions = previous_revisions;
}
self.comparison = Some(prepared);
result
@@ -2810,6 +2861,7 @@ impl Repository {
BTreeSet::new()
};
Comparison {
revisions: prepared.revisions.clone(),
before: prepared.before.clone(),
after: prepared.after.clone(),
files,
src/review.rs
+395
View File
@@ -0,0 +1,395 @@
use serde::{Deserialize, Serialize};
use std::{
cell::RefCell,
collections::BTreeSet,
fs::{self, OpenOptions},
io::{Read, Write},
path::{Path, PathBuf},
sync::{
Arc,
atomic::{AtomicU64, Ordering},
},
};
const LIMIT: usize = 64 * 1024 * 1024;
static NEXT_TEMP: AtomicU64 = AtomicU64::new(0);
#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
pub enum Side {
Before,
After,
}
impl Side {
pub fn index(self) -> usize {
match self {
Self::Before => 0,
Self::After => 1,
}
}
}
#[derive(Clone, Copy, Debug, Eq, PartialEq, Serialize, Deserialize)]
#[serde(rename_all = "snake_case")]
pub enum Verdict {
LooksGood,
ChangesRequested,
}
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct Anchor {
/// Frozen comparison context; `content` additionally captures any live buffer edits.
pub revision: String,
pub path: PathBuf,
pub side: Side,
pub start_line: usize,
pub end_line: usize,
/// Exact whole-side text, including its final newline when present.
// ponytail: whole-file text per comment; deduplicate snapshots if the 64 MiB ceiling matters.
pub content: String,
}
impl Anchor {
pub fn matches(&self, revision: &str, path: &Path, side: Side, text: &str) -> bool {
self.revision == revision && self.path == path && self.side == side && self.content == text
}
pub fn matches_lines(
&self,
revision: &str,
path: &Path,
side: Side,
lines: &[String],
eof: bool,
) -> bool {
self.revision == revision
&& self.path == path
&& self.side == side
&& self.content.split('\n').eq(lines
.iter()
.map(String::as_str)
.chain((eof || lines.is_empty()).then_some("")))
}
}
#[derive(Clone, Debug, Eq, PartialEq, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct Comment {
pub id: u64,
pub anchor: Anchor,
pub body: String,
}
#[derive(Clone, Debug, Serialize, Deserialize)]
#[serde(deny_unknown_fields)]
pub struct Review {
pub version: u32,
pub root: PathBuf,
pub summary: String,
pub verdict: Option<Verdict>,
pub comments: Vec<Comment>,
#[serde(skip)]
saved: RefCell<Option<(PathBuf, Arc<Vec<u8>>)>>,
}
impl Default for Review {
fn default() -> Self {
Self::new(PathBuf::new())
}
}
#[derive(Clone, Debug, Default)]
pub struct ReviewState {
pub document: Review,
pub path: Option<PathBuf>,
pub selected: Option<u64>,
pub pending_navigation: Option<u64>,
pub dirty: bool,
}
impl Review {
pub fn new(root: PathBuf) -> Self {
let root = if root.as_os_str().is_empty() {
root
} else {
fs::canonicalize(&root).unwrap_or(root)
};
Self {
version: 1,
root,
summary: String::new(),
verdict: None,
comments: Vec::new(),
saved: RefCell::new(None),
}
}
fn validate(&self) -> Result<(), String> {
if self.version != 1 {
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());
}
let mut ids = BTreeSet::new();
for comment in &self.comments {
if comment.id == 0 || !ids.insert(comment.id) {
return Err("Review comment IDs must be unique and nonzero".into());
}
let anchor = &comment.anchor;
crate::repository::safe_path(&anchor.path)?;
if anchor.path.to_str().is_none_or(|path| path.contains('\0')) {
return Err("Review paths must be UTF-8 without NUL bytes".into());
}
if anchor.revision.is_empty() || anchor.revision.chars().any(char::is_control) {
return Err("Review anchors require an immutable revision identity".into());
}
if anchor.start_line == 0
|| anchor.end_line < anchor.start_line
|| anchor.end_line > anchor.content.split('\n').count()
{
return Err(
"Review ranges must be inclusive, nonzero and within the anchor text".into(),
);
}
}
Ok(())
}
pub fn load(path: &Path, root: &Path) -> Result<Self, String> {
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}"))?;
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())?;
if actual != expected {
return Err("Review belongs to a different repository".into());
}
review.root = actual;
*review.saved.borrow_mut() = Some((identity(path)?, Arc::new(bytes)));
Ok(review)
}
pub fn save(&self, path: &Path) -> Result<(), String> {
self.validate()?;
let bytes = serde_json::to_vec_pretty(self).map_err(|error| error.to_string())?;
if bytes.len() > LIMIT {
return Err("Review exceeds the 64 MiB limit".into());
}
let path = identity(path)?;
let saved = self.saved.borrow();
let expected = saved
.as_ref()
.filter(|(previous, _)| previous == &path)
.map(|(_, bytes)| bytes.as_slice());
check_file(&path, expected)?;
let parent = path.parent().ok_or("Review path has no parent")?;
let (temporary, mut file) = loop {
let id = NEXT_TEMP.fetch_add(1, Ordering::Relaxed);
let temporary = parent.join(format!(".vamp-review-{}-{id}.tmp", std::process::id()));
let mut options = OpenOptions::new();
options.write(true).create_new(true);
#[cfg(unix)]
{
use std::os::unix::fs::OpenOptionsExt;
options.mode(0o600);
}
match options.open(&temporary) {
Ok(file) => break (temporary, file),
Err(error) if error.kind() == std::io::ErrorKind::AlreadyExists => continue,
Err(error) => return Err(error.to_string()),
}
};
let result: Result<(), String> = (|| {
file.write_all(&bytes)
.and_then(|_| file.sync_all())
.map_err(|error| error.to_string())?;
drop(file);
check_file(&path, expected)?;
if expected.is_some() {
// ponytail: optimistic check like editor saves; OS compare-and-swap for hostile writers.
fs::rename(&temporary, &path).map_err(|error| error.to_string())?;
} else {
// Publishing a new file must not replace a concurrently created target.
fs::hard_link(&temporary, &path).map_err(|error| error.to_string())?;
}
Ok(())
})();
let _ = fs::remove_file(&temporary);
result?;
drop(saved);
*self.saved.borrow_mut() = Some((path, Arc::new(bytes)));
Ok(())
}
}
fn identity(path: &Path) -> Result<PathBuf, String> {
// Resolve the parent only: never silently follow a target symlink.
let parent = path
.parent()
.filter(|path| !path.as_os_str().is_empty())
.unwrap_or(Path::new("."));
let name = path.file_name().ok_or("Review path must name a file")?;
Ok(fs::canonicalize(parent)
.map_err(|error| error.to_string())?
.join(name))
}
fn read_file(path: &Path) -> Result<Option<Vec<u8>>, String> {
let metadata = match fs::symlink_metadata(path) {
Ok(metadata) => metadata,
Err(error) if error.kind() == std::io::ErrorKind::NotFound => return Ok(None),
Err(error) => return Err(error.to_string()),
};
if !metadata.is_file() || metadata.file_type().is_symlink() {
return Err("Review file must be a regular file, not a symbolic link".into());
}
if metadata.len() > LIMIT as u64 {
return Err("Review exceeds the 64 MiB limit".into());
}
let mut bytes = Vec::new();
fs::File::open(path)
.and_then(|file| file.take(LIMIT as u64 + 1).read_to_end(&mut bytes))
.map_err(|error| error.to_string())?;
if bytes.len() > LIMIT {
return Err("Review exceeds the 64 MiB limit".into());
}
Ok(Some(bytes))
}
fn check_file(path: &Path, expected: Option<&[u8]>) -> Result<(), String> {
let actual = read_file(path)?;
if actual.as_deref() != expected {
return Err(
"Review file changed outside vamp or already exists; open it before saving".into(),
);
}
Ok(())
}
#[cfg(test)]
mod tests {
use super::*;
struct Directory(PathBuf);
impl Drop for Directory {
fn drop(&mut self) {
let _ = fs::remove_dir_all(&self.0);
}
}
#[test]
fn persistence_validates_anchors_and_preserves_files_on_failure() {
let directory = Directory(std::env::temp_dir().join(format!(
"vamp-review-test-{}-{}",
std::process::id(),
NEXT_TEMP.fetch_add(1, Ordering::Relaxed),
)));
fs::create_dir(&directory.0).unwrap();
let root = directory.0.canonicalize().unwrap();
let path = root.join("review.json");
let mut review = Review::new(root.clone());
review.summary = "Local review".into();
review.verdict = Some(Verdict::ChangesRequested);
review.comments.push(Comment {
id: 1,
anchor: Anchor {
revision: "working:immutable".into(),
path: "source.rs".into(),
side: Side::After,
start_line: 1,
end_line: 2,
content: "first\nlast\n".into(),
},
body: "Please adjust this".into(),
});
review.save(&path).unwrap();
let mut loaded = Review::load(&path, &root).unwrap();
assert_eq!(loaded.comments, review.comments);
assert_eq!(loaded.verdict, review.verdict);
let anchor = &loaded.comments[0].anchor;
assert!(anchor.matches(
"working:immutable",
Path::new("source.rs"),
Side::After,
"first\nlast\n"
));
assert!(!anchor.matches(
"working:other",
Path::new("source.rs"),
Side::After,
"first\nlast\n"
));
assert!(anchor.matches_lines(
"working:immutable",
Path::new("source.rs"),
Side::After,
&["first".into(), "last".into()],
true
));
assert!(!anchor.matches_lines(
"working:immutable",
Path::new("source.rs"),
Side::After,
&["first".into(), "last".into()],
false
));
loaded.summary = "Updated".into();
loaded.save(&path).unwrap();
assert_eq!(Review::load(&path, &root).unwrap().summary, "Updated");
assert!(Review::new(root.clone()).save(&path).is_err());
let valid = fs::read(&path).unwrap();
for (field, value) in [
("version", serde_json::json!(2)),
("root", serde_json::json!(root.join("other"))),
(
"comments",
serde_json::json!([loaded.comments[0], loaded.comments[0]]),
),
] {
let mut invalid = serde_json::to_value(&loaded).unwrap();
invalid[field] = value;
fs::write(&path, serde_json::to_vec(&invalid).unwrap()).unwrap();
assert!(Review::load(&path, &root).is_err());
}
for (field, value) in [
("path", serde_json::json!("../outside")),
("path", serde_json::json!("/absolute")),
("path", serde_json::json!(".git/config")),
("start_line", serde_json::json!(0)),
("start_line", serde_json::json!(3)),
("end_line", serde_json::json!(4)),
("revision", serde_json::json!("")),
] {
let mut invalid = serde_json::to_value(&loaded).unwrap();
invalid["comments"][0]["anchor"][field] = value;
fs::write(&path, serde_json::to_vec(&invalid).unwrap()).unwrap();
assert!(Review::load(&path, &root).is_err());
}
fs::create_dir(root.join("other")).unwrap();
fs::write(&path, &valid).unwrap();
assert!(Review::load(&path, &root.join("other")).is_err());
fs::write(&path, b"external edit").unwrap();
assert!(loaded.save(&path).is_err());
assert_eq!(fs::read(&path).unwrap(), b"external edit");
assert_eq!(loaded.summary, "Updated");
let invalid = root.join("invalid.json");
loaded.comments[0].anchor.start_line = 0;
assert!(loaded.save(&invalid).is_err());
assert!(!invalid.exists());
let large = root.join("large.json");
fs::File::create(&large)
.unwrap()
.set_len(LIMIT as u64 + 1)
.unwrap();
assert!(Review::load(&large, &root).is_err());
#[cfg(unix)]
{
let link = root.join("link.json");
std::os::unix::fs::symlink(&path, &link).unwrap();
assert!(Review::load(&link, &root).is_err());
assert!(review.save(&link).is_err());
assert_eq!(fs::read(&path).unwrap(), b"external edit");
}
}
}
src/session.rs
+2
View File
@@ -2226,6 +2226,7 @@ mod tests {
session.editor.apply_diff_result(
id,
Ok(DiffOutcome::Comparison(Comparison {
revisions: ["base".into(), "after".into()],
root: fixture.0.clone(),
before: "base".into(),
after: "after".into(),
@@ -5343,6 +5344,7 @@ end
session.editor.apply_diff_result(
id,
Ok(DiffOutcome::Comparison(Comparison {
revisions: ["base".into(), "working:snapshot".into()],
root: fixture.0.canonicalize().unwrap(),
before: "base".into(),
after: "Working".into(),
src/tui.rs
+255
View File
@@ -737,6 +737,9 @@ impl BufferViewports {
.retain(|id, _| view.has_buffer(*id));
let (top, left) = self.offsets.entry(view.buffer_id()).or_default();
let syntax = self.syntax.entry(view.buffer_id()).or_default();
let leaf = view.review_panel_title().map_or(leaf, |title| {
resolver_document_panel(frame, leaf, title, id == focused)
});
draw_document(
frame,
view,
@@ -4324,6 +4327,22 @@ fn draw_diff_pane(
*left = diff.bound_horizontal(focus, *left, width);
}
let left = *left;
let review_comments = diff
.local_review
.document
.comments
.iter()
.filter(|comment| {
let before = comment.anchor.side == crate::review::Side::Before;
let path = if before {
file.old_path.as_ref().unwrap_or(&file.path)
} else {
&file.path
};
(unified || before == (focus == Focus::Before))
&& comment.anchor.path == *path
&& diff.review_anchor_current(&comment.anchor)
});
for (screen, display_row, segments) in diff_screen_rows(diff, top, content.height, wrapped) {
let row = &diff.display_rows[display_row];
let row_area = Rect::new(content.x, content.y + screen as u16, content.width, 1);
@@ -4495,6 +4514,46 @@ fn draw_diff_pane(
paint_trailing_whitespace(frame, &text, text_area, left);
}
}
if gutter > 0 {
for comment in review_comments {
let side = match comment.anchor.side {
crate::review::Side::Before => Focus::Before,
crate::review::Side::After => Focus::After,
};
let selected = diff.local_review.selected == Some(comment.id);
for (screen, display, _) in diff_screen_rows(diff, top, content.height, wrapped) {
let source = diff.display_source_row(side, display).or_else(|| {
let DisplayRow::Line(index) = diff.display_rows[display] else {
return None;
};
let row = &diff.rows[index];
(unified && row.kind == RowKind::Equal)
.then_some(match side {
Focus::Before => row.before,
Focus::After => row.after,
Focus::Files => None,
})
.flatten()
});
if wrapped.is_some_and(|rows| rows[screen].position.continuation != 0)
|| !source.is_some_and(|row| {
(comment.anchor.start_line..=comment.anchor.end_line).contains(&(row + 1))
})
{
continue;
}
let cell = &mut frame.buffer_mut()
[(content.x + gutter as u16 - 1, content.y + screen as u16)];
if selected || cell.symbol() != "●" {
cell.set_symbol(if selected { "●" } else { "○" })
.set_fg(Color::Magenta);
if selected {
cell.modifier.insert(Modifier::BOLD);
}
}
}
}
}
Rect::new(
content.x + gutter as u16,
content.y,
@@ -8872,6 +8931,7 @@ mod tests {
editable_paths: Default::default(),
before: "old revision".into(),
after: "Working checkout".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files,
})),
);
@@ -9234,6 +9294,193 @@ mod tests {
.unwrap();
}
fn commented_diff() -> DiffState {
use crate::review::{Anchor, Comment, Side};
let before = format!("{}\nold second\nkeep", "old ".repeat(24));
let after = format!("{}\nnew second\nkeep", "new ".repeat(24));
let mut file = syntax_diff_file("new.txt", &before, &after);
file.hunks =
crate::diff::line_hunks(&file.before, file.before_eof, &file.after, file.after_eof);
file.old_path = Some("old.txt".into());
let mut diff = diff_comparison(vec![file]);
diff.files_visible = false;
diff.local_review.document.root = diff.comparison.as_ref().unwrap().root.clone();
diff.local_review.document.comments = vec![
Comment {
id: 1,
anchor: Anchor {
revision: "before-snapshot".into(),
path: "old.txt".into(),
side: Side::Before,
start_line: 1,
end_line: 2,
content: before,
},
body: "Before range".into(),
},
Comment {
id: 2,
anchor: Anchor {
revision: "after-snapshot".into(),
path: "new.txt".into(),
side: Side::After,
start_line: 1,
end_line: 1,
content: after,
},
body: "After line".into(),
},
];
diff.local_review.selected = Some(1);
diff
}
#[test]
fn local_review_markers_preserve_diff_signs_ranges_and_wrapped_geometry() {
for mode in [ViewMode::Split, ViewMode::Unified] {
for wrapping in [false, true] {
let mut diff = commented_diff();
diff.set_view_mode(mode);
diff.top = 0;
diff.word_wrap = wrapping;
let mut terminal =
Terminal::new(ratatui::backend::TestBackend::new(90, 40)).unwrap();
let mut panels = WheelPanels::default();
render_diff(&mut terminal, &diff, &mut panels);
let buffer = terminal.backend().buffer();
let markers: Vec<_> = buffer
.content
.iter()
.enumerate()
.filter(|(_, cell)| matches!(cell.symbol(), "●" | "○"))
.collect();
assert_eq!(markers.len(), 3, "{mode:?} {wrapping}");
assert_eq!(
markers
.iter()
.filter(|(_, cell)| cell.symbol() == "●")
.count(),
2
);
for &(offset, cell) in &markers {
assert_eq!(cell.fg, Color::Magenta);
assert_eq!(cell.modifier.contains(Modifier::BOLD), cell.symbol() == "●");
assert_eq!(
buffer.content[offset - 1].symbol(),
if cell.symbol() == "●" { "-" } else { "+" }
);
}
if wrapping {
let rows = &panels.diff_wrap.as_ref().unwrap().rows;
assert!(rows.iter().any(|row| row.position.continuation > 0));
for (area, _) in panels.diff.as_ref().unwrap().regions.into_iter().skip(1) {
let area = diff_inner(area);
for (screen, row) in rows.iter().take(area.height as usize).enumerate() {
if row.position.continuation > 0 {
for x in area.x..area.right() {
assert!(!matches!(
buffer[(x, area.y + screen as u16)].symbol(),
"●" | "○"
));
}
}
}
}
}
}
}
}
#[test]
fn local_review_markers_reject_stale_identity_text_and_rename_paths() {
for mode in [ViewMode::Split, ViewMode::Unified] {
for mismatch in ["revision", "content", "path", "side", "root"] {
let mut diff = commented_diff();
diff.set_view_mode(mode);
diff.local_review.document.comments.truncate(1);
let anchor = &mut diff.local_review.document.comments[0].anchor;
match mismatch {
"revision" => anchor.revision.push_str("-newer"),
"content" => anchor.content.push('\n'),
"path" => anchor.path = "new.txt".into(),
"side" => anchor.side = crate::review::Side::After,
"root" => diff.local_review.document.root = "another-repository".into(),
_ => unreachable!(),
}
let mut terminal =
Terminal::new(ratatui::backend::TestBackend::new(90, 20)).unwrap();
render_diff(&mut terminal, &diff, &mut WheelPanels::default());
assert!(
terminal
.backend()
.buffer()
.content
.iter()
.all(|cell| !matches!(cell.symbol(), "●" | "○")),
"{mode:?} {mismatch}"
);
}
}
}
#[test]
fn unified_context_shows_comments_from_either_snapshot_side() {
for side in [crate::review::Side::Before, crate::review::Side::After] {
let mut diff = commented_diff();
diff.set_view_mode(ViewMode::Unified);
diff.local_review
.document
.comments
.retain(|comment| comment.anchor.side == side);
let comment = &mut diff.local_review.document.comments[0];
comment.anchor.start_line = 3;
comment.anchor.end_line = 3;
diff.local_review.selected = Some(comment.id);
let mut terminal = Terminal::new(ratatui::backend::TestBackend::new(90, 20)).unwrap();
render_diff(&mut terminal, &diff, &mut WheelPanels::default());
assert!(
screen_text(&terminal).contains("●keep"),
"{side:?}: {}",
screen_text(&terminal)
);
}
}
#[test]
fn local_review_markers_yield_to_text_on_narrow_surfaces() {
for mode in [ViewMode::Split, ViewMode::Unified] {
for (width, height) in [(1, 1), (2, 2), (3, 3), (8, 4), (16, 8), (30, 12)] {
let mut diff = commented_diff();
diff.set_view_mode(mode);
diff.word_wrap = true;
let mut terminal =
Terminal::new(ratatui::backend::TestBackend::new(width, height)).unwrap();
let mut panels = WheelPanels::default();
render_diff(&mut terminal, &diff, &mut panels);
for (area, focus) in panels
.diff
.as_ref()
.into_iter()
.flat_map(|geometry| geometry.regions.into_iter().skip(1))
{
assert!(area.right() <= width && area.bottom() <= height);
let content = diff_inner(area);
let file = &diff.comparison.as_ref().unwrap().files[0];
let gutter = diff_gutter(&diff, file, content.width as usize, focus);
for y in content.y..content.bottom() {
for x in content.x..content.right() {
if matches!(terminal.backend().buffer()[(x, y)].symbol(), "●" | "○")
{
assert!(gutter > 0);
assert_eq!(x, content.x + gutter as u16 - 1);
}
}
}
}
}
}
}
const LIGHT_DIFF_THEME: ThemeColors = ThemeColors {
background: [250, 245, 240],
red: [210, 30, 50],
@@ -10658,6 +10905,7 @@ mod tests {
editable_paths: Default::default(),
before: "old revision".into(),
after: "Working checkout".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![deletion_file()],
})),
);
@@ -11480,6 +11728,7 @@ end
editable_paths: Default::default(),
before: "baseline".into(),
after: "historic".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![file],
})),
);
@@ -11573,6 +11822,7 @@ end
editable_paths: ["live.rs".into()].into(),
before: "baseline".into(),
after: "Working checkout".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![syntax_diff_file("live.rs", "old", "new")],
})),
);
@@ -12208,6 +12458,7 @@ end
editable_paths: Default::default(),
before: "baseline".into(),
after: "Working checkout".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![deletion_file()],
})),
);
@@ -13617,6 +13868,7 @@ end
editable_paths: Default::default(),
before: "baseline".into(),
after: "historic".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![source, unloaded],
})),
);
@@ -13714,6 +13966,7 @@ end
editable_paths: Default::default(),
before: "baseline".into(),
after: "historic".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![file],
})),
);
@@ -14336,6 +14589,7 @@ end
},
before: "baseline".into(),
after: "Working checkout".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![syntax_diff_file(
"live.rs",
"let value = 1;",
@@ -14462,6 +14716,7 @@ end
editable_paths: [std::path::PathBuf::from("live.rs")].into(),
before: "baseline".into(),
after: "Working checkout".into(),
revisions: ["before-snapshot".into(), "after-snapshot".into()],
files: vec![syntax_diff_file(
"live.rs",
"let value = \"old\";",