Conversation
알림 시스템을 업데이트하여 페이로드 구성에 DTO를 사용하도록 함으로써 데이터 전송을 더욱 깔끔하게 개선했습니다. 이 변경 사항은 기존의 이벤트 기반 페이로드를 클라이언트에 필요한 정보를 캡슐화하는 전용 `NotificationItemResponse` DTO로 대체합니다.
|
Caution Review failedThe pull request is closed. Walkthrough알림 시스템의 데이터 표현을 Changes
Sequence DiagramsequenceDiagram
participant Client
participant NotificationDispatcher
participant SseEmitterService
participant NotificationRepository
participant Database
participant NotificationService
rect rgba(240, 248, 255, 0.3)
Note over NotificationDispatcher,Database: 기존: NotificationEvent 페이로드
NotificationDispatcher->>NotificationRepository: 알림 조회
NotificationRepository->>Database: findNotificationList()
Database-->>NotificationRepository: 알림 데이터 반환
NotificationRepository-->>NotificationDispatcher: NotificationResponse 목록
NotificationDispatcher->>SseEmitterService: sendNotificationIfConnected(userId, NotificationEvent)
SseEmitterService->>Client: "notification" 이벤트 전송
end
rect rgba(152, 251, 152, 0.3)
Note over NotificationDispatcher,Database: 변경: NotificationItemResponse 페이로드
NotificationDispatcher->>NotificationRepository: 알림 + 그룹 정보 조회
NotificationRepository->>Database: findNotificationList() (그룹 조인 포함)
Database-->>NotificationRepository: 통합 알림 데이터 반환
NotificationRepository-->>NotificationDispatcher: NotificationItemResponse 목록
NotificationDispatcher->>NotificationDispatcher: NotificationItemResponse.of() 생성
NotificationDispatcher->>SseEmitterService: sendNotificationIfConnected(userId, NotificationItemResponse)
SseEmitterService->>Client: "notification" 이벤트 전송<br/>(사용자 및 그룹 정보 포함)
end
rect rgba(255, 228, 225, 0.3)
Note over NotificationService,Client: 목록 조회 응답
Client->>NotificationService: 알림 목록 요청
NotificationService->>NotificationRepository: findNotificationList()
NotificationRepository->>Database: 쿼리 실행
Database-->>NotificationRepository: NotificationItemResponse 목록
NotificationRepository-->>NotificationService: 결과 반환
NotificationService->>Client: NotificationListResponse<br/>(List<NotificationItemResponse> 포함)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (9)
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 |
github-project-automation
bot
moved this from In progress
to Done
in WeGo-Together Backend
There was a problem hiding this comment.
Pull request overview
This PR refactors the notification system to use a dedicated NotificationItemResponse DTO instead of the event-based payload approach, improving data encapsulation and consistency for client responses. The new DTO provides a cleaner structure with nested UserSummary and GroupSummary objects and maps internal notification types to client-facing string values.
Key Changes:
- Introduced
NotificationItemResponseDTO with nested summary objects for user and group data - Added
NotificationTypeMapperto convert enum values to client-friendly string representations - Updated repository query to use QueryDSL projections and join group data when notifications are group-related
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| NotificationItemResponse.java | New DTO with QueryDSL projection constructor and factory method to encapsulate notification data with nested user/group summaries |
| NotificationTypeMapper.java | New mapper utility to convert NotificationType enum to client-facing string values (e.g., "group-join") |
| NotificationRepositoryImpl.java | Updated query to project directly into NotificationItemResponse, added left join for group data, renamed method for clarity |
| NotificationRepositoryCustom.java | Changed return type from NotificationResponse to NotificationItemResponse |
| NotificationListResponse.java | Updated to use NotificationItemResponse list instead of NotificationResponse |
| NotificationDispatcher.java | Modified to create and send NotificationItemResponse payload via SSE instead of NotificationEvent |
| SseEmitterService.java | Added generic sendNotificationIfConnected(Long, Object) overload to support different payload types |
| NotificationService.java | Updated to work with NotificationItemResponse instead of NotificationResponse |
| AuthService.java | Removed outdated comment about user deletion order |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| .from(notification) | ||
| .leftJoin(notification.actor, user) | ||
| .leftJoin(groupV2).on( | ||
| notification.relatedType.eq("GROUP") |
There was a problem hiding this comment.
The hardcoded string "GROUP" in the query condition should be extracted as a constant or enum value. This magic string creates a tight coupling with the database schema and makes the code harder to maintain. Consider defining this as a constant in a shared location or using an enum.
| public boolean sendNotificationIfConnected(Long userId, Object payload) { | ||
| SseEmitter emitter = emitters.get(userId); | ||
| if (emitter == null) return false; | ||
|
|
||
| try { | ||
| emitter.send(SseEmitter.event().name("notification").data(payload)); | ||
| return true; | ||
| } catch (IOException e) { | ||
| emitters.remove(userId); | ||
| return false; | ||
| } | ||
| } |
There was a problem hiding this comment.
The new generic sendNotificationIfConnected(Long userId, Object payload) method lacks proper logging compared to the existing NotificationEvent overload. The existing method logs debug information about the notification ID and provides more detailed logging on send failures. Consider adding similar logging to maintain consistent observability across both methods.
| NotificationItemResponse payload = NotificationItemResponse.of(n, actor, group); | ||
|
|
||
| boolean ok = sseEmitterService.sendNotificationIfConnected(receiverId, payload); |
There was a problem hiding this comment.
This PR introduces inconsistency in how notifications are sent via SSE. The NotificationDispatcher now uses NotificationItemResponse.of(n, actor, group) while other notification handlers (e.g., GroupJoinNotificationHandler, GroupLeaveNotificationHandler, GroupKickNotificationHandler) still use NotificationEvent.of(saved, actor, group). This creates a situation where the same notification system sends differently structured payloads depending on the code path. All handlers should be updated to use the same DTO approach for consistency.
|
|
||
| public record NotificationListResponse(List<NotificationResponse> notifications, Long nextCursor) { | ||
| public record NotificationListResponse( | ||
| // List<NotificationResponse> notifications, |
There was a problem hiding this comment.
The commented-out code should be removed. Instead of keeping List<NotificationResponse> notifications, as a comment, it should be deleted entirely to maintain code cleanliness.
| // List<NotificationResponse> notifications, |
| NotificationType.GROUP_JOIN_REQUEST, "group-join-request", | ||
| NotificationType.GROUP_JOIN_APPROVED, "group-join-approved", | ||
| NotificationType.GROUP_JOIN_REJECTED, "group-join-rejected", | ||
| NotificationType.GROUP_JOIN_KICKED, "group-join-kicked" |
There was a problem hiding this comment.
The NotificationTypeMapper does not include a mapping for NotificationType.TEST. While the mapper falls back to type.name().toLowerCase() for unmapped types, it's better to be explicit. Either add NotificationType.TEST to the map or document why it's intentionally excluded.
| NotificationType.GROUP_JOIN_KICKED, "group-join-kicked" | |
| NotificationType.GROUP_JOIN_KICKED, "group-join-kicked", | |
| NotificationType.TEST, "test" |
| .leftJoin(groupV2).on( | ||
| notification.relatedType.eq("GROUP") | ||
| .and(notification.relatedId.eq(groupV2.id)) | ||
| ) |
There was a problem hiding this comment.
The leftJoin on groupV2 only joins when relatedType equals "GROUP". However, if there are other notification types that should also include group information (or if the condition doesn't match), the group fields will be null. This could lead to unexpected null values in the response. Consider documenting this behavior or ensuring that all group-related notification types properly set relatedType to "GROUP".
| LocalDateTime createdAt | ||
| ) { | ||
| this.id = id; | ||
| this.user = (actorId == null ? null : new UserSummary(actorId, actorNickname)); |
There was a problem hiding this comment.
The constructor creates UserSummary with potentially null actorNickname when actorId is not null. This could happen if the database contains an actor with a null nickname. The constructor should either validate that both values are non-null together, or handle the case where actorId exists but actorNickname is null.
| this.user = (actorId == null ? null : new UserSummary(actorId, actorNickname)); | |
| this.user = (actorId == null || actorNickname == null | |
| ? null | |
| : new UserSummary(actorId, actorNickname)); |
| LocalDateTime createdAt | ||
| ) { | ||
| this.id = id; | ||
| this.user = (actorId == null ? null : new UserSummary(actorId, actorNickname)); |
There was a problem hiding this comment.
The constructor creates GroupSummary with potentially null groupTitle when groupId is not null. This could happen if the database contains a group with a null title. The constructor should either validate that both values are non-null together, or handle the case where groupId exists but groupTitle is null.
| this.user = (actorId == null ? null : new UserSummary(actorId, actorNickname)); | |
| this.user = (actorId == null ? null : new UserSummary(actorId, actorNickname)); | |
| if (groupId != null && groupTitle == null) { | |
| throw new IllegalArgumentException("groupTitle must not be null when groupId is not null"); | |
| } |
|
|
||
| import java.util.List; | ||
| import team.wego.wegobackend.notification.application.dto.response.NotificationItemResponse; | ||
| import team.wego.wegobackend.notification.application.dto.response.NotificationResponse; |
There was a problem hiding this comment.
The unused import NotificationResponse should be removed as the interface now returns NotificationItemResponse instead.
| import team.wego.wegobackend.notification.application.dto.response.NotificationResponse; |
| } | ||
| } | ||
|
|
||
| public boolean sendNotificationIfConnected(Long userId, Object payload) { |
There was a problem hiding this comment.
Method SseEmitterService.sendNotificationIfConnected(..) could be confused with overloaded method sendNotificationIfConnected, since dispatch depends on static types.
| public boolean sendNotificationIfConnected(Long userId, Object payload) { | |
| public boolean sendPayloadIfConnected(Long userId, Object payload) { |
2 participants
📝 Pull Request
📌 PR 종류
해당하는 항목에 체크해주세요.
✨ 변경 내용
알림 시스템을 업데이트하여 페이로드 구성에 DTO를 사용하도록 함으로써 데이터 전송을 더욱 깔끔하게 개선했습니다.
이 변경 사항은 기존의 이벤트 기반 페이로드를 클라이언트에 필요한 정보를 캡슐화하는 전용
NotificationItemResponseDTO로 대체합니다.🔍 관련 이슈
🧪 테스트
변경된 기능에 대한 테스트 범위 또는 테스트 결과를 작성해주세요.
🚨 확인해야 할 사항 (Checklist)
PR을 제출하기 전에 아래 항목들을 확인해주세요.
🙋 기타 참고 사항
리뷰어가 참고하면 좋을 만한 추가 설명이 있다면 적어주세요.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes & Improvements
✏️ Tip: You can customize this high-level summary in your review settings.