-
Notifications
You must be signed in to change notification settings - Fork 0
feat - 푸시알림 등록 #54
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat - 푸시알림 등록 #54
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR implements Firebase Cloud Messaging (FCM) push notification support for the Android application, enabling the app to receive and display push notifications. The implementation includes FCM token management, notification channel setup for Android 8.0+, runtime permission handling for Android 13+, and integration with the sign-in flow to register device tokens with the backend.
Key Changes:
- Added Firebase Messaging SDK dependencies and service configuration
- Implemented FCM token storage and retrieval using DataStoreManager
- Modified sign-in flow to include FCM token in authentication requests
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| app/build.gradle.kts | Added Firebase BOM and messaging dependencies with Google services plugin |
| app/src/main/AndroidManifest.xml | Added POST_NOTIFICATIONS permission and registered FCM service with metadata |
| app/src/main/res/values/strings.xml | Added string resources for default notification channel configuration |
| app/src/main/java/com/apptive/japkor/push/NotificationChannels.kt | Created utility function to ensure notification channel exists for Android O+ |
| app/src/main/java/com/apptive/japkor/push/JapKorFirebaseMessagingService.kt | Implemented FCM service to handle token refresh and incoming messages |
| app/src/main/java/com/apptive/japkor/data/local/DataStoreManager.kt | Added FCM token storage and retrieval methods |
| app/src/main/java/com/apptive/japkor/data/model/Auth.kt | Extended SignInDTO to include fcmToken parameter |
| app/src/main/java/com/apptive/japkor/data/repository/AuthRepository.kt | Updated signIn method to pass FCM token to API |
| app/src/main/java/com/apptive/japkor/ui/login/LoginScreenViewModel.kt | Modified login flow to retrieve and send FCM token |
| app/src/main/java/com/apptive/japkor/MainActivity.kt | Added notification permission request and FCM token initialization |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| NotificationManagerCompat.from(this) | ||
| .notify(System.currentTimeMillis().toInt(), notification) |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing runtime permission check before displaying notifications. On Android 13 (API 33) and above, the POST_NOTIFICATIONS permission can be denied by users even after being requested. You should check if the permission is granted before calling notify(), otherwise a SecurityException could be thrown when the permission is denied.
|
|
||
| try { | ||
| val result = repository.signIn(email, password) | ||
| val fcmToken = dataStore.getFcmToken().first() |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential null pointer exception when fcmToken is empty. If getFcmToken() returns an empty string (which happens when the token hasn't been saved yet), this empty string will be sent to the API. The login could fail during app's first launch before the FCM token is obtained. Consider handling this case by either waiting for the token to be available, deferring the login, or making the token optional in the sign-in flow.
| val fcmToken = dataStore.getFcmToken().first() | |
| val storedFcmToken = dataStore.getFcmToken().first() | |
| val fcmToken = storedFcmToken.takeIf { it.isNotBlank() } | |
| if (fcmToken == null) { | |
| Log.w("LoginVM", "FCM token is not yet available; proceeding with sign-in without it.") | |
| } |
| FirebaseMessaging.getInstance().token | ||
| .addOnCompleteListener { task -> | ||
| if (!task.isSuccessful) { | ||
| Log.w(TAG, "Fetching FCM registration token failed", task.exception) | ||
| return@addOnCompleteListener | ||
| } | ||
|
|
||
| val token = task.result | ||
| Log.d(TAG, "FCM token: $token") | ||
| lifecycleScope.launch { | ||
| dataStoreManager.saveFcmToken(token) | ||
| } | ||
| } |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Race condition between FCM token fetch and login. The FCM token is fetched asynchronously in MainActivity.onCreate() (lines 54-66) and saved via lifecycleScope.launch. However, if a user navigates to the login screen and attempts to sign in before this async operation completes, the fcmToken retrieved at line 45 could be empty. This creates a timing-dependent bug where login behavior differs based on how quickly the user navigates.
| val email: String, | ||
| val password: String | ||
| val password: String, | ||
| val fcmToken: String, |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing trailing comma in data class parameter list. For consistency with Kotlin coding conventions and to reduce diff noise in future changes, add a trailing comma after the fcmToken parameter.
| val email: String, | ||
| val password: String | ||
| val password: String, | ||
| val fcmToken: String, |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
API design concern - mandatory FCM token may break existing clients. Adding fcmToken as a required parameter to SignInDTO makes it mandatory for all sign-in requests. If there are other clients or older app versions still using the API, this would break their login functionality. Consider making the fcmToken parameter optional (nullable) in the DTO to maintain backward compatibility.
| val fcmToken: String, | |
| val fcmToken: String? = null, |
|
|
||
| suspend fun signIn(email:String,password:String) : SignInResponse?{ | ||
| val response = api.signIn(SignInDTO(email,password)).awaitResponse() | ||
| suspend fun signIn(email: String, password: String, fcmToken: String) : SignInResponse?{ |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Inconsistent parameter spacing in function signature. The function parameters have inconsistent spacing around colons. The email parameter has "email: String" with proper spacing, while the original code likely had different spacing. Ensure consistent formatting with spaces after colons for all parameters.
| override fun onNewToken(token: String) { | ||
| Log.d(TAG, "New FCM token: $token") | ||
| CoroutineScope(Dispatchers.IO).launch { | ||
| DataStoreManager(applicationContext).saveFcmToken(token) |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing error handling for FCM token generation. If the FCM token retrieval fails in onNewToken(), the coroutine will crash silently without any fallback mechanism. Consider adding try-catch error handling and logging any failures, so that token save failures don't go unnoticed.
| DataStoreManager(applicationContext).saveFcmToken(token) | |
| try { | |
| DataStoreManager(applicationContext).saveFcmToken(token) | |
| } catch (e: Exception) { | |
| Log.e(TAG, "Failed to save FCM token", e) | |
| } |
| import android.content.Context | ||
| import android.os.Build | ||
| import com.apptive.japkor.R | ||
|
|
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing documentation for public function. The ensureDefaultNotificationChannel function is a public API used across multiple files but lacks documentation explaining its purpose, when it should be called, and why it's safe to call multiple times. Consider adding KDoc to explain that this function is idempotent and handles channel creation for Android O and above.
| /** | |
| * Ensures that the app's default notification channel exists. | |
| * | |
| * This function should be called before posting notifications that rely on the | |
| * default FCM notification channel (for example during app startup or when | |
| * initializing push notification handling). | |
| * | |
| * On Android versions prior to O (API 26), this method is a no-op because | |
| * notification channels are not supported. On Android O and above, it will | |
| * create the default channel if it does not already exist. | |
| * | |
| * The operation is idempotent and safe to call multiple times: if the channel | |
| * already exists, the existing channel is left unchanged. | |
| * | |
| * @param context a [Context] used to access resources and the [NotificationManager]. | |
| */ |
| .build() | ||
|
|
||
| NotificationManagerCompat.from(this) | ||
| .notify(System.currentTimeMillis().toInt(), notification) |
Copilot
AI
Dec 27, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Potential notification ID collision and memory issue. Using System.currentTimeMillis().toInt() as the notification ID can lead to problems: (1) notifications arriving within the same millisecond will have the same ID and overwrite each other, (2) the cast to Int can cause collisions due to truncation. Consider using a more reliable approach such as a sequential counter or hash of the message ID.
* feat: fcm 토큰 발급 * feat: 로그인 필드에 fcmtoken 추가 * feat: FCM 알림 기능 추가 및 기본 채널 설정 * feat: MainScreen에서 알림 권한 요청 기능 추가
* feat: 로그인 api 연동(진짜최종) * feat - 푸시알림 등록 (#54) * feat: fcm 토큰 발급 * feat: 로그인 필드에 fcmtoken 추가 * feat: FCM 알림 기능 추가 및 기본 채널 설정 * feat: MainScreen에서 알림 권한 요청 기능 추가 * feat: 버전 업데이트 (1.0.1) * feat - 회원가입, 필수정보입력 필드 업데이트 (#57) * feat: 필수정보입력 실명 필드 추가 * feat: 회원가입 이름 필드 제거 * feat: 버전 업데이트 (1.0.2) * feat: ADID 추가 * feat: FCM 토큰 가져오기 및 저장 로직 추가 (#59) * feat: 구글로그인 응답로그 추가 * feat: 구글로그인 status 필드 대응 * fix: 구글로그인 버튼 위치 변경 --------- Co-authored-by: BYEONGCHAN LEE <mark77234@naver.com>
작업내용