Skip to content

refactor: remove redundant scaffold, back icon and screen name#2587

Merged
therajanmaurya merged 5 commits intoopenMF:developmentfrom
Arinyadav1:remove-redundant-scaffolds
Jan 30, 2026
Merged

refactor: remove redundant scaffold, back icon and screen name#2587
therajanmaurya merged 5 commits intoopenMF:developmentfrom
Arinyadav1:remove-redundant-scaffolds

Conversation

@Arinyadav1
Copy link
Contributor

@Arinyadav1 Arinyadav1 commented Jan 28, 2026

Fixes - Jira-#633

WhatsApp.Video.2026-01-28.at.8.16.32.PM.mp4

Summary by CodeRabbit

  • New Features

    • Breadcrumb navigation added across many client screens for clearer context.
    • Search and filter toggles introduced on several account and listing screens.
  • Bug Fixes & Improvements

    • Unified, simplified layouts and spacing for more consistent visuals and touch targets.
    • Streamlined action rows and button behavior for easier workflows.
    • Improved empty-state messaging (including "No Savings Accounts found").
    • Polished loading, dialog and date-picker flows for more predictable UX.

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

@coderabbitai
Copy link

coderabbitai bot commented Jan 28, 2026

📝 Walkthrough

Walkthrough

Replaces many Scaffold-based screen wrappers with content-centric composables (renaming *Scaffold → *Content), inserts breadcrumb bars, shifts padding to DesignToken values, flattens UI composition (columns/LazyColumn) and dialog/bottom-sheet placement, and adjusts modifier propagation and button layouts while preserving state-driven branches and actions.

Changes

Cohort / File(s) Summary
Global Scaffold → Content
feature/client/src/commonMain/kotlin/com/mifos/feature/client/... (e.g., clientAddDocuments/ClientAddDocumentsScreen.kt, clientDocuments/ClientDocumentsScreen.kt, clientProfile/..., clientSignature/ClientSignatureScreen.kt, documentPreviewScreen/DocumentPreviewScreen.kt, note/..., createNewClient/CreateNewClientScreen.kt, fixedDepositAccount/..., recurringDepositAccount/..., shareAccounts/...)
Removed MifosScaffold wrappers, renamed many *Scaffold*Content, updated call sites, relocated top bar to MifosBreadcrumbNavBar, moved padding into DesignToken-based modifiers, and flattened conditional layouts.
Dialog / Buttons / Bottom-sheet
feature/client/.../ClientAddDocumentsScreen.kt, .../ClientClosureScreen.kt, .../ClientStaffScreen.kt, .../ClientTransferScreen.kt, .../DocumentPreviewScreen.kt, feature/note/.../AddEditNoteScreen.kt
Consolidated button rows (equal-weight Back/Submit, MifosTwoButtonRow), simplified back controls, moved bottom-sheet and dialog rendering into content scope, and adjusted enable/visibility logic.
Lists / State branches
.../ClientCollateralDetailScreen.kt, .../ClientLoanAccountsScreen.kt, .../ClientDocumentsScreen.kt, .../FixedDepositAccountScreen.kt, .../RecurringDepositAccountScreen.kt, .../ClientUpcomingChargesScreen.kt
Made loading/error/empty/success branches explicit (using MifosProgressIndicator, MifosEmptyCard, MifosErrorComponent), consolidated lists into single LazyColumn flows, preserved action wiring and search/filter toggles.
Modifier propagation tweaks
feature/loan/.../NewLoanAccountScreen.kt, feature/client/.../ChargesScreen.kt, .../ClientCollateralScreen.kt, .../ClientListScreen.kt, .../ClientIdentifiersListScreen.kt, .../ClientLoanAccountsScreen.kt
Adjusted how incoming modifier is applied to outer/inner Columns; some screens now apply modifier.fillMaxSize(), others use fresh Modifier for inner content—affects external styling/spacing.
API / Signature adjustments
.../ClientSignatureScreen.kt, .../SavingsAccountsScreen.kt, .../ShareAccountsScreen.kt, .../NoteScreen.kt, .../AddEditNoteScreen.kt, .../RecurringDepositAccountScreen.kt
Some public/internal composable signatures updated to include modifier or navController, renamed public entry points (e.g., SavingsAccountsScreenRouteSavingsAccountsScreen, new *Content wrappers), and one dialog removed an onRetry parameter.
Minor resources & spacing
feature/client/src/commonMain/composeResources/values/strings.xml, .../ClientDetailsProfile.kt, assorted small files
Added string feature_client_no_savings_accounts_found, tweaked paddings/spacings, and removed unused scaffold imports across files.

Sequence Diagram(s)

(omitted)

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related PRs

Suggested reviewers

  • biplab1
  • revanthkumarJ
  • sam-arth07

Poem

🐇 I hopped through scaffolds at morning light,
Breadcrumbs gleamed where titles once held tight.
Columns now stretch, buttons pair and sing,
Dialogs nestle softly in content's ring.
A rabbit clapped — the UI feels just right.

🚥 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 PR title clearly and specifically describes the main change: removing redundant scaffolds, back icons, and screen names across multiple client feature files.

✏️ 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: 20

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt (1)

241-253: Remove dead code statement.

Line 243 (DesignToken.padding) is a no-op statement that has no effect. This appears to be leftover code or an incomplete addition. Remove it or replace it with the intended spacing component (e.g., Spacer).

🧹 Proposed fix
         IconButton(
             onClick = { onAction.invoke(SavingsAccountAction.ToggleSearch) },
         ) {
             // add a cross icon when its active, talk with design team
             Icon(
                 painter = painterResource(Res.drawable.search),
                 contentDescription = null,
             )
         }
 
-        DesignToken.padding
+        Spacer(modifier = Modifier.width(DesignToken.padding.small))
 
         IconButton(
             onClick = { onAction.invoke(SavingsAccountAction.ToggleFilter) },

Or simply remove the line if no spacing is needed between the buttons.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt (1)

663-674: Fix staff selection mismatch after sorting options.
You sort staffInOffices for display, but still index into the unsorted list when selecting, so the saved selectedStaffId can be wrong. Use the same sorted list for both options and selection mapping.

🐛 Proposed fix
-            MifosTextFieldDropdown(
+            val sortedStaff = staffInOffices.sortedBy { it.displayName }
+            MifosTextFieldDropdown(
                 enabled = false,
                 value = staff,
                 onValueChanged = { staff = it },
                 onOptionSelected = { index, value ->
                     staff = value
-                    selectedStaffId = staffInOffices[index].id
+                    selectedStaffId = sortedStaff.getOrNull(index)?.id
                 },
                 label = stringResource(Res.string.feature_client_staff),
-                options = staffInOffices.sortedBy { it.displayName }
-                    .map { it.displayName.toString() },
+                options = sortedStaff.map { it.displayName.orEmpty() },
                 readOnly = true,
             )
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDocuments/ClientDocumentsScreen.kt (1)

289-305: Add content descriptions for accessibility.

The search and add icons are interactive but have contentDescription = null. Screen reader users won't be able to identify these buttons. Consider adding meaningful descriptions.

♿ Suggested fix
         Icon(
             imageVector = MifosIcons.Search,
-            contentDescription = null,
+            contentDescription = stringResource(Res.string.search_content_description), // or a suitable string
             modifier = Modifier.clickable {
                 onToggleSearch.invoke()
             },
         )

         Spacer(modifier = Modifier.width(DesignToken.spacing.largeIncreased))

         Icon(
             imageVector = MifosIcons.Add,
-            contentDescription = null,
+            contentDescription = stringResource(Res.string.add_document_content_description), // or a suitable string
             modifier = Modifier.clickable {
                 onAddDocument.invoke()
             },
         )

Note: You may need to add the appropriate string resources if they don't exist.

🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientApplyNewApplications/ClientApplyNewApplicationsScreen.kt`:
- Around line 113-152: The MifosSweetError currently takes its default
Modifier.fillMaxSize(), causing it to occupy the whole screen under the Column;
update the call to MifosSweetError to pass a Modifier.weight(1f).fillMaxWidth()
so it only fills the remaining vertical space after MifosBreadcrumbNavBar and
matches the column width; locate the MifosSweetError invocation in
ClientApplyNewApplicationsScreen (inside the else branch) and replace or augment
its modifier argument accordingly.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientClosure/ClientClosureScreen.kt`:
- Around line 166-178: The code reads
state.reasons[state.currentSelectedIndex].name which can crash if
currentSelectedIndex is out-of-bounds when reasons change; update the
MifosTextFieldDropdown usages to safely resolve the selected value and options
by first clamping or safely accessing the index (e.g., compute a safeIndex =
state.currentSelectedIndex.coerceIn(0, state.reasons.lastIndex) or use
state.reasons.getOrNull(state.currentSelectedIndex) and fallback to an empty
string) and use state.reasons.map { it.name } for options as before; ensure the
value parameter uses the safe lookup and that onOptionSelected still dispatches
onAction(ClientClosureAction.OptionChanged(index)).
- Around line 103-110: The DatePicker state created by rememberDatePickerState
(datePickerState) is only initialized with state.date and won't react to later
changes; add a LaunchedEffect keyed to state.date that assigns
datePickerState.selectedDateMillis = state.date (or null-handled) whenever
state.date changes so the UI stays in sync; use the existing datePickerState and
state.date references and ensure the LaunchedEffect runs on composition to
update the selectedDateMillis property.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCollateralDetails/ClientCollateralDetailScreen.kt`:
- Around line 70-72: The empty-state padding is ignored because MifosEmptyCard
does not apply the passed Modifier; either wrap the MifosEmptyCard call in a
padded container (e.g., Box or Column with
Modifier.padding(DesignToken.padding.large)) where
ClientCollateralDetailsState.State.Empty is handled, or modify MifosEmptyCard
itself to accept and apply the incoming modifier to its root composable so the
horizontal padding passed from ClientCollateralDetailScreen takes effect.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt`:
- Around line 189-209: The header title logic in ClientIdentitiesAddUpdateScreen
(look for state.feature and the Text block) is wrong because it checks
documentKey outside the feature switch and has an unreachable VIEW_DOCUMENT
branch; replace the current conditional with a when(state.feature) to choose
titles per feature (e.g., VIEW_DOCUMENT, ADD_IDENTIFIER, ADD_UPDATE_DOCUMENT)
and only inspect state.documentKey inside the ADD_UPDATE_DOCUMENT branch to
decide between "add" vs "update" titles so the ADD_IDENTIFIER title remains
stable while typing.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt`:
- Around line 132-147: The loanBalance prop is incorrectly populated from
loan.amountPaid (duplicating amountPaid); change the value passed to
MifosActionsLoanListingComponent for loanBalance to use the model's actual
balance field (e.g., loan.balanceOutstanding or loan.outstandingBalance —
whichever your data model defines) instead of loan.amountPaid, keeping the same
null-safe formatting and prefixing with symbol (same pattern as
originalLoan/amountPaid).
- Around line 171-178: The MakeRepayment path currently uses a parameterless
Actions.MakeRepayment / ClientLoanAccountsAction.MakeRepayment which causes the
ViewModel to emit ClientLoanAccountsEvent.MakeRepayment using only the client
id; change ClientLoanAccountsAction.MakeRepayment from a parameterless data
object to carry the loan id (e.g., MakeRepayment(loanId: Int)), update the call
site in onActionClicked to pass loan.id ?: 0, update the ViewModel handling that
action to forward the loanId in ClientLoanAccountsEvent.MakeRepayment(loanId)
instead of the client id, and adjust the navigation handler to consume the
provided loanId; reference: Actions.MakeRepayment,
ClientLoanAccountsAction.MakeRepayment, onActionClicked, onAction,
ClientLoanAccountsEvent.MakeRepayment.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt`:
- Around line 237-244: In PinpointClientScreen, avoid launching coroutines or
invoking onAddressesChanged directly in the composition when handling
PinPointClientUiState.SuccessMessage; wrap the side-effect in a LaunchedEffect
keyed by state (e.g., LaunchedEffect(state)) and move the
snackbarHostState.showSnackbar call and onAddressesChanged() into that
LaunchedEffect so they run only once per state change. Also ensure the UI
actually displays the snackbar by adding a SnackbarHost wired to
snackbarHostState (for example via Scaffold(snackbarHost = {
SnackbarHost(snackbarHostState) })) in the composable tree.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientStaff/ClientStaffScreen.kt`:
- Around line 103-117: The code is indexing state.staffOptions with
state.currentSelectedIndex which can be -1 or stale; fix by performing a safe
lookup when setting MifosTextFieldDropdown.value (e.g., use
state.staffOptions.getOrNull(state.currentSelectedIndex)?.displayName ?:
fallbackLabel) and ensure the onOptionSelected still dispatches
ClientStaffAction.OptionChanged(index); also guard any other usages of
state.currentSelectedIndex to clamp or use getOrNull before accessing
staffOptions to avoid out-of-bounds crashes.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpdateDefaultAccount/ClientUpdateDefaultAccountScreen.kt`:
- Around line 161-163: Replace the hardcoded English message in the Text call
inside ClientUpdateDefaultAccountScreen (the else branch that currently shows
"No Savings Accounts found for this client") with a call to stringResource(...)
and create a matching string resource (e.g., update_default_account_no_accounts)
in your strings.xml/multiplatform resources; ensure you reference it via
stringResource(R.string.update_default_account_no_accounts) in the Text
composable so the UI uses localized text.
- Around line 109-118: The UI accesses
state.accounts[state.currentSelectedIndex] in MifosTextFieldDropdown and the
ViewModel uses state.currentSelectedIndex in updateDefaultAccount() and
getClientSavingsAccountId(); add bounds checks and index normalization: when
accounts change or are loaded, clamp currentSelectedIndex to
0..(accounts.size-1) or set to 0 if empty, use safe access (getOrNull) instead
of direct indexing in MifosTextFieldDropdown and in updateDefaultAccount(), and
update getClientSavingsAccountId() to reset the index (or return null) when no
matching account is found so a stale index cannot be used.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/createNewClient/CreateNewClientScreen.kt`:
- Around line 202-285: Snackbars are never shown because snackbarHostState is
not attached to any SnackbarHost in the UI tree; attach a SnackbarHost using the
existing snackbarHostState (e.g., add SnackbarHost(hostState =
snackbarHostState) as a child of the top-level Column in CreateNewClientScreen)
so that calls to snackbarHostState.showSnackbar(...) actually render; ensure the
SnackbarHost is placed where it can display (typically near the top-level
layout) and that necessary compose imports are present.
- Around line 233-268: The composition currently triggers side effects directly:
in CreateNewClientUiState.SetClientId it calls uploadImage(uiState.id) and
navigateBack.invoke(), and in CreateNewClientUiState.OnImageUploadSuccess and
ShowWaitingForCheckerApproval it calls navigateBack.invoke() outside the
coroutine; move these into LaunchedEffect blocks keyed on the specific ui state
(e.g., LaunchedEffect(uiState) or LaunchedEffect(uiState.id) for SetClientId) so
uploadImage(uiState.id) and snackbarHostState.showSnackbar(...) are invoked
inside LaunchedEffect and navigation (navigateBack.invoke()) happens after the
snackbar within the same LaunchedEffect coroutine, and remove any direct calls
to uploadImage or navigateBack from the composable body to ensure each side
effect runs only once per state change.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt`:
- Around line 137-151: The root Column in FixedDepositAccountScreen currently
uses a hardcoded Modifier.fillMaxSize() so the passed-in modifier parameter is
only applied to the inner content Column; move or combine the incoming modifier
into the outer Column (the one that wraps MifosBreadcrumbNavBar and the
when(state.isLoading) branch) so that the caller's modifier (padding, insets,
test tags, etc.) applies to both the loading branch (MifosProgressIndicator) and
the content branch; update the outer Column to use
modifier.then(Modifier.fillMaxSize()) or modifier.fillMaxSize() while removing
the inner Column’s standalone modifier usage to avoid duplicate/contradictory
modifiers.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt`:
- Around line 137-140: The root Column in RecurringDepositAccountScreen ignores
the passed modifier parameter and uses Modifier.fillMaxSize() directly; update
the root Column to apply the incoming modifier combined with the size constraint
(e.g., modifier.fillMaxSize() or modifier.then(Modifier.fillMaxSize())) so
callers can style or position the composable using the modifier parameter.
- Around line 228-232: The ApproveAccount branch constructs a
RecurringDepositAccountAction.ApproveAccount but never dispatches it; update the
Actions.ApproveAccount case to call onAction(...) with the created action (i.e.,
create RecurringDepositAccountAction.ApproveAccount(recurringDeposit.accountNo
?: "") and immediately pass that instance into onAction) so the approve flow is
dispatched the same way as the ViewAccount handling; ensure you reference the
existing onAction callback and RecurringDepositAccountAction.ApproveAccount
symbol when making the change.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt`:
- Around line 97-109: The root composable in SavingsAccountsContent is not using
the passed-in modifier; apply the function parameter modifier to the outer
Column (the one currently using Modifier.fillMaxSize()) so callers can control
the component, and change the inner Column to use a local modifier (e.g.,
Modifier.fillMaxSize() or a composed localModifier) instead of the parameter;
update references to the outer Column and the inner Column in
SavingsAccountsContent accordingly.

In
`@feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/newLoanAccount/NewLoanAccountScreen.kt`:
- Around line 163-169: The Column is not consuming scaffold insets because the
MifosScaffold content lambda dropped the paddingValues; restore consuming
scaffold padding by accepting the padding parameter from MifosScaffold's content
lambda (e.g., paddingValues/innerPadding) and apply it to the root container
(the Column modifier) so the breadcrumb/stepper and transient UI (snackbar)
won't be overlapped; locate the MifosScaffold invocation and the Column in
NewLoanAccountScreen and add .padding(paddingValues) (or combine with
fillMaxSize via .then) to the Column's modifier.

In
`@feature/note/src/commonMain/kotlin/com/mifos/feature/note/notes/NoteScreen.kt`:
- Around line 190-194: The root Column in the NoteContent composable is ignoring
its caller-supplied modifier by using the companion Modifier; update the Column
to apply the function parameter instead (use the modifier parameter and chain
the paddings onto it, e.g. modifier = modifier.padding(...)) so any external
modifiers are preserved; locate the NoteContent function and replace Modifier
with the modifier parameter on the Column, chaining paddings via
modifier.padding(...) or modifier.then(...) as appropriate.

In
`@feature/savings/src/commonMain/kotlin/com/mifos/feature/savings/savingsAccountv2/SavingsAccountScreen.kt`:
- Around line 146-148: The Column in SavingsAccountScreen is not applying the
scaffold's paddingValues from MifosScaffold, causing content to ignore scaffold
insets; update the composable lambda where MifosScaffold provides paddingValues
(the content lambda in SavingsAccountScreen) to accept a parameter (e.g.,
paddingValues: PaddingValues) and apply it to the root Column via
Modifier.padding(paddingValues) or alternatively add systemBarsPadding() to the
Column's modifier so the root respects scaffold/system insets; locate the Column
declaration in SavingsAccountScreen.kt and modify its surrounding MifosScaffold
content lambda signature and Column modifier accordingly.
🧹 Nitpick comments (16)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt (1)

86-119: Consider adding fillMaxSize() to the root Column for explicit layout behavior.

The root Column currently has no modifier, relying on the inner Column at line 93 to propagate the fill behavior. While this works, adding fillMaxSize() to the root makes the layout intent explicit and guards against unexpected sizing if the composable is used in different contexts.

♻️ Suggested change
-    Column {
+    Column(modifier = Modifier.fillMaxSize()) {
         MifosBreadcrumbNavBar(navController)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileScreen.kt (1)

136-140: Consider UX implications of conditional content rendering.

When dialogState is not null (loading or error), the entire content is hidden. This causes the screen to flash empty before showing the loading indicator or error component. A smoother approach could be to show the content with an overlay or keep a skeleton/placeholder visible.

However, if this pattern aligns with how other screens in this refactoring behave, it may be intentional for consistency.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientPinpoint/PinpointClientScreen.kt (1)

86-89: Unused parameter onBackPressed after scaffold removal.

The onBackPressed callback is declared in both the ViewModel-connected and stateless PinpointClientScreen composables but is never invoked after removing the scaffold. The MifosBreadcrumbNavBar handles back navigation internally via navController.popBackStack().

Consider removing onBackPressed from the function signatures if it's no longer needed, or wire it up if custom back handling is required.

Also applies to: 131-143

feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (1)

163-177: TODO: Search bar functionality is incomplete.

The comment indicates that search functionality needs implementation. The search bar UI is wired to dispatch actions (UpdateSearch, Search, ToggleSearch), but the actual filtering/search logic may not be implemented in the ViewModel.

Would you like me to help implement the search filtering logic, or should I open an issue to track this task?

feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt (1)

256-281: Extract hardcoded strings to resources for i18n consistency.

Lines 269 and 276 use hardcoded strings ("No Item Found" and "Click Here To View Filled State. ") while the rest of the file uses stringResource(). For consistency and internationalization support, these should be moved to string resources.

Additionally, the text "Click Here To View Filled State." appears to be placeholder/debug text that may not be appropriate for production.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/documentPreviewScreen/DocumentPreviewScreen.kt (1)

129-129: Consider using a design token for consistency.

Line 129 uses a hardcoded 8.dp while other spacing in this file uses DesignToken.spacing.*. For consistency and maintainability, consider using an appropriate design token.

♻️ Suggested change
-            Spacer(modifier = Modifier.width(8.dp))
+            Spacer(modifier = Modifier.width(DesignToken.spacing.small))
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersAddUpdate/ClientIdentitiesAddUpdateScreen.kt (1)

186-195: Apply modifier to the outer Column for flexibility.
Right now the passed-in modifier only affects the inner Column. If callers need test tags, padding, or semantics on the root, they can’t set it.

♻️ Optional tweak
-    Column(
-        modifier = Modifier.fillMaxSize(),
-    ) {
+    Column(
+        modifier = modifier.fillMaxSize(),
+    ) {
         if (state.feature != Feature.VIEW_DOCUMENT) {
             MifosBreadcrumbNavBar(navController)
         }

         Column(
-            modifier = modifier.fillMaxSize().padding(
+            modifier = Modifier.fillMaxSize().padding(
                 horizontal = DesignToken.padding.large,
             ),
         ) {
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditProfile/ClientProfileEditScreen.kt (1)

95-105: Extract a single onAction lambda to avoid duplication.
This reduces repetition and keeps the same stable reference shared by both composables.

♻️ Suggested refactor
     val state by viewModel.stateFlow.collectAsStateWithLifecycle()
+    val onAction = remember(viewModel) { { viewModel.trySendAction(it) } }
 
     EventsEffect(viewModel.eventFlow) { event ->
         when (event) {
             ClientProfileEditEvent.NavigateBack -> onNavigateBack()
             ClientProfileEditEvent.OnSaveSuccess -> {
@@
     ClientProfileEditContent(
         modifier = modifier,
         state = state,
-        onAction = remember(viewModel) { { viewModel.trySendAction(it) } },
+        onAction = onAction,
         navController = navController,
     )
 
     ClientProfileEditDialogs(
         state = state,
-        onAction = remember(viewModel) { { viewModel.trySendAction(it) } },
+        onAction = onAction,
     )
 }
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpdateDefaultAccount/ClientUpdateDefaultAccountScreen.kt (1)

142-142: Use width instead of padding for horizontal spacing between buttons.

Using Modifier.padding() on a Spacer is semantically unusual. For horizontal spacing between row elements, Modifier.width() is more appropriate and explicit.

♻️ Proposed fix
-                        Spacer(Modifier.padding(DesignToken.padding.small))
+                        Spacer(Modifier.width(DesignToken.padding.small))
feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (2)

81-95: Use the passed modifier at the root to follow Jetpack Compose best practices.

The function signature exposes a modifier: Modifier = Modifier parameter (line 78), but the root Column (lines 81–84) uses Modifier.fillMaxSize() instead of the passed-in modifier. This prevents callers from applying padding, test tags, or other layout modifications to the whole screen. The inner Column (lines 93–95) uses the modifier correctly, but it should be at the root level.

Apply the passed modifier to the root Column:

Suggested fix
-    Column(
-        modifier = Modifier
-            .fillMaxSize(),
-    ) {
+    Column(
+        modifier = modifier
+            .fillMaxSize(),
+    ) {

104-141: Use itemsIndexed instead of wrapping the entire list in a single item block.

Using item { forEachIndexed(...) } defeats the lazy evaluation mechanism and forces Compose to measure and layout all rows whenever that single item enters the viewport, causing unnecessary memory overhead and potential jank on large lists. itemsIndexed enables proper lazy composition of individual items.

Suggested fix
-                        LazyColumn {
-                            item {
-                                state.accounts.forEachIndexed { index, account ->
-                                    MifosActionsShareListingComponent(
-                                        accountNo = account.accountNo ?: emptyText,
-                                        shareProductName = account.shortProductName
-                                            ?: emptyText,
-                                        pendingForApprovalShares = account.totalPendingForApprovalShares,
-                                        approvedShares = account.totalApprovedShares,
-                                        isExpanded = state.currentlyActiveIndex == index && state.isCardActive,
-                                        menuList = (listOf(Actions.ViewAccount())),
-                                        onActionClicked = { actions ->
-                                            when (actions) {
-                                                is Actions.ViewAccount -> {
-                                                    onAction(
-                                                        ShareAccountsAction.ViewAccount(
-                                                            account.id ?: -1,
-                                                        ),
-                                                    )
-                                                }
-                                                else -> {}
-                                            }
-                                        },
-                                        onClick = {
-                                            onAction(
-                                                ShareAccountsAction.CardClicked(
-                                                    index,
-                                                ),
-                                            )
-                                        },
-                                    )
-                                    Spacer(Modifier.height(DesignToken.padding.small))
-                                }
-                            }
-                        }
+                        LazyColumn {
+                            itemsIndexed(
+                                items = state.accounts,
+                                key = { index, account -> account.id ?: index },
+                            ) { index, account ->
+                                MifosActionsShareListingComponent(
+                                    accountNo = account.accountNo ?: emptyText,
+                                    shareProductName = account.shortProductName
+                                        ?: emptyText,
+                                    pendingForApprovalShares = account.totalPendingForApprovalShares,
+                                    approvedShares = account.totalApprovedShares,
+                                    isExpanded = state.currentlyActiveIndex == index && state.isCardActive,
+                                    menuList = (listOf(Actions.ViewAccount())),
+                                    onActionClicked = { actions ->
+                                        when (actions) {
+                                            is Actions.ViewAccount -> {
+                                                onAction(
+                                                    ShareAccountsAction.ViewAccount(
+                                                        account.id ?: -1,
+                                                    ),
+                                                )
+                                            }
+                                            else -> {}
+                                        }
+                                    },
+                                    onClick = {
+                                        onAction(
+                                            ShareAccountsAction.CardClicked(
+                                                index,
+                                            ),
+                                        )
+                                    },
+                                )
+                                Spacer(Modifier.height(DesignToken.padding.small))
+                            }
+                        }
feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)

135-208: Prefer stable keys + account-keyed expansion state to avoid mismatches after list updates.

Index-based expansion can highlight the wrong row after filtering or reordering (e.g., search). Consider keying expansion by account and providing a stable key to the list.

♻️ Suggested change
-    var expandedIndex by rememberSaveable { mutableStateOf(-1) }
+    var expandedKey by rememberSaveable { mutableStateOf<String?>(null) }

...
-                        LazyColumn {
-                            itemsIndexed(state.fixedDepositAccount) { index, fixedDepositAccount ->
+                        LazyColumn {
+                            itemsIndexed(
+                                state.fixedDepositAccount,
+                                key = { index, item -> item.accountNo ?: "index-$index" },
+                            ) { index, fixedDepositAccount ->
+                                val itemKey = fixedDepositAccount.accountNo ?: "index-$index"
+                                val isExpanded = expandedKey == itemKey
                                 MifosActionsSavingsListingComponent(
 ...
-                                    isExpanded = expandedIndex == index,
-                                    onExpandToggle = {
-                                        expandedIndex = if (expandedIndex == index) -1 else index
-                                    },
+                                    isExpanded = isExpanded,
+                                    onExpandToggle = {
+                                        expandedKey = if (isExpanded) null else itemKey
+                                    },
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientTransfer/ClientTransferScreen.kt (1)

196-221: Use a width spacer in the Row.

Line 220 uses Spacer(Modifier.padding(...)), which adds vertical padding inside a Row. A width spacer is clearer for horizontal gaps.

♻️ Suggested spacing tweak
 import androidx.compose.foundation.layout.height
 import androidx.compose.foundation.layout.padding
 import androidx.compose.foundation.layout.size
+import androidx.compose.foundation.layout.width
@@
-                        Spacer(Modifier.padding(DesignToken.padding.small))
+                        Spacer(Modifier.width(DesignToken.padding.small))
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/ClientAddressScreen.kt (1)

138-143: Use a stable, O(1) key for address items.

state.address.indexOf(it) is O(n) per item and can produce unstable keys when duplicates exist, which may cause incorrect item reuse during recomposition. Prefer a stable id (or itemsIndexed) instead.

♻️ Suggested refactor
- items(state.address, key = ({ state.address.indexOf(it) })) { address ->
+ itemsIndexed(
+     items = state.address,
+     key = { index, address -> address.id ?: index } // use a stable id if available
+ ) { _, address ->
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDocuments/ClientDocumentsScreen.kt (2)

151-154: Apply modifier parameter to the root composable.

The modifier parameter (line 149) is not applied to the outer Column. This breaks the Compose convention where the modifier should be applied to the root element, preventing parent composables from controlling sizing, padding, or other layout behavior.

♻️ Suggested fix
     Column(
-        modifier = Modifier
+        modifier = modifier
             .fillMaxSize(),
     ) {

Then update the inner Column at line 169 to use Modifier instead:

                 Column(
-                    modifier.fillMaxSize()
+                    Modifier.fillMaxSize()
                         .padding(

250-251: Prefer else -> Unit or an empty block over else -> null.

Returning null from a when branch used as a statement is unconventional. An empty block or Unit is clearer.

♻️ Suggested fix
-                                        else -> null
+                                        else -> Unit

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: 11

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt (1)

241-245: Dead statement: DesignToken.padding does nothing.

Line 243 evaluates DesignToken.padding but doesn't use the result. This appears to be incomplete code, possibly intended to add spacing between the icons.

🔧 Suggested fix: Add explicit spacing or remove
         IconButton(
             onClick = { onAction.invoke(SavingsAccountAction.ToggleSearch) },
         ) {
             // add a cross icon when its active, talk with design team
             Icon(
                 painter = painterResource(Res.drawable.search),
                 contentDescription = null,
             )
         }

-        DesignToken.padding
+        Spacer(modifier = Modifier.width(DesignToken.padding.small))

         IconButton(
             onClick = { onAction.invoke(SavingsAccountAction.ToggleFilter) },
         ) {
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientClosure/ClientClosureScreen.kt (1)

112-227: Add Error state to the content visibility condition.

The condition currently hides content only for Loading and ShowStatusDialog, but Error state still renders the form in the composition tree. While MifosErrorComponent is a full-screen component, the form should be conditionally excluded from rendering for better UX and to avoid unnecessary recomposition. Update the condition at line 112 to:

if (state.dialogState != ClientClosureState.DialogState.Loading &&
    state.dialogState !is ClientClosureState.DialogState.ShowStatusDialog &&
    state.dialogState !is ClientClosureState.DialogState.Error
)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientDetailsProfile/ClientProfileDetailsScreen.kt (1)

97-133: Guard against null client before firing navigation actions.
Several branches fall back to -1 when state.client is null, which risks routing to invalid screens if actions can be triggered during incomplete/error states. Consider short-circuiting when the client isn’t available.

🛠️ Suggested guard
-            is ClientProfileDetailsEvent.OnActionClick -> {
+            is ClientProfileDetailsEvent.OnActionClick -> {
+                val client = state.client ?: return@EventsEffect
                 when (event.action) {
                     ClientProfileDetailsActionItem.AddCharge -> {
-                        navigateToAddCharge(state.client?.id ?: -1)
+                        navigateToAddCharge(client.id)
                     }
                     ClientProfileDetailsActionItem.ApplyNewApplication -> {
                         navigateToApplyNewApplication(
-                            state.client?.id ?: -1,
-                            state.client?.status?.value ?: "",
+                            client.id,
+                            client.status?.value.orEmpty(),
                         )
                     }
                     ClientProfileDetailsActionItem.AssignStaff -> {
-                        navigateToAssignStaff(state.client?.id ?: -1)
+                        navigateToAssignStaff(client.id)
                     }
                     ClientProfileDetailsActionItem.ClosureApplication -> {
-                        navigateToClientClosure(state.client?.id ?: -1)
+                        navigateToClientClosure(client.id)
                     }
                     ClientProfileDetailsActionItem.CreateCollateral -> {
-                        navigateToCollateral(state.client?.id ?: -1)
+                        navigateToCollateral(client.id)
                     }
                     ClientProfileDetailsActionItem.TransferClient -> {
-                        navigateToClientTransfer(state.client?.id ?: -1)
+                        navigateToClientTransfer(client.id)
                     }
                     ClientProfileDetailsActionItem.UpdateDefaultAccount -> {
-                        navigateToUpdateDefaultAccount(state.client?.id ?: -1)
+                        navigateToUpdateDefaultAccount(client.id)
                     }
🤖 Fix all issues with AI agents
In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientAddress/ClientAddressScreen.kt`:
- Around line 139-145: The LazyColumn is using a non‑stable key
(state.address.indexOf(it)) which is O(n) and can produce duplicate/unstable
keys; update the items call to use the stable unique identifier from the model
(use address.addressId from ClientAddressEntity) as the key (e.g.,
items(state.address, key = { it.addressId }) ), ensuring addressId is
non-null/converted to a stable String if needed so MifosAddressCard composition
remains stable.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientCollateralDetails/ClientCollateralDetailScreen.kt`:
- Around line 74-83: The Error branch in ClientCollateralDetailScreen renders
MifosErrorComponent without the horizontal padding used by the Empty and Success
branches, causing inconsistent layout; update the
ClientCollateralDetailsState.State.Error branch so the MifosErrorComponent call
includes padding(horizontal = DesignToken.padding.large) (same as other
branches) to align visuals, keeping the existing isNetworkConnected,
isRetryEnabled, onRetry and message parameters unchanged.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientEditDetails/ClientEditDetailsScreen.kt`:
- Around line 180-223: The content under the Column (after
MifosBreadcrumbNavBar) can overflow because children like
MifosProgressIndicator, MifosSweetError and MifosStatusDialog use fillMaxSize();
wrap the when (state.dialogState) { ... } block in a container with weight to
constrain it to remaining space — e.g., replace the direct when block with a Box
(or Column) that uses Modifier.weight(1f, fill = true) and place the when inside
it so those full-size children no longer push content below the breadcrumb;
update references around MifosBreadcrumbNavBar, the when on state.dialogState,
and the listed composables (MifosProgressIndicator, MifosSweetError,
MifosStatusDialog) when making the change.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt`:
- Around line 125-200: The LazyColumn currently wraps a single item { ...
forEachIndexed } which forces all rows to compose eagerly; replace that pattern
with LazyColumn { itemsIndexed(...) { index, item -> ... } } using itemsIndexed
to lazily compose each MifosActionsIdentifierListingComponent (preserving props
like status, uniqueKeyForHandleDocument, onAction, onClick, isExpanded which
uses state.currentExpandedItem and state.expandClientIdentity). Also move the
reversed() call out of the composable (e.g., val displayList = remember {
state.clientIdentitiesList.reversed() } or compute it in the ViewModel) so it
isn’t recomputed on every recomposition before passing displayList into
itemsIndexed.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientStaff/ClientStaffScreen.kt`:
- Line 144: The Spacer used in ClientStaffScreen currently uses
Modifier.padding(DesignToken.padding.small) which has no effect because Spacer
has zero intrinsic size; replace that with a horizontal size modifier (e.g.,
Modifier.width(DesignToken.padding.small)) so the Spacer actually creates
visible gap between the buttons in the Row where it's used.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientTransfer/ClientTransferScreen.kt`:
- Around line 135-144: The code directly indexes state.offices with
state.currentSelectedIndex in the MifosTextFieldDropdown leading to possible
IndexOutOfBoundsException; update the value and options computation to
defensively access the list (e.g., use
state.offices.getOrNull(state.currentSelectedIndex)?.name ?: "" for the
displayed value and ensure options = state.offices.map { it.name ?: "" } remains
safe), or clamp/validate currentSelectedIndex before use, and keep the existing
onOptionSelected calling onAction(ClientTransferAction.OptionChanged(index))
unchanged so selected index handling remains consistent.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/documentPreviewScreen/DocumentPreviewScreen.kt`:
- Around line 78-179: The left button in ViewDocumentContent currently shows
"Back" but invokes DocumentPreviewScreenAction.RejectDocument when state.step ==
EntityDocumentState.Step.PREVIEW — change behavior so label and action align:
either swap the action to DocumentPreviewScreenAction.NavigateBack for PREVIEW
or change the displayed text to "Reject" when state.step == PREVIEW (update the
Text stringResource logic accordingly); additionally protect the bottom action
Row from system insets by adding systemBarsPadding() (or appropriate
WindowInsets.padding(WindowInsets.systemBars)) to the Column or the Row
containing the MifosOutlinedButton controls so the buttons are not obscured by
navigation bars.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt`:
- Around line 137-148: The layout overflows because both the loading branch
(MifosProgressIndicator) and the content branch Column use fillMaxSize() and
thus ignore the space taken by MifosBreadcrumbNavBar; update the branches where
state.isLoading is checked so the MifosProgressIndicator and the inner Column
use Modifier.weight(1f).fillMaxWidth() instead of fillMaxSize(), ensuring they
only occupy the remaining space after MifosBreadcrumbNavBar and prevent
overflow.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/newFixedDepositAccount/CreateFixedDepositAccountScreen.kt`:
- Around line 178-180: The Column is reusing the outer `modifier` (already
passed into `MifosScaffold`) which can duplicate padding/semantics; update the
content inside CreateFixedDepositAccountScreen so the Column uses a fresh
Modifier (e.g., `Modifier.padding(paddingValues)` or another local Modifier
chain) instead of `modifier.padding(paddingValues)` to avoid reapplying the
caller's modifier/semantics.

In
`@feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt`:
- Around line 104-142: The LazyColumn currently uses item {
state.accounts.forEachIndexed { ... } } which forces all rows to compose
eagerly; replace that block with LazyColumn { itemsIndexed(state.accounts) {
index, account -> ... } } so each account is produced lazily and keep the same
inner usage of MifosActionsShareListingComponent and the existing
onClick/onAction handling (ShareAccountsAction.ViewAccount /
ShareAccountsAction.CardClicked); also add the import
androidx.compose.foundation.lazy.itemsIndexed.

In
`@feature/note/src/commonMain/kotlin/com/mifos/feature/note/notes/NoteScreen.kt`:
- Around line 156-163: The MifosScaffold content lambda currently ignores the
provided PaddingValues causing content (Column) to render under system bars;
update the lambda to accept the padding parameter (e.g., "{ paddingValues -> ...
}") and apply it to the Column modifier via .padding(paddingValues) so the
content respects scaffold insets (change the MifosScaffold lambda and the Column
modifier accordingly).
🧹 Nitpick comments (20)
feature/client/src/commonMain/composeResources/values/strings.xml (1)

32-32: Duplicate string resource detected.

This string has the exact same value as update_default_no_accounts_found on line 438:

<string name="update_default_no_accounts_found">No Savings Accounts found for this client</string>

Consider reusing the existing string resource instead of creating a duplicate, or consolidate them into a single entry to reduce localization overhead and maintain consistency.

♻️ Suggested fix: Remove the duplicate and use the existing string
 <string name="feature_client_accounts">Accounts</string>
-    <string name="feature_client_no_savings_found">No Savings Accounts found for this client</string>

Then update the usage in ClientUpdateDefaultAccountScreen.kt to reference update_default_no_accounts_found instead.

Alternatively, if feature_client_* naming is preferred, remove line 438 and keep this new entry, updating any references to update_default_no_accounts_found.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpdateDefaultAccount/ClientUpdateDefaultAccountScreen.kt (1)

90-92: Consider the UX when hiding content during dialog states.

The content is completely hidden when dialogState is Loading or ShowStatusDialog. This means the user sees a blank screen during loading, rather than the form with a loading overlay. This may be intentional for this flow, but worth confirming it matches the expected UX pattern.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientUpcomingCharges/ClientUpcomingChargesScreen.kt (1)

87-89: Consider adding fillMaxSize() to the outer Column.

The outer Column only applies the passed modifier but doesn't explicitly fill the available space. If callers don't pass a size modifier, this could result in the content not expanding as expected. The inner Column (line 96) uses fillMaxSize(), but it's only rendered when not loading.

♻️ Suggested improvement
     Column(
-        modifier = modifier,
+        modifier = modifier.fillMaxSize(),
     ) {
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt (2)

181-182: Use Unit or empty block instead of null for no-op branch.

The else -> null returns a nullable value that's discarded. For clarity and to avoid lint warnings, use else -> Unit or else -> {}.

♻️ Suggested fix
-                                            else -> null
+                                            else -> Unit

186-186: Use DesignToken for consistent spacing.

The rest of the file uses DesignToken.padding.* values, but this line uses a hardcoded 8.dp. Consider using DesignToken.padding.medium (or the appropriate token) for consistency.

♻️ Suggested fix
-                                Spacer(modifier = Modifier.height(8.dp))
+                                Spacer(modifier = Modifier.height(DesignToken.padding.medium))
feature/client/src/commonMain/kotlin/com/mifos/feature/client/recurringDepositAccount/RecurringDepositAccountScreen.kt (3)

124-124: Consider using Unit instead of null in exhaustive when.

The else -> null branch returns a value that isn't used. For clarity in a when used as a statement, prefer else -> Unit or remove the explicit return if the when doesn't need to be exhaustive.


163-177: TODO: Search bar functionality is not implemented.

The comment indicates search functionality needs implementation. The UI components are wired but the actual search logic may be incomplete.

Would you like me to help implement the search functionality or open an issue to track this task?


234-235: Consider using Unit instead of null for the else branch.

Similar to the dialog, else -> null returns an unused value. For a when used as a statement, prefer else -> Unit or simply else -> {}.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientSignature/ClientSignatureScreen.kt (2)

93-94: Consider making ClientSignatureDialog internal for consistency.

ClientSignatureContent is marked internal (line 167), but ClientSignatureDialog lacks a visibility modifier and defaults to public. For screen-specific composables, internal visibility is typically preferred.

Suggested change
 `@Composable`
-fun ClientSignatureDialog(
+internal fun ClientSignatureDialog(
     state: ClientSignatureState,
     onAction: (ClientSignatureAction) -> Unit,
 ) {

287-293: Incomplete implementation: "More" option has empty onClick handler.

The comment // it implement further indicates this is a placeholder. The "More" option currently does nothing when clicked, which may confuse users.

Consider either:

  1. Hiding this option until implemented
  2. Adding a TODO comment with a tracking reference
  3. Showing a "coming soon" toast/snackbar when clicked

Do you want me to open a new issue to track this incomplete functionality?

feature/client/src/commonMain/kotlin/com/mifos/feature/client/savingsAccounts/SavingsAccountsScreen.kt (2)

168-186: Non-idiomatic: else -> null in a Unit-returning lambda.

The onActionClicked callback expects Unit, but line 184 returns null. While this compiles (the value is discarded), it's clearer to use else -> {} or else -> Unit to express "do nothing" intent.

💡 Suggested fix
                                 onActionClicked = { actions ->
                                     when (actions) {
                                         is Actions.ViewAccount -> onAction.invoke(...)
                                         is Actions.ApproveAccount -> onAction.invoke(...)
-                                        else -> null
+                                        else -> {}
                                     }
                                 },

303-306: Non-idiomatic: else -> null in composable when statement.

The else -> null branch is confusing in this context. For a statement-level when inside a @Composable function, use else -> {} or else -> Unit to clearly express "render nothing."

💡 Suggested fix
-        else -> null
+        else -> Unit
     }
 }
feature/note/src/commonMain/kotlin/com/mifos/feature/note/addEditNotes/AddEditNoteScreen.kt (1)

125-139: Empty container when error state is active.

When dialogState is Error, the Column renders empty since both MifosBreadcrumbNavBar and AddEditNote are conditionally hidden. This could cause a brief layout flicker or empty screen while MifosErrorComponent displays from AddEditNoteScreenDialog.

Consider whether the error component should be rendered inline here instead of separately, or ensure the error overlay covers the entire screen to avoid visual artifacts.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/shareAccounts/ShareAccountsScreen.kt (1)

117-128: Consider handling unknown actions explicitly or logging them.

The empty else -> {} branch silently ignores any unhandled Actions types. While currently safe since only ViewAccount is in the menu list, this could mask issues if the menu list is extended in the future.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientTransfer/ClientTransferScreen.kt (1)

155-180: Consider moving DatePickerDialog to ClientTransferDialogs for consistency.

The file already has a dedicated ClientTransferDialogs composable handling all dialog states (Loading, Error, ShowStatusDialog). Moving the DatePickerDialog there would centralize dialog management and align with the established pattern in this file.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientClosure/ClientClosureScreen.kt (1)

202-202: Use Modifier.width() for horizontal spacing between buttons.

Modifier.padding() applies padding on all sides; for a horizontal Row, prefer an explicit width to create horizontal space between the two buttons.

♻️ Suggested fix
-                        Spacer(Modifier.padding(DesignToken.padding.small))
+                        Spacer(Modifier.width(DesignToken.padding.small))
feature/note/src/commonMain/kotlin/com/mifos/feature/note/notes/NoteScreen.kt (1)

214-220: Minor: Missing modifier = named argument for clarity.

The Modifier is passed as a positional argument to Icon. While valid, using the named argument improves readability and consistency.

Suggested improvement
             Icon(
                 imageVector = MifosIcons.Add,
                 contentDescription = null,
-                Modifier.clickable {
+                modifier = Modifier.clickable {
                     onAction(NoteAction.OnClickAddScreen)
                 }.size(DesignToken.sizes.iconAverage),
             )
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientIdentifiersList/ClientIdentifiersListScreen.kt (1)

142-143: Consider a more robust unique key generation.

The current concatenation can produce collisions when nullable fields are null (e.g., "nullnullActive"). Consider using a delimiter or the item's id for uniqueness:

💡 Suggested improvement
-                            val uniqueKeyForHandleDocument =
-                                item.documentType?.name + item.documentKey + status
+                            val uniqueKeyForHandleDocument =
+                                "${item.documentType?.name.orEmpty()}_${item.documentKey.orEmpty()}_${status?.name.orEmpty()}"

Or, if item.id is always unique when present:

val uniqueKeyForHandleDocument = item.id?.toString() 
    ?: "${item.documentType?.name}_${item.documentKey}_${status?.name}"
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientGeneral/ClientProfileGeneralScreen.kt (1)

191-218: Consider centralizing click handling for icon row cards.
Click behavior is now attached via Modifier.clickable (Line 212-217) because the icon-based MifosRowCard overload has no onClick. If consistent styling/ripple or reuse is desired, consider adding an onClick param to that overload and handling it internally.

feature/client/src/commonMain/kotlin/com/mifos/feature/client/fixedDepositAccount/FixedDepositAccountScreen.kt (1)

164-178: Track or remove the TODO for search.

The TODO indicates incomplete work. If search is already wired through FixedDepositAccountAction, consider removing the comment; otherwise, please link it to a ticket so it doesn’t get lost.

Copy link
Contributor

@biplab1 biplab1 left a comment

Choose a reason for hiding this comment

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

Some minor suggestions requested.

Looks good to me. This can be merged.

@biplab1
Copy link
Contributor

biplab1 commented Jan 29, 2026

@coderabbitai Can you improve the PR title based on this document: https://github.com/openMF/mobile-wallet/wiki/Commit-style-guide? Give us just the title.

@coderabbitai
Copy link

coderabbitai bot commented Jan 29, 2026

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🌐 Web query:

site:github.com/openMF/mobile-wallet/wiki/Commit-style-guide

💡 Result:

The openMF/mobile-wallet wiki “Commit style guide” specifies:

  • Commit format: <type>(<scope>): <subject> (with <scope> optional, e.g. content-docs, theme-classic, core) [1]
  • Allowed types: feat, fix, docs, refactor, test, chore, misc [1]
  • Example: feat(core): allow overriding of gradle config [1]

Source: [1]


@biplab1

refactor(client): remove redundant scaffolds and navigation elements

Co-authored-by: Biplab Dutta <biplabdutta27@gmail.com>
@biplab1
Copy link
Contributor

biplab1 commented Jan 29, 2026

@Arinyadav1 Could you please update the PR title to this: refactor: remove redundant scaffold, back icon and screen name?

@Arinyadav1 Arinyadav1 changed the title remove redundant scaffold, back icon and screen name refactor: remove redundant scaffold, back icon and screen name Jan 29, 2026
@therajanmaurya therajanmaurya enabled auto-merge (squash) January 29, 2026 16:43
@therajanmaurya therajanmaurya enabled auto-merge (squash) January 29, 2026 16:43
@therajanmaurya therajanmaurya enabled auto-merge (squash) January 30, 2026 04:38
@therajanmaurya therajanmaurya enabled auto-merge (squash) January 30, 2026 14:37
@therajanmaurya therajanmaurya merged commit 1cc720c into openMF:development Jan 30, 2026
2 checks passed
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.

3 participants