fix: 알림 전송 로직 수정#2278
Merged
Merged
Conversation
📝 WalkthroughWalkthroughNotification service refactored to dispatch batches of FCM messages via ChangesNotification Batch Sending Refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 4❌ Failed checks (1 warning, 3 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
dh2906
approved these changes
May 28, 2026
| } | ||
| saveNotificationAfterSend(notification); | ||
| return new NotificationDeliveryResult(notification, true); | ||
| notificationJdbcRepository.batchInsert(notifications); |
Contributor
There was a problem hiding this comment.
현재는 FCM 배치 전송 결과를 확인하지 않고 전체 notifications를 저장해서, 전송에 실패한 알림도 같이 저장될 수 있어 보이는데 의도인건지 궁금합니다!
notification 테이블에 성공/실패에 대한 필드가 없어서 구분짓기 어려워보여요
Collaborator
Author
There was a problem hiding this 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 (1)
src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java (1)
62-93:⚠️ Potential issue | 🟠 Major | ⚡ Quick winBlank token validation missing in batch send, unlike single-message
sendMessage.
sendMessagevalidates blank tokens at lines 40-42 and returns early, butsendMessagespasses requests directly to FCM without filtering. If a request contains a blank/null device token, it will cause an FCM error.Additionally, the try-catch wraps the entire loop, so if any batch fails (including due to a blank token), all subsequent batches are skipped.
🔧 Proposed fix: Filter blank tokens and move error handling inside the loop
public void sendMessages(List<FcmSendRequest> requests) { - try { - for (int start = 0; start < requests.size(); start += FCM_MESSAGE_BATCH_SIZE) { + List<FcmSendRequest> validRequests = requests.stream() + .filter(r -> StringUtils.hasText(r.targetDeviceToken())) + .toList(); + + if (validRequests.isEmpty()) { + return; + } + + for (int start = 0; start < validRequests.size(); start += FCM_MESSAGE_BATCH_SIZE) { + try { - int end = Math.min(start + FCM_MESSAGE_BATCH_SIZE, requests.size()); - List<Message> messages = requests.subList(start, end).stream() + int end = Math.min(start + FCM_MESSAGE_BATCH_SIZE, validRequests.size()); + List<Message> messages = validRequests.subList(start, end).stream() .map(request -> Message.builder() // ... existing mapping .build() ) .toList(); FirebaseMessaging.getInstance().sendEach(messages); + } catch (Exception e) { + log.warn("FCM 알림 배치 전송 실패 (batch start={})", start, e); } - } catch (Exception e) { - log.warn("FCM 알림 전송 실패", e); } }🤖 Prompt for 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. In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java` around lines 62 - 93, sendMessages currently builds and sends batches without validating FcmSendRequest.targetDeviceToken (unlike sendMessage) and wraps the whole loop in one try-catch, so a bad token aborts all batches; fix by filtering requests to exclude null/blank targetDeviceToken before mapping to Message (use same validation logic as sendMessage) and move the try-catch inside the for-loop around the per-batch send (FirebaseMessaging.getInstance().sendEach) so failures in one batch (or from a bad token) are logged and skipped while subsequent batches still run; reference sendMessages, sendMessage, FCM_MESSAGE_BATCH_SIZE, FcmSendRequest.targetDeviceToken, and FirebaseMessaging.getInstance().sendEach.
🧹 Nitpick comments (2)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
42-66: 💤 Low value
@Transactionalscope encompasses external FCM call.The transaction starts before the FCM call (line 59) and includes the external network operation. If
batchInsertwere to throw an unchecked exception not caught by the try-catch (e.g., from infrastructure issues), the transaction would roll back but FCM messages would already be sent. Consider whether the transactional boundary should only wrap the persistence portion, or document this acceptable trade-off.The current error handling for persistence failures (lines 61-65) aligns with the PR objectives.
🤖 Prompt for 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. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 42 - 66, The method pushNotifications currently carries `@Transactional` and thus wraps the external fcmClient.sendMessages call; move the transaction boundary so only the DB insert is transactional: remove `@Transactional` from pushNotifications, create a new private `@Transactional` method (e.g., saveNotifications(List<Notification>) that calls notificationJdbcRepository.batchInsert(notifications), and in pushNotifications call fcmClient.sendMessages(fcmSendRequests) first then wrap the saveNotifications call in the existing try/catch to log failures; this ensures the external FCM call is outside the transactional scope while preserving your persistence error handling.src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java (1)
40-42: 💤 Low valueConsider adding a debug/info log when skipping due to blank token.
The silent early return makes it harder to diagnose why a notification wasn't sent. A debug-level log would help with troubleshooting without adding noise in production.
📝 Proposed addition
if (!StringUtils.hasText(targetDeviceToken)) { + log.debug("Skipping FCM send: blank device token"); return; }🤖 Prompt for 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. In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java` around lines 40 - 42, Add a debug/info log inside FcmClient where you currently early-return on a blank token (the check using StringUtils.hasText(targetDeviceToken)) so callers can see why a notification was skipped; update the method (e.g., sendNotification or whichever method contains the token check) to call the class logger (e.g., logger.debug or logger.info) before returning, including the targetDeviceToken variable and a brief message like "Skipping send: blank targetDeviceToken" to aid troubleshooting without spamming production logs.
🤖 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.
Outside diff comments:
In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java`:
- Around line 62-93: sendMessages currently builds and sends batches without
validating FcmSendRequest.targetDeviceToken (unlike sendMessage) and wraps the
whole loop in one try-catch, so a bad token aborts all batches; fix by filtering
requests to exclude null/blank targetDeviceToken before mapping to Message (use
same validation logic as sendMessage) and move the try-catch inside the for-loop
around the per-batch send (FirebaseMessaging.getInstance().sendEach) so failures
in one batch (or from a bad token) are logged and skipped while subsequent
batches still run; reference sendMessages, sendMessage, FCM_MESSAGE_BATCH_SIZE,
FcmSendRequest.targetDeviceToken, and FirebaseMessaging.getInstance().sendEach.
---
Nitpick comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 42-66: The method pushNotifications currently carries
`@Transactional` and thus wraps the external fcmClient.sendMessages call; move the
transaction boundary so only the DB insert is transactional: remove
`@Transactional` from pushNotifications, create a new private `@Transactional`
method (e.g., saveNotifications(List<Notification>) that calls
notificationJdbcRepository.batchInsert(notifications), and in pushNotifications
call fcmClient.sendMessages(fcmSendRequests) first then wrap the
saveNotifications call in the existing try/catch to log failures; this ensures
the external FCM call is outside the transactional scope while preserving your
persistence error handling.
In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java`:
- Around line 40-42: Add a debug/info log inside FcmClient where you currently
early-return on a blank token (the check using
StringUtils.hasText(targetDeviceToken)) so callers can see why a notification
was skipped; update the method (e.g., sendNotification or whichever method
contains the token check) to call the class logger (e.g., logger.debug or
logger.info) before returning, including the targetDeviceToken variable and a
brief message like "Skipping send: blank targetDeviceToken" to aid
troubleshooting without spamming production logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8dc99d3a-1229-49e9-b405-7db00108f7b7
📒 Files selected for processing (5)
src/main/java/in/koreatech/koin/domain/notification/service/KeywordNotificationService.javasrc/main/java/in/koreatech/koin/domain/notification/service/NotificationPersistenceService.javasrc/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javasrc/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.javasrc/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java
💤 Files with no reviewable changes (2)
- src/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java
- src/main/java/in/koreatech/koin/domain/notification/service/NotificationPersistenceService.java
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.
🔍 개요
🚀 주요 변경 내용
알림 전송 로직 수정
💬 참고 사항
✅ Checklist (완료 조건)
Summary by CodeRabbit
Release Notes
Refactor
Tests