MM-68829 Improving UX for DMs sent on PR comments#1010
Conversation
- Preventing edit or delete PR message events from sending a "new comment" DM notification - Improving logic that cleans body to prevent issues with html comments in the message from interfering with visibility of message
avasconcelos114
added
2: Dev Review
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds mention and author notification flows for PR review comments. Introduces cleanBody template function, invokes mention/author handlers on created review comments, parses mentioned users, enforces permission and mute checks, sends direct posts or DMs, and updates tests and mocks. ChangesReview Comment Mention and Author Notifications
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/plugin/webhook.go`:
- Around line 1106-1111: The comparisons that suppress self/author mentions are
using case-sensitive equality against event.GetSender().GetLogin() and
event.GetPullRequest().GetUser().GetLogin(), so usernames with different casing
can bypass suppression; update those checks to perform case-insensitive
comparisons (e.g., use strings.EqualFold or compare lowercased strings) for the
expressions that reference username, event.GetSender().GetLogin(), and
event.GetPullRequest().GetUser().GetLogin(); make the same change at the other
occurrence around lines 1143-1145 so both sender and PR author checks are
case-insensitive.
- Around line 1091-1097: The code reads the comment body into body and only
strips the email footer before calling parseGitHubUsernamesFromText, but HTML
comment blocks (<!-- ... -->) can still contain mentions and trigger DMs; before
calling parseGitHubUsernamesFromText, remove or sanitize any HTML comments from
the comment text (the variable body produced from event.GetComment().GetBody())
so mentions inside <!-- --> are ignored, then pass the cleaned body to
parseGitHubUsernamesFromText (update the body variable or use a new cleanedBody)
to ensure hidden-comment mentions are not parsed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 03b5dc71-be81-4372-b8f0-96ade155671a
📒 Files selected for processing (4)
server/plugin/template.goserver/plugin/test_utils.goserver/plugin/webhook.goserver/plugin/webhook_test.go
avasconcelos114
changed the title
| } | ||
| } | ||
|
|
||
| func (p *Plugin) handleReviewCommentMentionNotification(event *github.PullRequestReviewCommentEvent) { |
There was a problem hiding this comment.
handleCommentMentionNotification filters out mentioned users who are also assignees. Maybe worth mirroring the skip logic for consistency and to avoid double DM?
|
|
||
| func (p *Plugin) handleCommentAssigneeNotification(event *github.IssueCommentEvent) { | ||
| action := event.GetAction() | ||
| if action == actionEdited || action == actionDeleted { |
There was a problem hiding this comment.
We can check if handleCommentAssigneeNotification caller already filters out edit and delete actions. If yes this guard is dead code.
There was a problem hiding this comment.
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?
| } | ||
| cleaned = mdCommentRegex.ReplaceAllString(cleaned, "") | ||
| cleaned = strings.TrimSpace(cleaned) | ||
| if cleaned == "" { |
There was a problem hiding this comment.
nit: this if cleaned == "" { return "" } block returns "" either way. Just return cleaned
2 participants
Summary
This PR mainly does two things to improve the general experience of receiving DM notifications from the GitHub bot
Ticket Link
Fixes: https://mattermost.atlassian.net/browse/MM-68829
QA Notes
The main webhook event affected by this is the PR commenting, which mainly consists of:
We'll want to make sure that editing a PR comment does not send a new notification to the users as a DM
We might also want to run some regression tests on the following DM notifications to confirm they still append a quote with the body of the comment as well:
Change Impact: 🟡 Medium
Reasoning: The PR adds new notification handlers for PR review comments and refines message-body cleaning logic used across DM notifications; these are user-facing changes spanning multiple files and flows but remain localized to notification handling with added tests.
Regression Risk: Moderate. New early-return guards and DM-sending paths reduce spam risk but introduce additional code paths (user mapping, permission checks, muted-user filtering) that could silently fail; the
cleanBodytemplate helper is exercised via templates but lacks dedicated unit tests.** QA Recommendation:** Perform manual QA focusing on PR comment workflows: confirm edits/deletes do not produce new DMs and verify quoted comment bodies for the listed notification types (commentMentionNotification, commentAuthorIssueNotification, commentAssigneePullRequestNotification, commentAssigneeIssueNotification, commentAssigneeSelfMentionPullRequestNotification, commentAssigneeSelfMentionIssueNotification) are cleaned correctly. Automated tests cover many cases but do not fully eliminate the need for targeted manual verification.
Generated by CodeRabbitAI