feat/assignation-event#639
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis pull request introduces thread assignment functionality, adding Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ 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)
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 |
jbpenrath
force-pushed
the
feat/assignation-event
branch
2 times, most recently
from
b34fe1f to
9a1281a
Compare
As a follow-up of the mention feature, we build upon ThreadEvent & UserEvent models a feature to assign users to a thread. We allow to filter mailboxe's inbox through assignation state (assigned to me, unassigned). The thread share modal has been forked from ui-kit to be able to list users of each mailbox and add a cta to assign them to the thread. A section above shows assigned users.
jbpenrath
force-pushed
the
feat/assignation-event
branch
from
9a1281a to
2673725
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/backend/core/tests/api/test_provisioning_mailbox.py (1)
200-212:⚠️ Potential issue | 🟡 MinorKeep the test intent in sync with the asserted fields.
The assertion now requires
is_identity, but the docstring still omits it.📝 Proposed fix
def test_response_fields(self, client, auth_header, mailbox): - """Response includes id, email, name, role, and users.""" + """Response includes id, email, name, role, users, and is_identity."""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_provisioning_mailbox.py` around lines 200 - 212, Update the test_response_fields docstring to reflect the actual asserted response keys: change the docstring in the test_response_fields function so it mentions that the response includes id, email, name, role, users, and is_identity to keep the test intent consistent with the assertion that checks for {"id", "email", "name", "role", "users", "is_identity"}.src/backend/core/tests/api/test_thread_event.py (1)
675-1064:⚠️ Potential issue | 🟠 MajorRestore the misplaced read-mention and viewer-IM assertions.
test_read_mention_viewer_accessis empty, whiletest_im_viewer_forbiddencurrently exercisesread_mentioninstead of posting an IM. The IM POST check at Lines 1059-1064 is also folded into the unauthenticated read-mention test, so the D-02 viewer regression is not covered.🧪 Proposed fix outline
def test_read_mention_viewer_access(self, api_client): """A viewer can acknowledge their own mention (no edit access required).""" + user, _mailbox, thread = setup_user_with_thread_access( + role=enums.ThreadAccessRoleChoices.VIEWER + ) + api_client.force_authenticate(user=user) + + event = factories.ThreadEventFactory(thread=thread, author=user) + user_event = factories.UserEventFactory( + user=user, + thread=thread, + thread_event=event, + type=enums.UserEventTypeChoices.MENTION, + ) + + response = api_client.patch(get_read_mention_url(thread.id, event.id)) + + assert response.status_code == status.HTTP_204_NO_CONTENT + user_event.refresh_from_db() + assert user_event.read_at is not None @@ def test_im_viewer_forbidden(self, api_client): """Viewer role POST type=im returns 403 — D-02 regression test. @@ - event = factories.ThreadEventFactory(thread=thread, author=user) - user_event = factories.UserEventFactory( - user=user, - thread=thread, - thread_event=event, - type=enums.UserEventTypeChoices.MENTION, - ) - - response = api_client.patch(get_read_mention_url(thread.id, event.id)) - - assert response.status_code == status.HTTP_204_NO_CONTENT - user_event.refresh_from_db() - assert user_event.read_at is not None + data = {"type": "im", "data": {"content": "test"}} + response = api_client.post(get_thread_event_url(thread.id), data, format="json") + assert response.status_code == status.HTTP_403_FORBIDDEN @@ - data = {"type": "im", "data": {"content": "test"}} - response = api_client.post(get_thread_event_url(thread.id), data, format="json") - assert response.status_code in ( - status.HTTP_401_UNAUTHORIZED, - status.HTTP_403_FORBIDDEN, - )As per coding guidelines, “Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases”.
🤖 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.py` around lines 675 - 1064, The tests misplaced assertions: implement test_read_mention_viewer_access to authenticate a viewer via setup_user_with_thread_access(role=enums.ThreadAccessRoleChoices.VIEWER), create a ThreadEvent and corresponding UserEvent for that viewer, call PATCH get_read_mention_url(thread.id, event.id) and assert HTTP_204_NO_CONTENT and that the UserEvent.read_at was set; update test_im_viewer_forbidden to authenticate a viewer, POST an IM to get_thread_event_url(thread.id) with data {"type":"im","data":{"content":"..."}} and assert HTTP_403_FORBIDDEN (remove the read-mention behavior from this test); finally restore the unauthenticated POST IM assertions back to test_read_mention_unauthenticated by keeping the existing unauthenticated POST to get_thread_event_url and asserting 401/403 using get_thread_event_url.src/backend/core/api/openapi.json (2)
6037-6052:⚠️ Potential issue | 🟡 MinorAlign
stats_fieldsschema and description with the accepted values.The description advertises many fields not present in
THREAD_STATS_FIELDS_MAP(src/backend/core/enums.py:89-98), and the scalarstringenum cannot represent documented comma-separated examples likeall,all_unread.🛠️ Proposed OpenAPI update
"schema": { - "type": "string", - "enum": [ - "all", - "all_unread", - "has_assigned_to_me", - "has_delivery_failed", - "has_delivery_pending", - "has_mention", - "has_unassigned", - "has_unread_mention" - ] + "type": "array", + "items": { + "type": "string", + "enum": [ + "all", + "all_unread", + "has_assigned_to_me", + "has_delivery_failed", + "has_delivery_pending", + "has_mention", + "has_unassigned", + "has_unread_mention" + ] + } }, - "description": "Comma-separated list of fields to aggregate.\n Special values: 'all' (count all threads), 'all_unread' (count all unread threads).\n Boolean fields: has_trashed, has_draft, has_starred, has_attachments, has_archived,\n has_sender, has_active, has_delivery_pending, has_delivery_failed, is_spam, has_messages, has_unread_mention, has_mention, has_assigned_to_me, has_unassigned.\n Unread variants ('_unread' suffix): count threads where the condition is true AND the thread is unread.\n Examples: 'all,all_unread', 'has_starred,has_starred_unread', 'is_spam,is_spam_unread'", + "description": "Comma-separated list of fields to aggregate. Allowed values: all, all_unread, has_assigned_to_me, has_delivery_failed, has_delivery_pending, has_mention, has_unassigned, has_unread_mention. Examples: all,all_unread or has_assigned_to_me,has_unassigned",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 6037 - 6052, The OpenAPI param "stats_fields" currently defines a scalar string enum and a description that lists many values not present in THREAD_STATS_FIELDS_MAP and shows comma-separated examples; update the schema to match the actual accepted values by replacing the scalar enum with either (preferred) an array of strings (type: array, items: { type: string, enum: <values from THREAD_STATS_FIELDS_MAP plus "all" and "all_unread"> }) so the spec supports multiple selected fields, or if keeping a single string, remove the enum and add a pattern validating comma-separated tokens drawn from THREAD_STATS_FIELDS_MAP plus the two special values; also revise the description to only list the exact accepted tokens (reference THREAD_STATS_FIELDS_MAP and the special values "all" and "all_unread") and update the examples to reflect either an array payload or comma-separated format consistent with the chosen schema.
5582-5628:⚠️ Potential issue | 🟠 MajorDocument the idempotent
200response.Line 5582 says
ASSIGN/UNASSIGNcan return200, but the operation only declares201. Generated clients may treat the idempotent no-op response as undocumented or fail to deserialize it.🛠️ Proposed OpenAPI update
"responses": { + "200": { + "content": { + "application/json": { + "schema": { + "$ref": "#/components/schemas/ThreadEvent" + } + } + }, + "description": "Idempotent no-op or partially filtered assignment request." + }, "201": { "content": { "application/json": {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 5582 - 5628, The OpenAPI operation for the thread-events endpoint documents idempotent ASSIGN/UNASSIGN behavior in the description but only declares a 201 response; add a documented 200 response to match the described no-op behavior. Update the operation's "responses" to include a "200" entry (alongside the existing "201") with a clear "description" stating it's the idempotent no-op for ASSIGN/UNASSIGN and include the appropriate content/schema (either no content if the endpoint returns empty, or the same application/json -> $ref: "#/components/schemas/ThreadEvent" if it returns the existing ThreadEvent). Reference the existing ThreadEventRequest and ThreadEvent schemas when adding the 200 response so generated clients can deserialize the no-op response.
🧹 Nitpick comments (6)
src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/assigned-users-section.tsx (2)
38-46: Use a semantic/icon button for "Remove" for accessibility and visual consistency.Other UI in this codebase uses Cunningham's
Button(or an icon button with anaria-label) rather than a bare<button>styled as a text link. As written, the "Remove" control has no explicit accessible name beyond its visible text (fine) but also no keyboard-focus styling unless provided by the SCSS, and no visual consistency with other destructive actions in the share modal.🤖 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-view/components/share-modal-extensions/assigned-users-section.tsx` around lines 38 - 46, Replace the bare <button> used for "Remove" in the AssignedUsersSection with the project's Cunningham Button (or an IconButton) to match visual and accessibility patterns: use the Button component (or an icon-only button) with the destructive/variant prop, provide an explicit aria-label like `aria-label={`Remove ${user.name}`}` if using an icon-only control, keep the onClick handler as onUnassign(user.id), remove reliance on the raw CSS focus handling on the element (you can keep the share-modal-extensions__assigned__remove class if you need styling but prefer the Button's focus styles), and ensure the control is only rendered when canUpdate is true so behavior of AssignedUsersSection/onUnassign remains unchanged.
22-27: Incomplete plural fallback setup indefaultValueoption.When
Assigned to {{count}} peopleis missing from translations,defaultValue_onealone will render "Assigned to 1 person" forcount=1, butcount=2+will render the raw translation key sincedefaultValue_otheris not defined. Either add bothdefaultValue_oneanddefaultValue_other, or dropdefaultValue_oneand rely on the locale files.Suggested fix
{t('Assigned to {{count}} people', { count: assignedUsers.length, - defaultValue_one: 'Assigned to {{count}} person', })}🤖 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-view/components/share-modal-extensions/assigned-users-section.tsx` around lines 22 - 27, The i18n call in the AssignedUsersSection component (the t(...) invocation rendering the h3) uses only defaultValue_one, causing untranslated plural counts >1 to show the raw key; fix by providing a complete plural fallback—either add defaultValue_other alongside defaultValue_one (e.g., "Assigned to {{count}} people") or remove defaultValue_one and rely on the locale files so the pluralization works correctly for both singular and plural counts.src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
97-114: Prefer a runtime-guarded narrowing overascast.
event.data as ThreadEventAssigneesData | nullbypasses type safety if a non-assignment event slips intocomputeAssignmentNetChange. SinceisAssignmentEventalready narrowsevent.datatoThreadEventAssigneesDataon the typed event, filtering (or asserting) at the function boundary would preserve correctness without the cast.- for (const event of events) { - const data = event.data as ThreadEventAssigneesData | null; - const assignees = data?.assignees ?? []; + for (const event of events) { + if (!isAssignmentEvent(event)) continue; + const assignees = event.data.assignees ?? [];🤖 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-view/components/thread-event/index.tsx` around lines 97 - 114, computeAssignmentNetChange currently uses an unsafe cast "event.data as ThreadEventAssigneesData | null"; instead, filter or narrow the incoming events using the existing type guard isAssignmentEvent (or equivalent guard) so event.data is statically known to be ThreadEventAssigneesData; update computeAssignmentNetChange to first derive assignmentEvents = events.filter(isAssignmentEvent) and iterate that array (or early-return non-assignment events) so you can remove the cast and safely read assignees from event.data while keeping the rest of the logic (net Map updates and incoming determination based on ThreadEventTypeEnum) unchanged.src/backend/core/signals.py (1)
685-698: Potential performance concern on access revocation.When a user loses
MAILBOX_ROLES_CAN_EDIT, this iterates every thread they have an active ASSIGN on and creates oneThreadEvent(UNASSIGN)per thread — each cascading throughpost_save→delete_assign_user_events. For a user assigned to many threads in a busy mailbox, a single role change or delete triggers a sizable burst of DB writes and signal traffic on the request path.If this becomes a hot scenario, consider moving the cleanup to an async task (Celery) or batching the UNASSIGN events. Low priority — fine for now given the expected frequency of mailbox-access changes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/signals.py` around lines 685 - 698, The current synchronous _cleanup_threads_for_mailbox_user iterates threads and calls cleanup_invalid_assignments, which causes per-thread ThreadEvent(UNASSIGN) writes and triggers delete_assign_user_events synchronously; instead, change _cleanup_threads_for_mailbox_user to enqueue an asynchronous worker (e.g., a Celery task) that receives the mailbox_id and user_id (or the list of thread IDs) and performs the cleanup off-request. Implement a task like cleanup_assignments_task(mailbox_id, user_id) or cleanup_assignments_task(thread_ids, user_id) which loads affected threads and either (a) batches UNASSIGN ThreadEvent creation with bulk_create and suppresses/defers signal handling, or (b) calls cleanup_invalid_assignments within the background task so delete_assign_user_events and related signal work run asynchronously; update callers of _cleanup_threads_for_mailbox_user to use the new enqueue call.src/backend/core/api/serializers.py (2)
48-50: Align these docstrings with the runtime validation path.These serializers are now used by
ThreadEventSerializer.validate(), so the “OpenAPI-only / not used for runtime validation” wording is stale. Also wrap Line 86 to stay within the 100-character limit.Suggested docstring cleanup
- OpenAPI-only serializer: describes a single user inside - an ThreadEvent.data payload. (used for ``IM`` and ``ASSIGNEES`` events) + Serializer describing a single user inside a ``ThreadEvent.data`` payload + for ``IM`` and assignation events. - Not used for runtime validation (handled by ``ThreadEvent.clean()`` against - ``ThreadEvent.DATA_SCHEMAS``); exists solely to produce a named component - in the OpenAPI schema consumed by the generated frontend client. + Used by ``ThreadEventSerializer.validate()`` for request validation and by + drf-spectacular to produce a named OpenAPI component consumed by the + generated frontend client.- """OpenAPI-only serializer: shape of ``ThreadEvent.data`` for ``ASSIGN`` and ``UNASSIGN`` events. + """Serializer describing ``ThreadEvent.data`` for assignation events.- Runtime validation is performed by ``ThreadEvent.clean()`` against - ``ThreadEvent.DATA_SCHEMAS``. The ``@extend_schema_field`` decorator only + Runtime validation is performed by ``ThreadEventSerializer.validate()`` and + ``ThreadEvent.clean()``. The ``@extend_schema_field`` decorator onlyAs per coding guidelines,
src/backend/**/*.py: Follow Django/PEP 8 style with a 100-character line limit.Also applies to: 68-70, 86-93, 120-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 48 - 50, Update the outdated docstrings in src/backend/core/api/serializers.py to reflect that these serializers are used for runtime validation by ThreadEventSerializer.validate() (remove the “OpenAPI-only / not used for runtime validation” wording and, if relevant, reference ThreadEvent.DATA_SCHEMAS or ThreadEvent.clean() only as historical/secondary). Edit the docstrings around the noted ranges (including the block containing "Not used for runtime validation..." and the other docstrings at 68-70, 86-93, and 120-121) to state that the serializers participate in validation at runtime and to clarify their role for OpenAPI generation; also wrap any long lines (notably the line at ~86) to respect the 100-character limit per PEP8. Ensure references to ThreadEventSerializer.validate(), ThreadEvent.DATA_SCHEMAS, and any serializer class names remain accurate.
1181-1183: Consider validating againstThreadEvent.DATA_SCHEMASat the serializer layer for better error handling.The current serializer-level validation (lines 1181–1183) only validates declared fields through the sub-serializer's
is_valid(). DRF serializers by default ignore extra keys in the payload. While these extra keys are caught and rejected byThreadEvent.clean()during model save (sinceBaseModel.save()callsfull_clean()), validation occurs too late for optimal UX—the error surfaces as a genericValidationErrorfromfull_clean()rather than field-level validation errors.The existing test
test_create_im_event_rejects_extra_fieldsconfirms that requests with unexpected fields indatado fail with HTTP 400, but moving thejsonschemavalidation to the serializer'svalidate()method would produce cleaner field-level error responses at the API layer and align with the schemas'additionalProperties: falseconstraint.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 1181 - 1183, The serializer should perform jsonschema validation against ThreadEvent.DATA_SCHEMAS inside its validate() so extra keys are reported as field-level errors rather than surfacing later in ThreadEvent.clean(); update the serializer that builds data_serializer via data_serializer_cls (referenced as data_serializer and data_serializer_cls) to, in its validate(self, attrs) method, look up the appropriate schema from ThreadEvent.DATA_SCHEMAS for attrs["type"] (or the type key used), run jsonschema validation against attrs["data"] with additionalProperties enforced, and if validation fails raise serializers.ValidationError({"data": <schema errors>}) before calling data_serializer.is_valid(), ensuring consistent field-level error responses.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/permissions.md`:
- Around line 15-21: Add the language identifier "text" to the fenced ASCII
diagram block so markdownlint MD040 is satisfied: replace the opening triple
backticks before the diagram with ```text and ensure the closing triple
backticks remains; target the fenced block containing "Utilisateur
──(MailboxAccess)──▶ Mailbox ──(ThreadAccess)──▶ Thread" and the related lines.
In `@src/backend/core/api/serializers.py`:
- Around line 706-728: The users field returned by ThreadAccessDetailSerializer
via get_users is leaking PII for list endpoints; modify get_users (used by
ThreadAccessDetailSerializer and surfaced by ThreadSerializer.get_accesses) to
only populate and serialize assignable users when the serializer context
indicates a detail view or an explicit opt-in (e.g., check
self.context.get("detail") or a request query param like
"include_assignable_users"); otherwise return an empty list or None. Ensure you
reference get_users and ThreadAccessDetailSerializer when adding the guard so
list responses no longer emit per-thread mailbox user payloads unless explicitly
requested.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 161-232: The code accepts assignees_data entries that failed
uuid.UUID parsing and later calls uuid.UUID(...) on them causing 500s; fix by
normalizing/sanitizing assignees_data right after the initial loop that builds
assignee_ids: produce a cleaned_assignees list (only entries where
uuid.UUID(a["id"]) succeeds) and replace
serializer.validated_data["data"]["assignees"], assignees_data, and assignee_ids
with that cleaned list/IDs so all later uses (new_assignees filtering,
new_assignee_ids, remaining_data, and the _absorb_unassign_in_undo_window call)
operate only on validated entries; also change the early 400 response to raise
serializers.ValidationError (or rest_framework.exceptions.ValidationError)
instead of returning a raw dict to match the later error style.
In
`@src/backend/core/migrations/0026_userevent_usrevt_user_thread_assign_uniq.py`:
- Around line 13-16: The UniqueConstraint definition line is over the 100-char
limit; update the migrations.AddConstraint call (model_name='userevent') so the
models.UniqueConstraint parameters are broken across multiple lines (e.g., put
constraint=models.UniqueConstraint( on its own line, split
condition=models.Q(('type','assign')) and fields=('user','thread') onto separate
lines, and keep name='usrevt_user_thread_assign_uniq' on its own line) so the
constraint declaration (models.UniqueConstraint) and its arguments conform to
the 100-character line limit while preserving the same semantics.
In `@src/backend/core/models.py`:
- Around line 1628-1629: The JSON Schema "format": "uuid" is only enforced when
a FormatChecker is provided to jsonschema.validate; update the validation in
ThreadEvent.clean() (and any other model-level validation using the schema with
"format": "uuid") to pass a jsonschema.FormatChecker (e.g.,
format_checker=FormatChecker()) so UUID formats are actually checked and invalid
mention/assignee IDs are rejected; ensure you import jsonschema.FormatChecker
and use it in the validate(...) calls that reference the schema containing "id":
{"type":"string","format":"uuid"}.
In `@src/frontend/public/locales/common/en-US.json`:
- Around line 208-222: The JSON contains placeholder values for many translation
keys (e.g., "components.share.modalTitle", "components.share.shareButton",
"components.share.user.placeholder",
"components.share.members.title_plural_one", etc.) so the UI will show raw keys;
replace each placeholder value under the components.share.* keys with proper
English strings (readable labels/messages) appropriate for the share modal and
members/invitations UI, ensuring plural variants
(title_plural_one/title_plural_other and title_singular_*) map to correct
singular/plural forms and that modalAriaLabel, modalTitle, shareButton, user
placeholders and error/no_result messages are human-readable and consistent with
the app’s tone.
In `@src/frontend/public/locales/common/fr-FR.json`:
- Line 747: The translation for the "Unassigned" filter label is using plural
masculine ("Non assignés"); update the value for the "Unassigned" JSON key to
the singular masculine form matching other labels (e.g., change the value of
"Unassigned" to "Non assigné") so it follows the same mailbox-filter/bucket
label pattern as "Starred", "Unread", and "Read".
- Around line 262-278: The listed non-plural i18n keys in fr-FR.json
(components.share.access.delete, components.share.cannot_view.message,
components.share.invitations.title, components.share.item.add,
components.share.members.load_more, components.share.modalAriaLabel,
components.share.modalTitle, components.share.search.group_name,
components.share.shareButton, components.share.user.no_result,
components.share.user.placeholder) are empty and will render raw keys in the UI;
update this file by providing proper French translations for each of those keys
or remove them from fr-FR.json if the strings should come from the UI kit
namespace so the share modal displays localized text instead of literal keys.
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-role-dropdown.tsx`:
- Around line 40-53: The dropdown construction in the useMemo for options
currently adds a delete menu item when canDelete is true even if onDelete is
undefined, resulting in an enabled action with no handler; update the logic in
the options useMemo (the constant named options built from roles, onDelete,
canDelete) to only include the separator and the delete DropdownMenuItem when
onDelete is provided (and canDelete is true), so omit/hide the delete item
whenever onDelete is falsy; keep the rest of the items and dependency array
intact (roles, onDelete, t, canDelete).
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/invitation-user-selector.tsx`:
- Around line 68-81: The remove button in InvitationUserSelectorItem is
icon-only and needs an accessible label: update the Button component (inside
InvitationUserSelectorItem) to include an aria-label that identifies the user,
e.g. use user.full_name || user.email to build a string like "Remove {userName}"
so screen readers know which user will be removed when onRemoveUser is invoked.
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-member-item.tsx`:
- Around line 37-39: canDelete currently derives only from accessData flags and
may be true even when no delete handler is provided; update the logic in
share-member-item.tsx (the canDelete computation used by AccessRoleDropdown) to
also require the presence of a delete handler (deleteAccess) before enabling
delete UI, i.e. ensure canDelete checks deleteAccess is a function/defined in
addition to is_explicit and can_delete; apply the same fix to the duplicate
computation around the other occurrence referenced (lines 63-64).
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx`:
- Around line 237-279: The filter for searchMemberResult uses reference equality
via pendingInvitationUsers.includes(user), causing duplicates; change the
predicate in the useMemo that computes searchMemberResult (inside usersData) to
filter out users whose id appears in pendingInvitationUsers (e.g.,
pendingInvitationUsers.some(p => p.id === user.id)) so comparisons use unique
identifiers instead of object references; update any related predicates in this
block that rely on includes to use id-based matching to keep behavior
consistent.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx`:
- Around line 189-203: dispatchAssignEvent and handleUnassignUser currently only
call invalidateThreadEvents and invalidateThreadsStats after createThreadEvent,
leaving the threads list stale; update both functions so their onSuccess
handlers also invalidate the threads list (same key used elsewhere, e.g., the
["threads"] query) after the event completes—add a call to the existing threads
invalidation helper or queryClient.invalidateQueries(["threads"]) alongside
invalidateThreadEvents() and invalidateThreadsStats() in the onSuccess blocks of
createThreadEvent within dispatchAssignEvent and handleUnassignUser.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/upgrade-mailbox-role-modal.tsx`:
- Around line 24-25: The modal can render with blank placeholders because
userLabel and mailboxLabel fall back to empty strings when user or access are
null; update the UpgradeMailboxRoleModal component
(upgrade-mailbox-role-modal.tsx) to guard its render by returning null (or not
rendering the Dialog) when !user || !access so the dialog body never flashes
with empty interpolations, and remove or postpone computing
userLabel/mailboxLabel until after that guard.
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 6037-6052: The OpenAPI param "stats_fields" currently defines a
scalar string enum and a description that lists many values not present in
THREAD_STATS_FIELDS_MAP and shows comma-separated examples; update the schema to
match the actual accepted values by replacing the scalar enum with either
(preferred) an array of strings (type: array, items: { type: string, enum:
<values from THREAD_STATS_FIELDS_MAP plus "all" and "all_unread"> }) so the spec
supports multiple selected fields, or if keeping a single string, remove the
enum and add a pattern validating comma-separated tokens drawn from
THREAD_STATS_FIELDS_MAP plus the two special values; also revise the description
to only list the exact accepted tokens (reference THREAD_STATS_FIELDS_MAP and
the special values "all" and "all_unread") and update the examples to reflect
either an array payload or comma-separated format consistent with the chosen
schema.
- Around line 5582-5628: The OpenAPI operation for the thread-events endpoint
documents idempotent ASSIGN/UNASSIGN behavior in the description but only
declares a 201 response; add a documented 200 response to match the described
no-op behavior. Update the operation's "responses" to include a "200" entry
(alongside the existing "201") with a clear "description" stating it's the
idempotent no-op for ASSIGN/UNASSIGN and include the appropriate content/schema
(either no content if the endpoint returns empty, or the same application/json
-> $ref: "#/components/schemas/ThreadEvent" if it returns the existing
ThreadEvent). Reference the existing ThreadEventRequest and ThreadEvent schemas
when adding the 200 response so generated clients can deserialize the no-op
response.
In `@src/backend/core/tests/api/test_provisioning_mailbox.py`:
- Around line 200-212: Update the test_response_fields docstring to reflect the
actual asserted response keys: change the docstring in the test_response_fields
function so it mentions that the response includes id, email, name, role, users,
and is_identity to keep the test intent consistent with the assertion that
checks for {"id", "email", "name", "role", "users", "is_identity"}.
In `@src/backend/core/tests/api/test_thread_event.py`:
- Around line 675-1064: The tests misplaced assertions: implement
test_read_mention_viewer_access to authenticate a viewer via
setup_user_with_thread_access(role=enums.ThreadAccessRoleChoices.VIEWER), create
a ThreadEvent and corresponding UserEvent for that viewer, call PATCH
get_read_mention_url(thread.id, event.id) and assert HTTP_204_NO_CONTENT and
that the UserEvent.read_at was set; update test_im_viewer_forbidden to
authenticate a viewer, POST an IM to get_thread_event_url(thread.id) with data
{"type":"im","data":{"content":"..."}} and assert HTTP_403_FORBIDDEN (remove the
read-mention behavior from this test); finally restore the unauthenticated POST
IM assertions back to test_read_mention_unauthenticated by keeping the existing
unauthenticated POST to get_thread_event_url and asserting 401/403 using
get_thread_event_url.
---
Nitpick comments:
In `@src/backend/core/api/serializers.py`:
- Around line 48-50: Update the outdated docstrings in
src/backend/core/api/serializers.py to reflect that these serializers are used
for runtime validation by ThreadEventSerializer.validate() (remove the
“OpenAPI-only / not used for runtime validation” wording and, if relevant,
reference ThreadEvent.DATA_SCHEMAS or ThreadEvent.clean() only as
historical/secondary). Edit the docstrings around the noted ranges (including
the block containing "Not used for runtime validation..." and the other
docstrings at 68-70, 86-93, and 120-121) to state that the serializers
participate in validation at runtime and to clarify their role for OpenAPI
generation; also wrap any long lines (notably the line at ~86) to respect the
100-character limit per PEP8. Ensure references to
ThreadEventSerializer.validate(), ThreadEvent.DATA_SCHEMAS, and any serializer
class names remain accurate.
- Around line 1181-1183: The serializer should perform jsonschema validation
against ThreadEvent.DATA_SCHEMAS inside its validate() so extra keys are
reported as field-level errors rather than surfacing later in
ThreadEvent.clean(); update the serializer that builds data_serializer via
data_serializer_cls (referenced as data_serializer and data_serializer_cls) to,
in its validate(self, attrs) method, look up the appropriate schema from
ThreadEvent.DATA_SCHEMAS for attrs["type"] (or the type key used), run
jsonschema validation against attrs["data"] with additionalProperties enforced,
and if validation fails raise serializers.ValidationError({"data": <schema
errors>}) before calling data_serializer.is_valid(), ensuring consistent
field-level error responses.
In `@src/backend/core/signals.py`:
- Around line 685-698: The current synchronous _cleanup_threads_for_mailbox_user
iterates threads and calls cleanup_invalid_assignments, which causes per-thread
ThreadEvent(UNASSIGN) writes and triggers delete_assign_user_events
synchronously; instead, change _cleanup_threads_for_mailbox_user to enqueue an
asynchronous worker (e.g., a Celery task) that receives the mailbox_id and
user_id (or the list of thread IDs) and performs the cleanup off-request.
Implement a task like cleanup_assignments_task(mailbox_id, user_id) or
cleanup_assignments_task(thread_ids, user_id) which loads affected threads and
either (a) batches UNASSIGN ThreadEvent creation with bulk_create and
suppresses/defers signal handling, or (b) calls cleanup_invalid_assignments
within the background task so delete_assign_user_events and related signal work
run asynchronously; update callers of _cleanup_threads_for_mailbox_user to use
the new enqueue call.
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/assigned-users-section.tsx`:
- Around line 38-46: Replace the bare <button> used for "Remove" in the
AssignedUsersSection with the project's Cunningham Button (or an IconButton) to
match visual and accessibility patterns: use the Button component (or an
icon-only button) with the destructive/variant prop, provide an explicit
aria-label like `aria-label={`Remove ${user.name}`}` if using an icon-only
control, keep the onClick handler as onUnassign(user.id), remove reliance on the
raw CSS focus handling on the element (you can keep the
share-modal-extensions__assigned__remove class if you need styling but prefer
the Button's focus styles), and ensure the control is only rendered when
canUpdate is true so behavior of AssignedUsersSection/onUnassign remains
unchanged.
- Around line 22-27: The i18n call in the AssignedUsersSection component (the
t(...) invocation rendering the h3) uses only defaultValue_one, causing
untranslated plural counts >1 to show the raw key; fix by providing a complete
plural fallback—either add defaultValue_other alongside defaultValue_one (e.g.,
"Assigned to {{count}} people") or remove defaultValue_one and rely on the
locale files so the pluralization works correctly for both singular and plural
counts.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 97-114: computeAssignmentNetChange currently uses an unsafe cast
"event.data as ThreadEventAssigneesData | null"; instead, filter or narrow the
incoming events using the existing type guard isAssignmentEvent (or equivalent
guard) so event.data is statically known to be ThreadEventAssigneesData; update
computeAssignmentNetChange to first derive assignmentEvents =
events.filter(isAssignmentEvent) and iterate that array (or early-return
non-assignment events) so you can remove the cast and safely read assignees from
event.data while keeping the rest of the logic (net Map updates and incoming
determination based on ThreadEventTypeEnum) unchanged.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6f84c0ae-9051-4234-9efc-df19fbb318ed
⛔ Files ignored due to path filters (26)
share-modal-target-ui.pngis excluded by!**/*.pngsrc/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_light.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_thread_event_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_thread_event_request_data_one_of.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/patched_thread_event_request_data_one_of_mentions_item.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_access_detail.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_assignees_data.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_assignees_data_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_data.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_data_one_of.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_data_one_of_mentions_item.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_data_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_im_data.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_im_data_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_request_data_one_of.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_request_data_one_of_mentions_item.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_type_enum.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_user.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_user_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_stats_fields.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (41)
docs/permissions.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/migrations/0026_userevent_usrevt_user_thread_assign_uniq.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_thread_access.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_filter_assignment.pysrc/backend/core/tests/models/test_thread_event.pysrc/backend/core/tests/models/test_user_event.pysrc/backend/core/tests/test_signals.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/assignees-widget/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/assignees-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-role-dropdown.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/assigned-users-section.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/index.tssrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/invitation-user-selector.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-member-item.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/upgrade-mailbox-role-modal.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/group-assignment-events.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-assigned-users.tssrc/frontend/src/hooks/use-is-shared-context.tssrc/frontend/src/styles/main.scss
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/backend/core/tests/api/test_thread_event.py (1)
698-1007:⚠️ Potential issue | 🟠 MajorRestore the misplaced viewer tests.
test_read_mention_viewer_accesscurrently has no assertions, whiletest_im_viewer_forbiddenis exercising the read-mention endpoint and expecting204. This leaves the viewer-forbidden IM create regression untested.🧪 Proposed test split
def test_read_mention_viewer_access(self, api_client): """A viewer can acknowledge their own mention (no edit access required).""" + user, _mailbox, thread = setup_user_with_thread_access( + role=enums.ThreadAccessRoleChoices.VIEWER + ) + api_client.force_authenticate(user=user) + + event = factories.ThreadEventFactory(thread=thread, author=user) + user_event = factories.UserEventFactory( + user=user, + thread=thread, + thread_event=event, + type=enums.UserEventTypeChoices.MENTION, + ) + + response = api_client.patch(get_read_mention_url(thread.id, event.id)) + + assert response.status_code == status.HTTP_204_NO_CONTENT + user_event.refresh_from_db() + assert user_event.read_at is not None @@ def test_im_viewer_forbidden(self, api_client): """Viewer role POST type=im returns 403 — D-02 regression test. After switching ThreadEventViewSet to use HasThreadEditAccess for create, the existing IM permission enforcement must still work. """ user, _mailbox, thread = setup_user_with_thread_access( role=enums.ThreadAccessRoleChoices.VIEWER ) api_client.force_authenticate(user=user) - event = factories.ThreadEventFactory(thread=thread, author=user) - user_event = factories.UserEventFactory( - user=user, - thread=thread, - thread_event=event, - type=enums.UserEventTypeChoices.MENTION, - ) - - response = api_client.patch(get_read_mention_url(thread.id, event.id)) - - assert response.status_code == status.HTTP_204_NO_CONTENT - user_event.refresh_from_db() - assert user_event.read_at is not None + response = api_client.post( + get_thread_event_url(thread.id), + {"type": "im", "data": {"content": "test"}}, + format="json", + ) + + assert response.status_code == status.HTTP_403_FORBIDDEN🤖 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.py` around lines 698 - 1007, The test file has misplaced/merged tests: test_read_mention_viewer_access is empty while test_im_viewer_forbidden actually performs the read-mention patch and asserts 204/mark-read behavior, leaving the "IM create forbidden for VIEWER" regression untested; restore the intended split by moving the read-mention assertions (using get_read_mention_url, factories.ThreadEventFactory, factories.UserEventFactory and checking user_event.read_at) back into test_read_mention_viewer_access and change test_im_viewer_forbidden to instead attempt creating an IM ThreadEvent as a VIEWER (authenticate with role=enums.ThreadAccessRoleChoices.VIEWER) and assert a 403 on POST to get_thread_event_url for type="im" to re-establish the IM-create-forbidden regression test.src/backend/core/signals.py (1)
332-336:⚠️ Potential issue | 🟠 MajorAvoid logging raw assignee/user identifiers.
raw_idis client-provided and can contain PII, and the valid UUID logs still expose user identifiers. Keep the event/context in the log, but omit the user value.Privacy-safe log messages
logger.warning( - "Skipping user with invalid UUID '%s' in ThreadEvent %s", - raw_id, + "Skipping user with invalid UUID in ThreadEvent %s", thread_event.id, ) @@ logger.warning( - "Skipping user %s in ThreadEvent %s: " - "user not found or no thread access", - user_id, + "Skipping user in ThreadEvent %s: user not found or no thread access", thread_event.id, ) @@ logger.warning( - "Skipping user %s in ThreadEvent %s: " - "user does not have edit rights on the thread", - user_id, + "Skipping user in ThreadEvent %s: user does not have edit rights on the thread", thread_event.id, ) @@ logger.warning( - "Skipping unassign with invalid UUID '%s' in context %s", - raw_id, + "Skipping unassign with invalid UUID in context %s", context, )As per coding guidelines, “Do not log sensitive information (tokens, passwords, financial/health data, PII)”.
Also applies to: 365-399, 539-543
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/signals.py` around lines 332 - 336, The logger.warning call in the ThreadEvent handling (logger.warning(..., raw_id, thread_event.id)) exposes client-provided PII; change the message to omit or redact the user/assignee value and only include safe context such as the ThreadEvent id and a short reason (e.g., "invalid UUID" or "redacted user id"), update the logger.warning invocation(s) that reference raw_id (and any similar calls handling assignee/user identifiers) to remove the raw_id parameter and include thread_event.id and a non-sensitive descriptor instead; locate uses by looking for logger.warning lines that reference raw_id, ThreadEvent, or the assignee/user parsing logic and apply the same redaction pattern across the other occurrences mentioned (the other logger.warning/logger.info calls handling user IDs).src/backend/core/api/openapi.json (2)
5576-5621:⚠️ Potential issue | 🟠 MajorDocument the idempotent
204response.Line 5576 says
ASSIGN/UNASSIGNcan return204, but the operation only declares201. Generated clients and contract tests may treat the successful no-op path as unexpected.Proposed OpenAPI fix
"responses": { "201": { "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ThreadEvent" } } }, "description": "" + }, + "204": { + "description": "No effective assignment change; no response body." } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 5576 - 5621, The operation that creates a ThreadEvent (tag "thread-events" with requestBody schema ThreadEventRequest) only declares a 201 response but the spec text describes an idempotent no-op path that returns 204 for ASSIGN/UNASSIGN — add a "204" response entry alongside the existing "201" under "responses", set description to indicate an idempotent/no-content result (e.g. "No content — no changes were necessary for ASSIGN/UNASSIGN"), and leave content empty (no response body) so generated clients and contract tests treat the no-op success as a valid response.
6030-6049:⚠️ Potential issue | 🟠 MajorExpand the enum to include all documented and supported field tokens.
The
stats_fieldsparameter description documents many more fields and their_unreadvariants than are present in the schema enum. For example, the description listshas_starredandhas_draftas valid boolean fields and mentionshas_starred_unreadandis_spam_unreadas valid examples, but none of these appear in the current enum. This causes strict OpenAPI clients to reject documented valid calls.Either enumerate all backend-supported tokens (e.g.,
"has_starred","has_starred_unread","has_draft","has_draft_unread", etc.) or change the parameter to an array type withstyle: formandexplode: falseto properly represent comma-separated values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 6030 - 6049, The stats_fields OpenAPI parameter's enum is incomplete versus its description (see "stats_fields" schema in src/backend/core/api/openapi.json) causing clients to reject valid comma-separated tokens; update the schema to match the documented backend-supported tokens by either (A) expanding the "enum" to include every supported token and their "_unread" variants (e.g., "has_starred","has_starred_unread","has_draft","has_draft_unread","is_spam","is_spam_unread", etc.) or (B) change the parameter schema from "type":"string" with enum to "type":"array" with "items": {"type":"string"} plus style:"form" and explode:false to represent comma-separated values—pick one approach and ensure the description and examples remain accurate and consistent with the chosen schema.
♻️ Duplicate comments (2)
src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx (2)
282-355:⚠️ Potential issue | 🟠 MajorInvalidate the thread list after assignment changes.
Assign/unassign changes the data behind the “assigned to me” and “unassigned” thread filters, but these handlers only invalidate events and stats. The thread list can remain stale until another refetch happens.
Proposed fix
onSuccess: async () => { await invalidateThreadEvents(); await invalidateThreadsStats(); + await queryClient.invalidateQueries({ queryKey: ["threads"] }); }, @@ onSuccess: async () => { await invalidateThreadEvents(); await invalidateThreadsStats(); + await queryClient.invalidateQueries({ queryKey: ["threads"] }); },🤖 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-view/components/thread-accesses-widget/index.tsx` around lines 282 - 355, The assign/unassign handlers only call invalidateThreadEvents and invalidateThreadsStats but do not refresh the thread list, causing stale “assigned to me”/“unassigned” filters; update dispatchAssignEvent (inside its onSuccess) and the createThreadEvent call in handleUnassignUser to also call the thread-list invalidation function (e.g., invalidateThreads or whatever your threads list invalidator is named) so the UI refetches the thread list after assignments/unassignments; ensure the new invalidation is awaited alongside invalidateThreadEvents and invalidateThreadsStats in both dispatchAssignEvent and the onSuccess of handleUnassignUser.
101-129:⚠️ Potential issue | 🟠 MajorRefresh the source that actually renders the access rows.
The access mutations patch/invalidate the
/accesses/query, but the rendered rows, delete guards, search exclusion, and button count still come from theaccessesprop. After create/update/delete, the modal can keep showing stale access rows until whatever ownsaccessesrefetches. Either derivenormalizedAccessesfrom the patched access-query data where possible, or invalidate the thread/list query that feedsaccesseson these mutation successes.Possible fix direction
onSuccess: (_data, vars) => { invalidateThreadMessages(); patchAccessesCache((prev) => prev.filter((a) => a.id !== vars.id)); + queryClient.invalidateQueries({ queryKey: ["threads"] }); }, @@ onSuccess: () => { invalidateThreadMessages(); if (selectedThread?.id) { queryClient.invalidateQueries({ queryKey: getThreadsAccessesListQueryKey(selectedThread.id), }); } + queryClient.invalidateQueries({ queryKey: ["threads"] }); }, @@ onSuccess: (data) => { invalidateThreadMessages(); patchAccessesCache((prev) => prev.map((a) => a.id === data.data.id ? { ...a, role: data.data.role } : a, ), ); + queryClient.invalidateQueries({ queryKey: ["threads"] }); },Also applies to: 177-195, 209-233, 259-279
🤖 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-view/components/thread-accesses-widget/index.tsx` around lines 101 - 129, The component is rendering rows from the accesses prop which stays stale after mutations; in the onSuccess handlers for removeMutation, createMutation and updateMutation update the actual source used to render rows (either derive normalizedAccesses from the cached query data or force-refresh that query). Specifically, in removeMutation.onSuccess, createMutation.onSuccess and updateMutation.onSuccess update the queryClient cache for getThreadsAccessesListQueryKey(selectedThread.id) (or call queryClient.invalidateQueries for that key) instead of only calling patchAccessesCache, and ensure the component uses normalizedAccesses derived from the queryClient/getThreadsAccessesListQueryKey cache when available rather than the original accesses prop so delete guards, search exclusion and button counts reflect the latest state.
🧹 Nitpick comments (2)
src/backend/core/api/openapi.json (1)
9216-9242: Use a discriminator pattern to coupleThreadEvent.typetoThreadEvent.datain the OpenAPI schema.The current schema allows any
ThreadEventDatavariant with any eventtype(e.g.,type: "assign"paired with IM-shaped{content}data). While runtime validation prevents invalid combinations, generated clients lack type safety for this coupling. Replace the independenttypeanddataproperties with a top-leveloneOfdiscriminator for each type variant (im, assign, unassign), where each variant fixes itstypefield and references the matching data schema. Apply the same pattern toThreadEventRequestand bothThreadEventDataandThreadEventDataRequest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 9216 - 9242, Replace the current loose schema for ThreadEvent (and ThreadEventRequest) that defines independent "type" (ThreadEventTypeEnum) and "data" (ThreadEventData) properties with a top-level oneOf discriminated union: create separate schemas for each concrete variant (e.g., ThreadEventIm, ThreadEventAssign, ThreadEventUnassign) that each fix "type" to the corresponding enum literal and reference the matching data schema (ThreadEventDataIm, ThreadEventDataAssign, ThreadEventDataUnassign); add an OpenAPI discriminator on "type" that maps enum values (im, assign, unassign) to those variant schemas so generated clients get correct typings; repeat the same transformation for ThreadEventRequest and for ThreadEventData / ThreadEventDataRequest so request vs response variants preserve readOnly/etc semantics while coupling each type to its precise data schema, and update any $ref links to use the new variant names.src/backend/core/tests/test_signals.py (1)
689-716: Optional: deduplicate_setup_thread_with_assigned_userhelper.
_setup_thread_with_assigned_userinTestAssignUserEvents(lines 689–716) andTestDeleteAssignUserEvents(lines 936–955) are byte-identical, andTestCleanupInvalidAssignments._setup_assigned_user(lines 1088–1121) differs only by the trailingThreadEventFactory(... type="assign" ...). Consider extracting a module-level fixture (or pytest fixture) so future changes to the assignment policy only need to land in one place.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/test_signals.py` around lines 689 - 716, Duplicate helper logic in TestAssignUserEvents._setup_thread_with_assigned_user, TestDeleteAssignUserEvents._setup_thread_with_assigned_user, and TestCleanupInvalidAssignments._setup_assigned_user should be consolidated: extract a single module-level helper function or pytest fixture (e.g., setup_thread_with_assigned_user_fixture) that creates author, mailbox, thread, gives ADMIN MailboxAccess to author and target_user and EDITOR ThreadAccess on the thread, returns (author, target_user, thread, mailbox), and add an optional flag/parameter to create the trailing ThreadEventFactory(... type="assign" ...) when needed; then update the three methods to call the new helper/fixture so all tests share the same setup code.
🤖 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/api/serializers.py`:
- Around line 1113-1128: get_users currently only filters out mailbox-level
VIEWERs but still returns users who are viewer-only on the thread; update
get_users to also require the thread-level access role be EDITOR. Concretely,
inside get_users check both mailbox access (access.role !=
models.MailboxRoleChoices.VIEWER) AND that the thread has an Editor access for
that user (e.g. instance.accesses.filter(user=access.user).exists() with role ==
models.ThreadAccessRoleChoices.EDITOR or check access from instance.accesses
directly), then sort and serialize only those users; reference get_users,
instance.mailbox.accesses, instance.accesses, models.MailboxRoleChoices and
models.ThreadAccessRoleChoices when making the change.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 204-232: The .exists() check should operate only on assignees who
are currently assigned: first query models.UserEvent for ASSIGN rows with
thread=thread and user_id__in=assignee_ids to get the set of active assigned
user IDs, then narrow assignees_data, assignee_ids and
serializer.validated_data["data"]["assignees"] to that set (returning 204 if
empty) before calling _absorb_unassign_in_undo_window and before emitting the
UNASSIGN path; update the logic around
models.UserEvent.objects.filter(...).exists() to instead filter and assign the
active IDs and proceed with the existing undo/emit flow using those narrowed
values (references: _absorb_unassign_in_undo_window, serializer.validated_data,
models.UserEvent filter and enums.UserEventTypeChoices.ASSIGN).
In `@src/frontend/public/locales/common/en-US.json`:
- Around line 554-555: The current pluralization uses the `count` selector but
the sentence portion "…{{sharedCount}} of which are shared" needs pluralization
based on `sharedCount`; update the two translation entries ("Shared between
{{count}} mailboxes, {{sharedCount}} of which are shared_one" and "Shared
between {{count}} mailboxes, {{sharedCount}} of which are shared_other") so that
the plural suffix is driven by `sharedCount` (or convert the string to ICU-style
pluralization using both counts), ensuring the form chosen reflects
`sharedCount` rather than `count`.
In `@src/frontend/public/locales/common/fr-FR.json`:
- Line 409: Update the French translation for the key "Grant editor access to
the thread?" so it includes the required French space before the question mark;
specifically replace the current value "Accorder l'accès en édition à la
conversation?" with one that has a non‑breaking space before the question mark
(use \u00A0 in the JSON to preserve the space), e.g. "Accorder l'accès en
édition à la conversation\u00A0?" ensuring the JSON string remains valid.
- Around line 617-619: The French translations for the keys "Shared between
{{count}} mailboxes, {{sharedCount}} of which are shared_one", "Shared between
{{count}} mailboxes, {{sharedCount}} of which are shared_many", and "Shared
between {{count}} mailboxes, {{sharedCount}} of which are shared_other"
incorrectly agree the adjective with {{count}} instead of {{sharedCount}; fix by
rewriting the phrase so the agreement depends on {{sharedCount}} (e.g., change
to a neutral construction or to "Parmi {{count}} boîtes, {{sharedCount}}
est/sont partagé(s)" or split into forms that pluralize on {{sharedCount}}),
update the three translation entries accordingly so the grammatical number
matches the sharedCount variable.
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx`:
- Around line 11-18: The debounced search timer in the share modal can fire
after the modal unmounts; locate the debounce logic in the ShareModal component
(and the similar debounce block around lines 195-224) and store the timer id in
a ref (e.g., searchDebounceRef) when calling setTimeout, then
clearTimeout(searchDebounceRef.current) in a useEffect cleanup (and when the
modal is closed) to cancel any pending callback and avoid using stale state.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx`:
- Around line 171-187: Currently the code calls createAssignEvent.mutateAsync
once per mention, fragmenting the timeline and risking partial assignments;
change it to send a single createAssignEvent.mutateAsync call that includes all
active mentions in the data.assignees array (map activeMentions to objects like
{id, name}) with type ThreadEventTypeEnum.assign and the same threadId (keep the
existing activeMentions.length check and error handling around
createAssignEvent.mutateAsync).
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5576-5621: The operation that creates a ThreadEvent (tag
"thread-events" with requestBody schema ThreadEventRequest) only declares a 201
response but the spec text describes an idempotent no-op path that returns 204
for ASSIGN/UNASSIGN — add a "204" response entry alongside the existing "201"
under "responses", set description to indicate an idempotent/no-content result
(e.g. "No content — no changes were necessary for ASSIGN/UNASSIGN"), and leave
content empty (no response body) so generated clients and contract tests treat
the no-op success as a valid response.
- Around line 6030-6049: The stats_fields OpenAPI parameter's enum is incomplete
versus its description (see "stats_fields" schema in
src/backend/core/api/openapi.json) causing clients to reject valid
comma-separated tokens; update the schema to match the documented
backend-supported tokens by either (A) expanding the "enum" to include every
supported token and their "_unread" variants (e.g.,
"has_starred","has_starred_unread","has_draft","has_draft_unread","is_spam","is_spam_unread",
etc.) or (B) change the parameter schema from "type":"string" with enum to
"type":"array" with "items": {"type":"string"} plus style:"form" and
explode:false to represent comma-separated values—pick one approach and ensure
the description and examples remain accurate and consistent with the chosen
schema.
In `@src/backend/core/signals.py`:
- Around line 332-336: The logger.warning call in the ThreadEvent handling
(logger.warning(..., raw_id, thread_event.id)) exposes client-provided PII;
change the message to omit or redact the user/assignee value and only include
safe context such as the ThreadEvent id and a short reason (e.g., "invalid UUID"
or "redacted user id"), update the logger.warning invocation(s) that reference
raw_id (and any similar calls handling assignee/user identifiers) to remove the
raw_id parameter and include thread_event.id and a non-sensitive descriptor
instead; locate uses by looking for logger.warning lines that reference raw_id,
ThreadEvent, or the assignee/user parsing logic and apply the same redaction
pattern across the other occurrences mentioned (the other
logger.warning/logger.info calls handling user IDs).
In `@src/backend/core/tests/api/test_thread_event.py`:
- Around line 698-1007: The test file has misplaced/merged tests:
test_read_mention_viewer_access is empty while test_im_viewer_forbidden actually
performs the read-mention patch and asserts 204/mark-read behavior, leaving the
"IM create forbidden for VIEWER" regression untested; restore the intended split
by moving the read-mention assertions (using get_read_mention_url,
factories.ThreadEventFactory, factories.UserEventFactory and checking
user_event.read_at) back into test_read_mention_viewer_access and change
test_im_viewer_forbidden to instead attempt creating an IM ThreadEvent as a
VIEWER (authenticate with role=enums.ThreadAccessRoleChoices.VIEWER) and assert
a 403 on POST to get_thread_event_url for type="im" to re-establish the
IM-create-forbidden regression test.
---
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx`:
- Around line 282-355: The assign/unassign handlers only call
invalidateThreadEvents and invalidateThreadsStats but do not refresh the thread
list, causing stale “assigned to me”/“unassigned” filters; update
dispatchAssignEvent (inside its onSuccess) and the createThreadEvent call in
handleUnassignUser to also call the thread-list invalidation function (e.g.,
invalidateThreads or whatever your threads list invalidator is named) so the UI
refetches the thread list after assignments/unassignments; ensure the new
invalidation is awaited alongside invalidateThreadEvents and
invalidateThreadsStats in both dispatchAssignEvent and the onSuccess of
handleUnassignUser.
- Around line 101-129: The component is rendering rows from the accesses prop
which stays stale after mutations; in the onSuccess handlers for removeMutation,
createMutation and updateMutation update the actual source used to render rows
(either derive normalizedAccesses from the cached query data or force-refresh
that query). Specifically, in removeMutation.onSuccess, createMutation.onSuccess
and updateMutation.onSuccess update the queryClient cache for
getThreadsAccessesListQueryKey(selectedThread.id) (or call
queryClient.invalidateQueries for that key) instead of only calling
patchAccessesCache, and ensure the component uses normalizedAccesses derived
from the queryClient/getThreadsAccessesListQueryKey cache when available rather
than the original accesses prop so delete guards, search exclusion and button
counts reflect the latest state.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 9216-9242: Replace the current loose schema for ThreadEvent (and
ThreadEventRequest) that defines independent "type" (ThreadEventTypeEnum) and
"data" (ThreadEventData) properties with a top-level oneOf discriminated union:
create separate schemas for each concrete variant (e.g., ThreadEventIm,
ThreadEventAssign, ThreadEventUnassign) that each fix "type" to the
corresponding enum literal and reference the matching data schema
(ThreadEventDataIm, ThreadEventDataAssign, ThreadEventDataUnassign); add an
OpenAPI discriminator on "type" that maps enum values (im, assign, unassign) to
those variant schemas so generated clients get correct typings; repeat the same
transformation for ThreadEventRequest and for ThreadEventData /
ThreadEventDataRequest so request vs response variants preserve readOnly/etc
semantics while coupling each type to its precise data schema, and update any
$ref links to use the new variant names.
In `@src/backend/core/tests/test_signals.py`:
- Around line 689-716: Duplicate helper logic in
TestAssignUserEvents._setup_thread_with_assigned_user,
TestDeleteAssignUserEvents._setup_thread_with_assigned_user, and
TestCleanupInvalidAssignments._setup_assigned_user should be consolidated:
extract a single module-level helper function or pytest fixture (e.g.,
setup_thread_with_assigned_user_fixture) that creates author, mailbox, thread,
gives ADMIN MailboxAccess to author and target_user and EDITOR ThreadAccess on
the thread, returns (author, target_user, thread, mailbox), and add an optional
flag/parameter to create the trailing ThreadEventFactory(... type="assign" ...)
when needed; then update the three methods to call the new helper/fixture so all
tests share the same setup code.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 383728d5-a8f5-490a-bde7-787df0dc168e
⛔ Files ignored due to path filters (12)
src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_thread_access_list.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_access.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_assignees_data.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_assignees_data_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_im_data.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_im_data_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_user.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_user_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_accesses_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-access/thread-access.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (23)
docs/permissions.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread_access.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_thread_access.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/models/test_thread_event.pysrc/backend/core/tests/test_signals.pysrc/backend/core/utils.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-role-dropdown.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/invitation-user-selector.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-member-item.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx
✅ Files skipped from review due to trivial changes (1)
- src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scss
🚧 Files skipped from review as they are similar to previous changes (4)
- src/backend/core/tests/api/test_provisioning_mailbox.py
- src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-member-item.tsx
- src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-role-dropdown.tsx
- src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsx
Comment on lines
+171
to
+187
| if (activeMentions.length > 0) { | ||
| try { | ||
| await Promise.all( | ||
| activeMentions.map((mention) => | ||
| createAssignEvent.mutateAsync({ | ||
| threadId, | ||
| data: { | ||
| type: ThreadEventTypeEnum.assign, | ||
| data: { | ||
| assignees: [ | ||
| { id: mention.id, name: mention.name }, | ||
| ], | ||
| }, | ||
| }, | ||
| }) | ||
| ) | ||
| ); |
There was a problem hiding this comment.
Create one ASSIGN event for all active mentions.
The backend accepts multiple assignees in one event. Sending one mutation per mention fragments the timeline and can leave partial assignments if one request fails.
🔧 Proposed fix
if (activeMentions.length > 0) {
try {
- await Promise.all(
- activeMentions.map((mention) =>
- createAssignEvent.mutateAsync({
- threadId,
- data: {
- type: ThreadEventTypeEnum.assign,
- data: {
- assignees: [
- { id: mention.id, name: mention.name },
- ],
- },
- },
- })
- )
- );
+ await createAssignEvent.mutateAsync({
+ threadId,
+ data: {
+ type: ThreadEventTypeEnum.assign,
+ data: {
+ assignees: activeMentions.map((mention) => ({
+ id: mention.id,
+ name: mention.name,
+ })),
+ },
+ },
+ });
} catch {
// Assignment failures are non-critical -- the IM was already posted
}
}📝 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.
Suggested change
| if (activeMentions.length > 0) { | |
| try { | |
| await Promise.all( | |
| activeMentions.map((mention) => | |
| createAssignEvent.mutateAsync({ | |
| threadId, | |
| data: { | |
| type: ThreadEventTypeEnum.assign, | |
| data: { | |
| assignees: [ | |
| { id: mention.id, name: mention.name }, | |
| ], | |
| }, | |
| }, | |
| }) | |
| ) | |
| ); | |
| if (activeMentions.length > 0) { | |
| try { | |
| await createAssignEvent.mutateAsync({ | |
| threadId, | |
| data: { | |
| type: ThreadEventTypeEnum.assign, | |
| data: { | |
| assignees: activeMentions.map((mention) => ({ | |
| id: mention.id, | |
| name: mention.name, | |
| })), | |
| }, | |
| }, | |
| }); | |
| } catch { | |
| // Assignment failures are non-critical -- the IM was already posted | |
| } | |
| } |
🤖 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-view/components/thread-event-input/index.tsx`
around lines 171 - 187, Currently the code calls createAssignEvent.mutateAsync
once per mention, fragmenting the timeline and risking partial assignments;
change it to send a single createAssignEvent.mutateAsync call that includes all
active mentions in the data.assignees array (map activeMentions to objects like
{id, name}) with type ThreadEventTypeEnum.assign and the same threadId (keep the
existing activeMentions.length check and error handling around
createAssignEvent.mutateAsync).
47e2814 to
b04b07a
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx (1)
113-167:⚠️ Potential issue | 🟡 Minor
has_assigned_to_meis not combined with other active filters in the count label.Since
has_assigned_to_mecan be toggled independently alongsidehas_unread/has_starred/has_mention(seeTHREAD_PANEL_FILTER_PARAMS), stacking it with any other filter causes an earlier branch to short-circuit and the "assigned to you" qualifier is dropped from the label (e.g., ahas_unread+has_assigned_to_meselection still renders "{{count}} unread results"). The label stops matching the filter set actually being applied.This mirrors how
has_mentionalready combines withhas_unread/has_starred. Consider either (a) composing the label from the active filters instead of an exhaustive if-chain, or (b) at minimum explicitly handling thehas_assigned_to_me+ other-filter combinations for consistency.🤖 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 113 - 167, The countLabel useMemo currently builds labels via an exhaustive if-chain and misses combinations that include activeFilters.has_assigned_to_me (so e.g. has_unread + has_assigned_to_me drops the "assigned to you" qualifier); update the logic in the useMemo that defines countLabel to include has_assigned_to_me when other filters are present. Best fix: replace the long if-chain with a small compositional builder that inspects activeFilters (has_mention, has_unread, has_starred, has_assigned_to_me), builds the qualifier parts (e.g. "unread", "starred", "mentioning you", "assigned to you") in a consistent order, then calls t('{{count}} <qualifier> results|messages', { count: threads?.count, defaultValue_one: ... }) based on isSearch so all combinations (including ones with has_assigned_to_me) are rendered correctly.src/backend/core/api/viewsets/thread.py (1)
420-462:⚠️ Potential issue | 🟠 MajorMissing
annotation_fieldsentries for the two new annotation-backed filters —_unreadvariants will crash at query time.
has_assigned_to_meandhas_unassignedare exposed only as annotations (_has_assigned_to_me/_has_unassigned), not as real model columns. Validation currently letsstats_fields=has_assigned_to_me_unread(orhas_unassigned_unread) through becauseannotation_fieldsstill only contains the mention fields. The request then falls through to the generic_unreadbranch at lines 501–506 which buildsQ(has_assigned_to_me=True)against the real model — that raisesFieldError: Cannot resolve keyword 'has_assigned_to_me'when.aggregate()runs.Since they have no meaningful unread semantics anyway (UserEvent ASSIGN rows have no read state), they should be blocked the same way
has_mention/has_unread_mentionare.🛡️ Proposed fix
# Base fields that cannot be combined with the "_unread" suffix because # they are annotations (not real model columns) and their unread variant # is either already exposed (has_unread_mention) or meaningless. - annotation_fields = {"has_mention", "has_unread_mention"} + annotation_fields = { + "has_mention", + "has_unread_mention", + "has_assigned_to_me", + "has_unassigned", + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/thread.py` around lines 420 - 462, Validation currently allows stats_fields like "has_assigned_to_me_unread" and "has_unassigned_unread" which will later build Q(has_assigned_to_me=True) against model fields and crash; update the annotation_fields set (the one defined alongside valid_base_fields/special_fields) to include "has_assigned_to_me" and "has_unassigned" so their "_unread" variants are rejected the same way as "has_mention"/"has_unread_mention" during the requested_fields loop that checks field.endswith("_unread").src/backend/core/api/openapi.json (2)
5576-5622:⚠️ Potential issue | 🟠 MajorAdvertise the 204 no-op response for idempotent ASSIGN/UNASSIGN.
Line 5576 says this operation returns 204 when the request is absorbed as a no-op, but the response map still only documents 201. Generated clients will treat a valid idempotent response as unexpected.
🩹 Suggested schema fix
"responses": { + "204": { + "description": "No ThreadEvent created because the ASSIGN/UNASSIGN request was a no-op." + }, "201": { "content": { "application/json": { "schema": { "$ref": "#/components/schemas/ThreadEvent"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 5576 - 5622, The OpenAPI operation that creates a ThreadEvent must advertise the 204 no-op response so clients treat idempotent ASSIGN/UNASSIGN absorptions as valid; update the operation's "responses" map (the same object that currently lists 201 with schema "#/components/schemas/ThreadEvent") to also include a "204" response entry with an appropriate description (e.g., "No-op: all assignees already in target state") and no response body (no content or an empty content object) so generated clients accept the 204 path; ensure this change applies to the operation that references ThreadEventRequest and ThreadEvent schemas.
6030-6049:⚠️ Potential issue | 🟠 Major
stats_fieldsis modeled as a single enum value, not a comma-separated list.The description says callers can send values like
all,all_unread, buttype: "string"plusenumonly allows one exact string. That means the schema still rejects the documented multi-field form and misrepresents what the endpoint accepts.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 6030 - 6049, The stats_fields parameter currently uses "type": "string" with an enum which only permits a single literal, causing a mismatch with the documented comma-separated list; change the parameter schema to an array form that accepts multiple enum values (e.g., "type": "array", "items": { "type": "string", "enum": [...] }) and keep "style": "form" and "explode": false so callers can pass comma-separated values like "all,all_unread"; update the parameter entry for "name": "stats_fields" (and its description if needed) to use this array/items enum structure instead of a single-string enum.
♻️ Duplicate comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx (1)
282-355:⚠️ Potential issue | 🟠 MajorAlso invalidate the thread list after assign/unassign.
These handlers refresh the timeline and stats, but they never invalidate the
["threads"]query. That leaves the “assigned to me” / “unassigned” mailbox filters stale until some later refetch, even though the assignment mutation already succeeded.🛠️ Proposed fix
}, { onSuccess: async () => { await invalidateThreadEvents(); await invalidateThreadsStats(); + await queryClient.invalidateQueries({ queryKey: ["threads"] }); }, onSettled: () => options?.onSettled?.(), }); @@ }, { onSuccess: async () => { await invalidateThreadEvents(); await invalidateThreadsStats(); + await queryClient.invalidateQueries({ queryKey: ["threads"] }); }, }); };🤖 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-view/components/thread-accesses-widget/index.tsx` around lines 282 - 355, The assign/unassign flows call invalidateThreadEvents() and invalidateThreadsStats() but never invalidate the ["threads"] cache, leaving list filters stale; update dispatchAssignEvent (its onSuccess) and the createThreadEvent call in handleUnassignUser to also call the threads invalidation helper (e.g., await invalidateThreads()) so the threads query is refreshed after assign/unassign; ensure the same additional invalidateThreads() call is used in the onSuccess where invalidateThreadEvents() and invalidateThreadsStats() are currently called so both assign and unassign paths refresh the threads list.
🧹 Nitpick comments (7)
src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/invitation-user-selector.tsx (1)
71-90: LGTM — prior accessibility feedback addressed.The icon-only remove button now carries a translated
aria-labelbuilt fromdisplayName, resolving the prior review comment.Tiny nit (optional):
onRemoveUseris typed as required ((user: …) => void), so the optional chaining inonClick={() => onRemoveUser?.(user)}(Line 85) is unnecessary and can be simplified toonRemoveUser(user). Not blocking.🤖 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-view/components/share-modal-extensions/invitation-user-selector.tsx` around lines 71 - 90, The prop onRemoveUser on InvitationUserSelectorItem is typed as required, so remove the unnecessary optional chaining in the click handler; update the onClick from using onRemoveUser?.(user) to call onRemoveUser(user) directly inside the InvitationUserSelectorItem component to reflect the non-optional function signature.src/backend/core/models.py (1)
1799-1803: Consider referencing the enum value instead of the string literal"assign".Elsewhere in this file, partial unique constraints reference enum values explicitly (e.g.,
MessageTemplateusesMessageTemplateTypeChoices.AUTOREPLY.valuein itsQ(...)conditions). UsingUserEventTypeChoices.ASSIGNhere would make the constraint consistent and protect against silent drift if the enum value is ever renamed. Note that any change still requires a coordinated migration, but the symbol reference makes grep/refactor safer.♻️ Proposed tweak
models.UniqueConstraint( fields=["user", "thread"], - condition=Q(type="assign"), + condition=Q(type=UserEventTypeChoices.ASSIGN), name="usrevt_user_thread_assign_uniq", ),This would require importing
UserEventTypeChoicesfromcore.enumsat the top of the file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/models.py` around lines 1799 - 1803, Replace the literal string "assign" in the partial UniqueConstraint condition with the enum value from UserEventTypeChoices (use UserEventTypeChoices.ASSIGN.value to match existing patterns) so the Q(...) uses the enum constant instead of a hardcoded string; add an import for UserEventTypeChoices from core.enums at the top of the file and update the condition in the models.UniqueConstraint (the constraint definition using Q(type="assign")) accordingly.src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx (1)
285-303: Optional: deduplicate the assignee-names string.
thread.assigned_users.map((u) => u.name).join(', ')is built twice (Tooltip content + span aria-label). Minor DRY cleanup; also sidesteps any divergence if one call site ever changes.♻️ Optional refactor
- {thread.assigned_users.length > 0 && ( - <Tooltip - content={t('Assigned to {{names}}', { - names: thread.assigned_users.map((u) => u.name).join(', '), - })} - > - <span - aria-label={t('Assigned to {{names}}', { - names: thread.assigned_users.map((u) => u.name).join(', '), - })} - > + {thread.assigned_users.length > 0 && (() => { + const assignedLabel = t('Assigned to {{names}}', { + names: thread.assigned_users.map((u) => u.name).join(', '), + }); + return ( + <Tooltip content={assignedLabel}> + <span aria-label={assignedLabel}> <AssigneesAvatarGroup users={thread.assigned_users} maxAvatars={2} overflowMode="replace-last" /> </span> </Tooltip> - )} + ); + })()}Also worth verifying with a screen reader that the Cunningham
Tooltipdoesn't already set an accessible name on its trigger — if it does, thearia-labelon the wrapping<span>becomes redundant and can be removed entirely.🤖 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-item/index.tsx` around lines 285 - 303, The code builds the assignee-names string twice for Tooltip content and the span aria-label; extract the joined names into a single variable (e.g., const assigneeNames = thread.assigned_users.map(u => u.name).join(', ')) and reuse it in Tooltip content and the span aria-label (references: thread.assigned_users, Tooltip, span aria-label, AssigneesAvatarGroup); additionally verify whether Tooltip already exposes an accessible name and if so remove the redundant span aria-label instead of reusing it.src/frontend/src/features/utils/date-helper.ts (1)
46-74: Consider factoring out the date-branch logic shared withformatDate.
formatEventTimestampduplicates the same isToday/isYesterday/isSameWeek/isSameYear branching asformatDateabove — the only real differences are (1) time is always appended, (2)Today/Yesterday-style labels are replaced by the raw time for today. A small internal helper likeformatContextualDay(date, locale): string | nullreturning the prefix (ornullfor today) would let both methods share one source of truth and avoid drift when, for example, the locale extraction or week-start rules change.Not a blocker — current behavior is consistent with
formatDate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/utils/date-helper.ts` around lines 46 - 74, formatEventTimestamp duplicates the contextual day branching logic from formatDate; extract that shared logic into a private helper (e.g., formatContextualDay(date: Date, locale: Locale): string | null) that returns the prefix for non-today dates (like "Yesterday" or weekday/date strings) or null for today, and then update formatEventTimestamp and formatDate to call formatContextualDay and append the time only in formatEventTimestamp; ensure the helper uses the same locale extraction and isSameWeek/isSameYear/isYesterday/isToday checks so both functions share one source of truth.src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx (1)
234-242:useCallbackdependency[props]defeats memoization.Since
propsis a fresh object on every render,onSelectis re-created on every render, which also invalidates theusersDatamemo at line 296. If you want a stable reference, depend on the specific field used (props.onSearchUsers) instead — but since this mirrors upstream behavior and the file is slated for back-porting, feel free to keep the diff minimal and fix it in the ui-kit PR.♻️ Proposed fix
const onSelect = useCallback( (user: UserData<UserType>) => { setPendingInvitationUsers((prev) => [...prev, user]); setInputValue(""); setSearchQuery(""); props.onSearchUsers!(""); }, - [props], + [props.onSearchUsers], );🤖 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-view/components/share-modal-extensions/share-modal.tsx` around lines 234 - 242, The onSelect callback is being recreated every render because its useCallback dependency is the whole props object; change the dependency to the specific function used (props.onSearchUsers) so onSelect remains stable and doesn't invalidate downstream memoization (usersData); update the dependency array of the useCallback that defines onSelect to reference props.onSearchUsers (or destructure onSearchUsers from props and depend on that) while leaving the existing body that calls setPendingInvitationUsers, setInputValue, setSearchQuery, and props.onSearchUsers("") unchanged.src/backend/core/api/openapi.json (1)
5278-5281: Call out the breaking response-shape change forthreads/{thread_id}/accesses.This switches the endpoint from a paginated envelope to a raw array, so any generated client or pagination handling will need an explicit migration note before release.
Based on learnings, in
src/backend/core/api/openapi.json, allow breaking API changes prior to a stable release, but ensure consumers are notified and updated, and document the migration path (including versioning notes) so downstream services can adapt before the release.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` around lines 5278 - 5281, The OpenAPI change for the threads/{thread_id}/accesses response switched from a paginated envelope to a raw array of ThreadAccess, which is a breaking change for clients and generated SDKs; revert the response schema for the threads/{thread_id}/accesses operation in src/backend/core/api/openapi.json to the original paginated envelope (wrap items under the pagination schema previously used) or explicitly bump the API version and add a clear migration note in the OpenAPI description for that path indicating the change, update the components/schemas reference (ThreadAccess) usage accordingly, and ensure any client generation or pagination-handling logic is updated by adding versioning and migration documentation for downstream consumers.src/backend/core/api/serializers.py (1)
1249-1257: Minor: guard against non-dictValidationErrorshapes.
exc.message_dictonly exists when theDjangoValidationErrorwas constructed with a dict (asvalidate_json_schemadoes today). If a future schema/validator raisesValidationError("msg")or a list form, accessingmessage_dictwill raiseAttributeErrorand surface as a 500 instead of a 400. Considerexc.messagesor defensive handling:Proposed tweak
- try: - models.ThreadEvent.validate_data(event_type, attrs["data"]) - except DjangoValidationError as exc: - raise serializers.ValidationError(exc.message_dict) from exc + try: + models.ThreadEvent.validate_data(event_type, attrs["data"]) + except DjangoValidationError as exc: + detail = getattr(exc, "message_dict", None) or {"data": exc.messages} + raise serializers.ValidationError(detail) from exc🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/serializers.py` around lines 1249 - 1257, The current except block assumes DjangoValidationError has message_dict and re-raises serializers.ValidationError(exc.message_dict), which will crash if exc was constructed with a string or list; change the handler in the validate step that calls models.ThreadEvent.validate_data to defensively extract error payload (use exc.message_dict if present, otherwise exc.messages or str(exc)) and pass that into serializers.ValidationError so all ValidationError shapes (dict, list, string) map to a 400; update the except clause that references DjangoValidationError, exc.message_dict, and serializers.ValidationError accordingly.
🤖 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/api/viewsets/thread_event.py`:
- Around line 165-167: The assignee list in
serializer.validated_data["data"]["assignees"] is not normalized, so duplicate
ids like [{id: u}, {id: u}] get persisted into ThreadEvent.data.assignees even
though create_assign_user_events() only creates one UserEvent; fix by
deduplicating assignees early: compute unique UUIDs from assignees_data (replace
assignee_ids = [uuid.UUID(...)] with a deterministic de-duplicated list/set
preserving order) and use that deduped list everywhere you build the event
payload and call create_assign_user_events(); apply the same deduplication logic
to the other branches in the block covering the create paths (the code around
lines 176-223) so persisted ThreadEvent.data.assignees and created UserEvents
remain consistent.
In `@src/backend/core/tests/api/test_thread_event.py`:
- Around line 698-706: The test body for test_read_mention_viewer_access was
accidentally removed causing the following tests (including
test_im_viewer_forbidden and the code that calls PATCH /read-mention/) to be
mis-nested under the module level and the TestThreadEventAssign class; restore
the original body by moving the assertions and API call that verify viewer ACK
of their own mention back into test_read_mention_viewer_access, and ensure
TestThreadEventAssign begins only after that function (so its docstring and
test_create_assign_success remain inside the class); apply the same fix for the
duplicate occurrence around test_im_viewer_forbidden so both locations correctly
test the read-mention viewer case and the IM-create permission case
independently.
In `@src/backend/core/tests/test_signals.py`:
- Around line 865-873: The test asserts the wrong return type for
create_assign_user_events: it returns a set, not a list; update the assertion to
expect an empty set (e.g., change the assertion that checks created to assert
created == set()) so the test matches create_assign_user_events' return type and
correctly verifies the no-op behavior when given a malformed UUID.
In `@src/frontend/public/locales/common/fr-FR.json`:
- Around line 16-19: The French translations for the keys "{{author}} assigned
you and themself", "{{author}} assigned you, themself and {{assignees}}_one",
"_many", and "_other" use the gendered "lui-même" which breaks the
inclusive-writing pattern; update these values to a gender-neutral form (e.g.,
"elle/lui-même", "iel-même", or rephrase to avoid the reflexive pronoun) so they
match the existing "assigné·e"/"désassigné·e" style across the locale file;
locate the JSON entries for those exact keys and replace the "lui-même" token
with the chosen neutral alternative consistently for all four keys (and mirror
the same change for the other occurrences noted in the file).
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.ts`:
- Around line 58-66: The current branch in assignment-message.ts returns "You
were unassigned" when isSystem && !isAssign && selfInAssignees, dropping other
unassigned users; change the logic in the isSystem && !isAssign block to detect
when selfInAssignees && assignees.length > 1 and return a message like "You and
{{others}} were unassigned" (compute others by filtering assignees to exclude
the current user and joining their names), otherwise keep the singular "You were
unassigned" or the existing "{{assignees}} was unassigned" behavior; update uses
of t(...) to pass the computed others and count as needed.
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5576-5622: The OpenAPI operation that creates a ThreadEvent must
advertise the 204 no-op response so clients treat idempotent ASSIGN/UNASSIGN
absorptions as valid; update the operation's "responses" map (the same object
that currently lists 201 with schema "#/components/schemas/ThreadEvent") to also
include a "204" response entry with an appropriate description (e.g., "No-op:
all assignees already in target state") and no response body (no content or an
empty content object) so generated clients accept the 204 path; ensure this
change applies to the operation that references ThreadEventRequest and
ThreadEvent schemas.
- Around line 6030-6049: The stats_fields parameter currently uses "type":
"string" with an enum which only permits a single literal, causing a mismatch
with the documented comma-separated list; change the parameter schema to an
array form that accepts multiple enum values (e.g., "type": "array", "items": {
"type": "string", "enum": [...] }) and keep "style": "form" and "explode": false
so callers can pass comma-separated values like "all,all_unread"; update the
parameter entry for "name": "stats_fields" (and its description if needed) to
use this array/items enum structure instead of a single-string enum.
In `@src/backend/core/api/viewsets/thread.py`:
- Around line 420-462: Validation currently allows stats_fields like
"has_assigned_to_me_unread" and "has_unassigned_unread" which will later build
Q(has_assigned_to_me=True) against model fields and crash; update the
annotation_fields set (the one defined alongside
valid_base_fields/special_fields) to include "has_assigned_to_me" and
"has_unassigned" so their "_unread" variants are rejected the same way as
"has_mention"/"has_unread_mention" during the requested_fields loop that checks
field.endswith("_unread").
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Around line 113-167: The countLabel useMemo currently builds labels via an
exhaustive if-chain and misses combinations that include
activeFilters.has_assigned_to_me (so e.g. has_unread + has_assigned_to_me drops
the "assigned to you" qualifier); update the logic in the useMemo that defines
countLabel to include has_assigned_to_me when other filters are present. Best
fix: replace the long if-chain with a small compositional builder that inspects
activeFilters (has_mention, has_unread, has_starred, has_assigned_to_me), builds
the qualifier parts (e.g. "unread", "starred", "mentioning you", "assigned to
you") in a consistent order, then calls t('{{count}} <qualifier>
results|messages', { count: threads?.count, defaultValue_one: ... }) based on
isSearch so all combinations (including ones with has_assigned_to_me) are
rendered correctly.
---
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsx`:
- Around line 282-355: The assign/unassign flows call invalidateThreadEvents()
and invalidateThreadsStats() but never invalidate the ["threads"] cache, leaving
list filters stale; update dispatchAssignEvent (its onSuccess) and the
createThreadEvent call in handleUnassignUser to also call the threads
invalidation helper (e.g., await invalidateThreads()) so the threads query is
refreshed after assign/unassign; ensure the same additional invalidateThreads()
call is used in the onSuccess where invalidateThreadEvents() and
invalidateThreadsStats() are currently called so both assign and unassign paths
refresh the threads list.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5278-5281: The OpenAPI change for the threads/{thread_id}/accesses
response switched from a paginated envelope to a raw array of ThreadAccess,
which is a breaking change for clients and generated SDKs; revert the response
schema for the threads/{thread_id}/accesses operation in
src/backend/core/api/openapi.json to the original paginated envelope (wrap items
under the pagination schema previously used) or explicitly bump the API version
and add a clear migration note in the OpenAPI description for that path
indicating the change, update the components/schemas reference (ThreadAccess)
usage accordingly, and ensure any client generation or pagination-handling logic
is updated by adding versioning and migration documentation for downstream
consumers.
In `@src/backend/core/api/serializers.py`:
- Around line 1249-1257: The current except block assumes DjangoValidationError
has message_dict and re-raises serializers.ValidationError(exc.message_dict),
which will crash if exc was constructed with a string or list; change the
handler in the validate step that calls models.ThreadEvent.validate_data to
defensively extract error payload (use exc.message_dict if present, otherwise
exc.messages or str(exc)) and pass that into serializers.ValidationError so all
ValidationError shapes (dict, list, string) map to a 400; update the except
clause that references DjangoValidationError, exc.message_dict, and
serializers.ValidationError accordingly.
In `@src/backend/core/models.py`:
- Around line 1799-1803: Replace the literal string "assign" in the partial
UniqueConstraint condition with the enum value from UserEventTypeChoices (use
UserEventTypeChoices.ASSIGN.value to match existing patterns) so the Q(...) uses
the enum constant instead of a hardcoded string; add an import for
UserEventTypeChoices from core.enums at the top of the file and update the
condition in the models.UniqueConstraint (the constraint definition using
Q(type="assign")) accordingly.
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx`:
- Around line 285-303: The code builds the assignee-names string twice for
Tooltip content and the span aria-label; extract the joined names into a single
variable (e.g., const assigneeNames = thread.assigned_users.map(u =>
u.name).join(', ')) and reuse it in Tooltip content and the span aria-label
(references: thread.assigned_users, Tooltip, span aria-label,
AssigneesAvatarGroup); additionally verify whether Tooltip already exposes an
accessible name and if so remove the redundant span aria-label instead of
reusing it.
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/invitation-user-selector.tsx`:
- Around line 71-90: The prop onRemoveUser on InvitationUserSelectorItem is
typed as required, so remove the unnecessary optional chaining in the click
handler; update the onClick from using onRemoveUser?.(user) to call
onRemoveUser(user) directly inside the InvitationUserSelectorItem component to
reflect the non-optional function signature.
In
`@src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsx`:
- Around line 234-242: The onSelect callback is being recreated every render
because its useCallback dependency is the whole props object; change the
dependency to the specific function used (props.onSearchUsers) so onSelect
remains stable and doesn't invalidate downstream memoization (usersData); update
the dependency array of the useCallback that defines onSelect to reference
props.onSearchUsers (or destructure onSearchUsers from props and depend on that)
while leaving the existing body that calls setPendingInvitationUsers,
setInputValue, setSearchQuery, and props.onSearchUsers("") unchanged.
In `@src/frontend/src/features/utils/date-helper.ts`:
- Around line 46-74: formatEventTimestamp duplicates the contextual day
branching logic from formatDate; extract that shared logic into a private helper
(e.g., formatContextualDay(date: Date, locale: Locale): string | null) that
returns the prefix for non-today dates (like "Yesterday" or weekday/date
strings) or null for today, and then update formatEventTimestamp and formatDate
to call formatContextualDay and append the time only in formatEventTimestamp;
ensure the helper uses the same locale extraction and
isSameWeek/isSameYear/isYesterday/isToday checks so both functions share one
source of truth.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 04957be9-7a4b-4d44-aa96-9e33a77db394
⛔ Files ignored due to path filters (16)
src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_thread_access_list.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_access.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_assignees_data.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_assignees_data_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_im_data.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_im_data_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_user.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event_user_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_mentionable_user.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_mentionable_user_custom_attributes.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_accesses_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-access/thread-access.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-users/thread-users.tsis excluded by!**/gen/**
📒 Files selected for processing (50)
docs/permissions.mdsrc/backend/core/api/openapi.jsonsrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_access.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/api/viewsets/thread_user.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_provisioning_mailbox.pysrc/backend/core/tests/api/test_thread_access.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_filter_assignment.pysrc/backend/core/tests/api/test_thread_user.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/core/tests/models/test_thread_event.pysrc/backend/core/tests/test_signals.pysrc/backend/core/utils.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/hooks/use-thread-panel-filters.tssrc/frontend/src/features/layouts/components/thread-view/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/assignees-widget/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/assignees-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-role-dropdown.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/invitation-user-selector.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-member-item.tsxsrc/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/share-modal.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-accesses-widget/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-action-bar/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/assignment-message.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/group-system-events.test.tssrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/ui/components/assignees-avatar-group/_index.scsssrc/frontend/src/features/ui/components/assignees-avatar-group/index.test.tsxsrc/frontend/src/features/ui/components/assignees-avatar-group/index.tsxsrc/frontend/src/features/utils/date-helper.tssrc/frontend/src/styles/main.scss
✅ Files skipped from review due to trivial changes (6)
- src/frontend/src/features/layouts/components/thread-panel/components/thread-item/_index.scss
- src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/_index.scss
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss
- src/frontend/src/features/ui/components/assignees-avatar-group/_index.scss
- src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/_index.scss
- src/backend/core/signals.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/backend/core/api/viewsets/thread_access.py
- src/frontend/src/styles/main.scss
- src/frontend/src/features/layouts/components/thread-view/components/assignees-widget/index.tsx
- src/backend/core/tests/api/test_provisioning_mailbox.py
- src/backend/core/tests/models/test_thread_event.py
- src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-users-list.tsx
- src/frontend/src/features/layouts/components/thread-view/components/share-modal-extensions/access-role-dropdown.tsx
- src/backend/core/tests/api/test_thread_access.py
Comment on lines
+165
to
+167
| assignees_data = serializer.validated_data["data"]["assignees"] | ||
| assignee_ids = [uuid.UUID(a["id"]) for a in assignees_data] | ||
|
|
There was a problem hiding this comment.
Deduplicate assignees before saving the event payload.
The branch logic filters by idempotence/access, but it never normalizes duplicate user ids in a single request. A payload like [{id: u}, {id: u}] will still be persisted twice in ThreadEvent.data.assignees, while create_assign_user_events() only creates one UserEvent. That leaves duplicated assignees in the timeline/history even though the active assignment state is unique.
🛠️ Proposed fix
- assignees_data = serializer.validated_data["data"]["assignees"]
- assignee_ids = [uuid.UUID(a["id"]) for a in assignees_data]
+ raw_assignees = serializer.validated_data["data"]["assignees"]
+ seen_assignee_ids = set()
+ assignees_data = []
+ assignee_ids = []
+ for assignee in raw_assignees:
+ assignee_id = uuid.UUID(assignee["id"])
+ if assignee_id in seen_assignee_ids:
+ continue
+ seen_assignee_ids.add(assignee_id)
+ assignees_data.append(assignee)
+ assignee_ids.append(assignee_id)
+ serializer.validated_data["data"]["assignees"] = assignees_dataAlso applies to: 176-223
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/api/viewsets/thread_event.py` around lines 165 - 167, The
assignee list in serializer.validated_data["data"]["assignees"] is not
normalized, so duplicate ids like [{id: u}, {id: u}] get persisted into
ThreadEvent.data.assignees even though create_assign_user_events() only creates
one UserEvent; fix by deduplicating assignees early: compute unique UUIDs from
assignees_data (replace assignee_ids = [uuid.UUID(...)] with a deterministic
de-duplicated list/set preserving order) and use that deduped list everywhere
you build the event payload and call create_assign_user_events(); apply the same
deduplication logic to the other branches in the block covering the create paths
(the code around lines 176-223) so persisted ThreadEvent.data.assignees and
created UserEvents remain consistent.
Comment on lines
698
to
+706
| def test_read_mention_viewer_access(self, api_client): | ||
| """A viewer can acknowledge their own mention (no edit access required).""" | ||
|
|
||
|
|
||
| class TestThreadEventAssign: | ||
| """Test the POST /threads/{thread_id}/events/ endpoint for ASSIGN events.""" | ||
|
|
||
| def test_create_assign_success(self, api_client): | ||
| """POST type=assign with valid assignee returns 201 and creates UserEvent ASSIGN.""" |
There was a problem hiding this comment.
test_read_mention_viewer_access lost its body, so both regressions are testing the wrong endpoint.
TestThreadEventAssign starts right after the docstring at Line 699, which leaves test_read_mention_viewer_access() as a no-op. The code now living under test_im_viewer_forbidden() still calls PATCH /read-mention/, so the viewer-read-mention case is no longer asserted and the IM-create permission regression never runs.
Also applies to: 984-1007
🤖 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.py` around lines 698 - 706, The
test body for test_read_mention_viewer_access was accidentally removed causing
the following tests (including test_im_viewer_forbidden and the code that calls
PATCH /read-mention/) to be mis-nested under the module level and the
TestThreadEventAssign class; restore the original body by moving the assertions
and API call that verify viewer ACK of their own mention back into
test_read_mention_viewer_access, and ensure TestThreadEventAssign begins only
after that function (so its docstring and test_create_assign_success remain
inside the class); apply the same fix for the duplicate occurrence around
test_im_viewer_forbidden so both locations correctly test the read-mention
viewer case and the IM-create permission case independently.
Comment on lines
+865
to
+873
| with patch("core.signals.logger") as mock_logger: | ||
| created = create_assign_user_events( | ||
| event, | ||
| thread, | ||
| [{"id": "not-a-valid-uuid", "name": "Bad"}], | ||
| ) | ||
|
|
||
| assert created == [] | ||
| assert ( |
There was a problem hiding this comment.
This assertion expects the wrong return type.
create_assign_user_events() returns a set of assigned user ids and set() on a no-op, so assert created == [] will fail even though the helper handled the malformed UUID correctly.
🛠️ Proposed fix
- assert created == []
+ assert created == set()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/backend/core/tests/test_signals.py` around lines 865 - 873, The test
asserts the wrong return type for create_assign_user_events: it returns a set,
not a list; update the assertion to expect an empty set (e.g., change the
assertion that checks created to assert created == set()) so the test matches
create_assign_user_events' return type and correctly verifies the no-op behavior
when given a malformed UUID.
Comment on lines
+16
to
+19
| "{{author}} assigned you and themself": "{{author}} vous a assigné·e ainsi que lui-même", | ||
| "{{author}} assigned you, themself and {{assignees}}_one": "{{author}} vous a assigné·e, lui-même ainsi que {{assignees}}", | ||
| "{{author}} assigned you, themself and {{assignees}}_many": "{{author}} vous a assigné·e, lui-même ainsi que {{assignees}}", | ||
| "{{author}} assigned you, themself and {{assignees}}_other": "{{author}} vous a assigné·e, lui-même ainsi que {{assignees}}", |
There was a problem hiding this comment.
Gendered "lui-même" breaks the inclusive-writing pattern used elsewhere.
The rest of this block consistently uses assigné·e/désassigné·e (inclusive form), but lui-même (lines 16, 17–19, 31, 32–34) assumes the author is masculine. If the author is a woman, the rendered sentence mixes forms: "Alice vous a assigné·e ainsi que lui-même". Consider a neutral alternative such as elle/lui-même or a rewording that avoids the pronoun (e.g. "Alice vous a assigné·e avec elle-même/lui-même", or the neutral "ainsi qu'iel-même" if the codebase already commits to that register).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/frontend/public/locales/common/fr-FR.json` around lines 16 - 19, The
French translations for the keys "{{author}} assigned you and themself",
"{{author}} assigned you, themself and {{assignees}}_one", "_many", and "_other"
use the gendered "lui-même" which breaks the inclusive-writing pattern; update
these values to a gender-neutral form (e.g., "elle/lui-même", "iel-même", or
rephrase to avoid the reflexive pronoun) so they match the existing
"assigné·e"/"désassigné·e" style across the locale file; locate the JSON entries
for those exact keys and replace the "lui-même" token with the chosen neutral
alternative consistently for all four keys (and mirror the same change for the
other occurrences noted in the file).
Comment on lines
+58
to
+66
| if (isSystem && !isAssign) { | ||
| if (selfInAssignees) { | ||
| return t("You were unassigned"); | ||
| } | ||
| return t("{{assignees}} was unassigned", { | ||
| assignees: assignees.map((a) => a.name).join(", "), | ||
| count: assignees.length, | ||
| }); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how many assignees the system emits per UNASSIGN event on cascading cleanup.
rg -nP --type=py -C5 'UNASSIGN|unassign' -g '**/signals*.py' -g '**/handlers*.py' -g '**/services*.py' -g '**/thread*.py'Repository: suitenumerique/messages
Length of output: 24454
Fix message when system unassigns multiple users including the viewer.
When a system UNASSIGN event includes both the viewer and other users, the code returns "You were unassigned" and drops the other assignees from the message. The backend groups multiple unassigned users into a single system UNASSIGN event (see unassign_users_on_access_change() in signals.py), so this edge case occurs regularly when access permissions change. Reword to include others: "You and {{others}} were unassigned" or similar.
🤖 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-view/components/thread-event/assignment-message.ts`
around lines 58 - 66, The current branch in assignment-message.ts returns "You
were unassigned" when isSystem && !isAssign && selfInAssignees, dropping other
unassigned users; change the logic in the isSystem && !isAssign block to detect
when selfInAssignees && assignees.length > 1 and return a message like "You and
{{others}} were unassigned" (compute others by filtering assignees to exclude
the current user and joining their names), otherwise keep the singular "You were
unassigned" or the existing "{{assignees}} was unassigned" behavior; update uses
of t(...) to pass the computed others and count as needed.
sylvinus
reviewed
Apr 27, 2026
sylvinus
reviewed
Apr 27, 2026
| DATA_SCHEMAS = { | ||
| ThreadEventTypeChoices.IM: _IM_SCHEMA, | ||
| ThreadEventTypeChoices.ASSIGN: _ASSIGNEES_SCHEMA, | ||
| ThreadEventTypeChoices.UNASSIGN: _ASSIGNEES_SCHEMA, |
Member
There was a problem hiding this comment.
as we have the source of truth in UserEvent, do we need a specific type here for ASSIGN/UNASSIGN ? Couldn't we treat those as more generic "LOG" events ? when somebody did X in the thread (split, share, assign, ...)
sylvinus
reviewed
Apr 27, 2026
| # ``update_fields`` entirely, which would then silently skip cleanup. | ||
| @receiver(pre_save, sender=models.ThreadAccess) | ||
| @receiver(pre_save, sender=models.MailboxAccess) | ||
| def stash_previous_role(sender, instance, **kwargs): |
Member
There was a problem hiding this comment.
I'm a bit uneasy about managing all this in signals. Can't we do it more explicitly at the source?
sylvinus
reviewed
Apr 27, 2026
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
2 participants
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
Assign users to a thread and add mailbox filters to quickly find unassigned and self-assigned threads.
Summary by CodeRabbit
Release Notes
New Features
Documentation