fix: Server Config button and improved screen design #2568
Conversation
📝 WalkthroughWalkthroughAdded two navigation helpers and a server-config navigation graph; wired settings to open the server-config screen. Replaced a bottom snackbar with a centered RestartCountdownDialog after saving configuration, including an immediate "Restart Now" action and auto-restart on dismiss. Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsUI as "Settings UI"
participant Navigation as "Navigation"
participant ServerConfig as "ServerConfig Screen"
participant RestartDialog as "Restart Dialog"
participant App as "App"
User->>SettingsUI: Click "Update Configuration"
SettingsUI->>Navigation: navigateToServerConfigGraph()
Navigation->>ServerConfig: navigate to server config screen
ServerConfig->>ServerConfig: save config
ServerConfig->>RestartDialog: show RestartCountdownDialog on success
rect rgb(230,245,255)
Note over RestartDialog: Countdown phase (progress + numeric timer)
RestartDialog->>User: show check icon, message, timer
end
alt User taps "Restart Now"
User->>RestartDialog: Tap "Restart Now"
RestartDialog->>App: trigger immediate restart
else Countdown ends or dialog dismissed
RestartDialog->>App: onDismiss triggers restart
end
App->>App: restart with new config
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 1
🧹 Nitpick comments (2)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (2)
526-529: Consider removing the leading spaces.The two leading spaces in the button text are unusual. If spacing is needed, prefer using padding modifiers on the Text composable instead.
🔎 Proposed refactor
- Text( - " " + - stringResource( - Res.string.feature_settings_update_config_btn_text, - ).uppercase(), - fontFamily = FontFamily.SansSerif, - ) + Text( + stringResource( + Res.string.feature_settings_update_config_btn_text, + ).uppercase(), + fontFamily = FontFamily.SansSerif, + modifier = Modifier.padding(start = 4.dp), + )
624-633: Consider using a string resource for "Restart Now".The button text "Restart Now" is hardcoded. For consistency and internationalization support, consider adding it as a string resource in
feature_settings_.xml.🔎 Proposed refactor
Add to
feature_settings_.xml:<string name="feature_settings_restart_now">Restart Now</string>Then update the button:
- Text("Restart Now") + Text(stringResource(Res.string.feature_settings_restart_now))
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.ktfeature/settings/src/commonMain/composeResources/values/feature_settings_.xmlfeature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/navigation/SettingsNavigation.kt (1)
serverConfigGraph(45-53)
🪛 GitHub Actions: PR Checks
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt
[error] 87-87: spotlessKotlinCheck failed: formatting violations detected in the file. Run './gradlew :cmp-navigation:spotlessApply' to fix.
🔇 Additional comments (9)
feature/settings/src/commonMain/composeResources/values/feature_settings_.xml (1)
28-30: LGTM!The new string resources are well-named and support the updated restart countdown dialog flow.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt (2)
30-31: LGTM!The imports are correctly added to support the server config navigation flow.
89-91: LGTM!The server config graph is properly integrated with correct back navigation wiring.
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (6)
14-53: LGTM!The new import additions support the RestartCountdownDialog implementation and are all utilized in the updated UI flow.
124-142: LGTM! Clean modal overlay implementation.The semi-transparent overlay with click consumption prevents user interaction during the restart countdown, which is appropriate UX for this critical flow.
181-182: Nice UI refinement!The increased padding and spacing improves visual clarity and breathing room in the settings layout.
224-224: Good simplification!Removing the emoji unicode characters makes the button labels cleaner and more consistent across platforms.
Also applies to: 261-261
557-620: LGTM! Well-structured dialog UI.The RestartCountdownDialog has a clean, centered layout with appropriate spacing, colors, and visual hierarchy. The success icon, title, message, and countdown are well-organized.
539-555: The current progress calculation is correct and follows the expected UX pattern for a countdown timer. The progress drains from full (1.0 when 5 seconds remain) to empty (0.0 when 0 seconds remain), which aligns with the numeric countdown display (5 → 4 → 3 → 2 → 1 → 0) and clearly communicates "remaining time." Inverting the calculation would show elapsed time instead, which would be counterintuitive for a restart countdown dialog.Likely an incorrect or invalid review comment.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt
Show resolved
Hide resolved
...gs/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
Show resolved
Hide resolved
| .background(Color(0xfff8f9fa).copy(alpha = .1f)), | ||
| contentPadding = PaddingValues(4.dp), | ||
| verticalArrangement = Arrangement.spacedBy(4.dp), | ||
| contentPadding = PaddingValues(6.dp), |
There was a problem hiding this comment.
here color and dp values are hardcoded
...gs/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
Show resolved
Hide resolved
...gs/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
Outdated
Show resolved
Hide resolved
...gs/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (1)
118-118: Potential NPE from force unwrap on nullable state.Using
!!onviewModel.state.valuecan throw a NullPointerException if the state is null. Consider using a safe approach such as providing a default value or handling the null case gracefully.🔎 Proposed fix
- serverConfig = viewModel.state.value!!, + serverConfig = viewModel.state.value ?: return,Or collect the state safely:
val serverConfig by viewModel.state.collectAsStateWithLifecycle() // Then use serverConfig with null check or provide default
♻️ Duplicate comments (3)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (3)
130-138: Hardcoded color for overlay background.
Color.Black.copy(alpha = 0.6f)should use a theme-aware color fromMaterialTheme.colorScheme(e.g.,scrim) or define it inAppColors/DesignTokenfor consistency.🔎 Proposed fix
Box( modifier = Modifier .fillMaxSize() - .background(Color.Black.copy(alpha = 0.6f)) + .background(MaterialTheme.colorScheme.scrim.copy(alpha = 0.6f)) .clickable(
287-302: Hardcoded divider color.
Color.LightGrayshould be replaced with a theme-aware color such asMaterialTheme.colorScheme.outlineVariantfor consistency with the design system.🔎 Proposed fix
HorizontalDivider( modifier = Modifier.weight(1f), - color = Color.LightGray, + color = MaterialTheme.colorScheme.outlineVariant, ) // ... Text ... HorizontalDivider( modifier = Modifier.weight(1f), - color = Color.LightGray, + color = MaterialTheme.colorScheme.outlineVariant, )
601-606: HardcodedColor.Grayfor secondary text.Replace with a theme-aware color like
MaterialTheme.colorScheme.onSurfaceVariantfor consistency with the design system.🔎 Proposed fix
Text( text = stringResource(Res.string.feature_settings_restart_application), style = MaterialTheme.typography.bodyMedium, - color = Color.Gray, + color = MaterialTheme.colorScheme.onSurfaceVariant, textAlign = TextAlign.Center, )
🧹 Nitpick comments (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (1)
531-538: Avoid hardcoded spaces in text content.Prepending
" "for visual spacing is fragile. UseModifier.padding()on theTextor adjust the button'scontentPaddinginstead.🔎 Proposed fix
Text( - " " + - stringResource( - Res.string.feature_settings_update_config_btn_text, - ).uppercase(), + text = stringResource(Res.string.feature_settings_update_config_btn_text).uppercase(), + modifier = Modifier.padding(start = DesignToken.padding.small), fontFamily = FontFamily.SansSerif, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/settings/src/commonMain/composeResources/values/feature_settings_.xmlfeature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/settings/src/commonMain/composeResources/values/feature_settings_.xml
🧰 Additional context used
🧬 Code graph analysis (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigViewModel.kt (1)
onEvent(79-141)
⏰ 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: PR Checks / Static Analysis Check
🔇 Additional comments (4)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (4)
186-198: LGTM!Good use of
DesignTokenfor padding and spacing, aligning with the design system.
207-278: LGTM!The elevated buttons properly use
DesignTokenfor sizing/shapes andMaterialTheme.colorSchemefor colors, following the design system consistently.
545-641: Well-structured countdown dialog implementation.The
RestartCountdownDialogproperly usesLaunchedEffectfor countdown timing,rememberSaveablefor state persistence across configuration changes, and consistentDesignTokenstyling. The progress indicator API usage is correct.
306-477: LGTM!The form fields consistently use
DesignTokenfor shapes and padding, andMaterialTheme.colorSchemefor colors, providing a unified look.
...gs/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (1)
117-117: Force unwrap on nullable state could cause NPE.
viewModel.state.value!!will crash if the state is null. Consider using a null-safe approach or ensuring the state is never null by initializing with a default value.🔎 Proposed fix
Option 1: Use safe call with early return:
+ val serverConfigState = viewModel.state.value ?: return + Box(modifier = Modifier.fillMaxSize()) { UpdateServerConfigScreenContent( modifier = modifier, - serverConfig = viewModel.state.value!!, + serverConfig = serverConfigState,Option 2: Show loading state when null:
+ val serverConfigState = viewModel.state.value + if (serverConfigState == null) { + // Show loading or return early + return + } + Box(modifier = Modifier.fillMaxSize()) { UpdateServerConfigScreenContent( modifier = modifier, - serverConfig = viewModel.state.value!!, + serverConfig = serverConfigState,
♻️ Duplicate comments (2)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (2)
286-301: Replace hardcodedColor.LightGraywith theme color.The divider uses hardcoded
Color.LightGraywhich won't adapt to dark/light theme changes. Based on past review comments about hardcoded colors, use a theme-based color.🔎 Proposed fix
HorizontalDivider( modifier = Modifier.weight(1f), - color = Color.LightGray, + color = MaterialTheme.colorScheme.outlineVariant, ) // ... Text ... HorizontalDivider( modifier = Modifier.weight(1f), - color = Color.LightGray, + color = MaterialTheme.colorScheme.outlineVariant, )
595-600: Replace hardcodedColor.Graywith theme color.Using
Color.Graywon't adapt to theme changes. Use a theme-based color for better dark/light mode support.🔎 Proposed fix
Text( text = stringResource(Res.string.feature_settings_restart_application), style = MaterialTheme.typography.bodyMedium, - color = Color.Gray, + color = MaterialTheme.colorScheme.onSurfaceVariant, textAlign = TextAlign.Center, )
🧹 Nitpick comments (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (1)
526-532: Use padding instead of hardcoded spaces in text.The leading
" "spaces are a fragile way to add spacing. UseModifier.paddingon the Text or adjust button content padding instead.🔎 Proposed fix
Text( - " " + - stringResource( - Res.string.feature_settings_update_config_btn_text, - ).uppercase(), + stringResource(Res.string.feature_settings_update_config_btn_text).uppercase(), + modifier = Modifier.padding(start = DesignToken.padding.small), fontFamily = FontFamily.SansSerif, )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (1)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigViewModel.kt (1)
onEvent(79-141)
⏰ 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: PR Checks / Static Analysis Check
🔇 Additional comments (2)
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt (2)
181-188: Good migration to DesignToken and MaterialTheme.The styling changes properly use
DesignTokenfor padding, spacing, and shapes, andMaterialTheme.colorSchemefor colors. This is consistent with the codebase's design system approach.
558-634: Clean dialog implementation with proper countdown UX.The
RestartCountdownDialogis well-structured with:
- Clear visual hierarchy (success icon, title, subtitle, progress, action)
- Proper use of DesignToken for sizing and spacing
- Correct countdown logic with auto-dismiss
The circular progress indicator effectively communicates the remaining time before restart.
| private fun RestartCountdownDialog( | ||
| durationSeconds: Int = 5, | ||
| dismissText: String? = null, | ||
| onDismiss: () -> Unit, | ||
| modifier: Modifier = Modifier, | ||
| onAboutToEnd: () -> Unit = {}, | ||
| ) { | ||
| var countdown by rememberSaveable { mutableStateOf(durationSeconds) } | ||
|
|
||
| val progress = countdown.toFloat() / durationSeconds.toFloat() |
There was a problem hiding this comment.
Guard against division by zero.
If durationSeconds is passed as 0, line 547 will cause a division by zero. While the default is 5, the function is private and could be called with 0.
🔎 Proposed fix
private fun RestartCountdownDialog(
durationSeconds: Int = 5,
onDismiss: () -> Unit,
modifier: Modifier = Modifier,
) {
+ require(durationSeconds > 0) { "durationSeconds must be positive" }
var countdown by rememberSaveable { mutableStateOf(durationSeconds) }
- val progress = countdown.toFloat() / durationSeconds.toFloat()
+ val progress = countdown.toFloat() / durationSeconds.coerceAtLeast(1).toFloat()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private fun RestartCountdownDialog( | |
| durationSeconds: Int = 5, | |
| dismissText: String? = null, | |
| onDismiss: () -> Unit, | |
| modifier: Modifier = Modifier, | |
| onAboutToEnd: () -> Unit = {}, | |
| ) { | |
| var countdown by rememberSaveable { mutableStateOf(durationSeconds) } | |
| val progress = countdown.toFloat() / durationSeconds.toFloat() | |
| private fun RestartCountdownDialog( | |
| durationSeconds: Int = 5, | |
| onDismiss: () -> Unit, | |
| modifier: Modifier = Modifier, | |
| ) { | |
| require(durationSeconds > 0) { "durationSeconds must be positive" } | |
| var countdown by rememberSaveable { mutableStateOf(durationSeconds) } | |
| val progress = countdown.toFloat() / durationSeconds.coerceAtLeast(1).toFloat() |
🤖 Prompt for AI Agents
In
feature/settings/src/commonMain/kotlin/com/mifos/feature/settings/updateServer/UpdateServerConfigScreen.kt
around lines 540 to 547, the progress calculation can divide by zero if
durationSeconds is 0; change the logic to guard against that by normalizing the
denominator before division (e.g., use a safeDuration = max(1, durationSeconds)
or check if durationSeconds <= 0 and set progress to 0f), and then compute
progress using that safe value; ensure any other uses of durationSeconds in this
function respect the same guard so no other divisions or assumptions about >0
occur.
4 participants
Fixes - Jira-#620
Fixed the "Server Config" button so it correctly opens the screen and updated the design to make switching between Local and Demo environments easier.
Added a popup window with a countdown and a "Restart Now" button to clearly show that changes are saved before the app restarts.
Screenshots:
Summary by CodeRabbit
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.