feat/mention-event#621
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:
📝 WalkthroughWalkthroughAdds mention tracking with a new UserEvent model, DB migration, sync logic on ThreadEvent saves, mention-read PATCH endpoint, mention-related query annotations/filters/stats, edit-window gating for ThreadEvent edits/deletes, frontend mention UI/visibility batching, tests, and admin/factory updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend
participant API as Backend API
participant DB as Database
participant Signal as PostSaveHandler
Client->>API: GET /api/v1.0/threads/?has_unread_mention=1
API->>DB: Query threads annotated with Exists(UserEvent type=MENTION, read_at IS NULL)
DB-->>API: Annotated rows
API-->>Client: Threads JSON (has_unread_mention, events_count, ...)
Client->>API: PATCH /threads/{thread}/events/{event}/read-mention/
API->>DB: Update UserEvent set read_at=now() for user,event,type=MENTION
DB-->>API: 204
Client->>API: PATCH /threads/{thread}/events/{event}/ (edit IM)
API->>DB: Update ThreadEvent
DB-->>Signal: post_save(ThreadEvent)
Signal->>DB: sync_mention_user_events() (delete/create UserEvent rows)
DB-->>Signal: sync complete
API-->>Client: 200 (updated ThreadEvent)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~55 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jbpenrath
force-pushed
the
feat/mention-event
branch
2 times, most recently
from
303d9bb to
1f1f94f
Compare
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
25-30:⚠️ Potential issue | 🔴 CriticalMissing
mentionRefprop in type definition.The
mentionRefprop is used in the component (line 60) and referenced at line 181, but is not declared inThreadEventProps. This will cause a TypeScript compilation error since the destructuring includes a property not defined in the type.Proposed fix
type ThreadEventProps = { event: ThreadEventType; previousEvent?: ThreadEventType | null; onEdit?: (event: ThreadEventType) => void; onDelete?: (eventId: string) => void; + mentionRef?: React.RefCallback<HTMLSpanElement> | React.RefObject<HTMLSpanElement> | null; };🤖 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 25 - 30, Add the missing optional mentionRef prop to the ThreadEventProps type so TypeScript knows the component accepts it; declare it as something like mentionRef?: React.RefObject<HTMLElement> (or React.Ref<HTMLDivElement>) in the ThreadEventProps definition used by the ThreadEvent component, and ensure React is imported if not already so the type resolves.src/backend/core/api/viewsets/thread.py (1)
320-390:⚠️ Potential issue | 🟠 Major
has_*mention_unreadstats will raiseFieldError.The generic
_unreadbranch buildsQ(has_unread_mention=True)/Q(has_mention=True), but those names only exist as annotations with a leading underscore. Requests forhas_unread_mention_unreadorhas_mention_unreadtherefore hit nonexistent model fields.Suggested fix
elif field == "has_starred_unread": aggregations[agg_key] = Count( "pk", filter=starred_condition & unread_condition ) + elif field == "has_unread_mention_unread": + aggregations[agg_key] = Count( + "pk", filter=unread_mention_condition & unread_condition + ) + elif field == "has_mention_unread": + aggregations[agg_key] = Count( + "pk", filter=mention_condition & unread_condition + ) elif field == "has_unread_mention": aggregations[agg_key] = Count("pk", filter=unread_mention_condition) elif field == "has_mention": aggregations[agg_key] = Count("pk", filter=mention_condition)🤖 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 320 - 390, The issue is that annotated flags are named with a leading underscore (e.g. _has_starred, _has_unread_mention) but the code builds Q(...) using un-prefixed names when handling field.endswith("_unread") and the final else, causing FieldError for requests like has_mention_unread; fix by mapping base_field/field to the annotated name (prefix with "_" before building Q) in the "_unread" branch (where base_field = field[:-7]) and in the final else branch where Q(**{field: True}) is created—use Q(**{f"_{base_field}": True}) and Q(**{f"_{field}": True}) respectively, referencing the requested_fields loop, base_field variable, aggregations dict, and the unread_condition/unread_mention_condition variables to locate the change.src/backend/core/api/openapi.json (1)
5909-5925:⚠️ Potential issue | 🟠 Major
stats_fieldsis still typed as a single enum value, not the comma-separated list the docs describe.Line 5912 defines one enum-backed string, but Line 5922 documents combinations like
all,all_unreadand generic_unreadsuffixes. Generated clients will reject those documented values unless this is modeled as an array or the description/examples are narrowed to a single accepted value.🤖 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 5909 - 5925, The stats_fields parameter is defined as a single enum string but the docs describe comma-separated multiple values; update the OpenAPI schema for stats_fields to be an array of strings with the same enum items (and keep style: "form" and explode: false so clients accept comma-separated lists) so clients can pass values like "all,all_unread" or combinations with "_unread"; modify the schema object under the stats_fields parameter (replace type: "string" + enum with type: "array" + items: { type: "string", enum: [...] }) while preserving the description, required, style, and explode settings.
🧹 Nitpick comments (8)
src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx (1)
58-62: Remove unusedselectedMailboxfrom useMemo dependency array.The
selectedMailboxvariable is included in the dependency array but is not used in the calculation. This could cause unnecessary re-computations.♻️ Proposed fix
const title = useMemo(() => { if (searchParams.has('search')) return t('folder.search', { defaultValue: 'Search' }); if (searchParams.has('label_slug')) return (labelsQuery.data?.data || []).find((label) => label.slug === searchParams.get('label_slug'))?.name; return findFolder((folder) => new URLSearchParams(folder.filter).toString() === folderSearchParams.toString())?.name; - }, [searchParams, folderSearchParams, labelsQuery.data?.data, selectedMailbox, t]) + }, [searchParams, folderSearchParams, labelsQuery.data?.data, t])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx` around lines 58 - 62, The useMemo that computes the "title" (const title = useMemo(...)) includes selectedMailbox in its dependency array but selectedMailbox is not referenced inside the memo callback; remove selectedMailbox from the dependency list so the memo only depends on searchParams, folderSearchParams, labelsQuery.data?.data and t to avoid unnecessary recomputations.src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx (2)
167-180:findFolderonly supports one level of nesting.The current implementation searches top-level folders and their direct children, but won't find folders nested deeper. This is fine for the current structure, but if deeper nesting is added in the future, a recursive implementation would be needed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx` around lines 167 - 180, The findFolder function currently only checks top-level folders and their immediate children, so it misses folders nested deeper; update findFolder to perform a recursive search over MAILBOX_FOLDERS() (and each folder.children array) so it descends any depth until predicate(folder) is true, referencing the existing function name findFolder, the MAILBOX_FOLDERS() getter, the Folder.children property, and the predicate parameter to guide where to check and when to return the matching Folder or undefined.
151-151: Removingsatisfiesreduces compile-time type safety.Changing from
as const satisfies readonly Folder[]to justas constmeans TypeScript won't validate that each folder object conforms to theFoldertype at the definition site. This relies on the runtime casts at lines 172 and 196 to catch type mismatches.Consider restoring the
satisfiesclause or using a typed function to maintain type safety:♻️ Suggested fix
-] as const; +] as const satisfies readonly Folder[];This ensures the folder objects match the
Foldertype while preserving literal type inference for properties likeid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx` at line 151, The array of folder objects currently ends with "as const" which removes compile-time validation against the Folder type; restore the original type-safety by appending "satisfies readonly Folder[]" to that array definition (or wrap the array in a typed helper function that returns readonly Folder[]) so each entry is validated as a Folder while keeping literal inference for properties like id; also keep the runtime casts currently used elsewhere (the places casting to Folder) but they should become unnecessary once the array satisfies readonly Folder[].src/backend/core/tests/api/test_user_event.py (1)
34-144: Tests cover core scenarios well; consider adding edge cases.The test coverage for the mark-read endpoint is solid. Consider adding tests for:
- Empty
thread_event_idsarray (should returnupdated=0or validation error?)is_active=FalseUserEvents (should they be excluded from bulk update?)These would improve confidence in the endpoint's robustness.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_user_event.py` around lines 34 - 144, Add tests in TestUserEventMarkRead to cover the missing edge cases: create a test method (e.g., test_mark_read_empty_ids) that sends an authenticated PATCH to reverse("user-events-mark-read") with an empty "thread_event_ids": [] and assert response.status_code is 200 and response.data["updated"] == 0 (or adjust if validation expected); and create a test (e.g., test_mark_read_excludes_inactive_userevents) that creates a UserEvent with is_active=False for the authenticated user and a matching ThreadEvent, PATCH with that thread_event id and assert updated==0 and the inactive UserEvent.read_at remains None; use existing helpers like setup_user_with_thread_access(), factories.UserEventFactory, and reverse("user-events-mark-read") to implement both tests.src/backend/core/models.py (1)
1437-1492: Consider adding a unique constraint to prevent duplicateUserEvententries.There's no constraint preventing multiple
UserEventrows for the same(user, thread_event, type)combination. If the signal handler that creates these events runs multiple times (e.g., due to retries or race conditions), duplicates could be created.🛡️ Suggested constraint addition
class Meta: db_table = "messages_userevent" verbose_name = "user event" verbose_name_plural = "user events" ordering = ["-created_at"] + constraints = [ + models.UniqueConstraint( + fields=["user", "thread_event", "type"], + name="unique_user_thread_event_type", + ), + ] indexes = [🤖 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 1437 - 1492, Add a uniqueness constraint to the UserEvent model to prevent duplicate (user, thread_event, type) entries: update the UserEvent.Meta to include a models.UniqueConstraint(fields=["user", "thread_event", "type"], name="usrevt_user_threvt_type_uniq") and run a migration; before applying the constraint create a data migration or cleanup step to deduplicate existing rows for (user, thread_event, type) to avoid migration failures. Ensure you reference the UserEvent class and its Meta when making the change.src/backend/core/tests/api/test_thread_event.py (1)
390-400: Consider adding assertion forUserEventcreation on self-assign.The test verifies 201 status but doesn't confirm that a
UserEventwas actually created for the self-assigning user. This would strengthen the test.💡 Suggested enhancement
response = api_client.post(get_thread_event_url(thread.id), data, format="json") assert response.status_code == status.HTTP_201_CREATED + + # Verify UserEvent ASSIGN was created for self + assert models.UserEvent.objects.filter( + user=user, + thread=thread, + type=enums.UserEventTypeChoices.ASSIGN, + is_active=True, + ).exists()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_thread_event.py` around lines 390 - 400, Add an assertion that a UserEvent was created for the self-assigned user: after calling api_client.post in test_create_assign_self_assign, query the UserEvent model (UserEvent.objects.filter or equivalent) for an event tied to the thread and the acting user and assert exactly one (or at least one) exists and/or its type/data matches "assign" and contains the assignee id; reference the test function test_create_assign_self_assign, the get_thread_event_url call, and the UserEvent model to locate where to add this check.src/backend/core/tests/api/test_thread_filter_mention.py (1)
14-29: Consider extracting shared helper toconftest.py.The
setup_user_with_thread_accesshelper function is duplicated in bothtest_thread_event.py(line 24) and this file. Extracting it to a shared location (e.g.,conftest.pyor a test utilities module) would reduce duplication and ensure consistency.♻️ Suggested refactor
Create or update
src/backend/core/tests/api/conftest.py:import pytest from core import enums, factories `@pytest.fixture` def user_with_thread_access(): """Fixture factory for creating a user with mailbox and thread access.""" def _create(role=enums.ThreadAccessRoleChoices.EDITOR): user = factories.UserFactory() mailbox = factories.MailboxFactory() factories.MailboxAccessFactory( mailbox=mailbox, user=user, role=enums.MailboxRoleChoices.ADMIN, ) thread = factories.ThreadFactory() factories.ThreadAccessFactory( mailbox=mailbox, thread=thread, role=role, ) return user, mailbox, thread return _createThen in tests:
def test_example(self, api_client, user_with_thread_access): user, mailbox, thread = user_with_thread_access()🤖 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_filter_mention.py` around lines 14 - 29, The helper function setup_user_with_thread_access is duplicated across tests; extract it into a shared pytest fixture (e.g., user_with_thread_access) in the tests package conftest.py so other test modules can reuse it: move the body of setup_user_with_thread_access into a fixture that returns a callable (def _create(role=...): ... return user, mailbox, thread) and update tests that currently call setup_user_with_thread_access to accept the user_with_thread_access fixture and invoke it; finally remove the duplicate local setup_user_with_thread_access definitions from the test files (references: setup_user_with_thread_access, ThreadAccessFactory, MailboxAccessFactory).src/backend/core/migrations/0024_userevent.py (1)
30-31: Consider logging the backfill progress or removing unusedtotal_created.The
total_createdcounter is incremented but never logged or returned. For long-running migrations with large datasets, progress logging can be helpful for monitoring and debugging.💡 Suggested enhancement
+import logging + +logger = logging.getLogger(__name__) + def backfill_mention_user_events(apps, schema_editor): ... if batch: UserEvent.objects.bulk_create(batch) total_created += len(batch) + + if total_created: + logger.info("Backfilled %d UserEvent MENTION records", total_created)Also applies to: 83-83, 88-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/migrations/0024_userevent.py` around lines 30 - 31, The migration defines and increments a total_created counter (variable total_created) but never logs or returns it; either remove the unused total_created and any related increments to avoid dead code, or add progress logging that emits total_created periodically and at completion (for example inside the processing loop that uses batch) using the existing logger/print so long-running backfills report progress; update references to total_created in the migration function (where batch is processed) accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/backend/core/admin.py`:
- Around line 435-441: UserEventInline allows editing of user and thread_event
which can desync user_event.thread vs user_event.thread_event.thread; fix by
making the inline effectively read-only or constraining thread_event choices:
update UserEventInline to include "user" and "thread_event" in readonly_fields
(or override get_readonly_fields to add them conditionally), and/or override
formfield_for_foreignkey in UserEventInline to limit the thread_event queryset
to only events whose thread matches the parent thread (filter by
thread_event__thread == parent instance) so admins cannot pick a thread_event
from a different thread.
In `@src/backend/core/api/openapi.json`:
- Around line 5975-6015: The requestBody for the mark-read operation currently
allows omitted bodies and the 200 response schema allows a missing updated
count; make the request body required by adding "required": true on the
requestBody object and make the response object require the "updated" property
by adding "required": ["updated"] to the 200 response schema; target the
requestBody that contains the "thread_event_ids" property and the response
schema that defines the "updated" integer to ensure generated SDK types mark the
body and updated count as required.
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 34-43: get_permissions is returning
permissions.HasThreadEditAccess() for write actions but that permission's
has_object_permission expects a Thread (not a ThreadEvent), so DRF will call it
with a ThreadEvent and either deny valid editors or raise; fix by either
updating HasThreadEditAccess.has_object_permission to accept a ThreadEvent
(detect when obj is a ThreadEvent and use obj.thread) or create an adapter
permission (e.g., HasThreadEditAccessForThreadEvent) that delegates to
HasThreadEditAccess using obj.thread, then change get_permissions to return that
adapter for update/partial_update/destroy instead of HasThreadEditAccess
directly.
In `@src/backend/core/api/viewsets/user_event.py`:
- Around line 57-65: thread_event_ids is taken straight from request.data and
passed to thread_event_id__in, which will error or behave incorrectly if
thread_event_ids is None or not a list (e.g., a string); validate and coerce it
first. In the view method in src/backend/core/api/viewsets/user_event.py where
thread_event_ids is read and used in
models.UserEvent.objects.filter(...).update(...), require thread_event_ids to be
a list of UUIDs (use a DRF serializer/ListField child=UUIDField or explicitly
check isinstance(thread_event_ids, list) and validate each item as a UUID),
return a 400 Response on invalid input, and only call thread_event_id__in with
the validated list.
In `@src/backend/core/signals.py`:
- Around line 384-416: The handler handle_thread_event_post_save currently
returns early for non-created ThreadEvent instances which prevents resyncing
mentions on edits; update it to always process IM-type events (not just when
created) by removing the early return and invoking the mention synchronization
logic for updates as well—either call/create a function like
create_mention_user_events with a flag for created vs updated or add a new
sync_mention_user_events(thread_event, thread, mentions_data) function that
diffs current mentions and creates/deletes UserEvent rows accordingly; ensure
this logic is triggered inside the existing try/except for instance.type ==
enums.ThreadEventTypeChoices.IM so edits correctly add/remove UserEvent MENTION
records and keep mention/unread state in sync.
In `@src/backend/core/tests/models/test_user_event.py`:
- Around line 26-29: The test test_invalid_type_raises_validation_error
currently expects a ValidationError from saving via
factories.UserEventFactory(type="invalid"), but Django choice validation runs on
full_clean(), not save(); update the test to create an unsaved instance with
factories.UserEventFactory.build(type="invalid") (or create then reload) and
then call instance.full_clean() inside the pytest.raises(ValidationError) block,
or alternatively modify factories.UserEventFactory._create (or the model
UserEvent.save()) to call full_clean() before saving so that creating via
UserEventFactory triggers validation.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 68-71: The current debouncedMarkMentionRead call replaces the ids
array each invocation so overlapping observer callbacks lose earlier IDs; change
to accumulate IDs into a shared Set held in a React ref/closure (e.g.,
pendingMentionIdsRef = useRef<Set<string>>(new Set())), push incoming ids into
that Set inside the observer callback instead of passing them directly to
useDebounceCallback, and have the debounced handler (the function passed into
useDebounceCallback) read Array.from(pendingMentionIdsRef.current), call
markMentionsRead(with that array), then clear the Set; apply the same pattern to
the other similar debounced usage mentioned (lines 139-151) so all concurrent
callbacks are merged before the debounce fires.
In `@src/frontend/src/features/message/use-mention-read.tsx`:
- Around line 25-27: The markMentionsRead function currently performs cache
operations using selectedMailbox?.id which can be undefined; add a guard that
returns early if selectedMailbox or selectedMailbox.id is falsy before any
query/cache updates (e.g., before calls that use the ['threads','stats',
selectedMailbox?.id] key), so all cache reads/writes (in markMentionsRead and
any helper it calls) only run when a valid selectedMailbox.id exists.
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5909-5925: The stats_fields parameter is defined as a single enum
string but the docs describe comma-separated multiple values; update the OpenAPI
schema for stats_fields to be an array of strings with the same enum items (and
keep style: "form" and explode: false so clients accept comma-separated lists)
so clients can pass values like "all,all_unread" or combinations with "_unread";
modify the schema object under the stats_fields parameter (replace type:
"string" + enum with type: "array" + items: { type: "string", enum: [...] })
while preserving the description, required, style, and explode settings.
In `@src/backend/core/api/viewsets/thread.py`:
- Around line 320-390: The issue is that annotated flags are named with a
leading underscore (e.g. _has_starred, _has_unread_mention) but the code builds
Q(...) using un-prefixed names when handling field.endswith("_unread") and the
final else, causing FieldError for requests like has_mention_unread; fix by
mapping base_field/field to the annotated name (prefix with "_" before building
Q) in the "_unread" branch (where base_field = field[:-7]) and in the final else
branch where Q(**{field: True}) is created—use Q(**{f"_{base_field}": True}) and
Q(**{f"_{field}": True}) respectively, referencing the requested_fields loop,
base_field variable, aggregations dict, and the
unread_condition/unread_mention_condition variables to locate the change.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 25-30: Add the missing optional mentionRef prop to the
ThreadEventProps type so TypeScript knows the component accepts it; declare it
as something like mentionRef?: React.RefObject<HTMLElement> (or
React.Ref<HTMLDivElement>) in the ThreadEventProps definition used by the
ThreadEvent component, and ensure React is imported if not already so the type
resolves.
---
Nitpick comments:
In `@src/backend/core/migrations/0024_userevent.py`:
- Around line 30-31: The migration defines and increments a total_created
counter (variable total_created) but never logs or returns it; either remove the
unused total_created and any related increments to avoid dead code, or add
progress logging that emits total_created periodically and at completion (for
example inside the processing loop that uses batch) using the existing
logger/print so long-running backfills report progress; update references to
total_created in the migration function (where batch is processed) accordingly.
In `@src/backend/core/models.py`:
- Around line 1437-1492: Add a uniqueness constraint to the UserEvent model to
prevent duplicate (user, thread_event, type) entries: update the UserEvent.Meta
to include a models.UniqueConstraint(fields=["user", "thread_event", "type"],
name="usrevt_user_threvt_type_uniq") and run a migration; before applying the
constraint create a data migration or cleanup step to deduplicate existing rows
for (user, thread_event, type) to avoid migration failures. Ensure you reference
the UserEvent class and its Meta when making the change.
In `@src/backend/core/tests/api/test_thread_event.py`:
- Around line 390-400: Add an assertion that a UserEvent was created for the
self-assigned user: after calling api_client.post in
test_create_assign_self_assign, query the UserEvent model
(UserEvent.objects.filter or equivalent) for an event tied to the thread and the
acting user and assert exactly one (or at least one) exists and/or its type/data
matches "assign" and contains the assignee id; reference the test function
test_create_assign_self_assign, the get_thread_event_url call, and the UserEvent
model to locate where to add this check.
In `@src/backend/core/tests/api/test_thread_filter_mention.py`:
- Around line 14-29: The helper function setup_user_with_thread_access is
duplicated across tests; extract it into a shared pytest fixture (e.g.,
user_with_thread_access) in the tests package conftest.py so other test modules
can reuse it: move the body of setup_user_with_thread_access into a fixture that
returns a callable (def _create(role=...): ... return user, mailbox, thread) and
update tests that currently call setup_user_with_thread_access to accept the
user_with_thread_access fixture and invoke it; finally remove the duplicate
local setup_user_with_thread_access definitions from the test files (references:
setup_user_with_thread_access, ThreadAccessFactory, MailboxAccessFactory).
In `@src/backend/core/tests/api/test_user_event.py`:
- Around line 34-144: Add tests in TestUserEventMarkRead to cover the missing
edge cases: create a test method (e.g., test_mark_read_empty_ids) that sends an
authenticated PATCH to reverse("user-events-mark-read") with an empty
"thread_event_ids": [] and assert response.status_code is 200 and
response.data["updated"] == 0 (or adjust if validation expected); and create a
test (e.g., test_mark_read_excludes_inactive_userevents) that creates a
UserEvent with is_active=False for the authenticated user and a matching
ThreadEvent, PATCH with that thread_event id and assert updated==0 and the
inactive UserEvent.read_at remains None; use existing helpers like
setup_user_with_thread_access(), factories.UserEventFactory, and
reverse("user-events-mark-read") to implement both tests.
In
`@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx`:
- Around line 167-180: The findFolder function currently only checks top-level
folders and their immediate children, so it misses folders nested deeper; update
findFolder to perform a recursive search over MAILBOX_FOLDERS() (and each
folder.children array) so it descends any depth until predicate(folder) is true,
referencing the existing function name findFolder, the MAILBOX_FOLDERS() getter,
the Folder.children property, and the predicate parameter to guide where to
check and when to return the matching Folder or undefined.
- Line 151: The array of folder objects currently ends with "as const" which
removes compile-time validation against the Folder type; restore the original
type-safety by appending "satisfies readonly Folder[]" to that array definition
(or wrap the array in a typed helper function that returns readonly Folder[]) so
each entry is validated as a Folder while keeping literal inference for
properties like id; also keep the runtime casts currently used elsewhere (the
places casting to Folder) but they should become unnecessary once the array
satisfies readonly Folder[].
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Around line 58-62: The useMemo that computes the "title" (const title =
useMemo(...)) includes selectedMailbox in its dependency array but
selectedMailbox is not referenced inside the memo callback; remove
selectedMailbox from the dependency list so the memo only depends on
searchParams, folderSearchParams, labelsQuery.data?.data and t to avoid
unnecessary recomputations.
🪄 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: a474171c-3250-456a-8eb1-f9f0ebee26e0
⛔ Files ignored due to path filters (10)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/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/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_stats_fields.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/user_events_mark_read_partial_update200.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/user_events_mark_read_partial_update_body.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/user-events/user-events.tsis excluded by!**/gen/**
📒 Files selected for processing (27)
src/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/api/viewsets/user_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/migrations/0024_userevent.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_filter_mention.pysrc/backend/core/tests/api/test_user_event.pysrc/backend/core/tests/models/test_user_event.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-labels/components/label-item/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-mention-read.tsx
jbpenrath
force-pushed
the
feat/mention-event
branch
3 times, most recently
from
0f84ee8 to
e88def3
Compare
There was a problem hiding this comment.
Actionable comments posted: 11
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 (1)
5950-5965:⚠️ Potential issue | 🟠 Major
stats_fieldsenum is incomplete and omits many valid fields documented and supported by the backend.The OpenAPI schema declares only 6 enum values, but the backend's
stats()method validates against 13 boolean fields (has_trashed,has_archived,has_draft,has_starred,has_attachments,has_sender,has_active,has_delivery_failed,has_delivery_pending,has_unread_mention,has_mention,is_spam,has_messages) plus 2 special fields (all,all_unread), and supports all of them with_unreadsuffixes. Tests confirm comma-separated combinations like"has_messages,has_trashed,has_draft,has_starred,has_sender"and"has_starred,has_starred_unread"work. Generated clients using this schema will incorrectly reject many valid requests. The root cause isTHREAD_STATS_FIELDS_MAPinenums.pycontaining only 6 entries instead of all valid base fields—it should be expanded to match thevalid_base_fieldsset in thestats()method.🤖 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 5950 - 5965, The OpenAPI enum for stats_fields is missing many valid values; update THREAD_STATS_FIELDS_MAP in enums.py to include all base boolean fields used by stats() (has_trashed, has_archived, has_draft, has_starred, has_attachments, has_sender, has_active, has_delivery_pending, has_delivery_failed, is_spam, has_messages, has_unread_mention, has_mention) and ensure the map and generation emit both base and *_unread variants (plus the special values "all" and "all_unread") so the OpenAPI enum for stats_fields matches the valid_base_fields set used in stats(); regenerate the OpenAPI schema (openapi.json) so clients accept the same comma-separated combinations the backend validates.src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx (1)
279-289:⚠️ Potential issue | 🔴 CriticalGate the query on
!!selectedMailboxand use optional chaining in the key.
enabledonly prevents the fetch; it does not stopgetThreadsStatsQueryKey(selectedMailbox!.id, ...)from being evaluated during render. If the sidebar mounts before mailbox selection resolves, this crashes before React Query can short-circuit. UseselectedMailbox?.idin the key and add!!selectedMailboxto the enabled condition.This same issue also affects
src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsx.Minimal fix
query: { - enabled: folder.showStats, - queryKey: getThreadsStatsQueryKey(selectedMailbox!.id, queryParams), + enabled: folder.showStats && !!selectedMailbox, + queryKey: getThreadsStatsQueryKey(selectedMailbox?.id, queryParams), }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx` around lines 279 - 289, The query key and enabled flag for useThreadsStatsRetrieve currently dereference selectedMailbox (via getThreadsStatsQueryKey(selectedMailbox!.id, queryParams)) which can throw before React Query checks enabled; update the options to use optional chaining in the key (getThreadsStatsQueryKey(selectedMailbox?.id, queryParams)) and add a truthy guard to enabled (enabled: !!selectedMailbox && folder.showStats) so the key isn't evaluated when selectedMailbox is undefined; apply the same change pattern in the analogous label-item usage.
♻️ Duplicate comments (1)
src/backend/core/api/viewsets/thread_event.py (1)
38-48:⚠️ Potential issue | 🔴 CriticalMake sure
HasThreadEditAccessisThreadEvent-aware here.Line 48 also covers
update/partial_update/destroy, where DRF passes the fetchedThreadEventtohas_object_permission(). If that permission still expects aThread, detail writes will fail for valid editors. AThreadEvent-aware adapter permission is safer here.#!/bin/bash set -euo pipefail python - <<'PY' from pathlib import Path for path in Path("src/backend").rglob("permissions.py"): text = path.read_text(encoding="utf-8") start = text.find("class HasThreadEditAccess") if start == -1: continue end = text.find("\nclass ", start + 1) print(f"== {path} ==") print(text[start:end if end != -1 else None]) PYExpect
HasThreadEditAccess.has_object_permission()either to accept aThreadEventdirectly or to delegate toobj.thread.🤖 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 38 - 48, The current get_permissions uses HasThreadEditAccess for object-level write actions but HasThreadEditAccess.has_object_permission likely expects a Thread, causing detail writes to fail when DRF passes a ThreadEvent; update HasThreadEditAccess (the class and its has_object_permission method) to handle ThreadEvent objects by checking if obj is a ThreadEvent and using obj.thread (or otherwise accepting a ThreadEvent directly) so object permission checks for update/partial_update/destroy succeed for valid editors while leaving get_permissions (in ThreadEvent viewset) unchanged.
🤖 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 6853-6856: The OpenAPI schema erroneously declares
count_unread_mentions as "type: string"; change both occurrences (the field at
the shown diff and the second block around lines 6922-6927) to a numeric type
(preferably "type: integer" with "format: int32") so generated clients receive a
numeric counter; this corresponds to the value computed in serializers.py (the
logic around count_unread_mentions using .count() in the serializer at
src/backend/core/api/serializers.py), so update the openapi.json entries for
count_unread_mentions to integer (and include format if desired).
- Around line 5766-5805: The OpenAPI operation
threads_events_read_mention_partial_update (path
"/api/v1.0/threads/{thread_id}/events/{id}/read-mention/") documents that a
missing ThreadEvent yields 404 but the responses object only lists 204; update
the OpenAPI spec by adding a 404 response entry under "responses" (e.g., "404":
{ "description": "ThreadEvent not found" } – optionally include an error
schema/ref used elsewhere) so the contract matches the description.
In `@src/backend/core/migrations/0024_alter_threadevent_type_userevent.py`:
- Around line 102-120: The migration creating the UserEvent model
(migrations.CreateModel for UserEvent) lacks a database-level uniqueness
constraint for the logical key (thread_event, user, type); add a
UniqueConstraint to the model options (Meta) for fields ['thread_event', 'user',
'type'] with a clear name (e.g., 'usrevt_event_user_type_uniq') so the database
enforces at-most-one UserEvent per (thread_event, user, type) and prevents
concurrent duplicate inserts during backfill/sync.
- Around line 34-57: The migration loop assumes event.data is a dict and
mentions is a list of dicts with a valid UUID "id"; normalize and validate
before using them: ensure event.data is a mapping and get mentions as a list,
then iterate mentions only if each item is a dict and has an "id" that parses to
a UUID (or is a non-empty string), collecting only those valid ids into
unique_user_ids and skipping malformed entries; then perform the MailboxAccess
query using that validated set (referencing the for event in queryset.iterator
loop, the mentions variable, seen_user_ids/unique_user_ids, and
MailboxAccess.objects.filter) so malformed historical payloads are ignored and
cannot abort the migration.
In `@src/backend/core/models.py`:
- Around line 1483-1497: Add a DB-level UniqueConstraint to the UserEvent model
Meta to enforce uniqueness of the logical mention tuple (user, thread, type) —
e.g., add models.UniqueConstraint(fields=["user", "thread", "type"],
name="usrevt_user_thread_type_uniq") inside the Meta.constraints for the
UserEvent class — and then update the sync/bulk_create path that creates
UserEvent rows to tolerate conflicts (e.g., use bulk_create with conflict/ignore
behavior or a DB upsert strategy) so concurrent syncs do not produce duplicate
rows.
In `@src/backend/core/tests/api/test_thread_filter_mention.py`:
- Around line 35-121: Add negative-case assertions to the tests so mentions
belonging to other users are ignored: create a second user (e.g., other_user)
and a UserEventFactory with user=other_user tied to one of the threads, then
assert that when calling threads-list with has_unread_mention and threads-stats
with stats_fields=has_unread_mention the other_user's mention is not returned or
counted; update test_filter_returns_threads_with_unread_mention,
test_filter_returns_empty_when_no_unread_mentions (add a thread with a mention
for other_user and assert it’s ignored), and
TestThreadStatsUnreadMention.test_stats_returns_correct_count accordingly, and
add an assertion that ThreadEvent.has_unread_mention (or the relevant model
property/method) does not consider the other_user’s mention when evaluated for
the authenticated user.
In
`@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx`:
- Around line 188-206: The current code reads/writes localStorage inside the
useState lazy initializer and inside toggleFolder, which breaks SSR/hydration;
change expandedFolders initialization to a safe in-memory default (e.g. { inbox:
true }) and remove localStorage access from the lazy initializer, then add a
useEffect that runs on the client to load and apply persisted state from
EXPANDED_FOLDERS_KEY (using typeof window !== 'undefined' or similar guard) via
setExpandedFolders; also update toggleFolder to guard writes to localStorage
(only call localStorage.setItem when window is defined) so reads/writes happen
only inside client-only code paths (useEffect and guarded toggleFolder) while
keeping the expandedFolders, setExpandedFolders, and toggleFolder signatures
unchanged.
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsx`:
- Around line 14-25: The persisted filters returned by getStoredSelectedFilters
can include stale/invalid keys and must be validated against the allowed set
THREAD_PANEL_FILTER_PARAMS before being reused; update getStoredSelectedFilters
to parse localStorage (THREAD_SELECTED_FILTERS_KEY), ensure the parsed value is
an array of strings, filter that array to only include entries that exist in
THREAD_PANEL_FILTER_PARAMS (the valid param keys), and return that filtered
array, falling back to DEFAULT_SELECTED_FILTERS when parsing fails or no valid
entries remain so we don't render undefined labels or pass bogus params to
applyFilters().
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 107-118: pendingMentionIdsRef currently only deduplicates within
each 150ms debounce window so the same data-event-id can be re-queued later and
cause duplicate optimistic PATCHes; change the logic to maintain a persistent
seen/inFlight set (e.g., seenMentionIds or inFlightMentionIds) alongside
pendingMentionIdsRef, add IDs to that persistent set when queued (in the
intersection observer callback), ensure flushPendingMentions and
debouncedFlushMentions read from pendingMentionIdsRef but do not remove IDs from
the persistent set until the markMentionsRead PATCH succeeds, and on PATCH
failure remove the batch IDs from the persistent set so they can be retried;
apply the same persistent-set guard to the other occurrence of this pattern (the
second pending-mention block).
In `@src/frontend/src/features/message/use-mention-read.tsx`:
- Around line 40-57: The optimistic stats update is currently using
prefix-matching query helpers and mutates all matching keys; change
queryClient.getQueriesData and queryClient.setQueriesData to the exact-match
helpers queryClient.getQueryData and queryClient.setQueryData, using the
existing statsQueryKey variable so only the base stats entry is read/updated
(adjust previousStats to use getQueryData and keep the same safe check and
Math.max decrement logic in the setQueryData updater).
In `@src/frontend/src/features/providers/mailbox.tsx`:
- Around line 112-139: The current implementation collapses all folder/filter
variants into the same search cache key causing stale reuse; update
getMailboxThreadsListQueryKey to include the other searchParams normalized but
only replace the search value with a normalized token (e.g. keep
params.toString() but with search param set to a fixed marker) instead of
returning just ['...','search'], and update getMailboxThreadsListSearchQueryKey
to mirror that normalization by returning
[...getMailboxThreadsListQueryKeyPrefix(mailboxId),
normalizedParamsWithSearchMarker] so the cleanup/reset targets the exact same
cache entry; use getMailboxThreadsListQueryKeyPrefix and the existing
ReadonlySearchParamsLike accessor methods to build the normalized param string.
---
Outside diff comments:
In `@src/backend/core/api/openapi.json`:
- Around line 5950-5965: The OpenAPI enum for stats_fields is missing many valid
values; update THREAD_STATS_FIELDS_MAP in enums.py to include all base boolean
fields used by stats() (has_trashed, has_archived, has_draft, has_starred,
has_attachments, has_sender, has_active, has_delivery_pending,
has_delivery_failed, is_spam, has_messages, has_unread_mention, has_mention) and
ensure the map and generation emit both base and *_unread variants (plus the
special values "all" and "all_unread") so the OpenAPI enum for stats_fields
matches the valid_base_fields set used in stats(); regenerate the OpenAPI schema
(openapi.json) so clients accept the same comma-separated combinations the
backend validates.
In
`@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx`:
- Around line 279-289: The query key and enabled flag for
useThreadsStatsRetrieve currently dereference selectedMailbox (via
getThreadsStatsQueryKey(selectedMailbox!.id, queryParams)) which can throw
before React Query checks enabled; update the options to use optional chaining
in the key (getThreadsStatsQueryKey(selectedMailbox?.id, queryParams)) and add a
truthy guard to enabled (enabled: !!selectedMailbox && folder.showStats) so the
key isn't evaluated when selectedMailbox is undefined; apply the same change
pattern in the analogous label-item usage.
---
Duplicate comments:
In `@src/backend/core/api/viewsets/thread_event.py`:
- Around line 38-48: The current get_permissions uses HasThreadEditAccess for
object-level write actions but HasThreadEditAccess.has_object_permission likely
expects a Thread, causing detail writes to fail when DRF passes a ThreadEvent;
update HasThreadEditAccess (the class and its has_object_permission method) to
handle ThreadEvent objects by checking if obj is a ThreadEvent and using
obj.thread (or otherwise accepting a ThreadEvent directly) so object permission
checks for update/partial_update/destroy succeed for valid editors while leaving
get_permissions (in ThreadEvent viewset) unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4a6e940d-2970-4b67-9eeb-05d0481c5504
⛔ Files ignored due to path filters (8)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_stats_fields.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (39)
docs/env.mdsrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/migrations/0024_alter_threadevent_type_userevent.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_filter_mention.pysrc/backend/core/tests/models/test_user_event.pysrc/backend/messages/settings.pysrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/config/constants.tssrc/frontend/src/features/forms/components/search-filters-form/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/hooks/use-thread-panel-filters.tssrc/frontend/src/features/layouts/components/thread-panel/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/components/thread-summary/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-mention-read.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/features/utils/view-helper/index.tssrc/frontend/src/hooks/use-visibility-observer.ts
✅ Files skipped from review due to trivial changes (4)
- docs/env.md
- src/frontend/src/features/config/constants.ts
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss
🚧 Files skipped from review as they are similar to previous changes (12)
- src/frontend/public/locales/common/en-US.json
- src/backend/core/factories.py
- src/backend/core/api/permissions.py
- src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scss
- src/backend/core/admin.py
- src/backend/core/tests/models/test_user_event.py
- src/backend/core/signals.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx
- src/backend/core/api/viewsets/thread.py
- src/backend/core/api/serializers.py
sylvinus
reviewed
Apr 8, 2026
sylvinus
reviewed
Apr 8, 2026
sylvinus
reviewed
Apr 8, 2026
jbpenrath
force-pushed
the
feat/mention-event
branch
3 times, most recently
from
202c798 to
b03e883
Compare
There was a problem hiding this comment.
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/viewsets/thread.py (2)
580-602:⚠️ Potential issue | 🟠 MajorKeep the OpenSearch branch aligned with the normal list queryset.
get_queryset()now annotatesevents_count, but the OpenSearch fallback branch does not. That means/threads/can return different serialized data depending on whether search is enabled, even for the same threads.Please add the same
Count("events", distinct=True)annotation here so both list paths expose the same fields.💡 Suggested fix
threads = threads.annotate( _has_unread=models.ThreadAccess.thread_unread_filter( request.user, mailbox_id ), _has_starred=models.ThreadAccess.thread_starred_filter( request.user, mailbox_id ), _has_unread_mention=Exists( models.UserEvent.objects.filter( thread=OuterRef("pk"), user=request.user, type=enums.UserEventTypeChoices.MENTION, read_at__isnull=True, ) ), _has_mention=Exists( models.UserEvent.objects.filter( thread=OuterRef("pk"), user=request.user, type=enums.UserEventTypeChoices.MENTION, ) ), + events_count=Count("events", distinct=True), )🤖 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 580 - 602, The OpenSearch fallback branch's threads queryset is missing the same events_count annotation added in get_queryset(), causing inconsistent serialized output; update the threads Annotate call (where threads = threads.annotate(...)) to include events_count=models.Count("events", distinct=True) so both the normal get_queryset() path and the OpenSearch fallback expose the same events_count field for serialization.
319-389:⚠️ Potential issue | 🟠 MajorHandle mention
_unreadstats explicitly.Because
has_unread_mentionandhas_mentionare whitelisted invalid_base_fields, requests likestats_fields=has_mention_unreadnow pass validation. The generic_unreadbranch on Lines 382-389 then buildsQ(has_mention=True)/Q(has_unread_mention=True), but those are neither model fields nor queryset annotations, so this path can raise at aggregation time instead of returning a count.Either reject those two
_unreadvariants during validation, or map them tomention_condition & unread_condition/unread_mention_condition & unread_conditionexplicitly.💡 Suggested fix
elif field == "has_unread_mention": aggregations[agg_key] = Count("pk", filter=unread_mention_condition) elif field == "has_mention": aggregations[agg_key] = Count("pk", filter=mention_condition) + elif field == "has_unread_mention_unread": + aggregations[agg_key] = Count( + "pk", filter=unread_mention_condition & unread_condition + ) + elif field == "has_mention_unread": + aggregations[agg_key] = Count( + "pk", filter=mention_condition & unread_condition + ) elif field.endswith("_unread"): base_field = field[:-7] base_condition = Q(**{base_field: True})🤖 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 319 - 389, The validation/aggregation logic allows fields like "has_mention_unread" to pass but the generic field.endswith("_unread") branch builds Q(has_mention=True) which is not a real model/annotation; fix by handling mention/unread combos explicitly: in the aggregation loop (where aggregations is built and variables unread_condition, mention_condition, unread_mention_condition exist) add explicit branches for "has_mention_unread" -> Count("pk", filter=mention_condition & unread_condition) and "has_unread_mention_unread" (if needed) -> Count("pk", filter=unread_mention_condition & unread_condition), or alternatively update the validation loop over requested_fields to reject base_field values "has_mention" and "has_unread_mention" when used with the "_unread" suffix so they never reach the generic field.endswith("_unread") branch.
♻️ Duplicate comments (2)
src/frontend/src/features/message/use-mention-read.tsx (1)
45-61:⚠️ Potential issue | 🟡 MinorGuard against undefined
selectedMailboxbefore cache invalidation.The
markMentionsReadcallback usesselectedMailbox?.idat line 60, but ifselectedMailboxisnull/undefined, the query key becomes['threads', undefined]which could invalidate unintended cache entries. The past review suggested adding an early return guard.🛡️ Suggested fix
const markMentionsRead = useCallback((threadEventIds: string[]) => { - if (!threadEventIds.length) return; + if (!threadEventIds.length || !selectedMailbox?.id) return; Promise.all(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/message/use-mention-read.tsx` around lines 45 - 61, The markMentionsRead callback can call queryClient.invalidateQueries with an undefined mailbox id when selectedMailbox is null/undefined; add a guard at the start of markMentionsRead to return early if selectedMailbox is falsy (or if selectedMailbox.id is undefined) so you only call getMailboxThreadsListQueryKeyPrefix(selectedMailbox.id) when a valid mailbox exists, and keep the existing invalidateThreadsStats call if that should always run; update the logic around invalidateThreadsStats and queryClient.invalidateQueries in markMentionsRead to only run the latter when selectedMailbox?.id is present (reference markMentionsRead, selectedMailbox, invalidateThreadsStats, queryClient.invalidateQueries, getMailboxThreadsListQueryKeyPrefix).src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx (1)
188-209:⚠️ Potential issue | 🟠 MajorMove persisted folder-state loading out of the state initializer.
This still reads
localStorageduring render. The server path returns{ inbox: true }, while the first client render can hydrate from persisted storage, so the markup can diverge before React hydrates.JSON.parsehere can also crash the whole sidebar on malformed storage.Initialize from an in-memory default, then load/persist inside
useEffectwith a smalltry/catch.In Next.js App Router / React hydration, can reading `localStorage` in a `useState` initializer for a client component cause hydration mismatches when the server-rendered initial state differs from the client-rendered state?💡 Suggested fix
-import { useMemo, useState } from "react" +import { useEffect, useMemo, useState } from "react" - const [expandedFolders, setExpandedFolders] = useState<Record<string, boolean>>(() => { - if (typeof window === 'undefined') return { 'inbox': true }; - const savedState = localStorage.getItem(EXPANDED_FOLDERS_KEY); - if (savedState === null) return { 'inbox': true }; - return JSON.parse(savedState) as Record<string, boolean>; - }); + const [expandedFolders, setExpandedFolders] = useState<Record<string, boolean>>({ + inbox: true, + }); + + useEffect(() => { + if (typeof window === "undefined") return; + try { + const savedState = window.localStorage.getItem(EXPANDED_FOLDERS_KEY); + if (savedState !== null) { + setExpandedFolders(JSON.parse(savedState) as Record<string, boolean>); + } + } catch { + window.localStorage.removeItem(EXPANDED_FOLDERS_KEY); + } + }, []); const toggleFolder = (folderId: string) => { setExpandedFolders((prev) => { const nextState = { ...prev, [folderId]: !prev[folderId], }; - if (typeof window !== 'undefined') { - localStorage.setItem(EXPANDED_FOLDERS_KEY, JSON.stringify(nextState)); + if (typeof window !== "undefined") { + window.localStorage.setItem( + EXPANDED_FOLDERS_KEY, + JSON.stringify(nextState), + ); } return nextState; }); };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx` around lines 188 - 209, The state initializer for expandedFolders currently reads localStorage during render and JSON.parse can throw; change it to initialize useState(() => ({ inbox: true })) only, then add a useEffect that runs on mount to read EXPANDED_FOLDERS_KEY inside a try/catch, JSON.parse safely, and call setExpandedFolders(parsed) if valid; keep toggleFolder (and localStorage.setItem) as-is or alternatively move persistence to a separate useEffect that writes localStorage whenever expandedFolders changes, but do not perform any localStorage reads in the state initializer to avoid hydration mismatches and parse crashes.
🧹 Nitpick comments (6)
src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx (1)
145-145: Redundant dependency in useMemo.
hasActiveFiltersis derived fromactiveFilters, so including both in the dependency array is redundant. This doesn't cause bugs, but you could simplify it.♻️ Suggested simplification
- }, [hasActiveFilters, activeFilters, isSearch, threads?.count, t]); + }, [activeFilters, isSearch, threads?.count, t]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx` at line 145, Remove the redundant dependency hasActiveFilters from the dependency array of the useMemo in thread-panel-header.tsx because it is derived from activeFilters; update the dependency list to rely on activeFilters (and keep isSearch, threads?.count, t) so the memoization is correct and not duplicated.src/backend/e2e/management/commands/e2e_demo.py (1)
532-547: Edge-case timing: event may still be editable at the exact boundary.The
is_editable()method uses<=comparison:return timezone.now() - self.created_at <= timedelta(seconds=delay)With
past = timezone.now() - timedelta(seconds=delay), if checked immediately the event will be exactly at the boundary andis_editable()could still returnTrue. For a more deterministic test, consider subtracting an extra second:🛠️ Suggested adjustment
- past = timezone.now() - timedelta(seconds=settings.MAX_THREAD_EVENT_EDIT_DELAY) + past = timezone.now() - timedelta(seconds=settings.MAX_THREAD_EVENT_EDIT_DELAY + 1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/e2e/management/commands/e2e_demo.py` around lines 532 - 547, The test sets ThreadEvent.created_at to past = timezone.now() - timedelta(seconds=settings.MAX_THREAD_EVENT_EDIT_DELAY) which can land exactly on the is_editable() boundary (is_editable() uses <=), so change the past calculation to subtract an extra second (e.g., subtract timedelta(seconds=settings.MAX_THREAD_EVENT_EDIT_DELAY + 1)) before updating the ThreadEvent timestamps so events are deterministically non-editable; update the code that builds `past` and the subsequent models.ThreadEvent.objects.filter(pk=event.pk).update(created_at=past, updated_at=past) accordingly.src/backend/core/tests/api/test_thread_event.py (1)
204-238: Please lock in the new author-only rule with a regression test.
HasThreadEditAccessnow deniesPATCH/DELETEwhen the caller is an editor on the thread but is notobj.author_id. The suite still only covers the author path and the no-access path, so a regression back to “any editor can edit/delete” would slip through.Please add one
PATCHand oneDELETEcase for a second editor on the same thread returning403.As per coding guidelines "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases".
Also applies to: 396-423
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/backend/core/tests/api/test_thread_event.py` around lines 204 - 238, Add regression tests to ensure HasThreadEditAccess denies non-author editors: in the TestThreadEventUpdate class add one test method that creates a second user who has editor access via setup_user_with_thread_access (but is not the ThreadEvent author), authenticates that user with api_client.force_authenticate, then attempts a PATCH to get_thread_event_url(thread.id, event.id) with updated data and asserts response.status_code == status.HTTP_403_FORBIDDEN; add a second test that does the same setup but sends a DELETE to the same URL and asserts a 403. Use factories.ThreadEventFactory to create the event with author=user_a and ensure the authenticated user is user_b (editor) so the checked behavior exercises HasThreadEditAccess denying non-author editors.src/backend/core/migrations/0025_alter_threadevent_type_userevent.py (1)
39-40: Add an index that matches the new mention lookup shape.The hot queries added in
src/backend/core/api/viewsets/thread.pyfilterUserEventbythread,user,type, and sometimesread_at. The two separate indexes here only partially cover that predicate, so the inbox list/stats path will end up combining scans or post-filtering on what is now a frequentExists(...)lookup.A composite index keyed by
(thread, user, type, read_at)or(thread, user, type)would fit those lookups much better.As per coding guidelines "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/migrations/0025_alter_threadevent_type_userevent.py` around lines 39 - 40, Add a composite index to the migration's 'indexes' list so the hot lookup on UserEvent by thread, user, type (and optionally read_at) is covered; in the same block where the existing Index entries 'usrevt_user_type_read' and 'usrevt_thread_type' are defined, append a new models.Index with fields=['thread','user','type','read_at'] (or fields=['thread','user','type'] if you prefer to omit read_at) and give it a unique name like 'usrevt_thread_user_type_read' so the inbox Exists(...) queries are supported by an index.src/backend/core/api/openapi.json (1)
9050-9053: Consider adding descriptions for new exposed fields.
events_countandis_editablewould benefit from short descriptions in generated API docs.✍️ Suggested OpenAPI doc polish
"events_count": { "type": "integer", + "description": "Number of events currently attached to the thread.", "readOnly": true } ... "is_editable": { "type": "boolean", + "description": "Whether the current user can still edit this thread event.", "readOnly": true },Also applies to: 9289-9292
🤖 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 9050 - 9053, Add short "description" entries to the exposed schema fields to improve generated API docs: for the property "events_count" (currently with "type": "integer" and "readOnly": true) add a concise description like "Total number of events associated with this resource", and for "is_editable" add a description like "Indicates whether this resource can be edited". Make the change in the same schema objects where "events_count" and "is_editable" are defined (including the other occurrence around lines 9289-9292) and keep existing attributes (type, readOnly) intact.src/backend/core/tests/api/test_thread_filter_mention.py (1)
35-79: Add explicit “read mention is excluded” cases for unread filter/stats.
has_unread_mentiontests currently don’t seed a read mention and assert exclusion. If unread logic accidentally drifts to “any mention,” these tests could still pass.✅ Suggested test additions
+ def test_filter_excludes_read_mentions(self, api_client): + user, mailbox, thread = setup_user_with_thread_access() + api_client.force_authenticate(user=user) + + event = factories.ThreadEventFactory(thread=thread, author=user) + factories.UserEventFactory( + user=user, + thread=thread, + thread_event=event, + type=enums.UserEventTypeChoices.MENTION, + read_at=timezone.now(), + ) + + response = api_client.get( + reverse("threads-list"), + {"mailbox_id": str(mailbox.id), "has_unread_mention": "1"}, + ) + + assert response.status_code == status.HTTP_200_OK + assert len(response.data["results"]) == 0 + + def test_stats_excludes_read_mentions(self, api_client): + user, mailbox, thread = setup_user_with_thread_access() + api_client.force_authenticate(user=user) + + event = factories.ThreadEventFactory(thread=thread, author=user) + factories.UserEventFactory( + user=user, + thread=thread, + thread_event=event, + type=enums.UserEventTypeChoices.MENTION, + read_at=timezone.now(), + ) + + response = api_client.get( + reverse("threads-stats"), + {"mailbox_id": str(mailbox.id), "stats_fields": "has_unread_mention"}, + ) + + assert response.status_code == status.HTTP_200_OK + assert response.data["has_unread_mention"] == 0As per coding guidelines, "Unit tests should focus on a single use case, keep assertions minimal, and cover all possible cases."
Also applies to: 107-145
🤖 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_filter_mention.py` around lines 35 - 79, The tests test_filter_returns_threads_with_unread_mention and test_filter_returns_empty_when_no_unread_mentions lack a case verifying that read mentions are excluded; add a case that seeds a UserEvent with type=enums.UserEventTypeChoices.MENTION linked to the same thread and user but with read=True (or status indicating read according to UserEvent model) and assert that when calling reverse("threads-list") with {"mailbox_id": str(mailbox.id), "has_unread_mention": "1"} that thread is not returned; similarly, add a test where only read mentions exist and assert the results are empty to ensure the filter truly requires unread mentions (use factories.UserEventFactory, ThreadEventFactory, and the existing setup_user_with_thread_access helper to locate code).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/backend/core/api/viewsets/thread.py`:
- Around line 580-602: The OpenSearch fallback branch's threads queryset is
missing the same events_count annotation added in get_queryset(), causing
inconsistent serialized output; update the threads Annotate call (where threads
= threads.annotate(...)) to include events_count=models.Count("events",
distinct=True) so both the normal get_queryset() path and the OpenSearch
fallback expose the same events_count field for serialization.
- Around line 319-389: The validation/aggregation logic allows fields like
"has_mention_unread" to pass but the generic field.endswith("_unread") branch
builds Q(has_mention=True) which is not a real model/annotation; fix by handling
mention/unread combos explicitly: in the aggregation loop (where aggregations is
built and variables unread_condition, mention_condition,
unread_mention_condition exist) add explicit branches for "has_mention_unread"
-> Count("pk", filter=mention_condition & unread_condition) and
"has_unread_mention_unread" (if needed) -> Count("pk",
filter=unread_mention_condition & unread_condition), or alternatively update the
validation loop over requested_fields to reject base_field values "has_mention"
and "has_unread_mention" when used with the "_unread" suffix so they never reach
the generic field.endswith("_unread") branch.
---
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx`:
- Around line 188-209: The state initializer for expandedFolders currently reads
localStorage during render and JSON.parse can throw; change it to initialize
useState(() => ({ inbox: true })) only, then add a useEffect that runs on mount
to read EXPANDED_FOLDERS_KEY inside a try/catch, JSON.parse safely, and call
setExpandedFolders(parsed) if valid; keep toggleFolder (and
localStorage.setItem) as-is or alternatively move persistence to a separate
useEffect that writes localStorage whenever expandedFolders changes, but do not
perform any localStorage reads in the state initializer to avoid hydration
mismatches and parse crashes.
In `@src/frontend/src/features/message/use-mention-read.tsx`:
- Around line 45-61: The markMentionsRead callback can call
queryClient.invalidateQueries with an undefined mailbox id when selectedMailbox
is null/undefined; add a guard at the start of markMentionsRead to return early
if selectedMailbox is falsy (or if selectedMailbox.id is undefined) so you only
call getMailboxThreadsListQueryKeyPrefix(selectedMailbox.id) when a valid
mailbox exists, and keep the existing invalidateThreadsStats call if that should
always run; update the logic around invalidateThreadsStats and
queryClient.invalidateQueries in markMentionsRead to only run the latter when
selectedMailbox?.id is present (reference markMentionsRead, selectedMailbox,
invalidateThreadsStats, queryClient.invalidateQueries,
getMailboxThreadsListQueryKeyPrefix).
---
Nitpick comments:
In `@src/backend/core/api/openapi.json`:
- Around line 9050-9053: Add short "description" entries to the exposed schema
fields to improve generated API docs: for the property "events_count" (currently
with "type": "integer" and "readOnly": true) add a concise description like
"Total number of events associated with this resource", and for "is_editable"
add a description like "Indicates whether this resource can be edited". Make the
change in the same schema objects where "events_count" and "is_editable" are
defined (including the other occurrence around lines 9289-9292) and keep
existing attributes (type, readOnly) intact.
In `@src/backend/core/migrations/0025_alter_threadevent_type_userevent.py`:
- Around line 39-40: Add a composite index to the migration's 'indexes' list so
the hot lookup on UserEvent by thread, user, type (and optionally read_at) is
covered; in the same block where the existing Index entries
'usrevt_user_type_read' and 'usrevt_thread_type' are defined, append a new
models.Index with fields=['thread','user','type','read_at'] (or
fields=['thread','user','type'] if you prefer to omit read_at) and give it a
unique name like 'usrevt_thread_user_type_read' so the inbox Exists(...) queries
are supported by an index.
In `@src/backend/core/tests/api/test_thread_event.py`:
- Around line 204-238: Add regression tests to ensure HasThreadEditAccess denies
non-author editors: in the TestThreadEventUpdate class add one test method that
creates a second user who has editor access via setup_user_with_thread_access
(but is not the ThreadEvent author), authenticates that user with
api_client.force_authenticate, then attempts a PATCH to
get_thread_event_url(thread.id, event.id) with updated data and asserts
response.status_code == status.HTTP_403_FORBIDDEN; add a second test that does
the same setup but sends a DELETE to the same URL and asserts a 403. Use
factories.ThreadEventFactory to create the event with author=user_a and ensure
the authenticated user is user_b (editor) so the checked behavior exercises
HasThreadEditAccess denying non-author editors.
In `@src/backend/core/tests/api/test_thread_filter_mention.py`:
- Around line 35-79: The tests test_filter_returns_threads_with_unread_mention
and test_filter_returns_empty_when_no_unread_mentions lack a case verifying that
read mentions are excluded; add a case that seeds a UserEvent with
type=enums.UserEventTypeChoices.MENTION linked to the same thread and user but
with read=True (or status indicating read according to UserEvent model) and
assert that when calling reverse("threads-list") with {"mailbox_id":
str(mailbox.id), "has_unread_mention": "1"} that thread is not returned;
similarly, add a test where only read mentions exist and assert the results are
empty to ensure the filter truly requires unread mentions (use
factories.UserEventFactory, ThreadEventFactory, and the existing
setup_user_with_thread_access helper to locate code).
In `@src/backend/e2e/management/commands/e2e_demo.py`:
- Around line 532-547: The test sets ThreadEvent.created_at to past =
timezone.now() - timedelta(seconds=settings.MAX_THREAD_EVENT_EDIT_DELAY) which
can land exactly on the is_editable() boundary (is_editable() uses <=), so
change the past calculation to subtract an extra second (e.g., subtract
timedelta(seconds=settings.MAX_THREAD_EVENT_EDIT_DELAY + 1)) before updating the
ThreadEvent timestamps so events are deterministically non-editable; update the
code that builds `past` and the subsequent
models.ThreadEvent.objects.filter(pk=event.pk).update(created_at=past,
updated_at=past) accordingly.
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Line 145: Remove the redundant dependency hasActiveFilters from the dependency
array of the useMemo in thread-panel-header.tsx because it is derived from
activeFilters; update the dependency list to rely on activeFilters (and keep
isSearch, threads?.count, t) so the memoization is correct and not duplicated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a244905-aa1b-4c7f-89ab-d4e26301711a
⛔ Files ignored due to path filters (8)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_stats_fields.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (45)
docs/env.mdsrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/migrations/0025_alter_threadevent_type_userevent.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_filter_mention.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/conftest.pysrc/backend/core/tests/models/test_user_event.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/settings.pysrc/e2e/src/__tests__/thread-event.spec.tssrc/e2e/src/__tests__/thread-starred-read.spec.tssrc/e2e/src/utils-test.tssrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/config/constants.tssrc/frontend/src/features/forms/components/search-filters-form/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/hooks/use-thread-panel-filters.tssrc/frontend/src/features/layouts/components/thread-panel/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/components/thread-summary/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-mention-read.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/features/utils/view-helper/index.tssrc/frontend/src/hooks/use-visibility-observer.ts
✅ Files skipped from review due to trivial changes (11)
- docs/env.md
- src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss
- src/frontend/public/locales/common/en-US.json
- src/frontend/src/features/forms/components/search-filters-form/index.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsx
- src/frontend/src/features/layouts/components/thread-panel/_index.scss
- src/frontend/src/features/config/constants.ts
- src/e2e/src/utils-test.ts
- src/backend/core/admin.py
- src/backend/core/api/serializers.py
🚧 Files skipped from review as they are similar to previous changes (9)
- src/frontend/public/locales/common/fr-FR.json
- src/backend/messages/settings.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx
- src/backend/core/tests/api/test_mailboxes.py
- src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx
- src/backend/core/factories.py
- src/frontend/src/hooks/use-visibility-observer.ts
- src/backend/core/enums.py
- src/frontend/src/features/layouts/components/thread-panel/hooks/use-thread-panel-filters.ts
jbpenrath
force-pushed
the
feat/mention-event
branch
3 times, most recently
from
f1e0004 to
1b035d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
111-126:⚠️ Potential issue | 🟠 MajorFailed mention-read batches become non-retryable for the rest of the session.
These IDs are added to
sentMentionIdsRefbefore the PATCH succeeds, andflushPendingMentions()clears the pending batch immediately. BecauseuseMentionRead()swallows request failures, a transient error leaves the mention unread on the server but permanently suppressed from future retries until the user switches threads.Also applies to: 182-188
🤖 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 111 - 126, The current flow marks IDs in sentMentionIdsRef before the PATCH completes and clears pendingMentionIdsRef immediately in flushPendingMentions, causing failed requests to be permanently suppressed; change the logic so flushPendingMentions collects ids but does not move them into sentMentionIdsRef or clear pendingMentionIdsRef until markMentionsRead resolves successfully, and on failure leaves pendingMentionIdsRef intact (or re-enqueues the ids) so debouncedFlushMentions/markMentionsRead can retry; update the code paths around pendingMentionIdsRef, sentMentionIdsRef, flushPendingMentions, debouncedFlushMentions and the call site of markMentionsRead (and the useMentionRead hook behavior) to only mark as sent after a successful PATCH and to handle transient errors by retrying or restoring the pending set.
🧹 Nitpick comments (2)
src/backend/core/tests/api/test_mailboxes.py (1)
349-454: Add the shared-thread mailbox case.This only validates disjoint threads.
UserEventis global to the(user, thread)pair, so one mentioned thread shared throughmailbox_aandmailbox_bshould increment both counters. Without that fixture, a mailbox-scoped implementation would still pass.🧪 Suggested addition
+ # Shared thread visible from both mailboxes should count in both counters + shared_thread = factories.ThreadFactory() + factories.ThreadAccessFactory( + mailbox=mailbox_a, + thread=shared_thread, + role=enums.ThreadAccessRoleChoices.EDITOR, + ) + factories.ThreadAccessFactory( + mailbox=mailbox_b, + thread=shared_thread, + role=enums.ThreadAccessRoleChoices.EDITOR, + ) + shared_event = factories.ThreadEventFactory(thread=shared_thread, author=other_user) + factories.UserEventFactory( + user=user, + thread=shared_thread, + thread_event=shared_event, + type=enums.UserEventTypeChoices.MENTION, + ) + by_id = {m["id"]: m for m in response.data} - assert by_id[str(mailbox_a.id)]["count_unread_mentions"] == 2 - assert by_id[str(mailbox_b.id)]["count_unread_mentions"] == 1 + assert by_id[str(mailbox_a.id)]["count_unread_mentions"] == 3 + assert by_id[str(mailbox_b.id)]["count_unread_mentions"] == 2As 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_mailboxes.py` around lines 349 - 454, The test test_list_count_unread_mentions is missing the case where a single thread is shared across mailboxes: add a shared thread (e.g., thread_shared) and grant ThreadAccessFactory for both mailbox_a and mailbox_b, create a ThreadEvent and an unread UserEvent for user tied to that thread, then update the final assertions to expect mailbox_a.count_unread_mentions == 3 and mailbox_b.count_unread_mentions == 2 so the shared-thread increments both mailboxes' counters; reference ThreadAccessFactory, ThreadEventFactory, and UserEventFactory in the change.src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx (1)
212-219: Consider addingaria-hiddento the invisible trigger element.The hidden mention trigger span is purely for IntersectionObserver mechanics and has no semantic content. Adding
aria-hidden="true"ensures screen readers skip it entirely.♻️ Suggested improvement
{event.has_unread_mention && ( <span className="thread-event__mention-trigger" ref={mentionRef} data-event-id={event.id} style={{ position: 'absolute', width: 0, height: 0, overflow: 'hidden' }} + aria-hidden="true" /> )}🤖 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 212 - 219, The invisible mention trigger span used for IntersectionObserver (rendered when event.has_unread_mention) should include aria-hidden="true" so screen readers ignore it; update the span with className "thread-event__mention-trigger" (the element using ref mentionRef and data-event-id={event.id}) to add aria-hidden="true" while keeping its existing props and inline styles.
🤖 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 4855-4862: The OpenAPI schema for boolean-like query flags such as
has_mention currently allows any integer; tighten the schema for the
"has_mention" query parameter (and the same params in /api/v1.0/threads/ and
/api/v1.0/threads/stats/) to constrain values to 0 or 1 by replacing the loose
integer type with a constrained schema (e.g., use an "enum": [0, 1] or
"minimum": 0 and "maximum": 1) so generated clients and validation only accept
the documented 0|1 values.
In `@src/backend/core/models.py`:
- Around line 1687-1695: The current UserEvent Meta indexes split predicates
across models.Index named "usrevt_user_type_read" and "usrevt_thread_type",
causing expensive Exists() subqueries; add a covering composite index on
UserEvent with fields ["user", "thread", "type", "read_at"] (e.g., name it
"usrevt_user_thread_type_read") in the UserEvent model's Meta.indexes so lookups
that filter by user+thread+type(+read_at) can use a single index for efficient
queries.
In `@src/frontend/public/locales/common/fr-FR.json`:
- Line 214: The JSON key "Collapse {{name}}" is missing its French translation,
causing blank UI labels; add an appropriate French string (e.g., "Réduire
{{name}}" or "Fermer {{name}}") as the value for the "Collapse {{name}}" key and
also fill the matching counterpart key (e.g., "Expand {{name}}") at the other
occurrence, making sure to keep the {{name}} placeholder intact and properly
localized.
In
`@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsx`:
- Line 47: The query key uses a non-null assertion selectedMailbox!.id which can
be undefined; change to optional chaining and add an enabled guard: use
selectedMailbox?.id when calling getThreadsStatsQueryKey and set the query's
enabled flag to Boolean(selectedMailbox?.id) (or similar) so the query only runs
when mailbox context is initialized; update the queryKey and the options for the
hook where queryKey: getThreadsStatsQueryKey(...) is used (reference
selectedMailbox, useMailboxContext, getThreadsStatsQueryKey, and the query's
enabled option).
In
`@src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx`:
- Around line 55-60: The code currently compares folder filters via full
querystring equality which fails with differing key order or extra unrelated
params; instead, parse both the folder.filter and the computed folderParams into
URLSearchParams and perform a robust match: treat the folder.filter params as
requirements and ensure every key in new URLSearchParams(folder.filter) has the
same set of values present in folderParams (use getAll and compare as
sets/arrays rather than string equality) when searching with findRootFolder;
update the matching logic around
findRootFolder/activeFolder/THREAD_PANEL_FILTER_PARAMS so folders are identified
by param-wise equivalence (subset match) not raw toString() equality.
---
Duplicate comments:
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 111-126: The current flow marks IDs in sentMentionIdsRef before
the PATCH completes and clears pendingMentionIdsRef immediately in
flushPendingMentions, causing failed requests to be permanently suppressed;
change the logic so flushPendingMentions collects ids but does not move them
into sentMentionIdsRef or clear pendingMentionIdsRef until markMentionsRead
resolves successfully, and on failure leaves pendingMentionIdsRef intact (or
re-enqueues the ids) so debouncedFlushMentions/markMentionsRead can retry;
update the code paths around pendingMentionIdsRef, sentMentionIdsRef,
flushPendingMentions, debouncedFlushMentions and the call site of
markMentionsRead (and the useMentionRead hook behavior) to only mark as sent
after a successful PATCH and to handle transient errors by retrying or restoring
the pending set.
---
Nitpick comments:
In `@src/backend/core/tests/api/test_mailboxes.py`:
- Around line 349-454: The test test_list_count_unread_mentions is missing the
case where a single thread is shared across mailboxes: add a shared thread
(e.g., thread_shared) and grant ThreadAccessFactory for both mailbox_a and
mailbox_b, create a ThreadEvent and an unread UserEvent for user tied to that
thread, then update the final assertions to expect
mailbox_a.count_unread_mentions == 3 and mailbox_b.count_unread_mentions == 2 so
the shared-thread increments both mailboxes' counters; reference
ThreadAccessFactory, ThreadEventFactory, and UserEventFactory in the change.
In
`@src/frontend/src/features/layouts/components/thread-view/components/thread-event/index.tsx`:
- Around line 212-219: The invisible mention trigger span used for
IntersectionObserver (rendered when event.has_unread_mention) should include
aria-hidden="true" so screen readers ignore it; update the span with className
"thread-event__mention-trigger" (the element using ref mentionRef and
data-event-id={event.id}) to add aria-hidden="true" while keeping its existing
props and inline styles.
🪄 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: e6969d17-15b0-4b25-b9f2-d544826c879d
⛔ Files ignored due to path filters (8)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_stats_fields.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (46)
docs/env.mdsrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/migrations/0025_alter_threadevent_type_userevent.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_filter_mention.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/core/tests/conftest.pysrc/backend/core/tests/models/test_user_event.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/settings.pysrc/e2e/src/__tests__/thread-event.spec.tssrc/e2e/src/__tests__/thread-starred-read.spec.tssrc/e2e/src/utils-test.tssrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/config/constants.tssrc/frontend/src/features/forms/components/search-filters-form/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/hooks/use-thread-panel-filters.tssrc/frontend/src/features/layouts/components/thread-panel/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/components/thread-summary/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-mention-read.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/features/utils/view-helper/index.tssrc/frontend/src/hooks/use-visibility-observer.ts
✅ Files skipped from review due to trivial changes (10)
- src/frontend/src/features/config/constants.ts
- docs/env.md
- src/e2e/src/tests/thread-starred-read.spec.ts
- src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss
- src/frontend/src/features/forms/components/search-filters-form/index.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scss
- src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx
- src/frontend/src/features/utils/view-helper/index.ts
- src/backend/core/admin.py
- src/e2e/src/tests/thread-event.spec.ts
🚧 Files skipped from review as they are similar to previous changes (15)
- src/e2e/src/utils-test.ts
- src/frontend/src/features/layouts/components/thread-panel/index.tsx
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/backend/core/tests/conftest.py
- src/backend/e2e/management/commands/e2e_demo.py
- src/frontend/src/features/message/use-mention-read.tsx
- src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsx
- src/frontend/src/hooks/use-visibility-observer.ts
- src/backend/core/api/permissions.py
- src/backend/core/tests/api/test_thread_event.py
- src/backend/core/api/viewsets/thread_event.py
- src/backend/core/migrations/0025_alter_threadevent_type_userevent.py
- src/frontend/src/features/providers/mailbox.tsx
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx (1)
188-193:⚠️ Potential issue | 🟠 MajorLoad persisted folder state after hydration, not during render.
The
typeof windowguard avoids the SSR crash, but this initializer still reads and parseslocalStorageon the first client render. If the saved value differs from the server default, the sidebar hydrates to different markup; malformed JSON will also throw during render. Start from a deterministic in-memory default and hydrate storage in auseEffectwith a safe fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx` around lines 188 - 193, The state initializer for expandedFolders currently reads localStorage during render (in the useState initializer), which risks SSR hydration mismatch and JSON parse errors; change to initialize expandedFolders to a deterministic default (e.g., {inbox: true}) and move the localStorage read/parse into a useEffect that runs on mount: inside the effect, read EXPANDED_FOLDERS_KEY, safely JSON.parse with try/catch, and call setExpandedFolders to hydrate the stored state only on the client; ensure malformed JSON falls back to the default and does not throw.src/frontend/src/features/layouts/components/thread-view/index.tsx (1)
183-188:⚠️ Potential issue | 🟠 MajorOnly mark mention IDs as sent after the PATCH succeeds.
eventIdis added tosentMentionIdsRefbefore the batch is flushed, and this component never removes it again ifmarkMentionsRead()fails. One transient PATCH error therefore suppresses every retry for that mention until the thread is reopened.🤖 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 183 - 188, The code prematurely adds eventId to sentMentionIdsRef before the batch PATCH succeeds; instead, only add IDs to sentMentionIdsRef after markMentionsRead (or the function invoked by debouncedFlushMentions) resolves successfully. Remove the sentMentionIdsRef.current.add(eventId) from the observer callback, keep pendingMentionIdsRef.current.add(eventId) and call debouncedFlushMentions(), then in the flush/markMentionsRead success handler move the corresponding IDs from pendingMentionIdsRef to sentMentionIdsRef (and remove them from pending); on error do not add to sentMentionIdsRef so retries can occur.
🧹 Nitpick comments (1)
src/frontend/src/features/message/use-mention-read.tsx (1)
45-61: Guard against undefinedselectedMailboxbefore invalidating.When
selectedMailbox?.idisundefined, the query keygetMailboxThreadsListQueryKeyPrefix(undefined)will produce['threads', undefined], which may not match intended queries or could cause unexpected cache behavior.🛡️ Suggested fix
const markMentionsRead = useCallback((threadEventIds: string[]) => { - if (!threadEventIds.length) return; + if (!threadEventIds.length || !selectedMailbox?.id) return; Promise.all(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/frontend/src/features/message/use-mention-read.tsx` around lines 45 - 61, The invalidate step in markMentionsRead uses getMailboxThreadsListQueryKeyPrefix(selectedMailbox?.id) even when selectedMailbox?.id may be undefined, which can create an unintended query key; update markMentionsRead to guard before calling queryClient.invalidateQueries by checking selectedMailbox and selectedMailbox.id (or computing the prefix and validating it) and only call queryClient.invalidateQueries with getMailboxThreadsListQueryKeyPrefix(selectedMailbox.id) when the mailbox id is defined; keep invalidateThreadsStats() unchanged and ensure you reference the existing functions/vars (markMentionsRead, selectedMailbox, getMailboxThreadsListQueryKeyPrefix, queryClient.invalidateQueries) when making the conditional guard.
🤖 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/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 295-307: The condensed root's visible unread badge isn't receiving
a ref because mentionRef is gated by eventData.has_unread_mention instead of the
computed unread state; update the code around the ThreadEvent render so it
computes a hasUnread variable from unreadMentionGroupMap (e.g. const hasUnread =
unreadMentionGroupMap.get(item.data.id) ?? false) and pass that into both
hasUnreadMention and the mentionRef conditional (assigning
mentionRefs.current[item.data.id] when hasUnread is true) so condensed siblings
that surface the badge get a ref and can trigger markMentionsRead().
In `@src/frontend/src/features/providers/mailbox.tsx`:
- Around line 229-243: The hook currently returns null for selectedMailbox
during router hydration which allows consumers to dereference
selectedMailbox.id; change the logic in the selector (the function that computes
selectedMailbox using mailboxQuery.data and MailboxRoleChoices) to always return
a stable fallback mailbox (e.g. the last mailbox in mailboxQuery.data.data or a
prioritized role match) instead of null, and move or gate any redirect effect
that calls router.replace() so it only runs when router.isReady is true; update
references around selectedMailbox, router.isReady, mailboxQuery, and the
redirect effect to ensure selectedMailbox is never null during render and
redirects wait for router.isReady.
---
Duplicate comments:
In
`@src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsx`:
- Around line 188-193: The state initializer for expandedFolders currently reads
localStorage during render (in the useState initializer), which risks SSR
hydration mismatch and JSON parse errors; change to initialize expandedFolders
to a deterministic default (e.g., {inbox: true}) and move the localStorage
read/parse into a useEffect that runs on mount: inside the effect, read
EXPANDED_FOLDERS_KEY, safely JSON.parse with try/catch, and call
setExpandedFolders to hydrate the stored state only on the client; ensure
malformed JSON falls back to the default and does not throw.
In `@src/frontend/src/features/layouts/components/thread-view/index.tsx`:
- Around line 183-188: The code prematurely adds eventId to sentMentionIdsRef
before the batch PATCH succeeds; instead, only add IDs to sentMentionIdsRef
after markMentionsRead (or the function invoked by debouncedFlushMentions)
resolves successfully. Remove the sentMentionIdsRef.current.add(eventId) from
the observer callback, keep pendingMentionIdsRef.current.add(eventId) and call
debouncedFlushMentions(), then in the flush/markMentionsRead success handler
move the corresponding IDs from pendingMentionIdsRef to sentMentionIdsRef (and
remove them from pending); on error do not add to sentMentionIdsRef so retries
can occur.
---
Nitpick comments:
In `@src/frontend/src/features/message/use-mention-read.tsx`:
- Around line 45-61: The invalidate step in markMentionsRead uses
getMailboxThreadsListQueryKeyPrefix(selectedMailbox?.id) even when
selectedMailbox?.id may be undefined, which can create an unintended query key;
update markMentionsRead to guard before calling queryClient.invalidateQueries by
checking selectedMailbox and selectedMailbox.id (or computing the prefix and
validating it) and only call queryClient.invalidateQueries with
getMailboxThreadsListQueryKeyPrefix(selectedMailbox.id) when the mailbox id is
defined; keep invalidateThreadsStats() unchanged and ensure you reference the
existing functions/vars (markMentionsRead, selectedMailbox,
getMailboxThreadsListQueryKeyPrefix, queryClient.invalidateQueries) when making
the conditional guard.
🪄 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: d42bbce3-afcc-4c8a-afea-b5869150afef
⛔ Files ignored due to path filters (8)
src/frontend/package-lock.jsonis excluded by!**/package-lock.jsonsrc/frontend/src/features/api/gen/models/mailbox.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/thread_event.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_list_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_params.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/models/threads_stats_retrieve_stats_fields.tsis excluded by!**/gen/**src/frontend/src/features/api/gen/thread-events/thread-events.tsis excluded by!**/gen/**
📒 Files selected for processing (46)
docs/env.mdsrc/backend/core/admin.pysrc/backend/core/api/openapi.jsonsrc/backend/core/api/permissions.pysrc/backend/core/api/serializers.pysrc/backend/core/api/viewsets/thread.pysrc/backend/core/api/viewsets/thread_event.pysrc/backend/core/enums.pysrc/backend/core/factories.pysrc/backend/core/migrations/0025_alter_threadevent_type_userevent.pysrc/backend/core/models.pysrc/backend/core/signals.pysrc/backend/core/tests/api/test_mailboxes.pysrc/backend/core/tests/api/test_thread_event.pysrc/backend/core/tests/api/test_thread_filter_mention.pysrc/backend/core/tests/api/test_thread_split.pysrc/backend/core/tests/api/test_threads_list.pysrc/backend/core/tests/conftest.pysrc/backend/core/tests/models/test_user_event.pysrc/backend/e2e/management/commands/e2e_demo.pysrc/backend/messages/settings.pysrc/e2e/src/__tests__/thread-event.spec.tssrc/e2e/src/__tests__/thread-starred-read.spec.tssrc/e2e/src/utils-test.tssrc/frontend/public/locales/common/en-US.jsonsrc/frontend/public/locales/common/fr-FR.jsonsrc/frontend/src/features/config/constants.tssrc/frontend/src/features/forms/components/search-filters-form/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsxsrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scsssrc/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/_index.scsssrc/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-filter.tsxsrc/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsxsrc/frontend/src/features/layouts/components/thread-panel/hooks/use-thread-panel-filters.tssrc/frontend/src/features/layouts/components/thread-panel/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/components/thread-summary/index.tsxsrc/frontend/src/features/layouts/components/thread-view/index.tsxsrc/frontend/src/features/message/use-mention-read.tsxsrc/frontend/src/features/providers/mailbox.tsxsrc/frontend/src/features/utils/view-helper/index.tssrc/frontend/src/hooks/use-visibility-observer.ts
✅ Files skipped from review due to trivial changes (12)
- docs/env.md
- src/frontend/src/features/config/constants.ts
- src/backend/messages/settings.py
- src/frontend/src/features/layouts/components/thread-view/components/thread-event/_index.scss
- src/e2e/src/tests/thread-starred-read.spec.ts
- src/frontend/src/features/layouts/components/thread-panel/components/thread-item/index.tsx
- src/frontend/public/locales/common/fr-FR.json
- src/frontend/src/features/forms/components/search-filters-form/index.tsx
- src/frontend/src/features/utils/view-helper/index.ts
- src/backend/core/factories.py
- src/backend/core/api/viewsets/thread.py
- src/backend/core/api/openapi.json
🚧 Files skipped from review as they are similar to previous changes (16)
- src/frontend/src/features/layouts/components/thread-panel/index.tsx
- src/e2e/src/utils-test.ts
- src/frontend/src/features/layouts/components/thread-view/components/thread-summary/index.tsx
- src/frontend/src/features/layouts/components/thread-panel/_index.scss
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/index.tsx
- src/frontend/src/features/layouts/components/thread-panel/components/thread-panel-header.tsx
- src/backend/core/tests/api/test_mailboxes.py
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-labels/components/label-item/_index.scss
- src/backend/core/enums.py
- src/backend/core/tests/conftest.py
- src/backend/core/admin.py
- src/backend/core/signals.py
- src/e2e/src/tests/thread-event.spec.ts
- src/frontend/src/hooks/use-visibility-observer.ts
- src/backend/core/tests/api/test_thread_event.py
- src/backend/core/migrations/0025_alter_threadevent_type_userevent.py
👮 Files not reviewed due to content moderation or server errors (6)
- src/frontend/public/locales/common/en-US.json
- src/frontend/src/features/layouts/components/mailbox-panel/components/mailbox-list/_index.scss
- src/backend/e2e/management/commands/e2e_demo.py
- src/backend/core/models.py
- src/backend/core/tests/models/test_user_event.py
- src/backend/core/tests/api/test_threads_list.py
jbpenrath
force-pushed
the
feat/mention-event
branch
2 times, most recently
from
27cf5b9 to
41c72e1
Compare
ThreadEvent IM mentions previously lived only inside the event payload, with no per-user tracking, so a user had no way to see or filter the threads where they were mentioned. The new UserEvent model materializes mentions as first-class records (one row per mentioned user per event), reconciled by a post_save signal whenever a ThreadEvent is created or edited. ThreadEvent edits and deletes are now bounded by THREAD_EVENT_EDIT_DELAY (1h default) so UserEvent records cannot drift out of sync with stale audit data past the window.
jbpenrath
force-pushed
the
feat/mention-event
branch
from
41c72e1 to
bdc21e0
Compare
This was referenced Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Purpose
A user is able to mention other users in a instant message. We now add inbox subfolders, to retrieve with ease messages where the authenticated users has been mentionned.
Summary by CodeRabbit
New Features
Documentation