fix(comments): associate comments with their commit when reviewing per-commit#442
Merged
Merged
Conversation
…r-commit 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<String> 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.
Owner
|
Thanks! This makes sense to me. |
1 task
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Comments were keyed only by (path, line), so when reviewing commit-by-commit via the inline selector, a comment made on commit A's line 42 also showed on commit B's line 42 — mixing up the diff view, comment navigator, and submit preflight.
commit_id: Option<String>toComment(serde#[default], so legacy session JSON rehydrates asNone= always visible), stamped when the inline selector shows exactly one commit.comment_visible_with): visible ifcommit_idisNoneor its SHA is in the selected commit set. Applied inrebuild_annotations, both diff renderers,file_render_body_height, the comment navigator, and the submit preflight; hot paths compute the commit set once and pass it down.find_comment_at_cursorguards against stale annotations during an async commit-selection reload, so delete/edit no-op on hidden comments.delete_comment_at_cursorandenter_edit_modere-derived the comment index by counting side-matching comments, butAnnotatedLine::LineComment.comment_idxis the absolute index into the stored Vec. Both now use the absolute index directly.