refactor: remove unused review state and duplicate storage reads
This commit is contained in:
@@ -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.
|
- `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 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.
|
- `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.
|
`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.
|
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.
|
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.
|
||||||
|
|||||||
+17
-10
@@ -49,13 +49,11 @@ impl Editor {
|
|||||||
self.review_diff_mut().unwrap().local_review.initialized = true;
|
self.review_diff_mut().unwrap().local_review.initialized = true;
|
||||||
self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone());
|
self.review_diff_mut().unwrap().local_review.document = Review::new(root.clone());
|
||||||
let store = crate::review::Store::open(&directory)?;
|
let store = crate::review::Store::open(&directory)?;
|
||||||
let choices = store.list(&root)?;
|
let mut reviews = store.list(&root)?;
|
||||||
if choices.len() == 1 {
|
if reviews.len() == 1 {
|
||||||
self.review_diff_mut().unwrap().local_review.document =
|
self.review_diff_mut().unwrap().local_review.document = reviews.pop().unwrap();
|
||||||
store.load(&root, choices[0].0)?;
|
} else if reviews.len() > 1 {
|
||||||
} else if choices.len() > 1 {
|
self.show_review_selector(reviews)?;
|
||||||
self.review_diff_mut().unwrap().local_review.choices = choices;
|
|
||||||
self.open_review_selector()?;
|
|
||||||
}
|
}
|
||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
@@ -71,15 +69,24 @@ impl Editor {
|
|||||||
.as_ref()
|
.as_ref()
|
||||||
.ok_or("Comparison is loading")?
|
.ok_or("Comparison is loading")?
|
||||||
.root;
|
.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<Review>) -> Result<(), String> {
|
||||||
let mut text = String::from(
|
let mut text = String::from(
|
||||||
"# Local reviews\n\nEnter opens the review under the caret.\n:diff.review.new starts a separate review.\n",
|
"# 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 _;
|
use std::fmt::Write as _;
|
||||||
writeln!(text, "\n## Review #{id} · {label}").unwrap();
|
writeln!(text, "\n## Review #{id} · {label}").unwrap();
|
||||||
}
|
}
|
||||||
self.review_diff_mut().unwrap().local_review.choices = choices;
|
|
||||||
let row = text
|
let row = text
|
||||||
.lines()
|
.lines()
|
||||||
.position(|line| line.starts_with("## Review #"))
|
.position(|line| line.starts_with("## Review #"))
|
||||||
|
|||||||
+4
-16
@@ -59,10 +59,6 @@ impl Anchor {
|
|||||||
self.start.row..=end
|
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(
|
pub fn matches_lines(
|
||||||
&self,
|
&self,
|
||||||
revision: &str,
|
revision: &str,
|
||||||
@@ -116,7 +112,6 @@ impl Default for Review {
|
|||||||
pub struct ReviewState {
|
pub struct ReviewState {
|
||||||
pub document: Review,
|
pub document: Review,
|
||||||
pub initialized: bool,
|
pub initialized: bool,
|
||||||
pub choices: Vec<(i64, String)>,
|
|
||||||
pub path: Option<PathBuf>,
|
pub path: Option<PathBuf>,
|
||||||
pub selected: Option<u64>,
|
pub selected: Option<u64>,
|
||||||
pub pending_navigation: Option<u64>,
|
pub pending_navigation: Option<u64>,
|
||||||
@@ -407,7 +402,7 @@ impl Store {
|
|||||||
Ok(Self { connection })
|
Ok(Self { connection })
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn list(&self, root: &Path) -> Result<Vec<(i64, String)>, String> {
|
pub fn list(&self, root: &Path) -> Result<Vec<Review>, String> {
|
||||||
let root = fs::canonicalize(root).map_err(|error| error.to_string())?;
|
let root = fs::canonicalize(root).map_err(|error| error.to_string())?;
|
||||||
let root_text = root
|
let root_text = root
|
||||||
.to_str()
|
.to_str()
|
||||||
@@ -419,18 +414,11 @@ impl Store {
|
|||||||
)
|
)
|
||||||
.map_err(database_error)?;
|
.map_err(database_error)?;
|
||||||
let mut rows = statement.query([root_text]).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)? {
|
while let Some(row) = rows.next().map_err(database_error)? {
|
||||||
let review = stored_review(row, &root)?;
|
reviews.push(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));
|
|
||||||
}
|
}
|
||||||
Ok(choices)
|
Ok(reviews)
|
||||||
}
|
}
|
||||||
|
|
||||||
pub fn load(&self, root: &Path, id: i64) -> Result<Review, String> {
|
pub fn load(&self, root: &Path, id: i64) -> Result<Review, String> {
|
||||||
|
|||||||
+2
-6
@@ -125,12 +125,8 @@ pub fn run(request: &Request, config: Option<&Path>) -> Result<(), String> {
|
|||||||
Some(store) => store
|
Some(store) => store
|
||||||
.list(&root)?
|
.list(&root)?
|
||||||
.into_iter()
|
.into_iter()
|
||||||
.map(|(id, _)| {
|
.map(|review| envelope(&review, true, false))
|
||||||
store
|
.collect::<Vec<_>>(),
|
||||||
.load(&root, id)
|
|
||||||
.map(|review| envelope(&review, true, false))
|
|
||||||
})
|
|
||||||
.collect::<Result<Vec<_>, _>>()?,
|
|
||||||
None => Vec::new(),
|
None => Vec::new(),
|
||||||
};
|
};
|
||||||
json!({"repository": root, "reviews": reviews})
|
json!({"repository": root, "reviews": reviews})
|
||||||
|
|||||||
@@ -2910,17 +2910,6 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
|
|||||||
let mut choosing = editor();
|
let mut choosing = editor();
|
||||||
choosing.set_review_directory(storage);
|
choosing.set_review_directory(storage);
|
||||||
historical_diff(&mut choosing, &root, vec!["old".into()], vec!["new".into()]);
|
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<_>>(),
|
|
||||||
std::collections::BTreeSet::from([id, second_id])
|
|
||||||
);
|
|
||||||
choosing.handle(Input::Enter);
|
choosing.handle(Input::Enter);
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
choosing
|
choosing
|
||||||
|
|||||||
+12
-16
@@ -46,20 +46,21 @@ fn database_restarts_preserve_identity_and_exact_stale_anchors() {
|
|||||||
}
|
}
|
||||||
let mut store = Store::open(&storage).unwrap();
|
let mut store = Store::open(&storage).unwrap();
|
||||||
let id = review.storage_id.unwrap();
|
let id = review.storage_id.unwrap();
|
||||||
assert_eq!(
|
let listed = store.list(&root).unwrap();
|
||||||
store.list(&root).unwrap(),
|
assert_eq!(listed[0].storage_id, Some(id));
|
||||||
vec![(id, "Unicode review".into())]
|
assert_eq!(listed[0].summary, review.summary);
|
||||||
);
|
assert_eq!(listed[0].comments, review.comments);
|
||||||
let mut loaded = store.load(&root, id).unwrap();
|
let mut loaded = store.load(&root, id).unwrap();
|
||||||
assert_eq!(loaded.storage_id, Some(id));
|
assert_eq!(loaded.storage_id, Some(id));
|
||||||
assert_eq!(loaded.storage_revision, 1);
|
assert_eq!(loaded.storage_revision, 1);
|
||||||
assert_eq!(loaded.comments, review.comments);
|
assert_eq!(loaded.comments, review.comments);
|
||||||
assert_eq!(loaded.verdict, review.verdict);
|
assert_eq!(loaded.verdict, review.verdict);
|
||||||
assert!(!loaded.comments[0].anchor.matches(
|
assert!(!loaded.comments[0].anchor.matches_lines(
|
||||||
"working:new-snapshot",
|
"working:new-snapshot",
|
||||||
Path::new("日本語.rs"),
|
Path::new("日本語.rs"),
|
||||||
Side::Before,
|
Side::Before,
|
||||||
"changed\n",
|
&["changed".into()],
|
||||||
|
true,
|
||||||
));
|
));
|
||||||
let path = root.join("export.json");
|
let path = root.join("export.json");
|
||||||
loaded.save(&path).unwrap();
|
loaded.save(&path).unwrap();
|
||||||
@@ -107,8 +108,8 @@ fn database_repository_identity_is_canonical_and_independent() {
|
|||||||
let alias = first_root.join("alias");
|
let alias = first_root.join("alias");
|
||||||
std::os::unix::fs::symlink(&first_root, &alias).unwrap();
|
std::os::unix::fs::symlink(&first_root, &alias).unwrap();
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
store.list(&alias).unwrap(),
|
store.list(&alias).unwrap()[0].storage_id,
|
||||||
store.list(&first_root).unwrap()
|
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.comments, review.comments);
|
||||||
assert_eq!(loaded.verdict, review.verdict);
|
assert_eq!(loaded.verdict, review.verdict);
|
||||||
let anchor = &loaded.comments[0].anchor;
|
let anchor = &loaded.comments[0].anchor;
|
||||||
assert!(anchor.matches(
|
assert!(!anchor.matches_lines(
|
||||||
"working:immutable",
|
|
||||||
Path::new("source.rs"),
|
|
||||||
Side::After,
|
|
||||||
"first\nlast\n"
|
|
||||||
));
|
|
||||||
assert!(!anchor.matches(
|
|
||||||
"working:other",
|
"working:other",
|
||||||
Path::new("source.rs"),
|
Path::new("source.rs"),
|
||||||
Side::After,
|
Side::After,
|
||||||
"first\nlast\n"
|
&["first".into(), "last".into()],
|
||||||
|
true
|
||||||
));
|
));
|
||||||
assert!(anchor.matches_lines(
|
assert!(anchor.matches_lines(
|
||||||
"working:immutable",
|
"working:immutable",
|
||||||
|
|||||||
Reference in New Issue
Block a user