fix: read receipts on clear chat requests #744
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
WalkthroughThe pull request introduces modifications across several files, primarily focusing on enhancing the functionality of chat and message handling. Key changes include optimizing the Changes
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (5)
convex/schema.ts (1)
40-44: Consider grouping similar edge types togetherWhile the edge definition is correct, consider moving it after the single edges (
userandprivateChat) to maintain a consistent pattern of grouping similar edge types together. This would improve code organization and readability.clearRequests: defineEnt({}) .field( "status", v.union( v.literal("pending"), v.literal("rejected"), v.literal("expired"), ), ) - .edges("readBy", { - to: "users", - inverseField: "readRequests", - table: "readRequests", - }) .edge("user") .edge("privateChat") + .edges("readBy", { + to: "users", + inverseField: "readRequests", + table: "readRequests", + })convex/clearRequests.ts (1)
Line range hint
134-162: Consider read receipt handling in request state changesThe
rejectClearRequestandacceptClearRequestmutations don't interact with the newreadByfield. Consider whether:
- Changing a request's status should affect its read status
- Historical read data should be preserved when clearing chat history
This might impact analytics or user experience features that rely on read receipt data.
src/components/message.tsx (3)
Line range hint
1-607: Consider breaking down the Message component for better maintainabilityThe component handles multiple responsibilities and could benefit from being split into smaller, focused components:
- MessageActions (context menu)
- MessageContent
- ReadReceipt
- ClearRequestActions
This would improve code organization and make it easier to maintain and test each piece independently.
Example structure:
// MessageActions.tsx export const MessageActions = ({ message, isOpen, position, onClose, onDelete, onEdit, onReply }: MessageActionsProps) => { return ( <Portal> <div className="rounded-sm border-2 border-secondary-foreground"> {/* Action buttons */} </div> </Portal> ); }; // Message.tsx export const Message = ({ message, ...props }) => { return ( <div className="flex"> <MessageContent message={message} /> <ReadReceipt message={message} /> <MessageActions message={message} /> {message.type !== 'message' && ( <ClearRequestActions message={message} /> )} </div> ); };
Line range hint
214-246: Simplify date formatting logic using dayjsThe
sentInfofunction has complex date formatting logic that could be simplified using dayjs's built-in formatting.- const sentInfo = () => { - return ( - <> - Sent at {dayjs(message._creationTime).hour()}: - {dayjs(message._creationTime).minute() < 10 - ? "0" + dayjs(message._creationTime).minute() - : dayjs(message._creationTime).minute()} - {", "} - {dayjs(message._creationTime).date() < 10 - ? "0" + dayjs(message._creationTime).date() - : dayjs(message._creationTime).date()} - . - {dayjs(message._creationTime).month() + 1 < 10 - ? "0" + (dayjs(message._creationTime).month() + 1).toString() - : dayjs(message._creationTime).month() + 1} - .{dayjs(message._creationTime).year()} - </> - ); - }; + const sentInfo = () => ( + <> + Sent at {dayjs(message._creationTime).format('HH:mm, DD.MM.YYYY')} + </> + );
Line range hint
67-89: Extract mobile detection logic to a custom hookThe mobile detection logic could be moved to a reusable custom hook for better code organization and reusability.
// hooks/useDeviceDetection.ts export const useDeviceDetection = () => { const [isMobile, setIsMobile] = useState(false); useEffect(() => { const checkIfMobile = () => { const userAgent = navigator.userAgent; setIsMobile( /android/i.test(userAgent) || /iPad|iPhone|iPod/.test(userAgent) ); }; checkIfMobile(); }, []); return { isMobile }; }; // Usage in Message.tsx const { isMobile } = useDeviceDetection();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
convex/chats.ts(2 hunks)convex/clearRequests.ts(1 hunks)convex/messages.ts(2 hunks)convex/schema.ts(2 hunks)src/components/message.tsx(1 hunks)
🔇 Additional comments (9)
convex/schema.ts (2)
24-28: LGTM: Well-structured edge definition for tracking read status
The new readRequests edge follows the established pattern and correctly establishes the many-to-many relationship between users and clear requests.
24-28: Verify schema usage in mutation handlers
The schema changes look good and properly establish the relationships needed for read receipts. Let's verify that the mutation handlers are correctly using these new edges.
Also applies to: 40-44
✅ Verification successful
Let me gather more context about the implementation details of these mutations.
Schema changes are properly utilized in mutation handlers
The schema relationships are correctly implemented in both mutation handlers:
createClearRequestinitializes thereadByfield with the creator's ID (line 60)markMessageReadproperly handles both messages and clearRequests, using thereadByfield to track read status (lines 223-225 and 241-243)- The edge relationships defined in the schema are being used correctly for querying (lines 45-46)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if mutation handlers properly use the new schema relationships
# Check for readBy usage in clearRequests mutations
echo "Checking clearRequests mutations..."
rg -A 5 "createClearRequest.*mutation" convex/
# Check for readRequests usage in message read mutations
echo "Checking message read mutations..."
rg -A 5 "markMessageRead.*mutation" convex/
Length of output: 942
Script:
#!/bin/bash
# Check the full implementation of both mutations to verify schema usage
echo "Checking createClearRequest implementation..."
ast-grep --pattern 'export const createClearRequest = mutation({
$$$
handler: async (ctx, args) => {
$$$
}
});'
echo "Checking markMessageRead implementation..."
ast-grep --pattern 'export const markMessageRead = mutation({
$$$
handler: async (ctx, args) => {
$$$
}
});'
Length of output: 6589
convex/clearRequests.ts (2)
60-60: Consider if creator should be marked as having read their own request
The readBy array is initialized with the creator's ID, implying they've read their own clear request. While this is logically sound (as creating implies awareness), you might want to verify if this aligns with the broader read receipt system's behavior.
Let's check how message creation handles read receipts:
✅ Verification successful
Marking clear requests as read by creator aligns with message behavior
The verification confirms that regular messages are also marked as read by their creator (via readBy: [convexUser._id]), making this implementation consistent with the broader read receipt system's behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if messages are marked as read by their creator
ast-grep --pattern 'await ctx.table("messages").insert({
$$$
readBy: $_,
$$$
})'
Length of output: 553
Line range hint 1-62: Verify schema constraints for the readBy field
The mutation looks correct and properly authenticated. However, since this adds a new relationship between users and clear requests, we should verify the schema constraints.
Let's check the schema definition:
✅ Verification successful
Schema constraints for readBy field are correctly defined
The schema definition shows that the readBy field in clearRequests is properly defined as a many-to-many edge relationship with users table through the readRequests join table. The relationship is bidirectional with readRequests as the inverse field in the users table. This matches the usage in the mutation where readBy is initialized as an array containing the requesting user's ID.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check schema definition for clearRequests table
rg -A 5 'clearRequests:.*{' convex/schema.ts
Length of output: 144
Script:
#!/bin/bash
# Let's try a simpler search pattern to find schema definitions
rg "clearRequests" convex/schema.ts
Length of output: 180
Script:
#!/bin/bash
# Let's also check the entire schema file content to understand the structure
cat convex/schema.ts
Length of output: 1938
Script:
#!/bin/bash
# Search for any files that might contain schema definitions
fd schema.ts
Length of output: 31
convex/chats.ts (2)
Line range hint 121-154: Overall implementation looks solid
The changes effectively implement read receipts for clear chat requests while maintaining proper handling of regular messages. The parallel fetching optimization in the clear requests handling is a nice performance improvement.
Key improvements made:
- Efficient parallel fetching of read status data
- Proper differentiation between message types for read status
- Accurate counting of unread messages considering deletion status
147-154: Verify read status logic for deleted messages
The current implementation treats read status differently for messages vs. clear requests. Please confirm if this is the intended behavior:
- Regular messages: Considered unread if deleted
- Clear requests: Read status independent of deletion
Let's verify this behavior across the codebase:
Consider making the logic more explicit:
- if (message.type === "message" && message.deleted) {
+ const isDeletedMessage = message.type === "message" && message.deleted;
+ if (isDeletedMessage) {
deletedCount++;
}
const isReadMessage =
message.readBy.some(
(user) => user.clerkId === identity.tokenIdentifier,
) &&
- (message.type !== "message" || !message.deleted);
+ (!isDeletedMessage || message.type !== "message");✅ Verification successful
Read status logic for deleted messages is correctly implemented
The codebase consistently shows that:
- Regular messages (
type === "message"):- Have separate read/sent/sending status display logic when not deleted
- Cannot be replied to when deleted
- Cannot be edited when deleted
- Clear requests:
- Are tracked in a separate schema with their own read status
- Don't have deletion-related constraints on read status
The current implementation aligns with these patterns, where deleted regular messages are treated as unread while clear requests maintain their read status regardless of deletion state.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for related message read status handling
rg -l "readBy|deleted.*message" --type ts
# Check for any similar patterns in tests
rg -l "test.*message.*read|test.*deleted" --type ts
Length of output: 282
Script:
#!/bin/bash
# Check message read status handling in relevant files
echo "=== Checking message.tsx ==="
rg "readBy|deleted.*message" src/components/message.tsx -C 2
echo -e "\n=== Checking messages.ts ==="
rg "readBy|deleted.*message" convex/messages.ts -C 2
echo -e "\n=== Checking schema.ts ==="
rg "readBy|deleted.*message" convex/schema.ts -C 2
echo -e "\n=== Checking clearRequests.ts ==="
rg "readBy|deleted.*message" convex/clearRequests.ts -C 2
Length of output: 3003
convex/messages.ts (2)
191-191: LGTM: Type-safe union for handling both message and clear request IDs
The signature change correctly uses v.union() to handle both types of IDs while maintaining type safety.
Line range hint 191-248: Verify integration points and schema support
Let's verify that all callers are updated and the schema supports these changes.
✅ Verification successful
Schema and usage are properly aligned with the changes
The verification confirms that:
- The schema properly supports
readByfor bothmessagesandclearRequeststables through edge definitions with appropriate inverse fields - The caller in
src/components/message.tsxdoesn't rely on any return value frommarkMessageRead, so it's compatible with the changes - Both tables use the same edge structure for
readBy, making the union type in the mutation's args appropriate
The implementation is consistent with the schema design and existing usage patterns in the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify callers and schema support for markMessageRead changes
# Check for markMessageRead callers
echo "Checking markMessageRead callers..."
rg -A 2 "markMessageRead" --type ts
# Check schema for clearRequests table
echo "Checking schema definition..."
rg "clearRequests.*readBy" --type ts
Length of output: 657
Script:
#!/bin/bash
# Let's check the schema definition for clearRequests table
echo "Checking clearRequests schema..."
ast-grep --pattern 'export const clearRequests = defineTable({
$$$
})'
# Let's also check the messages table schema for comparison
echo "Checking messages schema..."
ast-grep --pattern 'export const messages = defineTable({
$$$
})'
Length of output: 284
Script:
#!/bin/bash
# Let's try a different approach to find schema definitions
echo "Searching for schema definitions..."
rg "defineTable.*clearRequests" -B 1 -A 10 --type ts
echo "Searching for readBy field usage in schema..."
rg "readBy.*:" -B 2 -A 2 --type ts
# Also check for any files that might contain schema definitions
echo "Looking for schema files..."
fd "schema" --type f
Length of output: 2104
Script:
#!/bin/bash
# Let's check the schema.ts file content
echo "Checking schema.ts content..."
cat convex/schema.ts
# Let's also check for any return type handling in the message component
echo "Checking message component handling..."
rg -A 5 "markRead\(" --type ts
Length of output: 2454
src/components/message.tsx (1)
Line range hint 193-198: LGTM: Simplified read receipt logic
The simplified condition for marking messages as read now correctly handles both regular messages and clear chat requests, which aligns with the PR objectives.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
convex/messages.ts (1)
191-191: Add return type safetyConsider adding a return type to the mutation for better type safety and consistency.
export const markMessageRead = mutation({ - args: { messageId: v.union(v.id("messages"), v.id("clearRequests")) }, + args: { messageId: v.union(v.id("messages"), v.id("clearRequests")) }, + returns: v.union(v.null(), v.object({ success: v.boolean() })), handler: async (ctx, args) => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
convex/messages.ts(2 hunks)
🔇 Additional comments (2)
convex/messages.ts (2)
191-191: LGTM: Well-structured type definition
The union type correctly represents that the function can handle both message and clear request IDs, aligning with the PR's objective.
234-264: LGTM: Well-structured clear request handling
The clear request handling logic is well-implemented with proper:
- ID normalization
- Error handling
- Authorization checks
- Read status updates
| const chat = await ctx.table("privateChats").getX(message.privateChatId); | ||
| const usersInChat = await chat.edge("users"); | ||
| if ( | ||
| !usersInChat.some((user) => user.clerkId === identity.tokenIdentifier) | ||
| ) { | ||
| throw new ConvexError( | ||
| "User not authorized to mark messages in this chat", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Reduce code duplication in authorization checks
The authorization check logic is duplicated between message and clear request handling. Consider extracting it into a reusable function.
+ async function validateUserInChat(ctx, chatId, identity) {
+ const chat = await ctx.table("privateChats").getX(chatId);
+ const usersInChat = await chat.edge("users");
+ if (!usersInChat.some((user) => user.clerkId === identity.tokenIdentifier)) {
+ throw new ConvexError("User not authorized to mark messages in this chat");
+ }
+ return chat;
+ }
// In message handling:
- const chat = await ctx.table("privateChats").getX(message.privateChatId);
- const usersInChat = await chat.edge("users");
- if (!usersInChat.some((user) => user.clerkId === identity.tokenIdentifier)) {
- throw new ConvexError("User not authorized to mark messages in this chat");
- }
+ await validateUserInChat(ctx, message.privateChatId, identity);
// In clear request handling:
- const chat = await ctx.table("privateChats").getX(request.privateChatId);
- const usersInChat = await chat.edge("users");
- if (!usersInChat.some((user) => user.clerkId === identity.tokenIdentifier)) {
- throw new ConvexError("User not authorized to mark messages in this chat");
- }
+ await validateUserInChat(ctx, request.privateChatId, identity);Also applies to: 244-254
| if (!message) { | ||
| return null; | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling
For consistency with the clear request handling (line 262), consider returning { success: false } instead of null when the message is not found.
if (!message) {
- return null;
+ return { success: false };
}📝 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 (!message) { | |
| return null; | |
| } | |
| if (!message) { | |
| return { success: false }; | |
| } |
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Style