Loan detail UI #2562
Loan detail UI #2562
Conversation
📝 WalkthroughWalkthroughClientLoanAccounts action now carries a concrete loanId; ViewModel and screen callsites updated. LoanAccountSummary screen and route signatures accept a NavController and UI was refactored (breadcrumb, InfoCard, copy-to-clipboard). New icons and color tokens plus several loan-related strings were added. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as ClientLoanAccountsScreen
participant VM as ClientLoanAccountsViewModel
participant Nav as NavController
participant Loan as LoanAccountSummaryScreen
UI->>VM: dispatch ViewAccount(loanId)
note right of VM: action carries loanId
VM->>Nav: navigate to loanSummaryRoute(loanId)
note right of Nav: routes to LoanAccountSummaryScreen with navController
Nav->>Loan: present LoanAccountSummaryScreen(navController, loanId)
Loan->>Loan: render breadcrumb, InfoCards
Loan->>Loan: copy loan id -> clipboard + snackbar
Loan-->>Nav: use navController for further navigation (back/more)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://waps.l3s.uni-hannover.de/live/im_/https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (3)
320-345: Improve accessibility for the status indicator.The
Canvashas an emptycontentDescription, which means screen readers won't announce the loan status. Since the status is conveyed only through color, users with visual impairments may miss this information.Consider providing a meaningful content description based on the status:
🔎 Suggested improvement
Canvas( modifier = Modifier .size(22.dp), - contentDescription = "", + contentDescription = when { + loanWithAssociations.status.active == true -> "Active" + loanWithAssociations.status.pendingApproval == true -> "Pending Approval" + loanWithAssociations.status.waitingForDisbursal == true -> "Waiting for Disbursal" + else -> "Closed" + }, onDraw = {
364-382: Localize the snackbar message and consider icon semantics.
- The "Copied to clipboard" message on line 369 is hardcoded. For i18n support, use a string resource.
MifosIcons.Shareis used for a copy action, which may be confusing. Consider using a copy-specific icon if available inMifosIcons.🔎 Suggested fix for localization
+ val copiedMessage = stringResource(Res.string.feature_loan_copied_to_clipboard) scope.launch { snackbarHostState.showSnackbar( - message = "Copied to clipboard", + message = copiedMessage, ) }
412-414: Localize "Loan Overview" title.The string "Loan Overview" on line 413 is hardcoded. Use a string resource for i18n consistency.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.ktfeature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreenRoute.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt
🔇 Additional comments (8)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/navigation/LoanNavigation.kt (1)
49-49: LGTM!The
navControlleris correctly threaded through toloanAccountSummary, consistent with the existing pattern used fornewLoanAccountDestinationon line 73.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreenRoute.kt (1)
37-50: LGTM!The
navControllerparameter is correctly added to the function signature and properly forwarded toLoanAccountSummaryScreen. The wiring aligns with the changes inLoanNavigation.kt.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (4)
116-144: LGTM!The
navControllerparameter is correctly added to the internal composable and properly forwarded to the UI-state variant.
224-234: Verify breadcrumb navigation bar layout.
MifosBreadcrumbNavBaris placed outside theBoxthat receives the scaffold padding (it). This means the breadcrumb bar won't respect the scaffold's content padding, which could lead to visual inconsistency if the scaffold has top padding.If the breadcrumb should be edge-to-edge, this is fine. Otherwise, consider moving it inside the padded
Boxor applying horizontal padding to it.
613-641: LGTM!The
InfoCardcomposable is well-structured. The fixed height of 80.dp works for the current content but consider usingwrapContentHeightwith aminHeightconstraint if content length varies significantly in different locales.
809-830: LGTM!The preview correctly uses
rememberNavController()to provide a mock controller and wraps the content withMifosThemefor accurate theme rendering.feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt (2)
70-74: Good fix: Using the correct loan ID.This change correctly passes the specific loan ID through the action parameter instead of incorrectly using
state.clientId. This fix ensures the correct loan account is viewed.
161-161: LGTM: Action signature updated appropriately.The ViewAccount action now carries the loan ID as a parameter, which aligns with the handler changes and improves data flow clarity.
| is Actions.ViewAccount -> onAction( | ||
| ClientLoanAccountsAction.ViewAccount(loan.id ?: 0), | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's understand the codebase structure
find . -name "*.kt" -path "*/client/*" | head -20Repository: openMF/android-client
Length of output: 1933
🏁 Script executed:
#!/bin/bash
# Find ViewAccount event/action handling in the codebase
rg -n "ViewAccount" --type=kt -B 2 -A 5Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Check for loan ID validation patterns
rg -n "loanId.*==.*0|id.*==.*0|loanId.*!=.*null|id.*!=.*null" --type=kt -C 3Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Look for repository or API calls that use loan ID as parameter
rg -n "fun.*getLoan|fun.*viewLoan|fun.*fetchLoan" --type=kt -A 3Repository: openMF/android-client
Length of output: 91
🏁 Script executed:
#!/bin/bash
# Search for ViewAccount in Kotlin files (using extension pattern)
rg -n "ViewAccount" --include="*.kt" -B 2 -A 5Repository: openMF/android-client
Length of output: 506
🏁 Script executed:
#!/bin/bash
# Look at the specific file mentioned in the review
cat -n ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt | sed -n '175,195p'Repository: openMF/android-client
Length of output: 1397
🏁 Script executed:
#!/bin/bash
# Search for loan ID validation or special handling of ID 0
rg -n "(loanId|loan\.id).*(==.*0|!=.*0|validate|check)" --include="*.kt" -C 3Repository: openMF/android-client
Length of output: 506
🏁 Script executed:
#!/bin/bash
# Find where the ViewAccount action leads to (handlers, navigation, etc)
rg -n "ClientLoanAccountsAction.ViewAccount" --include="*.kt" -B 2 -A 5Repository: openMF/android-client
Length of output: 506
🏁 Script executed:
#!/bin/bash
# Search for ViewAccount pattern across Kotlin files
fd '\.kt$' | xargs grep -l "ViewAccount" | head -20Repository: openMF/android-client
Length of output: 1969
🏁 Script executed:
#!/bin/bash
# Look for ClientLoanAccountsAction class definition
fd '\.kt$' | xargs grep -n "class ClientLoanAccountsAction\|sealed class ClientLoanAccountsAction\|object ClientLoanAccountsAction"Repository: openMF/android-client
Length of output: 49
🏁 Script executed:
#!/bin/bash
# Find where ViewAccount action is handled (look for when/case statements)
fd '\.kt$' | xargs grep -n "ClientLoanAccountsAction.ViewAccount" -A 3 -B 1Repository: openMF/android-client
Length of output: 1799
🏁 Script executed:
#!/bin/bash
# Search for any loan ID validation or special handling
fd '\.kt$' | xargs grep -n "id.*==.*0\|loanId.*==.*0\|loanId.*!=.*null"Repository: openMF/android-client
Length of output: 495
🏁 Script executed:
#!/bin/bash
# Find the ClientLoanAccountsAction definition
fd '\.kt$' | xargs grep -n "sealed class\|data class.*ViewAccount\|class.*ViewAccount" | grep -i "loanaccounts"Repository: openMF/android-client
Length of output: 583
🏁 Script executed:
#!/bin/bash
# Look for the event handler - where ViewAccount event is processed
fd '\.kt$' | xargs grep -n "ClientLoanAccountsEvent.ViewAccount\|is.*ViewAccount.*->.*navigate\|is.*ViewAccount.*->"Repository: openMF/android-client
Length of output: 4533
🏁 Script executed:
#!/antml/bin
# Check ClientLoanAccountsRoute for navigation handling
cat -n ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsRoute.ktRepository: openMF/android-client
Length of output: 1620
🏁 Script executed:
#!/bin/bash
# Look for LaunchedEffect or collectAsState in the ViewModel to see event handling
fd 'ClientLoanAccountsViewModel.kt$' | xargs cat -n | grep -A 20 "LaunchedEffect\|collectAsState\|event"Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Find where navigateToViewAccount is called/implemented (loan detail screen)
fd '\.kt$' | xargs grep -n "navigateToViewAccount\|LoanDetail\|LoanAccount.*Screen" | grep -v "ClientLoanAccounts"Repository: openMF/android-client
Length of output: 12981
🏁 Script executed:
#!/bin/bash
# Check the full ClientLoanAccountsScreen to see the viewAccount callback
cat -n ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt | head -100Repository: openMF/android-client
Length of output: 4925
🏁 Script executed:
#!/bin/bash
# Look for loan detail loading/fetching logic
fd '\.kt$' | xargs grep -n "fun.*getLoan\|suspend.*getLoan\|repository.*getLoan\|loanId.*Int" | grep -i "loan.*detail\|loan.*account" | head -20Repository: openMF/android-client
Length of output: 3313
🏁 Script executed:
#!/bin/bash
# Check if there are any data model definitions for loans
fd '\.kt$' | xargs grep -n "data class.*Loan\|class.*Loan" | grep -i "id.*Int\|loanId" | head -15Repository: openMF/android-client
Length of output: 797
🏁 Script executed:
#!/bin/bash
# Check LoanAccountSummaryScreen for state handling
cat -n ./feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt | sed -n '100,200p'Repository: openMF/android-client
Length of output: 4812
🏁 Script executed:
#!/bin/bash
# Check LoanAccountSummaryViewModel initialization and error handling
fd 'LoanAccountSummaryViewModel.kt' | xargs cat -n | head -80Repository: openMF/android-client
Length of output: 3000
🏁 Script executed:
#!/bin/bash
# Look for the ClientLoanAccountsScreen data model - check if loan.id can actually be null
fd '\.kt$' | xargs grep -n "data class.*Loan\|class.*Loan" -A 3 | grep -E "loan|id.*Int|nullable" | head -30Repository: openMF/android-client
Length of output: 5069
🏁 Script executed:
#!/bin/bash
# Check if there are any null checks or validations for loan data in the summary screen
fd 'LoanAccountSummary' | xargs grep -n "if.*null\|?.let\|?:\|!!" | head -20Repository: openMF/android-client
Length of output: 3474
🏁 Script executed:
#!/bin/bash
# Find the loan model/entity definition used in ClientLoanAccountsScreen
fd '\.kt$' | xargs grep -l "LoanWithAssociationsEntity\|LoanEntity" | head -5Repository: openMF/android-client
Length of output: 550
🏁 Script executed:
#!/bin/bash
# Get the structure of loan entity to see if id is nullable
fd 'LoanWithAssociationsEntity.kt$' | xargs cat -n | head -50Repository: openMF/android-client
Length of output: 2470
🏁 Script executed:
#!/bin/bash
# Check what data flows into ClientLoanAccountsScreen - the list of loans
cat -n ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt | sed -n '90,120p'Repository: openMF/android-client
Length of output: 1384
🏁 Script executed:
#!/bin/bash
# Get more of the LoanWithAssociationsEntity definition to see if id is nullable
fd 'LoanWithAssociationsEntity.kt$' | xargs cat -n | sed -n '50,150p'Repository: openMF/android-client
Length of output: 3486
🏁 Script executed:
#!/bin/bash
# Check how loans are displayed in the ClientLoanAccountsScreen to understand the data
cat -n ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt | sed -n '140,185p'Repository: openMF/android-client
Length of output: 3519
🏁 Script executed:
#!/bin/bash
# Look at the full loan accounts fetching to see if null IDs are filtered earlier
cat -n ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsViewModel.kt | sed -n '100,125p'Repository: openMF/android-client
Length of output: 1274
🏁 Script executed:
#!/bin/bash
# Check the type definition of the loanAccounts list in ClientLoanAccountsState
fd 'ClientLoanAccountsViewModel.kt$' | xargs grep -n "data class ClientLoanAccountsState\|loanAccounts.*List" -A 3Repository: openMF/android-client
Length of output: 421
🏁 Script executed:
#!/bin/bash
# Check if there's a LoansWithAssociations type that wraps the loan list
fd '\.kt$' | xargs grep -n "class.*Loans\|data class.*Loans" | grep -i "loanaccounts\|getClientAccounts" | head -10Repository: openMF/android-client
Length of output: 47
🏁 Script executed:
#!/bin/bash
# Look for the repository's getClientAccounts return type
fd '\.kt$' | xargs grep -n "fun getClientAccounts\|getClientAccounts.*:" -A 2 | head -20Repository: openMF/android-client
Length of output: 2304
🏁 Script executed:
#!/bin/bash
# Find LoanAccountEntity definition
fd 'LoanAccountEntity.kt$' | xargs cat -n | head -100Repository: openMF/android-client
Length of output: 3294
🏁 Script executed:
#!/bin/bash
# Search for LoanAccountEntity to see its id field
fd '\.kt$' | xargs grep -n "data class LoanAccountEntity\|class LoanAccountEntity" -A 10 | head -30Repository: openMF/android-client
Length of output: 1367
🏁 Script executed:
#!/bin/bash
# Check the actual data flow - what entity type contains the loan.id in the screen
fd '\.kt$' | xargs grep -n "LoanAccountEntity" | grep -i "id.*Int\|id.*nullable" | head -15Repository: openMF/android-client
Length of output: 213
Handle null loan ID properly instead of defaulting to 0.
loan.id is nullable (defined as Int? in LoanAccountEntity), and defaulting to 0 when null masks data quality issues:
- Loans with null IDs should not appear in the list; defaulting to 0 silently hides this problem
- Attempting to load a loan with ID 0 will return no results, displaying an empty loan with all default values
- This masks legitimate data integrity issues that should be surfaced to the user
Consider filtering out loans with null IDs earlier in the data flow, or validate that loan.id is not null before enabling the ViewAccount action.
🤖 Prompt for AI Agents
In
feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientLoanAccounts/ClientLoanAccountsScreen.kt
around lines 182-184, the code currently calls ViewAccount using loan.id ?: 0
which masks null IDs; instead ensure you never call ViewAccount with a null id
by either (A) filtering out LoanAccountEntity items with null id earlier (e.g.,
when mapping/collecting the loan list) so the UI only binds non-null ids, or (B)
check loan.id before invoking onAction and only call
onAction(ClientLoanAccountsAction.ViewAccount(id)) when id != null, otherwise
disable/hide the view button or show a validation/error state informing the user
of a data integrity problem; pick one approach and apply it consistently so a 0
id is never used as a fallback.
| Row { | ||
| InfoCard( | ||
| titleText = "Total Loan", | ||
| infoText = summary?.totalExpectedRepayment.toString(), | ||
| modifier = Modifier.fillMaxWidth(0.5f), | ||
| ) | ||
| Spacer(modifier = Modifier.width(12.dp)) | ||
| InfoCard( | ||
| titleText = "Amount Paid", | ||
| infoText = summary?.totalRepayment.toString(), | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) | ||
| } | ||
|
|
||
| HorizontalDivider( | ||
| modifier = Modifier | ||
| .fillMaxWidth() | ||
| .padding(top = 2.dp), | ||
| ) | ||
|
|
||
| LoanSummaryFarApartTextItem( | ||
| title = stringResource(Res.string.feature_loan_loan_amount_disbursed), | ||
| value = if (inflateLoanSummary) { | ||
| loanWithAssociations.summary.principalDisbursed?.toString() | ||
| ?: "" | ||
| } else { | ||
| "" | ||
| }, | ||
| InfoCard( | ||
| titleText = "Outstanding Balance", | ||
| infoText = summary?.totalOutstanding.toString(), | ||
| modifier = Modifier.fillMaxWidth(), | ||
| ) |
There was a problem hiding this comment.
Handle null values properly and localize UI strings.
-
Null display issue: When
summaryis null (i.e.,inflateLoanSummaryis false), callingsummary?.totalExpectedRepayment.toString()will produce the literal string"null"in the UI instead of an empty or placeholder value. -
Hardcoded strings: "Total Loan", "Amount Paid", and "Outstanding Balance" should use string resources for i18n.
🔎 Suggested fix for null handling
Row {
InfoCard(
- titleText = "Total Loan",
- infoText = summary?.totalExpectedRepayment.toString(),
+ titleText = stringResource(Res.string.feature_loan_total_loan),
+ infoText = summary?.totalExpectedRepayment?.toString() ?: "",
modifier = Modifier.fillMaxWidth(0.5f),
)
Spacer(modifier = Modifier.width(12.dp))
InfoCard(
- titleText = "Amount Paid",
- infoText = summary?.totalRepayment.toString(),
+ titleText = stringResource(Res.string.feature_loan_amount_paid),
+ infoText = summary?.totalRepayment?.toString() ?: "",
modifier = Modifier.fillMaxWidth(),
)
}
InfoCard(
- titleText = "Outstanding Balance",
- infoText = summary?.totalOutstanding.toString(),
+ titleText = stringResource(Res.string.feature_loan_outstanding_balance),
+ infoText = summary?.totalOutstanding?.toString() ?: "",
modifier = Modifier.fillMaxWidth(),
)Committable suggestion skipped: line range outside the PR's diff.
|
I have edited your PR description. Please compare and fix |
| approveLoan: (loadId: Int, loanWithAssociations: LoanWithAssociationsEntity) -> Unit, | ||
| disburseLoan: (loanId: Int) -> Unit, | ||
| onRepaymentClick: (loanWithAssociations: LoanWithAssociationsEntity) -> Unit, | ||
| navController: NavController, |
There was a problem hiding this comment.
why are we passing navcontroller here, take reference from other screens and fix it
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Outdated
Show resolved
Hide resolved
.../src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (3)
121-121: Verify NavController passing pattern aligns with project conventions.A previous reviewer flagged that passing
NavControllerdirectly to the screen may not follow the project's navigation patterns. Consider using navigation callbacks or checking how other screens in the codebase handle breadcrumb navigation.#!/bin/bash # Check how other screens handle NavController or MifosBreadcrumbNavBar usage rg -n "MifosBreadcrumbNavBar" --type=kt -C2
324-349: Use semantic theme colors instead of hardcoded Color values.Previous reviews flagged that
Color.Green,Color.Yellow,Color.Blue, andColor.Blackshould not be hardcoded. UseMaterialTheme.colorSchemesemantic colors (e.g.,tertiary,error,primary) or register status-specific colors inAppColors.🔎 Suggested approach
drawCircle( color = when { loanWithAssociations.status.active == true -> { - Color.Green + MaterialTheme.colorScheme.tertiary // or AppColors.statusActive } loanWithAssociations.status.pendingApproval == true -> { - Color.Yellow + MaterialTheme.colorScheme.secondary // or AppColors.statusPending } loanWithAssociations.status.waitingForDisbursal == true -> { - Color.Blue + MaterialTheme.colorScheme.primary // or AppColors.statusWaiting } else -> { - Color.Black + MaterialTheme.colorScheme.onSurfaceVariant // or AppColors.statusDefault } }, )Note: Since
Canvasrequires aColorat draw time, you'll need to capture the color value outside theCanvasblock or usedrawBehindmodifier.
390-407: Null values display as literal "null" string in UI.When
summaryis null (i.e.,inflateLoanSummaryis false),summary?.totalExpectedRepayment.toString()evaluates tonull.toString()which produces the literal string"null"in the UI. This was flagged in a previous review but remains unfixed.🔎 Proposed fix
Row { InfoCard( titleText = stringResource(Res.string.feature_loan_total_loan), - infoText = summary?.totalExpectedRepayment.toString(), + infoText = summary?.totalExpectedRepayment?.toString() ?: "", modifier = Modifier.fillMaxWidth(0.5f), ) Spacer(modifier = Modifier.width(DesignToken.spacing.medium)) InfoCard( titleText = stringResource(Res.string.feature_loan_amount_paid), - infoText = summary?.totalRepayment.toString(), + infoText = summary?.totalRepayment?.toString() ?: "", modifier = Modifier.fillMaxWidth(), ) } InfoCard( titleText = stringResource(Res.string.feature_loan_outstanding_balance), - infoText = summary?.totalOutstanding.toString(), + infoText = summary?.totalOutstanding?.toString() ?: "", modifier = Modifier.fillMaxWidth(), )
🧹 Nitpick comments (2)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
607-632: Consider providing a default modifier value.The
modifierparameter requires callers to always specify it. Consider providing a default value for flexibility.🔎 Suggested change
@Composable private fun InfoCard( titleText: String, infoText: String, - modifier: Modifier, + modifier: Modifier = Modifier, ) {feature/loan/src/commonMain/composeResources/values/strings.xml (1)
139-139: Consider more specific clipboard feedback message.While "Copied to clipboard" is clear, a more specific message like "Loan ID copied to clipboard" provides better context to the user about what was copied.
🔎 Proposed improvement
- <string name="feature_loan_loan_id_copied">Copied to clipboard</string> + <string name="feature_loan_loan_id_copied">Loan ID copied to clipboard</string>
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktfeature/loan/src/commonMain/composeResources/values/strings.xmlfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (4)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosProgressIndicator.kt (1)
MifosProgressIndicator(41-68)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosCard.kt (1)
MifosCard(32-55)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Theme.kt (1)
MifosTheme(250-275)
🔇 Additional comments (8)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.kt (1)
248-248: LGTM!The
Copyicon addition follows the existing naming convention and correctly maps toIcons.Outlined.ContentCopyfor the copy-to-clipboard feature.feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (4)
368-386: LGTM!The copy-to-clipboard implementation properly uses
LocalClipboardManager, provides snackbar feedback, uses localized strings, and follows design token conventions for icon sizing.
634-693: LGTM!The
DataTableRowupdates correctly use uniform weight distribution, consistent design tokens for padding, and proper theme colors for text styling.
799-820: LGTM!The preview function correctly wraps the component in
MifosThemeand usesrememberNavController()for preview purposes.
298-304: Good use of design tokens for consistent spacing.The layout correctly uses
DesignToken.padding.largefor horizontal padding andArrangement.spacedByfor consistent vertical spacing between elements.feature/loan/src/commonMain/composeResources/values/strings.xml (3)
109-109: LGTM!The string resource is correctly formatted and follows naming conventions.
138-138: No issues identified. The trailing "#" is intentional and serves as a prefix to the account number. InLoanAccountSummaryScreen.kt, the string is concatenated directly with the loan account number (loanWithAssociations.accountNo), resulting in a properly formatted display like "Loan ID: #12345", which is a standard UX pattern for displaying identifiers.
136-142: This concern does not apply to the project's architecture. All feature modules in the codebase (about, activate, auth, center, checker-inbox-task, collectionSheet, data-table, document, groups, loan, offline, path-tracking, report, settings, etc.) follow the same pattern: they usecomposeResourceswith only a defaultvalues/strings.xmlfile and no locale-specific subdirectories. The new strings are consistent with the established structure for feature modules.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (3)
121-121: NavController passing pattern - already flagged by reviewer.This concern was raised in a previous review. Follow the navigation patterns used in other screens in the codebase, typically using navigation callbacks rather than passing NavController directly to composables.
Also applies to: 148-148, 165-165
324-349: Invalid Canvas parameter and hardcoded colors.
Potential compilation error: The standard
androidx.compose.foundation.Canvascomposable does not accept acontentDescriptionparameter. Its signature isCanvas(modifier: Modifier, onDraw: DrawScope.() -> Unit). Line 327-328 will cause a compilation error unless there's a custom overload.Hardcoded colors (already flagged): Use Material theme colors or register colors in AppColors instead of
Color.Green,Color.Yellow,Color.Blue,Color.Black.🔎 Suggested fix
- Canvas( - modifier = Modifier - .size(DesignToken.sizes.iconMedium), - contentDescription = "", - onDraw = { - drawCircle( - color = when { - loanWithAssociations.status.active == true -> { - Color.Green - } - - loanWithAssociations.status.pendingApproval == true -> { - Color.Yellow - } - - loanWithAssociations.status.waitingForDisbursal == true -> { - Color.Blue - } - - else -> { - Color.Black - } - }, - ) - }, - ) + Canvas( + modifier = Modifier + .size(DesignToken.sizes.iconMedium), + ) { + drawCircle( + color = when { + loanWithAssociations.status.active == true -> { + // TODO: Use MaterialTheme.colorScheme or AppColors + Color.Green + } + loanWithAssociations.status.pendingApproval == true -> { + Color.Yellow + } + loanWithAssociations.status.waitingForDisbursal == true -> { + Color.Blue + } + else -> { + Color.Black + } + }, + ) + }#!/bin/bash # Verify Canvas composable signature in the codebase echo "Checking for custom Canvas composable overloads..." rg -n "fun Canvas\(" --type=kotlin echo "" echo "Checking standard Canvas import usage..." rg -n "import androidx.compose.foundation.Canvas" --type=kotlin -l | head -5 | xargs -I {} sh -c 'echo "File: {}"; rg -A5 "Canvas\(" {} | head -20'
390-407: Null values display as "null" string - previously flagged.The null handling issue remains: when
summaryis null,summary?.totalExpectedRepayment.toString()produces the literal string"null"in the UI because.toString()is called on a null value.🔎 Fix null handling
Row { InfoCard( titleText = stringResource(Res.string.feature_loan_total_loan), - infoText = summary?.totalExpectedRepayment.toString(), + infoText = summary?.totalExpectedRepayment?.toString() ?: "", modifier = Modifier.fillMaxWidth(0.5f), ) Spacer(modifier = Modifier.width(DesignToken.spacing.medium)) InfoCard( titleText = stringResource(Res.string.feature_loan_amount_paid), - infoText = summary?.totalRepayment.toString(), + infoText = summary?.totalRepayment?.toString() ?: "", modifier = Modifier.fillMaxWidth(), ) } InfoCard( titleText = stringResource(Res.string.feature_loan_outstanding_balance), - infoText = summary?.totalOutstanding.toString(), + infoText = summary?.totalOutstanding?.toString() ?: "", modifier = Modifier.fillMaxWidth(), )
🧹 Nitpick comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (3)
554-554: Consider using DesignToken for divider thickness.The hardcoded
0.5.dpthickness could be replaced with a DesignToken value for consistency with the rest of the codebase styling approach.Also applies to: 563-563, 572-572
607-632: Semantic mismatch: usingtopAppBarHeightfor card height.Line 614 uses
DesignToken.sizes.topAppBarHeightfor the InfoCard height. This size constant is semantically intended for the top app bar, not content cards. Consider using a more appropriate size constant (e.g., a card-specific height) or defining a new one in DesignToken for this purpose.MifosCard( - modifier = modifier.height(DesignToken.sizes.topAppBarHeight), + modifier = modifier.height(DesignToken.sizes.cardHeight), // or an appropriate size constant ) {
472-484: Minor: Incorrect content description for staff icon.Line 477 uses
stringResource(Res.string.feature_loan_info)as the content description for a Person icon representing staff. Consider usingstringResource(Res.string.feature_loan_staff)for better accessibility accuracy.Icon( imageVector = MifosIcons.Person, - contentDescription = stringResource(Res.string.feature_loan_info), + contentDescription = stringResource(Res.string.feature_loan_staff), modifier = Modifier.size(DesignToken.sizes.iconAverage), )
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (3)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosCard.kt (1)
MifosCard(32-55)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Theme.kt (1)
MifosTheme(250-275)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (3)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (3)
268-296: Good implementation of clipboard functionality and error handling.The clipboard integration using
LocalClipboardManagerwith snackbar feedback is well-implemented. The date parsing function includes appropriate exception handling.
358-386: Well-implemented copy-to-clipboard feature.The copy functionality properly uses
LocalClipboardManager, provides user feedback via snackbar, and follows DesignToken sizing patterns. The icon and button are appropriately sized.
799-820: Preview implementation looks good.Using
rememberNavController()is appropriate for preview contexts since it provides a mock NavController without requiring actual navigation infrastructure.
… LoanAccountSummaryScreen.kt
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (1)
391-408: Fix null handling to avoid displaying "null" text.When
summaryis null, calling.toString()on a nullable value produces the literal string"null"in the UI instead of an empty value. This issue was previously flagged but remains unresolved.🔎 Proposed fix for null handling
Row { InfoCard( titleText = stringResource(Res.string.feature_loan_total_loan), - infoText = summary?.totalExpectedRepayment.toString(), + infoText = summary?.totalExpectedRepayment?.toString() ?: "", modifier = Modifier.fillMaxWidth(0.5f), ) Spacer(modifier = Modifier.width(DesignToken.spacing.medium)) InfoCard( titleText = stringResource(Res.string.feature_loan_amount_paid), - infoText = summary?.totalRepayment.toString(), + infoText = summary?.totalRepayment?.toString() ?: "", modifier = Modifier.fillMaxWidth(), ) } InfoCard( titleText = stringResource(Res.string.feature_loan_outstanding_balance), - infoText = summary?.totalOutstanding.toString(), + infoText = summary?.totalOutstanding?.toString() ?: "", modifier = Modifier.fillMaxWidth(), )Based on past review comments.
🧹 Nitpick comments (1)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.kt (1)
252-255: Consider using specific hex color values for better design consistency.The basic
Color.Green,Color.Yellow,Color.Blue, andColor.Blackvalues may not provide optimal contrast ratios or align with the design system's color palette. The existing color constants in this file use specific hex values that are carefully chosen for accessibility and visual consistency.Consider defining more specific values that:
- Provide sufficient contrast ratios for accessibility
- Align with the existing design language
- Work well in both light and dark themes
🔎 Example with specific color values
- val loanIndicatorActive = Color.Green - val loanIndicatorPending = Color.Yellow - val loanIndicatorWaitingForDisbursal = Color.Blue - val loanIndicatorOther = Color.Black + val loanIndicatorActive = Color(0xFF4CAF50) // Material Green 500 + val loanIndicatorPending = Color(0xFFFFC107) // Material Amber 500 + val loanIndicatorWaitingForDisbursal = Color(0xFF2196F3) // Material Blue 500 + val loanIndicatorOther = Color(0xFF616161) // Material Grey 700
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.ktcore/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Color.ktfeature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
🚧 Files skipped from review as they are similar to previous changes (1)
- core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/icon/MifosIcons.kt
🧰 Additional context used
🧬 Code graph analysis (1)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (4)
core/ui/src/commonMain/kotlin/com/mifos/core/ui/components/MifosBreadCrumb.kt (1)
MifosBreadcrumbNavBar(36-107)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosSweetError.kt (1)
MifosSweetError(40-83)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/component/MifosCard.kt (1)
MifosCard(32-55)core/designsystem/src/commonMain/kotlin/com/mifos/core/designsystem/theme/Theme.kt (1)
MifosTheme(250-275)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: PR Checks / Static Analysis Check
🔇 Additional comments (6)
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt (6)
369-387: LGTM! Well-implemented clipboard functionality.The copy-to-clipboard feature is properly implemented using:
LocalClipboardManagerfor clipboard accessAnnotatedStringwrapper as required by the API- Localized string resources for user feedback
DesignTokenfor consistent sizing- Snackbar notification for user confirmation
609-633: LGTM! Well-structured InfoCard component.The new
InfoCardcomposable follows the project's design system patterns:
- Uses
MifosCardas the base component- Applies
DesignTokenfor consistent spacing and padding- Uses
MifosTypographyandMaterialTheme.colorSchemefor theming- Clean, reusable component with clear parameters
230-265: LGTM! Well-structured layout with breadcrumb navigation.The content layout improvements include:
- Clean separation of breadcrumb navigation and main content
- Proper UI state handling with exhaustive
whenexpression- Consistent use of
DesignTokenfor spacing- Appropriate theme color usage
- Clear visual hierarchy
306-389: LGTM! Comprehensive loan overview card with status indicator.The loan overview card implementation is well-structured:
- Status indicator uses
Canvasto draw a colored dot withAppColorsvalues- Copy-to-clipboard functionality for loan ID with user feedback
- Consistent use of
DesignTokenfor all spacing, padding, and sizing- Localized string resources throughout
- Clean visual hierarchy with proper typography
410-487: LGTM! Consistent loan details section with icons.The loan details section demonstrates good practices:
- Consistent icon sizing using
DesignToken.sizes.iconAverage- Proper accessibility with content descriptions for all icons
- Uniform spacing with
DesignToken.spacing.medium- Clean structure with icon + text pattern
- Localized string resources throughout
636-694: LGTM! Improved DataTableRow with consistent styling.The
DataTableRowcomponent improvements include:
- Uniform column widths using
.weight(1f)for all columns- Consistent padding with
DesignToken.padding.small- Material theme integration for typography and colors
- Proper alignment and arrangement properties
- Clean, maintainable table structure
| approveLoan: (loadId: Int, loanWithAssociations: LoanWithAssociationsEntity) -> Unit, | ||
| disburseLoan: (loanId: Int) -> Unit, | ||
| onRepaymentClick: (loanWithAssociations: LoanWithAssociationsEntity) -> Unit, | ||
| navController: NavController, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for Screen composables and their parameters
echo "=== Searching for Screen composables with their parameters ==="
rg -nP --type=kt '@Composable\s+fun\s+\w+Screen\s*\(' | head -50Repository: openMF/android-client
Length of output: 171
🏁 Script executed:
#!/bin/bash
# Check if MifosBreadcrumbNavBar is commonly used with NavController
echo "=== MifosBreadcrumbNavBar usage pattern ==="
rg -nP --type=kt 'MifosBreadcrumbNavBar' -A2 -B2 | head -80Repository: openMF/android-client
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Search for NavController imports in screen files
echo "=== Screen files importing NavController ==="
find . -name '*Screen.kt' -type f ! -path '*/build/*' | xargs grep -l 'NavController' | head -20Repository: openMF/android-client
Length of output: 2264
🏁 Script executed:
#!/bin/bash
# Search for Screen function signatures with grep
echo "=== Sample Screen composables with NavController ==="
grep -n "fun.*Screen.*NavController" ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/charges/ChargesScreen.kt | head -10
grep -n "fun.*Screen.*NavController" ./feature/client/src/commonMain/kotlin/com/mifos/feature/client/clientProfile/ClientProfileScreen.kt | head -10Repository: openMF/android-client
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Check MifosBreadcrumbNavBar definition and usage
echo "=== MifosBreadcrumbNavBar definition ==="
find . -name '*.kt' -type f ! -path '*/build/*' | xargs grep -l 'fun MifosBreadcrumbNavBar' | head -1Repository: openMF/android-client
Length of output: 185
🏁 Script executed:
#!/bin/bash
# Get the definition of MifosBreadcrumbNavBar
find . -name '*.kt' -type f ! -path '*/build/*' -exec grep -l 'fun MifosBreadcrumbNavBar' {} \; | head -1 | xargs catRepository: openMF/android-client
Length of output: 5282
🏁 Script executed:
#!/bin/bash
# Check how many screens in the codebase have NavController as a parameter
echo "=== Count of Screen files with NavController parameter ==="
find . -name '*Screen.kt' -type f ! -path '*/build/*' | while read file; do
if grep -q 'fun.*Screen.*NavController' "$file"; then
echo "$file: YES"
else
echo "$file: NO"
fi
done | head -30Repository: openMF/android-client
Length of output: 3354
Remove NavController parameter and use standard pattern for accessing navigation.
Most Screen composables in this project do not accept NavController as a parameter. While MifosBreadcrumbNavBar requires NavController internally, examine how other screens in the codebase obtain and provide it—this may be through a ViewModel, state management, or local composition. Align this implementation with the project's established Screen pattern.
Also applies to: 149-149, 166-166
🤖 Prompt for AI Agents
In
feature/loan/src/commonMain/kotlin/com/mifos/feature/loan/loanAccountSummary/LoanAccountSummaryScreen.kt
around lines 122, 149 and 166, remove the NavController parameter from the
Screen composable signature and all callers, and instead obtain the
NavController inside the composable using the project's standard
composition-local/provider pattern (for example use the project's
LocalNavController.current or equivalent provider that other Screens use).
Replace direct parameter usage by a locally retrieved navController when calling
MifosBreadcrumbNavBar and any internal navigation, update imports accordingly,
and ensure all call sites no longer pass NavController.
3 participants
Fixes - Jira-MIFOSAC-615
Please make sure these boxes are checked before submitting your pull request - thanks!
Run the static analysis check
./gradlew checkorci-prepush.shto make sure you didn't break anythingIf you have multiple commits please combine them into one commit by squashing them.
Summary by CodeRabbit
New Features
UI Improvements
Behavior
✏️ Tip: You can customize this high-level summary in your review settings.