-
Notifications
You must be signed in to change notification settings - Fork 0
[Feat/#238] FireBase RemoteConfig를 통한 앱 버전 관리를 구현합니다. #247
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
Changes from all commits
ce552ac
98d94bf
c8bf488
d6fd2e2
9f0d1c4
85e5a95
4f90c14
7d61aa6
825f73f
6e1c71c
d775bac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,33 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| package com.sopt.clody.data.remote.appupdate | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.sopt.clody.data.remote.datasource.RemoteConfigDataSource | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.sopt.clody.domain.appupdate.AppUpdateChecker | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.sopt.clody.domain.model.AppUpdateState | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import com.sopt.clody.domain.util.VersionComparator | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| import javax.inject.Inject | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class AppUpdateCheckerImpl @Inject constructor( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| private val remoteConfigDataSource: RemoteConfigDataSource, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) : AppUpdateChecker { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| override suspend fun getAppUpdateState(currentVersion: String): AppUpdateState { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| remoteConfigDataSource.fetch() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val latestVersion = remoteConfigDataSource.getLatestVersion() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| val minimumVersion = remoteConfigDataSource.getMinimumVersion() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| return when { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VersionComparator.compare(currentVersion, minimumVersion) < 0 -> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AppUpdateState.HardUpdate(latestVersion) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| VersionComparator.compare(currentVersion, latestVersion) < 0 -> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AppUpdateState.SoftUpdate(latestVersion) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| else -> { | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| AppUpdateState.Latest | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Comment on lines
+13
to
+32
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Add error handling for Remote Config fetching The implementation correctly determines update states based on version comparison, but lacks error handling for potential failures when fetching remote config. Since fetching from Firebase might fail due to network issues, you should add try-catch blocks to handle exceptions and implement a fallback strategy. override suspend fun getAppUpdateState(currentVersion: String): AppUpdateState {
- remoteConfigDataSource.fetch()
+ try {
+ remoteConfigDataSource.fetch()
+ } catch (e: Exception) {
+ // If fetch fails, continue with potentially cached values
+ // Log the error for monitoring
+ }
val latestVersion = remoteConfigDataSource.getLatestVersion()
val minimumVersion = remoteConfigDataSource.getMinimumVersion()
return when {
VersionComparator.compare(currentVersion, minimumVersion) < 0 -> {
AppUpdateState.HardUpdate(latestVersion)
}
VersionComparator.compare(currentVersion, latestVersion) < 0 -> {
AppUpdateState.SoftUpdate(latestVersion)
}
else -> {
AppUpdateState.Latest
}
}
}📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| package com.sopt.clody.data.remote.datasource | ||
|
|
||
| import com.google.firebase.remoteconfig.FirebaseRemoteConfig | ||
| import com.sopt.clody.BuildConfig | ||
| import kotlinx.coroutines.tasks.await | ||
| import javax.inject.Inject | ||
| import javax.inject.Singleton | ||
|
|
||
| @Singleton | ||
| class RemoteConfigDataSource @Inject constructor( | ||
| private val remoteConfig: FirebaseRemoteConfig, | ||
| ) { | ||
| suspend fun fetch() { | ||
| remoteConfig.fetchAndActivate().await() | ||
| } | ||
coderabbitai[bot] marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| fun getLatestVersion(): String = | ||
| remoteConfig.getString(KEY_LATEST_VERSION).ifEmpty { BuildConfig.VERSION_NAME } | ||
|
|
||
| fun getMinimumVersion(): String = | ||
| remoteConfig.getString(KEY_MINIMUM_VERSION).ifEmpty { BuildConfig.VERSION_NAME } | ||
|
|
||
| companion object { | ||
| private const val KEY_LATEST_VERSION = "latest_version" | ||
| private const val KEY_MINIMUM_VERSION = "min_required_version" | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,27 @@ | ||||||||||||||||||||||||||||
| package com.sopt.clody.di | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| import com.google.firebase.remoteconfig.FirebaseRemoteConfig | ||||||||||||||||||||||||||||
| import com.sopt.clody.data.remote.appupdate.AppUpdateCheckerImpl | ||||||||||||||||||||||||||||
| import com.sopt.clody.data.remote.datasource.RemoteConfigDataSource | ||||||||||||||||||||||||||||
| import com.sopt.clody.domain.appupdate.AppUpdateChecker | ||||||||||||||||||||||||||||
| import dagger.Module | ||||||||||||||||||||||||||||
| import dagger.Provides | ||||||||||||||||||||||||||||
| import dagger.hilt.InstallIn | ||||||||||||||||||||||||||||
| import dagger.hilt.components.SingletonComponent | ||||||||||||||||||||||||||||
| import javax.inject.Singleton | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Module | ||||||||||||||||||||||||||||
| @InstallIn(SingletonComponent::class) | ||||||||||||||||||||||||||||
| object AppUpdateModule { | ||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Provides | ||||||||||||||||||||||||||||
| @Singleton | ||||||||||||||||||||||||||||
| fun provideFirebaseRemoteConfig(): FirebaseRemoteConfig = | ||||||||||||||||||||||||||||
| FirebaseRemoteConfig.getInstance() | ||||||||||||||||||||||||||||
|
Comment on lines
+17
to
+20
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Firebase RemoteConfig provider should include configuration settings While the provider correctly returns the Firebase RemoteConfig instance, it's missing the configuration for fetch settings like minimum fetch interval and fetch timeout. According to the PR description, the fetch interval should be 12 hours, but this isn't set in the code. @Provides
@Singleton
fun provideFirebaseRemoteConfig(): FirebaseRemoteConfig =
- FirebaseRemoteConfig.getInstance()
+ FirebaseRemoteConfig.getInstance().apply {
+ val configSettings = FirebaseRemoteConfigSettings.Builder()
+ .setMinimumFetchIntervalInSeconds(43200) // 12 hours in seconds
+ .build()
+ setConfigSettingsAsync(configSettings)
+ }📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||
| @Provides | ||||||||||||||||||||||||||||
| @Singleton | ||||||||||||||||||||||||||||
| fun provideAppUpdateChecker( | ||||||||||||||||||||||||||||
| remoteConfigDataSource: RemoteConfigDataSource, | ||||||||||||||||||||||||||||
| ): AppUpdateChecker = AppUpdateCheckerImpl(remoteConfigDataSource) | ||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package com.sopt.clody.domain.appupdate | ||
|
|
||
| import com.sopt.clody.domain.model.AppUpdateState | ||
|
|
||
| interface AppUpdateChecker { | ||
| suspend fun getAppUpdateState(currentVersion: String): AppUpdateState | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,7 @@ | ||
| package com.sopt.clody.domain.model | ||
|
|
||
| sealed interface AppUpdateState { | ||
| data object Latest : AppUpdateState | ||
| data class SoftUpdate(val latestVersion: String) : AppUpdateState | ||
| data class HardUpdate(val latestVersion: String) : AppUpdateState | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| package com.sopt.clody.domain.util | ||
|
|
||
| object VersionComparator { | ||
| fun compare(current: String, latest: String): Int { | ||
| val currentParts = current.split(".").map { it.toIntOrNull() ?: 0 } | ||
| val latestParts = latest.split(".").map { it.toIntOrNull() ?: 0 } | ||
|
|
||
| for (i in 0 until maxOf(currentParts.size, latestParts.size)) { | ||
| val c = currentParts.getOrNull(i) ?: 0 | ||
| val l = latestParts.getOrNull(i) ?: 0 | ||
| if (c < l) return -1 | ||
| if (c > l) return 1 | ||
| } | ||
| return 0 | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,27 +1,48 @@ | ||
| package com.sopt.clody.presentation.ui.splash | ||
|
|
||
| import androidx.lifecycle.ViewModel | ||
| import androidx.lifecycle.viewModelScope | ||
| import com.sopt.clody.BuildConfig | ||
| import com.sopt.clody.domain.appupdate.AppUpdateChecker | ||
| import com.sopt.clody.domain.model.AppUpdateState | ||
| import com.sopt.clody.domain.repository.TokenRepository | ||
| import dagger.hilt.android.lifecycle.HiltViewModel | ||
| import kotlinx.coroutines.flow.MutableStateFlow | ||
| import kotlinx.coroutines.flow.StateFlow | ||
| import kotlinx.coroutines.launch | ||
| import javax.inject.Inject | ||
|
|
||
| @HiltViewModel | ||
| class SplashViewModel @Inject constructor( | ||
| private val tokenRepository: TokenRepository, | ||
| private val appUpdateChecker: AppUpdateChecker, | ||
| ) : ViewModel() { | ||
|
|
||
| private val _isUserLoggedIn = MutableStateFlow<Boolean?>(null) | ||
| val isUserLoggedIn: StateFlow<Boolean?> = _isUserLoggedIn | ||
|
|
||
| private val _updateState = MutableStateFlow<AppUpdateState?>(null) | ||
| val updateState: StateFlow<AppUpdateState?> = _updateState | ||
|
|
||
| init { | ||
| attemptAutoLogin() | ||
| checkVersion() | ||
| } | ||
|
|
||
| private fun attemptAutoLogin() { | ||
| val accessToken = tokenRepository.getAccessToken() | ||
| val refreshToken = tokenRepository.getRefreshToken() | ||
| _isUserLoggedIn.value = accessToken.isNotBlank() && refreshToken.isNotBlank() | ||
| } | ||
|
|
||
| private fun checkVersion() { | ||
| viewModelScope.launch { | ||
| val state = appUpdateChecker.getAppUpdateState(BuildConfig.VERSION_NAME) | ||
| _updateState.value = state | ||
| } | ||
| } | ||
|
|
||
| fun clearUpdateState() { | ||
| _updateState.value = AppUpdateState.Latest | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| package com.sopt.clody.presentation.utils.appupdate | ||
|
|
||
| import android.app.Activity | ||
| import android.content.Context | ||
| import android.content.Intent | ||
| import android.net.Uri | ||
|
|
||
| object AppUpdateUtils { | ||
|
|
||
| /** | ||
| * 마켓 이동 | ||
| * @param context Context | ||
| */ | ||
|
Comment on lines
+10
to
+13
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [3] 요즘 코드 작성 간에 함수 위에 이렇게 기능과 param에 대한 comment를 남겨놓으시더라구요! 이때까지 솝트에서 프로젝트를 하다보면 리뷰 간에 주석, 로그 제거에 좀 집착(?)하던 경향이 있었는데 어떻게 생각하시나요? 지금정도의 주석은 필요한 주석이다?! 사실 리뷰를 하는 입장에서 제 개인적으로는 좋다고 생각하기는한데 주변에 현직자들의 의견이나 문수님 의견 들은 바가 있다면 공유받고 싶습니다😀
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 좋은 질문입니다! 주석 관련 생각?사실 이 부분은 회사마다 다소 지향하는 바가 다를 수 있는데, 공식적인 KDoc 형식의 주석은 실무에서도 많이 사용하고 권장하는 경우가 많아요. 특히 SDK, Library, Utility Class처럼 다른 모듈이나 팀에서 재사용될 가능성이 있는 클래스나 함수에는 기능과 파라미터 설명을 남기는 게 좋습니다. BUT, 너무 obvious한 주석( 저도 개인적인 생각으로는 이런 정도의 KDoc 주석은 남기는 게 좋다고 생각하고, 특히 유틸리티성 함수들은 이런 주석이 없으면 오히려 의도가 잘 안 보일 때가 있어서 코드 리뷰할 때도 도움 되는 것 같습니다. ps: 카카오뱅크에 재직중인 익명의 시니어 분에게 여쭈어 봤는데 이런 형태의 주석을 많이 사용한다고 해요~ 그리고 왜 로그를 정리하라고 할까요?코드리뷰를 받을 때 항상 "불필요한 로그 지워주세요~" 라는 걸 많이 받잖아요? 크게 2가지 이유가 있어요. 첫 번째 - 성능 문제로그 출력은 단순한 텍스트 출력이 아니라 내부적으로 I/O가 발생해요. 두 번째 - 불필요한 메모리 할당 및 GC(Garbage Collection) 유발로그를 위해 생성된 문자열, 포맷팅된 파라미터 등이 내부적으로 객체 할당을 발생시켜요ㅠ ※ 물론 Timber와 같은 로깅 라이브러리를 활용하면 |
||
| fun navigateToMarket(context: Context) { | ||
| val packageName = context.packageName | ||
| val marketIntent = Intent(Intent.ACTION_VIEW, Uri.parse("market://details?id=$packageName")).apply { | ||
| addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) | ||
| } | ||
|
|
||
| if (marketIntent.resolveActivity(context.packageManager) != null) { | ||
| context.startActivity(marketIntent) | ||
| } else { | ||
| val webIntent = Intent(Intent.ACTION_VIEW, Uri.parse("https://play.google.com/store/apps/details?id=$packageName")).apply { | ||
| addFlags(Intent.FLAG_ACTIVITY_NEW_TASK) | ||
| } | ||
| context.startActivity(webIntent) | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * 마켓 이동 후 앱 종료 (Hard Update 용) | ||
| * @param activity Activity | ||
| */ | ||
| fun navigateToMarketAndFinish(activity: Activity) { | ||
| navigateToMarket(activity) | ||
| activity.finishAffinity() | ||
| } | ||
| } | ||
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.
💡 Verification agent
🧩 Analysis chain
Consider optimizing Remote Config fetch frequency
Currently, the implementation calls
fetch()every timegetAppUpdateState()is called, which might be inefficient if called frequently.Since Firebase RemoteConfig has a default caching mechanism, consider implementing a more strategic approach to fetching, such as only fetching if the cached data is stale.
Do you have a RemoteConfigDataSource implementation that manages caching efficiently? If not, consider adding a check for the last fetch time before making a new fetch request.
🏁 Script executed:
Length of output: 1227
Optimize Remote Config Fetching Frequency
The current implementation in
AppUpdateCheckerImpl.ktcallsremoteConfigDataSource.fetch()on every invocation ofgetAppUpdateState(). Our investigation ofRemoteConfigDataSource.ktconfirms that it simply callsfetchAndActivate()without any additional caching logic or a check on the last fetch time. While Firebase RemoteConfig has a built-in caching mechanism, relying solely on it might not be sufficient if this method is hit frequently.Suggestions: