Conversation
📝 WalkthroughWalkthroughAdds time-based dark theme support: new TimeBasedTheme model and BASED_ON_TIME enum; DateHelper utilities; repository and datastore observables/setters; UI for selecting time ranges (TimeBasedThemeDialog) and settings wiring; AppViewModel observes and applies time-based theme updates. Also a small AppThemeExtensions branch added for BASED_ON_TIME. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SettingsUI as Settings UI
participant TimeDialog as TimeBasedThemeDialog
participant SettingsVM as SettingsViewmodel
participant Repo as UserDataRepository
participant Prefs as UserPreferencesRepository
participant AppVM as AppViewModel
participant DateHelper as DateHelper
participant Theme as AppThemeExtensions
User->>SettingsUI: Choose "Based on Time"
SettingsUI->>TimeDialog: Open time picker
User->>TimeDialog: Set start/end -> Confirm
TimeDialog->>SettingsVM: updateTimeBasedThemeConfig(timeBasedTheme)
SettingsVM->>Repo: setTimeBasedThemeConfig(timeBasedTheme)
Repo->>Prefs: setTimeBasedThemeConfig(timeBasedTheme)
Prefs->>Prefs: Persist
Prefs-->>AppVM: observeTimeBasedThemeConfig emits
AppVM->>AppVM: Dispatch TimeBasedThemeUpdate
AppVM->>DateHelper: isDarkModeBasedOnTime(start,end)
DateHelper-->>AppVM: true/false
AppVM->>Theme: Apply dark/light theme
Theme-->>User: Render updated theme
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
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: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmp-android/src/main/kotlin/cmp/android/app/AppThemeExtensions.kt (1)
14-22: Incomplete implementation for time-based dark mode.The
BASED_ON_TIMEcase currently returns hardcodedfalse, which means time-based theming will always show light mode regardless of the configured time range.The function should:
- Accept
TimeBasedThemeparameter- Call
DateHelper.isDarkModeBasedOnTime(...)with the theme's time range- Return the result of that calculation
🔎 Suggested fix
+import org.mifos.core.common.DateHelper +import org.mifos.core.model.TimeBasedTheme fun DarkThemeConfig.isDarkMode( isSystemDarkMode: Boolean, + timeBasedTheme: TimeBasedTheme? = null, ): Boolean = when (this) { DarkThemeConfig.FOLLOW_SYSTEM -> isSystemDarkMode DarkThemeConfig.DARK -> true DarkThemeConfig.LIGHT -> false - DarkThemeConfig.BASED_ON_TIME -> false + DarkThemeConfig.BASED_ON_TIME -> { + timeBasedTheme?.let { + DateHelper.isDarkModeBasedOnTime( + it.hourStart, + it.minStart, + it.hourEnd, + it.minEnd + ) + } ?: false + } }
🧹 Nitpick comments (7)
core/model/src/commonMain/kotlin/org/mifos/core/model/TimeBasedTheme.kt (1)
14-20: Consider adding validation or documentation for time value bounds.The data class accepts any
Intvalues, but hours should be 0-23 and minutes 0-59. Consider either:
- Adding
initblock validation- Documenting expected ranges in KDoc
Also, property naming could be more explicit (
minuteStart/minuteEndvsminStart/minEnd).🔎 Optional: Add validation
@Serializable data class TimeBasedTheme( val hourStart: Int, val hourEnd: Int, val minStart: Int, val minEnd: Int, -) +) { + init { + require(hourStart in 0..23) { "hourStart must be 0-23" } + require(hourEnd in 0..23) { "hourEnd must be 0-23" } + require(minStart in 0..59) { "minStart must be 0-59" } + require(minEnd in 0..59) { "minEnd must be 0-59" } + } +}feature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsViewmodel.kt (1)
60-64: Missing analytics logging for time-based theme changes.Other theme update functions (
updateThemeBrand,updateDarkThemeConfig,updateDynamicColorPreference) log analytics events, butupdateTimeBasedThemeConfigdoes not. This inconsistency may impact feature usage tracking.🔎 Add analytics logging
fun updateTimeBasedThemeConfig(timeBasedTheme: TimeBasedTheme) { viewModelScope.launch { + analyticsHelper.logTimeBasedThemeChanged(timeBasedTheme) settingsRepository.setTimeBasedThemeConfig(timeBasedTheme) } }Add the helper function at the bottom of the file:
private fun AnalyticsHelper.logTimeBasedThemeChanged(theme: TimeBasedTheme) { logEvent( type = "time_based_theme_changed", params = mapOf( "hour_start" to theme.hourStart.toString(), "hour_end" to theme.hourEnd.toString(), ), ) }feature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsScreen.kt (3)
164-168: UserememberSaveablefor time picker state to survive configuration changes.The time values use
rememberwhich will lose state on configuration changes (e.g., rotation). This could frustrate users who partially configured times.🔎 Replace remember with rememberSaveable
- var startHour by remember { mutableStateOf(initialTheme.hourStart) } - var startMinute by remember { mutableStateOf(initialTheme.minStart) } - - var endHour by remember { mutableStateOf(initialTheme.hourEnd) } - var endMinute by remember { mutableStateOf(initialTheme.minEnd) } + var startHour by rememberSaveable { mutableStateOf(initialTheme.hourStart) } + var startMinute by rememberSaveable { mutableStateOf(initialTheme.minStart) } + + var endHour by rememberSaveable { mutableStateOf(initialTheme.hourEnd) } + var endMinute by rememberSaveable { mutableStateOf(initialTheme.minEnd) }
277-314: Consider extractingTimePickerDialogto a shared design system component.A similar
TimePickerDialogexists infeature/home/src/commonMain/kotlin/org/mifos/feature/home/task/EditTaskScreen.kt(lines 331-355). Consider extracting to a shared component in the design system module to avoid duplication and ensure consistent behavior.
154-159: Addmodifierparameter to public composable for flexibility.
TimeBasedThemeDialogis a public composable but lacks amodifierparameter, which limits caller customization.🔎 Add modifier parameter
@OptIn(ExperimentalMaterial3Api::class) @Composable fun TimeBasedThemeDialog( initialTheme: TimeBasedTheme, onDismiss: () -> Unit, onConfirm: (TimeBasedTheme) -> Unit, + modifier: Modifier = Modifier, ) {Then pass it to the
AlertDialog.feature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsDialog.kt (1)
222-243: Extract complex string formatting to improve readability.The string concatenation for the time-based theme option is complex and hard to maintain. Consider extracting to a helper function.
🔎 Extract to helper function
@Composable private fun formatTimeBasedThemeLabel(theme: TimeBasedTheme): String { val basedOnTime = stringResource(Res.string.feature_settings_dark_mode_based_on_time) val darkLabel = stringResource(Res.string.feature_settings_dark_mode_label) val lightLabel = stringResource(Res.string.feature_settings_light_mode_label) val darkRange = DateHelper.formatTimeRange( theme.hourStart, theme.minStart, theme.hourEnd, theme.minEnd ) val lightRange = DateHelper.formatTimeRange( theme.hourEnd, theme.minEnd, theme.hourStart, theme.minStart ) return "$basedOnTime\n$darkLabel [$darkRange]\n$lightLabel [$lightRange]" }Then use:
SettingsDialogThemeChooserRow( text = formatTimeBasedThemeLabel(settings.timeBasedTheme), selected = settings.darkThemeConfig == DarkThemeConfig.BASED_ON_TIME, onClick = { onChangeDarkThemeConfig(DarkThemeConfig.BASED_ON_TIME) }, )cmp-navigation/src/commonMain/kotlin/cmp/navigation/AppViewModel.kt (1)
101-116: Theme won't auto-update when time passes the threshold while app is open.The time-based theme is only evaluated when
ThemeUpdateaction is dispatched (e.g., on settings change or app launch). If the user has the app open when the time threshold is crossed, the theme won't change until they navigate to settings.Consider adding a periodic check or time-based trigger to re-evaluate the theme.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txtcmp-android/src/main/kotlin/cmp/android/app/AppThemeExtensions.ktcmp-navigation/src/commonMain/kotlin/cmp/navigation/AppViewModel.ktcore/common/src/commonMain/kotlin/org/mifos/core/common/DateHelper.ktcore/data/src/commonMain/kotlin/org/mifos/core/data/repository/UserDataRepository.ktcore/data/src/commonMain/kotlin/org/mifos/core/data/repositoryImpl/UserDataRepositoryImpl.ktcore/datastore/src/commonMain/kotlin/org/mifos/core/datastore/UserPreferencesRepository.ktcore/datastore/src/commonMain/kotlin/org/mifos/core/datastore/UserPreferencesRepositoryImpl.ktcore/model/src/commonMain/kotlin/org/mifos/core/model/DarkThemeConfig.ktcore/model/src/commonMain/kotlin/org/mifos/core/model/TimeBasedTheme.ktcore/model/src/commonMain/kotlin/org/mifos/core/model/UserData.ktfeature/settings/build.gradle.ktsfeature/settings/src/commonMain/composeResources/values/strings.xmlfeature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsDialog.ktfeature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsScreen.ktfeature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsViewmodel.kt
🧰 Additional context used
🧬 Code graph analysis (3)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/AppViewModel.kt (1)
core-base/ui/src/commonMain/kotlin/template/core/base/ui/BaseViewModel.kt (1)
trySendAction(83-85)
feature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsDialog.kt (5)
feature/settings/src/desktopMain/kotlin/org/mifos/feature/settings/Platform.desktop.kt (1)
supportsDynamicTheming(13-13)feature/settings/src/jsMain/kotlin/org/mifos/feature/settings/Platform.js.kt (1)
supportsDynamicTheming(13-13)feature/settings/src/wasmJsMain/kotlin/org/mifos/feature/settings/Platform.wasmJs.kt (1)
supportsDynamicTheming(13-13)feature/settings/src/nativeMain/kotlin/org/mifos/feature/settings/Platform.native.kt (1)
supportsDynamicTheming(13-13)feature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsScreen.kt (1)
TimeBasedThemeDialog(154-244)
feature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsScreen.kt (1)
feature/home/src/commonMain/kotlin/org/mifos/feature/home/task/EditTaskScreen.kt (1)
TimePickerDialog(332-356)
⏰ 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 (11)
feature/settings/build.gradle.kts (1)
22-22: LGTM - Dependency addition aligns with time-based theming.The addition of
core.commondependency is necessary to accessDateHelperutilities for time-based theme calculations.cmp-android/dependencies/prodReleaseRuntimeClasspath.tree.txt (1)
2289-2289: Dependency tree correctly reflects core:common addition.This generated dependency tree correctly shows the new
core:commonmodule in the runtime classpath, aligning with the build configuration changes.core/model/src/commonMain/kotlin/org/mifos/core/model/DarkThemeConfig.kt (1)
16-16: LGTM - Enum value addition is well-formed.The new
BASED_ON_TIMEenum constant follows the existing pattern and will be correctly handled by thefromStringcompanion function.feature/settings/src/commonMain/composeResources/values/strings.xml (1)
34-43: LGTM - String resources support new time-based theme UI.The added string resources follow the existing naming conventions and provide necessary UI text for the time-based dark mode feature.
core/common/src/commonMain/kotlin/org/mifos/core/common/DateHelper.kt (2)
20-41: LGTM - Time-based dark mode logic is correct.The function correctly handles both same-day ranges (e.g., 06:00 → 18:00) and cross-midnight ranges (e.g., 18:00 → 06:00). The cross-midnight logic using
!in endMinutes..<startMinutesappropriately inverts the range to capture times spanning midnight.
43-59: LGTM - Time formatting functions are correct.The
formatTimeRangeandformatfunctions correctly handle 12-hour time formatting with AM/PM notation and proper handling of midnight/noon edge cases.core/data/src/commonMain/kotlin/org/mifos/core/data/repositoryImpl/UserDataRepositoryImpl.kt (1)
40-41: LGTM - Repository implementation follows existing patterns.The new
observeTimeBasedThemeConfigandsetTimeBasedThemeConfigimplementations correctly delegate to the underlying preferences repository, consistent with other configuration properties in this class.Also applies to: 56-57
core/data/src/commonMain/kotlin/org/mifos/core/data/repository/UserDataRepository.kt (1)
39-39: LGTM - Interface additions follow existing patterns.The new
observeTimeBasedThemeConfigandsetTimeBasedThemeConfiginterface methods are consistent with the existing API design for other configuration properties.Also applies to: 51-51
core/model/src/commonMain/kotlin/org/mifos/core/model/UserData.kt (1)
29-29: LGTM!The
timeBasedThemeproperty is properly added toUserDatawith sensible default values (dark mode from 18:00 to 06:00). The structure integrates well with the existing data class.Note: The
enableScreenCapturedefault changed fromfalsetotrue. Verify this is intentional as it affects the default security posture.Also applies to: 45-51
feature/settings/src/commonMain/kotlin/org/mifos/feature/settings/SettingsDialog.kt (1)
64-78: LGTM!The
SettingsDialogcorrectly wires the viewmodel to handle time-based theme updates. The separation of concerns between the overloaded composables is well-structured.cmp-navigation/src/commonMain/kotlin/cmp/navigation/AppViewModel.kt (1)
130-136: LGTM!The
TimeBasedThemeUpdateaction and handler are correctly implemented, properly updating state flow when time-based theme configuration changes.Also applies to: 181-183
core/datastore/src/commonMain/kotlin/org/mifos/core/datastore/UserPreferencesRepository.kt
Outdated
Show resolved
Hide resolved
core/datastore/src/commonMain/kotlin/org/mifos/core/datastore/UserPreferencesRepositoryImpl.kt
Outdated
Show resolved
Hide resolved
2 participants
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.