✨(threadevents) add new ThreadEvent model#566
Merged
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR introduces a ThreadEvent feature enabling internal instant messaging (IM) within threads. It adds a new Changes
Sequence Diagram(s)sequenceDiagram
participant User as User<br/>(Frontend)
participant Client as Frontend<br/>App
participant API as REST API<br/>ThreadEventViewSet
participant DB as Database
participant Auth as Permission<br/>Check
User->>Client: Type `@username` to mention
Client->>API: GET /threads/{id}/users (autocomplete)
API->>Auth: Verify thread access
Auth-->>API: ✓ Allowed
API->>DB: Query users by thread
DB-->>API: Return matching users
API-->>Client: User suggestions
Client->>Client: Render popover & insert @[Name]
User->>Client: Submit message (Enter or Send)
Client->>Client: Transform @[Name] to @[Name] format
Client->>API: POST /threads/{id}/events
Note over API: { type: "im", data: {<br/>content: "text",<br/>mentions: [{id, name}]<br/>}}
API->>Auth: Verify user thread access<br/>(EDITOR role)
Auth->>DB: Check ThreadAccess
DB-->>Auth: ✓ User has EDITOR role
Auth-->>API: ✓ Permission granted
API->>DB: Create ThreadEvent<br/>(author auto-set)
DB-->>API: Event created
API-->>Client: 201 + Event data
Client->>Client: Invalidate thread events
Client->>Client: Render new IM bubble
User->>User: Sees IM with rendered mention
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/backend/core/management/commands/createsuperuser.py (1)
34-45:⚠️ Potential issue | 🟡 MinorHandle the “already superuser” case in the
The new
💡 Suggested fix
except UserModel.DoesNotExist: try: user = UserModel.objects.get(email=email) except UserModel.DoesNotExist: user = UserModel(admin_email=email) message = "Superuser created successfully." else: - message = "User already existed and was upgraded to superuser." + if user.is_superuser and user.is_staff: + message = "Superuser already exists." + else: + message = "User already existed and was upgraded to superuser." else: if user.is_superuser and user.is_staff: message = "Superuser already exists."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/management/commands/createsuperuser.py` around lines 34 - 45, In the email-fallback branch around the UserModel.objects.get(email=email) block, the code always sets message = "User already existed and was upgraded to superuser." even when the fetched user is already staff+superuser; change the logic so that after successfully fetching user (the try ... except ... else path where user is retrieved) you check user.is_superuser and user.is_staff and set message = "Superuser already exists." when both are true, otherwise set the upgraded message; reference the UserModel.objects.get(email=email) call and the user and message variables when making this change.src/backend/core/api/openapi.json (1)
6008-6013:⚠️ Potential issue | 🟡 MinorUpdate
stats_fieldsdescription to mentionhas_notification.
has_notificationwas added to the enum (Lines 6008-6010), but the explanatory text still lists older boolean fields only. This creates a doc mismatch.Suggested text update
-Boolean fields: has_trashed, has_draft, has_starred, has_attachments, has_archived, -has_sender, has_active, has_delivery_pending, has_delivery_failed, is_spam, has_messages. +Boolean fields: has_trashed, has_draft, has_starred, has_attachments, has_archived, +has_sender, has_active, has_delivery_pending, has_delivery_failed, has_notification, +is_spam, has_messages.🤖 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 6008 - 6013, The stats_fields description text in the OpenAPI schema (the "description" for stats_fields) is out of sync with the enum: update the explanatory paragraph to include the new boolean field "has_notification" and its unread variant (has_notification_unread) alongside the other boolean fields (e.g., has_trashed, has_draft, has_starred, ... has_notification) so the documentation matches the enum entry for has_notification.
🧹 Nitpick comments (6)
src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss (1)
50-71: Add explicit:focus-visiblestyles for keyboard users.Hover styles are present, but explicit focus-visible states would make keyboard navigation clearer and more consistent.
💡 Suggested refactor
.thread-event-input__send { @@ &:hover:not(:disabled) { background-color: var(--c--globals--colors--brand-100); } + + &:focus-visible { + outline: 2px solid var(--c--globals--colors--brand-600); + outline-offset: 2px; + } &:disabled { color: var(--c--contextuals--content--semantic--neutral--tertiary); cursor: not-allowed; } } @@ .thread-event-input__mention-item { @@ &:hover { background-color: var(--c--globals--colors--brand-100); } + + &:focus-visible { + outline: 2px solid var(--c--globals--colors--brand-600); + outline-offset: -2px; + } }Also applies to: 88-101
🤖 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.scss` around lines 50 - 71, Add an explicit :focus-visible state for keyboard users on the interactive send control by extending the .thread-event-input__send block to include a :focus-visible rule that mirrors the hover affordance (e.g., set a clear background-color or ring/box-shadow and maintain border-radius) while ensuring the :disabled state remains unaffected; apply the same change to the other interactive control block later in the file (the other thread-event-input__* button/interactive selector) so keyboard focus is visually consistent across controls.src/backend/core/api/openapi.json (2)
6858-6861: Consider aligningMailbox.is_identityrequiredness with the serialized payload.
is_identityis now documented inMailbox.propertiesbut omitted fromMailbox.required. If this field is always present in responses, this should be marked required to avoid client type drift.Suggested schema alignment
"required": [ "abilities", "count_delivering", "count_messages", "count_unread_messages", "email", "id", + "is_identity", "role" ]Also applies to: 6947-6955
🤖 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 6858 - 6861, The Mailbox schema lists "is_identity" in Mailbox.properties but not in Mailbox.required, causing a mismatch with the serialized payload; update the OpenAPI schema so the requiredness matches actual responses by adding "is_identity" to the Mailbox.required array (or, if the field is truly optional, remove it from properties/ensure responses omit it), and apply the same fix to the other Mailbox schema instance referenced in the file (the duplicate Mailbox definition around the later section) so both schemas remain consistent.
4501-4506: Constrain event/notificationtypevalues to the currently supported set.Given the PR objective says only
type=imis implemented now, documentingtypeas free-form string weakens client-side validation and API discoverability.Suggested schema tightening
"type": { - "type": "string" + "type": "string", + "enum": ["im"] }Also applies to: 8740-8742, 8794-8798
🤖 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 4501 - 4506, The OpenAPI query parameter named "type" (description: "Filter notifications by type.") is currently an unconstrained string; change its schema to restrict allowed values to the implemented set (e.g., replace {"type":"string"} with {"type":"string","enum":["im"]}) so clients can validate against the supported notification/event type; apply the same change to the other identical "type" query-parameter occurrences for notifications/events in this file.src/backend/core/tests/api/test_thread_event_permissions.py (1)
518-534: Strengthen this parameter-confusion test to avoid vacuous success.Right now it can pass with an empty result set. Seed one event in
thread_aand assert it is returned; also comparethreadasstr(thread_a.id).As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases."Suggested hardening
def test_url_thread_id_ignores_query_param_thread_id(self, api_client): ... + event_a = factories.ThreadEventFactory(thread=thread_a, author=user) # Thread B the user has no access to thread_b = factories.ThreadFactory() factories.ThreadEventFactory(thread=thread_b) ... response = api_client.get(url) assert response.status_code == status.HTTP_200_OK - # Should only return events from thread_a (the URL path), not thread_b - for event in response.data.get("results", []): - assert event["thread"] == thread_a.id + results = response.data.get("results", []) + returned_ids = {event["id"] for event in results} + assert str(event_a.id) in returned_ids + assert all(event["thread"] == str(thread_a.id) for event in results)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_thread_event_permissions.py` around lines 518 - 534, In test_url_thread_id_ignores_query_param_thread_id, the test can pass vacuously if thread_a has no events; create a ThreadEvent for thread_a (e.g. call factories.ThreadEventFactory(thread=thread_a)) before issuing the GET, then assert the response contains at least one result and that the returned event's "thread" field equals str(thread_a.id) (instead of the current int comparison) to ensure the path thread id is honored over the query param.src/backend/core/tests/api/test_notification.py (1)
40-70: Add list-filter coverage forthread_idto match the API contract.Current list tests cover
is_doneandtype, but notthread_id. Add one positivethread_idfilter test (and ideally one malformed input case) to lock behavior.As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases."Suggested test addition
+ def test_list_notifications_filter_by_thread_id(self, api_client): + user = factories.UserFactory() + api_client.force_authenticate(user=user) + + thread_a = factories.ThreadFactory() + thread_b = factories.ThreadFactory() + factories.UserNotificationFactory(user=user, thread=thread_a) + factories.UserNotificationFactory(user=user, thread=thread_b) + + response = api_client.get(f"{get_notification_url()}?thread_id={thread_a.id}") + assert response.status_code == status.HTTP_200_OK + assert response.data["count"] == 1 + assert response.data["results"][0]["thread"] == str(thread_a.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_notification.py` around lines 40 - 70, Add coverage for filtering by thread_id to the notification list tests: in the test module add a new test (e.g., test_list_notifications_filter_by_thread_id) that creates a user, authenticates via api_client, creates two ThreadFactory threads and two UserNotificationFactory entries for the same user tied to different threads, then call api_client.get(f"{get_notification_url()}?thread_id=<thread.id>") and assert status 200 and count == 1 and that results[0]["thread_id"] equals the requested thread id; also add a small negative case (e.g., test_list_notifications_filter_by_thread_id_malformed) that requests ?thread_id=not-an-int and asserts a 400 or that the filter returns 0 results depending on the API contract, using the same helpers (UserFactory, ThreadFactory, UserNotificationFactory, get_notification_url) to locate where to add the tests.src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
173-206: Extract duplicated<ThreadMessage>rendering into a helper to reduce drift.Both ternary branches render nearly identical message JSX. A small
renderMessage(message)helper will keep behavior aligned when this component evolves.🤖 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/index.tsx` around lines 173 - 206, The two branches render nearly identical ThreadMessage JSX; extract a helper function renderMessage(message: MessageWithDraftChild) that returns the ThreadMessage element (setting key=message.id, message, isLatest computed from latestMessage, ref that assigns unreadRefs.current[message.id] when message.is_unread, data-message-id and draftMessage=message.draft_message) and use it in both places: for the threadItems map cast item.data to MessageWithDraftChild and call renderMessage(message), and in the messages.map call renderMessage(message); keep the existing event branch (ThreadEventComponent) and typings (MessageWithDraftChild, ThreadEvent) intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Makefile`:
- Line 392: Remove the hardcoded privileged superuser creation in the Makefile
line that runs "@$(MANAGE_DB) createsuperuser --email user1@example.local
--password user1" and replace it with a safer flow: do not create a superuser by
default, instead create a regular demo user (non-superuser) via the same
MANAGE_DB invocation (e.g., use createsuperuser flags or a custom management
command that sets is_staff/is_superuser=false) and accept the password from an
environment variable (e.g., read DEMO_USER_PASSWORD) or fail if not provided;
ensure the Makefile references $(MANAGE_DB) and the demo user email/username via
variables rather than hardcoded values so no privileged account or predictable
password is committed.
In `@src/backend/core/api/openapi.json`:
- Around line 4544-4568: The OpenAPI spec mistakenly includes list-filter query
parameters on the notification detail endpoints; remove the query parameters
named "is_done", "thread_id", and "type" from the "/notifications/{id}/"
operations so that retrieve/update-by-id routes do not accept list filters.
Locate the occurrences where the operation for path "/notifications/{id}/"
defines query parameters (look for parameter objects with "in": "query" and
"name": "is_done" / "thread_id" / "type") and delete those parameter objects for
each detail operation instance.
In `@src/backend/core/api/permissions.py`:
- Around line 117-123: The current nested-create check uses models.ThreadAccess
with mailbox__accesses__user=request.user which only verifies mailbox
membership; tighten it by also ensuring the user's mailbox access role allows
edits. In the create branch (where thread_id_from_url and view.action ==
"create") update the queryset filter on models.ThreadAccess to add
mailbox__accesses__role__in=enums.MAILBOX_ROLES_CAN_EDIT (or the appropriate
enum for mailbox edit roles) alongside mailbox__accesses__user=request.user so
only users with an edit-capable mailbox role and thread edit role pass.
In `@src/backend/core/api/serializers.py`:
- Around line 900-920: ThreadEventSerializer currently allows a create-time
message that may belong to a different thread; add server-side validation in
ThreadEventSerializer (e.g. implement validate(self, attrs) or validate_message)
to ensure that if a message is provided its message.thread.id matches the thread
id coming from the request context (obtain the thread id from
self.context['view'].kwargs using common keys like 'thread_pk', 'thread_id' or
'pk'), and raise serializers.ValidationError on mismatch; alternatively enforce
this in create(self, validated_data) by checking message.thread before saving to
guarantee message/thread consistency for models.ThreadEvent.
In `@src/backend/core/api/viewsets/notification.py`:
- Around line 58-60: The code is silently coercing invalid is_done inputs to
False; change the handling around self.request.query_params.get("is_done") so
you validate allowed values (e.g., case-insensitive "true","1","false","0")
instead of using is_done.lower() in ("true","1") directly; if the value is not
one of the allowed tokens, raise a DRF ValidationError (or return a 400 Bad
Request) with a clear message, and only then set a boolean (e.g., is_done_bool)
and apply queryset = queryset.filter(is_done=is_done_bool) so typos like "flase"
result in an error rather than being treated as False.
- Around line 66-68: The code currently pulls thread_id from
self.request.query_params and calls queryset.filter(thread_id=thread_id)
directly; guard this by attempting to parse the value as a UUID (use
uuid.UUID(thread_id)) and if parsing fails return a controlled 400-level error
(raise rest_framework.exceptions.ParseError or ValidationError consistent with
message.py) instead of letting a malformed UUID raise a server error; update the
block around self.request.query_params.get("thread_id") to try/except ValueError
and only apply queryset.filter(thread_id=thread_id) when the UUID is valid.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 44-70: Wrap perform_create in a database transaction (use
`@transaction.atomic`) and validate mention_ids before saving the event: verify
self.request.data.get("mention_ids") is a list, parse each entry into a UUID
(raise serializers.ValidationError on bad format), build a set of UUIDs and
exclude the current user by comparing UUIDs (do not compare strings), then load
only valid user IDs via models.MailboxAccess filtered by
mailbox__thread_accesses__thread=thread and user_id__in=validated_uuid_set; only
after validation save the event (serializer.save(thread=thread,
author=self.request.user)) and create models.UserNotification entries in the
same transaction using bulk_create so any failure rolls back both event and
notifications.
In `@src/backend/core/api/viewsets/thread.py`:
- Around line 94-99: Update the boolean filtering for has_notification to handle
both "1" and "0": when query_params.get("has_notification") == "1" keep the
existing queryset.filter(...) using notifications__user=self.request.user and
notifications__is_done=False, and when it == "0" apply queryset =
queryset.exclude(notifications__user=self.request.user,
notifications__is_done=False).distinct() so threads without pending
notifications are returned; use the same symbols (query_params,
has_notification, queryset, notifications__user, notifications__is_done,
self.request.user) to locate and implement the change.
In `@src/backend/core/migrations/0017_threadevent_usernotification.py`:
- Around line 15-56: The migration lacks DB indexes for common query paths; add
explicit indexes for ThreadEvent and UserNotification to optimize
filters/orders. Update the CreateModel operations (or add migrations.AddIndex
ops) for ThreadEvent to include indexes on ('thread', 'created_at') and on
('type', 'created_at') and optionally on ('author')/('channel')/('message') as
single-column indexes; for UserNotification add composite indexes on ('user',
'-created_at') and ('user', 'is_done', '-created_at') and an index on
('thread',) and on ('thread_event',) and/or ('type') so queries filtering by
user, done-status, thread, thread_event, type and ordering by created_at are
covered; use models.Index with fields specified (use '-created_at' for
descending) and reference the model names ThreadEvent and UserNotification and
their fields (thread, type, created_at, author, channel, message, user, is_done,
thread_event) when adding the indexes.
In `@src/backend/core/models.py`:
- Around line 1214-1253: Add appropriate composite DB indexes on the new models
to support the hot query paths: in the ThreadEvent model's inner Meta add an
Index on ("thread", "created_at") (or equivalent fields) and in the
UserNotification model's Meta add composite Indexes for the high-traffic
filters/orderings such as ("user", "is_done"), ("user", "type", "created_at")
and ("thread", "created_at"); then create the corresponding Django migration
under migrations/ that adds these Index objects to the DB so they are applied.
Ensure you reference the Meta of ThreadEvent and UserNotification when adding
Index instances and include the migration file for review.
In `@src/frontend/public/locales/common/it-IT.json`:
- Line 2: Update the Italian translation value for the key "Add internal comment
(visible only to your team)..." in the locale JSON so the ellipsis matches other
locales: replace the single-character ellipsis (U+2026) at the end of "Aggiungi
commento interno (visibile solo al tuo team)…" with three ASCII dots "..." to
produce "Aggiungi commento interno (visibile solo al tuo team)...".
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx`:
- Around line 21-23: The mentionIds state currently persists and is sent even
when users remove or edit mention text; update the input change and submit
handlers to recompute mentionIds from the current text instead of relying on
stale state: in the component functions that manage the message input (where you
currently call setMentionIds, e.g., the input onChange handler and the
submit/send handler referenced around the other ranges), parse the current input
value to extract active mention tokens (e.g., `@username` spans or mention
markers), map those to user IDs and call setMentionIds with that fresh list, and
ensure you clear or remove IDs when the corresponding mention text is deleted or
edited before sending so only currently visible mentions are included. Keep this
sync logic in the same handlers used for paste/delete and before sending the
final payload (so mentionIds always reflects the current text).
- Around line 193-199: The send button currently relies on a title for
accessibility; add an explicit accessible name by adding an aria-label (or
aria-labelledby) attribute to the button element (the button with className
"thread-event-input__send" and onClick={handleSubmit}) so screen readers get a
reliable name (use the same localized string t("Send")); keep the existing
disabled logic (disabled={!content.trim() || createEvent.isPending}) and title
but ensure aria-label is present.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss`:
- Line 46: Replace deprecated CSS property usage: locate occurrences of
"word-break: break-word" in _index.scss and change them to "word-break: normal"
and add "overflow-wrap: anywhere" on the same selector (so long words still
wrap). Ensure both places (the two occurrences of the break-word declaration)
are updated and keep existing specificity/ordering unchanged.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 41-43: The code uses a cast (eventData.content as string) without
runtime checks, which can lead to crashes in renderContentWithMentions when
.split is called on non-strings; update the logic around eventData/content to
first check typeof eventData.content === "string" (or null/undefined) and only
assign content to that string value, otherwise default to "" (or call
String(...) if intentional coercion is OK), and ensure any downstream usage in
renderContentWithMentions or isCondensed only receives a guaranteed string;
adjust the variable declaration for content and any call sites
(renderContentWithMentions, isCondensed) accordingly so they never operate on
non-string values.
In `@src/frontend/src/features/providers/mailbox.tsx`:
- Around line 204-219: threadItems currently returns null whenever
messagesQuery.data?.data is missing, which hides threadEvents already fetched;
update the useMemo in threadItems to always build eventItems from
threadEventsQuery.data?.data?.results (defaulting to []), build messageItems
from messagesQuery.data?.data (defaulting to []), merge and sort them, and
return an empty array only if both sources are empty; reference the threadItems
constant and the messagesQuery and threadEventsQuery variables (and the
TimelineItem shape) when making this change.
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 6008-6013: The stats_fields description text in the OpenAPI schema
(the "description" for stats_fields) is out of sync with the enum: update the
explanatory paragraph to include the new boolean field "has_notification" and
its unread variant (has_notification_unread) alongside the other boolean fields
(e.g., has_trashed, has_draft, has_starred, ... has_notification) so the
documentation matches the enum entry for has_notification.
In `@src/backend/core/management/commands/createsuperuser.py`:
- Around line 34-45: In the email-fallback branch around the
UserModel.objects.get(email=email) block, the code always sets message = "User
already existed and was upgraded to superuser." even when the fetched user is
already staff+superuser; change the logic so that after successfully fetching
user (the try ... except ... else path where user is retrieved) you check
user.is_superuser and user.is_staff and set message = "Superuser already
exists." when both are true, otherwise set the upgraded message; reference the
UserModel.objects.get(email=email) call and the user and message variables when
making this change.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 6858-6861: The Mailbox schema lists "is_identity" in
Mailbox.properties but not in Mailbox.required, causing a mismatch with the
serialized payload; update the OpenAPI schema so the requiredness matches actual
responses by adding "is_identity" to the Mailbox.required array (or, if the
field is truly optional, remove it from properties/ensure responses omit it),
and apply the same fix to the other Mailbox schema instance referenced in the
file (the duplicate Mailbox definition around the later section) so both schemas
remain consistent.
- Around line 4501-4506: The OpenAPI query parameter named "type" (description:
"Filter notifications by type.") is currently an unconstrained string; change
its schema to restrict allowed values to the implemented set (e.g., replace
{"type":"string"} with {"type":"string","enum":["im"]}) so clients can validate
against the supported notification/event type; apply the same change to the
other identical "type" query-parameter occurrences for notifications/events in
this file.
In `@src/backend/core/tests/api/test_notification.py`:
- Around line 40-70: Add coverage for filtering by thread_id to the notification
list tests: in the test module add a new test (e.g.,
test_list_notifications_filter_by_thread_id) that creates a user, authenticates
via api_client, creates two ThreadFactory threads and two
UserNotificationFactory entries for the same user tied to different threads,
then call api_client.get(f"{get_notification_url()}?thread_id=<thread.id>") and
assert status 200 and count == 1 and that results[0]["thread_id"] equals the
requested thread id; also add a small negative case (e.g.,
test_list_notifications_filter_by_thread_id_malformed) that requests
?thread_id=not-an-int and asserts a 400 or that the filter returns 0 results
depending on the API contract, using the same helpers (UserFactory,
ThreadFactory, UserNotificationFactory, get_notification_url) to locate where to
add the tests.
In `@src/backend/core/tests/api/test_thread_event_permissions.py`:
- Around line 518-534: In test_url_thread_id_ignores_query_param_thread_id, the
test can pass vacuously if thread_a has no events; create a ThreadEvent for
thread_a (e.g. call factories.ThreadEventFactory(thread=thread_a)) before
issuing the GET, then assert the response contains at least one result and that
the returned event's "thread" field equals str(thread_a.id) (instead of the
current int comparison) to ensure the path thread id is honored over the query
param.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss`:
- Around line 50-71: Add an explicit :focus-visible state for keyboard users on
the interactive send control by extending the .thread-event-input__send block to
include a :focus-visible rule that mirrors the hover affordance (e.g., set a
clear background-color or ring/box-shadow and maintain border-radius) while
ensuring the :disabled state remains unaffected; apply the same change to the
other interactive control block later in the file (the other
thread-event-input__* button/interactive selector) so keyboard focus is visually
consistent across controls.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 173-206: The two branches render nearly identical ThreadMessage
JSX; extract a helper function renderMessage(message: MessageWithDraftChild)
that returns the ThreadMessage element (setting key=message.id, message,
isLatest computed from latestMessage, ref that assigns
unreadRefs.current[message.id] when message.is_unread, data-message-id and
draftMessage=message.draft_message) and use it in both places: for the
threadItems map cast item.data to MessageWithDraftChild and call
renderMessage(message), and in the messages.map call renderMessage(message);
keep the existing event branch (ThreadEventComponent) and typings
(MessageWithDraftChild, ThreadEvent) intact.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (19)
src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_partial_update_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_update_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_thread_event_list.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_user_notification_list.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_user_notification_request.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_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_events_list_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/models/user_notification.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/user_notification_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/notifications/notifications.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (36)
Makefilesrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/notification.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/management/commands/createsuperuser.pysrc/backend/core/migrations/0017_threadevent_usernotification.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_notification.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/urls.pysrc/frontend/public/locales/common/br-FR.jsonsrc/frontend/public/locales/common/de-DE.jsonsrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/es-ES.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/public/locales/common/it-IT.jsonsrc/frontend/public/locales/common/nl-NL.jsonsrc/frontend/public/locales/common/pt-PT.jsonsrc/frontend/public/locales/common/ru-RU.jsonsrc/frontend/public/locales/common/uk-UA.jsonsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsxsrc/frontend/src/features/layouts/components/thread-view/_index.scsssrc/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/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/pages/_app.tsx
Contributor
|
Pretty nice! About the comment input, I'll remove one icon and I'll make it sticky and some box-shadow and backdrop-filter to make it floating hover content and in this way it will always available at the bottom of the screen no matter the thread height. We could image to hide it when the user is scrolling down. 
|
jbpenrath
reviewed
Feb 26, 2026
Contributor
There was a problem hiding this comment.
Here it is my instruction I put into Claude.md to prevent it to translate all and bypassing i18next-cli.
## Internationalization
You should never modify translation files manually. You should use the `make i18n-generate` command to generate the translation files.
Once the files are updated, you can complete the empty translations. You will only translate French and English for now. You will revert changes on other languages.
jbpenrath
force-pushed
the
threadevents
branch
2 times, most recently
from
4dd6f39 to
f02d02b
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 (2)
src/backend/core/api/openapi.json (2)
5539-5723:⚠️ Potential issue | 🟡 MinorDrop
mailbox_idquery filter from thread-access detail operations.On
/api/v1.0/threads/{thread_id}/accesses/{id}/(put/patch/delete),mailbox_idis documented as a list filter and should not be present on detail-by-id routes.Suggested cleanup
"parameters": [ { "in": "path", "name": "id", ... }, - { "in": "query", "name": "mailbox_id", "schema": { "type": "string", "format": "uuid" }, "description": "Filter thread accesses by mailbox ID." }, { "in": "path", "name": "thread_id", ... } ]🤖 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 5539 - 5723, The OpenAPI detail operations for the ThreadAccess resource incorrectly include the query parameter "mailbox_id"; remove the "mailbox_id" parameter object from the "parameters" array of the operations with operationId "threads_accesses_update", "threads_accesses_partial_update", and "threads_accesses_destroy" so the detail path /api/v1.0/threads/{thread_id}/accesses/{id}/ only keeps the path params "id" and "thread_id" (leave all other requestBody, responses, and security entries unchanged).
6116-6124:⚠️ Potential issue | 🟡 MinorKeep
stats_fieldsdescription in sync with enum values.
has_notificationwas added to the enum, but the parameter description still lists the old boolean fields set.Doc fix
- "description": "... 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.\n ..." + "description": "... Boolean fields: has_trashed, has_draft, has_starred, has_attachments, has_archived,\n has_sender, has_active, has_delivery_pending, has_delivery_failed, has_notification, is_spam, has_messages.\n ..."🤖 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 6116 - 6124, The description for the parameter stats_fields is out of sync with its enum: add the newly introduced has_notification (and its unread variant has_notification_unread) to the listed boolean fields and any examples, and ensure the full list in the description matches the enum values (including special values 'all' and 'all_unread') so the description and enum stay consistent; update the text near the stats_fields parameter in openapi.json where the enum and description are defined to reflect these additions.
♻️ Duplicate comments (3)
src/backend/core/api/serializers.py (1)
951-970:⚠️ Potential issue | 🟠 MajorEnforce message/thread consistency during ThreadEvent creation
On Line 963,
messageis writable on create, but there’s no guard that the message belongs to the thread from the nested URL. This can create cross-thread event links.🔧 Proposed fix
class ThreadEventSerializer(CreateOnlyFieldsMixin, serializers.ModelSerializer): """Serialize thread event information.""" @@ class Meta: model = models.ThreadEvent @@ read_only_fields = ["id", "thread", "channel", "author", "created_at", "updated_at"] create_only_fields = ["type", "message"] + + def validate(self, attrs): + """Ensure the related message belongs to the target thread.""" + attrs = super().validate(attrs) + message = attrs.get("message") + if not message: + return attrs + + view = self.context.get("view") + kwargs = getattr(view, "kwargs", {}) if view else {} + thread_id = kwargs.get("thread_id") or kwargs.get("thread_pk") or kwargs.get("pk") + if thread_id and str(message.thread_id) != str(thread_id): + raise serializers.ValidationError( + {"message": "Message must belong to the target thread."} + ) + return attrs🤖 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 951 - 970, The ThreadEventSerializer allows a writable message on create but doesn't verify the message belongs to the thread in the nested URL; add validation in ThreadEventSerializer (e.g., override validate or validate_message) to fetch the provided message instance and compare its thread to the serializer's thread/context/validated_data['thread'] (or use self.context to get the nested thread id), and raise serializers.ValidationError if they differ so cross-thread event links cannot be created.src/backend/core/api/openapi.json (1)
4550-4756:⚠️ Potential issue | 🟡 MinorRemove list-filter query params from notification detail operations.
is_done,thread_id, andtypeare list filters and should not be on/api/v1.0/notifications/{id}/get/put/patchoperations.Suggested OpenAPI cleanup
"parameters": [ { "in": "path", "name": "id", "schema": { "type": "string" }, "required": true - }, - { - "in": "query", - "name": "is_done", - "schema": { "type": "boolean" }, - "description": "Filter notifications by done status." - }, - { - "in": "query", - "name": "thread_id", - "schema": { "type": "string", "format": "uuid" }, - "description": "Filter notifications by thread ID." - }, - { - "in": "query", - "name": "type", - "schema": { "type": "string" }, - "description": "Filter notifications by type." } ]🤖 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 4550 - 4756, The detail operations notifications_retrieve, notifications_update, and notifications_partial_update include list-filter query parameters is_done, thread_id, and type that don't belong on the /api/v1.0/notifications/{id}/ endpoints; remove those three parameter objects from the "parameters" array of the get/put/patch operations (operationId values above) so only the path "id" parameter remains, and ensure any list filters remain defined only on the list endpoint (/api/v1.0/notifications/) not on these detail operations.src/backend/core/api/viewsets/thread_event.py (1)
44-70:⚠️ Potential issue | 🔴 CriticalMake ThreadEvent + mention notifications atomic, and validate
mention_idsbefore writing.Line 48 persists the event before mention payload normalization. If
mention_idsis malformed (eg non-list, unhashable members, invalid UUID values), the request can fail after write and leave partial state.💡 Proposed fix
+import uuid +from django.db import transaction from django.shortcuts import get_object_or_404 from drf_spectacular.utils import extend_schema from rest_framework import mixins, viewsets +from rest_framework.exceptions import ValidationError @@ - def perform_create(self, serializer): + `@transaction.atomic` + def perform_create(self, serializer): """Set thread from URL and author from request user.""" thread = get_object_or_404(models.Thread, id=self.kwargs["thread_id"]) - mention_ids = self.request.data.get("mention_ids", []) - event = serializer.save(thread=thread, author=self.request.user) + raw_mention_ids = self.request.data.get("mention_ids") or [] + if not isinstance(raw_mention_ids, list): + raise ValidationError({"mention_ids": "Must be a list of user IDs."}) + + mention_ids = set() + for raw_id in raw_mention_ids: + try: + mention_id = uuid.UUID(str(raw_id)) + except (TypeError, ValueError) as exc: + raise ValidationError({"mention_ids": f"Invalid UUID: {raw_id}"}) from exc + if mention_id != self.request.user.id: + mention_ids.add(mention_id) + + event = serializer.save(thread=thread, author=self.request.user) @@ - if mention_ids: - # Deduplicate, exclude self-mention, and validate access - unique_mention_ids = set(mention_ids) - {str(self.request.user.id)} - if unique_mention_ids: - valid_user_ids = set( - models.MailboxAccess.objects.filter( - mailbox__thread_accesses__thread=thread, - user_id__in=unique_mention_ids, - ).values_list("user_id", flat=True) - ) - notifications = [ - models.UserNotification( - user_id=user_id, - type="mention", - thread=thread, - thread_event=event, - ) - for user_id in valid_user_ids - ] - models.UserNotification.objects.bulk_create(notifications) + if mention_ids: + valid_user_ids = set( + models.MailboxAccess.objects.filter( + mailbox__thread_accesses__thread=thread, + user_id__in=mention_ids, + ).values_list("user_id", flat=True) + ) + notifications = [ + models.UserNotification( + user_id=user_id, + type="mention", + thread=thread, + thread_event=event, + ) + for user_id in valid_user_ids + ] + models.UserNotification.objects.bulk_create(notifications)#!/bin/bash set -euo pipefail echo "Inspect current perform_create implementation" cat -n src/backend/core/api/viewsets/thread_event.py | sed -n '40,95p' echo echo "Check for transaction decorator and mention parsing patterns" rg -n "@transaction\.atomic|serializer\.save\(|mention_ids\s*=|set\(mention_ids\)|ValidationError" \ src/backend/core/api/viewsets/thread_event.pyExpected verification outcome:
serializer.save(...)appears before strictmention_idsvalidation.- No
@transaction.atomiconperform_create.🤖 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 44 - 70, perform_create currently calls serializer.save before validating mention_ids and is not atomic; update perform_create to (1) validate and normalize mention_ids from self.request.data: ensure it's a list/iterable, coerce members to strings, filter out the requesting user id, deduplicate, and validate each id as a proper UUID (use uuid.UUID(...) try/except) rejecting/ignoring malformed entries; (2) wrap the save and notification creation in a single transaction.atomic block so the ThreadEvent save (serializer.save(thread=..., author=...)) and the subsequent models.UserNotification.bulk_create are atomic; (3) after normalization, compute valid_user_ids via models.MailboxAccess filter (as before) and create notifications only for those ids; reference perform_create, mention_ids, serializer.save, models.MailboxAccess, models.UserNotification and ensure needed imports (transaction, UUID) are added.
🧹 Nitpick comments (4)
src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx (1)
60-66: Scope query invalidation to the current thread events key.The
/events/predicate invalidates broadly and may refetch unrelated event caches. Prefer a thread-specific key invalidation to reduce unnecessary network churn.🤖 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 60 - 66, The current broad predicate passed to queryClient.invalidateQueries can match unrelated event caches; tighten it to only invalidate queries for the current thread by checking the queryKey contains the current threadId (and still includes the events marker): update the second invalidateQueries call so its predicate verifies Array.isArray(queryKey) && queryKey.some(k => k === threadId) && queryKey.some(k => typeof k === "string" && k.includes("/events/")) (or match the exact array shape you use for thread events), keeping the explicit invalidateQueries({ queryKey: ["threads","events", threadId] }) and removing any logic that would match event keys for other threads.src/backend/core/tests/api/test_notification.py (1)
77-152: Add unauthenticated coverage for detail/retrieve and patch endpointsYou already assert 401 for list (Line 71). Add equivalent unauthorized tests for
GET /notifications/{id}/andPATCH /notifications/{id}/to lock in access-control behavior on detail routes too.As per coding guidelines
src/backend/**/{tests.py,tests/**/*.py}: “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_notification.py` around lines 77 - 152, Add unauthenticated tests that assert 401 for the detail endpoints: in TestNotificationRetrieve add a test (e.g. test_retrieve_notification_unauthenticated) that does NOT call api_client.force_authenticate, creates a notification via factories.UserNotificationFactory, calls api_client.get(get_notification_url(notification.id)) and asserts response.status_code == status.HTTP_401_UNAUTHORIZED; similarly, in TestNotificationUpdate add a test (e.g. test_update_notification_unauthenticated) that creates a notification, does not authenticate, calls api_client.patch(get_notification_url(notification.id), {"is_done": True}, format="json") and asserts response.status_code == status.HTTP_401_UNAUTHORIZED to mirror the existing list 401 coverage.src/backend/core/api/openapi.json (1)
8438-8442: Consider constrainingThreadEvent.typeto supported values.
typeis currently a free-form string in request/response schemas. Since this PR currently implementstype=im, documenting an enum improves contract clarity and generated client validation.Schema tightening
"type": { "type": "string", + "enum": ["im"], "maxLength": 36 }Also applies to: 8999-9002, 9054-9058
🤖 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 8438 - 8442, The ThreadEvent.type schema is a free-form string; tighten it by replacing the open string constraints with an enum of supported event types (at minimum include "im") so generated clients can validate; update the schema for "ThreadEvent.type" (and the other identical schemas flagged in the diff) to use "type": "string" with an "enum": ["im", ...otherSupportedValues] and keep min/maxLength if desired, ensuring all three occurrences (the main ThreadEvent schema and the two other matching schemas) are changed consistently.src/backend/core/tests/api/test_thread_event_permissions.py (1)
318-420: Add malformedmention_idspayload tests to close security-input gaps.Edge cases are good, but there is no case for invalid shape (non-list) or invalid UUID format in
mention_ids. Add dedicated tests for those payloads so behavior is explicit and regression-proof.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_permissions.py` around lines 318 - 420, Add two unit tests in TestMentionSecurityEdgeCases to assert explicit failure behavior for malformed mention_ids: implement test_malformed_mention_ids_not_list_returns_400 that authenticates via api_client, posts data where "mention_ids" is a non-list (e.g., a string or dict) to get_thread_event_url(thread.id) and asserts response.status_code == status.HTTP_400_BAD_REQUEST and models.UserNotification.objects.count() == 0; and implement test_malformed_mention_ids_invalid_uuid_returns_400 that posts mention_ids as a list containing an invalid UUID string, asserts a 400 response, and that no notifications were created; keep tests focused, minimal assertions, and reuse setup_user_with_thread_access, api_client.force_authenticate, and models.UserNotification for checks.
🤖 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/management/commands/createsuperuser.py`:
- Around line 34-40: The current lookup uses UserModel.objects.get(email=email)
but never sets the USERNAME_FIELD (admin_email) on existing rows and doesn't
handle ambiguous non-unique email matches; update the logic in the
createsuperuser flow around UserModel, email and admin_email so that: if no user
exists create user = UserModel(admin_email=email) as before; if a single user is
returned set user.admin_email = email (and any other required username-field
defaults) before promoting and saving; catch UserModel.MultipleObjectsReturned
and fail with an explicit ambiguous-email error (or prompt to resolve
duplicates) rather than silently picking one; ensure you call user.save() after
updating admin_email and before making the account a superuser.
In `@src/backend/core/models.py`:
- Around line 1401-1420: The model allows notifications without a thread which
breaks Thread.notifications-based logic; make thread required and enforce
consistency with thread_event: change the thread field to null=False,
blank=False (so every UserNotification has a thread), add a model-level
validation (override clean() and raise ValidationError in save() or call
full_clean()) to ensure that if thread_event is set then thread_event.thread ==
self.thread, and add Meta.constraints and Meta.indexes (e.g., a
CheckConstraint/Q to assert thread is not null and DB indexes on ("thread",) and
("thread_event",) via models.Index) so queries are efficient and the invariant
is enforced at model level (refer to the thread and thread_event fields and the
UserNotification model class when implementing).
In `@src/backend/core/tests/api/test_thread_event_permissions.py`:
- Around line 532-533: The test fails because it compares the JSON-serialized
thread id (a string) to a UUID object; change the assertion to compare
event["thread"] to str(thread_a.id) instead of thread_a.id so the serialized
response value and the expected value are the same type (update the assertion
that references event["thread"] and thread_a.id in the test function).
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx`:
- Around line 49-53: The ThreadEventRequest type used in the thread-event input
is missing the mention_ids field; stop masking it with a type assertion in the
createEvent.mutate call and regenerate the frontend API models from the OpenAPI
spec so ThreadEventRequest includes mention_ids (or manually add mention_ids?:
number[] to the generated interface in
src/frontend/src/features/api/gen/models/thread_event_request.ts). Then remove
the incorrect assertion at the createEvent.mutate data assignment and use the
proper typed object (data: { type: "im", data: { content: trimmed },
mention_ids: mentionIds.length > 0 ? mentionIds : undefined } as
ThreadEventRequest) so mention_ids is correctly typed and passed through.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 205-212: The fallback rendering of ThreadMessage (the branch that
runs when threadItems is null) omits the data-created-at attribute which the
intersection observer callback (used to update markAsReadAt) depends on; update
that fallback ThreadMessage element to include
data-created-at={message.created_at} (matching the existing data-created-at used
elsewhere), keep the existing data-message-id and ref/unreadRefs usage intact so
unread tracking and markAsReadAt continue to work.
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5539-5723: The OpenAPI detail operations for the ThreadAccess
resource incorrectly include the query parameter "mailbox_id"; remove the
"mailbox_id" parameter object from the "parameters" array of the operations with
operationId "threads_accesses_update", "threads_accesses_partial_update", and
"threads_accesses_destroy" so the detail path
/api/v1.0/threads/{thread_id}/accesses/{id}/ only keeps the path params "id" and
"thread_id" (leave all other requestBody, responses, and security entries
unchanged).
- Around line 6116-6124: The description for the parameter stats_fields is out
of sync with its enum: add the newly introduced has_notification (and its unread
variant has_notification_unread) to the listed boolean fields and any examples,
and ensure the full list in the description matches the enum values (including
special values 'all' and 'all_unread') so the description and enum stay
consistent; update the text near the stats_fields parameter in openapi.json
where the enum and description are defined to reflect these additions.
---
Duplicate comments:
In `@src/backend/core/api/openapi.json`:
- Around line 4550-4756: The detail operations notifications_retrieve,
notifications_update, and notifications_partial_update include list-filter query
parameters is_done, thread_id, and type that don't belong on the
/api/v1.0/notifications/{id}/ endpoints; remove those three parameter objects
from the "parameters" array of the get/put/patch operations (operationId values
above) so only the path "id" parameter remains, and ensure any list filters
remain defined only on the list endpoint (/api/v1.0/notifications/) not on these
detail operations.
In `@src/backend/core/api/serializers.py`:
- Around line 951-970: The ThreadEventSerializer allows a writable message on
create but doesn't verify the message belongs to the thread in the nested URL;
add validation in ThreadEventSerializer (e.g., override validate or
validate_message) to fetch the provided message instance and compare its thread
to the serializer's thread/context/validated_data['thread'] (or use self.context
to get the nested thread id), and raise serializers.ValidationError if they
differ so cross-thread event links cannot be created.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 44-70: perform_create currently calls serializer.save before
validating mention_ids and is not atomic; update perform_create to (1) validate
and normalize mention_ids from self.request.data: ensure it's a list/iterable,
coerce members to strings, filter out the requesting user id, deduplicate, and
validate each id as a proper UUID (use uuid.UUID(...) try/except)
rejecting/ignoring malformed entries; (2) wrap the save and notification
creation in a single transaction.atomic block so the ThreadEvent save
(serializer.save(thread=..., author=...)) and the subsequent
models.UserNotification.bulk_create are atomic; (3) after normalization, compute
valid_user_ids via models.MailboxAccess filter (as before) and create
notifications only for those ids; reference perform_create, mention_ids,
serializer.save, models.MailboxAccess, models.UserNotification and ensure needed
imports (transaction, UUID) are added.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 8438-8442: The ThreadEvent.type schema is a free-form string;
tighten it by replacing the open string constraints with an enum of supported
event types (at minimum include "im") so generated clients can validate; update
the schema for "ThreadEvent.type" (and the other identical schemas flagged in
the diff) to use "type": "string" with an "enum": ["im",
...otherSupportedValues] and keep min/maxLength if desired, ensuring all three
occurrences (the main ThreadEvent schema and the two other matching schemas) are
changed consistently.
In `@src/backend/core/tests/api/test_notification.py`:
- Around line 77-152: Add unauthenticated tests that assert 401 for the detail
endpoints: in TestNotificationRetrieve add a test (e.g.
test_retrieve_notification_unauthenticated) that does NOT call
api_client.force_authenticate, creates a notification via
factories.UserNotificationFactory, calls
api_client.get(get_notification_url(notification.id)) and asserts
response.status_code == status.HTTP_401_UNAUTHORIZED; similarly, in
TestNotificationUpdate add a test (e.g.
test_update_notification_unauthenticated) that creates a notification, does not
authenticate, calls api_client.patch(get_notification_url(notification.id),
{"is_done": True}, format="json") and asserts response.status_code ==
status.HTTP_401_UNAUTHORIZED to mirror the existing list 401 coverage.
In `@src/backend/core/tests/api/test_thread_event_permissions.py`:
- Around line 318-420: Add two unit tests in TestMentionSecurityEdgeCases to
assert explicit failure behavior for malformed mention_ids: implement
test_malformed_mention_ids_not_list_returns_400 that authenticates via
api_client, posts data where "mention_ids" is a non-list (e.g., a string or
dict) to get_thread_event_url(thread.id) and asserts response.status_code ==
status.HTTP_400_BAD_REQUEST and models.UserNotification.objects.count() == 0;
and implement test_malformed_mention_ids_invalid_uuid_returns_400 that posts
mention_ids as a list containing an invalid UUID string, asserts a 400 response,
and that no notifications were created; keep tests focused, minimal assertions,
and reuse setup_user_with_thread_access, api_client.force_authenticate, and
models.UserNotification for checks.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx`:
- Around line 60-66: The current broad predicate passed to
queryClient.invalidateQueries can match unrelated event caches; tighten it to
only invalidate queries for the current thread by checking the queryKey contains
the current threadId (and still includes the events marker): update the second
invalidateQueries call so its predicate verifies Array.isArray(queryKey) &&
queryKey.some(k => k === threadId) && queryKey.some(k => typeof k === "string"
&& k.includes("/events/")) (or match the exact array shape you use for thread
events), keeping the explicit invalidateQueries({ queryKey: ["threads","events",
threadId] }) and removing any logic that would match event keys for other
threads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 94171aab-1db8-42e0-a33e-c613aa63c9db
⛔ Files ignored due to path filters (18)
src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_partial_update_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_update_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_thread_event_list.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_user_notification_list.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_user_notification_request.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_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_events_list_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/models/user_notification.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/user_notification_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/notifications/notifications.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (27)
Makefilesrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/notification.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/management/commands/createsuperuser.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_notification.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/urls.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/_index.scsssrc/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/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/pages/_app.tsx
🚧 Files skipped from review as they are similar to previous changes (11)
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx
- src/frontend/src/pages/_app.tsx
- src/backend/core/tests/api/test_thread_event.py
- src/backend/core/enums.py
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/public/locales/common/en-US.json
- src/frontend/src/features/providers/mailbox.tsx
- src/backend/core/factories.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss
- src/backend/core/api/viewsets/notification.py
- src/frontend/src/features/layouts/components/thread-view/_index.scss
There was a problem hiding this comment.
♻️ Duplicate comments (3)
src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
205-214:⚠️ Potential issue | 🟠 MajorFallback
ThreadMessagerendering dropsdata-created-at, breaking read tracking.In the fallback map (when
threadItemsis null), line 211 hasdata-message-idbut omitsdata-created-at. The observer callback at line 77 depends on this attribute to track read status. WhenthreadItemsis null, unread messages may never advancemarkAsReadAt.Proposed fix
return ( <ThreadMessage key={message.id} message={message} isLatest={isLatest} ref={isUnread ? (el => { unreadRefs.current[message.id] = el; }) : undefined} data-message-id={message.id} + data-created-at={message.created_at} draftMessage={message.draft_message} /> );🤖 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/index.tsx` around lines 205 - 214, Fallback ThreadMessage render is missing the data-created-at attribute, which the intersection observer uses to advance markAsReadAt; update the fallback JSX that returns <ThreadMessage ...> to include data-created-at={message.created_at} (matching the attribute used elsewhere), ensuring unreadRefs and observer read-tracking (markAsReadAt) continue to work when threadItems is null.src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
43-44:⚠️ Potential issue | 🟡 MinorAdd runtime type guard for
contentbefore string operations.Line 44 uses
as stringwhich provides no runtime protection. If the API returns a non-string value fordata.content,renderContentWithMentions(line 70, 99) will crash on.split(...). The backend'sThreadEvent.datafield is unvalidated JSON with no schema enforcement.Proposed fix
const eventData = (event.data ?? {}) as Record<string, unknown>; - const content = (eventData.content as string) || ""; + const rawContent = eventData.content; + const content = typeof rawContent === "string" ? rawContent : "";🤖 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 43 - 44, The code casts eventData.content to string without runtime checks, which can crash renderContentWithMentions when it calls .split; update handling of eventData.content in this component by adding a runtime type guard (check typeof eventData.content === "string") and only assign content when it's a string, otherwise default to "" (or sanitize with String(...) if appropriate); ensure places that call renderContentWithMentions (and any direct .split usage) use this guarded content variable (referencing eventData, content, and renderContentWithMentions) so non-string JSON values from ThreadEvent.data cannot cause exceptions.src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss (1)
49-49:⚠️ Potential issue | 🟡 MinorReplace deprecated
word-break: break-wordto comply with CSS spec.Lines 49 and 103 use
word-break: break-word, which is deprecated per W3C CSS Text Module Level 3. The modern equivalent isword-break: normal;paired withoverflow-wrap: anywhere;.Proposed fix
.thread-event--im .thread-event__content { color: var(--c--contextuals--content--semantic--neutral--primary); font-size: var(--c--globals--font--sizes--sm); line-height: 1.5; white-space: pre-wrap; - word-break: break-word; + word-break: normal; + overflow-wrap: anywhere; padding: var(--c--globals--spacings--xs) var(--c--globals--spacings--sm); } .thread-event--generic .thread-event__content { font-size: var(--c--globals--font--sizes--sm); color: var(--c--contextuals--content--semantic--neutral--secondary); line-height: 1.5; white-space: pre-wrap; - word-break: break-word; + word-break: normal; + overflow-wrap: anywhere; }Also applies to: 103-103
🤖 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.scss` at line 49, Replace the deprecated CSS declaration "word-break: break-word" with the modern pattern by setting "word-break: normal" and adding "overflow-wrap: anywhere" in the same rule where "word-break: break-word" appears (both occurrences). Locate the occurrences of the literal "word-break: break-word" in _index.scss and update each rule to use word-break: normal plus overflow-wrap: anywhere to preserve the intended wrapping behavior.
🧹 Nitpick comments (1)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
98-109: Mention regex may capture trailing punctuation.The regex
/(@\S+)/gwill match@user.or@user,including the punctuation. If mentions should exclude trailing punctuation, consider a more precise pattern.Optional refinement
const renderContentWithMentions = (content: string) => { - const parts = content.split(/(@\S+)/g); + const parts = content.split(/(@[\w.-]+)/g); return parts.map((part, i) => { if (part.startsWith("@")) {🤖 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 98 - 109, The mention regex in renderContentWithMentions is currently /(@\S+)/g which captures trailing punctuation; update the split pattern to only capture the mention token (for example use a pattern like /(@[A-Za-z0-9_]+)/g or /(@\w+)/g or a lookahead-based pattern that stops at word boundaries) so punctuation stays in the surrounding text part; keep the rest of renderContentWithMentions intact so parts that start with "@" are wrapped with the "thread-event__mention" span and trailing punctuation will be returned as a separate non-mention part.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss`:
- Line 49: Replace the deprecated CSS declaration "word-break: break-word" with
the modern pattern by setting "word-break: normal" and adding "overflow-wrap:
anywhere" in the same rule where "word-break: break-word" appears (both
occurrences). Locate the occurrences of the literal "word-break: break-word" in
_index.scss and update each rule to use word-break: normal plus overflow-wrap:
anywhere to preserve the intended wrapping behavior.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 43-44: The code casts eventData.content to string without runtime
checks, which can crash renderContentWithMentions when it calls .split; update
handling of eventData.content in this component by adding a runtime type guard
(check typeof eventData.content === "string") and only assign content when it's
a string, otherwise default to "" (or sanitize with String(...) if appropriate);
ensure places that call renderContentWithMentions (and any direct .split usage)
use this guarded content variable (referencing eventData, content, and
renderContentWithMentions) so non-string JSON values from ThreadEvent.data
cannot cause exceptions.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 205-214: Fallback ThreadMessage render is missing the
data-created-at attribute, which the intersection observer uses to advance
markAsReadAt; update the fallback JSX that returns <ThreadMessage ...> to
include data-created-at={message.created_at} (matching the attribute used
elsewhere), ensuring unreadRefs and observer read-tracking (markAsReadAt)
continue to work when threadItems is null.
---
Nitpick comments:
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 98-109: The mention regex in renderContentWithMentions is
currently /(@\S+)/g which captures trailing punctuation; update the split
pattern to only capture the mention token (for example use a pattern like
/(@[A-Za-z0-9_]+)/g or /(@\w+)/g or a lookahead-based pattern that stops at word
boundaries) so punctuation stays in the surrounding text part; keep the rest of
renderContentWithMentions intact so parts that start with "@" are wrapped with
the "thread-event__mention" span and trailing punctuation will be returned as a
separate non-mention part.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44687589-b1a3-4003-bc44-528c7681b40c
📒 Files selected for processing (6)
src/frontend/src/features/layouts/components/thread-view/_index.scsssrc/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/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx
- src/frontend/src/features/layouts/components/thread-view/_index.scss
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 (1)
src/backend/core/api/openapi.json (1)
6113-6124:⚠️ Potential issue | 🟡 MinorUpdate the
stats_fieldsdescription forhas_notification.The enum now accepts
has_notification, but the prose on Line 6124 still omits it from the boolean-field list. That leaves the generated docs behind the actual accepted 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 6113 - 6124, The stats_fields enum now includes "has_notification" but the descriptive prose for stats_fields omits it from the boolean-field list; update the description string used for stats_fields to include has_notification among the Boolean fields (and clarify its unread variant 'has_notification_unread' follows the same unread-suffix behavior), keeping examples consistent with the other boolean fields and preserving existing formatting around the description.
♻️ Duplicate comments (12)
src/backend/core/models.py (2)
1383-1388:⚠️ Potential issue | 🟠 MajorStill missing composite indexes for the new event/notification query paths.
The new endpoints filter/order
ThreadEventby thread + chronology andUserNotificationby user/done/thread, but these Meta blocks only keep the default foreign-key indexes. That will age into table scans on the new folder/stats paths. Please add the compositeMeta.indexeshere and ship the matching migration.As per coding guidelines
src/backend/**/{models.py,migrations/**/*.py}: Implement database indexing and query optimization (Model Meta indexes, constraints).Also applies to: 1416-1420
🤖 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 1383 - 1388, The ThreadEvent model Meta (db_table "messages_threadevent", ordering = ["created_at"]) needs a composite index on (thread, created_at) to support the new filter/order by thread + chronology; likewise add a composite index on UserNotification for (user, done, thread) to support the user/done/thread query path. Add these indexes to the respective Model.Meta.indexes using django.db.models.Index with appropriate names, then run makemigrations and include the generated migration in the commit so the DB gets the new composite indexes.
1401-1420:⚠️ Potential issue | 🟠 MajorEnforce the notification→thread invariant at the model level.
has_notificationinsrc/backend/core/api/viewsets/thread.pytraversesThread.notifications. AUserNotificationsaved withthread=None— or with athread_idthat does not matchthread_event.thread_id— silently drops out of that filter and from stats. Makethreadrequired or derive/validate it fromthread_event, and back it with a DB constraint.As per coding guidelines
src/backend/**/{models.py,migrations/**/*.py}: Implement database indexing and query optimization (Model Meta indexes, constraints).🤖 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 1401 - 1420, The UserNotification model allows thread to be null which breaks Thread.notifications and metrics (see has_notification in src/backend/core/api/viewsets/thread.py); make thread non-nullable or automatically derive it from thread_event on save, and enforce consistency with a DB-level constraint and index: update the UserNotification model (fields thread and thread_event) so thread is required (or set in save()/clean() from thread_event.thread), add a CheckConstraint (or ForeignKey constraint) that thread_id = thread_event.thread_id to prevent mismatched pairs, and add appropriate Meta.indexes for (thread_id, created_at) to optimize Thread.notifications queries. Ensure migrations are created for the constraint and indexes.src/backend/core/api/viewsets/thread.py (1)
115-120:⚠️ Potential issue | 🟡 MinorHandle
has_notification=0explicitly.Only the positive branch is implemented here.
?has_notification=0currently falls through to the default queryset instead of returning threads without pending notifications.🤖 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 115 - 120, The current dynamic filter only handles has_notification == "1" and ignores has_notification == "0"; update the logic in the Thread viewset where query_params and queryset are used (the block referencing has_notification, notifications__user, notifications__is_done and self.request.user) to explicitly handle has_notification == "0" by returning threads that do NOT have unresolved notifications for the current user (use queryset.exclude(...) or an equivalent Q-based filter on notifications__user and notifications__is_done to invert the condition), keeping the existing distinct() behavior for the positive branch.src/backend/core/api/viewsets/notification.py (1)
58-68:⚠️ Potential issue | 🟠 MajorValidate
is_doneandthread_idbefore filtering.
is_done=flaseis treated likefalse, and a malformedthread_idcan escape into the UUID lookup as a 500. Normalize both params up front and return a 400 for invalid values instead of silently coercing or crashing.Suggested fix
+import uuid from drf_spectacular.types import OpenApiTypes from drf_spectacular.utils import OpenApiParameter, extend_schema from rest_framework import mixins, viewsets +from rest_framework.exceptions import ValidationError @@ # Apply optional filters is_done = self.request.query_params.get("is_done") if is_done is not None: - queryset = queryset.filter(is_done=is_done.lower() in ("true", "1")) + normalized = is_done.strip().lower() + if normalized not in {"true", "false", "1", "0"}: + raise ValidationError( + {"is_done": "Expected one of: true, false, 1, 0."} + ) + queryset = queryset.filter(is_done=normalized in {"true", "1"}) @@ thread_id = self.request.query_params.get("thread_id") if thread_id: - queryset = queryset.filter(thread_id=thread_id) + try: + thread_uuid = uuid.UUID(str(thread_id)) + except ValueError as exc: + raise ValidationError({"thread_id": "Invalid UUID."}) from exc + queryset = queryset.filter(thread_id=thread_uuid)#!/bin/bash set -euo pipefail echo "=== Current notification filters ===" sed -n '51,70p' src/backend/core/api/viewsets/notification.py echo echo "=== Existing UUID-validation pattern elsewhere in the repo ===" fd 'message.py$' src/backend/core/api/viewsets -x sed -n '175,205p' {}Expected result: the current viewset coerces
is_doneand passes rawthread_idinto the filter, while the repo already uses explicit UUID validation patterns elsewhere.
As per coding guidelinessrc/backend/**/*.py: Prefer try-except blocks to handle exceptions in business logic and views.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/viewsets/notification.py` around lines 58 - 68, The code currently coerces is_done and passes raw thread_id into queryset filters, causing incorrect truthy parsing (e.g., "flase") and potential 500s for malformed UUIDs; update the viewset to validate both params up front: for is_done (the request.query_params "is_done") accept only explicit values like "true","1","false","0" (case-insensitive), convert to a boolean, and return a 400 (raise a DRF ValidationError or return Response 400) for any other value before calling queryset.filter(is_done=...); for thread_id validate using uuid.UUID(thread_id) inside a try/except (catch ValueError) and return 400 for invalid UUIDs, then call queryset.filter(thread_id=validated_uuid) instead of passing the raw string; ensure you reference the existing variables/expressions (is_done, thread_id, queryset.filter(...)) and use a try/except per project guidelines.src/backend/core/api/openapi.json (1)
5554-5562:⚠️ Potential issue | 🟡 MinorRemove
mailbox_idfrom the ThreadAccess detail operations.Line 5555, Line 5624, and Line 5692 add a collection-style filter to
/threads/{thread_id}/accesses/{id}/. That endpoint is already uniquely scoped bythread_id+id, so exposingmailbox_idhere makes the contract broader than the route semantics.Also applies to: 5623-5631, 5691-5699
🤖 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 5554 - 5562, The ThreadAccess detail operations expose a collection-style query parameter "mailbox_id" which is inappropriate for the uniquely-scoped endpoint /threads/{thread_id}/accesses/{id}/; remove the "mailbox_id" parameter entry from the OpenAPI definition in the ThreadAccess detail operations (the objects adding "name": "mailbox_id" and its schema) so the detail route only accepts path parameters (thread_id and id) and not the mailbox_id filter.src/backend/core/tests/api/test_thread_event_permissions.py (1)
532-533:⚠️ Potential issue | 🟡 MinorCompare against the serialized UUID string.
DRF serializes this FK as a string in
response.data, so comparing it directly tothread_a.idwill fail even when the endpoint is behaving correctly.Suggested fix
- assert event["thread"] == thread_a.id + assert event["thread"] == str(thread_a.id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_thread_event_permissions.py` around lines 532 - 533, The test is comparing the DRF-serialized FK string against the UUID object thread_a.id which will fail; update the assertion in the loop (for event in response.data.get("results", []): ...) to compare event["thread"] to the serialized UUID string by converting thread_a.id to a string (e.g., str(thread_a.id)) so the types match when asserting equality.src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
215-225:⚠️ Potential issue | 🟠 MajorKeep
data-created-aton the fallback renderer.The observer above advances
markAsReadAtfrom this attribute. ThethreadItemsbranch sets it, but the fallbackmessages.map(...)branch still omits it, so unread tracking breaks wheneverthreadItemsisnull.Suggested fix
<ThreadMessage key={message.id} message={message} isLatest={isLatest} ref={isUnread ? (el => { unreadRefs.current[message.id] = el; }) : undefined} data-message-id={message.id} + data-created-at={message.created_at} draftMessage={message.draft_message} />🤖 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/index.tsx` around lines 215 - 225, The fallback renderer inside the messages.map branch is missing the data-created-at attribute that the observer uses to advance markAsReadAt; update the ThreadMessage call in that branch to include data-created-at={message.created_at} (same attribute used in the threadItems branch) so the unread tracking (observer/markAsReadAt) and unreadRefs logic continue to work when threadItems is null.src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss (1)
44-50:⚠️ Potential issue | 🟠 MajorReplace deprecated
word-break: break-word.Stylelint already flags both declarations. Use
overflow-wrap: anywherewithword-break: normalso long content still wraps without the deprecated keyword.Suggested fix
.thread-event--im .thread-event__content { color: var(--c--contextuals--content--semantic--neutral--primary); font-size: var(--c--globals--font--sizes--sm); line-height: 1.5; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: anywhere; + word-break: normal; padding: var(--c--globals--spacings--xs) var(--c--globals--spacings--sm); } @@ .thread-event--generic .thread-event__content { font-size: var(--c--globals--font--sizes--sm); color: var(--c--contextuals--content--semantic--neutral--secondary); line-height: 1.5; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: anywhere; + word-break: normal; }Also applies to: 98-103
🤖 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.scss` around lines 44 - 50, Update the CSS for the affected selectors (e.g., .thread-event--im .thread-event__content and the other occurrence around lines 98-103) to remove the deprecated word-break: break-word; and instead use word-break: normal; plus overflow-wrap: anywhere; so long strings still wrap correctly; keep the existing white-space, line-height and padding rules intact when making this replacement.src/backend/core/api/viewsets/thread_event.py (1)
44-70:⚠️ Potential issue | 🔴 CriticalValidate mentions before saving the event.
serializer.save()happens beforemention_idsare normalized. A body like{"mention_ids": [{}]}will blow up onset(mention_ids)after theThreadEventrow is already written, so the request can 500 with partial state. Read validated mentions from the serializer and wrap the event + notification inserts in one transaction.🤖 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 44 - 70, In perform_create, avoid calling serializer.save() before normalizing/validating mention_ids: read mention_ids from serializer.validated_data (e.g. serializer.validated_data.get("mention_ids", [])), normalize/deduplicate and exclude self-mentions, then open a single transaction (transaction.atomic) and inside it call serializer.save(thread=thread, author=self.request.user) to create the ThreadEvent and then create UserNotification rows (models.UserNotification.objects.bulk_create) only for validated mailbox-accessible user IDs (use models.MailboxAccess filter as before); this ensures invalid mention payloads don’t leave a partially written ThreadEvent and all writes are atomic.src/backend/core/api/serializers.py (2)
951-970:⚠️ Potential issue | 🟠 MajorReject cross-thread
messagereferences.
messageis writable on create, but this serializer still does not ensure it belongs to the nestedthread_id. That lets/threads/A/events/persist an event pointing at a message from thread B.🤖 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 951 - 970, ThreadEventSerializer currently allows creating an event with a writable message that may belong to a different thread; update ThreadEventSerializer to enforce the message belongs to the nested thread_id by adding validation in the serializer (e.g., implement validate() or validate_message()) that loads the provided message instance and checks message.thread.id (or message.thread) equals the serializer's thread value (or the incoming thread field), and raise serializers.ValidationError if they differ; ensure this check runs during create so /threads/<id>/events/ cannot reference messages from other threads.
951-970:⚠️ Potential issue | 🟠 MajorExpose
mention_idsin the serializer contract.The viewset accepts
mention_ids, but this serializer/schema still omits it. That keeps OpenAPI and the generatedThreadEventRequestout of sync with the actual API, which is why the frontend still has to cast this payload instead of sending a typed request. A write-onlyListField(UUIDField())here would also letperform_create()consume validated data instead of rawrequest.data.🤖 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 951 - 970, Add a write-only ListField of UUIDs named "mention_ids" to ThreadEventSerializer so the serializer contract matches the viewset; specifically, in class ThreadEventSerializer add something like mention_ids = serializers.ListField(child=serializers.UUIDField(), write_only=True), include "mention_ids" in the serializer's fields list (and/or create_only_fields if you want it only on create), and ensure perform_create() in the viewset consumes validated_data["mention_ids"] instead of raw request.data so OpenAPI/ThreadEventRequest stay in sync with the actual API.src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx (1)
23-25:⚠️ Potential issue | 🟠 MajorRecompute
mention_idsfrom the current textarea value.This state only grows when a suggestion is picked. If the user deletes or edits that
@mentionbefore sending, the old user ID still goes out and they receive a false notification.Also applies to: 44-55, 101-107
🤖 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/admin.py`:
- Around line 425-431: ThreadEventInline currently only autocompletes "author",
causing full select widgets for message and channel which hurts performance;
update ThreadEventInline to add "channel" to autocomplete_fields and add
"message" to raw_id_fields (mirroring MessageAdmin's FK handling) so admin uses
efficient lookup widgets — modify the ThreadEventInline class to set
autocomplete_fields = ("author","channel") and raw_id_fields = ("message",)
while keeping readonly_fields and extra unchanged.
In `@src/backend/core/api/openapi.json`:
- Around line 5563-5589: The OpenAPI spec exposes duplicate sources for the
thread relation: the path parameter thread_id and the body schema
ThreadAccessRequest (which currently requires thread), causing PUTs to demand
the same relation twice; update the spec so endpoints that include the path
param thread_id no longer require a body-level thread. Specifically, remove or
make optional the thread property from ThreadAccessRequest for operations that
have the path param (the PUT operation and the other occurrences noted around
lines 5632-5658), or introduce separate schemas (e.g., ThreadAccessCreate with
thread allowed and ThreadAccessUpdate without required thread) and reference the
appropriate schema for each operation, ensuring the server-side contract uses
the path thread_id as the single source of truth.
- Around line 8438-8442: The schema for ThreadEvent.type is currently an
unconstrained string; change it to an enum of implemented event kinds (e.g.,
replace the open string schema for ThreadEvent.type with an "enum": ["im"]) so
generated clients will only accept supported values; apply the same change to
the other identical schemas noted (the other occurrences of ThreadEvent.type in
the file) so all ThreadEvent.type definitions are constrained to the implemented
event kinds and can be widened later as new kinds are added.
In `@src/backend/core/tests/api/test_thread_event.py`:
- Around line 108-109: The test is comparing a serialized UUID to a UUID object;
update the assertions that reference response.data["thread"] to compare against
the string form of the UUID by replacing uses of thread.id with str(thread.id)
so the assertion reads like assert response.data["thread"] == str(thread.id); do
this for both occurrences that currently compare response.data["thread"] to
thread.id.
In `@src/frontend/src/features/layouts/components/thread-view/_index.scss`:
- Line 9: The .thread-view rule currently sets height: 100%, which
over-constrains the element when at the mobile breakpoint where .thread-view
becomes position: fixed with top: var(--header-height) and bottom: 0; update the
media query for the mobile/fixed layout to override that rule to height: auto
(i.e., in the media query that applies position: fixed/top/bottom to
.thread-view set height: auto); also apply the same override to the other
identical occurrences of the .thread-view height rule later in the file (the
block covering lines 45-53) so the bottom inset is respected.
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 6113-6124: The stats_fields enum now includes "has_notification"
but the descriptive prose for stats_fields omits it from the boolean-field list;
update the description string used for stats_fields to include has_notification
among the Boolean fields (and clarify its unread variant
'has_notification_unread' follows the same unread-suffix behavior), keeping
examples consistent with the other boolean fields and preserving existing
formatting around the description.
---
Duplicate comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5554-5562: The ThreadAccess detail operations expose a
collection-style query parameter "mailbox_id" which is inappropriate for the
uniquely-scoped endpoint /threads/{thread_id}/accesses/{id}/; remove the
"mailbox_id" parameter entry from the OpenAPI definition in the ThreadAccess
detail operations (the objects adding "name": "mailbox_id" and its schema) so
the detail route only accepts path parameters (thread_id and id) and not the
mailbox_id filter.
In `@src/backend/core/api/serializers.py`:
- Around line 951-970: ThreadEventSerializer currently allows creating an event
with a writable message that may belong to a different thread; update
ThreadEventSerializer to enforce the message belongs to the nested thread_id by
adding validation in the serializer (e.g., implement validate() or
validate_message()) that loads the provided message instance and checks
message.thread.id (or message.thread) equals the serializer's thread value (or
the incoming thread field), and raise serializers.ValidationError if they
differ; ensure this check runs during create so /threads/<id>/events/ cannot
reference messages from other threads.
- Around line 951-970: Add a write-only ListField of UUIDs named "mention_ids"
to ThreadEventSerializer so the serializer contract matches the viewset;
specifically, in class ThreadEventSerializer add something like mention_ids =
serializers.ListField(child=serializers.UUIDField(), write_only=True), include
"mention_ids" in the serializer's fields list (and/or create_only_fields if you
want it only on create), and ensure perform_create() in the viewset consumes
validated_data["mention_ids"] instead of raw request.data so
OpenAPI/ThreadEventRequest stay in sync with the actual API.
In `@src/backend/core/api/viewsets/notification.py`:
- Around line 58-68: The code currently coerces is_done and passes raw thread_id
into queryset filters, causing incorrect truthy parsing (e.g., "flase") and
potential 500s for malformed UUIDs; update the viewset to validate both params
up front: for is_done (the request.query_params "is_done") accept only explicit
values like "true","1","false","0" (case-insensitive), convert to a boolean, and
return a 400 (raise a DRF ValidationError or return Response 400) for any other
value before calling queryset.filter(is_done=...); for thread_id validate using
uuid.UUID(thread_id) inside a try/except (catch ValueError) and return 400 for
invalid UUIDs, then call queryset.filter(thread_id=validated_uuid) instead of
passing the raw string; ensure you reference the existing variables/expressions
(is_done, thread_id, queryset.filter(...)) and use a try/except per project
guidelines.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 44-70: In perform_create, avoid calling serializer.save() before
normalizing/validating mention_ids: read mention_ids from
serializer.validated_data (e.g. serializer.validated_data.get("mention_ids",
[])), normalize/deduplicate and exclude self-mentions, then open a single
transaction (transaction.atomic) and inside it call
serializer.save(thread=thread, author=self.request.user) to create the
ThreadEvent and then create UserNotification rows
(models.UserNotification.objects.bulk_create) only for validated
mailbox-accessible user IDs (use models.MailboxAccess filter as before); this
ensures invalid mention payloads don’t leave a partially written ThreadEvent and
all writes are atomic.
In `@src/backend/core/api/viewsets/thread.py`:
- Around line 115-120: The current dynamic filter only handles has_notification
== "1" and ignores has_notification == "0"; update the logic in the Thread
viewset where query_params and queryset are used (the block referencing
has_notification, notifications__user, notifications__is_done and
self.request.user) to explicitly handle has_notification == "0" by returning
threads that do NOT have unresolved notifications for the current user (use
queryset.exclude(...) or an equivalent Q-based filter on notifications__user and
notifications__is_done to invert the condition), keeping the existing distinct()
behavior for the positive branch.
In `@src/backend/core/models.py`:
- Around line 1383-1388: The ThreadEvent model Meta (db_table
"messages_threadevent", ordering = ["created_at"]) needs a composite index on
(thread, created_at) to support the new filter/order by thread + chronology;
likewise add a composite index on UserNotification for (user, done, thread) to
support the user/done/thread query path. Add these indexes to the respective
Model.Meta.indexes using django.db.models.Index with appropriate names, then run
makemigrations and include the generated migration in the commit so the DB gets
the new composite indexes.
- Around line 1401-1420: The UserNotification model allows thread to be null
which breaks Thread.notifications and metrics (see has_notification in
src/backend/core/api/viewsets/thread.py); make thread non-nullable or
automatically derive it from thread_event on save, and enforce consistency with
a DB-level constraint and index: update the UserNotification model (fields
thread and thread_event) so thread is required (or set in save()/clean() from
thread_event.thread), add a CheckConstraint (or ForeignKey constraint) that
thread_id = thread_event.thread_id to prevent mismatched pairs, and add
appropriate Meta.indexes for (thread_id, created_at) to optimize
Thread.notifications queries. Ensure migrations are created for the constraint
and indexes.
In `@src/backend/core/tests/api/test_thread_event_permissions.py`:
- Around line 532-533: The test is comparing the DRF-serialized FK string
against the UUID object thread_a.id which will fail; update the assertion in the
loop (for event in response.data.get("results", []): ...) to compare
event["thread"] to the serialized UUID string by converting thread_a.id to a
string (e.g., str(thread_a.id)) so the types match when asserting equality.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss`:
- Around line 44-50: Update the CSS for the affected selectors (e.g.,
.thread-event--im .thread-event__content and the other occurrence around lines
98-103) to remove the deprecated word-break: break-word; and instead use
word-break: normal; plus overflow-wrap: anywhere; so long strings still wrap
correctly; keep the existing white-space, line-height and padding rules intact
when making this replacement.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 215-225: The fallback renderer inside the messages.map branch is
missing the data-created-at attribute that the observer uses to advance
markAsReadAt; update the ThreadMessage call in that branch to include
data-created-at={message.created_at} (same attribute used in the threadItems
branch) so the unread tracking (observer/markAsReadAt) and unreadRefs logic
continue to work when threadItems is null.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1d5c66b9-eed0-4970-a8a2-2ab974c8d287
⛔ Files ignored due to path filters (18)
src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_partial_update_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/notifications_update_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_thread_event_list.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/paginated_user_notification_list.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_user_notification_request.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_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_events_list_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/models/user_notification.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/user_notification_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/notifications/notifications.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (28)
Makefilesrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/notification.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/management/commands/createsuperuser.pysrc/backend/core/migrations/0023_threadevent_usernotification.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_notification.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/urls.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/_index.scsssrc/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/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/pages/_app.tsx
✅ Files skipped from review due to trivial changes (4)
- src/frontend/public/locales/common/en-US.json
- src/backend/core/enums.py
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss
🚧 Files skipped from review as they are similar to previous changes (9)
- Makefile
- src/frontend/src/pages/_app.tsx
- src/backend/core/factories.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx
- src/backend/core/api/permissions.py
- src/frontend/src/features/providers/mailbox.tsx
- src/backend/core/urls.py
- src/backend/core/tests/api/test_notification.py
- src/backend/core/management/commands/createsuperuser.py
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
src/backend/core/api/viewsets/thread_event.py (1)
45-71:⚠️ Potential issue | 🔴 CriticalValidate
mention_idsbefore saving the event.
serializer.save(...)happens before any payload validation here. A malformedmention_idsvalue can therefore create theThreadEventand then fail during notification creation, leaving partial state behind. Please validate the list up front and make the event/notification writes atomic.🤖 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 45 - 71, The perform_create currently calls serializer.save(...) before validating mention_ids which can leave a created ThreadEvent if notification creation later fails; fix by validating mention_ids first (ensure it's a list/iterable of IDs, convert/deduplicate to a set, exclude self ID, and verify allowed IDs via models.MailboxAccess) and raise a DRF ValidationError on bad input, then perform the save and notification creation inside a single transaction (use transaction.atomic) so models.UserNotification bulk_create and serializer.save (creating the ThreadEvent) are atomic; update perform_create to validate mention_ids up front, then inside transaction.atomic call serializer.save(thread=thread, author=self.request.user) and create notifications using models.UserNotification and bulk_create.src/backend/core/api/openapi.json (2)
4563-4587:⚠️ Potential issue | 🟡 MinorSame issue remains: remove list filters from notification detail routes.
/notifications/{id}/already identifies a single record. Keepingis_done,thread_id, andtypeon the retrieve/update operations makes the generated contract look filterable when it is not.Also applies to: 4622-4646, 4695-4719
🤖 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 4563 - 4587, The OpenAPI spec incorrectly defines query parameters is_done, thread_id, and type on the notification detail path (/notifications/{id}/) making a singular retrieve/update endpoint appear filterable; remove those query parameter entries from the /notifications/{id}/ path item (both GET and PUT/PATCH operations) so the detail operations only use the path parameter id; search for any other /notifications/{id}/ path items (duplicate occurrences mentioned) and delete the same is_done, thread_id, and type parameter definitions there as well.
5555-5589:⚠️ Potential issue | 🟠 MajorSame ambiguity remains on nested
ThreadAccesswrites.These detail routes already identify the target with
thread_idandid, yetmailbox_idis still documented as a filter here, and PUT/PATCH still reuse request schemas that carry body-levelthread. That leaves multiple competing targeting inputs if values disagree.Also applies to: 5624-5658, 5692-5699
🤖 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 5555 - 5589, The OpenAPI docs allow competing targets for nested ThreadAccess writes: paths already identify thread via path params (thread_id and id) but still expose mailbox_id as a query filter and reuse ThreadAccessRequest which contains a body-level thread field; update the routes (those using path params thread_id and id, including the referenced ranges) to either remove mailbox_id as a query parameter for these nested write endpoints and/or stop reusing ThreadAccessRequest for PUT/PATCH and instead reference a dedicated ThreadAccessWrite schema that omits the body-level thread (or validates/forbids it), ensuring the canonical target is the path param and there are no conflicting body/query fields.
🧹 Nitpick comments (1)
src/backend/core/api/openapi.json (1)
8419-8419: Prefer an explicit object schema fordata.Empty schemas here allow scalars/arrays and collapse generated client types to
any. If these payloads are object-shaped in practice, make that explicit.🧩 Suggested schema tightening
- "data": {} + "data": { + "type": "object", + "additionalProperties": true + }Also applies to: 8993-8993, 9032-9032, 9181-9183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/api/openapi.json` at line 8419, The OpenAPI schema uses empty schemas for the "data" property which permits scalars/arrays and generates loose client types; update each "data" schema (the occurrences flagged around the current diff plus the ones near the other reported locations) to an explicit object schema (at minimum "type": "object", and preferably include "properties": {...} and "additionalProperties": false or the specific fields expected) so generated clients get correct object types; locate the schemas named "data" in the openapi.json (the entries shown in the diff and the other reported positions) and replace {} with the explicit object schema describing the payload shape.
🤖 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/openapi.json`:
- Around line 6114-6118: Update the "stats_fields" description string to reflect
the current enum: include "has_notification" (and any other fields present in
the enum) in the list of boolean aggregate fields and keep the note about unread
variants ('_unread' suffix) and special values ('all', 'all_unread'); edit the
description associated with the stats_fields/openapi property so the textual
list exactly matches the enum entries (e.g., include has_notification alongside
has_delivery_pending/has_delivery_failed, has_messages, is_spam, etc.).
In `@src/backend/core/migrations/0023_threadevent_usernotification.py`:
- Around line 29-34: The migration only creates FK indexes; add composite
Meta.indexes to match the new query shapes: for ThreadEvent add an index on
(thread, created_at) to support thread-scoped ordered reads, and for
UserNotification add a user-centric index that supports unread + recency lookups
(e.g., (user, is_read, created_at) or (user, is_read, -created_at) depending on
your ORM/index direction). Update the corresponding model Meta.indexes
(ThreadEvent and UserNotification), regenerate and commit the migration (replace
contents of 0023_threadevent_usernotification.py with the new migration that
includes these Index() entries) so the DB will have composite indexes aligned
with the query predicates.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx`:
- Around line 60-65: The onSuccess handler currently clears content and
mentionIds but leaves the mention popover state and filter stale; update the
onSuccess callback in the thread-event-input component to also close the mention
popover and reset the mention filter by calling setShowMentionPopover(false) and
resetting mentionFilter (e.g., setMentionFilter("") or equivalent) after
setContent and setMentionIds and before/after
invalidateThreadEvents/onEventCreated so that suggestions are not left visible
over the cleared textarea.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 266-273: filteredThreadItems currently rebuilds items from raw
threadItems and loses nested draft_message info from filteredMessages; update
the useMemo so that for message items you look up the corresponding message in
filteredMessages (by id) and use that message as item.data (preserving
draft_message), leaving event items unchanged; modify the filteredThreadItems
logic (referencing filteredThreadItems, threadItems, filteredMessages, Message
and draft_message) so ThreadViewComponent receives items whose data includes the
draft_reply/draft_message fields.
---
Duplicate comments:
In `@src/backend/core/api/openapi.json`:
- Around line 4563-4587: The OpenAPI spec incorrectly defines query parameters
is_done, thread_id, and type on the notification detail path
(/notifications/{id}/) making a singular retrieve/update endpoint appear
filterable; remove those query parameter entries from the /notifications/{id}/
path item (both GET and PUT/PATCH operations) so the detail operations only use
the path parameter id; search for any other /notifications/{id}/ path items
(duplicate occurrences mentioned) and delete the same is_done, thread_id, and
type parameter definitions there as well.
- Around line 5555-5589: The OpenAPI docs allow competing targets for nested
ThreadAccess writes: paths already identify thread via path params (thread_id
and id) but still expose mailbox_id as a query filter and reuse
ThreadAccessRequest which contains a body-level thread field; update the routes
(those using path params thread_id and id, including the referenced ranges) to
either remove mailbox_id as a query parameter for these nested write endpoints
and/or stop reusing ThreadAccessRequest for PUT/PATCH and instead reference a
dedicated ThreadAccessWrite schema that omits the body-level thread (or
validates/forbids it), ensuring the canonical target is the path param and there
are no conflicting body/query fields.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 45-71: The perform_create currently calls serializer.save(...)
before validating mention_ids which can leave a created ThreadEvent if
notification creation later fails; fix by validating mention_ids first (ensure
it's a list/iterable of IDs, convert/deduplicate to a set, exclude self ID, and
verify allowed IDs via models.MailboxAccess) and raise a DRF ValidationError on
bad input, then perform the save and notification creation inside a single
transaction (use transaction.atomic) so models.UserNotification bulk_create and
serializer.save (creating the ThreadEvent) are atomic; update perform_create to
validate mention_ids up front, then inside transaction.atomic call
serializer.save(thread=thread, author=self.request.user) and create
notifications using models.UserNotification and bulk_create.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Line 8419: The OpenAPI schema uses empty schemas for the "data" property which
permits scalars/arrays and generates loose client types; update each "data"
schema (the occurrences flagged around the current diff plus the ones near the
other reported locations) to an explicit object schema (at minimum "type":
"object", and preferably include "properties": {...} and "additionalProperties":
false or the specific fields expected) so generated clients get correct object
types; locate the schemas named "data" in the openapi.json (the entries shown in
the diff and the other reported positions) and replace {} with the explicit
object schema describing the payload shape.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 70615145-8321-47ae-8ba7-e0b3dd1b1f54
⛔ Files ignored due to path filters (8)
src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create_metadata_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create_metadata_type_enum.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/thread_event.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_type_enum.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (12)
src/backend/core/api/openapi.jsonsrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/migrations/0023_threadevent_usernotification.pysrc/backend/core/models.pysrc/frontend/src/features/layouts/components/thread-view/_index.scsssrc/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/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/providers/mailbox.tsx
✅ Files skipped from review due to trivial changes (1)
- src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss
🚧 Files skipped from review as they are similar to previous changes (3)
- src/frontend/src/features/layouts/components/thread-view/_index.scss
- src/frontend/src/features/providers/mailbox.tsx
- src/backend/core/models.py
jbpenrath
changed the title
jbpenrath
force-pushed
the
threadevents
branch
9 times, most recently
from
1c5eb4d to
cbf7bdb
Compare
jbpenrath
force-pushed
the
threadevents
branch
4 times, most recently
from
bdd991b to
7441e1f
Compare
jbpenrath
force-pushed
the
threadevents
branch
2 times, most recently
from
71839bd to
afc9df0
Compare
Contributor
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss (1)
39-45:⚠️ Potential issue | 🟡 MinorResolve the remaining deprecated
word-breakdeclarations.Stylelint is still flagging Lines 44 and 147, so the earlier
break-worddeprecation issue is still present in the final diff. Replace it withoverflow-wrap: anywhereplusword-break: normal.🧹 Proposed fix
.thread-event--im .thread-event__content { color: var(--c--contextuals--content--semantic--neutral--primary); font-size: var(--c--globals--font--sizes--sm); line-height: 1.5; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: anywhere; + word-break: normal; padding: var(--c--globals--spacings--xs) var(--c--globals--spacings--sm); margin-inline: var(--c--globals--spacings--xs); transition: transform 0.2s ease; background-color: var(--c--contextuals--background--semantic--neutral--secondary); border-radius: var(--c--globals--spacings--3xs) var(--c--globals--spacings--base) var(--c--globals--spacings--base) var(--c--globals--spacings--base); } ... .thread-event--generic .thread-event__content { font-size: var(--c--globals--font--sizes--sm); color: var(--c--contextuals--content--semantic--neutral--secondary); line-height: 1.5; white-space: pre-wrap; - word-break: break-word; + overflow-wrap: anywhere; + word-break: normal; }Also applies to: 142-148
🤖 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.scss` around lines 39 - 45, The CSS still uses the deprecated word-break: break-word in the .thread-event--im .thread-event__content rule (and the similar rule around lines 142–148); update those rules by replacing word-break: break-word with overflow-wrap: anywhere and set word-break: normal to preserve intended wrapping behavior; locate the declarations in the .thread-event--im .thread-event__content and the other .thread-event__content/related selectors and apply the two-property replacement consistently.src/frontend/src/features/providers/mailbox.tsx (1)
317-332:⚠️ Potential issue | 🟡 MinorDon't block the merged timeline on
messagesQuery.
threadItemsstill returnsnullwhilemessagesQuery.data?.datais undefined, so a resolvedthreadEventsQueryis hidden during the initial message load. Default both sources to[]and only returnnullwhen there is no selected thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/providers/mailbox.tsx` around lines 317 - 332, threadItems currently returns null whenever messagesQuery.data?.data is undefined which hides a resolved threadEventsQuery; change the logic in the useMemo so both message and event sources default to empty arrays (e.g. const messages = messagesQuery.data?.data ?? []; const events = threadEventsQuery.data?.data ?? []), merge and sort those arrays, and only return null if there is no selected thread (check your selectedThread/selectedThreadId variable) instead of returning null based on messagesQuery alone; update references to TimelineItem, messagesQuery, threadEventsQuery and threadItems accordingly.src/backend/core/tests/api/test_thread_event_permissions.py (1)
311-339:⚠️ Potential issue | 🟠 MajorThese parameter-confusion tests need a positive control.
Right now they can pass on an empty response, so they do not prove the query params were ignored. In the
thread_idcase specifically, if this endpoint is paginated, iteratingresponse.datawalks wrapper keys instead of event objects. Seed one allowed-thread event and assert the returned collection contains that event and excludes the blocked one; I'd mirror the same pattern in themailbox_idtest below.🧪 Suggested test shape
def test_url_thread_id_ignores_query_param_thread_id(self, api_client): """Passing ?thread_id=X on a nested /threads/Y/events/ should not affect results.""" user, _mailbox, thread_a = setup_user_with_thread_access() api_client.force_authenticate(user=user) + allowed_event = factories.ThreadEventFactory(thread=thread_a, author=user) # Thread B the user has no access to thread_b = factories.ThreadFactory() - factories.ThreadEventFactory(thread=thread_b) + blocked_event = factories.ThreadEventFactory(thread=thread_b) # Try to list events with confusing params url = f"{get_thread_event_url(thread_a.id)}?thread_id={thread_b.id}" response = api_client.get(url) assert response.status_code == status.HTTP_200_OK - # Should only return events from thread_a (the URL path), not thread_b - for event in response.data: - assert event["thread"] == thread_a.id + returned_ids = {item["id"] for item in response.data["results"]} + assert returned_ids == {str(allowed_event.id)} + assert str(blocked_event.id) not in returned_idsRun this read-only check to confirm the list response shape before updating the assertion:
#!/bin/bash set -euo pipefail echo "=== DRF pagination settings ===" rg -n "DEFAULT_PAGINATION_CLASS|PAGE_SIZE|pagination_class" src/backend --type py echo echo "=== ThreadEvent viewset ===" rg -n "class ThreadEventViewSet|pagination_class|get_queryset|def list" src/backend --type py -A20 -B5 echo echo "=== ThreadEvent serializer ===" rg -n "class ThreadEventSerializer|thread\\s*=|fields\\s*=" src/backend/core/api/serializers.py -A20 -B5 echo echo "=== Existing paginated list assertions in API tests ===" rg -n 'response\.data(\["results"\]|\.get\("results"' src/backend/core/tests --type py -A2 -B2Expected result: this endpoint uses the standard paginated list shape, so assertions should read from
response.data["results"]and compare serialized IDs.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_permissions.py` around lines 311 - 339, The tests test_url_thread_id_ignores_query_param_thread_id and test_url_thread_id_ignores_query_param_mailbox_id lack positive controls and may pass on empty/paginated responses; update each to create/seeding: (1) an allowed ThreadEvent via factories.ThreadEventFactory(thread=thread_a) (or thread=thread for mailbox test), and (2) a blocked ThreadEvent on the disallowed thread/mailbox, then call get_thread_event_url as before and read the list from response.data["results"] if present (fall back to response.data for non-paginated shape) and assert the results include the allowed event's id and do not include the blocked event's id; use the existing helper get_thread_event_url and factories.ThreadEventFactory to locate where to change assertions.
🧹 Nitpick comments (3)
src/frontend/src/features/ui/components/suggestion-input/index.tsx (1)
83-89: Type assertionnull as Tbypasses type safety.The
selectItem(null as T)cast works functionally to reset the selection but is misleading sinceTmay not includenull. Downshift'sselectItemdoes acceptnullfor resetting.♻️ Safer alternative using type annotation
- selectItem(null as T); + selectItem(null as unknown as T);Or define the component to explicitly handle nullable items:
// If downshift's types allow, use the proper null type: selectItem(null);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/ui/components/suggestion-input/index.tsx` around lines 83 - 89, The code uses a misleading cast `selectItem(null as T)` inside the onSelectedItemChange handler (function around onSelectedItemChange/selectItem/onSelect) which bypasses type safety; instead remove the assertion and call selectItem(null) directly if Downshift's types allow null, or update the component/generic/type parameter so the selected item type is nullable (e.g., use T | null in the component/prop/generic definition) so selectItem(null) is type-safe; ensure the change touches the selectItem call in onSelectedItemChange and the component's generic/props where T is declared.src/backend/core/api/viewsets/thread_event.py (1)
24-31: Consider adding pagination for large event lists.Setting
pagination_class = Nonereturns all events in a single response. If threads accumulate many IM events over time, this could impact performance. Consider using a cursor-based pagination or limiting results if the IM feature is expected to have high-volume threads.🤖 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 24 - 31, The viewset currently disables pagination by setting pagination_class = None which can return all ThreadEvents at once; restore pagination on the ThreadEvent viewset (the class using serializer_class = serializers.ThreadEventSerializer and permission_classes = [permissions.IsAuthenticated, permissions.IsAllowedToAccess]) by assigning a cursor or limit-offset paginator (e.g., a custom CursorPagination with a sensible page_size and ordering by the event timestamp) instead of None, or add a specific pagination_class like ThreadEventCursorPagination and wire it into the viewset so large IM event lists are returned in pages.src/backend/core/api/openapi.json (1)
5456-5493: Paginatethreads_events_listbefore the contract hardens.This list currently returns a raw array with no pagination controls. IM histories only grow, so large threads will eventually turn this into an unbounded payload, and adding pagination later will be a breaking API change. Prefer the same paginated shape already used by other collection endpoints.
🤖 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 5456 - 5493, The threads_events_list GET currently returns a raw array of ThreadEvent objects; change its OpenAPI response schema at "/api/v1.0/threads/{thread_id}/events/" (operationId threads_events_list) to use the project’s standard paginated collection shape (e.g., object with fields like count/total, next, previous and results which is an array of ThreadEvent) instead of a top-level array, and add the same pagination query parameters (page/limit or offset/limit consistent with other endpoints) to the parameters list so the contract is paginated and matches other collection endpoints.
🤖 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/openapi.json`:
- Around line 5774-5781: Replace the heavy UserWithoutAbilities schema on
IM/thread surfaces with a slimmer ThreadParticipant schema: add a new
components/schemas/ThreadParticipant containing only id, full_name, and an
optional email, then update references where participant lookups and
ThreadEvent.author currently point to UserWithoutAbilities (search for
"ThreadEvent.author" and the participant lookup responses that reference "$ref":
"#/components/schemas/UserWithoutAbilities") to instead reference "$ref":
"#/components/schemas/ThreadParticipant"; ensure the new schema is used
consistently across the other occurrences mentioned (the other block around the
second occurrence) so IM UI consumers only receive minimal profile fields.
- Around line 8145-8195: ThreadEvent and ThreadEventRequest currently allow an
"im" event without the "data" payload; update those schemas to require the
"data" object for the IM event just like PatchedThreadEventRequest does. Locate
the ThreadEvent and ThreadEventRequest schema definitions and either (a) make
the "im" variant a distinct oneOf entry that includes "required": ["data"] and
the same data object shape as PatchedThreadEventRequest, or (b) add an OpenAPI
conditional (if type == "im" then require data) so generated clients cannot omit
data for type "im"; ensure the "data" child schema matches the content/mentions
structure used in PatchedThreadEventRequest.
In `@src/backend/core/tests/api/test_thread_event_permissions.py`:
- Around line 199-214: The test test_create_cannot_set_timestamps currently only
asserts response.data["created_at"] and therefore misses verifying updated_at;
update the test to also assert that response.data["updated_at"] does not start
with "2000" (mirroring the created_at check) so both timestamps are protected
when posting to get_thread_event_url via api_client and response.
In `@src/backend/core/tests/api/test_thread_user.py`:
- Around line 374-405: The test test_empty_thread_returns_empty_list currently
creates MailboxAccess for the authenticated user so the result is never empty;
remove the lines that create MailboxAccess for the test user (the
factories.MailboxAccessFactory call) and any ThreadAccessFactory that links that
mailbox to the thread (the second factories.ThreadAccessFactory with role
EDITOR), so the thread is only linked to an orphan mailbox with no
MailboxAccess; then authenticate the user (api_client.force_authenticate) who
has no MailboxAccess and call get_thread_user_url(thread.id) and assert
response.data is empty (e.g., returned_ids == set() or len(response.data) == 0)
to actually exercise the empty-result case.
In `@src/e2e/src/__tests__/thread-event.spec.ts`:
- Around line 243-245: The test's wall-clock sleep uses await
page.waitForTimeout(1000) which can race a strict "> 1s" edited threshold;
replace that call with a slightly larger buffer (e.g., await
page.waitForTimeout(1200)) or, preferably, replace the sleep with an assertion
polling the persisted timestamp/DOM until the edited badge appears (check the
element that represents the edited-badge or the persisted message timestamp) to
avoid flaky timing-based failures.
- Around line 39-41: The tests currently call resetDatabase() only in
test.beforeAll, causing shared state across cases (e.g., the "Shared inbox
thread for IM" accumulating a .thread-event--condensed block); change the setup
to re-seed isolation per test by either moving resetDatabase() into
test.beforeEach or by creating a dedicated helper (e.g., createTestThread or
seedIsolatedThread) and invoking it from each test case; update the tests that
reference the shared "Shared inbox thread for IM" to use the per-test thread
instance so assertions no longer depend on prior test side effects.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/_index.scss`:
- Line 28: The animation name slideIn violates keyframes-name-pattern; rename
the keyframes and its usage so they match the project's naming convention (e.g.,
prefix and kebab-case). Update the `@keyframes` declaration currently named
slideIn to a compliant name (for example thread-event-input-slide-in) and change
the animation property that references slideIn (animation: slideIn 0.15s ...) to
use the new name; ensure both the keyframes block and any references in this
stylesheet (and same-named occurrences at line 35) are updated together.
- Around line 61-64: The &:focus-within rule in thread-event-input/_index.scss
sets outline using an undefined token
(--c--components--forms-input--border-color--focus), so keyboard focus can be
invisible because the textarea native outline was removed elsewhere; update the
&:focus-within declaration to provide a reliable fallback (for example: outline:
2px solid var(--c--components--forms-input--border-color--focus, var(--c--focus,
`#2684FF`))); keep the :focus-within selector as the visible focus indicator and
do not remove the textarea's focus indicator without ensuring this container
rule supplies a visible ring.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event-input/index.tsx`:
- Around line 66-75: The mention regex lacks a required token boundary before
'@', so e.g. "foo@bar.com" incorrectly matches; update processContent,
handleInput, and cleanup to share a single matcher that requires a real start
boundary (start of string, whitespace or chosen punctuation) before the '@'.
Implement a shared factory or constant (e.g., getMentionRegex /
MENTION_TOKEN_REGEX) and in processContent use a pattern that preserves the
preceding char (e.g., use a group like '(^|[\\s\\p{P}])@NAME(?![\\p{L}\\p{N}_])'
with the 'u' and 'g' flags) and replace with the captured prefix plus '@[NAME]'
(so you replace with '$1@[NAME]' or via a replacer callback); update handleInput
and cleanup to use the same shared regex to ensure consistent matching and avoid
accidental email matches. Ensure you reference and update the symbols:
processContent, handleInput, cleanup, mentions, and the new shared
getMentionRegex/MENTION_TOKEN_REGEX.
In `@src/frontend/src/features/utils/text-helper/index.tsx`:
- Line 6: The current TRAILING_PUNCTUATION_REGEX strips trailing ')' and breaks
valid URLs (e.g., https://.../Function_(mathematics)); update the trimming logic
so URLs are preserved: either change the regex to avoid removing a trailing ')'
when the string contains a matching '(' earlier (i.e., only strip ')' if it is
not closing a balanced parenthesis) or, simpler, detect URL tokens first (e.g.,
with a URL regex like /\bhttps?:\/\/[^\s]+/), skip trimming for those matches,
and only apply TRAILING_PUNCTUATION_REGEX to non-URL text; update the code that
uses TRAILING_PUNCTUATION_REGEX accordingly so links with closing parentheses
remain intact.
---
Duplicate comments:
In `@src/backend/core/tests/api/test_thread_event_permissions.py`:
- Around line 311-339: The tests
test_url_thread_id_ignores_query_param_thread_id and
test_url_thread_id_ignores_query_param_mailbox_id lack positive controls and may
pass on empty/paginated responses; update each to create/seeding: (1) an allowed
ThreadEvent via factories.ThreadEventFactory(thread=thread_a) (or thread=thread
for mailbox test), and (2) a blocked ThreadEvent on the disallowed
thread/mailbox, then call get_thread_event_url as before and read the list from
response.data["results"] if present (fall back to response.data for
non-paginated shape) and assert the results include the allowed event's id and
do not include the blocked event's id; use the existing helper
get_thread_event_url and factories.ThreadEventFactory to locate where to change
assertions.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss`:
- Around line 39-45: The CSS still uses the deprecated word-break: break-word in
the .thread-event--im .thread-event__content rule (and the similar rule around
lines 142–148); update those rules by replacing word-break: break-word with
overflow-wrap: anywhere and set word-break: normal to preserve intended wrapping
behavior; locate the declarations in the .thread-event--im
.thread-event__content and the other .thread-event__content/related selectors
and apply the two-property replacement consistently.
In `@src/frontend/src/features/providers/mailbox.tsx`:
- Around line 317-332: threadItems currently returns null whenever
messagesQuery.data?.data is undefined which hides a resolved threadEventsQuery;
change the logic in the useMemo so both message and event sources default to
empty arrays (e.g. const messages = messagesQuery.data?.data ?? []; const events
= threadEventsQuery.data?.data ?? []), merge and sort those arrays, and only
return null if there is no selected thread (check your
selectedThread/selectedThreadId variable) instead of returning null based on
messagesQuery alone; update references to TimelineItem, messagesQuery,
threadEventsQuery and threadItems accordingly.
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5456-5493: The threads_events_list GET currently returns a raw
array of ThreadEvent objects; change its OpenAPI response schema at
"/api/v1.0/threads/{thread_id}/events/" (operationId threads_events_list) to use
the project’s standard paginated collection shape (e.g., object with fields like
count/total, next, previous and results which is an array of ThreadEvent)
instead of a top-level array, and add the same pagination query parameters
(page/limit or offset/limit consistent with other endpoints) to the parameters
list so the contract is paginated and matches other collection endpoints.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 24-31: The viewset currently disables pagination by setting
pagination_class = None which can return all ThreadEvents at once; restore
pagination on the ThreadEvent viewset (the class using serializer_class =
serializers.ThreadEventSerializer and permission_classes =
[permissions.IsAuthenticated, permissions.IsAllowedToAccess]) by assigning a
cursor or limit-offset paginator (e.g., a custom CursorPagination with a
sensible page_size and ordering by the event timestamp) instead of None, or add
a specific pagination_class like ThreadEventCursorPagination and wire it into
the viewset so large IM event lists are returned in pages.
In `@src/frontend/src/features/ui/components/suggestion-input/index.tsx`:
- Around line 83-89: The code uses a misleading cast `selectItem(null as T)`
inside the onSelectedItemChange handler (function around
onSelectedItemChange/selectItem/onSelect) which bypasses type safety; instead
remove the assertion and call selectItem(null) directly if Downshift's types
allow null, or update the component/generic/type parameter so the selected item
type is nullable (e.g., use T | null in the component/prop/generic definition)
so selectItem(null) is type-safe; ensure the change touches the selectItem call
in onSelectedItemChange and the component's generic/props where T is declared.
🪄 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: 7c5a0269-58a9-48e5-8e5c-65053607c31d
⛔ Files ignored due to path filters (16)
src/frontend/src/features/api/gen/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/index.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create_metadata_request.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/mailbox_admin_create_metadata_type_enum.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_event.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_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/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 (42)
Makefilesrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/api/viewsets/thread_user.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/management/commands/createsuperuser.pysrc/backend/core/mda/outbound.pysrc/backend/core/migrations/0023_threadevent.pysrc/backend/core/models.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_event_permissions.pysrc/backend/core/tests/api/test_thread_user.pysrc/backend/core/tests/mda/test_autoreply.pysrc/backend/core/urls.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/e2e/src/__tests__/message-import.spec.tssrc/e2e/src/__tests__/thread-event.spec.tssrc/e2e/src/types.tssrc/frontend/Dockerfilesrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/forms/components/combobox/index.tsxsrc/frontend/src/features/layouts/components/thread-view/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/_index.scsssrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/calendar-helper.tsxsrc/frontend/src/features/layouts/components/thread-view/components/calendar-invite/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/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/features/ui/components/suggestion-input/_index.scsssrc/frontend/src/features/ui/components/suggestion-input/index.tsxsrc/frontend/src/features/utils/text-helper/index.test.tsxsrc/frontend/src/features/utils/text-helper/index.tsxsrc/frontend/src/pages/_app.tsxsrc/frontend/src/styles/main.scss
💤 Files with no reviewable changes (2)
- src/backend/core/mda/outbound.py
- src/backend/core/tests/mda/test_autoreply.py
✅ Files skipped from review due to trivial changes (7)
- src/frontend/src/features/forms/components/combobox/index.tsx
- src/e2e/src/types.ts
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/ui/components/suggestion-input/_index.scss
- src/backend/core/migrations/0023_threadevent.py
- src/frontend/src/styles/main.scss
- src/frontend/src/pages/_app.tsx
🚧 Files skipped from review as they are similar to previous changes (12)
- src/frontend/public/locales/common/en-US.json
- Makefile
- src/backend/core/management/commands/createsuperuser.py
- src/backend/core/urls.py
- src/frontend/src/features/layouts/components/thread-view/_index.scss
- src/backend/core/api/permissions.py
- src/backend/core/tests/api/test_thread_event.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx
- src/backend/core/models.py
- src/backend/core/api/serializers.py
- src/backend/core/enums.py
- src/frontend/src/features/layouts/components/thread-view/index.tsx
When a Thread has several accesses or is linked to a shared mailbox, an input allows to post internal messages. It also allows to mention user in a message. The ThreadEvent model is the foundation to enrich Threads with further kind of event. Co-authored-by: Sylvain Zimmer <sylvinus@users.noreply.github.com>
Merged
This was referenced Apr 16, 2026
Merged
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
For now we implement type=im for private messaging among team members of a shared inbox.
Reimplements part of #383
CleanShot.2026-03-19.at.19.05.48.mp4
Summary by CodeRabbit
Release Notes
New Features
Improvements
Localization