fix(history): Fetch transactions only for selected account#1953
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRemoved the combined accounts-with-transactions repository API; refactored History to load accounts then transactions sequentially, renamed SetFilter → SetTransactionType, simplified History state and UI bindings, and added a "No accounts found" string resource. Changes
Sequence DiagramsequenceDiagram
participant UI as HistoryScreen
participant VM as HistoryViewModel
participant Repo as SelfServiceRepository
participant State as StateStore
UI->>VM: Init
note over VM: init -> loadActiveAccounts()
VM->>Repo: getActiveAccounts(clientId)
Repo-->>VM: DataState<List<Account>>
VM->>State: update accounts, set selectedAccount
alt accounts exist
VM->>Repo: getTransactions(accountId)
Repo-->>VM: DataState<List<Transaction>>
VM->>VM: apply selectedTransactionType filter
VM->>State: update viewState with filtered transactions
else no accounts
VM->>State: emit viewState Error (feature_history_no_account)
end
UI->>VM: HistoryAction.SetTransactionType / SetSelectedAccount / ApplyFilter
VM->>VM: decide reload vs local filter
VM->>State: emit updated ViewState
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt (1)
151-164: Unfiltered empty list should surfaceViewState.Empty.
applyFilteralways emitsContent, even when the base list is empty and no filter is applied. That forces the UI into the “filtered empty” copy for a “no transactions yet” case. Consider emittingEmptywhenfilter == OTHERand the list is empty.💡 Proposed refinement
- mutableStateFlow.update { - it.copy( - viewState = HistoryState.ViewState.Content(filteredTransactions), - ) - } + mutableStateFlow.update { + it.copy( + viewState = if (filteredTransactions.isEmpty() && filter == TransactionType.OTHER) { + HistoryState.ViewState.Empty + } else { + HistoryState.ViewState.Content(filteredTransactions) + }, + ) + }
🤖 Fix all issues with AI agents
In
`@feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt`:
- Around line 59-77: The DataState.Error branch should not map to
HistoryState.ViewState.Empty while leaving prior transactions in state; update
the is DataState.Error case in mutableStateFlow.update to clear the transactions
(set transactions = emptyList()) and emit an error view state (e.g.,
HistoryState.ViewState.Error with the error/exception from the DataState
instance) instead of HistoryState.ViewState.Empty so stale data is removed and
the UI can surface the failure.
- Around line 117-126: The current logic in HistoryViewModel skips handling when
state.transactions is empty, so switching to an account with zero previously
loaded transactions never triggers loadTransactions; change the branch to always
attempt to load transactions for the currently selected account unless the
existing transactions are non-empty and already belong to that account—i.e., in
the block referencing state.transactions, state.currentSelectedAccount,
applyFilter, and loadTransactions, replace the guard that checks only
state.transactions.isNotEmpty() with logic that checks whether
state.transactions is non-empty AND the first transaction's accountId equals
state.currentSelectedAccount?.id (then call applyFilter), otherwise call
loadTransactions(state.currentSelectedAccount!!.id) when a
currentSelectedAccount exists.
- Around line 85-99: The loadActiveAccountsAndInitialTransactions function
currently only handles the Success case from repository.getActiveAccounts and
leaves mutableStateFlow unchanged on Error or empty results; update the
collector on repository.getActiveAccounts(state.clientId) to explicitly handle
all Resource states (e.g., Loading, Success, Error) and update mutableStateFlow
accordingly (set a loading flag on Loading, set an error state/message on
Error), and when Success contains an empty list emit an Empty/appropriate state
instead of leaving the UI stuck; ensure you still call
loadTransactions(accountId) only when a non-empty accounts list exists and use
viewModelScope.launchIn as before.
- Around line 82-83: The loadTransactions flow can produce stale updates when
multiple calls run concurrently; in HistoryViewModel, introduce a Job property
(e.g., private var loadJob: Job? = null) and in the function that calls
loadTransactions cancel loadJob?.cancel() before launching the new collector
with viewModelScope and assign the returned Job to loadJob, and additionally,
before applying any result to state verify it matches the current selection
(e.g., compare result.accountId to the ViewModel's
currentSelection/currentlySelectedAccountId) so late emissions are ignored;
update loadTransactions callers (the sites from which it is invoked) to rely on
this guarded/canceling behavior.
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt
Outdated
Show resolved
Hide resolved
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt
Show resolved
Hide resolved
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt
Outdated
Show resolved
Hide resolved
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt
Outdated
Show resolved
Hide resolved
sahilshivekar
force-pushed
the
feature/mw-326-transactions-clean-up
branch
from
b2fb02b to
691faea
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@feature/history/src/commonMain/composeResources/values/strings.xml`:
- Line 26: The string resource feature_history_no_account currently contains
literal quotation marks; remove the surrounding double quotes from the value so
it reads No accounts found (i.e., update the <string
name="feature_history_no_account"> entry to contain the plain text without
enclosing " characters) to match other strings and prevent quotes from appearing
in the UI.
In
`@feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt`:
- Around line 75-84: In HistoryViewModel's success branch, replace the call to
applyFilter(state.transactions) with applyFilter(transactions) so the freshly
received local variable is used; locate the block updating mutableStateFlow with
HistoryState.ViewState.Content(transactions) and pass the local transactions
variable to applyFilter to avoid reading from state after the update.
🧹 Nitpick comments (2)
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt (2)
63-67: Consider clearing transactions on error to avoid stale data.The error handling now correctly emits an error state (addressing the previous concern). However, the
transactionslist isn't cleared, meaning stale data from a previous account could resurface if the user switches back or retries. Consider clearing transactions when an error occurs.Proposed fix
is DataState.Error -> { mutableStateFlow.update { - it.copy(viewState = HistoryState.ViewState.Error(Res.string.feature_history_error)) + it.copy( + transactions = emptyList(), + viewState = HistoryState.ViewState.Error(Res.string.feature_history_error), + ) } }
113-128: Update state before triggering transaction load for consistent ordering.Currently,
loadTransactionsis called before updating the state with accounts andselectedAccount. While this works becauseloadTransactionsruns asynchronously, updating the state first provides clearer data flow and ensuresselectedAccountis set if any synchronous access occurs.Proposed reordering
is DataState.Success -> { val accounts = result.data if (accounts.isNotEmpty()) { - loadTransactions(accounts.first().id) mutableStateFlow.update { state -> state.copy( accounts = accounts, selectedAccount = accounts.firstOrNull(), ) } + loadTransactions(accounts.first().id) } else {
feature/history/src/commonMain/composeResources/values/strings.xml
Outdated
Show resolved
Hide resolved
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt
Outdated
Show resolved
Hide resolved
sahilshivekar
force-pushed
the
feature/mw-326-transactions-clean-up
branch
from
35d8930 to
56dac72
Compare
|
@sahilshivekar Since it is a refactor, please add a screen recording to ensure there are no regressions. |
sahilshivekar
force-pushed
the
feature/mw-326-transactions-clean-up
branch
from
56dac72 to
74b2623
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt (2)
75-84: Redundant intermediateContentemission beforeapplyFilter.Lines 78–83 write
Content(transactions)(unfiltered), andapplyFilteron line 84 immediately overwrites it withContent(filteredTransactions). WhenselectedTransactionType != OTHER, this produces an unnecessary state emission that briefly exposes unfiltered data. Removing theviewStateassignment from the first update and relying solely onapplyFiltereliminates the redundancy.♻️ Proposed fix
is DataState.Success -> { val transactions = result.data if (transactions.isNotEmpty()) { mutableStateFlow.update { it.copy( transactions = transactions, - viewState = HistoryState.ViewState.Content(transactions), ) } applyFilter(transactions) } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt` around lines 75 - 84, Remove the redundant unfiltered Content emission by updating the mutableStateFlow only with transactions (not viewState) before calling applyFilter; specifically, in the DataState.Success branch where result.data is assigned to transactions, stop setting viewState = HistoryState.ViewState.Content(transactions) in that first mutableStateFlow.update and instead let applyFilter(transactions) compute and set the proper HistoryState.ViewState.Content (filtered) based on selectedTransactionType, so only the final filtered state is emitted.
123-127:ViewState.Erroris semantically incorrect for "no accounts found."An empty account list is an informational/empty condition, not an error. Reusing
ViewState.Errorfor it means the UI will render the error UI (e.g. "Oops!") rather than a neutral empty message, which degrades UX. Consider adding amessageparameter toViewState.Empty, or introducing a dedicatedViewState.NoAccountsvariant.♻️ Proposed alternative
-data object Empty : ViewState +data class Empty(val message: StringResource? = null) : ViewStateThen in
loadActiveAccounts:-mutableStateFlow.update { - it.copy(viewState = HistoryState.ViewState.Error(Res.string.feature_history_no_account)) -} +mutableStateFlow.update { + it.copy(viewState = HistoryState.ViewState.Empty(Res.string.feature_history_no_account)) +}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt` around lines 123 - 127, The code sets ViewState.Error for the "no accounts" condition which is semantically wrong; update the ViewState model and the HistoryViewModel logic so the UI shows a neutral empty state: either (A) add an optional message parameter to ViewState.Empty (e.g., Empty(message: String?)) and set viewState = HistoryState.ViewState.Empty(Res.string.feature_history_no_account), or (B) introduce a dedicated variant ViewState.NoAccounts (or NoItems) and set viewState = HistoryState.ViewState.NoAccounts when loadActiveAccounts finds zero accounts; then replace the mutableStateFlow.update call that currently sets ViewState.Error with the new Empty(message) or NoAccounts variant and update any UI consumers to handle the new variant.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In
`@feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt`:
- Around line 63-67: The Error branch in HistoryViewModel currently sets
viewState to HistoryState.ViewState.Error but leaves stale state.transactions
intact; update the is DataState.Error handling in HistoryViewModel (the
mutableStateFlow.update call) to also clear or set transactions = emptyList()
(and any related filtered lists) so subsequent handleApplyFilterClick, which
reads state.transactions, won’t re-apply stale data; ensure the
mutableStateFlow.update modifies the state object to reset transactions before
setting viewState to HistoryState.ViewState.Error.
---
Nitpick comments:
In
`@feature/history/src/commonMain/kotlin/org/mifospay/feature/history/HistoryViewModel.kt`:
- Around line 75-84: Remove the redundant unfiltered Content emission by
updating the mutableStateFlow only with transactions (not viewState) before
calling applyFilter; specifically, in the DataState.Success branch where
result.data is assigned to transactions, stop setting viewState =
HistoryState.ViewState.Content(transactions) in that first
mutableStateFlow.update and instead let applyFilter(transactions) compute and
set the proper HistoryState.ViewState.Content (filtered) based on
selectedTransactionType, so only the final filtered state is emitted.
- Around line 123-127: The code sets ViewState.Error for the "no accounts"
condition which is semantically wrong; update the ViewState model and the
HistoryViewModel logic so the UI shows a neutral empty state: either (A) add an
optional message parameter to ViewState.Empty (e.g., Empty(message: String?))
and set viewState =
HistoryState.ViewState.Empty(Res.string.feature_history_no_account), or (B)
introduce a dedicated variant ViewState.NoAccounts (or NoItems) and set
viewState = HistoryState.ViewState.NoAccounts when loadActiveAccounts finds zero
accounts; then replace the mutableStateFlow.update call that currently sets
ViewState.Error with the new Empty(message) or NoAccounts variant and update any
UI consumers to handle the new variant.
revanthkumarJ
left a comment
revanthkumarJ
left a comment
There was a problem hiding this comment.
looks good to me see if we can add another account and show in video how it works when user changes account from drop down
@revanthkumarJ I have uploaded new videos with description and logs as a proof since nothing changes on UI. |
sahilshivekar
force-pushed
the
feature/mw-326-transactions-clean-up
branch
2 times, most recently
from
8964877 to
8a5a0d1
Compare
revanthkumarJ
left a comment
revanthkumarJ
left a comment
There was a problem hiding this comment.
fix static checks remaining fine
@revanthkumarJ Something is going wrong, check this https://mifos.slack.com/archives/C5TTJS4BX/p1772353496479949?thread_ts=1768831891.339249&cid=C5TTJS4BX |
sahilshivekar
force-pushed
the
feature/mw-326-transactions-clean-up
branch
from
90ba43f to
18b842f
Compare
4 participants
Jira Task: MW-326
Description
Before HistoryViewModel was fetching transactions for all the accounts initially.
Now it fetches transactions only for the selected account.
Behavior of the app on screen doesn't change.
Second savings account of user have 4 transactions on 28 feb 2026. (first savings account have no transactions on 28 feb 2026)
Screen recording
Before
Those 4 transactions of not selected account are fetched initially.
mw-326-old.mp4
After
Those 4 transactions of not selected account are fetched after selecting that account.
mw-326-new.mp4
Summary by CodeRabbit
Refactor
UI / Behavior
Bug Fixes
Chores