Skip to content

feat: Add user mention functionality to incident comments #4649

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.

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
merged 8 commits into from
May 22, 2025

Conversation

hemangsk
Copy link
Contributor

@hemangsk hemangsk commented Apr 29, 2025

Added proper support for @mentions in incident comments. Created a dedicated CommentMention model with relationships to AlertAudit.

Closes #2187

πŸ“‘ Description

βœ… Checks

  • My pull request adheres to the code style of this project
  • My code requires changes to the documentation
  • I have updated the documentation as required
  • All the tests have passed

β„Ή Additional Information

Screenshot 2025-04-29 at 5 19 10β€―PM

/claim #2187

Copy link

vercel bot commented Apr 29, 2025

@hemangsk is attempting to deploy a commit to the KeepHQ Team on Vercel.

A member of the Team first needs to authorize it.

@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Apr 29, 2025
@CLAassistant
Copy link

CLAassistant commented Apr 29, 2025

CLA assistant check
All committers have signed the CLA.

@hemangsk
Copy link
Contributor Author

hemangsk commented May 1, 2025

@talboren @shahargl , could you please take a look if this fulfils the requirements? Let me know if there are any changes required to the approach. I'm working on tests atm.

@talboren
Copy link
Member

talboren commented May 1, 2025

@greptile

@talboren
Copy link
Member

talboren commented May 1, 2025

@Kiryous may I ask your help with reviewing the UI part here? πŸ™πŸΌ

@talboren
Copy link
Member

talboren commented May 1, 2025

@talboren @shahargl , could you please take a look if this fulfils the requirements? Let me know if there are any changes required to the approach. I'm working on tests atm.

The functionality looks great! I'm just wondering if it's not a bit complex under the hood. Let me review a bit further.

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

Added user mention functionality to incident comments, including a new CommentMention table and UI components for handling @mentions in the activity feed.

  • Added new CommentMention table with proper indexes and foreign key constraints in /keep/api/models/db/migrations/2025-04-29-15-21_1b12e6d6ad1f.py
  • Implemented mention dropdown UI with keyboard navigation in /keep-ui/app/(keep)/incidents/[id]/activity/ui/IncidentActivityComment.tsx
  • Added mention highlighting and user data fetching in /keep-ui/app/(keep)/incidents/[id]/activity/ui/IncidentActivityItem.tsx
  • Modified /keep/api/routes/incidents.py to handle tagged users in comments with proper transaction management
  • Added CommentMentionDto interface and updated AuditEvent type in /keep-ui/entities/alerts/model/types.ts

πŸ’‘ (1/5) You can manually trigger the bot by mentioning @greptileai in a comment!

9 file(s) reviewed, 9 comment(s)
Edit PR Review Bot Settings | Greptile

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels May 2, 2025
@skynetigor
Copy link
Contributor

Hey!
Quick question β€” since we’re already using ReactQuill, could we integrate quill-mention to handle mentions instead of building a custom input from scratch?
It seems like a lighter approach, given that Quill is already part of our stack.

@hemangsk
Copy link
Contributor Author

hemangsk commented May 6, 2025

@skynetigor Thanks for the review! I'll get right on it

@hemangsk hemangsk force-pushed the hmg/tag branch 4 times, most recently from 1ea6e5a to c8507ad Compare May 6, 2025 19:42
@hemangsk
Copy link
Contributor Author

hemangsk commented May 6, 2025

@skynetigor , I've updated the MR with quill-mention package and here's the comment box in action.

Screenshot 2025-05-07 at 3 27 59β€―AM

Copy link
Contributor

@Kiryous Kiryous left a comment

Choose a reason for hiding this comment

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

Please check the comment on quill-mention.js lib

@talboren
Copy link
Member

@Kiryous @skynetigor is this done?

Copy link

vercel bot commented May 19, 2025

The latest updates on your projects. Learn more about Vercel for Git β†—οΈŽ

1 Skipped Deployment
Name Status Preview Comments Updated (UTC)
keep ⬜️ Ignored (Inspect) Visit Preview May 21, 2025 2:03pm

@hemangsk
Copy link
Contributor Author

@Kiryous I noticed that your PR #4841 has some refactoring of the CommentMention code in addition to the quill-mention fix. Shall we close this PR in favor of #4841 ?

@Kiryous
Copy link
Contributor

Kiryous commented May 20, 2025

@hemangsk hey! Please merge my PR into your branch and I have a few more important comments for you a bit later today.

@hemangsk
Copy link
Contributor Author

@Kiryous Done πŸ‘

@Kiryous
Copy link
Contributor

Kiryous commented May 20, 2025

@greptileai

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

PR Summary

(updates since last review)

Added user mention functionality to incident comments with backend support and UI components.

  • Added extractTaggedUsers utility in /keep-ui/app/(keep)/incidents/[id]/activity/lib/extractTaggedUsers.ts to parse mentions from Quill editor content
  • Potential XSS vulnerability in /keep-ui/app/(keep)/incidents/[id]/activity/ui/IncidentActivityItem.tsx due to unsanitized HTML rendering
  • Added proper mention handling in /keep/api/routes/incidents.py with CommentMention model integration
  • Empty merge migration in /keep/api/models/db/migrations/versions/2025-05-19-18-48_90e3eababbf0.py needs review
  • Empty root package.json file needs to be addressed

17 file(s) reviewed, 14 comment(s)
Edit PR Review Bot Settings | Greptile

Copy link
Contributor

@Kiryous Kiryous left a comment

Choose a reason for hiding this comment

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

hey @hemangsk! I left a few comments and greptileai bot also left a few important (I resolved unimportant). Please review and fix them and let's merge it!

reach out if you have questions

@hemangsk
Copy link
Contributor Author

@Kiryous , I've pushed an update and all the changes are now done

Regarding #4649 (comment)

I added the usage of IncidentActivity 96b35b5

Please let me know if that's alright or should we tackle it differently?

Copy link

codecov bot commented May 21, 2025

Codecov Report

Attention: Patch coverage is 78.94737% with 4 lines in your changes missing coverage. Please review.

Project coverage is 30.87%. Comparing base (7d126de) to head (c2ca497).
Report is 26 commits behind head on main.

Files with missing lines Patch % Lines
keep/api/models/incident.py 55.55% 4 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #4649       +/-   ##
===========================================
- Coverage   46.14%   30.87%   -15.28%     
===========================================
  Files         165       93       -72     
  Lines       17428    11051     -6377     
===========================================
- Hits         8043     3412     -4631     
+ Misses       9385     7639     -1746     

β˜” View full report in Codecov by Sentry.
πŸ“’ Have feedback on the report? Share it here.

πŸš€ New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • πŸ“¦ JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Copy link
Contributor

@Kiryous Kiryous left a comment

Choose a reason for hiding this comment

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

@hemangsk thank you for the changes, it looks good to me now! approving

@Kiryous Kiryous merged commit 7c5b4c8 into keephq:main May 22, 2025
22 of 24 checks passed
Copy link
Contributor

πŸš‚ Fantastic work @hemangsk! Your very first PR to keep has been merged! πŸŽ‰πŸ₯³

You've just taken your first step into open-source, and we couldn't be happier to have you onboard. πŸ™Œ
If you're feeling adventurous, why not dive into another issue and keep contributing? The community would love to see more from you! πŸš€

For any support, feel free to reach out on the community: https://slack.keephq.dev. Happy coding! πŸ‘©β€πŸ’»πŸ‘¨β€πŸ’»

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
πŸ™‹ Bounty claim Feature A new feature size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[πŸ”¨ Enhancement]: Allow tagging in Incident comment
5 participants