Skip to content

MM-68829 Improving UX for DMs sent on PR comments #1010

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
avasconcelos114 merged 3 commits into from
May 27, 2026
Merged

MM-68829 Improving UX for DMs sent on PR comments #1010

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c49b18c
Improving UX for DMs sent on PR comments
avasconcelos114 May 15, 2026
42b3f35
Fixing issues found in review
avasconcelos114 May 15, 2026
ea29a91
Removed useless empty string return
avasconcelos114 May 26, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
64 changes: 57 additions & 7 deletions server/plugin/template.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,19 @@ func init() {
return mdCommentRegex.ReplaceAllString(body, "")
}

funcMap["cleanBody"] = func(body string) string {
cleaned := body
if strings.Contains(cleaned, "notifications@github.com") {
cleaned = strings.Split(cleaned, "\n\nOn")[0]
}
cleaned = mdCommentRegex.ReplaceAllString(cleaned, "")
cleaned = strings.TrimSpace(cleaned)
if cleaned == "" {
Copy link
Copy Markdown
Contributor

@nang2049 nang2049 May 19, 2026

Choose a reason for hiding this comment

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

nit: this if cleaned == "" { return "" } block returns "" either way. Just return cleaned

return ""
}
return cleaned
}

// Replace any GitHub username with its corresponding Mattermost username, if any
funcMap["replaceAllGitHubUsernames"] = func(body string) string {
return gitHubUsernameRegex.ReplaceAllStringFunc(body, func(matched string) string {
Expand Down Expand Up @@ -358,41 +371,78 @@ Reviewers: {{range $i, $el := .RequestedReviewers -}} {{- if $i}}, {{end}}{{temp
{{template "repo" .GetRepo}} New review comment by {{template "user" .GetSender}} on {{template "pullRequest" .GetPullRequest}}:

{{.GetComment.GetBody | trimBody | replaceAllGitHubUsernames}}
`))

template.Must(masterTemplate.New("reviewCommentMentionNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} mentioned you in a review comment on [{{.GetRepo.GetFullName}}#{{.GetPullRequest.GetNumber}}]({{.GetComment.GetHTMLURL}}) - {{.GetPullRequest.GetTitle}}:
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("reviewCommentAuthorNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} commented on your pull request [{{.GetRepo.GetFullName}}#{{.GetPullRequest.GetNumber}}]({{.GetComment.GetHTMLURL}}) - {{.GetPullRequest.GetTitle}}:
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("commentMentionNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} mentioned you on [{{.GetRepo.GetFullName}}#{{.Issue.GetNumber}}]({{.GetComment.GetHTMLURL}}) - {{.Issue.GetTitle}}:
{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}}
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("commentAuthorPullRequestNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} commented on your pull request {{template "eventRepoIssueFullLinkWithTitle" .}}:
{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}}
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("commentAssigneePullRequestNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} commented on pull request you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}:
{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}}
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("commentAssigneeIssueNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} commented on an issue you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}:
{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}}
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("commentAssigneeSelfMentionPullRequestNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} mentioned you on a pull request that you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}:
{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}}
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("commentAssigneeSelfMentionIssueNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} mentioned you on an issue that you are assigned to {{template "eventRepoIssueFullLinkWithTitle" .}}:
{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}}
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("commentAuthorIssueNotification").Funcs(funcMap).Parse(`
{{template "user" .GetSender}} commented on your issue {{template "eventRepoIssueFullLinkWithTitle" .}}:
{{.GetComment.GetBody | trimBody | quote | replaceAllGitHubUsernames}}
{{- $body := .GetComment.GetBody | cleanBody | replaceAllGitHubUsernames}}
{{- if $body}}
{{$body | quote}}
{{- end}}
`))

template.Must(masterTemplate.New("pullRequestNotification").Funcs(funcMap).Parse(`
Expand Down
11 changes: 7 additions & 4 deletions server/plugin/test_utils.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -280,8 +280,9 @@ func GetMockPullRequestReviewEvent(action, state, repo string, isPrivate bool, r
}
}

func GetMockPullRequestReviewCommentEvent() *github.PullRequestReviewCommentEvent {
func GetMockPullRequestReviewCommentEvent(action, body, sender string) *github.PullRequestReviewCommentEvent {
return &github.PullRequestReviewCommentEvent{
Action: github.String(action),
Repo: &github.Repository{
Name: github.String(MockRepoName),
FullName: github.String(MockOrgRepo),
Expand All @@ -290,13 +291,15 @@ func GetMockPullRequestReviewCommentEvent() *github.PullRequestReviewCommentEven
},
Comment: &github.PullRequestComment{
ID: github.Int64(12345),
Body: github.String("This is a review comment"),
Body: github.String(body),
HTMLURL: github.String(fmt.Sprintf("%s%s/pull/1#discussion_r12345", GithubBaseURL, MockOrgRepo)),
},
Sender: &github.User{
Login: github.String(MockUserLogin),
Login: github.String(sender),
},
PullRequest: &github.PullRequest{
User: &github.User{Login: github.String(MockIssueAuthor)},
},
PullRequest: &github.PullRequest{},
}
}

Expand Down
104 changes: 102 additions & 2 deletions server/plugin/webhook.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -287,6 +287,8 @@ func (p *Plugin) handleWebhook(w http.ResponseWriter, r *http.Request) {
repo = event.GetRepo()
handler = func() {
p.postPullRequestReviewCommentEvent(event)
p.handleReviewCommentMentionNotification(event)
p.handleReviewCommentAuthorNotification(event)
}
case *github.PushEvent:
repo = ConvertPushEventRepositoryToRepository(event.GetRepo())
Expand Down Expand Up @@ -1019,12 +1021,16 @@ func (p *Plugin) postPullRequestReviewEvent(event *github.PullRequestReviewEvent
func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestReviewCommentEvent) {
repo := event.GetRepo()

if event.GetAction() != actionCreated {
return
}

subs := p.GetSubscribedChannelsForRepository(repo)
if len(subs) == 0 {
return
}

newReviewMessage, err := renderTemplate("newReviewComment", event)
message, err := renderTemplate("newReviewComment", event)
if err != nil {
p.client.Log.Warn("Failed to render template", "error", err.Error())
return
Expand Down Expand Up @@ -1061,7 +1067,7 @@ func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestRevi
continue
}

post := p.makeBotPost(newReviewMessage, "custom_git_pr_comment")
post := p.makeBotPost(message, "custom_git_pr_comment")

repoName := strings.ToLower(repo.GetFullName())
commentID := event.GetComment().GetID()
Expand All @@ -1077,6 +1083,95 @@ func (p *Plugin) postPullRequestReviewCommentEvent(event *github.PullRequestRevi
}
}

func (p *Plugin) handleReviewCommentMentionNotification(event *github.PullRequestReviewCommentEvent) {
Copy link
Copy Markdown
Contributor

@nang2049 nang2049 May 19, 2026

Choose a reason for hiding this comment

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

handleCommentMentionNotification filters out mentioned users who are also assignees. Maybe worth mirroring the skip logic for consistency and to avoid double DM?

Copy link
Copy Markdown
Contributor Author

@avasconcelos114 avasconcelos114 May 26, 2026

Choose a reason for hiding this comment

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

For the PullRequestReviewCommentEvent events there isn't an assignee handler like we do for IssueCommentEvent, only one handler for the author which this function is skipping inhandleReviewCommentMentionNotification:1112 to avoid duplication, so I think in here we're safe from the risk of double DMing here

if event.GetAction() != actionCreated {
return
}

body := event.GetComment().GetBody()

if strings.Contains(body, "notifications@github.com") {
body = strings.Split(body, "\n\nOn")[0]
}

mentionedUsernames := parseGitHubUsernamesFromText(body)
Comment thread
coderabbitai[bot] marked this conversation as resolved.

message, err := renderTemplate("reviewCommentMentionNotification", event)
if err != nil {
p.client.Log.Warn("Failed to render template", "error", err.Error())
return
}

for _, username := range mentionedUsernames {
if username == event.GetSender().GetLogin() {
continue
}

if username == event.GetPullRequest().GetUser().GetLogin() {
continue
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Outdated
}

userID := p.getGitHubToUserIDMapping(username)
if userID == "" {
continue
}

if event.GetRepo().GetPrivate() && !p.permissionToRepo(userID, event.GetRepo().GetFullName()) {
continue
}

if p.senderMutedByReceiver(userID, event.GetSender().GetLogin()) {
continue
}

channel, err := p.client.Channel.GetDirect(userID, p.BotUserID)
if err != nil {
continue
}

post := p.makeBotPost(message, "custom_git_mention")
post.ChannelId = channel.Id
if err = p.client.Post.CreatePost(post); err != nil {
p.client.Log.Warn("Error creating review comment mention post", "error", err.Error())
}

p.sendRefreshEvent(userID)
}
}

func (p *Plugin) handleReviewCommentAuthorNotification(event *github.PullRequestReviewCommentEvent) {
author := event.GetPullRequest().GetUser().GetLogin()
if author == event.GetSender().GetLogin() {
return
}

if event.GetAction() != actionCreated {
return
}

authorUserID := p.getGitHubToUserIDMapping(author)
if authorUserID == "" {
return
}

if event.GetRepo().GetPrivate() && !p.permissionToRepo(authorUserID, event.GetRepo().GetFullName()) {
return
}

if p.senderMutedByReceiver(authorUserID, event.GetSender().GetLogin()) {
return
}

message, err := renderTemplate("reviewCommentAuthorNotification", event)
if err != nil {
p.client.Log.Warn("Failed to render template", "error", err.Error())
return
}

p.CreateBotDMPost(authorUserID, message, "custom_git_author")
p.sendRefreshEvent(authorUserID)
}

func (p *Plugin) handleCommentMentionNotification(event *github.IssueCommentEvent) {
action := event.GetAction()
if action == actionEdited || action == actionDeleted {
Expand Down Expand Up @@ -1205,6 +1300,11 @@ func (p *Plugin) handleCommentAuthorNotification(event *github.IssueCommentEvent
}

func (p *Plugin) handleCommentAssigneeNotification(event *github.IssueCommentEvent) {
action := event.GetAction()
if action == actionEdited || action == actionDeleted {
Copy link
Copy Markdown
Contributor

@nang2049 nang2049 May 19, 2026

Choose a reason for hiding this comment

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

We can check if handleCommentAssigneeNotification caller already filters out edit and delete actions. If yes this guard is dead code.

Copy link
Copy Markdown
Contributor Author

@avasconcelos114 avasconcelos114 May 19, 2026

Choose a reason for hiding this comment

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

The caller (handleWebhook) doesn't filter out by action, this has been done for each individual handler previously but this handler in particular was missing it

I'm wondering if we want to move the guard up to handleWebhook but not entirely certain as this would deviate from how every other webhook event is processed (specific action-related guards happening at the handler function-level), what do you think works best here?

return
}

author := event.GetIssue().GetUser().GetLogin()
assignees := event.GetIssue().Assignees
repoName := event.GetRepo().GetFullName()
Expand Down
Loading
Loading