feat: 푸시 알림 기능 #93
feat: 푸시 알림 기능 #93
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @minjun011026, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces core push notification functionality to the application using Firebase Cloud Messaging. It sets up the necessary infrastructure for receiving and displaying notifications, manages device tokens for user association, and includes temporary testing mechanisms. Additionally, it includes some build configuration adjustments and dependency cleanups.
Highlights
- Firebase Cloud Messaging (FCM) Integration: Integrated Firebase Cloud Messaging by adding necessary Gradle plugins and dependencies, enabling the application to receive push notifications.
- Push Notification Service Implementation: A new
SaegilFirebaseMessagingServicehas been created to handle incoming FCM messages, extract notification data (title, body), and display them to the user. This service also manages the device token lifecycle. - Device Token Management: The FCM device token is now retrieved on application startup and saved to SharedPreferences. This token is also passed to the backend during the Kakao login process to associate the user with their device for targeted notifications.
- Android Manifest and Permissions Updates: Added the
POST_NOTIFICATIONSpermission to the Android Manifest and registered the newSaegilFirebaseMessagingServiceto ensure proper notification handling. - Temporary Notification Test Endpoint: A temporary test endpoint (
/api/v1/notifications/test) has been added and is called when fetching news. This is for testing purposes to trigger notifications and will likely be removed or refined later. - Build Configuration Adjustments: The
applicationIdSuffix = ".debug"inapp/build.gradle.ktshas been commented out to resolve conflicts with Firebase console registration. Also,.google-services.jsonhas been added to.gitignorefiles. - Dependency Cleanup: Removed unused media (ExoPlayer, Media3) and Room database related dependencies from
gradle/libs.versions.toml.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Pull Request Overview
This PR implements Firebase Cloud Messaging (FCM) push notification functionality for the Saegil Android app. The implementation includes a dedicated Firebase messaging service to handle notifications and integrates device token management into the OAuth login flow.
Key changes:
- Added
SaegilFirebaseMessagingServiceto handle FCM notifications and device token management - Modified OAuth login to include device token in authentication requests
- Added test notification endpoint integration in the news service for testing purposes
Reviewed Changes
Copilot reviewed 15 out of 16 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
gradle/libs.versions.toml |
Added Firebase BOM and messaging dependencies, Google Services plugin |
app/build.gradle.kts |
Added Firebase dependencies and Google Services plugin, commented out debug suffix |
app/src/main/AndroidManifest.xml |
Added notification permission and FCM service declaration |
app/src/main/java/com/saegil/android/SaegilFirebaseMessagingService.kt |
New Firebase messaging service for handling notifications |
app/src/main/java/com/saegil/android/App.kt |
Added FCM token initialization on app startup |
data/src/main/java/com/saegil/data/local/TokenDataSource.kt |
Added device token retrieval method |
data/src/main/java/com/saegil/data/local/TokenDataSourceImpl.kt |
Implemented device token storage and retrieval |
data/src/main/java/com/saegil/data/repository/OAuthRepositoryImpl.kt |
Modified login to include device token |
data/src/main/java/com/saegil/data/remote/OAuthService.kt |
Updated login method signature to include device token |
data/src/main/java/com/saegil/data/remote/OAuthServiceImpl.kt |
Added device token to login request payload |
data/src/main/java/com/saegil/data/remote/NewsServiceImpl.kt |
Added test notification endpoint call |
data/src/main/java/com/saegil/data/remote/HttpRoutes.kt |
Added test notification endpoint |
data/src/main/java/com/saegil/data/di/NetworkModule.kt |
Added test endpoint to auth-required routes |
| val testResponse = client.post(TEST) { | ||
| parameter("title", "테스트 알림") | ||
| parameter("body", "내용입니다.") | ||
| } | ||
| Log.d("경로",testResponse.toString()) | ||
| Log.d("경로",testResponse.body()) |
There was a problem hiding this comment.
Test notification code is mixed with production news retrieval logic. This should be removed or moved to a separate testing mechanism to keep production code clean.
| val testResponse = client.post(TEST) { | |
| parameter("title", "테스트 알림") | |
| parameter("body", "내용입니다.") | |
| } | |
| Log.d("경로",testResponse.toString()) | |
| Log.d("경로",testResponse.body()) |
| parameter("title", "테스트 알림") | ||
| parameter("body", "내용입니다.") | ||
| } | ||
| Log.d("경로",testResponse.toString()) |
There was a problem hiding this comment.
Debug logging with Korean text should be removed from production code. Use proper logging framework like Timber if logging is needed.
| parameter("body", "내용입니다.") | ||
| } | ||
| Log.d("경로",testResponse.toString()) | ||
| Log.d("경로",testResponse.body()) |
There was a problem hiding this comment.
Debug logging with Korean text should be removed from production code. Use proper logging framework like Timber if logging is needed.
| import dagger.hilt.android.AndroidEntryPoint | ||
| import androidx.core.content.edit | ||
|
|
||
| @AndroidEntryPoint // Hilt 쓰는 경우 |
There was a problem hiding this comment.
Comment contains Korean text. Use English for code comments to maintain consistency and accessibility for international developers.
| @AndroidEntryPoint // Hilt 쓰는 경우 | |
| @AndroidEntryPoint // Used when Hilt is being used for dependency injection |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| val channel = NotificationChannel( | ||
| channelId, | ||
| "기본 알림 채널", |
There was a problem hiding this comment.
Notification channel name is hardcoded in Korean. Consider using string resources for localization and consistency.
| override fun onMessageReceived(remoteMessage: RemoteMessage) { | ||
| super.onMessageReceived(remoteMessage) | ||
|
|
||
| val title = remoteMessage.notification?.title ?: "알림" |
There was a problem hiding this comment.
Default notification title is hardcoded in Korean. Consider using string resources for localization and consistency.
| super.onMessageReceived(remoteMessage) | ||
|
|
||
| val title = remoteMessage.notification?.title ?: "알림" | ||
| val body = remoteMessage.notification?.body ?: "내용 없음" |
There was a problem hiding this comment.
Default notification body is hardcoded in Korean. Consider using string resources for localization and consistency.
| buildTypes { | ||
| debug { | ||
| applicationIdSuffix = ".debug" | ||
| //applicationIdSuffix = ".debug" |
There was a problem hiding this comment.
Debug application ID suffix is commented out. This should be properly configured or removed if no longer needed, as it affects app installation and Firebase configuration.
| //applicationIdSuffix = ".debug" | |
| applicationIdSuffix = ".debug" |
There was a problem hiding this comment.
Code Review
This pull request introduces push notification functionality using Firebase Cloud Messaging. The changes include adding necessary dependencies, permissions, and a FirebaseMessagingService to handle incoming messages. My review identified areas for improvement related to maintainability and correctness. Key points include removing temporary test code, replacing hardcoded strings with constants and resources, improving error handling, and correcting the generation of notification IDs. These changes will make the implementation more robust and easier to maintain.
| .setContentText(message) | ||
| .setAutoCancel(true) | ||
| .build() | ||
| notificationManager.notify(System.currentTimeMillis().toInt(), notification) |
There was a problem hiding this comment.
Using System.currentTimeMillis().toInt() as a notification ID is not reliable because of potential collisions and integer overflow. Use a unique ID for each notification. If you don't need to update specific notifications, a random number is a simple and effective solution.
| notificationManager.notify(System.currentTimeMillis().toInt(), notification) | |
| notificationManager.notify(java.util.Random().nextInt(), notification) |
| val testResponse = client.post(TEST) { | ||
| parameter("title", "테스트 알림") | ||
| parameter("body", "내용입니다.") | ||
| } | ||
| Log.d("경로",testResponse.toString()) | ||
| Log.d("경로",testResponse.body()) |
There was a problem hiding this comment.
This block of code appears to be for testing the push notification functionality. This test code, including the API call and logging, should not be part of the production codebase. Remove it.
Use Timber for logging instead of android.util.Log, as Timber is already integrated into the project and provides more features like automatic tag generation and release build tree stripping.
return client.get(NEWS).body()| buildTypes { | ||
| debug { | ||
| applicationIdSuffix = ".debug" | ||
| //applicationIdSuffix = ".debug" |
There was a problem hiding this comment.
The applicationIdSuffix for debug builds is commented out. While this may be a temporary workaround for Firebase notifications, consider configuring different Firebase projects for debug and release builds using different google-services.json files as a more robust long-term solution. This would allow you to keep the .debug suffix and avoid potential issues with analytics and other Firebase services. Please add a TODO comment to track this technical debt.
| Firebase.messaging.token.addOnCompleteListener { task -> | ||
| if (task.isSuccessful) { | ||
| val deviceToken = task.result | ||
|
|
||
| getSharedPreferences("fcm", MODE_PRIVATE) | ||
| .edit { | ||
| putString("deviceToken", deviceToken) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The task completion listener currently only handles the success case. If task.isSuccessful is false, the failure is silently ignored. It's important to log the exception to help with debugging, especially for something as critical as fetching the FCM token.
Firebase.messaging.token.addOnCompleteListener { task ->
if (task.isSuccessful) {
val deviceToken = task.result
getSharedPreferences("fcm", MODE_PRIVATE)
.edit {
putString("deviceToken", deviceToken)
}
} else {
Timber.w(task.exception, "Fetching FCM registration token failed")
}
}| val title = remoteMessage.notification?.title ?: "알림" | ||
| val body = remoteMessage.notification?.body ?: "내용 없음" |
There was a problem hiding this comment.
The default values for the notification title and body are hardcoded. Use string resources (R.string...) for all user-facing text for maintainability and to support multiple languages in the future.
| val title = remoteMessage.notification?.title ?: "알림" | |
| val body = remoteMessage.notification?.body ?: "내용 없음" | |
| val title = remoteMessage.notification?.title ?: getString(R.string.notification_default_title) | |
| val body = remoteMessage.notification?.body ?: getString(R.string.notification_default_body) |
| } | ||
|
|
||
| private fun showNotification(title: String, message: String) { | ||
| val channelId = "default_channel" |
There was a problem hiding this comment.
The notification channel ID is hardcoded. Define this as a constant in a companion object or a separate constants file to avoid typos and improve maintainability.
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| val channel = NotificationChannel( | ||
| channelId, | ||
| "기본 알림 채널", |
There was a problem hiding this comment.
The channel name is hardcoded. This user-visible string should be a string resource (R.string...) to support localization and for better maintainability.
| "기본 알림 채널", | |
| getString(R.string.notification_channel_default_name), |
| val notification = NotificationCompat.Builder(this, channelId) | ||
| .setSmallIcon(R.drawable.ic_saegil_logo) | ||
| .setContentTitle(title) | ||
| .setContentText(message) | ||
| .setAutoCancel(true) | ||
| .build() |
There was a problem hiding this comment.
The created notification does not have a PendingIntent set via setContentIntent(). This means that when the user taps the notification, nothing will happen other than the notification being dismissed (due to setAutoCancel(true)). Add a PendingIntent that navigates the user to the relevant screen.
| val deviceToken = context | ||
| .getSharedPreferences("fcm", Context.MODE_PRIVATE) | ||
| .getString("deviceToken", null) |
There was a problem hiding this comment.
The SharedPreferences name "fcm" and key "deviceToken" are hardcoded. This pattern is repeated in App.kt and SaegilFirebaseMessagingService.kt. Extract these into constants in a shared location (e.g., a Constants object in the data module) and use them consistently across the app.
|
|
||
| const val NEWS_CATEGORIES = "$BASE_URL/api/v1/news/categories" | ||
|
|
||
| const val TEST = "$BASE_URL/api/v1/notifications/test" |
There was a problem hiding this comment.
The TEST route appears to be for temporary testing purposes. Remove this constant and its usage in NetworkModule and NewsServiceImpl.
⭐️ 변경된 내용
SaegilFirebaseMessagingService라는Service를 생성해서 얘가 알림을 받습니다.com.saegil.android로 등록되어 있어서 디버그용으로 돌리면 적용이 안되길래 일단 주석처리했습니다.📌 이 부분은 꼭 봐주세요! (Optional)
🏞️ 스크린샷 (Optional)