Repository navigation
feat/webhook-reviewer-name-parity - #39635
Open
stackedbyaradhya wants to merge 2 commits into
Open
stackedbyaradhya wants to merge 2 commits into
stackedbyaradhya wants to merge 2 commits into
Conversation
MSTeams already surfaces the requested reviewer's name on review-request/ review-request-removed pull request events (added in go-gitea#38289 for go-gitea#38270), but every other chat provider just says "Pull request review requested" with no indication of who. api.PullRequestPayload.RequestedReviewer is already populated for all providers; it just isn't consumed by Discord or Slack. Extend Discord and Slack the same way MSTeams does: when the action is a review-request(-removed) event and RequestedReviewer is set, append the reviewer's username (and full name, if set) to the notification. Scoped to these two providers for this PR to keep the diff small; the same change could be made for the remaining providers if wanted. Added TestDiscordPayload/PullRequestReviewRequest and TestSlackPayload/PullRequestReviewRequest, modeled on the existing MSTeams test for the same scenario. Verified both fail without the respective provider change and pass with it.
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Bound Discord titles and escape reviewer names in Slack mrkdwn before approval.
Review effort: Lite
Findings: 2
Open (2)
What changed in this PR
Adds requested reviewer names to Discord and Slack pull-request review notifications.
Changes:
- Appends reviewer usernames and optional full names.
- Adds Discord and Slack regression tests.
| File | Summary |
|---|---|
services/webhook/slack.go |
Adds reviewer details to Slack notifications. |
services/webhook/slack_test.go |
Tests Slack reviewer output. |
services/webhook/discord.go |
Adds reviewer details to Discord titles. |
services/webhook/discord_test.go |
Tests Discord reviewer output. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+200
to
+206
| if (p.Action == api.HookIssueReviewRequested || p.Action == api.HookIssueReviewRequestRemoved) && p.RequestedReviewer != nil { | ||
| reviewerName := p.RequestedReviewer.UserName | ||
| if p.RequestedReviewer.FullName != "" { | ||
| reviewerName += " (" + p.RequestedReviewer.FullName + ")" | ||
| } | ||
| title += " (Requested Reviewer: " + reviewerName + ")" | ||
| } |
| if p.RequestedReviewer.FullName != "" { | ||
| reviewerName += " (" + p.RequestedReviewer.FullName + ")" | ||
| } | ||
| text += " (Requested Reviewer: " + reviewerName + ")" |
Addresses review feedback on this PR: - Discord embed titles are capped at 256 characters by Discord's API. Appending an unbounded username/full name to the title could push a long PR title over that limit, which Discord then rejects outright instead of delivering. Truncate the combined title at discordTitleCharactersLimit (256), matching the existing discordDescriptionCharactersLimit pattern used for the description. - Slack's text field is mrkdwn, and the reviewer's full name was appended without the escaping SlackTextFormatter already applies to every other piece of user content in this file. A name containing &, <, or > could break rendering or be misinterpreted as Slack markup. Route it through SlackTextFormatter like everything else. Added PullRequestReviewRequestTruncatesLongTitle (Discord) and PullRequestReviewRequestEscapesReviewerName (Slack) to cover these. Verified both fail without their respective fix and pass with it.
This branch has not been deployed
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
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.
Closes #39634
MSTeams already surfaces the requested reviewer's name on review-request/
review-request-removed pull request events (added in #38289 for #38270),
but every other chat provider just says "Pull request review requested"
with no indication of who. api.PullRequestPayload.RequestedReviewer is
already populated for all providers; it just isn't consumed by Discord
or Slack.
Extends Discord and Slack the same way MSTeams does: when the action is
a review-request(-removed) event and RequestedReviewer is set, appends
the reviewer's username (and full name, if set) to the notification.
Scoped to these two providers for this PR to keep the diff small; the
same change could be made for the remaining providers if wanted.
Added TestDiscordPayload/PullRequestReviewRequest and
TestSlackPayload/PullRequestReviewRequest, modeled on the existing
MSTeams test for the same scenario. Verified both fail without the
respective provider change and pass with it.