-
-
Notifications
You must be signed in to change notification settings - Fork 821
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: functionality to view unread messages in chat #2611
base: develop
Are you sure you want to change the base?
Changes from all commits
a8c02f8
fe6907d
3a9799d
2fa65b1
f68bb01
f7089b9
2fe5353
7e31c14
0bf28a2
9c44ea4
fa5f6ec
3db8381
a1379bf
bd98a6b
7ec1773
3781ede
8aaf5f3
e83657d
23a9676
9576d4a
c2af22d
ad48619
77c7fde
d18843a
cf27ca6
8469653
87ad70f
f936adc
babf1c0
c84f33a
33535ac
98d06a7
5a61b43
a84f1c6
4d65b03
c23008b
a539e3e
074b79c
9926e63
1586732
8b873ea
6cb5dcc
15d6bed
46d4f5f
ce20fe0
dcd6006
ea23279
ef2d45d
dca6cf3
d33aada
56cd761
a2ccfb0
52165f1
ff1987f
369a81d
7cd8040
979bf37
f29ac65
dd9120f
a440bad
615a5ac
5ad1d76
08a6fed
c2d831a
bac5283
a2e5141
39d9086
b6fa93e
0e0b1d3
4f9ea48
69ee5c0
cb9bd96
d450059
7baed1d
aad9ea2
b5da053
50d579e
99a24c2
23ba0b5
c4f6cd6
3b0f12c
9f2f12d
b3032db
a6b960f
8548f25
bf38bc6
8f42668
d42aca6
4124a3a
7d5667d
9911641
3b4e431
d0b54f6
670f79e
b66d394
03a26d8
87eea3d
de1dea1
4050877
badec95
6a4ea4b
b625e84
2d49e0a
7d96ccc
7054695
de2fade
3f75514
c7fc6e3
8fdf40b
96189f7
70b59fb
0154a9c
48ee919
497d4de
6ef15db
df50b95
e638d96
40df8f8
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -206,6 +206,7 @@ type Chat { | |
messages: [ChatMessage] | ||
name: String | ||
organization: Organization | ||
unseenMessagesByUsers: JSON | ||
updatedAt: DateTime! | ||
users: [User!]! | ||
} | ||
|
@@ -1114,6 +1115,7 @@ type Mutation { | |
likePost(id: ID!): Post | ||
login(data: LoginInput!): AuthData! | ||
logout: Boolean! | ||
markChatMessagesAsRead(chatId: ID!, userId: ID!): Chat | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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!
} |
||
otp(data: OTPInput!): OtpData! | ||
recaptcha(data: RecaptchaVerification!): Boolean! | ||
refreshToken(refreshToken: String!): ExtendSession! | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,7 @@ export interface InterfaceChat { | |
createdAt: Date; | ||
updatedAt: Date; | ||
lastMessageId: string; | ||
unseenMessagesByUsers: JSON; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using a stricter type instead of JSON. Using the interface UnseenMessages {
[userId: string]: number; // Maps user IDs to their unseen message count
}
// Then use in interface:
unseenMessagesByUsers: UnseenMessages; Also, please add JSDoc documentation for this new field in the interface. |
||
} | ||
|
||
/** | ||
|
@@ -98,9 +99,13 @@ const chatSchema = new Schema( | |
type: String, | ||
required: false, | ||
}, | ||
unseenMessagesByUsers: { | ||
type: JSON, | ||
required: true, | ||
}, | ||
}, | ||
{ | ||
timestamps: true, | ||
timestamps: false, | ||
}, | ||
); | ||
|
||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -36,10 +36,6 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
// const userExists = (await User.exists({ | ||||||||||||||||||||||||||||||||||||||||||||||||
// _id: { $in: args.data.userIds }, | ||||||||||||||||||||||||||||||||||||||||||||||||
// })) as unknown as string[]; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
const userExists = await User.find({ | ||||||||||||||||||||||||||||||||||||||||||||||||
_id: { $in: args.data.userIds }, | ||||||||||||||||||||||||||||||||||||||||||||||||
}).lean(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -54,6 +50,13 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
const now = new Date(); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
const unseenMessagesByUsers = JSON.stringify( | ||||||||||||||||||||||||||||||||||||||||||||||||
args.data.userIds.reduce((unseenMessages: Record<string, number>, user) => { | ||||||||||||||||||||||||||||||||||||||||||||||||
unseenMessages[user] = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||
return unseenMessages; | ||||||||||||||||||||||||||||||||||||||||||||||||
}, {}), | ||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+53
to
+58
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add type safety and validation for unseenMessagesByUsers. The current implementation could benefit from:
+interface UnseenMessages {
+ [userId: string]: number;
+}
+
const unseenMessagesByUsers = JSON.stringify(
- args.data.userIds.reduce((unseenMessages: Record<string, number>, user) => {
+ args.data.userIds.reduce((unseenMessages: UnseenMessages, user) => {
+ if (!/^[0-9a-fA-F]{24}$/.test(user)) {
+ throw new errors.ValidationError(
+ 'Invalid user ID format',
+ 'INVALID_USER_ID',
+ 'userIds'
+ );
+ }
unseenMessages[user] = 0;
return unseenMessages;
}, {}),
); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
const chatPayload = args.data.isGroup | ||||||||||||||||||||||||||||||||||||||||||||||||
? { | ||||||||||||||||||||||||||||||||||||||||||||||||
isGroup: args.data.isGroup, | ||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -65,13 +68,15 @@ | |||||||||||||||||||||||||||||||||||||||||||||||
createdAt: now, | ||||||||||||||||||||||||||||||||||||||||||||||||
updatedAt: now, | ||||||||||||||||||||||||||||||||||||||||||||||||
image: args.data.image, | ||||||||||||||||||||||||||||||||||||||||||||||||
unseenMessagesByUsers, | ||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||
: { | ||||||||||||||||||||||||||||||||||||||||||||||||
creatorId: context.userId, | ||||||||||||||||||||||||||||||||||||||||||||||||
users: args.data.userIds, | ||||||||||||||||||||||||||||||||||||||||||||||||
isGroup: args.data.isGroup, | ||||||||||||||||||||||||||||||||||||||||||||||||
createdAt: now, | ||||||||||||||||||||||||||||||||||||||||||||||||
updatedAt: now, | ||||||||||||||||||||||||||||||||||||||||||||||||
unseenMessagesByUsers, | ||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||
const createdChat = await Chat.create(chatPayload); | ||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,62 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import type { MutationResolvers } from "../../types/generatedGraphQLTypes"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { errors, requestContext } from "../../libraries"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { Chat, User } from "../../models"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import { CHAT_NOT_FOUND_ERROR, USER_NOT_FOUND_ERROR } from "../../constants"; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
/** | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* This function marks all 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. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* 2. If the user exists | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
* @returns Updated chat object. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
*/ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
export const markChatMessagesAsRead: MutationResolvers["markChatMessagesAsRead"] = | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
async (_parent, args, context) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const chat = await Chat.findOne({ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_id: args.chatId, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}).lean(); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (!chat) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
throw new errors.NotFoundError( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
requestContext.translate(CHAT_NOT_FOUND_ERROR.MESSAGE), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
CHAT_NOT_FOUND_ERROR.CODE, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
CHAT_NOT_FOUND_ERROR.PARAM, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+16
to
+28
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add input validation for chatId. While MongoDB will handle invalid ObjectId formats, it's better to validate the input early to provide a better user experience. Add validation before the database query: export const markChatMessagesAsRead: MutationResolvers["markChatMessagesAsRead"] =
async (_parent, args, context) => {
+ if (!args.chatId?.match(/^[0-9a-fA-F]{24}$/)) {
+ throw new errors.ValidationError(
+ requestContext.translate(CHAT_NOT_FOUND_ERROR.MESSAGE),
+ CHAT_NOT_FOUND_ERROR.CODE,
+ CHAT_NOT_FOUND_ERROR.PARAM
+ );
+ }
+
const chat = await Chat.findOne({ 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
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, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+30
to
+40
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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"
+ );
+ }
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const unseenMessagesByUsers = JSON.parse( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
chat.unseenMessagesByUsers as unknown as string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+42
to
+45
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (unseenMessagesByUsers[context.userId] !== undefined) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unseenMessagesByUsers[context.userId] = 0; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+46
to
+48
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add type validation for unseenMessagesByUsers structure. The code assumes 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
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const updatedChat = await Chat.findByIdAndUpdate( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
_id: chat._id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
$set: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unseenMessagesByUsers: JSON.stringify(unseenMessagesByUsers), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+50
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add optimistic concurrency control to prevent race conditions. Multiple concurrent updates to 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'
+ );
+ }
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return updatedChat; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+50
to
+62
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fix update operation and add result validation. The update operation needs the Apply these changes: const updatedChat = await Chat.findByIdAndUpdate(
- {
- _id: chat._id,
- },
+ chat._id,
{
$set: {
unseenMessagesByUsers: JSON.stringify(unseenMessagesByUsers),
},
},
+ { new: true }
);
+ 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;
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -52,6 +52,17 @@ export const sendMessageToChat: MutationResolvers["sendMessageToChat"] = async ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
updatedAt: now, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
const unseenMessagesByUsers = JSON.parse( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
chat.unseenMessagesByUsers as unknown as string, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Object.keys(unseenMessagesByUsers).map((user: string) => { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if (user !== context.userId) { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
console.log("user", user, context.userId); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unseenMessagesByUsers[user] += 1; | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+55
to
+64
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Multiple improvements needed in unseen messages handling logic. The current implementation has several issues that need to be addressed:
Consider applying these improvements: - const unseenMessagesByUsers = JSON.parse(
- chat.unseenMessagesByUsers as unknown as string,
- );
-
- Object.keys(unseenMessagesByUsers).map((user: string) => {
- if (user !== context.userId) {
- console.log("user", user, context.userId);
- unseenMessagesByUsers[user] += 1;
- }
- });
+ let unseenMessagesByUsers;
+ try {
+ unseenMessagesByUsers = JSON.parse(chat.unseenMessagesByUsers);
+ if (!unseenMessagesByUsers || typeof unseenMessagesByUsers !== 'object') {
+ throw new Error('Invalid unseenMessagesByUsers structure');
+ }
+ } catch (error) {
+ throw new errors.ValidationError(
+ 'Invalid unseenMessagesByUsers format',
+ 'INVALID_UNSEEN_MESSAGES',
+ 'unseenMessagesByUsers'
+ );
+ }
+
+ // Use forEach instead of map for side effects
+ Object.keys(unseenMessagesByUsers).forEach((user: string) => {
+ if (user !== context.userId) {
+ unseenMessagesByUsers[user] += 1;
+ }
+ }); 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
// add createdDirectChatMessage to directChat | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
await Chat.updateOne( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@@ -61,6 +72,10 @@ export const sendMessageToChat: MutationResolvers["sendMessageToChat"] = async ( | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
$push: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
messages: createdChatMessage._id, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
$set: { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
unseenMessagesByUsers: JSON.stringify(unseenMessagesByUsers), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
updatedAt: now, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+75
to
+78
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using atomic operations for better concurrency handling. The current implementation might be susceptible to race conditions when multiple users send messages simultaneously. Consider using MongoDB's atomic operations. Here's a suggested improvement using atomic operations: - $set: {
- unseenMessagesByUsers: JSON.stringify(unseenMessagesByUsers),
- updatedAt: now,
- },
+ $set: {
+ updatedAt: now,
+ },
+ // Use dynamic field updates to atomically increment counters
+ ...Object.keys(unseenMessagesByUsers).reduce((acc, userId) => {
+ if (userId !== context.userId) {
+ acc[`unseenMessagesByUsers.${userId}`] = unseenMessagesByUsers[userId];
+ }
+ return acc;
+ }, {}), This approach:
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
}, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
); | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -1,5 +1,6 @@ | ||||||||||||||||||||||||
import type { QueryResolvers } from "../../types/generatedGraphQLTypes"; | ||||||||||||||||||||||||
import { Chat } from "../../models"; | ||||||||||||||||||||||||
import type { SortOrder } from "mongoose"; | ||||||||||||||||||||||||
/** | ||||||||||||||||||||||||
* This query will fetch all the Chats for the current user from the database. | ||||||||||||||||||||||||
* @param _parent- | ||||||||||||||||||||||||
|
@@ -13,11 +14,20 @@ export const chatsByUserId: QueryResolvers["chatsByUserId"] = async ( | |||||||||||||||||||||||
_parent, | ||||||||||||||||||||||||
args, | ||||||||||||||||||||||||
) => { | ||||||||||||||||||||||||
const sort = { | ||||||||||||||||||||||||
updatedAt: -1, | ||||||||||||||||||||||||
} as | ||||||||||||||||||||||||
| string | ||||||||||||||||||||||||
| { [key: string]: SortOrder | { $meta: unknown } } | ||||||||||||||||||||||||
| [string, SortOrder][] | ||||||||||||||||||||||||
| null | ||||||||||||||||||||||||
| undefined; | ||||||||||||||||||||||||
Comment on lines
+17
to
+24
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ 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:
📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||
|
||||||||||||||||||||||||
const chats = await Chat.find({ | ||||||||||||||||||||||||
users: args.id, | ||||||||||||||||||||||||
}).lean(); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
console.log(chats); | ||||||||||||||||||||||||
}) | ||||||||||||||||||||||||
.sort(sort) | ||||||||||||||||||||||||
.lean(); | ||||||||||||||||||||||||
|
||||||||||||||||||||||||
return chats; | ||||||||||||||||||||||||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -742,6 +742,7 @@ export const types = gql` | |
admins: [User] | ||
lastMessageId: String | ||
image: String | ||
unseenMessagesByUsers: JSON | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider using a strict type instead of JSON. Using the + type UnseenMessages {
+ userId: ID!
+ count: Int!
+ }
type Chat {
# ... other fields ...
- unseenMessagesByUsers: JSON
+ unseenMessagesByUsers: [UnseenMessages!]
} This provides several benefits:
|
||
} | ||
|
||
type ChatMessage { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,9 @@ export const createTestChat = async (): Promise< | |
createdAt: new Date(), | ||
updatedAt: new Date(), | ||
admins: [testUser._id], | ||
unseenMessagesByUsers: JSON.stringify({ | ||
[testUser._id]: 0, | ||
}), | ||
Comment on lines
+29
to
+31
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Add unseenMessagesByUsers initialization to all chat creation helpers and handle multiple users. The current implementation has several gaps:
Consider applying this pattern to all chat creation helpers: // Define a type for better type safety
type UnseenMessages = { [userId: string]: number };
// Helper function to create unseenMessagesByUsers
const createUnseenMessagesMap = (userIds: string[]): string => {
const unseenMessages: UnseenMessages = Object.fromEntries(
userIds.map(userId => [userId, 0])
);
return JSON.stringify(unseenMessages);
}; Then use it in chat creation: const testChat = await Chat.create({
creatorId: testUser._id,
users: [testUser._id],
organization: testOrganization._id,
isGroup: true,
createdAt: new Date(),
updatedAt: new Date(),
admins: [testUser._id],
- unseenMessagesByUsers: JSON.stringify({
- [testUser._id]: 0,
- }),
+ unseenMessagesByUsers: createUnseenMessagesMap([testUser._id]),
}); |
||
}); | ||
|
||
return [testUser, testOrganization, testChat]; | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ 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.
Add these type definitions: