refactor: remove legacy review JSON migration

This commit is contained in:
Valdemar Lorenzen
2026-10-16 15:07:03 +02:00
parent 2259d61633
commit 5b8f8c3f07
6 changed files with 6 additions and 197 deletions
+1 -1
View File
@@ -13,7 +13,7 @@
- characterwise Visual (`v`) anchors the exact selected text; Visual Line (`V`) anchors whole selected lines; no selection retains current-line behavior. - characterwise Visual (`v`) anchors the exact selected text; Visual Line (`V`) anchors whole selected lines; no selection retains current-line behavior.
- retain gutter markers for discovery and highlight the exact anchored text when its comment is selected. - retain gutter markers for discovery and highlight the exact anchored text when its comment is selected.
- preserve immutable snapshot identity and existing stale detection; never silently reanchor comments after source changes. - preserve immutable snapshot identity and existing stale detection; never silently reanchor comments after source changes.
- persist precise ranges in version 2 JSON import/export; migrate existing line-only anchors without losing comments. SQLite storage follows in the next item. - persist precise ranges in version 2 JSON import/export; no backwards compatibility with old review formats. SQLite storage follows in the next item.
- future GitHub publishing may map precise local anchors to supported line ranges without reducing precision in the local review. - future GitHub publishing may map precise local anchors to supported line ranges without reducing precision in the local review.
- [ ] persist local reviews automatically in a local SQLite database using rusqlite; no server, hosted storage, or synchronization. - [ ] persist local reviews automatically in a local SQLite database using rusqlite; no server, hosted storage, or synchronization.
- keep one database under $XDG_DATA_HOME/vamp/reviews/, falling back to ~/.local/share/vamp/reviews/; allow a configurable review_directory. - keep one database under $XDG_DATA_HOME/vamp/reviews/, falling back to ~/.local/share/vamp/reviews/; allow a configurable review_directory.
+1 -1
View File
@@ -17,7 +17,7 @@ One editor crate; the workspace also includes its patched Crossterm dependency.
- `diff`: owns searchable Before/After selection, category, focus and explorer visibility, request freshness, read-only comparison DTOs, aligned rows with cached grapheme-level change ranges, and shared vertical/independent horizontal offsets. Changed paths form a virtual `files::FilePicker` catalog, reusing the normal tree projection, filtering, and navigation without scanning disk. It has no subprocess or terminal dependencies. - `diff`: owns searchable Before/After selection, category, focus and explorer visibility, request freshness, read-only comparison DTOs, aligned rows with cached grapheme-level change ranges, and shared vertical/independent horizontal offsets. Changed paths form a virtual `files::FilePicker` catalog, reusing the normal tree projection, filtering, and navigation without scanning disk. It has no subprocess or terminal dependencies.
- `repository`: discovers jj/Git repositories and local/advertised remote revisions, builds disk snapshots and changed-file hunks, and owns isolated temporary comparison repositories with bounded, cancellable subprocesses. It never saves editor buffers or modifies active refs/checkouts. - `repository`: discovers jj/Git repositories and local/advertised remote revisions, builds disk snapshots and changed-file hunks, and owns isolated temporary comparison repositories with bounded, cancellable subprocesses. It never saves editor buffers or modifies active refs/checkouts.
- `review`: owns versioned local JSON review documents, immutable source anchors, validation, bounded reads, and atomic writes with external-change protection. It has no terminal or network operations. Comparison state owns the active review; `editor_review` keeps native comment/summary buffers and the read-only overview attached to their owning tab, synchronizes drafts on review commands, and guards unsaved teardown. The floating review editor has a tab-owned native pane outside the workspace `PaneTree`; rendering overlays it on the diff owner's rectangle without changing the source layout. Dismissal retains drafts and restores source focus. Working snapshot IDs hash the private captured paths, modes, contents, and notices; anchors also retain exact file text to account for live edits without reanchoring. - `review`: owns versioned local JSON review documents, immutable source anchors, validation, bounded reads, and atomic writes with external-change protection. It has no terminal or network operations. Comparison state owns the active review; `editor_review` keeps native comment/summary buffers and the read-only overview attached to their owning tab, synchronizes drafts on review commands, and guards unsaved teardown. The floating review editor has a tab-owned native pane outside the workspace `PaneTree`; rendering overlays it on the diff owner's rectangle without changing the source layout. Dismissal retains drafts and restores source focus. Working snapshot IDs hash the private captured paths, modes, contents, and notices; anchors also retain exact file text to account for live edits without reanchoring.
Version 2 anchors use the existing `plugin::TextPosition` type for zero-based grapheme start/end positions with an exclusive end. The JSON loading boundary migrates strict version 1 line-only records into this model while retaining the original disk bytes for external-write checks. Live code has no legacy coordinate aliases. Selected-comment painting reuses source-side grapheme clipping and wrapping; native selections retain higher priority. Version 2 anchors use the existing `plugin::TextPosition` type for zero-based grapheme start/end positions with an exclusive end. JSON loading accepts only the current schema and retains original disk bytes for external-write checks. Selected-comment painting reuses source-side grapheme clipping and wrapping; native selections retain higher priority.
- `clipboard`: owns native plain-text provider selection and bounded subprocess I/O; it never computes selections or editing offsets. - `clipboard`: owns native plain-text provider selection and bounded subprocess I/O; it never computes selections or editing offsets.
- `syntax`: owns Tree-sitter grammar detection, per-buffer incremental trees, bundled highlight/local/injection queries, and syntax byte spans; it has no terminal color/geometry or editing dependency. - `syntax`: owns Tree-sitter grammar detection, per-buffer incremental trees, bundled highlight/local/injection queries, and syntax byte spans; it has no terminal color/geometry or editing dependency.
- `markdown`: owns native Markdown link/reference resolution, local destination decoding, and heading fragment lookup through the installed block/inline grammars. It has no editor lifecycle, terminal, LSP, or external-process dependency. - `markdown`: owns native Markdown link/reference resolution, local destination decoding, and heading fragment lookup through the installed block/inline grammars. It has no editor lifecycle, terminal, LSP, or external-process dependency.
+1 -1
View File
@@ -39,7 +39,7 @@ Rust tests live alongside their modules; native LSP integration checks are in [`
- Editor checks cover Unicode editing, multi-target state, undo/redo, saving failures, and modal paste acceptance and rejection atomicity. - Editor checks cover Unicode editing, multi-target state, undo/redo, saving failures, and modal paste acceptance and rejection atomicity.
- Visual-command checks preserve directed characterwise/linewise ranges and multiple carets through command cancellation and plugin dispatch, exercise remapped command entry, and submit `:diff.comment` on either historical side. Live PTY smoke selects lines, cancels a prompt, then runs `:diff.comment` directly and verifies the saved original range without using the palette. - Visual-command checks preserve directed characterwise/linewise ranges and multiple carets through command cancellation and plugin dispatch, exercise remapped command entry, and submit `:diff.comment` on either historical side. Live PTY smoke selects lines, cancels a prompt, then runs `:diff.comment` directly and verifies the saved original range without using the palette.
- Presentation regressions render retained characterwise/linewise highlights during command entry in ordinary, split-diff, and unified views. Floating-review checks preserve source rectangles and the workspace layout, place the caret inside the overlay, retain dismissed drafts across tab switches, and restore source focus on Escape or pane close. Live PTY smoke edits, dismisses, reopens, and saves a floating comment and summary before returning to the unchanged diff. - Presentation regressions render retained characterwise/linewise highlights during command entry in ordinary, split-diff, and unified views. Floating-review checks preserve source rectangles and the workspace layout, place the caret inside the overlay, retain dismissed drafts across tab switches, and restore source focus on Escape or pane close. Live PTY smoke edits, dismisses, reopens, and saves a floating comment and summary before returning to the unchanged diff.
- Precise-range checks cover forward/reversed characterwise and partial-multiline selections on both sides, linewise and caret/empty-line fallbacks, source-column navigation, Unicode grapheme bounds, strict v1-to-v2 JSON migration, invalid/mixed schemas, and preserved save-conflict checks. Render checks cover exact text/newline cells, wrapping, horizontal clipping, shared unified context, stale suppression, and native Visual precedence. Live PTY smoke saves a selection containing a wide and combining grapheme, then imports and rewrites a legacy line-only review without changing its body. - Precise-range checks cover forward/reversed characterwise and partial-multiline selections on both sides, linewise and caret/empty-line fallbacks, source-column navigation, Unicode grapheme bounds, invalid schemas, and preserved save-conflict checks. Render checks cover exact text/newline cells, wrapping, horizontal clipping, shared unified context, stale suppression, and native Visual precedence. Live PTY smoke saves and reopens a selection containing a wide and combining grapheme.
- Markdown navigation checks cover inline/reference destinations, escaped/percent-encoded paths, heading markup and duplicates, same-document fragments, dirty target reuse, failed navigation atomicity, primary-only following, and `gf` prefix cancellation/configuration overrides. - Markdown navigation checks cover inline/reference destinations, escaped/percent-encoded paths, heading markup and duplicates, same-document fragments, dirty target reuse, failed navigation atomicity, primary-only following, and `gf` prefix cancellation/configuration overrides.
- Jump-history checks cover cross-buffer/same-document navigation, search and picker recording, counts and repeated visits, bounded history, branch truncation, closed buffers, edited-coordinate clamping, dirty text/undo preservation, independent Ctrl+i/Tab bindings, and custom bindings. Diff checks exercise Ctrl+i on either side in split/unified views, both control-letter and modified-Tab terminal events, and plain Tab pane cycling. Real LSP fixtures cover definition round-trips and delayed replies after leaving and returning to the same source location. - Jump-history checks cover cross-buffer/same-document navigation, search and picker recording, counts and repeated visits, bounded history, branch truncation, closed buffers, edited-coordinate clamping, dirty text/undo preservation, independent Ctrl+i/Tab bindings, and custom bindings. Diff checks exercise Ctrl+i on either side in split/unified views, both control-letter and modified-Tab terminal events, and plain Tab pane cycling. Real LSP fixtures cover definition round-trips and delayed replies after leaving and returning to the same source location.
- Tab checks cover shared edits/history with independent pane layouts, focused views, explorers and viewports; cross-tab close guards, immutable snapshots, hidden comparison refresh/result routing, and separate repository workers. Rendered checks cover sanitized/dirty titles, active-tab visibility in narrow terminals, overlay row reservation, and exclusion of hidden diff geometry from palette polling and viewport reporting. - Tab checks cover shared edits/history with independent pane layouts, focused views, explorers and viewports; cross-tab close guards, immutable snapshots, hidden comparison refresh/result routing, and separate repository workers. Rendered checks cover sanitized/dirty titles, active-tab visibility in narrow terminals, overlay row reservation, and exclusion of hidden diff geometry from palette polling and viewport reporting.
+1 -1
View File
@@ -471,7 +471,7 @@ Comments, summaries, and the review overview open in a focused floating editor o
In a review text pane, **`:w` saves the entire review**, not a source file. Choose a destination with `:diff.review.save path` first, or use `:w path`. Saving includes edits in hidden comment/summary buffers. Normal close, quit, restart, comparison replacement, and source-buffer deletion protect unsaved reviews; forced close/quit retains its explicit discard semantics. An externally changed review file is never silently overwritten, including with `:w!`; use a new path or reopen it after preserving your edits. In a review text pane, **`:w` saves the entire review**, not a source file. Choose a destination with `:diff.review.save path` first, or use `:w path`. Saving includes edits in hidden comment/summary buffers. Normal close, quit, restart, comparison replacement, and source-buffer deletion protect unsaved reviews; forced close/quit retains its explicit discard semantics. An externally changed review file is never silently overwritten, including with `:w!`; use a new path or reopen it after preserving your edits.
Version 2 JSON is the canonical, agent-readable artifact: repository root, summary, optional verdict, and comments with stable IDs, bodies, and anchors. 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. Existing version 1 line-only reviews load as equivalent whole-line spans, preserving IDs, text, verdicts, and snapshot identity; the next explicit save writes version 2. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. The file limit remains 64 MiB. Prefer saving outside the checkout so the review itself does not become another changed file. Version 2 JSON is the canonical, agent-readable artifact: repository root, summary, optional verdict, and comments with stable IDs, bodies, and anchors. Each anchor retains its immutable comparison revision, side, relative path (old path for renamed Before files), and exact whole-file text including the final newline. `start` and `end` contain zero-based source `row` and extended-grapheme `column` coordinates; `end` is exclusive. Columns count complete Unicode graphemes, not bytes, code points, UTF-16 units, or screen cells. Overview/window labels display one-based rows and columns. Only the current format is supported; old review files are not migrated. Working comparisons use content-addressed snapshot IDs; captured text additionally freezes live unsaved edits. The file limit remains 64 MiB. Prefer saving outside the checkout so the review itself does not become another changed file.
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. 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.
+1 -1
View File
@@ -84,7 +84,7 @@ Diff: :diff (Normal); choose Before then After; Working checkout After edits liv
Remote refs/history load into temporary comparison repos without changing active refs/checkout 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 Changed files: normal picker/explorer bindings, Ctrl+w close; A/M/D/R status and rename paths
Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the current line Local reviews: :diff.comment anchors exact Visual text, Visual Line rows, or the current line
Selected comments highlight their precise text; old line-only JSON reviews migrate on load Selected comments highlight their precise text
:diff.review.edit/delete [ID], :diff.review.next/previous, :diff.review.overview :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.summary; :diff.review.verdict looks_good|changes_requested|none
:diff.review.save PATH / :diff.review.open PATH; :w in floating review editors saves local JSON :diff.review.save PATH / :diff.review.open PATH; :w in floating review editors saves local JSON
+1 -192
View File
@@ -99,90 +99,6 @@ pub struct Review {
saved: RefCell<Option<(PathBuf, Arc<Vec<u8>>)>>, saved: RefCell<Option<(PathBuf, Arc<Vec<u8>>)>>,
} }
#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
struct LegacyReview {
version: u32,
root: PathBuf,
summary: String,
verdict: Option<Verdict>,
comments: Vec<LegacyComment>,
}
#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
struct LegacyComment {
id: u64,
anchor: LegacyAnchor,
body: String,
}
#[derive(Deserialize)]
#[serde(deny_unknown_fields)]
struct LegacyAnchor {
revision: String,
path: PathBuf,
side: Side,
start_line: usize,
end_line: usize,
content: String,
}
impl LegacyReview {
fn migrate(self) -> Result<Review, String> {
if self.version != 1 {
return Err(format!("Unsupported review version {}", self.version));
}
let comments = self
.comments
.into_iter()
.map(|comment| {
let old = comment.anchor;
if old.start_line == 0 || old.end_line < old.start_line {
return Err("Legacy review ranges must be inclusive and nonzero".into());
}
let mut lines = old.content.split('\n');
let last = lines
.nth(old.end_line - 1)
.ok_or("Legacy review range is outside the anchor text")?;
let end = if lines.next().is_some() {
crate::plugin::TextPosition {
row: old.end_line,
column: 0,
}
} else {
crate::plugin::TextPosition {
row: old.end_line - 1,
column: last.graphemes(true).count(),
}
};
Ok(Comment {
id: comment.id,
anchor: Anchor {
revision: old.revision,
path: old.path,
side: old.side,
start: crate::plugin::TextPosition {
row: old.start_line - 1,
column: 0,
},
end,
content: old.content,
},
body: comment.body,
})
})
.collect::<Result<_, String>>()?;
Ok(Review {
version: 2,
root: self.root,
summary: self.summary,
verdict: self.verdict,
comments,
saved: RefCell::new(None),
})
}
}
impl Default for Review { impl Default for Review {
fn default() -> Self { fn default() -> Self {
Self::new(PathBuf::new()) Self::new(PathBuf::new())
@@ -264,20 +180,8 @@ impl Review {
pub fn load(path: &Path, root: &Path) -> Result<Self, String> { pub fn load(path: &Path, root: &Path) -> Result<Self, String> {
let bytes = read_file(path)?.ok_or("Review file does not exist")?; let bytes = read_file(path)?.ok_or("Review file does not exist")?;
#[derive(Deserialize)] let mut review: Self = serde_json::from_slice(&bytes)
struct Version {
version: u32,
}
let Version { version } = serde_json::from_slice(&bytes)
.map_err(|error| format!("Invalid review JSON: {error}"))?; .map_err(|error| format!("Invalid review JSON: {error}"))?;
let mut review: Self = match version {
1 => serde_json::from_slice::<LegacyReview>(&bytes)
.map_err(|error| format!("Invalid review JSON: {error}"))?
.migrate()?,
2 => serde_json::from_slice(&bytes)
.map_err(|error| format!("Invalid review JSON: {error}"))?,
_ => return Err(format!("Unsupported review version {version}")),
};
review.validate()?; review.validate()?;
let expected = fs::canonicalize(root).map_err(|error| error.to_string())?; let expected = fs::canonicalize(root).map_err(|error| error.to_string())?;
let actual = fs::canonicalize(&review.root).map_err(|error| error.to_string())?; let actual = fs::canonicalize(&review.root).map_err(|error| error.to_string())?;
@@ -517,101 +421,6 @@ mod tests {
} }
} }
#[test]
fn legacy_lines_migrate_losslessly_and_save_exact_v2_ranges() {
use crate::plugin::TextPosition;
let directory = directory();
let root = directory.0.canonicalize().unwrap();
let path = root.join("review.json");
let cases = [
("first\nlast\n", 1, 2, (0, 0), (2, 0), 0..=1),
("first\nlast", 2, 2, (1, 0), (1, 4), 1..=1),
("é👩‍🔬e\u{301}", 1, 1, (0, 0), (0, 3), 0..=0),
("first\n\n", 2, 2, (1, 0), (2, 0), 1..=1),
("first\n", 2, 2, (1, 0), (1, 0), 1..=1),
("", 1, 1, (0, 0), (0, 0), 0..=0),
];
let comments: Vec<_> = cases
.iter()
.enumerate()
.map(|(index, case)| {
serde_json::json!({
"id": index + 11,
"anchor": {
"revision": "base:immutable",
"path": "old/name.rs",
"side": "before",
"start_line": case.1,
"end_line": case.2,
"content": case.0,
},
"body": format!("Comment {index}"),
})
})
.collect();
let original = serde_json::json!({
"version": 1,
"root": root,
"summary": "Keep this summary",
"verdict": "looks_good",
"comments": comments,
});
fs::write(&path, serde_json::to_vec(&original).unwrap()).unwrap();
let migrated = Review::load(&path, &root).unwrap();
assert_eq!(migrated.version, 2);
assert_eq!(migrated.root, root);
assert_eq!(migrated.summary, "Keep this summary");
assert_eq!(migrated.verdict, Some(Verdict::LooksGood));
for (index, (comment, case)) in migrated.comments.iter().zip(&cases).enumerate() {
assert_eq!(comment.id, index as u64 + 11);
assert_eq!(comment.body, format!("Comment {index}"));
assert_eq!(comment.anchor.revision, "base:immutable");
assert_eq!(comment.anchor.path, Path::new("old/name.rs"));
assert_eq!(comment.anchor.side, Side::Before);
assert_eq!(comment.anchor.content, case.0);
assert_eq!(
comment.anchor.start,
TextPosition {
row: case.3.0,
column: case.3.1
}
);
assert_eq!(
comment.anchor.end,
TextPosition {
row: case.4.0,
column: case.4.1
}
);
assert_eq!(comment.anchor.rows(), case.5);
}
migrated.save(&path).unwrap();
let saved: serde_json::Value = serde_json::from_slice(&fs::read(&path).unwrap()).unwrap();
assert_eq!(saved["version"], 2);
assert!(saved["comments"][0]["anchor"].get("start_line").is_none());
assert!(saved["comments"][0]["anchor"].get("end_line").is_none());
assert_eq!(
Review::load(&path, &root).unwrap().comments,
migrated.comments
);
for (start, end) in [(0, 1), (2, 1), (1, 4)] {
let mut invalid = original.clone();
invalid["comments"][0]["anchor"]["start_line"] = start.into();
invalid["comments"][0]["anchor"]["end_line"] = end.into();
fs::write(&path, serde_json::to_vec(&invalid).unwrap()).unwrap();
assert!(Review::load(&path, &root).is_err());
}
let mut mixed = original.clone();
mixed["comments"][0]["anchor"]["start"] = serde_json::json!({"row": 0, "column": 0});
fs::write(&path, serde_json::to_vec(&mixed).unwrap()).unwrap();
assert!(Review::load(&path, &root).is_err());
let mut wrong_version = original;
wrong_version["version"] = 2.into();
fs::write(&path, serde_json::to_vec(&wrong_version).unwrap()).unwrap();
assert!(Review::load(&path, &root).is_err());
}
#[test] #[test]
fn exact_grapheme_ranges_roundtrip_and_reject_invalid_coordinates() { fn exact_grapheme_ranges_roundtrip_and_reject_invalid_coordinates() {
use crate::plugin::TextPosition; use crate::plugin::TextPosition;