Conversation
github-actions
bot
changed the title
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Note
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Description check | PR 설명은 관련 이슈 번호를 명시했으나, 작업 내용과 주요 변경사항 섹션이 비워져 있어 변경의 의도와 구체적 내용이 부족합니다. | 작업 내용 섹션에 쿼리 정렬 변경의 목적을 작성하고, 주요 변경사항에 createdAt DESC 추가와 영향 범위를 구체적으로 기술해주세요. | |
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | You can run @coderabbitai generate docstrings to improve docstring coverage. |
|
| Linked Issues check | ❓ Inconclusive | 코드 변경사항(JPQL 쿼리에 createdAt DESC 추가)은 이슈 제목의 정렬 최적화 목표와 일치하지만, 이슈에 상세 요구사항이 없어 완전한 검증이 어렵습니다. | 이슈 #1054의 상세 작업 내용이 구체화되거나, 이 변경이 페이징 정렬 성능 문제를 어떻게 해결하는지 명시해주세요. |
✅ Passed checks (2 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | PR 제목은 변경 사항의 핵심을 정확히 반영하고 있으며, 알림 조회 쿼리의 정렬 최적화라는 주요 변경점을 명확하게 전달합니다. |
| Out of Scope Changes check | ✅ Passed | NotificationRepository의 두 JPQL 쿼리 정렬 변경은 이슈의 정렬 최적화 목표와 직접적으로 관련되어 있으며, 범위를 벗어난 변경은 없습니다. |
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 help to get the list of available commands and usage tips.
![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
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/src/main/java/backend/mulkkam/notification/repository/NotificationRepository.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
backend/**
⚙️ CodeRabbit configuration file
backend/**: - 1. 팀 및 공식 컨벤션, 가독성, 예외처리, 테스트/확장/유지보수성, 모듈화, API/DB/보안 설계 기준을 기반으로 리뷰해주세요.
- 최신 트렌드, 불필요한 로직, 클린코드, 리팩토링, 서비스/도메인 설계, 공통 예외 처리, 확장성도 함께 확인해주세요.
- 각 피드백은 문제점·대안·장단점을 짧고 논리적으로, 예시 코드가 있다면 간결히 포함해 주세요.
- 팀 내 스타일 통일성도 확인해주세요.
- 미작성한 테스트 코드 케이스가 있다면, 어떤 테스트가 필요한지 제안해주세요. (예: 컨트롤러는 인수 테스트, 나머지는 단위 테스트)
- 리뷰 남겨주는 부분은 해당 라인 범위의 코멘트에 작성해주세요.
Files:
backend/src/main/java/backend/mulkkam/notification/repository/NotificationRepository.java
🧠 Learnings (1)
📚 Learning: 2025-09-28T07:47:02.970Z
Learnt from: 2Jin1031
Repo: woowacourse-teams/2025-mul-kkam PR: 873
File: backend/src/main/java/backend/mulkkam/notification/repository/NotificationRepository.java:0-0
Timestamp: 2025-09-28T07:47:02.970Z
Learning: 알림 조회에서 커서 페이지네이션 구현 시 `n.id <= :lastId` 조건을 사용할 때는 중복 노출을 방지하기 위해 커서 찾는 방식의 순서를 적절히 조정해야 함.
Applied to files:
backend/src/main/java/backend/mulkkam/notification/repository/NotificationRepository.java
⏰ 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). (2)
- GitHub Check: test
- GitHub Check: test
| WHERE n.createdAt >= :limitStartDateTime | ||
| AND n.member.id = :memberId | ||
| ORDER BY n.id DESC | ||
| ORDER BY n.createdAt DESC, n.id DESC |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n backend/src/main/java/backend/mulkkam/notification/repository/NotificationRepository.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 3575
🏁 Script executed:
find . -path "*/notification/domain/Notification.java" -type f | head -1 | xargs cat -nRepository: woowacourse-teams/2025-mul-kkam
Length of output: 3186
🏁 Script executed:
find . -path "*/notification/domain/*.java" -type f | xargs ls -laRepository: woowacourse-teams/2025-mul-kkam
Length of output: 876
🏁 Script executed:
find . -path "*/migration*" -o -path "*/schema*" -o -path "*sql*" | grep -E "\.(sql|java)$" | head -20Repository: woowacourse-teams/2025-mul-kkam
Length of output: 1878
🏁 Script executed:
find . -path "*/notification*Test.java" -type fRepository: woowacourse-teams/2025-mul-kkam
Length of output: 1190
🏁 Script executed:
grep -n "notification" ./backend/src/main/resources/db/migration/common/V1__init.sql -i -A 20 | head -50Repository: woowacourse-teams/2025-mul-kkam
Length of output: 57
🏁 Script executed:
cat -n ./backend/src/main/resources/db/migration/common/V20250825_2349001__modify_notification.sqlRepository: woowacourse-teams/2025-mul-kkam
Length of output: 263
🏁 Script executed:
cat -n ./backend/src/main/resources/db/migration/common/V20250826_004900__modify_notification.sqlRepository: woowacourse-teams/2025-mul-kkam
Length of output: 218
🏁 Script executed:
grep -n "CREATE INDEX\|notification" ./backend/src/main/resources/db/migration/common/V1__init.sql -i | grep -A5 -B5 notificationRepository: woowacourse-teams/2025-mul-kkam
Length of output: 57
🏁 Script executed:
grep -rn "CREATE INDEX" ./backend/src/main/resources/db/migration/ | grep notificationRepository: woowacourse-teams/2025-mul-kkam
Length of output: 57
🏁 Script executed:
cat -n ./backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceIntegrationTest.java | head -100Repository: woowacourse-teams/2025-mul-kkam
Length of output: 5278
🏁 Script executed:
grep -n "cursor\|Cursor\|findByCursor" ./backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceIntegrationTest.java -iRepository: woowacourse-teams/2025-mul-kkam
Length of output: 446
🏁 Script executed:
wc -l ./backend/src/main/resources/db/migration/common/V1__init.sqlRepository: woowacourse-teams/2025-mul-kkam
Length of output: 139
🏁 Script executed:
sed -n '1,100p' ./backend/src/main/resources/db/migration/common/V1__init.sqlRepository: woowacourse-teams/2025-mul-kkam
Length of output: 919
🏁 Script executed:
grep -n "notification" ./backend/src/main/resources/db/migration/common/V1__init.sql -iRepository: woowacourse-teams/2025-mul-kkam
Length of output: 57
🏁 Script executed:
rg -l "CREATE TABLE notification" ./backend/src/main/resources/db/migration/ -t sqlRepository: woowacourse-teams/2025-mul-kkam
Length of output: 148
🏁 Script executed:
sed -n '140,170p' ./backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceIntegrationTest.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 1685
🏁 Script executed:
cat -n ./backend/src/main/resources/db/migration/common/V3__add_notification.sqlRepository: woowacourse-teams/2025-mul-kkam
Length of output: 451
🏁 Script executed:
sed -n '100,250p' ./backend/src/test/java/backend/mulkkam/notification/service/NotificationServiceIntegrationTest.javaRepository: woowacourse-teams/2025-mul-kkam
Length of output: 7333
🏁 Script executed:
grep -n "findByCursorRows\|readNotificationsAfter" ./backend/src/main/java/backend/mulkkam/notification/service/NotificationService.java -A 15 | head -60Repository: woowacourse-teams/2025-mul-kkam
Length of output: 1628
🏁 Script executed:
grep -n "class ReadNotificationRow\|record ReadNotificationRow" ./backend/src/main/java/backend/mulkkam/notification/dto/ReadNotificationRow.java -A 10Repository: woowacourse-teams/2025-mul-kkam
Length of output: 357
커서 기반 페이지네이션의 정렬 순서와 커서 조건이 일치하지 않아 데이터 손실이 발생할 수 있습니다.
findByCursorRows 메서드에서 심각한 문제가 있습니다. 52번 줄의 WHERE 조건은 n.id < :lastId만으로 커서를 판단하지만, 55번 줄의 ORDER BY는 n.createdAt DESC, n.id DESC로 정렬합니다. 이 둘이 일치하지 않으면 커서 페이지네이션에서 데이터가 손실됩니다.
발생 시나리오:
- 알림 A:
id=100, createdAt=2025-01-05 10:00 - 알림 B:
id=5, createdAt=2025-01-05 11:00
정렬 순서: B(11:00, id=5) → A(10:00, id=100)
첫 페이지가 B에서 끝나면 nextCursor=5로 설정되고, 다음 페이지 쿼리는:
WHERE n.id < 5 AND n.createdAt >= '2025-01-05' ...이 조건에서 A(id=100)는 id < 5를 만족하지 않아 제외되므로 데이터 손실이 발생합니다.
해결 방안:
Keyset 페이지네이션으로 변경하여 커서 조건을 정렬 기준과 일치시켜야 합니다:
WHERE (n.createdAt, n.id) < (:lastCreatedAt, :lastId)
AND n.createdAt >= :limitStartDateTime
AND n.member.id = :memberId
ORDER BY n.createdAt DESC, n.id DESC또는 service 계층에서 이전 페이지의 (createdAt, id) 튜플을 함께 전달받아 정렬된 결과 내에서 올바른 위치부터 조회해야 합니다.
추가로:
- 32번 줄의
findLatestRows변경은 초기 페이지 로드이므로 문제없습니다. - 쿼리 성능을 위해
(member_id, created_at DESC, id DESC)복합 인덱스 추가를 권장합니다.
🤖 Prompt for AI Agents
backend/src/main/java/backend/mulkkam/notification/repository/NotificationRepository.java
around line 32: the cursor-based pagination WHERE clause currently uses only
n.id < :lastId while ORDER BY is n.createdAt DESC, n.id DESC, which can drop
rows; change the cursor logic to compare the tuple (n.createdAt, n.id) against
the last cursor values (lastCreatedAt, lastId) — i.e., add a condition
equivalent to (n.createdAt, n.id) < (:lastCreatedAt, :lastId) together with the
existing member and date filters, and update the method signature to accept
lastCreatedAt (or accept a cursor object containing both createdAt and id) so
the query ordering and cursor predicate match; also consider adding a composite
index on (member_id, created_at DESC, id DESC) for performance.
| WHERE n.id < :lastId | ||
| AND n.createdAt >= :limitStartDateTime | ||
| AND n.member.id = :memberId | ||
| ORDER BY n.id DESC | ||
| ORDER BY n.createdAt DESC, n.id DESC | ||
| """) |
There was a problem hiding this comment.
[Critical] 커서 조건과 정렬 순서 불일치로 인한 데이터 누락 발생 가능
현재 커서 페이지네이션 로직에 심각한 결함이 있습니다. ORDER BY는 (createdAt DESC, id DESC) 복합 정렬인데, 커서 조건은 id < :lastId만 사용하고 있어 데이터가 누락됩니다.
문제 시나리오:
알림 A: id=100, createdAt=2025-12-14 10:00:00
알림 B: id=50, createdAt=2025-12-14 11:00:00
정렬 결과: B(50) → A(100) // createdAt이 더 최근인 B가 먼저
1페이지: B를 반환, lastId=50
2페이지: WHERE id < 50 조건으로 인해 A(id=100) 제외됨 ❌
왜 문제인가:
- 복합 정렬 시 커서는 모든 정렬 컬럼을 고려해야 합니다
id만으로는createdAt역순 정렬의 연속성을 보장할 수 없습니다- 특히
id가 순차 증가하는 경우 시간 역순과 id 역순이 반대 방향이므로 반드시 누락 발생
해결 방안:
튜플 비교를 사용한 올바른 커서 조건으로 변경해야 합니다:
@Query("""
SELECT new backend.mulkkam.notification.dto.ReadNotificationRow(
n.id,
n.createdAt,
n.content,
n.notificationType,
n.isRead,
s.recommendedTargetAmount,
s.applyTargetAmount
)
FROM Notification n
LEFT JOIN SuggestionNotification s ON s.id = n.id
WHERE (n.createdAt < :lastCreatedAt
OR (n.createdAt = :lastCreatedAt AND n.id < :lastId))
AND n.createdAt >= :limitStartDateTime
AND n.member.id = :memberId
ORDER BY n.createdAt DESC, n.id DESC
""")
List<ReadNotificationRow> findByCursorRows(
Long memberId,
Long lastId,
LocalDateTime lastCreatedAt, // 파라미터 추가 필요
LocalDateTime limitStartDateTime,
Pageable pageable
);장점:
- 정렬 순서와 커서 조건이 일치하여 데이터 누락/중복 없음
createdAt이 같은 경우에만id로 2차 정렬 보장
단점:
- 메서드 시그니처 변경으로 호출부 수정 필요
- 서비스 레이어에서
lastCreatedAt도 함께 전달해야 함
Based on learnings, 커서 페이지네이션에서는 정렬 순서에 맞춰 커서 조건을 정확히 구성해야 중복 노출이나 누락을 방지할 수 있습니다.
호출부 수정과 통합 테스트 코드가 필요하시면 추가로 제안해드릴까요?
🤖 Prompt for AI Agents
backend/src/main/java/backend/mulkkam/notification/repository/NotificationRepository.java
lines 52-56: The cursor WHERE clause uses only id (< :lastId) while ORDER BY is
(createdAt DESC, id DESC), causing missed rows; change the WHERE to compare the
tuple (createdAt, id) by adding lastCreatedAt and using (n.createdAt <
:lastCreatedAt OR (n.createdAt = :lastCreatedAt AND n.id < :lastId)) in addition
to the existing limitStartDateTime and memberId filters, update the @Query to
accept the new lastCreatedAt parameter, change the repository method signature
to include LocalDateTime lastCreatedAt, and update all callers/service layer to
pass lastCreatedAt (plus add/adjust integration tests) so cursor conditions
match the ORDER BY exactly.
1 participant
🔗 관련 이슈
📝 작업 내용
주요 변경사항
📸 스크린샷 (Optional)
Summary by CodeRabbit
릴리스 노트
✏️ Tip: You can customize this high-level summary in your review settings.