From 03245b8b8f518b2db71fb6ae1369147362f509f3 Mon Sep 17 00:00:00 2001 From: Thibault Saunier Date: Thu, 2 Jul 2026 15:19:56 -0400 Subject: [PATCH] fix(comments): associate comments with their commit when reviewing per-commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comments had no commit association — FileReview.line_comments was keyed only by (path, line). When reviewing commit-by-commit via the inline selector, a comment made on commit A's line 42 showed on commit B's line 42 even when the content was entirely different, causing a total mixup across the diff view, comment navigator, and submit preflight. Add a commit_id: Option field to Comment (serde #[default], so legacy session JSON rehydrates as None = always visible). The field records the SHA of the commit a comment was made against, stamped only when the inline selector shows exactly one commit. Visibility rule (App::comment_visible): a comment is visible if commit_id is None (legacy / review-level / full-range) OR its SHA is in the currently-selected commit set. Filtering is applied in every read path: rebuild_annotations, both diff renderers, the comment navigator (via filtered annotations), and the submit preflight. Also fixes a latent bug: delete_comment_at_cursor and enter_edit_mode re-derived the comment index by counting side-matching comments, but AnnotatedLine::LineComment.comment_idx is the absolute index into the stored Vec. Now both use the absolute index directly. --- AGENTS.md | 2 + CHANGELOG.md | 6 + src/app.rs | 593 +++++++++++++++++++++++++++++++++--- src/model/comment.rs | 20 ++ src/model/review.rs | 69 +++++ src/output/markdown.rs | 79 ++++- src/review_cli.rs | 2 + src/review_store.rs | 29 +- src/ui/diff_side_by_side.rs | 8 +- src/ui/diff_unified.rs | 11 +- 10 files changed, 759 insertions(+), 60 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 97cf0700..aecc0f01 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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: diff --git a/CHANGELOG.md b/CHANGELOG.md index 6a6bb66a..c1e90172 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -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 diff --git a/src/app.rs b/src/app.rs index c21cee0d..66bb7644 100644 --- a/src/app.rs +++ b/src/app.rs @@ -6135,8 +6135,16 @@ impl App { map }; + // Commit-selection filter — must mirror rebuild_annotations and + // both renderers exactly, or total_lines() disagrees with + // line_annotations.len() and scroll/cursor math drifts. + let commit_set = self.selected_commit_set(); + if let Some(review) = self.session.files.get(path) { for comment in &review.file_comments { + if !Self::comment_visible_with(comment, commit_set.as_ref()) { + continue; + } comment_lines += Self::comment_display_lines(comment, self.diff_state.viewport_width); } @@ -6186,7 +6194,12 @@ impl App { && let Some(comments) = line_comments.get(&old_ln) { for comment in comments { - if comment.side == Some(LineSide::Old) { + if comment.side == Some(LineSide::Old) + && Self::comment_visible_with( + comment, + commit_set.as_ref(), + ) + { comment_lines += Self::comment_display_lines( comment, self.diff_state.viewport_width, @@ -6199,7 +6212,12 @@ impl App { && let Some(comments) = line_comments.get(&new_ln) { for comment in comments { - if comment.side != Some(LineSide::Old) { + if comment.side != Some(LineSide::Old) + && Self::comment_visible_with( + comment, + commit_set.as_ref(), + ) + { comment_lines += Self::comment_display_lines( comment, self.diff_state.viewport_width, @@ -6241,7 +6259,12 @@ impl App { && let Some(comments) = line_comments.get(&new_ln) { for comment in comments { - if comment.side != Some(LineSide::Old) { + if comment.side != Some(LineSide::Old) + && Self::comment_visible_with( + comment, + commit_set.as_ref(), + ) + { comment_lines += Self::comment_display_lines( comment, self.diff_state.viewport_width, @@ -6288,7 +6311,12 @@ impl App { && let Some(comments) = line_comments.get(&old_ln) { for comment in comments { - if comment.side == Some(LineSide::Old) { + if comment.side == Some(LineSide::Old) + && Self::comment_visible_with( + comment, + commit_set.as_ref(), + ) + { comment_lines += Self::comment_display_lines( comment, @@ -6304,7 +6332,12 @@ impl App { && let Some(comments) = line_comments.get(&new_ln) { for comment in comments { - if comment.side != Some(LineSide::Old) { + if comment.side != Some(LineSide::Old) + && Self::comment_visible_with( + comment, + commit_set.as_ref(), + ) + { comment_lines += Self::comment_display_lines( comment, @@ -6344,7 +6377,12 @@ impl App { && let Some(comments) = line_comments.get(&new_ln) { for comment in comments { - if comment.side != Some(LineSide::Old) { + if comment.side != Some(LineSide::Old) + && Self::comment_visible_with( + comment, + commit_set.as_ref(), + ) + { comment_lines += Self::comment_display_lines( comment, self.diff_state.viewport_width, @@ -6581,6 +6619,7 @@ impl App { fn find_comment_at_cursor(&self) -> Option { let target = self.diff_state.cursor_line; + let commit_set = self.selected_commit_set(); match self.line_annotations.get(target) { Some(AnnotatedLine::ReviewComment { comment_idx }) => Some(CommentLocation::Review { index: *comment_idx, @@ -6590,6 +6629,18 @@ impl App { comment_idx, }) => { let path = self.diff_files.get(*file_idx)?.display_path().clone(); + // Guard against stale annotations from an async commit-selection + // reload: if the comment is no longer visible under the current + // selection, treat the cursor as not on a comment. + let visible = self + .session + .files + .get(&path) + .and_then(|r| r.file_comments.get(*comment_idx)) + .is_some_and(|c| Self::comment_visible_with(c, commit_set.as_ref())); + if !visible { + return None; + } Some(CommentLocation::File { path, index: *comment_idx, @@ -6602,6 +6653,16 @@ impl App { comment_idx, }) => { let path = self.diff_files.get(*file_idx)?.display_path().clone(); + let visible = self + .session + .files + .get(&path) + .and_then(|r| r.line_comments.get(line)) + .and_then(|c| c.get(*comment_idx)) + .is_some_and(|c| Self::comment_visible_with(c, commit_set.as_ref())); + if !visible { + return None; + } Some(CommentLocation::Line { path, line: *line, @@ -6646,28 +6707,21 @@ impl App { if let Some(review) = self.session.get_file_mut(&path) && let Some(comments) = review.line_comments.get_mut(&line) { - // Find the actual index by counting comments with matching side - let mut side_idx = 0; - let mut actual_idx = None; - for (i, comment) in comments.iter().enumerate() { - let comment_side = comment.side.unwrap_or(LineSide::New); + // `comment_idx` from the annotation is the absolute index + // into the stored Vec (see `push_comments`), so delete + // directly — no side-filtered re-count. + if index < comments.len() { + let comment_side = comments[index].side.unwrap_or(LineSide::New); if comment_side == side { - if side_idx == index { - actual_idx = Some(i); - break; + comments.remove(index); + if comments.is_empty() { + review.line_comments.remove(&line); } - side_idx += 1; - } - } - if let Some(idx) = actual_idx { - comments.remove(idx); - if comments.is_empty() { - review.line_comments.remove(&line); + self.dirty = true; + self.set_message(format!("Comment on line {line} deleted")); + self.rebuild_annotations(); + return true; } - self.dirty = true; - self.set_message(format!("Comment on line {line} deleted")); - self.rebuild_annotations(); - return true; } } } @@ -6840,26 +6894,23 @@ impl App { if let Some(review) = self.session.files.get(&path) && let Some(comments) = review.line_comments.get(&line) { - // Find the actual comment by counting comments with matching side - let mut side_idx = 0; - for comment in comments.iter() { - let comment_side = comment.side.unwrap_or(LineSide::New); - if comment_side == side { - if side_idx == index { - self.input_mode = InputMode::Comment; - self.diff_state.scroll_x = 0; - self.comment_buffer = comment.content.clone(); - self.comment_cursor = - self.comment_current_line_cursor(block_start, cursor_at_end); - self.comment_type = comment.comment_type.clone(); - self.comment_is_review_level = false; - self.comment_is_file_level = false; - self.comment_line = Some((line, side)); - self.editing_comment_id = Some(comment.id.clone()); - return true; - } - side_idx += 1; - } + // `comment_idx` from the annotation is the absolute index + // into the stored Vec (see `push_comments`); look it up + // directly, verifying the side matches. + if let Some(comment) = comments.get(index) + && comment.side.unwrap_or(LineSide::New) == side + { + self.input_mode = InputMode::Comment; + self.diff_state.scroll_x = 0; + self.comment_buffer = comment.content.clone(); + self.comment_cursor = + self.comment_current_line_cursor(block_start, cursor_at_end); + self.comment_type = comment.comment_type.clone(); + self.comment_is_review_level = false; + self.comment_is_file_level = false; + self.comment_line = Some((line, side)); + self.editing_comment_id = Some(comment.id.clone()); + return true; } } } @@ -7276,6 +7327,7 @@ impl App { content, comment_type: self.comment_type.clone(), author: self.username.clone(), + commit_id: None, }; message = match add_comment_to_session(&mut self.session, request) { Ok(_) => "Review comment added".to_string(), @@ -7311,6 +7363,7 @@ impl App { content, comment_type: self.comment_type.clone(), author: self.username.clone(), + commit_id: self.commit_id_for_new_comment(), }; message = match add_comment_to_session(&mut self.session, request) { Ok(_) => success_message, @@ -7496,7 +7549,7 @@ impl App { continue; }; for comment in &review.file_comments { - if comment.is_locked() { + if comment.is_locked() || !self.comment_visible(comment) { continue; } total_local_drafts += 1; @@ -7510,7 +7563,7 @@ impl App { keys.sort(); for key in keys { for comment in &review.line_comments[key] { - if comment.is_locked() { + if comment.is_locked() || !self.comment_visible(comment) { continue; } total_local_drafts += 1; @@ -8886,6 +8939,70 @@ impl App { } } + /// The set of commit SHAs currently selected in the inline commit + /// selector. `None` when there is no selector / no selection (working + /// tree, staged, etc.) — in that case every comment is visible. When + /// the full range is selected the set contains every commit SHA, so + /// all comments (including single-commit-scoped ones) show. When a + /// strict subset is selected, only comments whose `commit_id` is in + /// the set (or `None`) are visible. + fn selected_commit_set(&self) -> Option> { + let (start, end) = self.commit_selection_range?; + if start > end || self.review_commits.is_empty() { + return Some(std::collections::HashSet::new()); + } + let end = end.min(self.review_commits.len().saturating_sub(1)); + let set: std::collections::HashSet = (start..=end) + .filter_map(|i| self.review_commits.get(i)) + .filter(|c| !Self::is_special_commit(c)) + .map(|c| c.id.clone()) + .collect(); + Some(set) + } + + /// Whether a comment should be visible given the current commit + /// selection. A comment with `commit_id == None` (legacy or made + /// against the full cumulative diff) is always visible. Otherwise it + /// is visible only when its commit is in the selected set. + /// + /// Allocates the commit set on every call. Callers in hot paths + /// (per-comment-per-frame renderers, height calculation) should + /// compute [`selected_commit_set`] once and use + /// [`comment_visible_with`] instead. + pub fn comment_visible(&self, comment: &crate::model::Comment) -> bool { + Self::comment_visible_with(comment, self.selected_commit_set().as_ref()) + } + + /// Pure visibility check against a precomputed commit set — no + /// allocation. `set == None` means "no selector", so every comment + /// is visible. This is the shared predicate all filtering sites + /// should converge on so height math and rendering never drift. + pub fn comment_visible_with( + comment: &crate::model::Comment, + commit_set: Option<&std::collections::HashSet>, + ) -> bool { + match (&comment.commit_id, commit_set) { + (None, _) => true, + (Some(_), None) => true, + (Some(sha), Some(set)) => set.contains(sha), + } + } + + /// The single commit SHA to stamp on a *new* comment, when the inline + /// selector shows exactly one commit. `None` otherwise (full range, + /// multi-commit subset, or no selector) — those comments get + /// `commit_id = None` so they stay visible across selections. + fn commit_id_for_new_comment(&self) -> Option { + let (start, end) = self.commit_selection_range?; + if start != end { + return None; + } + self.review_commits + .get(start) + .filter(|c| !Self::is_special_commit(c)) + .map(|c| c.id.clone()) + } + fn set_pr_last_reviewed_commit_from_metadata( &mut self, commits: &[crate::forge::traits::PullRequestCommit], @@ -9703,6 +9820,9 @@ impl App { // suppressed don't appear in this map at all, so no annotations // are emitted for them. let remote_index = self.build_remote_thread_index(); + // Commit-selection filter: comments scoped to a commit outside the + // current inline selection are hidden. `None` => no selector, show all. + let commit_set = self.selected_commit_set(); // The review-comments header is omitted in single-file view (see // the matching guard in `src/ui/diff_unified.rs`), so the @@ -9773,6 +9893,9 @@ impl App { // File comments if let Some(review) = self.session.files.get(path) { for (comment_idx, comment) in review.file_comments.iter().enumerate() { + if !Self::comment_visible_with(comment, commit_set.as_ref()) { + continue; + } let comment_lines = Self::comment_display_lines(comment, self.diff_state.viewport_width); for _ in 0..comment_lines { @@ -9896,6 +10019,7 @@ impl App { &self.forge_review_threads, &remote_index, self.diff_state.viewport_width, + commit_set.as_ref(), ); } DiffViewMode::SideBySide => { @@ -9909,6 +10033,7 @@ impl App { &self.forge_review_threads, &remote_index, self.diff_state.viewport_width, + commit_set.as_ref(), ); } } @@ -9981,6 +10106,7 @@ impl App { line_comments: &std::collections::HashMap>, side: LineSide, viewport_width: usize, + commit_set: Option<&std::collections::HashSet>, ) { let Some(ln) = line_no else { return; @@ -9998,6 +10124,12 @@ impl App { continue; } + // Hide comments scoped to a commit outside the current selection. + // Uses the shared predicate so height math and rendering agree. + if !Self::comment_visible_with(comment, commit_set) { + continue; + } + let comment_lines = Self::comment_display_lines(comment, viewport_width); for _ in 0..comment_lines { annotations.push(AnnotatedLine::LineComment { @@ -10079,6 +10211,7 @@ impl App { remote_threads: &[crate::forge::remote_comments::RemoteReviewThread], remote_index: &RemoteThreadIndex, viewport_width: usize, + commit_set: Option<&std::collections::HashSet>, ) { for (line_idx, diff_line) in lines.iter().enumerate() { annotations.push(AnnotatedLine::DiffLine { @@ -10098,6 +10231,7 @@ impl App { line_comments, LineSide::Old, viewport_width, + commit_set, ); Self::push_remote_threads( annotations, @@ -10118,6 +10252,7 @@ impl App { line_comments, LineSide::New, viewport_width, + commit_set, ); Self::push_remote_threads( annotations, @@ -10143,6 +10278,7 @@ impl App { remote_threads: &[crate::forge::remote_comments::RemoteReviewThread], remote_index: &RemoteThreadIndex, viewport_width: usize, + commit_set: Option<&std::collections::HashSet>, ) { let mut i = 0; while i < lines.len() { @@ -10166,6 +10302,7 @@ impl App { line_comments, LineSide::New, viewport_width, + commit_set, ); if let Some(new_ln) = diff_line.new_lineno { Self::push_remote_threads( @@ -10231,6 +10368,7 @@ impl App { line_comments, LineSide::Old, viewport_width, + commit_set, ); if let Some(old_ln) = old_lineno { Self::push_remote_threads( @@ -10249,6 +10387,7 @@ impl App { line_comments, LineSide::New, viewport_width, + commit_set, ); if let Some(new_ln) = new_lineno { Self::push_remote_threads( @@ -10281,6 +10420,7 @@ impl App { line_comments, LineSide::New, viewport_width, + commit_set, ); if let Some(new_ln) = diff_line.new_lineno { Self::push_remote_threads( @@ -10849,6 +10989,311 @@ mod commit_selection_tests { } } +#[cfg(test)] +mod commit_scoped_comment_tests { + use super::*; + use crate::model::{CommentType, LineSide}; + use crate::vcs::traits::{CommitInfo, VcsType}; + + struct DummyVcs { + info: VcsInfo, + } + + impl VcsBackend for DummyVcs { + fn info(&self) -> &VcsInfo { + &self.info + } + fn get_working_tree_diff(&self, _highlighter: &SyntaxHighlighter) -> Result> { + Err(TuicrError::NoChanges) + } + fn fetch_context_lines( + &self, + _file_path: &Path, + _file_status: crate::model::FileStatus, + _ref_commit: Option<&str>, + _start_line: u32, + _end_line: u32, + ) -> Result> { + Ok(Vec::new()) + } + fn file_line_count( + &self, + _file_path: &Path, + _file_status: crate::model::FileStatus, + _ref_commit: Option<&str>, + ) -> Result { + Ok(0) + } + } + + fn build_app_with_review_commits(commits: Vec) -> App { + let vcs_info = VcsInfo { + root_path: PathBuf::from("/tmp"), + head_commit: "head".to_string(), + branch_name: Some("main".to_string()), + vcs_type: VcsType::Git, + }; + let session = ReviewSession::new( + vcs_info.root_path.clone(), + vcs_info.head_commit.clone(), + vcs_info.branch_name.clone(), + SessionDiffSource::WorkingTree, + ); + let mut app = App::build( + Box::new(DummyVcs { + info: vcs_info.clone(), + }), + vcs_info, + Theme::dark(), + None, + false, + Vec::new(), + session, + DiffSource::WorkingTree, + InputMode::Normal, + Vec::new(), + None, + None, + ) + .expect("failed to build test app"); + app.review_commits = commits; + app + } + + fn commit(id: &str) -> CommitInfo { + CommitInfo { + id: id.to_string(), + short_id: id.to_string(), + branch_name: None, + summary: format!("commit {id}"), + body: None, + author: "tester".to_string(), + time: Utc::now(), + } + } + + fn line_comment(content: &str, commit_id: Option<&str>) -> crate::model::Comment { + let mut c = + crate::model::Comment::new(content.to_string(), CommentType::Note, Some(LineSide::New)); + c.commit_id = commit_id.map(|s| s.to_string()); + c + } + + #[test] + fn legacy_comment_with_no_commit_id_is_always_visible() { + let mut app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + app.commit_selection_range = Some((0, 0)); // only "aaa" selected + let comment = line_comment("old comment", None); + assert!( + app.comment_visible(&comment), + "legacy comment with commit_id=None must be visible regardless of selection" + ); + } + + #[test] + fn comment_scoped_to_selected_commit_is_visible() { + let mut app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + app.commit_selection_range = Some((0, 0)); // only "aaa" selected + let comment = line_comment("on aaa", Some("aaa")); + assert!( + app.comment_visible(&comment), + "comment scoped to the selected commit must be visible" + ); + } + + #[test] + fn comment_scoped_to_unselected_commit_is_hidden() { + let mut app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + app.commit_selection_range = Some((0, 0)); // only "aaa" selected + let comment = line_comment("on bbb", Some("bbb")); + assert!( + !app.comment_visible(&comment), + "comment scoped to a commit outside the selection must be hidden" + ); + } + + #[test] + fn full_range_shows_all_commit_scoped_comments() { + let mut app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + app.commit_selection_range = Some((0, 1)); // full range + assert!( + app.comment_visible(&line_comment("on aaa", Some("aaa"))), + "full range includes commit aaa" + ); + assert!( + app.comment_visible(&line_comment("on bbb", Some("bbb"))), + "full range includes commit bbb" + ); + } + + #[test] + fn no_selector_shows_all_comments() { + let app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + // commit_selection_range is None by default + assert!( + app.comment_visible(&line_comment("on aaa", Some("aaa"))), + "no selector => all comments visible" + ); + assert!( + app.comment_visible(&line_comment("on bbb", Some("bbb"))), + "no selector => all comments visible" + ); + } + + #[test] + fn commit_id_for_new_comment_is_none_without_selector() { + let app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + assert_eq!( + app.commit_id_for_new_comment(), + None, + "no selector => no commit_id stamp" + ); + } + + #[test] + fn commit_id_for_new_comment_is_none_for_full_range() { + let mut app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + app.commit_selection_range = Some((0, 1)); + assert_eq!( + app.commit_id_for_new_comment(), + None, + "full range => no commit_id stamp (comment is against cumulative diff)" + ); + } + + #[test] + fn commit_id_for_new_comment_is_none_for_multi_commit_subset() { + let mut app = + build_app_with_review_commits(vec![commit("aaa"), commit("bbb"), commit("ccc")]); + app.commit_selection_range = Some((0, 1)); // 2 of 3 commits + assert_eq!( + app.commit_id_for_new_comment(), + None, + "multi-commit subset => no commit_id stamp" + ); + } + + #[test] + fn commit_id_for_new_comment_is_sha_for_single_commit() { + let mut app = build_app_with_review_commits(vec![commit("aaa"), commit("bbb")]); + app.commit_selection_range = Some((1, 1)); // only "bbb" + assert_eq!( + app.commit_id_for_new_comment(), + Some("bbb".to_string()), + "single commit selection stamps that commit's SHA" + ); + } + + #[test] + fn add_comment_to_session_stamps_commit_id_when_provided() { + use crate::review_store::{AddCommentRequest, CommentTarget, add_comment_to_session}; + use std::path::PathBuf; + + let mut session = ReviewSession::new( + PathBuf::from("/repo"), + "head".to_string(), + Some("main".to_string()), + SessionDiffSource::WorkingTree, + ); + session.add_file( + PathBuf::from("src/lib.rs"), + crate::model::FileStatus::Modified, + 0, + ); + + let comment = add_comment_to_session( + &mut session, + AddCommentRequest { + target: CommentTarget::Line { + path: PathBuf::from("src/lib.rs"), + line: 10, + side: LineSide::New, + }, + content: "scoped note".to_string(), + comment_type: CommentType::Note, + author: "user".to_string(), + commit_id: Some("abc123".to_string()), + }, + ) + .unwrap(); + + assert_eq!( + comment.commit_id, + Some("abc123".to_string()), + "add_comment_to_session must stamp the provided commit_id" + ); + let stored = &session + .files + .get(&PathBuf::from("src/lib.rs")) + .unwrap() + .line_comments + .get(&10) + .unwrap()[0]; + assert_eq!(stored.commit_id, Some("abc123".to_string())); + } + + #[test] + fn add_comment_to_session_leaves_commit_id_none_when_not_provided() { + use crate::review_store::{AddCommentRequest, CommentTarget, add_comment_to_session}; + use std::path::PathBuf; + + let mut session = ReviewSession::new( + PathBuf::from("/repo"), + "head".to_string(), + Some("main".to_string()), + SessionDiffSource::WorkingTree, + ); + session.add_file( + PathBuf::from("src/lib.rs"), + crate::model::FileStatus::Modified, + 0, + ); + + let comment = add_comment_to_session( + &mut session, + AddCommentRequest { + target: CommentTarget::Line { + path: PathBuf::from("src/lib.rs"), + line: 10, + side: LineSide::New, + }, + content: "unscoped note".to_string(), + comment_type: CommentType::Note, + author: "user".to_string(), + commit_id: None, + }, + ) + .unwrap(); + + assert_eq!( + comment.commit_id, None, + "commit_id must stay None when not provided" + ); + } + + #[test] + fn legacy_session_json_deserializes_comment_without_commit_id() { + let json = r#"{ + "id": "test-id", + "content": "old comment", + "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 + }"#; + let comment: crate::model::Comment = serde_json::from_str(json).unwrap(); + assert_eq!( + comment.commit_id, None, + "legacy JSON without commit_id must deserialize as None" + ); + } +} + #[cfg(test)] mod target_selector_tests { use super::*; @@ -14477,6 +14922,60 @@ mod expand_gap_tests { assert_eq!(app.total_lines(), app.line_annotations.len()); } + #[test] + fn total_lines_must_match_annotations_when_commit_scoped_comment_is_hidden() { + // Regression: file_render_body_height counted all comments unconditionally + // while rebuild_annotations and the renderers filtered commit-hidden ones, + // so total_lines() exceeded line_annotations.len() and scroll math drifted. + let files = vec![make_file_with_hunks("a.rs", vec![make_hunk(1, 5)])]; + let mut app = build_app_with_files(files, 5); + app.sync_viewport_width(80); + + // Add a line comment scoped to commit "aaa" on line 1. + let mut comment = Comment::new( + "scoped to aaa".to_string(), + CommentType::Note, + Some(LineSide::New), + ); + comment.commit_id = Some("aaa".to_string()); + app.session + .files + .get_mut(&PathBuf::from("a.rs")) + .unwrap() + .add_line_comment(1, comment); + + // Select a different commit "bbb" so the comment is hidden. + app.review_commits = vec![ + crate::vcs::traits::CommitInfo { + id: "aaa".to_string(), + short_id: "aaa".to_string(), + branch_name: None, + summary: "commit aaa".to_string(), + body: None, + author: "tester".to_string(), + time: chrono::Utc::now(), + }, + crate::vcs::traits::CommitInfo { + id: "bbb".to_string(), + short_id: "bbb".to_string(), + branch_name: None, + summary: "commit bbb".to_string(), + body: None, + author: "tester".to_string(), + time: chrono::Utc::now(), + }, + ]; + app.commit_selection_range = Some((1, 1)); // only "bbb" + + app.rebuild_annotations(); + + assert_eq!( + app.total_lines(), + app.line_annotations.len(), + "total_lines must match annotations when a commit-scoped comment is filtered out" + ); + } + #[test] fn comment_navigator_items_follow_rendered_comment_order() { use crate::forge::remote_comments::{ diff --git a/src/model/comment.rs b/src/model/comment.rs index cbc697d9..56fd85f3 100644 --- a/src/model/comment.rs +++ b/src/model/comment.rs @@ -172,6 +172,15 @@ pub struct Comment { /// inline comments; review-level / summary comments don't get one. #[serde(default)] pub remote_comment_id: Option, + /// 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, } impl Comment { @@ -188,6 +197,7 @@ impl Comment { lifecycle_state: CommentLifecycleState::default(), remote_review_id: None, remote_comment_id: None, + commit_id: None, } } @@ -210,6 +220,7 @@ impl Comment { lifecycle_state: CommentLifecycleState::default(), remote_review_id: None, remote_comment_id: None, + commit_id: None, } } @@ -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) -> 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 { diff --git a/src/model/review.rs b/src/model/review.rs index 7f84ce06..fed8e1d3 100644 --- a/src/model/review.rs +++ b/src/model/review.rs @@ -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 diff --git a/src/output/markdown.rs b/src/output/markdown.rs index 19506210..2c4ae7cf 100644 --- a/src/output/markdown.rs +++ b/src/output/markdown.rs @@ -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, Option, String, &'a str); +use crate::slug::short_sha; +/// (file_path, line_range, side, comment_type, content, commit_id) +type CommentEntry<'a> = ( + String, + Option, + Option, + 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. @@ -346,6 +353,7 @@ fn generate_markdown( None, export_comment_type_label(&comment.comment_type, comment_types), &comment.content, + None, )); } @@ -364,6 +372,7 @@ fn generate_markdown( None, export_comment_type_label(&comment.comment_type, comment_types), &comment.content, + comment.commit_id.as_deref(), )); } @@ -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(), )); } } @@ -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) @@ -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}"); @@ -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 diff --git a/src/review_cli.rs b/src/review_cli.rs index 49b37d14..434c7f48 100644 --- a/src/review_cli.rs +++ b/src/review_cli.rs @@ -100,6 +100,7 @@ fn add_comment( content: request_parts.content, comment_type, author, + commit_id: None, }, )?; let output = CommentOutput::from_target(&target, &comment); @@ -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(); diff --git a/src/review_store.rs b/src/review_store.rs index 1df1fbe6..b56a6738 100644 --- a/src/review_store.rs +++ b/src/review_store.rs @@ -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, } /// Where a new local draft comment should be attached. @@ -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); @@ -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 } @@ -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(); @@ -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(); @@ -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(); @@ -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(); @@ -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(); diff --git a/src/ui/diff_side_by_side.rs b/src/ui/diff_side_by_side.rs index 274cf699..30ff941a 100644 --- a/src/ui/diff_side_by_side.rs +++ b/src/ui/diff_side_by_side.rs @@ -329,6 +329,9 @@ pub(super) fn render_side_by_side_diff(frame: &mut Frame, app: &mut App, area: R // Show file-level comments if let Some(review) = app.session.files.get(path) { for comment in &review.file_comments { + if !app.comment_visible(comment) { + continue; + } // Skip rendering this comment if it's being edited let is_being_edited = app.editing_comment_id.as_ref() == Some(&comment.id) && is_file_comment_mode; @@ -1410,8 +1413,9 @@ fn add_comments_to_line( if let Some(comments) = line_comments.get(&line_num) { for comment in comments { let comment_side = comment.side.unwrap_or(LineSide::New); - if (side == LineSide::Old && comment_side == LineSide::Old) - || (side == LineSide::New && comment_side != LineSide::Old) + if ((side == LineSide::Old && comment_side == LineSide::Old) + || (side == LineSide::New && comment_side != LineSide::Old)) + && ctx.app.comment_visible(comment) { // Check if this comment is being edited let is_being_edited = diff --git a/src/ui/diff_unified.rs b/src/ui/diff_unified.rs index 71fd89d9..756dc005 100644 --- a/src/ui/diff_unified.rs +++ b/src/ui/diff_unified.rs @@ -288,6 +288,9 @@ pub(super) fn render_unified_diff(frame: &mut Frame, app: &mut App, area: Rect) // Show file-level comments right after the header if let Some(review) = app.session.files.get(path) { for comment in &review.file_comments { + if !app.comment_visible(comment) { + continue; + } // Skip rendering this comment if it's being edited let is_being_edited = app.editing_comment_id.as_ref() == Some(&comment.id) && is_file_comment_mode; @@ -643,7 +646,9 @@ pub(super) fn render_unified_diff(frame: &mut Frame, app: &mut App, area: Rect) if let Some(comments) = line_comments.get(&old_ln) { for comment in comments { - if comment.side == Some(LineSide::Old) { + if comment.side == Some(LineSide::Old) + && app.comment_visible(comment) + { // Skip if this comment is being edited let is_being_edited = is_line_comment_mode && app.editing_comment_id.as_ref() == Some(&comment.id); @@ -808,7 +813,9 @@ pub(super) fn render_unified_diff(frame: &mut Frame, app: &mut App, area: Rect) if let Some(comments) = line_comments.get(&new_ln) { for comment in comments { - if comment.side != Some(LineSide::Old) { + if comment.side != Some(LineSide::Old) + && app.comment_visible(comment) + { // Skip if this comment is being edited let is_being_edited = is_line_comment_mode && app.editing_comment_id.as_ref() == Some(&comment.id);