feat: persist diff file review checkmarks

tests/unit/diff.rs

@@ -2163,6 +2163,51 @@ fn reviewed_files_survive_lazy_details_and_views_but_reset_on_comparison() {
     assert!(state.reviewed_files.is_empty());
 }
 
+#[test]
+fn restored_review_marks_use_revision_pairs_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())));
+    assert_eq!(state.toggle_reviewed(), Ok(true));
+    state.comparison.as_mut().unwrap().files.swap(0, 1);
+    state.restore_reviewed_files();
+    assert_eq!(state.reviewed_files, BTreeSet::from([1]));
+    state.comparison.as_mut().unwrap().files[1].old_path = Some("different.txt".into());
+    state.restore_reviewed_files();
+    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");
+        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].loaded = false;
+    state.restore_reviewed_files();
+    assert_eq!(state.reviewed_files, BTreeSet::from([1]));
+}
+
+#[test]
+fn restored_live_review_marks_require_exact_text_and_final_newline() {
+    let mut state = live_state(vec!["base".into()], vec!["changed".into()], false);
+    state.refresh_after(&["unsaved".into(), String::new()], 1);
+    assert_eq!(state.toggle_reviewed(), Ok(true));
+    let document = state.local_review.document.clone();
+    let mut reopened = live_state(vec!["base".into()], vec!["changed".into()], false);
+    reopened.local_review.document = document.clone();
+    reopened.restore_reviewed_files();
+    assert!(reopened.reviewed_files.is_empty());
+    reopened.refresh_after(&["unsaved".into(), String::new()], 1);
+    assert_eq!(reopened.reviewed_files, BTreeSet::from([0]));
+    reopened.refresh_after(&["unsaved".into()], 2);
+    assert!(reopened.reviewed_files.is_empty());
+    assert!(reopened.local_review.document.reviewed_files.is_empty());
+    reopened.local_review.document = document;
+    reopened.restore_reviewed_files();
+    assert!(reopened.reviewed_files.is_empty());
+}
+
 #[test]
 fn working_review_marks_clear_only_for_changed_bytes_or_eof() {
     for (after, eof, lines) in [

tests/unit/editor.rs

@@ -3206,6 +3206,73 @@ fn review_metadata_autosaves_do_not_commit_hidden_comment_or_summary_drafts() {
     assert!(!editor.local_reviews_dirty());
 }
 
+#[test]
+fn reviewed_file_marks_autosave_restore_and_stay_with_the_selected_review() {
+    use crate::review::{Review, Store};
+    let directory = Directory::new();
+    let root = directory.0.canonicalize().unwrap();
+    let storage = root.join("data");
+    let mut first = editor();
+    first.set_review_directory(storage.clone());
+    historical_diff(&mut first, &root, vec!["old".into()], vec!["new".into()]);
+    first.execute("diff.review.toggle");
+    let store = Store::open(&storage).unwrap();
+    let saved = store.list(&root).unwrap().pop().unwrap();
+    let id = saved.storage_id.unwrap();
+    assert_eq!(saved.reviewed_files[0].path, Path::new("file.txt"));
+    let export = root.join("review.json");
+    first.execute(&format!("diff.review.save {}", export.display()));
+    assert_eq!(
+        Review::load(&export, &root).unwrap().reviewed_files,
+        saved.reviewed_files
+    );
+    first.execute("diff.review.summary");
+    type_text(&mut first, "iUnsaved summary");
+    first.handle(Input::Escape);
+    first.execute("diff.review.toggle");
+    assert!(store.load(&root, id).unwrap().reviewed_files.is_empty());
+    assert_eq!(store.load(&root, id).unwrap().summary, "");
+    first.execute("diff.review.toggle");
+    drop(first);
+    let mut reopened = editor();
+    reopened.set_review_directory(storage.clone());
+    historical_diff(&mut reopened, &root, vec!["old".into()], vec!["new".into()]);
+    assert_eq!(
+        reopened.workspace_diff().unwrap().reviewed_files,
+        BTreeSet::from([0])
+    );
+    reopened.execute("diff.review.new");
+    assert!(reopened.workspace_diff().unwrap().reviewed_files.is_empty());
+    reopened.execute(&format!("diff.review.open {id}"));
+    assert_eq!(
+        reopened.workspace_diff().unwrap().reviewed_files,
+        BTreeSet::from([0])
+    );
+    reopened.handle(Input::Escape);
+    reopened.execute("diff.review.toggle");
+    assert!(store.load(&root, id).unwrap().reviewed_files.is_empty());
+}
+
+#[test]
+fn editing_a_reviewed_working_file_persists_its_invalidation() {
+    let directory = Directory::new();
+    let root = directory.0.canonicalize().unwrap();
+    fs::write(root.join("file.txt"), "original\n").unwrap();
+    let storage = root.join("data");
+    let mut editor = editor();
+    editor.set_review_directory(storage.clone());
+    working_diff(&mut editor, &root, &["file.txt"]);
+    editor.handle(Input::Enter);
+    editor.execute("diff.review.toggle");
+    let store = crate::review::Store::open(&storage).unwrap();
+    let id = store.list(&root).unwrap()[0].storage_id.unwrap();
+    assert_eq!(store.load(&root, id).unwrap().reviewed_files.len(), 1);
+    type_text(&mut editor, "iEdited ");
+    editor.handle(Input::Escape);
+    assert!(editor.workspace_diff().unwrap().reviewed_files.is_empty());
+    assert!(store.load(&root, id).unwrap().reviewed_files.is_empty());
+}
+
 #[test]
 fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
     use crate::review::{Review, Store, Verdict};
@@ -5276,6 +5343,10 @@ fn diff_review_actions_preserve_readonly_text_history_and_parked_tabs() {
     editor.execute("tab.previous");
     assert_eq!(editor.tab_id(), first_tab);
     assert_eq!(editor.diff().unwrap().reviewed_files, BTreeSet::from([0]));
+    editor.execute(&format!(
+        "diff.review.save {}",
+        directory.path("review.json").display()
+    ));
     editor.execute("diff.close");
     historical_diff(
         &mut editor,

tests/unit/review.rs

@@ -38,6 +38,16 @@ fn database_restarts_preserve_identity_and_exact_stale_anchors() {
         body: "Please preserve 👩‍🔬".into(),
         resolved: true,
     });
+    review.reviewed_files = [Some("live 👩‍🔬\r\n"), None, Some("")]
+        .into_iter()
+        .enumerate()
+        .map(|(index, after)| ReviewedFile {
+            revisions: [format!("before-{index}"), "after".into()],
+            path: "日本語.rs".into(),
+            old_path: (index == 0).then(|| "old.rs".into()),
+            after: after.map(str::to_owned),
+        })
+        .collect();
     {
         let mut store = Store::open(&storage).unwrap();
         store.save(&mut review).unwrap();
@@ -50,10 +60,12 @@ fn database_restarts_preserve_identity_and_exact_stale_anchors() {
     assert_eq!(listed[0].storage_id, Some(id));
     assert_eq!(listed[0].summary, review.summary);
     assert_eq!(listed[0].comments, review.comments);
+    assert_eq!(listed[0].reviewed_files, review.reviewed_files);
     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.reviewed_files, review.reviewed_files);
     assert_eq!(loaded.verdict, review.verdict);
     assert!(!loaded.comments[0].anchor.matches_lines(
         "working:new-snapshot",
@@ -67,7 +79,10 @@ fn database_restarts_preserve_identity_and_exact_stale_anchors() {
     let exported = fs::read(&path).unwrap();
     let json: serde_json::Value = serde_json::from_slice(&exported).unwrap();
     assert_eq!(json["comments"][0]["resolved"], true);
-    assert_eq!(json["version"], 3);
+    assert_eq!(json["version"], 4);
+    assert_eq!(json["reviewed_files"][0]["after"], "live 👩‍🔬\r\n");
+    assert!(json["reviewed_files"][1]["after"].is_null());
+    assert_eq!(json["reviewed_files"][2]["after"], "");
     assert!(json.get("storage_id").is_none());
     assert!(json.get("storage_revision").is_none());
     loaded.summary = "Database only".into();
@@ -78,9 +93,28 @@ fn database_restarts_preserve_identity_and_exact_stale_anchors() {
     let mut imported = Review::load(&path, &root).unwrap();
     assert_eq!(imported.storage_id, None);
     assert_eq!(imported.storage_revision, 0);
+    assert_eq!(imported.reviewed_files, review.reviewed_files);
     store.save(&mut imported).unwrap();
     assert_ne!(imported.storage_id, Some(id));
     assert_eq!(store.list(&root).unwrap().len(), 2);
+    assert_eq!(
+        store
+            .load(&root, imported.storage_id.unwrap())
+            .unwrap()
+            .reviewed_files,
+        review.reviewed_files,
+    );
+    let comments = loaded.comments.clone();
+    let summary = loaded.summary.clone();
+    let verdict = loaded.verdict;
+    loaded.reviewed_files.clear();
+    store.save(&mut loaded).unwrap();
+    drop(store);
+    let loaded = Store::open(&storage).unwrap().load(&root, id).unwrap();
+    assert!(loaded.reviewed_files.is_empty());
+    assert_eq!(loaded.comments, comments);
+    assert_eq!(loaded.summary, summary);
+    assert_eq!(loaded.verdict, verdict);
 }
 
 #[test]
@@ -122,12 +156,20 @@ fn database_conflicts_and_locked_writes_keep_drafts_and_identity() {
     let mut first = Store::open(&storage).unwrap();
     let mut second = Store::open(&storage).unwrap();
     let mut review = Review::new(root.clone());
+    review.reviewed_files.push(ReviewedFile {
+        revisions: ["before".into(), "after".into()],
+        path: "source.rs".into(),
+        old_path: None,
+        after: None,
+    });
     first.save(&mut review).unwrap();
     let id = review.storage_id.unwrap();
     let mut draft = second.load(&root, id).unwrap();
     review.summary = "First writer".into();
+    review.reviewed_files[0].after = Some("First writer buffer\n".into());
     first.save(&mut review).unwrap();
     draft.summary = "Second writer draft".into();
+    draft.reviewed_files.clear();
     assert!(
         second
             .save(&mut draft)
@@ -137,6 +179,11 @@ fn database_conflicts_and_locked_writes_keep_drafts_and_identity() {
     assert_eq!(draft.summary, "Second writer draft");
     assert_eq!((draft.storage_id, draft.storage_revision), (Some(id), 1));
     assert_eq!(second.load(&root, id).unwrap().summary, "First writer");
+    assert_eq!(
+        second.load(&root, id).unwrap().reviewed_files,
+        review.reviewed_files,
+    );
+    assert!(draft.reviewed_files.is_empty());
     first.connection.execute_batch("BEGIN IMMEDIATE").unwrap();
     let mut fresh = Review::new(root.clone());
     assert!(second.save(&mut fresh).unwrap_err().contains("locked"));
@@ -148,7 +195,13 @@ fn database_conflicts_and_locked_writes_keep_drafts_and_identity() {
 
 #[test]
 fn incompatible_database_schemas_are_preserved() {
+    let previous = format!(
+        "{REVIEWS_SCHEMA}; {COMMENTS_SCHEMA};
+         INSERT INTO reviews VALUES (1, '/old/repo', 1, 'Preserve me', 'looks_good');
+         PRAGMA user_version = 2"
+    );
     for schema in [
+        previous.as_str(),
         "CREATE TABLE other (value TEXT); PRAGMA user_version = 42",
         "CREATE TABLE other (value TEXT); PRAGMA user_version = 1",
         "CREATE TABLE reviews (id INTEGER PRIMARY KEY, root TEXT NOT NULL,
@@ -163,6 +216,8 @@ fn incompatible_database_schemas_are_preserved() {
         connection.execute_batch(schema).unwrap();
         drop(connection);
         let original = fs::read(&path).unwrap();
+        assert!(Store::open_read_only(&directory.0).is_err());
+        assert_eq!(fs::read(&path).unwrap(), original);
         assert!(Store::open(&directory.0).err().unwrap().contains("schema"));
         assert_eq!(fs::read(&path).unwrap(), original);
     }
@@ -461,6 +516,12 @@ fn comment_rows_preserve_order_full_ids_and_atomic_replacement() {
         })
         .collect();
     review.summary = "Original".into();
+    review.reviewed_files.push(ReviewedFile {
+        revisions: ["before".into(), "after".into()],
+        path: "source.rs".into(),
+        old_path: None,
+        after: None,
+    });
     store.save(&mut review).unwrap();
     let id = review.storage_id.unwrap();
     assert_eq!(store.load(&root, id).unwrap().comments, review.comments);
@@ -488,6 +549,7 @@ fn comment_rows_preserve_order_full_ids_and_atomic_replacement() {
         .unwrap();
     let revision = review.storage_revision;
     let original = review.comments.clone();
+    let original_files = review.reviewed_files.clone();
     review.summary = "Unsaved".into();
     review.comments[0].body = "Changed".into();
     assert!(
@@ -501,10 +563,30 @@ fn comment_rows_preserve_order_full_ids_and_atomic_replacement() {
     assert_eq!(loaded.summary, "Original");
     assert_eq!(loaded.comments, original);
     assert_eq!(loaded.storage_revision, revision);
+    assert_eq!(loaded.reviewed_files, original_files);
     store
         .connection
         .execute_batch("DROP TRIGGER reject_comment")
         .unwrap();
+    store
+        .connection
+        .execute_batch(
+            "CREATE TEMP TRIGGER reject_reviewed BEFORE INSERT ON reviewed_files
+         BEGIN SELECT RAISE(ABORT, 'blocked reviewed file'); END",
+        )
+        .unwrap();
+    review.reviewed_files[0].after = Some("Unsaved buffer\n".into());
+    assert!(store.save(&mut review).is_err());
+    assert_eq!(review.storage_revision, revision);
+    let loaded = store.load(&root, id).unwrap();
+    assert_eq!(loaded.summary, "Original");
+    assert_eq!(loaded.comments, original);
+    assert_eq!(loaded.reviewed_files, original_files);
+    assert_eq!(loaded.storage_revision, revision);
+    store
+        .connection
+        .execute_batch("DROP TRIGGER reject_reviewed")
+        .unwrap();
     review.comments.reverse();
     store.save(&mut review).unwrap();
     assert_eq!(store.load(&root, id).unwrap().comments, review.comments);
@@ -547,16 +629,16 @@ fn exact_grapheme_ranges_roundtrip_and_reject_invalid_coordinates() {
         .as_object_mut()
         .unwrap()
         .remove("resolved");
-    for version in [2, 3] {
+    for version in [2, 3, 4] {
         missing_resolution["version"] = version.into();
         let bytes = serde_json::to_vec(&missing_resolution).unwrap();
         fs::write(&path, &bytes).unwrap();
         let error = Review::load(&path, &root).unwrap_err();
-        assert!(error.contains(if version == 2 {
-            "Unsupported review version 2"
+        if version < 4 {
+            assert!(error.contains("Unsupported review version"));
         } else {
-            "missing field `resolved`"
-        }));
+            assert!(error.contains("missing field"));
+        }
         assert_eq!(fs::read(&path).unwrap(), bytes);
     }
     for (field, value) in [
@@ -584,3 +666,159 @@ fn exact_grapheme_ranges_roundtrip_and_reject_invalid_coordinates() {
     loaded.comments[0].anchor.end = TextPosition { row: 0, column: 0 };
     assert!(loaded.save(&root.join("backwards.json")).is_err());
 }
+
+#[test]
+fn reviewed_files_reject_invalid_json_and_preserve_existing_data() {
+    let directory = directory();
+    let root = directory.0.canonicalize().unwrap();
+    let path = root.join("review.json");
+    let mut store = Store::open(&root.join("storage")).unwrap();
+    let mut review = Review::new(root.clone());
+    review.reviewed_files.push(ReviewedFile {
+        revisions: ["before".into(), "after".into()],
+        path: "source.rs".into(),
+        old_path: Some("old.rs".into()),
+        after: Some("Exact live text\r\n".into()),
+    });
+    store.save(&mut review).unwrap();
+    let valid = serde_json::to_value(&review).unwrap();
+    let mut invalid_documents = Vec::new();
+    for (field, value) in [
+        ("path", serde_json::json!("")),
+        ("path", serde_json::json!("../outside")),
+        ("path", serde_json::json!("/absolute")),
+        ("path", serde_json::json!(".git/config")),
+        ("path", serde_json::json!("nul\0.rs")),
+        ("old_path", serde_json::json!("../outside")),
+        ("old_path", serde_json::json!("/absolute")),
+        ("old_path", serde_json::json!(".jj/repo")),
+        ("old_path", serde_json::json!("nul\0.rs")),
+        ("revisions", serde_json::json!(["", "after"])),
+        ("revisions", serde_json::json!(["before", ""])),
+        ("revisions", serde_json::json!(["before\n", "after"])),
+        ("revisions", serde_json::json!(["before", "after\0"])),
+        ("revisions", serde_json::json!(["before"])),
+        ("unexpected", serde_json::json!(true)),
+    ] {
+        let mut invalid = valid.clone();
+        invalid["reviewed_files"][0][field] = value;
+        invalid_documents.push(invalid);
+    }
+    let mut duplicate = valid.clone();
+    duplicate["reviewed_files"]
+        .as_array_mut()
+        .unwrap()
+        .push(valid["reviewed_files"][0].clone());
+    invalid_documents.push(duplicate);
+    let mut missing = valid.clone();
+    missing.as_object_mut().unwrap().remove("reviewed_files");
+    invalid_documents.push(missing);
+    let mut old = valid;
+    old["version"] = 3.into();
+    invalid_documents.push(old);
+    for invalid in invalid_documents {
+        let bytes = serde_json::to_vec(&invalid).unwrap();
+        fs::write(&path, &bytes).unwrap();
+        assert!(Review::load(&path, &root).is_err());
+        assert_eq!(fs::read(&path).unwrap(), bytes);
+        if let Ok(mut invalid) = serde_json::from_value::<Review>(invalid) {
+            assert!(store.save(&mut invalid).is_err());
+            assert!(invalid.save(&root.join("invalid.json")).is_err());
+            assert!(!root.join("invalid.json").exists());
+        }
+    }
+    let loaded = store.load(&root, review.storage_id.unwrap()).unwrap();
+    assert_eq!(loaded.reviewed_files, review.reviewed_files);
+    assert_eq!(store.list(&root).unwrap().len(), 1);
+    #[cfg(unix)]
+    {
+        use std::os::unix::ffi::OsStringExt;
+        for old_path in [false, true] {
+            let mut invalid = review.clone();
+            let path = PathBuf::from(std::ffi::OsString::from_vec(vec![0xff]));
+            if old_path {
+                invalid.reviewed_files[0].old_path = Some(path);
+            } else {
+                invalid.reviewed_files[0].path = path;
+            }
+            assert!(store.save(&mut invalid).is_err());
+        }
+    }
+}
+
+#[test]
+fn reviewed_file_rows_enforce_identity_and_limit_snapshots_before_loading() {
+    let directory = directory();
+    let root = directory.0.canonicalize().unwrap();
+    let mut store = Store::open(&root.join("storage")).unwrap();
+    let mut review = Review::new(root.clone());
+    review.reviewed_files.push(ReviewedFile {
+        revisions: ["before".into(), "after".into()],
+        path: "source.rs".into(),
+        old_path: None,
+        after: None,
+    });
+    store.save(&mut review).unwrap();
+    let id = review.storage_id.unwrap();
+    for update in [
+        "review_id = 0",
+        "before_revision = ''",
+        "after_revision = ''",
+        "path = NULL",
+        "after_content = zeroblob(1)",
+        "position = -1",
+    ] {
+        assert!(
+            store
+                .connection
+                .execute(
+                    &format!("UPDATE reviewed_files SET {update} WHERE review_id = ?1"),
+                    [id],
+                )
+                .is_err(),
+            "{update}",
+        );
+    }
+    assert!(
+        store
+            .connection
+            .execute(
+                "INSERT INTO reviewed_files
+                 SELECT review_id, position + 1, before_revision, after_revision,
+                    path, old_path, after_content FROM reviewed_files WHERE review_id = ?1",
+                [id],
+            )
+            .is_err()
+    );
+    for update in [
+        "before_revision = char(10)",
+        "after_revision = 'after' || char(0)",
+        "path = '../outside'",
+        "old_path = '../outside'",
+        "old_path = 'nul' || char(0)",
+        "position = 1",
+    ] {
+        store
+            .connection
+            .execute(
+                &format!("UPDATE reviewed_files SET {update} WHERE review_id = ?1"),
+                [id],
+            )
+            .unwrap();
+        assert!(store.load(&root, id).is_err(), "{update}");
+        assert!(store.list(&root).is_err(), "{update}");
+        store.save(&mut review).unwrap();
+    }
+    store
+        .connection
+        .execute(
+            "UPDATE reviewed_files SET after_content = CAST(zeroblob(?1) AS TEXT)
+             WHERE review_id = ?2",
+            params![LIMIT as i64 + 1, id],
+        )
+        .unwrap();
+    assert!(store.load(&root, id).unwrap_err().contains("64 MiB"));
+    assert!(store.list(&root).unwrap_err().contains("64 MiB"));
+    review.reviewed_files[0].after = Some("x".repeat(LIMIT));
+    assert!(store.save(&mut review).unwrap_err().contains("64 MiB"));
+}