Skip to content

Conversation

@Kartikey15dem
Copy link

@Kartikey15dem Kartikey15dem commented Jan 24, 2026

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)

  • The Change: Implemented a default sort order that prioritizes clients by status: Active first, then Pending, then Closed. Consequently, the 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: This sorting is applied client-side to the currently loaded pages. As the user scrolls and the Paging library fetches new pages, any newly discovered "Active" clients will automatically be sorted to the top of the list.
  • Technical Note / Limitation: Since the backend API does not currently support server-side sorting for this data, this client-side approach is necessary. Users may initially perceive a limited number of "Active" users until they scroll down and trigger further pages to load.

Behavior Changes

Before After
Before After

2. Fixed Filter Persistence on Refresh

  • The Issue: Previously, triggering a refresh (e.g., when data failed to load) re-initialized the data flow in loadClients(), causing the active filters to be ignored or cleared.
  • The Fix: Updated the handleClientResult logic to check the ClientListState immediately 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

Before After
refreshbef.webm
refreshaft.webm

3. Fixed "Pagination Stall" (Infinite Scroll Bug)

  • The Issue: When a Sort and then Filter was applied, the displayed list often became shorter than the screen height (e.g., only 3 filtered items visible). Because the user could not scroll down to hit the Paging Library's "prefetch distance," the library assumed no more data was needed, and infinite scrolling stopped.
  • Technical Root Cause: To support local client-side sorting, we must convert the live LazyPagingItems flow into a static itemSnapshotList. 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.
  • The Fix: Implemented a manual workaround in LazyColumnForClientListApi to bridge this gap:
    • Auto-Fetch: If the sorted/filtered list is empty or fails to fill the screen, the code programmatically triggers the Paging source to fetch the next page.
    • Scroll Bridge: Added logic to detect when the user reaches the bottom of the sorted list and triggers a load request on the raw list, ensuring seamless scrolling even when the sorted view differs from the API order.

Behavior Changes

Before After
stallbef.webm
stallaft.webm

How to Test

  1. Sorting: Open the client list. Verify that "Active" clients appear at the top by default. Scroll down and verify that new "Active" clients appearing in later pages jump to the correct position.
  2. Persistence: Apply a status filter (e.g., "Pending"). Pull to refresh. Verify that the list reloads but the "Pending" filter remains active.
  3. Pagination: Apply a sort and then filter that results in very few items (less than a full screen). Verify that the app automatically loads the next page to try and fill the screen. Scroll to the bottom of a sorted list and verify the next page loads correctly.

Summary by CodeRabbit

  • New Features

    • Automatic loading of additional clients when scrolling near the end of the list
  • Improvements

    • Enhanced filtering of client lists based on selected status and office choices
    • Improved sorting capabilities by name, account number, and external ID
    • Optimized list rendering and memoization for better performance

✏️ Tip: You can customize this high-level summary in your review settings.

Copilot AI review requested due to automatic review settings January 24, 2026 09:21
@coderabbitai
Copy link

coderabbitai bot commented Jan 24, 2026

📝 Walkthrough

Walkthrough

Moves 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

Cohort / File(s) Summary
Client List ViewModel
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListViewModel.kt
Replaces direct DataState.Success handling with state-aware updates: filters pageItems by selectedStatus and selectedOffices, computes a status order, sorts by NAME/ACCOUNT_NUMBER/EXTERNAL_ID or status order, updates clients and unfilteredClients, and applies identical state-driven filtering to clientsFlow.
Client List Screen (Android UI)
feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt
Consolidates rendering into one memoized displayList from itemSnapshotList plus sort; removes duplicate LazyColumn branches; adds LaunchedEffect to trigger prefetch/refresh on last visible index; introduces public fun getStatusOrder(status: String?): Int.

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
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • biplab1
  • Arinyadav1

Poem

🐰 I hopped through lists by moonlit code,

Filters set and order sowed,
Status, office, sorted fine,
Paging kept the source benign,
A joyful hop — your clients shine!

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the main changes: applying default status ordering and ensuring filters persist on refresh, which are the core issues addressed in this PR.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

Copilot AI left a 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 selectedStatus to ["Active"] so the list defaults to Active clients.
  • Apply selectedStatus/selectedOffices filtering 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.

Copy link

@coderabbitai coderabbitai bot left a 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.contains and kotlin.toString are standard Kotlin functions that don't require explicit imports. The code uses the in operator (which compiles to contains) and toString() 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

@niyajali
Copy link
Collaborator

@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.

@Kartikey15dem
Copy link
Author

@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.
@niyajali I opened the pull request but I found some other issues so I am still working on it.

@niyajali
Copy link
Collaborator

Sort And Group Client List by Status and Initially display the Active client.

@Kartikey15dem Kartikey15dem force-pushed the feature/client-default-order-active branch from dfbfaeb to 83623f3 Compare January 28, 2026 09:31
Copy link

@coderabbitai coderabbitai bot left a 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: Duplicate getStatusOrder function.

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: Inefficient LaunchedEffect(Unit) key.

Using Unit as 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 getStatusOrder function duplicates the one in ClientListViewModel.kt (lines 166-173). Move this to a shared utility or make the ViewModel's version accessible to avoid drift between implementations.

@Kartikey15dem Kartikey15dem force-pushed the feature/client-default-order-active branch from 83623f3 to 329e335 Compare January 28, 2026 10:01
Copy link

@coderabbitai coderabbitai bot left a 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: Deduplicate getStatusOrder into 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.

… 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 Kartikey15dem force-pushed the feature/client-default-order-active branch from 329e335 to 037bf87 Compare January 28, 2026 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants