-
Notifications
You must be signed in to change notification settings - Fork 671
feat(feature/client): default to "Active" status and apply filters on refresh #2582
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
feat(feature/client): default to "Active" status and apply filters on refresh #2582
Conversation
📝 WalkthroughWalkthroughMoves client filtering and sorting into state-driven logic in the ViewModel, applies identical filtering to paging flows while preserving unfiltered sources, and updates the Android UI to render a memoized, sorted/filtered display list with a status-order helper and prefetch trigger. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as ClientListScreen (UI)
participant VM as ClientListViewModel
participant Repo as Repository/API/Paging
UI->>VM: observe clients & emit filter/sort selections
VM->>Repo: request clients (API or Paging source)
Repo-->>VM: return clients list or PagingData
VM->>VM: apply filters (selectedStatus, selectedOffices)
VM->>VM: compute status order and sort results
VM-->>UI: emit filtered/sorted list or clientsFlow
UI->>UI: remember(displayList) and render LazyColumn
UI->>VM: on last visible index -> request 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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. |
dfbfaeb to
83623f3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
83623f3 to
329e335
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
… 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
329e335 to
037bf87
Compare
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
✏️ Tip: You can customize this high-level summary in your review settings.