fix(client): Migrate Paging to commonMain from androidMain - #2638
Merged
therajanmaurya merged 3 commits intoMar 10, 2026
Conversation
…oving compatibility with other platforms.
📝 WalkthroughWalkthroughThis PR moves androidx.paging.compose to commonMain and consolidates multiple platform-specific UI stubs into concrete implementations in commonMain for client lists, client charges, and upcoming charges, removing platform-specific files across android, desktop, js, native, and wasmJs. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Common UI (Compose)
participant Paging as PagingSource/Flow
participant Loader as LoadState Handler
participant Actions as onAction Handler
UI->>Paging: collectAsLazyPagingItems(flow)
Paging-->>UI: emit PagingData items
UI->>Loader: observe items.loadState
Loader-->>UI: LoadState (Loading / Error / NotLoading)
UI-->>UI: render Loading/Error/List based on LoadState
UI->>Actions: onAction(Delete|Edit|Create|Dismiss)
Actions-->>UI: update state / trigger navigation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 6
🧹 Nitpick comments (6)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
368-373: Potential excessive calls toonUpdateOfficesduring pagination.
itemSnapshotList.itemschanges frequently as the user scrolls and more items are loaded. Sinceofficesis derived from these items and used as theLaunchedEffectkey,onUpdateOfficeswill be called repeatedly. Consider usingderivedStateOfwith a more stable comparison or debouncing the updates.♻️ Proposed optimization
val items = clientPagingList.itemSnapshotList.items if (items.isNotEmpty()) { - val offices = items.map { it.officeName } - .distinct() - LaunchedEffect(offices) { onUpdateOffices(offices) } + val offices = remember(items) { + items.map { it.officeName }.distinct() + } + LaunchedEffect(offices.hashCode()) { onUpdateOffices(offices) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt` around lines 368 - 373, The current LaunchedEffect keyed on a freshly computed offices list causes onUpdateOffices to run too often; compute a stable derived state for offices (e.g., wrap items.map { it.officeName }.distinct() in remember { derivedStateOf { ... } } or use derivedStateOf with a Set for stable equality), then key the LaunchedEffect on that derived state's value (offices.value) so onUpdateOffices is only invoked when the distinct office set actually changes; update references in ClientListScreen.kt to use the derivedStateOf result and call onUpdateOffices(offices.value).feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt (1)
344-346: Inconsistent theme usage: useKptThemeinstead ofMaterialTheme.This code uses
MaterialTheme.colorSchemewhile the rest of the codebase usesKptTheme.colorScheme. The same issue appears on lines 388, 397, 401, and 441 whereMaterialTheme.typographyis used instead ofKptTheme.typography.🎨 Proposed fix for consistent theming
- val surfaceColor = MaterialTheme.colorScheme.surface - val outlineColor = MaterialTheme.colorScheme.outline + val surfaceColor = KptTheme.colorScheme.surface + val outlineColor = KptTheme.colorScheme.outlineAnd for the border (line 388):
- .border(1.dp, MaterialTheme.colorScheme.outline, CircleShape), + .border(1.dp, KptTheme.colorScheme.outline, CircleShape),And for typography (lines 397, 401, 441):
- style = MaterialTheme.typography.bodyLarge, + style = KptTheme.typography.bodyLarge,- style = MaterialTheme.typography.bodyMedium, + style = KptTheme.typography.bodyMedium,Based on learnings: "In Kotlin files under the android-client module, replace MaterialTheme references with the established KptTheme import".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt` around lines 344 - 346, Replace all uses of MaterialTheme with the project's KptTheme in this file: change the color references that set surfaceColor, outlineColor and cardColor (currently using MaterialTheme.colorScheme) to use KptTheme.colorScheme, and replace all MaterialTheme.typography usages with KptTheme.typography so theming is consistent across ClientListScreen (update the declarations for surfaceColor, outlineColor, cardColor and the composables that reference MaterialTheme.typography).feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt (4)
292-292: UseMifosTypographyfor consistency with the rest of the file.Lines 166 and 171 use
MifosTypographyfor text styling, but this line usesMaterialTheme.typography. Based on learnings, prefer established theme imports (KptThemeorMifosTypography) overMaterialThemefor consistency.♻️ Suggested fix
- style = MaterialTheme.typography.bodyMedium, + style = MifosTypography.bodyMedium,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt` at line 292, The text style usage at the call site using MaterialTheme.typography.bodyMedium should be replaced with the project-specific typography to match the rest of the file: change the style reference to MifosTypography.bodyMedium (or the corresponding MifosTypography token used in lines 166/171) inside the ClientUpcomingChargesScreen composable so styling is consistent with other text components.
270-270: Remove unnecessary fully qualified import.
stringResourceis already imported at line 61. The fully qualified reference here is inconsistent with the rest of the file.♻️ Suggested fix
- MifosSweetError(message = org.jetbrains.compose.resources.stringResource(Res.string.client_upcoming_charges_failed_message)) { + MifosSweetError(message = stringResource(Res.string.client_upcoming_charges_failed_message)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt` at line 270, Remove the unnecessary fully-qualified call to org.jetbrains.compose.resources.stringResource inside the MifosSweetError invocation and replace it with the already imported stringResource to match the file's import style; update the call in the MifosSweetError(...) where the message is Res.string.client_upcoming_charges_failed_message so it uses stringResource(Res.string.client_upcoming_charges_failed_message) rather than org.jetbrains.compose.resources.stringResource(...).
239-244: Address or track the TODO comment.The TODO indicates uncertainty about the "due" calculation logic. If this is a known limitation, consider creating an issue to track it; otherwise, verify the calculation with domain requirements.
Would you like me to open a new issue to track this TODO?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt` around lines 239 - 244, The TODO about the "due" calculation in ClientUpcomingChargesScreen (where due is computed from charge.amount and charge.amountPaid) must be resolved: either confirm the business rule with product/domain owners and, if correct, remove the TODO and add a short clarifying comment stating the rule (e.g., due = amount - amountPaid) or, if uncertain/known limitation, create a tracked issue in the repo referencing ClientUpcomingChargesScreen and the due calculation, replace the TODO with a one-line comment linking to that issue ID, and include expected behavior to verify later.
222-226: Consider wrapping LazyColumn in an else block to avoid rendering both empty card and list.When refresh completes with zero items,
MifosEmptyCard()is shown but theLazyColumnbelow still renders (albeit empty). This could cause minor layout issues.♻️ Suggested refactor
- if (chargesPagingList.loadState.refresh is LoadState.NotLoading && chargesPagingList.itemCount == 0) { - MifosEmptyCard() - } - - LazyColumn { + if (chargesPagingList.loadState.refresh is LoadState.NotLoading && chargesPagingList.itemCount == 0) { + MifosEmptyCard() + } else { + LazyColumn { // ... existing content + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt` around lines 222 - 226, The current code calls MifosEmptyCard() when chargesPagingList.loadState.refresh is NotLoading and itemCount == 0 but still proceeds to render LazyColumn; change the control flow so LazyColumn is rendered only when there are items (i.e. use an else branch) — locate the conditional using chargesPagingList.loadState.refresh and itemCount and wrap the LazyColumn block in the corresponding else so that MifosEmptyCard() and LazyColumn are mutually exclusive.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.kt`:
- Around line 89-92: The code is rendering literal "null" by calling .toString()
on nullable fields; update ShowClientCharge to safely handle nullable values:
replace usages of charge.chargeCalculationType?.value.toString() and
charge.amount.toString() with null-safe formatting (e.g. use the safe-call +
elvis pattern or conditional formatting) so that when
charge.chargeCalculationType or charge.amount is null you return a sensible
default (empty string or formatted zero) instead of "null"; locate these
expressions on the Charge rendering logic in ShowClientCharge and change them to
use charge.chargeCalculationType?.value ?: "" and charge.amount?.let{ /* format
*/ } ?: "" (or your preferred default).
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt`:
- Around line 338-341: The key lambda in the items call currently falls back to
the list index (key = { index -> clientPagingList[index]?.id ?: index }), which
can break recomposition when items shift; update the key to avoid using the raw
index as a fallback by either guaranteeing a non-null stable id on each client
or generating a stable surrogate key from the client object (e.g., a persistent
tempId, a stable composite of immutable fields, or the item's hashCode) inside
the key lambda, or skip rendering entries with null id until they resolve;
change the key logic around clientPagingList and its id access so keys remain
stable when items are inserted/removed.
- Around line 417-422: In the LoadState.Error branch (where MifosSweetError is
shown) replace the call to failedRefresh() with a call to
clientPagingList.retry() so that only the failed append page is retried instead
of refreshing the whole list; locate the LoadState.Error handling around
MifosSweetError in ClientListScreen.kt and swap failedRefresh() for
clientPagingList.retry() to perform a targeted retry of the append operation.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt`:
- Around line 387-419: The sorted branch currently renders a sorted snapshot
(using sort, clientPagingList.itemSnapshotList.items, sortedItems, LazyColumn
and items { client -> ... }) but omits append load-state UI (loading spinner,
error retry, end-of-list message) that exists for the unsorted branch; extract
the append-state rendering logic that inspects clientPagingList.loadState.append
(and shows loading, error with retry, or "no more clients") into a small
reusable composable or helper (e.g., renderAppendState or AppendStateItems) that
accepts the PagingData source (clientPagingList), the images map, and
fetchImage/onClientSelect handlers, then invoke that helper inside both the
sorted branch (after items iteration over sortedItems) and the unsorted branch
so additional page loading, errors, and "no more clients" are displayed
consistently.
- Around line 424-427: The key fallback is unsafe: stop using index as a
fallback key in the items { ... key = { ... } } lambda; instead, fetch the item
once (val item = clientPagingList[index]) and only use item.id as the key,
skipping/returning early when the item is null so you never fall back to the
unstable index; update the items block to compute the item, use item.id in the
key lambda (or guard render with if (item == null) return@items) to ensure
stable, unique keys come only from the entity id (referencing clientPagingList
and the items/key lambda).
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt`:
- Around line 245-247: In ClientUpcomingChargesScreen.kt the fields `paid`,
`waived`, and `outstanding` are calling `.toString()` on nullable values
(`charge.amountPaid`, `charge.amountWaived`, `charge.amountOutstanding`) which
will render the literal "null"; change each to handle nulls using a safe call +
elvis (e.g., `charge.amountPaid?.toString() ?: ""` or a default like `"0"`) so
the UI shows a sensible default instead of "null" for `paid`, `waived`, and
`outstanding`.
---
Nitpick comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.kt`:
- Around line 344-346: Replace all uses of MaterialTheme with the project's
KptTheme in this file: change the color references that set surfaceColor,
outlineColor and cardColor (currently using MaterialTheme.colorScheme) to use
KptTheme.colorScheme, and replace all MaterialTheme.typography usages with
KptTheme.typography so theming is consistent across ClientListScreen (update the
declarations for surfaceColor, outlineColor, cardColor and the composables that
reference MaterialTheme.typography).
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt`:
- Around line 368-373: The current LaunchedEffect keyed on a freshly computed
offices list causes onUpdateOffices to run too often; compute a stable derived
state for offices (e.g., wrap items.map { it.officeName }.distinct() in remember
{ derivedStateOf { ... } } or use derivedStateOf with a Set for stable
equality), then key the LaunchedEffect on that derived state's value
(offices.value) so onUpdateOffices is only invoked when the distinct office set
actually changes; update references in ClientListScreen.kt to use the
derivedStateOf result and call onUpdateOffices(offices.value).
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt`:
- Line 292: The text style usage at the call site using
MaterialTheme.typography.bodyMedium should be replaced with the project-specific
typography to match the rest of the file: change the style reference to
MifosTypography.bodyMedium (or the corresponding MifosTypography token used in
lines 166/171) inside the ClientUpcomingChargesScreen composable so styling is
consistent with other text components.
- Line 270: Remove the unnecessary fully-qualified call to
org.jetbrains.compose.resources.stringResource inside the MifosSweetError
invocation and replace it with the already imported stringResource to match the
file's import style; update the call in the MifosSweetError(...) where the
message is Res.string.client_upcoming_charges_failed_message so it uses
stringResource(Res.string.client_upcoming_charges_failed_message) rather than
org.jetbrains.compose.resources.stringResource(...).
- Around line 239-244: The TODO about the "due" calculation in
ClientUpcomingChargesScreen (where due is computed from charge.amount and
charge.amountPaid) must be resolved: either confirm the business rule with
product/domain owners and, if correct, remove the TODO and add a short
clarifying comment stating the rule (e.g., due = amount - amountPaid) or, if
uncertain/known limitation, create a tracked issue in the repo referencing
ClientUpcomingChargesScreen and the due calculation, replace the TODO with a
one-line comment linking to that issue ID, and include expected behavior to
verify later.
- Around line 222-226: The current code calls MifosEmptyCard() when
chargesPagingList.loadState.refresh is NotLoading and itemCount == 0 but still
proceeds to render LazyColumn; change the control flow so LazyColumn is rendered
only when there are items (i.e. use an else branch) — locate the conditional
using chargesPagingList.loadState.refresh and itemCount and wrap the LazyColumn
block in the corresponding else so that MifosEmptyCard() and LazyColumn are
mutually exclusive.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 189bb74e-8909-46e8-be9b-4d074026cf1c
📒 Files selected for processing (25)
feature/client/build.gradle.ktsfeature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.ktfeature/client/src/androidMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.android.ktfeature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.ktfeature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ktfeature/client/src/desktopMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.desktop.ktfeature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.desktop.ktfeature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.desktop.ktfeature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.ktfeature/client/src/jsMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.js.ktfeature/client/src/jsMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.js.ktfeature/client/src/jsMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.js.ktfeature/client/src/jsMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.js.ktfeature/client/src/nativeMain/kotlin/com/mifos/feature/client/charges/ClientChargesScreen.native.ktfeature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.native.ktfeature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.native.ktfeature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.native.ktfeature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.wasmJs.ktfeature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.wasmJs.ktfeature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.wasmJs.ktfeature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.wasmJs.kt
💤 Files with no reviewable changes (20)
- feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.desktop.kt
- feature/client/src/jsMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.js.kt
- feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.native.kt
- feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.desktop.kt
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.android.kt
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.android.kt
- feature/client/src/nativeMain/kotlin/com/mifos/feature/client/charges/ClientChargesScreen.native.kt
- feature/client/src/jsMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.js.kt
- feature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.wasmJs.kt
- feature/client/src/desktopMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.desktop.kt
- feature/client/src/jsMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.js.kt
- feature/client/src/desktopMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.desktop.kt
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.android.kt
- feature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.wasmJs.kt
- feature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/charges/ShowClientCharge.wasmJs.kt
- feature/client/src/jsMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.js.kt
- feature/client/src/wasmJsMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.wasmJs.kt
- feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientList/ClientListScreen.native.kt
- feature/client/src/androidMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.android.kt
- feature/client/src/nativeMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.native.kt
Contributor
|
@techsavvy185 The screen recording should demonstrate the following actions:
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
110-116:⚠️ Potential issue | 🟠 MajorPass
isRefreshingfromstateto reflect actual refresh progress.
ClientListContentScreendefinesisRefreshingparameter (Line 208) but the call site at Lines 110-116 doesn't pass it, defaulting tofalse. This meansPullToRefreshBox(Line 225) never shows a loading indicator even whenRefreshClientsis triggered, breaking refresh UX feedback.Pass the refresh state from
statetoClientListContentScreen:
- Determine if refresh state exists in
ClientListState- Wire it to the
isRefreshingparameter at the call site🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt` around lines 110 - 116, ClientListContentScreen is not receiving the refresh flag so PullToRefreshBox never shows loading; update the call site where ClientListContentScreen(...) is invoked to pass the refresh state from your screen state (e.g. add isRefreshing = state.isRefreshing or the correct boolean property on ClientListState) and ensure the ClientListState exposes that boolean (e.g. refreshInProgress/ isRefreshing) so ClientListContentScreen's isRefreshing parameter reflects actual refresh progress.
♻️ Duplicate comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt (1)
436-439:⚠️ Potential issue | 🟡 MinorUse stable paging keys only; avoid index fallback.
Line 438 falls back to
indexas key when item is null. That key is unstable for paged content and can cause wrong item reuse during list updates.Suggested fix
+import androidx.paging.compose.itemKey ... items( count = clientPagingList.itemCount, - key = { index -> clientPagingList[index]?.id ?: index }, + key = clientPagingList.itemKey { it.id }, ) { index ->🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt` around lines 436 - 439, The key fallback to index is unstable; change the items block so the key uses only the stable item id and do not fall back to index when the item is null: inside the items(...) lambda, fetch the item once (val client = clientPagingList[index]), if client is null return@items (skip rendering that slot), and set key = { index -> clientPagingList[index]!!.id } (or compute the id from the same non-null client variable) so the key is always a stable item id and never the index.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt`:
- Around line 261-263: Replace the hardcoded empty-state text in the
ClientListScreen UI with a localized string: instead of MifosEmptyCard("No
clients found"), retrieve the string from composeResources/stringResource (e.g.,
call composeResources.stringResource(R.string.no_clients_found) or equivalent)
and pass that into MifosEmptyCard; ensure you add a corresponding entry
(no_clients_found) to the strings resource so the text is localizable.
- Around line 399-431: The current sorted branch converts Paging3's snapshot to
a static list (sortedItems) and calls items(items=sortedItems), which breaks
Paging3 pagination; revert the UI to always use the count-based API so Paging3
can trigger loads: use LazyColumn { items(count = clientPagingList.itemCount) {
index -> val client = clientPagingList[index]; LaunchedEffect(client?.id) { if
(client != null) fetchImage(client.id) }; ClientItem(client = client, byteArray
= images[client?.id], onClientClick = onClientSelect) } } and remove the
items(items = sortedItems, ...) branch; if true global sorting is required,
implement sort at the Pager/PagingSource level (or via PagingData
transformations) rather than in the composable.
---
Outside diff comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt`:
- Around line 110-116: ClientListContentScreen is not receiving the refresh flag
so PullToRefreshBox never shows loading; update the call site where
ClientListContentScreen(...) is invoked to pass the refresh state from your
screen state (e.g. add isRefreshing = state.isRefreshing or the correct boolean
property on ClientListState) and ensure the ClientListState exposes that boolean
(e.g. refreshInProgress/ isRefreshing) so ClientListContentScreen's isRefreshing
parameter reflects actual refresh progress.
---
Duplicate comments:
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt`:
- Around line 436-439: The key fallback to index is unstable; change the items
block so the key uses only the stable item id and do not fall back to index when
the item is null: inside the items(...) lambda, fetch the item once (val client
= clientPagingList[index]), if client is null return@items (skip rendering that
slot), and set key = { index -> clientPagingList[index]!!.id } (or compute the
id from the same non-null client variable) so the key is always a stable item id
and never the index.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45c1cd57-1c7d-4a96-9396-a3218bbfcbdc
📒 Files selected for processing (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt
Comment on lines
+261
to
+263
| else -> { | ||
| MifosEmptyCard("No clients found") | ||
| } |
There was a problem hiding this comment.
Localize the empty-state text instead of hardcoding it.
Line 262 uses a hardcoded user-facing string ("No clients found"). This should come from composeResources via stringResource(...).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt`
around lines 261 - 263, Replace the hardcoded empty-state text in the
ClientListScreen UI with a localized string: instead of MifosEmptyCard("No
clients found"), retrieve the string from composeResources/stringResource (e.g.,
call composeResources.stringResource(R.string.no_clients_found) or equivalent)
and pass that into MifosEmptyCard; ensure you add a corresponding entry
(no_clients_found) to the strings resource so the text is localizable.
Comment on lines
+399
to
+431
| if (sort != null) { | ||
| val currentItems = clientPagingList.itemSnapshotList.items | ||
|
|
||
| val sortedItems = when (sort) { | ||
| SortTypes.NAME -> { | ||
| currentItems.sortedBy { it.displayName?.lowercase() } | ||
| } | ||
| SortTypes.ACCOUNT_NUMBER -> { | ||
| currentItems.sortedBy { it.accountNo } | ||
| } | ||
| SortTypes.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, | ||
| ) | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt | sed -n '380,450p'Repository: openMF/mifos-x-field-officer-app
Length of output: 2823
🏁 Script executed:
# Search for the full conditional and both branches
rg -n -B10 "if \(sort != null\)" feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.ktRepository: openMF/mifos-x-field-officer-app
Length of output: 408
🏁 Script executed:
# Check if there's an unsorted branch that uses different paging logic
rg -n -A50 "if \(sort != null\)" feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt | head -100Repository: openMF/mifos-x-field-officer-app
Length of output: 1943
Sorting mode bypasses Paging3's pagination mechanism, stalling item loading when sort is active.
The sorted branch (line 399) uses itemSnapshotList.items and renders via items(items = sortedItems, ...), which provides a static list. The unsorted branch uses items(count = clientPagingList.itemCount, ...) with index-based access (clientPagingList[index]), enabling Paging3 to detect end-of-list and trigger pagination. While sorting, only currently-loaded items are sorted and displayed; new pages will not load as users scroll because the LazyColumn is no longer using Paging3's count-based mechanism to signal pagination.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientsList/ClientListScreen.kt`
around lines 399 - 431, The current sorted branch converts Paging3's snapshot to
a static list (sortedItems) and calls items(items=sortedItems), which breaks
Paging3 pagination; revert the UI to always use the count-based API so Paging3
can trigger loads: use LazyColumn { items(count = clientPagingList.itemCount) {
index -> val client = clientPagingList[index]; LaunchedEffect(client?.id) { if
(client != null) fetchImage(client.id) }; ClientItem(client = client, byteArray
= images[client?.id], onClientClick = onClientSelect) } } and remove the
items(items = sortedItems, ...) branch; if true global sorting is required,
implement sort at the Pager/PagingSource level (or via PagingData
transformations) rather than in the composable.
Contributor
|
@techsavvy185 Please upload screen recordings for all supported platforms covering all tests mentioned above. |
biplab1
approved these changes
Mar 10, 2026
biplab1
left a comment
biplab1
left a comment
Contributor
There was a problem hiding this comment.
Looks good to me. This can be merged.
|
biplab1
changed the title
2 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes - Jira-#616
Android
Screen_recording_20260309_233957.webm
iOS
Screen.Recording.2026-03-06.at.5.53.06.PM.mov
Desktop
Screen.Recording.2026-03-09.at.11.47.40.PM.mov
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