feat: functionality to view unread messages in chat #2611
Conversation
…into chat-feature
…o reply-functionality
… into unread-msgs
WalkthroughThe pull request introduces a new field Changes
Assessment against linked issues
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
Our Pull Request Approval ProcessWe have these basic policies to make the approval process smoother for our volunteer team. Testing Your CodePlease make sure your code passes all tests. Our test code coverage system will fail if these conditions occur:
The process helps maintain the overall reliability of the code base and is a prerequisite for getting your PR approved. Assigned reviewers regularly review the PR queue and tend to focus on PRs that are passing. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
CONTRIBUTING.mdRead our CONTRIBUTING.md file. Most importantly:
Other
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2611 +/- ##
========================================
Coverage 98.44% 98.44%
========================================
Files 342 343 +1
Lines 16734 16800 +66
Branches 2408 2414 +6
========================================
+ Hits 16473 16538 +65
- Misses 258 259 +1
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (10)
src/resolvers/Query/chatsByUserId.ts (1)
25-30: Consider adding index for chat sorting.Since we're sorting on
updatedAt, consider adding an index to optimize query performance, especially as the number of chats grows.Add this index to your Chat model:
// In your Chat model definition { timestamps: true, indexes: [ { updatedAt: -1 } ] }src/resolvers/Mutation/sendMessageToChat.ts (1)
Line range hint
1-92: Consider architectural improvements for better scalability.The current implementation of storing unseen message counts as a JSON string has several scalability concerns:
- JSON parsing/stringifying on every message operation is inefficient
- Storing counts as a JSON string prevents using MongoDB's native querying and indexing
- No apparent limit on message history or unseen count size
Consider these architectural improvements:
- Store unseen messages as native MongoDB fields:
interface Chat { unseenMessages: { [userId: string]: number; }; // ... other fields }
- Use MongoDB schema validation to ensure data integrity:
unseenMessages: { bsonType: "object", patternProperties: { "^[0-9a-fA-F]{24}$": { bsonType: "int" } } }
- Add indexes for common queries:
db.chats.createIndex({ "unseenMessages.$**": 1 });Would you like me to create a separate issue to track these architectural improvements?
src/resolvers/Mutation/createChat.ts (1)
Line range hint
39-47: Enhance error reporting for missing users.While the user existence check is efficient, consider enhancing the error message to specify which user IDs weren't found. This would improve debugging and user experience.
const userExists = await User.find({ _id: { $in: args.data.userIds }, }).lean(); if (userExists && userExists.length !== args.data.userIds.length) { + const foundUserIds = userExists.map(user => user._id.toString()); + const missingUserIds = args.data.userIds.filter(id => !foundUserIds.includes(id)); throw new errors.NotFoundError( - requestContext.translate(USER_NOT_FOUND_ERROR.MESSAGE), + requestContext.translate(USER_NOT_FOUND_ERROR.MESSAGE) + `: ${missingUserIds.join(', ')}`, USER_NOT_FOUND_ERROR.CODE, USER_NOT_FOUND_ERROR.PARAM, ); }src/models/Chat.ts (1)
Line range hint
1-108: Consider implementing a message tracking strategy.The current implementation tracks unseen messages at the chat level. For better scalability and performance, consider:
- Adding an index on
unseenMessagesByUsersif you'll be querying by this field- Implementing a cleanup strategy to prevent the JSON object from growing indefinitely
- Adding a maximum limit to the number of unseen messages tracked per user
tests/resolvers/Mutation/sendMessageToChat.spec.ts (1)
Line range hint
142-185: Add test cases for unseen messages functionality.The successful message sending test should verify that the
unseenMessagesByUserscount is incremented correctly. Consider adding assertions to verify:
- The count increases for the recipient but not the sender
- The JSON structure remains valid after updates
Here's a suggested addition to the existing test:
const sendMessageToChatPayload = await sendMessageToChatResolver?.( {}, args, context, ); + // Verify the message payload expect(sendMessageToChatPayload).toEqual( expect.objectContaining({ chatMessageBelongsTo: testChat._id, sender: testUsers[0]?._id, messageContent: "messageContent", }), ); + // Verify unseen messages count + const updatedChat = await Chat.findById(testChat._id); + const unseenMessages = JSON.parse(updatedChat.unseenMessagesByUsers); + expect(unseenMessages[testUsers[0]?.id]).toBe(0); // Sender's count unchanged + expect(unseenMessages[testUsers[1]?.id]).toBe(1); // Recipient's count increasedWould you like me to help implement these test cases?
tests/resolvers/Mutation/markChatMessagesAsRead.spec.ts (2)
65-66: Replace hardcoded timestamps with dynamic values.Using hardcoded timestamps makes the tests less maintainable and could cause issues if time-based logic is added later.
- createdAt: "23456789", - updatedAt: "23456789", + createdAt: new Date().toISOString(), + updatedAt: new Date().toISOString(),
78-169: Add test cases for edge scenarios.The test suite is missing important edge cases:
- Marking messages as read for a user not in the chat
- Handling invalid unseenMessagesByUsers data
- Concurrent marking of messages
Would you like me to help generate these additional test cases?
src/typeDefs/mutations.ts (1)
253-254: The mutation definition looks good but could be enhanced.The implementation follows the project's conventions and includes proper authentication. However, consider these enhancements:
- Define specific error types for better error handling:
- markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat @auth + markChatMessagesAsRead(chatId: ID!, userId: ID!): MarkChatMessagesAsReadResponse @auth + type MarkChatMessagesAsReadResponse { + chat: Chat + success: Boolean! + error: MarkChatMessagesAsReadError + } + type MarkChatMessagesAsReadError { + code: MarkChatMessagesAsReadErrorCode! + message: String! + } + enum MarkChatMessagesAsReadErrorCode { + CHAT_NOT_FOUND + USER_NOT_IN_CHAT + UNAUTHORIZED + }
- Consider adding rate limiting to prevent abuse:
- markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat @auth + markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat @auth @rateLimit(window: "5m", max: 100)src/typeDefs/types.ts (1)
745-745: Add documentation for theunseenMessagesByUsersfield.The field purpose and structure are not immediately clear. Please add GraphQL documentation using triple quotes to explain:
- The expected JSON structure
- How the field is used to track unread messages
- Any constraints or assumptions
Example documentation:
+ """ + Tracks the number of unseen messages per user in the chat. + Structure: { userId: number of unseen messages } + """ unseenMessagesByUsers: JSONschema.graphql (1)
209-209: Add descriptions to the new chat-related schema elements.Missing descriptions make it harder for other developers to understand the purpose and behavior of these fields.
Add descriptions using triple-quote blocks:
type Chat { """ Tracks which messages are unseen by which users in the chat. The structure maps user IDs to their last seen message timestamp. """ unseenMessagesByUsers: JSON } type Mutation { """ Marks all messages in a chat as read for a specific user. Returns the updated chat or error if the operation fails. """ markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat }Also applies to: 1118-1118
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (13)
- schema.graphql (2 hunks)
- src/models/Chat.ts (2 hunks)
- src/resolvers/Mutation/createChat.ts (2 hunks)
- src/resolvers/Mutation/index.ts (2 hunks)
- src/resolvers/Mutation/markChatMessagesAsRead.ts (1 hunks)
- src/resolvers/Mutation/sendMessageToChat.ts (2 hunks)
- src/resolvers/Query/chatsByUserId.ts (2 hunks)
- src/typeDefs/mutations.ts (1 hunks)
- src/typeDefs/types.ts (1 hunks)
- src/types/generatedGraphQLTypes.ts (5 hunks)
- tests/helpers/chat.ts (1 hunks)
- tests/resolvers/Mutation/markChatMessagesAsRead.spec.ts (1 hunks)
- tests/resolvers/Mutation/sendMessageToChat.spec.ts (1 hunks)
🔇 Additional comments (19)
src/resolvers/Query/chatsByUserId.ts (2)
3-3: LGTM!The type import is correctly added and properly scoped.
25-30: Verify the impact on unread messages feature.The query changes look good, but let's ensure the sorting doesn't affect the unread messages functionality.
Let's verify the chat model structure and related tests:
✅ Verification successful
No impact on unread messages functionality
The sorting of chats doesn't affect the unread messages feature because:
- The
unseenMessagesByUsersis stored as a JSON field in the Chat model and is managed independently through separate mutations:
sendMessageToChat: Increments unread count for other usersmarkChatMessagesAsRead: Resets unread count for the current user- The sorting only affects the order of returned chats and doesn't modify any chat data
- The test coverage confirms the query returns the same data with or without sorting
The changes are safe to proceed with.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify chat model structure and related test coverage # Check if Chat model includes unseenMessagesByUsers field echo "Checking Chat model structure..." rg -A 5 "unseenMessagesByUsers" "src/models/" # Check if there are tests covering the interaction between sorting and unread messages echo "Checking test coverage..." rg -A 5 "sort.*unread|unread.*sort" "tests/"Length of output: 668
Script:
#!/bin/bash # Let's check how unseenMessagesByUsers is used in the codebase echo "Checking usage of unseenMessagesByUsers..." rg -A 5 "unseenMessagesByUsers" "src/" # Let's also check if there are any specific unread message related functions echo "Checking unread message related functions..." ast-grep --pattern 'function $FUNC($$$) { $$$ unseenMessagesByUsers $$$ }' # Check if there are any tests for chatsByUserId echo "Checking chatsByUserId tests..." rg -A 10 "chatsByUserId" "tests/"Length of output: 8904
src/resolvers/Mutation/markChatMessagesAsRead.ts (1)
1-63: Verify related schema changes for unseenMessagesByUsers.The implementation assumes the existence of
unseenMessagesByUsersin the Chat model. Let's verify the related schema changes.✅ Verification successful
The schema verification shows that
unseenMessagesByUsersis properly defined in both the Mongoose model (src/models/Chat.ts) and GraphQL type definitions (src/typeDefs/types.ts). Let me verify one more aspect - the GraphQL mutation definition to ensure it's properly exposed in the API.
Schema implementation is correctly aligned with the functionality
The verification confirms that:
unseenMessagesByUsersis properly defined in the Chat model schema as a required JSON field- GraphQL type definitions include the field in the Chat type
- The mutation
markChatMessagesAsReadis properly exposed in the GraphQL API with authentication🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the Chat model schema and GraphQL type definitions include unseenMessagesByUsers # Check Chat model schema echo "Checking Chat model schema..." rg -A 5 "unseenMessagesByUsers" "src/models/" # Check GraphQL type definitions echo "Checking GraphQL type definitions..." rg -A 5 "unseenMessagesByUsers" "src/typeDefs/"Length of output: 913
Script:
#!/bin/bash # Check GraphQL mutation definition echo "Checking GraphQL mutation definition..." rg "markChatMessagesAsRead" "src/typeDefs/"Length of output: 221
src/resolvers/Mutation/createChat.ts (2)
Line range hint
53-79: Consider performance implications of JSON storage.The current implementation stores unseenMessagesByUsers as a JSON string:
Performance Considerations:
- JSON parsing/stringifying overhead on every read/write
- Limited query capabilities on the JSON field
- Potential atomicity issues with concurrent updates
Alternative Approaches:
- Consider using a separate collection for unread message counts
- Use MongoDB's native object storage instead of JSON strings
- Implement optimistic locking for concurrent updates
#!/bin/bash # Check if sendMessageToChat updates unseenMessagesByUsers rg -A 5 "sendMessageToChat.*unseenMessagesByUsers" # Check for potential race condition handling rg -A 5 "findOneAndUpdate.*unseenMessagesByUsers"
71-71: Verify unseenMessagesByUsers is defined before creating chat.Add a validation check to ensure the unseenMessagesByUsers field is properly initialized before creating the chat.
Also applies to: 79-79
src/models/Chat.ts (2)
108-108:⚠️ Potential issueResolve timestamp handling inconsistency.
The schema has
timestamps: falsebut includes requiredcreatedAtandupdatedAtfields. This creates a potential risk as timestamps won't be automatically managed by Mongoose anymore.Consider either:
- Keeping
timestamps: trueto let Mongoose handle these fields automatically, or- Removing the timestamp fields if they're not needed
Let's verify timestamp usage in mutations:
#!/bin/bash # Search for places where Chat documents are created or updated ast-grep --pattern 'Chat.create|Chat.update|Chat.findOneAndUpdate'
102-105: Verify migration strategy for existing chat documents.Adding a required field without a default value could cause issues with existing chat documents in the database.
Let's check if there are existing chat documents that need migration:
tests/helpers/chat.ts (1)
Line range hint
127-137: Verify the impact of missing unseenMessagesByUsers in chat creation helpers.The
createTestChatwithUsersandcreateTestMessageForMultipleUserfunctions don't initialize theunseenMessagesByUsersfield. This might affect tests related to unread messages functionality.Let's check if any tests rely on these helpers:
After verifying the usage, update these helpers to include
unseenMessagesByUsersinitialization for consistency with the new chat schema.Also applies to: 139-149
tests/resolvers/Mutation/sendMessageToChat.spec.ts (1)
68-71: LGTM: Proper initialization of unseenMessagesByUsers.The initialization of
unseenMessagesByUserswith zero counts for both users is correct and matches the expected initial state.tests/resolvers/Mutation/markChatMessagesAsRead.spec.ts (3)
1-28: LGTM! Imports and type declarations are well-structured.The imports cover all necessary dependencies and the type declarations are properly defined.
74-82: LGTM! Test cleanup is properly implemented.The afterAll and afterEach hooks ensure proper cleanup of database connections and mocks.
67-70: Consider storing unseenMessagesByUsers as a Map or plain object.Storing the unseenMessagesByUsers as stringified JSON could lead to parsing/stringifying overhead and potential errors. Consider using a native MongoDB data structure.
Let's check how unseenMessagesByUsers is used across the codebase:
src/resolvers/Mutation/index.ts (2)
88-88: LGTM! Clean import statement.The import follows the established pattern and is correctly placed alphabetically.
241-241: LGTM! Mutation resolver properly registered.The mutation resolver is correctly added to the Mutation object and follows the alphabetical ordering convention.
Let's verify the mutation implementation and its integration:
✅ Verification successful
Mutation implementation and integration verified successfully
The verification confirms:
- Mutation implementation exists with proper error handling for chat and user existence
- Schema defines the mutation with correct parameters (chatId, userId)
- Comprehensive test coverage including error cases and successful execution
- Implementation correctly updates the
unseenMessagesByUserscount for the current user🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the mutation implementation and schema definition # Test 1: Check if the mutation implementation file exists echo "Checking mutation implementation..." fd "markChatMessagesAsRead.ts" src/resolvers/Mutation # Test 2: Verify the mutation is properly defined in the schema echo "Checking GraphQL schema definition..." rg "markChatMessagesAsRead.*:.*Chat.*@auth" -g "*.graphql" # Test 3: Check for test coverage echo "Checking test coverage..." fd "markChatMessagesAsRead.spec.ts" tests/Length of output: 489
Script:
#!/bin/bash # Let's verify the implementation details and schema definition # Check the mutation implementation echo "=== Mutation Implementation ===" cat src/resolvers/Mutation/markChatMessagesAsRead.ts # Check the schema definition echo -e "\n=== Schema Definition ===" rg "markChatMessagesAsRead" -A 3 -g "*.graphql" # Check the test implementation echo -e "\n=== Test Implementation ===" cat tests/resolvers/Mutation/markChatMessagesAsRead.spec.tsLength of output: 7392
src/typeDefs/mutations.ts (1)
253-254: Verify Chat type integration.Let's ensure the Chat type properly defines the
unseenMessagesByUsersfield mentioned in the AI summary.✅ Verification successful
Chat type integration is properly defined
The
unseenMessagesByUsersfield is correctly defined in the Chat type as a JSON scalar type, which aligns with its usage in the mutation. The field is consistently used across the codebase in relevant files including models, resolvers, and tests.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify Chat type definition includes unseenMessagesByUsers field # Expected: Find the Chat type definition with the unseenMessagesByUsers field echo "Searching for Chat type definition..." rg -l 'type\s+Chat\s*\{' --type ts echo "Checking unseenMessagesByUsers field in Chat type..." rg 'unseenMessagesByUsers.*:.*JSON' --type tsLength of output: 1234
Script:
#!/bin/bash # Let's check the complete Chat type definition echo "Checking complete Chat type definition..." rg -A 20 'type\s+Chat\s*\{' src/typeDefs/types.tsLength of output: 573
src/typeDefs/types.ts (1)
745-745: Verify the implementation details across the codebase.Let's ensure the field is properly integrated with the chat system.
✅ Verification successful
Let me gather more specific information about the implementation details.
Field is properly integrated with the chat system
The
unseenMessagesByUsersfield is correctly implemented across the codebase with proper:
- Initialization in
createChat.tswith zero unread messages for all users- Updates in
sendMessageToChat.tsto increment counters for recipients- Reset functionality in
markChatMessagesAsRead.tswhen messages are read- Schema definitions in models and types
- Test coverage for both sending messages and marking as read
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of unseenMessagesByUsers across the codebase # Test 1: Check how the field is initialized in chat creation echo "Checking chat creation..." rg -l "unseenMessagesByUsers.*createChat" # Test 2: Check how the field is updated when messages are sent echo "Checking message handling..." rg -l "unseenMessagesByUsers.*sendMessage" # Test 3: Check how the field is updated when messages are read echo "Checking mark as read functionality..." rg -l "unseenMessagesByUsers.*markChatMessagesAsRead" # Test 4: Look for any test coverage echo "Checking test coverage..." fd -e spec.ts -e test.ts | xargs rg "unseenMessagesByUsers"Length of output: 779
Script:
#!/bin/bash # Let's examine the actual implementation in the test files and related resolvers # Check the test implementations echo "=== Test Implementation Details ===" rg -A 10 "unseenMessagesByUsers" tests/resolvers/Mutation/sendMessageToChat.spec.ts echo "\n=== Mark as Read Test Details ===" rg -A 10 "unseenMessagesByUsers" tests/resolvers/Mutation/markChatMessagesAsRead.spec.ts # Look for resolver implementations echo "\n=== Resolver Implementations ===" fd -e ts | grep -v "test\|spec" | xargs rg -A 5 "unseenMessagesByUsers" # Check for any GraphQL schema definitions echo "\n=== Schema Definitions ===" fd -e graphql | xargs rg "unseenMessagesByUsers"Length of output: 6561
src/types/generatedGraphQLTypes.ts (3)
282-282: LGTM! Added unseenMessagesByUsers field to Chat typeThe field type
Maybe<JSON>is appropriate for storing a dynamic mapping of user IDs to unseen message counts.
1571-1574: LGTM! Added markChatMessagesAsRead mutation argumentsThe required
chatIdanduserIdarguments of typeIDare appropriate for identifying the chat and user.
4290-4290: LGTM! Added markChatMessagesAsRead resolverThe resolver return type
Maybe<Chat>is appropriate as it may return null if the chat is not found.
| const sort = { | ||
| updatedAt: -1, | ||
| } as | ||
| | string | ||
| | { [key: string]: SortOrder | { $meta: unknown } } | ||
| | [string, SortOrder][] | ||
| | null | ||
| | undefined; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider simplifying the sort configuration.
While the current implementation works, we can make it more maintainable and readable.
Consider this simplified version:
- const sort = {
- updatedAt: -1,
- } as
- | string
- | { [key: string]: SortOrder | { $meta: unknown } }
- | [string, SortOrder][]
- | null
- | undefined;
+ const sort: { [key: string]: SortOrder } = {
+ updatedAt: SortOrder.DESC
+ };This change:
- Uses the
SortOrderenum for better readability - Simplifies the type definition to match the actual usage
- Maintains the same functionality while being more explicit
📝 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.
| const sort = { | |
| updatedAt: -1, | |
| } as | |
| | string | |
| | { [key: string]: SortOrder | { $meta: unknown } } | |
| | [string, SortOrder][] | |
| | null | |
| | undefined; | |
| const sort: { [key: string]: SortOrder } = { | |
| updatedAt: SortOrder.DESC | |
| }; |
| const currentUserExists = await User.exists({ | ||
| _id: context.userId, | ||
| }).lean(); | ||
|
|
||
| if (!currentUserExists) { | ||
| throw new errors.NotFoundError( | ||
| requestContext.translate(USER_NOT_FOUND_ERROR.MESSAGE), | ||
| USER_NOT_FOUND_ERROR.CODE, | ||
| USER_NOT_FOUND_ERROR.PARAM, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Verify user's authorization to access the chat.
The code only checks if the user exists but doesn't verify if they are a member of the chat. This could lead to unauthorized access.
Add authorization check:
if (!currentUserExists) {
throw new errors.NotFoundError(
requestContext.translate(USER_NOT_FOUND_ERROR.MESSAGE),
USER_NOT_FOUND_ERROR.CODE,
USER_NOT_FOUND_ERROR.PARAM,
);
}
+
+ // Verify user is a member of the chat
+ if (!chat.users?.includes(context.userId)) {
+ throw new errors.UnauthorizedError(
+ requestContext.translate("User is not a member of this chat"),
+ "UNAUTHORIZED_CHAT_ACCESS"
+ );
+ }Committable suggestion was skipped due to low confidence.
| const unseenMessagesByUsers = JSON.parse( | ||
| chat.unseenMessagesByUsers as unknown as string, | ||
| ); | ||
|
|
There was a problem hiding this comment.
Add error handling for JSON parsing.
The JSON.parse operation could throw if the data is malformed. Also, the type casting looks suspicious.
Add error handling and proper typing:
- const unseenMessagesByUsers = JSON.parse(
- chat.unseenMessagesByUsers as unknown as string,
- );
+ let unseenMessagesByUsers: Record<string, number>;
+ try {
+ unseenMessagesByUsers = JSON.parse(
+ typeof chat.unseenMessagesByUsers === 'string'
+ ? chat.unseenMessagesByUsers
+ : JSON.stringify(chat.unseenMessagesByUsers)
+ );
+ } catch (error) {
+ throw new errors.ValidationError(
+ requestContext.translate("Invalid chat data format"),
+ "INVALID_CHAT_DATA"
+ );
+ }📝 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.
| const unseenMessagesByUsers = JSON.parse( | |
| chat.unseenMessagesByUsers as unknown as string, | |
| ); | |
| let unseenMessagesByUsers: Record<string, number>; | |
| try { | |
| unseenMessagesByUsers = JSON.parse( | |
| typeof chat.unseenMessagesByUsers === 'string' | |
| ? chat.unseenMessagesByUsers | |
| : JSON.stringify(chat.unseenMessagesByUsers) | |
| ); | |
| } catch (error) { | |
| throw new errors.ValidationError( | |
| requestContext.translate("Invalid chat data format"), | |
| "INVALID_CHAT_DATA" | |
| ); | |
| } |
| } | ||
| }); | ||
|
|
||
| it(`creates the directChatMessage and returns it`, async () => { |
There was a problem hiding this comment.
Fix misleading test description.
The test description "creates the directChatMessage" is incorrect as this test verifies marking messages as read.
- it(`creates the directChatMessage and returns it`, async () => {
+ it(`marks messages as read and returns updated chat`, async () => {📝 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.
| it(`creates the directChatMessage and returns it`, async () => { | |
| it(`marks messages as read and returns updated chat`, async () => { |
| it(`creates the directChatMessage and returns it`, async () => { | ||
| await Chat.updateOne( | ||
| { | ||
| _id: testChat._id, | ||
| }, | ||
| { | ||
| $push: { | ||
| users: testUsers[0]?._id, | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Remove unnecessary chat update in the success test case.
The test unnecessarily adds a user that should already be present in the chat from the setup. This makes the test harder to understand and could mask potential issues.
- await Chat.updateOne(
- {
- _id: testChat._id,
- },
- {
- $push: {
- users: testUsers[0]?._id,
- },
- },
- );📝 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.
| it(`creates the directChatMessage and returns it`, async () => { | |
| await Chat.updateOne( | |
| { | |
| _id: testChat._id, | |
| }, | |
| { | |
| $push: { | |
| users: testUsers[0]?._id, | |
| }, | |
| }, | |
| ); | |
| it(`creates the directChatMessage and returns it`, async () => { |
| @@ -742,6 +742,7 @@ export const types = gql` | |||
| admins: [User] | |||
| lastMessageId: String | |||
| image: String | |||
| unseenMessagesByUsers: JSON | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a strict type instead of JSON.
Using the JSON scalar type loses GraphQL's type safety benefits. Consider creating a dedicated type to represent unseen messages:
+ type UnseenMessages {
+ userId: ID!
+ count: Int!
+ }
type Chat {
# ... other fields ...
- unseenMessagesByUsers: JSON
+ unseenMessagesByUsers: [UnseenMessages!]
}This provides several benefits:
- Type safety and validation at the GraphQL schema level
- Better documentation through the type system
- Improved developer experience with IDE support
Committable suggestion was skipped due to low confidence.
| @@ -206,6 +206,7 @@ type Chat { | |||
| messages: [ChatMessage] | |||
| name: String | |||
| organization: Organization | |||
| unseenMessagesByUsers: JSON | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a more type-safe structure for unseenMessagesByUsers.
The JSON type provides flexibility but lacks type safety. Consider creating a dedicated type to represent unseen messages data.
- unseenMessagesByUsers: JSON
+ unseenMessagesByUsers: UnseenMessagesAdd these type definitions:
type UnseenMessages {
"""Map of user IDs to their last seen message timestamp"""
userToLastSeen: [UserLastSeen!]
}
type UserLastSeen {
userId: ID!
lastSeenAt: DateTime!
}| @@ -1114,6 +1115,7 @@ | |||
| likePost(id: ID!): Post | |||
| login(data: LoginInput!): AuthData! | |||
| logout: Boolean! | |||
| markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat | |||
There was a problem hiding this comment.
Add error handling to markChatMessagesAsRead mutation.
The mutation should handle cases like invalid chat/user IDs and unauthorized access.
- markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat
+ markChatMessagesAsRead(chatId: ID!, userId: ID!): MarkChatMessagesAsReadPayload!
Add these type definitions:
type MarkChatMessagesAsReadPayload {
chat: Chat
userErrors: [MarkChatMessagesAsReadError!]!
}
union MarkChatMessagesAsReadError = ChatNotFoundError | UserNotFoundError | UserNotAuthorizedError
type ChatNotFoundError implements Error {
message: String!
}
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
| if (unseenMessagesByUsers[context.userId] !== undefined) { | ||
| unseenMessagesByUsers[context.userId] = 0; | ||
| } |
There was a problem hiding this comment.
Add type validation for unseenMessagesByUsers structure.
The code assumes unseenMessagesByUsers is a valid object with number values, but there's no validation of its structure.
Add type validation:
+ // Validate unseenMessagesByUsers structure
+ if (typeof unseenMessagesByUsers !== 'object' || unseenMessagesByUsers === null) {
+ throw new errors.ValidationError(
+ requestContext.translate('Invalid unseenMessagesByUsers format'),
+ 'INVALID_MESSAGE_COUNT_FORMAT'
+ );
+ }
+
+ // Validate the specific user's message count
+ if (unseenMessagesByUsers[context.userId] !== undefined &&
+ typeof unseenMessagesByUsers[context.userId] !== 'number') {
+ throw new errors.ValidationError(
+ requestContext.translate('Invalid message count format'),
+ 'INVALID_MESSAGE_COUNT_TYPE'
+ );
+ }
+
if (unseenMessagesByUsers[context.userId] !== undefined) {📝 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.
| if (unseenMessagesByUsers[context.userId] !== undefined) { | |
| unseenMessagesByUsers[context.userId] = 0; | |
| } | |
| // Validate unseenMessagesByUsers structure | |
| if (typeof unseenMessagesByUsers !== 'object' || unseenMessagesByUsers === null) { | |
| throw new errors.ValidationError( | |
| requestContext.translate('Invalid unseenMessagesByUsers format'), | |
| 'INVALID_MESSAGE_COUNT_FORMAT' | |
| ); | |
| } | |
| // Validate the specific user's message count | |
| if (unseenMessagesByUsers[context.userId] !== undefined && | |
| typeof unseenMessagesByUsers[context.userId] !== 'number') { | |
| throw new errors.ValidationError( | |
| requestContext.translate('Invalid message count format'), | |
| 'INVALID_MESSAGE_COUNT_TYPE' | |
| ); | |
| } | |
| if (unseenMessagesByUsers[context.userId] !== undefined) { | |
| unseenMessagesByUsers[context.userId] = 0; | |
| } |
| const updatedChat = await Chat.findByIdAndUpdate( | ||
| { | ||
| _id: chat._id, | ||
| }, | ||
| { | ||
| $set: { | ||
| unseenMessagesByUsers: JSON.stringify(unseenMessagesByUsers), | ||
| }, | ||
| }, | ||
| ); |
There was a problem hiding this comment.
Add optimistic concurrency control to prevent race conditions.
Multiple concurrent updates to unseenMessagesByUsers could lead to lost updates.
Add version control to prevent race conditions:
const updatedChat = await Chat.findByIdAndUpdate(
- {
- _id: chat._id,
- },
+ chat._id,
{
$set: {
unseenMessagesByUsers: JSON.stringify(unseenMessagesByUsers),
},
},
+ {
+ new: true,
+ // Add version control
+ __v: chat.__v,
+ }
);
+ if (!updatedChat) {
+ throw new errors.ConflictError(
+ requestContext.translate('Chat was updated by another operation'),
+ 'CONCURRENT_MODIFICATION'
+ );
+ }Committable suggestion was skipped due to low confidence.
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
|
Please fix the conflicting files and make code rabbit approve your changes |
|
During the week of November 11, 2024 we will start a code freeze on the We have completed a project to convert the Talawa-API backend to use PostgreSQL. Work will then begin with us merging code in the Planning activities for this will be managed in our #talawa-projects slack channel. A GitHub project will be created to track specially labeled issues. We completed a similar exercise last year using a similar methodology. Starting November 12, California time no new PRs will be accepted against the There are some GSoC project features that will need to be merged into develop. These will be the only exceptions. This activity and the post GSoC 2024 start date was announced in our #general Slack channel last month as a pinned post. |
|
This pull request did not get any activity in the past 10 days and will be closed in 180 days if no update occurs. Please verify it has no conflicts with the develop branch and rebase if needed. Mention it now if you need help or give permission to other people to finish your work. |
github-actions
bot
added
no-pr-activity
|
@disha1202 Are you still working on this? |
What kind of change does this PR introduce?
Issue Number:
Fixes #2610
Did you add tests for your changes?
Snapshots/Videos:
If relevant, did you update the documentation?
Summary
Does this PR introduce a breaking change?
Other information
Have you read the contributing guide?
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests