[Feat] 알림 이벤트 추가 - #120
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough분석 완료 시 연습명 알림을 발행합니다. 주간 누적 연주 시간이 새로운 10시간 단위에 도달하면 마일스톤 알림을 발행합니다. 현재 재생 세션을 제외한 누적 시간 조회를 추가합니다. Changes알림 이벤트 연동
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant AnalysisStateService
participant ApplicationEventPublisher
participant NotificationEvent
AnalysisStateService->>ApplicationEventPublisher: 분석 완료 후 연습명 알림 발행
ApplicationEventPublisher->>NotificationEvent: 사용자 대상 알림 전달
sequenceDiagram
participant PlayingService
participant PlayingRepository
participant ApplicationEventPublisher
PlayingService->>PlayingRepository: 현재 세션을 제외한 주간 누적 시간 조회
PlayingRepository-->>PlayingService: durationSec 합계 반환
PlayingService->>ApplicationEventPublisher: 신규 10시간 마일스톤 알림 발행
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/java/com/mr/domain/playing/repository/PlayingRepository.java`:
- Around line 100-106: Update the duration-sum query in PlayingRepository to add
the same p.deletedAt is null predicate used by the other Playing queries,
ensuring soft-deleted records are excluded from weekly accumulated time and
milestone calculations.
- Around line 100-112: Update PlayingRepository.sumDurationSecExcludeCurrent()
to return Long, then adjust the consuming PlayingService calculations so
previousWeeklySeconds, totalWeeklySeconds, and previousMilestones use long/Long
values consistently, preserving the existing aggregation and milestone behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c0d82743-e8b3-436b-8dc5-592f0b7d675b
📒 Files selected for processing (3)
src/main/java/com/mr/domain/analysis/service/AnalysisStateService.javasrc/main/java/com/mr/domain/playing/repository/PlayingRepository.javasrc/main/java/com/mr/domain/playing/service/PlayingService.java
p1001q
left a comment
p1001q
left a comment
There was a problem hiding this comment.
결론:
새 이슈는 이 P3 하나뿐이고 심각도도 낮아서 실질적으로는 그냥 머지해도 될 거 같아요.
AnalysisStateService 쪽 알림 로직(백킹트랙 없으면 "자유 연주"로 표시)도 Playing.backingTrack이 nullable이라 null 체크가 맞게 들어가 있고, NotificationEvent.forAnalysis/forPractice도
기존에 이미 있던(아마 은우님이 먼저 만들어두신) 팩토리 메서드를 정확히 갖다 쓰고 있어서 문제없습니다.
바로 머지하셔도 될 듯요!!!
| LocalDateTime weekStart = LocalDate.now(clock).with(DayOfWeek.MONDAY).atStartOfDay(); | ||
|
|
||
| // 방금 끝낸 연주를 제외한 이전 누적 시간 | ||
| Long previousWeeklySeconds = playingRepository.sumDurationSecExcludeCurrent( |
There was a problem hiding this comment.
🟡 P3
📍 PlayingService.java (saveMidiEvents 내 마일스톤 계산 블록)
문제상황: 주간 연습 시간 마일스톤 계산이 previousWeeklySeconds = sumDurationSecExcludeCurrent(...) 조회 → 계산 → 알림 발행 흐름인데, 이 집계 쿼리에 아무 락도 없습니다.
문제가 되는 이유: 같은 유저가 서로 다른 연주 세션 두 개를 거의 동시에 완료(saveMidiEvents)하는 경우(연속으로 빠르게 제출하거나 멀티 탭 등), 두 트랜잭션이 서로의 커밋을 못 본 채로 동일한 previousWeeklySeconds를 읽어서 같은 10시간 마일스톤 경계를 각자 "처음 넘었다"고 판단하고 알림을 중복 발행할 수 있습니다. (같은 playingId 재요청은 completeWithMidiData가 상태 검증으로 막아주지만, 서로 다른 playingId 두 개가 겹치는 경우는 안 막힙니다.) PR #119(멘토 스트리밍)에서는 비슷한 "동시 진행 상태" 문제를 토큰 기반으로 막아뒀는데, 여기는 그런 보호가 없어요.
수정 방향: 알림 자체가 크리티컬한 데이터는 아니라서 지금 당장 락을 걸 정도는 아니라고 봅니다. 다만 실제로 중복 알림이 이슈가 되면, Playing 저장 시 유저 행에 락을 걸거나, 혹은 알림 발행 자체를 "이번 주 이 마일스톤을 이미 보냈는지" 별도 상태로 멱등하게 관리하는 방향을 고려하면 좋을 것 같습니다.
Tip: 발생 확률이 낮고(정확히 같은 유저가 10시간 단위 경계를 걸치는 두 세션을 동시에 완료해야 함) 영향도도 낮아서(푸시 알림 한 번 더 가는 정도), 이번 PR을 막을 이슈는 아니라고 생각합니다. 그냥 참고만 하시면 될 것 같아요.
3 participants
📍 개요
⛓️💥 관련 이슈
🛠️ 작업 내용
🔥 리뷰 요청 사항
✅ 체크리스트
📎 참고 사항
Summary by CodeRabbit