Skip to content

Conversation

@techsavvy185
Copy link
Contributor

@techsavvy185 techsavvy185 commented Jan 25, 2026

Fixes - Jira-#629

Screen_recording_20260127_184125.webm

Please make sure these boxes are checked before submitting your pull request - thanks!

  • Run the static analysis check ./gradlew check or ci-prepush.sh to 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

    • Added "Add Account" actions across loan, fixed deposit, recurring deposit, savings, and share account screens, enabling direct navigation to account-creation flows.
    • Empty-state cards now offer an actionable button to create accounts and include new contextual empty-list messages.
  • Style

    • Header layouts refined for improved vertical alignment and consistent icon placement.

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai
Copy link

coderabbitai bot commented Jan 25, 2026

📝 Walkthrough

Walkthrough

Adds a createAccount callback across multiple account screens/routes, introduces AddAccount actions/events in several ViewModels, wires header Add buttons and empty-state CTA to dispatch AddAccount, and routes those events to navigation helpers that open the appropriate create-account flows.

Changes

Cohort / File(s) Summary
Client Loan Accounts
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.kt, .../ClientLoanAccountsScreen.kt, .../ClientLoanAccountsViewModel.kt
Added createAccount: (Int) -> Unit param to route/screen; header Add triggers AddAccount action; ViewModel maps action → event; screen calls createAccount(clientId).
Fixed Deposit Account
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/...
Route/screen accept createAccount; header Add triggers AddAccount; route uses navController.navigateToCreateFixedDepositRoute(clientId).
Recurring Deposit Account
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/...
Added createAccount param and header Add wiring; ViewModel adds AddAccount action/event; new NavController.navigateToRecurringDepositAccountRoute(clientId) helper and route wiring.
Savings Accounts
feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/...
Added createAccount param to route/screen; header Add and empty-state CTA dispatch AddAccount; ViewModel maps action → event.
Share Accounts
feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/...
Added createAccount param to route/screen; header Add and empty-state CTA dispatch AddAccount; route wired to navigateToCreateShareAccountRoute.
Navigation
feature/client/src/commonMain/kotlin/com/mifos/feature/client/navigation/ClientNavigation.kt
Destination signatures updated (e.g., savingsAccountsDestination, clientLoanAccountsDestination) to accept createAccount: (Int) -> Unit and wired to create-route helpers.
Empty State UI & Resources
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosEmptyCard.kt, core/ui/src/commonMain/composeResources/values/strings.xml, feature/client/src/commonMain/composeResources/values/strings.xml
MifosEmptyCard gains isButtonPresent + onClick to show an action button; added string resources for "Click to add new" and per-account empty-list messages; screens use these for empty-state CTA.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant Screen as AccountScreen
  participant VM as ViewModel
  participant Nav as NavController

  User->>Screen: tap Add button / empty-state CTA
  Screen->>VM: onAction(AddAccount)
  VM->>VM: emit Event.AddAccount(clientId)
  VM->>Screen: sendEvent(AddAccount(clientId))
  Screen->>Nav: createAccount(clientId)
  Nav->>Nav: navigateToCreate<X>AccountRoute(clientId)
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

Suggested reviewers

  • revanthkumarJ
  • biplab1
  • sam-arth07

Poem

🐰 A click, a hop, a tiny start,
Buttons summon routes apart.
Actions leap and events take wing,
Nav leads on to create and bring.
Ledger grows — a joyful spring! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and specifically summarizes the main feature: adding Create Account buttons across multiple account types in the client module.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: ApproveAccount action is not dispatched.

The ApproveAccount action is constructed but never sent via onAction(), so the approve functionality won't work. Compare with ViewAccount (lines 237-243) which correctly calls onAction(...).

🐛 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 meaningful contentDescription for 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",
         )

Comment on lines 242 to 249
IconButton(
onClick = { onAction.invoke(ClientLoanAccountsAction.AddAccount) },
) {
Icon(
painter = painterResource(Res.drawable.add_icon),
contentDescription = null,
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

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.

Suggested change
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.

Comment on lines 13 to 14
import androidclient.feature.client.generated.resources.add_icon
import androidclient.feature.client.generated.resources.client_empty_card_message
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

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.

Comment on lines 254 to 262
DesignToken.padding
IconButton(
onClick = { onAction.invoke(SavingsAccountAction.AddAccount) },
) {
Icon(
painter = painterResource(Res.drawable.add_icon),
contentDescription = null,
)
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Dead code and accessibility concerns.

  1. Line 254 has a standalone DesignToken.padding expression that has no effect. If spacing is intended, use Spacer(modifier = Modifier.width(...)).

  2. The contentDescription = null on 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 techsavvy185 changed the title Create account flow fix(client): Added Create Account buttons for all types of accounts. Jan 26, 2026
Copy link
Collaborator

@niyajali niyajali left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these require changes in all places

@niyajali
Copy link
Collaborator

@techsavvy185 could you also post images/videos of the outcome

@techsavvy185
Copy link
Contributor Author

@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?

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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: is keyword used for data object.

Other data object actions in this when block (e.g., ToggleFiler, ToggleSearchBar, Refresh) use direct matching without is. For consistency, consider removing the is keyword 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 addAccount parameter (without default) is placed after modifier (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 adding contentDescription for accessibility.

Setting contentDescription = null means 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",
         )

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt`:
- Around line 12-21: The icon usage (add_icon) in RecurringDepositAccountScreen
currently sets contentDescription = null and always renders a Spacer after the
icon, which removes the action from screen readers and leaves a gap when the
icon is hidden; update the composable(s) that render add_icon (e.g., the
icon/Image composable and its surrounding layout in
RecurringDepositAccountScreen and the similar block around the code referenced
at lines ~307-318) to: 1) provide a meaningful contentDescription using an
accessible string resource (e.g.,
Res.client_profile_recurring_deposit_account_title or a dedicated "add" label)
instead of null, and 2) render the Spacer conditionally only when the icon is
actually shown (wrap the Spacer in the same if that shows the icon) so no extra
gap appears when the icon is omitted. Ensure the accessible label is localized
via the existing resources import.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt`:
- Around line 12-17: The use of add_icon currently sets contentDescription =
null (making it invisible to screen readers) and always renders a Spacer even
when the icon is hidden; update the Icon/Image usage for add_icon in
ShareAccountsScreen (and the similar nodes around the 192-206 block) to provide
an accessible label by passing a meaningful string resource (e.g., from Res like
feature_share_account_empty_list_message or a new localized string such as "add
share" via Res) into contentDescription instead of null, and render the Spacer
conditionally only when the add_icon is actually visible (wrap Spacer rendering
with the same visibility condition used for the icon).
🧹 Nitpick comments (1)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)

301-328: Consider aligning icon button patterns with SavingsAccountsHeader.

The header icons use Icon with clickable modifier, while SavingsAccountsHeader uses IconButton. For consistency across the codebase and better accessibility (proper touch targets), consider refactoring all interactive icons to use IconButton.

Also note that search icon (line 303) and filter icon (line 324) have contentDescription = null, though these lines aren't part of this PR's changes.

Comment on lines 12 to 21
import androidclient.feature.client.generated.resources.Res
import androidclient.feature.client.generated.resources.client_empty_card_message
import androidclient.feature.client.generated.resources.add_icon
import androidclient.feature.client.generated.resources.client_product_recurring_deposit_account
import androidclient.feature.client.generated.resources.client_profile_recurring_deposit_account_title
import androidclient.feature.client.generated.resources.client_savings_item
import androidclient.feature.client.generated.resources.client_savings_not_avilable
import androidclient.feature.client.generated.resources.client_savings_pending_approval
import androidclient.feature.client.generated.resources.feature_client_dialog_action_ok
import androidclient.feature.client.generated.resources.feature_recurring_account_empty_list_message
import androidclient.feature.client.generated.resources.filter
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add icon needs an accessible label and conditional spacing.

contentDescription = null hides the action from screen readers, and the spacer after the conditional icon leaves a gap when the icon isn’t shown.

🔧 Suggested fix
 import androidclient.feature.client.generated.resources.Res
 import androidclient.feature.client.generated.resources.add_icon
+import androidclient.feature.client.generated.resources.client_apply_new_applications_recurring_account
 import androidclient.feature.client.generated.resources.client_product_recurring_deposit_account
 import androidclient.feature.client.generated.resources.client_profile_recurring_deposit_account_title
@@
-        if (!isRecurringDepositScreenEmpty) {
-            Icon(
-                painter = painterResource(Res.drawable.add_icon),
-                contentDescription = null,
-                modifier = Modifier.clickable {
-                    addAccount.invoke()
-                },
-            )
-        }
-
-        Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased))
+        if (!isRecurringDepositScreenEmpty) {
+            Icon(
+                painter = painterResource(Res.drawable.add_icon),
+                contentDescription = stringResource(
+                    Res.string.client_apply_new_applications_recurring_account,
+                ),
+                modifier = Modifier.clickable { addAccount.invoke() },
+            )
+            Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased))
+        }

Also applies to: 307-318

🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt`
around lines 12 - 21, The icon usage (add_icon) in RecurringDepositAccountScreen
currently sets contentDescription = null and always renders a Spacer after the
icon, which removes the action from screen readers and leaves a gap when the
icon is hidden; update the composable(s) that render add_icon (e.g., the
icon/Image composable and its surrounding layout in
RecurringDepositAccountScreen and the similar block around the code referenced
at lines ~307-318) to: 1) provide a meaningful contentDescription using an
accessible string resource (e.g.,
Res.client_profile_recurring_deposit_account_title or a dedicated "add" label)
instead of null, and 2) render the Spacer conditionally only when the icon is
actually shown (wrap the Spacer in the same if that shows the icon) so no extra
gap appears when the icon is omitted. Ensure the accessible label is localized
via the existing resources import.

Comment on lines 12 to 17
import androidclient.feature.client.generated.resources.Res
import androidclient.feature.client.generated.resources.add_icon
import androidclient.feature.client.generated.resources.client_product_shares_account
import androidclient.feature.client.generated.resources.client_savings_item
import androidclient.feature.client.generated.resources.feature_share_account_empty_list_message
import androidclient.feature.client.generated.resources.filter
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Add icon needs an accessible label and conditional spacing.

contentDescription = null hides the action from screen readers, and the spacer should only appear when the icon is visible.

🔧 Suggested fix
 import androidclient.feature.client.generated.resources.Res
 import androidclient.feature.client.generated.resources.add_icon
+import androidclient.feature.client.generated.resources.client_apply_new_applications_share_account
 import androidclient.feature.client.generated.resources.client_product_shares_account
 import androidclient.feature.client.generated.resources.client_savings_item
@@
         Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased))
         if (!isShareAccountsEmpty) {
             Icon(
                 modifier = Modifier.onClick { onAction.invoke(ShareAccountsAction.AddAccount) },
                 painter = painterResource(Res.drawable.add_icon),
-                contentDescription = null,
+                contentDescription = stringResource(
+                    Res.string.client_apply_new_applications_share_account,
+                ),
             )
+            Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased))
         }
-        Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased))

Also applies to: 192-206

🤖 Prompt for AI Agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt`
around lines 12 - 17, The use of add_icon currently sets contentDescription =
null (making it invisible to screen readers) and always renders a Spacer even
when the icon is hidden; update the Icon/Image usage for add_icon in
ShareAccountsScreen (and the similar nodes around the 192-206 block) to provide
an accessible label by passing a meaningful string resource (e.g., from Res like
feature_share_account_empty_list_message or a new localized string such as "add
share" via Res) into contentDescription instead of null, and render the Spacer
conditionally only when the add_icon is actually visible (wrap Spacer rendering
with the same visibility condition used for the icon).

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

249-255: Bug: ApproveAccount action is created but never dispatched.

The ApproveAccount action is instantiated but not passed to onAction(), so clicking "Approve Account" does nothing. Compare with ViewAccount at line 243 which correctly calls onAction(...).

🐛 Proposed fix
                                         is Actions.ApproveAccount -> {
-                                            RecurringDepositAccountAction.ApproveAccount(
+                                            onAction(
+                                                RecurringDepositAccountAction.ApproveAccount(
-                                                recurringDeposit.accountNo ?: "",
+                                                    recurringDeposit.accountNo ?: "",
+                                                ),
                                             )
                                         }
🧹 Nitpick comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)

66-74: Parameter ordering violates Compose conventions.

The createAccount parameter is required (no default value) but is placed after modifier which has a default. Compose API guidelines recommend placing required parameters before optional ones, with modifier typically being the last optional parameter.

♻️ Suggested fix
 `@Composable`
 fun RecurringDepositAccountScreen(
     navController: NavController,
     navigateBack: () -> Unit,
     onApproveAccount: (String) -> Unit,
     onViewAccount: (String) -> Unit,
+    createAccount: (Int) -> Unit,
     modifier: Modifier = Modifier,
-    createAccount: (Int) -> Unit,
     viewModel: RecurringDepositAccountViewModel = koinViewModel(),
 ) {
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (2)

66-74: Consider reordering parameters for better API consistency.

The createAccount parameter (required, no default) is placed after modifier (optional with default). Kotlin/Compose conventions typically place required parameters before optional ones. Consider moving createAccount before modifier for consistency.

♻️ Suggested reordering
 `@Composable`
 fun FixedDepositAccountScreen(
     navController: NavController,
     navigateBack: () -> Unit,
     onApproveAccount: (String) -> Unit,
     onViewAccount: (String) -> Unit,
+    createAccount: (Int) -> Unit,
     modifier: Modifier = Modifier,
-    createAccount: (Int) -> Unit,
     viewModel: FixedDepositAccountViewModel = koinViewModel(),
 )

273-281: Consider consistent parameter ordering in header function.

Required parameters (onToggleFilter, addAccount, onToggleSearch, isFixedDepositScreenEmpty) are interleaved around the optional modifier parameter. For consistency, group all required parameters before optional ones.

♻️ Suggested reordering
 `@Composable`
 fun FixedDepositAccountHeader(
     totalItem: String,
     onToggleFilter: () -> Unit,
+    addAccount: () -> Unit,
     onToggleSearch: () -> Unit,
+    isFixedDepositScreenEmpty: Boolean,
     modifier: Modifier = Modifier,
-    addAccount: () -> Unit,
-    onToggleSearch: () -> Unit,
-    isFixedDepositScreenEmpty: Boolean,
 )

@techsavvy185
Copy link
Contributor Author

@niyajali I'm done with all the changes that were required. Could you please go through this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants