fix(groups): Migrate Paging to commonMain from androidMain - #2636
Conversation
…oving compatibility with other platforms.
📝 WalkthroughWalkthroughConsolidates the GroupsList UI into commonMain by adding a concrete GroupsListScreen and GroupsListRoute, moves paging and backhandler dependencies to commonMain, and removes platform-specific actual/stub implementations (android, desktop, js, native, wasmJs). Changes
Sequence Diagram(s)sequenceDiagram
participant UI as "Compose UI\n(GroupsListScreen)"
participant VM as "GroupsListViewModel"
participant Paging as "PagingData\n/ Pager"
participant Nav as "Navigation"
rect rgba(135,206,235,0.5)
UI->>VM: request paged items / refresh
VM->>Paging: load page
Paging-->>VM: PagingData<PagedItem>
VM-->>UI: provide PagingData (collectAsLazyPagingItems)
end
rect rgba(144,238,144,0.5)
UI->>UI: user selects / long-press item
UI->>VM: notify selection or action (delete/sync)
UI->>Nav: onGroupClick -> navigate to detail
end
rect rgba(255,182,193,0.5)
UI->>UI: BackHandler clears selection
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
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: 4
🧹 Nitpick comments (1)
feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt (1)
114-119: Prefer stable-id selection checks over full-entity equality.Current selection toggling and
isSelectedchecks rely onGroupEntityequality. With paging updates, new instances for the same group can break selection continuity.Proposed refactor
onSelectItem = { - if (selectedItems.contains(it)) { - selectedItems.remove(it) - } else { - selectedItems.add(it) - } + it.id?.let { candidateId -> + val selectedIndex = selectedItems.indexOfFirst { selected -> selected.id == candidateId } + if (selectedIndex >= 0) { + selectedItems.removeAt(selectedIndex) + } else { + selectedItems.add(it) + } + } }, @@ successState( pagingItems = data, isInSelectionMode = selectedItems.isNotEmpty(), - isSelected = selectedItems::contains, + isSelected = { item -> + item.id?.let { id -> selectedItems.any { selected -> selected.id == id } } == true + }, onGroupClick = onGroupClick, onSelectItem = onSelectItem, )Also applies to: 215-217
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt` around lines 114 - 119, The selection toggle currently compares full GroupEntity instances (selectedItems.contains(it)) which breaks when paging creates new instances; change selection to use the group's stable id instead: store and check IDs (e.g., selectedItemIds) rather than GroupEntity objects, update the onSelectItem lambda to add/remove the group's id (use the unique id property on GroupEntity) and update all isSelected checks to reference that id-set (also apply the same change for the other occurrence around lines 215-217). Ensure helper names referenced (selectedItems → selectedItemIds, onSelectItem, any isSelected or selection-check logic) are updated consistently so selection survives entity instance changes.
🤖 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/groups/build.gradle.kts`:
- Line 35: The dependency implementation(libs.androidx.paging.compose) is
declared in commonMain but it’s Android-only; move the
implementation(libs.androidx.paging.compose) declaration out of commonMain and
add it under the androidMain source set (where platform-specific deps belong)
while keeping libs.androidx.paging.common in commonMain; update the
multiplatform sourceSets block to place paging-compose under androidMain
(reference the existing implementation(libs.androidx.paging.compose) token and
the sourceSets/androidMain block) so non-Android targets (desktop/iOS/web) no
longer depend on an Android-only artifact.
In
`@feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt`:
- Around line 105-124: The pull-to-refresh UI isn't tied to the Paging refresh
state: update calls to GroupsListScreen so its isRefreshing parameter is set
from the PagingData's load state (e.g., isRefreshing = data.loadState.refresh is
LoadState.Loading) instead of the default false; inside GroupsListRoute locate
the data variable (PagingData/LazyPagingItems) and pass isRefreshing as
described to GroupsListScreen (and make the same change for the other
GroupsListScreen invocations referenced around the other call sites), ensuring
you import androidx.paging.LoadState and use the Paging loadState.refresh to
drive the refresh indicator and swipe-to-refresh behavior.
- Around line 147-149: The hide callback for the dialog in GroupsListScreen (the
hide = { ... } block) is a no-op and must be implemented to perform the intended
"hide" behavior distinct from dismiss; update the hide lambda to call the same
state-change or helper used by dismiss (e.g., the dialogVisible/dialogShown
state setter or the existing dismiss handler) or invoke the ViewModel action
that hides the dialog without clearing selection, ensuring it uses the same
unique symbols used elsewhere (hide lambda, dismiss handler, and the ViewModel
method that toggles dialog visibility) so both dialog paths perform the correct
state transition.
- Around line 190-195: The Column currently only applies a conditional top inset
via padding(top = if (selectedItems.isNotEmpty())
paddingValues.calculateTopPadding() else 0.dp), which drops start/bottom/end
scaffold insets; replace that conditional top-only padding with applying the
full Scaffold content insets using padding(paddingValues) (or combine
padding(paddingValues) with any additional conditional top offset if needed) on
the Column modifier so the list respects start/bottom/end insets and avoids
being clipped by FAB/system bars; look for the Column, selectedItems, and
paddingValues.calculateTopPadding() usage in GroupsListScreen to make this
change.
---
Nitpick comments:
In
`@feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt`:
- Around line 114-119: The selection toggle currently compares full GroupEntity
instances (selectedItems.contains(it)) which breaks when paging creates new
instances; change selection to use the group's stable id instead: store and
check IDs (e.g., selectedItemIds) rather than GroupEntity objects, update the
onSelectItem lambda to add/remove the group's id (use the unique id property on
GroupEntity) and update all isSelected checks to reference that id-set (also
apply the same change for the other occurrence around lines 215-217). Ensure
helper names referenced (selectedItems → selectedItemIds, onSelectItem, any
isSelected or selection-check logic) are updated consistently so selection
survives entity instance changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ee461ccc-c6e2-4ae0-8e3d-83dc5f5cc6cf
📒 Files selected for processing (7)
feature/groups/build.gradle.ktsfeature/groups/src/androidMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.android.ktfeature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.ktfeature/groups/src/desktopMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.desktop.ktfeature/groups/src/jsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.js.ktfeature/groups/src/nativeMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.native.ktfeature/groups/src/wasmJsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.wasmJs.kt
💤 Files with no reviewable changes (5)
- feature/groups/src/wasmJsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.wasmJs.kt
- feature/groups/src/jsMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.js.kt
- feature/groups/src/nativeMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.native.kt
- feature/groups/src/desktopMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.desktop.kt
- feature/groups/src/androidMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.android.kt
|
@techsavvy185 The screen recording should demonstrate the following actions:
|
|
@techsavvy185 Please upload screen recordings for all supported platforms covering all tests mentioned above. |
biplab1
changed the title
|
@techsavvy185 Can you please address the @coderabbitai reviews? |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 26068
❌ Actionable Comments (4)1. 2. isRefreshing = data.loadState.refresh is LoadState.Loading3. 4. Modifier.padding(paddingValues) // Apply full scaffold insets💡 Nitpick Comment (1)5. // Toggle by ID
it.id?.let { candidateId ->
val selectedIndex = selectedItems.indexOfFirst { selected -> selected.id == candidateId }
if (selectedIndex >= 0) selectedItems.removeAt(selectedIndex)
else selectedItems.add(it)
}
// Check selection by ID
isSelected = { item ->
item.id?.let { id -> selectedItems.any { selected -> selected.id == id } } == true
},Please address items 1–4 (actionable) as a priority, and consider item 5 (nitpick) as well since it can cause subtle bugs during pagination. Let me know if you need more details on any of these! |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt (1)
95-97: Consider selection by stable ID instead of full entity equality.Storing full
GroupEntityobjects and usingcontains/remove/addrelies on object equality. When paging emits new instances after refresh or page loads, the same logical group may have a different object reference, causing selection state to be lost.♻️ Proposed refactor to use ID-based selection
- val selectedItems = remember { - mutableStateListOf<GroupEntity>() - } + val selectedItemIds = remember { + mutableStateListOf<Int>() + }Then update the toggle logic:
onSelectItem = { - if (selectedItems.contains(it)) { - selectedItems.remove(it) + it.id?.let { id -> + if (selectedItemIds.contains(id)) { + selectedItemIds.remove(id) + } else { + selectedItemIds.add(id) + } - } else { - selectedItems.add(it) } },And update
isSelectedcheck accordingly:- isSelected = selectedItems::contains, + isSelected = { group -> group.id in selectedItemIds },Also applies to: 114-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt` around lines 95 - 97, Currently selectedItems stores GroupEntity instances which breaks when paging emits new instances; change selection to store stable IDs (e.g., Long or String) instead of full GroupEntity objects: replace mutableStateListOf<GroupEntity>() with a mutableStateListOf<IdType>() or a MutableStateSet of IDs (referencing selectedItems), update any toggleSelection logic that previously added/removed GroupEntity to add/remove group.id (use the group.id value from the UI item and functions like toggleSelection(groupId)), and update isSelected checks to compare against IDs (e.g., isSelected = selectedItems.contains(group.id)). Also update any other uses around the toggle and selection checks (the code referenced near lines 114-119) to operate on IDs instead of object equality.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt`:
- Around line 95-97: Currently selectedItems stores GroupEntity instances which
breaks when paging emits new instances; change selection to store stable IDs
(e.g., Long or String) instead of full GroupEntity objects: replace
mutableStateListOf<GroupEntity>() with a mutableStateListOf<IdType>() or a
MutableStateSet of IDs (referencing selectedItems), update any toggleSelection
logic that previously added/removed GroupEntity to add/remove group.id (use the
group.id value from the UI item and functions like toggleSelection(groupId)),
and update isSelected checks to compare against IDs (e.g., isSelected =
selectedItems.contains(group.id)). Also update any other uses around the toggle
and selection checks (the code referenced near lines 114-119) to operate on IDs
instead of object equality.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 53de6707-b3d3-4150-b101-3eb8ba222f04
📒 Files selected for processing (1)
feature/groups/src/commonMain/kotlin/com/mifos/feature/groups/groupList/GroupsListScreen.kt
|
|
@techsavvy185 Have you tested the screen after the recent updates were pushed? |
|
@biplab1 Yes, I've tested it. It looks exactly the same as before. |
biplab1
left a comment
biplab1
left a comment
There was a problem hiding this comment.
Looks good to me. This can be merged.
3 participants
Fixes - Jira-#616
Android
Screen_recording_20260309_235730.webm
iOS
Screen.Recording.2026-03-05.at.10.58.00.PM.mov
Desktop
Screen.Recording.2026-03-10.at.12.01.25.AM.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