feat(feature/client): default to "Active" status and apply filters on refresh #2582
Conversation
📝 WalkthroughWalkthroughMoves filtering and status-order sorting into the ViewModel for both API and paging results (preserving unfiltered sources), and consolidates Android UI to render a memoized display list with a prefetch trigger and a new public Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ClientListScreen (UI)
participant VM as ClientListViewModel
participant Repo as Repository / PagingSource
UI->>VM: observe clients + emit filter/sort selections
VM->>Repo: request clients (API or Paging)
Repo-->>VM: return clients list or PagingData
VM->>VM: apply selectedStatus & selectedOffices filters
VM->>VM: compute status order (getStatusOrder) and sort results
VM-->>UI: emit filtered/sorted clientsFlow and unfilteredClientsFlow
UI->>UI: remember(displayList) from clientsFlow / itemSnapshotList
UI->>VM: on last visible index -> trigger prefetch/refresh
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
Pull request overview
This PR updates the Client List screen behavior to default the status filter to Active and to re-apply the currently selected Status/Office filters whenever client data is refreshed (API or DB).
Changes:
- Set the initial
selectedStatusto["Active"]so the list defaults to Active clients. - Apply
selectedStatus/selectedOfficesfiltering when new data arrives from DB and when paging data arrives from API. - Preserve an unfiltered paging flow (
unfilteredClientsFlow) so filter changes can be re-applied without losing the original stream.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Outdated
Show resolved
Hide resolved
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Outdated
Show resolved
Hide resolved
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Show resolved
Hide resolved
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Show resolved
Hide resolved
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Show resolved
Hide resolved
![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
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt`:
- Around line 155-176: When handling the DataState.Success branch, stop using
the magic string "Null" for client.officeName and instead treat null explicitly:
compute officeMatch as true when selectedOffices is empty, otherwise compare
client.officeName directly (nullable) against the selectedOffices set or handle
a dedicated sentinel for unassigned offices (e.g., a null entry or explicit
"UNASSIGNED" constant) so filtering is explicit and not coupled to the literal
"Null"; also when data.isEmpty() set isEmpty = true AND clear stale results by
setting clients = emptyList() and unfilteredClients = emptyList() (and
dialogState = null) so stale data is not preserved. Ensure these changes are
applied inside updateState where selectedStatus, selectedOffices, clients,
unfilteredClients, dialogState and isEmpty are updated.
🧹 Nitpick comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (2)
31-32: Remove unnecessary imports.
kotlin.text.containsandkotlin.toStringare standard Kotlin functions that don't require explicit imports. The code uses theinoperator (which compiles tocontains) andtoString()is always available on any object.Suggested fix
-import kotlin.text.contains -import kotlin.toString
161-167: Consider extracting duplicated filter logic.The same filtering predicate (status match + office match) is duplicated in three locations:
handleClientResultFromDb(lines 161-166)handleClientResult(lines 182-187)handleFilterClick(lines 272-276)Extract to a reusable helper for consistency and maintainability.
Suggested extraction
private fun ClientEntity.matchesFilters( selectedStatus: List<String>, selectedOffices: List<String>, ): Boolean { val statusMatch = selectedStatus.isEmpty() || status?.value in selectedStatus val officeMatch = selectedOffices.isEmpty() || (officeName ?: "Null") in selectedOffices return statusMatch && officeMatch }Then use it as:
val filteredData = data.filter { client -> client.matchesFilters(it.selectedStatus, it.selectedOffices) }Also applies to: 182-188, 272-277
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Show resolved
Hide resolved
|
@Kartikey15dem If the data is filtered solely by "Active" status, the user may neglect to filter other statuses, thereby failing to execute the necessary action on that data. Additionally, the Jira ticket mandates sorting the data by "Active" status, not filtering it. |
|
|
Sort And Group Client List by Status and Initially display the Active client. |
Kartikey15dem
force-pushed
the
feature/client-default-order-active
branch
from
dfbfaeb to
83623f3
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In
`@feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt`:
- Around line 134-137: The Modifier chain in ClientListScreen.android.kt applies
.padding(bottom = DesignToken.padding.extraExtraLarge) twice on the modifier
(within the modifier = Modifier... chain) causing double bottom spacing; remove
the duplicated .padding call so the modifier reads
Modifier.fillMaxWidth().padding(bottom = DesignToken.padding.extraExtraLarge)
(keep a single padding invocation) to restore the intended spacing.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt`:
- Around line 191-207: handleClientResult currently captures
state.selectedStatus and state.selectedOffices at creation time inside
filteredFlow, causing stale filters to be applied; change this so the filtered
PagingData is derived reactively at collection time by combining the
unfilteredClientsFlow with the current UI filter state instead of closing over
state values. Specifically, stop using result.map { ... state.selectedStatus ...
state.selectedOffices } in handleClientResult; instead set unfilteredClientsFlow
= result and create clientsFlow by combining or flatMapLatest with a StateFlow
representing the UI filters (or re-run handleClientResult when filters change)
so clientsFlow always applies the up-to-date state
selectedStatus/selectedOffices (references: handleClientResult, filteredFlow,
clientsFlow, unfilteredClientsFlow, state.selectedStatus, state.selectedOffices,
handleFilterClick).
🧹 Nitpick comments (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (2)
31-33: Unnecessary explicit imports.These imports (
kotlin.collections.sort,kotlin.text.contains,kotlin.toString) are part of Kotlin's standard library and are implicitly available. They can be removed.♻️ Proposed fix
-import kotlin.collections.sort -import kotlin.text.contains -import kotlin.toString
166-173: DuplicategetStatusOrderfunction.This function is duplicated in
ClientListScreen.android.kt(lines 148-155). Extract it to a shared location (e.g., as a top-level function in the ViewModel file or a utility file) to maintain a single source of truth.feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (3)
81-88: Consider extracting magic number.The threshold
10(line 84) is a magic number. Consider extracting it to a named constant for clarity on why this specific threshold was chosen for prefetching.
97-104: InefficientLaunchedEffect(Unit)key.Using
Unitas the key means this effect runs on every recomposition of the last item. Consider using a stable key like the client ID to avoid redundant prefetch triggers:♻️ Proposed fix
if (client == displayList.last()) { - LaunchedEffect(Unit) { + LaunchedEffect(client.id) { val lastIndex = clientPagingList.itemCount - 1 if (lastIndex >= 0) { clientPagingList[lastIndex] } } }
148-155: Consolidate duplicate function.This
getStatusOrderfunction duplicates the one inClientListViewModel.kt(lines 166-173). Move this to a shared utility or make the ViewModel's version accessible to avoid drift between implementations.
...ient/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt
Show resolved
Hide resolved
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Show resolved
Hide resolved
Kartikey15dem
force-pushed
the
feature/client-default-order-active
branch
from
83623f3 to
329e335
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt`:
- Around line 81-88: The current logic uses clientPagingList.itemCount - 1
(lastRawIndex) which can point to placeholder positions when enablePlaceholders
is on; change it to compute the last loaded index from the snapshot (e.g., val
lastLoadedIndex = clientPagingList.itemSnapshotList.items.lastIndex) and use
that (after checking lastLoadedIndex >= 0) when indexing clientPagingList inside
the LaunchedEffect (same block using clientPagingList and displayList) so you
only access actual loaded items and avoid unintended prefetching.
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (1)
162-169: DeduplicategetStatusOrderinto a shared helper.Line 162-169 defines a local status-order map that also exists in the Android screen. Consider moving this to a shared commonMain utility to avoid divergence and per-update allocations.
...ient/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt
Show resolved
Hide resolved
Kartikey15dem
force-pushed
the
feature/client-default-order-active
branch
2 times, most recently
from
037bf87 to
a6b520c
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt`:
- Around line 170-179: The unfilteredClients is being stored raw as data so
later filter/clear flows rebuild from backend order and lose the default status
sort; fix by always persisting a sorted baseline and applying the same sort when
rebuilding filtered lists: when assigning unfilteredClients = data in the state
update, sort data according to state.sort (or default status order using
getStatusOrder) before storing, and centralize the logic into a helper like
sortClients(list, sortType) that is used both when setting unfilteredClients and
when computing sortedList from filteredData so filters/clears always respect
Active→Pending→Closed or the selected SortTypes (NAME, ACCOUNT_NUMBER,
EXTERNAL_ID).
🧹 Nitpick comments (1)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (1)
148-155: Consider sharing the status-order helper across modules.This mapping also exists in the ViewModel. Extracting to a commonMain utility avoids divergence and keeps DB + paging paths aligned.
...ure/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Show resolved
Hide resolved
Kartikey15dem
force-pushed
the
feature/client-default-order-active
branch
from
a6b520c to
763bfe4
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (2)
224-238:⚠️ Potential issue | 🟡 MinorDefault status sorting not applied when
sortis null.In
handleSortClick, whensortisnull(the else branch at line 230), clients are returned unsorted (it.clients). This is inconsistent withhandleClientResultFromDb(line 174) which appliesgetStatusOrderas the default sort. After toggling sorting off, users may lose the Active→Pending→Closed ordering.🐛 Proposed fix for consistency
private fun handleSortClick(sort: SortTypes?) { updateState { + fun getStatusOrder(status: String?): Int { + return when (status?.lowercase()) { + "active" -> 1 + "pending" -> 2 + "closed" -> 3 + else -> 4 + } + } val sortedList = when (sort) { SortTypes.NAME -> it.clients.sortedBy { it.displayName?.lowercase() } SortTypes.ACCOUNT_NUMBER -> it.clients.sortedBy { it.accountNo } SortTypes.EXTERNAL_ID -> it.clients.sortedBy { it.externalId } - else -> it.clients + else -> it.clients.sortedBy { client -> getStatusOrder(client.status?.value) } }
301-311:⚠️ Potential issue | 🟡 Minor
clearFiltersdoesn't reapply default status sorting.After clearing filters,
unfilteredClientsis restored directly without applying the default Active→Pending→Closed sort order. This means users lose the default ordering after clearing filters, which contradicts the PR's stated goal of maintaining default status-based sorting.🐛 Proposed fix
private fun clearFilters() { updateState { + fun getStatusOrder(status: String?): Int { + return when (status?.lowercase()) { + "active" -> 1 + "pending" -> 2 + "closed" -> 3 + else -> 4 + } + } + val sortedClients = it.unfilteredClients.sortedBy { client -> + getStatusOrder(client.status?.value) + } it.copy( - clients = it.unfilteredClients, + clients = sortedClients, clientsFlow = it.unfilteredClientsFlow, sort = null, selectedStatus = emptyList(), selectedOffices = emptyList(), ) } }
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt (1)
162-169: DuplicategetStatusOrderfunction violates DRY principle.This function is duplicated verbatim in
ClientListScreen.android.kt(lines 148-155). Extract it to a shared location (e.g., a common utility or companion object) to avoid divergence and reduce maintenance burden.♻️ Suggested approach
Move
getStatusOrderto a shared utility file incommonMain:// In a new file like ClientListUtils.kt or add to existing common utilities fun getStatusOrder(status: String?): Int { return when (status?.lowercase()) { "active" -> 1 "pending" -> 2 "closed" -> 3 else -> 4 } }Then import and use it in both
ClientListViewModel.ktandClientListScreen.android.kt.feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt (2)
97-104: Inefficient comparisonclient == displayList.last()on every item.Calling
displayList.last()inside theitemsloop means it's invoked for every client, resulting in O(n) overhead. Since you already have the index available via the iteration, compare against the precomputed last index instead.♻️ Proposed optimization
items( items = displayList, key = { client -> client.id }, - ) { client -> + ) { index, client -> LaunchedEffect(client.id) { fetchImage(client.id) } - if (client == displayList.last()) { + if (index == displayList.lastIndex) { LaunchedEffect(Unit) { val lastIndex = clientPagingList.itemCount - 1 if (lastIndex >= 0) { clientPagingList[lastIndex] } } }Note: Use
itemsIndexedinstead ofitemsto get the index parameter.
148-155: Duplicate function - extract to shared utility.This
getStatusOrderfunction is identical to the one inClientListViewModel.kt(lines 162-169). See the comment on the ViewModel file for the recommended refactor.
Kartikey15dem
force-pushed
the
feature/client-default-order-active
branch
from
763bfe4 to
8791552
Compare
… initial load feat(feature/client): default to "Active" status and apply filters on refresh feat(feature/client): default to "Active" status and apply filters on refresh feat(feature/client): default to "Active" status and apply filters on refresh feat(feature/client): default to "Active" status and apply filters on refresh feat(feature/client): default to "Active" status and apply filters on refresh
Kartikey15dem
force-pushed
the
feature/client-default-order-active
branch
from
8791552 to
57c6794
Compare
2 participants
Fixes - Jira-#631
Fix: Client List Sorting, Filter Persistence & Pagination Stall
Overview
This PR addresses three core issues in the Client List feature to improve data presentation and user experience. It introduces a default business-logic sort order, ensures filters persist through refreshes, and fixes a critical bug where pagination would stop working when sorting and then filters were applied.
Key Changes
1. Default "Business Logic" Sorting (Active → Pending → Closed)
if (sort != null)conditional block was removed from the UI, as sorting logic is now always applied (either via the default business logic or user selection).Behavior Changes
2. Fixed Filter Persistence on Refresh
loadClients(), causing the active filters to be ignored or cleared.handleClientResultlogic to check theClientListStateimmediately upon receiving new data. The new flow is now wrapped with the existing filter logic before being emitted to the UI, ensuring the user's context is preserved across reloads.Behavior Changes
refreshbef.webm
refreshaft.webm
3. Fixed "Pagination Stall" (Infinite Scroll Bug)
LazyPagingItemsflow into a staticitemSnapshotList. Accessing items in this static snapshot does not trigger the Paging Library's internal load mechanism (which normally auto-fetches data when indices are accessed). This disconnects the "live" trigger required for infinite scrolling.LazyColumnForClientListApito bridge this gap:Behavior Changes
stallbef.webm
stallaft.webm
How to Test
Summary by CodeRabbit
New Features
Improvements