feat(scm): add line anchoring to ReviewComment#120
Conversation
Surface inline-comment line anchors on the provider-agnostic ReviewComment TypedDict: line, start_line, original_line, original_start_line. Populated by the GitHub REST mapper and the GitLab diff-note mapper; the GitHub GraphQL mapper sets them to None (unavailable in that mutation response), and GitLab leaves original_* as None (no equivalent concept). Consumers of get_review_comments and the other ReviewComment-returning actions can now render file_path:line instead of file-only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Drop original_line/original_start_line from ReviewComment; instead line/start_line fall back to the GitHub struct's original_* values when the head-diff line is null (outdated comments). GitLab has no original_* source, so its behavior is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
@sentry review |
map_review_comment now derives line/start_line via the same shared _position_line_anchor helper as map_review_thread, which prefers the line_range endpoints and falls back to top-level new_line/old_line. Previously a note whose anchor lived only in line_range.end resolved to line=None. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| unique_id=f"{discussion_id}:{raw['id']}", | ||
| url=None, | ||
| file_path=raw.get("position", {}).get("new_path"), | ||
| file_path=position.get("new_path"), |
There was a problem hiding this comment.
map_review_comment resolves file_path as just position.get("new_path"), but map_review_thread (line ~2151) uses position.get("new_path") or position.get("old_path"). For a comment on a deleted file only old_path is present, so this path returns file_path=None while the thread mapper returns the correct path.
Since this PR is already consolidating position parsing into _position_line_anchor, this is a good moment to fold file_path resolution into a shared helper (or add a sibling _position_path(position)) and call it from both mappers — that removes the divergence and fixes the removed-file case.
| author: Author | None | ||
| created_at: str | None | ||
| diff_hunk: str | None | ||
| line: int | None |
There was a problem hiding this comment.
Informational only — no change requested.
Worth being aware of an asymmetry this cements: the write path takes a DiffLine ({base, head}, from #116), while this read path collapses base/head into a flat int | None via _position_line (new_line else old_line). That's the right call for the stated goal (Seer just wants a single line number), but it means a fetched ReviewComment can't be round-tripped back into create_review_comment without re-deriving which side the line is on.
Flagging so it's a conscious trade-off rather than an accident — nothing to do here.
Fold file_path resolution into a shared _position_path helper used by both map_review_thread and map_review_comment, so comments on deleted files resolve old_path instead of returning None. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
705849f to
22635bd
Compare
There was a problem hiding this comment.
i realized it shortly after pushing 😓 fixed now
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| line: int | None | ||
| start_line: int | None |
There was a problem hiding this comment.
Would it make sense to make these types as DiffLine to match ReviewCommentInput?
There was a problem hiding this comment.
yea, it would make sense to use DiffLine especially after looking at getsentry/seer#7333
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
Change ReviewComment.line/start_line from int to DiffLine so a comment's anchor carries which side of the diff it lands on (base vs head), matching the write-side ReviewCommentInput. A raw int collapsed GitHub's LEFT/RIGHT side and GitLab's old_line/new_line into one number, losing that distinction. ReviewThread.line stays int (already released). GitHub: add side/start_side to the struct and build DiffLine via _github_diff_line (inverse of _github_line_side), keeping the original_* fallback for outdated comments. GitLab: add _position_diff_line* helpers mapping new_line->head/old_line->base, leaving the int path for map_review_thread untouched. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want reviews to match your repository better? Bugbot Learning can learn team-specific rules from PR activity. A team admin can enable Learning in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 41322b2. Configure here.
Address review feedback. Make the int-returning _position_line_anchor delegate to _position_diff_line_anchor so the line_range fallback lives in one place; remove the now-unused _position_line. Fix BaseTestProvider so create_review_comment_file returns a null line (file-level) and create_review_comment mirrors its DiffLine input instead of the fixture default. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
2 participants
We want to add line numbers to PR/MR review comments, so we can pass in the line number information to seer about which line the review comment was left on.
github has 4 line number information in their review comment object:
line,start_line,original_line,original_start_line. However, GitLab only haslineandstart_line, so we only includelineandstart_linein ReviewComment. Those will be populated withoriginal_lineandoriginal_start_linein GH iflineandstart_lineis emptyno change required on sentry nor seer, nothing uses the line or start_line