feat(api): support mentioning users in work-item comments#9403
Open
clwluvw wants to merge 1 commit into
Open
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughIssue comments now accept user IDs through a ChangesIssue comment mentions
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant IssueCommentCreateSerializer
participant ProjectMember
participant IssueComment
participant ActivityDispatcher
participant NotificationPipeline
Client->>IssueCommentCreateSerializer: Submit comment and mentions
IssueCommentCreateSerializer->>ProjectMember: Filter mention targets
ProjectMember-->>IssueCommentCreateSerializer: Return eligible member IDs
IssueCommentCreateSerializer->>IssueComment: Save rendered comment_html
IssueComment->>ActivityDispatcher: Emit persisted comment activity
ActivityDispatcher->>NotificationPipeline: Dispatch notifying activity
NotificationPipeline-->>Client: Create in-app notification
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
Adds first-class user-mention support to the public REST API for work-item comments, aligning external comment create/update/delete behavior with the internal app so mention notifications reliably fire and mention markup is server-rendered/round-trippable.
Changes:
- Extend the public comment create/update serializer to accept a
mentionslist and append<mention-component>markup intocomment_html. - Fix public comment activity dispatch to include persisted comment data (with
id+ rendered markup) and to enable notifications on create/update/delete. - Add contract tests (including an end-to-end Notification row assertion) and update the OpenAPI request example.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| apps/api/plane/utils/openapi/examples.py | Updates request example for comment creation to document the new mentions field. |
| apps/api/plane/tests/contract/api/test_issue_comment_mentions.py | Adds contract coverage for mention rendering, validation, de-duplication/idempotence, and notification creation. |
| apps/api/plane/api/views/issue.py | Ensures comment activities carry persisted comment payload and enables notifications/origin for create/update/delete. |
| apps/api/plane/api/serializers/issue.py | Implements server-side mention markup rendering and project-member filtering for the new mentions field. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/api/serializers/issue.py (1)
737-747: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the reliance on
values_list("id", flat=True); it silently drives the id-based logic downstream.Because the child field's queryset is a flat
values_list,PrimaryKeyRelatedField.to_internal_valuereturns the rawid(UUID) rather than aUserinstance. The rest ofvalidatedepends on that:member_id__in=mention_user_idsand especiallyuser_id in valid_member_ids(UUID-vs-UUID). A well-meaning change toUser.objects.all()would returnUserinstances and break theincomparisons silently (mentions would just stop rendering). A short comment guarding this intent would prevent a future regression.🤖 Prompt for 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. In `@apps/api/plane/api/serializers/issue.py` around lines 737 - 747, Add a concise comment above the `mentions` field documenting that `values_list("id", flat=True)` intentionally makes `PrimaryKeyRelatedField` return UUID ids, which downstream `validate` logic relies on for `member_id__in` and `user_id in valid_member_ids`; warn against changing it to a queryset of `User` instances.
🤖 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.
Nitpick comments:
In `@apps/api/plane/api/serializers/issue.py`:
- Around line 737-747: Add a concise comment above the `mentions` field
documenting that `values_list("id", flat=True)` intentionally makes
`PrimaryKeyRelatedField` return UUID ids, which downstream `validate` logic
relies on for `member_id__in` and `user_id in valid_member_ids`; warn against
changing it to a queryset of `User` instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 214cbf45-6a6c-4d65-8c08-05bc71176f41
📒 Files selected for processing (4)
apps/api/plane/api/serializers/issue.pyapps/api/plane/api/views/issue.pyapps/api/plane/tests/contract/api/test_issue_comment_mentions.pyapps/api/plane/utils/openapi/examples.py
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/api/plane/api/serializers/issue.py (1)
743-753: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAdd a length cap to
mentions
mentionsaccepts an unbounded list, so a single request can trigger a large number of validation queries and generate oversizedcomment_html. Setmax_lengthon theListFieldto bound the payload.🤖 Prompt for 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. In `@apps/api/plane/api/serializers/issue.py` around lines 743 - 753, Update the mentions ListField declaration to include an appropriate max_length limit, bounding the number of user IDs accepted while preserving its existing validation and optional behavior.
🤖 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.
Nitpick comments:
In `@apps/api/plane/api/serializers/issue.py`:
- Around line 743-753: Update the mentions ListField declaration to include an
appropriate max_length limit, bounding the number of user IDs accepted while
preserving its existing validation and optional behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 83be16a1-f9a7-4ef3-9f5f-6c4dea2bad83
📒 Files selected for processing (5)
apps/api/plane/api/serializers/issue.pyapps/api/plane/api/views/issue.pyapps/api/plane/tests/contract/api/conftest.pyapps/api/plane/tests/contract/api/test_issue_comment_mentions.pyapps/api/plane/utils/openapi/examples.py
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/api/plane/tests/contract/api/test_issue_comment_mentions.py
Add a first-class `mentions` field to the public REST API comment create/update serializer. The server renders the internal <mention-component> markup into comment_html so mentioned members are notified without callers hand-crafting the markup, and the ids remain parseable back out of comment_html. The list is capped (max_length=100) so a single comment cannot render unbounded mention markup. Also fix the public comment endpoints so notifications actually fire: - pass notification=True (and origin) on comment create/update/delete, matching the internal app API (issue_activity defaults notification=False) - dispatch the persisted comment (IssueCommentSerializer) as requested_data so the activity carries the comment id and rendered markup, without which the notification pipeline's comment-mention branch was skipped Sanitize comment_html on the public write path with validate_html_content (after mention rendering), matching the internal app serializer so external clients cannot store unsafe/invalid HTML; the whitelisted mention markup survives sanitization. Only active, non-bot project members can be mentioned; unknown ids are rejected and non-members are ignored. Adds contract tests (including an end-to-end assertion that a Notification row is created and that unsafe HTML is stripped) and updates the OpenAPI create/update examples. Also reset the public API rate limiter between contract tests (tests/contract/api/conftest.py): ApiKeyRateThrottle counts requests per API key in the shared cache, and the suite reuses one fixed token, so the per-run count accumulated across tests and tripped the 60/minute limit (HTTP 429) once enough request-making tests were added. Signed-off-by: Seena Fallah <seenafallah@gmail.com>
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.
Description
Add a first-class
mentionsfield to the public REST API comment create/update serializer. The server renders the internal markup into comment_html so mentioned members are notified without callers hand-crafting the markup, and the ids remain parseable back out of comment_html.Also fix the public comment endpoints so notifications actually fire:
Only active, non-bot project members can be mentioned; unknown ids are rejected and non-members are ignored. Adds contract tests (including an end-to-end assertion that a Notification row is created) and updates the OpenAPI example.
Type of Change
Summary by CodeRabbit
New Features
mentionsfield for issue comment create/update to render mention markup intocomment_html.Bug Fixes
Documentation
mentions.Tests