Skip to content

Conversation

@christinaw4848
Copy link

Issues

  • Implement a GET /chats/:recipientId route that retrieves messages where the current user is either the sender or receiver.

Changes

  • New chat controller and route set up

Copilot AI review requested due to automatic review settings January 29, 2026 23:25
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR implements a real-time chat backend feature with multiple endpoints for managing chat messages between users. The implementation follows the existing codebase patterns using in-memory storage as a prototype before database integration.

Changes:

  • Added GET /chats/:recipientId endpoint to retrieve messages between two users
  • Added GET /chats endpoint to retrieve all messages (not mentioned in PR description)
  • Added POST /chats/:recipientId endpoint to send messages (not mentioned in PR description)

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.

File Description
src/routes/chats.routes.ts Defines three new routes for chat functionality: getAllChats, getMessages, and sendMessage
src/controllers/chats.controller.ts Implements chat controller with in-memory message storage and handlers for retrieving and creating messages
src/index.ts Registers the new chat router at /api/chats endpoint

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

// TODO: add Clerk requireAuth middleware to all routes

// Get messages between current user and recipient
router.get('/', getAllChats);
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The POST route pattern '/:recipientId' is inconsistent with other POST routes in the codebase. Other controllers use POST '/' for creating resources (users.routes.ts:14, appointments.routes.ts:14, documents.routes.ts:15). While the current pattern may be semantically meaningful for sending a message to a recipient, it deviates from the established REST convention where POST to a collection endpoint creates a new resource. Consider using POST '/' and including recipientId in the request body instead.

Copilot uses AI. Check for mistakes.
Comment on lines +66 to +69
(req.body.currentUserId as string);

if (!currentUserId) {
return res.status(400).json({ message: 'currentUserId required' });
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function attempts to read currentUserId from req.body on a GET request, but GET requests typically don't have a request body. While Express will parse it if sent, this is non-standard and could lead to confusion. Consider removing req.body.currentUserId from the fallback chain for GET requests, or document why it's needed for this specific use case.

Copilot uses AI. Check for mistakes.
*/
export const getAllChats = async (req: Request, res: Response) => {
// TODO: add filtering, search, pagination
res.json(messages);
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The function getAllChats returns all individual messages, but the name suggests it would return all chat conversations/threads. This is misleading. Consider renaming to getAllMessages to match what the function actually returns, or refactor the function to return a list of unique conversations grouped by participants.

Suggested change
res.json(messages);
// Group messages into conversations keyed by participant pairs.
const conversationsMap = new Map<
string,
{
participantIds: string[];
messages: typeof messages;
}
>();
for (const message of messages) {
// Use a stable key based on the unordered pair of participants.
const participants = [message.senderId, message.recipientId].sort();
const key = participants.join(':');
let conversation = conversationsMap.get(key);
if (!conversation) {
conversation = {
participantIds: participants,
messages: [],
};
conversationsMap.set(key, conversation);
}
conversation.messages.push(message);
}
res.json(Array.from(conversationsMap.values()));

Copilot uses AI. Check for mistakes.
} from '../controllers/chats.controller.js';

const router = Router();

Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR description only mentions implementing "GET /chats/:recipientId", but the actual implementation includes additional routes: GET /chats (getAllChats) and POST /chats/:recipientId (sendMessage). While these additions may be useful, they should be documented in the PR description to accurately reflect the scope of changes.

Copilot uses AI. Check for mistakes.
{
id: 'm3',
senderId: '1',
recipientId: '3',
Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The message with id 'm3' has recipientId set to '3', but based on the context and the text "Okay, thank you!" it appears to be a continuation of the conversation between user '1' and user '2'. The recipientId should be '2' to maintain conversation consistency.

Suggested change
recipientId: '3',
recipientId: '2',

Copilot uses AI. Check for mistakes.
Comment on lines +100 to +101
}

Copy link

Copilot AI Jan 29, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation for 'text' only checks if it's falsy, which would allow whitespace-only strings (e.g., " ") to be accepted as valid message text. Consider trimming the text and validating that it's not empty to prevent creation of messages with no meaningful content.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant