[Fix] FCM과 로컬 알림의 Launch Path 분리 #373
Conversation
Summary of ChangesHello @PeraSite, 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! 이 PR은 FCM(원격) 알림과 로컬 알림을 통해 앱이 실행될 때 사용되는 Highlights
Using Gemini Code AssistThe 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 by creating a comment using either
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 Limitations & Feedback Gemini Code Assist 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. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. 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
|
![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
이번 PR은 FCM과 로컬 알림의 실행 경로(launch path)를 분리하여 로깅하도록 수정한 점이 좋습니다. 코드 변경 사항은 명확하며 의도한 대로 동작할 것으로 보입니다. 다만, 코드의 유지보수성을 높이기 위해 몇 가지 개선점을 제안합니다. 여러 파일에 걸쳐 사용되는 문자열들을 상수로 관리하고, LaunchPath enum을 활용하여 타입 안정성을 높이는 방향으로 수정하면 더 좋을 것 같습니다. 자세한 내용은 각 파일의 주석을 참고해주세요.
|
|
||
| val intent = Intent(this, IntroActivity::class.java).apply { | ||
| putExtra("launch_path", "notification") | ||
| putExtra("launch_path", "remote_notification") |
There was a problem hiding this comment.
하드코딩된 문자열 "remote_notification" 대신 LaunchPath enum의 value를 사용하는 것이 좋습니다. 이렇게 하면 타입 안정성을 높이고, LaunchPath enum 정의와 실제 사용되는 값 사이의 일관성을 보장할 수 있습니다. 또한, "launch_path" 키도 상수로 정의하여 여러 곳에서 재사용하는 것을 고려해보세요.
| putExtra("launch_path", "remote_notification") | |
| putExtra("launch_path", com.eatssu.common.enums.LaunchPath.REMOTE_NOTIFICATION.value) |
| context, | ||
| 0, | ||
| intent.putExtra("launch_path", "notification"), | ||
| intent.putExtra("launch_path", "local_notification"), |
There was a problem hiding this comment.
여기에서도 하드코딩된 문자열 "local_notification" 대신 LaunchPath enum의 value를 사용하는 것이 좋습니다. 이렇게 하면 EatSsuFirebaseMessagingService.kt에서와 마찬가지로 코드의 일관성과 유지보수성을 향상시킬 수 있습니다. "launch_path" 키 또한 상수로 관리하는 것을 추천합니다.
| intent.putExtra("launch_path", "local_notification"), | |
| intent.putExtra("launch_path", com.eatssu.common.enums.LaunchPath.LOCAL_NOTIFICATION.value), |
| "local_notification" -> EventLogger.appLaunch(LaunchPath.LOCAL_NOTIFICATION) | ||
| "remote_notification" -> EventLogger.appLaunch(LaunchPath.REMOTE_NOTIFICATION) |
There was a problem hiding this comment.
when 문에서 하드코딩된 문자열을 사용하는 대신 LaunchPath enum의 value를 직접 사용하는 것이 더 안전하고 유지보수하기 좋습니다. 이렇게 하면 enum 값이 변경될 때 코드 변경이 용이하고, 코드의 의도가 더 명확해집니다. "widget"의 경우에도 동일하게 적용하는 것을 고려해보세요.
| "local_notification" -> EventLogger.appLaunch(LaunchPath.LOCAL_NOTIFICATION) | |
| "remote_notification" -> EventLogger.appLaunch(LaunchPath.REMOTE_NOTIFICATION) | |
| LaunchPath.LOCAL_NOTIFICATION.value -> EventLogger.appLaunch(LaunchPath.LOCAL_NOTIFICATION) | |
| LaunchPath.REMOTE_NOTIFICATION.value -> EventLogger.appLaunch(LaunchPath.REMOTE_NOTIFICATION) |
HI-JIN2
left a comment
HI-JIN2
left a comment
There was a problem hiding this comment.
좋습니다~ 현재 자잘한 수정들 3.1.1로 올리면 좋을 것 같아요
근데, 아요 쪽에서 notification이 로컬인지 fcm인지 분간이 어렵다고 하지 않었나요???
확인했습니다! 기술적으론 가능한데 당장 구현하기엔 시간이 없어서 그랬던 것으로 기억해요! |
Summary
시간 상의 이유로 진행하지 못했던 launch_path 구분 추가
local_notification과 remote_notification으로 분리했습니다.
To reviewers