Open
Conversation
taejinn
requested review from
BaeJinho4028,
DHkimgit,
ImTotem,
Soundbar91,
Copilot and
dh2906
|
Important Review skippedThis PR was authored by the user configured for CodeRabbit reviews. By default, CodeRabbit skips reviewing PRs authored by this user. It's recommended to use a dedicated user account to post CodeRabbit review feedback. To trigger a single review, invoke the You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAggregates keyword matches per article into a userId→keyword map carried by Changes
Sequence DiagramsequenceDiagram
participant KS as KeywordService
participant AR as ArticleRepository
participant KE as KeywordExtractor
participant AKE as ArticleKeywordEventListener
participant UNR as UserNotificationStatusRepository
participant NS as NotificationService
KS->>KS: Deduplicate article IDs
alt has articles
KS->>AR: fetch articles by IDs
AR-->>KS: articles
KS->>KE: matchKeyword(articles)
KE->>KE: group matches per-article per-user (userId→keyword)
KE-->>KS: List<ArticleKeywordEvent>
loop per ArticleKeywordEvent
KS->>KS: publish ArticleKeywordEvent
KS->>AKE: ArticleKeywordEvent
AKE->>AKE: build per-user subscription map
AKE->>UNR: getAlreadyNotifiedUserIds
alt user is author or already-notified
AKE->>AKE: skip
else
AKE->>NS: createNotification(user, article, keyword)
NS-->>AKE: NotificationDeliveryResult
AKE->>UNR: upsertLastNotifiedArticleId for delivered users
end
end
else no articles
KS->>KS: return (no-op)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
Pull request overview
키워드 알림에서 동일 게시글이 여러 키워드에 매칭될 때 사용자에게 중복 발송되던 문제(#2164)를 해결하기 위해, 이벤트 생성/소비 흐름을 “게시글당 1건 이벤트 + 사용자당 1회 발송” 구조로 정리한 PR입니다.
Changes:
ArticleKeywordEvent를 단일 키워드 →matchedKeywords(다중 키워드)로 확장하고,KeywordExtractor가 게시글당 이벤트 1건만 생성하도록 변경ArticleKeywordEventListener에서 (1) 구독 중복 제거 (2) 사용자당 1회만 알림 생성 (3) 더 구체적인 키워드(문자열 길이) 우선 선택 로직 추가KeywordService에서 요청 articleId 중복 제거 및UserNotificationStatus저장을 update-or-insert 형태로 변경 + 관련 테스트 추가
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java | 이벤트 payload를 다중 키워드 매칭 구조로 변경 |
| src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java | 게시글당 이벤트 1건만 생성되도록 매칭 결과를 articleId로 그룹핑 |
| src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java | 요청 ID dedupe 및 알림 상태 저장 로직을 update-or-insert로 변경 |
| src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java | 구독/키워드 매칭 중복 제거 및 사용자당 1회 알림 생성으로 변경 |
| src/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.java | 불필요 import 정리 |
| src/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.java | 상태 갱신 메서드 추가 |
| src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java | 다중 키워드 매칭 시 이벤트 1건 생성 테스트 추가 |
| src/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.java | 중복 articleId 요청 dedupe 테스트 추가 |
| src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java | 중복 구독/다중 키워드 매칭에도 사용자당 1회 발송 테스트 추가 |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java
Outdated
Show resolved
Hide resolved
![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
🧹 Nitpick comments (1)
src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java (1)
33-54: Good test coverage for the core fix.The test correctly verifies that multiple matching keywords for a single article produce only one event with all matched keywords aggregated. Consider adding edge case tests for better coverage:
- No keywords match any article (empty result)
- Multiple articles with varying keyword matches
- Empty keyword list from repository
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java` around lines 33 - 54, Add additional unit tests in KeywordExtractorTest targeting KeywordExtractor.matchKeyword to cover the suggested edge cases: (1) no keywords match any article — assert an empty result when repository returns keywords that don't appear in articles, (2) multiple articles with varying keyword matches — create multiple mocked Article instances with different titles and assert one event per article aggregating that article's matched keywords, and (3) empty keyword list from repository — mock articleKeywordRepository.findAll(...) to return an empty list and assert matchKeyword returns an empty list; use the same mocking style (when(...).thenReturn(...)) and assertions (assertThat(...)) as the existing test to keep consistency.
🤖 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/in/koreatech/koin/domain/community/keyword/service/KeywordService.java`:
- Around line 202-209: The current createNotifiedArticleStatus method uses
userNotificationStatusRepository.findByUserId(...) followed by save which is not
atomic and can race; change this to an atomic upsert or guarded transaction:
implement a repository-level upsert (e.g., a custom repository method using
INSERT ... ON CONFLICT / ON DUPLICATE KEY UPDATE) or use a transactional
read-for-update (pessimistic lock) when loading the UserNotificationStatus and
then call updateNotifiedArticleId, or catch
unique-constraint/DataIntegrityViolation exceptions around save and retry the
lookup+update; reference createNotifiedArticleStatus,
userNotificationStatusRepository, findByUserId, save, UserNotificationStatus,
and updateNotifiedArticleId when making the change.
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 84-96: getMatchedKeywordByUserId triggers lazy loading via
ArticleKeyword#getArticleKeywordUserMaps causing LazyInitializationException
when run after commit; fix by ensuring ArticleKeyword.articleKeywordUserMaps is
eagerly loaded at retrieval: update the repository/query in KeywordExtractor
that returns the matched keywords to use a JOIN FETCH for articleKeywordUserMaps
(or create a new method like findWithUserMapsByIds(...) that uses JOIN FETCH) so
getMatchedKeywordByUserId can iterate without accessing the closed persistence
context; alternatively, if changing the fetch path is undesirable, annotate the
listener method that calls getMatchedKeywordByUserId with `@Transactional` to open
a session during execution.
---
Nitpick comments:
In
`@src/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.java`:
- Around line 33-54: Add additional unit tests in KeywordExtractorTest targeting
KeywordExtractor.matchKeyword to cover the suggested edge cases: (1) no keywords
match any article — assert an empty result when repository returns keywords that
don't appear in articles, (2) multiple articles with varying keyword matches —
create multiple mocked Article instances with different titles and assert one
event per article aggregating that article's matched keywords, and (3) empty
keyword list from repository — mock articleKeywordRepository.findAll(...) to
return an empty list and assert matchKeyword returns an empty list; use the same
mocking style (when(...).thenReturn(...)) and assertions (assertThat(...)) as
the existing test to keep consistency.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.javasrc/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.javasrc/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.javasrc/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.javasrc/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.javasrc/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.javasrc/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.javasrc/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
💤 Files with no reviewable changes (1)
- src/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.java
src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java
Show resolved
Hide resolved
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Outdated
Show resolved
Hide resolved
taejinn
force-pushed
the
fix/2164-notification-duplication
branch
from
5e78b18 to
89619ba
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Show resolved
Hide resolved
.../in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java (1)
5-9: Consider making the event payload map immutable.A defensive copy (e.g.,
Map.copyOf(...)) in the record constructor would prevent accidental mutation after publish.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java` around lines 5 - 9, The record ArticleKeywordEvent currently exposes a mutable Map via the matchedKeywordByUserId component; make the payload immutable by adding a defensive copy in the record constructor (e.g., use Map.copyOf(...) or Collections.unmodifiableMap(...)) so the stored field is an immutable map; implement this in the record's canonical or compact constructor to replace matchedKeywordByUserId with the copied immutable map before assignment.src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java (1)
43-54: Potential over-fetch in batch loop.
findAllByArticleKeywordIdIn(keywordIds)runs before title matching, so user-map rows are loaded even for keywords that match no article title in that batch. Consider querying user maps only for matched keyword IDs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java` around lines 43 - 54, The current code fetches ArticleKeywordUserMap rows for all keywordIds via articleKeywordUserMapRepository.findAllByArticleKeywordIdIn(keywordIds) before filtering keywords by title, which can over-fetch; change the flow to first filter/match keywords by title (use the existing keywords stream and title-matching predicate to produce matchedKeywordIds or matchedKeywords list), then call findAllByArticleKeywordIdIn(matchedKeywordIds) and build userMapsByKeywordId (preserving grouping with LinkedHashMap::new) only for those matched IDs; update any downstream references from keywordIds to matchedKeywordIds and ensure ArticleKeywordUserMap filtering on getIsDeleted() remains.src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java (1)
158-161: Consider replacing per-IDgetByIdcalls with a batch fetch.The current loop issues one DB call per article ID. For crawler-triggered batches, a single repository batch query will scale better.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java` around lines 158 - 161, The loop in KeywordService that builds articles by calling articleRepository.getById for each id (using updateNotificationIds -> articles) causes N DB calls; replace it with a single batch fetch (e.g., articleRepository.findAllById or findByIdIn(updateNotificationIds)) to retrieve all Article entities in one query, then map the returned list back into the original updateNotificationIds order (and handle missing ids/nulls) before assigning to articles; ensure you use the repository batch method name that exists (or add one) and remove the per-id getById loop.src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java (1)
44-50: Move the empty-recipient short-circuit before loadingArticle/Board.Line [44]-Line [45] hits the DB before the early return at Line [48]. For empty
matchedKeywordByUserId, this is unnecessary work.♻️ Proposed change
- Article article = articleRepository.getById(event.articleId()); - Board board = article.getBoard(); Map<Integer, String> matchedKeywordByUserId = event.matchedKeywordByUserId(); if (matchedKeywordByUserId.isEmpty()) { return; } + + Article article = articleRepository.getById(event.articleId()); + Board board = article.getBoard();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java` around lines 44 - 50, In ArticleKeywordEventListener, avoid loading Article/Board when there are no recipients by moving the matched-recipient empty check before calling articleRepository.getById; specifically, check event.matchedKeywordByUserId().isEmpty() first and return early, then only call articleRepository.getById(event.articleId()) and use article.getBoard() when the map is non-empty.
🤖 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/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 63-66: The stream in ArticleKeywordEventListener uses
isNewNotifiedArticleId per subscriber which triggers an N+1 exists query;
instead add a repository projection method (e.g.,
NotificationRepository.findUserIdsByNotifiedArticleIdAndUserIdIn(articleId,
userIds)) to fetch all userIds already notified for the article in one query,
call it once before building the stream, store the result as a Set<Long>, and
replace the isNewNotifiedArticleId(...) check with a simple contains check
against that set when filtering keywordSubscribersByUserId; keep
isNewNotifiedArticleId only if you refactor it to use the pre-fetched set or
remove it and reference the set directly.
- Around line 67-75: The code currently persists notification status inside
createAndRecordNotification while mapping subscribers
(ArticleKeywordEventListener) which causes permanent loss if
notificationService.pushNotifications (which uses fcmClient.sendMessage) fails;
change the flow so createAndRecordNotification no longer writes status to DB but
only constructs Notification DTOs, then call
notificationService.pushNotifications(notifications) to send, and only after
each send succeeds persist the delivered status (e.g., add a new method like
notificationService.recordNotificationStatus or use a separate
persistNotifications(List<Notification>, deliveryResults) that updates the
database). Ensure references: modify ArticleKeywordEventListener to stop calling
a persistence side-effect in createAndRecordNotification, update
createAndRecordNotification to be pure (construct Notification), and add logic
after notificationService.pushNotifications to persist statuses based on send
results from fcmClient.sendMessage.
---
Nitpick comments:
In `@src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.java`:
- Around line 5-9: The record ArticleKeywordEvent currently exposes a mutable
Map via the matchedKeywordByUserId component; make the payload immutable by
adding a defensive copy in the record constructor (e.g., use Map.copyOf(...) or
Collections.unmodifiableMap(...)) so the stored field is an immutable map;
implement this in the record's canonical or compact constructor to replace
matchedKeywordByUserId with the copied immutable map before assignment.
In
`@src/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.java`:
- Around line 158-161: The loop in KeywordService that builds articles by
calling articleRepository.getById for each id (using updateNotificationIds ->
articles) causes N DB calls; replace it with a single batch fetch (e.g.,
articleRepository.findAllById or findByIdIn(updateNotificationIds)) to retrieve
all Article entities in one query, then map the returned list back into the
original updateNotificationIds order (and handle missing ids/nulls) before
assigning to articles; ensure you use the repository batch method name that
exists (or add one) and remove the per-id getById loop.
In `@src/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.java`:
- Around line 43-54: The current code fetches ArticleKeywordUserMap rows for all
keywordIds via
articleKeywordUserMapRepository.findAllByArticleKeywordIdIn(keywordIds) before
filtering keywords by title, which can over-fetch; change the flow to first
filter/match keywords by title (use the existing keywords stream and
title-matching predicate to produce matchedKeywordIds or matchedKeywords list),
then call findAllByArticleKeywordIdIn(matchedKeywordIds) and build
userMapsByKeywordId (preserving grouping with LinkedHashMap::new) only for those
matched IDs; update any downstream references from keywordIds to
matchedKeywordIds and ensure ArticleKeywordUserMap filtering on getIsDeleted()
remains.
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 44-50: In ArticleKeywordEventListener, avoid loading Article/Board
when there are no recipients by moving the matched-recipient empty check before
calling articleRepository.getById; specifically, check
event.matchedKeywordByUserId().isEmpty() first and return early, then only call
articleRepository.getById(event.articleId()) and use article.getBoard() when the
map is non-empty.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
src/main/java/in/koreatech/koin/common/event/ArticleKeywordEvent.javasrc/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/ArticleKeywordUserMapRepository.javasrc/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.javasrc/main/java/in/koreatech/koin/domain/community/keyword/service/KeywordService.javasrc/main/java/in/koreatech/koin/domain/community/util/KeywordExtractor.javasrc/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.javasrc/main/java/in/koreatech/koin/domain/notification/repository/NotificationSubscribeRepository.javasrc/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.javasrc/test/java/in/koreatech/koin/unit/domain/community/util/KeywordExtractorTest.javasrc/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/in/koreatech/koin/unit/domain/community/keyword/service/KeywordServiceTest.java
- src/main/java/in/koreatech/koin/domain/community/keyword/model/UserNotificationStatus.java
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Show resolved
Hide resolved
...in/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java
Outdated
Show resolved
Hide resolved
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)
39-67:⚠️ Potential issue | 🟠 Major
sendMessageWithResult가try이전 예외를 놓치고 있습니다.현재는
FirebaseMessaging.send(...)만try/catch로 감싸고 있어, 그 이전 단계에서 예외가 나면false반환 대신 메서드가 그대로 예외를 던집니다. 결과 기반 흐름에서는 여기서도 일관되게false를 반환해야 안전합니다.🔧 Proposed fix
public boolean sendMessageWithResult( String targetDeviceToken, String title, String content, String imageUrl, MobileAppPath path, String schemeUri, String type ) { if (targetDeviceToken == null) { return false; } log.info("call FcmClient sendMessage: title: {}, content: {}", title, content); - - ApnsConfig apnsConfig = generateAppleConfig(title, content, imageUrl, path, type, schemeUri); - AndroidConfig androidConfig = generateAndroidConfig(title, content, imageUrl, schemeUri, type); - - Message message = Message.builder() - .setToken(targetDeviceToken) - .setApnsConfig(apnsConfig) - .setAndroidConfig(androidConfig).build(); try { + ApnsConfig apnsConfig = generateAppleConfig(title, content, imageUrl, path, type, schemeUri); + AndroidConfig androidConfig = generateAndroidConfig(title, content, imageUrl, schemeUri, type); + + Message message = Message.builder() + .setToken(targetDeviceToken) + .setApnsConfig(apnsConfig) + .setAndroidConfig(androidConfig) + .build(); String result = FirebaseMessaging.getInstance().send(message); log.info("FCM 알림 전송 성공: {}", result); return true; } catch (Exception e) { log.warn("FCM 알림 전송 실패", e); return false; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java` around lines 39 - 67, The method sendMessageWithResult currently only wraps FirebaseMessaging.getInstance().send(...) in a try/catch, so exceptions from generateAppleConfig, generateAndroidConfig or Message.builder() escape; move or expand the try/catch to cover the code that builds ApnsConfig/AndroidConfig and the Message (everything after the null-check) so any thrown Exception is caught, logged via log.warn (include the exception) and the method returns false consistently; keep the early null check for targetDeviceToken outside the try if you prefer, but ensure all calls to generateAppleConfig, generateAndroidConfig, Message.builder() and FirebaseMessaging.getInstance().send are inside the try block.
🧹 Nitpick comments (3)
src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java (1)
151-185:delivered=false경로도 테스트에 추가하면 더 견고합니다.성공 시에만
createNotifiedArticleStatus(...)를 호출하는 로직이 핵심이므로, 실패 전송 케이스를 명시적으로 고정해두는 것을 권장합니다.🧪 Suggested test case
+ `@Test` + `@DisplayName`("푸시 전송 실패 시 기발송 상태를 기록하지 않는다.") + void onKeywordRequest_whenDeliveryFailed_doesNotRecordStatus() { + Integer articleId = 400; + Integer boardId = 13; + Integer userId = 4; + User subscriber = UserFixture.id_설정_코인_유저(userId); + subscriber.permitNotification("device-token"); + + NotificationSubscribe subscribe = createKeywordSubscribe(subscriber); + ArticleKeywordEvent event = new ArticleKeywordEvent(articleId, 999, Map.of(userId, "근로")); + + Article article = mock(Article.class); + Board board = mock(Board.class); + when(articleRepository.getById(articleId)).thenReturn(article); + when(article.getId()).thenReturn(articleId); + when(article.getTitle()).thenReturn("근로 공지"); + when(article.getBoard()).thenReturn(board); + when(board.getId()).thenReturn(boardId); + when(notificationSubscribeRepository.findAllBySubscribeTypeAndDetailTypeIsNullWithUser(ARTICLE_KEYWORD)) + .thenReturn(List.of(subscribe)); + when(userNotificationStatusRepository.findUserIdsByNotifiedArticleIdAndUserIdIn(eq(articleId), any())) + .thenReturn(List.of()); + + Notification notification = mock(Notification.class); + when(notification.getUser()).thenReturn(subscriber); + when(notificationFactory.generateKeywordNotification(any(), anyInt(), anyString(), anyString(), anyInt(), anyString(), any())) + .thenReturn(notification); + when(notificationService.pushNotificationsWithResult(any())) + .thenReturn(List.of(new NotificationService.NotificationDeliveryResult(notification, false))); + + articleKeywordEventListener.onKeywordRequest(event); + + verify(keywordService, never()).createNotifiedArticleStatus(anyInt(), anyInt()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java` around lines 151 - 185, Add a failing-delivery test to ArticleKeywordEventListenerTest (e.g., a new test alongside onKeywordRequest_whenAlreadyNotified_skipsNotification) that simulates notification push returning a result with delivered=false; set up the same Article/Board/subscribe mocks and have notificationService.pushNotificationsWithResult(...) return a list containing a NotificationResult (or equivalent) with delivered=false, then assert that notificationFactory.generateKeywordNotification is invoked (if applicable) but keywordService.createNotifiedArticleStatus(...) is NOT called and that pushNotificationsWithResult was called with the expected list—this explicitly verifies the non-creation path when delivered=false.src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
47-52: 결과 기반 전송 루프의 외부 I/O를 트랜잭션 경계 밖으로 분리하는 것을 권장합니다.현재는 FCM 호출이 DB 트랜잭션 내부에서 수행되어, 대상이 많을 때 트랜잭션 유지 시간이 길어질 수 있습니다.
♻️ Suggested minimal change
- `@Transactional` public List<NotificationDeliveryResult> pushNotificationsWithResult(List<Notification> notifications) { return notifications.stream() .map(this::pushNotificationWithResult) .toList(); }Also applies to: 69-82
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 47 - 52, The pushNotificationsWithResult method is currently annotated with `@Transactional` and calls pushNotificationWithResult inside the transaction, causing external I/O (FCM calls) to run within the DB transaction; refactor so that DB reads/writes remain inside a short transaction and the actual FCM send happens outside it — for example, have a transactional helper that loads/updates Notification entities (keep method like pushNotificationsWithResult or a new fetchAndMarkForSend) then collect payloads and call pushNotificationWithResult (or a new sendNotification) outside the `@Transactional` boundary; apply the same pattern to the other transactional send loop (lines 69-82) to ensure all network/FCM calls occur after the transaction commits.src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java (1)
65-68: 이미 발송 조회 대상을 매칭된 사용자로 먼저 축소하면 더 효율적입니다.현재는 디바이스 토큰이 있는 전체 키워드 구독자를 기준으로 조회해, 매칭되지 않은 사용자까지
IN대상에 포함됩니다.⚡ Proposed optimization
- Set<Integer> alreadyNotifiedUserIds = getAlreadyNotifiedUserIds( - event.articleId(), - keywordSubscribersByUserId.keySet() - ); + Set<Integer> targetUserIds = keywordSubscribersByUserId.keySet().stream() + .filter(matchedKeywordByUserId::containsKey) + .collect(Collectors.toSet()); + + Set<Integer> alreadyNotifiedUserIds = getAlreadyNotifiedUserIds( + event.articleId(), + targetUserIds + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java` around lines 65 - 68, The call to getAlreadyNotifiedUserIds currently passes keywordSubscribersByUserId.keySet() (all subscribers with device tokens), causing the DB IN query to include users who weren’t matched; narrow the IN list by first computing the set of matched user IDs and pass that to getAlreadyNotifiedUserIds. Concretely, derive a Set<Integer> matchedUserIds from keywordSubscribersByUserId (or from whatever matching result you have) and call getAlreadyNotifiedUserIds(event.articleId(), matchedUserIds) so only matched subscribers are queried for prior notifications.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java`:
- Around line 39-67: The method sendMessageWithResult currently only wraps
FirebaseMessaging.getInstance().send(...) in a try/catch, so exceptions from
generateAppleConfig, generateAndroidConfig or Message.builder() escape; move or
expand the try/catch to cover the code that builds ApnsConfig/AndroidConfig and
the Message (everything after the null-check) so any thrown Exception is caught,
logged via log.warn (include the exception) and the method returns false
consistently; keep the early null check for targetDeviceToken outside the try if
you prefer, but ensure all calls to generateAppleConfig, generateAndroidConfig,
Message.builder() and FirebaseMessaging.getInstance().send are inside the try
block.
---
Nitpick comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.java`:
- Around line 65-68: The call to getAlreadyNotifiedUserIds currently passes
keywordSubscribersByUserId.keySet() (all subscribers with device tokens),
causing the DB IN query to include users who weren’t matched; narrow the IN list
by first computing the set of matched user IDs and pass that to
getAlreadyNotifiedUserIds. Concretely, derive a Set<Integer> matchedUserIds from
keywordSubscribersByUserId (or from whatever matching result you have) and call
getAlreadyNotifiedUserIds(event.articleId(), matchedUserIds) so only matched
subscribers are queried for prior notifications.
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 47-52: The pushNotificationsWithResult method is currently
annotated with `@Transactional` and calls pushNotificationWithResult inside the
transaction, causing external I/O (FCM calls) to run within the DB transaction;
refactor so that DB reads/writes remain inside a short transaction and the
actual FCM send happens outside it — for example, have a transactional helper
that loads/updates Notification entities (keep method like
pushNotificationsWithResult or a new fetchAndMarkForSend) then collect payloads
and call pushNotificationWithResult (or a new sendNotification) outside the
`@Transactional` boundary; apply the same pattern to the other transactional send
loop (lines 69-82) to ensure all network/FCM calls occur after the transaction
commits.
In
`@src/test/java/in/koreatech/koin/unit/domain/notification/eventlistener/ArticleKeywordEventListenerTest.java`:
- Around line 151-185: Add a failing-delivery test to
ArticleKeywordEventListenerTest (e.g., a new test alongside
onKeywordRequest_whenAlreadyNotified_skipsNotification) that simulates
notification push returning a result with delivered=false; set up the same
Article/Board/subscribe mocks and have
notificationService.pushNotificationsWithResult(...) return a list containing a
NotificationResult (or equivalent) with delivered=false, then assert that
notificationFactory.generateKeywordNotification is invoked (if applicable) but
keywordService.createNotifiedArticleStatus(...) is NOT called and that
pushNotificationsWithResult was called with the expected list—this explicitly
verifies the non-creation path when delivered=false.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/main/java/in/koreatech/koin/domain/community/keyword/repository/UserNotificationStatusRepository.javasrc/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.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/eventlistener/ArticleKeywordEventListenerTest.java
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java (1)
71-84: Notification is persisted before delivery confirmation.
notificationRepository.save(notification)at line 72 occurs before the FCM send at line 74. IfsendMessageWithResultreturnsfalse, the notification record remains in the database even though delivery failed. This could lead to orphaned notification records that the user never received.Consider either:
- Moving
save()after successful delivery, or- Accepting the current behavior as an audit trail of attempted notifications
If the intent is to record all attempted notifications (even failed ones), this is acceptable but should be documented.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 71 - 84, The NotificationService.pushNotificationWithResult currently persists the Notification before attempting delivery (notificationRepository.save called before fcmClient.sendMessageWithResult), which can leave failed delivery records in the DB; either move the save call to after a successful send (call notificationRepository.save(notification) only when fcmClient.sendMessageWithResult(...) returns true and then construct NotificationDeliveryResult), or if you intend to record attempted deliveries, add a clear comment and update documentation/tests to reflect that notificationRepository.save(notification) intentionally remains before the send; locate and update the pushNotificationWithResult method and the use of notificationRepository.save and fcmClient.sendMessageWithResult accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 71-84: The NotificationService.pushNotificationWithResult
currently persists the Notification before attempting delivery
(notificationRepository.save called before fcmClient.sendMessageWithResult),
which can leave failed delivery records in the DB; either move the save call to
after a successful send (call notificationRepository.save(notification) only
when fcmClient.sendMessageWithResult(...) returns true and then construct
NotificationDeliveryResult), or if you intend to record attempted deliveries,
add a clear comment and update documentation/tests to reflect that
notificationRepository.save(notification) intentionally remains before the send;
locate and update the pushNotificationWithResult method and the use of
notificationRepository.save and fcmClient.sendMessageWithResult accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/main/java/in/koreatech/koin/domain/notification/eventlistener/ArticleKeywordEventListener.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/eventlistener/ArticleKeywordEventListenerTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/in/koreatech/koin/infrastructure/fcm/FcmClient.java
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/domain/notification/service/NotificationService.java (1)
57-69:⚠️ Potential issue | 🟠 MajorLegacy send path still persists undelivered notifications.
Line 58 saves before delivery, and Line 165 still routes dining notifications through this path. If FCM send fails, the DB can still record it as sent, which conflicts with the success-only persistence behavior introduced in this PR.
Proposed fix
`@Transactional`(propagation = Propagation.NOT_SUPPORTED) public void pushNotification(Notification notification) { - notificationRepository.save(notification); - String deviceToken = notification.getUser().getDeviceToken(); - fcmClient.sendMessage( - deviceToken, - notification.getTitle(), - notification.getMessage(), - notification.getImageUrl(), - notification.getMobileAppPath(), - notification.getSchemeUri(), - notification.getType().toLowerCase() - ); + pushNotificationWithResult(notification); }Also applies to: 153-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java` around lines 57 - 69, The current pushNotification method saves the Notification via notificationRepository.save(notification) before calling fcmClient.sendMessage, leaving undelivered notifications persisted; change the flow so the Notification is only persisted after a successful FCM send. Specifically, modify pushNotification to call fcmClient.sendMessage(...) first, check the send result/acknowledgement or catch exceptions from fcmClient, and only then call notificationRepository.save(notification); for failures, do not save and instead propagate or log the error. Also update any callers that route dining notifications through the legacy path (the block around the dining-notification routing that invokes pushNotification) to follow the same send-then-save pattern or to use the updated pushNotification behavior so failed sends are not persisted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In
`@src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.java`:
- Around line 57-69: The current pushNotification method saves the Notification
via notificationRepository.save(notification) before calling
fcmClient.sendMessage, leaving undelivered notifications persisted; change the
flow so the Notification is only persisted after a successful FCM send.
Specifically, modify pushNotification to call fcmClient.sendMessage(...) first,
check the send result/acknowledgement or catch exceptions from fcmClient, and
only then call notificationRepository.save(notification); for failures, do not
save and instead propagate or log the error. Also update any callers that route
dining notifications through the legacy path (the block around the
dining-notification routing that invokes pushNotification) to follow the same
send-then-save pattern or to use the updated pushNotification behavior so failed
sends are not persisted.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/in/koreatech/koin/domain/notification/service/NotificationService.javasrc/test/java/in/koreatech/koin/unit/domain/notification/service/NotificationServiceTest.java
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 14 out of 14 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
44
to
46
| @TransactionalEventListener | ||
| public void onKeywordRequest(ArticleKeywordEvent event) { | ||
| Article article = articleRepository.getById(event.articleId()); |
There was a problem hiding this comment.
onKeywordRequest가 pushNotificationsWithResult를 호출하면서 FCM 전송 결과를 기다리게 되어(현재 sendMessageWithResult는 동기 호출), 이벤트 발행 스레드를 오래 점유할 수 있습니다. 다른 알림 이벤트 리스너들처럼 @Async + @TransactionalEventListener(phase = AFTER_COMMIT)로 비동기 처리해 요청 지연/스레드 고갈 위험을 줄이는 게 안전합니다.
Comment on lines
48
to
+52
| if (targetDeviceToken == null) { | ||
| return; | ||
| return false; | ||
| } | ||
| log.info("call FcmClient sendMessage: title: {}, content: {}", title, content); | ||
| try { | ||
| log.info("call FcmClient sendMessage: title: {}, content: {}", title, content); |
There was a problem hiding this comment.
sendMessageWithResult는 targetDeviceToken == null만 필터링하고 있어 빈 문자열/공백 토큰이 들어오면 불필요한 Firebase 호출 및 warn 로그가 발생할 수 있습니다. StringUtils.isBlank(targetDeviceToken) 같은 체크로 조기 반환(false) 처리하는 편이 안전합니다.
| @Transactional | ||
| @Transactional(propagation = Propagation.NOT_SUPPORTED) | ||
| public List<NotificationDeliveryResult> pushNotificationsWithResult(List<Notification> notifications) { | ||
| return notifications.stream() |
There was a problem hiding this comment.
pushNotificationsWithResult가 알림 개수만큼 sendMessageWithResult를 순차 호출하므로(네트워크 I/O), 대상자가 많을 때 처리 시간이 선형으로 증가합니다. Firebase Admin SDK의 batch 전송(sendAll/sendEachForMulticast 등)으로 묶어서 보내고 결과를 매핑하면 지연과 호출 수를 크게 줄일 수 있습니다.
Suggested change
| return notifications.stream() | |
| return notifications.parallelStream() |
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
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.
🔍 개요
근로,근로장학/A,C) 사용자에게 동일 게시글 알림이 중복 발송되었습니다.🚀 주요 변경 내용
ArticleKeywordEvent를 단일 키워드에서matchedKeywords(다중 키워드) 구조로 변경KeywordExtractor에서 게시글당 이벤트를 1건만 생성하도록 변경KeywordService.sendKeywordNotification에서 중복 게시글 ID를 제거 후 처리하도록 변경ArticleKeywordEventListener에서UserNotificationStatus저장 로직을 upsert 방식(기존 row 갱신)으로 보완하여 충돌 가능성 제거💬 참고 사항
/articles/keyword/notification을 호출하는 흐름에서 재현되던 중복 알림을 기준으로 수정했습니다.KeywordExtractorTestKeywordServiceTestArticleKeywordEventListenerTest./gradlew test --rerun-tasks전체 테스트 통과✅ Checklist (완료 조건)
Summary by CodeRabbit
New Features
Bug Fixes
Tests