fix: avoid merging unrelated comments into replies
This commit is contained in:
@@ -46,6 +46,8 @@ pub struct ReviewCommentDto {
|
|||||||
pub path: Option<String>,
|
pub path: Option<String>,
|
||||||
pub line: Option<i64>,
|
pub line: Option<i64>,
|
||||||
pub pull_request_review_id: Option<i64>,
|
pub pull_request_review_id: Option<i64>,
|
||||||
|
#[serde(default, alias = "in_reply_to", alias = "in_reply_to_id", alias = "reply_to")]
|
||||||
|
pub in_reply_to: Option<i64>,
|
||||||
pub original_position: Option<u64>,
|
pub original_position: Option<u64>,
|
||||||
pub position: Option<u64>,
|
pub position: Option<u64>,
|
||||||
pub commit_id: Option<String>,
|
pub commit_id: Option<String>,
|
||||||
|
|||||||
+36
-42
@@ -48,59 +48,53 @@ pub fn normalize_bundle(repo: &str, bundle: PullBundleDto) -> PrReviewDocument {
|
|||||||
}
|
}
|
||||||
|
|
||||||
fn normalize_threads(comments: Vec<ReviewCommentDto>) -> Vec<CommentThread> {
|
fn normalize_threads(comments: Vec<ReviewCommentDto>) -> Vec<CommentThread> {
|
||||||
let mut grouped: BTreeMap<String, Vec<ReviewCommentDto>> = BTreeMap::new();
|
let mut all = comments;
|
||||||
|
all.sort_by_key(comment_sort_key);
|
||||||
|
|
||||||
for comment in comments {
|
let comment_ids: std::collections::BTreeSet<i64> = all.iter().map(|comment| comment.id).collect();
|
||||||
let key = thread_key(&comment);
|
let mut roots: Vec<ReviewCommentDto> = Vec::new();
|
||||||
grouped.entry(key).or_default().push(comment);
|
let mut children: BTreeMap<i64, Vec<ReviewCommentDto>> = BTreeMap::new();
|
||||||
|
|
||||||
|
for comment in all {
|
||||||
|
if let Some(parent_id) = comment.in_reply_to
|
||||||
|
&& comment_ids.contains(&parent_id)
|
||||||
|
{
|
||||||
|
children.entry(parent_id).or_default().push(comment);
|
||||||
|
continue;
|
||||||
|
}
|
||||||
|
roots.push(comment);
|
||||||
}
|
}
|
||||||
|
|
||||||
grouped
|
roots
|
||||||
.into_iter()
|
.into_iter()
|
||||||
.map(|(thread_id, mut group)| {
|
.map(|root| {
|
||||||
group.sort_by_key(comment_sort_key);
|
let mut replies = Vec::new();
|
||||||
|
collect_replies(root.id, &mut children, &mut replies);
|
||||||
let root = group.remove(0);
|
|
||||||
let file_path = root
|
|
||||||
.path
|
|
||||||
.clone()
|
|
||||||
.or_else(|| group.iter().find_map(|comment| comment.path.clone()));
|
|
||||||
let line = root
|
|
||||||
.line
|
|
||||||
.or_else(|| group.iter().find_map(|comment| comment.line));
|
|
||||||
|
|
||||||
CommentThread {
|
CommentThread {
|
||||||
thread_id,
|
thread_id: format!("comment-{}", root.id),
|
||||||
file_path,
|
file_path: root.path.clone(),
|
||||||
line,
|
line: root.line,
|
||||||
root_comment: to_comment_item(root),
|
root_comment: to_comment_item(root),
|
||||||
replies: group.into_iter().map(to_comment_item).collect(),
|
replies: replies.into_iter().map(to_comment_item).collect(),
|
||||||
}
|
}
|
||||||
})
|
})
|
||||||
.collect()
|
.collect()
|
||||||
}
|
}
|
||||||
|
|
||||||
fn thread_key(comment: &ReviewCommentDto) -> String {
|
fn collect_replies(
|
||||||
match comment.pull_request_review_id {
|
parent_id: i64,
|
||||||
Some(review_id) => format!(
|
children: &mut BTreeMap<i64, Vec<ReviewCommentDto>>,
|
||||||
"review-{review_id}-{path}-{line}-{position}-{commit}",
|
out: &mut Vec<ReviewCommentDto>,
|
||||||
path = comment.path.as_deref().unwrap_or(""),
|
) {
|
||||||
line = comment
|
let Some(mut direct_replies) = children.remove(&parent_id) else {
|
||||||
.line
|
return;
|
||||||
.map(|value| value.to_string())
|
};
|
||||||
.unwrap_or_default(),
|
direct_replies.sort_by_key(comment_sort_key);
|
||||||
position = comment
|
|
||||||
.original_position
|
for reply in direct_replies {
|
||||||
.or(comment.position)
|
let reply_id = reply.id;
|
||||||
.map(|value| value.to_string())
|
out.push(reply);
|
||||||
.unwrap_or_default(),
|
collect_replies(reply_id, children, out);
|
||||||
commit = comment
|
|
||||||
.original_commit_id
|
|
||||||
.as_deref()
|
|
||||||
.or(comment.commit_id.as_deref())
|
|
||||||
.unwrap_or("")
|
|
||||||
),
|
|
||||||
None => format!("comment-{}", comment.id),
|
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -55,6 +55,7 @@ fn normalize_groups_replies_and_sorts_threads_by_time() {
|
|||||||
path: Some("src/main.rs".into()),
|
path: Some("src/main.rs".into()),
|
||||||
line: Some(10),
|
line: Some(10),
|
||||||
pull_request_review_id: Some(7),
|
pull_request_review_id: Some(7),
|
||||||
|
in_reply_to: Some(1),
|
||||||
original_position: Some(10),
|
original_position: Some(10),
|
||||||
position: Some(10),
|
position: Some(10),
|
||||||
commit_id: Some("abc123".into()),
|
commit_id: Some("abc123".into()),
|
||||||
@@ -72,6 +73,7 @@ fn normalize_groups_replies_and_sorts_threads_by_time() {
|
|||||||
path: Some("src/main.rs".into()),
|
path: Some("src/main.rs".into()),
|
||||||
line: Some(10),
|
line: Some(10),
|
||||||
pull_request_review_id: Some(7),
|
pull_request_review_id: Some(7),
|
||||||
|
in_reply_to: None,
|
||||||
original_position: Some(10),
|
original_position: Some(10),
|
||||||
position: Some(10),
|
position: Some(10),
|
||||||
commit_id: Some("abc123".into()),
|
commit_id: Some("abc123".into()),
|
||||||
@@ -89,6 +91,7 @@ fn normalize_groups_replies_and_sorts_threads_by_time() {
|
|||||||
path: Some("src/lib.rs".into()),
|
path: Some("src/lib.rs".into()),
|
||||||
line: Some(22),
|
line: Some(22),
|
||||||
pull_request_review_id: Some(8),
|
pull_request_review_id: Some(8),
|
||||||
|
in_reply_to: None,
|
||||||
original_position: Some(22),
|
original_position: Some(22),
|
||||||
position: Some(22),
|
position: Some(22),
|
||||||
commit_id: Some("def456".into()),
|
commit_id: Some("def456".into()),
|
||||||
@@ -170,3 +173,79 @@ fn normalize_groups_replies_and_sorts_threads_by_time() {
|
|||||||
assert_eq!(doc.threads[1].file_path.as_deref(), Some("src/main.rs"));
|
assert_eq!(doc.threads[1].file_path.as_deref(), Some("src/main.rs"));
|
||||||
assert_eq!(doc.threads[1].line, Some(10));
|
assert_eq!(doc.threads[1].line, Some(10));
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn normalize_does_not_merge_unrelated_comments_on_same_file() {
|
||||||
|
let bundle = PullBundleDto {
|
||||||
|
pull: PullDto {
|
||||||
|
number: 100,
|
||||||
|
title: "T".into(),
|
||||||
|
state: "open".into(),
|
||||||
|
body: None,
|
||||||
|
user: UserDto {
|
||||||
|
login: "alice".into(),
|
||||||
|
},
|
||||||
|
base: PullBranchDto {
|
||||||
|
ref_name: "main".into(),
|
||||||
|
},
|
||||||
|
head: PullBranchDto {
|
||||||
|
ref_name: "feature/y".into(),
|
||||||
|
},
|
||||||
|
created_at: "2026-04-08T10:00:00Z".into(),
|
||||||
|
updated_at: "2026-04-08T10:00:00Z".into(),
|
||||||
|
merged_at: None,
|
||||||
|
additions: None,
|
||||||
|
deletions: None,
|
||||||
|
changed_files: None,
|
||||||
|
},
|
||||||
|
reviews: vec![],
|
||||||
|
comments: vec![
|
||||||
|
ReviewCommentDto {
|
||||||
|
id: 10,
|
||||||
|
body: "first line comment".into(),
|
||||||
|
created_at: "2026-04-08T12:01:00Z".into(),
|
||||||
|
updated_at: None,
|
||||||
|
user: UserDto {
|
||||||
|
login: "bob".into(),
|
||||||
|
},
|
||||||
|
path: Some("src/main.rs".into()),
|
||||||
|
line: Some(10),
|
||||||
|
pull_request_review_id: Some(1),
|
||||||
|
in_reply_to: None,
|
||||||
|
original_position: Some(10),
|
||||||
|
position: Some(10),
|
||||||
|
commit_id: Some("abc123".into()),
|
||||||
|
original_commit_id: Some("abc123".into()),
|
||||||
|
diff_hunk: None,
|
||||||
|
},
|
||||||
|
ReviewCommentDto {
|
||||||
|
id: 11,
|
||||||
|
body: "second line comment".into(),
|
||||||
|
created_at: "2026-04-08T12:02:00Z".into(),
|
||||||
|
updated_at: None,
|
||||||
|
user: UserDto {
|
||||||
|
login: "bob".into(),
|
||||||
|
},
|
||||||
|
path: Some("src/main.rs".into()),
|
||||||
|
line: Some(20),
|
||||||
|
pull_request_review_id: Some(1),
|
||||||
|
in_reply_to: None,
|
||||||
|
original_position: Some(20),
|
||||||
|
position: Some(20),
|
||||||
|
commit_id: Some("abc123".into()),
|
||||||
|
original_commit_id: Some("abc123".into()),
|
||||||
|
diff_hunk: None,
|
||||||
|
},
|
||||||
|
],
|
||||||
|
commits: vec![],
|
||||||
|
files: vec![],
|
||||||
|
};
|
||||||
|
|
||||||
|
let doc = normalize_bundle("org/repo", bundle);
|
||||||
|
|
||||||
|
assert_eq!(doc.threads.len(), 2);
|
||||||
|
assert_eq!(doc.threads[0].root_comment.id, 10);
|
||||||
|
assert_eq!(doc.threads[0].replies.len(), 0);
|
||||||
|
assert_eq!(doc.threads[1].root_comment.id, 11);
|
||||||
|
assert_eq!(doc.threads[1].replies.len(), 0);
|
||||||
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user