fix(savedcards): refresh card list after delete and wire pull-to-refresh#2037
fix(savedcards): refresh card list after delete and wire pull-to-refresh#2037bhuvan-somisetty wants to merge 2 commits into
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughPull-to-refresh functionality is added to the saved cards feature. The ViewModel introduces a refresh trigger that re-fetches cards on explicit refresh or after deletion. The UI wires the scaffold to enable pull-to-refresh and dispatch the refresh action. ChangesPull-to-Refresh for Saved Cards
Sequence DiagramsequenceDiagram
participant User
participant CardsScreen
participant CardsScreenViewModel
participant Repository
User->>CardsScreen: Pull to refresh
CardsScreen->>CardsScreenViewModel: CardAction.RefreshCards
CardsScreenViewModel->>CardsScreenViewModel: emit to refreshTrigger
CardsScreenViewModel->>Repository: getSavedCards()
Repository-->>CardsScreenViewModel: cards
CardsScreenViewModel-->>CardsScreen: cardState updated
CardsScreen-->>User: Display refreshed cards
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@feature/savedcards/src/commonMain/kotlin/org/mifospay/feature/savedcards/CardsScreenViewModel.kt`:
- Line 106: CardAction.RefreshCards handling uses refreshTrigger.tryEmit(Unit)
which can silently drop events; replace both unchecked tryEmit calls (the one
handling CardAction.RefreshCards and the other at the later occurrence) with a
suspending emit invoked from the ViewModel's coroutine scope (e.g., launch {
refreshTrigger.emit(Unit) }) so the emission will suspend until buffer space is
available; update the CardsScreenViewModel to launch a coroutine on its existing
scope (e.g., viewModelScope / viewModelCoroutineScope / the class's
CoroutineScope) and call refreshTrigger.emit(Unit) for both refresh locations to
ensure refreshes are not lost and cardState stays consistent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a73804af-18e6-4a80-a32a-ec286eff2788
📒 Files selected for processing (2)
feature/savedcards/src/commonMain/kotlin/org/mifospay/feature/savedcards/CardsScreen.ktfeature/savedcards/src/commonMain/kotlin/org/mifospay/feature/savedcards/CardsScreenViewModel.kt
| } | ||
| } | ||
|
|
||
| is CardAction.RefreshCards -> refreshTrigger.tryEmit(Unit) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In kotlinx.coroutines MutableSharedFlow, under what conditions can tryEmit return false (especially with replay = 1 and default onBufferOverflow), and is emit recommended when refresh events must not be dropped?
💡 Result:
In Kotlin's MutableSharedFlow, the tryEmit function returns false only when the BufferOverflow strategy is set to SUSPEND (the default) and there are active subscribers that are not ready to accept the new value [1][2][3]. When you configure a MutableSharedFlow with replay = 1 and the default onBufferOverflow = BufferOverflow.SUSPEND, tryEmit will return false if the internal buffer is full and at least one subscriber is currently unable to process the emission [1][4][5]. Because the default extraBufferCapacity is 0, the flow has no additional room to buffer values beyond the replay cache [4][6]. If a subscriber is slow or busy, the flow cannot immediately place the new value, causing tryEmit to fail [6][5]. If your application requires that refresh events must not be dropped, using tryEmit is not recommended [6][5]. Instead, you should use the suspending emit function [5][7]. The emit function will suspend the producer coroutine until there is sufficient buffer space available to deliver the value to all subscribers, ensuring that no events are lost due to backpressure [8][2][5]. To safely handle events that must not be dropped: 1. Use emit to ensure the producer waits for subscribers [5][7]. 2. If you must use tryEmit (e.g., in a non-suspending context), ensure you have configured sufficient extraBufferCapacity to handle expected bursts of events, though this only delays the point at which backpressure occurs rather than eliminating it [6][5]. 3. If the producer cannot suspend, consider using a different architecture, such as a Channel or a dedicated event bus, that better aligns with your specific requirements for event delivery and backpressure [5].
Citations:
- 1: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-mutable-shared-flow/try-emit.html
- 2: https://medium.com/%40shinoogoyalaggarwal/sharedflow-tryemit-vs-emit-1000f12b3236
- 3: Unexpected tryEmit behaviour Kotlin/kotlinx.coroutines#2387
- 4: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-mutable-shared-flow.html
- 5: https://oleksii-tym.medium.com/sharedflow-deep-dive-emit-vs-tryemit-and-using-replay-and-buffercapacity-correctly-a1af3eddd778
- 6: https://medium.com/@yassine.sayah/default-vs-buffered-sharedflow-in-kotlin-a-practical-guide-6abe92aad7d7
- 7: https://medium.com/@jatingujjar646/difference-between-emit-and-tryemit-in-kotlin-flow-7688f99e86ad
- 8: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-core/kotlinx.coroutines.flow/-mutable-shared-flow/
🏁 Script executed:
# First, find the exact file location
find . -name "CardsScreenViewModel.kt" -type f 2>/dev/nullRepository: openMF/mifos-pay
Length of output: 159
🏁 Script executed:
cat -n "./feature/savedcards/src/commonMain/kotlin/org/mifospay/feature/savedcards/CardsScreenViewModel.kt"Repository: openMF/mifos-pay
Length of output: 10058
Use non-lossy refresh emission instead of unchecked tryEmit.
Both refresh paths at lines 106 and 159 ignore tryEmit's boolean result. With MutableSharedFlow<Unit>(replay = 1) and default BufferOverflow.SUSPEND, if a subscriber is slow or the internal buffer fills, tryEmit silently fails and drops the refresh event. Since cardState depends on refreshTrigger to refresh the card list, dropped emissions will leave stale data on screen—undoing the fix this PR provides.
Replace with the non-blocking suspending emit pattern below, which waits for buffer space instead of silently dropping:
🔧 Proposed fix
+ private fun requestRefresh() {
+ viewModelScope.launch {
+ refreshTrigger.emit(Unit)
+ }
+ }
override fun handleAction(action: CardAction) {
when (action) {
// ...
- is CardAction.RefreshCards -> refreshTrigger.tryEmit(Unit)
+ is CardAction.RefreshCards -> requestRefresh()
// ...
}
}
private fun handleCardDeleteResult(action: HandleCardDeleteResult) {
when (action.result) {
is DataState.Success -> {
mutableStateFlow.update {
it.copy(dialogState = null)
}
- refreshTrigger.tryEmit(Unit)
+ requestRefresh()
sendEvent(CardEvent.ShowToast(action.result.data))
}
}
}📝 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 CardAction.RefreshCards -> refreshTrigger.tryEmit(Unit) | |
| private fun requestRefresh() { | |
| viewModelScope.launch { | |
| refreshTrigger.emit(Unit) | |
| } | |
| } | |
| override fun handleAction(action: CardAction) { | |
| when (action) { | |
| // ... | |
| is CardAction.RefreshCards -> requestRefresh() | |
| // ... | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@feature/savedcards/src/commonMain/kotlin/org/mifospay/feature/savedcards/CardsScreenViewModel.kt`
at line 106, CardAction.RefreshCards handling uses refreshTrigger.tryEmit(Unit)
which can silently drop events; replace both unchecked tryEmit calls (the one
handling CardAction.RefreshCards and the other at the later occurrence) with a
suspending emit invoked from the ViewModel's coroutine scope (e.g., launch {
refreshTrigger.emit(Unit) }) so the emission will suspend until buffer space is
available; update the CardsScreenViewModel to launch a coroutine on its existing
scope (e.g., viewModelScope / viewModelCoroutineScope / the class's
CoroutineScope) and call refreshTrigger.emit(Unit) for both refresh locations to
ensure refreshes are not lost and cardState stays consistent.
Signed-off-by: bhuvan-somisetty <somisettybhuvan5@gmail.com>
tryEmit on a replay=1 SharedFlow with default BufferOverflow.SUSPEND can silently drop the refresh event if a subscriber isn't ready, leaving stale data on screen. Switch both refresh call sites to a suspending emit via a shared helper so the event is never dropped.
bhuvan-somisetty
force-pushed
the
fix/savedcards-delete-stale-state
branch
from
def4cf3 to
f29ff45
Compare
|
Rebased on latest dev and pushed a fix for the CodeRabbit finding: both refresh call sites now go through a |
1 participant
Deleting a saved card left the list showing the deleted entry until you navigated away and back. The root cause was that
cardStatewas built directly from a one-shot API flow: once it emitted and completed, there was nothing to trigger a re-fetch after a delete.The fix introduces a
MutableSharedFlow<Unit>as a refresh trigger (seeded with an initial emission so the first load still happens automatically). ThecardStateflow now usesflatMapLateston that trigger, so everytryEmit(Unit)restarts the underlyinggetSavedCardscall and the list updates immediately.Two places now emit to the trigger:
CardAction.RefreshCardsis dispatched, which the screen sends on pull-to-refreshThe
isPullRefreshEnabledproperty was already defined onViewStatebut the scaffold was never wired up to it. This PR connects it viarememberMifosPullToRefreshState, so users can also manually refresh the list by pulling down.The known-issue comment on
CardsScreenis removed since the underlying problem is resolved.Tested manually: deleting a card now removes it from the list without any navigation, and pull-to-refresh re-fetches correctly.
Summary by CodeRabbit