Fix Chat Room Bugs When Changing Rooms and in Direct Messages #160
Conversation
Reviewer's Guide by SourceryThis pull request addresses bugs in the chat room functionality, specifically when changing rooms and handling direct messages. The changes include adding logic to handle event_id in the server-side notification publishing, fixing state management issues in the client-side chat store, and ensuring notifications are correctly managed and displayed. File-Level Changes
Tips
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @odkhang - I've reviewed your changes and they look great!
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Testing: all looks good
- 🟡 Complexity: 1 issue found
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment to tell me if it was helpful.
| if event_id: | ||
| async with aredis() as redis: | ||
| read_pointer = await redis.hget(f"chat:read:{self.consumer.user.id}", body["data"]["event"]["channel"]) | ||
| read_pointer = int(read_pointer.decode()) if read_pointer else 0 |
There was a problem hiding this comment.
issue (bug_risk): Handle potential decoding errors
Consider adding error handling for the decode method in case the data is not in the expected format.
| notification_counts = await database_sync_to_async( | ||
| self.service.get_notification_counts | ||
| )(self.consumer.user.id) |
There was a problem hiding this comment.
suggestion: Consider using a more descriptive variable name
The variable notification_counts could be more descriptive to indicate what kind of notifications it is counting, e.g., unread_notification_counts.
| notification_counts = await database_sync_to_async( | |
| self.service.get_notification_counts | |
| )(self.consumer.user.id) | |
| unread_notification_counts = await database_sync_to_async( | |
| self.service.get_unread_notification_counts | |
| )(self.consumer.user.id) |
| self.service.get_notification_counts | ||
| )(self.consumer.user.id) | ||
| await self.consumer.send_json(["chat.notification_counts", notification_counts]) | ||
| return |
There was a problem hiding this comment.
nitpick: Unnecessary return statement
The return statement here is redundant since the function will exit after this block anyway.
| @@ -349,6 +349,18 @@ | |||
|
|
|||
| @event("notification") | |||
| async def publish_notification(self, body): | |||
| event_id = body.get("data", {}).get("event", {}).get("event_id") | |||
There was a problem hiding this comment.
issue (complexity): Consider refactoring the code to reduce nesting and encapsulate specific logic into helper methods.
The new code introduces increased complexity due to additional nested conditionals, asynchronous context management, multiple return points, and potential error handling issues. To improve readability and maintainability, consider refactoring the code to reduce nesting and encapsulate specific logic into helper methods. Here's a suggested refactor:
@event("notification")
async def publish_notification(self, body):
event_data = body.get("data", {}).get("event", {})
event_id = event_data.get("event_id")
channel = event_data.get("channel")
if event_id and channel:
read_pointer = await self._get_read_pointer(channel)
if read_pointer is not None and read_pointer >= event_id:
if await self.service.remove_notifications(self.consumer.user.id, self.channel_id, read_pointer):
await self._send_notification_counts()
return
await self.consumer.send_json(["chat.notification", body.get("data")])
async def _get_read_pointer(self, channel):
async with aredis() as redis:
read_pointer = await redis.hget(f"chat:read:{self.consumer.user.id}", channel)
return int(read_pointer.decode()) if read_pointer else None
async def _send_notification_counts(self):
notification_counts = await database_sync_to_async(
self.service.get_notification_counts
)(self.consumer.user.id)
await self.consumer.send_json(["chat.notification_counts", notification_counts])This refactor reduces complexity by introducing helper methods, making the main method cleaner and easier to follow. It also adheres to the Single Responsibility Principle, making future maintenance simpler.
Summary by Sourcery
This pull request addresses bugs related to chat room notifications and direct messages. It ensures notifications are correctly updated when changing rooms and marks direct message notifications as read. Additionally, it enhances the handling of direct message channel names and introduces a new state property to track message fetching status.