feat : Added Time Based Theming#3022
Conversation
📝 WalkthroughWalkthroughThis PR introduces time-based dark mode theming, allowing users to configure specific hours when dark mode automatically activates. The feature includes UI components for time selection, data persistence, and integration with the existing theme system across the presentation, data, and model layers. Changes
Sequence DiagramssequenceDiagram
actor User
participant ThemeScreen as ChangeThemeScreen
participant ThemeVM as ChangeThemeViewModel
participant Repo as UserPreferencesRepository
participant DataSource as UserPreferencesDataSource
User->>ThemeScreen: Select BASED_ON_TIME
ThemeScreen->>ThemeVM: Emit ShowTimeBasedDialog
ThemeVM->>ThemeVM: Update state: showTimeBasedDialog=true
ThemeScreen->>ThemeScreen: Render TimeBasedThemeDialog
User->>ThemeScreen: Configure start/end hours, confirm
ThemeScreen->>ThemeVM: Emit UpdateTimeBasedTheme(newTheme)
ThemeVM->>Repo: updateTimeBasedTheme(newTheme)
Repo->>DataSource: updateTimeBasedTheme(newTheme)
DataSource->>DataSource: Update settingsInfo.timeBasedTheme
DataSource-->>Repo: Success
Repo-->>ThemeVM: DataState.Success
ThemeVM->>ThemeVM: Update state: currentTheme=BASED_ON_TIME<br/>timeBasedTheme=newTheme<br/>showTimeBasedDialog=false
Repo->>Repo: updateTheme(BASED_ON_TIME)
ThemeVM-->>ThemeScreen: Emit updated ThemeState
sequenceDiagram
participant App as ComposeAppViewModel
participant DateHelper
participant Repo as UserPreferencesRepository
participant Theme as AppTheme
App->>Repo: Observe timeBasedThemeConfig
Repo-->>App: Flow[TimeBasedTheme]
App->>App: Emit TimeBasedThemeUpdate(timeBasedTheme)
App->>App: Handle TimeBasedThemeUpdate
alt themeConfig == BASED_ON_TIME
App->>DateHelper: isDarkModeBasedOnTime(start, end)
DateHelper->>DateHelper: Check current time vs range
DateHelper-->>App: Boolean (isDark)
App->>App: Compute darkTheme = isDark
else themeConfig == FOLLOW_SYSTEM
App->>App: Use system darkTheme
else
App->>App: Use explicit LIGHT/DARK
end
App->>App: Update state: darkTheme, timeBasedTheme
App->>Theme: Apply theme with computed darkTheme
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes The changes span multiple architectural layers with heterogeneous modifications: new data types, repository/datastore extensions, non-trivial UI component composition with nested dialogs and state management, time-based calculations, and integration across the app's view models. Each cohort requires distinct reasoning, though patterns are generally consistent. Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.kt (1)
150-167: Missing automatic theme updates for time-based theming.The time-based theme is only evaluated when
handleAppThemeUpdatedis triggered (on theme config changes). This means the app won't automatically switch between light and dark modes when crossing the 6 AM or 6 PM thresholds unless the user restarts the app or manually changes the theme.Consider implementing a periodic check or scheduling updates at 6 AM and 6 PM when
themeConfig == BASED_ON_TIMEto provide a seamless experience.Potential implementation approach
You could add a coroutine that schedules the next theme update:
private fun scheduleNextThemeUpdate() { if (mutableStateFlow.value.themeConfig != MifosThemeConfig.BASED_ON_TIME) return viewModelScope.launch { val now = Clock.System.now().toLocalDateTime(TimeZone.currentSystemDefault()) val currentHour = now.time.hour // Calculate delay until next transition (6 AM or 6 PM) val nextTransitionHour = if (currentHour < 6) 6 else if (currentHour < 18) 18 else 6 // ... calculate delay and schedule update delay(calculatedDelay) trySendAction(AppAction.Internal.ThemeUpdate(MifosThemeConfig.BASED_ON_TIME)) scheduleNextThemeUpdate() // Schedule next } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.kt(2 hunks)core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DateHelper.kt(1 hunks)core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/MifosThemeConfig.kt(1 hunks)feature/settings/src/commonMain/composeResources/values/strings.xml(1 hunks)feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeViewModel.kt(2 hunks)
⏰ 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 KMP / Static Analysis Check
🔇 Additional comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeViewModel.kt (1)
140-140: LGTM!The new time-based theme option is correctly integrated into the theme selection UI with proper string resource mapping.
feature/settings/src/commonMain/composeResources/values/strings.xml (1)
212-212: LGTM!The string resource for the time-based theme option is properly defined.
core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/MifosThemeConfig.kt (1)
16-16: LGTM!The new
BASED_ON_TIMEenum constant is properly defined with sequentialosValueand will be automatically included in thefromStringmapping.
revanthkumarJ
force-pushed
the
time_based_theme
branch
from
691e557 to
57fa06c
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
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/org/mifos/mobile/feature/settings/theme/ChangeThemeViewModel.kt (1)
1-20: Remove duplicate copyright header.The copyright notice appears twice (lines 1-9 and 12-20). Remove the duplicate at lines 12-20.
🔎 Proposed fix
package org.mifos.mobile.feature.settings.theme -/* - * Copyright 2025 Mifos Initiative - * - * This Source Code Form is subject to the terms of the Mozilla Public - * License, v. 2.0. If a copy of the MPL was not distributed with this - * file, You can obtain one at https://mozilla.org/MPL/2.0/. - * - * See https://github.com/openMF/mobile-mobile/blob/master/LICENSE.md - */ - import androidx.lifecycle.viewModelScope
🧹 Nitpick comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (2)
185-185: Format time display to use leading zeros for consistency.The time display uses
"$startHour:$startMinute", which shows single-digit hours and minutes without padding (e.g., "6:5" instead of "06:05"). This is inconsistent with standard time formatting.🔎 Proposed fix
TimeRow( label = "Dark mode starts at", - time = "$startHour:$startMinute", + time = "${startHour.toString().padStart(2, '0')}:${startMinute.toString().padStart(2, '0')}", onClick = { showStartPicker = true }, ) TimeRow( label = "Dark mode ends at", - time = "$endHour:$endMinute", + time = "${endHour.toString().padStart(2, '0')}:${endMinute.toString().padStart(2, '0')}", onClick = { showEndPicker = true }, )Or extract a helper function:
private fun formatTime(hour: Int, minute: Int): String { return "${hour.toString().padStart(2, '0')}:${minute.toString().padStart(2, '0')}" }Also applies to: 191-191
198-206: Add validation to prevent invalid time ranges.The dialog allows users to set any start and end times without validation. Consider adding checks to ensure the time range is sensible (e.g., dark mode period is at least 1 minute, or provide clear feedback about cross-midnight ranges).
While cross-midnight ranges are supported by the logic, users might accidentally set identical or invalid start/end times.
🔎 Example validation
MifosButton( onClick = { + val theme = TimeBasedTheme( + hourStart = startHour, + timeStart = startMinute, + hourEnd = endHour, + timeEnd = endMinute, + ) + + // Validate that start and end aren't identical + if (startHour == endHour && startMinute == endMinute) { + // Show error toast or prevent closing + return@MifosButton + } + - onConfirm( - TimeBasedTheme( - hourStart = startHour, - timeStart = startMinute, - hourEnd = endHour, - timeEnd = endMinute, - ), - ) + onConfirm(theme) }, ) {feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeViewModel.kt (1)
94-107: Consider adding validation for time-based theme configuration.The
UpdateTimeBasedThemeaction doesn't validate the input (e.g., ensuringhourStart < hourEndor that hours are in valid range 0-23). Invalid configurations could lead to unexpected behavior when determining dark/light mode.🔎 Proposed validation
is ThemeAction.UpdateTimeBasedTheme -> { viewModelScope.launch { + // Validate time-based theme configuration + require(action.theme.hourStart in 0..23) { "hourStart must be in range 0-23" } + require(action.theme.hourEnd in 0..23) { "hourEnd must be in range 0-23" } + require(action.theme.hourStart != action.theme.hourEnd) { "hourStart and hourEnd must be different" } + repository.updateTimeBasedTheme(action.theme) repository.updateTheme(MifosThemeConfig.BASED_ON_TIME)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.ktcore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DateHelper.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepository.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepositoryImpl.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/model/AppSettings.ktcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/model/TimeBasedTheme.ktcore/model/src/commonMain/kotlin/org/mifos/mobile/core/model/MifosThemeConfig.ktfeature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeViewModel.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/model/src/commonMain/kotlin/org/mifos/mobile/core/model/MifosThemeConfig.kt
🧰 Additional context used
🧬 Code graph analysis (1)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.kt (1)
core/ui/src/commonMain/kotlin/org/mifos/mobile/core/ui/utils/BaseViewModel.kt (1)
trySendAction(83-85)
⏰ 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 KMP / Static Analysis Check
🔇 Additional comments (16)
feature/settings/src/commonMain/composeResources/values/strings.xml (1)
212-212: LGTM!The string resource is properly added to support the time-based theming option.
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepository.kt (1)
42-42: LGTM!The interface properly exposes time-based theme configuration observability and mutation capabilities.
Also applies to: 52-52
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.kt (1)
152-172: Verify theme recomputation doesn't cause unnecessary updates.When
TimeBasedThemeUpdateis received but the current theme config is notBASED_ON_TIME, the function still updates the state with the newtimeBasedThemevalue. This is fine for storing the configuration, but ensure this doesn't trigger unnecessary recompositions when dark mode shouldn't actually change.The logic correctly preserves the current
darkThemevalue when not in BASED_ON_TIME mode (line 163), which is good.core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesRepositoryImpl.kt (1)
76-77: LGTM!The implementation correctly delegates to the preference manager and wraps results in
DataStatefor consistent error handling.Also applies to: 103-110
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (1)
143-154: LGTM!The conditional rendering of the dialog based on
showTimeBasedDialogstate is implemented correctly, with proper action handling for dismiss and confirm callbacks.core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt (3)
27-27: LGTM!The import is correctly added to support the new time-based theme feature.
87-88: LGTM!The observable follows the established pattern used for other settings (e.g.,
observeDarkThemeConfigon lines 84-85).
144-150: LGTM!The implementation correctly follows the same pattern as
updateTheme(lines 136-142), ensuring consistency in how settings are persisted.feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeViewModel.kt (8)
28-28: LGTM!The imports are correctly added to support time-based theming.
Also applies to: 33-33
60-64: LGTM!The observer correctly subscribes to time-based theme config changes and updates state via the internal action.
80-93: LGTM!The dialog visibility toggle handlers are straightforward and correct.
109-111: LGTM!The internal action handler correctly delegates to the appropriate method.
169-173: LGTM!The method correctly updates state with the loaded time-based theme configuration.
199-199: LGTM!The new theme option is correctly added to the list.
234-238: LGTM!The action definitions are well-structured and follow the existing pattern.
Also applies to: 251-251
184-189: ThetimeStartandtimeEndfields represent minutes (0-59), not a separate time unit.The default values in ChangeThemeViewModel.kt (
timeStart = 0, timeEnd = 0) are correct and represent 06:00-18:00 for light mode. However, verify that the default inAppSettings.kt(hourStart = 6, hourEnd = 0, timeStart = 17, timeEnd = 59) is intentional, as it currently represents 06:17-00:59 rather than the documented dark mode range (18:00-05:59).
cmp-navigation/src/commonMain/kotlin/cmp/navigation/ComposeAppViewModel.kt
Show resolved
Hide resolved
core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DateHelper.kt
Show resolved
Hide resolved
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/model/AppSettings.kt
Show resolved
Hide resolved
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/model/TimeBasedTheme.kt
Show resolved
Hide resolved
.../settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
Outdated
Show resolved
Hide resolved
...ttings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeViewModel.kt
Show resolved
Hide resolved
revanthkumarJ
force-pushed
the
time_based_theme
branch
from
8ececd0 to
1885791
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (1)
255-284: Consider adding accessibility semantics.The clickable Surface could benefit from explicit semantics to improve screen reader support, though this is an optional enhancement.
🔎 Optional enhancement
+import androidx.compose.ui.semantics.Role +import androidx.compose.ui.semantics.role +import androidx.compose.ui.semantics.semantics androidx.compose.material3.Surface( modifier = Modifier .fillMaxWidth() - .clickable(onClick = onClick), + .clickable(onClick = onClick) + .semantics { role = Role.Button }, shape = DesignToken.shapes.medium, tonalElevation = 1.dp, ) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
feature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- feature/settings/src/commonMain/composeResources/values/strings.xml
⏰ 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 KMP / Static Analysis Check
🔇 Additional comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (3)
12-56: LGTM - Imports are well-organized.All necessary imports for the time-based theme dialog functionality are present and properly organized.
149-160: LGTM - Dialog integration is correct.The conditional rendering and action wiring for the time-based theme dialog are properly implemented.
286-323: LGTM - TimePicker implementation is solid.The time picker dialog is properly configured with 24-hour format and correct state management.
.../settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
Show resolved
Hide resolved
.../settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
Show resolved
Hide resolved
.../settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
Show resolved
Hide resolved
revanthkumarJ
force-pushed
the
time_based_theme
branch
from
f94f138 to
8fea0b7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (2)
210-210: Apply zero-padding to time display.Time values are displayed without zero-padding (e.g., "6:5" instead of "06:05"), which degrades readability. This issue was previously flagged but remains unresolved.
🔎 Proposed fix
TimeRow( label = stringResource(Res.string.feature_settings_theme_choose_dark_mode_starts_at), - time = "$startHour:$startMinute", + time = "${startHour.toString().padStart(2, '0')}:${startMinute.toString().padStart(2, '0')}", onClick = { showStartPicker = true }, ) TimeRow( label = stringResource(Res.string.feature_settings_theme_choose_dark_mode_ends_at), - time = "$endHour:$endMinute", + time = "${endHour.toString().padStart(2, '0')}:${endMinute.toString().padStart(2, '0')}", onClick = { showEndPicker = true }, )Also applies to: 216-216
224-231: Add validation to prevent start time equaling end time.The confirm button allows users to set identical start and end times, which would enable dark mode continuously. This edge case was previously flagged but remains unaddressed. Add validation to prevent
start == endor disable the confirm button when times are equal.🔎 Proposed fix
Add a computed validation state and conditionally enable the button:
var endHour by remember { mutableStateOf(initialTheme.hourEnd) } var endMinute by remember { mutableStateOf(initialTheme.timeEnd) } + + val isValid = !(startHour == endHour && startMinute == endMinute) AlertDialog( onDismissRequest = onDismiss, ... confirmButton = { MifosButton( + enabled = isValid, onClick = { onConfirm( TimeBasedTheme(
🧹 Nitpick comments (1)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (1)
184-184: Consider reducing API visibility tointernal.
TimeBasedThemeDialogis currently public but appears to be used only within this feature module. Unless there's a specific requirement to expose it as a public API, consider marking it asinternalto reduce the API surface.🔎 Proposed change
-fun TimeBasedThemeDialog( +internal fun TimeBasedThemeDialog( initialTheme: TimeBasedTheme, onDismiss: () -> Unit, onConfirm: (TimeBasedTheme) -> Unit,
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
⏰ 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 KMP / Static Analysis Check
🔇 Additional comments (2)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (2)
168-179: LGTM! Dialog integration is well-structured.The conditional rendering of
TimeBasedThemeDialogcorrectly responds to state changes, and the callbacks are properly wired to theme actions for dismissal and confirmation.
274-342: LGTM! Helper components are well-implemented.Both
TimeRowandTimePickerDialogare properly encapsulated as private helpers with clean interfaces. The use of Material3'sTimePickerwith 24-hour format is appropriate, and the state management and callback wiring are correct.
.../settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (3)
123-143: Time display formatting and hardcoded strings still need attention.The time-based theme label construction still has the issues identified in the previous review:
- Time values lack zero-padding (e.g., "6:5" instead of "06:05")
- Hardcoded bracket characters and structure that should use parameterized string resources for i18n
213-223: Time display lacks zero-padding for consistent formatting.Both time displays still use raw interpolation without zero-padding, resulting in inconsistent formats like "6:5" instead of "06:05".
🔎 Proposed fix
TimeRow( label = stringResource(Res.string.feature_settings_theme_choose_dark_mode_starts_at), - time = "$startHour:$startMinute", + time = "${startHour.toString().padStart(2, '0')}:${startMinute.toString().padStart(2, '0')}", onClick = { showStartPicker = true }, ) TimeRow( label = stringResource(Res.string.feature_settings_theme_choose_dark_mode_ends_at), - time = "$endHour:$endMinute", + time = "${endHour.toString().padStart(2, '0')}:${endMinute.toString().padStart(2, '0')}", onClick = { showEndPicker = true }, )
226-237: Missing validation when start and end times are identical.The confirm button allows submission when
startHour == endHour && startMinute == endMinute, which would result in dark mode being active all day (or never, depending on interpretation). Consider disabling the confirm button or showing a validation error in this edge case.🔎 Proposed approach
+ val isValidTimeRange = !(startHour == endHour && startMinute == endMinute) + confirmButton = { MifosButton( + enabled = isValidTimeRange, onClick = { onConfirm( TimeBasedTheme( hourStart = startHour, timeStart = startMinute, hourEnd = endHour, timeEnd = endMinute, ), ) }, ) { Text(stringResource(Res.string.feature_settings_theme_apply)) } },
🧹 Nitpick comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (3)
294-306: Consider importingSurfaceand using design tokens for padding.Minor style improvements:
androidx.compose.material3.Surfaceis used with its fully qualified name - consider adding it to the imports for consistency- The hardcoded
16.dppadding could use a design token (e.g.,KptTheme.spacing.md) for consistency with the rest of the codebase🔎 Proposed fix
Add to imports:
import androidx.compose.material3.SurfaceThen:
- androidx.compose.material3.Surface( + Surface( modifier = Modifier .fillMaxWidth() .clickable(onClick = onClick), shape = DesignToken.shapes.medium, tonalElevation = 1.dp, ) { Text( text = time, - modifier = Modifier.padding(16.dp), + modifier = Modifier.padding(KptTheme.spacing.md), style = MifosTypography.titleMedium, ) }
335-341: Consider importingButtonDefaultsto avoid FQN usage.The fully qualified
androidx.compose.material3.ButtonDefaults.textButtonColors()is used here and on line 245. Adding it to the imports would improve readability.🔎 Proposed fix
Add to imports:
import androidx.compose.material3.ButtonDefaultsThen replace both occurrences:
- colors = androidx.compose.material3.ButtonDefaults.textButtonColors(), + colors = ButtonDefaults.textButtonColors(),
318-322: The suggested solution won't work in commonMain without expect/actual declarations.The
is24Hour = trueis hardcoded, but the suggestion to useDateFormat.is24HourFormat(LocalContext.current)is not viable here.DateFormatandLocalContextare Android-specific APIs that are unavailable incommonMaincode.If locale-aware 24-hour format detection is desired, implement it using the expect/actual pattern similar to
AppVersion.kt:
- Declare
expect fun is24HourFormat(): Booleanin commonMain- Provide platform-specific
actualimplementations (Android can useDateFormat.is24HourFormat(), other platforms with sensible defaults)However, the hardcoding may be intentional. Clarify with the team whether this should be locale-aware or remains a fixed design choice.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
🔇 Additional comments (1)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (1)
172-184: LGTM!The dialog integration follows a clean pattern - conditionally rendering based on UI state and properly dispatching actions through the
onActioncallback for both dismiss and confirm flows.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (3)
123-143: Hardcoded strings and missing zero-padding persist.This was flagged in a previous review. The label still contains hardcoded bracket characters and uses unpadded time values (e.g., "6:5" instead of "06:05").
213-223: Time display lacks zero-padding.This was flagged in previous reviews. Time values like "6:5" should display as "06:05" for readability.
226-237: Missing validation for start == end.This was flagged in a previous review. The confirm handler should prevent setting identical start and end times to avoid enabling dark mode all day.
🧹 Nitpick comments (2)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (2)
189-193: Consider makingTimeBasedThemeDialoginternal.Other screen composables in this file (
ChangeThemeScreen,ThemeScreenContent) are markedinternal, while helper composables (TimeRow,TimePickerDialog) areprivate. This public visibility may unintentionally expose the dialog beyond the module.🔎 Proposed fix
@OptIn(ExperimentalMaterial3Api::class) @Composable -fun TimeBasedThemeDialog( +internal fun TimeBasedThemeDialog( initialTheme: TimeBasedTheme, onDismiss: () -> Unit, onConfirm: (TimeBasedTheme) -> Unit,
294-299: Prefer imports over fully qualified references.
androidx.compose.material3.Surface(line 294) andButtonDefaults(lines 245, 338) are used with fully qualified names. Adding these to the import block would improve consistency and readability.🔎 Proposed fix
Add to imports:
import androidx.compose.material3.ButtonDefaults import androidx.compose.material3.SurfaceThen simplify usages:
- colors = androidx.compose.material3.ButtonDefaults.textButtonColors(), + colors = ButtonDefaults.textButtonColors(),- androidx.compose.material3.Surface( + Surface(
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
🔇 Additional comments (3)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (3)
12-59: LGTM!The new imports are appropriate for the time-based theming functionality. The organization follows the existing pattern in the file.
310-347: LGTM!The
TimePickerDialogimplementation correctly usesrememberTimePickerStatewith 24-hour format and properly passes the selected time via theonConfirmcallback.
279-308: LGTM!The
TimeRowcomposable is well-structured with clear separation of label and clickable time display. The private visibility is appropriate for this helper component.
revanthkumarJ
force-pushed
the
time_based_theme
branch
from
e10d638 to
5fa0c76
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (4)
124-145: Hardcoded strings in label construction need i18n support.The label still uses hardcoded bracket characters and formatting patterns that should be moved to string resources for proper internationalization. The time values are now formatted via
DateHelper.formatTimeRange, but the surrounding structure remains hardcoded.This was flagged in a previous review. Consider using parameterized string resources like:
<string name="feature_settings_theme_time_range_format">%1$s [%2$s]\n%3$s [%4$s]</string>
217-217: Format time display with zero-padding.The time string lacks zero-padding for single-digit values, resulting in formats like "6:5" instead of "06:05".
Apply the fix from the previous review:
- time = "$startHour:$startMinute", + time = "${startHour.toString().padStart(2, '0')}:${startMinute.toString().padStart(2, '0')}",
223-223: Format time display with zero-padding.Same zero-padding issue as the start time display.
Apply the fix:
- time = "$endHour:$endMinute", + time = "${endHour.toString().padStart(2, '0')}:${endMinute.toString().padStart(2, '0')}",
228-242: Add validation to prevent identical start and end times.The confirm button allows submitting a
TimeBasedThemewhere start and end times are identical, which would result in dark mode being active all day (or never, depending on interpretation). This edge case should be prevented.Consider disabling the confirm button or showing an error when
startHour == endHour && startMinute == endMinute.core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DateHelper.kt (1)
117-139: Add KDoc documentation and input validation.The function lacks documentation explaining its purpose and parameter semantics, as well as input validation for hour/minute ranges. This was flagged in a previous review.
The logic is correct for both same-day and cross-midnight ranges. Please address the previously suggested improvements:
- Add KDoc explaining that parameters define the dark mode active window
- Add
requirestatements to validatehour in 0..23andminute in 0..59
🧹 Nitpick comments (2)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (1)
296-308: Consider adding explicit import forSurface.
androidx.compose.material3.Surfaceis used with a fully qualified name while other Material3 components (AlertDialog,Text,TimePicker) are imported at the top of the file. For consistency, consider addingSurfaceto the imports.🔎 Proposed fix
Add to imports:
import androidx.compose.material3.SurfaceThen simplify:
- androidx.compose.material3.Surface( + Surface( modifier = Modifier .fillMaxWidth() .clickable(onClick = onClick),core/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DateHelper.kt (1)
141-157: Add input validation and consider i18n for AM/PM.The function lacks input validation for hour (0-23) and minute (0-59) ranges. Additionally, "AM" and "PM" are hardcoded English strings.
🔎 Proposed fix
fun formatTimeRange( startHour: Int, startMinute: Int, endHour: Int, endMinute: Int, ): String { + require(startHour in 0..23) { "startHour must be in range 0-23" } + require(endHour in 0..23) { "endHour must be in range 0-23" } + require(startMinute in 0..59) { "startMinute must be in range 0-59" } + require(endMinute in 0..59) { "endMinute must be in range 0-59" } + fun format(hour: Int, minute: Int): String { val period = if (hour < 12) "AM" else "PM" val hour12 = when (hour % 12) { 0 -> 12 else -> hour % 12 } return "$hour12:${minute.toString().padStart(2, '0')} $period" } return "${format(startHour, startMinute)} - ${format(endHour, endMinute)}" }For proper i18n, consider using platform-specific time formatters that respect the user's locale settings, though this may be acceptable for an initial implementation.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmp-shared/cmp_shared.podspeccore/common/src/commonMain/kotlin/org/mifos/mobile/core/common/DateHelper.ktfeature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt
🔇 Additional comments (2)
feature/settings/src/commonMain/kotlin/org/mifos/mobile/feature/settings/theme/ChangeThemeScreen.kt (2)
175-186: LGTM!The conditional dialog rendering and action wiring are correctly implemented. The dialog visibility is controlled by
uiState.showTimeBasedDialog, and the dismiss/confirm actions are properly delegated to the ViewModel.
312-349: LGTM!The
TimePickerDialogimplementation is clean and correctly wires the time picker state to the confirm callback. Using 24-hour format is appropriate for a time-based theme configuration.
revanthkumarJ
force-pushed
the
time_based_theme
branch
from
4a5e432 to
9b9878b
Compare
2 participants
Fixes - Jira-#473
Summary by CodeRabbit
Release Notes
✏️ Tip: You can customize this high-level summary in your review settings.