✨(backend) add limit on distinct reactions per comment#1978
✨(backend) add limit on distinct reactions per comment#1978maboukerfa wants to merge 2 commits intosuitenumerique:mainfrom
Conversation
maboukerfa
force-pushed
the
feat/add-max-reaction
branch
4 times, most recently
from
0399104 to
af0eeff
Compare
lunika
left a comment
lunika
left a comment
There was a problem hiding this comment.
I only focus on the backend part.
If possible, can you split in 2 commits ? A first one for the backend and a second one for the frontend ?
Thank you so much for your contribution
Implement a configurable limit (default: 15) on the number of distinct emoji reactions per comment. - Backend validation ensures the limit cannot be exceeded via API Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
maboukerfa
force-pushed
the
feat/add-max-reaction
branch
from
af0eeff to
390085d
Compare
maboukerfa
force-pushed
the
feat/add-max-reaction
branch
from
390085d to
0356763
Compare
WalkthroughAdds a configurable limit Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@CHANGELOG.md`:
- Around line 12-14: The CHANGELOG.md is missing a blank line before the "###
Changed" heading which triggers markdownlint MD022; insert a single blank line
immediately above the "### Changed" heading (the block containing "### Changed"
and the list item "- ✨(backend) add limit on distinct reactions per comment
`#1978`") so the heading is separated from the previous paragraph.
In `@src/backend/core/api/viewsets.py`:
- Around line 2781-2805: The POST path is vulnerable to TOCTOU: acquire a DB
transaction and lock the parent comment row with select_for_update before
re-checking reactions and creating the Reaction; specifically wrap the logic
around the existing comment variable in an atomic transaction (using Django's
transaction.atomic()) and re-fetch the comment with
Comment.objects.select_for_update().get(pk=comment.pk), then perform the
existence check (models.Reaction.objects.filter(comment=comment,
emoji=emoji).exists()), re-evaluate comment.reactions.count() against
settings.REACTIONS_MAX_PER_COMMENT, and only then call
models.Reaction.objects.get_or_create(...) so the count/enforce-check and create
are done under the same lock.
In `@src/backend/impress/settings.py`:
- Around line 185-189: The REACTIONS_MAX_PER_COMMENT setting currently allows
negative or zero values; add a startup validation immediately after the
REACTIONS_MAX_PER_COMMENT declaration in src/backend/impress/settings.py that
reads the resolved integer and raises a clear configuration error (e.g.,
ValueError or Django's ImproperlyConfigured) if the value is less than 1 so the
process fails fast on bad envs like REACTIONS_MAX_PER_COMMENT=-1; reference the
REACTIONS_MAX_PER_COMMENT symbol and include the environ_name in the error
message for clarity.
In
`@src/frontend/apps/impress/src/features/docs/doc-editor/components/comments/DocsThreadStoreAuth.tsx`:
- Around line 6-10: The constructor default for maxReactions in
DocsThreadStoreAuth is out of sync with the backend (10 vs backend 15); update
the constructor parameter default (private readonly maxReactions: number = 10)
to match the backend default 15 or remove the inline default so callers (e.g.,
useComments which passes config?.REACTIONS_MAX_PER_COMMENT) always control the
value; locate the DocsThreadStoreAuth constructor and change the default
accordingly.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 113c97ad-f0ef-4f98-89da-f86045632297
📒 Files selected for processing (8)
CHANGELOG.mdsrc/backend/core/api/viewsets.pysrc/backend/core/factories.pysrc/backend/core/tests/documents/test_api_documents_comments.pysrc/backend/impress/settings.pysrc/frontend/apps/impress/src/core/config/api/useConfig.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/components/comments/DocsThreadStoreAuth.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/components/comments/useComments.ts
| if request.method == "POST": | ||
| emoji = serializer.validated_data["emoji"] | ||
|
|
||
| if ( | ||
| not models.Reaction.objects.filter( | ||
| comment=comment, emoji=emoji | ||
| ).exists() | ||
| and comment.reactions.count() >= settings.REACTIONS_MAX_PER_COMMENT | ||
| ): | ||
| return drf.response.Response( | ||
| { | ||
| "emoji": [ | ||
| _( | ||
| "A comment can have a maximum of %(max)d distinct reactions." | ||
| ) | ||
| % {"max": settings.REACTIONS_MAX_PER_COMMENT} | ||
| ] | ||
| }, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
|
|
||
| reaction, created = models.Reaction.objects.get_or_create( | ||
| comment=comment, | ||
| emoji=serializer.validated_data["emoji"], | ||
| emoji=emoji, | ||
| ) |
There was a problem hiding this comment.
Serialize the limit check with the create step.
This is still a TOCTOU race: two concurrent POSTs with different new emojis can both observe count() < REACTIONS_MAX_PER_COMMENT and both create, leaving the comment above the configured cap. Since the backend limit is the main point of this PR, the POST path needs a transaction + row lock on the parent comment before the existence/count/create sequence.
Suggested fix
if request.method == "POST":
emoji = serializer.validated_data["emoji"]
-
- if (
- not models.Reaction.objects.filter(
- comment=comment, emoji=emoji
- ).exists()
- and comment.reactions.count() >= settings.REACTIONS_MAX_PER_COMMENT
- ):
- return drf.response.Response(
- {
- "emoji": [
- _(
- "A comment can have a maximum of %(max)d distinct reactions."
- )
- % {"max": settings.REACTIONS_MAX_PER_COMMENT}
- ]
- },
- status=status.HTTP_400_BAD_REQUEST,
- )
-
- reaction, created = models.Reaction.objects.get_or_create(
- comment=comment,
- emoji=emoji,
- )
+ with transaction.atomic():
+ comment = models.Comment.objects.select_for_update().get(pk=comment.pk)
+ reactions = models.Reaction.objects.filter(comment=comment)
+
+ if (
+ not reactions.filter(emoji=emoji).exists()
+ and reactions.count() >= settings.REACTIONS_MAX_PER_COMMENT
+ ):
+ return drf.response.Response(
+ {
+ "emoji": [
+ _(
+ "A comment can have a maximum of %(max)d distinct reactions."
+ )
+ % {"max": settings.REACTIONS_MAX_PER_COMMENT}
+ ]
+ },
+ status=status.HTTP_400_BAD_REQUEST,
+ )
+
+ reaction, created = models.Reaction.objects.get_or_create(
+ comment=comment,
+ emoji=emoji,
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets.py` around lines 2781 - 2805, The POST path is
vulnerable to TOCTOU: acquire a DB transaction and lock the parent comment row
with select_for_update before re-checking reactions and creating the Reaction;
specifically wrap the logic around the existing comment variable in an atomic
transaction (using Django's transaction.atomic()) and re-fetch the comment with
Comment.objects.select_for_update().get(pk=comment.pk), then perform the
existence check (models.Reaction.objects.filter(comment=comment,
emoji=emoji).exists()), re-evaluate comment.reactions.count() against
settings.REACTIONS_MAX_PER_COMMENT, and only then call
models.Reaction.objects.get_or_create(...) so the count/enforce-check and create
are done under the same lock.
There was a problem hiding this comment.
@lunika, this is prone to race condition, a user can create a MAX+1 emoji if he spams emojis, is it worth fixing or it's an overkill ?
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
I think it's overkill and I'm not ok to add a lock on the table.
Prevent users from adding more reactions once the per-message limit has been exceeded. It Disables reaction buttons when limit is reached Signed-off-by: Mohamed El Amine BOUKERFA <boukerfa.ma@gmail.com>
maboukerfa
force-pushed
the
feat/add-max-reaction
branch
from
0356763 to
af1448b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/backend/core/api/viewsets.py (1)
2783-2803:⚠️ Potential issue | 🔴 CriticalTOCTOU race between the existence/count check and
get_or_create.Two concurrent POSTs with different new emojis can both observe
count() < REACTIONS_MAX_PER_COMMENTand both succeed, pushing the comment above the configured cap. Since the whole point of this PR is backend enforcement, the check + create should be serialized viatransaction.atomic()+select_for_update()on the parent comment (as already proposed in an earlier review). This comment has already been raised on a prior commit.Additionally, once you're inside the atomic block, you can reuse the prefetched
comment.reactions.all()vialen(...)instead of a secondCOUNTquery.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets.py` around lines 2783 - 2803, Wrap the existence/count check and the get_or_create call in a single transaction and lock the parent comment row to prevent TOCTOU races: start a transaction.atomic() and re-fetch the Comment with select_for_update() (use the same Comment instance used by the view), then check the number of distinct reactions using the prefetched comment.reactions.all() (use len(...) instead of .count()) and perform Reaction.objects.get_or_create(...) inside that atomic block; update the code paths around the POST handling in the viewset so the existence check and creation are serialized on the locked comment to enforce settings.REACTIONS_MAX_PER_COMMENT.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/tests/documents/test_api_documents_comments.py`:
- Around line 881-900: The test test_create_reaction_exceeds_maximum claims
"here we set it to 10" but never overrides settings.REACTIONS_MAX_PER_COMMENT;
update the test to explicitly set the limit using the provided settings fixture
(e.g. assign settings.REACTIONS_MAX_PER_COMMENT = 10 before creating reactions)
so the test adds exactly 10 distinct reactions and then verifies behavior, or
alternatively change the docstring to reflect that it uses the default setting;
modify the test logic in test_create_reaction_exceeds_maximum to reference the
overridden value consistently.
In
`@src/frontend/apps/impress/src/features/docs/doc-editor/components/comments/useComments.ts`:
- Line 29: The code currently uses config?.REACTIONS_MAX_PER_COMMENT ?? 0 which
sets max reactions to 0 while config is still loading and prevents adding new
distinct emoji; change the fallback to the backend default (15) or gate
rendering until config is defined: update the expression to
config?.REACTIONS_MAX_PER_COMMENT ?? 15 (or wrap the reactions UI in a check
like if (config === undefined) show loading) and ensure callers such as
canAddReaction(comment, maxPerComment) receive the non-zero default or a
not-ready state.
---
Duplicate comments:
In `@src/backend/core/api/viewsets.py`:
- Around line 2783-2803: Wrap the existence/count check and the get_or_create
call in a single transaction and lock the parent comment row to prevent TOCTOU
races: start a transaction.atomic() and re-fetch the Comment with
select_for_update() (use the same Comment instance used by the view), then check
the number of distinct reactions using the prefetched comment.reactions.all()
(use len(...) instead of .count()) and perform
Reaction.objects.get_or_create(...) inside that atomic block; update the code
paths around the POST handling in the viewset so the existence check and
creation are serialized on the locked comment to enforce
settings.REACTIONS_MAX_PER_COMMENT.
🪄 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: ASSERTIVE
Plan: Pro
Run ID: 9d3574e2-bf82-4ae5-a5c3-064e079f28e9
📒 Files selected for processing (5)
src/backend/core/api/viewsets.pysrc/backend/core/tests/documents/test_api_documents_comments.pysrc/frontend/apps/impress/src/core/config/api/useConfig.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/components/comments/DocsThreadStoreAuth.tsxsrc/frontend/apps/impress/src/features/docs/doc-editor/components/comments/useComments.ts
| def test_create_reaction_exceeds_maximum(settings): | ||
| """ | ||
| Users should not be able to add more than REACTIONS_MAX_PER_COMMENT | ||
| (here we set it to 10) distinct emoji reactions to a comment. | ||
| They should, however, be able to add themselves to an existing reaction. | ||
| """ | ||
| user1 = factories.UserFactory() | ||
| user2 = factories.UserFactory() | ||
| document = factories.DocumentFactory( | ||
| link_reach="restricted", | ||
| users=[(user1, models.RoleChoices.ADMIN), (user2, models.RoleChoices.ADMIN)], | ||
| ) | ||
| thread = factories.ThreadFactory(document=document) | ||
| comment = factories.CommentFactory(thread=thread) | ||
|
|
||
| client = APIClient() | ||
| client.force_login(user1) | ||
|
|
||
| # Add max distinct reactions | ||
| max_reactions = settings.REACTIONS_MAX_PER_COMMENT |
There was a problem hiding this comment.
Docstring/behavior mismatch: test doesn't actually set the limit to 10.
The docstring says "here we set it to 10" but the test reads settings.REACTIONS_MAX_PER_COMMENT without overriding it, so it runs against the default (15) and issues ~15 extra POSTs. Either explicitly set it to a small value via the settings fixture (which is also the point of using that fixture), or update the docstring.
🛠️ Proposed fix
def test_create_reaction_exceeds_maximum(settings):
"""
Users should not be able to add more than REACTIONS_MAX_PER_COMMENT
(here we set it to 10) distinct emoji reactions to a comment.
They should, however, be able to add themselves to an existing reaction.
"""
+ settings.REACTIONS_MAX_PER_COMMENT = 10
user1 = factories.UserFactory()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/tests/documents/test_api_documents_comments.py` around lines
881 - 900, The test test_create_reaction_exceeds_maximum claims "here we set it
to 10" but never overrides settings.REACTIONS_MAX_PER_COMMENT; update the test
to explicitly set the limit using the provided settings fixture (e.g. assign
settings.REACTIONS_MAX_PER_COMMENT = 10 before creating reactions) so the test
adds exactly 10 distinct reactions and then verifies behavior, or alternatively
change the docstring to reflect that it uses the default setting; modify the
test logic in test_create_reaction_exceeds_maximum to reference the overridden
value consistently.
| new DocsThreadStoreAuth( | ||
| encodeURIComponent(user?.full_name || ''), | ||
| canComment, | ||
| config?.REACTIONS_MAX_PER_COMMENT ?? 0, |
There was a problem hiding this comment.
Minor: ?? 0 disables all new reactions until config loads.
When config hasn't resolved yet (first load) or cached config predates this field, REACTIONS_MAX_PER_COMMENT is undefined and this falls back to 0, which gates canAddReaction(..., undefined) to false and blocks any new distinct emoji. This is briefly user-visible because useQuery seeds initialData from localStorage before the refetch completes.
Consider using the backend's documented default (15) as the fallback, or rendering the picker as loading until config is defined:
💡 Optional refactor
- config?.REACTIONS_MAX_PER_COMMENT ?? 0,
+ config?.REACTIONS_MAX_PER_COMMENT ?? 15,📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| config?.REACTIONS_MAX_PER_COMMENT ?? 0, | |
| config?.REACTIONS_MAX_PER_COMMENT ?? 15, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/frontend/apps/impress/src/features/docs/doc-editor/components/comments/useComments.ts`
at line 29, The code currently uses config?.REACTIONS_MAX_PER_COMMENT ?? 0 which
sets max reactions to 0 while config is still loading and prevents adding new
distinct emoji; change the fallback to the backend default (15) or gate
rendering until config is defined: update the expression to
config?.REACTIONS_MAX_PER_COMMENT ?? 15 (or wrap the reactions UI in a check
like if (config === undefined) show loading) and ensure callers such as
canAddReaction(comment, maxPerComment) receive the non-zero default or a
not-ready state.
2 participants
Purpose
The current implementation allows users to add an unlimited number of reactions per comment.
Because the
/threadsendpoint is called on document load, having multiple comments withthousands of reactions each could significantly impact page load performance.
Proposal
Implement a configurable limit (default: 15) on the number of distinct emoji reactions per comment.
Screen.Recording.2026-03-11.at.00.21.53.mov