fix: preserve review comments across unrelated edits

2026-10-20 14:58:56 +02:00
parent 2727425518
commit 31203889bb
10 changed files with 422 additions and 147 deletions
@@ -2895,15 +2895,15 @@ fn review_anchors_match_frozen_context_and_exact_live_eof_without_allocating() {
        end: crate::plugin::TextPosition { row: 1, column: 0 },
        content: "new\n".into(),
    };
-    assert!(state.review_anchor_current(&anchor));
+    assert!(state.review_anchor_range(&anchor).is_some());
    state.live_files.insert(0);
-    assert!(!state.review_anchor_current(&anchor));
+    assert!(state.review_anchor_range(&anchor).is_none());
    state.comparison.as_mut().unwrap().files[0]
        .after
        .push(String::new());
-    assert!(state.review_anchor_current(&anchor));
+    assert!(state.review_anchor_range(&anchor).is_some());
    state.comparison.as_mut().unwrap().files[0].after[0] = "changed".into();
-    assert!(!state.review_anchor_current(&anchor));
+    assert!(state.review_anchor_range(&anchor).is_none());
    state.comparison.as_mut().unwrap().files[0].old_path = Some("original".into());
    let before = Anchor {
        revision: "base".into(),
@@ -2912,12 +2912,12 @@ fn review_anchors_match_frozen_context_and_exact_live_eof_without_allocating() {
        content: "old\n".into(),
        ..anchor
    };
-    assert!(state.review_anchor_current(&before));
+    assert!(state.review_anchor_range(&before).is_some());
    state.comparison.as_mut().unwrap().files[0].loaded = false;
-    assert!(!state.review_anchor_current(&before));
+    assert!(state.review_anchor_range(&before).is_none());
    state.comparison.as_mut().unwrap().files[0].loaded = true;
    state.comparison.as_mut().unwrap().revisions[0] = "other".into();
-    assert!(!state.review_anchor_current(&before));
+    assert!(state.review_anchor_range(&before).is_none());
 }

 #[test]
@@ -3282,6 +3282,85 @@ fn editing_a_reviewed_working_file_persists_its_invalidation() {
    assert!(store.load(&root, id).unwrap().reviewed_files.is_empty());
 }

+#[test]
+fn review_navigation_maps_working_ranges_and_preserves_stale_snapshots() {
+    use crate::review::{Anchor, Comment, Side};
+    let directory = Directory::new();
+    let root = directory.0.canonicalize().unwrap();
+    fs::write(root.join("file.txt"), "header\nunique target\ntail\n").unwrap();
+    let mut editor = editor();
+    working_diff(&mut editor, &root, &["file.txt"]);
+    editor.handle(Input::Enter);
+    editor
+        .diff
+        .as_mut()
+        .unwrap()
+        .comparison
+        .as_mut()
+        .unwrap()
+        .revisions[1] = "working:old".into();
+    editor.diff.as_mut().unwrap().local_review.document = crate::review::Review::new(root);
+    let anchor = Anchor {
+        revision: "working:old".into(),
+        path: "file.txt".into(),
+        side: Side::After,
+        start: TextPosition { row: 1, column: 7 },
+        end: TextPosition { row: 1, column: 13 },
+        content: editor.lines().join("\n"),
+    };
+    editor
+        .diff
+        .as_mut()
+        .unwrap()
+        .local_review
+        .document
+        .comments
+        .push(Comment {
+            id: 1,
+            anchor: anchor.clone(),
+            body: "Feedback".into(),
+            resolved: false,
+        });
+    editor
+        .apply(vec![Effect::Replace(
+            "inserted\nheader\nprefix unique target\ntail\n".into(),
+        )])
+        .unwrap();
+    editor.refresh_live_diff();
+    editor
+        .diff
+        .as_mut()
+        .unwrap()
+        .comparison
+        .as_mut()
+        .unwrap()
+        .revisions[1] = "working:new".into();
+    editor.execute("diff.review.edit 1");
+    let owner = editor.diff_owner.unwrap();
+    editor.select_pane(owner);
+    assert_eq!(editor.cursor(), Cursor { row: 2, column: 14 });
+    assert_eq!(
+        editor.diff.as_ref().unwrap().local_review.document.comments[0].anchor,
+        anchor
+    );
+
+    editor
+        .apply(vec![Effect::Replace(
+            "inserted\nheader\nprefix unique changed\ntail\n".into(),
+        )])
+        .unwrap();
+    editor.refresh_live_diff();
+    set_carets(&mut editor, &[(0, 0)], 0);
+    editor.execute("diff.review.edit 1");
+    assert!(editor.message().contains("stale"));
+    editor.select_pane(owner);
+    assert_eq!(editor.cursor(), Cursor::default());
+    assert_eq!(
+        editor.diff.as_ref().unwrap().local_review.document.comments[0].anchor,
+        anchor
+    );
+}
+
 #[test]
 fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
    use crate::review::{Review, Store, Verdict};
@@ -3362,10 +3441,11 @@ fn sqlite_reviews_autosave_restore_select_and_keep_exports_independent() {
    assert_eq!(restored.storage_id, Some(id));
    assert_eq!(restored.comments[0].body, "First feedback revised");
    assert!(
-        !reopened
+        reopened
            .workspace_diff()
            .unwrap()
-            .review_anchor_current(&restored.comments[0].anchor)
+            .review_anchor_range(&restored.comments[0].anchor)
+            .is_none()
    );
    reopened.execute("diff.review.new");
    let second_id = reopened
@@ -3777,7 +3857,7 @@ fn precise_review_ranges_capture_graphemes_and_navigate_to_their_start() {
            };
            assert_eq!(anchor.start, start);
            assert_eq!(anchor.end, end);
-            assert!(diff.review_anchor_current(anchor));
+            assert!(diff.review_anchor_range(anchor).is_some());
            editor.handle(Input::Escape);
            assert_eq!(editor.pane_id(), owner);
            assert_eq!(
@@ -3906,7 +3986,7 @@ fn local_review_visual_commands_and_palette_preserve_both_side_ranges() {
                Side::After
            }
        );
-        assert!(diff.review_anchor_current(&anchor));
+        assert!(diff.review_anchor_range(&anchor).is_some());
        let source_pane = editor.diff_owner.unwrap();
        editor.select_pane(source_pane);
        editor
@@ -7511,15 +7591,7 @@ mod system_clipboard {
            let mut store = Store::open(&storage).unwrap();
            store.save(&mut review).unwrap();
            let id = review.storage_id.unwrap();
-            let expected = format!(
-                "Repository: {}\nReview {id} (revision 1): Review summary\n  \
-                 Verdict: changes_requested; 1 unresolved, 1 resolved (2 total)\n\
-                 \nComment 1 [unresolved] file.txt (before) 0:1–1:1 @ base\n\
-                 e\u{301}👩‍💻\n界\nFirst\nfeedback\n\
-                 \nComment 2 [resolved] other.rs (after) 0:0–0:3 @ stale\n\
-                 old\nResolved feedback\n",
-                root.display(),
-            );
+            let expected = review.format_text(true);
            let mut editor = fixture.editor();
            editor.set_clipboard_mode(mode).unwrap();
            editor.clipboard = vec!["internal".into()];
@@ -67,13 +67,18 @@ fn database_restarts_preserve_identity_and_exact_stale_anchors() {
    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",
-        Path::new("日本語.rs"),
-        Side::Before,
-        &["changed".into()],
-        true,
-    ));
+    assert!(
+        loaded.comments[0]
+            .anchor
+            .mapped_range(
+                "working:new-snapshot",
+                Path::new("日本語.rs"),
+                Side::Before,
+                &["changed".into()],
+                true,
+            )
+            .is_none()
+    );
    let path = root.join("export.json");
    loaded.save(&path).unwrap();
    let exported = fs::read(&path).unwrap();
@@ -387,6 +392,92 @@ fn default_directory_uses_absolute_xdg_then_absolute_home() {
    assert!(resolve(None, Some("")).is_err());
 }

+#[test]
+fn anchor_survives_unrelated_edit() {
+    let anchor = Anchor {
+        revision: "working:old".into(),
+        path: "source.rs".into(),
+        side: Side::After,
+        start: crate::plugin::TextPosition { row: 1, column: 0 },
+        end: crate::plugin::TextPosition { row: 1, column: 6 },
+        content: "before\ntarget\nafter\n".into(),
+    };
+    let map = |text: &str| {
+        anchor.mapped_range(
+            "working:new",
+            Path::new("source.rs"),
+            Side::After,
+            &text
+                .trim_end_matches('\n')
+                .split('\n')
+                .map(str::to_owned)
+                .collect::<Vec<_>>(),
+            text.ends_with('\n'),
+        )
+    };
+    assert_eq!(
+        map("changed\ntarget\nafter\n"),
+        Some(anchor.start..anchor.end)
+    );
+    assert_eq!(
+        map("inserted\nbefore\ntarget\nafter\n"),
+        Some(
+            crate::plugin::TextPosition { row: 2, column: 0 }..crate::plugin::TextPosition {
+                row: 2,
+                column: 6
+            }
+        )
+    );
+    assert!(map("before\ntargEt\nafter\n").is_none());
+    assert!(map("before\nafter\n").is_none());
+    assert!(map("before\ntarget\nafter\nbefore\ntarget\nafter\n").is_none());
+    assert_eq!(map("target\nbefore\ntarget\nafter\n").unwrap().start.row, 2);
+    assert_eq!(anchor.content, "before\ntarget\nafter\n");
+    let partial = Anchor {
+        content: "header\r\nprefix 👩‍💻e\u{301} suffix\r\nfooter\r\n".into(),
+        start: crate::plugin::TextPosition { row: 1, column: 7 },
+        end: crate::plugin::TextPosition { row: 1, column: 9 },
+        ..anchor.clone()
+    };
+    let moved = partial
+        .mapped_range(
+            "working:new",
+            Path::new("source.rs"),
+            Side::After,
+            &[
+                "inserted\r".into(),
+                "header\r".into(),
+                "longer prefix 👩‍💻e\u{301} suffix\r".into(),
+                "footer\r".into(),
+            ],
+            true,
+        )
+        .unwrap();
+    assert_eq!(
+        moved.start,
+        crate::plugin::TextPosition { row: 2, column: 14 }
+    );
+    assert_eq!(
+        moved.end,
+        crate::plugin::TextPosition { row: 2, column: 16 }
+    );
+    assert!(
+        partial
+            .mapped_range(
+                "other-commit",
+                Path::new("source.rs"),
+                Side::After,
+                &[
+                    "header\r".into(),
+                    "prefix 👩‍💻e\u{301} suffix\r".into(),
+                    "footer\r".into()
+                ],
+                true
+            )
+            .is_none()
+    );
+}
+
 #[test]
 fn persistence_validates_anchors_and_preserves_files_on_failure() {
    let directory = directory();
@@ -413,27 +504,39 @@ 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_lines(
-        "working:other",
-        Path::new("source.rs"),
-        Side::After,
-        &["first".into(), "last".into()],
-        true
-    ));
-    assert!(anchor.matches_lines(
-        "working:immutable",
-        Path::new("source.rs"),
-        Side::After,
-        &["first".into(), "last".into()],
-        true
-    ));
-    assert!(!anchor.matches_lines(
-        "working:immutable",
-        Path::new("source.rs"),
-        Side::After,
-        &["first".into(), "last".into()],
-        false
-    ));
+    assert!(
+        anchor
+            .mapped_range(
+                "working:other",
+                Path::new("source.rs"),
+                Side::After,
+                &["first".into(), "last".into()],
+                true
+            )
+            .is_some()
+    );
+    assert!(
+        anchor
+            .mapped_range(
+                "working:immutable",
+                Path::new("source.rs"),
+                Side::After,
+                &["first".into(), "last".into()],
+                true
+            )
+            .is_some()
+    );
+    assert!(
+        anchor
+            .mapped_range(
+                "working:immutable",
+                Path::new("source.rs"),
+                Side::After,
+                &["first".into(), "last".into()],
+                false
+            )
+            .is_none()
+    );
    loaded.summary = "Updated".into();
    loaded.save(&path).unwrap();
    assert_eq!(Review::load(&path, &root).unwrap().summary, "Updated");
@@ -3225,7 +3225,7 @@ fn local_review_markers_reject_stale_identity_text_and_rename_paths() {
            let anchor = &mut diff.local_review.document.comments[0].anchor;
            match mismatch {
                "revision" => anchor.revision.push_str("-newer"),
-                "content" => anchor.content.push('\n'),
+                "content" => anchor.content.replace_range(..1, "X"),
                "path" => anchor.path = "new.txt".into(),
                "side" => anchor.side = crate::review::Side::After,
                "root" => diff.local_review.document.root = "another-repository".into(),