Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions AGENTS.md
Original file line number Diff line number Diff line change
Expand Up @@ -240,6 +240,8 @@ These are non-obvious things the implementation chain hit. Worth preserving for

12. **`cd` into the worktree before running `cargo`.** `cargo` resolves `Cargo.toml` from `pwd`. Running gates from the wrong worktree silently exercises the wrong tree.

13. **Comments are commit-scoped via `Comment::commit_id`.** When the inline commit selector shows exactly one commit, `App::save_comment` stamps that commit's SHA on the comment. Comments with `commit_id = Some(sha)` are hidden when a different commit (or a subset not containing `sha`) is selected; `commit_id = None` (legacy, review-level, or made against the full cumulative diff) is always visible. The filter runs in `rebuild_annotations`, both diff renderers, the comment navigator (via filtered annotations), and the submit preflight. `App::comment_visible(&Comment)` is the single predicate. `AnnotatedLine::LineComment`/`FileComment` `comment_idx` is the **absolute** index into the stored `Vec`/`HashMap` value — `delete_comment_at_cursor` and `enter_edit_mode` must look it up directly, not re-count by side.

### Keeping Docs Updated

When adding user-facing features, update the relevant documentation:
Expand Down
6 changes: 6 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@

All notable changes to this project will be documented in this file.

## [Unreleased]

### Bug Fixes

- **comments:** Associate comments with their commit when reviewing per-commit, preventing comments from one commit bleeding into another commit's diff view

## [0.18.0] - 2026-06-20

### Bug Fixes
Expand Down
593 changes: 546 additions & 47 deletions src/app.rs

Large diffs are not rendered by default.

20 changes: 20 additions & 0 deletions src/model/comment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -172,6 +172,15 @@ pub struct Comment {
/// inline comments; review-level / summary comments don't get one.
#[serde(default)]
pub remote_comment_id: Option<String>,
/// The commit SHA this comment was made against, when it was created
/// while the inline commit selector showed exactly one commit. `None`
/// for review-level comments, for comments made against the full
/// cumulative diff (all commits selected), and for legacy session JSON
/// predating this field. Filtering by the active commit selection hides
/// comments whose `commit_id` does not intersect the selection; `None`
/// comments are always shown.
#[serde(default)]
pub commit_id: Option<String>,
}

impl Comment {
Expand All @@ -188,6 +197,7 @@ impl Comment {
lifecycle_state: CommentLifecycleState::default(),
remote_review_id: None,
remote_comment_id: None,
commit_id: None,
}
}

Expand All @@ -210,6 +220,7 @@ impl Comment {
lifecycle_state: CommentLifecycleState::default(),
remote_review_id: None,
remote_comment_id: None,
commit_id: None,
}
}

Expand All @@ -221,6 +232,15 @@ impl Comment {
self
}

/// Builder: set `commit_id` and return self. Called by
/// `App::save_comment` when the inline commit selector shows exactly
/// one commit, so the comment is scoped to that commit and hidden when
/// a different commit (or subset) is selected.
pub fn with_commit_id(mut self, commit_id: impl Into<String>) -> Self {
self.commit_id = Some(commit_id.into());
self
}

/// True if this comment has been pushed/submitted to the forge and is
/// therefore locked from local edits/deletions.
pub fn is_locked(&self) -> bool {
Expand Down
69 changes: 69 additions & 0 deletions src/model/review.rs
Original file line number Diff line number Diff line change
Expand Up @@ -540,6 +540,75 @@ mod tests {
assert_eq!(restored.commit_selection_range, Some((1, 3)));
}

#[test]
fn should_round_trip_comment_commit_id_on_session() {
// given a session with a file comment and a line comment both scoped
// to a single commit
use crate::model::comment::LineSide;
let mut session = test_session();
session.add_file(PathBuf::from("src/lib.rs"), FileStatus::Modified, SOME_HASH);
let file_comment = Comment::new(
"file note on commit aaa".to_string(),
CommentType::Note,
None,
)
.with_commit_id("aaa111");
let line_comment = Comment::new(
"line note on commit bbb".to_string(),
CommentType::Issue,
Some(LineSide::New),
)
.with_commit_id("bbb222");
let review = session.get_file_mut(&PathBuf::from("src/lib.rs")).unwrap();
review.add_file_comment(file_comment.clone());
review.add_line_comment(42, line_comment.clone());

// when serialized and restored
let json = serde_json::to_string(&session).unwrap();
let restored: ReviewSession = serde_json::from_str(&json).unwrap();

// then the commit_id survives the round trip
let r = restored.files.get(&PathBuf::from("src/lib.rs")).unwrap();
assert_eq!(r.file_comments.len(), 1);
assert_eq!(
r.file_comments[0].commit_id,
Some("aaa111".to_string()),
"file comment commit_id must round-trip"
);
let line = r.line_comments.get(&42).unwrap();
assert_eq!(line.len(), 1);
assert_eq!(
line[0].commit_id,
Some("bbb222".to_string()),
"line comment commit_id must round-trip"
);
}

#[test]
fn should_default_commit_id_to_none_for_legacy_comment_json() {
// given a comment JSON saved before commit_id existed
let json = r#"{
"id": "legacy-id",
"content": "old note",
"comment_type": "note",
"created_at": "2024-01-01T00:00:00Z",
"line_context": null,
"side": null,
"line_range": null,
"author": "user",
"lifecycle_state": "local_draft",
"remote_review_id": null,
"remote_comment_id": null
}"#;
// when
let comment: Comment = serde_json::from_str(json).unwrap();
// then
assert_eq!(
comment.commit_id, None,
"comment JSON without commit_id must default to None"
);
}

#[test]
fn should_default_commit_selection_range_to_none_for_legacy_session() {
// given a session JSON saved before commit_selection_range existed
Expand Down
79 changes: 74 additions & 5 deletions src/output/markdown.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,9 +11,16 @@ use crate::forge::remote_comments::{
PrCommentsVisibility, RemoteReviewThread, filter_threads, group_threads_by_path,
};
use crate::model::{CommentType, LineRange, LineSide, ReviewSession};

/// (file_path, line_range, side, comment_type, content)
type CommentEntry<'a> = (String, Option<LineRange>, Option<LineSide>, String, &'a str);
use crate::slug::short_sha;
/// (file_path, line_range, side, comment_type, content, commit_id)
type CommentEntry<'a> = (
String,
Option<LineRange>,
Option<LineSide>,
String,
&'a str,
Option<&'a str>,
);

/// Generate markdown content from the review session.
/// Returns the markdown string or an error if there are no comments.
Expand Down Expand Up @@ -346,6 +353,7 @@ fn generate_markdown(
None,
export_comment_type_label(&comment.comment_type, comment_types),
&comment.content,
None,
));
}

Expand All @@ -364,6 +372,7 @@ fn generate_markdown(
None,
export_comment_type_label(&comment.comment_type, comment_types),
&comment.content,
comment.commit_id.as_deref(),
));
}

Expand All @@ -383,6 +392,7 @@ fn generate_markdown(
comment.side,
export_comment_type_label(&comment.comment_type, comment_types),
&comment.content,
comment.commit_id.as_deref(),
));
}
}
Expand All @@ -395,7 +405,9 @@ fn generate_markdown(
let _ = writeln!(md);
local_section_written = true;
}
for (i, (file, line_range, side, comment_type, content)) in all_comments.iter().enumerate() {
for (i, (file, line_range, side, comment_type, content, commit_id)) in
all_comments.iter().enumerate()
{
let number = i + 1;
let location = match (line_range, side) {
// Range on deleted side (old lines)
Expand All @@ -415,13 +427,19 @@ fn generate_markdown(
// File comment
(None, _) => format!("`{file}`"),
};
// Append the commit short SHA so the LLM knows which commit the
// comment was made against — crucial for per-commit reviews.
let commit_suffix = match commit_id {
Some(sha) => format!(" (commit {})", short_sha(sha)),
None => String::new(),
};
let marker = format!("{number}.");
let continuation_indent = " ".repeat(marker.len() + 1);
let mut content_lines = content.split('\n').map(|line| line.trim_end_matches('\r'));
let first_line = content_lines.next().unwrap_or_default();
let _ = writeln!(
md,
"{marker} **[{comment_type}]** {location} - {first_line}"
"{marker} **[{comment_type}]** {location}{commit_suffix} - {first_line}"
);
for line in content_lines {
let _ = writeln!(md, "{continuation_indent}{line}");
Expand Down Expand Up @@ -854,6 +872,57 @@ mod tests {
));
}

#[test]
fn should_include_commit_sha_in_export_for_commit_scoped_comments() {
let mut session = create_test_session();
// Add a line comment scoped to commit abc1234567890
if let Some(review) = session.get_file_mut(&PathBuf::from("src/main.rs")) {
review.add_line_comment(
10,
Comment::new(
"Wrong variable name".to_string(),
CommentType::Issue,
Some(LineSide::New),
)
.with_commit_id("abc1234567890"),
);
}

let markdown = generate_markdown(
&session,
&DiffSource::CommitRange(vec!["abc1234567890".to_string()]),
&comment_types(),
true,
&[],
None,
);

assert!(
markdown.contains("`src/main.rs:10` (commit abc1234) - Wrong variable name"),
"commit-scoped comment must include the short SHA in the export; got:\n{markdown}"
);
}

#[test]
fn should_omit_commit_suffix_for_unscoped_comments() {
let session = create_test_session();

let markdown = generate_markdown(
&session,
&DiffSource::WorkingTree,
&comment_types(),
true,
&[],
None,
);

// Existing comments have commit_id = None — no (commit ...) suffix
assert!(
!markdown.contains("(commit "),
"unscoped comments must not have a commit suffix; got:\n{markdown}"
);
}

#[test]
fn should_fail_export_when_no_comments() {
// given
Expand Down
2 changes: 2 additions & 0 deletions src/review_cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ fn add_comment(
content: request_parts.content,
comment_type,
author,
commit_id: None,
},
)?;
let output = CommentOutput::from_target(&target, &comment);
Expand Down Expand Up @@ -846,6 +847,7 @@ mod tests {
content: "check this".to_string(),
comment_type: CommentType::Issue,
author: crate::model::comment::DEFAULT_AUTHOR.to_string(),
commit_id: None,
},
)
.unwrap();
Expand Down
29 changes: 25 additions & 4 deletions src/review_store.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,6 +180,11 @@ pub struct AddCommentRequest {
/// picking a sensible default (`Comment::DEFAULT_AUTHOR`) when none is
/// supplied.
pub author: String,
/// Commit SHA to stamp on the comment when it was created while the
/// inline commit selector showed exactly one commit. `None` for
/// review-level comments and full-range selections. Library callers
/// (the `review add` CLI) leave this `None`.
pub commit_id: Option<String>,
}

/// Where a new local draft comment should be attached.
Expand Down Expand Up @@ -216,6 +221,7 @@ pub fn add_comment_to_session(
}

let author = request.author;
let commit_id = request.commit_id;
let comment = match request.target {
CommentTarget::Review => {
let comment = Comment::new(content, request.comment_type, None).with_author(author);
Expand All @@ -224,21 +230,31 @@ pub fn add_comment_to_session(
}
CommentTarget::File { path } => {
let review = file_review_mut(session, &path)?;
let comment = Comment::new(content, request.comment_type, None).with_author(author);
let mut comment = Comment::new(content, request.comment_type, None).with_author(author);
if let Some(sha) = &commit_id {
comment = comment.with_commit_id(sha.clone());
}
review.add_file_comment(comment.clone());
comment
}
CommentTarget::Line { path, line, side } => {
let review = file_review_mut(session, &path)?;
let comment =
let mut comment =
Comment::new(content, request.comment_type, Some(side)).with_author(author);
if let Some(sha) = &commit_id {
comment = comment.with_commit_id(sha.clone());
}
review.add_line_comment(line, comment.clone());
comment
}
CommentTarget::LineRange { path, range, side } => {
let review = file_review_mut(session, &path)?;
let comment = Comment::new_with_range(content, request.comment_type, Some(side), range)
.with_author(author);
let mut comment =
Comment::new_with_range(content, request.comment_type, Some(side), range)
.with_author(author);
if let Some(sha) = &commit_id {
comment = comment.with_commit_id(sha.clone());
}
review.add_line_comment(range.end, comment.clone());
comment
}
Expand Down Expand Up @@ -284,6 +300,7 @@ mod tests {
content: "looks good".to_string(),
comment_type: CommentType::Praise,
author: crate::model::comment::DEFAULT_AUTHOR.to_string(),
commit_id: None,
},
)
.unwrap();
Expand All @@ -304,6 +321,7 @@ mod tests {
content: "file note".to_string(),
comment_type: CommentType::Note,
author: crate::model::comment::DEFAULT_AUTHOR.to_string(),
commit_id: None,
},
)
.unwrap();
Expand All @@ -328,6 +346,7 @@ mod tests {
content: "range note".to_string(),
comment_type: CommentType::Suggestion,
author: crate::model::comment::DEFAULT_AUTHOR.to_string(),
commit_id: None,
},
)
.unwrap();
Expand All @@ -349,6 +368,7 @@ mod tests {
content: "note".to_string(),
comment_type: CommentType::Note,
author: crate::model::comment::DEFAULT_AUTHOR.to_string(),
commit_id: None,
},
)
.unwrap_err();
Expand Down Expand Up @@ -394,6 +414,7 @@ mod tests {
content: "line note".to_string(),
comment_type: CommentType::Note,
author: crate::model::comment::DEFAULT_AUTHOR.to_string(),
commit_id: None,
},
)
.unwrap();
Expand Down
Loading
Loading