Addapting baseapp_chats to follow plugin architecture#432
Open
Hercilio1 wants to merge 1 commit into
Open
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baseapp_core/apps.py (1)
11-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd explicit
ready()typing and a short docstring.
PackageConfig.readyhas startup side effects but is missing an explicit return annotation and method docstring.Proposed fix
class PackageConfig(AppConfig): @@ - def ready(self): + def ready(self) -> None: + """Apply registered pghistory and pgtrigger tracks during app startup.""" from .pghelpers import apply_pghistory_tracks, apply_pgtrigger_tracksAs per coding guidelines "Type-annotate all function parameters and return values in Python code" and "Add docstrings to non-trivial functions in Python code".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baseapp_core/apps.py` around lines 11 - 20, The ready method is missing an explicit return type and a docstring; update the PackageConfig.ready(self) method signature to include a return annotation (-> None) and add a concise docstring that explains it runs startup side effects (calls apply_pghistory_tracks() and apply_pgtrigger_tracks() to register DB history and domain triggers before makemigrations). Keep the docstring short and mention the functions apply_pghistory_tracks and apply_pgtrigger_tracks by name so future readers know what startup actions occur.
🧹 Nitpick comments (3)
baseapp_chats/tests/test_chats_participation_service.py (2)
21-22: 💤 Low valueConsider resolving swapped models at function scope.
Module-level
swapper.load_modelcalls execute at import time, which can be fragile if test collection happens before the app registry is fully ready in some configurations. The pattern works here (other tests likely do the same), but moving them inside each test (or a fixture) makes the file resilient to import-order changes. Optional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baseapp_chats/tests/test_chats_participation_service.py` around lines 21 - 22, Move the swapper.load_model calls out of module scope and resolve swapped models inside test functions or a fixture: replace the module-level ChatRoom = swapper.load_model("baseapp_chats","ChatRoom") and ChatRoomParticipant = swapper.load_model("baseapp_chats","ChatRoomParticipant") with calls to swapper.load_model within the specific test(s) or a setup fixture (e.g., in test_chats_participation_service tests or a pytest fixture) so model resolution happens at runtime when the app registry is ready.
63-67: 💤 Low valueStrengthen the no-rooms test with an explicit assertion.
The test currently only verifies that no exception is raised. Add a positive assertion that participation rows are unchanged (or simply that the user still has none) so a regression that, e.g., raises silently or returns early on the wrong branch is caught.
✅ Proposed addition
def test_cleanup_user_participation_handles_user_with_no_rooms(): """Idempotent on a clean user — no DB activity, no exception.""" user = UserFactory() shared_services.get("chats_participation").cleanup_user_participation(user) + + assert not ChatRoomParticipant.objects.filter(profile__user=user).exists()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baseapp_chats/tests/test_chats_participation_service.py` around lines 63 - 67, Add a positive assertion after invoking chats_participation.cleanup_user_participation(user): verify that there are no participation rows for that user (e.g. assert Participation.query.filter_by(user_id=user.id).count() == 0) so the test not only checks for no exception but also that the DB state is unchanged; reference the test name test_cleanup_user_participation_handles_user_with_no_rooms, the UserFactory-created user, and the cleanup_user_participation method to locate where to add the assertion.baseapp_chats/services.py (1)
29-36: ⚡ Quick winReplace the per-room
get+count+saveloop with a single bulk update.The current loop issues 3 queries per affected room (
SELECTthe room,COUNT(*)participants,UPDATE). For a user in many rooms this is a clear N+1 pattern, and the read-then-write also opens a small window for lost updates if anything else mutatesparticipants_countbetween the count and save (mitigated but not eliminated by the caller'stransaction.atomic). A single annotated update keeps it atomic and O(1) queries.♻️ Proposed refactor
- participant_qs = ChatRoomParticipant.objects.filter(profile__user=user) - room_ids = list(participant_qs.values_list("room_id", flat=True).distinct()) - participant_qs.delete() - - for room_id in room_ids: - room = ChatRoom.objects.get(id=room_id) - room.participants_count = ChatRoomParticipant.objects.filter(room=room).count() - room.save(update_fields=["participants_count"]) + from django.db.models import Count + + participant_qs = ChatRoomParticipant.objects.filter(profile__user=user) + room_ids = list(participant_qs.values_list("room_id", flat=True).distinct()) + participant_qs.delete() + + if not room_ids: + return + + affected_rooms = ChatRoom.objects.filter(id__in=room_ids).annotate( + remaining_participants=Count("chatroomparticipant") + ) + for room in affected_rooms: + room.participants_count = room.remaining_participants + ChatRoom.objects.bulk_update(affected_rooms, ["participants_count"])(Adjust the reverse accessor name if
ChatRoomParticipant.room'srelated_namediffers.)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@baseapp_chats/services.py` around lines 29 - 36, The per-room loop using participant_qs, room_ids, ChatRoomParticipant and ChatRoom causes N+1 queries and potential race windows; replace it with a single bulk update that computes participant counts via aggregation and writes them in one query — e.g. filter ChatRoom by room_ids, annotate each room with Count of the participant reverse relation (adjust the reverse accessor name if ChatRoomParticipant.room.related_name differs), then call update(participants_count=F('annotated_count')) so participants_count is set in one atomic bulk query instead of get/count/save per room.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@baseapp_chats/apps.py`:
- Around line 11-19: Both register_shared_services and
register_graphql_shared_interfaces lack type annotations; add explicit parameter
and return types (e.g. annotate the registry parameter with the appropriate
Registry type from baseapp_core.plugins and mark the functions as returning
None). Update the signatures of register_shared_services and
register_graphql_shared_interfaces to use that Registry type (or the correct
concrete plugin registry type) for the registry parameter and add -> None to
each function, importing the Registry type at top if needed.
In `@baseapp_chats/graphql/object_types.py`:
- Around line 77-98: Add missing type annotations to pre_optimization_hook:
annotate cls as Type[Any], queryset as django.db.models.QuerySet, optimizer as
Any, and the return type as django.db.models.QuerySet (or the appropriate
QuerySet subtype). Also add the necessary imports (from typing import Any, Type
and from django.db.models import QuerySet) and keep the body intact (references:
pre_optimization_hook, optimizer.only_fields, and return
super().pre_optimization_hook(queryset, optimizer)).
In `@baseapp_chats/graphql/shared_interfaces.py`:
- Around line 1-4: Add an explicit return type annotation to
get_chat_rooms_interface so its signature states it returns the
ChatRoomsInterface type; import or reference ChatRoomsInterface in the
annotation (e.g., using a forward reference string or typing.TYPE_CHECKING if
needed) and keep the body unchanged—ensure the function signature references
ChatRoomsInterface (the symbol returned) as the annotated return type.
In `@baseapp_chats/models.py`:
- Around line 185-198: The save() method performs synchronous fan-out and causes
N+1 queries and failure leakage: when created is True, iterate participants
using self.room.participants.select_related("profile").all() (or batch-resolve
profile IDs) to avoid per-participant queries, and move the notification loop
into a transaction.on_commit callback so dispatch happens after commit; inside
that callback call ChatRoomOnMessagesCountUpdate.send_updated_chat_count for
each participant but wrap each send in try/except (or log-and-swallow) so one
failing send doesn't raise for the whole save operation.
- Around line 185-198: The save method on the Message model unconditionally
accesses self.room.participants which will raise AttributeError when room is
null; update the save(self, *args, **kwargs) implementation to guard the
subscription/notification block with a check that self.room is not None (and
optionally that self.room.participants.exists()) before importing
ChatRoomOnMessagesCountUpdate and iterating participants, so newly-saved
messages without a room complete without error.
- Around line 131-140: The ForeignKey field action_object_content_type currently
uses related_name="action_object", which is singular and collides conceptually
with the GenericForeignKey attribute action_object; change the related_name to a
plural or non-conflicting name (e.g., "action_object_messages" or "+" if no
reverse is needed) on action_object_content_type to follow the
plural-related_name guideline and avoid confusion with the GenericForeignKey
action_object declaration; update any code that references the old reverse name
accordingly.
In `@baseapp_chats/services.py`:
- Line 25: The method cleanup_user_participation lacks a parameter type for
user; add a type annotation (e.g., user: AbstractBaseUser or a project-typed
alias for settings.AUTH_USER_MODEL) and import AbstractBaseUser under a
TYPE_CHECKING block or reference the app's USER model type alias; update the
function signature def cleanup_user_participation(self, user: AbstractBaseUser)
-> None: (or use the alias) so it complies with the typing guideline.
- Line 29: The queryset uses an incorrect field traversal:
ChatRoomParticipant.objects.filter(profile__user=user) fails because
baseapp_profiles.Profile defines owner (not user); change the filter to use
profile__owner=user or replace the direct filter with a helper like
Profile.for_user(user) / ChatRoomParticipant.for_user(user) to hide the mapping;
update the call site in the service (where participant_qs is built) to use
profile__owner=user or the new for_user(user) helper so the lookup matches the
Profile model shape (reference symbols: ChatRoomParticipant, profile,
Profile.owner, for_user).
---
Outside diff comments:
In `@baseapp_core/apps.py`:
- Around line 11-20: The ready method is missing an explicit return type and a
docstring; update the PackageConfig.ready(self) method signature to include a
return annotation (-> None) and add a concise docstring that explains it runs
startup side effects (calls apply_pghistory_tracks() and
apply_pgtrigger_tracks() to register DB history and domain triggers before
makemigrations). Keep the docstring short and mention the functions
apply_pghistory_tracks and apply_pgtrigger_tracks by name so future readers know
what startup actions occur.
---
Nitpick comments:
In `@baseapp_chats/services.py`:
- Around line 29-36: The per-room loop using participant_qs, room_ids,
ChatRoomParticipant and ChatRoom causes N+1 queries and potential race windows;
replace it with a single bulk update that computes participant counts via
aggregation and writes them in one query — e.g. filter ChatRoom by room_ids,
annotate each room with Count of the participant reverse relation (adjust the
reverse accessor name if ChatRoomParticipant.room.related_name differs), then
call update(participants_count=F('annotated_count')) so participants_count is
set in one atomic bulk query instead of get/count/save per room.
In `@baseapp_chats/tests/test_chats_participation_service.py`:
- Around line 21-22: Move the swapper.load_model calls out of module scope and
resolve swapped models inside test functions or a fixture: replace the
module-level ChatRoom = swapper.load_model("baseapp_chats","ChatRoom") and
ChatRoomParticipant = swapper.load_model("baseapp_chats","ChatRoomParticipant")
with calls to swapper.load_model within the specific test(s) or a setup fixture
(e.g., in test_chats_participation_service tests or a pytest fixture) so model
resolution happens at runtime when the app registry is ready.
- Around line 63-67: Add a positive assertion after invoking
chats_participation.cleanup_user_participation(user): verify that there are no
participation rows for that user (e.g. assert
Participation.query.filter_by(user_id=user.id).count() == 0) so the test not
only checks for no exception but also that the DB state is unchanged; reference
the test name test_cleanup_user_participation_handles_user_with_no_rooms, the
UserFactory-created user, and the cleanup_user_participation method to locate
where to add the assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67c00732-cbc3-42cd-8126-229cf895bbfb
📒 Files selected for processing (42)
baseapp_auth/rest_framework/users/tasks.pybaseapp_chats/README.mdbaseapp_chats/apps.pybaseapp_chats/base.pybaseapp_chats/graphql/interfaces.pybaseapp_chats/graphql/object_types.pybaseapp_chats/graphql/shared_interfaces.pybaseapp_chats/migrations/0001_initial.pybaseapp_chats/migrations/0002_message_set_last_message_and_more.pybaseapp_chats/migrations/0003_alter_unreadmessagecount_room.pybaseapp_chats/migrations/0004_chatroomparticipant_has_archived_room.pybaseapp_chats/migrations/0005_remove_messagestatus_increment_unread_count_and_more.pybaseapp_chats/migrations/0006_remove_messagestatus_increment_unread_count_and_more.pybaseapp_chats/migrations/0007_messagestatus_increment_unread_count.pybaseapp_chats/migrations/0008_alter_chatroomparticipant_options.pybaseapp_chats/migrations/0009_remove_message_create_message_status_and_more.pybaseapp_chats/migrations/0010_message_deleted.pybaseapp_chats/migrations/0011_alter_chatroom_created_by_and_more.pybaseapp_chats/migrations/0012_chatroom_created_by_profile.pybaseapp_chats/migrations/0013_chatroom_insert_document_id_and_more.pybaseapp_chats/migrations/0014_remove_chatroom_insert_document_id_and_more.pybaseapp_chats/models.pybaseapp_chats/plugin.pybaseapp_chats/services.pybaseapp_chats/tests/test_chats_participation_service.pybaseapp_chats/tests/test_graphql_queries.pybaseapp_chats/tests/test_graphql_query_counts.pybaseapp_chats/tests/test_shared_interfaces.pybaseapp_core/apps.pybaseapp_core/pghelpers.pybaseapp_core/tests/test_pghelpers.pybaseapp_profiles/graphql/object_types.pypyproject.tomltestproject/chats/migrations/__init__.pytestproject/chats/models.pytestproject/graphql.pytestproject/settings.pytestproject/social/chats/__init__.pytestproject/social/chats/apps.pytestproject/social/chats/migrations/0001_initial.pytestproject/social/chats/migrations/__init__.pytestproject/social/chats/models.py
💤 Files with no reviewable changes (17)
- baseapp_chats/migrations/0008_alter_chatroomparticipant_options.py
- baseapp_chats/migrations/0002_message_set_last_message_and_more.py
- baseapp_chats/migrations/0011_alter_chatroom_created_by_and_more.py
- testproject/chats/models.py
- baseapp_chats/migrations/0001_initial.py
- baseapp_chats/migrations/0014_remove_chatroom_insert_document_id_and_more.py
- baseapp_chats/migrations/0007_messagestatus_increment_unread_count.py
- baseapp_chats/migrations/0009_remove_message_create_message_status_and_more.py
- baseapp_chats/base.py
- baseapp_chats/migrations/0005_remove_messagestatus_increment_unread_count_and_more.py
- baseapp_chats/migrations/0010_message_deleted.py
- baseapp_chats/migrations/0013_chatroom_insert_document_id_and_more.py
- baseapp_chats/migrations/0004_chatroomparticipant_has_archived_room.py
- baseapp_chats/migrations/0006_remove_messagestatus_increment_unread_count_and_more.py
- baseapp_chats/migrations/0012_chatroom_created_by_profile.py
- baseapp_chats/migrations/0003_alter_unreadmessagecount_room.py
- testproject/graphql.py
Comment on lines
+11
to
+19
| def register_shared_services(self, registry): | ||
| from .services import ChatsParticipationService | ||
|
|
||
| registry.register(ChatsParticipationService()) | ||
|
|
||
| def register_graphql_shared_interfaces(self, registry): | ||
| from .graphql.shared_interfaces import get_chat_rooms_interface | ||
|
|
||
| registry.register("ChatRoomsInterface", get_chat_rooms_interface) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type annotations to the new registration hooks.
Both register_shared_services and register_graphql_shared_interfaces are missing parameter and return type annotations. As per coding guidelines, "Type-annotate all function parameters and return values in Python code".
♻️ Suggested annotations (substitute the actual registry types from baseapp_core.plugins)
- def register_shared_services(self, registry):
+ def register_shared_services(self, registry: "SharedServicesRegistry") -> None:
from .services import ChatsParticipationService
registry.register(ChatsParticipationService())
- def register_graphql_shared_interfaces(self, registry):
+ def register_graphql_shared_interfaces(self, registry: "SharedInterfacesRegistry") -> None:
from .graphql.shared_interfaces import get_chat_rooms_interface
registry.register("ChatRoomsInterface", get_chat_rooms_interface)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def register_shared_services(self, registry): | |
| from .services import ChatsParticipationService | |
| registry.register(ChatsParticipationService()) | |
| def register_graphql_shared_interfaces(self, registry): | |
| from .graphql.shared_interfaces import get_chat_rooms_interface | |
| registry.register("ChatRoomsInterface", get_chat_rooms_interface) | |
| def register_shared_services(self, registry: "SharedServicesRegistry") -> None: | |
| from .services import ChatsParticipationService | |
| registry.register(ChatsParticipationService()) | |
| def register_graphql_shared_interfaces(self, registry: "SharedInterfacesRegistry") -> None: | |
| from .graphql.shared_interfaces import get_chat_rooms_interface | |
| registry.register("ChatRoomsInterface", get_chat_rooms_interface) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/apps.py` around lines 11 - 19, Both register_shared_services
and register_graphql_shared_interfaces lack type annotations; add explicit
parameter and return types (e.g. annotate the registry parameter with the
appropriate Registry type from baseapp_core.plugins and mark the functions as
returning None). Update the signatures of register_shared_services and
register_graphql_shared_interfaces to use that Registry type (or the correct
concrete plugin registry type) for the registry parameter and add -> None to
each function, importing the Registry type at top if needed.
Comment on lines
+77
to
+98
| @classmethod | ||
| def pre_optimization_hook(cls, queryset, optimizer): | ||
| """Preload three columns the optimizer would otherwise defer. | ||
|
|
||
| - `room_id`: when `chatRoom.allMessages` is evaluated, Django's | ||
| prefetch pipeline reads `message.room_id` to map children | ||
| back to their parent. If it's not in `only_fields`, every | ||
| message triggers `refresh_from_db(['room_id'])` (N round trips). | ||
| - `deleted`, `message_type`: `resolve_content` branches on both | ||
| before returning `root.content`. They live on the model but | ||
| aren't surfaced through GraphQL, so they get deferred and | ||
| each access fans out as a per-row `refresh_from_db`. | ||
|
|
||
| All three are tiny scalars; loading them unconditionally costs | ||
| a few bytes per row and is far cheaper than the AST-walk we'd | ||
| need to gate on which fields the caller actually requested. | ||
| """ | ||
| for field_name in ("room_id", "deleted", "message_type"): | ||
| if field_name not in optimizer.only_fields: | ||
| optimizer.only_fields.append(field_name) | ||
| return super().pre_optimization_hook(queryset, optimizer) | ||
|
|
There was a problem hiding this comment.
Missing type annotations on pre_optimization_hook.
As per coding guidelines, "Type-annotate all function parameters and return values in Python code". The docstring is great; just round it out with annotations.
Proposed fix
`@classmethod`
- def pre_optimization_hook(cls, queryset, optimizer):
+ def pre_optimization_hook(cls, queryset: "QuerySet", optimizer: "QueryOptimizer") -> "QuerySet":
"""Preload three columns the optimizer would otherwise defer.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| @classmethod | |
| def pre_optimization_hook(cls, queryset, optimizer): | |
| """Preload three columns the optimizer would otherwise defer. | |
| - `room_id`: when `chatRoom.allMessages` is evaluated, Django's | |
| prefetch pipeline reads `message.room_id` to map children | |
| back to their parent. If it's not in `only_fields`, every | |
| message triggers `refresh_from_db(['room_id'])` (N round trips). | |
| - `deleted`, `message_type`: `resolve_content` branches on both | |
| before returning `root.content`. They live on the model but | |
| aren't surfaced through GraphQL, so they get deferred and | |
| each access fans out as a per-row `refresh_from_db`. | |
| All three are tiny scalars; loading them unconditionally costs | |
| a few bytes per row and is far cheaper than the AST-walk we'd | |
| need to gate on which fields the caller actually requested. | |
| """ | |
| for field_name in ("room_id", "deleted", "message_type"): | |
| if field_name not in optimizer.only_fields: | |
| optimizer.only_fields.append(field_name) | |
| return super().pre_optimization_hook(queryset, optimizer) | |
| `@classmethod` | |
| def pre_optimization_hook(cls, queryset: "QuerySet", optimizer: "QueryOptimizer") -> "QuerySet": | |
| """Preload three columns the optimizer would otherwise defer. | |
| - `room_id`: when `chatRoom.allMessages` is evaluated, Django's | |
| prefetch pipeline reads `message.room_id` to map children | |
| back to their parent. If it's not in `only_fields`, every | |
| message triggers `refresh_from_db(['room_id'])` (N round trips). | |
| - `deleted`, `message_type`: `resolve_content` branches on both | |
| before returning `root.content`. They live on the model but | |
| aren't surfaced through GraphQL, so they get deferred and | |
| each access fans out as a per-row `refresh_from_db`. | |
| All three are tiny scalars; loading them unconditionally costs | |
| a few bytes per row and is far cheaper than the AST-walk we'd | |
| need to gate on which fields the caller actually requested. | |
| """ | |
| for field_name in ("room_id", "deleted", "message_type"): | |
| if field_name not in optimizer.only_fields: | |
| optimizer.only_fields.append(field_name) | |
| return super().pre_optimization_hook(queryset, optimizer) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/graphql/object_types.py` around lines 77 - 98, Add missing type
annotations to pre_optimization_hook: annotate cls as Type[Any], queryset as
django.db.models.QuerySet, optimizer as Any, and the return type as
django.db.models.QuerySet (or the appropriate QuerySet subtype). Also add the
necessary imports (from typing import Any, Type and from django.db.models import
QuerySet) and keep the body intact (references: pre_optimization_hook,
optimizer.only_fields, and return super().pre_optimization_hook(queryset,
optimizer)).
Comment on lines
+1
to
+4
| def get_chat_rooms_interface(): | ||
| from .interfaces import ChatRoomsInterface | ||
|
|
||
| return ChatRoomsInterface |
There was a problem hiding this comment.
Add return type annotation.
As per coding guidelines, "Type-annotate all function parameters and return values in Python code".
Proposed fix
-def get_chat_rooms_interface():
+def get_chat_rooms_interface() -> type:
from .interfaces import ChatRoomsInterface
return ChatRoomsInterface📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def get_chat_rooms_interface(): | |
| from .interfaces import ChatRoomsInterface | |
| return ChatRoomsInterface | |
| def get_chat_rooms_interface() -> type: | |
| from .interfaces import ChatRoomsInterface | |
| return ChatRoomsInterface |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/graphql/shared_interfaces.py` around lines 1 - 4, Add an
explicit return type annotation to get_chat_rooms_interface so its signature
states it returns the ChatRoomsInterface type; import or reference
ChatRoomsInterface in the annotation (e.g., using a forward reference string or
typing.TYPE_CHECKING if needed) and keep the body unchanged—ensure the function
signature references ChatRoomsInterface (the symbol returned) as the annotated
return type.
Comment on lines
+131
to
+140
| action_object_content_type = models.ForeignKey( | ||
| ContentType, | ||
| blank=True, | ||
| null=True, | ||
| related_name="action_object", | ||
| on_delete=models.CASCADE, | ||
| db_index=True, | ||
| ) | ||
| action_object_object_id = models.IntegerField(blank=True, null=True, db_index=True) | ||
| action_object = GenericForeignKey("action_object_content_type", "action_object_object_id") |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
related_name="action_object" is singular on a ForeignKey and collides with the GenericForeignKey name.
Two small issues here:
- The reverse accessor on
ContentTypeis singular (action_object), but the guideline calls for plural onForeignKey. Something likerelated_name="action_object_messages"(or"+"if the reverse is never needed) would be more accurate — aContentTypecan be referenced by many messages. - The chosen reverse name is also identical to the
GenericForeignKeyattribute declared two lines below (action_object = GenericForeignKey(...)). It's not a Python collision (they live on different models) but it is easy to misread and to grep for.
As per coding guidelines: "Use plural related_name on ForeignKey/ManyToManyField; use singular related_name on OneToOneField in Django models".
♻️ Suggested rename
action_object_content_type = models.ForeignKey(
ContentType,
blank=True,
null=True,
- related_name="action_object",
+ related_name="action_object_messages",
on_delete=models.CASCADE,
db_index=True,
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| action_object_content_type = models.ForeignKey( | |
| ContentType, | |
| blank=True, | |
| null=True, | |
| related_name="action_object", | |
| on_delete=models.CASCADE, | |
| db_index=True, | |
| ) | |
| action_object_object_id = models.IntegerField(blank=True, null=True, db_index=True) | |
| action_object = GenericForeignKey("action_object_content_type", "action_object_object_id") | |
| action_object_content_type = models.ForeignKey( | |
| ContentType, | |
| blank=True, | |
| null=True, | |
| related_name="action_object_messages", | |
| on_delete=models.CASCADE, | |
| db_index=True, | |
| ) | |
| action_object_object_id = models.IntegerField(blank=True, null=True, db_index=True) | |
| action_object = GenericForeignKey("action_object_content_type", "action_object_object_id") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/models.py` around lines 131 - 140, The ForeignKey field
action_object_content_type currently uses related_name="action_object", which is
singular and collides conceptually with the GenericForeignKey attribute
action_object; change the related_name to a plural or non-conflicting name
(e.g., "action_object_messages" or "+" if no reverse is needed) on
action_object_content_type to follow the plural-related_name guideline and avoid
confusion with the GenericForeignKey action_object declaration; update any code
that references the old reverse name accordingly.
Comment on lines
+185
to
+198
| def save(self, *args, **kwargs): | ||
| created = self._state.adding | ||
| super().save(*args, **kwargs) | ||
|
|
||
| if created: | ||
| from baseapp_chats.graphql.subscriptions import ( | ||
| ChatRoomOnMessagesCountUpdate, | ||
| ) | ||
|
|
||
| for participant in self.room.participants.all(): | ||
| if participant.profile_id != self.profile_id: | ||
| ChatRoomOnMessagesCountUpdate.send_updated_chat_count( | ||
| profile=participant.profile, profile_id=participant.profile.relay_id | ||
| ) |
There was a problem hiding this comment.
Synchronous fan-out inside save() is an N+1 and a reliability hazard.
Two concerns on the new write path:
-
N+1 queries.
self.room.participants.all()is iterated withoutselect_related("profile"), so eachparticipant.profile.relay_idlookup issues an extra query. For rooms with many participants this scales poorly on every message insert (a very hot path). Considerselect_related("profile")(see proposed diff in the critical comment above), or batch the IDs and resolve once. -
Post-commit failure visibility. Subscription dispatch happens after
super().save()succeeds, with no try/except and notransaction.on_commitdeferral. Ifsend_updated_chat_countraises (broker down, serialization error, etc.), the message is already persisted but the caller sees an exception, leaving subscribers permanently out of sync with no retry. Worth wrapping intransaction.on_commit(...)and/or logging-and-swallowing per participant so one bad subscriber doesn't poison the rest.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/models.py` around lines 185 - 198, The save() method performs
synchronous fan-out and causes N+1 queries and failure leakage: when created is
True, iterate participants using
self.room.participants.select_related("profile").all() (or batch-resolve profile
IDs) to avoid per-participant queries, and move the notification loop into a
transaction.on_commit callback so dispatch happens after commit; inside that
callback call ChatRoomOnMessagesCountUpdate.send_updated_chat_count for each
participant but wrap each send in try/except (or log-and-swallow) so one failing
send doesn't raise for the whole save operation.
save() will raise AttributeError when room is null.
room is declared null=True, blank=True (Line 117), so a Message can legitimately be persisted without a room. On creation, however, this block unconditionally dereferences self.room.participants, which will raise AttributeError: 'NoneType' object has no attribute 'participants' instead of completing the save cleanly.
🐛 Proposed guard
- if created:
+ if created and self.room_id:
from baseapp_chats.graphql.subscriptions import (
ChatRoomOnMessagesCountUpdate,
)
- for participant in self.room.participants.all():
+ for participant in self.room.participants.select_related("profile").all():
if participant.profile_id != self.profile_id:
ChatRoomOnMessagesCountUpdate.send_updated_chat_count(
profile=participant.profile, profile_id=participant.profile.relay_id
)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/models.py` around lines 185 - 198, The save method on the
Message model unconditionally accesses self.room.participants which will raise
AttributeError when room is null; update the save(self, *args, **kwargs)
implementation to guard the subscription/notification block with a check that
self.room is not None (and optionally that self.room.participants.exists())
before importing ChatRoomOnMessagesCountUpdate and iterating participants, so
newly-saved messages without a room complete without error.
| def is_available(self) -> bool: | ||
| return apps.is_installed("baseapp_chats") | ||
|
|
||
| def cleanup_user_participation(self, user) -> None: |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add a type annotation for user.
Per coding guidelines, all function parameters should be type-annotated.
📝 Proposed fix
- def cleanup_user_participation(self, user) -> None:
+ def cleanup_user_participation(self, user: "AbstractBaseUser") -> None:Add from django.contrib.auth.models import AbstractBaseUser under TYPE_CHECKING (or use settings.AUTH_USER_MODEL-typed alias).
As per coding guidelines: "Type-annotate all function parameters and return values in Python code".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def cleanup_user_participation(self, user) -> None: | |
| def cleanup_user_participation(self, user: "AbstractBaseUser") -> None: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/services.py` at line 25, The method cleanup_user_participation
lacks a parameter type for user; add a type annotation (e.g., user:
AbstractBaseUser or a project-typed alias for settings.AUTH_USER_MODEL) and
import AbstractBaseUser under a TYPE_CHECKING block or reference the app's USER
model type alias; update the function signature def
cleanup_user_participation(self, user: AbstractBaseUser) -> None: (or use the
alias) so it complies with the typing guideline.
| ChatRoomParticipant = swapper.load_model("baseapp_chats", "ChatRoomParticipant") | ||
| ChatRoom = swapper.load_model("baseapp_chats", "ChatRoom") | ||
|
|
||
| participant_qs = ChatRoomParticipant.objects.filter(profile__user=user) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether the abstract ChatRoomParticipant defines a `profile` FK,
# and how downstream concrete models relate participants to users.
rg -nP --type=py -C3 '\bclass\s+(Abstract)?(ChatRoom)?Participant\b'
rg -nP --type=py -C2 '\bprofile\s*=\s*models\.(ForeignKey|OneToOneField)'Repository: silverlogic/baseapp-backend
Length of output: 5113
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Show the queryset in question
echo "---- baseapp_chats/services.py around line 29 ----"
sed -n '1,120p' baseapp_chats/services.py | nl -ba | sed -n '1,120p'
echo "---- baseapp_chats/services.py exact line 29 ----"
nl -ba baseapp_chats/services.py | sed -n '20,40p'
# 2) Inspect the participant models and their profile/user relations
echo "---- baseapp_chats/models.py around AbstractChatRoomParticipant (and nearby ChatRoomParticipant) ----"
nl -ba baseapp_chats/models.py | sed -n '160,320p'
# 3) Inspect Profile model: find where it defines the `user` link
echo "---- baseapp_profiles/models.py: find `class Profile` and the `user` field ----"
rg -n --type=py 'class Profile\b|verbose_name=_\("user"\)|models\.(OneToOneField|ForeignKey)\(.*user' baseapp_profiles/models.py
rg -n --type=py 'user\s*=' baseapp_profiles/models.py
# print around the likely user field location
echo "---- baseapp_profiles/models.py around discovered locations ----"
nl -ba baseapp_profiles/models.py | sed -n '430,560p'Repository: silverlogic/baseapp-backend
Length of output: 183
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- baseapp_chats/services.py: print lines 1-120 with line numbers (awk) ----"
awk '{printf "%5d:%s\n", NR, $0}' baseapp_chats/services.py | sed -n '1,120p'
echo "---- baseapp_chats/services.py: print lines 20-45 ----"
awk '{printf "%5d:%s\n", NR, $0}' baseapp_chats/services.py | sed -n '20,45p'
echo "---- baseapp_chats/models.py: print lines 150-320 ----"
awk '{printf "%5d:%s\n", NR, $0}' baseapp_chats/models.py | sed -n '150,320p'
echo "---- baseapp_profiles/models.py: find where Profile is defined ----"
rg -n --type=py 'class Profile\b' baseapp_profiles/models.py
echo "---- baseapp_profiles/models.py: find field definitions related to user ----"
rg -n --type=py 'models\.(OneToOneField|ForeignKey)\(.*user' baseapp_profiles/models.py || true
rg -n --type=py 'verbose_name=_\("user"\)' baseapp_profiles/models.py || true
rg -n --type=py '\buser\s*=' baseapp_profiles/models.py || true
echo "---- baseapp_profiles/models.py: print around likely user field area ----"
awk '{printf "%5d:%s\n", NR, $0}' baseapp_profiles/models.py | sed -n '450,540p'Repository: silverlogic/baseapp-backend
Length of output: 9364
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- baseapp_profiles/models.py: locate class Profile ----"
rg -n --type=py 'class Profile\b' baseapp_profiles/models.py
echo "---- baseapp_profiles/models.py: show around line 470-520 (where initial rg hinted user field) ----"
awk '{if(NR>=450 && NR<=530) printf "%5d:%s\n", NR, $0}' baseapp_profiles/models.py
echo "---- baseapp_chats/models.py: locate concrete ChatRoomParticipant ----"
rg -n --type=py 'class ChatRoomParticipant\b' baseapp_chats/models.py
echo "---- baseapp_chats/models.py: show around where ChatRoomParticipant (concrete) is defined ----"
awk '{if(NR>=1 && NR<=360) printf "%5d:%s\n", NR, $0}' baseapp_chats/models.py | sed -n '180,280p'Repository: silverlogic/baseapp-backend
Length of output: 130
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- baseapp_profiles/models.py: line count ----"
wc -l baseapp_profiles/models.py
echo "---- baseapp_profiles/models.py: list lines containing 'Profile' class definitions (limit 50) ----"
rg -n --type=py 'class .*Profile' baseapp_profiles/models.py | head -n 50 || true
echo "---- baseapp_profiles/models.py: search for the user relation field ----"
rg -n --type=py 'verbose_name=_\("user"\)' baseapp_profiles/models.py || true
rg -n --type=py '\buser\s*=' baseapp_profiles/models.py || true
rg -n --type=py 'related_name="members"' baseapp_profiles/models.py || true
rg -n --type=py 'swapper\.get_model_name\("baseapp_profiles", "Profile"\)' baseapp_profiles/models.py || true
echo "---- baseapp_profiles/models.py: print around the earlier known user-field vicinity (470-520) ----"
awk '{if(NR>=470 && NR<=520) printf "%5d:%s\n", NR, $0}' baseapp_profiles/models.pyRepository: silverlogic/baseapp-backend
Length of output: 2939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- baseapp_profiles/models.py: lines mentioning AUTH_USER_MODEL ----"
rg -n --type=py 'AUTH_USER_MODEL' baseapp_profiles/models.py
echo "---- baseapp_profiles/models.py: look for a model field named 'user' ----"
rg -n --type=py '^\s*user\s*=\s*models\.(OneToOneField|ForeignKey)' baseapp_profiles/models.py
echo "---- baseapp_profiles/models.py: print around any 'user = models.' matches ----"
rg -n --type=py '^\s*user\s*=\s*models\.(OneToOneField|ForeignKey)' baseapp_profiles/models.py | awk -F: '{print $1}' > /tmp/user_field_lines.txt
if [ -s /tmp/user_field_lines.txt ]; then
while read -r ln; do
start=$((ln-10)); end=$((ln+10));
echo "### context around line $ln ($start-$end) ###"
awk -v s="$start" -v e="$end" 'NR>=s && NR<=e {printf "%5d:%s\n", NR, $0}' baseapp_profiles/models.py
done < /tmp/user_field_lines.txt
fi
echo "---- baseapp_profiles/models.py: print around AbstractProfile (if it contains user FK) ----"
awk '{if(NR>=1 && NR<=250) printf "%5d:%s\n", NR, $0}' baseapp_profiles/models.py | sed -n '1,250p'Repository: silverlogic/baseapp-backend
Length of output: 11926
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- rg: usages of profile__user ----"
rg -n --type=py '\bprofile__user\b' .
echo "---- rg: definition of `user = models.*` within baseapp_profiles/models.py ----"
rg -n --type=py '^\s*user\s*=\s*models\.(ForeignKey|OneToOneField|ManyToManyField)' baseapp_profiles/models.py
echo "---- rg: any mention of '__owner' traversals for profiles ----"
rg -n --type=py 'profile__owner' .
echo "---- rg: chat cleanup / participant filters ----"
rg -n --type=py 'cleanup_user_participation|ChatRoomParticipant\.objects\.filter' baseapp_chats/services.py baseapp_chats -SRepository: silverlogic/baseapp-backend
Length of output: 4074
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- baseapp_chats/tests/test_chats_participation_service.py around uses ----"
awk '{printf "%5d:%s\n", NR, $0}' baseapp_chats/tests/test_chats_participation_service.py | sed -n '1,130p'
echo "---- locate concrete Profile model in testproject (or anywhere) ----"
# show any model named Profile that is not abstract
rg -n --type=py 'class Profile\s*\(' testproject . --hidden || true
echo "---- locate concrete Profile definitions by app label (baseapp_profiles) ----"
rg -n --type=py 'baseapp_profiles.*Profile' testproject . --hidden || true
echo "---- locate any model field named `user` inside a Profile model ----"
rg -n --type=py 'class .*Profile.*:\n(?:.*\n){0,80}.*\buser\s*=\s*models\.(ForeignKey|OneToOneField)' testproject . --hidden || true
echo "---- locate `user = models.*` in testproject social profiles ----"
rg -n --type=py '\buser\s*=\s*models\.(ForeignKey|OneToOneField)\b' testproject . --hidden || true
echo "---- locate swapper setting for baseapp_profiles.Profile in testproject ----"
rg -n --type=py 'swapper\.swappable_setting\("baseapp_profiles", "Profile"\)|SWAPPABLE_SETTINGS|AUTH_PROFILE|PROFILE' testproject . --hidden || trueRepository: silverlogic/baseapp-backend
Length of output: 42982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- testproject/profiles/models.py ----"
wc -l testproject/profiles/models.py
awk '{printf "%5d:%s\n", NR, $0}' testproject/profiles/models.py
echo "---- testproject/profiles/models.py: search for user/owner fields ----"
rg -n --type=py '\b(user|owner)\s*=' testproject/profiles/models.py
echo "---- baseapp_profiles/models.py: check Profile fields (AbstractProfile) for user/owner ----"
awk '{printf "%5d:%s\n", NR, $0}' baseapp_profiles/models.py | sed -n '35,130p'
echo "---- baseapp_profiles/models.py: ensure AbstractProfile has owner but not user ----"
rg -n --type=py '^class AbstractProfile\b|^\s*owner\s*=\s*models\.(ForeignKey|OneToOneField)|^\s*user\s*=\s*models\.' baseapp_profiles/models.pyRepository: silverlogic/baseapp-backend
Length of output: 1066
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "---- baseapp_profiles/models.py: any 'def user' or '`@property` def user' ----"
rg -n --type=py 'def\s+user\b' baseapp_profiles/models.py
rg -n --type=py '`@property`\s*\n\s*def\s+user\b' baseapp_profiles/models.py || true
echo "---- baseapp_profiles/models.py: any mention of 'profile__user' or '__user' traversal guidance ----"
rg -n --type=py 'profile__user' baseapp_profiles/models.py || true
rg -n --type=py '\b__user\b' baseapp_profiles/models.py || true
echo "---- inspect testproject/profiles/models.py for any user attribute ----"
rg -n --type=py 'user\b' testproject/profiles/models.py
echo "---- baseapp_profiles/models.py: inspect around AbstractProfile class for 'user' keyword occurrences ----"
awk 'NR>=46 && NR<=120 {printf "%5d:%s\n", NR, $0}' baseapp_profiles/models.pyRepository: silverlogic/baseapp-backend
Length of output: 148
Fix the queryset filter: profile__user doesn’t match the current Profile model shape.
ChatRoomParticipant still has a profile FK to baseapp_profiles.Profile, but that Profile model defines owner (FK to AUTH_USER_MODEL) and does not define a user field; the only user FK lives on AbstractProfileUserRole. So ChatRoomParticipant.objects.filter(profile__user=user) will fail field resolution (and the service will not work for the default swapped profiles.Profile).
Use the correct traversal (e.g. profile__owner=user) or provide an explicit abstract-model helper like for_user(user) that hides the identity mapping.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@baseapp_chats/services.py` at line 29, The queryset uses an incorrect field
traversal: ChatRoomParticipant.objects.filter(profile__user=user) fails because
baseapp_profiles.Profile defines owner (not user); change the filter to use
profile__owner=user or replace the direct filter with a helper like
Profile.for_user(user) / ChatRoomParticipant.for_user(user) to hide the mapping;
update the call site in the service (where participant_qs is built) to use
profile__owner=user or the new for_user(user) helper so the lookup matches the
Profile model shape (reference symbols: ChatRoomParticipant, profile,
Profile.owner, for_user).
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.
Title
Refactor BaseApp Chats to Plugin Architecture and Remove Profile Coupling
User Story
The
baseapp_chatsapp needs to be aligned with the BaseApp plugin architecture and updated to remove its dependency on profile-based models. The current implementation relies on direct foreign keys to Profile and tightly coupled structures, which limits extensibility and prevents reuse across different projects.This effort refactors the chats system to use a decoupled identity approach (e.g., DocumentId or project-owned models), integrates it into the plugin architecture, and standardizes cross-app interactions through shared services.
Acceptance Criteria
This story is done when:
Concrete models in
baseapp_chats(e.g., Message, ChatRoom, ChatRoomParticipant, UnreadMessageCount, MessageStatus) are reviewed and removed if they should be extendable, so that projects can define their own implementations.The migrations folder is removed if no concrete models remain, so that unnecessary database artifacts are eliminated.
All coupling to
baseapp_profiles(e.g., FKs via swapper in chat-related models) is removed or replaced with a decoupled identity reference (such as DocumentId or project-owned models), so that the chats system is independent.The app is registered as a plugin and contributes its GraphQL queries, mutations, subscriptions, and settings via the registry, so that configuration is centralized and dynamic.
The root GraphQL schema consumes chats contributions via registry getters only, so that no direct imports of chats GraphQL modules are required.
Any URLs exposed by the chats app are contributed via plugin callbacks, so that routing is managed through the registry.
GraphQL shared interfaces are resolved by name using the shared interface registry, so that no direct cross-package imports are required.
The chats app consumes notifications via the shared service registry (e.g.,
shared_service_registry.get_service("notifications")), so that it no longer relies on direct imports frombaseapp_notifications.No new direct cross-app dependencies are introduced, so that the chats app remains modular and reusable.
A shared service is introduced only if reusable chat functionality (e.g., unread message counts) needs to be exposed to other apps, so that inter-app communication remains intentional and standardized.
All plugin registrations and optional shared services are initialized in
AppConfig.ready(), so that the app bootstraps consistently.The final structure adheres to plugin architecture principles and allows the chats system to be reused independently of other baseapp modules.
Summary by CodeRabbit
New Features
Improvements
Refactor