feat(feature): Implemented Client Filter functionality #2553
Conversation
WalkthroughAdds a client-list filtering feature: UI for sort/status/office (FilterBottomSheet), ViewModel support for filter state and unfiltered data/flows, and platform updates to pass Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant UI as FilterBottomSheet
participant Screen as ClientListScreen
participant VM as ClientListViewModel
participant Data as DataLayer
User->>Screen: Open filter sheet (toggle)
Screen->>VM: ToggleFilterVisibility
VM-->>Screen: isFilterVisible updated
User->>UI: Choose sort/status/offices -> Apply
UI->>Screen: Dispatch HandleSortClick / HandleFilterClick
Screen->>VM: Dispatch actions
VM->>VM: update sort/selectedStatus/selectedOffices
VM->>Data: derive filtered list & paging from unfilteredClients / unfilteredClientsFlow
Data-->>VM: filtered results (list + PagingData)
VM-->>Screen: Emit clients / clientsFlow (updated)
Screen->>User: Re-rendered client list (sorted/filtered)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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: 2
🧹 Nitpick comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (1)
83-91: Filter updates (status/office) work but can accumulate duplicates
addStatus/addOfficeappend toselectedStatus/selectedOfficeswithout checking for existing entries, so repeated taps of the same checkbox over time (or future callers) could accumulate duplicates even though current UI logic avoids it in normal flows. This isn’t breaking, but you could defensively change the updates to useunion/distinct()to keep the selections unique and simpler to reason about.Consider updating these setters to something like
selectedStatus = (it.selectedStatus + status).distinct()and similarly for offices, and then verify via UI tests that toggling checkboxes still behaves as expected.Also applies to: 195-214, 249-267
feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt (1)
1-20: iOS stub is structurally correct but still needs real implementationThe iOS
LazyColumnForClientListApicorrectly mirrors the commonexpectsignature (includingsortandonUpdateOffices), so it will compile cleanly, but it’s still a no-op stub and won’t render any client list on iOS.Once you prioritize iOS, you’ll want an implementation paralleling the Android behavior (paging, sorting, office extraction); I can help sketch that out when you’re ready.
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (1)
138-146: Duplicate bottom padding on “no more clients” messageThe
Textcomposable applies.padding(bottom = DesignToken.padding.extraExtraLarge)twice, which isn’t harmful but is redundant.You can simplify this to a single padding call for clarity and to avoid confusion for future maintainers.
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (2)
247-248: Empty-state and list API updates are fine but rely on hard-coded textThe
MifosEmptyCard("No clients found")and the updatedLazyColumnForClientListApiexpect/actual signatures are consistent with the rest of the code, but"No clients found"is a new hard-coded string. For a localized app, this should ideally come from resources like other user-facing text.Consider adding a string resource for the empty-clients message (and using it here) so that future locales don’t miss this new text.
Also applies to: 351-361
363-589: FilterBottomSheet uses hard-coded labels; consider moving to string resourcesThe bottom sheet currently hard-codes user-facing labels such as
"Filters","Sort by","Account Status","Office Name", and the specific sort/status values, which bypasses your existing resource-based i18n pattern (stringResource(Res.string.… )). While this works functionally, it will be harder to localize and maintain.I’d recommend introducing corresponding string resources (and, if needed, enum-like definitions for the sort keys) and wiring them into
FilterBottomSheet, then verifying resource usage matches whatever is asserted in your design/UX guidelines.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt(3 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt(8 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt(8 hunks)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt(1 hunks)feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (7)
toggleFilterVisibility(233-239)clearFilters(295-305)handleSortClick(216-230)addStatus(196-204)removeStatus(206-214)addOffice(249-257)removeOffice(259-267)feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (1)
LazyColumnForClientListApi(37-153)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt (1)
LazyColumnForClientListApi(18-29)feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt (1)
LazyColumnForClientListApi(9-20)feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.native.kt (1)
LazyColumnForClientListApi(18-27)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-46)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
ClientItem(277-328)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosPagingAppendProgress(104-120)
🔇 Additional comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (2)
16-45: State extensions for unfiltered data and filter UI look consistentThe additions of
unfilteredClients,unfilteredClientsFlow, and the new filter-related fields onClientListStateline up with how DB vs API paths are handled and give you a clear separation between source data and filtered views; no functional issues stand out here.Please double-check via manual testing that initial load for both DB and API paths correctly populates
unfilteredClients/unfilteredClientsFlowand thatclearFilters()reliably restores the original lists in all cases.Also applies to: 308-327
156-173: The code already applies sorting withinapplyFilters()at line 306 viaapplySort(state.sort, filteredClients), contradicting the core claim that filters ignorestate.sort. This centralized sort application means the described divergence between DB and API paths—where sorting would be lost after filtering—is not occurring in the current implementation. TheapplySortfunction properly handles sort types and is consistently called during filter operations, ensuring sort selection is preserved across filter changes.Likely an incorrect or invalid review comment.
feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt (1)
18-28: Desktop actual signature matches common expectAdding
sort: String?andonUpdateOffices: (List<String?>) -> Unitkeeps the desktop actual aligned with the commonexpectdeclaration; behavior is unchanged and safe until a real desktop implementation is added.When you later implement the desktop body, ensure you also honor
sortand callonUpdateOfficesconsistently with the Android/iOS behavior.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
69-118: Filter bottom sheet wiring into ClientListScreen looks solidThe integration from the filter icon (
ClientActions) throughtoggleFilterVisibility,FilterBottomSheet, and into the variousClientListActioncallbacks (status/office add/remove, sort, clear) is consistent and matches the ViewModel API. Passingsort = state.sortandonUpdateOfficesdown toLazyColumnForClientListApialso aligns with the platform-specific implementations.It’s worth manually testing: (1) opening/closing the sheet, (2) combining status + office filters, and (3) sorting while on the paged (API) path to confirm all actions update the list as expected.
Also applies to: 195-245
| val clientPagingList = pagingFlow.collectAsLazyPagingItems() | ||
|
|
||
| val items = clientPagingList.itemSnapshotList.items | ||
| if (items.isNotEmpty()) { | ||
| val offices = items.map{ it.officeName } | ||
| .distinct() | ||
| onUpdateOffices(offices) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "ClientListScreen.android.kt" -o -name "ClientListScreen*" | grep -i androidRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
fd "ClientListScreen" --type f -e ktRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
rg "fun ClientListScreen" --type kt -B 5 -A 50Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
rg "onUpdateOffices" --type kt -lRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
find . -type f -name "ClientListScreen.android.kt" 2>/dev/nullRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
git ls-files | grep -i "clientlist" | head -20Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
cat feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.ktRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
find . -type f -name "*.kt" -path "*clientsList*" 2>/dev/nullRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
git ls-files | head -30Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
ls -la feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ 2>/dev/null || find . -type f -name "*ClientList*" 2>/dev/null | head -20Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
rg "ClientListScreen" --type kt | head -20Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
find . -type f -name "*.kt" | wc -lRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
rg "onUpdateOffices" --type kt -B 5 -A 5Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
rg "itemSnapshotList" --type kt -B 10 -A 10Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
find . \( -name "*.kt" -o -name "*.kts" \) -type f 2>/dev/null | xargs rg -l "collectAsLazyPagingItems" 2>/dev/nullRepository: openMF/android-client
Length of output: 186
🏁 Script executed:
ls -la . 2>&1 | head -30Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
git ls-files | grep -E "\.kt$" | head -30Repository: openMF/android-client
Length of output: 186
🏁 Script executed:
cat feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt 2>&1 | head -100Repository: openMF/android-client
Length of output: 186
🌐 Web query:
Jetpack Compose LaunchedEffect vs direct state update in composable body recomposition loop
💡 Result:
Short answer
- Don’t update Compose state directly from the composable body as part of composition — that creates a recomposition loop (state change -> recomposition -> state change -> …). Use side-effect APIs instead. (developer.android.com)
- LaunchedEffect runs its suspend block in a Compose-aware coroutine started after composition and scoped to the composable; it avoids doing side effects during composition and won’t re-run unless its key changes. Use it for long‑running or suspend work that must not run on every recomposition. (developer.android.com)
- If you need the LaunchedEffect coroutine to see up‑to‑date values without restarting the effect, wrap those values with rememberUpdatedState and read that inside LaunchedEffect. That gives the latest value without relaunching. (composables.com)
Practical patterns
- Simple UI-only updates (button clicks, local counters): update state from event handlers (onClick) or remember/rememberSaveable. Avoid changing state unconditionally during composition. (rezaramesh.medium.com)
- One-shot async work when entering screen: LaunchedEffect(Unit) { … } and use rememberUpdatedState for callbacks/values you want to keep current. (kodeco.com)
- Publish Compose state to outside systems after recomposition: SideEffect { … } (executes after successful recomposition). (developer.android.com)
If you want, paste the code snippet that’s causing the loop and I’ll point exactly how to fix it.
Citations:
- 1: https://developer.android.com/develop/ui/compose/side-effects?utm_source=openai
- 2: https://developer.android.com/develop/ui/compose/side-effects?utm_source=openai
- 3: https://composables.com/docs/androidx.compose.runtime/runtime/composable-functions/rememberUpdatedState?utm_source=openai
- 4: https://rezaramesh.medium.com/state-management-in-jetpack-compose-best-practices-and-common-pitfalls-c9b4c6ea0949?utm_source=openai
- 5: https://www.kodeco.com/books/jetpack-compose-by-tutorials/v2.0/chapters/11-reacting-to-compose-lifecycle?utm_source=openai
- 6: https://developer.android.com/develop/ui/compose/side-effects?utm_source=openai
🌐 Web query:
Kotlin Flow collectAsLazyPagingItems recomposition behavior itemSnapshotList
💡 Result:
Short answer
- collectAsLazyPagingItems() exposes a LazyPagingItems whose items are held in an immutable ItemSnapshotList. Compose will recompose when you read properties that are backed by observable state (e.g. itemCount, indexing into LazyPagingItems inside LazyColumn.items), because Paging produces new ItemSnapshotList instances when the presented items change. [1][2]
Important details & gotchas
- ItemSnapshotList is immutable and replaced wholesale on updates; equality is by reference, so mutating some external collection won’t trigger updates — Paging emits a new PagingData which produces a new ItemSnapshotList to notify observers. [1]
- Calling collectAsLazyPagingItems() in composition is fine, but avoid recreating the Paging flow each recomposition (use remember/cachedIn in the ViewModel). Creating a new Pager/flow each recomposition causes extra emissions and UI blinking. [2][3]
- Accessing lazyPagingItems.itemSnapshotList directly is a snapshot (not a live mutable list). If you need to react to changes outside of LazyList scopes, observe changes via itemCount / indexing inside LazyColumn, or convert the snapshot into observable Compose state (e.g. snapshotFlow { lazyPagingItems.itemSnapshotList } or derivedStateOf) so recomposition happens predictably. [1][2]
Practical patterns
- In LazyColumn: use items(count = lazy.itemCount) { index -> val it = lazy[index] ... } — this is the recommended, observable pattern. [1]
- If you must read the whole list elsewhere, use snapshotFlow { lazy.itemSnapshotList } or snapshotFlow { lazy.itemCount } and collectAsState/collect to drive Compose state and recomposition. [1][2]
Sources
[1] AndroidX paging-compose LazyPagingItems API (itemSnapshotList, itemCount).
[2] StackOverflow discussion: collectAsLazyPagingItems/itemCount behavior and why it fires even when empty.
[3] StackOverflow / answers recommending remember + cachedIn to avoid recreating Pager on recomposition.
🌐 Web query:
Compose side effects LaunchedEffect preventing recomposition loops state updates
💡 Result:
Brief guidance and patterns to prevent recomposition loops when using LaunchedEffect in Jetpack Compose:
Root causes
- Using the wrong key (e.g., the mutable State object instead of its value) so LaunchedEffect restarts on every recomposition. [1][3]
- Updating Compose state inside the effect unconditionally so the effect (or the composable that owns the state) immediately recomposes and retriggers the effect. [1][2]
Fixes (concise patterns)
- Use primitive/immutable keys (or the actual value) for LaunchedEffect
- Wrong: LaunchedEffect(someState) — this watches the State object reference.
- Right: LaunchedEffect(someState.value) or LaunchedEffect(id, otherPrimitive). [1][3]
- Guard state updates inside the effect
- Only set state when it actually needs to change:
if (newValue != state.value) state.value = newValue
This avoids write->recompose->write loops. [2]
- Use rememberUpdatedState for long‑lived effects that should not restart when a value changes
- Capture up-to-date lambdas/values without restarting the effect:
val currentOnDone by rememberUpdatedState(onDone)
LaunchedEffect(Unit) { /* use currentOnDone() */ }.
(Docs warn about using constant keys carefully.) [1]
- Use snapshotFlow / distinctUntilChanged to observe state changes safely
- Convert frequent state updates into a flow and collect distinct changes:
LaunchedEffect(key1 = someKey) {
snapshotFlow { uiState.someField }
.distinctUntilChanged()
.collect { value -> /* handle value once */ }
}
This prevents reacting to intermediate identical values. [1]
- Move side effects to ViewModel / outside composition for long‑running tasks
- Launch coroutines in ViewModelScope and expose UI state; the composable only observes state (one‑source‑of‑truth). This reduces effect complexity and accidental loops. [1][4]
- Use derivedStateOf to reduce recompositions when inputs change frequently
- Compute derived values so recomposition only occurs when the derived result actually changes. [1]
Minimal example combining best practices:
@Composable
fun MyScreen(viewModel: MyVm = viewModel()) {
val status by viewModel.status // State<T>
val currentOnFinish by rememberUpdatedState(viewModel::onFinish)
LaunchedEffect(status) {
if (status == Status.Ready) {
// guard before writing
currentOnFinish()
}
}
}
Or use snapshotFlow to avoid extra restarts:
LaunchedEffect(Unit) {
snapshotFlow { viewModel.status }
.distinctUntilChanged()
.collect { status ->
if (status == Status.Ready) viewModel.onFinish()
}
}
Sources
- Android Compose side‑effects / rememberUpdatedState / produceState / derivedStateOf docs. [1]
- StackOverflow example showing nav call causing infinite recomposition and using LaunchedEffect to fix. [2]
- Compose best practices (derivedStateOf, keys, avoiding unnecessary recomposition). [3]
References
[1] Android Developers — Side‑effects in Compose (LaunchedEffect, rememberUpdatedState, snapshotFlow, produceState, derivedStateOf).
[2] StackOverflow — infinite loop of recomposition due to navigation call; use LaunchedEffect.
[3] Android Developers — Compose performance / best practices (derivedStateOf, keys).
Move onUpdateOffices into LaunchedEffect to prevent recomposition loop
Calling onUpdateOffices(offices) directly in the composable body triggers a state update in the ViewModel, which causes recomposition, which recalculates offices, which triggers the callback again—creating a feedback loop. This violates Compose's core rule: side effects that modify state must not run during composition.
Move this into a keyed side effect:
val offices = clientPagingList.itemSnapshotList.items
.map { it.officeName }
.distinct()
LaunchedEffect(offices) {
if (offices.isNotEmpty()) onUpdateOffices(offices)
}Additionally, update the ViewModel's onUpdateOffices to only emit state when the new office list differs from the current value, preventing redundant emissions.
| if (sort!=null) { | ||
| val currentItems = clientPagingList.itemSnapshotList.items | ||
|
|
||
| val sortedItems = when (sort) { | ||
| "Name" -> { currentItems.sortedBy { it.displayName?.lowercase() } } | ||
| "Account Number" -> { currentItems.sortedBy { it.accountNo } } | ||
| "External ID" -> { currentItems.sortedBy { it.externalId } } | ||
| else -> currentItems | ||
| } | ||
|
|
||
|
|
||
| LazyColumn( | ||
| modifier = modifier, | ||
| ) { | ||
| items( | ||
| items = sortedItems, | ||
| key = { client-> client.id } | ||
| ) { client -> | ||
| LaunchedEffect(client.id) { | ||
| fetchImage(client.id) | ||
| } | ||
| ClientItem( | ||
| client = client, | ||
| byteArray = images[client.id], | ||
| onClientClick = onClientSelect, | ||
| onClientClick = onClientSelect | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorted list path drops paging append error/progress UX
In the if (sort != null) branch you render LazyColumn with items(sortedItems) but never handle clientPagingList.loadState.append, so users won’t see the “load more” progress indicator, append errors, or the “no more clients available” message while sorting is active. The unsorted path still has the previous UX.
Consider factoring the append-state footer into a shared helper and using it in both branches (e.g., always render items(sortedItems) but then add an extra item { …append UI… } that inspects clientPagingList.loadState.append), and verify that sorted and unsorted modes behave identically with regard to pagination feedback.
Also applies to: 118-151
🤖 Prompt for AI Agents
In
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt
around lines 70-97 (also apply same change to 118-151), the sorted branch
renders LazyColumn over sortedItems but omits handling
clientPagingList.loadState.append, losing “load more” progress/error/no-more UI;
factor the append-state footer into a shared helper and include it in both
branches by appending a final composable item { /* footer */ } that reads
clientPagingList.loadState.append and renders the progress indicator, retry on
error, or empty/no-more message accordingly so sorted and unsorted modes show
identical pagination feedback.
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (2)
69-101: Sorted path still missing pagination append UI.This issue was previously identified. The sorted rendering path (lines 69-101) doesn't handle
clientPagingList.loadState.append, so users lose the "load more" progress indicator, append errors, and "no more clients" message while sorting is active.As suggested in the previous review, factor the append-state footer into a shared composable and include it in both branches.
48-55: Recomposition loop:onUpdateOfficesstill called in composable body.This issue was previously identified. Calling
onUpdateOffices(offices)directly during composition triggers a state update in the ViewModel, causing recomposition, which recalculatesofficesand triggers the callback again.Move this into a
LaunchedEffectas suggested in the previous review:val offices = clientPagingList.itemSnapshotList.items .map { it.officeName } .distinct() LaunchedEffect(offices) { if (offices.isNotEmpty()) onUpdateOffices(offices) }
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
360-586: Consider extracting filter sections into separate composables.
FilterBottomSheetis 227 lines with repeated patterns for each filter section (Sort by, Account Status, Office Name). Extract these into reusable components likeExpandableFilterSectionto improve readability and maintainability.For example:
@Composable private fun ExpandableFilterSection( title: String, content: @Composable () -> Unit, ) { var isExpanded by remember { mutableStateOf(false) } Column(modifier = Modifier.padding(10.dp)) { Row( modifier = Modifier.fillMaxWidth() .clickable { isExpanded = !isExpanded }, horizontalArrangement = Arrangement.SpaceBetween, ) { Text(text = title, style = MifosTypography.titleMediumEmphasized) Icon( imageVector = if (isExpanded) MifosIcons.ArrowDropUp else MifosIcons.ArrowDropDown, contentDescription = null, ) } AnimatedVisibility(visible = isExpanded) { content() } } }Then use it for each section:
ExpandableFilterSection(title = "Sort by") { Column { sortTypes.forEach { sort -> // RadioButton row } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt(4 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt(8 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt(8 hunks)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt(1 hunks)feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt
- feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
ClientItem(274-325)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosPagingAppendProgress(104-120)
🔇 Additional comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (1)
292-302: LGTM!
clearFilterscorrectly resets both the list and flow to their unfiltered sources and clears all filter state including sort.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
77-92: LGTM!FilterBottomSheet integration correctly manages sheet visibility with state and properly dispatches all filter actions to the ViewModel.
| val sortedItems = when (sort) { | ||
| "Name" -> { | ||
| currentItems.sortedBy { it.displayName?.lowercase() } | ||
| } | ||
| "Account Number" -> { | ||
| currentItems.sortedBy { it.accountNo } | ||
| } | ||
| "External ID" -> { | ||
| currentItems.sortedBy { it.externalId } | ||
| } | ||
| else -> currentItems | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Sorting logic duplicated between ViewModel and Android platform.
The sorting logic here (lines 72-83) duplicates the implementation in ClientListViewModel.handleSortClick (lines 217-222). This creates maintenance burden—changes to sort criteria must be applied in both places.
Since the ViewModel already implements sorting, consider having it produce the sorted output rather than re-sorting in the UI layer. Alternatively, extract the sorting logic into a shared function.
| officeNames.forEach { name -> | ||
| val isChecked = selectedOffices.contains(name) | ||
| if (name != null) { | ||
| Row( | ||
| verticalAlignment = Alignment.CenterVertically, | ||
| ) { | ||
| Spacer(modifier = Modifier.width(10.dp)) | ||
| Checkbox( | ||
| checked = isChecked, | ||
| onCheckedChange = { | ||
| if (it) { | ||
| addOffice(name) | ||
| } else { | ||
| removeOffice(name) | ||
| } | ||
| }, | ||
| ) | ||
| Text(text = name) | ||
| } | ||
| } |
There was a problem hiding this comment.
Null office handling inconsistent with ViewModel.
Line 561 filters out null office names from the UI, but the ViewModel's applyFilters (line 270) maps nulls to the string "Null". This inconsistency means:
- UI never displays an option for clients with no office
- ViewModel attempts to filter by
"Null"string which will never match UI selections
Align the null handling: either consistently filter out nulls everywhere (UI + ViewModel) or use a sentinel value everywhere.
| private fun handleSortClick(sort: String?) { | ||
| updateState { | ||
| val sortedList = when (sort) { | ||
| "Name" -> it.clients.sortedBy { it.displayName?.lowercase() } | ||
| "Account Number" -> it.clients.sortedBy { it.accountNo } | ||
| "External ID" -> it.clients.sortedBy { it.externalId } | ||
| else -> it.clients | ||
| } | ||
|
|
||
| it.copy( | ||
| sort = sort, | ||
| clients = sortedList, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Sorting only affects the list, not the paging flow.
handleSortClick sorts state.clients but leaves state.clientsFlow untouched. When the UI is rendering from clientsFlow (API/paging mode), this sort will have no visible effect. Users will see different behavior depending on whether they're in DB mode (list) or API mode (flow).
Either apply sorting to both data sources consistently, or document that sorting is only supported in DB mode and disable the sort UI when clientsFlow is active.
private fun handleSortClick(sort: String?) {
updateState {
val sortedList = when (sort) {
"Name" -> it.clients.sortedBy { it.displayName?.lowercase() }
"Account Number" -> it.clients.sortedBy { it.accountNo }
"External ID" -> it.clients.sortedBy { it.externalId }
else -> it.clients
}
// Also need to sort the flow if present
val sortedFlow = it.clientsFlow?.map { pagingData ->
// Note: Sorting PagingData is complex and may require different approach
pagingData
}
it.copy(
sort = sort,
clients = sortedList,
clientsFlow = sortedFlow, // Currently not sorted
)
}
}| private fun applyFilters() { | ||
| fun keep(client: ClientEntity): Boolean { | ||
| val statusMatch = state.selectedStatus.isEmpty() || client.status?.value in state.selectedStatus | ||
| val officeMatch = state.selectedOffices.isEmpty() || (client.officeName ?: "Null") in state.selectedOffices | ||
|
|
||
| return statusMatch && officeMatch | ||
| } | ||
| val filteredList = state.unfilteredClients.filter { client -> | ||
| keep(client) | ||
| } | ||
|
|
||
| val filteredFlow = state.unfilteredClientsFlow?.map { clients -> | ||
| clients.filter { client -> | ||
| keep(client) | ||
| } | ||
| } | ||
|
|
||
| updateState { | ||
| it.copy( | ||
| clients = filteredList, | ||
| clientsFlow = filteredFlow, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Sort is lost when filters change.
applyFilters always derives from unfilteredClients and unfilteredClientsFlow, discarding the current sort. If a user sorts by Name and then adds a status filter, the list will return to unsorted order (filtered, but not sorted).
Apply the current sort after filtering to preserve the user's sort selection:
private fun applyFilters() {
fun keep(client: ClientEntity): Boolean {
val statusMatch = state.selectedStatus.isEmpty() || client.status?.value in state.selectedStatus
val officeMatch = state.selectedOffices.isEmpty() || (client.officeName ?: "Null") in state.selectedOffices
return statusMatch && officeMatch
}
- val filteredList = state.unfilteredClients.filter { client ->
+ var filteredList = state.unfilteredClients.filter { client ->
keep(client)
}
+
+ // Reapply sort after filtering
+ filteredList = when (state.sort) {
+ "Name" -> filteredList.sortedBy { it.displayName?.lowercase() }
+ "Account Number" -> filteredList.sortedBy { it.accountNo }
+ "External ID" -> filteredList.sortedBy { it.externalId }
+ else -> filteredList
+ }
val filteredFlow = state.unfilteredClientsFlow?.map { clients ->
clients.filter { client ->
keep(client)
}
}
updateState {
it.copy(
clients = filteredList,
clientsFlow = filteredFlow,
)
}
}Committable suggestion skipped: line range outside the PR's diff.
🛠️ Refactor suggestion | 🟠 Major
Avoid magic string "Null" for null office names.
Line 270 maps null office names to the string "Null" for filtering. This is fragile—if an office is actually named "Null", the logic will conflate it with clients having no office. Additionally, the UI in ClientListScreen.kt (line 561) filters out null office names entirely, creating inconsistent behavior.
Consider normalizing null office names to a sentinel constant or filtering them out consistently across the codebase.
private fun applyFilters() {
fun keep(client: ClientEntity): Boolean {
val statusMatch = state.selectedStatus.isEmpty() || client.status?.value in state.selectedStatus
- val officeMatch = state.selectedOffices.isEmpty() || (client.officeName ?: "Null") in state.selectedOffices
+ val officeMatch = state.selectedOffices.isEmpty() || client.officeName in state.selectedOffices
return statusMatch && officeMatch
}And update onUpdateOffice to exclude nulls:
private fun onUpdateOffice(offices: List<String?>) {
updateState {
it.copy(
- officeNames = (offices + it.officeNames).distinct().sortedBy { it },
+ officeNames = (offices.filterNotNull() + it.officeNames.filterNotNull()).distinct().sorted(),
)
}
}📝 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 applyFilters() { | |
| fun keep(client: ClientEntity): Boolean { | |
| val statusMatch = state.selectedStatus.isEmpty() || client.status?.value in state.selectedStatus | |
| val officeMatch = state.selectedOffices.isEmpty() || (client.officeName ?: "Null") in state.selectedOffices | |
| return statusMatch && officeMatch | |
| } | |
| val filteredList = state.unfilteredClients.filter { client -> | |
| keep(client) | |
| } | |
| val filteredFlow = state.unfilteredClientsFlow?.map { clients -> | |
| clients.filter { client -> | |
| keep(client) | |
| } | |
| } | |
| updateState { | |
| it.copy( | |
| clients = filteredList, | |
| clientsFlow = filteredFlow, | |
| ) | |
| } | |
| } | |
| private fun applyFilters() { | |
| fun keep(client: ClientEntity): Boolean { | |
| val statusMatch = state.selectedStatus.isEmpty() || client.status?.value in state.selectedStatus | |
| val officeMatch = state.selectedOffices.isEmpty() || client.officeName in state.selectedOffices | |
| return statusMatch && officeMatch | |
| } | |
| val filteredList = state.unfilteredClients.filter { client -> | |
| keep(client) | |
| } | |
| val filteredFlow = state.unfilteredClientsFlow?.map { clients -> | |
| clients.filter { client -> | |
| keep(client) | |
| } | |
| } | |
| updateState { | |
| it.copy( | |
| clients = filteredList, | |
| clientsFlow = filteredFlow, | |
| ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
around lines 267 to 290, avoid using the magic string "Null" for null office
names; instead normalize nulls consistently by introducing a sentinel constant
(e.g., OFFICE_NONE) or by filtering out null office names everywhere. Change the
officeMatch check to compare against either the sentinel or skip clients with
null officeName, and update the clientsFlow mapping similarly; also update
onUpdateOffice to exclude nulls when building selectedOffices so UI filtering
behavior is consistent with ClientListScreen.kt.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (2)
48-55: MoveonUpdateOfficesintoLaunchedEffectto prevent recomposition loop.Calling
onUpdateOffices(offices)directly in the composable body triggers state updates during composition, causing infinite recomposition. This was flagged in a previous review.
69-101: Sorted list path missing pagination append handling.The sorted branch renders items but doesn't handle
loadState.append, losing progress/error UI during pagination. This was flagged in a previous review.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
549-562: Null office handling inconsistent with ViewModel.Line 551 filters out null office names from the UI, but the ViewModel's
handleFilterClick(line 247) maps nulls to the string"Null". This means clients with no office can never be filtered. This was flagged in a previous review but appears unresolved.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (2)
192-206: Sorting only affects the list, not the paging flow.
handleSortClicksortsstate.clientsbut leavesstate.clientsFlowuntouched. When using API/paging mode, sorting has no visible effect. This was flagged in a previous review.
245-247: Magic string "Null" for null office names creates inconsistency.Line 247 maps null office names to
"Null"string for matching, but the UI filters out nulls entirely. This was flagged in a previous review.
🧹 Nitpick comments (5)
feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt (1)
18-29: Stub implementation leaves desktop without client list functionality.The function body is empty, so desktop users won't see any client list. The new
sortandonUpdateOfficesparameters are unused. Consider adding a TODO comment or implementing basic functionality to maintain feature parity across platforms.@Composable internal actual fun LazyColumnForClientListApi( pagingFlow: Flow<PagingData<ClientEntity>>, onRefresh: () -> Unit, onClientSelect: (Int) -> Unit, fetchImage: (Int) -> Unit, images: Map<Int, ByteArray?>, modifier: Modifier, sort: SortTypes?, onUpdateOffices: (List<String?>) -> Unit, ) { + // TODO: Implement desktop client list with sorting and office update support }feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt (1)
18-29: Stub implementation leaves iOS without client list functionality.Same as desktop, the function body is empty. iOS users won't see any client list. Consider adding a TODO comment to track implementation.
@Composable internal actual fun LazyColumnForClientListApi( pagingFlow: Flow<PagingData<ClientEntity>>, onRefresh: () -> Unit, onClientSelect: (Int) -> Unit, fetchImage: (Int) -> Unit, images: Map<Int, ByteArray?>, modifier: Modifier, sort: SortTypes?, onUpdateOffices: (List<String?>) -> Unit, ) { + // TODO: Implement iOS client list with sorting and office update support }feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (3)
378-379: Hardcoded UI strings should be localized.The sort types and status types use hardcoded English strings. These should use string resources for proper internationalization.
Consider extracting these to string resources:
- "Sort by" (line 430)
- "Account Status" (line 480)
- "Office Name" (line 529)
- "Filters" (line 391)
- "Clear" (line 404)
- "Apply" (line 412)
- Status values: "Active", "Pending", "Closed" (line 379)
382-382: Use design tokens instead of magic numbers for padding.Multiple padding values use magic numbers (
15.dp,10.dp) instead ofDesignToken.padding.*. This creates inconsistency with the rest of the codebase.Column( - modifier = Modifier.padding(15.dp), + modifier = Modifier.padding(DesignToken.padding.large), ) {Apply similar changes to other instances using
10.dppadding.
421-465: Consider extracting collapsible section into reusable component.The expand/collapse pattern with
isExpandedstate, row with title and arrow icon, andAnimatedVisibilityis duplicated three times (Sort by, Account Status, Office Name). Extract to a reusableCollapsibleSectioncomposable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt(4 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt(8 hunks)feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt(8 hunks)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt(1 hunks)feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (4)
toggleFilterVisibility(208-214)clearFilters(269-279)handleSortClick(192-206)handleFilterClick(224-267)feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (1)
LazyColumnForClientListApi(37-157)feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt (1)
LazyColumnForClientListApi(18-29)feature/client/src/iosMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ios.kt (1)
LazyColumnForClientListApi(18-29)feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.native.kt (1)
LazyColumnForClientListApi(18-27)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt (1)
MifosEmptyCard(28-46)
🔇 Additional comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (3)
308-317: LGTM on enum definitions.The
SortTypesandFilterTypeenums are well-structured with display values. This provides type safety for sort/filter operations.
339-343: LGTM on new action definitions.The new actions (
ToggleFilterVisibility,HandleFilterClick,HandleSortClick,OnUpdateOffice,ClearFilters) follow the existing sealed interface pattern and provide clear contracts for filter functionality.
269-279:clearFiltersintentionally resets sort - verify this matches UX expectations.
clearFilterssetssort = null, which means clicking "Clear" will also remove the sort selection. Confirm this is the intended UX behavior, as some users might expect "Clear Filters" to only clear status/office filters while preserving sort order.
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(bottom = DesignToken.padding.extraExtraLarge) | ||
| .padding(bottom = DesignToken.padding.extraExtraLarge), |
There was a problem hiding this comment.
Duplicate padding modifier applied.
The bottom padding is applied twice on the same modifier chain, resulting in double the intended spacing.
Text(
modifier = Modifier
.fillMaxWidth()
- .padding(bottom = DesignToken.padding.extraExtraLarge)
.padding(bottom = DesignToken.padding.extraExtraLarge),
text = stringResource(Res.string.feature_client_no_more_clients_available),🤖 Prompt for AI Agents
In
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt
around lines 143 to 146, the Modifier chain applies .padding(bottom =
DesignToken.padding.extraExtraLarge) twice causing double bottom spacing; remove
the duplicate .padding(bottom = DesignToken.padding.extraExtraLarge) so the
modifier only sets the intended single bottom padding (ensure other modifiers
remain in the same order).
| val currentItems = clientPagingList.itemSnapshotList.items | ||
|
|
||
| val sortedItems = when (sort) { | ||
| "Name" -> { |
There was a problem hiding this comment.
Don't use hardcoded string here use enum for handle this sort logic because in that logic may occur typo error in future that's why use enum class
2 participants
Fixes - Jira-MIFOSAC-517
screen-20251211-193323.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
✏️ Tip: You can customize this high-level summary in your review settings.