feat(feature/client): default to "Active" status and apply filters on refresh #2582
Conversation
📝 WalkthroughWalkthroughThe pull request modifies the ClientListViewModel to add filtering functionality for client lists. It initializes selectedStatus with "Active" as default, then applies filtering by selectedStatus and selectedOffices in both direct database result handling and paging data flows, while preserving unfiltered data separately. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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
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.
| import kotlin.text.contains | ||
| import kotlin.toString |
There was a problem hiding this comment.
The added imports kotlin.text.contains and kotlin.toString are unused here (the file doesn't call contains(...), and toString() doesn't need an import). Please remove them to avoid lint/ktlint failures.
| import kotlin.text.contains | |
| import kotlin.toString |
| updateState { | ||
| val data = result.data.pageItems | ||
| if (data.isEmpty()) { | ||
| it.copy(isEmpty = true, dialogState = null) |
There was a problem hiding this comment.
When the DB returns an empty data list, the state update only sets isEmpty = true and leaves clients/unfilteredClients untouched. This can display stale clients (UI renders clients when non-empty) while also hiding actions due to isEmpty. Clear the client lists in this branch (and likely reset related fields like unfilteredClients).
| it.copy(isEmpty = true, dialogState = null) | |
| it.copy( | |
| isEmpty = true, | |
| clients = emptyList(), | |
| unfilteredClients = emptyList(), | |
| dialogState = null, | |
| ) |
| } | ||
| it.copy( | ||
| clients = filteredData, | ||
| unfilteredClients = data, |
There was a problem hiding this comment.
In the non-empty DB success branch, isEmpty is never reset to false. If a previous load set isEmpty = true, the screen can remain in the empty-state mode (e.g., hiding ClientActions) even though clients now has data. Set isEmpty = false when updating state with non-empty results (and consider basing it on filteredData.isEmpty() if that's the intended UX).
| unfilteredClients = data, | |
| unfilteredClients = data, | |
| isEmpty = filteredData.isEmpty(), |
| dialogState = null, | ||
| clientsFlow = filteredFlow, | ||
| unfilteredClientsFlow = result, | ||
| dialogState = null, |
There was a problem hiding this comment.
handleClientResult updates clientsFlow/unfilteredClientsFlow but doesn't reset isEmpty. If isEmpty was previously set to true (e.g., from a DB load), the UI can stay in the empty-state mode while paging data is shown. Consider explicitly setting isEmpty = false here when assigning a non-null flow.
| dialogState = null, | |
| dialogState = null, | |
| isEmpty = false, |
| val filteredData = data.filter { client -> | ||
| val statusMatch = it.selectedStatus.isEmpty() || | ||
| client.status?.value in it.selectedStatus | ||
| val officeMatch = it.selectedOffices.isEmpty() || | ||
| (client.officeName ?: "Null") in it.selectedOffices | ||
| statusMatch && officeMatch | ||
| } |
There was a problem hiding this comment.
The same status/office filter predicate is duplicated in multiple places (handleClientResultFromDb, handleClientResult, and the local keep(...) in handleFilterClick). To reduce the chance of future inconsistencies, consider extracting this into a single helper (e.g., a function that returns a (ClientEntity) -> Boolean based on selected filters) and reuse it for list + paging filtering.
![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
| is DataState.Success -> { | ||
| updateState { | ||
| val data = result.data.pageItems | ||
| if (data.isEmpty()) { | ||
| it.copy(isEmpty = true, dialogState = null) | ||
| } else { | ||
| val filteredData = data.filter { client -> | ||
| val statusMatch = it.selectedStatus.isEmpty() || | ||
| client.status?.value in it.selectedStatus | ||
| val officeMatch = it.selectedOffices.isEmpty() || | ||
| (client.officeName ?: "Null") in it.selectedOffices | ||
| statusMatch && officeMatch | ||
| } | ||
| it.copy( | ||
| clients = filteredData, | ||
| unfilteredClients = data, | ||
| dialogState = null | ||
| ) | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Potential issues with null handling and empty state.
-
Magic string
"Null": Using"Null"as a fallback for nullofficeName(line 165) creates a hidden coupling. If users can filter by offices, they'd need to select a literal"Null"string to match clients with no office assigned. Consider whether this is intentional UX or if null offices should be handled differently. -
Empty state doesn't clear clients: When
data.isEmpty()(line 158), onlyisEmpty = trueis set without clearingclientsorunfilteredClients, potentially leaving stale data.
Suggested fix for empty state
if (data.isEmpty()) {
- it.copy(isEmpty = true, dialogState = null)
+ it.copy(
+ isEmpty = true,
+ clients = emptyList(),
+ unfilteredClients = emptyList(),
+ dialogState = null
+ )
} else {📝 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.
| is DataState.Success -> { | |
| updateState { | |
| val data = result.data.pageItems | |
| if (data.isEmpty()) { | |
| it.copy(isEmpty = true, dialogState = null) | |
| } else { | |
| val filteredData = data.filter { client -> | |
| val statusMatch = it.selectedStatus.isEmpty() || | |
| client.status?.value in it.selectedStatus | |
| val officeMatch = it.selectedOffices.isEmpty() || | |
| (client.officeName ?: "Null") in it.selectedOffices | |
| statusMatch && officeMatch | |
| } | |
| it.copy( | |
| clients = filteredData, | |
| unfilteredClients = data, | |
| dialogState = null | |
| ) | |
| } | |
| } | |
| } | |
| } | |
| is DataState.Success -> { | |
| updateState { | |
| val data = result.data.pageItems | |
| if (data.isEmpty()) { | |
| it.copy( | |
| isEmpty = true, | |
| clients = emptyList(), | |
| unfilteredClients = emptyList(), | |
| dialogState = null | |
| ) | |
| } else { | |
| val filteredData = data.filter { client -> | |
| val statusMatch = it.selectedStatus.isEmpty() || | |
| client.status?.value in it.selectedStatus | |
| val officeMatch = it.selectedOffices.isEmpty() || | |
| (client.officeName ?: "Null") in it.selectedOffices | |
| statusMatch && officeMatch | |
| } | |
| it.copy( | |
| clients = filteredData, | |
| unfilteredClients = data, | |
| dialogState = null | |
| ) | |
| } | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt`
around lines 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.
|
@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. |
2 participants
Fixes - Jira-#I632
Description
This PR improves the Client List behavior by setting Active as the default status filter and ensuring that applied filters persist across data refreshes.
Behavior Changes
Changes Implemented
Default Active Filter
Filter Persistence on Refresh
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.