Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements a Web Push notification system, including subscription management, event-driven triggers for promise confirmations, and integration with the web-push library. The review feedback identifies several areas for improvement: reusing the PushService instance to prevent resource leaks, utilizing a Spring-managed ObjectMapper for consistent serialization, and optimizing database performance by batching notification saves. Furthermore, the reviewer pointed out a structural mismatch in the fallback JSON payload and recommended moving the test HTML file out of the public static resources to avoid security risks in production environments.
| PushService pushService = new PushService( | ||
| properties.getVapidPublicKey(), | ||
| properties.getVapidPrivateKey(), | ||
| properties.getVapidSubject() | ||
| ); |
There was a problem hiding this comment.
PushService를 매번 생성하면 내부적으로 새로운 CloseableHttpClient가 생성되지만 명시적으로 닫히지 않아 리소스 누수(File Descriptor 고갈)가 발생할 수 있습니다. PushService를 빈으로 등록하거나 멤버 변수로 유지하여 재사용하는 것을 권장합니다.
| private final NotificationRepository notificationRepository; | ||
| private final PushSubscriptionRepository pushSubscriptionRepository; | ||
| private final WebPushSender webPushSender; | ||
| private final ObjectMapper objectMapper = new ObjectMapper(); |
There was a problem hiding this comment.
ObjectMapper를 직접 생성하는 대신 Spring에서 제공하는 빈을 주입받아 사용하는 것이 좋습니다. 이를 통해 프로젝트의 공통 직렬화 설정을 일관되게 유지할 수 있습니다.
| private final ObjectMapper objectMapper = new ObjectMapper(); | |
| private final ObjectMapper objectMapper; |
| notification.markSent(now); | ||
| } | ||
|
|
||
| notificationRepository.save(notification); |
There was a problem hiding this comment.
루프 내에서 notificationRepository.save()를 호출하면 멤버 수만큼 쿼리가 발생합니다. 알림 객체들을 리스트에 모은 뒤 루프 외부에서 saveAll()을 호출하여 성능을 최적화할 수 있습니다.
| )); | ||
| } catch (JsonProcessingException e) { | ||
| log.warn("Failed to serialize promise confirmed push payload. promiseId={}", event.promiseId(), e); | ||
| return "{\"title\":\"%s\",\"body\":\"%s\",\"url\":\"%s\"}".formatted(title, body, linkUrl); |
There was a problem hiding this comment.
직렬화 실패 시 반환되는 폴백 JSON 문자열이 WebPushPayload 레코드의 구조(type, targetId 누락)와 일치하지 않습니다. 클라이언트에서 해당 필드들을 필수값으로 처리하고 있다면 런타임 에러가 발생할 수 있으므로 필드를 추가하거나 일관성을 맞추는 것이 좋습니다.
| @@ -0,0 +1,317 @@ | |||
| <!doctype html> | |||
There was a problem hiding this comment.
테스트용 HTML 파일이 src/main/resources/static에 포함되어 있어 운영 환경에서도 외부 접근이 가능합니다. 보안 및 리소스 관리를 위해 이 파일을 src/test/resources로 옮기거나 빌드 시 제외되도록 설정하는 것을 권장합니다.
1 participant
🔍 관련 이슈
✅ 작업 분류
✨ 작업 내용
👥 전달사항
✅ 체크리스트
📸 스크린샷
💡 배운 것 / 시도한 것 / 고민한 점