🐛(global) allow thread viewers to post internal comments#632
Merged
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 22 minutes and 5 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
📝 WalkthroughWalkthroughThis PR introduces type-aware permissions for thread event creation and comment access, adds comment-specific authorization checks to multiple viewsets, updates frontend permission gating and component behavior based on thread editability, and includes styling refinements for form controls and typography. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (4)
src/backend/core/tests/api/test_thread_event_permissions.py (1)
390-411: Minor: Docstring describes a scenario not demonstrated by the test setup.The docstring states "Mailbox role dropped to VIEWER after the event was created," but the test creates the event via factory while the user already has VIEWER role from the start. The test correctly simulates the end state (author with VIEWER mailbox role cannot edit), but doesn't demonstrate the downgrade process itself.
Consider clarifying:
def test_mailbox_viewer_author_cannot_update_own_im(self, api_client): - """Mailbox role dropped to VIEWER after the event was created: - the author has lost comment rights and can no longer edit. + """Even if the author owns the IM event, they cannot edit it + without sufficient mailbox privileges (simulates role downgrade). """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_thread_event_permissions.py` around lines 390 - 411, Update the docstring of test_mailbox_viewer_author_cannot_update_own_im to reflect the actual setup: the author already has a VIEWER mailbox role when the event is created (not that the role was "dropped" after creation). Locate the test in which factories.MailboxAccessFactory assigns enums.MailboxRoleChoices.VIEWER before factories.ThreadEventFactory creates the event and replace the misleading sentence with one that states the author has VIEWER role at creation time and therefore cannot edit their "im" event.src/backend/core/api/permissions.py (1)
608-618: Consider defensive handling for malformedrequest.data.Line 609 assumes
request.datais dict-like. If a client sends malformed JSON or an unexpected content type,request.data.get("type")could raise anAttributeError. DRF typically handles this, but if you want extra safety:event_type = request.data.get("type") if isinstance(request.data, dict) else NoneThat said, DRF's content negotiation generally ensures
request.datais a dict for JSON payloads, so this may be acceptable as-is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/permissions.py` around lines 608 - 618, In the create-action branch where event_type is read (the if view.action == "create" block), guard against malformed request.data by checking its type before calling .get; replace the current event_type = request.data.get("type") if hasattr(request, "data") else None with a safe check like using isinstance(request.data, dict) (so event_type defaults to None for non-dict request.data) and keep the subsequent logic that calls _user_can_comment_on_thread(request.user, thread_id) for IM and the models.ThreadAccess.objects.editable_by(...).filter(thread_id=thread_id).exists() path for other types.src/frontend/src/styles/cunningham-tokens.css (1)
363-364: Font weight--blacknow equals--extrabold— verify intentional.Both
--c--globals--font--weights--black(line 364) and--c--globals--font--weights--extrabold(line 363) are now set to800. Traditionally, "black" is900and "extrabold" is800. If the design intent is to collapse these two weights, the tokens are now functionally identical; otherwise,--blackmay need to remain900to preserve typographic hierarchy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/styles/cunningham-tokens.css` around lines 363 - 364, The two CSS tokens --c--globals--font--weights--extrabold and --c--globals--font--weights--black are both set to 800 which collapses their semantic difference; decide whether --c--globals--font--weights--black should be the traditional 900 and update its value accordingly (change --c--globals--font--weights--black: 900;) or, if intentional, add a comment/docstring near the tokens stating that black and extrabold are unified at 800 to preserve intended hierarchy.src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx (1)
203-268: LGTM! Excellent refactoring of permission checks.Replacing inline permission checks with the derived
canArchive,canReportSpam,canTrash, andcanAssignLabelbooleans is a solid improvement:
- Eliminates code duplication (DRY principle)
- Improves readability with semantic naming
- Creates a single source of truth for permission logic
- Reduces cognitive load in the JSX rendering logic
The implementation is consistent across all four action controls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx` around lines 203 - 268, The permission-refactor is good as-is; no code changes required—keep the derived booleans canArchive, canReportSpam, canTrash, and canAssignLabel and their usage in the Tooltip/Button blocks and LabelsWidget(threadIds={Array.from(selectedThreadIds)}), retaining the existing onClick mutation calls (archiveMutation, spamMutation, trashMutation) with their onSuccess handlers that call unselectThread() and onClearSelection().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/backend/core/api/permissions.py`:
- Around line 608-618: In the create-action branch where event_type is read (the
if view.action == "create" block), guard against malformed request.data by
checking its type before calling .get; replace the current event_type =
request.data.get("type") if hasattr(request, "data") else None with a safe check
like using isinstance(request.data, dict) (so event_type defaults to None for
non-dict request.data) and keep the subsequent logic that calls
_user_can_comment_on_thread(request.user, thread_id) for IM and the
models.ThreadAccess.objects.editable_by(...).filter(thread_id=thread_id).exists()
path for other types.
In `@src/backend/core/tests/api/test_thread_event_permissions.py`:
- Around line 390-411: Update the docstring of
test_mailbox_viewer_author_cannot_update_own_im to reflect the actual setup: the
author already has a VIEWER mailbox role when the event is created (not that the
role was "dropped" after creation). Locate the test in which
factories.MailboxAccessFactory assigns enums.MailboxRoleChoices.VIEWER before
factories.ThreadEventFactory creates the event and replace the misleading
sentence with one that states the author has VIEWER role at creation time and
therefore cannot edit their "im" event.
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Around line 203-268: The permission-refactor is good as-is; no code changes
required—keep the derived booleans canArchive, canReportSpam, canTrash, and
canAssignLabel and their usage in the Tooltip/Button blocks and
LabelsWidget(threadIds={Array.from(selectedThreadIds)}), retaining the existing
onClick mutation calls (archiveMutation, spamMutation, trashMutation) with their
onSuccess handlers that call unselectThread() and onClearSelection().
In `@src/frontend/src/styles/cunningham-tokens.css`:
- Around line 363-364: The two CSS tokens --c--globals--font--weights--extrabold
and --c--globals--font--weights--black are both set to 800 which collapses their
semantic difference; decide whether --c--globals--font--weights--black should be
the traditional 900 and update its value accordingly (change
--c--globals--font--weights--black: 900;) or, if intentional, add a
comment/docstring near the tokens stating that black and extrabold are unified
at 800 to preserve intended hierarchy.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44e598eb-518b-4e5c-a588-78804959f0a6
📒 Files selected for processing (15)
src/backend/core/api/permissions.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/api/viewsets/thread_user.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/tests/api/test_thread_user.pysrc/frontend/src/features/forms/components/combobox/_index.scsssrc/frontend/src/features/forms/components/message-form/attachment-uploader.tsxsrc/frontend/src/features/forms/components/message-form/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-actions/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/styles/cunningham-tokens.csssrc/frontend/src/styles/cunningham-tokens.scsssrc/frontend/src/styles/cunningham-tokens.tssrc/frontend/src/styles/globals.scss
💤 Files with no reviewable changes (1)
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-actions/index.tsx
jbpenrath
force-pushed
the
fix/few-issues
branch
from
7906e62 to
ae731ba
Compare
Writing an internal comment is a personal authoring act that should not require thread edit rights: support teammates invited as thread viewers must still be able to comment and mention colleagues, as long as they have edit rights on the mailbox. ThreadEvent IM writes and ThreadUser listing are relaxed accordingly, while every other thread mutation keeps the full edit-rights check. The message composer is now gated by the thread edit ability so that read-only users cannot bypass the check through reply or forward, and the thread-panel selection separator is hidden when no bulk action is available. A few unrelated UI polish fixes (disabled link button style, combobox placeholder visibility) ship alongside.
jbpenrath
force-pushed
the
fix/few-issues
branch
from
ae731ba to
400a9b6
Compare
Merged
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
1 participant
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.
Purpose
Writing an internal comment is a personal authoring act that should not require thread edit rights: support teammates invited as thread viewers must still be able to comment and mention colleagues, as long as they have edit rights on the mailbox. ThreadEvent IM writes and ThreadUser listing are relaxed accordingly, while every other thread mutation keeps the full edit-rights check.
The message composer is now gated by the thread edit ability so that
read-only users cannot bypass the check through reply or forward, and
the thread-panel selection separator is hidden when no bulk action is
available. A few unrelated UI polish fixes (disabled link button
style, combobox placeholder visibility) ship alongside.
Summary by CodeRabbit
New Features
Bug Fixes
Style