Skip to content

Handle view changes link for body-less reviews with review comments#2364

Merged
Urgau merged 4 commits intorust-lang:masterfrom
Urgau:gh-changes-since-review-comment
Apr 7, 2026
Merged

Handle view changes link for body-less reviews with review comments#2364
Urgau merged 4 commits intorust-lang:masterfrom
Urgau:gh-changes-since-review-comment

Conversation

@Urgau
Copy link
Copy Markdown
Member

@Urgau Urgau commented Apr 5, 2026

This PR address a frustration brought up by some reviewers when a review with a body/description is submitted.

We currently for technical reason we can't add the "View changes this review" link, GitHub blocks it.

This PR adds the links to the closets things available: the review comments (plural).

image

To avoid an unnecessary API call when we receive the review comments webhooks I added a small cache.

Discussed at #triagebot > View changes link for body-less reviews
cc @jdonszelmann

@Urgau Urgau requested a review from Kobzol April 5, 2026 13:36
@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented Apr 7, 2026

Implementation looks fine, but I'm confused by the link message, especially the above part can be confusing if the review threads are long and there are many of them. Why not just use the same string as for the normal reviews with bodies? Then it's obvious that it points to the same thing, and it's just a copy-pasted link.

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented Apr 7, 2026

I'm worried some people might get confused by the timeline of events, since the link will refer to the state of events when the review (and it's comments) were submitted, not created.

See this example where the comment was created several hours before the review was submitted.
image

@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented Apr 7, 2026

Hmm, that might be confusing indeed, though I'm not sure if people actually read the timestamps here, and whether they make a difference between when a comment was created and when the review was submitted; I considered these to be mostly an atomic event, from my point of view.

Maybe I'd just drop the "above", because the review might not actually be directly above the comment.

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented Apr 7, 2026

Well, there might having been changes pushed in the mean time.

But I agree with you that the link text is not ideal, too long and subtlety is not worth confusing people.

Maybe we can just change "this" to "the", like so "View changes since the review"? that way it's clear that we are referring to the review, not the comment.

@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented Apr 7, 2026

Yeah, I was thinking the same; the comment already contains "the", so just dropping "above" should do the trick, I think.

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented Apr 7, 2026

So "View changes since the review was submitted"? or "View changes since the review"?

@Kobzol
Copy link
Copy Markdown
Member

Kobzol commented Apr 7, 2026

I'm fine with both, up to you! (sorry 😆)

@Urgau Urgau force-pushed the gh-changes-since-review-comment branch from ea4682c to 52bfca0 Compare April 7, 2026 17:07
@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented Apr 7, 2026

I went with the second proposal, it's the shortest and closest one to the already existing link.

I also fixed the condition for detecting reviews, otherwise it would trigger even on normal comments (that would have been embarrassing ;-)).

Copy link
Copy Markdown
Member

@Kobzol Kobzol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! Let's see how this goes.

View changes since this review

@Urgau Urgau added this pull request to the merge queue Apr 7, 2026
Merged via the queue into rust-lang:master with commit 459a03a Apr 7, 2026
3 checks passed
@Urgau Urgau deleted the gh-changes-since-review-comment branch April 7, 2026 17:17
@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented Apr 7, 2026

Let's do some testing.

Normal comment.

Copy link
Copy Markdown
Member Author

@Urgau Urgau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a review (link should be here)

View changes since this review

Copy link
Copy Markdown
Member Author

@Urgau Urgau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Urgau
Copy link
Copy Markdown
Member Author

Urgau commented Apr 7, 2026

Alright, everything seems to be working fine, now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants