Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review infoConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough관리자가 시스템 관리자 전용 채팅방을 조회할 때 모든 관리자 멤버의 lastReadAt을 일괄 갱신하거나 요청자만 갱신하고, 읽음 기준 계산을 관리자·비관리자 최신값 기반으로 분기하도록 ChatService의 메시지 조회 로직을 변경했습니다. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
이 PR은 어드민 채팅방에서 메시지 읽음 처리가 제대로 되지 않는 오류를 수정합니다. 시스템 어드민 채팅방에서 어드민이 메시지를 확인할 때, 모든 어드민 멤버의 읽음 시간을 업데이트하고, 읽음 표시 기준선(baseline) 계산 로직을 어드민 채팅방에 특화된 방식으로 처리하도록 변경했습니다.
Changes:
- 어드민이 시스템 채팅방을 조회할 때 모든 어드민 멤버의 lastReadAt을 업데이트하도록 수정
- 어드민 채팅방에 특화된 읽음 기준선(read baseline) 계산 로직 추가
- isAdminViewingSystemRoom 계산 위치를 앞으로 이동하여 중복 계산 방지
| adminLastReadAt = member.getLastReadAt(); | ||
| } | ||
| } else { | ||
| userLastReadAt = member.getLastReadAt(); |
There was a problem hiding this comment.
toAdminChatReadBaselines 메서드의 724번 줄에서 non-admin 사용자가 여러 명일 경우 마지막 사용자의 lastReadAt만 저장됩니다. 현재 시스템 어드민 채팅방이 1:1 다이렉트 채팅이라는 가정하에 동작하지만, 이 가정이 위반될 경우 읽음 처리가 올바르게 동작하지 않을 수 있습니다. 719번 줄의 어드민 처리와 동일하게 최신 lastReadAt을 추적하거나, 1:1 채팅방임을 명시적으로 검증하는 로직을 추가하는 것이 안전합니다.
| userLastReadAt = member.getLastReadAt(); | |
| if (userLastReadAt == null || member.getLastReadAt().isAfter(userLastReadAt)) { | |
| userLastReadAt = member.getLastReadAt(); | |
| } |
| if (member.getUser().getRole() == UserRole.ADMIN) { | ||
| if (adminLastReadAt == null || member.getLastReadAt().isAfter(adminLastReadAt)) { | ||
| adminLastReadAt = member.getLastReadAt(); | ||
| } | ||
| } else { | ||
| userLastReadAt = member.getLastReadAt(); |
There was a problem hiding this comment.
toAdminChatReadBaselines 메서드에서 member.getLastReadAt()이 null일 수 있는 경우를 처리하지 않고 있습니다. 720번 줄에서 member.getLastReadAt().isAfter(adminLastReadAt)를 호출할 때 getLastReadAt()이 null을 반환하면 NullPointerException이 발생합니다. ChatRoomMember 모델에서 lastReadAt은 nullable이 아니지만, 데이터베이스에서 조회된 기존 레코드가 null 값을 가질 수 있습니다. getLastReadAt() 결과에 대한 null 체크를 추가해야 합니다.
| if (member.getUser().getRole() == UserRole.ADMIN) { | |
| if (adminLastReadAt == null || member.getLastReadAt().isAfter(adminLastReadAt)) { | |
| adminLastReadAt = member.getLastReadAt(); | |
| } | |
| } else { | |
| userLastReadAt = member.getLastReadAt(); | |
| LocalDateTime lastReadAt = member.getLastReadAt(); | |
| if (lastReadAt == null) { | |
| continue; | |
| } | |
| if (member.getUser().getRole() == UserRole.ADMIN) { | |
| if (adminLastReadAt == null || lastReadAt.isAfter(adminLastReadAt)) { | |
| adminLastReadAt = lastReadAt; | |
| } | |
| } else { | |
| userLastReadAt = lastReadAt; |
| if (isAdminViewingSystemRoom) { | ||
| updateAllAdminMembersLastReadAt(roomId, readAt); | ||
| } else { | ||
| member.updateLastReadAt(readAt); | ||
| } | ||
|
|
||
| PageRequest pageable = PageRequest.of(page - 1, limit); | ||
| Page<ChatMessage> messages = chatMessageRepository.findByChatRoomId(roomId, pageable); | ||
| List<ChatRoomMember> members = chatRoomMemberRepository.findByChatRoomId(roomId); |
There was a problem hiding this comment.
updateAllAdminMembersLastReadAt 메서드가 346번 줄에서 호출된 후, 353번 줄에서 다시 동일한 chatRoomMemberRepository.findByChatRoomId(roomId)를 호출합니다. updateAllAdminMembersLastReadAt 내부(740번 줄)와 getDirectChatRoomMessages 메서드(353번 줄)에서 중복으로 데이터베이스를 조회하고 있습니다. 346번 줄 전에 members를 먼저 조회한 후, updateAllAdminMembersLastReadAt에 members를 파라미터로 전달하도록 리팩토링하면 불필요한 데이터베이스 쿼리를 제거할 수 있습니다.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/gg/agit/konect/domain/chat/service/ChatService.java`:
- Around line 739-746: The call to
chatRoomMemberRepository.findByChatRoomId(roomId) is duplicated; modify
updateAllAdminMembersLastReadAt to accept the previously-fetched
List<ChatRoomMember> (instead of fetching inside) and update callers (notably
getDirectChatRoomMessages) to pass the member list they already obtained;
specifically, change updateAllAdminMembersLastReadAt signature to take
List<ChatRoomMember> members and LocalDateTime readAt, remove the internal
repository call, and update getDirectChatRoomMessages to reuse its existing
members variable when invoking updateAllAdminMembersLastReadAt to eliminate the
extra DB roundtrip.
- Around line 714-737: In toAdminChatReadBaselines(List<ChatRoomMember> members)
ensure you null-check member.getLastReadAt() before using it: skip members with
null lastReadAt or handle them safely so you never call isAfter on a null; when
iterating the members in ChatService.toAdminChatReadBaselines, only compare and
assign to adminLastReadAt/userLastReadAt if member.getLastReadAt() != null (and
use isAfter only when adminLastReadAt is non-null), so the method tolerates
members with null lastReadAt and returns correct baselines.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
src/main/java/gg/agit/konect/domain/chat/service/ChatService.java
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Agent
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/**/*.java
⚙️ CodeRabbit configuration file
src/main/java/**/*.java: 아래 원칙으로 리뷰 코멘트를 작성한다.
- 코멘트는 반드시 한국어로 작성한다.
- 반드시 수정이 필요한 항목만 코멘트로 남기고, 단순 취향 차이는 지적하지 않는다.
- 각 코멘트 첫 줄에 심각도를
[LEVEL: high|medium|low]형식으로 반드시 표기한다.- 심각도 기준: high=운영 장애 가능, medium=품질 저하, low=개선 권고.
- 각 코멘트는 "문제 -> 영향 -> 제안" 순서로 3문장 이내로 간결하게 작성한다.
- 가능하면 재현 조건 및 실패 시나리오도 포함한다.
- 제안은 현재 코드베이스(Spring Boot + JPA + Flyway) 패턴과 일치해야 한다.
- 보안, 트랜잭션 경계, 예외 처리, N+1, 성능 회귀 가능성을 우선 점검한다.
- 가독성: 변수/메서드 이름이 의도를 바로 드러내는지, 중첩과 메서드 길이가 과도하지 않은지 점검한다.
- 단순화: 불필요한 추상화, 중복 로직, 과한 방어 코드가 있으면 더 단순한 대안을 제시한다.
- 확장성: 새 요구사항 추가 시 변경 범위가 최소화되는 구조인지(하드코딩 분기/값 여부 포함) 점검한다.
Files:
src/main/java/gg/agit/konect/domain/chat/service/ChatService.java
**/*
⚙️ CodeRabbit configuration file
**/*: 공통 리뷰 톤 가이드:
- 모든 코멘트는 첫 줄에
[LEVEL: ...]태그를 포함한다.- 과장된 표현 없이 사실 기반으로 작성한다.
- 한 코멘트에는 하나의 이슈만 다룬다.
- 코드 예시가 필요하면 최소 수정 예시를 제시한다.
- 가독성/단순화/확장성 이슈를 발견하면 우선순위를 높여 코멘트한다.
Files:
src/main/java/gg/agit/konect/domain/chat/service/ChatService.java
🔇 Additional comments (2)
src/main/java/gg/agit/konect/domain/chat/service/ChatService.java (2)
342-349: [LEVEL: low] 관리자 읽음 처리 분기 로직 – 의도 명확, 승인합니다.어드민이 시스템 관리자 채팅방을 조회할 때 모든 관리자 멤버의
lastReadAt을 일괄 갱신하고, 그 외에는 요청자 멤버만 갱신하는 분기가 올바르게 구현되어 있습니다.
354-357: [LEVEL: low] 관리자 채팅방 읽음 기준 분기 – 적절한 구현입니다.관리자가 시스템 채팅방을 조회할 때 모든 관리자의
lastReadAt을 하나의 baseline으로 집계하여 unread count를 산정하는 로직이 PR 목적에 부합합니다.
| private List<LocalDateTime> toAdminChatReadBaselines(List<ChatRoomMember> members) { | ||
| LocalDateTime adminLastReadAt = null; | ||
| LocalDateTime userLastReadAt = null; | ||
|
|
||
| for (ChatRoomMember member : members) { | ||
| if (member.getUser().getRole() == UserRole.ADMIN) { | ||
| if (adminLastReadAt == null || member.getLastReadAt().isAfter(adminLastReadAt)) { | ||
| adminLastReadAt = member.getLastReadAt(); | ||
| } | ||
| } else { | ||
| userLastReadAt = member.getLastReadAt(); | ||
| } | ||
| } | ||
|
|
||
| List<LocalDateTime> baselines = new ArrayList<>(); | ||
| if (adminLastReadAt != null) { | ||
| baselines.add(adminLastReadAt); | ||
| } | ||
| if (userLastReadAt != null) { | ||
| baselines.add(userLastReadAt); | ||
| } | ||
| baselines.sort(Comparator.naturalOrder()); | ||
| return baselines; | ||
| } |
There was a problem hiding this comment.
[LEVEL: medium] member.getLastReadAt()가 null일 경우 NPE 발생 가능.
Line 720에서 adminLastReadAt이 이미 non-null로 설정된 상태에서, 다음 관리자 멤버의 getLastReadAt()이 null이면 null.isAfter(...) 호출로 NullPointerException이 발생합니다. updateAllAdminMembersLastReadAt가 직전에 호출되지만, 동시 접근(새 관리자 멤버 추가 등) 시 lastReadAt이 null인 멤버가 포함될 수 있습니다. null-safe 비교를 추가해 주세요.
🐛 수정 제안
private List<LocalDateTime> toAdminChatReadBaselines(List<ChatRoomMember> members) {
LocalDateTime adminLastReadAt = null;
LocalDateTime userLastReadAt = null;
for (ChatRoomMember member : members) {
- if (member.getUser().getRole() == UserRole.ADMIN) {
- if (adminLastReadAt == null || member.getLastReadAt().isAfter(adminLastReadAt)) {
+ LocalDateTime readAt = member.getLastReadAt();
+ if (member.getUser().getRole() == UserRole.ADMIN) {
+ if (readAt != null && (adminLastReadAt == null || readAt.isAfter(adminLastReadAt))) {
adminLastReadAt = member.getLastReadAt();
}
} else {
- userLastReadAt = member.getLastReadAt();
+ userLastReadAt = readAt;
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/gg/agit/konect/domain/chat/service/ChatService.java` around
lines 714 - 737, In toAdminChatReadBaselines(List<ChatRoomMember> members)
ensure you null-check member.getLastReadAt() before using it: skip members with
null lastReadAt or handle them safely so you never call isAfter on a null; when
iterating the members in ChatService.toAdminChatReadBaselines, only compare and
assign to adminLastReadAt/userLastReadAt if member.getLastReadAt() != null (and
use isAfter only when adminLastReadAt is non-null), so the method tolerates
members with null lastReadAt and returns correct baselines.
src/main/java/gg/agit/konect/domain/chat/service/ChatService.java
Outdated
Show resolved
Hide resolved
1 participant
🔍 개요
🚀 주요 변경 내용
💬 참고 사항
✅ Checklist (완료 조건)