Implement private direct messaging between users#31
Conversation
…and WebSocket support Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
…dicators, WebSocket subscription Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
…Read, cleaner JS Co-authored-by: dmccoystephenson <21204351+dmccoystephenson@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This pull request implements private direct messaging (DM) functionality for the Accord chat application, allowing one-on-one conversations between users separate from public channels. The implementation adds backend entities, services, and REST endpoints for DM management, along with WebSocket integration for real-time message delivery. The frontend adds a user list sidebar, DM conversation views, and unread message indicators.
Changes:
- Added DirectMessage entity with repository, service, and controller layers including REST APIs for sending, retrieving, and marking messages as read
- Extended WebSocket configuration to support user-specific message queues for real-time DM delivery
- Enhanced frontend with DM UI components, WebSocket subscriptions, and conversation management
- Added comprehensive test coverage with 26 new tests across entity, service, and controller layers
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| backend/src/main/java/com/accord/model/DirectMessage.java | New entity with sender/recipient IDs, content, timestamp, and read status |
| backend/src/main/java/com/accord/repository/DirectMessageRepository.java | JPQL queries for conversations, unread counts, and batch mark-as-read operations |
| backend/src/main/java/com/accord/service/DirectMessageService.java | Business logic for sending, retrieving, and marking DMs as read |
| backend/src/main/java/com/accord/controller/DirectMessageController.java | REST endpoints for DM operations with validation and WebSocket notifications |
| backend/src/main/java/com/accord/controller/UserController.java | Added endpoint to list all users for the DM panel |
| backend/src/main/java/com/accord/service/UserService.java | Added method to retrieve all users |
| backend/src/main/java/com/accord/config/WebSocketConfig.java | Enabled /user broker destination for user-specific messaging |
| webapp/src/main/resources/templates/index.html | Modified login to store token and userId from auth response |
| webapp/src/main/resources/templates/chat.html | Added DM UI components, WebSocket subscriptions, and conversation management |
| backend/src/test/java/com/accord/model/DirectMessageTest.java | Entity tests covering constructors, setters, and field behavior |
| backend/src/test/java/com/accord/service/DirectMessageServiceTest.java | Service layer tests for all operations including edge cases |
| backend/src/test/java/com/accord/controller/DirectMessageControllerTest.java | Controller tests validating endpoints, error handling, and WebSocket dispatch |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @Autowired | ||
| private DirectMessageRepository directMessageRepository; | ||
|
|
||
| public DirectMessage sendMessage(Long senderId, Long recipientId, String content) { |
There was a problem hiding this comment.
The DirectMessageService.sendMessage method does not validate that the senderId and recipientId are different. While the controller does check this (line 52-54 in DirectMessageController), the service layer should also enforce this business rule for defense in depth. If the service is called from another context in the future, self-messaging could be allowed unintentionally.
Add a check in the service method: if (senderId.equals(recipientId)) throw new IllegalArgumentException("Cannot send message to self");
| public DirectMessage sendMessage(Long senderId, Long recipientId, String content) { | |
| public DirectMessage sendMessage(Long senderId, Long recipientId, String content) { | |
| if (senderId != null && senderId.equals(recipientId)) { | |
| throw new IllegalArgumentException("Cannot send message to self"); | |
| } |
| .andExpect(jsonPath("$.recipientId").value(2)) | ||
| .andExpect(jsonPath("$.content").value("Hello Bob")); | ||
|
|
||
| verify(messagingTemplate).convertAndSend(eq("/user/2/queue/messages"), any(DirectMessage.class)); |
There was a problem hiding this comment.
The test verifies that messagingTemplate.convertAndSend is called with the correct destination path, but this is a unit test with mocked dependencies. It doesn't verify that the message can actually be delivered through Spring's WebSocket user destination mechanism.
The actual runtime behavior will fail because the destination uses user ID ("2") but Spring matches against username principals (e.g., "bob"). Consider adding an integration test that establishes a real WebSocket connection with authentication to verify end-to-end message delivery.
| verify(messagingTemplate).convertAndSend(eq("/user/2/queue/messages"), any(DirectMessage.class)); | |
| verify(messagingTemplate).convertAndSend(eq("/user/bob/queue/messages"), any(DirectMessage.class)); |
| messages.forEach(msg => displayDmMessage(msg)); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error loading DM conversation:', error); |
There was a problem hiding this comment.
The conversation loading endpoint doesn't handle errors gracefully. If the fetch fails or returns a non-200 status, the error is only logged to console and the conversation view will be empty. Users won't know if the conversation failed to load due to a network error or server issue.
Add user-visible error handling, such as displaying an error message in the chat area when conversation loading fails.
| messages.forEach(msg => displayDmMessage(msg)); | |
| } | |
| } catch (error) { | |
| console.error('Error loading DM conversation:', error); | |
| messages.forEach(msg => displayDmMessage(msg)); | |
| } else { | |
| console.error('Failed to load DM conversation. Status:', response.status, response.statusText); | |
| alert('Unable to load this conversation right now. Please try again later.'); | |
| } | |
| } catch (error) { | |
| console.error('Error loading DM conversation:', error); | |
| alert('An error occurred while loading this conversation. Please check your connection and try again.'); |
| function displayDmMessage(message) { | ||
| const messagesDiv = document.getElementById('messages'); | ||
|
|
||
| // Find username from sender/recipient IDs | ||
| const senderUser = allUsers.find(u => u.id === message.senderId); | ||
| const msgUsername = senderUser ? senderUser.username : 'Unknown'; | ||
|
|
||
| const messageDiv = document.createElement('div'); | ||
| messageDiv.className = 'message'; | ||
|
|
||
| const time = new Date(message.timestamp).toLocaleTimeString('en-US', { | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| second: '2-digit' | ||
| }); | ||
|
|
||
| messageDiv.innerHTML = ` | ||
| <div class="message-header"> | ||
| <span class="message-username">${escapeHtml(msgUsername)}</span> | ||
| <span class="message-time">${time}</span> | ||
| </div> | ||
| <div class="message-content">${escapeHtml(message.content)}</div> | ||
| `; | ||
|
|
||
| messagesDiv.appendChild(messageDiv); | ||
| messagesDiv.scrollTop = messagesDiv.scrollHeight; | ||
| } |
There was a problem hiding this comment.
The displayDmMessage function doesn't implement duplicate message detection and count badges that the displayMessage function has (see lines 644-685). This creates an inconsistent user experience where duplicate messages in channels are collapsed with a count badge, but duplicate DMs are shown as separate messages.
Consider implementing the same duplicate detection logic in displayDmMessage for consistency, or document why different behavior is intentional for DMs.
| // Subscribe to user-specific DM queue | ||
| if (currentUserId) { | ||
| dmSubscription = stompClient.subscribe(`/user/${currentUserId}/queue/messages`, onDmReceived); |
There was a problem hiding this comment.
The frontend subscribes to /user/${currentUserId}/queue/messages using a numeric user ID, but this won't work with Spring's user destination resolution. Spring matches subscriptions against the authenticated principal name, which is the username (not the numeric ID).
The subscription should use the username instead: stompClient.subscribe('/user/' + username + '/queue/messages', onDmReceived). Spring will automatically route messages to the correct session based on the authenticated username, making the userId unnecessary for the subscription path.
| // Subscribe to user-specific DM queue | |
| if (currentUserId) { | |
| dmSubscription = stompClient.subscribe(`/user/${currentUserId}/queue/messages`, onDmReceived); | |
| // Subscribe to user-specific DM queue (Spring routes by username/principal, not numeric ID) | |
| if (username) { | |
| dmSubscription = stompClient.subscribe(`/user/${username}/queue/messages`, onDmReceived); |
| config.enableSimpleBroker("/topic"); | ||
| config.enableSimpleBroker("/topic", "/user"); | ||
| config.setApplicationDestinationPrefixes("/app"); | ||
| config.setUserDestinationPrefix("/user"); |
There was a problem hiding this comment.
The WebSocket configuration sets the user destination prefix to "/user" explicitly with setUserDestinationPrefix("/user"), but this is actually the default value in Spring. While this is not incorrect, it's redundant. The configuration could be simplified by removing line 27, as Spring WebSocket already defaults to "/user" for user destinations.
This is a minor code cleanliness issue and does not affect functionality.
| config.setUserDestinationPrefix("/user"); |
| // Subscribe to DM queue now that we have userId | ||
| if (stompClient && connected) { | ||
| dmSubscription = stompClient.subscribe(`/user/${currentUserId}/queue/messages`, onDmReceived); |
There was a problem hiding this comment.
Same issue as line 537 - the subscription uses userId instead of username. This should be stompClient.subscribe('/user/' + username + '/queue/messages', onDmReceived) to properly receive messages through Spring's user destination mechanism.
| // Subscribe to DM queue now that we have userId | |
| if (stompClient && connected) { | |
| dmSubscription = stompClient.subscribe(`/user/${currentUserId}/queue/messages`, onDmReceived); | |
| // Subscribe to DM queue using Spring's user destination mechanism (username-based) | |
| if (stompClient && connected) { | |
| dmSubscription = stompClient.subscribe('/user/' + username + '/queue/messages', onDmReceived); |
|
|
||
| @Column(nullable = false) | ||
| private Long recipientId; | ||
|
|
There was a problem hiding this comment.
The DirectMessage entity does not have any validation constraints on the senderId and recipientId fields to ensure they reference valid users. While the controller validates that users exist before saving messages, the entity could benefit from referential integrity if these were @manytoone relationships to User entities, or at minimum @column(nullable = false) constraints (which are present).
Consider whether foreign key constraints should be added at the database level to ensure data integrity, preventing orphaned messages if users are deleted. This would require a decision on the deletion cascade behavior (e.g., should messages be deleted when a user is deleted, or should the user deletion be prevented if they have messages?).
| @ManyToOne(fetch = FetchType.LAZY) | |
| @JoinColumn(name = "sender_id", insertable = false, updatable = false, | |
| foreignKey = @ForeignKey(name = "fk_direct_message_sender")) | |
| private User sender; | |
| @ManyToOne(fetch = FetchType.LAZY) | |
| @JoinColumn(name = "recipient_id", insertable = false, updatable = false, | |
| foreignKey = @ForeignKey(name = "fk_direct_message_recipient")) | |
| private User recipient; |
| function onDmReceived(payload) { | ||
| const message = JSON.parse(payload.body); | ||
|
|
||
| // Find the sender's username from allUsers | ||
| const senderUser = allUsers.find(u => u.id === message.senderId); | ||
| const senderName = senderUser ? senderUser.username : 'Unknown'; | ||
|
|
There was a problem hiding this comment.
When displaying a DM message from the sender's perspective (after sending via REST API), the message is displayed using displayDmMessage. However, when receiving a DM via WebSocket, the code finds the sender from allUsers array (line 578). If the sender's data is not in allUsers (e.g., due to a race condition or cache inconsistency), the username will show as 'Unknown'.
Consider handling this edge case more robustly, perhaps by fetching the user details if not found in the local cache, or ensuring allUsers is always up-to-date before processing DMs.
| function onDmReceived(payload) { | |
| const message = JSON.parse(payload.body); | |
| // Find the sender's username from allUsers | |
| const senderUser = allUsers.find(u => u.id === message.senderId); | |
| const senderName = senderUser ? senderUser.username : 'Unknown'; | |
| async function onDmReceived(payload) { | |
| const message = JSON.parse(payload.body); | |
| // Try to find the sender's user object from the local cache | |
| let senderUser = allUsers.find(u => u.id === message.senderId); | |
| // If not found, refresh the users list and try again before falling back | |
| if (!senderUser) { | |
| try { | |
| await loadUsers(); | |
| senderUser = allUsers.find(u => u.id === message.senderId); | |
| } catch (e) { | |
| console.error('Failed to refresh users for DM sender lookup:', e); | |
| } | |
| } | |
| // Derive a sender name, preferring cached data; fall back defensively if needed | |
| const senderName = senderUser | |
| ? senderUser.username | |
| : (message.senderUsername || 'Unknown'); | |
| async function openDmConversation(recipientUsername) { | ||
| isDmMode = true; | ||
| dmRecipient = recipientUsername; | ||
|
|
||
| // Update header | ||
| document.getElementById('currentChannelName').textContent = '@ ' + recipientUsername; | ||
|
|
||
| // Clear messages | ||
| document.getElementById('messages').innerHTML = ''; | ||
| lastMessage = null; | ||
|
|
||
| // Clear unread count for this user | ||
| unreadCounts[recipientUsername] = 0; | ||
|
|
||
| // Update sidebar highlights | ||
| displayChannels(); | ||
| displayUsers(); | ||
|
|
||
| // Update input placeholder | ||
| document.getElementById('messageInput').placeholder = 'Message @' + recipientUsername + '...'; | ||
|
|
||
| // Load DM conversation | ||
| try { | ||
| const response = await fetch( | ||
| `${backendUrl}/api/dm/conversation?user1=${encodeURIComponent(username)}&user2=${encodeURIComponent(recipientUsername)}&limit=50`, { | ||
| headers: { | ||
| 'Authorization': 'Bearer ' + sessionStorage.getItem('token') | ||
| } | ||
| }); | ||
| if (response.ok) { | ||
| const messages = await response.json(); | ||
| messages.forEach(msg => displayDmMessage(msg)); | ||
| } | ||
| } catch (error) { | ||
| console.error('Error loading DM conversation:', error); | ||
| } | ||
|
|
||
| // Mark conversation as read | ||
| try { | ||
| await fetch(`${backendUrl}/api/dm/read/conversation`, { | ||
| method: 'POST', | ||
| headers: { | ||
| 'Content-Type': 'application/json', | ||
| 'Authorization': 'Bearer ' + sessionStorage.getItem('token') | ||
| }, | ||
| body: JSON.stringify({ | ||
| recipientUsername: username, | ||
| senderUsername: recipientUsername | ||
| }) | ||
| }); | ||
| } catch (error) { | ||
| console.error('Error marking conversation as read:', error); | ||
| } |
There was a problem hiding this comment.
The unread count is incremented locally when a DM arrives but the user is not viewing that conversation. However, when opening a DM conversation (line 837), the unread count is cleared immediately before the mark-as-read API call completes. If the API call fails, the unread count will be incorrectly cleared in the UI while messages remain unread on the server.
Consider clearing the unread count only after the mark-as-read API call successfully completes, or implement a retry mechanism if the call fails. This ensures the UI state remains consistent with the server state.
dmccoystephenson
changed the base branch from
copilot/add-user-authentication-system
to
develop
# Conflicts: # webapp/src/main/resources/templates/chat.html # webapp/src/main/resources/templates/index.html
dmccoystephenson
left a comment
dmccoystephenson
left a comment
There was a problem hiding this comment.
Unable to log in at the moment, I am just redirected back to the login page.
Adds one-on-one direct messaging separate from public channels, with real-time delivery via user-specific WebSocket destinations and read receipts.
Backend
DirectMessageentity —id,senderId,recipientId,content,timestamp,read(defaults false)DirectMessageRepository— JPQL queries for conversations, batch@Modifyingupdate for mark-as-read, unread counts per senderDirectMessageService— send, retrieve conversation (paginated, oldest-first), mark read (single + batch), unread countsDirectMessageController— REST endpoints under/api/dm/:POST /send— persists DM and pushes to recipient viaSimpMessagingTemplateGET /conversation?user1=&user2=&limit=— paginated conversation historyPOST /read/{messageId},POST /read/conversation— read receiptsGET /unread,GET /unread/from— unread counts/userprefix alongside/topicfor user-specific destinations (/user/{userId}/queue/messages)GET /api/users— new endpoint returning[{id, username}]for the user list panelFrontend (webapp)
/user/{userId}/queue/messageson connect for real-time deliverytokenanduserIdfrom auth responseTests (26 new)
DirectMessageTest— entity construction and field behaviorDirectMessageServiceTest— service methods including batch mark-as-readDirectMessageControllerTest— endpoint validation, error cases, WebSocket dispatch verificationOriginal prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.