✨[FEAT] #135: Firebase Remote Config 연동#138
Conversation
Summary of ChangesHello @HwangJaemin49, 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은 Firebase Remote Config, Analytics, Crashlytics를 애플리케이션에 통합하여 전반적인 안정성과 유연성을 향상시키는 데 중점을 둡니다. 주요 변경 사항으로는 동적 API 기본 URL 구성을 위한 Remote Config 설정, 새로운 'qa' 빌드 타입 도입, 그리고 Crashlytics와 연동되는 커스텀 로깅 및 사용자 컨텍스트 관리 시스템이 포함됩니다. 또한, 토큰 갱신 메커니즘을 강화하여 다양한 인증 실패 시나리오를 보다 견고하게 처리할 수 있도록 개선되었습니다. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
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 review |
![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은 Firebase Remote Config를 연동하여 서버의 기본 URL을 동적으로 관리하도록 구현한 점이 인상적입니다. 또한, 중앙화된 로깅 시스템(AppLogger) 도입, UserContext를 통한 크래시 리포팅 개선, TokenManager의 스레드 안전성 강화를 위한 리팩토링 등 많은 개선이 이루어졌습니다. 전반적인 변경 사항은 구조가 잘 잡혀 있으며 애플리케이션의 안정성과 관찰 가능성을 크게 향상시킵니다. 의존성 관리 및 잠재적인 성능 문제와 관련하여 몇 가지 개선 제안 사항이 있습니다.
app/build.gradle.kts
Outdated
| // Import the Firebase BoM | ||
| implementation(platform("com.google.firebase:firebase-bom:34.9.0")) | ||
| implementation("com.google.firebase:firebase-analytics") |
There was a problem hiding this comment.
Firebase 의존성이 중복으로 선언되었습니다. libs.firebase.bom을 사용하는 Firebase 의존성들이 이미 136-139 라인에서 버전 카탈로그를 통해 선언되어 있습니다. 여기에 추가된 하드코딩된 의존성들은 불필요하며, 다른 BOM 버전(34.9.0 vs libs.versions.toml의 33.1.0)을 사용하고 있어 예기치 않은 의존성 문제나 빌드 오류를 유발할 수 있습니다. 이 라인들을 제거하는 것이 좋습니다.
There was a problem hiding this comment.
확인했습니다. 수정해주셔서 감사합니다.
| override fun clearCachedTokens() { | ||
| cachedAccessToken = null | ||
| cachedRefreshToken = null | ||
| runBlocking { | ||
| mutex.withLock { | ||
| cachedAccessToken = null | ||
| cachedRefreshToken = null | ||
| } | ||
| } | ||
| } | ||
|
|
||
| override fun setCachedTokens(accessToken: String, refreshToken: String?) { | ||
| cachedAccessToken = accessToken | ||
| cachedRefreshToken = refreshToken | ||
| runBlocking { | ||
| mutex.withLock { | ||
| cachedAccessToken = accessToken | ||
| cachedRefreshToken = refreshToken | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
clearCachedTokens와 setCachedTokens 메서드에서 runBlocking을 사용하면 호출 스레드를 블로킹할 수 있습니다. 만약 이 메서드들이 메인 스레드에서 호출될 경우, UI가 멈추고 ANR(Application Not Responding) 오류로 이어질 수 있습니다. 이 메서드들이 메인 스레드에서 호출되지 않도록 주의가 필요합니다. 장기적으로는 AccessTokenProvider 인터페이스의 해당 함수들을 suspend로 변경하여 runBlocking 없이 비동기적으로 처리하는 것을 고려해보는 것이 좋습니다.
There was a problem hiding this comment.
수정해주셔서 감사합니다. AccessTokenProvider 인터페이스의 clearCachedTokens와 setCachedTokens 함수를 suspend로 변경하신 것은 좋은 방향입니다. 하지만 TokenManager 구현체에서 여전히 runBlocking을 사용하고 있어, 이 함수들이 호출될 때 여전히 호출 스레드를 블로킹할 수 있습니다. suspend 함수 내에서 runBlocking을 사용하는 것은 비동기 처리의 이점을 상쇄하고 잠재적인 UI 블로킹을 유발할 수 있으므로, runBlocking을 제거하고 mutex.withLock을 직접 사용하도록 수정하는 것이 좋습니다.
clearCachedTokens:
| override fun clearCachedTokens() { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| runBlocking { | |
| mutex.withLock { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| } | |
| } | |
| } | |
| override fun setCachedTokens(accessToken: String, refreshToken: String?) { | |
| cachedAccessToken = accessToken | |
| cachedRefreshToken = refreshToken | |
| runBlocking { | |
| mutex.withLock { | |
| cachedAccessToken = accessToken | |
| cachedRefreshToken = refreshToken | |
| } | |
| } | |
| } | |
| override suspend fun clearCachedTokens() { | |
| mutex.withLock { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| } | |
| } |
setCachedTokens:
| override fun clearCachedTokens() { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| runBlocking { | |
| mutex.withLock { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| } | |
| } | |
| } | |
| override fun setCachedTokens(accessToken: String, refreshToken: String?) { | |
| cachedAccessToken = accessToken | |
| cachedRefreshToken = refreshToken | |
| runBlocking { | |
| mutex.withLock { | |
| cachedAccessToken = accessToken | |
| cachedRefreshToken = refreshToken | |
| } | |
| } | |
| } | |
| override suspend fun setCachedTokens(accessToken: String, refreshToken: String?) { | |
| mutex.withLock { | |
| cachedAccessToken = accessToken | |
| cachedRefreshToken = refreshToken | |
| } | |
| } |
| override fun clearCachedTokens() { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| runBlocking { | |
| mutex.withLock { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| } | |
| } | |
| } | |
| override fun setCachedTokens(accessToken: String, refreshToken: String?) { | |
| cachedAccessToken = accessToken | |
| cachedRefreshToken = refreshToken | |
| runBlocking { | |
| mutex.withLock { | |
| cachedAccessToken = accessToken | |
| cachedRefreshToken = refreshToken | |
| } | |
| } | |
| } | |
| override suspend fun clearCachedTokens() { | |
| mutex.withLock { | |
| cachedAccessToken = null | |
| cachedRefreshToken = null | |
| } | |
| } | |
app/build.gradle.kts
Outdated
| applicationIdSuffix = ".qa" | ||
| versionNameSuffix = "-qa" | ||
| matchingFallbacks += listOf("debug") | ||
| buildConfigField("String", "BASE_URL", "\"${localProperties["base_url"] ?: ""}\"") |
There was a problem hiding this comment.
qa 빌드 타입은 initWith(getByName("debug"))를 통해 debug 빌드 타입의 모든 설정을 상속받습니다. 여기에는 BASE_URL에 대한 buildConfigField 설정도 포함됩니다. 따라서 이 라인에서 다시 설정하는 것은 중복입니다. 코드를 더 깔끔하게 유지하기 위해 이 라인을 제거할 수 있습니다.
There was a problem hiding this comment.
수정해주셔서 감사합니다. 해당 변경사항이 적용된 것을 확인했습니다.
|
|
||
| private fun initRemoteConfig() { | ||
|
|
||
| val settings = com.google.firebase.remoteconfig.FirebaseRemoteConfigSettings.Builder() |
There was a problem hiding this comment.
코드 가독성과 일관성을 위해 com.google.firebase.remoteconfig.FirebaseRemoteConfigSettings를 import하고 클래스 이름을 직접 사용하는 것을 권장합니다.
| val settings = com.google.firebase.remoteconfig.FirebaseRemoteConfigSettings.Builder() | |
| val settings = FirebaseRemoteConfigSettings.Builder() |
There was a problem hiding this comment.
수정해주셔서 감사합니다!
3 participants
#⃣ 연관된 이슈
close #135
📝 작업 내용
Remote Config에 default(prod), release, qa 분기로 나누어 서버 호스트 등록