fix(client): Added Create Account buttons for all types of accounts. #2583
Conversation
📝 WalkthroughWalkthroughAdds a createAccount callback across multiple account screens/routes, wires AddAccount UI buttons to ViewModel actions/events, and connects those events to navigation helpers for creating new accounts. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Screen as AccountScreen
participant VM as ViewModel
participant Nav as NavController
User->>Screen: taps Add button
Screen->>VM: onAction(AddAccount)
VM->>VM: emit Event.AddAccount(clientId)
VM->>Screen: sendEvent(AddAccount(clientId))
Screen->>Nav: createAccount(clientId) -- invokes navigation helper
Nav->>Nav: navigate to CreateAccountRoute(clientId)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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
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: 3
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/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
244-248: Pre-existing bug:ApproveAccountaction is not dispatched.The
ApproveAccountaction is constructed but never sent viaonAction(), so the approve functionality won't work. Compare withViewAccount(lines 237-243) which correctly callsonAction(...).🐛 Proposed fix
is Actions.ApproveAccount -> { - RecurringDepositAccountAction.ApproveAccount( + onAction(RecurringDepositAccountAction.ApproveAccount( recurringDeposit.accountNo ?: "", - ) + )) }
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 242-249: Replace the null content description on the add-icon
button so screen readers can announce it: in the Icon inside IconButton (the one
invoking onAction(ClientLoanAccountsAction.AddAccount) and using
painterResource(Res.drawable.add_icon)), set contentDescription to a meaningful
string (preferably via a string resource, e.g.
stringResource(R.string.add_account) or a localized label like "Add account")
instead of null so the button is accessible.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt`:
- Around line 13-14: The Add icon in FixedDepositAccountScreen (the clickable
add icon near where the composable shows an empty state) has a null
contentDescription, making it inaccessible; update the composable that renders
the add_icon (in FixedDepositAccountScreen) to pass a meaningful, localized
contentDescription string (create a new string resource like "add_fixed_deposit"
or reuse an appropriate existing resource) and wire it via stringResource(...)
so screen readers announce the action; ensure the contentDescription is non-null
for the Image/Icon composable and update any related preview/tests to reflect
the new resource.
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt`:
- Around line 254-262: Remove the no-op DesignToken.padding expression and
replace it with a proper Spacer using Modifier.width or Modifier.padding as
appropriate (replace the standalone DesignToken.padding near the IconButton);
also fix accessibility by providing a meaningful contentDescription for the Icon
(do not use contentDescription = null) — either hardcode a short label like "Add
account" or reference a string resource, and ensure the IconButton callback
remains onAction.invoke(SavingsAccountAction.AddAccount).
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (1)
186-202: Consider adding meaningfulcontentDescriptionfor accessibility.The new Add icon (and the existing Search/Filter icons) have
contentDescription = null, which makes them inaccessible to screen readers. For interactive icons, providing descriptive labels improves accessibility.♿ Suggested improvement for accessibility
Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.ToggleSearchBar) }, painter = painterResource(Res.drawable.search), - contentDescription = null, + contentDescription = "Search accounts", ) Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased)) Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.AddAccount) }, painter = painterResource(Res.drawable.add_icon), - contentDescription = null, + contentDescription = "Add account", ) Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased)) Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.ToggleFiler) }, painter = painterResource(Res.drawable.filter), - contentDescription = null, + contentDescription = "Filter accounts", )
| IconButton( | ||
| onClick = { onAction.invoke(ClientLoanAccountsAction.AddAccount) }, | ||
| ) { | ||
| Icon( | ||
| painter = painterResource(Res.drawable.add_icon), | ||
| contentDescription = null, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Add content description for accessibility.
The contentDescription = null makes this button invisible to screen readers.
Proposed fix
IconButton(
onClick = { onAction.invoke(ClientLoanAccountsAction.AddAccount) },
) {
Icon(
painter = painterResource(Res.drawable.add_icon),
- contentDescription = null,
+ contentDescription = "Add Loan Account",
)
}📝 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.
| IconButton( | |
| onClick = { onAction.invoke(ClientLoanAccountsAction.AddAccount) }, | |
| ) { | |
| Icon( | |
| painter = painterResource(Res.drawable.add_icon), | |
| contentDescription = null, | |
| ) | |
| } | |
| IconButton( | |
| onClick = { onAction.invoke(ClientLoanAccountsAction.AddAccount) }, | |
| ) { | |
| Icon( | |
| painter = painterResource(Res.drawable.add_icon), | |
| contentDescription = "Add Loan Account", | |
| ) | |
| } |
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`
around lines 242 - 249, Replace the null content description on the add-icon
button so screen readers can announce it: in the Icon inside IconButton (the one
invoking onAction(ClientLoanAccountsAction.AddAccount) and using
painterResource(Res.drawable.add_icon)), set contentDescription to a meaningful
string (preferably via a string resource, e.g.
stringResource(R.string.add_account) or a localized label like "Add account")
instead of null so the button is accessible.
| import androidclient.feature.client.generated.resources.add_icon | ||
| import androidclient.feature.client.generated.resources.client_empty_card_message |
There was a problem hiding this comment.
Add a meaningful contentDescription for the new Add icon (accessibility).
The new clickable Add icon (Line 303) has a null contentDescription, which makes the action invisible to screen readers. Please add an accessible label (add a string resource if needed).
✅ Suggested fix
Icon(
painter = painterResource(Res.drawable.add_icon),
- contentDescription = null,
+ contentDescription = stringResource(Res.string.client_add_account),
modifier = Modifier.clickable {
addAccount.invoke()
},
)Also applies to: 303-309
🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt`
around lines 13 - 14, The Add icon in FixedDepositAccountScreen (the clickable
add icon near where the composable shows an empty state) has a null
contentDescription, making it inaccessible; update the composable that renders
the add_icon (in FixedDepositAccountScreen) to pass a meaningful, localized
contentDescription string (create a new string resource like "add_fixed_deposit"
or reuse an appropriate existing resource) and wire it via stringResource(...)
so screen readers announce the action; ensure the contentDescription is non-null
for the Image/Icon composable and update any related preview/tests to reflect
the new resource.
| DesignToken.padding | ||
| IconButton( | ||
| onClick = { onAction.invoke(SavingsAccountAction.AddAccount) }, | ||
| ) { | ||
| Icon( | ||
| painter = painterResource(Res.drawable.add_icon), | ||
| contentDescription = null, | ||
| ) | ||
| } |
There was a problem hiding this comment.
Dead code and accessibility concerns.
-
Line 254 has a standalone
DesignToken.paddingexpression that has no effect. If spacing is intended, useSpacer(modifier = Modifier.width(...)). -
The
contentDescription = nullon the add icon button makes this control invisible to screen readers, which is an accessibility issue.
Proposed fix
- DesignToken.padding
IconButton(
onClick = { onAction.invoke(SavingsAccountAction.AddAccount) },
) {
Icon(
painter = painterResource(Res.drawable.add_icon),
- contentDescription = null,
+ contentDescription = "Add Savings Account",
)
}🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccounts.kt`
around lines 254 - 262, Remove the no-op DesignToken.padding expression and
replace it with a proper Spacer using Modifier.width or Modifier.padding as
appropriate (replace the standalone DesignToken.padding near the IconButton);
also fix accessibility by providing a meaningful contentDescription for the Icon
(do not use contentDescription = null) — either hardcode a short label like "Add
account" or reference a string resource, and ensure the IconButton callback
remains onAction.invoke(SavingsAccountAction.AddAccount).
techsavvy185
changed the title
niyajali
left a comment
niyajali
left a comment
There was a problem hiding this comment.
Do these require changes in all places
| ) { | ||
| composable<ClientLoanAccountsRoute> { | ||
| composable<ClientLoanAccountsRoute> { backStackEntry -> | ||
| val route = backStackEntry.toRoute<ClientLoanAccountsRoute>() |
There was a problem hiding this comment.
Instead of collecting navigation data in this manner, leverage SavedStateHandle within your ViewModel. Retrieve the necessary data and transmit it via an event.
| is ClientLoanAccountsEvent.MakeRepayment -> makeRepayment(event.id) | ||
| ClientLoanAccountsEvent.NavigateBack -> navigateBack() | ||
| is ClientLoanAccountsEvent.ViewAccount -> viewAccount(event.id) | ||
| ClientLoanAccountsEvent.AddAccount -> createAccount() |
There was a problem hiding this comment.
Instead of "Data Object" use "Data class AddAccount(val clientId: Int)" and pass in the lambda
| makeRepayment: (Int) -> Unit, | ||
| viewAccount: (Int) -> Unit, | ||
| navController: NavController, | ||
| createAccount: () -> Unit, |
There was a problem hiding this comment.
update this to "createAccount: (Int) -> Unit" to accept the Int parameter
|
@techsavvy185 could you also post images/videos of the outcome |
| ) | ||
| } | ||
|
|
||
| IconButton( |
There was a problem hiding this comment.
Remove this toolbar icon instead update the card with a proper message with a button to create a new account
|
@niyajali Thanks for the review! I'll make the required changes. I've already attached a screen recording in the PR, is there anything specific I should cover? |
There was a problem hiding this comment.
Actionable comments posted: 0
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/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)
270-310: Add an accessible contentDescription for the new add button.The new clickable icon has
contentDescription = null, so screen readers can’t discover or announce the action. Please provide a localized label.♿ Suggested fix
Icon( painter = painterResource(Res.drawable.add_icon), - contentDescription = null, + contentDescription = stringResource(Res.string.client_add_account), modifier = Modifier.clickable { addAccount.invoke() }, )
🧹 Nitpick comments (4)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsViewModel.kt (1)
36-36: Minor style inconsistency:iskeyword used for data object.Other
data objectactions in thiswhenblock (e.g.,ToggleFiler,ToggleSearchBar,Refresh) use direct matching withoutis. For consistency, consider removing theiskeyword here.Suggested change
- is ShareAccountsAction.AddAccount -> sendEvent(ShareAccountsEvent.AddAccount(route.clientId)) + ShareAccountsAction.AddAccount -> sendEvent(ShareAccountsEvent.AddAccount(route.clientId))feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsRoute.kt (1)
35-35: Simplify the pass-through lambda.The lambda wrapping is unnecessary since it just forwards the argument directly.
♻️ Suggested simplification
- createAccount = { clientId -> createAccount(clientId) }, + createAccount = createAccount,feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)
267-274: Consider reordering parameters for consistency.The
addAccountparameter (without default) is placed aftermodifier(with default). While Kotlin's named arguments make this work, grouping parameters without defaults before those with defaults improves readability and aligns with common Compose conventions.♻️ Suggested parameter order
`@Composable` fun FixedDepositAccountHeader( totalItem: String, onToggleFilter: () -> Unit, + addAccount: () -> Unit, onToggleSearch: () -> Unit, modifier: Modifier = Modifier, - addAccount: () -> Unit, - onToggleSearch: () -> Unit, )feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (1)
192-196: Consider addingcontentDescriptionfor accessibility.Setting
contentDescription = nullmeans screen readers cannot describe this button. For better accessibility, provide a meaningful description.This also applies to the existing search and filter icons in this header.
♻️ Suggested improvement
Icon( modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.AddAccount) }, painter = painterResource(Res.drawable.add_icon), - contentDescription = null, + contentDescription = "Add Share Account", )
2 participants
Fixes - Jira-#629
Screen_recording_20260126_032328.webm
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
New Features
Style
✏️ Tip: You can customize this high-level summary in your review settings.