fix: key review checkmarks by file snapshots instead of revision pairs

This commit is contained in:
Valdemar Lorenzen
2026-10-20 11:09:48 +02:00
parent ea04e30072
commit 8f8d96ead9
9 changed files with 164 additions and 12 deletions
+3
View File
@@ -1,3 +1,6 @@
- [x] improve: checkmark persistance should be preserved in a diff where a certain file hasn't changed.
- this is to avoid that changing a single file will cause all reviewed files to drop the reviewed state, making it harder to spot what actually requires a re-review and what can be ignored.
- [ ] bug: diff view - git blame doesn't show in diff buffers for some reason.
- [ ] feature: diff view - GitHub code review integration after the local review UX is usable.
- publish local reviews explicitly: general PR comments, inline line/range comments, review summaries, approval, and requests for changes.
- preserve explicit PR identity and reviewed commit IDs; validate anchors against GitHub's PR diff and recheck the PR head before submission.
+1
View File
@@ -77,6 +77,7 @@ Live PTY/CLI smoke composes and explicitly saves a comment, resolves it in the e
CLI deletion checks exercise explicit review/comment IDs, resolved-comment deletion, repository isolation, preserved summaries/verdicts/other comments, last-comment deletion, missing targets/storage, and stale-save conflicts through the executable. CLI smoke verifies persisted deletion and rejects a repeated deletion without advancing the review revision.
Relational-storage smoke inspects typed review/comment columns, round-trips exact Unicode anchors through the store and JSON export/import, deletes through the real CLI, and confirms the stored review and existing export remain intact.
Reviewed-file checks cover SQLite/JSON round-trips, unmarking, exact revision pairs and rename paths across catalog reordering, lazy loading, live text/final-newline mismatch, edit-triggered invalidation, selected-review isolation, hidden-draft isolation, conflicts, and transactional rollback. Live PTY smoke marks a file in a real Git comparison, verifies the SQLite row, restarts the editor, observes the restored checkmark, unmarks it, and verifies row removal with unchanged source bytes.
Per-file persistence also exercises a real Git Working comparison: mark two lazy files, save/reload the review, edit one file, refresh the snapshot, and retain only the unchanged file's mark through detail loading.
Conflicted-revision regressions preserve unchanged symlinks, defer oversized unselected blobs until file selection, compare selected conflict text with jj's projection, and exclude unchanged conflicts when comparing later revisions. Live terminal smoke covers local bookmark selection and opening an actual conflicted file without changing the checkout.
+2 -2
View File
@@ -462,7 +462,7 @@ Mark or unmark a diff file with **`c` while browsing the changed-file tree**, **
Collapsed folders show a green **`✓`** when every changed file beneath them is reviewed, including nested files and descendants hidden by filtering. Expanded folders stay unmarked. Folder marks update automatically when a descendant's review mark or the changed-file catalog changes.
Review marks are saved with the selected local review and restored when the same immutable Before/After revision pair and file paths are reopened. They survive filtering, file navigation, lazy loading, tabs, split/unified/orientation changes, and pinned checkout-only transitions. Other comparisons do not inherit their marks; returning to a previously reviewed comparison restores them. Editing a writable After file clears only that file's mark and saves the removal; unchanged refreshes and saves preserve it. Marks on live text also retain its exact contents, including the final newline, so different unsaved text cannot inherit a checkmark. Marks do not change document text, undo history, or repository state. Without configured SQLite storage, use `:diff.review.save path` to save them as JSON.
Review marks are saved with the selected local review and restored by immutable per-file Before/After identities and file paths. Changes to unrelated files or commit metadata do not clear them; changes to either side's bytes, final newline, file mode, or rename paths require re-review. They survive filtering, file navigation, lazy loading, tabs, split/unified/orientation changes, and pinned checkout-only transitions. Editing a writable After file clears only that file's mark and saves the removal; unchanged refreshes and saves preserve it. Marks on live text also retain its exact contents, including the final newline, so different unsaved text cannot inherit a checkmark. Marks do not change document text, undo history, or repository state. Without configured SQLite storage, use `:diff.review.save path` to save them as JSON. Older revision-pair-scoped marks remain stored but must be marked again to acquire per-file identities; comments, summaries, and verdicts are unaffected.
**Local review comments** share local storage with the file-reviewed checkmarks above; neither feature publishes to GitHub. Focus either diff text pane and run **`:diff.comment`** to comment on the current line. A characterwise Visual selection (`v`) anchors its exact text, including partial lines and multiline spans; Visual Line (`V`) anchors whole selected lines. Type **`:diff.comment`** and press Enter, or use the palette/a configured binding. Multiple independent carets/ranges must be reviewed separately.
@@ -495,7 +495,7 @@ Write errors, locked databases, incompatible schemas, and conflicting edits from
The current SQLite format is schema 3: typed `reviews`, `comments`, and `reviewed_files` tables, not serialized JSON documents. Older databases are incompatible and are left untouched; there is no migration or automatic reset. JSON version 4 is the sharing format, independent of the database schema.
Version 4 JSON is the agent-readable sharing format: repository root, summary, optional verdict, comments, and a required `reviewed_files` array. Each reviewed file stores its immutable `revisions` pair, relative `path`, optional rename `old_path`, and optional exact live `after` text. Comments have 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 JSON formats are rejected and preserved without migration. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Stored reviews have a 64 MiB aggregate text budget including 64 bytes of overhead per comment and reviewed file; imported/exported JSON files have a separate 64 MiB serialized-size limit.
Version 4 JSON is the agent-readable sharing format: repository root, summary, optional verdict, comments, and a required `reviewed_files` array. Each reviewed file stores its immutable per-file `revisions` pair (`file:` identities encoding mode, Git object ID or exact data, and notices), relative `path`, optional rename `old_path`, and optional exact live `after` text. Comments have 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 JSON formats are rejected and preserved without migration. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. Stored reviews have a 64 MiB aggregate text budget including 64 bytes of overhead per comment and reviewed file; imported/exported JSON files have a separate 64 MiB serialized-size limit.
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.
+26 -6
View File
@@ -127,6 +127,8 @@ pub struct Hunk {
}
#[derive(Debug)]
pub struct ChangedFile {
/// Immutable file-side identities, independent of unrelated revision changes.
pub review_revisions: Option<[String; 2]>,
pub path: PathBuf,
pub old_path: Option<PathBuf>,
pub status: FileStatus,
@@ -676,6 +678,7 @@ impl DiffState {
});
comparison.root = root.clone();
comparison.files.resize_with(paths.len(), || ChangedFile {
review_revisions: None,
path: PathBuf::new(),
old_path: None,
status: FileStatus::Modified,
@@ -1801,11 +1804,20 @@ impl DiffState {
}
let records = &mut self.local_review.document.reviewed_files;
records.retain(|record| {
record.revisions != comparison.revisions || record.path != file.path
record.revisions
!= *file
.review_revisions
.as_ref()
.unwrap_or(&comparison.revisions)
|| record.path != file.path
});
if reviewed {
records.push(crate::review::ReviewedFile {
revisions: comparison.revisions.clone(),
revisions: file
.review_revisions
.as_ref()
.unwrap_or(&comparison.revisions)
.clone(),
path: file.path.clone(),
old_path: file.old_path.clone(),
after: self
@@ -1832,12 +1844,16 @@ impl DiffState {
.document
.reviewed_files
.iter()
.filter(|record| record.revisions == comparison.revisions)
.map(|record| (&record.path, record))
.map(|record| ((&record.revisions, &record.path), record))
.collect();
for (index, file) in comparison.files.iter().enumerate() {
let Some(record) = records
.get(&file.path)
.get(&(
file.review_revisions
.as_ref()
.unwrap_or(&comparison.revisions),
&file.path,
))
.filter(|record| record.old_path == file.old_path)
else {
continue;
@@ -2263,7 +2279,11 @@ impl DiffState {
.reviewed_files
.iter()
.position(|record| {
record.revisions == comparison.revisions
record.revisions
== *file
.review_revisions
.as_ref()
.unwrap_or(&comparison.revisions)
&& record.path == file.path
&& record.old_path == file.old_path
})
+15
View File
@@ -2770,6 +2770,7 @@ impl Repository {
continue;
}
files.push(ChangedFile {
review_revisions: None,
path: path.clone(),
old_path: None,
status: if a.is_none() {
@@ -2827,6 +2828,10 @@ impl Repository {
.files
.iter()
.map(|(path, old_path, status)| ChangedFile {
review_revisions: Some([
review_file_revision(prepared.left.get(old_path.as_ref().unwrap_or(path))),
review_file_revision(prepared.right.get(path)),
]),
path: path.clone(),
old_path: old_path.clone(),
status: *status,
@@ -2902,6 +2907,7 @@ impl Repository {
)?;
file.status = *status;
file.old_path = old_path.clone();
file.review_revisions = Some([review_file_revision(before), review_file_revision(after)]);
Ok(file)
}
fn changed_file(
@@ -2972,6 +2978,7 @@ impl Repository {
};
notices.dedup();
Ok(ChangedFile {
review_revisions: None,
path: path.to_path_buf(),
old_path: None,
status,
@@ -2990,6 +2997,14 @@ impl Repository {
}
}
fn review_file_revision(file: Option<&FileData>) -> String {
// Git object IDs cover exact bytes (including binary data and EOF), without loading blobs.
format!(
"file:{}",
serde_json::to_string(&file.map(file_identity)).unwrap()
)
}
fn file_identity(file: &FileData) -> (&str, Option<&str>, &[u8], Option<&str>) {
(
&file.mode,
+100 -4
View File
@@ -787,6 +787,7 @@ fn comparison() -> Comparison {
editable_paths: BTreeSet::new(),
files: vec![
ChangedFile {
review_revisions: None,
path: "renamed.txt".into(),
old_path: Some("old.txt".into()),
status: FileStatus::Renamed,
@@ -824,6 +825,7 @@ fn comparison() -> Comparison {
notice: None,
},
ChangedFile {
review_revisions: None,
path: "added.txt".into(),
old_path: None,
status: FileStatus::Added,
@@ -841,6 +843,7 @@ fn comparison() -> Comparison {
notice: None,
},
ChangedFile {
review_revisions: None,
path: "removed.bin".into(),
old_path: None,
status: FileStatus::Removed,
@@ -1561,6 +1564,7 @@ fn closing_diff_rejects_pending_file_details_even_after_reopening() {
let detail = catalog.files.remove(0);
catalog.files.clear();
catalog.files.push(ChangedFile {
review_revisions: None,
path: detail.path.clone(),
old_path: detail.old_path.clone(),
status: detail.status,
@@ -1808,6 +1812,7 @@ fn live_state(before: Vec<String>, after: Vec<String>, eof: bool) -> DiffState {
after: "Working".into(),
editable_paths: BTreeSet::from([PathBuf::from("file")]),
files: vec![ChangedFile {
review_revisions: None,
path: "file".into(),
old_path: None,
status: FileStatus::Modified,
@@ -2164,10 +2169,12 @@ fn reviewed_files_survive_lazy_details_and_views_but_reset_on_comparison() {
}
#[test]
fn restored_review_marks_use_revision_pairs_and_rename_paths_not_catalog_order() {
fn restored_review_marks_use_file_identities_and_rename_paths_not_catalog_order() {
let mut state = DiffState::new();
state.request_kind = Some(RequestKind::Comparison);
state.apply_result(1, Ok(Outcome::Comparison(comparison())));
state.comparison.as_mut().unwrap().files[0].review_revisions =
Some(["file:before".into(), "file:after".into()]);
assert_eq!(state.toggle_reviewed(), Ok(true));
state.comparison.as_mut().unwrap().files.swap(0, 1);
state.restore_reviewed_files();
@@ -2177,17 +2184,104 @@ fn restored_review_marks_use_revision_pairs_and_rename_paths_not_catalog_order()
assert!(state.reviewed_files.is_empty());
state.comparison.as_mut().unwrap().files[1].old_path = Some("old.txt".into());
for side in [0, 1] {
let original = state.comparison.as_ref().unwrap().revisions[side].clone();
state.comparison.as_mut().unwrap().revisions[side].push_str("-different");
let identities = state.comparison.as_mut().unwrap().files[1]
.review_revisions
.as_mut()
.unwrap();
let original = identities[side].clone();
identities[side].push_str("-different");
state.restore_reviewed_files();
assert!(state.reviewed_files.is_empty());
state.comparison.as_mut().unwrap().revisions[side] = original;
state.comparison.as_mut().unwrap().files[1]
.review_revisions
.as_mut()
.unwrap()[side] = original;
}
state.comparison.as_mut().unwrap().files[1].loaded = false;
state.restore_reviewed_files();
assert_eq!(state.reviewed_files, BTreeSet::from([1]));
}
#[test]
fn unchanged_file_review_survives_other_files_changing() {
use crate::repository::Repository;
use std::sync::{Arc, atomic::AtomicBool};
let root = std::env::temp_dir().join(format!("vamp-review-files-{}", std::process::id()));
std::fs::create_dir(&root).unwrap();
let git = |args: &[&str]| {
let output = std::process::Command::new("git")
.current_dir(&root)
.args(args)
.env("GIT_CONFIG_GLOBAL", "/dev/null")
.env("GIT_CONFIG_NOSYSTEM", "1")
.env("GIT_AUTHOR_DATE", "2000-01-01T00:00:00Z")
.env("GIT_COMMITTER_DATE", "2000-01-01T00:00:00Z")
.output()
.unwrap();
assert!(
output.status.success(),
"{}",
String::from_utf8_lossy(&output.stderr)
);
String::from_utf8(output.stdout).unwrap().trim().to_owned()
};
git(&["init", "--initial-branch=local"]);
git(&["config", "user.name", "Review Test"]);
git(&["config", "user.email", "review@example.invalid"]);
for path in ["a.txt", "b.txt"] {
std::fs::write(root.join(path), "base\n").unwrap();
}
git(&["add", "."]);
git(&["commit", "-m", "base"]);
for path in ["a.txt", "b.txt"] {
std::fs::write(root.join(path), "changed\n").unwrap();
}
let before = revision(
RevisionTarget::Commit(git(&["rev-parse", "HEAD"])),
"base",
"",
Category::Commits,
);
let after = revision(RevisionTarget::Working, "Working", "", Category::Commits);
let mut repository = Repository::open(&root, Arc::new(AtomicBool::new(false))).unwrap();
let mut state = DiffState::new();
let changes = repository
.compare(1, &before, &after, ComparisonMode::Snapshots, &[])
.unwrap();
let original_revisions = changes.revisions.clone();
state.request_kind = Some(RequestKind::Comparison);
state.apply_result(1, Ok(Outcome::Comparison(changes)));
state.toggle_reviewed().unwrap();
state.file = 1;
state.focus = Focus::Before;
state.toggle_reviewed().unwrap();
let mut store = crate::review::Store::open(&root.join("reviews")).unwrap();
store.save(&mut state.local_review.document).unwrap();
let id = state.local_review.document.storage_id.unwrap();
std::fs::write(root.join("b.txt"), "edited again\n").unwrap();
let changes = repository
.compare(2, &before, &after, ComparisonMode::Snapshots, &[])
.unwrap();
assert_ne!(changes.revisions, original_revisions);
state.request_kind = Some(RequestKind::Comparison);
state.apply_result(2, Ok(Outcome::Comparison(changes)));
state.local_review.document = store.load(&root, id).unwrap();
state.restore_reviewed_files();
assert_eq!(state.reviewed_files, BTreeSet::from([0]));
let detail = repository.load_file(2, 0).unwrap();
state.apply_result(
3,
Ok(Outcome::File {
comparison_id: 2,
file_index: 0,
file: detail,
}),
);
state.restore_reviewed_files();
assert_eq!(state.reviewed_files, BTreeSet::from([0]));
std::fs::remove_dir_all(root).unwrap();
}
#[test]
fn restored_live_review_marks_require_exact_text_and_final_newline() {
let mut state = live_state(vec!["base".into()], vec!["changed".into()], false);
@@ -2405,6 +2499,7 @@ fn reverting_loaded_file_keeps_unloaded_changes_selectable() {
state.comparison_id = Some(17);
state.pending = None;
state.comparison.as_mut().unwrap().files.push(ChangedFile {
review_revisions: None,
path: "unloaded.txt".into(),
old_path: None,
status: FileStatus::Modified,
@@ -2632,6 +2727,7 @@ fn lazy_selection_waits_for_details_rejects_stale_files_and_reuses_loaded_cache(
let details: Vec<_> = catalog.files.drain(..).collect();
for detail in &details {
catalog.files.push(ChangedFile {
review_revisions: None,
path: detail.path.clone(),
old_path: detail.old_path.clone(),
status: detail.status,
+7
View File
@@ -1885,6 +1885,7 @@ fn hidden_tab_diff_results_restore_visible_insert_interaction_and_route_request_
after: "working".into(),
editable_paths: Default::default(),
files: vec![ChangedFile {
review_revisions: None,
loaded: true,
path: "file.txt".into(),
old_path: None,
@@ -3050,6 +3051,7 @@ fn working_diff(editor: &mut Editor, root: &Path, paths: &[&str]) -> Box<[(PathB
let text = fs::read_to_string(root.join(path)).unwrap();
let lines: Vec<String> = text.lines().map(str::to_owned).collect();
ChangedFile {
review_revisions: None,
loaded: true,
path: PathBuf::from(*path),
old_path: None,
@@ -3109,6 +3111,7 @@ fn historical_diff(editor: &mut Editor, root: &Path, before: Vec<String>, after:
after: "head".into(),
editable_paths: Default::default(),
files: vec![ChangedFile {
review_revisions: None,
loaded: true,
path: "file.txt".into(),
old_path: None,
@@ -4071,6 +4074,7 @@ fn bookmark_refresh_reloads_paths_and_preserves_view_until_lazy_content_arrives(
let old_snapshot = editor.buffer_id();
let cursor = editor.cursor();
let file = |path: &str, loaded| ChangedFile {
review_revisions: None,
path: path.into(),
old_path: None,
status: FileStatus::Modified,
@@ -4278,6 +4282,7 @@ fn completed_working_refresh_serializes_lazy_selection_and_invalidates_pending_n
}
if changed {
let file = crate::diff::ChangedFile {
review_revisions: None,
path: "nested/new.txt".into(),
old_path: Some("nested/old.txt".into()),
status: crate::diff::FileStatus::Renamed,
@@ -4318,6 +4323,7 @@ fn lazy_lsp_target(editor: &mut Editor) -> crate::diff::ChangedFile {
let diff = editor.diff.as_mut().unwrap();
let comparison = diff.comparison.as_mut().unwrap();
comparison.files.push(ChangedFile {
review_revisions: None,
path: "nested/new.txt".into(),
old_path: Some("nested/old.txt".into()),
status: FileStatus::Renamed,
@@ -4338,6 +4344,7 @@ fn lazy_lsp_target(editor: &mut Editor) -> crate::diff::ChangedFile {
.map(|(index, file)| (index, file.path.clone())),
));
ChangedFile {
review_revisions: None,
path: "nested/new.txt".into(),
old_path: Some("nested/old.txt".into()),
status: FileStatus::Renamed,
+1
View File
@@ -3727,6 +3727,7 @@ fn async_responses_edit_only_current_bound_working_after_and_refresh_projection(
after: "Working".into(),
editable_paths: [PathBuf::from("buffer.txt")].into_iter().collect(),
files: vec![ChangedFile {
review_revisions: None,
path: PathBuf::from("buffer.txt"),
old_path: None,
status: FileStatus::Modified,
+9
View File
@@ -1984,6 +1984,7 @@ fn syntax_diff_file(path: &str, before: &str, after: &str) -> ChangedFile {
let before: Vec<String> = before.split('\n').map(str::to_owned).collect();
let after: Vec<String> = after.split('\n').map(str::to_owned).collect();
ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,
@@ -2605,6 +2606,7 @@ fn diff_comparison(files: Vec<ChangedFile>) -> DiffState {
fn deletion_file() -> ChangedFile {
ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,
@@ -3555,6 +3557,7 @@ fn diff_excerpts_render_original_lines_and_expand_without_losing_changes() {
after[30] = "let first_change = 900;".into();
after[60] = "let second_change = 901;".into();
let mut diff = diff_comparison(vec![ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,
@@ -3694,6 +3697,7 @@ fn diff_row_tint_covers_blank_lines_and_wrapped_gutters() {
#[test]
fn diff_added_rows_hatch_missing_before_but_tint_real_empty_after_lines() {
let mut diff = diff_comparison(vec![ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,
@@ -3751,6 +3755,7 @@ fn diff_added_rows_hatch_missing_before_but_tint_real_empty_after_lines() {
#[test]
fn diff_changed_backgrounds_preserve_syntax_and_clipped_source_cells() {
let mut diff = diff_comparison(vec![ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,
@@ -3880,6 +3885,7 @@ fn diff_changed_backgrounds_preserve_syntax_and_clipped_source_cells() {
#[test]
fn diff_view_changes_files_focus_and_keeps_horizontal_offsets_independent() {
let added = ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,
@@ -4641,6 +4647,7 @@ fn large_generated_additions_render_first_scroll_and_final_rows_without_reparsin
.collect::<Vec<_>>();
let hunks = crate::diff::line_hunks(&[], false, &after, true);
let mut diff = diff_comparison(vec![ChangedFile {
review_revisions: None,
path: "parser.c".into(),
old_path: None,
status: FileStatus::Added,
@@ -4690,6 +4697,7 @@ fn diff_notices_empty_comparisons_and_tiny_layouts_remain_renderable() {
assert!(screen_text(&terminal).contains("No changed files"));
assert!(screen_text(&terminal).contains("Explorer · Tree"));
let binary = diff_comparison(vec![ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,
@@ -4704,6 +4712,7 @@ fn diff_notices_empty_comparisons_and_tiny_layouts_remain_renderable() {
render_diff(&mut terminal, &binary, &mut panels);
assert!(screen_text(&terminal).contains("Binary file"));
let blank = diff_comparison(vec![ChangedFile {
review_revisions: None,
loaded: true,
before_eof: false,
after_eof: false,