From 14abdfd2cf4bd3ff7e854b168e4993cc7a673827 Mon Sep 17 00:00:00 2001 From: Valdemar Lorenzen Date: Fri, 16 Oct 2026 18:49:28 +0200 Subject: [PATCH] refactor: remove unused review state and duplicate storage reads --- docs/architecture.md | 1 + src/editor_review.rs | 27 +++++++++++++++++---------- src/review.rs | 20 ++++---------------- src/review_cli.rs | 8 ++------ tests/unit/editor.rs | 11 ----------- tests/unit/review.rs | 28 ++++++++++++---------------- 6 files changed, 36 insertions(+), 59 deletions(-) diff --git a/docs/architecture.md b/docs/architecture.md index 54e1f1d..3c73fb9 100644 --- a/docs/architecture.md +++ b/docs/architecture.md @@ -17,6 +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. - `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 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. + Store listing returns validated review documents in descending ID order. CLI metadata, sole-review restoration, and selector labels reuse those documents without reloading them; selector choices live only in the displayed buffer. `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. diff --git a/src/editor_review.rs b/src/editor_review.rs index 409d5cc..e133897 100644 --- a/src/editor_review.rs +++ b/src/editor_review.rs @@ -49,13 +49,11 @@ impl Editor { self.review_diff_mut().unwrap().local_review.initialized = true; self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone()); let store = crate::review::Store::open(&directory)?; - let choices = store.list(&root)?; - if choices.len() == 1 { - self.review_diff_mut().unwrap().local_review.document = - store.load(&root, choices[0].0)?; - } else if choices.len() > 1 { - self.review_diff_mut().unwrap().local_review.choices = choices; - self.open_review_selector()?; + let mut reviews = store.list(&root)?; + if reviews.len() == 1 { + self.review_diff_mut().unwrap().local_review.document = reviews.pop().unwrap(); + } else if reviews.len() > 1 { + self.show_review_selector(reviews)?; } Ok(()) } @@ -71,15 +69,24 @@ impl Editor { .as_ref() .ok_or("Comparison is loading")? .root; - let choices = crate::review::Store::open(directory)?.list(root)?; + let reviews = crate::review::Store::open(directory)?.list(root)?; + self.show_review_selector(reviews) + } + + fn show_review_selector(&mut self, reviews: Vec) -> Result<(), String> { let mut text = String::from( "# Local reviews\n\nEnter opens the review under the caret.\n:diff.review.new starts a separate review.\n", ); - for (id, label) in &choices { + for review in reviews { + let id = review.storage_id.unwrap(); + let label = review + .summary + .lines() + .find(|line| !line.trim().is_empty()) + .map_or_else(|| format!("Review {id}"), |line| line.trim().to_owned()); use std::fmt::Write as _; writeln!(text, "\n## Review #{id} · {label}").unwrap(); } - self.review_diff_mut().unwrap().local_review.choices = choices; let row = text .lines() .position(|line| line.starts_with("## Review #")) diff --git a/src/review.rs b/src/review.rs index 6ac20cf..6dbcc19 100644 --- a/src/review.rs +++ b/src/review.rs @@ -59,10 +59,6 @@ impl Anchor { self.start.row..=end } - 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, @@ -116,7 +112,6 @@ impl Default for Review { pub struct ReviewState { pub document: Review, pub initialized: bool, - pub choices: Vec<(i64, String)>, pub path: Option, pub selected: Option, pub pending_navigation: Option, @@ -407,7 +402,7 @@ impl Store { Ok(Self { connection }) } - pub fn list(&self, root: &Path) -> Result, String> { + pub fn list(&self, root: &Path) -> Result, String> { let root = fs::canonicalize(root).map_err(|error| error.to_string())?; let root_text = root .to_str() @@ -419,18 +414,11 @@ impl Store { ) .map_err(database_error)?; let mut rows = statement.query([root_text]).map_err(database_error)?; - let mut choices = Vec::new(); + let mut reviews = Vec::new(); while let Some(row) = rows.next().map_err(database_error)? { - let review = stored_review(row, &root)?; - let id = review.storage_id.unwrap(); - let label = review - .summary - .lines() - .find(|line| !line.trim().is_empty()) - .map_or_else(|| format!("Review {id}"), |line| line.trim().to_owned()); - choices.push((id, label)); + reviews.push(stored_review(row, &root)?); } - Ok(choices) + Ok(reviews) } pub fn load(&self, root: &Path, id: i64) -> Result { diff --git a/src/review_cli.rs b/src/review_cli.rs index 29b2985..44e6a27 100644 --- a/src/review_cli.rs +++ b/src/review_cli.rs @@ -125,12 +125,8 @@ pub fn run(request: &Request, config: Option<&Path>) -> Result<(), String> { Some(store) => store .list(&root)? .into_iter() - .map(|(id, _)| { - store - .load(&root, id) - .map(|review| envelope(&review, true, false)) - }) - .collect::, _>>()?, + .map(|review| envelope(&review, true, false)) + .collect::>(), None => Vec::new(), }; json!({"repository": root, "reviews": reviews}) diff --git a/tests/unit/editor.rs b/tests/unit/editor.rs index 4fbefac..8a7cb56 100644 --- a/tests/unit/editor.rs +++ b/tests/unit/editor.rs @@ -2910,17 +2910,6 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() { let mut choosing = editor(); choosing.set_review_directory(storage); historical_diff(&mut choosing, &root, vec!["old".into()], vec!["new".into()]); - assert_eq!( - choosing - .workspace_diff() - .unwrap() - .local_review - .choices - .iter() - .map(|(id, _)| *id) - .collect::>(), - std::collections::BTreeSet::from([id, second_id]) - ); choosing.handle(Input::Enter); assert_eq!( choosing diff --git a/tests/unit/review.rs b/tests/unit/review.rs index 67edc1d..cebdd00 100644 --- a/tests/unit/review.rs +++ b/tests/unit/review.rs @@ -46,20 +46,21 @@ fn database_restarts_preserve_identity_and_exact_stale_anchors() { } let mut store = Store::open(&storage).unwrap(); let id = review.storage_id.unwrap(); - assert_eq!( - store.list(&root).unwrap(), - vec![(id, "Unicode review".into())] - ); + let listed = store.list(&root).unwrap(); + assert_eq!(listed[0].storage_id, Some(id)); + assert_eq!(listed[0].summary, review.summary); + assert_eq!(listed[0].comments, review.comments); let mut loaded = store.load(&root, id).unwrap(); assert_eq!(loaded.storage_id, Some(id)); assert_eq!(loaded.storage_revision, 1); assert_eq!(loaded.comments, review.comments); assert_eq!(loaded.verdict, review.verdict); - assert!(!loaded.comments[0].anchor.matches( + assert!(!loaded.comments[0].anchor.matches_lines( "working:new-snapshot", Path::new("日本語.rs"), Side::Before, - "changed\n", + &["changed".into()], + true, )); let path = root.join("export.json"); loaded.save(&path).unwrap(); @@ -107,8 +108,8 @@ fn database_repository_identity_is_canonical_and_independent() { let alias = first_root.join("alias"); std::os::unix::fs::symlink(&first_root, &alias).unwrap(); assert_eq!( - store.list(&alias).unwrap(), - store.list(&first_root).unwrap() + store.list(&alias).unwrap()[0].storage_id, + store.list(&first_root).unwrap()[0].storage_id ); } } @@ -324,17 +325,12 @@ fn persistence_validates_anchors_and_preserves_files_on_failure() { 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( + assert!(!anchor.matches_lines( "working:other", Path::new("source.rs"), Side::After, - "first\nlast\n" + &["first".into(), "last".into()], + true )); assert!(anchor.matches_lines( "working:immutable",