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

refactor: encapsulate review identity changes

Browse Source
This commit is contained in:
Valdemar Lorenzen
2026-10-16 18:03:15 +02:00
parent 8174e81c5c
commit befa432095
3 changed files with 128 additions and 46 deletions
Download Patch File Download Diff File Expand all files Collapse all files
docs/architecture.md
+1
View File
@@ -19,6 +19,7 @@ One editor crate; the workspace also includes its patched Crossterm dependency.
- `review`: owns the validated local review model, exact immutable anchors, SQLite storage through bundled rusqlite, and atomic JSON import/export with external-change protection. `Store` uses schema-versioned transactions and compare-and-swap review revisions; repository roots and persistent review IDs are independent of comparison revisions. One bounded JSON document is stored per row, retaining exact grapheme endpoints and stale snapshots. Incompatible schemas and concurrent edits fail without replacing data or advancing in-memory saved state.
`editor_review` commits native comment/summary buffers on dismissal or switching, and verdicts/deletions immediately. It restores a sole stored review or opens the floating selector for multiple reviews. Exports retain their own file baseline and do not become automatic persistence targets; imports get fresh database identities. Review editors remain tab-owned native panes outside the workspace `PaneTree`. Storage opens lazily when a comparison is ready; the session supplies the configured or XDG default directory. Direct `Editor` callers inject a directory with `set_review_directory` to enable persistence.
Successful SQLite autosaves and JSON-only saves share one acknowledgement operation: clear review dirtiness and advance the owning tab's native draft saved baselines only after persistence succeeds. Failed writes retain unsaved state; JSON exports do not acknowledge a pending SQLite save.
Review replacement and comment deletion are private editor-owned operations. Replacement loads and validates its Stored/JSON/New source before retiring old bindings, installing the document, and resetting selection/navigation. Deletion retires and acknowledges only the matching comment drafts, preventing stale text from overwriting reused IDs. Both persist before opening the overview; a post-mutation save failure retains the applied, unsaved identity change. Native text/undo and general transition guards remain unchanged.
- `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.
- `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.
src/editor_review.rs
+71 -46
View File
@@ -9,6 +9,12 @@ enum ReviewText {
Reviews,
}
enum ReviewSource<'a> {
Stored(i64),
Json(&'a Path),
New,
}
pub(super) struct ReviewBuffer {
owner: usize,
kind: ReviewText,
@@ -204,6 +210,63 @@ impl Editor {
}
}
fn replace_local_review(
&mut self,
root: PathBuf,
source: ReviewSource<'_>,
) -> Result<(), String> {
let (document, path, dirty) = match source {
ReviewSource::Stored(id) => {
let directory = self
.review_directory
.as_deref()
.ok_or("Review storage is not configured")?;
(
crate::review::Store::open(directory)?.load(&root, id)?,
None,
false,
)
}
ReviewSource::Json(path) => (
Review::load(path, &root)?,
Some(path.to_path_buf()),
self.review_directory.is_some(),
),
ReviewSource::New => (Review::new(root), None, true),
};
// Retire old identities before autosave can synchronize their native drafts.
self.detach_review_buffers();
let state = &mut self.review_diff_mut().unwrap().local_review;
state.document = document;
state.path = path;
state.selected = None;
state.pending_navigation = None;
state.dirty = dirty;
// Persistence failure leaves the identity change applied and unsaved.
self.autosave_local_review()?;
self.open_review_overview()
}
fn delete_local_review_comment(&mut self, args: &str) -> Result<(), String> {
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.autosave_local_review()?;
self.open_review_overview()
}
fn sync_review_text(&mut self) {
let owner = self.tab_id;
let updates: Vec<_> = std::iter::once(&self.active)
@@ -327,39 +390,19 @@ impl Editor {
if args.is_empty() {
return self.open_review_selector();
}
let document = if let (Some(directory), Ok(id)) =
(self.review_directory.as_deref(), args.parse::<i64>())
{
crate::review::Store::open(directory)?.load(&root, id)?
let source = if self.review_directory.is_some() {
args.parse::<i64>()
.map(ReviewSource::Stored)
.unwrap_or_else(|_| ReviewSource::Json(Path::new(args)))
} else {
Review::load(Path::new(args), &root)?
ReviewSource::Json(Path::new(args))
};
let imported = document.storage_id.is_none();
let dirty = imported && self.review_directory.is_some();
self.detach_review_buffers();
let state = &mut self.review_diff_mut().unwrap().local_review;
state.document = document;
state.path = imported.then(|| PathBuf::from(args));
state.selected = None;
state.pending_navigation = None;
state.dirty = dirty;
self.autosave_local_review()?;
return self.open_review_overview();
return self.replace_local_review(root, source);
}
self.sync_review_text();
match action {
Action::DiffComment => self.add_review_comment()?,
Action::ReviewNew => {
self.detach_review_buffers();
let state = &mut self.review_diff_mut().unwrap().local_review;
state.document = Review::new(root);
state.selected = None;
state.pending_navigation = None;
state.path = None;
state.dirty = true;
self.autosave_local_review()?;
self.open_review_overview()?;
}
Action::ReviewNew => self.replace_local_review(root, ReviewSource::New)?,
Action::ReviewEdit => {
let id = self.review_comment_id(args)?;
self.show_review_comment(id)?;
@@ -398,25 +441,7 @@ impl Editor {
self.autosave_local_review()?;
self.open_review_overview()?;
}
Action::ReviewDelete => {
let id = self.review_comment_id(args)?;
let state = &mut self.review_diff_mut().unwrap().local_review;
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.autosave_local_review()?;
self.open_review_overview()?;
}
Action::ReviewDelete => self.delete_local_review_comment(args)?,
Action::ReviewNext | Action::ReviewPrevious => {
let state = &self.workspace_diff().unwrap().local_review;
let comments = &state.document.comments;
tests/unit/editor.rs
+56
View File
@@ -3217,6 +3217,62 @@ fn local_review_native_composition_roundtrip_and_draft_lifecycle() {
);
}
#[test]
fn review_identity_changes_retire_drafts_before_reusing_comment_ids() {
for sqlite in [false, true] {
let directory = Directory::new();
let root = directory.0.canonicalize().unwrap();
let mut editor = editor();
if sqlite {
editor.set_review_directory(root.join("data"));
}
historical_diff(&mut editor, &root, vec!["old".into()], vec!["new".into()]);
editor.execute("diff.comment");
type_text(&mut editor, "iOriginal feedback");
editor.handle(Input::Escape);
editor.execute("diff.review.summary");
type_text(&mut editor, "iKeep summary");
editor.handle(Input::Escape);
let original = root.join("original.json");
editor.execute(&format!("diff.review.save {}", original.display()));
editor.execute("diff.review.delete 1");
editor.execute("diff.comment");
assert_eq!(editor.context().text, "");
type_text(&mut editor, "iFresh feedback");
editor.handle(Input::Escape);
let fresh = root.join("fresh.json");
editor.execute(&format!("diff.review.save {}", fresh.display()));
let document = crate::review::Review::load(&fresh, &root).unwrap();
assert_eq!(document.comments[0].id, 1);
assert_eq!(document.comments[0].body, "Fresh feedback");
assert_eq!(document.summary, "Keep summary");
editor.execute(&format!("diff.review.open {}", original.display()));
editor.execute("diff.review.edit 1");
assert_eq!(editor.context().text, "Original feedback");
editor.execute("diff.review.summary");
assert_eq!(editor.context().text, "Keep summary");
let restored = root.join("restored.json");
editor.execute(&format!("diff.review.save {}", restored.display()));
assert_eq!(
crate::review::Review::load(&restored, &root).unwrap().comments[0].body,
"Original feedback"
);
editor.execute("diff.review.new");
editor.execute("diff.comment");
assert_eq!(editor.context().text, "");
type_text(&mut editor, "iSeparate review");
editor.handle(Input::Escape);
let separate = root.join("separate.json");
editor.execute(&format!("diff.review.save {}", separate.display()));
let document = crate::review::Review::load(&separate, &root).unwrap();
assert_eq!(document.comments[0].body, "Separate review");
assert_eq!(document.summary, "");
}
}
#[test]
fn precise_review_ranges_capture_graphemes_and_navigate_to_their_start() {
for focus in [Focus::Before, Focus::After] {