Add mutations to mark chat messages as read, update message, update group chat info, add user to group chat, share images in chats, and create dedicated group chats for events #2625
Conversation
…into chat-feature
…o reply-functionality
WalkthroughThe pull request introduces significant enhancements to the GraphQL schema and related functionality for chat operations. Key updates include the addition of new fields, input types, and mutations for managing chats and messages. Notably, the Changes
Assessment against linked issues
Possibly related PRs
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 ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. 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 #2625 +/- ##
===========================================
- Coverage 98.44% 98.39% -0.05%
===========================================
Files 342 348 +6
Lines 16734 17150 +416
Branches 2408 2481 +73
===========================================
+ Hits 16473 16874 +401
- Misses 258 272 +14
- Partials 3 4 +1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
🛑 Comments failed to post (80)
src/resolvers/Chat/messages.ts (1)
13-17: 🛠️ Refactor suggestion
Consider preserving message order.
The current query might return messages in a different order than specified in
parent.messages. This could affect message chronology in the chat interface.Consider maintaining the original order using MongoDB's
$lookupaggregation or post-query sorting:- let messages = await ChatMessage.find({ - _id: { - $in: parent.messages, - }, - }).lean(); + let messages = await ChatMessage.aggregate([ + { + $match: { + _id: { $in: parent.messages.map(id => new ObjectId(id)) } + } + }, + { + $addFields: { + __order: { + $indexOfArray: [parent.messages.map(id => id.toString()), { $toString: "$_id" }] + } + } + }, + { $sort: { "__order": 1 } }, + { $project: { "__order": 0 } } + ]);📝 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.let messages = await ChatMessage.aggregate([ { $match: { _id: { $in: parent.messages.map(id => new ObjectId(id)) } } }, { $addFields: { __order: { $indexOfArray: [parent.messages.map(id => id.toString()), { $toString: "$_id" }] } } }, { $sort: { "__order": 1 } }, { $project: { "__order": 0 } } ]);src/resolvers/Query/chatsByUserId.ts (2)
14-16:
⚠️ Potential issueAdd error handling for missing context.apiRootUrl
The resolver now depends on
context.apiRootUrlfor group chat images, but there's no validation to ensure it exists.Add validation at the start of the function:
export const chatsByUserId: QueryResolvers["chatsByUserId"] = async ( _parent, args, context, ) => { + if (!context.apiRootUrl) { + throw new Error('API root URL is not configured'); + }📝 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._parent, args, context, ) => { if (!context.apiRootUrl) { throw new Error('API root URL is not configured'); }
18-25: 🛠️ Refactor suggestion
Consider making sort configurable via query arguments
The sort is currently hardcoded to
updatedAt: -1. Consider making it configurable through GraphQL arguments for better flexibility.Example implementation:
+ type SortInput { + field: String + order: SortOrder + } const sort = { - updatedAt: -1, + [args.sort?.field || 'updatedAt']: args.sort?.order || -1, } asCommittable suggestion was skipped due to low confidence.
src/resolvers/Query/getGroupChatsByUserId.ts (5)
15-22: 🛠️ Refactor suggestion
Simplify sort configuration type.
The complex type assertion for the sort configuration can be simplified using a more specific type.
- const sort = { - updatedAt: -1, - } as - | string - | { [key: string]: SortOrder | { $meta: unknown } } - | [string, SortOrder][] - | null - | undefined; + const sort: { [key: string]: SortOrder } = { + updatedAt: -1 + };📝 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: { [key: string]: SortOrder } = { updatedAt: -1 };
24-28:
⚠️ Potential issueAdd user context validation.
The resolver assumes
context.userIdexists but doesn't validate it. This could lead to unexpected behavior if the context is invalid.Add validation before the database query:
+ if (!context.userId) { + throw new Error('User must be authenticated'); + } + const chats = await Chat.find({📝 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 (!context.userId) { throw new Error('User must be authenticated'); } const chats = await Chat.find({ users: context.userId, isGroup: true, }) .sort(sort)
31-36:
⚠️ Potential issueSecure image URL construction.
The current image URL construction could be vulnerable to path traversal attacks. Additionally, there's no validation of the image path format.
Add path validation and sanitization:
const chatList = chats.map((chat) => { if (chat.isGroup && chat.image) { + // Ensure image path is relative and contains no path traversal + const sanitizedImage = chat.image.replace(/^\/+/, '').replace(/\.\./g, ''); - return { ...chat, image: `${context.apiRootUrl}${chat.image}` }; + return { ...chat, image: `${context.apiRootUrl}/${sanitizedImage}` }; } return chat; });📝 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 chatList = chats.map((chat) => { if (chat.isGroup && chat.image) { // Ensure image path is relative and contains no path traversal const sanitizedImage = chat.image.replace(/^\/+/, '').replace(/\.\./g, ''); return { ...chat, image: `${context.apiRootUrl}/${sanitizedImage}` }; } return chat; });
4-12:
⚠️ Potential issueUpdate documentation to match implementation.
The documentation has several inconsistencies:
- It mentions throwing
NotFoundErrorbut this isn't implemented- References "directChats" but this resolver returns group chats
- The
@param _parenthas a dangling hyphenApply this diff to fix the documentation:
/** - * This query will fetch all the Chats for the current user from the database. + * This query will fetch all group chats for the current user from the database. * @param _parent- * @param args - An object that contains `id` of the user. - * @returns An object `directChats` that contains all direct chats of the current user. - * If the `directChats` object is null then it throws `NotFoundError` error. + * @returns An array of group chat objects that the current user is a member of. + * Returns an empty array if no group chats are found. * @remarks You can learn about GraphQL `Resolvers` * {@link https://www.apollographql.com/docs/apollo-server/data/resolvers/ | here}. */📝 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./** * This query will fetch all group chats for the current user from the database. * @param _parent- * @param args - An object that contains `id` of the user. * @returns An array of group chat objects that the current user is a member of. * Returns an empty array if no group chats are found. * @remarks You can learn about GraphQL `Resolvers` * {@link https://www.apollographql.com/docs/apollo-server/data/resolvers/ | here}. */
24-29: 🛠️ Refactor suggestion
Add pagination and error handling.
Consider these improvements for scalability and reliability:
- Implement pagination to handle large datasets efficiently
- Add error handling for the database operation
+ try { + const limit = 20; // or get from args + const skip = 0; // or get from args + const chats = await Chat.find({ users: context.userId, isGroup: true, }) .sort(sort) + .limit(limit) + .skip(skip) .lean(); + } catch (error) { + throw new Error(`Failed to fetch group chats: ${error.message}`); + }📝 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.try { const limit = 20; // or get from args const skip = 0; // or get from args const chats = await Chat.find({ users: context.userId, isGroup: true, }) .sort(sort) .limit(limit) .skip(skip) .lean(); } catch (error) { throw new Error(`Failed to fetch group chats: ${error.message}`); }src/resolvers/Query/getUnreadChatsByUserId.ts (1)
15-22: 🛠️ Refactor suggestion
Simplify sort configuration using mongoose constants.
The sort configuration can be improved for better maintainability and type safety.
Apply this diff to simplify the sort configuration:
+import { SortOrder } from "mongoose"; + +const CHAT_SORT_CONFIG = { + updatedAt: SortOrder.DESC, +} as const; + export const getUnreadChatsByUserId: QueryResolvers["getUnreadChatsByUserId"] = async (_parent, _args, context) => { - const sort = { - updatedAt: -1, - } as - | string - | { [key: string]: SortOrder | { $meta: unknown } } - | [string, SortOrder][] - | null - | undefined; + const sort = CHAT_SORT_CONFIG;📝 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.import { SortOrder } from "mongoose"; const CHAT_SORT_CONFIG = { updatedAt: SortOrder.DESC, } as const; const sort = CHAT_SORT_CONFIG;tests/resolvers/Chat/messages.spec.ts (2)
18-18:
⚠️ Potential issueAdd validation for array access.
Direct array index access could fail if
userOrgChatdoesn't have enough elements.Consider adding validation:
- testMessage = userOrgChat[3]; + if (userOrgChat.length < 4) { + throw new Error("Test chat messages not properly initialized"); + } + testMessage = userOrgChat[3];📝 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 (userOrgChat.length < 4) { throw new Error("Test chat messages not properly initialized"); } testMessage = userOrgChat[3];
47-66: 🛠️ Refactor suggestion
Optimize query and improve test description.
The test case has a few areas for improvement:
- Using
$inoperator for a single ID is inefficient- Test description could be more specific about what's being tested
Consider these improvements:
- it(`returns message`, async () => { + it(`returns specific message when filtered by message ID`, async () => { const parent = testChat?.toObject(); if (!parent) { throw new Error("Parent object is undefined."); } const messagesPayload = await messagesResolver?.( parent, {}, - { apiRootUrl: "" }, + { apiRootUrl: "https://api.example.com" }, ); const messages = await ChatMessage.find({ - _id: { - $in: testMessage?._id, - }, + _id: testMessage?._id, }).lean(); expect(messagesPayload).toEqual(messages); });📝 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(`returns specific message when filtered by message ID`, async () => { const parent = testChat?.toObject(); if (!parent) { throw new Error("Parent object is undefined."); } const messagesPayload = await messagesResolver?.( parent, {}, { apiRootUrl: "https://api.example.com" }, ); const messages = await ChatMessage.find({ _id: testMessage?._id, }).lean(); expect(messagesPayload).toEqual(messages); });tests/resolvers/Event/baseRecurringEvent.spec.ts (1)
56-56: 💡 Codebase verification
Based on the gathered evidence, I can now provide the final response:
Test coverage for chat creation is needed in the main event creation tests
The chat creation functionality is implemented in the codebase (
createEvent.tsandcreateChat.ts), but there's no test coverage for the positive case (createChat: true) in any of the event test files. This is particularly important since:
- The implementation shows chat creation logic is triggered when
createChatis true- The feature affects both regular and recurring events
- The helper code in
getEventData.tsshows chat-related state managementPlease add test coverage in
tests/resolvers/Mutation/createEvent.spec.tsto verify:
- Chat creation when
createChat: true- Chat association with the event
- Chat properties (name derived from event title)
🔗 Analysis chain
Add test coverage for chat creation.
While the test covers the case where
createChatis false, we should also add test coverage for whencreateChatis true to ensure the chat creation functionality works correctly with recurring events.Let's verify if other event test files have complete coverage:
Consider adding another test case with this structure:
it('creates a chat when createChat is true', async () => { const args: MutationCreateEventArgs = { data: { // ... existing test data ... createChat: true, }, // ... rest of the test setup ... }; // Add assertions to verify: // 1. Chat is created and associated with the event // 2. Base recurring event relationship with chat is correct });
Let me gather more context about the chat-related functionality in events to ensure our verification is thorough.
Let me check the event creation test files to see if there are any existing test cases for chat creation in other event types.
Let me check the main event creation test file for chat-related test coverage.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for createChat in event-related tests # Expected: Both true/false cases should be tested across event test files # Search for createChat in event test files rg -A 5 "createChat:" "tests/resolvers/Event/"Length of output: 985
Script:
#!/bin/bash # Check for chat-related event tests and implementations # Look for chat creation patterns in test files and source code # Search for chat-related test cases in event tests rg -B 2 -A 5 "it\(.*chat" "tests/resolvers/Event/" # Search for chat creation implementation in resolvers ast-grep --pattern 'createChat: boolean' # Look for chat creation logic rg -B 2 -A 5 "createChat.*=.*" "src/"Length of output: 1067
Script:
#!/bin/bash # Check event creation test files for chat-related test coverage # Search for test files related to event creation fd ".*event.*spec.ts$" tests/resolvers/ # Look for existing chat-related test cases in these files rg -B 3 -A 7 "it\(.*create.*event" tests/resolvers/Length of output: 12646
Script:
#!/bin/bash # Check createEvent.spec.ts for chat-related test data # Look for createChat in the main event creation test file rg -B 5 -A 10 "data: {" "tests/resolvers/Mutation/createEvent.spec.ts" # Also check the implementation to see what happens when createChat is true rg -B 5 -A 10 "createChat.*true" "src/"Length of output: 1016
tests/resolvers/RecurrenceRule/organization.spec.ts (1)
57-57: 🛠️ Refactor suggestion
Consider adding test cases for chat creation scenarios.
While the test verifies the organization resolver functionality, it would be beneficial to add test cases that explicitly verify:
- A chat is actually created when
createChatis true- No chat is created when
createChatis false- Error handling for chat creation failures
Here's a suggested test structure:
describe("event chat creation", () => { it("creates a chat when createChat is true", async () => { // ... existing test setup ... const createEventPayload = await createEventResolver?.({}, args, context); // Verify chat was created and linked to the event const chat = await Chat.findOne({ eventId: createEventPayload?.id }); expect(chat).toBeTruthy(); }); it("does not create a chat when createChat is false", async () => { const argsWithoutChat = { ...args, data: { ...args.data, createChat: false } }; const createEventPayload = await createEventResolver?.({}, argsWithoutChat, context); // Verify no chat was created const chat = await Chat.findOne({ eventId: createEventPayload?.id }); expect(chat).toBeNull(); }); });src/resolvers/Mutation/createChat.ts (1)
61-66:
⚠️ Potential issueAdd error handling and file size validation for image uploads.
The image upload implementation needs additional safeguards:
- Missing test coverage as indicated by codecov
- No file size validation
- No error handling for failed uploads
Consider adding these safety checks:
let uploadMediaFile = null; if (args.data.isGroup && args.data.image) { + // Validate image size (e.g., 5MB limit) + const base64Size = Buffer.from(args.data.image.split(',')[1], 'base64').length; + if (base64Size > 5 * 1024 * 1024) { + throw new errors.ValidationError( + 'Image size exceeds limit', + 'IMAGE_SIZE_EXCEEDED' + ); + } + + try { uploadMediaFile = await uploadEncodedImage(args.data.image); + } catch (error) { + throw new errors.ValidationError( + 'Failed to upload image', + 'IMAGE_UPLOAD_FAILED' + ); + } args.data.image = uploadMediaFile; }Would you like me to help generate test cases for the image upload functionality?
Committable suggestion was skipped due to low confidence.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 64-65: src/resolvers/Mutation/createChat.ts#L64-L65
Added lines #L64 - L65 were not covered by teststests/resolvers/Query/getUnreadChatsByUserId.spec.ts (3)
58-63: 🛠️ Refactor suggestion
Refactor duplicated filtering logic.
The chat filtering logic is duplicated. Consider extracting it into a helper function.
+const filterUnreadChats = (chats: any[], userId: string) => { + return chats.filter((chat) => { + const unseenMessages = JSON.parse( + chat.unseenMessagesByUsers as unknown as string, + ); + return unseenMessages[userId] > 0; + }); +}; - const filteredChats = chatsByUserId.filter((chat) => { - const unseenMessages = JSON.parse( - chat.unseenMessagesByUsers as unknown as string, - ); - return unseenMessages[testUser?._id] > 0; - }); + const filteredChats = filterUnreadChats(chatsByUserId, testUser?._id);Also applies to: 83-88
59-61:
⚠️ Potential issueAdd error handling for JSON.parse.
The JSON.parse operation could throw if the data is malformed. Consider adding error handling or validation.
- const unseenMessages = JSON.parse( - chat.unseenMessagesByUsers as unknown as string, - ); + let unseenMessages = {}; + try { + unseenMessages = JSON.parse( + chat.unseenMessagesByUsers as unknown as string, + ); + } catch (error) { + console.error('Invalid unseenMessagesByUsers format:', error); + return false; + }Also applies to: 84-86
29-41:
⚠️ Potential issueImprove error test case reliability.
The current error test doesn't guarantee that an error is actually thrown. Use expect().rejects.toThrow() for better assertions.
- try { - const args: QueryChatsByUserIdArgs = { - id: new Types.ObjectId().toString(), - }; - - await getUnreadChatsByUserIdResolver?.({}, args, {}); - } catch (error: unknown) { - expect((error as Error).message).toEqual("Chats not found"); - } + const args: QueryChatsByUserIdArgs = { + id: new Types.ObjectId().toString(), + }; + + await expect( + getUnreadChatsByUserIdResolver?.({}, args, {}) + ).rejects.toThrow("Chats not found");📝 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.describe("resolvers -> Query -> getUnreadChatsByUserId", () => { it(`throws NotFoundError if no Chats exists with chats.users containing user with _id === args.id`, async () => { const args: QueryChatsByUserIdArgs = { id: new Types.ObjectId().toString(), }; await expect( getUnreadChatsByUserIdResolver?.({}, args, {}) ).rejects.toThrow("Chats not found"); });tests/resolvers/Query/getGroupChatsByUserId.spec.ts (4)
58-70: 🛠️ Refactor suggestion
Extract common filter logic to reduce duplication.
The chat filtering logic is duplicated between test cases. Consider extracting it to a helper function:
const getFilteredChats = async (userId: string) => { const chats = await Chat.find({ users: userId }).lean(); return chats.filter((chat) => { const unseenMessages = JSON.parse( chat.unseenMessagesByUsers as unknown as string ); return unseenMessages[userId] > 0; }); };Then use it in both test cases:
- const chatsByUserId = await Chat.find({ - users: testUser?._id, - }).lean(); - - const filteredChats = chatsByUserId.filter((chat) => { - const unseenMessages = JSON.parse( - chat.unseenMessagesByUsers as unknown as string, - ); - return unseenMessages[testUser?._id] > 0; - }); + const filteredChats = await getFilteredChats(testUser._id);Also applies to: 83-95
63-65:
⚠️ Potential issueImprove type safety for unseenMessagesByUsers parsing.
The current type casting of unseenMessagesByUsers is unsafe. Consider adding type validation:
interface UnseenMessages { [userId: string]: number; } function parseUnseenMessages(raw: unknown): UnseenMessages { if (typeof raw !== 'string') { throw new Error('unseenMessagesByUsers must be a string'); } const parsed = JSON.parse(raw); if (typeof parsed !== 'object' || parsed === null) { throw new Error('Invalid unseenMessagesByUsers format'); } return parsed as UnseenMessages; }Then use it in the filter function:
- const unseenMessages = JSON.parse( - chat.unseenMessagesByUsers as unknown as string, - ); + const unseenMessages = parseUnseenMessages(chat.unseenMessagesByUsers);Also applies to: 88-90
20-31:
⚠️ Potential issueAdd error handling and improve type safety in test setup.
The test setup could be more robust with the following improvements:
- Add error handling for database connection:
beforeAll(async () => { - MONGOOSE_INSTANCE = await connect(); + try { + MONGOOSE_INSTANCE = await connect(); + } catch (error) { + console.error('Failed to connect to test database:', error); + throw error; + } const resultArray = await createTestChat();
- Add type assertion for better type safety:
- const resultArray = await createTestChat(); + const resultArray = (await createTestChat()) as TestUserType[];
- Clean up unused destructuring:
- [testUser1, ,] = await createTestGroupChatWithoutImage(); + [testUser1] = await createTestGroupChatWithoutImage();📝 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.beforeAll(async () => { try { MONGOOSE_INSTANCE = await connect(); } catch (error) { console.error('Failed to connect to test database:', error); throw error; } const resultArray = (await createTestChat()) as TestUserType[]; [testUser1] = await createTestGroupChatWithoutImage(); testUser = resultArray[0]; }); afterAll(async () => { await disconnect(MONGOOSE_INSTANCE); });
33-70:
⚠️ Potential issueImprove type safety and reduce code duplication in tests.
The first two test cases need improvements in error handling and type safety:
- Extract error message to a constant:
+const CHATS_NOT_FOUND_ERROR = "Chats not found"; + describe("resolvers -> Query -> getGroupChatsByUserId", () => { it(`throws NotFoundError if no Chats exists with chats.users containing user with _id === args.id`, async () => { try { const args: QueryChatsByUserIdArgs = { id: new Types.ObjectId().toString(), }; await getGroupChatsByUserIdResolver?.({}, args, {}); + fail('Expected error to be thrown'); } catch (error: unknown) { - expect((error as Error).message).toEqual("Chats not found"); + expect((error as Error).message).toEqual(CHATS_NOT_FOUND_ERROR); } });
- Remove optional chaining on resolver as it should always be defined:
- await getGroupChatsByUserIdResolver?.({}, args, {}); + await getGroupChatsByUserIdResolver({}, args, {});Committable suggestion was skipped due to low confidence.
src/models/ChatMessage.ts (1)
16-16: 🛠️ Refactor suggestion
Consider using a more specific type for media content.
The
mediafield type could be more descriptive and constrained. Consider using a union type or interface that explicitly defines the expected media format (e.g., URL, base64, file path).- media: string; + media: { + url: string; + type: 'IMAGE' | 'VIDEO' | 'FILE'; + mimeType: string; + };Also, please add JSDoc documentation for this new field to maintain consistency with the rest of the 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.media: { url: string; type: 'IMAGE' | 'VIDEO' | 'FILE'; mimeType: string; };src/models/Chat.ts (2)
14-14: 🛠️ Refactor suggestion
Consider using a more specific type for
unseenMessagesByUsersUsing the raw
JSONtype reduces type safety. Consider defining a specific interface that represents the structure of unseen messages data.- unseenMessagesByUsers: JSON; + unseenMessagesByUsers: { + [userId: string]: { + count: number; + lastSeen?: Date; + }; + };Also, please add JSDoc comments for these new fields to maintain documentation consistency.
Also applies to: 24-24
103-106: 🛠️ Refactor suggestion
Improve schema definition for
unseenMessagesByUsersUsing the
JSONtype in Mongoose has limitations:
- Limited query capabilities on nested properties
- No schema validation for the structure
- Missing default value for a required field
Consider using a nested schema structure:
- unseenMessagesByUsers: { - type: JSON, - required: true, - }, + unseenMessagesByUsers: { + type: Map, + of: new Schema({ + count: { type: Number, required: true, default: 0 }, + lastSeen: { type: Date } + }), + required: true, + default: new Map() + },📝 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.unseenMessagesByUsers: { type: Map, of: new Schema({ count: { type: Number, required: true, default: 0 }, lastSeen: { type: Date } }), required: true, default: new Map() },src/typeDefs/queries.ts (2)
51-52:
⚠️ Potential issueAdd user ID parameter for security.
The query
getUnreadChatsByUserIdshould require a user ID parameter to prevent unauthorized access to other users' unread chats.Apply this diff to fix the security issue:
- getUnreadChatsByUserId: [Chat] + getUnreadChatsByUserId(userId: ID!): [Chat]📝 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.getUnreadChatsByUserId(userId: ID!): [Chat]
53-53:
⚠️ Potential issueAdd user ID parameter for security.
The query
getGroupChatsByUserIdshould require a user ID parameter to prevent unauthorized access to other users' group chats.Apply this diff to fix the security issue:
- getGroupChatsByUserId: [Chat] + getGroupChatsByUserId(userId: ID!): [Chat]📝 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.getGroupChatsByUserId(userId: ID!): [Chat]tests/resolvers/Mutation/markChatMessagesAsRead.spec.ts (5)
47-116: 🛠️ Refactor suggestion
Refactor repeated spy mocking pattern.
The translation spy mocking pattern is repeated in multiple tests. Consider extracting it into a helper function or beforeEach hook.
let translateSpy: SpyInstance; beforeEach(() => { const { requestContext } = require("../../../src/libraries"); translateSpy = vi .spyOn(requestContext, "translate") .mockImplementation((message) => message); }); afterEach(() => { translateSpy.mockRestore(); });
64-66: 🛠️ Refactor suggestion
Enhance error assertions with type checking.
The error assertions only check the error message. Consider also verifying the error type for more robust testing.
-expect((error as Error).message).toEqual(USER_NOT_FOUND_ERROR.MESSAGE); +expect(error).toBeInstanceOf(Error); +expect(error).toHaveProperty('message', USER_NOT_FOUND_ERROR.MESSAGE); +expect(error).toHaveProperty('code', USER_NOT_FOUND_ERROR.CODE);Also applies to: 88-89, 111-113
41-41:
⚠️ Potential issueFix typo in describe block name.
The describe block has a typo: "markChatMessagesMessagesAsRead" should be "markChatMessagesAsRead".
-describe("resolvers -> Mutation -> markChatMessagesMessagesAsRead", () => { +describe("resolvers -> Mutation -> markChatMessagesAsRead", () => {📝 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.describe("resolvers -> Mutation -> markChatMessagesAsRead", () => {
27-35: 🛠️ Refactor suggestion
Consider moving test data creation into individual tests.
The test data setup in
beforeAllcreates shared state across all tests. For better test isolation and to prevent potential interference between tests, consider moving the test data creation into the individual test cases where they are needed.-let testUser: TestUserType; -let testChat: TestChatType; -beforeAll(async () => { - MONGOOSE_INSTANCE = await connect(); - const resultsArray = await createTestUserAndOrganization(); - testUser = resultsArray[0]; - [, , testChat] = await createTestChatMessage(); -}); +beforeAll(async () => { + MONGOOSE_INSTANCE = await connect(); +});Committable suggestion was skipped due to low confidence.
118-130:
⚠️ Potential issueAdd assertions to verify successful mutation result.
The successful test case doesn't verify that the messages were actually marked as read. Consider adding assertions to check the return value and database state.
await markChatMessagesAsReadResolver?.({}, args, context); +const result = await markChatMessagesAsReadResolver?.({}, args, context); +expect(result).toBeTruthy(); + +// Verify database state +const updatedChat = await Chat.findById(args.chatId); +expect(updatedChat.messages.every(msg => msg.readBy.includes(args.userId))).toBe(true);Committable suggestion was skipped due to low confidence.
tests/resolvers/Mutation/updateChat.spec.ts (3)
51-75: 🛠️ Refactor suggestion
Standardize spy cleanup and enhance test coverage.
Several improvements needed across multiple test cases:
- Some tests are missing spy cleanup (e.g., successful update test)
- The successful update test could verify more fields
- Error assertions could use
expect.toThrow()pattern for better error handlingExample improvement for the successful update test:
const updatedChat = await updateChatResolver?.({}, args, context); expect(updatedChat?.name).toEqual(args.input.name); + expect(updatedChat?._id).toEqual(args.input._id); + expect(updatedChat?.users).toBeDefined(); + expect(updatedChat?.users).toContain(context.userId);Example improvement for error assertions:
- try { - await updateChatResolver?.({}, args, context); - } catch (error: unknown) { - expect((error as Error).message).toEqual(USER_NOT_FOUND_ERROR.MESSAGE); - } + await expect( + updateChatResolver?.({}, args, context) + ).rejects.toThrow(USER_NOT_FOUND_ERROR.MESSAGE);Also applies to: 77-101, 130-145
45-45:
⚠️ Potential issueFix incorrect describe block name.
The describe block is named "addUserToGroupChat" but this test file is for the "updateChat" mutation.
-describe("resolvers -> Mutation -> addUserToGroupChat", () => { +describe("resolvers -> Mutation -> updateChat", () => {📝 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.describe("resolvers -> Mutation -> updateChat", () => {
147-170:
⚠️ Potential issueEnhance image upload test assertions.
The test for updating chat with image doesn't verify that the image URL was actually updated in the chat. Consider adding assertions to check the final state of the chat.
const updatedChat = await updateChatResolver?.({}, args, context); + expect(updatedChat?.image).toEqual(args.input.image); spy.mockRestore();Committable suggestion was skipped due to low confidence.
tests/helpers/chat.ts (3)
134-134: 🛠️ Refactor suggestion
Consider parameterizing the media field.
The hardcoded "media" value might not be suitable for all test scenarios.
- media: "media", + media: mediaContent ?? "default-media",Committable suggestion was skipped due to low confidence.
138-149: 🛠️ Refactor suggestion
Optimize database operations by combining update and fetch.
The separate update and fetch operations can be combined using
{ new: true }option.- await Chat.findByIdAndUpdate( + const chat = await Chat.findByIdAndUpdate( { _id: testChat._id, }, { messages: [testChatMessage._id], }, + { new: true } ); - - const chat = await Chat.findById(testChat._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.const chat = await Chat.findByIdAndUpdate( { _id: testChat._id, }, { messages: [testChatMessage._id], }, { new: true } ); return [testUser, testOrganization, chat, testChatMessage];
31-33: 🛠️ Refactor suggestion
Consider parameterizing the unseen messages count.
The hardcoded value of 5 for unseenMessagesByUsers might not be flexible enough for different test scenarios.
- unseenMessagesByUsers: JSON.stringify({ - [testUser._id]: 5, - }), + unseenMessagesByUsers: JSON.stringify({ + [testUser._id]: unseenMessagesCount ?? 5, + }),Committable suggestion was skipped due to low confidence.
tests/resolvers/Mutation/addUserToGroupChat.spec.ts (1)
48-69: 🛠️ Refactor suggestion
Enhance error test cases for better reliability.
Consider the following improvements for error test cases:
- Move spy cleanup to afterEach to avoid repetition
- Add expect.assertions() to ensure errors are actually thrown
- Use more specific error type checking
describe("resolvers -> Mutation -> addUserToGroupChat", () => { afterEach(() => { vi.doUnmock("../../../src/constants"); vi.resetModules(); + spy.mockRestore(); }); it(`throws NotFoundError message if current user does not exists`, async () => { + expect.assertions(1); // ... existing code ... - spy.mockRestore(); });Also applies to: 71-92, 94-117, 119-142, 144-165
src/resolvers/Mutation/createEvent.ts (1)
181-197:
⚠️ Potential issueAdd error handling for chat creation
The chat creation block needs additional error handling and validation:
- The chat creation could fail silently if the function is undefined
- There's no explicit error handling for chat creation failures
- The result of chat creation isn't validated before use
Consider wrapping the chat creation in a try-catch block and validating the result:
let chat; if (args.data.createChat && typeof createChat === "function") { + try { chat = await createChat( _parent, { data: { name: args.data.title, organizationId: args.data.organizationId, userIds: [currentUser._id.toString()], isGroup: true, - image: null, }, }, context, ); + if (!chat?._id) { + throw new Error("Chat creation failed: Invalid chat object returned"); + } + } catch (error) { + throw new errors.ValidationError( + `Failed to create chat for event: ${error.message}`, + "CHAT_CREATION_FAILED" + ); + } }📝 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.let chat; if (args.data.createChat && typeof createChat === "function") { try { chat = await createChat( _parent, { data: { name: args.data.title, organizationId: args.data.organizationId, userIds: [currentUser._id.toString()], isGroup: true, }, }, context, ); if (!chat?._id) { throw new Error("Chat creation failed: Invalid chat object returned"); } } catch (error) { throw new errors.ValidationError( `Failed to create chat for event: ${error.message}`, "CHAT_CREATION_FAILED" ); } }tests/resolvers/Mutation/updateChatMessage.spec.ts (3)
51-226: 🛠️ Refactor suggestion
Refactor common test setup to reduce duplication.
The test file has significant duplication in setup code. Consider extracting common setup into helper functions or beforeEach hooks.
Here's a suggested refactoring:
describe("resolvers -> Mutation -> updateChatMessage", () => { let requestContext: any; let spy: any; let updateChatMessageResolver: any; beforeEach(async () => { // Import dependencies const imports = await import("../../../src/libraries"); requestContext = imports.requestContext; spy = vi .spyOn(requestContext, "translate") .mockImplementation((message) => message); const resolverImport = await import( "../../../src/resolvers/Mutation/updateChatMessage" ); updateChatMessageResolver = resolverImport.updateChatMessage; }); afterEach(() => { spy.mockRestore(); vi.doUnmock("../../../src/constants"); vi.resetModules(); }); const createTestArgs = ( messageContent: string = "New Message", chatId: string = testChat?.id, messageId: string = testMessage?._id.toString() || "" ): MutationUpdateChatMessageArgs => ({ input: { messageContent, chatId, messageId } }); // Then each test becomes more concise: it(`throws NotFoundError message if current user does not exists`, async () => { const args = createTestArgs(); const context = { userId: new Types.ObjectId().toString() }; await expect( updateChatMessageResolver?.({}, args, context) ).rejects.toThrow(USER_NOT_FOUND_ERROR.MESSAGE); }); // ... other tests follow similar pattern });
45-45:
⚠️ Potential issueFix incorrect test suite name.
The describe block name "addUserToGroupChat" doesn't match the functionality being tested. This file tests the "updateChatMessage" mutation.
-describe("resolvers -> Mutation -> addUserToGroupChat", () => { +describe("resolvers -> Mutation -> updateChatMessage", () => {📝 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.describe("resolvers -> Mutation -> updateChatMessage", () => {
198-225:
⚠️ Potential issueComplete the successful update test case.
The test case for successful message update is incomplete:
- Missing assertions to verify the updated message content
- Contains a debug console.log statement
- Doesn't verify the message was actually updated in the database
Here's how to improve the test:
it(`updates the chat message and returns it`, async () => { const { requestContext } = await import("../../../src/libraries"); const spy = vi .spyOn(requestContext, "translate") .mockImplementationOnce((message) => message); const newMessageContent = "New Message"; const args: MutationUpdateChatMessageArgs = { input: { - messageContent: "New Message", + messageContent: newMessageContent, chatId: testChat?.id, messageId: testMessage?._id.toString() ? testMessage?._id.toString() : "", }, }; const context = { userId: testMessage?.sender, }; const { updateChatMessage: updateChatMessageResolver } = await import( "../../../src/resolvers/Mutation/updateChatMessage" ); const updatedMessage = await updateChatMessageResolver?.({}, args, context); - console.log(updatedMessage); + // Verify the message was updated + expect(updatedMessage).toBeDefined(); + expect(updatedMessage?.messageContent).toBe(newMessageContent); + + // Verify the message was updated in the database + const { ChatMessage } = await import("../../../src/models"); + const messageInDb = await ChatMessage.findById(testMessage?._id); + expect(messageInDb?.messageContent).toBe(newMessageContent); spy.mockRestore(); });📝 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(`updates the chat message and returns it`, async () => { const { requestContext } = await import("../../../src/libraries"); const spy = vi .spyOn(requestContext, "translate") .mockImplementationOnce((message) => message); const newMessageContent = "New Message"; const args: MutationUpdateChatMessageArgs = { input: { messageContent: newMessageContent, chatId: testChat?.id, messageId: testMessage?._id.toString() ? testMessage?._id.toString() : "", }, }; const context = { userId: testMessage?.sender, }; const { updateChatMessage: updateChatMessageResolver } = await import( "../../../src/resolvers/Mutation/updateChatMessage" ); const updatedMessage = await updateChatMessageResolver?.({}, args, context); // Verify the message was updated expect(updatedMessage).toBeDefined(); expect(updatedMessage?.messageContent).toBe(newMessageContent); // Verify the message was updated in the database const { ChatMessage } = await import("../../../src/models"); const messageInDb = await ChatMessage.findById(testMessage?._id); expect(messageInDb?.messageContent).toBe(newMessageContent); spy.mockRestore(); });src/typeDefs/mutations.ts (2)
254-254: 🛠️ Refactor suggestion
Consider optimizing the markChatMessagesAsRead mutation.
The current implementation has a few potential improvements:
- The
userIdparameter seems redundant since the mutation is already authenticated with@auth. The user context could be used instead.- Consider returning a more specific type (e.g.,
MarkMessagesAsReadPayload) that includes only the necessary fields instead of the entireChatobject.- markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat @auth + markChatMessagesAsRead(chatId: ID!): MarkMessagesAsReadPayload @authCommittable suggestion was skipped due to low confidence.
249-250: 💡 Codebase verification
Consider using the URL scalar type for the media field
The codebase already has a URL scalar type that's being used for media URLs in other parts of the schema (e.g., Advertisement's mediaUrl). For consistency and type safety:
- Change
media: Stringtomedia: URLin the sendMessageToChat mutation- This provides better type validation for URLs and maintains consistency with existing patterns in the codebase
🔗 Analysis chain
Consider adding input validation and documentation for media uploads.
While the changes support image sharing in chats, consider the following improvements:
- Add documentation comments describing accepted media formats and size limits
- Consider using a more specific type for media (e.g., URL scalar type) instead of generic String
Let's check if there are any existing custom scalar types we could use:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for custom scalar type definitions that might be suitable for media URLs rg "scalar.*URL" --type tsLength of output: 65
Script:
#!/bin/bash # Let's check if URL scalar is already being used for media fields in the schema rg "media.*URL" --type ts -A 2 -B 2Length of output: 6374
Script:
#!/bin/bash # Let's also check the scalar definition and its usage cat src/typeDefs/scalars.tsLength of output: 403
Script:
#!/bin/bash # Check for any existing documentation or validation related to media rg "media.*validation|validation.*media" --type tsLength of output: 832
src/typeDefs/inputs.ts (1)
33-36: 🛠️ Refactor suggestion
Consider enhancing chat filtering capabilities.
The
ChatWhereInputtype could benefit from additional fields to support more comprehensive filtering:
organizationIdfor filtering chats by organizationisGroupfor filtering between direct and group chatsname_starts_withfor consistency with other filter typesinput ChatWhereInput { user: UserWhereInput name_contains: String + name_starts_with: String + organizationId: ID + isGroup: Boolean }📝 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.input ChatWhereInput { user: UserWhereInput name_contains: String name_starts_with: String organizationId: ID isGroup: Boolean }src/typeDefs/types.ts (4)
745-745: 🛠️ Refactor suggestion
Consider using a more type-safe approach instead of JSON.
The
unseenMessagesByUsersfield using JSON type might make it harder to validate the data structure and could lead to runtime errors. Consider creating a dedicated type for better type safety and documentation.- unseenMessagesByUsers: JSON + unseenMessagesByUsers: [UnseenMessages!]Add this new type:
type UnseenMessages { user: User! messageIds: [ID!]! }
754-754: 🛠️ Refactor suggestion
Enhance media field type safety and documentation.
The current
mediafield is too generic. Consider:
- Using a more specific name to indicate it's for image URLs
- Adding validation for supported media types
- media: String + imageUrl: URLOr for better media type support:
type ChatMessageMedia { url: URL! type: MediaType! } enum MediaType { IMAGE VIDEO DOCUMENT }
766-770: 🛠️ Refactor suggestion
Enhance message update capabilities.
The current input type has some limitations:
- No support for updating media content
- No validation for message content length
- No field to track edit history
input UpdateChatMessageInput { chatId: ID! - messageContent: String! + messageContent: String! @constraint(minLength: 1, maxLength: 1000) messageId: ID! + media: ChatMessageMediaInput } + input ChatMessageMediaInput { + url: URL! + type: MediaType! + }Also consider adding an
editHistoryfield to theChatMessagetype to track message modifications.Committable suggestion was skipped due to low confidence.
760-764: 🛠️ Refactor suggestion
Add URL validation for the image field.
Consider using the
URLscalar type for the image field to ensure valid URLs are provided.input UpdateChatInput { _id: ID! name: String - image: String + image: URL }Committable suggestion was skipped due to low confidence.
src/resolvers/Query/helperFunctions/getWhere.ts (1)
306-311:
⚠️ Potential issueFix duplicate name filter conditions
There are two conflicting conditions for
where.name:
- A case-insensitive regex search at line 306
- An exact match at line 324
This creates ambiguity in the filter behavior, where the regex search will never execute because it will be overridden by the exact match condition.
Consolidate the conditions to ensure consistent behavior. Here's the suggested fix:
- if (where.name) { - wherePayload = { - ...wherePayload, - name: { $regex: where.name, $options: "i" }, - }; - } // Returns provided name organization if (where.name) { wherePayload = { ...wherePayload, - name: where.name, + name: { $regex: where.name, $options: "i" }, }; }Also applies to: 324-328
src/resolvers/Mutation/markChatMessagesAsRead.ts (3)
58-60: 🛠️ Refactor suggestion
Consider storing
unseenMessagesByUsersas a native objectCurrently,
unseenMessagesByUsersis stored as a JSON string in the database, requiring parsing and stringifying during read and write operations. Storing it as a native object would improve efficiency and simplify the code.To implement this refactor, remove unnecessary
JSON.parseandJSON.stringifycalls:-const unseenMessagesByUsers = JSON.parse( - chat.unseenMessagesByUsers as unknown as string, -); +const unseenMessagesByUsers = chat.unseenMessagesByUsers as Record<string, number>; ... $set: { - unseenMessagesByUsers: JSON.stringify(unseenMessagesByUsers), + unseenMessagesByUsers: unseenMessagesByUsers, },Also applies to: 74-75
62-66:
⚠️ Potential issueReplace
mapwithforEachfor side-effect iterationUsing
mapwithout utilizing its returned array is inefficient and may cause confusion. Since you are performing an operation for its side effects,forEachis more appropriate.Apply this diff to correct the iteration method:
-Object.keys(unseenMessagesByUsers).map((user: string) => { +Object.keys(unseenMessagesByUsers).forEach((user: string) => { if (user === context.userId) { unseenMessagesByUsers[user] = 0; } });📝 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.Object.keys(unseenMessagesByUsers).forEach((user: string) => { if (user === context.userId) { unseenMessagesByUsers[user] = 0; } });
10-18:
⚠️ Potential issueCorrect the function description in the JSDoc comment
The JSDoc comment describes sending a message to a direct chat, but this function actually marks chat messages as read. Please update the comment to accurately reflect the function's purpose.
Apply this diff to correct the JSDoc comment:
/** - * This function enables to send message to direct chat. + * Marks chat messages as read for the current user in a chat. * @param _parent - parent of current request * @param args - payload provided with the request * @param context - context of entire application * @remarks The following checks are done: - * 1. If the direct chat exists. + * 1. If the chat exists. * 2. If the user exists + * 3. If the user is a member of the chat. - * @returns Direct chat message. + * @returns Updated chat object. */📝 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./** * Marks chat messages as read for the current user in a chat. * @param _parent - parent of current request * @param args - payload provided with the request * @param context - context of entire application * @remarks The following checks are done: * 1. If the chat exists. * 2. If the user exists * 3. If the user is a member of the chat. * @returns Updated chat object. */src/resolvers/Mutation/sendMessageToChat.ts (4)
50-59:
⚠️ Potential issueEnhance media input validation and add test coverage.
The media handling logic checks the prefix of
args.mediato determine the media type. Consider improving validation to ensure thatargs.mediais a properly formatted data URL and to mitigate potential security risks from malformed inputs.Additionally, the new media processing code is not covered by tests. Please add unit tests to cover image and video uploads, as well as the error case for unsupported file types.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 51-58: src/resolvers/Mutation/sendMessageToChat.ts#L51-L58
Added lines #L51 - L58 were not covered by tests
71-74:
⚠️ Potential issueRefine type handling for
unseenMessagesByUsers.Casting
chat.unseenMessagesByUsersasunknown as stringmay indicate type inconsistencies. Consider updating the type definitions to avoid unnecessary casting and ensure type safety. Also, ensure thatchat.unseenMessagesByUsersis properly defined to prevent potential runtime errors when parsing.
90-93: 🛠️ Refactor suggestion
Consider storing
unseenMessagesByUsersas an object in the database.Storing
unseenMessagesByUsersas a JSON string adds complexity and overhead due to the need forJSON.stringifyandJSON.parse. Consider updating the database schema to store this field as an object directly, which can improve performance and simplify the code.
75-79: 🛠️ Refactor suggestion
Use
.forEachinstead of.mapfor side effects.The
.mapmethod is intended for transforming arrays and returning a new array, but here it's used for side effects without utilizing the returned array. Replace.mapwith.forEachfor clarity and to reflect that the code is iterating for side effects.Apply this diff to make the change:
- Object.keys(unseenMessagesByUsers).map((user: string) => { + Object.keys(unseenMessagesByUsers).forEach((user: string) => {📝 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.Object.keys(unseenMessagesByUsers).forEach((user: string) => { if (user !== context.userId) { unseenMessagesByUsers[user] += 1; } });src/resolvers/Mutation/updateChat.ts (5)
67-72:
⚠️ Potential issueUse Appropriate Error Class for Authorization Failures
Throwing a
NotFoundErrorwhen the user is not authorized might mislead clients and hinder proper error handling.Consider using
ForbiddenErrorto accurately represent authorization errors:- throw new errors.NotFoundError( + throw new errors.ForbiddenError( requestContext.translate(USER_NOT_AUTHORIZED_ERROR.MESSAGE), USER_NOT_AUTHORIZED_ERROR.CODE, USER_NOT_AUTHORIZED_ERROR.PARAM, );This ensures that the error message and HTTP status code correctly reflect the nature of the error, improving client-side handling.
📝 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.throw new errors.ForbiddenError( requestContext.translate(USER_NOT_AUTHORIZED_ERROR.MESSAGE), USER_NOT_AUTHORIZED_ERROR.CODE, USER_NOT_AUTHORIZED_ERROR.PARAM, ); }
78-79:
⚠️ Potential issueAdd Error Handling for Image Upload Failures
The
uploadEncodedImagefunction might throw an error if the image upload fails, but currently, there's no error handling in place.Consider wrapping the upload in a try-catch block to handle potential errors:
if (existingchat.isGroup === true && args.input.image) { + try { uploadMediaFile = await uploadEncodedImage(args.input.image, null); + } catch (error) { + throw new errors.InternalServerError( + 'Failed to upload image.', + 'IMAGE_UPLOAD_ERROR', + ); + } }This provides clearer feedback to the client and prevents unhandled exceptions from breaking the application.
📝 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.try { uploadMediaFile = await uploadEncodedImage(args.input.image, null); } catch (error) { throw new errors.InternalServerError( 'Failed to upload image.', 'IMAGE_UPLOAD_ERROR', ); }
41-59: 🛠️ Refactor suggestion
Simplify User Retrieval Logic for Better Readability
The current logic for fetching
currentUserinvolves multiple steps and checks, which can be streamlined to improve readability and maintainability.Consider refactoring as follows:
- let currentUser: InterfaceUser | null; - const userFoundInCache = await findUserInCache([context.userId]); - currentUser = userFoundInCache[0]; - if (currentUser === null) { - currentUser = await User.findOne({ - _id: context.userId, - }).lean(); - if (currentUser !== null) { - await cacheUsers([currentUser]); - } - } - if (!currentUser) { - throw new errors.NotFoundError( - requestContext.translate(USER_NOT_FOUND_ERROR.MESSAGE), - USER_NOT_FOUND_ERROR.CODE, - USER_NOT_FOUND_ERROR.PARAM, - ); - } + const currentUser = await findOrCreateCachedUser(context.userId); + if (!currentUser) { + throw new errors.NotFoundError( + requestContext.translate(USER_NOT_FOUND_ERROR.MESSAGE), + USER_NOT_FOUND_ERROR.CODE, + USER_NOT_FOUND_ERROR.PARAM, + ); + }Create a helper function
findOrCreateCachedUser(userId)that handles fetching the user from cache or database and caching them if necessary. This reduces code duplication and enhances clarity.Committable suggestion was skipped due to low confidence.
81-84:
⚠️ Potential issueEnsure Accurate Fields Update Logic
The current logic for
fieldsToUpdatemight overwrite theimagefield incorrectly ifargs.input.imageis not provided.Consider modifying the logic to ensure that the
imagefield is only updated when a new image is uploaded:const fieldsToUpdate = { ...args.input, - image: args.input.image ? uploadMediaFile : existingchat.image, + ...(uploadMediaFile ? { image: uploadMediaFile } : {}), };This way, if no new image is provided, the existing image remains unchanged without overwriting it with
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.// Prepare fields to update const fieldsToUpdate = { ...args.input, ...(uploadMediaFile ? { image: uploadMediaFile } : {}), };
87-99:
⚠️ Potential issueHandle Potential
nullValue After Updating ChatThere's a possibility that
updatedChatcould benullif the chat no longer exists or the update operation fails.Add a check to handle this scenario:
const updatedChat = await Chat.findOneAndUpdate( // existing parameters ).lean(); + if (!updatedChat) { + throw new errors.NotFoundError( + requestContext.translate(CHAT_NOT_FOUND_ERROR.MESSAGE), + CHAT_NOT_FOUND_ERROR.CODE, + CHAT_NOT_FOUND_ERROR.PARAM, + ); + } return updatedChat as InterfaceChat;This ensures that the function does not return
null, which could lead to unexpected errors downstream.Committable suggestion was skipped due to low confidence.
src/resolvers/Mutation/updateChatMessage.ts (2)
106-106:
⚠️ Potential issueValidate and sanitize
messageContentbefore updatingTo prevent potential security risks, ensure that
args.input.messageContentis properly validated and sanitized before updating the database.
85-89: 💡 Codebase verification
⚠️ Potential issueUse
UnauthorizedErrorfor authorization failuresThe codebase has
UnauthorizedErrordefined insrc/libraries/errors/unauthorizedError.tswhich is the appropriate error type to use for authorization failures. The suggested change to useUnauthorizedErrorinstead ofNotFoundErroris correct.🔗 Analysis chain
Use appropriate error type for authorization failures
Throwing a
NotFoundErrorwhen the user is not authorized may be misleading. Consider using anUnauthorizedErrororForbiddenErrorto accurately represent the authorization error.Apply this diff to use the appropriate error type:
- throw new errors.NotFoundError( + throw new errors.UnauthorizedError( requestContext.translate(USER_NOT_AUTHORIZED_ERROR.MESSAGE), USER_NOT_AUTHORIZED_ERROR.CODE, USER_NOT_AUTHORIZED_ERROR.PARAM, );Run the following script to verify if
UnauthorizedErrorexists in the errors module:🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify if UnauthorizedError exists in the errors module. # Expected: Should find UnauthorizedError or ForbiddenError defined. rg 'class (UnauthorizedError|ForbiddenError)' -t ts -A 5Length of output: 648
src/resolvers/Mutation/addUserToGroupChat.ts (5)
13-27:
⚠️ Potential issueSynchronize function comments with code implementation
The function comments mention
args.input.chatId,args.input.userId, andargs.input.organizationId, but the code usesargs.chatIdandargs.userId, and does not utilizeorganizationId.Please update the comments to reflect the actual parameters used in the code, or adjust the code to align with the comments.
21-22:
⚠️ Potential issueUpdate or implement missing functionality
The comments specify:
- Step 5: "Ensures that the organization specified by
args.input.organizationIdexists."- Step 6: "Adds the user to the chat's admins list."
However, the current code does not implement these steps. Additionally, in step 6, the code adds the user to the
userslist instead of theadminslist.Please verify whether these steps are required. If they are, implement the necessary code; otherwise, update the comments to accurately describe the function's behavior.
101-113:
⚠️ Potential issueCorrect misleading comments and variable names
The comments at lines 101 and 113 refer to "Ad" instead of "chat":
- Line 101:
// Creates new Ad.- Line 113:
// Returns createdAd.These appear to be copy-paste errors. Please update the comments to accurately reflect the operation:
- Line 101:
// Adds the user to the chat.- Line 113:
// Returns the updated chat.
77-80: 🛠️ Refactor suggestion
Simplify admin check logic
The current admin check uses
mapandincludesto compare IDs:if ( !chat.admins .map((admin) => admin.toString()) .includes(currentUser._id.toString()) ) { // ... }This can be simplified using the
somemethod withequalsfor object ID comparison.Apply this diff to enhance readability:
if ( - !chat.admins - .map((admin) => admin.toString()) - .includes(currentUser._id.toString()) + !chat.admins.some((admin) => admin.equals(currentUser._id)) ) { // ... }
102-111:
⚠️ Potential issueEnsure
findOneAndUpdatereturns the updated documentBy default,
findOneAndUpdatereturns the document before the update. To return the updated document, include the option{ new: true }.Apply this diff to fix the issue:
const updatedChat = await Chat.findOneAndUpdate( { _id: chat._id, }, { $push: { users: args.userId, }, }, + { + new: 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.const updatedChat = await Chat.findOneAndUpdate( { _id: chat._id, }, { $push: { users: args.userId, }, }, { new: true, }, );schema.graphql (7)
209-209: 🛠️ Refactor suggestion
Consider defining a specific type instead of using
JSONforunseenMessagesByUsersUsing
JSONas the type forunseenMessagesByUsersmay reduce type safety and clarity in your schema. Consider defining a specific type to represent the unseen messages per user to improve type safety and facilitate better code maintenance.
219-219: 🛠️ Refactor suggestion
Use the
URLscalar type instead ofStringformediaif it represents a URLIf the
mediafield represents a URL to the media content, consider using the existingURLscalar type to improve type safety and clarity.
1160-1160:
⚠️ Potential issueEnsure that at least one of
messageContentormediais requiredIn the
sendMessageToChatmutation, bothmessageContentandmediaare optional. This could allow sending messages without content or media, resulting in empty messages. Consider enforcing that at least one ofmessageContentormediamust be provided.
1491-1491: 🛠️ Refactor suggestion
Ensure consistent use of user ID in chat queries
The
chatsByUserIdquery accepts anidparameter, whereasgetGroupChatsByUserIdandgetUnreadChatsByUserIddo not. For consistency and better security practices, consider inferring the user ID from the authentication context in all chat queries and removing the explicitidparameter fromchatsByUserId.Also applies to: 1515-1515, 1519-1519
1713-1717: 🛠️ Refactor suggestion
Use
idinstead of_idinUpdateChatInputfor consistencyIn
UpdateChatInput, consider usingid: ID!instead of_id: ID!to maintain consistency with other input types in the schema.
1719-1723:
⚠️ Potential issueInclude
mediafield and adjust nullability inUpdateChatMessageInputThe
UpdateChatMessageInputlacks themediafield and requiresmessageContent. To allow updating both text and media content of a message, consider addingmedia: Stringto the input and makingmessageContentoptional, similar to thesendMessageToChatmutation.
1126-1126:
⚠️ Potential issueAvoid passing
userIdas a parameter; infer it from authentication contextPassing
userIdas a parameter inmarkChatMessagesAsReadcould lead to security issues, as users might mark messages as read on behalf of others. Consider inferring theuserIdfrom the authenticated user context instead.Apply this diff to adjust the mutation signature:
-markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat +markChatMessagesAsRead(chatId: ID!): Chat📝 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.markChatMessagesAsRead(chatId: ID!): Chatsrc/types/generatedGraphQLTypes.ts (4)
282-282: 🛠️ Refactor suggestion
Consider defining a specific type for 'unseenMessagesByUsers'
Using
Scalars['JSON']['output']forunseenMessagesByUsersmay reduce type safety. Consider defining a specific interface or type to represent the structure of this field for better clarity and type checking.
293-293: 🛠️ Refactor suggestion
Use a more specific scalar type for 'media' field
The
mediafield inChatMessageis of typeString. If it represents a URL, consider using theURLscalar type to enhance type safety and validation.
2802-2806: 🛠️ Refactor suggestion
Add missing fields to 'UpdateChatInput'
The
UpdateChatInputtype lacks fields for updating other chat properties such asadminsorusers. If intended, consider adding these fields to allow full chat updates.
1743-1744:
⚠️ Potential issueEnsure at least one of 'media' or 'messageContent' is required in 'sendMessageToChat'
Currently, both
mediaandmessageContentare optional, which could allow sending empty messages. To prevent this, ensure that at least one of these fields is required.Consider updating the mutation input to enforce this requirement:
export type MutationSendMessageToChatArgs = { chatId: Scalars['ID']['input']; - media?: InputMaybe<Scalars['String']['input']>; - messageContent?: InputMaybe<Scalars['String']['input']>; + media?: InputMaybe<Scalars['String']['input']>; + messageContent?: InputMaybe<Scalars['String']['input']>; + /** At least one of 'media' or 'messageContent' is required */ + _atLeastOneOfMediaOrMessageContent: Scalars['Boolean']['input']; replyTo?: InputMaybe<Scalars['ID']['input']>; };Alternatively, add validation in the resolver to check that at least one of
mediaormessageContentis provided.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. |
|
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. |
|
Please fix the conflicting files and tests |
|
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? |
|
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
|
Closing this as inactive. |
|
Reopening. GSoC |
What kind of change does this PR introduce?
Feature
Issue Number:
Fixes #2624
Did you add tests for your changes?
Yes
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
New Features
Bug Fixes
Documentation
Tests