feat(home): add service visibility customization#3091
feat(home): add service visibility customization#3091biplab1 merged 32 commits intoopenMF:developmentfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis pull request introduces service selection functionality to the home screen, enabling users to toggle edit mode, selectively display services, and persist their preferences. Changes span the data persistence layer (DataSource and Repository), UI components (HomeScreen with edit and selection UI), state management (HomeViewModel with new actions and computed state), and design system (new icon resources). Changes
Sequence DiagramsequenceDiagram
participant User
participant HomeScreen
participant HomeViewModel
participant UserPreferencesRepository
participant UserPreferencesDataSource
User->>HomeScreen: Toggle Edit Mode
HomeScreen->>HomeViewModel: ToggleEditMode action
HomeViewModel->>HomeViewModel: Update isEditMode state
User->>HomeScreen: Select/Deselect Service
HomeScreen->>HomeViewModel: ToggleServiceSelection(route) action
HomeViewModel->>HomeViewModel: Toggle service in selectedServices
HomeViewModel->>HomeViewModel: Compute new visibleItems
User->>HomeScreen: Confirm Service Selection (Exit Edit)
HomeScreen->>HomeViewModel: ToggleEditMode action
HomeViewModel->>UserPreferencesRepository: setSelectedServices(selectedServices)
UserPreferencesRepository->>UserPreferencesDataSource: setSelectedServices(selectedServices)
UserPreferencesDataSource->>UserPreferencesDataSource: Save to preferences
UserPreferencesDataSource-->>UserPreferencesRepository: Success
UserPreferencesRepository-->>HomeViewModel: Success
HomeViewModel->>HomeViewModel: Update isEditMode = false
HomeViewModel-->>HomeScreen: Updated state
HomeScreen->>User: Display updated services
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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🧪 Generate unit tests (beta)
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)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
12-28: Add the missingclickableimport to prevent a compile error.
Modifier.clickableis used at lines 254 and 344 but the corresponding import fromandroidx.compose.foundationis missing.🐛 Add the missing import
import androidx.compose.foundation.Image +import androidx.compose.foundation.clickable import androidx.compose.foundation.border
🤖 Fix all issues with AI agents
In
`@core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt`:
- Around line 239-253: The direct getters/setters currently use
SELECTED_SERVICES_KEY and diverge from the APP_SETTINGS path used by
setSelectedServices, causing stale state; change saveSelectedServicesDirectly
and getSelectedServicesDirectly to read/write the same SettingsPreference via
settings.getSettingsPreference() and settings.putSettingsPreference(...) (i.e.,
build a newPreference = settings.getSettingsPreference().copy(selectedServices =
services) when saving) and ensure they also update _settingsInfo.value to the
new preference so all callers share a single source of truth; reference
setSelectedServices, saveSelectedServicesDirectly, getSelectedServicesDirectly,
_settingsInfo, settings.getSettingsPreference and settings.putSettingsPreference
when making the change.
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Around line 370-373: Replace the hard-coded contentDescription "Selected" in
the Icon usage (the Icon with imageVector = MifosIcons.CheckCircle1 inside
HomeScreen.kt) with a localized stringResource; add a new string key
feature_home_selected to
feature/home/src/commonMain/composeResources/values/strings.xml (and localized
variants) and reference it via stringResource(Res.string.feature_home_selected)
for accessibility/i18n.
- Around line 248-251: The Icon's contentDescription is hard-coded; create a
localized string resource named "feature_home_edit_services" and replace the
literal "Edit services" with a lookup to that resource so the Icon uses the
localized text based on isEditMode (i.e., keep the Icon call using imageVector =
if (isEditMode) MifosIcons.Edit else MifosIcons.GridApps but set
contentDescription to the localized string obtained from resources), ensuring
the same resource is used for accessibility in HomeScreen.
🧹 Nitpick comments (3)
core/designsystem/src/commonMain/kotlin/org/mifos/mobile/core/designsystem/icon/MifosIcons.kt (1)
270-272: Prefer descriptive aliases overCheckCircle1/Pencil.These overlap existing
CheckCircleandEditaliases. Consider renaming to clarify intent (or reuse the existing names) to avoid confusion for consumers.♻️ Suggested renames (update call sites accordingly)
- val CheckCircle1 = FluentIcons.Filled.CheckmarkCircle - val Pencil = Icons.Filled.Edit + val CheckCircleFilled = FluentIcons.Filled.CheckmarkCircle + val EditFilled = Icons.Filled.Editcore/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt (1)
35-35: Consider refactoring instead of a broad suppression (optional).If feasible, splitting responsibilities would avoid the
TooManyFunctionssuppression and improve cohesion.feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
294-295: Consider an empty-state when no services are selected.When
selectedServicesis empty, the non-edit view renders nothing. A lightweight empty-state or fallback could prevent a blank UI.
...datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
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)
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/di/PreferenceModule.kt (1)
20-28: ConflictingSettingsbindings in Koin modules must be resolved.Two different scopes define
Settings:
PreferencesModule(core/datastore):single<Settings>PasscodePreferenceModule(libs/mifos-passcode):factory<Settings>Both modules are loaded together in the production app via
KoinModules.kt, causing Koin to fail or override bindings. Align both to use the same scope (recommendsingle<Settings>for shared application state), or refactor to prevent both from being loaded simultaneously.
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Around line 241-258: The Icon currently uses Modifier.size(16.dp).clickable {
toggleEditMode() } which yields a 16.dp touch target; replace this raw Icon
click handling with a Material IconButton to provide the standard 48dp
accessible touch target: wrap the Icon in an IconButton (or use IconButton
composable) and move the toggleEditMode() click handler to the IconButton's
onClick, keeping the same imageVector selection (if (isEditMode) MifosIcons.Edit
else MifosIcons.GridApps), contentDescription, tint, and size for the visual
icon while allowing the IconButton to ensure proper padding and accessibility in
the Row where Text and Icon are defined.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
Aditya002500
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Around line 242-259: The accessibility label currently always says "Edit
services" even when isEditMode is true; change the Icon's contentDescription to
be conditional on isEditMode (e.g., use a "Done/Save services" or "Exit edit
mode" string resource when isEditMode is true, and "Edit services" when false)
so the IconButton(toggleEditMode) accurately describes the action; update the
string resource keys if needed and reference isEditMode, toggleEditMode(),
IconButton and Icon in HomeScreen.kt when making the change.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
amanna13
left a comment
amanna13
left a comment
There was a problem hiding this comment.
Hey @Aditya002500 ! I have left few comments here. Please have a look
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Around line 147-151: toggleEditMode currently calls the blocking, non-suspend
preferencesRepository.saveSelectedServices(...) on the main thread; wrap that
call in a coroutine launched on a background dispatcher (e.g., Dispatchers.IO or
an injected/background CoroutineScope) so the synchronous SharedPreferences
write runs off the UI thread, while keeping any UI state updates (toggling
isEditMode) on the main/Main thread; locate the toggleEditMode function and
replace the direct call to
preferencesRepository.saveSelectedServices(selectedServices) with a coroutine
launch that calls that method on an IO/background scope and optionally handles
errors.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/di/PreferenceModule.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt (1)
161-171:⚠️ Potential issue | 🟡 MinorGood fix to sync in-memory
_userInfoon clear, butSELECTED_SERVICES_KEYis not cleared.Adding
_userInfo.value = UserData.DEFAULT(Line 164) is a solid fix to keep in-memory state consistent. However,clearInfodoesn't removeSELECTED_SERVICES_KEY— so a logged-out user's service selection persists across sessions. If the dual-storage path is consolidated intoAppSettings(per comments above), this is automatically handled by the existingclearedcopy on Line 165. Otherwise, addsettings.remove(SELECTED_SERVICES_KEY).feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (2)
343-392:⚠️ Potential issue | 🟡 MinorTap target excludes the service label text.
The
clickablemodifier is on the innerBox(Line 349) which wraps only the icon image. TheTextlabel (Line 386) sits outside thatBox, so tapping the label does nothing. Move theclickableto the outerColumn(or wrap both in a single clickable surface) for a more intuitive hit area.Proposed fix
Column( - modifier = modifier.padding(vertical = KptTheme.spacing.sm), + modifier = modifier + .padding(vertical = KptTheme.spacing.sm) + .clickable { onClick() }, verticalArrangement = Arrangement.spacedBy(KptTheme.spacing.sm), horizontalAlignment = Alignment.CenterHorizontally, ) { Box( - modifier = Modifier.clickable { onClick() }, + modifier = Modifier, ) {
443-456:⚠️ Potential issue | 🟠 Major
@Previewwill crash —koinInject()requires a Koin context.
HomeContentnow callskoinInject<UserPreferencesRepository>()(Line 140), but@Previewcomposables don't have a Koin dependency graph initialized. This will throw at preview render time.This is another reason to move the repository interaction into
HomeViewModel(as suggested above) — the preview only needs aHomeState, not a live DI container.
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Around line 299-300: When not in edit mode the filtered displayItems can be
empty (selectedServices empty) leaving users without guidance; update the
HomeScreen UI logic to detect when displayItems.isEmpty() and render an
empty-state placeholder message or composable (e.g., "Tap the grid icon to add
services") instead of the grid rows. Locate the displayItems/rows calculation
and the composable that renders chunked rows (references: displayItems, rows,
columnCount, isEditMode, selectedServices) and add a conditional branch to show
the placeholder when displayItems.isEmpty() and !isEditMode.
- Around line 150-158: The toggleEditMode currently launches a coroutine to call
preferencesRepository.saveSelectedServices but flips isEditMode immediately, so
save failures are ignored; change toggleEditMode so that when isEditMode is true
you perform the save inside scope.launch and only set isEditMode = false after a
successful save, catching exceptions from
preferencesRepository.saveSelectedServices (use try/catch or runCatching) and
handle failures by logging and surfacing an error to the user (e.g., snackbar or
error state) instead of toggling off; keep the immediate toggle behavior when
entering edit mode (when isEditMode is false) outside the save path. Use the
existing scope, toggleEditMode, isEditMode and
preferencesRepository.saveSelectedServices symbols to locate and update the
logic.
🧹 Nitpick comments (3)
core/datastore/src/commonMain/kotlin/org/mifos/mobile/core/datastore/UserPreferencesDataSource.kt (1)
35-35: Consider splittingUserPreferencesDataSourcerather than suppressing the lint.The class is growing (20+ public methods). A future refactor could extract service-selection concerns into a dedicated data source (e.g.,
ServiceSelectionDataSource), keeping each class focused.feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (2)
140-158: Service selection state is managed in the Composable, bypassing the ViewModel/MVI pattern.The rest of
HomeScreenfollows a clean MVI pattern:HomeViewModel→HomeState→HomeAction→HomeEvent. But service selection is handled entirely in the Composable viakoinInject(), localmutableStateOf, and arememberCoroutineScope. This creates two separate state management approaches in the same screen.Consider moving
selectedServices,isEditMode, and the toggle/persistence logic intoHomeViewModeland exposing them throughHomeState/HomeAction. This would:
- Keep a single source of truth for screen state
- Make the logic testable without Compose
- Align with the existing architecture
380-380: Hardcoded4.dp— use a design token for consistency.The rest of the file uses
DesignTokenandKptThemespacing values. Replace4.dpwith the appropriate token (e.g.,KptTheme.spacing.xsorDesignToken.spacing.extraSmall).
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.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/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
343-392:⚠️ Potential issue | 🟡 MinorService text label is not part of the tap target.
The
clickablemodifier is on the innerBox(Line 349) that wraps only the icon image. TheTextlabel (Line 386) sits outside thisBoxin the parentColumn, so tapping the service name does nothing. Users will likely expect the entire card (icon + label) to be tappable.Move the
clickablemodifier up to theColumn(or wrap the whole card in a single clickable surface):Proposed fix
Column( - modifier = modifier.padding(vertical = KptTheme.spacing.sm), + modifier = modifier + .clickable { onClick() } + .padding(vertical = KptTheme.spacing.sm), verticalArrangement = Arrangement.spacedBy(KptTheme.spacing.sm), horizontalAlignment = Alignment.CenterHorizontally, ) { Box( - modifier = Modifier.clickable { onClick() }, + modifier = Modifier, ) {
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Around line 140-158: Move the service-selection state and persistence out of
the composable into HomeViewModel: stop calling koinInject() in
HomeScreen/HomeContent, inject UserPreferencesRepository into HomeViewModel
instead, expose selectedServices and isEditMode as StateFlow (or
MutableStateFlow) and provide a toggleEditMode() method that handles loading,
mutating and saving via coroutine scope inside the ViewModel; update
HomeScreen/HomeContent to consume those StateFlows and call
viewModel.toggleEditMode(), ensuring previews/tests can construct the composable
without Koin by passing mocked/empty state flows or a preview HomeViewModel.
🧹 Nitpick comments (1)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
378-381: Minor: prefer design token over hard-coded4.dp.Line 380 uses a literal
4.dpwhile the rest of the file consistently usesDesignToken.*orKptTheme.spacing.*. Consider using the appropriate token for consistency.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.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/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
425-438:⚠️ Potential issue | 🟡 MinorPreview shows empty-state hint instead of service cards.
With the new defaults (
selectedServices = emptySet(),isEditMode = false), the preview will render the "Tap the grid icon to add services" hint instead of actual service cards, sincedisplayItemswill be empty. Consider populatingselectedServicesso the preview is representative of normal usage.Suggested adjustment
HomeContent( state = HomeState( dialogState = null, items = serviceCards, uiState = HomeScreenState.Success, + selectedServices = serviceCards.map { it.route }.toSet(), ), onAction = {}, modifier = Modifier, )
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt`:
- Around line 248-264: handleToggleEditMode currently catches Exception which
swallows CancellationException and also silently discards persisted selections
on save failure; change the try/catch to rethrow CancellationException (or only
catch specific save-related exceptions) around the call to
userPreferencesRepositoryImpl.saveSelectedServices(state.selectedServices), and
on any non-cancellation save failure restore the UI selection by resetting
state.selectedServices to the last persisted value (obtainable from
userPreferencesRepositoryImpl.getSelectedServices() or your repository's
equivalent) before calling updateState { it.copy(isEditMode = false) } so
cancellation propagates correctly and the in-memory selections remain consistent
with storage.
🧹 Nitpick comments (3)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (2)
330-331: Consider adding a semanticrolefor accessibility.The
Boxuses bare.clickable { onClick() }without a semanticrole. Screen readers won't announce this as an interactive element. The codebase already has aModifier.onClick(...)extension (inModifierExt.kt) that accepts aroleparameter — using it withRole.Buttonwould improve accessibility.Suggested adjustment
Box( - modifier = Modifier.clickable { onClick() }, + modifier = Modifier.clickable(role = Role.Button) { onClick() }, ) {
360-363: Hard-coded4.dp— prefer a design token for consistency.The rest of the file consistently uses
DesignToken.*andKptTheme.spacing.*for spacing values. This hard-coded4.dpis a minor inconsistency.feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (1)
499-501: Remove the unusedallServiceRoutesstate field.
allServiceRoutesis populated inloadSavedServices(line 238) but is never read anywhere in the codebase—not inHomeScreen.ktor elsewhere in the ViewModel. Removing it will reduce state complexity without affecting functionality.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt`:
- Around line 326-367: The clickable Box with role=Role.Button lacks an
accessible label because Image has contentDescription=null and the service title
Text sits outside the touch target; update the accessibility by either moving
the clickable modifier to the parent Column (so the onClick on Column encloses
both Image and the title Text) or add an explicit spoken label via the
clickable's onClickLabel and/or provide a non-null contentDescription on the
Image (use stringResource for the service title or a dedicated label string).
Locate the clickable modifier in the Box and the Image(contentDescription), and
change one of them so the button exposes a descriptive label to screen readers
(e.g., use Column.clickable or clickable(onClickLabel = stringResource(...))
and/or remove contentDescription=null and supply the title string).
🧹 Nitpick comments (3)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
426-439: Preview shows empty state instead of service cards.
HomeStatedefaultsselectedServicestoemptySet(), so the preview renders only the "no services" hint rather than the service grid. PopulateselectedServicesto make the preview representative:state = HomeState( dialogState = null, items = serviceCards, uiState = HomeScreenState.Success, + selectedServices = serviceCards.map { it.route }.toSet(), ),feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (2)
49-49: Misleading variable name:userPreferencesRepositoryImplis typed as the interface.The constructor parameter is typed
UserPreferencesRepository(the interface), but named...Impl. This is confusing — theImplsuffix conventionally indicates a concrete implementation. Consider renaming touserPreferencesRepositoryfor consistency with the type.- private val userPreferencesRepositoryImpl: UserPreferencesRepository, + private val userPreferencesRepository: UserPreferencesRepository,
230-242: ExtractallRoutesto avoid duplicated computation.
serviceCards.map { it.route }.toSet()appears both here (Line 235) and in the catch block ofhandleToggleEditMode(Line 262). Extract it once:Suggested refactor
+ private val allServiceRoutes = serviceCards.map { it.route }.toSet() + private fun loadSavedServices() { - val allRoutes = serviceCards.map { it.route }.toSet() val savedServices = userPreferencesRepositoryImpl.selectedServices updateState { it.copy( - selectedServices = savedServices ?: allRoutes, + selectedServices = savedServices ?: allServiceRoutes, ) } }And in
handleToggleEditMode:- selectedServices = lastSaved ?: serviceCards.map { it.route }.toSet(), + selectedServices = lastSaved ?: allServiceRoutes,
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Some minor changes suggested.
I have a suggestion: the user shouldn't be allowed to hide all the services. At least one service should remain visible.
You may discuss it with @therajanmaurya during the standup meeting.
Looks good to me. This can be merged.
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt`:
- Around line 305-317: The toggle logic in handleToggleServiceSelection reads
state.selectedServices outside updateState, causing a race where rapid taps can
clobber toggles; move the computation of newSelection and visibleItems inside
the updateState lambda so you operate on the latest state snapshot (use the
lambda parameter e.g. it.selectedServices), then set selectedServices to the
computed newSelection and visibleItems to computeVisibleItems(selectedServices =
newSelection) within the same updateState call.
- Around line 63-65: The call to loadSavedServices() in HomeViewModel's init is
doing synchronous disk I/O via userPreferencesRepositoryImpl.selectedServices ->
getSelectedServicesDirectly() -> settings.getStringOrNull(); wrap the
loadSavedServices() invocation inside viewModelScope.launch(Dispatchers.IO) (or
launch { withContext(Dispatchers.IO) { ... } }) so the disk read runs off the
main thread, leaving saveSelectedServices as-is (it already uses
viewModelScope.launch); update references to loadSavedServices() in the init
block to call it from that coroutine context.
🧹 Nitpick comments (4)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (2)
321-322: DefaultisSelected = trueis a misleading default.Defaulting to
truemeans any call site that omitsisSelectedwill render the card in a "selected" visual state. A safer default isfalseto avoid accidental visual misrepresentation.Suggested fix
- isSelected: Boolean = true, + isSelected: Boolean = false,
424-437: Preview shows all items as unselected.
selectedServicesdefaults toemptySet()inHomeState, so the preview renders everyServiceItemCardwithisSelected = false(non-primary border/tint). PassselectedServicesexplicitly to show a realistic preview:Suggested fix
HomeContent( state = HomeState( dialogState = null, items = serviceCards, uiState = HomeScreenState.Success, + selectedServices = serviceCards.map { it.route }.toSet(), ), onAction = {}, modifier = Modifier, )feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt (2)
245-248: Same stale-state pattern inhandleAmountVisible.
state.isAmountVisibleis read outside the lambda's perspective — this uses the capturedstateproperty rather than theitparameter provided byMutableStateFlow.update. While this is pre-existing, the same pattern was introduced in the new toggle code. Consider fixing both:private fun handleAmountVisible() { updateState { - it.copy(isAmountVisible = !state.isAmountVisible) + it.copy(isAmountVisible = !it.isAmountVisible) } }
231-240:computeVisibleItemsreadsstateby default — fragile when called insideupdateState.The default parameter values (
state.isEditMode,state.selectedServices) capture a potentially stale snapshot when this function is invoked from within anupdateStatelambda. Callers insideupdateStatemust always pass explicit parameters — but nothing enforces this. Consider removing the defaults to make callers explicit, or passing the full state:Option: remove defaults to enforce explicit call sites
private fun computeVisibleItems( - isEditMode: Boolean = state.isEditMode, - selectedServices: Set<String> = state.selectedServices, + isEditMode: Boolean, + selectedServices: Set<String>, ): ImmutableList<ServiceItem> {
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt
Show resolved
Hide resolved
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeViewModel.kt
Show resolved
Hide resolved
|
@mena-rizkalla Please review the updates. |
Sir i have not implemented anything new i just reverted to the original state |
Hi @Aditya002500 why you revert doing the filter in viewmodel ? |
Actually I was facing some issues in the implementation, and there is some stuff to discuss with Rajan Sir , so if another fix is recommended by sir , I will implement this together |
mena-rizkalla
left a comment
mena-rizkalla
left a comment
There was a problem hiding this comment.
LGTM!
Moving the filtering logic to the ViewModel is a solid improvement and ensures a single source of truth for the UI state.
4 participants
Fixes - MM-505
|
WhatsApp.Video.2026-02-10.at.22.07.42.mp4
|
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
Release Notes