Features/recent transaction screen #3007
Conversation
WalkthroughRenamed Home navigation destination from Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as RecentTransactionScreen
participant VM as RecentTransactionViewModel
participant AccountsRepo as AccountsRepository
participant SavingsRepo as SavingsAccountRepository
participant Network as NetworkMonitor
User->>Screen: open RecentTransactionScreen
Screen->>VM: handleAction(LoadInitial)
par Parallel loads
VM->>AccountsRepo: fetchAccounts()
AccountsRepo-->>VM: AccountsLoaded(accounts)
and
VM->>SavingsRepo: loadTransactions(account?, offset?)
SavingsRepo-->>VM: TransactionsLoaded(items)
and
Network->>VM: network status updates
end
VM->>VM: compose RecentTransactionUiState (applyLocalFilters)
VM-->>Screen: emit uiState (Content / Loading / Error)
Screen->>Screen: render UI (list / sheet / error)
User->>Screen: open filter sheet
Screen->>VM: handleAction(ToggleFilter) / handleAction(ApplyFilter(...))
VM->>VM: applyLocalFilters -> update uiState
VM-->>Screen: emit updated uiState
Screen->>Screen: re-render filtered list
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🔇 Additional comments (2)
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 (5)
feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/utils/RecentTransactionState.kt (1)
39-61: Consider removing redundantlistproperty inViewState.Content.
RecentTransactionUiStatealready has atransactionsfield at lines 42, andViewState.Contentduplicates this with its ownlistproperty. This creates potential for state inconsistency if they diverge. Consider makingContenta data object likeEmpty, since the transactions are already available from the parent state.sealed interface ViewState { data object Loading : ViewState data object Empty : ViewState data class Error(val message: String?) : ViewState - data class Content(val list: List<Transactions>) : ViewState + data object Content : ViewState }feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/screen/RecentTransactionScreen.kt (4)
80-128: Move back button tonavigationIconslot.The back
IconButtonis placed inside thetitlecomposable, which is unconventional for Material Design. Use thenavigationIconslot ofTopAppBarfor proper layout and accessibility.TopAppBar( title = { Column { - Row(verticalAlignment = Alignment.CenterVertically) { - IconButton(onClick = navigateBack) { - Icon( - imageVector = MifosIcons.ArrowBack, - contentDescription = "Back", - ) - } - - Text( - text = "Transaction History", - style = MaterialTheme.typography.titleMedium.copy(fontWeight = FontWeight.Bold), - modifier = Modifier.padding(end = 8.dp), - ) - } + Text( + text = "Transaction History", + style = MaterialTheme.typography.titleMedium.copy(fontWeight = FontWeight.Bold), + ) state.selectedAccount?.let { account -> Text( text = account.accountNo ?: "Selected Account", ... ) } } }, + navigationIcon = { + IconButton(onClick = navigateBack) { + Icon( + imageVector = MifosIcons.ArrowBack, + contentDescription = "Back", + ) + } + }, actions = { ... }, )
190-191: Use theme colors instead of hardcoded values.Hardcoded
Color(0xFF0A7E48)andColor.Redwon't adapt to dark mode and may conflict with the app's color scheme. Consider defining semantic colors in the theme (e.g.,MaterialTheme.colorScheme.tertiaryfor credit,MaterialTheme.colorScheme.errorfor debit) or using a custom color palette.
256-282: Addkeyparameter and pagination trigger.
itemsIndexedshould use akeyparameter for stable item identity, improving performance and preventing UI glitches during list updates.- The
LoadMoreaction is defined but never triggered. Consider adding pagination detection.LazyColumn( modifier = modifier.fillMaxSize(), ) { itemsIndexed( items = transactions, + key = { _, transaction -> transaction.id ?: it } ) { index, transaction -> TransactionItem( transaction = transaction, ) if (index != transactions.size - 1) { HorizontalDivider(...) } + // Trigger pagination when reaching near the end + if (index == transactions.size - 5) { + // Consider calling viewModel.handleAction(RecentTransactionAction.LoadMore(...)) + } } }
354-414: Hardcoded colors break dark mode support.
Color.White,Color.Gray, andColor.Black(lines 361, 381, 387, 397) won't adapt to dark theme. Use semantic theme colors likeMaterialTheme.colorScheme.surface,MaterialTheme.colorScheme.onSurfaceVariant, andMaterialTheme.colorScheme.onSurface.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt(2 hunks)feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt(1 hunks)feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/navigation/HomeNavigation.kt(1 hunks)feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/screen/RecentTransactionScreen.kt(1 hunks)feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/utils/RecentTransactionState.kt(1 hunks)feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/viewmodel/RecentTransactionViewModel.kt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/viewmodel/RecentTransactionViewModel.kt (5)
feature/accounts/src/commonMain/kotlin/org/mifos/mobile/feature/accounts/accounts/AccountsViewModel.kt (1)
handleAction(45-89)feature/share-account/src/commonMain/kotlin/org/mifos/mobile/feature/shareaccount/shareAccount/ShareAccountViewModel.kt (1)
handleAction(56-84)feature/savings-account/src/commonMain/kotlin/org/mifos/mobile/feature/savingsaccount/savingsAccountDetails/SavingsAccountDetailsViewModel.kt (1)
handleAction(100-123)feature/client-charge/src/commonMain/kotlin/org/mifos/mobile/feature/charge/chargeDetails/ChargeDetailsViewModel.kt (1)
handleAction(55-65)feature/client-charge/src/commonMain/kotlin/org/mifos/mobile/feature/charge/charges/ClientChargeViewModel.kt (1)
handleAction(106-126)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks KMP / Static Analysis Check
🔇 Additional comments (9)
feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/navigation/HomeNavigation.kt (2)
36-36: LGTM!The new
AccountsWithTypedata class provides a clean, parameterized navigation destination for different account types.
32-32: Verify that all usages of the renamed destination have been updated.The renaming from
TransactiontoTransactionHistoryis a breaking change to the navigation API. Confirm all references throughout the codebase have been updated consistently by searching for any remaining references to the oldTransactiondestination name and verifying that all call sites now useTransactionHistory.feature/home/src/commonMain/kotlin/org/mifos/mobile/feature/home/HomeScreen.kt (1)
93-93: LGTM!The navigation update correctly uses the renamed
TransactionHistorydestination, consistent with the changes inHomeNavigation.kt.feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/utils/RecentTransactionState.kt (1)
66-81: LGTM! Well-structured action hierarchy.The separation of user-initiated actions from internal events using a nested
Internalsealed interface is consistent with other ViewModels in the codebase (e.g.,ClientChargeAction,ShareAccountsAction). This cleanly distinguishes public API from internal state machine events.feature/recent-transaction/src/commonMain/kotlin/org/mifos/mobile/feature/recent/transaction/viewmodel/RecentTransactionViewModel.kt (5)
49-64: LGTM! Proper reactive initialization.Network status monitoring with
launchInand clientId observation withcollectare correctly set up in the init block. Each clientId change triggeringLoadInitialhandles re-login scenarios appropriately.
126-140: LGTM! Account fetching logic is sound.The method correctly filters for active savings accounts and dispatches appropriate internal actions. The
clientIdbeing nullable is handled upstream by the init block flow that only triggersLoadInitialwhenclientIdis non-null.
162-183: Potential double error handling betweencatchandDataState.Error.Both the
catchoperator (line 167) andDataState.Errorbranch (line 177-179) callhandleAction(Internal.LoadFailed(...)). If the repository emitsDataState.Error, both paths could potentially execute, though typicallycatchhandles upstream exceptions whilecollecthandles downstream emissions.Verify whether this is the intended behavior or if the
catchshould be removed since error states are already handled viaDataState.Error.
216-228: LGTM! Robust transaction type detection.The
isTransactionCreditLogicmethod correctly prioritizes explicit boolean flags (deposit,withdrawal) before falling back to string matching on the transaction type value. This defensive approach handles various data representations well.
70-74: Pagination implementation requires manual verification.The review claims that the
offsetparameter inLoadMoreis unused andcanPaginateis alwaysfalse, but these assertions require direct codebase inspection to confirm. Verify:
- Whether
LoadMore.offsetis read anywhere in the action handler or related methods.- Whether
canPaginateis always set tofalseor if there are conditions that set it totrue.- If pagination is intentionally deferred, add a TODO comment; otherwise, remove unused code.
cmp-navigation/src/commonMain/kotlin/cmp/navigation/authenticated/AuthenticatedNavigation.kt
Outdated
Show resolved
Hide resolved
...monMain/kotlin/org/mifos/mobile/feature/recent/transaction/screen/RecentTransactionScreen.kt
Show resolved
Hide resolved
...monMain/kotlin/org/mifos/mobile/feature/recent/transaction/screen/RecentTransactionScreen.kt
Show resolved
Hide resolved
...monMain/kotlin/org/mifos/mobile/feature/recent/transaction/screen/RecentTransactionScreen.kt
Show resolved
Hide resolved
Removed duplicate navigation case for TransactionHistory.
3 participants
Fixes - MM-452
After
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 anything[] If you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.