[PC-1662] 서비스 일시 중단 팝업 구현 #208
Conversation
…ce-Android into feature/PC-1662
📝 WalkthroughWalkthroughAdds maintenance-period support across layers: new domain model and network DTO, Remote Config key and init, repository method, ViewModel state and refresh flow, MainActivity/PieceApp wiring, a MaintenanceDialog UI, design-system dialog description support, and new string resources. Changes
Sequence DiagramsequenceDiagram
participant Activity as MainActivity
participant ViewModel as MainViewModel
participant Repo as ConfigureRepository
participant DataSource as ConfigureDataSource
participant Firebase as FirebaseRemoteConfig
participant UI as PieceApp / MaintenanceDialog
Activity->>ViewModel: onResume() -> refreshAppStatus()
ViewModel->>ViewModel: checkMaintenancePeriod()
ViewModel->>Repo: getMaintenancePeriod()
Repo->>DataSource: queryRemoteConfig(MAINTENANCE_PERIOD)
DataSource->>Firebase: fetch/activate
Firebase-->>DataSource: maintenancePeriod string
DataSource-->>Repo: GetMaintenancePeriodInfoResponse
Repo-->>ViewModel: MaintenancePeriod (toDomain())
ViewModel->>Activity/UI: emit maintenancePeriod state
UI->>UI: show MaintenanceDialog if non-blank
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@core/network/src/main/java/com/puzzle/network/source/configure/ConfigureDataSource.kt`:
- Around line 24-35: The init block in ConfigureDataSource.kt currently swallows
Remote Config errors and uses minimumFetchIntervalInSeconds = 0; update the
remoteConfig.addOnConfigUpdateListener implementation so onError(error:
FirebaseRemoteConfigException) logs the exception (use your app
logger/Timber/Log.e) including contextual text, and avoid silently discarding
it; also change minimumFetchIntervalInSeconds from 0 to a production-safe
default (e.g., 3600) and/or make it configurable via build flags (keep 0 only
for debug builds) so fetches aren’t throttled in production; ensure
remoteConfig.activate() remains called only on successful onUpdate.
In `@presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt`:
- Around line 116-118: The current composition allows both MaintenanceDialog and
ForceUpdateDialog to display simultaneously (NetworkScreen,
MaintenanceDialog(maintenancePeriod?.maintenancePeriod),
ForceUpdateDialog(forceUpdate)); to fix this, ensure only the higher-priority
dialog shows: either reorder to compose ForceUpdateDialog before
MaintenanceDialog so it renders below maintenance, or add conditional logic so
when maintenancePeriod is non-null you only call MaintenanceDialog (which
triggers finishAffinity()), else if forceUpdate is non-null call
ForceUpdateDialog; reference the composables MaintenanceDialog,
ForceUpdateDialog and the state props maintenancePeriod and forceUpdate when
implementing the change.
♻️ Duplicate comments (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (2)
156-202: Same duplication applies here.This overload also contains the same duplicated description rendering logic. The suggested
DescriptionBoxextraction would apply here as well.
204-250: Same duplication applies here.This overload also contains the same duplicated description rendering logic.
🧹 Nitpick comments (4)
core/domain/src/main/java/com/puzzle/domain/model/configure/MaintenancePeriod.kt (1)
3-5: Consider renaming the property to avoid redundancy.The property name
maintenancePeriodis redundant with the class name, resulting inMaintenancePeriod.maintenancePeriodat usage sites. A simpler name likeperiodordescriptionwould improve readability.♻️ Suggested improvement
data class MaintenancePeriod ( - val maintenancePeriod: String + val period: String )presentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.kt (1)
44-54: Consider handling the case whereactivityis null.While the safe call
activity?.finishAffinity()prevents a crash, clicking the button whenactivityis null results in a silent no-op. In the context ofPieceApp,LocalContext.currentshould always be anActivity, but adding a fallback (e.g., logging or showing a toast) could help diagnose issues in edge cases.♻️ Optional: Add fallback handling
dialogBottom = { PieceSolidButton( label = stringResource(id = R.string.maintenance_label), onClick = { - activity?.finishAffinity() + activity?.finishAffinity() ?: run { + // Fallback: close via system back if activity is unavailable + (context as? Activity)?.finish() + } }, modifier = Modifier .fillMaxWidth() .padding(bottom = 20.dp, top = 12.dp), ) }core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (2)
85-104: Redundant background modifier on Text.The
background(PieceTheme.colors.light3)modifier on line 101 is redundant since the parentBoxalready applies the same background at line 91. This duplication exists in all 4 overloads.♻️ Proposed fix
description?.let { Box( contentAlignment = Alignment.Center, modifier = Modifier .padding(top = 4.dp) .fillMaxWidth() .background(PieceTheme.colors.light3) .padding(vertical = 12.dp), ) { Text( text = it, color = PieceTheme.colors.dark3, style = PieceTheme.typography.bodySM, textAlign = TextAlign.Center, modifier = Modifier .fillMaxWidth() - .background(PieceTheme.colors.light3) ) } }
108-154: Identical rendering logic duplicated across overloads.The description rendering block (lines 133-152) is identical to the one in the previous overload. Consider extracting this to a private composable to reduce duplication.
♻️ Suggested extraction
`@Composable` private fun DescriptionBox(description: AnnotatedString) { Box( contentAlignment = Alignment.Center, modifier = Modifier .padding(top = 4.dp) .fillMaxWidth() .background(PieceTheme.colors.light3) .padding(vertical = 12.dp), ) { Text( text = description, color = PieceTheme.colors.dark3, style = PieceTheme.typography.bodySM, textAlign = TextAlign.Center, modifier = Modifier.fillMaxWidth() ) } }Then each overload can simply call:
description?.let { DescriptionBox(it) }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
core/data/src/main/java/com/puzzle/data/repository/ConfigureRepositoryImpl.ktcore/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.ktcore/designsystem/src/main/res/values/strings.xmlcore/domain/src/main/java/com/puzzle/domain/model/configure/MaintenancePeriod.ktcore/domain/src/main/java/com/puzzle/domain/repository/ConfigureRepository.ktcore/network/src/main/java/com/puzzle/network/model/configure/GetMaintenancePeriodInfoResponse.ktcore/network/src/main/java/com/puzzle/network/source/configure/ConfigureDataSource.ktpresentation/src/main/java/com/puzzle/presentation/MainActivity.ktpresentation/src/main/java/com/puzzle/presentation/MainViewModel.ktpresentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.ktpresentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt
🧰 Additional context used
🧬 Code graph analysis (2)
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (1)
presentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.kt (1)
MaintenanceDialog(20-56)
presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt (5)
core/network/src/main/java/com/puzzle/network/api/PieceApi.kt (1)
loadTerms(70-71)core/data/src/main/java/com/puzzle/data/repository/TermsRepositoryImpl.kt (1)
loadTerms(23-30)core/network/src/main/java/com/puzzle/network/source/term/TermDataSource.kt (2)
loadTerms(5-8)loadTerms(6-6)core/network/src/main/java/com/puzzle/network/source/term/TermDataSourceImpl.kt (1)
loadTerms(14-14)core/common/src/main/java/com/puzzle/common/ResultUtil.kt (1)
suspendRunCatching(5-13)
🪛 detekt (1.23.8)
core/network/src/main/java/com/puzzle/network/source/configure/ConfigureDataSource.kt
[warning] 33-33: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (14)
core/domain/src/main/java/com/puzzle/domain/repository/ConfigureRepository.kt (1)
4-8: LGTM!The new
getMaintenancePeriod()method follows the same pattern as the existinggetUpdateInfo()method. Clean and consistent interface extension.core/network/src/main/java/com/puzzle/network/source/configure/ConfigureDataSource.kt (1)
63-66: LGTM!The new
MAINTENANCE_PERIODconstant follows the existing pattern established byFORCE_UPDATE.core/designsystem/src/main/res/values/strings.xml (1)
320-326: LGTM!The new maintenance strings are well-structured and follow the existing pattern in the file. The user-facing text is clear and appropriate for a maintenance notification dialog.
core/network/src/main/java/com/puzzle/network/model/configure/GetMaintenancePeriodInfoResponse.kt (1)
1-13: LGTM!The network model follows the established project pattern with
@Serializableannotation and atoDomain()mapper. The default empty string formaintenancePeriodis appropriate for handling cases when no maintenance is scheduled. This dependency direction (network → domain) is consistent across the network layer.core/data/src/main/java/com/puzzle/data/repository/ConfigureRepositoryImpl.kt (1)
22-27: LGTM!The implementation follows the established pattern of
getUpdateInfo()and correctly retrieves the maintenance period configuration using the data source with proper domain mapping.presentation/src/main/java/com/puzzle/presentation/MainViewModel.kt (4)
54-56: LGTM!State declaration follows the same pattern as
forceUpdatewith proper encapsulation.
95-101: LGTM!Good refactor to parallel execution using
coroutineScope. All launched coroutines will complete before the function returns, which is the correct behavior for initialization.
103-106: LGTM!The
refreshAppStatus()function correctly groups the version and maintenance checks for resumption scenarios, enabling real-time remote config reflection.
116-122: LGTM!The implementation follows the same error-handling pattern as
checkMinVersion(). The UI layer (MaintenanceDialog) correctly handles empty/blank strings by checkingisNullOrBlank()before rendering.presentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.kt (1)
20-26: LGTM!Good defensive programming with the early return for null/blank strings and safe cast to
Activity.presentation/src/main/java/com/puzzle/presentation/MainActivity.kt (2)
99-99: LGTM!State observation and passing follows the established pattern used for
forceUpdate.Also applies to: 108-108
155-160: LGTM!Using
lifecycleScopeensures proper coroutine cancellation when the activity is destroyed. The approach of refreshing app status on every resume is appropriate for real-time remote config updates, andStateFlowwill deduplicate identical values to prevent unnecessary recompositions.presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (1)
54-54: LGTM!The nullable parameter follows the same pattern as
forceUpdateand allows for a clean no-maintenance state.core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (1)
431-451: LGTM!The preview correctly demonstrates the new
descriptionparameter usage, which is helpful for visual verification of the feature.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| init { | ||
| val configSettings = remoteConfigSettings { | ||
| minimumFetchIntervalInSeconds = 0 | ||
| } | ||
| remoteConfig.setConfigSettingsAsync(configSettings) | ||
| remoteConfig.addOnConfigUpdateListener(object : ConfigUpdateListener { | ||
| override fun onUpdate(configUpdate: ConfigUpdate) { | ||
| remoteConfig.activate() | ||
| } | ||
| override fun onError(error: FirebaseRemoteConfigException) {} | ||
| }) | ||
| } |
There was a problem hiding this comment.
Silent error handling could mask configuration issues.
The empty onError handler will silently swallow FirebaseRemoteConfigException errors, making it difficult to diagnose connectivity or configuration issues in production. Consider logging the error at minimum.
Also, minimumFetchIntervalInSeconds = 0 means every fetch request will hit the server, which could lead to throttling in production. Firebase recommends longer intervals (e.g., 3600 seconds) for production.
🔧 Suggested improvement
init {
val configSettings = remoteConfigSettings {
- minimumFetchIntervalInSeconds = 0
+ minimumFetchIntervalInSeconds = if (BuildConfig.DEBUG) 0 else 3600
}
remoteConfig.setConfigSettingsAsync(configSettings)
remoteConfig.addOnConfigUpdateListener(object : ConfigUpdateListener {
override fun onUpdate(configUpdate: ConfigUpdate) {
remoteConfig.activate()
}
- override fun onError(error: FirebaseRemoteConfigException) {}
+ override fun onError(error: FirebaseRemoteConfigException) {
+ // Log error for observability; real-time updates will retry automatically
+ android.util.Log.w("ConfigDataSource", "Remote config update failed", error)
+ }
})
}🧰 Tools
🪛 detekt (1.23.8)
[warning] 33-33: This empty block of code can be removed.
(detekt.empty-blocks.EmptyFunctionBlock)
🤖 Prompt for AI Agents
In
`@core/network/src/main/java/com/puzzle/network/source/configure/ConfigureDataSource.kt`
around lines 24 - 35, The init block in ConfigureDataSource.kt currently
swallows Remote Config errors and uses minimumFetchIntervalInSeconds = 0; update
the remoteConfig.addOnConfigUpdateListener implementation so onError(error:
FirebaseRemoteConfigException) logs the exception (use your app
logger/Timber/Log.e) including contextual text, and avoid silently discarding
it; also change minimumFetchIntervalInSeconds from 0 to a production-safe
default (e.g., 3600) and/or make it configurable via build flags (keep 0 only
for debug builds) so fetches aren’t throttled in production; ensure
remoteConfig.activate() remains called only on successful onUpdate.
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt`:
- Around line 117-121: The code accesses maintenancePeriod.maintenancePeriod
without a safe-call so Kotlin won't smart-cast the nullable receiver; fix by
extracting the inner string to a local val (e.g., maintenanceText =
maintenancePeriod?.maintenancePeriod) and use that in the conditional: if
(!maintenanceText.isNullOrBlank()) call MaintenanceDialog(maintenanceText) else
call ForceUpdateDialog(forceUpdate); this ensures null-safety and correct
smart-casting.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
presentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.ktpresentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- presentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.kt
🧰 Additional context used
🧬 Code graph analysis (1)
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (2)
presentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.kt (1)
MaintenanceDialog(20-55)presentation/src/main/java/com/puzzle/presentation/update/ForceUpdateDialog.kt (1)
ForceUpdateDialog(25-61)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (1)
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (1)
40-43: LGTM on imports and parameter addition.The new imports and nullable parameter correctly support the maintenance period feature.
Also applies to: 54-54
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
presentation/src/main/java/com/puzzle/presentation/update/ForceUpdateDialog.kt (1)
51-59: Avoid force unwrap onversionNamewhich can be null.
PackageInfo.versionNamecan be null according to Android documentation (e.g., if not set in the manifest). Using!!could cause a crash in edge cases.🛡️ Suggested defensive handling
internal fun isShowForceUpdateDialog( context: Context, info: ForceUpdate?, ): Boolean { if (info == null) return false - val currentVersion = context.packageManager.getPackageInfo(context.packageName, 0).versionName - return checkShouldUpdate(currentVersion!!, info.minVersion) + val currentVersion = context.packageManager.getPackageInfo(context.packageName, 0).versionName + ?: return false + return checkShouldUpdate(currentVersion, info.minVersion) }
🧹 Nitpick comments (1)
presentation/src/main/java/com/puzzle/presentation/update/ForceUpdateDialog.kt (1)
81-83: Minor: consider handling single-part versions.The function normalizes 2-part versions (e.g.,
"1.0"→[1, 0, 0]) but leaves 1-part versions as-is (e.g.,"1"→[1]). WhilecheckShouldUpdatehandles this viagetOrElse, you might want to normalize all short versions for consistency.♻️ Optional: normalize all short versions
-private fun normalizeVersion(version: String): List<Int> = version.split('.') - .map { it.toIntOrNull() ?: 0 } - .let { if (it.size == 2) it + 0 else it } +private fun normalizeVersion(version: String): List<Int> = version.split('.') + .map { it.toIntOrNull() ?: 0 } + .let { parts -> parts + List(maxOf(0, 3 - parts.size)) { 0 } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.ktpresentation/src/main/java/com/puzzle/presentation/update/ForceUpdateDialog.kt
🧰 Additional context used
🧬 Code graph analysis (2)
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (2)
presentation/src/main/java/com/puzzle/presentation/update/ForceUpdateDialog.kt (2)
isShowForceUpdateDialog(51-59)ForceUpdateDialog(20-49)presentation/src/main/java/com/puzzle/presentation/maintenance/MaintenanceDialog.kt (1)
MaintenanceDialog(20-55)
presentation/src/main/java/com/puzzle/presentation/update/ForceUpdateDialog.kt (2)
core/designsystem/src/main/java/com/puzzle/designsystem/component/Dialog.kt (1)
PieceDialog(33-58)core/designsystem/src/main/java/com/puzzle/designsystem/component/Button.kt (1)
PieceSolidButton(44-72)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (4)
presentation/src/main/java/com/puzzle/presentation/ui/PieceApp.kt (3)
65-69: State derivation logic looks correct and addresses previous null safety concerns.The
takeIf { it.isNotBlank() }pattern correctly handles the nullable chain, and the prioritization logic (maintenance takes precedence over force update) is properly implemented.Minor performance consideration:
isShowForceUpdateDialog(context, forceUpdate)callsPackageManager.getPackageInfo()on each recomposition. This is unlikely to cause issues in practice, but you could wrap it inremember(forceUpdate)if recomposition frequency becomes a concern.
74-74: LGTM!The blur modifier is correctly applied to the Scaffold when either popup is showing, providing consistent visual feedback.
129-134: LGTM! Dialog precedence is correctly implemented.The conditional rendering ensures mutual exclusivity between
MaintenanceDialogandForceUpdateDialog, with maintenance taking priority. This addresses the previously raised concerns about:
- Both dialogs showing simultaneously (now impossible due to
else if)- Null safety (resolved by extracting
maintenanceStringearlier)presentation/src/main/java/com/puzzle/presentation/update/ForceUpdateDialog.kt (1)
20-49: LGTM!The refactored
ForceUpdateDialogcleanly delegates visibility control to the caller, making the component simpler and more reusable. The emptyonDismissRequestis appropriate for a force update scenario where the user must take action.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
| } | ||
|
|
||
| override suspend fun getMaintenancePeriod(): MaintenancePeriod { | ||
| return configureDataSource.getReferenceType( |
There was a problem hiding this comment.
한줄리턴이면 그냥 = 쓰는방향으로 가면 좋을듯합니다
| override fun onUpdate(configUpdate: ConfigUpdate) { | ||
| remoteConfig.activate() | ||
| } | ||
| override fun onError(error: FirebaseRemoteConfigException) {} |
1. ⭐️ 변경된 내용
2. 🖼️ 스크린샷(선택)
3. 💡 알게된 부분
4. 📌 이 부분은 꼭 봐주세요!
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.