[FEAT/#8] fcm 모듈 세팅#25
Conversation
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (6)
core/fcm/src/main/java/com/ssing/core/notification/NotificationTokenProvider.kt (2)
12-20: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value취소 시 콜백 정리 로직 부재
suspendCancellableCoroutine취소 시Task리스너가 계속 살아있을 수 있습니다.invokeOnCancellation으로 정리하는 것이 안전합니다.🤖 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 `@core/fcm/src/main/java/com/ssing/core/notification/NotificationTokenProvider.kt` around lines 12 - 20, The getToken() suspendCancellableCoroutine currently leaves FirebaseMessaging.getInstance().token listeners active if the coroutine is cancelled. Update NotificationTokenProvider.getToken() to register cancellation cleanup with invokeOnCancellation and remove or ignore the Task callbacks appropriately, while keeping the existing success/failure handling in the getToken() continuation flow.
11-20: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
FirebaseMessaging.getInstance()직접 호출 대신 DI 주입 고려싱글턴을 직접 참조하면 테스트 시 목(mock) 대체가 어렵습니다. Hilt 모듈에서
FirebaseMessaging인스턴스를 제공하고 생성자 주입받는 것을 권장합니다.♻️ 제안
-class NotificationTokenProvider `@Inject` constructor() { +class NotificationTokenProvider `@Inject` constructor( + private val firebaseMessaging: FirebaseMessaging, +) { suspend fun getToken(): String? = suspendCancellableCoroutine { continuation -> - FirebaseMessaging.getInstance().token + firebaseMessaging.token .addOnSuccessListener { continuation.resume(it) }🤖 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 `@core/fcm/src/main/java/com/ssing/core/notification/NotificationTokenProvider.kt` around lines 11 - 20, NotificationTokenProvider currently calls FirebaseMessaging.getInstance() directly inside getToken(), which makes it hard to replace in tests. Refactor NotificationTokenProvider to receive FirebaseMessaging through constructor injection, and add a Hilt module/provider that supplies the FirebaseMessaging instance so getToken() uses the injected dependency instead of the singleton lookup.core/fcm/src/main/java/com/ssing/core/notification/PushNotificationService.kt (2)
74-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win시스템 디버그 아이콘 대신 앱 전용 알림 아이콘 사용 권장
android.R.drawable.ic_dialog_info는 시스템 디버그용 아이콘으로, 프로덕션 알림에는 앱 고유의 알림 아이콘 리소스를 사용하는 것이 브랜딩과 사용자 경험 측면에서 바람직합니다.🤖 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 `@core/fcm/src/main/java/com/ssing/core/notification/PushNotificationService.kt` around lines 74 - 76, The notification builder in PushNotificationService should not use the system debug icon android.R.drawable.ic_dialog_info for production alerts. Update the icon passed to NotificationCompat.Builder in the notification creation flow to use an app-specific notification drawable resource instead, and ensure the same app icon resource is used consistently wherever the PushNotificationService builds notifications.
53-53: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value채널 생성 로직을 서비스 생명주기 초기화 시점으로 이동 권장
createChannels가 메시지 수신마다 호출됩니다.createNotificationChannel은 멱등이라 오류는 없지만,onCreate()에서 한 번만 호출하는 것이 더 효율적입니다.🤖 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 `@core/fcm/src/main/java/com/ssing/core/notification/PushNotificationService.kt` at line 53, PushNotificationService에서 createChannels(this)가 메시지 수신 시마다 호출되고 있으니, 채널 생성 책임을 서비스 생명주기 초기화로 옮기세요. createChannels 호출 위치를 메시지 처리 경로에서 제거하고, PushNotificationService의 onCreate()에서 한 번만 실행되도록 변경하면 됩니다. createChannels와 onCreate를 기준으로 위치를 찾아 수정하세요.build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt (1)
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff공용 컨벤션 플러그인에 Firebase 전용 의존성 하드코딩 - 단일 책임 원칙 검토 필요.
AndroidApplicationConventionPlugin은 원래 범용 Android 앱 설정을 담당하는데, 이번 변경으로core:fcm,firebase-bom,firebase-analytics,firebase-messaging이 모든 소비 앱(consumer/instructor)에 무조건 추가됩니다.core:fcm이 이미firebase-bom+firebase-messaging-ktx를 자체적으로 포함하고 있어(컨텍스트 스니펫 참고), 여기서 다시firebase-bom/firebase-messaging을 중복 선언하는 것도 불필요한 결합입니다. Firebase 관련 의존성 설정을 별도의 컨벤션 플러그인(e.g.,AndroidFirebaseConventionPlugin)으로 분리하는 것을 고려해보세요.As per path instructions,
build-logic/**/*.kt는 "한 책임 원칙"을 지켜야 합니다.🤖 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 `@build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt` around lines 53 - 56, `AndroidApplicationConventionPlugin` is now hardcoding Firebase-specific dependencies for every consumer app, which violates the convention plugin’s single-responsibility scope and duplicates what `core:fcm` already brings in. Move the Firebase setup out of `AndroidApplicationConventionPlugin` (the block adding `core:fcm`, `firebase-bom`, `firebase-analytics`, and `firebase-messaging`) into a separate Firebase-focused convention plugin such as `AndroidFirebaseConventionPlugin`, and keep the application plugin generic so only apps that need Firebase apply it.Source: Path instructions
gradle/libs.versions.toml (1)
27-29: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick wincore/fcm/build.gradle.kts:27-29도 버전 카탈로그로 통일하세요.
firebase-bom과firebase-messaging-ktx를 직접 선언하고 있어libs.firebase.bom/libs.firebase.messaging과 불일치합니다.libs로 맞추거나,-ktx를 유지할 거면 카탈로그 별칭도 추가해 주세요.🤖 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 `@gradle/libs.versions.toml` around lines 27 - 29, The version catalog and FCM Gradle usage are out of sync because core/fcm/build.gradle.kts still declares Firebase dependencies directly while libs.firebase.bom and libs.firebase.messaging exist. Update the FCM module to use the version catalog aliases consistently, or if firebase-messaging-ktx must remain, add a matching catalog alias in libs.versions.toml and switch the build script to that alias so dependency naming stays centralized and consistent.Source: Path instructions
🤖 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 `@build.gradle.kts`:
- Line 10: google-services 플러그인이 build.gradle.kts에서 직접 버전 하드코딩되어 있어 version
catalog 규칙과 다른 상태입니다. build.gradle.kts의 plugin 선언을
alias(libs.plugins.google.services) 형태로 바꾸고, gradle/libs.versions.toml의
[plugins] 섹션에 com.google.gms.google-services 항목을 추가해 버전을 중앙 관리하도록 수정하세요. 다른
플러그인들과 동일하게 alias 기반으로 정리해 일관성을 맞추면 됩니다.
In `@core/fcm/build.gradle.kts`:
- Around line 28-29: The Firebase dependencies in the FCM Gradle setup are
hardcoded as string coordinates instead of using the version catalog like the
other convention plugins. Add `firebase-bom` and `firebase-messaging-ktx` to
`libs.versions.toml`, then update the `implementation(...)` entries in the FCM
build script to resolve them through `libs.findLibrary(...)` or the catalog
aliases, keeping the dependency style consistent with
`CoreNetworkConventionPlugin` and the rest of the Gradle conventions.
In `@core/fcm/src/main/AndroidManifest.xml`:
- Line 5: `POST_NOTIFICATIONS` is only declared in the FCM manifest, but the
host apps do not request it at runtime, so add the permission request flow in
both ConsumerMainActivity and InstructorMainActivity. Update the
startup/onboarding path in com.ssing.consumer.ConsumerMainActivity and
com.ssing.instructor.InstructorMainActivity to check Android 13+ notification
permission and request it when missing, handling the user response before
relying on FCM notifications.
In
`@core/fcm/src/main/java/com/ssing/core/notification/PushNotificationService.kt`:
- Around line 50-57: The current PushNotificationService.onMessageReceived path
depends on message.notification, which will not run for background/terminated
notification-only FCM payloads. Update the FCM flow so server messages use
data-only payloads for custom channel/deeplink handling, and change
onMessageReceived to read from message.data instead of message.notification;
keep createChannels(this) and showNotification(title, body) wired to the data
fields so the delivery path works consistently.
In
`@data/src/main/java/com/ssing/data/repository/impl/NotificationDataSourceImpl.kt`:
- Around line 8-15: `saveNotificationToken` in `NotificationRepositoryImpl` is
currently returning the `BaseResponse<Unit>` from
`dataSource.postNotificationToken(...)`, so the `runCatching` result becomes
`Result<BaseResponse<Unit>>` instead of `Result<Unit>`. Update
`saveNotificationToken` to discard the response from `postNotificationToken` and
end the `runCatching` block with `Unit`, keeping the repository contract as
`Result<Unit>`.
In
`@data/src/main/java/com/ssing/data/repository/impl/NotificationRepositoryImpl.kt`:
- Around line 9-18: The file/class mapping is swapped: the current file contains
NotificationDataSourceImpl while the neighboring NotificationDataSourceImpl file
contains NotificationRepositoryImpl. Update the affected pair so each file name
matches its primary class, either by swapping the contents between the two
implementations or renaming the files to align with NotificationRepositoryImpl
and NotificationDataSourceImpl, and keep the constructor/API usage in each class
unchanged.
---
Nitpick comments:
In
`@build-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.kt`:
- Around line 53-56: `AndroidApplicationConventionPlugin` is now hardcoding
Firebase-specific dependencies for every consumer app, which violates the
convention plugin’s single-responsibility scope and duplicates what `core:fcm`
already brings in. Move the Firebase setup out of
`AndroidApplicationConventionPlugin` (the block adding `core:fcm`,
`firebase-bom`, `firebase-analytics`, and `firebase-messaging`) into a separate
Firebase-focused convention plugin such as `AndroidFirebaseConventionPlugin`,
and keep the application plugin generic so only apps that need Firebase apply
it.
In
`@core/fcm/src/main/java/com/ssing/core/notification/NotificationTokenProvider.kt`:
- Around line 12-20: The getToken() suspendCancellableCoroutine currently leaves
FirebaseMessaging.getInstance().token listeners active if the coroutine is
cancelled. Update NotificationTokenProvider.getToken() to register cancellation
cleanup with invokeOnCancellation and remove or ignore the Task callbacks
appropriately, while keeping the existing success/failure handling in the
getToken() continuation flow.
- Around line 11-20: NotificationTokenProvider currently calls
FirebaseMessaging.getInstance() directly inside getToken(), which makes it hard
to replace in tests. Refactor NotificationTokenProvider to receive
FirebaseMessaging through constructor injection, and add a Hilt module/provider
that supplies the FirebaseMessaging instance so getToken() uses the injected
dependency instead of the singleton lookup.
In
`@core/fcm/src/main/java/com/ssing/core/notification/PushNotificationService.kt`:
- Around line 74-76: The notification builder in PushNotificationService should
not use the system debug icon android.R.drawable.ic_dialog_info for production
alerts. Update the icon passed to NotificationCompat.Builder in the notification
creation flow to use an app-specific notification drawable resource instead, and
ensure the same app icon resource is used consistently wherever the
PushNotificationService builds notifications.
- Line 53: PushNotificationService에서 createChannels(this)가 메시지 수신 시마다 호출되고 있으니,
채널 생성 책임을 서비스 생명주기 초기화로 옮기세요. createChannels 호출 위치를 메시지 처리 경로에서 제거하고,
PushNotificationService의 onCreate()에서 한 번만 실행되도록 변경하면 됩니다. createChannels와
onCreate를 기준으로 위치를 찾아 수정하세요.
In `@gradle/libs.versions.toml`:
- Around line 27-29: The version catalog and FCM Gradle usage are out of sync
because core/fcm/build.gradle.kts still declares Firebase dependencies directly
while libs.firebase.bom and libs.firebase.messaging exist. Update the FCM module
to use the version catalog aliases consistently, or if firebase-messaging-ktx
must remain, add a matching catalog alias in libs.versions.toml and switch the
build script to that alias so dependency naming stays centralized and
consistent.
🪄 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: ab9dec5d-f527-4468-81c0-15eeb332b63a
⛔ Files ignored due to path filters (20)
core/fcm/.gitignoreis excluded by none and included by nonecore/fcm/proguard-rules.prois excluded by none and included by nonecore/fcm/src/main/res/drawable/ic_launcher_background.xmlis excluded by none and included by nonecore/fcm/src/main/res/drawable/ic_launcher_foreground.xmlis excluded by none and included by nonecore/fcm/src/main/res/mipmap-anydpi-v26/ic_launcher.xmlis excluded by none and included by nonecore/fcm/src/main/res/mipmap-anydpi-v26/ic_launcher_round.xmlis excluded by none and included by nonecore/fcm/src/main/res/mipmap-hdpi/ic_launcher.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-hdpi/ic_launcher_round.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-mdpi/ic_launcher.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-mdpi/ic_launcher_round.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-xhdpi/ic_launcher.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-xhdpi/ic_launcher_round.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-xxhdpi/ic_launcher.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-xxhdpi/ic_launcher_round.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-xxxhdpi/ic_launcher.webpis excluded by none and included by nonecore/fcm/src/main/res/mipmap-xxxhdpi/ic_launcher_round.webpis excluded by none and included by nonecore/fcm/src/main/res/values-night/themes.xmlis excluded by none and included by nonecore/fcm/src/main/res/values/colors.xmlis excluded by none and included by nonecore/fcm/src/main/res/values/strings.xmlis excluded by none and included by nonecore/fcm/src/main/res/values/themes.xmlis excluded by none and included by none
📒 Files selected for processing (20)
app/consumer/build.gradle.ktsapp/consumer/src/main/AndroidManifest.xmlapp/instructor/build.gradle.ktsbuild-logic/convention/src/main/kotlin/AndroidApplicationConventionPlugin.ktbuild.gradle.ktscore/fcm/build.gradle.ktscore/fcm/src/main/AndroidManifest.xmlcore/fcm/src/main/java/com/ssing/core/auth/manager/AuthManager.ktcore/fcm/src/main/java/com/ssing/core/notification/NotificationTokenProvider.ktcore/fcm/src/main/java/com/ssing/core/notification/PushNotificationService.ktcore/network/src/main/java/com/ssing/core/network/di/NetworkModule.ktdata/src/main/java/com/ssing/data/di/NotificationModule.ktdata/src/main/java/com/ssing/data/remote/datasource/NotificationDataSource.ktdata/src/main/java/com/ssing/data/remote/dto/NotificationRequestDto.ktdata/src/main/java/com/ssing/data/repository/NotificationRepository.ktdata/src/main/java/com/ssing/data/repository/api/NotificationApi.ktdata/src/main/java/com/ssing/data/repository/impl/NotificationDataSourceImpl.ktdata/src/main/java/com/ssing/data/repository/impl/NotificationRepositoryImpl.ktgradle/libs.versions.tomlsettings.gradle.kts
| <manifest xmlns:android="http://schemas.android.com/apk/res/android"> | ||
|
|
||
| <uses-permission android:name="android.permission.INTERNET" /> | ||
| <uses-permission android:name="android.permission.POST_NOTIFICATIONS" /> |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: POST_NOTIFICATIONS 런타임 권한 요청 코드 검색
rg -n -C3 'POST_NOTIFICATIONS|requestPermission' --type kotlinRepository: TEAM-SSING/SSING-ANDROID
Length of output: 162
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect the manifest around the reported line.
manifest="core/fcm/src/main/AndroidManifest.xml"
if [ -f "$manifest" ]; then
nl -ba "$manifest" | sed -n '1,80p'
fi
# 2) Search broadly for notification permission request flows in the repository.
rg -n -C 3 --hidden --glob '!**/build/**' --glob '!**/.gradle/**' \
'POST_NOTIFICATIONS|requestPermissions\(|shouldShowRequestPermissionRationale|NotificationManagerCompat|areNotificationsEnabled|ACTION_APP_NOTIFICATION_SETTINGS|ActivityCompat\.requestPermissions|registerForActivityResult|ActivityResultContracts\.RequestPermission' .Repository: TEAM-SSING/SSING-ANDROID
Length of output: 202
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find modules and entry points related to app/consumer and app/instructor to see
# whether runtime permission prompting is likely handled there.
git ls-files | rg '(^|/)(app|core|.*fcm).*'Repository: TEAM-SSING/SSING-ANDROID
Length of output: 7948
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# If permission code exists in non-Kotlin sources, search those too.
rg -n -C 3 --hidden --glob '!**/build/**' --glob '!**/.gradle/**' \
'POST_NOTIFICATIONS|requestPermissions\(|ActivityCompat\.requestPermissions|registerForActivityResult|RequestPermission|NotificationManagerCompat|areNotificationsEnabled' \
--glob '*.{kt,kts,java,xml,txt,md}' .Repository: TEAM-SSING/SSING-ANDROID
Length of output: 707
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the manifest directly and search for common notification permission patterns
# that may not include the exact keywords from the previous search.
sed -n '1,40p' core/fcm/src/main/AndroidManifest.xml
printf '\n--- search ---\n'
rg -n -C 2 --hidden --glob '!**/build/**' --glob '!**/.gradle/**' \
'rememberPermissionState|PermissionState|ActivityResultContracts\.RequestPermission|areNotificationsEnabled\(|ACTION_APP_NOTIFICATION_SETTINGS|NotificationManagerCompat|notification.*permission|권한.*알림|알림.*권한' \
.Repository: TEAM-SSING/SSING-ANDROID
Length of output: 771
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for any notification-related runtime permission handling in the app entry points.
rg -n -C 3 --hidden --glob '!**/build/**' --glob '!**/.gradle/**' \
'notification|알림|POST_NOTIFICATIONS|Permission' \
app/consumer/src/main/java app/instructor/src/main/java core/fcm/src/main/javaRepository: TEAM-SSING/SSING-ANDROID
Length of output: 7248
POST_NOTIFICATIONS 요청 흐름을 호스트 앱에 추가하세요.
core/fcm/src/main/AndroidManifest.xml에 권한만 선언되어 있고, app/consumer/src/main/java/com/ssing/consumer/ConsumerMainActivity.kt와 app/instructor/src/main/java/com/ssing/instructor/InstructorMainActivity.kt 쪽에 런타임 요청 처리가 없습니다. API 33+에서는 이 권한이 없으면 알림이 표시되지 않습니다.
🤖 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 `@core/fcm/src/main/AndroidManifest.xml` at line 5, `POST_NOTIFICATIONS` is
only declared in the FCM manifest, but the host apps do not request it at
runtime, so add the permission request flow in both ConsumerMainActivity and
InstructorMainActivity. Update the startup/onboarding path in
com.ssing.consumer.ConsumerMainActivity and
com.ssing.instructor.InstructorMainActivity to check Android 13+ notification
permission and request it when missing, handling the user response before
relying on FCM notifications.
Source: Path instructions
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -a 'ConsumerMainActivity.kt|InstructorMainActivity.kt' app || true
echo "---consumer---"
fd 'ConsumerMainActivity.kt' -x cat -n {}
echo "---instructor---"
fd 'InstructorMainActivity.kt' -x cat -n {}
echo "---gradle sdk---"
rg -n 'compileSdk|targetSdk|minSdk' --type kotlin app build-logic/convention/src/main/kotlinLength of output: 8007
@oilbeaneda 넵, 리뷰 반영해서 두 Activity에 런타임 알림 권한 요청 플로우를 추가하겠습니다.
Plan
ConsumerMainActivity.kt와InstructorMainActivity.kt에ActivityResultContracts.RequestPermission()을 이용한POST_NOTIFICATIONS런타임 권한 요청 launcher를 추가onCreate()에서 API 33(TIRAMISU) 이상일 때만 권한 상태를 확인 후 미허용 시 요청하도록 구현 (minSdk가 28이므로 버전 분기 필수)
minSdk가 28이라 Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU 분기가 필요합니다. 권한 결과 콜백은 현재 no-op으로 두었으니, 거부 시 안내 UI가 필요하다면 알려주세요.
- 📌 Commit the changes to this branch
| override fun onMessageReceived(message: RemoteMessage) { | ||
| super.onMessageReceived(message) | ||
|
|
||
| createChannels(this) | ||
| val title = message.notification?.title ?: return | ||
| val body = message.notification?.body ?: return | ||
| showNotification(title, body) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
FCM notification-only 메시지 백그라운드 상태에서 onMessageReceived 호출 여부
💡 Result:
안드로이드 환경에서 FCM(Firebase Cloud Messaging) '알림 전용(notification-only)' 메시지를 앱이 백그라운드 상태일 때 수신하면, Firebase SDK는 개발자의 onMessageReceived 콜백을 호출하지 않습니다 [1][2][3]. 이 경우, FCM SDK가 시스템 알림 센터(System Tray)에 알림을 자동으로 표시합니다 [2][4]. 사용자가 이 알림을 탭하면, 앱의 런처 액티비티(Launcher Activity)가 실행되며 알림 데이터는 해당 인텐트(Intent)의 Extras에 포함되어 전달됩니다 [2][5][6]. 만약 백그라운드 상태에서도 개발자가 직접 메시지를 처리해야 하거나 onMessageReceived 콜백을 호출하고 싶다면, '알림' 페이로드를 포함하지 않는 '데이터 전송 전용(data-only)' 메시지를 전송해야 합니다 [7][6]. 데이터 전송 전용 메시지는 앱의 상태와 관계없이 항상 onMessageReceived를 호출하므로, 이를 통해 백그라운드에서도 로직을 수행하거나 직접 커스텀 알림을 생성할 수 있습니다 [7][6].
Citations:
- 1: https://firebase.google.com/docs/cloud-messaging/android/receive-messages
- 2: https://firebase.google.com/docs/cloud-messaging/customize-messages/set-message-type
- 3: https://firebase.google.com/docs/reference/android/com/google/firebase/messaging/FirebaseMessagingService
- 4: https://firebase.blog/posts/2018/09/handle-fcm-messages-on-android/
- 5: https://stackoverflow.com/questions/58877935/receive-fcm-data-messages-in-service-when-the-app-is-not-in-foreground
- 6: https://stackoverflow.com/questions/37711082/how-to-handle-notification-when-app-in-background-in-firebase
- 7: https://stackoverflow.com/questions/40311279/firebase-onmessagereceived-not-called-when-app-is-in-the-background
백그라운드용 FCM은 notification-only에 의존하면 안 됩니다
앱이 백그라운드/종료 상태에서 notification만 포함된 메시지는 onMessageReceived가 호출되지 않습니다. 이 경로에서 커스텀 채널/딥링크 처리가 필요하면 서버 payload를 data-only로 맞추고, 이 서비스도 message.data를 읽도록 바꿔야 합니다.
🤖 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
`@core/fcm/src/main/java/com/ssing/core/notification/PushNotificationService.kt`
around lines 50 - 57, The current PushNotificationService.onMessageReceived path
depends on message.notification, which will not run for background/terminated
notification-only FCM payloads. Update the FCM flow so server messages use
data-only payloads for custom channel/deeplink handling, and change
onMessageReceived to read from message.data instead of message.notification;
keep createChannels(this) and showNotification(title, body) wired to the data
fields so the delivery path works consistently.
| class NotificationRepositoryImpl @Inject constructor( | ||
| private val dataSource: NotificationDataSource | ||
| ) : NotificationRepository { | ||
|
|
||
| override suspend fun saveNotificationToken(token: String): Result<Unit> = runCatching { | ||
| dataSource.postNotificationToken(NotificationRequestDto(token = token)) | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: NotificationRepositoryImpl(실제로는 NotificationDataSourceImpl.kt 파일에 위치)의 반환 타입 확인
ast-grep run --pattern 'class NotificationRepositoryImpl $$$ { $$$ saveNotificationToken($_) : Result<Unit> = runCatching { $$$ } $$$ }' --lang kotlin data/src/main/java/com/ssing/data/repository/implRepository: TEAM-SSING/SSING-ANDROID
Length of output: 565
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files:\n'
git ls-files 'data/src/main/java/com/ssing/data/repository/impl/*' | sed -n '1,20p'
printf '\nMatching symbols:\n'
rg -n 'class NotificationRepositoryImpl|saveNotificationToken|postNotificationToken|NotificationRequestDto|interface NotificationDataSource|interface NotificationRepository' data/src/main/java/com/ssing -S
printf '\nFile content with line numbers:\n'
cat -n data/src/main/java/com/ssing/data/repository/impl/NotificationDataSourceImpl.ktRepository: TEAM-SSING/SSING-ANDROID
Length of output: 3237
saveNotificationToken의 반환 타입을 맞춰야 합니다. runCatching의 마지막 식이 BaseResponse<Unit>라서 현재 구현은 Result<BaseResponse<Unit>>로 추론됩니다. Result<Unit>을 유지하려면 응답을 버리고 Unit을 반환하세요.
🤖 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
`@data/src/main/java/com/ssing/data/repository/impl/NotificationDataSourceImpl.kt`
around lines 8 - 15, `saveNotificationToken` in `NotificationRepositoryImpl` is
currently returning the `BaseResponse<Unit>` from
`dataSource.postNotificationToken(...)`, so the `runCatching` result becomes
`Result<BaseResponse<Unit>>` instead of `Result<Unit>`. Update
`saveNotificationToken` to discard the response from `postNotificationToken` and
end the `runCatching` block with `Unit`, keeping the repository contract as
`Result<Unit>`.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@core/fcm/build.gradle.kts`:
- Line 9: `core/fcm`의 compileSdk 설정이 공통 AGP/컨벤션과 불일치합니다. `build.gradle.kts`의
compileSdk 값을 공통 설정과 맞추도록 조정하고, 필요하다면 `gradle/libs.versions.toml`의 AGP 버전 및 관련
컨벤션과 함께 일관되게 올리거나 현재 기준인 35로 통일하세요. 위치는 `compileSdk` 선언과 관련된 모듈 설정을 확인해
`core/fcm`만 별도로 37을 쓰지 않도록 정리하면 됩니다.
🪄 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: 51da614a-591a-4ff6-8ac1-cd2b09dc896a
📒 Files selected for processing (2)
core/fcm/build.gradle.ktsgradle/libs.versions.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- gradle/libs.versions.toml
There was a problem hiding this comment.
♻️ Duplicate comments (1)
data/src/main/java/com/ssing/data/repository/impl/NotificationRepositoryImpl.kt (1)
12-14: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
saveNotificationToken의 반환 타입 불일치 — 컴파일 오류 발생 가능.
runCatching블록의 마지막 식이dataSource.postNotificationToken(...)이며 이는BaseResponse<Unit>을 반환합니다. 따라서runCatching의 추론 타입은Result<BaseResponse<Unit>>가 되어, 선언된 반환 타입Result<Unit>과 충돌합니다.Result는 불변(invariant) 제네릭이므로 컴파일 오류가 발생합니다.이는 이전 리뷰에서 (당시 파일이 뒤바뀐 상태에서) 이미 지적된 것과 동일한 문제로, 파일명/클래스명 스왑 이후에도 로직 자체는 수정되지 않은 채 이 파일로 이동한 것으로 보입니다.
🐛 반환 타입 수정 제안
override suspend fun saveNotificationToken(token: String): Result<Unit> = runCatching { - dataSource.postNotificationToken(NotificationRequestDto(token = token)) + dataSource.postNotificationToken(NotificationRequestDto(token = token)) + Unit }🤖 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 `@data/src/main/java/com/ssing/data/repository/impl/NotificationRepositoryImpl.kt` around lines 12 - 14, `saveNotificationToken`의 `runCatching` 결과 타입이 `dataSource.postNotificationToken(...)` 때문에 `Result<BaseResponse<Unit>>`로 추론되어 선언된 `Result<Unit>`와 불일치합니다. `NotificationRepositoryImpl.saveNotificationToken`에서 `runCatching` 블록의 반환값을 `Unit`으로 맞추도록 `postNotificationToken` 호출 결과를 소비하고, 성공 시 `Unit`을 반환하도록 수정해 타입 충돌을 없애세요.
🤖 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.
Duplicate comments:
In
`@data/src/main/java/com/ssing/data/repository/impl/NotificationRepositoryImpl.kt`:
- Around line 12-14: `saveNotificationToken`의 `runCatching` 결과 타입이
`dataSource.postNotificationToken(...)` 때문에 `Result<BaseResponse<Unit>>`로 추론되어
선언된 `Result<Unit>`와 불일치합니다. `NotificationRepositoryImpl.saveNotificationToken`에서
`runCatching` 블록의 반환값을 `Unit`으로 맞추도록 `postNotificationToken` 호출 결과를 소비하고, 성공 시
`Unit`을 반환하도록 수정해 타입 충돌을 없애세요.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 932fccc0-e012-4305-b882-fff51e5f673a
📒 Files selected for processing (2)
data/src/main/java/com/ssing/data/repository/impl/NotificationDataSourceImpl.ktdata/src/main/java/com/ssing/data/repository/impl/NotificationRepositoryImpl.kt
apffkxhsls
force-pushed
the
feat/#8-fcm_setting
branch
from
bfe663c to
3c7b69f
Compare
apffkxhsls
force-pushed
the
feat/#8-fcm_setting
branch
from
466ffce to
9545375
Compare
oilbeaneda
left a comment
oilbeaneda
left a comment
There was a problem hiding this comment.
예지너무 고생많았다.............. 빌드 오류에 ....... IDE 에러까지 .......... 최강예지 너무 잘해요 ....
| override fun onNewToken(token: String) { | ||
| super.onNewToken(token) | ||
|
|
||
| if (!authManager.isLoggedIn.value) return |
There was a problem hiding this comment.
p1: 토큰 저장이 AuthManager.isLoggedIn에 의해 게이트되는데, setLoggedIn(true)를 호출하는 곳이 어디에도 없어서 항상 false로 남고있습니당
AuthManager를 아예 없애고, core/notification 모듈이 core/network에 의존하도록 만든 뒤(build.gradle.kts에 project(":core:network") 추가), PushNotificationService.onNewToken에서 TokenAccessManager.getAccessToken() != null 같이 로그인 상태를 직접 확인하도록 변경해주면 좋을 것 같아요 ㅎㅎ
There was a problem hiding this comment.
저는 AuthManager 지우는 건 찬성! 그치만 NotificationTokenProvider는 NotificationRepository 하나만 주입받으면 좋겠어용
이유는 NotificationTokenProvider의 책임이 새 FCM 토큰이 오면 서버에 전송한다에 그쳐야 한다고 생각하기 때문입니당.
NotificationTokenProvider가 로그인 상태인지 확인하고, 로그인 상태가 맞다면 서버에 전송한다는 과책임 같아요. 그래서 직접 로컬데이터에 접근하지 않으면 좋겠습니다.
저는 NotificationTokenProvider은 notificationRepository.saveNotificationToken(token)만 처리하고
추후 notificationRepositoryImpl.saveNotificationToken(token) 구현 시에 TokenAccessManager.getAccessToken() != null로 토큰 확인하는 방향을 추천드립니당
class NotificationRepositoryImpl @Inject constructor(
private val dataSource: NotificationDataSource,
private val tokenAccessManager: TokenAccessManager,
) : NotificationRepository {
override suspend fun saveNotificationToken(token: String): Result<Unit> {
val accessToken = tokenAccessManager.getAccessToken()
if (accessToken == null) {
Timber.d("로그인 상태가 아니라 FCM 토큰을 서버에 전송하지 않음")
return Result.success(Unit)
}
// API 호출해 서버에 토큰 전송
} There was a problem hiding this comment.
크크 이해됐어요 로직은 Impl 지녀야하는 게 맞는 거같아요 도짱 만세 찬성!!
There was a problem hiding this comment.
흐엉 감사합니다 근데 저도 예전에 서비스 안에서 토큰 직접 읽었답니다 과거의 나와 싸우는중
There was a problem hiding this comment.
오 NotificationTokenProvider는 토큰을 제공하는 역할만 하고 행동하는 역할(impl)이랑 분리해서 캡슐화하는 거군요! 조은것같아요 ㅎㅎ
| import javax.inject.Inject | ||
|
|
||
| @AndroidEntryPoint | ||
| class PushNotificationService : FirebaseMessagingService() { |
There was a problem hiding this comment.
p1: 이와 비슷하게getToken()을 호출하는 곳이 전체 저장소에 없어서 NotificationTokenProvider.getToken()을 실제로 호출해줄 진입점이 필요할 것 같아요!!
There was a problem hiding this comment.
p1: 토큰 저장이 AuthManager.isLoggedIn에 의해 게이트되는데, setLoggedIn(true)를 호출하는 곳이 어디에도 없어서 항상 false로 남고있습니당
AuthManager를 아예 없애고, core/notification 모듈이 core/network에 의존하도록 만든 뒤(build.gradle.kts에 project(":core:network") 추가), PushNotificationService.onNewToken에서 TokenAccessManager.getAccessToken() != null 같이 로그인 상태를 직접 확인하도록 변경해주면 좋을 것 같아요 ㅎㅎ
이건가??
| <manifest xmlns:android="http://schemas.android.com/apk/res/android"> | ||
|
|
||
| <uses-permission android:name="android.permission.INTERNET" /> | ||
| <uses-permission android:name="android.permission.POST_NOTIFICATIONS" /> |
There was a problem hiding this comment.
p1: : 요거CoreNotificationConventionPlugin만들어서 다른 것처럼 관리해주면 좋을 것 같아요! 저기 밑에다가 생성해주시면 됩니당
| namespace = "com.ssing.core.notification" | ||
|
|
||
| defaultConfig { | ||
| minSdk = 24 |
There was a problem hiding this comment.
p1: : AndroidLibraryConventionPlugin에서는 minSdk = 28을 기본값으로 사용하고 있어서 맞춰주면 좋을 것 같아용
There was a problem hiding this comment.
p1 :인증 상태 관리하는AuthManager가 core:notification 모듈 내부에 위치하고 있으면 안될 것 같우요 현재 구조는 알림 모듈이 로그인 로직까지 책임지고있는데 별도의 :core: auth 모듈을 생성하기 보다 :core:network 모듈의 AuthSessionManager와 통합하면 어떨까요?
There was a problem hiding this comment.
헉 그렇네요 AuthMaganger를 notification 안에 넣어놨구나...!! 바로 수정했습니다
| import javax.inject.Inject | ||
|
|
||
| class NotificationDataSourceImpl @Inject constructor( | ||
| private val api: NotificationApi, |
There was a problem hiding this comment.
| private val api: NotificationApi, | |
| private val apiResponseHandler: ApiResponseHandler, | |
| private val dataSource: NotificationDataSource, |
p1:ApiResponseHandler/suspendRunCatching 요거 도도가 만들어둔거 쓰자1!
There was a problem hiding this comment.
이거모냐면 예지가 데이커소스를 레포지토리처럼 구현해서 유빈언냐가 이가 데이터소스아니고 레포지토리인줄알고 데이터소스 주입시켜준거임 난 뉴런공유 중이라 알아
There was a problem hiding this comment.
NotificationDataSourceImpl 파일에 private val dataSource: NotificationDataSource, 를 적어도 되나...? 빌드가 안 되는 것 같은데!! 😢
| override suspend fun deleteNotificationToken(token: String): BaseResponse<Unit> = | ||
| api.deleteNotificationToken(token) | ||
| } |
There was a problem hiding this comment.
p1:
| override suspend fun deleteNotificationToken(token: String): BaseResponse<Unit> = | |
| api.deleteNotificationToken(token) | |
| } | |
| override suspend fun saveNotificationToken(token: String): Result<Unit> = | |
| apiResponseHandler.safeApiCall { | |
| dataSource.postNotificationToken(NotificationRequestDto(token = token)) | |
| } |
이렇게 ㅎㅎ
There was a problem hiding this comment.
p1 : 이거 서버 명세상 성공 시에 바디 자체를 안줘서 safeApiCall 말고 꼭 safeUnitApiCall 사용해주세요!!
| companion object { | ||
| private const val DEFAULT_CHANNEL_ID = "default_channel" | ||
| private const val DEFAULT_CHANNEL_NAME = "기본 알림" | ||
|
|
There was a problem hiding this comment.
| private val channelsCreated = AtomicBoolean(false) |
| } | ||
| } | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
| } | |
| if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return | |
| if (!channelsCreated.compareAndSet(false, true)) return | |
| val notificationManager = | |
| context.getSystemService(NOTIFICATION_SERVICE) as NotificationManager | |
| notificationManager.createNotificationChannel( | |
| NotificationChannel( | |
| DEFAULT_CHANNEL_ID, | |
| DEFAULT_CHANNEL_NAME, | |
| NotificationManager.IMPORTANCE_DEFAULT | |
| ), | |
| ) |
p1: 채널 생성은 한 번만 하면 되는데 메시지 올 때마다 매번 호출하고 잇네요 위에거랑 같이 이렇게 한번만 생성해주자융
There was a problem hiding this comment.
minSdk = 28으로 바꿨는디 if (Build.VERSION.SDK_INT < Build.VERSION_CODES.O) return 이거 지워도 되나요?
Unnecessary; SDK_INT is never < 28 이런 경고가 떠가지고
…into feat/#8-fcm_setting
doyeon0307
left a comment
doyeon0307
left a comment
There was a problem hiding this comment.
너무 어려웠겠어...진짜 고생많았다ㅠㅠㅠ 지금 파이어베이스 연동하고 NotificationTokenProvider, PushNotificationService 이 두개 만든 걸로도 충분하니까 너무 많은 걸 한 번에 하려고 안해두 돼!!
서버 API 연동-DataSource, Repository 구현 등은 진짜루 나중에 해도 되니까 부담갖지 말고 차근차근 해보자 예지최고 예지짱짱
| val title = message.notification?.title ?: return | ||
| val body = message.notification?.body ?: return |
There was a problem hiding this comment.
p1 : 어헣 이거 코드래빗 리뷰 뭔소린가하고 알아봤는데 너무나 처음 안 사실.. 이건 서버 분들도 다같이 아셔야할 거 같아서 슬랙에도 올렸어요!!
지금 무엇이 문제냐 하면 씽은 알림 클릭 시 해당되는 특정 화면이 열려야 해요. 이때 필요한게 딥링크라는 개념이 있습니다. 딥링크는 클릭 시 앱의 특정 화면이 열리게 하는 url 개념이에요.
- FCM 메시지 수신으로
onMessageReceived()콜백 발동 - 메시지 값(딥링크 포함) 읽어
showNotification()에 전달 + 호출 showNotification()에서 액티비티 열 때 딥링크 전달 -> 특정 화면 열리게 함!
따라서 onMessageReceived() 콜백이 반드시 발동되어야 합니다.
🚨 그런데 앱이 백그라운드 상태일 때 onMessageReceived 콜백이 발동하지 않을 수 있음! 🚨
FCM 데이터는 json 형태인데,
백그라운드 상태일 때 아래와 같이 notification 키가 포함된 경우에 onMessageReceived()가 호출되지 않는다고 해요,,
{
"notification" : {
"title" : "안녕하세요",
"body" : "반갑습니다"
}
"data" : {
"name" : "유저 이름",
"age" : "나이"
}
}그래서 서버 구현시에 notification 필드를 제거하고, data 필드에 모든 정보를 담는 구조가 안전하다고 합니다.
{
"data" : {
"title" : "안녕하세요",
"body" : "반갑습니다",
"name" : "유저 이름",
"age" : "나이"
}
}그럼 어떻게 고쳐야 하느냐??
지금 제가 서버 쪽에 data 필드만 있는 구조로 요청드렸으므로 (해달라는데로 해주겠지.....)
notification을 읽지 않고 바로 data에서 title과 body를 읽어야 합니다.
| val title = message.notification?.title ?: return | |
| val body = message.notification?.body ?: return | |
| val title = message.data?.["title"] ?: return | |
| val body = message.data?.["body"] ?: return |
| val pendingIntent = launchIntent?.let { | ||
| PendingIntent.getActivity( | ||
| this, | ||
| 0, |
There was a problem hiding this comment.
p1 : 이것도 좀 어려운 개념인데ㅠㅠ,, 딥링크로 상세 페이지 이돟 시에 오류가 발생할 수 있어요.
먼저 PendingIntent를 알아야 하는데, 우리 액티비티 화면 이동시킬 때 Intent 객체 만들었던 것 기억하나요??
푸시 알림에서 PendingIntent는 보류 중인 인텐트로, 화면 이동 정보를 들고 있다가 유저가 푸시를 클릭하면 해당되는 화면으로 이동하게 합니다.
- 푸시알림A 도착
requestCode = 0인PendingIntent토큰 생성
- (상태바에 푸시알림A 남아있는 상태)
- 푸시알림B 도착
reqeust가 동일하므로 새로운 토큰을 만들지 않고 푸시알림A가 만든 토큰을 덮어 씌움
유저는 푸시알림A를 클릭했는데 PendingIntent의 화면 이동 정보만 푸시알림B로 덮어씌워져 화면B가 열릴 수 있다해요!
그래서 푸시알림 각각이 고유한 PendingIntent를 생성할 수 있게
requestCode를 고유값으로 넣어주어야 한다고 합니다.
현재 시각과 같은 값 사용해주면 될 것 같아용
| val pendingIntent = launchIntent?.let { | |
| PendingIntent.getActivity( | |
| this, | |
| 0, | |
| val pendingIntent = launchIntent?.let { | |
| PendingIntent.getActivity( | |
| this, | |
| System.currentTimeMillis().toInt(), |
There was a problem hiding this comment.
조금 더 코드 개선을 해보자면!! 밑에서 이미 id값을 만들기 위해 System.currentTimeMillis().toInt()을 사용하고 있으니 이 값을 변수로 분리해 같이 활용해줄 수도 있어요!
val id = System.currentTimeMillis().toInt()
val pendingIntent = launchIntent?.let {
PendingIntent.getActivity(
this,
id,
it,
PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE,
)
}
val notification = NotificationCompat.Builder(this, DEFAULT_CHANNEL_ID)
.setSmallIcon(android.R.drawable.ic_dialog_info)
.setContentTitle(title)
.setContentText(body)
.apply { pendingIntent?.let { setContentIntent(it) } }
.setAutoCancel(true)
.build()
notificationManager.notify(id, notification)| override fun onNewToken(token: String) { | ||
| super.onNewToken(token) | ||
|
|
||
| if (!authManager.isLoggedIn.value) return |
There was a problem hiding this comment.
저는 AuthManager 지우는 건 찬성! 그치만 NotificationTokenProvider는 NotificationRepository 하나만 주입받으면 좋겠어용
이유는 NotificationTokenProvider의 책임이 새 FCM 토큰이 오면 서버에 전송한다에 그쳐야 한다고 생각하기 때문입니당.
NotificationTokenProvider가 로그인 상태인지 확인하고, 로그인 상태가 맞다면 서버에 전송한다는 과책임 같아요. 그래서 직접 로컬데이터에 접근하지 않으면 좋겠습니다.
저는 NotificationTokenProvider은 notificationRepository.saveNotificationToken(token)만 처리하고
추후 notificationRepositoryImpl.saveNotificationToken(token) 구현 시에 TokenAccessManager.getAccessToken() != null로 토큰 확인하는 방향을 추천드립니당
class NotificationRepositoryImpl @Inject constructor(
private val dataSource: NotificationDataSource,
private val tokenAccessManager: TokenAccessManager,
) : NotificationRepository {
override suspend fun saveNotificationToken(token: String): Result<Unit> {
val accessToken = tokenAccessManager.getAccessToken()
if (accessToken == null) {
Timber.d("로그인 상태가 아니라 FCM 토큰을 서버에 전송하지 않음")
return Result.success(Unit)
}
// API 호출해 서버에 토큰 전송
} There was a problem hiding this comment.
p1 : core 모듈은 data 모듈을 참조하면 안 돼요! 의존성 흐름이 깨집니당.
data에 작성된 모든 코드(NotificationModule, NotificationDataSource, NotificationRequestDto, NotificationApi, NotificationDataSourceImpl, NotificationRepositoryImpl) 전부 다 core:notification 내부로 옮겨주세요!
|
|
||
| @Serializable | ||
| @OptIn(InternalSerializationApi::class) | ||
| data class NotificationRequestDto( |
There was a problem hiding this comment.
p2 : 유빈언니가 지금 연동해둔 코드 보면 Dto 없이 네이밍하고 있어 따르면 조을거같아요
| data class NotificationRequestDto( | |
| data class NotificationRequest( |
| @POST("notifications/token") | ||
| suspend fun postNotificationToken( | ||
| @Body request: NotificationRequestDto, | ||
| ): BaseResponse<Unit> |
There was a problem hiding this comment.
p1 : 서버 명세를 보면 성공 시 응답 body를 내려주지 않습니다.이기 때문에 BaseResponse없이 Unit만 써주면 돼용
| @DELETE("notifications/token") | ||
| suspend fun deleteNotificationToken( | ||
| @Query("token") token: String, | ||
| ): BaseResponse<Unit> |
There was a problem hiding this comment.
p1 : 여기두 Unit! 명세보면 반환값이 없어용
| ): BaseResponse<Unit> | |
| ): Unit |
| override suspend fun deleteNotificationToken(token: String): BaseResponse<Unit> = | ||
| api.deleteNotificationToken(token) | ||
| } |
There was a problem hiding this comment.
p1 : 이거 서버 명세상 성공 시에 바디 자체를 안줘서 safeApiCall 말고 꼭 safeUnitApiCall 사용해주세요!!
| import retrofit2.http.POST | ||
| import retrofit2.http.Query | ||
|
|
||
| interface NotificationApi { |
There was a problem hiding this comment.
p1 : 이름 서비스 써주자용
| interface NotificationApi { | |
| interface NotificationService { |
| class NotificationDataSourceImpl @Inject constructor( | ||
| private val api: NotificationApi, | ||
| ) : NotificationDataSource { | ||
|
|
||
| override suspend fun postNotificationToken(request: NotificationRequestDto): BaseResponse<Unit> = | ||
| api.postNotificationToken(request) |
There was a problem hiding this comment.
p1 : 헛 지금 DataSource와 Repository에 혼동이 있네요..!!
DataSource는 Service를 호출하고,
Repository가 DataSource를 호출하고 safeApiCall을 적용해 예외처리를 합니다.
이 구조 헷갈리면 당장 안해도 되니까 그냥 data.notification 파일들 core:notifaction으로 옮기는 것만 하까..?? FCM만 공부해도 너무 머리아플 것 같아ㅠㅠ,,, 원래 FCM 연동이 서버 API까지 안가고 딱 FCM 세팅까지만 하려고 했던 거라!! 진짜루 더 안해도 괜찮습니당
PushNotificationService 코리만 반영해도 충분해!!
3 participants
Related issue 🛠
Work Description ✏️
Screenshot 📸
Uncompleted Tasks 😅
To Reviewers 📢
사실 다 이해하고 하고 싶었는데 그러면 1년 걸릴 것 같아서 그냥 도연언니 레포 없으면 작업 못하는 사람이 되...
이상한 부분 많을테니까 많은 피드백 받고싶습니다 ㅎㅎ.
유빈언니덕분에빌드오류고침 너무나도든든한리더...
Summary by CodeRabbit