Conversation
|
Caution Review failedThe pull request is closed. WalkthroughClub 클래스의 Changes
Sequence Diagram(s)sequenceDiagram
participant Client as User (작성자)
participant CommentSvc as CommentService
participant NotificationSvc as NotificationService
participant Repo as NotificationRepository
participant WS as WebSocketPusher
Client->>CommentSvc: createComment(payload)
CommentSvc->>CommentSvc: validate & save Comment
alt reply (has parent)
CommentSvc->>NotificationSvc: createReplyNotification(savedComment)
else top-level comment
CommentSvc->>NotificationSvc: createCommentNotification(savedComment)
end
NotificationSvc->>Repo: save Notification (type=COMMENT/REPLY, comment=...)
NotificationSvc->>WS: push notification to target user
sequenceDiagram
participant Sender as User A
participant ChatCtrl as ChatStompController
participant ChatRepo as ChatRoomMemberRepository
participant UserRepo as UserRepository
participant NotificationSvc as NotificationService
participant WS as WebSocketPusher
Sender->>ChatCtrl: sendMessage(chatRoomId, message)
ChatCtrl->>ChatRepo: find members of chatRoom
ChatCtrl->>UserRepo: lookup other participant
ChatCtrl->>NotificationSvc: createNoteNotification(chatMessage, otherUser)
NotificationSvc->>WS: push NOTE notification to otherUser
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
시
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/com/be/sportizebe/domain/notification/service/JoinClubRequestServiceImpl.java (1)
84-85:⚠️ Potential issue | 🔴 Critical
Long타입 비교 시!=대신!Objects.equals()사용 필요.
Club.isLeader()와 동일한 문제입니다.Long객체를!=로 비교하면 캐시 범위(-128~127) 밖에서 잘못된 결과를 반환합니다. 본인 신청 취소 검증이 ID 128 이상에서 실패할 수 있습니다.🐛 수정 제안
- if (request.getUser().getId() != userId) { + if (!request.getUser().getId().equals(userId)) {src/main/java/com/be/sportizebe/domain/notification/entity/Notification.java (1)
52-54:⚠️ Potential issue | 🟡 Minor주석과 실제 엔티티 필드 불일치.
Line 52의 주석에서
targetId와targetType으로 다형성 처리한다고 설명하지만, 실제 엔티티에는targetType필드가 존재하지 않습니다.NotificationType이 그 역할을 대신한다면 주석을 수정하는 것이 좋겠습니다. 만약 별도의targetType필드가 필요한 설계라면 누락된 것입니다.✏️ 주석 수정 제안
- // 댓글 알림용 - targetId와 targetType으로 다형성 처리 - // COMMENT: postId, CHAT: chatRoomId 등 + // COMMENT, CHAT 등 알림용 - NotificationType에 따라 참조 대상이 달라짐 + // COMMENT: postId, CHAT: chatRoomIdsrc/main/java/com/be/sportizebe/domain/club/entity/Club.java (1)
71-74:⚠️ Potential issue | 🔴 Critical
Long타입 비교 시==대신.equals()사용 필요.
Long객체를==로 비교하면 -128~127 범위 밖의 값에서는 동일한 값이어도false를 반환합니다.userId가 128 이상이 되면 리더 검증이 실패합니다.🐛 수정 제안
public boolean isLeader(Long userId) { ClubMember leaderMember = getLeaderMember(); - return leaderMember != null && leaderMember.getUser().getId() == userId; + return leaderMember != null && leaderMember.getUser().getId().equals(userId); }동일한 패턴이 다른 파일에서도 발견되었습니다:
PostServiceImpl.java: line 63, 78JoinClubRequestServiceImpl.java: line 84ChatRoomService.java: line 39CommentServiceImpl.java: line 53, 94각 파일의 비교 연산자를
.equals()로 통일하여 일관성 있는 Long 객체 비교를 적용해야 합니다.
🧹 Nitpick comments (1)
src/main/java/com/be/sportizebe/domain/notification/entity/Notification.java (1)
22-29:NotificationType확장 승인.
JOIN_REJECTED,CHAT,COMMENT추가는 PR 목적에 부합합니다. TODO 주석이 남아 있으니 후속 작업 시 정리해 주세요.TODO 항목 추적을 위해 이슈를 생성할까요?
Member
|
무슨 느낌인지 알죠 ( ✅ ) |
Member
Author
|
pr merge 아직 하지 말아주세요 @angoroa |
Member
Author
|
알림 응답 dto 수정 완료 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@src/main/java/com/be/sportizebe/domain/notification/dto/response/NotificationResponse.java`:
- Around line 28-30: The builder chain in NotificationResponse (the lines
calling .clubName(...) and .applicantNickname(...) on joinRequest) can NPE if
joinRequest.getClub() or joinRequest.getUser() is null; update the expressions
to safely handle those cases (either by adding explicit null checks for
joinRequest.getClub() and joinRequest.getUser() before calling
getName()/getNickname(), or use Optional.ofNullable(joinRequest).map(j ->
j.getClub()).map(Club::getName).orElse(null) and similarly for the user
nickname) so the builder receives null when the related entity is missing rather
than throwing an exception; add import java.util.Optional if you choose the
Optional approach.
Comment on lines
+28
to
+30
| .joinRequestId(joinRequest != null ? joinRequest.getId() : null) | ||
| .clubName(joinRequest != null ? joinRequest.getClub().getName() : null) | ||
| .applicantNickname(joinRequest != null ? joinRequest.getUser().getNickname() : null) |
There was a problem hiding this comment.
joinRequest.getClub() 또는 joinRequest.getUser()가 null일 경우 NPE 발생 가능
joinRequest != null 검사만으로는 체이닝된 .getClub().getName(), .getUser().getNickname() 호출의 안전성을 보장하지 못합니다. 연관 엔티티가 null이면 NullPointerException이 발생합니다.
Optional을 사용하면 null 체크 반복도 줄이고 체이닝 안전성도 확보할 수 있습니다.
🛡️ Optional을 활용한 수정 제안
public static NotificationResponse from(Notification notification) {
- var joinRequest = notification.getJoinClubRequest();
-
+ var joinRequest = Optional.ofNullable(notification.getJoinClubRequest());
+
return NotificationResponse.builder()
.id(notification.getId())
.type(notification.getType())
.isRead(notification.getIsRead())
- .joinRequestId(joinRequest != null ? joinRequest.getId() : null)
- .clubName(joinRequest != null ? joinRequest.getClub().getName() : null)
- .applicantNickname(joinRequest != null ? joinRequest.getUser().getNickname() : null)
+ .joinRequestId(joinRequest.map(jr -> jr.getId()).orElse(null))
+ .clubName(joinRequest.map(jr -> jr.getClub()).map(club -> club.getName()).orElse(null))
+ .applicantNickname(joinRequest.map(jr -> jr.getUser()).map(user -> user.getNickname()).orElse(null))
.targetId(notification.getTargetId())
.createdAt(notification.getCreatedAt())
.build();
}java.util.Optional import도 추가해야 합니다:
import java.util.Optional;📝 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.
Suggested change
| .joinRequestId(joinRequest != null ? joinRequest.getId() : null) | |
| .clubName(joinRequest != null ? joinRequest.getClub().getName() : null) | |
| .applicantNickname(joinRequest != null ? joinRequest.getUser().getNickname() : null) | |
| import java.util.Optional; | |
| public static NotificationResponse from(Notification notification) { | |
| var joinRequest = Optional.ofNullable(notification.getJoinClubRequest()); | |
| return NotificationResponse.builder() | |
| .id(notification.getId()) | |
| .type(notification.getType()) | |
| .isRead(notification.getIsRead()) | |
| .joinRequestId(joinRequest.map(jr -> jr.getId()).orElse(null)) | |
| .clubName(joinRequest.map(jr -> jr.getClub()).map(club -> club.getName()).orElse(null)) | |
| .applicantNickname(joinRequest.map(jr -> jr.getUser()).map(user -> user.getNickname()).orElse(null)) | |
| .targetId(notification.getTargetId()) | |
| .createdAt(notification.getCreatedAt()) | |
| .build(); | |
| } |
🤖 Prompt for AI Agents
In
`@src/main/java/com/be/sportizebe/domain/notification/dto/response/NotificationResponse.java`
around lines 28 - 30, The builder chain in NotificationResponse (the lines
calling .clubName(...) and .applicantNickname(...) on joinRequest) can NPE if
joinRequest.getClub() or joinRequest.getUser() is null; update the expressions
to safely handle those cases (either by adding explicit null checks for
joinRequest.getClub() and joinRequest.getUser() before calling
getName()/getNickname(), or use Optional.ofNullable(joinRequest).map(j ->
j.getClub()).map(Club::getName).orElse(null) and similarly for the user
nickname) so the builder receives null when the related entity is missing rather
than throwing an exception; add import java.util.Optional if you choose the
Optional approach.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#️⃣ Issue Number
📝 요약(Summary)
동호회 리더 조회 메서드
-> getLeaderMember()는 클래스 내부에서만 사용중이므로 접근제어자 private으로 변경
알람 메세지
-> 응답 필드에 수신자의 정보를 잘 담도록 dto 수정 (완료)
🛠️ PR 유형
어떤 변경 사항이 있나요?
📸스크린샷 (선택)
💬 공유사항 to 리뷰어
✅ PR Checklist
PR이 다음 요구 사항을 충족하는지 확인하세요.
Summary by CodeRabbit
릴리스 노트
New Features
Refactor