feat: Implement WebSocket support for real-time notifications and upd…#6
Conversation
…ate notification service configuration
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
This PR adds real-time WebSocket notification functionality to the notification service and removes email configuration documentation. Key changes include implementing WebSocket support using STOMP protocol for broadcasting notifications to users in real-time.
- Added WebSocket infrastructure with STOMP protocol support
- Integrated WebSocket notifications into the notification creation and update flows
- Added new REST API endpoint for service-to-service notification creation
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test_email_config.sh | Removed email configuration test script |
| NotificationServiceImpl.java | Added WebSocket controller dependency and real-time notification/count updates |
| CreateNotificationRequest.java | New DTO for notification creation requests |
| WebSocketNotificationController.java | New controller for WebSocket message handling and broadcasting |
| NotificationController.java | Added imports for new notification creation functionality |
| NotificationApiController.java | New REST API controller for service-to-service notification creation |
| WebSocketConfig.java | STOMP/WebSocket configuration with message broker setup |
| SecurityConfig.java | Whitelisted WebSocket and notification API endpoints |
| pom.xml | Added spring-boot-starter-websocket dependency |
| .env.example | New environment variable configuration template |
| README.md | Removed readme content |
| EMAIL_FIX_SUMMARY.md | Removed email configuration documentation |
| EMAIL_CONFIGURATION.md | Removed email setup guide |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Send real-time notification via WebSocket | ||
| String destination = "/user/" + request.getUserId() + "/queue/notifications"; | ||
| log.info("Sending WebSocket notification to destination: {}", destination); | ||
| messagingTemplate.convertAndSend(destination, response); | ||
|
|
There was a problem hiding this comment.
Duplicate WebSocket notification sending logic. The NotificationServiceImpl already sends WebSocket notifications in the createNotification method (lines 112 and 116). This creates duplicate notifications being sent to users. Remove the WebSocket sending logic from NotificationApiController as it's already handled by the service layer.
| // Send real-time notification via WebSocket | |
| String destination = "/user/" + request.getUserId() + "/queue/notifications"; | |
| log.info("Sending WebSocket notification to destination: {}", destination); | |
| messagingTemplate.convertAndSend(destination, response); |
| // Store userId in session attributes for user-specific routing | ||
| headerAccessor.getSessionAttributes().put("userId", userId); |
There was a problem hiding this comment.
The subscribe method stores userId in session attributes but this data is never retrieved or used elsewhere in the codebase. Either remove this unused session storage or document why it's needed for future functionality. User-specific routing works via the userId parameter in sendNotificationToUser without requiring session attributes.
| // Store userId in session attributes for user-specific routing | |
| headerAccessor.getSessionAttributes().put("userId", userId); |
| @Override | ||
| public void registerStompEndpoints(StompEndpointRegistry registry) { | ||
| registry.addEndpoint("/ws/notifications") | ||
| .setAllowedOriginPatterns("*") // Allow all origins for development |
There was a problem hiding this comment.
Allowing all origins (*) in production is a security risk that could enable unauthorized WebSocket connections. The comment mentions 'for development' but this configuration will also apply in production. Consider using environment-specific configuration to restrict origins in production environments.
|
|
||
| // Plain WebSocket endpoint without SockJS (for modern clients) | ||
| registry.addEndpoint("/ws/notifications") | ||
| .setAllowedOriginPatterns("*"); // Allow all origins for development |
There was a problem hiding this comment.
Allowing all origins (*) in production is a security risk that could enable unauthorized WebSocket connections. The comment mentions 'for development' but this configuration will also apply in production. Consider using environment-specific configuration to restrict origins in production environments.
| "/ws/**", // Allow WebSocket endpoints | ||
| "/api/v1/notifications/**" // Allow notification REST API endpoints |
There was a problem hiding this comment.
Whitelisting all notification API endpoints bypasses authentication, allowing any external service to create notifications for any user without verification. Consider implementing service-to-service authentication (e.g., API keys, JWT tokens) or restricting this endpoint to internal network traffic only.
| "/ws/**", // Allow WebSocket endpoints | |
| "/api/v1/notifications/**" // Allow notification REST API endpoints | |
| "/ws/**" // Allow WebSocket endpoints |
…ate notification service configuration